diff --git a/AGENTS.md b/AGENTS.md index 1bb32121e9..9c584b2072 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,7 @@ A passing test pins the behavior the code **currently** has — not necessarily Before you preserve a behavior solely to keep a test green, ask: is this behavior load-bearing (a real consumer depends on it, a contract promises it, a user observes it), or is it an artifact? If it's an artifact, **change the behavior AND its test together, in the same change, and say why in the PR** — do not contort new code to keep an obsolete assertion passing, and do not treat "but the test expects X" as a reason X must stay. Conversely, do not delete a test just because it is inconvenient: the discipline cuts both ways — you must show the *behavior* is dead, not merely that the test is in your way. -The worked example is [Drop the mutable session summary](docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md): an entire `SessionSummary` type, a `SessionPersistence.update()` method, a JSONL sidecar, and SQLite columns existed and were exercised by their own contract test — yet nothing in production read or wrote any of it. The tests documented the behavior perfectly; the behavior was dead. Deleting the behavior and its tests together removed ~400 lines and erased a durability divergence the next refactor would have had to model. (This is the test-tier echo of "verify the world, not a synthetic stand-in" in § Defensive patterns: a test agrees with whatever it was written to assert; only a real consumer proves the behavior matters.) +The worked example is [Drop the mutable session summary](docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md): an entire `SessionSummary` type, a `SessionPersistence.update()` method, a JSONL sidecar, and SQLite columns existed and were exercised by their own contract test — yet **nothing in production CONSUMED any of it, and `update()` had no production caller**. (The backends did *write* summary state — JSONL touched the sidecar after a durable append, SQLite bumped `updated_at` in the append transaction — but those writes fed only reads that nothing performed.) The tests documented the behavior perfectly; the behavior was dead. Deleting the behavior and its tests together removed ~400 lines and erased a durability divergence the next refactor would have had to model. (This is the test-tier echo of "verify the world, not a synthetic stand-in" in § Defensive patterns: a test agrees with whatever it was written to assert; only a real consumer proves the behavior matters.) ## Architecture @@ -176,7 +176,7 @@ In the **core** packages (`packages/llm`, `packages/tools`, `packages/agent`, `p Verbose documentation is fine **as long as docs and code stay strictly in sync**. Out-of-sync docs are worse than no docs. **When you change code, update its docs in the SAME change** — grep the package README and the module/JSDoc comments for the old behavior (config keys, defaults, error codes, wire field names, event names) and fix every hit. CI runs `pnpm run doc-sync` (`doc-typecheck` + `verify-event-taxonomy` + `verify-md-wrap` + `verify-md-links`), which typechecks every fenced `ts` block in `README.md`, `docs/**/*.md`, and `packages/*/README.md`, verifies the event-taxonomy table against source, asserts no hard-wrapped prose paragraphs, and checks that every relative Markdown cross-link resolves — across those files plus `AGENTS.md` / `packages/AGENTS.md` — but that scope does NOT catch prose drift in `AGENTS.md` / `packages/AGENTS.md` / `packages/README.md` (config keys, defaults, error codes), so keeping those in sync remains on the author. Every module has a module-level doc comment explaining its role. Every exported class, interface, type, function, and non-obvious method has a JSDoc that explains semantics (not just the name) — contracts (what events fire when), disposal behavior, error behavior, and extension intent. Internal helpers get docs only where non-obvious. Prefer one-liners when one line suffices. -**Write an RFC when — and only when — a PR makes a decision that is durable, contested, and surprising.** RFCs (`docs/rfc/`, grouped into `proposed/` / `implemented/` / `rejected/`) record the *why* behind choices a future reader would otherwise re-litigate (the vendoring policy, event-sourcing, the schema DSL are the existing examples). A PR that introduces such a decision — a new third-party runtime dependency over the vendoring default, a cross-package contract, a security/isolation model, a deviation from a documented architecture rule — writes the RFC in `implemented/` **in the same PR**, and links it from the relevant code. A proposal for future work not yet built goes in `proposed/`. A PR whose changes are mechanical, self-evident, or already covered by an existing RFC needs none — do not manufacture an RFC for a routine change. When unsure, the test is: would a competent maintainer six months from now ask "why was it done this way?" and be unable to answer from the code alone? If yes, write it. See [docs/rfc/README.md](docs/rfc/README.md) for the naming scheme and [docs/AGENTS.md](docs/AGENTS.md) for the cross-link convention. +**Document the CURRENT state — the "what" and "why" — never the PROCESS or HISTORY of how it got there.** A comment, JSDoc, or doc paragraph describes what the code *is* and why it is that way, as if it had always been so. Do NOT narrate the change that produced it: no "previously X, now Y", "changed from", "used to", "this replaces", "the old map", "renamed", "moved here", "as of this PR", or "(was …)". Such phrasing rots the instant the next change lands, and a reader of the current code does not need the diff narrated in prose — that belongs in the commit message, the PR description, or an RFC (the durable home for "why we moved away from X"). Write "the owner token lives on the task in the executor" — not "ownership *now* lives on the executor instead of a plugin-local map". When a contrast genuinely aids understanding (a non-obvious choice between live alternatives), frame it against the alternative as a standing fact ("stored on the executor, NOT the tool plugin, so it survives an HMR reload"), not against the codebase's past. The same rule governs review-fix commits: the *commit message* records what the review caught; the *code comment* it touches states only the resulting truth. RFCs (`docs/rfc/`, grouped into `proposed/` / `implemented/` / `rejected/`) record the *why* behind choices a future reader would otherwise re-litigate (the vendoring policy, event-sourcing, the schema DSL are the existing examples). A PR that introduces such a decision — a new third-party runtime dependency over the vendoring default, a cross-package contract, a security/isolation model, a deviation from a documented architecture rule — writes the RFC in `implemented/` **in the same PR**, and links it from the relevant code. A proposal for future work not yet built goes in `proposed/`. A PR whose changes are mechanical, self-evident, or already covered by an existing RFC needs none — do not manufacture an RFC for a routine change. When unsure, the test is: would a competent maintainer six months from now ask "why was it done this way?" and be unable to answer from the code alone? If yes, write it. See [docs/rfc/README.md](docs/rfc/README.md) for the naming scheme and [docs/AGENTS.md](docs/AGENTS.md) for the cross-link convention. **Markdown is not hard-wrapped**: write one line per paragraph and let the editor soft-wrap. Hard line breaks mid-paragraph make docs harder to edit and diff — a one-word change reflows and re-diffs the whole paragraph. This applies to prose only: leave fenced code blocks, tables, and list structure intact (a wrapped list item folds to one line per bullet). Code comments / JSDoc are exempt — they stay under the linter's column limit. `pnpm run verify-md-wrap` (part of `doc-sync`) enforces this across `README.md`, `docs/**/*.md`, `packages/*/README.md`, and `AGENTS.md` / `packages/AGENTS.md`; `pnpm run verify-md-links` (also part of `doc-sync`) checks that every relative cross-link in those files resolves. diff --git a/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md b/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md index 3e68024fa9..3dc779ffa4 100644 --- a/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md +++ b/docs/rfc/proposed/2026-06-14-acp-agent-client-protocol.md @@ -3,7 +3,7 @@ Status: proposed -> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. One further best-effort limitation is tracked as `TODO(rfc010-cancel-prestep)`: `session/cancel` aborts a running step and settles the RPC as `cancelled`, but a turn still queued (not yet started) when the cancel arrives may execute before the abort takes effect, pending a loop-level pre-step cancel. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): `session/new` accepts any absolute `cwd`, and `session/load` requires the request `cwd` to match the persisted session `cwd` so the editor and bash executor agree on the workspace. +> **Implementation status (MVP landed):** steps 1, 2, 3, 4, 6, 7, 8 are implemented in `packages/acp` + `examples/acp-agent`. **Step 5 (the `session/request_permission` permission gate) is deferred** — the bridge ships a pass-through (tools run with the executor's full authority) marked `TODO(rfc010-permission-gate)`, and lays down only the `WeakMap` ownership seam the gate will build on. Status stays `proposed` until the gate lands. `session/cancel` is the queue-aware `agent.cancel()`: it aborts a running step, clears queued + steering work, and drops a turn that is about to start, so a queued-but-not-yet-started prompt never runs and a later prompt cannot be batched into the cancelled turn. **Per-session `cwd` is now honored** (lifting the original "launch the server in the workspace root" restriction — see § Deferred): `session/new` accepts any absolute `cwd`, and `session/load` requires the request `cwd` to match the persisted session `cwd` so the editor and bash executor agree on the workspace. ## Problem @@ -33,7 +33,7 @@ The mapping between ACP and existing harness seams — each row names the seam a | `session/update: tool_call` (pending→in_progress) | `session/event` `tool/call` | demux via a Session→sessionId map; `kind` inferred from the tool name | | `session/update: tool_call_update` (completed/failed) | `session/event` `tool/result` | a throwing `tools/execute` yields NO `tool/result` → fail the pending tool UI from `agent/error`/turn-end | | `session/request_permission {sessionId, toolCall, options}` | prepended `tools/execute` listener | no-op unless `exec.agent` is ACP-owned; await the outcome; `selected/allow_*` → `next()`; `reject_*`/`cancelled` → veto `ToolExecutionResult{isError}` | -| `session/cancel` (notification) | `agent.abort(reason)` | settle the in-flight prompt as `cancelled`; resolve any pending permission as `cancelled` exactly once | +| `session/cancel` (notification) | `agent.cancel(reason)` | the queue-aware cancel (abort running step, clear queued + steering, drop an about-to-start turn); settle the in-flight prompt as `cancelled`; resolve any pending permission as `cancelled` exactly once | 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` 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. diff --git a/docs/rfc/proposed/2026-06-14-acp-multi-session.md b/docs/rfc/proposed/2026-06-14-acp-multi-session.md index 8b3bcfa395..2eadf3eb63 100644 --- a/docs/rfc/proposed/2026-06-14-acp-multi-session.md +++ b/docs/rfc/proposed/2026-06-14-acp-multi-session.md @@ -18,7 +18,7 @@ The harness core already supports many agents (`AgentRegistry.list()` and `Agent - Lift the single-session guard in `session/new`; allow N live sessions, each mapped to its own `ReactLoopAgent`. - The bridge's `sessionId→agent` and `Session→sessionId` maps (introduced single-entry by [the ACP support RFC](2026-06-14-acp-agent-client-protocol.md)) become true multi-entry, plus a third `agent→sessionId` reverse map: the `tools/execute` permission gate receives only `exec.agent` (no sessionId), so it needs an O(1) reverse lookup to find the owning session. Every `agent/*` event and every `session/event` is demuxed strictly by id, so two sessions streaming at once never interleave their `session/update` notifications. - Per-session prompt queues: [the ACP support RFC](2026-06-14-acp-agent-client-protocol.md)'s single-entry in-flight-prompt state becomes multi-entry — one in-flight prompt *per session*, tracked per `sessionId`. -- Per-session cancel routing: `session/cancel` aborts only its own session's agent and settles only that session's in-flight prompt. `agent.abort()` drives a per-agent `AbortController`, so the per-session `exec.signal` is the natural isolation fence. +- Per-session cancel routing: `session/cancel` cancels only its own session's agent (via the queue-aware `agent.cancel()`) and settles only that session's in-flight prompt. The cancel is scoped to that one agent — a per-agent `AbortController` for the running step plus the agent's own queued/steering FIFOs — so it never touches another session's stream or pending prompt. - Per-session permission ownership: a `session/request_permission` and its outcome are bound to the originating session via the reverse map, so a permission prompt or a cancel in one session can never resolve another session's pending permission. ## Plan diff --git a/packages/acp/tests/dispose.spec.ts b/packages/acp/tests/dispose.spec.ts index d3af9ccd6c..3196994a0e 100644 --- a/packages/acp/tests/dispose.spec.ts +++ b/packages/acp/tests/dispose.spec.ts @@ -273,4 +273,47 @@ describe('acp bridge — disposal & HMR safety', () => { expect(harness.ctx.sessions.get('guard-a')).toBeUndefined() // detach still ran await harness.dispose() }) + + it('concurrent AgentHandle dispose() calls all await the SAME teardown (memoized)', async () => { + // The handle's dispose() must memoize: the underlying cordis effect disposer + // is single-shot, so a second dispose() while the first is mid-teardown would + // otherwise resolve IMMEDIATELY (effect epoch already cleared) — before the + // first call's await agent.done + final flush finished. Every caller must + // observe the same quiescence boundary. + const harness = await makeBridgeHarness({ storageDir, script: ['hang'] }) + const handle = harness.ctx.agents.create({ + agentId: 'conc-a', sessionId: 'conc-a', agentOptions: { model: 'mock' }, + }) + // Drive a turn that hangs in the model stream, so the loop is mid-turn when + // disposed — its exit runs a final session/flush we can gate to hold the + // teardown observably in-flight. + handle.agent.send([{ type: 'text', text: 'go' }]) + await new Promise(r => setTimeout(r, 30)) + expect(handle.agent.status).toBe('running') + let releaseFlush!: () => void + const flushGate = new Promise((resolve) => { releaseFlush = resolve }) + harness.ctx.on('session/flush', () => flushGate) + + // First dispose enters teardown (aborts the hanging step) and blocks in the + // gated final flush. + const first = handle.dispose() + let firstSettled = false + void first.then(() => { firstSettled = true }) + await new Promise(r => setTimeout(r, 20)) + expect(firstSettled).toBe(false) + + // Second dispose MUST await the same in-flight teardown, not resolve early. + const second = handle.dispose() + let secondSettled = false + void second.then(() => { secondSettled = true }) + await new Promise(r => setTimeout(r, 20)) + expect(secondSettled).toBe(false) // memoized: still pending with the first + + // Release the flush; both resolve together and the session is gone. + releaseFlush() + await Promise.all([first, second]) + expect(harness.ctx.agents.get('conc-a')).toBeUndefined() + expect(harness.ctx.sessions.get('conc-a')).toBeUndefined() + await harness.dispose() + }) }) diff --git a/packages/agent-loop/src/index.ts b/packages/agent-loop/src/index.ts index 3963b24594..5c25eb197d 100644 --- a/packages/agent-loop/src/index.ts +++ b/packages/agent-loop/src/index.ts @@ -267,15 +267,25 @@ export class AgentLoop extends Service implements AgentFactory { /** * Build an {@link AgentHandle} for a PREPARED session + a fresh agent. The - * handle's `dispose()` just runs the composite effect's disposer (see + * handle's `dispose()` runs the composite effect's disposer (see * {@link start}) — which stops the loop, awaits its exit (final flush * captured), unregisters the agent, and detaches the session, in that order. * The same composite effect is what a fiber unload disposes, so both teardown * triggers honor the ordering identically. + * + * `dispose()` is MEMOIZED: the underlying cordis effect disposer is + * single-shot (a second call returns immediately because the effect's epoch is + * already cleared, NOT awaiting the in-flight teardown), so concurrent/repeated + * `dispose()` calls would otherwise resolve before the first call's + * `await agent.done` + final flush completed. Memoizing the promise makes every + * caller observe the SAME quiescence boundary, honoring the + * `AgentHandle.dispose(): Promise` contract (mirrors the ACP `quiesce()` + * helper). */ private startOwned(id: AgentId, options: AgentOptions, session: Session): AgentHandle { const { agent, disposeAgent } = this.start(id, options, session) - return { agent, dispose: disposeAgent } + let disposing: Promise | undefined + return { agent, dispose: () => (disposing ??= disposeAgent()) } } } diff --git a/packages/agent-loop/src/loop.ts b/packages/agent-loop/src/loop.ts index 3a6ac76a74..b8f43a0a9b 100644 --- a/packages/agent-loop/src/loop.ts +++ b/packages/agent-loop/src/loop.ts @@ -201,14 +201,22 @@ export async function runLoop(ctx: Context, agent: ReactLoopAgent, handle: LoopH // Pre-step cancel (window 2): `setStatus('running')` emits `agent/status` // SYNCHRONOUSLY, so a `running` listener can `cancel()` in the gap between the - // check above and `runTurn`. `cancel()` already cleared the queued FIFO, so - // drop the turn before it starts (runTurn would otherwise throw on an empty - // queue) and transition back to idle — `running` was already emitted, so a - // real `idle` transition (which also settles waiters) balances the status. + // check above and `runTurn`. Mirror window 1: clear the marker, then + // - if NOTHING new is queued, drop the about-to-run turn and transition + // back to `idle` (`running` was already emitted, so a real idle + // transition balances the status AND settles `whenIdle()` waiters); + // - if a NEW prompt was queued AFTER the cancel (a `running` listener that + // cancels then sends), the marker was for the cancelled work only — fall + // through and run the new prompt's turn (status is already `running`), so + // a `whenIdle()` waiter resolves on THAT turn's running→idle, not before + // it runs. Settling here would resolve quiescence while the replacement + // is still queued and unrun (the same early-resolve race window 1 fixes). if (handle.isCancelled()) { handle.clearCancel() - handle.setStatus('idle') - continue + if (!agent.inbox.hasQueued) { + handle.setStatus('idle') + continue + } } // Re-derive the turn number from the log each iteration (do NOT keep a local diff --git a/packages/agent-loop/tests/cancel.spec.ts b/packages/agent-loop/tests/cancel.spec.ts index ab05c4e25b..9392b4b3d0 100644 --- a/packages/agent-loop/tests/cancel.spec.ts +++ b/packages/agent-loop/tests/cancel.spec.ts @@ -252,6 +252,36 @@ describe('Agent.cancel()', () => { expect(agent.session.events.some(e => e.type === 'turn/start')).toBe(false) }) + it('window 2: whenIdle() does NOT resolve early when a running listener cancels then queues replacement work', async () => { + // The window-1 early-resolve race has a window-2 twin: a synchronous + // agent/status('running') listener cancels the about-to-run turn AND queues a + // replacement. window 2 must NOT settle waiters (via setStatus('idle')) while + // the replacement is still queued-and-unrun — it must fall through and run it, + // so whenIdle() resolves on the replacement turn's running→idle, not before. + const adapter = new MockAdapter([textResponse('A reply'), textResponse('B reply')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create('a1', { model: 'mock' }) + + let replaced = false + const dispose = ctx.on('agent/status', (subject, status) => { + if (subject !== agent || status !== 'running' || replaced) return + replaced = true + agent.cancel('drop A') + send(agent, 'B') + }) + + send(agent, 'A') + const idle = agent.whenIdle() + await idle + dispose() + + // whenIdle() resolved only AFTER B's turn ran: B's user message + a turn/end + // are in the log, and A was dropped. + expect(userTexts(agent)).toContain('B') + expect(userTexts(agent)).not.toContain('A') + expect(agent.session.events.some(e => e.type === 'turn/end')).toBe(true) + }) + it('whenIdle() does NOT resolve early when a new prompt is queued during a pre-step cancel', async () => { // The subtle race: a whenIdle() waiter is registered for prompt A; cancel() // clears A; prompt B is queued BEFORE the loop resumes from the idle wait. diff --git a/packages/session-persistence/src/coordinator.ts b/packages/session-persistence/src/coordinator.ts index f35685c677..5371873097 100644 --- a/packages/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/src/coordinator.ts @@ -2,21 +2,20 @@ * The backend-agnostic write-path orchestration shared by every first-party * {@link SessionPersistence} backend. * - * The two durable backends (`dsh-session-persistence-jsonl` over file bytes, - * `dsh-session-persistence-sqlite` over `node:sqlite` rows) were byte-identical - * — or same-algorithm — for ALL of their orchestration: the in-memory - * bookkeeping (the per-id state, the write-behind buffers, the per-id - * serialization chains, the per-session init promises), the `session/event` → - * buffer → `session/flush` drain, lazy materialization, crash-tail repair on - * load, the four `session/created` adoption cases (new / HMR-adopt / collision / - * ownerless-claim), and dispose-time quiescence. Only the STORAGE primitives - * differed (write bytes vs. INSERT rows). {@link PersistenceCoordinator} owns - * the orchestration once; a backend supplies the storage primitives as a small + * Every durable backend needs the same orchestration: the in-memory bookkeeping + * (the per-id state, the write-behind buffers, the per-id serialization chains, + * the per-session init promises), the `session/event` → buffer → `session/flush` + * drain, lazy materialization, crash-tail repair on load, the four + * `session/created` adoption cases (new / HMR-adopt / collision / + * ownerless-claim), and dispose-time quiescence. Only the STORAGE primitives are + * backend-specific (file bytes for `dsh-session-persistence-jsonl`, `node:sqlite` + * rows for `dsh-session-persistence-sqlite`). {@link PersistenceCoordinator} owns + * the orchestration; a backend supplies the storage primitives as a small * {@link PersistenceBackend} hook object. * - * The abstract {@link SessionPersistence} service's public API is unchanged: a - * backend still IS a `SessionPersistence` (its six public methods delegate to a - * coordinator it composes), so a third-party backend MAY implement the service + * The abstract {@link SessionPersistence} service's public API is independent of + * this: a backend IS a `SessionPersistence` (its six public methods delegate to + * a coordinator it composes), so a third-party backend MAY implement the service * directly without using the coordinator at all. * * See the write-coordinator RFC (docs/rfc/implemented/2026-06-18-shared-persistence-write-coordinator.md) @@ -115,7 +114,19 @@ interface SessionState { meta: SessionHeader /** The next seq the backend expects to append (the stored log length). */ cursor: number - /** Whether the session has been physically materialized. */ + /** + * Whether the backend has physically written this session (a JSONL file / + * SQLite row exists). `create()` registers state LAZILY — cursor 0, + * materialized false, nothing on disk — so an empty session leaves no + * artifact and the FIRST `appendBatch` writes the header + its events in ONE + * transaction (the "a row exists ⇔ it has events" invariant `has`/`list` + * rely on; a separate up-front materialize could crash leaving a row with + * zero events). The flag is the only signal that distinguishes a session + * registered-but-never-written from one durably present, which two callers + * need: `has()` (lazy-but-unwritten is not yet durable) and the reclaim path + * (an abandoned id with no artifact AND no buffered events is free to reuse; + * a materialized one is a real collision). + */ materialized: boolean /** * The live Session this state was bound to via `onCreated`, if any. State @@ -450,9 +461,18 @@ export class PersistenceCoordinator { if (tracked.owner === session) return if (tracked.owner === undefined) { // Ownerless state from the public create()/load() API. The FIRST live - // session claims it — but ONLY if its seed reproduces the persisted - // prefix (else a fresh, unrelated session reusing the id would have its - // seq 0..cursor-1 events filtered as already-written and grafted on). + // session claims it — but ONLY if BOTH the cwd scope and the seed match. + // The cwd guard mirrors case-2's cwd-scoped loadLive(): a same-id + // ownerless artifact at a DIFFERENT cwd is a collision, not a claim + // (claiming it would append the live cwd's events under the stored + // header's cwd, the exact cross-cwd corruption the loadLive scope + // prevents). The seed guard then ensures the live events reproduce the + // persisted prefix (else a fresh, unrelated session reusing the id would + // have its seq 0..cursor-1 events filtered as already-written and + // grafted on). + if (tracked.meta.cwd !== session.header.cwd) { + throw new Error(`session "${id}" is already persisted at a different cwd (persisted: ${String(tracked.meta.cwd)}, live: ${String(session.header.cwd)}) (id collision)`) + } if (!await this.seedMatchesPersisted(id, seed, tracked.cursor)) { throw new Error(`session "${id}" is already persisted with ${tracked.cursor} event(s) that do not match this live session (id collision)`) } diff --git a/packages/session-persistence/tests/coordinator-contract.ts b/packages/session-persistence/tests/coordinator-contract.ts index 24a8f4c0cf..7769e6f5ec 100644 --- a/packages/session-persistence/tests/coordinator-contract.ts +++ b/packages/session-persistence/tests/coordinator-contract.ts @@ -68,6 +68,7 @@ export interface CoordinatorFixture { /** A constant absolute cwd; jsonl keys directories off it, memory/sqlite ignore it. */ const WORK = '/w' +const OTHER = '/other' /** The per-session init map a backend exposes for white-box init awaits. */ function inits(persistence: SessionPersistence): Map> { @@ -535,6 +536,58 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise< } }) + it('a live session at a DIFFERENT cwd cannot claim cursor-0 ownerless state (cwd scope)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // create() registers ownerless state at cwd /a (cursor 0 — claims would + // otherwise match trivially on the seed). + await ctx.sessionPersistence.create(meta('wrong-cwd-claim', OTHER)) + // A live session reusing the id but at cwd WORK must NOT claim it — the + // cwd scope is the fence (without it, WORK events would append under the + // OTHER header). Rejected as a collision. + const live = ctx.sessions.create('wrong-cwd-claim', { seed: oneTurnLog(), meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + + it('a live session at a DIFFERENT cwd cannot claim loaded-prefix ownerless state (cwd scope)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // Materialize + load at cwd OTHER (ownerless, cursor = 6). + await ctx.sessionPersistence.create(meta('wrong-cwd-load', OTHER)) + await ctx.sessionPersistence.append(SessionId('wrong-cwd-load'), oneTurnLog()) + const { events } = await ctx.sessionPersistence.load(SessionId('wrong-cwd-load')) + // A live session whose SEED matches the loaded prefix but whose cwd is + // WORK must still be rejected — the cwd guard runs before the seed check. + const live = ctx.sessions.create('wrong-cwd-load', { seed: events, meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + + it('a no-cwd ownerless state cannot be claimed by a live session WITH a cwd (cwd scope, undefined side)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // Ownerless state created WITHOUT a cwd (the no-cwd bucket). + await ctx.sessionPersistence.create(meta('no-cwd-state')) + // A live session reusing the id but WITH cwd WORK is a cwd mismatch + // (undefined vs WORK) and must be rejected. + const live = ctx.sessions.create('no-cwd-state', { seed: oneTurnLog(), meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + // --- append adopts a storage-only session (fresh instance, no prior create/load) --- it('append adopts a storage-only session (fresh instance) and continues the seq', async () => { diff --git a/packages/session/src/index.ts b/packages/session/src/index.ts index 8d1471d5f3..57210c3431 100644 --- a/packages/session/src/index.ts +++ b/packages/session/src/index.ts @@ -285,14 +285,18 @@ export class SessionStore extends Service { * {@link announce}, so a throwing `session/created` listener rolls the attach * back instead of leaking it. * - * The id was already validated by {@link prepare}, which runs in the SAME - * synchronous sequence as `enter` (a config/factory caller does - * `prepare()` → `ctx.effect(generator)`, and a synchronous generator effect - * iterates inline — no await between them), so no concurrent create can claim - * the id in the gap. `enter` therefore does not re-check; it is not a public - * reservation primitive. + * Re-checks the id for a duplicate: `prepare` and `enter` are public + * cross-package primitives and a caller may interleave arbitrary work (or + * another create) between them, so a stale prepared session must NOT overwrite + * a live store entry of the same id — its detach disposer would later delete + * the REAL session. The {@link create} convenience and the agent factory call + * the two back-to-back so they never trip this, but the public seam cannot + * assume that. + * + * @throws if a session with this id is already in the store. */ enter(session: Session): () => void { + if (this.store.has(session.id)) throw new Error(`session "${session.id}" already exists`) session.onAppend = (event) => { this.ctx.emit('session/event', session, event) } this.store.set(session.id, session) return () => { diff --git a/packages/session/tests/session.spec.ts b/packages/session/tests/session.spec.ts index ac40a8ea3f..593eed36c0 100644 --- a/packages/session/tests/session.spec.ts +++ b/packages/session/tests/session.spec.ts @@ -221,6 +221,40 @@ describe('SessionStore', () => { expect(forked.deriveMessages()).toEqual(a.deriveMessages()) }) + it('enter() rejects a stale prepared session whose id is already live (no overwrite)', async () => { + // prepare()/enter() are public cross-package primitives that a caller may + // separate with arbitrary work. A stale prepared session must NOT overwrite + // a live store entry of the same id — its detach disposer would later delete + // the REAL session, breaking the store-uniqueness invariant. + const ctx = new Context() + await ctx.plugin(SessionStore) + const stale = ctx.sessions.prepare('racy') + const live = ctx.sessions.create('racy') + expect(() => ctx.sessions.enter(stale)).toThrow(/already exists/) + // The live session is intact and still the store entry. + expect(ctx.sessions.get('racy')).toBe(live) + }) + + it('prepare() + enter() + announce() register a session and emit session/created', async () => { + const ctx = new Context() + await ctx.plugin(SessionStore) + const created: Session[] = [] + ctx.on('session/created', session => void created.push(session)) + + const session = ctx.sessions.prepare('lifecycle') + // prepare alone does NOT enter the store. + expect(ctx.sessions.get('lifecycle')).toBeUndefined() + const detach = ctx.sessions.enter(session) + expect(ctx.sessions.get('lifecycle')).toBe(session) + // enter does NOT announce. + expect(created).toEqual([]) + ctx.sessions.announce(session) + expect(created).toEqual([session]) + // The detach disposer removes the entry + stops notification. + detach() + expect(ctx.sessions.get('lifecycle')).toBeUndefined() + }) + it('synthesizes a minimal v1 header for a bare-created session', async () => { const ctx = new Context() await ctx.plugin(SessionStore)