diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index e933f632b..b1c20a026 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -70,10 +70,12 @@ const HELP = `gstack-review-dispatch — verbatim reviewer briefs and loss-free review_exclude_paths prints REVIEW_EXCLUDE_RECORD: digest= (the selection every reader's newest brief names: the value gstack-ship-receipt write --review-exclude takes), and a mixed set of digests makes it dirty pass --base --out --pass [--ledger ] [--diff-base ] [--learnings-file ] [--extras ] [--audit-only] - [--checklist-root ] [--fix-list --hunks --tests [--callers ]] [-- ] + [--voices] [--checklist-root ] [--fix-list --hunks --tests [--callers ]] [-- ] runs gstack-review-plan and writes /pass-manifest.json: stage first (audit readers marked first in --extras), specialists (with the other --extras readers), red-team, voices (Step 11). Renders first and specialists now; prints the plan, MANIFEST: and one READER: line per reader. A plan whose NEXT is not full, scoped or fix-check prints PASS: none. + --voices (Step 11 on its own --out) keeps only the plan's voices, so it renders nothing its program never sends; it + takes no --extras, and a reviewer pass refuses to resume its manifest. is letters, digits and dashes, read as lowercase (P1 is p1): the manifest, reader titles and a resume use that form. A fix-check reader (stage specialists) renders from decision 3.5's --fix-list, --hunks, --tests and --callers files. --diff-base sets every reader's base, a scoped pass's Red Team included (the targeted reader keeps the merge base). @@ -255,6 +257,7 @@ function pass(): number { if (fs.existsSync(file)) { const m = readManifest(file); if (m.pass !== id) throw new BriefError(`${file} belongs to pass ${m.pass}, not ${id}`); + if (m.voices_only && !args.includes('--voices')) throw new BriefError(`${file} holds only Step 11's voices (pass --voices); a reviewer pass needs its own --out`); process.stdout.write([...m.plan_lines, `MANIFEST: file=${file} pass=${m.pass} readers=${m.readers.length} resumed`, ...m.readers.map(r => readerLine(m, r))].join('\n') + '\n'); return EXIT.ok; } @@ -264,7 +267,7 @@ function pass(): number { if (!extrasFile) { process.stderr.write('gstack-review-dispatch pass --audit-only: needs --extras (the audit readers, e.g. Step 14.5\'s docs reader)\n'); return EXIT.usage; } const extras = JSON.parse(readFile(extrasFile, 'extras')!) as Extra[]; const m = buildManifest({ pass: id, out, ledger: flag('--ledger') ?? path.join(out, 'review-ledger.jsonl'), base, diffBase: mergeBaseWith(base) ?? 'HEAD', root: git(['rev-parse', '--show-toplevel'])?.trim() ?? process.cwd(), - checklistRoot: flag('--checklist-root') ?? ROOT, dispatches: [], next: 'audit', planLines: ['PASS: audit-only (no reviewer plan; its extras run as one batch)'], extras }); + checklistRoot: flag('--checklist-root') ?? ROOT, dispatches: [], next: 'audit', planLines: ['PASS: audit-only (no reviewer plan; its extras run as one batch)'], extras, voices: args.includes('--voices') }); fs.mkdirSync(m.out, { recursive: true, mode: 0o700 }); const t = sectionTexts(ROOT); renderStage(m, 'first', t, branchDiff); @@ -295,7 +298,7 @@ function pass(): number { dispatches: step === 'fix-check' ? p.dispatches : p.dispatches.filter(d => d.role !== 'fix-check'), next: step, planLines, learnings: readFile(flag('--learnings-file'), 'learnings') ?? null, extras, targetedClasses: p.targeted?.classes, mergeBase, rangeBase: flag('--diff-base') ? undefined : rangeBase, fixCheck: step === 'fix-check' && (flag('--fix-list') || flag('--hunks') || flag('--tests')) ? { fix_list: flag('--fix-list')!, hunks: flag('--hunks')!, tests: flag('--tests')!, callers: flag('--callers') } : undefined, - ...planExclude(p.review_exclude), ...(p.readers ? { readers: p.readers } : {}) }); + ...planExclude(p.review_exclude), ...(p.readers ? { readers: p.readers } : {}), voices: args.includes('--voices') }); const over = ceiling('gstack-review-dispatch pass', base, m.out, m.readers, m.diff_base, (m.review_exclude as PlanExclude) ?? null); if (over) { process.stdout.write(planLines.join('\n') + '\n'); process.stderr.write(over.line + '\n'); return over.exit; } fs.mkdirSync(m.out, { recursive: true, mode: 0o700 }); diff --git a/changelog.d/garrytan-gst-1368-voices-pass-renders-only-voices.md b/changelog.d/garrytan-gst-1368-voices-pass-renders-only-voices.md new file mode 100644 index 000000000..ffbaf5338 --- /dev/null +++ b/changelog.d/garrytan-gst-1368-voices-pass-renders-only-voices.md @@ -0,0 +1 @@ +- **Step 11's review voices no longer leave an unsent brief behind (GST-1368).** When /ship ran Step 11 on its own output folder, which happens after a fix round moves the head, `gstack-review-dispatch pass` built the manifest from the whole pass-1 plan and wrote the specialist brief into the ledger, but the voices program never sends it. `gstack-review-dispatch verify` then failed the run with `BRIEF specialist-batched NOT_SENT`. Step 11 now asks for `pass --voices`, which keeps only the plan's voices, so nothing is rendered that no reader gets. `--voices` refuses `--extras`, and a reviewer pass refuses to resume a voices-only manifest. A ledger written before this fix still verifies dirty; rerun Step 11 with a fresh `--out` and `--pass` to get a clean one. diff --git a/docs/review-policy.md b/docs/review-policy.md index e0761fbc4..70bcd3aad 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -825,7 +825,9 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: checked-in program `lib/capy-review-pass.js`, served by `program` and loaded unchanged by the gstack Capy skill. A full repeat pass takes `--diff-base` (RANGE_BASE; a scoped pass, none), a fix-check plan renders its reader from `--fix-list`, `--hunks`, `--tests` and `--callers`, - `--audit-only` runs just the `--extras` readers (Step 14.5's docs audit), and the fork + `--audit-only` runs just the `--extras` readers (Step 14.5's docs audit), `--voices` + keeps only the plan's voices for Step 11 on its own `--out` (it takes no `--extras`, and a + reviewer pass refuses to resume that manifest, GST-1368), and the fork point is found against `origin/` first, as preflight does. 7. Every brief, audit readers' included, opens with a fingerprint step and ends with a workspace paragraph naming the checkout the bins run in (GST-482): a reader on a shared diff --git a/lib/capy-review-pass.js b/lib/capy-review-pass.js index 192087edd..0fe143498 100644 --- a/lib/capy-review-pass.js +++ b/lib/capy-review-pass.js @@ -56,6 +56,7 @@ if (input.extras && typeof input.extras !== 'string') { if (input.learnings) passCmd.push('--learnings-file', q(input.learnings)); if (input.diffBase) passCmd.push('--diff-base', q(input.diffBase)); if (input.step === '14.5') passCmd.push('--audit-only'); +if (input.stage === 'voices') passCmd.push('--voices'); for (const [k, v] of Object.entries(input.fixCheck || {})) passCmd.push('--' + k.replace('_', '-'), q(v)); passCmd.push('--', ...(input.planFlags || []).map(q)); const pr = await sh(passCmd.join(' ')); diff --git a/lib/review-pass.ts b/lib/review-pass.ts index 41ad9fbca..00b14d186 100644 --- a/lib/review-pass.ts +++ b/lib/review-pass.ts @@ -56,6 +56,8 @@ export interface Manifest { review_exclude?: Record; disclosure?: string; /** Decision 3.5's files for a fix-check reader: the fix list, touched hunks, test results and callers. */ fix_check?: FixCheckFiles; + /** Step 11's own pass (`pass --voices`): the plan's voices only, so nothing is rendered that the voices program never sends. */ + voices_only?: true; /** The plan's readers policy (lib/review-fetch.ts): what every brief says readers may not fetch or open; absent means the default rule. */ fetch_policy?: EffectiveReaders; } @@ -93,16 +95,18 @@ const shellQuote = (s: string) => `'${s.replace(/'/g, "'\\''")}'`; const safe = (reader: string) => reader.replace(/[^a-zA-Z0-9._-]/g, '_'); const STAGE_OF: Record = { specialist: 'specialists', 'fix-check': 'specialists', 'red-team': 'red-team', 'adversarial-native': 'voices', 'outside-voice': 'voices', targeted: 'voices' }; -/** The manifest for one pass from the plan's dispatches. A fix-check reader needs decision 3.5's files. */ +/** The manifest for one pass from the plan's dispatches. A fix-check reader needs decision 3.5's files. `voices` keeps only Step 11's readers. */ export function buildManifest(o: { pass: string; out: string; ledger: string; base: string; diffBase: string; root: string; checklistRoot: string; dispatches: Dispatch[]; next: string; planLines: string[]; learnings?: string | null; extras?: Extra[]; - targetedClasses?: readonly string[]; mergeBase?: string; rangeBase?: string; fixCheck?: FixCheckFiles; reviewExclude?: Record; disclosure?: string; readers?: EffectiveReaders; + targetedClasses?: readonly string[]; mergeBase?: string; rangeBase?: string; fixCheck?: FixCheckFiles; reviewExclude?: Record; disclosure?: string; readers?: EffectiveReaders; voices?: boolean; }): Manifest { + if (o.voices && o.extras?.length) throw new BriefError(`Step 11's voices pass sends no audit reader, so it takes no --extras: ${o.extras.map(e => e.name).join(', ')}`); const readers: PassReader[] = []; for (const d of o.dispatches) { const stage = STAGE_OF[d.role]; if (!stage) throw new BriefError(`a pass dispatches no ${d.role} reader`); + if (o.voices && stage !== 'voices') continue; if (d.role === 'fix-check' && !o.fixCheck) throw new BriefError('the plan dispatches a fix-check: pass needs --fix-list, --hunks and --tests (decision 3.5\'s files; --callers optional)'); const role: BriefRole = d.role === 'specialist' && d.checklists ? 'specialist-batched' : d.role as BriefRole; const name = role === 'specialist' ? d.name : undefined; @@ -128,7 +132,7 @@ export function buildManifest(o: { return { version: 1, pass: o.pass, out, ledger: path.resolve(o.ledger), merged: path.join(out, 'merged.jsonl'), base: o.base, diff_base: o.diffBase, stack: detectStack(o.root), test_fw: detectTestFw(o.root), learnings: o.learnings ?? null, checklist_root: o.checklistRoot, - next: o.next, plan_lines: o.planLines, readers, ...(o.reviewExclude ? { review_exclude: o.reviewExclude } : {}), ...(o.disclosure ? { disclosure: o.disclosure } : {}), ...(o.readers ? { fetch_policy: o.readers } : {}), + next: o.next, plan_lines: o.planLines, readers, ...(o.voices ? { voices_only: true as const } : {}), ...(o.reviewExclude ? { review_exclude: o.reviewExclude } : {}), ...(o.disclosure ? { disclosure: o.disclosure } : {}), ...(o.readers ? { fetch_policy: o.readers } : {}), ...(o.fixCheck ? { fix_check: Object.fromEntries(Object.entries(o.fixCheck).filter(([, v]) => v).map(([k, v]) => [k, path.resolve(v as string)])) as unknown as FixCheckFiles } : {}), }; } diff --git a/test/review-pass.test.ts b/test/review-pass.test.ts index e4b941d30..f039f9ba9 100644 --- a/test/review-pass.test.ts +++ b/test/review-pass.test.ts @@ -359,6 +359,34 @@ describe('Capy pass program (fake host, real bins)', () => { expect(Object.keys(m.log_starts ?? {}).sort()).toEqual(['pass:review', 'voices:adversarial-review']); }, 240_000); + // Value: protects=Step 11 on its own --out (the head moved after a fix round) renders and sends only the voices, so its ledger verifies clean; fails_when=pass renders the plan's first and specialists stages for a voices-only program, leaving BRIEF specialist-batched NOT_SENT in the ledger; + // why_new=GST-1368 (gstack-evals#208 at fb407fb: VERIFY: dirty briefs=4 outputs=3); seam=none; no_claim=which plan flags Step 11 takes + test('Step 11 on a fresh --out renders only the voices, and its ledger verifies clean', async () => { + const { dir, env } = repo(); + const out = path.join(dir, '.state/run/s11'); + const h = host(dir, env); + const models = { 'outside-voice': { model_id: 'openai/gpt-6.1-sol' } }; + const res = await runPass(h, { base: 'main', out, pass: 'x7s11', planFlags: ['--depth-floor', 'full'], stage: 'voices', models }); + expect(res).toContain('NEXT: judge the voices'); + const m = JSON.parse(fs.readFileSync(path.join(out, 'pass-manifest.json'), 'utf8')) as Manifest; + fs.writeFileSync(path.join(out, 'judged.jsonl'), ''); + expect(bin(dir, env, ['judge', '--ledger', m.ledger, '--merged', m.merged, '--judged', path.join(out, 'judged.jsonl')]).status).toBe(0); + const v = bin(dir, env, ['verify', '--ledger', m.ledger]); + expect(v.stdout).not.toContain('NOT_SENT'); + expect(v.stdout).toContain('VERIFY: clean briefs=' + m.readers.length); + expect(m.readers.length).toBeGreaterThan(0); + expect(m.readers.every(r => r.stage === 'voices' && r.state === 'collected')).toBe(true); + expect(h.creates.map(c => c.title).sort()).toEqual(m.readers.map(r => r.title).sort()); + const rendered = fs.readFileSync(m.ledger, 'utf8').trim().split('\n').map(l => JSON.parse(l)).filter(e => e.event === 'rendered').map(e => e.reader); + expect(rendered.sort()).toEqual(m.readers.map(r => r.reader).sort()); + const reviewer = bin(dir, env, ['pass', '--base', 'main', '--out', out, '--pass', 'x7s11', '--', '--depth-floor', 'full']); + expect([reviewer.status === 0, reviewer.stderr]).toEqual([false, expect.stringContaining("holds only Step 11's voices")]); + const extras = path.join(dir, '.state/extras.json'); + fs.writeFileSync(extras, JSON.stringify([{ name: 'plan', prompt_file: path.join(dir, 'package.json') }])); + const audit = bin(dir, env, ['pass', '--base', 'main', '--out', path.join(dir, '.state/run/s11x'), '--pass', 'x7s11x', '--voices', '--extras', extras, '--', '--depth-floor', 'full']); + expect([audit.status === 0, audit.stderr]).toEqual([false, expect.stringContaining("Step 11's voices pass sends no audit reader")]); + }, 240_000); + test('Step 11 voices run from the same manifest; the outside voice needs a model of another family', async () => { const { dir, env } = repo(); const out = path.join(dir, '.state/run/p1');