From f0f938d9a3a7fd8c99a5be3869d50a0d0f87ef21 Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 20:42:04 +0000 Subject: [PATCH 1/4] fix(GSTA-167): a Step 7/8 audit is one packet, never sharded Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- docs/review-policy.md | 3 + lib/review-packet.ts | 5 +- test/review-packet.test.ts | 161 +++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 2 deletions(-) diff --git a/docs/review-policy.md b/docs/review-policy.md index bb0addd92..0fb53e7ea 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -352,6 +352,9 @@ writes one packet per `DISPATCH:` line holding: A packet over 200,000 bytes is split by path into `.1ofK` packets of the same reviewer, each with the whole instructions and checklist: no hunk is cut and no path dropped, and a hunk alone over the cap gets its own shard with a `SHARD_OVERSIZE` note. +A Step 7/8 audit packet is never split: the parent gates on one coverage or plan +verdict with one generation allowance, and a plan item built in another shard's paths +would read NOT DONE. The Red Team's packet waits for the specialists (`--only red-team --findings /merged.jsonl`; until then collect lists it `pending`), the fix-check packet takes `--fix-list`, `--tests` and `--callers` with `--range $PASS_HEAD`, and `--audit diff --git a/lib/review-packet.ts b/lib/review-packet.ts index 70cce97df..7281e9d32 100644 --- a/lib/review-packet.ts +++ b/lib/review-packet.ts @@ -4,7 +4,8 @@ * contract (lib/review-prompts.ts, the text the generated prose renders), its * checklists inlined, its DIFF_CMD diff at -U8 against the literal diff base with * a stat, at most 8 KB of the plan, a read budget, and the output path its result - * goes to. A packet over PACKET_CAP_BYTES is sharded by path, never truncated. + * goes to. A reviewer packet over PACKET_CAP_BYTES is sharded by path, never + * truncated; a Step 7/8 audit is one packet (one verdict, one generation allowance). * Selection is lib/review-depth.ts planReview's (through lib/review-plan-live.ts); * collect is lib/review-packet-collect.ts. */ @@ -177,7 +178,7 @@ export function writePackets(draft: PacketDraft, ctx: BuildContext): PacketEntry const fileOf = (name: string) => path.join(ctx.outDir, `${name}.packet.md`); const fixed = bytes(render(draft, `${draft.name}.99of99`, outOf(`${draft.name}.99of99`), { index: 99, total: 99, oversize: true }, '')); const budget = Math.max(cap - fixed - 64, 1); - let shards = shardChunks(draft.diff.chunks, budget); + let shards = draft.role === 'audit' ? [{ text: draft.diff.chunks.join(''), oversize: false }] : shardChunks(draft.diff.chunks, budget); const total = draft.diff.chunks.reduce((n, c) => n + bytes(c), 0); for (let b = Math.ceil((total / shards.length) * 1.02); shards.length > 1 && b < budget; b = Math.ceil(b * 1.05)) { const even = shardChunks(draft.diff.chunks, b); diff --git a/test/review-packet.test.ts b/test/review-packet.test.ts index 73fe7fc5a..c0b3c48f5 100644 --- a/test/review-packet.test.ts +++ b/test/review-packet.test.ts @@ -166,6 +166,101 @@ describe('gstack-review-packet build', () => { expect(a.text).toContain('+export const zeta19 = 19;'); expect(build(r, ['--audit', 'plan-completion']).status).toBe(2); }); + + // Value: protects=a Step 7/8 audit past the packet cap stays one packet holding the whole diff, so the parent gets one coverage or plan verdict and one generation allowance; fails_when=audits shard like reviewers (a plan item built in another shard reads NOT DONE, each shard generates tests); + // why_new=the sharding test covers reviewers only; seam=none; no_claim=how a child reads a packet past its context (it reads the file in ranges) + test('an audit past the cap is never sharded: one packet, every path of the diff', () => { + const files: Record = {}; + for (let i = 0; i < 30; i++) files[`src/f${i}.ts`] = lines(400, `v${i}_`); + const r = repo({}, files); + fs.writeFileSync(path.join(r.state, 'plan.md'), '# Plan\n- [ ] add src/f0.ts\n'); + for (const extra of [['--audit', 'test-coverage'], ['--audit', 'plan-completion', '--plan-file', path.join(r.state, 'plan.md')]]) { + const b = build(r, extra); + expect(b.status).toBe(0); + const ps = packets(b.stdout); + expect(ps.map(p => p.name)).toEqual([extra[1]!]); + expect(ps[0]!.bytes).toBeGreaterThan(200_000); + expect(ps[0]!.text).not.toContain('This is shard'); + for (let i = 0; i < 30; i++) expect(ps[0]!.text).toContain(`+export const v${i}_399 = 399;`); + } + }); + // Value: protects=a full rebuild deletes the previous head's output files, so collect cannot count a stale review as fresh coverage; fails_when=writePackets/writeManifest stop removing old .out files; + // why_new=no test rebuilds over an out dir that holds results; seam=none; no_claim=removal of packets for reviewers absent from the new plan beyond the reviewer rebuilt here + test('a rebuild clears earlier reviewer outputs: a stale result never stands as coverage of the new head', () => { + const r = repo({}, { 'src/a.ts': lines(80, 'eta') }); + const first = packets(build(r, ['--only', 'testing']).stdout)[0]!; + fs.writeFileSync(first.out, 'NO FINDINGS\n'); + expect(spawnSync(BIN, ['collect', '--dir', r.out], { encoding: 'utf8', timeout: 60_000 }).status).toBe(0); + expect(build(r, ['--only', 'testing']).status).toBe(0); + expect(fs.existsSync(first.out)).toBe(false); + const c = spawnSync(BIN, ['collect', '--dir', r.out], { encoding: 'utf8', timeout: 60_000 }); + expect(c.status).toBe(3); + expect(c.stdout).toContain('REVIEWER: testing status=missing'); + }); + + // Value: protects=build refuses bad inputs with usage exit 2 instead of writing packets; fails_when=an --audit name, unreadable --plan-file or non-commit --range is accepted; + // why_new=only missing fix-check inputs and plan-completion without a plan are tested; seam=none; no_claim=the stderr wording + test('usage refusals: an unknown --audit, an unreadable --plan-file and a --range that is not a commit exit 2 with no packet', () => { + const r = repo({}, { 'src/a.ts': lines(20, 'theta') }); + for (const extra of [['--audit', 'bogus'], ['--plan-file', path.join(r.state, 'nope.md')], ['--range', 'not-a-commit']]) { + const b = build(r, extra); + expect(b.status).toBe(2); + expect(b.stdout).not.toContain('PACKET:'); + } + }); + + // Value: protects=build without --out-dir and collect without --dir refuse with usage exit 2 and write nothing; fails_when=either guard is dropped and the command defaults a directory (cwd) or crashes; + // why_new=every other test passes --out-dir/--dir; seam=none; no_claim=the stderr wording + test('build without --out-dir and collect without --dir exit 2 and write nothing', () => { + const r = repo({}, { 'src/a.ts': lines(20, 'iota') }); + const before = spawnSync('git', ['status', '--porcelain', '--untracked-files=all'], { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout; + const b = run(r, ['build', '--base', 'main', '--gate-tagged', '0']); + expect(b.status).toBe(2); + expect(b.stdout).not.toContain('PACKET:'); + const c = run(r, ['collect']); + expect(c.status).toBe(2); + expect(c.stdout).toBe(''); + expect(spawnSync('git', ['status', '--porcelain', '--untracked-files=all'], { cwd: r.dir, encoding: 'utf8', timeout: 10_000 }).stdout).toBe(before); + expect(fs.readdirSync(r.out)).toEqual([]); + }); + + // Value: protects=the outside-voice packet carries CODEX_BOUNDARY_TEXT, OUTSIDE_CONTRACT and the verbatim-reply instruction; fails_when=the outside-voice role text drops the boundary, contract or reply line; + // why_new=only the resolver's boundary literal is compared, never the packet; seam=none; no_claim=that the outside model obeys the boundary + test('the outside-voice packet carries the filesystem boundary, the severity contract and the verbatim-reply instruction', () => { + const r = repo({}, { 'src/a.ts': lines(80, 'kappa') }); + const b = build(r, ['--only', 'outside-voice']); + expect(b.status).toBe(0); + const ov = packets(b.stdout).find(p => p.role === 'outside-voice')!; + expect(ov.text).toContain(P.CODEX_BOUNDARY_TEXT); + expect(ov.text).toContain(P.OUTSIDE_ADVERSARIAL); + expect(ov.text).toContain(P.OUTSIDE_CONTRACT); + expect(ov.text).toContain('the parent saves it verbatim to the output path below'); + expect(ov.text).toContain('+export const kappa79 = 79;'); + }); + + // Value: protects=packets written under an in-repo --out-dir or anything under .gstack/tmp never enter a later packet's diff; fails_when=the insideOut skip is dropped, so a rebuild reviews the previous packets (and their diffs); + // why_new=every other test writes --out-dir outside the fixture repo; seam=none; no_claim=a later build into a different out dir, or tracked files under the out dir + test('packets under an in-repo --out-dir and files under .gstack/tmp never appear in a later packet\'s diff', () => { + const r = repo({}, { 'src/a.ts': lines(80, 'lambda') }); + fs.mkdirSync(path.join(r.dir, '.gstack/tmp/other'), { recursive: true }); + fs.writeFileSync(path.join(r.dir, '.gstack/tmp/other/stray.md'), 'STRAY_TMP_MARKER\n'); + const inRepo = path.join(r.dir, 'review-out'); + const first = run(r, ['build', '--base', 'main', '--gate-tagged', '0', '--out-dir', inRepo, '--only', 'testing']); + expect(first.status).toBe(0); + expect(fs.readdirSync(inRepo).some(f => f.endsWith('.packet.md'))).toBe(true); + const second = run(r, ['build', '--base', 'main', '--gate-tagged', '0', '--out-dir', inRepo]); + expect(second.status).toBe(0); + const all = packets(second.stdout); + expect(all.length).toBeGreaterThan(0); + for (const p of all) { + expect(p.text).toContain('+export const lambda79 = 79;'); + expect(p.text).not.toContain('diff --git a/review-out/'); + expect(p.text).not.toMatch(/^ review-out\/\S* \|/m); + expect(p.text).not.toContain('diff --git a/.gstack/tmp/'); + expect(p.text).not.toMatch(/^ \.gstack\/tmp\/\S* \|/m); + expect(p.text).not.toContain('STRAY_TMP_MARKER'); + } + }); }); describe('the packet prose and the generated SKILL.md prose are one text', () => { @@ -184,6 +279,21 @@ describe('the packet prose and the generated SKILL.md prose are one text', () => const line = read('scripts/resolvers/outside-voice-steps.ts').split('\n').find(l => l.startsWith('const CODEX_BOUNDARY = '))!; expect(line).toBe(`const CODEX_BOUNDARY = '${P.CODEX_BOUNDARY_TEXT}\\\\n\\\\n';`); }); + // Value: protects=every gstack-review-packet command the /ship sections tell the parent to run uses only flags the binary documents; fails_when=a flag is renamed in the bin or the prose keeps a retired one (--hunks); + // why_new=build tests drive the bin directly, never the commands the prose names; seam=none; no_claim=flag values, argument order and the prose's "plus that plan run's flags" composition + test('/ship prose names only gstack-review-packet flags the binary documents', () => { + const known = new Set(spawnSync(BIN, ['--help'], { encoding: 'utf8', timeout: 30_000 }).stdout.match(/--[a-z][a-z-]*/g)!); + const spans: string[] = []; + for (const s of ['review-army', 'adversarial', 'test-coverage', 'plan-completion']) { + const text = read(`ship/sections/${s}.md`); + spans.push(...(text.match(/`[^`]*gstack-review-packet (?:build|collect)[^`]*`/g) ?? [])); + spans.push(...(text.match(/build its packet with that plan run's flags plus `[^`]*`/g) ?? [])); + } + expect(spans.length).toBeGreaterThanOrEqual(7); + const named = spans.flatMap(s => s.match(/--[a-z][a-z-]*/g) ?? []); + expect(named).toContain('--fix-list'); + expect(named.filter(n => !known.has(n))).toEqual([]); + }); test('the resolvers import the prompt bodies instead of holding a copy', () => { for (const f of ['scripts/resolvers/review-army.ts', 'scripts/resolvers/outside-voice-steps.ts', 'scripts/resolvers/fix-check.ts', 'scripts/resolvers/defensive-review.ts']) { const src = read(f); @@ -266,4 +376,55 @@ describe('gstack-review-packet collect', () => { expect(r.stdout).toContain('REVIEWER: adversarial status=findings crit=0 info=1'); expect(r.stdout).toContain('RECOMMENDATION: adversarial: Ship as-is because the race needs two writers'); }); + // Value: protects=collect treats a missing or non-JSON Step 7/8 audit output as incomplete coverage (exit 3); fails_when=the AUDIT status check or its push into results is dropped or inverted; + // why_new=collect tests above use reviewer roles only; seam=none; no_claim=the audit's JSON schema (only that its last line parses as an object) + test('an audit output whose last line is a JSON object is present; missing or prose output is incomplete coverage, exit 3', () => { + const audit = [{ role: 'specialist', name: 'testing' }, { role: 'audit', name: 'test-coverage' }]; + const ok = collect(dir(audit, { testing: 'NO FINDINGS', 'test-coverage': 'diagram\n{"coverage_pct":50,"gaps":1}\n' })); + expect(ok.status).toBe(0); + expect(ok.stdout).toMatch(/^AUDIT: test-coverage status=present /m); + expect(ok.stdout).toMatch(/^COLLECT: reviewers=1 /m); + const missing = collect(dir(audit, { testing: 'NO FINDINGS', 'test-coverage': null })); + expect(missing.status).toBe(3); + expect(missing.stdout).toMatch(/test-coverage=missing.*\(REVIEW_COVERAGE_INCOMPLETE\)$/m); + const prose = collect(dir(audit, { testing: 'NO FINDINGS', 'test-coverage': 'All paths are covered.\n' })); + expect(prose.status).toBe(3); + expect(prose.stdout).toMatch(/^AUDIT: test-coverage status=invalid /m); + }); + + // Value: protects=a deferred Red Team is reported pending and neither read nor counted as missing coverage; fails_when=collect reads deferred entries' (empty) out path or drops the pending line; + // why_new=the build test asserts only PACKET_DEFERRED, never collect's handling; seam=none; no_claim=that the Red Team later runs (parent workflow) + test('a deferred Red Team is listed pending with its build hint and does not make coverage incomplete', () => { + const r = collect(dir([...specialists, { role: 'red-team', name: 'red-team', deferred: true } as Entry], { testing: 'NO FINDINGS', security: 'NO FINDINGS' })); + expect(r.status).toBe(0); + expect(r.stdout).toMatch(/^REVIEWER: red-team status=pending \(build its packet with --findings \S+merged\.jsonl/m); + expect(r.stdout).toMatch(/^COLLECT: reviewers=3 clean=2 findings=0 missing=0 invalid=0 .* pending=1 /m); + }); + + // Value: protects=per-role output validation: a matrix record alone is clean, a specialist line without its required fields is invalid, a refused outside voice is invalid; + // fails_when=validFinding stops requiring confidence/category/specialist, matrix records stop counting, or a refusal reads clean; why_new=no collect test covers these branches; + // seam=none; no_claim=gstack-state-matrix check semantics (matrix content is not validated here) + test.each([ + ['state-machines matrix record alone is clean', 'specialist', '{"matrix":{"states":["a","b"]}}', 0, 'clean'], + ['specialist line missing confidence/category/specialist is invalid', 'specialist', '{"severity":"CRITICAL","path":"src/a.ts","summary":"s"}', 3, 'invalid'], + ['outside voice refusal is invalid', 'outside-voice', 'I cannot review this code.', 3, 'invalid'], + ])('%s', (_label, role, text, code, status) => { + const r = collect(dir([{ role, name: 'r1' }], { r1: text })); + expect(r.status).toBe(code); + expect(r.stdout).toContain(`REVIEWER: r1 status=${status}`); + }); + + // Value: protects=collect on a dir without a readable manifest exits 1 and never reports clean; fails_when=the missing-manifest guard is dropped and an empty plan reads as complete coverage (exit 0); + // why_new=every collect test writes a valid manifest; seam=none; no_claim=the stderr wording + test('collect on a dir with no readable manifest exits 1 and never reports clean', () => { + const empty = tmp('review-packet-collect-'); + const corrupt = tmp('review-packet-collect-'); + fs.writeFileSync(path.join(corrupt, 'manifest.json'), '{not json'); + for (const d of [empty, corrupt]) { + const r = collect(d); + expect(r.status).toBe(1); + expect(r.stdout).not.toContain('COLLECT:'); + expect(r.stderr).toContain('manifest'); + } + }); }); From dd5bf5493f16974578b6f5a25b67d15d251ff8ba Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 21:03:05 +0000 Subject: [PATCH 2/4] fix(GSTA-167): packets survive the replay: base as given, budget roles, read= ranges, data summaries, no stale partial manifest - --base origin/main no longer renders origin/origin/main (resolveBaseRef shared by build and audits) - PACKET lines carry budget_role and read= ranges under 30,000 bytes; build prints BUDGET_NEXT for gstack-ship-budget --next - each packet ends with END OF PACKET; the brief reads it with the read tool in parallel ranges, about 3 calls - .json/.jsonl/.ndjson/.csv/.tsv additions past 400 changed lines reach reviewers as a DATA SUMMARY - a partial build keeps other reviewers only at the same head, base and diff content - collect: a matrix counts only from a state-machines reviewer; a CRITICAL line failing validation is invalid Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-packet | 15 +- capy/skill/gstack/SKILL.md | 2 +- docs/review-policy.md | 24 +++- lib/review-collect.ts | 7 +- lib/review-packet-collect.ts | 12 +- lib/review-packet.ts | 86 ++++++++++-- lib/review-prompts.ts | 4 +- scripts/resolvers/outside-voice-steps.ts | 2 +- scripts/resolvers/review-army.ts | 7 +- ship/sections/adversarial.md | 6 +- ship/sections/adversarial.md.tmpl | 4 +- ship/sections/budget-handoff.md | 6 +- ship/sections/budget-handoff.md.tmpl | 6 +- ship/sections/review-army.md | 7 +- ship/sections/test-coverage.md | 6 +- ship/sections/test-coverage.md.tmpl | 6 +- test/defensive-review-framing.test.ts | 2 +- test/review-packet.test.ts | 130 ++++++++++++++++-- .../ship/gsta167-review-packets.json | 4 +- 19 files changed, 271 insertions(+), 65 deletions(-) diff --git a/bin/gstack-review-packet b/bin/gstack-review-packet index 7f766f543..8f7f08933 100755 --- a/bin/gstack-review-packet +++ b/bin/gstack-review-packet @@ -20,7 +20,8 @@ import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { runScriptSync } from '../lib/script-invocation'; import { reviewPlanLine } from '../lib/review-depth'; import { flagReader, livePlan, parsePlanFlags, planPass, PLAN_FLAGS_HELP } from '../lib/review-plan-live'; -import { AUDITS, auditDraft, draftFor, packetLine, PACKET_CAP_BYTES, readManifest, writeManifest, writePackets, type AuditName, type BuildContext, type PacketEntry } from '../lib/review-packet'; +import { AUDITS, auditDraft, budgetRole, contentKey, draftFor, packetLine, PACKET_CAP_BYTES, READ_CHUNK_BYTES, readManifest, writeManifest, writePackets, type AuditName, type BuildContext, type PacketEntry } from '../lib/review-packet'; +import { resolveBaseRef } from '../lib/review-collect'; import { collect } from '../lib/review-packet-collect'; const BIN = 'gstack-review-packet'; @@ -39,9 +40,10 @@ ${PLAN_FLAGS_HELP} collect --dir read every packet's output: one REVIEWER: line each, every CRITICAL on one line, INFORMATIONAL counts; writes merged.jsonl and informational.md in -build prints PACKET: effort= bytes= tokens~= file= out= per packet -(a reviewer past ${PACKET_CAP_BYTES} bytes is sharded by path as .1ofK, never truncated) and -PACKETS: n= total_bytes= fingerprint=. collect exits 3 with +build prints PACKET: effort= bytes= tokens~= budget_role= read= file= out= +per packet (a reviewer past ${PACKET_CAP_BYTES} bytes is sharded by path as .1ofK, never truncated; read= are the line +ranges, each under ${READ_CHUNK_BYTES} bytes, a reviewer reads in parallel), PACKETS: n= total_bytes= fingerprint= +and BUDGET_NEXT: dispatch: for gstack-ship-budget check --next. collect exits 3 with REVIEW_COVERAGE_INCOMPLETE when any packet's output is missing or unusable. ${exitTableHelp()}`; @@ -76,7 +78,7 @@ function build(): number { if (range !== undefined && spawnSync('git', ['rev-parse', '--verify', '-q', `${range}^{commit}`], { cwd: root, encoding: 'utf8', timeout: 15_000 }).status !== 0) return fail(`--range ${range} is not a commit`); const diffBase = range ?? collected.mergeBase!; fs.mkdirSync(absOut, { recursive: true, mode: 0o700 }); - const ctx: BuildContext = { root, skillRoot: path.resolve(binDir, '..'), binDir, outDir: absOut, head, diffBase, fingerprint: plan.numbers.fingerprint, learnings: learningsFor(binDir), + const ctx: BuildContext = { root, skillRoot: path.resolve(binDir, '..'), binDir, outDir: absOut, head, diffBase, baseRef: resolveBaseRef(root, flags.base), fingerprint: plan.numbers.fingerprint, learnings: learningsFor(binDir), ...(planText !== undefined ? { plan: { path: flag('--plan-file')!, text: planText } } : {}) }; const out: string[] = []; let entries: PacketEntry[] = []; @@ -116,9 +118,10 @@ function build(): number { } out.unshift(reviewPlanLine(plan, planPass(plan, flags.pass))); } - writeManifest(absOut, { version: 1, head, diff_base: diffBase, fingerprint: plan.numbers.fingerprint }, entries, has('--only')); + writeManifest(absOut, { version: 1, head, diff_base: diffBase, fingerprint: plan.numbers.fingerprint, content: contentKey(ctx) }, entries, has('--only')); const built = entries.filter(e => !e.deferred); out.push(...built.map(packetLine), `PACKETS: n=${built.length} total_bytes=${built.reduce((n, e) => n + e.bytes, 0)} fingerprint=${plan.numbers.fingerprint}`); + if (built.length) out.push(`BUDGET_NEXT: dispatch:${built.map(e => budgetRole(e.role)).join(',')}`); process.stdout.write(out.join('\n') + '\n'); return EXIT.ok; } diff --git a/capy/skill/gstack/SKILL.md b/capy/skill/gstack/SKILL.md index 7dbaa6f88..d39c2cf95 100644 --- a/capy/skill/gstack/SKILL.md +++ b/capy/skill/gstack/SKILL.md @@ -44,7 +44,7 @@ Capy checklist for `/autoplan`: drive `gstack-autoplan`, never a hand adaptation Token caps on /ship: pass a brief's cap as `--token-cap `. /ship reads `cost.tokens` from `tools.thread.get({ thread_id: "this" })` (one execute program returning only that number) into `gstack-ship-budget --reading ` and obeys `verdict=` (`docs/ship-budget.md`). -/ship reviewers on Capy (`docs/review-policy.md#reviewer-packets`): one execute program runs `gstack-review-packet build` with the plan run's flags, creates one subagent per `PACKET:` line with `machine: "shared"` on the lane's model at reasoning `medium` (red-team and adversarial keep the lane's reasoning; a cross-family outside voice stays as it is), each briefed only with the packet brief plus "run `install.sh --check --for review-voice` in the same bash call that reads the packet", and starts at most the free slots of the 8-running-subagent cap per batch. The same program polls `tools.subagent.get` every ~20 s until every reviewer settles, bounded by a timeout, then runs `gstack-review-packet collect --dir ` and returns only its output; never read a reviewer's transcript. +/ship reviewers on Capy (`docs/review-policy.md#reviewer-packets`): one execute program runs `gstack-review-packet build` with the plan run's flags, creates one subagent per `PACKET:` line (`tools.subagent.create` takes at most 10 per call) with `machine: "shared"` on the lane's model at reasoning `medium` (red-team and adversarial keep the lane's reasoning; a cross-family outside voice stays as it is), and starts at most the free slots of the running-subagent cap, retrying refused starts as slots free. Each brief is the packet brief with that packet's `read=` ranges, plus one more parallel call in turn 1: `install.sh --check --for review-voice` in bash. A reviewer then finishes in about three calls: the parallel reads, at most one turn of extra ranges, one write. Under a token cap, `gstack-ship-budget check --next` takes build's `BUDGET_NEXT:` value. The same program polls `tools.subagent.get` with `transcript: "none"` every ~20 s until every reviewer settles, then runs `gstack-review-packet collect --dir ` and returns only its output; keep one poll program under ~8 minutes and run it again until collect has run (a lost program loses no reviewer), and never read a reviewer's transcript. ## Plan, approve, ship diff --git a/docs/review-policy.md b/docs/review-policy.md index 0fb53e7ea..0d951e2ec 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -343,11 +343,27 @@ writes one packet per `DISPATCH:` line holding: - its `DIFF_CMD` diff at `-U8` against the literal diff base (`--range ` from pass 2 on), exclusions honored and untracked files included, under a stat; the security specialist, the Red Team and the native adversarial pass see tests and - fixtures as a stat only; + fixtures as a stat only; a data-format file (`.json`, `.jsonl`, `.ndjson`, `.csv`, + `.tsv`, not a package or config manifest) past 400 changed lines reaches every role + as a `DATA SUMMARY` (changed-line count, top-level keys, first and last 40 changed + lines), except the testing reviewer and the coverage audit for data under `test/` or + `fixtures/`; - at most 8 KB of `--plan-file`, and past learnings for specialists; -- a read budget: at most 5 more reads of named ranges (200 lines at most) of files in - the diff, no repository search, no other packets, no transcript; -- its output path and the reply `DONE crit= info=`. +- a read budget: at most 2 more named ranges (200 lines at most) of files in the diff, + in one turn, no repository search, no other packets, no transcript; +- its output path and the reply `DONE crit= info=`, then `END OF PACKET `. + +Each `PACKET:` line carries `budget_role=` (the `gstack-ship-budget` dispatch role: +audits reserve as `specialist`, shards as their parent role) and `read=`, line ranges +under 30,000 bytes each. Capy's read tool returns about 37,800 characters of a file and +drops the rest without a notice, and shell output past about 35,000 characters spills to +a log, so a reviewer reads its packet with the read tool, one parallel call per range, +and finishes in about three calls. `BUDGET_NEXT: dispatch:` is the value for +`gstack-ship-budget check --next`. A partial build (`--only`) keeps other reviewers' +entries only at the same head, diff base and diff content; any other build starts the +manifest over and removes the old outputs. Collect counts a matrix record only from a +reviewer that applies the state-machines checklist, and a `CRITICAL` line that fails +validation makes its reviewer `invalid`. A packet over 200,000 bytes is split by path into `.1ofK` packets of the same reviewer, each with the whole instructions and checklist: no hunk is cut and no path diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 3846df0f3..07f2031ac 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -109,10 +109,15 @@ export function collectCommit(root: string, baseSha: string, headSha: string): C return result(diff, { root: skillDirs, registry, errors, mergeBase, untracked: [] }); } +/** The ref a base names: origin/ when it resolves, else as given (so origin/main stays origin/main). */ +export function resolveBaseRef(root: string, base: string): string { + return git(root, ['rev-parse', '--verify', '-q', `origin/${base}^{commit}`]).ok ? `origin/${base}` : base; +} + /** The working tree against the merge base with origin/ (or ), untracked files counted per path; the registry is the base ref's commit. */ export function collectLive(root: string, base: string): Collected { const errors: DepthError[] = []; - const ref = git(root, ['rev-parse', '--verify', '-q', `origin/${base}^{commit}`]).ok ? `origin/${base}` : base; + const ref = resolveBaseRef(root, base); const registry = loadRegistryAt(root, ref); const mb = git(root, ['merge-base', ref, 'HEAD']); if (!mb.ok) { diff --git a/lib/review-packet-collect.ts b/lib/review-packet-collect.ts index a888220f0..1e5646cf6 100644 --- a/lib/review-packet-collect.ts +++ b/lib/review-packet-collect.ts @@ -38,7 +38,7 @@ export function parseOutput(packet: PacketEntry, text: string | null): ReviewerR const r: ReviewerResult = { packet, status: 'missing', findings: [], records: 0, invalid: 0 }; if (text === null || !text.trim()) return r; if (packet.role === 'outside-voice') return parseOutside(r, text); - let clean = false; + let clean = false, badCritical = false; for (const raw of text.split('\n')) { const line = raw.trim(); if (!line || line.startsWith('```')) continue; @@ -46,12 +46,16 @@ export function parseOutput(packet: PacketEntry, text: string | null): ReviewerR if (/^Recommendation:/i.test(line)) { r.recommendation = line; continue; } let v: unknown; try { v = JSON.parse(line); } catch { r.invalid++; continue; } - if (v && typeof v === 'object' && 'matrix' in v) { r.records++; continue; } + if (v && typeof v === 'object' && !Array.isArray(v) && 'matrix' in v) { + const m = (v as { matrix: unknown }).matrix; + if (packet.checklists.includes('state-machines') && m && typeof m === 'object') r.records++; else r.invalid++; + continue; + } const f = validFinding(v, packet.role); - if (!f) { r.invalid++; continue; } + if (!f) { r.invalid++; if ((v as { severity?: unknown } | null)?.severity === 'CRITICAL') badCritical = true; continue; } r.findings.push({ ...f, specialist: str(f.specialist) ? f.specialist : packet.reviewer }); } - r.status = r.findings.length ? 'findings' : clean || r.records ? 'clean' : 'invalid'; + r.status = badCritical ? 'invalid' : r.findings.length ? 'findings' : clean || r.records ? 'clean' : 'invalid'; return r; } diff --git a/lib/review-packet.ts b/lib/review-packet.ts index 7281e9d32..8ca58d003 100644 --- a/lib/review-packet.ts +++ b/lib/review-packet.ts @@ -10,6 +10,7 @@ * collect is lib/review-packet-collect.ts. */ import { spawnSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; import * as fs from 'node:fs'; import * as path from 'node:path'; import type { Dispatch, Role } from './review-depth'; @@ -17,6 +18,10 @@ import * as P from './review-prompts'; export const PACKET_CAP_BYTES = 200_000; export const PLAN_EXCERPT_BYTES = 8 * 1024; +/** One read-tool call's share of a packet: Capy's read returns ~37.8K chars and drops the rest silently, so packets are read as parallel ranges under this. */ +export const READ_CHUNK_BYTES = 30_000; +/** A data-format file (.json/.jsonl/.ndjson/.csv/.tsv, not a package or config manifest) past this many changed lines reaches reviewers as a summary. */ +export const DATA_SUMMARY_LINES = 400; export const MANIFEST = 'manifest.json'; export const AUDITS = ['test-coverage', 'plan-completion'] as const; export type AuditName = (typeof AUDITS)[number]; @@ -25,13 +30,17 @@ export type PacketRole = Role | 'audit'; export interface PacketEntry { role: PacketRole; name: string; reviewer: string; effort: string; file: string; out: string; bytes: number; checklists: string[]; fixtures: 'full' | 'summary'; shard: { index: number; total: number; oversize: boolean } | null; + /** Line ranges (`from-to`, comma-joined) that read the packet whole, each under READ_CHUNK_BYTES. */ + read?: string; /** The Red Team before collect: listed so collect reports it pending until its packet is built. */ deferred?: boolean; } -export interface Manifest { version: 1; head: string; diff_base: string; fingerprint: string; packets: PacketEntry[] } +export interface Manifest { version: 1; head: string; diff_base: string; fingerprint: string; content?: string; packets: PacketEntry[] } export interface BuildContext { root: string; skillRoot: string; binDir: string; outDir: string; head: string; diffBase: string; fingerprint: string; + /** The ref `origin/` stands for in prompt text (lib/review-collect.ts resolveBaseRef). */ + baseRef?: string; plan?: { path: string; text: string }; /** Merged findings for the Red Team (collect's merged.jsonl), one summary line each. */ findings?: string[]; @@ -75,7 +84,29 @@ export function checklistPath(skillRoot: string, name: string): string { export interface ReviewerDiff { stat: string; chunks: string[]; fixtureStat: string | null; paths: string[] } /** The reviewer's DIFF_CMD at -U8 (tracked and untracked, exclusions honored) split per file; fixtures go to a stat in summary mode. */ -export function reviewerDiff(root: string, diffBase: string, exclude: readonly string[], summary: boolean, skip: (p: string) => boolean): ReviewerDiff { +const DATA_FILE = /\.(json|jsonl|ndjson|csv|tsv)$/i; +const MANIFEST_FILE = /^(package(-lock)?\.json|composer\.json|deno\.jsonc?|tsconfig[^/]*\.json|[^/]*\.config\.json)$/i; + +function topLevelKeys(file: string): string { + try { + const text = fs.readFileSync(file, 'utf8'); + if (/\.(csv|tsv)$/i.test(file)) return `columns: ${text.split('\n')[0]!.slice(0, 300)}`; + const v = /\.json$/i.test(file) ? JSON.parse(text) : JSON.parse(text.split('\n').find(l => l.trim()) ?? 'null'); + const keys = (o: unknown) => (o && typeof o === 'object' && !Array.isArray(o) ? Object.keys(o).slice(0, 30).join(', ') : ''); + return Array.isArray(v) ? `array of ${v.length}; first item keys: ${keys(v[0]) || '(none)'}` : `top-level keys: ${keys(v) || '(none)'}`; + } catch { return 'top-level keys: (not parsed)'; } +} + +/** A large data-format file's chunk as a summary: its changed-line count, keys, and the first and last 40 changed lines. */ +function dataSummary(root: string, p: string, chunk: string): string | null { + if (!DATA_FILE.test(p) || MANIFEST_FILE.test(path.basename(p))) return null; + const changed = chunk.split('\n').filter(l => /^[+-]/.test(l) && !/^(\+\+\+|---) /.test(l)); + if (changed.length <= DATA_SUMMARY_LINES) return null; + return [`diff --git a/${p} b/${p}`, `DATA SUMMARY: ${p} (${changed.length} changed lines, a data file shown as a summary; ask the parent for a line range if a finding needs one)`, + topLevelKeys(path.join(root, p)), 'first 40 changed lines:', ...changed.slice(0, 40), '…', 'last 40 changed lines:', ...changed.slice(-40), ''].join('\n'); +} + +export function reviewerDiff(root: string, diffBase: string, exclude: readonly string[], summary: boolean, skip: (p: string) => boolean, keepData: (p: string) => boolean = () => false): ReviewerDiff { const fixtureSpecs = summary ? P.FIXTURE_PATHSPECS.map(p => `:(exclude)${p}`) : []; const spec = ['--', '.', ...exclude, ...fixtureSpecs]; const diff = git(root, ['diff', '-U8', '--no-color', '--no-ext-diff', '--no-renames', diffBase, ...spec]); @@ -94,7 +125,8 @@ export function reviewerDiff(root: string, diffBase: string, exclude: readonly s fixtureStat = [git(root, ['diff', '--stat=200', '--no-renames', diffBase, ...fx]).trimEnd(), ...listed.map(p => ` ${p} | untracked`)].filter(Boolean).join('\n') || '(no test or fixture changes)'; } const paths = chunks.map(c => /^diff --git a\/(.*?) b\//.exec(c)?.[1] ?? '?'); - return { stat: stat || '(empty diff)', chunks, fixtureStat, paths }; + const shown = chunks.map((c, i) => (keepData(paths[i]!) ? c : dataSummary(root, paths[i]!, c) ?? c)); + return { stat: stat || '(empty diff)', chunks: shown, fixtureStat, paths }; } /** Diff pieces packed into shards under `budget` bytes; a file over it splits at hunks; a hunk over it is its own oversize shard. */ @@ -168,7 +200,21 @@ function render(draft: PacketDraft, name: string, out: string, shard: { index: n `## Output\n\nOutput path: \`${out}\``, '', ...draft.tail, `## Diff stat\n\n${block(draft.diff.stat)}`, '', ...(draft.diff.fixtureStat !== null ? [`## Tests and fixtures (summary mode)\n\n${block(draft.diff.fixtureStat)}`, ''] : []), - `## ${draft.diffLabel} (\`${draft.diffCmd}\`, untracked files included)\n\n${block(diffText, 'diff')}`, ''].join('\n'); + `## ${draft.diffLabel} (\`${draft.diffCmd}\`, untracked files included)\n\n${block(diffText, 'diff')}`, '', `END OF PACKET ${name}`, ''].join('\n'); +} + +/** Line ranges that read `text` whole, each at most READ_CHUNK_BYTES (a single longer line is its own range). */ +export function readRanges(text: string): string { + const lines = text.replace(/\n$/, '').split('\n'); + const out: string[] = []; + let from = 1, size = 0; + lines.forEach((l, i) => { + const b = bytes(l) + 1; + if (size && size + b > READ_CHUNK_BYTES) { out.push(`${from}-${i}`); from = i + 1; size = 0; } + size += b; + }); + out.push(`${from}-${lines.length}`); + return out.join(','); } /** One reviewer's packet files (one, or k shards), written under outDir; returns their manifest entries. */ @@ -190,7 +236,7 @@ export function writePackets(draft: PacketDraft, ctx: BuildContext): PacketEntry const text = render(draft, name, outOf(name), shard, s.text); fs.writeFileSync(fileOf(name), text, { mode: 0o600 }); fs.rmSync(outOf(name), { force: true }); - return { role: draft.role, name, reviewer: draft.name, effort: draft.effort, file: fileOf(name), out: outOf(name), bytes: bytes(text), checklists: draft.checklists, fixtures: draft.summary ? 'summary' : 'full', shard }; + return { role: draft.role, name, reviewer: draft.name, effort: draft.effort, file: fileOf(name), out: outOf(name), bytes: bytes(text), checklists: draft.checklists, fixtures: draft.summary ? 'summary' : 'full', shard, read: readRanges(text) }; }); } @@ -198,6 +244,7 @@ const diffCmd = (base: string, exclude: readonly string[], summary: boolean) => const specs = [...exclude, ...(summary ? P.FIXTURE_PATHSPECS.map(p => `:(exclude)${p}`) : [])]; return `git diff -U8 ${base}${specs.length ? ` -- . ${specs.map(x => `'${x}'`).join(' ')}` : ''}`; }; +const underTests = (p: string) => /(^|\/)(tests?|fixtures?|__tests__)\//.test(p); const insideOut = (ctx: BuildContext) => (p: string) => p.startsWith('.gstack/tmp/') || path.resolve(ctx.root, p).startsWith(path.resolve(ctx.outDir) + path.sep); function common(ctx: BuildContext, stack: string, testFw: string, learnings: string | null, budget: string): string[] { @@ -219,37 +266,43 @@ export function draftFor(d: Dispatch, ctx: BuildContext): PacketDraft { `## Instructions\n\n${t.instructions.join('\n\n')}`, '', `## Findings contract\n\n${t.contract}\n\n${t.reply}`, '', ...t.extra.flatMap(s => [s, '']), ...common(ctx, stack, testFw, learnings, P.READ_BUDGET), ...checklists.flatMap(s => [s, ''])]; return { role: d.role, name: d.name.split(' ')[0]!, effort: d.effort, checklists: t.checklists, summary: t.summary, head, tail: [], - diff: reviewerDiff(ctx.root, ctx.diffBase, d.exclude, t.summary, insideOut(ctx)), diffLabel: t.diffLabel, diffCmd: diffCmd(ctx.diffBase, d.exclude, t.summary) }; + diff: reviewerDiff(ctx.root, ctx.diffBase, d.exclude, t.summary, insideOut(ctx), d.role === 'specialist' && !d.checklists && d.name === 'testing' ? underTests : undefined), diffLabel: t.diffLabel, diffCmd: diffCmd(ctx.diffBase, d.exclude, t.summary) }; } /** An audit's prompt is the generated section's text fence: the packet and the SKILL.md prose are one text. */ -export function auditPrompt(skillRoot: string, audit: AuditName, base: string): string { +export function auditPrompt(skillRoot: string, audit: AuditName, base: string, baseRef = `origin/${base}`): string { const section = fs.readFileSync(path.join(skillRoot, 'ship', 'sections', `${audit}.md`), 'utf8'); const m = /^(`{4,})text\n([\s\S]*?)\n\1$/m.exec(section); if (!m) throw new Error(`ship/sections/${audit}.md has no prompt fence`); - return m[2]!.replaceAll('', base); + return m[2]!.replaceAll('origin/', baseRef).replaceAll('', base); } export function auditDraft(audit: AuditName, ctx: BuildContext, base: string, effort: string): PacketDraft { const { stack, testFw } = detectStack(ctx.root); const budget = 'The diff is below; do not rerun git diff. Read only files in the diff, their tests and the files the plan names, in named line ranges of at most 200 lines; no whole-repository search, no other packets, no transcript. Writes and test runs only as the audit instructions permit.'; const plan = audit === 'plan-completion' && ctx.plan ? [`## Plan (whole, \`${ctx.plan.path}\`)\n\n${block(ctx.plan.text)}`, ''] : []; - const head = [`Role: ${audit} audit (effort=${effort}).`, '', `## Instructions\n\n${auditPrompt(ctx.skillRoot, audit, base)}`, '', + const head = [`Role: ${audit} audit (effort=${effort}).`, '', `## Instructions\n\n${auditPrompt(ctx.skillRoot, audit, base, ctx.baseRef)}`, '', `## Output\n\nWrite your whole final answer to the output path below, its LAST line the JSON object the instructions name. Then reply with exactly one line, \`DONE \`.`, '', ...(ctx.context ? [`## Parent context\n\n${block(ctx.context.trim())}`, ''] : []), ...common({ ...ctx, plan: audit === 'plan-completion' ? undefined : ctx.plan }, stack, testFw, null, budget), ...plan]; - return { role: 'audit', name: audit, effort, checklists: [], summary: false, head, tail: [], diff: reviewerDiff(ctx.root, ctx.diffBase, [], false, insideOut(ctx)), diffLabel: 'Diff', diffCmd: diffCmd(ctx.diffBase, [], false) }; + return { role: 'audit', name: audit, effort, checklists: [], summary: false, head, tail: [], diff: reviewerDiff(ctx.root, ctx.diffBase, [], false, insideOut(ctx), audit === 'test-coverage' ? underTests : undefined), diffLabel: 'Diff', diffCmd: diffCmd(ctx.diffBase, [], false) }; +} + +/** The reviewed content's identity: the whole diff a reviewer could be shown, hashed; a partial build keeps other reviewers only when it is unchanged. */ +export function contentKey(ctx: BuildContext): string { + return createHash('sha256').update(reviewerDiff(ctx.root, ctx.diffBase, [], false, insideOut(ctx), () => true).chunks.join('')).digest('hex').slice(0, 16); } export function readManifest(dir: string): Manifest | null { try { return JSON.parse(fs.readFileSync(path.join(dir, MANIFEST), 'utf8')) as Manifest; } catch { return null; } } -/** Write the manifest; a partial build (--only) replaces entries of the same reviewer and keeps the rest. */ +/** Write the manifest; a partial build (--only) at the same head, diff base and content replaces entries of the same reviewer and keeps the rest; any other build starts over. */ export function writeManifest(dir: string, m: Omit, packets: PacketEntry[], partial: boolean): Manifest { - const prior = partial ? readManifest(dir) : null; + const old = readManifest(dir); + const prior = partial && old && old.head === m.head && old.diff_base === m.diff_base && old.content === m.content ? old : null; const rebuilt = new Set(packets.map(p => p.reviewer)); - if (!partial) for (const old of readManifest(dir)?.packets ?? []) { if (old.deferred) continue; fs.rmSync(old.out, { force: true }); if (!packets.some(p => p.file === old.file)) fs.rmSync(old.file, { force: true }); } + if (!prior) for (const old of readManifest(dir)?.packets ?? []) { if (old.deferred) continue; fs.rmSync(old.out, { force: true }); if (!packets.some(p => p.file === old.file)) fs.rmSync(old.file, { force: true }); } const kept = (prior?.packets ?? []).filter(p => !rebuilt.has(p.reviewer)); const manifest: Manifest = { ...m, packets: [...kept, ...packets] }; fs.writeFileSync(path.join(dir, MANIFEST), JSON.stringify(manifest, null, 2) + '\n', { mode: 0o600 }); @@ -257,5 +310,10 @@ export function writeManifest(dir: string, m: Omit, packets } export function packetLine(p: PacketEntry): string { - return `PACKET: ${p.role} ${p.name} effort=${p.effort} bytes=${p.bytes} tokens~=${Math.ceil(p.bytes / 4)} file=${p.file} out=${p.out}`; + return `PACKET: ${p.role} ${p.name} effort=${p.effort} bytes=${p.bytes} tokens~=${Math.ceil(p.bytes / 4)} budget_role=${budgetRole(p.role)} read=${p.read ?? '1-1'} file=${p.file} out=${p.out}`; +} + +/** The gstack-ship-budget dispatch role a packet reserves as: audits as a specialist reader, shards as their parent role. */ +export function budgetRole(role: PacketRole): Exclude { + return role === 'audit' ? 'specialist' : role; } diff --git a/lib/review-prompts.ts b/lib/review-prompts.ts index 465f5685c..bd74c2c15 100644 --- a/lib/review-prompts.ts +++ b/lib/review-prompts.ts @@ -74,7 +74,7 @@ export function renderFixCheckPrompt(i: FixCheckInput): string { } /** What every reviewer packet allows beyond itself. */ -export const READ_BUDGET = 'Everything you need is in this packet. You may make at most 5 more reads, each a named line range of at most 200 lines of a file in this diff (code around a hunk, a caller the diff changed). No whole-repository search, no git commands, no other packets, no transcript, no web.'; +export const READ_BUDGET = 'Everything you need is in this packet. You may read at most 2 more named line ranges, each at most 200 lines of a file in this diff (code around a hunk, a caller the diff changed), both in one turn as parallel calls. No whole-repository search, no git commands, no other packets, no transcript, no web.'; /** The native adversarial pass writes JSONL like the specialists, plus a classification and its Recommendation line. */ export const ADVERSARIAL_CONTRACT = 'Write each finding as one JSON object per line with the specialist fields (`"specialist":"adversarial"`) plus `"classification":"FIXABLE|INVESTIGATE"`, rated against the same floor (CRITICAL only for a test that would fail, a production break, or a security or data-loss defect); `NO FINDINGS` when nothing would fail. End with the one `Recommendation:` line either way.'; @@ -84,5 +84,5 @@ export const OUTSIDE_CONTRACT = 'Write one finding per line as `[Critical] path: /** The whole brief a packet reviewer receives; the packet carries the rest. */ export function packetBrief(file: string, out: string): string { - return `Read ${file} and follow it: it is your whole task. Write your result to ${out}. Reply with only the DONE line it names.`; + return `Turn 1: read ${file} with your file-reading tool (never cat: shell output past ~35 KB spills to a log), one parallel call per line range its PACKET line lists as read=; load no skill and read no SKILL.md. The packet is your whole task and ends with END OF PACKET. Turn 2, only if needed: at most 2 more ranges, in parallel. Last turn: write your result to ${out} in one write and reply with only the DONE line it names.`; } diff --git a/scripts/resolvers/outside-voice-steps.ts b/scripts/resolvers/outside-voice-steps.ts index 2b141db55..a1793e0b4 100644 --- a/scripts/resolvers/outside-voice-steps.ts +++ b/scripts/resolvers/outside-voice-steps.ts @@ -129,7 +129,7 @@ Those files are part of the recorded content too. Dispatch via the Agent tool with ${FOREGROUND_IF_AVAILABLE} (background is the default since ${CC_BACKGROUND_DEFAULT_SINCE}); findings must arrive before review concludes. ${BACKGROUND_RECOVERY} Fresh context avoids checklist bias, but this is the same harness, not an independent model unless runtime identity proves otherwise. -${isShip ? `Build Step 11's packets from Step 9's last full-pass plan: \`~/.claude/skills/gstack/bin/gstack-review-packet build --only adversarial,outside-voice --base --out-dir "$_PKA"\` with \`_PKA=.gstack/tmp/review-packets-adv-p$PASS\`, that plan run's flags and \`--range \` from pass 2 on. The \`adversarial\` packet carries the defensive framing, the prompt below, the diff with fixtures in summary mode and a JSONL contract (\`NO FINDINGS\` is the expected result for a clean range). Its whole brief is "${packetBrief('', '')}"` : `Subagent prompt: +${isShip ? `Build Step 11's packets from Step 9's last full-pass plan: \`~/.claude/skills/gstack/bin/gstack-review-packet build --only adversarial,outside-voice --base --out-dir "$_PKA"\` with \`_PKA=.gstack/tmp/review-packets-adv-p$PASS\`, that plan run's flags and \`--range \` from pass 2 on. Leave \`outside-voice\` out of \`--only\` when nothing will run it (\`CODEX_MODE\` neither \`ready\` nor \`unverified\` and no reviewer on another model family): collect counts a built packet with no output as missing coverage. The \`adversarial\` packet carries the defensive framing, the native adversarial prompt (\`ADVERSARIAL_NATIVE\`), the diff with fixtures in summary mode and a JSONL contract (\`NO FINDINGS\` is the expected result for a clean range). Its whole brief is "${packetBrief('', '')}" with that packet's \`read=\` ranges.` : `Subagent prompt: "${DEFENSIVE_REVIEW_FRAMING} Read the diff for this branch. First list changed files: \`DIFF_BASE=$(git merge-base origin/ HEAD) && git diff --name-status "$DIFF_BASE"\`. ${FIXTURE_SUMMARY_MODE} diff --git a/scripts/resolvers/review-army.ts b/scripts/resolvers/review-army.ts index 3d068d3b2..c9888327b 100644 --- a/scripts/resolvers/review-army.ts +++ b/scripts/resolvers/review-army.ts @@ -169,12 +169,15 @@ diff base with a stat, past learnings, a read budget and its output path. With --out-dir "$_PK" --only specialist,red-team\` plus the plan run's flags, \`--range \` (the literal \`RANGE_BASE\` sha) from pass 2 on and \`--plan-file \` when a plan is bound. Each \`PACKET:\` line is one reviewer; a diff past 200 KB is split by path into shards (\`testing.1of3\`), each its -own reviewer. \`PACKETS:\` carries the plan's fingerprint. Step 9.1 dispatches every +own reviewer. Its \`read=\` ranges are what the reviewer reads in parallel, each under one +read call's budget. \`PACKETS:\` carries the plan's fingerprint; under a token cap, +\`gstack-ship-budget check --next\` takes build's \`BUDGET_NEXT:\` value (one +\`budget_role=\` per packet), never packet names. Step 9.1 dispatches every packet; the Red Team's waits for collect (below) and Step 11 builds its own. Launch one subagent per packet via the Agent tool (\`subagent_type: "general-purpose"\`, ${FOREGROUND_IF_AVAILABLE}; ${BACKGROUND_RECOVERY}) at its \`effort=\` on the same model -family. The whole brief is "${packetBrief('', '')}" Never paste a diff, +family. The whole brief is "${packetBrief('', '')}" with that packet's \`read=\` ranges. Never paste a diff, checklist or finding into a brief. **Fan-out (\`--review-fanout auto|serial|parallel\` in the prompt; default \`auto\`).** diff --git a/ship/sections/adversarial.md b/ship/sections/adversarial.md index e52706e61..a0e34faf1 100644 --- a/ship/sections/adversarial.md +++ b/ship/sections/adversarial.md @@ -5,8 +5,8 @@ `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. 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. +reserve the planned voices as one batch (`gstack-ship-budget check --next `, never packet names) and add `--reservation ` to the `--start adversarial-review` call below. ## Step 11: Adversarial review @@ -98,7 +98,7 @@ Those files are part of the recorded content too. Dispatch via the Agent tool with `run_in_background: false` when available (background is the default since Claude Code v2.1.198); findings must arrive before review concludes. A launch receipt means it went background: await its completion notice. Fresh context avoids checklist bias, but this is the same harness, not an independent model unless runtime identity proves otherwise. -Build Step 11's packets from Step 9's last full-pass plan: `~/.claude/skills/gstack/bin/gstack-review-packet build --only adversarial,outside-voice --base --out-dir "$_PKA"` with `_PKA=.gstack/tmp/review-packets-adv-p$PASS`, that plan run's flags and `--range ` from pass 2 on. The `adversarial` packet carries the defensive framing, the prompt below, the diff with fixtures in summary mode and a JSONL contract (`NO FINDINGS` is the expected result for a clean range). Its whole brief is "Read and follow it: it is your whole task. Write your result to . Reply with only the DONE line it names." +Build Step 11's packets from Step 9's last full-pass plan: `~/.claude/skills/gstack/bin/gstack-review-packet build --only adversarial,outside-voice --base --out-dir "$_PKA"` with `_PKA=.gstack/tmp/review-packets-adv-p$PASS`, that plan run's flags and `--range ` from pass 2 on. Leave `outside-voice` out of `--only` when nothing will run it (`CODEX_MODE` neither `ready` nor `unverified` and no reviewer on another model family): collect counts a built packet with no output as missing coverage. The `adversarial` packet carries the defensive framing, the native adversarial prompt (`ADVERSARIAL_NATIVE`), the diff with fixtures in summary mode and a JSONL contract (`NO FINDINGS` is the expected result for a clean range). Its whole brief is "Turn 1: read with your file-reading tool (never cat: shell output past ~35 KB spills to a log), one parallel call per line range its PACKET line lists as read=; load no skill and read no SKILL.md. The packet is your whole task and ends with END OF PACKET. Turn 2, only if needed: at most 2 more ranges, in parallel. Last turn: write your result to in one write and reply with only the DONE line it names." with that packet's `read=` ranges. Present findings under an `ADVERSARIAL REVIEW (Claude subagent):` header. **FIXABLE findings** are queued for the parent; do not edit during Step 11. **INVESTIGATE findings** are presented as informational. diff --git a/ship/sections/adversarial.md.tmpl b/ship/sections/adversarial.md.tmpl index 417be026e..74eda3e95 100644 --- a/ship/sections/adversarial.md.tmpl +++ b/ship/sections/adversarial.md.tmpl @@ -3,8 +3,8 @@ `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. 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. +reserve the planned voices as one batch (`gstack-ship-budget check --next `, never packet names) and add `--reservation ` to the `--start adversarial-review` call below. {{ADVERSARIAL_STEP}} diff --git a/ship/sections/budget-handoff.md b/ship/sections/budget-handoff.md index 5529caca6..eca673d3d 100644 --- a/ship/sections/budget-handoff.md +++ b/ship/sections/budget-handoff.md @@ -18,10 +18,12 @@ Field-by-field reference: docs/ship-budget.md. Exit 0 continue or delegate, 3 st **Every boundary.** `gstack-ship-budget check --run --step --next --reading `. Before each dispatch batch, reserve the batch as one sum, then pass the printed id once -(`READING` is the current counter reading; `RESERVATION_ID` is the id the check prints): +(`READING` is the current counter reading; `RESERVATION_ID` is the id the check prints; +`--next` is the `BUDGET_NEXT:` value the batch's `gstack-review-packet build` printed, one +role per packet, never packet names): ```bash -~/.claude/skills/gstack/bin/gstack-ship-budget check --run --step 9 --next dispatch:specialist,red-team --reading "$READING" +~/.claude/skills/gstack/bin/gstack-ship-budget check --run --step 9 --next "$BUDGET_NEXT" --reading "$READING" ~/.claude/skills/gstack/bin/gstack-review-log --start review --reservation "$RESERVATION_ID" ``` diff --git a/ship/sections/budget-handoff.md.tmpl b/ship/sections/budget-handoff.md.tmpl index 30fff488e..84680877d 100644 --- a/ship/sections/budget-handoff.md.tmpl +++ b/ship/sections/budget-handoff.md.tmpl @@ -16,10 +16,12 @@ Field-by-field reference: docs/ship-budget.md. Exit 0 continue or delegate, 3 st **Every boundary.** `gstack-ship-budget check --run --step --next --reading `. Before each dispatch batch, reserve the batch as one sum, then pass the printed id once -(`READING` is the current counter reading; `RESERVATION_ID` is the id the check prints): +(`READING` is the current counter reading; `RESERVATION_ID` is the id the check prints; +`--next` is the `BUDGET_NEXT:` value the batch's `gstack-review-packet build` printed, one +role per packet, never packet names): ```bash -~/.claude/skills/gstack/bin/gstack-ship-budget check --run --step 9 --next dispatch:specialist,red-team --reading "$READING" +~/.claude/skills/gstack/bin/gstack-ship-budget check --run --step 9 --next "$BUDGET_NEXT" --reading "$READING" ~/.claude/skills/gstack/bin/gstack-review-log --start review --reservation "$RESERVATION_ID" ``` diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index d587f9c9a..6059542be 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -371,12 +371,15 @@ diff base with a stat, past learnings, a read budget and its output path. With --out-dir "$_PK" --only specialist,red-team` plus the plan run's flags, `--range ` (the literal `RANGE_BASE` sha) from pass 2 on and `--plan-file ` when a plan is bound. Each `PACKET:` line is one reviewer; a diff past 200 KB is split by path into shards (`testing.1of3`), each its -own reviewer. `PACKETS:` carries the plan's fingerprint. Step 9.1 dispatches every +own reviewer. Its `read=` ranges are what the reviewer reads in parallel, each under one +read call's budget. `PACKETS:` carries the plan's fingerprint; under a token cap, +`gstack-ship-budget check --next` takes build's `BUDGET_NEXT:` value (one +`budget_role=` per packet), never packet names. Step 9.1 dispatches every packet; the Red Team's waits for collect (below) and Step 11 builds its own. Launch one subagent per packet via the Agent tool (`subagent_type: "general-purpose"`, `run_in_background: false` when available; A launch receipt means it went background: await its completion notice.) at its `effort=` on the same model -family. The whole brief is "Read and follow it: it is your whole task. Write your result to . Reply with only the DONE line it names." Never paste a diff, +family. The whole brief is "Turn 1: read with your file-reading tool (never cat: shell output past ~35 KB spills to a log), one parallel call per line range its PACKET line lists as read=; load no skill and read no SKILL.md. The packet is your whole task and ends with END OF PACKET. Turn 2, only if needed: at most 2 more ranges, in parallel. Last turn: write your result to in one write and reply with only the DONE line it names." with that packet's `read=` ranges. Never paste a diff, checklist or finding into a brief. **Fan-out (`--review-fanout auto|serial|parallel` in the prompt; default `auto`).** diff --git a/ship/sections/test-coverage.md b/ship/sections/test-coverage.md index 229e632ef..bd2cc6493 100644 --- a/ship/sections/test-coverage.md +++ b/ship/sections/test-coverage.md @@ -18,9 +18,9 @@ Dispatch the audit through Agent with `subagent_type: "general-purpose"` and run `~/.claude/skills/gstack/bin/gstack-review-packet build --audit test-coverage --base --out-dir .gstack/tmp/review-packets-audit --context .gstack/tmp/audit-context.txt --generation --passes-used ` (the packet holds the prompt below and -the diff) and brief the child only "Read and follow it: it is your whole task. Write -your result to . Reply with only the DONE line it names." Wait for its `out=` file's -LAST-line JSON before applying the coverage gate. +the diff; an audit is one packet, never sharded) and brief the child only with the packet brief and its `read=` ranges: +"Turn 1: read with your file-reading tool (never cat: shell output past ~35 KB spills to a log), one parallel call per line range its PACKET line lists as read=; load no skill and read no SKILL.md. The packet is your whole task and ends with END OF PACKET. Turn 2, only if needed: at most 2 more ranges, in parallel. Last turn: write your result to in one write and reply with only the DONE line it names." +Wait for its `out=` file's LAST-line JSON before applying the coverage gate. **Generation allowance:** Maximum 2 generation passes total per invocation. Count each generation-authorized attempt before dispatch/inline execution, including diff --git a/ship/sections/test-coverage.md.tmpl b/ship/sections/test-coverage.md.tmpl index 214ec9a32..41841161a 100644 --- a/ship/sections/test-coverage.md.tmpl +++ b/ship/sections/test-coverage.md.tmpl @@ -16,9 +16,9 @@ Dispatch the audit through Agent with `subagent_type: "general-purpose"` and run `~/.claude/skills/gstack/bin/gstack-review-packet build --audit test-coverage --base --out-dir .gstack/tmp/review-packets-audit --context .gstack/tmp/audit-context.txt --generation --passes-used ` (the packet holds the prompt below and -the diff) and brief the child only "Read and follow it: it is your whole task. Write -your result to . Reply with only the DONE line it names." Wait for its `out=` file's -LAST-line JSON before applying the coverage gate. +the diff; an audit is one packet, never sharded) and brief the child only with the packet brief and its `read=` ranges: +"Turn 1: read with your file-reading tool (never cat: shell output past ~35 KB spills to a log), one parallel call per line range its PACKET line lists as read=; load no skill and read no SKILL.md. The packet is your whole task and ends with END OF PACKET. Turn 2, only if needed: at most 2 more ranges, in parallel. Last turn: write your result to in one write and reply with only the DONE line it names." +Wait for its `out=` file's LAST-line JSON before applying the coverage gate. **Generation allowance:** Maximum 2 generation passes total per invocation. Count each generation-authorized attempt before dispatch/inline execution, including diff --git a/test/defensive-review-framing.test.ts b/test/defensive-review-framing.test.ts index fd0322dfd..1c3a47b62 100644 --- a/test/defensive-review-framing.test.ts +++ b/test/defensive-review-framing.test.ts @@ -56,7 +56,7 @@ describe('defensive-security framing reaches every adversarial dispatch', () => const army = fs.readFileSync(path.join(ROOT, 'ship', 'sections', 'review-army.md'), 'utf-8').replace(/\s+/g, ' '); const adversarial = fs.readFileSync(path.join(ROOT, 'ship', 'sections', 'adversarial.md'), 'utf-8').replace(/\s+/g, ' '); expect(army).toContain('the defensive framing and the diff with fixtures in summary mode'); - expect(adversarial).toContain('packet carries the defensive framing, the prompt below, the diff with fixtures in summary mode'); + expect(adversarial).toContain('packet carries the defensive framing, the native adversarial prompt (`ADVERSARIAL_NATIVE`), the diff with fixtures in summary mode'); expect(army).not.toContain(FRAMING_SENTENCE); }); diff --git a/test/review-packet.test.ts b/test/review-packet.test.ts index c0b3c48f5..8d3c3199b 100644 --- a/test/review-packet.test.ts +++ b/test/review-packet.test.ts @@ -9,7 +9,8 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import * as P from '../lib/review-prompts'; -import { shardChunks } from '../lib/review-packet'; +import { READ_CHUNK_BYTES, readManifest, shardChunks } from '../lib/review-packet'; +import { parseNext } from '../lib/ship-budget'; const ROOT = path.join(import.meta.dir, '..'); const BIN = path.join(ROOT, 'bin', 'gstack-review-packet'); @@ -36,8 +37,9 @@ function repo(base: Record, feat: Record, untrac } const run = (r: Repo, args: string[]) => spawnSync(BIN, args, { cwd: r.dir, encoding: 'utf8', timeout: 120_000, env: { ...process.env, GSTACK_STATE_ROOT: r.state } }); const build = (r: Repo, extra: string[] = []) => run(r, ['build', '--base', 'main', '--gate-tagged', '0', '--out-dir', r.out, ...extra]); -const packets = (stdout: string) => [...stdout.matchAll(/^PACKET: (\S+) (\S+) effort=\S+ bytes=(\d+) tokens~=\d+ file=(\S+) out=(\S+)$/gm)].map(m => ({ role: m[1]!, name: m[2]!, bytes: Number(m[3]), file: m[4]!, out: m[5]!, text: fs.readFileSync(m[4]!, 'utf8') })); +const packets = (stdout: string) => [...stdout.matchAll(/^PACKET: (\S+) (\S+) effort=\S+ bytes=(\d+) tokens~=\d+ budget_role=(\S+) read=(\S+) file=(\S+) out=(\S+)$/gm)].map(m => ({ role: m[1]!, name: m[2]!, bytes: Number(m[3]), budgetRole: m[4]!, read: m[5]!.split(',').map(r => r.split('-').map(Number) as [number, number]), file: m[6]!, out: m[7]!, text: fs.readFileSync(m[6]!, 'utf8') })); const lines = (n: number, tag: string) => Array.from({ length: n }, (_, i) => `export const ${tag}${i} = ${i};`).join('\n') + '\n'; +const readManifestOf = (dir: string) => readManifest(dir)!.packets; const checklist = (name: string) => fs.readFileSync(path.join(ROOT, 'review', 'specialists', `${name}.md`), 'utf8'); describe('gstack-review-packet build', () => { @@ -161,7 +163,7 @@ describe('gstack-review-packet build', () => { test('audits: the packet carries the generated section prompt fence with the base filled in', () => { const r = repo({}, { 'src/a.ts': lines(20, 'zeta') }); const a = packets(build(r, ['--audit', 'test-coverage', '--generation', 'audit-only', '--passes-used', '1']).stdout)[0]!; - expect(a.text).toContain('You are running a ship-workflow test coverage audit. Run `git diff origin/main`'); + expect(a.text).toContain('You are running a ship-workflow test coverage audit. Run `git diff main`'); expect(a.text).toContain('Generation: audit-only; passes used: 1 of 2.'); expect(a.text).toContain('+export const zeta19 = 19;'); expect(build(r, ['--audit', 'plan-completion']).status).toBe(2); @@ -169,6 +171,95 @@ describe('gstack-review-packet build', () => { // Value: protects=a Step 7/8 audit past the packet cap stays one packet holding the whole diff, so the parent gets one coverage or plan verdict and one generation allowance; fails_when=audits shard like reviewers (a plan item built in another shard reads NOT DONE, each shard generates tests); // why_new=the sharding test covers reviewers only; seam=none; no_claim=how a child reads a packet past its context (it reads the file in ranges) + // Value: protects=--base origin/main names origin/main in the audit prompt; fails_when=the prompt prefixes origin/ to a ref that already has it (git diff origin/origin/main); + // why_new=every build test passes --base main with no origin remote; seam=none; no_claim=a base with no origin remote (covered: it stays as given) + test('--base origin/main and --base main both put origin/main in the audit prompt, never origin/origin/main', () => { + const r = repo({}, { 'src/a.ts': lines(20, 'kappa') }); + spawnSync('git', ['update-ref', 'refs/remotes/origin/main', 'main'], { cwd: r.dir, timeout: 10_000 }); + for (const base of ['origin/main', 'main']) { + const b = run(r, ['build', '--base', base, '--gate-tagged', '0', '--out-dir', r.out, '--audit', 'test-coverage']); + expect(b.status).toBe(0); + const a = packets(b.stdout)[0]!; + expect(a.text).toContain('Run `git diff origin/main`'); + expect(a.text).not.toContain('origin/origin/'); + } + }); + + // Value: protects=every PACKET line names a role gstack-ship-budget accepts and build prints one ready BUDGET_NEXT; fails_when=a packet or shard name leaks into --next and check exits 2; + // why_new=no test joins the packet path to the budget's role contract; seam=none; no_claim=the budget's per-role pricing + test('PACKET lines carry a budget_role the budget accepts; BUDGET_NEXT lists one role per packet, shards keep their parent role', () => { + const files: Record = {}; + for (let i = 0; i < 30; i++) files[`src/f${i}.ts`] = lines(400, `w${i}_`); + const r = repo({}, files); + const b = build(r, ['--only', 'testing,adversarial,outside-voice']); + expect(b.status).toBe(0); + const ps = packets(b.stdout); + expect(ps.some(p => p.name.startsWith('testing.1of'))).toBe(true); + const want: Record = { specialist: 'specialist', 'adversarial-native': 'adversarial-native', 'outside-voice': 'outside-voice' }; + for (const p of ps) expect(p.budgetRole).toBe(want[p.role]!); + const next = /^BUDGET_NEXT: (\S+)$/m.exec(b.stdout)![1]!; + expect(next).toBe(`dispatch:${ps.map(p => p.budgetRole).join(',')}`); + expect(parseNext(next)).toEqual({ kind: 'dispatch', roles: ps.map(p => p.budgetRole) }); + const a = build(r, ['--audit', 'test-coverage']); + expect(packets(a.stdout)[0]!.budgetRole).toBe('specialist'); + expect(a.stdout).toMatch(/^BUDGET_NEXT: dispatch:specialist$/m); + }); + + // Value: protects=a packet is read whole in one turn: its read= ranges cover every line, each under the read tool's measured budget, and it ends with END OF PACKET; fails_when=a range overruns the budget (silently truncated read) or a line falls between ranges; + // why_new=no test reads a packet the way a reviewer does; seam=none; no_claim=the host's read budget itself (measured on Capy at ~37.8K chars) + test('read= ranges cover the packet line for line, each range under READ_CHUNK_BYTES, ending at END OF PACKET', () => { + const files: Record = {}; + for (let i = 0; i < 12; i++) files[`src/g${i}.ts`] = lines(300, `r${i}_`); + const r = repo({}, files); + const ps = packets(build(r, ['--only', 'testing,maintainability']).stdout); + expect(ps.map(p => p.name).sort()).toEqual(['maintainability', 'testing']); + expect(ps.every(p => p.read.length > 1)).toBe(true); + for (const p of ps) { + const all = p.text.split('\n'); + expect(p.text.trimEnd().split('\n').at(-1)).toBe(`END OF PACKET ${p.name}`); + let next = 1; + for (const [from, to] of p.read) { + expect(from).toBe(next); + expect(Buffer.byteLength(all.slice(from - 1, to).join('\n'))).toBeLessThanOrEqual(READ_CHUNK_BYTES); + next = to + 1; + } + expect(next - 1).toBeGreaterThanOrEqual(p.text.trimEnd().split('\n').length); + } + }); + + // Value: protects=large data-format additions (#34's wiki *.json) reach reviewers as a summary (stat, head and tail, top-level keys), so each role gets one packet; fails_when=data files ride the diff whole and every role shards; + // why_new=FIXTURE_PATHSPECS misses data files outside test paths; seam=none; no_claim=reviewing the data content itself (testing still reads data under test/) + test('a #34-shaped diff (three ~70 KB wiki JSON files and a small code change) gives every role one packet; testing still reads data under test/', () => { + const big = (tag: string) => JSON.stringify({ pages: Array.from({ length: 1400 }, (_, i) => ({ id: `${tag}-${i}`, title: `Page ${i}`, body: 'lorem ipsum dolor' })), meta: { tag } }, null, 1) + '\n'; + const r = repo({}, { 'wiki/a.json': big('a'), 'wiki/b.json': big('b'), 'wiki/c.json': big('c'), 'test/fixtures/d.json': big('d'), 'src/a.ts': lines(60, 'lam'), 'package.json': '{"name":"x"}\n' }); + const b = build(r, ['--only', 'testing,maintainability,security,adversarial']); + expect(b.status).toBe(0); + const ps = packets(b.stdout); + expect(ps.map(p => p.name).sort()).toEqual(['adversarial', 'maintainability', 'security', 'testing']); + for (const p of ps) { + expect(p.text).toContain('DATA SUMMARY: wiki/a.json'); + expect(p.text).toContain('top-level keys: pages, meta'); + expect(p.text).not.toContain('"id": "a-700"'); + expect(p.text).toContain('+export const lam59 = 59;'); + } + expect(ps.find(p => p.name === 'testing')!.text).toContain('"id": "d-700"'); + expect(ps.find(p => p.name === 'maintainability')!.text).not.toContain('"id": "d-700"'); + }); + + // Value: protects=a partial build after the head moved drops the old run's reviewers and their outputs; fails_when=writeManifest keeps entries from another head, so stale clean results count as coverage of new code; + // why_new=partial-build tests stay on one head; seam=none; no_claim=a working-tree edit at the same head beyond the one exercised here + test('a partial build keeps other reviewers only at the same head, base and content; otherwise their entries and outputs go', () => { + const r = repo({}, { 'src/a.ts': lines(80, 'mu') }); + const first = packets(build(r, ['--only', 'testing,maintainability']).stdout); + for (const p of first) fs.writeFileSync(p.out, 'NO FINDINGS\n'); + expect(build(r, ['--only', 'testing']).status).toBe(0); + expect(readManifestOf(r.out).map(p => p.reviewer).sort()).toEqual(['maintainability', 'testing']); + fs.writeFileSync(path.join(r.dir, 'src/a.ts'), lines(81, 'mu')); + expect(build(r, ['--only', 'testing']).status).toBe(0); + expect(readManifestOf(r.out).map(p => p.reviewer)).toEqual(['testing']); + expect(fs.existsSync(first.find(p => p.name === 'maintainability')!.out)).toBe(false); + }); + test('an audit past the cap is never sharded: one packet, every path of the diff', () => { const files: Record = {}; for (let i = 0; i < 30; i++) files[`src/f${i}.ts`] = lines(400, `v${i}_`); @@ -274,6 +365,7 @@ describe('the packet prose and the generated SKILL.md prose are one text', () => expect(read('review/sections/adversarial.md')).toContain(P.OUTSIDE_ADVERSARIAL); expect(read('ship/sections/review-army.md')).toContain(P.packetBrief('', '')); expect(read('ship/sections/adversarial.md')).toContain(P.packetBrief('', '')); + expect(read('ship/sections/test-coverage.md')).toContain(P.packetBrief('', '')); }); test('the outside voice boundary the safety-rule registry pins in the resolver is the packet\'s boundary', () => { const line = read('scripts/resolvers/outside-voice-steps.ts').split('\n').find(l => l.startsWith('const CODEX_BOUNDARY = '))!; @@ -303,10 +395,10 @@ describe('the packet prose and the generated SKILL.md prose are one text', () => }); describe('gstack-review-packet collect', () => { - type Entry = { role: string; name: string }; + type Entry = { role: string; name: string; checklists?: string[] }; function dir(entries: Entry[], outs: Record) { const d = tmp('review-packet-collect-'); - const packets = entries.map(e => ({ ...e, reviewer: e.name, effort: 'full', file: path.join(d, `${e.name}.packet.md`), out: path.join(d, `${e.name}.out`), bytes: 1, checklists: [], fixtures: 'full', shard: null })); + const packets = entries.map(e => ({ ...e, reviewer: e.name, effort: 'full', file: path.join(d, `${e.name}.packet.md`), out: path.join(d, `${e.name}.out`), bytes: 1, checklists: e.checklists ?? [], fixtures: 'full', shard: null })); fs.writeFileSync(path.join(d, 'manifest.json'), JSON.stringify({ version: 1, head: 'h', diff_base: 'b', fingerprint: 'none', packets })); for (const [n, text] of Object.entries(outs)) if (text !== null) fs.writeFileSync(path.join(d, `${n}.out`), text); return d; @@ -405,11 +497,11 @@ describe('gstack-review-packet collect', () => { // fails_when=validFinding stops requiring confidence/category/specialist, matrix records stop counting, or a refusal reads clean; why_new=no collect test covers these branches; // seam=none; no_claim=gstack-state-matrix check semantics (matrix content is not validated here) test.each([ - ['state-machines matrix record alone is clean', 'specialist', '{"matrix":{"states":["a","b"]}}', 0, 'clean'], - ['specialist line missing confidence/category/specialist is invalid', 'specialist', '{"severity":"CRITICAL","path":"src/a.ts","summary":"s"}', 3, 'invalid'], - ['outside voice refusal is invalid', 'outside-voice', 'I cannot review this code.', 3, 'invalid'], - ])('%s', (_label, role, text, code, status) => { - const r = collect(dir([{ role, name: 'r1' }], { r1: text })); + ['state-machines matrix record alone is clean', 'specialist', '{"matrix":{"states":["a","b"]}}', 0, 'clean', ['state-machines']], + ['specialist line missing confidence/category/specialist is invalid', 'specialist', '{"severity":"CRITICAL","path":"src/a.ts","summary":"s"}', 3, 'invalid', []], + ['outside voice refusal is invalid', 'outside-voice', 'I cannot review this code.', 3, 'invalid', []], + ])('%s', (_label, role, text, code, status, checklists) => { + const r = collect(dir([{ role, name: 'r1', checklists }], { r1: text })); expect(r.status).toBe(code); expect(r.stdout).toContain(`REVIEWER: r1 status=${status}`); }); @@ -427,4 +519,22 @@ describe('gstack-review-packet collect', () => { expect(r.stderr).toContain('manifest'); } }); + + // Value: protects=a matrix record counts only for a reviewer that applies the state-machines checklist, and only as an object; fails_when={"matrix":null} from security reads clean; + // why_new=the matrix shortcut was never tested outside state-machines; seam=none; no_claim=gstack-state-matrix check semantics + test('{"matrix":null} or a matrix from a non-state-machines reviewer is not a clean review', () => { + for (const [checklists, text] of [[['security'], '{"matrix":{"states":["a"]}}'], [['state-machines'], '{"matrix":null}']] as [string[], string][]) { + const r = collect(dir([{ role: 'specialist', name: 'r1', checklists }], { r1: text })); + expect(r.status).toBe(3); + expect(r.stdout).toContain('REVIEWER: r1 status=invalid'); + } + }); + + // Value: protects=a CRITICAL line that fails validation makes the reviewer invalid (coverage incomplete) instead of vanishing behind a valid peer line; fails_when=the bad CRITICAL is only counted in invalid_lines and collect exits 0; + // why_new=invalid-line tests only use non-JSON text; seam=none; no_claim=the validity rules themselves + test('a CRITICAL line missing required fields next to a valid finding makes the reviewer invalid, exit 3', () => { + const r = collect(dir([{ role: 'specialist', name: 'testing' }], { testing: [finding({}), '{"severity":"CRITICAL","path":"src/a.ts","summary":"real blocker"}'].join('\n') })); + expect(r.status).toBe(3); + expect(r.stdout).toContain('REVIEWER: testing status=invalid'); + }); }); diff --git a/test/size-ledger/ship/gsta167-review-packets.json b/test/size-ledger/ship/gsta167-review-packets.json index 4d693101a..8714905b9 100644 --- a/test/size-ledger/ship/gsta167-review-packets.json +++ b/test/size-ledger/ship/gsta167-review-packets.json @@ -1,4 +1,4 @@ { - "bytes": -3324, - "why": "GSTA-167 reviewer packets: Step 9.1 dispatches each reviewer by one packet file (gstack-review-packet build/collect) in place of the inlined specialist prompt; Red Team, fix-check, Step 11 and the Step 7/8 audits read packets too" + "bytes": -1608, + "why": "GSTA-167 reviewer packets: Step 9.1 dispatches each reviewer by one packet file (gstack-review-packet build/collect) in place of the inlined specialist prompt; Red Team, fix-check, Step 11 and the Step 7/8 audits read packets too; replay fixes: read= ranges and END OF PACKET in the reviewer brief, BUDGET_NEXT handoff, outside-voice left out of --only when nothing runs it" } From 1d460c6e1c499055a18c092d6d325973e0e2611c Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 21:15:36 +0000 Subject: [PATCH 3/4] fix(GSTA-167): pass-2 review fixes: data summaries read only the diff, a stale Red Team build refuses, any-case CRITICAL lines - topLevelKeys takes keys and columns from the diff's own lines, never the working tree, which a symlink can point at a private file (outside voice CRITICAL) - a --findings build whose head, diff base or content differs from the manifest refuses with STALE_MANIFEST instead of dropping the specialists' coverage (adversarial + Red Team) - collect: a CRITICAL line in any case, or one that fails to parse, makes its reviewer invalid (Red Team) - docs and the misplaced Value header in the review-packet tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-review-packet | 6 +++++- docs/review-policy.md | 10 ++++++---- lib/review-packet-collect.ts | 4 ++-- lib/review-packet.ts | 12 +++++++----- test/review-packet.test.ts | 38 +++++++++++++++++++++++++++++++++--- 5 files changed, 55 insertions(+), 15 deletions(-) diff --git a/bin/gstack-review-packet b/bin/gstack-review-packet index 8f7f08933..1664d1495 100755 --- a/bin/gstack-review-packet +++ b/bin/gstack-review-packet @@ -80,6 +80,10 @@ function build(): number { fs.mkdirSync(absOut, { recursive: true, mode: 0o700 }); const ctx: BuildContext = { root, skillRoot: path.resolve(binDir, '..'), binDir, outDir: absOut, head, diffBase, baseRef: resolveBaseRef(root, flags.base), fingerprint: plan.numbers.fingerprint, learnings: learningsFor(binDir), ...(planText !== undefined ? { plan: { path: flag('--plan-file')!, text: planText } } : {}) }; + const content = contentKey(ctx); + const old = has('--findings') ? readManifest(absOut) : null; + const stale = old && ([['head', head], ['diff_base', diffBase], ['content', content]] as const).find(([k, v]) => old[k] !== v)?.[0]; + if (stale) { process.stdout.write(`STALE_MANIFEST: ${stale} changed since the build in ${outDir} that produced --findings; the Red Team would review another tree than the specialists. Rebuild that pass's packets first.\n`); return EXIT.refused; } const out: string[] = []; let entries: PacketEntry[] = []; if (audit) { @@ -118,7 +122,7 @@ function build(): number { } out.unshift(reviewPlanLine(plan, planPass(plan, flags.pass))); } - writeManifest(absOut, { version: 1, head, diff_base: diffBase, fingerprint: plan.numbers.fingerprint, content: contentKey(ctx) }, entries, has('--only')); + writeManifest(absOut, { version: 1, head, diff_base: diffBase, fingerprint: plan.numbers.fingerprint, content }, entries, has('--only')); const built = entries.filter(e => !e.deferred); out.push(...built.map(packetLine), `PACKETS: n=${built.length} total_bytes=${built.reduce((n, e) => n + e.bytes, 0)} fingerprint=${plan.numbers.fingerprint}`); if (built.length) out.push(`BUDGET_NEXT: dispatch:${built.map(e => budgetRole(e.role)).join(',')}`); diff --git a/docs/review-policy.md b/docs/review-policy.md index 0d951e2ec..eae963672 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -351,7 +351,7 @@ writes one packet per `DISPATCH:` line holding: - at most 8 KB of `--plan-file`, and past learnings for specialists; - a read budget: at most 2 more named ranges (200 lines at most) of files in the diff, in one turn, no repository search, no other packets, no transcript; -- its output path and the reply `DONE crit= info=`, then `END OF PACKET `. +- its output path and the reply `DONE crit= info=`; its last line is `END OF PACKET `. Each `PACKET:` line carries `budget_role=` (the `gstack-ship-budget` dispatch role: audits reserve as `specialist`, shards as their parent role) and `read=`, line ranges @@ -361,9 +361,11 @@ a log, so a reviewer reads its packet with the read tool, one parallel call per and finishes in about three calls. `BUDGET_NEXT: dispatch:` is the value for `gstack-ship-budget check --next`. A partial build (`--only`) keeps other reviewers' entries only at the same head, diff base and diff content; any other build starts the -manifest over and removes the old outputs. Collect counts a matrix record only from a -reviewer that applies the state-machines checklist, and a `CRITICAL` line that fails -validation makes its reviewer `invalid`. +manifest over and removes the old outputs, except a Red Team build (`--findings`), which refuses +with `STALE_MANIFEST` (exit 3) so it never reviews another tree than the specialists did. +Collect counts a matrix record only from a reviewer that applies the state-machines +checklist, and a line naming `CRITICAL` in any case that fails to parse or validate makes its +reviewer `invalid`. A packet over 200,000 bytes is split by path into `.1ofK` packets of the same reviewer, each with the whole instructions and checklist: no hunk is cut and no path diff --git a/lib/review-packet-collect.ts b/lib/review-packet-collect.ts index 1e5646cf6..78e9935cb 100644 --- a/lib/review-packet-collect.ts +++ b/lib/review-packet-collect.ts @@ -45,14 +45,14 @@ export function parseOutput(packet: PacketEntry, text: string | null): ReviewerR if (NO_FINDINGS.test(line)) { clean = true; continue; } if (/^Recommendation:/i.test(line)) { r.recommendation = line; continue; } let v: unknown; - try { v = JSON.parse(line); } catch { r.invalid++; continue; } + try { v = JSON.parse(line); } catch { r.invalid++; if (/\bCRITICAL\b/i.test(line)) badCritical = true; continue; } if (v && typeof v === 'object' && !Array.isArray(v) && 'matrix' in v) { const m = (v as { matrix: unknown }).matrix; if (packet.checklists.includes('state-machines') && m && typeof m === 'object') r.records++; else r.invalid++; continue; } const f = validFinding(v, packet.role); - if (!f) { r.invalid++; if ((v as { severity?: unknown } | null)?.severity === 'CRITICAL') badCritical = true; continue; } + if (!f) { r.invalid++; if (/^critical$/i.test(String((v as { severity?: unknown } | null)?.severity))) badCritical = true; continue; } r.findings.push({ ...f, specialist: str(f.specialist) ? f.specialist : packet.reviewer }); } r.status = badCritical ? 'invalid' : r.findings.length ? 'findings' : clean || r.records ? 'clean' : 'invalid'; diff --git a/lib/review-packet.ts b/lib/review-packet.ts index 8ca58d003..0ead52f70 100644 --- a/lib/review-packet.ts +++ b/lib/review-packet.ts @@ -87,9 +87,11 @@ export interface ReviewerDiff { stat: string; chunks: string[]; fixtureStat: str const DATA_FILE = /\.(json|jsonl|ndjson|csv|tsv)$/i; const MANIFEST_FILE = /^(package(-lock)?\.json|composer\.json|deno\.jsonc?|tsconfig[^/]*\.json|[^/]*\.config\.json)$/i; -function topLevelKeys(file: string): string { +/** Keys or columns read from the diff's own lines (added, else removed), never from the working tree, which a symlink can point outside the diff. */ +function topLevelKeys(file: string, changed: string[]): string { + const side = changed.some(l => l.startsWith('+')) ? '+' : '-'; + const text = changed.filter(l => l.startsWith(side)).map(l => l.slice(1)).join('\n'); try { - const text = fs.readFileSync(file, 'utf8'); if (/\.(csv|tsv)$/i.test(file)) return `columns: ${text.split('\n')[0]!.slice(0, 300)}`; const v = /\.json$/i.test(file) ? JSON.parse(text) : JSON.parse(text.split('\n').find(l => l.trim()) ?? 'null'); const keys = (o: unknown) => (o && typeof o === 'object' && !Array.isArray(o) ? Object.keys(o).slice(0, 30).join(', ') : ''); @@ -98,12 +100,12 @@ function topLevelKeys(file: string): string { } /** A large data-format file's chunk as a summary: its changed-line count, keys, and the first and last 40 changed lines. */ -function dataSummary(root: string, p: string, chunk: string): string | null { +function dataSummary(p: string, chunk: string): string | null { if (!DATA_FILE.test(p) || MANIFEST_FILE.test(path.basename(p))) return null; const changed = chunk.split('\n').filter(l => /^[+-]/.test(l) && !/^(\+\+\+|---) /.test(l)); if (changed.length <= DATA_SUMMARY_LINES) return null; return [`diff --git a/${p} b/${p}`, `DATA SUMMARY: ${p} (${changed.length} changed lines, a data file shown as a summary; ask the parent for a line range if a finding needs one)`, - topLevelKeys(path.join(root, p)), 'first 40 changed lines:', ...changed.slice(0, 40), '…', 'last 40 changed lines:', ...changed.slice(-40), ''].join('\n'); + topLevelKeys(p, changed), 'first 40 changed lines:', ...changed.slice(0, 40), '…', 'last 40 changed lines:', ...changed.slice(-40), ''].join('\n'); } export function reviewerDiff(root: string, diffBase: string, exclude: readonly string[], summary: boolean, skip: (p: string) => boolean, keepData: (p: string) => boolean = () => false): ReviewerDiff { @@ -125,7 +127,7 @@ export function reviewerDiff(root: string, diffBase: string, exclude: readonly s fixtureStat = [git(root, ['diff', '--stat=200', '--no-renames', diffBase, ...fx]).trimEnd(), ...listed.map(p => ` ${p} | untracked`)].filter(Boolean).join('\n') || '(no test or fixture changes)'; } const paths = chunks.map(c => /^diff --git a\/(.*?) b\//.exec(c)?.[1] ?? '?'); - const shown = chunks.map((c, i) => (keepData(paths[i]!) ? c : dataSummary(root, paths[i]!, c) ?? c)); + const shown = chunks.map((c, i) => (keepData(paths[i]!) ? c : dataSummary(paths[i]!, c) ?? c)); return { stat: stat || '(empty diff)', chunks: shown, fixtureStat, paths }; } diff --git a/test/review-packet.test.ts b/test/review-packet.test.ts index 8d3c3199b..a9d23fdfe 100644 --- a/test/review-packet.test.ts +++ b/test/review-packet.test.ts @@ -169,8 +169,6 @@ describe('gstack-review-packet build', () => { expect(build(r, ['--audit', 'plan-completion']).status).toBe(2); }); - // Value: protects=a Step 7/8 audit past the packet cap stays one packet holding the whole diff, so the parent gets one coverage or plan verdict and one generation allowance; fails_when=audits shard like reviewers (a plan item built in another shard reads NOT DONE, each shard generates tests); - // why_new=the sharding test covers reviewers only; seam=none; no_claim=how a child reads a packet past its context (it reads the file in ranges) // Value: protects=--base origin/main names origin/main in the audit prompt; fails_when=the prompt prefixes origin/ to a ref that already has it (git diff origin/origin/main); // why_new=every build test passes --base main with no origin remote; seam=none; no_claim=a base with no origin remote (covered: it stays as given) test('--base origin/main and --base main both put origin/main in the audit prompt, never origin/origin/main', () => { @@ -246,20 +244,46 @@ describe('gstack-review-packet build', () => { expect(ps.find(p => p.name === 'maintainability')!.text).not.toContain('"id": "d-700"'); }); + // Value: protects=a data summary is built from the diff alone, so no file outside the diff reaches a packet; fails_when=topLevelKeys reads the working tree and follows a symlink that replaced a large CSV; + // why_new=the #34 test adds plain files only; seam=none; no_claim=other symlink shapes git shows as one-line diffs + test('a large CSV replaced by a symlink to a private file: the summary never reads the link target', () => { + const secret = path.join(tmp('review-packet-secret-'), 'id_key'); + fs.writeFileSync(secret, 'PRIVATE-KEY-MATERIAL-7f3a,never,in,a,packet\n'); + const csv = 'id,name,score\n' + Array.from({ length: 500 }, (_, i) => `${i},row${i},${i % 7}`).join('\n') + '\n'; + const r = repo({ 'data/scores.csv': csv }, { 'src/a.ts': lines(5, 'nu') }); + fs.rmSync(path.join(r.dir, 'data/scores.csv')); + fs.symlinkSync(secret, path.join(r.dir, 'data/scores.csv')); + const b = build(r, ['--only', 'security,outside-voice']); + expect(b.status).toBe(0); + const ps = packets(b.stdout); + expect(ps.map(p => p.name).sort()).toEqual(['outside-voice', 'security']); + for (const p of ps) { + expect(p.text).toContain('DATA SUMMARY: data/scores.csv'); + expect(p.text).not.toContain('PRIVATE-KEY-MATERIAL'); + } + }); + // Value: protects=a partial build after the head moved drops the old run's reviewers and their outputs; fails_when=writeManifest keeps entries from another head, so stale clean results count as coverage of new code; // why_new=partial-build tests stay on one head; seam=none; no_claim=a working-tree edit at the same head beyond the one exercised here - test('a partial build keeps other reviewers only at the same head, base and content; otherwise their entries and outputs go', () => { + test('a partial build keeps other reviewers only at the same head, base and content; otherwise it starts over, and a Red Team build refuses', () => { const r = repo({}, { 'src/a.ts': lines(80, 'mu') }); const first = packets(build(r, ['--only', 'testing,maintainability']).stdout); for (const p of first) fs.writeFileSync(p.out, 'NO FINDINGS\n'); expect(build(r, ['--only', 'testing']).status).toBe(0); expect(readManifestOf(r.out).map(p => p.reviewer).sort()).toEqual(['maintainability', 'testing']); fs.writeFileSync(path.join(r.dir, 'src/a.ts'), lines(81, 'mu')); + fs.writeFileSync(path.join(r.out, 'merged.jsonl'), ''); + const red = build(r, ['--only', 'red-team', '--findings', path.join(r.out, 'merged.jsonl')]); + expect(red.status).toBe(3); + expect(red.stdout).toContain('STALE_MANIFEST: content changed'); + expect(readManifestOf(r.out).map(p => p.reviewer).sort()).toEqual(['maintainability', 'testing']); expect(build(r, ['--only', 'testing']).status).toBe(0); expect(readManifestOf(r.out).map(p => p.reviewer)).toEqual(['testing']); expect(fs.existsSync(first.find(p => p.name === 'maintainability')!.out)).toBe(false); }); + // Value: protects=a Step 7/8 audit past the packet cap stays one packet holding the whole diff, so the parent gets one coverage or plan verdict and one generation allowance; fails_when=audits shard like reviewers (a plan item built in another shard reads NOT DONE, each shard generates tests); + // why_new=the sharding test covers reviewers only; seam=none; no_claim=how a child reads a packet past its context (it reads the file in ranges) test('an audit past the cap is never sharded: one packet, every path of the diff', () => { const files: Record = {}; for (let i = 0; i < 30; i++) files[`src/f${i}.ts`] = lines(400, `v${i}_`); @@ -537,4 +561,12 @@ describe('gstack-review-packet collect', () => { expect(r.status).toBe(3); expect(r.stdout).toContain('REVIEWER: testing status=invalid'); }); + + // Value: protects=a CRITICAL in another case or on an unparseable line still makes the reviewer invalid; fails_when=only an exact, parseable severity "CRITICAL" trips the guard; + // why_new=the guard test above uses a parseable exact-case line; seam=none; no_claim=prose findings, which the contract already rejects + test.each(['{"severity":"Critical","confidence":8,"path":"a.ts","category":"x","summary":"s","specialist":"testing"}', '{"severity":"CRITICAL","path":"a.ts","summary":"trailing comma",}', '[CRITICAL] a.ts: prose blocker'])('a case-variant or unparseable CRITICAL line next to a valid finding makes the reviewer invalid: %s', bad => { + const r = collect(dir([{ role: 'specialist', name: 'testing' }], { testing: [finding({}), bad].join('\n') })); + expect(r.status).toBe(3); + expect(r.stdout).toContain('REVIEWER: testing status=invalid'); + }); }); From 1eda7b9c9e11a43dac1348492100208b0e108f4e Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 29 Oct 2026 21:30:30 +0000 Subject: [PATCH 4/4] docs(GSTA-167): CHANGELOG entry for reviewer packets; ship-budget names BUDGET_NEXT as the --next value Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 22 ++++++++++++++++++++++ docs/ship-budget.md | 5 ++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0970e47ac..019d85529 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ # Changelog +## [Unreleased] + +**/ship reviewers now read one packet file each, and the parent reads a findings record of at most 60 lines instead of every reviewer's transcript.** On this PR's own /ship, pass 2's eight reviewers cost 8.6M tokens on the root counter, against 52.6M for pass 1's eleven (pass 2 read the fix range, pass 1 the whole diff, so the two are not directly comparable). + +### Added +- **`gstack-review-packet build`** writes one packet per `DISPATCH:` line of the review plan: the role's instructions and findings contract, its checklists, that reviewer's diff, a plan excerpt, a read budget and the output path. Steps 7 and 8 audits, the specialists, the Red Team, the fix-check and Step 11's adversarial pass and outside voice all dispatch this way. A packet over 200 KB is split by path; no hunk is cut and no path is dropped. Audits are never split. +- Each `PACKET:` line names its `budget_role` and `read=` line ranges of at most 30,000 bytes, and build prints `BUDGET_NEXT: dispatch:` for `gstack-ship-budget check --next`. A reviewer reads its packet with parallel range reads and finishes in about three calls. +- **`gstack-review-packet collect`** parses, validates and dedupes every reviewer's output, prints one line per reviewer plus each CRITICAL, and writes `merged.jsonl` and `informational.md` for the PR body. Missing or unusable output exits 3 with `REVIEW_COVERAGE_INCOMPLETE`. + +### Changed +- Every reviewer prompt lives in `lib/review-prompts.ts`; the resolvers import it, so a packet and the generated SKILL.md prose cannot drift. /review's generated output is unchanged. +- Large `.json`, `.jsonl`, `.ndjson`, `.csv` and `.tsv` changes (over 400 changed lines, not package or config manifests) reach reviewers as a summary built from the diff alone. The testing reviewer and the coverage audit still read data under `test/` or `fixtures/` in full. +- The Capy dispatch paragraph in `capy/skill/gstack/SKILL.md` builds packets, starts reviewers on the shared machine, polls with `transcript: "none"` and returns only collect's output. + +### Fixed +- `--base origin/main` no longer becomes `origin/origin/main` in audit prompts. +- A partial build keeps other reviewers' results only at the same head, diff base and diff content; a Red Team build against a changed tree refuses with `STALE_MANIFEST`. +- Collect counts a state-machines matrix record only from the state-machines reviewer, and a CRITICAL line that fails to parse or validate makes its reviewer invalid instead of disappearing. + +### Tests +- `test/review-packet.test.ts`: 41 tests covering packet contents, sharding, audits, budget roles, read ranges, data summaries (including a symlinked CSV that must not leak its target), stale manifests and collect's validation. Each fix in this entry has a test that failed before it. + ## [1.91.324.0] - 2026-10-29 **A /ship token cap now projects each step and reviewer from what the run has actually spent, so real caps stop refusing work that costs a fraction of the old guess.** On GSTA-152 a 29.28M cap refused its third reviewer because every dispatch was projected at 7.43M while real ones cost 0.23 to 1.02M; on GSTA-2 an 8M cap stopped before Step 6 after Steps 1 to 5 cost about 0.27M each, because every step was projected at 2.25M. diff --git a/docs/ship-budget.md b/docs/ship-budget.md index d48b3dcdd..232c3c0f3 100644 --- a/docs/ship-budget.md +++ b/docs/ship-budget.md @@ -80,7 +80,10 @@ gstack-review-log --start review --reservation ``` Roles are review-depth's: `specialist`, `red-team`, `adversarial-native`, `outside-voice`, -`fix-check`. `--finish ` is the pass's completion receipt: it settles that reservation and +`fix-check`. /ship passes the `BUDGET_NEXT: dispatch:` value that the batch's +`gstack-review-packet build` prints, one role per packet: a Step 7/8 audit reserves as +`specialist` and a shard as its reviewer's role +([review-policy.md](review-policy.md#reviewer-packets)). `--finish ` is the pass's completion receipt: it settles that reservation and every other dispatch reservation started in the same step, so reviewers admitted one at a time do not stay reserved. Name a reviewer that came back in your next check (`check ... --returned [,...]`), or, when other work comes first, `settle --reservation