ci: add SPDX license header check for source files - #842
Conversation
📝 WalkthroughWalkthroughAdds a GitHub Actions job Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Comment |
![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 the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr.yaml:
- Line 180: The exclusion glob list in the CI workflow string that currently
contains
"node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*"
needs to also exclude test fixture paths: add "testdata/*" and "test/fixtures/*"
to that same pattern so CI ignores those directories; update the matching
pattern (the one containing
node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) to
include these two new segments separated by |.
- Around line 173-183: The loop that builds and iterates over files uses
unquoted word-splitting (the files and filtered variables and the for f in
$files loop), which breaks on filenames with spaces; change the pipeline to use
NUL-delimited transport: call git diff -z --name-only (store to a NUL-terminated
variable or pipe), build filtered by reading entries with read -r -d '' (or
filter via while read -r -d '' f; do ...; done) and collect safely without
unquoted expansion, then pass results to the SPDX check using xargs -0 or a
NUL-aware loop instead of unquoted variable expansion so filenames containing
spaces are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ffd36e4-628e-4287-acf9-fcb8f7c7c80b
📒 Files selected for processing (1)
.github/workflows/pr.yaml
| files=$(git diff --name-only --diff-filter=ACMRT "$base" -- \ | ||
| '*.ts' '*.tsx' '*.js' '*.mjs' '*.cjs' '*.py' '*.sh' '*.yaml' '*.yml' \ | ||
| || true) | ||
| # Exclude paths that are generated, vendored, or don't support comments. | ||
| filtered="" | ||
| for f in $files; do | ||
| case "$f" in | ||
| node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;; | ||
| esac | ||
| filtered="$filtered $f" | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual workflow file to see the code in context
head -250 .github/workflows/pr.yaml | tail -100Repository: NVIDIA/NemoClaw
Length of output: 3270
🏁 Script executed:
# Also check the script being called
cat scripts/check-spdx-headers.shRepository: NVIDIA/NemoClaw
Length of output: 2634
🏁 Script executed:
# Check the entire pr.yaml file for context around lines 173-183, 191-193, 205
wc -l .github/workflows/pr.yamlRepository: NVIDIA/NemoClaw
Length of output: 88
🏁 Script executed:
# Search for actual files with spaces in the repo that would match the file patterns
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.mjs" -o -name "*.cjs" -o -name "*.py" -o -name "*.sh" -o -name "*.yaml" -o -name "*.yml" \) -print0 | xargs -0 -I {} basename {} | grep ' ' | head -10Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Also verify the lines mentioned in the review match what we see
sed -n '173,183p' .github/workflows/pr.yaml
sed -n '191,193p' .github/workflows/pr.yaml
sed -n '205,205p' .github/workflows/pr.yamlRepository: NVIDIA/NemoClaw
Length of output: 788
Use NUL-delimited file transport to avoid silent check failures on paths with spaces.
The workflow at lines 173–183, 191–193, and 205 uses unquoted variable expansion for file transport, which causes word-splitting. A source file with spaces in its name (e.g., my file.ts) would be split into separate arguments; the script's [[ -f "$file" ]] || continue silently skips non-existent entries, bypassing the SPDX header check without error.
The fix uses git diff -z to output NUL-separated filenames, processes them safely with read -r -d '', and transports via xargs -0:
Suggested fix
- name: Get changed source files
id: changed
run: |
base="${{ github.event.pull_request.base.sha }}"
# File types that must carry SPDX headers.
- files=$(git diff --name-only --diff-filter=ACMRT "$base" -- \
- '*.ts' '*.tsx' '*.js' '*.mjs' '*.cjs' '*.py' '*.sh' '*.yaml' '*.yml' \
- || true)
+ tmp_raw="$RUNNER_TEMP/spdx-raw-files.zlist"
+ git diff -z --name-only --diff-filter=ACMRT "$base" -- \
+ '*.ts' '*.tsx' '*.js' '*.mjs' '*.cjs' '*.py' '*.sh' '*.yaml' '*.yml' \
+ > "$tmp_raw" || true
# Exclude paths that are generated, vendored, or don't support comments.
- filtered=""
- for f in $files; do
+ filtered=()
+ while IFS= read -r -d '' f; do
case "$f" in
node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;;
esac
- filtered="$filtered $f"
- done
- filtered="${filtered# }"
- if [ -z "$filtered" ]; then
+ filtered+=("$f")
+ done < "$tmp_raw"
+ if [ "${`#filtered`[@]}" -eq 0 ]; then
echo "No source files changed."
echo "skip=true" >> "$GITHUB_OUTPUT"
else
+ list_file="$RUNNER_TEMP/spdx-files.zlist"
+ printf '%s\0' "${filtered[@]}" > "$list_file"
echo "skip=false" >> "$GITHUB_OUTPUT"
- echo "$filtered"
- echo "files<<EOF" >> "$GITHUB_OUTPUT"
- echo "$filtered" >> "$GITHUB_OUTPUT"
- echo "EOF" >> "$GITHUB_OUTPUT"
+ echo "file_list=$list_file" >> "$GITHUB_OUTPUT"
fi
- name: Check SPDX license headers
if: steps.changed.outputs.skip != 'true'
run: |
echo ""
echo "Expected header (comment style varies by language):"
echo " SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved."
echo " SPDX-License-Identifier: Apache-2.0"
echo ""
- # shellcheck disable=SC2086
- bash scripts/check-spdx-headers.sh ${{ steps.changed.outputs.files }}
+ xargs -0 -r bash scripts/check-spdx-headers.sh < "${{ steps.changed.outputs.file_list }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr.yaml around lines 173 - 183, The loop that builds and
iterates over files uses unquoted word-splitting (the files and filtered
variables and the for f in $files loop), which breaks on filenames with spaces;
change the pipeline to use NUL-delimited transport: call git diff -z --name-only
(store to a NUL-terminated variable or pipe), build filtered by reading entries
with read -r -d '' (or filter via while read -r -d '' f; do ...; done) and
collect safely without unquoted expansion, then pass results to the SPDX check
using xargs -0 or a NUL-aware loop instead of unquoted variable expansion so
filenames containing spaces are handled correctly.
| filtered="" | ||
| for f in $files; do | ||
| case "$f" in | ||
| node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;; |
There was a problem hiding this comment.
Exclusion patterns are missing required fixture/testdata paths.
Line 180 does not exclude testdata/ and test/fixtures/, which are part of the stated acceptance criteria for this PR/issue. This can cause unintended CI failures on fixture files.
Targeted pattern update
- node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;;
+ node_modules/*|dist/*|build/*|docs/*|testdata/*|test/fixtures/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;;📝 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.
| node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;; | |
| node_modules/*|dist/*|build/*|docs/*|testdata/*|test/fixtures/*|nemoclaw/node_modules/*|.venv/*|_deps/*) continue ;; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr.yaml at line 180, The exclusion glob list in the CI
workflow string that currently contains
"node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*"
needs to also exclude test fixture paths: add "testdata/*" and "test/fixtures/*"
to that same pattern so CI ignores those directories; update the matching
pattern (the one containing
node_modules/*|dist/*|build/*|docs/*|nemoclaw/node_modules/*|.venv/*|_deps/*) to
include these two new segments separated by |.
Add a check-spdx-headers job to the PR workflow that runs the existing scripts/check-spdx-headers.sh (check-only mode, no --fix) against source files changed in the PR. The job diffs .ts, .js, .py, .sh, .yaml, and .yml files against the PR base, excludes generated/vendored paths (node_modules, dist, docs, .venv), and prints the expected header format on failure so contributors can copy-paste it. Fixes NVIDIA#551
17a26e1 to
ce9db2c
Compare
|
The prek hooks should take care of that, and they also run in CI |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pr.yaml (1)
191-193: Consider dynamically generating the expected header year.The year "2026" is hardcoded in this informational message. While this doesn't affect validation (the script's
COPYRIGHT_SUBSTRonly matches the prefix without the year), it will become stale and could confuse contributors in future years.Optional: Use dynamic year substitution
- name: Check SPDX license headers if: steps.changed.outputs.skip != 'true' run: | + year=$(date +%Y) echo "" echo "Expected header (comment style varies by language):" - echo " SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved." + echo " SPDX-FileCopyrightText: Copyright (c) $year NVIDIA CORPORATION & AFFILIATES. All rights reserved." echo " SPDX-License-Identifier: Apache-2.0" echo ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr.yaml around lines 191 - 193, The informational echo lines hardcode the year "2026", which will become stale; update the workflow to substitute the current year dynamically (e.g., use the shell date command) when constructing the expected header message so the echoed lines reflect the current year; change the two echo statements that print the SPDX-FileCopyrightText and SPDX-License-Identifier to use a dynamic year variable (computed via date +%Y) instead of the literal "2026" and keep the existing COPYRIGHT_SUBSTR behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/pr.yaml:
- Around line 191-193: The informational echo lines hardcode the year "2026",
which will become stale; update the workflow to substitute the current year
dynamically (e.g., use the shell date command) when constructing the expected
header message so the echoed lines reflect the current year; change the two echo
statements that print the SPDX-FileCopyrightText and SPDX-License-Identifier to
use a dynamic year variable (computed via date +%Y) instead of the literal
"2026" and keep the existing COPYRIGHT_SUBSTR behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c268ad1c-d22f-4269-b504-aff44cc89dbe
📒 Files selected for processing (1)
.github/workflows/pr.yaml
Good call — I didn't realize the lint job's prek run --all-files --stage pre-push already covers the SPDX hook. That makes this redundant. Happy to close the PR. |
3 participants
Summary
Hooks
scripts/check-spdx-headers.shinto the PR workflow so missing license headers get caught in CI. The script was already wired into prek pre-commit hooks with--fix, but contributors who skip local hooks could push files without headers and nobody would notice until review.Related Issue
Fixes #551
Changes
check-spdx-headersjob to.github/workflows/pr.yaml.ts,.js,.py,.sh,.yaml,.yml) against PR basenode_modules/,dist/,docs/,.venv/)--fix) — fails with file list + expected header on missing headersType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Ran
check-spdx-headers.shlocally against existing source files — all pass. Confirmed it correctly rejects a test file without the header (exits 1, prints filename).Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Summary by CodeRabbit