From 1f7fd476eced5733d3b7ac9f501e74e9d320ac24 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 4 Aug 2026 05:46:04 +0400 Subject: [PATCH] ipc: test mapErr, and make a new store sentinel a decision (V-408) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folded into #408 from the same review. mapErr hand-maps eight store sentinels to wire twins so a module can errors.Is without importing internal/store. The design is right; the failure mode is silent. Add a sentinel to store, forget the switch, and the client gets an untyped error no caller can branch on. Three tests. The pairs, asserted through a wrap because every real caller wraps. An unrecognised error, asserted to pass through untouched. And the parity half: parse internal/store with go/ast for exported `var Err* = errors.New(...)` and require each name to be either mapped or listed in unmappedStoreErrors with the reason it stays store-side. Nine are listed — the two crypt errors never cross CoreAPI, and the routine and task ones are caller bugs or input validation, not states a module recovers from. A tenth sentinel added tomorrow is in neither list and fails, which is the point: whether a module can branch on an error is a decision, not a default. Co-Authored-By: Claude Opus 5 --- internal/ipc/maperr_test.go | 148 ++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 internal/ipc/maperr_test.go diff --git a/internal/ipc/maperr_test.go b/internal/ipc/maperr_test.go new file mode 100644 index 0000000..9b75f80 --- /dev/null +++ b/internal/ipc/maperr_test.go @@ -0,0 +1,148 @@ +package ipc + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "strings" + "testing" + + "github.com/kami/maven/internal/store" +) + +// mapErr turns a store sentinel into its wire twin so a module can errors.Is +// without importing internal/store. The design is right and the failure mode is +// quiet: add a sentinel to store, forget the switch, and the client gets an +// untyped error that no caller can branch on. These two tests are the alarm. + +// mapErrPairs — every store sentinel that has a wire twin, and the twin. +var mapErrPairs = []struct { + name string // the store identifier, for the coverage test below + from error + wants error +}{ + {"ErrNoFact", store.ErrNoFact, ErrNoFact}, + {"ErrConfidence", store.ErrConfidence, ErrConfidence}, + {"ErrVoidsMissing", store.ErrVoidsMissing, ErrVoidsMissing}, + {"ErrNudgeNotFound", store.ErrNudgeNotFound, ErrNudgeNotFound}, + {"ErrNudgeOutcome", store.ErrNudgeOutcome, ErrNudgeOutcome}, + {"ErrReminderNotFound", store.ErrReminderNotFound, ErrReminderNotFound}, + {"ErrReminderState", store.ErrReminderState, ErrReminderState}, + {"ErrToolNotFound", store.ErrToolNotFound, ErrToolNotFound}, +} + +// unmappedStoreErrors — store sentinels that deliberately have no wire twin, +// each with the reason it stays store-side. A new sentinel is in neither list +// and fails TestMapErrCoversEveryStoreSentinel, which is the point: whether a +// module can branch on an error is a decision, not a default. +var unmappedStoreErrors = map[string]string{ + "ErrKeyLen": "unlock path — the key never crosses CoreAPI", + "ErrDecrypt": "unlock path — the key never crosses CoreAPI", + + "ErrToolCmd": "write-side validation of an allowlist mutation; the caller is the owner at a step-up, not a module branching on the verdict", + + "ErrProposedRoutineNotFound": "no module branches on a routine id that vanished; accept and dismiss are owner clicks", + "ErrProposedRoutineExists": "the propose path already reports 'nothing new' through its bool return", + "ErrRoutineStatus": "an unknown status is a caller bug, not a state a module recovers from", + + "ErrTaskNotFound": "the task surfaces re-list rather than branch", + "ErrTaskEmpty": "input validation — the surface refuses empty text before it gets here", + "ErrTaskStatus": "an illegal status move is a caller bug; the surface offers only legal ones", +} + +// The mapping itself, through a wrap, because every real caller wraps. +func TestMapErrMapsEveryPair(t *testing.T) { + for _, p := range mapErrPairs { + got := mapErr(fmt.Errorf("storeapi: %w", p.from)) + if got != p.wants { + t.Errorf("mapErr(store.%s) = %v, want %v", p.name, got, p.wants) + } + } +} + +// Anything mapErr does not recognise passes through untouched. A module that +// cannot branch on an error must still see the original text. +func TestMapErrPassesUnknownThrough(t *testing.T) { + if mapErr(nil) != nil { + t.Error("mapErr(nil) must stay nil") + } + own := fmt.Errorf("socket closed") + if got := mapErr(own); got != own { + t.Errorf("mapErr(%v) = %v, want the same error back", own, got) + } +} + +// The parity half: every exported sentinel in internal/store is either mapped +// or listed with a reason. Read off the source, so a sentinel added in a file +// this package never touches still trips it. +func TestMapErrCoversEveryStoreSentinel(t *testing.T) { + mapped := map[string]bool{} + for _, p := range mapErrPairs { + mapped[p.name] = true + } + + for _, name := range storeSentinelNames(t) { + if mapped[name] || unmappedStoreErrors[name] != "" { + continue + } + t.Errorf("store.%s is a new sentinel with no verdict: add it to mapErr and mapErrPairs, "+ + "or to unmappedStoreErrors with the reason a module cannot branch on it", name) + } +} + +// storeSentinelNames reads internal/store for exported package-level error +// values: `var ErrX = errors.New(...)`, inside a block or on its own. +func storeSentinelNames(t *testing.T) []string { + t.Helper() + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, "../store", func(fi fs.FileInfo) bool { + return !strings.HasSuffix(fi.Name(), "_test.go") + }, 0) + if err != nil { + t.Fatalf("parse internal/store: %v", err) + } + var out []string + for _, pkg := range pkgs { + for _, f := range pkg.Files { + for _, d := range f.Decls { + gd, ok := d.(*ast.GenDecl) + if !ok || gd.Tok != token.VAR { + continue + } + for _, spec := range gd.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for i, n := range vs.Names { + if !strings.HasPrefix(n.Name, "Err") || !n.IsExported() { + continue + } + if i < len(vs.Values) && isErrorsNew(vs.Values[i]) { + out = append(out, n.Name) + } + } + } + } + } + } + if len(out) < len(mapErrPairs) { + t.Fatalf("found %d sentinels in internal/store, fewer than the %d already mapped — the scan is broken, not the store", len(out), len(mapErrPairs)) + } + return out +} + +func isErrorsNew(e ast.Expr) bool { + call, ok := e.(*ast.CallExpr) + if !ok { + return false + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "New" { + return false + } + id, ok := sel.X.(*ast.Ident) + return ok && id.Name == "errors" +}