From 5299e43bed538d28e856720dadedb8af1711870a Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 15 Jun 2026 23:33:00 +0800 Subject: [PATCH] fix(session): snapshot seed + appended data at the boundary (review #31) The source-level JSON-serializability invariant was only a preflight: the Session constructor copied the seed array but shared every event/data object with the caller, and append() stored the caller's `data` reference verbatim. A post-create/post-append mutation could rewrite the durable log or reintroduce a non-JSON-serializable value AFTER validation, so session.events could diverge from what was validated / what a backend can persist. - ctor deep-clones each seed event after validation (not just the array). - append() stores structuredClone(data) (serializability already checked, so the clone is safe); the returned event carries the same snapshot. Regression tests: mutating the original seed / the passed append object after the call leaves session.events unchanged. Adapted the dev-freeze invariants test to assert on the logged clone (append no longer freezes the caller's input). Documented isJsonValue's exact scope (own enumerable string keys, matching JSON.stringify) and synced the README create() signature with meta.createdAt. --- packages/invariants/tests/invariants.spec.ts | 11 ++++--- packages/session/README.md | 2 +- packages/session/src/index.ts | 21 +++++++++++-- packages/session/src/json.ts | 8 +++++ packages/session/tests/session.spec.ts | 33 ++++++++++++++++++++ 5 files changed, 68 insertions(+), 7 deletions(-) diff --git a/packages/invariants/tests/invariants.spec.ts b/packages/invariants/tests/invariants.spec.ts index bbf93f44dc..a2e205457f 100644 --- a/packages/invariants/tests/invariants.spec.ts +++ b/packages/invariants/tests/invariants.spec.ts @@ -241,12 +241,15 @@ describe('dev-freeze', () => { // mutable. deepFreeze must descend into the already-frozen object and // freeze the descendant, not short-circuit on the frozen container — // otherwise dev-mode misses exactly the history mutation ADR 0012 catches. + // `append` snapshots `data`, so the freeze applies to the LOGGED clone, not + // the caller's input — read the event back and assert on its data. const innerContent: { type: 'text'; text: string }[] = [{ type: 'text', text: 'inner' }] const block = Object.freeze({ type: 'tool-result' as const, toolCallId: CallId('c1'), content: innerContent, isError: false }) - session.append('user/message', { content: [block], source: { kind: 'user' } }) - expect(Object.isFrozen(block.content)).toBe(true) - expect(Object.isFrozen(block.content[0])).toBe(true) - expect(() => { block.content.push({ type: 'text', text: 'mutation' }) }).toThrow() + const event = session.append('user/message', { content: [block], source: { kind: 'user' } }) + const logged = event.data.content[0] as { content: { type: 'text'; text: string }[] } + expect(Object.isFrozen(logged.content)).toBe(true) + expect(Object.isFrozen(logged.content[0])).toBe(true) + expect(() => { logged.content.push({ type: 'text', text: 'mutation' }) }).toThrow() }) it('terminates on a cyclic event datum (WeakSet guard)', async () => { diff --git a/packages/session/README.md b/packages/session/README.md index 705e3d5b39..7cf443fa71 100644 --- a/packages/session/README.md +++ b/packages/session/README.md @@ -8,7 +8,7 @@ Creates and holds event-sourced `Session` instances. Persistence is intentionall ### Public API -- `ctx.sessions.create(id?: string, options?: { seed?: SessionEvent[]; meta?: { cwd?: string; parentSession?: SessionId } }): Session` — Create a session. `options.seed` replays/forks an existing event log; `options.meta` attaches creation metadata (validated absolute `cwd`, `parentSession` lineage) as the immutable `SessionHeader` (the store fills `version`/`id`/`createdAt`). Disposed with the calling fiber. +- `ctx.sessions.create(id?: string, options?: { seed?: SessionEvent[]; meta?: { cwd?: string; parentSession?: SessionId; createdAt?: number } }): Session` — Create a session. `options.seed` replays/forks an existing event log; `options.meta` attaches creation metadata (validated absolute `cwd`, `parentSession` lineage) as the immutable `SessionHeader`. The store fills `version`/`id` and defaults `createdAt` to now; a caller reconstructing a persisted session passes the original `createdAt` to preserve it. Disposed with the calling fiber. - `ctx.sessions.get(id: string): Session | undefined` - `ctx.sessions.list(): Session[]` diff --git a/packages/session/src/index.ts b/packages/session/src/index.ts index 62cce89c7b..8fe54cb97b 100644 --- a/packages/session/src/index.ts +++ b/packages/session/src/index.ts @@ -90,7 +90,15 @@ export class Session { throw new Error(`seed event "${event.type}" (seq ${event.seq}) carries non-JSON-serializable data`) } }) - this.log = [...seed] + // Deep-clone each seed event, NOT just the array: the seed events and + // their `data` are still owned by the caller (or the source session of a + // fork), so keeping the references would let a post-create mutation of the + // original rewrite this session's durable log — or reintroduce a + // non-JSON-serializable value AFTER the validation above. Snapshotting at + // the boundary makes `session.events` independent and keeps it equal to + // what was validated. Serializability is guaranteed by the check above, so + // structuredClone can never hit a non-cloneable value here. + this.log = seed.map(event => structuredClone(event)) } this.header = header ?? { version: 1, id, createdAt: Date.now() } } @@ -120,7 +128,16 @@ export class Session { if (!isJsonValue(data)) { throw new Error(`session event "${type}" carries non-JSON-serializable data`) } - const event = { type, seq: this.log.length, time: Date.now(), data } as SessionEvent + // Snapshot `data` into the log, NOT the caller's reference: the validation + // above proves it is JSON-serializable AT THIS MOMENT, but the caller still + // owns the object and could mutate it afterwards (before a persistence + // flush, or permanently in the in-memory history) — making `session.events` + // diverge from the value that passed validation, or reintroducing a + // non-serializable value. Cloning here keeps the log equal to what was + // validated. structuredClone is safe because serializability was just + // checked. The returned event carries the SAME snapshot, so a caller reading + // back `event.data` sees the logged value, not its own mutable input. + const event = { type, seq: this.log.length, time: Date.now(), data: structuredClone(data) } as SessionEvent this.log.push(event) this.onAppend?.(event) return event diff --git a/packages/session/src/json.ts b/packages/session/src/json.ts index ebb0c00c64..99fe80ea80 100644 --- a/packages/session/src/json.ts +++ b/packages/session/src/json.ts @@ -22,6 +22,14 @@ * convert lossily. Sparse arrays are rejected too: a hole serializes to `null`, * so `[1, , 3]` would not round-trip. Detects circular references (which would * throw) and reports them as non-serializable rather than propagating the throw. + * + * Scope — matches `JSON.stringify` exactly: only an object's OWN ENUMERABLE + * STRING-keyed properties are inspected (`Object.values`). Symbol-keyed and + * non-enumerable properties are NOT examined, because `JSON.stringify` likewise + * drops them — they never reach the durable form, so a non-serializable value + * hiding under a symbol/non-enumerable key cannot make the round-trip lossy. + * Getters are invoked during the check (again as `JSON.stringify` would), so the + * contract is for plain data records, not objects with side-effecting accessors. */ export function isJsonValue(value: unknown, seen: Set = new Set()): boolean { if (value === null) return true diff --git a/packages/session/tests/session.spec.ts b/packages/session/tests/session.spec.ts index 39d29872ee..50dc9d5907 100644 --- a/packages/session/tests/session.spec.ts +++ b/packages/session/tests/session.spec.ts @@ -139,6 +139,39 @@ describe('Session', () => { const session = new Session(SessionId('seed-ok'), goodSeed) expect(session.events).toHaveLength(3) }) + + it('snapshots the seed: mutating the original after construction does not affect session.events', () => { + const seed = [ + { type: 'turn/start' as const, seq: 0, time: 1, data: { turn: 1, trigger: { kind: 'message' as const, source: { kind: 'user' as const } } } }, + { type: 'user/message' as const, seq: 1, time: 2, data: { content: [{ type: 'text' as const, text: 'original' }], source: { kind: 'user' as const } } }, + { type: 'turn/end' as const, seq: 2, time: 3, data: { turn: 1, reason: { kind: 'completed' as const } } }, + ] as SessionEvent[] + const session = new Session(SessionId('seed-snapshot'), seed) + // Mutate the ORIGINAL seed objects after construction: a shared reference + // would let this rewrite the forked log (or reintroduce non-serializable + // data past validation). The snapshot must shield session.events. + const um = seed[1]! + ;(um.data as { content: { type: 'text'; text: string }[] }).content[0]!.text = 'HACKED' + ;(um.data as Record)['injected'] = 1n // would have failed validation + const logged = session.events[1]! + expect(logged.type === 'user/message' && (logged.data.content[0] as { text: string }).text).toBe('original') + expect((logged.data as Record)['injected']).toBeUndefined() + }) + + it('snapshots append data: mutating the passed object after append does not affect session.events', () => { + const session = new Session(SessionId('append-snapshot')) + const data = { content: [{ type: 'text' as const, text: 'original' }], source: { kind: 'user' as const } } + const event = session.append('user/message', data) + // Mutate the caller's object after append returns. A shared reference would + // make session.events diverge from the value that passed validation. + data.content[0]!.text = 'HACKED' + ;(data as Record)['injected'] = 1n + const logged = session.events[0]! + expect(logged.type === 'user/message' && (logged.data.content[0] as { text: string }).text).toBe('original') + expect((logged.data as Record)['injected']).toBeUndefined() + // The returned event carries the same snapshot, not the caller's input. + expect((event.data.content[0] as { text: string }).text).toBe('original') + }) })