ci: block banned files in pull request diffs#2475
Open
WilliamK112 wants to merge 2 commits intoNVIDIA:mainfrom
Open
ci: block banned files in pull request diffs#2475WilliamK112 wants to merge 2 commits intoNVIDIA:mainfrom
WilliamK112 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a banned-files detection flow: a Node.js CLI inspects git diffs for sensitive/generated files; GitHub Actions PR workflow runs the check using full git history; Vitest tests validate detection and allowed exceptions. Changes
Sequence DiagramsequenceDiagram
actor GH as GitHub Actions
participant WF as PR Workflow
participant Git as Git
participant Script as check-banned-files.mjs
participant Rules as Block Rules
participant Out as Output/Exit
GH->>WF: PR triggers workflow
WF->>Git: fetch base branch (origin)
WF->>Script: run with base ref & HEAD
Script->>Git: git diff --name-only base...HEAD
Git-->>Script: list of changed files
Script->>Script: normalize & filter fixture paths
loop per file
Script->>Rules: evaluate file against block rules
Rules-->>Script: match/no-match
end
alt blocked files found
Script->>Out: print violations
Script->>Out: exit 1
else no violations
Script->>Out: print clean message
Script->>Out: exit 0
end
Out-->>WF: status reported
WF-->>GH: job result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/banned-files-guard.test.ts (1)
27-35: Clean up temporary repos after each test run.
makeRepo()creates temp directories but never removes them. AddingafterEachcleanup keeps local/dev CI environments tidy.Suggested patch
-import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; @@ const REPO_ROOT = path.join(import.meta.dirname, ".."); const SCRIPT = path.join(REPO_ROOT, "scripts", "check-banned-files.mjs"); +const TEMP_REPOS: string[] = []; @@ function makeRepo() { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-banned-files-")); + TEMP_REPOS.push(dir); git(dir, ["init", "-b", "main"]); @@ return { dir, base }; } + +afterEach(() => { + for (const dir of TEMP_REPOS.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/banned-files-guard.test.ts` around lines 27 - 35, makeRepo() creates temporary repositories but never deletes them; modify the test file to track created temp dirs (from makeRepo) and add an afterEach hook that removes each directory (use fs.rmSync or equivalent with recursive/force) to clean up after tests; reference the makeRepo helper and ensure the afterEach cleans any dirs pushed to the tracking array so CI/local runs don't accumulate temp repos.
🤖 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/check-banned-files.mjs`:
- Around line 9-60: BLOCK_RULES currently omits several sensitive patterns
present in our .gitignore; update the BLOCK_RULES array to add rules (or extend
existing matches functions) to catch private key and credential file patterns
such as *_rsa, *_ed25519, *_ecdsa, *.jks, *.keystore, .netrc, .npmrc, *.tfvars,
and the .direnv directory; implement each as either new objects with unique ids
(e.g., "ssh-keys", "java-keystores", "netrc-npmrc", "terraform-vars", "direnv")
and a matches: (filePath) => { ... } predicate using path.posix.basename and
regexes to mirror the style of existing entries, and ensure case-insensitive
matching where appropriate so these files are rejected by the existing check
logic.
---
Nitpick comments:
In `@test/banned-files-guard.test.ts`:
- Around line 27-35: makeRepo() creates temporary repositories but never deletes
them; modify the test file to track created temp dirs (from makeRepo) and add an
afterEach hook that removes each directory (use fs.rmSync or equivalent with
recursive/force) to clean up after tests; reference the makeRepo helper and
ensure the afterEach cleans any dirs pushed to the tracking array so CI/local
runs don't accumulate temp repos.
🪄 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: fe20e59c-646d-449f-985f-f9a1a98038a4
📒 Files selected for processing (3)
.github/workflows/pr.yamlscripts/check-banned-files.mjstest/banned-files-guard.test.ts
WilliamK112
force-pushed
the
ci/banned-files-guard
branch
from
b2890c7 to
45e1992
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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:
- Around line 53-57: Replace the mutable github.base_ref with the immutable pull
request base SHA and fetch that commit so the PR diff is stable: change the git
fetch target from "origin/${{ github.base_ref }}" to the captured commit
"origin/${{ github.event.pull_request.base.sha }}" (or fetch that SHA
explicitly) and pass that SHA to node scripts/check-banned-files.mjs instead of
github.base_ref; ensure the fetch command fetches the referenced commit (remove
the --depth=1 shallow ref if needed) so merge-base calculation and
scripts/check-banned-files.mjs receive an immutable base.
🪄 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: f99371f9-4a6c-478a-8867-bbb875c4b351
📒 Files selected for processing (3)
.github/workflows/pr.yamlscripts/check-banned-files.mjstest/banned-files-guard.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/banned-files-guard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/check-banned-files.mjs
Contributor
Author
|
Follow-up update after review:
|
wscurran
added
CI/CD
Contributor
|
✨ Thanks for submitting this pull request that proposes a way to block banned files in pull request diffs. Related open issues: |
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
2 participants
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.
Summary
Testing
npm test -- test/banned-files-guard.test.tsnpx eslint scripts/check-banned-files.mjs test/banned-files-guard.test.tsRelated
Summary by CodeRabbit
New Features
Tests