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/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", "") 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 } 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) } } 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