From ecc67984c10639c2324cf38b7730634df69d31b5 Mon Sep 17 00:00:00 2001 From: admin Date: Mon, 26 Oct 2026 15:16:19 -0400 Subject: [PATCH] test(repoqa): package-size and fan-in ratchets for god packages (D-0033) The 300-line file cap cannot see a package that grows without bound or one that everything imports. Packages over 6000 non-test lines or 20 importers now need a ceiling in a shrink-only list; growth and new offenders fail. --- internal/repoqa/Architecture-Guardrails.md | 12 + internal/repoqa/package_budget_test.go | 261 ++++++++++++++++++ .../testdata/package-fanin-allowlist.txt | 10 + .../testdata/package-size-allowlist.txt | 13 + 4 files changed, 296 insertions(+) create mode 100644 internal/repoqa/package_budget_test.go create mode 100644 internal/repoqa/testdata/package-fanin-allowlist.txt create mode 100644 internal/repoqa/testdata/package-size-allowlist.txt diff --git a/internal/repoqa/Architecture-Guardrails.md b/internal/repoqa/Architecture-Guardrails.md index a4539f179..cd8d385c4 100644 --- a/internal/repoqa/Architecture-Guardrails.md +++ b/internal/repoqa/Architecture-Guardrails.md @@ -33,3 +33,15 @@ check that fails closed in CI; a syntactically valid ticket ID is insufficient. HQ overview section 4.2 is maintained separately. Its accepted-edge count and pending description must be reconciled with `acceptedBasis` by the HQ owner; the CLI tests do not depend on a private sibling checkout in public CI. + +## Package size and fan-in + +`TestPackageSizeBudget` and `TestPackageFanInBudget` (D-0033) cover what the +300-line file cap cannot see. A package with more than 6000 non-test Go lines, +or one imported by more than 20 other packages of the module, needs a line in +`testdata/package-size-allowlist.txt` or `testdata/package-fanin-allowlist.txt` +and may not grow past it. A new package over either threshold fails until it is +split along domain lines. The lists are ceilings: a shrinking package passes, +and `go test ./internal/repoqa -update` lowers the line or drops it once the +package is under the threshold. The update never adds a line or raises a number. +Fan-in counts distinct importing packages from non-test files only. diff --git a/internal/repoqa/package_budget_test.go b/internal/repoqa/package_budget_test.go new file mode 100644 index 000000000..851cb6a25 --- /dev/null +++ b/internal/repoqa/package_budget_test.go @@ -0,0 +1,261 @@ +package repoqa + +import ( + "bytes" + "fmt" + "go/parser" + "go/token" + "io/fs" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// Package budget (D-0033): the per-file 300-line cap cannot see a package that +// stays under it file by file while growing without bound, or one that every +// other package leans on. Two ratchets close that gap. +// +// Size: a package whose non-test Go code exceeds pkgLOCThreshold lines must +// +// have a line in testdata/package-size-allowlist.txt (" "); +// it may not grow past that ceiling. A new package that crosses the +// threshold is refused until it is split or a reviewer adds a line. +// +// Fan-in: a package imported by more than pkgFanInThreshold other packages of +// +// the module must have a line in testdata/package-fanin-allowlist.txt +// (" "); the importer count may not grow past it. +// +// Both lists are ceilings, not exact counts: a package that shrinks passes +// without an edit, and `go test ./internal/repoqa -update` lowers the line (or +// drops it once the package is under the threshold). The update only ever +// lowers; it never adds a line or raises a ceiling. The header of each list +// says how to read it. +// +// Scope: every non-_test.go .go file of the root module, whatever its build +// tags. Skipped: vendor/, testdata/, dot-dirs and nested modules (the same +// rules as the funnel scan). Fan-in counts distinct importing packages, from +// non-test files only; a test-only import does not add coupling to the binary. +const ( + pkgLOCThreshold = 6000 + pkgFanInThreshold = 20 + + pkgSizeList = "testdata/package-size-allowlist.txt" + pkgFanInList = "testdata/package-fanin-allowlist.txt" +) + +// pkgMetrics are the measured numbers of one package directory. +type pkgMetrics struct { + loc int + importers map[string]bool +} + +// scanPackages walks the module rooted at root and returns the metrics of +// every package directory, keyed by module-relative slash path ("." is the +// module root). module is the module path from go.mod. +func scanPackages(root, module string) (map[string]*pkgMetrics, error) { + pkgs := map[string]*pkgMetrics{} + get := func(dir string) *pkgMetrics { + m, ok := pkgs[dir] + if !ok { + m = &pkgMetrics{importers: map[string]bool{}} + pkgs[dir] = m + } + return m + } + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, werr error) error { + if werr != nil { + return werr + } + if d.IsDir() { + if p == root { + return nil + } + name := d.Name() + if name == "vendor" || name == "testdata" || strings.HasPrefix(name, ".") { + return filepath.SkipDir + } + if _, serr := os.Stat(filepath.Join(p, "go.mod")); serr == nil { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + rel, rerr := filepath.Rel(root, filepath.Dir(p)) + if rerr != nil { + return rerr + } + dir := filepath.ToSlash(rel) + src, rerr := os.ReadFile(p) + if rerr != nil { + return rerr + } + file, perr := parser.ParseFile(token.NewFileSet(), p, src, parser.ImportsOnly|parser.SkipObjectResolution) + if perr != nil { + return perr + } + get(dir).loc += bytes.Count(src, []byte("\n")) + for _, imp := range file.Imports { + ip, uerr := strconv.Unquote(imp.Path.Value) + if uerr != nil || (ip != module && !strings.HasPrefix(ip, module+"/")) { + continue + } + target := "." + if ip != module { + target = strings.TrimPrefix(ip, module+"/") + } + if target != dir { + get(target).importers[dir] = true + } + } + return nil + }) + return pkgs, err +} + +// overThreshold returns, for every package whose metric exceeds threshold, the +// measured value. Only packages that exist in pkgs and have code are counted; +// a directory that is only imported (no non-test source) has loc 0. +func overThreshold(pkgs map[string]*pkgMetrics, threshold int, metric func(*pkgMetrics) int) countList { + out := countList{} + for dir, m := range pkgs { + if n := metric(m); n > threshold { + out[dir] = n + } + } + return out +} + +// comparePackageBudget returns one finding per package that is over the +// threshold without a line, over its ceiling, or listed but no longer over the +// threshold. unit names the metric in the messages. +func comparePackageBudget(unit string, threshold int, list, over countList) []string { + var out []string + for _, dir := range sortedKeys(over) { + n := over[dir] + m, ok := list[dir] + switch { + case !ok: + out = append(out, fmt.Sprintf("%s: %s has %d, over the %d threshold and not in the list; split it along domain lines", unit, dir, n, threshold)) + case n > m: + out = append(out, fmt.Sprintf("%s: %s has %d, ceiling is %d; the package may not grow, move code out of it", unit, dir, n, m)) + } + } + for _, dir := range sortedKeys(list) { + if _, ok := over[dir]; !ok { + out = append(out, fmt.Sprintf("%s: %s is under the %d threshold; delete the line (go test ./internal/repoqa -update)", unit, dir, threshold)) + } + } + sort.Strings(out) + return out +} + +// runPackageRatchet is the shared body of the size and fan-in tests. +func runPackageRatchet(t *testing.T, unit, list string, threshold int, metric func(*pkgMetrics) int) { + t.Helper() + root := repoRoot(t) + module, err := modulePath(filepath.Join(root, "go.mod")) + if err != nil { + t.Fatalf("module path: %v", err) + } + pkgs, err := scanPackages(root, module) + if err != nil { + t.Fatalf("scan: %v", err) + } + over := overThreshold(pkgs, threshold, metric) + listPath := filepath.Join(root, "internal", "repoqa", filepath.FromSlash(list)) + recorded := readCountList(t, listPath) + if *update { + // Lines whose package is no longer over the threshold are dropped, the + // rest are lowered to the measured value, never raised. + writeCountListDown(t, listPath, recorded, over) + recorded = readCountList(t, listPath) + } + if findings := comparePackageBudget(unit, threshold, recorded, over); len(findings) > 0 { + t.Fatalf("%s ratchet:\n%s", unit, strings.Join(findings, "\n")) + } +} + +// TestPackageSizeBudget refuses package growth past the recorded ceilings. +func TestPackageSizeBudget(t *testing.T) { + runPackageRatchet(t, "package-size", pkgSizeList, pkgLOCThreshold, func(m *pkgMetrics) int { return m.loc }) +} + +// TestPackageFanInBudget refuses growth of the number of importing packages. +func TestPackageFanInBudget(t *testing.T) { + runPackageRatchet(t, "package-fan-in", pkgFanInList, pkgFanInThreshold, func(m *pkgMetrics) int { return len(m.importers) }) +} + +// TestScanPackages pins what the scan counts: non-test lines, distinct +// importing packages, no self import, no external import, and the skipped dirs. +func TestScanPackages(t *testing.T) { + root := t.TempDir() + files := map[string]string{ + "internal/a/a.go": "package a\n\nimport _ \"example.com/m/internal/b\"\n", + "internal/a/a2.go": "package a\n\nimport _ \"example.com/m/internal/b\"\nimport _ \"example.com/m/internal/a\"\n", + "internal/a/a_test.go": "package a\n\nimport _ \"example.com/m/internal/c\"\n", + "internal/b/b.go": "package b\n\nimport _ \"example.com/other/internal/c\"\n", + "cmd/x/x.go": "package x\n\nimport _ \"example.com/m/internal/b\"\nimport _ \"example.com/m\"\n", + "main.go": "package main\n", + "vendor/v/v.go": "package v\n\nimport _ \"example.com/m/internal/b\"\n", + "internal/b/testdata/t.go": "package t\n\nimport _ \"example.com/m/internal/b\"\n", + "nested/go.mod": "module nested\n", + "nested/n.go": "package n\n\nimport _ \"example.com/m/internal/b\"\n", + } + for name, body := range files { + p := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + pkgs, err := scanPackages(root, "example.com/m") + if err != nil { + t.Fatal(err) + } + if got := pkgs["internal/a"].loc; got != 7 { + t.Errorf("internal/a loc = %d, want 7 (test file excluded)", got) + } + var b []string + for imp := range pkgs["internal/b"].importers { + b = append(b, imp) + } + sort.Strings(b) + if want := "cmd/x,internal/a"; strings.Join(b, ",") != want { + t.Errorf("internal/b importers = %v, want %s (vendor, testdata, nested module skipped)", b, want) + } + if got := len(pkgs["internal/a"].importers); got != 0 { + t.Errorf("internal/a importers = %d, want 0 (self import and test import ignored)", got) + } + if got := pkgs["."].importers; !got["cmd/x"] || len(got) != 1 { + t.Errorf("module root importers = %v, want cmd/x", got) + } + if _, ok := pkgs["internal/c"]; ok { + t.Error("internal/c is only imported from a test file and an external module path; it must not appear") + } +} + +// TestComparePackageBudget covers the three findings and the passing shapes. +func TestComparePackageBudget(t *testing.T) { + list := countList{"p/above": 100, "p/shrunk": 100, "p/stale": 50, "p/same": 80} + over := countList{"p/above": 101, "p/shrunk": 90, "p/same": 80, "p/new": 70} + got := comparePackageBudget("package-size", 60, list, over) + want := []string{ + "package-size: p/above has 101, ceiling is 100; the package may not grow, move code out of it", + "package-size: p/new has 70, over the 60 threshold and not in the list; split it along domain lines", + "package-size: p/stale is under the 60 threshold; delete the line (go test ./internal/repoqa -update)", + } + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Fatalf("findings:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } + if extra := comparePackageBudget("x", 1, countList{"a": 2}, countList{"a": 2}); len(extra) != 0 { + t.Fatalf("clean list produced findings: %v", extra) + } +} diff --git a/internal/repoqa/testdata/package-fanin-allowlist.txt b/internal/repoqa/testdata/package-fanin-allowlist.txt new file mode 100644 index 000000000..2aead9e80 --- /dev/null +++ b/internal/repoqa/testdata/package-fanin-allowlist.txt @@ -0,0 +1,10 @@ +# Package fan-in ceilings (D-0033), checked by TestPackageFanInBudget. +# " ": distinct packages of this module that import the package +# from non-test files. A package imported by more than 20 needs a line here and +# may not gain importers past it. The list only shrinks: lower a line with +# `go test ./internal/repoqa -update`, delete it once the count is 20 or less. +internal/compat 25 +internal/config 37 +internal/docker 27 +internal/errs 58 +internal/httptimeout 25 diff --git a/internal/repoqa/testdata/package-size-allowlist.txt b/internal/repoqa/testdata/package-size-allowlist.txt new file mode 100644 index 000000000..d585b87b7 --- /dev/null +++ b/internal/repoqa/testdata/package-size-allowlist.txt @@ -0,0 +1,13 @@ +# Package size ceilings (D-0033), checked by TestPackageSizeBudget. +# " ": non-test Go lines of the package. A package over 6000 lines +# needs a line here and may not grow past it. The list only shrinks: lower a +# line with `go test ./internal/repoqa -update`, delete it once the package is +# under 6000. A new package over 6000 is split by domain, not added here. +cmd/commands 65362 +internal/backup 6279 +internal/build 13575 +internal/config 7498 +internal/database 10048 +internal/deploy 6667 +internal/doctor 9414 +internal/plugin 10079