review

review patchsets using your default editor
Log | Files | Refs

commit e38d4e87fba9f4f8c891268f89534f0f55a0d8c8
parent 809f6fdda4d0de8f71215e1bd331620d5018dee2
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date:   Mon, 21 Sep 2026 11:14:06 -0300

review: mechanistically drive code quality

Via a combination of static analysis and cheap inference, provide
a rigid, quality forcing mechanism for AI agents to iterate against.

Diffstat:
Acache.go | 140+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Acache_test.go | 151++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Achain.go | 67+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Achain_test.go | 98+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mclient.go | 555+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
Mcriteria/claims.md | 6+++++-
Mcriteria/duplication.md | 40+++++++++++++++++++++++++++++++++++++++-
Mcriteria/namer.md | 16++++++++++++++--
Mcriteria/tests.md | 8++++++++
Aeval_test.go | 383+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mfinding.go | 21+++++++++++++++++++++
Afinding_test.go | 224+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Afrontend.go | 166+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Afrontend_test.go | 128+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Agaming.go | 257+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Agaming_test.go | 244+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Agofrontend.go | 197+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Ahelper_test.go | 66++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aheuristic.go | 116+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mindex.go | 107+++++++++++++++++++------------------------------------------------------------
Aindex_test.go | 178+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mjob.go | 25+++++++++++++++++++++++++
Ajob_test.go | 397+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mmain.go | 266+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
Amain_test.go | 323+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodinfrontend.go | 180+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aodinfrontend_test.go | 143+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mpacket.go | 409+++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------
Apacket_test.go | 399+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aprovider.go | 284+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Aprovider_test.go | 363+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Mreadme.md | 208++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
Asidecar/odin/.gitignore | 1+
Asidecar/odin/main.odin | 216+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Astatic.go | 635+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Astatic_test.go | 597+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Astaticcheck.go | 215+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Astaticcheck_test.go | 141+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Atsfrontend.go | 347+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Atsfrontend_test.go | 190+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Averify_test.go | 179+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
41 files changed, 8367 insertions(+), 319 deletions(-)

