fix: MCP-first Jira auth and diagnose Ambient env var accessibility#104
Open
vmrh21 wants to merge 11 commits intoambient-code:mainfrom
Open
fix: MCP-first Jira auth and diagnose Ambient env var accessibility#104vmrh21 wants to merge 11 commits intoambient-code:mainfrom
vmrh21 wants to merge 11 commits intoambient-code:mainfrom
Conversation
Two issues fixed: 1. MCP not checked proactively: workflow jumped straight to curl/env var auth and failed even when a Jira MCP server was available. Now Step 2 explicitly checks for mcp__jira* / mcp__atlassian* tools first and uses MCP if found — no credentials needed from the user. 2. Ambient custom env vars not accessible to bash: env vars configured in Ambient session settings may not be automatically exported to bash subprocesses. Added a diagnostic step that checks accessibility before attempting the curl auth call, and gives a clear message with the specific export commands if the vars aren't reaching bash. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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:
WalkthroughJira access now prefers detected MCP Jira tools and skips curl/basic-auth when available; otherwise adds explicit token/export diagnostics and direct token-regeneration on HTTP 401. /cve.fix adds an opt-in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as /cve.fix (CLI)
participant Scanner
participant Build as Build/Test Runner
participant Verifier as Post-Fix Verifier
participant GH as gh CLI
participant GitHub
User->>CLI: run /cve.fix [--automerge]
CLI->>Scanner: run initial scan -> identify CVE
CLI->>Build: run build/tests and apply fixes on branch
Build->>Verifier: trigger post-fix scan
Verifier-->>CLI: CVE resolved? (yes/no)
alt CVE resolved
CLI->>GH: gh pr create -> returns PR_URL
GH->>GitHub: create PR
alt AUTOMERGE=true
CLI->>GH: gh pr merge --auto --squash PR_URL
GH->>GitHub: merge PR
end
else CVE still present
CLI->>GitHub: post Jira comment / write fix-failed artifact
end
sequenceDiagram
participant CLI as /cve.find (CLI)
participant Env as Bash Env
participant MCP as MCP Jira Tool
participant JiraAPI as Jira REST
CLI->>Env: inspect available session tools (mcp__jira*, mcp__atlassian*)
alt MCP found
CLI->>MCP: pass JQL to MCP tool (no auth setup)
MCP->>JiraAPI: query Jira via MCP
else MCP not found
CLI->>Env: check JIRA_API_TOKEN and JIRA_EMAIL exported
alt tokens present
CLI->>JiraAPI: use curl with constructed AUTH
else tokens missing
CLI-->>Env: instruct export of JIRA_API_TOKEN/JIRA_EMAIL
end
JiraAPI-->>CLI: HTTP 401 => instruct generate/export new token
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workflows/cve-fixer/.claude/commands/cve.find.md (1)
115-203:⚠️ Potential issue | 🔴 CriticalCritical: Missing MCP execution path in Step 3.
The documentation instructs Step 2.1 to detect MCP and "skip the curl/auth setup entirely," but Step 3 only provides curl-based code that references the
${AUTH}variable. This creates a critical logic gap:
- If MCP is detected in Step 2.1,
AUTHis never defined- Step 3's curl commands (lines 184-203, 238-253, 302-310) all reference
${AUTH}- The workflow will fail with undefined variable errors
The comment at line 120 acknowledges "If using MCP... no AUTH needed," but there's no corresponding bash code showing how to execute the query using the MCP tool.
Required fix:
Step 3 needs conditional logic to handle both execution paths. For example:
🔧 Add MCP execution path to Step 3
At the beginning of Step 3, add a conditional check:
# Determine which path to use based on Step 2 if [[ $(list_available_tools) =~ mcp__jira|mcp__atlassian ]]; then USE_MCP=true echo "Using MCP tool for Jira query" else USE_MCP=false echo "Using curl for Jira query" fiThen in section 3b (around line 184), replace the single curl execution with:
# Execute query based on authentication method if [ "$USE_MCP" = "true" ]; then # MCP execution path # Call the appropriate MCP tool with JQL query # Example (adjust based on actual MCP tool interface): RESPONSE=$(mcp_jira_search --jql "${JQL}" --fields "key,summary,status,priority,created,components" --max-results 100) # Parse MCP response format (may differ from curl/JSON response) # Extract and save results appropriately else # Curl execution path (existing code) RESPONSE=$(curl -s -w "\n%{http_code}" -X GET \ --connect-timeout 10 \ --max-time 30 \ --retry 3 \ --retry-delay 2 \ -H "Content-Type: application/json" \ -H "Authorization: Basic ${AUTH}" \ "${JIRA_BASE_URL}/rest/api/3/search/jql?jql=${ENCODED_JQL}&fields=key,summary,status,priority,created,components&maxResults=100&startAt=0") # ... existing curl response handling ... fiApply similar conditional logic to:
- Pagination code (lines 238-253)
- Comment fetching code (lines 302-310)
Alternative approach: If the specific MCP tool interface is not yet defined, add a clear TODO or placeholder in Step 3 indicating that MCP execution code needs to be added, and temporarily disable the MCP detection in Step 2 until the execution path is implemented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.find.md` around lines 115 - 203, The Step 3 code uses ${AUTH} and curl unconditionally causing failures when MCP was detected in Step 2; add a conditional execution path controlled by a USE_MCP flag (set from the existing MCP detection logic or via a small list_available_tools check) and replace the direct curl calls that set RESPONSE/HTTP_CODE/BODY with a branch: when USE_MCP=true invoke the MCP query interface (e.g., mcp_jira_search or equivalent) using the JQL/ENCODED_JQL and populate RESPONSE/HTTP_CODE/BODY in the same format expected by the rest of the script (or add a clear TODO placeholder for MCP parsing if interface is unknown), and when USE_MCP=false keep the existing curl+AUTH path; apply this branching for the initial search, pagination loop, and comment-fetching sections so AUTH is never referenced when MCP is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workflows/cve-fixer/.claude/commands/cve.find.md`:
- Around line 76-90: Add blank lines before and after each code fence in the
markdown around the diagnostic bash snippet so static analysis stops flagging
formatting; specifically update the block containing TOKEN_SET and EMAIL_SET and
ensure there's an empty line before the opening ```bash and an empty line after
the closing ``` that precedes the bullet list and similarly an empty line before
the following ```bash export ...``` fence and after it so the code fences are
separated from surrounding text.
---
Outside diff comments:
In `@workflows/cve-fixer/.claude/commands/cve.find.md`:
- Around line 115-203: The Step 3 code uses ${AUTH} and curl unconditionally
causing failures when MCP was detected in Step 2; add a conditional execution
path controlled by a USE_MCP flag (set from the existing MCP detection logic or
via a small list_available_tools check) and replace the direct curl calls that
set RESPONSE/HTTP_CODE/BODY with a branch: when USE_MCP=true invoke the MCP
query interface (e.g., mcp_jira_search or equivalent) using the JQL/ENCODED_JQL
and populate RESPONSE/HTTP_CODE/BODY in the same format expected by the rest of
the script (or add a clear TODO placeholder for MCP parsing if interface is
unknown), and when USE_MCP=false keep the existing curl+AUTH path; apply this
branching for the initial search, pagination loop, and comment-fetching sections
so AUTH is never referenced when MCP is used.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a5f7e2ff-f09f-4b5f-9866-dd99de0f01fd
📒 Files selected for processing (1)
workflows/cve-fixer/.claude/commands/cve.find.md
When passed, enables GitHub automerge on each PR after creation so it merges automatically once all required checks pass. Off by default — user must explicitly opt in. Usage: /cve.fix --automerge /cve.fix RHOAIENG-4973 --automerge Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
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 `@workflows/cve-fixer/.claude/commands/cve.fix.md`:
- Around line 1236-1240: Update the fenced code block in the new usage example
so it satisfies markdownlint MD031/MD040: add a blank line before and after the
triple-backtick block and include a language identifier (e.g., text) after the
opening ```; specifically replace the snippet shown (the two lines with
"/cve.fix --automerge" and "/cve.fix RHOAIENG-4973 --automerge") with a fenced
block that begins with "```text", contains those two lines, and ends with "```",
ensuring there is an empty line above and below the fenced block.
- Around line 1082-1091: Add a guard that verifies PR_URL is non-empty before
attempting automerge: instead of only testing AUTOVERGE, update the conditional
around gh pr merge to require both AUTOMERGE="true" and a non-empty PR_URL
(e.g., test -n "$PR_URL"), and if PR_URL is empty emit an error message and
skip/exit rather than calling gh pr merge with an invalid URL; update the block
that references PR_URL, AUTOMERGE, and the gh pr merge command accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cd4e696d-19a5-4d1d-b0ee-bf6796593fa2
📒 Files selected for processing (1)
workflows/cve-fixer/.claude/commands/cve.fix.md
Comment on lines
+1082
to
+1091
| PR_URL=$(gh pr create \ | ||
| --base <target-branch> \ | ||
| --title "Security: Fix CVE-YYYY-XXXXX (<package-name>)" \ | ||
| --body "$PR_BODY" | ||
| --body "$PR_BODY") | ||
|
|
||
| # Enable automerge if --automerge flag was passed | ||
| if [ "$AUTOMERGE" = "true" ]; then | ||
| gh pr merge --auto --squash "$PR_URL" | ||
| echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" | ||
| fi |
Contributor
There was a problem hiding this comment.
Add a guard before enabling automerge to avoid acting on an empty PR URL.
At Line 1082 and Line 1088, automerge is attempted based only on AUTOMERGE. Add a non-empty PR_URL check first so failure paths are explicit and don’t continue with invalid input.
Suggested fix
- PR_URL=$(gh pr create \
+ PR_URL=$(gh pr create \
--base <target-branch> \
--title "Security: Fix CVE-YYYY-XXXXX (<package-name>)" \
--body "$PR_BODY")
# Enable automerge if --automerge flag was passed
- if [ "$AUTOMERGE" = "true" ]; then
+ if [ -z "$PR_URL" ]; then
+ echo "❌ PR creation did not return a URL; skipping automerge"
+ elif [ "$AUTOMERGE" = "true" ]; then
gh pr merge --auto --squash "$PR_URL"
echo "✅ Automerge enabled on $PR_URL — will merge when checks pass"
fi📝 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.
Suggested change
| PR_URL=$(gh pr create \ | |
| --base <target-branch> \ | |
| --title "Security: Fix CVE-YYYY-XXXXX (<package-name>)" \ | |
| --body "$PR_BODY" | |
| --body "$PR_BODY") | |
| # Enable automerge if --automerge flag was passed | |
| if [ "$AUTOMERGE" = "true" ]; then | |
| gh pr merge --auto --squash "$PR_URL" | |
| echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" | |
| fi | |
| PR_URL=$(gh pr create \ | |
| --base <target-branch> \ | |
| --title "Security: Fix CVE-YYYY-XXXXX (<package-name>)" \ | |
| --body "$PR_BODY") | |
| # Enable automerge if --automerge flag was passed | |
| if [ -z "$PR_URL" ]; then | |
| echo "❌ PR creation did not return a URL; skipping automerge" | |
| elif [ "$AUTOMERGE" = "true" ]; then | |
| gh pr merge --auto --squash "$PR_URL" | |
| echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" | |
| fi |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1082-1082: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1082 - 1091,
Add a guard that verifies PR_URL is non-empty before attempting automerge:
instead of only testing AUTOVERGE, update the conditional around gh pr merge to
require both AUTOMERGE="true" and a non-empty PR_URL (e.g., test -n "$PR_URL"),
and if PR_URL is empty emit an error message and skip/exit rather than calling
gh pr merge with an invalid URL; update the block that references PR_URL,
AUTOMERGE, and the gh pr merge command accordingly.
After applying the fix but before creating the PR, re-run the same scanner (govulncheck/pip-audit/npm audit) to confirm the CVE is actually gone. If CVE is still present after fix: - Do NOT create a PR (fix was insufficient) - Add Jira comment explaining the fix failed and why (transitive dep conflict, wrong package, etc.) - Save to artifacts/cve-fixer/fixes/fix-failed-CVE-*.md This catches cases like: - Transitive dependency still pulling in the old vulnerable version - Go toolchain directive not propagating correctly - Wrong package targeted in the fix Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Previously only searched for PRs containing the CVE ID. Bots like Dependabot and Renovate open PRs with just the package name and version bump (e.g. "Bump urllib3 from 1.26.5 to 2.2.3") without mentioning the CVE ID, causing the workflow to create duplicate fix PRs. Now runs 3 searches before creating a PR: 1. CVE ID search — catches our own and manually created PRs 2. Package name + bot author filter — catches Dependabot/Renovate PRs 3. Package name broadly — catches any human PR for the same package Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
workflows/cve-fixer/.claude/commands/cve.fix.md (2)
1312-1315:⚠️ Potential issue | 🟡 MinorFix fenced-block linting in the new
--automergeusage example.The block at Line 1312-Line 1315 is missing a language and blank-line spacing around the fence (MD031/MD040), matching a previously reported pattern.
Suggested fix
Fix and enable automerge on all created PRs (merges automatically when checks pass): -``` + +```text /cve.fix --automerge /cve.fix RHOAIENG-4973 --automerge
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@workflows/cve-fixer/.claude/commands/cve.fix.mdaround lines 1312 - 1315,
The fenced code block showing usage for the /cve.fix command lacks a language
tag and surrounding blank lines, violating MD031/MD040; update the block (the
example containing "/cve.fix --automerge" and "/cve.fix RHOAIENG-4973
--automerge") to include a language identifier (e.g., ```text) and ensure there
is a blank line before and after the fenced block so the code fence is properly
spaced and lint-clean.</details> --- `1159-1163`: _⚠️ Potential issue_ | _🟠 Major_ **Guard automerge behind a non-empty PR URL before calling `gh pr merge`.** At Line 1160-Line 1162, automerge runs only on `AUTOMERGE=true`, but not on successful PR creation. If `gh pr create` returns empty output, merge is attempted with invalid input. This was previously flagged and is still present. <details> <summary>Suggested guard</summary> ```diff -# Enable automerge if --automerge flag was passed -if [ "$AUTOMERGE" = "true" ]; then +# Enable automerge if --automerge flag was passed +if [ -z "$PR_URL" ]; then + echo "❌ PR creation did not return a URL; skipping automerge" +elif [ "$AUTOMERGE" = "true" ]; then gh pr merge --auto --squash "$PR_URL" echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" fi ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1159 - 1163, The automerge block uses AUTOMERGE and calls gh pr merge on PR_URL without verifying PR_URL is non-empty; update the condition to require both AUTOMERGE=true and a non-empty PR_URL (e.g., test -n "$PR_URL") before invoking gh pr merge --auto --squash so you never call gh pr merge with an empty/invalid PR URL; modify the guard around the gh pr merge call that references AUTOMERGE and PR_URL accordingly. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@workflows/cve-fixer/.claude/commands/cve.fix.md:
- Around line 1040-1058: Replace the hardcoded Basic auth flow around AUTH and
the curl POST with the repository's MCP-first Jira auth pattern: detect and use
the MCP-provided Jira token/credentials helper (instead of using JIRA_EMAIL and
JIRA_API_TOKEN directly), fall back to well-documented env vars only if MCP is
unavailable, and propagate diagnostics when env vars are missing; update the
block that builds COMMENT_JSON and issues the curl POST to call the MCP auth
helper (or export the MCP token into the Authorization header) and ensure
variables referenced (COMMENT_TEXT, COMMENT_JSON, JIRA_BASE_URL, JIRA_KEY)
remain unchanged so the POST uses the MCP-derived Authorization header rather
than AUTH built from JIRA_EMAIL:JIRA_API_TOKEN.- Around line 1020-1032: The post-fix scan captures tool output into
POST_SCAN_OUTPUT but never checks the scan command status, so a failed
govulncheck (or pip-audit/npm audit) can produce an empty output and falsely
mark CVE_STILL_PRESENT=false; update the block that runs govulncheck
(GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck ...) to capture its exit code
($?) immediately and if non-zero either fail the workflow or set
CVE_STILL_PRESENT=true and emit a clear error via echo/process logger; then only
set CVE_STILL_PRESENT=false when the scan succeeded and grep of POST_SCAN_OUTPUT
for $CVE_ID returns no match (apply the same exit-code check pattern for the
Python/Node variants).- Around line 597-600: The EXISTING_PR lookup uses a loose gh pr list --search
"$PACKAGE" which can match unrelated PRs for common package names; update the
EXISTING_PR query (the gh pr list invocation that writes to EXISTING_PR) to
tighten matching by requiring the package name appear as an exact word in the PR
title or by additionally filtering for a security/CVE context (e.g., require
"CVE" or "security" in the title/body) using the gh --jq filter so false
positives are avoided; apply the same fix to the second occurrence of the gh pr
list that appears around the 642–647 region so both searches use the stricter
filter referencing $PACKAGE, TARGET_BRANCH and REPO_FULL.
Duplicate comments:
In@workflows/cve-fixer/.claude/commands/cve.fix.md:
- Around line 1312-1315: The fenced code block showing usage for the /cve.fix
command lacks a language tag and surrounding blank lines, violating MD031/MD040;
update the block (the example containing "/cve.fix --automerge" and "/cve.fix
RHOAIENG-4973 --automerge") to include a language identifier (e.g., ```text) and
ensure there is a blank line before and after the fenced block so the code fence
is properly spaced and lint-clean.- Around line 1159-1163: The automerge block uses AUTOMERGE and calls gh pr
merge on PR_URL without verifying PR_URL is non-empty; update the condition to
require both AUTOMERGE=true and a non-empty PR_URL (e.g., test -n "$PR_URL")
before invoking gh pr merge --auto --squash so you never call gh pr merge with
an empty/invalid PR URL; modify the guard around the gh pr merge call that
references AUTOMERGE and PR_URL accordingly.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `76288db4-5ead-4b19-8636-275995b368b2` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 16eaf6cad7b59159f4c44a6dd04ae3952afb0fdb and 9f94c2fa25cfa68b4229bab5e4a691df3efd5289. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `workflows/cve-fixer/.claude/commands/cve.fix.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+597
to
+600
| EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \ | ||
| --search "$PACKAGE" --json number,title,url --jq '.[0]' 2>/dev/null) | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
Package-name fallback can skip unrelated PRs on common package terms.
At Line 597-Line 599, the third search picks the first open PR matching $PACKAGE on the target branch. For common names, this can create false positives and incorrectly skip required CVE fixes.
Proposed tightening
-EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \
- --search "$PACKAGE" --json number,title,url --jq '.[0]' 2>/dev/null)
+EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \
+ --search "$PACKAGE" --json number,title,url \
+ --jq '[.[] | select(.title | test("(^|[^A-Za-z0-9])" + $pkg + "([^A-Za-z0-9]|$)"; "i"))
+ | select(.title | test("bump|upgrade|security|cve"; "i"))] | .[0]' \
+ --arg pkg "$PACKAGE" 2>/dev/null)Also applies to: 642-647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 597 - 600, The
EXISTING_PR lookup uses a loose gh pr list --search "$PACKAGE" which can match
unrelated PRs for common package names; update the EXISTING_PR query (the gh pr
list invocation that writes to EXISTING_PR) to tighten matching by requiring the
package name appear as an exact word in the PR title or by additionally
filtering for a security/CVE context (e.g., require "CVE" or "security" in the
title/body) using the gh --jq filter so false positives are avoided; apply the
same fix to the second occurrence of the gh pr list that appears around the
642–647 region so both searches use the stricter filter referencing $PACKAGE,
TARGET_BRANCH and REPO_FULL.
Comment on lines
+1020
to
+1032
| ```bash | ||
| echo "Re-scanning to verify CVE-${CVE_ID} is resolved..." | ||
| # Run same scan as Step 5 with same GOTOOLCHAIN/language tooling | ||
| POST_SCAN_OUTPUT=$(GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck -show verbose ./... 2>&1) | ||
| # For Python: pip-audit -r requirements.txt 2>/dev/null | ||
| # For Node: npm audit --json 2>/dev/null | ||
|
|
||
| if echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"; then | ||
| CVE_STILL_PRESENT=true | ||
| else | ||
| CVE_STILL_PRESENT=false | ||
| fi | ||
| ``` |
Contributor
There was a problem hiding this comment.
Post-fix verification can produce false passes when scan command fails.
At Line 1023, the command is executed without checking exit status, and the decision at Line 1027 only greps output for $CVE_ID. If govulncheck fails (tool missing/network/toolchain issue), this can incorrectly set CVE_STILL_PRESENT=false and allow PR creation.
Proposed fix
-POST_SCAN_OUTPUT=$(GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck -show verbose ./... 2>&1)
-# For Python: pip-audit -r requirements.txt 2>/dev/null
-# For Node: npm audit --json 2>/dev/null
-
-if echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"; then
+POST_SCAN_OUTPUT=""
+POST_SCAN_EXIT=0
+
+if [ -f "go.mod" ]; then
+ POST_SCAN_OUTPUT=$(GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck -show verbose ./... 2>&1)
+ POST_SCAN_EXIT=$?
+elif [ -f "requirements.txt" ] || [ -f "pyproject.toml" ]; then
+ POST_SCAN_OUTPUT=$(pip-audit -r requirements.txt 2>&1)
+ POST_SCAN_EXIT=$?
+elif [ -f "package.json" ]; then
+ POST_SCAN_OUTPUT=$(npm audit --json 2>&1)
+ POST_SCAN_EXIT=$?
+else
+ echo "⚠️ Could not determine scanner for repository; skipping PR creation for safety."
+ CVE_STILL_PRESENT=true
+fi
+
+if [ "${POST_SCAN_EXIT}" -ne 0 ]; then
+ echo "⚠️ Post-fix scan failed (exit ${POST_SCAN_EXIT}); skipping PR creation for safety."
+ CVE_STILL_PRESENT=true
+elif echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"; then
CVE_STILL_PRESENT=true
else
CVE_STILL_PRESENT=false
fiAlso applies to: 1036-1037
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1020 - 1032,
The post-fix scan captures tool output into POST_SCAN_OUTPUT but never checks
the scan command status, so a failed govulncheck (or pip-audit/npm audit) can
produce an empty output and falsely mark CVE_STILL_PRESENT=false; update the
block that runs govulncheck (GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck
...) to capture its exit code ($?) immediately and if non-zero either fail the
workflow or set CVE_STILL_PRESENT=true and emit a clear error via echo/process
logger; then only set CVE_STILL_PRESENT=false when the scan succeeded and grep
of POST_SCAN_OUTPUT for $CVE_ID returns no match (apply the same exit-code check
pattern for the Python/Node variants).
The post-fix verification was being skipped because it wasn't marked as mandatory. Also, re-scanning source code (govulncheck ./...) after a fix can give false negatives — the go.mod may be updated but the CVE could still be present in the compiled binary if a transitive dep overrides it. Changes: - Mark Step 10.5 as CRITICAL/MANDATORY with explicit "Do NOT skip" warning - For Go: build the binary first, then scan with govulncheck --mode binary (gold standard — verifies the actual compiled output, not just source) - Fall back to source scan if binary build fails - For Python: re-run pip-audit on modified requirements - For Node: regenerate lockfile then npm audit The workflow must not proceed to PR creation without completing this step. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (5)
workflows/cve-fixer/.claude/commands/cve.fix.md (5)
1033-1039:⚠️ Potential issue | 🟠 MajorPost-fix scan still allows false pass on scanner failure.
POST_SCAN_OUTPUTis checked only via grep at Line 1060; scanner exit codes are not captured/validated. A failinggovulncheck/pip-audit/npm auditcan incorrectly produceCVE_STILL_PRESENT=false. This issue was previously raised and remains.#!/bin/bash # Verify whether post-fix scanners capture and validate exit status before grep checks. rg -n -C3 'POST_SCAN_OUTPUT|POST_SCAN_EXIT|CVE_STILL_PRESENT|govulncheck|pip-audit|npm audit' workflows/cve-fixer/.claude/commands/cve.fix.mdAlso applies to: 1060-1064
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1033 - 1039, The post-fix scanner output is stored in POST_SCAN_OUTPUT but its exit status isn't checked, so failures from govulncheck/pip-audit/npm audit can produce false "no CVE" results; capture each scanner's exit code (e.g., POST_SCAN_EXIT=$? right after running govulncheck/pip-audit/npm audit), and update the logic that sets CVE_STILL_PRESENT to treat non-zero POST_SCAN_EXIT as a scanner failure (set CVE_STILL_PRESENT=true or abort the job) before relying on grep of POST_SCAN_OUTPUT; apply the same pattern for all scanner invocations referenced by POST_SCAN_OUTPUT, POST_SCAN_EXIT, CVE_STILL_PRESENT, govulncheck, pip-audit and npm audit so grep is only used for content checks when the scanner exited successfully.
1345-1348:⚠️ Potential issue | 🟡 MinorFix fenced-block markdownlint regression in usage example.
The usage snippet still needs fenced-block language and surrounding blank lines (MD031/MD040), matching the prior lint finding pattern.
Lint-compliant snippet
Fix and enable automerge on all created PRs (merges automatically when checks pass): -``` +```text /cve.fix --automerge /cve.fix RHOAIENG-4973 --automerge</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@workflows/cve-fixer/.claude/commands/cve.fix.mdaround lines 1345 - 1348,
The usage example for the /cve.fix command is missing a fenced code block
language and surrounding blank lines which triggers MD031/MD040; wrap the two
lines starting with "/cve.fix" in a fenced block annotated with a language
(e.g., ```text) and ensure there's a blank line before and after the fenced
block so the snippet for the /cve.fix usage becomes a proper fenced-block code
sample.</details> --- `1192-1195`: _⚠️ Potential issue_ | _🟠 Major_ **Guard automerge on non-empty `PR_URL` before merge.** Automerge is gated only by `AUTOMERGE`; if `gh pr create` fails or returns empty output, `gh pr merge --auto --squash "$PR_URL"` can run with invalid input. This is a previously reported issue. <details> <summary>Suggested guard</summary> ```diff - if [ "$AUTOMERGE" = "true" ]; then + if [ -z "$PR_URL" ]; then + echo "❌ PR creation did not return a URL; skipping automerge" + elif [ "$AUTOMERGE" = "true" ]; then gh pr merge --auto --squash "$PR_URL" echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" fi ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1192 - 1195, Automerge currently runs purely on the AUTOMERGE flag and may call gh pr merge with an empty PR_URL; update the guard so you only call gh pr merge --auto --squash "$PR_URL" when PR_URL is non-empty (e.g., test [ -n "$PR_URL" ] or [[ -n "$PR_URL" ]]) and otherwise log an error and skip/exit; change the block around the AUTOMERGE check that references $AUTOMERGE and $PR_URL so it verifies PR_URL first and only runs gh pr merge when PR_URL contains a valid value. ``` </details> --- `1084-1090`: _⚠️ Potential issue_ | _🟠 Major_ **Jira comment path still bypasses MCP-first auth flow.** This block still hardcodes Basic auth from `JIRA_EMAIL:JIRA_API_TOKEN` and does not follow the MCP-first Jira strategy documented in `workflows/cve-fixer/.claude/commands/cve.find.md` (Context snippet 1). Same reliability gap as previously reported. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1084 - 1090, The code block currently builds a Basic auth header from JIRA_EMAIL:JIRA_API_TOKEN (AUTH) and calls curl directly, which bypasses the MCP-first Jira auth flow; replace the hardcoded Basic auth usage with the MCP-first auth helper used elsewhere (e.g., call the project’s get_mcp_jira_auth or equivalent helper to obtain the Authorization header) and pass that header to the curl request that posts COMMENT_JSON to "${JIRA_BASE_URL}/rest/api/3/issue/${JIRA_KEY}/comment"; keep a clear fallback to the existing AUTH only if the MCP helper returns nothing, and ensure the variable used in the curl -H "Authorization: ..." reflects the MCP token rather than AUTH when available. ``` </details> --- `597-599`: _⚠️ Potential issue_ | _🟠 Major_ **Tighten package-name fallback matching to avoid false-positive PR skips.** Line 597 still uses a broad `--search "$PACKAGE"` fallback without contextual filtering, which can match unrelated open PRs for common package names and skip valid fixes. This is the same issue previously reported. <details> <summary>Proposed tightening</summary> ```diff - EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \ - --search "$PACKAGE" --json number,title,url --jq '.[0]' 2>/dev/null) + EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \ + --search "$PACKAGE" --json number,title,url \ + --jq '[.[] + | select(.title | test("(^|[^A-Za-z0-9])" + $pkg + "([^A-Za-z0-9]|$)"; "i")) + | select(.title | test("bump|upgrade|security|cve"; "i"))] + | .[0]' --arg pkg "$PACKAGE" 2>/dev/null) ``` </details> Also applies to: 642-647 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 597 - 599, EXISTING_PR discovery is too broad because it relies on --search "$PACKAGE" which can match unrelated PRs; update the EXISTING_PR assignment (the gh pr list call that sets EXISTING_PR) to remove the loose --search fallback and instead call gh pr list with --json number,title,url (and baseRefName if available) and use --jq filtering to select PRs whose title (and/or changed files) explicitly matches the exact package name and the TARGET_BRANCH, ensuring you only pick PRs that are truly for this package; apply the same tightening to the other gh pr list usage around the block referenced (the second occurrence at lines ~642-647) so both checks use the stricter jq-based matching rather than the broad --search "$PACKAGE". ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In@workflows/cve-fixer/.claude/commands/cve.fix.md:
- Around line 1033-1039: The post-fix scanner output is stored in
POST_SCAN_OUTPUT but its exit status isn't checked, so failures from
govulncheck/pip-audit/npm audit can produce false "no CVE" results; capture each
scanner's exit code (e.g., POST_SCAN_EXIT=$? right after running
govulncheck/pip-audit/npm audit), and update the logic that sets
CVE_STILL_PRESENT to treat non-zero POST_SCAN_EXIT as a scanner failure (set
CVE_STILL_PRESENT=true or abort the job) before relying on grep of
POST_SCAN_OUTPUT; apply the same pattern for all scanner invocations referenced
by POST_SCAN_OUTPUT, POST_SCAN_EXIT, CVE_STILL_PRESENT, govulncheck, pip-audit
and npm audit so grep is only used for content checks when the scanner exited
successfully.- Around line 1345-1348: The usage example for the /cve.fix command is missing a
fenced code block language and surrounding blank lines which triggers
MD031/MD040; wrap the two lines starting with "/cve.fix" in a fenced block
annotated with a language (e.g., ```text) and ensure there's a blank line before
and after the fenced block so the snippet for the /cve.fix usage becomes a
proper fenced-block code sample.- Around line 1192-1195: Automerge currently runs purely on the AUTOMERGE flag
and may call gh pr merge with an empty PR_URL; update the guard so you only call
gh pr merge --auto --squash "$PR_URL" when PR_URL is non-empty (e.g., test [ -n
"$PR_URL" ] or [[ -n "$PR_URL" ]]) and otherwise log an error and skip/exit;
change the block around the AUTOMERGE check that references $AUTOMERGE and
$PR_URL so it verifies PR_URL first and only runs gh pr merge when PR_URL
contains a valid value.- Around line 1084-1090: The code block currently builds a Basic auth header
from JIRA_EMAIL:JIRA_API_TOKEN (AUTH) and calls curl directly, which bypasses
the MCP-first Jira auth flow; replace the hardcoded Basic auth usage with the
MCP-first auth helper used elsewhere (e.g., call the project’s get_mcp_jira_auth
or equivalent helper to obtain the Authorization header) and pass that header to
the curl request that posts COMMENT_JSON to
"${JIRA_BASE_URL}/rest/api/3/issue/${JIRA_KEY}/comment"; keep a clear fallback
to the existing AUTH only if the MCP helper returns nothing, and ensure the
variable used in the curl -H "Authorization: ..." reflects the MCP token rather
than AUTH when available.- Around line 597-599: EXISTING_PR discovery is too broad because it relies on
--search "$PACKAGE" which can match unrelated PRs; update the EXISTING_PR
assignment (the gh pr list call that sets EXISTING_PR) to remove the loose
--search fallback and instead call gh pr list with --json number,title,url (and
baseRefName if available) and use --jq filtering to select PRs whose title
(and/or changed files) explicitly matches the exact package name and the
TARGET_BRANCH, ensuring you only pick PRs that are truly for this package; apply
the same tightening to the other gh pr list usage around the block referenced
(the second occurrence at lines ~642-647) so both checks use the stricter
jq-based matching rather than the broad --search "$PACKAGE".</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: ASSERTIVE **Plan**: Pro **Run ID**: `c22f67b7-9c7d-45a4-ab20-2392755d39f7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9f94c2fa25cfa68b4229bab5e4a691df3efd5289 and 5e46a412e3e79c0300ac275114397f9963b5b0f1. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `workflows/cve-fixer/.claude/commands/cve.fix.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- Add mandatory execution rule at top: every step must be executed in order, none may be skipped. If a step produces no output, log it and continue — do not silently skip. - Mark Step 4.5 (load .cve-fix/ guidance) as CRITICAL/MANDATORY with explicit bash loop that checks every cloned repo and always logs the result whether guidance is found or not. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
When a fork already exists from a previous run, its branches may be weeks out of date. The fix branch would then be based on stale code, potentially missing upstream changes or causing merge conflicts in the PR. Now uses `gh repo sync --source upstream --branch TARGET_BRANCH` to bring the fork's target branch up to date with upstream before creating the fix branch. Logs a warning if sync fails but continues. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
New command guides teams through adding their component and repos to component-repository-mappings.json and opens a PR automatically. Features: - Interactive: collects component name, repos, types, branches one step at a time - Validates Jira component name against the API before proceeding - Auto-discovers default branch and active release branches from GitHub - Generates the correct JSON schema entry and shows it for confirmation - Forks ambient-code/workflows if user lacks write access, syncs fork with upstream main before branching - Creates PR with description including component details and next steps Usage: /onboard (fully interactive) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
workflows/cve-fixer/.claude/commands/cve.fix.md (3)
1229-1238:⚠️ Potential issue | 🟠 MajorValidate PR_URL before attempting automerge.
At line 1235, automerge is attempted based only on the
AUTOMERGEflag. Ifgh pr createat line 1229 fails (network issue, validation error, permission denied),PR_URLwill be empty, and line 1236 will rungh pr merge --auto --squash "", producing a confusing error. Add a non-empty check forPR_URLbefore enabling automerge.🐛 Proposed fix to validate PR_URL
PR_URL=$(gh pr create \ --base <target-branch> \ --title "Security: Fix CVE-YYYY-XXXXX (<package-name>)" \ --body "$PR_BODY") # Enable automerge if --automerge flag was passed - if [ "$AUTOMERGE" = "true" ]; then + if [ -z "$PR_URL" ]; then + echo "❌ PR creation did not return a URL; skipping automerge" + elif [ "$AUTOMERGE" = "true" ]; then gh pr merge --auto --squash "$PR_URL" echo "✅ Automerge enabled on $PR_URL — will merge when checks pass" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1229 - 1238, Validate PR_URL after the `gh pr create` call (`PR_URL=$(gh pr create ...)`) and before attempting automerge: ensure PR_URL is non-empty and likely valid; modify the automerge block that currently checks only `if [ "$AUTOMERGE" = "true" ]` to also check that `PR_URL` is not empty (e.g., `if [ "$AUTOMERGE" = "true" ] && [ -n "$PR_URL" ]`), and if PR_URL is empty, log an error or fail gracefully instead of calling `gh pr merge --auto --squash ""`.
1386-1390:⚠️ Potential issue | 🟡 MinorFix markdown formatting in usage example.
Lines 1387-1389 need blank lines before and after the code block, plus a language identifier to satisfy markdownlint MD031 and MD040. The past review flagged this and marked it as addressed, but the issue is still present in the current code.
📝 Proposed fix for markdown formatting
Fix and enable automerge on all created PRs (merges automatically when checks pass): -``` + +```text /cve.fix --automerge /cve.fix RHOAIENG-4973 --automerge
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@workflows/cve-fixer/.claude/commands/cve.fix.mdaround lines 1386 - 1390,
The usage example code block containing "/cve.fix --automerge" and "/cve.fix
RHOAIENG-4973 --automerge" needs proper Markdown formatting: add a blank line
before and after the fenced code block and include a language identifier (e.g.,fenced block around the existing lines so it becomes `\n\n```text\n/cve.fix --automerge\n/cve.fix RHOAIENG-4973 --automerge\n```\n\n`.
1067-1097:⚠️ Potential issue | 🔴 CriticalPost-fix scan missing exit code validation — can produce false negatives.
The scan commands at lines 1071, 1080, 1088, and 1096 don't capture or check exit codes. If
govulncheck,pip-audit, ornpm auditfails (tool missing, network timeout, build failure), the output will be empty or error text, and thegrep -q "$CVE_ID"check at line 1102 will likely return false, incorrectly settingCVE_STILL_PRESENT=false. This allows a PR to be created even though the scan didn't actually verify the fix.🐛 Proposed fix to capture and validate exit codes
+POST_SCAN_EXIT=0 + echo "Building and scanning binary to verify fix for CVE-${CVE_ID}..." cd "$REPO_DIR" # Build the binary with the fixed go.mod go build -o /tmp/fixed-binary-${REPO_NAME} ./... 2>&1 if [ $? -eq 0 ]; then # Scan the compiled binary — this is the gold standard POST_SCAN_OUTPUT=$(GOTOOLCHAIN="go${TARGET_GO_VERSION}" \ govulncheck -mode binary /tmp/fixed-binary-${REPO_NAME} 2>&1) + POST_SCAN_EXIT=$? rm -f /tmp/fixed-binary-${REPO_NAME} else echo "⚠️ Binary build failed — falling back to source scan" POST_SCAN_OUTPUT=$(GOTOOLCHAIN="go${TARGET_GO_VERSION}" govulncheck -show verbose ./... 2>&1) + POST_SCAN_EXIT=$? fi **For Python projects:** ```bash # Re-run pip-audit on the modified requirements file POST_SCAN_OUTPUT=$(pip-audit -r requirements.txt 2>&1) +POST_SCAN_EXIT=$?For Node.js projects:
# Re-run npm audit after package.json changes npm install --package-lock-only 2>/dev/null # regenerate lockfile POST_SCAN_OUTPUT=$(npm audit --json 2>&1) +POST_SCAN_EXIT=$?Check result:
+if [ "${POST_SCAN_EXIT}" -ne 0 ]; then + echo "⚠️ Post-fix scan failed (exit ${POST_SCAN_EXIT}) — cannot verify fix. Skipping PR creation for safety." + CVE_STILL_PRESENT=true # conservative: assume not fixed if we can't verify +elif echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"; then -if echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"; then CVE_STILL_PRESENT=true else CVE_STILL_PRESENT=false fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 1067 - 1097, The post-fix scan steps (commands that set POST_SCAN_OUTPUT for govulncheck, pip-audit, and npm audit) do not capture or validate exit codes, which can yield false negatives; update each scan block that sets POST_SCAN_OUTPUT (the govulncheck binary/source branches, the pip-audit block, and the npm audit block) to also capture POST_SCAN_EXIT=$? immediately after the command, then change the verification logic that sets CVE_STILL_PRESENT to first check POST_SCAN_EXIT (treat non-zero as a failed/unverifiable scan and set CVE_STILL_PRESENT=true and emit a warning), else fall back to the existing echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID" check to set CVE_STILL_PRESENT accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workflows/cve-fixer/.claude/commands/cve.fix.md`:
- Around line 1115-1133: The Jira comment block currently builds Authorization
with AUTH from JIRA_EMAIL:JIRA_API_TOKEN and calls curl directly (see AUTH,
COMMENT_JSON and the curl POST); change this to follow the MCP-first pattern:
attempt to call any mcp__jira* helper/tool first to post the comment, and only
if that fails or is unavailable, construct AUTH from env vars; before using env
vars, detect if JIRA_EMAIL/JIRA_API_TOKEN are actually exported to the bash
subshell and, if not, emit the same diagnostic used in cve.find.md explaining
ambient-to-bash env visibility and fail with a clear message; ensure the final
curl POST still uses COMMENT_JSON but only after the MCP fallback check and the
env-var availability check.
- Around line 638-641: Add an inline comment next to the EXISTING_PR lookup (the
gh pr list invocation using --search "$PACKAGE" and the jq '.[0]' selection)
that documents the known false-positive trade-off: explain that Search 3
intentionally performs a broad search to catch human-opened PRs that may not
mention the CVE, that this can match generic package names (e.g., "client",
"utils", "config") and return unrelated PRs, and instruct operators to manually
review matches for such common PACKAGE values; state this limitation is accepted
to avoid creating duplicate PRs.
In `@workflows/cve-fixer/.claude/commands/onboard.md`:
- Around line 35-46: The snippet builds Basic auth with
JIRA_EMAIL:JIRA_API_TOKEN and calls curl (variables AUTH, JIRA_BASE_URL,
ENCODED, RESULT) without checking MCP tools; change it to follow the MCP-first
Jira auth pattern: first detect MCP tools named like mcp__jira* or
mcp__atlassian* and, if found, extract and use their credentials/URL for AUTH
and JIRA_BASE_URL; if no MCP tool is present, run the bash diagnostic used in
cve.find.md to confirm that JIRA_EMAIL/JIRA_API_TOKEN are exported to
subprocesses and only then build AUTH and call curl; ensure the logic falls back
to the original env-var Basic auth path when the diagnostic passes and emits a
clear error/exit when neither MCP tools nor usable env vars are available.
- Around line 156-171: The heredoc Python block currently injects shell
variables ${COMPONENT_NAME} and ${NEW_COMPONENT_JSON} directly into Python code
which can break parsing or allow injection; change the implementation to pass
the new component JSON via stdin (read with json.load(sys.stdin)) and pass
COMPONENT_NAME and MAPPING_FILE and the date as argv, or replace the whole step
with a jq command that safely updates data["components"] and
metadata.last_updated; update the Python heredoc to read from stdin and use
sys.argv (instead of interpolating ${NEW_COMPONENT_JSON} and ${COMPONENT_NAME})
so quoting/escaping issues are eliminated.
---
Duplicate comments:
In `@workflows/cve-fixer/.claude/commands/cve.fix.md`:
- Around line 1229-1238: Validate PR_URL after the `gh pr create` call
(`PR_URL=$(gh pr create ...)`) and before attempting automerge: ensure PR_URL is
non-empty and likely valid; modify the automerge block that currently checks
only `if [ "$AUTOMERGE" = "true" ]` to also check that `PR_URL` is not empty
(e.g., `if [ "$AUTOMERGE" = "true" ] && [ -n "$PR_URL" ]`), and if PR_URL is
empty, log an error or fail gracefully instead of calling `gh pr merge --auto
--squash ""`.
- Around line 1386-1390: The usage example code block containing "/cve.fix
--automerge" and "/cve.fix RHOAIENG-4973 --automerge" needs proper Markdown
formatting: add a blank line before and after the fenced code block and include
a language identifier (e.g., ```text) on the opening fence to satisfy
markdownlint MD031/MD040; update the fenced block around the existing lines so
it becomes `\n\n```text\n/cve.fix --automerge\n/cve.fix RHOAIENG-4973
--automerge\n```\n\n`.
- Around line 1067-1097: The post-fix scan steps (commands that set
POST_SCAN_OUTPUT for govulncheck, pip-audit, and npm audit) do not capture or
validate exit codes, which can yield false negatives; update each scan block
that sets POST_SCAN_OUTPUT (the govulncheck binary/source branches, the
pip-audit block, and the npm audit block) to also capture POST_SCAN_EXIT=$?
immediately after the command, then change the verification logic that sets
CVE_STILL_PRESENT to first check POST_SCAN_EXIT (treat non-zero as a
failed/unverifiable scan and set CVE_STILL_PRESENT=true and emit a warning),
else fall back to the existing echo "$POST_SCAN_OUTPUT" | grep -q "$CVE_ID"
check to set CVE_STILL_PRESENT accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 6112e702-4a72-4859-a638-f61e83a142cf
📒 Files selected for processing (2)
workflows/cve-fixer/.claude/commands/cve.fix.mdworkflows/cve-fixer/.claude/commands/onboard.md
Comment on lines
+638
to
+641
| if [ -z "$EXISTING_PR" ] || [ "$EXISTING_PR" = "null" ]; then | ||
| EXISTING_PR=$(gh pr list --repo "$REPO_FULL" --state open --base "$TARGET_BRANCH" \ | ||
| --search "$PACKAGE" --json number,title,url --jq '.[0]' 2>/dev/null) | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Simulate broad package search to assess false positive risk.
# Example: search for common package names in a sample repo
REPO_FULL="opendatahub-io/odh-dashboard"
COMMON_PACKAGES=("config" "utils" "client" "core" "common")
for PKG in "${COMMON_PACKAGES[@]}"; do
echo "=== Searching for package: $PKG ==="
gh pr list --repo "$REPO_FULL" --state open \
--search "$PKG" --json number,title --jq '.[] | "\(.number): \(.title)"' 2>/dev/null | head -5
echo ""
doneRepository: ambient-code/workflows
Length of output: 257
🏁 Script executed:
#!/bin/bash
# Read the file and check its size
wc -l "workflows/cve-fixer/.claude/commands/cve.fix.md"Repository: ambient-code/workflows
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Read the relevant section (lines 613-690 as mentioned in scratchpad)
sed -n '613,690p' "workflows/cve-fixer/.claude/commands/cve.fix.md"Repository: ambient-code/workflows
Length of output: 4118
🏁 Script executed:
#!/bin/bash
# Search for where PACKAGE is extracted/defined
rg -n "PACKAGE=" "workflows/cve-fixer/.claude/commands/cve.fix.md" | head -20Repository: ambient-code/workflows
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Check the Step 1 section to understand PACKAGE extraction
sed -n '1,612p' "workflows/cve-fixer/.claude/commands/cve.fix.md" | tail -100Repository: ambient-code/workflows
Length of output: 5814
Search 3's broad package matching is by design, but consider documenting its false-positive trade-off.
The three-stage search strategy at lines 638-641 is intentional: Search 3 uses --search "$PACKAGE" without additional filtering to catch human-opened PRs that may already address the CVE without mentioning it explicitly (e.g., "Bump urllib3 from 1.26.5 to 2.2.3"). This is documented in the comments at lines 684-686.
However, the trade-off is real: for package names that are common words ("client", "utils", "config"), the gh pr list --search command can match unrelated PRs whose titles or bodies happen to contain these words. The jq filter .[0] then selects the first match, which may not actually be about updating the package.
Rather than tightening the search with word boundaries or keyword filters (which would change the design intent), consider:
- Adding a comment in the code explicitly stating this false-positive trade-off and why it's acceptable (better to skip a CVE if someone may be working on it than to create a duplicate)
- Documenting that operators should review matched PRs when package names are common words
- Accepting this as a known limitation of the approach when PACKAGE values are generic
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/cve-fixer/.claude/commands/cve.fix.md` around lines 638 - 641, Add
an inline comment next to the EXISTING_PR lookup (the gh pr list invocation
using --search "$PACKAGE" and the jq '.[0]' selection) that documents the known
false-positive trade-off: explain that Search 3 intentionally performs a broad
search to catch human-opened PRs that may not mention the CVE, that this can
match generic package names (e.g., "client", "utils", "config") and return
unrelated PRs, and instruct operators to manually review matches for such common
PACKAGE values; state this limitation is accepted to avoid creating duplicate
PRs.
Comment on lines
+156
to
+171
| python3 - <<PYEOF | ||
| import json | ||
|
|
||
| with open("${MAPPING_FILE}") as f: | ||
| data = json.load(f) | ||
|
|
||
| # Add new component | ||
| data["components"]["${COMPONENT_NAME}"] = ${NEW_COMPONENT_JSON} | ||
|
|
||
| # Update metadata | ||
| data["metadata"]["last_updated"] = "$(date +%Y-%m-%d)" | ||
|
|
||
| with open("${MAPPING_FILE}", "w") as f: | ||
| json.dump(data, f, indent=2, ensure_ascii=False) | ||
| f.write("\n") | ||
| PYEOF |
Contributor
There was a problem hiding this comment.
Shell variable expansion in Python heredoc risks injection and syntax errors.
Lines 163 and 166 interpolate shell variables ${COMPONENT_NAME} and ${NEW_COMPONENT_JSON} directly into Python code. If the component name contains quotes, backslashes, or newlines, this can break the Python syntax or cause unintended behavior.
🛡️ Safer approach using jq or proper Python arg passing
Option 1: Use jq (simpler, no Python):
-python3 - <<PYEOF
-import json
-
-with open("${MAPPING_FILE}") as f:
- data = json.load(f)
-
-# Add new component
-data["components"]["${COMPONENT_NAME}"] = ${NEW_COMPONENT_JSON}
-
-# Update metadata
-data["metadata"]["last_updated"] = "$(date +%Y-%m-%d)"
-
-with open("${MAPPING_FILE}", "w") as f:
- json.dump(data, f, indent=2, ensure_ascii=False)
- f.write("\n")
-PYEOF
+jq --arg comp "$COMPONENT_NAME" \
+ --argjson entry "$NEW_COMPONENT_JSON" \
+ --arg date "$(date +%Y-%m-%d)" \
+ '.components[$comp] = $entry | .metadata.last_updated = $date' \
+ "${MAPPING_FILE}" > "${MAPPING_FILE}.tmp" && mv "${MAPPING_FILE}.tmp" "${MAPPING_FILE}"Option 2: Use Python with stdin (if jq not available):
echo "$NEW_COMPONENT_JSON" | python3 -c "
import sys, json
comp_name = sys.argv[1]
date_str = sys.argv[2]
mapping_file = sys.argv[3]
new_entry = json.load(sys.stdin)
with open(mapping_file) as f:
data = json.load(f)
data['components'][comp_name] = new_entry
data['metadata']['last_updated'] = date_str
with open(mapping_file, 'w') as f:
json.dump(data, f, indent=2, ensure_ascii=False)
f.write('\n')
" "$COMPONENT_NAME" "$(date +%Y-%m-%d)" "${MAPPING_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.
Suggested change
| python3 - <<PYEOF | |
| import json | |
| with open("${MAPPING_FILE}") as f: | |
| data = json.load(f) | |
| # Add new component | |
| data["components"]["${COMPONENT_NAME}"] = ${NEW_COMPONENT_JSON} | |
| # Update metadata | |
| data["metadata"]["last_updated"] = "$(date +%Y-%m-%d)" | |
| with open("${MAPPING_FILE}", "w") as f: | |
| json.dump(data, f, indent=2, ensure_ascii=False) | |
| f.write("\n") | |
| PYEOF | |
| jq --arg comp "$COMPONENT_NAME" \ | |
| --argjson entry "$NEW_COMPONENT_JSON" \ | |
| --arg date "$(date +%Y-%m-%d)" \ | |
| '.components[$comp] = $entry | .metadata.last_updated = $date' \ | |
| "${MAPPING_FILE}" > "${MAPPING_FILE}.tmp" && mv "${MAPPING_FILE}.tmp" "${MAPPING_FILE}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflows/cve-fixer/.claude/commands/onboard.md` around lines 156 - 171, The
heredoc Python block currently injects shell variables ${COMPONENT_NAME} and
${NEW_COMPONENT_JSON} directly into Python code which can break parsing or allow
injection; change the implementation to pass the new component JSON via stdin
(read with json.load(sys.stdin)) and pass COMPONENT_NAME and MAPPING_FILE and
the date as argv, or replace the whole step with a jq command that safely
updates data["components"] and metadata.last_updated; update the Python heredoc
to read from stdin and use sys.argv (instead of interpolating
${NEW_COMPONENT_JSON} and ${COMPONENT_NAME}) so quoting/escaping issues are
eliminated.
Two fixes for the auth detection failure in automated runs: 1. MCP check: don't filter by specific tool name patterns (mcp__jira*). Jira MCP may appear under any name. Search ALL available tools and their descriptions. Also try mcp__session__refresh_credentials first to activate workspace-configured Jira credentials before checking. 2. Curl fallback: the bash env var diagnostic (TOKEN_SET=yes/no) was causing the workflow to stop when vars showed as "no". This is wrong — Ambient secrets can be injected at the curl level even when not visible to shell checks. Now the diagnostic is informational only and the API call is ALWAYS attempted. Only a 401 response (not a "no" from the bash check) stops the workflow. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Generic ToolSearch queries like "jira search issue JQL" match unrelated tools instead of mcp__mcp-atlassian__jira_search. Now uses: - select:mcp__mcp-atlassian__jira_search (exact) - +atlassian jira (targeted) Also tells the workflow to read the <available-deferred-tools> list directly before searching, since the tool name is visible there. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…arch Generic ToolSearch returns wrong results. The Jira tool name is visible in <available-deferred-tools> at conversation start. Workflow now reads that list directly and fetches with select:mcp__mcp-atlassian__jira_search instead of relying on keyword search to discover it. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two auth-related issues observed:
MCP server not detected proactively — workflow jumped straight to curl/env var auth and failed with "credentials not configured" even when a Jira MCP server was available. Users had to manually say "use mcp" to get it working.
Ambient custom env vars not reaching bash —
JIRA_API_TOKENandJIRA_EMAILconfigured as custom env vars in Ambient session settings are available to Claude's context but not automatically exported to bash subprocesses, causing curl to get empty strings and return 401.Fix
mcp__jira*,mcp__atlassian*) before attempting curl. If found, MCP is used directly — no credentials needed.Test plan
/cve.findwith Jira MCP server configured — verify it uses MCP without asking for credentials/cve.findwith custom env vars in Ambient session — verify diagnostic correctly identifies if vars are/aren't accessible to bash🤖 Generated with Claude Code