fix review findings: correct teardown framing (dispose, not cancel+whenIdle)

Codex's second pass caught that the prior doc fix swapped one wrong primitive
for another: framing teardown as cancel()+whenIdle() (or awaiting
agent.whenIdle() on disposal) is still wrong. whenIdle() only OBSERVES
quiescence; cancel() only stops queued/in-flight work. Neither unregisters the
agent or detaches the session. Real teardown is AgentHandle.dispose(), whose
disposer does `stop(); await agent.done` — stop the loop, await its exit, and
unregister (packages/core/agent-loop/src/index.ts:271). Copying the old framing
would reintroduce the orphaned-agent/session leak the AgentHandle seam exists
to prevent.

- docs/architecture.md: whenIdle() is a non-owner quiescence-observation hook,
  explicitly NOT teardown; teardown is `await AgentHandle.dispose()`.
- docs/cookbook/extension-cookbook.md (prose + the ts comment): tear agents
  down via AgentHandle.dispose(), not agent.whenIdle().
- docs/rfc/proposed/feature/2026-06-14-acp-agent-client-protocol.md: the
  lifecycle/disposal paragraph routes teardown through the handle's dispose().
This commit is contained in:
Tianyi Cui
2026-06-21 09:37:52 +08:00
parent c6ed980d6f
commit 436305b1c2
3 changed files with 4 additions and 4 deletions
@@ -37,7 +37,7 @@ The mapping between ACP and existing harness seams — each row names the seam a
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<Agent, sessionId>` 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.
Lifecycle and disposal: the connection, listeners, and in-flight permission promises register via `ctx.effect`/`ctx.on`; teardown is async and awaits quiescence — close the connection, settle/reject pending permissions, `agent.cancel()`, and wait for the agent to settle. The disposal-settle signal must come from the `dsh-agent` interface, not the loop: `agent.done` exists only on the concrete `ReactLoopAgent`, so the bridge instead observes `agent/status` reaching `idle`/`disposed` (or the RFC lifts a quiescence promise onto the `Agent` interface). Every listener contains its `send()` exceptions (log, never reject the turn) because stream chunks are emitted inside the model step, so a throwing listener would corrupt the turn.
Lifecycle and disposal: the connection, listeners, and in-flight permission promises register via `ctx.effect`/`ctx.on`; teardown is async and must *reach* quiescence, not just request it — close the connection, settle/reject pending permissions, and dispose each owned agent through its `AgentHandle.dispose()` (which stops the loop, `await`s its exit, and unregisters). Disposal must come through the `dsh-agent` handle seam, not the loop: `agent.done` exists only on the concrete `ReactLoopAgent`, so a bridge that wanted to wait on quiescence directly would instead observe `agent/status` reaching `idle`/`disposed` — but routing teardown through the handle's `dispose()` makes that unnecessary. Every listener contains its `send()` exceptions (log, never reject the turn) because stream chunks are emitted inside the model step, so a throwing listener would corrupt the turn.
**Dependency note (architecture rule).** [docs/architecture.md](../../../architecture.md) states "plugins depend on interface packages, never on `dsh-agent-loop`." Creating and resuming agents is currently only on the concrete `AgentLoop` (`ctx.agentLoop`), so this RFC proposes adding an **abstract create/resume factory** to the `dsh-agent` interface (registry-level `create({ sessionId, meta })` / `resume(...)`), implemented by the loop, so `dsh-acp` injects only `agents` (the interface) and the dependency rule holds. The alternative — injecting the concrete `agentLoop` and recording a documented exception in the architecture doc — is explicitly the non-preferred fallback.