ci: add code scanning workflow#3202
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow file that runs CodeQL (JS/TS, Python) and ShellCheck on PRs, pushes to main, scheduled runs, and manual dispatch, and uploads SARIF results to GitHub Code Scanning with scoped permissions. ChangesCode Scanning Workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Hi @cv, I opened this draft PR for #665 following your latest guidance. It adds a dedicated
I kept it separate from Could you review whether this is the right initial scope, especially whether ShellCheck SARIF should stay in this PR or be split into a follow-up? |
wscurran
added
security
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/code-scanning.yaml:
- Around line 67-70: The "Generate ShellCheck SARIF" step currently swallows all
failures by appending "|| true" to the run command; remove the "|| true" from
the xargs shellcheck invocation and instead set the step-level
continue-on-error: true for the step named "Generate ShellCheck SARIF" so lint
findings don't fail the job but actual execution/tool failures remain visible;
ensure the run still writes to shellcheck.sarif from shell-files.txt
(referencing shell-files.txt and shellcheck.sarif) and that no other masking of
the exit status remains.
- Around line 35-38: Replace mutable action version tags with immutable commit
SHAs: for each `uses:` reference (e.g., `actions/checkout@v6`,
`github/codeql-action/init@v4` and the other three `uses:` entries in this
workflow), update the value to the action's full commit SHA and add an inline
comment preserving the human-readable tag (e.g., `# v6`) for readability; ensure
you update all five `uses:` occurrences consistently so the workflow references
immutable SHAs while retaining the original tag in a comment.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e401b50-2167-4f0c-b45f-9bbf84c78aa2
📒 Files selected for processing (1)
.github/workflows/code-scanning.yaml
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
|
@ChunkyMonkey11 can you take a look at https://github.com/NVIDIA/NemoClaw/actions/runs/25595872841/job/75180455239?pr=3202 when you have a chance, please? |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/code-scanning.yaml (2)
35-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all GitHub Actions to immutable commit SHAs
These
uses:entries still reference mutable tags. Please pin each action to a full commit SHA and keep the human-readable tag in an inline comment.Suggested patch pattern
- - name: Checkout - uses: actions/checkout@v6 + - name: Checkout + uses: actions/checkout@<FULL_SHA> # v6 - - name: Initialize CodeQL - uses: github/codeql-action/init@v4 + - name: Initialize CodeQL + uses: github/codeql-action/init@<FULL_SHA> # v4 - - name: Perform CodeQL analysis - uses: github/codeql-action/analyze@v4 + - name: Perform CodeQL analysis + uses: github/codeql-action/analyze@<FULL_SHA> # v4 - - name: Checkout - uses: actions/checkout@v6 + - name: Checkout + uses: actions/checkout@<FULL_SHA> # v6 - - name: Upload ShellCheck SARIF - uses: github/codeql-action/upload-sarif@v4 + - name: Upload ShellCheck SARIF + uses: github/codeql-action/upload-sarif@<FULL_SHA> # v4#!/bin/bash set -euo pipefail rg -n '^\s*uses:\s*[^@]+@(v[0-9]+(\.[0-9]+){0,2}|main|master)\s*$' .github/workflows/code-scanning.yaml # Expect: no matches after pinningAlso applies to: 38-38, 44-44, 52-52, 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/code-scanning.yaml at line 35, The workflow uses mutable action tags (e.g., the uses: actions/checkout@v6 line and similar entries at the other indicated lines) which must be pinned to immutable commit SHAs; update each uses: entry (for example actions/checkout@v6 and the other referenced uses lines) to the corresponding full commit SHA while preserving the human-readable tag as an inline comment (e.g., uses: actions/checkout@<full-commit-sha> # v6), repeating this for every uses: occurrence noted in the review.
72-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t suppress all ShellCheck non-zero exits
Line 72-75 currently turns off fail-fast and never re-raises unexpected ShellCheck execution failures, so scanner/tooling breakage can be silently masked.
Suggested fix
- name: Generate ShellCheck SARIF if: steps.shell-files.outputs.has_files == 'true' run: | # ShellCheck can fail on parse issues and leave partial JSON; upload-sarif # requires syntactically valid SARIF. Validate and fall back to an empty run. set +e xargs -r shellcheck --format=sarif < shell-files.txt > shellcheck.sarif sc_exit=$? set -e if ! jq -e . shellcheck.sarif >/dev/null 2>&1; then echo "ShellCheck produced invalid SARIF (exit=$sc_exit); writing empty SARIF fallback." cat > shellcheck.sarif <<'EOF' { "version": "2.1.0", "$schema": "https://json.schemastore.org/sarif-2.1.0.json", "runs": [] } EOF fi + + # Keep lint findings non-blocking, but surface true execution/tool failures. + if [ "$sc_exit" -gt 1 ]; then + echo "ShellCheck execution failed (exit=$sc_exit)." + exit "$sc_exit" + fiAlso applies to: 77-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/code-scanning.yaml around lines 72 - 75, The workflow currently disables fail-fast (set +e) around the ShellCheck run and never re-raises failures, hiding real tool errors; change the block around the xargs shellcheck invocation to preserve exit-on-error and re-exit the script with the captured ShellCheck exit code (sc_exit) when non-zero (or only allow known acceptable codes), i.e. ensure the xargs -> shellcheck pipeline that writes shellcheck.sarif still captures sc_exit and then calls exit $sc_exit (or conditionally exit when sc_exit != 0) instead of leaving set +e in effect so CI fails on unexpected ShellCheck/tool failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/code-scanning.yaml:
- Line 35: The workflow uses mutable action tags (e.g., the uses:
actions/checkout@v6 line and similar entries at the other indicated lines) which
must be pinned to immutable commit SHAs; update each uses: entry (for example
actions/checkout@v6 and the other referenced uses lines) to the corresponding
full commit SHA while preserving the human-readable tag as an inline comment
(e.g., uses: actions/checkout@<full-commit-sha> # v6), repeating this for every
uses: occurrence noted in the review.
- Around line 72-75: The workflow currently disables fail-fast (set +e) around
the ShellCheck run and never re-raises failures, hiding real tool errors; change
the block around the xargs shellcheck invocation to preserve exit-on-error and
re-exit the script with the captured ShellCheck exit code (sc_exit) when
non-zero (or only allow known acceptable codes), i.e. ensure the xargs ->
shellcheck pipeline that writes shellcheck.sarif still captures sc_exit and then
calls exit $sc_exit (or conditionally exit when sc_exit != 0) instead of leaving
set +e in effect so CI fails on unexpected ShellCheck/tool failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fcf44d54-6da2-4fbc-bf09-0dfe95ef5323
📒 Files selected for processing (1)
.github/workflows/code-scanning.yaml
|
@cv addressed the duplicate findings in .github/workflows/code-scanning.yaml:
This should unblock and all required Actions should pass once workflows are approved and re-run. |
|
https://github.com/NVIDIA/NemoClaw/actions/runs/25613063951/job/75189628784?pr=3202 still failing, it seems. |
|
@cv thanks for flagging this. That failing run (job 75189628784) is on commit 429337a and does not include the latest fixes. I’ve pushed two updates on this PR branch: 41b8dfb (skip SARIF upload when shellcheck.sarif has zero runs) and 627afee (correct ShellCheck exit-code capture so failures are not masked). Please re-run checks on the current head; this should resolve the "1 item required; only 0 were supplied" error and address the duplicate review finding. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/code-scanning.yaml (1)
73-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExit code capture is incorrect after negation, so ShellCheck failures can be silently treated as success.
At Line 74,
sc_exit=$?reads the status of! xargs ...(the negated test), not ShellCheck’s actual non-zero exit status. This can leavesc_exit=0even when ShellCheck failed, defeating the fail-on-error intent.Suggested fix
- sc_exit=0 - if ! xargs -r shellcheck --format=sarif < shell-files.txt > shellcheck.sarif; then - sc_exit=$? - fi + if xargs -r shellcheck --format=sarif < shell-files.txt > shellcheck.sarif; then + sc_exit=0 + else + sc_exit=$? + fi#!/bin/bash set -euo pipefail echo "Inspect current snippet:" nl -ba .github/workflows/code-scanning.yaml | sed -n '67,90p' echo echo "Reproduce shell semantics:" sc_exit=0 if ! bash -lc 'exit 3'; then sc_exit=$? fi echo "Captured sc_exit with current pattern: $sc_exit (expected bug: 0)" echo echo "Control case with non-negated if/else:" if bash -lc 'exit 3'; then sc_exit2=0 else sc_exit2=$? fi echo "Captured sc_exit with if/else pattern: $sc_exit2 (expected: 3)"Expected result: first capture prints
0(bug), second prints3(correct behavior).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/code-scanning.yaml around lines 73 - 75, The negated if (! xargs -r shellcheck ...) captures the exit of the negation, so sc_exit=$? may be 0 even when ShellCheck failed; change the pattern to capture the command's exit immediately (or use a non-negated if/else). For example, run the xargs invocation, assign rc=$? right after it and set sc_exit=$rc if non-zero, or rewrite the block as "if xargs -r shellcheck --format=sarif < shell-files.txt > shellcheck.sarif; then sc_exit=0; else sc_exit=$?; fi" so sc_exit records ShellCheck's actual exit status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/code-scanning.yaml:
- Around line 73-75: The negated if (! xargs -r shellcheck ...) captures the
exit of the negation, so sc_exit=$? may be 0 even when ShellCheck failed; change
the pattern to capture the command's exit immediately (or use a non-negated
if/else). For example, run the xargs invocation, assign rc=$? right after it and
set sc_exit=$rc if non-zero, or rewrite the block as "if xargs -r shellcheck
--format=sarif < shell-files.txt > shellcheck.sarif; then sc_exit=0; else
sc_exit=$?; fi" so sc_exit records ShellCheck's actual exit status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1adf7247-e6da-4eb5-8684-98e735aa411b
📒 Files selected for processing (1)
.github/workflows/code-scanning.yaml
|
I fixed the ShellCheck SARIF failure on this branch. Root cause was not script lint findings: the runner’s ShellCheck (0.9.0 from ubuntu-latest apt) does not support Fix in commit 6c45c64:
Run triggered here: It is currently |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Approved after making ShellCheck SARIF reporting non-gating so this PR can land before the existing ShellCheck findings are cleaned up separately.
Add CodeQL and ShellCheck SARIF reporting for code scanning. Keep ShellCheck reporting non-gating until the existing ShellCheck findings are fixed separately. Addresses NVIDIA#665. Signed-off-by: Revant <revant.h.patel@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Thanks @cv |
4 participants
Addresses #665.
Adds a dedicated code-scanning workflow with:
This is additive security reporting and does not replace the existing PR/main CI checks.
Summary by CodeRabbit
Signed-off-by: Revant revant.h.patel@gmail.com