fix(subagent): close ACP diagnostic review gaps

examples/acp-agent/subagent-acp-diagnostic.cordis.snapshot.yml

@@ -28,9 +28,7 @@
               permission: reject
               env:
                 MOCK_TEXT: partial ACP assistant text
-                MOCK_STOP: max_turn_requests
                 MOCK_PERMISSION: '1'
-                MOCK_PERMISSION_IGNORE_DECISION: '1'
                 MOCK_TOOL_KIND: execute
           - id: tool-subagent-acp-diagnostic
             name: '@deepseek-ai/dsh-tool-subagent'

examples/acp-agent/subagent-acp-diagnostic.cordis.yml

@@ -18,9 +18,7 @@
               permission: reject
               env:
                 MOCK_TEXT: partial ACP assistant text
-                MOCK_STOP: max_turn_requests
                 MOCK_PERMISSION: '1'
-                MOCK_PERMISSION_IGNORE_DECISION: '1'
                 MOCK_TOOL_KIND: execute
           - id: tool-subagent-acp-diagnostic
             name: '@deepseek-ai/dsh-tool-subagent'

examples/acp-agent/tests/snapshots/subagent-acp-diagnostic/session.jsonl

@@ -15,7 +15,7 @@
 {"type":"assistant/chunk","seq":13,"time":1787254574889,"data":{"turn":1,"step":1,"chunk":{"type":"finish","reason":{"kind":"tool-calls"}}}}
 {"type":"assistant/message","seq":14,"time":1787254574889,"data":{"turn":1,"step":1,"message":{"role":"assistant","content":[{"type":"tool-call","id":"call_acp_foreground","name":"subagent_acp","arguments":"{\"description\":\"Observe ACP foreground failure\",\"prompt\":\"Return the scripted ACP failure.\",\"run_in_background\":false}"}],"source":{"kind":"model","provider":"deepseek-official","model":"deepseek-v4-pro"},"id":"ef8e9ff0-886c-4d55-bbdb-8e878258fb53"},"usage":{"inputTokens":10,"outputTokens":5}},"sourceEventSeqs":[9,10,11,12,13],"surfaceOp":"append"}
 {"type":"tool/call","seq":15,"time":1787254574890,"data":{"turn":1,"step":1,"callId":"call_acp_foreground","name":"subagent_acp","arguments":"{\"description\":\"Observe ACP foreground failure\",\"prompt\":\"Return the scripted ACP failure.\",\"run_in_background\":false}"}}
-{"type":"tool/result","seq":16,"time":1787254574996,"data":{"turn":1,"step":1,"message":{"source":{"kind":"tool","callId":"call_acp_foreground"},"content":[{"type":"tool-result","toolCallId":"call_acp_foreground","content":[{"type":"text","text":"Error: subagent run failed\nDiagnostic: Subagent failure (provider: ACP; stage: prompt; category: remote-limit; stop reason: max_turn_requests)\nACP unattended decision (policy: reject; request: execute; decision: denied)\nPartial output before the run ended:\npartial ACP assistant text"}],"isError":true}],"role":"user","id":"9a82d328-e8dc-43c6-94c5-cfaf93b64c5d"}},"sourceEventSeqs":[15],"surfaceOp":"append"}
+{"type":"tool/result","seq":16,"time":1787254574996,"data":{"turn":1,"step":1,"message":{"source":{"kind":"tool","callId":"call_acp_foreground"},"content":[{"type":"tool-result","toolCallId":"call_acp_foreground","content":[{"type":"text","text":"Error: subagent run was cancelled\nDiagnostic: Subagent failure (provider: ACP; stage: prompt; category: permission; stop reason: cancelled)\nACP unattended decision (policy: reject; request: execute; decision: denied)"}],"isError":true}],"role":"user","id":"720dc6b6-6788-4f6f-8426-b888ccafc84a"}},"sourceEventSeqs":[15],"surfaceOp":"append"}
 {"type":"step/end","seq":17,"time":1787254574996,"data":{"turn":1,"step":1}}
 {"type":"step/start","seq":18,"time":1787254575002,"data":{"turn":1,"step":2}}
 {"type":"assistant/chunk","seq":19,"time":1787254575006,"data":{"turn":1,"step":2,"chunk":{"type":"block-start","index":0,"blockType":"tool-call"}}}
