Refactor CVE fix skill into shell scripts#2383
Refactor CVE fix skill into shell scripts#2383dfarrell07 wants to merge 3 commits intosubmariner-io:develfrom
Conversation
|
🤖 Created branch: z_pr2383/dfarrell07/cve-fix-skill-redesign |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a CVE remediation automation suite: detection, grype-based scanning, per-package and stdlib fixers, ignore management, orchestration (fix-all), review integration (agent prompt), CI/test wiring, skill/docs/commit-template updates, and unit/integration tests. All changes are scripts/docs/CI/tests; no exported library APIs changed. ChangesCVE remediation automation
Sequence Diagram(s)sequenceDiagram
actor User
participant FixAll as Fix-All
participant Detect as Detect
participant Grype as Grype
participant Fixer as Fixers
participant Git as Git
participant Review as Review/Agent
User->>FixAll: run fix-all.sh REPO BRANCH (or Make target)
FixAll->>Detect: run --setup-branch
Detect->>Git: validate repo, fetch, create/check out fix branch
Detect-->>FixAll: return STATE_FILE
FixAll->>Grype: run initial scan (--json)
Grype-->>FixAll: scan results
alt CVEs found
FixAll->>FixAll: group CVEs by package
loop per package
FixAll->>Fixer: invoke fix-package or fix-stdlib
Fixer->>Git: run go commands, commit or rollback
Fixer-->>FixAll: FIXED or NEEDS_REVIEW
end
FixAll->>Grype: run verification scan
Grype-->>FixAll: updated results
FixAll->>Review: run review.sh with FIX_SUMMARY and scan
Review->>Grype: optionally run locate/scan evidence
Review->>Review: render prompt and invoke agent (if available)
Review-->>FixAll: review summary (FIXED/IGNORED/UNRESOLVED)
else No CVEs
FixAll-->>User: report clean, delete fix branch
end
FixAll-->>User: print commits + git push / gh pr create guidance, exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 13 minutes and 34 seconds.Comment |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further |
dfarrell07
force-pushed
the
cve-fix-skill-redesign
branch
3 times, most recently
from
43aea4b to
43f1d40
Compare
dfarrell07
requested review from
Oats87,
mkolesnik,
skitt,
sridhargaddam,
tpantelis and
vthapar
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 68-70: The Makefile target cve-fix currently invokes
./scripts/cve/fix-all.sh with unquoted expansions of $(or $(REPO),.) and $(or
$(BRANCH),$(shell git branch --show-current)), which will split paths containing
spaces; update the cve-fix recipe to pass quoted arguments to fix-all.sh (wrap
the two expansions in double quotes) so REPO and BRANCH overrides are forwarded
as single arguments to ./scripts/cve/fix-all.sh.
In `@scripts/cve/detect.sh`:
- Around line 137-153: The heredoc that writes the sourced state file in
scripts/cve/detect.sh currently injects raw variable values (STATE_FILE, REPO,
BRANCH, FIX_BRANCH, ORIGINAL_REF, HAS_TOOLS_GOMOD, GENERATED_FILE,
DIFF_IGNORE_ARGS, NEEDS_BUILD_FOR_SCAN, CONTAINER_CMD, HAS_LOCAL_GRYPE,
SHIPYARD_IMAGE, SHIPYARD_GO_VERSION, FETCH_FAILED, CVE_SCRIPTS) unescaped, so
values containing quotes or command substitution can break or be executed when
the file is sourced; change the writer to shell-escape each value before
emitting it (for example, generate each line using a safe escape routine such as
printf '%q' or by safely single-quoting and escaping internal single quotes) and
write the escaped results into "$STATE_FILE" instead of the raw expansions in
the heredoc so the resulting file can be safely sourced by scan.sh, fix-all.sh,
review.sh, etc.
In `@scripts/cve/fix-all.sh`:
- Around line 145-152: The final post-fix scan and agent review results are
being ignored when deciding the script exit status; ensure the exit code
reflects unresolved CVEs or review failures by capturing the exit/status of
"$SCRIPT_DIR/scan.sh" "$STATE_FILE" (post-fix scan) and "$SCRIPT_DIR/review.sh"
"$STATE_FILE" "$FIX_SUMMARY", and combine them with the existing REVIEW_COUNT
logic so the script exits non-zero if the post-fix scan reports matches or if
review.sh returns non-zero; update handling around the scan.sh call and the
review.sh invocation (also mirror the same fix where similar logic appears
around lines 196-199) to aggregate their exit codes into the final exit
decision.
- Around line 78-83: The current fixedIn selection uses lexicographic sort
(map(.fixedIn) | sort | last) which misorders semantic versions; replace it with
a version-aware sort, e.g. compute a sortable key by stripping a leading "v",
splitting on dots and converting numeric segments, and then use sort_by(... ) |
last; update the fixedIn expression inside the map block (the object with keys
pkg,type,fixedIn,cves) so it uses something like (map(.fixedIn) |
sort_by(gsub("^v";"") | split(".") | map(tonumber? // 0)) | last) to pick the
true highest semantic version.
- Around line 11-17: The cleanup function currently always runs git checkout --
. and can discard the caller's uncommitted edits; change it to be conditional:
before making any repo changes, record the repo's initial dirty state (e.g., via
git status --porcelain or git ls-files -m) and set a flag (e.g., INITIAL_CLEAN)
or write that list to STATE_FILE; then update cleanup (and the trap) to only
revert files that the script created/modified (or only run git checkout if the
repo was initially clean) rather than unconditionally running git checkout -- .;
ensure you still remove STATE_FILE in cleanup. Reference: cleanup, trap EXIT,
STATE_FILE.
In `@scripts/cve/fix-package.sh`:
- Line 40: K8S_BEFORE currently only reads the top-level go.mod; instead, scan
all go.mod files recursively and record the module path alongside its k8s.io
version so you can compare per-module after the update. Update the logic that
builds GO_BEFORE to produce a list of module=>version pairs (use each module's
"module" line and grep 'k8s.io/' to extract the minor version) and store that as
K8S_BEFORE; do the same after the change to produce K8S_AFTER, then compare
versions for the same module names to detect breaking minor upgrades; update any
checks that reference K8S_BEFORE/K8S_AFTER and GO_BEFORE to iterate module keys
rather than assuming a single root module.
- Around line 45-50: Replace the fragile grep-based check that looks for
"$PACKAGE" in "$GOMOD" with a structured module inspection using Go's module
JSON output (e.g., run go list -m -json all or go mod edit -json and parse the
"Require" and "Replace" fields) to determine whether the package is required and
whether its required version is still vulnerable; use the parsed Require entry
for the post-update success test (and ensure Replace entries do not mask a
still-vulnerable require) before reporting FIXED. Update the logic around the
PACKAGE/GOMOD/MODDIR/VERSION blocks (both the instance shown and the similar
block at lines 91-100) so the script programmatically checks the module JSON for
the package name and version rather than grepping the go.mod text.
- Around line 108-109: The pipeline "git diff --name-only | grep -E
'go\.(mod|sum)$' | xargs -r git add" can fail under set -euo pipefail when there
are no matches; change the logic in scripts/cve/fix-package.sh to first collect
matches (e.g., into a variable or array using git diff --name-only and grep -E
'go\.(mod|sum)$'), then check if that collection is non-empty before calling git
add (or skip/add a no-op) so the script does not abort; update the block that
currently contains that pipeline to perform the collect-check-add flow (or
explicitly guard the no-match case) instead of relying on the pipeline to be
safe.
In `@scripts/cve/fix-stdlib.sh`:
- Around line 59-60: Replace the current staging pipeline that uses only "git
diff --name-only | grep -E 'go\\.(mod|sum)$' | xargs -r git add" with a command
that also includes untracked files; e.g. combine tracked-changes and untracked
lists by using "git diff --name-only" and "git ls-files -o --exclude-standard",
filter both with grep -E 'go\\.(mod|sum)$', then pipe to xargs -r git add so
newly created go.sum files are staged; update the line in
scripts/cve/fix-stdlib.sh that currently contains the git diff pipeline to use
this combined approach.
In `@scripts/cve/lib.sh`:
- Around line 120-139: The insert_grype_ignore function currently always inserts
a new YAML block, causing duplicates; change it to be idempotent by first
searching the target file for an existing ignore block that matches both the
cve_id and package (e.g., matching the lines "vulnerability: $cve_id" and "name:
$package") and, if found, replace that existing block with the new entry instead
of appending; otherwise fall back to the current insertion logic. Update the
logic inside insert_grype_ignore to locate the start/end of the matching block
(or use a multiline grep/awk/sed pattern), perform an in-place replacement of
that block with the constructed $entry when present, and only insert a new block
when no match exists, ensuring no duplicate entries for the same CVE+package
pair.
In `@scripts/cve/locate.sh`:
- Line 20: Change the grep invocations that search module/package variables to
use fixed-string matching: replace uses of grep -q "$PACKAGE" "$GOMOD" (and the
other grep calls that use PACKAGE or package-derived patterns) with grep -F --
"$PACKAGE" "$GOMOD" 2>/dev/null (and similarly add -F -- for the grep calls at
the other occurrences mentioned) so dots in module paths are treated as
literals; ensure you update all grep calls referencing the PACKAGE variable (and
any package-derived patterns) in this script.
In `@scripts/cve/review.sh`:
- Around line 33-35: NEEDS_REVIEW_PKGS loop is passing the literal "stdlib" into
locate.sh which produces spurious errors; update the loop in review.sh that
iterates NEEDS_REVIEW_PKGS to special-case the token "stdlib" (from fix-all.sh)
so it does not call locate.sh for that entry—either skip it or append a
dedicated message containing the stdlib-specific context (e.g., Go version
transition) to LOCATE_OUTPUT; reference the variables/commands in the diff
(NEEDS_REVIEW_PKGS, PKG, LOCATE_OUTPUT, STATE_FILE, and locate.sh) when making
the change.
- Around line 58-63: The review phase currently invokes the Claude agent with
Bash execution effectively enabled via the flags in the claude invocation (the
OUTPUT assignment calling claude with --allowedTools "Bash" and
--dangerously-skip-permissions), and the review prompt invites mutation
commands; to fix this remove mutate-inviting commands from review-prompt.md (the
lines that suggest running fix-package.sh, fix-stdlib.sh, ignore.sh) and change
the claude invocation that sets OUTPUT to not permit Bash by default (remove
--allowedTools "Bash" and drop --dangerously-skip-permissions), and instead
require an explicit opt-in flag (e.g., --allow-agent-mutations) before enabling
Bash in the claude call; update any gating logic that reads that flag so review
remains read-only unless explicitly opted into mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1793d672-bd0c-426d-aa5d-66b3cbb499ad
📒 Files selected for processing (17)
.agents/commit-templates.md.agents/workflows/cve-fix.md.github/workflows/testing.ymlMakefilescripts/cve/detect.shscripts/cve/fix-all.shscripts/cve/fix-package.shscripts/cve/fix-stdlib.shscripts/cve/ignore.shscripts/cve/lib.shscripts/cve/locate.shscripts/cve/review-prompt.mdscripts/cve/review.shscripts/cve/scan.shscripts/cve/test-lib.shskills/cve-fix/SKILL.mdtest/scripts/cve/test.sh
| cat > "$STATE_FILE" <<EOF | ||
| STATE_FILE="$STATE_FILE" | ||
| REPO="$REPO" | ||
| BRANCH="$BRANCH" | ||
| FIX_BRANCH="$FIX_BRANCH" | ||
| ORIGINAL_REF="$ORIGINAL_REF" | ||
| HAS_TOOLS_GOMOD=$HAS_TOOLS_GOMOD | ||
| GENERATED_FILE="$GENERATED_FILE" | ||
| DIFF_IGNORE_ARGS="$DIFF_IGNORE_ARGS" | ||
| NEEDS_BUILD_FOR_SCAN=$NEEDS_BUILD_FOR_SCAN | ||
| CONTAINER_CMD="$CONTAINER_CMD" | ||
| HAS_LOCAL_GRYPE=$HAS_LOCAL_GRYPE | ||
| SHIPYARD_IMAGE="$SHIPYARD_IMAGE" | ||
| SHIPYARD_GO_VERSION="$SHIPYARD_GO_VERSION" | ||
| FETCH_FAILED=$FETCH_FAILED | ||
| CVE_SCRIPTS="$SCRIPT_DIR" | ||
| EOF |
There was a problem hiding this comment.
Shell-escape values before writing the sourced state file.
This file is later sourced by scan.sh, fix-all.sh, review.sh, and others. Writing raw values inside double quotes means a repo path or branch containing ", backticks, or $(...) can break parsing or execute commands when the state file is loaded.
🛡️ Proposed hardening
STATE_FILE=$(state_file_path "$REPO" "$BRANCH")
-cat > "$STATE_FILE" <<EOF
-STATE_FILE="$STATE_FILE"
-REPO="$REPO"
-BRANCH="$BRANCH"
-FIX_BRANCH="$FIX_BRANCH"
-ORIGINAL_REF="$ORIGINAL_REF"
-HAS_TOOLS_GOMOD=$HAS_TOOLS_GOMOD
-GENERATED_FILE="$GENERATED_FILE"
-DIFF_IGNORE_ARGS="$DIFF_IGNORE_ARGS"
-NEEDS_BUILD_FOR_SCAN=$NEEDS_BUILD_FOR_SCAN
-CONTAINER_CMD="$CONTAINER_CMD"
-HAS_LOCAL_GRYPE=$HAS_LOCAL_GRYPE
-SHIPYARD_IMAGE="$SHIPYARD_IMAGE"
-SHIPYARD_GO_VERSION="$SHIPYARD_GO_VERSION"
-FETCH_FAILED=$FETCH_FAILED
-CVE_SCRIPTS="$SCRIPT_DIR"
-EOF
+{
+ printf 'STATE_FILE=%q\n' "$STATE_FILE"
+ printf 'REPO=%q\n' "$REPO"
+ printf 'BRANCH=%q\n' "$BRANCH"
+ printf 'FIX_BRANCH=%q\n' "$FIX_BRANCH"
+ printf 'ORIGINAL_REF=%q\n' "$ORIGINAL_REF"
+ printf 'HAS_TOOLS_GOMOD=%q\n' "$HAS_TOOLS_GOMOD"
+ printf 'GENERATED_FILE=%q\n' "$GENERATED_FILE"
+ printf 'DIFF_IGNORE_ARGS=%q\n' "$DIFF_IGNORE_ARGS"
+ printf 'NEEDS_BUILD_FOR_SCAN=%q\n' "$NEEDS_BUILD_FOR_SCAN"
+ printf 'CONTAINER_CMD=%q\n' "$CONTAINER_CMD"
+ printf 'HAS_LOCAL_GRYPE=%q\n' "$HAS_LOCAL_GRYPE"
+ printf 'SHIPYARD_IMAGE=%q\n' "$SHIPYARD_IMAGE"
+ printf 'SHIPYARD_GO_VERSION=%q\n' "$SHIPYARD_GO_VERSION"
+ printf 'FETCH_FAILED=%q\n' "$FETCH_FAILED"
+ printf 'CVE_SCRIPTS=%q\n' "$SCRIPT_DIR"
+} > "$STATE_FILE"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cat > "$STATE_FILE" <<EOF | |
| STATE_FILE="$STATE_FILE" | |
| REPO="$REPO" | |
| BRANCH="$BRANCH" | |
| FIX_BRANCH="$FIX_BRANCH" | |
| ORIGINAL_REF="$ORIGINAL_REF" | |
| HAS_TOOLS_GOMOD=$HAS_TOOLS_GOMOD | |
| GENERATED_FILE="$GENERATED_FILE" | |
| DIFF_IGNORE_ARGS="$DIFF_IGNORE_ARGS" | |
| NEEDS_BUILD_FOR_SCAN=$NEEDS_BUILD_FOR_SCAN | |
| CONTAINER_CMD="$CONTAINER_CMD" | |
| HAS_LOCAL_GRYPE=$HAS_LOCAL_GRYPE | |
| SHIPYARD_IMAGE="$SHIPYARD_IMAGE" | |
| SHIPYARD_GO_VERSION="$SHIPYARD_GO_VERSION" | |
| FETCH_FAILED=$FETCH_FAILED | |
| CVE_SCRIPTS="$SCRIPT_DIR" | |
| EOF | |
| { | |
| printf 'STATE_FILE=%q\n' "$STATE_FILE" | |
| printf 'REPO=%q\n' "$REPO" | |
| printf 'BRANCH=%q\n' "$BRANCH" | |
| printf 'FIX_BRANCH=%q\n' "$FIX_BRANCH" | |
| printf 'ORIGINAL_REF=%q\n' "$ORIGINAL_REF" | |
| printf 'HAS_TOOLS_GOMOD=%q\n' "$HAS_TOOLS_GOMOD" | |
| printf 'GENERATED_FILE=%q\n' "$GENERATED_FILE" | |
| printf 'DIFF_IGNORE_ARGS=%q\n' "$DIFF_IGNORE_ARGS" | |
| printf 'NEEDS_BUILD_FOR_SCAN=%q\n' "$NEEDS_BUILD_FOR_SCAN" | |
| printf 'CONTAINER_CMD=%q\n' "$CONTAINER_CMD" | |
| printf 'HAS_LOCAL_GRYPE=%q\n' "$HAS_LOCAL_GRYPE" | |
| printf 'SHIPYARD_IMAGE=%q\n' "$SHIPYARD_IMAGE" | |
| printf 'SHIPYARD_GO_VERSION=%q\n' "$SHIPYARD_GO_VERSION" | |
| printf 'FETCH_FAILED=%q\n' "$FETCH_FAILED" | |
| printf 'CVE_SCRIPTS=%q\n' "$SCRIPT_DIR" | |
| } > "$STATE_FILE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/detect.sh` around lines 137 - 153, The heredoc that writes the
sourced state file in scripts/cve/detect.sh currently injects raw variable
values (STATE_FILE, REPO, BRANCH, FIX_BRANCH, ORIGINAL_REF, HAS_TOOLS_GOMOD,
GENERATED_FILE, DIFF_IGNORE_ARGS, NEEDS_BUILD_FOR_SCAN, CONTAINER_CMD,
HAS_LOCAL_GRYPE, SHIPYARD_IMAGE, SHIPYARD_GO_VERSION, FETCH_FAILED, CVE_SCRIPTS)
unescaped, so values containing quotes or command substitution can break or be
executed when the file is sourced; change the writer to shell-escape each value
before emitting it (for example, generate each line using a safe escape routine
such as printf '%q' or by safely single-quoting and escaping internal single
quotes) and write the escaped results into "$STATE_FILE" instead of the raw
expansions in the heredoc so the resulting file can be safely sourced by
scan.sh, fix-all.sh, review.sh, etc.
There was a problem hiding this comment.
Won't fix. All values are controlled (paths from pwd, git branch names, hardcoded strings like docker/true/false). No user-supplied input reaches the state file.
There was a problem hiding this comment.
@dfarrell07, fair point — looking at the concrete values: pwd-derived paths, git ref names (which cannot contain " or $()), true/false literals, hardcoded image strings, and go version output. None of these realistically carry shell-metacharacter payloads, so the heredoc approach is safe in practice here. Acknowledged, moving on.
😔
| # Clean up on exit: revert uncommitted changes and remove state file | ||
| # shellcheck disable=SC2317,SC2329 # invoked via trap EXIT | ||
| cleanup() { | ||
| git checkout -- . 2>/dev/null || true | ||
| rm -f "${STATE_FILE:-}" | ||
| } | ||
| trap cleanup EXIT |
There was a problem hiding this comment.
Don't reset the caller's working tree on exit.
The EXIT trap always runs git checkout -- ., but there is no clean-tree precondition before the script creates or switches to a fix branch. Any tracked edits the caller already had in the repo can be discarded on both success and failure.
🛡️ Safer guardrail
+git diff --quiet && git diff --cached --quiet || {
+ echo "ERROR: Working tree must be clean before running fix-all.sh" >&2
+ exit 1
+}
+
# --- Phase 1: Deterministic ---
banner "Phase 1: Detect and Scan"Also applies to: 24-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/fix-all.sh` around lines 11 - 17, The cleanup function currently
always runs git checkout -- . and can discard the caller's uncommitted edits;
change it to be conditional: before making any repo changes, record the repo's
initial dirty state (e.g., via git status --porcelain or git ls-files -m) and
set a flag (e.g., INITIAL_CLEAN) or write that list to STATE_FILE; then update
cleanup (and the trap) to only revert files that the script created/modified (or
only run git checkout if the repo was initially clean) rather than
unconditionally running git checkout -- .; ensure you still remove STATE_FILE in
cleanup. Reference: cleanup, trap EXIT, STATE_FILE.
There was a problem hiding this comment.
Fixed differently. Added a precondition check in detect.sh that refuses to run on a dirty working tree (git diff --quiet && git diff --cached --quiet), preventing the cleanup trap from ever discarding uncommitted work.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "git diff" scripts/cve/detect.sh
Length of output: 89
@dfarrell07, confirmed — Line 111 of detect.sh has the dirty-tree guard:
if ! git diff --quiet 2>/dev/null || ! git diff --cached --quiet 2>/dev/null;That's a clean approach: failing fast at entry is simpler and more robust than trying to track and conditionally revert only script-modified files. The concern is fully addressed.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| echo "" | ||
| echo "Final scan..." | ||
| "$SCRIPT_DIR/scan.sh" "$STATE_FILE" 2>/dev/null || true | ||
|
|
||
| # --- Phase 2: Agent Review --- | ||
|
|
||
| banner "Phase 2: Agent Review" | ||
| "$SCRIPT_DIR/review.sh" "$STATE_FILE" "$FIX_SUMMARY" || true |
There was a problem hiding this comment.
Final success/failure ignores the post-fix scan.
The script runs a final scan and the agent review, but the exit code still depends only on the earlier REVIEW_COUNT. If every fix script returns 0 yet Grype still reports matches, this path still exits 0, which does not match the documented contract for unresolved CVEs.
Also applies to: 196-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/fix-all.sh` around lines 145 - 152, The final post-fix scan and
agent review results are being ignored when deciding the script exit status;
ensure the exit code reflects unresolved CVEs or review failures by capturing
the exit/status of "$SCRIPT_DIR/scan.sh" "$STATE_FILE" (post-fix scan) and
"$SCRIPT_DIR/review.sh" "$STATE_FILE" "$FIX_SUMMARY", and combine them with the
existing REVIEW_COUNT logic so the script exits non-zero if the post-fix scan
reports matches or if review.sh returns non-zero; update handling around the
scan.sh call and the review.sh invocation (also mirror the same fix where
similar logic appears around lines 196-199) to aggregate their exit codes into
the final exit decision.
There was a problem hiding this comment.
Won't fix. By design, the exit code reflects the deterministic phase. The agent review may fix additional CVEs, but parsing its output would be fragile. Exit 2 means "human should check," which is correct when there were unresolved CVEs.
There was a problem hiding this comment.
@dfarrell07, understood — that's a clear and sensible design boundary. Keeping the exit code tied to the deterministic phase avoids brittleness from parsing agent output, and exit 2 correctly signals "human should review" for anything the deterministic phase couldn't resolve. Thanks for the explanation.
✏️ Learnings added
Learnt from: dfarrell07
Repo: submariner-io/shipyard PR: 2383
File: scripts/cve/fix-all.sh:0-0
Timestamp: 2026-05-02T04:41:18.760Z
Learning: In `scripts/cve/fix-all.sh` in the submariner-io/shipyard repo, the script exit code is intentionally based only on the deterministic phase (REVIEW_COUNT from fix-stdlib.sh/fix-package.sh). The post-fix scan and agent review (review.sh) are non-fatal by design. Exit 0 = clean, exit 1 = scan error, exit 2 = unresolved CVEs needing human review. Parsing agent review output for exit codes was deliberately avoided as fragile.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| insert_grype_ignore() { | ||
| local file="$1" cve_id="$2" package="$3" reason="$4" | ||
| local entry | ||
| entry=" # $reason | ||
| - vulnerability: $cve_id | ||
| package: | ||
| name: $package" | ||
|
|
||
| if grep -qn '^exclude:' "$file" 2>/dev/null; then | ||
| local exclude_line | ||
| exclude_line=$(grep -n '^exclude:' "$file" | head -1 | cut -d: -f1) | ||
| { | ||
| head -n "$((exclude_line - 1))" "$file" | ||
| printf '%s\n' "$entry" | ||
| tail -n +"$exclude_line" "$file" | ||
| } > "${file}.tmp" | ||
| mv "${file}.tmp" "$file" | ||
| else | ||
| printf '\n%s\n' "$entry" >> "$file" | ||
| fi |
There was a problem hiding this comment.
Make ignore insertion idempotent.
Re-running the ignore flow for the same CVE will append a second YAML block instead of updating the existing one. That keeps .grype.yaml noisy and makes repeated review passes harder to reason about.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/lib.sh` around lines 120 - 139, The insert_grype_ignore function
currently always inserts a new YAML block, causing duplicates; change it to be
idempotent by first searching the target file for an existing ignore block that
matches both the cve_id and package (e.g., matching the lines "vulnerability:
$cve_id" and "name: $package") and, if found, replace that existing block with
the new entry instead of appending; otherwise fall back to the current insertion
logic. Update the logic inside insert_grype_ignore to locate the start/end of
the matching block (or use a multiline grep/awk/sed pattern), perform an
in-place replacement of that block with the constructed $entry when present, and
only insert a new block when no match exists, ensuring no duplicate entries for
the same CVE+package pair.
There was a problem hiding this comment.
Won't fix. insert_grype_ignore is called once per CVE by ignore.sh. The orchestrator tracks what's been fixed/ignored, so duplicate calls don't happen in practice.
There was a problem hiding this comment.
@dfarrell07, understood — since fix-all.sh tracks per-CVE outcomes and drives ignore.sh exactly once per CVE, the orchestration layer already provides the idempotency guarantee. I'll drop the concern.
✏️ Learnings added
Learnt from: dfarrell07
Repo: submariner-io/shipyard PR: 2383
File: scripts/cve/lib.sh:120-139
Timestamp: 2026-05-02T04:41:55.538Z
Learning: In `scripts/cve/lib.sh` (submariner-io/shipyard), `insert_grype_ignore` is intentionally non-idempotent at the function level. Duplicate-call prevention is enforced by the orchestrator (`scripts/cve/fix-all.sh`), which tracks each CVE's fix/ignore outcome and invokes `ignore.sh` (and therefore `insert_grype_ignore`) exactly once per CVE. Do not flag the function itself for lacking idempotency checks.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Find in all go.mod files | ||
| FOUND_IN="" | ||
| while IFS= read -r GOMOD; do | ||
| if grep -q "$PACKAGE" "$GOMOD" 2>/dev/null; then |
There was a problem hiding this comment.
Use fixed-string matching for package paths.
These grep calls treat . in module paths as regex wildcards, so the locator can report unrelated go.mod hits or graph edges. Switching package-derived matches to grep -F -- keeps the review evidence deterministic.
Also applies to: 35-35, 50-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/locate.sh` at line 20, Change the grep invocations that search
module/package variables to use fixed-string matching: replace uses of grep -q
"$PACKAGE" "$GOMOD" (and the other grep calls that use PACKAGE or
package-derived patterns) with grep -F -- "$PACKAGE" "$GOMOD" 2>/dev/null (and
similarly add -F -- for the grep calls at the other occurrences mentioned) so
dots in module paths are treated as literals; ensure you update all grep calls
referencing the PACKAGE variable (and any package-derived patterns) in this
script.
There was a problem hiding this comment.
Won't fix. Go module paths (github.com/docker/docker, k8s.io/client-go) don't contain regex metacharacters. grep -q is safe and readable.
There was a problem hiding this comment.
@dfarrell07, understood — Go module paths follow a well-defined format where the . in domain segments (e.g. github.com, k8s.io) won't produce harmful regex expansions in practice. I'll accept the decision.
| NEEDS_REVIEW_PKGS=$(echo "$FIX_SUMMARY" | grep "^NEEDS_REVIEW:" | sed 's/NEEDS_REVIEW: \([^ ]*\).*/\1/' || true) | ||
| for PKG in $NEEDS_REVIEW_PKGS; do | ||
| LOCATE_OUTPUT+="$(bash "$SCRIPT_DIR/locate.sh" "$STATE_FILE" "$PKG" 2>&1 || true)" |
There was a problem hiding this comment.
Handle stdlib review items separately.
fix-all.sh records stdlib failures as NEEDS_REVIEW: stdlib ..., but locate.sh only knows how to search module/package references. Passing stdlib here just appends a guaranteed error to the prompt and drops the useful context for this case, like the Go version transition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/review.sh` around lines 33 - 35, NEEDS_REVIEW_PKGS loop is
passing the literal "stdlib" into locate.sh which produces spurious errors;
update the loop in review.sh that iterates NEEDS_REVIEW_PKGS to special-case the
token "stdlib" (from fix-all.sh) so it does not call locate.sh for that
entry—either skip it or append a dedicated message containing the
stdlib-specific context (e.g., Go version transition) to LOCATE_OUTPUT;
reference the variables/commands in the diff (NEEDS_REVIEW_PKGS, PKG,
LOCATE_OUTPUT, STATE_FILE, and locate.sh) when making the change.
There was a problem hiding this comment.
Won't fix. The review agent receives all items in FIX_SUMMARY and handles them uniformly. It understands both stdlib and package types from context.
There was a problem hiding this comment.
@dfarrell07, understood — since the Claude subagent consumes the full FIX_SUMMARY and can reason about stdlib vs. package entries from that context, the spurious locate.sh error output is handled gracefully. Thanks for the clarification!
😔 🐇
| OUTPUT=$(claude -p "$PROMPT" \ | ||
| --print \ | ||
| --model sonnet \ | ||
| --allowedTools "Bash" \ | ||
| --dangerously-skip-permissions \ | ||
| 2>&1) || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f scripts/cve | head -20Repository: submariner-io/shipyard
Length of output: 416
🏁 Script executed:
cat -n scripts/cve/review.sh | head -100Repository: submariner-io/shipyard
Length of output: 2673
🏁 Script executed:
cat -n scripts/cve/fix-all.sh | head -150Repository: submariner-io/shipyard
Length of output: 5688
🏁 Script executed:
tail -n +150 scripts/cve/fix-all.sh | head -50Repository: submariner-io/shipyard
Length of output: 1664
🏁 Script executed:
cat scripts/cve/locate.sh | head -80Repository: submariner-io/shipyard
Length of output: 1622
🏁 Script executed:
ls -la scripts/cve/Repository: submariner-io/shipyard
Length of output: 841
🌐 Web query:
claude CLI --allowedTools Bash --dangerously-skip-permissions security
💡 Result:
The command “claude CLI --allowedTools Bash --dangerously-skip-permissions” effectively defeats Claude Code’s tool/permission gating and is therefore not meaningfully “secure.” In practice, when bypass mode is enabled, Claude Code can execute Bash even if you attempted to restrict it via --allowedTools, and multiple reports indicate tool allowlists don’t reliably constrain behavior under bypass. What the flags do (security-relevant): 1) --dangerously-skip-permissions enables bypassPermissions / bypassPermissions mode, which skips all permission prompts. Anthropic’s permissions docs state bypassPermissions skips permission prompts and should only be used in isolated environments like containers or VMs; removals targeting / or ~ still prompt as a circuit breaker. 2) --allowedTools is supposed to “allow specific tools without prompting.” However, community-reported bugs show that when bypassPermissions is active, --allowedTools does not restrict tool availability as intended (Bash can run even when not allowlisted). Another report explicitly calls out that bypassPermissions can also bypass sandbox protections. Concrete implications for your exact query: - Allowlisting only “Bash” while also enabling --dangerously-skip-permissions does not provide a safe, non-interactive “Bash-only but constrained” posture. With bypassPermissions, there are reports that tool restrictions via allowlists are ignored, and that sandbox/extra isolation may be reduced. - Multiple bypass-mode issues have been filed, including: a) --allowedTools seemingly ignored with permission-mode bypassPermissions. b) auto-approval of Bash commands even without Bash being in an allow list. c) a general bug report claiming allowedTools has no effect when permission bypass flags are active. Recommended secure usage patterns: - Do not use --dangerously-skip-permissions on a host machine. Use it only inside strong isolation (e.g., a container/VM) as Anthropic explicitly recommends. - Prefer permission modes that do not skip prompts (default/acceptEdits/plan/dontAsk) and use allow/deny rules for policy. - If you need non-interactive automation, use a locked-down allowlist policy (permissions.allow / permissions.deny) plus isolation, and consider additional external enforcement layers rather than relying solely on Claude Code’s bypass behavior (community mitigations describe external approval gates). Practical takeaway: Treat --dangerously-skip-permissions as a “trust the entire environment” switch, not a “trust but restrict to Bash” switch. If you need security, rely on isolation + deny-by-default policies, not bypassPermissions + allowlists.
Citations:
- 1: https://docs.anthropic.com/en/docs/claude-code/permissions
- 2: https://code.claude.com/docs/en/headless
- 3: Is --allowedTools with --permission-mode bypassPermissions behavior expected? anthropics/claude-code#12232
- 4: Bash commands auto-approved despite not being in allowedTools anthropics/claude-code#25181
- 5: [BUG]
--allowedToolshas no effect when permission bypass flags are active anthropics/claude-code#50303
🏁 Script executed:
# Check if review.sh output is used for any further mutations
grep -n "review.sh\|OUTPUT=" scripts/cve/fix-all.shRepository: submariner-io/shipyard
Length of output: 359
🏁 Script executed:
# Verify that review-prompt.md doesn't instruct the agent to perform mutations
cat scripts/cve/review-prompt.mdRepository: submariner-io/shipyard
Length of output: 1467
Gate Bash execution behind explicit opt-in; the review phase should be read-only.
The prompt in review-prompt.md (lines 44–50) explicitly instructs the Claude agent to run mutation commands (fix-package.sh, fix-stdlib.sh, ignore.sh), yet --dangerously-skip-permissions with --allowedTools does not safely restrict tool use. Anthropic's documentation and community reports confirm that the bypass flag negates tool allowlists, and Bash can execute even when not explicitly allowlisted. This means the agent can make unvetted changes to the working tree during Phase 2 (review), which happens after Phase 1 deterministic fixes and after the cleanup trap is no longer active to revert changes.
Since the review phase pre-fetches all evidence upfront, the agent does not need write access to produce review output. Remove mutation command invitations from the prompt and disable Bash access by default, or require an explicit --allow-agent-mutations flag if review-phase mutations are genuinely needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/review.sh` around lines 58 - 63, The review phase currently
invokes the Claude agent with Bash execution effectively enabled via the flags
in the claude invocation (the OUTPUT assignment calling claude with
--allowedTools "Bash" and --dangerously-skip-permissions), and the review prompt
invites mutation commands; to fix this remove mutate-inviting commands from
review-prompt.md (the lines that suggest running fix-package.sh, fix-stdlib.sh,
ignore.sh) and change the claude invocation that sets OUTPUT to not permit Bash
by default (remove --allowedTools "Bash" and drop
--dangerously-skip-permissions), and instead require an explicit opt-in flag
(e.g., --allow-agent-mutations) before enabling Bash in the claude call; update
any gating logic that reads that flag so review remains read-only unless
explicitly opted into mutations.
There was a problem hiding this comment.
Won't fix. The review agent has Bash access and can run git log/diff if needed. Pre-fetching commit diffs would add more data to an already information-dense prompt.
There was a problem hiding this comment.
@dfarrell07, understood — the lazy-fetch pattern makes sense here. Letting the agent pull git diffs on demand avoids bloating the context with evidence that may not be relevant to the specific CVEs under review.
ʕ •ᴥ•ʔ
✏️ Learnings added
Learnt from: dfarrell07
Repo: submariner-io/shipyard PR: 2383
File: scripts/cve/review.sh:59-64
Timestamp: 2026-05-02T04:41:58.453Z
Learning: In `scripts/cve/review.sh` (submariner-io/shipyard), the Claude subagent is intentionally invoked with `--allowedTools "Bash"` and `--dangerously-skip-permissions`. Bash access is kept so the agent can run `git log`/`git diff` on demand during the review phase; evidence is lazily fetched rather than pre-fetched to avoid bloating the prompt context.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
dfarrell07
force-pushed
the
cve-fix-skill-redesign
branch
from
43f1d40 to
f77172c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/cve-fix/SKILL.md (1)
50-51: 💤 Low valueCleanup command kills all matching processes indiscriminately.
The
pkill -f fix-allanddocker kill $(docker ps -q)commands are aggressive — they will kill all running containers, not just orphaned CVE-related ones. Consider documenting this risk or providing a more targeted cleanup approach.📝 Suggested improvement
-`pkill -f fix-all; docker kill $(docker ps -q)` before reporting. +`pkill -f fix-all; docker ps --filter "ancestor=quay.io/submariner/shipyard-dapper-base" -q | xargs -r docker kill` before reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/cve-fix/SKILL.md` around lines 50 - 51, The documented cleanup commands "pkill -f fix-all" and "docker kill $(docker ps -q)" are overly broad; update SKILL.md to either warn about the risk of killing unrelated processes/containers or replace with targeted alternatives: recommend using a specific pkill pattern or storing and killing the exact PIDs for the fix-all process (reference "pkill -f fix-all"), and for containers suggest filtering by name, label or image (e.g., use docker ps --filter with a project/name/label and docker rm -f on that filtered list instead of "docker kill $(docker ps -q)"); include a short example of the safer filter-based approach and a one-line warning if maintainers keep the global commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@skills/cve-fix/SKILL.md`:
- Around line 50-51: The documented cleanup commands "pkill -f fix-all" and
"docker kill $(docker ps -q)" are overly broad; update SKILL.md to either warn
about the risk of killing unrelated processes/containers or replace with
targeted alternatives: recommend using a specific pkill pattern or storing and
killing the exact PIDs for the fix-all process (reference "pkill -f fix-all"),
and for containers suggest filtering by name, label or image (e.g., use docker
ps --filter with a project/name/label and docker rm -f on that filtered list
instead of "docker kill $(docker ps -q)"); include a short example of the safer
filter-based approach and a one-line warning if maintainers keep the global
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51a4d4d8-ad0f-43fc-b788-c06b1ec26589
📒 Files selected for processing (17)
.agents/commit-templates.md.agents/workflows/cve-fix.md.github/workflows/testing.ymlMakefilescripts/cve/detect.shscripts/cve/fix-all.shscripts/cve/fix-package.shscripts/cve/fix-stdlib.shscripts/cve/ignore.shscripts/cve/lib.shscripts/cve/locate.shscripts/cve/review-prompt.mdscripts/cve/review.shscripts/cve/scan.shscripts/cve/test-lib.shskills/cve-fix/SKILL.mdtest/scripts/cve/test.sh
✅ Files skipped from review due to trivial changes (5)
- .agents/commit-templates.md
- .agents/workflows/cve-fix.md
- .github/workflows/testing.yml
- scripts/cve/locate.sh
- test/scripts/cve/test.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- Makefile
- scripts/cve/review-prompt.md
- scripts/cve/lib.sh
- scripts/cve/fix-stdlib.sh
- scripts/cve/scan.sh
- scripts/cve/fix-all.sh
- scripts/cve/ignore.sh
- scripts/cve/review.sh
- scripts/cve/test-lib.sh
dfarrell07
force-pushed
the
cve-fix-skill-redesign
branch
from
f77172c to
21978d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
scripts/cve/fix-all.sh (1)
87-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
jqsemver sorting now fails on normal Go module versions.
fixedInvalues are typicallyv-prefixed.map(tonumber)throws onv1, so this wholejqprogram falls into the|| echo ""path,CVE_LINESbecomes empty, and the deterministic fixer quietly skips packages that do have available fixes.Small fix
- fixedIn: (map(.fixedIn) | sort_by(split(".") | map(tonumber)) | last), + fixedIn: (map(.fixedIn) | sort_by(gsub("^v"; "") | split(".") | map(tonumber? // 0)) | last),#!/bin/bash set -euo pipefail printf '%s\n' '[{"fixedIn":"v1.10.0"},{"fixedIn":"v1.9.9"}]' \ | jq -r 'map(.fixedIn) | sort_by(split(".") | map(tonumber)) | last'Expected result: the current expression errors on the
vprefix. After the fix above, it should returnv1.10.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cve/fix-all.sh` around lines 87 - 92, The jq semver sort fails because fixedIn values are v-prefixed (e.g., "v1.10.0"); update the sort expression so it strips a leading "v" before splitting and converting to numbers: in the pipeline that builds fixedIn (the map({ pkg: .[0].pkg, type: .[0].type, fixedIn: (map(.fixedIn) | sort_by(... ) | last), cves: map(.cve) }) block) replace the sort_by argument to first remove a leading "v" (e.g. using sub("^v";"") on each fixedIn), then split on "." and map(tonumber) so versions like "v1.10.0" sort correctly and fixedIn returns the expected highest version.
🧹 Nitpick comments (1)
scripts/cve/locate.sh (1)
17-23: ⚡ Quick winUse a Bash array instead of a space-joined string to collect
go.modpaths.
FOUND_INis built with space separators at line 21 and then iterated with unquoted$FOUND_INat line 47. Anygo.modpath containing a space would be split incorrectly. A Bash array handles arbitrary paths safely.♻️ Proposed refactor
-FOUND_IN="" +FOUND_IN=() while IFS= read -r GOMOD; do if grep -q "$PACKAGE" "$GOMOD" 2>/dev/null; then - FOUND_IN="${FOUND_IN:+$FOUND_IN }$GOMOD" + FOUND_IN+=("$GOMOD") fi done < <(find_gomods) if [[ -z "$FOUND_IN" ]]; then +if [[ ${`#FOUND_IN`[@]} -eq 0 ]]; then echo "ERROR: $PACKAGE not found in any go.mod" >&2 exit 1 fi -echo "Found in: $FOUND_IN" +echo "Found in: ${FOUND_IN[*]}"Then for the dependency graph loop:
-for GOMOD in $FOUND_IN; do +for GOMOD in "${FOUND_IN[@]}"; doAlso applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cve/locate.sh` around lines 17 - 23, The script currently builds FOUND_IN as a space-joined string which breaks on paths containing spaces; change FOUND_IN to a Bash array (e.g., initialize FOUND_IN=() and append each match with FOUND_IN+=("$GOMOD") inside the find_gomods read loop) and then iterate over the results with for GOMOD in "${FOUND_IN[@]}" (also update the later dependency graph loop that currently iterates over unquoted $FOUND_IN to use the same "${FOUND_IN[@]}" form); ensure every use of these elements is quoted to preserve paths with spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/cve/fix-all.sh`:
- Around line 149-152: The script currently ignores `make unit` failures and can
still show a successful PR/branch result when `REVIEW_COUNT` is zero; change the
Verify block so the exit status of `make unit` is captured and propagated: run
`make unit`, save its exit code, and if non-zero either increment `REVIEW_COUNT`
(so the script treats it as needing attention) or set a `VERIFICATION_FAILED`
flag and ensure the final success path checks that flag and exits non-zero;
update the logic that prints the final branch/PR instructions to consider this
new flag or the adjusted `REVIEW_COUNT` so test failures are reflected in the
final status.
In `@scripts/cve/fix-package.sh`:
- Around line 40-41: The script uses GNU-only flags (grep -oP and xargs -r)
which fail on BSD/macOS; update the K8S_BEFORE assignment and the other
occurrences to portable POSIX commands: replace grep -oP 'v0\.\K[0-9]+' with a
pipeline like grep -Eo 'v0\.[0-9]+' | sed 's/^v0\.//' (so in the K8S_BEFORE
assignment that calls find_gomods you still grep for 'k8s.io/client-go' but
extract the digit with the grep|sed combo), and replace any xargs -r usage by
guarding the xargs call with an if test that the input is non-empty (e.g.
capture to a variable and run xargs only if [ -n "$VAR" ]), updating the
occurrences referenced in the diff (K8S_BEFORE and the commands at lines flagged
around 75 and 104) so the script runs on BSD/macOS as well.
In `@scripts/cve/locate.sh`:
- Around line 32-35: The current search uses PKG_BASENAME which yields false
positives; change the grep to search the full module path stored in PACKAGE (not
PKG_BASENAME). Update the while-loop/grep invocation that computes HITS (and any
related variables like GOMOD) to use "$PACKAGE" (properly quoted/escaped) as the
search term (e.g., grep "replace.*${PACKAGE}" "$GOMOD") so replace directives
match the full module path rather than just the basename.
---
Duplicate comments:
In `@scripts/cve/fix-all.sh`:
- Around line 87-92: The jq semver sort fails because fixedIn values are
v-prefixed (e.g., "v1.10.0"); update the sort expression so it strips a leading
"v" before splitting and converting to numbers: in the pipeline that builds
fixedIn (the map({ pkg: .[0].pkg, type: .[0].type, fixedIn: (map(.fixedIn) |
sort_by(... ) | last), cves: map(.cve) }) block) replace the sort_by argument to
first remove a leading "v" (e.g. using sub("^v";"") on each fixedIn), then split
on "." and map(tonumber) so versions like "v1.10.0" sort correctly and fixedIn
returns the expected highest version.
---
Nitpick comments:
In `@scripts/cve/locate.sh`:
- Around line 17-23: The script currently builds FOUND_IN as a space-joined
string which breaks on paths containing spaces; change FOUND_IN to a Bash array
(e.g., initialize FOUND_IN=() and append each match with FOUND_IN+=("$GOMOD")
inside the find_gomods read loop) and then iterate over the results with for
GOMOD in "${FOUND_IN[@]}" (also update the later dependency graph loop that
currently iterates over unquoted $FOUND_IN to use the same "${FOUND_IN[@]}"
form); ensure every use of these elements is quoted to preserve paths with
spaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e36ace12-31f7-4fd4-a08b-93c15ee66b23
📒 Files selected for processing (17)
.agents/commit-templates.md.agents/workflows/cve-fix.md.github/workflows/testing.ymlMakefilescripts/cve/detect.shscripts/cve/fix-all.shscripts/cve/fix-package.shscripts/cve/fix-stdlib.shscripts/cve/ignore.shscripts/cve/lib.shscripts/cve/locate.shscripts/cve/review-prompt.mdscripts/cve/review.shscripts/cve/scan.shscripts/cve/test-lib.shskills/cve-fix/SKILL.mdtest/scripts/cve/test.sh
✅ Files skipped from review due to trivial changes (8)
- scripts/cve/review-prompt.md
- test/scripts/cve/test.sh
- .agents/workflows/cve-fix.md
- .agents/commit-templates.md
- scripts/cve/test-lib.sh
- scripts/cve/fix-stdlib.sh
- scripts/cve/detect.sh
- scripts/cve/lib.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/testing.yml
- scripts/cve/scan.sh
- scripts/cve/ignore.sh
- skills/cve-fix/SKILL.md
- scripts/cve/review.sh
| # shellcheck disable=SC2046 # word splitting is intentional (multiple file args) | ||
| K8S_BEFORE=$(grep -h 'k8s.io/client-go' $(find_gomods) 2>/dev/null | grep -oP 'v0\.\K[0-9]+' | sort -un | tr '\n' ' ') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' 'require k8s.io/client-go v0.31.1' | grep -oP 'v0\.\K[0-9]+'
printf '%s\n' foo | xargs -r printf '%s\n'Repository: submariner-io/shipyard
Length of output: 130
🏁 Script executed:
cat -n scripts/cve/fix-package.sh | head -120Repository: submariner-io/shipyard
Length of output: 5342
🏁 Script executed:
#!/bin/bash
# Test 1: sed alternative for grep -oP replacement
printf '%s\n' 'require k8s.io/client-go v0.31.1' | sed -nE 's/.*v0\.([0-9]+).*/\1/p'
echo "---"
# Test 2: mapfile alternative for xargs -r
changed_go_files=()
printf '%s\n' 'go.mod' 'go.sum' | mapfile -t changed_go_files
echo "mapfile exit code: $?"
echo "Array size: ${`#changed_go_files`[@]}"Repository: submariner-io/shipyard
Length of output: 108
GNU-only grep and xargs flags break standalone script on BSD/macOS.
Lines 41, 75 use grep -oP (Perl regex mode) and line 104 uses xargs -r (no-run-if-empty), both GNU-only extensions. On BSD/macOS systems, this causes the script to fail before completing K8s checks or staging updated files, breaking the new non-Dapper cve-fix entrypoint.
Replace with portable POSIX alternatives:
Portable shell-only fixes
-# shellcheck disable=SC2046 # word splitting is intentional (multiple file args)
-K8S_BEFORE=$(grep -h 'k8s.io/client-go' $(find_gomods) 2>/dev/null | grep -oP 'v0\.\K[0-9]+' | sort -un | tr '\n' ' ')
+# shellcheck disable=SC2046 # word splitting is intentional (multiple file args)
+K8S_BEFORE=$(grep -h 'k8s.io/client-go' $(find_gomods) 2>/dev/null | sed -nE 's/.*v0\.([0-9]+).*/\1/p' | sort -un | tr '\n' ' ')
…
-# shellcheck disable=SC2046 # word splitting is intentional (multiple file args)
-K8S_AFTER=$(grep -h 'k8s.io/client-go' $(find_gomods) 2>/dev/null | grep -oP 'v0\.\K[0-9]+' | sort -un | tr '\n' ' ')
+# shellcheck disable=SC2046 # word splitting is intentional (multiple file args)
+K8S_AFTER=$(grep -h 'k8s.io/client-go' $(find_gomods) 2>/dev/null | sed -nE 's/.*v0\.([0-9]+).*/\1/p' | sort -un | tr '\n' ' ')
…
-git diff --name-only | grep -E 'go\.(mod|sum)$' | xargs -r git add || true
+mapfile -t changed_go_files < <(git diff --name-only | grep -E 'go\.(mod|sum)$' || true)
+if ((${`#changed_go_files`[@]})); then
+ git add "${changed_go_files[@]}"
+fiAlso applies to: lines 75, 104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cve/fix-package.sh` around lines 40 - 41, The script uses GNU-only
flags (grep -oP and xargs -r) which fail on BSD/macOS; update the K8S_BEFORE
assignment and the other occurrences to portable POSIX commands: replace grep
-oP 'v0\.\K[0-9]+' with a pipeline like grep -Eo 'v0\.[0-9]+' | sed 's/^v0\.//'
(so in the K8S_BEFORE assignment that calls find_gomods you still grep for
'k8s.io/client-go' but extract the digit with the grep|sed combo), and replace
any xargs -r usage by guarding the xargs call with an if test that the input is
non-empty (e.g. capture to a variable and run xargs only if [ -n "$VAR" ]),
updating the occurrences referenced in the diff (K8S_BEFORE and the commands at
lines flagged around 75 and 104) so the script runs on BSD/macOS as well.
There was a problem hiding this comment.
Won't fix. Our CI runs on Linux with GNU grep. The -oP flag is standard there. macOS portability isn't a requirement for this project.
There was a problem hiding this comment.
@dfarrell07, understood — since the project targets Linux CI with GNU grep, flagging grep -oP and xargs -r as portability issues isn't applicable here. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: dfarrell07
Repo: submariner-io/shipyard PR: 2383
File: scripts/cve/fix-package.sh:40-41
Timestamp: 2026-05-02T14:58:58.278Z
Learning: In submariner-io/shipyard CVE fix scripts (scripts/cve/), GNU-only grep flags such as `grep -oP` (Perl regex) and `xargs -r` (no-run-if-empty) are intentional and acceptable. The project's CI runs exclusively on Linux with GNU grep, and macOS/BSD portability is not a requirement. Do not flag these as portability issues in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dfarrell07
Repo: submariner-io/shipyard PR: 2383
File: scripts/cve/fix-stdlib.sh:0-0
Timestamp: 2026-05-02T04:41:52.237Z
Learning: In Submariner/shipyard-style CVE fix scripts, don’t treat untracked `go.sum` files as a concern: these repos already include a `go.sum` in every Go module directory, and `go mod tidy` only updates existing `go.sum` files (it won’t create new ones). Therefore, for staging changes, it’s sufficient to use `git diff --name-only` (or equivalent) to stage modified `go.mod`/`go.sum` files, and avoid flagging any absence/presence of untracked `go.sum` as a false positive.
21978d4 to
0e1ad56
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/cve/fix-package.sh`:
- Around line 63-67: The BREAKING string is being overwritten inside the loop;
change the assignment in the if-block that sets BREAKING for the matching module
(using PAIR, GOMOD, GO_WAS, GO_AFTER) to append the new entry instead of
replacing it — use the existing accumulation idiom like ${BREAKING:+$BREAKING; }
so each match adds "$GOMOD: Go $GO_WAS -> $GO_AFTER" to BREAKING rather than
overwriting previous entries.
- Line 21: The ERR trap only runs "git checkout -- ." which leaves the
index/staged files intact after the script runs (so files added by the later
"git add" remain staged); update the trap command (the line that currently reads
trap 'git checkout -- . 2>/dev/null || true' ERR) to also reset the index by
running a git reset for the working tree before checkout — for example: run "git
reset --quiet -- . 2>/dev/null || true" (or "git reset HEAD -- . 2>/dev/null ||
true") followed by "git checkout -- . 2>/dev/null || true" so failures after the
script's git add/commit steps (see the script's git add and git commit usage)
will unstage and revert files like go.mod/go.sum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c037ab09-2f3c-4de2-98fc-24e595bb24a2
📒 Files selected for processing (17)
.agents/commit-templates.md.agents/workflows/cve-fix.md.github/workflows/testing.ymlMakefilescripts/cve/detect.shscripts/cve/fix-all.shscripts/cve/fix-package.shscripts/cve/fix-stdlib.shscripts/cve/ignore.shscripts/cve/lib.shscripts/cve/locate.shscripts/cve/review-prompt.mdscripts/cve/review.shscripts/cve/scan.shscripts/cve/test-lib.shskills/cve-fix/SKILL.mdtest/scripts/cve/test.sh
✅ Files skipped from review due to trivial changes (6)
- test/scripts/cve/test.sh
- scripts/cve/scan.sh
- .agents/workflows/cve-fix.md
- .agents/commit-templates.md
- scripts/cve/detect.sh
- scripts/cve/review.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/testing.yml
- scripts/cve/locate.sh
- scripts/cve/test-lib.sh
- scripts/cve/ignore.sh
- scripts/cve/review-prompt.md
- scripts/cve/fix-all.sh
- scripts/cve/fix-stdlib.sh
- skills/cve-fix/SKILL.md
- scripts/cve/lib.sh
dfarrell07
force-pushed
the
cve-fix-skill-redesign
branch
from
0e1ad56 to
5295949
Compare
skitt
left a comment
skitt
left a comment
There was a problem hiding this comment.
I’ve started reviewing this but it’s going to take me a while, so I’ll leave comments as I go and let you know once it’s all done.
| SCAN_LOG=$(mktemp) | ||
| "$SCRIPT_DIR/scan.sh" "$STATE_FILE" --fresh --json > "$SCAN_LOG" || true | ||
| SCAN_JSON=$(cat "$SCAN_LOG") | ||
| rm -f "$SCAN_LOG" |
There was a problem hiding this comment.
| SCAN_LOG=$(mktemp) | |
| "$SCRIPT_DIR/scan.sh" "$STATE_FILE" --fresh --json > "$SCAN_LOG" || true | |
| SCAN_JSON=$(cat "$SCAN_LOG") | |
| rm -f "$SCAN_LOG" | |
| SCAN_JSON=$("$SCRIPT_DIR/scan.sh" "$STATE_FILE" --fresh --json || true) |
| MATCH_COUNT=$(printf '%s\n' "$SCAN_JSON" | jq '.matches | length' 2>/dev/null) || MATCH_COUNT="" | ||
| if [[ -z "$MATCH_COUNT" ]]; then | ||
| echo "ERROR: Scan failed or produced invalid output." >&2 | ||
| echo "Check scanner availability (grype or docker/podman)." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
| MATCH_COUNT=$(printf '%s\n' "$SCAN_JSON" | jq '.matches | length' 2>/dev/null) || MATCH_COUNT="" | |
| if [[ -z "$MATCH_COUNT" ]]; then | |
| echo "ERROR: Scan failed or produced invalid output." >&2 | |
| echo "Check scanner availability (grype or docker/podman)." >&2 | |
| exit 1 | |
| fi | |
| if ! MATCH_COUNT=$(printf '%s\n' "$SCAN_JSON" | jq '.matches | length' 2>/dev/null); then | |
| echo "ERROR: Scan failed or produced invalid output." >&2 | |
| echo "Check scanner availability (grype or docker/podman)." >&2 | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Adopted the if ! pattern, thanks. Added || [[ -z "$MATCH_COUNT" ]] because jq exits 0 on empty input with no output, so if ! alone misses the case where scan.sh produces nothing (e.g., no scanner available).
Extract all inline bash from skills/cve-fix/SKILL.md into shellcheck-linted scripts. The SKILL.md becomes a thin wrapper that delegates to fix-all.sh. The deterministic orchestrator (fix-all.sh) handles the full flow: detect config, create fix branch, scan with grype, fix each CVE, verify, commit, run tests. After the deterministic phase, it invokes a Claude subagent (review.sh) to review all outcomes and handle edge cases like breaking changes or persistent CVEs. Add make cve-fix target for standalone use outside the skill. See SEP-0032 for design details. Signed-off-by: Daniel Farrell <dfarrell@redhat.com>
Capture scan.sh output directly instead of writing to a temp file. Use if ! to check jq exit code, with an empty-output guard since jq exits 0 on empty input. Signed-off-by: Daniel Farrell <dfarrell@redhat.com>
dfarrell07
force-pushed
the
cve-fix-skill-redesign
branch
from
5295949 to
ccc0ba9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai What of your feedback is unaddressed? Why haven't you approved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 141548
✅ Confirmed addressed
🤝 Withdrawn (I accepted your explanation / added learning)
|
Signed-off-by: Daniel Farrell <dfarrell@redhat.com>
|
@coderabbitai Both remaining items addressed:
|
🧩 Analysis chainLet me verify the 🏁 Script executed: Length of output: 673
All of my feedback is now addressed or withdrawn. Triggering a fresh review pass. ✅ Actions performedReview triggered.
|
3 participants
Extract all inline bash from skills/cve-fix/SKILL.md into shellcheck-linted scripts. The SKILL.md becomes a thin wrapper that delegates to fix-all.sh.
The deterministic orchestrator (fix-all.sh) handles the full flow: detect config, create fix branch, scan with grype, fix each CVE, verify, commit, run tests. After the deterministic phase, it invokes a Claude subagent (review.sh) to review all outcomes and handle edge cases like breaking changes or persistent CVEs.
Add make cve-fix target for standalone use outside the skill.
See SEP-0032 for design details.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores