amadan
Fix guard-blind-spot review findings: tree matching, not exact-dir
Review found the guards checking file location or import identity by exact string/directory equality rather than tree membership, so a future subpackage would go unchecked instead of exempted or caught — the exact failure mode these guards exist to prevent, happening inside the guard itself. - calc_isolation_test.go (Important): calcHome was matched against filepath.Dir(rel) directly, so only files whose *immediate* parent is internal/sheets/calc were scanned. A calc/functions subpackage would be invisible. Now uses inTree() (prefix match) so the whole calc tree is covered. - calc_isolation_test.go (Minor): the store forbidden-import comparison was exact equality, so a hypothetical store/schema subpackage could be imported undetected. Now uses importUnder() (prefix match) for the store target. The internal/sheets-itself target stays exact-match on purpose — treating it as a tree would also silently forbid calc from importing sibling packages (format, xlsx, seal, sealtest, guards) that the rule never named; that's a real rule change, not a blind-spot fix, and not this review's ask. - excelize_confinement_test.go (Minor): same exact-dir pattern, but in the safe direction (a future xlsx subpackage would be wrongly blocked rather than wrongly allowed). Fixed for consistency, using the same inTree() helper. - linecap_test.go (Minor): countLines' comment claimed wc -l semantics it doesn't have (bufio.Scanner counts a final unterminated line; wc -l counts newline characters and would not). Comment now describes the actual, stricter behavior instead. Added inTree() and importUnder() to repo_test.go as the shared tree/prefix-matching helpers both fixes use. Verified: gofmt clean, go vet clean, all five guards still pass on the real tree, and the calc-isolation fix was proven live — created internal/sheets/calc/scratch/scratch.go importing store, confirmed TestCalcIsolatedFromStoreAndSheets now catches it (it would have been invisible under the old exact-dir-match code), then deleted the scratch package. Full `make test` green end to end (1m44.9s).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
4 files changed,
+62
−27
internal/guards/calc_isolation_test.go+26 −19internal/guards/excelize_confinement_test.go+9 −6internal/guards/linecap_test.go+8 −2internal/guards/repo_test.go+19 −0
Intent
diff --git a/internal/guards/calc_isolation_test.go b/internal/guards/calc_isolation_test.go| index 1e4ea45..9c8a12e 100644 |
| --- a/internal/guards/calc_isolation_test.go |
| +++ b/internal/guards/calc_isolation_test.go |
| @@ -1,38 +1,45 @@ |
| package guards |
| import ( |
| - "path/filepath" |
| "testing" |
| ) |
| -// calcHome is the formula engine's package. |
| +// calcHome is the formula engine's package tree — calc itself and any |
| +// subpackage of it (e.g. a future calc/functions). |
| const calcHome = "internal/sheets/calc" |
| -// calcForbiddenImports are import paths calc must never reach for: the |
| -// storage layer, and the app package (internal/sheets) that wires it to |
| -// storage and HTTP. calc is the pure formula engine — parsing, the |
| -// dependency graph, evaluation — and importing either would mean it is |
| -// no longer pure. |
| -var calcForbiddenImports = []string{ |
| - "amadan.net/oficina/sheets/internal/sheets/store", |
| - "amadan.net/oficina/sheets/internal/sheets", |
| -} |
| +// storeImport is the storage layer's own import path. Matched as a tree |
| +// (storeImport itself, or anything nested under it, e.g. a hypothetical |
| +// future store/schema) so a subpackage of store can't quietly slip past |
| +// an exact-equality check the way store's root package can't today. |
| +const storeImport = "amadan.net/oficina/sheets/internal/sheets/store" |
| + |
| +// sheetsImport is internal/sheets itself — the app/wiring package, not |
| +// its namespace. Deliberately matched by exact equality, not as a tree: |
| +// internal/sheets/format, /xlsx, /seal, /sealtest and /guards all live |
| +// under that same import prefix, and the rule as written ("internal/sheets |
| +// itself") names the one package, not everything nested under it. Widening |
| +// this to a tree match would silently forbid calc from importing any of |
| +// those siblings too — a real rule change, not a blind-spot fix, and one |
| +// nobody has decided to make. |
| +const sheetsImport = "amadan.net/oficina/sheets/internal/sheets" |
| -// TestCalcIsolatedFromStoreAndSheets fails on any file in |
| -// internal/sheets/calc that imports internal/sheets/store or |
| -// internal/sheets itself. |
| +// TestCalcIsolatedFromStoreAndSheets fails on any file in the |
| +// internal/sheets/calc tree that imports the store package (or any |
| +// future subpackage of it) or internal/sheets itself. |
| func TestCalcIsolatedFromStoreAndSheets(t *testing.T) { |
| root := repoRoot(t) |
| for _, path := range goFiles(t, root) { |
| rel := relPath(t, root, path) |
| - if filepath.ToSlash(filepath.Dir(rel)) != calcHome { |
| + if !inTree(rel, calcHome) { |
| continue |
| } |
| for _, imp := range fileImports(t, path) { |
| - for _, forbidden := range calcForbiddenImports { |
| - if imp == forbidden { |
| - t.Errorf("%s: imports %q, forbidden in %s (calc isolation)", rel, imp, calcHome) |
| - } |
| + switch { |
| + case importUnder(imp, storeImport): |
| + t.Errorf("%s: imports %q, forbidden in %s (calc isolation)", rel, imp, calcHome) |
| + case imp == sheetsImport: |
| + t.Errorf("%s: imports %q, forbidden in %s (calc isolation)", rel, imp, calcHome) |
| } |
| } |
| } |
diff --git a/internal/guards/excelize_confinement_test.go b/internal/guards/excelize_confinement_test.go| index 427b4ec..c74a8de 100644 |
| --- a/internal/guards/excelize_confinement_test.go |
| +++ b/internal/guards/excelize_confinement_test.go |
| @@ -1,25 +1,28 @@ |
| package guards |
| import ( |
| - "path/filepath" |
| "testing" |
| ) |
| // excelizeImport is the XLSX library's import path. |
| const excelizeImport = "github.com/xuri/excelize/v2" |
| -// excelizeHome is the only package allowed to import it. Confining |
| -// excelize's quirks to one package keeps the XLSX format's weirdness |
| -// from leaking into the rest of Sheets. |
| +// excelizeHome is the package tree allowed to import it — xlsx itself |
| +// and any subpackage of it. Matched as a tree, not by exact directory |
| +// equality: equality would make a future xlsx subpackage look like it |
| +// was outside home and wrongly forbid it from importing excelize (the |
| +// over-strict direction — a false positive, not a leak — but still |
| +// wrong). Confining excelize's quirks to this one tree keeps the XLSX |
| +// format's weirdness from leaking into the rest of Sheets. |
| const excelizeHome = "internal/sheets/xlsx" |
| // TestExcelizeConfinedToXLSXPackage fails on any excelize import outside |
| -// internal/sheets/xlsx. |
| +// the internal/sheets/xlsx tree. |
| func TestExcelizeConfinedToXLSXPackage(t *testing.T) { |
| root := repoRoot(t) |
| for _, path := range goFiles(t, root) { |
| rel := relPath(t, root, path) |
| - if filepath.ToSlash(filepath.Dir(rel)) == excelizeHome { |
| + if inTree(rel, excelizeHome) { |
| continue |
| } |
| for _, imp := range fileImports(t, path) { |
diff --git a/internal/guards/linecap_test.go b/internal/guards/linecap_test.go| index ec3cc51..e475cb6 100644 |
| --- a/internal/guards/linecap_test.go |
| +++ b/internal/guards/linecap_test.go |
| @@ -38,8 +38,14 @@ func TestFileLineCap(t *testing.T) { |
| } |
| } |
| -// countLines counts newline-terminated lines the way `wc -l` does, |
| -// which is what the 300-line figure in AGENTS.md was measured against. |
| +// countLines counts lines the way bufio.Scanner's ScanLines does: each |
| +// newline-terminated line, plus a final line even if it has no trailing |
| +// newline. That is NOT what `wc -l` reports — wc -l counts newline |
| +// characters, so a file whose last line lacks a trailing newline gets |
| +// one fewer from wc than from this. This function's count is the |
| +// stricter of the two (it can only report equal or one higher), and |
| +// gofmt already guarantees a trailing newline on every .go file this |
| +// test walks, so the difference never shows up here in practice. |
| func countLines(path string) (int, error) { |
| f, err := os.Open(path) |
| if err != nil { |
diff --git a/internal/guards/repo_test.go b/internal/guards/repo_test.go| index 6627e19..1d9fb71 100644 |
| --- a/internal/guards/repo_test.go |
| +++ b/internal/guards/repo_test.go |
| @@ -125,3 +125,22 @@ func fileImports(t *testing.T, path string) []string { |
| } |
| return imports |
| } |
| + |
| +// inTree reports whether rel — a repo-relative file path — lives inside |
| +// the directory tree rooted at dir (dir itself, or any subdirectory of |
| +// it, at any depth). Used instead of comparing filepath.Dir(rel) == dir |
| +// so a rule means "this whole package tree" and not just "files whose |
| +// immediate parent happens to match" — a future subpackage (e.g. |
| +// calc/functions) must inherit the rule, not silently fall outside it. |
| +func inTree(rel, dir string) bool { |
| + return strings.HasPrefix(rel, dir+"/") |
| +} |
| + |
| +// importUnder reports whether imp is base itself, or an import path |
| +// nested under base (base plus "/something"). Used instead of plain |
| +// equality so a forbidden import stays forbidden if it grows |
| +// subpackages later (e.g. a future .../store/schema), rather than the |
| +// check silently narrowing to just the package's root import path. |
| +func importUnder(imp, base string) bool { |
| + return imp == base || strings.HasPrefix(imp, base+"/") |
| +} |