From 4e99cea8f6db6221ec861185e7a908b7ae1eb784 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Wed, 9 Sep 2026 00:11:04 -0700 Subject: [PATCH] fix(test): a guard file may name the module it forbids 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 --- internal/detectors/guards_test.go | 55 +++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/internal/detectors/guards_test.go b/internal/detectors/guards_test.go index f49f6920..9728a846 100644 --- a/internal/detectors/guards_test.go +++ b/internal/detectors/guards_test.go @@ -113,6 +113,35 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) { } } +// guardFiles are the files whose job is to forbid a module, so they are the +// files that have to spell it. Exempting them is not a loophole -- naming a +// module in a rule that bans it is the opposite of reaching for it. +// +// An explicit set of canonical paths, for two reasons learned the hard way. +// Matching a name instead exempted every guards_test.go under internal/, so a +// second guard file anywhere could name a forbidden module unnoticed. And +// exempting only this one file made the guards flag each other the moment a +// second one existed: internal/output grew its own presentation-layer guard, +// which must name packageurl-go to forbid it, and this rule reported it. +// +// Adding a guard therefore costs one line here, on purpose. That is the point: +// a new exemption should be a deliberate edit somebody reviews, not a pattern +// that silently widens. +var guardFiles = map[string]struct{}{ + filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go")): {}, + filepath.Clean(filepath.Join(internalRoot, "output", "registry_lookup_guard_test.go")): {}, +} + +// isGuardFile reports whether a path is one of the guard files above. +// +// By path. Exempting anything named guards_test.go was the earlier bug: it +// meant a second guard file in any package could name a forbidden module and +// no rule would report it. +func isGuardFile(path string) bool { + _, ok := guardFiles[filepath.Clean(path)] + return ok +} + // filesNamingModule returns every Go file under internal/ -- test files // included -- whose text names the module path. Tests count because a test // reaching a library directly proves the hazard is still reachable, and a test @@ -127,13 +156,15 @@ func TestNoDirectSPDXExpressionUse(t *testing.T) { // the right direction to fail. func filesNamingModule(t *testing.T, module string) []string { t.Helper() - self := filepath.Clean(filepath.Join(internalRoot, "detectors", "guards_test.go")) var offenders []string err := filepath.Walk(internalRoot, func(path string, info os.FileInfo, err error) error { if err != nil { return err } - if info.IsDir() || !strings.HasSuffix(path, ".go") || filepath.Clean(path) == self { + if info.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + if isGuardFile(path) { return nil } body, err := os.ReadFile(path) @@ -279,3 +310,23 @@ func TestDetectionResultsCarryingGraphsAreAttributed(t *testing.T) { "wrap the result in detectors.Attributed: %v", offenders) } } + +// The exemption is a set of paths, and a file is not exempt for being named +// like a guard. Both halves have been wrong here before: matching the basename +// hid a forbidden import in a second guards_test.go, and exempting only one +// file made two guards report each other. +func TestGuardExemptionIsByPathNotByName(t *testing.T) { + for path := range guardFiles { + if _, err := os.Stat(path); err != nil { + t.Errorf("guard file %q does not exist; a dead exemption is a rule nobody is applying", path) + } + if !isGuardFile(path) { + t.Errorf("guard file %q is not recognized by its own predicate", path) + } + } + // A file named like a guard, in a package that has none, must not be + // exempt -- whether or not it exists today. + if impostor := filepath.Join(internalRoot, "sbom", "guards_test.go"); isGuardFile(impostor) { + t.Errorf("%q is exempt for being named guards_test.go rather than for being a guard", impostor) + } +}