* fix(#3194): verify source-grounded lane evidence from review output * chore(#3194): fill changeset pr number --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
5
.changeset/happy-birds-roar.md
Normal file
5
.changeset/happy-birds-roar.md
Normal file
@@ -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).
|
||||
@@ -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}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 = /(?<![/:])(?:[^\s:]*[/\\][^\s:]*|[^\s:]*\.[A-Za-z0-9]{1,16}):[0-9]+/;
|
||||
|
||||
/** The marker the Consensus Summary step recognizes and down-weights (#3194). */
|
||||
const UNGROUNDED_MARKER = '[reviewed-without-source-citations]';
|
||||
|
||||
/**
|
||||
* Stamp a machine-readable marker when a source-grounded lane's review cites no `file:line`
|
||||
* evidence (#3194).
|
||||
*
|
||||
* The sibling of `stampBlindReview`, for a different failure mode: that one fires when a
|
||||
* reviewer REPORTS it had no repo access; this one fires when a lane that DECLARES
|
||||
* `source-grounded` evidence delivered none — the review restates the plan's own claims with
|
||||
* at most invented plan-line references. Either way the Consensus Summary must not count the
|
||||
* verdict at full weight, which is why both prepend a marker the consensus step recognizes.
|
||||
*
|
||||
* Idempotent and empty-safe: an already-stamped review passes through unchanged, and an empty
|
||||
* review is left to the empty-output policy's diagnostic stub.
|
||||
*/
|
||||
export function stampUngroundedReview(review: string): string {
|
||||
if (isEmptyReview(review)) return review;
|
||||
if (review.startsWith(`> ${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<LaneRunResult> {
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -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 `<!-- gsd:section -->` 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)."
|
||||
}
|
||||
}
|
||||
@@ -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)."
|
||||
}
|
||||
}
|
||||
@@ -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-<slug>.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-<slug>.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',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user