mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 10:40:32 +01:00
fix(review-exclude): every reader path, standalone render included, reads the policy's selection and excerpts (GST-1476) (#4048)
* fix(review-exclude): standalone render reads the base policy's selection and excerpts (GST-1476) The plan, buildManifest and gstack-review-dispatch render share lib/review-selection.ts, so a first render, a re-render after a failed dispatch and a late stage read the same diff. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * chore: name the changelog fragment after the branch (GST-1476) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: read a reader's selection from the checkout root, not the caller's directory (GST-1476) Review-Finding: p1v/outside-voice#1 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: name the checkout-root read in the changelog fragment (GST-1476) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
4743132ca0
commit
a339b93597
8 files changed
+313
-35
No files matched your search
+50
-24
@@ -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<string, ReaderDiff>();
|
||||
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<PassReader, 'exclude' | 'keep' | 'excerpt'>) => {
|
||||
@@ -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<PassReader, 'reader' | 'exclude' | 'keep' | 'excerpt' | 'excerpt_file'>, 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<PassReader, 'reader' | 'exclude' | 'keep' | 'excerpt' | 'excerpt_file'> & { diff_base: string; disclosure?: string } {
|
||||
let x: (Record<string, unknown> & { 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<string, unknown> & { 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 <diff-base> '.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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
@@ -782,7 +782,8 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
|
||||
1. `render --role <role> [--name <n>] --out <dir>` 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
|
||||
|
||||
@@ -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).
|
||||
|
||||
<a id="one-selection-every-reader"></a>
|
||||
**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
|
||||
|
||||
+4
-1
@@ -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;
|
||||
|
||||
@@ -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<typeof Bun.Glob>[] }
|
||||
/** 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<EffectiveReaders, 'forbid_paths'> | 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 <base>'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 extends { exclude: string[]; keep?: string[]; excerpt?: string[] }>(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;
|
||||
}
|
||||
@@ -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<string, string> = { '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<BriefRole, string[] | { none: string }> = {
|
||||
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);
|
||||
});
|
||||
Reference in new issue
Block a user