@@ -35,7 +35,7 @@
 {"type":"assistant/chunk","seq":33,"time":1787254575021,"data":{"turn":1,"step":3,"chunk":{"type":"finish","reason":{"kind":"tool-calls"}}}}
 {"type":"assistant/message","seq":34,"time":1787254575021,"data":{"turn":1,"step":3,"message":{"role":"assistant","content":[{"type":"tool-call","id":"call_acp_output","name":"job_output","arguments":"{\"job_id\":\"subagent-1\",\"wait\":true}"}],"source":{"kind":"model","provider":"deepseek-official","model":"deepseek-v4-pro"},"id":"b20d64f1-7fcf-498d-84ec-afe518983863"},"usage":{"inputTokens":10,"outputTokens":5}},"sourceEventSeqs":[29,30,31,32,33],"surfaceOp":"append"}
 {"type":"tool/call","seq":35,"time":1787254575021,"data":{"turn":1,"step":3,"callId":"call_acp_output","name":"job_output","arguments":"{\"job_id\":\"subagent-1\",\"wait\":true}"}}
-{"type":"tool/result","seq":36,"time":1787254575110,"data":{"turn":1,"step":3,"message":{"source":{"kind":"tool","callId":"call_acp_output"},"content":[{"type":"tool-result","toolCallId":"call_acp_output","content":[{"type":"text","text":"(no new output)\n[status: failed, error; diagnostic: Subagent failure (provider: ACP; stage: prompt; category: remote-limit; stop reason: max_turn_requests)\nACP unattended decision (policy: reject; request: execute; decision: denied)]"}],"isError":false}],"role":"user","id":"26ecb040-cc32-474c-9db2-a27ffa7fe9fe"}},"sourceEventSeqs":[35],"surfaceOp":"append"}
+{"type":"tool/result","seq":36,"time":1787254575110,"data":{"turn":1,"step":3,"message":{"source":{"kind":"tool","callId":"call_acp_output"},"content":[{"type":"tool-result","toolCallId":"call_acp_output","content":[{"type":"text","text":"(no new output)\n[status: failed, aborted; diagnostic: Subagent failure (provider: ACP; stage: prompt; category: permission; stop reason: cancelled)\nACP unattended decision (policy: reject; request: execute; decision: denied)]"}],"isError":false}],"role":"user","id":"372a7fa1-e148-46ba-bc6a-33a3e0963276"}},"sourceEventSeqs":[35],"surfaceOp":"append"}
 {"type":"step/end","seq":37,"time":1787254575110,"data":{"turn":1,"step":3}}
 {"type":"step/start","seq":38,"time":1787254575116,"data":{"turn":1,"step":4}}
 {"type":"assistant/chunk","seq":39,"time":1787254575121,"data":{"turn":1,"step":4,"chunk":{"type":"block-start","index":0,"blockType":"text"}}}

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

