E4: address review round 2 on planning leftovers

- PlanningStorage types planning's leftovers apart from Ask's, so an
  Ask-policy RetainedStorage cannot be passed to the planning provider.
- removePlanningRoot deletes only planning roots under the temporary
  directory, and makes the read-only input directory writable first, also on
  retries.
- A failed request reports its own error followed by the cleanup failure.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: mchwang

runner/planning-provider.ts

@@ -1,7 +1,7 @@
 import { randomUUID } from 'node:crypto';
 import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
-import { join } from 'node:path';
+import { basename, dirname, join } from 'node:path';
 import type { InvocationContext, InvocationInput, InvocationResult, StopReason } from '../agents/contract.ts';
 import type { TaskFilesystems, TaskStorageLimits } from '../agents/container/storage.ts';
 import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts';
@@ -26,21 +26,37 @@ export interface PlanningProviderOptions {
   readonly deps: PlanningDependencies;
   /** Shared across providers in one process, so the image is built once. */
   readonly image?: { id?: string };
-  /** What earlier requests left behind, from `planningLeftovers()`. Invocations stay refused while any remains. */
-  readonly retained?: RetainedStorage;
+  /** What earlier requests left behind. Invocations stay refused while any of it remains. */
+  readonly retained?: PlanningStorage;
 }
 
 /**
  * Planning's leftovers: lane D allocations, allocations lane D returned no handle for, and host copies of the planned
  * code. Use one instance for planning and never share it with Ask, so a planning failure never turns Ask off.
  */
 export const PLANNING_LEFTOVERS: LeftoverPolicy = Object.freeze({
-  removePath: (path: string) => rmSync(path, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }),
+  removePath: removePlanningRoot,
   paths: (paths: readonly string[]) => `A copy of the planned code from an earlier request could not be deleted (${paths.join(', ')}). Planning stays off until it is deleted.`,
   untracked: () => 'A planning agent\'s setup or cleanup failed and was not confirmed, so codeboost cannot tell which Docker resources were left. Planning is off until codeboost restarts.',
   retained: (count: number) => `Planning storage from an earlier request could not be removed (${allocations(count)}). Planning stays off until Docker removes it. Check that Docker is running, then retry.`,
 });
