diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index 4dddc1222..82e0cdbc9 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -25,7 +25,8 @@ import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { BRIEF_ROLES, BriefError, briefFile, checkReviewedTree, checkoutRoot, checkoutTree, promptHash, renderBrief, sectionTexts, sliceDiff, sliceNote, withWorkspace, type BriefRole, type Slots, type TreeCheck } from '../lib/review-brief'; import { cleanMarkers, collect, fixableRaises, mayBeUnavailable, parseFindings, readJsonl, readLedger, readerKey, recordJudgment, recordSent, appendEvent, verify, type JudgedLine, type MergedLine, type VerifyRow } from '../lib/review-ledger'; import { buildManifest, chunkCount, chunkOf, manifestPath, markReader, readManifest, renderStage, withAuditModel, writeManifest, STAGES, type Extra, type Manifest, type PassReader, type ReaderState, type Stage } from '../lib/review-pass'; -import type { Dispatch } from '../lib/review-depth'; +import { diffCommand, type Dispatch } from '../lib/review-depth'; +import { mayExclude, policySelection, selectFor } from '../lib/review-selection'; import { REVIEW_PLAN_TIMEOUT_MS } from '../lib/review-collect'; import { resultLine } from '../lib/result-codes'; import { runScriptSync } from '../lib/script-invocation'; @@ -146,13 +147,17 @@ function render(): number { 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; - const keepItems = parseTail(flag('--keep')).filter(s => s !== '--'); - const policyItems = parseTail(flag('--exclude')).filter(s => s.startsWith(':(top,exclude,glob)')); + const readers = readersAt(checkoutRoot(), flag('--base')); const resolvable = !!diffBase && git(['rev-parse', '--verify', '--quiet', `${diffBase}^{commit}`]) !== null; - if (role !== 'fix-check' && diffBase && (resolvable || role === 'outside-voice')) { - const over = ceiling('gstack-review-dispatch render', resolveBaseBranch(process.cwd(), flag('--base')), out, [{ reader: readerKey(role, name), exclude: flag('--exclude'), keep: keepItems }], diffBase, undefined); + const reads = role !== 'fix-check' && !!diffBase && (resolvable || role === 'outside-voice'); + const base = reads ? resolveBaseBranch(process.cwd(), flag('--base')) : undefined; + const sel = base && diffBase ? standaloneSelection(readerKey(role, name), base, diffBase, readers) : { reader: readerKey(role, name), exclude: flag('--exclude'), keep: parseTail(flag('--keep')).filter(s => s !== '--'), disclosure: flag('--disclosure') }; + const policyItems = parseTail(sel.exclude).filter(s => s.startsWith(':(top,exclude,glob)')); + if (base && diffBase) { + const over = ceiling('gstack-review-dispatch render', base, out, [sel], diffBase, undefined); if (over) { process.stderr.write(over.line + '\n'); return over.exit; } - if (role === 'outside-voice') diff = branchDiff(diffBase, { exclude: flag('--exclude'), keep: keepItems }); + settleExcerpt(sel, diffBase, path.resolve(out)); + if (role === 'outside-voice') diff = branchDiff(diffBase, sel); } let diffFile: Slots = {}; if (diff !== undefined) { @@ -163,17 +168,17 @@ function render(): number { diffFile = { diff_file: f, diff_bytes: String(s.bytes), diff_sha256: createHash('sha256').update(s.text).digest('hex'), ...(s.sliced ? { diff_slice: sliceNote(s, REVIEW_DIFF_MAX_BYTES) } : {}) }; } const slots: Slots = { - name, diff_base: diffBase, exclude: flag('--exclude'), stack: flag('--stack'), test_fw: flag('--test-fw'), + name, diff_base: diffBase, exclude: sel.exclude, stack: flag('--stack'), test_fw: flag('--test-fw'), learnings: readFile(flag('--learnings-file'), 'learnings'), checklist, checklists: checklists?.join(','), findings: readFile(flag('--findings-file'), 'findings'), specialists: flag('--specialists'), ...diffFile, classes: flag('--classes'), brief, ...(merged ? { checklist: path.join(ROOT, 'review/specialists/targeted.md') } : {}), - policy_exclude: policyItems.map(quote).join(' ') || undefined, ...(flag('--disclosure') ? { disclosure: flag('--disclosure'), keep: keepItems.map(quote).join(' ') || undefined } : {}), + policy_exclude: policyItems.map(quote).join(' ') || undefined, ...(sel.disclosure ? { disclosure: sel.disclosure, keep: sel.keep?.map(quote).join(' ') || undefined } : {}), + ...(sel.disclosure && 'excerpt_file' in sel && sel.excerpt_file ? { excerpt_file: sel.excerpt_file.file, excerpt_bytes: String(sel.excerpt_file.bytes), excerpt_sha256: sel.excerpt_file.sha256, excerpt_files: String(sel.excerpt_file.files) } : {}), fix_list: readFile(flag('--fix-list'), 'fix list'), hunks: readFile(flag('--hunks'), 'hunks'), tests: readFile(flag('--tests'), 'tests'), callers: readFile(flag('--callers'), 'callers'), }; const workdir = checkoutRoot(); const tree = checkoutTree(workdir); if (!tree) throw new BriefError(`cannot read HEAD^{tree} and the toplevel of ${workdir}; a brief names the checkout its reply must prove`); - const readers = readersAt(workdir, flag('--base')); const text = withWorkspace(renderBrief(role, slots, sectionTexts(ROOT)), workdir, readers); fs.mkdirSync(out, { recursive: true, mode: 0o700 }); const file = path.join(path.resolve(out), briefFile(role, name)); @@ -181,7 +186,7 @@ function render(): number { const ledger = flag('--ledger') ?? path.join(out, 'review-ledger.jsonl'); const sha = promptHash(text); const targeted = brief === undefined ? {} : { classes: parseClassList(flag('--classes')!), rules: TARGETED_RULES, brief_sha: targetedBriefSha(merged ? path.join(ROOT, 'review/specialists/targeted.md') : checklist), diff_base: diffBase ?? null, ...(diffBase ? targetedRead(process.cwd(), diffBase) : { patch_id: 'none', head: null }) }; - appendEvent(ledger, { event: 'rendered', reader: readerKey(role, name), role, name: name ?? null, file, sha256: sha, bytes: Buffer.byteLength(text), workdir, tree, ...targeted, ...(diffFile.diff_file ? { payload: { file: diffFile.diff_file, sha256: diffFile.diff_sha256 } } : {}), ...(disclosureDigest(flag('--disclosure')) ? { exclude: disclosureDigest(flag('--disclosure')) } : {}), readers: readers.digest }); + appendEvent(ledger, { event: 'rendered', reader: readerKey(role, name), role, name: name ?? null, file, sha256: sha, bytes: Buffer.byteLength(text), workdir, tree, ...targeted, ...(diffFile.diff_file ? { payload: { file: diffFile.diff_file, sha256: diffFile.diff_sha256 } } : {}), ...(disclosureDigest(sel.disclosure) ? { exclude: disclosureDigest(sel.disclosure) } : {}), readers: readers.digest }); process.stdout.write(`BRIEF: reader=${readerKey(role, name)} sha256=${sha} bytes=${Buffer.byteLength(text)} file=${file}\n`); return EXIT.ok; } @@ -286,7 +291,7 @@ const readerLine = (m: Manifest, r: PassReader) => `READER: ${r.reader} stage=${ const selections = new Map(); const selectionOf = (base: string, exclude: string | undefined, keep: readonly string[] = [], excerpt: readonly string[] = []) => { const key = JSON.stringify([base, exclude ?? '', keep, excerpt]); - if (!selections.has(key)) selections.set(key, readSelection(process.cwd(), base, parseTail(exclude), keep, excerpt)); + if (!selections.has(key)) selections.set(key, readSelection(checkoutRoot(), base, parseTail(exclude), keep, excerpt)); return selections.get(key)!; }; const branchDiff = (base: string, r?: Pick) => { @@ -312,20 +317,41 @@ function ceiling(context: string, base: string, outDir: string, readers: Readonl * hash); a file the excerpt could not be built for moves to the reader's keep list, so its whole diff reaches the reader. */ function settleExcerpts(m: Manifest): void { - for (const r of m.readers) { - if (!r.excerpt?.length) continue; - const d = selectionOf(r.diff_base ?? m.diff_base, r.exclude, r.keep, r.excerpt); - if (!d.ok) throw new BriefError(`git diff ${r.diff_base ?? m.diff_base} failed for ${r.reader}'s secret-line excerpt`); - const whole = r.excerpt.filter(s => d.whole?.includes(s)); - if (whole.length) r.keep = [...(r.keep ?? []), ...whole]; - r.excerpt = r.excerpt.filter(s => !whole.includes(s)); - if (!r.excerpt.length || !d.excerpt) { delete r.excerpt; continue; } - const sha256 = createHash('sha256').update(d.excerpt).digest('hex'); - const file = path.join(m.out, 'briefs', `secret-excerpt-${sha256.slice(0, 16)}.md`); - fs.mkdirSync(path.dirname(file), { recursive: true, mode: 0o700 }); - fs.writeFileSync(file, d.excerpt, { mode: 0o600 }); - r.excerpt_file = { file, sha256, bytes: Buffer.byteLength(d.excerpt), files: r.excerpt.length }; + for (const r of m.readers) settleExcerpt(r, r.diff_base ?? m.diff_base, path.join(m.out, 'briefs')); +} +function settleExcerpt(r: Pick, diffBase: string, dir: string): void { + if (!r.excerpt?.length) return; + const d = selectionOf(diffBase, r.exclude, r.keep, r.excerpt); + if (!d.ok) throw new BriefError(`git diff ${diffBase} failed for ${r.reader}'s secret-line excerpt`); + const whole = r.excerpt.filter(s => d.whole?.includes(s)); + if (whole.length) r.keep = [...(r.keep ?? []), ...whole]; + r.excerpt = r.excerpt.filter(s => !whole.includes(s)); + if (!r.excerpt.length || !d.excerpt) { delete r.excerpt; return; } + const sha256 = createHash('sha256').update(d.excerpt).digest('hex'); + const file = path.join(dir, `secret-excerpt-${sha256.slice(0, 16)}.md`); + fs.mkdirSync(path.dirname(file), { recursive: true, mode: 0o700 }); + fs.writeFileSync(file, d.excerpt, { mode: 0o600 }); + r.excerpt_file = { file, sha256, bytes: Buffer.byteLength(d.excerpt), files: r.excerpt.length }; +} + +/** + * A standalone render's reader with the selection the pass gives its readers (GST-1476): the base policy's review_exclude + * from gstack-review-plan --json (only when the base may declare one), merged into --exclude and --keep by selectFor, and its + * disclosure line unless --disclosure names one. A plan that fails stops the render: no brief without its selection. + */ +function standaloneSelection(reader: string, base: string, diffBase: string, readers: EffectiveReaders): Pick & { diff_base: string; disclosure?: string } { + let x: (Record & { state: string }) | null = null; + if (mayExclude(process.cwd(), base)) { + const tail = args.includes('--') ? args.slice(args.indexOf('--') + 1) : []; + const p = runScriptSync(path.join(ROOT, 'bin', 'gstack-review-plan'), ['--base', base, ...tail, '--json'], { encoding: 'utf8', timeout: REVIEW_PLAN_TIMEOUT_MS, maxBuffer: 64 * 1024 * 1024 }); + if (p.status !== 0) throw new BriefError(`gstack-review-plan --base ${base} --json exited ${p.status ?? p.signal}, so the base policy's review selection is unknown and no brief is rendered without it: ${(p.stderr ?? '').trim().slice(-400)}`); + x = (JSON.parse(p.stdout) as { review_exclude?: Record & { state: string } }).review_exclude ?? null; } + const given = parseTail(flag('--exclude')).filter(s => s !== '--' && s !== '.'); + const s = selectFor({ exclude: given, keep: parseTail(flag('--keep')).filter(k => k !== '--') }, policySelection(x, readers)); + const exclude = s.exclude.length === given.length ? flag('--exclude') : diffCommand({ exclude: s.exclude } as Dispatch).slice('git diff '.length); + const disclosure = flag('--disclosure') ?? (x?.state === 'active' ? planExclude(x).disclosure : undefined); + return { reader, diff_base: diffBase, exclude, keep: (s.keep ?? []).filter(k => !s.excerpt?.includes(k)), ...(s.excerpt ? { excerpt: s.excerpt } : {}), ...(disclosure ? { disclosure } : {}) }; } function pass(): number { diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 0638d0e1b..b5c87d80c 100755 --- a/bin/gstack-review-plan +++ b/bin/gstack-review-plan @@ -35,7 +35,8 @@ import { isUnavailable } from '../lib/ship-policy'; import { reviewPolicy } from '../lib/ship-policy-review'; import { platformReviewAt } from '../lib/review-platform'; import { loadPolicyAt } from '../lib/ship-policy-at'; -import { forbidSpecs, forbiddenPath, loadReaders, readersLine } from '../lib/review-fetch'; +import { loadReaders, readersLine } from '../lib/review-fetch'; +import { policySelection, selectFor } from '../lib/review-selection'; import { effectiveReaders, READERS_POLICY_DEFAULT } from '../lib/ship-policy-readers'; import { classifyDiff, diffCommand, keepCommand, planReview, reviewDepthLine, reviewPlanLine, scanNumbers, SCOPED_RANGE_LINES, type Depth, type History } from '../lib/review-depth'; import { classList, targetedBriefSha, targetedClasses, targetedPatchId } from '../lib/review-targeted'; @@ -219,14 +220,8 @@ function main(): number { criticalFound: has('--critical-found'), adversarialRan: has('--adversarial-ran'), reviewedNumbers: flag('--reviewed-numbers'), history: { fullPasses: pass, last, fixedBlocking: has('--fixed-blocking'), fixedBelow: has('--fixed-below') && last !== 'fix-check', blockingOpen: has('--blocking-open'), touchedGate: has('--touched-gate') || !!fixRange?.touched_gate, pass1Depth, rangeRuntimeLines: fixRange?.runtime_lines, granted: grant, fixChecks: has('--fix-checks') ? fixChecks : undefined }, }); - const forbidden = readers.forbid_paths.map(g => new Bun.Glob(g.replace(/\/+$/, ''))); - for (const d of plan.dispatches) { - if (d.role === 'fix-check') continue; - d.exclude.push(...forbidSpecs(readers).filter(s => !d.exclude.includes(s))); - if (d.keep && forbidden.length) d.keep = d.keep.filter(k => !forbiddenPath(forbidden, k.replace(/^:\(top,literal\)/, ''))); - const excerpt = active?.excerpt.filter(k => d.keep?.includes(k)) ?? []; - if (excerpt.length) Object.assign(d, { excerpt }); - } + const selection = policySelection(active && { state: 'active', specs: active.specs, keep: active.keep, excerpt: active.excerpt }, readers); + plan.dispatches.forEach((d, i) => { if (d.role !== 'fix-check') plan.dispatches[i] = selectFor(d, selection); }); const gateScan = { tagged: gateTagged, head }; if (has('--json')) { const { runtimeLines, stepDepth, depthReasons, ...rest } = plan; diff --git a/changelog.d/capy-every-reader-path-standalone.md b/changelog.d/capy-every-reader-path-standalone.md new file mode 100644 index 000000000..65337eb93 --- /dev/null +++ b/changelog.d/capy-every-reader-path-standalone.md @@ -0,0 +1 @@ +- **Every reviewer reads the same trimmed diff, however its brief was rendered.** A brief rendered on its own with `gstack-review-dispatch render` (the Red Team rendered before the specialists finished, or rendered again after a failed dispatch) now gets the base policy's `review_exclude_paths` selection, the readers policy's `forbid_paths`, the kept files, the secret-line excerpts and the disclosure line, the same as the pass's readers. Before, it built its diff from its own flags alone, so harness-evals #269's Red Team refused at the unexcluded 30.3 MB while the other readers read 2 MB. The plan, the pass manifest and the standalone render now share one selection helper (`lib/review-selection.ts`), and the selection is read from the checkout root, so an outside voice rendered from a subdirectory no longer drops the changes outside that directory. (GST-1476) diff --git a/docs/review-policy.md b/docs/review-policy.md index 058030698..f4eac9b17 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -782,7 +782,8 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: 1. `render --role [--name ] --out ` cuts the brief out of `ship/sections/review-army.md` or `ship/sections/adversarial.md` (the fix-check from `scripts/resolvers/fix-check.ts`) and fills only declared slots: the - literal diff base, the plan's `DIFF_CMD` excludes, stack, test framework, + literal diff base, the plan's `DIFF_CMD` excludes (with the base policy's + selection added, [as the pass's readers get it](ship-policy.md#one-selection-every-reader)), stack, test framework, learnings, checklist paths, the Red Team's merged findings and specialist count, and the outside voice's branch diff. A section that lost an anchor or placeholder refuses (`BRIEF_RENDER_FAILED`) instead of rendering something else, and so does diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 20a621f68..e82521484 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1465,6 +1465,21 @@ whole through its keep command. Results PRs that capture model requests need this: their system prompts name `${…_API_KEY}` variables and keychain commands (harness-evals #269: 75 such files, 12.9 MB whole). + +**One selection for every reader (GST-1476).** Every path that renders a +reader's brief builds its diff from the same selection: the policy's +`:(top,exclude,glob)` items, the readers policy's `forbid_paths`, the kept files +and the secret-line excerpts. That covers the pass manifest's stages, a fresh +pass after a failed reader, and a standalone `gstack-review-dispatch render` (a +reader rendered on its own, or rendered again after a failed dispatch). A +standalone render reads the selection from `gstack-review-plan --json` when the +base may declare `review_exclude_paths`, adds whatever its `--exclude` and +`--keep` lack, writes the excerpt beside the brief and carries the disclosure +line, so its ceiling check counts the same bytes as the pass's readers. A plan +that fails stops the render. Before this, the Red Team's own render on +harness-evals #269 counted the whole 30.3 MB diff while the pass's readers read +2 MB. + **Depth, numbers and the targeted pass.** Excluded paths count as docs for the review depth everywhere it is computed (`gstack-review-plan`, the receipt writer, carry and the queue), and their lines never trigger the numeric-claim diff --git a/lib/review-pass.ts b/lib/review-pass.ts index 5bbf4b22f..4c8cae78b 100644 --- a/lib/review-pass.ts +++ b/lib/review-pass.ts @@ -18,6 +18,7 @@ import { AgentModelError, resolveAgentModel } from './agent-model'; import { BriefError, briefFile, checkoutRoot, checkoutTree, promptHash, renderBrief, sliceDiff, sliceNote, withWorkspace, type BriefRole, type SectionTexts, type Slots } from './review-brief'; import { REVIEW_DIFF_MAX_BYTES, disclosureDigest } from './review-exclude'; import { appendEvent, readerKey } from './review-ledger'; +import { policySelection, selectFor } from './review-selection'; import { diffCommand, type Dispatch } from './review-depth'; import { TARGETED_RULES, parseClassList, targetedBriefSha, targetedRead } from './review-targeted'; import type { FetchHit } from './review-fetch'; @@ -135,7 +136,9 @@ export function buildManifest(o: { }): 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 selection = policySelection(o.reviewExclude, o.readers); + for (const planned of o.dispatches) { + const d = planned.role === 'fix-check' ? planned : selectFor(planned, selection); const stage = STAGE_OF[d.role]; if (!stage) throw new BriefError(`a pass dispatches no ${d.role} reader`); if (o.voices && stage !== 'voices') continue; diff --git a/lib/review-selection.ts b/lib/review-selection.ts new file mode 100644 index 000000000..557c7d8f8 --- /dev/null +++ b/lib/review-selection.ts @@ -0,0 +1,52 @@ +/** + * The one selection every reader's diff is built from (GST-1476, docs/ship-policy.md#review-exclude-paths): the base policy's + * review_exclude_paths specs, the readers policy's forbid_paths specs, the kept files and the secret-line excerpts (GST-1447). + * gstack-review-plan applies it to every dispatch, buildManifest applies it again to every pass reader, and a standalone + * gstack-review-dispatch render applies it to its one reader, so a first render, a re-render after a failed dispatch and a + * late stage all read the same diff (harness-evals #269: the Red Team's own render read 30.3 MB, the pass's readers 2 MB). + */ +import { spawnSync } from 'node:child_process'; +import { forbidSpecs, forbiddenPath } from './review-fetch'; +import { isUnavailable } from './ship-policy'; +import { loadPolicyAt } from './ship-policy-at'; +import type { EffectiveReaders } from './ship-policy-readers'; + +export interface PolicySelection { specs: string[]; keep: string[]; excerpt: string[]; forbidden: InstanceType[] } +/** The plan's review_exclude object (an active one carries specs, keep and excerpt) or the manifest's copy of it. */ +export type ExcludeSource = { state?: unknown; specs?: unknown; keep?: unknown; excerpt?: unknown } | null | undefined; + +const strings = (v: unknown): string[] => (Array.isArray(v) ? v.filter((s): s is string => typeof s === 'string') : []); + +export function policySelection(x: ExcludeSource, readers: Pick | null | undefined): PolicySelection { + const active = x?.state === 'active'; + const forbid = readers?.forbid_paths ?? []; + return { + specs: [...(active ? strings(x.specs) : []), ...forbidSpecs({ forbid_paths: forbid })], + keep: active ? strings(x.keep) : [], + excerpt: active ? strings(x.excerpt) : [], + forbidden: forbid.map(g => new Bun.Glob(g.replace(/\/+$/, ''))), + }; +} + +/** Whether 's policy may exclude anything: false only when it reads cleanly and declares no review_exclude_paths (an unreadable one goes to the plan, which refuses). */ +export function mayExclude(root: string, base: string): boolean { + const rev = (r: string) => spawnSync('git', ['rev-parse', '--verify', '-q', `${r}^{commit}`], { cwd: root, encoding: 'utf8', timeout: 30_000 }).stdout?.trim() || null; + const b = base.replace(/^origin\//, ''); + const ref = rev(`origin/${b}`) ? `origin/${b}` : b; + const sha = rev(ref); + if (!sha) return false; + const load = loadPolicyAt(root, sha, ref); + return isUnavailable(load) || load.policy.review_exclude_paths.length > 0; +} + +/** + * A reader's pathspecs with the selection applied: every spec it lacks appended, the kept files (its own first) less any + * forbidden path, and the excerpt files among them. keep still lists the excerpt files, as the plan's dispatches do. + */ +export function selectFor(d: D, s: PolicySelection): D { + const exclude = [...d.exclude, ...s.specs.filter(x => !d.exclude.includes(x))]; + const keep = [...new Set([...(d.keep ?? []), ...s.keep])].filter(k => !s.forbidden.length || !forbiddenPath(s.forbidden, k.replace(/^:\(top,literal\)/, ''))); + const excerpt = [...new Set([...(d.excerpt ?? []), ...s.excerpt])].filter(k => keep.includes(k)); + const { keep: _k, excerpt: _e, ...rest } = d; + return { ...rest, exclude, ...(keep.length ? { keep } : {}), ...(excerpt.length ? { excerpt } : {}) } as D; +} diff --git a/test/review-reader-selection.test.ts b/test/review-reader-selection.test.ts new file mode 100644 index 000000000..ae23551cb --- /dev/null +++ b/test/review-reader-selection.test.ts @@ -0,0 +1,185 @@ +/** + * One selection for every reader (GST-1476, docs/ship-policy.md#review-exclude-paths): the base policy's review_exclude_paths, + * the kept files and the GST-1447 secret-line excerpts reach every reader's diff, whichever path renders it: the pass + * manifest's stages, a fresh pass after a failed reader, or a standalone render (a first render, or a re-render after a + * failed dispatch). harness-evals #269: the Red Team's standalone render read the unexcluded 30.3 MB diff while the pass's + * other readers read the 2 MB selection. + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { EXIT } from '../lib/headless-artifacts'; +import { BRIEF_ROLES, type BriefRole } from '../lib/review-brief'; +import { ROLE_EFFORT, type Role } from '../lib/review-depth'; +import { REVIEW_DIFF_MAX_BYTES } from '../lib/review-exclude'; +import { buildManifest } from '../lib/review-pass'; +import { REVIEW_EXCLUDE_KEY_VERSION } from '../lib/ship-policy'; +import { cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture'; + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); +const SPEC = ':(top,exclude,glob)docs/benchmarks/**'; +const QUOTED = "'" + SPEC + "'"; + +/** A gate-code change (full panel, Red Team included) beside about 2.6 MB under docs/benchmarks/**, two files of it kept only for a secret line. */ +function fixture() { + const policy = JSON.stringify({ review_exclude_paths: ['docs/benchmarks/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n'; + const repo = makeFixtureRepo({ prefix: 'gstack-sel-', baseFiles: { '.gstack/ship-policy.json': policy } }); + repos.push(repo); + const files: Record = { 'lib/ship-queue-x.ts': Array.from({ length: 260 }, (_, i) => 'export const f' + i + ' = (a: number) => a + ' + i + ';').join('\n') + '\n' }; + for (let k = 0; k < 6; k++) files['docs/benchmarks/r/t' + k + '.json'] = ('{"x": "' + 'y'.repeat(200) + '"}\n').repeat(2000); + for (let k = 0; k < 2; k++) files['docs/benchmarks/r/cap' + k + '.json'] = Array.from({ length: 400 }, (_, n) => n % 100 === 7 + ? ' "system": "Read ${OPENAI_API_KEY_F' + k + '_L' + n + '} from the environment and never print it.",' + : ' "line' + n + '": "' + 'x'.repeat(40) + '",').join('\n') + '\n'; + commit(repo, files); + const mb = repo.git('merge-base', 'origin/main', 'HEAD'); + const p = JSON.parse(runBin(repo, 'gstack-review-plan', ['--base', 'main', '--json']).out); + return { repo, mb, x: p.review_exclude as { state: string; digest: string; specs: string[]; keep: string[]; excerpt: string[] } }; +} +const empty = (repo: FixtureRepo) => { const f = path.join(repo.root, 'findings.jsonl'); fs.writeFileSync(f, ''); return f; }; +const render = (repo: FixtureRepo, mb: string, out: string, role: string, extra: string[] = []) => + runBin(repo, 'gstack-review-dispatch', ['render', '--role', role, ...extra, '--out', out, '--base', 'main', '--diff-base', mb]); +const rendered = (ledger: string) => fs.readFileSync(ledger, 'utf8').split('\n').filter(Boolean).map(l => JSON.parse(l)).filter(e => e.event === 'rendered'); +const excerptSha = (brief: string) => /in the file \S+ \(\d+ bytes, sha256 ([0-9a-f]{64})\)/.exec(brief)?.[1]; +const mark = (repo: FixtureRepo, mf: string, reader: string, states: Array<[string, string[]]>) => { + for (const [s, extra] of states) expect(runBin(repo, 'gstack-review-dispatch', ['mark', '--manifest', mf, '--reader', reader, '--state', s, ...extra]).status).toBe(EXIT.ok); +}; +const collectEarlier = (repo: FixtureRepo, mf: string) => { + const m = JSON.parse(fs.readFileSync(mf, 'utf8')); + for (const r of m.readers.filter((x: any) => x.stage === 'first' || x.stage === 'specialists')) mark(repo, mf, r.reader, [['intent', []], ['created', ['--path', '1']], ['collected', []]]); +}; +/** What a rendered brief must carry under the fixture's policy: the exclusion in its git diff commands, the disclosure and the excerpt. */ +function expectSelection(brief: string, digest: string) { + expect(brief).toContain(QUOTED); + expect(brief).toContain('(selection ' + digest + ')'); + expect(excerptSha(brief)).toMatch(/^[0-9a-f]{64}$/); +} + +describe('every reader path builds its diff from the base policy\'s selection', () => { + // Value: protects=a Red Team rendered on its own (before the specialists' output exists) reads the selection the pass's + // other readers read; fails_when=render builds the Red Team's diff from its flags alone, so it refuses REVIEW_DIFF_TOO_LARGE + // at the unexcluded size or renders with no disclosure and no excerpt (harness-evals #269, 30.3 MB vs 2 MB); why_new=GST-1476; + // seam=gstack-review-dispatch render on a real repository; no_claim=what a model does with the brief + test('the Red Team on a first render reads the selection and the excerpts', () => { + const { repo, mb, x } = fixture(); + expect(x.state).toBe('active'); + expect(x.excerpt.length).toBe(2); + const out = path.join(repo.root, 'rt'); + const r = render(repo, mb, out, 'red-team', ['--findings-file', empty(repo), '--specialists', '1']); + expect(r.err).not.toContain('REVIEW_DIFF_TOO_LARGE'); + expect(r.status).toBe(EXIT.ok); + const brief = fs.readFileSync(path.join(out, 'red-team.prompt.md'), 'utf8'); + expectSelection(brief, x.digest); + expect(rendered(path.join(out, 'review-ledger.jsonl')).map(e => e.exclude)).toEqual([x.digest]); + }, 240_000); + + // Value: protects=the Red Team dispatched before the specialists' output, failed, then rendered again (by hand or by a + // fresh pass) reads the same selection and excerpt; fails_when=the re-render (standalone or the fresh pass's late red-team + // stage) drops review_exclude_paths or the excerpts; why_new=GST-1476 (the #269 Red Team was marked failed and re-rendered); + // seam=gstack-review-dispatch render, sent, pass, mark and stage on a real repository; no_claim=the dispatch program's retry policy + test('the Red Team re-rendered after a failed dispatch reads the same selection', () => { + const { repo, mb, x } = fixture(); + const out = path.join(repo.root, 'rt'); + expect(render(repo, mb, out, 'red-team', ['--findings-file', empty(repo), '--specialists', '1']).status).toBe(EXIT.ok); + const first = fs.readFileSync(path.join(out, 'red-team.prompt.md'), 'utf8'); + const wrong = path.join(repo.root, 'sent.txt'); + fs.writeFileSync(wrong, 'a prompt the parent wrote by hand\n'); + expect(runBin(repo, 'gstack-review-dispatch', ['sent', '--ledger', path.join(out, 'review-ledger.jsonl'), '--role', 'red-team', '--sent', wrong]).status).not.toBe(EXIT.ok); + const findings = path.join(repo.root, 'merged.jsonl'); + fs.writeFileSync(findings, JSON.stringify({ id: 's1', reader: 'specialist-batched', severity: 'medium', text: 'a finding' }) + '\n'); + const again = render(repo, mb, out, 'red-team', ['--findings-file', findings, '--specialists', '1']); + expect(again.err).not.toContain('REVIEW_DIFF_TOO_LARGE'); + expect(again.status).toBe(EXIT.ok); + const second = fs.readFileSync(path.join(out, 'red-team.prompt.md'), 'utf8'); + expectSelection(second, x.digest); + expect(excerptSha(second)).toBe(excerptSha(first)!); + expect(rendered(path.join(out, 'review-ledger.jsonl')).map(e => e.exclude)).toEqual([x.digest, x.digest]); + + const p1 = path.join(repo.root, 'p1'), mf1 = path.join(p1, 'pass-manifest.json'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', p1, '--pass', 'p1']).status).toBe(EXIT.ok); + collectEarlier(repo, mf1); + expect(runBin(repo, 'gstack-review-dispatch', ['stage', '--manifest', mf1, '--stage', 'red-team']).status).toBe(EXIT.ok); + mark(repo, mf1, 'red-team', [['intent', []], ['created', ['--path', '2']], ['failed', ['--error', 'failed']]]); + const p2 = path.join(repo.root, 'p2'), mf2 = path.join(p2, 'pass-manifest.json'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', p2, '--pass', 'p2']).status).toBe(EXIT.ok); + collectEarlier(repo, mf2); + const late = runBin(repo, 'gstack-review-dispatch', ['stage', '--manifest', mf2, '--stage', 'red-team']); + expect(late.err).not.toContain('REVIEW_DIFF_TOO_LARGE'); + expect(late.status).toBe(EXIT.ok); + const rt = JSON.parse(fs.readFileSync(mf2, 'utf8')).readers.find((r: any) => r.reader === 'red-team'); + expectSelection(fs.readFileSync(rt.file, 'utf8'), x.digest); + expect(rt.excerpt_file.sha256).toBe(excerptSha(second)!); + }, 240_000); + + // Value: protects=a reader kind added later cannot render (standalone or through the manifest) without the policy's + // selection; fails_when=any BRIEF_ROLES role renders a brief without the exclusion, the disclosure digest or the excerpt, a + // role has no entry here, or a plan dispatch that reaches buildManifest without the policy keeps reading the whole diff; + // why_new=GST-1476; seam=gstack-review-dispatch render for every role and buildManifest for every Role; no_claim=brief wording + test('every reader kind gets the same selection, standalone and in the manifest', () => { + const { repo, mb, x } = fixture(); + const findings = empty(repo); + const fix = (n: string) => { const f = path.join(repo.root, n); fs.writeFileSync(f, n + '\n'); return f; }; + const p0 = path.join(repo.root, 'p0'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', p0, '--pass', 'p0']).status).toBe(EXIT.ok); + const disclosure = JSON.parse(fs.readFileSync(path.join(p0, 'pass-manifest.json'), 'utf8')).disclosure as string; + const ROLE_ARGS: Record = { + specialist: ['--name', 'security'], + 'specialist-batched': ['--checklists', 'testing,security'], + 'red-team': ['--findings-file', findings, '--specialists', '1'], + 'adversarial-native': [], + 'outside-voice': [], + targeted: ['--classes', 'secret', '--disclosure', disclosure], + 'fix-check': { none: 'reads decision 3.5\'s fix list and touched hunks, never a git diff' }, + }; + for (const role of BRIEF_ROLES) { + const a = ROLE_ARGS[role]; + expect({ role, listed: a !== undefined }).toEqual({ role, listed: true }); + if (!Array.isArray(a)) continue; + const out = path.join(repo.root, 'r-' + role); + const r = render(repo, mb, out, role, a); + expect({ role, err: r.status === EXIT.ok ? '' : r.err, status: r.status }).toEqual({ role, err: '', status: EXIT.ok }); + const ev = rendered(path.join(out, 'review-ledger.jsonl')); + expect({ role, exclude: ev.map(e => e.exclude) }).toEqual({ role, exclude: [x.digest] }); + const brief = fs.readFileSync(ev[0].file, 'utf8'); + expect({ role, spec: brief.includes(QUOTED), digest: brief.includes('(selection ' + x.digest + ')') }).toEqual({ role, spec: true, digest: true }); + if (role === 'outside-voice') { + const supplied = fs.readFileSync(path.join(out, 'outside-voice.diff'), 'utf8'); + expect(supplied).toContain('## Secret-line excerpts (2 files)'); + expect(supplied).not.toContain('diff --git a/docs/benchmarks/r/t0.json'); + expect(Buffer.byteLength(supplied)).toBeLessThan(REVIEW_DIFF_MAX_BYTES); + } else expect({ role, excerpt: !!excerptSha(brief) }).toEqual({ role, excerpt: true }); + } + const roles = Object.keys(ROLE_EFFORT) as Role[]; + const m = buildManifest({ pass: 'p9', out: path.join(repo.root, 'p9'), ledger: path.join(repo.root, 'p9', 'l.jsonl'), base: 'main', diffBase: mb, root: repo.work, checklistRoot: path.resolve(import.meta.dir, '..'), + dispatches: roles.map(role => ({ role, name: role === 'specialist' ? 'security' : role, effort: ROLE_EFFORT[role], exclude: [] })), + next: 'full', planLines: [], reviewExclude: { ...x }, disclosure, fixCheck: { fix_list: fix('fl'), hunks: fix('h'), tests: fix('t') } }); + for (const r of m.readers.filter(r => r.role !== 'fix-check')) { + expect({ reader: r.reader, exclude: r.exclude?.includes(QUOTED), excerpt: r.excerpt }).toEqual({ reader: r.reader, exclude: true, excerpt: x.excerpt }); + expect({ reader: r.reader, keep: (r.keep ?? []).some(k => x.excerpt.includes(k)) }).toEqual({ reader: r.reader, keep: false }); + } + expect(m.readers.map(r => r.role).sort()).toEqual([...roles].sort()); + }, 300_000); + + // Value: protects=the outside voice's embedded diff, rendered from a subdirectory, still holds every changed file outside + // the exclusion; fails_when=render reads the selection's `-- .` from the caller's directory, so a sibling security change + // silently drops out of the outside voice's "whole diff"; why_new=GST-1476 /ship outside voice (render from docs/ lost + // lib/security.ts); seam=gstack-review-dispatch render on a real repository; no_claim=readers that run git themselves + test('the outside voice rendered from a subdirectory reads the whole selection', () => { + const policy = JSON.stringify({ review_exclude_paths: ['docs/results/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n'; + const repo = makeFixtureRepo({ prefix: 'gstack-sel-sub-', baseFiles: { '.gstack/ship-policy.json': policy, 'docs/README.md': 'a\n' } }); + repos.push(repo); + commit(repo, { 'lib/security.ts': 'export const s = 1;\n', 'docs/README.md': 'b\n', 'docs/results/r.json': '{}\n' }); + const mb = repo.git('merge-base', 'origin/main', 'HEAD'); + const supplied = (cwd: string, out: string) => { + const r = runBin(repo, 'gstack-review-dispatch', ['render', '--role', 'outside-voice', '--out', out, '--base', 'main', '--diff-base', mb], {}, cwd); + expect(r.err).toBe(''); + expect(r.status).toBe(EXIT.ok); + return fs.readFileSync(path.join(out, 'outside-voice.diff'), 'utf8'); + }; + const root = supplied(repo.work, path.join(repo.root, 'o-root')); + const sub = supplied(path.join(repo.work, 'docs'), path.join(repo.root, 'o-sub')); + expect(root).toContain('diff --git a/lib/security.ts'); + expect(root).not.toContain('docs/results/r.json'); + expect(sub).toBe(root); + }, 120_000); +});