commit 034e4d8dbf495159a775083b4411e2376dfb46c5
parent 027eec57b4a25b7a8a2d4399ddb898094d93b609
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Wed, 23 Sep 2026 19:50:49 -0300
odin: report findings through the Go tool's contract
finding is one thing a reading noticed, sorted serious first, named by
the same sha256 the Go tool names it by so both readings agree on an id,
and dismissed by the same //review:ignore comment beside the line.
report is the JSON contract key for key — status, findings, retracted,
failed, skipped, uncovered, dismissed, usage — and the rendering a
person reads. The driver reports through it, and says which files no
reader covered, with the reason each.
Diffstat:
8 files changed, 1063 insertions(+), 30 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 frontend git tree change; do
+ for p in frontend git tree change finding report; do
{{odin}} test odin/$p {{odin_flags}} -out:build/${p}_test
done
diff --git a/odin/change/change.odin b/odin/change/change.odin
@@ -7,6 +7,8 @@ what new work is judged against.
*/
package change
+import "core:fmt"
+import "core:path/filepath"
import "core:strconv"
import "core:strings"
@@ -53,6 +55,12 @@ Declared :: struct {
body: string,
}
+// Gap is a file the deterministic side could not read, and why.
+Gap :: struct {
+ file: string `json:"file"`,
+ reason: string `json:"reason"`,
+}
+
// Diff_Line is one added line and the number it lands on.
Diff_Line :: struct {
line: int,
@@ -77,8 +85,8 @@ Change :: struct {
tests: [dynamic]Function,
comments: [dynamic]Located,
imports: map[string][]string,
- // uncovered are the code files no sidecar reads.
- uncovered: [dynamic]string,
+ // uncovered are the code files no sidecar read, with a reason each.
+ uncovered: [dynamic]Gap,
}
max_diff :: 60000
@@ -202,6 +210,16 @@ side :: proc(path, prefix: string) -> string {
read :: proc(c: ^Change, t: tree.Tree, allocator := context.allocator) -> bool {
context.allocator = allocator
ok := true
+ for name in c.files {
+ // 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 _, covered := frontend.sidecar_for(name);
+ !covered && is_code_file(name) && tree.exists(t, name) {
+ append(&c.uncovered, Gap{name, "no reader"})
+ }
+ }
for sidecar in frontend.Sidecar {
names := make([dynamic]string, context.temp_allocator)
for name in c.files {
@@ -215,14 +233,21 @@ read :: proc(c: ^Change, t: tree.Tree, allocator := context.allocator) -> bool {
}
answers, err := scan(sidecar, t, names[:], context.temp_allocator)
if err != .None {
- append(&c.uncovered, ..names[:])
+ why := fmt.aprintf("%s: %s", frontend.binary(sidecar), scan_error(err))
+ for name in names {
+ append(&c.uncovered, Gap{name, why})
+ }
ok = false
continue
}
for name in names {
file, answered := answers[name]
- if !answered || file.error != "" {
- append(&c.uncovered, name)
+ if !answered {
+ append(&c.uncovered, Gap{name, "no answer from the sidecar"})
+ continue
+ }
+ if file.error != "" {
+ append(&c.uncovered, Gap{name, strings.clone(file.error)})
continue
}
read_file(c, t, name, file)
@@ -373,6 +398,44 @@ scan :: proc(
return answers, .None
}
+// scan_error says what a sidecar failure was.
+scan_error :: proc(err: frontend.Scan_Error) -> string {
+ switch err {
+ case .None:
+ return ""
+ case .Not_Installed:
+ return "not on the path"
+ case .Failed:
+ return "gave no answer"
+ case .Unreadable:
+ return "gave an answer that is not its JSON"
+ }
+ return ""
+}
+
+// is_code_file reports a file a reader could plausibly exist for: not
+// prose, data, configuration or the files a repository keeps for git.
+is_code_file :: proc(path: string) -> bool {
+ base := filepath.base(path)
+ switch base {
+ case ".gitignore",
+ ".gitattributes",
+ ".gitmodules",
+ ".editorconfig",
+ "Makefile",
+ "Dockerfile",
+ "LICENSE",
+ "CODEOWNERS":
+ return false
+ }
+ for ext in ([]string{".md", ".mdx", ".txt", ".rst", ".adoc", ".json", ".yaml", ".yml", ".toml", ".lock", ".sum", ".mod", ".html", ".htm", ".css", ".svg", ".xml"}) {
+ if strings.has_suffix(base, ext) {
+ return false
+ }
+ }
+ return true
+}
+
// is_test_file reports a file the test runner reads, in Go's habit.
is_test_file :: proc(name: string) -> bool {
return strings.has_suffix(name, "_test.go")
diff --git a/odin/change/change_test.odin b/odin/change/change_test.odin
@@ -140,6 +140,16 @@ gather_reads_a_range_through_the_sidecar :: proc(t: ^testing.T) {
}
@(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"))
+ testing.expect(t, !is_code_file("readme.md"))
+ testing.expect(t, !is_code_file("go.sum"))
+ testing.expect(t, !is_code_file("x/.gitignore"))
+ testing.expect(t, !is_code_file("Dockerfile"))
+}
+
+@(test)
gather_of_nothing_staged_is_empty :: proc(t: ^testing.T) {
root, made := new_repo(t)
testing.expect(t, made, "the fixture repository")
diff --git a/odin/finding/finding.odin b/odin/finding/finding.odin
@@ -0,0 +1,263 @@
+/*
+Package finding is one thing a reading noticed: what it concerns, how
+seriously to take it, and the stable name a loop refers to it by. It also
+reads the dismissal a source file carries beside what it justifies.
+*/
+package finding
+
+import "core:crypto/sha2"
+import "core:encoding/hex"
+import "core:fmt"
+import "core:os"
+import "core:path/filepath"
+import "core:slice"
+import "core:strconv"
+import "core:strings"
+
+// Severity is what a finding means for the change, most serious first.
+// Only Must_Fix is worth blocking on; the rest are reported and left to
+// judgement, because a loop that treats taste as an error never finishes.
+Severity :: enum {
+ Must_Fix,
+ Consider,
+ Note,
+}
+
+severity_name :: proc(s: Severity) -> string {
+ switch s {
+ case .Must_Fix:
+ return "must-fix"
+ case .Consider:
+ return "consider"
+ case .Note:
+ return "note"
+ }
+ return "unknown"
+}
+
+// parse_severity 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.
+parse_severity :: proc(s: string) -> Severity {
+ switch strings.to_lower(strings.trim_space(s), context.temp_allocator) {
+ case "must-fix", "must fix", "mustfix", "error":
+ return .Must_Fix
+ case "consider", "warning":
+ return .Consider
+ }
+ return .Note
+}
+
+// Finding is one thing a job noticed. The JSON names are the contract's,
+// shared with the Go tool, so a loop reads either report the same way.
+Finding :: struct {
+ // job is the job that reported it; rule 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.
+ job: string `json:"job"`,
+ rule: string `json:"rule"`,
+ severity: Severity `json:"-"`,
+ // severity_name is how the severity travels in and out of JSON.
+ severity_name: string `json:"severity"`,
+ // file and line locate it, where it has a location; symbol names what
+ // it concerns, for the findings that are about a name rather than a
+ // place.
+ file: string `json:"file,omitempty"`,
+ line: int `json:"line,omitempty"`,
+ symbol: string `json:"symbol,omitempty"`,
+ 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; snippet is the
+ // line the finding points at, as it stands.
+ fix: string `json:"fix,omitempty"`,
+ snippet: string `json:"snippet,omitempty"`,
+ // verified is what stands behind the finding: the deterministic checks
+ // verify themselves; a model's finding is verified when a second
+ // reading of the same evidence let it stand.
+ verified: bool `json:"verified"`,
+ // id is a stable short name: a hash of what the finding is about, not
+ // where it sits, so that a re-run names the same finding again.
+ id: string `json:"id,omitempty"`,
+ // part is which part of a split subject the finding came from.
+ part: int `json:"-"`,
+}
+
+// to_string is the finding as a person reads it.
+to_string :: proc(f: Finding, allocator := context.allocator) -> string {
+ b := strings.builder_make(allocator)
+ strings.write_string(&b, severity_name(f.severity))
+ strings.write_string(&b, ": ")
+ if f.file != "" {
+ strings.write_string(&b, f.file)
+ if f.line > 0 {
+ fmt.sbprintf(&b, ":%d", f.line)
+ }
+ strings.write_string(&b, ": ")
+ } else if f.symbol != "" {
+ strings.write_string(&b, f.symbol)
+ strings.write_string(&b, ": ")
+ }
+ strings.write_string(&b, f.message)
+ if f.fix != "" {
+ strings.write_string(&b, "\n → ")
+ strings.write_string(&b, f.fix)
+ }
+ strings.write_string(&b, "\n [")
+ strings.write_string(&b, f.job)
+ strings.write_string(&b, "/")
+ strings.write_string(&b, f.rule)
+ strings.write_string(&b, "]")
+ return strings.to_string(b)
+}
+
+// sort orders findings so the ones worth reading first are first.
+sort :: proc(findings: []Finding) {
+ slice.stable_sort_by_cmp(findings, proc(a, b: Finding) -> slice.Ordering {
+ if a.severity != b.severity {
+ return .Less if a.severity < b.severity else .Greater
+ }
+ if a.file != b.file {
+ return .Less if a.file < b.file else .Greater
+ }
+ if a.line != b.line {
+ return .Less if a.line < b.line else .Greater
+ }
+ return .Equal
+ })
+}
+
+// must_fix reports whether any finding is one worth blocking on.
+must_fix :: proc(findings: []Finding) -> bool {
+ for f in findings {
+ if f.severity == .Must_Fix {
+ return true
+ }
+ }
+ return false
+}
+
+// identify gives a finding its short id: a hash of what it is about, not
+// where it sits or how it was worded. The line is left out because lines
+// move under edits that do not touch the finding; a model's message is
+// left out because a fresh reading may word it differently. A finding
+// with no symbol is told from its neighbours by its line, and a
+// deterministic check's message is part of what it is about and stable,
+// so it stays in. The hash is the Go tool's, byte for byte, so both
+// readings name a finding the same.
+identify :: proc(f: ^Finding, allocator := context.allocator) {
+ parts := make([dynamic]string, context.temp_allocator)
+ append(&parts, f.job, f.rule, f.file, f.symbol)
+ switch {
+ case f.job == "static":
+ append(&parts, f.message)
+ case f.symbol == "":
+ append(&parts, fmt.tprintf("%d", f.line))
+ }
+ joined := strings.join(parts[:], "\x00", context.temp_allocator)
+ ctx: sha2.Context_256
+ sha2.init_256(&ctx)
+ sha2.update(&ctx, transmute([]byte)joined)
+ digest: [32]byte
+ sha2.final(&ctx, digest[:])
+ encoded := hex.encode(digest[:], context.temp_allocator)
+ f.id = strings.clone(string(encoded[:12]), allocator)
+}
+
+// name fills in what a finding carries only in the report: its severity
+// as text, and the id that names it across runs. Two findings that hash
+// the same — one rule, one file, two comments — are told apart by a
+// counter, in the order they are listed.
+name :: proc(findings: []Finding, allocator := context.allocator) {
+ seen := make(map[string]int, context.temp_allocator)
+ for &f in findings {
+ f.severity_name = severity_name(f.severity)
+ identify(&f, allocator)
+ seen[f.id] += 1
+ if n := seen[f.id]; n > 1 {
+ f.id = fmt.aprintf("%s-%d", f.id, n, allocator = allocator)
+ }
+ }
+}
+
+// suppressed reports whether the source dismisses a finding, and with what
+// reason. The dismissal lives in the source beside what it justifies:
+//
+// //review:ignore <rule> <why>
+//
+// One on or just above the line it concerns covers that line; one anywhere
+// in a file covers the findings that name no line.
+suppressed :: proc(
+ f: Finding,
+ root: string,
+ allocator := context.allocator,
+) -> (
+ why: string,
+ ok: bool,
+) {
+ if f.file == "" {
+ return "", false
+ }
+ path := f.file
+ if root != "" {
+ path = filepath.join({root, f.file}, context.temp_allocator) or_else f.file
+ }
+ source, err := os.read_entire_file_from_path(path, context.temp_allocator)
+ if err != nil {
+ return "", false
+ }
+ text_left := string(source)
+ line := 0
+ for text in strings.split_lines_iterator(&text_left) {
+ line += 1
+ rule, reason, found := dismissal(text)
+ if !found || !rules_match(rule, f.rule) {
+ continue
+ }
+ if reason == "" {
+ reason = "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 strings.clone(reason, allocator), true
+ }
+ }
+ return "", false
+}
+
+// dismissal reads the rule and reason out of a line holding a dismissal:
+// two slashes, review:ignore, the rule, and what follows.
+dismissal :: proc(text: string) -> (rule, why: string, found: bool) {
+ marker :: "review:ignore"
+ at := strings.index(text, marker)
+ if at < 0 {
+ return
+ }
+ before := strings.trim_right_space(text[:at])
+ if !strings.has_suffix(before, "//") {
+ return
+ }
+ rest := strings.trim_left_space(text[at + len(marker):])
+ if len(rest) == len(text[at + len(marker):]) {
+ return // The marker is not followed by whitespace.
+ }
+ end := strings.index_any(rest, " \t")
+ if end < 0 {
+ return rest, "", len(rest) > 0
+ }
+ return rest[:end], strings.trim_space(rest[end:]), true
+}
+
+// rules_match compares a dismissal against a rule, where "all" dismisses
+// anything the job found at that spot.
+rules_match :: proc(dismissed, rule: string) -> bool {
+ return dismissed == "all" || strings.equal_fold(dismissed, rule)
+}
+
+// atoi reads a line number a job reported, which may arrive as a string.
+atoi :: proc(s: string) -> int {
+ n, ok := strconv.parse_int(strings.trim_space(s))
+ return n if ok else 0
+}
diff --git a/odin/finding/finding_test.odin b/odin/finding/finding_test.odin
@@ -0,0 +1,211 @@
+package finding
+
+import "core:os"
+import "core:path/filepath"
+import "core:strings"
+import "core:testing"
+
+@(test)
+severity_round_trips :: proc(t: ^testing.T) {
+ testing.expect_value(t, parse_severity(" Must-Fix "), Severity.Must_Fix)
+ testing.expect_value(t, parse_severity("error"), Severity.Must_Fix)
+ testing.expect_value(t, parse_severity("warning"), Severity.Consider)
+ testing.expect_value(t, parse_severity("whatever"), Severity.Note)
+ testing.expect_value(t, severity_name(.Consider), "consider")
+}
+
+@(test)
+to_string_says_where_and_what :: proc(t: ^testing.T) {
+ s := to_string(
+ Finding {
+ job = "tests",
+ rule = "cannot-fail",
+ severity = .Must_Fix,
+ file = "a_test.go",
+ line = 4,
+ message = "m",
+ fix = "do x",
+ },
+ context.temp_allocator,
+ )
+ testing.expect_value(
+ t,
+ s,
+ "must-fix: a_test.go:4: m\n → do x\n [tests/cannot-fail]",
+ )
+ s = to_string(
+ Finding {
+ job = "namer",
+ rule = "abbreviation",
+ severity = .Note,
+ symbol = "cfg",
+ message = "m",
+ },
+ context.temp_allocator,
+ )
+ testing.expect_value(t, s, "note: cfg: m\n [namer/abbreviation]")
+}
+
+@(test)
+sort_puts_the_serious_first :: proc(t: ^testing.T) {
+ findings := []Finding {
+ {severity = .Note, file = "b.go", line = 1},
+ {severity = .Must_Fix, file = "b.go", line = 9},
+ {severity = .Must_Fix, file = "a.go", line = 2},
+ {severity = .Must_Fix, file = "a.go", line = 1},
+ }
+ sort(findings)
+ testing.expect_value(t, findings[0].file, "a.go")
+ testing.expect_value(t, findings[0].line, 1)
+ testing.expect_value(t, findings[1].line, 2)
+ testing.expect_value(t, findings[2].line, 9)
+ testing.expect_value(t, findings[3].severity, Severity.Note)
+ testing.expect(t, must_fix(findings))
+ testing.expect(t, !must_fix(findings[3:]))
+}
+
+@(test)
+id_is_stable_and_matches_the_go_tool :: proc(t: ^testing.T) {
+ f := Finding {
+ job = "tests",
+ rule = "cannot-fail",
+ file = "x_test.go",
+ line = 4,
+ symbol = "TestX",
+ message = "m",
+ }
+ identify(&f, context.temp_allocator)
+ // sha256("tests\x00cannot-fail\x00x_test.go\x00TestX")[:12], as Go names it.
+ testing.expect_value(t, f.id, "52ef4321ce06")
+ moved := f
+ moved.line = 9
+ identify(&moved, context.temp_allocator)
+ testing.expect_value(t, moved.id, f.id)
+ reworded := f
+ reworded.message = "said another way"
+ identify(&reworded, context.temp_allocator)
+ testing.expect_value(t, reworded.id, f.id)
+ other := f
+ other.symbol = "TestY"
+ identify(&other, context.temp_allocator)
+ testing.expect(t, other.id != f.id, "two findings share one id")
+
+ a := Finding {
+ job = "claims",
+ rule = "unsupported-claim",
+ file = "x.go",
+ line = 4,
+ }
+ b := Finding {
+ job = "claims",
+ rule = "unsupported-claim",
+ file = "x.go",
+ line = 9,
+ }
+ identify(&a, context.temp_allocator)
+ identify(&b, context.temp_allocator)
+ testing.expect(t, a.id != b.id, "two comments share one id")
+
+ s1 := Finding {
+ job = "static",
+ rule = "history-coupled-file",
+ file = "a.go",
+ message = "ties a.go to b.go",
+ }
+ s2 := Finding {
+ job = "static",
+ rule = "history-coupled-file",
+ file = "a.go",
+ message = "ties a.go to c.go",
+ }
+ identify(&s1, context.temp_allocator)
+ identify(&s2, context.temp_allocator)
+ testing.expect(t, s1.id != s2.id, "two partners share one id")
+}
+
+@(test)
+name_tells_twins_apart :: proc(t: ^testing.T) {
+ findings := []Finding {
+ {job = "namer", rule = "abbreviation", file = "x.go", symbol = "cfg", severity = .Note},
+ {job = "namer", rule = "abbreviation", file = "x.go", symbol = "cfg", severity = .Note},
+ }
+ name(findings, context.temp_allocator)
+ testing.expect(t, findings[0].id != findings[1].id)
+ testing.expect(t, strings.has_suffix(findings[1].id, "-2"))
+ testing.expect_value(t, findings[0].severity_name, "note")
+}
+
+write_source :: proc(t: ^testing.T, name, src: string) -> (dir: string) {
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ scratch, err := os.make_directory_temp(temp, "review-finding-*", context.temp_allocator)
+ if err != nil {
+ testing.fail_now(t, "no scratch directory")
+ }
+ path := filepath.join({scratch, name}, context.temp_allocator) or_else name
+ testing.expect(t, os.write_entire_file(path, transmute([]byte)src) == nil)
+ return scratch
+}
+
+@(test)
+suppressed_reads_the_dismissal_beside_the_line :: proc(t: ^testing.T) {
+ dir := write_source(
+ t,
+ "x.go",
+ "package x\n\n// nothing here\nconst a = 1\n\n//review:" +
+ "ignore restates-a-fact the ico package owns the other one\nconst b = 6\n\nconst c = 7\n\nconst d = 8\n\nconst e = 9\n",
+ )
+ defer os.remove_all(dir)
+ Case :: struct {
+ name: string,
+ f: Finding,
+ want: bool,
+ why: string,
+ }
+ for c in ([]Case{{"just above", {file = "x.go", line = 7, rule = "restates-a-fact"}, true, "the ico package owns the other one"}, {"on the line", {file = "x.go", line = 6, rule = "restates-a-fact"}, true, "the ico package owns the other one"}, {"distant", {file = "x.go", line = 12, rule = "restates-a-fact"}, false, ""}, {"another rule", {file = "x.go", line = 7, rule = "already-named"}, false, ""}, {"no line", {file = "x.go", rule = "restates-a-fact"}, true, "the ico package owns the other one"}, {"no file", {rule = "restates-a-fact"}, false, ""}, {"missing file", {file = "nowhere.go", line = 1, rule = "restates-a-fact"}, false, ""}}) {
+ why, ok := suppressed(c.f, dir, context.temp_allocator)
+ testing.expectf(t, ok == c.want, "%s: got %v, want %v", c.name, ok, c.want)
+ if ok {
+ testing.expectf(t, why == c.why, "%s: reason %q", c.name, why)
+ }
+ }
+}
+
+@(test)
+suppressed_reads_all_and_no_reason :: proc(t: ^testing.T) {
+ dir := write_source(t, "x.go", "//review:" + "ignore all generated\nconst a = 1\n")
+ defer os.remove_all(dir)
+ why, ok := suppressed(
+ Finding{file = "x.go", line = 2, rule = "anything-at-all"},
+ dir,
+ context.temp_allocator,
+ )
+ testing.expect(t, ok)
+ testing.expect_value(t, why, "generated")
+ other := write_source(
+ t,
+ "y.go",
+ "// review:" + "ignore cannot-fail\nfunc TestX(t *testing.T) {}\n",
+ )
+ defer os.remove_all(other)
+ why, ok = suppressed(
+ Finding{file = "y.go", line = 2, rule = "cannot-fail"},
+ other,
+ context.temp_allocator,
+ )
+ testing.expect(t, ok, "a space after the slashes still dismisses")
+ testing.expect_value(t, why, "no reason given")
+}
+
+@(test)
+dismissal_needs_the_slashes :: proc(t: ^testing.T) {
+ rule, why, found := dismissal("x := 1 //review:" + "ignore no-shadow it is the loop's")
+ testing.expect(t, found)
+ testing.expect_value(t, rule, "no-shadow")
+ testing.expect_value(t, why, "it is the loop's")
+ _, _, found = dismissal("review:ignore no-shadow prose")
+ testing.expect(t, !found)
+ _, _, found = dismissal("//review:" + "ignore")
+ testing.expect(t, !found)
+ testing.expect_value(t, atoi(" 42 "), 42)
+ testing.expect_value(t, atoi("x"), 0)
+}
diff --git a/odin/report/report.odin b/odin/report/report.odin
@@ -0,0 +1,272 @@
+/*
+Package report is the whole measurement 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. The JSON is the Go tool's, key for key.
+*/
+package report
+
+import "core:encoding/json"
+import "core:fmt"
+import "core:strings"
+
+import "../change"
+import "../finding"
+
+version :: 1
+
+// Job_Fault is one job that could not finish, and what failed about it.
+Job_Fault :: 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.
+Metered :: struct {
+ tokens_in: int `json:"in"`,
+ tokens_out: int `json:"out"`,
+ cached: int `json:"cached"`,
+ replayed: int `json:"replayed"`,
+ cost: f64 `json:"usd"`,
+}
+
+// Retracted is a finding the second reading did not let stand, and why.
+Retracted :: struct {
+ finding: finding.Finding `json:"finding"`,
+ reason: string `json:"reason"`,
+}
+
+// Dismissed is a finding the source itself answered, and the answer.
+Dismissed :: struct {
+ finding: finding.Finding `json:"finding"`,
+ why: string `json:"why"`,
+}
+
+// Baseline is this run's findings named against a previous report's, by
+// id. A loop reads resolved to know its fixes took, persisting to know
+// what is left, and new to know what the fixes cost.
+Baseline :: struct {
+ from: string `json:"from"`,
+ resolved: []string `json:"resolved"`,
+ persisting: []string `json:"persisting"`,
+ new: []string `json:"new"`,
+}
+
+Contract :: struct {
+ version: int `json:"version"`,
+ status: string `json:"status"`,
+ provider: string `json:"provider,omitempty"`,
+ findings: []finding.Finding `json:"findings"`,
+ retracted: []Retracted `json:"retracted"`,
+ failed: []Job_Fault `json:"failed"`,
+ skipped: []string `json:"skipped"`,
+ uncovered: []change.Gap `json:"uncovered"`,
+ dismissed: []Dismissed `json:"dismissed"`,
+ truncated: bool `json:"truncated,omitempty"`,
+ baseline: Maybe(Baseline) `json:"baseline,omitempty"`,
+ usage: Metered `json:"usage"`,
+}
+
+// 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 {
+ if failures > 0 || len(c.uncovered) > 0 || c.truncated {
+ return "incomplete"
+ }
+ return "complete"
+}
+
+// filter drops the findings the source itself dismisses, and keeps them
+// so that a silent dismissal can still be read back.
+filter :: proc(
+ root: string,
+ findings: []finding.Finding,
+ allocator := context.allocator,
+) -> (
+ kept: []finding.Finding,
+ dismissed: []Dismissed,
+) {
+ keep := make([dynamic]finding.Finding, allocator)
+ drop := make([dynamic]Dismissed, allocator)
+ for f in findings {
+ if why, ok := finding.suppressed(f, root, allocator); ok {
+ append(&drop, Dismissed{f, why})
+ continue
+ }
+ append(&keep, f)
+ }
+ return keep[:], drop[:]
+}
+
+// render is the report as a person reads it, grouped by severity, the
+// serious first. A dismissal reaches a few lines either side of itself,
+// so which findings one answered is worth being able to read when asked;
+// a retraction is the second reading's word against the first's.
+render :: proc(env: Contract, verbose := false, allocator := context.allocator) -> string {
+ b := strings.builder_make(allocator)
+ if verbose {
+ for d in env.dismissed {
+ fmt.sbprintf(
+ &b,
+ " dismissed: %s:%d %s (%s)\n",
+ d.finding.file,
+ d.finding.line,
+ d.finding.rule,
+ d.why,
+ )
+ }
+ for r in env.retracted {
+ fmt.sbprintf(
+ &b,
+ " retracted: [%s] %s:%d — %s\n",
+ r.finding.rule,
+ r.finding.file,
+ r.finding.line,
+ first(r.reason, 120),
+ )
+ }
+ }
+ if len(env.findings) == 0 {
+ strings.write_string(&b, "no findings")
+ if len(env.dismissed) > 0 {
+ fmt.sbprintf(&b, " (%d dismissed in the source)", len(env.dismissed))
+ }
+ strings.write_string(&b, "\n")
+ notes(&b, env)
+ return strings.to_string(b)
+ }
+ announced := false
+ severity: finding.Severity
+ for f in env.findings {
+ if !announced || f.severity != severity {
+ severity = f.severity
+ announced = true
+ fmt.sbprintf(
+ &b,
+ "\n%s\n",
+ strings.to_upper(finding.severity_name(severity), context.temp_allocator),
+ )
+ }
+ line := finding.to_string(f, context.temp_allocator)
+ indented, _ := strings.replace_all(line, "\n ", "\n ", context.temp_allocator)
+ fmt.sbprintf(&b, " %s\n", indented)
+ }
+ fmt.sbprintf(&b, "\n%d findings", len(env.findings))
+ if len(env.dismissed) > 0 {
+ fmt.sbprintf(&b, ", %d dismissed in the source", len(env.dismissed))
+ }
+ strings.write_string(&b, "\n")
+ notes(&b, env)
+ if verbose {
+ strings.write_string(
+ &b,
+ "\nDismiss a finding where it is wrong, in the source it concerns:\n //review:ignore <rule> <why>\n",
+ )
+ }
+ return strings.to_string(b)
+}
+
+// notes says what a reader would otherwise miss: findings that did not
+// survive verification, and how the run stands against a baseline.
+notes :: proc(b: ^strings.Builder, env: Contract) {
+ if len(env.retracted) > 0 {
+ fmt.sbprintf(
+ b,
+ "%d of the findings reported did not survive verification\n",
+ len(env.retracted),
+ )
+ }
+ if base, given := env.baseline.?; given {
+ fmt.sbprintf(
+ b,
+ "against %s: %d resolved, %d persisting, %d new\n",
+ base.from,
+ len(base.resolved),
+ len(base.persisting),
+ len(base.new),
+ )
+ if len(base.persisting) > 0 {
+ fmt.sbprintf(
+ b,
+ " persisting: %s\n",
+ strings.join(base.persisting, ", ", context.temp_allocator),
+ )
+ }
+ }
+}
+
+// encode is the contract as JSON. Every list is said even when empty: a
+// key an agent cannot find is a hole it guesses about. 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.
+encode :: proc(env: Contract, allocator := context.allocator) -> (out: string, ok: bool) {
+ named := env
+ named.version = version
+ named.findings = present(env.findings, context.temp_allocator)
+ named.retracted = present(env.retracted, context.temp_allocator)
+ named.failed = present(env.failed, context.temp_allocator)
+ named.skipped = present(env.skipped, context.temp_allocator)
+ named.uncovered = present(env.uncovered, context.temp_allocator)
+ named.dismissed = present(env.dismissed, context.temp_allocator)
+ finding.name(named.findings, context.temp_allocator)
+ retracted := make([]finding.Finding, len(named.retracted), context.temp_allocator)
+ for r, i in named.retracted {
+ retracted[i] = r.finding
+ }
+ finding.name(retracted, context.temp_allocator)
+ for &r, i in named.retracted {
+ r.finding = retracted[i]
+ }
+ dismissed := make([]finding.Finding, len(named.dismissed), context.temp_allocator)
+ for d, i in named.dismissed {
+ dismissed[i] = d.finding
+ }
+ finding.name(dismissed, context.temp_allocator)
+ for &d, i in named.dismissed {
+ d.finding = dismissed[i]
+ }
+ data, err := json.marshal(
+ named,
+ {pretty = true, use_spaces = true, spaces = 2},
+ context.temp_allocator,
+ )
+ if err != nil {
+ return "", false
+ }
+ return compact(string(data), allocator), true
+}
+
+// compact writes an empty list as [] on one line, where the marshaller
+// leaves a blank line inside it.
+compact :: proc(text: string, allocator := context.allocator) -> string {
+ out := text
+ for depth in 0 ..< 8 {
+ indent := strings.repeat(" ", depth, context.temp_allocator)
+ pattern := strings.concatenate({"[\n\n", indent, "]"}, context.temp_allocator)
+ out, _ = strings.replace_all(out, pattern, "[]", context.temp_allocator)
+ }
+ return strings.clone(out, allocator)
+}
+
+// present is a slice that is never nil, so that it is written as [] rather
+// than left out or written as null.
+present :: proc(items: []$T, allocator := context.allocator) -> []T {
+ if items == nil {
+ return make([]T, 0, allocator)
+ }
+ copied := make([]T, len(items), allocator)
+ copy(copied, items)
+ return copied
+}
+
+// first is the start of a string, with an ellipsis where it was cut.
+first :: proc(s: string, n: int, allocator := context.allocator) -> string {
+ trimmed := strings.trim_space(s)
+ if len(trimmed) > n {
+ return strings.concatenate({trimmed[:n], "…"}, allocator)
+ }
+ return trimmed
+}
diff --git a/odin/report/report_test.odin b/odin/report/report_test.odin
@@ -0,0 +1,172 @@
+package report
+
+import "core:encoding/json"
+import "core:os"
+import "core:path/filepath"
+import "core:strings"
+import "core:testing"
+
+import "../change"
+import "../finding"
+
+@(test)
+render_groups_by_severity :: proc(t: ^testing.T) {
+ out := render(
+ Contract {
+ findings = {
+ {
+ job = "tests",
+ rule = "cannot-fail",
+ severity = .Must_Fix,
+ file = "a_test.go",
+ line = 4,
+ message = "one",
+ },
+ {
+ job = "namer",
+ rule = "abbreviation",
+ severity = .Note,
+ symbol = "cfg",
+ message = "two",
+ },
+ },
+ dismissed = {{}, {}},
+ },
+ allocator = context.temp_allocator,
+ )
+ for want in ([]string{"MUST-FIX", "NOTE", "one", "two", "2 findings", "2 dismissed in the source"}) {
+ testing.expectf(t, strings.contains(out, want), "%q missing from:\n%s", want, out)
+ }
+ testing.expect_value(t, strings.count(out, "MUST-FIX"), 1)
+}
+
+@(test)
+render_nothing :: proc(t: ^testing.T) {
+ testing.expect_value(
+ t,
+ strings.trim_space(render(Contract{}, allocator = context.temp_allocator)),
+ "no findings",
+ )
+ out := render(
+ Contract{dismissed = {{}, {}, {}}, retracted = {{}}},
+ allocator = context.temp_allocator,
+ )
+ testing.expect(t, strings.contains(out, "3 dismissed in the source"))
+ testing.expect(
+ t,
+ strings.contains(out, "1 of the findings reported did not survive verification"),
+ )
+ out = render(
+ Contract{baseline = Baseline{from = "old.json", persisting = {"abc"}}},
+ allocator = context.temp_allocator,
+ )
+ testing.expect(t, strings.contains(out, "against old.json: 0 resolved, 1 persisting, 0 new"))
+ testing.expect(t, strings.contains(out, "persisting: abc"))
+}
+
+@(test)
+encode_carries_the_contract :: proc(t: ^testing.T) {
+ out, ok := encode(
+ Contract {
+ status = "incomplete",
+ provider = "api/probe",
+ findings = {
+ {
+ job = "static",
+ rule = "test-deleted",
+ severity = .Must_Fix,
+ file = "x_test.go",
+ line = 3,
+ message = "m",
+ verified = true,
+ },
+ },
+ retracted = {
+ {
+ finding = {
+ job = "tests",
+ rule = "cannot-fail",
+ severity = .Must_Fix,
+ file = "x_test.go",
+ message = "m",
+ },
+ reason = "the test can fail",
+ },
+ },
+ failed = {{job = "duplication", error = "context canceled"}},
+ skipped = {"hygiene"},
+ uncovered = {{file = "a.odin", reason = "no tests parser"}},
+ dismissed = {{finding = {rule = "cannot-fail"}, why = "wrong"}},
+ truncated = true,
+ usage = {tokens_in = 10, tokens_out = 2, cached = 1, replayed = 3, cost = 0.5},
+ },
+ context.temp_allocator,
+ )
+ testing.expect(t, ok, "encode")
+ for want in ([]string{`"version": 1`, `"status": "incomplete"`, `"provider": "api/probe"`, `"severity": "must-fix"`, `"verified": true`, `"id": "`, `"reason": "the test can fail"`, `"error": "context canceled"`, `"skipped": [`, `"a.odin"`, `"why": "wrong"`, `"truncated": true`, `"usd": 0.5`}) {
+ testing.expectf(t, strings.contains(out, want), "%s missing from:\n%s", want, out)
+ }
+ testing.expect(t, !strings.contains(out, `"baseline"`), "an absent baseline is left out")
+ testing.expect(t, !strings.contains(out, `"part"`), "part is the reading's own")
+
+ // Every list is present even when empty, and the JSON parses.
+ empty, empty_ok := encode(Contract{status = "complete"}, context.temp_allocator)
+ testing.expect(t, empty_ok)
+ for key in ([]string{"findings", "retracted", "failed", "skipped", "uncovered", "dismissed"}) {
+ testing.expectf(t, strings.contains(empty, key), "%s missing from:\n%s", key, empty)
+ }
+ parsed, parse_err := json.parse_string(empty, allocator = context.temp_allocator)
+ testing.expect(t, parse_err == nil, "the JSON parses")
+ root := parsed.(json.Object)
+ testing.expect_value(t, len(root["findings"].(json.Array)), 0)
+ testing.expect(t, !strings.contains(empty, "null"), "no list is null")
+ testing.expect(t, strings.contains(empty, `"findings": []`), "an empty list is one line")
+}
+
+@(test)
+filter_drops_what_the_source_dismisses :: proc(t: ^testing.T) {
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ root, err := os.make_directory_temp(temp, "review-report-*", context.temp_allocator)
+ testing.expect(t, err == nil)
+ defer os.remove_all(root)
+ path := filepath.join({root, "x.go"}, context.temp_allocator) or_else ""
+ testing.expect(
+ t,
+ os.write_entire_file(
+ path,
+ transmute([]byte)string(
+ "package x\n\n//review:" +
+ "ignore restates-a-fact the ico package owns it\nconst b = 6\n",
+ ),
+ ) ==
+ nil,
+ )
+
+ kept, dismissed := filter(
+ root,
+ {
+ {file = "x.go", line = 4, rule = "restates-a-fact", message = "dismissed"},
+ {file = "x.go", line = 4, rule = "already-named", message = "kept"},
+ },
+ context.temp_allocator,
+ )
+ testing.expect_value(t, len(dismissed), 1)
+ testing.expect_value(t, dismissed[0].why, "the ico package owns it")
+ testing.expect_value(t, len(kept), 1)
+ testing.expect_value(t, kept[0].message, "kept")
+}
+
+@(test)
+status_says_where_the_hole_is :: proc(t: ^testing.T) {
+ c: change.Change
+ c.uncovered = make([dynamic]change.Gap, context.temp_allocator)
+ testing.expect_value(t, status_of(c, 0), "complete")
+ testing.expect_value(t, status_of(c, 1), "incomplete")
+ c.truncated = true
+ testing.expect_value(t, status_of(c, 0), "incomplete")
+ c.truncated = false
+ append(&c.uncovered, change.Gap{"a.zig", "no reader"})
+ testing.expect_value(t, status_of(c, 0), "incomplete")
+ f := finding.Finding{}
+ testing.expect_value(t, f.severity, finding.Severity.Must_Fix)
+}
diff --git a/odin/review/main.odin b/odin/review/main.odin
@@ -1,20 +1,37 @@
// review, the Odin reading: what a change adds, read through the language
-// sidecars, and the repository's declarations it is judged against. The
-// change is the staged one, or the revision range given.
+// sidecars, judged, and reported. The change is the staged one, or the
+// revision range given.
//
-// review # the staged change
-// review HEAD^..HEAD
+// review [--json] [--verbose] [--show] [rev]
package main
import "core:fmt"
import "core:os"
import "../change"
+import "../finding"
import "../git"
+import "../report"
import "../tree"
main :: proc() {
- rev := os.args[1] if len(os.args) > 1 else ""
+ as_json, verbose, show: bool
+ rev := ""
+ for arg in os.args[1:] {
+ switch arg {
+ case "--json":
+ as_json = true
+ case "--verbose":
+ verbose = true
+ case "--show":
+ show = true
+ case "-h", "--help":
+ fmt.println("usage: review [--json] [--verbose] [--show] [rev]")
+ return
+ case:
+ rev = arg
+ }
+ }
cwd, cwd_err := os.get_working_directory(context.temp_allocator)
if cwd_err != nil {
fmt.eprintln("review: no working directory")
@@ -31,7 +48,12 @@ main :: proc() {
os.exit(1)
}
if len(c.files) == 0 {
- fmt.println("nothing to review")
+ if as_json {
+ out, _ := report.encode(report.Contract{status = "empty"})
+ fmt.println(out)
+ } else {
+ fmt.println("nothing to review")
+ }
return
}
t, at_ok := tree.at(root, rev)
@@ -41,8 +63,43 @@ main :: proc() {
}
defer tree.close(t)
- read_ok := change.read(&c, t)
- fmt.printfln("=== change %s: %d files, %d added lines ===", rev, len(c.files), added(c))
+ change.read(&c, t)
+ if show {
+ list(c, t)
+ }
+ findings: [dynamic]finding.Finding
+ finding.sort(findings[:])
+ kept, dismissed := report.filter(t.dir, findings[:])
+ env := report.Contract {
+ status = report.status_of(c, 0),
+ findings = kept,
+ dismissed = dismissed,
+ uncovered = c.uncovered[:],
+ truncated = c.truncated,
+ }
+ if as_json {
+ out, ok := report.encode(env)
+ if !ok {
+ fmt.eprintln("review: the report could not be encoded")
+ os.exit(1)
+ }
+ fmt.println(out)
+ return
+ }
+ fmt.print(report.render(env, verbose))
+ for gap in c.uncovered {
+ fmt.eprintfln("unread: %s: %s", gap.file, gap.reason)
+ }
+}
+
+// list prints what the sidecars read, for a look at the evidence before
+// any judgement of it.
+list :: proc(c: change.Change, t: tree.Tree) {
+ added := 0
+ for _, lines in c.added {
+ added += len(lines)
+ }
+ fmt.printfln("=== change: %d files, %d added lines ===", len(c.files), added)
for s in c.symbols {
mark := " exported" if s.exported else ""
fmt.printfln("symbol %s:%d %s %s%s", s.file, s.line, s.kind, s.name, mark)
@@ -53,21 +110,6 @@ main :: proc() {
for l in c.comments {
fmt.printfln("comment %s:%d %s", l.file, l.line, l.text)
}
- for name in c.uncovered {
- fmt.printfln("unread %s", name)
- }
- declared, indexed := change.index(t)
+ declared, _ := change.index(t)
fmt.printfln("=== index: %d declarations ===", len(declared))
- if !read_ok || !indexed {
- fmt.eprintln("review: a sidecar did not answer")
- os.exit(1)
- }
-}
-
-added :: proc(c: change.Change) -> int {
- n := 0
- for _, lines in c.added {
- n += len(lines)
- }
- return n
}
