From f3906af225c291e1164cc2a9583f2d9fb15c06dd Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 17 Jun 2026 10:01:18 +0800 Subject: [PATCH] =?UTF-8?q?feat(acp):=20honor=20per-session=20cwd=20?= =?UTF-8?q?=E2=80=94=20run=20each=20ACP=20session=20in=20its=20own=20works?= =?UTF-8?q?pace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lifts the RFC 010 § Deferred restriction that the server had to launch in the workspace ("cwd must equal the launch directory"). An editor can now open any project folder, and N concurrent sessions over one connection can each target a different directory. - packages/acp: drop the `cwd === process.cwd()` guard in validateWorkspaceParams (keep "must be absolute" — the cwd becomes the session header / bash workdir), and drop the persisted-cwd-vs-launch-dir check in session/load (a resumed session keeps its original header.cwd, so its bash tools run in its workspace). - packages/tool-bash: the missing link — default the bash workdir to the calling agent's session cwd (`exec.agent.session.header.cwd`) via a new resolveWorkdir helper. An explicit model `workdir` still wins; a relative one resolves against the session cwd. This is the only correct spot for multi-session: N sessions share one ctx.bash executor, so the workdir must come per-call from exec.agent, not executor config. Falls back to the executor default when no session cwd is available (preserves non-ACP behavior). - Trust: the cwd originates from the ACP client (the user's editor) at session/new — same trust level as the old launch dir; no new untrusted-input path. `additionalDirectories` (scope widening / sandbox) stays rejected. - Tests: bridge accepts any absolute cwd + records it on the header; session/load honors the persisted cwd; bash defaults to / resolves relative against the session cwd; two sessions with different cwds each run bash in their own dir; non-absolute cwd still rejected. 100% per-file coverage maintained. - Docs: RFC 010 status + § Deferred cwd bullet marked RESOLVED; acp README adds a Per-session cwd section; tool-bash + example READMEs and e2e comments updated. --- docs/rfc/010-acp-agent-client-protocol.md | 6 +-- examples/acp-agent/README.md | 4 +- examples/acp-agent/tests/acp.e2e.ts | 10 ++-- packages/acp/README.md | 10 ++-- packages/acp/src/index.ts | 58 +++++++++++------------ packages/acp/tests/bridge.spec.ts | 12 +++-- packages/acp/tests/load.spec.ts | 48 +++++++++++++------ packages/tool-bash/README.md | 4 +- packages/tool-bash/src/index.ts | 29 +++++++++++- packages/tool-bash/tests/tools.spec.ts | 50 ++++++++++++++++++- 10 files changed, 165 insertions(+), 66 deletions(-) diff --git a/docs/rfc/010-acp-agent-client-protocol.md b/docs/rfc/010-acp-agent-client-protocol.md index 2e1233d924..47f59792de 100644 --- a/docs/rfc/010-acp-agent-client-protocol.md +++ b/docs/rfc/010-acp-agent-client-protocol.md @@ -2,7 +2,7 @@ Status: proposed -> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. One further best-effort limitation is tracked as `TODO(rfc010-cancel-prestep)`: `session/cancel` aborts a running step and settles the RPC as `cancelled`, but a turn still queued (not yet started) when the cancel arrives may execute before the abort takes effect, pending a loop-level pre-step cancel. +> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. One further best-effort limitation is tracked as `TODO(rfc010-cancel-prestep)`: `session/cancel` aborts a running step and settles the RPC as `cancelled`, but a turn still queued (not yet started) when the cancel arrives may execute before the abort takes effect, pending a loop-level pre-step cancel. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): any absolute `cwd` is accepted and routed to the bash workdir via `session.header.cwd`, so an editor can open any project folder and N sessions can each target a different directory. ## Problem @@ -23,7 +23,7 @@ The mapping between ACP and existing harness seams — each row names the seam a | ACP (client ⇄ agent) | Harness seam | Notes | |---|---|---| | `initialize` | static handler | negotiate `protocolVersion` (echo the supported version, else error); advertise text-only `promptCapabilities` and `loadSession: true`; report agent name/version | -| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `AgentLoop.create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see RFC 011); `cwd` validated (require absolute) with "launch the server in the workspace root" documented until the workdir seam exists; `mcpServers` ignored (no `mcpCapabilities` advertised); non-empty `additionalDirectories` rejected for the MVP (the bridge cannot yet widen bash/tool filesystem scope, so silently ignoring them would desync the client's filesystem-scope UI) | +| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `AgentLoop.create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see RFC 011); `cwd` validated (require absolute) — any absolute cwd is honored: it becomes the session's `SessionHeader.cwd` and the default bash workdir (per-session cwd, see § Deferred → RESOLVED), so the server need not launch in the workspace; `mcpServers` ignored (no `mcpCapabilities` advertised); non-empty `additionalDirectories` rejected for the MVP (the bridge cannot yet widen bash/tool filesystem scope, so silently ignoring them would desync the client's filesystem-scope UI) | | `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | the `dsh-agent` resume factory (RFC 009 + Dependency note) | load `{ meta, events }`, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `additionalDirectories` rejected as in `session/new` | | `session/prompt {prompt}` | `agent.send()` (idle) | text blocks → `TextBlock`; reject image/audio per advertised capabilities; one in-flight prompt per session | | resolve `session/prompt` → `{stopReason}` | `agent/turn-end` (extended, see Plan) | map the harness kebab `TurnEndReason` to the ACP snake_case `StopReason` wire enum: `completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`(cancel)→`cancelled`, plus `refusal`/`max_turn_requests` when applicable; honor the batch-into-one-turn and send-not-synchronously-running settle semantics | @@ -54,7 +54,7 @@ Lifecycle and disposal: the connection, listeners, and in-flight permission prom Deferred (each names its owning future work): - Multiplexing concurrent sessions → RFC 011. -- `cwd` honoring. There is no current path from `session/new.cwd` to the bash workdir (`AgentLoop.create` takes only `AgentOptions`; `tool-bash` forwards only an explicit `args.workdir`; `LocalBashExecutor.resolve` defaults to its own config or `process.cwd()`). The MVP validates `cwd` (require absolute) and requires the server to be launched in the workspace root, erroring on a mismatch rather than silently running tools in the wrong directory; honoring an arbitrary `cwd` later means extending the agent-creation seam to carry a workdir. +- ~~`cwd` honoring.~~ **RESOLVED.** Originally there was no path from `session/new.cwd` to the bash workdir (`tool-bash` forwarded only an explicit `args.workdir`; `LocalBashExecutor.resolve` defaulted to its own config or `process.cwd()`), so the MVP validated `cwd` (require absolute) AND required the server to launch in the workspace root, erroring on a mismatch. This is now lifted: the validated `cwd` is stored as `SessionHeader.cwd`, and `dsh-tool-bash` defaults the bash workdir to the calling agent's `session.header.cwd` (an explicit model `workdir` still wins; a relative one resolves against it). Any absolute `cwd` is honored — the server need not launch in the workspace, and N sessions can each target a different directory. Widening scope beyond the single cwd (`additionalDirectories`) remains deferred. - Client `terminal/*` proxying (a live editor terminal) and `fs/*` (editor-rendered diffs) — a future `BashExecutor` over the [ADR 0009](../adr/0009-capability-seams.md) bash seam, gated on `clientCapabilities.terminal`. - Image/audio prompts (blocked on the DeepSeek adapter, which skips `image` blocks today), modes, auth, `available_commands`/slash-commands, `plan`, and `usage_update`. diff --git a/examples/acp-agent/README.md b/examples/acp-agent/README.md index 0aca353204..846e49fe78 100644 --- a/examples/acp-agent/README.md +++ b/examples/acp-agent/README.md @@ -28,8 +28,8 @@ Add to your Zed `settings.json` under `agent_servers`: } ``` -Run from the repo root (the MVP requires the server's launch directory to be the workspace — see the `cwd` note in `packages/acp`). +The editor sets each session's `cwd` to the project it opens; the agent's bash tools run there (see the per-session `cwd` note in `packages/acp`), so the server does not need to be launched in the workspace. ## MVP limitations -The bridge supports N concurrent sessions per connection (RFC 011). Remaining limits: text-only prompts, `cwd` must equal the launch directory, and the tool-permission gate is deferred (`TODO(rfc010-permission-gate)` — tools run with the executor's full authority). See `packages/acp/README.md` for the full contract. +The bridge supports N concurrent sessions per connection, each in its own workspace `cwd` (RFC 011). Remaining limits: text-only prompts, `additionalDirectories` rejected (a session operates in its single `cwd`), and the tool-permission gate is deferred (`TODO(rfc010-permission-gate)` — tools run with the executor's full authority). See `packages/acp/README.md` for the full contract. diff --git a/examples/acp-agent/tests/acp.e2e.ts b/examples/acp-agent/tests/acp.e2e.ts index 0c74393e24..e69a4bfc0f 100644 --- a/examples/acp-agent/tests/acp.e2e.ts +++ b/examples/acp-agent/tests/acp.e2e.ts @@ -28,9 +28,10 @@ import { const startScript = fileURLToPath(new URL('../start.ts', import.meta.url)) // Resolve tsx's loader to an ABSOLUTE path: the subprocess runs with cwd set to -// a temp workdir (the MVP requires session cwd === process.cwd()), where a bare -// `--import tsx` would not resolve from node_modules. import.meta.resolve gives -// the worktree's tsx regardless of the child's cwd. +// 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')) interface Spawned { @@ -123,7 +124,8 @@ describe.skipIf(!process.env.DEEPSEEK_API_KEY)('acp-agent e2e: real prompt over const { client, updates } = spawned await client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) - // The MVP requires cwd === the server's launch dir (its cwd is `workdir`). + // Any absolute cwd is honored now; use the temp `workdir` as this session's + // workspace (the bash tool will run there) — it need not equal the launch dir. const { sessionId } = await client.newSession({ cwd: workdir, mcpServers: [] }) const res = await client.prompt({ diff --git a/packages/acp/README.md b/packages/acp/README.md index 74cb65a44e..3d2fd15feb 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -24,8 +24,8 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` | ACP method | Harness seam | Notes | |---|---|---| | `initialize` | static | negotiate `protocolVersion`; advertise text-only `promptCapabilities` and `loadSession: true` | -| `session/new` | `ctx.agents.create({ sessionId, meta:{cwd} })` | creates a new session/agent; N concurrent sessions are allowed (RFC 011), keyed by id; `cwd` must be absolute AND equal the server launch dir; `additionalDirectories` rejected; `mcpServers` ignored | -| `session/load` | `ctx.agents.resume(...)` | replays the persisted event log to the client as `session/update` — the USER side (`user/message` → `user_message_chunk`), assistant text/reasoning (`assistant/chunk`), and tool calls/results (`tool/call` + `tool/result`). Re-loading an already-live id is rejected; the id's load slot is reserved (`loadingIds`) BEFORE the async resume so a pipelined load of the SAME id can't leak a second agent (distinct ids load concurrently); the PERSISTED header `cwd` is validated via a metadata-only `list()` BEFORE resume (not just the requested `cwd`), so a mismatch rejects without ever constructing an agent. After the async resume a `closed` re-check refuses to install a record if the bridge tore down mid-load | +| `session/new` | `ctx.agents.create({ sessionId, meta:{cwd} })` | creates a new session/agent; N concurrent sessions are allowed (RFC 011), keyed by id; `cwd` must be absolute (it becomes the session's workspace — see Per-session cwd); `additionalDirectories` rejected; `mcpServers` ignored | +| `session/load` | `ctx.agents.resume(...)` | replays the persisted event log to the client as `session/update` — the USER side (`user/message` → `user_message_chunk`), assistant text/reasoning (`assistant/chunk`), and tool calls/results (`tool/call` + `tool/result`). Re-loading an already-live id is rejected; the id's load slot is reserved (`loadingIds`) BEFORE the async resume so a pipelined load of the SAME id can't leak a second agent (distinct ids load concurrently). The resumed session keeps its PERSISTED header `cwd`, so its bash tools run in the original workspace; the requested `cwd` only needs to be absolute. After the async resume a `closed` re-check refuses to install a record if the bridge tore down mid-load | | `session/prompt` | `agent.send()` | text-only; rejects image/audio and empty prompts; one in-flight prompt PER session (independent); settles on the OWNING turn's end (a turn that ends in `error` rejects the RPC) | | `session/cancel` | `agent.abort()` | aborts a running step + settles the prompt `cancelled` for ONLY that session — a cancel never touches another session's stream or prompt (see limitation below) | | `session/update` | `session/event` | `agent_message_chunk` (text-delta), `agent_thought_chunk` (reasoning-delta), `user_message_chunk` (load replay), `tool_call`/`tool_call_update` | @@ -36,6 +36,10 @@ The bridge multiplexes N sessions over one connection. Live sessions are held in Background-task isolation rides on `dsh-tool-bash`: bash task ids are global and predictable, so the tool layer records each background task's owning agent and `bash_output`/`bash_kill` reject a task owned by a different agent — one session's agent can't read or kill another's task. +## Per-session cwd + +Each session runs in its own workspace, recorded as the session's `SessionHeader.cwd`. On `session/new` the (absolute) request `cwd` becomes that header cwd; on `session/load` the resumed session keeps its PERSISTED header cwd (the request `cwd` is only shape-checked — it does not override the stored one), and a load whose persisted session has no absolute cwd is REJECTED up front via a metadata-only `list()` check, BEFORE resume constructs an agent (else bash would silently fall back to the server's launch dir, and a post-resume reject would leak the registered agent). `dsh-tool-bash` then defaults the bash workdir to the calling agent's `session.header.cwd` (an explicit model `workdir` still wins; a relative one resolves against the session cwd; with no session cwd the executor falls back to its own config / `process.cwd()`). So the server no longer has to be launched in the workspace — an editor can open any project folder, and N sessions over one connection can each target a different directory. (`additionalDirectories` is still rejected: widening the tool/filesystem scope beyond the single cwd is a separate sandbox concern.) + ## Settle-exactly-once A `session/prompt` resolves (or rejects) exactly once, keyed off the canonical session log (the `session/event` stream), NOT the `agent/turn-start`/`agent/turn-end` events. One listener captures the prompt's owning turn from the log's `turn/start` and settles on the matching `turn/end` — the one signal that always fires (`closeTurn` appends it unconditionally, even when a boundary emit throws and the `agent/turn-end` EVENT is skipped). A prompt settles only on ITS OWN turn (`inflight.turn === turn/end.turn`), so a stale `turn/end` for a previously-cancelled turn whose end arrives late can never settle the wrong prompt. A turn that ends `error` REJECTS the RPC with an internal error carrying the failure message (ACP has no error stop reason); every other reason resolves via the codec. As a fallback, when the agent settles to `idle`/`disposed` with a prompt still pending — e.g. a `session/event` listener registered before the bridge threw and starved the bridge's listener — an `agent/status` handler reconciles the prompt from the log (the owning turn's `turn/end`, or `cancelled` if the turn was torn down without one). An empty/whitespace prompt is rejected up front — it would queue no work, so no turn would start and the RPC would hang. @@ -49,7 +53,7 @@ Teardown reaches quiescence: for EVERY live session settle any pending prompt as - **`TODO(rfc010-permission-gate)`** — the `tools/execute` permission gate (`session/request_permission`) is NOT implemented; tools run with the executor's full authority. The `agent→sessionId` reverse map is in place so the gate can route a permission request (which receives only `exec.agent`) back to its originating session. RFC 010/011 stay `proposed` until the gate (and per-session permission ownership) land. - **`TODO(rfc010-cancel-prestep)`** — `session/cancel` (and teardown/disconnect) is honest RPC/UI cancellation plus best-effort abort: a *running* step is aborted, but a turn that is queued-but-not-yet-started (the gap before `agent.abort()` has an `AbortController` to signal) may still run to completion. This same window means disposal/disconnect can return while one short queued turn per session still runs, and a prompt accepted right after a pre-step cancel can be batched into the cancelled turn (the loop merges queued messages into one turn). A loop-level queue-aware cancel will close this; the single-in-flight-per-session rule bounds the worst case to one extra prompt per session. - **`TODO(rfc010-agent-disposal)`** — the factory (`ctx.agents.create`/`resume`) returns no per-agent disposer, so teardown aborts+drains each agent but cannot individually unregister it; on a bare client disconnect (no host dispose) the idled agents linger in `ctx.agents` until the host context disposes. A reconnect spins up a fresh context, so this strands no work; a per-agent disposal seam is the follow-up. -- **`cwd`** — only the server's launch directory is honored; a `session/new.cwd` (or a persisted `session/load` header cwd) that differs is rejected (RFC 010 § Deferred — no path from session cwd to the bash workdir yet). +- **`additionalDirectories`** — rejected. A session operates in its single `cwd` (see Per-session cwd); widening the tool/filesystem scope to extra roots is a separate sandbox concern, not yet implemented. ## stdout is the protocol diff --git a/packages/acp/src/index.ts b/packages/acp/src/index.ts index 71240d97c5..03e0d17a5e 100644 --- a/packages/acp/src/index.ts +++ b/packages/acp/src/index.ts @@ -402,17 +402,21 @@ export function apply(ctx: Context, config: AcpConfig): void { loadingIds.add(params.sessionId) try { // Validate the PERSISTED cwd BEFORE resuming — `list()` is a - // metadata-only read (no full-log parse) — so a mismatch rejects - // without ever constructing/registering a live agent (which would - // then leak in `ctx.agents`/`ctx.sessions` with no disposer here). - // A session persisted in workspace A must not be loaded by a server - // launched in workspace B: it would replay A's history while tools run - // in B. (If the id is unknown to `list()`, fall through to resume, - // which rejects with the backend's not-found error.) + // metadata-only read (no full-log parse), so this rejects a session we + // can't honor WITHOUT ever constructing/registering an agent (a + // post-resume reject would leak the registered agent — abort() does not + // unregister it — and wedge the id against re-load). The session's bash + // workdir is derived from its persisted `header.cwd` and the request + // `cwd` does NOT override it (resume takes no cwd), so a session with no + // absolute persisted cwd would silently run bash in the SERVER's launch + // dir, not the client's workspace. A session created by this bridge + // always has a cwd (session/new requires it); reject the rest loudly. + // (An id unknown to `list()` falls through to resume, which rejects with + // the backend's not-found error.) const meta = (await ctx.sessionPersistence.list()).find(m => m.id === params.sessionId) - if (meta?.cwd !== undefined && meta.cwd !== process.cwd()) { + if (meta !== undefined && (meta.cwd === undefined || !isAbsolute(meta.cwd))) { throw invalidParams( - `session was created in ${meta.cwd}, but the server's launch directory is ${process.cwd()}; honoring a different cwd is not yet supported — launch the server in the session's workspace`, + `session ${params.sessionId} has no absolute persisted cwd; cannot determine its workspace (it predates per-session cwd, or was created without one)`, ) } const agent = await ctx.agents.resume({ @@ -600,33 +604,27 @@ export function agentOptions(config: AcpConfig): { model?: string; systemPrompt? } /** - * Validate `session/new` params per the MVP contract: `cwd` absolute AND equal - * to the server's launch directory (there is no path from session cwd to the - * bash workdir yet — RFC 010 § Deferred — so the server must be launched in the - * workspace root, and we error loudly rather than silently run tools in the - * wrong directory); `additionalDirectories` empty (we cannot widen filesystem - * scope yet, and silently ignoring them would desync the client's scope UI). - */ -/** - * Validate the MVP `cwd`/`additionalDirectories` contract shared by - * `session/new` and `session/load`: `cwd` must be absolute AND equal the - * server's launch directory (there is no path from session cwd to the bash - * workdir yet — RFC 010 § Deferred — so the server must be launched in the - * workspace root, and we error loudly rather than silently run tools in the - * wrong directory); `additionalDirectories` must be empty (we cannot widen - * filesystem scope yet, and silently ignoring it would desync the client's - * scope UI). Both request shapes carry `cwd: string` and + * Validate the `cwd`/`additionalDirectories` contract shared by `session/new` + * and `session/load`: `cwd` must be absolute (a relative path would be ambiguous + * as a workspace root). What the cwd is USED for differs by method, and this + * validator only enforces shape: + * - `session/new`: the validated `cwd` becomes the session's `SessionHeader.cwd` + * (via `agents.create({meta:{cwd}})`) and thus the default bash workdir. + * - `session/load`: the request `cwd` is shape-checked only; the RESUMED + * session keeps its PERSISTED `header.cwd`, which stays authoritative for the + * bash workdir — the request cwd does not override it. + * Any absolute path is accepted (the per-session cwd flows to the bash executor + * — see `dsh-tool-bash`), so the server no longer has to launch in the + * workspace. `additionalDirectories` must still be empty: widening the + * tool/filesystem scope beyond the single cwd is a separate, unimplemented + * concern (a sandbox seam), and silently ignoring extra roots would desync the + * client's filesystem-scope UI. Both request shapes carry `cwd: string` and * `additionalDirectories?: string[]`, so one validator covers both. */ function validateWorkspaceParams(params: { cwd: string; additionalDirectories?: string[] }): void { if (!isAbsolute(params.cwd)) { throw invalidParams(`cwd must be an absolute path: ${params.cwd}`) } - if (params.cwd !== process.cwd()) { - throw invalidParams( - `cwd must equal the server's launch directory (${process.cwd()}); honoring an arbitrary cwd is not yet supported — launch the server in the workspace root`, - ) - } if (params.additionalDirectories !== undefined && params.additionalDirectories.length > 0) { throw invalidParams('additionalDirectories is not supported in this MVP') } diff --git a/packages/acp/tests/bridge.spec.ts b/packages/acp/tests/bridge.spec.ts index 9ff75c0ec5..d537f4f684 100644 --- a/packages/acp/tests/bridge.spec.ts +++ b/packages/acp/tests/bridge.spec.ts @@ -65,13 +65,19 @@ describe('acp bridge', () => { expect(harness.ctx.agents.get(b.sessionId)).toBeDefined() }) - it('rejects a non-absolute cwd and a cwd that differs from the launch dir', async () => { + it('rejects a non-absolute cwd but accepts any absolute cwd (per-session workspace)', async () => { harness = await makeBridgeHarness({ storageDir }) await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + // Relative cwd is still rejected (it becomes the session header / bash workdir). await expect(harness.client.newSession({ cwd: 'relative/path', mcpServers: [] })) .rejects.toThrow(/absolute/) - await expect(harness.client.newSession({ cwd: '/some/other/dir', mcpServers: [] })) - .rejects.toThrow(/launch directory/) + // An absolute cwd that differs from the server launch dir is now ACCEPTED — + // the per-session cwd is honored (routed to the bash workdir), so the server + // no longer has to launch in the workspace. + const res = await harness.client.newSession({ cwd: '/tmp', mcpServers: [] }) + expect(res.sessionId).toBeTruthy() + // The session header records that cwd, so its bash tools run there. + expect(harness.ctx.agents.get(res.sessionId)!.session.header.cwd).toBe('/tmp') }) it('rejects non-empty additionalDirectories', async () => { diff --git a/packages/acp/tests/load.spec.ts b/packages/acp/tests/load.spec.ts index e78b64c710..3bed1f836d 100644 --- a/packages/acp/tests/load.spec.ts +++ b/packages/acp/tests/load.spec.ts @@ -85,11 +85,11 @@ describe('acp bridge — session/load replay', () => { expect(loader.ctx.agents.get(sessionId)).toBeUndefined() }) - it('rejects load when the persisted session cwd differs from the launch dir', async () => { + it('loads a session whose persisted cwd differs from the launch dir (honors per-session cwd)', async () => { // Seed a session on disk whose header.cwd is a DIFFERENT absolute path than - // the server's launch dir, then load it requesting the launch cwd (so the - // request-cwd check passes). The bridge must still reject on the persisted - // header cwd — else it would replay that session while tools run here. + // the server's launch dir. The bridge must LOAD it (per-session cwd is + // honored — the resumed session keeps header.cwd, and bash routes there), no + // longer reject on a mismatch. loader = await makeBridgeHarness({ storageDir, script: [] }) const otherCwd = '/some/other/workspace' await loader.ctx.sessionPersistence.create({ @@ -101,23 +101,41 @@ describe('acp bridge — session/load replay', () => { ]) await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) - await expect(loader.client.loadSession({ sessionId: 'elsewhere', cwd: process.cwd(), mcpServers: [] })) - .rejects.toThrow(/created in \/some\/other\/workspace/) - // The rejected load must NOT have constructed/registered a live agent (the - // cwd is validated from persisted metadata BEFORE resume) — no leak. - expect(loader.ctx.agents.get('elsewhere')).toBeUndefined() - // And a fresh newSession still works (the connection is not wedged). - const ok = await loader.client.newSession({ cwd: process.cwd(), mcpServers: [] }) - expect(ok.sessionId).toBeTruthy() + // Load succeeds even though the requested cwd is the launch dir, not otherCwd. + const res = await loader.client.loadSession({ sessionId: 'elsewhere', cwd: process.cwd(), mcpServers: [] }) + expect(res).toBeDefined() + // The resumed session retains its ORIGINAL workspace cwd (so bash runs there). + expect(loader.ctx.agents.get('elsewhere')!.session.header.cwd).toBe(otherCwd) }) - it('rejects load for a non-absolute or mismatched cwd', async () => { + it('rejects load for a non-absolute cwd (still required to be absolute)', async () => { loader = await makeBridgeHarness({ storageDir, script: [] }) await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) await expect(loader.client.loadSession({ sessionId: 's', cwd: 'rel', mcpServers: [] })) .rejects.toThrow(/absolute/) - await expect(loader.client.loadSession({ sessionId: 's', cwd: '/other', mcpServers: [] })) - .rejects.toThrow(/launch directory/) + }) + + it('rejects loading a persisted session that has NO cwd (would silently run in the launch dir)', async () => { + // A legacy / externally-created session log with no header.cwd. The bridge + // must reject the load rather than accept it and let bash silently fall back + // to the server's launch dir (the request cwd does not override the header). + loader = await makeBridgeHarness({ storageDir, script: [] }) + await loader.ctx.sessionPersistence.create({ + version: 1, id: SessionId('legacy'), createdAt: 1, updatedAt: 1, // no cwd + }) + await loader.ctx.sessionPersistence.append(SessionId('legacy'), [ + { type: 'turn/start', seq: 0, time: 0, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }, + { type: 'turn/end', seq: 1, time: 0, data: { turn: 1, reason: { kind: 'completed' } } }, + ]) + await loader.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} }) + await expect(loader.client.loadSession({ sessionId: 'legacy', cwd: process.cwd(), mcpServers: [] })) + .rejects.toThrow(/no absolute persisted cwd/) + // Rejected BEFORE resume (metadata-only check) — no agent was registered, so + // the id is not wedged: a later attempt hits the same clean rejection, not a + // duplicate-registration error. + expect(loader.ctx.agents.get('legacy')).toBeUndefined() + await expect(loader.client.loadSession({ sessionId: 'legacy', cwd: process.cwd(), mcpServers: [] })) + .rejects.toThrow(/no absolute persisted cwd/) }) it('allows loading alongside an existing session but rejects re-loading the SAME id', async () => { diff --git a/packages/tool-bash/README.md b/packages/tool-bash/README.md index 2736884765..83f84e967d 100644 --- a/packages/tool-bash/README.md +++ b/packages/tool-bash/README.md @@ -13,10 +13,10 @@ Requires a loaded executor implementation (e.g. `@deepseek-ai/dsh-bash-local`); | `command` | string (required) | Run via `bash -c`. No state persists between calls — use `workdir`, not `cd`. | | `description` | string (required) | One-line, active-voice summary of the command (5-10 words), for UI/log display only — no effect on execution. | | `timeoutMs` | number | Default/max from executor config (120s/600s for bash-local). | -| `workdir` | string | Working directory for this call. | +| `workdir` | string | Working directory for this call. Defaults to the calling agent's session cwd (`session.header.cwd`) so each session runs in its own workspace; a relative `workdir` is resolved against that session cwd. | | `run_in_background` | boolean | Return a task id immediately; no timeout applies. | -`command`, `workdir`, and `timeoutMs` are resolved against the executor's config defaults via `ctx.bash.resolve()` before execution, so the executor seam (`BashExecSpec`) receives explicit `workdir`/`timeoutMs` values. +`command`, `workdir`, and `timeoutMs` are resolved against the executor's config defaults via `ctx.bash.resolve()` before execution, so the executor seam (`BashExecSpec`) receives explicit `workdir`/`timeoutMs` values. The workdir default is applied in the tool layer (from the calling agent's `session.header.cwd`) BEFORE `resolve()` — the per-session cwd must come from `exec.agent`, since N sessions share one executor; only when no session cwd is available does the executor fall back to its own config / `process.cwd()`. Result text: stdout, then a `[stderr]` section, then status markers — `[timed out after Nms]` whenever the executor's timer fired (reported independently of how the process ended, so a command that traps SIGTERM and exits 0 still shows it), `[killed by signal: …]` for a signal death, `[exit code: N]` for a non-zero exit (reported, **not** `isError`: the model decides how to react), and `[output truncated; full output: ]` when the tail was kept. Only infrastructure failures (spawn errors, aborts) surface as `isError` results. diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index b812b32f83..f4b0559a0b 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -37,6 +37,7 @@ */ import type { Context } from 'cordis' +import { isAbsolute, resolve as resolvePath } from 'node:path' import { defineTool } from '@deepseek-ai/dsh-tools' import type { Agent } from '@deepseek-ai/dsh-agent' import type { BashRunResult, BashTask, CollectedOutput } from '@deepseek-ai/dsh-bash' @@ -122,6 +123,26 @@ export function renderResult(result: BashRunResult): string { return body + markers.join('\n') } +/** + * Resolve the working directory for a bash call. Precedence: an explicit model + * `workdir` wins; otherwise default to the calling agent's session cwd + * (`session.header.cwd`) so each ACP session's commands run in ITS workspace, + * not the server's launch dir. A RELATIVE model `workdir` is resolved against + * the session cwd (the tool tells the model to pass `workdir` instead of `cd`, + * so a relative one should be relative to the session's root, not `process.cwd()`). + * Returns `undefined` when neither is available (no agent / headerless session / + * no session cwd) — the executor then applies its own config/`process.cwd()` + * default, preserving today's non-ACP behavior. + */ +function resolveWorkdir(modelWorkdir: string | undefined, exec: { agent?: Agent }): string | undefined { + const sessionCwd = exec.agent?.session.header.cwd + if (modelWorkdir === undefined) return sessionCwd + if (sessionCwd !== undefined && !isAbsolute(modelWorkdir)) { + return resolvePath(sessionCwd, modelWorkdir) + } + return modelWorkdir +} + /** Status line for background task reads. */ function statusLine(task: BashTask): string { switch (task.status) { @@ -193,7 +214,7 @@ export function apply(ctx: Context): void { + '"git status" → "Show working tree status"; "npm install" → "Install package dependencies".', }, timeoutMs: { type: 'number', description: 'Timeout in milliseconds (default 120000, max 600000). The command is killed on expiry.' }, - workdir: { type: 'string', description: 'Working directory for this command.' }, + workdir: { type: 'string', description: 'Working directory for this command. Defaults to the session workspace; a relative path is resolved against it.' }, run_in_background: { type: 'boolean', description: 'Run in the background and return a task id immediately. No timeout applies.' }, }, async execute(args, exec) { @@ -201,9 +222,13 @@ export function apply(ctx: Context): void { // `description` is display/logging metadata only (surfaced to UIs via // the tool/call session event); it is intentionally NOT forwarded to // ctx.bash and has no effect on execution. + // Default the workdir to the calling agent's session cwd so each ACP + // session runs in its own workspace (see resolveWorkdir); an explicit + // model workdir still wins. + const workdir = resolveWorkdir(args.workdir, exec) const request = { command: args.command, - ...args.workdir !== undefined ? { workdir: args.workdir } : {}, + ...workdir !== undefined ? { workdir } : {}, ...args.timeoutMs !== undefined ? { timeoutMs: args.timeoutMs } : {}, ...exec.signal ? { signal: exec.signal } : {}, } diff --git a/packages/tool-bash/tests/tools.spec.ts b/packages/tool-bash/tests/tools.spec.ts index c5c058a6f2..c33ccbd5fa 100644 --- a/packages/tool-bash/tests/tools.spec.ts +++ b/packages/tool-bash/tests/tools.spec.ts @@ -265,7 +265,7 @@ describe('background tools', () => { it('injects a completion notice into the owning agent', async () => { const ctx = await setup() const inject = vi.fn() - const agent = { inject } as unknown as import('@deepseek-ai/dsh-agent').Agent + const agent = { inject, session: { header: { version: 1, id: 'bg', createdAt: 0 } } } as unknown as import('@deepseek-ai/dsh-agent').Agent const started = await ctx.tools.execute({ callId: CallId('call-bg'), @@ -290,6 +290,7 @@ describe('background tools', () => { const ctx = await setup() const agent = { inject: () => { throw new Error('agent "x" is disposed') }, + session: { header: { version: 1, id: 'bg', createdAt: 0 } }, } as unknown as import('@deepseek-ai/dsh-agent').Agent const started = await ctx.tools.execute({ @@ -311,6 +312,7 @@ describe('background tools', () => { try { const agent = { inject: () => { throw new Error('unexpected inject bug') }, + session: { header: { version: 1, id: 'bg', createdAt: 0 } }, } as unknown as import('@deepseek-ai/dsh-agent').Agent const started = await ctx.tools.execute({ @@ -344,7 +346,7 @@ describe('background task ownership (cross-session isolation)', () => { return ctx.tools.execute({ callId: CallId(`own-${++callCounter}`), name, arguments: args, ...agent ? { agent } : {} }) } // Distinct identities — ownership is by agent object identity, not id. - const fakeAgent = () => ({ inject: () => undefined }) as unknown as import('@deepseek-ai/dsh-agent').Agent + const fakeAgent = () => ({ inject: () => undefined, session: { header: { version: 1, id: 'bg', createdAt: 0 } } }) as unknown as import('@deepseek-ai/dsh-agent').Agent it('rejects bash_output/bash_kill for a task owned by a DIFFERENT agent', async () => { const ctx = await setup() @@ -438,6 +440,50 @@ describe('background task ownership (cross-session isolation)', () => { }) }) +describe('session-cwd routing (per-session workdir)', () => { + function callAs(ctx: Context, agent: import('@deepseek-ai/dsh-agent').Agent | undefined, args: unknown) { + return ctx.tools.execute({ callId: CallId(`cwd-${++callCounter}`), name: 'bash', arguments: args, ...agent ? { agent } : {} }) + } + // An agent whose session header carries a cwd (what session/new records). + const agentInCwd = (cwd: string) => + ({ inject: () => undefined, session: { header: { version: 1, id: 'c', createdAt: 0, cwd } } }) as unknown as import('@deepseek-ai/dsh-agent').Agent + + it('defaults bash to the agent\'s session cwd (not the server launch dir)', async () => { + const ctx = await setup() + const result = await callAs(ctx, agentInCwd('/tmp'), { command: 'pwd', description: 'pwd' }) + expect(text(result).trim()).toMatch(/\/tmp$/) + }) + + it('an explicit absolute workdir overrides the session cwd', async () => { + const ctx = await setup() + const result = await callAs(ctx, agentInCwd('/'), { command: 'pwd', description: 'pwd', workdir: '/tmp' }) + expect(text(result).trim()).toMatch(/\/tmp$/) + }) + + it('a relative workdir is resolved against the session cwd', async () => { + const ctx = await setup() + // session cwd /usr + relative 'bin' → /usr/bin + const result = await callAs(ctx, agentInCwd('/usr'), { command: 'pwd', description: 'pwd', workdir: 'bin' }) + expect(text(result).trim()).toMatch(/\/usr\/bin$/) + }) + + it('two sessions with different cwds each run bash in their own dir', async () => { + const ctx = await setup() + const inUsr = await callAs(ctx, agentInCwd('/usr'), { command: 'pwd', description: 'pwd' }) + const inTmp = await callAs(ctx, agentInCwd('/tmp'), { command: 'pwd', description: 'pwd' }) + expect(text(inUsr).trim()).toMatch(/\/usr$/) + expect(text(inTmp).trim()).toMatch(/\/tmp$/) + }) + + it('falls back to the executor default when the agent has no session cwd', async () => { + const ctx = await setup() + // No exec.agent at all → executor uses its config/process.cwd() default. + const result = await ctx.tools.execute({ callId: CallId('cwd-noagent'), name: 'bash', arguments: { command: 'pwd', description: 'pwd' } }) + expect(result.isError).toBe(false) + expect(text(result).trim().length).toBeGreaterThan(0) + }) +}) + describe('renderResult', () => { const base = { exitCode: 0 as number | null,