-export const planningLeftovers = () => new RetainedStorage(PLANNING_LEFTOVERS);
+/** Planning's own leftovers. Typed apart from Ask's, so an instance with Ask's wording and removal cannot be passed in. */
+export class PlanningStorage extends RetainedStorage {
+  readonly feature = 'planning';
+  constructor() { super(PLANNING_LEFTOVERS); }
+}
+
+const ROOT_PREFIX = 'codeboost-planning-';
+/**
+ * Delete a planning root: a direct child of the temporary directory named with planning's prefix, and nothing else.
+ * Its input directory is made writable first, since the request left it read-only.
+ */
+export function removePlanningRoot(root: string): void {
+  if (dirname(root) !== tmpdir() || !basename(root).startsWith(ROOT_PREFIX))
+    throw new Error(`Refusing to remove a path that is not a planning root (${root}).`);
+  try { chmodSync(join(root, 'input'), 0o700); } catch { /* never created, or already removed */ }
+  rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 });
+}
 
 // Planning reads the code; it needs no room to write. The same ceilings as Ask.
 export const PLANNING_STORAGE: TaskStorageLimits = Object.freeze({
@@ -93,7 +109,7 @@ export interface PlanningProvider extends AuthorProvider {
   invoke(request: AuthorRequest, signal: AbortSignal): Promise<string>;
 }
 export function createPlanningProvider(options: PlanningProviderOptions): PlanningProvider {
-  const { vendor, deps } = options, image = options.image ?? {}, retained = options.retained ?? planningLeftovers();
+  const { vendor, deps } = options, image = options.image ?? {}, retained = options.retained ?? new PlanningStorage();
   const timeoutMs = options.timeoutMs ?? DEFAULT_TIMEOUT_MS;
   if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1) throw new Error('Planning timeout must be a positive integer.');
   return { async invoke(request: AuthorRequest, signal: AbortSignal): Promise<string> {
@@ -109,9 +125,9 @@ export function createPlanningProvider(options: PlanningProviderOptions): Planni
     const credential = planningCredential(vendor, deps.env);
     retained.release(deps.removeFilesystems);
     image.id ??= deps.buildImage(remaining());
-    const root = mkdtempSync(join(tmpdir(), 'codeboost-planning-'));
+    const root = mkdtempSync(join(tmpdir(), ROOT_PREFIX));
     const staging = join(root, 'staging'), input = join(root, 'input');
-    let filesystems: TaskFilesystems | undefined, failed = false;
+    let filesystems: TaskFilesystems | undefined, failure: { error: unknown } | undefined;
     try {
       mkdirSync(staging); mkdirSync(input);
       writeFileSync(join(input, 'schema.json'), request.schemaText, { mode: 0o444 });
@@ -147,20 +163,24 @@ export function createPlanningProvider(options: PlanningProviderOptions): Planni
       if (result.unreleased !== undefined) retained.markUntracked();
       return planningOutput(result, invocation);
     } catch (error) {
-      failed = true;
+      failure = { error };
       throw error;
     } finally {
       const failures: unknown[] = [];
       if (filesystems) try { deps.removeFilesystems(filesystems); } catch (error) { retained.retain(filesystems); failures.push(error); }
-      try { chmodSync(input, 0o700); } catch { /* input was never created */ }
-      try { PLANNING_LEFTOVERS.removePath(root); }
+      try { removePlanningRoot(root); }
       catch (error) {
         retained.retainPath(root);
         failures.push(new Error(`A copy of the planned code could not be deleted (${root}).`, { cause: error }));
       }
-      // A failed request keeps its own error: what cleanup left is retained above, and the next request names it. A
-      // successful one is refused rather than returned, as Ask does, so a plan is never kept from an unsettled run.
-      if (failures.length && !failed) throw new AggregateError(failures, 'Planning container cleanup did not settle.');
+      // What cleanup left is retained above, and the next request names it. A successful request is refused rather than
+      // returned, as Ask does, so a plan is never kept from an unsettled run. A failed one keeps its own message first.
+      if (failures.length) {
+        const text = (error: unknown) => error instanceof Error ? error.message : String(error);
+        throw failure
+          ? new AggregateError([failure.error, ...failures], `${text(failure.error)} Cleanup also did not settle: ${failures.map(text).join('; ')}`)
+          : new AggregateError(failures, 'Planning container cleanup did not settle.');
+      }
     }
   } };
 }

scripts/record-planning.ts

@@ -17,7 +17,7 @@ import { captureInvocation } from '../agents/contract.ts';
 import { AGENT_IMAGE, buildAgentImage, CLAUDE_VERSION } from '../agents/container/image.ts';
 import { prepareTaskFilesystems, removeTaskFilesystems } from '../agents/container/run.ts';
 import { createTaskClone } from '../git/clone.ts';
-import { createPlanningProvider, planningCredential, planningLeftovers, type PlanningDependencies } from '../runner/planning-provider.ts';
+import { createPlanningProvider, planningCredential, PlanningStorage, type PlanningDependencies } from '../runner/planning-provider.ts';
 import { credentialEnvironment, measureGitRepository } from '../runner/question-container.ts';
 import { planningCommandSha256, prepareRecording, RECORDING_FILES, RECORDING_KIND, recordingFilesSha256, sha256,
   type PlanningRecording } from '../test/fixtures/planning/recording-inputs.ts';
@@ -49,7 +49,7 @@ const git = (cwd: string, ...args: string[]) => execFileSync('git', ['-c', 'core
   { cwd, env: gitEnv, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim();
 
 const root = mkdtempSync(join(tmpdir(), 'codeboost-recording-'));
-const retained = planningLeftovers(), runnerOwner = randomBytes(16).toString('hex');
+const retained = new PlanningStorage(), runnerOwner = randomBytes(16).toString('hex');
 let failed = false;
 try {
   const repository = join(root, 'repository');

test/planning-provider.test.ts

@@ -1,12 +1,12 @@
-import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync } from 'node:fs';
+import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { afterEach, expect, it } from 'vitest';
 import type { InvocationHandle, InvocationInput, InvocationResult, StopReason } from '../agents/contract.ts';
 import type { AgentAdapterRequest } from '../agents/adapters/types.ts';
 import type { TaskFilesystems } from '../agents/container/storage.ts';
 import { SuggestionCoordinator } from '../core/planning-suggestions.ts';
-import { CODEX_PLANNING_REFUSED, createPlanningProvider, planningLeftovers, type PlanningDependencies } from '../runner/planning-provider.ts';
+import { CODEX_PLANNING_REFUSED, createPlanningProvider, PlanningStorage, removePlanningRoot, type PlanningDependencies } from '../runner/planning-provider.ts';
 import { RetainedStorage, StopError } from '../runner/question-container.ts';
 import { Store } from '../runner/store.ts';
 import { prepareRecording, recordingContext, recordingInput, recordingPreviousPlan } from './fixtures/planning/recording-inputs.ts';
@@ -47,7 +47,7 @@ function fakeDeps(result: Partial<InvocationResult> | 'hold' = {}, env: Record<s
   };
   return { deps, events, captured, started, cancels, settle: (value: Partial<InvocationResult>) => settle(value) };
 }
-const provider = (deps: PlanningDependencies, extra: { vendor?: 'claude' | 'codex'; retained?: RetainedStorage } = {}) =>
+const provider = (deps: PlanningDependencies, extra: { vendor?: 'claude' | 'codex'; retained?: PlanningStorage } = {}) =>
   createPlanningProvider({ vendor: extra.vendor ?? 'claude', repository: '/repo', head: HEAD, snapshotId: 'snapshot-1',
     runnerOwner: OWNER, deps, retained: extra.retained });
 const draftRequest = (requestId = 'b3c1f3b2-6a55-4d7e-9a47-0c7c8f6f1a01') => prepareRecording('draft', HEAD, requestId).request;
@@ -127,7 +127,7 @@ it.each([
 });
 
 it('refuses later requests after lane D reports resources it could not release', async () => {
-  const retained = planningLeftovers(), fake = fakeDeps({ unreleased: [], stopReason: 'capture-failure', exitCode: null });
+  const retained = new PlanningStorage(), fake = fakeDeps({ unreleased: [], stopReason: 'capture-failure', exitCode: null });
   await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal)).rejects.toThrow();
   const next = fakeDeps();
   await expect(provider(next.deps, { retained }).invoke(draftRequest('c4d2e4c3-7b66-4e8f-8b58-1d8d907f2b12'), new AbortController().signal))
@@ -136,7 +136,7 @@ it('refuses later requests after lane D reports resources it could not release',
 });
 
 it('marks an allocation lane D could not clean up as untracked, and refuses later requests', async () => {
-  const retained = planningLeftovers(), fake = fakeDeps();
+  const retained = new PlanningStorage(), fake = fakeDeps();
   fake.deps.prepareFilesystems = () => { throw new AggregateError([new Error('docker rm failed')], 'setup cleanup'); };
   await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal)).rejects.toThrow('setup cleanup');
   expect(retained.untracked).toBe(1);
@@ -146,14 +146,14 @@ it('marks an allocation lane D could not clean up as untracked, and refuses late
 });
 
 it('does not mark an ordinary setup failure as untracked', async () => {
-  const retained = planningLeftovers(), fake = fakeDeps();
+  const retained = new PlanningStorage(), fake = fakeDeps();
   fake.deps.prepareFilesystems = () => { throw new Error('no space'); };
   await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal)).rejects.toThrow('no space');
   expect(retained.untracked).toBe(0);
 });
 
 it('keeps storage Docker did not remove, refuses until it is removed, then runs again', async () => {
-  const retained = planningLeftovers(), fake = fakeDeps({ stdout: '{"plan":true}' });
+  const retained = new PlanningStorage(), fake = fakeDeps({ stdout: '{"plan":true}' });
   let removable = false;
   fake.deps.removeFilesystems = () => { if (!removable) throw new Error('docker volume rm failed'); };
   await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal))
