Settle ACP cancel without the child's cooperation; preserve flattened errors (review feedback)

Two findings on the ACP backend:

Blocking: cancel() only sent session/cancel, so a child that ignores the notify
or wedges the prompt left result hung forever — the model-facing tool awaits
result before its finally disposes, so the parent cancellation hung and the
child stayed alive, violating the SubagentRun.cancel() contract (result settles
aborted). The result path now races the ACP drive against a cancelSettled
promise that requestCancel resolves, so result settles aborted the instant a
cancel is requested, regardless of the child. dispose() still kills+reaps the
process. New MOCK_IGNORE_CANCEL mock mode (receives cancel, never resolves the
prompt, never exits) drives a regression proven to hang without the race.

Nit: the drive-path catch was an empty broad catch that discarded the error
(AGENTS.md forbids). Because cancellation is now handled by the race arm, a
rejection reaching the catch is always a genuine child-level error — bind it,
flatten to error, and surface the original via a new AcpRunSpec.onError sink
that the provider wires to ctx.logger.warn, so a real fault is preserved.

packages/subagent/subagent-acp/src/index.ts

@@ -71,7 +71,7 @@ export const Config: z<Config> = 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))
 }

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<void> {
   // 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<void>((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<SubagentResult> => {
         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' }
     }
   })()
 

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()
     },

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<never>((_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')!