commit 809f6fdda4d0de8f71215e1bd331620d5018dee2
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Mon, 21 Sep 2026 11:06:49 -0300
review: read a change the way several narrow readers would
The checks a repository can run deterministically do not cover names, restated
facts, tests that cannot fail, or comments asserting what nobody verified. Five
narrow readings, each given only the part of the change it needs.
Diffstat:
| A | .gitignore | | | 1 | + |
| A | client.go | | | 166 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | criteria/claims.md | | | 19 | +++++++++++++++++++ |
| A | criteria/duplication.md | | | 18 | ++++++++++++++++++ |
| A | criteria/hygiene.md | | | 17 | +++++++++++++++++ |
| A | criteria/namer.md | | | 22 | ++++++++++++++++++++++ |
| A | criteria/tests.md | | | 20 | ++++++++++++++++++++ |
| A | finding.go | | | 173 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | go.mod | | | 18 | ++++++++++++++++++ |
| A | go.sum | | | 26 | ++++++++++++++++++++++++++ |
| A | index.go | | | 161 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | job.go | | | 199 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | main.go | | | 222 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | packet.go | | | 318 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
| A | readme.md | | | 69 | +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ |
15 files changed, 1449 insertions(+), 0 deletions(-)
diff --git a/.gitignore b/.gitignore
@@ -0,0 +1 @@
+/review
diff --git a/client.go b/client.go
@@ -0,0 +1,166 @@
+package main
+
+import (
+ "context"
+ "fmt"
+ "regexp"
+ "strings"
+ "sync"
+
+ "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.
+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
+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.
+- 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.`
+
+// rule matches a rule id where the criteria define one, which is the list a
+// job is allowed to cite.
+var rule = regexp.MustCompile("(?m)^- `([a-z0-9-]+)`")
+
+// rules reads the ids a job may cite out of its own criteria.
+func rules(criteria string) map[string]bool {
+ out := map[string]bool{}
+ for _, match := range rule.FindAllStringSubmatch(criteria, -1) {
+ out[match[1]] = true
+ }
+ return out
+}
+
+// schema is what a job must answer in. It is enforced rather than requested:
+// strict validation means a malformed answer cannot arrive.
+var schema = anthropic.ToolInputSchemaParam{
+ Properties: map[string]any{
+ "findings": map[string]any{
+ "type": "array",
+ "items": map[string]any{
+ "type": "object",
+ "properties": map[string]any{
+ "rule": map[string]any{"type": "string", "description": "the rule id from the criteria"},
+ "severity": map[string]any{"type": "string", "enum": []string{"must-fix", "consider", "note"}},
+ "file": map[string]any{"type": "string"},
+ "line": map[string]any{"type": "integer"},
+ "symbol": map[string]any{"type": "string"},
+ "message": map[string]any{"type": "string", "description": "one sentence stating the finding"},
+ "fix": map[string]any{"type": "string", "description": "the concrete change to make"},
+ },
+ "required": []string{"rule", "severity", "message", "fix", "file", "line", "symbol"},
+ "additionalProperties": false,
+ },
+ },
+ },
+ Required: []string{"findings"},
+}
+
+// Reviewer runs jobs against a change.
+type Reviewer struct {
+ Client anthropic.Client
+ Verbose bool
+}
+
+// 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
+}
+
+// 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) {
+ tool := anthropic.ToolParam{
+ Name: "report_findings",
+ Description: anthropic.String("Report what this reading found, or an empty list."),
+ InputSchema: schema,
+ Strict: anthropic.Bool(true),
+ }
+ resp, err := r.Client.Messages.New(ctx, anthropic.MessageNewParams{
+ Model: 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,
+ CacheControl: anthropic.NewCacheControlEphemeralParam(),
+ }},
+ Tools: []anthropic.ToolUnionParam{{OfTool: &tool}},
+ Messages: []anthropic.MessageParam{
+ anthropic.NewUserMessage(anthropic.NewTextBlock(subject)),
+ },
+ })
+ if err != nil {
+ return nil, 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)
+ }
+ 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
+ }
+ out = append(out, answer.findings(job.Name, allowed)...)
+ }
+ return out, nil
+}
diff --git a/criteria/claims.md b/criteria/claims.md
@@ -0,0 +1,19 @@
+# Claims
+
+You are given comment and documentation lines the change adds. Some of them assert
+things about the world: how a system behaves, what a tool accepts, what a format
+requires. Those assertions are the subject.
+
+- `unsupported-claim` — the comment asserts an empirical fact about an external
+ system, and nothing in the change demonstrates it. Comments describing what the
+ code does are not claims; comments describing what Windows, macOS, a compiler or a
+ specification does are.
+- `overreaches` — the claim is broader than what could have been observed. "Adding
+ lighting avoids the crash" from one passing case is a rule invented from an
+ instance.
+- `cites-nothing-checkable` — the claim would need a source and gives none: no
+ version, no tool, no test that pins it.
+- `stale` — the comment describes behaviour the change itself has altered.
+
+A comment that is merely wordy is not a finding. Only judge assertions that could be
+false.
diff --git a/criteria/duplication.md b/criteria/duplication.md
@@ -0,0 +1,18 @@
+# Duplication
+
+You are given each name the change adds, and a list of existing declarations found by
+searching for its words. Decide whether the new thing already exists.
+
+- `already-named` — the repository already has a name for this concept. A second
+ name for one idea is worse than an awkward first one.
+- `restates-a-fact` — a constant or literal states something already stated
+ elsewhere: the same magic bytes, the same size, the same layout, written twice.
+ Facts belong to one owner.
+- `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.
+
+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.
diff --git a/criteria/hygiene.md b/criteria/hygiene.md
@@ -0,0 +1,17 @@
+# Commit hygiene
+
+You are given a commit message, the per-file statistics, and the subjects of recent
+commits in the same repository.
+
+- `message-omits-a-change` — the diff contains a change the message does not
+ describe. A rename swept into a commit about something else is the common case.
+- `message-claims-more` — the message describes work the diff does not contain.
+- `two-changes` — the diff is two unrelated changes that should be two commits.
+ Judge by whether one could be reverted without the other.
+- `subject-not-local-style` — the subject departs from the convention visible in the
+ recent subjects you were given. Follow what the repository does, not a house style
+ from elsewhere; if the recent subjects disagree with each other, say nothing.
+- `body-explains-what-not-why` — the body restates what the diff shows instead of
+ saying why it was done.
+
+The statistics are enough to notice a commit doing two things. Do not ask to see more.
diff --git a/criteria/namer.md b/criteria/namer.md
@@ -0,0 +1,22 @@
+# Naming
+
+Judge each name against these rules. Report only names the change adds or renames.
+
+- `noun-for-type` — a type is a noun or noun phrase. An adjective standing in for a
+ 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`).
+- `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
+ a standard library package the file imports (`binary`, `path`, `url`, `sort`).
+- `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.
+- `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.
diff --git a/criteria/tests.md b/criteria/tests.md
@@ -0,0 +1,20 @@
+# Test integrity
+
+You are given whole test functions the change adds or alters. The question is not
+whether they are tidy. The question is whether they can fail.
+
+- `cannot-fail` — nothing in the body could make the test fail. An assertion that
+ compares a literal to itself, or that only checks a key exists without checking
+ what is under it, tests nothing.
+- `skips-in-normal-conditions` — the test skips when a tool, file or environment
+ variable is missing, and that absence is ordinary rather than exceptional. A
+ skipping test reports success having checked nothing. Tests that hold code against
+ an external authority must fail when the authority is unavailable.
+- `name-overclaims` — the name says it checks a property the body does not check.
+- `passes-on-a-stub` — the test would still pass if the function under test returned
+ its input unchanged, or returned a zero value. Say which.
+- `asserts-the-shape-not-the-value` — it checks that a field or key is present but
+ never checks its content, so a wrong value passes.
+
+Report the specific assertion at fault, and what to assert instead. Ignore style,
+naming and table-versus-loop questions entirely.
diff --git a/finding.go b/finding.go
@@ -0,0 +1,173 @@
+package main
+
+import (
+ "bufio"
+ "cmp"
+ "fmt"
+ "os"
+ "regexp"
+ "slices"
+ "strconv"
+ "strings"
+)
+
+// Severity is what a finding means for the change.
+type Severity int
+
+// The severities, most serious first. Only MustFix is worth blocking on; the
+// rest are reported and left to judgement, because a loop that treats taste
+// as an error never finishes.
+const (
+ MustFix Severity = iota
+ Consider
+ Note
+)
+
+func (s Severity) String() string {
+ switch s {
+ case MustFix:
+ return "must-fix"
+ case Consider:
+ return "consider"
+ case Note:
+ return "note"
+ }
+ return "unknown"
+}
+
+// ParseSeverity reads a severity the way a job reports it, defaulting to the
+// least serious rather than failing: a job that invents a word should not
+// stop the run.
+func ParseSeverity(s string) Severity {
+ switch strings.ToLower(strings.TrimSpace(s)) {
+ case "must-fix", "must fix", "mustfix", "error":
+ return MustFix
+ case "consider", "warning":
+ return Consider
+ }
+ return Note
+}
+
+// Finding is one thing a job noticed.
+type Finding struct {
+ // Job is the job that reported it.
+ Job string `json:"job"`
+ // Rule is the criterion it was judged against. A finding that cites no
+ // rule is dropped, so that the criteria are what gets tuned rather than
+ // the prompt.
+ Rule string `json:"rule"`
+ // Severity is how seriously to take it.
+ Severity Severity `json:"-"`
+ // SeverityName is how it travels in and out of JSON.
+ SeverityName string `json:"severity"`
+ // File and Line locate it, where it has a location.
+ File string `json:"file,omitempty"`
+ Line int `json:"line,omitempty"`
+ // Symbol names what it concerns, for the findings that are about a name
+ // rather than a place.
+ Symbol string `json:"symbol,omitempty"`
+ // Message states the finding.
+ Message string `json:"message"`
+ // 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"`
+}
+
+func (f Finding) String() string {
+ var b strings.Builder
+ b.WriteString(f.Severity.String())
+ b.WriteString(": ")
+ if f.File != "" {
+ b.WriteString(f.File)
+ if f.Line > 0 {
+ fmt.Fprintf(&b, ":%d", f.Line)
+ }
+ b.WriteString(": ")
+ } else if f.Symbol != "" {
+ b.WriteString(f.Symbol)
+ b.WriteString(": ")
+ }
+ b.WriteString(f.Message)
+ if f.Fix != "" {
+ b.WriteString("\n → ")
+ b.WriteString(f.Fix)
+ }
+ b.WriteString("\n [")
+ b.WriteString(f.Job)
+ b.WriteString("/")
+ b.WriteString(f.Rule)
+ b.WriteString("]")
+ return b.String()
+}
+
+// Sort orders findings so the ones worth reading first are first.
+func Sort(findings []Finding) {
+ slices.SortStableFunc(findings, func(a, b Finding) int {
+ if order := cmp.Compare(a.Severity, b.Severity); order != 0 {
+ return order
+ }
+ if order := cmp.Compare(a.File, b.File); order != 0 {
+ return order
+ }
+ return cmp.Compare(a.Line, b.Line)
+ })
+}
+
+// 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.
+//
+// //review:ignore <rule> <why>
+var ignore = regexp.MustCompile(`//\s*review:ignore\s+(\S+)(?:\s+(.*))?`)
+
+// Suppressed reports whether the source dismisses a finding, and with what
+// reason. A dismissal on or just above the line it concerns covers that line;
+// one anywhere in a file covers the findings that name no line.
+func Suppressed(f Finding) (string, bool) {
+ if f.File == "" {
+ return "", false
+ }
+ file, err := os.Open(f.File)
+ if err != nil {
+ return "", false
+ }
+ defer file.Close()
+
+ var (
+ scanner = bufio.NewScanner(file)
+ line int
+ )
+ for scanner.Scan() {
+ line++
+ match := ignore.FindStringSubmatch(scanner.Text())
+ if match == nil || !rulesMatch(match[1], f.Rule) {
+ continue
+ }
+ why := strings.TrimSpace(match[2])
+ if why == "" {
+ why = "no reason given"
+ }
+ // A dismissal with no line to answer to covers the file; otherwise it
+ // has to sit within a few lines of what it dismisses, so that moving
+ // code does not carry a dismissal somewhere it was never meant.
+ if f.Line == 0 || (line >= f.Line-3 && line <= f.Line+1) {
+ return why, true
+ }
+ }
+ return "", false
+}
+
+// rulesMatch compares a dismissal against a rule, where "all" dismisses
+// anything the job found at that spot.
+func rulesMatch(dismissed, rule string) bool {
+ return dismissed == "all" || strings.EqualFold(dismissed, rule)
+}
+
+// atoi reads a line number a job reported, which may arrive as a string.
+func atoi(s string) int {
+ n, err := strconv.Atoi(strings.TrimSpace(s))
+ if err != nil {
+ return 0
+ }
+ return n
+}
diff --git a/go.mod b/go.mod
@@ -0,0 +1,18 @@
+module github.com/jackmordaunt/review
+
+go 1.27.0
+
+require (
+ github.com/anthropics/anthropic-sdk-go v1.74.0 // indirect
+ github.com/bahlo/generic-list-go v0.2.0 // indirect
+ github.com/buger/jsonparser v1.1.2 // indirect
+ github.com/invopop/jsonschema v0.14.0 // indirect
+ github.com/pb33f/ordered-map/v2 v2.3.1 // indirect
+ github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 // indirect
+ github.com/tidwall/gjson v1.18.0 // indirect
+ github.com/tidwall/match v1.1.1 // indirect
+ github.com/tidwall/pretty v1.2.1 // indirect
+ github.com/tidwall/sjson v1.2.5 // indirect
+ go.yaml.in/yaml/v4 v4.0.0-rc.2 // indirect
+ golang.org/x/sync v0.16.0 // indirect
+)
diff --git a/go.sum b/go.sum
@@ -0,0 +1,26 @@
+github.com/anthropics/anthropic-sdk-go v1.74.0 h1:u6pcrBJHLJZb1Q6uziqQ5VLvAjh82sJrreWmABdOOas=
+github.com/anthropics/anthropic-sdk-go v1.74.0/go.mod h1:x+lPk/cCl48uRegeP0hlYYBN1b7bEBTveInIMgLicnY=
+github.com/bahlo/generic-list-go v0.2.0 h1:5sz/EEAK+ls5wF+NeqDpk5+iNdMDXrh3z3nPnH1Wvgk=
+github.com/bahlo/generic-list-go v0.2.0/go.mod h1:2KvAjgMlE5NNynlg/5iLrrCCZ2+5xWbdbCW3pNTGyYg=
+github.com/buger/jsonparser v1.1.2 h1:frqHqw7otoVbk5M8LlE/L7HTnIq2v9RX6EJ48i9AxJk=
+github.com/buger/jsonparser v1.1.2/go.mod h1:6RYKKt7H4d4+iWqouImQ9R2FZql3VbhNgx27UK13J/0=
+github.com/invopop/jsonschema v0.14.0 h1:MHQqLhvpNUZfw+hM3AZDYK7jxO8FZoQeQM77g8iyZjg=
+github.com/invopop/jsonschema v0.14.0/go.mod h1:ygm6C2EaVNMBDPpaPlnOA2pFAxBnxGjFlMZABxm9n2I=
+github.com/pb33f/ordered-map/v2 v2.3.1 h1:5319HDO0aw4DA4gzi+zv4FXU9UlSs3xGZ40wcP1nBjY=
+github.com/pb33f/ordered-map/v2 v2.3.1/go.mod h1:qxFQgd0PkVUtOMCkTapqotNgzRhMPL7VvaHKbd1HnmQ=
+github.com/standard-webhooks/standard-webhooks/libraries v0.0.1 h1:uOfcYT+3QungH6tIGSVCR/Y3KJmgJiHcojJbMTPDZAI=
+github.com/standard-webhooks/standard-webhooks/libraries v0.0.1/go.mod h1:L1MQhA6x4dn9r007T033lsaZMv9EmBAdXyU/+EF40fo=
+github.com/tidwall/gjson v1.14.2/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
+github.com/tidwall/gjson v1.18.0 h1:FIDeeyB800efLX89e5a8Y0BNH+LOngJyGrIWxG2FKQY=
+github.com/tidwall/gjson v1.18.0/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk=
+github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA=
+github.com/tidwall/match v1.1.1/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM=
+github.com/tidwall/pretty v1.2.0/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
+github.com/tidwall/pretty v1.2.1 h1:qjsOFOWWQl+N3RsoF5/ssm1pHmJJwhjlSbZ51I6wMl4=
+github.com/tidwall/pretty v1.2.1/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU=
+github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY=
+github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28=
+go.yaml.in/yaml/v4 v4.0.0-rc.2 h1:/FrI8D64VSr4HtGIlUtlFMGsm7H7pWTbj6vOLVZcA6s=
+go.yaml.in/yaml/v4 v4.0.0-rc.2/go.mod h1:aZqd9kCMsGL7AuUv/m/PvWLdg5sjJsZ4oHDEnfPPfY0=
+golang.org/x/sync v0.16.0 h1:ycBJEhp9p4vXvUZNszeOq0kGTPghopOL8q0fq3vstxw=
+golang.org/x/sync v0.16.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA=
diff --git a/index.go b/index.go
@@ -0,0 +1,161 @@
+package main
+
+import (
+ "fmt"
+ "go/ast"
+ "go/parser"
+ "go/token"
+ "strings"
+)
+
+// Declared is one declaration somewhere in the repository, which a new name
+// might turn out to duplicate.
+type Declared struct {
+ Name string
+ Kind string
+ File string
+ Line int
+ // Text is the line it was declared on, which is what shows a reader that
+ // two constants hold the same number.
+ Text string
+}
+
+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.
+func Resembling(index []Declared, symbol Symbol, limit int) []string {
+ words := split(symbol.Name)
+ var out []string
+ seen := map[string]bool{}
+ for _, word := range words {
+ if len(word) < 4 {
+ continue
+ }
+ lower := strings.ToLower(word)
+ for _, declared := range index {
+ if declared.File == symbol.File && declared.Line == symbol.Line {
+ continue
+ }
+ if !strings.Contains(strings.ToLower(declared.Name), lower) {
+ continue
+ }
+ line := declared.String()
+ if seen[line] {
+ continue
+ }
+ seen[line] = true
+ out = append(out, line)
+ if len(out) >= limit {
+ return out
+ }
+ }
+ }
+ return out
+}
+
+// same reports whether two declarations hold the same literal, which is the
+// 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, "=")
+ return okA && okB && strings.TrimSpace(left) == strings.TrimSpace(right)
+}
+
+// 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 {
+ var (
+ words []string
+ word strings.Builder
+ )
+ for i, r := range name {
+ if i > 0 && r >= 'A' && r <= 'Z' {
+ words = append(words, word.String())
+ word.Reset()
+ }
+ word.WriteRune(r)
+ }
+ if word.Len() > 0 {
+ words = append(words, word.String())
+ }
+ return words
+}
diff --git a/job.go b/job.go
@@ -0,0 +1,199 @@
+package main
+
+import (
+ "embed"
+ "encoding/json"
+ "fmt"
+ "strings"
+)
+
+//go:embed criteria/*.md
+var criteria embed.FS
+
+// Job is one narrow reading of a change. Each is given the part of the change
+// it needs and nothing else: a job that reads less is cheaper, and harder to
+// distract into reporting something another job owns.
+type Job struct {
+ // Name is how it is asked for and how its findings are labelled.
+ Name string
+ // Criteria is the rules it judges against, and the thing to tune when it
+ // reports the wrong things.
+ Criteria string
+ // Subject renders the part of the change this job reads. An empty string
+ // means there is nothing here for it and the job is skipped, which is
+ // what keeps a documentation-only commit from paying for a test review.
+ Subject func(*Change) string
+}
+
+// Jobs are the readings, in the order their findings are worth having.
+func Jobs() []Job {
+ return []Job{
+ {Name: "duplication", Criteria: read("duplication"), Subject: duplicationSubject},
+ {Name: "tests", Criteria: read("tests"), Subject: testsSubject},
+ {Name: "namer", Criteria: read("namer"), Subject: namerSubject},
+ {Name: "claims", Criteria: read("claims"), Subject: claimsSubject},
+ {Name: "hygiene", Criteria: read("hygiene"), Subject: hygieneSubject},
+ }
+}
+
+func read(name string) string {
+ data, err := criteria.ReadFile("criteria/" + name + ".md")
+ if err != nil {
+ panic(err) // The criteria are embedded; a missing one is a build fault.
+ }
+ return string(data)
+}
+
+func namerSubject(c *Change) string {
+ if len(c.Symbols) == 0 {
+ return ""
+ }
+ var b strings.Builder
+ b.WriteString("Names this change adds or renames:\n\n")
+ for _, s := range c.Symbols {
+ fmt.Fprintf(&b, "%s:%d %s %s", s.File, s.Line, s.Kind, s.Name)
+ if s.Exported {
+ b.WriteString(" (exported)")
+ }
+ b.WriteString("\n")
+ if s.Signature != "" {
+ fmt.Fprintf(&b, " %s\n", s.Signature)
+ }
+ if s.Doc != "" {
+ fmt.Fprintf(&b, " doc: %s\n", firstLine(s.Doc))
+ }
+ if near := c.Candidates[s.Name]; len(near) > 0 {
+ fmt.Fprintf(&b, " names already in this repository: %s\n", strings.Join(short(near, 6), "; "))
+ }
+ b.WriteString("\n")
+ }
+ return b.String()
+}
+
+func duplicationSubject(c *Change) string {
+ if len(c.Symbols) == 0 {
+ return ""
+ }
+ var b strings.Builder
+ 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)
+ if s.Signature != "" {
+ fmt.Fprintf(&b, " %s\n", s.Signature)
+ }
+ if s.Doc != "" {
+ fmt.Fprintf(&b, " doc: %s\n", firstLine(s.Doc))
+ }
+ candidates := c.Candidates[s.Name]
+ if len(candidates) == 0 {
+ b.WriteString(" candidates: none found\n\n")
+ continue
+ }
+ b.WriteString(" candidates:\n")
+ for _, line := range candidates {
+ fmt.Fprintf(&b, " %s\n", line)
+ }
+ b.WriteString("\n")
+ }
+ return b.String()
+}
+
+func testsSubject(c *Change) string {
+ if len(c.Tests) == 0 {
+ return ""
+ }
+ var b strings.Builder
+ b.WriteString("Test functions this change adds or alters:\n\n")
+ for _, t := range c.Tests {
+ fmt.Fprintf(&b, "--- %s:%d %s\n%s\n\n", t.File, t.Line, t.Name, t.Body)
+ }
+ return b.String()
+}
+
+func claimsSubject(c *Change) string {
+ if len(c.Comments) == 0 {
+ return ""
+ }
+ var b strings.Builder
+ b.WriteString("Comment and documentation lines this change adds:\n\n")
+ for _, comment := range c.Comments {
+ fmt.Fprintf(&b, "%s:%d %s\n", comment.File, comment.Line, comment.Text)
+ }
+ return b.String()
+}
+
+func hygieneSubject(c *Change) string {
+ if strings.TrimSpace(c.Message) == "" {
+ return ""
+ }
+ var b strings.Builder
+ b.WriteString("Commit message:\n\n")
+ b.WriteString(c.Message)
+ b.WriteString("\n\nFiles changed:\n")
+ b.WriteString(c.Stat)
+ if len(c.Convention) > 0 {
+ b.WriteString("\nRecent subjects in this repository, as the local convention:\n")
+ for _, subject := range c.Convention {
+ fmt.Fprintf(&b, " %s\n", subject)
+ }
+ }
+ return b.String()
+}
+
+func firstLine(s string) string {
+ if i := strings.IndexByte(s, '\n'); i >= 0 {
+ s = s[:i]
+ }
+ if len(s) > 140 {
+ s = s[:140] + "…"
+ }
+ return s
+}
+
+func short(lines []string, n int) []string {
+ if len(lines) > n {
+ lines = lines[:n]
+ }
+ return lines
+}
+
+// reported is the shape a job answers in. It is enforced by the schema rather
+// than asked for in prose, so a malformed answer is impossible rather than
+// merely unlikely.
+type reported struct {
+ Findings []struct {
+ Rule string `json:"rule"`
+ Severity string `json:"severity"`
+ File string `json:"file"`
+ Line int `json:"line"`
+ Symbol string `json:"symbol"`
+ Message string `json:"message"`
+ Fix string `json:"fix"`
+ } `json:"findings"`
+}
+
+func (r reported) findings(job string, rules map[string]bool) []Finding {
+ var out []Finding
+ for _, f := range r.Findings {
+ // A finding that cites no rule from the criteria is dropped: the
+ // criteria are what gets tuned, so a job may not invent one.
+ if !rules[f.Rule] {
+ continue
+ }
+ out = append(out, Finding{
+ Job: job, Rule: f.Rule,
+ Severity: ParseSeverity(f.Severity), SeverityName: f.Severity,
+ File: f.File, Line: f.Line, Symbol: f.Symbol,
+ Message: f.Message, Fix: f.Fix,
+ })
+ }
+ return out
+}
+
+func decode(raw string) (reported, error) {
+ var r reported
+ if err := json.Unmarshal([]byte(raw), &r); err != nil {
+ return reported{}, fmt.Errorf("reading the answer: %w", err)
+ }
+ return r, nil
+}
diff --git a/main.go b/main.go
@@ -0,0 +1,222 @@
+// Command review reads a change the way several narrow readers would, and
+// reports what they noticed.
+//
+// It is advisory. It never fails a build and never blocks a commit: the
+// 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.
+//
+// review the staged change
+// review HEAD^ the last commit
+// review --json for a program rather than a person
+//
+// A finding is dismissed where it is wrong, in the source it concerns:
+//
+// //review:ignore <rule> <why>
+package main
+
+import (
+ "context"
+ "encoding/json"
+ "flag"
+ "fmt"
+ "os"
+ "os/exec"
+ "strings"
+
+ "github.com/anthropics/anthropic-sdk-go"
+)
+
+func main() {
+ if err := run(); err != nil {
+ fmt.Fprintln(os.Stderr, "review:", err)
+ os.Exit(1)
+ }
+}
+
+func run() error {
+ var (
+ asJSON bool
+ verbose bool
+ show bool
+ only string
+ )
+ 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.StringVar(&only, "jobs", "", "Run only these jobs, comma separated.")
+ flag.Usage = usage
+ flag.Parse()
+
+ root, err := repository()
+ if err != nil {
+ return err
+ }
+ change, err := Gather(flag.Arg(0), root)
+ if err != nil {
+ return err
+ }
+ if strings.TrimSpace(change.Diff) == "" {
+ if !asJSON {
+ fmt.Println("nothing to review")
+ } else {
+ fmt.Println(`{"findings":[]}`)
+ }
+ return nil
+ }
+
+ jobs, err := chosen(only)
+ if err != nil {
+ return err
+ }
+ if show {
+ for _, job := range jobs {
+ subject := job.Subject(change)
+ fmt.Printf("=== %s ===\n", job.Name)
+ if strings.TrimSpace(subject) == "" {
+ fmt.Print("(nothing to read)\n\n")
+ continue
+ }
+ fmt.Printf("%s\n", subject)
+ }
+ return nil
+ }
+ if verbose {
+ fmt.Printf("reviewing %d files, %d new names, %d tests, %d comments\n",
+ 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)
+
+ kept, dismissed := filter(root, findings)
+ Sort(kept)
+
+ if asJSON {
+ return report(kept)
+ }
+ render(kept, dismissed, 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) {
+ if err := os.Chdir(root); err != nil {
+ return findings, 0
+ }
+ for _, f := range findings {
+ if _, ok := Suppressed(f); ok {
+ dismissed++
+ continue
+ }
+ kept = append(kept, f)
+ }
+ return kept, dismissed
+}
+
+func render(findings []Finding, dismissed int, failures []error, verbose bool) {
+ for _, err := range failures {
+ fmt.Fprintln(os.Stderr, " job failed:", err)
+ }
+ if len(findings) == 0 {
+ fmt.Print("no findings")
+ if dismissed > 0 {
+ fmt.Printf(" (%d dismissed in the source)", dismissed)
+ }
+ fmt.Println()
+ return
+ }
+ var severity Severity = -1
+ for _, f := range findings {
+ if f.Severity != severity {
+ severity = f.Severity
+ fmt.Printf("\n%s\n", strings.ToUpper(severity.String()))
+ }
+ 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)
+ }
+ fmt.Println()
+ if verbose {
+ fmt.Println("\nDismiss a finding where it is wrong, in the source it concerns:")
+ fmt.Println(" //review:ignore <rule> <why>")
+ }
+}
+
+func indent(s string) string {
+ return strings.ReplaceAll(s, "\n ", "\n ")
+}
+
+func report(findings []Finding) error {
+ if findings == nil {
+ findings = []Finding{}
+ }
+ for i := range findings {
+ findings[i].SeverityName = findings[i].Severity.String()
+ }
+ out, err := json.MarshalIndent(map[string]any{"findings": findings}, "", " ")
+ if err != nil {
+ return err
+ }
+ fmt.Println(string(out))
+ return nil
+}
+
+func chosen(only string) ([]Job, error) {
+ all := Jobs()
+ if only == "" {
+ return all, nil
+ }
+ var out []Job
+ for _, name := range strings.Split(only, ",") {
+ name = strings.TrimSpace(name)
+ found := false
+ for _, job := range all {
+ if job.Name == name {
+ out = append(out, job)
+ found = true
+ }
+ }
+ if !found {
+ return nil, fmt.Errorf("no job called %q", name)
+ }
+ }
+ return out, nil
+}
+
+func repository() (string, error) {
+ out, err := exec.Command("git", "rev-parse", "--show-toplevel").Output()
+ if err != nil {
+ return "", fmt.Errorf("not inside a git repository")
+ }
+ return strings.TrimSpace(string(out)), nil
+}
+
+func usage() {
+ fmt.Fprint(flag.CommandLine.Output(), `review reads a change the way several narrow readers would.
+
+Usage:
+ review [flags] [revision]
+
+With no revision the staged change is read. A revision is anything git diff
+takes, such as HEAD^ for the last commit.
+
+Flags:
+`)
+ flag.PrintDefaults()
+ fmt.Fprint(flag.CommandLine.Output(), `
+Jobs:
+ duplication whether the change states something the repository already states
+ tests whether the tests it adds can fail
+ namer whether the names it adds say what the things are
+ claims whether the comments it adds assert what nobody checked
+ hygiene whether the commit message matches the commit
+
+Dismiss a finding where it is wrong, in the source it concerns:
+ //review:ignore <rule> <why>
+`)
+}
diff --git a/packet.go b/packet.go
@@ -0,0 +1,318 @@
+package main
+
+import (
+ "fmt"
+ "go/ast"
+ "go/parser"
+ "go/token"
+ "os"
+ "os/exec"
+ "path/filepath"
+ "regexp"
+ "slices"
+ "strings"
+)
+
+// Change is everything the jobs are allowed to see, gathered once. Each job
+// takes the part it needs and no more: a job that reads less is both cheaper
+// and harder to distract.
+type Change struct {
+ // Diff is the change itself, capped.
+ Diff string
+ // Files are the paths it touches.
+ Files []string
+ // Message is the commit message under review.
+ Message string
+ // Stat is the per-file line counts, which is enough to notice a commit
+ // doing two things without reading either.
+ Stat string
+ // Symbols are the declarations the change adds or renames.
+ Symbols []Symbol
+ // Tests are the test functions it adds or changes, whole.
+ Tests []Function
+ // Comments are the comment and documentation lines it adds.
+ Comments []Located
+ // Candidates are existing names that resemble each new one, gathered by
+ // search rather than by the model, which is what keeps the duplication
+ // job cheap.
+ Candidates map[string][]string
+ // Convention is the recent commit subjects, so a job can read the local
+ // habit rather than impose one.
+ Convention []string
+}
+
+// Symbol is a declaration the change introduces.
+type Symbol struct {
+ Name string `json:"name"`
+ Kind string `json:"kind"`
+ Doc string `json:"doc,omitempty"`
+ File string `json:"file"`
+ Line int `json:"line"`
+ Exported bool `json:"exported"`
+ // Signature is the declaration line, which is what a name has to be
+ // judged against.
+ Signature string `json:"signature,omitempty"`
+}
+
+// Function is a whole function, which the test job needs because a test can
+// only be judged by what it asserts.
+type Function struct {
+ Name string `json:"name"`
+ File string `json:"file"`
+ Line int `json:"line"`
+ Body string `json:"body"`
+}
+
+// Located is a line of prose and where it came from.
+type Located struct {
+ Text string `json:"text"`
+ File string `json:"file"`
+ Line int `json:"line"`
+}
+
+// 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.
+func at(root, rev, path string) ([]byte, error) {
+ if rev == "" {
+ 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.Dir = root
+ return cmd.Output()
+}
+
+// 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) {
+ var diffArgs, nameArgs, statArgs []string
+ switch rev {
+ case "":
+ diffArgs = []string{"diff", "--cached", "-U3"}
+ nameArgs = []string{"diff", "--cached", "--name-only"}
+ statArgs = []string{"diff", "--cached", "--stat"}
+ default:
+ diffArgs = []string{"diff", rev, "-U3"}
+ nameArgs = []string{"diff", rev, "--name-only"}
+ statArgs = []string{"diff", rev, "--stat"}
+ }
+
+ change := &Change{Candidates: 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"
+ }
+ names, err := git(root, nameArgs...)
+ if err != nil {
+ return nil, err
+ }
+ for _, name := range strings.Split(strings.TrimSpace(names), "\n") {
+ if name != "" {
+ change.Files = append(change.Files, name)
+ }
+ }
+ if change.Stat, err = git(root, statArgs...); err != nil {
+ return nil, err
+ }
+ if rev == "" {
+ change.Message, _ = git(root, "log", "-1", "--format=%B")
+ change.Message = "(uncommitted; the message below is the previous commit's)\n" + change.Message
+ } else {
+ change.Message, _ = git(root, "log", "-1", "--format=%B", strings.TrimSuffix(rev, "^"))
+ }
+ if subjects, err := git(root, "log", "-12", "--format=%s"); err == nil {
+ for _, s := range strings.Split(strings.TrimSpace(subjects), "\n") {
+ if s != "" {
+ change.Convention = append(change.Convention, s)
+ }
+ }
+ }
+
+ change.read(root, rev)
+ change.findCandidates(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.
+ }
+ 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
+ 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]),
+ })
+ }
+ }
+ 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,
+ })
+ }
+ }
+ }
+}
+
+// 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
+ }
+ // 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.
+ for _, symbol := range c.Symbols {
+ var twins []string
+ for _, declared := range index {
+ if declared.File == symbol.File && declared.Line == symbol.Line {
+ continue
+ }
+ if same(declared, Declared{Text: symbol.Signature}) {
+ twins = append(twins, declared.String()+" <- same value")
+ }
+ }
+ c.Candidates[symbol.Name] = append(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{}
+ var (
+ file string
+ line int
+ )
+ for _, text := range strings.Split(diff, "\n") {
+ switch {
+ case strings.HasPrefix(text, "+++ b/"):
+ file = strings.TrimPrefix(text, "+++ b/")
+ case hunk.MatchString(text):
+ line = atoi(hunk.FindStringSubmatch(text)[1])
+ case strings.HasPrefix(text, "+") && file != "":
+ out[file] = append(out[file], line)
+ line++
+ case strings.HasPrefix(text, "-"):
+ default:
+ line++
+ }
+ }
+ return out
+}
+
+func touchedBetween(lines []int, from, to int) bool {
+ for _, line := range lines {
+ if line >= from && line <= to {
+ return true
+ }
+ }
+ return false
+}
+
+func text(lines []string, from, to int) string {
+ if from < 1 || to > len(lines) {
+ return ""
+ }
+ 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
+ }
+ return path
+}
+
+func git(root string, args ...string) (string, error) {
+ cmd := exec.Command("git", args...)
+ cmd.Dir = root
+ out, err := cmd.Output()
+ if err != nil {
+ return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err)
+ }
+ return string(out), nil
+}
diff --git a/readme.md b/readme.md
@@ -0,0 +1,69 @@
+# review
+
+Reads a change the way several narrow readers would, and reports what they noticed.
+
+It is **advisory**. It never fails a build and never blocks a commit — the deterministic
+checks a repository already runs are what should do that. This looks at what those
+checks cannot: whether a name says what the thing is, whether a fact is already stated
+somewhere else, whether a test can fail, whether a comment claims something nobody
+verified.
+
+```
+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
+```
+
+## The jobs
+
+Each is a separate call, run at once, given only the part of the change it needs. A job
+that reads less is cheaper and harder to distract into reporting something another job
+owns.
+
+| job | reads | asks |
+|---|---|---|
+| `duplication` | new declarations + existing ones that resemble them | does the repository already state this? |
+| `tests` | whole test functions the change adds | could this test fail? |
+| `namer` | new and renamed declarations, with their doc comments | does the name say what the thing is? |
+| `claims` | comment lines the change adds | does anything support this assertion? |
+| `hygiene` | the commit message, the file statistics, recent subjects | does the message match the commit? |
+
+The candidate list the `duplication` job judges is built by parsing the repository, not
+by searching it. A pattern over lines misses an indented constant inside a block, which
+is exactly where a duplicated fact tends to live.
+
+## Criteria
+
+`criteria/*.md` holds the rules, one file per job, each rule with an id. **Every finding
+must cite one**, and a finding citing anything else is dropped before you see it. That
+is deliberate: it makes the criteria the thing you tune, rather than the prompt, and it
+stops a job inventing a standard on the spot.
+
+## Dismissing a finding
+
+Where a finding is wrong, say so in the source it concerns:
+
+```go
+//review:ignore already-named the zero value, taken by omission
+```
+
+Not a configuration file. The reason belongs beside the code it justifies, where a
+reader meets it, and it survives a clone. `//review:ignore all <why>` dismisses
+everything at that spot.
+
+## Running it
+
+Needs credentials for the Anthropic API — `ant auth login`, or `ANTHROPIC_API_KEY`.
+
+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.
+
+Findings arrive through a strict tool schema rather than as prose to be parsed, so a
+malformed answer is impossible rather than merely unlikely.
+
+## What it is not
+
+It does not check formatting, vet, lint, dead code, or vulnerabilities. Those have
+deterministic tools that are better at it, cost nothing to run, and can be trusted to
+gate a merge. Point this at what is left over.
