diff --git a/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md b/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md index 3e68024fa9..3dc779ffa4 100644 --- a/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md +++ b/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md @@ -3,7 +3,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. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): `session/new` accepts any absolute `cwd`, and `session/load` requires the request `cwd` to match the persisted session `cwd` so the editor and bash executor agree on the workspace. +> **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. `session/cancel` is the queue-aware `agent.cancel()`: it aborts a running step, clears queued + steering work, and drops a turn that is about to start, so a queued-but-not-yet-started prompt never runs and a later prompt cannot be batched into the cancelled turn. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): `session/new` accepts any absolute `cwd`, and `session/load` requires the request `cwd` to match the persisted session `cwd` so the editor and bash executor agree on the workspace. ## Problem @@ -33,7 +33,7 @@ The mapping between ACP and existing harness seams — each row names the seam a | `session/update: tool_call` (pending→in_progress) | `session/event` `tool/call` | demux via a Session→sessionId map; `kind` inferred from the tool name | | `session/update: tool_call_update` (completed/failed) | `session/event` `tool/result` | a throwing `tools/execute` yields NO `tool/result` → fail the pending tool UI from `agent/error`/turn-end | | `session/request_permission {sessionId, toolCall, options}` | prepended `tools/execute` listener | no-op unless `exec.agent` is ACP-owned; await the outcome; `selected/allow_*` → `next()`; `reject_*`/`cancelled` → veto `ToolExecutionResult{isError}` | -| `session/cancel` (notification) | `agent.abort(reason)` | settle the in-flight prompt as `cancelled`; resolve any pending permission as `cancelled` exactly once | +| `session/cancel` (notification) | `agent.cancel(reason)` | the queue-aware cancel (abort running step, clear queued + steering, drop an about-to-start turn); settle the in-flight prompt as `cancelled`; resolve any pending permission as `cancelled` exactly once | The permission gate is the first real consumer of the `tools/execute` veto seam (the documented "single veto/sandbox/permission seam" plus the deferred "Permission system" TODO in [docs/architecture.md](../../architecture.md)). It is a single global listener registered with `prepend: true` so it runs before any other tool wrapper. `ToolExecution.agent` is optional and the `Agent` interface carries no origin marker, so the bridge tracks ownership itself: it records each agent it creates in a `WeakMap` and the gate no-ops (calls `next()` immediately) for any `exec.agent` it does not own — non-ACP agents and the no-agent case pass straight through. For an owned agent it resolves the session, issues `session/request_permission`, and stores the pending resolver on that session's record so the outcome — or a `session/cancel`/connection-close — settles it exactly once. diff --git a/docs/rfc/proposed/2026-06-14-acp-multi-session.md b/docs/rfc/proposed/2026-06-14-acp-multi-session.md index 8cc2237eff..25fe24c972 100644 --- a/docs/rfc/proposed/2026-06-14-acp-multi-session.md +++ b/docs/rfc/proposed/2026-06-14-acp-multi-session.md @@ -18,7 +18,7 @@ The harness core already supports many agents (`AgentRegistry.list()` and `Agent - Lift the single-session guard in `session/new`; allow N live sessions, each mapped to its own `ReactLoopAgent`. - The bridge's `sessionId→agent` and `Session→sessionId` maps (introduced single-entry by [the ACP support RFC](2026-06-14-acp-agent-client-protocol.md)) become true multi-entry, plus a third `agent→sessionId` reverse map: the `tools/execute` permission gate receives only `exec.agent` (no sessionId), so it needs an O(1) reverse lookup to find the owning session. Every `agent/*` event and every `session/event` is demuxed strictly by id, so two sessions streaming at once never interleave their `session/update` notifications. - Per-session prompt queues: [the ACP support RFC](2026-06-14-acp-agent-client-protocol.md)'s single-entry in-flight-prompt state becomes multi-entry — one in-flight prompt *per session*, tracked per `sessionId`. -- Per-session cancel routing: `session/cancel` aborts only its own session's agent and settles only that session's in-flight prompt. `agent.abort()` drives a per-agent `AbortController`, so the per-session `exec.signal` is the natural isolation fence. +- Per-session cancel routing: `session/cancel` cancels only its own session's agent (via the queue-aware `agent.cancel()`) and settles only that session's in-flight prompt. The cancel is scoped to that one agent — a per-agent `AbortController` for the running step plus the agent's own queued/steering FIFOs — so it never touches another session's stream or pending prompt. - Per-session permission ownership: a `session/request_permission` and its outcome are bound to the originating session via the reverse map, so a permission prompt or a cancel in one session can never resolve another session's pending permission. ## Plan diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index 3a6ac76a74..b8f43a0a9b 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -201,14 +201,22 @@ export async function runLoop(ctx: Context, agent: ReactLoopAgent, handle: LoopH // Pre-step cancel (window 2): `setStatus('running')` emits `agent/status` // SYNCHRONOUSLY, so a `running` listener can `cancel()` in the gap between the - // check above and `runTurn`. `cancel()` already cleared the queued FIFO, so - // drop the turn before it starts (runTurn would otherwise throw on an empty - // queue) and transition back to idle — `running` was already emitted, so a - // real `idle` transition (which also settles waiters) balances the status. + // check above and `runTurn`. Mirror window 1: clear the marker, then + // - if NOTHING new is queued, drop the about-to-run turn and transition + // back to `idle` (`running` was already emitted, so a real idle + // transition balances the status AND settles `whenIdle()` waiters); + // - if a NEW prompt was queued AFTER the cancel (a `running` listener that + // cancels then sends), the marker was for the cancelled work only — fall + // through and run the new prompt's turn (status is already `running`), so + // a `whenIdle()` waiter resolves on THAT turn's running→idle, not before + // it runs. Settling here would resolve quiescence while the replacement + // is still queued and unrun (the same early-resolve race window 1 fixes). if (handle.isCancelled()) { handle.clearCancel() - handle.setStatus('idle') - continue + if (!agent.inbox.hasQueued) { + handle.setStatus('idle') + continue + } } // Re-derive the turn number from the log each iteration (do NOT keep a local diff --git a/packages/agent-loop/tests/cancel.spec.ts b/packages/agent-loop/tests/cancel.spec.ts index ab05c4e25b..9392b4b3d0 100644 --- a/packages/agent-loop/tests/cancel.spec.ts +++ b/packages/agent-loop/tests/cancel.spec.ts @@ -252,6 +252,36 @@ describe('Agent.cancel()', () => { expect(agent.session.events.some(e => e.type === 'turn/start')).toBe(false) }) + it('window 2: whenIdle() does NOT resolve early when a running listener cancels then queues replacement work', async () => { + // The window-1 early-resolve race has a window-2 twin: a synchronous + // agent/status('running') listener cancels the about-to-run turn AND queues a + // replacement. window 2 must NOT settle waiters (via setStatus('idle')) while + // the replacement is still queued-and-unrun — it must fall through and run it, + // so whenIdle() resolves on the replacement turn's running→idle, not before. + const adapter = new MockAdapter([textResponse('A reply'), textResponse('B reply')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + let replaced = false + const dispose = ctx.on('agent/status', (subject, status) => { + if (subject !== agent || status !== 'running' || replaced) return + replaced = true + agent.cancel('drop A') + send(agent, 'B') + }) + + send(agent, 'A') + const idle = agent.whenIdle() + await idle + dispose() + + // whenIdle() resolved only AFTER B's turn ran: B's user message + a turn/end + // are in the log, and A was dropped. + expect(userTexts(agent)).toContain('B') + expect(userTexts(agent)).not.toContain('A') + expect(agent.session.events.some(e => e.type === 'turn/end')).toBe(true) + }) + it('whenIdle() does NOT resolve early when a new prompt is queued during a pre-step cancel', async () => { // The subtle race: a whenIdle() waiter is registered for prompt A; cancel() // clears A; prompt B is queued BEFORE the loop resumes from the idle wait.