fix(subagent-acp): contain onError sink exceptions to keep result from rejecting

spec.onError is a caller-supplied callback boundary, but the flattening
catch invoked it unguarded: a throwing sink rejected the whole async result,
breaking the seam's "result never rejects on a child-level failure"
contract (and docs/defensive-patterns.md's contain-callback-exceptions
rule). The sink's own throw is now swallowed — named as such — while the
original child failure still settles as stopReason 'error'. Regression test
drives a spawn failure through a throwing sink and asserts result resolves.

Same defect as the codex backend's, fixed there on PR #240; this is the
symmetric fix for the already-merged ACP backend.
This commit is contained in:
pku-xht
2026-07-10 16:42:20 +08:00
parent 42ebbfdf8f
commit 62627d7625
2 changed files with 30 additions and 1 deletions

View File

@@ -89,6 +89,7 @@ export interface AcpRunSpec {
* (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`.
* A throw from the sink itself is contained — it cannot reject `result`.
* Optional — omitted in a unit test that asserts the stop reason directly.
*/
onError?: (error: Error, stopReason: SubagentStopReason) => void
@@ -336,7 +337,13 @@ export function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpec): Su
// (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')
try {
spec.onError?.(toError(error), 'error')
} catch {
// Swallows only the caller-supplied sink's OWN throw: an unguarded
// sink exception would reject `result` and break the contract above.
// The child-level failure being reported still settles as `error`.
}
return { output: collectOutput(), stopReason: 'error' }
}
})()

View File

@@ -483,6 +483,28 @@ describe('dsh-subagent-acp', () => {
await run.dispose()
})
it('resolves error (never rejects) even when the onError sink itself throws', async () => {
// onError is a caller-supplied callback boundary: its own exception must be
// contained, or it would reject `result` and break the seam's "result never
// rejects" contract that the flattening above exists to uphold.
const run = startAcpRun(
{ prompt: [{ type: 'text', text: 'p' }], parent: fakeParent },
{
command: '/nonexistent/acp-agent-binary',
args: [],
cwd: process.cwd(),
permission: 'reject',
env: {},
disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS,
disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS,
onError: () => { throw new Error('sink boom') },
},
)
const result = await run.result
expect(result.stopReason).toBe('error')
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