fix(test): a guard file may name the module it forbids - #444
Conversation
main is red. #436 and #437 each added a guard and merged independently: the presentation-layer guard has to spell packageurl-go in order to ban it, and the module-boundary guard reports any file under internal/ that names it. Two rules doing their job, one flagging the other. The exemption is a set of canonical paths now. Not a name -- exempting anything called guards_test.go was the earlier bug in this same line, and it hid a forbidden import in a second guard file. Not one hard-coded path either, which is what made the guards collide the moment a second one existed. Adding a guard costs one line in that set, deliberately: a new exemption should be an edit somebody reviews, not a pattern that widens on its own. The predicate is extracted so the property can be pinned rather than described. TestGuardExemptionIsByPathNotByName fails if a file becomes exempt for being *named* like a guard, and if an entry names a file that no longer exists -- a dead exemption is a rule nobody is applying. The first mutation I ran against the old shape passed, which is how the missing test surfaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e99cea8f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isGuardFile(path) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The unconditional isGuardFile return exempts each listed file from every filesNamingModule check, not just from the module its own guard must name. For example, registry_lookup_guard_test.go legitimately names the official package-URL module, but it can now directly import github.com/github/go-spdx or the deprecated Anchore package-URL module without either repository guard reporting it, despite the rule covering all files under internal/, including tests. Track exemptions by both path and module so each guard only suppresses its intentional self-reference.
AGENTS.md reference: AGENTS.md:L122-L123
Useful? React with 👍 / 👎.
…de/adopt-sdk-0.9.5 Test on this PR fails on a collision inherited from main, not on anything this branch changed: #436's module-boundary guard reports #437's presentation guard, which has to spell packageurl-go in order to forbid it. #444 fixes it. Merging that branch in so this PR's CI reflects its own changes; the merge collapses when #444 lands on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main is red: #436 tightened TestNoDirectPackageURLUse while #437 added internal/output/registry_lookup_guard_test.go, whose own forbidden-string literal names the module the other guard forbids, so the two guards report each other. The failure is inherited by every branch cut from main, including this one, and is unrelated to the digest vocabulary change here. #444 fixes it and is green. Merging that branch rather than writing a second fix: it is the same commit, so when #444 lands on main this history dedupes with no conflict, and #443 never carries a competing version of the same fix. It touches only internal/detectors/guards_test.go, which this PR does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
mainis red right now. This fixes it. Reproduced onorigin/maindirectly, not just on a branch:Cause
#436 and #437 each added a guard and merged independently, and neither could have seen it alone:
packageurl-goin order to forbid it.internal/naming that module, exempting only itself.Two rules doing their jobs, one flagging the other. A guard file naming a module in a rule that bans it is the opposite of reaching for it.
Fix
The exemption is a set of canonical paths.
Not a name — exempting anything called
guards_test.gowas the earlier bug in this very line, and it hid a forbidden import in a second guard file. Not a single hard-coded path either, which is what made the two guards collide as soon as a second one existed.Adding a guard now costs one line in that set, on purpose: a new exemption should be an edit someone reviews, not a pattern that widens by itself.
The test exists because a mutation passed
My first mutation — reverting the predicate to a basename match — passed, which meant nothing pinned "exempt by path, not by name". The predicate is extracted now and
TestGuardExemptionIsByPathNotByNamecovers both failure directions:Re-run after extracting:
"../../internal/sbom/guards_test.go" is exempt for being named guards_test.go rather than for being a guard. Dropping the output guard from the set reproduces main's current failure.make verifygreen.Merge this before #440 — that PR is otherwise green and only red because it merged a broken
main.🤖 Generated with Claude Code