Add a second axis to every RFC — its class (feature, bug-fix,
simplification, architecture, process, testing) — encoded in the path
as docs/rfc/{lifecycle}/{class}/file.md. The folder is the label, so
the closed set is enforced by structure rather than a parsed field.
Two new doc-sync gates back it:
- verify-rfc-classification: every RFC sits in a valid class folder and
the README index lists it under the matching lifecycle→class heading.
- verify-doc-refs: every docs/*.md path cited in a packages|examples TS
comment resolves — closes a drift class verify-md-links can't see, and
catches the four comment refs this reorg moved.
The README gains a Classification section explaining the taxonomy and
per-class index sub-sections. A self-referential process RFC records why
the scheme is path-encoded and gated.
9.8 KiB
RFC: Prune dead methods from the persistence and bash capability seams
Status: proposed
Problem
Two capability seams (interface / implementation / consumer) 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.
SessionPersistence.has() and .delete()
The abstract service declares four operations beyond create/append: load, list, has, delete (packages/session-persistence/src/index.ts:142-151). Production consumers of ctx.sessionPersistence use only two of them: the agent-loop resume path calls load() (packages/agent-loop/src/index.ts:176-194), and the ACP bridge calls list() for session/list (packages/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/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.
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/src/coordinator.ts:298-310). delete() drags the deleteStored backend hook (coordinator.ts:99, coordinator.ts:313-319) that every backend must implement. This is the drop-mutable-session-summary 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/src/index.ts:88-107), both implemented by LocalBashExecutor (packages/bash-local/src/index.ts:179-191). 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/tests/service.spec.ts, packages/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/tool-bash/tests/tools.spec.ts, packages/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.
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'shas/delete/deleteCore, and thePersistenceBackend.deleteStoredhook. Remove thehas/deleterows from the contract suite and the per-backend specs (jsonl + sqlite each implementdeleteStoredonly to satisfy the hook — that implementation goes too). The backends are the dual-backend 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 theLocalBashExecutorimpls. The seam/executor specs that assertget()/list()directly (bash/tests/service.spec.ts,bash-local/tests/executor.spec.ts) lose those assertions (the behavior is being removed). Thedsh-tool-bashtests that reach throughctx.bash.get(id)to awaitdone, readstatus, or inspect task fields switch to the public completion/status seam they should use —onTaskDone(or thedonepromise and status thestart()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(/deleteStoredcall 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 grepshas/delete/get/list/deleteStored/{@link/sixacrossdocs/,packages/*/README.md, and source comments, and fixes each. The known doc sites: the seam READMEs (packages/session-persistence/README.md'shas(id)/delete(id)API row and its "delegates its six public service methods" prose → four, packages/bash/README.md'sget(id)/list()row), the backend READMEs that describehas/listsemantics (packages/session-persistence-sqlite/README.md, packages/session-persistence-jsonl/README.md — reword "absent fromhas()/list()" to justlist()), the service-map / seam docs in docs/architecture.md, and the persistence prose in the session-persistence RFC and shared write-coordinator RFC. The known source-comment sites: the abstractcreate()JSDoc's{@link has}/{@link list}link (packages/session-persistence/src/index.ts — drop thehaslink), the coordinator's "six public methods"/"six public service methods" module + class JSDoc and its lazy-materialization JSDoc justifying thematerializedflag by "the signalhas/listrely on" (packages/session-persistence/src/coordinator.ts), the JSONL backend'sloadStored/deleteStoredcomment, and the SQLite backend'sschema.tsandindex.tscomments that mention "absent fromhas/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: 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_listtool that shows the model its running tasks will wantlist()— add it with the tool.
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/deleteStoredandget/listare gone from their seams, impls, and contract suites;pnpm run knipreports no new dead exports.- The remaining seam operations (
create/append/load/listfor persistence;run/start/ownerOf/onTaskDone/readOutput/kill/resolvefor bash) are untouched; ACPsession/list, bash tool flows, and crash-recovery behave identically. pnpm run test:coveragestays 100% per-file (the contract/spec rows for the removed methods are deleted with them).- Seam READMEs and
docs/architecture.mdno longer list the removed 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 adeleteStoredimpl, which is a bounded edit in otherwise-out-of-scope packages.list()on the bash seam is the natural seed for a futurebash_list. Acknowledged in the pre-release foundation stance: add the seed when the tool lands. The executor still tracks tasks internally (thetasksmap backsownerOf/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.
Modest size, but it converts two seams from "what an implementation must provide for nobody" back to "exactly what a consumer uses."