From df75f706b6a78e8ddeb323033e9fd89c0d41314e Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 00:50:59 +0000 Subject: [PATCH] GSTA-258 C: pin old-panel tests to review.panel all, outside voice reads its diff file, parity cap and size ledger Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- ship/SKILL.md | 3 +-- ship/SKILL.md.tmpl | 3 +-- ship/sections/adversarial.md | 5 ++-- ship/sections/adversarial.md.tmpl | 5 ++-- ship/sections/review-army.md | 6 ++--- ship/sections/review-army.md.tmpl | 6 ++--- test/review-dispatch-origin-base.test.ts | 2 ++ test/review-exclude.test.ts | 13 +++++++---- test/review-pass.test.ts | 23 +++++++++++++------ test/review-plan-cli.test.ts | 9 ++++++-- .../ship/gsta258-c-review-depth.json | 4 ++++ 11 files changed, 48 insertions(+), 31 deletions(-) create mode 100644 test/size-ledger/ship/gsta258-c-review-depth.json diff --git a/ship/SKILL.md b/ship/SKILL.md index 81e8ab8b9..e6c3891e4 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -495,8 +495,7 @@ repeat Step 2 (decisions included) on the merged content, then Step 4. 1. **Select the two reviews.** Run `~/.claude/skills/gstack/bin/gstack-review-read`. Select this invocation's final Step 9 record (`skill:"review"`, `via:"ship"`, or a last 3.5 `review-fix-check`) and Step 11 native record (`skill:"adversarial-review"`; - none after `ADVERSARIAL: skipped` or `NEXT: platform`, whose Step 9 row is the planned skip: - carry its `PLATFORM_REVIEW:` id to Step 16's receipt write as `--platform-review `). Match each to its saved + none after `ADVERSARIAL: skipped`/`NEXT: platform`). Match each to its saved handle, original token and source; reject outside-provider or older records. 2. **Compare their content.** All three snapshots must match: the native record's `wtree` (its `review_binding.state` `verified`), the Step 9 record's diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index abc1e7047..d17c420c1 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -218,8 +218,7 @@ repeat Step 2 (decisions included) on the merged content, then Step 4. 1. **Select the two reviews.** Run `~/.claude/skills/gstack/bin/gstack-review-read`. Select this invocation's final Step 9 record (`skill:"review"`, `via:"ship"`, or a last 3.5 `review-fix-check`) and Step 11 native record (`skill:"adversarial-review"`; - none after `ADVERSARIAL: skipped` or `NEXT: platform`, whose Step 9 row is the planned skip: - carry its `PLATFORM_REVIEW:` id to Step 16's receipt write as `--platform-review `). Match each to its saved + none after `ADVERSARIAL: skipped`/`NEXT: platform`). Match each to its saved handle, original token and source; reject outside-provider or older records. 2. **Compare their content.** All three snapshots must match: the native record's `wtree` (its `review_binding.state` `verified`), the Step 9 record's diff --git a/ship/sections/adversarial.md b/ship/sections/adversarial.md index 224feafcc..0c025218e 100644 --- a/ship/sections/adversarial.md +++ b/ship/sections/adversarial.md @@ -4,9 +4,8 @@ `DISPATCH: adversarial-native` and the outside voice only on `DISPATCH: outside-voice`. Without the native line (light depth at runtime 0; above 0 the plan lists it beside the batched specialist), print `ADVERSARIAL: skipped (review plan)`; -with neither line, take decision 3 of Finish the adversarial phase. Render the native pass with -`--checklists` and `--classes` from its `checklists=` and `classes=`, on its `model=`; with `classes=` its -`verify --targeted-out` record is the targeted pass. On `DISPATCH: targeted`, also run the targeted reader +with neither line, take decision 3 of Finish the adversarial phase. The native pass takes its line's +`checklists=` and `classes=` as render flags and runs on its `model=` (`lane`: yours). On `DISPATCH: targeted`, also run the targeted reader (`gstack-review-dispatch render --role targeted --classes --base --diff-base `, the merge base on every pass, never RANGE_BASE) like the voices; `verify --targeted-out .gstack/tmp/targeted.json` feeds the receipt's `--review-targeted`. Under a token cap, reserve the planned voices as one batch (`gstack-ship-budget check --next dispatch:`) and add `--reservation ` to the `--start adversarial-review` call below. diff --git a/ship/sections/adversarial.md.tmpl b/ship/sections/adversarial.md.tmpl index 8ba9c7925..180c7adf5 100644 --- a/ship/sections/adversarial.md.tmpl +++ b/ship/sections/adversarial.md.tmpl @@ -2,9 +2,8 @@ `DISPATCH: adversarial-native` and the outside voice only on `DISPATCH: outside-voice`. Without the native line (light depth at runtime 0; above 0 the plan lists it beside the batched specialist), print `ADVERSARIAL: skipped (review plan)`; -with neither line, take decision 3 of Finish the adversarial phase. Render the native pass with -`--checklists` and `--classes` from its `checklists=` and `classes=`, on its `model=`; with `classes=` its -`verify --targeted-out` record is the targeted pass. On `DISPATCH: targeted`, also run the targeted reader +with neither line, take decision 3 of Finish the adversarial phase. The native pass takes its line's +`checklists=` and `classes=` as render flags and runs on its `model=` (`lane`: yours). On `DISPATCH: targeted`, also run the targeted reader (`gstack-review-dispatch render --role targeted --classes --base --diff-base `, the merge base on every pass, never RANGE_BASE) like the voices; `verify --targeted-out .gstack/tmp/targeted.json` feeds the receipt's `--review-targeted`. Under a token cap, reserve the planned voices as one batch (`gstack-ship-budget check --next dispatch:`) and add `--reservation ` to the `--start adversarial-review` call below. diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index edf83b67e..c76eb6f71 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -27,10 +27,8 @@ family (`full` is the lane's reasoning, `medium` reasoning medium, `light` low; host that cannot set it runs its default) and reading its `DIFF_CMD:`, in place of 9.1's and 9.2's own selection rules; `specialist batched` is one subagent reading every checklist its `checklists=` names (light adds Red Team), tagging each finding with its checklist. -`adversarial-native single` or `merged` is Step 11's native pass carrying its `checklists=` -(and `classes=`) on its `model=` (`lane`: yours; `cross-family`: the outside voice's). -`NEXT: platform` dispatches nothing: skip the core read, log item 6's row with -`"depth":"platform"` and the `PLATFORM_REVIEW:` id, then continue to Step 10. +`NEXT: platform` runs no reader: log item 6's row with `"depth":"platform"` and the +`PLATFORM_REVIEW:` id (Step 16's receipt write takes `--platform-review `), then go to Step 10. A `SKIP:` reviewer logs `{"dispatched":false,"reason":"scope"}`. Audits run at `AUDIT_EFFORT:`. The core checklist, gate scan and QA always run. Item 6's row adds `"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`, `"depth"`, diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index 4e49ecd26..82da53d85 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -25,10 +25,8 @@ family (`full` is the lane's reasoning, `medium` reasoning medium, `light` low; host that cannot set it runs its default) and reading its `DIFF_CMD:`, in place of 9.1's and 9.2's own selection rules; `specialist batched` is one subagent reading every checklist its `checklists=` names (light adds Red Team), tagging each finding with its checklist. -`adversarial-native single` or `merged` is Step 11's native pass carrying its `checklists=` -(and `classes=`) on its `model=` (`lane`: yours; `cross-family`: the outside voice's). -`NEXT: platform` dispatches nothing: skip the core read, log item 6's row with -`"depth":"platform"` and the `PLATFORM_REVIEW:` id, then continue to Step 10. +`NEXT: platform` runs no reader: log item 6's row with `"depth":"platform"` and the +`PLATFORM_REVIEW:` id (Step 16's receipt write takes `--platform-review `), then go to Step 10. A `SKIP:` reviewer logs `{"dispatched":false,"reason":"scope"}`. Audits run at `AUDIT_EFFORT:`. The core checklist, gate scan and QA always run. Item 6's row adds `"dispatches":[{"role":R,"prompt_bytes":N,"diff_bytes":N}]`, `"depth"`, diff --git a/test/review-dispatch-origin-base.test.ts b/test/review-dispatch-origin-base.test.ts index 9f35c0cac..40391918a 100644 --- a/test/review-dispatch-origin-base.test.ts +++ b/test/review-dispatch-origin-base.test.ts @@ -22,6 +22,8 @@ function fixture() { fs.writeFileSync(path.join(root, 'gitconfig'), '[commit]\n\tgpgsign = false\n'); const sh = (cwd: string, c: string) => { const r = spawnSync('bash', ['-c', c], { cwd, env, encoding: 'utf8', timeout: 60_000 }); if (r.status !== 0) throw new Error(`${c}: ${r.stderr}`); return r.stdout.trim(); }; sh(root, 'git init -q --bare -b main origin.git && git clone -q origin.git work && git clone -q origin.git other 2>/dev/null || true'); + // The briefs this case reads are today's panel (review.panel all); R65's one native reader renders at Step 11. + fs.mkdirSync(path.join(work, '.gstack')); fs.writeFileSync(path.join(work, '.gstack', 'ship-policy.json'), JSON.stringify({ review: { panel: 'all' }, min_gstack_version: '1.94.0.0' }) + '\n'); sh(work, "echo '{}' > package.json && mkdir src && echo 'export const a = 1;' > src/a.ts && git add -A && git commit -qm base && git push -q origin main"); sh(root, 'rm -rf other && git clone -q origin.git other'); sh(path.join(root, 'other'), 'mkdir -p up && for i in $(seq 1 12); do echo "export const u$i = $i;" > up/u$i.ts; done && git add -A && git commit -qm upstream && git push -q origin main'); diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index 4c8465b2b..85e0de2cf 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -18,7 +18,8 @@ import { cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } fro const repos: FixtureRepo[] = []; afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); -const policyText = (patterns: string[]) => JSON.stringify({ review_exclude_paths: patterns, min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n'; +// review.panel all keeps today's panel, whose every brief these cases read; R65's one-reader default is pinned in review-depth-lean.test.ts. +const policyText = (patterns: string[]) => JSON.stringify({ review_exclude_paths: patterns, review: { panel: 'all' }, min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n'; const GZ = Buffer.from([0x1f, 0x8b, 0, 1, 2, 3, 0, 0]); const EXCLUDED = ['receipts/a/result.json', 'receipts/a/trial.gz', 'receipts/config/blob.bin', 'receipts/c/result.json', 'receipts/e/result.json', 'receipts/notes.md']; const KEPT = ['receipts/b/result.json', 'receipts/d/fixture.json', 'receipts/run.sh']; @@ -118,8 +119,11 @@ describe('what review_exclude_paths excludes and keeps', () => { for (const f of KEPT) expect(read.has(f)).toBe(true); const outside = briefs.find(b => b.includes('## Branch diff'))!; expect(outside).toContain(`git diff "$DIFF_BASE" -- . ${m.readers[0].policy_exclude} and git diff "$DIFF_BASE" -- '${reader.keep[0]}'`); - for (const f of EXCLUDED) expect(outside).not.toContain(`diff --git a/${f}`); - expect(outside).toContain('diff --git a/receipts/b/result.json'); + const diffFile = path.join(out, 'briefs', 'outside-voice.diff'); + expect(outside).toContain(`The supplied branch diff is the file ${diffFile} `); + const supplied = fs.readFileSync(diffFile, 'utf8'); + for (const f of EXCLUDED) expect(supplied).not.toContain(`diff --git a/${f}`); + expect(supplied).toContain('diff --git a/receipts/b/result.json'); for (const b of briefs) expect(b).toContain(`(selection ${m.review_exclude.digest})`); }, 180_000); @@ -150,7 +154,8 @@ describe('what review_exclude_paths excludes and keeps', () => { const c = JSON.parse(plan(control, ['--json']).out); const w = JSON.parse(plan(with_, ['--json']).out); expect(c.numbers.publishes).toBe(true); - expect(c.targeted.classes).toEqual(['money', 'secret']); + // HARN-1 (harness-evals#231): a JSON data file is judged by its path, so cost_usd names no money even unexcluded; its secret line still counts + expect(c.targeted.classes).toEqual(['secret']); expect(w.review_exclude.kept_files).toEqual([{ path: 'receipts/s.json', reason: 'secret-line' }]); expect(w.numbers.publishes).toBe(false); expect(w.targeted.classes).toEqual(['secret']); diff --git a/test/review-pass.test.ts b/test/review-pass.test.ts index 2e8ec1126..da11af484 100644 --- a/test/review-pass.test.ts +++ b/test/review-pass.test.ts @@ -10,6 +10,7 @@ import * as path from 'node:path'; import { promptHash, sectionTexts, withWorkspace, workspaceParagraph, workspaceStep } from '../lib/review-brief'; import { CHUNK_CHARS, PARENT_ONLY_QA, buildManifest, chunkCount, chunkOf, markReader, renderStage, specialistFindings, type Manifest } from '../lib/review-pass'; import type { Dispatch } from '../lib/review-depth'; +import { REVIEW_POLICY_KEY_VERSION } from '../lib/ship-policy'; const ROOT = path.resolve(import.meta.dir, '..'); const BIN = path.join(ROOT, 'bin'); @@ -94,20 +95,23 @@ describe('pass manifest', () => { expect(() => renderStage(d, 'specialists', texts, () => '')).toThrow(/cannot read audit\/plan's prompt file/); }); - test('a brief over 1 MiB with invalid UTF-8 in its diff travels in bounded chunks that rejoin to the rendered sha', () => { - const m = manifest([{ role: 'outside-voice', name: 'outside-voice', effort: 'host', exclude: [] }]); - const diff = Buffer.concat([Buffer.from('+line \u00e9\n'.repeat(90_000)), Buffer.from([0xff, 0xfe, 0x0a]), Buffer.from('+"quoted" \\ back\n'.repeat(20_000))]).toString('utf8'); - renderStage(m, 'voices', texts, () => diff); + // The outside voice now reads its diff from a file (test/review-depth-lean.test.ts), so a large brief here is an audit + // reader's prompt file carrying the same bytes. + test('a brief over 1 MiB with invalid UTF-8 in it travels in bounded chunks that rejoin to the rendered sha', () => { + const big = path.join(tmp('rp-big-'), 'plan.md'); + fs.writeFileSync(big, Buffer.concat([Buffer.from('+line \u00e9\n'.repeat(90_000)), Buffer.from([0xff, 0xfe, 0x0a]), Buffer.from('+"quoted" \\ back\n'.repeat(20_000))])); + const m = manifest([], { extras: [{ name: 'plan', prompt_file: big }] }); + renderStage(m, 'specialists', texts, () => ''); const r = m.readers[0]!; const text = fs.readFileSync(r.file!, 'utf8'); expect(Buffer.byteLength(text)).toBeGreaterThan(1024 * 1024); expect(r.chunks).toBe(chunkCount(text)); let joined = ''; for (let i = 0; i < r.chunks!; i++) { - const c = spawnSync(path.join(BIN, 'gstack-review-dispatch'), ['chunk', '--manifest', writeManifestFile(m), '--reader', 'outside-voice', '--index', String(i)], { encoding: 'utf8', timeout: 30_000, maxBuffer: 8 * 1024 * 1024 }); + const c = spawnSync(path.join(BIN, 'gstack-review-dispatch'), ['chunk', '--manifest', writeManifestFile(m), '--reader', 'audit/plan', '--index', String(i)], { encoding: 'utf8', timeout: 30_000, maxBuffer: 8 * 1024 * 1024 }); expect(c.status, c.stderr).toBe(0); const [head, body] = c.stdout.split('\n'); - expect(head).toBe('BRIEF_CHUNK: reader=outside-voice index=' + i + ' of=' + r.chunks + ' sha256=' + r.sha256); + expect(head).toBe('BRIEF_CHUNK: reader=audit/plan index=' + i + ' of=' + r.chunks + ' sha256=' + r.sha256); expect(Buffer.byteLength(body!)).toBeLessThan(1024 * 1024); joined += JSON.parse(body!); } @@ -128,6 +132,9 @@ function repo(): { dir: string; env: NodeJS.ProcessEnv } { const dir = tmp('rp-repo-'); const env = { ...process.env, PATH: BIN + path.delimiter + process.env.PATH, GSTACK_STATE_ROOT: path.join(dir, '.state'), GIT_AUTHOR_NAME: 't', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 't', GIT_COMMITTER_EMAIL: 't@t' }; const sh = (c: string) => { const r = spawnSync('bash', ['-c', c], { cwd: dir, env, encoding: 'utf8', timeout: 60_000 }); if (r.status !== 0) throw new Error(r.stderr); }; + // The pass program cases pin today's panel (review.panel all keeps it on every class); R65's default one-reader table is + // pinned in test/review-depth-lean.test.ts. + fs.mkdirSync(path.join(dir, '.gstack')); fs.writeFileSync(path.join(dir, '.gstack', 'ship-policy.json'), JSON.stringify({ review: { panel: 'all' }, min_gstack_version: REVIEW_POLICY_KEY_VERSION }) + '\n'); sh("git init -q -b main && echo '{}' > package.json && mkdir src && echo 'export const a = 1;' > src/a.ts && echo .state > .gitignore && git add -A && git commit -qm base && git checkout -qb feat"); sh('for i in $(seq 1 80); do echo "export const v$i = $i;" >> src/a.ts; done && git commit -qam feat'); return { dir, env }; @@ -921,7 +928,9 @@ describe('a scoped pass after a blocker fix (GST-619)', () => { expect(redTeam).not.toContain(s.mergeBase); expect(bin(s.dir, s.env, ['stage', '--manifest', s.mf, '--stage', 'voices']).status).toBe(0); const voices = JSON.parse(fs.readFileSync(s.mf, 'utf8')) as Manifest; - expect(fs.readFileSync(voices.readers.find(r => r.reader === 'outside-voice')!.file!, 'utf8')).toContain('+export const v1 = 1;'); + const outsideBrief = voices.readers.find(r => r.reader === 'outside-voice')!.file!; + expect(fs.readFileSync(outsideBrief, 'utf8')).toContain(`The supplied branch diff is the file ${path.join(path.dirname(outsideBrief), 'outside-voice.diff')} `); + expect(fs.readFileSync(path.join(path.dirname(outsideBrief), 'outside-voice.diff'), 'utf8')).toContain('+export const v1 = 1;'); const again = bin(s.dir, s.env, s.args); expect(again.status, again.stdout + again.stderr).toBe(0); expect(again.stdout).toMatch(/^FIX_CHECK: planned effort=full /m); diff --git a/test/review-plan-cli.test.ts b/test/review-plan-cli.test.ts index 36a2355ed..07e00f8c8 100644 --- a/test/review-plan-cli.test.ts +++ b/test/review-plan-cli.test.ts @@ -3,6 +3,7 @@ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; +import { REVIEW_POLICY_KEY_VERSION } from '../lib/ship-policy'; const BIN = path.join(import.meta.dir, '..', 'bin', 'gstack-review-plan'); const dirs: string[] = []; @@ -14,7 +15,11 @@ function repo(files: Record): { dir: string; state: string } { dirs.push(dir, state); const git = (...a: string[]) => spawnSync('git', a, { cwd: dir, encoding: 'utf8', timeout: 10_000 }); git('init', '-q', '-b', 'main'); git('config', 'user.email', 't@t'); git('config', 'user.name', 't'); git('config', 'commit.gpgsign', 'false'); - fs.writeFileSync(path.join(dir, 'README.md'), '# x\n'); git('add', '.'); git('commit', '-qm', 'base'); + fs.writeFileSync(path.join(dir, 'README.md'), '# x\n'); + // These cases pin today's panel and its scoped second pass, which review.panel all keeps for every class; R65's default + // table (one reader off risk class) is pinned in test/review-depth-lean.test.ts. + fs.mkdirSync(path.join(dir, '.gstack')); fs.writeFileSync(path.join(dir, '.gstack', 'ship-policy.json'), JSON.stringify({ review: { panel: 'all' }, min_gstack_version: REVIEW_POLICY_KEY_VERSION }) + '\n'); + git('add', '.'); git('commit', '-qm', 'base'); git('checkout', '-qb', 'feat'); for (const [p, text] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(dir, p)), { recursive: true }); fs.writeFileSync(path.join(dir, p), text); } git('add', '.'); git('commit', '-qm', 'feat'); @@ -121,7 +126,7 @@ describe('gstack-review-plan', () => { } expect(plan([...capStop.slice(0, -2), '--grant', id]).status).toBe(2); expect(plan([...capStop, '--grant', '--json']).status).toBe(2); - }); + }, 30_000); test('--fix-check-prompt renders the fix-check reviewer prompt from the parent\'s files and refuses a missing one', () => { const r = repo({ 'docs/howto.md': '# x\n' }); const w = (n: string, t: string) => { const f = path.join(r.state, n); fs.writeFileSync(f, t); return f; }; diff --git a/test/size-ledger/ship/gsta258-c-review-depth.json b/test/size-ledger/ship/gsta258-c-review-depth.json new file mode 100644 index 000000000..d1908a260 --- /dev/null +++ b/test/size-ledger/ship/gsta258-c-review-depth.json @@ -0,0 +1,4 @@ +{ + "bytes": 373, + "why": "GSTA-258 C: risk-scaled review depth (single/merged reader lines, NEXT: platform planned skip with its PLATFORM_REVIEW id, round 2 on risk class)" +}