From 809420070bf2f82ef80b9d4b04e07d227ac86f4f Mon Sep 17 00:00:00 2001 From: Mathias Date: Sat, 3 Oct 2026 17:03:09 +0200 Subject: [PATCH] test(extract): enforce the raw/ open discipline with the Go parser PR #156 round 2: the FIFO fix lives in OpenRegular, and reverting any of its three callers to a plain Root.Open stayed green; the extract.Text guard was a text search that an aliased import or a func value passed. - TestRawReadersOpenOnlyThroughOpenRegular parses rawroot.go, extract.go and watcher.go and fails on any .Open/.OpenFile call outside OpenRegular, except a named os.OpenFile log append. - TestNoPackageReferencesExtractTextOutsideExtract resolves the extract import under any alias and fails on any reference to its Text, called or taken as a value. Each was flipped: the three caller reverts, an os.Open revert, an alias and a func value all go red. Refs #153 Co-Authored-By: Claude Opus 5.5 --- ingestion/internal/extract/callsites_test.go | 112 ++++++++++++++++--- 1 file changed, 94 insertions(+), 18 deletions(-) diff --git a/ingestion/internal/extract/callsites_test.go b/ingestion/internal/extract/callsites_test.go index 10be812..4ba7b01 100644 --- a/ingestion/internal/extract/callsites_test.go +++ b/ingestion/internal/extract/callsites_test.go @@ -1,43 +1,119 @@ package extract import ( + "go/ast" + "go/parser" + "go/token" "os" "path/filepath" + "strconv" "strings" "testing" ) -// PR #156 review: putting extract.Text(path) back at the ingest call sites -// stayed green, because only TextIn itself was tested. Reading a caller-chosen -// path by name reopens it after the containment check (hyperguild#153 item 3), -// so outside this package only TextIn may be used. -func TestNothingOutsideExtractCallsTextByPath(t *testing.T) { - root := filepath.Join("..", "..") - var offenders []string - err := filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error { +const extractPkg = "github.com/mathiasbq/hyperguild/ingestion/internal/extract" + +// parseNonTest parses every non-test Go file under the module. +func parseNonTest(t *testing.T) map[string]*ast.File { + t.Helper() + fset := token.NewFileSet() + out := map[string]*ast.File{} + err := filepath.WalkDir(filepath.Join("..", ".."), func(p string, d os.DirEntry, err error) error { if err != nil { return err } if d.IsDir() && (d.Name() == "vendor" || d.Name() == ".git") { return filepath.SkipDir } - if d.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") || - strings.Contains(filepath.ToSlash(p), "internal/extract/") { + if d.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { return nil } - b, err := os.ReadFile(p) - if err != nil { - return err - } - if strings.Contains(string(b), "extract.Text(") { - offenders = append(offenders, p) + f, perr := parser.ParseFile(fset, p, nil, 0) + if perr != nil { + return perr } + out[filepath.ToSlash(p)] = f return nil }) if err != nil { t.Fatal(err) } - if len(offenders) > 0 { - t.Fatalf("extract.Text( called outside internal/extract (use TextIn through an os.Root): %v", offenders) + return out +} + +// PR #156 review: reading a caller-chosen path by name reopens it after the +// containment check (hyperguild#153 item 3). Outside this package, nothing +// may reference extract.Text — called OR taken as a value — under whatever +// name the package is imported (round 2: an alias and a func value slipped +// past a plain text search). +func TestNoPackageReferencesExtractTextOutsideExtract(t *testing.T) { + for path, f := range parseNonTest(t) { + if strings.Contains(path, "internal/extract/") { + continue + } + names := map[string]bool{} + for _, imp := range f.Imports { + if p, _ := strconv.Unquote(imp.Path.Value); p == extractPkg { + name := "extract" + if imp.Name != nil { + name = imp.Name.Name + } + names[name] = true + } + } + if len(names) == 0 { + continue + } + ast.Inspect(f, func(n ast.Node) bool { + if sel, ok := n.(*ast.SelectorExpr); ok { + if id, ok := sel.X.(*ast.Ident); ok && names[id.Name] && sel.Sel.Name == "Text" { + t.Errorf("%s references %s.Text; use extract.TextIn through an os.Root", path, id.Name) + } + } + return true + }) + } +} + +// PR #156 review round 2: the FIFO fix (open non-blocking, check the handle) +// lives in OpenRegular, and reverting any caller to a plain Root.Open stayed +// green. In the raw/ readers, the only Open/OpenFile on anything is inside +// OpenRegular itself. +// osOpenAllowed names the functions in guarded files that may call os.Open* +// on a path that is not a raw/ read. +var osOpenAllowed = map[string]bool{"internal/watcher/watcher.go:appendWatcherLog": true} + +func TestRawReadersOpenOnlyThroughOpenRegular(t *testing.T) { + guarded := []string{"internal/api/rawroot.go", "internal/extract/extract.go", "internal/watcher/watcher.go"} + files := parseNonTest(t) + for _, g := range guarded { + var f *ast.File + for p, af := range files { + if strings.HasSuffix(p, g) { + f = af + } + } + if f == nil { + t.Fatalf("guarded file %s not found", g) + } + for _, decl := range f.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name == "OpenRegular" { + continue + } + ast.Inspect(fn, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if sel, ok := call.Fun.(*ast.SelectorExpr); ok && (sel.Sel.Name == "Open" || sel.Sel.Name == "OpenFile") { + if id, ok := sel.X.(*ast.Ident); ok && id.Name == "os" && osOpenAllowed[g+":"+fn.Name.Name] { + return true // a named non-raw write (the ingest log append) + } + t.Errorf("%s: %s calls .%s; raw/ reads must go through OpenRegular", g, fn.Name.Name, sel.Sel.Name) + } + return true + }) + } } }