refactor(session): drop the dead mutable SessionSummary
SessionSummary (updatedAt/title/firstPrompt) and SessionPersistence.update() were dead state: zero production callers of update(), no production reader of updatedAt/firstPrompt, and ACP's title comes from a tool-call presenter, not storage. The live Session.header was already typed SessionHeader, so the summary only ever existed in the persistence layer, written and read by nothing but its own contract test. Delete it entirely (no SessionMeta alias — SessionMeta collapses to SessionHeader everywhere). This removes the JSONL .summary.json sidecar machinery, the SQLite title/first_prompt/updated_at columns and per-append updated_at bump, and the update() method from the abstract service and both backends. SQLite SCHEMA_VERSION goes 1->2 and openDatabase now rejects any non-current user_version (older or newer) — no migration, unreleased software. Net -400 lines, and it erases the JSONL-sidecar-vs-SQLite-column durability divergence that the upcoming write coordinator would otherwise have to model. Records the decision in docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md and migrates the 2026-06-14 session-persistence RFC's facts to current truth. Adds a standalone AGENTS.md section "Tests document behavior, not golden truth" (a passing test pins current behavior, not necessarily correct behavior) with the summary-drop as its worked example, and reinforces the no-migration pre-release stance.
This commit is contained in:
Tianyi Cui
@@ -45,7 +45,7 @@ Dependency rule: plugins depend on interface packages, never on `dsh-agent-loop`
|
||||
|---|---|---|---|
|
||||
| `ctx.llm` | `LlmService` | dsh-llm | adapter registry; `stream()` / `streamBlocks()` / `generate()` |
|
||||
| `ctx.sessions` | `SessionStore` | dsh-session | creates/holds event-sourced `Session`s |
|
||||
| `ctx.sessionPersistence` | `SessionPersistence` (abstract) | dsh-session-persistence | durable persistence seam: create/append/load/list/update sessions |
|
||||
| `ctx.sessionPersistence` | `SessionPersistence` (abstract) | dsh-session-persistence | durable persistence seam: create/append/load/list sessions |
|
||||
| `ctx.systemPrompt` | `SystemPrompt` | dsh-system-prompt | ordered sections + tool schemas → `assemble()` |
|
||||
| `ctx.tools` | `ToolRegistry` | dsh-tools | tool definitions; `execute()` through waterfall |
|
||||
| `ctx.agents` | `AgentRegistry` | dsh-agent | live `Agent` handles + the create/resume factory seam |
|
||||
@@ -85,7 +85,7 @@ A `Session` is an append-only log of typed `SessionEvent`s — the single source
|
||||
|
||||
Replay/fork = `ctx.sessions.create(id, { seed: seedEvents })`. Trace/telemetry = listen to `session/event`.
|
||||
|
||||
**Durability seam**: `session/event` is a synchronous notification; persistence plugins buffer (write-behind) and drain at the awaited `session/flush` checkpoint the loop fires at every turn end. The durable backend is a real **capability seam**: the abstract `SessionPersistence` service (`dsh-session-persistence`, `ctx.sessionPersistence`) defines create/append/load/list/update over the existing `SessionEvent` (no parallel persisted type), and `dsh-session-persistence-jsonl` is the first implementation — an append-only JSONL log per session with crash-safe atomic writes, crash recovery that PRESERVES an interrupted turn (closing it with a synthetic `turn/end {interrupted}` rather than truncating — a turn can be huge), and a read/replay path. Session metadata (format version, cwd, lineage) travels separately as `SessionMeta`, attached to a `Session` via `session.header`. Resuming a persisted session into a live agent is `ctx.agents.resume({ resumeSessionId })`. A second backend, `dsh-session-persistence-sqlite` (`node:sqlite`, one row per `SessionEvent` — the row shape `(session_id, seq, type, time, data)` maps 1:1 onto it), passes the same `runPersistenceContract` suite, proving the seam is genuinely backend-agnostic.
|
||||
**Durability seam**: `session/event` is a synchronous notification; persistence plugins buffer (write-behind) and drain at the awaited `session/flush` checkpoint the loop fires at every turn end. The durable backend is a real **capability seam**: the abstract `SessionPersistence` service (`dsh-session-persistence`, `ctx.sessionPersistence`) defines create/append/load/list over the existing `SessionEvent` (no parallel persisted type), and `dsh-session-persistence-jsonl` is the first implementation — an append-only JSONL log per session with crash-safe atomic writes, crash recovery that PRESERVES an interrupted turn (closing it with a synthetic `turn/end {interrupted}` rather than truncating — a turn can be huge), and a read/replay path. Session metadata (format version, cwd, lineage) travels separately as `SessionHeader`, attached to a `Session` via `session.header`. Resuming a persisted session into a live agent is `ctx.agents.resume({ resumeSessionId })`. A second backend, `dsh-session-persistence-sqlite` (`node:sqlite`, one row per `SessionEvent` — the row shape `(session_id, seq, type, time, data)` maps 1:1 onto it), passes the same `runPersistenceContract` suite, proving the seam is genuinely backend-agnostic.
|
||||
|
||||
## Prompt assembly (dsh-system-prompt)
|
||||
|
||||
|
||||
@@ -60,6 +60,7 @@ Do NOT write one for a mechanical or local choice (a variable name, a one-file r
|
||||
| [Rich ACP bash rendering — the terminal card (`_meta`) and command classification](implemented/2026-06-18-acp-terminal-and-tool-rendering.md) | 2026-06-18 |
|
||||
| [ACP snapshot tests — record-once / replay-deterministic](implemented/2026-06-19-acp-snapshot-tests.md) | 2026-06-19 |
|
||||
| [Real-API e2e in CI against the external DeepSeek API](implemented/2026-06-19-real-api-e2e-ci.md) | 2026-06-19 |
|
||||
| [Drop the mutable session summary](implemented/2026-06-19-drop-mutable-session-summary.md) | 2026-06-19 |
|
||||
|
||||
## Rejected
|
||||
|
||||
|
||||
@@ -16,15 +16,15 @@ 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`/`update`. 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`**) plus an atomic `.summary.json` sidecar for the mutable `SessionSummary`.
|
||||
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.
|
||||
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:
|
||||
|
||||
- **The canonical durable log persists every `SessionEvent` verbatim, including `assistant/chunk`.** `deriveMessages()` skips chunks, and a chunk-filtered rollout (Codex's `policy.rs`) is tempting — but `seq = log.length` and the load-validation `events[i].seq === i` require a *contiguous* log; filtering chunks out would leave holes and break both the contract and resume. A chunk-filtered projection is possible later as a derived view with its own renumbering, but it is NOT the canonical log.
|
||||
- **Append-only; a crashed turn is closed, never truncated.** Committed events — those at or below a flushed `turn/end` — are never rewritten. The loop only flushes at `turn/end`, so a crash can leave a durable log whose final turn never closed: real, fully-written events sit after the last `turn/end`. **A single turn can be huge in a long-horizon task** (many steps, large tool output spanning a long autonomous run), so discarding the interrupted turn would silently destroy a large amount of real work — truncating a turn is wrong. Instead, on reload `load` PRESERVES those events and CLOSES the orphaned turn by durably appending the minimal synthetic boundary events: an error `tool/result` for every `tool-call` the crash left unanswered, then a `step/end` if a step was still open, then a `turn/end` carrying the merge-extensible `{ kind: 'interrupted' }` reason (a marker that records the turn was cut short by a crash, not completed by the model — no loop ever emits it). The synthetic tool results matter for resume correctness: the loop logs the `assistant/message` (carrying the `tool-call` blocks) BEFORE running the tools, so a crash mid-tool leaves calls without results; `deriveMessages()` would then replay a dangling assistant tool-call, which every provider rejects as an invalid transcript on the next request. Answering each orphaned call with an error result keeps the rehydrated history valid. `load` returns the balanced log, so a resumed session is immediately usable. The ONLY thing discarded is a never-fully-written **torn tail fragment** — a final record whose bytes (JSONL) or row were never completely flushed; that fragment is not a valid event and is dropped before the synthetic closers are written. A parse error or `seq` gap in the COMMITTED region (at or before the last real `turn/end`) is genuine corruption and makes the session unloadable.
|
||||
- **File backend canonical, DB backend a proven drop-in.** `SessionEvent` maps 1:1 onto a row `(session_id, seq, type, time, data)` — `append` is INSERT (in a transaction asserting the contiguous-seq contract), `load` is SELECT … ORDER BY seq. `dsh-session-persistence-sqlite` is exactly this: a `SessionPersistence` subclass with no interface change (opencode runs this exact shape on SQLite/WAL), and it passes the same `runPersistenceContract` suite as the JSONL backend — so the contract holds both backends to identical semantics (lazy materialization, interrupted-turn close on load, contiguous-seq), expressed once over file bytes and once over rows.
|
||||
- **Metadata is out-of-log.** Format version, cwd, and lineage are storage concerns, not replayable conversation state, so they live in a `SessionMeta` (`SessionHeader & SessionSummary`) owned by `dsh-session` and attached to a `Session` via a new readonly `session.header` — never in `SessionEventMap`, never reaching `deriveMessages()`. The alternative (a merge-extensible `session/meta` event as log line 0) was rejected: an in-log event would ride along with a seeded/forked session for free, but metadata is not replayable state, so the explicit out-of-log header seam is the cleaner cost.
|
||||
- **Metadata is out-of-log.** Format version, cwd, and lineage are storage concerns, not replayable conversation state, so they live in a `SessionHeader` owned by `dsh-session` and attached to a `Session` via a new readonly `session.header` — never in `SessionEventMap`, never reaching `deriveMessages()`. The alternative (a merge-extensible `session/meta` event as log line 0) was rejected: an in-log event would ride along with a seeded/forked session for free, but metadata is not replayable state, so the explicit out-of-log header seam is the cleaner cost. (The header was originally split into an immutable `SessionHeader` plus a mutable `SessionSummary` whose union was `SessionMeta`; the mutable summary was later removed as dead state — see [Drop the mutable session summary](2026-06-19-drop-mutable-session-summary.md).)
|
||||
- **Resume is an async factory, not a change to synchronous create.** `ctx.agents.resume({ resumeSessionId })` awaits `ctx.sessionPersistence.load`, recreates the live session with the loaded events (so `lastTurnNumber`/`deriveMessages` continue), and starts a fresh agent on the resumed id (NOT `${agentId}-session`). The agent-loop does NOT hard-inject `sessionPersistence` (that would pend non-persistent demos forever); `resume` rejects with a clear error when it is absent.
|
||||
|
||||
Format versioning: the header carries a `version`; `load` rejects an unknown version (no v1 migration). Stated honestly: append-only + flush is robust to partial trailing writes (tolerated on load) but not to fsync-less power loss mid-line; a DB/WAL backend is the stronger option later.
|
||||
|
||||
@@ -0,0 +1,31 @@
|
||||
# RFC: Drop the mutable session summary
|
||||
|
||||
Status: implemented (proposed and accepted 2026-06-19)
|
||||
|
||||
## Context
|
||||
|
||||
The [session-persistence seam](2026-06-14-session-persistence.md) split a session's out-of-log metadata into two types owned by `dsh-session`: an immutable `SessionHeader` (`version`, `id`, `createdAt`, `cwd?`, `parentSession?`) written once at creation, and a mutable `SessionSummary` (`updatedAt`, `title?`, `firstPrompt?`) "updateable without touching the append-only log". Their union was `SessionMeta = SessionHeader & SessionSummary`, and the abstract `SessionPersistence` service carried a seventh method — `update(id, summary)` — for rewriting the summary. Each backend implemented the mutable store its own way: JSONL wrote a separate atomic `.summary.json` **sidecar** beside the log (temp-write + rename, best-effort), SQLite kept `updated_at`/`title`/`first_prompt` **columns** bumped inside the append transaction.
|
||||
|
||||
The summary was designed for a future session picker (recency ordering via `updatedAt`, a `title`/`firstPrompt` preview). That picker was never built. An audit of the whole repo found the entire `SessionSummary` surface is **dead state**:
|
||||
|
||||
- `SessionPersistence.update()` has **zero production callers** (every `.update(` hit is `createHash().update()` or a test).
|
||||
- `firstPrompt` is **never read** anywhere in production.
|
||||
- `title` *is* read in the ACP bridge — but from a tool-call **presenter** (`present.title`), never from stored session metadata.
|
||||
- `updatedAt` has **no consumer**: the only production caller of `list()` reads `meta.cwd` (a `SessionHeader` field) to validate a workspace on `session/load`; resume reads `createdAt`/`cwd`/`parentSession` — all header fields.
|
||||
- Decisively: the live `Session.header` was already typed `SessionHeader`, not `SessionMeta` — the summary never existed on the live session object; it lived only in the persistence layer, written and read by nothing but its own contract test.
|
||||
|
||||
## Decision
|
||||
|
||||
Delete the mutable session summary entirely. `SessionSummary` and the `SessionMeta` name are removed; the metadata a backend stores and returns is just `SessionHeader`. `SessionPersistence.update()` is removed from the abstract service and every backend. JSONL loses the whole sidecar machinery (`writeSidecar`/`readSidecar`/`touchSummary`/`removeSidecars`/`sidecarPath` and the load/list overlays); SQLite drops the `updated_at`/`title`/`first_prompt` columns and the per-append `updated_at` bump, and its `SCHEMA_VERSION` goes `1 → 2`.
|
||||
|
||||
Anything the summary was meant to provide is **derivable from the append-only log** when a consumer actually needs it (`firstPrompt` = first `user/message`; recency = the last event's `time` or the file mtime) or already lives in the immutable header (`createdAt`, `cwd`). The one thing *not* derivable — a user-*edited* title — had no implementation and is pure YAGNI; it can return as its own log event or header field if a real feature ever needs it.
|
||||
|
||||
This is recorded as a decision because it is **durable** (it narrows a public service contract and an on-disk format across two backends), **contested** (the summary was a deliberate forward-looking design, not an accident), and **surprising** (a future reader finding `SessionHeader` where the original RFC describes `SessionMeta` would otherwise ask why the summary vanished). It also unblocks the [shared persistence write coordinator](../proposed/2026-06-18-shared-persistence-write-coordinator.md): with no mutable summary, the coordinator's hook interface needs no `updateSummary` hook and the JSONL-sidecar-vs-SQLite-column durability divergence disappears, so the two backends' write paths converge.
|
||||
|
||||
## No migration
|
||||
|
||||
This is unreleased software (see [root AGENTS.md](../../../AGENTS.md) § "Pre-release stance: foundation over blast radius"), so there are no on-disk databases or logs to preserve. SQLite does not migrate a v1 database: the `openDatabase` guard now rejects any non-current on-disk `user_version` (`onDisk !== 0 && onDisk !== SCHEMA_VERSION`) — older *or* newer — so a stale v1 DB is cleanly rejected rather than half-read against the new column set. A fresh database stamps the current version; that is the only path that needs to work.
|
||||
|
||||
## What we gave up
|
||||
|
||||
A future session picker now has to derive its preview/ordering from the log (or reintroduce a typed field) rather than reading a ready-made summary row. That is the correct cost: a cache for a feature that does not exist is dead weight that every backend pays to maintain and every contract test pays to assert. The principle — **a passing test pins current behavior, not necessarily correct behavior; behavior can be an artifact of a past compromise** — is now recorded as a standalone convention in [root AGENTS.md](../../../AGENTS.md), with this change as its worked example.
|
||||
Reference in New Issue
Block a user