Reasoning behind #414 — making the default review agents loop-aware

[alekb]

Up front: this is a suggestion, nothing more. If the direction doesn't fit how you want the defaults to behave, feel free to reject the PR — no hard feelings, and the per-file override mechanism means I can carry these in my own config anyway.

I opened #414 after going down a rabbit hole with the five default agent prompts. It started as "let me tune these for my repo" and turned into reading how expandAgentReferences composes them and what review_first.txt / review_second.txt actually expect back. The PR description lists the individual edits; this is the reasoning that ties them together, which felt too long for a PR body.

The core observation: in ralphex's loop, a weak finding is not free. Every finding gets verified by the parent at file:line, every confirmed one gets fixed, and any fix forces another full review iteration — REVIEW_DONE only fires on a zero-findings pass. So the cost of one speculative or out-of-scope finding isn't a bad line in a report, it's potentially a whole extra 5-agent iteration. That reframes what these prompts should optimize for: not "find as much as possible" but "find only what you can defend, inside the diff, and say clearly when there's nothing."

Measured against that, a few things stood out:

• The agents are told "report problems only - no positive observations" but never told what to output when there are no problems. An agent looking at a clean diff literally can't comply — and LLMs under that pressure tend to manufacture marginal findings rather than come back empty-handed, which is exactly what stalls convergence. The NO ISSUES FOUND convention already exists for external reviewers; the spawned agents just never adopted it.
• Nothing scopes findings to the branch. Items like "all user inputs validated" or "functions without test coverage" read as repo-wide audit mandates, and combined with review_first's "fix pre-existing issues anyway" rule, a mature repo can feed the loop indefinitely.
• Nothing says the reviewers are read-only. They're full-tooled general-purpose subagents running five in parallel while the parent owns fix-and-commit. documentation.txt actually instructs edits ("Mark completed items as done") — mid-review, concurrently with four siblings, and duplicating what task.txt already did before review started.
• implementation.txt verifies "the stated goal" without ever being given the goal. Variable expansion inside agent bodies already works — the defaults just don't use it, so the one agent whose whole job is goal-vs-code has to reverse-engineer the goal from commit messages.

The smaller edits (severity fields, de-overlapping quality vs simplification, the testing prompt's location field not being able to express its own primary finding type) all fall out of the same lens.

Besides the unit tests, I smoke-tested the branch end-to-end with scripts/internal/prep-toy-test.sh (haiku for task + review models, external review off): clean reviewers actually used the NO ISSUES FOUND sentinel, the second pass discarded a minor finding on severity grounds and emitted REVIEW_DONE, all fixes stayed parent-authored, and the loop converged and exited 0. A toy repo is not proof, but nothing broke.

One judgment call worth flagging separately: I put the shared contract lines (scope / read-only / sentinel) into reviewContextInstruction in Go rather than into each txt file, on the theory that user custom agents inherit the same gaps and the lead-in is the one place that covers them all. The counter-argument is real though — it makes the contract invisible to people reading the txt files and non-overridable without a rebuild. If you'd rather keep the Go side dumb and the contract in the txt files, I'm happy to rework the PR that way.

Also curious whether the churn scenarios above match what you've seen in real runs, or whether in practice the models behave better than a literal reading of the prompts suggests.

[umputun]

coming back to this late, sorry. Short version: keep it in Go, no rework needed.

I answered both questions on the PR thread before merging, so this is mostly me closing the loop here. Since then #428 pushed the same decision further in the same direction: the no-delegation line had to go into reviewContextInstruction for exactly the reason you gave, a spawned agent's whole prompt is that lead-in plus its body, so anything in review_first.txt / review_second.txt only ever reaches the root session. Go placement is right, non-overridability is the trade-off I want.

on the churn question, I went back and counted across my own progress logs in this repo: 46 review-loop terminations, 44 of them the clean zero-findings exit, 2 on the HEAD-unchanged guard, none hit max iterations. So the mechanism you describe is real but much rarer here than "can feed the loop indefinitely" suggests. Worth correcting one detail in the cost model too: the 5-agent pass runs once and never loops, the looped prompt is review_second.txt which spawns 2 agents, and the loop is capped at max(3, max_iterations/10) on top of the HEAD guard. So a confirmed finding costs an extra 2-agent round, not a 5-agent one.

the one thing you raised that I hadn't dealt with is discoverability. reviewContextInstruction was documented nowhere while codexReviewGuidance and codexTaskGuidance both have CLAUDE.md entries, so someone reading their own agents/*.txt had no way to see what gets prepended to it. Fixed that.