From 0e7d539bbc5b75ad224b312a11cc1e15e1ba527a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 00:18:00 +0800 Subject: [PATCH 01/16] refactor: share the ACP test launcher --- .../2026-07-08-shared-acp-snapshot-package.md | 10 +- examples/acp-agent/tests/acp.e2e.ts | 182 +++--------------- examples/acp-agent/tests/hooks.e2e.ts | 68 ++----- .../sandbox-acp-agent/tests/escalation.e2e.ts | 79 +++----- packages/support/README.md | 4 +- packages/support/acp-snapshot/README.md | 5 +- packages/support/acp-snapshot/package.json | 2 +- packages/support/acp-snapshot/src/harness.ts | 130 ++----------- packages/support/acp-snapshot/src/index.ts | 24 ++- packages/support/acp-snapshot/src/launcher.ts | 152 +++++++++++++++ .../acp-snapshot/tests/harness.spec.ts | 33 ++++ 11 files changed, 292 insertions(+), 397 deletions(-) create mode 100644 packages/support/acp-snapshot/src/launcher.ts diff --git a/docs/rfc/implemented/testing/2026-07-08-shared-acp-snapshot-package.md b/docs/rfc/implemented/testing/2026-07-08-shared-acp-snapshot-package.md index eca3661b33..59734535b8 100644 --- a/docs/rfc/implemented/testing/2026-07-08-shared-acp-snapshot-package.md +++ b/docs/rfc/implemented/testing/2026-07-08-shared-acp-snapshot-package.md @@ -6,13 +6,15 @@ Status: implemented The ACP snapshot tier ([snapshot RFC](2026-06-19-acp-snapshot-tests.md)) was built from three modules living inside one example's test directory: `snapshot-harness.ts` (boot the real bin subprocess, drive it over ACP JSON-RPC, harvest the persisted logs), `snapshot-normalize.ts` (the pure golden normalizers), and the ~150-line scenario body plus fixture guards in `acp.snapshot.ts` (record/replay modes, the stdout-golden and log compares, the pinned-header uniformity guard, the orphan/required-file/single-pin meta-tests). -A second ACP example wanting snapshot coverage — the sandbox/approval composition is the immediate consumer — could only copy those modules, forking exactly the logic that must not drift: record write-back, header scrubbing, child-session harvest ordering. The spawn/client glue was already triplicated across `acp.e2e.ts`, `hooks.e2e.ts`, and the harness (`TODO(acp-test-harness)`). Location also decided test rigor: the per-file 100% coverage gate measures `packages/*/*/src` only, so none of this machinery was measured — the same gap that had moved `dsh-llm-replay` out of `examples/` into [packages/support](../../../../packages/support/README.md). And the harness's ACP client hardcoded `requestPermission → cancelled`, so an approval round-trip — the headline behavior of the sandbox composition — could not be expressed at the snapshot tier at all. +A second ACP example wanting snapshot coverage — the sandbox/approval composition is the immediate consumer — could only copy those modules, forking exactly the logic that must not drift: record write-back, header scrubbing, child-session harvest ordering. The spawn/client glue was also triplicated across `acp.e2e.ts`, `hooks.e2e.ts`, and the harness. Location decided test rigor: the per-file 100% coverage gate measures `packages/*/*/src` only, so none of this machinery was measured — the same gap that had moved `dsh-llm-replay` out of `examples/` into [packages/support](../../../../packages/support/README.md). And the harness's ACP client hardcoded `requestPermission → cancelled`, so an approval round-trip — the headline behavior of the sandbox composition — could not be expressed at the snapshot tier at all. ## Decision The machinery lives in [`packages/support/acp-snapshot`](../../../../packages/support/acp-snapshot/README.md) (`@deepseek-ai/dsh-acp-snapshot`); an example's `*.snapshot.ts` is its scenario table, its agent paths, and one factory call, over its own `snapshots/` fixtures and `cordis.snapshot.yml` overlay ([single-source replay config](2026-07-04-single-source-acp-replay-config.md)). Reading `DSH_SNAPSHOT` stays at that edge — the library takes a resolved `mode`. -**`src/harness.ts`** — `runScenario` and the input-script/result types, parameterized by an `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath`; absolute paths the consuming suite resolves from its own `import.meta.url`). The client's `session/request_permission` handler consumes an optional `InputScript.permissionAnswers` FIFO queue, each entry selecting by option **kind** (ids are agent-issued randoms a committed script cannot know; kinds are the ACP-stable vocabulary, mapped to the offered `optionId` at answer time); an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run — the agent itself is answered `cancelled`, so the scenario bug fails the harness rather than being absorbed as an agent-side denial. This is what lets an approval suite drive allow/reject round-trips deterministically from `input.json`. +**`src/launcher.ts`** — `launchAcpTestAgent` owns the common unbuilt-process boundary: absolute tsx loader resolution, `TSX_TSCONFIG_PATH`, isolated harness homes, stdio wiring, a raw-byte stdout tee, stderr and update capture, fail-closed permission fallback, update waiters, and graceful or signalled shutdown. Snapshot scenarios and ordinary e2e suites supply the same `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath`); a test that plays a user supplies only its permission handler. The ACP and hook e2e suites plus the sandbox/approval e2e suite use this launcher instead of rebuilding the SDK client boundary. + +**`src/harness.ts`** — `runScenario` and the input-script/result types layer deterministic steps, temp workspaces, snapshot environment, and persisted-log harvest over the launcher. Its `session/request_permission` handler consumes an optional `InputScript.permissionAnswers` FIFO queue, each entry selecting by option **kind** (ids are agent-issued randoms a committed script cannot know; kinds are the ACP-stable vocabulary, mapped to the offered `optionId` at answer time); an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run — the agent itself is answered `cancelled`, so the scenario bug fails the harness rather than being absorbed as an agent-side denial. This is what lets an approval suite drive allow/reject round-trips deterministically from `input.json`. **`src/normalize.ts`** — the pure normalizers, hook-free by policy: when a future event carries a new volatile field (an approval duration, say), the shared normalizer learns it in the same change, keeping one home for what "normalized" means rather than per-suite scrub extensions. @@ -29,8 +31,8 @@ The machinery lives in [`packages/support/acp-snapshot`](../../../../packages/su ## Testing -Extraction parity was proven mechanically: after the move, `pnpm run test:snapshot` matched the base commit's result with zero byte changes under `examples/acp-agent/tests/snapshots/`. The package's `src/` holds per-file 100% statements/branches/functions/lines under the gating unit run, driven through the REAL spawn path by a scripted fake ACP bin (`tests/fixtures/fake-acp-agent.ts`, behavior scripted per scenario via a `behavior.json` beside the fixture): `harness.spec.ts` covers every step op, both expect-error arms, the permission queue (selection, fallback, impossible-click), env forwarding, workspace seeding, and the harvest ordering/noise/fallback branches; `suite.spec.ts` runs the factory for real at collection time — a replay suite over committed synthetic fixtures and a record suite over a temp copy (write-back never touches the committed tree; `ACP_SNAPSHOT_SPEC_BOOTSTRAP=1` re-bootstraps it) — plus direct cases for the pure helpers. Two structurally unreachable guards carry reasoned `v8 ignore` comments. The fake bin substitutes the `session/new` cwd, not `process.cwd()`, into scripted logs, matching what the real bin's header carries (darwin realpaths `/var/folders/…` to `/private/var/folders/…`). +Extraction parity was proven mechanically: after the move, `pnpm run test:snapshot` matched the base commit's result with zero byte changes under `examples/acp-agent/tests/snapshots/`. The package's `src/` holds per-file 100% statements/branches/functions/lines under the gating unit run, driven through the real launcher by a scripted fake ACP bin (`tests/fixtures/fake-acp-agent.ts`, behavior scripted per scenario via a `behavior.json` beside the fixture): `harness.spec.ts` directly covers launcher defaults, captures, update waiting, shutdown, and environment/config variants, then covers every scenario step op, both expect-error arms, the permission queue (selection, fallback, impossible-click), workspace seeding, and the harvest ordering/noise/fallback branches; `suite.spec.ts` runs the factory for real at collection time — a replay suite over committed synthetic fixtures and a record suite over a temp copy (write-back never touches the committed tree; `ACP_SNAPSHOT_SPEC_BOOTSTRAP=1` re-bootstraps it) — plus direct cases for the pure helpers. The fake bin substitutes the `session/new` cwd, not `process.cwd()`, into scripted logs, matching what the real bin's header carries (darwin realpaths `/var/folders/…` to `/private/var/folders/…`). ## Consequences -A new example gets the whole snapshot tier from a scenario table plus fixtures — the sandbox branch merges master down and adds its own suite (own pin scenario, own overlay, fixtures via `test:snapshot:record`, approvals via `permissionAnswers`). The costs: `suite.ts` imports vitest, so the package is importable only inside a vitest run — a shape no other package has, stated in its README; each suite pins its own ~8 KB header fixture (a genuinely distinct composition deserves its own pin; an identical one would be caught by that suite's uniformity guard); and the e2e launcher duplication remains (`TODO(acp-test-harness)`) — the harness is the extraction target when that migration lands. +A new example gets the whole snapshot tier from a scenario table plus fixtures, while an ordinary ACP e2e gets the same tested process/client boundary from one launcher call. The costs: `suite.ts` imports vitest, so the package entry is importable only inside a vitest run — a shape no other package has, stated in its README; and each suite pins its own ~8 KB header fixture (a genuinely distinct composition deserves its own pin; an identical one would be caught by that suite's uniformity guard). diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index 8ff012c2c3..4b6c675208 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -1,20 +1,14 @@ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' -import { Readable, Writable } from 'node:stream' import { mkdtemp, rm, readFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { afterEach, describe, expect, it } from 'vitest' +import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { - ClientSideConnection, - ndJsonStream, - PROTOCOL_VERSION, - type Agent as AcpAgent, - type Client, - type RequestPermissionRequest, - type RequestPermissionResponse, - type SessionNotification, -} from '@agentclientprotocol/sdk' + launchAcpTestAgent, + type AgentUnderTest, + type LaunchedAcpTestAgent, +} from '@deepseek-ai/dsh-acp-snapshot' /** * End-to-end: boot examples/acp-agent as a real subprocess speaking ACP over @@ -26,127 +20,18 @@ import { * WITHOUT a key, since it only needs the server to boot and answer initialize. */ -// The dsh-acp-agent bin (the demo:acp entry) and this example's cordis.yml. The -// bin resolves its config-path arg from CWD; the subprocess runs from a temp -// workdir, so pass the example config's ABSOLUTE path. -const binScript = fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)) -const configPath = fileURLToPath(new URL('../cordis.yml', import.meta.url)) -// Resolve tsx's loader to an ABSOLUTE path: the subprocess runs with cwd set to -// a temp workdir (this test launches there and uses it as the session cwd; the -// bridge no longer requires cwd === the launch dir, but a temp dir keeps the -// test hermetic), where a bare `--import tsx` would not resolve from -// node_modules. import.meta.resolve gives the worktree's tsx regardless of cwd. -const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) -// Absolute path to the repo-root tsconfig. Dev/test/demo run UNBUILT: the -// `@deepseek-ai/dsh-*` workspace imports resolve through the `paths` map in the -// root tsconfig (tsx reads it), NOT through built `lib/` output. But tsx finds -// that tsconfig by searching UP from the child's cwd — and the child's cwd is a -// temp workdir OUTSIDE the repo, so the search misses and the dsh-* imports fail -// (the child dies before writing a byte). Point tsx at the repo tsconfig -// explicitly via TSX_TSCONFIG_PATH so resolution is cwd-independent. (Without -// this the suite only passed by accident when a stale built `lib/` happened to -// exist — exactly the contamination that masked the inject bug this suite now -// guards.) The repo root is four levels up from this file (examples/acp-agent/tests). -const repoTsconfig = fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)) - -interface Spawned { - child: ChildProcessWithoutNullStreams - client: ClientSideConnection - updates: SessionNotification['update'][] - stderr: string[] +const AGENT: AgentUnderTest = { + binScript: fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)), + configPath: fileURLToPath(new URL('../cordis.yml', import.meta.url)), + tsconfigPath: fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)), } -// TODO(acp-test-harness): this subprocess/client boot glue is duplicated with -// hooks.e2e.ts and partly with dsh-acp-snapshot's harness. Migrate both e2e -// files onto that launcher before the TSX/env/permission-stub details drift. -function spawnAcpAgent(cwd: string, env: NodeJS.ProcessEnv = process.env): Spawned { - const child = spawn( - process.execPath, - ['--import', tsxLoader, binScript, configPath], - { - cwd, - env: { - ...env, - TSX_TSCONFIG_PATH: repoTsconfig, - DSH_HOME: join(cwd, '.dsh'), - DSH_AGENTS_HOME: join(cwd, '.agents'), - }, - stdio: ['pipe', 'pipe', 'pipe'], - }, - ) - const stderr: string[] = [] - child.stderr.setEncoding('utf8') - child.stderr.on('data', (chunk: string) => stderr.push(chunk)) - - const updates: SessionNotification['update'][] = [] - const stream = ndJsonStream( - Writable.toWeb(child.stdin) as WritableStream, - Readable.toWeb(child.stdout) as ReadableStream, - ) - const makeClient = (_agent: AcpAgent): Client => ({ - sessionUpdate(params: SessionNotification): Promise { - updates.push(params.update) - return Promise.resolve() - }, - requestPermission(_params: RequestPermissionRequest): Promise { - // This example composes no ask-producing policy (no hooks), so the - // bridge never prompts here; answer cancelled (fail closed) if it ever - // does — an unexpected prompt must not grant anything. - return Promise.resolve({ outcome: { outcome: 'cancelled' } }) - }, - }) - const client = new ClientSideConnection(makeClient, stream) - return { child, client, updates, stderr } -} - -let spawned: Spawned | undefined +let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined -function hasStdoutLine(out: string[]): boolean { - return out.join('').split('\n').some(line => line.trim().length > 0) -} - -async function waitForStdoutLine(child: ChildProcessWithoutNullStreams, out: string[], stderr: string[], timeoutMs: number): Promise { - await new Promise((resolve, reject) => { - const cleanup = () => { - clearTimeout(timeout) - child.stdout.off('data', onData) - child.off('exit', onExit) - child.off('error', onError) - } - const pass = () => { - cleanup() - resolve() - } - const fail = (reason: string) => { - cleanup() - reject(new Error(`${reason}; stderr: ${stderr.join('')}`)) - } - const onData = () => { - if (hasStdoutLine(out)) pass() - } - const onExit = (code: number | null, signal: NodeJS.Signals | null) => { - fail(`ACP child exited before emitting a stdout frame (code ${code ?? 'null'}, signal ${signal ?? 'null'})`) - } - const onError = (error: Error) => { - fail(`ACP child failed before emitting a stdout frame: ${error.message}`) - } - const timeout = setTimeout(() => { - fail(`ACP child did not emit a stdout frame within ${timeoutMs}ms`) - }, timeoutMs) - - child.stdout.on('data', onData) - child.on('exit', onExit) - child.on('error', onError) - onData() - }) -} - afterEach(async () => { - if (spawned) { - spawned.child.kill('SIGKILL') - spawned = undefined - } + await spawned?.close('SIGKILL') + spawned = undefined if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) workdir = undefined }) @@ -154,39 +39,18 @@ afterEach(async () => { describe('acp-agent over real stdio (no key required)', () => { it('emits only framed JSON-RPC on stdout', async () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) - // Collect raw stdout bytes directly (bypass the SDK framing) to inspect. + // Inspect the launcher's raw-byte tee in addition to driving its SDK client. // A dummy key lets the deepseek adapter APPLY (it only checks the key is // present at boot, not valid — the key is used only on a real model call, // which this purity test never triggers). So this runs WITHOUT real creds. - const child = spawn(process.execPath, ['--import', tsxLoader, binScript, configPath], { + spawned = launchAcpTestAgent({ + agent: AGENT, cwd: workdir, - env: { - ...process.env, - DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot', - TSX_TSCONFIG_PATH: repoTsconfig, - DSH_HOME: join(workdir, '.dsh'), - DSH_AGENTS_HOME: join(workdir, '.agents'), - }, - stdio: ['pipe', 'pipe', 'pipe'], + env: { DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' }, }) - const out: string[] = [] - const stderr: string[] = [] - child.stdout.setEncoding('utf8') - child.stderr.setEncoding('utf8') - child.stdout.on('data', (c: string) => out.push(c)) - child.stderr.on('data', (c: string) => stderr.push(c)) + await spawned.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) - // Send a single initialize request as a newline-delimited JSON-RPC frame. - const req = JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} } }) - child.stdin.write(req + '\n') - - try { - await waitForStdoutLine(child, out, stderr, 15_000) - } finally { - child.kill('SIGKILL') - } - - const lines = out.join('').split('\n').filter(l => l.trim().length > 0) + const lines = spawned.rawStdout().split('\n').filter(line => line.trim().length > 0) expect(lines.length).toBeGreaterThan(0) for (const line of lines) { // Every stdout line MUST parse as JSON (a JSON-RPC frame). A non-JSON @@ -210,7 +74,11 @@ describe('acp-agent over real stdio (no key required)', () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) // A dummy key lets the deepseek adapter boot (it only checks presence, not // validity, at apply time); no model call is made, so the key is never used. - spawned = spawnAcpAgent(workdir, { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' }) + spawned = launchAcpTestAgent({ + agent: AGENT, + cwd: workdir, + env: { DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' }, + }) const { client } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) @@ -223,7 +91,7 @@ describe('acp-agent over real stdio (no key required)', () => { describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over ACP', () => { it('runs a real turn and the agent writes the requested file (verified on disk)', async () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) - spawned = spawnAcpAgent(workdir) + spawned = launchAcpTestAgent({ agent: AGENT, cwd: workdir }) const { client, updates } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) @@ -264,7 +132,7 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over it('with the terminal_output capability, a real bash call renders as a terminal card (content + _meta + exit)', async () => { workdir = await mkdtemp(join(tmpdir(), 'acp-e2e-')) - spawned = spawnAcpAgent(workdir) + spawned = launchAcpTestAgent({ agent: AGENT, cwd: workdir }) const { client, updates } = spawned // Advertise the Zed `_meta.terminal_output` capability so the bridge emits diff --git a/examples/acp-agent/tests/hooks.e2e.ts b/examples/acp-agent/tests/hooks.e2e.ts index bdb800186a..a553addc96 100644 --- a/examples/acp-agent/tests/hooks.e2e.ts +++ b/examples/acp-agent/tests/hooks.e2e.ts @@ -1,20 +1,14 @@ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' -import { Readable, Writable } from 'node:stream' import { mkdtemp, rm, writeFile, access } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { afterEach, describe, expect, it } from 'vitest' +import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { - ClientSideConnection, - ndJsonStream, - PROTOCOL_VERSION, - type Agent as AcpAgent, - type Client, - type RequestPermissionRequest, - type RequestPermissionResponse, - type SessionNotification, -} from '@agentclientprotocol/sdk' + launchAcpTestAgent, + type AgentUnderTest, + type LaunchedAcpTestAgent, +} from '@deepseek-ai/dsh-acp-snapshot' /** * With-key e2e: the Claude Code hook bridge running against the REAL acp-agent @@ -34,54 +28,18 @@ import { * only a real model deciding to call bash exercises the PreToolUse seam live. */ -const binScript = fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)) -const configPath = fileURLToPath(new URL('../cordis.yml', import.meta.url)) -const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) -const repoTsconfig = fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)) - -interface Spawned { - child: ChildProcessWithoutNullStreams - client: ClientSideConnection - updates: SessionNotification['update'][] - stderr: string[] +const AGENT: AgentUnderTest = { + binScript: fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)), + configPath: fileURLToPath(new URL('../cordis.yml', import.meta.url)), + tsconfigPath: fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)), } -function spawnAcpAgent(cwd: string): Spawned { - const child = spawn( - process.execPath, - ['--import', tsxLoader, binScript, configPath], - { cwd, env: { ...process.env, TSX_TSCONFIG_PATH: repoTsconfig }, stdio: ['pipe', 'pipe', 'pipe'] }, - ) - const stderr: string[] = [] - child.stderr.setEncoding('utf8') - child.stderr.on('data', (chunk: string) => stderr.push(chunk)) - - const updates: SessionNotification['update'][] = [] - const stream = ndJsonStream( - Writable.toWeb(child.stdin) as WritableStream, - Readable.toWeb(child.stdout) as ReadableStream, - ) - const makeClient = (_agent: AcpAgent): Client => ({ - sessionUpdate(params: SessionNotification): Promise { - updates.push(params.update) - return Promise.resolve() - }, - requestPermission(_params: RequestPermissionRequest): Promise { - return Promise.resolve({ outcome: { outcome: 'cancelled' } }) - }, - }) - const client = new ClientSideConnection(makeClient, stream) - return { child, client, updates, stderr } -} - -let spawned: Spawned | undefined +let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined afterEach(async () => { - if (spawned) { - spawned.child.kill('SIGKILL') - spawned = undefined - } + await spawned?.close('SIGKILL') + spawned = undefined if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) workdir = undefined }) @@ -96,7 +54,7 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: a PreToolUse hook hooks: { PreToolUse: [{ hooks: [{ type: 'command', command: 'echo "bash blocked by policy" >&2; exit 2' }] }] }, })) - spawned = spawnAcpAgent(workdir) + spawned = launchAcpTestAgent({ agent: AGENT, cwd: workdir }) const { client, updates } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) diff --git a/examples/sandbox-acp-agent/tests/escalation.e2e.ts b/examples/sandbox-acp-agent/tests/escalation.e2e.ts index 93915717a1..a7e8787e36 100644 --- a/examples/sandbox-acp-agent/tests/escalation.e2e.ts +++ b/examples/sandbox-acp-agent/tests/escalation.e2e.ts @@ -1,20 +1,18 @@ -import { spawn, spawnSync, type ChildProcessWithoutNullStreams } from 'node:child_process' -import { Readable, Writable } from 'node:stream' +import { spawnSync } from 'node:child_process' import { mkdtemp, readFile, rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' import { afterEach, describe, expect, it } from 'vitest' import { - ClientSideConnection, - ndJsonStream, PROTOCOL_VERSION, - type Agent as AcpAgent, - type Client, type RequestPermissionRequest, - type RequestPermissionResponse, - type SessionNotification, } from '@agentclientprotocol/sdk' +import { + launchAcpTestAgent, + type AgentUnderTest, + type LaunchedAcpTestAgent, +} from '@deepseek-ai/dsh-acp-snapshot' /** * examples/sandbox-acp-agent end to end. @@ -35,12 +33,11 @@ import { * escalation target the model picks can land the write. */ -const binScript = fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)) -const configPath = fileURLToPath(new URL('../cordis.yml', import.meta.url)) -const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) -// The subprocess runs from a temp cwd OUTSIDE the repo; point tsx at the repo -// tsconfig so the unbuilt `paths` map resolves (see examples/AGENTS.md). -const repoTsconfig = fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)) +const AGENT: AgentUnderTest = { + binScript: fileURLToPath(new URL('../../../packages/ui/acp-agent/src/bin.ts', import.meta.url)), + configPath: fileURLToPath(new URL('../cordis.yml', import.meta.url)), + tsconfigPath: fileURLToPath(new URL('../../../tsconfig.json', import.meta.url)), +} // A usable confining runner, probed the same way the executor suites do: // bwrap on Linux, Seatbelt's sandbox-exec on macOS. Without one the strict @@ -56,44 +53,19 @@ const hasSeatbelt = process.platform === 'darwin' && spawnSync('sandbox-exec', [ }).status === 0 const hasRunner = hasBwrap || hasSeatbelt -interface Spawned { - child: ChildProcessWithoutNullStreams - client: ClientSideConnection - updates: SessionNotification['update'][] +interface Spawned extends LaunchedAcpTestAgent { permissionRequests: RequestPermissionRequest[] - stderr: string[] } /** Boot the example as an ACP subprocess; the scripted client answers every permission prompt with `answer`. */ -function spawnSandboxAcpAgent(cwd: string, answer: 'allow-once' | 'reject-once'): Spawned { - const child = spawn( - process.execPath, - ['--import', tsxLoader, binScript, configPath], - { - cwd, - // A dummy key lets the deepseek adapter boot keyless (presence-checked at - // apply, used only on a real model call); the with-key tests carry the - // real key, so the fallback is inert there. - env: { ...process.env, DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot', TSX_TSCONFIG_PATH: repoTsconfig }, - stdio: ['pipe', 'pipe', 'pipe'], - }, - ) - const stderr: string[] = [] - child.stderr.setEncoding('utf8') - child.stderr.on('data', (chunk: string) => stderr.push(chunk)) - - const updates: SessionNotification['update'][] = [] +function launchSandboxAcpAgent(cwd: string, answer: 'allow-once' | 'reject-once'): Spawned { const permissionRequests: RequestPermissionRequest[] = [] - const stream = ndJsonStream( - Writable.toWeb(child.stdin) as WritableStream, - Readable.toWeb(child.stdout) as ReadableStream, - ) - const makeClient = (_agent: AcpAgent): Client => ({ - sessionUpdate(params: SessionNotification): Promise { - updates.push(params.update) - return Promise.resolve() - }, - requestPermission(params: RequestPermissionRequest): Promise { + const launched = launchAcpTestAgent({ + agent: AGENT, + cwd, + // A dummy key lets the adapter boot keylessly; live tests carry the real key. + env: { DEEPSEEK_API_KEY: process.env.DEEPSEEK_API_KEY ?? 'sk-dummy-for-boot' }, + requestPermission(params) { permissionRequests.push(params) const option = params.options.find(o => o.optionId === answer) // The scripted human: pick the requested option when the prompt offers @@ -102,15 +74,14 @@ function spawnSandboxAcpAgent(cwd: string, answer: 'allow-once' | 'reject-once') return Promise.resolve({ outcome: { outcome: 'selected', optionId: option.optionId } }) }, }) - const client = new ClientSideConnection(makeClient, stream) - return { child, client, updates, permissionRequests, stderr } + return Object.assign(launched, { permissionRequests }) } let spawned: Spawned | undefined let workdir: string | undefined afterEach(async () => { - if (spawned !== undefined && spawned.child.exitCode === null) spawned.child.kill('SIGKILL') + await spawned?.close('SIGKILL') spawned = undefined if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) workdir = undefined @@ -119,7 +90,7 @@ afterEach(async () => { describe('sandbox-acp-agent keyless smoke (real cordis.yml via the Loader)', () => { it('boots the tree — sandbox executor + approval service + bridge — and opens a session', async () => { workdir = await mkdtemp(join(tmpdir(), 'sandbox-acp-smoke-')) - spawned = spawnSandboxAcpAgent(workdir, 'reject-once') + spawned = launchSandboxAcpAgent(workdir, 'reject-once') const { client } = spawned // A dummy key boots the adapter; no prompt is ever sent, so no model call // and no sandbox runner probe happen. This drives the fiber tree the same @@ -132,7 +103,7 @@ describe('sandbox-acp-agent keyless smoke (real cordis.yml via the Loader)', () it('advertises both session config options and honors a switch end to end (no key, no model)', async () => { workdir = await mkdtemp(join(tmpdir(), 'sandbox-acp-config-')) - spawned = spawnSandboxAcpAgent(workdir, 'reject-once') + spawned = launchSandboxAcpAgent(workdir, 'reject-once') const { client } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) // This tree composes bash-sandbox (mode: read-only) + approval → both @@ -164,7 +135,7 @@ describe('sandbox-acp-agent keyless smoke (real cordis.yml via the Loader)', () describe.skipIf(!process.env.DEEPSEEK_API_KEY || !hasRunner)('sandbox-acp-agent e2e: the live approval loop', () => { it('denial → model escalation → editor prompt → allow-once → the retried write lands on disk', async () => { workdir = await mkdtemp(join(tmpdir(), 'sandbox-acp-e2e-')) - spawned = spawnSandboxAcpAgent(workdir, 'allow-once') + spawned = launchSandboxAcpAgent(workdir, 'allow-once') const { client, permissionRequests } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) @@ -193,7 +164,7 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY || !hasRunner)('sandbox-acp-agent it('a rejected escalation stays denied: no write lands, the turn still ends', async () => { workdir = await mkdtemp(join(tmpdir(), 'sandbox-acp-e2e-')) - spawned = spawnSandboxAcpAgent(workdir, 'reject-once') + spawned = launchSandboxAcpAgent(workdir, 'reject-once') const { client, permissionRequests } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) diff --git a/packages/support/README.md b/packages/support/README.md index c8883a89ff..9a05ad3c9a 100644 --- a/packages/support/README.md +++ b/packages/support/README.md @@ -4,9 +4,9 @@ Packages that exist to serve development, testing, and the examples rather than | Package | Role | ctx key | |---|---|---| -| `acp-snapshot/` | ACP snapshot suite kit: subprocess scenario harness + golden normalizers + the `defineAcpSnapshotSuite` factory | (library — imported by example `*.snapshot.ts` suites) | +| `acp-snapshot/` | ACP test kit: shared subprocess/client launcher + snapshot harness, normalizers, and suite factory | (library — imported by ACP e2e and `*.snapshot.ts` suites) | | `invariants/` | Dev-mode event-contract assertions | (listens on `session/*`, `agent/*`) | | `llm-replay/` | Record/replay adapter: short-circuits `llm/stream` from a recorded session JSONL (keyless snapshot tests) | (listens on `llm/stream`) | | `subagent-mock/` | Scripted `SubagentProvider` for deterministic seam/tool tests | (registers on `ctx.subagents`) | -`invariants` runs only in dev mode (contract checks, not runtime behavior). `llm-replay` backs the demos and the snapshot test tier under the per-file coverage gate. `acp-snapshot` carries the snapshot tier's harness/normalizer/suite machinery so every example's suite is a scenario table over one shared, gate-covered implementation. `subagent-mock` exercises the real `ctx.subagents` load path without a model or child agent. A package graduates OUT of `support/` into a product group only when it gains documented product consumers. +`invariants` runs only in dev mode (contract checks, not runtime behavior). `llm-replay` backs the demos and the snapshot test tier under the per-file coverage gate. `acp-snapshot` carries the ACP subprocess/client boundary plus the snapshot harness, normalizers, and suite machinery, so e2e tests share one launcher and every snapshot suite is a scenario table over one gate-covered implementation. `subagent-mock` exercises the real `ctx.subagents` load path without a model or child agent. A package graduates OUT of `support/` into a product group only when it gains documented product consumers. diff --git a/packages/support/acp-snapshot/README.md b/packages/support/acp-snapshot/README.md index ac9c0ceee3..4a43d0eef9 100644 --- a/packages/support/acp-snapshot/README.md +++ b/packages/support/acp-snapshot/README.md @@ -2,8 +2,9 @@ The ACP snapshot suite kit: the shared machinery behind the keyless snapshot tier (`pnpm run test:snapshot`, [testing policy](../../../docs/testing.md)). An example gets a full snapshot suite from a scenario table plus a fixtures directory; every compare/guard mechanic lives here, under the per-file coverage gate, instead of being copied per example. -Three layers, importable separately: +Four layers, importable separately: +- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. - **`runScenario` (harness)** — boots the real agent bin as a subprocess via tsx (unbuilt, Loader path), drives it over ACP JSON-RPC stdio from a deterministic `input.json` script, tees raw stdout for the golden + purity check, and harvests every persisted session JSONL (parent + subagent children, primary-first) after a graceful stdin-EOF shutdown. Parameterized by `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath` — absolute paths; the subprocess cwd is a temp dir outside the repo). - **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept), `scrubSystemPrompts` (prompt text → `{{system}}` in every JSONL), and `scrubRequestHeaders` (the remaining header bulk → `{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)). - **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario golden + re-persisted-log compares, record/refresh fixture write-back, the per-header-class pin (`system-prompt.golden.md` plus the JSONL's full tool schemas) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session..jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time. @@ -39,4 +40,4 @@ A scenario booting a differently-composed tree sets its own `configPath` (an ove The example also ships a `cordis.snapshot.yml` replay overlay next to its `cordis.yml` (the bin swaps them under `DSH_SNAPSHOT=replay` — [single-source replay config RFC](../../../docs/rfc/implemented/testing/2026-07-04-single-source-acp-replay-config.md)); replay fixtures are served by [`dsh-llm-replay`](../llm-replay/README.md), which this package points at via the `DSH_SNAPSHOT_*` env vars it sets on the child. `pnpm run test:snapshot:record` calls the live LLM and rewrites the recorded scenarios' model fixtures; `pnpm run test:snapshot:refresh` stays keyless, runs the replay overlay, and rewrites stdout, comparable session-log goldens, and each pin's Markdown prompt snapshot from the committed model scripts. Fixture roles, record/replay/refresh semantics, and scenario-table fields are documented on `Scenario` and in the [snapshot RFC](../../../docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md). -Constraints: `suite.ts` imports vitest, so the package is importable only inside a vitest run (the harness and normalizers have no such dependency but ship from the same entry). ACP-specific by design — the harness speaks the SDK's `ClientSideConnection`. Permission round-trips are scriptable: `InputScript.permissionAnswers` is a FIFO queue of option-kind selections (`allow_once`, `reject_once`, …) the client maps to the agent-issued `optionId` at answer time; an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run (the agent is answered `cancelled`, so a tolerant agent cannot absorb the scenario bug). Session config options are scriptable too: the `setConfigOption` step switches a knob over `session/set_config_option`, and `setConfigOptionExpectError` asserts the bridge rejects an unknown id or out-of-vocabulary value (the error frame stays in the transcript). +Constraints: `suite.ts` imports vitest, so the package entry is importable only inside a vitest run (the launcher, harness, and normalizers have no such dependency but ship from the same entry). ACP-specific by design — the launcher speaks the SDK's `ClientSideConnection`. Permission round-trips are scriptable: `InputScript.permissionAnswers` is a FIFO queue of option-kind selections (`allow_once`, `reject_once`, …) the client maps to the agent-issued `optionId` at answer time; an absent or exhausted queue answers `cancelled`, and a kind the request never offered rejects the run (the agent is answered `cancelled`, so a tolerant agent cannot absorb the scenario bug). Session config options are scriptable too: the `setConfigOption` step switches a knob over `session/set_config_option`, and `setConfigOptionExpectError` asserts the bridge rejects an unknown id or out-of-vocabulary value (the error frame stays in the transcript). diff --git a/packages/support/acp-snapshot/package.json b/packages/support/acp-snapshot/package.json index 363bc86e25..b14be09c5b 100644 --- a/packages/support/acp-snapshot/package.json +++ b/packages/support/acp-snapshot/package.json @@ -1,6 +1,6 @@ { "name": "@deepseek-ai/dsh-acp-snapshot", - "description": "ACP snapshot suite kit: real-subprocess scenario harness, golden normalizers, and the suite factory behind the keyless snapshot tier", + "description": "ACP test kit: shared subprocess launcher, snapshot scenario harness, golden normalizers, and suite factory", "version": "0.0.1", "private": true, "type": "module", diff --git a/packages/support/acp-snapshot/src/harness.ts b/packages/support/acp-snapshot/src/harness.ts index 491d3ea884..93bb556ba6 100644 --- a/packages/support/acp-snapshot/src/harness.ts +++ b/packages/support/acp-snapshot/src/harness.ts @@ -16,54 +16,20 @@ * @module @deepseek-ai/dsh-acp-snapshot/harness */ -import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' import { cp, mkdtemp, readFile, readdir, rm } from 'node:fs/promises' import { existsSync } from 'node:fs' import { tmpdir } from 'node:os' import { join, delimiter } from 'node:path' -import { fileURLToPath } from 'node:url' -import { Readable, Writable } from 'node:stream' import { ClientSideConnection, - ndJsonStream, PROTOCOL_VERSION, - type Agent as AcpAgent, - type Client, type RequestPermissionRequest, type RequestPermissionResponse, type SessionNotification, } from '@agentclientprotocol/sdk' +import { launchAcpTestAgent, type AgentUnderTest, type LaunchedAcpTestAgent } from './launcher.ts' -// Resolve tsx's ESM loader to an ABSOLUTE path once: the child runs with its -// cwd in a temp dir OUTSIDE the repo, where a bare `--import tsx` would not -// resolve from node_modules. import.meta.resolve gives this package's tsx -// regardless of the child cwd. -const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) - -/** - * The agent composition a scenario runs against: which bin to boot and which - * leaf config it loads. All paths are ABSOLUTE — the subprocess cwd is a temp - * dir outside the repo, so relative resolution would miss; a suite resolves - * them from its own `import.meta.url`. - */ -export interface AgentUnderTest { - /** The agent bin entry (e.g. `packages/ui/acp-agent/src/bin.ts`), run unbuilt via tsx. */ - binScript: string - /** - * The example's live `cordis.yml`. Under `DSH_SNAPSHOT=replay` the bin swaps - * it for the sibling `cordis.snapshot.yml` (the keyless replay overlay), so - * one path serves both modes. - */ - configPath: string - /** - * The repo-root tsconfig whose `paths` map resolves the unbuilt workspace - * imports. Passed to the child as `TSX_TSCONFIG_PATH`: tsx finds a tsconfig - * by searching UP from the child's cwd — a temp dir outside the repo — so - * without the explicit pin the dsh-* imports fail before the bin writes a - * byte. - */ - tsconfigPath: string -} +export type { AgentUnderTest } from './launcher.ts' /** * One step of a scenario's deterministic input script (`input.json`). The @@ -194,11 +160,9 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise // Everything past the temp-dir creation runs under a try/finally that always // removes both dirs — so a failure in workspace seeding, spawn, or any step // never leaks them (the "e2e tests own their resources" rule). - let child: ChildProcessWithoutNullStreams | undefined + let launched: LaunchedAcpTestAgent | undefined let sessionId: string | undefined let sessionLogs: HarvestedLog[] = [] - const rawBuffers: Buffer[] = [] - const stderrChunks: string[] = [] try { // Seed the workspace if the scenario ships one (a file the agent reads/edits). // Copied into the temp cwd so the agent's bash tools see it; the goldens @@ -207,51 +171,15 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise await cp(opts.workspaceDir, cwd, { recursive: true }) } const env: NodeJS.ProcessEnv = { - ...process.env, - TSX_TSCONFIG_PATH: opts.agent.tsconfigPath, DSH_SNAPSHOT: opts.mode, DSH_SNAPSHOT_FILE: opts.fixtureFile, DSH_SNAPSHOT_SESSIONS_ROOT: sessionsRoot, - DSH_HOME: join(cwd, '.dsh'), - DSH_AGENTS_HOME: join(cwd, '.agents'), ...opts.overrideFile !== undefined ? { DSH_SNAPSHOT_OVERRIDE: opts.overrideFile } : {}, ...opts.childFiles !== undefined && opts.childFiles.length > 0 ? { DSH_SNAPSHOT_CHILD_FILES: opts.childFiles.join(delimiter) } : {}, } - child = spawn( - process.execPath, - ['--import', tsxLoader, opts.agent.binScript, opts.configPath ?? opts.agent.configPath], - { cwd, env, stdio: ['pipe', 'pipe', 'pipe'] }, - ) - - child.stderr.setEncoding('utf8') - child.stderr.on('data', (c: string) => stderrChunks.push(c)) - - // Tee raw stdout: accumulate the bytes for the golden + purity check, and ALSO - // feed the same bytes to the SDK client through a passthrough. Buffer the raw - // bytes (not per-chunk utf8 strings) and decode once at the end, so a - // multibyte sequence split across two 'data' events can't corrupt the golden. - const passthrough = new Readable({ read() {} }) - child.stdout.on('data', (buf: Buffer) => { - rawBuffers.push(buf) - passthrough.push(buf) - }) - child.stdout.on('end', () => passthrough.push(null)) - - const stream = ndJsonStream( - Writable.toWeb(child.stdin) as WritableStream, - Readable.toWeb(passthrough) as ReadableStream, - ) - // Watcher so a step can block until the client OBSERVES a particular - // session/update — used by promptAndCancel to pin frame order (send cancel - // only after the streamed agent_message_chunk has arrived, so those frames - // deterministically precede the cancelled prompt response). - const updateWaiters: { match: (u: SessionNotification['update']) => boolean; resolve: () => void }[] = [] - const waitForUpdate = (match: (u: SessionNotification['update']) => boolean): Promise => - new Promise(resolve => updateWaiters.push({ match, resolve })) - // Permission answers are consumed FIFO across the whole run; exhaustion // falls back to `cancelled` so approval-free scenarios keep the plain stub. const permissionQueue = [...input.permissionAnswers ?? []] @@ -263,22 +191,11 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise // callback answers `cancelled` (a well-defined path for the agent), // captures the error here, and the step loop fails the run on it. let scriptError: Error | undefined - const makeClient = (_agent: AcpAgent): Client => ({ - sessionUpdate(params: SessionNotification): Promise { - for (let i = updateWaiters.length - 1; i >= 0; i--) { - const waiter = updateWaiters[i] - // The index is always in-bounds (i only decreases; splice removes at - // i, so lower entries stay valid); the guard satisfies - // noUncheckedIndexedAccess. - /* v8 ignore next 1 -- unreachable in-bounds guard, see above */ - if (waiter === undefined) continue - if (waiter.match(params.update)) { - updateWaiters.splice(i, 1) - waiter.resolve() - } - } - return Promise.resolve() - }, + launched = launchAcpTestAgent({ + agent: opts.agent, + cwd, + ...opts.configPath !== undefined ? { configPath: opts.configPath } : {}, + env, requestPermission(params: RequestPermissionRequest): Promise { const answer = permissionQueue.shift() if (answer === undefined) return Promise.resolve({ outcome: { outcome: 'cancelled' } }) @@ -296,10 +213,11 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise return Promise.resolve({ outcome: { outcome: 'selected', optionId: option.optionId } }) }, }) - const client = new ClientSideConnection(makeClient, stream) + const active = launched + const { client } = active for (const step of input.steps) { - await runStep(client, step, cwd, waitForUpdate, () => sessionId, (id) => { sessionId = id }) + await runStep(client, step, cwd, match => active.waitForUpdate(match), () => sessionId, (id) => { sessionId = id }) // A permission exchange happens while a step's request is in flight, so // by the time the step settles any script bug it exposed is captured — // fail the run HERE, as a harness error, rather than hoping the agent's @@ -308,26 +226,22 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise } // Done driving: close stdin so the server disposes gracefully (flushing // persistence) and exits. Then await exit so the harvested log is complete. - child.stdin.end() - await waitForExit(child) + await active.close() // Harvest EVERY persisted log (parent + any subagent children) while the // temp dirs still exist, ordered primary-first. sessionLogs = await harvestSessionLogs(sessionsRoot) } finally { // Failure-safe teardown: kill a still-running child and drop the temp dirs // even if seeding/spawn/a step/harvest threw, so a flaky run never leaks a - // process or dir. `child` is undefined only if spawn itself threw. - if (child !== undefined && child.exitCode === null && child.signalCode === null) { - child.kill('SIGKILL') - await waitForExit(child) - } + // process or dir. `launched` is undefined only if launch itself threw. + await launched?.close('SIGKILL') await rm(cwd, { recursive: true, force: true }) await rm(sessionsRoot, { recursive: true, force: true }) } return { - rawStdout: Buffer.concat(rawBuffers).toString('utf8'), - stderr: stderrChunks.join(''), + rawStdout: launched.rawStdout(), + stderr: launched.stderr(), cwd, ...sessionId !== undefined ? { sessionId } : {}, sessionLogs, @@ -339,7 +253,7 @@ async function runStep( client: ClientSideConnection, step: InputStep, cwd: string, - waitForUpdate: (match: (u: SessionNotification['update']) => boolean) => Promise, + waitForUpdate: (match: (u: SessionNotification['update']) => boolean) => Promise, getSessionId: () => string | undefined, setSessionId: (id: string) => void, ): Promise { @@ -433,16 +347,6 @@ async function runStep( } } -/** Resolve once the child process exits (any code/signal). */ -function waitForExit(child: ChildProcessWithoutNullStreams): Promise { - // Race guard: both call sites run within one synchronous frame of - // stdin.end()/kill(), so the exit event cannot have been delivered yet; - // kept for any future caller that awaits in between. - /* v8 ignore next 1 -- unreachable race guard, see above */ - if (child.exitCode !== null || child.signalCode !== null) return Promise.resolve() - return new Promise(resolve => child.once('exit', () => { resolve() })) -} - /** * Harvest EVERY persisted `.jsonl` session log under a sessions root, parse each * header line, and return them ordered primary-first: the top-level session (no diff --git a/packages/support/acp-snapshot/src/index.ts b/packages/support/acp-snapshot/src/index.ts index 74bee95385..402bd3aa3d 100644 --- a/packages/support/acp-snapshot/src/index.ts +++ b/packages/support/acp-snapshot/src/index.ts @@ -1,13 +1,14 @@ /** * ACP snapshot suite kit — the shared machinery behind the keyless snapshot - * tier (`pnpm run test:snapshot`). Three layers, composable per example: - * the subprocess scenario harness ({@link runScenario}), the pure golden - * normalizers ({@link normalizeStdout} / {@link normalizeSessionLog} / - * {@link scrubRequestHeaders} / {@link scrubSystemPrompts}), and the suite factory - * ({@link defineAcpSnapshotSuite}) that registers a scenario table as a full - * describe/it tree. An example's `*.snapshot.ts` supplies only its - * {@link AgentUnderTest} paths, its snapshots directory, and its - * {@link Scenario} table. + * tier (`pnpm run test:snapshot`). Four layers, composable per example: the + * shared subprocess/client launcher ({@link launchAcpTestAgent}), the scripted + * scenario harness ({@link runScenario}), the pure golden normalizers + * ({@link normalizeStdout} / {@link normalizeSessionLog} / + * {@link scrubRequestHeaders} / {@link scrubSystemPrompts}), and the suite + * factory ({@link defineAcpSnapshotSuite}) that registers a scenario table as a + * full describe/it tree. Ordinary ACP e2e tests can use the launcher directly; + * an example's `*.snapshot.ts` supplies only its {@link AgentUnderTest} paths, + * snapshots directory, and {@link Scenario} table. * * NOTE: ./suite.ts imports vitest, so this package is importable only inside a * vitest run — a support-tier constraint stated in the README. @@ -17,7 +18,6 @@ export { runScenario, - type AgentUnderTest, type HarvestedLog, type InputScript, type InputStep, @@ -25,6 +25,12 @@ export { type RunOptions, type RunResult, } from './harness.ts' +export { + launchAcpTestAgent, + type AcpTestLaunchOptions, + type AgentUnderTest, + type LaunchedAcpTestAgent, +} from './launcher.ts' export { normalizeSessionLog, normalizeStdout, diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts new file mode 100644 index 0000000000..635a02ca35 --- /dev/null +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -0,0 +1,152 @@ +/** + * Shared launcher for ACP tests that drive an unbuilt agent subprocess over + * JSON-RPC stdio. It owns the tsx loader, workspace-resolution environment, + * stdout tee, SDK client, update collection, permission fallback, and process + * shutdown so e2e and snapshot suites do not each reconstruct that boundary. + * + * @module @deepseek-ai/dsh-acp-snapshot/launcher + */ + +import { spawn, type ChildProcessWithoutNullStreams } from 'node:child_process' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { Readable, Writable } from 'node:stream' +import { + ClientSideConnection, + ndJsonStream, + type Agent as AcpAgent, + type Client, + type RequestPermissionRequest, + type RequestPermissionResponse, + type SessionNotification, +} from '@agentclientprotocol/sdk' + +// The child runs from a temp directory outside the repo, where a bare +// `--import tsx` cannot resolve. Resolve this package's loader once instead. +const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) + +/** The unbuilt agent entry, leaf config, and workspace tsconfig an ACP test boots. */ +export interface AgentUnderTest { + /** The agent bin entry (for example `packages/ui/acp-agent/src/bin.ts`). */ + binScript: string + /** The leaf `cordis.yml` loaded by the bin. */ + configPath: string + /** The repo tsconfig whose paths resolve unbuilt workspace imports. */ + tsconfigPath: string +} + +/** Options for one ACP test subprocess. */ +export interface AcpTestLaunchOptions { + /** The agent composition to boot. */ + agent: AgentUnderTest + /** Process cwd and default session-home root. */ + cwd: string + /** Alternate leaf config for this launch. */ + configPath?: string + /** Extra environment values layered over the parent environment. */ + env?: NodeJS.ProcessEnv + /** Permission handler; omitted requests fail closed as `cancelled`. */ + requestPermission?: (params: RequestPermissionRequest) => Promise +} + +/** A running ACP test process and its captured client-side surfaces. */ +export interface LaunchedAcpTestAgent { + /** The child process, exposed for process-level assertions. */ + child: ChildProcessWithoutNullStreams + /** The SDK connection backed by the child's stdio. */ + client: ClientSideConnection + /** Session updates in receive order. */ + updates: SessionNotification['update'][] + /** Decode all stdout bytes captured so far. */ + rawStdout(): string + /** Decode all stderr chunks captured so far. */ + stderr(): string + /** Resolve when a future session update matches the predicate. */ + waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise + /** Gracefully close stdin, or send a signal, and wait for process exit. */ + close(signal?: NodeJS.Signals): Promise +} + +/** + * Boot an ACP agent subprocess and connect an SDK client to its stdio. + * + * @param options Agent paths, cwd, environment, and optional permission handler. + * @returns The running process, connected client, captures, and shutdown handle. + */ +export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTestAgent { + const { agent, cwd } = options + const child = spawn( + process.execPath, + ['--import', tsxLoader, agent.binScript, options.configPath ?? agent.configPath], + { + cwd, + env: { + ...process.env, + ...options.env, + TSX_TSCONFIG_PATH: agent.tsconfigPath, + DSH_HOME: join(cwd, '.dsh'), + DSH_AGENTS_HOME: join(cwd, '.agents'), + }, + stdio: ['pipe', 'pipe', 'pipe'], + }, + ) + + const stderrChunks: string[] = [] + child.stderr.setEncoding('utf8') + child.stderr.on('data', (chunk: string) => stderrChunks.push(chunk)) + + const rawBuffers: Buffer[] = [] + const passthrough = new Readable({ read() {} }) + child.stdout.on('data', (buffer: Buffer) => { + rawBuffers.push(buffer) + passthrough.push(buffer) + }) + child.stdout.on('end', () => passthrough.push(null)) + + const updates: SessionNotification['update'][] = [] + const updateWaiters: { + match: (update: SessionNotification['update']) => boolean + resolve: (update: SessionNotification['update']) => void + }[] = [] + const stream = ndJsonStream( + Writable.toWeb(child.stdin) as WritableStream, + Readable.toWeb(passthrough) as ReadableStream, + ) + const makeClient = (_agent: AcpAgent): Client => ({ + sessionUpdate(params: SessionNotification): Promise { + updates.push(params.update) + for (let index = updateWaiters.length - 1; index >= 0; index--) { + const waiter = updateWaiters[index] + /* v8 ignore next 1 -- index is bounded by the array length */ + if (waiter === undefined) continue + if (!waiter.match(params.update)) continue + updateWaiters.splice(index, 1) + waiter.resolve(params.update) + } + return Promise.resolve() + }, + requestPermission: options.requestPermission + ?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' } })), + }) + const client = new ClientSideConnection(makeClient, stream) + + return { + child, + client, + updates, + rawStdout: () => Buffer.concat(rawBuffers).toString('utf8'), + stderr: () => stderrChunks.join(''), + waitForUpdate: match => new Promise(resolve => updateWaiters.push({ match, resolve })), + async close(signal?: NodeJS.Signals): Promise { + if (child.exitCode !== null || child.signalCode !== null) return + if (signal === undefined) child.stdin.end() + else child.kill(signal) + await waitForExit(child) + }, + } +} + +/** Resolve once a running child exits. */ +function waitForExit(child: ChildProcessWithoutNullStreams): Promise { + return new Promise(resolve => child.once('exit', () => { resolve() })) +} diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index b0817a8d04..ec1ed5cf6f 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -3,7 +3,9 @@ import { tmpdir } from 'node:os' import { delimiter, join } from 'node:path' import { fileURLToPath } from 'node:url' import { afterAll, describe, expect, it } from 'vitest' +import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { runScenario, type AgentUnderTest, type InputStep } from '../src/harness.ts' +import { launchAcpTestAgent } from '../src/launcher.ts' /** * Unit tests for the subprocess harness, driven through the REAL spawn path @@ -38,6 +40,37 @@ async function scenario(behavior: object): Promise<{ dir: string; fixtureFile: s const boot: InputStep[] = [{ op: 'initialize' }, { op: 'newSession' }] describe('runScenario', () => { + it('centralizes ACP boot, captures, updates, fail-closed permissions, and shutdown', { timeout: 20_000 }, async () => { + const { dir, fixtureFile } = await scenario({ permissionProbe: true, echoEnv: true, stderrNote: 'launcher stderr' }) + const sessionsRoot = await mkdtemp(join(tmpdir(), 'acp-launcher-sessions-')) + tempDirs.push(sessionsRoot) + const launched = launchAcpTestAgent({ + agent: AGENT, + cwd: dir, + configPath: AGENT.configPath, + env: { + DSH_SNAPSHOT: 'replay', + DSH_SNAPSHOT_FILE: fixtureFile, + DSH_SNAPSHOT_SESSIONS_ROOT: sessionsRoot, + }, + }) + await launched.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await launched.client.newSession({ cwd: dir, mcpServers: [] }) + const nextChunk = launched.waitForUpdate(update => update.sessionUpdate === 'agent_message_chunk') + await launched.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'go' }] }) + expect((await nextChunk).sessionUpdate).toBe('agent_message_chunk') + expect(launched.updates.some(update => update.sessionUpdate === 'agent_message_chunk')).toBe(true) + expect(launched.rawStdout()).toContain('permission:{\\"outcome\\":\\"cancelled\\"}') + expect(launched.stderr()).toContain('launcher stderr') + await launched.close() + await launched.close('SIGKILL') + + // The minimal shape needs no environment or config override. + const minimal = launchAcpTestAgent({ agent: AGENT, cwd: dir }) + await minimal.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await minimal.close() + }) + it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ permissionProbe: true, From 9028c9b63b6c3ed1737c168ae5e1548bab064668 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 05:35:34 +0800 Subject: [PATCH 02/16] fix: contain ACP update predicate failures --- packages/support/acp-snapshot/src/launcher.ts | 13 +++++++++++-- packages/support/acp-snapshot/tests/harness.spec.ts | 4 ++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 635a02ca35..975b1bb852 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -107,6 +107,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe const updateWaiters: { match: (update: SessionNotification['update']) => boolean resolve: (update: SessionNotification['update']) => void + reject: (reason: unknown) => void }[] = [] const stream = ndJsonStream( Writable.toWeb(child.stdin) as WritableStream, @@ -119,7 +120,15 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe const waiter = updateWaiters[index] /* v8 ignore next 1 -- index is bounded by the array length */ if (waiter === undefined) continue - if (!waiter.match(params.update)) continue + let matches: boolean + try { + matches = waiter.match(params.update) + } catch (error: unknown) { + updateWaiters.splice(index, 1) + waiter.reject(error) + continue + } + if (!matches) continue updateWaiters.splice(index, 1) waiter.resolve(params.update) } @@ -136,7 +145,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe updates, rawStdout: () => Buffer.concat(rawBuffers).toString('utf8'), stderr: () => stderrChunks.join(''), - waitForUpdate: match => new Promise(resolve => updateWaiters.push({ match, resolve })), + waitForUpdate: match => new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })), async close(signal?: NodeJS.Signals): Promise { if (child.exitCode !== null || child.signalCode !== null) return if (signal === undefined) child.stdin.end() diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index ec1ed5cf6f..db715db90c 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -57,7 +57,11 @@ describe('runScenario', () => { await launched.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) const { sessionId } = await launched.client.newSession({ cwd: dir, mcpServers: [] }) const nextChunk = launched.waitForUpdate(update => update.sessionUpdate === 'agent_message_chunk') + const predicateFailure = new Error('predicate failed') + const failedPredicate = launched.waitForUpdate(() => { throw predicateFailure }) + .catch((error: unknown): unknown => error) await launched.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'go' }] }) + expect(await failedPredicate).toBe(predicateFailure) expect((await nextChunk).sessionUpdate).toBe('agent_message_chunk') expect(launched.updates.some(update => update.sessionUpdate === 'agent_message_chunk')).toBe(true) expect(launched.rawStdout()).toContain('permission:{\\"outcome\\":\\"cancelled\\"}') From 60ce23d77c52b6f4c183c320d2a1b54c4aedea7f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:07:37 +0800 Subject: [PATCH 03/16] fix: surface ACP launcher spawn failures --- packages/support/acp-snapshot/README.md | 2 +- packages/support/acp-snapshot/src/harness.ts | 1 + packages/support/acp-snapshot/src/launcher.ts | 22 ++++++++++++++++++- .../acp-snapshot/tests/harness.spec.ts | 13 ++++++++++- 4 files changed, 35 insertions(+), 3 deletions(-) diff --git a/packages/support/acp-snapshot/README.md b/packages/support/acp-snapshot/README.md index 4a43d0eef9..0058e9116f 100644 --- a/packages/support/acp-snapshot/README.md +++ b/packages/support/acp-snapshot/README.md @@ -4,7 +4,7 @@ The ACP snapshot suite kit: the shared machinery behind the keyless snapshot tie Four layers, importable separately: -- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. +- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through its startup lifecycle, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. - **`runScenario` (harness)** — boots the real agent bin as a subprocess via tsx (unbuilt, Loader path), drives it over ACP JSON-RPC stdio from a deterministic `input.json` script, tees raw stdout for the golden + purity check, and harvests every persisted session JSONL (parent + subagent children, primary-first) after a graceful stdin-EOF shutdown. Parameterized by `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath` — absolute paths; the subprocess cwd is a temp dir outside the repo). - **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept), `scrubSystemPrompts` (prompt text → `{{system}}` in every JSONL), and `scrubRequestHeaders` (the remaining header bulk → `{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)). - **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario golden + re-persisted-log compares, record/refresh fixture write-back, the per-header-class pin (`system-prompt.golden.md` plus the JSONL's full tool schemas) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session..jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time. diff --git a/packages/support/acp-snapshot/src/harness.ts b/packages/support/acp-snapshot/src/harness.ts index 93bb556ba6..d04b0bc2b5 100644 --- a/packages/support/acp-snapshot/src/harness.ts +++ b/packages/support/acp-snapshot/src/harness.ts @@ -214,6 +214,7 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise }, }) const active = launched + await active.spawned const { client } = active for (const step of input.steps) { diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 975b1bb852..3508b02ec5 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -53,6 +53,8 @@ export interface AcpTestLaunchOptions { export interface LaunchedAcpTestAgent { /** The child process, exposed for process-level assertions. */ child: ChildProcessWithoutNullStreams + /** Resolve when the OS spawns the child; reject with its asynchronous spawn failure. */ + spawned: Promise /** The SDK connection backed by the child's stdio. */ client: ClientSideConnection /** Session updates in receive order. */ @@ -90,6 +92,18 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe stdio: ['pipe', 'pipe', 'pipe'], }, ) + // A spawn-level failure is an asynchronous `error` event. Observe it in the + // same tick as spawn so a missing cwd or OS rejection cannot crash the test + // runner, then make startup and shutdown surface the original error. + const childFailure = new Promise(resolve => child.once('error', resolve)) + const spawned = Promise.race([ + new Promise(resolve => child.once('spawn', resolve)), + childFailure.then((error): never => { throw error }), + ]) + // `spawned` is public and close() also awaits it, but a caller may ignore both. + // Keep that misuse from turning the already-observed child error into an + // unhandled promise rejection. + void spawned.catch(() => undefined) const stderrChunks: string[] = [] child.stderr.setEncoding('utf8') @@ -141,16 +155,22 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe return { child, + spawned, client, updates, rawStdout: () => Buffer.concat(rawBuffers).toString('utf8'), stderr: () => stderrChunks.join(''), waitForUpdate: match => new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })), async close(signal?: NodeJS.Signals): Promise { + await spawned if (child.exitCode !== null || child.signalCode !== null) return if (signal === undefined) child.stdin.end() else child.kill(signal) - await waitForExit(child) + const failure = await Promise.race([ + waitForExit(child).then((): undefined => undefined), + childFailure, + ]) + if (failure !== undefined) throw failure }, } } diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index db715db90c..69c02a407f 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -40,6 +40,13 @@ async function scenario(behavior: object): Promise<{ dir: string; fixtureFile: s const boot: InputStep[] = [{ op: 'initialize' }, { op: 'newSession' }] describe('runScenario', () => { + it('surfaces an asynchronous child spawn failure through startup and close', async () => { + const { dir } = await scenario({}) + const launched = launchAcpTestAgent({ agent: AGENT, cwd: join(dir, 'missing') }) + await expect(launched.spawned).rejects.toMatchObject({ code: 'ENOENT' }) + await expect(launched.close()).rejects.toMatchObject({ code: 'ENOENT' }) + }) + it('centralizes ACP boot, captures, updates, fail-closed permissions, and shutdown', { timeout: 20_000 }, async () => { const { dir, fixtureFile } = await scenario({ permissionProbe: true, echoEnv: true, stderrNote: 'launcher stderr' }) const sessionsRoot = await mkdtemp(join(tmpdir(), 'acp-launcher-sessions-')) @@ -72,7 +79,11 @@ describe('runScenario', () => { // The minimal shape needs no environment or config override. const minimal = launchAcpTestAgent({ agent: AGENT, cwd: dir }) await minimal.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) - await minimal.close() + const childFailure = new Error('child process failed') + const exited = new Promise(resolve => minimal.child.once('exit', () => { resolve() })) + minimal.child.emit('error', childFailure) + await expect(minimal.close('SIGKILL')).rejects.toBe(childFailure) + await exited }) it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { From 140d32681ef2fc4b35dbae710044c7b7c4e693b7 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 06:33:22 +0800 Subject: [PATCH 04/16] fix: make ACP test teardown failure-safe --- examples/acp-agent/tests/acp.e2e.ts | 14 ++++-- examples/acp-agent/tests/hooks.e2e.ts | 14 ++++-- .../sandbox-acp-agent/tests/escalation.e2e.ts | 14 ++++-- packages/support/acp-snapshot/README.md | 2 +- packages/support/acp-snapshot/src/harness.ts | 44 ++++++++++++------- packages/support/acp-snapshot/src/launcher.ts | 25 +++++++++-- .../acp-snapshot/tests/harness.spec.ts | 8 ++-- 7 files changed, 86 insertions(+), 35 deletions(-) diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index 4b6c675208..871006dfb4 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -30,10 +30,16 @@ let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined afterEach(async () => { - await spawned?.close('SIGKILL') - spawned = undefined - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - workdir = undefined + try { + await spawned?.close('SIGKILL') + } finally { + spawned = undefined + try { + if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) + } finally { + workdir = undefined + } + } }) describe('acp-agent over real stdio (no key required)', () => { diff --git a/examples/acp-agent/tests/hooks.e2e.ts b/examples/acp-agent/tests/hooks.e2e.ts index a553addc96..8dc169f437 100644 --- a/examples/acp-agent/tests/hooks.e2e.ts +++ b/examples/acp-agent/tests/hooks.e2e.ts @@ -38,10 +38,16 @@ let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined afterEach(async () => { - await spawned?.close('SIGKILL') - spawned = undefined - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - workdir = undefined + try { + await spawned?.close('SIGKILL') + } finally { + spawned = undefined + try { + if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) + } finally { + workdir = undefined + } + } }) describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: a PreToolUse hook blocks bash (real model)', () => { diff --git a/examples/sandbox-acp-agent/tests/escalation.e2e.ts b/examples/sandbox-acp-agent/tests/escalation.e2e.ts index a7e8787e36..0ddaa6426e 100644 --- a/examples/sandbox-acp-agent/tests/escalation.e2e.ts +++ b/examples/sandbox-acp-agent/tests/escalation.e2e.ts @@ -81,10 +81,16 @@ let spawned: Spawned | undefined let workdir: string | undefined afterEach(async () => { - await spawned?.close('SIGKILL') - spawned = undefined - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - workdir = undefined + try { + await spawned?.close('SIGKILL') + } finally { + spawned = undefined + try { + if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) + } finally { + workdir = undefined + } + } }) describe('sandbox-acp-agent keyless smoke (real cordis.yml via the Loader)', () => { diff --git a/packages/support/acp-snapshot/README.md b/packages/support/acp-snapshot/README.md index 0058e9116f..26192f1cac 100644 --- a/packages/support/acp-snapshot/README.md +++ b/packages/support/acp-snapshot/README.md @@ -4,7 +4,7 @@ The ACP snapshot suite kit: the shared machinery behind the keyless snapshot tie Four layers, importable separately: -- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through its startup lifecycle, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. +- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through its startup lifecycle, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Shutdown waits for process exit before resolving or propagating a child error, so callers can remove owned paths after either outcome. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. - **`runScenario` (harness)** — boots the real agent bin as a subprocess via tsx (unbuilt, Loader path), drives it over ACP JSON-RPC stdio from a deterministic `input.json` script, tees raw stdout for the golden + purity check, and harvests every persisted session JSONL (parent + subagent children, primary-first) after a graceful stdin-EOF shutdown. Parameterized by `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath` — absolute paths; the subprocess cwd is a temp dir outside the repo). - **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept), `scrubSystemPrompts` (prompt text → `{{system}}` in every JSONL), and `scrubRequestHeaders` (the remaining header bulk → `{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)). - **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario golden + re-persisted-log compares, record/refresh fixture write-back, the per-header-class pin (`system-prompt.golden.md` plus the JSONL's full tool schemas) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session..jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time. diff --git a/packages/support/acp-snapshot/src/harness.ts b/packages/support/acp-snapshot/src/harness.ts index d04b0bc2b5..3db1355b0f 100644 --- a/packages/support/acp-snapshot/src/harness.ts +++ b/packages/support/acp-snapshot/src/harness.ts @@ -163,7 +163,7 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise let launched: LaunchedAcpTestAgent | undefined let sessionId: string | undefined let sessionLogs: HarvestedLog[] = [] - try { + const outcome = await (async (): Promise => { // Seed the workspace if the scenario ships one (a file the agent reads/edits). // Copied into the temp cwd so the agent's bash tools see it; the goldens // normalize the cwd, so the seeded paths stay stable across runs. @@ -231,22 +231,36 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise // Harvest EVERY persisted log (parent + any subagent children) while the // temp dirs still exist, ordered primary-first. sessionLogs = await harvestSessionLogs(sessionsRoot) - } finally { - // Failure-safe teardown: kill a still-running child and drop the temp dirs - // even if seeding/spawn/a step/harvest threw, so a flaky run never leaks a - // process or dir. `launched` is undefined only if launch itself threw. - await launched?.close('SIGKILL') - await rm(cwd, { recursive: true, force: true }) - await rm(sessionsRoot, { recursive: true, force: true }) - } + return { + rawStdout: launched.rawStdout(), + stderr: launched.stderr(), + cwd, + ...sessionId !== undefined ? { sessionId } : {}, + sessionLogs, + } + })().then( + value => ({ status: 'fulfilled', value } as const), + (error: unknown) => ({ status: 'rejected', error } as const), + ) - return { - rawStdout: launched.rawStdout(), - stderr: launched.stderr(), - cwd, - ...sessionId !== undefined ? { sessionId } : {}, - sessionLogs, + // Failure-safe teardown: wait for a still-running child, then attempt BOTH + // directory removals even when an earlier cleanup rejects. The main outcome + // wins over teardown noise so a step/harvest failure is never replaced; on a + // successful run, the first cleanup failure remains visible to the caller. + const cleanupResults: PromiseSettledResult[] = [] + const cleanup = async (action: () => Promise): Promise => { + cleanupResults.push(...await Promise.allSettled([action()])) } + /* v8 ignore next 1 -- launch itself can only throw on a defensive synchronous spawn API failure */ + await cleanup(() => launched?.close('SIGKILL') ?? Promise.resolve()) + await cleanup(() => rm(cwd, { recursive: true, force: true })) + await cleanup(() => rm(sessionsRoot, { recursive: true, force: true })) + + if (outcome.status === 'rejected') throw outcome.error + const cleanupFailure = cleanupResults.find((result): result is PromiseRejectedResult => result.status === 'rejected') + /* v8 ignore next 1 -- defensive OS cleanup failure after an otherwise successful real subprocess run */ + if (cleanupFailure !== undefined) throw cleanupFailure.reason + return outcome.value } /** Drive one input step over the client connection. */ diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 3508b02ec5..7f2ec9b0a7 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -95,7 +95,10 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // A spawn-level failure is an asynchronous `error` event. Observe it in the // same tick as spawn so a missing cwd or OS rejection cannot crash the test // runner, then make startup and shutdown surface the original error. - const childFailure = new Promise(resolve => child.once('error', resolve)) + // Keep observing after the first error: a fallback kill attempted during + // shutdown may itself report another process error, which must not become an + // unhandled EventEmitter error after the promise has already settled. + const childFailure = new Promise(resolve => child.on('error', resolve)) const spawned = Promise.race([ new Promise(resolve => child.once('spawn', resolve)), childFailure.then((error): never => { throw error }), @@ -163,14 +166,23 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe waitForUpdate: match => new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })), async close(signal?: NodeJS.Signals): Promise { await spawned - if (child.exitCode !== null || child.signalCode !== null) return + if (!isRunning(child)) return + const exited = waitForExit(child) if (signal === undefined) child.stdin.end() else child.kill(signal) const failure = await Promise.race([ - waitForExit(child).then((): undefined => undefined), + exited.then((): undefined => undefined), childFailure, ]) - if (failure !== undefined) throw failure + if (failure === undefined) return + + // An `error` after spawn is not an exit edge: in particular, a failed + // signal can leave the subprocess live. Force termination, await the + // already-observed exit edge, and only then propagate the child error so + // callers may safely remove cwd/session resources after close rejects. + child.kill('SIGKILL') + await exited + throw failure }, } } @@ -179,3 +191,8 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe function waitForExit(child: ChildProcessWithoutNullStreams): Promise { return new Promise(resolve => child.once('exit', () => { resolve() })) } + +/** Whether the child still lacks either OS termination marker. */ +function isRunning(child: ChildProcessWithoutNullStreams): boolean { + return child.exitCode === null && child.signalCode === null +} diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 69c02a407f..3c1bb0d44a 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -80,10 +80,12 @@ describe('runScenario', () => { const minimal = launchAcpTestAgent({ agent: AGENT, cwd: dir }) await minimal.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) const childFailure = new Error('child process failed') - const exited = new Promise(resolve => minimal.child.once('exit', () => { resolve() })) + let exited = false + minimal.child.once('exit', () => { exited = true }) minimal.child.emit('error', childFailure) - await expect(minimal.close('SIGKILL')).rejects.toBe(childFailure) - await exited + await expect(minimal.close('SIGTERM')).rejects.toBe(childFailure) + // close rejects only after the fallback SIGKILL has produced an exit edge. + expect(exited).toBe(true) }) it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { From e2a5a160d3763ee7faaa9b4235ae7284144f05ee Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 07:00:10 +0800 Subject: [PATCH 05/16] fix: settle ACP update waiters on shutdown --- packages/support/acp-snapshot/src/launcher.ts | 43 ++++++++++++++----- .../acp-snapshot/tests/harness.spec.ts | 3 ++ 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 7f2ec9b0a7..253e67fd5a 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -114,18 +114,26 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe const rawBuffers: Buffer[] = [] const passthrough = new Readable({ read() {} }) - child.stdout.on('data', (buffer: Buffer) => { - rawBuffers.push(buffer) - passthrough.push(buffer) - }) - child.stdout.on('end', () => passthrough.push(null)) - const updates: SessionNotification['update'][] = [] const updateWaiters: { match: (update: SessionNotification['update']) => boolean resolve: (update: SessionNotification['update']) => void reject: (reason: unknown) => void }[] = [] + let updateStreamFailure: Error | undefined + const closeUpdateStream = (): void => { + if (updateStreamFailure !== undefined) return + updateStreamFailure = new Error('ACP test agent update stream closed before a matching session update arrived') + for (const waiter of updateWaiters.splice(0)) waiter.reject(updateStreamFailure) + } + child.stdout.on('data', (buffer: Buffer) => { + rawBuffers.push(buffer) + passthrough.push(buffer) + }) + child.stdout.on('end', () => { + passthrough.push(null) + closeUpdateStream() + }) const stream = ndJsonStream( Writable.toWeb(child.stdin) as WritableStream, Readable.toWeb(passthrough) as ReadableStream, @@ -163,10 +171,21 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe updates, rawStdout: () => Buffer.concat(rawBuffers).toString('utf8'), stderr: () => stderrChunks.join(''), - waitForUpdate: match => new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })), + waitForUpdate(match): Promise { + if (updateStreamFailure !== undefined) return Promise.reject(updateStreamFailure) + return new Promise((resolve, reject) => updateWaiters.push({ match, resolve, reject })) + }, async close(signal?: NodeJS.Signals): Promise { - await spawned - if (!isRunning(child)) return + try { + await spawned + } catch (error: unknown) { + closeUpdateStream() + throw error + } + if (!isRunning(child)) { + closeUpdateStream() + return + } const exited = waitForExit(child) if (signal === undefined) child.stdin.end() else child.kill(signal) @@ -174,7 +193,10 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe exited.then((): undefined => undefined), childFailure, ]) - if (failure === undefined) return + if (failure === undefined) { + closeUpdateStream() + return + } // An `error` after spawn is not an exit edge: in particular, a failed // signal can leave the subprocess live. Force termination, await the @@ -182,6 +204,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // callers may safely remove cwd/session resources after close rejects. child.kill('SIGKILL') await exited + closeUpdateStream() throw failure }, } diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 3c1bb0d44a..192d36dc6c 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -73,7 +73,10 @@ describe('runScenario', () => { expect(launched.updates.some(update => update.sessionUpdate === 'agent_message_chunk')).toBe(true) expect(launched.rawStdout()).toContain('permission:{\\"outcome\\":\\"cancelled\\"}') expect(launched.stderr()).toContain('launcher stderr') + const unmatched = expect(launched.waitForUpdate(() => false)).rejects.toThrow(/update stream closed/) await launched.close() + await unmatched + await expect(launched.waitForUpdate(() => true)).rejects.toThrow(/update stream closed/) await launched.close('SIGKILL') // The minimal shape needs no environment or config override. From 0b492e6e62be53abb8560d16f806a4078f56c651 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 07:22:22 +0800 Subject: [PATCH 06/16] fix: drain ACP test launcher streams --- packages/support/acp-snapshot/README.md | 2 +- packages/support/acp-snapshot/src/launcher.ts | 17 +++++++++++++-- .../tests/fixtures/fake-acp-agent.ts | 21 +++++++++++++++++++ .../acp-snapshot/tests/harness.spec.ts | 21 +++++++++++++++++++ 4 files changed, 58 insertions(+), 3 deletions(-) diff --git a/packages/support/acp-snapshot/README.md b/packages/support/acp-snapshot/README.md index 26192f1cac..b95615abea 100644 --- a/packages/support/acp-snapshot/README.md +++ b/packages/support/acp-snapshot/README.md @@ -4,7 +4,7 @@ The ACP snapshot suite kit: the shared machinery behind the keyless snapshot tie Four layers, importable separately: -- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through its startup lifecycle, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Shutdown waits for process exit before resolving or propagating a child error, so callers can remove owned paths after either outcome. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. +- **`launchAcpTestAgent` (launcher)** — boots an unbuilt ACP agent from a temp cwd, pins tsx to the repo tsconfig, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through its startup lifecycle, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Shutdown waits for process exit, inherited stdio closure, and ACP parser exhaustion before resolving or propagating a child error, so captures are complete and callers can remove owned paths after either outcome. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy. - **`runScenario` (harness)** — boots the real agent bin as a subprocess via tsx (unbuilt, Loader path), drives it over ACP JSON-RPC stdio from a deterministic `input.json` script, tees raw stdout for the golden + purity check, and harvests every persisted session JSONL (parent + subagent children, primary-first) after a graceful stdin-EOF shutdown. Parameterized by `AgentUnderTest` (`binScript`, `configPath`, `tsconfigPath` — absolute paths; the subprocess cwd is a temp dir outside the repo). - **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept), `scrubSystemPrompts` (prompt text → `{{system}}` in every JSONL), and `scrubRequestHeaders` (the remaining header bulk → `{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)). - **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario golden + re-persisted-log compares, record/refresh fixture write-back, the per-header-class pin (`system-prompt.golden.md` plus the JSONL's full tool schemas) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session..jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time. diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 253e67fd5a..815753ae5f 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -65,7 +65,7 @@ export interface LaunchedAcpTestAgent { stderr(): string /** Resolve when a future session update matches the predicate. */ waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise - /** Gracefully close stdin, or send a signal, and wait for process exit. */ + /** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, and ACP parser drain. */ close(signal?: NodeJS.Signals): Promise } @@ -132,7 +132,6 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe }) child.stdout.on('end', () => { passthrough.push(null) - closeUpdateStream() }) const stream = ndJsonStream( Writable.toWeb(child.stdin) as WritableStream, @@ -163,6 +162,17 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe ?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' } })), }) const client = new ClientSideConnection(makeClient, stream) + // `exit` only reports the parent process's status. Descendants may retain + // inherited stdout/stderr handles and buffered ACP frames may still be + // crossing the SDK parser. Node's `close` follows stdio closure; the SDK's + // `closed` follows parser exhaustion. Capture both eagerly so a caller that + // invokes close after process exit still joins the complete drain boundary. + const stdioClosed = new Promise(resolve => child.once('close', () => { resolve() })) + const drained = Promise.all([stdioClosed, client.closed]).then(() => undefined) + // A caller may await a pending update without calling close(). Make natural + // stream exhaustion terminal for those waiters too, but only after the + // parser has dispatched every buffered frame. + void client.closed.then(closeUpdateStream) return { child, @@ -183,6 +193,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe throw error } if (!isRunning(child)) { + await drained closeUpdateStream() return } @@ -194,6 +205,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe childFailure, ]) if (failure === undefined) { + await drained closeUpdateStream() return } @@ -204,6 +216,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // callers may safely remove cwd/session resources after close rejects. child.kill('SIGKILL') await exited + await drained closeUpdateStream() throw failure }, diff --git a/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts b/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts index 861c9d2b04..9b0760213c 100644 --- a/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts +++ b/packages/support/acp-snapshot/tests/fixtures/fake-acp-agent.ts @@ -18,6 +18,7 @@ import { mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { readdirSync } from 'node:fs' +import { spawn } from 'node:child_process' import { dirname, join } from 'node:path' import { randomUUID } from 'node:crypto' import { createInterface } from 'node:readline' @@ -50,6 +51,8 @@ interface Behavior { echoWorkspace?: boolean /** Write a line to stderr on boot (spec-side stderr-capture assertions). */ stderrNote?: string + /** Let a short-lived descendant retain stdio and emit one final ACP update plus stderr line after this parent exits. */ + lateInheritedOutput?: boolean /** Session logs to persist on stdin EOF. */ logs?: ScriptedLog[] /** Leave a stray FILE directly under the sessions root (harvest must skip it). */ @@ -256,6 +259,24 @@ function flushLogsAndExit(): void { writeFileSync(join(sessionsRoot, 'bucket-noise', 'notes.txt'), 'not a session log\n') } if (behavior.deleteSessionsRoot === true) rmSync(sessionsRoot, { recursive: true, force: true }) + if (behavior.lateInheritedOutput === true) { + const frame = JSON.stringify({ + jsonrpc: '2.0', + method: 'session/update', + params: { + sessionId, + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: 'late inherited stdout' }, + }, + }, + }) + const code = [ + `setTimeout(() => process.stdout.write(${JSON.stringify(`${frame}\n`)}), 50)`, + `setTimeout(() => process.stderr.write(${JSON.stringify('late inherited stderr\n')}), 75)`, + ].join(';') + spawn(process.execPath, ['-e', code], { stdio: ['ignore', 1, 2] }).unref() + } process.exit(0) } diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 192d36dc6c..051db6a0ba 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -91,6 +91,27 @@ describe('runScenario', () => { expect(exited).toBe(true) }) + it('waits for inherited stdio and buffered ACP parsing after the parent exits', { timeout: 20_000 }, async () => { + const { dir, fixtureFile } = await scenario({ lateInheritedOutput: true }) + const launched = launchAcpTestAgent({ + agent: AGENT, + cwd: dir, + env: { DSH_SNAPSHOT_FILE: fixtureFile }, + }) + await launched.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await launched.client.newSession({ cwd: dir, mcpServers: [] }) + const lateUpdate = launched.waitForUpdate(update => + update.sessionUpdate === 'agent_message_chunk' + && update.content.type === 'text' + && update.content.text === 'late inherited stdout') + + await launched.close() + + await expect(lateUpdate).resolves.toMatchObject({ sessionUpdate: 'agent_message_chunk' }) + expect(launched.rawStdout()).toContain('late inherited stdout') + expect(launched.stderr()).toContain('late inherited stderr') + }) + it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ permissionProbe: true, From e784e4dce5f5178b5e22a3d3376599144d8bea1b Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:12:32 +0800 Subject: [PATCH 07/16] fix: await ACP client callbacks during shutdown --- packages/support/acp-snapshot/src/launcher.ts | 59 ++++++++++++------- .../acp-snapshot/tests/harness.spec.ts | 36 +++++++++++ 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 815753ae5f..30702a5888 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -65,7 +65,7 @@ export interface LaunchedAcpTestAgent { stderr(): string /** Resolve when a future session update matches the predicate. */ waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise - /** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, and ACP parser drain. */ + /** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, ACP parsing, and client callbacks. */ close(signal?: NodeJS.Signals): Promise } @@ -137,29 +137,41 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe Writable.toWeb(child.stdin) as WritableStream, Readable.toWeb(passthrough) as ReadableStream, ) + const inFlightClientCallbacks = new Set>() + const trackClientCallback = (callback: () => T | PromiseLike): Promise => { + const pending = Promise.resolve().then(callback) + inFlightClientCallbacks.add(pending) + void pending.then( + () => { inFlightClientCallbacks.delete(pending) }, + () => { inFlightClientCallbacks.delete(pending) }, + ) + return pending + } + const requestPermission = options.requestPermission + ?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' as const } })) const makeClient = (_agent: AcpAgent): Client => ({ sessionUpdate(params: SessionNotification): Promise { - updates.push(params.update) - for (let index = updateWaiters.length - 1; index >= 0; index--) { - const waiter = updateWaiters[index] - /* v8 ignore next 1 -- index is bounded by the array length */ - if (waiter === undefined) continue - let matches: boolean - try { - matches = waiter.match(params.update) - } catch (error: unknown) { + return trackClientCallback(() => { + updates.push(params.update) + for (let index = updateWaiters.length - 1; index >= 0; index--) { + const waiter = updateWaiters[index] + /* v8 ignore next 1 -- index is bounded by the array length */ + if (waiter === undefined) continue + let matches: boolean + try { + matches = waiter.match(params.update) + } catch (error: unknown) { + updateWaiters.splice(index, 1) + waiter.reject(error) + continue + } + if (!matches) continue updateWaiters.splice(index, 1) - waiter.reject(error) - continue + waiter.resolve(params.update) } - if (!matches) continue - updateWaiters.splice(index, 1) - waiter.resolve(params.update) - } - return Promise.resolve() + }) }, - requestPermission: options.requestPermission - ?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' } })), + requestPermission: params => trackClientCallback(() => requestPermission(params)), }) const client = new ClientSideConnection(makeClient, stream) // `exit` only reports the parent process's status. Descendants may retain @@ -168,7 +180,14 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // `closed` follows parser exhaustion. Capture both eagerly so a caller that // invokes close after process exit still joins the complete drain boundary. const stdioClosed = new Promise(resolve => child.once('close', () => { resolve() })) - const drained = Promise.all([stdioClosed, client.closed]).then(() => undefined) + const drained = Promise.all([stdioClosed, client.closed]).then(async () => { + // The ACP SDK's readable loop dispatches client callbacks without awaiting + // them. Once `closed` settles no new callbacks can start, but callbacks + // already in flight still belong to this launch's teardown boundary. + while (inFlightClientCallbacks.size > 0) { + await Promise.allSettled([...inFlightClientCallbacks]) + } + }) // A caller may await a pending update without calling close(). Make natural // stream exhaustion terminal for those waiters too, but only after the // parser has dispatched every buffered frame. diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 051db6a0ba..902f48d2cf 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -1,4 +1,5 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { once } from 'node:events' import { tmpdir } from 'node:os' import { delimiter, join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -112,6 +113,41 @@ describe('runScenario', () => { expect(launched.stderr()).toContain('late inherited stderr') }) + it('waits for in-flight client callbacks after the ACP stream closes', { timeout: 20_000 }, async () => { + const { dir, fixtureFile } = await scenario({ permissionProbe: true }) + let releasePermission: (() => void) | undefined + const permissionReleased = new Promise((resolve) => { releasePermission = resolve }) + let markPermissionStarted: (() => void) | undefined + const permissionStarted = new Promise((resolve) => { markPermissionStarted = resolve }) + let permissionFinished = false + const launched = launchAcpTestAgent({ + agent: AGENT, + cwd: dir, + env: { DSH_SNAPSHOT_FILE: fixtureFile }, + async requestPermission() { + markPermissionStarted?.() + await permissionReleased + permissionFinished = true + return { outcome: { outcome: 'cancelled' } } + }, + }) + await launched.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + const { sessionId } = await launched.client.newSession({ cwd: dir, mcpServers: [] }) + void launched.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'go' }] }).catch(() => undefined) + await permissionStarted + + const childClosed = once(launched.child, 'close') + let closeSettled = false + const closing = launched.close('SIGKILL').then(() => { closeSettled = true }) + await childClosed + await launched.client.closed + expect(closeSettled).toBe(false) + + releasePermission?.() + await closing + expect(permissionFinished).toBe(true) + }) + it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ permissionProbe: true, From d4c96deac3802164c430335309f936f3e5ab4f1d Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:19:02 +0800 Subject: [PATCH 08/16] refactor: share ACP callback cleanup --- packages/support/acp-snapshot/src/launcher.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 30702a5888..f1271986c0 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -141,10 +141,8 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe const trackClientCallback = (callback: () => T | PromiseLike): Promise => { const pending = Promise.resolve().then(callback) inFlightClientCallbacks.add(pending) - void pending.then( - () => { inFlightClientCallbacks.delete(pending) }, - () => { inFlightClientCallbacks.delete(pending) }, - ) + const untrack = (): void => { inFlightClientCallbacks.delete(pending) } + void pending.then(untrack, untrack) return pending } const requestPermission = options.requestPermission From 6b59b6050e379de30bb8f53bbc38172f8ffe896a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:43:12 +0800 Subject: [PATCH 09/16] fix: preserve ACP scenario cleanup failures --- packages/support/acp-snapshot/src/harness.ts | 20 +++++++--- .../acp-snapshot/tests/harness.spec.ts | 38 ++++++++++++++++++- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/packages/support/acp-snapshot/src/harness.ts b/packages/support/acp-snapshot/src/harness.ts index 3db1355b0f..22b22deacd 100644 --- a/packages/support/acp-snapshot/src/harness.ts +++ b/packages/support/acp-snapshot/src/harness.ts @@ -244,9 +244,9 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise ) // Failure-safe teardown: wait for a still-running child, then attempt BOTH - // directory removals even when an earlier cleanup rejects. The main outcome - // wins over teardown noise so a step/harvest failure is never replaced; on a - // successful run, the first cleanup failure remains visible to the caller. + // directory removals even when an earlier cleanup rejects. Report every + // teardown failure alongside a scenario failure so neither orthogonal + // outcome hides the other. const cleanupResults: PromiseSettledResult[] = [] const cleanup = async (action: () => Promise): Promise => { cleanupResults.push(...await Promise.allSettled([action()])) @@ -256,10 +256,18 @@ export async function runScenario(input: InputScript, opts: RunOptions): Promise await cleanup(() => rm(cwd, { recursive: true, force: true })) await cleanup(() => rm(sessionsRoot, { recursive: true, force: true })) + const cleanupFailures = cleanupResults + .filter((result): result is PromiseRejectedResult => result.status === 'rejected') + .map(result => result.reason as unknown) + if (cleanupFailures.length > 0) { + throw new AggregateError( + outcome.status === 'rejected' ? [outcome.error, ...cleanupFailures] : cleanupFailures, + outcome.status === 'rejected' + ? 'snapshot scenario and cleanup failed' + : 'snapshot cleanup failed', + ) + } if (outcome.status === 'rejected') throw outcome.error - const cleanupFailure = cleanupResults.find((result): result is PromiseRejectedResult => result.status === 'rejected') - /* v8 ignore next 1 -- defensive OS cleanup failure after an otherwise successful real subprocess run */ - if (cleanupFailure !== undefined) throw cleanupFailure.reason return outcome.value } diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 902f48d2cf..de764cfd8a 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -3,11 +3,29 @@ import { once } from 'node:events' import { tmpdir } from 'node:os' import { delimiter, join } from 'node:path' import { fileURLToPath } from 'node:url' -import { afterAll, describe, expect, it } from 'vitest' +import { afterAll, describe, expect, it, vi } from 'vitest' import { PROTOCOL_VERSION } from '@agentclientprotocol/sdk' import { runScenario, type AgentUnderTest, type InputStep } from '../src/harness.ts' import { launchAcpTestAgent } from '../src/launcher.ts' +const fsControl = vi.hoisted(() => ({ cleanupFailure: undefined as Error | undefined })) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + async rm(...args: Parameters): Promise { + if (String(args[0]).includes('acp-snap-cwd-') && fsControl.cleanupFailure !== undefined) { + const failure = fsControl.cleanupFailure + fsControl.cleanupFailure = undefined + await actual.rm(...args) + throw failure + } + await actual.rm(...args) + }, + } +}) + /** * Unit tests for the subprocess harness, driven through the REAL spawn path * (tsx loader, temp cwd, env plumbing) against the scripted fake ACP bin in @@ -238,6 +256,24 @@ describe('runScenario', () => { )).rejects.toThrow(/expected the prompt to fail/) }) + it('reports scenario and cleanup failures together', { timeout: 20_000 }, async () => { + const { fixtureFile } = await scenario({ prompt: 'respond' }) + const cleanupFailure = new Error('cleanup failed') + fsControl.cleanupFailure = cleanupFailure + + const failure = await runScenario( + { steps: [...boot, { op: 'promptExpectError', text: 'fine' }] }, + { agent: AGENT, mode: 'replay', fixtureFile }, + ).catch((error: unknown): unknown => error) + + expect(failure).toBeInstanceOf(AggregateError) + const failures = (failure as AggregateError).errors as unknown[] + expect(failures).toHaveLength(2) + expect(failures[0]).toBeInstanceOf(Error) + expect((failures[0] as Error).message).toMatch(/expected the prompt to fail/) + expect(failures[1]).toBe(cleanupFailure) + }) + it('newSessionExpectError swallows the rejection, with and without extra dirs', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ rejectExtraDirs: true }) const result = await runScenario( From a8c0e8a03c35b1303d7780b8215e482809c32d6f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 09:55:15 +0800 Subject: [PATCH 10/16] test: cover successful ACP cleanup failure --- .../support/acp-snapshot/tests/harness.spec.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index de764cfd8a..23b4192a4d 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -274,6 +274,21 @@ describe('runScenario', () => { expect(failures[1]).toBe(cleanupFailure) }) + it('reports cleanup failure after an otherwise successful scenario', { timeout: 20_000 }, async () => { + const { fixtureFile } = await scenario({}) + const cleanupFailure = new Error('cleanup failed') + fsControl.cleanupFailure = cleanupFailure + + const failure = await runScenario( + { steps: boot }, + { agent: AGENT, mode: 'replay', fixtureFile }, + ).catch((error: unknown): unknown => error) + + expect(failure).toBeInstanceOf(AggregateError) + expect((failure as AggregateError).message).toBe('snapshot cleanup failed') + expect((failure as AggregateError).errors as unknown[]).toEqual([cleanupFailure]) + }) + it('newSessionExpectError swallows the rejection, with and without extra dirs', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ rejectExtraDirs: true }) const result = await runScenario( From 3e2ba3f5574b43dc5af5d03e146769be241f96a3 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 10:13:05 +0800 Subject: [PATCH 11/16] fix: stop ACP teardown when fallback kill fails --- packages/support/acp-snapshot/src/launcher.ts | 27 ++++++++- .../acp-snapshot/tests/harness.spec.ts | 59 +++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index f1271986c0..8d25a6f56c 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -65,7 +65,7 @@ export interface LaunchedAcpTestAgent { stderr(): string /** Resolve when a future session update matches the predicate. */ waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise - /** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, ACP parsing, and client callbacks. */ + /** Close the process and drain its streams and callbacks; rejects promptly if fallback termination is refused. */ close(signal?: NodeJS.Signals): Promise } @@ -231,8 +231,29 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // signal can leave the subprocess live. Force termination, await the // already-observed exit edge, and only then propagate the child error so // callers may safely remove cwd/session resources after close rejects. - child.kill('SIGKILL') - await exited + const fallbackError = Promise.withResolvers() + const observeFallbackError = (error: Error): void => { fallbackError.resolve(error) } + child.once('error', observeFallbackError) + if (!child.kill('SIGKILL')) { + child.off('error', observeFallbackError) + closeUpdateStream() + throw new AggregateError( + [failure, new Error('Fallback SIGKILL was not accepted by the child process')], + 'ACP test agent failed and fallback termination was refused', + ) + } + const fallbackFailure = await Promise.race([ + exited.then((): undefined => undefined), + fallbackError.promise, + ]) + child.off('error', observeFallbackError) + if (fallbackFailure !== undefined) { + closeUpdateStream() + throw new AggregateError( + [failure, fallbackFailure], + 'ACP test agent failed and fallback termination was refused', + ) + } await drained closeUpdateStream() throw failure diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 23b4192a4d..ff7c304121 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -131,6 +131,65 @@ describe('runScenario', () => { expect(launched.stderr()).toContain('late inherited stderr') }) + it('rejects promptly when fallback termination is refused', async () => { + const { dir } = await scenario({}) + const launched = launchAcpTestAgent({ agent: AGENT, cwd: dir }) + await launched.spawned + + const childFailure = Object.assign(new Error('signal refused'), { code: 'EPERM' }) + const originalKill = launched.child.kill.bind(launched.child) + const kill = vi.spyOn(launched.child, 'kill').mockReturnValue(false) + const closed = new Promise(resolve => launched.child.once('close', () => { resolve() })) + try { + launched.child.emit('error', childFailure) + const rejection = await launched.close('SIGTERM').catch((error: unknown): unknown => error) + expect(rejection).toBeInstanceOf(AggregateError) + expect(rejection).toMatchObject({ + message: 'ACP test agent failed and fallback termination was refused', + errors: [ + childFailure, + expect.objectContaining({ message: 'Fallback SIGKILL was not accepted by the child process' }), + ], + }) + expect(kill).toHaveBeenNthCalledWith(1, 'SIGTERM') + expect(kill).toHaveBeenNthCalledWith(2, 'SIGKILL') + } finally { + kill.mockRestore() + originalKill('SIGKILL') + await closed + } + }) + + it('rejects promptly when fallback termination emits an error', async () => { + const { dir } = await scenario({}) + const launched = launchAcpTestAgent({ agent: AGENT, cwd: dir }) + await launched.spawned + + const childFailure = Object.assign(new Error('signal refused'), { code: 'EPERM' }) + const fallbackFailure = Object.assign(new Error('fallback signal refused'), { code: 'EPERM' }) + const originalKill = launched.child.kill.bind(launched.child) + const kill = vi.spyOn(launched.child, 'kill').mockImplementation((signal) => { + if (signal === 'SIGKILL') queueMicrotask(() => launched.child.emit('error', fallbackFailure)) + return signal === 'SIGKILL' + }) + const closed = new Promise(resolve => launched.child.once('close', () => { resolve() })) + try { + launched.child.emit('error', childFailure) + const rejection = await launched.close('SIGTERM').catch((error: unknown): unknown => error) + expect(rejection).toBeInstanceOf(AggregateError) + expect(rejection).toMatchObject({ + message: 'ACP test agent failed and fallback termination was refused', + errors: [childFailure, fallbackFailure], + }) + expect(kill).toHaveBeenNthCalledWith(1, 'SIGTERM') + expect(kill).toHaveBeenNthCalledWith(2, 'SIGKILL') + } finally { + kill.mockRestore() + originalKill('SIGKILL') + await closed + } + }) + it('waits for in-flight client callbacks after the ACP stream closes', { timeout: 20_000 }, async () => { const { dir, fixtureFile } = await scenario({ permissionProbe: true }) let releasePermission: (() => void) | undefined From 2027c70a17661fe2b8b5aab1685cd443ac2c3b56 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 12:38:32 +0800 Subject: [PATCH 12/16] fix: drain failed ACP launches --- packages/support/acp-snapshot/src/launcher.ts | 4 +++- packages/support/acp-snapshot/tests/harness.spec.ts | 9 +++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 3f9a21d596..30ca0b42b8 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -178,13 +178,14 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // `closed` follows parser exhaustion. Capture both eagerly so a caller that // invokes close after process exit still joins the complete drain boundary. const stdioClosed = new Promise(resolve => child.once('close', () => { resolve() })) - const drained = Promise.all([stdioClosed, client.closed]).then(async () => { + const drained = Promise.allSettled([stdioClosed, client.closed]).then(async ([, clientResult]) => { // The ACP SDK's readable loop dispatches client callbacks without awaiting // them. Once `closed` settles no new callbacks can start, but callbacks // already in flight still belong to this launch's teardown boundary. while (inFlightClientCallbacks.size > 0) { await Promise.allSettled([...inFlightClientCallbacks]) } + if (clientResult.status === 'rejected') throw clientResult.reason }) // A caller may await a pending update without calling close(). Make natural // stream exhaustion terminal for those waiters too, but only after the @@ -206,6 +207,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe try { await spawned } catch (error: unknown) { + await drained.catch(() => undefined) closeUpdateStream() throw error } diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index ff7c304121..6adb0d4268 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -62,8 +62,17 @@ describe('runScenario', () => { it('surfaces an asynchronous child spawn failure through startup and close', async () => { const { dir } = await scenario({}) const launched = launchAcpTestAgent({ agent: AGENT, cwd: join(dir, 'missing') }) + let stdioClosed = false + let clientClosed = false + launched.child.once('close', () => { stdioClosed = true }) + void launched.client.closed.then( + () => { clientClosed = true }, + () => { clientClosed = true }, + ) await expect(launched.spawned).rejects.toMatchObject({ code: 'ENOENT' }) await expect(launched.close()).rejects.toMatchObject({ code: 'ENOENT' }) + expect(stdioClosed).toBe(true) + expect(clientClosed).toBe(true) }) it('centralizes ACP boot, captures, updates, fail-closed permissions, and shutdown', { timeout: 20_000 }, async () => { From bb40259083f10cb2071c024f05810c74021e4035 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 14:11:33 +0800 Subject: [PATCH 13/16] test: preserve ACP cleanup failures --- examples/acp-agent/tests/acp.e2e.ts | 18 ++++------ examples/acp-agent/tests/cleanup.e2e.ts | 38 ++++++++++++++++++++++ examples/acp-agent/tests/cleanup.ts | 23 +++++++++++++ examples/acp-agent/tests/escalation.e2e.ts | 18 ++++------ examples/acp-agent/tests/hooks.e2e.ts | 18 ++++------ 5 files changed, 82 insertions(+), 33 deletions(-) create mode 100644 examples/acp-agent/tests/cleanup.e2e.ts create mode 100644 examples/acp-agent/tests/cleanup.ts diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index a1ddbeccc6..a0bfc9d943 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, readFile } from 'node:fs/promises' +import { mkdtemp, readFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -9,6 +9,7 @@ import { type AgentUnderTest, type LaunchedAcpTestAgent, } from '@deepseek-ai/dsh-acp-snapshot' +import { cleanupAcpExampleTest } from './cleanup.ts' /** * End-to-end: boot examples/acp-agent as a real subprocess speaking ACP over @@ -31,16 +32,11 @@ let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined afterEach(async () => { - try { - await spawned?.close('SIGKILL') - } finally { - spawned = undefined - try { - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - } finally { - workdir = undefined - } - } + const ownedSpawned = spawned + const ownedWorkdir = workdir + spawned = undefined + workdir = undefined + await cleanupAcpExampleTest(ownedSpawned, ownedWorkdir) }) describe('acp-agent over real stdio (no key required)', () => { diff --git a/examples/acp-agent/tests/cleanup.e2e.ts b/examples/acp-agent/tests/cleanup.e2e.ts new file mode 100644 index 0000000000..1f6e6493e0 --- /dev/null +++ b/examples/acp-agent/tests/cleanup.e2e.ts @@ -0,0 +1,38 @@ +/** Regression coverage for ACP example teardown. */ + +import { access, mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { cleanupAcpExampleTest } from './cleanup.ts' + +let fallbackWorkdir: string | undefined + +afterEach(async () => { + if (fallbackWorkdir !== undefined) await rm(fallbackWorkdir, { recursive: true, force: true }) + fallbackWorkdir = undefined +}) + +describe('cleanupAcpExampleTest', () => { + it('removes the workspace after process shutdown fails', async () => { + fallbackWorkdir = await mkdtemp(join(tmpdir(), 'acp-cleanup-')) + const closeFailure = new Error('close failed') + const spawned = { close: vi.fn().mockRejectedValue(closeFailure) } + + await expect(cleanupAcpExampleTest(spawned, fallbackWorkdir)) + .rejects.toMatchObject({ errors: [closeFailure] }) + await expect(access(fallbackWorkdir)).rejects.toThrow() + fallbackWorkdir = undefined + }) + + it('reports process and workspace failures together', async () => { + const closeFailure = new Error('close failed') + const spawned = { close: vi.fn().mockRejectedValue(closeFailure) } + + const failure = await cleanupAcpExampleTest(spawned, '\0').catch((error: unknown) => error) + + expect(failure).toBeInstanceOf(AggregateError) + expect((failure as AggregateError).errors).toHaveLength(2) + expect((failure as AggregateError).errors[0]).toBe(closeFailure) + }) +}) diff --git a/examples/acp-agent/tests/cleanup.ts b/examples/acp-agent/tests/cleanup.ts new file mode 100644 index 0000000000..28a896334a --- /dev/null +++ b/examples/acp-agent/tests/cleanup.ts @@ -0,0 +1,23 @@ +/** Shared teardown for ACP example tests. */ + +import { rm } from 'node:fs/promises' +import type { LaunchedAcpTestAgent } from '@deepseek-ai/dsh-acp-snapshot' + +/** + * Close the test agent, then remove its workspace, attempting both operations + * and reporting every failure instead of allowing the later one to mask the + * earlier one. + */ +export async function cleanupAcpExampleTest( + spawned: Pick | undefined, + workdir: string | undefined, +): Promise { + const results: PromiseSettledResult[] = [] + if (spawned !== undefined) results.push(...await Promise.allSettled([spawned.close('SIGKILL')])) + if (workdir !== undefined) results.push(...await Promise.allSettled([rm(workdir, { recursive: true, force: true })])) + + const failures = results + .filter((result): result is PromiseRejectedResult => result.status === 'rejected') + .map(result => result.reason as unknown) + if (failures.length > 0) throw new AggregateError(failures, 'ACP example cleanup failed') +} diff --git a/examples/acp-agent/tests/escalation.e2e.ts b/examples/acp-agent/tests/escalation.e2e.ts index da8d3408c2..3b84dd721c 100644 --- a/examples/acp-agent/tests/escalation.e2e.ts +++ b/examples/acp-agent/tests/escalation.e2e.ts @@ -1,5 +1,5 @@ import { spawnSync } from 'node:child_process' -import { mkdtemp, readFile, rm } from 'node:fs/promises' +import { mkdtemp, readFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -13,6 +13,7 @@ import { type AgentUnderTest, type LaunchedAcpTestAgent, } from '@deepseek-ai/dsh-acp-snapshot' +import { cleanupAcpExampleTest } from './cleanup.ts' /** * The default ACP composition (`cordis.yml`) end to end. @@ -81,16 +82,11 @@ let spawned: Spawned | undefined let workdir: string | undefined afterEach(async () => { - try { - await spawned?.close('SIGKILL') - } finally { - spawned = undefined - try { - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - } finally { - workdir = undefined - } - } + const ownedSpawned = spawned + const ownedWorkdir = workdir + spawned = undefined + workdir = undefined + await cleanupAcpExampleTest(ownedSpawned, ownedWorkdir) }) describe('default sandbox composition keyless smoke (real cordis.yml via the Loader)', () => { diff --git a/examples/acp-agent/tests/hooks.e2e.ts b/examples/acp-agent/tests/hooks.e2e.ts index df231184c3..dbe98ab358 100644 --- a/examples/acp-agent/tests/hooks.e2e.ts +++ b/examples/acp-agent/tests/hooks.e2e.ts @@ -1,4 +1,4 @@ -import { mkdtemp, rm, writeFile, access } from 'node:fs/promises' +import { mkdtemp, writeFile, access } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { fileURLToPath } from 'node:url' @@ -9,6 +9,7 @@ import { type AgentUnderTest, type LaunchedAcpTestAgent, } from '@deepseek-ai/dsh-acp-snapshot' +import { cleanupAcpExampleTest } from './cleanup.ts' /** * With-key e2e: the Claude Code hook bridge running against the REAL acp-agent @@ -38,16 +39,11 @@ let spawned: LaunchedAcpTestAgent | undefined let workdir: string | undefined afterEach(async () => { - try { - await spawned?.close('SIGKILL') - } finally { - spawned = undefined - try { - if (workdir !== undefined) await rm(workdir, { recursive: true, force: true }) - } finally { - workdir = undefined - } - } + const ownedSpawned = spawned + const ownedWorkdir = workdir + spawned = undefined + workdir = undefined + await cleanupAcpExampleTest(ownedSpawned, ownedWorkdir) }) describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: a PreToolUse hook blocks bash (real model)', () => { From 7e9bf9b951913b1c5d70b5e94e22ccd8f60bed76 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 15:03:01 +0800 Subject: [PATCH 14/16] refactor: remove impossible ACP drain branch --- packages/support/acp-snapshot/src/launcher.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index 30ca0b42b8..c0dc21dc06 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -178,14 +178,13 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe // `closed` follows parser exhaustion. Capture both eagerly so a caller that // invokes close after process exit still joins the complete drain boundary. const stdioClosed = new Promise(resolve => child.once('close', () => { resolve() })) - const drained = Promise.allSettled([stdioClosed, client.closed]).then(async ([, clientResult]) => { + const drained = Promise.all([stdioClosed, client.closed]).then(async () => { // The ACP SDK's readable loop dispatches client callbacks without awaiting // them. Once `closed` settles no new callbacks can start, but callbacks // already in flight still belong to this launch's teardown boundary. while (inFlightClientCallbacks.size > 0) { await Promise.allSettled([...inFlightClientCallbacks]) } - if (clientResult.status === 'rejected') throw clientResult.reason }) // A caller may await a pending update without calling close(). Make natural // stream exhaustion terminal for those waiters too, but only after the @@ -207,7 +206,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe try { await spawned } catch (error: unknown) { - await drained.catch(() => undefined) + await drained closeUpdateStream() throw error } From 8e7cf8cc10c3559ded0ee7f93f62d14ff5f18ad4 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:09:18 +0800 Subject: [PATCH 15/16] Update ACP launcher example path --- packages/support/acp-snapshot/src/launcher.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/support/acp-snapshot/src/launcher.ts b/packages/support/acp-snapshot/src/launcher.ts index c0dc21dc06..95292df38a 100644 --- a/packages/support/acp-snapshot/src/launcher.ts +++ b/packages/support/acp-snapshot/src/launcher.ts @@ -27,7 +27,7 @@ const tsxLoader = fileURLToPath(import.meta.resolve('tsx')) /** The unbuilt agent entry, leaf config, and workspace tsconfig an ACP test boots. */ export interface AgentUnderTest { - /** The agent bin entry (for example `packages/ui/acp-agent/src/bin.ts`). */ + /** The agent bin entry (for example `packages/examples/acp-demo/src/bin.ts`). */ binScript: string /** The leaf `cordis.yml` loaded by the bin. */ configPath: string From e846a115f29fd4fdfe7d6fc7ac303688a093a715 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 18 Jul 2026 13:51:44 +0800 Subject: [PATCH 16/16] test(acp-snapshot): cover pre-spawn launch failure --- .../support/acp-snapshot/tests/harness.spec.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/support/acp-snapshot/tests/harness.spec.ts b/packages/support/acp-snapshot/tests/harness.spec.ts index 3436f32f96..2b9d6e032f 100644 --- a/packages/support/acp-snapshot/tests/harness.spec.ts +++ b/packages/support/acp-snapshot/tests/harness.spec.ts @@ -244,6 +244,23 @@ describe('runScenario', () => { )).rejects.toThrow(/agent stderr:\nfake agent requested startup failure/) }) + it('preserves launch-resolution errors when no child process exists', async () => { + const { dir, fixtureFile } = await scenario({}) + vi.stubEnv('DSH_EXAMPLE_MODE', 'lib') + try { + await expect(runScenario( + { steps: [] }, + { + agent: { ...AGENT, binScript: join(dir, 'outside-src.ts'), libBinScript: undefined }, + mode: 'replay', + fixtureFile, + }, + )).rejects.toThrow(/expected a "\/src\/" segment/) + } finally { + vi.unstubAllEnvs() + } + }) + it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => { const { fixtureFile } = await scenario({ permissionProbe: true,