commit 88fad333d418c28b52df26d735d97edd4fce3d06
parent f01f78e10da35b1b9acf69574241b5ef3eaf3bd7
Author: Jack Mordaunt <jackmordaunt.dev@gmail.com>
Date: Thu, 24 Sep 2026 06:30:27 -0300
review: never ask a job an empty part, and try claude in the chain
A splittable job over the packet cap grouped files by whether they held
anything at all, so the duplication job was handed a test file — whose
duplication subject is empty — as a second part, and claude -p refused
the empty stdin. Parts are grouped by what the job itself reads from each
file now, an empty part is never asked, an empty prompt is refused before
any child starts or request goes out, a failure in a split job names its
part, and --show prints each part rather than the whole. The default
chain tries the coding assistant between the API and the gateway, so a
machine with no key and no gateway still gets a reading. Found by an
agent using the tool on real work, whose report reproduced it through a
fake provider.
Diffstat:
8 files changed, 76 insertions(+), 46 deletions(-)
diff --git a/job/job.odin b/job/job.odin
@@ -441,8 +441,10 @@ packet_cap :: 16000
// parts is the subjects a job is asked, as changes: the whole change when
// it fits or cannot be split, else the change cut file by file into runs
-// that each render under the cap. A file that alone renders over the cap
-// is a part by itself.
+// that each render under the cap. A file the job reads nothing from — a
+// test file, for the duplication job — is left out of every run, so no
+// part is ever asked with an empty subject. A file that alone renders
+// over the cap is a part by itself.
parts :: proc(j: Job, c: ^change.Change, allocator := context.allocator) -> []^change.Change {
out := make([dynamic]^change.Change, allocator)
if !j.splittable || len(j.subject(c, context.temp_allocator)) <= packet_cap {
@@ -451,13 +453,14 @@ parts :: proc(j: Job, c: ^change.Change, allocator := context.allocator) -> []^c
}
group := make([dynamic]string, context.temp_allocator)
for file in c.files {
- if !contributes(c, file) {
+ alone := make([]string, 1, context.temp_allocator)
+ alone[0] = file
+ if strings.trim_space(j.subject(part(c, alone, context.temp_allocator), context.temp_allocator)) == "" {
continue
}
if len(group) > 0 {
- trial := slice.clone(group[:], context.temp_allocator)
with := make([dynamic]string, context.temp_allocator)
- append(&with, ..trial)
+ append(&with, ..group[:])
append(&with, file)
piece := part(c, with[:], context.temp_allocator)
if len(j.subject(piece, context.temp_allocator)) > packet_cap {
@@ -473,26 +476,6 @@ parts :: proc(j: Job, c: ^change.Change, allocator := context.allocator) -> []^c
return out[:]
}
-// contributes is whether a file has anything a splittable job reads.
-contributes :: proc(c: ^change.Change, file: string) -> bool {
- for s in c.symbols {
- if s.file == file {
- return true
- }
- }
- for t in c.tests {
- if t.file == file {
- return true
- }
- }
- for comment in c.comments {
- if comment.file == file {
- return true
- }
- }
- return false
-}
-
// part is the change narrowed to some of its files: the declarations,
// tests and comments in them, with everything the jobs read beside those
// — candidates, twins, the index, the message — shared.
diff --git a/job/job_test.odin b/job/job_test.odin
@@ -195,5 +195,25 @@ parts_cut_a_large_subject_by_file :: proc(t: ^testing.T) {
change.Symbol{name = "S", kind = "func", file = "a.go", line = 1, signature = "short"},
)
testing.expect_value(t, len(parts(j, &small, context.temp_allocator)), 1)
- testing.expect(t, !contributes(&c, "c.md"))
+ // A file the job reads nothing from is in no part: the tests job
+ // reads a test file, the namer does not.
+ tests_only := change.Change{files = {"a.go", "a_test.go"}}
+ tests_only.symbols = make([dynamic]change.Symbol)
+ append(&tests_only.symbols, change.Symbol{name = "S", kind = "func", file = "a.go", line = 1, signature = strings.repeat("x", 17000)})
+ tests_only.tests = make([dynamic]change.Function)
+ append(&tests_only.tests, change.Function{name = "TestS", file = "a_test.go", line = 1, body = strings.repeat("y", 17000)})
+ namer_pieces := parts(all(context.temp_allocator)[2], &tests_only, context.temp_allocator)
+ testing.expect_value(t, len(namer_pieces), 1)
+ if len(namer_pieces) == 1 {
+ testing.expect_value(t, len(namer_pieces[0].files), 1)
+ testing.expect_value(t, namer_pieces[0].files[0], "a.go")
+ }
+ for piece in namer_pieces {
+ testing.expect(t, strings.trim_space(all(context.temp_allocator)[2].subject(piece, context.temp_allocator)) != "", "an empty part is never asked")
+ }
+ tests_pieces := parts(all(context.temp_allocator)[1], &tests_only, context.temp_allocator)
+ testing.expect_value(t, len(tests_pieces), 1)
+ if len(tests_pieces) == 1 {
+ testing.expect_value(t, tests_pieces[0].files[0], "a_test.go")
+ }
}
diff --git a/provider/chain.odin b/provider/chain.odin
@@ -17,23 +17,19 @@ Entry :: struct {
}
// default_chain is the configured preference. The direct API is tried
-// before the coding assistant, because it can enforce the answer's shape
-// and reports usage; the local gateway is last, as the reading of last
-// resort. REVIEW_MODEL re-points the first slot.
+// 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 :: proc(allocator := context.temp_allocator) -> []Entry {
model := os.get_env("REVIEW_MODEL", allocator)
if model == "" {
model = default_api_model
}
- entries := make([]Entry, 2, allocator)
- entries[0] = Entry {
- strings.concatenate({"api/", model}, allocator),
- Provider{kind = .Api, model = model},
- }
- entries[1] = Entry {
- "pi/maple/glm-5-3-flash",
- Provider{kind = .Pi, model = "glm-5-3-flash", upstream = "maple"},
- }
+ entries := make([]Entry, 3, allocator)
+ entries[0] = Entry{strings.concatenate({"api/", model}, allocator), Provider{kind = .Api, model = model}}
+ entries[1] = Entry{strings.concatenate({"claude/", default_claude_model}, allocator), Provider{kind = .Claude, model = default_claude_model}}
+ entries[2] = Entry{"pi/maple/glm-5-3-flash", Provider{kind = .Pi, model = "glm-5-3-flash", upstream = "maple"}}
return entries
}
diff --git a/provider/provider.odin b/provider/provider.odin
@@ -121,6 +121,9 @@ ask :: proc(
Answer,
string,
) {
+ if strings.trim_space(user) == "" {
+ return {}, "nothing to ask: the prompt is empty"
+ }
switch p.kind {
case .Claude:
return ask_claude(p, system, user, allocator)
@@ -144,6 +147,9 @@ ask_verdict :: proc(
Answer,
string,
) {
+ if strings.trim_space(user) == "" {
+ return {}, "nothing to ask: the prompt is empty"
+ }
if p.kind == .Api {
return ask_api(p, system, user, verdicts_tool, allocator)
}
diff --git a/provider/provider_test.odin b/provider/provider_test.odin
@@ -34,6 +34,15 @@ names_carry_the_model :: proc(t: ^testing.T) {
}
@(test)
+the_chain_tries_the_assistant_between_api_and_gateway :: proc(t: ^testing.T) {
+ chain := default_chain(context.temp_allocator)
+ testing.expect_value(t, len(chain), 3)
+ testing.expect_value(t, chain[0].provider.kind, Kind.Api)
+ testing.expect_value(t, chain[1].name, "claude/sonnet")
+ testing.expect_value(t, chain[2].provider.kind, Kind.Pi)
+}
+
+@(test)
pi_answers_from_its_stream :: proc(t: ^testing.T) {
out := `{"type":"message_start"}
{"type":"message_end","message":{"role":"assistant","content":[{"type":"text","text":"{\"findings\":[]}"}],"usage":{"input":10,"output":2,"cacheRead":1,"cost":{"total":0.5}},"stopReason":"stop"}}
@@ -72,6 +81,8 @@ command_answers_whole_or_by_field :: proc(t: ^testing.T) {
testing.expect_value(t, field("not json", "answer", context.temp_allocator), "not json")
_, err = ask(Provider{kind = .Command}, "s", "u", context.temp_allocator)
testing.expect(t, strings.contains(err, "names no command"))
+ _, err = ask(p, "s", " \n", context.temp_allocator)
+ testing.expect_value(t, err, "nothing to ask: the prompt is empty")
_, err = ask(Provider{kind = .Command, argv = {"false"}}, "s", "u", context.temp_allocator)
testing.expect(t, strings.contains(err, "exit 1"))
}
diff --git a/readme.md b/readme.md
@@ -384,7 +384,7 @@ Nothing here cares which model answers. A provider takes a prompt and returns te
| `--provider` | how | credentials |
|---|---|---|
-| `chain` (default) | probes the configured order — the console API first, then `pi` against the local gateway — and the first that answers serves the whole reading | whichever entry it lands on holds |
+| `chain` (default) | probes the configured order — the console API, then `claude` on the path, then `pi` against the local gateway — and the first that answers serves the whole reading | whichever entry it lands on holds |
| `claude` | the coding assistant on the path, `-p --output-format json` | whatever it already holds |
| `api` | the console API directly | `ant auth login` or `ANTHROPIC_API_KEY` |
| `pi` | `pi -p --mode json`, which speaks to several providers of its own | its own; name the upstream in `REVIEW_PI_PROVIDER` |
diff --git a/review/main.odin b/review/main.odin
@@ -234,13 +234,20 @@ run :: proc(flags: Flags) -> int {
if flags.show {
for j in jobs {
- subject := j.subject(&c, context.temp_allocator)
- fmt.printfln("=== %s ===", j.name)
- if strings.trim_space(subject) == "" {
- fmt.print("(nothing to read)\n\n")
- continue
+ pieces := job.parts(j, &c, context.temp_allocator)
+ for piece, i in pieces {
+ subject := j.subject(piece, context.temp_allocator)
+ if len(pieces) > 1 {
+ fmt.printfln("=== %s (part %d of %d) ===", j.name, i + 1, len(pieces))
+ } else {
+ fmt.printfln("=== %s ===", j.name)
+ }
+ if strings.trim_space(subject) == "" {
+ fmt.print("(nothing to read)\n\n")
+ continue
+ }
+ fmt.printfln("%s", subject)
}
- fmt.printfln("%s", subject)
}
fmt.println("=== static ===")
kept, _ := report.filter(t.dir, static[:])
diff --git a/reviewer/reviewer.odin b/reviewer/reviewer.odin
@@ -202,10 +202,17 @@ read :: proc(t: ^Task) {
fmt.printfln(" %-12s asked in %d parts", t.j.name, len(t.pieces))
}
for piece, i in t.pieces {
- found, answer, err := ask(t.r, t.j, t.j.subject(piece, context.temp_allocator))
+ user := t.j.subject(piece, context.temp_allocator)
+ if strings.trim_space(user) == "" {
+ continue // A part with nothing in it is not a question.
+ }
+ found, answer, err := ask(t.r, t.j, user)
for &f in found {
f.part = i
}
+ if err != "" && len(t.pieces) > 1 {
+ err = fmt.tprintf("part %d of %d: %s", i + 1, len(t.pieces), err)
+ }
record(t.result, t.lock, t.j.name, found, answer, err)
}
}
