feat(bazel): guard the nested-module inventory - #484
Merged
Conversation
Phase 2 of docs/dev/bazel-consolidation.md. Consolidation only converges if the count of nested modules moves in one direction, and nothing today notices when it does not: a new service arriving with its own MODULE.bazel is invisible until someone counts by hand. Add tools/ci/nested-modules.txt classifying all 20 nested modules on main, and tools/ci/check-nested-modules to enforce it in both directions. An unlisted nested module fails, because that is the thing consolidation exists to stop. A ledger entry whose module no longer exists also fails, so a completed migration must prune its line and the backlog cannot overstate itself. The ledger keeps the plan's three categories rather than collapsing to one allowlist, because only `exception` carries the exception contract. Merging them would make every outstanding migration read as an accepted exception. Today that is 18 awaiting migration, 1 vendored (cel.dev/expr), 1 scaffolding (rules/oci-destinations), and no permanent exceptions. nvsnap is absent entirely, which is what a finished migration looks like here. The check also prints the remaining count, so the phase has a running scoreboard instead of a periodic manual audit. Verified it fails on the real tree in both directions, not just that it passes today: adding a MODULE.bazel under src/clis/nvcf-cli is reported as unclassified, and removing ratelimiter's is reported as a stale entry. Runs in build-test, not bazel.yml, so it does not trigger the full Bazel matrix. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Bash inventory ledger and reconciliation check for nested Bazel modules, behavioral coverage for ledger and mismatch cases, and a CI step that runs both scripts. ChangesNested Bazel module inventory
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant InventoryCheck
participant Ledger
participant Repository
CI->>InventoryCheck: Run test and inventory checks
InventoryCheck->>Ledger: Read classified module paths
InventoryCheck->>Repository: Discover nested MODULE.bazel files
InventoryCheck-->>CI: Return counts or mismatch errors
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 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-nested-modules`:
- Around line 51-55: Update the read loop in check-nested-modules so every
non-comment row missing its required path exits with status 2 instead of being
silently skipped, including ledger and exception rows. Preserve
comment/blank-row handling, and add a behavioral test covering a root-only tree
with a malformed ledger row.
- Around line 65-68: Update the validation flow in check-nested-modules before
calculating metrics to detect duplicate entries within each category and paths
classified under multiple categories, including ledger and exception. Reject any
repeated or conflicting path classification, then retain the existing
deduplication and count logic only for validated unique classifications.
- Around line 71-73: Update the nested-module discovery pipeline in the find
invocation to stop suppressing errors: remove the stderr redirection and
trailing “|| true”, and explicitly prune the .git tree while searching. Preserve
the existing root exclusion, directory output, normalization, sorting, and
found-file behavior so invalid or unreadable roots cause the script to fail.
🪄 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: fa991de2-50c0-4c6b-9cf8-1f3ebfdad1e4
📒 Files selected for processing (4)
.github/workflows/build-test.ymltools/ci/check-nested-modulestools/ci/nested-modules.txttools/ci/test-check-nested-modules
Three review findings on the nested-module guard, all cases where it could report a clean result it had not actually established. A row naming a category with no path was silently skipped, so a typo could quietly shrink the guard's coverage while it still passed. It now reports the file and line and exits 2. Category files were not deduplicated, only the combined list was. A repeated `ledger svc/a` inflated the backlog count, and the same path classified as both `ledger` and `exception` inflated both while passing, misreporting how much of the migration is outstanding. One path now maps to exactly one category. Discovery suppressed find's errors and ended with `|| true`, so an unreadable tree produced a partial inventory rather than a failure. That is the same false-green pattern this guard exists to prevent, so errors now terminate and `.git` is pruned explicitly instead. Each is covered by a test, including that a MODULE.bazel planted inside .git is pruned rather than discovered. One of those tests initially asserted the wrong message because it passed /dev/null as the ledger, which is not a regular file, so the script exited earlier than intended; the fixture is a real empty file now. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-nested-modules`:
- Line 134: Update the test command around the nested-module check to create a
temporary directory, remove it, and pass the resulting guaranteed-missing path
as the --root argument instead of the hard-coded /nonexistent-root-xyz value;
preserve the existing output capture and return-code assertions.
🪄 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: b8fc8e05-5442-4ceb-8aad-2267a88757d9
📒 Files selected for processing (2)
tools/ci/check-nested-modulestools/ci/test-check-nested-modules
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/ci/check-nested-modules
Contributor
Author
|
All three findings fixed in the latest push, and each is covered by a test. Missing path — a row naming a category with no path was silently skipped, so a typo could quietly shrink the guard's coverage while it still passed. Now reports file and line, exits 2. Duplicate and conflicting classifications — only the combined list was deduplicated, not the per-category files. A repeated Masked discovery failures — this was the important one. Worth noting one of the new tests initially asserted the wrong error because it passed Unrelated: the earlier |
kristinapathak
approved these changes
Jul 28, 2026
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).Consolidation only succeeds if the number of nested Bazel modules moves in one direction, and nothing in CI notices when it does not. A new service arriving with its own
MODULE.bazelis invisible until someone counts by hand, and a service that finishes migrating leaves no trace that the backlog shrank. Both directions need to be enforced for the migration to be measurable rather than asserted.What changed
tools/ci/nested-modules.txtclassifies all 20 nested modules currently on main.tools/ci/check-nested-modulesenforces it both ways. An unlisted nested module fails, because that is precisely what consolidation exists to prevent. A ledger entry whose module no longer exists also fails, so a completed migration must prune its line and the backlog cannot overstate how much work remains.tools/ci/test-check-nested-modulescovers the behavior, including both failure modes.build-test, notbazel.yml, so it does not trigger the full Bazel matrix.The ledger keeps the plan's three categories instead of collapsing into a single allowlist, because only
exceptioncarries the exception contract. Merging them would make every outstanding migration read as an accepted exception.Current state, which the check prints on every run:
cel.dev/expr)rules/oci-destinations)nvsnapis absent from the file entirely, which is what a finished migration looks like here — it moved to the root module in #471.Customer Release Notes
Not customer visible.
Plan Summary
Not applicable.
Usage
Testing
Both scripts run in
build-test. Beyond the unit tests, I verified the guard fails on the real tree in both directions rather than only passing today:src/clis/nvcf-cli/MODULE.bazelis reported as unclassified, exit 1src/invocation-plane-services/ratelimiter/MODULE.bazelis reported as a stale ledger entry, exit 1Notes
The check prints the remaining count on every run, so the phase has a running scoreboard rather than a periodic manual audit. As services migrate, their lines leave the ledger and the count falls toward zero.
References
None
Related Merge Requests/Pull Requests
Plan: #445. Phase 5a (
nvsnap) landed in #471.Dependencies
None
Summary by CodeRabbit
CI Improvements
Documentation
Tests