diff --git a/packages/subagent/subagent-acp/src/index.ts b/packages/subagent/subagent-acp/src/index.ts index 66f254b831..037d32889e 100644 --- a/packages/subagent/subagent-acp/src/index.ts +++ b/packages/subagent/subagent-acp/src/index.ts @@ -71,7 +71,7 @@ export const Config: z = z.object({ class AcpProvider implements SubagentProvider { readonly capabilities: SubagentCapabilities = { outputSchema: false, depthLimit: false, toolFilter: false } - constructor(readonly name: string, private readonly config: Config) {} + constructor(readonly name: string, private readonly ctx: Context, private readonly config: Config) {} start(request: SubagentStartRequest) { const spec: AcpRunSpec = { @@ -80,11 +80,16 @@ class AcpProvider implements SubagentProvider { cwd: this.config.cwd ?? process.cwd(), permission: this.config.permission, env: this.config.env, + onError: (error, stopReason) => { + // The seam forbids `result` rejecting, so a child-level failure is + // flattened to a stop reason — preserve it here rather than losing it. + this.ctx.logger.warn(`subagent-acp "${this.name}": child run failed (${stopReason}): ${error.message}`) + }, } return startAcpRun(request, spec) } } export function apply(ctx: Context, config: Config): void { - ctx.subagents.registerProvider(new AcpProvider(config.providerName, config)) + ctx.subagents.registerProvider(new AcpProvider(config.providerName, ctx, config)) } diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index 8aeb136f2d..06f7a9ece8 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -83,6 +83,14 @@ export interface AcpRunSpec { * a test injects a small value to exercise the escalation without a long wait. */ disposeGraceMs?: number + /** + * Sink for a child-level failure that the run flattened into a stop reason + * (the seam contract forbids `result` rejecting). The driver calls this with + * the original error and the chosen stop reason so the fault is preserved + * rather than silently lost; the provider wires it to `ctx.logger.warn`. + * Optional — omitted in a unit test that asserts the stop reason directly. + */ + onError?: (error: Error, stopReason: SubagentStopReason) => void } /** @@ -160,6 +168,15 @@ export function toAcpPrompt(prompt: ContentBlock[]): AcpContentBlock[] { return blocks } +/** Normalize an unknown thrown value to an Error (the catch binding is `unknown`). */ +function toError(value: unknown): Error { + // The catch only sees rejections from the ACP SDK RPCs and the spawn `error` + // event, which are always `Error`s; the `String(value)` arm is a defensive + // fallback for a non-Error throw that the typed surfaces cannot produce. + /* v8 ignore next */ + return value instanceof Error ? value : new Error(String(value)) +} + /** Resolve once the child process exits (any code/signal); immediate if gone. */ function waitForExit(child: ChildProcess): Promise { // Already-exited fast path: dispose guards on exitCode before calling, so in @@ -264,8 +281,19 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su ) let sessionId: string | undefined + // Resolves when a cancel is requested, so `result` can settle `aborted` even + // if the child never cooperates with `session/cancel` (it ignores the notify, + // or the prompt wedges). The result path races this against the ACP drive: the + // FIRST to settle wins, so `cancel()` always honors the contract (`result` + // settles `aborted`) without waiting on a non-cooperative child. `dispose` + // still kills the process and reaps it; this only unblocks `result`. The + // executor runs synchronously, so `signalCancelSettled` is assigned before the + // Promise constructor returns (the `!` asserts the definite assignment). + let signalCancelSettled!: () => void + const cancelSettled = new Promise((resolve) => { signalCancelSettled = resolve }) const requestCancel = (): void => { flags.cancelled = true + signalCancelSettled() // Best-effort: tell the child to cancel the in-flight turn. Swallows a // rejection — the session may not exist yet, or the pipe may be gone; the // dispose path kills the process regardless. If the session has NOT been @@ -289,9 +317,14 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su return text.length > 0 ? [{ type: 'text', text }] : [] } try { - // Race the ACP drive against a spawn failure: a bad command never speaks - // ACP, so `initialize` would hang forever — the spawn `error` event is the - // only signal, and a rejected race settles the run `error` via the catch. + // Race three outcomes, first to settle wins: + // - driveAcp: the normal initialize → newSession → prompt path; + // - spawnFailed: a bad command never speaks ACP, so `initialize` would + // hang forever — the spawn `error` event is the only signal, and a + // rejected race settles the run `error` via the catch; + // - cancelSettled: a cancel was requested — settle `aborted` immediately + // rather than waiting on a child that may ignore `session/cancel` or + // wedge the prompt (the `cancel()` contract: `result` settles `aborted`). const driveAcp = async (): Promise => { await conn.initialize({ protocolVersion: PROTOCOL_VERSION, @@ -312,13 +345,19 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su return await Promise.race([ driveAcp(), spawnFailed.then((err): SubagentResult => { throw err }), + cancelSettled.then((): SubagentResult => ({ output: collectOutput(), stopReason: 'aborted' })), ]) - } catch { + } catch (error: unknown) { // The seam contract: result resolves (never rejects) on a child-level - // failure. A spawn/transport/RPC error becomes an error/aborted result — - // `aborted` if a cancel was requested (the failure is the cancellation - // surfacing as a torn pipe / rejected RPC), else a genuine `error`. - return { output: collectOutput(), stopReason: flags.cancelled ? 'aborted' : 'error' } + // failure. Cancellation is handled by the `cancelSettled` race arm above + // (it settles `aborted` the instant cancel is requested, beating any + // rejection), so a rejection that reaches HERE is always a genuine + // child-level error — the awaited ACP RPCs or the spawn-failure race + // (initialize/newSession/prompt transport/RPC errors, or ENOENT), not a + // local bug. Flatten to `error` and surface the original via onError so a + // real fault is preserved rather than silently lost. + spec.onError?.(toError(error), 'error') + return { output: collectOutput(), stopReason: 'error' } } })() diff --git a/packages/subagent/subagent-acp/tests/mock-acp-server.ts b/packages/subagent/subagent-acp/tests/mock-acp-server.ts index 74ae340bde..9cfeac1f44 100644 --- a/packages/subagent/subagent-acp/tests/mock-acp-server.ts +++ b/packages/subagent/subagent-acp/tests/mock-acp-server.ts @@ -8,6 +8,11 @@ * (`end_turn` default, or `max_tokens`/`refusal`/…). * - `MOCK_HANG` — if `1`, `prompt` never resolves on its own (it waits for * a `session/cancel`), to exercise the client's cancel path. + * - `MOCK_IGNORE_CANCEL` — if `1` (with MOCK_HANG), the agent receives + * `session/cancel` but NEVER resolves the pending prompt + * and never exits — a non-cooperative child. The backend's + * `result` must still settle `aborted` on its own and + * `dispose()` must still kill the process. * - `MOCK_PERMISSION` — if `1`, the agent calls `session/request_permission` * before answering, to exercise the client's auto-answer. * - `MOCK_READY_FILE` — if set, the path the agent touches once its `prompt` @@ -63,6 +68,7 @@ const WANT_PERMISSION = process.env.MOCK_PERMISSION === '1' const NO_ALLOW = process.env.MOCK_NO_ALLOW === '1' const THOUGHT = process.env.MOCK_THOUGHT === '1' const CRASH_ON_CANCEL = process.env.MOCK_CRASH_ON_CANCEL === '1' +const IGNORE_CANCEL = process.env.MOCK_IGNORE_CANCEL === '1' const READY_FILE = process.env.MOCK_READY_FILE const FLUSH_ON_EOF = process.env.MOCK_FLUSH_ON_EOF // When MOCK_NEWSESSION_READY/GO are set, newSession touches READY then blocks @@ -150,6 +156,14 @@ function makeAgent(conn: AgentSideConnection): Agent { // path: a transport failure after a cancel settles `aborted`). process.exit(1) } + if (IGNORE_CANCEL) { + // A NON-COOPERATIVE child: receive session/cancel but never resolve the + // pending prompt and never exit. The backend's `result` must still settle + // `aborted` on its own (the cancel-settle race), and `dispose()` must + // still kill the process — proving cancellation does not depend on the + // child cooperating. + return Promise.resolve() + } resolveCancel?.('cancelled') return Promise.resolve() }, diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index 926319ad87..9819320ec5 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -385,6 +385,20 @@ describe('dsh-subagent-acp', () => { }) it('resolves error (not reject) when the spawn command does not exist', async () => { + // Direct startAcpRun with NO onError sink — the catch must still flatten the + // spawn failure to `error` (the onError call is optional, covering the + // absent-sink branch). + const run = startAcpRun( + { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, + { command: '/nonexistent/acp-agent-binary', args: [], cwd: process.cwd(), permission: 'reject', env: {} }, + ) + const result = await run.result + // The seam contract: a child-level failure resolves error, never rejects. + expect(result.stopReason).toBe('error') + await run.dispose() + }) + + it('resolves error via the provider (real load path) when the command does not exist', async () => { const ctx = new Context() await ctx.plugin(SubagentService) await ctx.plugin(acp, { @@ -396,11 +410,35 @@ describe('dsh-subagent-acp', () => { }) const run = ctx.subagents.start('acp', { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }) const result = await run.result - // The seam contract: a child-level failure resolves error, never rejects. expect(result.stopReason).toBe('error') await run.dispose() }) + it('reports a flattened child failure through onError (preserved, not silently lost)', async () => { + // The seam forbids `result` rejecting, so a child-level failure is flattened + // to a stop reason — onError must still surface the original error so a real + // fault is logged, not swallowed. A nonexistent command triggers the spawn + // failure path; the spy records the error + the chosen stop reason. + const errors: { message: string; stopReason: string }[] = [] + const run = startAcpRun( + { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }, + { + command: '/nonexistent/acp-agent-binary', + args: [], + cwd: process.cwd(), + permission: 'reject', + env: {}, + onError: (error, stopReason) => { errors.push({ message: error.message, stopReason }) }, + }, + ) + const result = await run.result + expect(result.stopReason).toBe('error') + expect(errors).toHaveLength(1) + expect(errors[0]!.stopReason).toBe('error') + expect(errors[0]!.message.length).toBeGreaterThan(0) + await run.dispose() + }) + it('settles aborted when the child crashes (tears the pipe) AFTER a cancel', async () => { // The child hangs, we cancel, and instead of answering the child exits hard // — the pending prompt RPC rejects. With a cancel already requested, the @@ -421,6 +459,31 @@ describe('dsh-subagent-acp', () => { } }) + it('settles aborted on cancel even when the child IGNORES session/cancel (non-cooperative)', async () => { + // The contract: run.cancel() → result settles `aborted`. A child that hangs + // its prompt AND ignores session/cancel must not wedge the parent — the + // backend's own cancel-settle path resolves `aborted` without the child's + // cooperation, and dispose() still reaps the process. + const tmp = mkdtempSync(join(tmpdir(), 'acp-ignorecancel-')) + const ready = join(tmp, 'ready') + try { + const ctx = await setup({ MOCK_TEXT: 'partial', MOCK_HANG: '1', MOCK_IGNORE_CANCEL: '1', MOCK_READY_FILE: ready }) + const run = ctx.subagents.start('acp', { prompt: [{ type: 'text', text: 'p' }], parent: fakeParent }) + await waitForFile(ready) + run.cancel('test') + // Bound it: a regression (cancel only notifies the child, which ignores it) + // would hang result forever — fail loud instead of stalling the suite. + const result = await Promise.race([ + run.result, + new Promise((_r, reject) => { setTimeout(() => { reject(new Error('result did not settle on cancel — backend waited on the child')) }, 4000) }), + ]) + expect(result.stopReason).toBe('aborted') + await run.dispose() + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + it('advertises no start-time capabilities (out-of-process child)', async () => { const ctx = await setup() const provider = ctx.subagents.getProvider('acp')!