diff --git a/cache.go b/cache.go @@ -0,0 +1,140 @@ +package main + +// The cache remembers what a provider answered, so that a re-run of a +// review whose parts did not change does not ask again. A loop iterates by +// re-running: one fix, one more reading. Without the cache every re-run +// re-asks every job, at full price and with a fresh roll of the dice — the +// same question answered two ways is the flicker that makes a loop chase +// ghosts. With it, a job whose rendered subject is byte for byte the same +// replays the recorded answer, which is both free and stable. + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "os" + "path/filepath" + "sort" + "strings" + "sync" + "time" +) + +// cacheVersion is part of every key, so that a change to what an ask holds +// invalidates the recorded answers instead of replaying stale ones. +const cacheVersion = "review-answers-v1" + +// cacheBound is the entry count past which the oldest answers are dropped. +const cacheBound = 4000 + +// AnswerCache is one file of recorded answers, keyed by the exact question. +type AnswerCache struct { + mu sync.Mutex + path string + fresh bool + entries map[string]cacheEntry + dirty bool + hits int +} + +// cacheEntry is one recorded answer and what it cost when it was asked. +type cacheEntry struct { + Text string `json:"text"` + In int `json:"in"` + Out int `json:"out"` + Cached int `json:"cached"` + Cost float64 `json:"cost"` + At time.Time `json:"at"` +} + +// openCache reads the answer file from the user's cache directory. A cache +// that cannot be read or written is no fault of the review: the run asks +// the provider as it would have. Fresh skips the reads but keeps the +// writes, so --fresh re-asks everything and leaves the answers behind it. +func openCache(fresh bool) *AnswerCache { + dir, err := os.UserCacheDir() + if err != nil { + return nil + } + c := &AnswerCache{ + path: filepath.Join(dir, "review", "answers.json"), + fresh: fresh, + entries: map[string]cacheEntry{}, + } + data, err := os.ReadFile(c.path) + if err != nil { + return c // No cache yet, which is not an error. + } + // A corrupt file is answered with whatever parsed. The rest is + // discarded rather than repaired; the next save rewrites the file. + json.Unmarshal(data, &c.entries) + return c +} + +func (c *AnswerCache) key(provider, system, user string) string { + h := sha256.Sum256([]byte(strings.Join([]string{cacheVersion, provider, system, user}, "\x00"))) + return hex.EncodeToString(h[:]) +} + +// get returns the answer recorded for exactly this question, marked as a +// replay: the run asked nothing, so the replay's usage counts stay at zero. +func (c *AnswerCache) get(provider, system, user string) (Answer, bool) { + c.mu.Lock() + defer c.mu.Unlock() + if c.fresh { + return Answer{}, false + } + entry, ok := c.entries[c.key(provider, system, user)] + if !ok { + return Answer{}, false + } + c.hits++ + return Answer{Text: entry.Text, Replayed: true}, true +} + +func (c *AnswerCache) put(provider, system, user string, answer Answer) { + c.mu.Lock() + defer c.mu.Unlock() + c.entries[c.key(provider, system, user)] = cacheEntry{ + Text: answer.Text, In: answer.In, Out: answer.Out, + Cached: answer.Cached, Cost: answer.Cost, At: time.Now(), + } + c.dirty = true +} + +// save writes the file back when this run recorded anything, dropping the +// oldest entries past the bound. A failed write stays silent: the cache is +// a saving, not a result. +func (c *AnswerCache) save() { + c.mu.Lock() + defer c.mu.Unlock() + if !c.dirty { + return + } + if len(c.entries) > cacheBound { + type aged struct { + key string + at time.Time + } + ages := make([]aged, 0, len(c.entries)) + for key, entry := range c.entries { + ages = append(ages, aged{key, entry.At}) + } + sort.Slice(ages, func(i, j int) bool { return ages[i].at.Before(ages[j].at) }) + for _, a := range ages[:len(ages)-cacheBound] { + delete(c.entries, a.key) + } + } + data, err := json.Marshal(c.entries) + if err != nil { + return + } + if err := os.MkdirAll(filepath.Dir(c.path), 0o755); err != nil { + return + } + temp := c.path + ".tmp" + if err := os.WriteFile(temp, data, 0o644); err != nil { + return + } + os.Rename(temp, c.path) +} diff --git a/cache_test.go b/cache_test.go @@ -0,0 +1,151 @@ +package main + +// The answer cache is what makes a re-run cheap and stable where the part +// of the change being judged did not change. These tests hold the contract: +// a recorded question replays and counts no usage, an unrecorded one asks, +// a fresh cache asks but keeps writing, a corrupt file is survived, and the +// file is pruned rather than grown without bound. + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +func testCache(t *testing.T) *AnswerCache { + t.Helper() + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + cache := openCache(false) + if cache == nil { + t.Fatal("no cache") + } + return cache +} + +func TestCacheReplaysAndRecords(t *testing.T) { + cache := testCache(t) + // Nothing is recorded yet. + if _, ok := cache.get("api/probe", "system", "user"); ok { + t.Error("an unrecorded question was replayed") + } + cache.put("api/probe", "system", "user", Answer{Text: "answer", In: 10, Out: 2, Cost: 0.5}) + // A replay is the same text, marked as a replay, and counts no usage: + // it cost nothing this run. + answer, ok := cache.get("api/probe", "system", "user") + if !ok || answer.Text != "answer" || !answer.Replayed || answer.In != 0 || answer.Cost != 0 { + t.Errorf("got %+v", answer) + } + // A different question is not a replay. + for _, key := range [][3]string{ + {"api/probe", "system", "other user"}, + {"api/probe", "other system", "user"}, + {"api/other", "system", "user"}, + } { + if _, ok := cache.get(key[0], key[1], key[2]); ok { + t.Errorf("replayed %v", key) + } + } + // What was recorded survives the run. + cache.save() + again := openCache(false) + if answer, ok := again.get("api/probe", "system", "user"); !ok || answer.Text != "answer" { + t.Errorf("got %+v", answer) + } +} + +// Fresh is for when the answer itself is in doubt: it asks again, and +// leaves what it learned behind it. +func TestCacheFreshAsksAndKeepsWriting(t *testing.T) { + cache := testCache(t) + cache.put("api/probe", "system", "user", Answer{Text: "stale"}) + fresh := openCache(true) + if _, ok := fresh.get("api/probe", "system", "user"); ok { + t.Error("fresh replayed a recorded answer") + } + fresh.put("api/probe", "system", "other", Answer{Text: "new"}) + fresh.save() + // The write is on disk, so it is read back from a new cache. + after := openCache(false) + if answer, ok := after.get("api/probe", "system", "other"); !ok || answer.Text != "new" { + t.Errorf("got %+v", answer) + } +} + +func TestCacheToleratesCorruption(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CACHE_HOME", dir) + if err := os.MkdirAll(filepath.Join(dir, "review"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "review", "answers.json"), []byte("{not json"), 0o644); err != nil { + t.Fatal(err) + } + cache := openCache(false) + cache.put("api/probe", "system", "user", Answer{Text: "kept"}) + cache.save() + data, err := os.ReadFile(filepath.Join(dir, "review", "answers.json")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "kept") { + t.Errorf("got %s", data) + } +} + +func TestCachePrunesTheOldest(t *testing.T) { + cache := testCache(t) + for i := range cacheBound + 10 { + cache.put(string(rune('a'+i%26))+itoa(i), "system", "user", Answer{Text: "x"}) + } + cache.save() + again := openCache(false) + if len(again.entries) != cacheBound { + t.Errorf("kept %d, want %d", len(again.entries), cacheBound) + } +} + +// A reviewer with a cache asks once and replays from then on, and the +// replay carries the findings the first ask produced. +func TestReviewerReplaysWhatItAsked(t *testing.T) { + provider := &relenting{answers: []string{findingsJSON("cannot-fail")}} + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + cache := openCache(false) + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + first := Reviewer{Provider: provider, Cache: cache}.Run(context.Background(), verifyChange(), jobs) + if len(first.Findings) != 1 || first.Replayed != 0 { + t.Fatalf("got %+v", first) + } + asked := len(provider.asked) + second := Reviewer{Provider: provider, Cache: cache}.Run(context.Background(), verifyChange(), jobs) + if len(provider.asked) != asked { + t.Errorf("asked %d times on the second run", len(provider.asked)-asked) + } + if second.Replayed != 1 { + t.Errorf("got %+v", second) + } + if len(second.Findings) != 1 || second.Findings[0].Rule != "cannot-fail" { + t.Fatalf("got %+v", second.Findings) + } + // A fresh cache asks again even where the answer is recorded. + fresh := openCache(true) + third := Reviewer{Provider: provider, Cache: fresh}.Run(context.Background(), verifyChange(), jobs) + if third.Replayed != 0 || len(provider.asked) != asked+1 { + t.Errorf("fresh replayed: %+v", third) + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + var digits []byte + for ; n > 0; n /= 10 { + digits = append([]byte{byte('0' + n%10)}, digits...) + } + return string(digits) +} diff --git a/chain.go b/chain.go @@ -0,0 +1,67 @@ +package main + +import ( + "context" + "fmt" + "os" + "strings" + "time" +) + +// probeTimeout bounds one reachability ask. A provider that cannot answer a +// trivial ask that quickly is unlikely to answer a job in time either, and +// the jobs are the expensive part. +const probeTimeout = 45 * time.Second + +// Chain is a priority order of providers. The first one that answers a probe +// serves the whole reading, so a provider over its limit costs one cheap ask +// rather than a failed review. Which provider is reachable is a property of +// the hour: a team limit exhausted, a gateway asleep, a key unset — the chain +// is how the tool rides that out without being told. +type Chain []entry + +type entry struct { + Name string + Provider Provider +} + +// defaultChain is the configured preference. The direct API is tried before +// the coding assistant, because it can enforce the answer's shape and reports +// usage; the local gateway is last, as the reading of last resort. The model +// flag re-points the first slot for a one-off, as it always has. +func defaultChain() Chain { + return Chain{ + {"api/" + orElse(os.Getenv("REVIEW_MODEL"), "claude-sonnet-5"), API{Model: orElse(os.Getenv("REVIEW_MODEL"), "claude-sonnet-5")}}, + {"pi/maple/glm-5-3-flash", Pi{Model: "glm-5-3-flash", Upstream: "maple"}}, + } +} + +// pick asks every entry a trivial question, in order, and returns the first +// that answers. Every failure is told to stderr, so a review run on the +// second choice says why the first was passed over. A reason is one line: +// a credential error walks the whole SDK ladder before it reaches the user, +// and the ladder is not what they asked about. +func (c Chain) pick(ctx context.Context, warn func(string)) (Provider, string, error) { + first := func(err error) string { + s := err.Error() + if i := strings.IndexAny(s, "\r\n"); i >= 0 { + s = s[:i] + } + return s + } + var reasons []string + for _, e := range c { + pctx, cancel := context.WithTimeout(ctx, probeTimeout) + answer, err := e.Provider.Ask(pctx, "Answer the user's message.", "Answer with the single word OK.") + cancel() + if err == nil && strings.TrimSpace(answer.Text) != "" { + return e.Provider, e.Name, nil + } + if err == nil { + err = fmt.Errorf("answered nothing") + } + warn(fmt.Sprintf("skipping %s: %s", e.Name, first(err))) + reasons = append(reasons, fmt.Sprintf("%s: %s", e.Name, first(err))) + } + return nil, "", fmt.Errorf("no provider answered: %s", strings.Join(reasons, "; ")) +} diff --git a/chain_test.go b/chain_test.go @@ -0,0 +1,98 @@ +package main + +import ( + "context" + "fmt" + "strings" + "testing" +) + +// stub answers from a script: each ask consumes the next outcome, and the +// count records how far a chain walked. +type stub struct { + outcomes []string // one per ask: "" means error, "ok" means text + asks int + label string +} + +func (s *stub) Name() string { return s.label } + +func (s *stub) Ask(ctx context.Context, system, user string) (Answer, error) { + s.asks++ + outcome := "error" + if s.asks <= len(s.outcomes) { + outcome = s.outcomes[s.asks-1] + } + switch outcome { + case "ok": + return Answer{Text: "OK"}, nil + case "empty": + return Answer{}, nil + default: + return Answer{}, fmt.Errorf("429: too many requests") + } +} + +func chainOf(entries ...*stub) Chain { + out := Chain{} + for _, p := range entries { + out = append(out, entry{Name: p.label, Provider: p}) + } + return out +} + +func TestChainPicksTheFirstThatAnswers(t *testing.T) { + first, second := &stub{label: "first", outcomes: []string{"ok"}}, &stub{label: "second", outcomes: []string{"ok"}} + provider, name, err := chainOf(first, second).pick(context.Background(), func(string) {}) + if err != nil { + t.Fatalf("pick: %v", err) + } + if name != "first" || provider != Provider(first) { + t.Fatalf("picked %q, want the first entry", name) + } + if first.asks != 1 || second.asks != 0 { + t.Fatalf("asks: first %d, second %d, want 1 and 0", first.asks, second.asks) + } +} + +func TestChainSkipsAFailingProvider(t *testing.T) { + first, second := &stub{label: "first", outcomes: []string{"error"}}, &stub{label: "second", outcomes: []string{"ok"}} + var warned []string + provider, name, err := chainOf(first, second).pick(context.Background(), func(s string) { warned = append(warned, s) }) + if err != nil || name != "second" || provider != Provider(second) { + t.Fatalf("pick: %v, %q", err, name) + } + if first.asks != 1 || second.asks != 1 { + t.Fatalf("asks: first %d, second %d", first.asks, second.asks) + } + if len(warned) != 1 || !strings.Contains(warned[0], "too many requests") { + t.Fatalf("warned %q, want the failure reason", warned) + } +} + +func TestChainSkipsAnEmptyAnswer(t *testing.T) { + first, second := &stub{label: "first", outcomes: []string{"empty"}}, &stub{label: "second", outcomes: []string{"ok"}} + provider, name, err := chainOf(first, second).pick(context.Background(), func(string) {}) + if err != nil { + t.Fatalf("pick: %v", err) + } + if name != "second" || provider != Provider(second) { + t.Fatalf("picked %q, want the second entry", name) + } + if first.asks != 1 || second.asks != 1 { + t.Fatalf("asks: first %d, second %d", first.asks, second.asks) + } +} + +func TestChainReportsEveryReasonWhenAllFail(t *testing.T) { + first, second := &stub{label: "first", outcomes: []string{"error"}}, &stub{label: "second", outcomes: []string{"empty"}} + _, _, err := chainOf(first, second).pick(context.Background(), func(string) {}) + if err == nil { + t.Fatal("pick: want an error naming every entry") + } + for _, want := range []string{"first", "second", "too many requests", "answered nothing"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("pick: %v, want it to name %q", err, want) + } + } +} diff --git a/client.go b/client.go @@ -2,22 +2,20 @@ package main import ( "context" + "encoding/json" "fmt" + "os" "regexp" "strings" "sync" + "time" "github.com/anthropics/anthropic-sdk-go" ) -// model is the one these jobs run on. Each is a narrow reading of a small -// packet, which is the cheapest tier's work, and it is the only tier where -// sampling can still be pinned. -const model = "claude-haiku-4-5" - // instruction is what every job is told, before its own criteria. It is kept -// identical across jobs so that the only thing that differs between them is -// the criteria and the subject. +// identical across jobs and providers, so the only thing that differs between +// two readings is the criteria and the subject. const instruction = `You are reviewing one narrow aspect of a change to a repository. Report only what the criteria below cover. Everything else is another reader's @@ -25,14 +23,49 @@ job: say nothing about formatting, style, performance, or correctness unless a criterion names it. Rules for reporting: -- Every finding cites one rule id from the criteria, exactly as written. -- Severity is must-fix when the criterion is plainly broken, consider when it is - a judgement call, note otherwise. +- Every finding cites one rule id from the criteria, exactly as written. A + finding citing anything else is discarded. +- Severity is must-fix when the criterion is plainly broken, consider when it + is a judgement call, note otherwise. - A finding names the file and line it concerns where it has one. - The fix is the concrete change to make, not a restatement of the problem. - Reporting nothing is the right answer when the criteria are met. Do not manufacture findings to appear useful. -- You are shown only part of the change. Never infer what the rest contains.` +- You are shown only part of the change. Never infer what the rest contains. + +Answer with one JSON object and nothing else. No preamble, no explanation, no +code fence: + +{"findings":[{"rule":"","severity":"must-fix|consider|note","file":"","line":0,"symbol":"","message":"","fix":""}]} + +Report no findings as {"findings":[]}.` + +// verifyInstruction is what the second reading is told. It sees what the +// first one saw — the same criteria, the same part of the change — and +// nothing the first one concluded beyond the findings themselves, so that a +// verdict is a reading of the evidence rather than an agreement with a +// colleague. +const verifyInstruction = `You are checking findings another reviewer made against one narrow aspect of a change. + +You are shown the criteria that reader judged against, the same part of the +change it read, and the findings it reported. For each finding, decide +whether it holds: the code it points at must meet the fault its rule +describes, judged from the evidence in front of you. + +- Judge every finding, by its number. Report no verdicts but those. +- holds is false for a finding you would not report yourself from this + evidence; say why in reason, in one sentence. +- A finding that holds but overstates its case does not hold as written. + +Answer with one JSON object and nothing else. No preamble, no explanation, no +code fence: + +{"verdicts":[{"index":0,"holds":true,"reason":""}]}` + +// verdictsAgain is what the second reading is told when it answered in +// prose, in the shape of the retry the findings get. +const verdictsAgain = `Your previous answer was not a JSON object, so nothing was read from it. +Answer again, with one verdict per finding, as one JSON object and nothing else.` // rule matches a rule id where the criteria define one, which is the list a // job is allowed to cite. @@ -47,8 +80,383 @@ func rules(criteria string) map[string]bool { return out } -// schema is what a job must answer in. It is enforced rather than requested: -// strict validation means a malformed answer cannot arrive. +// Retracted is a finding the verification pass did not let stand, and the +// reason. It is reported rather than dropped, so that a reading that retracts +// half of what it found says so instead of looking tidy. +type Retracted struct { + Finding Finding `json:"finding"` + Reason string `json:"reason"` +} + +// RunResult is what a set of readings produced. The parts beyond the +// findings are what a program reading the JSON needs to trust an empty +// list: which jobs failed, which had nothing to read, and what the asking +// cost. An empty findings list against a run where every job failed is not +// a pass, and the result says which. +type RunResult struct { + Findings []Finding + Failures []error + Retracted []Retracted + Skipped []string + In int + Out int + Cached int + Replayed int + Cost float64 +} + +// Reviewer runs jobs against a change, through whichever provider answers. +// Verify puts each job's findings back to the provider once, against the +// same evidence; the ones a second reading does not let stand are retracted +// rather than reported. Cache records what each ask cost and answered, so +// that a re-run of an unchanged part of the change replays instead of +// asking. +type Reviewer struct { + Provider Provider + Verify bool + Verbose bool + Cache *AnswerCache +} + +// Run works every job that has something to read, at once. One job failing +// does not stop the others: four readings out of five is worth more than none. +// REVIEW_SERIAL asks them one at a time instead, for providers that cannot +// take concurrent reads — the local maple gateway, which was observed to +// answer the biggest ask empty four runs in a row under parallelism and to +// answer the same ask fine on its own. +func (r Reviewer) Run(ctx context.Context, change *Change, jobs []Job) RunResult { + var ( + mu sync.Mutex + result RunResult + ) + record := func(job Job, findings []Finding, answer Answer, err error) { + mu.Lock() + defer mu.Unlock() + result.In += answer.In + result.Out += answer.Out + result.Cached += answer.Cached + result.Cost += answer.Cost + if answer.Replayed { + result.Replayed++ + } + if err != nil { + result.Failures = append(result.Failures, fmt.Errorf("%s: %w", job.Name, err)) + return + } + result.Findings = append(result.Findings, findings...) + } + read := func(job Job) { + subject := job.Subject(change) + if strings.TrimSpace(subject) == "" { + mu.Lock() + result.Skipped = append(result.Skipped, job.Name) + mu.Unlock() + if r.Verbose { + fmt.Printf(" %-12s nothing to read\n", job.Name) + } + return + } + askCtx, cancel := askContext(ctx) + findings, answer, err := r.ask(askCtx, job, subject) + cancel() + record(job, findings, answer, err) + } + if os.Getenv("REVIEW_SERIAL") != "" { + for _, job := range jobs { + read(job) + } + } else { + var wg sync.WaitGroup + for _, job := range jobs { + wg.Add(1) + go func(job Job) { + defer wg.Done() + read(job) + }(job) + } + wg.Wait() + } + if r.Verify { + r.verify(ctx, change, jobs, &result) + } + return result +} + +// verifyGroup is one job's findings, for the pass that checks them. +type verifyGroup struct { + job Job + indexes []int +} + +// verify puts each job's findings back to the provider once, against the +// same evidence the first reading had, and keeps only the ones it also +// reports. A reading that misread the shortlist — the measured way a small +// model reports the first duplicate it finds — answers differently when the +// question is whether each finding holds. A verdict that cannot be asked +// fails open: the findings stand, marked unverified, and the failure joins +// the others. +func (r Reviewer) verify(ctx context.Context, change *Change, jobs []Job, result *RunResult) { + var groups []verifyGroup + for _, job := range jobs { + var indexes []int + for i, f := range result.Findings { + if f.Job == job.Name { + indexes = append(indexes, i) + } + } + if len(indexes) > 0 { + groups = append(groups, verifyGroup{job: job, indexes: indexes}) + } + } + if len(groups) == 0 { + return + } + var ( + mu sync.Mutex + wg sync.WaitGroup + drop = map[int]bool{} + ) + apply := func(group verifyGroup) { + subject := group.job.Subject(change) + askCtx, cancel := askContext(ctx) + held, answer, err := r.verdicts(askCtx, group.job, subject, result.Findings, group.indexes) + cancel() + mu.Lock() + defer mu.Unlock() + result.In += answer.In + result.Out += answer.Out + result.Cached += answer.Cached + result.Cost += answer.Cost + if answer.Replayed { + result.Replayed++ + } + if err != nil { + result.Failures = append(result.Failures, fmt.Errorf("verify/%s: %w", group.job.Name, err)) + for _, i := range group.indexes { + result.Findings[i].Verified = false + } + return + } + for position, i := range group.indexes { + if v, judged := held[position]; judged && !v.Holds { + result.Retracted = append(result.Retracted, Retracted{ + Finding: result.Findings[i], + Reason: v.Reason, + }) + drop[i] = true + continue + } + // A finding the verdict list omits stands rather than + // falls: a strict pass would let one dropped number + // retract everything the reading found. + result.Findings[i].Verified = true + } + } + defer func() { + // A retracted finding is out of the findings: it is reported as a + // retraction, with its reason, not twice over. + if len(drop) == 0 { + return + } + survivors := make([]Finding, 0, len(result.Findings)) + for i, f := range result.Findings { + if !drop[i] { + survivors = append(survivors, f) + } + } + result.Findings = survivors + }() + if os.Getenv("REVIEW_SERIAL") != "" { + for _, group := range groups { + apply(group) + } + return + } + for _, group := range groups { + wg.Add(1) + go func(group verifyGroup) { + defer wg.Done() + apply(group) + }(group) + } + wg.Wait() +} + +// again is what a job is told when it answered in prose. Only the api +// provider can be held to a schema; the rest describe their findings instead +// often enough that losing the whole reading to it is the worse outcome. +const again = `Your previous answer was not a JSON object, so nothing was read from it. +Answer again, with the findings you already made, as one JSON object and nothing else.` + +// askTimeout bounds one job's ask. The probe that picks a provider is +// bounded, and a job should be too: a gateway that accepts a reachability +// ask and then stalls on a real one must not hold the review forever. A +// timed-out job is a failed job, so the rest of the readings and the +// deterministic checks still report. Measured serial readings through the +// maple gateway run about 80 seconds at the largest; this is four times +// that, and REVIEW_ASK_TIMEOUT can widen it. +const askTimeout = 5 * time.Minute + +func askTimeoutDuration() time.Duration { + if s := os.Getenv("REVIEW_ASK_TIMEOUT"); s != "" { + if d, err := time.ParseDuration(s); err == nil && d > 0 { + return d + } + } + return askTimeout +} + +// askContext is one ask's deadline, counted from the ask's start. +func askContext(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(ctx, askTimeoutDuration()) +} + +// ask puts one job's question. The criteria are the system prompt, where a +// provider that caches anything will cache them; the subject goes last. The +// retry ask below gets its own deadline: a second chance is not bound by +// the first's remaining time. +func (r Reviewer) ask(ctx context.Context, job Job, subject string) ([]Finding, Answer, error) { + system := instruction + "\n\n" + job.Criteria + answer, err := r.answer(ctx, system, subject, r.Provider.Ask) + if err != nil { + return nil, Answer{}, err + } + r.spent(job, answer) + raw, err := object(answer.Text) + if err != nil { + prose := first(answer.Text, 200) + answer, err = r.answer(ctx, system, subject+"\n\n"+again, r.Provider.Ask) + if err != nil { + return nil, Answer{}, err + } + r.spent(job, answer) + if raw, err = object(answer.Text); err != nil { + return nil, answer, fmt.Errorf("%w, twice: %s", err, prose) + } + } + reported, err := decode(raw) + if err != nil { + return nil, answer, err + } + return reported.findings(job.Name, rules(job.Criteria)), answer, nil +} + +// answer asks through the cache: the same question asked of the same +// provider is replayed rather than asked, which is what makes a re-run +// cheap where nothing it judges changed. +func (r Reviewer) answer(ctx context.Context, system, user string, ask func(context.Context, string, string) (Answer, error)) (Answer, error) { + if r.Cache != nil { + if answer, ok := r.Cache.get(r.Provider.Name(), system, user); ok { + if r.Verbose { + fmt.Printf(" %-12s replayed\n", r.Provider.Name()) + } + return answer, nil + } + } + answer, err := ask(ctx, system, user) + if err == nil && r.Cache != nil { + r.Cache.put(r.Provider.Name(), system, user, answer) + } + return answer, err +} + +func (r Reviewer) spent(job Job, answer Answer) { + if !r.Verbose { + return + } + if answer.Replayed { + fmt.Printf(" %-12s replayed\n", job.Name) + return + } + fmt.Printf(" %-12s %d in, %d out, %d cached, $%.4f\n", + job.Name, answer.In, answer.Out, answer.Cached, answer.Cost) +} + +// verdict is what the second reading says about one finding. +type verdict struct { + Holds bool + Reason string +} + +// verdicts puts one job's findings back to the provider, against the same +// evidence the first reading had. Verdicts are keyed by position in the +// listing; a position the answer omits stands rather than falls. +func (r Reviewer) verdicts(ctx context.Context, job Job, subject string, findings []Finding, indexes []int) (map[int]verdict, Answer, error) { + system := verifyInstruction + "\n\n" + job.Criteria + var listed strings.Builder + for position, i := range indexes { + f := findings[i] + where := "" + if f.File != "" { + where = fmt.Sprintf(" at %s:%d", f.File, f.Line) + } + fmt.Fprintf(&listed, "%d. [%s] %s%s: %s\n fix: %s\n", + position, f.Rule, f.Severity, where, f.Message, f.Fix) + } + user := subject + "\n\nThe findings reported against it:\n\n" + listed.String() + answer, err := r.answer(ctx, system, user, verdictAsk(r.Provider)) + if err != nil { + return nil, answer, err + } + r.spent(job, answer) + raised, err := object(answer.Text) + if err != nil { + prose := first(answer.Text, 200) + answer, err = r.answer(ctx, system, user+"\n\n"+verdictsAgain, verdictAsk(r.Provider)) + if err != nil { + return nil, answer, err + } + r.spent(job, answer) + if raised, err = object(answer.Text); err != nil { + return nil, answer, fmt.Errorf("%w, twice: %s", err, prose) + } + } + var read struct { + Verdicts []struct { + Index int `json:"index"` + Holds bool `json:"holds"` + Reason string `json:"reason"` + } `json:"verdicts"` + } + if err := json.Unmarshal([]byte(raised), &read); err != nil { + return nil, answer, fmt.Errorf("the verdicts are not the shape asked for: %w", err) + } + held := map[int]verdict{} + for _, v := range read.Verdicts { + if v.Index >= 0 && v.Index < len(indexes) { + held[v.Index] = verdict{Holds: v.Holds, Reason: v.Reason} + } + } + return held, answer, nil +} + +// verdictAsk adapts whichever way a provider can be asked for verdicts — +// enforced as a tool where it can be, described in the prompt where it +// cannot — to one ask. +func verdictAsk(provider Provider) func(context.Context, string, string) (Answer, error) { + if v, ok := provider.(Verdicts); ok { + return v.AskVerdict + } + return provider.Ask +} + +func first(s string, n int) string { + s = strings.TrimSpace(s) + if len(s) > n { + return s[:n] + "…" + } + return s +} + +// API asks the console API directly. It is the one provider that can enforce +// the answer's shape rather than request it, so a malformed answer is +// impossible here rather than merely unlikely. +type API struct{ Model string } + +func (a API) Name() string { return "api/" + a.Model } + +// schema is the shape a finding takes, enforced where a provider can enforce +// it and described in the prompt where it cannot. var schema = anthropic.ToolInputSchemaParam{ Properties: map[string]any{ "findings": map[string]any{ @@ -72,95 +480,76 @@ var schema = anthropic.ToolInputSchemaParam{ Required: []string{"findings"}, } -// Reviewer runs jobs against a change. -type Reviewer struct { - Client anthropic.Client - Verbose bool +// verdictSchema is the shape the second reading answers in, held to the +// same strictness where the provider allows it. +var verdictSchema = anthropic.ToolInputSchemaParam{ + Properties: map[string]any{ + "verdicts": map[string]any{ + "type": "array", + "items": map[string]any{ + "type": "object", + "properties": map[string]any{ + "index": map[string]any{"type": "integer", "description": "the number of the finding, as it was listed"}, + "holds": map[string]any{"type": "boolean", "description": "whether the finding stands against the criteria"}, + "reason": map[string]any{"type": "string", "description": "why it holds or falls, one sentence"}, + }, + "required": []string{"index", "holds", "reason"}, + "additionalProperties": false, + }, + }, + }, + Required: []string{"verdicts"}, } -// Run works every job that has something to read, at once. One job failing -// does not stop the others: a review that reports four of five readings is -// worth more than one that reports none. -func (r Reviewer) Run(ctx context.Context, change *Change, jobs []Job) ([]Finding, []error) { - type result struct { - findings []Finding - err error - } - var ( - wg sync.WaitGroup - mu sync.Mutex - all []Finding - errored []error - ) - for _, job := range jobs { - subject := job.Subject(change) - if strings.TrimSpace(subject) == "" { - if r.Verbose { - fmt.Printf(" %-12s nothing to read\n", job.Name) - } - continue - } - wg.Add(1) - go func() { - defer wg.Done() - findings, err := r.ask(ctx, job, subject) - mu.Lock() - defer mu.Unlock() - if err != nil { - errored = append(errored, fmt.Errorf("%s: %w", job.Name, err)) - return - } - all = append(all, findings...) - }() - } - wg.Wait() - return all, errored +func (a API) Ask(ctx context.Context, system, user string) (Answer, error) { + return a.call(ctx, system, user, "report_findings", "Report what this reading found, or an empty list.", schema) } -// ask puts one job's question. The criteria go in the system prompt where they -// can be cached across jobs and runs; the subject goes last, where it cannot -// disturb that prefix. -func (r Reviewer) ask(ctx context.Context, job Job, subject string) ([]Finding, error) { +// Verdicts is what a provider offers when it can hold the second reading to +// its shape as well. +type Verdicts interface { + AskVerdict(ctx context.Context, system, user string) (Answer, error) +} + +func (a API) AskVerdict(ctx context.Context, system, user string) (Answer, error) { + return a.call(ctx, system, user, "report_verdicts", "Report whether each finding holds.", verdictSchema) +} + +// call asks one question with one strict tool, which is the whole of both +// asks: the shape differs, the conversation does not. +func (a API) call(ctx context.Context, system, user, name, description string, shape anthropic.ToolInputSchemaParam) (Answer, error) { tool := anthropic.ToolParam{ - Name: "report_findings", - Description: anthropic.String("Report what this reading found, or an empty list."), - InputSchema: schema, + Name: name, + Description: anthropic.String(description), + InputSchema: shape, Strict: anthropic.Bool(true), } - resp, err := r.Client.Messages.New(ctx, anthropic.MessageNewParams{ - Model: model, + client := anthropic.NewClient() + resp, err := client.Messages.New(ctx, anthropic.MessageNewParams{ + Model: a.Model, MaxTokens: 4096, // Pinned, because a loop between two models cannot converge if one of // them answers differently each time it is asked. Temperature: anthropic.Float(0), System: []anthropic.TextBlockParam{{ - Text: instruction + "\n\n" + job.Criteria, + Text: system, CacheControl: anthropic.NewCacheControlEphemeralParam(), }}, - Tools: []anthropic.ToolUnionParam{{OfTool: &tool}}, - Messages: []anthropic.MessageParam{ - anthropic.NewUserMessage(anthropic.NewTextBlock(subject)), - }, + Tools: []anthropic.ToolUnionParam{{OfTool: &tool}}, + Messages: []anthropic.MessageParam{anthropic.NewUserMessage(anthropic.NewTextBlock(user))}, }) if err != nil { - return nil, err + return Answer{}, err } - if r.Verbose { - fmt.Printf(" %-12s %d in, %d out, %d cached\n", - job.Name, resp.Usage.InputTokens, resp.Usage.OutputTokens, resp.Usage.CacheReadInputTokens) + answer := Answer{ + In: int(resp.Usage.InputTokens), Out: int(resp.Usage.OutputTokens), + Cached: int(resp.Usage.CacheReadInputTokens), } - allowed := rules(job.Criteria) - var out []Finding for _, block := range resp.Content { - use, ok := block.AsAny().(anthropic.ToolUseBlock) - if !ok { - continue - } - answer, err := decode(use.JSON.Input.Raw()) - if err != nil { - return nil, err + if use, ok := block.AsAny().(anthropic.ToolUseBlock); ok { + answer.Text = use.JSON.Input.Raw() + return answer, nil } - out = append(out, answer.findings(job.Name, allowed)...) } - return out, nil + return answer, fmt.Errorf("the model answered without reporting") } diff --git a/criteria/claims.md b/criteria/claims.md @@ -15,5 +15,9 @@ requires. Those assertions are the subject. version, no tool, no test that pins it. - `stale` — the comment describes behaviour the change itself has altered. +These overlap. Where more than one fits, report the first that does and only that +one: a comment gets one finding. + A comment that is merely wordy is not a finding. Only judge assertions that could be -false. +false. A comment saying what the code in front of it does is never a claim, however +confidently it says it. diff --git a/criteria/duplication.md b/criteria/duplication.md @@ -11,8 +11,46 @@ searching for its words. Decide whether the new thing already exists. - `in-the-stdlib` — the standard library already defines this. A hand-copied constant (`0x2000` for `IMAGE_FILE_DLL`) is the common case. - `layout-crosses-a-seam` — one package states the byte layout, field order or - wire format that another package owns. The owner should expose it. + wire format that another package owns. A size constant describing a structure + another package reads or writes is the usual shape of it. The owner should + expose it. + +An error value is the exception to all of these. A package declares its own +sentinels so that its callers can test for them without importing somewhere else, +and that is part of its interface however identical the text is. Never report one +error value as a duplicate of another. + +## The pass to make + +Before judging anything, list the declarations that have a candidate marked +`<- same value`. Then judge every entry on that list, including the ones that come +after the first duplicate you find. Stopping early because a duplicate has already +been reported is the mistake this job makes most often. + +A declaration with several same-value candidates is judged against each of them in +turn, and each pair that survives is its own finding. + +For each pair, say what the two values measure, then compare the answers: + +- Both measure the same field of the same structure — the same header, the same + directory row, the same signature, the same magic. One fact with two owners: + report it, and say which declaration should own it. +- One identifies and the other measures — a resource type id beside a row width, a + count beside a byte size, an index beside a length. Different facts that happen to + be equal: say nothing about them. + +A header size and another package's directory size that are equal because they +describe the same header are the case this job exists for. Report each such pair +separately, one finding each. Only report when the candidate list actually contains the thing. You cannot see the whole repository, so do not guess that something exists somewhere. If the candidates do not show a duplicate, report nothing. + +## Reading across languages + +The repository's declarations arrive read by the language's own grammar, so a +name is found however it is laid out and a constant's value is compared +whether the line says `=` or `::`. The error-value exception above is a Go and +TypeScript shape: a package owns its sentinels. A language that reports errors +by returning them has nothing of that shape to skip. diff --git a/criteria/namer.md b/criteria/namer.md @@ -6,7 +6,10 @@ Judge each name against these rules. Report only names the change adds or rename noun (`Stored`, `Encoded`, `Prepared`) names a property rather than a thing, and reads wrong in the plural: "a slice of stored" is not a sentence. - `verb-for-func` — a function that acts is a verb phrase (`Assemble`, `Identify`). - A function that answers a question may read as one (`IsLibrary`, `Supports`). + A function that answers a question may read as one (`IsLibrary`, `Supports`). A + function whose whole job is to produce or convert to the thing it is named for is + idiomatic Go and is not a finding: `Payload`, `side`, `String`. Go names an + accessor for the value, not for the fetching. - `no-stutter` — a name does not repeat its package: `ico.IcoEntry`, `exe.ExeKind`. Within a package the package name is already said. - `no-shadow` — a name does not collide with something already in scope, especially @@ -14,9 +17,18 @@ Judge each name against these rules. Report only names the change adds or rename - `says-what-not-how` — the name describes what the thing is or does, not the mechanism. `containerSniff` says how; `container` says what. - `matches-neighbours` — the name uses the vocabulary its package already uses. If - a package calls them entries, a new one is not a record. + a package calls them entries, a new one is not a record. This is about the word + chosen for a thing, not about two things holding the same value: whether a fact has + been stated twice is another reader's job, and you are not the one to report it. - `abbreviation` — no invented abbreviation. Established ones (`id`, `url`, `png`) are fine; `cfg`, `mgr`, `hdlr` are not. Do not comment on names the change did not touch. Do not propose a rename whose only merit is your preference: if the existing name satisfies the rules, say nothing. + +## Reading across languages + +The names arrive whatever the language writes them in; judge the words, not the +separators. The accessor convention named above is Go's shape of a wider rule: +wherever the language has a factory or getter convention, a name following it is +not a finding. diff --git a/criteria/tests.md b/criteria/tests.md @@ -18,3 +18,11 @@ whether they are tidy. The question is whether they can fail. Report the specific assertion at fault, and what to assert instead. Ignore style, naming and table-versus-loop questions entirely. + +## Reading across languages + +A test body arrives whole however the language runs it: a Go `Test` function, a +call to `test` or `it` in TypeScript, an `@(test)` procedure in Odin. The skip +shapes differ with them (`t.Skip`, `test.skip`, a test returning early) but the +rule does not: a test that checks nothing and reports success is a finding in +each. diff --git a/eval_test.go b/eval_test.go @@ -0,0 +1,383 @@ +package main + +import ( + "context" + "flag" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" +) + +// The eval set is the only thing here that spends money, so it is off unless +// asked for: go test -run TestEval -eval=1. More than one run per case is +// worth having, because a single reading walks between the two failure +// directions rather than sitting at one. +var ( + evalRuns = flag.Int("eval", 0, "Run the eval set this many times per case. This asks a provider, and costs money.") + evalCases = flag.String("eval.cases", "", "Run only the eval cases whose names contain this.") + evalShow = flag.Bool("eval.show", false, "Print every finding, not only the ones that scored.") +) + +// A want is a finding the reading has to produce. The rule and the file are +// compared where they are given; About is the words that identify which +// duplicate, name or assertion is meant. +type want struct { + Rule string + File string + About []string +} + +func (w want) String() string { + about := strings.Join(w.About, "+") + if about == "" { + about = "anything at all" + } + if w.File != "" { + about += " in " + w.File + } + if w.Rule == "" { + return about + } + return w.Rule + " about " + about +} + +// matches reports whether a finding is the one wanted. The subject is looked +// for in what the finding says rather than where it says it, so that a case +// about one name is not answered by any finding in a file of that name. +func (w want) matches(f Finding) bool { + if w.Rule != "" && f.Rule != w.Rule { + return false + } + if w.File != "" && f.File != w.File { + return false + } + text := strings.ToLower(strings.Join([]string{f.Message, f.Fix, f.Symbol}, " ")) + for _, about := range w.About { + if !strings.Contains(text, strings.ToLower(about)) { + return false + } + } + return true +} + +// An evalCase is one reading with a known right answer. The silences matter +// more than the wants: a reading that reports a true and unwanted thing is +// the failure this set exists to catch. +type evalCase struct { + Name string + // Corpus builds or names the repository to read. + Corpus func(*testing.T) string + Rev string + Jobs string + Want []want + // Silent is the subjects no finding may raise. + Silent []want + // Most is the largest number of findings the reading may report. Left + // out, any number is allowed: it is the cases with one right answer that + // need a ceiling. + Most int +} + +func evalSet() []evalCase { + return []evalCase{{ + Name: "duplication/layout-crosses-a-seam", + Corpus: icns, + Rev: "ebed95f^..ebed95f", + Jobs: "duplication", + Want: []want{ + {Rule: "layout-crosses-a-seam", About: []string{"groupHeaderSize"}}, + {About: []string{"icoEntrySize"}}, + }, + Silent: []want{ + // Per-package error sets are a deliberate convention here. + {About: []string{"ErrNoIcons"}}, + // A resource type id and a row width are not the same fact + // however equal they are. + {About: []string{"typeIconGroup"}}, + }, + }, { + Name: "namer/adjective-for-a-type", + Corpus: icns, + Rev: "6f52a0b^..6f52a0b", + Jobs: "namer", + Want: []want{{Rule: "noun-for-type", About: []string{"Stored"}}}, + Silent: []want{ + {About: []string{"maxIcons"}}, + {About: []string{"payload"}}, + {About: []string{"largest"}}, + }, + Most: 1, + }, { + // A purely mechanical loop rewrite. Every finding here is noise. + Name: "quiet/mechanical-rewrite", + Corpus: icns, + Rev: "4c2e616^..4c2e616", + Jobs: "duplication,namer,claims", + Silent: []want{{About: []string{""}}}, + }, { + Name: "tests/skips-in-normal-conditions", + Corpus: synthetic, + Rev: "", + Jobs: "tests", + Want: []want{{Rule: "skips-in-normal-conditions", File: "shell_test.go"}}, + // No ceiling: the skipping test has one fault worth the name, but a + // second reading of how weakly it asserts is a judgement the criteria + // allow. The test that is meant to be good is the negative here. + Silent: []want{{About: []string{"reverse"}}, {About: []string{"TestDecodeReverses"}}}, + }, { + Name: "claims/unsupported-claim", + Corpus: synthetic, + Rev: "", + Jobs: "claims", + Want: []want{{Rule: "unsupported-claim", About: []string{"Windows"}}}, + Silent: []want{{File: "shell.go", About: []string{"decode"}}}, + Most: 1, + }} +} + +// icns is the corpus: a repository with rich history and known problems. +func icns(t *testing.T) string { + t.Helper() + root := os.Getenv("REVIEW_EVAL_CORPUS") + if root == "" { + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory to find the corpus in") + } + root = filepath.Join(home, "Source", "Personal", "icns") + } + if _, err := os.Stat(filepath.Join(root, ".git")); err != nil { + t.Skipf("no corpus at %s; set REVIEW_EVAL_CORPUS", root) + } + return root +} + +// synthetic is a repository written for the cases the corpus does not happen +// to contain. +func synthetic(t *testing.T) string { + t.Helper() + r := newRepo(t) + r.write("go.mod", "module synthetic\n\ngo 1.27.0\n") + r.write("shell.go", "package synthetic\n") + r.write("shell_test.go", "package synthetic\n") + r.commit("first", "go.mod", "shell.go", "shell_test.go") + + r.write("shell.go", `package synthetic + +// Windows refuses an icon group whose rows are not ordered largest first, so +// the rows are sorted before they are written. +func order(rows []int) []int { return rows } + +// decode reads the bitmap an icon holds, turning it the other way up. +func decode(data []byte) []byte { + out := make([]byte, len(data)) + for i, b := range data { + out[len(data)-1-i] = b + } + return out +} +`) + r.write("shell_test.go", `package synthetic + +import ( + "os/exec" + "testing" +) + +func TestAgainstIcotool(t *testing.T) { + if _, err := exec.LookPath("icotool"); err != nil { + t.Skip("icotool is not installed") + } + out, err := exec.Command("icotool", "--list", "testdata/one.ico").Output() + if err != nil { + t.Fatal(err) + } + const want = "--icon --index=1 --width=16 --height=16 --bit-depth=32\n" + if string(out) != want { + t.Fatalf("got %q, want %q", out, want) + } +} + +func TestDecodeReverses(t *testing.T) { + got := decode([]byte{1, 2, 3}) + if len(got) != 3 || got[0] != 3 || got[1] != 2 || got[2] != 1 { + t.Fatalf("got %v, want the bytes the other way up", got) + } +} +`) + r.stage("shell.go", "shell_test.go") + return r.Root +} + +// counting wraps a provider so a run can report what it spent. +type counting struct { + Provider + mu sync.Mutex + cost float64 + asks int +} + +func (c *counting) Ask(ctx context.Context, system, user string) (Answer, error) { + answer, err := c.Provider.Ask(ctx, system, user) + c.mu.Lock() + c.cost += answer.Cost + c.asks++ + c.mu.Unlock() + return answer, err +} + +// score is one case's result over however many runs it was given. The +// subjects keep the order the case lists them in, so two scoreboards can be +// read side by side. +type score struct { + Name string + // Most is the largest number of findings any run reported, where that + // was more than the case allows. + Most int + Found []tally + Noise []tally + Failures int +} + +// tally is how often one subject was raised across the runs. +type tally struct { + Subject string + Count int +} + +func TestEval(t *testing.T) { + if *evalRuns <= 0 { + t.Skip("the eval set asks a provider; run it with -eval=1") + } + which := orElse(os.Getenv("REVIEW_PROVIDER"), "claude") + build, known := Providers()[which] + if !known { + t.Fatalf("no provider called %q", which) + } + provider := &counting{Provider: build(os.Getenv("REVIEW_MODEL"))} + + var scores []score + for _, c := range evalSet() { + if *evalCases != "" && !strings.Contains(c.Name, *evalCases) { + continue + } + t.Run(c.Name, func(t *testing.T) { + scores = append(scores, runCase(t, c, provider)) + }) + } + t.Cleanup(func() { + board := scoreboard(scores, *evalRuns) + t.Log("\n" + board) + fmt.Printf("\n%s\nasked %s %d times for $%.4f\n", board, provider.Name(), provider.asks, provider.cost) + }) +} + +func runCase(t *testing.T, c evalCase, provider Provider) score { + root := c.Corpus(t) + change, err := Gather(c.Rev, root) + if err != nil { + t.Fatal(err) + } + jobs, err := chosen(c.Jobs) + if err != nil { + t.Fatal(err) + } + s := score{Name: c.Name} + for _, w := range c.Want { + s.Found = append(s.Found, tally{Subject: w.String()}) + } + for _, w := range c.Silent { + s.Noise = append(s.Noise, tally{Subject: w.String()}) + } + for range *evalRuns { + result := Reviewer{Provider: provider}.Run(context.Background(), change, jobs) + for _, err := range result.Failures { + t.Errorf("job failed: %v", err) + s.Failures++ + } + for i, w := range c.Want { + if raised(w, result.Findings) { + s.Found[i].Count++ + } + } + for i, w := range c.Silent { + if raised(w, result.Findings) { + s.Noise[i].Count++ + } + } + if c.Most > 0 && len(result.Findings) > c.Most { + s.Most = max(s.Most, len(result.Findings)) + } + if *evalShow { + for _, f := range result.Findings { + t.Logf(" %s", strings.ReplaceAll(f.String(), "\n", " ")) + } + if len(result.Findings) == 0 { + t.Log(" (nothing reported)") + } + } + } + for _, found := range s.Found { + if found.Count < *evalRuns { + t.Errorf("missed %s in %d of %d runs", found.Subject, *evalRuns-found.Count, *evalRuns) + } + } + for _, noise := range s.Noise { + if noise.Count > 0 { + t.Errorf("raised %s in %d of %d runs, which is true but unwanted", noise.Subject, noise.Count, *evalRuns) + } + } + if s.Most > 0 { + t.Errorf("reported %d findings at most, where %d is the whole right answer", s.Most, c.Most) + } + return s +} + +func raised(w want, findings []Finding) bool { + for _, f := range findings { + if w.matches(f) { + return true + } + } + return false +} + +// scoreboard lays the results out so that a change to the criteria can be +// read as a movement rather than a verdict. +func scoreboard(scores []score, runs int) string { + var b strings.Builder + var found, wanted, noise int + fmt.Fprintf(&b, "eval over %d run(s) per case\n", runs) + for _, s := range scores { + fmt.Fprintf(&b, "\n%s\n", s.Name) + for _, t := range s.Found { + label := "found" + if t.Count < runs { + label = "MISS " + } + fmt.Fprintf(&b, " %s %d/%d %s\n", label, t.Count, runs, t.Subject) + found += t.Count + wanted += runs + } + for _, t := range s.Noise { + label := "quiet" + if t.Count > 0 { + label = "NOISE" + } + fmt.Fprintf(&b, " %s %d/%d %s\n", label, t.Count, runs, t.Subject) + noise += t.Count + } + if s.Most > 0 { + fmt.Fprintf(&b, " LOUD reported as many as %d findings\n", s.Most) + noise++ + } + if s.Failures > 0 { + fmt.Fprintf(&b, " failed %d time(s)\n", s.Failures) + } + } + fmt.Fprintf(&b, "\nfound %d of %d wanted, %d unwanted\n", found, wanted, noise) + return b.String() +} diff --git a/finding.go b/finding.go @@ -3,6 +3,8 @@ package main import ( "bufio" "cmp" + "crypto/sha256" + "encoding/hex" "fmt" "os" "regexp" @@ -71,6 +73,15 @@ type Finding struct { // Fix is the concrete change suggested, which is what lets an agent act // on the finding rather than reason about it again. Fix string `json:"fix,omitempty"` + // Verified is what stands behind the finding. The deterministic checks + // and staticcheck verify themselves; a model's finding is verified when + // a second reading of the same evidence let it stand. A finding no + // second reading has seen is false until one has. + Verified bool `json:"verified"` + // ID is a stable short name for the finding: a hash of what it is about, + // not where it sits, so that a re-run of the same change names the same + // finding again and a loop can refer to one by name. + ID string `json:"id,omitempty"` } func (f Finding) String() string { @@ -113,6 +124,16 @@ func Sort(findings []Finding) { }) } +// identify gives a finding its short id. The line is left out of the hash: +// lines move under edits that do not touch the finding, and an id that +// changes with every re-run names nothing. +func identify(f *Finding) { + h := sha256.Sum256([]byte(strings.Join([]string{ + f.Job, f.Rule, f.File, f.Symbol, f.Message, + }, "\x00"))) + f.ID = hex.EncodeToString(h[:])[:12] +} + // ignore matches the comment that dismisses a finding. It lives in the source // beside what it justifies, rather than in a file of its own, so that the // reason is where a reader needs it and survives a clone. diff --git a/finding_test.go b/finding_test.go @@ -0,0 +1,224 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestParseSeverity(t *testing.T) { + for _, test := range []struct { + in string + want Severity + }{ + {"must-fix", MustFix}, + {"must fix", MustFix}, + {"MustFix", MustFix}, + {"error", MustFix}, + {" CONSIDER ", Consider}, + {"warning", Consider}, + {"note", Note}, + // A job that invents a word is read as the least serious rather than + // stopping the run. + {"catastrophic", Note}, + {"", Note}, + } { + if got := ParseSeverity(test.in); got != test.want { + t.Errorf("ParseSeverity(%q) = %v, want %v", test.in, got, test.want) + } + } +} + +func TestSeverityString(t *testing.T) { + for severity, want := range map[Severity]string{ + MustFix: "must-fix", Consider: "consider", Note: "note", Severity(9): "unknown", + } { + if got := severity.String(); got != want { + t.Errorf("Severity(%d) = %q, want %q", severity, got, want) + } + } +} + +func TestSort(t *testing.T) { + findings := []Finding{ + {Severity: Note, File: "a.go", Line: 1}, + {Severity: MustFix, File: "b.go", Line: 9}, + {Severity: MustFix, File: "b.go", Line: 2}, + {Severity: MustFix, File: "a.go", Line: 50}, + {Severity: Consider, File: "a.go", Line: 1}, + } + Sort(findings) + var got []string + for _, f := range findings { + got = append(got, f.Severity.String()+" "+f.File+":"+string(rune('0'+f.Line%10))) + } + want := []string{"must-fix a.go:0", "must-fix b.go:2", "must-fix b.go:9", "consider a.go:1", "note a.go:1"} + for i := range want { + if got[i] != want[i] { + t.Fatalf("got %v, want %v", got, want) + } + } +} + +func TestFindingString(t *testing.T) { + f := Finding{ + Job: "namer", Rule: "noun-for-type", Severity: Consider, + File: "ico/ico.go", Line: 12, Message: "Stored is an adjective.", + Fix: "Call it Encoded.", + } + got := f.String() + for _, want := range []string{"consider: ", "ico/ico.go:12: ", "Stored is an adjective.", "→ Call it Encoded.", "[namer/noun-for-type]"} { + if !strings.Contains(got, want) { + t.Errorf("%q missing from:\n%s", want, got) + } + } + // A finding about a name rather than a place is located by the name. + bare := Finding{Job: "namer", Rule: "abbreviation", Symbol: "cfg", Message: "Invented."} + if !strings.Contains(bare.String(), "cfg: Invented.") { + t.Errorf("got %q", bare.String()) + } +} + +// Suppressed reads the source the finding names, relative to the working +// directory, which is the repository root by the time it is called. +func TestSuppressed(t *testing.T) { + dir := t.TempDir() + source := `package x + +// nothing here +const a = 1 + +//review:ignore restates-a-fact the ico package owns the other one +const b = 6 + +const c = 7 + +const d = 8 + +const e = 9 +` + if err := os.WriteFile(filepath.Join(dir, "x.go"), []byte(source), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(dir) + + for _, test := range []struct { + name string + finding Finding + want bool + why string + }{{ + name: "a dismissal just above the line covers it", + finding: Finding{File: "x.go", Line: 7, Rule: "restates-a-fact"}, + want: true, + why: "the ico package owns the other one", + }, { + name: "a dismissal on the line covers it", + finding: Finding{File: "x.go", Line: 6, Rule: "restates-a-fact"}, + want: true, + why: "the ico package owns the other one", + }, { + name: "a dismissal does not reach a distant line", + finding: Finding{File: "x.go", Line: 12, Rule: "restates-a-fact"}, + want: false, + }, { + name: "a dismissal does not cover another rule", + finding: Finding{File: "x.go", Line: 7, Rule: "already-named"}, + want: false, + }, { + name: "a finding with no line is covered anywhere in the file", + finding: Finding{File: "x.go", Rule: "restates-a-fact"}, + want: true, + why: "the ico package owns the other one", + }, { + name: "a finding with no file is never dismissed", + finding: Finding{Rule: "restates-a-fact"}, + want: false, + }, { + name: "a file that is not there dismisses nothing", + finding: Finding{File: "nowhere.go", Line: 1, Rule: "restates-a-fact"}, + want: false, + }} { + t.Run(test.name, func(t *testing.T) { + why, ok := Suppressed(test.finding) + if ok != test.want { + t.Fatalf("got %v, want %v", ok, test.want) + } + if ok && why != test.why { + t.Errorf("reason: got %q, want %q", why, test.why) + } + }) + } +} + +func TestSuppressedAll(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "x.go"), []byte("//review:ignore all generated\nconst a = 1\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(dir) + why, ok := Suppressed(Finding{File: "x.go", Line: 2, Rule: "anything-at-all"}) + if !ok || why != "generated" { + t.Errorf("got (%q, %v), want (generated, true)", why, ok) + } +} + +func TestSuppressedWithoutAReason(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "x.go"), []byte("// review:ignore cannot-fail\nfunc TestX(t *testing.T) {}\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(dir) + why, ok := Suppressed(Finding{File: "x.go", Line: 2, Rule: "cannot-fail"}) + if !ok { + t.Fatal("a dismissal with a space after the slashes still dismisses") + } + if why != "no reason given" { + t.Errorf("got %q", why) + } +} + +func TestRulesMatch(t *testing.T) { + for _, test := range []struct { + dismissed, rule string + want bool + }{ + {"all", "anything", true}, + {"cannot-fail", "cannot-fail", true}, + {"Cannot-Fail", "cannot-fail", true}, + {"cannot-fail", "name-overclaims", false}, + } { + if got := rulesMatch(test.dismissed, test.rule); got != test.want { + t.Errorf("rulesMatch(%q, %q) = %v, want %v", test.dismissed, test.rule, got, test.want) + } + } +} + +func TestAtoi(t *testing.T) { + for in, want := range map[string]int{"12": 12, " 7 ": 7, "": 0, "x": 0, "-3": -3} { + if got := atoi(in); got != want { + t.Errorf("atoi(%q) = %d, want %d", in, got, want) + } + } +} + +// A finding's id is what a loop uses to answer it and to check it stayed +// answered, so the same finding names itself the same way across runs, and +// a line that moved under it does not change its name. +func TestFindingIDIsStable(t *testing.T) { + f := Finding{Job: "tests", Rule: "cannot-fail", File: "x_test.go", Line: 4, Symbol: "TestX", Message: "m"} + identify(&f) + moved := f + moved.Line = 9 + identify(&moved) + if moved.ID != f.ID || f.ID == "" { + t.Errorf("the id moved with the line: %q then %q", f.ID, moved.ID) + } + other := f + other.Message = "a different finding" + identify(&other) + if other.ID == f.ID { + t.Error("two findings share one id") + } +} diff --git a/frontend.go b/frontend.go @@ -0,0 +1,166 @@ +package main + +import ( + "fmt" + "maps" + "os" + "os/exec" + "path/filepath" + "slices" + "strings" +) + +// Features names what a frontend can read out of a language, so a job can be +// skipped with a reason where the reading is not possible rather than faked. +type Features uint8 + +const ( + // FeatSymbols covers the declarations a change adds, which the + // duplication and namer jobs read. + FeatSymbols Features = 1 << iota + // FeatTests covers whole test functions, which the tests job reads. + FeatTests + // FeatComments covers the prose a change adds, which the claims job + // reads. + FeatComments + // FeatIndex covers the declarations already in the repository, which the + // duplication shortlist is built from. + FeatIndex +) + +// String renders the mask as the feature names, comma separated. +func (f Features) String() string { + names := []struct { + bit Features + name string + }{ + {FeatSymbols, "symbols"}, + {FeatTests, "tests"}, + {FeatComments, "comments"}, + {FeatIndex, "index"}, + } + var out []string + for _, n := range names { + if f&n.bit != 0 { + out = append(out, n.name) + } + } + if len(out) == 0 { + return "nothing" + } + s := out[0] + for _, rest := range out[1:] { + s += ", " + rest + } + return s +} + +// Frontend reads one language. Its Change method appends what the change +// adds; its Whole method reads the declarations already in the repository, +// which is what a new name is judged against. Language judgment — what +// counts as exported, which functions are tests, where a doc comment sits — +// is the frontend's, so the jobs never carry it. +type Frontend interface { + Name() string + // Covers reports whether the path is this frontend's language. + Covers(path string) bool + Features() Features + // Change appends the declarations, tests and prose the added lines of + // this frontend's files introduce. + Change(root, rev string, c *Change, added map[string][]int) error + // Whole reads the declarations in the repository at the revision. + Whole(root, rev string) ([]Declared, error) +} + +// frontends returns the readers, in coverage order. A frontend whose +// dependencies are not installed is left out rather than asked for and +// failed, so its files fall to the heuristic reader and say why. +func frontends() []Frontend { + readers := []Frontend{GoFrontend{}} + // The TypeScript frontend is only offered when ast-grep is installed; its + // files fall to the heuristic reader otherwise, which says why. + if _, err := exec.LookPath("ast-grep"); err == nil { + readers = append(readers, TSFrontend{}) + } + // Same for the Odin sidecar, which is built from sidecar/odin in this + // repository. + if _, err := exec.LookPath("odin-review-extract"); err == nil { + readers = append(readers, OdinFrontend{}) + } + return append(readers, Heuristic{}) +} + +// reader picks the first frontend that covers the path. +func reader(path string, frontends []Frontend) Frontend { + for _, f := range frontends { + if f.Covers(path) { + return f + } + } + return nil +} + +// read parses the change's files through the frontend that covers each. What +// a language cannot provide is skipped with a line on stderr, so a job's +// "nothing to read" is explained rather than mysterious. Every file that +// went without its part is named in the change's uncovered list, because a +// job that read nothing reports nothing, and the program reading the report +// has to know which silence is real. +func (c *Change) read(root, rev string, frontends []Frontend) { + added := addedLines(c.Diff) + + // owned records, per frontend, the paths it covers in this change, so a + // missing feature is reported once per language rather than per file. + owned := map[Frontend]map[string]bool{} + exts := map[Frontend]map[string]bool{} + for _, name := range c.Files { + f := reader(name, frontends) + if f == nil { + // A file no reader could plausibly exist for — prose, data, + // configuration — is not a promise the review broke; a code + // file with no reader is, because a job that would have read + // it read nothing. + if isCodeFile(name) { + c.Uncovered = append(c.Uncovered, Gap{File: name, Reason: "no reader"}) + } + continue + } + if owned[f] == nil { + owned[f] = map[string]bool{} + exts[f] = map[string]bool{} + } + owned[f][name] = true + exts[f][filepath.Ext(name)] = true + } + if len(owned) == 0 { + return + } + + missing := func(covers map[string]bool, f Frontend, want Features, jobs string) { + if f.Features()&want != 0 { + return + } + extensions := make([]string, 0, len(exts[f])) + for ext := range exts[f] { + extensions = append(extensions, ext) + } + slices.Sort(extensions) + fmt.Fprintf(os.Stderr, " skipping %s for %s (%d file(s), %s): no parser\n", jobs, strings.Join(extensions, ", "), len(covers), f.Name()) + for _, name := range slices.Sorted(maps.Keys(covers)) { + c.Uncovered = append(c.Uncovered, Gap{File: name, Reason: "no " + jobs + " parser"}) + } + } + + for f, covers := range owned { + if err := f.Change(root, rev, c, added); err != nil { + fmt.Fprintf(os.Stderr, " %s: %v\n", f.Name(), err) + for _, name := range slices.Sorted(maps.Keys(covers)) { + c.Uncovered = append(c.Uncovered, Gap{File: name, Reason: "reader failed"}) + } + continue + } + missing(covers, f, FeatSymbols, "duplication, namer") + missing(covers, f, FeatTests, "tests") + missing(covers, f, FeatComments, "claims") + } +} diff --git a/frontend_test.go b/frontend_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "os" + "strings" + "testing" +) + +// captureStderr swaps stderr for a pipe while the work runs, so a test can +// read what the gatherer told the user. +func captureStderr(t *testing.T, work func()) string { + t.Helper() + real := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + os.Stderr = w + work() + w.Close() + os.Stderr = real + var out strings.Builder + buf := make([]byte, 4096) + for { + n, err := r.Read(buf) + out.Write(buf[:n]) + if err != nil { + break + } + } + return out.String() +} + +func TestHeuristicReadsOdinComments(t *testing.T) { + r := newRepo(t) + r.write("heimdall/main.odin", `package main + +import "core:fmt" + +main :: proc() { + fmt.println("hello") + // counts the icons a binary carries +} +`) + r.commit("main: hello", "heimdall/main.odin") + r.write("heimdall/main.odin", `package main + +import "core:fmt" + +// across the whole window +main :: proc() { + fmt.println("goodbye") +} +`) + r.stage("heimdall/main.odin") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Comments) != 1 || change.Comments[0].Text != "across the whole window" { + t.Fatalf("comments %v, want the one added comment", change.Comments) + } +} + +func TestHeuristicKeepsBlockCommentsWhole(t *testing.T) { + r := newRepo(t) + r.write("f/open.c", "int open(void) { return 0; }\n") + r.commit("f: open", "f/open.c") + r.write("f/open.c", `/* reads the icons + a binary carries */ +int open(void) { return 1; } +`) + r.stage("f/open.c") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Comments) != 2 { + t.Fatalf("comments %v, want both block lines", change.Comments) + } + if change.Comments[0].Text != "reads the icons" || change.Comments[1].Text != "a binary carries" { + t.Fatalf("comments %q, %q", change.Comments[0].Text, change.Comments[1].Text) + } +} + +func TestHeuristicLeavesProseAlone(t *testing.T) { + r := newRepo(t) + r.write("f/open.c", "int open(void) { return 0; }\n") + r.commit("f: open", "f/open.c") + r.write("f/open.c", "#!/usr/bin/env runtime\nint open(void) { return 1; }\n") + r.stage("f/open.c") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Comments) != 0 { + t.Fatalf("comments %v, want a shebang rejected", change.Comments) + } +} + +func TestHeuristicLogsTheSkips(t *testing.T) { + r := newRepo(t) + r.write("f/main.rs", "fn main() {}\n") + r.commit("f: empty", "f/main.rs") + r.write("f/main.rs", "fn main() { }\n") + r.stage("f/main.rs") + + var logged string + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + logged = captureStderr(t, func() { + change.read(r.Root, "", frontends()) + }) + if !strings.Contains(logged, "skipping duplication, namer for .rs") { + t.Fatalf("logged %q, want the duplication and namer skip", logged) + } + if !strings.Contains(logged, "skipping tests for .rs") { + t.Fatalf("logged %q, want the tests skip", logged) + } + if strings.Contains(logged, "skipping claims") { + t.Fatalf("logged %q, want claims served", logged) + } +} diff --git a/gaming.go b/gaming.go @@ -0,0 +1,257 @@ +package main + +// These checks hold the review to itself. A change can pass every job by +// dismissing what they find or by deleting the tests that would have +// failed, and no narrow reader would say a word about it: each job reads +// only its own criteria, and neither mechanism is one job's subject. Both +// are the person reviewing's decision to make, not the change's progress, +// so they are measured here, where nothing is asked of a model and nothing +// can be talked around. + +import ( + "fmt" + "maps" + "path/filepath" + "regexp" + "slices" + "strings" +) + +// checkSuppressionAdded reports what the change dismisses before the +// readers run — the criteria name the rule suppression-added. A finding +// dismissed by the same change that produced it is the reviewer's decision +// to accept, not the change's progress; left unmeasured, a reading can be +// iterated into silence one ignore at a time. It is reported as one finding +// without a file, so it cannot be dismissed in turn — a check that polices +// the dismissing must not be dismissible. +func checkSuppressionAdded(c *Change) []Finding { + added, _ := diffSides(c.Diff) + var spots []string + for _, file := range slices.Sorted(maps.Keys(added)) { + if !isCodeFile(file) { + continue + } + for _, l := range added[file] { + m := ignore.FindStringSubmatch(l.Text) + if m == nil || !ruleID.MatchString(m[1]) { + continue + } + spots = append(spots, fmt.Sprintf("%s:%d (%s)", file, l.Line, m[1])) + } + } + if len(spots) == 0 { + return nil + } + return []Finding{{ + Job: "static", + Rule: "suppression-added", + Severity: MustFix, + Message: fmt.Sprintf( + "the change adds its own dismissal%s: %s; a dismissal is a decision for the person reviewing the change, and a review that dismisses itself cannot be failed", + plural(len(spots)), strings.Join(spots, "; ")), + Fix: "correct what the dismissal covers; a dismissal belongs to the person reviewing, who accepts it deliberately", + }} +} + +// isCodeFile reports whether a path is a place code could live — which is +// where a dismissal could work, and where a reader could plausibly exist. +// The prose and data formats are where the mechanism gets documented and +// where configuration lives, not where findings are made. +func isCodeFile(path string) bool { + base := filepath.Base(path) + switch base { + case ".gitignore", ".gitattributes", ".gitmodules", ".editorconfig", + "Makefile", "Dockerfile", "LICENSE", "CODEOWNERS": + return false + } + for _, ext := range []string{ + ".md", ".mdx", ".txt", ".rst", ".adoc", ".json", ".yaml", ".yml", + ".toml", ".lock", ".sum", ".mod", ".html", ".htm", ".css", ".svg", + ".xml", + } { + if strings.HasSuffix(base, ext) { + return false + } + } + return true +} + +// ruleID is what a dismissal's rule id has to look like. The prose that +// documents the mechanism writes placeholders (<rule>) and quoted examples, +// which match the ignore pattern but dismiss nothing. +var ruleID = regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + +// goTest is a test function's declaration, as a removed line carries it. +var goTest = regexp.MustCompile(`^func ((?:Test|Benchmark|Fuzz)[A-Za-z0-9_]+)\(`) + +// jsTestCall is a test's registration in the JavaScript and TypeScript +// shapes, with the skipping and focusing modifiers read through. +var jsTestCall = regexp.MustCompile(`^(?:test|describe|it)(?:\.(?:skip|only|todo|fails))*\(`) + +// isTestFile reports whether a path is one a test runner reads, in any of the +// naming habits the tool's languages have. +func isTestFile(path string) bool { + base := filepath.Base(path) + for _, marker := range []string{"_test.", ".test.", ".spec."} { + if strings.Contains(base, marker) { + return true + } + } + return false +} + +// checkDeletedTests reports the tests a change deletes. The tests job reads +// the tests a change adds or alters, and a test is never heard from again +// once deleted, so deletion is where a test the reading would have failed +// goes to pass the review. A test the change also adds under a name made of +// the same words is a rename, and is spared. The shapes read here are a Go +// test function's and a JS test registration's; tests in other languages +// are not named by anything this package reads. +func checkDeletedTests(c *Change) []Finding { + added, removed := diffSides(c.Diff) + spoken := addedTestNames(added) + var findings []Finding + for _, file := range slices.Sorted(maps.Keys(removed)) { + if !isTestFile(file) { + continue + } + var names []string + seen := map[string]bool{} + for _, text := range removed[file] { + name := removedTestName(text) + if name == "" || seen[name] { + continue + } + seen[name] = true + if !covered(name, spoken) { + names = append(names, name) + } + } + if len(names) == 0 { + continue + } + shown := names + if len(shown) > 8 { + shown = append(shown[:8], fmt.Sprintf("and %d more", len(names)-8)) + } + var listed []string + for _, name := range shown { + listed = append(listed, fmt.Sprintf("%q", name)) + } + findings = append(findings, Finding{ + Job: "static", + Rule: "test-deleted", + Severity: MustFix, + File: file, + Symbol: names[0], + Message: fmt.Sprintf( + "the change deletes the test%s %s from %s; a deleted test cannot fail again, so deletion is where a failing test goes to pass the review", + plural(len(names)), strings.Join(listed, ", "), file), + Fix: fmt.Sprintf("restore the test, or dismiss it where a reader can read why: a review:ignore comment naming test-deleted, with the reason, in %s", file), + }) + } + return findings +} + +// removedTestName reads the name of a deleted test out of its removed line, +// in the shapes the tool's languages write tests in. +func removedTestName(text string) string { + trimmed := strings.TrimLeft(text, " \t") + if m := goTest.FindStringSubmatch(trimmed); m != nil { + return m[1] + } + if loc := jsTestCall.FindStringSubmatchIndex(trimmed); loc != nil { + return readQuoted(strings.TrimLeft(trimmed[loc[1]:], " \t")) + } + return "" +} + +// readQuoted reads the string literal a JS test registration is named by, +// from the text that follows its opening parenthesis. +func readQuoted(text string) string { + if len(text) == 0 { + return "" + } + q := text[0] + if q != '"' && q != '\'' && q != '`' { + return "" + } + if end := strings.IndexByte(text[1:], q); end >= 0 { + return text[1 : 1+end] + } + return "" +} + +// addedTestNames collects the names of the tests a change adds, read with +// the same shapes the removed side is read with, so that a rename is not +// read as a deletion. +func addedTestNames(added map[string][]diffLine) []string { + seen := map[string]bool{} + for _, lines := range added { + for _, l := range lines { + trimmed := strings.TrimLeft(l.Text, " \t") + if m := goTest.FindStringSubmatch(trimmed); m != nil { + seen[m[1]] = true + } else if loc := jsTestCall.FindStringSubmatchIndex(trimmed); loc != nil { + if name := readQuoted(strings.TrimLeft(trimmed[loc[1]:], " \t")); name != "" { + seen[name] = true + } + } + } + } + return slices.Sorted(maps.Keys(seen)) +} + +// covered reports whether a deleted test's name is spoken for by a test the +// change adds: every word of the old name is in a new one, which is what a +// rename or a re-anchored test looks like. +func covered(deleted string, spoken []string) bool { + want := words(deleted) + if len(want) == 0 { + return false + } + for _, name := range spoken { + have := words(name) + saysAll := true + for _, w := range want { + if !slices.Contains(have, w) { + saysAll = false + break + } + } + if saysAll { + return true + } + } + return false +} + +// words breaks a test's name into its lowercased words: camel humps and +// underscores for the Go shapes, spaces for the strings the JavaScript +// shapes are named by, with the runner's own prefixes and short leftovers +// left out. +func words(name string) []string { + var out []string + for _, part := range split(name) { + for _, piece := range strings.Fields(part) { + piece = strings.ToLower(piece) + if len(piece) < 3 { + continue + } + switch piece { + case "test", "benchmark", "fuzz", "skip", "only", "todo", "fails": + continue + } + out = append(out, piece) + } + } + return out +} + +// plural is the s a count takes when it is not one. +func plural(n int) string { + if n == 1 { + return "" + } + return "s" +} diff --git a/gaming_test.go b/gaming_test.go @@ -0,0 +1,244 @@ +package main + +// The anti-gaming checks hold the review to itself: a change that dismisses +// what the reading found, or deletes the tests that would have failed, is +// measured rather than waved through. Nothing here asks a provider, so +// nothing here can be talked around. + +import ( + "strings" + "testing" +) + +func TestDiffSidesReadsBothSides(t *testing.T) { + diff := `--- a/x_test.go ++++ b/x_test.go +@@ -1,4 +1,4 @@ + package x +-func TestA() {} ++func TestB() {} + func f() {} +` + added, removed := diffSides(diff) + if len(added["x_test.go"]) != 1 || added["x_test.go"][0].Line != 2 || added["x_test.go"][0].Text != "func TestB() {}" { + t.Errorf("added %+v", added) + } + if len(removed["x_test.go"]) != 1 || removed["x_test.go"][0] != "func TestA() {}" { + t.Errorf("removed %+v", removed) + } +} + +// A removed line of content that begins like a header is content, not a +// header: the headers are read where they sit between hunks. +func TestDiffSidesKeepsContentAndHeadersApart(t *testing.T) { + diff := `--- a/notes.txt ++++ b/notes.txt +@@ -1,2 +1,3 @@ + text +-+++ a/other.txt ++--- b/other.txt + more +--- a/gone.txt ++++ /dev/null +@@ -1,2 +0,0 @@ +---- b/other.txt +-more +` + added, removed := diffSides(diff) + if len(added["notes.txt"]) != 1 || added["notes.txt"][0].Line != 2 || added["notes.txt"][0].Text != "--- b/other.txt" { + t.Errorf("added %+v", added) + } + if len(removed["notes.txt"]) != 1 || removed["notes.txt"][0] != "+++ a/other.txt" { + t.Errorf("removed %+v", removed) + } + if len(removed["gone.txt"]) != 2 || removed["gone.txt"][0] != "--- b/other.txt" { + t.Errorf("removed %+v", removed) + } +} + +func TestSuppressionAdded(t *testing.T) { + // The ignore comment is assembled at runtime so this file does not add + // a dismissal of its own — the check reads added lines, and this test's + // source is an added line of itself. + diff := `--- a/ico.go ++++ b/ico.go +@@ -1,3 +1,5 @@ + package ico ++//` + `review:ignore cannot-fail the test can fail ++//review:ignore <rule> <why> + func f() {} +` + findings := checkSuppressionAdded(&Change{Diff: diff}) + if len(findings) != 1 { + t.Fatalf("got %+v", findings) + } + f := findings[0] + if f.Rule != "suppression-added" || f.Severity != MustFix || f.Job != "static" { + t.Errorf("got %+v", f) + } + if !strings.Contains(f.Message, "ico.go:2 (cannot-fail)") { + t.Errorf("got %q", f.Message) + } + // A placeholder in the prose that documents the mechanism is not a + // suppression being added. + if strings.Contains(f.Message, "<rule>") { + t.Errorf("counted a documented placeholder: %q", f.Message) + } + // The check polices the ignoring, so it cannot be ignored in turn. + if f.File != "" { + t.Errorf("got file %q", f.File) + } + if _, ok := Suppressed(f); ok { + t.Error("a suppression-added finding dismissed itself") + } +} + +// A dismissal can only be written where a dismissal works; the prose that +// documents the mechanism is not one. +func TestSuppressionSkipsProse(t *testing.T) { + diff := `--- a/readme.md ++++ b/readme.md +@@ -1,3 +1,4 @@ + # review ++` + "`//review:ignore <rule> <why>`" + ` + done +` + if findings := checkSuppressionAdded(&Change{Diff: diff}); len(findings) != 0 { + t.Fatalf("got %+v", findings) + } + // The zero means nothing unless the check still finds a real one: the + // same comment in a file the readers read is a suppression. + live := `--- a/ico.go ++++ b/ico.go +@@ -1,2 +1,3 @@ + package ico ++//` + `review:ignore cannot-fail the test can fail + func f() {} +` + if findings := checkSuppressionAdded(&Change{Diff: live}); len(findings) != 1 { + t.Fatalf("got %+v", findings) + } +} + +func TestDeletedTests(t *testing.T) { + diff := `--- a/ico_test.go ++++ b/ico_test.go +@@ -1,5 +1,4 @@ + package ico +-func TestAssemble(t *testing.T) {} +-func TestWrite(t *testing.T) {} ++func TestAssembleIcons(t *testing.T) {} + func f() {} +` + findings := checkDeletedTests(&Change{Diff: diff}) + if len(findings) != 1 { + t.Fatalf("got %+v", findings) + } + f := findings[0] + if f.Rule != "test-deleted" || f.Severity != MustFix || f.Job != "static" { + t.Errorf("got %+v", f) + } + if f.File != "ico_test.go" { + t.Errorf("got file %q", f.File) + } + if !strings.Contains(f.Message, "TestWrite") { + t.Errorf("the deleted test is not named: %q", f.Message) + } + if strings.Contains(f.Message, "TestAssemble") { + t.Errorf("the rename was counted as a deletion: %q", f.Message) + } + if !strings.Contains(f.Fix, "test-deleted") { + t.Errorf("the fix does not say how to answer it: %q", f.Fix) + } + // A deleted test is the reviewer's decision, so the finding is + // dismissible where a reader can read it. + if _, ok := Suppressed(f); ok { + t.Error("a test-deleted finding dismissed nothing") + } +} + +func TestDeletedJSTests(t *testing.T) { + diff := `--- a/web/app.test.ts ++++ b/web/app.test.ts +@@ -1,4 +1 @@ + import { it } from "testing"; +-it.skip("parses icons", () => {}); +-describe("loads", () => {}); ++export {}; +` + findings := checkDeletedTests(&Change{Diff: diff}) + if len(findings) != 1 { + t.Fatalf("got %+v", findings) + } + for _, want := range []string{"parses icons", "loads"} { + if !strings.Contains(findings[0].Message, want) { + t.Errorf("%q missing from %q", want, findings[0].Message) + } + } +} + +// A test in a file the runners do not read is not a test going missing; +// it is a function that stopped existing, which is somebody else's subject. +func TestDeletedFunctionsOutsideTestsAreNotTests(t *testing.T) { + diff := `--- a/ico.go ++++ b/ico.go +@@ -1,3 +1,2 @@ + package ico +-func TestWrite(w io.Writer) {} +-func helper() {} ++func helper() {} +` + if findings := checkDeletedTests(&Change{Diff: diff}); len(findings) != 0 { + t.Fatalf("got %+v", findings) + } + // The zero means nothing unless the same shape inside a test file is + // still heard: the same deletion there is a test going missing. + inTests := `--- a/ico_test.go ++++ b/ico_test.go +@@ -1,3 +1,2 @@ + package ico +-func TestWrite(t *testing.T) {} +-func helper() {} ++func helper() {} +` + if findings := checkDeletedTests(&Change{Diff: inTests}); len(findings) != 1 { + t.Fatalf("got %+v", findings) + } +} + +func TestCoveredSparesARename(t *testing.T) { + if !covered("TestParseIcons", []string{"TestParseIconsV2", "TestWrite"}) { + t.Error("a rename was read as a deletion") + } + if covered("TestParseIcons", []string{"TestParse", "TestWrite"}) { + t.Error("a different test was read as the same one") + } + // A name with no words of its own cannot be matched safely, so it is + // never spared. + if covered("TestV2", []string{"TestV2"}) { + t.Error("covered a name it cannot read") + } +} + +func TestFilePredicates(t *testing.T) { + for _, path := range []string{"ico.go", "web/app.ts", "main.rs", "makefile"} { + if !isCodeFile(path) { + t.Errorf("isCodeFile(%q) = false", path) + } + } + for _, path := range []string{"readme.md", ".gitignore", "LICENSE"} { + if isCodeFile(path) { + t.Errorf("isCodeFile(%q) = true", path) + } + } + for _, path := range []string{"ico_test.go", "web/app.test.ts", "web/app.spec.js"} { + if !isTestFile(path) { + t.Errorf("isTestFile(%q) = false", path) + } + } + for _, path := range []string{"ico.go", "web/app.ts"} { + if isTestFile(path) { + t.Errorf("isTestFile(%q) = true", path) + } + } +} diff --git a/gofrontend.go b/gofrontend.go @@ -0,0 +1,197 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "slices" + "strings" +) + +// GoFrontend reads Go through the standard library's parser, which is the +// grammar Go ships with itself. +type GoFrontend struct{} + +func (GoFrontend) Name() string { return "go" } + +func (GoFrontend) Covers(path string) bool { return strings.HasSuffix(path, ".go") } + +func (GoFrontend) Features() Features { + return FeatSymbols | FeatTests | FeatComments | FeatIndex +} + +// Change appends the declarations, tests and prose the added lines of the +// change's Go files introduce. Only what the diff added is reported, so a job +// sees new work rather than the file it landed in. +func (g GoFrontend) Change(root, rev string, c *Change, added map[string][]int) error { + for _, name := range c.Files { + if !g.Covers(name) { + continue + } + source, err := at(root, rev, name) + if err != nil { + continue // Deleted by the change, so there is nothing to read. + } + path := filepath.Join(root, name) + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, path, source, parser.ParseComments) + if err != nil { + continue + } + lines := strings.Split(string(source), "\n") + touched := added[name] + + ast.Inspect(file, func(n ast.Node) bool { + switch decl := n.(type) { + case *ast.FuncDecl: + line := fset.Position(decl.Pos()).Line + end := fset.Position(decl.End()).Line + name := decl.Name.Name + // A test is judged by what it asserts, so a change anywhere + // inside one is a change to the test. + if isTest(name) { + if !touchedBetween(touched, line, end) { + return true + } + c.Tests = append(c.Tests, Function{ + Name: name, File: relative(root, path), Line: line, + Body: text(lines, line, end), + }) + return true + } + // A name is new when its declaration is. Work inside a body + // changes what a function does, not what it is called. + if !slices.Contains(touched, line) { + return true + } + c.Symbols = append(c.Symbols, Symbol{ + Name: name, Kind: "func", Doc: doc(decl.Doc), + File: relative(root, path), Line: line, + Exported: ast.IsExported(name), Signature: strings.TrimSpace(lines[line-1]), + }) + case *ast.TypeSpec: + line := fset.Position(decl.Pos()).Line + if !slices.Contains(touched, line) { + return true + } + c.Symbols = append(c.Symbols, Symbol{ + Name: decl.Name.Name, Kind: "type", File: relative(root, path), Line: line, + Exported: ast.IsExported(decl.Name.Name), Signature: strings.TrimSpace(lines[line-1]), + }) + case *ast.ValueSpec: + for _, ident := range decl.Names { + line := fset.Position(ident.Pos()).Line + if !slices.Contains(touched, line) { + continue + } + c.Symbols = append(c.Symbols, Symbol{ + Name: ident.Name, Kind: "value", File: relative(root, path), Line: line, + Exported: ast.IsExported(ident.Name), Signature: strings.TrimSpace(lines[line-1]), + }) + } + } + return true + }) + + for _, group := range file.Comments { + for _, comment := range group.List { + line := fset.Position(comment.Pos()).Line + if !slices.Contains(touched, line) { + continue + } + c.Comments = append(c.Comments, Located{ + Text: strings.TrimSpace(strings.TrimPrefix(comment.Text, "//")), + File: relative(root, path), Line: line, + }) + } + } + } + return nil +} + +// Whole reads every declaration in the repository. It is built by parsing +// rather than by searching: a pattern over lines misses an indented constant +// inside a block, which is exactly where a duplicated fact tends to live. +func (GoFrontend) Whole(root, rev string) ([]Declared, error) { + out, err := git(root, "ls-files", "*.go") + if err != nil { + return nil, err + } + var index []Declared + for _, name := range strings.Split(strings.TrimSpace(out), "\n") { + if name == "" || strings.HasSuffix(name, "_test.go") { + continue + } + source, err := at(root, rev, name) + if err != nil { + continue + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, name, source, 0) + if err != nil { + continue + } + lines := strings.Split(string(source), "\n") + at := func(pos token.Pos) (int, string) { + line := fset.Position(pos).Line + if line-1 < 0 || line-1 >= len(lines) { + return line, "" + } + return line, lines[line-1] + } + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + line, text := at(d.Pos()) + index = append(index, Declared{Name: d.Name.Name, Kind: "func", File: name, Line: line, Text: text}) + case *ast.GenDecl: + for _, spec := range d.Specs { + switch s := spec.(type) { + case *ast.TypeSpec: + line, text := at(s.Pos()) + index = append(index, Declared{Name: s.Name.Name, Kind: "type", File: name, Line: line, Text: text}) + // A struct's fields are declarations too, and a field + // is where a restated fact often sits. + if structure, ok := s.Type.(*ast.StructType); ok { + for _, field := range structure.Fields.List { + for _, ident := range field.Names { + line, text := at(ident.Pos()) + index = append(index, Declared{Name: ident.Name, Kind: "field", File: name, Line: line, Text: text}) + } + } + } + case *ast.ValueSpec: + for _, ident := range s.Names { + line, text := at(ident.Pos()) + index = append(index, Declared{Name: ident.Name, Kind: kindOf(d.Tok), File: name, Line: line, Text: text}) + } + } + } + } + } + } + return index, nil +} + +// isTest reports whether a function is one the testing package runs. +func isTest(name string) bool { + return strings.HasPrefix(name, "Test") || strings.HasPrefix(name, "Fuzz") || strings.HasPrefix(name, "Benchmark") +} + +func kindOf(tok token.Token) string { + switch tok { + case token.CONST: + return "const" + case token.VAR: + return "var" + } + return "value" +} + +func doc(group *ast.CommentGroup) string { + if group == nil { + return "" + } + return strings.TrimSpace(group.Text()) +} diff --git a/helper_test.go b/helper_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// repo builds a git repository in a temporary directory, so that the parts of +// this tool that read a revision can be tested against real git output rather +// than a transcript of it. +type repo struct { + t *testing.T + Root string +} + +func newRepo(t *testing.T) *repo { + t.Helper() + r := &repo{t: t, Root: t.TempDir()} + r.run("init", "-q", "-b", "main") + r.run("config", "user.email", "test@example.com") + r.run("config", "user.name", "Test") + r.run("config", "commit.gpgsign", "false") + return r +} + +func (r *repo) run(args ...string) string { + r.t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = r.Root + cmd.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null") + out, err := cmd.CombinedOutput() + if err != nil { + r.t.Fatalf("git %s: %v\n%s", strings.Join(args, " "), err, out) + } + return string(out) +} + +// write puts a file in the working tree, creating the directories it needs. +func (r *repo) write(path, content string) { + r.t.Helper() + full := filepath.Join(r.Root, path) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + r.t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0o644); err != nil { + r.t.Fatal(err) + } +} + +// commit stages the named paths and records them. +func (r *repo) commit(message string, paths ...string) string { + r.t.Helper() + r.run(append([]string{"add", "--"}, paths...)...) + r.run("commit", "-q", "-m", message) + return strings.TrimSpace(r.run("rev-parse", "HEAD")) +} + +// stage adds paths to the index without committing, which is what a bare +// review reads. +func (r *repo) stage(paths ...string) { + r.t.Helper() + r.run(append([]string{"add", "--"}, paths...)...) +} diff --git a/heuristic.go b/heuristic.go @@ -0,0 +1,116 @@ +package main + +import ( + "strings" +) + +// Heuristic reads the languages no parser frontend covers: the comment lines +// a change adds, by the prefixes those languages share. It is deliberately +// line-shaped, so it is right about prose and silent about declarations — +// a pattern over lines missing an indented constant inside a block is the +// mistake that built the real frontends. +type Heuristic struct{} + +func (Heuristic) Name() string { return "heuristic" } + +// Covers reports the languages whose comment shape it knows. Prose-only +// formats (markdown, configs) are left out: their text is not a comment, and +// the claims job would be asked to judge ordinary writing. +func (Heuristic) Covers(path string) bool { + for _, ext := range []string{ + ".odin", ".py", ".rb", ".rs", ".c", ".h", ".cc", ".cpp", ".hpp", + ".js", ".mjs", ".cjs", ".lua", ".zig", ".swift", ".kt", ".java", + ".php", ".scala", ".cs", + } { + if strings.HasSuffix(path, ext) { + return true + } + } + return false +} + +// Features is comments only. The jobs that need declarations or test bodies +// are skipped with a logged reason rather than served a guess. +func (Heuristic) Features() Features { return FeatComments } + +// Change appends the comment lines the added lines introduce, keeping block +// comments whole across their lines. +func (g Heuristic) Change(root, rev string, c *Change, added map[string][]int) error { + for _, name := range c.Files { + if !g.Covers(name) { + continue + } + source, err := at(root, rev, name) + if err != nil { + continue // Deleted by the change, so there is nothing to read. + } + c.Comments = append(c.Comments, commentProse(source, name, added[name])...) + } + return nil +} + +// commentProse lifts the comment lines among the added ones out of a source +// file by the prefixes the C-family shares: // and # for line comments, an +// open /* kept whole until its close. The shebang is not a comment. +func commentProse(source []byte, name string, touched []int) []Located { + lines := strings.Split(string(source), "\n") + if len(touched) == 0 { + return nil + } + isTouched := map[int]bool{} + for _, line := range touched { + isTouched[line] = true + } + var out []Located + inBlock := false + for i, line := range lines { + number := i + 1 + trimmed := strings.TrimSpace(line) + switch { + case inBlock: + if isTouched[number] { + t := strings.TrimSuffix(trimmed, "*/") + t = strings.TrimSpace(strings.TrimPrefix(t, "*")) + if t != "" { + out = append(out, Located{Text: t, File: name, Line: number}) + } + } + if strings.Contains(trimmed, "*/") { + inBlock = false + } + case strings.HasPrefix(trimmed, "/*"): + t := trimmed[2:] + if end := strings.Index(t, "*/"); end >= 0 { + t = t[:end] + if isTouched[number] && strings.TrimSpace(t) != "" { + out = append(out, Located{Text: strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(t), "*")), File: name, Line: number}) + } + } else { + if isTouched[number] && strings.TrimSpace(t) != "" { + out = append(out, Located{Text: strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(t), "*")), File: name, Line: number}) + } + inBlock = true + } + case strings.HasPrefix(trimmed, "//"): + if !isTouched[number] { + continue + } + out = append(out, Located{ + Text: strings.TrimSpace(strings.TrimPrefix(trimmed, "//")), + File: name, Line: number, + }) + case strings.HasPrefix(trimmed, "#") && !strings.HasPrefix(trimmed, "#!"): + if !isTouched[number] { + continue + } + out = append(out, Located{ + Text: strings.TrimSpace(strings.TrimPrefix(trimmed, "#")), + File: name, Line: number, + }) + } + } + return out +} + +// Whole is never asked: the heuristic declares no index. +func (Heuristic) Whole(root, rev string) ([]Declared, error) { return nil, nil } diff --git a/index.go b/index.go @@ -2,9 +2,6 @@ package main import ( "fmt" - "go/ast" - "go/parser" - "go/token" "strings" ) @@ -24,80 +21,6 @@ func (d Declared) String() string { return fmt.Sprintf("%s:%d: %s", d.File, d.Line, strings.TrimSpace(d.Text)) } -// Index reads every declaration in the repository. It is built by parsing -// rather than by searching: a pattern over lines misses an indented constant -// inside a block, which is exactly where a duplicated fact tends to live. -func Index(root, rev string) ([]Declared, error) { - out, err := git(root, "ls-files", "*.go") - if err != nil { - return nil, err - } - var index []Declared - for _, name := range strings.Split(strings.TrimSpace(out), "\n") { - if name == "" || strings.HasSuffix(name, "_test.go") { - continue - } - source, err := at(root, rev, name) - if err != nil { - continue - } - fset := token.NewFileSet() - file, err := parser.ParseFile(fset, name, source, 0) - if err != nil { - continue - } - lines := strings.Split(string(source), "\n") - at := func(pos token.Pos) (int, string) { - line := fset.Position(pos).Line - if line-1 < 0 || line-1 >= len(lines) { - return line, "" - } - return line, lines[line-1] - } - for _, decl := range file.Decls { - switch d := decl.(type) { - case *ast.FuncDecl: - line, text := at(d.Pos()) - index = append(index, Declared{Name: d.Name.Name, Kind: "func", File: name, Line: line, Text: text}) - case *ast.GenDecl: - for _, spec := range d.Specs { - switch s := spec.(type) { - case *ast.TypeSpec: - line, text := at(s.Pos()) - index = append(index, Declared{Name: s.Name.Name, Kind: "type", File: name, Line: line, Text: text}) - // A struct's fields are declarations too, and a field - // is where a restated fact often sits. - if structure, ok := s.Type.(*ast.StructType); ok { - for _, field := range structure.Fields.List { - for _, ident := range field.Names { - line, text := at(ident.Pos()) - index = append(index, Declared{Name: ident.Name, Kind: "field", File: name, Line: line, Text: text}) - } - } - } - case *ast.ValueSpec: - for _, ident := range s.Names { - line, text := at(ident.Pos()) - index = append(index, Declared{Name: ident.Name, Kind: kindOf(d.Tok), File: name, Line: line, Text: text}) - } - } - } - } - } - } - return index, nil -} - -func kindOf(tok token.Token) string { - switch tok { - case token.CONST: - return "const" - case token.VAR: - return "var" - } - return "value" -} - // Resembling returns the declarations whose names share a word with the one // given, which is the shortlist a reader is asked to judge. Its own // declaration is left out, since a name always resembles itself. @@ -135,11 +58,23 @@ func Resembling(index []Declared, symbol Symbol, limit int) []string { // cheapest signal that a fact has been written twice. It is a hint for the // shortlist, not a finding. func same(a, b Declared) bool { - _, left, okA := strings.Cut(a.Text, "=") - _, right, okB := strings.Cut(b.Text, "=") + _, left, okA := cutLiteral(a.Text) + _, right, okB := cutLiteral(b.Text) return okA && okB && strings.TrimSpace(left) == strings.TrimSpace(right) } +// cutLiteral takes the value half of a declaration line. Go and TypeScript +// write `name = value`; Odin writes `name :: value`. +func cutLiteral(text string) (string, string, bool) { + if _, right, ok := strings.Cut(text, "="); ok { + return text, right, true + } + if _, right, ok := strings.Cut(text, "::"); ok { + return text, right, true + } + return text, "", false +} + // split breaks a name into the words it is made of, so that Assemble finds // assemble and icoEntrySize finds both Entry and Size. func split(name string) []string { @@ -148,9 +83,17 @@ func split(name string) []string { word strings.Builder ) for i, r := range name { - if i > 0 && r >= 'A' && r <= 'Z' { - words = append(words, word.String()) - word.Reset() + // A capital starts a word; an underscore ends one, as snake_case + // languages put the next word after it. + if i > 0 && (r >= 'A' && r <= 'Z' || r == '_') { + if word.Len() > 0 { + words = append(words, word.String()) + word.Reset() + } + if r != '_' { + word.WriteRune(r) + } + continue } word.WriteRune(r) } diff --git a/index_test.go b/index_test.go @@ -0,0 +1,178 @@ +package main + +import ( + "reflect" + "strings" + "testing" +) + +func TestSplit(t *testing.T) { + for _, test := range []struct { + name string + want []string + }{ + {"Assemble", []string{"Assemble"}}, + {"icoEntrySize", []string{"ico", "Entry", "Size"}}, + {"groupHeaderSize", []string{"group", "Header", "Size"}}, + {"Stored", []string{"Stored"}}, + {"", nil}, + {"x", []string{"x"}}, + // An acronym splits per letter. Words shorter than four characters + // are dropped by the caller, so this costs nothing. + {"ID", []string{"I", "D"}}, + } { + if got := split(test.name); !reflect.DeepEqual(got, test.want) { + t.Errorf("split(%q) = %v, want %v", test.name, got, test.want) + } + } +} + +func TestSame(t *testing.T) { + for _, test := range []struct { + left, right string + want bool + }{ + {"\tgroupHeaderSize = 6", "\tdirectorySize = 6", true}, + {"const icoEntrySize = 16", "\tentrySize = 16", true}, + {"const a = 6", "const b = 7", false}, + {"const a = 6", "type T struct{}", false}, + {"type T struct{}", "type U struct{}", false}, + {"x := 6", "const y = 6", true}, + {"\tmagic = \"PE\\x00\\x00\"", "\theader = \"PE\\x00\\x00\"", true}, + } { + got := same(Declared{Text: test.left}, Declared{Text: test.right}) + if got != test.want { + t.Errorf("same(%q, %q) = %v, want %v", test.left, test.right, got, test.want) + } + } +} + +func TestIndex(t *testing.T) { + r := newRepo(t) + r.write("ico/writer.go", `package ico + +const ( + directorySize = 6 + entrySize = 16 +) + +type Entry struct { + Width int + Offset int +} + +func Write() {} + +var ErrNoIcons = errorString("none") +`) + // Tests are not part of the index: a duplicate in a test is not a fact + // with two owners. + r.write("ico/writer_test.go", "package ico\n\nconst directorySize = 99\n") + rev := r.commit("first", "ico/writer.go", "ico/writer_test.go") + + index, err := GoFrontend{}.Whole(r.Root, rev+".."+rev) + if err != nil { + t.Fatal(err) + } + kinds := map[string]string{} + for _, d := range index { + if kinds[d.Name] != "" { + t.Errorf("%s indexed twice", d.Name) + } + kinds[d.Name] = d.Kind + } + want := map[string]string{ + "directorySize": "const", + "entrySize": "const", + "Entry": "type", + "Width": "field", + "Offset": "field", + "Write": "func", + "ErrNoIcons": "var", + } + if !reflect.DeepEqual(kinds, want) { + t.Errorf("got %v, want %v", kinds, want) + } + + // An indented constant inside a block is exactly where a duplicated fact + // lives, so its declaring line has to survive into the index. + for _, d := range index { + if d.Name == "entrySize" { + if !strings.Contains(d.Text, "16") { + t.Errorf("entrySize carries no value: %q", d.Text) + } + if d.File != "ico/writer.go" || d.Line != 5 { + t.Errorf("entrySize located at %s:%d, want ico/writer.go:5", d.File, d.Line) + } + } + } +} + +func TestIndexSkipsUnparseableFiles(t *testing.T) { + r := newRepo(t) + r.write("good.go", "package x\n\nconst a = 1\n") + r.write("bad.go", "package x\n\nfunc (\n") + rev := r.commit("first", "good.go", "bad.go") + + index, err := GoFrontend{}.Whole(r.Root, rev+".."+rev) + if err != nil { + t.Fatal(err) + } + if len(index) != 1 || index[0].Name != "a" { + t.Errorf("got %v, want only a", index) + } +} + +func TestResembling(t *testing.T) { + index := []Declared{ + {Name: "entrySize", File: "ico/writer.go", Line: 5, Text: "\tentrySize = 16"}, + {Name: "directorySize", File: "ico/writer.go", Line: 4, Text: "\tdirectorySize = 6"}, + {Name: "Entry", File: "ico/ico.go", Line: 10, Text: "type Entry struct {"}, + {Name: "colour", File: "png/png.go", Line: 3, Text: "var colour int"}, + } + symbol := Symbol{Name: "icoEntrySize", File: "exe/exe.go", Line: 12} + + got := Resembling(index, symbol, 16) + joined := strings.Join(got, "\n") + for _, want := range []string{"entrySize", "Entry", "directorySize"} { + if !strings.Contains(joined, want) { + t.Errorf("%s missing from the shortlist:\n%s", want, joined) + } + } + if strings.Contains(joined, "colour") { + t.Errorf("an unrelated name reached the shortlist:\n%s", joined) + } + // "ico" is three letters, so it contributes nothing and cannot drag in + // every declaration in the ico package. + if len(got) != 3 { + t.Errorf("got %d candidates, want 3:\n%s", len(got), joined) + } +} + +func TestResemblingLeavesOutItself(t *testing.T) { + index := []Declared{ + {Name: "entrySize", File: "exe/exe.go", Line: 12, Text: "\tentrySize = 16"}, + {Name: "entrySize", File: "ico/ico.go", Line: 4, Text: "\tentrySize = 16"}, + } + got := Resembling(index, Symbol{Name: "entrySize", File: "exe/exe.go", Line: 12}, 16) + if len(got) != 1 || !strings.Contains(got[0], "ico/ico.go") { + t.Errorf("got %v, want only the declaration elsewhere", got) + } +} + +func TestResemblingObeysItsLimit(t *testing.T) { + var index []Declared + for i := range 40 { + index = append(index, Declared{Name: "entrySize", File: "x.go", Line: i + 1, Text: "entrySize = 1"}) + } + if got := Resembling(index, Symbol{Name: "entrySize", File: "y.go"}, 16); len(got) != 16 { + t.Errorf("got %d candidates, want the limit of 16", len(got)) + } +} + +func TestDeclaredString(t *testing.T) { + d := Declared{File: "ico/writer.go", Line: 4, Text: "\tdirectorySize = 6\t"} + if got := d.String(); got != "ico/writer.go:4: directorySize = 6" { + t.Errorf("got %q", got) + } +} diff --git a/job.go b/job.go @@ -75,6 +75,14 @@ func duplicationSubject(c *Change) string { return "" } var b strings.Builder + // The pairs holding the same literal go first and alone. Buried among + // the resemblances a reading finds one of them and stops. + if twinned := twins(c); twinned != "" { + b.WriteString("Declarations this change adds that hold a value already declared elsewhere.\n") + b.WriteString("Judge every pair on this list.\n\n") + b.WriteString(twinned) + b.WriteString("\n") + } b.WriteString("Each name the change adds, with existing declarations found by searching for its words.\n\n") for _, s := range c.Symbols { fmt.Fprintf(&b, "NEW %s:%d %s %s\n", s.File, s.Line, s.Kind, s.Name) @@ -98,6 +106,23 @@ func duplicationSubject(c *Change) string { return b.String() } +// twins renders the declarations whose value already exists, in the order the +// change declares them. +func twins(c *Change) string { + var b strings.Builder + for _, s := range c.Symbols { + lines := c.Twins[s.Name] + if len(lines) == 0 { + continue + } + fmt.Fprintf(&b, " %s:%d %s\n", s.File, s.Line, s.Signature) + for _, line := range lines { + fmt.Fprintf(&b, " %s\n", strings.TrimSuffix(line, " <- same value")) + } + } + return b.String() +} + func testsSubject(c *Change) string { if len(c.Tests) == 0 { return "" diff --git a/job_test.go b/job_test.go @@ -0,0 +1,397 @@ +package main + +import ( + "context" + "fmt" + "strings" + "testing" +) + +func TestRules(t *testing.T) { + criteria := "# Naming\n\n" + + "- `noun-for-type` — a type is a noun.\n" + + "- `verb-for-func` — a function is a verb.\n" + + " - `not-a-rule` — indented, so not a rule id.\n" + + "- not a rule at all\n" + + "- `Shouty` — capitals are not rule ids.\n" + got := rules(criteria) + want := map[string]bool{"noun-for-type": true, "verb-for-func": true} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for id := range want { + if !got[id] { + t.Errorf("%s missing from %v", id, got) + } + } +} + +// Every job's criteria have to define at least one citable id, or the job can +// report nothing that survives filtering. +func TestEveryJobDefinesRules(t *testing.T) { + for _, job := range Jobs() { + ids := rules(job.Criteria) + if len(ids) == 0 { + t.Errorf("job %q defines no rule ids", job.Name) + } + for id := range ids { + if strings.TrimSpace(id) == "" { + t.Errorf("job %q defines an empty rule id", job.Name) + } + } + } +} + +func TestReportedFindingsDropsAnUnknownRule(t *testing.T) { + r, err := decode(`{"findings":[ + {"rule":"cannot-fail","severity":"must-fix","file":"x_test.go","line":4,"message":"kept","fix":"f"}, + {"rule":"invented-by-the-model","severity":"must-fix","message":"dropped"}, + {"rule":"","severity":"note","message":"also dropped"} + ]}`) + if err != nil { + t.Fatal(err) + } + got := r.findings("tests", map[string]bool{"cannot-fail": true}) + if len(got) != 1 { + t.Fatalf("got %d findings, want only the one citing a known rule: %v", len(got), got) + } + if got[0].Job != "tests" || got[0].Rule != "cannot-fail" || got[0].Severity != MustFix { + t.Errorf("got %+v", got[0]) + } + if got[0].SeverityName != "must-fix" { + t.Errorf("the severity as reported is kept for JSON, got %q", got[0].SeverityName) + } +} + +func TestDecode(t *testing.T) { + r, err := decode(`{"findings":[{"rule":"cannot-fail","severity":"note","file":"x.go","line":3,"message":"m","fix":"f"}]}`) + if err != nil { + t.Fatal(err) + } + if len(r.Findings) != 1 || r.Findings[0].Line != 3 { + t.Fatalf("got %+v", r.Findings) + } + if _, err := decode(`not json`); err == nil { + t.Error("decoding rubbish should fail") + } + // An empty list is how a job reports nothing, and must not be an error. + if _, err := decode(`{"findings":[]}`); err != nil { + t.Errorf("an empty report is valid: %v", err) + } +} + +func TestChosen(t *testing.T) { + all, err := chosen("") + if err != nil { + t.Fatal(err) + } + if len(all) != len(Jobs()) { + t.Errorf("got %d jobs, want all of them", len(all)) + } + some, err := chosen("namer, tests") + if err != nil { + t.Fatal(err) + } + if len(some) != 2 || some[0].Name != "namer" || some[1].Name != "tests" { + t.Errorf("got %v", some) + } + if _, err := chosen("nonsense"); err == nil { + t.Error("an unknown job should be named as such") + } +} + +// A job with nothing to read renders nothing, which is what keeps a +// documentation-only commit from paying for a test review. +func TestSubjectsAreEmptyWithNothingToRead(t *testing.T) { + empty := &Change{Candidates: map[string][]string{}} + for _, job := range Jobs() { + if got := job.Subject(empty); strings.TrimSpace(got) != "" { + t.Errorf("job %q renders %q for an empty change", job.Name, got) + } + } +} + +func TestSubjectsRenderWhatTheirJobReads(t *testing.T) { + change := &Change{ + Symbols: []Symbol{{ + Name: "Stored", Kind: "type", File: "ico/ico.go", Line: 12, + Exported: true, Signature: "type Stored struct {", Doc: "Stored is an icon already encoded.", + }}, + Tests: []Function{{Name: "TestWrite", File: "ico/w_test.go", Line: 3, Body: "func TestWrite(t *testing.T) {\n\tt.Skip()\n}"}}, + Comments: []Located{{Text: "Windows rejects a 256 pixel icon.", File: "exe/exe.go", Line: 40}}, + Message: "ico: assemble a file", + Stat: " ico/ico.go | 3 +++", + Convention: []string{"ico: read a directory", "exe: carry icons"}, + Candidates: map[string][]string{"Stored": {"ico/ico.go:4: type Encoded struct {"}}, + } + for _, test := range []struct { + job string + want []string + gone []string + }{ + {"namer", []string{"Stored", "(exported)", "type Stored struct {", "doc:", "type Encoded struct {"}, []string{"TestWrite", "Windows rejects"}}, + {"duplication", []string{"NEW", "Stored", "candidates:", "type Encoded struct {"}, []string{"TestWrite"}}, + {"tests", []string{"TestWrite", "t.Skip()"}, []string{"Stored"}}, + {"claims", []string{"Windows rejects a 256 pixel icon.", "exe/exe.go:40"}, []string{"TestWrite"}}, + {"hygiene", []string{"ico: assemble a file", "ico/ico.go | 3 +++", "ico: read a directory"}, []string{"TestWrite"}}, + } { + t.Run(test.job, func(t *testing.T) { + jobs, err := chosen(test.job) + if err != nil { + t.Fatal(err) + } + got := jobs[0].Subject(change) + for _, want := range test.want { + if !strings.Contains(got, want) { + t.Errorf("%q missing from:\n%s", want, got) + } + } + for _, gone := range test.gone { + if strings.Contains(got, gone) { + t.Errorf("%q leaked into another job's subject:\n%s", gone, got) + } + } + }) + } +} + +func TestDuplicationSubjectSaysWhenThereAreNoCandidates(t *testing.T) { + change := &Change{ + Symbols: []Symbol{{Name: "Alone", Kind: "type", File: "x.go", Line: 1}}, + Candidates: map[string][]string{}, + } + if got := duplicationSubject(change); !strings.Contains(got, "candidates: none found") { + t.Errorf("got:\n%s", got) + } +} + +func TestFirstLine(t *testing.T) { + if got := firstLine("one\ntwo"); got != "one" { + t.Errorf("got %q", got) + } + long := strings.Repeat("x", 200) + got := firstLine(long) + if len([]rune(got)) != 141 || !strings.HasSuffix(got, "…") { + t.Errorf("a long line is cut, got %d runes", len([]rune(got))) + } +} + +func TestShort(t *testing.T) { + lines := []string{"a", "b", "c"} + if got := short(lines, 2); len(got) != 2 { + t.Errorf("got %v", got) + } + if got := short(lines, 9); len(got) != 3 { + t.Errorf("got %v", got) + } +} + +// still is a provider that answers with whatever it was handed, so the whole +// path from an answer to a finding can be walked without a model. +type still struct { + answer string + system string + user string +} + +func (s *still) Name() string { return "still" } + +func (s *still) Ask(ctx context.Context, system, user string) (Answer, error) { + s.system, s.user = system, user + return Answer{Text: s.answer}, nil +} + +func TestReviewerReadsAnAnswer(t *testing.T) { + provider := &still{answer: "Sure! ```json\n{\"findings\":[" + + `{"rule":"cannot-fail","severity":"must-fix","file":"x_test.go","line":4,"message":"m","fix":"f"},` + + `{"rule":"made-up","severity":"must-fix","message":"dropped"}` + + "]}\n```"} + change := &Change{Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider}.Run(context.Background(), change, jobs) + if len(result.Failures) != 0 { + t.Fatalf("failures: %v", result.Failures) + } + if len(result.Findings) != 1 || result.Findings[0].Rule != "cannot-fail" { + t.Fatalf("got %v", result.Findings) + } + // The criteria are the system prompt, where a provider that caches + // anything will cache them. + if !strings.Contains(provider.system, instruction) || !strings.Contains(provider.system, "cannot-fail") { + t.Errorf("the system prompt is missing the instruction or the criteria:\n%s", provider.system) + } + if !strings.Contains(provider.user, "TestX") { + t.Errorf("the subject never reached the provider:\n%s", provider.user) + } +} + +func TestReviewerReportsABadAnswer(t *testing.T) { + change := &Change{Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: &still{answer: "I would rather not."}}.Run(context.Background(), change, jobs) + if len(result.Failures) != 1 { + t.Fatalf("got %v, want one failure", result.Failures) + } + if !strings.Contains(result.Failures[0].Error(), "tests:") { + t.Errorf("a failure names its job, got %q", result.Failures[0]) + } +} + +// One job failing does not stop the others: four readings out of five is +// worth more than none. +func TestReviewerKeepsGoingAfterAFailure(t *testing.T) { + change := &Change{ + Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}, + Comments: []Located{{Text: "Windows rejects this.", File: "x.go", Line: 1}}, + } + jobs, err := chosen("tests,claims") + if err != nil { + t.Fatal(err) + } + provider := &refusing{good: "claims"} + result := Reviewer{Provider: provider}.Run(context.Background(), change, jobs) + if len(result.Failures) != 1 { + t.Fatalf("failures: %v", result.Failures) + } + if len(result.Findings) != 1 || result.Findings[0].Job != "claims" { + t.Fatalf("got %v, want the reading that worked", result.Findings) + } +} + +// refusing answers only the job whose criteria it recognises. +type refusing struct{ good string } + +func (r *refusing) Name() string { return "refusing" } + +func (r *refusing) Ask(ctx context.Context, system, user string) (Answer, error) { + if r.good == "claims" && strings.Contains(system, "unsupported-claim") { + return Answer{Text: `{"findings":[{"rule":"unsupported-claim","severity":"note","file":"x.go","line":1,"message":"m","fix":"f"}]}`}, nil + } + return Answer{}, context.Canceled +} + +// A reading finds one same-value pair and stops when the pairs are buried +// among the resemblances, so they are rendered first and alone. +func TestDuplicationSubjectLeadsWithTheTwins(t *testing.T) { + change := &Change{ + Symbols: []Symbol{ + {Name: "typeIconGroup", Kind: "value", File: "exe/exe.go", Line: 32, Signature: "typeIconGroup = 14"}, + {Name: "groupHeaderSize", Kind: "value", File: "exe/exe.go", Line: 44, Signature: "groupHeaderSize = 6"}, + }, + Twins: map[string][]string{ + "typeIconGroup": {"exe/exe.go:47: groupEntrySize = 14 <- same value"}, + "groupHeaderSize": {"ico/writer.go:15: directorySize = 6 <- same value"}, + }, + Candidates: map[string][]string{ + "groupHeaderSize": {"ico/writer.go:15: directorySize = 6 <- same value", "exe/exe.go:53: type Group struct {"}, + }, + } + got := duplicationSubject(change) + list, detail, found := strings.Cut(got, "Each name the change adds") + if !found { + t.Fatalf("no detail section in:\n%s", got) + } + for _, want := range []string{"groupEntrySize = 14", "directorySize = 6", "exe/exe.go:44"} { + if !strings.Contains(list, want) { + t.Errorf("%q missing from the list of pairs:\n%s", want, list) + } + } + // The pairs are declared in the order the change declares them, so two + // readings of the same change see the same list. + if strings.Index(list, "typeIconGroup") > strings.Index(list, "groupHeaderSize") { + t.Errorf("the pairs are out of order:\n%s", list) + } + if !strings.Contains(detail, "type Group struct {") { + t.Errorf("the resemblances are gone:\n%s", detail) + } +} + +func TestDuplicationSubjectWithoutTwins(t *testing.T) { + change := &Change{ + Symbols: []Symbol{{Name: "Alone", Kind: "type", File: "x.go", Line: 1}}, + Twins: map[string][]string{}, + Candidates: map[string][]string{}, + } + if got := duplicationSubject(change); strings.Contains(got, "already declared elsewhere") { + t.Errorf("a change with no pairs is not asked about them:\n%s", got) + } +} + +// A provider that cannot be held to a schema sometimes describes its +// findings instead of reporting them. Losing the whole reading to that is +// worse than asking again. +func TestReviewerAsksAgainForAnAnswerItCanRead(t *testing.T) { + provider := &relenting{ + answers: []string{ + "Reported two findings: `Stored` (must-fix) and `largest` (consider).", + `{"findings":[{"rule":"cannot-fail","severity":"must-fix","file":"x_test.go","line":4,"message":"m","fix":"f"}]}`, + }, + } + change := &Change{Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider}.Run(context.Background(), change, jobs) + if len(result.Failures) != 0 { + t.Fatalf("failures: %v", result.Failures) + } + if len(result.Findings) != 1 || result.Findings[0].Rule != "cannot-fail" { + t.Fatalf("got %v", result.Findings) + } + if len(provider.asked) != 2 { + t.Fatalf("asked %d times, want two", len(provider.asked)) + } + if !strings.Contains(provider.asked[1], again) { + t.Errorf("the second question does not say what was wrong:\n%s", provider.asked[1]) + } + // The subject is asked again, not only the correction. + if !strings.Contains(provider.asked[1], "TestX") { + t.Errorf("the subject was dropped from the second question:\n%s", provider.asked[1]) + } +} + +// It is asked twice and no more: a provider that will not answer in JSON +// will not do so on the third try either. +func TestReviewerGivesUpAfterAskingTwice(t *testing.T) { + provider := &relenting{answers: []string{"no", "still no"}} + change := &Change{Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider}.Run(context.Background(), change, jobs) + if len(result.Failures) != 1 { + t.Fatalf("got %v, want one failure", result.Failures) + } + // The first answer is the one worth seeing: the second was made under + // duress. + if !strings.Contains(result.Failures[0].Error(), "no") || !strings.Contains(result.Failures[0].Error(), "twice") { + t.Errorf("got %q", result.Failures[0]) + } + if len(provider.asked) != 2 { + t.Errorf("asked %d times, want two", len(provider.asked)) + } +} + +// relenting answers each question with the next answer it was given. +type relenting struct { + answers []string + asked []string +} + +func (r *relenting) Name() string { return "relenting" } + +func (r *relenting) Ask(ctx context.Context, system, user string) (Answer, error) { + r.asked = append(r.asked, user) + if len(r.asked) > len(r.answers) { + return Answer{}, fmt.Errorf("asked once too often") + } + return Answer{Text: r.answers[len(r.asked)-1]}, nil +} diff --git a/main.go b/main.go @@ -5,7 +5,13 @@ // deterministic checks a repository already runs are what should do that. This // looks at the things those checks cannot — whether a name says what the thing // is, whether a fact is already stated elsewhere, whether a test can fail, -// whether a comment claims something nobody verified. +// whether a comment claims something nobody verified. A set of deterministic +// checks — the commit message, the repository's history, and staticcheck where +// it is installed — run before anything is asked. Each job's findings are put +// back to the provider once, against the same evidence, and the ones a second +// reading does not let stand are retracted rather than reported; what each ask +// answered is cached, so that a re-run of an unchanged part of the change +// replays it rather than asking again. // // review the staged change // review HEAD^ the last commit @@ -24,8 +30,6 @@ import ( "os" "os/exec" "strings" - - "github.com/anthropics/anthropic-sdk-go" ) func main() { @@ -37,15 +41,23 @@ func main() { func run() error { var ( - asJSON bool - verbose bool - show bool - only string + asJSON bool + verbose bool + show bool + only string + which string + model string + noVerify bool + fresh bool ) flag.BoolVar(&asJSON, "json", false, "Report findings as JSON, for an agent rather than a person.") flag.BoolVar(&verbose, "verbose", false, "Show what each job read and what it cost.") flag.BoolVar(&show, "show", false, "Print what each job would be sent and stop, without asking anything.") + flag.BoolVar(&noVerify, "no-verify", false, "Skip the second reading that checks what each job reported.") + flag.BoolVar(&fresh, "fresh", false, "Ask the provider even where the answer cache holds this exact question.") flag.StringVar(&only, "jobs", "", "Run only these jobs, comma separated.") + flag.StringVar(&which, "provider", orElse(os.Getenv("REVIEW_PROVIDER"), "chain"), "Who answers: chain (probe the default order), claude, pi, api, or command.") + flag.StringVar(&model, "model", os.Getenv("REVIEW_MODEL"), "Which model, in whatever form the provider names them.") flag.Usage = usage flag.Parse() @@ -58,11 +70,10 @@ func run() error { return err } if strings.TrimSpace(change.Diff) == "" { - if !asJSON { - fmt.Println("nothing to review") - } else { - fmt.Println(`{"findings":[]}`) + if asJSON { + return report(contract{Version: contractVersion, Status: "empty"}) } + fmt.Println("nothing to review") return nil } @@ -87,28 +98,158 @@ func run() error { len(change.Files), len(change.Symbols), len(change.Tests), len(change.Comments)) } - reviewer := Reviewer{Client: anthropic.NewClient(), Verbose: verbose} - findings, failures := reviewer.Run(context.Background(), change, jobs) + // The deterministic checks ask nothing of a provider, so they run before + // one is built and survive a model that cannot answer. + static := runChecks(change) + static = append(static, checkStaticcheck(root, flag.Arg(0), change)...) + + var provider Provider + var providerName string + if which == "chain" { + picked, name, err := defaultChain().pick(context.Background(), func(s string) { fmt.Fprintln(os.Stderr, s) }) + if err != nil { + return err + } + provider = picked + providerName = provider.Name() + if verbose { + fmt.Printf("asking %s\n", name) + } + } else { + build, known := Providers()[which] + if !known { + return fmt.Errorf("no provider called %q", which) + } + provider = build(model) + providerName = provider.Name() + if verbose { + fmt.Printf("asking %s\n", providerName) + } + } + // The cache records what each ask answered, so that a re-run of a part + // of the change that did not change replays it instead of asking. A + // cache that cannot exist is no fault of the review. + cache := openCache(fresh) + if cache != nil { + defer cache.save() + } + reviewer := Reviewer{Provider: provider, Verbose: verbose, Verify: !noVerify, Cache: cache} + result := reviewer.Run(context.Background(), change, jobs) + + // The deterministic checks are their own verification: what they report + // was measured, not read once. + for i := range static { + static[i].Verified = true + } + findings := append(result.Findings, static...) kept, dismissed := filter(root, findings) Sort(kept) + env := contract{ + Version: contractVersion, + Status: statusOf(change, result), + Provider: providerName, + Findings: kept, + Retracted: result.Retracted, + Failed: faults(result.Failures), + Skipped: result.Skipped, + Uncovered: change.Uncovered, + Dismissed: dismissed, + Truncated: change.Truncated, + Usage: metered{ + In: result.In, Out: result.Out, Cached: result.Cached, + Replayed: result.Replayed, Cost: result.Cost, + }, + } if asJSON { - return report(kept) + return report(env) } - render(kept, dismissed, failures, verbose) + render(env, result.Failures, verbose) return nil } -// filter drops the findings the source itself dismisses, and counts them so a -// silent dismissal is still visible. -func filter(root string, findings []Finding) (kept []Finding, dismissed int) { +// contractVersion is the shape of the JSON report, so that a program +// reading it can tell when the shape moves under it. +const contractVersion = 1 + +// JobFault is one job that could not finish, and what failed about it. +type JobFault struct { + Job string `json:"job"` + Error string `json:"error"` +} + +// metered is what the asks cost, in tokens and dollars. Answers replayed +// from the cache count as replayed rather than as usage: they cost nothing +// this run. +type metered struct { + In int `json:"in"` + Out int `json:"out"` + Cached int `json:"cached"` + Replayed int `json:"replayed"` + Cost float64 `json:"usd"` +} + +// contract is the whole report a program reads: what was found, what the +// second reading did not let stand, what was dismissed in the source, and +// how complete the measurement was. The status is what makes an empty +// findings list readable: "complete" is the only status an empty list can +// be read as a pass against, because the others say which part of the +// measurement is missing. +type contract struct { + Version int `json:"version"` + Status string `json:"status"` + Provider string `json:"provider,omitempty"` + Findings []Finding `json:"findings"` + Retracted []Retracted `json:"retracted"` + Failed []JobFault `json:"failed"` + Skipped []string `json:"skipped"` + Uncovered []Gap `json:"uncovered"` + Dismissed []Dismissed `json:"dismissed"` + Truncated bool `json:"truncated,omitempty"` + Usage metered `json:"usage"` +} + +// statusOf names how complete the measurement was. The parts of the change +// the deterministic side could not read and the jobs that failed are what +// keep an empty findings list from being read as a pass. +func statusOf(change *Change, result RunResult) string { + if len(result.Failures) > 0 || len(change.Uncovered) > 0 || change.Truncated { + return "incomplete" + } + return "complete" +} + +// faults pairs each failure's job with what failed about it, for the +// report. +func faults(failures []error) []JobFault { + var out []JobFault + for _, err := range failures { + job, cause, found := strings.Cut(err.Error(), ": ") + if !found { + out = append(out, JobFault{Job: err.Error(), Error: err.Error()}) + continue + } + out = append(out, JobFault{Job: job, Error: cause}) + } + return out +} + +// Dismissed is a finding the source itself answered, and the answer. +type Dismissed struct { + Finding Finding `json:"finding"` + Why string `json:"why"` +} + +// filter drops the findings the source itself dismisses, and keeps them so +// that a silent dismissal can still be read back. +func filter(root string, findings []Finding) (kept []Finding, dismissed []Dismissed) { if err := os.Chdir(root); err != nil { - return findings, 0 + return findings, nil } for _, f := range findings { - if _, ok := Suppressed(f); ok { - dismissed++ + if why, ok := Suppressed(f); ok { + dismissed = append(dismissed, Dismissed{Finding: f, Why: why}) continue } kept = append(kept, f) @@ -116,16 +257,30 @@ func filter(root string, findings []Finding) (kept []Finding, dismissed int) { return kept, dismissed } -func render(findings []Finding, dismissed int, failures []error, verbose bool) { +func render(env contract, failures []error, verbose bool) { for _, err := range failures { fmt.Fprintln(os.Stderr, " job failed:", err) } + // A dismissal reaches a few lines either side of itself, so which + // findings one answered is worth being able to read; a retraction is + // worth reading in full, because it is the second reading's word + // against the first's. + if verbose { + for _, d := range env.Dismissed { + fmt.Printf(" dismissed: %s:%d %s (%s)\n", d.Finding.File, d.Finding.Line, d.Finding.Rule, d.Why) + } + for _, r := range env.Retracted { + fmt.Printf(" retracted: [%s] %s:%d — %s\n", r.Finding.Rule, r.Finding.File, r.Finding.Line, first(r.Reason, 120)) + } + } + findings := env.Findings if len(findings) == 0 { fmt.Print("no findings") - if dismissed > 0 { - fmt.Printf(" (%d dismissed in the source)", dismissed) + if len(env.Dismissed) > 0 { + fmt.Printf(" (%d dismissed in the source)", len(env.Dismissed)) } fmt.Println() + retractedNote(env) return } var severity Severity = -1 @@ -137,28 +292,63 @@ func render(findings []Finding, dismissed int, failures []error, verbose bool) { fmt.Printf(" %s\n", indent(f.String())) } fmt.Printf("\n%d findings", len(findings)) - if dismissed > 0 { - fmt.Printf(", %d dismissed in the source", dismissed) + if len(env.Dismissed) > 0 { + fmt.Printf(", %d dismissed in the source", len(env.Dismissed)) } fmt.Println() + retractedNote(env) if verbose { fmt.Println("\nDismiss a finding where it is wrong, in the source it concerns:") fmt.Println(" //review:ignore <rule> <why>") } } +// retractedNote says how many findings did not survive the second reading, +// because a retraction that went unmentioned would read as the first +// reading having been right all along. +func retractedNote(env contract) { + if len(env.Retracted) == 0 { + return + } + fmt.Printf("%d of the findings reported did not survive verification\n", len(env.Retracted)) +} + func indent(s string) string { return strings.ReplaceAll(s, "\n ", "\n ") } -func report(findings []Finding) error { - if findings == nil { - findings = []Finding{} +// report prints the contract: the whole measurement, not only the +// findings, so that a program reading it can tell a pass from a hole. Each +// finding is given its stable id and its severity as text, which is what a +// loop needs to answer a finding and check it stayed answered. +func report(env contract) error { + // Every list is said even when empty: a key an agent cannot find is a + // hole it guesses about, and the contract is what makes the reading + // readable without guessing. + if env.Findings == nil { + env.Findings = []Finding{} } - for i := range findings { - findings[i].SeverityName = findings[i].Severity.String() + if env.Retracted == nil { + env.Retracted = []Retracted{} + } + if env.Failed == nil { + env.Failed = []JobFault{} + } + if env.Skipped == nil { + env.Skipped = []string{} + } + if env.Uncovered == nil { + env.Uncovered = []Gap{} + } + if env.Dismissed == nil { + env.Dismissed = []Dismissed{} + } + nameFindings(env.Findings) + for i := range env.Retracted { + env.Retracted[i].Finding.SeverityName = env.Retracted[i].Finding.Severity.String() + identify(&env.Retracted[i].Finding) } - out, err := json.MarshalIndent(map[string]any{"findings": findings}, "", " ") + out, err := json.MarshalIndent(env, "", " ") if err != nil { return err } @@ -166,6 +356,15 @@ func report(findings []Finding) error { return nil } +// nameFindings fills in what a finding carries only in the report: its +// severity as text, and the id that names it across runs. +func nameFindings(findings []Finding) { + for i := range findings { + findings[i].SeverityName = findings[i].Severity.String() + identify(&findings[i]) + } +} + func chosen(only string) ([]Job, error) { all := Jobs() if only == "" { @@ -216,6 +415,9 @@ Jobs: claims whether the comments it adds assert what nobody checked hygiene whether the commit message matches the commit +The commit message is also measured before the jobs run, without a model: +see the static checks in the readme. + Dismiss a finding where it is wrong, in the source it concerns: //review:ignore <rule> <why> `) diff --git a/main_test.go b/main_test.go @@ -0,0 +1,323 @@ +package main + +import ( + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +// capture collects what is printed to standard output, which is how the +// command reports. +func capture(t *testing.T, fn func()) string { + t.Helper() + read, write, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + stdout := os.Stdout + os.Stdout = write + done := make(chan string) + go func() { + out, _ := io.ReadAll(read) + done <- string(out) + }() + fn() + write.Close() + os.Stdout = stdout + return <-done +} + +func TestFilterDropsWhatTheSourceDismisses(t *testing.T) { + root := t.TempDir() + source := "package x\n\n//review:ignore restates-a-fact the ico package owns it\nconst b = 6\n" + if err := os.WriteFile(filepath.Join(root, "x.go"), []byte(source), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(t.TempDir()) // Somewhere else, so that filter has to change directory itself. + + kept, dismissed := filter(root, []Finding{ + {File: "x.go", Line: 4, Rule: "restates-a-fact", Message: "dismissed"}, + {File: "x.go", Line: 4, Rule: "already-named", Message: "kept"}, + }) + if len(dismissed) != 1 { + t.Fatalf("dismissed %v, want one", dismissed) + } + if dismissed[0].Why != "the ico package owns it" { + t.Errorf("the reason is kept, got %q", dismissed[0].Why) + } + if len(kept) != 1 || kept[0].Message != "kept" { + t.Errorf("kept %v", kept) + } +} + +// A dismissal reaches a few lines either side of itself, so which findings +// it answered has to be readable rather than only countable. +func TestRenderNamesWhatWasDismissed(t *testing.T) { + dismissed := []Dismissed{{ + Finding: Finding{File: "x.go", Line: 7, Rule: "restates-a-fact"}, + Why: "the ico package owns it", + }} + out := capture(t, func() { render(contract{Dismissed: dismissed}, nil, true) }) + for _, want := range []string{"dismissed: x.go:7", "restates-a-fact", "the ico package owns it"} { + if !strings.Contains(out, want) { + t.Errorf("%q missing from:\n%s", want, out) + } + } + // Without being asked, only the count. + quiet := capture(t, func() { render(contract{Dismissed: dismissed}, nil, false) }) + if strings.Contains(quiet, "the ico package owns it") { + t.Errorf("got %q", quiet) + } +} + +func TestFilterKeepsEverythingWhenTheRootIsGone(t *testing.T) { + findings := []Finding{{File: "x.go", Line: 1, Rule: "a"}} + kept, dismissed := filter(filepath.Join(t.TempDir(), "nowhere"), findings) + if len(kept) != 1 || len(dismissed) != 0 { + t.Errorf("got %v, %v", kept, dismissed) + } +} + +func TestReport(t *testing.T) { + out := capture(t, func() { + if err := report(contract{Findings: []Finding{{ + Job: "tests", Rule: "cannot-fail", Severity: MustFix, + File: "x_test.go", Line: 4, Message: "m", Fix: "f", + }}}); err != nil { + t.Error(err) + } + }) + var envelope struct { + Findings []struct { + Rule string `json:"rule"` + Severity string `json:"severity"` + Line int `json:"line"` + } `json:"findings"` + } + if err := json.Unmarshal([]byte(out), &envelope); err != nil { + t.Fatalf("%v in:\n%s", err, out) + } + if len(envelope.Findings) != 1 { + t.Fatalf("got %v", envelope.Findings) + } + // The severity travels as its name, since the number means nothing to a + // program reading this. + if envelope.Findings[0].Severity != "must-fix" || envelope.Findings[0].Line != 4 { + t.Errorf("got %+v", envelope.Findings[0]) + } +} + +// An agent reading the JSON needs an empty list rather than null. +func TestReportNothing(t *testing.T) { + out := capture(t, func() { + if err := report(contract{}); err != nil { + t.Error(err) + } + }) + if !strings.Contains(out, `"findings": []`) { + t.Errorf("got %s", out) + } +} + +// The contract is what makes an empty findings list readable: the status, +// the parts of the measurement that went missing, and the cost, all in one +// object, with every finding named and marked. +func TestReportCarriesTheContract(t *testing.T) { + out := capture(t, func() { + if err := report(contract{ + Version: contractVersion, + Status: "incomplete", + Provider: "api/probe", + Retracted: []Retracted{{ + Finding: Finding{Job: "tests", Rule: "cannot-fail", Severity: MustFix, File: "x_test.go", Message: "m"}, + Reason: "the test can fail", + }}, + Failed: []JobFault{{Job: "duplication", Error: "context canceled"}}, + Skipped: []string{"hygiene"}, + Uncovered: []Gap{{File: "a.odin", Reason: "no tests parser"}}, + Dismissed: []Dismissed{{Finding: Finding{Rule: "cannot-fail"}, Why: "wrong"}}, + Truncated: true, + Usage: metered{In: 10, Out: 2, Cached: 1, Replayed: 3, Cost: 0.5}, + }); err != nil { + t.Error(err) + } + }) + var env struct { + Version int `json:"version"` + Status string `json:"status"` + Provider string `json:"provider"` + Findings []any `json:"findings"` + Retracted []struct { + Finding struct { + ID string `json:"id"` + Verified bool `json:"verified"` + Severity string `json:"severity"` + } `json:"finding"` + Reason string `json:"reason"` + } `json:"retracted"` + Failed []struct { + Job string `json:"job"` + Error string `json:"error"` + } `json:"failed"` + Skipped []string `json:"skipped"` + Uncovered []struct { + File string `json:"file"` + Reason string `json:"reason"` + } `json:"uncovered"` + Dismissed []struct { + Why string `json:"why"` + } `json:"dismissed"` + Truncated bool `json:"truncated"` + Usage struct { + In int `json:"in"` + Out int `json:"out"` + Cached int `json:"cached"` + Replayed int `json:"replayed"` + Cost float64 `json:"usd"` + } `json:"usage"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("%v in:\n%s", err, out) + } + if env.Version != contractVersion || env.Status != "incomplete" || env.Provider != "api/probe" { + t.Errorf("got %d %q %q", env.Version, env.Status, env.Provider) + } + if len(env.Retracted) != 1 || env.Retracted[0].Reason != "the test can fail" { + t.Errorf("got %+v", env.Retracted) + } + if env.Retracted[0].Finding.ID == "" || env.Retracted[0].Finding.Verified { + t.Errorf("a retracted finding is named and unverified, got %+v", env.Retracted[0].Finding) + } + if env.Retracted[0].Finding.Severity != "must-fix" { + t.Errorf("got %q", env.Retracted[0].Finding.Severity) + } + if len(env.Failed) != 1 || env.Failed[0].Job != "duplication" || env.Failed[0].Error != "context canceled" { + t.Errorf("got %+v", env.Failed) + } + if len(env.Skipped) != 1 || env.Skipped[0] != "hygiene" { + t.Errorf("got %+v", env.Skipped) + } + if len(env.Uncovered) != 1 || env.Uncovered[0].File != "a.odin" { + t.Errorf("got %+v", env.Uncovered) + } + if len(env.Dismissed) != 1 || env.Dismissed[0].Why != "wrong" { + t.Errorf("got %+v", env.Dismissed) + } + if !env.Truncated || env.Usage.In != 10 || env.Usage.Replayed != 3 || env.Usage.Cost != 0.5 { + t.Errorf("got %v %+v", env.Truncated, env.Usage) + } + if len(env.Findings) != 0 { + t.Errorf("got %+v", env.Findings) + } +} + +// The status is what keeps an empty findings list from being read as a +// pass when part of the measurement is missing. +func TestStatusOfTheMeasurement(t *testing.T) { + cases := []struct { + change *Change + run RunResult + want string + }{ + {&Change{}, RunResult{}, "complete"}, + {&Change{Truncated: true}, RunResult{}, "incomplete"}, + {&Change{Uncovered: []Gap{{File: "a.odin", Reason: "no tests parser"}}}, RunResult{}, "incomplete"}, + {&Change{}, RunResult{Failures: []error{fmt.Errorf("tests: context canceled")}}, "incomplete"}, + } + for _, test := range cases { + if got := statusOf(test.change, test.run); got != test.want { + t.Errorf("got %q, want %q", got, test.want) + } + } +} + +// A retraction is worth reading in full under verbose, because it is the +// second reading's word against the first's. +func TestRenderNamesWhatWasRetracted(t *testing.T) { + env := contract{Retracted: []Retracted{{ + Finding: Finding{Rule: "cannot-fail", File: "x_test.go", Line: 4}, + Reason: "the test can fail", + }}} + out := capture(t, func() { render(env, nil, true) }) + for _, want := range []string{"retracted: [cannot-fail] x_test.go:4", "the test can fail"} { + if !strings.Contains(out, want) { + t.Errorf("%q missing from:\n%s", want, out) + } + } + // The count is said even without being asked. + quiet := capture(t, func() { render(env, nil, false) }) + if !strings.Contains(quiet, "1 of the findings reported did not survive verification") { + t.Errorf("got %q", quiet) + } +} + +func TestRenderGroupsBySeverity(t *testing.T) { + out := capture(t, func() { + render(contract{ + Findings: []Finding{ + {Job: "tests", Rule: "cannot-fail", Severity: MustFix, File: "a_test.go", Line: 4, Message: "one"}, + {Job: "namer", Rule: "abbreviation", Severity: Note, Symbol: "cfg", Message: "two"}, + }, + Dismissed: []Dismissed{{}, {}}, + }, nil, false) + }) + for _, want := range []string{"MUST-FIX", "NOTE", "one", "two", "2 findings", "2 dismissed in the source"} { + if !strings.Contains(out, want) { + t.Errorf("%q missing from:\n%s", want, out) + } + } + if strings.Count(out, "MUST-FIX") != 1 { + t.Errorf("a severity is announced once:\n%s", out) + } +} + +func TestRenderNothing(t *testing.T) { + out := capture(t, func() { render(contract{}, nil, false) }) + if strings.TrimSpace(out) != "no findings" { + t.Errorf("got %q", out) + } + // A silent dismissal is still visible. + out = capture(t, func() { render(contract{Dismissed: []Dismissed{{}, {}, {}}}, nil, false) }) + if !strings.Contains(out, "3 dismissed in the source") { + t.Errorf("got %q", out) + } +} + +func TestRepository(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("first", "x.go") + t.Chdir(r.Root) + + root, err := repository() + if err != nil { + t.Fatal(err) + } + // A temporary directory may be reached through a symlink, so compare + // what the filesystem resolves to. + want, err := filepath.EvalSymlinks(r.Root) + if err != nil { + t.Fatal(err) + } + if got, _ := filepath.EvalSymlinks(root); got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestRepositoryOutsideOne(t *testing.T) { + t.Chdir(t.TempDir()) + t.Setenv("GIT_CEILING_DIRECTORIES", os.TempDir()) + if _, err := repository(); err == nil { + t.Skip("the temporary directory is inside a repository on this machine") + } +} + +func TestIndent(t *testing.T) { + if got := indent("a\n b"); got != "a\n b" { + t.Errorf("got %q", got) + } +} diff --git a/odinfrontend.go b/odinfrontend.go @@ -0,0 +1,180 @@ +package main + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// OdinFrontend reads Odin through the odin-review-extract sidecar, a small +// program built on Odin's own parser (core:odin). Declaring a review in terms +// of the language's grammar is the point; the sidecar ships in this +// repository and is found on the path. +type OdinFrontend struct{} + +func (OdinFrontend) Name() string { return "odin" } + +func (OdinFrontend) Covers(path string) bool { return strings.HasSuffix(path, ".odin") } + +func (OdinFrontend) Features() Features { + return FeatSymbols | FeatTests | FeatComments | FeatIndex +} + +// odinDecl is one declaration as the sidecar reports it. +type odinDecl struct { + Name string `json:"name"` + Kind string `json:"kind"` + Line int `json:"line"` + EndLine int `json:"end_line"` + Exported bool `json:"exported"` + Test bool `json:"test"` + Text string `json:"text"` + Doc string `json:"doc"` +} + +type odinFile struct { + Name string `json:"name"` + Decls []odinDecl `json:"decls"` +} + +type odinOutput struct { + Files []odinFile `json:"files"` +} + +// odinScan materialises the files at the revision into a scratch directory, +// asks the sidecar for their declarations, and maps the answers back to the +// names given. A missing sidecar is the caller's problem: frontends() only +// offers this one when it is on the path. +func odinScan(files []string, root, rev string) ([]odinDecl, map[string][]odinDecl, error) { + if len(files) == 0 { + return nil, nil, nil + } + dir, err := os.MkdirTemp("", "review-odin") + if err != nil { + return nil, nil, err + } + defer os.RemoveAll(dir) + + var args []string + for i, name := range files { + source, err := at(root, rev, name) + if err != nil { + continue // Deleted by the change, so there is nothing to read. + } + scratch := filepath.Join(dir, fmt.Sprintf("%04d%s", i, filepath.Ext(name))) + if err := os.WriteFile(scratch, source, 0o644); err != nil { + return nil, nil, err + } + args = append(args, scratch) + } + cmd := exec.Command("odin-review-extract", args...) + stdout, err := cmd.Output() + if err != nil && len(stdout) == 0 { + return nil, nil, fmt.Errorf("odin-review-extract: %w", err) + } + var out odinOutput + if err := json.Unmarshal(stdout, &out); err != nil { + return nil, nil, fmt.Errorf("odin-review-extract: %w", err) + } + byFile := map[string][]odinDecl{} + var all []odinDecl + for _, f := range out.Files { + base := strings.TrimSuffix(filepath.Base(f.Name), filepath.Ext(f.Name)) + index, err := fmt.Sscanf(base, "%04d", new(int)) + if err != nil || index == 0 { + continue + } + var n int + if _, err := fmt.Sscanf(base, "%d", &n); err != nil || n >= len(files) { + continue + } + byFile[files[n]] = f.Decls + all = append(all, f.Decls...) + } + return all, byFile, nil +} + +// Change appends the declarations, tests and prose the added lines of the +// change's Odin files introduce. +func (g OdinFrontend) Change(root, rev string, c *Change, added map[string][]int) error { + var covered []string + for _, name := range c.Files { + if g.Covers(name) { + covered = append(covered, name) + } + } + _, byFile, err := odinScan(covered, root, rev) + if err != nil { + return err + } + for _, name := range covered { + touched := map[int]bool{} + for _, line := range added[name] { + touched[line] = true + } + for _, decl := range byFile[name] { + if !touched[decl.Line] { + continue + } + if decl.Test { + source, err := at(root, rev, name) + if err != nil { + continue + } + lines := strings.Split(string(source), "\n") + end := decl.EndLine + if end > len(lines) { + end = len(lines) + } + c.Tests = append(c.Tests, Function{ + Name: decl.Name, File: name, Line: decl.Line, + Body: text(lines, decl.Line, end), + }) + continue + } + c.Symbols = append(c.Symbols, Symbol{ + Name: decl.Name, Kind: decl.Kind, Doc: decl.Doc, + File: name, Line: decl.Line, + Exported: decl.Exported, Signature: decl.Text, + }) + } + source, err := at(root, rev, name) + if err != nil { + continue + } + c.Comments = append(c.Comments, commentProse(source, name, added[name])...) + } + return nil +} + +// Whole reads every declaration in the repository's Odin files, so a new +// name can be checked against the ones it may duplicate. +func (g OdinFrontend) Whole(root, rev string) ([]Declared, error) { + out, err := git(root, "ls-files", "*.odin") + if err != nil { + return nil, err + } + var files []string + for _, name := range strings.Split(strings.TrimSpace(out), "\n") { + if name != "" && g.Covers(name) { + files = append(files, name) + } + } + _, byFile, err := odinScan(files, root, rev) + if err != nil { + return nil, err + } + var index []Declared + for _, name := range files { + for _, decl := range byFile[name] { + if decl.Test { + continue // Tests are not facts with two owners. + } + index = append(index, Declared{Name: decl.Name, Kind: decl.Kind, File: name, Line: decl.Line, Text: decl.Text}) + } + } + return index, nil +} diff --git a/odinfrontend_test.go b/odinfrontend_test.go @@ -0,0 +1,143 @@ +package main + +import ( + "os/exec" + "testing" +) + +// The Odin tests need the sidecar; without it the frontend is not offered. +func needSidecar(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("odin-review-extract"); err != nil { + t.Skip("odin-review-extract is not installed") + } +} + +func odinFixture(t *testing.T) *repo { + t.Helper() + r := newRepo(t) + r.write("src/.keep", "") + r.commit("odin: begin", "src/.keep") + return r +} + +func TestOdinReadsDeclarations(t *testing.T) { + needSidecar(t) + r := odinFixture(t) + r.write("src/icons.odin", `package icons + +// Counts the icons a binary carries. +count_icons :: proc(groups: []Group) -> int { return 0 } + +@(test) +test_counts_icons :: proc(t: ^testing.T) {} + +Group :: struct { + id: int, +} + +@(private="file") +helper :: proc() {} + +MAX_ICONS :: 12 +`) + rev := r.commit("odin: count", "src/icons.odin") + + change, err := Gather(rev+"^.."+rev, r.Root) + if err != nil { + t.Fatal(err) + } + got := map[string]Symbol{} + for _, s := range change.Symbols { + got[s.Name] = s + } + for name, want := range map[string]struct { + kind string + exported bool + }{ + "count_icons": {"func", true}, + "Group": {"type", true}, + "helper": {"func", false}, + "MAX_ICONS": {"value", true}, + } { + s, ok := got[name] + if !ok { + t.Errorf("%s not read", name) + continue + } + if s.Kind != want.kind || s.Exported != want.exported { + t.Errorf("%s: got %s exported=%v, want %s exported=%v", name, s.Kind, s.Exported, want.kind, want.exported) + } + } + if s := got["count_icons"]; s.Doc != "Counts the icons a binary carries." { + t.Errorf("doc %q", s.Doc) + } + if len(change.Tests) != 1 || change.Tests[0].Name != "test_counts_icons" { + t.Fatalf("tests %v", change.Tests) + } + if len(change.Tests[0].Body) == 0 { + t.Error("test body is empty") + } +} + +func TestOdinIndexesTheRepository(t *testing.T) { + needSidecar(t) + r := odinFixture(t) + r.write("src/icons.odin", `package icons + +count_icons :: proc(groups: []Group) -> int { return 0 } + +@(test) +test_counts_icons :: proc(t: ^testing.T) {} + +Group :: struct { + id: int, +} +`) + rev := r.commit("odin: index", "src/icons.odin") + + declared, err := OdinFrontend{}.Whole(r.Root, rev) + if err != nil { + t.Fatal(err) + } + index := map[string]Declared{} + for _, d := range declared { + if _, dup := index[d.Name]; dup { + t.Errorf("%s indexed twice", d.Name) + } + index[d.Name] = d + } + if d, ok := index["count_icons"]; !ok || d.Kind != "func" || d.Text == "" { + t.Errorf("count_icons %v", d) + } + if d, ok := index["Group"]; !ok || d.Kind != "type" { + t.Errorf("Group %v", d) + } + if _, ok := index["test_counts_icons"]; ok { + t.Error("test procedure indexed") + } +} + +func TestOdinReadsComments(t *testing.T) { + needSidecar(t) + r := odinFixture(t) + r.write("src/icons.odin", `package icons + +count_icons :: proc() {} +`) + r.commit("odin: empty", "src/icons.odin") + r.write("src/icons.odin", `package icons + +// counts the icons a binary carries +count_icons :: proc() {} +`) + r.stage("src/icons.odin") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Comments) != 1 || change.Comments[0].Text != "counts the icons a binary carries" { + t.Fatalf("comments %v", change.Comments) + } +} diff --git a/packet.go b/packet.go @@ -1,10 +1,9 @@ package main import ( + "cmp" "fmt" - "go/ast" - "go/parser" - "go/token" + "maps" "os" "os/exec" "path/filepath" @@ -26,6 +25,13 @@ type Change struct { // Stat is the per-file line counts, which is enough to notice a commit // doing two things without reading either. Stat string + // Truncated is whether the diff was capped, which makes every count + // under it a count of what was read rather than of what changed. + Truncated bool + // Uncovered is the part of the change the deterministic side could not + // read, with a reason for each file. Jobs read what they can; what + // they could not is said rather than missed. + Uncovered []Gap // Symbols are the declarations the change adds or renames. Symbols []Symbol // Tests are the test functions it adds or changes, whole. @@ -36,9 +42,35 @@ type Change struct { // search rather than by the model, which is what keeps the duplication // job cheap. Candidates map[string][]string + // Twins are the existing declarations holding the same literal as a new + // one. They are the shortest list worth reading, so they are kept apart + // from the resemblances they would otherwise be buried in. + Twins map[string][]string // Convention is the recent commit subjects, so a job can read the local // habit rather than impose one. Convention []string + // History is the subjects of the repository's last thousand commits, the + // word frequencies the commit message is measured against. + History []string + // Temporal is what the repository's history counts about the files the + // change touches: how many commits touch each, and which other files + // those commits also touched. Nil when the history says nothing. + Temporal *Temporal +} + +// Temporal is the counted history of the files a change touches. +type Temporal struct { + // Commits counts, over the counted commits, how many touch each file. + Commits map[string]int + // Partners lists, per changed file, the other files those commits also + // touched, nearest first. + Partners map[string][]Partner +} + +// Partner is another file that history shows changing with a changed one. +type Partner struct { + Name string + Shared int } // Symbol is a declaration the change introduces. @@ -70,48 +102,73 @@ type Located struct { Line int `json:"line"` } +// uncommittedNote is what a message read for a staged change says instead of +// claiming to be one: git has no message for a commit that does not exist. +const uncommittedNote = "(uncommitted; the message below is the previous commit's)\n" + +// Gap is a file the deterministic side could not read at all, and +// why. It is reported rather than skipped silently, because a job that +// read nothing reports nothing, and silence is what an empty findings list +// is made of. +type Gap struct { + File string `json:"file"` + Reason string `json:"reason"` +} + // maxDiff caps what is sent. A change larger than this is reviewed by its // parts rather than badly as a whole. const maxDiff = 60000 -// at reads a file as it stands at a revision, so that reviewing an old commit -// reads the code that commit left behind rather than whatever the working -// tree holds now. +// at reads a file as it stands at the end of the change under review, so that +// reviewing an old commit reads the code that commit left behind rather than +// whatever the working tree holds now. func at(root, rev, path string) ([]byte, error) { - if rev == "" { + after, ranged := ends(rev) + if !ranged { + // The diff for a bare revision runs from it to the working tree, so + // the working tree is where the change arrived. return os.ReadFile(filepath.Join(root, path)) } - // A..B is reviewed as B, which is the state the change arrived at. - if _, after, found := strings.Cut(rev, ".."); found { - rev = after - } - cmd := exec.Command("git", "show", rev+":"+path) + cmd := exec.Command("git", "show", after+":"+path) cmd.Dir = root return cmd.Output() } +// ends reports the revision a range arrives at, and whether it is a range at +// all. Both A..B and A...B are reviewed as B. +func ends(rev string) (string, bool) { + _, after, found := strings.Cut(rev, "..") + if !found { + return rev, false + } + return strings.TrimPrefix(after, "."), true +} + // Gather collects the change at a revision range, or the staged change when // the range is empty. func Gather(rev string, root string) (*Change, error) { + // The prefixes are named rather than inherited: diff.mnemonicPrefix + // rewrites them per side, and the diff is parsed by them below. var diffArgs, nameArgs, statArgs []string switch rev { case "": - diffArgs = []string{"diff", "--cached", "-U3"} + diffArgs = []string{"diff", "--cached", "-U3", "--src-prefix=a/", "--dst-prefix=b/"} nameArgs = []string{"diff", "--cached", "--name-only"} statArgs = []string{"diff", "--cached", "--stat"} default: - diffArgs = []string{"diff", rev, "-U3"} + diffArgs = []string{"diff", rev, "-U3", "--src-prefix=a/", "--dst-prefix=b/"} nameArgs = []string{"diff", rev, "--name-only"} statArgs = []string{"diff", rev, "--stat"} } - change := &Change{Candidates: map[string][]string{}} + change := &Change{Candidates: map[string][]string{}, Twins: map[string][]string{}} var err error if change.Diff, err = git(root, diffArgs...); err != nil { return nil, err } if len(change.Diff) > maxDiff { change.Diff = change.Diff[:maxDiff] + "\n… diff truncated\n" + change.Truncated = true } names, err := git(root, nameArgs...) if err != nil { @@ -127,7 +184,7 @@ func Gather(rev string, root string) (*Change, error) { } if rev == "" { change.Message, _ = git(root, "log", "-1", "--format=%B") - change.Message = "(uncommitted; the message below is the previous commit's)\n" + change.Message + change.Message = uncommittedNote + change.Message } else { change.Message, _ = git(root, "log", "-1", "--format=%B", strings.TrimSuffix(rev, "^")) } @@ -138,101 +195,179 @@ func Gather(rev string, root string) (*Change, error) { } } } + if subjects, err := git(root, "log", "-1000", "--format=%s"); err == nil { + for _, s := range strings.Split(strings.TrimSpace(subjects), "\n") { + if s != "" { + change.History = append(change.History, s) + } + } + } - change.read(root, rev) - change.findCandidates(root, rev) + change.read(root, rev, frontends()) + change.findCandidates(root, rev, frontends()) + change.readTemporal(root, rev) return change, nil } -// read pulls the declarations, tests and prose out of the files the change -// touches. Only what the diff added is reported, so a job sees new work -// rather than the file it landed in. -func (c *Change) read(root, rev string) { - added := addedLines(c.Diff) - for _, name := range c.Files { - if !strings.HasSuffix(name, ".go") { - continue - } - source, err := at(root, rev, name) - if err != nil { - continue // Deleted by the change, so there is nothing to read. +// The counting window and width: the coupling is counted over the last +// thousand commits before the change, and a commit listing more than a +// hundred files is left out of the count — a sweep touching everything +// once says nothing about any pair, and leaving it out errs toward +// silence. +const ( + temporalWindow = 1000 + temporalWidth = 100 + + // partnerList is the fewest-nearest partners kept per changed file, so + // a file whose history touches everything does not carry the history. + partnerList = 20 +) + +// readTemporal counts, over the last thousand commits before the change, +// how often each changed file is touched by a commit that also touches +// another file. The history starts at the commit the change begins at, so +// the change under review is never counted against itself. +func (c *Change) readTemporal(root, rev string) { + if len(c.Files) == 0 { + return + } + var args []string + start := "" + switch { + case rev == "": + case strings.Contains(rev, ".."): + start = strings.SplitN(rev, "..", 2)[0] + case strings.HasSuffix(rev, "^"): + start = rev + default: + start = rev + "^" + } + args = []string{"log", fmt.Sprintf("-%d", temporalWindow), "--format=%x00", "--name-only"} + if start != "" { + args = append(args, start) + } + logs, err := git(root, args...) + if err != nil { + return // No history, or a bare first commit: nothing to count. + } + changed := map[string]bool{} + for _, f := range c.Files { + changed[f] = true + } + commits := map[string]int{} + pairs := map[string]map[string]int{} + for _, chunk := range strings.Split(logs, "\x00") { + var files []string + for _, line := range strings.Split(chunk, "\n") { + if line = strings.TrimSpace(line); line != "" { + files = append(files, line) + } } - path := filepath.Join(root, name) - fset := token.NewFileSet() - file, err := parser.ParseFile(fset, path, source, parser.ParseComments) - if err != nil { + if len(files) > temporalWidth { continue } - lines := strings.Split(string(source), "\n") - touched := added[name] - - ast.Inspect(file, func(n ast.Node) bool { - switch decl := n.(type) { - case *ast.FuncDecl: - line := fset.Position(decl.Pos()).Line - end := fset.Position(decl.End()).Line - if !touchedBetween(touched, line, end) { - return true - } - name := decl.Name.Name - if strings.HasPrefix(name, "Test") || strings.HasPrefix(name, "Fuzz") || strings.HasPrefix(name, "Benchmark") { - c.Tests = append(c.Tests, Function{ - Name: name, File: relative(root, path), Line: line, - Body: text(lines, line, end), - }) - return true - } - c.Symbols = append(c.Symbols, Symbol{ - Name: name, Kind: "func", Doc: doc(decl.Doc), - File: relative(root, path), Line: line, - Exported: ast.IsExported(name), Signature: strings.TrimSpace(lines[line-1]), - }) - case *ast.TypeSpec: - line := fset.Position(decl.Pos()).Line - if !slices.Contains(touched, line) { - return true - } - c.Symbols = append(c.Symbols, Symbol{ - Name: decl.Name.Name, Kind: "type", File: relative(root, path), Line: line, - Exported: ast.IsExported(decl.Name.Name), Signature: strings.TrimSpace(lines[line-1]), - }) - case *ast.ValueSpec: - for _, ident := range decl.Names { - line := fset.Position(ident.Pos()).Line - if !slices.Contains(touched, line) { - continue - } - c.Symbols = append(c.Symbols, Symbol{ - Name: ident.Name, Kind: "value", File: relative(root, path), Line: line, - Exported: ast.IsExported(ident.Name), Signature: strings.TrimSpace(lines[line-1]), - }) + for _, f := range files { + commits[f]++ + } + for _, f := range files { + if !changed[f] { + continue + } + if pairs[f] == nil { + pairs[f] = map[string]int{} + } + for _, g := range files { + if g != f { + pairs[f][g]++ } } - return true + } + } + remaining := c.remaining(root, rev, pairs) + t := &Temporal{Commits: commits, Partners: map[string][]Partner{}} + for f, ps := range pairs { + type named struct { + name string + j float64 + } + list := make([]named, 0, len(ps)) + for name, shared := range ps { + union := commits[f] + commits[name] - shared + if union <= 0 { + continue + } + list = append(list, named{name, float64(shared) / float64(union)}) + } + slices.SortFunc(list, func(a, b named) int { + if c := cmp.Compare(b.j, a.j); c != 0 { + return c + } + return cmp.Compare(a.name, b.name) }) + var partners []Partner + for _, e := range list { + if len(partners) == partnerList { + break + } + if remaining[e.name] { + partners = append(partners, Partner{Name: e.name, Shared: ps[e.name]}) + } + } + if len(partners) > 0 { + t.Partners[f] = partners + } + } + if len(t.Partners) > 0 { + c.Temporal = t + } +} - for _, group := range file.Comments { - for _, comment := range group.List { - line := fset.Position(comment.Pos()).Line - if !slices.Contains(touched, line) { - continue - } - c.Comments = append(c.Comments, Located{ - Text: strings.TrimSpace(strings.TrimPrefix(comment.Text, "//")), - File: relative(root, path), Line: line, - }) +// remaining reports which of the partner paths are still files at the end +// of the change: a partner the tree no longer holds is history's partner, +// not this change's. A review of a range asks git once for all of them; +// one of the working tree asks the filesystem. +func (c *Change) remaining(root, rev string, pairs map[string]map[string]int) map[string]bool { + all := map[string]bool{} + for _, ps := range pairs { + for name := range ps { + all[name] = true + } + } + out := map[string]bool{} + after, ranged := ends(rev) + if !ranged { + for name := range all { + if _, err := os.Stat(filepath.Join(root, name)); err == nil { + out[name] = true } } + return out } + args := append([]string{"ls-tree", "--name-only", after, "--"}, slices.Collect(maps.Keys(all))...) + if ls, err := git(root, args...); err == nil { + for _, line := range strings.Split(ls, "\n") { + if line = strings.TrimSpace(line); line != "" { + out[line] = true + } + } + } + return out } // findCandidates gathers, for every new name, the existing declarations that // might already mean the same thing. Finding them is the cheap part; judging // the shortlist is what the job is for. -func (c *Change) findCandidates(root, rev string) { - index, err := Index(root, rev) - if err != nil { - return +func (c *Change) findCandidates(root, rev string, frontends []Frontend) { + var index []Declared + for _, f := range frontends { + if f.Features()&FeatIndex == 0 { + continue + } + declared, err := f.Whole(root, rev) + if err != nil { + continue + } + index = append(index, declared...) } // A constant holding the same literal as a new one is the strongest hint // a fact has been written twice, so it goes first where it exists. @@ -246,32 +381,89 @@ func (c *Change) findCandidates(root, rev string) { twins = append(twins, declared.String()+" <- same value") } } - c.Candidates[symbol.Name] = append(twins, Resembling(index, symbol, 16)...) + if len(twins) > 0 { + c.Twins[symbol.Name] = twins + } + c.Candidates[symbol.Name] = slices.Concat(twins, Resembling(index, symbol, 16)) } } var hunk = regexp.MustCompile(`^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@`) -// addedLines reports which lines of which files the diff adds, so that a job -// is shown new work rather than whatever file it landed in. -func addedLines(diff string) map[string][]int { - out := map[string][]int{} +// diffLine is one added line and the number it lands on in the file the +// change arrives at. +type diffLine struct { + Line int + Text string +} + +// diffSides splits a diff into the added and removed lines of each file, +// with the added lines' text. The header lines are read wherever they sit, +// as they sit between hunks; the hunk's own content is read only inside a +// hunk, so that a removed line of content beginning "+++ " is not mistaken +// for the header it resembles. +func diffSides(diff string) (added map[string][]diffLine, removed map[string][]string) { + added = map[string][]diffLine{} + removed = map[string][]string{} var ( - file string - line int + addedFile, removedFile string + line int + inHunk bool ) for _, text := range strings.Split(diff, "\n") { switch { - case strings.HasPrefix(text, "+++ b/"): - file = strings.TrimPrefix(text, "+++ b/") + case strings.HasPrefix(text, "+++ "): + // A deletion names /dev/null, which owns no added lines. + addedFile = side(text[4:], "b/") + inHunk = false + case strings.HasPrefix(text, "--- "): + // The left side is not being read, but a removed line is + // only a removed line inside a hunk, so the header is + // where the removed file is named. + removedFile = side(text[4:], "a/") + inHunk = false + case strings.HasPrefix(text, "\\ "): + // The no-newline marker annotates the line above it rather than + // standing for a line of its own. case hunk.MatchString(text): + inHunk = true line = atoi(hunk.FindStringSubmatch(text)[1]) - case strings.HasPrefix(text, "+") && file != "": - out[file] = append(out[file], line) + case strings.HasPrefix(text, "+") && inHunk: + if addedFile != "" { + added[addedFile] = append(added[addedFile], diffLine{line, strings.TrimPrefix(text, "+")}) + } line++ - case strings.HasPrefix(text, "-"): + case strings.HasPrefix(text, "-") && inHunk: + if removedFile != "" { + removed[removedFile] = append(removed[removedFile], strings.TrimPrefix(text, "-")) + } default: - line++ + if inHunk { + line++ + } + } + } + return added, removed +} + +// side strips a diff's prefix from a header path, and a deletion's +// /dev/null with it. +func side(path, prefix string) string { + path = strings.TrimPrefix(path, prefix) + if path == "/dev/null" { + return "" + } + return path +} + +// addedLines reports which lines of which files the diff adds, so that a +// job is shown new work rather than whatever file it landed in. +func addedLines(diff string) map[string][]int { + added, _ := diffSides(diff) + out := map[string][]int{} + for file, lines := range added { + for _, l := range lines { + out[file] = append(out[file], l.Line) } } return out @@ -293,13 +485,6 @@ func text(lines []string, from, to int) string { return strings.Join(lines[from-1:to], "\n") } -func doc(group *ast.CommentGroup) string { - if group == nil { - return "" - } - return strings.TrimSpace(group.Text()) -} - func relative(root, path string) string { if rel, err := filepath.Rel(root, path); err == nil { return rel diff --git a/packet_test.go b/packet_test.go @@ -0,0 +1,399 @@ +package main + +import ( + "reflect" + "strings" + "testing" +) + +func TestAddedLines(t *testing.T) { + for _, test := range []struct { + name string + diff string + want map[string][]int + }{{ + name: "a new file is added whole", + diff: `diff --git a/x.go b/x.go +new file mode 100644 +--- /dev/null ++++ b/x.go +@@ -0,0 +1,3 @@ ++package x ++ ++const a = 1 +`, + want: map[string][]int{"x.go": {1, 2, 3}}, + }, { + name: "context lines advance the count and removals do not", + diff: `--- a/x.go ++++ b/x.go +@@ -1,4 +1,4 @@ + package x +-const a = 1 ++const a = 2 + + func f() {} +`, + want: map[string][]int{"x.go": {2}}, + }, { + name: "a second hunk restarts at its own line", + diff: `--- a/x.go ++++ b/x.go +@@ -1,2 +1,3 @@ + package x ++const a = 1 + +@@ -20,2 +21,3 @@ + func f() {} ++func g() {} + +`, + want: map[string][]int{"x.go": {2, 22}}, + }, { + name: "each file keeps its own lines", + diff: `--- a/x.go ++++ b/x.go +@@ -0,0 +1 @@ ++package x +--- a/y.go ++++ b/y.go +@@ -0,0 +1 @@ ++package y +`, + want: map[string][]int{"x.go": {1}, "y.go": {1}}, + }, { + name: "a hunk header without counts still gives the line", + diff: `--- a/x.go ++++ b/x.go +@@ -7 +7 @@ +-const a = 1 ++const a = 2 +`, + want: map[string][]int{"x.go": {7}}, + }, { + name: "a diff touching nothing adds nothing", + diff: ``, + want: map[string][]int{}, + }} { + t.Run(test.name, func(t *testing.T) { + got := addedLines(test.diff) + if !reflect.DeepEqual(got, test.want) { + t.Errorf("got %v, want %v", got, test.want) + } + }) + } +} + +// The no-newline marker sits between the two sides of a hunk, so counting it +// as a line puts every added line after it one too far down. +func TestAddedLinesIgnoresTheNoNewlineMarker(t *testing.T) { + diff := `--- a/f.txt ++++ b/f.txt +@@ -1,2 +1,3 @@ + a +-b +\ No newline at end of file ++B ++c +` + want := map[string][]int{"f.txt": {2, 3}} + if got := addedLines(diff); !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +// A deletion writes +++ /dev/null, which must not be read as a line added to +// whichever file came before it. +func TestAddedLinesIgnoresADeletedFile(t *testing.T) { + diff := `--- a/x.go ++++ b/x.go +@@ -1,1 +1,2 @@ + package x ++const a = 1 +diff --git a/y.go b/y.go +deleted file mode 100644 +--- a/y.go ++++ /dev/null +@@ -1,2 +0,0 @@ +-package y +-const b = 2 +` + want := map[string][]int{"x.go": {2}} + if got := addedLines(diff); !reflect.DeepEqual(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestTouchedBetween(t *testing.T) { + lines := []int{4, 9} + for _, test := range []struct { + from, to int + want bool + }{ + {1, 3, false}, + {1, 4, true}, + {4, 4, true}, + {5, 8, false}, + {5, 20, true}, + {10, 20, false}, + } { + if got := touchedBetween(lines, test.from, test.to); got != test.want { + t.Errorf("touchedBetween(%v, %d, %d) = %v, want %v", lines, test.from, test.to, got, test.want) + } + } + if touchedBetween(nil, 1, 100) { + t.Error("nothing touched should report false") + } +} + +func TestText(t *testing.T) { + lines := []string{"one", "two", "three"} + if got := text(lines, 1, 2); got != "one\ntwo" { + t.Errorf("got %q", got) + } + if got := text(lines, 2, 2); got != "two" { + t.Errorf("got %q", got) + } + // Out of range asks for text that is not there, which is empty rather + // than a panic. + if got := text(lines, 0, 2); got != "" { + t.Errorf("got %q, want empty", got) + } + if got := text(lines, 1, 4); got != "" { + t.Errorf("got %q, want empty", got) + } +} + +func TestAtReadsTheRevisionUnderReview(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n\nconst a = 1\n") + first := r.commit("first", "x.go") + r.write("x.go", "package x\n\nconst a = 2\n") + second := r.commit("second", "x.go") + r.write("x.go", "package x\n\nconst a = 3\n") + + for _, test := range []struct { + rev string + want string + }{ + {"", "const a = 3"}, // The working tree. + {first + ".." + second, "const a = 2"}, // A range is read at its end. + {first + "..." + second, "const a = 2"}, // So is a symmetric range. + {first, "const a = 3"}, // A bare revision diffs to now. + {second, "const a = 3"}, + } { + got, err := at(r.Root, test.rev, "x.go") + if err != nil { + t.Fatalf("at(%q): %v", test.rev, err) + } + if !strings.Contains(string(got), test.want) { + t.Errorf("at(%q) = %q, want it to contain %q", test.rev, got, test.want) + } + } +} + +func TestAtReportsAMissingFile(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + first := r.commit("first", "x.go") + if _, err := at(r.Root, first+".."+first, "nowhere.go"); err == nil { + t.Error("reading a file that is not in the revision should fail") + } +} + +// Gather with no revision reads the staged change, which is the default the +// command documents and the one a commit hook would use. +func TestGatherStaged(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("first", "x.go") + + r.write("x.go", `package x + +// Size is how wide a row is. +const Size = 6 + +// Assemble builds a thing. +func Assemble() {} +`) + r.write("x_test.go", `package x + +import "testing" + +func TestAssemble(t *testing.T) { + Assemble() +} +`) + r.stage("x.go", "x_test.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Files) != 2 { + t.Fatalf("files: got %v, want two", change.Files) + } + names := map[string]string{} + for _, s := range change.Symbols { + names[s.Name] = s.Kind + } + if names["Size"] != "value" { + t.Errorf("Size: got kind %q, want value; symbols were %v", names["Size"], names) + } + if names["Assemble"] != "func" { + t.Errorf("Assemble: got kind %q, want func; symbols were %v", names["Assemble"], names) + } + if len(change.Tests) != 1 || change.Tests[0].Name != "TestAssemble" { + t.Errorf("tests: got %v, want TestAssemble", change.Tests) + } + if !strings.Contains(change.Tests[0].Body, "Assemble()") { + t.Errorf("a test is carried whole, got %q", change.Tests[0].Body) + } + var comments []string + for _, c := range change.Comments { + comments = append(comments, c.Text) + } + if len(comments) != 2 { + t.Errorf("comments: got %v, want both doc lines", comments) + } + if strings.TrimSpace(change.Stat) == "" { + t.Error("the statistics are empty") + } +} + +// Reviewing a range reads the code that range left behind, not whatever the +// working tree holds now. +func TestGatherRangeIgnoresTheWorkingTree(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + first := r.commit("first", "x.go") + r.write("x.go", "package x\n\nconst Size = 6\n") + second := r.commit("second", "x.go") + // Work that landed after the revision under review. + r.write("x.go", "package x\n\nconst Size = 6\n\nconst Later = 9\n") + r.commit("third", "x.go") + + change, err := Gather(first+".."+second, r.Root) + if err != nil { + t.Fatal(err) + } + for _, s := range change.Symbols { + if s.Name == "Later" { + t.Fatalf("read a symbol added after the revision under review: %v", change.Symbols) + } + } + if len(change.Symbols) != 1 || change.Symbols[0].Name != "Size" { + t.Fatalf("symbols: got %v, want only Size", change.Symbols) + } +} + +// The shortlist is what makes the duplication job cheap, so a constant +// holding the same literal as an existing one has to reach it. +func TestGatherFindsTheTwinConstant(t *testing.T) { + r := newRepo(t) + r.write("ico/writer.go", "package ico\n\nconst (\n\tdirectorySize = 6\n)\n") + first := r.commit("first", "ico/writer.go") + r.write("exe/exe.go", "package exe\n\nconst (\n\tgroupHeaderSize = 6\n)\n") + second := r.commit("second", "exe/exe.go") + + change, err := Gather(first+".."+second, r.Root) + if err != nil { + t.Fatal(err) + } + candidates := strings.Join(change.Candidates["groupHeaderSize"], "\n") + if !strings.Contains(candidates, "directorySize = 6") { + t.Errorf("the twin constant is missing from the shortlist:\n%s", candidates) + } + if !strings.Contains(candidates, "same value") { + t.Errorf("the twin is not marked as holding the same value:\n%s", candidates) + } +} + +func TestGatherEmptyChange(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("first", "x.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if strings.TrimSpace(change.Diff) != "" { + t.Errorf("nothing is staged, so the diff should be empty, got %q", change.Diff) + } +} + +// A loop rewritten inside a function body is work on what the function does. +// Reading it as a new name is what made a purely mechanical commit report a +// dozen names it never introduced. +func TestGatherIgnoresABodyChange(t *testing.T) { + r := newRepo(t) + r.write("x.go", `package x + +func existing() int { + total := 0 + for i := 0; i < 3; i++ { + total += i + } + return total +} +`) + first := r.commit("first", "x.go") + r.write("x.go", `package x + +func existing() int { + total := 0 + for i := range 3 { + total += i + } + return total +} + +func added() int { return 1 } +`) + second := r.commit("second", "x.go") + + change, err := Gather(first+".."+second, r.Root) + if err != nil { + t.Fatal(err) + } + var names []string + for _, s := range change.Symbols { + names = append(names, s.Name) + } + if len(names) != 1 || names[0] != "added" { + t.Errorf("got %v, want only the declaration the change adds", names) + } +} + +// A test is judged by what it asserts, so a change anywhere inside one is a +// change to the test. +func TestGatherReadsATestWhoseBodyChanged(t *testing.T) { + r := newRepo(t) + r.write("x_test.go", "package x\n\nimport \"testing\"\n\nfunc TestX(t *testing.T) {\n\tt.Log(\"one\")\n}\n") + first := r.commit("first", "x_test.go") + r.write("x_test.go", "package x\n\nimport \"testing\"\n\nfunc TestX(t *testing.T) {\n\tt.Skip(\"two\")\n}\n") + second := r.commit("second", "x_test.go") + + change, err := Gather(first+".."+second, r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Tests) != 1 || change.Tests[0].Name != "TestX" { + t.Fatalf("got %v, want TestX", change.Tests) + } + if !strings.Contains(change.Tests[0].Body, "t.Skip") { + t.Errorf("the test is carried whole, got %q", change.Tests[0].Body) + } +} + +func TestIsTest(t *testing.T) { + for name, want := range map[string]bool{ + "TestX": true, "FuzzX": true, "BenchmarkX": true, + "Assemble": false, "testHelper": false, "Tested": true, + } { + if got := isTest(name); got != want { + t.Errorf("isTest(%q) = %v, want %v", name, got, want) + } + } +} diff --git a/provider.go b/provider.go @@ -0,0 +1,284 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os" + "os/exec" + "strings" +) + +// Provider answers a question. What answers it is not this tool's business: +// a console API, a coding assistant on the path, or anything else that takes +// a prompt and returns text. +type Provider interface { + // Name is how it is asked for. + Name() string + // Ask puts one question and returns what came back, as text. Findings are + // read out of that text by the caller, so every provider answers the same + // way whether or not it can enforce a schema. + Ask(ctx context.Context, system, user string) (Answer, error) +} + +// Answer is what a provider returned, with whatever it could say about the +// cost. A provider that reports no usage leaves the counts at zero rather +// than inventing them. Replayed marks an answer the cache remembered rather +// than asked for: it cost nothing this run, so its counts stay at zero. +type Answer struct { + Text string + In int + Out int + Cached int + Cost float64 + Replayed bool +} + +// Providers returns the ones built in, by name. +// +// The default model is the middle one rather than the smallest. Measured +// against the eval set, the smallest reads the shortlist and reports the +// first duplicate it finds rather than all of them, which is the one thing +// this tool is for. +func Providers() map[string]func(model string) Provider { + return map[string]func(string) Provider{ + "claude": func(model string) Provider { return ClaudeCode{Model: orElse(model, "sonnet")} }, + "pi": func(model string) Provider { return Pi{Model: model, Upstream: os.Getenv("REVIEW_PI_PROVIDER")} }, + "api": func(model string) Provider { return API{Model: orElse(model, "claude-sonnet-5")} }, + "command": func(model string) Provider { + return Command{Argv: strings.Fields(os.Getenv("REVIEW_COMMAND")), Field: os.Getenv("REVIEW_COMMAND_FIELD")} + }, + } +} + +func orElse(value, fallback string) string { + if value == "" { + return fallback + } + return value +} + +// ClaudeCode asks the coding assistant on the path, in its non-interactive +// mode. It uses whatever credentials that assistant already holds, which is +// why it works where no API key is set. +type ClaudeCode struct{ Model string } + +// Name carries the model, so that two asks through differently pointed +// providers are never confused — the answer cache keys on it. +func (c ClaudeCode) Name() string { return "claude/" + c.Model } + +func (c ClaudeCode) Ask(ctx context.Context, system, user string) (Answer, error) { + // The tools are refused rather than left to judgement: these jobs are + // given everything they may read, and a reader that goes looking for more + // is answering a different question from the one asked. + out, err := shell(ctx, "claude", + []string{ + "-p", "--model", c.Model, "--output-format", "json", + "--disallowedTools", "Bash,Read,Write,Edit,Glob,Grep,WebFetch,WebSearch,Task,NotebookEdit", + "--append-system-prompt", system, + }, user) + if err != nil { + return Answer{}, err + } + var envelope struct { + Result string `json:"result"` + IsError bool `json:"is_error"` + Cost float64 `json:"total_cost_usd"` + Usage struct { + In int `json:"input_tokens"` + Out int `json:"output_tokens"` + Cached int `json:"cache_read_input_tokens"` + } `json:"usage"` + } + if err := json.Unmarshal(out, &envelope); err != nil { + return Answer{}, fmt.Errorf("reading the answer: %w", err) + } + if envelope.IsError { + return Answer{}, fmt.Errorf("%s", strings.TrimSpace(envelope.Result)) + } + return Answer{ + Text: envelope.Result, In: envelope.Usage.In, Out: envelope.Usage.Out, + Cached: envelope.Usage.Cached, Cost: envelope.Cost, + }, nil +} + +// Pi asks the assistant of the same name, which speaks to several providers +// of its own. The upstream provider is named through the environment rather +// than here, since which one is reachable is a property of the machine. Its +// answer arrives as a stream of events, not as one envelope. +type Pi struct{ Model, Upstream string } + +// Name carries the upstream and the model, for the same reason. +func (p Pi) Name() string { + name := "pi" + if p.Upstream != "" { + name += "/" + p.Upstream + } + if p.Model != "" { + name += "/" + p.Model + } + return name +} + +func (p Pi) Ask(ctx context.Context, system, user string) (Answer, error) { + // The tools are refused for the same reason as the other assistant: a + // reader that goes looking for more is answering a different question. + args := []string{"-p", "--mode", "json", "--no-session", "--no-tools", "--system-prompt", system} + if p.Upstream != "" { + args = append(args, "--provider", p.Upstream) + } + if p.Model != "" { + args = append(args, "--model", p.Model) + } + out, err := shell(ctx, "pi", args, user) + if err != nil { + return Answer{}, err + } + return spoken(out) +} + +// spoken reads pi's answer out of its event stream, which is one JSON object +// per line rather than one envelope. The last assistant message is the +// answer; everything before it is the working. +func spoken(out []byte) (Answer, error) { + var ( + answer Answer + said bool + prose []string + ) + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + var event struct { + Type string `json:"type"` + Message struct { + Role string `json:"role"` + Content []struct { + Type string `json:"type"` + Text string `json:"text"` + } `json:"content"` + Usage struct { + Input int `json:"input"` + Output int `json:"output"` + CacheRead int `json:"cacheRead"` + Cost struct { + Total float64 `json:"total"` + } `json:"cost"` + } `json:"usage"` + StopReason string `json:"stopReason"` + ErrorMessage string `json:"errorMessage"` + } `json:"message"` + } + if err := json.Unmarshal([]byte(line), &event); err != nil { + // Whatever pi says outside the stream is the only clue to a + // refusal, so it is kept for the error. + prose = append(prose, line) + continue + } + if event.Type != "message_end" || event.Message.Role != "assistant" { + continue + } + if event.Message.StopReason == "error" { + return Answer{}, fmt.Errorf("%s", orElse(event.Message.ErrorMessage, "the model stopped with an error")) + } + var text strings.Builder + for _, block := range event.Message.Content { + if block.Type == "text" { + text.WriteString(block.Text) + } + } + usage := event.Message.Usage + answer = Answer{ + Text: text.String(), In: usage.Input, Out: usage.Output, + Cached: usage.CacheRead, Cost: usage.Cost.Total, + } + said = true + } + if !said { + return Answer{}, fmt.Errorf("%s", orElse(first(strings.Join(prose, " "), 200), "no assistant message in the answer")) + } + return answer, nil +} + +// Command asks whatever the environment names, for a provider this tool has +// never heard of. REVIEW_COMMAND is the command line; REVIEW_COMMAND_FIELD is +// the JSON field its answer arrives in, where it answers in JSON at all. +type Command struct { + Argv []string + Field string +} + +// Name carries the command line, because two commands under the same name +// are different providers as far as the cache is concerned. +func (c Command) Name() string { + if len(c.Argv) == 0 { + return "command" + } + return "command: " + strings.Join(c.Argv, " ") +} + +func (c Command) Ask(ctx context.Context, system, user string) (Answer, error) { + if len(c.Argv) == 0 { + return Answer{}, fmt.Errorf("REVIEW_COMMAND names no command") + } + out, err := shell(ctx, c.Argv[0], c.Argv[1:], system+"\n\n"+user) + if err != nil { + return Answer{}, err + } + if c.Field == "" { + return Answer{Text: string(out)}, nil + } + return Answer{Text: field(out, c.Field)}, nil +} + +// shell works a command with the prompt on its input, which every assistant +// here accepts and which keeps a long prompt out of the argument list. +func shell(ctx context.Context, name string, args []string, prompt string) ([]byte, error) { + cmd := exec.CommandContext(ctx, name, args...) + cmd.Stdin = strings.NewReader(prompt) + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + detail := strings.TrimSpace(stderr.String()) + if detail == "" { + detail = strings.TrimSpace(string(out)) + } + if len(detail) > 400 { + detail = detail[:400] + } + return nil, fmt.Errorf("%s: %w: %s", name, err, detail) + } + return out, nil +} + +// field pulls the answer out of a JSON envelope, taking the first of the +// names that holds a string. An answer that is not JSON at all is returned +// whole, since plenty of commands simply print what they were asked for. +func field(out []byte, names ...string) string { + var envelope map[string]any + if err := json.Unmarshal(out, &envelope); err != nil { + return string(out) + } + for _, name := range names { + if text, ok := envelope[name].(string); ok && text != "" { + return text + } + } + return string(out) +} + +// object finds the JSON object in an answer. Only a provider that can enforce +// a schema returns bare JSON; the rest wrap it in whatever they were minded +// to say, so the object is taken from within the text. +func object(text string) (string, error) { + start := strings.Index(text, "{") + end := strings.LastIndex(text, "}") + if start < 0 || end < start { + return "", fmt.Errorf("no findings object in the answer") + } + return text[start : end+1], nil +} diff --git a/provider_test.go b/provider_test.go @@ -0,0 +1,363 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestObject(t *testing.T) { + for _, test := range []struct { + name string + answer string + want string + }{{ + name: "bare json is returned whole", + answer: `{"findings":[]}`, + want: `{"findings":[]}`, + }, { + name: "a fenced answer gives up its object", + answer: "Here is what I found:\n```json\n{\"findings\":[{\"rule\":\"cannot-fail\"}]}\n```\nHope that helps.", + want: `{"findings":[{"rule":"cannot-fail"}]}`, + }, { + name: "prose on both sides is stripped", + answer: "I reviewed it. {\"findings\":[]} Let me know.", + want: `{"findings":[]}`, + }, { + name: "nested braces survive", + answer: `prefix {"findings":[{"rule":"a"},{"rule":"b"}]} suffix`, + want: `{"findings":[{"rule":"a"},{"rule":"b"}]}`, + }} { + t.Run(test.name, func(t *testing.T) { + got, err := object(test.answer) + if err != nil { + t.Fatal(err) + } + if got != test.want { + t.Errorf("got %q, want %q", got, test.want) + } + }) + } +} + +func TestObjectWithNothingToFind(t *testing.T) { + for _, answer := range []string{"", "I could not answer that.", "}{"} { + if _, err := object(answer); err == nil { + t.Errorf("object(%q) should fail", answer) + } + } +} + +func TestField(t *testing.T) { + // The first name holding a string wins. + got := field([]byte(`{"text":"","result":"answer","content":"other"}`), "result", "text", "content") + if got != "answer" { + t.Errorf("got %q", got) + } + // An empty value is passed over, which is what lets one set of names + // serve several envelopes. + got = field([]byte(`{"result":"","text":"answer"}`), "result", "text") + if got != "answer" { + t.Errorf("got %q", got) + } + // Plenty of commands simply print what they were asked for. + if got := field([]byte("just text"), "result"); got != "just text" { + t.Errorf("got %q", got) + } + // JSON that holds none of the names is returned whole rather than lost. + raw := `{"other":"answer"}` + if got := field([]byte(raw), "result"); got != raw { + t.Errorf("got %q", got) + } +} + +func TestProvidersAreNamed(t *testing.T) { + providers := Providers() + if len(providers) == 0 { + t.Fatal("Providers() returned no providers") + } + for name, build := range providers { + // A name is the start of what the provider calls itself: the model + // is carried as well, because the answer cache keys on the pair. + if got := build("").Name(); !strings.HasPrefix(got, name) { + t.Errorf("provider %q calls itself %q", name, got) + } + } +} + +func TestNamesCarryTheModel(t *testing.T) { + providers := Providers() + if len(providers) == 0 { + t.Fatal("Providers() returned no providers") + } + for name, build := range providers { + got := build("probe-model").Name() + // The command provider is named by its command line rather than its + // model, which it does not take; its argv is the identity that two + // commands under one name would collide on. + if name == "command" { + continue + } + // The model is carried after the provider's own name: a name that + // echoed the model alone would not say which provider answered, + // and two providers sharing a model would collide in the cache. + if !strings.HasPrefix(got, name+"/") { + t.Errorf("provider %q names itself %q", name, got) + } + if !strings.Contains(got, "probe-model") { + t.Errorf("provider %q names itself %q without the model", name, got) + } + } + argv := Command{Argv: []string{"my", "reviewer"}}.Name() + if argv != "command: my reviewer" { + t.Errorf("got %q", argv) + } +} + +func TestOrElse(t *testing.T) { + if got := orElse("", "fallback"); got != "fallback" { + t.Errorf("got %q", got) + } + if got := orElse("value", "fallback"); got != "value" { + t.Errorf("got %q", got) + } +} + +// Command is the seam for a provider this tool has never heard of, so it is +// exercised with a command that is certainly on the path. +func TestCommandProvider(t *testing.T) { + c := Command{Argv: []string{"sh", "-c", `cat > /dev/null; printf '{"findings":[]}'`}} + answer, err := c.Ask(context.Background(), "system", "user") + if err != nil { + t.Fatal(err) + } + if answer.Text != `{"findings":[]}` { + t.Errorf("got %q", answer.Text) + } +} + +// The prompt reaches the command on its input, which is what keeps a long +// prompt out of the argument list. +func TestCommandProviderIsGivenThePrompt(t *testing.T) { + c := Command{Argv: []string{"cat"}} + answer, err := c.Ask(context.Background(), "the criteria", "the change") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(answer.Text, "the criteria") || !strings.Contains(answer.Text, "the change") { + t.Errorf("got %q", answer.Text) + } +} + +func TestCommandProviderReadsItsField(t *testing.T) { + c := Command{ + Argv: []string{"sh", "-c", `cat > /dev/null; printf '{"answer":"{\\"findings\\":[]}"}'`}, + Field: "answer", + } + answer, err := c.Ask(context.Background(), "", "") + if err != nil { + t.Fatal(err) + } + if answer.Text != `{"findings":[]}` { + t.Errorf("got %q", answer.Text) + } +} + +func TestCommandProviderWithoutACommand(t *testing.T) { + if _, err := (Command{}).Ask(context.Background(), "", ""); err == nil { + t.Error("a provider with no command should say so") + } +} + +// A failing command reports what it printed, since that is the only clue to +// why it failed. +func TestCommandProviderReportsFailure(t *testing.T) { + c := Command{Argv: []string{"sh", "-c", "echo not logged in >&2; exit 1"}} + _, err := c.Ask(context.Background(), "", "") + if err == nil { + t.Fatal("a command that exits non-zero should fail") + } + if !strings.Contains(err.Error(), "not logged in") { + t.Errorf("the reason is missing from %q", err) + } +} + +func TestShellCancels(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if _, err := shell(ctx, "sh", []string{"-c", "sleep 10"}, ""); err == nil { + t.Error("a cancelled context should stop the command") + } +} + +// standIn puts a command of the given name on the path, which answers with +// what it is given and records the arguments and input it was called with. +// The assistants themselves need credentials this machine does not have. +func standIn(t *testing.T, dir, name, answer string) string { + t.Helper() + path := filepath.Join(dir, name) + script := "#!/bin/sh\ncat > \"$0.stdin\"\nprintf '%s' \"$*\" > \"$0.args\"\n" + answer + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return path +} + +// piStream is the shape pi's documented event stream takes: one JSON object +// per line, the last assistant message carrying the answer. +const piStream = `{"type":"session","version":3,"id":"01a0","timestamp":"2026-09-21T16:57:13.552Z","cwd":"/tmp"} +{"type":"agent_start"} +{"type":"turn_start"} +{"type":"message_start","message":{"role":"assistant","content":[]}} +{"type":"message_update","usage":{"input":0,"output":0},"assistantMessageEvent":{"type":"text_delta","contentIndex":0,"delta":"{"}} +{"type":"message_end","message":{"role":"assistant","content":[{"type":"thinking","thinking":"weighing it up"},{"type":"text","text":"{\"findings\":[]}"}],"usage":{"input":1200,"output":40,"cacheRead":900,"cacheWrite":0,"totalTokens":1240,"cost":{"input":0.001,"output":0.002,"cacheRead":0,"cacheWrite":0,"total":0.003}},"stopReason":"stop"}} +{"type":"turn_end","message":{"role":"assistant","content":[]},"toolResults":[]} +{"type":"agent_end","messages":[]} +` + +func TestSpoken(t *testing.T) { + answer, err := spoken([]byte(piStream)) + if err != nil { + t.Fatal(err) + } + if answer.Text != `{"findings":[]}` { + t.Errorf("got %q", answer.Text) + } + // Thinking is not the answer. + if strings.Contains(answer.Text, "weighing") { + t.Errorf("thinking leaked into the answer: %q", answer.Text) + } + if answer.In != 1200 || answer.Out != 40 || answer.Cached != 900 { + t.Errorf("usage: got %+v", answer) + } + if answer.Cost != 0.003 { + t.Errorf("cost: got %v, want the total", answer.Cost) + } +} + +// pi reports a refusal in the message it stopped on, which is the only place +// the reason appears. +func TestSpokenReportsAnError(t *testing.T) { + stream := `{"type":"message_end","message":{"role":"assistant","content":[],"stopReason":"error","errorMessage":"context length exceeded"}}` + _, err := spoken([]byte(stream)) + if err == nil || !strings.Contains(err.Error(), "context length exceeded") { + t.Errorf("got %v", err) + } +} + +// With no credentials pi says so outside the stream, so that is what the +// error has to carry. +func TestSpokenCarriesWhatWasSaidOutsideTheStream(t *testing.T) { + out := `{"type":"session","version":3,"id":"01a0"} +No API key found for the selected model. +` + _, err := spoken([]byte(out)) + if err == nil || !strings.Contains(err.Error(), "No API key found") { + t.Errorf("got %v", err) + } +} + +func TestSpokenWithNothingAtAll(t *testing.T) { + if _, err := spoken(nil); err == nil { + t.Error("an empty answer should fail") + } +} + +// Pi.Ask is exercised against a stand-in on the path, since the assistant +// itself needs credentials this machine does not have. +func TestPiAsk(t *testing.T) { + dir := t.TempDir() + path := standIn(t, dir, "pi", "cat <<'STREAM'\n"+piStream+"STREAM\n") + + p := Pi{Model: "gpt-5", Upstream: "openai"} + answer, err := p.Ask(context.Background(), "the criteria", "the change") + if err != nil { + t.Fatal(err) + } + if answer.Text != `{"findings":[]}` { + t.Errorf("got %q", answer.Text) + } + args, err := os.ReadFile(path + ".args") + if err != nil { + t.Fatal(err) + } + for _, want := range []string{"--mode json", "--no-session", "--no-tools", "--system-prompt the criteria", "--provider openai", "--model gpt-5"} { + if !strings.Contains(string(args), want) { + t.Errorf("%q missing from the arguments: %s", want, args) + } + } + // The prompt goes on the input, not the argument list. + stdin, err := os.ReadFile(path + ".stdin") + if err != nil { + t.Fatal(err) + } + if strings.TrimSpace(string(stdin)) != "the change" { + t.Errorf("got %q on the input", stdin) + } + if strings.Contains(string(args), "the change") { + t.Errorf("the prompt reached the argument list: %s", args) + } +} + +// The tools that answer these jobs are refused rather than left to +// judgement, and the prompt goes on the input where a variadic flag cannot +// swallow it. +func TestClaudeCodeAsk(t *testing.T) { + dir := t.TempDir() + path := standIn(t, dir, "claude", "cat <<'ENVELOPE'\n"+ + `{"result":"{\"findings\":[]}","is_error":false,"total_cost_usd":0.05,`+ + `"usage":{"input_tokens":9,"output_tokens":40,"cache_read_input_tokens":800}}`+ + "\nENVELOPE\n") + + answer, err := ClaudeCode{Model: "sonnet"}.Ask(context.Background(), "the criteria", "the change") + if err != nil { + t.Fatal(err) + } + if answer.Text != `{"findings":[]}` || answer.Cost != 0.05 || answer.Cached != 800 { + t.Errorf("got %+v", answer) + } + args, err := os.ReadFile(path + ".args") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(args), "--disallowedTools Bash,Read,") { + t.Errorf("the tools are not refused: %s", args) + } + // --disallowedTools is variadic and swallows a positional prompt. + if strings.Contains(string(args), "the change") { + t.Errorf("the prompt reached the argument list: %s", args) + } + // --bare sets CLAUDE_CODE_SIMPLE=1 and restricts authentication to an + // API key, which breaks a machine using the logged-in session. + if strings.Contains(string(args), "--bare") { + t.Errorf("--bare is back: %s", args) + } +} + +func TestClaudeCodeReportsAnError(t *testing.T) { + dir := t.TempDir() + standIn(t, dir, "claude", "cat <<'ENVELOPE'\n"+ + `{"result":"Not logged in","is_error":true}`+"\nENVELOPE\n") + + _, err := ClaudeCode{Model: "sonnet"}.Ask(context.Background(), "", "") + if err == nil || !strings.Contains(err.Error(), "Not logged in") { + t.Errorf("got %v", err) + } +} + +// The default is the middle model: the smallest reads the shortlist and +// reports the first duplicate rather than all of them. +func TestDefaultModels(t *testing.T) { + if got := Providers()["claude"]("").(ClaudeCode).Model; got != "sonnet" { + t.Errorf("claude defaults to %q", got) + } + if got := Providers()["api"]("").(API).Model; got != "claude-sonnet-5" { + t.Errorf("api defaults to %q", got) + } + if got := Providers()["claude"]("haiku").(ClaudeCode).Model; got != "haiku" { + t.Errorf("a named model is used, got %q", got) + } +} diff --git a/readme.md b/readme.md @@ -13,6 +13,8 @@ review the staged change review HEAD^..HEAD the last commit review --json for an agent rather than a person review --show what each job would be sent, without asking anything +review --fresh the change again, asking rather than replaying the cache +review --no-verify skip the second reading that verifies the findings ``` ## The jobs @@ -33,6 +35,83 @@ The candidate list the `duplication` job judges is built by parsing the reposito by searching it. A pattern over lines misses an indented constant inside a block, which is exactly where a duplicated fact tends to live. +## The static checks + +Eleven checks run before anything is asked of a provider, and whether or not one can be +asked. Seven measure the commit message; one measures the change against the +repository's history; two hold the review to itself. A twelfth runs the repository's own +analyser when it is installed: staticcheck, on the packages the change touches. + +| check | reads | asks | +|---|---|---| +| `staticcheck/<code>` | the analyser's findings, on the lines the change adds | is the code the analyser argues about? SA faults report as must-fix, unused code and simplifications as consider, style as note | +| `message-low-entropy` | the Shannon entropy of the message, in bits per byte | is this one phrase repeated rather than a description? | +| `message-boilerplate` | the share of its length the message keeps after zlib | is this one block of text pasted whole? | +| `message-common-words` | each word's rarity in the repository's own subject history | is this made only of the words this repository says most, naming nothing in the change? | +| `message-frustration` | the message's words against a short exclamation list | is this the author's reaction — oops, whoops, damn — rather than a description? | +| `message-not-imperative` | the subject's first word after its package prefix | does the subject open as a command — not past tense, not a gerund, not the author? | +| `message-no-body` | the diff's size in changed lines | does a change over 50 lines say anything below the subject at all? | +| `message-long-body` | the body's word count | is the body over 150 words, listing what the diff already shows? | +| `history-coupled-file` | each changed file's co-changes over the last 1,000 commits, as a Jaccard | does history tie this file to a partner the change does not touch — the test beside the code, the header beside the source, the golden file beside the renderer? | +| `suppression-added` | dismissal comments the change adds | is the change dismissing what the readers would have found, before the readers ran? | +| `test-deleted` | test functions the change removes, in Go, TypeScript and JavaScript | is the change deleting the tests that would have failed? | + +The first two are calibrated against the 21,000 commit messages on this machine, and +fire below every one of them: under 3.2 bits per byte, where ordinary messages measure +3.7–4.9; under 20% of length, where none kept less than 26%. Entropy says nothing about +a message under 40 bytes, and zlib nothing about one under 400 — a short subject is +low-entropy whatever it says, and framing dominates the ratio below that. + +The staticcheck check needs the binary on the path and a `go.mod` at the repository +root; without either it says nothing, and a run that cannot finish inside five minutes +is abandoned rather than holding the review. Findings land only on lines the change +adds — the analyser is free to say what it likes about the rest of the repository, and +the review's contract is the change. A range is analysed on the tree the range arrived +at, materialised to a scratch directory. Dismiss one where it is wrong: + +```go +//review:ignore staticcheck/S1002 the comparison states the contract +``` + +The third needs a history to measure against, so it says nothing in a repository +younger than a hundred commits. Against 23,956 measured messages it fires on sixteen, +every one of them a message such as `fix`, `fix ci`, `Fix fix.` — and on nothing else. +A message is spared when any word is rarer than a fifth of the history, when it names +anything the diff adds, removes or touches (`Update requests.ts`), when it carries a +number (`Bump to 2.0.26`), or when it is a merge or squash subject, which is git's +prose rather than the commit's. + +The coupling check is the one that is not about the message: it is counted from +`git log` alone, so it works for every language and sees pairs the compiler cannot — +a frontend component and the test beside it, a C source and its header, a renderer +and its golden files. It is counted over the last thousand commits before the change, +never over the change itself, and commits listing over a hundred files are left out: +a sweep touching everything once says nothing about any pair. It asks about at most +three pairs, and only those that still exist at the end of the change. Measured fire +rate on sampled history: 3.6% of commits at J ≥ 0.7 with at least 5 shared commits — +12% at J ≥ 0.5, which is why the threshold sits where it does. + +The last two are about gaming the review itself. `suppression-added` reads every +dismissal comment the change adds — a suppression that the readers have not seen yet, +nameable before they run — and reports it as must-fix; the report carries no file and +so cannot be dismissed itself. The prose that documents the mechanism does not count, +and neither does a comment in a file no reader reads. `test-deleted` reads the test +functions the change removes and reports each file that lost one, must-fix; a test +that was renamed is spared, judged name-word by name-word, and a deleted test that +survived as a new test elsewhere in the change is spared the same way. + +They catch a large change that never explains itself — unless the diff only moves +text around, which its subject describes — a body that explains past its point, +text repeated or pasted — `asdf asdf asdf`, a licence notice, a dumped log +— a message made only of the repository's usual words, and a message that is only an +exclamation. Words that history shows describing code legitimately are not markers: +Odin says `stupid UB` and implements `dumb PtrMap`, and `finally` and `eventually` +belong to real prose. Verbs whose past and imperative share a form — read, +set, split — are spared, as are articles, because an explainer may be a noun +phrase on purpose. They do not catch fluent emptiness: `do the needful` and +`updated the thing` measure like prose and their words are rare precisely because +nobody writes them, so no frequency count will hold them. That is the `hygiene` job's. + ## Criteria `criteria/*.md` holds the rules, one file per job, each rule with an id. **Every finding @@ -52,15 +131,132 @@ Not a configuration file. The reason belongs beside the code it justifies, where reader meets it, and it survives a clone. `//review:ignore all <why>` dismisses everything at that spot. -## Running it +A dismissal reaches a few lines either side of itself, so one can answer a finding you +did not mean it to. `--verbose` names each finding it dismissed and the reason given. +The one dismissal that cannot be written is one that answers the suppression check +itself: a finding with no file behind it is not dismissible by anyone. + +## A second reading verifies + +After every job has answered, each job with findings is asked once more, with its +findings listed and the same evidence it read the first time, and answers for each: it +holds, or it does not, with a reason. A finding that falls is reported as a retraction +rather than deleted — the first reading's word and the second's are both on the record, +with the reason. The pass is advisory like the rest: a job that cannot be asked again +leaves its findings standing, marked unverified, and reports why. + +A finding the verdict list omits stands rather than falls, marked verified: a strict +pass would let one dropped number retract everything the reading found. `--no-verify` +skips the pass — useful when the answer itself is being tested, since then the review is +one ask per job, deterministic against a frozen provider. + +Retractions are verified like everything else: each retracted finding carries an id, so +an agent can answer it — restore the test, rewrite the comment — and read the next run +to confirm the finding stayed retracted. + +## The answer cache + +Every answer is keyed by the provider and the exact prompts that produced it, and +recorded under `os.UserCacheDir()/review/answers.json` (usually +`~/.cache/review/answers.json`). A second run of the same change asks nothing: it +replays the recorded answers, reports `replayed` in the usage instead of token counts, +and returns the same findings — ids and verdicts included. That is what makes a loop +deterministic: the finding and the retraction come back the same way until the change, +the criteria or the model changes. + +`--fresh` asks rather than replays, and records what it learned, so the cache is +replaced rather than grown stale. The cache holds the 4,000 newest answers; a file it +cannot read is replaced on the next save, and a prompt the cache cannot answer is asked +the ordinary way. -Needs credentials for the Anthropic API — `ant auth login`, or `ANTHROPIC_API_KEY`. +## The JSON contract + +`--json` writes one object, so an agent can read the whole measurement without parsing +prose: + +```json +{ + "version": 1, + "provider": "pi/maple/glm-5-3-flash", + "status": "complete", + "findings": [], + "retracted": [], + "failed": [], "skipped": [], "uncovered": [], "dismissed": [], + "truncated": false, + "usage": {"in": 3327, "out": 15550, "cached": 0, "replayed": 5, "usd": 0} +} +``` + +`status` is what keeps an empty findings list from being read as a pass: `empty` (no +change), `complete` (every job answered, every file read), or `incomplete` — when a job +failed, a file no reader could read, or the diff was cut to fit. A finding carries its +`id` (stable across runs while the finding stands), `verified` (only the verify pass can +say true; static checks are their own word), `severity`, and the `rule` id from the +criteria it cites. A retraction carries the finding it ends and the verdict's `reason`. +`usage` is always present, and counts `replayed` answers rather than tokens when the +answers were cached; `usd` is 0 unless the provider reports a cost. + +## Who answers + +Nothing here cares which model answers. A provider takes a prompt and returns text. + +| `--provider` | how | credentials | +|---|---|---| +| `chain` (default) | probes the configured order — the console API first, then `pi` against the local gateway — and the first that answers serves the whole reading | whichever entry it lands on holds | +| `claude` | the coding assistant on the path, `-p --output-format json` | whatever it already holds | +| `api` | the console API directly | `ant auth login` or `ANTHROPIC_API_KEY` | +| `pi` | `pi -p --mode json`, which speaks to several providers of its own | its own; name the upstream in `REVIEW_PI_PROVIDER` | +| `command` | anything at all: `REVIEW_COMMAND` is the command line, `REVIEW_COMMAND_FIELD` the JSON field its answer arrives in | its own | + +`--model` names the model in whatever form that provider uses. `REVIEW_PROVIDER` and +`REVIEW_MODEL` set the defaults. + +The chain is for the hours when a limit is spent: one cheap ask per skipped entry, +the reason on stderr, and the reading happens on whatever still answers. `REVIEW_SERIAL` +asks a provider's jobs one at a time, for providers that cannot take concurrent reads. + +The default model is the middle one, not the smallest. Against the eval set the +smallest reads the shortlist of duplicates and reports the first one it finds rather +than all of them — roughly two thirds of the wanted findings, and never both of a pair +in one reading. That is the one thing this tool is for, so it is not a saving. + +Only the `api` provider can enforce the answer's shape, through a strict tool schema. +The rest are asked for JSON in the prompt and the object is taken out of whatever they +wrap it in, so every provider answers the same way whether or not it can be held to it. +It also pins sampling, which the others cannot: a loop between two models converges +faster when one of them answers the same way twice. + +Measured, one job over a seven-file commit through `claude`: **$0.19 at the default +model**, $0.06 at the smallest. A whole change, all five jobs, is a few times that. The +same packet through `api` costs a fraction of a cent — the difference is the +assistant's own harness, which these jobs do not use. Convenience has a price and this +is it. + +Prompt caching does work through `claude`, but what it caches is that harness: about +22,000 tokens, the same count on every job however different their criteria. The +criteria themselves are appended after it and are not cached. The `input_tokens` that +provider reports is 9 whatever it was sent, so `total_cost_usd` is the only figure from +it worth reading. + +## The eval set + +`eval_test.go` holds the cases with a known right answer — two revisions of a corpus +repository and three written for the purpose — and scores a reading against them. It is +off unless asked for, because it is the only thing here that spends money: + +``` +go test -run TestEval -eval=1 # one reading of each case +go test -run TestEval -eval=3 -eval.show # three, printing every finding +go test -run TestEval -eval=1 -eval.cases=duplication +``` -Runs on Claude Haiku 4.5 at temperature 0. Pinned sampling matters: a loop between two -models cannot converge if one of them answers differently each time it is asked. +`REVIEW_EVAL_CORPUS` names the repository the corpus cases read; they skip without it. +More than one run per case is worth having: a single reading walks between the two +failure directions rather than sitting at one. -Findings arrive through a strict tool schema rather than as prose to be parsed, so a -malformed answer is impossible rather than merely unlikely. +A case says what a reading must find and what it must not raise. The silences matter +more: the persistent failure is reporting something true and unwanted, and every rule +in `criteria/` that reads like an exception was written to stop one. ## What it is not diff --git a/sidecar/odin/.gitignore b/sidecar/odin/.gitignore @@ -0,0 +1 @@ +odin-review-extract diff --git a/sidecar/odin/main.odin b/sidecar/odin/main.odin @@ -0,0 +1,216 @@ +// odin-review-extract prints the declarations of the Odin files it is given, +// as JSON, using Odin's own parser (core:odin). One line per invocation: +// +// odin-review-extract file.odin ... > decls.json +// +// where the JSON is {"files":[{"name":"a.odin","decls":[ +// {"name":"main","kind":"func","line":7,"end_line":9, +// "exported":true,"test":false,"text":"...","doc":"..."}]}]} +// +// Odin has no export keyword: a package-level declaration is the package's +// API, so it is reported exported unless marked @(private=...). +package main + +import "core:encoding/json" +import "core:fmt" +import "core:odin/ast" +import "core:odin/parser" +import "core:odin/tokenizer" +import "core:os" +import "core:strings" + +Decl :: struct { + name: string, + kind: string, + line: int, + end_line: int, + exported: bool, + test: bool, + text: string, + doc: string, +} + +File_Result :: struct { + name: string, + decls: [dynamic]Decl, +} + +Output :: struct { + files: [dynamic]File_Result, +} + +main :: proc() { + args := os.args[1:] + out := Output{} + for arg in args { + data, err := os.read_entire_file_from_path(arg, context.allocator) + if err != nil { + continue + } + append(&out.files, read_file(arg, data)) + } + data, err := json.marshal(out, json.Marshal_Options{pretty = true, use_spaces = true}) + if err != nil { + fmt.eprintf("odin-review-extract: %v\n", err) + os.exit(1) + } + fmt.print(string(data)) +} + +read_file :: proc(path: string, src: []byte) -> File_Result { + result := File_Result{name = path} + file := new(ast.File) + file.fullpath = path + file.src = string(src) + + p := parser.default_parser() + parser.parse_file(&p, file) + + for stmt in file.decls { + decl, ok := stmt.derived_stmt.(^ast.Value_Decl) + if !ok { + continue // Imports and foreign blocks are not this file's work. + } + private := is_private(decl.attributes[:]) + for name_expr in decl.names { + ident, ok := name_expr.derived_expr.(^ast.Ident) + if !ok { + continue + } + entry := Decl{ + name = ident.name, + kind = kind_of(decl), + line = int(name_expr.pos.line), + end_line = int(stmt.end.line), + exported = private == false, + text = line_text(src, name_expr.pos.line), + doc = doc_text(decl.docs), + } + if kind_of(decl) == "func" { + entry.test = is_test(decl.attributes[:]) + } + append(&result.decls, entry) + } + } + return result +} + +// kind_of reports what a value declaration declares: a procedure, a type, or +// a value. The type sits on the declaration, or in its only value when the +// declaration is `name :: thing`. +kind_of :: proc(decl: ^ast.Value_Decl) -> string { + kind := "value" + if decl.type != nil { + kind = expr_kind(decl.type.derived_expr) + } + for value in decl.values { + if k := expr_kind(value.derived_expr); k != "value" { + return k + } + } + return kind +} + +expr_kind :: proc(e: ast.Any_Expr) -> string { + #partial switch v in e { + case ^ast.Proc_Lit: + return "func" + case ^ast.Proc_Group: + return "func" + case ^ast.Struct_Type, ^ast.Union_Type, ^ast.Enum_Type, + ^ast.Bit_Set_Type, ^ast.Distinct_Type, ^ast.Poly_Type, + ^ast.Typeid_Type, ^ast.Pointer_Type, ^ast.Array_Type, + ^ast.Dynamic_Array_Type, ^ast.Fixed_Capacity_Dynamic_Array_Type, + ^ast.Map_Type, ^ast.Relative_Type, ^ast.Matrix_Type, + ^ast.Bit_Field_Type: + return "type" + case: + return "value" + } + return "value" +} + +// is_test reports the @(test) attribute, which the testing package runs. +is_test :: proc(attributes: []^ast.Attribute) -> bool { + for attribute in attributes { + for elem in attribute.elems { + #partial switch v in elem.derived_expr { + case ^ast.Ident: + if v.name == "test" { + return true + } + case ^ast.Field_Value: + if field, ok := v.field.derived_expr.(^ast.Ident); ok && field.name == "test" { + return true + } + } + } + } + return false +} + +// is_private reports an @(private=...) attribute, which keeps a declaration +// out of the package's public surface. +is_private :: proc(attributes: []^ast.Attribute) -> bool { + for attribute in attributes { + for elem in attribute.elems { + if field, ok := elem.derived_expr.(^ast.Field_Value); ok { + if name, ok := field.field.derived_expr.(^ast.Ident); ok && name.name == "private" { + return true + } + } + } + } + return false +} + +doc_text :: proc(group: ^ast.Comment_Group) -> string { + if group == nil { + return "" + } + parts: [dynamic]string + for token in group.list { + t := strings.trim_space(token.text) + switch { + case strings.has_prefix(t, "///"): + t = strings.trim_prefix(t, "///") + case strings.has_prefix(t, "//"): + t = strings.trim_prefix(t, "//") + case strings.has_prefix(t, "/*"): + t = strings.trim_suffix(strings.trim_prefix(t, "/*"), "*/") + for raw in strings.split(t, "\n") { + line := strings.trim_space(raw) + line = strings.trim_prefix(line, "*") + line = strings.trim_space(line) + if len(line) > 0 { + append(&parts, line) + } + } + continue + } + t = strings.trim_space(t) + if len(t) > 0 { + append(&parts, t) + } + } + defer delete(parts) + return strings.join(parts[:], " ") +} + +line_text :: proc(src: []byte, line: int) -> string { + current := 1 + start := 0 + for i := 0; i < len(src); i += 1 { + if src[i] == '\n' { + if current == line { + return strings.trim_space(string(src[start:i])) + } + current += 1 + start = i + 1 + } + } + if current == line { + return strings.trim_space(string(src[start:])) + } + return "" +} diff --git a/static.go b/static.go @@ -0,0 +1,635 @@ +package main + +// The checks here need no model. They measure the commit message and report +// what fails, before anything is asked of a provider and whether or not one +// can answer. A check that passes says nothing. + +import ( + "bytes" + "cmp" + "compress/zlib" + "fmt" + "math" + "path/filepath" + "slices" + "strings" + "unicode" +) + +// Check measures a change and reports what it noticed. +type Check func(*Change) []Finding + +// Checks are the deterministic readings, each independent of the others. +func Checks() []Check { + // The last two hold the review to itself: the mechanisms a change could + // use to pass it are measured rather than trusted absent. + return []Check{checkEntropy, checkCompressibility, checkCommon, checkVenting, checkMood, checkBody, checkTemporal, checkSuppressionAdded, checkDeletedTests} +} + +const ( + // minEntropy is the Shannon entropy, in bits per byte, under which a + // message is one short phrase said over rather than a description of a + // change. Ordinary commit messages measure between 3.7 and 4.9. + minEntropy = 3.2 + + // entropyFloor is the message length under which entropy says nothing: a + // subject of a dozen characters is low-entropy whatever it says. + entropyFloor = 40 + + // maxCompression is the share of its length a message keeps after zlib, + // under which the message is one block of text pasted or repeated whole. + // No written commit message measured keeps less than 0.26. + maxCompression = 0.20 + + // compressionFloor is the message length under which zlib cannot beat its + // own framing, and the ratio means nothing. + compressionFloor = 400 + + // commonIDF is the natural log under which a word counts as one of the + // repository's commonest: a word this common appears in at least a fifth + // of the repository's commit subjects, however many there are. + commonIDF = 1.6 + + // historyFloor is the number of commit subjects under which the word + // frequencies are too thin to judge a message by, and the check says + // nothing. + historyFloor = 100 + + // digits are the characters whose presence spares a message: a version + // or an issue number is information however few words carry it. + digits = "0123456789" + + // bodyFloorLines is the size of diff, in changed lines, under which a + // body is optional: the subject may say enough by itself. + bodyFloorLines = 50 + + // maxBodyWords is the length, in words, over which a body is listing + // what the diff already shows rather than explaining the change. + maxBodyWords = 150 + + // temporalCoupling is the Jaccard over which history says two files + // change together: over the counted commits, at least this share of the + // commits touching either file touches both. Measured fire rates: 12% + // of sampled commits at 0.5, 3.6% at 0.7, and the pairs the higher + // threshold keeps are the changed-without-its-test kind. + temporalCoupling = 0.7 + + // temporalSupport is the fewest commits two files must share before + // their history is a pattern rather than a coincidence. + temporalSupport = 5 + + // temporalFindings caps how many pairs one change is asked about, so a + // change grazing five coupled files starts one conversation, not five. + temporalFindings = 3 +) + +// ventingWords are the exclamations and profanities of a message written in +// the moment of the mistake rather than about the change. Every word here +// is unambiguous: stupid, dumb, finally, eventually and annoying all appear +// in measured history describing the code legitimately, so they are not +// here. +var ventingWords = map[string]bool{ + "fuck": true, "fucking": true, "fucked": true, + "shit": true, "bullshit": true, "wtf": true, + "damn": true, "dammit": true, "damnit": true, + "oops": true, "oopsie": true, "whoops": true, + "ugh": true, "argh": true, "grr": true, "sigh": true, + "fml": true, "yolo": true, "idk": true, +} + +// runChecks applies every check. It is called before any provider is built, +// so what the checks find is reported whether or not a model can be asked. +func runChecks(change *Change) []Finding { + var out []Finding + for _, check := range Checks() { + out = append(out, check(change)...) + } + return out +} + +// checkEntropy reports a message whose characters carry too little entropy: +// the shape of a placeholder, a keyboard mash, or one phrase repeated. +func checkEntropy(c *Change) []Finding { + msg, note := measured(c) + if len(msg) < entropyFloor { + return nil + } + h := shannonEntropy(msg) + if h >= minEntropy { + return nil + } + return []Finding{{ + Job: "static", Rule: "message-low-entropy", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message measures %.1f bits of Shannon entropy per byte, under the %.1f beneath which a message is a phrase repeated rather than a description; ordinary messages measure 3.7 to 4.9%s", + h, minEntropy, note), + Fix: "write a message that says what the change does and why", + }} +} + +// checkCompressibility reports a message zlib keeps only a fraction of: the +// shape of text pasted or repeated wholesale, such as a licence notice or a +// log, rather than prose written for this change. +func checkCompressibility(c *Change) []Finding { + msg, note := measured(c) + if len(msg) < compressionFloor { + return nil + } + ratio := compressionRatio(msg) + if ratio >= maxCompression { + return nil + } + return []Finding{{ + Job: "static", Rule: "message-boilerplate", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message compresses to %.0f%% of its length, under the %.0f%% beneath which it is one block of text pasted or repeated rather than prose about the change; no written message measured keeps less than 26%%%s", + 100*ratio, 100*maxCompression, note), + Fix: "keep only what the reader needs of the quoted text, and write the rest", + }} +} + +// checkCommon reports a message made entirely of the words this +// repository's own history uses most, which names nothing the change +// touches. A word the history has never used is the one thing a message +// like this cannot have, which is why a message holding any rarer word is +// left to the reader. +func checkCommon(c *Change) []Finding { + msg, note := measured(c) + if len(c.History) < historyFloor { + return nil + } + words := contentWords(msg) + if len(words) == 0 { + return nil + } + if strings.HasPrefix(msg, "Merge ") || strings.HasPrefix(msg, "Squashed ") || + strings.ContainsAny(msg, digits) { + return nil + } + freq := frequencies(c.History) + n := len(c.History) + for w := range words { + if math.Log(float64(n)/float64(1+freq[w])) > commonIDF { + return nil + } + } + ground := diffWords(c) + for w := range words { + if ground[w] { + return nil + } + } + listed := make([]string, 0, len(words)) + for w := range words { + listed = append(listed, w) + } + slices.Sort(listed) + return []Finding{{ + Job: "static", Rule: "message-common-words", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message is made of the repository's commonest commit words — %s — with nothing rarer than a fifth of its %d commit subjects, and it names nothing the change touches; a word the history has not used is the one thing a message like this cannot have%s", + strings.Join(listed, ", "), n, note), + Fix: "name the part and the fault, in words the change itself uses", + }} +} + +// checkVenting reports a message whose words are the author's reaction +// rather than the change's description: oops, whoops, damn, profanity. +func checkVenting(c *Change) []Finding { + msg, note := measured(c) + var hit []string + for _, piece := range fields(msg) { + w := strings.ToLower(piece) + if ventingWords[w] { + hit = append(hit, w) + } + } + if len(hit) == 0 { + return nil + } + slices.Sort(hit) + hit = slices.Compact(hit) + return []Finding{{ + Job: "static", Rule: "message-frustration", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message is an exclamation — %s — where a description should be; the log then records the author's feeling, and the change goes undescribed%s", + strings.Join(hit, ", "), note), + Fix: "describe the change, not the moment", + }} +} + +// checkMood reports a subject that does not open as a command. The +// discipline is package: explainer, with the explainer in the imperative +// mood: a subject that opens in the past tense or on a gerund records that +// a thing was done, and one that opens on the author narrates the author. +// The suffix rules cannot hear an adjective from a past participle, and +// verbs that wear one form for every mood are spared, as are articles: an +// explainer may be a noun phrase on purpose, as the tool's own subjects +// are. +func checkMood(c *Change) []Finding { + msg, note := measured(c) + subject := strings.SplitN(msg, "\n", 2)[0] + if strings.HasPrefix(subject, "Merge ") || strings.HasPrefix(subject, "Squashed ") { + return nil + } + words := fields(explainer(subject)) + if len(words) == 0 { + return nil + } + first, quoted := strings.ToLower(words[0]), words[0] + what := "" + switch { + case openers[first]: + what = "narration" + case invariantVerbs[first]: + return nil + case irregularPast[first] || (len(first) >= 4 && strings.HasSuffix(first, "ed") && + !strings.HasSuffix(first, "eed")): + what = "the past tense" + case len(first) >= 5 && strings.HasSuffix(first, "ing") && !nonVerbIng[first]: + what = "a gerund" + default: + return nil + } + return []Finding{{ + Job: "static", Rule: "message-not-imperative", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message opens on %s — %q — where the discipline is a command: package: explainer, with the explainer in the imperative mood; a subject that opens in the past tense, on a gerund, or on the author records what was done rather than saying what to do%s", + what, quoted, note), + Fix: "open the subject on its verb, in the imperative", + }} +} + +// explainer is the part of a subject after its package prefix: explainer is +// the part after "review:" in "review: measure it". A subject without a +// lowercase prefix is all explainer. +func explainer(subject string) string { + i := strings.Index(subject, ": ") + if i < 2 || i > 23 { + return subject + } + for _, r := range subject[:i] { + if !('a' <= r && r <= 'z' || '0' <= r && r <= '9' || r == '-' || r == '_') { + return subject + } + } + return subject[i+2:] +} + +// openers are the words whose presence alone is narration: the author as +// subject, and the demonstratives of "this fixes". +var openers = map[string]bool{ + "i": true, "we": true, "my": true, + "this": true, "these": true, "those": true, +} + +// invariantVerbs are the verbs whose past and imperative share a form, such +// as read and set: a subject opening on one is a command until the +// sentence says otherwise, and the suffix rules cannot hear which. +var invariantVerbs = map[string]bool{ + "read": true, "cut": true, "set": true, "put": true, "let": true, + "hit": true, "cost": true, "split": true, "shut": true, "cast": true, + "hurt": true, "quit": true, "burst": true, "spread": true, "slit": true, +} + +// irregularPast are the past forms that no suffix rule could catch, which +// cannot be imperatives because their imperatives are other words. +var irregularPast = map[string]bool{ + "wrote": true, "made": true, "kept": true, "went": true, "got": true, + "ran": true, "brought": true, "built": true, "bought": true, "caught": true, + "drove": true, "found": true, "held": true, "left": true, "met": true, + "paid": true, "sent": true, "spent": true, "took": true, "won": true, + "sold": true, "freed": true, +} + +// nonVerbIng are the words that end in ing without being a verb's gerund. +var nonVerbIng = map[string]bool{ + "during": true, "nothing": true, "something": true, "anything": true, + "everything": true, "morning": true, "evening": true, "offing": true, + "outing": true, "bring": true, "king": true, "ring": true, "sing": true, + "spring": true, "string": true, "swing": true, "thing": true, "wing": true, + "cling": true, "sting": true, "fling": true, +} + +// checkBody asks a large change to say something in its body: a diff of +// more than bodyFloorLines owes a body, however short, and no body may +// exceed maxBodyWords. The diff records what moved; the body is the only +// place the change's why is recorded. The diff's size is a weak measure of +// how much a change needs saying — a small diff can hold the subtle fault, +// a large one can be a formatting run — so the check asks only for +// existence, and a change that only moves text around, whose subject says +// what it did, owes nothing. Subjects of merges and squashes are git's +// words, not the commit's. +func checkBody(c *Change) []Finding { + msg, note := measured(c) + subject := strings.SplitN(msg, "\n", 2)[0] + if strings.HasPrefix(subject, "Merge ") || strings.HasPrefix(subject, "Squashed ") { + return nil + } + n := bodyWords(msg) + if n > maxBodyWords { + return []Finding{{ + Job: "static", Rule: "message-long-body", Severity: MustFix, + Message: fmt.Sprintf( + "the commit message's body holds %d words, over the %d the discipline allows; past that a body lists what the diff already shows, and the reader of the log stops before the why%s", + n, maxBodyWords, note), + Fix: "cut the body to the change's why", + }} + } + if n > 0 { + return nil + } + changed := changedLines(c.Diff) + if changed <= bodyFloorLines || moved(c.Diff) { + return nil + } + return []Finding{{ + Job: "static", Rule: "message-no-body", Severity: MustFix, + Message: fmt.Sprintf( + "the change is %d lines of diff, and the commit message carries no body; the diff records what moved, and the body is the only place the change's why is recorded%s", + changed, note), + Fix: "write the body, saying why the change is what it is", + }} +} + +// checkTemporal reports a changed file whose history names a partner the +// change does not touch: over the last thousand commits before the change, +// at least temporalCoupling of the commits touching either file have +// touched both, at least temporalSupport times. A coupling like that is +// not the compiler's to see, and only the author can say whether the pair +// still changes together; files that change together this reliably usually +// fail together. +func checkTemporal(c *Change) []Finding { + if c.Temporal == nil { + return nil + } + changed := map[string]bool{} + for _, f := range c.Files { + changed[f] = true + } + type pair struct { + file string + partner Partner + union int + j float64 + } + var pairs []pair + for file, partners := range c.Temporal.Partners { + for _, p := range partners { + if changed[p.Name] { + continue + } + union := c.Temporal.Commits[file] + c.Temporal.Commits[p.Name] - p.Shared + if union <= 0 { + continue + } + j := float64(p.Shared) / float64(union) + if j < temporalCoupling { + // The list is nearest first, so the rest of this file's + // partners are further away still. + break + } + if p.Shared < temporalSupport { + continue + } + pairs = append(pairs, pair{file, p, union, j}) + } + } + slices.SortFunc(pairs, func(a, b pair) int { + if c := cmp.Compare(b.partner.Shared, a.partner.Shared); c != 0 { + return c + } + if c := cmp.Compare(b.j, a.j); c != 0 { + return c + } + if c := cmp.Compare(a.file, b.file); c != 0 { + return c + } + return cmp.Compare(a.partner.Name, b.partner.Name) + }) + if len(pairs) > temporalFindings { + pairs = pairs[:temporalFindings] + } + findings := make([]Finding, 0, len(pairs)) + for _, p := range pairs { + findings = append(findings, Finding{ + Job: "static", Rule: "history-coupled-file", Severity: MustFix, + File: p.file, + Message: fmt.Sprintf( + "history ties %s to %s: %d of the %d commits touching either file have touched both, and this change touches %s without %s; files that change together this reliably usually fail together, and the half of the pair left out is where a forgotten change usually is", + p.file, p.partner.Name, p.partner.Shared, p.union, p.file, p.partner.Name), + Fix: fmt.Sprintf("touch %s too, or be sure it stands without this change", p.partner.Name), + }) + } + return findings +} + +// moved is whether a diff's added and removed sides hold the same lines: +// the change rearranged text rather than changing it. Whitespace may be +// load-bearing in a few languages; the exemption errs toward silence, as +// the others do. +func moved(diff string) bool { + counts := map[string]int{} + n := 0 + for _, line := range strings.Split(diff, "\n") { + if !strings.HasPrefix(line, "+") && !strings.HasPrefix(line, "-") { + continue + } + if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { + continue + } + s := strings.TrimSpace(line[1:]) + if s == "" { + continue + } + n++ + if strings.HasPrefix(line, "+") { + counts[s]++ + } else { + counts[s]-- + } + } + for _, v := range counts { + if v != 0 { + return false + } + } + return n > 0 +} + +// bodyWords counts the words below the subject line. The staged note is not +// the commit's words and is already gone from the measured message. +func bodyWords(msg string) int { + parts := strings.SplitN(msg, "\n", 2) + if len(parts) == 1 { + return 0 + } + return len(fields(parts[1])) +} + +// changedLines counts the lines a diff adds or removes, without the diff's +// own framing. A diff truncated by the packet's cap undercounts, which +// asks less of the body and errs toward silence. +func changedLines(diff string) int { + n := 0 + for _, line := range strings.Split(diff, "\n") { + if !strings.HasPrefix(line, "+") && !strings.HasPrefix(line, "-") { + continue + } + if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") { + continue + } + n++ + } + return n +} + +// contentWords is the vocabulary of a text: lowercased, split on anything +// that is not a letter or digit, split again at camel humps, depluralised, +// and stripped of the words that name nothing. +func contentWords(text string) map[string]bool { + out := map[string]bool{} + for _, piece := range fields(text) { + for _, part := range humps(piece) { + w := depluralise(strings.ToLower(part)) + if len(w) > 2 && !stopWords[w] { + out[w] = true + } + } + } + return out +} + +// fields splits on anything that is not a letter or a digit. +func fields(s string) []string { + return strings.FieldsFunc(s, func(r rune) bool { + return !unicode.IsLetter(r) && !unicode.IsDigit(r) + }) +} + +// stopWords are the function words, which name nothing: both a message and +// the change it describes are full of them. +var stopWords = map[string]bool{} + +func init() { + for _, w := range strings.Fields(`a an the and or but if then than that this these those + to of in on at by for with from into up out it its is are was were be been being am + as so not no nor do does did done doing can could will would shall should may might + must have has had having i we you they he she him her them his hers their ours your + my me us what which who whom when where why how all any both each few more most + other some such only own same too very just also now here there over under again + further once about between through during before after above below because while + until against`) { + stopWords[w] = true + } +} + +// humps splits containerSniff into container and sniff, so that a message +// naming a thing meets the identifier for it in the diff. +func humps(s string) []string { + var out []string + var cur strings.Builder + rs := []rune(s) + for i, r := range rs { + if i > 0 && unicode.IsUpper(r) && unicode.IsLower(rs[i-1]) { + out = append(out, cur.String()) + cur.Reset() + } + cur.WriteRune(r) + } + if cur.Len() > 0 { + out = append(out, cur.String()) + } + return out +} + +// depluralise drops a trailing s, except where dropping it would leave +// another: class stays class. +func depluralise(w string) string { + if len(w) > 3 && strings.HasSuffix(w, "s") && w[len(w)-2] != 's' { + return w[:len(w)-1] + } + return w +} + +// frequencies counts, over the repository's subjects, in how many of them +// each content word appears. +func frequencies(history []string) map[string]int { + out := map[string]int{} + for _, s := range history { + for w := range contentWords(s) { + out[w]++ + } + } + return out +} + +// diffWords is the vocabulary of what a change touches: the file paths, and +// the words of every line it adds or removes, so that a message naming its +// ground meets the ground it names. +func diffWords(c *Change) map[string]bool { + out := map[string]bool{} + for _, path := range c.Files { + for w := range contentWords(strings.TrimSuffix(path, filepath.Ext(path))) { + out[w] = true + } + } + for _, line := range strings.Split(c.Diff, "\n") { + if strings.HasPrefix(line, "+++") || strings.HasPrefix(line, "---") || + strings.HasPrefix(line, "@@") || strings.HasPrefix(line, "diff ") || + strings.HasPrefix(line, "index ") || strings.HasPrefix(line, "\\ ") { + continue + } + if strings.HasPrefix(line, "+") || strings.HasPrefix(line, "-") { + for w := range contentWords(line[1:]) { + out[w] = true + } + } + } + return out +} + +// measured is the message the checks read, and, where the change is staged, +// the note that what was measured is the previous commit's message rather +// than the staged one's. The note itself is kept out of the measurement: it +// is the tool's own words, not the commit's. +func measured(c *Change) (msg, note string) { + msg = strings.TrimSpace(strings.TrimPrefix(c.Message, uncommittedNote)) + if strings.HasPrefix(c.Message, uncommittedNote) { + note = " (measured on the previous commit's message: the change is staged, not committed)" + } + return msg, note +} + +// shannonEntropy is the entropy of s in bits per byte, over its bytes. A +// message holding runes outside ASCII measures higher than its characters +// alone would, which errs toward silence. +func shannonEntropy(s string) float64 { + n := len(s) + if n == 0 { + return 0 + } + var counts [256]int + for i := range n { + counts[s[i]]++ + } + var h float64 + for _, c := range counts { + if c == 0 { + continue + } + p := float64(c) / float64(n) + h -= p * math.Log2(p) + } + return h +} + +// compressionRatio is the share of its length s keeps after zlib. The +// framing counts, which is why the check applies only past compressionFloor. +func compressionRatio(s string) float64 { + var buf bytes.Buffer + w := zlib.NewWriter(&buf) + w.Write([]byte(s)) + w.Close() + return float64(buf.Len()) / float64(len(s)) +} diff --git a/static_test.go b/static_test.go @@ -0,0 +1,597 @@ +package main + +import ( + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +// The checks are pure over the message, so they are tested directly; the +// Gathered shape of a message is tested where it is gathered. + +func TestLowEntropyIsReported(t *testing.T) { + change := &Change{Message: strings.Repeat("asdf asdf ", 6)} + findings := runChecks(change) + if len(findings) != 1 { + t.Fatalf("got %d findings, want one:\n%v", len(findings), findings) + } + f := findings[0] + if f.Job != "static" || f.Rule != "message-low-entropy" || f.Severity != MustFix { + t.Errorf("got %s/%s/%s", f.Job, f.Rule, f.Severity) + } + // A failure explains itself: the measurement, the threshold, and why a + // message can measure that low. + for _, want := range []string{"2.3", "Shannon", "3.2", "phrase repeated"} { + if !strings.Contains(f.Message, want) { + t.Errorf("%q missing from: %s", want, f.Message) + } + } +} + +func TestBoilerplateIsReported(t *testing.T) { + change := &Change{Message: strings.Repeat(licenceLine, 9)} + findings := runChecks(change) + if len(findings) != 1 { + t.Fatalf("got %d findings, want one:\n%v", len(findings), findings) + } + f := findings[0] + if f.Rule != "message-boilerplate" { + t.Errorf("got %s", f.Rule) + } + for _, want := range []string{"compresses", "20%", "pasted"} { + if !strings.Contains(f.Message, want) { + t.Errorf("%q missing from: %s", want, f.Message) + } + } +} + +// A message can fail both at once, and each check answers for itself. +func TestBothChecksFireOnOneMessage(t *testing.T) { + change := &Change{Message: strings.Repeat("asdf asdf asdf asdf asdf asdf\n", 20)} + var got []string + for _, f := range runChecks(change) { + got = append(got, f.Rule) + } + if len(got) != 2 || got[0] == got[1] { + t.Errorf("got %v, want both rules once each", got) + } +} + +func TestChecksStaySilentOnAMessageThatSaysSomething(t *testing.T) { + change := &Change{Message: goodMessage} + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// Below the floors a measurement says nothing either way: a short subject is +// low-entropy whatever it says, and zlib cannot beat its own framing. +func TestShortMessagesAreNotMeasured(t *testing.T) { + for _, msg := range []string{"wip", strings.Repeat(licenceLine, 4)} { + if findings := runChecks(&Change{Message: msg}); len(findings) != 0 { + t.Errorf("%q: got %v", msg, findings) + } + } +} + +// A staged change carries the previous commit's message, so a finding says so +// rather than letting the reader believe the staged work was measured. +func TestStagedChangeMeasuresThePreviousMessage(t *testing.T) { + change := &Change{Message: uncommittedNote + strings.Repeat("asdf asdf ", 6)} + findings := runChecks(change) + if len(findings) != 1 { + t.Fatalf("got %d findings:\n%v", len(findings), findings) + } + if !strings.Contains(findings[0].Message, "previous commit") { + t.Errorf("does not say what it measured: %s", findings[0].Message) + } +} + +// The checks read what Gather read, including a message git itself produced. +func TestStaticAgainstARealCommit(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("first", "x.go") + r.write("y.go", "package x\n") + r.commit(strings.Repeat("asdf asdf ", 6), "y.go") + + change, err := Gather("HEAD^", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-low-entropy" { + t.Errorf("got %v", findings) + } +} + +func TestStaticAgainstALicensedCommitMessage(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit(strings.Repeat(licenceLine, 9), "x.go") + + change, err := Gather("HEAD", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-boilerplate" { + t.Errorf("got %v", findings) + } +} + +// commonHistory is a history in which fix and build are common and nothing +// else is: enough subjects, said over and over, for the frequencies to mean +// something. +func commonHistory() []string { + var h []string + for i := 0; i < 60; i++ { + h = append(h, "fix the failing build", "build: fix the build", "fix build") + } + return h +} + +func TestCommonWordsAreReported(t *testing.T) { + change := &Change{ + Message: "fix build", + History: commonHistory(), + Files: []string{"auth_service.go"}, + Diff: "+type JWTParser struct{}\n", + } + findings := runChecks(change) + if len(findings) != 1 { + t.Fatalf("got %d findings, want one:\n%v", len(findings), findings) + } + f := findings[0] + if f.Rule != "message-common-words" { + t.Errorf("got %s", f.Rule) + } + for _, want := range []string{"fix", "build", "180", "names nothing"} { + if !strings.Contains(f.Message, want) { + t.Errorf("%q missing from: %s", want, f.Message) + } + } +} + +// One word the history has never used is enough to spare a message: a +// message like this cannot have written that word without reading the +// change. +func TestAWordTheHistoryHasNotUsedIsSilent(t *testing.T) { + change := &Change{ + Message: "fix the parser deadlock", + History: commonHistory(), + Files: []string{"auth_service.go"}, + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// Naming what the change touches spares a message even when every word is +// common: Update requests.ts is not updated the thing. +func TestNamingTheChangeIsSilent(t *testing.T) { + history := commonHistory() + for i := 0; i < 40; i++ { + history = append(history, "update the parser", "parser: update") + } + change := &Change{ + Message: "update parser", + History: history, + Files: []string{"parser.go"}, + Diff: "+func Parse() {}\n", + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// A version or an issue number is information however few words carry it. +func TestVersionNumberSparesAMessage(t *testing.T) { + history := commonHistory() + for i := 0; i < 40; i++ { + history = append(history, "bump version") + } + change := &Change{ + Message: "bump to 2.0.26", + History: history, + Files: []string{"version.go"}, + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// Below a hundred subjects the frequencies are too thin to judge by, and a +// repository that young has not earned the accusation. +func TestThinHistoryIsSilent(t *testing.T) { + change := &Change{ + Message: "fix build", + History: commonHistory()[:99], + Files: []string{"auth_service.go"}, + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// Merge and squash subjects are git's words, not the commit's. +func TestMergeMessagesAreSilent(t *testing.T) { + for _, msg := range []string{ + "Merge branch 'main' into dev", + "Squashed 'vendor/x/' content from branch main", + } { + change := &Change{Message: msg, History: commonHistory()} + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("%q: got %v", msg, findings) + } + } +} + +func TestVentingIsReported(t *testing.T) { + for _, msg := range []string{ + "whoops", + "oops, missed a comma", + "damn, ran the formatter with spaces instead of tabs", + "WHOOPS, left the debug print in", + } { + findings := runChecks(&Change{Message: msg}) + if len(findings) != 1 || findings[0].Rule != "message-frustration" { + t.Errorf("%q: got %v", msg, findings) + continue + } + if !strings.Contains(findings[0].Message, "exclamation") { + t.Errorf("%q: %s", msg, findings[0].Message) + } + } +} + +// Words that history shows describing code legitimately are not markers: +// Odin says stupid UB and implements dumb PtrMap, and finally and +// eventually belong to real prose. +func TestTechnicalWordsAreSilent(t *testing.T) { + for _, msg := range []string{ + "Change >> behaviour in LLVM to prevent stupid UB", + "Respect TERM=dumb in the test runner", + "Implement dumb PtrMap", + "Temporarily fix the syscall (eventually to be replaced)", + "finally fixed the flaky resize test", + } { + if findings := runChecks(&Change{Message: msg}); len(findings) != 0 { + t.Errorf("%q: got %v", msg, findings) + } + } +} + +// A real commit that is only an admission is caught through Gather too. +func TestVentingAgainstARealCommit(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("whoops", "x.go") + + change, err := Gather("HEAD", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-frustration" { + t.Errorf("got %v", findings) + } +} + +func TestMoodIsChecked(t *testing.T) { + for _, msg := range []string{ + "Added readme.", + "Fixing the build", + "This fixes the crash", + "i can't spell", + "web: solarized palette at artifact-page contrast", + "deps: upgraded the webp", + } { + findings := runChecks(&Change{Message: msg}) + if len(findings) != 1 || findings[0].Rule != "message-not-imperative" { + t.Errorf("%q: got %v", msg, findings) + continue + } + if !strings.Contains(findings[0].Message, "imperative mood") { + t.Errorf("%q: %s", msg, findings[0].Message) + } + } +} + +// Verbs whose past and imperative share a form are spared, articles are +// spared because an explainer may be a noun phrase on purpose, and the +// suffix rules must not hear speed and feed as past. +func TestMoodIsSpared(t *testing.T) { + for _, msg := range []string{ + "review: measure the message without a model", + "icnsify: read icons out of a Windows binary", + "Add caching for responses", + "Bump to 2.0.x", + "review: an eval set, and the criteria tuned against it", + "docs: the vocabulary, the voices section, and the three providers", + "Speed up the build", + "Merge branch 'main' into dev", + "Squashed 'vendor/x/' content from branch main", + } { + if findings := runChecks(&Change{Message: msg}); len(findings) != 0 { + t.Errorf("%q: got %v", msg, findings) + } + } +} + +// A past-tense commit in a real history is caught through Gather too. +func TestMoodAgainstARealCommit(t *testing.T) { + r := newRepo(t) + r.write("x.go", "package x\n") + r.commit("Added readme.", "x.go") + + change, err := Gather("HEAD", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-not-imperative" { + t.Errorf("got %v", findings) + } +} + +func TestBodyIsOwed(t *testing.T) { + findings := runChecks(&Change{Message: "feat: add the thing", Diff: strings.Repeat("+line\n", 51)}) + if len(findings) != 1 || findings[0].Rule != "message-no-body" { + t.Errorf("got %v", findings) + return + } + if !strings.Contains(findings[0].Message, "carries no body") { + t.Errorf("%s", findings[0].Message) + } +} + +func TestBodyIsSpared(t *testing.T) { + for _, tt := range []struct { + diff, subject, body string + }{ + {strings.Repeat("+line\n", 10), "feat: add the thing", ""}, + {strings.Repeat("+line\n", 500), "feat: add the thing", "one two three four five"}, + {strings.Repeat("+line\n", 500), "feat: add the thing", variedWords(150)}, + {strings.Repeat("+line\n", 500), "Merge branch 'main'", ""}, + // A formatting run: the same lines removed and added in another + // order, changed not at all. + {strings.Repeat("+x\n", 30) + strings.Repeat("-x\n", 30), "format: run gofmt", ""}, + } { + msg := tt.subject + if tt.body != "" { + msg += "\n\n" + tt.body + } + if findings := runChecks(&Change{Message: msg, Diff: tt.diff}); len(findings) != 0 { + t.Errorf("%q over %d diff lines: got %v", tt.subject, len(tt.diff)/6, findings) + } + } +} + +// variedWords are n distinct words, so that a fixture body of a given +// length does not read to the other checks as one phrase repeated. +func variedWords(n int) string { + words := make([]string, 0, n) + for i := range n { + words = append(words, fmt.Sprintf("word%d", i)) + } + return strings.Join(words, " ") +} + +func TestBodyIsCapped(t *testing.T) { + msg := "feat: add the thing\n\n" + variedWords(151) + findings := runChecks(&Change{Message: msg, Diff: strings.Repeat("+line\n", 10)}) + if len(findings) != 1 || findings[0].Rule != "message-long-body" { + t.Errorf("got %v", findings) + } + if findings := runChecks(&Change{Message: "feat: add\n\n" + variedWords(150)}); len(findings) != 0 { + t.Errorf("150 words: got %v", findings) + } +} + +// A large subject-only staged change is caught through Gather too, where +// the message measured is the previous commit's. +func TestBodyAgainstARealCommit(t *testing.T) { + r := newRepo(t) + r.write("seed.txt", "seed\n") + r.commit("seed", "seed.txt") + + parts := []string{"package x\n\n"} + for i := 0; i < 60; i++ { + parts = append(parts, fmt.Sprintf("var v%d = %d\n", i, i)) + } + r.write("x.go", strings.Join(parts, "")) + r.stage("x.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-no-body" { + t.Errorf("got %v", findings) + } +} + +// TestTemporalIsReported runs the whole check set over a change whose +// history names an untouched partner, so the finding survives the others. +func TestTemporalIsReported(t *testing.T) { + findings := runChecks(&Change{ + Files: []string{"login.go"}, + Temporal: &Temporal{ + Commits: map[string]int{"login.go": 10, "session.go": 9}, + Partners: map[string][]Partner{"login.go": {{Name: "session.go", Shared: 9}}}, + }, + }) + if len(findings) != 1 || findings[0].Rule != "history-coupled-file" { + t.Errorf("got %v", findings) + return + } + for _, want := range []string{"login.go", "session.go", "9 of the 10"} { + if !strings.Contains(findings[0].Message, want) { + t.Errorf("message lacks %q: %s", want, findings[0].Message) + } + } +} + +func TestTemporalIsSpared(t *testing.T) { + tests := []struct { + name string + files []string + temporal *Temporal + }{ + {"no history", []string{"a.go"}, nil}, + {"partner touched too", []string{"a.go", "b.go"}, &Temporal{ + Commits: map[string]int{"a.go": 10, "b.go": 9}, + Partners: map[string][]Partner{"a.go": {{Name: "b.go", Shared: 9}}}, + }}, + {"below the coupling", []string{"a.go"}, &Temporal{ + Commits: map[string]int{"a.go": 10, "b.go": 10}, + Partners: map[string][]Partner{"a.go": {{Name: "b.go", Shared: 5}}}, + }}, + {"below the support", []string{"a.go"}, &Temporal{ + Commits: map[string]int{"a.go": 3, "b.go": 3}, + Partners: map[string][]Partner{"a.go": {{Name: "b.go", Shared: 3}}}, + }}, + } + for _, tt := range tests { + if findings := checkTemporal(&Change{Files: tt.files, Temporal: tt.temporal}); len(findings) != 0 { + t.Errorf("%s: got %v", tt.name, findings) + } + } +} + +// Through Gather, the check counts the history git holds, and a change that +// touches both halves of a coupled pair is left alone. +func TestTemporalAgainstARealCommit(t *testing.T) { + r := newRepo(t) + r.write("seed.txt", "seed\n") + r.commit("seed", "seed.txt") + for i := 0; i < 8; i++ { + r.write("a.go", fmt.Sprintf("package a\n\nvar v%d = %d\n", i, i)) + r.write("b.go", fmt.Sprintf("package b\n\nvar w%d = %d\n", i, i)) + r.commit(fmt.Sprintf("grow: the pair, round %d", i), "a.go", "b.go") + } + r.write("a.go", "package a\n\nvar v8 = 8\n\nvar v9 = 9\n") + r.stage("a.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "history-coupled-file" { + t.Errorf("got %v", findings) + return + } + if findings[0].File != "a.go" || !strings.Contains(findings[0].Message, "b.go") { + t.Errorf("%s\n%s", findings[0].File, findings[0].Message) + } + + // The pair touched together is the coupling honoured, and the check + // says nothing. + r.write("b.go", "package b\n\nvar w8 = 8\n\nvar w9 = 9\n") + r.stage("b.go") + change, err = Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// A partner the tree no longer holds is history's partner, not the +// change's, and the check says nothing of it. +func TestTemporalSparesAGonePartner(t *testing.T) { + r := newRepo(t) + r.write("a.go", "package a\n") + r.write("b.go", "package b\n") + for i := 0; i < 8; i++ { + r.write("a.go", fmt.Sprintf("package a\n\nvar v%d = %d\n", i, i)) + r.write("b.go", fmt.Sprintf("package b\n\nvar w%d = %d\n", i, i)) + r.commit(fmt.Sprintf("grow: the pair, round %d", i), "a.go", "b.go") + } + r.write("b.go", "gone") + r.commit("retire: the partner", "b.go") + if err := os.Remove(filepath.Join(r.Root, "b.go")); err != nil { + t.Fatal(err) + } + r.write("a.go", "package a\n\nvar v8 = 8\n\nvar v9 = 9\n") + r.stage("a.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + if findings := runChecks(change); len(findings) != 0 { + t.Errorf("got %v", findings) + } +} + +// Through Gather, the check reads the history git holds, and a staged +// change is measured on the previous commit's message and says so. +func TestCommonWordsAgainstARealHistory(t *testing.T) { + r := newRepo(t) + for i := 0; i < 110; i++ { + r.run("commit", "--allow-empty", "-q", "-m", "fix build") + } + r.write("auth_service.go", "package auth\n\ntype JWTParser struct{}\n") + r.stage("auth_service.go") + + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + findings := runChecks(change) + if len(findings) != 1 || findings[0].Rule != "message-common-words" { + t.Fatalf("got %v", findings) + } + if !strings.Contains(findings[0].Message, "previous commit") { + t.Errorf("does not say what it measured: %s", findings[0].Message) + } +} + +const licenceLine = "Copyright 2026 Example Corp. All rights reserved. Licensed under the Apache License, Version 2.0.\n" + +const goodMessage = `review: say which findings the source dismissed + +A dismissal answered the finding above it and was then thrown away with +the finding, so a reading that met a third of the change reported less +than it read. Kept beside the finding they answered, a dismissal is +visible where a silent one was not, and --verbose names the reason it +gave rather than leaving a gap in the list for nobody to explain. + +The window a dismissal covers stays as written: a few lines either side +of the comment, because narrowing it waits on a case that shows the +narrowing losing a finding it should have kept.` + +// The gaming checks are wired into the ordinary run: a change that +// dismisses what the readers would have found, or deletes the tests that +// would have failed, is measured before anything is asked of a provider. +// The ignore comment is assembled at runtime, so this file does not add a +// dismissal of its own while testing the check that counts them. +func TestChecksHoldTheChangeToTheReview(t *testing.T) { + change := &Change{Diff: `--- a/x_test.go ++++ b/x_test.go +@@ -1,2 +1,2 @@ + package x ++//` + `review:ignore all tidy +-func TestA(t *testing.T) {} +`} + var got []string + for _, f := range runChecks(change) { + if f.Severity != MustFix { + t.Errorf("%s is %s, want must-fix", f.Rule, f.Severity) + } + got = append(got, f.Rule) + } + for _, want := range []string{"suppression-added", "test-deleted"} { + if !slices.Contains(got, want) { + t.Errorf("%s missing from %v", want, got) + } + } +} diff --git a/staticcheck.go b/staticcheck.go @@ -0,0 +1,215 @@ +package main + +// staticcheck is the repository's own Go analyser, and review has nothing to +// teach it about Go. The check below only decides which of its findings +// belong to the change: staticcheck is free to say what it likes about the +// rest of the repository, and review's contract is the change, so a finding +// is kept only when it lands on a line the change adds. + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "regexp" + "slices" + "strings" + "time" +) + +// staticcheckTimeout bounds one run. A cold first analysis of a large module +// can take minutes; past this the check says nothing rather than holding the +// review, and the run warms the build cache for the next one. +const staticcheckTimeout = 5 * time.Minute + +// checkCode matches a staticcheck finding code, such as SA4006 or S1002. +// Anything else staticcheck emits — compile errors, for instance — is not a +// finding about the code. +var checkCode = regexp.MustCompile(`^[A-Z]+[0-9]+$`) + +// checkStaticcheck runs staticcheck over the packages the change touches and +// reports what it found on the lines the change adds. It is called beside +// the other deterministic checks, so its findings survive a model that +// cannot answer. It says nothing where it has nothing to say: without the +// binary, without Go files, or without a module to analyse. +func checkStaticcheck(root, rev string, c *Change) []Finding { + if !touchesGo(c) { + return nil + } + if _, err := exec.LookPath("staticcheck"); err != nil { + return nil + } + tree := root + after, ranged := ends(rev) + if ranged { + // The change arrived at a tree the working directory may have left + // behind long ago, so the analysis runs on that tree, materialised + // to a scratch directory the way the other frontends do. + tmp, err := os.MkdirTemp("", "review-staticcheck-") + if err != nil { + noteStaticcheck("cannot make a scratch directory: %v", err) + return nil + } + defer os.RemoveAll(tmp) + archive := filepath.Join(tmp, "tree.tar") + if _, err := git(root, "archive", "-o", archive, after); err != nil { + noteStaticcheck("cannot materialise %s: %v", after, err) + return nil + } + if err := exec.Command("tar", "-xf", archive, "-C", tmp).Run(); err != nil { + noteStaticcheck("cannot unpack %s: %v", after, err) + return nil + } + os.Remove(archive) + tree = tmp + } + if _, err := os.Stat(filepath.Join(tree, "go.mod")); err != nil { + fmt.Fprintln(os.Stderr, "skipping staticcheck: no go.mod at the repository root") + return nil + } + args := changedPackages(tree, c) + if len(args) == 0 { + return nil + } + + ctx, cancel := context.WithTimeout(context.Background(), staticcheckTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "staticcheck", append([]string{"-f", "json"}, args...)...) + cmd.Dir = tree + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + err := cmd.Run() + if ctx.Err() != nil { + noteStaticcheck("a run past %v is not waited for", staticcheckTimeout) + } + if err != nil && ctx.Err() == nil { + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 1 { + // Exit 1 is staticcheck reporting findings; anything else is a + // broken run, and its stderr is the only clue. + noteStaticcheck("%s", tail(stderr.String(), 200)) + } + } + + // The diff was capped before it was gathered, so a change larger than + // the cap is judged on the lines that survived, never on ones invented. + added := addedLines(c.Diff) + changed := map[string]bool{} + for _, f := range c.Files { + changed[f] = true + } + var ( + out []Finding + seen = map[string]bool{} + ) + for _, line := range strings.Split(stdout.String(), "\n") { + if line = strings.TrimSpace(line); line == "" { + continue + } + var p scProblem + if json.Unmarshal([]byte(line), &p) != nil || !checkCode.MatchString(p.Code) { + continue + } + file := relative(tree, p.Location.File) + if !changed[file] || !slices.Contains(added[file], p.Location.Line) { + continue + } + key := fmt.Sprintf("%s:%d:%s", file, p.Location.Line, p.Code) + if seen[key] { + continue + } + seen[key] = true + out = append(out, Finding{ + Job: "static", + Rule: "staticcheck/" + p.Code, + Severity: staticcheckSeverity(p.Code), + File: file, + Line: p.Location.Line, + Message: p.Message, + }) + } + return out +} + +// scProblem is one finding in staticcheck's -f json output, one JSON object +// per line. Only the fields the check reads are named. +type scProblem struct { + Code string `json:"code"` + Location struct { + File string `json:"file"` + Line int `json:"line"` + } `json:"location"` + Message string `json:"message"` +} + +// staticcheckSeverity maps a staticcheck category onto the report's +// severities. SA is a fault the analyser argues for; U is code that serves +// nobody, and S a simplification; ST and QF are style, and anything +// unfamiliar errs toward a note. +func staticcheckSeverity(code string) Severity { + switch { + case strings.HasPrefix(code, "SA"): + return MustFix + case strings.HasPrefix(code, "S") && !strings.HasPrefix(code, "ST"), + strings.HasPrefix(code, "U"): + return Consider + } + return Note +} + +// changedPackages names the packages of the change's Go files, as patterns +// staticcheck takes. The leading ./ is load-bearing: without it a directory +// reads as a module path and matches nothing. +func changedPackages(tree string, c *Change) []string { + dirs := map[string]bool{} + for _, f := range c.Files { + if !strings.HasSuffix(f, ".go") { + continue + } + dir := filepath.Dir(f) + if dir == "vendor" || strings.HasPrefix(dir, "vendor/") { + continue + } + // A package the change deleted stands only in the materialised tree + // where it does; a directory that is nowhere is not a pattern. + if _, err := os.Stat(filepath.Join(tree, dir)); err != nil { + continue + } + dirs[dir] = true + } + var out []string + for dir := range dirs { + out = append(out, "./"+dir) + } + slices.Sort(out) + return out +} + +// touchesGo is whether the change touches a Go file at all. +func touchesGo(c *Change) bool { + for _, f := range c.Files { + if strings.HasSuffix(f, ".go") { + return true + } + } + return false +} + +// noteStaticcheck says why the check said nothing. One line, and only when +// there is a reader who was owed staticcheck's opinion. +func noteStaticcheck(format string, args ...any) { + fmt.Fprintf(os.Stderr, "skipping staticcheck: "+format+"\n", args...) +} + +// tail is the last few bytes of a longer text, for an error message. +func tail(s string, n int) string { + s = strings.TrimSpace(s) + if len(s) <= n { + return s + } + return "…" + s[len(s)-n:] +} diff --git a/staticcheck_test.go b/staticcheck_test.go @@ -0,0 +1,141 @@ +package main + +import ( + "os/exec" + "testing" +) + +// The staticcheck tests hold the check against the analyser itself, an +// external authority: when it is absent they fail, they do not skip. +func needStaticcheck(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("staticcheck"); err != nil { + t.Fatalf("staticcheck is not installed: %v", err) + } +} + +// goModule is the smallest module a fixture needs. +const goModule = "module probe\n\ngo 1.27.0\n" + +// withFault is a file staticcheck faults at line 4: a comparison with the +// bool constant it can be simplified past. +const withFault = "package probe\n\nfunc F(x bool) int {\n\tif x == true {\n\t\treturn 1\n\t}\n\treturn 2\n}\n" + +func TestStaticcheckReadsTheChange(t *testing.T) { + needStaticcheck(t) + r := newRepo(t) + r.write("go.mod", goModule) + r.write("a.go", withFault) + r.stage("go.mod", "a.go") + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + findings := checkStaticcheck(r.Root, "", change) + if len(findings) != 1 { + t.Fatalf("got %v, want the simplification finding", findings) + } + f := findings[0] + if f.Rule != "staticcheck/S1002" || f.File != "a.go" || f.Line != 4 { + t.Errorf("got %s at %s:%d", f.Rule, f.File, f.Line) + } + if f.Severity != Consider { + t.Errorf("severity %s, want consider", f.Severity) + } + if f.Job != "static" || f.Message == "" { + t.Errorf("job %q message %q", f.Job, f.Message) + } +} + +func TestStaticcheckKeepsToTheChange(t *testing.T) { + needStaticcheck(t) + r := newRepo(t) + r.write("go.mod", goModule) + r.write("a.go", withFault) + r.commit("a: carry the fault history already holds", "go.mod", "a.go") + r.write("b.go", "package probe\n\nfunc G() int { return 3 }\n") + r.stage("b.go") + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + // The fault in a.go is history's, not this change's; the analyser sees + // it and the check must say nothing about it. + if findings := checkStaticcheck(r.Root, "", change); len(findings) != 0 { + t.Fatalf("got %v, want nothing: the fault is not on a line the change adds", findings) + } +} + +func TestStaticcheckReadsARange(t *testing.T) { + needStaticcheck(t) + r := newRepo(t) + r.write("go.mod", goModule) + r.write("a.go", "package probe\n\nfunc A() int { return 1 }\n") + r.commit("a: begin", "go.mod", "a.go") + r.write("b.go", withFault) + r.commit("b: add the fault", "b.go") + change, err := Gather("HEAD^..HEAD", r.Root) + if err != nil { + t.Fatal(err) + } + // A range's tree is not the working tree: the analysis has to run on + // the tree the change arrived at. + findings := checkStaticcheck(r.Root, "HEAD^..HEAD", change) + if len(findings) != 1 { + t.Fatalf("got %v, want the simplification finding", findings) + } + if f := findings[0]; f.File != "b.go" || f.Line != 4 || f.Rule != "staticcheck/S1002" { + t.Errorf("got %s at %s:%d", f.Rule, f.File, f.Line) + } +} + +func TestStaticcheckSkipsWithoutGoModule(t *testing.T) { + r := newRepo(t) + r.write("a.go", "package probe\n") + r.stage("a.go") + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + var findings []Finding + out := captureStderr(t, func() { + findings = checkStaticcheck(r.Root, "", change) + }) + if len(findings) != 0 { + t.Fatalf("got %v, want nothing without a go.mod", findings) + } + if want := "skipping staticcheck: no go.mod at the repository root\n"; out != want { + t.Errorf("stderr %q, want %q", out, want) + } +} + +func TestStaticcheckSilentWithoutTheBinary(t *testing.T) { + r := newRepo(t) + r.write("go.mod", goModule) + r.write("a.go", withFault) + r.stage("go.mod", "a.go") + change, err := Gather("", r.Root) + if err != nil { + t.Fatal(err) + } + // The path goes once the change is gathered: only the check is meant to + // look for the binary and find it gone. + t.Setenv("PATH", "") + if findings := checkStaticcheck(r.Root, "", change); len(findings) != 0 { + t.Fatalf("got %v, want nothing without the binary", findings) + } +} + +func TestStaticcheckSeverity(t *testing.T) { + for code, want := range map[string]Severity{ + "SA4006": MustFix, + "S1002": Consider, + "U1000": Consider, + "ST1005": Note, + "QF1003": Note, + } { + if got := staticcheckSeverity(code); got != want { + t.Errorf("%s is %s, want %s", code, got, want) + } + } +} diff --git a/tsfrontend.go b/tsfrontend.go @@ -0,0 +1,347 @@ +package main + +import ( + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" +) + +// TSFrontend reads TypeScript through ast-grep, which matches its patterns +// against the tree-sitter syntax tree rather than against lines. A pattern +// over lines misses an indented constant inside a block; a query over syntax +// does not, and the top-level constraint keeps function bodies out. +type TSFrontend struct{} + +func (TSFrontend) Name() string { return "typescript" } + +func (TSFrontend) Covers(path string) bool { + return strings.HasSuffix(path, ".ts") || strings.HasSuffix(path, ".tsx") +} + +func (TSFrontend) Features() Features { + return FeatSymbols | FeatTests | FeatComments | FeatIndex +} + +// tsPatterns is the set of top-level declarations the queries bind. Export +// status is part of the pattern so a symbol's audience is known to the jobs; +// the plain forms also match the inner node of an export, which the +// gatherer resolves by preferring the exported match. +var tsPatterns = []struct { + kind string + exported bool + pattern string +}{ + {"value", false, "const $N = $$$V"}, + {"value", true, "export const $N = $$$V"}, + {"value", false, "const $N: $$$T = $$$V"}, + {"value", true, "export const $N: $$$T = $$$V"}, + {"value", false, "let $N = $$$V"}, + {"value", true, "export let $N = $$$V"}, + {"value", true, "export let $N: $$$T = $$$V"}, + {"value", false, "var $N = $$$V"}, + {"value", true, "export var $N = $$$V"}, + {"type", false, "type $N = $$$B"}, + {"type", true, "export type $N = $$$B"}, + {"type", false, "interface $N { $$$B }"}, + {"type", true, "export interface $N { $$$B }"}, + {"type", false, "enum $N { $$$V }"}, + {"type", true, "export enum $N { $$$V }"}, + {"type", false, "class $N { $$$B }"}, + {"type", true, "export class $N { $$$B }"}, + {"func", false, "function $N($$$P) { $$$B }"}, + {"func", true, "export function $N($$$P) { $$$B }"}, + {"func", true, "export function $N($$$P): $$$R { $$$B }"}, + {"func", false, "async function $N($$$P) { $$$B }"}, + {"func", true, "export async function $N($$$P) { $$$B }"}, + {"func", true, "export async function $N($$$P): $$$R { $$$B }"}, + {"func", true, "export default function $N($$$P) { $$$B }"}, + {"func", true, "export default function $N($$$P): $$$R { $$$B }"}, +} + +// tsTestPatterns bind the runner's calls. A test inside a describe block is +// still a test, so these carry no top-level constraint; they are only ever +// asked for, so a helper named test would not distract them elsewhere. +var tsTestPatterns = []string{"test($S, $$$B)", "it($S, $$$B)"} + +// grepMatch is one ast-grep finding. +type grepMatch struct { + Rule string `json:"ruleId"` + Text string `json:"text"` + File string `json:"file"` + Range struct { + Start struct { + Line int `json:"line"` + } `json:"start"` + } `json:"range"` + Meta struct { + Single map[string]struct { + Text string `json:"text"` + } `json:"single"` + } `json:"metaVariables"` +} + +func (m grepMatch) name() string { + for _, key := range []string{"N", "S"} { + if bound, ok := m.Meta.Single[key]; ok { + return bound.Text + } + } + return "" +} + +// scan materialises the files at the revision into a scratch directory +// (ast-grep reads the working tree, and a review is a change, not a tree), +// writes the rules, and asks for the matches. The returned matches carry the +// scratch path in File; the caller maps it back through the order given. +func scan(files []string, root, rev string) ([]grepMatch, error) { + if len(files) == 0 { + return nil, nil + } + dir, err := os.MkdirTemp("", "review-ts") + if err != nil { + return nil, err + } + defer os.RemoveAll(dir) + + if err := os.MkdirAll(filepath.Join(dir, "rules"), 0o755); err != nil { + return nil, err + } + config := "ruleDirs:\n - rules\n" + if err := os.WriteFile(filepath.Join(dir, "sgconfig.yml"), []byte(config), 0o644); err != nil { + return nil, err + } + if err := writeRules(filepath.Join(dir, "rules")); err != nil { + return nil, err + } + + var args []string + for i, name := range files { + source, err := at(root, rev, name) + if err != nil { + continue // Deleted by the change, so there is nothing to read. + } + // The extension is kept: ast-grep infers the grammar from it. + scratch := filepath.Join(dir, fmt.Sprintf("%04d%s", i, filepath.Ext(name))) + if err := os.WriteFile(scratch, source, 0o644); err != nil { + return nil, err + } + args = append(args, scratch) + } + cmd := exec.Command("ast-grep", append([]string{"scan", "-c", filepath.Join(dir, "sgconfig.yml"), "--json"}, args...)...) + stdout, err := cmd.Output() + if err != nil && len(stdout) == 0 { + return nil, err // A clean failure; anything else is partial output to read. + } + var out []grepMatch + if err := json.Unmarshal(stdout, &out); err != nil { + return nil, fmt.Errorf("ast-grep: %w", err) + } + for i := range out { + base := strings.TrimSuffix(filepath.Base(out[i].File), filepath.Ext(out[i].File)) + index, err := strconv.Atoi(base) + if err == nil && index < len(files) { + out[i].File = files[index] + } + } + return out, nil +} + +// writeRules emits the query set for both grammars. The top-level rule keeps +// declarations out of function bodies; the test rules go anywhere. +func writeRules(dir string) error { + top := func(pattern string) string { + return fmt.Sprintf("pattern: %q\nnot:\n inside:\n kind: statement_block\n stopBy: end\n", pattern) + } + for _, table := range []struct { + prefix string + language string + patterns []struct { + kind string + exported bool + pattern string + } + }{{"ts", "TypeScript", tsPatterns}, {"tsx", "TSX", tsPatterns}} { + for i, p := range table.patterns { + status := "plain" + if p.exported { + status = "export" + } + id := fmt.Sprintf("%s-%s-%s-%d", table.prefix, p.kind, status, i) + body := fmt.Sprintf("id: %s\nlanguage: %s\nseverity: info\nrule:\n %s", id, table.language, strings.ReplaceAll(top(p.pattern), "\n", "\n ")) + if err := os.WriteFile(filepath.Join(dir, id+".yml"), []byte(body), 0o644); err != nil { + return err + } + } + } + for _, table := range []struct { + prefix string + language string + }{{"ts", "TypeScript"}, {"tsx", "TSX"}} { + for i, pattern := range tsTestPatterns { + id := fmt.Sprintf("%s-test-%d", table.prefix, i) + body := fmt.Sprintf("id: %s\nlanguage: %s\nseverity: info\nrule:\n pattern: %q\n", id, table.language, pattern) + if err := os.WriteFile(filepath.Join(dir, id+".yml"), []byte(body), 0o644); err != nil { + return err + } + } + } + return nil +} + +// Change appends the declarations, tests and prose the added lines of the +// change's TypeScript files introduce. +func (g TSFrontend) Change(root, rev string, c *Change, added map[string][]int) error { + var covered []string + for _, name := range c.Files { + if g.Covers(name) { + covered = append(covered, name) + } + } + matches, err := scan(covered, root, rev) + if err != nil { + return err + } + sources := map[string][]byte{} + lines := map[string][]string{} + touched := map[string]map[int]bool{} + for _, name := range covered { + source, err := at(root, rev, name) + if err != nil { + continue + } + sources[name] = source + lines[name] = strings.Split(string(source), "\n") + touched[name] = map[int]bool{} + for _, line := range added[name] { + touched[name][line] = true + } + } + + // One declaration, several matches: the plain forms see the inner node of + // an export. Keep the exported reading of each line. + best := map[string]grepMatch{} + for _, m := range matches { + key := m.File + ":" + fmt.Sprint(m.Range.Start.Line) + old, ok := best[key] + if !ok || (strings.Contains(m.Rule, "export") && !strings.Contains(old.Rule, "export")) { + best[key] = m + } + } + for _, m := range best { + if strings.Contains(m.Rule, "-test-") { + name := unquote(m.name()) + if name == "" || !touched[m.File][m.Range.Start.Line+1] { + continue + } + c.Tests = append(c.Tests, Function{ + Name: name, File: m.File, Line: m.Range.Start.Line + 1, Body: m.Text, + }) + continue + } + name := m.name() + if name == "" || !touched[m.File][m.Range.Start.Line+1] { + continue + } + line := m.Range.Start.Line + 1 + signature := "" + if line-1 < len(lines[m.File]) { + signature = strings.TrimSpace(lines[m.File][line-1]) + } + c.Symbols = append(c.Symbols, Symbol{ + Name: name, Kind: ruleKind(m.Rule), Doc: docAbove(lines[m.File], line), + File: m.File, Line: line, + Exported: strings.Contains(m.Rule, "export"), + Signature: signature, + }) + } + for _, name := range covered { + c.Comments = append(c.Comments, commentProse(sources[name], name, added[name])...) + } + return nil +} + +// Whole reads every top-level declaration in the repository's TypeScript +// files, so a new name can be checked against the ones it may duplicate. +func (g TSFrontend) Whole(root, rev string) ([]Declared, error) { + out, err := git(root, "ls-files", "*.ts", "*.tsx") + if err != nil { + return nil, err + } + var files []string + for _, name := range strings.Split(strings.TrimSpace(out), "\n") { + if name != "" && g.Covers(name) { + files = append(files, name) + } + } + matches, err := scan(files, root, rev) + if err != nil { + return nil, err + } + best := map[string]grepMatch{} + for _, m := range matches { + key := m.File + ":" + fmt.Sprint(m.Range.Start.Line) + old, ok := best[key] + if !ok || (strings.Contains(m.Rule, "export") && !strings.Contains(old.Rule, "export")) { + best[key] = m + } + } + var index []Declared + for _, m := range best { + name := m.name() + if name == "" || strings.Contains(m.Rule, "-test-") { + continue + } + line := m.Range.Start.Line + 1 + text := "" + if source, err := at(root, rev, m.File); err == nil { + fileLines := strings.Split(string(source), "\n") + if line-1 < len(fileLines) { + text = fileLines[line-1] + } + } + index = append(index, Declared{Name: name, Kind: ruleKind(m.Rule), File: m.File, Line: line, Text: text}) + } + return index, nil +} + +// ruleKind turns a rule id back into the shape the findings report. +func ruleKind(rule string) string { + switch { + case strings.Contains(rule, "-func-"): + return "func" + case strings.Contains(rule, "-type-"): + return "type" + } + return "value" +} + +// docAbove collects the comment block ending on the line before the +// declaration, as the doc a reader would attach to it. +func docAbove(lines []string, line int) string { + var parts []string + for i := line - 2; i >= 0; i-- { + trimmed := strings.TrimSpace(lines[i]) + if !strings.HasPrefix(trimmed, "//") && !strings.HasPrefix(trimmed, "*") && !strings.HasPrefix(trimmed, "/*") { + break + } + trimmed = strings.TrimPrefix(trimmed, "///") + trimmed = strings.TrimPrefix(trimmed, "//") + trimmed = strings.TrimPrefix(trimmed, "/**") + trimmed = strings.TrimPrefix(trimmed, "/*") + trimmed = strings.TrimSuffix(trimmed, "*/") + part := strings.TrimSpace(strings.TrimPrefix(trimmed, "*")) + if part != "" { + parts = append([]string{part}, parts...) + } + } + return strings.Join(parts, " ") +} + +// unquote strips the quotes a string literal keeps in its text. +func unquote(literal string) string { + return strings.Trim(literal, "\"'`") +} diff --git a/tsfrontend_test.go b/tsfrontend_test.go @@ -0,0 +1,190 @@ +package main + +import ( + "os/exec" + "slices" + "strings" + "testing" +) + +// The TypeScript tests hold the frontend against ast-grep's parsing, an +// external authority: when it is absent they fail, they do not skip. +func needAstGrep(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("ast-grep"); err != nil { + t.Fatalf("ast-grep is not installed: %v", err) + } +} + +func TestTypeScriptReadsDeclarations(t *testing.T) { + needAstGrep(t) + r := newRepo(t) + r.write("src/icons.ts", `export const maxIcons: number = 12; +const hidden = 4; +export function readIcons(src: string): Group[] { return []; } +export default function load(src: string) { return src; } +export class Reader { size = 0; } +export interface Wide {} +export type Group = { id: number }; +export enum Kind { Small = 1 } +function helper() { return hidden; } +`) + r.write("src/.keep", "") + r.commit("ts: begin", "src/.keep") + rev := r.commit("ts: first", "src/icons.ts") + + change, err := Gather(rev+"^.."+rev, r.Root) + if err != nil { + t.Fatal(err) + } + got := map[string]Symbol{} + for _, s := range change.Symbols { + got[s.Name] = s + } + for name, want := range map[string]struct { + kind string + exported bool + }{ + "maxIcons": {"value", true}, + "hidden": {"value", false}, + "readIcons": {"func", true}, + "load": {"func", true}, + "Reader": {"type", true}, + "Wide": {"type", true}, + "Group": {"type", true}, + "Kind": {"type", true}, + "helper": {"func", false}, + } { + s, ok := got[name] + if !ok { + t.Errorf("%s not read", name) + continue + } + if s.Kind != want.kind || s.Exported != want.exported { + t.Errorf("%s: got %s exported=%v, want %s exported=%v", name, s.Kind, s.Exported, want.kind, want.exported) + } + if !strings.HasPrefix(s.Signature, name) && !strings.Contains(s.Signature, name) { + t.Errorf("%s: signature %q", name, s.Signature) + } + } +} + +func TestTypeScriptReadsTestsInsideDescribe(t *testing.T) { + needAstGrep(t) + r := newRepo(t) + r.write("src/icons.test.ts", `import {test, expect} from 'vitest'; + +describe('icons', () => { + it('decodes the largest', () => { + expect(1).toBe(1); + }); + test('reads a binary', () => { + expect(2).toBe(2); + }); +}); +`) + r.write("src/.keep", "") + r.commit("ts: begin", "src/.keep") + rev := r.commit("ts: tests", "src/icons.test.ts") + + change, err := Gather(rev+"^.."+rev, r.Root) + if err != nil { + t.Fatal(err) + } + names := []string{} + for _, test := range change.Tests { + names = append(names, test.Name) + if !strings.Contains(test.Body, "expect") { + t.Errorf("%s: body %q has no assertion", test.Name, test.Body) + } + } + if len(names) != 2 { + t.Fatalf("tests %v, want the it and the test", names) + } + for _, want := range []string{"decodes the largest", "reads a binary"} { + if !slices.Contains(names, want) { + t.Errorf("tests %v, want %q among them", names, want) + } + } +} + +func TestTypeScriptReadsDocComments(t *testing.T) { + needAstGrep(t) + r := newRepo(t) + r.write("src/icons.ts", `/** Counts the icons a binary carries. + * Rejects a binary with none. + */ +export function countIcons(): number { return 0; } +`) + r.write("src/.keep", "") + r.commit("ts: begin", "src/.keep") + rev := r.commit("ts: count", "src/icons.ts") + + change, err := Gather(rev+"^.."+rev, r.Root) + if err != nil { + t.Fatal(err) + } + if len(change.Symbols) != 1 { + t.Fatalf("symbols %v", change.Symbols) + } + want := "Counts the icons a binary carries. Rejects a binary with none." + if change.Symbols[0].Doc != want { + t.Errorf("doc %q, want %q", change.Symbols[0].Doc, want) + } + if len(change.Comments) != 2 { + t.Fatalf("comments %v, want both comment lines", change.Comments) + } + if change.Comments[0].Text != "Counts the icons a binary carries." { + t.Errorf("comments[0] %q", change.Comments[0].Text) + } + if change.Comments[1].Text != "Rejects a binary with none." { + t.Errorf("comments[1] %q", change.Comments[1].Text) + } +} + +func TestTypeScriptIndexesTheRepository(t *testing.T) { + needAstGrep(t) + r := newRepo(t) + r.write("src/a.ts", `export const plain = 3; +const hidden = 4; +export class Sized { size = 0; } +function local() {} +`) + r.write("src/a.test.ts", `test('local is not a fact with two owners', () => {}); +`) + r.write("src/.keep", "") + r.commit("ts: begin", "src/.keep") + rev := r.commit("ts: index", "src/a.ts", "src/a.test.ts") + + declared, err := TSFrontend{}.Whole(r.Root, rev) + if err != nil { + t.Fatal(err) + } + index := map[string]Declared{} + for _, d := range declared { + if _, dup := index[d.Name]; dup { + t.Errorf("%s indexed twice", d.Name) + } + index[d.Name] = d + } + if d, ok := index["plain"]; !ok || d.Text != "export const plain = 3;" { + t.Errorf("plain %v", d) + } + if d, ok := index["hidden"]; !ok || d.Text != "const hidden = 4;" { + t.Errorf("hidden %v", d) + } + if d, ok := index["Sized"]; !ok || d.Text != "export class Sized { size = 0; }" { + t.Errorf("Sized %v", d) + } + if _, ok := index["local"]; !ok { + t.Error("local not indexed") + } + if d, ok := index["local"]; ok && d.Text != "function local() {}" { + t.Errorf("local text %q", d.Text) + } + for name := range index { + if strings.Contains(name, "owners") { + t.Errorf("test file name %s indexed", name) + } + } +} diff --git a/verify_test.go b/verify_test.go @@ -0,0 +1,179 @@ +package main + +// The second reading is what stands behind a finding. These tests hold the +// contract both ways: a verdict that holds keeps the finding and marks it +// verified, a verdict that falls retracts the finding with its reason, and +// a verdict that cannot be asked fails open — the finding stands, marked +// unverified, and the failure joins the others rather than ending the run. + +import ( + "context" + "strings" + "testing" +) + +func findingsJSON(rules ...string) string { + var b strings.Builder + b.WriteString(`{"findings":[`) + for i, r := range rules { + if i > 0 { + b.WriteString(",") + } + b.WriteString(`{"rule":"` + r + `","severity":"must-fix","file":"x_test.go","line":4,"message":"m","fix":"f"}`) + } + b.WriteString(`]}`) + return b.String() +} + +func verdictJSON(pairs ...string) string { + var b strings.Builder + b.WriteString(`{"verdicts":[`) + for i, p := range pairs { + if i > 0 { + b.WriteString(",") + } + b.WriteString(p) + } + b.WriteString(`]}`) + return b.String() +} + +func verifyChange() *Change { + return &Change{Tests: []Function{{Name: "TestX", File: "x_test.go", Line: 3, Body: "func TestX(t *testing.T) {}"}}} +} + +func TestVerifyLetsAReadingStand(t *testing.T) { + provider := &relenting{answers: []string{ + findingsJSON("cannot-fail"), + verdictJSON(`{"index":0,"holds":true,"reason":""}`), + }} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider, Verify: true}.Run(context.Background(), verifyChange(), jobs) + if len(result.Failures) != 0 { + t.Fatalf("failures: %v", result.Failures) + } + if len(result.Findings) != 1 || !result.Findings[0].Verified { + t.Fatalf("got %+v", result.Findings) + } + if len(result.Retracted) != 0 { + t.Fatalf("retracted %+v", result.Retracted) + } + // The verdict is asked against the same evidence the finding came from, + // not against the finding alone. + if len(provider.asked) != 2 { + t.Fatalf("asked %d times, want two", len(provider.asked)) + } + for _, want := range []string{"TestX", "cannot-fail"} { + if !strings.Contains(provider.asked[1], want) { + t.Errorf("%q missing from the verdict's evidence:\n%s", want, provider.asked[1]) + } + } +} + +func TestVerifyRetracts(t *testing.T) { + provider := &relenting{answers: []string{ + findingsJSON("cannot-fail"), + verdictJSON(`{"index":0,"holds":false,"reason":"the test cannot fail"}`), + }} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider, Verify: true}.Run(context.Background(), verifyChange(), jobs) + if len(result.Failures) != 0 { + t.Fatalf("failures: %v", result.Failures) + } + if len(result.Findings) != 0 { + t.Fatalf("kept %+v", result.Findings) + } + if len(result.Retracted) != 1 { + t.Fatalf("got %+v", result.Retracted) + } + r := result.Retracted[0] + if r.Reason != "the test cannot fail" || r.Finding.Rule != "cannot-fail" { + t.Errorf("got %+v", r) + } +} + +// A verdict that cannot be asked is not a verdict: the findings stand, +// marked as what they are, and the failure is reported like any other. +func TestVerifyFailsOpen(t *testing.T) { + provider := &relenting{answers: []string{findingsJSON("cannot-fail")}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider, Verify: true}.Run(context.Background(), verifyChange(), jobs) + if len(result.Findings) != 1 { + t.Fatalf("kept %+v", result.Findings) + } + if result.Findings[0].Verified { + t.Error("a finding no second reading saw is marked verified") + } + if len(result.Failures) != 1 || !strings.Contains(result.Failures[0].Error(), "verify/tests:") { + t.Fatalf("got %v", result.Failures) + } +} + +// A verdict list that omits a finding stands rather than falls for it: a +// strict pass would let one dropped number retract everything. +func TestVerifyStandsForAnOmittedVerdict(t *testing.T) { + provider := &relenting{answers: []string{ + findingsJSON("cannot-fail", "skips-in-normal-conditions"), + verdictJSON(`{"index":0,"holds":true,"reason":""}`), + }} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider, Verify: true}.Run(context.Background(), verifyChange(), jobs) + if len(result.Findings) != 2 { + t.Fatalf("kept %+v", result.Findings) + } + for _, f := range result.Findings { + if !f.Verified { + t.Errorf("%s stands unverified", f.Rule) + } + } +} + +// A provider that answers the verdict in prose is asked once more, the way +// the findings are. +func TestVerifyAsksTwiceForProse(t *testing.T) { + provider := &relenting{answers: []string{ + findingsJSON("cannot-fail"), + "The finding holds.", + verdictJSON(`{"index":0,"holds":false,"reason":"the test can fail"}`), + }} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider, Verify: true}.Run(context.Background(), verifyChange(), jobs) + if len(result.Retracted) != 1 || len(result.Findings) != 0 { + t.Fatalf("got %+v kept, %+v retracted", result.Findings, result.Retracted) + } + if len(provider.asked) != 3 { + t.Fatalf("asked %d times, want three", len(provider.asked)) + } + if !strings.Contains(provider.asked[2], verdictsAgain) { + t.Errorf("the retry does not say what was wrong:\n%s", provider.asked[2]) + } +} + +// Without the second reading the findings are what they were read as: seen +// once, by the reading that reported them. +func TestVerifyOffLeavesFindingsUnverified(t *testing.T) { + provider := &relenting{answers: []string{findingsJSON("cannot-fail")}} + jobs, err := chosen("tests") + if err != nil { + t.Fatal(err) + } + result := Reviewer{Provider: provider}.Run(context.Background(), verifyChange(), jobs) + if len(result.Findings) != 1 || result.Findings[0].Verified { + t.Fatalf("got %+v", result.Findings) + } +}