diff --git a/.changeset/rapid-ibex-sprint.md b/.changeset/rapid-ibex-sprint.md new file mode 100644 index 000000000..bb29d4ac3 --- /dev/null +++ b/.changeset/rapid-ibex-sprint.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3755 +--- +**An unevidenced lone reviewer finding no longer forces an extra replan cycle** — with two or more reviewers running, `/gsd-plan-review-convergence` now weighs a single reviewer's HIGH by what it claims: an existence claim about a symbol, file or ID must be source-grounded or corroborated, while a design finding still counts on its own unless that reviewer cited no source evidence anywhere in its review. Findings that stop counting stay visible, tagged rather than dropped, and single-reviewer runs are unchanged. A design finding from a reviewer that did cite evidence still counts alone — deliberately, so that a real architectural concern only one reviewer noticed keeps blocking. (#2398) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index a6f6a72d2..be8b5fa94 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -272,6 +272,19 @@ Cross-AI plan convergence loop — replan with review feedback until no HIGH con **Exit behavior:** Loop exits when both `current_high` and `current_actionable` hit zero. Stall detection warns when the total unresolved review count is not decreasing across cycles. Escalation gate asks the user to proceed or review manually when `--max-cycles` is hit with HIGH or actionable non-HIGH concerns still open. +**Consensus gate (2+ reviewers only).** When two or more reviewers actually run in a cycle, a HIGH raised by exactly one of them is weighed by what the claim asserts before it counts toward `current_high`: + +| Lone reviewer's HIGH asserts | Counts toward `current_high` when | +|---|---| +| **Existence** — a symbol, file, flag, commit or ID exists, is absent, or says something specific | source-grounding confirms it, **or** another reviewer raised the same concern | +| **Judgment** — a design or correctness property (missing idempotency, a race, an absent rate limit) | always, **unless** that reviewer's section opens with an evidence-quality discount marker (`[reviewed-without-source-citations]`, `[reviewed-without-repo-access]`, or a diff-only lane) | + +Judgment-class findings are deliberately exempt from corroboration: reviewers catch materially different classes of issue, so requiring two of them to independently raise the same architectural concern would suppress exactly what a multi-reviewer setup exists to surface. A suppressed HIGH is still reported, tagged `(single-reviewer, unconfirmed)` — never dropped. If **every** reviewer in a cycle carries a discount marker the gate disengages entirely, so a cycle in which nothing was verified can never be counted as converged. `current_actionable` is unaffected. + +With a single reviewer configured — the common case — behavior is unchanged. See [reviewer instances](../gsd-core/references/reviewer-instances.md) for how this interacts with `review.reviewer_instances`. + +**What this gate does not do.** It weighs *evidence*, not correctness. A reviewer that cites source evidence anywhere in its review is never discount-marked, so a **judgment-class finding it invents still counts on its own** — the marker catches "cited nothing" and "had no repo access", not "drew the wrong conclusion from a real citation". That is the deliberate side of the trade: the alternative is requiring corroboration for design findings, which suppresses the genuine architectural concern only one reviewer noticed, and would make adding reviewers *weaken* the gate. Existence-class claims are the ones tightened here. + ```bash /gsd-plan-review-convergence 3 # Default reviewers, 3 cycles /gsd-plan-review-convergence 3 --codex # Codex-only review diff --git a/gsd-core/references/reviewer-instances.md b/gsd-core/references/reviewer-instances.md index a03d1c1d8..d4d79f799 100644 --- a/gsd-core/references/reviewer-instances.md +++ b/gsd-core/references/reviewer-instances.md @@ -106,3 +106,34 @@ with an argv array and `shell: false`. - **Shared-adapter caveat:** when ≥2 invoked instances share the same base `cli`, print a one-line caveat immediately after the frontmatter (before the first section), e.g.: `> Note: opencode-deepseek and opencode-mimo share the opencode adapter; their consensus is cross-model, not cross-tool.` + +--- + +## Interaction with the convergence loop (#2398) + +Running 2+ instances changes how `/gsd:plan-review-convergence` counts HIGHs. Its **consensus gate** +(`plan-review-convergence.md`, step 5a, immediately before the counting rules) engages only when two +or more reviewers actually ran in a cycle — which is precisely the configuration this file enables. + +Under that gate, a HIGH raised by exactly one instance is treated by what the claim asserts: + +- an **existence-class** claim (a symbol, file, flag, commit or ID exists / is absent / says X) + counts toward `current_high` only if source-grounding confirms it or another reviewer raised the + same concern; +- a **judgment-class** claim (a design or correctness property) counts unless that instance's own + section opens with an evidence-quality discount marker — `[reviewed-without-source-citations]` + (#3194) or `[reviewed-without-repo-access]` (#2176). + +Judgment-class findings are deliberately exempt from the corroboration requirement: instances catch +materially different classes of issue, so demanding two of them independently raise the same +architectural concern would suppress the findings this feature exists to surface. + +A suppressed HIGH is still reported, tagged `(single-reviewer, unconfirmed)`. If every instance that +ran carries a discount marker the gate disengages entirely, so a cycle in which nothing was verified +can never be counted as converged. + +**Practical consequence for this file's use case:** instances of uneven reliability are safe to +configure. A weak instance that returns no `file:line` evidence gets stamped, and its lone +judgment-class HIGHs stop forcing replan cycles — while any instance that does produce grounded +evidence keeps full blocking weight, alone, on exactly the architectural findings it was added to +catch. diff --git a/gsd-core/workflows/plan-review-convergence.md b/gsd-core/workflows/plan-review-convergence.md index fd84fa691..e5f6690f6 100644 --- a/gsd-core/workflows/plan-review-convergence.md +++ b/gsd-core/workflows/plan-review-convergence.md @@ -200,9 +200,50 @@ Your final response MUST include a machine-readable line of exactly this form: Where is the integer count of HIGH-severity concerns that REMAIN UNRESOLVED in this cycle's findings. Where is the integer count of actionable MEDIUM/LOW concerns that REMAIN UNRESOLVED because the latest PLAN.md files do not yet incorporate them or explicitly defer/reject them. +Consensus gate (applies to NEWLY RAISED HIGHs only; evaluate before the counting rules below): + This gate engages ONLY when 2 or more reviewers actually ran and produced a review section this + cycle. With exactly one reviewer, skip this entire gate — a single reviewer's HIGH always counts, + exactly as before. + + Classify each newly raised HIGH by what the claim ASSERTS, not by whether it happens to contain a + file:line citation: + - EXISTENCE-CLASS — asserts that a named symbol, file, path, flag, commit, or ID exists, + is absent, or says something specific ("X does not exist", "the plan cites Y which is missing", + "file Z contains Q"). + - JUDGMENT-CLASS — asserts a design or correctness property ("no idempotency on retried writes", + "race between A and B", "missing rate limit"). A judgment-class HIGH stays judgment-class even + when it cites a file for context. + + A HIGH raised by 2+ reviewers is corroborated and always counts. + + For a HIGH raised by exactly ONE reviewer: + - EXISTENCE-CLASS — counts only if the source-grounding pass independently confirms it against + real project source, or another reviewer raised the same or a materially overlapping concern + (i.e. it lands in REVIEWS.md's Consensus Summary "Agreed Concerns"). + - JUDGMENT-CLASS — counts UNLESS that reviewer's own section OPENS with an evidence-quality + discount marker blockquote: `[reviewed-without-source-citations]` or + `[reviewed-without-repo-access]`, or the reviewer is a diff-only lane (CodeRabbit). The marker + must be the LEADING blockquote of that reviewer's section — a review that merely quotes a + marker while discussing it is NOT marked. Corroboration by another reviewer overrides the + marker and the HIGH counts. + + Judgment-class findings are deliberately NOT subject to corroboration. Different reviewers catch + materially different classes of issue, so requiring two of them to independently raise the same + architectural concern would suppress exactly the findings a multi-reviewer setup exists to surface. + + FAIL OPEN: if EVERY reviewer that ran this cycle carries a discount marker, this gate does not + apply at all — count as if it were absent. A gate must never manufacture convergence out of a + cycle in which nothing was verified. + + A HIGH suppressed by this gate is still listed under "## Current HIGH Concerns", tagged + `(single-reviewer, unconfirmed)`. It is excluded from current_high only — never silently dropped, + and never removed from the report. + + This gate governs current_high only. current_actionable is unaffected. + Counting rules: INCLUDE in the count: - - Newly raised HIGHs in this cycle + - Newly raised HIGHs in this cycle (subject to the consensus gate above) - PARTIALLY RESOLVED HIGHs: concern acknowledged and a mitigation is in progress, but not yet verified/completed - Previously raised HIGHs that are still unresolved diff --git a/tests/emitted-drift-acks/2398-consensus-gate-cycle-summary.json b/tests/emitted-drift-acks/2398-consensus-gate-cycle-summary.json new file mode 100644 index 000000000..58b0e4413 --- /dev/null +++ b/tests/emitted-drift-acks/2398-consensus-gate-cycle-summary.json @@ -0,0 +1,7 @@ +{ + "$comment": "Growth ack (#2914 fragment). Reason: #2398 adds the consensus gate to the CYCLE_SUMMARY contract in plan-review-convergence.md step 5a — a claim-class split (existence-class needs source-grounding or corroboration; judgment-class counts unless its raiser opens with an evidence-quality discount marker), plus the all-marked fail-open and the leading-marker requirement. 30889 -> 33566 LF bytes (+2677). Marker literals are pinned to what gsd-core/bin/lib/review-lane-runner.cjs actually emits by a parity test in tests/plan-review-convergence.test.cjs, so the gate cannot key on a signal nothing produces. gsd-core/references/reviewer-instances.md also grew (+1823B) but is deliberately NOT acked here: currentSizes() (tests/helpers/emitted-runtime.cjs:916-929) scans only gsd-core/workflows/ and agents/, so references/ is outside the growth ratchet entirely and an entry for it would be inert. Its emitted-hash ripple is attributable to this diff and needs no acknowledgment.", + "version": 1, + "paths": { + "plan-review-convergence.md": "gsd-core/workflows/plan-review-convergence.md +2677B: consensus gate on newly-raised HIGHs, split by claim class, resolving B2 from closed PR #2417 (#2398)" + } +} diff --git a/tests/plan-review-convergence.test.cjs b/tests/plan-review-convergence.test.cjs index f9ef48649..8ca4cd2c6 100644 --- a/tests/plan-review-convergence.test.cjs +++ b/tests/plan-review-convergence.test.cjs @@ -33,6 +33,7 @@ const fs = require('fs'); const path = require('path'); const { execFileSync } = require('node:child_process'); const { readFileNormalized } = require('./helpers.cjs'); +const fc = require('fast-check'); const COMMAND_PATH = path.join(__dirname, '..', 'commands', 'gsd', 'plan-review-convergence.md'); const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md'); @@ -1913,3 +1914,277 @@ describe('plan-review-convergence: cross-artifact fact-drift pass (#1956)', () = }); }); }); + +// ══ #2398 — consensus gate for CYCLE_SUMMARY with multi-reviewer runs ═══════════════════ +// +// Supersedes PR #2417, which its author closed on the unresolved B2 finding: the approved +// wording let a lone HIGH count if "the source-grounding pass independently confirms it", +// but that pass verifies "every symbol THE PLAN cites" — it never takes reviewer claims as +// input. For a genuine architectural HIGH raised by one reviewer and missed by another: +// ungroundable, uncorroborated, so it stopped gating. Net effect, in the #2417 review's +// words: configuring MORE reviewers produced a WEAKER gate than configuring one. +// +// The gate therefore splits by what a claim ASSERTS: +// - existence/citation-class -> source-grounding OR corroboration (catches fabricated cites) +// - judgment/architectural -> counts unless the RAISER carries an evidence-quality +// discount marker <- this is the B2 fix +// +// Linus's Law is the reason: "different reviewers think differently" — demanding two of them +// independently raise the SAME architectural finding destroys the mechanism a multi-reviewer +// setup exists for. Its limits clause supplies the other half: "rubber-stamp reviews don't +// count", and a reviewer that produced no file:line evidence is a rubber-stamp for that cycle. +// +// Assertions are on parsed structure and typed sets. The contract SENTENCES are themselves the +// deliverable (source-text-is-the-product), and carry no allow-test-rule marker deliberately: +// no-source-grep never inspects .md reads, so a marker suppresses nothing and consumes the +// ceilinged unverified-marker budget (measured 2026-08-21, 280 -> 281 fails the gate). +// +// See https://github.com/open-gsd/gsd-core/issues/2398 + +const WORKFLOW_2398 = path.join(__dirname, '..', 'gsd-core', 'workflows', 'plan-review-convergence.md'); +const REVIEWER_INSTANCES_2398 = path.join(__dirname, '..', 'gsd-core', 'references', 'reviewer-instances.md'); +const runner2398 = require('../gsd-core/bin/lib/review-lane-runner.cjs'); + +const lf2398 = (t) => String(t == null ? '' : t).replace(/\r\n/g, '\n'); + +function read2398(p) { + return fs.existsSync(p) ? fs.readFileSync(p, 'utf8') : ''; +} + +/** The `### 5a` step body, bounded by the next `### ` heading. */ +function step5a2398(text) { + const lines = lf2398(text).split('\n'); + const start = lines.findIndex((l) => /^###\s+5a[.\s]/.test(l)); + if (start === -1) return ''; + let end = start + 1; + while (end < lines.length && !/^###\s/.test(lines[end])) end += 1; + return lines.slice(start, end).join('\n'); +} + +/** The consensus-gate block: from its heading to the line before `Counting rules:`. */ +function consensusGate2398(text) { + const body = lf2398(text); + const gateAt = body.search(/^\s*Consensus gate\b/m); + if (gateAt === -1) return ''; + const countingAt = body.indexOf('Counting rules:', gateAt); + return countingAt === -1 ? body.slice(gateAt) : body.slice(gateAt, countingAt); +} + +/** Every `[reviewed-without-*]` marker literal named in `text`, deduped and sorted. */ +function markerNames2398(text) { + return [...new Set([...lf2398(text).matchAll(/\[reviewed-without-[a-z-]+\]/g)].map((m) => m[0]))].sort(); +} + +/** Ordered positions of the contract landmarks the gate must sit between. */ +function landmarks2398(text) { + const body = lf2398(text); + return { + contract: body.indexOf('IMPORTANT — CYCLE_SUMMARY contract'), + gate: body.search(/^\s*Consensus gate\b/m), + counting: body.indexOf('Counting rules:'), + definitions: body.indexOf('Definitions:'), + }; +} + +describe('#2398 — consensus gate is declared and correctly positioned', () => { + const workflow = read2398(WORKFLOW_2398); + + test('step 5a declares a consensus gate', () => { + assert.notEqual(consensusGate2398(step5a2398(workflow)), '', 'no Consensus gate block found in step 5a'); + }); + + test('gate precedes the counting rules it constrains', () => { + const at = landmarks2398(step5a2398(workflow)); + assert.ok(at.contract >= 0 && at.gate >= 0 && at.counting >= 0 && at.definitions >= 0, + `missing landmark: ${JSON.stringify(at)}`); + assert.ok(at.contract < at.gate, 'gate must sit inside the CYCLE_SUMMARY contract'); + assert.ok(at.gate < at.counting, 'gate must precede Counting rules, or it constrains nothing'); + assert.ok(at.counting < at.definitions, 'existing Counting rules -> Definitions order must survive'); + }); + + test('the CYCLE_SUMMARY contract line itself is unchanged', () => { + // The orchestrator greps `current_high=[0-9]+` at :320-321. This change alters the NUMBER + // the agent computes, never the line's shape — that is the Hyrum-safe boundary. + assert.ok(lf2398(workflow).includes('CYCLE_SUMMARY: current_high= current_actionable=')); + }); + + test('step 5a fences stay balanced', () => { + const fences = (lf2398(step5a2398(workflow)).match(/^\s*```/gm) || []).length; + assert.equal(fences % 2, 0, `odd fence count (${fences}) in step 5a — a fence was left open`); + }); +}); + +describe('#2398 — gate semantics: the clauses that make it correct', () => { + const gate = () => lf2398(consensusGate2398(step5a2398(read2398(WORKFLOW_2398)))).toLowerCase(); + + test('gate engages only when 2+ reviewers actually ran', () => { + const g = gate(); + assert.ok(/2\+|two or more|at least two/.test(g), 'gate must state its 2+ reviewer trigger'); + assert.ok(/\bran\b|produced|returned/.test(g), + 'trigger must be reviewers that RAN, not merely configured — a failed reviewer must not arm the gate'); + }); + + test('single reviewer is documented as unchanged', () => { + assert.ok(/single reviewer|one reviewer|exactly one/.test(gate()), + 'the single-reviewer no-op is the backward-compatibility promise and must be stated'); + }); + + test('threshold is two, not three', () => { + const g = gate(); + assert.ok(!/three or more|3\+ reviewers/.test(g), 'threshold must be 2, matching the approved scope'); + }); + + // ── the B2 regression ────────────────────────────────────────────────────────────────── + test('judgment-class lone HIGH is exempt from corroboration (B2)', () => { + const g = gate(); + assert.ok(/judgment|architectural/.test(g), 'gate must name the judgment/architectural class'); + assert.ok(/counts?\b[\s\S]{0,200}?(unless|except)[\s\S]{0,200}?marker/.test(g), + 'a judgment-class lone HIGH must COUNT unless the raiser is marked — if it instead requires ' + + 'corroboration, B2 is back and more reviewers produce a weaker gate'); + }); + + test('existence-class lone HIGH requires grounding or corroboration', () => { + const g = gate(); + assert.ok(/existence|citation-class|cites a symbol/.test(g), 'gate must name the checkable class'); + assert.ok(/source-ground/.test(g) && /corroborat/.test(g), + 'the checkable class keeps both original paths: grounding OR corroboration'); + }); + + test('classification is by assertion, not by citation presence (row 14)', () => { + assert.ok(/asserts?\b/.test(gate()), + 'gate must classify by what the claim ASSERTS — keying on the presence of a file:line ' + + 'silently reclassifies every architectural finding that cites context, reintroducing B2'); + }); + + test('an all-marked cycle fails open', () => { + const g = gate(); + assert.ok(/every reviewer|all reviewers/.test(g) && /(does not (apply|engage)|fails? open)/.test(g), + 'if every reviewer is marked the gate must disengage — a gate must never manufacture convergence'); + }); + + test('a suppressed HIGH remains listed and tagged', () => { + const g = gate(); + assert.ok(/current high concerns/.test(g), 'suppressed HIGHs must still be listed'); + assert.ok(/tag|unconfirmed|single-reviewer/.test(g), 'and must be visibly tagged, not silently dropped'); + }); + + test('gate governs current_high only, and says so explicitly', () => { + const g = gate(); + assert.ok(/current_high/.test(g), 'gate must name the count it governs'); + // Asserting the gate never MENTIONS current_actionable was the wrong test: stating the + // exclusion is what keeps a future editor from quietly widening the gate's reach. + assert.ok(/current_actionable is unaffected|does not affect current_actionable|current_actionable is out of scope/.test(g), + 'gate must state explicitly that current_actionable is out of scope'); + }); + + test('gate keys on a leading marker, not a quoted one', () => { + // stampBlindReview's own doc warns a review that merely QUOTES a marker must not be + // mis-stamped; the stamp is a LEADING blockquote, so the gate must say so. + assert.ok(/leading|opens|begins|first line|blockquote/.test(gate()), + 'gate must require the marker to OPEN the reviewer section, or a review quoting a marker ' + + 'gets its own findings suppressed'); + }); +}); + +describe('#2398 — marker parity: the gate names markers the runner actually PRODUCES', () => { + // Earlier this asserted the marker string appeared somewhere in the runner's SOURCE TEXT. + // That would pass even if stampUngroundedReview were broken or never called — string + // co-occurrence, not behavior. These invoke the real exported stampers instead. + + /** Markers the runner genuinely emits, observed by calling it. */ + function emittedMarkers2398() { + const observed = new Set(); + const ungrounded = runner2398.stampUngroundedReview('HIGH: no idempotency on retried writes.'); + const blind = runner2398.stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nHIGH: something.'); + for (const stamped of [ungrounded, blind]) { + const m = /^> (\[reviewed-without-[a-z-]+\])/.exec(stamped); + if (m) observed.add(m[1]); + } + return [...observed].sort(); + } + + test('the runner stamps an uncited review, and the marker LEADS the output', () => { + const stamped = runner2398.stampUngroundedReview('HIGH: no idempotency on retried writes.'); + assert.match(stamped, /^> \[reviewed-without-source-citations\]/, + 'the marker must be the leading blockquote — the gate keys on that position'); + assert.ok(stamped.includes('HIGH: no idempotency on retried writes.'), + 'the original review must be preserved beneath the marker'); + }); + + test('the runner does NOT stamp a review carrying a file:line citation', () => { + const cited = 'HIGH: see src/a.ts:42 — the race is real.'; + assert.equal(runner2398.stampUngroundedReview(cited), cited); + }); + + test('the runner stamps a self-reported blind review', () => { + assert.match( + runner2398.stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nHIGH: something.'), + /^> \[reviewed-without-repo-access\]/, + ); + }); + + test('stamping is idempotent — an already-stamped review gains no second marker', () => { + const once = runner2398.stampUngroundedReview('bare review'); + assert.equal(runner2398.stampUngroundedReview(once), once); + }); + + test('gate names only markers the runner actually produces', () => { + const emitted = emittedMarkers2398(); + const named = markerNames2398(consensusGate2398(step5a2398(read2398(WORKFLOW_2398)))); + assert.deepEqual(emitted, ['[reviewed-without-repo-access]', '[reviewed-without-source-citations]'], + 'runner must produce both markers when invoked'); + assert.ok(named.length >= 1, 'the gate must name at least one concrete marker literal'); + assert.deepEqual(named.filter((m) => !emitted.includes(m)), [], + 'gate names a marker the runner never produces — the gate would be inert'); + }); + + test('parity fails when the gate names a marker the runner does not produce', () => { + // Non-vacuity: a guard that only reads a correct tree never runs its failure branch. + const emitted = emittedMarkers2398(); + const mutated = markerNames2398('[reviewed-without-source-citations] and [reviewed-without-telemetry]'); + assert.deepEqual(mutated.filter((m) => !emitted.includes(m)), ['[reviewed-without-telemetry]']); + }); + + test('parsers are total on empty, whitespace-only and absent input', () => { + for (const input of ['', ' \n\t\n ', null, undefined, read2398('/nonexistent/2398.md')]) { + assert.equal(step5a2398(input), ''); + assert.equal(consensusGate2398(input), ''); + assert.deepEqual(markerNames2398(input), []); + } + }); + + test('parsers are newline-agnostic (CRLF === LF)', () => { + for (const p of [WORKFLOW_2398, REVIEWER_INSTANCES_2398]) { + const lfText = lf2398(read2398(p)); + const crlf = lfText.replace(/\n/g, '\r\n'); + assert.equal(step5a2398(crlf), step5a2398(lfText)); + assert.deepEqual(markerNames2398(crlf), markerNames2398(lfText)); + } + }); + + test('property: parity is strictly sensitive to a marker the runner never produces', () => { + const emitted = emittedMarkers2398(); + fc.assert( + fc.property( + fc.subarray(emitted, { minLength: 1 }), + fc.constantFrom('telemetry', 'network', 'sandbox', 'cache'), + (subset, novel) => { + assert.deepEqual(markerNames2398(subset.join(' ')).filter((m) => !emitted.includes(m)), []); + const withNovel = `${subset.join(' ')} [reviewed-without-${novel}]`; + assert.deepEqual(markerNames2398(withNovel).filter((m) => !emitted.includes(m)), + [`[reviewed-without-${novel}]`]); + }, + ), + { seed: 2398, numRuns: 100 }, + ); + }); +}); + +describe('#2398 — reviewer-instances cross-reference', () => { + test('reviewer-instances documents the convergence-gate interaction', () => { + const ref = lf2398(read2398(REVIEWER_INSTANCES_2398)).toLowerCase(); + assert.ok(/consensus gate/.test(ref), 'the reference must name the gate'); + assert.ok(/plan-review-convergence|current_high/.test(ref), + 'and must point at where it takes effect, so a reader configuring instances finds it'); + }); +});