diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index fc1f0cd69..33cf23f28 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -18,6 +18,7 @@ import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { BRIEF_ROLES, BriefError, briefFile, promptHash, renderBrief, sectionTexts, type BriefRole, type Slots } from '../lib/review-brief'; import { collect, readJsonl, readLedger, readerKey, recordJudgment, recordSent, appendEvent, verify, type JudgedLine, type MergedLine, type VerifyRow } from '../lib/review-ledger'; import { TARGETED_RULES, parseClassList, targetedBriefSha, targetedPatchId } from '../lib/review-targeted'; +import { resolveBaseBranch } from '../lib/ship-policy'; const HELP = `gstack-review-dispatch — verbatim reviewer briefs and loss-free collection for /ship @@ -27,7 +28,8 @@ const HELP = `gstack-review-dispatch — verbatim reviewer briefs and loss-free --learnings-file --checklist (default /review/specialists/.md; red-team.md for red-team) --checklists (specialist-batched: the plan's checklists= names, or paths) --findings-file --specialists (red-team) --fix-list --hunks --tests [--callers ] (fix-check) - --classes (targeted: the plan's TARGETED classes; the brief is review/specialists/targeted.md) + --classes (targeted: the plan's TARGETED classes; the brief is review/specialists/targeted.md; + --diff-base must be the merge base with --base, default origin/HEAD's branch, on every pass) writes /[-].prompt.md, records it in the ledger (default /review-ledger.jsonl) and prints BRIEF: reader= sha256= bytes= file=. Pass that file's content as the reader's prompt, unchanged. sent --ledger --role [--name ] --sent @@ -61,6 +63,11 @@ function render(): number { const checklist = flag('--checklist') ?? (role === 'red-team' ? path.join(ROOT, 'review/specialists/red-team.md') : role === 'targeted' ? path.join(ROOT, 'review/specialists/targeted.md') : role === 'specialist' && name ? path.join(ROOT, 'review/specialists', `${name}.md`) : undefined); if (checklist && (role === 'specialist' || role === 'red-team' || role === 'targeted') && !fs.existsSync(checklist)) throw new BriefError(`checklist not found: ${checklist}`); const brief = role === 'targeted' ? readFile(checklist, 'targeted brief') : undefined; + if (role === 'targeted') { + const base = resolveBaseBranch(process.cwd(), flag('--base')); + const mb = [`origin/${base}`, base].map(ref => spawnSync('git', ['merge-base', ref, 'HEAD'], { encoding: 'utf8', timeout: 30_000 })).find(r => r.status === 0)?.stdout.trim(); + if (!diffBase || diffBase !== mb) throw new BriefError(`the targeted reader reads the whole diff on every pass: --diff-base must be the merge base with ${base} (${mb ?? 'unresolved'}), got ${diffBase ?? 'none'}`); + } const checklists = flag('--checklists')?.split(',').map(s => s.trim()).filter(Boolean).map(c => (c.includes('/') ? c : path.join(ROOT, 'review/specialists', `${c}.md`))); for (const c of checklists ?? []) if (!fs.existsSync(c)) throw new BriefError(`checklist not found: ${c}`); let diff: string | undefined; diff --git a/docs/review-policy.md b/docs/review-policy.md index 3f6dbb3ff..7165acb73 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -313,7 +313,9 @@ full review. The plan prints `TARGETED: classes= rules= brief= targeted_patch_id=` and, when a class hit, -`DISPATCH: targeted targeted effort=full`. After a fix, pass +`DISPATCH: targeted targeted effort=full`. The targeted reader always reads the whole diff: `render --role targeted` +refuses a `--diff-base` other than the merge base, so on pass 2 and later it +gets the merge base, not `RANGE_BASE`. After a fix, pass `--targeted-ran ` from the line the reader ran on: the same committed patch skips the reader, and a changed one, uncommitted edits or `none` dispatch it again. `targeted_patch_id` is the patch-id of the whole diff --git a/test/review-targeted.test.ts b/test/review-targeted.test.ts index 6f17fe38e..f86751c4e 100644 --- a/test/review-targeted.test.ts +++ b/test/review-targeted.test.ts @@ -263,6 +263,7 @@ describe('the targeted role end to end on a fixture repository', () => { expect(brief).toContain(`DIFF_BASE=${base} && git diff "$DIFF_BASE"`); expect(brief).toContain('Read the targeted brief above ('); expect(runBin(repo, 'gstack-review-dispatch', ['render', '--role', 'targeted', '--classes', 'cost', '--diff-base', base, '--out', path.join(repo.root, 'bad')]).err).toContain('BRIEF_RENDER_FAILED: slot classes must list'); + expect(runBin(repo, 'gstack-review-dispatch', ['render', '--role', 'targeted', '--classes', 'money', '--diff-base', head, '--out', path.join(repo.root, 'range')]).err).toContain(`the targeted reader reads the whole diff on every pass: --diff-base must be the merge base with main (${base})`); expect(runBin(repo, 'gstack-review-dispatch', ['sent', '--ledger', ledger, '--role', 'targeted', '--sent', file]).out).toContain('BRIEF_SENT: reader=targeted'); const output = path.join(repo.root, 'targeted.out'); const finding = { severity: 'CRITICAL', confidence: 8, path: 'src/charge.ts', line: 1, category: 'money', summary: 'cap is never enforced', fingerprint: 'src/charge.ts:1:money', specialist: 'targeted' }; @@ -281,6 +282,9 @@ describe('the targeted role end to end on a fixture repository', () => { repo.git('commit', '-qam', 'later product change the reader never read'); expect(runBin(repo, 'gstack-review-dispatch', ['verify', '--ledger', ledger]).out).toContain('TARGETED_RECORD: classes=money status=failed findings=0'); repo.git('reset', '-q', '--hard', head); + fs.writeFileSync(path.join(repo.work, 'src', 'charge.ts'), 'export const spend_cap_usd = 7;\n'); + expect(runBin(repo, 'gstack-review-dispatch', ['verify', '--ledger', ledger]).out).toContain('TARGETED_RECORD: classes=money status=failed findings=0'); + repo.git('checkout', '--', 'src/charge.ts'); expect(runBin(repo, 'gstack-review-dispatch', ['verify', '--ledger', ledger]).out).toContain('TARGETED_RECORD: classes=money status=done findings=1'); const body = path.join(repo.root, 'body.md');