kevin/deepseek-harness
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs: resolve self-consistency follow-ups on RFCs 009-010

Browse Source 
- 009: the crash-tail "overwrite" contradicted the append-only contract.
  Name it explicitly as a one-time truncation-repair (ftruncate+fsync to
  the last complete turn/end byte offset) that removes only the
  never-committed crash tail; committed events are never rewritten.
  Qualify the append/impl/ADR wording to match.
- 010: remove the remaining concrete-loop references — the session/new
  and session/load table rows now point at the dsh-agent create/resume
  factory, and the Risks disposal line uses the interface-level settle
  signal (agent/status) instead of LoopAgent-only agent.done.
This commit is contained in:
Tianyi Cui
2026-06-14 21:57:59 +08:00
parent 64ce7caedc
commit de73afbe39
2 changed files with 8 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/rfc/009-session-persistence-and-resumability.md
+5 -5
View File
@@ -17,15 +17,15 @@ Mirror the codebase's capability-seam pattern ([ADR 0009](../adr/0009-capability
**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 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.
- `append(id, events: readonly SessionEvent[]): Promise<void>` — durably persist a batch (called from the flush drain). Committed events (at or below a flushed `turn/end`) are append-only and never rewritten; the only exception is the one-time truncation-repair of a never-committed crash tail on the first `append` after a `load` (see `load`). **Contract**: the first event's `seq` MUST equal the backend's stored next-seq after any such repair (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 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 *below* the last committed checkpoint — `load` returns events only up to the **last complete `turn/end`**, and a subsequent `append` runs the **truncation-repair** step (see impl) that physically discards the orphaned tail before writing. This keeps the append-only contract honest: only the never-committed crash tail is ever removed; events at or below a flushed `turn/end` are never rewritten.
- `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`. 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).
**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 committed events untouched: `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; **truncation-repair on the first append after a crash** — `load` computes the byte offset of the last complete `turn/end`, and the impl truncates the file to that offset (`ftruncate`, then `fsync`) before its first append, atomically discarding the never-committed tail. Only the uncommitted crash tail is ever removed. 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,11 +43,11 @@ 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/`: 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.
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 computes its byte offset; the first post-load `append` runs truncation-repair (`ftruncate` to that offset + `fsync`, discarding only the uncommitted crash tail) before writing (rejects mid-log gaps), `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.
7. Docs: update the "Event-sourced sessions" durability-seam paragraph and the "Deferred work" list in [docs/architecture.md](../architecture.md) (persistence is no longer deferred); sync the affected package READMEs/JSDoc (`dsh-session` for the `create`/`session.header` change); add a [cookbook](../cookbook/) note on writing a persistence backend; resolve the `TODO(review)` on the event vocabulary now that a real persistence plugin coexists with the loop. On implementation this likely graduates to an ADR — "persistence is an abstract service over the existing `SessionEvent`; verbatim append-only log; file canonical, DB drop-in" is durable, contested, and surprising enough to record.
7. Docs: update the "Event-sourced sessions" durability-seam paragraph and the "Deferred work" list in [docs/architecture.md](../architecture.md) (persistence is no longer deferred); sync the affected package READMEs/JSDoc (`dsh-session` for the `create`/`session.header` change); add a [cookbook](../cookbook/) note on writing a persistence backend; resolve the `TODO(review)` on the event vocabulary now that a real persistence plugin coexists with the loop. On implementation this likely graduates to an ADR — "persistence is an abstract service over the existing `SessionEvent`; verbatim append-only log (committed events never rewritten; only an uncommitted crash tail is truncation-repaired); file canonical, DB drop-in" is durable, contested, and surprising enough to record.
## Risks
docs/rfc/010-acp-agent-client-protocol.md
+3 -3
View File
@@ -21,8 +21,8 @@ The mapping between ACP and existing harness seams — each row names the seam a
| ACP (client ⇄ agent) | Harness seam | Notes |
|---|---|---|
| `initialize` | static handler | negotiate `protocolVersion` (echo the supported version, else error); advertise text-only `promptCapabilities` and `loadSession: true`; report agent name/version |
| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | a new `agentLoop` create seam (see Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see RFC 011); `cwd` validated (require absolute) with "launch the server in the workspace root" documented until the workdir seam exists; `mcpServers` ignored (no `mcpCapabilities` advertised); non-empty `additionalDirectories` rejected for the MVP (the bridge cannot yet widen bash/tool filesystem scope, so silently ignoring them would desync the client's filesystem-scope UI) |
| `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | RFC 009's async `agentLoop` resume seam | load `{ meta, events }`, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `additionalDirectories` rejected as in `session/new` |
| `session/new {cwd, mcpServers, additionalDirectories}` → `{sessionId}` | the `dsh-agent` create factory (see Dependency note + Plan) | the seam must accept `{ sessionId, meta }` so the ACP-generated `sessionId` becomes the live/persisted session id and the validated `cwd` is attached as the `SessionHeader` (today `AgentLoop.create(id)` hardcodes `${id}-session` and takes no metadata); reject a 2nd session (single-session MVP, see RFC 011); `cwd` validated (require absolute) with "launch the server in the workspace root" documented until the workdir seam exists; `mcpServers` ignored (no `mcpCapabilities` advertised); non-empty `additionalDirectories` rejected for the MVP (the bridge cannot yet widen bash/tool filesystem scope, so silently ignoring them would desync the client's filesystem-scope UI) |
| `session/load {sessionId, cwd, mcpServers, additionalDirectories}` | the `dsh-agent` resume factory (RFC 009 + Dependency note) | load `{ meta, events }`, seed the session, re-derive history via `deriveMessages()`, replay prior turns to the client as `session/update` per the ACP load contract; `additionalDirectories` rejected as in `session/new` |
| `session/prompt {prompt}` | `agent.send()` (idle) | text blocks → `TextBlock`; reject image/audio per advertised capabilities; one in-flight prompt per session |
| resolve `session/prompt` → `{stopReason}` | `agent/turn-end` (extended, see Plan) | map the harness kebab `TurnEndReason` to the ACP snake_case `StopReason` wire enum: `completed`→`end_turn`, `max-tokens`→`max_tokens`, `aborted`(cancel)→`cancelled`, plus `refusal`/`max_turn_requests` when applicable; honor the batch-into-one-turn and send-not-synchronously-running settle semantics |
| `session/update: agent_message_chunk` | `agent/stream-chunk` `text-delta` only | do NOT also emit on `block-end(TextBlock)` — it carries the fully-assembled block and would duplicate the streamed text |
@@ -64,7 +64,7 @@ New third-party runtime dependency plus protocol drift: `@agentclientprotocol/sd
Turn-settle and prompt-correlation hazards: honor "queued messages batch into one turn" and "`send()` does not synchronously flip to running" (see `stdio-chat.ts` and the defensive-patterns section of [docs/architecture.md](../architecture.md)); gate resolution on an observed running→idle transition and handle the empty-prompt / no-work branch so an RPC can't hang.
Permission-await and disposal hangs: a pending `request_permission` whose connection closes or whose turn aborts must settle exactly once; disposal must reach quiescence (await `agent.done`), not orphan awaits on a closed pipe.
Permission-await and disposal hangs: a pending `request_permission` whose connection closes or whose turn aborts must settle exactly once; disposal must reach quiescence (observe the interface-level settle signal — `agent/status` reaching `idle`/`disposed`, since `agent.done` is `LoopAgent`-only), not orphan awaits on a closed pipe.
The 100% per-file coverage gate (repo policy) makes a branch-heavy protocol bridge real work. Accepted deliberately, surfaced so it isn't a surprise at PR time.