commit 5db1d92aac6d0003fa071a39282d17c2d92d8d3f
parent 88fad333d418c28b52df26d735d97edd4fce3d06
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Thu, 24 Sep 2026 06:42:25 -0300
review: resolve collections against the repository, and answer the first dogfood run
The odin-check analyser read ols.json from the tree at the end of the
change but resolved a relative collection path against that tree, which
for a range is a scratch copy with no neighbours, so every range review
of this repository skipped odin-check. The path is resolved against the
repository now. The first run of the tool on its own last three commits
asked for two comments to claim less about other programs and for the
rules subcommand to be named for what it does; all three are answered.
Diffstat:
4 files changed, 48 insertions(+), 20 deletions(-)
diff --git a/analyser/analyser_test.odin b/analyser/analyser_test.odin
@@ -192,6 +192,31 @@ a_run_past_the_timeout_is_not_waited_for :: proc(t: ^testing.T) {
}
@(test)
+collections_resolve_against_the_repository :: proc(t: ^testing.T) {
+ temp := os.temp_directory(context.temp_allocator) or_else ""
+ scratch, err := os.make_directory_temp(temp, "review-ols-*", context.temp_allocator)
+ testing.expect(t, err == nil)
+ defer os.remove_all(scratch)
+ testing.expect(
+ t,
+ os.write_entire_file(
+ join(scratch, "ols.json"),
+ transmute([]byte)string(
+ `{"collections":[{"name":"jfm","path":"../odin"},{"name":"abs","path":"/opt/abs"},{"name":""}]}`,
+ ),
+ ) ==
+ nil,
+ )
+ got := collections(scratch, "/repo", context.temp_allocator)
+ testing.expect_value(
+ t,
+ fmt.tprint(got),
+ `["-collection:jfm=/odin", "-collection:abs=/opt/abs"]`,
+ )
+ testing.expect_value(t, len(collections("/nowhere", "/repo", context.temp_allocator)), 0)
+}
+
+@(test)
nearest_walks_up :: proc(t: ^testing.T) {
temp := os.temp_directory(context.temp_allocator) or_else ""
root, err := os.make_directory_temp(temp, "review-analyser-*", context.temp_allocator)
diff --git a/analyser/odin.odin b/analyser/odin.odin
@@ -23,7 +23,7 @@ odin_check := Analyser {
for dir in sorted(dirs) {
args := make([dynamic]string, context.temp_allocator)
append(&args, "check", dir, "-vet", "-strict-style", "-json-errors", "-no-entry-point")
- append(&args, ..collections(tree_dir))
+ append(&args, ..collections(tree_dir, root))
stdout, stderr, err := execute_both(tree_dir, "odin", args[:], context.temp_allocator)
out := strings.concatenate({stdout, stderr}, context.temp_allocator)
if err != "" && !strings.contains(out, "error_count") {
@@ -35,10 +35,11 @@ odin_check := Analyser {
},
}
-// collections are the -collection flags the repository's ols.json
-// declares, which is where an Odin project names the collections its
-// imports resolve through; a relative path is relative to the repository.
-collections :: proc(tree_dir: string, allocator := context.temp_allocator) -> []string {
+// collections are the -collection flags the ols.json at the end of the
+// change declares, which is where an Odin project names the collections
+// its imports resolve through; a relative path is relative to the
+// repository the change is in.
+collections :: proc(tree_dir, root: string, allocator := context.temp_allocator) -> []string {
Config :: struct {
collections: []struct {
name: string `json:"name"`,
@@ -58,9 +59,11 @@ collections :: proc(tree_dir: string, allocator := context.temp_allocator) -> []
if c.name == "" || c.path == "" {
continue
}
+ // A relative path is relative to the repository, not to the
+ // scratch copy a range is read from, which has no neighbours.
path := c.path
if !strings.has_prefix(path, "/") {
- path = join(tree_dir, path, allocator)
+ path = join(root, path, allocator)
}
append(&flags, strings.concatenate({"-collection:", c.name, "=", path}, allocator))
}
diff --git a/provider/chain.odin b/provider/chain.odin
@@ -16,11 +16,11 @@ Entry :: struct {
provider: Provider,
}
-// default_chain is the configured preference. The direct API is tried
-// first, because it can enforce the answer's shape and reports usage;
-// then the coding assistant on the path, which holds its own credentials
-// and so answers on a machine with no key set; the local gateway is last,
-// as the reading of last resort. REVIEW_MODEL re-points the first slot.
+// default_chain is the configured preference: the direct API, then the
+// coding assistant on the path, then the local gateway as the reading of
+// last resort. The order is a choice about where an answer is likeliest
+// to be had with the least fuss, not a claim about any of them; a probe
+// decides. REVIEW_MODEL re-points the first slot.
default_chain :: proc(allocator := context.temp_allocator) -> []Entry {
model := os.get_env("REVIEW_MODEL", allocator)
if model == "" {
diff --git a/review/main.odin b/review/main.odin
@@ -61,12 +61,12 @@ Flags :: struct {
}
main :: proc() {
- // A debug build runs under the collection's debug allocator when
- // REVIEW_DEBUG_ALLOC is set: it names the site of every overflow,
- // double free, write after free and size mismatch, and lists the leaks
- // at exit — which a one-shot program has plenty of, so the report is
- // asked for rather than printed on every run — and under trace, which
- // symbolises an assertion failure. The release build is untouched.
+ // A debug build sets trace's assertion handler, and with
+ // REVIEW_DEBUG_ALLOC set runs under the collection's debug allocator
+ // and prints its report at exit; what the allocator detects is its own
+ // header's to say. The report is asked for rather than printed on every
+ // run, since a one-shot program leaves everything to exit. The release
+ // build is untouched.
when ODIN_DEBUG {
context.assertion_failure_proc = trace.assertion_failure_proc
if os.get_env("REVIEW_DEBUG_ALLOC", context.temp_allocator) != "" {
@@ -88,7 +88,7 @@ dispatch :: proc() -> int {
if len(os.args) > 1 {
switch os.args[1] {
case "rules":
- return rules(os.args[2:])
+ return print_rules(os.args[2:])
case "hook":
out, err := hook.run(os.args[2:])
fmt.print(out)
@@ -386,10 +386,10 @@ faults :: proc(failures: []string) -> []report.Job_Fault {
return out
}
-// rules prints the catalogue, or one rule's description, so that an
+// print_rules prints the catalogue, or one rule's description, so that an
// agent given a finding can read what it was judged against without
// leaving the terminal.
-rules :: proc(args: []string) -> int {
+print_rules :: proc(args: []string) -> int {
if len(args) > 0 && (args[0] == "-dismissed" || args[0] == "--dismissed") {
cwd, _ := os.get_working_directory(context.temp_allocator)
root, in_repo := git.toplevel(cwd)
