From a1f0d08ccda279bfdbe57947f51a16d700a1c1ee Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 17 Oct 2026 01:13:45 +0000 Subject: [PATCH] fix(autoplan): export refuses artifact paths outside the run before writing; a deferred close keeps earlier undecided findings; reopening CEO grants Eng its rerun (review of #3234) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/autoplan-budget.ts | 10 +++++-- lib/autoplan-gate.ts | 18 ++++++------- lib/autoplan-public.ts | 7 +++-- test/autoplan-budget.test.ts | 42 ++++++++++++++++++++++++++++- test/autoplan-public-export.test.ts | 19 +++++++++++++ 5 files changed, 82 insertions(+), 14 deletions(-) diff --git a/lib/autoplan-budget.ts b/lib/autoplan-budget.ts index 37ac60edd..e87189a8e 100644 --- a/lib/autoplan-budget.ts +++ b/lib/autoplan-budget.ts @@ -134,7 +134,8 @@ export const pendingDeltas = (state: RunState) => (state.capped_deltas ?? []).fi /** * The owner's answers to cap items (deferred closes, deferred blockers, capped deltas) once the plan proceeds: - * `b` grants that phase one more round, journaled as a budget change, and adds it (and Eng) to the reopen set; + * `b` grants that phase one more round, journaled as a budget change, and adds it to the reopen set; Eng, which + * reruns last, gets its own grant unless it already has one; * `a` accepts. Capped deltas record which. */ export function applyCapChoices(state: RunState, choices: Array<{ item: GateItem; chosen: string }>, reopen: Set, by: string | undefined): string[] { @@ -150,8 +151,13 @@ export function applyCapChoices(state: RunState, choices: Array<{ item: GateItem (state.round_grants ??= {})[cap.phase] = (state.round_grants[cap.phase] ?? 0) + 1; journal(state.out, { event: 'budget_changed', by: by ?? 'gate', round_grant: { phase: cap.phase, cap: roundCap(state, cap.phase) }, item: item.id }); reopen.add(cap.phase); - if (state.phase_order.includes('eng')) reopen.add('eng'); lines.push(`ROUND_GRANTED: ${cap.phase} cap=${roundCap(state, cap.phase)} (${item.id}b by ${by ?? 'gate'})`); + if (cap.phase === 'eng' || !state.phase_order.includes('eng') || granted.has('eng')) continue; + granted.add('eng'); + state.round_grants.eng = (state.round_grants.eng ?? 0) + 1; + journal(state.out, { event: 'budget_changed', by: by ?? 'gate', round_grant: { phase: 'eng', cap: roundCap(state, 'eng') }, item: item.id, reason: `eng reruns after ${cap.phase}` }); + reopen.add('eng'); + lines.push(`ROUND_GRANTED: eng cap=${roundCap(state, 'eng')} (reruns after ${cap.phase}; ${item.id}b by ${by ?? 'gate'})`); } return lines; } diff --git a/lib/autoplan-gate.ts b/lib/autoplan-gate.ts index 6270e7925..0c831028c 100644 --- a/lib/autoplan-gate.ts +++ b/lib/autoplan-gate.ts @@ -74,7 +74,7 @@ type OpenRow = FindingRow & { urgent?: boolean; user_challenge?: boolean; settle * Open voices become skipped. Their dispatched or execution-unknown attempts are settled `unknown` at their * reservation (a deferred review may still have run and been billed); only reservations with no journaled dispatch * are released. The consensus file is written from the bound voices so export, validate and resume work. Open - * findings (this round's bound rows, else the rows of the round last reviewed, minus answered ones) carry into + * findings (this round's bound rows plus the unanswered rows of the round last reviewed) carry into * tasks.jsonl and the gate; Critical, urgent and user-challenge findings become owner approval items. */ export function closeDeferred(state: RunState, phase: Phase, o: { reason: string; by: string; cause: 'cap' | 'owner' }): { lines: string[]; blockers: string[] } { @@ -95,15 +95,15 @@ export function closeDeferred(state: RunState, phase: Phase, o: { reason: string const consensus = consensusRecord(phase, ps.native?.status === 'completed' ? { rows: native, model: ps.native.model } : null, ps.outside?.status === 'completed' ? { rows: outside, model: ps.outside.model, provider: ps.outside.provider } : null, reconcile(native, outside, [])); const answered = settledIds(state); - let rows: OpenRow[] = [...native, ...outside]; + const current: OpenRow[] = [...native, ...outside]; + const ids = new Set(current.map(r => r.id)); + const earlier: OpenRow[] = reviewRows(state, previousReview(state, phase)).filter(r => !answered.has(r.id) && !ids.has(r.id)); + const rows = [...current, ...earlier]; let carried: string | undefined; - if (!rows.length) { - rows = reviewRows(state, previousReview(state, phase)).filter(r => !answered.has(r.id)); - if (rows.length) { - carried = `${phase}-deferred.findings.jsonl`; - fs.writeFileSync(path.join(state.out, carried), rows.map(r => JSON.stringify({ ...r, disposition: 'deferred' })).join('\n') + '\n'); - state.artifacts[carried] = { path: carried, sha256: sha256File(path.join(state.out, carried)) }; - } + if (earlier.length) { + carried = `${phase}-deferred.findings.jsonl`; + fs.writeFileSync(path.join(state.out, carried), earlier.map(r => JSON.stringify({ ...r, disposition: 'deferred' })).join('\n') + '\n'); + state.artifacts[carried] = { path: carried, sha256: sha256File(path.join(state.out, carried)) }; } const open = rows.filter(r => !r.settled_by && !answered.has(r.id)); const counts = Object.fromEntries(['Critical', 'High', 'Medium', 'Low', 'Informational'].map(s => [s, open.filter(r => r.severity === s).length])); diff --git a/lib/autoplan-public.ts b/lib/autoplan-public.ts index 05292d19b..2e1c94404 100644 --- a/lib/autoplan-public.ts +++ b/lib/autoplan-public.ts @@ -147,10 +147,11 @@ export function exportPublic(state: RunState, dir: string, env: NodeJS.ProcessEn try { const files: Provenance['files'] = {}; for (const [name, entry] of Object.entries(state.artifacts)) { - const src = path.join(out, entry.path); + const src = path.resolve(out, entry.path); + const dest = path.resolve(staging, entry.path); + if (!src.startsWith(out + path.sep) || !dest.startsWith(staging + path.sep)) throw new RunError('ARTIFACT_PATH_ESCAPE', `artifact ${name} path ${JSON.stringify(entry.path)} leaves the run directory; nothing was written`, 3); if (!fs.existsSync(src) || sha256File(src) !== entry.sha256) throw new RunError('ARTIFACT_STALE', `${entry.path} does not match run.json; run gstack-autoplan export --out ${out} first`, 3); const text = fs.readFileSync(src, 'utf8'); - const dest = path.join(staging, entry.path); fs.mkdirSync(path.dirname(dest), { recursive: true }); fs.writeFileSync(dest, name === 'plan.md' ? rewriteProse(text, roots) : entry.path.endsWith('.json') ? JSON.stringify(rewriteJson(JSON.parse(text), roots), null, 2) + '\n' : rewriteRoots(text, roots)); files[name] = { private_sha256: entry.sha256, public_sha256: sha256File(dest) }; @@ -160,6 +161,8 @@ export function exportPublic(state: RunState, dir: string, env: NodeJS.ProcessEn manifest.provenance = { private_run: state.run, private_manifest_sha256: sha256File(privateRun), exported_at: new Date().toISOString(), roots: roots.map(r => r.label), files }; fs.writeFileSync(path.join(staging, FILES.run), JSON.stringify(manifest, null, 2) + '\n'); const check = validatePublic(staging, privateRun, env); + const escapes = check.errors.filter(e => e.code === 'ARTIFACT_PATH_ESCAPE'); + if (escapes.length) throw new RunError('ARTIFACT_PATH_ESCAPE', escapes.map(e => `${e.path}: ${e.message}`).join('; '), 3); const leaks = check.errors.filter(e => e.code === 'ARTIFACT_MACHINE_PATH'); if (leaks.length) throw new RunError('ARTIFACT_MACHINE_PATH', leaks.slice(0, 5).map(e => `${e.path}: ${e.message}`).join('; ') + (leaks.length > 5 ? ` (+${leaks.length - 5} more)` : ''), 3); fs.rmSync(target, { recursive: true, force: true }); diff --git a/test/autoplan-budget.test.ts b/test/autoplan-budget.test.ts index 713418f87..83f512768 100644 --- a/test/autoplan-budget.test.ts +++ b/test/autoplan-budget.test.ts @@ -127,7 +127,8 @@ describe('close --deferred', () => { expect(reply.status, reply.stderr).toBe(0); expect(reply.stdout).toMatch(/^ROUND_GRANTED: ceo cap=4 \(uc\d+b by human\)$/m); expect(reply.stdout).toMatch(/^REOPENED: ceo,eng/m); - expect(readRunJson(s.out).round_grants).toEqual({ ceo: 1 }); + expect(reply.stdout).toMatch(/^ROUND_GRANTED: eng cap=4 \(reruns after ceo; uc\d+b by human\)$/m); + expect(readRunJson(s.out).round_grants).toEqual({ ceo: 1, eng: 1 }); }); }); @@ -177,6 +178,45 @@ describe('the round cap', () => { }); }); +describe('deferred close keeps earlier open findings; a reopened phase reruns Eng', () => { + test('a deferred confirm round with a new finding still carries the earlier undecided Critical to the gate and tasks', () => { + const s = start(); + phaseVia(r, s.out, 'ceo', { native: [{ severity: 'Critical', title: 'Retries can double-charge', fix: 'Add a durable idempotency key' }], outside: [] }); + phaseVia(r, s.out, 'eng', NONE); + expect(answer(s.out, readRunJson(s.out).gate_rev, 'p1d').status).toBe(0); + expect(r.ap(['next', '--out', s.out]).status).toBe(0); + const rr = readRunJson(s.out); + const review = r.review(path.join(s.out, 'ceo-native-prompt.md'), { FIXTURE_FINDINGS: JSON.stringify([{ severity: 'Medium', title: 'Jitter source is unspecified' }]) }); + expect(review.status, review.stderr).toBe(0); + const sub = r.ap(['submit', '--out', s.out, '--phase', 'ceo', '--voice', 'native', '--result', rr.phases.ceo.native.attempts.at(-1).result, '--model', 'anthropic/claude-opus-5-5', '--attempt', rr.phases.ceo.native.attempt]); + expect(sub.status, sub.stderr).toBe(0); + const closed = r.ap(['close', '--out', s.out, '--phase', 'ceo', '--deferred', 'owner pauses remaining reviews', '--by', 'human']); + expect(closed.status, closed.stderr).toBe(0); + expect(closed.stdout).toMatch(/^PHASE_DEFERRED: ceo round=2\/\S+ by=human open=2 \(Critical=1 Medium=1\) blockers=\S+-ceo-native-1 /m); + phaseVia(r, s.out, 'eng', NONE); + expect(gate(s.out).items.filter((i: any) => i.cap?.kind === 'blocker').map((i: any) => i.title)).toEqual([expect.stringContaining('Retries can double-charge')]); + expect(readJsonl(path.join(s.out, 'tasks.jsonl')).map((t: any) => t.priority).sort()).toEqual(['Critical', 'Medium']); + }); + + test('reopening a capped CEO round also grants Eng its round, so Eng reruns last instead of deferring', () => { + const s = start(['--max-rounds', '1']); + phaseVia(r, s.out, 'ceo', NONE); + phaseVia(r, s.out, 'eng', NONE); + expect(r.ap(['revise', '--out', s.out, '--plan', v2File(s.dir)]).status).toBe(0); + const items = gate(s.out).items; + const ceo = items.find((i: any) => i.cap?.kind === 'delta' && i.phase === 'ceo'); + const eng = items.find((i: any) => i.cap?.kind === 'delta' && i.phase === 'eng'); + const replied = answer(s.out, readRunJson(s.out).gate_rev, `${ceo.id}b ${eng.id}a p1a`); + expect(replied.status, replied.stderr).toBe(0); + expect(replied.stdout).toMatch(/^ROUND_GRANTED: eng cap=2 \(reruns after ceo; /m); + phaseVia(r, s.out, 'ceo', NONE); + const next = r.ap(['next', '--out', s.out]); + expect(next.status, next.stderr).toBe(0); + expect(next.stdout).toMatch(/^PHASE: eng /m); + expect(next.stdout).not.toContain('PHASE_DEFERRED: eng'); + }); +}); + describe('confirm rounds', () => { test('a reopened phase reviews the delta; a re-raised settled challenge is recorded settled unless it cites a changed line', () => { const s = start(); diff --git a/test/autoplan-public-export.test.ts b/test/autoplan-public-export.test.ts index 3ff7c62fe..3ff0af115 100644 --- a/test/autoplan-public-export.test.ts +++ b/test/autoplan-public-export.test.ts @@ -107,6 +107,25 @@ describe('export --public', () => { }); }); +describe('export --public path containment', () => { + test('an artifact path that leaves the run directory refuses before anything is written, outside or inside --public', () => { + const { out, dir } = gatedRun(); + const text = 'synthetic artifact text\n'; + fs.writeFileSync(path.join(dir, 'sentinel.txt'), text); + const victim = path.join(r.host.base, `victim${n}.txt`); + fs.writeFileSync(victim, 'unrelated owner file\n'); + const state = readRunJson(out); + state.artifacts['escape.txt'] = { path: `../../${path.basename(victim)}`, sha256: crypto.createHash('sha256').update(text).digest('hex') }; + fs.writeFileSync(path.join(out, 'run.json'), JSON.stringify(state) + '\n'); + const pub = path.join(dir, 'public'); + const x = r.ap(['export', '--out', out, '--public', pub]); + expect(x.status, x.stdout + x.stderr).toBe(3); + expect(x.stderr).toContain('ARTIFACT_PATH_ESCAPE'); + expect(fs.readFileSync(victim, 'utf8')).toBe('unrelated owner file\n'); + expect(fs.existsSync(pub)).toBe(false); + }); +}); + describe('rewriteRoots', () => { test('a worktree path becomes repo-relative; a local root nested in the worktree still reads as local notes', () => { const roots = [{ label: 'run', root: '/w/repo/.context/run' }, { label: 'home', root: '/home/u' }, { label: 'worktree', root: '/w/repo' }];