From 7792347c4f2f65afe1136ea1dc3eb852c59c2786 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 02:17:27 +0800 Subject: [PATCH 1/5] simplify(seams): prune dead methods from the persistence and bash seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two capability seams carried abstract methods no production consumer calls. A method no consumer programs against is not a seam — it is speculative surface every implementation must still provide and test. - SessionPersistence: remove has() and delete(), the coordinator's has/delete/deleteCore, and the PersistenceBackend.deleteStored hook (with its jsonl + sqlite + in-spec memory-stub impls). Surviving service surface: create/append/load/list. Production uses only load() (resume) and list() (ACP session/list). - BashExecutor: remove get(id) and list(), the abstract decls and the LocalBashExecutor impls. The internal tasks map survives (it backs ownerOf/readOutput/kill); get/list were pure public accessors over it with no shipping caller and no bash_list tool. - Migrate tests that reached through ctx.bash.get(id) to the public completion seam: a doneFor(id) helper over onTaskDone awaits a task by id, and the HMR-reload ownership test now proves task survival through A's own bash_output ([status: running]) plus ownerOf + B-rejection — a stronger through-the-tool assertion than the removed lookup peek. - Update seam READMEs (six -> four service methods, drop the deleteStored hook and the get/list row) and the two implemented persistence RFCs in place. Implements docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md --- docs/cordis-catalog/events-and-services.md | 4 -- docs/rfc/README.md | 2 +- .../2026-06-14-session-persistence.md | 2 +- ...18-shared-persistence-write-coordinator.md | 12 ++--- .../2026-06-20-prune-dead-seam-methods.md | 2 +- packages/bash/bash-local/src/index.ts | 8 --- .../bash/bash-local/tests/executor.spec.ts | 6 +-- packages/bash/bash/README.md | 1 - packages/bash/bash/src/index.ts | 6 --- packages/bash/bash/tests/service.spec.ts | 10 ---- .../bash/tool-bash/tests/integration.spec.ts | 12 +++-- packages/bash/tool-bash/tests/tools.spec.ts | 54 +++++++++++-------- .../session-persistence-jsonl/README.md | 2 +- .../session-persistence-jsonl/src/index.ts | 22 ++------ .../tests/jsonl.spec.ts | 42 ++++++--------- .../session-persistence-sqlite/README.md | 4 +- .../session-persistence-sqlite/src/index.ts | 18 +------ .../session-persistence-sqlite/src/schema.ts | 2 +- .../tests/sqlite.spec.ts | 4 +- .../session-persistence/README.md | 7 ++- .../session-persistence/src/coordinator.ts | 43 +++------------ .../session-persistence/src/index.ts | 8 +-- .../session-persistence/tests/contract.ts | 20 +------ .../tests/coordinator-contract.ts | 13 +---- .../tests/persistence.spec.ts | 12 ----- 25 files changed, 92 insertions(+), 224 deletions(-) rename docs/rfc/{proposed => implemented}/simplification/2026-06-20-prune-dead-seam-methods.md (99%) diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index 231c7adc83..e8445b4786 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -327,9 +327,7 @@ Semantics every implementation must honor: abstract resolve(request: BashExecRequest): BashExecSpec abstract run(spec: BashExecSpec): Promise abstract start(spec: BashExecSpec): BashTask -abstract get(id: string): BashTask | undefined abstract ownerOf(id: string): string | undefined -abstract list(): BashTask[] abstract readOutput(id: string): BashTaskRead abstract kill(id: string): boolean onTaskDone(listener: BashTaskListener): () => void @@ -369,8 +367,6 @@ abstract create(meta: SessionHeader): Promise abstract append(id: SessionId, events: readonly SessionEvent[]): Promise abstract load(id: SessionId): Promise<{ meta: SessionHeader; events: SessionEvent[] }> abstract list(): Promise -abstract has(id: SessionId): Promise -abstract delete(id: SessionId): Promise ``` Types: [SessionEvent](../core-data-structures/core.md) diff --git a/docs/rfc/README.md b/docs/rfc/README.md index 21036c5769..be22e0c84b 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -52,7 +52,6 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Unify the agent id and the session id](proposed/simplification/2026-06-20-unify-agent-and-session-id.md) | 2026-06-20 | | [Stop mirroring durable boundaries as agent events](proposed/simplification/2026-06-20-remove-agent-boundary-mirror-events.md) | 2026-06-20 | | [Keep one public stop primitive](proposed/simplification/2026-06-20-public-agent-stop-surface.md) | 2026-06-20 | -| [Prune dead methods from the persistence and bash seams](proposed/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | | [Fold trace-only session facts into load-bearing events](proposed/simplification/2026-06-20-collapse-trace-only-session-events.md) | 2026-06-20 | ### Architecture @@ -96,6 +95,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Drop the mutable session summary](implemented/simplification/2026-06-19-drop-mutable-session-summary.md) | 2026-06-19 | | [Drop unconsumed assembled LLM convenience surfaces](implemented/simplification/2026-06-20-drop-unconsumed-llm-assembled-surfaces.md) | 2026-06-20 | | [Drop the unconsumed `llm/adapter-change` event](implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md) | 2026-06-20 | +| [Prune dead methods from the persistence and bash seams](implemented/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | ### Architecture diff --git a/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md b/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md index 9bcd1f8c6f..a5cbd3bd61 100644 --- a/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md +++ b/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md @@ -16,7 +16,7 @@ The [event-sourced model](2026-06-11-event-sourced-sessions.md) makes the append Persistence is an abstract **capability seam** ([capability seams](2026-06-13-capability-seams.md), the `dsh-bash` template), not loop or core logic: -1. **Interface** (`dsh-session-persistence`, `ctx.sessionPersistence`) — an abstract `SessionPersistence` service: `create`/`append`/`load`/`list`/`has`/`delete`. Its persisted unit IS the existing `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type. +1. **Interface** (`dsh-session-persistence`, `ctx.sessionPersistence`) — an abstract `SessionPersistence` service: `create`/`append`/`load`/`list`. Its persisted unit IS the existing `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type. 2. **Implementation** (`dsh-session-persistence-jsonl`) — an append-only JSONL log per session (a `SessionHeader` line then one `SessionEvent` per line, verbatim **including `assistant/chunk`**). Key choices recorded here because they are durable, contested, and surprising: diff --git a/docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md b/docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md index 59ee9bb7c9..44ae67f872 100644 --- a/docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md +++ b/docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md @@ -4,24 +4,24 @@ Status: implemented (proposed and accepted 2026-06-18, implemented 2026-06-20) ## Problem -`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration was duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards had already moved into the seam package; the remaining orchestration was still correctness-heavy and received the same fixes twice. A code-level diff showed the two backends were byte-identical — or same-algorithm — for ALL of it: the four maps (`states`/`buffers`/`chains`/`inits`), `installWritePath`, `initFor`, `onCreated`'s four cases, `flush`, `drain`, `serialize`, `adopt`, `adoptLivePrefix`, `assertVersion`, and the `create`/`append`/`load`/`has`/`delete` skeletons. Only the storage primitives (write bytes vs. INSERT rows) differed. +`dsh-session-persistence-jsonl` and `dsh-session-persistence-sqlite` intentionally prove the same `SessionPersistence` contract over different storage media, but their write-path orchestration was duplicated: per-session state, `session/created` adoption, backend-specific prefix reads, write-behind buffers, serialized flush chains, HMR seeding, and dispose drains. The pure seed-prefix collision and serializability guards had already moved into the seam package; the remaining orchestration was still correctness-heavy and received the same fixes twice. A code-level diff showed the two backends were byte-identical — or same-algorithm — for ALL of it: the four maps (`states`/`buffers`/`chains`/`inits`), `installWritePath`, `initFor`, `onCreated`'s four cases, `flush`, `drain`, `serialize`, `adopt`, `adoptLivePrefix`, `assertVersion`, and the `create`/`append`/`load` skeletons. Only the storage primitives (write bytes vs. INSERT rows) differed. ## Decision -Extract a backend-agnostic `PersistenceCoordinator` into `dsh-session-persistence`. The coordinator owns the orchestration once; each first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements a small `PersistenceBackend` hook interface, and delegates its six public service methods (`create`/`append`/`load`/`list`/`has`/`delete`) to it. +Extract a backend-agnostic `PersistenceCoordinator` into `dsh-session-persistence`. The coordinator owns the orchestration once; each first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements a small `PersistenceBackend` hook interface, and delegates its four public service methods (`create`/`append`/`load`/`list`) to it. Composition, not inheritance. The coordinator is a concrete class the backend holds, not a base class the backend extends. The RFC's risk — "a coordinator must not make unusual backends fight an inheritance hierarchy" — is avoided: a backend exposes only the hooks; it cannot reach the coordinator's private orchestration state, and the public `SessionPersistence` service shape is unchanged, so a third-party backend MAY still implement the abstract service directly without the coordinator at all. ### The hook interface (`PersistenceBackend`) -Seven methods (six required + an optional lifecycle hook) — the only seam between the coordinator and storage: +Six methods (five required + an optional lifecycle hook) — the only seam between the coordinator and storage: - `name` — backend label for the dispose-failure `AggregateError`. -- `loadStored(id)` — read a stored prefix by id, scanning ANY storage scope (every JSONL cwd bucket; SQLite's id is globally unique). Used by resume/load and, via `!== undefined`, the create-collision probe and `has`. +- `loadStored(id)` — read a stored prefix by id, scanning ANY storage scope (every JSONL cwd bucket; SQLite's id is globally unique). Used by resume/load and, via `!== undefined`, the create-collision probe. - `loadLive(id, cwd)` — read a stored prefix SCOPED to `cwd`. **Deliberately distinct from `loadStored`**: HMR live-adoption must only adopt a persisted log at the SAME cwd as the live session; a same-id log at a different cwd is a collision, not a resume. Collapsing the two reintroduces a cross-cwd adoption bug. SQLite ignores `cwd`. - `appendBatch(meta, events, isMaterialized)` — durably append a contiguous batch, lazily materializing the session ATOMICALLY when not yet materialized (the materialize-write and the first event batch must commit together — a crash between them must not leave a materialized-but-empty session; this is why there is no separate `materialize` hook). - `commitRepair(meta, tornMarker, closers)` — make a crash repair durable: truncate the torn tail (iff `tornMarker !== undefined`) and append `closers`. **NOT required to be atomic** — JSONL legitimately truncates-then-appends in two fsync'd steps, SQLite does DELETE+INSERT in one transaction. Used by `load` (truncate + synthetic closers) and live-adoption (truncate only, `closers = []`). -- `deleteStored(id)` / `list()` — remove a stored artifact / list all stored metadata. +- `list()` — list all stored metadata. - `close?()` — optional lifecycle teardown (SQLite closes its db handle; JSONL omits it), awaited in the dispose effect AFTER the quiescence drain so a close failure never masks a drain error. ### The opaque torn marker @@ -34,4 +34,4 @@ The shared `runPersistenceContract` (public-API contract) keeps running for ever ## Risks and what we gave up -The pre-extraction duplication was verbose but explicit — each backend read top-to-bottom. The coordinator adds one indirection (the hook seam) and one new concept (the opaque torn marker). This clears the bar because the centralized logic is the correctness-heavy part that was already being fixed twice, and the hook set is narrow (seven methods, no inheritance). The hook surface was deliberately held to the minimum: `has` and the create-collision probe are NOT separate hooks — they fold into `loadStored(id) !== undefined`; there is no separate `materialize` hook (folded into `appendBatch` for atomicity); `list()` stays a backend method with no coordinator pass-through (listing needs none of the orchestration). The net effect is a reduction: one orchestration copy instead of two, the backends shrank by ~1200 lines of duplicated churn, and a future backend implements ~7 small primitives instead of copying the entire `session/event` → buffer → flush machinery. +The pre-extraction duplication was verbose but explicit — each backend read top-to-bottom. The coordinator adds one indirection (the hook seam) and one new concept (the opaque torn marker). This clears the bar because the centralized logic is the correctness-heavy part that was already being fixed twice, and the hook set is narrow (six methods, no inheritance). The hook surface was deliberately held to the minimum: the create-collision probe is NOT a separate hook — it folds into `loadStored(id) !== undefined`; there is no separate `materialize` hook (folded into `appendBatch` for atomicity); `list()` stays a backend method with no coordinator pass-through (listing needs none of the orchestration). The net effect is a reduction: one orchestration copy instead of two, the backends shrank by ~1200 lines of duplicated churn, and a future backend implements a handful of small primitives instead of copying the entire `session/event` → buffer → flush machinery. diff --git a/docs/rfc/proposed/simplification/2026-06-20-prune-dead-seam-methods.md b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md similarity index 99% rename from docs/rfc/proposed/simplification/2026-06-20-prune-dead-seam-methods.md rename to docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md index 117fe9c72b..ec9dc45223 100644 --- a/docs/rfc/proposed/simplification/2026-06-20-prune-dead-seam-methods.md +++ b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md @@ -1,6 +1,6 @@ # RFC: Prune dead methods from the persistence and bash capability seams -Status: proposed +Status: implemented (proposed and accepted 2026-06-20) ## Problem diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index df6e2285a9..7b55200cf8 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -176,20 +176,12 @@ export class LocalBashExecutor extends BashExecutor { return task } - get(id: string): BashTask | undefined { - return this.tasks.get(id) - } - ownerOf(id: string): string | undefined { // Unknown id and known-but-ownerless both read as undefined — the consumer // treats undefined as "open" and a truly unknown id fails at readOutput/kill. return this.tasks.get(id)?.owner } - list(): BashTask[] { - return [...this.tasks.values()] - } - readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) diff --git a/packages/bash/bash-local/tests/executor.spec.ts b/packages/bash/bash-local/tests/executor.spec.ts index 3dd7f7983a..ec90aeb77e 100644 --- a/packages/bash/bash-local/tests/executor.spec.ts +++ b/packages/bash/bash-local/tests/executor.spec.ts @@ -98,8 +98,6 @@ describe('LocalBashExecutor background tasks', () => { const task = bash.start(bash.resolve({ command: 'sleep 0.2; echo done' })) expect(Date.now() - before).toBeLessThan(150) expect(task.status).toBe('running') - expect(bash.get(task.id)).toBe(task) - expect(bash.list()).toContain(task) await task.done expect(task.status).toBe('completed') expect(task.exitCode).toBe(0) @@ -237,7 +235,6 @@ describe('LocalBashExecutor background tasks', () => { await running.done expect(finished.status).toBe('completed') expect(running.signal).toBe('SIGTERM') - expect(bash.list()).toEqual([]) }) it('disposing the executor fiber kills running tasks (no orphans)', async () => { @@ -249,14 +246,13 @@ describe('LocalBashExecutor background tasks', () => { bash.onTaskDone(listener) const task = bash.start(bash.resolve({ command: 'sleep 60' })) - const running = bash.get(task.id)! + const running = task await new Promise(resolve => setTimeout(resolve, 50)) // Grab the pid before dispose clears the registry. const pid = (running as unknown as { running: { pid: number } }).running.pid await fiber.dispose() await waitGone(pid) - expect(bash.list()).toEqual([]) // Listener silenced by base-class teardown — no late notifications. expect(listener).not.toHaveBeenCalled() }) diff --git a/packages/bash/bash/README.md b/packages/bash/bash/README.md index ce8816dee7..c982a217c7 100644 --- a/packages/bash/bash/README.md +++ b/packages/bash/bash/README.md @@ -18,7 +18,6 @@ The split mirrors the LLM seam (`LlmService`/`LlmAdapter`) and the agent-tool su |---|---| | `run(spec)` | Foreground execution. Resolves when the command finishes. **Rejects only for infrastructure failures** (unusable workdir, missing shell, pre-aborted signal); nonzero exits, timeout kills, and abort kills resolve with a descriptive `BashRunResult`. | | `start(spec)` | Background execution. Returns a `BashTask` handle immediately; **no timeout applies** (stop tasks via `kill`). | -| `get(id)` / `list()` | Task lookup. | | `ownerOf(id)` | The opaque OWNER token recorded for a background task at `start` (from the spec's `owner`), or `undefined` for an unknown id OR a known-but-ownerless task. The executor stores/returns it verbatim and NEVER interprets it — the access POLICY lives in the consumer (`dsh-tool-bash`), which compares `ownerOf(id)` to the caller's token. Storing ownership here (disposed with the executor's fiber) is what makes it survive a consumer HMR reload. | | `readOutput(id)` | **Incremental** output read — consecutive reads never re-deliver. Reads that lost data to buffer bounds flag `lossy` and point at full-stream spill files. Throws for unknown ids. | | `kill(id)` | Kill a running task. Returns `false` when it already finished; throws for unknown ids. | diff --git a/packages/bash/bash/src/index.ts b/packages/bash/bash/src/index.ts index f4e2d964fe..9df8c720aa 100644 --- a/packages/bash/bash/src/index.ts +++ b/packages/bash/bash/src/index.ts @@ -85,9 +85,6 @@ export abstract class BashExecutor extends Service { /** Start a background task and return its handle immediately. */ abstract start(spec: BashExecSpec): BashTask - /** Look up a background task by id. */ - abstract get(id: string): BashTask | undefined - /** * The opaque OWNER token recorded for a background task at {@link start} * (from the {@link BashExecSpec}'s `owner`), or `undefined` for an unknown id @@ -103,9 +100,6 @@ export abstract class BashExecutor extends Service { */ abstract ownerOf(id: string): string | undefined - /** All tracked background tasks (insertion order). */ - abstract list(): BashTask[] - /** Read output produced since the previous read. Throws for unknown ids. */ abstract readOutput(id: string): BashTaskRead diff --git a/packages/bash/bash/tests/service.spec.ts b/packages/bash/bash/tests/service.spec.ts index 4b28bb72be..2646715df9 100644 --- a/packages/bash/bash/tests/service.spec.ts +++ b/packages/bash/bash/tests/service.spec.ts @@ -44,18 +44,10 @@ class StubExecutor extends BashExecutor { return task } - get(id: string): BashTask | undefined { - return this.tasks.get(id) - } - ownerOf(id: string): string | undefined { return this.owners.get(id) } - list(): BashTask[] { - return [...this.tasks.values()] - } - readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) @@ -88,8 +80,6 @@ describe('BashExecutor service seam', () => { it('registers as ctx.bash and serves the abstract API', async () => { const { bash } = await setup() const task = bash.start(bash.resolve({ command: 'sleep 1' })) - expect(bash.get(task.id)).toBe(task) - expect(bash.list()).toEqual([task]) expect(bash.kill(task.id)).toBe(true) expect(bash.kill(task.id)).toBe(false) const result = await bash.run(bash.resolve({ command: 'true' })) diff --git a/packages/bash/tool-bash/tests/integration.spec.ts b/packages/bash/tool-bash/tests/integration.spec.ts index 0ab786ca85..fadcbf741d 100644 --- a/packages/bash/tool-bash/tests/integration.spec.ts +++ b/packages/bash/tool-bash/tests/integration.spec.ts @@ -8,6 +8,7 @@ import ToolRegistry from '@deepseek-ai/dsh-tools' import AgentRegistry from '@deepseek-ai/dsh-agent' import AgentLoop, { ReactLoopAgent } from '@deepseek-ai/dsh-agent-loop' import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local' +import type { BashTask } from '@deepseek-ai/dsh-bash' import * as ToolBash from '@deepseek-ai/dsh-tool-bash' import { MockAdapter, textResponse, toolCallResponse } from '../../../core/agent-loop/tests/mock-adapter.ts' @@ -143,13 +144,18 @@ describe('bash tool through the agent loop', () => { return next() }) + // Capture the single background task's completion. Registered BEFORE send so + // a fast task (echo) can't finish before the listener is attached; onTaskDone + // delivers the task object once it completes (completion may race turn end). + const taskDone = new Promise((resolve) => { + const dispose = ctx.bash.onTaskDone((task) => { dispose(); resolve(task) }) + }) + agent.send([{ type: 'text', text: 'run echo bg-ok in the background' }]) await waitForIdle(ctx, agent) // Wait for the background task itself (completion may race turn end). - const task = ctx.bash.get(taskId) - if (!task) throw new Error(`task ${taskId} not registered`) - await task.done + await taskDone const log = events(agent) const firstResult = findEvent(log, 'tool/result') diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index c49410a9b2..d8d166ea5b 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -66,6 +66,23 @@ function text(result: { content: { type: string; text?: string }[] }): string { return result.content.filter(block => block.type === 'text').map(block => block.text).join('') } +/** + * Resolve once the background task with `id` completes. The task is started + * indirectly (via `ctx.tools.execute`), so `start()`'s return is not accessible + * here; the executor's `onTaskDone` listener delivers the SAME task object on + * completion, which is the surviving seam for awaiting a task by id. + */ +function doneFor(ctx: Context, id: string): Promise { + return new Promise((resolve) => { + const dispose = ctx.bash.onTaskDone((task) => { + if (task.id === id) { + dispose() + resolve(task) + } + }) + }) +} + class LossyReadBashExecutor extends BashExecutor { private readonly task: BashTask = { id: 'bash-lossy', @@ -94,18 +111,10 @@ class LossyReadBashExecutor extends BashExecutor { return this.task } - get(id: string): BashTask | undefined { - return id === this.task.id ? this.task : undefined - } - ownerOf(): string | undefined { return undefined } - list(): BashTask[] { - return [this.task] - } - readOutput(id: string): BashTaskRead { if (id !== this.task.id) throw new Error(`unknown bash task "${id}"`) return { task: this.task, delta: 'tail', lossy: true } @@ -286,7 +295,7 @@ describe('background tools', () => { expect(text(first)).toContain('first') expect(text(first)).toContain('[status: running]') - await ctx.bash.get(id)!.done + await doneFor(ctx, id) const second = await call(ctx, 'bash_output', { task_id: id }) expect(text(second)).toContain('second') expect(text(second)).not.toContain('first') @@ -306,7 +315,7 @@ describe('background tools', () => { const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await ctx.bash.get(id)!.done + await doneFor(ctx, id) const read = await call(ctx, 'bash_output', { task_id: id }) expect(text(read)).toContain('[some output was dropped from memory; full output: ') }) @@ -329,7 +338,7 @@ describe('background tools', () => { const killed = await call(ctx, 'bash_kill', { task_id: id }) expect(text(killed)).toBe(`killed background task ${id}`) - await ctx.bash.get(id)!.done + await doneFor(ctx, id) const again = await call(ctx, 'bash_kill', { task_id: id }) expect(text(again)).toBe(`task ${id} had already finished`) @@ -373,7 +382,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await ctx.bash.get(id)!.done + await doneFor(ctx, id) expect(inject).toHaveBeenCalledTimes(1) const [content, options] = inject.mock.calls[0] as [ @@ -396,7 +405,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() + await expect(doneFor(ctx, id)).resolves.toBeDefined() }) it('rethrows a non-disposed inject failure (not blindly swallowed)', async () => { @@ -415,7 +424,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await ctx.bash.get(id)!.done + await doneFor(ctx, id) // notifyTaskDone caught and logged the rethrown error. expect(errorSpy).toHaveBeenCalled() const logged = errorSpy.mock.calls.flat().some(arg => arg instanceof Error && arg.message === 'unexpected inject bug') @@ -443,7 +452,7 @@ describe('background tools', () => { const id = /task (bash-\d+)/.exec(text(started))![1]! // Unregister the agent BEFORE the task completes (simulate disconnect). unregisterFakeAgents(ctx) - await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() + await expect(doneFor(ctx, id)).resolves.toBeDefined() expect(inject).not.toHaveBeenCalled() }) @@ -451,7 +460,7 @@ describe('background tools', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() + await expect(doneFor(ctx, id)).resolves.toBeDefined() }) }) @@ -534,7 +543,7 @@ describe('background task ownership (cross-session isolation)', () => { const b = fakeAgent('sess-b') const started = await callAs(ctx, a, 'bash', { command: 'echo done', description: 'bg', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await ctx.bash.get(id)!.done + await doneFor(ctx, id) // Completion does NOT clear ownership: B is still rejected, A still allowed. const readByB = await callAs(ctx, b, 'bash_output', { task_id: id }) expect(readByB.isError).toBe(true) @@ -567,7 +576,9 @@ describe('background task ownership (cross-session isolation)', () => { // token) survive. await fiber.dispose() await ctx.plugin(ToolBash) - expect(ctx.bash.get(id)?.status).toBe('running') + // The task survived the reload, still running and still owned by A — proven + // via A's own bash_output (reports running status) and the surviving owner token. + expect(text(await callAs(ctx, a, 'bash_output', { task_id: id }))).toContain('[status: running]') expect(ctx.bash.ownerOf(id)).toBe('sess-a') // After reload, ownership is INTACT → B is STILL rejected. @@ -675,10 +686,10 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'sleep 60', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const task = ctx.bash.get(id)! + const done = doneFor(ctx, id) await call(ctx, 'bash_kill', { task_id: id }) - await task.done + const task = await done // Simulate the variant where the close event carried no signal. task.signal = null const read = await call(ctx, 'bash_output', { task_id: id }) @@ -689,8 +700,7 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const task = ctx.bash.get(id)! - await task.done + const task = await doneFor(ctx, id) // Defensive: completed tasks always carry an exit code in practice; the // ?? 0 fallback covers task shapes from other executor implementations. task.exitCode = null diff --git a/packages/session-persistence/session-persistence-jsonl/README.md b/packages/session-persistence/session-persistence-jsonl/README.md index 28a64c4c11..54514755a3 100644 --- a/packages/session-persistence/session-persistence-jsonl/README.md +++ b/packages/session-persistence/session-persistence-jsonl/README.md @@ -21,7 +21,7 @@ The JSONL durable session-persistence backend — a concrete `SessionPersistence ## Durability and crash semantics -- **Lazy materialization.** `create(meta)` writes nothing; the `.jsonl` (header + first batch) is written atomically (temp-write + `fsync` + rename) on the first `append`. A created-but-never-appended session leaves nothing on disk and is absent from `has`/`list`. +- **Lazy materialization.** `create(meta)` writes nothing; the `.jsonl` (header + first batch) is written atomically (temp-write + `fsync` + rename) on the first `append`. A created-but-never-appended session leaves nothing on disk and is absent from `list`. - **Append-only.** Committed events (at or below a flushed `turn/end`) are never rewritten. Subsequent appends are line appends at EOF + `fsync`. - **Crash recovery — close, don't truncate.** A crash can leave a log whose final turn never closed (real events after the last `turn/end`). `load` PRESERVES those events (a turn can be huge — they are real work) and closes the orphaned turn by durably appending synthetic boundary events: an error `tool/result` for every `tool-call` the crash left unanswered (the loop logs the assistant message before running the tools, so a mid-tool crash leaves dangling calls — and `deriveMessages()` would replay an assistant tool-call with no result, which providers reject), then a `step/end` if a step was open, then `turn/end {kind:'interrupted'}`, returning a balanced log. Only a never-fully-written **torn tail fragment** (a final line with no newline / unparseable) is `ftruncate`d away before the closers are written. See [session persistence](../../../docs/rfc/implemented/architecture/2026-06-14-session-persistence.md). - **Contiguous-seq.** `load` rejects a mid-log parse error or `seq` gap (unloadable); `append` rejects a batch whose first `seq` does not continue the stored log, and rejects non-JSON-serializable `event.data` naming the offending event type. diff --git a/packages/session-persistence/session-persistence-jsonl/src/index.ts b/packages/session-persistence/session-persistence-jsonl/src/index.ts index 76df3f3ccb..6de7eafeb3 100644 --- a/packages/session-persistence/session-persistence-jsonl/src/index.ts +++ b/packages/session-persistence/session-persistence-jsonl/src/index.ts @@ -11,7 +11,7 @@ * (the `session/event` → buffer → `session/flush` drain, per-session * serialization, write cursors, fork-seed persistence, HMR live-adoption, * crash-repair sequencing, dispose quiescence) lives in the backend-agnostic - * {@link PersistenceCoordinator} this class composes. The six public + * {@link PersistenceCoordinator} this class composes. The four public * {@link SessionPersistence} methods delegate to the coordinator. * * @module @deepseek-ai/dsh-session-persistence-jsonl @@ -101,14 +101,6 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi return this.coordinator.load(id) } - has(id: SessionId): Promise { - return this.coordinator.has(id) - } - - delete(id: SessionId): Promise { - return this.coordinator.delete(id) - } - // `list` is BOTH the public service method and the PersistenceBackend hook — // one method, the bucket walk below. The coordinator adds no orchestration for // listing (no per-id serialization, no cursor), so it would just call back into @@ -180,12 +172,6 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi if (closers.length > 0) await this.appendLines(meta, closers) } - /** Remove a session's log file (the coordinator clears its in-memory state). */ - async deleteStored(id: SessionId): Promise { - const file = await this.findLog(id) - if (file) await rm(file.path, { force: true }) - } - /** List all stored sessions' metadata (header line only — no full-log parse). */ async list(): Promise { const metas: SessionHeader[] = [] @@ -341,9 +327,9 @@ export class SessionPersistenceJsonl extends SessionPersistence implements Persi /** * Find a session's log file by id across ALL cwd buckets — the any-cwd scan - * for `loadStored`/`deleteStored` (resume and removal identify a session by id - * alone). The cwd-scoped lookup (`loadLive`) does NOT use this; it goes - * straight to `logPath(cwd)` so a no-cwd session can't match a real-cwd bucket. + * for `loadStored` (resume identifies a session by id alone). The cwd-scoped + * lookup (`loadLive`) does NOT use this; it goes straight to `logPath(cwd)` so + * a no-cwd session can't match a real-cwd bucket. */ private async findLog(id: SessionId): Promise<{ path: string; cwd: string | undefined } | undefined> { const target = encodeSegment(id) + '.jsonl' diff --git a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts index d36723f396..8acc578521 100644 --- a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts @@ -105,12 +105,12 @@ describe('SessionPersistenceJsonl: durability and crash semantics', () => { // nothing on disk yet const dir = sessionDir(root, '/work') await expect(stat(logPath(root, '/work', m.id))).rejects.toThrow() - expect(await ctx.sessionPersistence.has(m.id)).toBe(false) + expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id) await ctx.sessionPersistence.append(m.id, oneTurnLog()) // now materialized expect((await stat(logPath(root, '/work', m.id))).isFile()).toBe(true) - expect(await ctx.sessionPersistence.has(m.id)).toBe(true) + expect((await ctx.sessionPersistence.list()).map(h => h.id)).toContain(m.id) void dir }) @@ -448,19 +448,6 @@ describe('SessionPersistenceJsonl: edge cases', () => { expect(ids).toContain('big') }) - it('has() finds a session on disk under an unknown cwd (cross-bucket scan)', async () => { - const m = meta('scan-me', '/somewhere') - await ctx.sessionPersistence.create(m) - await ctx.sessionPersistence.append(m.id, oneTurnLog()) - // A fresh backend with no in-memory state → has() must scan disk buckets. - const ctx2 = new Context() - await ctx2.plugin(SessionStore) - await ctx2.plugin(SessionPersistenceJsonl, { root }) - expect(await ctx2.sessionPersistence.has(m.id)).toBe(true) - expect(await ctx2.sessionPersistence.has(SessionId('absent'))).toBe(false) - await ctx2.fiber.dispose() - }) - it('a DIFFERENT live session object reusing a disposed id gets its own init (no stale cache)', async () => { // Session A materializes a log under id "reuse". const sessFiberA = await ctx.plugin(Object.assign((inner: Context) => { @@ -579,20 +566,23 @@ describe('SessionPersistenceJsonl: edge cases', () => { await ctx2.fiber.dispose() }) - it('exists() surfaces a non-ENOENT lookup error (ENOTDIR) instead of reporting absent', async () => { - // Same contract on the existence path: a non-ENOENT error from the per-id - // open() must surface, not be collapsed to "not found" (which would let a - // collision check proceed under a false absence assumption). A LAZY session - // (created, never appended) keeps its cwd in state, so has() reaches - // loadLive(id, cwd) → exists(logPath). Make that cwd's bucket DIRECTORY a - // regular file: open()ing `bucket/.jsonl` under it then fails ENOTDIR. + it('loadLive surfaces a non-ENOENT lookup error (ENOTDIR) instead of reporting absent', async () => { + // A non-ENOENT error from the per-id open() must surface, not be collapsed to + // "not found" (which would let live-adoption proceed under a false absence + // assumption). A live session's onCreated reaches loadLive(id, cwd) → + // exists(logPath). Make that cwd's bucket DIRECTORY a regular file: open()ing + // `bucket/.jsonl` under it then fails ENOTDIR. const cwd = '/x' const ctx2 = new Context() await ctx2.plugin(SessionStore) await ctx2.plugin(SessionPersistenceJsonl, { root }) - await ctx2.sessionPersistence.create(meta('exists-fault', cwd)) // lazy: no bucket yet await writeFile(sessionDir(root, cwd), 'x') // bucket path is now a FILE - await expect(ctx2.sessionPersistence.has(SessionId('exists-fault'))).rejects.toThrow(/ENOTDIR/) + const backend = ctx2.sessionPersistence as unknown as { inits: Map> } + let s!: Session + await ctx2.plugin(Object.assign((inner: Context) => { + s = inner.sessions.create('exists-fault', { meta: { cwd } }) + }, { inject: ['sessions'] })) + await expect(backend.inits.get(s)).rejects.toThrow(/ENOTDIR/) await ctx2.fiber.dispose() }) @@ -687,7 +677,7 @@ describe('SessionPersistenceJsonl: edge cases', () => { circ.self = circ await expect(ctx.sessionPersistence.append(m.id, bad(circ))).rejects.toThrow(/non-JSON-serializable/) // The session was never materialized by any of the rejected appends. - expect(await ctx.sessionPersistence.has(m.id)).toBe(false) + expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id) }) it('accepts well-formed JSON values (null, booleans, nested arrays/objects)', async () => { @@ -695,7 +685,7 @@ describe('SessionPersistenceJsonl: edge cases', () => { await ctx.sessionPersistence.create(m) const ev = [{ type: 'user/message', seq: 0, time: 1, data: { content: [{ type: 'text', text: 'x' }], source: { kind: 'user' }, extra: { a: null, b: true, c: [1, 2, { d: 'nested' }] } } }] as unknown as SessionEvent[] await ctx.sessionPersistence.append(m.id, ev) - expect(await ctx.sessionPersistence.has(m.id)).toBe(true) + expect((await ctx.sessionPersistence.list()).map(h => h.id)).toContain(m.id) }) it('Session.append rejects a non-serializable event at the source (never enters the log)', () => { diff --git a/packages/session-persistence/session-persistence-sqlite/README.md b/packages/session-persistence/session-persistence-sqlite/README.md index 23916f2bfe..02255c024f 100644 --- a/packages/session-persistence/session-persistence-sqlite/README.md +++ b/packages/session-persistence/session-persistence-sqlite/README.md @@ -13,8 +13,8 @@ The repo targets Node ≥ 24 (the root `engines` field), which includes the stab ## Contract semantics over rows - **Append = a transaction.** `append` runs `BEGIN`/`COMMIT` around the batch: it materializes the `sessions` row (if still lazy) and INSERTs every event, asserting the contiguous-seq contract first (the first event's `seq` must equal the stored next-seq). A mid-batch failure (a UNIQUE violation on a duplicated seq) rolls back entirely, so the stored log and the in-memory cursor stay consistent. (`load()` already balanced the stored log, so `append` never has to repair a crash tail.) -- **Lazy materialization.** `create()` records intent in memory only — no row is written until the first `append`. A created-but-never-appended session has no `sessions` row, so it is absent from `has()`/`list()` (which report exactly the sessions that have a row). -- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (an error `tool/result` for every assistant tool call left unanswered, a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `has()`/`list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`. +- **Lazy materialization.** `create()` records intent in memory only — no row is written until the first `append`. A created-but-never-appended session has no `sessions` row, so it is absent from `list()` (which reports exactly the sessions that have a row). +- **Interrupted-turn close on load.** `load()` reads every stored event ordered by `seq` and finds the longest seq-contiguous, parseable prefix — INCLUDING the real events of an interrupted final turn after the last `turn/end` (the loop only flushes at `turn/end`, so a process killed mid-turn leaves real, fully-written rows past it). A single turn can be huge in a long-horizon task, so those events are **preserved, never truncated**: `load()` CLOSES the orphaned turn by durably appending the minimal synthetic boundary events (an error `tool/result` for every assistant tool call left unanswered, a `step/end` if a step was open, then a `turn/end` carrying `{ kind: 'interrupted' }`), inside one transaction that also DELETEs any never-fully-written torn tail row. `load()` is therefore mutating — after it the stored rows are balanced and the cursor is truthful, so the next `append` continues cleanly. The boundary (last `turn/end`, torn-tail detection) is computed from the `seq`/`type` columns so a malformed `data` in a torn tail row is never parsed (discarded, not unloadable). A parse error or `seq` gap inside the committed region (at or before the last real `turn/end`) makes the session unloadable. A session whose only turn never closed keeps its metadata row and stays present in `list()` — the same as the JSONL backend, whose file likewise survives a first append that never reached `turn/end`. ## Configuration (schemastery) diff --git a/packages/session-persistence/session-persistence-sqlite/src/index.ts b/packages/session-persistence/session-persistence-sqlite/src/index.ts index 49cf3882d4..cef61cb071 100644 --- a/packages/session-persistence/session-persistence-sqlite/src/index.ts +++ b/packages/session-persistence/session-persistence-sqlite/src/index.ts @@ -11,7 +11,7 @@ * Like the JSONL backend it supplies ONLY the storage primitives (the * {@link PersistenceBackend} hooks below — INSERT/DELETE/SELECT inside * transactions); all the write-path orchestration lives in the backend-agnostic - * {@link PersistenceCoordinator} this class composes. The six public + * {@link PersistenceCoordinator} this class composes. The four public * {@link SessionPersistence} methods delegate to the coordinator. * * @module @deepseek-ai/dsh-session-persistence-sqlite @@ -99,14 +99,6 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers return this.coordinator.load(id) } - has(id: SessionId): Promise { - return this.coordinator.has(id) - } - - delete(id: SessionId): Promise { - return this.coordinator.delete(id) - } - // `list` is BOTH the public service method and the PersistenceBackend hook — // one method (the SELECT below). The coordinator adds no orchestration for // listing, so routing it through the coordinator would just recurse. Defined @@ -203,12 +195,6 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers } } - /** Remove a session's row (ON DELETE CASCADE drops its events). */ - async deleteStored(id: SessionId): Promise { - await this.ready - this.db.prepare('DELETE FROM sessions WHERE id = ?').run(id) - } - /** List all materialized sessions' metadata (every row is a materialized session). */ async list(): Promise { await this.ready @@ -234,7 +220,7 @@ export class SessionPersistenceSqlite extends SessionPersistence implements Pers /** * Insert-or-replace a session's metadata row. The only caller is the first * materializing `appendBatch`, so writing the row IS the materialization (its - * existence is the signal `has`/`list` read). + * existence is the signal `list` reads). */ private writeRow(meta: SessionHeader): void { this.db.prepare(` diff --git a/packages/session-persistence/session-persistence-sqlite/src/schema.ts b/packages/session-persistence/session-persistence-sqlite/src/schema.ts index b6e05a0a3f..8238cba30c 100644 --- a/packages/session-persistence/session-persistence-sqlite/src/schema.ts +++ b/packages/session-persistence/session-persistence-sqlite/src/schema.ts @@ -21,7 +21,7 @@ export const SCHEMA_VERSION = 2 * A row of the `sessions` table — the out-of-log metadata ({@link SessionHeader}). * The row's EXISTENCE is the materialization signal: it is written only by the * first `append` (lazy materialization), so a created-but-never-appended - * session has no row and is absent from `has`/`list`, mirroring the JSONL + * session has no row and is absent from `list`, mirroring the JSONL * backend's "no file until first append". */ export interface SessionRow { diff --git a/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts b/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts index aecfa5665d..262a085ce2 100644 --- a/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts +++ b/packages/session-persistence/session-persistence-sqlite/tests/sqlite.spec.ts @@ -214,17 +214,15 @@ describe('SessionPersistenceSqlite: durability and crash semantics', () => { { type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }, { type: 'user/message', seq: 1, time: 2, data: { content: [{ type: 'text', text: 'hi' }], source: { kind: 'user' } } }, ]) - expect(await b1.ctx.sessionPersistence.has(m.id)).toBe(true) // materialized await b1.dispose() // A fresh backend loads it: the interrupted (only) turn's real events are // preserved and closed with a synthetic turn/end {interrupted} — NOT - // truncated. The session was materialized, so has()/list() report it present. + // truncated. The session was materialized, so list() reports it present. const b2 = await backend(path) const loaded = await b2.ctx.sessionPersistence.load(m.id) expect(loaded.events.map(e => e.type)).toEqual(['turn/start', 'user/message', 'turn/end']) expect(loaded.events.at(-1)!.type === 'turn/end' && loaded.events.at(-1)!.data).toMatchObject({ reason: { kind: 'interrupted' } }) - expect(await b2.ctx.sessionPersistence.has(m.id)).toBe(true) expect((await b2.ctx.sessionPersistence.list()).map(x => x.id)).toContain(m.id) await b2.dispose() }) diff --git a/packages/session-persistence/session-persistence/README.md b/packages/session-persistence/session-persistence/README.md index b21a01b763..928e7b033d 100644 --- a/packages/session-persistence/session-persistence/README.md +++ b/packages/session-persistence/session-persistence/README.md @@ -11,8 +11,7 @@ The persisted unit IS the existing `SessionEvent` (event-sourced model — the l | `create(meta): Promise` | Register a new session's metadata. MAY defer the physical write until the first `append` (lazy materialization). | | `append(id, events): Promise` | Durably persist a batch (from the `session/flush` drain). Append-only; first event `seq` == stored next-seq after any repair; rejects non-JSON-serializable data naming the offending type. | | `load(id): Promise<{ meta; events }>` | Reload meta + log. Preserves an interrupted (unclosed) final turn and closes it with synthetic closers — an error `tool/result` per unanswered `tool-call`, then `step/end?`+`turn/end {interrupted}` (a turn can be huge — never truncated); only a torn tail fragment is dropped. Events contiguous (`events[i].seq === i`); rejects a committed-region gap/parse error or unknown `version`. | -| `list(): Promise` | Lightweight listing from metadata, no full-log parse. | -| `has(id)` / `delete(id)` | Existence / removal. A zero-event lazily-materialized session is absent from `has`/`list`. | +| `list(): Promise` | Lightweight listing from metadata, no full-log parse. A zero-event lazily-materialized session is absent from `list`. | ## Invariants every backend must honor @@ -25,7 +24,7 @@ The persisted unit IS the existing `SessionEvent` (event-sourced model — the l The two first-party backends were byte-identical (or same-algorithm) for ALL of their write-path orchestration — the in-memory bookkeeping (per-id state, write-behind buffers, per-id serialization chains, 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). -`PersistenceCoordinator` owns that orchestration once. A first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements the small `PersistenceBackend` hook interface, and delegates its six public service methods to the coordinator. This keeps the duplicated, correctness-heavy orchestration in a single place (it used to receive the same fixes twice). +`PersistenceCoordinator` owns that orchestration once. A first-party backend composes one (`new PersistenceCoordinator(ctx, this)`), implements the small `PersistenceBackend` hook interface, and delegates its four public service methods to the coordinator. This keeps the duplicated, correctness-heavy orchestration in a single place (it used to receive the same fixes twice). The `PersistenceBackend` hooks (the only seam between the coordinator and storage): @@ -36,7 +35,7 @@ The `PersistenceBackend` hooks (the only seam between the coordinato | `loadLive(id, cwd)` | Read a stored prefix SCOPED to `cwd` (HMR live-adoption must only adopt a log at the SAME cwd; a same-id log elsewhere is a collision, not a resume). A globally-unique-id backend ignores `cwd`. | | `appendBatch(meta, events, isMaterialized)` | Durably append a contiguous batch, lazily materializing ATOMICALLY when not yet materialized. | | `commitRepair(meta, tornMarker, closers)` | Make a crash repair durable: truncate the torn tail (iff `tornMarker !== undefined` — a marker may be falsy, e.g. seq/offset `0`) and append `closers`. NOT required to be atomic. Used by load (truncate + closers) and live-adoption (truncate only). | -| `deleteStored(id)` / `list()` | Remove a stored artifact / list all stored metadata. | +| `list()` | List all stored metadata. | | `close?()` | Optional lifecycle teardown (e.g. close a db handle), awaited after the dispose drain. | The `tornMarker` is fully OPAQUE: the coordinator only tests `!== undefined` and round-trips it to `commitRepair`, never inspecting its value (the JSONL backend uses the byte offset to truncate to, the SQLite backend the seq to delete from). The public `SessionPersistence` service shape is unchanged, so a third-party backend MAY still implement the abstract service directly without the coordinator. See [the write-coordinator RFC](../../../docs/rfc/implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md). diff --git a/packages/session-persistence/session-persistence/src/coordinator.ts b/packages/session-persistence/session-persistence/src/coordinator.ts index ad21f3a8ca..d2f5712502 100644 --- a/packages/session-persistence/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/session-persistence/src/coordinator.ts @@ -14,7 +14,7 @@ * {@link PersistenceBackend} hook object. * * The abstract {@link SessionPersistence} service's public API is independent of - * this: a backend IS a `SessionPersistence` (its six public methods delegate to + * this: a backend IS a `SessionPersistence` (its four public methods delegate to * a coordinator it composes), so a third-party backend MAY implement the service * directly without using the coordinator at all. * @@ -95,9 +95,6 @@ export interface PersistenceBackend { */ commitRepair(meta: SessionHeader, tornMarker: TornMarker | undefined, closers: readonly SessionEvent[]): Promise - /** Remove the stored artifact for `id` (the coordinator clears in-memory state). */ - deleteStored(id: SessionId): Promise - /** List all stored (materialized) sessions' metadata. */ list(): Promise @@ -119,13 +116,12 @@ interface SessionState { * 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 + * transaction (the "a row exists ⇔ it has events" invariant `list` + * relies 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). + * registered-but-never-written from one durably present, which the reclaim + * path needs (an abandoned id with no artifact AND no buffered events is free + * to reuse; a materialized one is a real collision). */ materialized: boolean /** @@ -150,7 +146,7 @@ async function settledErrors(promises: Iterable>): Promise { // through the coordinator would only forward to that same hook, so the // coordinator stays out of the listing path entirely. - /** Whether a session is durably present (materialized). */ - async has(id: SessionId): Promise { - const state = this.states.get(id) - if (state?.materialized) return true - // A TRACKED lazy session has a known cwd: probe that exact bucket via - // loadLive(id, cwd) — including the no-cwd bucket when its cwd is undefined. - // An UNTRACKED id has a genuinely UNKNOWN cwd, so it must scan ANY scope via - // loadStored — loadLive(id, undefined) would (correctly) look ONLY in the - // no-cwd bucket and miss a materialized session that lives in a real cwd. - const probe = state !== undefined - ? await this.backend.loadLive(id, state.meta.cwd) - : await this.backend.loadStored(id) - return probe !== undefined - } - - /** Remove a session and all its persisted artifacts. */ - delete(id: SessionId): Promise { - return this.serialize(id, () => this.deleteCore(id)) - } - - private async deleteCore(id: SessionId): Promise { - await this.backend.deleteStored(id) - this.states.delete(id) - } - // --- per-id serialization + adoption helpers --- /** diff --git a/packages/session-persistence/session-persistence/src/index.ts b/packages/session-persistence/session-persistence/src/index.ts index 8ff9aa8cb2..a9ffd11792 100644 --- a/packages/session-persistence/session-persistence/src/index.ts +++ b/packages/session-persistence/session-persistence/src/index.ts @@ -103,7 +103,7 @@ export abstract class SessionPersistence extends Service { /** * Register a new session's metadata. A backend MAY defer the physical write * until the first {@link append} (lazy materialization), in which case a - * created-but-never-appended session is absent from {@link has}/{@link list} + * created-but-never-appended session is absent from {@link list} * — abandoned sessions leave nothing behind. */ abstract create(meta: SessionHeader): Promise @@ -143,12 +143,6 @@ export abstract class SessionPersistence extends Service { /** Lightweight listing from metadata, without a full-log parse. */ abstract list(): Promise - - /** Whether a session is durably present (materialized). */ - abstract has(id: SessionId): Promise - - /** Remove a session and all its persisted artifacts. */ - abstract delete(id: SessionId): Promise } export default SessionPersistence diff --git a/packages/session-persistence/session-persistence/tests/contract.ts b/packages/session-persistence/session-persistence/tests/contract.ts index 704e0abfb0..aa7c76c84c 100644 --- a/packages/session-persistence/session-persistence/tests/contract.ts +++ b/packages/session-persistence/session-persistence/tests/contract.ts @@ -142,24 +142,22 @@ export function runPersistenceContract(name: string, make: () => Promise { + it('list() excludes a created-but-never-appended (zero-event) session', async () => { const { persistence, dispose } = await make() try { await persistence.create(meta('empty')) - expect(await persistence.has(SessionId('empty'))).toBe(false) expect((await persistence.list()).map(m => m.id)).not.toContain(SessionId('empty')) } finally { await dispose() } }) - it('has()/list() include a session once it has events', async () => { + it('list() includes a session once it has events', async () => { const { persistence, dispose } = await make() try { const m = meta('s2') await persistence.create(m) await persistence.append(m.id, oneTurnLog()) - expect(await persistence.has(m.id)).toBe(true) expect((await persistence.list()).map(x => x.id)).toContain(m.id) } finally { await dispose() @@ -227,19 +225,5 @@ export function runPersistenceContract(name: string, make: () => Promise { - const { persistence, dispose } = await make() - try { - const m = meta('s6') - await persistence.create(m) - await persistence.append(m.id, oneTurnLog()) - expect(await persistence.has(m.id)).toBe(true) - await persistence.delete(m.id) - expect(await persistence.has(m.id)).toBe(false) - } finally { - await dispose() - } - }) }) } diff --git a/packages/session-persistence/session-persistence/tests/coordinator-contract.ts b/packages/session-persistence/session-persistence/tests/coordinator-contract.ts index 9f42eebd10..1d9a1339d1 100644 --- a/packages/session-persistence/session-persistence/tests/coordinator-contract.ts +++ b/packages/session-persistence/session-persistence/tests/coordinator-contract.ts @@ -625,7 +625,7 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise< const m = meta('empty-batch', WORK) await ctx.sessionPersistence.create(m) await ctx.sessionPersistence.append(m.id, []) - expect(await ctx.sessionPersistence.has(m.id)).toBe(false) + expect((await ctx.sessionPersistence.list()).map(h => h.id)).not.toContain(m.id) } finally { await fiber.dispose() await fix.cleanup() @@ -643,17 +643,6 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise< } }) - it('delete of a non-existent session is a no-op', async () => { - const fix = await makeFixture() - const { ctx, fiber } = await freshCtx(fix) - try { - await expect(ctx.sessionPersistence.delete(SessionId('ghost'))).resolves.toBeUndefined() - } finally { - await fiber.dispose() - await fix.cleanup() - } - }) - it('create rejects a duplicate id (in memory and on a persisted log)', async () => { const fix = await makeFixture() const first = await freshCtx(fix) diff --git a/packages/session-persistence/session-persistence/tests/persistence.spec.ts b/packages/session-persistence/session-persistence/tests/persistence.spec.ts index 8b5a437735..4e4cf67822 100644 --- a/packages/session-persistence/session-persistence/tests/persistence.spec.ts +++ b/packages/session-persistence/session-persistence/tests/persistence.spec.ts @@ -61,14 +61,6 @@ class MemoryPersistence extends SessionPersistence implements PersistenceBackend return this.coordinator.load(id) } - has(id: SessionId): Promise { - return this.coordinator.has(id) - } - - delete(id: SessionId): Promise { - return this.coordinator.delete(id) - } - /** White-box accessor: await a specific session's onCreated init. */ get inits(): Map> { return this.coordinator.inits @@ -114,10 +106,6 @@ class MemoryPersistence extends SessionPersistence implements PersistenceBackend if (closers.length > 0) entry.events.push(...structuredClone(closers) as SessionEvent[]) } - async deleteStored(id: SessionId): Promise { - this.store.delete(id) - } - async list(): Promise { return [...this.store.values()].map(e => structuredClone(e.meta)) } From 5f9d10c58793de43dcc208d80d04a3fd0bc622ea Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 02:45:21 +0800 Subject: [PATCH 2/5] fix review findings: stale seam docs + race-free doneFor test helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of the PR2 diff caught doc/comment sites doc-sync does not gate (core-data-structures prose) and a latent test-helper race: - docs/core-data-structures/persistence.md + bash.md, sqlite README, and two source comments (coordinator.ts, jsonl.spec.ts) still listed the removed has/delete/get/list methods — updated to the surviving four-method persistence surface and the get/list-free bash seam. - doneFor(ctx, id) attached its onTaskDone listener lazily, after the task could already have closed (e.g. `true`), so it could miss the completion and hang. Replaced with trackCompletions(ctx): one eagerly-installed listener (mounted in setup() before any task starts) records every completion, and doneFor resolves immediately for an already-finished task or on completion otherwise. Race-free, and there is no get-by-id seam left to poll instead. --- docs/core-data-structures/bash.md | 2 +- docs/core-data-structures/persistence.md | 4 +- packages/bash/tool-bash/tests/tools.spec.ts | 46 ++++++++++++++----- .../tests/jsonl.spec.ts | 4 +- .../session-persistence-sqlite/README.md | 2 +- .../session-persistence/src/coordinator.ts | 2 +- 6 files changed, 41 insertions(+), 19 deletions(-) diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index c601d8cd74..dfbdec1d2f 100644 --- a/docs/core-data-structures/bash.md +++ b/docs/core-data-structures/bash.md @@ -120,4 +120,4 @@ interface BashTaskRead { ## The service -`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `get`/`ownerOf`/`list`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). +`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `ownerOf`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). diff --git a/docs/core-data-structures/persistence.md b/docs/core-data-structures/persistence.md index 630d38480f..f1ee857998 100644 --- a/docs/core-data-structures/persistence.md +++ b/docs/core-data-structures/persistence.md @@ -2,7 +2,7 @@ The **durability seam** for the event log. [session.md](session.md) describes the in-memory `Session` — the append-only `SessionEvent` log that is the source of truth. This page describes how that log is made durable: the abstract `SessionPersistence` service, its backends, the flush checkpoint, crash recovery, and the metadata header that travels alongside the log. -The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list/has/delete over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md). +The seam is a textbook [capability seam](../rfc/implemented/architecture/2026-06-13-capability-seams.md): one abstract service ([dsh-session-persistence](../../packages/session-persistence/session-persistence), `ctx.sessionPersistence`) defining create/append/load/list over the existing `SessionEvent` — **no parallel persisted type** — and two interchangeable backends that pass the same `runPersistenceContract` suite. See the [session-persistence RFC](../rfc/implemented/architecture/2026-06-14-session-persistence.md). ## The flush checkpoint @@ -55,7 +55,7 @@ Replay/fork is therefore `ctx.sessions.create(id, { seed: seedEvents })`; resumi ## The backends -Both implement the same abstract `SessionPersistence` (create/append/load/list/has/delete over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic: +Both implement the same abstract `SessionPersistence` (create/append/load/list over `SessionEvent`) and pass `runPersistenceContract`, proving the seam is genuinely backend-agnostic: - **[dsh-session-persistence-jsonl](../../packages/session-persistence/session-persistence-jsonl)** — an append-only JSONL log per session with crash-safe atomic writes, the interrupted-turn crash recovery above, and a read/replay path. - **[dsh-session-persistence-sqlite](../../packages/session-persistence/session-persistence-sqlite)** — `node:sqlite`, one row per `SessionEvent`. The row shape `(session_id, seq, type, time, data)` maps 1:1 onto the event, so there is no parallel persisted schema to keep in sync. diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index d8d166ea5b..b7429ff9bb 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -24,6 +24,7 @@ async function setup() { await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) + trackCompletions(ctx) return ctx } @@ -67,22 +68,42 @@ function text(result: { content: { type: string; text?: string }[] }): string { } /** - * Resolve once the background task with `id` completes. The task is started - * indirectly (via `ctx.tools.execute`), so `start()`'s return is not accessible - * here; the executor's `onTaskDone` listener delivers the SAME task object on - * completion, which is the surviving seam for awaiting a task by id. + * Per-context background-completion tracker. The task is started indirectly + * (via `ctx.tools.execute`), so `start()`'s return is not accessible here, and + * there is no get-by-id seam to poll current state — the only surviving way to + * await a task by id is the executor's `onTaskDone` listener. Registering that + * listener lazily (after the task may have already closed) would miss the + * completion and hang; so {@link trackCompletions} installs ONE listener + * EAGERLY (before any task starts) that records every completion, and + * {@link doneFor} resolves from that record — immediately if the task already + * finished, otherwise when it does. Call `trackCompletions(ctx)` right after + * the executor is mounted (`setup()` does this for you). */ -function doneFor(ctx: Context, id: string): Promise { - return new Promise((resolve) => { - const dispose = ctx.bash.onTaskDone((task) => { - if (task.id === id) { - dispose() - resolve(task) - } - }) +const completions = new WeakMap; waiters: Map void> }>() + +function trackCompletions(ctx: Context): void { + const state = { done: new Map(), waiters: new Map void>() } + completions.set(ctx, state) + ctx.bash.onTaskDone((task) => { + const waiter = state.waiters.get(task.id) + if (waiter) { + state.waiters.delete(task.id) + waiter(task) + } else { + state.done.set(task.id, task) + } }) } +/** Resolve (with the task object) once the background task `id` has completed. */ +function doneFor(ctx: Context, id: string): Promise { + const state = completions.get(ctx) + if (!state) throw new Error('trackCompletions(ctx) must be called before doneFor(ctx, …)') + const already = state.done.get(id) + if (already) return Promise.resolve(already) + return new Promise(resolve => state.waiters.set(id, resolve)) +} + class LossyReadBashExecutor extends BashExecutor { private readonly task: BashTask = { id: 'bash-lossy', @@ -312,6 +333,7 @@ describe('background tools', () => { await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) + trackCompletions(ctx) const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! diff --git a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts index 8acc578521..86c4a9d08b 100644 --- a/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts +++ b/packages/session-persistence/session-persistence-jsonl/tests/jsonl.spec.ts @@ -629,8 +629,8 @@ describe('SessionPersistenceJsonl: edge cases', () => { const a = meta('dup-id', '/projA') await ctx.sessionPersistence.create(a) await ctx.sessionPersistence.append(a.id, oneTurnLog()) - // A fresh backend creating the SAME id under cwd B must still refuse: load/ - // has identify by id across all buckets, so a second log would make resume + // A fresh backend creating the SAME id under cwd B must still refuse: load + // identifies by id across all buckets, so a second log would make resume // nondeterministic. create scans every bucket, not just meta.cwd's. const ctx2 = new Context() await ctx2.plugin(SessionStore) diff --git a/packages/session-persistence/session-persistence-sqlite/README.md b/packages/session-persistence/session-persistence-sqlite/README.md index 02255c024f..f397f80445 100644 --- a/packages/session-persistence/session-persistence-sqlite/README.md +++ b/packages/session-persistence/session-persistence-sqlite/README.md @@ -6,7 +6,7 @@ A SQLite durable session-persistence backend — a second `SessionPersistence` i ## Storage model -Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`has`/`list` report exactly the sessions that have a row), so no separate column is needed. +Each `SessionEvent` maps 1:1 onto a row in an `events` table `(session_id, seq, type, time, data)` — `data` is the event payload as JSON text, so the row shape is the event verbatim (including `assistant/chunk`, keeping `seq` contiguous). Out-of-log metadata (`SessionHeader`) lives in a `sessions` row. A `sessions` row is written only by the first `append` — its existence is the lazy-materialization signal (`list` reports exactly the sessions that have a row), so no separate column is needed. The repo targets Node ≥ 24 (the root `engines` field), which includes the stable `node:sqlite` module. The database opens with `foreign_keys = ON` (so `ON DELETE CASCADE` drops a session's events with its row) and `journal_mode = WAL`. The table-layout version is stored in `PRAGMA user_version` and checked on open: a fresh database is stamped with the current `SCHEMA_VERSION`; a database written by any other, incompatible build (a non-current `user_version`, older or newer) is rejected rather than opened against an unknown layout — there is no migration (unreleased software). diff --git a/packages/session-persistence/session-persistence/src/coordinator.ts b/packages/session-persistence/session-persistence/src/coordinator.ts index d2f5712502..58f1246763 100644 --- a/packages/session-persistence/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/session-persistence/src/coordinator.ts @@ -202,7 +202,7 @@ export class PersistenceCoordinator { throw new Error(`session "${meta.id}" already exists in this backend`) } // A persisted artifact under this id (in ANY scope) blocks creation: load/ - // has/resume identify a session by id alone, so a second artifact would make + // resume identify a session by id alone, so a second artifact would make // resume nondeterministic. if (await this.backend.loadStored(meta.id) !== undefined) { throw new Error(`session "${meta.id}" already has a persisted log on disk; load/resume it instead of creating`) From 6ca8c3b99a672d12b84ebff6443f98d26451f27e Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 03:00:07 +0800 Subject: [PATCH 3/5] fix review findings: stale get/list in proposed RFCs + doneFor double-await Second Codex pass caught two proposed RFCs that describe the bash seam as it WAS (with get/list) and would read stale once this prune lands, plus a latent test-helper edge: - docs/rfc/proposed/architecture/2026-06-20-branded-ids.md and 2026-06-20-generic-long-running-tool-runtime.md: drop get/list from the BashExecutor seam description (surviving: resolve/run/start/ownerOf/ readOutput/kill/onTaskDone). branded-ids will be further updated when it is implemented; this keeps it accurate in the meantime. - trackCompletions now records every completion to `done` unconditionally (and also wakes a parked waiter), so a second doneFor(id) after completion resolves instead of hanging. --- docs/rfc/proposed/architecture/2026-06-20-branded-ids.md | 4 ++-- .../2026-06-20-generic-long-running-tool-runtime.md | 2 +- packages/bash/tool-bash/tests/tools.spec.ts | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md index 93a4bf6cda..d69eb8f8c6 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md +++ b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md @@ -6,7 +6,7 @@ Status: proposed The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded = string & { readonly [BRAND]: B }` machinery and a zero-cost cast factory per type. `brand.ts` also states the governing policy: *"Branding is for IDs that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today. -**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. +**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.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](../simplification/2026-06-20-unify-agent-and-session-id.md) proposal calls the "bash owner-token alias hole". @@ -16,7 +16,7 @@ The bash **owner token** is the related sub-case: `BashExecRequest.owner?: strin A type-only change. Brands are zero-cost casts; nothing about runtime behavior, serialization, comparison, or the wire format changes. The work is in three parts, all honoring the existing "not every string" policy. -- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). +- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). - **Mint a distinct `OwnerToken` brand.** Add `OwnerToken = Branded<'OwnerToken'>` in `packages/bash/bash/src/types.ts`; type `BashExecRequest.owner` / `BashExecSpec.owner` / `BashExecutor.ownerOf` as `OwnerToken | undefined`. The `dsh-tool-bash` consumer casts the agent's `session.header.id` (a `SessionId`) into an `OwnerToken` at the boundary — the one place the two vocabularies meet. The bash seam never imports `dsh-session`. (Rationale in the next section.) diff --git a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md index 4f034e3020..d17224c46f 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md +++ b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md @@ -4,7 +4,7 @@ Status: proposed ## Problem -The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `get`, `ownerOf`, `list`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. +The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `ownerOf`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. The [tool cookbook](../../../cookbook/adding-a-tool.md) already points at the real design smell: background bash is really generic long-running-tool infrastructure living inside one tool. If future tools need background execution, polling, kill, ownership, and completion notices, those semantics should not be hidden in `dsh-bash`. diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index b7429ff9bb..e0cc1165e3 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -85,12 +85,13 @@ function trackCompletions(ctx: Context): void { const state = { done: new Map(), waiters: new Map void>() } completions.set(ctx, state) ctx.bash.onTaskDone((task) => { + // Always record the completion so a later doneFor(id) still resolves; also + // wake any waiter already parked on this id. + state.done.set(task.id, task) const waiter = state.waiters.get(task.id) if (waiter) { state.waiters.delete(task.id) waiter(task) - } else { - state.done.set(task.id, task) } }) } From 24168aee70ac77d3dbaaecf0a8224561f7002bfd Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 06:17:11 +0800 Subject: [PATCH 4/5] =?UTF-8?q?revert=20bash=20get()/list()=20removal=20?= =?UTF-8?q?=E2=80=94=20keep=20persistence-only=20prune?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original prune removed BashExecutor.get()/.list() too, but each is a one-line accessor over the executor's already-tracked tasks map, and removing them forced dsh-tool-bash's tests onto a ~35-line onTaskDone completion-tracking harness just to replace the one-line ctx.bash.get(id) lookup. Per the AGENTS.md "RFCs are proposals, not golden truth" principle, that disproportionate migration cost is evidence the methods earn their keep — a test harness IS a consumer programming against the seam. Restore get()/list() (seam + LocalBashExecutor impl + the bash tests that used them, dropping the doneFor/trackCompletions scaffolding). The persistence has()/delete()/deleteStored removal stands — it had only contract-test callers and no test-ergonomics cost. The RFC is retitled persistence-only with an implementation note recording the bash revert. --- docs/cordis-catalog/events-and-services.md | 2 + docs/core-data-structures/bash.md | 2 +- .../2026-06-20-prune-dead-seam-methods.md | 12 +-- .../architecture/2026-06-20-branded-ids.md | 4 +- ...06-20-generic-long-running-tool-runtime.md | 2 +- packages/bash/bash-local/src/index.ts | 8 ++ .../bash/bash-local/tests/executor.spec.ts | 6 +- packages/bash/bash/README.md | 1 + packages/bash/bash/src/index.ts | 6 ++ packages/bash/bash/tests/service.spec.ts | 10 +++ .../bash/tool-bash/tests/integration.spec.ts | 12 +-- packages/bash/tool-bash/tests/tools.spec.ts | 77 ++++++------------- 12 files changed, 68 insertions(+), 74 deletions(-) diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index e8445b4786..0422555ec2 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -327,7 +327,9 @@ Semantics every implementation must honor: abstract resolve(request: BashExecRequest): BashExecSpec abstract run(spec: BashExecSpec): Promise abstract start(spec: BashExecSpec): BashTask +abstract get(id: string): BashTask | undefined abstract ownerOf(id: string): string | undefined +abstract list(): BashTask[] abstract readOutput(id: string): BashTaskRead abstract kill(id: string): boolean onTaskDone(listener: BashTaskListener): () => void diff --git a/docs/core-data-structures/bash.md b/docs/core-data-structures/bash.md index dfbdec1d2f..c601d8cd74 100644 --- a/docs/core-data-structures/bash.md +++ b/docs/core-data-structures/bash.md @@ -120,4 +120,4 @@ interface BashTaskRead { ## The service -`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `ownerOf`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). +`BashExecutor` (`ctx.bash`, abstract — defined in [`packages/bash/bash/src/index.ts`](../../packages/bash/bash/src/index.ts)) mirrors the `LlmService`/`LlmAdapter` split: `resolve` (request → spec), `run` (foreground), `start` (background), `get`/`ownerOf`/`list`/`readOutput`/`kill`, and `onTaskDone` (a `BashTaskListener` completion callback). Spawned commands get a **scrubbed env** (dropping `*KEY*`/`*SECRET*`/`*TOKEN*`) and spill files use a private 0700 dir with random names and owner-only opens — model output never gets the ambient environment or a predictable path. The implementation that provides all this is `dsh-bash-local`; the model-facing `bash`/`bash_output`/`bash_kill` schemas that call it are in `dsh-tool-bash` (and present as terminals via the [tool-presentation vocabulary](tools.md#tool-presentation-ui-vocabulary)). diff --git a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md index ec9dc45223..3cf99e47ef 100644 --- a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md +++ b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md @@ -1,7 +1,9 @@ -# RFC: Prune dead methods from the persistence and bash capability seams +# RFC: Prune dead methods from the persistence seam Status: implemented (proposed and accepted 2026-06-20) +> **Implementation note (scope narrowed from the original proposal).** This RFC proposed pruning dead methods from BOTH the persistence seam (`SessionPersistence.has()`/`.delete()`) and the bash seam (`BashExecutor.get()`/`.list()`). Only the **persistence** removal shipped. The bash `get()`/`.list()` removal was reverted before merge: each is a one-line accessor over the executor's already-tracked `tasks` map, and removing them forced `dsh-tool-bash`'s tests onto a ~35-line `onTaskDone`-based completion-tracking harness to replace the one-line `ctx.bash.get(id)` lookup — the migration cost dwarfed the surface removed. Per the [AGENTS.md "RFCs are proposals, not golden truth"](../../../../AGENTS.md) principle, that friction is evidence the method earns its keep (a test harness IS a consumer that programs against the seam), so `get()`/`list()` stay. The bash-seam analysis below is retained for the record but was NOT acted on; `BashTaskId`-branding those methods lands in the [branded-ids RFC](../../proposed/architecture/2026-06-20-branded-ids.md) instead. The persistence removal stands: `has()`/`delete()` had only contract-test callers and no test-ergonomics cost to remove. + ## Problem Two capability seams ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carry abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test. @@ -35,10 +37,10 @@ Re-adding a seam method with a live consumer is cheap and better-designed than t ## Acceptance criteria -- `has`/`delete`/`deleteStored` and `get`/`list` are gone from their seams, impls, and contract suites; `pnpm run knip` reports no new dead exports. -- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`ownerOf`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically. -- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed methods are deleted with them). -- Seam READMEs and `docs/architecture.md` no longer list the removed methods. +- `has`/`delete`/`deleteStored` are gone from the persistence seam, impl, and contract suites; `pnpm run knip` reports no new dead exports. (The bash `get`/`list` removal was reverted — see the implementation note above; those methods remain.) +- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`get`/`ownerOf`/`list`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically. +- `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed persistence methods are deleted with them). +- Persistence seam READMEs and `docs/architecture.md` no longer list the removed `has`/`delete` methods. ## Risks diff --git a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md index d69eb8f8c6..93a4bf6cda 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md +++ b/docs/rfc/proposed/architecture/2026-06-20-branded-ids.md @@ -6,7 +6,7 @@ Status: proposed The harness already brands three identifiers — `CallId` (`packages/llm/llm/src/brand.ts`), `SessionId` (`packages/core/session/src/types.ts`), and `AgentId` (`packages/core/agent/src/types.ts`) — using the `Branded = string & { readonly [BRAND]: B }` machinery and a zero-cost cast factory per type. `brand.ts` also states the governing policy: *"Branding is for IDs that cross package boundaries and could plausibly be confused; not every string needs a brand."* That policy is right; the problem is that it is only half-applied. Two gaps let a structurally-identical-but-semantically-wrong string slip through the type checker today. -**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.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. +**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](../simplification/2026-06-20-unify-agent-and-session-id.md) proposal calls the "bash owner-token alias hole". @@ -16,7 +16,7 @@ The bash **owner token** is the related sub-case: `BashExecRequest.owner?: strin A type-only change. Brands are zero-cost casts; nothing about runtime behavior, serialization, comparison, or the wire format changes. The work is in three parts, all honoring the existing "not every string" policy. -- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). +- **Brand the bash task id.** Add `BashTaskId = Branded<'BashTaskId'>` plus its same-named factory in `packages/bash/bash/src/types.ts` (the package that *owns* the id), importing `Branded` from `@deepseek-ai/dsh-llm` exactly as `SessionId`/`AgentId` already do. Thread it through `BashTask.id`, the `BashExecutor` seam methods (`get`/`ownerOf`/`readOutput`/`kill`), the generation site in `dsh-bash-local` (brand the counter output once, at creation), and the `dsh-tool-bash` validate/access surface (`validateTaskId` returns a `BashTaskId`; `task_id` is branded at the tool boundary where the model's string arrives). - **Mint a distinct `OwnerToken` brand.** Add `OwnerToken = Branded<'OwnerToken'>` in `packages/bash/bash/src/types.ts`; type `BashExecRequest.owner` / `BashExecSpec.owner` / `BashExecutor.ownerOf` as `OwnerToken | undefined`. The `dsh-tool-bash` consumer casts the agent's `session.header.id` (a `SessionId`) into an `OwnerToken` at the boundary — the one place the two vocabularies meet. The bash seam never imports `dsh-session`. (Rationale in the next section.) diff --git a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md index d17224c46f..4f034e3020 100644 --- a/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md +++ b/docs/rfc/proposed/architecture/2026-06-20-generic-long-running-tool-runtime.md @@ -4,7 +4,7 @@ Status: proposed ## Problem -The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `ownerOf`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. +The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `get`, `ownerOf`, `list`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard. The [tool cookbook](../../../cookbook/adding-a-tool.md) already points at the real design smell: background bash is really generic long-running-tool infrastructure living inside one tool. If future tools need background execution, polling, kill, ownership, and completion notices, those semantics should not be hidden in `dsh-bash`. diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index 7b55200cf8..df6e2285a9 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -176,12 +176,20 @@ export class LocalBashExecutor extends BashExecutor { return task } + get(id: string): BashTask | undefined { + return this.tasks.get(id) + } + ownerOf(id: string): string | undefined { // Unknown id and known-but-ownerless both read as undefined — the consumer // treats undefined as "open" and a truly unknown id fails at readOutput/kill. return this.tasks.get(id)?.owner } + list(): BashTask[] { + return [...this.tasks.values()] + } + readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) diff --git a/packages/bash/bash-local/tests/executor.spec.ts b/packages/bash/bash-local/tests/executor.spec.ts index ec90aeb77e..3dd7f7983a 100644 --- a/packages/bash/bash-local/tests/executor.spec.ts +++ b/packages/bash/bash-local/tests/executor.spec.ts @@ -98,6 +98,8 @@ describe('LocalBashExecutor background tasks', () => { const task = bash.start(bash.resolve({ command: 'sleep 0.2; echo done' })) expect(Date.now() - before).toBeLessThan(150) expect(task.status).toBe('running') + expect(bash.get(task.id)).toBe(task) + expect(bash.list()).toContain(task) await task.done expect(task.status).toBe('completed') expect(task.exitCode).toBe(0) @@ -235,6 +237,7 @@ describe('LocalBashExecutor background tasks', () => { await running.done expect(finished.status).toBe('completed') expect(running.signal).toBe('SIGTERM') + expect(bash.list()).toEqual([]) }) it('disposing the executor fiber kills running tasks (no orphans)', async () => { @@ -246,13 +249,14 @@ describe('LocalBashExecutor background tasks', () => { bash.onTaskDone(listener) const task = bash.start(bash.resolve({ command: 'sleep 60' })) - const running = task + const running = bash.get(task.id)! await new Promise(resolve => setTimeout(resolve, 50)) // Grab the pid before dispose clears the registry. const pid = (running as unknown as { running: { pid: number } }).running.pid await fiber.dispose() await waitGone(pid) + expect(bash.list()).toEqual([]) // Listener silenced by base-class teardown — no late notifications. expect(listener).not.toHaveBeenCalled() }) diff --git a/packages/bash/bash/README.md b/packages/bash/bash/README.md index c982a217c7..ce8816dee7 100644 --- a/packages/bash/bash/README.md +++ b/packages/bash/bash/README.md @@ -18,6 +18,7 @@ The split mirrors the LLM seam (`LlmService`/`LlmAdapter`) and the agent-tool su |---|---| | `run(spec)` | Foreground execution. Resolves when the command finishes. **Rejects only for infrastructure failures** (unusable workdir, missing shell, pre-aborted signal); nonzero exits, timeout kills, and abort kills resolve with a descriptive `BashRunResult`. | | `start(spec)` | Background execution. Returns a `BashTask` handle immediately; **no timeout applies** (stop tasks via `kill`). | +| `get(id)` / `list()` | Task lookup. | | `ownerOf(id)` | The opaque OWNER token recorded for a background task at `start` (from the spec's `owner`), or `undefined` for an unknown id OR a known-but-ownerless task. The executor stores/returns it verbatim and NEVER interprets it — the access POLICY lives in the consumer (`dsh-tool-bash`), which compares `ownerOf(id)` to the caller's token. Storing ownership here (disposed with the executor's fiber) is what makes it survive a consumer HMR reload. | | `readOutput(id)` | **Incremental** output read — consecutive reads never re-deliver. Reads that lost data to buffer bounds flag `lossy` and point at full-stream spill files. Throws for unknown ids. | | `kill(id)` | Kill a running task. Returns `false` when it already finished; throws for unknown ids. | diff --git a/packages/bash/bash/src/index.ts b/packages/bash/bash/src/index.ts index 9df8c720aa..f4e2d964fe 100644 --- a/packages/bash/bash/src/index.ts +++ b/packages/bash/bash/src/index.ts @@ -85,6 +85,9 @@ export abstract class BashExecutor extends Service { /** Start a background task and return its handle immediately. */ abstract start(spec: BashExecSpec): BashTask + /** Look up a background task by id. */ + abstract get(id: string): BashTask | undefined + /** * The opaque OWNER token recorded for a background task at {@link start} * (from the {@link BashExecSpec}'s `owner`), or `undefined` for an unknown id @@ -100,6 +103,9 @@ export abstract class BashExecutor extends Service { */ abstract ownerOf(id: string): string | undefined + /** All tracked background tasks (insertion order). */ + abstract list(): BashTask[] + /** Read output produced since the previous read. Throws for unknown ids. */ abstract readOutput(id: string): BashTaskRead diff --git a/packages/bash/bash/tests/service.spec.ts b/packages/bash/bash/tests/service.spec.ts index 2646715df9..4b28bb72be 100644 --- a/packages/bash/bash/tests/service.spec.ts +++ b/packages/bash/bash/tests/service.spec.ts @@ -44,10 +44,18 @@ class StubExecutor extends BashExecutor { return task } + get(id: string): BashTask | undefined { + return this.tasks.get(id) + } + ownerOf(id: string): string | undefined { return this.owners.get(id) } + list(): BashTask[] { + return [...this.tasks.values()] + } + readOutput(id: string): BashTaskRead { const task = this.tasks.get(id) if (!task) throw new Error(`unknown bash task "${id}"`) @@ -80,6 +88,8 @@ describe('BashExecutor service seam', () => { it('registers as ctx.bash and serves the abstract API', async () => { const { bash } = await setup() const task = bash.start(bash.resolve({ command: 'sleep 1' })) + expect(bash.get(task.id)).toBe(task) + expect(bash.list()).toEqual([task]) expect(bash.kill(task.id)).toBe(true) expect(bash.kill(task.id)).toBe(false) const result = await bash.run(bash.resolve({ command: 'true' })) diff --git a/packages/bash/tool-bash/tests/integration.spec.ts b/packages/bash/tool-bash/tests/integration.spec.ts index fadcbf741d..0ab786ca85 100644 --- a/packages/bash/tool-bash/tests/integration.spec.ts +++ b/packages/bash/tool-bash/tests/integration.spec.ts @@ -8,7 +8,6 @@ import ToolRegistry from '@deepseek-ai/dsh-tools' import AgentRegistry from '@deepseek-ai/dsh-agent' import AgentLoop, { ReactLoopAgent } from '@deepseek-ai/dsh-agent-loop' import { LocalBashExecutor } from '@deepseek-ai/dsh-bash-local' -import type { BashTask } from '@deepseek-ai/dsh-bash' import * as ToolBash from '@deepseek-ai/dsh-tool-bash' import { MockAdapter, textResponse, toolCallResponse } from '../../../core/agent-loop/tests/mock-adapter.ts' @@ -144,18 +143,13 @@ describe('bash tool through the agent loop', () => { return next() }) - // Capture the single background task's completion. Registered BEFORE send so - // a fast task (echo) can't finish before the listener is attached; onTaskDone - // delivers the task object once it completes (completion may race turn end). - const taskDone = new Promise((resolve) => { - const dispose = ctx.bash.onTaskDone((task) => { dispose(); resolve(task) }) - }) - agent.send([{ type: 'text', text: 'run echo bg-ok in the background' }]) await waitForIdle(ctx, agent) // Wait for the background task itself (completion may race turn end). - await taskDone + const task = ctx.bash.get(taskId) + if (!task) throw new Error(`task ${taskId} not registered`) + await task.done const log = events(agent) const firstResult = findEvent(log, 'tool/result') diff --git a/packages/bash/tool-bash/tests/tools.spec.ts b/packages/bash/tool-bash/tests/tools.spec.ts index e0cc1165e3..c49410a9b2 100644 --- a/packages/bash/tool-bash/tests/tools.spec.ts +++ b/packages/bash/tool-bash/tests/tools.spec.ts @@ -24,7 +24,6 @@ async function setup() { await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) - trackCompletions(ctx) return ctx } @@ -67,44 +66,6 @@ function text(result: { content: { type: string; text?: string }[] }): string { return result.content.filter(block => block.type === 'text').map(block => block.text).join('') } -/** - * Per-context background-completion tracker. The task is started indirectly - * (via `ctx.tools.execute`), so `start()`'s return is not accessible here, and - * there is no get-by-id seam to poll current state — the only surviving way to - * await a task by id is the executor's `onTaskDone` listener. Registering that - * listener lazily (after the task may have already closed) would miss the - * completion and hang; so {@link trackCompletions} installs ONE listener - * EAGERLY (before any task starts) that records every completion, and - * {@link doneFor} resolves from that record — immediately if the task already - * finished, otherwise when it does. Call `trackCompletions(ctx)` right after - * the executor is mounted (`setup()` does this for you). - */ -const completions = new WeakMap; waiters: Map void> }>() - -function trackCompletions(ctx: Context): void { - const state = { done: new Map(), waiters: new Map void>() } - completions.set(ctx, state) - ctx.bash.onTaskDone((task) => { - // Always record the completion so a later doneFor(id) still resolves; also - // wake any waiter already parked on this id. - state.done.set(task.id, task) - const waiter = state.waiters.get(task.id) - if (waiter) { - state.waiters.delete(task.id) - waiter(task) - } - }) -} - -/** Resolve (with the task object) once the background task `id` has completed. */ -function doneFor(ctx: Context, id: string): Promise { - const state = completions.get(ctx) - if (!state) throw new Error('trackCompletions(ctx) must be called before doneFor(ctx, …)') - const already = state.done.get(id) - if (already) return Promise.resolve(already) - return new Promise(resolve => state.waiters.set(id, resolve)) -} - class LossyReadBashExecutor extends BashExecutor { private readonly task: BashTask = { id: 'bash-lossy', @@ -133,10 +94,18 @@ class LossyReadBashExecutor extends BashExecutor { return this.task } + get(id: string): BashTask | undefined { + return id === this.task.id ? this.task : undefined + } + ownerOf(): string | undefined { return undefined } + list(): BashTask[] { + return [this.task] + } + readOutput(id: string): BashTaskRead { if (id !== this.task.id) throw new Error(`unknown bash task "${id}"`) return { task: this.task, delta: 'tail', lossy: true } @@ -317,7 +286,7 @@ describe('background tools', () => { expect(text(first)).toContain('first') expect(text(first)).toContain('[status: running]') - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const second = await call(ctx, 'bash_output', { task_id: id }) expect(text(second)).toContain('second') expect(text(second)).not.toContain('first') @@ -334,11 +303,10 @@ describe('background tools', () => { await ctx.plugin(LocalBashExecutor, { maxOutputBytes: 100 }) ;(ctx.bash as LocalBashExecutor).internals = { spillDir, graceMs: 200 } await ctx.plugin(ToolBash) - trackCompletions(ctx) const started = await call(ctx, 'bash', { command: 'for i in $(seq 1 200); do printf "line-%04d\\n" $i; done', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const read = await call(ctx, 'bash_output', { task_id: id }) expect(text(read)).toContain('[some output was dropped from memory; full output: ') }) @@ -361,7 +329,7 @@ describe('background tools', () => { const killed = await call(ctx, 'bash_kill', { task_id: id }) expect(text(killed)).toBe(`killed background task ${id}`) - await doneFor(ctx, id) + await ctx.bash.get(id)!.done const again = await call(ctx, 'bash_kill', { task_id: id }) expect(text(again)).toBe(`task ${id} had already finished`) @@ -405,7 +373,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done expect(inject).toHaveBeenCalledTimes(1) const [content, options] = inject.mock.calls[0] as [ @@ -428,7 +396,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() }) it('rethrows a non-disposed inject failure (not blindly swallowed)', async () => { @@ -447,7 +415,7 @@ describe('background tools', () => { agent, }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done // notifyTaskDone caught and logged the rethrown error. expect(errorSpy).toHaveBeenCalled() const logged = errorSpy.mock.calls.flat().some(arg => arg instanceof Error && arg.message === 'unexpected inject bug') @@ -475,7 +443,7 @@ describe('background tools', () => { const id = /task (bash-\d+)/.exec(text(started))![1]! // Unregister the agent BEFORE the task completes (simulate disconnect). unregisterFakeAgents(ctx) - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() expect(inject).not.toHaveBeenCalled() }) @@ -483,7 +451,7 @@ describe('background tools', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await expect(doneFor(ctx, id)).resolves.toBeDefined() + await expect(ctx.bash.get(id)!.done).resolves.toBeUndefined() }) }) @@ -566,7 +534,7 @@ describe('background task ownership (cross-session isolation)', () => { const b = fakeAgent('sess-b') const started = await callAs(ctx, a, 'bash', { command: 'echo done', description: 'bg', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - await doneFor(ctx, id) + await ctx.bash.get(id)!.done // Completion does NOT clear ownership: B is still rejected, A still allowed. const readByB = await callAs(ctx, b, 'bash_output', { task_id: id }) expect(readByB.isError).toBe(true) @@ -599,9 +567,7 @@ describe('background task ownership (cross-session isolation)', () => { // token) survive. await fiber.dispose() await ctx.plugin(ToolBash) - // The task survived the reload, still running and still owned by A — proven - // via A's own bash_output (reports running status) and the surviving owner token. - expect(text(await callAs(ctx, a, 'bash_output', { task_id: id }))).toContain('[status: running]') + expect(ctx.bash.get(id)?.status).toBe('running') expect(ctx.bash.ownerOf(id)).toBe('sess-a') // After reload, ownership is INTACT → B is STILL rejected. @@ -709,10 +675,10 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'sleep 60', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const done = doneFor(ctx, id) + const task = ctx.bash.get(id)! await call(ctx, 'bash_kill', { task_id: id }) - const task = await done + await task.done // Simulate the variant where the close event carried no signal. task.signal = null const read = await call(ctx, 'bash_output', { task_id: id }) @@ -723,7 +689,8 @@ describe('status lines', () => { const ctx = await setup() const started = await call(ctx, 'bash', { command: 'true', description: 'test command', run_in_background: true }) const id = /task (bash-\d+)/.exec(text(started))![1]! - const task = await doneFor(ctx, id) + const task = ctx.bash.get(id)! + await task.done // Defensive: completed tasks always carry an exit code in practice; the // ?? 0 fallback covers task shapes from other executor implementations. task.exitCode = null From 00d76465581d3259730cd17e6eb3d150ad7afb77 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 11:05:56 +0800 Subject: [PATCH 5/5] fix review findings: make the prune-seam RFC + index match the persistence-only shipped scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer caught two pieces of both-seams drift left over after the bash get()/list() removal was reverted to a persistence-only change. - docs/rfc/README.md: rename the index row from "persistence and bash seams" to "Prune dead methods from the persistence seam" so it matches the RFC title and the actually-shipped scope (verify-rfc-classification only checks the path is indexed, so this prose slipped the gate). - The implemented RFC body still read like the original both-seams proposal (the "Two capability seams" framing, a `### BashExecutor.get()/.list()` problem section, a bash removal bullet in the Proposal, and current-source links that imply bash get/list were removed). Rewrite the body into the durable decision-record form: Problem/Proposal/criteria/risks now describe only the persistence has()/delete() removal that shipped, and the bash reasoning (why get()/list() earn their keep — a ~35-line test-harness migration cost makes the test consumer a real consumer) is folded into the top decision note as "considered and deliberately kept", not as a shipped change. Drop the stale bash source-line refs; keep the persistence consumer links pointing at current code (agent-loop load, ACP session/list). --- docs/rfc/README.md | 2 +- .../2026-06-20-prune-dead-seam-methods.md | 33 +++++++------------ 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/docs/rfc/README.md b/docs/rfc/README.md index be22e0c84b..b19e4933ac 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -95,7 +95,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r | [Drop the mutable session summary](implemented/simplification/2026-06-19-drop-mutable-session-summary.md) | 2026-06-19 | | [Drop unconsumed assembled LLM convenience surfaces](implemented/simplification/2026-06-20-drop-unconsumed-llm-assembled-surfaces.md) | 2026-06-20 | | [Drop the unconsumed `llm/adapter-change` event](implemented/simplification/2026-06-20-drop-unconsumed-llm-adapter-change-event.md) | 2026-06-20 | -| [Prune dead methods from the persistence and bash seams](implemented/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | +| [Prune dead methods from the persistence seam](implemented/simplification/2026-06-20-prune-dead-seam-methods.md) | 2026-06-20 | ### Architecture diff --git a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md index 3cf99e47ef..1b1c56c02f 100644 --- a/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md +++ b/docs/rfc/implemented/simplification/2026-06-20-prune-dead-seam-methods.md @@ -2,50 +2,41 @@ Status: implemented (proposed and accepted 2026-06-20) -> **Implementation note (scope narrowed from the original proposal).** This RFC proposed pruning dead methods from BOTH the persistence seam (`SessionPersistence.has()`/`.delete()`) and the bash seam (`BashExecutor.get()`/`.list()`). Only the **persistence** removal shipped. The bash `get()`/`.list()` removal was reverted before merge: each is a one-line accessor over the executor's already-tracked `tasks` map, and removing them forced `dsh-tool-bash`'s tests onto a ~35-line `onTaskDone`-based completion-tracking harness to replace the one-line `ctx.bash.get(id)` lookup — the migration cost dwarfed the surface removed. Per the [AGENTS.md "RFCs are proposals, not golden truth"](../../../../AGENTS.md) principle, that friction is evidence the method earns its keep (a test harness IS a consumer that programs against the seam), so `get()`/`list()` stay. The bash-seam analysis below is retained for the record but was NOT acted on; `BashTaskId`-branding those methods lands in the [branded-ids RFC](../../proposed/architecture/2026-06-20-branded-ids.md) instead. The persistence removal stands: `has()`/`delete()` had only contract-test callers and no test-ergonomics cost to remove. +> **Decision (scope: persistence only).** The shipped change removes the two dead persistence methods `SessionPersistence.has()` and `.delete()`; the body below records that decision. The bash seam's `BashExecutor.get()`/`.list()` were **considered for the same treatment and deliberately kept**: each is a one-line accessor over the executor's already-tracked `tasks` map, and removing them would force `dsh-tool-bash`'s tests onto a ~35-line `onTaskDone`-based completion-tracking harness to replace the one-line `ctx.bash.get(id)` lookup — the migration cost dwarfs the surface removed. Per the [AGENTS.md "RFCs are proposals, not golden truth"](../../../../AGENTS.md) principle, that friction is evidence the method earns its keep: a test harness IS a consumer that programs against the seam, so `get()`/`list()` stay. (`BashTaskId`-branding those surviving methods is taken up by the [branded-ids RFC](../../proposed/architecture/2026-06-20-branded-ids.md).) The persistence removal carries no such cost: `has()`/`delete()` had only contract-test callers and no test-ergonomics consumer to migrate. ## Problem -Two capability seams ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carry abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test. +A capability seam ([interface / implementation / consumer](../../implemented/architecture/2026-06-13-capability-seams.md)) carries abstract methods that no consumer calls. The seam exists to let implementations and consumers evolve independently — but a method no consumer programs against is not a seam, it is speculative surface every implementation must still implement and test. ### `SessionPersistence.has()` and `.delete()` -The abstract service declares four operations beyond create/append: `load`, `list`, `has`, `delete` ([packages/session-persistence/session-persistence/src/index.ts:142-151](../../../../packages/session-persistence/session-persistence/src/index.ts)). Production consumers of `ctx.sessionPersistence` use only two of them: the agent-loop resume path calls `load()` ([packages/core/agent-loop/src/index.ts:176-194](../../../../packages/core/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/ui/acp/src/index.ts](../../../../packages/ui/acp/src/index.ts)). Grepping every `sessionPersistence.*` / `persistence.*` use across `packages/*/src` and `examples/` finds no `has(` and no `delete(` on the service. The `.has(`/`.delete(` calls in `packages/ui/acp/src/index.ts` are on the in-memory `SessionStore` and a local `Set` of loading ids, not persistence. The only callers of `has`/`delete` are the contract suites and per-backend specs. +The abstract service declared its operations beyond create/append: `load`, `list`, `has`, `delete`. Production consumers of `ctx.sessionPersistence` use only two: the agent-loop resume path calls `load()` ([packages/core/agent-loop/src/index.ts:176](../../../../packages/core/agent-loop/src/index.ts)), and the ACP bridge calls `list()` for `session/list` ([packages/ui/acp/src/index.ts:494](../../../../packages/ui/acp/src/index.ts)). Grepping every `sessionPersistence.*` / `persistence.*` use across `packages/*/src` and `examples/` finds no `has(` and no `delete(` on the service. The `.has(`/`.delete(` calls in `packages/ui/acp/src/index.ts` are on the in-memory `SessionStore` and a local `Set` of loading ids, not persistence. The only callers of `has`/`delete` were the contract suites and per-backend specs. -`has()` is not just unused — it is the most intricate branch in the shared coordinator: a tracked-vs-untracked dual-probe (`loadLive(id, cwd)` for a live-tracked session vs `loadStored(id)` for an untracked one) with a multi-line rationale ([packages/session-persistence/session-persistence/src/coordinator.ts:298-310](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)). `delete()` drags the `deleteStored` backend hook ([coordinator.ts:99](../../../../packages/session-persistence/session-persistence/src/coordinator.ts), [coordinator.ts:313-319](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)) that every backend must implement. This is the [drop-mutable-session-summary](../../implemented/simplification/2026-06-19-drop-mutable-session-summary.md) pattern: a contract test exercises both, but no shipping code asks "is this session persisted?" or removes one. - -### `BashExecutor.get()` and `.list()` - -The bash seam declares `get(id)` ("look up a background task by id") and `list()` ("all tracked background tasks") ([packages/bash/bash/src/index.ts:88-107](../../../../packages/bash/bash/src/index.ts)), both implemented by `LocalBashExecutor` ([packages/bash/bash-local/src/index.ts:179-191](../../../../packages/bash/bash-local/src/index.ts)). The sole production consumer — `dsh-tool-bash` — drives tasks via `ownerOf`, `onTaskDone`, `start`, `readOutput`, `kill`, `resolve`, `run`; it never calls `get`/`list` in shipping code, and there is no `bash_list` tool exposing a task roster to the model. So both are dead production seam surface. They are used by tests, more broadly than a single idiom: the bash seam/executor specs assert them directly ([packages/bash/bash/tests/service.spec.ts](../../../../packages/bash/bash/tests/service.spec.ts), [packages/bash/bash-local/tests/executor.spec.ts](../../../../packages/bash/bash-local/tests/executor.spec.ts) both call `get()`/`list()`), and several `dsh-tool-bash` tests reach through `ctx.bash.get(id)` to await a task's `done`, read its `status`, or inspect task fields ([packages/bash/tool-bash/tests/tools.spec.ts](../../../../packages/bash/tool-bash/tests/tools.spec.ts), [packages/bash/tool-bash/tests/integration.spec.ts](../../../../packages/bash/tool-bash/tests/integration.spec.ts)). These are test-harness conveniences, not shipping consumers — but they are real test code an implementing PR must migrate or delete. +`has()` was not just unused — it was the most intricate branch in the shared coordinator: a tracked-vs-untracked dual-probe (`loadLive(id, cwd)` for a live-tracked session vs `loadStored(id)` for an untracked one) with a multi-line rationale. `delete()` dragged the `deleteStored` backend hook that every backend had to implement. This is the [drop-mutable-session-summary](../../implemented/simplification/2026-06-19-drop-mutable-session-summary.md) pattern: a contract test exercised both, but no shipping code asks "is this session persisted?" or removes one. ## Proposal Remove the methods nothing consumes, from the abstract seam, the implementation, and the contract/spec suites that exist only to exercise them: -- `SessionPersistence.has()` / `.delete()`: delete the abstract declarations, the coordinator's `has`/`delete`/`deleteCore`, and the `PersistenceBackend.deleteStored` hook. Remove the `has`/`delete` rows from the contract suite and the per-backend specs (jsonl + sqlite each implement `deleteStored` only to satisfy the hook — that implementation goes too). The backends are the [dual-backend](../../implemented/architecture/2026-06-14-session-persistence.md) design and otherwise out of scope, but removing a hook they implement for no consumer is part of removing the hook, not a backend redesign. -- `BashExecutor.get()` / `.list()`: delete the abstract declarations and the `LocalBashExecutor` impls. The seam/executor specs that assert `get()`/`list()` directly (`bash/tests/service.spec.ts`, `bash-local/tests/executor.spec.ts`) lose those assertions (the behavior is being removed). The `dsh-tool-bash` tests that reach through `ctx.bash.get(id)` to await `done`, read `status`, or inspect task fields switch to the public completion/status seam they should use — `onTaskDone` (or the `done` promise and status the `start()` return already exposes) — keeping their coverage without the removed lookup method. -- Update every doc and source-comment reference to the removed methods — not only literal `has(`/`delete(`/`get(`/`list(`/`deleteStored` call spellings, but also `{@link has}`/`{@link delete}` JSDoc links and prose that counts the methods (removing 2 of the persistence service's 6 public methods makes any "six public methods" phrasing wrong). The implementing PR greps `has`/`delete`/`get`/`list`/`deleteStored`/`{@link `/`six ` across `docs/`, `packages/*/README.md`, and source comments, and fixes each. The known doc sites: the seam READMEs ([packages/session-persistence/session-persistence/README.md](../../../../packages/session-persistence/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four, [packages/bash/bash/README.md](../../../../packages/bash/bash/README.md)'s `get(id)`/`list()` row), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence/session-persistence-sqlite/README.md](../../../../packages/session-persistence/session-persistence-sqlite/README.md), [packages/session-persistence/session-persistence-jsonl/README.md](../../../../packages/session-persistence/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../architecture.md), and the persistence prose in the [session-persistence RFC](../../implemented/architecture/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../../implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md). The known source-comment sites: the abstract `create()` JSDoc's `{@link has}/{@link list}` link ([packages/session-persistence/session-persistence/src/index.ts](../../../../packages/session-persistence/session-persistence/src/index.ts) — drop the `has` link), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying the `materialized` flag by "the signal `has`/`list` rely on" ([packages/session-persistence/session-persistence/src/coordinator.ts](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)), the JSONL backend's `loadStored`/`deleteStored` comment, and the SQLite backend's `schema.ts` and `index.ts` comments that mention "absent from `has`/`list`" — all reworded to the surviving four-method, `list()`-only contract. +- `SessionPersistence.has()` / `.delete()`: delete the abstract declarations, the coordinator's `has`/`delete`/`deleteCore`, and the `PersistenceBackend.deleteStored` hook. Remove the `has`/`delete` rows from the contract suite and the per-backend specs (jsonl + sqlite each implemented `deleteStored` only to satisfy the hook — that implementation goes too). The backends are the [dual-backend](../../implemented/architecture/2026-06-14-session-persistence.md) design and otherwise out of scope, but removing a hook they implement for no consumer is part of removing the hook, not a backend redesign. +- Update every doc and source-comment reference to the removed methods — not only literal `has(`/`delete(`/`deleteStored` call spellings, but also `{@link has}`/`{@link delete}` JSDoc links and prose that counts the methods (removing 2 of the persistence service's 6 public methods makes any "six public methods" phrasing wrong). The implementing PR greps `has`/`delete`/`deleteStored`/`{@link `/`six ` across `docs/`, `packages/*/README.md`, and source comments, and fixes each. The known doc sites: the seam README ([packages/session-persistence/session-persistence/README.md](../../../../packages/session-persistence/session-persistence/README.md)'s `has(id)`/`delete(id)` API row and its "delegates its six public service methods" prose → four), the backend READMEs that describe `has`/`list` semantics ([packages/session-persistence/session-persistence-sqlite/README.md](../../../../packages/session-persistence/session-persistence-sqlite/README.md), [packages/session-persistence/session-persistence-jsonl/README.md](../../../../packages/session-persistence/session-persistence-jsonl/README.md) — reword "absent from `has()`/`list()`" to just `list()`), the service-map / seam docs in [docs/architecture.md](../../../architecture.md), and the persistence prose in the [session-persistence RFC](../../implemented/architecture/2026-06-14-session-persistence.md) and [shared write-coordinator RFC](../../implemented/architecture/2026-06-18-shared-persistence-write-coordinator.md). The known source-comment sites: the abstract `create()` JSDoc's `{@link has}/{@link list}` link ([packages/session-persistence/session-persistence/src/index.ts](../../../../packages/session-persistence/session-persistence/src/index.ts) — drop the `has` link), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying the `materialized` flag by "the signal `has`/`list` rely on" ([packages/session-persistence/session-persistence/src/coordinator.ts](../../../../packages/session-persistence/session-persistence/src/coordinator.ts)), the JSONL backend's `loadStored`/`deleteStored` comment, and the SQLite backend's `schema.ts` and `index.ts` comments that mention "absent from `has`/`list`" — all reworded to the surviving four-method, `list()`-only contract. ## Why not keep them as "the seam should be complete"? -The instinct that a persistence seam "should" offer delete, or a task executor "should" offer enumeration, is real — and it is exactly the speculative-completeness the pre-release stance warns against ([AGENTS.md](../../../../AGENTS.md): optimize for the correct foundation, not for hypothetical callers you do not have). Each of these is one method to re-add the day a consumer needs it: - -- A session-management UI that deletes old sessions will want `delete()` — add it then, designed against that UI's real needs (soft-delete? cascade? confirmation?), not guessed now. -- A `bash_list` tool that shows the model its running tasks will want `list()` — add it with the tool. +The instinct that a persistence seam "should" offer delete is real — and it is exactly the speculative-completeness the pre-release stance warns against ([AGENTS.md](../../../../AGENTS.md): optimize for the correct foundation, not for hypothetical callers you do not have). `delete()` is one method to re-add the day a consumer needs it: a session-management UI that deletes old sessions will want it — add it then, designed against that UI's real needs (soft-delete? cascade? confirmation?), not guessed now. Re-adding a seam method with a live consumer is cheap and better-designed than the speculative version, because the consumer pins the contract. Carrying it unused means every implementation (and every future backend) must implement and test a method that does nothing. ## Acceptance criteria -- `has`/`delete`/`deleteStored` are gone from the persistence seam, impl, and contract suites; `pnpm run knip` reports no new dead exports. (The bash `get`/`list` removal was reverted — see the implementation note above; those methods remain.) -- The remaining seam operations (`create`/`append`/`load`/`list` for persistence; `run`/`start`/`get`/`ownerOf`/`list`/`onTaskDone`/`readOutput`/`kill`/`resolve` for bash) are untouched; ACP `session/list`, bash tool flows, and crash-recovery behave identically. +- `has`/`delete`/`deleteStored` are gone from the persistence seam, impl, and contract suites; `pnpm run knip` reports no new dead exports. +- The remaining persistence operations (`create`/`append`/`load`/`list`) are untouched; ACP `session/list` and crash-recovery behave identically. - `pnpm run test:coverage` stays 100% per-file (the contract/spec rows for the removed persistence methods are deleted with them). -- Persistence seam READMEs and `docs/architecture.md` no longer list the removed `has`/`delete` methods. +- The persistence seam README and `docs/architecture.md` no longer list the removed `has`/`delete` methods. ## Risks - **`delete()` is the kind of operation a product eventually wants.** True — but "eventually" is the point. Deleting it now and re-adding it against a real consumer is strictly better than shipping a guessed contract. The dual backends each shed a `deleteStored` impl, which is a bounded edit in otherwise-out-of-scope packages. -- **`list()` on the bash seam is the natural seed for a future `bash_list`.** Acknowledged in the [pre-release foundation stance](../../../../AGENTS.md): add the seed when the tool lands. The executor still tracks tasks internally (the `tasks` map backs `ownerOf`/`readOutput`/`kill`); exposing an enumeration is a one-line re-add. -- **Low coupling.** Both removals are confined to their seam + impl + tests; no cross-package consumer references the removed methods, so there is no ripple beyond the docs. +- **Low coupling.** The removal is confined to the persistence seam + impl + tests; no cross-package consumer references the removed methods, so there is no ripple beyond the docs. -Modest size, but it converts two seams from "what an implementation must provide for nobody" back to "exactly what a consumer uses." +Modest size, but it converts the seam from "what an implementation must provide for nobody" back to "exactly what a consumer uses."