From 6356471edd8f4b2e1075910d668887bb3b453971 Mon Sep 17 00:00:00 2001 From: heaven Date: Tue, 4 Aug 2026 01:45:40 +0300 Subject: [PATCH] Replace the AST test guards with typed archguard analyzers under tmvet covering shape and provenance egress plus stage and request seams --- backend/cmd/tmvet/main.go | 21 + backend/internal/archguard/archguard.go | 475 ++++++++++++++++++ backend/internal/archguard/archguard_test.go | 17 + .../internal/archguard/shippingtree_test.go | 114 +++++ .../backend/egresssites/egresssites.go | 153 ++++++ .../backend/internal/config/config.go | 12 + .../textmachine/backend/internal/llm/llm.go | 45 ++ .../backend/internal/pipeline/pipeline.go | 6 + .../backend/requestsites/requestsites.go | 94 ++++ .../backend/stagesites/stagesites.go | 132 +++++ .../internal/pipeline/content_routing_test.go | 51 +- .../pipeline/synthetic_stage_seam_test.go | 266 +--------- 12 files changed, 1080 insertions(+), 306 deletions(-) create mode 100644 backend/cmd/tmvet/main.go create mode 100644 backend/internal/archguard/archguard.go create mode 100644 backend/internal/archguard/archguard_test.go create mode 100644 backend/internal/archguard/shippingtree_test.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/egresssites/egresssites.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/internal/config/config.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/internal/llm/llm.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/internal/pipeline/pipeline.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/requestsites/requestsites.go create mode 100644 backend/internal/archguard/testdata/src/textmachine/backend/stagesites/stagesites.go diff --git a/backend/cmd/tmvet/main.go b/backend/cmd/tmvet/main.go new file mode 100644 index 00000000..502c191f --- /dev/null +++ b/backend/cmd/tmvet/main.go @@ -0,0 +1,21 @@ +// tmvet is the project's vet tool: `go vet -vettool=$(tmvet) ./...` runs the architectural invariants, +// from `make battery` and CI. +// +// It REPLACES the standard vet suite for that invocation rather than adding to it (measured: findings +// plain `go vet` reports disappear under -vettool), which is why the Makefile runs both passes. A doc +// here once said "alongside"; it was wrong, and believing it would have silently dropped every standard +// check the day someone simplified the Makefile down to one line. +// +// A separate binary is needed because `go test -vet=` refuses anything but its built-in list +// ("-vet argument must be a supported analyzer") and `go test` ignores GOFLAGS=-vettool — measured, not +// assumed. That is why the invariants used to live in _test.go files: it was the only way to make them +// run on the ordinary battery. With `make battery` as the single entry point they no longer have to. +package main + +import ( + "golang.org/x/tools/go/analysis/multichecker" + + "textmachine/backend/internal/archguard" +) + +func main() { multichecker.Main(archguard.Analyzers()...) } diff --git a/backend/internal/archguard/archguard.go b/backend/internal/archguard/archguard.go new file mode 100644 index 00000000..6bbdc782 --- /dev/null +++ b/backend/internal/archguard/archguard.go @@ -0,0 +1,475 @@ +// Package archguard holds the architectural invariants as go/analysis analyzers, run by `go vet +// -vettool` from `make battery` and CI. +// +// The defect they exist for was never "somebody forgot a key" — it was the SHAPE of the code. Engine +// -internal calls each hand-built their own config.Stage, and a Go literal silently zero-fills every +// field it does not name, so adding the reasoning knob left three sites riding the vendor default +// (D39.87). Adding the field in three places would have left the fourth site, tomorrow, just as broken. +// The literals are gone; these analyzers are what keeps them gone. +// +// WHY TYPES AND NOT SYNTAX. The predecessors walked the AST and resolved types by IMPORT PATH, which +// works but has to enumerate the ways a type can be SPELLED — and each enumeration was found by planting +// a bypass and watching the guard report ok (a byte scan was defeated five ways, the first AST version +// two more). types.Info closes that axis: an alias import, a dot-import, a `type X = config.Stage` and +// the elided element type in `[]config.Stage{{…}}` all resolve to the same types.Type. +// +// WHAT IS NOT CAUGHT. These guards are a floor, not a proof. Twice now the thing that broke them was a +// COMMENT claiming closure the code did not have, so the rule here is: a gap is named below and carries +// a fixture in testdata with no expectation comment, which makes analysistest go red the day it starts +// being reported. The list is what has been planted and measured — not a proof of exhaustiveness. +// +// - Node kind, both seams: a Stage or Request obtained by struct EMBEDDING (`type w struct{ +// config.Stage }`), by `make([]config.Stage, 1)` then assigning fields, or through a generic zero +// value (`zeroOf[config.Stage]()`) is not reported. Each yields the zero value with Reasoning +// empty, which is exactly the D39.87 defect. Fixtures: stagesites.go, "documented misses". The +// construction that ENDS this class is a type change — making the zero value unusable outside its +// package — not a longer switch, which is why the switch is not being extended further. +// - Type reach, egress: typeReaches walks type CONSTRUCTORS, and stops at a named type. A signature +// that hides the pair inside one — `type batch struct{ reqs []llm.LLMRequest }`, then +// `Complete(b batch) error` — is not reported. Descending into every named type's underlying would +// reach half the module through context.Context and make the reports hard to predict. +// - Name and shape, egress: a provider call is recognised either by carrying the llm.LLMRequest → +// llm.LLMResponse pair (name-independent) or by being spelled `Complete` against internal/llm. A +// helper that is called something else AND launders the pair through its own types — `func Ask(ctx, +// prompt string) (string, error)` building the request inside — is invisible to both halves. +// - Reflection, egress: a call made through reflect.Value.Call is spelled `Call` and typed +// func([]reflect.Value) []reflect.Value, so neither half can see the provider behind it. No static +// analyzer closes this one; it is named so that nobody has to find it twice. +// +// WHAT WAS GIVEN UP. +// +// The predecessors walked the whole REPOSITORY, reaching eval/ — Go files outside the module, where the +// original defect's fourth call site lived. go/analysis cannot: it needs a package the loader can build, +// and eval/ has no go.mod (`go build` there fails with "cannot find main module"). Measured before +// dropping it: eval/ holds three programs, one of which imports config and pipeline, and NONE constructs +// a Stage or a Request. So the reach guarded nothing, and the polygon is scratch space by design +// (CLAUDE.md) — gating it is not worth a constraint on the guard's form. If a harness there ever needs +// to be covered, the honest fix is a go.mod in eval/, not a syntax-only walk here. +// +// They also read every .go file regardless of BUILD TAGS, which go/analysis cannot do either: a pass +// sees only the files its build configuration selects. `make battery` therefore vets two configurations +// — default and `live` — and a file behind any third tag is not covered by anything. That is a real +// reach the byte scan had; a new tag in this module needs a matching vet line in the Makefile. +package archguard + +import ( + "go/ast" + "go/types" + "path/filepath" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +const ( + configPkg = "textmachine/backend/internal/config" + pipelinePkg = "textmachine/backend/internal/pipeline" + llmPkg = "textmachine/backend/internal/llm" + + // The provider-neutral pair every real egress carries: a request goes in, a response comes out. + llmRequestType = "LLMRequest" + llmResponseType = "LLMResponse" +) + +// Analyzers is every invariant this package enforces. Both drivers — cmd/tmvet and the +// shipping-tree test — take the whole slice, and that test asserts its length, so an analyzer +// dropped from here fails loudly instead of silently enforcing nothing. +func Analyzers() []*analysis.Analyzer { + return []*analysis.Analyzer{StageSeam, RequestSeam, EgressSeam} +} + +// exempt reports whether the file holding n is one of the named exceptions, identified by IMPORT PATH +// plus base name and compared exactly. +// +// The obvious spelling — HasSuffix over the absolute filename — was wrong in both directions, and both +// were reproduced: any nested directory or symlink whose tail happened to replay +// ".../backend/internal/pipeline/stagerun.go" INHERITED the exemption, while the intended files LOST it +// on any checkout where the module directory is not literally named "backend". An import path is +// canonical, so neither can happen. PositionFor(…, false) is deliberate: the adjusted position honours +// //line directives, which would let a generated file claim another file's identity. +func exempt(pass *analysis.Pass, n ast.Node, files []string) bool { + id := strings.TrimSuffix(pass.Pkg.Path(), "_test") + "/" + + filepath.Base(pass.Fset.PositionFor(n.Pos(), false).Filename) + for _, f := range files { + if f == id { + return true + } + } + return false +} + +// inPackage reports whether the pass is analyzing pkgPath, counting its EXTERNAL test package (path +// "_test") as the same package. +// +// Callers use this to SKIP a package, so the TrimSuffix widens an exemption rather than closing a hole +// — an earlier comment here claimed the opposite, and the measurement is unambiguous: with a +// `package config_test` file building a Stage, dropping the TrimSuffix makes it report (exit 1) and +// keeping it makes it silent (exit 0). That is intended — a package's external tests are the seam's own +// test surface — but it is an exemption, and the cost is that any file declaring `package config_test` +// or `package llm_test` may build Stages and call providers without a word. +func inPackage(pass *analysis.Pass, pkgPath string) bool { + return strings.TrimSuffix(pass.Pkg.Path(), "_test") == pkgPath +} + +// isConversion reports whether call is a type conversion T(x) rather than a function call: the callee +// expression denotes a TYPE, which go/types records in Types[fun].IsType(). +func isConversion(pass *analysis.Pass, call *ast.CallExpr) bool { + tv, ok := pass.TypesInfo.Types[call.Fun] + return ok && tv.IsType() +} + +// isNamed reports whether t is exactly pkgPath.name, seeing through aliases. It does NOT unwrap +// pointers, slices or maps: `[]config.Stage` as a TYPE is a legitimate field, while each ELEMENT +// literal inside it types as config.Stage on its own and is caught there. +// +// The pointer unwrap this used to do was load-bearing in ONE place and a false positive everywhere +// else (measured both ways): `[]*config.Stage{{…}}` types its elided element as *config.Stage, so a +// literal needs the deref — but `var s *config.Stage` and `(*config.Stage)(p)` construct no Stage at +// all, and go vet has no per-line suppression, so reporting them made the gate unarguable. Only the +// literal branch derefs now. +func isNamed(t types.Type, pkgPath, name string) bool { + named, ok := types.Unalias(t).(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj != nil && obj.Pkg() != nil && obj.Pkg().Path() == pkgPath && obj.Name() == name +} + +// deref removes one pointer level, for the composite-literal branch only. +func deref(t types.Type) types.Type { + if p, ok := types.Unalias(t).(*types.Pointer); ok { + return p.Elem() + } + return t +} + +// typeReaches reports whether t, or any type reachable from it through the type CONSTRUCTORS — pointer, +// slice, array, map, channel, signature, tuple, anonymous struct/interface, generic type argument — +// is a named type satisfying want. +// +// The recursion is what the flat version lacked: `Complete(ctx, reqs []llm.LLMRequest) +// ([]*llm.LLMResponse, error)` unwrapped to a slice and stopped, so the batch form of our own client +// passed the egress guard in silence (D39.96 §3в-2). It stops at a NAMED type — see the package doc. +func typeReaches(t types.Type, want func(*types.Named) bool) bool { + return reaches(t, want, map[types.Type]bool{}) +} + +func reaches(t types.Type, want func(*types.Named) bool, seen map[types.Type]bool) bool { + t = types.Unalias(t) + if t == nil || seen[t] { + return false // a recursive type (`type node struct{ next *node }`) would not terminate + } + seen[t] = true + switch v := t.(type) { + case *types.Named: + if want(v) { + return true + } + // Type ARGUMENTS are still constructors: `Result[llm.LLMResponse]` carries the response. + for args, i := v.TypeArgs(), 0; args != nil && i < args.Len(); i++ { + if reaches(args.At(i), want, seen) { + return true + } + } + case *types.Pointer: + return reaches(v.Elem(), want, seen) + case *types.Slice: + return reaches(v.Elem(), want, seen) + case *types.Array: + return reaches(v.Elem(), want, seen) + case *types.Chan: + return reaches(v.Elem(), want, seen) + case *types.Map: + return reaches(v.Key(), want, seen) || reaches(v.Elem(), want, seen) + case *types.Tuple: + for i := 0; i < v.Len(); i++ { + if reaches(v.At(i).Type(), want, seen) { + return true + } + } + case *types.Signature: + return reaches(v.Params(), want, seen) || reaches(v.Results(), want, seen) + case *types.Struct: // reachable only while ANONYMOUS — a named struct stops the walk above + for i := 0; i < v.NumFields(); i++ { + if reaches(v.Field(i).Type(), want, seen) { + return true + } + } + case *types.Interface: // likewise anonymous: `interface{ Complete(llm.LLMRequest) error }` + for i := 0; i < v.NumMethods(); i++ { + if reaches(v.Method(i).Type(), want, seen) { + return true + } + } + for i := 0; i < v.NumEmbeddeds(); i++ { + if reaches(v.EmbeddedType(i), want, seen) { + return true + } + } + case *types.TypeParam: + // Inside a generic body the request can travel as the parameter itself; what pins it down is + // the CONSTRAINT. Underlying() is deliberate and is the one place this walk goes THROUGH a + // named type: a constraint is usually named (`type reqBound interface{ … }`), and stopping + // there — the rule everywhere else — reaches nothing at all. It is safe here precisely because + // a constraint is a type-level bound rather than a data structure: it cannot drag in the + // module the way descending into every named struct would. + return reaches(v.Constraint().Underlying(), want, seen) + case *types.Union: // the term list of such a constraint + for i := 0; i < v.Len(); i++ { + if reaches(v.Term(i).Type(), want, seen) { + return true + } + } + } + return false +} + +func fromPkg(pkgPath string) func(*types.Named) bool { + return func(n *types.Named) bool { + o := n.Obj() + return o != nil && o.Pkg() != nil && o.Pkg().Path() == pkgPath + } +} + +func namedIs(pkgPath, name string) func(*types.Named) bool { + return func(n *types.Named) bool { + o := n.Obj() + return o != nil && o.Pkg() != nil && o.Pkg().Path() == pkgPath && o.Name() == name + } +} + +// seamRule is one "there is exactly ONE place this type may be built" invariant. Stage and Request +// differ only in the type protected, the files allowed to build it and the sentence printed — sharing +// the walk is what stopped the two from drifting apart: Request used to inspect ONE node kind against +// Stage's four, so `var r pipeline.Request` and `new(pipeline.Request)` passed both generations of the +// guard (D39.96 §3б). +type seamRule struct { + pkgPath string // package declaring the protected type + name string // its name + ownPkg string // a package that may build it freely ("" — none; exempt files carry it instead) + exempt []string // file path suffixes allowed to build it + msg string +} + +func (s seamRule) run(pass *analysis.Pass) (any, error) { + if s.ownPkg != "" && inPackage(pass, s.ownPkg) { + return nil, nil // the type's OWN package: the seam lives here + } + ins := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + ins.Preorder([]ast.Node{ + (*ast.CompositeLit)(nil), (*ast.ValueSpec)(nil), (*ast.CallExpr)(nil), (*ast.TypeSpec)(nil), + (*ast.FuncType)(nil), + }, func(n ast.Node) { + if exempt(pass, n, s.exempt) { + return + } + is := func(t types.Type) bool { return isNamed(t, s.pkgPath, s.name) } + hit := false + switch v := n.(type) { + case *ast.CompositeLit: + // TypeOf resolves the ELIDED element type too, so []T{{…}} and map[k]T{k:{…}} are caught + // on their elements; []*T{{…}} types that element as *T, which is why this one derefs. + hit = is(deref(pass.TypesInfo.TypeOf(v))) + case *ast.ValueSpec: // var s T — then assign field by field, no literal anywhere + hit = v.Type != nil && is(pass.TypesInfo.TypeOf(v.Type)) + case *ast.FuncType: + // A NAMED RESULT is the same zero-value declaration as `var s T`, one keystroke apart: + // `func f() (s config.Stage) { s.Name = …; return s }`. Params are not declarations — + // the caller supplies them — so only results count. + if v.Results != nil { + for _, f := range v.Results.List { + if len(f.Names) > 0 && is(pass.TypesInfo.TypeOf(f.Type)) { + hit = true + } + } + } + case *ast.CallExpr: + if id, ok := v.Fun.(*ast.Ident); ok && len(v.Args) == 1 && + pass.TypesInfo.Uses[id] == types.Universe.Lookup("new") { + hit = is(pass.TypesInfo.TypeOf(v.Args[0])) // new(T) + } else if isConversion(pass, v) { + // T(twin{…}) — a conversion from a structurally identical type builds a T just as + // a literal does, and drops exactly the same fields. + hit = is(pass.TypesInfo.TypeOf(v)) + } + case *ast.TypeSpec: // type myT = T / type myT T + hit = is(pass.TypesInfo.TypeOf(v.Type)) + } + if hit { + pass.Reportf(n.Pos(), "%s", s.msg) + } + }) + return nil, nil +} + +// StageSeam: outside the config package there is exactly ONE way to obtain a Stage for an engine-internal +// call — config.InternalCall.Stage() — so a fifth call site inherits every decision the seam took instead +// of re-taking them by omission. +var StageSeam = &analysis.Analyzer{ + Name: "stageseam", + Doc: "config.Stage must be built only through config.InternalCall.Stage()", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: stageSeam.run, +} + +var stageSeam = seamRule{ + pkgPath: configPkg, + name: "Stage", + ownPkg: configPkg, + exempt: []string{ + "textmachine/backend/internal/pipeline/fewshot_test.go", // pure predicate test: no wire, no ledger, no checkpoint + // Its subject IS stage inheritance: it builds a PARENT stage to assert what repair derives from it. + "textmachine/backend/internal/pipeline/synthetic_stage_seam_test.go", + }, + msg: "an engine-internal call must derive its stage through config.InternalCall.Stage(), " + + "not by building one — a hand-built stage silently drops reasoning/temperature (D39.87)", +} + +// RequestSeam: the tuple RequestHash addresses a checkpoint by is assembled in exactly ONE place +// (Runner.attemptRequest). Two assemblies agreeing is not a property anyone maintains by eye — it held +// only while the omitted fields happened to be zero, and the effort knob makes them non-zero. The guard +// is "no Request BUILT outside the seam", not "no RequestHash(Request{…})": the second form is one +// refactor away from invisible (hoist the literal into a variable), and the construction is the hazard. +var RequestSeam = &analysis.Analyzer{ + Name: "requestseam", + Doc: "pipeline.Request must be assembled only in Runner.attemptRequest", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: requestSeam.run, +} + +var requestSeam = seamRule{ + pkgPath: pipelinePkg, + name: "Request", + // No ownPkg: pipeline is where Request lives AND where every drifting re-assembly would live, so + // the exemption is per-file rather than per-package. + exempt: []string{ + "textmachine/backend/internal/pipeline/stagerun.go", // owns attemptRequest — the seam itself + // render.go — where Request is DEFINED and hashed — used to be exempt too. Measured: with the + // exemption removed the tree is still clean, because declaring the type and hashing a + // parameter construct nothing. An exemption that covers no existing code is not neutral; it + // silently pre-approves the next literal somebody adds there. + // The tests OF the hashing contract legitimately build Requests — that is their subject. + "textmachine/backend/internal/pipeline/render_test.go", + "textmachine/backend/internal/pipeline/render_memory_test.go", + }, + msg: "a call's request identity must come from Runner.attemptRequest, not a re-assembled " + + "Request literal — a probe that drifts from the attempt turns the role sub-budget off", +} + +// EgressSeam: exactly ONE production call site may reach a provider (stagerun.go runAttempt, behind +// clientFor's label assert), plus the live-gated conformance probe. A new egress path — a Ф2 judge, an +// annotator — must extend the routing assert, not slip past it. +// +// The predecessor matched the BYTES `.Complete(`, which cannot tell our client's method from any other +// type's method of that name and had to exempt this guard's own source from matching its own needle. +// Two halves replace it, and a call is reported if EITHER fires: +// +// - SHAPE (isEgressShaped) — the callee carries the llm.LLMRequest → llm.LLMResponse pair. Name +// -independent, so a helper called Do or Ask is caught exactly like Complete, and it reads through +// slices and pointers, so the batch form is caught too. +// - PROVENANCE (isCompleteAgainstLLM) — the callee is spelled Complete and either is declared in +// internal/llm or mentions an internal/llm type at all. This is the parity half: the byte scan +// matched every `.Complete(`, and a client whose request/response types get renamed would slip +// past the shape half alone. +var EgressSeam = &analysis.Analyzer{ + Name: "egressseam", + Doc: "provider egress must stay in one place (stagerun.go runAttempt)", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: runEgressSeam, +} + +var egressExempt = []string{ + "textmachine/backend/internal/pipeline/stagerun.go", // runAttempt — the single production egress + "textmachine/backend/internal/pipeline/live_conformance_test.go", // live-gated probe: calls the real catalog on purpose +} + +func runEgressSeam(pass *analysis.Pass) (any, error) { + if inPackage(pass, llmPkg) { + return nil, nil // the adapters ARE the implementation of Complete — the seam, not a bypass + } + const msg = "provider egress must stay in ONE place (stagerun.go runAttempt, behind clientFor's " + + "label assert) — route this through runAttempt or extend the routing assert deliberately" + ins := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + ins.Preorder([]ast.Node{(*ast.CallExpr)(nil)}, func(n ast.Node) { + call := n.(*ast.CallExpr) + if exempt(pass, n, egressExempt) { + return + } + tv, ok := pass.TypesInfo.Types[call.Fun] + if !ok || tv.IsType() { + return // a conversion calls nothing + } + // The callee's signature is NOT always available: through a type PARAMETER, Underlying() is + // the constraint interface, and a guard that required a signature returned right here — a + // whole generic provider path was invisible (found by planting `runJudge[F providerFn](…, + // ask F, …)` and watching `go vet -vettool` exit 0). The shape half therefore reads the CALL + // SITE, which go/types resolves in every spelling; the signature is only what provenance + // needs, and it may be nil. + sig, _ := tv.Type.Underlying().(*types.Signature) + if isEgressShaped(pass, call, sig) || isCompleteAgainstLLM(pass, call.Fun, sig) { + pass.Reportf(call.Pos(), "%s", msg) + } + }) + return nil, nil +} + +// isEgressShaped reports whether this call has the shape every provider call has: an llm.LLMRequest +// goes in and an llm.LLMResponse comes out. A wrapper declared OUTSIDE internal/llm cannot launder +// this: to present its own types it has to call the real client somewhere, and that inner call carries +// the pair. A wrapper declared INSIDE internal/llm can, because this analyzer skips that package. +// +// Arguments and the call's own result type are read from the call site, so the answer does not depend +// on how the callee is spelled — a plain func, a method, a func-typed field, a type parameter. The +// declared signature is consulted as well, which covers the one case the call site cannot express: a +// request arriving as a variadic tail that this call happens to leave empty. +func isEgressShaped(pass *analysis.Pass, call *ast.CallExpr, sig *types.Signature) bool { + wantsRequest := func() bool { + for _, a := range call.Args { + if typeReaches(pass.TypesInfo.TypeOf(a), namedIs(llmPkg, llmRequestType)) { + return true + } + } + return sig != nil && typeReaches(sig.Params(), namedIs(llmPkg, llmRequestType)) + } + yieldsResponse := func() bool { + if typeReaches(pass.TypesInfo.TypeOf(call), namedIs(llmPkg, llmResponseType)) { + return true + } + return sig != nil && typeReaches(sig.Results(), namedIs(llmPkg, llmResponseType)) + } + return wantsRequest() && yieldsResponse() +} + +// isCompleteAgainstLLM reports whether the callee is spelled Complete and belongs to internal/llm by +// declaration or by signature. +// +// Resolving the callee through Uses is what the previous version got wrong: it read Selections, which +// go/types does not populate for a QUALIFIED IDENTIFIER, so a package-level `llm.Complete(…)` — a second +// egress reachable from anywhere, since internal/llm is skipped wholesale — returned nil and the +// analyzer walked away (D39.96 §3в-1). Uses is populated for both forms, and taking the callee ident +// covers a dot-imported `Complete(…)` as well. +func isCompleteAgainstLLM(pass *analysis.Pass, fun ast.Expr, sig *types.Signature) bool { + id := calleeIdent(fun) + if id == nil || id.Name != "Complete" { + return false + } + obj := pass.TypesInfo.Uses[id] + declaredInLLM := obj != nil && obj.Pkg() != nil && obj.Pkg().Path() == llmPkg + return declaredInLLM || + typeReaches(sig.Params(), fromPkg(llmPkg)) || typeReaches(sig.Results(), fromPkg(llmPkg)) +} + +// calleeIdent is the name a call is written under: the selector's tail for x.f() and llm.f(), the +// identifier itself for a dot-imported or local f(). +func calleeIdent(fun ast.Expr) *ast.Ident { + switch v := fun.(type) { + case *ast.Ident: + return v + case *ast.SelectorExpr: + return v.Sel + } + return nil +} diff --git a/backend/internal/archguard/archguard_test.go b/backend/internal/archguard/archguard_test.go new file mode 100644 index 00000000..6d502a28 --- /dev/null +++ b/backend/internal/archguard/archguard_test.go @@ -0,0 +1,17 @@ +package archguard + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +// The acceptance set: every bypass that ever defeated a predecessor, replayed against the analyzer. +// The fixtures carry `// want` on each planted line, so a form the analyzer stops catching fails HERE +// rather than in the shipping tree six months later. The negative controls in the same files are the +// other half — a guard that reports everything is as useless as one that reports nothing. +func TestSeamsCatchEveryKnownBypass(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), StageSeam, "textmachine/backend/stagesites") + analysistest.Run(t, analysistest.TestData(), RequestSeam, "textmachine/backend/requestsites") + analysistest.Run(t, analysistest.TestData(), EgressSeam, "textmachine/backend/egresssites") +} diff --git a/backend/internal/archguard/shippingtree_test.go b/backend/internal/archguard/shippingtree_test.go new file mode 100644 index 00000000..9aa720b6 --- /dev/null +++ b/backend/internal/archguard/shippingtree_test.go @@ -0,0 +1,114 @@ +package archguard + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/tools/go/analysis/checker" + "golang.org/x/tools/go/packages" +) + +// packageFloor is the smallest number of packages a healthy load returns. Without it, a run that loads +// NOTHING — a bad pattern, a rename, a broken module — is indistinguishable from a clean tree and the +// guard silently becomes decoration. `go vet` is loud about a package that fails to build, but it is +// perfectly quiet about analyzing zero of them, so the floor is asserted rather than assumed. The module +// holds ~20 packages (~40 with test variants); the floor catches ZERO, it does not track the count. +const packageFloor = 15 + +// TestInvariantsHoldInTheShippingTree runs the analyzers over the REAL module from an ordinary `go test`. +// +// The invariants moved out of _test.go files into go/analysis, which made them depend on somebody typing +// `make vet` — `go test -vet=` refuses a custom analyzer, so the ordinary battery stopped carrying them. +// This puts them back without a second implementation: the same Analyzers() the vet tool wires up, driven +// here through the checker API instead of unitchecker. +func TestInvariantsHoldInTheShippingTree(t *testing.T) { + // Every analyzer, from the same slice cmd/tmvet uses: dropping one from Analyzers() must fail HERE, + // which is the property the old walk's file floor used to provide for its own coverage. + if got := len(Analyzers()); got != 3 { + t.Fatalf("Analyzers() returns %d analyzers, want 3 (stageseam, requestseam, egressseam) — an "+ + "invariant that is not in this slice is enforced by nothing", got) + } + + cfg := &packages.Config{Mode: packages.LoadAllSyntax, Tests: true, Dir: "../.."} + pkgs, err := packages.Load(cfg, "textmachine/backend/...") + if err != nil { + t.Fatalf("load the module: %v", err) + } + if len(pkgs) < packageFloor { + t.Fatalf("loaded only %d packages (floor %d) — the pattern matches nothing or the module is "+ + "broken, which makes this guard decoration rather than a guard", len(pkgs), packageFloor) + } + var loadErrs []string + packages.Visit(pkgs, nil, func(p *packages.Package) { + for _, e := range p.Errors { + loadErrs = append(loadErrs, e.Error()) + } + }) + if len(loadErrs) > 0 { + // A package that does not type-check is analyzed as nothing at all — silently, unless said here. + t.Fatalf("packages failed to load, so they were NOT analyzed:\n %s", strings.Join(loadErrs, "\n ")) + } + + graph, err := checker.Analyze(Analyzers(), pkgs, nil) + if err != nil { + t.Fatalf("run the analyzers: %v", err) + } + var found []string + for act := range graph.All() { + if act.Err != nil { + found = append(found, act.Analyzer.Name+" errored on "+act.Package.PkgPath+": "+act.Err.Error()) + } + for _, d := range act.Diagnostics { + found = append(found, act.Package.Fset.Position(d.Pos).String()+": "+d.Message) + } + } + if len(found) > 0 { + t.Fatalf("architectural invariants violated in the shipping tree:\n %s", strings.Join(found, "\n ")) + } + assertGuardVocabularyIsLive(t, pkgs) +} + +// assertGuardVocabularyIsLive fails when the guards name something that no longer exists. +// +// Both halves of this analyzer set are keyed on STRINGS — the exempt lists name files, and the egress +// shape names two types. A rename on either side does not break a build and does not fail a test: the +// exemption simply stops matching (a legitimate file starts being reported, which is loud) or, far +// worse, the shape half stops matching anything at all and the guard goes quietly decorative. That +// second direction is exactly the failure mode this whole package was built to end, so it is asserted +// rather than trusted. +func assertGuardVocabularyIsLive(t *testing.T, pkgs []*packages.Package) { + t.Helper() + const modulePrefix = "textmachine/backend/" + for _, list := range [][]string{stageSeam.exempt, requestSeam.exempt, egressExempt} { + for _, f := range list { + rel, ok := strings.CutPrefix(f, modulePrefix) + if !ok { + t.Errorf("exemption %q is not inside this module — it can never match", f) + continue + } + if _, err := os.Stat(filepath.Join("../..", rel)); err != nil { + t.Errorf("exemption %q names a file that does not exist (%v) — a rename left it behind, "+ + "and an exemption nobody can see is how a seam re-opens", f, err) + } + } + } + var llmPkgFound bool + for _, p := range pkgs { + if p.PkgPath != llmPkg || p.Types == nil { + continue + } + llmPkgFound = true + for _, name := range []string{llmRequestType, llmResponseType} { + if p.Types.Scope().Lookup(name) == nil { + t.Errorf("the egress shape is keyed on %s.%s, which no longer exists — renaming it "+ + "silences the name-independent half of the guard without failing anything else", + llmPkg, name) + } + } + } + if !llmPkgFound { + t.Errorf("package %s was not loaded, so the egress vocabulary could not be checked", llmPkg) + } +} diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/egresssites/egresssites.go b/backend/internal/archguard/testdata/src/textmachine/backend/egresssites/egresssites.go new file mode 100644 index 00000000..9c5336ea --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/egresssites/egresssites.go @@ -0,0 +1,153 @@ +// Every known way to reach a provider outside stagerun.go runAttempt, so the call skips clientFor's +// content-label assert. Cases 13/17 defeated a predecessor; 18–21 defeated the FIRST analyzer and were +// found by planting them during the D39.96 acceptance, with the guard reporting ok. +package egresssites + +import ( + "context" + + "textmachine/backend/internal/llm" +) + +// 13. through the client interface — what a second egress looks like when it is written plainly. +func viaInterface(ctx context.Context, c llm.Client) string { + r, _ := c.Complete(ctx, llm.LLMRequest{}) // want "provider egress must stay in ONE place" + return r.Text +} + +// the same bypass through the concrete adapter rather than the interface. +func viaConcrete(ctx context.Context, h llm.HTTP) string { + r, _ := h.Complete(ctx, llm.LLMRequest{}) // want "provider egress must stay in ONE place" + return r.Text +} + +// 17. a Complete reached through an interface declared HERE, not in llm. The method object then belongs +// to this package, so "declared in llm" reports nothing — while the call still reaches a real provider. +type legacyLocal interface { + Complete(prompt string) (llm.Reply, error) +} + +func viaLocalInterface(c llm.LegacyClient) string { + var x legacyLocal = c + r, _ := x.Complete("hi") // want "provider egress must stay in ONE place" + return r.Text +} + +// 18. PACKAGE FUNCTION. `llm.Complete` is a qualified identifier, which go/types records in Uses and NOT +// in Selections — the first analyzer read Selections, got nil and returned. internal/llm is skipped +// wholesale, so a helper parked there was a second egress visible to nothing. The signature carries no +// llm type on purpose: resolving the callee's declaring package is the ONLY thing that reports this. +func viaPackageFunc(ctx context.Context) string { + s, _ := llm.Complete(ctx, "hi") // want "provider egress must stay in ONE place" + return s +} + +// 19. NESTED IN A COMPOSITE. The batch form of our own client: the pair is one slice deep, and a flat +// signature scan unwrapped a pointer, saw a slice and stopped. +type batcher interface { + Complete(ctx context.Context, reqs []llm.LLMRequest) ([]*llm.LLMResponse, error) +} + +func viaBatchInterface(ctx context.Context, b batcher) int { + rs, _ := b.Complete(ctx, []llm.LLMRequest{{}}) // want "provider egress must stay in ONE place" + return len(rs) +} + +func viaBatchPackageFunc(ctx context.Context) int { + rs, _ := llm.BatchComplete(ctx, []llm.LLMRequest{{}}) // want "provider egress must stay in ONE place" + return len(rs) +} + +// 20. ANOTHER NAME. Keying on `Complete` is what the deleted byte scan did; a helper called anything +// else was free. Carrying the request/response pair is the part that cannot be renamed away. +func viaOtherName(ctx context.Context) string { + r, _ := llm.Do(ctx, llm.LLMRequest{}) // want "provider egress must stay in ONE place" + return r.Text +} + +// 21. METHOD VALUE. The selector is hoisted into a variable, so the call site is a bare identifier and +// no selector named Complete appears on it at all. +func viaMethodValue(ctx context.Context, c llm.Client) string { + f := c.Complete + r, _ := f(ctx, llm.LLMRequest{}) // want "provider egress must stay in ONE place" + return r.Text +} + +// 27. GENERIC DRIVE SITE. The callee is a type PARAMETER, whose Underlying() is the constraint +// INTERFACE and not a signature — a guard that required a signature returned before testing anything, +// and this entire path (a realistic Ф2 judge) was invisible to both drivers. Nothing about the call is +// unusual; only the callee's spelling is. +type providerFn interface { + ~func(context.Context, llm.LLMRequest) (*llm.LLMResponse, error) +} + +func viaTypeParam[F providerFn](ctx context.Context, ask F) string { + r, _ := ask(ctx, llm.LLMRequest{}) // want "provider egress must stay in ONE place" + return r.Text +} + +func driveTypeParam(ctx context.Context, c llm.Client) string { + return viaTypeParam(ctx, c.Complete) +} + +var _ = driveTypeParam + +// 28. REQUEST TRAVELLING AS A TYPE PARAMETER. The argument's type is Q, and what pins Q to the request +// is its CONSTRAINT — so the type walk has to enter the constraint's term list, not stop at the +// parameter. Without that the call site sees an argument of type "Q" and reaches nothing. +type reqBound interface { + llm.LLMRequest | *llm.LLMRequest +} + +func viaBoundTypeParam[Q reqBound](ctx context.Context, ask func(context.Context, Q) (*llm.LLMResponse, error), q Q) string { + r, _ := ask(ctx, q) // want "provider egress must stay in ONE place" + return r.Text +} + +var _ = viaBoundTypeParam[llm.LLMRequest] + +// --- negative controls: these must NOT be reported --- + +// The byte scan matched ANY `.Complete(`. A Complete of our own that has nothing to do with a provider +// must stay silent. +type job struct{} + +func (job) Complete(string) (string, error) { return "", nil } + +func ownComplete(j job) string { + s, _ := j.Complete("hi") + return s +} + +// A constructor touches llm types in its RESULT but carries no request — building a client is not +// calling one, and reporting it would make the guard unusable. +func buildsAClient() llm.Client { return llm.New() } + +var _, _ = ownComplete, buildsAClient + +// --- documented misses: NOT controls. Each is a gap named in the package doc, planted here so that +// closing it later starts from a failing case rather than from a discovery. + +// A. Both halves launder the pair through their own types: the name is unknown AND the request never +// appears in the signature. Catching this needs the seam to be a type, not an analyzer. +func gapLaunderedPair(ctx context.Context) string { + s, _ := llm.Ask(ctx, "prompt") + return s +} + +// B. The pair hidden inside NAMED local types. typeReaches walks constructors and stops at a named +// type, so neither side is visible; descending into every named underlying reaches half the module. +type batch struct{ reqs []llm.LLMRequest } + +type replies struct{ resps []*llm.LLMResponse } + +type hidden interface { + Complete(b batch) (replies, error) +} + +func gapNamedWrapper(h hidden) int { + r, _ := h.Complete(batch{}) + return len(r.resps) +} + +var _, _ = gapLaunderedPair, gapNamedWrapper diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/internal/config/config.go b/backend/internal/archguard/testdata/src/textmachine/backend/internal/config/config.go new file mode 100644 index 00000000..b80d1a8d --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/internal/config/config.go @@ -0,0 +1,12 @@ +package config + +type Stage struct { + Name, Role, Model, Reasoning string + Temperature float64 +} + +type InternalCall struct{ Name, Role, Model, Reasoning string } + +func (c InternalCall) Stage() Stage { + return Stage{Name: c.Name, Role: c.Role, Model: c.Model, Reasoning: c.Reasoning} +} diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/internal/llm/llm.go b/backend/internal/archguard/testdata/src/textmachine/backend/internal/llm/llm.go new file mode 100644 index 00000000..560fdc5c --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/internal/llm/llm.go @@ -0,0 +1,45 @@ +// A miniature of internal/llm: the request/response pair the egress guard keys on, one client +// interface, one adapter, and the package-level shapes that defeated the guard during acceptance. +package llm + +import "context" + +type LLMRequest struct{ Model string } + +type LLMResponse struct{ Text string } + +type Client interface { + Complete(ctx context.Context, req LLMRequest) (*LLMResponse, error) +} + +type HTTP struct{} + +func (HTTP) Complete(ctx context.Context, req LLMRequest) (*LLMResponse, error) { + return &LLMResponse{}, nil +} + +// Complete as a PACKAGE function, and deliberately the HARDEST form of it: a convenience wrapper whose +// signature mentions no llm type at all. A qualified identifier is not a selection, so a guard reading +// Selections resolves nothing here, and with the pair absent from the signature nothing else can stand +// in — only resolving the callee's declaring package reports this call. +func Complete(ctx context.Context, prompt string) (string, error) { return "", nil } + +// BatchComplete hides the pair inside slices. +func BatchComplete(ctx context.Context, reqs []LLMRequest) ([]*LLMResponse, error) { return nil, nil } + +// Do reaches a provider under a name no guard knows in advance. +func Do(ctx context.Context, req LLMRequest) (*LLMResponse, error) { return &LLMResponse{}, nil } + +// Reply is a response type spelled differently than the pair — the provenance half of the guard is +// what covers a client built on it. +type Reply struct{ Text string } + +type LegacyClient interface { + Complete(prompt string) (Reply, error) +} + +// New builds a client: it touches llm types but carries no request, so it is not egress. +func New() Client { return HTTP{} } + +// Ask launders the pair through its own types — the documented miss of both halves. +func Ask(ctx context.Context, prompt string) (string, error) { return "", nil } diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/internal/pipeline/pipeline.go b/backend/internal/archguard/testdata/src/textmachine/backend/internal/pipeline/pipeline.go new file mode 100644 index 00000000..76f00f49 --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/internal/pipeline/pipeline.go @@ -0,0 +1,6 @@ +package pipeline + +type Request struct { + BookID, Stage, Role, Model string + MaxTokens int +} diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/requestsites/requestsites.go b/backend/internal/archguard/testdata/src/textmachine/backend/requestsites/requestsites.go new file mode 100644 index 00000000..23cdf2f6 --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/requestsites/requestsites.go @@ -0,0 +1,94 @@ +// Every known way to build a pipeline.Request outside Runner.attemptRequest. The var/new forms passed +// BOTH the deleted byte guard and the first analyzer — a hole the acceptance proved by execution +// (D39.96 §3б): the request seam inspected one node kind where the stage seam inspected four. +package requestsites + +import ( + "textmachine/backend/internal/pipeline" + + pl "textmachine/backend/internal/pipeline" + + . "textmachine/backend/internal/pipeline" +) + +// 11. HOISTED: the literal moves into a variable and `RequestHash(Request{…})` disappears from the +// source. This is why the guard is "no Request built", not "no RequestHash(Request{…})". +func hoisted() pipeline.Request { + req := pipeline.Request{BookID: "b", Stage: "draft"} // want "request identity must come from" + return req +} + +// 12. slice of Requests, element type elided. +var batch = []pipeline.Request{{BookID: "b"}} // want "request identity must come from" + +// map value, elided the same way. +var mapped = map[string]pipeline.Request{"a": {BookID: "b"}} // want "request identity must come from" + +// alias import of the pipeline package. +var aliased = pl.Request{BookID: "b"} // want "request identity must come from" + +// dot import: no qualifier at all. +var dotted = Request{BookID: "b"} // want "request identity must come from" + +// 22. NO LITERAL AT ALL: declare, then assign field by field. The drift this guard exists to stop needs +// no literal — a re-assembly that omits Reasoning is exactly as wrong spelled this way. +func varForm() pipeline.Request { + var r pipeline.Request // want "request identity must come from" + r.BookID = "b" + r.Stage = "draft" + return r +} + +// 23. new() — a literal-only check never sees it. +func newForm() *pipeline.Request { + return new(pipeline.Request) // want "request identity must come from" +} + +// 24. type alias, then build through the new name. +type myRequest = pipeline.Request // want "request identity must come from" + +var viaAlias = myRequest{BookID: "b"} // want "request identity must come from" + +// 25. defined type over Request: a different type, but it copies the field set, so a literal of it +// drops exactly the same fields and converts back for free. +type derivedRequest pipeline.Request // want "request identity must come from" + +// 26. conversion from a structurally identical local type. +type twin struct { + BookID, Stage, Role, Model string + MaxTokens int +} + +var converted = pipeline.Request(twin{BookID: "b"}) // want "request identity must come from" + +// 27. NAMED RESULT — the same zero-value declaration as case 22, one keystroke apart. The var form was +// caught and this was not, which is the shape of every hole found so far: an enumeration with a gap. +func namedResult() (r pipeline.Request) { // want "request identity must come from" + r.BookID = "b" + return r +} + +// --- negative controls --- + +func takesRequest(r pipeline.Request) string { return r.BookID } + +// A PARAMETER of the protected type is not a declaration — the caller supplies the value, so nothing +// is constructed here and reporting it would flag every function that accepts a Request. +func namedParam(r pipeline.Request) string { return r.Stage } + +// An UNNAMED result declares nothing: the value returned is built at the return site and caught there. +func unnamedResult() pipeline.Request { return takesRequestBack() } + +func takesRequestBack() pipeline.Request { return pipeline.Request{} } // want "request identity must come from" + +// A POINTER declaration constructs no Request — it is nil until something assigns to it. +var pending *pipeline.Request + +var _, _, _, _ = namedParam, unnamedResult, namedResult, pending + +// A []pipeline.Request as a TYPE (a field, a parameter, an empty slice) constructs nothing. +type holder struct{ reqs []pipeline.Request } + +var empty = []pipeline.Request{} + +var _, _, _ = takesRequest, holder{}, empty diff --git a/backend/internal/archguard/testdata/src/textmachine/backend/stagesites/stagesites.go b/backend/internal/archguard/testdata/src/textmachine/backend/stagesites/stagesites.go new file mode 100644 index 00000000..b246ed68 --- /dev/null +++ b/backend/internal/archguard/testdata/src/textmachine/backend/stagesites/stagesites.go @@ -0,0 +1,132 @@ +// Every known way to obtain a config.Stage outside the seam. Each was found by PLANTING it against a +// previous guard and watching that guard report ok — a byte scan was defeated five ways, the first AST +// version two more. They are the acceptance set: the analyzer must catch all of them. +package stagesites + +import ( + "textmachine/backend/internal/config" + + cfg "textmachine/backend/internal/config" + + . "textmachine/backend/internal/config" +) + +// 1. the plain literal — what the original three defect sites looked like. +var plain = config.Stage{Name: "terminology"} // want "engine-internal call must derive its stage" + +// 2. address-of: the literal is still there, one token away. +var ptr = &config.Stage{Name: "classify"} // want "engine-internal call must derive its stage" + +// 3. slice with an ELIDED element type — invisible to a check that only reads CompositeLit.Type. +var slice = []config.Stage{{Name: "repair"}} // want "engine-internal call must derive its stage" + +// 4. map value, elided the same way. +var mapped = map[string]config.Stage{"a": {Name: "annotate"}} // want "engine-internal call must derive its stage" + +// 5. no literal at all: declare, then assign field by field. +func varForm() config.Stage { + var s config.Stage // want "engine-internal call must derive its stage" + s.Name = "annotate" + return s +} + +// 6. new() — a literal-only check never sees it. +func newForm() *config.Stage { + return new(config.Stage) // want "engine-internal call must derive its stage" +} + +// 7. ALIAS IMPORT: `cfg.Stage` is the same type under another local name. A guard that matched the +// package NAME waved this through — found by planting a realistic annotator call site. +var aliased = cfg.Stage{Name: "annotate"} // want "engine-internal call must derive its stage" + +// 8. DOT IMPORT: no qualifier at all. +var dotted = Stage{Name: "judge"} // want "engine-internal call must derive its stage" + +// 9. type alias, then build through the new name. +type myStage = config.Stage // want "engine-internal call must derive its stage" + +var viaAlias = myStage{Name: "select"} // want "engine-internal call must derive its stage" + +// 10. defined type over Stage: a different type, but it copies the field set, so a literal of it drops +// exactly the same fields. +type derivedStage config.Stage // want "engine-internal call must derive its stage" + +// --- negative controls: these must NOT be reported --- + +// The seam itself is how a stage is meant to be obtained. +var viaSeam = config.InternalCall{Name: "terminology", Reasoning: "low"}.Stage() + +// A []config.Stage as a TYPE (a field, a parameter, an empty slice) constructs nothing. +type holder struct{ stages []config.Stage } + +func takesStages(ss []config.Stage) int { return len(ss) } + +var empty = []config.Stage{} + +var _, _, _ = holder{}, takesStages, empty + +// 16. CONVERSION from a structurally identical local type. types.Info knows the result is a Stage; +// the first version of this analyzer only asked about new(), so it walked past. +type twin struct { + Name, Role, Model, Reasoning string + Temperature float64 +} + +var converted = config.Stage(twin{Name: "annotate"}) // want "engine-internal call must derive its stage" + +// 17. NAMED RESULT: identical to case 5 (`var s config.Stage`) but declared in the signature. +func namedResult() (s config.Stage) { // want "engine-internal call must derive its stage" + s.Name = "annotate" + return s +} + +// --- negative control for the accepted false positive: a var DECLARED with the type and assigned from +// the seam is still reported (the var form is how `var s config.Stage; s.Name=…` is caught). Documented +// here so the trade is visible rather than discovered. +var declaredFromSeam config.Stage = config.InternalCall{Name: "terminology"}.Stage() // want "engine-internal call must derive its stage" + +// A POINTER declaration or conversion constructs no Stage — nil until something assigns to it. These +// used to be reported, because the type test unwrapped one pointer level for every branch instead of +// only for composite literals; go vet has no per-line suppression, so a false positive here is a gate +// nobody can keep green honestly. +var pendingPtr *config.Stage + +func asPtr(p *derivedStage) *config.Stage { return (*config.Stage)(p) } + +// The elided element of a []*config.Stage IS a construction, and types as *config.Stage — which is why +// the literal branch still derefs. This is the case that keeps the deref honest. +var ptrSlice = []*config.Stage{{Name: "judge"}} // want "engine-internal call must derive its stage" + +var _, _, _ = pendingPtr, asPtr, ptrSlice + +// --- documented misses: NOT controls. Each is a gap named in the package doc. They carry no +// expectation comment on purpose, so analysistest fails the moment one starts being reported — which +// is how a fixture pins a MISS: closing the gap becomes a red test, not a silent behaviour change. + +// A. STRUCT EMBEDDING. The wrapper's zero value contains a zero Stage with Reasoning empty — the +// D39.87 defect exactly — but no node here mentions config.Stage as a constructed type. +type embedder struct{ config.Stage } + +func gapEmbedding() string { + var w embedder + w.Name = "annotate" + return w.Name +} + +// B. make() THEN ASSIGN. The slice element is a zero Stage; the literal branch never sees a literal. +func gapMakeThenAssign() config.Stage { + ss := make([]config.Stage, 1) + ss[0].Name = "annotate" + return ss[0] +} + +// C. GENERIC ZERO VALUE. `var zero T` inside a generic body declares the type PARAMETER, not Stage; +// the instantiation that pins T to config.Stage is at the call site, where nothing is declared. +func zeroOf[T any]() T { + var zero T + return zero +} + +func gapGenericZero() config.Stage { return zeroOf[config.Stage]() } + +var _, _, _ = gapEmbedding, gapMakeThenAssign, gapGenericZero diff --git a/backend/internal/pipeline/content_routing_test.go b/backend/internal/pipeline/content_routing_test.go index 0ba469c8..fa9e1614 100644 --- a/backend/internal/pipeline/content_routing_test.go +++ b/backend/internal/pipeline/content_routing_test.go @@ -4,7 +4,6 @@ import ( "context" "os" "path/filepath" - "regexp" "strings" "testing" @@ -357,51 +356,11 @@ func TestRetiredAdultKeyForms(t *testing.T) { } } -// TestProviderEgressSeamIsSingle is the mechanical guard for the choke point the routing assert sits in: -// exactly ONE production call site may reach a provider, plus a NAMED allowlist for the live-gated -// conformance probe. A new egress path (a Ф2 judge, an annotator) must extend the assert, not slip past -// it — and this test, unlike a grep in a report, cannot be forgotten. -func TestProviderEgressSeamIsSingle(t *testing.T) { - const allowedTest = "live_conformance_test.go" // live-gated probe, calls the real catalog on purpose - // The MODULE root, not just internal/: an egress added under cmd/ would otherwise be invisible. - root := filepath.Join("..", "..") - // Built from parts so this file's own source cannot match the needle it looks for — the previous - // literal форм made the guard exempt itself by name. - needle := regexp.MustCompile(`\.` + "Complete" + `\(`) - var offenders []string - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.IsDir() || !strings.HasSuffix(path, ".go") { - return nil - } - // The adapters themselves are the implementation of Complete — they are the seam, not a bypass. - if strings.Contains(filepath.ToSlash(path), "/llm/") { - return nil - } - base := filepath.Base(path) - // stagerun.go is exempted by its PACKAGE PATH, not by basename, so a same-named file in another - // package cannot inherit the exemption. - if base == allowedTest || strings.HasSuffix(filepath.ToSlash(path), "internal/pipeline/stagerun.go") { - return nil - } - raw, rerr := os.ReadFile(path) - if rerr != nil { - return rerr - } - if needle.Match(raw) { - offenders = append(offenders, path) - } - return nil - }) - if err != nil { - t.Fatal(err) - } - if len(offenders) > 0 { - t.Fatalf("provider egress must stay in ONE place (stagerun.go runAttempt, behind clientFor's label assert); found other call sites: %v — route them through runAttempt or extend the assert deliberately", offenders) - } -} +// The single-egress invariant this file used to guard by scanning bytes for `.Complete(` now lives in +// internal/archguard (egressseam). It reports a call that carries the llm.LLMRequest → llm.LLMResponse +// pair whatever the callee is called, OR one spelled Complete against internal/llm — so it tells our +// client's method from any other type's method of that name, which the byte scan could not, and it no +// longer has to exempt its own source from matching its own needle. // TestClientForRefusesUnknownModel keeps the pre-existing loud behaviour of the client lookup intact: // the label assert is an ADDITION to it, not a replacement (an un-enumerated model is still an error, diff --git a/backend/internal/pipeline/synthetic_stage_seam_test.go b/backend/internal/pipeline/synthetic_stage_seam_test.go index c011963d..fe15cde3 100644 --- a/backend/internal/pipeline/synthetic_stage_seam_test.go +++ b/backend/internal/pipeline/synthetic_stage_seam_test.go @@ -1,25 +1,15 @@ package pipeline -// synthetic_stage_seam_test.go: the mechanical guards behind the effort knob (D39.87). +// synthetic_stage_seam_test.go: the BEHAVIOURAL half of the effort knob (D39.87). The mechanical half — +// "no config.Stage / pipeline.Request built outside its seam" — used to live here as two AST walks and +// now lives in internal/archguard, run by `go vet -vettool` from `make battery`. The walks were replaced, +// not dropped: every bypass that ever defeated them is an analysistest case there. // -// The defect the knob fixed was never "somebody forgot a key" — it was the SHAPE of the code: engine-internal -// calls each hand-built their own config.Stage, and a hand-built literal drops every field it does not name. -// Adding the key to the three shipping literals would have left the fourth site (and the fifth, tomorrow) -// exactly as broken, so the literals are gone. These tests are what keeps them gone. -// -// They walk the AST, not the bytes. A byte scan for `config.Stage{` was the first version and it was -// defeated five ways — `var s config.Stage` plus field assignment, `new(config.Stage)`, a type alias, a -// newline between `RequestHash(` and `Request{`, and the most natural rewrite of all, `req := Request{…}` -// followed by `RequestHash(req)`. It also failed the build on a doccomment that merely NAMED the type. A -// guard that a careless edit slips past is worse than none, because it reads as protection. +// What stays here is what an analyzer cannot answer: whether the seam, when it runs, actually carries the +// knob through to the money path. import ( - "go/ast" - "go/parser" - "go/token" - "os" "path/filepath" - "strings" "testing" "textmachine/backend/internal/chunk" @@ -28,213 +18,6 @@ import ( "textmachine/backend/internal/store" ) -// configPkgPath is the import path the guards resolve against. Matching the PATH, not the local package -// NAME, is load-bearing: `import cfg "…/internal/config"` followed by `cfg.Stage{…}` is a perfectly ordinary -// thing for an engineer to write, and a name-based predicate waves it through. That evasion was found by -// planting a realistic fifth call site — an annotator role — in shipping code, and the name-based version of -// this guard reported `ok`. -const configPkgPath = "textmachine/backend/internal/config" - -// walkedFileFloor is the smallest number of files a healthy walk parses. Without it, a walk that parses -// NOTHING — a bad root, a rename, a build-tag change — is indistinguishable from a clean tree, and the guard -// silently becomes decoration. That is the exact "reads as protection" failure this file's header warns -// about, so it is asserted rather than assumed. The tree holds ~200 .go files; the floor is deliberately far -// below that, because it exists to catch ZERO, not to track the file count. -const walkedFileFloor = 50 - -// fileImports maps a file's LOCAL package identifiers to the import paths they stand for. A dot-import is -// recorded under "." so a bare `Stage{…}` can be resolved too. -func fileImports(f *ast.File) map[string]string { - out := map[string]string{} - for _, im := range f.Imports { - path := strings.Trim(im.Path.Value, `"`) - name := path[strings.LastIndex(path, "/")+1:] // the default local name - if im.Name != nil { - name = im.Name.Name - } - out[name] = path - } - return out -} - -// walkGoFiles parses every .go file under the REPO root — not just internal/ — and hands each file's AST to -// visit. The reach past the Go module is deliberate and load-bearing: eval/ has no go.mod of its own yet -// imports this module's config package, and the ORIGINAL defect's fourth call site was exactly such a rig. -// Any go/types-based tool would be blind to it. -func walkGoFiles(t *testing.T, exempt func(slashPath string) bool, visit func(path string, fset *token.FileSet, f *ast.File, imports map[string]string)) { - t.Helper() - root := filepath.Join("..", "..", "..") // internal/pipeline -> backend -> repo root - parsed := 0 - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if info.IsDir() { - // Trees that hold no call sites of ours and cost seconds to walk. - switch info.Name() { - case ".git", "node_modules", "vendor", "dist", "backups": - return filepath.SkipDir - } - return nil - } - if !strings.HasSuffix(path, ".go") { - return nil - } - slash := filepath.ToSlash(path) - if exempt(slash) { - return nil - } - fset := token.NewFileSet() - f, perr := parser.ParseFile(fset, path, nil, 0) // no comments: a doccomment naming the type is not a call site - if perr != nil { - return nil // not part of this module's build (a testdata fixture, a scratch file) - } - parsed++ - visit(path, fset, f, fileImports(f)) - return nil - }) - if err != nil { - t.Fatal(err) - } - if parsed < walkedFileFloor { - t.Fatalf("the guard parsed only %d files (floor %d) — it is walking the wrong root or nothing at all, "+ - "which makes it decoration rather than a guard", parsed, walkedFileFloor) - } -} - -// namedType reports whether expr denotes pkgPath.name in a file with these imports, unwrapping the -// composite-literal wrappers `[]T`, `map[K]T` and `*T`. The unwrapping matters: `[]config.Stage{{…}}` ELIDES -// the element type on each element, so a literal-only check sees `&ast.CompositeLit{Type:nil}` and misses it -// — verified by planting exactly that and watching the guard report `ok`. -func namedType(expr ast.Expr, imports map[string]string, pkgPath, name string) bool { - switch t := expr.(type) { - case *ast.ArrayType: - return namedType(t.Elt, imports, pkgPath, name) - case *ast.MapType: - return namedType(t.Value, imports, pkgPath, name) - case *ast.StarExpr: - return namedType(t.X, imports, pkgPath, name) - case *ast.Ident: // a dot-imported type, or a package-local one - return t.Name == name && imports["."] == pkgPath - case *ast.SelectorExpr: - if t.Sel == nil || t.Sel.Name != name { - return false - } - id, ok := t.X.(*ast.Ident) - return ok && imports[id.Name] == pkgPath // by PATH, so an alias cannot slip through - } - return false -} - -// TestSyntheticStageSeamIsSingle: outside the config package there is exactly ONE way to obtain a Stage for -// an engine-internal call — config.InternalCall.Stage() — so a fifth call site inherits every decision the -// seam took instead of re-taking them by omission. This is the mechanical answer to the acceptance question -// "a FOURTH synthetic stage appears tomorrow: does it get the knob, or is it forgotten again?". -func TestSyntheticStageSeamIsSingle(t *testing.T) { - // Exemptions are FULL PATHS, never basenames: a basename exemption spreads to every file in the repo that - // happens to share the name, which is how `internal/anything/fewshot_test.go` would have inherited a pass. - exemptPaths := []string{ - "/backend/internal/pipeline/fewshot_test.go", // pure predicate test: no wire, no ledger, no checkpoint - "/backend/internal/pipeline/synthetic_stage_seam_test.go", // this guard builds a PARENT stage to test inheritance - } - var offenders []string - walkGoFiles(t, - func(slashPath string) bool { - for _, p := range exemptPaths { - if strings.HasSuffix(slashPath, p) { - return true - } - } - // The type's OWN package — the directory exactly, not a substring: `strings.Contains` would also - // exempt every SUBpackage (internal/config/annotator/) and any other tree ending in that path. - return strings.HasSuffix(filepath.ToSlash(filepath.Dir(slashPath)), "/backend/internal/config") - }, - func(path string, fset *token.FileSet, f *ast.File, imports map[string]string) { - isStage := func(e ast.Expr) bool { return namedType(e, imports, configPkgPath, "Stage") } - ast.Inspect(f, func(n ast.Node) bool { - var bad ast.Node - switch v := n.(type) { - case *ast.CompositeLit: // config.Stage{...}, []config.Stage{{...}}, map[string]config.Stage{...} - if v.Type != nil && isStage(v.Type) { - bad = v - } - case *ast.ValueSpec: // var s config.Stage (NOT unwrapped: `var ss []config.Stage` is a legitimate slice) - if v.Type != nil && namedTypeExact(v.Type, imports, configPkgPath, "Stage") { - bad = v - } - case *ast.CallExpr: // new(config.Stage) - if id, ok := v.Fun.(*ast.Ident); ok && id.Name == "new" && len(v.Args) == 1 && namedTypeExact(v.Args[0], imports, configPkgPath, "Stage") { - bad = v - } - case *ast.TypeSpec: // type myStage = config.Stage / type myStage config.Stage - if namedTypeExact(v.Type, imports, configPkgPath, "Stage") { - bad = v - } - } - if bad != nil { - offenders = append(offenders, fset.Position(bad.Pos()).String()) - } - return true - }) - }) - if len(offenders) > 0 { - t.Fatalf("an engine-internal call must derive its stage through config.InternalCall.Stage(), not by "+ - "building one (a hand-built stage silently drops reasoning/temperature — D39.87 §2); found: %v", offenders) - } -} - -// TestRequestIdentitySeamIsSingle: the tuple RequestHash addresses a checkpoint by is assembled in exactly -// ONE place (Runner.attemptRequest). Two assemblies of it agreeing is not a property anyone can maintain by -// eye — it held only while the omitted fields happened to be zero, and the effort knob makes them non-zero. -// -// The guard is "no Request composite literal outside attemptRequest", not "no RequestHash(Request{…})": -// the second form is one refactor away from invisible (hoist the literal into a variable), and it is the -// literal that is the hazard. -func TestRequestIdentitySeamIsSingle(t *testing.T) { - const seamFile = "stagerun.go" // the file that owns attemptRequest - // The tests OF the hashing contract legitimately build Requests — that is their subject. Named - // individually, so the exemption cannot spread to a file that merely happens to end in _test.go. - exemptPaths := []string{ - "/backend/internal/pipeline/" + seamFile, - "/backend/internal/pipeline/render.go", // where Request is DEFINED and hashed - // The tests OF the hashing contract legitimately build Requests — that is their subject. - "/backend/internal/pipeline/render_test.go", - "/backend/internal/pipeline/render_memory_test.go", - "/backend/internal/pipeline/synthetic_stage_seam_test.go", - } - var offenders []string - walkGoFiles(t, - func(slashPath string) bool { - for _, p := range exemptPaths { - if strings.HasSuffix(slashPath, p) { - return true - } - } - return false - }, - func(path string, fset *token.FileSet, f *ast.File, imports map[string]string) { - inPipeline := strings.HasSuffix(filepath.ToSlash(filepath.Dir(path)), "/backend/internal/pipeline") - ast.Inspect(f, func(n ast.Node) bool { - lit, ok := n.(*ast.CompositeLit) - if !ok || lit.Type == nil { - return true - } - hit := namedType(lit.Type, imports, pipelinePkgPath, "Request") - if !hit && inPipeline { // package-local `Request{…}` carries no qualifier to resolve - hit = namedTypeLocal(lit.Type, "Request") - } - if hit { - offenders = append(offenders, fset.Position(lit.Pos()).String()) - } - return true - }) - }) - if len(offenders) > 0 { - t.Fatalf("a call's request identity must come from Runner.attemptRequest, not a re-assembled Request "+ - "literal (a probe that drifts from the attempt turns the role sub-budget off — see attemptRequest); found: %v", offenders) - } -} - // TestBankProbeAndAttemptAddressOneCheckpoint is the BEHAVIOURAL half, and it runs the REAL functions: // bankCheckpointExists is asked about a checkpoint written under the hash the attempt path derives. If the // two ever address different tuples the probe answers "unpaid" for paid work, or — the expensive direction @@ -356,40 +139,3 @@ func TestRepairStageTakesItsEffortFromItsOwnGate(t *testing.T) { t.Fatalf("a repair span is re-parsed structure, not prose — temperature stays 0, got %v", rst.Temperature) } } - -// pipelinePkgPath is this package's own import path, for resolving `pipeline.Request{…}` written from -// outside it (eval/ harnesses do exactly this). -const pipelinePkgPath = "textmachine/backend/internal/pipeline" - -// namedTypeExact is namedType WITHOUT the []/map/* unwrapping. `var ss []config.Stage` is a legitimate -// slice variable, not a hand-built stage, so unwrapping there would false-flag ordinary code — it did, on -// waveStages' parameter list. -func namedTypeExact(expr ast.Expr, imports map[string]string, pkgPath, name string) bool { - switch t := expr.(type) { - case *ast.Ident: - return t.Name == name && imports["."] == pkgPath - case *ast.SelectorExpr: - if t.Sel == nil || t.Sel.Name != name { - return false - } - id, ok := t.X.(*ast.Ident) - return ok && imports[id.Name] == pkgPath - } - return false -} - -// namedTypeLocal matches an UNQUALIFIED type name, unwrapping composite-literal wrappers. Only meaningful -// for a file inside the package that declares the type. -func namedTypeLocal(expr ast.Expr, name string) bool { - switch t := expr.(type) { - case *ast.ArrayType: - return namedTypeLocal(t.Elt, name) - case *ast.MapType: - return namedTypeLocal(t.Value, name) - case *ast.StarExpr: - return namedTypeLocal(t.X, name) - case *ast.Ident: - return t.Name == name - } - return false -}