@@ -168,31 +168,52 @@ it('keeps storage Docker did not remove, refuses until it is removed, then runs
 });
 
 it('keeps a failed request\'s own error when its cleanup also fails, and refuses the next request', async () => {
-  const retained = planningLeftovers(), fake = fakeDeps({ exitCode: 1, stdout: 'Invalid API key' });
+  const retained = new PlanningStorage(), fake = fakeDeps({ exitCode: 1, stdout: 'Invalid API key' });
   fake.deps.removeFilesystems = () => { throw new Error('docker volume rm failed'); };
   await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal))
-    .rejects.toThrow('Claude could not write the plan.');
+    .rejects.toThrow(/^Claude could not write the plan\..* Cleanup also did not settle: docker volume rm failed$/);
   expect(retained.size).toBe(1);
   await expect(provider(fake.deps, { retained }).invoke(draftRequest('a8b6c8a7-bfaa-4c23-8f9c-5bc1d4b26f56'), new AbortController().signal))
     .rejects.toThrow(/Planning storage from an earlier request could not be removed/);
 });
 
+/** A real planning root that rmSync cannot empty: its child directory sits in a read-only parent. */
+function lockedPlanningRoot(): string {
+  const root = mkdtempSync(join(tmpdir(), 'codeboost-planning-'));
+  mkdirSync(join(root, 'staging', 'child'), { recursive: true }); chmodSync(join(root, 'staging'), 0o500);
+  cleanups.push(() => chmodSync(join(root, 'staging'), 0o700)); roots.push(root);
+  return root;
+}
 const refusal = (retained: RetainedStorage) => {
   try { retained.release(() => { throw new Error('still there'); }); } catch (error) { return (error as Error).message; }
   throw new Error('release did not refuse');
 };
 it.each([
   ['an allocation Docker did not remove', (r: RetainedStorage) => r.retain({} as TaskFilesystems), /^Planning storage from an earlier request could not be removed \(1 allocation\)/],
   ['an allocation lane D returned no handle for', (r: RetainedStorage) => r.markUntracked(), /^A planning agent's setup or cleanup failed.*Planning is off until codeboost restarts\.$/],
-  ['a host copy that cannot be deleted', (r: RetainedStorage) => {
-    const parent = mkdtempSync(join(tmpdir(), 'planning-locked-')), root = join(parent, 'root');
-    mkdirSync(join(root, 'child'), { recursive: true }); chmodSync(root, 0o500);
-    roots.push(parent); cleanups.push(() => chmodSync(root, 0o700));
-    r.retainPath(join(root, 'child'));
-  }, /^A copy of the planned code from an earlier request could not be deleted \(.*child\)\. Planning stays off/],
+  ['a host copy that cannot be deleted', (r: RetainedStorage) => r.retainPath(lockedPlanningRoot()),
+    /^A copy of the planned code from an earlier request could not be deleted \(.*codeboost-planning-.*\)\. Planning stays off/],
 ] as const)('words the refusal for %s as planning, never as Ask', (_, leave, message) => {
-  const retained = planningLeftovers(); leave(retained);
+  const retained = new PlanningStorage(); leave(retained);
   const text = refusal(retained);
   expect(text).toMatch(message);
   expect(text).not.toMatch(/Ask|question/);
 });
+
+it('removes only planning roots, and makes a read-only input directory writable first', () => {
+  const foreign = mkdtempSync(join(tmpdir(), 'not-planning-')); roots.push(foreign);
+  expect(() => removePlanningRoot(foreign)).toThrow(/not a planning root/);
+  expect(existsSync(foreign)).toBe(true);
+  const nested = mkdtempSync(join(foreign, 'codeboost-planning-'));
+  expect(() => removePlanningRoot(nested)).toThrow(/not a planning root/);
+  const root = mkdtempSync(join(tmpdir(), 'codeboost-planning-')); roots.push(root);
+  mkdirSync(join(root, 'input')); writeFileSync(join(root, 'input', 'schema.json'), '{}'); chmodSync(join(root, 'input'), 0o555);
+  removePlanningRoot(root);
+  expect(existsSync(root)).toBe(false);
+});
+
+it('accepts only planning storage, never Ask\'s', () => {
+  const fake = fakeDeps();
+  // @ts-expect-error Ask's RetainedStorage words refusals for Ask and cannot remove planning roots.
+  createPlanningProvider({ vendor: 'claude', repository: '/repo', head: HEAD, snapshotId: 's', runnerOwner: OWNER, deps: fake.deps, retained: new RetainedStorage() });
+});