docs: add convergence loops to multi-agent-plan and self-review-relay - #1168
docs: add convergence loops to multi-agent-plan and self-review-relay#1168bjagg wants to merge 3 commits into
Conversation
The skill ran a fixed four passes, v1 through v4, and stopped. Four is arbitrary: v4 may still have gaps and v2 may already be clean, and nothing checked either way. The plan was declared "implementation-ready" by the same agent that wrote it. Adds a convergence tail. A fresh reviewer checks the plan against all three lens checklists plus scope and returns structured findings; if any are blockers, a targeted revision closes exactly those and the reviewer runs again. At most two rounds. The pipeline itself is unchanged, on purpose. The value of v1 to v4 is lens diversity, not repetition -- looping the same lens adds nothing. The loop belongs on the tail, where the question stops being "what else should this cover?" and becomes "is it done?", which needs an independent answer. A loop that exits when a judge reports zero issues pushes in exactly one direction: toward the judge reporting nothing. Four guards separate a real convergence from a manufactured one, and the skill documents why each exists: - the reviewer is never the reviser, and is told it did not write the plan - the cap is a reported outcome (cap-reached means blockers survived), not a silent stop that would state the opposite of the truth - a round must produce a delta; blockers standing plus an unchanged plan is a stall, and re-running only re-litigates the same disagreement - a first-round zero on a 250-400 line plan is flagged for a human as suspicious rather than banked as success Findings reuse the severity vocabulary from the self-review-relay skill, with `section` in place of `file`/`line`, so the two are comparable. Verified by extracting the script and exercising the loop with stubbed agents across all five paths (clean, converged-after-revision, cap-reached, stalled, first-round-zero). That caught two real bugs before commit: the loop variable shadowed the existing `plan` prompt-builder, and finalVersion counted rounds rather than accepted revisions, so a stalled run would have written a -v5.md file containing v4's content.
The convergence tail exited on the FIRST review that found no blockers. That
declares a plan done on one opinion, and spec-forge already had the rule that
says why it's not enough (orchestration/process.md, "Loop control", June 2026):
the open-gap set is empty for two consecutive audit passes
(loop-until-dry -- a single clean pass isn't enough)
Ported. A clean pass now increments a streak and the same unchanged plan goes
back to a fresh reviewer; convergence needs two in a row. That is requiring two
reviewers to agree, not shopping until one says yes -- the plan is never revised
between the two passes, so the second reviewer is a second sample, not a retry.
The dry run shows why it matters: in the clean -> blocker -> clean -> clean case
the old code exited after the first clean pass and shipped a plan the very next
reviewer found a blocker in.
Replaces the old first-round-zero guard with a stronger one. A single zero-finding
round is no longer special-cased, because two clean passes already cover it; what
is now flagged is a run where NO reviewer found anything at any severity, which
measures agreement between two weak reviewers rather than plan quality. That
reports as clean-but-unverified.
Caps move from 2 rounds to MAX_REVIEWS = 4, since two clean passes plus a
revision cycle no longer fits in two. cap-reached now also covers the subtle case
where a lone clean pass was followed by a reviewer that found more.
Verified by re-running the stubbed dry run over six paths: two-clean, streak-reset,
one-clean-then-cap, blockers-throughout, stalled, and all-zero.
The relay was one shot. It told you what one round of reviewers saw in one version of the diff, then stopped -- and the only nod to iteration was an informal "offer to fix the blockers, then re-run the relevant lens", with no cap, no fresh-reviewer rule, and no definition of done. Adds an explicit convergence loop, driven by the main loop since the relay reviews and never edits. Exit requires BOTH: zero blockers on two consecutive relay passes, and ruff + ty + pytest green. Capped at 3 relay runs; hitting the cap means not ready, and the skill says to report it that way. The two-condition exit is the point, and it is deliberately NOT a copy of multi-agent-plan. A plan has no oracle, so a judge loop is all there is. A diff has one, and the two catch different things: the gate catches a failing test or a type error the judge cannot verify; the judge catches scope creep, a missing tenant-isolation argument, an undocumented env var, a silent fallback that passes every test. A green suite is not a clean review and a clean review is not a green suite. Also states the cheap ordering rule -- if the gate is red, fix it before spending judges on a diff nobody will ship. Two consecutive clean passes comes from spec-forge (orchestration/process.md, "Loop control"): a single clean pass is one opinion, not evidence. When the diff did not change between passes the second relay is an independent second sample, which is requiring two panels to agree rather than re-rolling until one says yes. The synthesis agent now returns a structured verdict (blockers / shouldFix / nits) instead of prose, because the loop branches on blockers.length and a verdict buried in a paragraph cannot be branched on reliably. Reuses the lens FINDINGS item schema so severities stay consistent end to end.
bjagg
changed the title
Update — scope now covers both skills, plus a correction to my own first commitTwo commits were added after the initial review request. Both are worth reading separately. 1.
|
|
Dereck — you now have two from me. Take #1167 first; it's the one blocking a live demo outage. This one has no deadline at all. What to skip here: the four lens prompts (v1–v4) and the three relay lens prompts are unchanged from what's already on main. Don't re-read them. What's actually new is the loop control in The one question I'd most like your read on is in the addendum above: is two consecutive clean passes worth it for Context you might find useful more than the diff: the second commit exists because my first one got this wrong — I exited the loop on the first clean review, when a skill already in No rush, and a "this isn't worth the agents" verdict is a perfectly good outcome. |
2 participants
Description of Change
Problem.
multi-agent-planran a fixed four passes (v1→v4) and stopped. Four is arbitrary — v4 may still have gaps, v2 may already be clean, and nothing checked either way. The plan ended by declaring itself "implementation-ready", asserted by the same agent that wrote it.Solution. A convergence tail. A fresh reviewer checks the plan against all three lens checklists plus scope and returns structured findings; if any are blockers, a targeted revision closes exactly those, and the reviewer runs again. At most two rounds.
The v1→v4 pipeline is deliberately unchanged. Its value is lens diversity, not repetition — looping the same lens adds nothing. The loop belongs on the tail, where the question stops being "what else should this cover?" and becomes "is it done?" — and that second question needs an independent answer.
The design risk, and the guards. A loop that exits when a judge reports zero issues applies pressure in exactly one direction: toward the judge reporting nothing. That is not the same as the plan being good. Four guards, each documented in the skill with its reason:
cap-reachedis a reported outcome, not a silent stopFindings reuse the severity vocabulary from
self-review-relay(blocker/should-fix/nit), withsectionin place offile/line, so the two skills stay comparable and the plan reviewer can be pointed at a diff later.Reporting. The workflow returns
outcomeplus a per-round findings array, and the skill's "After the Workflow returns" section now has a table telling the caller what each outcome means for whether the plan is usable.cap-reachedandstalledboth mean not ready.Cost. 1–3 extra Opus agents on a skill already spending 4. Pre-flight now states 5–8 agents and 4–6 files instead of the old "~4 plan files".
Related Issues
None — dev-tooling change, no tracking issue.
Type of Change
Project Area(s) Affected
Checklist
Testing
The embedded script was extracted and exercised with stubbed agents across all five paths — clean, converged-after-revision, cap-reached, stalled, first-round-zero:
That dry run caught two real bugs before commit, which is the argument for doing it rather than eyeballing the script:
planshadowed the existingplanprompt-builder function — a hardSyntaxErrorthe script would have hit at runtime.finalVersioncounted rounds-with-blockers instead of accepted revisions, so a stalled run reportedv5whilefinalPlanwas still v4 — we'd have written a-v5.mdfile containing v4's content.Syntax verified with
node --checkagainst the body wrapped in an async function, which is how Workflow actually executes it (a barenode --checkmis-reports the top-levelreturnthat every workflow script ends with).Additional Notes
The reasoning behind which skills should and shouldn't get this — including the ones that explicitly should not (
test,promote-to-demo,issue-sweep) — is written up outside this repo in the AI-training notes. The short version: prefer a deterministic gate (ruff/ty/pytest/poly check) to a judge wherever one exists. A plan has no such oracle, which is the only reason a judge loop is right here.self-review-relayis the natural next candidate — it already half-loops informally ("offer to fix the blockers, then re-run the relevant lens") without a cap or a fresh-reviewer rule. Not in this PR.SKILL.md grows 140 → 283 lines. Most of that is the embedded script; skills load on demand rather than sitting in context like
CLAUDE.md, so the ~200-line guidance for agent maps doesn't apply — but flagging the growth rather than letting it pass unmentioned.