From 90a19f072d73c0c2a283f28968bf89d20da48872 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 13:38:48 +0800 Subject: [PATCH] docs(acp,rfc): fix stale ownership wording + propose unifying agent/session id (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the bash owner-token PR: - packages/acp/README.md still described task isolation in object-identity terms ("records each background task's owning agent", "a different agent"). Rewrite to the session-token model: ownership is by `session.header.id`, stored on the executor's task, so a different Agent object on the same session may access it and ownership survives a tool-bash HMR reload. - The reviewer flagged that the notice routes by `session.header.id` while the registry only enforces unique `agent.id`, so a programmatic caller could register two agents sharing a session token and mis-route a notice (not reachable via ACP). Rather than bolt a session-id invariant onto the generic registry, add a proposed RFC (2026-06-20-unify-agent-and-session-id) to remove the precondition by construction — an agent IS its session, one id — with a full risks discussion (forecloses multi-session-actor / fork futures, makes the config resume-or-create policy load-bearing, migration churn). The actual unification ships as its own Codex-converged PR. Cross-linked from the agent-lifecycle RFC's seam-precondition note. - Reframe the tool-bash module-doc ownership paragraph to current-state (per the new AGENTS.md doc convention): contrast storing the token on the executor vs in the plugin as a standing rationale, not as "closing the old gap". --- docs/rfc/README.md | 1 + ...-18-agent-lifecycle-and-ownership-seams.md | 2 + .../2026-06-20-unify-agent-and-session-id.md | 57 +++++++++++++++++++ packages/acp/README.md | 2 +- packages/tool-bash/src/index.ts | 9 +-- 5 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 docs/rfc/proposed/2026-06-20-unify-agent-and-session-id.md diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 196958a709..0fe36dca75 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -31,6 +31,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Multiplex concurrent ACP sessions over one connection](proposed/2026-06-14-acp-multi-session.md) | 2026-06-14 | | [Optional Code Mode — model writes TypeScript against an SDK of all tools](proposed/2026-06-15-optional-code-mode.md) | 2026-06-15 | | [Runtime schemas for the event vocabulary (Zod vs the merge-extensible-map pattern)](proposed/2026-06-16-typed-event-schemas.md) | 2026-06-16 | +| [Unify the agent id and the session id](proposed/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | ## Implemented diff --git a/docs/rfc/implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md b/docs/rfc/implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md index 9f4e930b44..839e5d6c9f 100644 --- a/docs/rfc/implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md +++ b/docs/rfc/implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md @@ -35,6 +35,8 @@ Background-task ownership moved from a `tool-bash` plugin-local `Map`). +- **Resume**: a caller-supplied `agentId` (e.g. `"main"`) on a persisted `resumeSessionId`. + +Everywhere a live consumer actually looks an agent up — the **ACP bridge, the only production path** — the two are already unified: `agentId === sessionId === `. + +The separation is **latent generality no consumer exercises**: nothing reads a *stable* `agentId` back across runs (each process starts fresh, and persistence keys off the session id, never the agent id). The config path's "stable agentId, fresh sessionId" buys nothing concrete — it is cosmetic. And the `agentId !== sessionId` case is precisely what opens the bash owner-token alias hole: the bash completion-notice routes by `session.header.id`, but the registry enforces uniqueness only on `agentId`, so a programmatic caller registering two agents with different agent ids but the SAME session id can mis-route a notice (see [agent lifecycle and ownership seams](../implemented/2026-06-18-agent-lifecycle-and-ownership-seams.md) § Seam precondition). The current code documents this as a precondition rather than guaranteeing it. + +## Proposal + +Make an agent BE its session: one id. An agent's registry handle IS its `session.header.id`. + +- `CreateAgentOptions` drops the separate `sessionId` — the single `id` is both the registry handle and the live/persisted session id. (ACP already passes the same UUID for both, so its call site simplifies to one field.) +- `ResumeAgentOptions` drops the separate `agentId` — resuming `sessionId` X registers the agent under id X. (ACP already does this.) +- The config path (`AgentLoop.create`) uses its configured `id` directly as the session id, applying whatever resume-or-create policy it adopts (today it appends a per-run uuid to avoid colliding with an on-disk log; that policy moves onto the single id, e.g. the config id IS the session and a durable backend resumes it — to be settled in the implementing PR). +- The registry's existing unique-`agentId` check becomes, by construction, a unique-session-id guarantee — the bash alias hole is closed with NO new defensive invariant: two agents cannot share a session id because the session id is the agent id. + +## Why not just enforce session-id uniqueness in `AgentRegistry.register()`? + +That was the review's first suggestion. It would couple the generic registry to a session-uniqueness assumption (the registry tracks *agents*, not sessions) and entrench the very separation this RFC removes. Unifying the ids closes the hole more cleanly — there is nothing left to enforce. + +## Acceptance criteria + +- `ctx.agents.create`/`resume` take a single id; the ACP bridge passes one id. +- The config-driven agent path has a deliberate, documented session-id policy (no silent per-run id divergence that no consumer reads). +- The bash owner-token alias hole is gone by construction (no two live agents can share a session id). +- All existing behavior the tests pin (ACP create/resume/load, config startup, durability) still holds — or the tests change WITH the behavior where the divergence was an artifact (per AGENTS.md "tests document behavior, not golden truth"). + +## Risks + +This touches public factory interfaces (`CreateAgentOptions`, `ResumeAgentOptions`, `AgentFactory`) and the config-agent id scheme, so it is a deliberate cross-package change, not a local patch — it ships as its own PR (converged with Codex), stacked on the bash owner-token work that surfaced the precondition. + +The genuine risks of collapsing the two ids into one (the case AGAINST this proposal — to be weighed honestly before implementing): + +- **It forecloses a one-agent-resumes-many-sessions / one-session-driven-by-many-agents future.** Today the separate ids leave room for an agent (a stable actor) to detach from one session and attach to another, or for a handoff where a new agent process adopts an existing session under a new actor handle. Unifying makes "agent" and "session" the same lifetime, so any such future needs a NEW seam (e.g. an explicit `actorId` distinct from the session) — re-introducing the very separation we removed. We judge this generality currently unused, but it is a door this change closes. + +- **Sub-agents / fork / spawn (an explicitly deferred seam) may WANT a stable actor id across forked sessions.** `AgentLoop.create`'s `TODO(sub-agents)` envisions a child agent seeded from a parent's event log. If the design wants "the same agent identity across a fork" (parent and child share an actor but have distinct session logs), a unified id blocks it. The implementing PR must check the intended fork/spawn model BEFORE unifying, or accept that fork always mints a fresh combined id. + +- **The config-driven resume-or-create policy becomes load-bearing, not cosmetic.** Today the per-run-uuid session id quietly sidesteps the "a fixed id collides with its own on-disk log on the second run" problem. Once the id is unified and stable, a config agent restarting MUST decide resume-vs-fresh deliberately — there is no longer a throwaway session id to hide behind. Getting this wrong reintroduces the create-collision the uuid was avoiding (a durable backend refuses to re-create an id whose log exists). This is the one real design decision the implementing PR owns, and it is easy to get subtly wrong. + +- **Persisted/on-disk identity becomes the agent identity.** Unifying means the registry handle is now a persisted, externally-meaningful string (a session id a client chose), not an internal label. A caller that previously used a short human label (`"main"`) as the agent id now must use the session id. This is fine for ACP (already a UUID) but is a semantic narrowing for any programmatic embedder that relied on naming its agents independently of session storage. + +- **Migration churn touches every create/resume call site and its tests.** `CreateAgentOptions`/`ResumeAgentOptions` shape changes ripple to ACP, the config path, the agent-loop factory, and ~dozens of test fixtures that currently pass distinct `agentId`/`sessionId` (some deliberately distinct to exercise the divergence — those tests change WITH the behavior, per AGENTS.md "tests document behavior, not golden truth"). The risk is mechanical but broad; a missed call site is a type error, but a missed *test* could silently lose coverage of a path. + +The one real design question the implementing PR must settle first is the config-driven resume-or-create policy once the id is unified (today's per-run-uuid behavior is a demo simplification already flagged `TODO(demo)`). If, on closer look, the fork/spawn or multi-session-actor futures turn out to be wanted, this RFC should be REJECTED in favor of the lighter "enforce session-id uniqueness in the registry" guard — the alias hole is not reachable via ACP, so keeping the ids separate and merely documenting (or mechanically enforcing) the precondition remains a valid alternative. diff --git a/packages/acp/README.md b/packages/acp/README.md index 78c35eff86..23f32b9bd4 100644 --- a/packages/acp/README.md +++ b/packages/acp/README.md @@ -34,7 +34,7 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map` (forward) with a `WeakMap` reverse map so `agent/*` events — which carry only the `Agent` — demux in O(1). Every `session/event` and `agent/status` is routed strictly to its owning record, so concurrent sessions never cross-settle or interleave their `session/update` notifications. State is per session: one in-flight prompt each, `session/cancel` aborts and settles only its own agent/prompt, and disposal drains every live session in parallel to quiescence. (Per-session *permission* ownership is reserved for the deferred permission gate — `TODO(rfc010-permission-gate)`.) -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. +Background-task isolation rides on `dsh-tool-bash`: bash task ids are global and predictable, so each task carries an opaque owner token — the owning agent's `session.header.id` — stored on the task inside the executor (`dsh-bash`'s `ownerOf(id)` seam). `bash_output`/`bash_kill` reject a task whose token differs from the caller's session token, so one session's agent can't read or kill another's task. Ownership is by session TOKEN, not `Agent` object identity — a different `Agent` object on the same session may access the task — and because the token lives on the executor's task it survives a `tool-bash` HMR reload. ## Per-session cwd diff --git a/packages/tool-bash/src/index.ts b/packages/tool-bash/src/index.ts index 7302aed6f1..0ea01ca50d 100644 --- a/packages/tool-bash/src/index.ts +++ b/packages/tool-bash/src/index.ts @@ -22,10 +22,11 @@ * multi-session ACP (RFC 011) this token check is the fence that stops one * session's agent from reading or killing another session's background task. * - * Because ownership lives on the task in the EXECUTOR (disposed with the - * `dsh-bash` fiber), it SURVIVES a `tool-bash` HMR reload — closing the old - * plugin-local-map gap where a reload orphaned pre-reload tasks. (The - * `onTaskDone` listener is still effect-scoped to this plugin's `apply`, so a + * Storing the token on the task in the EXECUTOR (disposed with the `dsh-bash` + * fiber), rather than in this plugin, is what makes ownership survive a + * `tool-bash` HMR reload — a reload that reset a plugin-local map would orphan + * a task spawned before it. (The `onTaskDone` listener is still effect-scoped + * to this plugin's `apply`, so a * completion landing during the reload gap still drops its one notice — the * pre-existing reload-gap drop — but the ownership fence itself is HMR-proof.) *