commit cdc53fe8abd7ef32d5f46b3522875c2db7ee0ce7
parent b19c14be45b47b618803c6ad262e6955ca320ab4
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Wed, 23 Sep 2026 20:43:27 -0300
odin: gate commits, compare against a baseline, and measure the checks
hook prints or installs the commit-msg hook that reviews the staged
change with the message being committed and refuses on a must-fix, and
the stanza an agent's harness takes; agent prints what an agent's
instructions should say. --message-file reads the message a hook is
given, comment lines stripped; --baseline names each finding as new,
persisting or resolved against a previous report. bench counts what
fires over recent commits, per rule or listing one rule's commits, and
rules -dismissed counts the dismissals in the tree. The shared word and
JSON-object readings move to one package, txt, so no body is written
twice.
Diffstat:
12 files changed, 820 insertions(+), 6 deletions(-)
diff --git a/justfile b/justfile
@@ -41,7 +41,7 @@ odin-test:
#!/usr/bin/env bash
set -euo pipefail
mkdir -p build
- for p in txt frontend git tree change finding report check analyser job provider cache reviewer; do
+ for p in txt frontend git tree change finding report check analyser job provider cache reviewer hook bench; do
{{odin}} test odin/$p {{odin_flags}} -out:build/${p}_test
done
diff --git a/odin/bench/bench.odin b/odin/bench/bench.odin
@@ -0,0 +1,160 @@
+/*
+Package bench measures the deterministic checks over as much history as
+there is: run them over the last N commits and count what fires. A rule
+that fires on a tenth of ordinary commits is not measuring what it claims
+to.
+*/
+package bench
+
+import "core:fmt"
+import "core:slice"
+import "core:strings"
+
+import "../change"
+import "../check"
+import "../git"
+import "../tree"
+
+// Options are what a bench is narrowed to: how many commits, whose, and
+// which rule to list the commits of.
+Options :: struct {
+ n: int,
+ author: string,
+ rule: string,
+}
+
+// Fire is one commit a rule fired on.
+Fire :: struct {
+ commit, subject, message: string,
+}
+
+// run measures the checks over the repository's recent commits and
+// reports the fire rate per rule, or the commits one rule fired on.
+// progress is told how far along the measuring is.
+run :: proc(
+ root: string,
+ opts: Options,
+ progress: proc(done, total: int),
+ allocator := context.allocator,
+) -> (
+ out: string,
+ err: string,
+) {
+ context.allocator = allocator
+ listing := make([dynamic]string, context.temp_allocator)
+ append(&listing, "rev-list", "--no-merges", fmt.tprintf("-%d", opts.n))
+ if opts.author != "" {
+ append(&listing, strings.concatenate({"--author=", opts.author}, context.temp_allocator))
+ }
+ append(&listing, "HEAD")
+ commits, listed := git.lines(root, listing[:], context.temp_allocator)
+ if !listed || len(commits) == 0 {
+ return "", "no commits to measure"
+ }
+ fires := make(map[string][dynamic]Fire, context.temp_allocator)
+ measured := 0
+ for commit, i in commits {
+ if progress != nil {
+ progress(i + 1, len(commits))
+ }
+ rev := fmt.tprintf("%s^..%s", commit, commit)
+ c, gathered := change.gather(rev, root, context.temp_allocator)
+ if !gathered {
+ continue // A root commit has no parent to diff against.
+ }
+ t, at_ok := tree.at(root, rev, context.temp_allocator)
+ if !at_ok {
+ continue
+ }
+ change.read(&c, t, context.temp_allocator)
+ c.index, _ = change.index(t, context.temp_allocator)
+ findings := check.run(
+ check.scope_of(&c, t, context.temp_allocator),
+ context.temp_allocator,
+ )
+ tree.close(t) // One tree per commit; a bench must not keep them all.
+ measured += 1
+ seen := make(map[string]bool, context.temp_allocator)
+ subject := strings.trim_space(c.message)
+ if nl := strings.index_byte(subject, '\n'); nl >= 0 {
+ subject = subject[:nl]
+ }
+ for f in findings {
+ if seen[f.rule] {
+ continue
+ }
+ seen[f.rule] = true
+ list := fires[f.rule]
+ if list.allocator.procedure == nil {
+ list = make([dynamic]Fire, context.temp_allocator)
+ }
+ append(&list, Fire{commit[:min(8, len(commit))], subject, f.message})
+ fires[strings.clone(f.rule, context.temp_allocator)] = list
+ }
+ }
+ b := strings.builder_make(allocator)
+ if opts.rule != "" {
+ listed_fires := fires[opts.rule]
+ for f in listed_fires {
+ fmt.sbprintf(
+ &b,
+ "%s %s\n %s\n",
+ f.commit,
+ f.subject,
+ f.message[:min(160, len(f.message))],
+ )
+ }
+ fmt.sbprintf(
+ &b,
+ "\n%s fired on %d of %d commits\n",
+ opts.rule,
+ len(listed_fires),
+ measured,
+ )
+ return strings.to_string(b), ""
+ }
+ rules, _ := slice.map_keys(fires, context.temp_allocator)
+ slice.sort_by_cmp(rules, proc(a, b: string) -> slice.Ordering {
+ return .Less if a < b else (.Greater if a > b else .Equal)
+ })
+ // The most frequent first, the rest by name.
+ counted = &fires
+ slice.stable_sort_by(rules, proc(a, b: string) -> bool {
+ return len(fires_of(a)) > len(fires_of(b))
+ })
+ counted = nil
+ fmt.sbprintf(&b, "%d commits measured\n\n", measured)
+ fmt.sbprintf(&b, "%-28s %6s %s\n", "rule", "fires", "rate")
+ for r in rules {
+ count := strings.right_justify(
+ fmt.tprintf("%d", len(fires[r])),
+ 6,
+ " ",
+ context.temp_allocator,
+ )
+ rate := strings.right_justify(
+ fmt.tprintf("%.1f%%", 100 * f64(len(fires[r])) / f64(measured)),
+ 5,
+ " ",
+ context.temp_allocator,
+ )
+ fmt.sbprintf(&b, "%-28s %s %s\n", r, count, rate)
+ }
+ if len(rules) == 0 {
+ strings.write_string(&b, "nothing fired\n")
+ }
+ return strings.to_string(b), ""
+}
+
+// The fires under measurement, reachable from the sort's comparator,
+// which cannot capture them.
+@(private)
+counted: ^map[string][dynamic]Fire
+
+@(private)
+fires_of :: proc(rule: string) -> [dynamic]Fire {
+ if counted == nil {
+ return {}
+ }
+ return counted[rule]
+}
diff --git a/odin/bench/bench_test.odin b/odin/bench/bench_test.odin
@@ -0,0 +1,59 @@
+package bench
+
+import "core:os"
+import "core:path/filepath"
+import "core:strings"
+import "core:testing"
+import "jfm:sh"
+
+@(test)
+bench_counts_what_fires :: proc(t: ^testing.T) {
+ context.allocator = context.temp_allocator
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ root, err := os.make_directory_temp(temp, "review-bench-*", context.temp_allocator)
+ testing.expect(t, err == nil)
+ defer os.remove_all(root)
+ git := proc(root: string, args: ..string) -> bool {
+ argv := make([dynamic]string, context.temp_allocator)
+ append(
+ &argv,
+ "git",
+ "-c",
+ "user.email=t@t",
+ "-c",
+ "user.name=t",
+ "-c",
+ "commit.gpgsign=false",
+ )
+ append(&argv, ..args)
+ return sh.exec(argv[:], {dir = root}, context.temp_allocator).ok
+ }
+ write := proc(root, name, src: string) -> bool {
+ path := filepath.join({root, name}, context.temp_allocator) or_else name
+ return os.write_entire_file(path, transmute([]byte)src) == nil
+ }
+ testing.expect(t, git(root, "init", "-q"))
+ testing.expect(t, write(root, "x.go", "package x\n"))
+ testing.expect(t, git(root, "add", "x.go"))
+ testing.expect(t, git(root, "commit", "-q", "-m", "x: begin"))
+ testing.expect(t, write(root, "x.go", "package x\n\nvar a = 1\n"))
+ testing.expect(t, git(root, "add", "x.go"))
+ testing.expect(t, git(root, "commit", "-q", "-m", "whoops"))
+ testing.expect(t, write(root, "x.go", "package x\n\nvar a = 1\nvar b = 2\n"))
+ testing.expect(t, git(root, "add", "x.go"))
+ testing.expect(t, git(root, "commit", "-q", "-m", "x: add b"))
+
+ out, run_err := run(root, Options{n = 10}, nil)
+ testing.expect_value(t, run_err, "")
+ testing.expect(t, strings.contains(out, "2 commits measured"), out)
+ testing.expect(t, strings.contains(out, "message-frustration"), out)
+ listed, _ := run(root, Options{n = 10, rule = "message-frustration"}, nil)
+ testing.expect(t, strings.contains(listed, "whoops"), listed)
+ testing.expect(
+ t,
+ strings.contains(listed, "message-frustration fired on 1 of 2 commits"),
+ listed,
+ )
+ _, none := run(root, Options{n = 10, author = "nobody-here"}, nil)
+ testing.expect_value(t, none, "no commits to measure")
+}
diff --git a/odin/change/change.odin b/odin/change/change.odin
@@ -178,6 +178,29 @@ gather :: proc(rev, root: string, allocator := context.allocator) -> (c: Change,
return c, true
}
+// read_message reads a commit message the way git will: the lines its
+// comment character opens are the template's, not the author's, and a
+// message file a hook is given is full of them.
+read_message :: proc(path: string, allocator := context.allocator) -> (message: string, ok: bool) {
+ data, err := os.read_entire_file_from_path(path, context.temp_allocator)
+ if err != nil {
+ return "", false
+ }
+ kept := make([dynamic]string, context.temp_allocator)
+ rest := string(data)
+ for line in strings.split_lines_iterator(&rest) {
+ if strings.has_prefix(line, "#") {
+ continue
+ }
+ append(&kept, line)
+ }
+ return strings.clone(
+ strings.trim_space(strings.join(kept[:], "\n", context.temp_allocator)),
+ allocator,
+ ),
+ true
+}
+
// count_numstat sums the lines added and removed over git's --numstat
// output. A binary file's counts are dashes and count nothing.
count_numstat :: proc(out: string) -> int {
diff --git a/odin/change/change_test.odin b/odin/change/change_test.odin
@@ -247,6 +247,28 @@ temporal_counts_the_pair :: proc(t: ^testing.T) {
}
@(test)
+a_message_file_loses_its_template :: proc(t: ^testing.T) {
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ path := filepath.join({temp, "review_message_fixture.txt"}, context.temp_allocator) or_else ""
+ testing.expect(
+ t,
+ os.write_entire_file(
+ path,
+ transmute([]byte)string(
+ "x: do it\n\n# Please enter the commit message\n# Lines starting with '#' are ignored\nbecause it matters\n",
+ ),
+ ) ==
+ nil,
+ )
+ defer os.remove(path)
+ message, ok := read_message(path, context.temp_allocator)
+ testing.expect(t, ok)
+ testing.expect_value(t, message, "x: do it\n\nbecause it matters")
+ _, missing := read_message("/nowhere/at/all", context.temp_allocator)
+ testing.expect(t, !missing)
+}
+
+@(test)
code_files_are_the_ones_a_reader_could_exist_for :: proc(t: ^testing.T) {
testing.expect(t, is_code_file("a/b.zig"))
testing.expect(t, is_code_file("Justfile"))
diff --git a/odin/check/check_test.odin b/odin/check/check_test.odin
@@ -1265,6 +1265,36 @@ over_scope :: proc(s: Scope) -> []finding.Finding {
}
@(test)
+dismissals_are_counted_per_rule :: proc(t: ^testing.T) {
+ context.allocator = context.temp_allocator
+ sources := make(map[string][]byte)
+ sources["a.go"] = transmute([]byte)strings.concatenate(
+ {
+ "package a\n//review:",
+ "ignore no-shadow the loop's\nvar x = 1\n// review:",
+ "ignore no-shadow\n",
+ },
+ )
+ sources["b_test.go"] = transmute([]byte)strings.concatenate(
+ {"const fixture = \"//review:", "ignore cannot-fail it can\\nconst b = 6\"\n"},
+ )
+ sources["readme.md"] = transmute([]byte)strings.concatenate(
+ {"`//review:", "ignore <rule> <why>`\n"},
+ )
+ out := dismissals(Scope{sources = sources})
+ testing.expect(t, strings.has_prefix(out, "no-shadow 2\n"), out)
+ testing.expect(t, strings.contains(out, " a.go:2 the loop's\n"), out)
+ testing.expect(t, strings.contains(out, " a.go:4 no reason given\n"), out)
+ testing.expect(
+ t,
+ strings.contains(out, "cannot-fail 1\n b_test.go:1 it can\n"),
+ out,
+ )
+ testing.expect(t, !strings.contains(out, "<rule>"), out)
+ testing.expect_value(t, dismissals(Scope{}), "no dismissals in the tree\n")
+}
+
+@(test)
names_unknown_reads_the_tree :: proc(t: ^testing.T) {
if !frontend.installed(.Go) {
testing.fail_now(t, "review-go is not on the path")
diff --git a/odin/check/gaming.odin b/odin/check/gaming.odin
@@ -52,6 +52,65 @@ check_suppression_added :: proc(s: Scope, out: ^[dynamic]finding.Finding) {
)
}
+// dismissals counts the dismissals in the tree per rule, with where each
+// is and why, so that the rules people argue with are visible: a rule
+// dismissed everywhere is a rule to rewrite.
+dismissals :: proc(s: Scope, allocator := context.allocator) -> string {
+ Spot :: struct {
+ where_at, why: string,
+ }
+ by_rule := make(map[string][dynamic]Spot, context.temp_allocator)
+ for file in sorted_keys(s.sources) {
+ if !change.is_code_file(file) {
+ continue
+ }
+ rest := string(s.sources[file])
+ number := 0
+ for line in strings.split_lines_iterator(&rest) {
+ number += 1
+ rule, why, found := finding.dismissal(line)
+ if !found || !matches(rule_id, rule) {
+ continue
+ }
+ // A dismissal quoted inside a string literal, as a test's
+ // fixture is, ends where the literal's line does.
+ if cut := strings.index(why, "\\n"); cut >= 0 {
+ why = why[:cut]
+ }
+ why = strings.trim_space(strings.trim_suffix(strings.trim_space(why), "\""))
+ if why == "" {
+ why = "no reason given"
+ }
+ spots := by_rule[rule]
+ if spots.allocator.procedure == nil {
+ spots = make([dynamic]Spot, context.temp_allocator)
+ }
+ append(&spots, Spot{fmt.tprintf("%s:%d", file, number), why})
+ by_rule[strings.clone(rule, context.temp_allocator)] = spots
+ }
+ }
+ b := strings.builder_make(allocator)
+ if len(by_rule) == 0 {
+ strings.write_string(&b, "no dismissals in the tree\n")
+ return strings.to_string(b)
+ }
+ rules := sorted_keys(by_rule)
+ // The most dismissed first, the rest by name.
+ for i in 1 ..< len(rules) {
+ for j := i; j > 0 && len(by_rule[rules[j]]) > len(by_rule[rules[j - 1]]); j -= 1 {
+ rules[j], rules[j - 1] = rules[j - 1], rules[j]
+ }
+ }
+ for rule in rules {
+ spots := by_rule[rule]
+ fmt.sbprintf(&b, "%-28s %d\n", rule, len(spots))
+ for spot in spots {
+ fmt.sbprintf(&b, " %s %s\n", spot.where_at, spot.why)
+ }
+ }
+ return strings.to_string(b)
+}
+
// The shapes a test's declaration takes on a diff line, per runner.
go_test :: `^func ((?:Test|Benchmark|Fuzz)[A-Za-z0-9_]+)\(`
py_test :: `^(?:async\s+)?def (test_\w+)\(`
diff --git a/odin/hook/hook.odin b/odin/hook/hook.odin
@@ -0,0 +1,201 @@
+/*
+Package hook is what makes the review a gate: a commit-msg hook, which
+gates every agent that commits through git, and the stanza an agent's own
+harness takes. The text an agent needs in its instructions is printed
+from the binary too, so the tool explains itself wherever it is.
+*/
+package hook
+
+import "core:fmt"
+import "core:os"
+import "core:path/filepath"
+import "core:strings"
+
+import "../git"
+
+// run is the hook subcommand: install, or print.
+run :: proc(args: []string) -> (out: string, err: string) {
+ force := false
+ verb := ""
+ for arg in args {
+ switch arg {
+ case "-force", "--force":
+ force = true
+ case:
+ verb = arg
+ }
+ }
+ switch verb {
+ case "install":
+ return install(force)
+ case "print", "":
+ return text(), ""
+ }
+ return "", fmt.aprintf("hook takes install or print, not %q", verb)
+}
+
+// install writes the commit-msg hook into the repository, naming this
+// binary by its absolute path so the hook works wherever the shell's path
+// does not reach. A hook already there is not overwritten unasked: it is
+// someone's, and it may do more than this.
+install :: proc(force: bool) -> (out: string, err: string) {
+ cwd, cwd_err := os.get_working_directory(context.temp_allocator)
+ if cwd_err != nil {
+ return "", "no working directory"
+ }
+ root, in_repo := git.toplevel(cwd, context.temp_allocator)
+ if !in_repo {
+ return "", "not in a git repository"
+ }
+ // With core.hooksPath set, git reads hooks from one directory for
+ // every repository and ignores .git/hooks. Writing there would change
+ // every repository on the machine, and writing to .git/hooks would
+ // change nothing; neither is this tool's to do unasked.
+ if shared, set := git.run(root, {"config", "--get", "core.hooksPath"}, context.temp_allocator);
+ set && strings.trim_space(shared) != "" {
+ where_at := strings.trim_space(shared)
+ return commit_msg_hook(
+
+ ), fmt.aprintf("core.hooksPath is %s, so git reads hooks there and not from .git/hooks; add the exec line above to %s/commit-msg yourself", where_at, where_at)
+ }
+ dir, found := git.run(root, {"rev-parse", "--git-path", "hooks"}, context.temp_allocator)
+ if !found {
+ return "", "git names no hooks directory"
+ }
+ dir = strings.trim_space(dir)
+ if !os.is_absolute_path(dir) {
+ dir = filepath.join({root, dir}, context.temp_allocator) or_else dir
+ }
+ path := filepath.join({dir, "commit-msg"}, context.temp_allocator) or_else ""
+ if os.exists(path) && !force {
+ return "", fmt.aprintf("%s exists; read it, then pass -force to replace it", path)
+ }
+ if !os.is_dir(dir) && os.make_directory_all(dir) != nil {
+ return "", fmt.aprintf("cannot make %s", dir)
+ }
+ if os.write_entire_file(
+ path,
+ transmute([]byte)commit_msg_hook(),
+ os.Permissions_Read_All + {.Write_User} + os.Permissions_Execute_All,
+ ) !=
+ nil {
+ return "", fmt.aprintf("cannot write %s", path)
+ }
+ return fmt.aprintf("wrote %s\n\n%s", path, agent_stanza()), ""
+}
+
+// self is the absolute path of the running binary, or its bare name where
+// that cannot be known.
+self :: proc(allocator := context.allocator) -> string {
+ exe, err := os.get_executable_path(allocator)
+ if err != nil {
+ return "review"
+ }
+ return exe
+}
+
+// commit_msg_hook is the hook git runs with the proposed message's file:
+// the staged change is reviewed with that message, and a must-fix finding
+// refuses the commit.
+commit_msg_hook :: proc(allocator := context.allocator) -> string {
+ return fmt.aprintf(
+ "#!/bin/sh\n# Installed by review. The staged change is reviewed with the message being\n# committed; a must-fix finding that stands refuses the commit. Dismiss a\n# finding where it is wrong, in the source: //review:ignore <rule> <why>\nexec %q --message-file \"$1\" --exit-code\n",
+ self(context.temp_allocator),
+ allocator = allocator,
+ )
+}
+
+// text is everything hook print shows: the git hook and the harness
+// stanzas.
+text :: proc(allocator := context.allocator) -> string {
+ return strings.concatenate(
+ {
+ "# .git/hooks/commit-msg — or run: review hook install\n",
+ commit_msg_hook(context.temp_allocator),
+ "\n",
+ agent_stanza(context.temp_allocator),
+ },
+ allocator,
+ )
+}
+
+// agent_stanza is the hook an agent's harness takes, for the harnesses
+// whose shape is known. Claude Code runs a PreToolUse hook before each
+// tool call; the one below reviews the staged change when the call is a
+// git commit, and a non-zero exit stops the call with the review's output
+// as the reason.
+agent_stanza :: proc(allocator := context.allocator) -> string {
+ return strings.concatenate(
+ {
+ "# Claude Code: .claude/settings.json (or ~/.claude/settings.json)\n",
+ "# Reviews the staged change before any \"git commit\" the agent runs.\n",
+ "{\n \"hooks\": {\n \"PreToolUse\": [{\n \"matcher\": \"Bash\",\n \"hooks\": [{\n \"type\": \"command\",\n",
+ " \"command\": \"if grep -q 'git commit' ; then ",
+ self(context.temp_allocator),
+ " --exit-code; fi\"\n",
+ " }]\n }]\n }\n}\n\n",
+ "# Any other agent: the commit-msg hook above gates every commit it makes\n",
+ "# through git, whatever harness it runs in. Put the output of\n# review agent\n",
+ "# in its instructions so it knows what the refusal means and how to answer it.\n",
+ },
+ allocator,
+ )
+}
+
+// agent_text is what an agent's instructions should say about this tool:
+// how to run it, how to read it, how to answer it, and when it may stop.
+agent_text ::
+ `## Code review
+
+Before committing, run ` +
+ "`review --json`" +
+ ` on the staged change and act on the report.
+
+- ` +
+ "`status`" +
+ ` is ` +
+ "`complete`" +
+ ` when every reader read everything. An empty findings list
+ under any other status is a hole, not a pass.
+- Every finding carries a stable ` +
+ "`id`" +
+ `, a ` +
+ "`rule`" +
+ `, a ` +
+ "`severity`" +
+ `, the ` +
+ "`file`" +
+ ` and
+ ` +
+ "`line`" +
+ `, the ` +
+ "`snippet`" +
+ ` at that line, and a ` +
+ "`fix`" +
+ `: the concrete change to make.
+ Read the rule with ` +
+ "`review rules <rule>`" +
+ ` when the finding is unclear.
+- Fix every ` +
+ "`must-fix`" +
+ `. Weigh each ` +
+ "`consider`" +
+ `. A ` +
+ "`note`" +
+ ` needs no action.
+- Where a finding is wrong, dismiss it in the source it concerns, on the line
+ above, with the reason: ` +
+ "`//review:ignore <rule> <why>`" +
+ `. Never dismiss to pass;
+ a dismissal added by the change under review is itself a must-fix finding.
+- Re-run with ` +
+ "`--baseline previous.json`" +
+ ` to see which ids resolved, persist or
+ are new. Stop when nothing must-fix persists and the status is complete.
+- Never delete a test to pass the review; deleting one is a must-fix finding.
+- The commit message is measured too: ` +
+ "`package: explainer`" +
+ `, the explainer in
+ the imperative, a body saying why for any change over fifty lines, under 150
+ words, naming only code that exists.
+`
diff --git a/odin/hook/hook_test.odin b/odin/hook/hook_test.odin
@@ -0,0 +1,55 @@
+package hook
+
+import "core:os"
+import "core:path/filepath"
+import "core:strings"
+import "core:testing"
+import "jfm:sh"
+
+@(test)
+install_writes_the_hook_once :: proc(t: ^testing.T) {
+ context.allocator = context.temp_allocator
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ root, err := os.make_directory_temp(temp, "review-hook-*", context.temp_allocator)
+ testing.expect(t, err == nil)
+ defer os.remove_all(root)
+ testing.expect(t, sh.exec({"git", "init", "-q"}, {dir = root}, context.temp_allocator).ok)
+ // The tool's own git reads the real configuration, and a machine
+ // with core.hooksPath set would send the install elsewhere.
+ os.set_env("GIT_CONFIG_GLOBAL", "/dev/null")
+ os.set_env("GIT_CONFIG_NOSYSTEM", "1")
+ defer os.unset_env("GIT_CONFIG_GLOBAL")
+ defer os.unset_env("GIT_CONFIG_NOSYSTEM")
+ before, _ := os.get_working_directory(context.temp_allocator)
+ testing.expect(t, os.set_working_directory(root) == nil)
+ defer os.set_working_directory(before)
+
+ out, install_err := run({"install"})
+ testing.expect_value(t, install_err, "")
+ testing.expect(t, strings.contains(out, "PreToolUse"), "the harness stanza is printed")
+ path := filepath.join({root, ".git", "hooks", "commit-msg"}, context.temp_allocator) or_else ""
+ data, read_err := os.read_entire_file_from_path(path, context.temp_allocator)
+ testing.expect(t, read_err == nil)
+ testing.expect(t, strings.has_prefix(string(data), "#!/bin/sh"))
+ testing.expect(t, strings.contains(string(data), `--message-file "$1" --exit-code`))
+ // A hook already there is someone's, and is not replaced unasked.
+ _, again := run({"install"})
+ testing.expect(t, strings.contains(again, "-force"))
+ _, forced := run({"-force", "install"})
+ testing.expect_value(t, forced, "")
+ _, unknown := run({"dance"})
+ testing.expect(t, strings.contains(unknown, "hook takes install or print"))
+}
+
+@(test)
+print_and_agent_text_explain_the_gate :: proc(t: ^testing.T) {
+ out, err := run({})
+ defer delete(out)
+ testing.expect_value(t, err, "")
+ for want in ([]string{"commit-msg", "--exit-code", "PreToolUse", "review agent"}) {
+ testing.expectf(t, strings.contains(out, want), "%q missing from hook print", want)
+ }
+ for want in ([]string{"review --json", "must-fix", "review:ignore", "--baseline"}) {
+ testing.expectf(t, strings.contains(agent_text, want), "%q missing from agent text", want)
+ }
+}
diff --git a/odin/report/report.odin b/odin/report/report.odin
@@ -10,6 +10,8 @@ package report
import "core:encoding/json"
import "core:fmt"
+import "core:os"
+import "core:slice"
import "core:strings"
import "../change"
@@ -70,6 +72,59 @@ Contract :: struct {
usage: Metered `json:"usage"`,
}
+// compare reads the findings of a previous report and names this run's
+// against them, by id. Only the report's standing findings count: a
+// finding it retracted or dismissed was not one to fix.
+compare :: proc(
+ path: string,
+ now: []finding.Finding,
+ allocator := context.allocator,
+) -> (
+ b: Baseline,
+ err: string,
+) {
+ data, read_err := os.read_entire_file_from_path(path, context.temp_allocator)
+ if read_err != nil {
+ return b, fmt.aprintf("reading the baseline: %s", path, allocator = allocator)
+ }
+ Previous :: struct {
+ findings: []struct {
+ id: string `json:"id"`,
+ } `json:"findings"`,
+ }
+ previous: Previous
+ if json.unmarshal(data, &previous, allocator = context.temp_allocator) != nil {
+ return b, "the baseline is not a review report"
+ }
+ before := make(map[string]bool, context.temp_allocator)
+ for f in previous.findings {
+ if f.id != "" {
+ before[f.id] = true
+ }
+ }
+ b.from = strings.clone(path, allocator)
+ resolved := make([dynamic]string, allocator)
+ persisting := make([dynamic]string, allocator)
+ fresh := make([dynamic]string, allocator)
+ seen := make(map[string]bool, context.temp_allocator)
+ for f in now {
+ seen[f.id] = true
+ if before[f.id] {
+ append(&persisting, strings.clone(f.id, allocator))
+ } else {
+ append(&fresh, strings.clone(f.id, allocator))
+ }
+ }
+ for id in before {
+ if !seen[id] {
+ append(&resolved, strings.clone(id, allocator))
+ }
+ }
+ slice.sort(resolved[:])
+ b.resolved, b.persisting, b.new = resolved[:], persisting[:], fresh[:]
+ return b, ""
+}
+
// status_of is how complete the measurement was: a job that failed, a
// file no reader covered, or a diff cut short each leave a hole.
status_of :: proc(c: change.Change, failures: int) -> string {
diff --git a/odin/report/report_test.odin b/odin/report/report_test.odin
@@ -1,6 +1,7 @@
package report
import "core:encoding/json"
+import "core:fmt"
import "core:os"
import "core:path/filepath"
import "core:strings"
@@ -163,6 +164,38 @@ filter_drops_what_the_source_dismisses :: proc(t: ^testing.T) {
}
@(test)
+compare_names_this_run_against_the_last :: proc(t: ^testing.T) {
+ context.allocator = context.temp_allocator
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ dir, err := os.make_directory_temp(temp, "review-baseline-*", context.temp_allocator)
+ testing.expect(t, err == nil)
+ defer os.remove_all(dir)
+ path := filepath.join({dir, "previous.json"}, context.temp_allocator) or_else ""
+ testing.expect(
+ t,
+ os.write_entire_file(
+ path,
+ transmute([]byte)string(`{"findings":[{"id":"aaa"},{"id":"bbb"},{"id":""}]}`),
+ ) ==
+ nil,
+ )
+ b, cmp_err := compare(path, {{id = "bbb"}, {id = "ccc"}})
+ testing.expect_value(t, cmp_err, "")
+ testing.expect_value(t, b.from, path)
+ testing.expect_value(t, fmt.tprint(b.resolved), `["aaa"]`)
+ testing.expect_value(t, fmt.tprint(b.persisting), `["bbb"]`)
+ testing.expect_value(t, fmt.tprint(b.new), `["ccc"]`)
+ _, missing := compare(
+ filepath.join({dir, "nowhere.json"}, context.temp_allocator) or_else "",
+ {},
+ )
+ testing.expect(t, strings.has_prefix(missing, "reading the baseline"))
+ testing.expect(t, os.write_entire_file(path, transmute([]byte)string("prose")) == nil)
+ _, prose := compare(path, {})
+ testing.expect_value(t, prose, "the baseline is not a review report")
+}
+
+@(test)
status_says_where_the_hole_is :: proc(t: ^testing.T) {
c: change.Change
c.uncovered = make([dynamic]change.Gap, context.temp_allocator)
diff --git a/odin/review/main.odin b/odin/review/main.odin
@@ -12,11 +12,13 @@ import "core:os"
import "core:strings"
import "../analyser"
+import "../bench"
import "../cache"
import "../change"
import "../check"
import "../finding"
import "../git"
+import "../hook"
import "../job"
import "../provider"
import "../report"
@@ -27,6 +29,11 @@ usage :: `review reads a change the way several narrow readers would.
usage: review [flags] [rev] the staged change, or a revision range
review rules [<rule>] the deterministic checks, or one of them
+ review rules -dismissed the dismissals in the tree, per rule
+ review hook [install] the commit-msg hook, printed or installed
+ review agent what an agent's instructions should say
+ review bench [-n N] [-author A] [-rule R] [root]
+ the checks' fire rate over recent commits
--json Report findings as JSON, for an agent rather than a person.
--verbose Show what each job read and what it cost.
@@ -34,20 +41,38 @@ usage: review [flags] [rev] the staged change, or a revision range
--no-verify Skip the second reading that checks what each job reported.
--fresh Ask the provider even where the answer cache holds this exact question.
--exit-code Exit 1 when a must-fix finding stands, so a hook can refuse the change.
+ --message-file f Read the commit message from this file, as a commit-msg hook is given it.
+ --baseline f A previous --json report; each finding is then new, persisting or resolved against it.
--jobs a,b Run only these jobs, comma separated.
--provider p Who answers: chain (probe the default order), claude, pi, api, or command.
--model m Which model, in whatever form the provider names them.
`
Flags :: struct {
- as_json, verbose, show, no_verify, fresh, exit_code: bool,
- only, which, model, rev: string,
+ as_json, verbose, show, no_verify, fresh, exit_code: bool,
+ only, which, model, rev, message_file, baseline_path: string,
}
main :: proc() {
- if len(os.args) > 1 && os.args[1] == "rules" {
- rules(os.args[2:])
- return
+ if len(os.args) > 1 {
+ switch os.args[1] {
+ case "rules":
+ rules(os.args[2:])
+ return
+ case "hook":
+ out, err := hook.run(os.args[2:])
+ fmt.print(out)
+ if err != "" {
+ fmt.eprintfln("review: %s", err)
+ os.exit(1)
+ }
+ return
+ case "agent":
+ fmt.print(hook.agent_text)
+ return
+ case "bench":
+ os.exit(run_bench(os.args[2:]))
+ }
}
flags, ok := parse(os.args[1:])
if !ok {
@@ -101,6 +126,10 @@ parse :: proc(args: []string) -> (flags: Flags, ok: bool) {
flags.which = take(args, &i, value)
case "--model", "-model":
flags.model = take(args, &i, value)
+ case "--message-file", "-message-file":
+ flags.message_file = take(args, &i, value)
+ case "--baseline", "-baseline":
+ flags.baseline_path = take(args, &i, value)
case "-h", "--help", "-help":
fmt.print(usage)
return flags, false
@@ -135,6 +164,14 @@ run :: proc(flags: Flags) -> int {
fmt.eprintfln("review: git could not describe %q", flags.rev)
return 1
}
+ if flags.message_file != "" {
+ message, read := change.read_message(flags.message_file)
+ if !read {
+ fmt.eprintfln("review: reading the message: %s", flags.message_file)
+ return 1
+ }
+ c.message = message
+ }
if strings.trim_space(c.diff) == "" {
if flags.as_json {
out, _ := report.encode(report.Contract{status = "empty"})
@@ -235,8 +272,18 @@ run :: proc(flags: Flags) -> int {
f.snippet = tree.line(t, f.file, f.line)
}
}
+ against: Maybe(report.Baseline)
+ if flags.baseline_path != "" {
+ b, cmp_err := report.compare(flags.baseline_path, kept)
+ if cmp_err != "" {
+ fmt.eprintfln("review: %s", cmp_err)
+ return 1
+ }
+ against = b
+ }
env := report.Contract {
status = report.status_of(c, len(result.failures)),
+ baseline = against,
provider = r.name,
findings = kept,
retracted = result.retracted[:],
@@ -307,6 +354,17 @@ faults :: proc(failures: []string) -> []report.Job_Fault {
// agent given a finding can read what it was judged against without
// leaving the terminal.
rules :: proc(args: []string) {
+ if len(args) > 0 && (args[0] == "-dismissed" || args[0] == "--dismissed") {
+ cwd, _ := os.get_working_directory(context.temp_allocator)
+ root, in_repo := git.toplevel(cwd)
+ if !in_repo {
+ fmt.eprintln("review: not in a git repository")
+ os.exit(2)
+ }
+ t, _ := tree.at(root, "")
+ fmt.print(check.dismissals(check.scope_of(nil, t)))
+ return
+ }
if len(args) == 0 {
fmt.print(check.catalogue())
for j in job.all() {
@@ -357,3 +415,62 @@ criterion :: proc(id: string) -> (name, text: string, found: bool) {
}
return "", "", false
}
+
+// run_bench measures the checks over recent commits and prints the fire
+// rate per rule. It returns the exit code.
+run_bench :: proc(args: []string) -> int {
+ opts := bench.Options {
+ n = 200,
+ }
+ root := ""
+ i := 0
+ for i < len(args) {
+ take := proc(args: []string, i: ^int) -> string {
+ if i^ + 1 < len(args) {
+ i^ += 1
+ return args[i^]
+ }
+ return ""
+ }
+ switch args[i] {
+ case "-n", "--n":
+ n := 0
+ for c in take(args, &i) {
+ if c >= '0' && c <= '9' {
+ n = n * 10 + int(c - '0')
+ }
+ }
+ if n > 0 {
+ opts.n = n
+ }
+ case "-author", "--author":
+ opts.author = take(args, &i)
+ case "-rule", "--rule":
+ opts.rule = take(args, &i)
+ case:
+ root = args[i]
+ }
+ i += 1
+ }
+ if root == "" {
+ cwd, _ := os.get_working_directory(context.temp_allocator)
+ found: bool
+ root, found = git.toplevel(cwd)
+ if !found {
+ fmt.eprintln("review: not in a git repository")
+ return 2
+ }
+ }
+ out, err := bench.run(
+ root,
+ opts,
+ proc(done, total: int) {fmt.eprintf("\r %d of %d", done, total)},
+ )
+ fmt.eprint("\r \r")
+ if err != "" {
+ fmt.eprintfln("review: %s", err)
+ return 1
+ }
+ fmt.print(out)
+ return 0
+}
