feat(acp): honor per-session cwd — run each ACP session in its own workspace
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.
This commit is contained in:
Tianyi Cui
@@ -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
|
||||
|
||||
|
||||
+28
-30
@@ -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')
|
||||
}
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user