From 61136b22bbfabae49227843f924cc4832a07fe51 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 14 Jul 2026 02:09:09 +0800 Subject: [PATCH] refactor: remove UI identity translations --- docs/config-catalog.md | 4 +- docs/rfc/INDEX.md | 2 +- .../architecture/2026-06-20-branded-ids.md | 4 +- .../2026-06-20-unify-agent-and-session-id.md | 38 ++++++++++++++ .../2026-06-20-unify-agent-and-session-id.md | 38 -------------- packages/ui/acp/README.md | 4 +- packages/ui/acp/src/index.ts | 52 ++++++++----------- packages/ui/acp/tests/approval.spec.ts | 9 ++-- packages/ui/acp/tests/bridge.spec.ts | 3 +- packages/ui/jsonrpc/README.md | 2 +- packages/ui/jsonrpc/src/server.ts | 34 ++++-------- packages/ui/jsonrpc/tests/server.spec.ts | 20 ++++--- 12 files changed, 100 insertions(+), 110 deletions(-) create mode 100644 docs/rfc/implemented/simplification/2026-06-20-unify-agent-and-session-id.md delete mode 100644 docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md diff --git a/docs/config-catalog.md b/docs/config-catalog.md index 94d613bc4e..4ae020f0be 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -11,7 +11,7 @@ A `Requires:` line lists the service keys the plugin `inject`s: its `cordis.yml` ## `@deepseek-ai/dsh-acp` -Requires: `agents` · `sessions` · `sessionPersistence` · `tools` · `userInteraction` +Requires: `agents` · `sessionPersistence` · `tools` · `userInteraction` ```ts config-catalog /** Plugin config: the agent template ACP sessions are created from. */ @@ -31,7 +31,7 @@ export interface AcpConfig { Depends on: `Stream` (`@agentclientprotocol/sdk`) -Source: [`packages/ui/acp/src/index.ts:249`](../packages/ui/acp/src/index.ts) +Source: [`packages/ui/acp/src/index.ts:246`](../packages/ui/acp/src/index.ts) ## `@deepseek-ai/dsh-acp-agent` diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index a5334a63e1..5ed8c6ae6d 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -17,7 +17,6 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | Title | First proposed | |---|---| -| [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | | [Prune dead public and result surface](proposed/simplification/2026-07-04-prune-dead-core-spine-surface.md) | 2026-07-04 | | [Drop unconsumed skill provider events](proposed/simplification/2026-07-12-drop-unconsumed-skill-provider-events.md) | 2026-07-12 | | [Prune unused web seam fields](proposed/simplification/2026-07-12-prune-unused-web-seam-fields.md) | 2026-07-12 | @@ -87,6 +86,7 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | [Prune dead methods from the persistence seam](implemented/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | | [Keep one public stop primitive](implemented/simplification/2026-06-20-public-agent-stop-surface.md) | 2026-06-20 | | [Stop mirroring durable boundaries as agent events](implemented/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | +| [Unify the agent id and the session id](implemented/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | | [Split the filesystem seam — provider text mutations plus the `dsh-fs-policy` plugin](implemented/simplification/2026-06-26-fsspec-style-fs-seam.md) | 2026-06-26 | | [Stop mirroring the token stream as an agent event](implemented/simplification/2026-07-02-remove-stream-chunk-mirror.md) | 2026-07-02 | | [Drop the `image` content block until a path can honor it](implemented/simplification/2026-07-04-drop-image-content-block.md) | 2026-07-04 | diff --git a/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md b/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md index 5fe8224644..4fe7f58168 100644 --- a/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md +++ b/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md @@ -8,7 +8,7 @@ The harness brands `CallId` (`packages/llm/llm/src/brand.ts`) and the shared age **Gap 1 — unbranded cross-boundary IDs in the bash seam.** The background-task id is a plain `string`: `BashTask.id: string` (`packages/bash/bash/src/types.ts`), carried as `string` through the whole executor seam (`BashExecutor.get`/`ownerOf`/`readOutput`/`kill(id: string)` in `packages/bash/bash/src/index.ts`) and validated/passed as `string` by the model-facing tools (`validateTaskId`, `assertTaskAccess`, the `task_id` schema arg in `packages/bash/tool-bash/src/index.ts`). It is generated by a per-executor counter — `` `bash-${this.nextTaskId++}` `` in `packages/bash/bash-local/src/index.ts` — which gives it **exactly the same `name-N` shape as `SessionId`'s default** (`` `session-${++counter}` `` in `packages/core/session/src/index.ts`). A bash task id and a session id are trivially swappable at a call site and the compiler says nothing. This is the headline case the user asked about, and it is a model-facing id (the model passes `task_id` back to `bash_output`/`bash_kill`), so a confusion here is reachable from untrusted input. -The bash **owner token** is the related sub-case: `BashExecRequest.owner?: string` and `BashExecSpec.owner: string | undefined` (`packages/bash/bash/src/types.ts`) are documented as a deliberately *opaque* isolation key, but in every live caller the value IS the owning agent's `session.header.id` (`callerToken = (exec) => exec.agent?.session.header.id` in `packages/bash/tool-bash/src/index.ts`) — i.e. a `SessionId` wearing a `string` disguise. It is compared for access control (`owner !== callerToken(exec)`), so a mismatched-but-well-typed string here is a cross-session isolation bug the type system currently cannot catch. This is the same `session.header.id`-as-owner alias that the [unify-the-agent-id-and-the-session-id](../../proposed/simplification/2026-06-20-unify-agent-and-session-id.md) proposal calls the "bash owner-token alias hole". +The bash **owner token** is the related sub-case: `BashExecRequest.owner?: string` and `BashExecSpec.owner: string | undefined` (`packages/bash/bash/src/types.ts`) are documented as a deliberately *opaque* isolation key, but in every live caller the value IS the owning agent's `session.header.id` (`callerToken = (exec) => exec.agent?.session.header.id` in `packages/bash/tool-bash/src/index.ts`) — i.e. a `SessionId` wearing a `string` disguise. It is compared for access control (`owner !== callerToken(exec)`), so a mismatched-but-well-typed string here is a cross-session isolation bug the type system currently cannot catch. This is the same `session.header.id`-as-owner alias covered by the [unified agent/session identity decision](../simplification/2026-06-20-unify-agent-and-session-id.md). **Gap 2 — brand erosion at the seams of the *already-branded* IDs.** Even `CallId` and `SessionId` decay back to bare `string` at exactly the places confusion is most likely: registry/store key types and public method params. Representative sites include the session store, the agent registry (both keyed by the shared `SessionId`), `ToolPresenter`'s call-id map, ACP's session-id records and loading set, and the persistence coordinator. A brand that is dropped at a collection key buys nothing on lookups — the value of the existing brands is partly unrealized. @@ -62,6 +62,6 @@ The landed invariants: `BashTaskId` and `OwnerToken` are defined in `dsh-bash` a ## Consequences -- **Mechanical churn across two surfaces.** Propagating brands touches the bash seam (interface + impl + consumer) and the ACP session-id surface plus the persistence coordinator. The churn is broad but low-severity: a missed site is a compile error, not a silent bug. The change is observably type-only — no snapshot or e2e behavioral diff. It sits next to the [unify-the-agent-id-and-the-session-id](../../proposed/simplification/2026-06-20-unify-agent-and-session-id.md) proposal (both touch the session-id / owner-token boundary); if that proposal lands, `OwnerToken` still stays distinct from the unified id for the decoupling reason above. +- **Mechanical churn across two surfaces.** Propagating brands touches the bash seam (interface + impl + consumer) and the ACP session-id surface plus the persistence coordinator. The churn is broad but low-severity: a missed site is a compile error, not a silent bug. The change is observably type-only — no snapshot or e2e behavioral diff. It sits next to the [unified agent/session identity decision](../simplification/2026-06-20-unify-agent-and-session-id.md) because both touch the session-id / owner-token boundary; `OwnerToken` stays distinct from the unified id for the decoupling reason above. - **Brands do not validate.** A brand is a confusability guard, not a correctness proof: a *wrong* session id that is still a well-formed string passes the type checker exactly as before. This RFC does not close that gap (see Out of scope) — it only stops the *category* error of passing the wrong *kind* of id. - **The "where to stop" line stays a judgment call.** Branding `BashTaskId` but not `ToolName`, `OwnerToken` but not `ModelId`, is a taste call about which strings "could plausibly be confused." Reasonable reviewers may want more or fewer; the policy in `brand.ts` is the tie-breaker, and this RFC errs toward the ids that are model-facing or used for access control. diff --git a/docs/rfc/implemented/simplification/2026-06-20-unify-agent-and-session-id.md b/docs/rfc/implemented/simplification/2026-06-20-unify-agent-and-session-id.md new file mode 100644 index 0000000000..79d7332553 --- /dev/null +++ b/docs/rfc/implemented/simplification/2026-06-20-unify-agent-and-session-id.md @@ -0,0 +1,38 @@ +# RFC: Unify the agent id and the session id + +Status: implemented + +## Problem + +The agent factory previously carried two ids for each live agent/session pair: `agentId`, the `AgentRegistry` routing handle, and `sessionId`, the event-sourced/persisted-log identity. `CreateAgentOptions` took both; `ResumeAgentOptions` took `agentId` plus `resumeSessionId`; in-process subagents minted two independent UUIDs despite recording lineage separately. + +ACP already used the same value for both identities. Where they diverged, stdio kept `labelBySession` solely to recover an agent label from session events, and hooks exposed both values for authors to reconcile. No production path reattached one live agent object to several sessions or drove one session through several agent ids. + +The [agent-scope runtime](../../implemented/architecture/2026-07-12-agent-scope-runtime-design.md) had no reservation side tables: create and resume used one `AgentCreationTransaction`, and agent/session entries used the same final-entry collision rule. Separate ids therefore did not duplicate asynchronous liveness, rollback, or quiescence machinery. Identity unification was only an API and representation simplification: it deleted one caller-supplied id, one UUID per in-process child, and the remaining translation paths without changing the transaction lifecycle. + +Session itself repeated the same fact as `Session.id` and `Session.header.id`. Construction rejected a header whose id differed, so the aliases were constrained equal; the durable boundary nevertheless had to validate the duplicate, and production consumers chose between its two homes. + +## Decision + +An agent's registry id equals its session id. `CreateAgentOptions` accepts one `sessionId` used for both final registry entries; resume registers the agent under `resumeSessionId`; subagent creation mints one combined id; and `Session.id` derives from `header.id`. The existing creation transaction, final-entry collision checks, and exact-entry detach semantics remain; maps and fields whose sole job was translating between the ids are gone. + +The config-driven path keeps `agents[].id` as a stable configuration label, not a live routing identity. A fresh start mints the combined id `${label}-session-${randomUUID()}` so durable restarts do not collide; `resumeSessionId` instead supplies the exact combined identity to load and register. Logs may use the stable label while all live and durable lookups use the one `SessionId`. + +`agent/created` and `agent/disposed` remain. They are paired publication lifecycle events, not identity aliases; any later consumer-free removal needs its own proposal after a fresh search. + +## Alternatives considered + +**Keep separate routing and log identities.** A stable configured label plus a fresh durable conversation is useful, but it does not require two live identities: the label can remain configuration/display metadata while the combined per-run `SessionId` owns routing and persistence. Keeping two ids would preserve translation maps and permit impossible pairings without adding lifecycle capability. + +## Verification + +- Agent create/resume and subagent creation carry one identity, and `Session` stores it in one place. +- The creation transaction retains final-entry collision, exact-entry detach, rollback, and quiescence coverage without identity-specific lifecycle state. +- ACP, stdio, hooks, bash ownership, persistence, and lineage use the shared `SessionId` directly. ACP verifies exact `Agent` ownership from the forward session map; JSON-RPC caches only disposable-child parent lineage. +- The config-driven resume-or-create policy is explicit and covered across a durable restart. +- A production listener search kept `agent/created`/`agent/disposed` and their publication semantics. +- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. + +## Consequences + +This forecloses latent multi-session-actor and session-handoff designs and makes persisted client-chosen session identity the registry identity. If separate routing identity becomes a real requirement, it needs an explicit lifecycle design rather than an unconstrained caller-supplied pair. diff --git a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md b/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md deleted file mode 100644 index 6e3eea1515..0000000000 --- a/docs/rfc/proposed/simplification/2026-06-20-unify-agent-and-session-id.md +++ /dev/null @@ -1,38 +0,0 @@ -# RFC: Unify the agent id and the session id - -Status: proposed - -## Problem - -The agent factory carries two ids for each live agent/session pair: `agentId`, the `AgentRegistry` routing handle, and `sessionId`, the event-sourced/persisted-log identity. `CreateAgentOptions` takes both; `ResumeAgentOptions` takes `agentId` plus `resumeSessionId`; in-process subagents mint two independent UUIDs despite recording lineage separately. - -ACP already uses the same value for both identities. Where they diverge, stdio keeps `labelBySession` solely to recover an agent label from session events, and hooks expose both values for authors to reconcile. No production path reattaches one live agent object to several sessions or drives one session through several agent ids. - -The [agent-scope runtime](../../implemented/architecture/2026-07-12-agent-scope-runtime-design.md) has no reservation side tables: create and resume use one `AgentCreationTransaction`, and agent/session entries use the same final-entry collision rule. Separate ids therefore do not duplicate asynchronous liveness, rollback, or quiescence machinery. Identity unification is only an API and representation simplification: it deletes one caller-supplied id, one UUID per in-process child, and the remaining translation paths without changing the transaction lifecycle. - -Session itself repeats the same fact as `Session.id` and `Session.header.id`. Construction rejects a header whose id differs, so the aliases are constrained equal; the durable boundary must nevertheless validate the duplicate, and production consumers choose between its two homes. - -## Proposal - -Make an agent's registry id equal its session id. `CreateAgentOptions` accepts one id used for both final registry entries; resume registers the agent under the resumed session id; subagent creation mints one combined id; Session keeps one identity home by deriving `id` from `header.id` or removing the alias. Keep the existing creation transaction, final-entry collision checks, and exact-entry detach semantics; remove only maps and fields whose sole job is translating between the ids. - -The config-driven path keeps `agents[].id` as a stable configuration label, not a live routing identity. A fresh start mints the combined id `${label}-session-${randomUUID()}` so durable restarts do not collide; `resumeSessionId` instead supplies the exact combined identity to load and register. Logs may use the stable label while all live and durable lookups use the one `SessionId`. - -`agent/created` and `agent/disposed` remain outside this proposal. They are paired publication lifecycle events, not identity aliases; any later consumer-free removal needs its own proposal after a fresh search. - -## Alternatives considered - -**Keep separate routing and log identities.** A stable configured label plus a fresh durable conversation is useful, but it does not require two live identities: the label can remain configuration/display metadata while the combined per-run `SessionId` owns routing and persistence. Keeping two ids would preserve translation maps and permit impossible pairings without adding lifecycle capability. - -## Acceptance criteria - -- Agent create/resume and subagent creation carry one identity; `Session` stores it in one place. -- The existing creation transaction keeps final-entry collision, exact-entry detach, rollback, and quiescence guarantees without adding identity-specific lifecycle state. -- ACP, stdio, hooks, bash ownership, persistence, and lineage need no agent/session id translation. -- The config-driven resume-or-create policy is explicit and covered across a durable restart. -- `agent/created`/`agent/disposed` are removed only if a post-change production search finds no listener; otherwise they and their publication semantics stay. -- Typecheck, coverage, snapshots, doc-sync, module-graph verification, build, and hygiene pass. - -## Risks - -This forecloses latent multi-session-actor and session-handoff designs, makes persisted client-chosen session identity the registry identity, and touches every factory fixture. If separate routing identity becomes a real requirement, it needs an explicit lifecycle design rather than an unconstrained caller-supplied pair. diff --git a/packages/ui/acp/README.md b/packages/ui/acp/README.md index f4947e5d25..31a865c068 100644 --- a/packages/ui/acp/README.md +++ b/packages/ui/acp/README.md @@ -8,7 +8,7 @@ It is a **client-driver / UI plugin**, the structured analogue of the readline ` `apply(ctx, config)` — wires an `AgentSideConnection` (from `@agentclientprotocol/sdk`) to `process.stdin`/`process.stdout` and implements the ACP `Agent` method surface. -`inject: ['agents', 'sessions', 'sessionPersistence', 'tools', 'userInteraction']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`; `tools` lets a tool own how its calls render (`presentCall`/`presentResult`) — the bridge looks the definition up by name and falls back to a generic presentation when a tool declares none (see Tool-call presentation). `userInteraction` lets agent-owned `ask_user_question` calls become ACP form elicitations routed to the owning session. +`inject: ['agents', 'sessionPersistence', 'tools', 'userInteraction']` — programs against the interface packages only (never `dsh-agent-loop`). `sessionPersistence` is required because `initialize` advertises `loadSession: true`; `tools` lets a tool own how its calls render (`presentCall`/`presentResult`) — the bridge looks the definition up by name and falls back to a generic presentation when a tool declares none (see Tool-call presentation). `userInteraction` lets agent-owned `ask_user_question` calls become ACP form elicitations routed to the owning session. ### Config @@ -36,7 +36,7 @@ The `initialize` handshake reports a fixed server identity (`agentInfo: { name: ## Multi-session -The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map` (forward) with a `WeakMap` reverse map so agent-scoped approval events demultiplex in O(1). Every `session/event` 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. Permission prompts follow the same ownership: the `approval/request` answerer resolves the owning session through the reverse map and prompts only there. +The bridge multiplexes N sessions over one connection. Live sessions are held in a `Map` keyed by the shared agent/session id. Agent-scoped events derive that id from `agent.session.id` and verify the record owns the exact agent object, so a foreign same-id object cannot claim the bridge's session. Every `session/event` 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. Permission prompts follow the same ownership and prompt only the matching session. ## Session config options diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index e795c7e177..369c8ae166 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -18,12 +18,11 @@ * turn about to start) + settle the in-flight prompt * * Multi-session (RFC 011): N concurrent sessions per connection, each mapped to - * its own `ReactLoopAgent`. Sessions are keyed by id in `sessions` (forward) with an - * `agent→sessionId` reverse map for O(1) demux of `agent/*` events; every - * `session/event` and `agent/*` event is routed strictly to its owning session - * record, so two sessions streaming at once never interleave their - * `session/update` notifications. Permission prompts ride the same ownership - * map: the bridge answers `approval/request` for its own agents over + * its own `ReactLoopAgent`. Sessions are keyed by their shared agent/session id; + * every `session/event` and `agent/*` event is routed strictly to its owning + * session record, so two sessions streaming at once never interleave their + * `session/update` notifications. Permission prompts use the same identity: the + * bridge answers `approval/request` for its own agents over * `session/request_permission` (see the approval answerer below) — whether a * call ASKS is policy (a hook or plugin returning `ask`), not the bridge's. * @@ -106,9 +105,7 @@ export const name = 'acp' // because `initialize` advertises `loadSession: true`. `tools` lets a tool own // how its calls render (`presentCall`/`presentResult`); the bridge looks up the // definition by name and falls back to a generic presentation when absent. -// TODO(acp-session-inject): drop `sessions`; this bridge never reads -// ctx.sessions, and agent/session ownership is already behind ctx.agents. -export const inject = ['agents', 'sessions', 'sessionPersistence', 'tools', 'userInteraction'] +export const inject = ['agents', 'sessionPersistence', 'tools', 'userInteraction'] /** * Build an ACP "invalid params" error whose human detail rides in the message. @@ -268,7 +265,6 @@ export const Config: Schema = Schema.object({ * map keyed by id (RFC 011 multi-session). */ interface SessionRecord { - sessionId: SessionId agent: Agent /** * The owned-agent disposer (from the {@link AgentHandle} the factory returned). @@ -353,15 +349,8 @@ export function apply(ctx: Context, config: AcpConfig): void { // this warn sink so a throwing tool presenter is logged, not propagated. const makePresenter = (agent?: Agent): ToolPresenter => new ToolPresenter(tools, (message) => { logger.warn(message) }, agent) - // TODO(derive-acp-session-id): derive an event's id from agent.session and - // verify sessions.get(id)?.agent === agent; then remove this reverse map and - // SessionRecord.sessionId, whose sole read duplicates the same identity. - // Live sessions keyed by id (RFC 011 multi-session), plus an agent→sessionId - // reverse map so `agent/*` events (which carry only the Agent) demux in O(1). - // The forward record and weak reverse entry are installed together; removing - // the record releases its strong Agent reference, so the WeakMap entry expires. + // Live sessions keyed by their shared agent/session id (RFC 011 multi-session). const sessions = new Map() - const bySession = new WeakMap() // Session ids whose `session/load` is mid-`resume()` (the slot is reserved // before the async resume so a pipelined load/new for the SAME id can't create // two agents). Distinct ids load concurrently; a given id loads once at a time. @@ -382,20 +371,26 @@ export function apply(ctx: Context, config: AcpConfig): void { // `notify` never observes it unset — no undefined guard needed. let conn: AgentSideConnection + /** Return the bridge-owned record for an agent, rejecting same-id impostors. */ + const ownedRecord = (agent: Agent): SessionRecord | undefined => { + const rec = sessions.get(agent.session.id) + return rec?.agent === agent ? rec : undefined + } + userInteraction.registerProvider({ async ask(request: AskUserQuestionRequest): Promise { if (request.agent === undefined) { throw new UserInteractionError('ACP user questions must come from an agent-owned request', 'NO_AGENT') } - const sessionId = bySession.get(request.agent) - if (sessionId === undefined) { + const rec = ownedRecord(request.agent) + if (rec === undefined) { throw new UserInteractionError('ACP user question has no matching session', 'NO_SESSION') } const answers: AskUserQuestionAnswerItem[] = [] for (const question of request.questions) { const options = question.options ?? [] const response = await withAbort(conn.unstable_createElicitation( - elicitationForQuestion(sessionId, question, options), + elicitationForQuestion(rec.agent.session.id, question, options), ), request.signal).catch((error: unknown) => { if (error instanceof UserInteractionError) throw error throw new UserInteractionError('ACP elicitation request failed', 'ASK_FAILED', { cause: error }) @@ -496,7 +491,7 @@ export function apply(ctx: Context, config: AcpConfig): void { const rec = sessions.get(session.header.id) if (rec === undefined) return try { - streamSessionEventUpdate(rec.sessionId, event, notify, rec.presenter, { + streamSessionEventUpdate(rec.agent.session.id, event, notify, rec.presenter, { enabled: rec.terminalEnabled, cwd: session.header.cwd, }, { includeUserMessages: false }) @@ -527,12 +522,12 @@ export function apply(ctx: Context, config: AcpConfig): void { // allow_always is a grant-storage design the approval RFC defers, so the // prompt never offers a durable grant the harness could not honor. ctx.on('approval/request', (req, next) => { - const sessionId = bySession.get(req.agent) + const rec = ownedRecord(req.agent) // The protocol requires `toolCall` (the prompt renders attached to it), so // a request without a callId has nothing to attach to — delegate. - if (sessionId === undefined || req.callId === undefined) return next() + if (rec === undefined || req.callId === undefined) return next() return conn.requestPermission({ - sessionId, + sessionId: rec.agent.session.id, toolCall: { toolCallId: req.callId }, options: [ { optionId: 'allow-once', name: 'Allow once', kind: 'allow_once' }, @@ -639,8 +634,7 @@ export function apply(ctx: Context, config: AcpConfig): void { // turn) leaves the switch pending — it runs no step, so nothing executes // or assembles under a stale value. ctx.on('agent/prompt-submit', (agent, _content, _source, next) => { - const sessionId = bySession.get(agent) - const rec = sessionId === undefined ? undefined : sessions.get(sessionId) + const rec = ownedRecord(agent) if (rec !== undefined) flushPendingSwitches(rec) return next() }) @@ -699,9 +693,7 @@ export function apply(ctx: Context, config: AcpConfig): void { await handle.dispose() throw internalError('connection closed during session/new') } - bySession.set(handle.agent, sessionId) sessions.set(sessionId, { - sessionId, agent: handle.agent, dispose: () => handle.dispose(), presenter: makePresenter(handle.agent), @@ -773,13 +765,11 @@ export function apply(ctx: Context, config: AcpConfig): void { throw invalidParams('connection closed during session/load') } const agent = handle.agent - bySession.set(agent, sessionId) // Snapshot the terminal capability ONCE for this session (used by both // the replay below and the post-load live stream) so a later // `initialize` can't desync the call/result of a tool card. const terminalEnabled = terminalOutputCap const record: SessionRecord = { - sessionId, agent, dispose: () => handle.dispose(), presenter: makePresenter(agent), diff --git a/packages/ui/acp/tests/approval.spec.ts b/packages/ui/acp/tests/approval.spec.ts index 36846ac728..65679cd905 100644 --- a/packages/ui/acp/tests/approval.spec.ts +++ b/packages/ui/acp/tests/approval.spec.ts @@ -90,9 +90,12 @@ describe('acp bridge — approval answerer', () => { await harness.ctx.plugin(ApprovalService) harness.onPermission = () => ({ outcome: { outcome: 'selected', optionId: 'allow-once' } }) - // Not created through the bridge: no bySession entry, so the answerer must - // call next() — nobody else answers, so the seam fails closed. - const foreign = { session: { events: [{ type: 'turn/start' }], append: () => ({}) } } as unknown as Agent + const { agent } = await ownedAgentRequest(harness) + // Even an impostor that claims the bridge-owned session id must delegate: + // ownership requires the exact Agent object stored in the session record. + const foreign = { + session: { id: agent.session.id, events: [{ type: 'turn/start' }], append: () => ({}) }, + } as unknown as Agent await expect(harness.ctx.approval.request({ agent: foreign, toolName: 'echo', callId: CallId('c') })) .resolves.toBe('unavailable') expect(harness.permissionRequests).toHaveLength(0) diff --git a/packages/ui/acp/tests/bridge.spec.ts b/packages/ui/acp/tests/bridge.spec.ts index 7e780348ba..e7170c380f 100644 --- a/packages/ui/acp/tests/bridge.spec.ts +++ b/packages/ui/acp/tests/bridge.spec.ts @@ -205,7 +205,8 @@ describe('acp bridge', () => { await expect(harness.ctx.userInteraction.ask({ questions: [{ id: 'x', question: 'No agent?' }] })) .rejects.toMatchObject({ name: 'UserInteractionError', code: 'NO_AGENT' }) - await expect(harness.ctx.userInteraction.ask({ agent: { id: 'other' } as typeof agent, questions: [{ id: 'x', question: 'No session?' }] })) + const impostor = { session: { id: agent.session.id } } as typeof agent + await expect(harness.ctx.userInteraction.ask({ agent: impostor, questions: [{ id: 'x', question: 'No session?' }] })) .rejects.toMatchObject({ code: 'NO_SESSION' }) harness.onElicitation = () => ({ action: 'cancel' }) diff --git a/packages/ui/jsonrpc/README.md b/packages/ui/jsonrpc/README.md index 007bd1a5f1..933023a2d9 100644 --- a/packages/ui/jsonrpc/README.md +++ b/packages/ui/jsonrpc/README.md @@ -4,7 +4,7 @@ The **SDK server plugin** (`jsonrpc`): mounting it serves a stdio JSON-RPC serve ## Wiring -`inject: ['agents']` — the server creates one agent per SDK `sessionId` (get-or-create on `session/prompt`) and demuxes `subagent/end` through the registry. The LLM seam is read opportunistically via `ctx.get('llm')` (not injected): when `initialize.model` has no registered adapter, the plugin mounts `dsh-llm-deepseek` for it (credentials from `$DEEPSEEK_API_KEY` / `$DEEPSEEK_BASE_URL`); a config-registered adapter for the model wins. Everything else — persistence, the tool stacks, the adapter set — comes from the surrounding `cordis.yml`. +`inject: ['agents']` — the server creates one agent per SDK `sessionId` (get-or-create on `session/prompt`). A subagent's shared agent/session id supplies `subagent.finished.childSessionId` directly; the server caches only parent lineage because the child may be disposed before `subagent/end`. The LLM seam is read opportunistically via `ctx.get('llm')` (not injected): when `initialize.model` has no registered adapter, the plugin mounts `dsh-llm-deepseek` for it (credentials from `$DEEPSEEK_API_KEY` / `$DEEPSEEK_BASE_URL`); a config-registered adapter for the model wins. Everything else — persistence, the tool stacks, the adapter set — comes from the surrounding `cordis.yml`. ## Config diff --git a/packages/ui/jsonrpc/src/server.ts b/packages/ui/jsonrpc/src/server.ts index 4dbfeb0fa2..cfa083c394 100644 --- a/packages/ui/jsonrpc/src/server.ts +++ b/packages/ui/jsonrpc/src/server.ts @@ -58,11 +58,6 @@ interface SessionRecord { activePrompt: boolean } -interface SubagentRecord { - childSessionId: string - parentSessionId: string | undefined -} - /** * The SDK server over a booted harness context. Constructing it subscribes to * the context's `session/event`, `session/created`, `agent/created`, and @@ -76,7 +71,7 @@ export class HarnessSdkServer { private llmFiber: { dispose(): Promise } | undefined private readonly sessions = new Map() private readonly sessionCreations = new Map>() - private readonly subagentSessions = new Map() + private readonly subagentParents = new Map() private readonly disposers: (() => void)[] = [] private shutdownTask: Promise> | undefined private shuttingDown = false @@ -100,29 +95,22 @@ export class HarnessSdkServer { childSessionId: String(session.id), }) })) - // Cache agent → session lineage on creation: by the time `subagent/end` - // fires the child agent may already be disposed and gone from the registry. + // Cache parent lineage on creation: by the time `subagent/end` fires the + // child agent may already be disposed and gone from the registry. The child + // session id needs no cache because it is the shared agent/session id. this.disposers.push(ctx.on('agent/created', (agent) => { - this.subagentSessions.set(String(agent.id), { - childSessionId: String(agent.session.id), - parentSessionId: agent.session.header.parentSession === undefined - ? undefined - : String(agent.session.header.parentSession), - }) + const parentSessionId = agent.session.header.parentSession + if (parentSessionId !== undefined) this.subagentParents.set(agent.id, parentSessionId) })) this.disposers.push(ctx.on('subagent/end', (info: SubagentRunEndInfo) => { - const rec = this.subagentSessions.get(String(info.id)) const agent = this.ctx.agents.get(info.id) - const childSessionId = rec?.childSessionId ?? (agent === undefined ? undefined : String(agent.session.id)) - const parentSessionId = rec?.parentSessionId ?? ( - agent?.session.header.parentSession === undefined ? undefined : String(agent.session.header.parentSession) - ) - if (childSessionId === undefined) return + const parentSessionId = this.subagentParents.get(info.id) ?? agent?.session.header.parentSession + this.subagentParents.delete(info.id) this.transport.notify('subagent.finished', { provider: info.provider, agentId: String(info.id), - ...(parentSessionId === undefined ? {} : { parentSessionId }), - childSessionId, + ...(parentSessionId === undefined ? {} : { parentSessionId: String(parentSessionId) }), + childSessionId: String(info.id), status: info.stopReason === 'completed' ? 'ok' : 'error', stopReason: info.stopReason, ...(info.lastAssistantMessage === undefined ? {} : { lastAssistantMessage: info.lastAssistantMessage }), @@ -195,7 +183,7 @@ export class HarnessSdkServer { this.sessionCreations.clear() const records = [...this.sessions.values()] this.sessions.clear() - this.subagentSessions.clear() + this.subagentParents.clear() const failures: unknown[] = [] while (this.disposers.length > 0) { try { diff --git a/packages/ui/jsonrpc/tests/server.spec.ts b/packages/ui/jsonrpc/tests/server.spec.ts index 9260a59aae..433119e046 100644 --- a/packages/ui/jsonrpc/tests/server.spec.ts +++ b/packages/ui/jsonrpc/tests/server.spec.ts @@ -272,6 +272,9 @@ describe('HarnessSdkServer', () => { meta: { cwd: storageDir, parentSession: SessionId('main') }, agentOptions: { model: 'deepseek' }, }) + // The backend may dispose the child before publishing its run outcome; + // only the cached parent lineage should be needed at this point. + await handle.dispose() await settleSubagent(ctx, parentHandle.agent, { provider: 'spawn', id: SessionId('child-session'), @@ -292,7 +295,6 @@ describe('HarnessSdkServer', () => { }, }) - await handle.dispose() await parentHandle.dispose() await server.shutdown() } finally { @@ -301,7 +303,7 @@ describe('HarnessSdkServer', () => { } }) - it('falls back to live agent lineage for uncached subagent end events', async () => { + it('falls back to live lineage and treats the shared id as the child session id', async () => { const storageDir = await mkdtemp(join(tmpdir(), 'dsh-jsonrpc-subagent-fallback-')) const ctx = await makeHarness(storageDir) let parentHandle: AgentHandle | undefined @@ -365,10 +367,16 @@ describe('HarnessSdkServer', () => { stopReason: 'error', }, }) - expect(transport.notifications.some(n => - n.method === 'subagent.finished' - && n.params?.agentId === 'missing-child-agent', - )).toBe(false) + expect(transport.notifications).toContainEqual({ + method: 'subagent.finished', + params: { + provider: 'fork', + agentId: 'missing-child-agent', + childSessionId: 'missing-child-agent', + status: 'error', + stopReason: 'error', + }, + }) await server.shutdown() } finally {