From 2eb6ad3260b2221dc87ccca385662a4617135896 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 21 Jun 2026 11:33:41 +0800 Subject: [PATCH] fix review findings: sync stale v1/removed-event doc references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's re-confirmation pass verified both blocker fixes correct but found doc/comment drift the fix commit missed: - session/index.ts + session/README.md: "minimal v1 header" → "minimal header (stamped with the current SESSION_FORMAT_VERSION)" — the version is 0, not 1. - session/index.ts deriveMessages comment listed "usage, and errors" as trace data — those standalone events no longer exist; only boundaries + chunks are. - session-persistence RFC: "no v1 migration" → the pinned-v0 pre-release stance. - collapse-trace-only RFC format-version note: reframed off the "bump the version and reject" wording (which now reads as the OTHER AGENTS.md stance) onto the pinned-0 unstable stance the session log actually uses. - agent-loop/loop.ts finishError JSDoc: "with a logged `error` event" → the failure is recorded on turn/end.reason (no standalone error event). - acp/acp-feature-support.md (two spots): usage is recorded on assistant/message now, not as standalone internal usage events. - Regenerate the cordis catalog (finishError JSDoc line shift). --- .../architecture/2026-06-14-session-persistence.md | 2 +- .../2026-06-20-collapse-trace-only-session-events.md | 2 +- packages/core/agent-loop/src/loop.ts | 4 ++-- packages/core/session/README.md | 2 +- packages/core/session/src/index.ts | 10 +++++----- packages/ui/acp/acp-feature-support.md | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md b/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md index a5cbd3bd61..7abe7ec8ed 100644 --- a/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md +++ b/docs/rfc/implemented/architecture/2026-06-14-session-persistence.md @@ -27,7 +27,7 @@ Key choices recorded here because they are durable, contested, and surprising: - **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](../simplification/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. +Format versioning: the header carries a `version`; `load` rejects any non-current version (no migration — the pre-release session format is pinned at `SESSION_FORMAT_VERSION = 0` and absorbs shape churn, per the AGENTS.md pre-release stance). 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. ## Consequences diff --git a/docs/rfc/implemented/simplification/2026-06-20-collapse-trace-only-session-events.md b/docs/rfc/implemented/simplification/2026-06-20-collapse-trace-only-session-events.md index f0bc1f107f..4ec39bcce4 100644 --- a/docs/rfc/implemented/simplification/2026-06-20-collapse-trace-only-session-events.md +++ b/docs/rfc/implemented/simplification/2026-06-20-collapse-trace-only-session-events.md @@ -38,6 +38,6 @@ Shipped as proposed, with one scope refinement (per AGENTS.md "RFCs are proposal - **Empty-content `assistant/message` hosts usage with no data loss.** The proof the proposal demanded (no persisted usage chunk becomes unrepresented) lands on the max-tokens path: a step cut off with usage but empty content (e.g. only a dropped tool call) previously emitted a standalone `usage`. It now records an empty-content `assistant/message { content: [], usage }`. To keep that from injecting a spurious content-less assistant turn into the provider transcript, `deriveMessages()` skips empty-content `assistant/message` events. A regression test asserts usage stays represented AND derived history is uncorrupted. -**Format version.** The persisted `SessionEventMap` shape changed (usage folded onto `assistant/message`, standalone `usage`/`error` removed, `step` on `turn/end.reason.error`), so per the AGENTS.md "bump the version and reject — don't migrate" policy a backend must reject any non-current log. The version literal is centralized in an exported `SESSION_FORMAT_VERSION` constant (read by both write sites and the coordinator's load-time check). While the harness is unreleased the on-disk format is pre-release/unstable, so the constant stays **`0`**: a breaking format change is absorbed at v0 (no monotonic bump until the first tagged release, when a specific format boundary becomes worth distinguishing) and old logs at any other version are rejected on load — there is no v0→vN migration (no persisted user data exists). `turn/end.reason.error.step` is required for newly-written logs. +**Format version.** The persisted `SessionEventMap` shape changed (usage folded onto `assistant/message`, standalone `usage`/`error` removed, `step` on `turn/end.reason.error`). The session log uses the **pinned-`0` "unstable / pre-release"** format stance (one of the two stances AGENTS.md § pre-release sanctions): `SESSION_FORMAT_VERSION` stays `0` and absorbs this and every other pre-release shape change without a monotonic bump — bumping on each tweak would dress up an unstable format as a sequence of stable boundaries that mean nothing yet. The constant is centralized in `dsh-session` and read by both write sites and the coordinator's load-time check, which rejects any non-`0` log (no migration — there is no persisted user data to preserve; a real monotonic policy begins at the first tagged release). `turn/end.reason.error.step` is required for newly-written logs. Usage is now observed on `assistant/message.usage`; an operational error's step on `turn/end.reason` for `kind: 'error'`. `agent/error` + logging are unchanged for live diagnostics. diff --git a/packages/core/agent-loop/src/loop.ts b/packages/core/agent-loop/src/loop.ts index c39924b34e..8d19c464fd 100644 --- a/packages/core/agent-loop/src/loop.ts +++ b/packages/core/agent-loop/src/loop.ts @@ -38,8 +38,8 @@ function toError(error: unknown): CodedError { * caller's try/catch), OR end the stream with a finish-error/aborted chunk * (the only option for adapters that can't throw mid-stream, e.g. * library-backed ones). This translates the latter into a thrown step error - * so the turn ends error/aborted with a logged `error` event, never as a - * normal `completed` assistant message. + * so the turn ends error/aborted (the failure recorded on `turn/end.reason`), + * never as a normal `completed` assistant message. * * `FinishReason` is merge-extensible (plugins/adapters can add `kind`s), so * the switch handles the known terminal-failure kinds and treats every other diff --git a/packages/core/session/README.md b/packages/core/session/README.md index 74c543fc07..682d5517dd 100644 --- a/packages/core/session/README.md +++ b/packages/core/session/README.md @@ -37,7 +37,7 @@ Plain class (not a Cordis Service). Create via `ctx.sessions.create()`. - `session.append(type, data): SessionEvent` — synchronous, never blocks on I/O. **Throws** if `data` is not losslessly JSON-serializable (BigInt, function, symbol, undefined, non-finite number, circular ref, or an exotic object like Map/Set/Date) — the event log is the durable source of truth, so this invariant is enforced at the source (exported as `isJsonValue` for backends to reuse on their replay/fork entry points). - `session.deriveMessages(): Message[]` — derive the LLM message history from the event log. Raw `assistant/chunk` events are skipped; `context/message` and `steering/message` render as tagged synthetic user messages. - `session.events`, `session.seq`, `session.id` -- `session.header: SessionHeader` — immutable creation metadata (`version`, `id`, `createdAt`, optional `cwd`/`parentSession`). Kept out of the event log (a storage concern, not replayable state); a minimal v1 header is synthesized for bare `Session` construction. +- `session.header: SessionHeader` — immutable creation metadata (`version`, `id`, `createdAt`, optional `cwd`/`parentSession`). Kept out of the event log (a storage concern, not replayable state); a minimal header (stamped with the current `SESSION_FORMAT_VERSION`) is synthesized for bare `Session` construction. ### Metadata types (`types.ts`) diff --git a/packages/core/session/src/index.ts b/packages/core/session/src/index.ts index 5f423f9b93..cef91c110c 100644 --- a/packages/core/session/src/index.ts +++ b/packages/core/session/src/index.ts @@ -79,9 +79,10 @@ export class Session { /** * Immutable creation metadata (format version, cwd, lineage). Supplied by * the store via `ctx.sessions.create()`. When a `Session` is constructed - * bare (tests, ad-hoc replay), a minimal v1 header is synthesized so - * `session.header` is always present. Kept out of the event log — it is a - * storage concern, not replayable conversation state. + * bare (tests, ad-hoc replay), a minimal header is synthesized (stamped with + * the current {@link SESSION_FORMAT_VERSION}) so `session.header` is always + * present. Kept out of the event log — it is a storage concern, not + * replayable conversation state. */ readonly header: SessionHeader @@ -180,8 +181,7 @@ export class Session { const messages: Message[] = [] for (const event of this.log) { // Intentionally non-exhaustive: only message-producing events derive - // history; turn/step boundaries, chunks, usage, and errors are - // trace/replay data. + // history; turn/step boundaries and chunks are trace/replay data. // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check switch (event.type) { case 'user/message': { diff --git a/packages/ui/acp/acp-feature-support.md b/packages/ui/acp/acp-feature-support.md index 6b171769d3..429c862b27 100644 --- a/packages/ui/acp/acp-feature-support.md +++ b/packages/ui/acp/acp-feature-support.md @@ -87,7 +87,7 @@ These are capabilities the bridge would *drive* on the editor. The harness runs | `available_commands_update` | S | ❌ | ✅ | ✅ | No slash commands advertised. | | `current_mode_update` | S | ❌ | ✅ | ✅ | No session modes. | | `config_option_update` | S | ❌ | ✅ | ✅ | No config options. | -| `usage_update` | S | ❌ | ✅ | ✅ | Token/cost reporting not surfaced (the harness HAS usage events internally). | +| `usage_update` | S | ❌ | ✅ | ✅ | Token/cost reporting not surfaced (the harness records token usage internally on `assistant/message`). | | `session_info_update` | S | ❌ | ⚠️ | ⚠️ | Session title/metadata not pushed. | ## 5. Tool-call rendering @@ -148,7 +148,7 @@ Ranked by how commonly the reference adapters ship them and how much UX they unl 6. **MCP passthrough** (`mcpServers` on `session/new` + `mcpCapabilities`). 7. **Richer prompt content** — image / embedded `resource` blocks (needs a multimodal model path). 8. **Diff + location tool rendering** — `diff` content and `locations` for edit tools. -9. **Usage reporting** (`usage_update`) — the harness already has the internal usage events. +9. **Usage reporting** (`usage_update`) — the harness already records token usage internally (on `assistant/message`). 10. **Editor filesystem delegation** (`fs/read_text_file` / `fs/write_text_file`) — lets the agent see unsaved buffers; lower priority since the harness has direct disk access. ## Out of scope