diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9333dd5..adc9b8e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,14 +47,48 @@ build): tagging a release). - gosec, govulncheck, and semgrep clean; CodeQL gated against a baseline. -Structural ratchets (see the -[structural gates issue](https://github.com/txn2/m6t/issues/19)) are plain Go -tests that fail on architectural decay: package-size budgets, an import -ratchet, exported-surface budgets, a god-object budget on the backend -coordinator struct (AST field/method ceilings pinned to actuals), dead-package -and noop-interface detection, and an integration guard proving integration -tests actually ran. **Ceilings carry zero slack and only move down.** Raising -one is a regression that must be explicitly justified in the PR. +### Structural ratchets + +The per-function linters all evaluate code *inside* one function, so a +god-package assembled from a hundred small, tidy functions passes every one of +them. The structural gates bound what those linters cannot see. They are plain +Go tests in the repository root — no external tooling — so `make test` runs +them and `make verify` gates on them. + +| Gate | What it bounds | Where | +|---|---|---| +| Package size | Lines and files per package | `package_budget_test.go` | +| Package pin | Every package has a ratchet entry | `package_budget_test.go` | +| Exported surface | Package-scope exported identifiers | `surface_budget_test.go` | +| God-object | Fields and methods on the `App` coordinator | `godobject_budget_test.go` | +| Dead package | Every package is reachable from `main` | `package_graph_test.go` | +| Import graph | What is allowed to depend on what | `package_graph_test.go` | +| No-op interface | Interfaces implemented only by stubs | `noop_interface_test.go` | +| Integration guard | Tagged tests are actually executed | `integration_guard_test.go` | +| Frontend ratchet | ESLint suppressions only shrink | `frontend_ratchet_test.go` | +| Wiring guard | The gates above still run | `structural_gates_test.go` | + +**Ceilings carry zero slack and only move down.** Every ceiling is pinned at +the measured actual, next to the gate that enforces it, with a comment saying +what it is for. Raising one is a regression: it belongs in the PR that needs +it, on that line, with the reason. There is no suppression comment and no +escape hatch — the justification in review *is* the mechanism. + +Two deliberate exceptions, both documented at the constant: + +- **LOC ceilings carry headroom.** A line-count ceiling pinned to the exact + current count is a freeze, not a ratchet — one more line of doc comment would + fail the build. They are seeded as policy and re-pinned against real + measurements once the backend services land + ([#2](https://github.com/txn2/m6t/issues/2), + [#5](https://github.com/txn2/m6t/issues/5)). +- **The `App` coordinator's ceilings will rise as services land**, one composed + handle at a time, each in the PR that adds it. What the gate stops is the + accumulation nobody decided on. + +The ESLint suppressions ceiling is **0** — the frontend baseline starts empty +and stays empty. (That figure is checked against the gate by the agreement test, +like every other floor on this page.) Hard rules: diff --git a/frontend_ratchet_test.go b/frontend_ratchet_test.go new file mode 100644 index 0000000..129ecba --- /dev/null +++ b/frontend_ratchet_test.go @@ -0,0 +1,124 @@ +package main_test + +import ( + "encoding/json" + "fmt" + "sort" + "strings" + "testing" +) + +// The frontend suppressions ratchet. +// +// ESLint's bulk-suppressions file is what lets the complexity gates run at +// error level without a mass rewrite: existing violations are baselined, new +// ones fail. That is only true while the baseline shrinks. Left unwatched it +// becomes the opposite — the place violations go to be forgotten, one +// `--suppress-rule` at a time. +// +// So the count is pinned, and it only ratchets down. Growing it requires +// editing the number here, in the PR that grows it, with the reason. +// +// Run: go test -run TestFrontendSuppressionsOnlyShrink . + +// maxFrontendSuppressions caps the total suppressed ESLint violations. +// +// Pinned at 0: the scaffold has none, and the complexity budgets were sized so +// that honest code passes. A PR that needs to raise this is a PR that should +// have split a component instead. +// +// Prune entries a fixed file no longer needs before touching this number: +// +// cd frontend && npx eslint . --prune-suppressions +const maxFrontendSuppressions = 0 + +// suppressionsFile is the ESLint bulk-suppressions baseline, at ESLint's +// default location. +const suppressionsFile = "frontend/eslint-suppressions.json" + +// TestFrontendSuppressionsOnlyShrink fails when the baseline holds more +// suppressed violations than the pin allows. +func TestFrontendSuppressionsOnlyShrink(t *testing.T) { + total, byRule := countSuppressions(t, readRepoFile(t, suppressionsFile)) + t.Logf("%s: %d suppressed violations (ceiling %d)", suppressionsFile, total, maxFrontendSuppressions) + + if total > maxFrontendSuppressions { + t.Errorf("%s suppresses %d violations (%s), exceeding the ceiling of %d — "+ + "fix the code rather than baselining it; if a suppression is genuinely "+ + "warranted it needs maintainer sign-off and a lower ceiling in the same PR", + suppressionsFile, total, strings.Join(byRule, ", "), maxFrontendSuppressions) + } +} + +// countSuppressions totals the suppressed violations in an ESLint +// bulk-suppressions document and summarises them per rule. +// +// The format is {file: {rule: {count: n}}}, so the total is the sum of every +// count — a file-level or rule-level tally would undercount a single file that +// baselines many violations of one rule. +func countSuppressions(t *testing.T, raw string) (total int, byRule []string) { + t.Helper() + var doc map[string]map[string]struct { + Count int `json:"count"` + } + if err := json.Unmarshal([]byte(raw), &doc); err != nil { + t.Fatalf("parsing %s: %v", suppressionsFile, err) + } + + perRule := map[string]int{} + for _, rules := range doc { + for rule, entry := range rules { + perRule[rule] += entry.Count + total += entry.Count + } + } + for rule, n := range perRule { + byRule = append(byRule, fmt.Sprintf("%s x%d", rule, n)) + } + sort.Strings(byRule) + return total, byRule +} + +// TestSuppressionCountingSumsEveryEntry pins the counter. A counter that +// tallied files or rules instead of violations would report 1 for a file +// baselining twenty violations of one rule — the exact case the ratchet is +// meant to stop. +func TestSuppressionCountingSumsEveryEntry(t *testing.T) { + tests := []struct { + name string + raw string + wantTotal int + wantRules []string + }{ + { + name: "empty baseline", + raw: `{}`, + wantTotal: 0, + }, + { + name: "one file, one rule, many violations", + raw: `{"src/a.ts":{"complexity":{"count":20}}}`, + wantTotal: 20, + wantRules: []string{"complexity x20"}, + }, + { + name: "counts sum across files and rules", + raw: `{"src/a.ts":{"complexity":{"count":2},"sonarjs/cognitive-complexity":{"count":1}}, + "src/b.ts":{"complexity":{"count":3}}}`, + wantTotal: 6, + wantRules: []string{"complexity x5", "sonarjs/cognitive-complexity x1"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + total, byRule := countSuppressions(t, tt.raw) + if total != tt.wantTotal { + t.Errorf("total = %d, want %d", total, tt.wantTotal) + } + if strings.Join(byRule, ",") != strings.Join(tt.wantRules, ",") { + t.Errorf("byRule = %v, want %v", byRule, tt.wantRules) + } + }) + } +} diff --git a/godobject_budget_test.go b/godobject_budget_test.go new file mode 100644 index 0000000..ff7cde7 --- /dev/null +++ b/godobject_budget_test.go @@ -0,0 +1,176 @@ +package main_test + +import ( + "go/ast" + "testing" +) + +// The god-object gate. The package-size budget is gameable in exactly the +// direction that matters here: moving code out of app.go into sibling files +// shrinks the line count while the App struct keeps every field and every +// method. This gate caps the struct itself. +// +// App is m6t's coordinator. Wails binds it to the frontend and it composes the +// backend services (git, pty, kube, helm; DESIGN.md §3.2) as they land, so it +// is the one type in the tree that everything else will be reachable through — +// which is precisely why it needs a ceiling from day one rather than after it +// has grown into a decomposition project. +// +// Run: go test -run TestAppGodObjectBudget . +const ( + // maxAppFields caps fields on the App struct. Pinned at today's actual + // with zero slack. + // + // This ceiling WILL need raising as backend services land, and that is the + // design: each service arrives as one composed handle, in a PR that says so + // on this line. What it stops is the accumulation nobody decided on — six + // loose fields where one owner struct belonged. If raising it by more than + // one per service, the question to answer in review is why the service is + // not one handle. + maxAppFields = 1 + + // maxAppMethods caps methods with an App receiver, counting value and + // pointer receivers alike. Pinned at today's actual with zero slack. + // + // Every exported method here is also Wails-bound API — it crosses the + // bridge into TypeScript — so this ceiling doubles as the budget on the + // backend's public surface. Behaviour belongs on the service that owns it, + // reached through a handle, not on the coordinator. + maxAppMethods = 1 + + // appCoordinatorType is the struct these ceilings bound. + appCoordinatorType = "App" + + // appPackageDir holds the coordinator. + appPackageDir = "internal/app" +) + +// TestAppGodObjectBudget fails when the coordinator gains fields or methods +// beyond the pinned ceilings. Unlike a line-count budget these numbers cannot +// be satisfied by shuffling code between files: they only come down through +// real decomposition — moving state and behaviour onto the service that owns +// it. +func TestAppGodObjectBudget(t *testing.T) { + fields, methods := countCoordinator(t) + t.Logf("%s coordinator: %d fields, %d methods (ceilings %d / %d)", + appCoordinatorType, fields, methods, maxAppFields, maxAppMethods) + + if fields > maxAppFields { + t.Errorf("%s has %d fields, exceeding the ceiling of %d — group the new state into a service handle rather than holding it directly, or justify the raise on maxAppFields in this PR", + appCoordinatorType, fields, maxAppFields) + } + if methods > maxAppMethods { + t.Errorf("%s has %d methods, exceeding the ceiling of %d — move behaviour onto the service that owns it (and remember every exported method here is also Wails-bound API), or justify the raise on maxAppMethods in this PR", + appCoordinatorType, methods, maxAppMethods) + } +} + +// countCoordinator parses the coordinator's package and returns the struct's +// field count and the number of methods declared on it. +func countCoordinator(t *testing.T) (fields, methods int) { + t.Helper() + files := parsePackage(t, appPackageDir) + + found := false + for _, file := range files { + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if name, ok := receiverTypeName(d); ok && name == appCoordinatorType { + methods++ + } + case *ast.GenDecl: + if n, ok := structFieldCount(d, appCoordinatorType); ok { + fields = n + found = true + } + } + } + } + if !found { + t.Fatalf("did not find `type %s struct` in %s — if the coordinator was renamed, retarget this gate rather than deleting it", + appCoordinatorType, appPackageDir) + } + return fields, methods +} + +// structFieldCount returns the field count of the named struct, counting each +// name in a grouped declaration (`a, b int` is two) and each embedded field as +// one. The bool is false for any declaration that is not that struct. +// +// Embedded fields count deliberately: embedding a struct to inherit its +// methods is a way of growing the coordinator without naming a field. +func structFieldCount(decl *ast.GenDecl, typeName string) (int, bool) { + for _, spec := range decl.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok || ts.Name.Name != typeName { + continue + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + continue + } + count := 0 + for _, field := range st.Fields.List { + if len(field.Names) == 0 { + count++ // embedded + continue + } + count += len(field.Names) + } + return count, true + } + return 0, false +} + +// TestGodObjectMetricCountsGroupedAndEmbeddedFields pins the metric. A field +// counter that missed grouped or embedded declarations would let the +// coordinator grow while reporting a flat number — the failure mode that makes +// a ratchet worthless. +func TestGodObjectMetricCountsGroupedAndEmbeddedFields(t *testing.T) { + const src = `package sample + +type Embedded struct{} + +type Target struct { + a, b int + c string + Embedded +} + +type Other struct{ x, y, z int } + +func (t *Target) PointerMethod() {} +func (t Target) ValueMethod() {} +func (o *Other) NotCounted() {} +func Free() {} +` + file := parseSource(t, src) + + fields, found := 0, false + methods := 0 + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if name, ok := receiverTypeName(d); ok && name == "Target" { + methods++ + } + case *ast.GenDecl: + if n, ok := structFieldCount(d, "Target"); ok { + fields, found = n, true + } + } + } + + if !found { + t.Fatal("structFieldCount did not find the Target struct") + } + // a, b, c, and the embedded field. + if want := 4; fields != want { + t.Errorf("fields = %d, want %d (grouped names and embedded fields each count)", fields, want) + } + // Both receiver forms count; the other type's method and the free function do not. + if want := 2; methods != want { + t.Errorf("methods = %d, want %d (value and pointer receivers both count)", methods, want) + } +} diff --git a/integration_guard_test.go b/integration_guard_test.go new file mode 100644 index 0000000..6d6e6c2 --- /dev/null +++ b/integration_guard_test.go @@ -0,0 +1,232 @@ +package main_test + +import ( + "go/build/constraint" + "io/fs" + "regexp" + "sort" + "strings" + "testing" +) + +// The integration guard closes the "test that never runs" gap. +// +// A build-tagged test is invisible to `go test ./...`: it compiles, it looks +// like coverage, and it executes nowhere. A tag typo has the same effect as +// deleting the suite, silently. m6t has no integration suite yet — the ones +// that will need one (git against real repositories, PTY against a real shell, +// kubectl against a cluster) arrive with #2 and #5 — so this gate is quiet +// today and fires the moment a tagged file lands without the wiring to run it. +// +// Run: go test -run TestIntegrationTestsAreExecuted . + +// integrationTag is the build tag reserved for tests that need real external +// systems. +const integrationTag = "integration" + +// TestIntegrationTestsAreExecuted fails when an integration-tagged test exists +// with no make target that runs it, and when a target exists that `make verify` +// never calls. +func TestIntegrationTestsAreExecuted(t *testing.T) { + tagged := taggedTestFiles(t) + makefile := readRepoFile(t, "Makefile") + runner := integrationRunnerTarget(makefile) + + if len(tagged) == 0 { + if runner != "" { + t.Errorf("the Makefile has target %q running -tags=%s, but no integration-tagged test files exist; "+ + "remove the target or add the suite it was written for", runner, integrationTag) + } + t.Logf("no %s-tagged test files in the tree; the guard is armed for when one lands", integrationTag) + return + } + + if runner == "" { + sort.Strings(tagged) + t.Fatalf("these test files require the %q build tag but no Makefile target runs them, "+ + "so they execute nowhere and rot silently:\n %s\n"+ + "Add a target that runs `go test -tags=%s ./...` and make `verify` depend on it, "+ + "or drop the tag so the plain test run picks them up.", + integrationTag, strings.Join(tagged, "\n "), integrationTag) + } + + if !verifyRunsTarget(t, makefile, runner) { + t.Errorf("target %q runs the %s suite but `make verify` does not depend on it, "+ + "so the suite still never runs in the gate; add it to verify's prerequisites", + runner, integrationTag) + } +} + +// taggedTestFiles returns the repo-relative paths of test files whose build +// constraints require the integration tag. +func taggedTestFiles(t *testing.T) []string { + t.Helper() + var tagged []string + err := fs.WalkDir(repoFS, rootPackageDir, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if p != rootPackageDir && skipDir(d.Name()) { + return fs.SkipDir + } + return nil + } + if !strings.HasSuffix(d.Name(), "_test.go") { + return nil + } + content, readErr := fs.ReadFile(repoFS, p) + if readErr != nil { + return readErr + } + if requiresIntegrationTag(string(content)) { + tagged = append(tagged, p) + } + return nil + }) + if err != nil { + t.Fatalf("walking for integration-tagged tests: %v", err) + } + return tagged +} + +// requiresIntegrationTag reports whether the source's build constraints make it +// build with the integration tag set and not without it. Constraints precede +// the package clause, so the scan stops there. +func requiresIntegrationTag(src string) bool { + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "package ") { + return false + } + if !constraint.IsGoBuild(trimmed) { + continue + } + expr, err := constraint.Parse(trimmed) + if err != nil { + return false + } + // Vary ONLY the integration tag and treat every other constraint as + // satisfiable. A file tagged `integration && linux` still requires the + // integration tag; whether the host is Linux is a separate question, + // and evaluating GOOS against the machine running the gate would make + // the answer depend on who ran it. + eval := func(withIntegration bool) bool { + return expr.Eval(func(tag string) bool { + if tag == integrationTag { + return withIntegration + } + return true + }) + } + return eval(true) && !eval(false) + } + return false +} + +// integrationRunnerRe finds a Makefile target whose recipe runs go test with +// the integration tag, capturing the target name. +var integrationRunnerRe = regexp.MustCompile(`(?ms)^([a-zA-Z0-9_-]+):[^\n]*\n(?:\t[^\n]*\n)*?\t[^\n]*-tags[= ]` + integrationTag) + +// integrationRunnerTarget returns the name of the Makefile target that runs the +// integration suite, or "" when no target does. +func integrationRunnerTarget(makefile string) string { + m := integrationRunnerRe.FindStringSubmatch(makefile) + if m == nil { + return "" + } + return m[1] +} + +// verifyRunsTarget reports whether the named target is one of verify's +// prerequisites. +func verifyRunsTarget(t *testing.T, makefile, target string) bool { + t.Helper() + deps := firstSubmatch(t, makefile, + `(?m)^verify:((?:[^\n]*\\\n)*[^\n]*)`, "the verify target's prerequisites") + return regexp.MustCompile(`\b` + regexp.QuoteMeta(target) + `\b`).MatchString(deps) +} + +// TestIntegrationTagDetection pins the constraint reader. A reader that missed +// the tag would make the guard permanently silent; one that saw it everywhere +// would fail every ordinary test file. Both look like a working gate. +func TestIntegrationTagDetection(t *testing.T) { + tests := []struct { + name string + src string + want bool + }{ + { + name: "plain test file is not tagged", + src: "package x\n\nimport \"testing\"\n", + want: false, + }, + { + name: "integration constraint is detected", + src: "//go:build integration\n\npackage x\n", + want: true, + }, + { + name: "integration in a conjunction is detected", + src: "//go:build integration && linux\n\npackage x\n", + want: true, + }, + { + name: "a disjunction that also builds untagged is not integration-only", + src: "//go:build integration || !integration\n\npackage x\n", + want: false, + }, + { + name: "an unrelated tag is not the integration tag", + src: "//go:build e2e\n\npackage x\n", + want: false, + }, + { + name: "a constraint-looking line after the package clause is ignored", + src: "package x\n\n//go:build integration\n", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := requiresIntegrationTag(tt.src); got != tt.want { + t.Errorf("requiresIntegrationTag = %v, want %v", got, tt.want) + } + }) + } +} + +// TestIntegrationRunnerDetection pins the Makefile reader against the shapes a +// real target takes. +func TestIntegrationRunnerDetection(t *testing.T) { + tests := []struct { + name string + makefile string + want string + }{ + { + name: "no target runs the tag", + makefile: "test:\n\tgo test ./...\n", + want: "", + }, + { + name: "target running the tag is found", + makefile: "test-integration:\n\t@echo running\n\tgo test -tags=integration ./...\n", + want: "test-integration", + }, + { + name: "space-separated tags flag is found", + makefile: "itest:\n\tgo test -tags integration ./...\n", + want: "itest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := integrationRunnerTarget(tt.makefile); got != tt.want { + t.Errorf("integrationRunnerTarget = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/noop_interface_test.go b/noop_interface_test.go new file mode 100644 index 0000000..56ffd92 --- /dev/null +++ b/noop_interface_test.go @@ -0,0 +1,267 @@ +package main_test + +import ( + "fmt" + "go/ast" + "sort" + "strings" + "testing" +) + +// The no-op interface gate. +// +// The smell: an interface is declared, one type implements it, and every method +// of that implementation does nothing. The interface exists so a dependency can +// be "injected" and a test can assert the no-op was called — coverage goes up, +// nothing is verified. It is the structural form of the tautological test that +// CLAUDE.md bans, and mutation testing cannot catch it because there is no +// behaviour to mutate. +// +// Detection is by method-name set rather than full type checking: a type +// implements an interface here if its method names are a superset of the +// interface's. That is an approximation, and it is the conservative direction — +// a false positive needs a type that both shares every method name with an +// interface AND has nothing but empty bodies, which is itself the thing this +// gate is looking for. +// +// Run: go test -run TestNoNoopOnlyInterfaces . + +// methodBody describes one method for the purposes of this gate. +type methodBody struct { + name string + isNop bool +} + +// TestNoNoopOnlyInterfaces fails when every implementation of a first-party +// interface does nothing. +func TestNoNoopOnlyInterfaces(t *testing.T) { + interfaces, implementations := collectInterfacesAndImplementations(t) + + var violations []string + for name, required := range interfaces { + implementers, allNop := classifyImplementations(required, implementations) + if len(implementers) == 0 || !allNop { + continue + } + sort.Strings(implementers) + violations = append(violations, fmt.Sprintf( + "interface %s is satisfied only by no-op implementations (%s) — either give it a real implementation or delete it; an interface whose only implementation does nothing launders coverage without verifying behaviour", + name, strings.Join(implementers, ", "))) + } + sort.Strings(violations) + + if len(violations) > 0 { + t.Errorf("no-op interface detected:\n %s", strings.Join(violations, "\n ")) + } +} + +// collectInterfacesAndImplementations scans first-party non-test source for +// interface declarations (name to its method-name set) and for named types +// with methods (name to those methods). +func collectInterfacesAndImplementations(t *testing.T) (map[string][]string, map[string][]methodBody) { + t.Helper() + interfaces := map[string][]string{} + implementations := map[string][]methodBody{} + + for _, dir := range packageDirs(t) { + for _, file := range parsePackage(t, dir) { + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.GenDecl: + for name, methods := range interfaceMethodNames(d) { + interfaces[dir+"."+name] = methods + } + case *ast.FuncDecl: + recv, ok := receiverTypeName(d) + if !ok { + continue + } + key := dir + "." + recv + implementations[key] = append(implementations[key], methodBody{ + name: d.Name.Name, + isNop: isNoopBody(d), + }) + } + } + } + } + return interfaces, implementations +} + +// interfaceMethodNames returns the method names of each interface type declared +// by decl. Embedded interfaces are skipped: their methods are not named here, +// and an interface built only from embeddings is not the smell being detected. +func interfaceMethodNames(decl *ast.GenDecl) map[string][]string { + found := map[string][]string{} + for _, spec := range decl.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + it, ok := ts.Type.(*ast.InterfaceType) + if !ok { + continue + } + var names []string + for _, field := range it.Methods.List { + if _, isFunc := field.Type.(*ast.FuncType); !isFunc { + continue // embedded interface or a type constraint element + } + for _, ident := range field.Names { + names = append(names, ident.Name) + } + } + if len(names) > 0 { + found[ts.Name.Name] = names + } + } + return found +} + +// classifyImplementations returns the types whose method sets cover required, +// and whether every one of them implements all of those methods as no-ops. +func classifyImplementations(required []string, implementations map[string][]methodBody) (implementers []string, allNop bool) { + allNop = true + for typeName, methods := range implementations { + byName := map[string]methodBody{} + for _, m := range methods { + byName[m.name] = m + } + if !covers(byName, required) { + continue + } + implementers = append(implementers, typeName) + for _, name := range required { + if !byName[name].isNop { + allNop = false + } + } + } + return implementers, allNop +} + +// covers reports whether byName holds every required method. +func covers(byName map[string]methodBody, required []string) bool { + for _, name := range required { + if _, ok := byName[name]; !ok { + return false + } + } + return true +} + +// isNoopBody reports whether a method does nothing observable: an empty body, +// or a single return of nothing but literals and nil. A body that calls +// anything, assigns anything, or returns a computed value is real. +func isNoopBody(fn *ast.FuncDecl) bool { + if fn.Body == nil { + return true // declared without a body (assembly or external linkage) + } + if len(fn.Body.List) == 0 { + return true + } + if len(fn.Body.List) > 1 { + return false + } + ret, ok := fn.Body.List[0].(*ast.ReturnStmt) + if !ok { + return false + } + for _, result := range ret.Results { + if !isZeroValueExpr(result) { + return false + } + } + return true +} + +// isZeroValueExpr reports whether e is a literal or the identifier nil — the +// things a stub returns when it has nothing to say. +func isZeroValueExpr(e ast.Expr) bool { + switch v := e.(type) { + case *ast.BasicLit: + return true + case *ast.Ident: + return v.Name == "nil" || v.Name == "true" || v.Name == "false" + case *ast.CompositeLit: + return len(v.Elts) == 0 + default: + return false + } +} + +// TestNoopDetectionDistinguishesStubsFromBehaviour pins the classifier. A +// detector that called everything a no-op would fail every honest interface; +// one that called nothing a no-op would never fire. Both look like a working +// gate from the outside, which is why the classifier is tested directly. +func TestNoopDetectionDistinguishesStubsFromBehaviour(t *testing.T) { + const src = `package sample + +type T struct{} + +func (T) EmptyBody() {} +func (T) BareReturn() { return } +func (T) ReturnsNil() error { return nil } +func (T) ReturnsLiteral() int { return 0 } +func (T) ReturnsEmptyStruct() S { return S{} } +func (T) CallsSomething() error { return doWork() } +func (T) Assigns() int { x := 1; return x } +func (T) TwoStatements() error { log(); return nil } +` + want := map[string]bool{ + "EmptyBody": true, + "BareReturn": true, + "ReturnsNil": true, + "ReturnsLiteral": true, + "ReturnsEmptyStruct": true, + "CallsSomething": false, + "Assigns": false, + "TwoStatements": false, + } + + for _, decl := range parseSource(t, src).Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + expected, known := want[fn.Name.Name] + if !known { + t.Fatalf("fixture method %s has no expectation", fn.Name.Name) + } + if got := isNoopBody(fn); got != expected { + t.Errorf("isNoopBody(%s) = %v, want %v", fn.Name.Name, got, expected) + } + } +} + +// TestNoopInterfaceGateFires proves the gate reports a no-op-only interface and +// stays quiet for one with a real implementation. Without this the gate could +// be permanently vacuous — there are no interfaces in the tree yet — and nobody +// would know until it failed to catch the first one. +func TestNoopInterfaceGateFires(t *testing.T) { + required := []string{"Do"} + + stubs := map[string][]methodBody{ + "internal/x.Stub": {{name: "Do", isNop: true}}, + } + implementers, allNop := classifyImplementations(required, stubs) + if len(implementers) != 1 || !allNop { + t.Errorf("a stub-only implementation should be reported: implementers=%v allNop=%v", implementers, allNop) + } + + mixed := map[string][]methodBody{ + "internal/x.Stub": {{name: "Do", isNop: true}}, + "internal/x.Real": {{name: "Do", isNop: false}}, + } + implementers, allNop = classifyImplementations(required, mixed) + if len(implementers) != 2 || allNop { + t.Errorf("a real implementation alongside a stub must clear the gate: implementers=%v allNop=%v", implementers, allNop) + } + + unrelated := map[string][]methodBody{ + "internal/x.Other": {{name: "SomethingElse", isNop: true}}, + } + if implementers, _ := classifyImplementations(required, unrelated); len(implementers) != 0 { + t.Errorf("a type that does not cover the interface is not an implementation: %v", implementers) + } +} diff --git a/package_budget_test.go b/package_budget_test.go new file mode 100644 index 0000000..585bfb3 --- /dev/null +++ b/package_budget_test.go @@ -0,0 +1,236 @@ +package main_test + +import ( + "bufio" + "fmt" + "io/fs" + "regexp" + "sort" + "strings" + "testing" + "testing/fstest" +) + +// packagePin is one package's ratchet entry. Every first-party package has +// exactly one, and the table below IS the import ratchet: adding a package +// means adding an entry in the same PR, which makes package sprawl a +// deliberate act instead of a side effect. +type packagePin struct { + // loc caps hand-written, non-test lines in the package. + loc int + // exported caps package-scope exported identifiers. Pinned to the actual + // with zero slack: widening a seam should be visible in review, and the + // diff that widens it is where the justification belongs. + exported int + // why records what the package is for, so a reviewer reading a ratchet + // bump can judge whether the growth belongs there. + why string +} + +// structuralPins is the ratchet table, pinned to today's measurements. +// +// Reproduce every number with: +// +// go test -count=1 -run 'TestPackageSizeBudget|TestPackageExportedSurfaceBudget' -v . +// +// Exported-surface counts are pinned at actuals with zero slack. The LOC +// ceilings are the one exception and carry headroom — see locCeilingNote, +// because that difference is a judgement call a reviewer should be able to +// challenge rather than discover. +var structuralPins = map[string]packagePin{ + rootPackageDir: { + loc: 60, exported: 0, + why: "composition root: embeds the frontend, hands options to the Wails runtime", + }, + "internal/app": { + loc: 200, exported: 2, + why: "Wails binding layer: the bound object plus the window options", + }, + "internal/buildinfo": { + loc: 150, exported: 2, + why: "link-time build identity; a dependency root importing nothing first-party", + }, +} + +// locCeilingNote explains why the LOC ceilings carry headroom while every other +// figure here is pinned to the actual. +// +// A LOC ceiling set to a package's exact current line count is not a ratchet, +// it is a freeze: one more line of doc comment in buildinfo.go would fail the +// build. The number has to represent the size at which a package stops being +// readable in one sitting, and m6t cannot measure that yet — the backend +// services that will be the real packages (git, pty, kube, helm; DESIGN.md +// §3.2) land in #2 and #5. So these figures are seeded as policy: +// +// - 200 for internal/app and 150 for buildinfo — roughly 3x and 2x today's +// size, so ordinary work does not trip the gate while a package that +// doubles again arrives in review as a decomposition question. +// - 60 for the root: main.go does one thing and must keep doing only that. +// +// Re-pin against real measurements once #2 and #5 land. No other ceiling here +// needs the caveat: counts of packages, files and exported names do not grow +// through ordinary editing. +const locCeilingNote = "LOC ceilings are policy-seeded; re-pin after #2/#5 (see locCeilingNote)" + +// maxFilesPerPackage stops a package from escaping its LOC budget by fanning +// the same code across many small files. +const maxFilesPerPackage = 8 + +// generatedMarkerRe matches the canonical generated-code marker +// (https://go.dev/s/generatedcode). A file carrying it is excluded from the +// size budget: generated code is not a package's maintenance burden. +var generatedMarkerRe = regexp.MustCompile(`^// Code generated .* DO NOT EDIT\.?$`) + +// packageSize is one package's measured footprint. +type packageSize struct { + loc int + files int +} + +// measurePackages returns the non-generated, non-test footprint of every +// first-party package, keyed by repo-relative directory. +func measurePackages(t *testing.T) map[string]packageSize { + t.Helper() + sizes := map[string]packageSize{} + walkGoSource(t, func(file, dir string) { + generated, loc := countGoFile(t, repoFS, file) + if generated { + return + } + size := sizes[dir] + size.loc += loc + size.files++ + sizes[dir] = size + }) + return sizes +} + +// countGoFile reports whether the named file is generated and, if not, how +// many lines it holds. The marker conventionally precedes the package clause, +// but scanning the whole file is cheap and does not miss one placed after a +// build constraint or licence header. +func countGoFile(t *testing.T, fsys fs.FS, file string) (generated bool, loc int) { + t.Helper() + f, err := fsys.Open(file) + if err != nil { + t.Fatalf("opening %s: %v", file, err) + } + defer func() { + if closeErr := f.Close(); closeErr != nil { + t.Errorf("closing %s: %v", file, closeErr) + } + }() + + scanner := bufio.NewScanner(f) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for scanner.Scan() { + if generatedMarkerRe.MatchString(strings.TrimSpace(scanner.Text())) { + generated = true + } + loc++ + } + if err := scanner.Err(); err != nil { + t.Fatalf("scanning %s: %v", file, err) + } + return generated, loc +} + +// TestPackageSizeBudget fails when a package outgrows its ceiling. Hitting it +// is the signal to decompose the package into cohesive pieces — not to raise +// the number, which defeats the gate. +func TestPackageSizeBudget(t *testing.T) { + sizes := measurePackages(t) + + var violations []string + for dir, size := range sizes { + pin, pinned := structuralPins[dir] + if !pinned { + // TestEveryPackageIsPinned reports unpinned packages with the full + // explanation; skipping here keeps one new package to one failure. + continue + } + t.Logf("%-20s %4d LOC (ceiling %d), %d files (ceiling %d)", + dir, size.loc, pin.loc, size.files, maxFilesPerPackage) + if size.loc > pin.loc { + violations = append(violations, fmt.Sprintf( + "%s: %d LOC exceeds its ceiling of %d — decompose the package, do not raise the ceiling (%s)", + dir, size.loc, pin.loc, locCeilingNote)) + } + if size.files > maxFilesPerPackage { + violations = append(violations, fmt.Sprintf( + "%s: %d files exceeds the ceiling of %d — split the package, do not raise the ceiling", + dir, size.files, maxFilesPerPackage)) + } + } + sort.Strings(violations) + + if len(violations) > 0 { + t.Errorf("package size budget exceeded:\n %s", strings.Join(violations, "\n ")) + } +} + +// TestEveryPackageIsPinned is the other half of the size budget: a package with +// no pin is measured against nothing, so it fails here rather than silently +// escaping every ceiling. +func TestEveryPackageIsPinned(t *testing.T) { + for _, dir := range packageDirs(t) { + if _, ok := structuralPins[dir]; !ok { + t.Errorf("package %s has no entry in structuralPins; add one (loc, exported, why) in this PR", dir) + } + } + for dir := range structuralPins { + if _, err := fs.Stat(repoFS, dir); err != nil { + t.Errorf("structuralPins pins %s, which no longer exists; remove the stale entry", dir) + } + } +} + +// TestCountGoFileDetectsGeneratedCode exercises marker detection and line +// counting directly: it is the unit that proves generated code is excluded +// from the budget rather than quietly inflating it. +func TestCountGoFileDetectsGeneratedCode(t *testing.T) { + tests := []struct { + name string + content string + wantGenerated bool + wantLOC int + }{ + { + name: "hand-written file is counted", + content: "package x\n\nfunc f() {}\n", + wantGenerated: false, + wantLOC: 3, + }, + { + name: "canonical generated marker is detected", + content: "// Code generated by stringer. DO NOT EDIT.\npackage x\n", + wantGenerated: true, + wantLOC: 2, + }, + { + name: "marker after a build constraint is still detected", + content: "//go:build ignore\n\n// Code generated by mockgen. DO NOT EDIT.\npackage m\n", + wantGenerated: true, + wantLOC: 4, + }, + { + name: "prose that merely mentions generated code is not a marker", + content: "package x\n\n// this is not Code generated by anything, DO NOT EDIT it\n", + wantGenerated: false, + wantLOC: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fsys := fstest.MapFS{"f.go": &fstest.MapFile{Data: []byte(tt.content)}} + generated, loc := countGoFile(t, fsys, "f.go") + if generated != tt.wantGenerated { + t.Errorf("generated = %v, want %v", generated, tt.wantGenerated) + } + if loc != tt.wantLOC { + t.Errorf("loc = %d, want %d", loc, tt.wantLOC) + } + }) + } +} diff --git a/package_graph_test.go b/package_graph_test.go new file mode 100644 index 0000000..b6c1fe8 --- /dev/null +++ b/package_graph_test.go @@ -0,0 +1,126 @@ +package main_test + +import ( + "fmt" + "sort" + "strings" + "testing" +) + +// TestNoDeadPackages fails when a first-party package is not reachable from +// main through non-test imports. +// +// This is the no-vaporware rule made mechanical (CLAUDE.md). Code that compiles +// but is not wired into the app still has to be read, reviewed, updated and +// kept building — it costs everything real code costs and returns nothing. The +// usual way it appears is a package written ahead of the feature that was going +// to use it. +// +// Reachability is computed over non-test imports only, deliberately: a package +// imported solely by its own tests is exactly the case this gate exists to +// catch, and counting test imports would hide it. +func TestNoDeadPackages(t *testing.T) { + graph := firstPartyImports(t) + reached := reachableFromMain(t, graph) + + var dead []string + for dir := range graph { + if !reached[dir] { + dead = append(dead, dir) + } + } + sort.Strings(dead) + + if len(dead) > 0 { + t.Errorf("these packages are not reachable from main through non-test imports:\n %s\n"+ + "Wire each one into the app in this PR or delete it. A package that only its own "+ + "tests import is not shipped code.", strings.Join(dead, "\n ")) + } +} + +// TestImportGraphIsPinned pins the first-party dependency edges. +// +// The package list is pinned by structuralPins; this pins what depends on what. +// It is the ratchet that keeps the layering in DESIGN.md §3.2 from eroding one +// convenient import at a time — depguard enforces the same shape at lint time, +// and this is the version that survives a config edit. +// +// Adding an edge means updating this table in the same PR, which is where the +// reviewer gets to ask whether the new dependency belongs. +func TestImportGraphIsPinned(t *testing.T) { + // The composition root imports the binding layer; the binding layer reads + // build identity. buildinfo imports nothing first-party — it is a leaf, and + // depguard pins that independently. + want := map[string][]string{ + rootPackageDir: {"internal/app"}, + "internal/app": {"internal/buildinfo"}, + "internal/buildinfo": {}, + } + + graph := firstPartyImports(t) + + var problems []string + for dir, deps := range graph { + got := sortedKeys(deps) + expected, pinned := want[dir] + if !pinned { + problems = append(problems, fmt.Sprintf( + "%s has no entry in the pinned import graph; add one in this PR", dir)) + continue + } + sort.Strings(expected) + if strings.Join(got, ",") != strings.Join(expected, ",") { + problems = append(problems, fmt.Sprintf( + "%s imports [%s], pinned as [%s] — update the pin in this PR and say why the dependency belongs", + dir, strings.Join(got, " "), strings.Join(expected, " "))) + } + } + for dir := range want { + if _, ok := graph[dir]; !ok { + problems = append(problems, fmt.Sprintf( + "the import graph pins %s, which no longer exists; remove the stale entry", dir)) + } + } + sort.Strings(problems) + + if len(problems) > 0 { + t.Errorf("import graph drifted from its pin:\n %s", strings.Join(problems, "\n ")) + } +} + +// TestReachabilityFollowsTransitiveImports pins the reachability walk. A BFS +// that only looked one level deep would call a transitively-used package dead, +// and a walk that marked everything reached would never call anything dead — +// both failures look like a passing gate. +func TestReachabilityFollowsTransitiveImports(t *testing.T) { + graph := map[string]map[string]bool{ + rootPackageDir: {"internal/a": true}, + "internal/a": {"internal/b": true}, + "internal/b": {}, + "internal/orphan": {}, + "internal/testonly": {}, + } + + reached := reachableFromMain(t, graph) + + for _, dir := range []string{rootPackageDir, "internal/a", "internal/b"} { + if !reached[dir] { + t.Errorf("%s should be reachable from main", dir) + } + } + for _, dir := range []string{"internal/orphan", "internal/testonly"} { + if reached[dir] { + t.Errorf("%s is imported by nothing and must not be reported as reachable", dir) + } + } +} + +// sortedKeys returns a set's members in sorted order. +func sortedKeys(set map[string]bool) []string { + keys := make([]string, 0, len(set)) + for k := range set { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/pins_test.go b/pins_test.go index 0005801..a0c04de 100644 --- a/pins_test.go +++ b/pins_test.go @@ -16,6 +16,7 @@ import ( "os" "path" "regexp" + "strconv" "testing" ) @@ -157,6 +158,11 @@ func TestGateFiguresAgree(t *testing.T) { "CONTRIBUTING.md mutation-efficacy floor", contributing, `Mutation-testing efficacy ≥ \*\*([0-9]+)%\*\*`, efficacy, }, + { + "CONTRIBUTING.md ESLint suppressions ceiling", contributing, + `ESLint suppressions ceiling is \*\*([0-9]+)\*\*`, + strconv.Itoa(maxFrontendSuppressions), + }, }) } diff --git a/structural_gates_test.go b/structural_gates_test.go new file mode 100644 index 0000000..4d06ce6 --- /dev/null +++ b/structural_gates_test.go @@ -0,0 +1,152 @@ +package main_test + +import ( + "fmt" + "io/fs" + "regexp" + "sort" + "strings" + "testing" +) + +// The guard on the guards. +// +// Every gate in this directory is a plain Go test, which is what makes it cheap +// and what makes it fragile in one specific way: a test that stops running +// fails nothing. Move a gate into a package the root test run does not reach, +// give it a build tag, or rename it out of the Test prefix, and the ratchet +// silently stops ratcheting while `make verify` stays green. +// +// This file pins the wiring: the gate files exist, they are in the root test +// package that `go test ./...` runs, they carry no build tag, and `make verify` +// runs the test target that executes them. +// +// Run: go test -run TestStructuralGatesAreWired . + +// structuralGateFiles lists the gate files and the gate each one provides. +// Adding a gate means adding it here, which is what keeps this guard honest as +// the set grows. +var structuralGateFiles = map[string]string{ + "package_budget_test.go": "package size budget and the pinned package list", + "surface_budget_test.go": "exported-surface budget", + "godobject_budget_test.go": "coordinator field/method ceilings", + "package_graph_test.go": "dead-package detection and the pinned import graph", + "noop_interface_test.go": "no-op-only interface detection", + "integration_guard_test.go": "integration-tagged tests actually execute", + "frontend_ratchet_test.go": "ESLint suppressions only shrink", + "pins_test.go": "gate figures agree across Makefile, CI, codecov and docs", +} + +// rootTestPackage is the package every root gate file must declare so that the +// module's own test run executes it. +const rootTestPackage = "package main_test" + +// TestStructuralGatesAreWired fails when a gate file is missing, lives outside +// the root test package, or carries a build constraint that would keep it out +// of the default test run. +func TestStructuralGatesAreWired(t *testing.T) { + var problems []string + + for file, gate := range structuralGateFiles { + content, err := fs.ReadFile(repoFS, file) + if err != nil { + problems = append(problems, fmt.Sprintf( + "%s is missing — it provides the %s gate; restore it or remove it from structuralGateFiles with the reason", file, gate)) + continue + } + src := string(content) + if !strings.Contains(src, rootTestPackage) { + problems = append(problems, fmt.Sprintf( + "%s does not declare %q, so the root test run does not execute the %s gate", file, rootTestPackage, gate)) + } + if requiresIntegrationTag(src) || hasBuildConstraint(src) { + problems = append(problems, fmt.Sprintf( + "%s carries a build constraint, so the %s gate is excluded from the default test run", file, gate)) + } + if !hasTestFunc(src) { + problems = append(problems, fmt.Sprintf( + "%s declares no Test function, so the %s gate runs nothing", file, gate)) + } + } + sort.Strings(problems) + + if len(problems) > 0 { + t.Errorf("structural gates are not wired into the test run:\n %s", strings.Join(problems, "\n ")) + } +} + +// TestVerifyRunsTheStructuralGates pins the other half of the wiring: the gates +// run under `go test ./...`, and `make verify` has to actually run that. +// +// pins_test.go asserts verify's prerequisite list contains `test`; this asserts +// the test target runs the whole module rather than a subset that could quietly +// exclude the root package where every gate lives. +func TestVerifyRunsTheStructuralGates(t *testing.T) { + makefile := readRepoFile(t, "Makefile") + + recipe := firstSubmatch(t, makefile, + `(?m)^test:[^\n]*\n((?:\t[^\n]*\n)+)`, "the test target's recipe") + + if !strings.Contains(recipe, "./...") { + t.Errorf("the `test` target does not run ./..., so the root package holding every structural gate may be skipped:\n%s", recipe) + } + if !strings.Contains(recipe, "-count=1") { + t.Errorf("the `test` target does not pass -count=1, so a cached pass could stand in for a gate that was never re-run:\n%s", recipe) + } +} + +// buildConstraintRe matches a //go:build line. +var buildConstraintRe = regexp.MustCompile(`(?m)^//go:build `) + +// packageClauseRe matches the package clause that ends the constraint region. +var packageClauseRe = regexp.MustCompile(`(?m)^package `) + +// hasBuildConstraint reports whether the source carries a build constraint. +// +// Only the region before the package clause counts: a constraint is only a +// constraint there, and scanning the whole file would misread the //go:build +// strings that this repository's own gate fixtures contain. +func hasBuildConstraint(src string) bool { + head := src + if loc := packageClauseRe.FindStringIndex(src); loc != nil { + head = src[:loc[0]] + } + return buildConstraintRe.MatchString(head) +} + +// testFuncRe matches a top-level Go test function declaration. +var testFuncRe = regexp.MustCompile(`(?m)^func Test[A-Z_]\w*\(t \*testing\.T\)`) + +// hasTestFunc reports whether the source declares at least one test function. +func hasTestFunc(src string) bool { + return testFuncRe.MatchString(src) +} + +// TestWiringDetectorsFire pins the two detectors this guard depends on. A +// constraint detector that never matched, or a test-function detector that +// always matched, would make the guard above report success unconditionally. +func TestWiringDetectorsFire(t *testing.T) { + constrained := "//go:build integration\n\npackage main_test\n\nfunc TestX(t *testing.T) {}\n" + plain := "package main_test\n\nfunc TestX(t *testing.T) {}\n" + // A gate file that holds a constraint string as a fixture — which several + // of the files in this directory do — must not read as constrained itself. + fixtureHoldsAConstraint := "package main_test\n\nconst src = `\n" + + "//go:build integration\n\npackage x\n`\n\nfunc TestX(t *testing.T) {}\n" + noTests := "package main_test\n\nfunc helper() {}\n" + + if !hasBuildConstraint(constrained) { + t.Error("hasBuildConstraint missed a real //go:build line") + } + if hasBuildConstraint(plain) { + t.Error("hasBuildConstraint fired on a file with no constraint") + } + if hasBuildConstraint(fixtureHoldsAConstraint) { + t.Error("hasBuildConstraint fired on a constraint that appears only in a fixture after the package clause") + } + if !hasTestFunc(plain) { + t.Error("hasTestFunc missed a test function") + } + if hasTestFunc(noTests) { + t.Error("hasTestFunc fired on a file with no test function") + } +} diff --git a/structure_test.go b/structure_test.go new file mode 100644 index 0000000..6fba2dc --- /dev/null +++ b/structure_test.go @@ -0,0 +1,229 @@ +// Structural ratchets: gates that make architectural decay a test failure +// rather than a review opinion. +// +// The per-function linters (gocyclo, gocognit, revive) all evaluate code INSIDE +// one function, so a god-package assembled from a hundred small, tidy functions +// passes every one of them. These tests bound the shapes those linters cannot +// see: how big a package is, how much it exports, how many packages exist, what +// depends on what, and how much state the coordinator holds. +// +// Ceilings only move DOWN. Raising one is a regression that must be justified +// in the PR that raises it — that justification, written next to the number, is +// the whole mechanism. There is no suppression comment and no escape hatch. +// +// This file holds the shared source-tree analysis the gates are built on. +package main_test + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path" + "strconv" + "strings" + "testing" +) + +// modulePath is this module's import path. Import paths under it are +// first-party; everything else is a dependency. +const modulePath = "github.com/txn2/m6t" + +// rootPackageDir is how the main package's directory is spelled in the +// repo-relative paths these gates use. +const rootPackageDir = "." + +// skipDir reports whether a directory holds no first-party Go source and +// should be pruned from every walk. +// +// frontend/node_modules matters specifically: npm packages ship Go source +// (flatted/golang), and counting a dependency's code as m6t's would corrupt +// every measurement here. go.mod's `ignore` directive keeps it out of the +// build for the same reason. +// The skipped names match what the go tool itself excludes from a package +// walk, so a directory these gates ignore is one the compiler ignores too. +// Anything else — including build/, which holds packaging assets today — stays +// in the walk: a directory pruned here is a directory where a package could +// live outside every ceiling in this file. +func skipDir(name string) bool { + switch name { + case "node_modules", "vendor", "testdata": + return true + } + // .git, .github, .semgrep and friends: no first-party Go source, and the + // go tool skips dot-prefixed directories for the same reason. + return strings.HasPrefix(name, ".") && name != rootPackageDir +} + +// goSourceFile reports whether name is hand-written, non-test Go source. +func goSourceFile(name string) bool { + return strings.HasSuffix(name, ".go") && !strings.HasSuffix(name, "_test.go") +} + +// walkGoSource calls visit for every hand-written, non-test .go file in the +// module, passing the file's repo-relative slash path and the directory that +// holds it (the root package's directory is "."). +func walkGoSource(t *testing.T, visit func(file, dir string)) { + t.Helper() + err := fs.WalkDir(repoFS, rootPackageDir, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if p != rootPackageDir && skipDir(d.Name()) { + return fs.SkipDir + } + return nil + } + if !goSourceFile(d.Name()) { + return nil + } + dir := path.Dir(p) + if dir == "" { + dir = rootPackageDir + } + visit(p, dir) + return nil + }) + if err != nil { + t.Fatalf("walking the module for Go source: %v", err) + } +} + +// packageDirs returns every directory holding hand-written, non-test Go +// source, as repo-relative slash paths. +func packageDirs(t *testing.T) []string { + t.Helper() + seen := map[string]bool{} + var dirs []string + walkGoSource(t, func(_, dir string) { + if !seen[dir] { + seen[dir] = true + dirs = append(dirs, dir) + } + }) + if len(dirs) == 0 { + t.Fatal("found no first-party Go packages; the walk is broken, not the tree") + } + return dirs +} + +// parsePackage parses every hand-written, non-test file in dir. +func parsePackage(t *testing.T, dir string) []*ast.File { + t.Helper() + fset := token.NewFileSet() + var files []*ast.File + walkGoSource(t, func(file, fileDir string) { + if fileDir != dir { + return + } + src, err := fs.ReadFile(repoFS, file) + if err != nil { + t.Fatalf("reading %s: %v", file, err) + } + parsed, err := parser.ParseFile(fset, file, src, parser.SkipObjectResolution) + if err != nil { + t.Fatalf("parsing %s: %v", file, err) + } + files = append(files, parsed) + }) + if len(files) == 0 { + t.Fatalf("no non-test Go files found in %s", dir) + } + return files +} + +// parseSource parses a Go source literal. The gates' metrics are unit-tested +// against literals so their behaviour is pinned without adding fixture +// packages to the module — which the dead-package gate would rightly reject. +func parseSource(t *testing.T, src string) *ast.File { + t.Helper() + file, err := parser.ParseFile(token.NewFileSet(), "fixture.go", src, parser.SkipObjectResolution) + if err != nil { + t.Fatalf("parsing fixture source: %v", err) + } + return file +} + +// importDir maps a first-party import path to the repo-relative directory that +// holds it, reporting false for third-party imports. +func importDir(importPath string) (string, bool) { + if importPath == modulePath { + return rootPackageDir, true + } + rest, ok := strings.CutPrefix(importPath, modulePath+"/") + if !ok { + return "", false + } + return rest, true +} + +// firstPartyImports returns the first-party import graph: each package +// directory mapped to the set of package directories it imports from +// non-test code. +// +// Non-test code only, deliberately. A package reachable solely from a test is +// not wired into the app; that is exactly what the dead-package gate exists to +// catch, and counting test imports would hide it. +func firstPartyImports(t *testing.T) map[string]map[string]bool { + t.Helper() + graph := map[string]map[string]bool{} + for _, dir := range packageDirs(t) { + deps := map[string]bool{} + for _, file := range parsePackage(t, dir) { + for _, spec := range file.Imports { + importPath, err := strconv.Unquote(spec.Path.Value) + if err != nil { + t.Fatalf("unquoting import %s in %s: %v", spec.Path.Value, dir, err) + } + if dep, ok := importDir(importPath); ok { + deps[dep] = true + } + } + } + graph[dir] = deps + } + return graph +} + +// reachableFromMain returns the set of package directories reachable from the +// main package through non-test imports — the packages that actually ship in +// the binary. +func reachableFromMain(t *testing.T, graph map[string]map[string]bool) map[string]bool { + t.Helper() + if _, ok := graph[rootPackageDir]; !ok { + t.Fatal("the module has no root main package; the entrypoint gates cannot run") + } + reached := map[string]bool{rootPackageDir: true} + queue := []string{rootPackageDir} + for len(queue) > 0 { + dir := queue[0] + queue = queue[1:] + for dep := range graph[dir] { + if !reached[dep] { + reached[dep] = true + queue = append(queue, dep) + } + } + } + return reached +} + +// receiverTypeName returns the name of fn's receiver type, unwrapping a +// pointer receiver. Counting value receivers alongside pointer ones closes an +// escape hatch: a method ceiling that only saw `func (a *App)` could be ducked +// by rewriting the receiver as `func (a App)` with no real decomposition. +func receiverTypeName(fn *ast.FuncDecl) (string, bool) { + if fn.Recv == nil || len(fn.Recv.List) != 1 { + return "", false + } + recv := fn.Recv.List[0].Type + if star, ok := recv.(*ast.StarExpr); ok { + recv = star.X + } + ident, ok := recv.(*ast.Ident) + if !ok { + return "", false + } + return ident.Name, true +} diff --git a/surface_budget_test.go b/surface_budget_test.go new file mode 100644 index 0000000..e3237d8 --- /dev/null +++ b/surface_budget_test.go @@ -0,0 +1,121 @@ +package main_test + +import ( + "fmt" + "go/ast" + "sort" + "strings" + "testing" +) + +// exportedNames returns the exported package-scope identifiers declared across +// files: the top-level funcs, types, vars and consts another package can name. +// +// Methods and struct fields live in a type's scope rather than the package's, +// so they are not counted here — the god-object gate bounds those. Each name in +// a grouped var/const block counts separately, because each is independently +// referenceable. +func exportedNames(files []*ast.File) []string { + var names []string + for _, file := range files { + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + // A method belongs to its receiver's scope, not the package's. + if _, isMethod := receiverTypeName(d); isMethod { + continue + } + if d.Name.IsExported() { + names = append(names, d.Name.Name) + } + case *ast.GenDecl: + names = append(names, exportedSpecNames(d)...) + } + } + } + sort.Strings(names) + return names +} + +// exportedSpecNames returns the exported names declared by a type, var or +// const declaration. +func exportedSpecNames(decl *ast.GenDecl) []string { + var names []string + for _, spec := range decl.Specs { + switch s := spec.(type) { + case *ast.TypeSpec: + if s.Name.IsExported() { + names = append(names, s.Name.Name) + } + case *ast.ValueSpec: + for _, ident := range s.Names { + if ident.IsExported() { + names = append(names, ident.Name) + } + } + } + } + return names +} + +// TestPackageExportedSurfaceBudget fails when a package exports more top-level +// identifiers than its pin allows. +// +// This is the seam-width gate. All m6t code lives under internal/, so an +// exported name is not a semver commitment — but it is still the surface other +// packages couple to, and a seam that widens unnoticed is how one service +// becomes everyone's dependency. Shrink the surface (unexport the helper, take +// an interface) rather than raising the pin. +func TestPackageExportedSurfaceBudget(t *testing.T) { + var violations []string + for _, dir := range packageDirs(t) { + pin, pinned := structuralPins[dir] + if !pinned { + continue // reported by TestEveryPackageIsPinned + } + names := exportedNames(parsePackage(t, dir)) + t.Logf("%-20s %d exported (ceiling %d): %s", + dir, len(names), pin.exported, strings.Join(names, " ")) + if len(names) > pin.exported { + violations = append(violations, fmt.Sprintf( + "%s exports %d identifiers (%s), exceeding its pin of %d — unexport what callers do not need, or justify the wider seam in this PR", + dir, len(names), strings.Join(names, ", "), pin.exported)) + } + } + sort.Strings(violations) + + if len(violations) > 0 { + t.Errorf("exported-surface budget exceeded:\n %s", strings.Join(violations, "\n ")) + } +} + +// TestExportedNamesCountsPackageScopeOnly pins the metric itself. Without it, a +// refactor that quietly started counting methods, or stopped counting grouped +// consts, would move every measurement at once and still look green. +func TestExportedNamesCountsPackageScopeOnly(t *testing.T) { + const src = `package sample + +type Exported struct{ Field int } +type unexported struct{} + +func (e Exported) Method() {} +func (e *Exported) PointerMethod() {} + +func Fn() {} +func fn() {} + +const ( + ConstA = 1 + ConstB = 2 + constC = 3 +) + +var VarA, varB = 1, 2 +` + want := []string{"ConstA", "ConstB", "Exported", "Fn", "VarA"} + got := exportedNames([]*ast.File{parseSource(t, src)}) + + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("exported names = %v, want %v", got, want) + } +}