fix review findings: bump session format version + restore late turn-end warn
Codex review of the trace-event fold found two merge-blockers. Blocker #1 — format version. Folding usage onto assistant/message and removing the standalone usage/error events changed the persisted SessionEventMap shape, which per the AGENTS.md "bump the version and reject — don't migrate" policy requires a backend to reject any non-current log. Centralize the version in an exported SESSION_FORMAT_VERSION constant (dsh-session), read by both write sites (Session constructor default, SessionStore.prepare header) and the coordinator's load-time assertVersion check. The constant is pinned at 0: while unreleased the on-disk format is unstable/pre-release, so breaking shape churn is absorbed at v0 (no monotonic bump until the first tagged release) and any non-0 log is rejected on load — no migration. Update every test/fixture/doc that stamps a currently-written header to the constant, bump the ACP snapshot fixture + golden headers to v0, and keep the version-rejection test meaningful by switching its bad value to a clearly non-current 99. AGENTS.md documents both the monotonic (SQLite SCHEMA_VERSION) and pinned-0 (session log) pre-release stances. Blocker #2 — restore the late turn-end warn. failTurn now sets the error reason only while the turn is still open; once turn/end is appended (a throwing agent/turn-end listener after closeTurn) the reason can no longer reach the durable log, so the late throw is logged via ctx.logger.warn instead of vanishing into a futile post-close assignment. A regression test asserts the warn fires. Also guard the normal-step assistant/message append with the same content-or-usage condition as the max-tokens branch (a content-less, usage-less step records no trace-only row), with a covering test.
This commit is contained in:
Tianyi Cui
@@ -25,7 +25,7 @@ The JSONL durable session-persistence backend — a concrete `SessionPersistence
|
||||
- **Append-only.** Committed events (at or below a flushed `turn/end`) are never rewritten. Subsequent appends are line appends at EOF + `fsync`.
|
||||
- **Crash recovery — close, don't truncate.** A crash can leave a log whose final turn never closed (real events after the last `turn/end`). `load` PRESERVES those events (a turn can be huge — they are real work) and closes the orphaned turn by durably appending synthetic boundary events: an error `tool/result` for every `tool-call` the crash left unanswered (the loop logs the assistant message before running the tools, so a mid-tool crash leaves dangling calls — and `deriveMessages()` would replay an assistant tool-call with no result, which providers reject), then a `step/end` if a step was open, then `turn/end {kind:'interrupted'}`, returning a balanced log. Only a never-fully-written **torn tail fragment** (a final line with no newline / unparseable) is `ftruncate`d away before the closers are written. See [session persistence](../../../docs/rfc/implemented/architecture/2026-06-14-session-persistence.md).
|
||||
- **Contiguous-seq.** `load` rejects a mid-log parse error or `seq` gap (unloadable); `append` rejects a batch whose first `seq` does not continue the stored log, and rejects non-JSON-serializable `event.data` naming the offending event type.
|
||||
- **Format version.** Only v1 is supported; `load` rejects an unknown version. While the harness is unreleased a format change bumps the version and rejects non-current logs — there is no migration (no persisted user data to preserve).
|
||||
- **Format version.** Only the current `SESSION_FORMAT_VERSION` (v0) is supported; `load` rejects any other version. While the harness is unreleased the on-disk format is pre-release/unstable: a breaking format change is absorbed at v0 (no bump until the first tagged release) and non-current logs are rejected — there is no migration (no persisted user data to preserve).
|
||||
|
||||
## Write path
|
||||
|
||||
|
||||
@@ -249,7 +249,7 @@ describe('SessionPersistenceJsonl: durability and crash semantics', () => {
|
||||
|
||||
it('path-traversal session ids are neutralized (no escape from root)', async () => {
|
||||
const evil = SessionId('../../etc/pwn')
|
||||
const m = { version: 1, id: evil, createdAt: 1 }
|
||||
const m = { version: 0, id: evil, createdAt: 1 }
|
||||
await ctx.sessionPersistence.create(m)
|
||||
await ctx.sessionPersistence.append(evil, oneTurnLog())
|
||||
// The file lives UNDER root, not at ../../etc.
|
||||
@@ -310,7 +310,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
|
||||
it('a seq gap after the last turn/end bounds the preserved tail (torn fragment tolerated)', () => {
|
||||
const log = [
|
||||
JSON.stringify({ type: 'session', version: 1, id: 'g', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'session', version: 0, id: 'g', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }),
|
||||
JSON.stringify({ type: 'step/start', seq: 2, time: 2, data: { turn: 1, step: 1 } }), // gap: missing seq 1
|
||||
].join('\n') + '\n'
|
||||
@@ -323,7 +323,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
|
||||
it('rejects a seq gap BEFORE a later committed turn/end (committed data damaged)', () => {
|
||||
const log = [
|
||||
JSON.stringify({ type: 'session', version: 1, id: 'g2', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'session', version: 0, id: 'g2', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }),
|
||||
JSON.stringify({ type: 'step/start', seq: 2, time: 2, data: { turn: 1, step: 1 } }), // gap: missing seq 1
|
||||
JSON.stringify({ type: 'turn/end', seq: 3, time: 3, data: { turn: 1, reason: { kind: 'completed' } } }),
|
||||
@@ -335,7 +335,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
|
||||
it('rejects a corrupt line BEFORE a later committed turn/end (committed data damaged)', () => {
|
||||
const log = [
|
||||
JSON.stringify({ type: 'session', version: 1, id: 'c', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'session', version: 0, id: 'c', createdAt: 1 }),
|
||||
'{not json', // corrupt, sits in the committed region (a turn/end follows)
|
||||
JSON.stringify({ type: 'turn/end', seq: 1, time: 2, data: { turn: 1, reason: { kind: 'completed' } } }),
|
||||
].join('\n') + '\n'
|
||||
@@ -343,7 +343,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
})
|
||||
|
||||
it('a header-only log (no event lines at all) preserves nothing — committedBytes is the header', () => {
|
||||
const log = JSON.stringify({ type: 'session', version: 1, id: 'h0', createdAt: 1 }) + '\n'
|
||||
const log = JSON.stringify({ type: 'session', version: 0, id: 'h0', createdAt: 1 }) + '\n'
|
||||
const scanned = scanLog(Buffer.from(log))
|
||||
expect(scanned.events).toEqual([])
|
||||
// committedBytes falls back to the header line's end (no preserved events).
|
||||
@@ -352,7 +352,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
|
||||
it('a corrupt line after the last turn/end bounds the preserved tail', () => {
|
||||
const log = [
|
||||
JSON.stringify({ type: 'session', version: 1, id: 'c2', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'session', version: 0, id: 'c2', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }),
|
||||
'{not json', // corrupt crash fragment, no turn/end committed
|
||||
].join('\n') + '\n'
|
||||
@@ -363,7 +363,7 @@ describe('SessionPersistenceJsonl: scanLog unit', () => {
|
||||
|
||||
it('tolerates a seq gap AFTER a turn/end (uncommitted tail)', () => {
|
||||
const log = [
|
||||
JSON.stringify({ type: 'session', version: 1, id: 't', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'session', version: 0, id: 't', createdAt: 1 }),
|
||||
JSON.stringify({ type: 'turn/start', seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message', source: { kind: 'user' } } } }),
|
||||
JSON.stringify({ type: 'turn/end', seq: 1, time: 2, data: { turn: 1, reason: { kind: 'completed' } } }),
|
||||
JSON.stringify({ type: 'step/start', seq: 9, time: 3, data: { turn: 2, step: 1 } }), // gap in uncommitted tail
|
||||
@@ -442,7 +442,7 @@ describe('SessionPersistenceJsonl: edge cases', () => {
|
||||
// field is tolerated by the header type guard) and confirm list() reads it.
|
||||
const bucket = join(root, '_no-cwd')
|
||||
await mkdir(bucket, { recursive: true })
|
||||
const bigHeader = JSON.stringify({ type: 'session', version: 1, id: 'big', createdAt: 1, pad: 'x'.repeat(9000) })
|
||||
const bigHeader = JSON.stringify({ type: 'session', version: 0, id: 'big', createdAt: 1, pad: 'x'.repeat(9000) })
|
||||
await writeFile(join(bucket, 'big.jsonl'), bigHeader + '\n')
|
||||
const ids = (await ctx.sessionPersistence.list()).map(x => x.id)
|
||||
expect(ids).toContain('big')
|
||||
|
||||
@@ -25,7 +25,7 @@
|
||||
*/
|
||||
|
||||
import { Context } from 'cordis'
|
||||
import { interruptedTurnClosers } from '@deepseek-ai/dsh-session'
|
||||
import { interruptedTurnClosers, SESSION_FORMAT_VERSION } from '@deepseek-ai/dsh-session'
|
||||
import type { Session, SessionEvent, SessionId, SessionHeader } from '@deepseek-ai/dsh-session'
|
||||
import { assertSerializable, seedCoversPrefix } from './index.ts'
|
||||
|
||||
@@ -319,8 +319,8 @@ export class PersistenceCoordinator<TornMarker = unknown> {
|
||||
}
|
||||
|
||||
private assertVersion(meta: SessionHeader): void {
|
||||
if (meta.version !== 1) {
|
||||
throw new Error(`unsupported session format version ${meta.version} for "${meta.id}" (only v1 is supported)`)
|
||||
if (meta.version !== SESSION_FORMAT_VERSION) {
|
||||
throw new Error(`unsupported session format version ${meta.version} for "${meta.id}" (only v${SESSION_FORMAT_VERSION} is supported)`)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -9,7 +9,7 @@
|
||||
*/
|
||||
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { SessionId } from '@deepseek-ai/dsh-session'
|
||||
import { SESSION_FORMAT_VERSION, SessionId } from '@deepseek-ai/dsh-session'
|
||||
import type { SessionEvent, SessionHeader } from '@deepseek-ai/dsh-session'
|
||||
import { CallId } from '@deepseek-ai/dsh-llm'
|
||||
import type { SessionPersistence } from '../src/index.ts'
|
||||
@@ -23,7 +23,7 @@ export interface ContractBackend {
|
||||
/** Build a minimal {@link SessionHeader} for a session id. */
|
||||
export function meta(id: string, cwd?: string): SessionHeader {
|
||||
return {
|
||||
version: 1,
|
||||
version: SESSION_FORMAT_VERSION,
|
||||
id: SessionId(id),
|
||||
createdAt: 1000,
|
||||
...cwd !== undefined ? { cwd } : {},
|
||||
@@ -57,7 +57,7 @@ export function runPersistenceContract(name: string, make: () => Promise<Contrac
|
||||
await persistence.append(m.id, log)
|
||||
|
||||
const loaded = await persistence.load(m.id)
|
||||
expect(loaded.meta).toMatchObject({ version: 1, id: m.id, cwd: '/work' })
|
||||
expect(loaded.meta).toMatchObject({ version: SESSION_FORMAT_VERSION, id: m.id, cwd: '/work' })
|
||||
expect(loaded.events).toEqual(log)
|
||||
} finally {
|
||||
await dispose()
|
||||
|
||||
@@ -28,7 +28,7 @@
|
||||
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { Context, type Fiber } from 'cordis'
|
||||
import SessionStore, { SessionId } from '@deepseek-ai/dsh-session'
|
||||
import SessionStore, { SESSION_FORMAT_VERSION, SessionId } from '@deepseek-ai/dsh-session'
|
||||
import type { Session, SessionEvent } from '@deepseek-ai/dsh-session'
|
||||
import type { SessionPersistence } from '../src/index.ts'
|
||||
import { meta, oneTurnLog } from './contract.ts'
|
||||
@@ -671,7 +671,7 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise<
|
||||
const fix = await makeFixture()
|
||||
const { ctx, fiber } = await freshCtx(fix)
|
||||
try {
|
||||
const m = { version: 2, id: SessionId('v2'), createdAt: 1, cwd: WORK }
|
||||
const m = { version: 99, id: SessionId('v99'), createdAt: 1, cwd: WORK }
|
||||
await ctx.sessionPersistence.create(m)
|
||||
await ctx.sessionPersistence.append(m.id, oneTurnLog())
|
||||
await expect(ctx.sessionPersistence.load(m.id)).rejects.toThrow(/version/)
|
||||
@@ -685,7 +685,7 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise<
|
||||
const fix = await makeFixture()
|
||||
const { ctx, fiber } = await freshCtx(fix)
|
||||
try {
|
||||
const m = { version: 1, id: SessionId('forked-child'), createdAt: 1, cwd: WORK, parentSession: SessionId('the-parent') }
|
||||
const m = { version: SESSION_FORMAT_VERSION, id: SessionId('forked-child'), createdAt: 1, cwd: WORK, parentSession: SessionId('the-parent') }
|
||||
await ctx.sessionPersistence.create(m)
|
||||
await ctx.sessionPersistence.append(m.id, oneTurnLog())
|
||||
const loaded = await ctx.sessionPersistence.load(m.id)
|
||||
|
||||
Reference in New Issue
Block a user