docs(acp,rfc): fix stale ownership wording + propose unifying agent/session id (review)
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".
This commit is contained in:
@@ -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 |
|
| [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 |
|
| [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 |
|
| [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
|
## Implemented
|
||||||
|
|
||||||
|
|||||||
@@ -35,6 +35,8 @@ Background-task ownership moved from a `tool-bash` plugin-local `Map<string, Age
|
|||||||
|
|
||||||
The bash owner-token comparison relies on `session.header.id` being unique among live agents. The agent registry does NOT enforce this — it rejects a duplicate *agentId*, not a duplicate session id, and `createAgent` accepts an arbitrary `sessionId`. This is NOT reachable via ACP (UUID sessionId, `agentId === sessionId`, duplicate-load rejected), so it is not a live product hole, but a programmatic caller that registers two agents with the same session id would break bash isolation and mis-route the completion notice. The access *policy* (token comparison) stays in `tool-bash` (the consumer); the bash seam stores only an opaque `owner` string and never interprets it — the correct interface/impl/consumer split.
|
The bash owner-token comparison relies on `session.header.id` being unique among live agents. The agent registry does NOT enforce this — it rejects a duplicate *agentId*, not a duplicate session id, and `createAgent` accepts an arbitrary `sessionId`. This is NOT reachable via ACP (UUID sessionId, `agentId === sessionId`, duplicate-load rejected), so it is not a live product hole, but a programmatic caller that registers two agents with the same session id would break bash isolation and mis-route the completion notice. The access *policy* (token comparison) stays in `tool-bash` (the consumer); the bash seam stores only an opaque `owner` string and never interprets it — the correct interface/impl/consumer split.
|
||||||
|
|
||||||
|
The planned resolution is to remove the precondition by construction — see [unify the agent id and the session id](../proposed/2026-06-20-unify-agent-and-session-id.md): once an agent IS its session (one id), the registry's existing unique-`agentId` check is a unique-session-id guarantee and no two live agents can share a session token.
|
||||||
|
|
||||||
## Notes
|
## Notes
|
||||||
|
|
||||||
This touched public interfaces (`Agent`, `AgentFactory`, the bash seam) deliberately, not as a local ACP patch. The simple synchronous `Agent.send()` ergonomics were preserved; the async lifecycle path is additive, for owners that need it.
|
This touched public interfaces (`Agent`, `AgentFactory`, the bash seam) deliberately, not as a local ACP patch. The simple synchronous `Agent.send()` ergonomics were preserved; the async lifecycle path is additive, for owners that need it.
|
||||||
|
|||||||
57
docs/rfc/proposed/2026-06-20-unify-agent-and-session-id.md
Normal file
57
docs/rfc/proposed/2026-06-20-unify-agent-and-session-id.md
Normal file
@@ -0,0 +1,57 @@
|
|||||||
|
# RFC: Unify the agent id and the session id
|
||||||
|
|
||||||
|
Status: proposed
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
The agent factory carries TWO ids for what is, in every live consumer, one thing:
|
||||||
|
|
||||||
|
- `agentId` — the `AgentRegistry` handle (the actor identity; the registry rejects a duplicate).
|
||||||
|
- `sessionId` — the event-sourced session / persisted-log identity (`session.header.id`).
|
||||||
|
|
||||||
|
`CreateAgentOptions` takes both separately; `ResumeAgentOptions` takes an `agentId` plus a `resumeSessionId`. They diverge in exactly two places:
|
||||||
|
|
||||||
|
- **Config-driven create** (`AgentLoop.create`): a stable `agentId` (e.g. `"echo"`) with a fresh per-run `sessionId` (`${id}-session-<uuid>`).
|
||||||
|
- **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 === <uuid>`.
|
||||||
|
|
||||||
|
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.
|
||||||
@@ -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<sessionId, SessionRecord>` (forward) with a `WeakMap<Agent, sessionId>` 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)`.)
|
The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map<sessionId, SessionRecord>` (forward) with a `WeakMap<Agent, sessionId>` 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
|
## Per-session cwd
|
||||||
|
|
||||||
|
|||||||
@@ -22,10 +22,11 @@
|
|||||||
* multi-session ACP (RFC 011) this token check is the fence that stops one
|
* 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.
|
* session's agent from reading or killing another session's background task.
|
||||||
*
|
*
|
||||||
* Because ownership lives on the task in the EXECUTOR (disposed with the
|
* Storing the token on the task in the EXECUTOR (disposed with the `dsh-bash`
|
||||||
* `dsh-bash` fiber), it SURVIVES a `tool-bash` HMR reload — closing the old
|
* fiber), rather than in this plugin, is what makes ownership survive a
|
||||||
* plugin-local-map gap where a reload orphaned pre-reload tasks. (The
|
* `tool-bash` HMR reload — a reload that reset a plugin-local map would orphan
|
||||||
* `onTaskDone` listener is still effect-scoped to this plugin's `apply`, so a
|
* 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
|
* 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.)
|
* pre-existing reload-gap drop — but the ownership fence itself is HMR-proof.)
|
||||||
*
|
*
|
||||||
|
|||||||
Reference in New Issue
Block a user