@@ -146,7 +146,7 @@ function diagnosticText(facts: AcpFailureFacts, permission?: AcpPermissionDecisi
 }
 
 class AcpRunFailure extends Error {
-  constructor(readonly facts: AcpFailureFacts, cause: unknown) {
+  constructor(facts: AcpFailureFacts, cause: unknown) {
     super(
       `subagent-acp: ${failureDiagnostic(facts)}`,
       { cause },
@@ -347,13 +347,19 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
   // Keep diagnostics on parent stderr ('inherit'); only ACP output contributes
   // to the result. The seam's scrub drops ambient credentials and DSH_* names
   // while spec.env (the child's own key, its deployment facts) merges after it.
-  const child = spec.spawn({
-    argv: [spec.command, ...spec.args],
-    cwd: spec.cwd,
-    stdio: { stdin: 'pipe', stdout: 'pipe', stderr: 'inherit' },
-    graceMs: spec.disposeGraceMs,
-    env: spec.env,
-  })
+  let child: SubprocessHandle
+  try {
+    child = spec.spawn({
+      argv: [spec.command, ...spec.args],
+      cwd: spec.cwd,
+      stdio: { stdin: 'pipe', stdout: 'pipe', stderr: 'inherit' },
+      graceMs: spec.disposeGraceMs,
+      env: spec.env,
+    })
+  } catch (error: unknown) {
+    reportFailure(spec, error)
+    throw new AcpRunFailure({ stage: 'process', category: 'process-start' }, error)
+  }
   /* v8 ignore start -- 'pipe' dispositions expose both streams by the seam contract; defensive. */
   if (child.stdin === undefined || child.stdout === undefined) {
     throw new Error('subagent-acp: subprocess implementation dropped a piped protocol stream')
@@ -509,7 +515,9 @@ export async function startAcpRun(request: SubagentStartRequest, spec: AcpRunSpe
         category: processOutcome === undefined ? 'unknown' : 'process-exit',
         ...(processOutcome === undefined ? {} : { outcome: processOutcome }),
       }, cleanupError)
-      if (cancelledBeforeCleanup) throw cleanupFailure
+      if (cancelledBeforeCleanup) {
+        throw new AggregateError([cleanupFailure], cleanupFailure.message)
+      }
       throw new AggregateError(
         [failure, cleanupFailure],
         `${failure.message}; ${cleanupFailure.message}`,

packages/subagent/subagent-acp/tests/mock-acp-server.ts

@@ -21,8 +21,8 @@
  * - `MOCK_PERMISSION_IGNORE_DECISION` — if `1`, continue after a denied
  *                        permission so the terminal failure can carry the
  *                        provider's fixed permission fact.
- * - `MOCK_CRASH_ON_INITIALIZE` / `MOCK_CRASH_ON_NEW_SESSION` — exit while the
- *                        named unpublished protocol operation is active.
+ * - `MOCK_CRASH_ON_INITIALIZE` — exit while the unpublished initialize
+ *                        operation is active.
  * - `MOCK_CLOSE_PROTOCOL_ON_PROMPT` — close stdout while keeping the process
  *                        alive, producing a prompt-stage transport failure.
  * - `MOCK_CRASH_AFTER_CHUNK` — exit after streaming the assistant chunk, so
@@ -94,7 +94,6 @@ const IGNORE_PERMISSION_DECISION = process.env.MOCK_PERMISSION_IGNORE_DECISION =
 const NO_ALLOW = process.env.MOCK_NO_ALLOW === '1'
 const THOUGHT = process.env.MOCK_THOUGHT === '1'
 const CRASH_ON_INITIALIZE = process.env.MOCK_CRASH_ON_INITIALIZE === '1'
-const CRASH_ON_NEW_SESSION = process.env.MOCK_CRASH_ON_NEW_SESSION === '1'
 const CRASH_ON_CANCEL = process.env.MOCK_CRASH_ON_CANCEL === '1'
 const CRASH_ON_PROMPT = process.env.MOCK_CRASH_ON_PROMPT === '1'
 const CLOSE_PROTOCOL_ON_PROMPT = process.env.MOCK_CLOSE_PROTOCOL_ON_PROMPT === '1'
@@ -126,7 +125,6 @@ function makeAgent(conn: AgentSideConnection): Agent {
       })
     },
     async newSession(params: NewSessionRequest): Promise<NewSessionResponse> {
-      if (CRASH_ON_NEW_SESSION) process.exit(12)
       sessionCwd = params.cwd
       // Optionally signal "newSession reached" and block until released, so a
       // test can cancel DURING newSession (the early-cancel race window) on a

packages/subagent/subagent-acp/tests/subagent-acp.spec.ts

@@ -658,7 +658,8 @@ describe('dsh-subagent-acp', () => {
       controller.abort()
       writeFileSync(go, 'go')
       const error = await starting.catch((cause: unknown) => cause)
-      expect(error).toBeInstanceOf(Error)
+      expect(error).toBeInstanceOf(AggregateError)
+      expect((error as AggregateError).errors).toHaveLength(1)
       expect((error as Error).message).toBe(
         `subagent-acp: ${expectedFailure('stage: teardown; category: unknown')}`,
       )
@@ -952,6 +953,28 @@ describe('dsh-subagent-acp', () => {
     expect((error as Error).message).not.toContain(privateCommand)
   })
 
+  it('sanitizes a synchronous subprocess-provider spawn rejection', async () => {
+    const rawMessage = 'spawn rejected /private/path SECRET_TOKEN'
+    const errors: string[] = []
+    const error = await startAcpRun(request(), {
+      command: 'unused',
+      args: [],
+      cwd: process.cwd(),
+      permission: 'reject',
+      env: {},
+      disposeEofGraceMs: DEFAULT_DISPOSE_EOF_GRACE_MS,
+      disposeGraceMs: DEFAULT_DISPOSE_GRACE_MS,
+      spawn: () => { throw new Error(rawMessage) },
+      onError: (failure) => { errors.push(failure.message) },
+    }).catch((cause: unknown) => cause)
+    expect(error).toBeInstanceOf(Error)
+    expect((error as Error).message).toBe(
+      `subagent-acp: ${expectedFailure('stage: process; category: process-start')}`,
+    )
+    expect((error as Error).message).not.toContain(rawMessage)
+    expect(errors).toEqual([rawMessage])
+  })
+
   it('plugin-config dispose graces reach the run (SIGKILL escalation through the provider)', async () => {
     // Same trap scenario as the direct startAcpRun escalation test, but the
     // graces arrive via the PLUGIN CONFIG through the registered provider — so a

packages/subagent/subagent/src/out-of-process.ts

@@ -41,13 +41,8 @@ function limitSubagentDiagnostic(diagnostic: string): string {
     + DIAGNOSTIC_TRUNCATION_SUFFIX
 }
 
-/** Enforce success omission and the byte limit on a provider-returned result. */
+/** Enforce the byte limit on a provider-returned diagnostic. */
 function normalizeSubagentDiagnostic(result: SubagentResult): SubagentResult {
-  if (result.stopReason === 'completed') {
-    const normalized = { ...result }
-    Reflect.deleteProperty(normalized, 'diagnostic')
-    return normalized
-  }
   return result.diagnostic === undefined
     ? result
     : { ...result, diagnostic: limitSubagentDiagnostic(result.diagnostic) }
@@ -188,8 +183,8 @@ export interface RunResultSettlement {
  * rejects after publication. A normally completed or rejected attempt resolves
  * as `aborted` when cancellation already settled locally; another rejection is
  * flattened to `stopReason: 'error'` through the contained diagnostic sink.
- * Provider-returned diagnostics use the same byte limit, and completed results
- * omit them. The abort listener is removed on every path.
+ * Provider-returned diagnostics use the same byte limit. The abort listener is
+ * removed on every path.
  * @param parts - the attempt, output snapshot, cancellation state, sink, and signal wiring.
  * @returns the terminal result (never a rejection).
  */

packages/subagent/subagent/tests/run-settlement.spec.ts

@@ -149,19 +149,6 @@ describe('outcome mapping helpers', () => {
       .toBeLessThanOrEqual(MAX_SUBAGENT_DIAGNOSTIC_BYTES)
     expect(failed.diagnostic).toMatch(/\[diagnostic truncated\]$/)
 
-    const completed = await settleRunResult({
-      attempt: () => Promise.resolve({
-        output: [],
-        diagnostic: 'must not survive success',
-        stopReason: 'completed',
-      }),
-      collectOutput: () => [],
-      cancelled: () => false,
-      signal: controller.signal,
-      onAbort: () => {},
-    })
-    expect(completed).toEqual({ output: [], stopReason: 'completed' })
-
     const plainFailure = await settleRunResult({
       attempt: () => Promise.resolve({ output: [], stopReason: 'error' }),
       collectOutput: () => [],