From c432f85c075b0b083d93f6b6a1e4982bde413364 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Sun, 19 Jul 2026 15:21:33 +0800 Subject: [PATCH] fix(subagent-acp): resolve child cwd from the parent session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ACP backend resolved an unset cwd to the harness process's launch directory, so a delegated child ran — and announced its ACP session — in the server's cwd instead of the delegating session's workspace. Resolve the working directory explicitly: the config cwd override (made absolute and validated at load), else the parent session header's cwd (validated at start), failing loud before spawning when neither exists. --- docs/config-catalog.md | 9 +- packages/subagent/subagent-acp/README.md | 9 +- packages/subagent/subagent-acp/src/index.ts | 70 ++++++++- packages/subagent/subagent-acp/src/run.ts | 6 +- .../subagent-acp/tests/mock-acp-server.ts | 19 ++- .../subagent-acp/tests/subagent-acp.spec.ts | 133 +++++++++++++++++- 6 files changed, 226 insertions(+), 20 deletions(-) diff --git a/docs/config-catalog.md b/docs/config-catalog.md index f77334b442..24e48b82f0 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -860,8 +860,11 @@ export interface Config { /** Arguments passed to {@link command}. */ args: string[] /** - * Working directory for the child process and its ACP session. Defaults to - * the parent process's cwd when omitted. + * Working directory override for the child process and its ACP session. A + * relative path resolves against the harness launch directory at load, and + * the result must be an existing directory. When omitted, each child + * inherits its delegating parent session's cwd — and starting one from a + * parent session that has no cwd fails. */ cwd?: string /** @@ -891,7 +894,7 @@ export interface Config { export type PermissionPolicy = 'allow' | 'reject' ``` -Source: [`packages/subagent/subagent-acp/src/index.ts:18`](../packages/subagent/subagent-acp/src/index.ts) +Source: [`packages/subagent/subagent-acp/src/index.ts:21`](../packages/subagent/subagent-acp/src/index.ts) ## `@deepseek-ai/dsh-subagent-fork` diff --git a/packages/subagent/subagent-acp/README.md b/packages/subagent/subagent-acp/README.md index a620051506..839bfd109f 100644 --- a/packages/subagent/subagent-acp/README.md +++ b/packages/subagent/subagent-acp/README.md @@ -4,7 +4,9 @@ The ACP provider runs each subagent in a fresh subprocess and drives it as an Ag ## Start and ownership -`start(request)` performs `spawn` → ACP `initialize` → `newSession` before it fulfills. Fulfillment therefore means a remote session is ready and ownership has transferred to the caller. A spawn, initialization, new-session, or pre-publication cancellation failure rejects only after the subprocess has been reaped. +`start(request)` resolves the child's working directory, then performs `spawn` → ACP `initialize` → `newSession` before it fulfills. Fulfillment therefore means a remote session is ready and ownership has transferred to the caller. A spawn, initialization, new-session, or pre-publication cancellation failure rejects only after the subprocess has been reaped; a working-directory resolution failure rejects before anything is spawned. + +The working directory is the configured `cwd` override when set, else the delegating parent session's cwd — never the server process's own cwd, because one server process serves sessions from many workspaces. The parent-derived value must be an absolute path naming an existing directory, and the same resolved path becomes both the subprocess cwd and the ACP `session/new` workspace. The returned run id is minted in the parent namespace. The child server's session id remains private to ACP wire calls because ACP guarantees it only within that fresh child process; using it as the parent lifecycle id could collide with another remote run or a local agent. @@ -14,7 +16,7 @@ After publication, the provider sends the prompt and collects streamed `agent_me ## Capabilities and context -ACP advertises no start-time capabilities because this process cannot enforce the remote child's depth, tool filter, persona, or structured-output runtime. It also reports `inheritsParentContext: false`: the remote session starts fresh and ignores `request.parent` beyond the seam's required attribution field. +ACP advertises no start-time capabilities because this process cannot enforce the remote child's depth, tool filter, persona, or structured-output runtime. It also reports `inheritsParentContext: false`: the remote session starts fresh, and the only parent-derived input is the workspace cwd described above — no conversation context crosses the process boundary. ## Configuration @@ -23,7 +25,7 @@ ACP advertises no start-time capabilities because this process cannot enforce th | `providerName` | `acp` | Registry name on `ctx.subagents`. | | `command` | required | Executable spawned for each run. | | `args` | `[]` | Command arguments. | -| `cwd` | process cwd | Child process and ACP session working directory. | +| `cwd` | parent session cwd | Working-directory override for the child process and its ACP session; a relative value resolves against the harness launch directory at load and must name an existing directory. | | `permission` | `reject` | Auto-answer permission requests by rejecting or choosing the first allow-shaped option. | | `env` | `{}` | Explicit child environment layered over a credential-scrubbed parent environment. | | `disposeEofGraceMs` | `6000` | Grace after stdin EOF before SIGTERM. | @@ -76,6 +78,7 @@ Keyless tests drive a scripted ACP subprocess over real stdio. The with-key e2e ## Known Limitations and Deferred Work - **A fresh process per run** — persistent-process pooling is a future optimization ([the seam RFC](../../../docs/rfc/implemented/feature/2026-06-21-subagent-capability-seam.md)). +- **Local workspaces only** — the resolved cwd is a local path handed to a child on the same machine; workspace mapping for a remote ACP agent would need its own backend capability and is not designed here. - **No optional start-time capabilities** — this provider cannot apply the local harness's `outputSchema`, depth cap, tool filter, or persona inside the remote process, so it advertises none and the service rejects requests that require them. - **Only `agent_message_chunk` text is collected** — the child's tool-call activity, thought chunks, and plan updates are not surfaced to the parent. - **Permission prompts are auto-answered** (`permission: allow | reject`) — no human is surfaced a child's `session/request_permission` in this cut. diff --git a/packages/subagent/subagent-acp/src/index.ts b/packages/subagent/subagent-acp/src/index.ts index 80766ed831..3722911f28 100644 --- a/packages/subagent/subagent-acp/src/index.ts +++ b/packages/subagent/subagent-acp/src/index.ts @@ -1,11 +1,14 @@ /** * Out-of-process ACP subagent backend. Each child has its own process, session, model, and - * tools, so it shares no Cordis context, ignores `request.parent`, and advertises no parent- - * enforced start capabilities. This plugin uses named exports only; a default would hide its + * tools, so it shares no Cordis context and advertises no parent-enforced start capabilities; + * the ONE thing it reads off `request.parent` is the session's workspace cwd (see + * {@link resolveCwd}). This plugin uses named exports only; a default would hide its * loader metadata (see `docs/postmortem/0001-acp-default-export-drops-inject.md`). * @module @deepseek-ai/dsh-subagent-acp */ +import { statSync } from 'node:fs' +import { isAbsolute, resolve } from 'node:path' import type { Context } from 'cordis' import z from 'schemastery' import type { SubagentCapabilities, SubagentProvider, SubagentStartRequest } from '@deepseek-ai/dsh-subagent' @@ -23,8 +26,11 @@ export interface Config { /** Arguments passed to {@link command}. */ args: string[] /** - * Working directory for the child process and its ACP session. Defaults to - * the parent process's cwd when omitted. + * Working directory override for the child process and its ACP session. A + * relative path resolves against the harness launch directory at load, and + * the result must be an existing directory. When omitted, each child + * inherits its delegating parent session's cwd — and starting one from a + * parent session that has no cwd fails. */ cwd?: string /** @@ -71,6 +77,53 @@ function assertPositiveFinite(name: string, value: number): void { /** The shape after schemastery applied the defaults (cwd has none). */ type ResolvedConfig = Required> & Pick +/** Whether `path` names an existing, accessible directory. */ +function isDirectory(path: string): boolean { + try { + return statSync(path).isDirectory() + } catch { + // statSync throws only filesystem access errors here (ENOENT/EACCES/ENOTDIR/…), + // and every one of them means the path cannot serve as the child's cwd. + return false + } +} + +/** + * Assert `cwd` can actually host the child: absolute (it doubles as the ACP + * session workspace, and a relative path would be re-anchored to the server + * process's launch directory) and an existing directory (fail here, before the + * process boundary, instead of as an ambiguous spawn ENOENT). + * @param label - which source supplied the value, for the diagnostic. + * @param cwd - the candidate working directory. + * @returns `cwd`, validated. + */ +function assertUsableCwd(label: string, cwd: string): string { + if (!isAbsolute(cwd)) { + throw new Error(`subagent-acp: ${label} must be an absolute path: ${cwd}`) + } + if (!isDirectory(cwd)) { + throw new Error(`subagent-acp: ${label} is not an accessible directory: ${cwd}`) + } + return cwd +} + +/** + * Resolve the child's working directory: the deployment `cwd` override when + * configured (already validated at load), else the parent session's workspace + * cwd (validated here, its earliest resolvable point). Fails loud when neither + * exists — falling back to the harness process cwd would silently bind the + * child to the server's launch directory instead of the delegating session's + * workspace (one server process serves many sessions, each with its own cwd). + */ +function resolveCwd(configured: string | undefined, request: SubagentStartRequest): string { + if (configured !== undefined) return configured + const parentCwd = request.parent.session.header.cwd + if (parentCwd === undefined) { + throw new Error('subagent-acp: no working directory for the child — configure `cwd` or delegate from a parent session that has one') + } + return assertUsableCwd('parent session cwd', parentCwd) +} + /** * The ACP provider. Advertises NO start-time capabilities: an out-of-process * child cannot honor `outputSchema`/`maxDepth`/`toolFilter` (the service rejects @@ -87,7 +140,7 @@ class AcpProvider implements SubagentProvider { const spec: AcpRunSpec = { command: this.config.command, args: this.config.args, - cwd: this.config.cwd ?? process.cwd(), + cwd: resolveCwd(this.config.cwd, request), permission: this.config.permission, env: this.config.env, disposeEofGraceMs: this.config.disposeEofGraceMs, @@ -107,5 +160,10 @@ export function apply(ctx: Context, config: Config): void { const resolved = config as ResolvedConfig assertPositiveFinite('disposeEofGraceMs', resolved.disposeEofGraceMs) assertPositiveFinite('disposeGraceMs', resolved.disposeGraceMs) - ctx.subagents.registerProvider(new AcpProvider(resolved.providerName, ctx, resolved)) + // Interpret a relative configured cwd against the harness launch directory + // ONCE, at load, and fail a misconfigured directory here — not per start. + const validated: ResolvedConfig = resolved.cwd === undefined + ? resolved + : { ...resolved, cwd: assertUsableCwd('config cwd', resolve(resolved.cwd)) } + ctx.subagents.registerProvider(new AcpProvider(validated.providerName, ctx, validated)) } diff --git a/packages/subagent/subagent-acp/src/run.ts b/packages/subagent/subagent-acp/src/run.ts index a10a87a940..82e7f1e08e 100644 --- a/packages/subagent/subagent-acp/src/run.ts +++ b/packages/subagent/subagent-acp/src/run.ts @@ -37,7 +37,11 @@ export interface AcpRunSpec { command: string /** Arguments passed to {@link command}. */ args: string[] - /** Working directory for the child process AND its ACP session `cwd`. */ + /** + * Absolute working directory for the child process AND its ACP session + * `cwd`. The provider resolves it before this spec exists: config override, + * else the delegating parent session's workspace. + */ cwd: string /** How to auto-answer the child's permission prompts. */ permission: PermissionPolicy diff --git a/packages/subagent/subagent-acp/tests/mock-acp-server.ts b/packages/subagent/subagent-acp/tests/mock-acp-server.ts index 2bbf457c18..6b3f8157e8 100644 --- a/packages/subagent/subagent-acp/tests/mock-acp-server.ts +++ b/packages/subagent/subagent-acp/tests/mock-acp-server.ts @@ -15,6 +15,11 @@ * `dispose()` must still kill the process. * - `MOCK_PERMISSION` — if `1`, the agent calls `session/request_permission` * before answering, to exercise the client's auto-answer. + * - `MOCK_ECHO_CWD` — if `1`, ignore MOCK_TEXT and stream two lines instead: + * the agent PROCESS's `process.cwd()` and the `cwd` the + * client announced in `session/new` — so a test can assert + * where the child actually ran and what workspace it was + * told it has. * - `MOCK_READY_FILE` — if set, the path the agent touches once its `prompt` * handler is in flight (it has streamed its chunk). A test * polls for this file to cancel on a CONDITION rather than @@ -63,6 +68,7 @@ import { } from '@agentclientprotocol/sdk' const TEXT = process.env.MOCK_TEXT ?? 'mock child answer' +const ECHO_CWD = process.env.MOCK_ECHO_CWD === '1' const STOP = (process.env.MOCK_STOP ?? 'end_turn') as StopReason const HANG = process.env.MOCK_HANG === '1' const WANT_PERMISSION = process.env.MOCK_PERMISSION === '1' @@ -83,6 +89,8 @@ function makeAgent(conn: AgentSideConnection): Agent { // Pending cancel resolver for the HANG path: a `session/cancel` resolves the // prompt with `cancelled`. let resolveCancel: ((reason: StopReason) => void) | undefined + // The cwd the client announced in `session/new`, echoed under MOCK_ECHO_CWD. + let sessionCwd: string | undefined return { initialize(_params: InitializeRequest): Promise { @@ -92,7 +100,8 @@ function makeAgent(conn: AgentSideConnection): Agent { authMethods: [], }) }, - async newSession(_params: NewSessionRequest): Promise { + async newSession(params: NewSessionRequest): Promise { + sessionCwd = params.cwd // Optionally signal "newSession reached" and block until released, so a // test can cancel DURING newSession (the early-cancel race window) on a // condition rather than a timeout. @@ -136,10 +145,14 @@ function makeAgent(conn: AgentSideConnection): Agent { update: { sessionUpdate: 'agent_thought_chunk', content: { type: 'text', text: 'thinking…' } }, }) } - // Stream the canned assistant text as one chunk. + // Stream the canned assistant text as one chunk (or, under MOCK_ECHO_CWD, + // the observable process cwd + announced session cwd). await conn.sessionUpdate({ sessionId: params.sessionId, - update: { sessionUpdate: 'agent_message_chunk', content: { type: 'text', text: TEXT } }, + update: { + sessionUpdate: 'agent_message_chunk', + content: { type: 'text', text: ECHO_CWD ? `${process.cwd()}\n${sessionCwd ?? ''}` : TEXT }, + }, }) // Signal "prompt is in flight" by touching the readiness file, so a test // can wait on a CONDITION (file exists) rather than an arbitrary timeout diff --git a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts index e4231c1598..9e32e3acf3 100644 --- a/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts +++ b/packages/subagent/subagent-acp/tests/subagent-acp.spec.ts @@ -1,9 +1,9 @@ import { describe, expect, it } from 'vitest' import { Context } from 'cordis' import Loader from '@cordisjs/plugin-loader' -import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import SubagentService from '@deepseek-ai/dsh-subagent' import { buildChildEnv } from '@deepseek-ai/dsh-subagent-subprocess' @@ -22,8 +22,8 @@ import { acpStopReason, acpContentText, DEFAULT_DISPOSE_EOF_GRACE_MS, DEFAULT_DI const mockServer = fileURLToPath(new URL('./mock-acp-server.ts', import.meta.url)) -/** A throwaway parent Agent — the ACP backend ignores it, but the seam requires one. */ -const fakeParent = { id: 'parent', session: { header: {} } } as unknown as Agent +/** A parent Agent stub. The ACP backend reads exactly one thing off it: the session header's cwd (the workspace its child inherits). */ +const fakeParent = { id: 'parent', session: { header: { cwd: process.cwd() } } } as unknown as Agent function request(text = 'p', signal = new AbortController().signal) { return { prompt: [{ type: 'text' as const, text }], parent: fakeParent, signal } @@ -115,6 +115,131 @@ describe('buildChildEnv', () => { }) }) +describe('cwd resolution', () => { + it('falls back to the parent session cwd for the child process AND its ACP session', async () => { + // realpath: on macOS `tmpdir()` sits behind a symlink (/var → /private/var), + // and the child reports its REAL process.cwd() — compare canonical paths. + const workdir = realpathSync(mkdtempSync(join(tmpdir(), 'acp-parent-cwd-'))) + try { + const ctx = await setup({ MOCK_ECHO_CWD: '1' }) + const parent = { id: 'parent', session: { header: { cwd: workdir } } } as unknown as Agent + const run = await ctx.subagents.start('acp', { prompt: [{ type: 'text' as const, text: 'p' }], parent, signal: new AbortController().signal }) + const result = await run.result + await run.dispose() + // Line 1: where the child process actually ran; line 2: the workspace the + // backend announced in `session/new`. Both must be the parent's workspace. + expect(text(result.output)).toBe(`${workdir}\n${workdir}`) + } finally { + rmSync(workdir, { recursive: true, force: true }) + } + }) + + it('rejects before spawning when neither config.cwd nor the parent session provides one', async () => { + const tmp = mkdtempSync(join(tmpdir(), 'acp-no-cwd-')) + const sentinel = join(tmp, 'spawned') + try { + const ctx = new Context() + await ctx.plugin(SubagentService) + // A command that would create the sentinel if the child were ever spawned. + await ctx.plugin(acp, { providerName: 'acp', command: 'touch', args: [sentinel], permission: 'reject', env: {} }) + const parent = { id: 'parent', session: { header: {} } } as unknown as Agent + await expect(ctx.subagents.start('acp', { prompt: [{ type: 'text' as const, text: 'p' }], parent, signal: new AbortController().signal })) + .rejects.toThrow('no working directory') + // Resolution failed BEFORE the process boundary — nothing was launched. + expect(existsSync(sentinel)).toBe(false) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) + + it('prefers the configured cwd override to the parent session cwd', async () => { + const configured = realpathSync(mkdtempSync(join(tmpdir(), 'acp-cfg-cwd-'))) + const parentDir = realpathSync(mkdtempSync(join(tmpdir(), 'acp-parent-cwd-'))) + try { + const ctx = new Context() + await ctx.plugin(SubagentService) + await ctx.plugin(acp, { + providerName: 'acp', + command: process.execPath, + args: [mockServer], + cwd: configured, + permission: 'reject', + env: { MOCK_ECHO_CWD: '1' }, + }) + const parent = { id: 'parent', session: { header: { cwd: parentDir } } } as unknown as Agent + const run = await ctx.subagents.start('acp', { prompt: [{ type: 'text' as const, text: 'p' }], parent, signal: new AbortController().signal }) + const result = await run.result + await run.dispose() + expect(text(result.output)).toBe(`${configured}\n${configured}`) + } finally { + rmSync(configured, { recursive: true, force: true }) + rmSync(parentDir, { recursive: true, force: true }) + } + }) + + it('resolves a relative config cwd against the launch directory at load', async () => { + // The child process AND its announced ACP session cwd must both get the + // ABSOLUTE form — DSH's own ACP server rejects a relative session cwd, and + // deferring resolution to spawn would hide the launch-dir dependency. + const relative = 'packages/subagent/subagent-acp' + const absolute = resolve(relative) + const ctx = new Context() + await ctx.plugin(SubagentService) + await ctx.plugin(acp, { + providerName: 'acp', + command: process.execPath, + args: [mockServer], + cwd: relative, + permission: 'reject', + env: { MOCK_ECHO_CWD: '1' }, + }) + const run = await ctx.subagents.start('acp', request()) + const result = await run.result + await run.dispose() + expect(text(result.output)).toBe(`${realpathSync(absolute)}\n${absolute}`) + }) + + it('rejects a config cwd that is not an accessible directory at load', async () => { + const ctx = new Context() + await ctx.plugin(SubagentService) + await expect(ctx.plugin(acp, { + providerName: 'acp', + command: 'true', + args: [], + cwd: '/nonexistent/acp-child-workspace', + permission: 'reject', + env: {}, + })).rejects.toThrow('not an accessible directory') + await ctx.fiber.dispose() + }) + + it('rejects a parent session cwd that is not absolute', async () => { + // SessionHeader documents cwd as absolute; a relative value here is a broken + // header, and resolving it against the server process cwd would silently + // re-introduce the launch-directory dependency this resolution removes. + const ctx = await setup({}) + const parent = { id: 'parent', session: { header: { cwd: 'relative/workspace' } } } as unknown as Agent + await expect(ctx.subagents.start('acp', { prompt: [{ type: 'text' as const, text: 'p' }], parent, signal: new AbortController().signal })) + .rejects.toThrow('must be an absolute path') + }) + + it('rejects a parent session cwd that is not an accessible directory, before spawning', async () => { + const tmp = mkdtempSync(join(tmpdir(), 'acp-bad-parent-cwd-')) + const sentinel = join(tmp, 'spawned') + try { + const ctx = new Context() + await ctx.plugin(SubagentService) + await ctx.plugin(acp, { providerName: 'acp', command: 'touch', args: [sentinel], permission: 'reject', env: {} }) + const parent = { id: 'parent', session: { header: { cwd: join(tmp, 'vanished') } } } as unknown as Agent + await expect(ctx.subagents.start('acp', { prompt: [{ type: 'text' as const, text: 'p' }], parent, signal: new AbortController().signal })) + .rejects.toThrow('not an accessible directory') + expect(existsSync(sentinel)).toBe(false) + } finally { + rmSync(tmp, { recursive: true, force: true }) + } + }) +}) + describe('dsh-subagent-acp', () => { it('drives child processes with parent-unique run ids and returns streamed output', async () => { const ctx = await setup({ MOCK_TEXT: 'hello from acp child', MOCK_STOP: 'end_turn', MOCK_SESSION_ID: 'acp-child-session' })