* fix(#3771): separate the binding property from the advisory remediation
Checker findings fused "what property failed" with "how to fix it" into a
single `fix_hint` and never marked which half binds. The checker rendered
every hint under a "must fix" heading, the orchestrators injected the issues
verbatim and ordered targeted updates, and the shared revision references
mapped each hint to a prescriptive strategy — so a contract-following planner
applied a hint literally even when a smaller mechanism satisfied the same
property, or when the hint contradicted a locked decision. There was no
channel to report that conflict, and every attempt burned a revision
iteration.
Checker side: every issue now carries a binding `required_property` (the
invariant that failed) plus its evidence and severity, and `fix_hint` is
labelled non-binding wherever it appears — including the human-facing blocker
rendering, so "must fix" unambiguously names the property and never the
example.
Planner side: revision re-checks locked decisions, capability guidance and
existing plan constraints before editing; satisfying a blocker through a
smaller valid alternative counts as addressing it; and a hint that conflicts
with any of those returns `REVISION_CONFLICT` carrying the conflict and the
alternatives considered. Orchestrators route that to user choice or the
configured plan-review convergence loop without consuming retry budget.
Also applied to the UI-spec revision loop and the gap-plan hint, and the
generic pattern's stray `suggested_fix` field name is reconciled to the
plan-checker's `fix_hint`.
Nothing legitimately binding is weakened: blockers still block, severity
still gates, iteration caps and stall escalation still fire, and required
task fields and decision coverage still hold.
Refs #3771
* test(#3771): pin the binding/advisory split across the revision chain
Locks the separation at every link that carries it: the checker's issue
schema and blocker rendering, the planner's constraint re-check and
REVISION_CONFLICT return, the generic pattern's reconciled field names, and
each orchestrator's conflict routing without retry-budget consumption. Also
pins what must not have been weakened — blockers, severity gating, iteration
caps and stall escalation.
Red against the pre-fix prose: 32 of 34 assertions fail (the 2 that pass are
the preservation checks, correctly).
Refs #3771
* chore(#3771): add changeset fragment for the remediation-binding fix
* chore(#3771): acknowledge the remediation-binding growth
Five runtime-loaded files grow: the two checkers carry the binding/advisory
split where the model reads it (a `required_property` on every dimension
example, since a schema the examples contradict teaches the examples), and
the three orchestrators carry the REVISION_CONFLICT route, which has to live
with the `iteration_count`/`revision_count` state it declines to spend.
Deletes tests/emitted-drift-acks/3172-stated-failing-direction.json: it is
fully spent on next and still owned plan-phase.md, so it walls off a key it
can no longer clear (#3078). Its removal is the documented remedy for the
duplicate-key collision, not drive-by cleanup.
* fix(#3771): close the review gaps in the conflict contract
Adversarial review (Codex, Antigravity) found four real defects in the first
pass, each confirmed against the source before acting:
- The UI checker's structured return still ordered `Fix: {exact fix required}`
and "list each BLOCK dimension with exact fix required". The dimension
examples had been marked non-binding but the rendering the researcher
actually reads had not — the same omission this issue is about.
- `ui-phase` and the canonical `revision-loop` flow incremented their counter
BEFORE dispatching the reviser, so "do NOT increment on REVISION_CONFLICT"
was unreachable prose: the iteration was already spent. The increment now
sits on the return path in both.
- The conflict gate offered "accept as-is", which is an early exit from a
still-failing blocker — a weakening the brief explicitly forbids. The three
options are now adopt an alternative / override the constraint / amend the
constraint; every one resolves the conflict. Accepting an unaddressed blocker
remains available only at the unchanged iteration-cap escalation.
- The convergence route was declarative: nothing in
plan-review-convergence.md could receive a conflict. plan-phase now records
it in REVIEWS.md — the channel that loop already consumes — convergence
refuses to declare convergence over an open entry, and routing back into a
run convergence itself started is explicitly excluded as a cycle. `quick` has
no REVIEWS.md and no phase, so its convergence branch was dead prose and is
deleted in favour of asking the user.
Also reconciles the last two drifted field names (`finding`, `affected_field`)
to the plan-checker schema, and repairs a silent no-op: the few-shot
`required_property` insertion never applied because those lines are
blockquoted, and the test's own block filter was anchored on indentation only,
so a vacuous loop passed over zero blocks. Both are fixed and the filter now
asserts it found blocks.
Refs #3771
* chore(#3771): extend the growth acknowledgment for the review round
plan-review-convergence.md joins the list: the conflict route needed a
receiving end, and it lands on the seam that loop already reads (REVIEWS.md)
rather than a new mechanism. The plan-phase, ui-phase and gsd-ui-checker
entries gain the second-pass reasoning — an executable convergence branch, the
increment moved onto the return path, and the structured return that still
ordered an exact fix.
* fix(#3771): make the conflict route bounded, ordered, and owned
Round-2 adversarial review found five more defects, each confirmed in the
source before acting:
- The convergence gate sat AFTER `gsd_run state planned-phase` and the success
banner, so a run could write and announce convergence over an unresolved
conflict. OPEN_CONFLICTS is now read from REVIEWS.md and is part of the
converged CONDITION, evaluated before any write.
- plan-phase's cycle-exclusion ("unless this run was invoked by convergence")
was not a question the orchestrator can answer at runtime. plan-phase now
never invokes convergence at all — it records the conflict when a phase
REVIEWS.md exists and resolves it with the user in-place, which removes the
cycle instead of describing it.
- Closure had no owner. plan-phase writes the row, so plan-phase strikes it
resolved; convergence only reads. An open row is a live blocker, never a
stale artifact.
- Declining to increment the counter removed the only bound on the conflict
path: an agent returning the same conflict forever would loop unattended. A
conflict naming the same `required_property` twice in a row is now a stall
and escalates through the existing gate.
- verify-work's gap-plan revision loop hands `<revision_context>` to
gsd-planner and so inherits the whole contract, but stated none of it and
could not handle the conflict return. It is now covered like the others, and
is in the test's orchestrator table.
Refs #3771
* chore(#3771): acknowledge the round-2 growth
verify-work.md joins the list — the flow the second review found missed — and
the plan-phase, ui-phase and plan-review-convergence entries gain the
round-2 reasoning: the gate moved ahead of the state write, the convergence
hand-off replaced with a runtime-checkable record-and-resolve, and the
recurrence bound that replaces the counter the conflict path stopped spending.
* fix(#3771): make the convergence gate countable and stop the conflict fall-through
Third adversarial round (Antigravity) found three defects:
- The OPEN_CONFLICTS pipeline had no `grep -v '~~'` despite its own comment
claiming one, and `grep -c '^| '` also counts a markdown table's header and
separator rows — every resolved conflict would have read as open and
convergence would have deadlocked instead of converging. plan-phase now
records each conflict as a `- [ ]` checklist line and flips it to `- [x]`, so
the gate is an exact fixed-string match with no table parsing.
- "then continue below" fell through to the checker spawn, so a SECOND
REVISION_CONFLICT would have been handed to the checker as though it were a
revised plan. plan-phase, quick and verify-work now re-evaluate the return
from the top of the conflict handler; ui-phase already looped back.
- revision-loop.md still described plan-phase routing a conflict to the
convergence loop instead of asking — the behaviour round 2 removed. Recording
is now stated as being in addition to asking, never instead of it.
Refs #3771
* chore(#3771): bring the changeset in line with what shipped
Two review rounds widened the change after the fragment was written:
verify-work's gap-plan revision and the convergence loop are covered, two
more drifted field names are reconciled, and the conflict path carries an
explicit recurrence bound.
* fix(#3771): declare and emit the REVISION_CONFLICT marker
check:contract-drift on CI caught what local lint never reached: four
workflows dispatch on `## REVISION_CONFLICT`, but no agent declared or emitted
it — an orphan consumer, matching a marker nothing produces. The shared
reference (planner-revision.md Step 7b) described the return; the agent
definitions did not carry it.
gsd-planner and gsd-ui-researcher now emit the marker in-fence alongside their
other return markers, and both registry rows in agent-contracts.md declare it.
gsd-planner's Consumed by gains the two workflows that dispatch on it and were
missing from the row.
The gate is right: a return contract belongs where the agent is defined, not
only in a reference the agent happens to load.
Refs #3771
* chore(#3771): acknowledge the return-marker growth
gsd-planner.md and gsd-ui-researcher.md each gain the REVISION_CONFLICT
marker that check:contract-drift requires them to emit.
* fix(#3771): hoist the shared conflict protocol out of the workflows
Two CI failures, both correct gates:
- tests/few-shot-calibration.test.cjs pins the plan-checker calibration file
at exactly 4 examples (2 positive, 2 negative). The example added in the
first pass broke that balance — and described PLANNER behaviour in the
CHECKER's calibration set, which is the wrong surface for it. Removed; the
smaller-alternative rule is already normative in gsd-plan-checker.md and
planner-revision.md, and pinned by the regression suite.
- tests/phase6-capstone-conformance.test.cjs (ADR-857 phase 6, #1168) requires
plan-phase.md to stay BELOW its pre-phase-6 baseline of 94519 bytes. The
inline conflict block pushed it to 94988.
The fix for the second is the one that should have been made first: the
record/resolve/close protocol and the recurrence bound were identical in four
workflows, and revision-loop.md — which plan-phase already @-imports — is what
a shared contract is for. The protocol now lives there once; plan-phase states
only its bindings (which counter, which artifact, which next step) and points
at it. plan-phase.md: 94988 -> 92739, under the ratchet with headroom, and the
four-way duplication is gone.
quick, ui-phase and verify-work do not import the reference, so they keep their
inline statements. The suite asserts each rule against what the runtime
actually loads for that orchestrator, not against the file in isolation.
Refs #3771
* docs(#3771): state the shared-protocol relationship accurately
Three of the four revision-bearing workflows do not @-import revision-loop.md,
so 'follows it verbatim' overstated the coupling. Only plan-phase defers; the
others restate the rules inline and this section is the authority they must
agree with.
* refactor(#3771): name the authority instead of restating it four times
Self-review finding: the same ~700-byte reviser paragraph was inlined in four
prompts while gsd-planner already loads planner-revision.md whenever
<revision_context> is present (agents/gsd-planner.md:555, :588) — a fifth copy
of the same contract. Four places to edit in lockstep is precisely the drift
class this PR exists to fix; the generic pattern calling the field
suggested_fix while the checker emitted fix_hint is what that looks like after
a year.
Each prompt now carries only the load-bearing clauses and names the authority
it summarises. Deliberately NOT reduced to a bare pointer: these are LLM
prompts, and a contract stated only in a file the reader is supposed to fetch
is the failure mode of this very bug. The saving is modest (~50 bytes each) —
the point is the named source of truth, not the bytes.
Suite: dropped seven assertions that pinned heading text and bold-lead
phrasing a reword would break without changing what the runtime is told; their
neighbours already pin the same contract by content. 519 -> 499 lines, 61
tests. Red gate against origin/next: 56 of 61 fail.
Refs #3771
* fix(#3771): sanitize agent-authored conflict text and bound total conflicts
Cross-AI review (agy/Gemini) raised two security findings. I confirmed the
first by running the gate's own awk against a crafted file and reproduced it
exactly; the second is a real hole in my bound.
1. FAIL-OPEN in the convergence gate. The conflict text is agent-authored and
is appended to REVIEWS.md, which the gate scans with an awk that stops at
the next '## ' heading. One line of agent text beginning '## ' ends that
scan early, so conflicts below it are never counted and convergence declares
success over a live blocker. Measured: 3 open conflicts, awk returned 2.
Fixed at the write boundary, which is the trust boundary: every field has
newlines and tabs collapsed to spaces and a leading '#', '-', '|' or fence
stripped, so one conflict is exactly one line. Both producing agents now
declare their fields single-line plain text, and the reader states the
invariant it depends on so a later edit cannot silently break it. Verified:
3 open + 1 resolved now counts 3; missing file and absent section count 0.
2. The recurrence bound was 'same required_property twice in a row', which an
agent alternating property names never trips, leaving the un-incremented
conflict path unbounded. Now bounded twice: the repeat rule catches the
common case, and the THIRD conflict return of a loop escalates whatever
property it names. A conflict still never consumes a revision iteration;
this cap is separate from and additional to the revision cap.
Rejected from the same review: deleting 'a planner that reaches
required_property by a smaller or different mechanism has addressed the issue
in full' from the CHECKER prompt as misplaced. It is load-bearing exactly
there. A checker that does not know a different mechanism counts will re-flag
the issue on re-check, which is the revision loop that never terminates. The
argument offered for deleting it, that the checker evaluates the new state
independently, describes the failure mode.
Refs #3771
* fix(#3771): fail closed on an unverifiable convergence gate
Second cross-AI pass (agy, this time with the full files rather than the diff)
found two more, both real:
1. The gate read REVIEWS_FILE with `2>/dev/null || echo 0`, so an unreadable or
empty path counted as ZERO open conflicts and converged. That path is
resolved a few lines earlier by a pre-existing unquoted
`ls ${phase_dir}/${padded_phase}-REVIEWS.md` (line 346, not touched by this
PR), which yields an empty string rather than an error when the path
contains a space. Unverifiable is not clean: the gate now tests -z and -r
first and BLOCKS. Verified both branches.
The unquoted ls itself is left alone deliberately — it predates this change
and belongs to the reviews lookup, not the conflict gate. Fixing it at my
own boundary removes its effect on this gate without widening scope.
2. REVIEWS.md is writable by the review agent, which could flip a `- [ ]` to
`- [x]` or delete the section and forge the state of a blocking gate. The
section now declares a single writer: /gsd:plan-phase appends and closes,
every other agent leaves it byte-for-byte alone, readers read.
Also trimmed a clause that explained the increment ordering by reference to
what the file said before this PR. Commit history is not instruction, and
these files are prompts.
Rejected: the claim that quick's conflict gate deadlocks autonomous pipelines
by asking the user. Its existing max-iteration escalation in the same file
already asks the user the same way; this adds no new interaction class.
Noted but out of scope: the per-dimension YAML example blocks and the shim
boilerplate duplicated across agent prompts both predate this change.
Refs #3771
* fix(#3771): count conflicts by line shape, not by section
CodeRabbit review on the rehearsal PR. Five findings, all valid, all applied.
The best one is a deletion. The convergence gate scanned between
'## Plan-Revision Conflicts' and the next '## ' heading, and that scan stops at
the FIRST heading it meets — so one stray '## ' line hid every conflict beneath
it and returned 0, converging over a live blocker. Reproduced: section-scan 0,
shape-scan 1. Sanitizing at the write boundary does not cover a hand-edited,
legacy, or foreign-written REVIEWS.md, so the reader needed its own guarantee.
It now matches the conflict line SHAPE anywhere in the file:
grep -c '^- \[ \] .*required_property:'
No section bookkeeping, nothing a heading can truncate, and it composes with the
writer's sanitization (which strips a leading '-' from agent text, so agent prose
cannot forge the shape). Verified: injected heading -> 1, all resolved -> 0.
The other four:
- Both checkers told the author never to emit a contradictory fix_hint, then
offered an escape hatch that put the forbidden route in the hint anyway. They
now name NO route in that case and state only that the property conflicts with
the constraint. A hint carrying a forbidden route is applied by anyone who
trusts hints.
- The REVISION_CONFLICT marker description in gsd-planner.md was narrower than
planner-revision.md: it covered a contradictory hint but not an unreachable
required_property. A planner reading only the agent file would have burned
retry budget on the case the reference routes to a conflict.
- The few-shot calibration examples used uppercase BLOCKER/INFO while the schema
defines blocker/warning/info. Pre-existing, but it is the same schema-vs-example
disagreement this PR exists to end, and the file was already being edited.
- verify-work's re-entry instruction existed but sat after the Bounded clause, so
the paragraph read "re-spawn ... stop re-spawning ... after re-spawning". The
re-entry now immediately follows the re-spawn, and states that only a
non-conflict return may reach the checker or increment iteration_count.
Refs #3771
* fix(#3771): resolve the contradictory scope_sanity severity examples
Sixth CodeRabbit finding, posted outside the diff range and missed on my first
read — I had claimed all findings were addressed after reading only the five
inline comments. This one was in the review body.
agents/gsd-plan-checker.md carried TWO scope_sanity examples with identical
metrics (5 tasks, 12 files) and OPPOSITE severities: warning in Dimension 5,
blocker in <examples>. Line 872 states "2-3 tasks/plan good, 4 warning, 5+
blocker" and the severity table lists warning as "Scope 4 tasks (borderline)",
so the warning example contradicted both.
ADR-2629 Decision 5's "over budget is a WARNING, never a blocker" does not
excuse it: that rule governs the smart-zone TOKEN estimate (the estimate-check
verb, lines 299-306), which is a different axis from task count. Verified in
source before touching it.
The contradiction is pre-existing but this PR made it binding and visible:
severity is now declared part of the binding payload, and both examples were
given the same required_property, so they now disagree on the severity of an
identical finding about an identical property.
Deviating from the proposed correction, which was warning -> blocker: that
would duplicate the <examples> entry outright (same tasks, files, severity).
The Dimension 5 example is instead made a genuine 4-task borderline warning, so
the file keeps one worked example per severity and the thresholds, the severity
table and both examples finally agree.
Refs #3771
* fix(#3771): stop laundering a grep error into zero open conflicts
Seventh CodeRabbit finding — from a SECOND review round my own CR-4 push
triggered, which I had not looked for. This one is a regression I introduced
while fixing the previous fail-open.
CR-4 replaced the truncatable section scan with:
OPEN_CONFLICTS=$(grep -c '^- \[ \] .*required_property:' "$REVIEWS_FILE" || true)
`|| true` masks every grep failure. grep exits 1 for "no matches" (a legitimate
zero) but 2 for a read error, and `|| true` turns both into an empty capture
that `${OPEN_CONFLICTS:-0}` renders as 0. If REVIEWS.md is removed or becomes
unreadable between the -r check and the scan, the gate reports no conflicts and
convergence proceeds. Proven: unreadable file -> captured empty -> 0.
The status is now inspected, and only exit 1 counts as zero; anything else
blocks.
My first attempt at this fix was itself wrong and my own harness caught it: I
wrote `if ! grep ...; then grep_status=$?`, but `!` inverts the status, so `$?`
in that branch is 0 and every failure reads as success — the clean-file case
printed "BLOCKED (grep exit 0)". The status must be read in the ELSE branch of a
non-negated `if`, which is what CodeRabbit proposed. Both traps are now pinned
by tests.
Verified end to end: all resolved -> 0, no conflicts at all -> 0, injected
heading -> 1, unreadable file -> BLOCKED with grep exit 2.
Refs #3771
* test(#3771): execute the conflict gate instead of reading it
CodeRabbit round three: 0 actionable, 1 nitpick — "these assertions inspect
Markdown source only; they do not prove that grep status 1 produces zero
conflicts or that a scan error exits before convergence." Rated Trivial. It is
the most valuable finding of the three rounds.
This gate has been wrong three times: a section scan a heading could truncate, a
`|| true` that laundered grep's error status into zero, and an `if !` whose `$?`
reported the negation rather than the command. Every one of those passed the
text assertions that existed at the time. I proved each fix by hand in a shell,
and none of that proof lived in the suite.
The gate is one self-contained fenced block, so the test now extracts it from
the workflow — located by content, not line number — writes it to a script and
RUNS it against fixtures: two open plus one resolved counts 2; no matches counts
0 and does not fail; a conflict below an injected `## ` heading still counts; an
unreadable path and an empty path both BLOCK with a non-zero status and no zero
count on stdout.
Non-vacuity proven by mutation rather than asserted. Reverting the gate to each
of its three historical broken forms reds the suite:
section-scan awk -> 7 failures (5 in the gate cases)
|| true -> 4 failures (3 in the gate cases)
if ! (negated $?) -> 4 failures (3 in the gate cases)
restored -> 69 pass, 0 fail
The prose assertions stay: they are the right instrument for a prompt. This
covers the one part of the change that is real shell an orchestrator executes.
Refs #3771
* test(#3771): route the gate harness through the shared test helpers
ESLint's project rules caught three violations in the new harness: an unbounded
execFileSync (DEFECT.UNBOUNDED-SUBPROCESS — an unbounded spawn is an indefinite
hang, and on macOS CI that is how a stuck run stops reporting instead of failing)
and two raw fs.rmSync calls, which skip the Windows-EBUSY retry budget that
helpers.cleanup carries.
Now uses createTempDir/cleanup from tests/helpers.cjs and passes an explicit
30s timeout. Suppressing the rules was available and would have been the wrong
call: both exist because of real CI failure modes on platforms I am not testing
on.
* chore(#3771): backfill the changeset PR number
The pr: field is drift-checked against the PR event payload, so it cannot be
written before the PR exists. Set to 3916.
* fix(#3771): close revision conflict persistence gaps
Use the authoritative review artifact, keep conflict and normal retry paths disjoint, and enforce one writer-reader grammar so malformed state fails closed.
Emitted-Drift-Ack-Growth: diagnose-issues.md — #3771 marks the gap-plan remediation hint non-binding while keeping root_cause authoritative
Emitted-Drift-Ack-Growth: gsd-plan-checker.md — #3771 separates binding required_property evidence from advisory fix_hint examples across the checker contract
Emitted-Drift-Ack-Growth: gsd-planner.md — #3771 declares the REVISION_CONFLICT return used when remediation contradicts governing constraints
Emitted-Drift-Ack-Growth: gsd-ui-checker.md — #3771 applies the same binding-property and advisory-hint split to UI review findings
Emitted-Drift-Ack-Growth: gsd-ui-researcher.md — #3771 defines the UI revision producer's structured REVISION_CONFLICT return
Emitted-Drift-Ack-Growth: plan-phase.md — #3771 routes and persists bounded revision conflicts before spending the normal retry budget
Emitted-Drift-Ack-Growth: plan-review-convergence.md — #3771 adds the fail-closed owned-block parser and prevents convergence over open conflicts
Emitted-Drift-Ack-Growth: review.md — #3771 emits and preserves the canonical writer-owned conflict block across review regeneration
Emitted-Drift-Ack-Growth: ui-phase.md — #3771 routes UI revision conflicts to resolution before consuming revision_count
Emitted-Drift-Ack-Growth: verify-work.md — #3771 gives gap-plan revision the same bounded conflict route before iteration_count
* test(#3916): guard rebases against schema drift
Load the current-base progressive-disclosure examples so every integrated issue remains bound by required_property after branch reconciliation.
* test(#3916): skip the extracted-gate suite's bash spawns on win32
Third review round's sole survivor: runConflictGate()/withReviews() spawn
bash against a Node-native temp path built by createTempDir(), which is
backslash-separated on the Windows CI lane and not a path Git Bash is
guaranteed to accept (DEFECT.WINDOWS-TEST-PORTABILITY, matching the
observed CI failure at revision-remediation-binding.test.cjs:844,
ENOENT on a path Windows read as a directory separator). No eslint rule
catches it since the call has neither a chmod nor a `bash -c` form.
Guards the four call sites with the repo's existing skipOnWin32
convention (describe/test `{ skip: IS_WINDOWS }`) rather than
normalizing the harness path to forward slashes, which would defeat the
one test whose purpose is proving the production gate does NOT rewrite
a literal backslash in a POSIX filename.
* fix(#3916): backfill changeset pr field to the fork validation PR number
* fix(#3771): forbid silently accepting an open plan-revision conflict at max-cycles escalation
The max-cycles escalation prompt only surfaced HIGH_COUNT and ACTIONABLE_COUNT; an open
plan-revision conflict (OPEN_CONFLICTS > 0) was never disclosed there, and "Proceed anyway"
could exit successfully over it — exactly the failure mode this PR exists to close (a success
banner over an unresolved conflict nobody resolved). Blockers still block: withhold "Proceed
anyway" and route to Manual review whenever a conflict is open.
* fix(#3771): do not hard-block REVISION_CONFLICT persistence when no REVIEWS.md exists yet
A phase's first-ever revision cycle can return REVISION_CONFLICT before any REVIEWS.md has been
written — REVIEWS_PATH is then legitimately empty, not a corrupt or deleted file. The persistence
gate's own accompanying prose already says the record channel applies 'when REVIEWS_FILE is
non-empty', but the bash condition never checked that, so it hard-blocked every conflict on a
brand-new phase regardless of whether persistence was even expected to run. Require a non-empty
REVIEWS_FILE before treating a missing file as an error.
* chore(#3771): raise the plan-phase.md ADR-857 host-loop ceiling to 96700
The frozen pre-phase-6 ceiling (94519) collided on rebase: this PR's own
REVISION_CONFLICT persistence/routing gate is core planner control flow, not an
un-extracted optional feature, and landed alongside an unrelated, already-merged
same-file growth (the #4.6 context-drift pre-check) already on next. Same
rationale #1298 already established for execute-phase.md's ceiling.
* chore(#3916): backfill changeset pr field to the upstream PR number
* fix(#3771): make the writer-side REVISION_CONFLICT sanitize step real shell
The Conflict Return record channel sanitized agent-authored fields via a
prose instruction ("Sanitize each agent-authored field before appending")
for the orchestrator LLM to apply by hand, while the reader-side gate in
plan-review-convergence.md parses the same slot with real, executed awk.
Flagged Minor across two review rounds (round 4, round 6) since no code
performed the sanitize anywhere.
plan-phase.md's Conflict Return step now runs a real bash gate: sanitize
each field (collapse newline/tab to space, strip a leading #/-/|/fence),
build the one-line record, skip the append if an identical line already
exists (idempotent), insert before the writer-owned end delimiter, and
fail closed if that delimiter is missing rather than silently dropping
the conflict.
tests/revision-remediation-binding.test.cjs extracts and RUNS the new
fence (matching how the reader gate is already tested), composing it
with the existing reader gate: hostile-field fast-check fuzzing, a
repeated-conflict idempotency check, and a missing-delimiter fail-closed
check that the file is left byte-for-byte unchanged on failure.
Emitted-Drift-Ack-Growth: plan-phase.md — #3916 turns the writer-side
REVISION_CONFLICT sanitize+insert step into real, executed shell instead
of a prose instruction, matching the reader gate's existing rigor
* fix(#3916): backfill changeset pr field to the fork validation PR number
Fork CI's changeset-lint reads the real PR number from its own event
payload; the fragment still carried the upstream number from the last
sync, so the DEFECT.CHANGESET-PR-FIELD-DRIFT check failed on this fork
PR. Re-backfill to the upstream number before the final push.
* fix(#3771): close the awk -v forgery and same-session close gaps agy found
Adversarial review (gemini-3.8-flash-high via the internal agy review
lane) on the full PR found two BLOCKERs against the just-added
writer-side conflict gate:
1. `awk -v line="${LINE}"` decodes escape sequences in its argument, so
a literal two-character `\n` in agent-authored text became a real
newline inside awk, splitting the appended record across two
physical lines. `tr` only strips actual control bytes, so it never
saw this — it defeated the exact forgery the gate exists to
prevent, both the reader's zero-count and the writer's own
idempotency check. Fixed by passing LINE/END through awk's
ENVIRON, which is not escape-decoded.
2. A conflict resolved and re-spawned within the same plan-phase
session was never flipped from `- [ ]` to `- [x]` — the record
channel bullet said "plan-phase closes it," but no step did. Only
a *separate* `--reviews` re-entry (line ~622, still prose-only)
closes conflicts; the in-session resolve path left them open
forever, permanently blocking convergence. Fixed by carrying the
just-written line in `PENDING_CONFLICT` and closing it in the
`Otherwise` branch before the checker re-spawns.
Also fixed a MAJOR: docs/COMMANDS.md described the `--max-cycles`
escalation gate as uniformly offering "proceed or review manually,"
but the code (this PR's own change) withholds "Proceed anyway"
specifically when a plan-revision conflict is open — only manual
review is offered in that case. Docs now say so.
Not applied: the reviewer's `\r` truncated to plain tr from a MINOR
that also asked for temp-file permission preservation across `mktemp`.
Applying `chmod --reference` is not portable to macOS/BSD `chmod`, so
this is left as a documented low-severity tradeoff — the temp file now
sits alongside REVIEWS.md (same filesystem, atomic `mv`), which was
the same finding's more substantive half. Also not applied: a
suggested `gsd_run review record-conflict` CLI subcommand to
deduplicate the two `awk` blocks — a new command plus wiring is out of
scope for a review-remediation fix.
tests/revision-remediation-binding.test.cjs adds regression coverage
for both BLOCKERs: a literal-backslash-n hostile field composed with
the reader gate, and a close-gate extraction that verifies the flip to
`[x]`, the reader's count dropping to 0, and a fail-closed path when
the pending line is missing.
Emitted-Drift-Ack-Growth: plan-phase.md — #3916 fixes an awk -v escape-
decoding forgery and adds the missing same-session conflict-close step
an adversarial review found in the writer-side gate
* chore(#3916): backfill changeset pr field to the upstream PR number
Fork-validation CI needed pr: 1 to pass its own changeset-lint; restore
pr: 3916 before this push reaches open-gsd/gsd-core.
* fix(#3771): trim plan-phase.md prose back under the XL byte cap
Merging origin/next's unrelated growth pushed plan-phase.md 473 bytes
past the workflow-size-budget XL cap and the ADR-857 phase-6 baseline,
both tripped by CI after review approval. Removed an unpinned inert
bash comment and tightened connective prose in three REVISION_CONFLICT
bullets; no executable shell or test-pinned substring changed.
* chore(rehearsal): pin changeset pr field to fork rehearsal PR #25
Scratch-only commit for the rehearsal branch's own CI. Will not be
carried onto the branch backing upstream #3916 — that keeps pr: 3916.
* fix(#3771): address CodeRabbit findings on the REVISION_CONFLICT protocol
Fork rehearsal PR #25's first CodeRabbit pass surfaced 7 findings against
the already-approved #3916 diff; each verified against current code
before fixing (none hallucinated):
- plan-phase.md: writer-side awk gates now strip a trailing \r before
comparing lines, matching the reader gate (plan-review-convergence.md)
-- a CRLF REVIEWS.md previously made both writer gates fail closed.
- plan-phase.md: the close-fence's REVIEWS_FILE/PENDING_CONFLICT/
CONFLICT_RESOLUTION were read without ever being (re)defined in that
fence -- shell state does not survive across separate fenced blocks
(same convention already documented in review.md). Added the explicit
recompute/set instruction.
- revision-loop.md: previous_conflict_property was never reset after a
normal (non-conflict) revision, so a later, unrelated conflict on the
same property could be misread as a repeat and escalate prematurely.
- gsd-plan-checker.md / few-shot-examples/plan-checker.md: two example
required_property strings were unconditionally binding in a way their
own dimension's rules aren't (no-analog RESEARCH.md fallback; tasks
that create no functions), now scoped to match.
- quick/steps/plan-checker-loop.md: added the same disjoint
"Otherwise (not REVISION_CONFLICT)" branch plan-phase.md already had,
closing an ambiguity between the conflict and non-conflict return paths.
- revision-remediation-binding.test.cjs: the REVIEWS_PATH init-order
assertion used indexOf() without checking for -1, so it would pass
vacuously if either anchor were renamed away.
Also restores an "Export the row's CONFLICT_*" instruction I had cut in
the prior byte-budget trim -- checked non-pinned by tests, but it was the
only text telling the agent to set those vars before the awk block reads
them via ENVIRON.
Net growth from these fixes required reclaiming bytes elsewhere in
plan-phase.md (verified against every pinned substring in
revision-remediation-binding.test.cjs) to stay under the XL tier's
hard 98304-byte cap; final size 98245 bytes.
* fix(#3771): resync the #4079 shrink-only mirror to the current PRE_PHASE6 line
tests/plan-phase-background-wait-wakeup.test.cjs (landed on next via an
unrelated #4079 PR, merged in by this branch's next-sync) mirrored
plan-phase.md's phase6 shrink-only ceiling as a hardcoded local constant
(94519) rather than reading tests/phase6-capstone-conformance.test.cjs's
PRE_PHASE6 value. That value has since been legitimately raised twice
during this PR's own review (94519 -> 96700 -> 98300) to accommodate the
REVISION_CONFLICT persistence/routing gate. The two branches' independent
histories left the mirror stale post-merge -- not a textual git conflict,
but the same class of thing. Resynced to 98300.
* fix(#3771): address round-2 CodeRabbit findings on the conflict gates
CodeRabbit's re-review of the previous remediation commit found two real
issues in what it had already flagged:
- Both writer-side awk CRLF fixes used \`sub(/\r$/, "")\` directly on \`\$0\`,
which mutates it in place -- \`{ print }\` then emitted the CR-stripped
copy for every passed-through line, silently rewriting an unrelated
CRLF REVIEWS.md to LF on any insert or close. Now compares against a
separate \`cur\` copy and prints the original, untouched \`\$0\`.
- The close-fence's "recompute REVIEWS_FILE/PENDING_CONFLICT" prose
implied in-fence derivation, but the fence has no such code and the
test harness (\`runCloseGate\`) deliberately supplies all three as
pre-set env vars -- matching how the open fence's "Export the row's
CONFLICT_*" instruction already works. Reworded to "export ... in the
same invocation", matching that established, test-verified pattern
instead of promising logic that isn't there.
Added a regression test proving the CRLF fix no longer touches
passthrough lines (red against the mutate-in-place version, green now).
* fix(#3771): use a CRLF-safe check in the new passthrough regression test
local/no-crlf-fragile-split forbids splitting readFileSync content on a
literal \n (Windows git-autocrlf checkouts yield \r\n). My CRLF
passthrough-preservation test from the previous commit did exactly that
to inspect the first line. Replaced with a direct startsWith() check
against the known CRLF-terminated header, which needs no split.
* test(#3771): assert the record itself is inserted in the CRLF passthrough test
CodeRabbit nitpick (round 3): the passthrough-preservation test checked
gate status and the pre-existing line's CRLF ending, but never asserted
the new REVISION_CONFLICT record was actually written.
* fix(#3771): address agy/gemini-3.8-flash-high adversarial review findings
Full-PR adversarial review (internal /gsd-review antigravity lane,
gemini-3.8-flash-high) surfaced 9 findings; each verified against current
code before fixing (none hallucinated):
HIGH:
- quick-batch/steps/plan-checker-loop.md never received the
required_property/fix_hint binding language or REVISION_CONFLICT
handling this PR added everywhere else -- a genuinely unmigrated
producing context. Migrated to match quick/steps/plan-checker-loop.md,
and added it to the ORCHESTRATORS consistency battery in
revision-remediation-binding.test.cjs so future drift is caught
automatically.
- The close-fence's PENDING_CONFLICT was an agent-supplied env var that
had to exactly reconstruct a five-field sanitized line across a
multi-minute subagent dispatch -- fragile, and a scalar var also meant
a second simultaneous conflict silently dropped the first on overwrite.
Redesigned to match the open conflict by CONFLICT_DIMENSION/
CONFLICT_PLAN identity instead: the agent re-supplies two short,
already-tracked identifiers rather than reconstructing the full
sanitized text, and each conflict resolves independently regardless of
how many are open. Updated the test harness's runCloseGate contract to
match, and added a two-open-conflicts regression test.
MEDIUM:
- plan-phase.md's `--reviews` replanning path told the reader to "flip
the matching line to [x]" in prose only, with no executable path to
it -- pointed it at the same close gate used in step 12.
- plan-review-convergence.md's reader-gate awk tolerated a blank line
before the opening delimiter but not before the heading that follows
it; a formatter or LLM writer inserting one would hard-abort
convergence on an otherwise well-formed REVIEWS.md. Added the same
tolerance already granted above it, with a regression test.
LOW:
- Clarified that the escalation destination for a stalled conflict is
the same iteration/revision-count cap gate already defined in each of
quick, quick-batch, ui-phase, and verify-work, rather than an
undefined "stall" concept.
- Clarified "twice in a row" means no successful revision intervened,
matching revision-loop.md's now-explicit previous_conflict_property
reset.
- Fixed gsd-ui-researcher.md's stale rationale text, copied verbatim
from planner-revision.md: ui-phase presents the conflict table
directly to the user, it does not persist to a shared file scanned by
heading.
Net growth again required reclaiming bytes in plan-phase.md (verified
against every pinned substring in revision-remediation-binding.test.cjs)
to stay under the XL tier's hard 98304-byte cap; removed a now-dead
PENDING_CONFLICT assignment in the process. Final size 98258 bytes.
* fix(#3771): scope row 48's quick/steps guard away from plan-checker-loop.md
tests/gsd-quick-batch-quick-regression.test.cjs's row 48 (#3676) flagged
this branch's quick-batch/steps/plan-checker-loop.md migration (the agy
HIGH finding) as a violation, because it also edits
quick/steps/plan-checker-loop.md for the same underlying #3771 protocol
fix.
Verified against git history before scoping: 2f64e6230 (#3676's own
landing commit) CREATED quick-batch/steps/plan-checker-loop.md as a new,
independent 119-line file, never a call-site into quick/'s copy. Row
48's "shared primitives, never edits the ordinary quick command" premise
was never about this specific file -- it was always meant to carry its
own per-flow copy of whatever revision-loop contract applies, same as
ui-phase.md/verify-work.md throughout this PR. This is the same
false-positive class the row's own comments already document scoping
away twice (#3730, #2529 round 40); excluded plan-checker-loop.md from
its touched-quick-steps check with the same evidence trail.
* chore(#3771): point changeset pr field at upstream PR 3916
---------
Co-authored-by: davdittrich <davdittrich@gmail.com>
Co-authored-by: CI Rebase Check <ci@gsd-redux>
Co-authored-by: Test <test@test.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
48 KiB
@~/.claude/gsd-core/references/response-language-directive.md
Cross-AI peer review — invoke external AI CLIs to independently review phase plans. Each CLI gets the same prompt (PROJECT.md context, phase plans, requirements) and produces structured feedback. Results are combined into REVIEWS.md for the planner to incorporate via --reviews flag.This implements adversarial review: different AI models catch different blind spots. A plan that survives review from 2-3 independent AI systems is more robust.
Check which AI CLIs are available on the system:_GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}"; GSD_TOOLS="${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}"; _gsd_at() { for _p; do if [ -f "$_p" ]; then GSD_TOOLS="$_p"; return 0; fi; done; return 1; }; if _gsd_at "${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}" "${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}" "${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}"; then gsd_run() { node "$GSD_TOOLS" "$@"; }; elif unset -f gsd_run; _G="$(command -v gsd_run)"; then GSD_TOOLS="$_G"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif _gsd_at "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; then gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd_run is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; GSD_IDENTITY_STATUS=unverified; case "$(gsd_run runtime-identity --raw 2>/dev/null || true)" in '{"packageName":"@opengsd/gsd-core"'*'}') GSD_IDENTITY_STATUS=ok;; esac; export GSD_IDENTITY_STATUS; [ "$GSD_IDENTITY_STATUS" = ok ] || echo "WARNING: \"$GSD_TOOLS\" did not prove it is @opengsd/gsd-core - it is either a different package or an @opengsd/gsd-core older than the runtime-identity verb. See docs/how-to/diagnose-a-foreign-gsd-tools.md" >&2; if [ -n "${CLAUDE_ENV_FILE:-}" ] && [ -n "${GSD_TOOLS:-}" ]; then printf "export PATH='%s':\"\$PATH\"\n" "${GSD_TOOLS%/*}" >> "$CLAUDE_ENV_FILE" 2>/dev/null || true; fi
# Check each CLI
command -v gemini >/dev/null 2>&1 && echo "gemini:available" || echo "gemini:missing"
command -v claude >/dev/null 2>&1 && echo "claude:available" || echo "claude:missing"
command -v codex >/dev/null 2>&1 && echo "codex:available" || echo "codex:missing"
command -v coderabbit >/dev/null 2>&1 && echo "coderabbit:available" || echo "coderabbit:missing"
command -v opencode >/dev/null 2>&1 && echo "opencode:available" || echo "opencode:missing"
command -v qwen >/dev/null 2>&1 && echo "qwen:available" || echo "qwen:missing"
command -v cursor-agent >/dev/null 2>&1 && echo "cursor:available" || echo "cursor:missing"
command -v agy >/dev/null 2>&1 && echo "antigravity:available" || echo "antigravity:missing"
command -v kimi >/dev/null 2>&1 && echo "kimi-code:available" || echo "kimi-code:missing"
# Check local model servers (OpenAI-compatible HTTP API — no CLI binary required)
OLLAMA_HOST=$(gsd_run query config-get review.ollama_host --raw 2>/dev/null || echo "")
if [ -z "$OLLAMA_HOST" ] || [ "$OLLAMA_HOST" = "null" ]; then OLLAMA_HOST="http://localhost:11434"; fi
curl -s --max-time 2 "${OLLAMA_HOST}/v1/models" >/dev/null 2>&1 && echo "ollama:available" || echo "ollama:missing"
LM_STUDIO_HOST=$(gsd_run query config-get review.lm_studio_host --raw 2>/dev/null || echo "")
if [ -z "$LM_STUDIO_HOST" ] || [ "$LM_STUDIO_HOST" = "null" ]; then LM_STUDIO_HOST="http://localhost:1234"; fi
curl -s --max-time 2 "${LM_STUDIO_HOST}/v1/models" >/dev/null 2>&1 && echo "lm_studio:available" || echo "lm_studio:missing"
LLAMA_CPP_HOST=$(gsd_run query config-get review.llama_cpp_host --raw 2>/dev/null || echo "")
if [ -z "$LLAMA_CPP_HOST" ] || [ "$LLAMA_CPP_HOST" = "null" ]; then LLAMA_CPP_HOST="http://localhost:8080"; fi
curl -s --max-time 2 "${LLAMA_CPP_HOST}/v1/models" >/dev/null 2>&1 && echo "llama_cpp:available" || echo "llama_cpp:missing"
# jq prerequisite (#2589). The config/model/budget lookups in this workflow no
# longer need jq — they use the native --raw/--pick flags. But the lanes listed
# under "jq-dependent reviewer lanes" below parse structured JSON that gsd-tools
# does not emit (OpenAI-compatible /v1/chat/completions responses, opencode's
# JSONL event stream, agy's conversation cache), so they cannot run without jq.
# Probe it here rather than letting each lane swallow exit 127 into empty output.
command -v jq >/dev/null 2>&1 && echo "jq:available" || echo "jq:missing"
jq-dependent reviewer lanes. jq is a production prerequisite for the
ollama, lm_studio, llama_cpp, opencode, and antigravity lanes only. If
detect_clis reports jq:missing, treat those five as undetected — they
follow the same "known-but-undetected" path as a missing CLI. Which path that is
depends on how the lane was selected (see the precedence rules below): reached
through review.default_reviewers or --all it is an info note and the lane is
ignored; named by an explicit flag it is an error, because the user asserted
that lane. Tell the user to install jq:
NOTE: jq is not on PATH — the ollama, lm_studio, llama_cpp, opencode, and
antigravity reviewer lanes are unavailable. Install jq (https://jqlang.org/download/)
or select a lane that does not require it (--gemini, --claude, --codex,
--coderabbit, --qwen, --cursor).
The remaining lanes (gemini, claude, codex, coderabbit, qwen, cursor)
do not require jq and must stay selectable on a jq-less host.
Parse flags from $ARGUMENTS:
--gemini→ include Gemini--claude→ include Claude--codex→ include Codex--coderabbit→ include CodeRabbit--opencode→ include OpenCode--qwen→ include Qwen Code--cursor→ include Cursor--agyor--antigravity→ include Antigravity CLI--kimi-code→ include Kimi CLI--ollama→ include Ollama (local server, OpenAI-compatible)--lm-studio→ include LM Studio (local server, OpenAI-compatible)--llama-cpp→ include llama.cpp (local server, OpenAI-compatible)--all→ include all available (CLIs + running local servers)- No flags → if
review.default_reviewersis set, include only configured reviewers that are detected; otherwise include all available
Reviewer-selection precedence:
- Individual reviewer flags (
--gemini,--codex, etc.) --allreview.default_reviewers- No key + no flags → all detected reviewers
Explicit reviewer flags are an assertion, not a preference (ADR-2782 D4). A lane the user
named on the command line and that cannot run is an error, surfaced and non-silent — even
when other named lanes did run. Do not proceed with a thinner reviewer set and report success:
--gemini --qwen on a host without qwen fails, it does not quietly become a Gemini-only
review. This applies however the lane became unavailable — binary missing, prerequisite jq
absent, or a local server not reachable.
The asymmetry is deliberate: not finding a lane nobody asked for is normal; failing to run a
lane somebody asked for is an error. A user who wants "whatever is available" has --all; a
user who wants a preferred set has review.default_reviewers. Both stay lenient below.
review.default_reviewers behavior:
- Value must be a non-empty array of slug strings (configured via
gsd config-set review.default_reviewers '["gemini","codex"]') - Unknown slugs warn and are ignored
- Known-but-undetected slugs emit an info note and are ignored — a configured default is a preference evaluated across many hosts, so a subset being present is expected, not an error
- If all configured reviewers are unavailable, fail with an actionable message
If section_manifest is null or "reviewer-instances-note-1" is in its included list: read and execute gsd-core/workflows/review/steps/reviewer-instances-note-1.md. Otherwise skip — do not read the file.
If no CLIs are available:
No external AI CLIs found. Install at least one:
- gemini: https://github.com/google-gemini/gemini-cli
- codex: https://github.com/openai/codex
- claude: https://github.com/anthropics/claude-code
- opencode: https://opencode.ai (leverages GitHub Copilot subscription models)
- qwen: https://github.com/nicepkg/qwen-code (Alibaba Qwen models)
- cursor: https://cursor.com (Cursor IDE agent mode)
- agy: curl -fsSL https://antigravity.google/cli/install.sh | bash (Antigravity CLI — free with Google credentials)
Then run /gsd:review again.
Exit.
Determine which CLI to skip based on the current runtime environment:
# Environment-based runtime detection (priority order)
if [ "$ANTIGRAVITY_AGENT" = "1" ]; then
# Antigravity is a separate client — all CLIs are external, skip none
SELF_CLI="none"
elif [ -n "$CURSOR_SESSION_ID" ]; then
# Running inside Cursor agent — skip cursor for independence
SELF_CLI="cursor"
elif [ -n "$CLAUDE_CODE_ENTRYPOINT" ]; then
# Running inside Claude Code CLI — skip claude for independence
SELF_CLI="claude"
else
# Other environments (Gemini CLI, Codex CLI, etc.)
# Fall back to AI self-identification to decide which CLI to skip
SELF_CLI="auto"
fi
Rules:
- If
SELF_CLI="none"→ invoke ALL available CLIs (no skip) - If
SELF_CLI="claude"→ skip claude, use gemini/codex - If
SELF_CLI="auto"→ the executing AI identifies itself and skips its own CLI - At least one DIFFERENT CLI must be available for the review to proceed.
INIT=$(gsd_run query init.review "${PHASE_ARG}")
if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi
# #2358: ONE run-scoped temp dir (portable via ${TMPDIR:-/tmp}) so overlapping
# runs never collide or read each other's stale files.
RUN_DIR=$(mktemp -d "${TMPDIR:-/tmp}/gsd-review-XXXXXX")
echo "RUN_DIR=$RUN_DIR"
Read from init: phase_dir, phase_number, padded_phase.
Capture RUN_DIR above (created ONCE) and thread it into every {run_dir}
placeholder and $RUN_DIR/${RUN_DIR} reference within a bash block. Do NOT
re-run mktemp -d later — every block must resolve to this same directory, or
build_prompt's writes and invoke_reviewers' reads split.
Then read:
.planning/PROJECT.md(first 80 lines — project context)- Phase section from
.planning/ROADMAP.md - All
*-PLAN.mdfiles in the phase directory *-CONTEXT.mdif present (user decisions)*-RESEARCH.mdif present (domain research).planning/REQUIREMENTS.md(requirements this phase addresses)
# Cross-AI Plan Review Request
You are reviewing implementation plans for a software project phase.
Provide structured feedback on plan quality, completeness, and risks.
## Project Context
{first 80 lines of PROJECT.md}
## Phase {N}: {phase name}
### Roadmap Section
{roadmap phase section}
### Requirements Addressed
{requirements for this phase}
### User Decisions (CONTEXT.md)
{context if present}
### Research Findings
{research if present}
### Plans to Review
For each `*-PLAN.md` in the phase directory, in glob order, include its full content preceded by a `####` header carrying the plan's **repo-relative path** (e.g. `#### .planning/phases/<phase>/<NN>-PLAN.md`). The path header is the citable anchor for findings about the plan itself — cite it as `<repo-relative plan path>:<line>` (name the heading in prose beside the citation if it helps the reader); reserve `path:line` for repo files the plan references.
{per-plan: `#### <repo-relative plan path>` + full plan contents}
## Review Instructions
**Verify against source — do not review the plan text in isolation.** The plans reference real files, migrations, routes, and tests in this repo.
1. Open the referenced files and check each claim against the actual code.
2. For every strength or concern, cite concrete `path/to/file:line` evidence plus the mechanism.
3. When a plan asserts a mechanism works (a guard, a query filter, a test that exercises a path), trace whether it actually does what is claimed — do not take the plan's word for it.
4. If you cannot read the repo (no file access), say so and downgrade that finding to an open question rather than asserting it.
Findings citing `file:line` evidence are weighted far more heavily than impressionistic ones; a review that only restates the plan's own claims has low value.
**Plan coverage is mandatory (#3301).** The exact list of plan ids and the total plan count for
this review are given in the "## Plan Coverage Manifest" section below. Give **every** listed id
its own `##`-level section headed with that id **verbatim** (e.g. `## 12.6-01`) before writing any
cross-plan comparison, an overall risk assessment, or a consensus-style summary. A review that
stops before every id has its own section is an incomplete review, not a summary — if you must
stop early, say so explicitly and name which ids you did not reach.
Analyze each plan and provide:
1. **Summary** — One-paragraph assessment
2. **Strengths** — What's well-designed (bullet points)
3. **Concerns** — Potential issues, gaps, risks (bullet points with severity: HIGH/MEDIUM/LOW)
4. **Suggestions** — Specific improvements (bullet points)
5. **Risk Assessment** — Overall risk level (LOW/MEDIUM/HIGH) with justification
Focus on:
- Missing edge cases or error handling
- Dependency ordering issues
- Scope creep or over-engineering
- Security considerations
- Performance implications
- Whether the plans actually achieve the phase goals
Output your review in markdown format.
Write to a temp file: {run_dir}/gsd-review-prompt.md
Also write individual section files so the budget tool can re-trim per reviewer:
# #2962: zsh aborts the block on an unmatched for-list glob (nomatch); bash passes it through. nullglob both.
shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null
RUN_DIR="{run_dir}" # from gather_context
# Write individual section files for per-reviewer budget trimming
# These are always written so reviewers with a budget can invoke prompt-budget
cp "$INSTRUCTIONS_BLOCK_FILE" "${RUN_DIR}/gsd-review-instructions.md"
cp "$ROADMAP_SECTION_FILE" "${RUN_DIR}/gsd-review-roadmap.md"
# Plan files: copy each PLAN.md to a predictable path named after its source
# plan id (#3959: a bare padded index discards provenance — the budget tool's
# per-plan `### <file>` header then renders a run-dir artifact name no reviewer
# or consensus step can resolve. The plan id keeps the gsd-review-plan-*.md glob
# prepare_trimmed_prompt_for_reviewer consumes.)
for PLAN_FILE in "${PHASE_DIR}"/*-PLAN.md; do
PLAN_BASENAME=$(basename "$PLAN_FILE")
PLAN_ID="${PLAN_BASENAME%-PLAN.md}"
cp "$PLAN_FILE" "${RUN_DIR}/gsd-review-plan-${PLAN_ID}.md"
done
# #3301: plan coverage manifest — tell reviewers exactly which plan ids exist and
# how many there are, so a review that silently covers 6 of 7 plans is no longer
# indistinguishable from one that covers all 7. The id is the plan file's own
# basename with the `-PLAN.md` suffix stripped (e.g. `12.6-01-PLAN.md` ->
# `12.6-01`) — NOT the `plan:` frontmatter key, which holds only the bare
# in-phase sequence number ("01") and can never reconstruct the phase-qualified
# id reviewers need to cite. The filename is guaranteed present for every copied
# plan, so no plan is ever dropped from the manifest for lacking a key.
# Count and bullets are both derived directly from the glob loop below, never
# from re-splitting an accumulated string: bash word-splits an unquoted `$VAR`
# on IFS by default, but zsh does not (no `setopt SH_WORD_SPLIT` here), so a
# prior `PLAN_IDS="$PLAN_IDS $id"` + `for x in $PLAN_IDS` round-trip silently
# collapsed every id onto one iteration under zsh whenever there were 2+ plans
# (gsd-core#4099). Direct glob iteration (`for f in "${PHASE_DIR}"/*-PLAN.md`,
# same pattern as the copy loop above) is identical under both shells, so this
# never needs word-splitting at all.
PLAN_COUNT=0
PLAN_ID_BULLETS=""
for PLAN_FILE in "${PHASE_DIR}"/*-PLAN.md; do
PLAN_BASENAME=$(basename "$PLAN_FILE")
PLAN_ID="${PLAN_BASENAME%-PLAN.md}"
PLAN_COUNT=$((PLAN_COUNT + 1))
PLAN_ID_BULLETS="${PLAN_ID_BULLETS}- ${PLAN_ID}
"
done
# Named to avoid BOTH existing RUN_DIR globs: `gsd-review-*.md` (reviewer
# reports, invoke_reviewers) and `gsd-review-plan-*.md` (the plan copies just
# above) — a manifest matching either would be picked up as a report or as a
# plan to review.
{
echo ""
echo "## Plan Coverage Manifest"
echo ""
echo "Total plans in this review: ${PLAN_COUNT}"
echo ""
echo "Plan ids (give each one its own \`##\`-level section, headed verbatim):"
printf '%s' "$PLAN_ID_BULLETS"
} > "${RUN_DIR}/.plans-manifest.md"
# Optional section files (only if content was included in the combined prompt)
if [ -f ".planning/PROJECT.md" ]; then
cp .planning/PROJECT.md "${RUN_DIR}/gsd-review-project.md"
fi
_CTX=( "${PHASE_DIR}"/*-CONTEXT.md )
if [ ${#_CTX[@]} -gt 0 ]; then
cat "${_CTX[@]}" > "${RUN_DIR}/gsd-review-context.md"
fi
_RESEARCH=( "${PHASE_DIR}"/*-RESEARCH.md )
if [ ${#_RESEARCH[@]} -gt 0 ]; then
cat "${_RESEARCH[@]}" > "${RUN_DIR}/gsd-review-research.md"
fi
if [ -f ".planning/REQUIREMENTS.md" ]; then
cp .planning/REQUIREMENTS.md "${RUN_DIR}/gsd-review-requirements.md"
fi
# #3301: append the manifest to BOTH files reviewers actually read — the
# per-lane budget-trimmed instructions file (descriptor lanes get
# `--instructions-file`) and the full combined prompt (combined-prompt lanes
# read the whole file). The `instructions` fragment is in prompt-budget's
# `minimumFor` floor set and is never trimmed, so this survives per-lane
# budget trimming intact.
cat "${RUN_DIR}/.plans-manifest.md" >> "${RUN_DIR}/gsd-review-instructions.md"
cat "${RUN_DIR}/.plans-manifest.md" >> "${RUN_DIR}/gsd-review-prompt.md"
Note: INSTRUCTIONS_BLOCK_FILE, ROADMAP_SECTION_FILE, and PHASE_DIR come from prompt assembly; RUN_DIR is the run-scoped dir from gather_context (#2358) re-assigned from {run_dir} above. Copy the temp files written during prompt assembly to these section paths (or write each section here if the prompt was built inline).
Do not re-add a per-CLI block. A <!-- reviewer-lane: … --> marker anywhere in this step now
FAILS the parity gate (checkReviewerLaneParity → bespoke_leg_present). Lane divergence is
declared in the manifest — timeout floor, probe, prompt/output channel, empty-output policy — and
behaviour that data genuinely cannot express is a named first-party handler (ADR-2782 D6), never
a bespoke block here.
Effort and model resolution (#4255). A lane's reasoning effort and model each resolve through their own declared key, and the resolution order is inspectable rather than implicit:
| piece | order, highest first |
|---|---|
| model | pinned reviewer-instance --model → the lane's modelConfigKey (review.models.<slug>) → the CLI's own default |
| effort | the lane's effortConfigKey (review.effort.<slug>) → the lane's declared defaultEffort → nothing emitted, so the CLI's own configuration applies |
Both come from the LANE. Effort in particular is never read from an agent's execution settings:
until #4255 it was resolved by querying gsd-plan-checker, so every prompt-fed lane ran at that
verifier's low and, because the rendered argument is a CLI config override, it silently beat the
effort the operator had configured for the reviewer CLI itself. A lane that declares no effort
emits no argument at all — a value borrowed from an unrelated agent is worse than no value.
Timeout guidance (#2194): prompt-fed source-grounded reviews are slow — measured ~570 s for
Codex at xhigh effort and ~525 s for headless Claude on a large plan set. Each lane declares its
own timeoutFloorMs and the runner enforces it internally, but the Bash tool call wrapping the
loop below must still be given a high timeout: — at least 900000, and 1200000 when Codex or
headless Claude are in the selection — or the host kills the whole loop mid-lane. On Claude Code,
raise the host cap via BASH_MAX_TIMEOUT_MS if a review can exceed it.
A silent empty output after a long run is a timeout kill, not a crash — the Codex 0xc0000142
misdiagnosis persisted for exactly this reason, because an empty result cannot distinguish the two
on its own. Treat an empty result on a slow lane as a dropped lane and re-run with more time rather
than diagnosing a CLI or sandbox failure. A cross-AI review that silently drops a lane is blind in
one eye.
No hook-trust bypass (#2479): no lane passes a hook-trust bypass flag and none runs a capability
probe for one. That flag only bypasses persisted hook trust (a first-run condition) and flagless
invocations work in steady state, while host-harness safety classifiers deny commands carrying it.
An environment that genuinely hits an untrusted-hook prompt surfaces through the .err capture and
the empty-output stub as a dropped lane with diagnosable stderr, not silent attrition. Do not
reintroduce the flag (even spelled out in prose — a regression test bans the literal file-wide).
If section_manifest is null or "reviewer-instances-note-2" is in its included list: read and execute gsd-core/workflows/review/steps/reviewer-instances-note-2.md. Otherwise skip — do not read the file.
Lanes run sequentially by default — concurrent invocation trips provider rate limits, and a lane
lost to one is a cross-AI review that quietly went blind in one eye. A project whose providers can
accept the concurrency opts in with review.parallel_lanes: true (#3034): the selected lanes are
dispatched together and all joined before aggregation. The default is unchanged, and convergence
cycles stay sequential either way — only the lanes within one pass overlap.
# #2962: zsh aborts the block on an unmatched for-list glob (nomatch); bash passes it through. nullglob both.
shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null
RUN_DIR="{run_dir}"
REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)"
# SELECTED_REVIEWERS is the comma-separated result of reviewer selection (ADR-0011 precedence:
# explicit flags > --all > review.default_reviewers > all detected). Unchanged by this phase.
# #3034: opt-in concurrent lane dispatch. STRICT equality on "true" is deliberate — "1", "yes" and
# "TRUE" must NOT opt in, so a mistyped config gets the conservative behaviour rather than firing
# concurrent requests at a rate-limited provider. Note the `|| echo "false"` fallback is the
# OPPOSITE polarity from the commit_docs guard, which fails OPEN: there, failing open preserves the
# user's intent; here it would fire exactly the requests the default exists to prevent.
PARALLEL_LANES=$(gsd_run query config-get review.parallel_lanes --raw 2>/dev/null || echo "false")
# Shared budget-trim helper. Was defined inside the Ollama leg; it is lane-agnostic, so it is
# hoisted here now that any lane may declare a promptBudgetKey. Returns non-zero when the budget
# is too small for the minimum review set (prompt-budget exit 2 / 11).
prepare_trimmed_prompt_for_reviewer() {
REVIEWER_KEY="$1"; REVIEWER_BUDGET="$2"; OUTPUT_PROMPT="$3"; OUTPUT_META="$4"
PLAN_FILE_ARGS=""
for p in "$RUN_DIR"/gsd-review-plan-*.md; do
[ -f "$p" ] && PLAN_FILE_ARGS="$PLAN_FILE_ARGS --plan-file $p"
done
PROJECT_ARG=""
[ -f "$RUN_DIR/gsd-review-project.md" ] && PROJECT_ARG="--project-file $RUN_DIR/gsd-review-project.md"
CONTEXT_ARG=""
[ -f "$RUN_DIR/gsd-review-context.md" ] && CONTEXT_ARG="--context-file $RUN_DIR/gsd-review-context.md"
RESEARCH_ARG=""
[ -f "$RUN_DIR/gsd-review-research.md" ] && RESEARCH_ARG="--research-file $RUN_DIR/gsd-review-research.md"
REQUIREMENTS_ARG=""
[ -f "$RUN_DIR/gsd-review-requirements.md" ] && REQUIREMENTS_ARG="--requirements-file $RUN_DIR/gsd-review-requirements.md"
gsd_run query prompt-budget \
--budget "$REVIEWER_BUDGET" \
--instructions-file "$RUN_DIR/gsd-review-instructions.md" \
--roadmap-file "$RUN_DIR/gsd-review-roadmap.md" \
$PLAN_FILE_ARGS $PROJECT_ARG $CONTEXT_ARG $RESEARCH_ARG $REQUIREMENTS_ARG \
--output-prompt "$OUTPUT_PROMPT" \
--output-metadata "$OUTPUT_META"
return $?
}
gsd_run query review-lane plan \
--selected "$SELECTED_REVIEWERS" --run-dir "$RUN_DIR" --repo-root "$REPO_ROOT" --json \
> "$RUN_DIR/gsd-review-lanes.json"
# One lane, start to finish. Hoisted into a function so the sequential and concurrent paths share
# ONE body: two dispatch bodies kept in sync by hand is the generative-fix divergence ADR-2782 spent
# a phase deleting, and it is what let #2494/#2605 be filed twice as the same defect.
#
# The result goes to a SLUG-SCOPED file, never a shared append. Concurrent O_APPEND is atomic only
# below PIPE_BUF (4096 on Linux, 512 on some platforms), so a lane result above that bound could
# interleave — and write_reviews parses this JSONL to render the models:/model_sources: frontmatter,
# so a torn line is a broken REVIEWS.md, not a cosmetic log defect.
run_review_lane() {
# `local` is hygiene, not a live fix: each `&`-dispatched call already forks its own subshell, so
# concurrent lanes cannot share these today. Scoped anyway so the isolation is a property of this
# function rather than of the dispatch mechanism happening to fork.
local SLUG LANE_BUDGET PROMPT_ARG TRIMMED
SLUG="$1"
# Per-lane prompt budget. The lane declares its own `promptBudgetKey`; `plan` resolved it,
# applying #2797's sentinel rule (-1 = unset → fall back to the global budget; 0 legitimately
# means "do not trim this lane"). Trimming itself stays in prompt-budget, which owns it.
LANE_BUDGET=$(gsd_run query review-lane plan --selected "$SLUG" --run-dir "$RUN_DIR" \
--repo-root "$REPO_ROOT" --json 2>/dev/null \
| sed -n 's/.*"promptBudget": *\([0-9-]*\).*/\1/p' | head -1)
PROMPT_ARG=""
if [ -n "$LANE_BUDGET" ] && [ "$LANE_BUDGET" != "null" ] && [ "$LANE_BUDGET" -gt 0 ] 2>/dev/null; then
TRIMMED="$RUN_DIR/gsd-review-prompt-$SLUG.md"
if prepare_trimmed_prompt_for_reviewer "$SLUG" "$LANE_BUDGET" "$TRIMMED" \
"$RUN_DIR/gsd-review-prompt-$SLUG.metadata.json"; then
PROMPT_ARG="--prompt-file $TRIMMED"
else
# A budget too small for the minimum review set drops the lane just as silently as an empty
# response used to (#2605), so leave the skip visible in the review output, not only on stderr.
echo "$SLUG review skipped: prompt budget (${LANE_BUDGET} tokens) too small for the minimum review set." \
> "$RUN_DIR/gsd-review-$SLUG.md"
# Was `continue` when this was a loop body. Inside a function that keyword is not the loop
# control it looks like — `return 0` is what skips this lane and leaves it with no result line.
return 0
fi
fi
# One invocation, whatever the lane's transport, prompt channel, output channel or handler.
# `--explicit` marks a lane the user NAMED: ADR-2782 D4 — not finding a lane nobody asked for is
# normal, failing to run one somebody asked for is an error.
gsd_run query review-lane invoke --slug "$SLUG" \
--run-dir "$RUN_DIR" --repo-root "$REPO_ROOT" $PROMPT_ARG $EXPLICIT_FLAG --json \
> "$RUN_DIR/gsd-review-lane-result-$SLUG.json"
}
# Split ONCE, de-duplicated, and reuse for both loops below. Two reasons, and the second is
# load-bearing: a slug repeated in SELECTED_REVIEWERS would put TWO concurrent background jobs on
# `> "$RUN_DIR/gsd-review-lane-result-$SLUG.json"` — the same file, both truncating. The shared-append
# form this replaced could not corrupt itself that way, so de-duping is what keeps the concurrent
# path no worse than the sequential one. Selection de-dupes today (the roster is a Set;
# review.default_reviewers normalizes lowercase-unique), but reachability analysis is not a contract
# and the next caller should not have to redo it.
#
# A plain string accumulator, not an array: zsh and bash disagree on array indexing and this block
# runs under both (see the nullglob/NULL_GLOB pairing above).
DISPATCH_SLUGS=""
for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
case " $DISPATCH_SLUGS " in
*" $SLUG "*) continue ;;
esac
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
done
# Rewrapped through unquoted command substitution, not consumed as a bare
# `$DISPATCH_SLUGS`: bash word-splits an unquoted scalar on IFS by default,
# but zsh does not, so a bare re-split collapsed every slug onto one
# iteration under zsh whenever 2+ reviewers were selected (gsd-core#4109).
# Unquoted `$(...)` re-splits identically under both shells regardless of
# `SH_WORD_SPLIT` — same reason the accumulator-building loop above already
# works under both.
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
if [ "$PARALLEL_LANES" = "true" ]; then
run_review_lane "$SLUG" &
else
run_review_lane "$SLUG"
fi
done
# Join every dispatched lane. A bare `wait` with no background jobs returns 0, so the sequential
# path needs no guard around it. NOTHING below this line may run before every lane has finished —
# write_reviews renders REVIEWS.md and the consensus summary from the aggregate below, and a review
# assembled from a partial set looks complete while silently missing a reviewer.
wait
# Aggregate in SELECTED_REVIEWERS order, NOT completion order, so the JSONL a concurrent run
# produces is byte-identical to the one a sequential run produces. This is post-join and therefore
# single-threaded, so `>>` here is safe. A lane that was budget-skipped, or that never started,
# leaves no result file and correctly contributes no line.
# Rewrapped through unquoted command substitution (gsd-core#4109) — see the
# dispatch loop above for why a bare `$DISPATCH_SLUGS` collapses under zsh.
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
LANE_RESULT="$RUN_DIR/gsd-review-lane-result-$SLUG.json"
if [ -f "$LANE_RESULT" ]; then
cat "$LANE_RESULT" >> "$RUN_DIR/gsd-review-lane-results.jsonl"
fi
done
Each lane leaves {run_dir}/gsd-review-<slug>.md — its review, or a diagnostic stub carrying the
captured stderr (and, for an OpenAI-compatible lane, the raw response body, where such a server puts
its error JSON on an HTTP 4xx/5xx while still exiting 0). A stub is never mistaken for a clean
review: it keeps its "failed or returned empty output" header (#2494/#2605/#2794).
A lane that will not run reports a typed reason rather than an empty file — missing_binary,
probe_failed, probe_timeout, missing_required_binary, host_unreachable,
egress_host_changed, unknown_handler, budget_too_small. egress_host_changed means the lane
was consented to send plans to one destination and .planning/config.json now names another; it is
blocked, not silently redirected (ADR-2782 D5).
Display progress:
### GSD ► CROSS-AI REVIEW — Phase {N}
◆ Reviewing with {CLI}... done ✓
◆ Reviewing with {CLI}... done ✓
RUN_DIR="{run_dir}"
JSONL="$RUN_DIR/gsd-review-lane-results.jsonl"
LANE_LINES=0
[ -f "$JSONL" ] && LANE_LINES=$(wc -l < "$JSONL" | tr -d ' ')
TOTAL_LANE_FAILURE="false"
ALL_LANES_SKIPPED="false"
if [ "${LANE_LINES:-0}" -eq 0 ]; then
# Zero lines means every dispatched lane left no result JSON — either every one
# was budget-skipped (N5: a skip is not a failure) or every one actually failed
# to run. Re-derive the dispatched-slug set the same way invoke_reviewers did
# (SELECTED_REVIEWERS is a shell block boundary — recompute, do not assume the
# earlier step's local DISPATCH_SLUGS variable survived into this block).
DISPATCH_SLUGS=""
for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
case " $DISPATCH_SLUGS " in
*" $SLUG "*) continue ;;
esac
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
done
# Distinguish by whether every dispatched slug's stub markdown says "skipped":
# a skip stub always does (see run_review_lane's budget branch, which writes
# this exact text before returning without ever invoking the lane); a real
# failure stub does not. If a slug has no stub at all, it is not a skip.
DISPATCHED_COUNT=0
SKIPPED_COUNT=0
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
# `$DISPATCH_SLUGS` word-splits under bash but not zsh, collapsing every
# slug onto one iteration there whenever 2+ reviewers were selected.
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
DISPATCHED_COUNT=$((DISPATCHED_COUNT + 1))
STUB="$RUN_DIR/gsd-review-$SLUG.md"
if [ -f "$STUB" ] && grep -q "review skipped: prompt budget" "$STUB" 2>/dev/null; then
SKIPPED_COUNT=$((SKIPPED_COUNT + 1))
fi
done
if [ "$DISPATCHED_COUNT" -gt 0 ] && [ "$SKIPPED_COUNT" -eq "$DISPATCHED_COUNT" ]; then
ALL_LANES_SKIPPED="true"
else
TOTAL_LANE_FAILURE="true"
fi
fi
- If
ALL_LANES_SKIPPED=true: do NOT writeREVIEWS.mdand do NOT run the commit below — there is nothing to review. Report to the user that every selected lane was budget-skipped (not a failure) and stop; do not proceed topresent_results' summary claiming a review ran. - If
TOTAL_LANE_FAILURE=true: do NOT writeREVIEWS.mdand do NOT run the commit below. Report the total lane failure to the user (name the lanes that were dispatched and point at their.err/stub files preserved under.review-diagnostics/bypresent_results) and stop. - Otherwise (at least one lane produced a result — R1, unchanged): proceed exactly as below.
#3301: plan coverage check. For each dispatched lane that produced a real review (not a
stub, not budget-skipped, not empty), check whether its output mentions every plan id from
.plans-manifest.md — the same manifest build_prompt gave the reviewer, so the expected-id list
here can never diverge from what the reviewer was actually told. This is diagnostic only: it never
blocks the workflow, never fails a lane, and never changes the TOTAL_LANE_FAILURE/
ALL_LANES_SKIPPED gate above.
CodeRabbit is excluded — it is a diff-only lane that never receives the source-grounding prompt (and therefore never receives the manifest or the per-id section instruction either), the same fact that already excludes it from grounded-review weighting in the Consensus Summary below.
The match is intentionally lenient about where an id appears (a ##-headed section is asked for,
but plain prose mentioning the id still counts as coverage — grading only the letter of the
formatting instruction would produce false INCOMPLETE verdicts against a reviewer that cited real
evidence correctly). It is strict about what counts as a match: the id is regex-escaped (a
decimal phase like 12.6 must not let 12X6-01 satisfy 12.6-01 through an unescaped .), and a
-/word character immediately before or after the candidate match does not count as a boundary (so
a threat id like T-04-07 elsewhere in the review must not register as covering plan 04-07).
RUN_DIR="{run_dir}"
MANIFEST="$RUN_DIR/.plans-manifest.md"
# Recompute — a shell variable does not survive across separate fenced blocks
# (each is its own process), so DISPATCH_SLUGS from the gate-check block above
# cannot be assumed to still be set here. Same recomputation as that block and
# as invoke_reviewers.
DISPATCH_SLUGS=""
for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
case " $DISPATCH_SLUGS " in
*" $SLUG "*) continue ;;
esac
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
done
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
# `$DISPATCH_SLUGS` word-splits under bash but not zsh, collapsing every
# slug onto one iteration there whenever 2+ reviewers were selected.
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
[ "$SLUG" = "coderabbit" ] && continue
REVIEW_FILE="$RUN_DIR/gsd-review-$SLUG.md"
[ -f "$REVIEW_FILE" ] || continue
[ -s "$REVIEW_FILE" ] || continue
grep -q "review skipped: prompt budget" "$REVIEW_FILE" 2>/dev/null && continue
grep -q "failed or returned empty output" "$REVIEW_FILE" 2>/dev/null && continue
node -e '
const fs = require("fs");
const { escapeRegex } = require("./gsd-core/bin/lib/pattern.cjs");
const manifest = fs.readFileSync(process.argv[1], "utf8");
const review = fs.readFileSync(process.argv[2], "utf8");
const ids = manifest.split("\n")
.filter((l) => l.startsWith("- "))
.map((l) => l.slice(2).trim())
.filter(Boolean);
const missing = ids.filter((id) => {
const re = new RegExp("(?<![\\w-])" + escapeRegex(id) + "(?![\\w-])");
return !re.test(review);
});
process.stdout.write(JSON.stringify({ complete: missing.length === 0, missing_ids: missing, total: ids.length }));
' "$MANIFEST" "$REVIEW_FILE" > "$RUN_DIR/.plan-coverage-$SLUG.json"
done
Each ${RUN_DIR}/.plan-coverage-<slug>.json carries {complete, missing_ids, total} for one
graded lane. Collect these into a plan_coverage frontmatter block — only when at least one
graded lane has complete: false (mirrors the existing trimmed_reviewers precedent: present
only when there is something to report):
plan_coverage: # only present if at least one graded lane is incomplete
<slug>:
total: 7
missing: ["12.6-07"]
Combine all review responses into {phase_dir}/{padded_phase}-REVIEWS.md:
Capture only the existing conflict entry bytes after the exact ## Plan-Revision Conflicts
heading and before the end of the first exact <!-- gsd:plan-revision-conflicts:begin --> /
<!-- gsd:plan-revision-conflicts:end --> pair immediately after the artifact title, if present,
as {preserved_plan_revision_conflict_entries}. Ignore identical headings or delimiters in reviewer
output: reviewers do not own blocking state. Restore the captured bytes at the explicit slot below.
After all reviewers complete, collect trim metadata files written during the run. For each reviewer that was trimmed (i.e. a .metadata.json file exists and hardFailed or omitted is non-empty, or projectMdShrunk is true, or planTruncationPct > 0), include a trimmed_reviewers block in the frontmatter. Omit the key entirely if no reviewer was trimmed.
Reviewer instances (#1517, optional): when instances ran, frontmatter records their
names, each gets its own ## <Adapter> Review (<instance>) section, and ≥2 same-cli
instances print a one-line shared-adapter caveat. Format in
gsd-core/references/reviewer-instances.md.
Resolved model (#2295): each lane's review-lane invoke --json line in
{run_dir}/gsd-review-lane-results.jsonl carries a model object; render models: from
its value and model_sources: from its source. Both maps carry exactly one entry per
reviewer that appears in reviewers: — the two key sets always match. Write the literal
unknown rather than omitting a key: an omitted key is indistinguishable from the feature
not having run, and a reader must be able to tell no model recorded from nothing to
look at. Emit every models:/model_sources: value as a DOUBLE-QUOTED YAML scalar — a
legitimate model id can contain : (llama3:70b, qwen2.5:7b), which is unquotable as a
bare scalar; a control character is already refused at the recording seam, so quoting is
what closes the remaining :/#/leading-- cases. When GSD applied a reasoning effort to
a lane, its value already carries a (reasoning=<level>) suffix (e.g.
gpt-5.6-sol (reasoning=high)) — render it as-is, without re-deriving or re-formatting it.
---
phase: {N}
reviewers: [gemini, claude, codex, coderabbit, opencode, qwen, cursor, antigravity, ollama, lm_studio, llama_cpp] # populate at runtime with only the reviewers actually invoked
reviewed_at: {ISO timestamp}
plans_reviewed: [{list of PLAN.md files}]
models: # resolved model per reviewer; `unknown` when not recoverable
codex: "gpt-5.6-sol (reasoning=low)"
antigravity: "unknown"
model_sources: # how each value above was determined
codex: "banner"
antigravity: "unknown"
trimmed_reviewers: # only present if at least one reviewer was trimmed
ollama:
budget: 6000
effective_budget: 5400
estimated_tokens: 5380
omitted: [context, research]
project_md_shrunk: true
plan_truncation_pct: 22
hard_failed: false
note_injected: true
plan_coverage: # only present if at least one graded lane is incomplete (#3301)
ollama:
total: 7
missing: ["12.6-07"]
---
# Cross-AI Plan Review — Phase {N}
<!-- gsd:plan-revision-conflicts:begin -->
## Plan-Revision Conflicts
{preserved_plan_revision_conflict_entries}
<!-- gsd:plan-revision-conflicts:end -->
<!-- Sections are RENDERED from each lane's declared `reviewsSection`, in descriptor order.
There is deliberately no hardcoded per-reviewer heading list here any more: a hand-maintained
list is exactly the drift #2781 was filed about, and it silently disagreed with the roster.
`gsd_run query review-lane sections --selected "$SELECTED_REVIEWERS"` emits
`<slug><TAB><reviewsSection>` in order; for each row, emit:
## <reviewsSection> Review
{contents of {run_dir}/gsd-review-<slug>.md}
---
Two headings must NOT be generated from this list, because they are not lanes:
* `## <Adapter> Review (<instance>)` — an ADR-1517 reviewer INSTANCE resolves THROUGH a lane
and is rendered from the instance list, not the lane list (ADR-2782 D8).
* `## Consensus Summary` — not a review section at all.
A lane whose `evidenceClass` is `diff-only` (CodeRabbit) carries its caveat from data: it never
received the source-grounding prompt, so its verdict is folded in as a diff observation and is
not weighted as a grounded plan review. -->
## Consensus Summary
{synthesize common concerns across all reviewers. CodeRabbit is a diff-only reviewer (it never received the source-grounding prompt), so do not weight its verdict as a grounded plan review — fold in its diff findings, but base plan-level consensus on the prompt-fed reviewers. A reviewer output carrying the `[reviewed-without-repo-access]` marker (or beginning with `REVIEWED-WITHOUT-REPO-ACCESS`) ran without repo access (#2176) — treat it the same way: note its concerns, but do not count its verdict at full consensus weight. A reviewer output carrying the `[reviewed-without-source-citations]` marker (#3194) declared source-grounded evidence but cited no `file:line` evidence, so it reviewed the plan text only — treat it the same way: note its concerns, but do not count its verdict at full consensus weight.}
### Agreed Strengths
{strengths mentioned by 2+ reviewers}
### Agreed Concerns
{concerns raised by 2+ reviewers — highest priority}
### Divergent Views
{where reviewers disagreed — worth investigating}
Commit (only reached when TOTAL_LANE_FAILURE and ALL_LANES_SKIPPED are both false — the
gate above):
gsd_run query commit "docs: cross-AI review for phase {N}" --files {phase_dir}/{padded_phase}-REVIEWS.md
### GSD ► REVIEW FAILED
Phase {N}: every selected reviewer lane {failed to produce a result|was budget-skipped} — no
REVIEWS.md was written.
{If the preserve+cleanup block below reports success: "Diagnostics preserved:
{phase_dir}/.review-diagnostics/". If it reports failure: relay its own warning verbatim —
it names the intact run directory holding the un-preserved evidence instead.}
Otherwise (at least one lane succeeded), display summary:
### GSD ► REVIEW COMPLETE
Phase {N} reviewed by {count} AI systems.
Consensus concerns:
{top 3 shared concerns}
Full review: {padded_phase}-REVIEWS.md
To incorporate feedback into planning:
/gsd:plan-phase {N} --reviews
#3352 (ADR-3473 §8.5, R3): preserve per-lane evidence before destroying it. Regardless of
which branch above ran, the run's temp directory is the only record that a lane failed at all —
copy it beside the phase's artifacts BEFORE cleanup. A lane that produced no output at all (L4)
leaves nothing to preserve; that is a smaller diagnostics folder, not a fabricated one, and is
NOT a preservation failure. This copy is deliberately NOT part of the commit above (N6) — that
step names only {padded_phase}-REVIEWS.md explicitly, never a directory glob, so
.review-diagnostics/ is never swept into it.
Preservation and cleanup MUST run in the same fenced block below (a shell variable cannot
survive across separate fences — each is its own process). mkdir -p and every cp are
exit-status checked; rm -rf "$RUN_DIR" runs ONLY if nothing was preserved (nothing to
preserve is not a failure) or everything that needed preserving was copied successfully. If
preservation fails partway, $RUN_DIR is left intact and a message names it as the location of
the un-preserved evidence — a leftover temp directory is far cheaper than destroyed evidence:
shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null
RUN_DIR="{run_dir}"
DIAG_DIR="{phase_dir}/.review-diagnostics"
_DIAG_MD=( "$RUN_DIR"/gsd-review-*.md )
_DIAG_ERR=()
for f in "$RUN_DIR"/gsd-review-*.err; do
[ -s "$f" ] && _DIAG_ERR+=("$f")
done
_PRESERVE_OK=true
if [ ${#_DIAG_MD[@]} -gt 0 ] || [ ${#_DIAG_ERR[@]} -gt 0 ]; then
if mkdir -p "$DIAG_DIR"; then
if [ ${#_DIAG_MD[@]} -gt 0 ] && ! cp "${_DIAG_MD[@]}" "$DIAG_DIR/"; then
_PRESERVE_OK=false
fi
if [ ${#_DIAG_ERR[@]} -gt 0 ] && ! cp "${_DIAG_ERR[@]}" "$DIAG_DIR/"; then
_PRESERVE_OK=false
fi
else
_PRESERVE_OK=false
fi
fi
if [ "$_PRESERVE_OK" = "true" ]; then
rm -rf "$RUN_DIR"
else
echo "WARNING: evidence preservation to $DIAG_DIR failed — leaving the un-preserved run directory intact at: $RUN_DIR" >&2
fi
<success_criteria>
- At least one external CLI invoked successfully
- REVIEWS.md written with structured feedback
- Consensus summary synthesized from multiple reviewers
- Temp files cleaned up
- User knows how to use feedback (/gsd:plan-phase --reviews) </success_criteria>