fix(acp): address Codex review — strict ctx.get, correct fiber-ownership doc + test

- AgentLoop.resume uses `this.ctx.get('sessionPersistence')` (strict) instead
  of the `, false` overload: still topology-independent, but an inactive/
  absent backend reads as undefined (rejected by the existing guard) rather
  than being handed back mid-teardown.
- Correct the bridge teardown comment: an ACP-created agent's registry entry
  binds to the BRIDGE fiber (the factory is reached through the bridge's
  traceable proxy, so AgentLoop.start's `this.ctx.effect` registration uses the
  caller context), not the AgentLoop fiber — so an ACP-only HMR dispose
  reclaims it. Add a regression test pinning that ownership.
- Sync the ctx.get guidance in the post-mortem, packages/AGENTS.md, and the
  dsh-code-review skill to the strict form.
This commit is contained in:
Tianyi Cui
2026-06-18 03:48:33 +08:00
parent 86ec067bff
commit 7c168fca34
6 changed files with 53 additions and 27 deletions

View File

@@ -543,14 +543,19 @@ export function apply(ctx: Context, config: AcpConfig): void {
* the worst case is one short queued turn, since the bridge enforces a single
* in-flight prompt.
*
* The agent itself is NOT individually disposed/unregistered here — the
* factory (`ctx.agents.create`/`resume`) registers it on the AgentLoop fiber
* and returns no per-agent disposer, so the registry entry is reclaimed when
* the host context disposes. On a bare client disconnect (without a host
* dispose) the idled agent therefore lingers in `ctx.agents` until shutdown;
* since the MVP is single-session-per-connection and a reconnect spins up a
* fresh context, this does not strand work. A per-agent disposal seam is
* RFC 011 follow-up (TODO(rfc010-agent-disposal)).
* The agent itself is NOT individually disposed/unregistered here. The
* factory (`ctx.agents.create`/`resume`) registers it via `AgentLoop.start`'s
* `this.ctx.effect(...)`; because the factory is reached through this bridge's
* traceable service proxy, that effect's `this.ctx` is the CALLER context (the
* bridge fiber), so the registry entry is bound to the bridge fiber and is
* reclaimed when the bridge fiber disposes (whole-context dispose, or an
* ACP-only HMR `acpFiber.dispose()` — both unregister the agent). What this
* teardown path handles is a bare client disconnect, which resolves
* `conn.closed` WITHOUT disposing the fiber: the agent is idled+aborted here
* but stays in `ctx.agents` until the fiber is disposed. Since the MVP is
* single-session-per-connection and a reconnect spins up a fresh context, the
* lingering idle agent strands no work. A per-agent disposal seam (unregister
* on disconnect) is an RFC 011 follow-up (TODO(rfc010-agent-disposal)).
*/
let quiescing: Promise<void> | undefined
const quiesce = (): Promise<void> => {

View File

@@ -48,6 +48,25 @@ describe('acp bridge — disposal & HMR safety', () => {
await harness.dispose()
})
it('an agent created through the bridge is unregistered when ONLY the bridge fiber is disposed', async () => {
// The factory (`ctx.agents.create`) is reached through the bridge's
// traceable service proxy, so `AgentLoop.start`'s `this.ctx.effect(...)`
// registration binds to the CALLER context — the bridge fiber — not the
// AgentLoop fiber. Disposing JUST the bridge fiber (an ACP-only HMR reload)
// must therefore reclaim the agent's registry entry, even though agents/
// agent-loop stay up. This pins the fiber-ownership the bridge's teardown
// doc comment relies on; if a refactor rebinds the registration to the
// AgentLoop fiber, the agent would survive bridge dispose and this fails.
const harness = await makeBridgeHarness({ storageDir, script: [] })
await harness.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
const { sessionId } = await harness.client.newSession({ cwd: process.cwd(), mcpServers: [] })
expect(harness.ctx.agents.get(sessionId)).toBeDefined()
await harness.acpFiber.dispose() // tear down ONLY the bridge
expect(harness.ctx.agents.get(sessionId)).toBeUndefined()
await harness.dispose()
})
it('no agent is created by a session/new after the bridge has closed (closed guard)', async () => {
// After teardown (here a client disconnect sets `closed`), a late
// `session/new` must NOT create an orphan agent the bridge can no longer