* feat(#1318): require external reviewers to verify plan claims against source /gsd-review built its external-reviewer prompt from plan text only and never asked reviewers to open the repo and verify claims, so a grounded HIGH could be outvoted by ungrounded LOWs. Add a concise, generic source-grounding block to build_prompt's Review Instructions: treat yourself as running in the working tree, open referenced files, cite path:line + mechanism, trace asserted mechanisms, downgrade to an open question if you have no file access, and know that grounded findings are weighted more heavily. Also clarify that CodeRabbit (a diff-only reviewer that never receives the prompt) must not be weighted as a grounded plan-level verdict in consensus synthesis. Workflow stays under its size cap (baseline bumped deliberately). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(#1318): add changeset for reviewer source-grounding Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(#1318): mark changeset docs-exempt (internal reviewer-prompt wording) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(#1318): document reviewer source-grounding in COMMANDS.md; drop docs-exempt Review: a user-visible behavioral Changed warrants a docs touch, not a docs-exempt. Add a sentence to the /gsd-review entry in docs/COMMANDS.md (reviewers verify against source, cite file:line, grounded findings weighted higher) and remove the changeset docs-exempt marker so lint:docs passes via docs-updated. Also note the literal build_prompt test anchor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(#1318): harden build_prompt fence extraction to be fence-run-aware Addresses maintainer review on PR #1421 (required-before-merge). The buildPromptReviewInstructions() test helper located the closing fence with `src.indexOf('\n```')`, which terminates at the FIRST triple-backtick line — so a build_prompt ```markdown block whose body embeds a fenced code example would truncate mid-content (dropping the `## Review Instructions` section) and give a spurious failure or false pass. Since this feature feeds source/plan content (which routinely contains code fences) to reviewers, that is a live fragility. Rewrite the extraction to be fence-run-aware, mirroring the CommonMark close rule in src/markdown-sectionizer.cts stripFencedCode: parse the opener's backtick run length, then close on the first line with >= that many backticks and only trailing whitespace — so a shorter nested fence is treated as content. Add a fail-first regression test (a 4-backtick outer fence wrapping a nested ```bash block) asserting the trailing `## Review Instructions` still extracts. Test-only change; no production .cts touched. Verified: test file 7/7, empirical fail-first proof the old indexOf logic truncated, full suite 4236/4236, eslint clean. Codex review: approve. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Behruz Nassre Esfahani
committed by
GitHub
GitHub
parent
a570cd049c
commit
ac40f070ef
@@ -46,3 +46,107 @@ describe('review workflow default reviewer selection contract (#3079)', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('review workflow source-grounding requirement in build_prompt (#1318)', () => {
|
||||
const workflow = fs.readFileSync(
|
||||
path.join(process.cwd(), 'gsd-core', 'workflows', 'review.md'),
|
||||
'utf8'
|
||||
);
|
||||
|
||||
// Extract ONLY the build_prompt Review Instructions region — the slice of the
|
||||
// assembled prompt that is actually piped to the prompt-fed reviewers. The
|
||||
// grounding instruction is worthless unless it lives HERE (#1318): asserting
|
||||
// against the whole file would still pass if the text drifted into a note,
|
||||
// the consensus step, or a comment that never reaches a reviewer's stdin.
|
||||
//
|
||||
// The region is the fenced prompt's `## Review Instructions` section, from
|
||||
// that heading up to the next `## ` heading inside the same fenced block.
|
||||
function buildPromptReviewInstructions(src) {
|
||||
// Locate the build_prompt step, then its first fenced ```markdown block.
|
||||
// NOTE: '<step name="build_prompt">' is a literal anchor — update it if the
|
||||
// step is ever renamed or gains/reorders attributes.
|
||||
const stepIdx = src.indexOf('<step name="build_prompt">');
|
||||
assert.ok(stepIdx !== -1, 'build_prompt step must exist');
|
||||
|
||||
// Fence-run-aware extraction (CommonMark): a naive `indexOf('\n```')` would
|
||||
// terminate at the FIRST triple-backtick line, truncating the prompt if its
|
||||
// body embeds a fenced code example. Mirror the close rule used by
|
||||
// src/markdown-sectionizer.cts stripFencedCode: the closing fence is a line
|
||||
// of the SAME char and >= the opener's run length, with no trailing content,
|
||||
// so a shorter nested fence inside the block is treated as content (#1318).
|
||||
// Backtick-fenced only by design — the build_prompt block is ```markdown.
|
||||
const lines = src.slice(stepIdx).split('\n');
|
||||
const openRe = /^ {0,3}(`{3,})markdown\s*$/;
|
||||
let openLen = 0;
|
||||
let bodyStart = -1;
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const m = openRe.exec(lines[i].replace(/\r$/, ''));
|
||||
if (m) { openLen = m[1].length; bodyStart = i + 1; break; }
|
||||
}
|
||||
assert.ok(bodyStart !== -1, 'build_prompt must contain a ```markdown prompt block');
|
||||
const closeRe = new RegExp(`^ {0,3}\`{${openLen},}\\s*$`);
|
||||
let bodyEnd = -1;
|
||||
for (let i = bodyStart; i < lines.length; i++) {
|
||||
if (closeRe.test(lines[i].replace(/\r$/, ''))) { bodyEnd = i; break; }
|
||||
}
|
||||
assert.ok(bodyEnd !== -1, 'build_prompt markdown fence must be closed');
|
||||
const fenced = lines.slice(bodyStart, bodyEnd).join('\n');
|
||||
|
||||
const hdr = fenced.indexOf('## Review Instructions');
|
||||
assert.ok(hdr !== -1, 'fenced prompt must contain a ## Review Instructions section');
|
||||
// Next top-level `## ` heading after the Review Instructions heading.
|
||||
const after = fenced.indexOf('\n## ', hdr + 1);
|
||||
return after === -1 ? fenced.slice(hdr) : fenced.slice(hdr, after);
|
||||
}
|
||||
|
||||
const reviewInstructions = buildPromptReviewInstructions(workflow);
|
||||
|
||||
test('instructs reviewers to verify plan claims against source and cite file:line', () => {
|
||||
// The cross-AI prompt assembled from plan text must push agentic reviewers
|
||||
// to open the referenced source and ground findings in evidence, instead of
|
||||
// paraphrasing plan text (the false-LOW failure mode in #1318). Assert the
|
||||
// instruction lives INSIDE the prompt region, not merely somewhere in file.
|
||||
assert.ok(
|
||||
reviewInstructions.includes('Verify against source') &&
|
||||
reviewInstructions.includes('check each claim against the actual code') &&
|
||||
reviewInstructions.includes('`path/to/file:line`'),
|
||||
'build_prompt Review Instructions region must require source verification + file:line evidence'
|
||||
);
|
||||
});
|
||||
|
||||
test('includes a graceful-degradation clause for reviewers without file access', () => {
|
||||
// Prompt-only reviewers (ollama / lm_studio / llama.cpp) must flag that they
|
||||
// could not verify rather than asserting an unverified finding — and this
|
||||
// clause must sit WITHIN the prompt region so reviewers actually receive it.
|
||||
assert.ok(
|
||||
reviewInstructions.includes('If you cannot read the repo (no file access)') &&
|
||||
reviewInstructions.includes('downgrade that finding to an open question'),
|
||||
'build_prompt Review Instructions region must degrade gracefully for prompt-only reviewers'
|
||||
);
|
||||
});
|
||||
|
||||
test('#1318: prompt extraction is fence-run-aware — a nested code fence does not truncate it', () => {
|
||||
// Regression guard for the fenceClose hardening. The feature feeds source/plan
|
||||
// content (which routinely contains code fences) into the prompt; a naive
|
||||
// first-`\n```` close scan would stop at a nested fence and drop everything
|
||||
// after it — including the `## Review Instructions` section — yielding a
|
||||
// spurious failure or false pass. A 4-backtick outer fence must extract in
|
||||
// full past a nested 3-backtick block.
|
||||
const synthetic = [
|
||||
'<step name="build_prompt">',
|
||||
'````markdown',
|
||||
'# Prompt',
|
||||
'Example for reviewers:',
|
||||
'```bash',
|
||||
'echo hi',
|
||||
'```',
|
||||
'## Review Instructions',
|
||||
'- Verify against source and cite `path/to/file:line`.',
|
||||
'````',
|
||||
'</step>',
|
||||
].join('\n');
|
||||
const extracted = buildPromptReviewInstructions(synthetic);
|
||||
assert.match(extracted, /## Review Instructions/);
|
||||
assert.match(extracted, /cite `path\/to\/file:line`/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -62,7 +62,7 @@
|
||||
"remove-phase.md": 8469,
|
||||
"remove-workspace.md": 7507,
|
||||
"resume-project.md": 17226,
|
||||
"review.md": 38031,
|
||||
"review.md": 39404,
|
||||
"scan.md": 7688,
|
||||
"secure-phase.md": 12282,
|
||||
"session-report.md": 4044,
|
||||
|
||||
Reference in New Issue
Block a user