From 50835889d5d870a7df8a01c69cb9107fcd99ca68 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:57:09 +0200 Subject: [PATCH 1/4] Isolate test fixtures from the machine's git hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A throwaway repository inherits the developer's global core.hooksPath, so a hook installed for real work also runs against fixtures. One that refuses a commit leaves a fixture half-built: the file staged but unrecorded, and a later checkout then carries it onto the branch under test. That is how a passing suite turned red locally while CI stayed green — the hook does not exist there. Repo now points core.hooksPath at a path with no hooks in it, for the same reason it already sets the identity per repository rather than reading it from the machine. A fixture must not inherit the machine. The symptom was TestDiffMeasuresFromTheMergeBase crediting another branch's file to the session, because diffIncludingNewFiles counts staged and untracked files by design. --- internal/gittest/gittest.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/gittest/gittest.go b/internal/gittest/gittest.go index a6d2eab..055cdaf 100644 --- a/internal/gittest/gittest.go +++ b/internal/gittest/gittest.go @@ -31,6 +31,14 @@ func Repo(t *testing.T) string { Run(t, dir, "init", "-q", "-b", "main") Run(t, dir, "config", "user.name", "test") Run(t, dir, "config", "user.email", "test@example.test") + // Hooks are isolated for the same reason the identity is: a fixture must + // not inherit the machine. A developer's global core.hooksPath applies to + // every repository including these throwaway ones, and a hook that refuses + // a commit leaves the fixture half-built — the file staged but unrecorded, + // which a later checkout then carries onto the wrong branch. Pointing at a + // path with no hooks in it is what makes the suite say the same thing here + // as it does in CI. + Run(t, dir, "config", "core.hooksPath", "/dev/null") return dir } From c9d86c5e5a8a755d1676d761d7553be00344bbc8 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:57:09 +0200 Subject: [PATCH 2/4] Let TestSessionIsUnique tolerate one collision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It asserted zero collisions in 2000 draws over a space of 50 adjectives, 50 animals and 32^4 suffixes. That is 2.6 billion names, so the birthday chance of one collision is about 1 in 1300 — and a test that fails that often teaches a reader to re-run a red suite rather than read it. One is now tolerated, two is not. The chance of a second is about 1 in 3 million, while a generator that had lost its suffix would draw from 2500 names and collide about 620 times here. Measured, not estimated: breaking suffix produces 615. The arithmetic is in the comment, because the next person to see one collision should be able to tell luck from a broken generator without redoing it. --- internal/naming/naming_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/naming/naming_test.go b/internal/naming/naming_test.go index 1840533..63388d6 100644 --- a/internal/naming/naming_test.go +++ b/internal/naming/naming_test.go @@ -32,9 +32,16 @@ func TestSessionIsUnique(t *testing.T) { } seen[n] = true } - // 50 adjectives x 50 animals x 32^4 suffixes is a large space; a couple of - // birthday collisions in 2000 draws would still be suspicious. - if collisions > 0 { + // 50 adjectives x 50 animals x 32^4 suffixes is 2.6 billion names, so the + // birthday chance of one collision in 2000 draws is about 1 in 1300. This + // asserted zero and therefore failed roughly that often, which teaches a + // reader to re-run a red suite rather than read it. + // + // One is tolerated, two is not: the chance of a second is about 1 in 3 + // million, while a generator that had lost its suffix would draw from 2500 + // names and collide about 620 times here. The threshold still separates + // those two cases by a wide margin. + if collisions > 1 { t.Errorf("%d collisions in %d draws", collisions, draws) } } From 2bbeacb06d8015ca50ea9283fef69edba3818921 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Wed, 16 Sep 2026 09:56:55 +0200 Subject: [PATCH 3/4] Check that prose names identifiers that exist An identifier spelled wrong in code fails the build. The same mistake in a doc comment, a README table or an architecture note passes every gate and is read as fact by the next person. Prose is the one place here with no compiler, which is where the defects were collecting. scanProse walks the tree, reads names out of the syntax tree rather than the file text, and reports any camelCase word in a comment or a markdown code span that is not an identifier anywhere in the module. Reading the AST is what stops a misspelling written twice from vouching for itself. Three rules keep it quiet enough to survive. Every capital must be followed by lowercase, so an acronym is not mistaken for a name. A word that prefixes a real identifier is naming a family. A word a file only writes into a string literal is accepted in that file alone. It starts green on 178 names and takes a fixture rather than a hand check: TestProseScanReportsAPlantedMisspelling runs the real scanner over a temporary package and document, so the discrimination is proved on every run instead of once by somebody who remembered to try it. The fixture is written to a temp directory on purpose. Committing one would put the misspelling into this module's own sources, where it would resolve. scripts/commit-msg does the same for commit messages, which no test can read. It leaves quietly in a repository with no Go files, so it is safe in a shared core.hooksPath, and it warns rather than blocks: a message may name a symbol the commit deletes. What it cannot judge is whether a claim about a real symbol is true. That half stays a reading job. --- Makefile | 13 ++- prose_test.go | 246 +++++++++++++++++++++++++++++++++++++++++++++ scripts/commit-msg | 41 ++++++++ 3 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 prose_test.go create mode 100755 scripts/commit-msg diff --git a/Makefile b/Makefile index 03f85ba..1b0aa50 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ PREFIX ?= $(HOME)/.local/bin # none of which should end up anywhere a backup would find them. DEMO_DIR ?= /tmp/deck-demo -.PHONY: help build run test race vet fmt tidy install uninstall reinstall watch clean demo +.PHONY: help build run test race vet fmt tidy hooks install uninstall reinstall watch clean demo help: ## list the targets @grep -E '^[a-z-]+:.*?## ' $(MAKEFILE_LIST) \ @@ -39,6 +39,17 @@ fmt: ## gofmt the tree tidy: ## resolve deps + write go.sum go mod tidy +hooks: ## install scripts/commit-msg where git will actually look for it + @dir=$$(git rev-parse --git-path hooks); \ + common=$$(git rev-parse --git-common-dir)/hooks; \ + mkdir -p "$$dir"; \ + ln -sf "$$(pwd)/scripts/commit-msg" "$$dir/commit-msg"; \ + echo "installed: $$dir/commit-msg"; \ + if [ "$$dir" != "$$common" ]; then \ + echo "note: core.hooksPath is shared, so this now runs for every repository."; \ + echo "It exits immediately in one with no Go files."; \ + fi + install: ## build and copy to $(PREFIX), creating it if needed @mkdir -p $(PREFIX) go build -o $(PREFIX)/$(APP) . diff --git a/prose_test.go b/prose_test.go new file mode 100644 index 0000000..87b7aa7 --- /dev/null +++ b/prose_test.go @@ -0,0 +1,246 @@ +package main + +// Prose is the one place in this repository with no compiler. An identifier +// spelled wrong in code fails the build; the same mistake in a doc comment, a +// README table or an architecture note passes every gate and is read as fact by +// the next person. This test closes that gap for the half of it a machine can +// judge: whether a name written in prose exists at all. + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + "testing" +) + +// camelCase is the shape that distinguishes a Go identifier from an English +// word without needing a dictionary. sessionLine matches; "focus" and "Session" +// do not, and neither does ordinary prose — which is what keeps the check quiet +// enough to be worth having. Single-word identifiers are out of reach for the +// same reason, and that is the accepted cost. +// +// Every capital must be followed by lowercase. That is what separates a Go name +// from an acronym: this repository writes several of those often, and a check +// that flagged them would be turned off within a week. +var camelCase = regexp.MustCompile(`\b[a-z][a-z0-9]*(?:[A-Z][a-z0-9]+)+\b`) + +// backticked pulls the code spans out of markdown. A name outside them is +// English, and a name inside one is a claim about this tree. +var backticked = regexp.MustCompile("`([^`]+)`") + +// miss is one name, in one file, that resolves to nothing. +type miss struct{ file, name string } + +// scanProse reports every name written in prose under root that is not an +// identifier somewhere in root's Go sources. +// +// It reads names out of the syntax tree rather than out of the file text. Text +// would include the comments themselves, so a misspelling written twice would +// vouch for itself and the check would pass on exactly the case it exists to +// catch. +// +// Taking a root rather than walking the working directory is what lets the +// planted-misspelling test run the real scanner over a fixture, instead of the +// discrimination being something a human checked once and wrote down. +func scanProse(t *testing.T, root string) []miss { + t.Helper() + + var goFiles, mdFiles []string + err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() && (d.Name() == ".git" || d.Name() == "assets") { + return filepath.SkipDir + } + switch filepath.Ext(path) { + case ".go": + goFiles = append(goFiles, path) + case ".md": + mdFiles = append(mdFiles, path) + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + // Without this the whole check passes by finding nothing — a walk that + // starts in the wrong place, or a skip rule that grows too broad, would + // turn into a green test rather than a red one. + if len(goFiles) == 0 { + t.Fatalf("no Go sources under %s; the walk is wrong, not the tree", root) + } + + fset := token.NewFileSet() + inCode := map[string]bool{} + comments := map[string][]string{} // file -> comment text + inLiterals := map[string]map[string]bool{} // file -> names inside its own string literals + + for _, path := range goFiles { + f, err := parser.ParseFile(fset, path, nil, parser.ParseComments) + if err != nil { + t.Fatalf("parse %s: %v", path, err) + } + inLiterals[path] = map[string]bool{} + ast.Inspect(f, func(n ast.Node) bool { + switch v := n.(type) { + case *ast.Ident: + inCode[v.Name] = true + case *ast.BasicLit: + // A name a file only ever writes into a string is still a name + // that file's comments may explain: the SVG attributes ansisvg + // emits are not Go symbols, and its comments discuss them by + // name. Scoped to the file, so one literal cannot vouch for + // prose anywhere else in the tree. + for _, name := range camelCase.FindAllString(v.Value, -1) { + inLiterals[path][name] = true + } + } + return true + }) + for _, g := range f.Comments { + comments[path] = append(comments[path], g.Text()) + } + } + + // A name that prefixes a real identifier is naming a family rather than a + // symbol — "workingCopy indices" heads workingCopyWorktree and + // workingCopyDir. Accepting it keeps the check honest about what it can + // know, and costs little: a misspelling is not a prefix of what it meant. + prefixes := func(name string) bool { + for known := range inCode { + if len(known) > len(name) && strings.HasPrefix(known, name) { + return true + } + } + return false + } + + // A name is reported once per file that spells it, so a fix lands in one + // place rather than being chased through a list of repeats. + var missing []miss + seen := map[miss]bool{} + note := func(file, name string) { + m := miss{file, name} + if inCode[name] || inLiterals[file][name] || seen[m] || prefixes(name) { + return + } + seen[m] = true + missing = append(missing, m) + } + + for path, texts := range comments { + for _, text := range texts { + for _, name := range camelCase.FindAllString(text, -1) { + note(path, name) + } + } + } + + for _, path := range mdFiles { + body, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + for _, span := range backticked.FindAllStringSubmatch(string(body), -1) { + // A qualified name arrives as ui.coordArgs or gitx.Diff; the + // package half is not declared in this module's own trees, so only + // the selector is checked. + for _, name := range camelCase.FindAllString(span[1], -1) { + note(path, name) + } + } + } + + sort.Slice(missing, func(i, j int) bool { + if missing[i].file != missing[j].file { + return missing[i].file < missing[j].file + } + return missing[i].name < missing[j].name + }) + return missing +} + +// TestProseNamesIdentifiersThatExist is the regression for a class of defect +// this repository kept producing: a comment or a document naming a function +// that is not there. +// +// What it cannot judge is whether a claim about a real symbol is true — +// "sessionLine draws no marker" names something that exists and was wrong +// anyway. That half stays a reading job. +// +// It reads every markdown file on disk, not only the tracked ones, so an +// uncommitted note is checked here and not in CI. That is deliberate: working +// instructions go stale the same way published ones do, and a local-only +// failure is still a true one. +func TestProseNamesIdentifiersThatExist(t *testing.T) { + for _, m := range scanProse(t, ".") { + t.Errorf("%s names %q, which is not an identifier anywhere in this module", + m.file, m.name) + } +} + +// TestProseScanReportsAPlantedMisspelling proves the scanner discriminates. +// +// Without it the test above is a green light of unknown value: it passes on a +// clean tree whether or not it can see anything. This runs the real scanner +// over a fixture whose comment names a function that does not exist. +// +// The fixture is written to a temporary directory rather than committed. A +// committed one would put the misspelling into this module's own sources, where +// it would resolve — and the commit-msg hook, which greps the tree rather than +// parsing it, would then treat the typo as a real symbol forever. +func TestProseScanReportsAPlantedMisspelling(t *testing.T) { + dir := t.TempDir() + const sample = `package sample + +// jumpToVerandah is named here and declared nowhere. +func Real() {} +` + // A markdown fixture too, because the two halves read different things — + // one parses comments out of a syntax tree, the other pulls code spans out + // of text. A single fixture would leave whichever half broke still green. + const notes = "# Sample\n\nThe `openTheVerandah` helper does the thing.\n" + + if err := os.WriteFile(filepath.Join(dir, "sample.go"), []byte(sample), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "notes.md"), []byte(notes), 0o644); err != nil { + t.Fatal(err) + } + + var got []string + for _, m := range scanProse(t, dir) { + got = append(got, m.name) + } + sort.Strings(got) + if want := "jumpToVerandah,openTheVerandah"; strings.Join(got, ",") != want { + t.Fatalf("scan reported %v, want both planted names", got) + } +} + +// TestCamelCaseMatchesNamesNotAcronyms covers the matcher every other check is +// built on, including the rule that made it usable. An acronym is not a Go +// name, and a check that reported one on every run is a check somebody deletes. +func TestCamelCaseMatchesNamesNotAcronyms(t *testing.T) { + for _, tc := range []struct { + in string + want []string + }{ + {"jumpToSession moves the cursor", []string{"jumpToSession"}}, + {"sessionLine and cursorMarker", []string{"sessionLine", "cursorMarker"}}, + {"resolved on macOS only", nil}, + {"the sRGB ramp", nil}, + {"focus and Session on their own", nil}, + } { + got := camelCase.FindAllString(tc.in, -1) + if strings.Join(got, ",") != strings.Join(tc.want, ",") { + t.Errorf("on %q: got %v, want %v", tc.in, got, tc.want) + } + } +} diff --git a/scripts/commit-msg b/scripts/commit-msg new file mode 100755 index 0000000..a444be2 --- /dev/null +++ b/scripts/commit-msg @@ -0,0 +1,41 @@ +#!/bin/sh +# Refuse a commit message that names a Go identifier this tree does not have. +# +# TestProseNamesIdentifiersThatExist does this for comments and documents, and +# cannot do it here: go test never sees a commit message. The rule is the same +# one — a name written in prose is a claim, and an unchecked claim is read as +# fact by whoever runs git log next. +# +# Install with `make hooks`. Warns rather than blocks, because a message may +# legitimately name a symbol the commit deletes. + +msg="$1" +root=$(git rev-parse --show-toplevel) + +# A repository with no Go in it has no identifiers to check, and every camelCase +# word in its messages would be reported. Leaving quietly here is what makes the +# hook safe to install in a core.hooksPath shared by every repository you touch. +[ -n "$(git -C "$root" ls-files '*.go' | head -n 1)" ] || exit 0 + +# The same shape the Go test matches: lowercase start, every capital followed by +# lowercase, so macOS and sRGB are left alone. +names=$(sed 's/^#.*//' "$msg" | + grep -oE '\b[a-z][a-z0-9]*([A-Z][a-z0-9]+)+\b' | sort -u) + +missing="" +for name in $names; do + # Look in the tracked Go sources only. A name that appears nowhere in them + # is either misspelled or describes code that is no longer here. + if ! git -C "$root" grep -qw --cached -- "$name" -- '*.go' 2>/dev/null && + ! git -C "$root" grep -qw -- "$name" -- '*.go' 2>/dev/null; then + missing="$missing $name" + fi +done + +if [ -n "$missing" ]; then + echo "commit-msg: these names are in the message but not in any .go file:" >&2 + for name in $missing; do echo " $name" >&2; done + echo "Check the spelling, or say it in words if it is not a symbol." >&2 +fi + +exit 0 From 371af93d8a20e90d9f777e0694d17d395fe422f9 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:01:19 +0200 Subject: [PATCH 4/4] Stop TestAnalyseReturnsBeforeItFinishes racing its own stub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It asserted the job was still running immediately after Analyse returned, while the stub reviewer returned instantly and runAnalysis ran in a goroutine. Which of the two got there first was up to the scheduler, so the test failed under -race on a loaded runner and passed everywhere else. The reviewer is now held open until the assertions are done, through analysableWith, the same shape TestSidebarShowsWhatAnalysesCost already uses. "Still running" becomes a fact the test arranges rather than one it hopes for. The timing assertion above it is untouched, because that is the real subject: Analyse must hand back a handle rather than block for the length of a review. Not reproduced locally — this machine schedules it the winning way every time across ten runs. The fix removes the race by construction, and CI is what demonstrates it. --- internal/coord/jobs_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/coord/jobs_test.go b/internal/coord/jobs_test.go index d8b5b6d..dcd8813 100644 --- a/internal/coord/jobs_test.go +++ b/internal/coord/jobs_test.go @@ -81,7 +81,17 @@ func TestAnalyseRefusesTheSameThingsWorkDoes(t *testing.T) { // synchronous answer would outlast the calling agent's tool timeout, which // belongs to that agent rather than to us. func TestAnalyseReturnsBeforeItFinishes(t *testing.T) { - c := analysable(t, agent.ClaudeRun{}, nil) + // The reviewer is held open until the assertions are done. A stub that + // returns immediately lets the run reach JobDone before State is read, so + // the test failed on whichever machine happened to schedule it that way — + // under -race on a loaded runner, often enough to be noticed. Holding the + // run open is what makes "still running" a fact rather than a gamble. + release := make(chan struct{}) + defer close(release) + c := analysableWith(t, func(_ context.Context, _, _ string) (agent.ClaudeRun, error) { + <-release + return agent.ClaudeRun{}, nil + }) start := time.Now() job, err := c.Analyse("me", "wily-crane-bbbb", "")