feat(bazel): enforce that BUILD files match their sources - #491
Merged
Conversation
|
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 (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughBazel definitions now cover additional CLI tests, API key tests, client dependencies, and the admin-mock binary. A Gazelle test and CI helper validate BUILD files, with the build-test workflow invoking both helper scripts. ChangesBazel target coverage
Gazelle test configuration
CI Gazelle enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant build-test workflow
participant test-check-gazelle
participant check-gazelle
participant Bazel/Gazelle
build-test workflow->>test-check-gazelle: Run behavioral checks
test-check-gazelle->>check-gazelle: Invoke with stubbed BAZEL
check-gazelle->>Bazel/Gazelle: Run //:gazelle --mode=diff
Bazel/Gazelle-->>check-gazelle: Return status and output
check-gazelle-->>test-check-gazelle: Report clean, stale, or execution failure
build-test workflow->>check-gazelle: Run repository BUILD check
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ci/check-gazelle`:
- Around line 47-64: Update the Gazelle result handling after the command in the
check-gazelle script to inspect the captured status before the no-diff success
path. For any non-zero status, report the failure and exit nonzero; only use the
diff-specific exit code when unified-diff markers are present, while preserving
exit 0 for successful runs without diffs.
- Around line 31-77: add a focused shell test named test-check-gazelle that
stubs BAZEL and exercises check-gazelle’s three outcomes: exit 0 when no unified
diff is produced, exit 1 when Gazelle emits a generated diff, and exit 2 for
invocation failures such as ERROR/FATAL output. Assert each status and keep the
test isolated from the real Bazel/Gazelle commands.
🪄 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: 25675416-2de6-48b2-a7b5-a35074dde5c6
📒 Files selected for processing (7)
.github/workflows/build-test.ymlsrc/clis/nvcf-cli/cmd/BUILD.bazelsrc/clis/nvcf-cli/internal/apikeys/BUILD.bazelsrc/clis/nvcf-cli/internal/client/BUILD.bazelsrc/clis/nvcf-cli/scripts/admin-mock/BUILD.bazelsrc/libraries/go/lib/BUILD.bazeltools/ci/check-gazelle
Contributor
Author
|
Both findings fixed in the latest push. Unchecked exit status. This was a real false green and worth stating plainly: the script captured gazelle's status and never looked at it, so any failure printing neither an Non-zero with diff markers stays exit 1, since gazelle exits non-zero whenever it emits a diff and that is the normal out-of-date signal. Non-zero without them is now exit 2. Missing test.
I checked the test catches the regression rather than merely passing today: reverting the status check makes it fail with Both this and |
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 `@tools/ci/test-check-gazelle`:
- Around line 41-50: Update the substring check in expect() to match substr as a
literal fixed string and avoid the printf-to-grep pipeline; use here-string
input with grep’s fixed-string and quiet options while preserving the existing
failure reporting behavior.
- Around line 21-30: Update make_bazel to record its received arguments and
current working directory before producing the fixed payload, then add focused
assertions that check-gazelle invokes Bazel with --mode=diff, //:gazelle, the
expected Go roots, and the correct working directory. Keep the existing output
and exit-status classification assertions intact.
🪄 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: b61a8751-eefb-4113-8b1e-15a899a3fa61
📒 Files selected for processing (3)
.github/workflows/build-test.ymltools/ci/check-gazelletools/ci/test-check-gazelle
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-test.yml
- tools/ci/check-gazelle
kristinapathak
approved these changes
Jul 28, 2026
Contributor
Author
|
Both fixed in the latest push. Asserting the invocation. Good catch, and the scope check was the part that most needed it. Go-only is a deliberate decision — Gazelle also wants to reorder attributes in the The stub now records its working directory and argv. The tests assert Literal matching. Switched to Verified the new assertions bite rather than merely passing today:
|
Phase 2 of docs/dev/bazel-consolidation.md. Nothing in CI noticed when a BUILD file fell behind its sources, and the consequences were not theoretical. Running Gazelle from the root found four test files that Bazel was never building. apikey_test.go, deploy_test.go and root_test.go were missing from //src/clis/nvcf-cli/cmd, and internal/apikeys had a client_test.go with no go_test target at all. They exist in the repository and have never run in CI. Both targets pass once declared, so this is coverage that was silently absent rather than tests that were failing. That is the same failure mode as nvsnap, whose BUILD file was last regenerated twelve days before the sources it was meant to build. tools/ci/check-gazelle runs Gazelle from the root in --mode=diff, so it reports rather than rewrites and leaves no dirty tree. It distinguishes "there is a diff" from "gazelle failed", because a fetch or Starlark error must not read as tidy BUILD files. Scope is Go packages only. Gazelle also wants to reorder attributes in the src/libraries/java BUILD files, which is cosmetic and would churn files the Java modules own for no functional gain. Those modules move into the root in Phase 6; widen this then, as part of that move. One manual fix on top of the generated output: Gazelle hoists the rules_shell load in src/libraries/go/lib and carried a comment with it that describes the sh_test target, not the load. The comment is back on sh_test, and the check is idempotent with it there. Known gaps, deliberately left: two BUILD files Gazelle cannot merge (nvsnap/ui, go/worker/test/testutils) and three imports under tests/bdd that are not valid module paths. Neither blocks this check. Runs in build-test, not bazel.yml. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review caught a false green. The script captured gazelle's exit status and never checked it, so any failure that printed neither an ERROR/FATAL prefix nor unified-diff markers fell through to the success path and reported "BUILD files are up to date" for a run that verified nothing. A crashed server, a killed process, an unrecognised failure mode: all of them read as tidy BUILD files. That is precisely the class of silent pass this check was added to eliminate, so it mattered more here than it would in most scripts. Non-zero with diff markers stays exit 1, since gazelle exits non-zero whenever it produces a diff and that is the normal "out of date" signal. Non-zero without them is now exit 2. tools/ci/test-check-gazelle covers all three outcomes with a stubbed BAZEL, so nothing touches a real Bazel or the network: clean exits 0, diff exits 1 whether gazelle reports zero or non-zero, and ERROR, FATAL and silent non-zero exits all fail closed with 2. Verified the test catches the regression rather than only passing today. Reverting the status check makes it fail with "want exit 2, got 0", which is the exact symptom reported. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The stub ignored its arguments, so the suite verified only how check-gazelle classified gazelle's output. It would still have passed if the script dropped --mode=diff and started rewriting the tree, renamed the target, ran from the wrong directory, or changed which roots it scopes. Everything about the invocation was untested. The scope is the part that most needed a guard. Go-only is a deliberate decision: Gazelle also wants to reorder attributes in the src/libraries/java BUILD files, which is cosmetic and would churn files the Java modules own. Nothing recorded that intent in a way a change could trip over. The stub now records its working directory and argv, and the tests assert the target, --mode=diff, the repository root as cwd, each of the five Go roots, and that src/libraries/java is absent. Also switches assertions to fixed-string matching. The helper passed its needle to grep as a regex, so "new_test.go" would have accepted "newXtest.go", and it used a pipeline where grep exiting early could SIGPIPE the producer and trip pipefail on large output. Verified the new assertions fail when the behaviour changes rather than only passing today: adding src/libraries/java to the scope fails the exclusion check, and removing --mode=diff fails the diff-mode check. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
balajinvda
force-pushed
the
feat/bazel-gazelle-enforcement
branch
from
July 28, 2026 21:24
dda92d7 to
82b4ae2
Compare
balajinvda
enabled auto-merge
July 28, 2026 21:25
Contributor
Author
|
🎉 This PR is included in version nvcf-cli-v1.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
balajinvda
added a commit
that referenced
this pull request
Jul 29, 2026
CI's BUILD-file check (#491) failed: pathsafe_write_test.go was a new file absent from internal/agent/BUILD.bazel srcs. Adding it to BUILD would work, but the package already pairs one test file per source file and pathsafe_test.go was the obvious home. Same tests, no BUILD churn, and one fewer place to look for coverage of pathsafe.go. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
added a commit
that referenced
this pull request
Jul 29, 2026
CI's BUILD-file check (#491) failed: pathsafe_write_test.go was a new file absent from internal/agent/BUILD.bazel srcs. Adding it to BUILD would work, but the package already pairs one test file per source file and pathsafe_test.go was the obvious home. Same tests, no BUILD churn, and one fewer place to look for coverage of pathsafe.go. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.com>
balajinvda
added a commit
that referenced
this pull request
Aug 3, 2026
CI's "Check BUILD files match their sources" failed: internal/manifests and internal/manifests/gen had no BUILD.bazel at all. Both packages were added on this branch before that check landed on main (#491), so the merge is what surfaced it rather than anything new here. Generated with `bazel run //:gazelle` as the check instructs, not hand-written, so the result is byte-for-byte what CI regenerates and compares against. Gazelle also rewrote eight unrelated BUILD files under src/libraries/java/nv-boot-parent/. Those are outside this PR and outside the check's scope (check-gazelle passes only the Go roots, and its own test asserts "leaves src/libraries/java out of scope"), so they were reverted rather than swept in. Verified by running the failing job's steps locally: check-gazelle now reports "BUILD files are up to date", and test-check-gazelle, check-nested-modules and test-check-nested-modules all pass. Co-Authored-By: Balaji Ganesan <bganesan@nvidia.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
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.
Why
Phase 2 of
docs/dev/bazel-consolidation.md(#445).Nothing in CI notices when a BUILD file falls behind its sources, and the consequence is not theoretical. Running Gazelle from the root surfaced four test files that Bazel was never building:
apikey_test.go//src/clis/nvcf-cli/cmdsrcsdeploy_test.go//src/clis/nvcf-cli/cmdsrcsroot_test.go//src/clis/nvcf-cli/cmdsrcsclient_test.go//src/clis/nvcf-cli/internal/apikeysgo_testtarget at allThese exist in the repository and have never run in CI. Both targets pass once declared, so this is coverage that was silently absent, not tests that were failing.
That is the same failure mode as
nvsnap, whoseinternal/agent/BUILD.bazelwas last regenerated 2026-07-02 whilecheckpoint_v2.goandrestore_v2.golanded 2026-07-14 — twelve days broken with nothing reporting it.What changed
tools/ci/check-gazelleruns Gazelle from the repository root in--mode=diff, so it reports rather than rewrites and never leaves a dirty tree. The failure output is the patch to apply.build-test, notbazel.yml.Scope
Go packages only. Gazelle also wants to reorder attributes in the
src/libraries/javaBUILD files:java_library( name = "optional_compile_deps", - exports = CORE_OPTIONAL_COMPILE_DEPS, neverlink = True, visibility = ["//visibility:private"], + exports = CORE_OPTIONAL_COMPILE_DEPS, )That is cosmetic and would churn 8 files the Java modules own for no functional gain. Those modules move into the root module in Phase 6; widening the check belongs in that change rather than ahead of it.
Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Testing
bazel test //src/clis/nvcf-cli/cmd:cmd_test //src/clis/nvcf-cli/internal/apikeys:apikeys_test— both passtools/ci/check-gazelleis idempotent: clean after the regeneration, including after the manual comment fix belowNotes
One manual fix on top of Gazelle's output. It hoists the
rules_shellload insrc/libraries/go/lib/BUILD.bazelto the top of the file and carried a comment along with it — a comment that describes thesh_testtarget, not the load. The comment is reattached tosh_test, and the check stays clean with it there.Two known gaps, deliberately left and not blocking this check:
src/compute-plane-services/nvsnap/ui/BUILD.bazelorsrc/libraries/go/worker/test/testutils/BUILD.bazel(could not merge expression)tests/bddare not valid Go module paths:nvcf-bdd/dsl,nvcf-bdd/harness,nvcf-bdd/stepsReferences
None
Related Merge Requests/Pull Requests
Phase 2 companion: #484 (nested-module guard). Plan: #445.
Dependencies
None
Summary by CodeRabbit