From 83e97ed222035616a1fff6d307649b9d1a920118 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 11:04:28 +0800 Subject: [PATCH 1/2] fix review findings: document the util/ group + align branded-ids RFC with dsh-brand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding packages/util/brand/ created a new top-level packages/util/ group that the hierarchy/dependency docs never enumerated. Document it: - Add packages/util/README.md, the group README (low-level zero-dependency utilities shared across groups; lists dsh-brand). - packages/README.md: add the util/ group to the group table, dsh-brand to the package table, and dsh-brand to the dependency graph. Correct the now-false "no harness deps" claims — dsh-llm and dsh-bash both depend on dsh-brand (verified dsh-bash imports Branded from dsh-brand, not dsh-llm; dsh-session and dsh-agent depend on it too). - Root AGENTS.md Repository Layout: add the util/ group with brand/. Align the implemented branded-ids RFC with what shipped: Branded lives in @deepseek-ai/dsh-brand (packages/util/brand/), and dsh-bash depends only on that utility package instead of dsh-llm. Fix the BashTaskId import source, the illustrative snippet, and the opening policy reference (now dsh-brand). --- AGENTS.md | 3 +++ .../architecture/2026-06-20-branded-ids.md | 6 +++--- packages/README.md | 11 +++++++---- packages/util/README.md | 9 +++++++++ 4 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 packages/util/README.md diff --git a/AGENTS.md b/AGENTS.md index dc4e65a962..ee17175a69 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,6 +71,9 @@ packages/ Harness packages, grouped by role at packages///. feeds stdin lines to the agent (shared by the demos) llm-replay/ record/replay adapter: short-circuits llm/stream from a recorded session JSONL (keyless snapshot tests) + util/ low-level zero-dependency utilities shared across groups + brand/ type-only Branded nominal-typing primitive (no runtime + code, no harness deps; owns the brand for cross-boundary ids) examples/ Runnable demos (not workspaces; see examples/AGENTS.md). echo-agent = mock model + echo tool + stdio UI + JSONL persistence, wired via cordis.yml. coding-agent = the real thing: DeepSeek V4 + bash tools diff --git a/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md b/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md index c203e76d42..0b4975089d 100644 --- a/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md +++ b/docs/rfc/implemented/architecture/2026-06-20-branded-ids.md @@ -4,7 +4,7 @@ Status: implemented (proposed and accepted 2026-06-20) ## Problem -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. +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 (owned by the type-only `@deepseek-ai/dsh-brand` package at `packages/util/brand/` — see its [README](../../../../packages/util/brand/README.md)) and a zero-cost cast factory per type. `dsh-brand` 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. @@ -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-brand` exactly as `SessionId`/`AgentId` already do. The brand primitive lives in the dependency-free `dsh-brand` utility package precisely so `dsh-bash` can brand its ids by depending on it alone — it never pulls in `dsh-llm` (or `dsh-session`) just to reach `Branded`. 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.) @@ -25,7 +25,7 @@ A type-only change. Brands are zero-cost casts; nothing about runtime behavior, Illustrative shape (the factory pattern is identical to the three existing brands): ```ts ignore-check -import type { Branded } from '@deepseek-ai/dsh-llm' +import type { Branded } from '@deepseek-ai/dsh-brand' /** A background bash task handle (generated `bash-N` by the local executor). */ export type BashTaskId = Branded<'BashTaskId'> diff --git a/packages/README.md b/packages/README.md index 139050683d..e02454bb48 100644 --- a/packages/README.md +++ b/packages/README.md @@ -14,17 +14,19 @@ Packages are grouped by modular role at `packages///`. The group dir | [`session-persistence/`](session-persistence/README.md) | Persistence capability family: the seam + JSONL/SQLite backends | Product — stable surface | | [`ui/`](ui/README.md) | Editor/client integration surfaces (the ACP bridge) | Product — stable surface | | [`support/`](support/README.md) | Dev/test/example infrastructure (invariants, stdio UI, replay adapter) | Support — lower compatibility expectations | +| [`util/`](util/README.md) | Low-level zero-dependency utilities shared across groups (the `Branded` primitive) | Support — small, stable, harness-dep-free | The split is the point: a package's group says whether it is part of the product API or support/test/example infrastructure, so release and removal decisions do not have to treat every package as an equal public contract. New packages join an existing group; adding a new top-level group is a deliberate act (extend the group READMEs and the hierarchy docs). ## Dependency graph ``` -dsh-llm (no harness deps — pure vocabulary) -dsh-bash (no harness deps — abstract executor seam) -dsh-session ← dsh-llm +dsh-brand (no harness deps — type-only Branded primitive) +dsh-llm ← dsh-brand (vocabulary; brands CallId) +dsh-bash ← dsh-brand (abstract executor seam; brands BashTaskId/OwnerToken) +dsh-session ← dsh-llm, dsh-brand dsh-system-prompt ← dsh-llm -dsh-agent ← dsh-llm, dsh-session +dsh-agent ← dsh-llm, dsh-session, dsh-brand dsh-tools ← dsh-llm, dsh-system-prompt, dsh-agent dsh-bash-local ← dsh-bash (BashExecutor impl) dsh-tool-bash ← dsh-bash, dsh-tools (bash tool schemas) @@ -61,6 +63,7 @@ The rule: plugins depend on interfaces, never on the concrete loop. `dsh-agent-l | `acp/` | `ui` | Agent Client Protocol bridge: serves the agent to an ACP editor over JSON-RPC stdio | (drives `ctx.agents`/`ctx.sessions`) | | `ui-stdio/` | `support` | Minimal stdio (readline) UI plugin: renders `agent/*` events, feeds stdin lines to the agent | (drives `ctx.agents`) | | `llm-replay/` | `support` | Record/replay adapter: short-circuits `llm/stream` with chunks from a recorded session JSONL (keyless snapshot tests) | (listens on `llm/stream`) | +| `brand/` | `util` | Type-only `Branded` nominal-typing primitive (no runtime code, no harness deps) | (none — type-only) | Each package has its own `README.md` with purpose, service API, events, extension points, and deliberate non-goals (TODOs). diff --git a/packages/util/README.md b/packages/util/README.md new file mode 100644 index 0000000000..ae73c8125f --- /dev/null +++ b/packages/util/README.md @@ -0,0 +1,9 @@ +# util/ — low-level shared utilities + +Zero-dependency primitives shared across the other groups. A package lands here when it owns a tiny, foundational type or helper that several capability families need but that belongs to none of them — keeping it out of any one group avoids a capability package depending on an unrelated one just to reach a shared primitive. These are **support** packages: small, stable, and free of harness dependencies. + +| Package | Role | +|---|---| +| `brand/` | The type-only `Branded` nominal-typing primitive (no runtime code, no harness deps) | + +`dsh-brand` is the canonical case: it owns ONLY the `Branded` helper, so a capability package can brand the ids it owns (`dsh-bash`'s `BashTaskId`/`OwnerToken`, `dsh-session`'s `SessionId`, …) by depending on `dsh-brand` alone, without pulling in an unrelated package just to reach `Branded`. 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 2/2] 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."