Use review sparingly: make the pass count and cost visible, refuse a pass mid-work, and land at CONVERGED nf-e4xe ← Beads

closed priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/844 by tools/beads/import_github.py's reconcile pass.

@aedanpope asked for this after watching #830 take ten review passes — *"argh, it's looping again m8… I'm worried we're looping"* — and stopping it by hand with rev:off.

The reviewer was not at fault: across those ten passes it raised fifteen findings, none false and none repeated, and every one was mutation-checkable. The loop was the coder's. This issue is the coder-side correction: two prose edits, and the tool changes that make the prose unnecessary.

The measurement

#830 (dev.py pr), 2026-08-11. Fifteen findings, by cause:

| cause | findings | n | |---|---|---| | Real defects in the original code | F1, F2, F3, F9, F11 | 5 | | Blast radius of one CLAUDE.md restructure | F4, F5, F6, F12 | 4 | | Created by the coder's own fix for the previous finding | F10, F13, F14, F15 | 4 | | A claim written into code and comments that was false | F7 | 1 | | Landed by another PR merging underneath | F8 | 1 |

Cost, from total_cost_usd in the final result record of each ~/.ninefold-reviewer/logs/pr-<n>.*.log: $5.45 (52 turns) and $4.86 (62 turns) for two sampled passes; ten passes is roughly $50. Consistent with #678's measured $3.58–6.59 per full pass.

Two distinct wastes, and they want different fixes:

  1. Two passes reviewed a sha the branch had already moved past. review_now.py reviews the head *at spawn time* and takes ~12 minutes; work pushed in that window buys a verdict about code that no longer exists, plus a B2 section full of staleness the coder caused. Fix: a refusal at spawn.
  2. Four findings were self-inflicted — F11's fix caused F13, whose fix caused F14, whose fix caused F15. Each was a real miss, each fix was correct, and the method that found them would have kept finding them. The underlying defect was one shape (verb_pr reasoning about three branches through two fields and a boolean — now filed on #836). Fix: a rule that says *stop*, and a visible pass count that makes a coder ask at pass 4 rather than pass 10.

⚠️ Neither is the reviewer spiralling. Its charter already has a severity floor and treats CONVERGED as a successful terminal state. What is missing is the coder's half of that contract.

Part 1 — CLAUDE.md step 5

Six lines today; the proposal replaces them with five, so it lands exactly at the 5-added-line budget and needs no override.

Current:

``` 5. You drive your PR to reviewed, approved and merged. Pull the review when you are ready./tools/pr/review_now.py <n> — once per batch, not once per push; a comment from Aedan is reviewed at once, your own pushes wait. merge_pr.py --why <n> then says what is left, perimeter reasons included. Fold the verdict's cheap nits in and merge. → notes/merging.md, notes/reviewer.md ```

Proposed:

``` 5. You drive your PR to reviewed, approved and merged. Pull the review once you are done touching the branch (a pass is ~12 min and ~$5; what you push meanwhile is reviewed at a sha that no longer exists) — review_now.py <n>. Aedan's comments are pulled at once, your own pushes wait. **CONVERGED is done: fold the cheap nits in, file the rest unclaimed, merge.** → notes/reviewer.md ```

What it evicts, stated rather than hidden — this is the trade CLAUDE_MD_BUDGET's refusal exists to force:

What it deliberately does not try to fit: the stop rule. It is the highest-value of the three but needs two sentences to be actionable, and CLAUDE.md's own standing instruction is to put that in notes/ and link it.

Part 2 — a new section in notes/reviewer.md

⚠️ notes/reviewer.md is in perimeter.GOVERNANCE, so this needs @aedanpope's approval whoever writes it.

``` ## Using review sparingly

A pass is one synchronous xhigh run: ~12 minutes and ~$4–6 (measured; total_cost_usd in ~/.ninefold-reviewer/logs/pr-<n>.*.log). #830 took ten passes and ~$50, and the last four findings were each caused by the fix for the one before it. Three rules, in the order they would have saved the money:

1. CONVERGED is a successful terminal state, not a lesser PASS. The charter says so and it means it: "this is as good as bot review gets it — over to the human." Chasing PASS through residuals is the failure mode the severity floor exists to stop, and the floor binds the reviewer, not you. Your side of it: at CONVERGED, land it. 2. Pull the review when you are done touching the branch — not merely when a batch is pushed. review_now.py reviews the sha it spawned against, so anything you push in the next ~12 minutes buys a verdict about code that no longer exists, and a B2 section full of staleness you caused yourself. Commit locally and hold the push if something occurs to you mid-pass. 3. If finding N+1 was created by your fix for finding N, stop. That is a loop and it is yours, not the reviewer's. File the remainder on the PR's follow-up issue, unclaimed, and merge. The same applies the *second* time a class repeats: fix the mechanism — a type, or a test that reads the source of truth — or file it. Fixing the third instance and waiting for the fourth is what ten passes look like.

⚠️ A standing approval plus a green presubmit is enough. rev:off on the PR, or UNCONVERGED_OK=1 ./tools/pr/merge_pr.py <n>, are the documented ways to land it — they are not last resorts, they are the exit from this loop. ```

Part 3 — the tool changes, which are the real answer

Design 006 S8: an incident's correction lands as a refusal; prose is the fallback. The prose above would not have stopped this loop — every individual round looked justified at the time, and I would have read all of it and still done ten. Ranked by build cost against loop stopped:

(a) Print the running cost before spawning — recommended first

~10 lines, no new state, nothing blocked. The data is already on disk. Before review_now.py spawns:

``` #830: pass 7 — 6 passes so far on this PR, $28.40. Last verdict: CONVERGED at f8c0e0b. ```

Why this first: the spend is currently invisible at the moment of spending. A coder at pass 4 has no signal distinguishing that from pass 1. This is the cheapest item here and probably the highest-value, because the number is what prompts the question the other rules answer.

Implementation note: parse the last line of each ~/.ninefold-reviewer/logs/pr-<n>.*.log as JSON and sum total_cost_usd; the count is the number of matching logs. ⚠️ Dispatcher-driven passes may log elsewhere — #830's rounds 3+ produced no new pr-830.* log under ~/.ninefold-reviewer/logs/, so the count would have under-read. Worth resolving as part of this, or the number teaches the wrong thing.

(b) Refuse a pass when the branch is dirty or has unpushed commits — recommended with (a)

> review_now.py: you are still working — a pass reviews the sha you PUSH, not the one you are on. Commit and push, or --force.

A git status --porcelain and a rev-list @{u}..HEAD. Directly kills waste #1 above, which was two of #830's ten passes.

(c) Refuse at pass ≥ N without --force

Naming the cost so far and the CONVERGED-is-terminal rule. This is the actual gate for the loop — but N is arbitrary and it is @aedanpope's number to set, not a session's (the same reasoning as design 006 §5.4 objection 3 on CLAUDE_MD_BUDGET). Hold it until (a) has produced a few PRs' worth of real pass counts, so the threshold is set against a distribution rather than one anecdote.

(d) merge_pr.py --why reports CONVERGED as an answer, not a lesser state

Today it prints result=GAPS — you have not converged with it yet. It should equally say *"the reviewer CONVERGED at <sha>; you have pushed N commits since"* — putting the terminal state at the surface a coder consults immediately before merging, which is exactly where the decision to stop gets made.

Scope, and what this is not

Unclaimed. Filed by nf-dev-dev-py-pr, which has wrapped; not taking it.

Dependencies

None.

Comments

No comments.

Add a comment