docs: address review on RFCs 009-011

Resolve the inline review feedback on PR #18 (all verified against the
codebase, the published @agentclientprotocol/sdk@0.25.1 tarball, and
Cordis fiber semantics):

- 009: dsh-session owns SessionMeta (persistence re-exports) to avoid a
  package cycle; split mutable summary into a sidecar so the event log
  stays append-only and list/load can return it; pick one load-repair
  rule (resume from the last complete turn/end, overwrite the orphan).
- 010: SDK has a zod peer dep + runtime zod/v4 import (drop "zero runtime
  deps"); session/new needs a create seam taking {sessionId, meta};
  propose an abstract create/resume factory on dsh-agent so the bridge
  depends on the interface not the loop, and observe agent/status for
  quiescence since agent.done is LoopAgent-only; add the explicit
  TurnEndReason -> ACP StopReason wire mapping + test; reject non-empty
  additionalDirectories for the MVP; remove the EOF blank line.
- 011: ctx.extend() does not create a disposable fiber — use a real
  per-session disposer scope.
This commit is contained in:
Tianyi Cui
2026-06-14 21:43:16 +08:00
parent a44f7f3486
commit 64ce7caedc
3 changed files with 16 additions and 15 deletions

View File

@@ -14,18 +14,18 @@ The event-sourced model (ADR 0003) makes the log the single source of truth and
Mirror the codebase's capability-seam pattern ([ADR 0009](../adr/0009-capability-seams.md), the `bash` template: an abstract `Service` interface, a concrete implementation, and consumers) for persistence.
**1. Abstract service `SessionPersistence`** — a new interface package `@deepseek-ai/dsh-session-persistence` owning `ctx.sessionPersistence`, depending only on `cordis` and `dsh-session`. Its persisted unit IS `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type. The method surface:
**1. Abstract service `SessionPersistence`** — a new interface package `@deepseek-ai/dsh-session-persistence` owning `ctx.sessionPersistence`, depending only on `cordis` and `dsh-session`. The `SessionHeader`/`SessionSummary`/`SessionMeta` types are owned by **`dsh-session`** (they live beside `SessionId` because `Session.header` is typed by them — see item 3a); the persistence package imports/re-exports them. Owning them in the persistence package would force `dsh-session` to depend back on it to type `Session.header`, a package cycle. Its persisted unit IS `SessionEvent` (`{ type, seq, time, data }`), reused verbatim — no conversion type. The method surface:
- `create(meta: SessionMeta): Promise<void>` — register a new session's header. The backend MAY defer the physical write until the first `append` (lazy materialization); `has`/`list` semantics for a zero-event session are specified, not left implicit.
- `append(id, events: readonly SessionEvent[]): Promise<void>` — durably persist a batch (called from the flush drain). Append-only; never rewrites. **Contract**: the first event's `seq` MUST equal the backend's stored next-seq (a DB impl asserts this inside a transaction; the file impl appends at EOF). All persisted `event.data` MUST be JSON-serializable.
- `load(id): Promise<{ meta: SessionMeta; events: SessionEvent[] }>` — replay header plus the full event log. Returns `meta` AND `events` so the live session is reconstructed with its `cwd`/lineage, not just its log. **Validation**: `events[i].seq === i` (contiguous, zero-based) or the load rejects.
- `load(id): Promise<{ meta: SessionMeta; events: SessionEvent[] }>` — replay header plus the event log up to the last durable checkpoint. Returns `meta` AND `events` so the live session is reconstructed with its `cwd`/lineage, not just its log. **Validation/repair**: the returned events MUST be contiguous (`events[i].seq === i`); a parse error or `seq` gap in the *middle* of the log makes the session unloadable (reject). The loop only flushes at `turn/end`, so a crash can leave a half-written final turn — `load` returns events only up to the **last complete `turn/end`** and the impl sets its write cursor to that length, so a subsequent `append` overwrites the orphaned tail rather than appending past it (no seq desync). This is the single chosen behavior: reject incomplete final turns, resume from the last clean checkpoint.
- `list(): Promise<SessionMeta[]>` — lightweight listing from headers, no full-log parse.
- `has(id)` / `delete(id)` — existence and removal.
- `update(id, summary: Partial<SessionSummary>): Promise<void>` — update mutable header fields without touching the append-only event log.
The new `SessionMeta` splits into an immutable `SessionHeader` (`{ id, version, createdAt, cwd?, parentSession? }`) and a mutable `SessionSummary` (`{ updatedAt, title?, firstPrompt? }`); `SessionMeta = SessionHeader & SessionSummary`. Every reference system writes such a header (pi's `version: 3` header line, Codex's `SessionMeta`, Claude Code's tail metadata). It is kept *separate from the event log* deliberately: format-version, cwd, and lineage are storage concerns, not conversation events, so they stay out of `SessionEventMap` and never reach `deriveMessages()`. The alternative — a merge-extensible `session/meta` event as log line 0 — was considered: an in-log event would ride along with a seeded/forked session for free, whereas an out-of-log header must be threaded through a seam (item 3a). It was rejected because metadata is not replayable conversation state; the explicit metadata seam is the cleaner cost.
**2. Concrete impl `SessionPersistenceJsonl`** — a new package `@deepseek-ai/dsh-session-persistence-jsonl`. One file per session: a header line (`{ type: 'session', version, id, cwd, createdAt, parentSession? }`) followed by one `SessionEvent` JSON per line. On disk: a configured root with per-cwd subdirectories (pi-style `--encoded-cwd--/<timestamp>_<id>.jsonl`) so sessions group by project. `list()` reads only each file's header line. Resilience over the example: append plus explicit flush; on `load`, drop only a corrupt/incomplete *trailing* line but reject a parse error or `seq` gap in the *middle* (a hole would desync `seq = log.length` appends); lazy materialization (no file until the first real event, so abandoned sessions leave nothing behind).
**2. Concrete impl `SessionPersistenceJsonl`** — a new package `@deepseek-ai/dsh-session-persistence-jsonl`. Per session: an append-only `.jsonl` event log (a `SessionHeader` line — `{ type: 'session', version, id, cwd, createdAt, parentSession? }` — followed by one `SessionEvent` JSON per line), plus a small sidecar `.<id>.summary.json` holding the mutable `SessionSummary` (`updatedAt`, `title?`, `firstPrompt?`). The split keeps the event log strictly append-only: `update(id, summary)` rewrites only the tiny sidecar (atomic temp-write + rename), never the log; `load`/`list` read the header line from the log and merge the sidecar to return a full `SessionMeta` (sidecar absent → summary fields default). On disk: a configured root with per-cwd subdirectories (pi-style `--encoded-cwd--/<timestamp>_<id>.jsonl`) so sessions group by project. `list()` reads only each file's header line plus its sidecar. Resilience over the example: append plus explicit flush; **on `load`, an incomplete final turn is rejected back to the last complete `turn/end`** — see the load-repair rule below; lazy materialization (no file until the first real event, so abandoned sessions leave nothing behind).
**2a. `assistant/chunk` persistence policy** (decided here, not deferred). The loop appends one `assistant/chunk` per raw stream chunk, but `deriveMessages()` skips chunks entirely — the assembled `assistant/message` is authoritative for history. It is tempting to drop chunks from the durable log (Codex's `policy.rs` filters deltas from its rollout). But `seq = log.length` and the load-validation `events[i].seq === i` require a *contiguous* log: filtering chunks out would leave holes (`[0,1,4,6,8]`) and break both the contract and resume. **Decision: the canonical durable log persists every `SessionEvent` verbatim, including `assistant/chunk`** — this keeps `seq` contiguous, keeps "persist `SessionEvent` directly" literally true, and lets RFC 010 replay streamed turns on `session/load`. A chunk-filtered *projection* (for export or a compacted listing) is possible later as a derived view with its own renumbering, but it is NOT the canonical log and NOT the default. The round-trip test asserts byte-identical events.
@@ -43,7 +43,7 @@ Seed handling has two cases that the plugin must distinguish, and neither is the
1. Interface package `packages/session-persistence/` per [the cookbook](../cookbook/adding-a-package.md): abstract `SessionPersistence extends Service` (`super(ctx, 'sessionPersistence')`), the `declare module 'cordis'` ctx key, the `SessionHeader`/`SessionSummary`/`SessionMeta` types, and method contracts documented in JSDoc (durability, append-only, contiguous-seq, JSON-serializable, error semantics).
2. `dsh-session` changes: add the three meta types beside `SessionId`; add the metadata seam. `SessionStore.create(id?, seed?)` becomes `create(id?, options?: { seed?; meta? })` — a breaking signature change (callers pass `seed` positionally today: `AgentLoop.create`, and ~20+ call sites across `session`/`invariants`/`agent-loop` tests), so either migrate every caller or keep a deprecated overload during transition. Add a readonly `session.header`. Persistence captures the header on `session/created` (a synchronous event), so the impl must hold a per-session init promise that every `session/flush` awaits before `append`, and must seed existing live sessions via `ctx.sessions.list()` on plugin apply (HMR does not replay `session/created`, mirroring `dsh-invariants`). Do NOT add meta to `SessionEventMap`.
3. JSONL impl `packages/session-persistence-jsonl/`: header plus event lines (all events, verbatim — see 2a), sanitized per-cwd dirs and filenames, lazy materialize (header + first batch written atomically), append plus flush, trailing-line-tolerant and mid-gap-rejecting `load` that truncates back to the last complete `turn/end` rather than resuming a half-written turn, header-only `list`, the per-session write cursor, and snapshot-on-buffer. `static Config` for root dir and flush policy.
3. JSONL impl `packages/session-persistence-jsonl/`: append-only event log (header line + all events verbatim — see 2a) plus an atomic `.summary.json` sidecar for mutable fields, sanitized per-cwd dirs and filenames, lazy materialize (header + first batch written atomically), append plus flush, a `load` that returns events up to the last complete `turn/end` and sets the write cursor there (rejects mid-log gaps; overwrites the orphaned tail on next append), `list` from header + sidecar, the per-session write cursor, and snapshot-on-buffer. `static Config` for root dir and flush policy.
4. Generalize the write-path plugin: the impl subscribes to `session/created` (capture header, persist any seed for forks), `session/event` (snapshot + buffer), and `session/flush`/dispose (drain), replacing the per-example `session-jsonl.ts`; both examples load the shared plugin.
5. Resume seam: the async `AgentLoop.resume(agentId, resumeSessionId, options?)`; initialize the write cursor to the loaded length; verify `lastTurnNumber`/`deriveMessages` continuity. `AgentLoop` does NOT hard-inject `sessionPersistence` (that would break non-persistent examples) — `resume` checks for the service and throws a typed "persistence not configured" error; consumers that need resume (ACP) load the persistence plugin.
6. Tests (event-sourcing makes these strong): a round-trip property (persist an arbitrary log → reload → byte-identical events and identical `deriveMessages()` output — the replay equivalence ADR 0003 promises); resume vs fork (resume appends no duplicate seqs; a fork persists its seed once); contiguous-seq enforcement (mid-log gap rejected, re-append of a stored seq rejected); crash tolerance (a truncated final turn truncates back to the last `turn/end`); JSON-serializability rejection for a plugin-added event carrying non-serializable data; mutation-after-`session/event` does not corrupt the persisted snapshot; SessionId path-traversal is neutralized; lazy materialization (no file until the first event); `has`/`list` semantics for a zero-event session; HMR-safety (dispose drains buffers and closes file handles; apply seeds existing live sessions); concurrent sessions do not cross buffers.