From 483083aea62246d5491127c71b12d82fa19bd06e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 14 Aug 2026 00:29:42 -0400 Subject: [PATCH] fix(#3194): verify source-grounded lane evidence from review output (#3436) * fix(#3194): verify source-grounded lane evidence from review output * chore(#3194): fill changeset pr number --------- Co-authored-by: sim --- .changeset/happy-birds-roar.md | 5 + gsd-core/workflows/review.md | 2 +- src/review-lane-invocation.cts | 22 +++ src/review-lane-runner.cts | 57 +++++- ...-review-and-discuss-phase-assumptions.json | 6 - .../3194-gemini-lane-evidence-grounding.json | 6 + tests/review-lane-runner.test.cjs | 163 +++++++++++++++++- 7 files changed, 252 insertions(+), 9 deletions(-) create mode 100644 .changeset/happy-birds-roar.md delete mode 100644 tests/emitted-drift-acks/2994-fragmentize-review-and-discuss-phase-assumptions.json create mode 100644 tests/emitted-drift-acks/3194-gemini-lane-evidence-grounding.json diff --git a/.changeset/happy-birds-roar.md b/.changeset/happy-birds-roar.md new file mode 100644 index 000000000..609180c2d --- /dev/null +++ b/.changeset/happy-birds-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3436 +--- +Reviewer lanes that declare source-grounded evidence are now verified at run time: a review citing zero file:line source evidence is stamped [reviewed-without-source-citations] and down-weighted in the Consensus Summary, instead of silently riding its declared evidence class at full weight (gemini plan-only reviews were measured doing exactly this). diff --git a/gsd-core/workflows/review.md b/gsd-core/workflows/review.md index a5d131980..2aec065f4 100644 --- a/gsd-core/workflows/review.md +++ b/gsd-core/workflows/review.md @@ -464,7 +464,7 @@ trimmed_reviewers: # only present if at least one reviewer was trimmed ## 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.} +{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} diff --git a/src/review-lane-invocation.cts b/src/review-lane-invocation.cts index efbaa6da9..3b5049a88 100644 --- a/src/review-lane-invocation.cts +++ b/src/review-lane-invocation.cts @@ -24,6 +24,7 @@ import type { EmptyOutputPolicy, + EvidenceClass, LaneHandler, LaneProbe, ReviewerLane, @@ -92,6 +93,19 @@ export interface SpawnPlan { errPath: string; timeoutMs: number; emptyOutput: EmptyOutputPolicy; + /** + * The lane's declared evidence class, carried onto the plan so the runner can VERIFY the + * declaration against the review's actual output (#3194): a `source-grounded` lane whose + * review cites no `file:line` evidence is stamped and down-weighted in the Consensus + * Summary, while `diff-only` lanes are exempt (their verdict is already folded in as a + * diff observation). + * + * NORMALIZED, not trusted: this module is the overlay-manifest trust boundary and a + * third-party body can declare any value. Anything that is not exactly `'diff-only'` + * resolves as `'source-grounded'` — the fail-toward-verification direction, since the + * only behavioral consequence is whether the lane's OWN review gets down-weighted. + */ + evidenceClass: EvidenceClass; handler: LaneHandler; requiresBinaries: readonly string[]; probe: LaneProbe; @@ -121,6 +135,8 @@ export interface HttpPlan { errPath: string; timeoutMs: number; emptyOutput: EmptyOutputPolicy; + /** Declared evidence class, carried for run-time verification — see `SpawnPlan`. */ + evidenceClass: EvidenceClass; handler: LaneHandler; requiresBinaries: readonly string[]; probe: LaneProbe; @@ -319,6 +335,10 @@ export function resolveLanePlan(input: ResolveInput): ResolveResult { ? lane.timeoutFloorMs : 900_000; const emptyOutput: EmptyOutputPolicy = lane.emptyOutput === 'handler-owned' ? 'handler-owned' : 'stub-with-stderr'; + // #3194: only an EXACT 'diff-only' declaration exempts a lane from evidence verification. + // Anything else — including a missing or garbage value on a third-party overlay body — + // resolves as 'source-grounded', so the runner verifies rather than trusts it. + const evidenceClass: EvidenceClass = lane.evidenceClass === 'diff-only' ? 'diff-only' : 'source-grounded'; const requiresBinaries = Array.isArray(lane.requiresBinaries) ? lane.requiresBinaries.filter((b): b is string => typeof b === 'string') : []; @@ -378,6 +398,7 @@ export function resolveLanePlan(input: ResolveInput): ResolveResult { errPath, timeoutMs, emptyOutput, + evidenceClass, handler, requiresBinaries, probe: lane.probe, @@ -496,6 +517,7 @@ export function resolveLanePlan(input: ResolveInput): ResolveResult { errPath, timeoutMs, emptyOutput, + evidenceClass, handler, requiresBinaries, probe: lane.probe, diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index 1517b8485..4ae556c1f 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -559,6 +559,52 @@ export function stampBlindReview(review: string): string { ); } +/** + * A `path/to/file:line`-shaped source citation (#3194). + * + * The Review Instructions (review.md) require every reviewer to "cite concrete + * `path/to/file:line` evidence"; this recognizes that shape in review output. Two anchors + * make the match a source citation rather than any `colon-digits`: + * - the token before the colon must contain a path separator (`/` or `\`) or end in a + * `.extension`, so a bare PLAN-line reference — "see line 42", "L12-L18", the invented + * references measured in #3194 — does not match; + * - the token may not itself contain `:` and may not start immediately after `/` or `:`, + * so a URL (`http://localhost:8080`) and its host:port do not match either. + * + * KNOWN LIMIT (deliberate, #3194 scope): presence is checked, not resolution. A citation to + * a line that does not exist still counts — catching invented references that look impeccable + * requires repo access at stamp time and is follow-up material, not part of this fix. + */ +const SOURCE_CITATION_RE = /(? ${UNGROUNDED_MARKER}`)) return review; + if (SOURCE_CITATION_RE.test(review)) return review; + return ( + `> ${UNGROUNDED_MARKER} This reviewer declared source-grounded evidence but cited no ` + + 'file:line source evidence, so it reviewed the pasted plan text only — down-weight its ' + + 'verdict in the Consensus Summary.\n\n' + + review + ); +} + /** * `openai-compatible` — model discovery, the chat-completions round trip, and the served-model * mismatch warning. @@ -714,6 +760,13 @@ function runSpawnLane(plan: SpawnPlan, deps: RunnerDeps, repoRoot: string): Lane } } + // #3194: verify the declared evidence class against the review's actual output. A + // source-grounded lane whose review cites no file:line evidence reviewed the plan text + // only; stamp it so the Consensus Summary down-weights the verdict instead of silently + // trusting the lane's declaration. diff-only lanes are exempt — their verdict is already + // folded in as a diff observation, and the citation check must not change that surface. + if (plan.evidenceClass !== 'diff-only') review = stampUngroundedReview(review); + const { stubbed } = writeReviewOrStub(plan, review, deps, extra); return { slug: plan.slug, ok: true, stubbed }; } @@ -721,7 +774,9 @@ function runSpawnLane(plan: SpawnPlan, deps: RunnerDeps, repoRoot: string): Lane async function runHttpLane(plan: HttpPlan, deps: RunnerDeps): Promise { const promptText = deps.exists(plan.promptPath) ? deps.readFile(plan.promptPath) : ''; const { review, rawBody } = await runOpenAiCompatible(plan, promptText, deps); + // #3194: same verification on the http path — see runSpawnLane. + const stamped = plan.evidenceClass !== 'diff-only' ? stampUngroundedReview(review) : review; deps.writeFile(plan.errPath, ''); - const { stubbed } = writeReviewOrStub(plan, review, deps, rawBody); + const { stubbed } = writeReviewOrStub(plan, stamped, deps, rawBody); return { slug: plan.slug, ok: true, stubbed }; } diff --git a/tests/emitted-drift-acks/2994-fragmentize-review-and-discuss-phase-assumptions.json b/tests/emitted-drift-acks/2994-fragmentize-review-and-discuss-phase-assumptions.json deleted file mode 100644 index a258d707e..000000000 --- a/tests/emitted-drift-acks/2994-fragmentize-review-and-discuss-phase-assumptions.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "version": 1, - "paths": { - "review.md": "#2994 (epic #1671 Phase 6.3, further amendment): fragmentizes review.md onto the marker grammar, admitting `state:reviewer-instances-configured` (shared by two peripheral notes, `reviewer-instances-note-1` in detect_clis and `reviewer-instances-note-2` in invoke_reviewers — the core reviewer-lane dispatch itself stays unmarked). Also retargets the init line from the shared `init.phase-op` to a new dedicated `init.review` entry point (cmdInitReview), which computes the reviewer-instances-configured fact via `review.reviewer_instances` config presence. Net SOURCE growth is +55 bytes (29,063 -> 29,118): two `` marker-pair stubs (~331 B and ~195 B) replace their extracted prose bodies (328 B and 277 B respectively, now living in gsd-core/workflows/review/steps/*.md), and `init.phase-op` shrinks by 2 bytes to `init.review` on the gather_context init line. The EMITTED artifact composeWorkflow produces at install time still includes the extracted prose verbatim (markers strip, gap+section bodies re-join byte-for-byte) when the atom is unresolved (section_manifest null -> read-everything fallback), so installed behavior is unchanged; only the SOURCE file's on-disk byte count moves. #2962 amendment: 2 bash blocks with for-glob loops (PLAN_FILE copy iteration ~line 254, RUN_DIR plan-file args ~line 327) gained a nullglob shim (shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null) so zsh no longer aborts the block on an unmatched for-list glob (+342 bytes). #3035 amendment: added kimi-code CLI detection probe (command -v kimi) and --kimi-code flag to detect_clis and flag-parse steps (+127 bytes)." - } -} diff --git a/tests/emitted-drift-acks/3194-gemini-lane-evidence-grounding.json b/tests/emitted-drift-acks/3194-gemini-lane-evidence-grounding.json new file mode 100644 index 000000000..3a6fe043b --- /dev/null +++ b/tests/emitted-drift-acks/3194-gemini-lane-evidence-grounding.json @@ -0,0 +1,6 @@ +{ + "version": 1, + "paths": { + "review.md": "#3194: the Consensus Summary step gained one sentence teaching it to recognize the new [reviewed-without-source-citations] marker, exactly alongside its existing [reviewed-without-repo-access] recognition (#2176). The marker is prepended by stampUngroundedReview (src/review-lane-runner.cts) when a lane that declares source-grounded evidence delivers a review with zero file:line citations, so the consensus step must know it or the down-weight the fix exists to deliver never happens. Net growth +288 bytes (29,587 -> 29,875). Replaces the spent #2994 fragment (also keyed on review.md; deleted in this PR — two acks naming one path is a hard error)." + } +} diff --git a/tests/review-lane-runner.test.cjs b/tests/review-lane-runner.test.cjs index 6f21e80ba..da17b7973 100644 --- a/tests/review-lane-runner.test.cjs +++ b/tests/review-lane-runner.test.cjs @@ -9,6 +9,8 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs'); const { resolveLanePlan, LANE_UNAVAILABLE } = require('../gsd-core/bin/lib/review-lane-invocation.cjs'); @@ -19,11 +21,14 @@ const { writeReviewOrStub, handleOpencodeOutput, stampBlindReview, + stampUngroundedReview, antigravityWatermark, antigravityTranscriptFallback, runOpenAiCompatible, } = require('../gsd-core/bin/lib/review-lane-runner.cjs'); +const REVIEW_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'review.md'); + const RUN = '/run'; const ROOT = '/repo'; @@ -276,11 +281,14 @@ describe('runner — opencode handler (#1936)', () => { test('the raw JSON envelope never becomes the review', async () => { // The regression this locks: a plain stdout copy would write the JSON stream into REVIEWS.md. + // #3194: 'THE REVIEW' cites no file:line evidence, so the runner stamps it before + // writing — the assertion is therefore anchored on the review BODY, not byte-exact + // equality with the whole file. const p = plan('opencode'); const stream = JSON.stringify({ type: 'text', part: { text: 'THE REVIEW' } }); const d = deps({ spawn: () => ({ status: 0, stdout: stream, stderr: '' }) }); await runLane(p, d, { repoRoot: ROOT }); - assert.equal(d.files[p.reviewPath].trim(), 'THE REVIEW'); + assert.ok(d.files[p.reviewPath].endsWith('THE REVIEW\n')); assert.ok(!d.files[p.reviewPath].includes('"type"')); }); @@ -1008,3 +1016,156 @@ describe('#2794 qwen reviewer stderr capture', () => { } }); }); + +// #3194 — a source-grounded lane's grounding is VERIFIED from its review output, never trusted +// from its declaration. The gemini lane declares evidenceClass 'source-grounded', but nothing at +// invocation obliged grounding and nothing verified it, so measured plan-only reviews (zero real +// file:line citations, invented PLAN-line references) rode the declared class at full consensus +// weight — the weakest review getting the strongest weight, silently. The fix stamps a +// down-weight marker onto a citation-free review BEFORE it reaches {run_dir}/gsd-review-.md, +// and the Consensus Summary step recognizes the marker exactly as it already recognizes +// [reviewed-without-repo-access] (#2176). +describe('#3194 — evidence grounding is verified from review output, not declared', () => { + const MARKER = '[reviewed-without-source-citations]'; + + // The measured failure shape: a review of the pasted plan text only, restating the plan's own + // claims with invented plan-line references — "line 42", "L12-L18" — and NO file path anywhere, + // which is what separates a plan-line reference from a source citation. + const PLAN_ONLY = [ + '## Review', + '', + 'The plan looks broadly sound. Per step 2 (see line 42) the executor writes the phase', + 'artifact, and the roadmap item at line 5 is already covered by step 1 (L12-L18).', + 'One concern: the verification at line 77 lacks a timeout.', + ].join('\n'); + + const CITED = `${PLAN_ONLY}\n\nHowever, src/executor/run-phase.cts:214 has no timeout guard.`; + + describe('stampUngroundedReview — the citation check, table-tested at 0 vs 1', () => { + test('a plan-only review with invented plan-line references IS stamped', () => { + const out = stampUngroundedReview(PLAN_ONLY); + assert.ok(out.startsWith(`> ${MARKER}`), 'the marker must be prepended to the head'); + assert.ok(/down-weight/i.test(out), 'the stamp must instruct down-weighting'); + assert.ok(out.endsWith(PLAN_ONLY), 'the review body must survive verbatim after the stamp'); + }); + + test('a single real file:line citation keeps the review at full weight', () => { + assert.equal(stampUngroundedReview(CITED), CITED, 'a grounded review passes through untouched'); + }); + + test('citation shapes: paths, backticks, root files, ranges, windows separators, absolutes', () => { + for (const body of [ + 'see src/foo.cts:42 for the guard', + 'see `src/foo.cts:42` for the guard', + 'README.md:12 documents the flag', + 'a/b/c.py:7-9 duplicates the loop', + 'src\\foo.cts:42 windows path', + '(src/foo.cts:42) in parens', + 'absolute /Users/u/repo/src/x.ts:9 citation', + ]) { + assert.ok(!stampUngroundedReview(body).includes(MARKER), `${body} must count as grounded`); + } + }); + + test('non-citations: bare line refs, times, URLs — none count as grounding', () => { + for (const body of [ + 'see line 42', + 'plan line 5 says', + 'the run took 12:30', + 'server at http://localhost:8080 responded', + 'fetch https://example.com/x first', + ]) { + assert.ok(stampUngroundedReview(body).includes(MARKER), `${body} must not count as grounded`); + } + }); + + test('empty and whitespace-only reviews pass through unstamped', () => { + // The empty-output policy owns that case with a diagnostic stub; the stamp must not + // decorate a stub. + assert.equal(stampUngroundedReview(''), ''); + assert.equal(stampUngroundedReview(' \n'), ' \n'); + }); + + test('an already-stamped review is not stamped twice', () => { + const once = stampUngroundedReview(PLAN_ONLY); + assert.ok(once.includes(MARKER)); + assert.equal(stampUngroundedReview(once), once, 'stamping must be idempotent'); + }); + }); + + describe('runLane — the stamp reaches {run_dir}/gsd-review-.md', () => { + test('gemini: zero citations → the review file carries the down-weight marker', async () => { + const p = plan('gemini'); + const d = deps({ spawn: () => ({ status: 0, stdout: PLAN_ONLY, stderr: '' }) }); + await runLane(p, d, { repoRoot: ROOT }); + assert.ok( + d.files[p.reviewPath].startsWith(`> ${MARKER}`), + 'the marker must be prepended before the review is written', + ); + }); + + test('gemini: one citation → full weight, no marker', async () => { + const p = plan('gemini'); + const d = deps({ spawn: () => ({ status: 0, stdout: CITED, stderr: '' }) }); + await runLane(p, d, { repoRoot: ROOT }); + assert.ok(!d.files[p.reviewPath].includes(MARKER)); + }); + + test('coderabbit (diff-only): NOT stamped — its existing weighting must not change', async () => { + // Out-of-scope surface: CodeRabbit is already down-weighted via its declared diff-only + // class; the citation check must not add a second, redundant signal on top. + const p = plan('coderabbit'); + const d = deps({ spawn: () => ({ status: 0, stdout: PLAN_ONLY, stderr: '' }) }); + await runLane(p, d, { repoRoot: ROOT }); + assert.ok(!d.files[p.reviewPath].includes(MARKER)); + }); + + test('ollama (openai-http, source-grounded): the http path stamps too', async () => { + // The probe GET on /v1/models must be answered separately or the lane never reaches the + // chat call these assertions are about (same shape as the #2605 suite's reachableThen). + const laneDeps = (content) => deps({ + httpJson: async (url, opts) => + opts.method === 'GET' + ? { ok: true, status: 200, body: JSON.stringify({ data: [{ id: 'stub-model' }] }) } + : { + ok: true, status: 200, + body: JSON.stringify({ choices: [{ message: { content } }] }), + }, + }); + + const d0 = laneDeps(PLAN_ONLY); + const p0 = plan('ollama'); + await runLane(p0, d0, { repoRoot: ROOT }); + assert.ok( + d0.files[p0.reviewPath].startsWith(`> ${MARKER}`), + 'zero citations must stamp on the http path', + ); + + const d1 = laneDeps(CITED); + const p1 = plan('ollama'); + await runLane(p1, d1, { repoRoot: ROOT }); + assert.ok(!d1.files[p1.reviewPath].includes(MARKER), 'a citation must not stamp'); + }); + }); + + test('the resolved plan carries the declared evidenceClass (#3194 seam)', () => { + // The runner gates the stamp on this field; before #3194 the plan did not carry it at all, + // so the executor had no access to the declaration it was supposed to verify. + assert.equal(plan('gemini').evidenceClass, 'source-grounded'); + assert.equal(plan('ollama').evidenceClass, 'source-grounded'); + assert.equal(plan('coderabbit').evidenceClass, 'diff-only'); + }); + + test('the Consensus Summary step recognizes the marker, like [reviewed-without-repo-access]', () => { + // The down-weight only happens if the consensus instruction knows the marker. This locks + // the review.md prose to the runner's stamp so the two cannot drift apart silently. + const prose = fs.readFileSync(REVIEW_MD, 'utf-8'); + const step = prose.slice(prose.indexOf('## Consensus Summary')); + assert.ok(step.length > 0, 'review.md must carry a Consensus Summary step'); + assert.ok(step.includes(MARKER), 'the consensus step must name the ungrounded marker'); + assert.ok( + step.includes('[reviewed-without-repo-access]'), + 'the existing blind-review recognition must survive alongside it', + ); + }); +});