diff --git a/docs/implementation/planning-acceptance.md b/docs/implementation/planning-acceptance.md index 41c4c7cb..aca2f0fd 100644 --- a/docs/implementation/planning-acceptance.md +++ b/docs/implementation/planning-acceptance.md @@ -67,12 +67,15 @@ tests therefore show which check rejects the text: at exactly 1 MiB the schema, and at 1 MiB + 1 byte the size limit. If a real output fails validation, fix the prompt or schema and record again. Do not edit the recording. -Left for F's wiring: the provider's image build, clone and storage calls are +The provider and Ask share one container runner, `runReadOnlyAgent` in +`runner/question-container.ts`. Each passes its phase, host root and removal, +limits, credential rule, output check and wording as a `ReadOnlyFeature` (`ASK_FEATURE`, `PLANNING_FEATURE`). Ask's wording +and cleanup error are unchanged (#117 part 1). + +Left for #117 part 2: the provider's image build, clone and storage calls are synchronous, so a server must run it off its request thread, as Ask does. That worker should also set TMPDIR to a planning root, as Ask's does, so the input and -auth staging that lane D leaves when it gives up on cleanup is covered. At that -point the provider and `askInContainer` should share one container runner; they -are separate copies today. E3's +auth staging that lane D leaves when it gives up on cleanup is covered. E3's default 120-second suggestion timeout is shorter than the provider's ten-minute budget. Startup recovery does not yet remove planning's Docker resources after a crash, because they are not attempt allocations in the Store. The recording diff --git a/runner/planning-provider.ts b/runner/planning-provider.ts index e598ac64..d1267e52 100644 --- a/runner/planning-provider.ts +++ b/runner/planning-provider.ts @@ -1,11 +1,11 @@ -import { randomUUID } from 'node:crypto'; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; 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 { InvocationInput, InvocationResult, StopReason } from '../agents/contract.ts'; +import type { TaskStorageLimits } from '../agents/container/storage.ts'; import type { AuthorProvider, AuthorRequest } from '../core/planning-author.ts'; -import { allocations, RetainedStorage, stopOf, type ContainerDependencies, type LeftoverPolicy, type Provider } from './question-container.ts'; +import { allocations, exceedsStorage, RetainedStorage, runReadOnlyAgent, sameContext, type ContainerDependencies, type LeftoverPolicy, type Provider, + type ReadOnlyFeature, type RepositorySize } from './question-container.ts'; /** Ask's lane D dependencies, less startup recovery, which the runner owns for planning. */ export type PlanningDependencies = Omit; @@ -53,9 +53,10 @@ const ROOT_PREFIX = 'codeboost-planning-'; */ 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}).`); + throw new Error(`A copy of the planned code could not be deleted (${root}): it is not a planning root.`); try { chmodSync(join(root, 'input'), 0o700); } catch { /* never created, or already removed */ } - rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); + try { rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 }); } + catch (error) { throw new Error(`A copy of the planned code could not be deleted (${root}).`, { cause: error }); } } // Planning reads the code; it needs no room to write. The same ceilings as Ask. @@ -79,9 +80,6 @@ const stopMessages: Record = { cancelled: 'Planning agent cancelled.', timeout: 'Planning agent timed out.', shutdown: 'Server stopped during planning.', 'output-limit': 'Planning agent output exceeded its limit.', 'capture-failure': 'Planning agent output could not be captured.', }; -const sameContext = (left: InvocationContext, right: InvocationContext) => - (Object.keys(right) as (keyof InvocationContext)[]).every(key => left[key] === right[key]) - && Object.keys(left).length === Object.keys(right).length; /** Accept only this exact invocation's output, and only after a clean exit. The text is still unvalidated. */ export function planningOutput(result: InvocationResult, invocation: InvocationInput): string { if (result.attemptId !== invocation.attemptId || !result.context || !sameContext(result.context, invocation.context)) @@ -96,6 +94,22 @@ export function planningOutput(result: InvocationResult, invocation: InvocationI return result.stdout; } +const text = (error: unknown) => error instanceof Error ? error.message : String(error); +/** Planning's part of a read-only run (see `runReadOnlyAgent`). */ +export const PLANNING_FEATURE: ReadOnlyFeature = Object.freeze({ + phase: 'planning', rootPrefix: ROOT_PREFIX, removeRoot: removePlanningRoot, storage: PLANNING_STORAGE, + credential: planningCredential, timedOut: stopMessages.timeout, + assertFits: (size: RepositorySize) => { + if (exceedsStorage(size, PLANNING_STORAGE)) throw new Error('The repository is too large for planning.'); + }, + output: (_vendor: Provider, result: InvocationResult, invocation: InvocationInput) => planningOutput(result, invocation), + // What cleanup left is retained, 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. + cleanupFailed: (failures: readonly unknown[], failure?: { error: unknown }) => 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.'), +}); + /** * E2's provider over lane D: each request runs in a fresh container on a read-only copy of `head`, in the "planning" * phase (read, list and search only; no commands) with vendor-only egress. The request's schema is the only file in @@ -114,73 +128,12 @@ export function createPlanningProvider(options: PlanningProviderOptions): Planni if (!Number.isSafeInteger(timeoutMs) || timeoutMs < 1) throw new Error('Planning timeout must be a positive integer.'); return { async invoke(request: AuthorRequest, signal: AbortSignal): Promise { if (request.phase !== 'planning' || request.access !== 'read-only') throw new Error('Planning requests must be read-only.'); - const deadline = Date.now() + timeoutMs; - const remaining = () => { - signal.throwIfAborted(); - const value = deadline - Date.now(); - if (value < 1) throw new Error(stopMessages.timeout); - return value; - }; signal.throwIfAborted(); - const credential = planningCredential(vendor, deps.env); - retained.release(deps.removeFilesystems); - image.id ??= deps.buildImage(remaining()); - const root = mkdtempSync(join(tmpdir(), ROOT_PREFIX)); - const staging = join(root, 'staging'), input = join(root, 'input'); - let filesystems: TaskFilesystems | undefined, failure: { error: unknown } | undefined; - try { - mkdirSync(staging); mkdirSync(input); - writeFileSync(join(input, 'schema.json'), request.schemaText, { mode: 0o444 }); - chmodSync(input, 0o555); - // The clone is a full host copy with no byte limit of its own, so the repository must fit before it is made. - const size = deps.measureRepository(options.repository, options.head, Math.min(60_000, remaining())); - if (size.checkoutBytes > PLANNING_STORAGE.workBytes || size.entries > PLANNING_STORAGE.workInodes - || size.objectBytes > PLANNING_STORAGE.metadataBytes) - throw new Error('The repository is too large for planning.'); - const clone = deps.createClone({ source: options.repository, parent: staging, taskId: `planning-${request.requestId}`, - head: options.head, timeoutMs: Math.min(120_000, remaining()) }); - const owner = { runnerOwner: options.runnerOwner, attemptId: request.requestId, allocationId: randomUUID() }; - try { filesystems = deps.prepareFilesystems(clone, PLANNING_STORAGE, image.id, owner, Math.min(60_000, remaining())); } - catch (error) { - // D throws an AggregateError only when a failed allocation's own cleanup did not settle; it returns no handle. - if (error instanceof AggregateError) retained.markUntracked(); - throw error; - } - remaining(); - const invocation = deps.capture({ clone, phase: 'planning', vendor: 'claude', approvedArgv: [], deadline, - attemptId: request.requestId, runnerOwner: options.runnerOwner, - context: { snapshotId: options.snapshotId, planId: request.identity.planId, planRevision: request.revision, - assignmentId: `${request.mode}-${request.issue}`, referencedCodeHash: options.head, stateVersion: 0 } }); - const adapterRequest = { invocation, filesystems, inputDirectory: input, imageId: image.id, prompt: request.prompt, - networkAllocationId: randomUUID() }; - const handle = deps.startClaude(adapterRequest, credential); - const cancel = () => handle.cancel(stopOf(signal.reason)); - if (signal.aborted) cancel(); else signal.addEventListener('abort', cancel, { once: true }); - let result: InvocationResult; - try { result = await handle.settled; } - finally { signal.removeEventListener('abort', cancel); } - // D stopped before confirming cleanup; the resources are found again by their labels after a restart. - if (result.unreleased !== undefined) retained.markUntracked(); - return planningOutput(result, invocation); - } catch (error) { - failure = { error }; - throw error; - } finally { - const failures: unknown[] = []; - if (filesystems) try { deps.removeFilesystems(filesystems); } catch (error) { retained.retain(filesystems); failures.push(error); } - 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 })); - } - // 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.'); - } - } + return runReadOnlyAgent(PLANNING_FEATURE, { provider: vendor, repository: options.repository, head: options.head, + runnerOwner: options.runnerOwner, attemptId: request.requestId, taskId: `planning-${request.requestId}`, + deadline: Date.now() + timeoutMs, prompt: request.prompt, schemaText: request.schemaText, + context: { snapshotId: options.snapshotId, planId: request.identity.planId, planRevision: request.revision, + assignmentId: `${request.mode}-${request.issue}`, referencedCodeHash: options.head, stateVersion: 0 } }, + deps, signal, image, retained); } }; } diff --git a/runner/question-container.ts b/runner/question-container.ts index 64d95e12..b1e0e0fe 100644 --- a/runner/question-container.ts +++ b/runner/question-container.ts @@ -114,9 +114,11 @@ export function measureGitRepository(source: string, head: string, timeoutMs: nu return { checkoutBytes, entries, objectBytes }; } /** Refuse a repository whose staging copy would exceed the question's storage, before any host copy is made. */ +/** Whether a repository's checkout or Git objects would not fit `limits`. */ +export const exceedsStorage = (size: RepositorySize, limits: TaskStorageLimits) => + size.checkoutBytes > limits.workBytes || size.entries > limits.workInodes || size.objectBytes > limits.metadataBytes; export function assertFitsQuestionStorage(size: RepositorySize): void { - if (size.checkoutBytes > QUESTION_STORAGE.workBytes || size.entries > QUESTION_STORAGE.workInodes - || size.objectBytes > QUESTION_STORAGE.metadataBytes) + if (exceedsStorage(size, QUESTION_STORAGE)) throw new Error(`The repository is too large for Ask (checkout ${Math.ceil(size.checkoutBytes / 1048576)} MiB in ${size.entries} entries, Git objects ${Math.ceil(size.objectBytes / 1048576)} MiB; the limit is ${QUESTION_STORAGE.workBytes / 1048576} MiB and ${QUESTION_STORAGE.workInodes} entries).`); } @@ -182,7 +184,7 @@ const stopMessages: Record = { cancelled: 'Agent cancelled.', timeout: 'Agent timed out. Try again.', shutdown: 'Server stopped. Retry the question.', 'output-limit': 'Agent output exceeded its limit.', 'capture-failure': 'The agent container failed. Try again.', }; -const sameContext = (left: InvocationContext, right: InvocationContext) => +export const sameContext = (left: InvocationContext, right: InvocationContext) => (Object.keys(right) as (keyof InvocationContext)[]).every(key => left[key] === right[key]) && Object.keys(left).length === Object.keys(right).length; /** Accept only the result of this exact invocation, and only a clean exit. */ @@ -201,46 +203,86 @@ export function answerFromResult(provider: Provider, result: InvocationResult, i } /** - * Answer one question inside the lane D container: a read-only `/work` checkout of the reviewed head, - * the "questions" phase (read, list and search only; no commands), and vendor-only network access. - * Every step is bounded by `deadline`. Storage is released only after the invocation settles. + * What differs between features that run one read-only agent in lane D's container (Ask, planning): the phase, the + * host root and its removal, limits, the credential rule, the output check, and the wording a person sees. Everything + * else is `runReadOnlyAgent`. */ -export async function askInContainer(question: ContainerQuestion, deps: ContainerDependencies, - signal: AbortSignal, image: { id?: string } = {}, retained = new RetainedStorage()): Promise { +export interface ReadOnlyFeature { + readonly phase: 'questions' | 'planning'; + /** Prefix of the host root, under tmpdir(), that holds the clone's staging and the input mount. */ + readonly rootPrefix: string; + /** Deletes one of this feature's roots and refuses any other path. Throws while the root is still on disk. */ + removeRoot(root: string): void; + readonly storage: TaskStorageLimits; + /** The credential for `provider`, or a refusal. Runs before any Docker or Git work. */ + credential(provider: Provider, env: ContainerDependencies['env']): string; + /** Throws when the repository at `head` would not fit `storage`. Runs before any host copy is made. */ + assertFits(size: RepositorySize): void; + /** Shown when the deadline passes during setup. */ + readonly timedOut: string; + /** This exact invocation's text after a clean exit, or a refusal. The text is still unvalidated. */ + output(provider: Provider, result: InvocationResult, invocation: InvocationInput): string; + /** What a run throws when its cleanup did not settle. `failure` holds the run's own error, when it had one. */ + cleanupFailed(failures: readonly unknown[], failure?: { error: unknown }): Error; +} +/** One read-only invocation: whose code, which attempt, and what the agent is asked. */ +export interface ReadOnlyRun { + readonly provider: Provider; + /** Host checkout to clone. The agent sees a read-only copy of `head`, never this directory. */ + readonly repository: string; + readonly head: string; + /** Written as `io.codeboost.runner` on every Docker object this run creates. */ + readonly runnerOwner: string; + readonly attemptId: string; + readonly taskId: string; + readonly deadline: number; + readonly context: InvocationContext; + readonly prompt: string; + /** The only file in the input mount. Claude planning receives it as `--json-schema`. */ + readonly schemaText: string; +} + +/** + * Run one read-only agent in lane D's container: a fresh container on a read-only copy of `head`, with vendor-only + * egress and the feature's phase. Resources are released on every path; what Docker or the host did not confirm + * removed is kept in `retained`, and the feature stays off while any remains. + * + * Lane D's image build, clone and storage allocation are synchronous Docker and Git calls, so a serving process runs + * this in a worker thread whose TMPDIR is the feature's own root, as Ask's worker does. + */ +export async function runReadOnlyAgent(feature: ReadOnlyFeature, run: ReadOnlyRun, deps: Omit, + signal: AbortSignal, image: { id?: string }, retained: RetainedStorage): Promise { const remaining = () => { signal.throwIfAborted(); - const value = question.deadline - Date.now(); - if (value < 1) throw new Error('Agent timed out. Try again.'); + const value = run.deadline - Date.now(); + if (value < 1) throw new Error(feature.timedOut); return value; }; - const credential = questionCredential(question.provider, deps.env); + const credential = feature.credential(run.provider, deps.env); retained.release(deps.removeFilesystems); image.id ??= deps.buildImage(remaining()); - const root = mkdtempSync(join(tmpdir(), 'codeboost-question-')); + const root = mkdtempSync(join(tmpdir(), feature.rootPrefix)); const staging = join(root, 'staging'), input = join(root, 'input'); - let filesystems: TaskFilesystems | undefined; + let filesystems: TaskFilesystems | undefined, failure: { error: unknown } | undefined; try { mkdirSync(staging); mkdirSync(input); - writeFileSync(join(input, 'schema.json'), ANSWER_SCHEMA, { mode: 0o444 }); + writeFileSync(join(input, 'schema.json'), run.schemaText, { mode: 0o444 }); chmodSync(input, 0o555); // The clone is a full host copy with no byte limit of its own, so the repository must fit before it is made. - assertFitsQuestionStorage(deps.measureRepository(question.repository, question.head, Math.min(60_000, remaining()))); - const clone = deps.createClone({ source: question.repository, parent: staging, taskId: `question-${question.noteId}`, - head: question.head, timeoutMs: Math.min(120_000, remaining()) }); - const owner = { runnerOwner: question.runnerOwner, attemptId: question.attemptId, allocationId: randomUUID() }; - try { filesystems = deps.prepareFilesystems(clone, QUESTION_STORAGE, image.id, owner, Math.min(60_000, remaining())); } + feature.assertFits(deps.measureRepository(run.repository, run.head, Math.min(60_000, remaining()))); + const clone = deps.createClone({ source: run.repository, parent: staging, taskId: run.taskId, + head: run.head, timeoutMs: Math.min(120_000, remaining()) }); + const owner = { runnerOwner: run.runnerOwner, attemptId: run.attemptId, allocationId: randomUUID() }; + try { filesystems = deps.prepareFilesystems(clone, feature.storage, image.id, owner, Math.min(60_000, remaining())); } catch (error) { // D throws an AggregateError only when a failed allocation's own cleanup did not settle; it returns no handle. if (error instanceof AggregateError) retained.markUntracked(); throw error; } remaining(); - const invocation = deps.capture({ clone, phase: 'questions', vendor: question.provider, approvedArgv: [], - deadline: question.deadline, attemptId: question.attemptId, runnerOwner: question.runnerOwner, - context: { snapshotId: question.snapshotId, planId: question.planId, planRevision: question.planRevision, - assignmentId: question.noteId, referencedCodeHash: question.contextId, - stateVersion: 0 } }); - const request = { invocation, filesystems, inputDirectory: input, imageId: image.id, prompt: question.prompt, + const invocation = deps.capture({ clone, phase: feature.phase, vendor: run.provider, approvedArgv: [], + deadline: run.deadline, attemptId: run.attemptId, runnerOwner: run.runnerOwner, context: run.context }); + const request = { invocation, filesystems, inputDirectory: input, imageId: image.id, prompt: run.prompt, networkAllocationId: randomUUID() }; const handle = deps.startClaude(request, credential); const cancel = () => handle.cancel(stopOf(signal.reason)); @@ -248,21 +290,47 @@ export async function askInContainer(question: ContainerQuestion, deps: Containe let result: InvocationResult; try { result = await handle.settled; } finally { signal.removeEventListener('abort', cancel); } - // D gave up on cleanup: these resources are no longer owned by anything in this process. Fail closed so no new - // question starts until a restart, whose recovery removes them by the review's owner label. - // Presence, not length, is the signal: an empty list still means D stopped before cleanup was confirmed. Host leftovers (D's input and auth staging directories) are covered by - // the Ask root: the worker's TMPDIR is that root, D stages under tmpdir(), and the root is recorded durably before - // setup and deleted at startup before Ask is enabled again. + // D gave up on cleanup: these resources are no longer owned by anything in this process. Fail closed so nothing + // new starts until a restart, whose recovery removes them by the feature's owner label. + // Presence, not length, is the signal: an empty list still means D stopped before cleanup was confirmed. Host + // leftovers (D's input and auth staging directories) are covered by the worker's root: its TMPDIR is that root, + // D stages under tmpdir(), and the root is recorded durably before setup and deleted at startup. if (result.unreleased !== undefined) retained.markUntracked(); - return answerFromResult(question.provider, result, invocation); + return feature.output(run.provider, result, invocation); + } catch (error) { + failure = { error }; + throw error; } finally { const failures: unknown[] = []; if (filesystems) try { deps.removeFilesystems(filesystems); } catch (error) { retained.retain(filesystems); failures.push(error); } - try { removeStaging(root); } catch (error) { retained.retainPath(root); failures.push(error); } - if (failures.length) throw new AggregateError(failures, 'Question container cleanup did not settle.'); + try { feature.removeRoot(root); } catch (error) { retained.retainPath(root); failures.push(error); } + if (failures.length) throw feature.cleanupFailed(failures, failure); } } +/** Ask's part of a read-only run. Its wording and cleanup error are Ask's own and unchanged by the shared runner. */ +export const ASK_FEATURE: ReadOnlyFeature = Object.freeze({ + phase: 'questions', rootPrefix: 'codeboost-question-', removeRoot: removeStaging, storage: QUESTION_STORAGE, + credential: questionCredential, assertFits: assertFitsQuestionStorage, timedOut: 'Agent timed out. Try again.', + output: answerFromResult, + cleanupFailed: (failures: readonly unknown[]) => new AggregateError(failures, 'Question container cleanup did not settle.'), +}); + +/** + * Answer one question inside the lane D container: a read-only `/work` checkout of the reviewed head, + * the "questions" phase (read, list and search only; no commands), and vendor-only network access. + * Every step is bounded by `deadline`. Storage is released only after the invocation settles. + */ +export async function askInContainer(question: ContainerQuestion, deps: ContainerDependencies, + signal: AbortSignal, image: { id?: string } = {}, retained = new RetainedStorage()): Promise { + return runReadOnlyAgent(ASK_FEATURE, { provider: question.provider, repository: question.repository, head: question.head, + runnerOwner: question.runnerOwner, attemptId: question.attemptId, taskId: `question-${question.noteId}`, + deadline: question.deadline, prompt: question.prompt, schemaText: ANSWER_SCHEMA, + context: { snapshotId: question.snapshotId, planId: question.planId, planRevision: question.planRevision, + assignmentId: question.noteId, referencedCodeHash: question.contextId, stateVersion: 0 } }, + deps, signal, image, retained); +} + // Bounds lane D's recovery, which otherwise allows two minutes; the first question waits for it. const RECOVERY_TIMEOUT_MS = 60_000; /** diff --git a/test/planning-provider.test.ts b/test/planning-provider.test.ts index 885862c5..0901a651 100644 --- a/test/planning-provider.test.ts +++ b/test/planning-provider.test.ts @@ -202,10 +202,10 @@ it.each([ 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(() => removePlanningRoot(foreign)).toThrow(/is not a planning root/); expect(existsSync(foreign)).toBe(true); const nested = mkdtempSync(join(foreign, 'codeboost-planning-')); - expect(() => removePlanningRoot(nested)).toThrow(/not a planning root/); + expect(() => removePlanningRoot(nested)).toThrow(/is 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); @@ -217,3 +217,43 @@ it('accepts only planning storage, never Ask\'s', () => { // @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() }); }); + +it('refuses a repository too large for planning before anything is copied', async () => { + const fake = fakeDeps(); + fake.deps.measureRepository = () => ({ checkoutBytes: 512 * 1024 * 1024 + 1, entries: 3, objectBytes: 0 }); + await expect(provider(fake.deps).invoke(draftRequest(), new AbortController().signal)) + .rejects.toThrow(/^The repository is too large for planning\.$/); + expect(fake.events).toEqual(['build']); +}); + +it('stops before starting the container once its budget has passed', async () => { + const fake = fakeDeps(), build = fake.deps.buildImage; + fake.deps.buildImage = timeout => { const end = Date.now() + 5; while (Date.now() < end) { /* spend the budget */ } return build(timeout); }; + const short = createPlanningProvider({ vendor: 'claude', repository: '/repo', head: HEAD, snapshotId: 'snapshot-1', + runnerOwner: OWNER, deps: fake.deps, timeoutMs: 1 }); + await expect(short.invoke(draftRequest(), new AbortController().signal)).rejects.toThrow(/^Planning agent timed out\.$/); + expect(fake.events).not.toContain('start'); +}); + +it('builds the agent image once across requests', async () => { + const fake = fakeDeps(), image = {}; + const shared = createPlanningProvider({ vendor: 'claude', repository: '/repo', head: HEAD, snapshotId: 'snapshot-1', + runnerOwner: OWNER, deps: fake.deps, image }); + await shared.invoke(draftRequest(), new AbortController().signal); + await shared.invoke(draftRequest('b9c7d9b8-c0bb-4d34-9fad-6cd2e5c37a67'), new AbortController().signal); + expect(fake.events.filter(event => event === 'build')).toEqual(['build']); +}); + +it.skipIf(process.getuid?.() === 0)('keeps a host copy it could not delete, naming it after the request\'s own error', async () => { + // Stage inside a parent we control; making it read-only stops the planning root from being removed. + const parent = mkdtempSync(join(tmpdir(), 'planning-tmp-')), saved = process.env.TMPDIR; + roots.push(parent); cleanups.push(() => chmodSync(parent, 0o700)); + process.env.TMPDIR = parent; + const retained = new PlanningStorage(), fake = fakeDeps({ exitCode: 1, stdout: 'Invalid API key' }), clone = fake.deps.createClone; + fake.deps.createClone = options => { chmodSync(parent, 0o555); return clone(options); }; + try { + await expect(provider(fake.deps, { retained }).invoke(draftRequest(), new AbortController().signal)).rejects + .toThrow(/^Claude could not write the plan\..* Cleanup also did not settle: A copy of the planned code could not be deleted \(.*codeboost-planning-.*\)\.$/); + expect(retained.paths()).toHaveLength(1); + } finally { if (saved === undefined) delete process.env.TMPDIR; else process.env.TMPDIR = saved; } +}); diff --git a/test/question-agent.test.ts b/test/question-agent.test.ts index 66f21339..567fda5e 100644 --- a/test/question-agent.test.ts +++ b/test/question-agent.test.ts @@ -131,7 +131,7 @@ it('releases storage when setup fails after allocation, and not before', async ( it('stops before starting the container once the deadline has passed', async () => { const fake = fakeDeps(); - await expect(askInContainer(question({ deadline: Date.now() - 1 }), fake.deps, new AbortController().signal)).rejects.toThrow('timed out'); + await expect(askInContainer(question({ deadline: Date.now() - 1 }), fake.deps, new AbortController().signal)).rejects.toThrow(/^Agent timed out\. Try again\.$/); expect(fake.events).not.toContain('start'); }); @@ -209,7 +209,7 @@ it.skipIf(process.getuid?.() === 0)('keeps a host copy of the code it could not const clone = fake.deps.createClone; fake.deps.createClone = options => { chmodSync(parent, 0o555); return clone(options); }; try { - await expect(askInContainer(question(), fake.deps, new AbortController().signal, {}, retained)).rejects.toThrow('cleanup did not settle'); + await expect(askInContainer(question(), fake.deps, new AbortController().signal, {}, retained)).rejects.toThrow(/^Question container cleanup did not settle\.$/); const [root] = retained.paths(); expect(dirname(root!)).toBe(parent); expect(existsSync(root!)).toBe(true); @@ -270,11 +270,21 @@ it('turns Ask off when a failed setup leaves storage D cannot hand back', async expect(clean.untracked).toBe(0); }); +it('reports only Ask\'s cleanup error when a failed question\'s cleanup also fails', async () => { + // Ask's wording is unchanged by the shared runner (#117): its own error is not prefixed, unlike planning's. + const retained = new RetainedStorage(), fake = fakeDeps(); + fake.deps.capture = () => { throw new Error('capture refused'); }; + fake.deps.removeFilesystems = () => { throw new Error('Docker did not confirm removal.'); }; + await expect(askInContainer(question(), fake.deps, new AbortController().signal, {}, retained)) + .rejects.toThrow(/^Question container cleanup did not settle\.$/); + expect(retained.size).toBe(1); +}); + it('keeps storage whose removal failed, refuses Ask until it is removed, then continues', async () => { const retained = new RetainedStorage(); const first = fakeDeps(); first.deps.removeFilesystems = () => { throw new Error('Docker did not confirm removal.'); }; - await expect(askInContainer(question(), first.deps, new AbortController().signal, {}, retained)).rejects.toThrow('cleanup did not settle'); + await expect(askInContainer(question(), first.deps, new AbortController().signal, {}, retained)).rejects.toThrow(/^Question container cleanup did not settle\.$/); expect(retained.size).toBe(1); const blocked = fakeDeps();