From 8f5c592b9f8390d0309c8c93f245d650943702c6 Mon Sep 17 00:00:00 2001 From: kingwl Date: Wed, 15 Jul 2026 22:44:32 +0800 Subject: [PATCH] fix(session): break chunk runs on time gaps that cannot subtract exactly Two safe-integer timestamps can differ by more than 2^53-1 (e.g. MIN_SAFE_INTEGER to MAX_SAFE_INTEGER-1), so the dt subtraction rounds and the packed row decodes to a timestamp one off the original -- violating the codec's lossless contract. Unreachable from a real clock (the gap needs ~285k years) but reachable from hand-written fixtures, and the decoder accepts hand-written rows. continues() now refuses to extend a run across such a gap (the check is exact both ways: an in-range true gap subtracts without rounding and passes; an out-of-range one rounds to an out-of-range value and fails), splitting the run instead -- whitelist philosophy, compression lost, data never. The decoder tightens to match the encoder's image: time0 and dt must be safe integers, and reconstructed member seqs/times must stay in safe range, so float reconstruction is exact wherever validation passes. The round-trip property now draws times from the full safe-integer range (it previously generated only small gaps, which is how this escaped); the bot's counterexample is pinned as an example test. Found by ds-review-bot on #338. --- packages/core/session/src/chunk-rows.ts | 37 +++++++++++++++---- .../core/session/tests/chunk-rows.spec.ts | 34 +++++++++++++++-- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/packages/core/session/src/chunk-rows.ts b/packages/core/session/src/chunk-rows.ts index c42532946c..56de67b405 100644 --- a/packages/core/session/src/chunk-rows.ts +++ b/packages/core/session/src/chunk-rows.ts @@ -135,6 +135,12 @@ function indexOf(event: DeltaEvent): number { /** Whether `next` extends a run ending in `prev` (same kind already checked by the caller). */ function continues(prev: DeltaEvent, next: DeltaEvent, kind: DeltaKind): boolean { if (next.seq !== prev.seq + 1) return false + // Two safe-integer times can sit further apart than a double subtracts + // exactly (2^53-1 and its negation differ by ~2^54); a rounded gap would + // decode to a different timestamp. The check is exact in both directions: a + // true gap within safe range subtracts without rounding and passes, while a + // true gap beyond it rounds to a value that is itself beyond and fails. + if (!Number.isSafeInteger(next.time - prev.time)) return false if (next.data.turn !== prev.data.turn || next.data.step !== prev.data.step) return false if (indexOf(next) !== indexOf(prev)) return false if (kind !== 'tool-call-delta') return true @@ -219,8 +225,8 @@ function malformed(tag: string, why: string): never { throw new Error(`malformed ${tag} storage row: ${why}`) } -/** Validate the shared run-data fields and the payload/dt arity. */ -function validateRunData(tag: string, data: Record, payloadKey: 'texts' | 'args'): void { +/** Validate the shared run-data fields and the payload/dt arity; returns the member payload. */ +function validateRunData(tag: string, data: Record, payloadKey: 'texts' | 'args'): string[] { if (typeof data.turn !== 'number' || typeof data.step !== 'number' || typeof data.index !== 'number') { malformed(tag, 'turn/step/index must be numbers') } @@ -229,12 +235,13 @@ function validateRunData(tag: string, data: Record, payloadKey: malformed(tag, `${payloadKey} must be a non-empty string array`) } const dt = data.dt - if (!Array.isArray(dt) || dt.some(gap => typeof gap !== 'number' || !Number.isFinite(gap))) { - malformed(tag, 'dt must be an array of finite numbers') + if (!Array.isArray(dt) || dt.some(gap => !Number.isSafeInteger(gap))) { + malformed(tag, 'dt must be an array of safe integers') } if (dt.length !== payload.length - 1) { malformed(tag, `dt length ${dt.length} does not match ${payload.length} members`) } + return payload as string[] } /** Validate a row-tagged parsed value's envelope and data, throwing on any malformation. */ @@ -245,11 +252,12 @@ function validateRow(value: Record, tag: ChunkRow['type']): Chu if (!Number.isSafeInteger(value.seq0) || (value.seq0 as number) < 0) { malformed(tag, 'seq0 must be a non-negative safe integer') } - if (typeof value.time0 !== 'number' || !Number.isFinite(value.time0)) { - malformed(tag, 'time0 must be a finite number') + if (!Number.isSafeInteger(value.time0)) { + malformed(tag, 'time0 must be a safe integer') } const data = value.data if (!isRecord(data)) malformed(tag, 'data must be an object') + let payload: string[] if (tag === 'tool-call-chunks') { const withName = hasExactKeys(data, ['turn', 'step', 'index', 'id', 'name', 'dt', 'args']) if (!withName && !hasExactKeys(data, ['turn', 'step', 'index', 'id', 'dt', 'args'])) { @@ -258,12 +266,25 @@ function validateRow(value: Record, tag: ChunkRow['type']): Chu if (typeof data.id !== 'string' || (withName && typeof data.name !== 'string')) { malformed(tag, 'id (and name when present) must be strings') } - validateRunData(tag, data, 'args') + payload = validateRunData(tag, data, 'args') } else { if (!hasExactKeys(data, ['turn', 'step', 'index', 'dt', 'texts'])) { malformed(tag, 'data must be exactly {turn, step, index, dt, texts}') } - validateRunData(tag, data, 'texts') + payload = validateRunData(tag, data, 'texts') + } + // Reconstruction bounds. The encoder only packs runs whose member seqs and + // times are all safe integers, so a running value that leaves safe range is + // outside any encoder's image: float arithmetic would round it to a + // different number than exact arithmetic, a silent corruption. Within safe + // range every step is exact, so the first departure is always caught. + if (!Number.isSafeInteger((value.seq0 as number) + payload.length - 1)) { + malformed(tag, 'member seqs must stay safe integers') + } + let time = value.time0 as number + for (const gap of data.dt as number[]) { + time += gap + if (!Number.isSafeInteger(time)) malformed(tag, 'member times must stay safe integers') } return value as unknown as ChunkRow } diff --git a/packages/core/session/tests/chunk-rows.spec.ts b/packages/core/session/tests/chunk-rows.spec.ts index 9eb9173bca..28611e5e41 100644 --- a/packages/core/session/tests/chunk-rows.spec.ts +++ b/packages/core/session/tests/chunk-rows.spec.ts @@ -106,6 +106,22 @@ describe('packChunkRuns', () => { expect(packChunkRuns(events)).toStrictEqual(events) }) + it('breaks a run on a time gap beyond safe-integer range (subtraction would round)', () => { + // Both endpoints are safe integers, but their true difference (~2^54) + // exceeds exact double range: b - a rounds, so a + (b - a) !== b and a + // packed row would decode to a different timestamp. + const a = Number.MIN_SAFE_INTEGER + const b = Number.MAX_SAFE_INTEGER - 1 + expect(a + (b - a)).not.toBe(b) // the rounding this guard exists for + const events = [ + chunkEvent(0, a, { type: 'text-delta', index: 0, text: 'x' }), + chunkEvent(1, b, { type: 'text-delta', index: 0, text: 'y' }), + chunkEvent(2, b + 1, { type: 'text-delta', index: 0, text: 'z' }), + ] + expect(packChunkRuns(events)).toStrictEqual(events) // split at the gap; halves too short + expect(decodeAll(packChunkRuns(events))).toStrictEqual(events) + }) + it('stores a delta with an off-whitelist data envelope verbatim (parsed-fixture shapes)', () => { const mk = (seq: number, data: unknown): SessionEvent => ({ type: 'assistant/chunk', seq, time: 1000, data } as SessionEvent) @@ -144,11 +160,15 @@ describe('decodeStorageRecord', () => { ['an envelope with extra keys', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], texts: ['a'] }, extra: 1 }], ['a negative seq0', { type: 'text-chunks', seq0: -1, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], texts: ['a'] } }], ['a non-finite time0', { type: 'text-chunks', seq0: 0, time0: Infinity, data: { turn: 1, step: 1, index: 0, dt: [], texts: ['a'] } }], + ['a fractional time0', { type: 'text-chunks', seq0: 0, time0: 1.5, data: { turn: 1, step: 1, index: 0, dt: [], texts: ['a'] } }], ['a data shape mismatch', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], args: ['a'] } }], ['a non-string member', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], texts: [7] } }], ['an empty member list', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], texts: [] } }], ['a dt arity mismatch', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [1, 2], texts: ['a', 'b'] } }], ['a non-finite dt gap', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [NaN], texts: ['a', 'b'] } }], + ['a fractional dt gap', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [0.5], texts: ['a', 'b'] } }], + ['a member seq leaving safe range', { type: 'text-chunks', seq0: Number.MAX_SAFE_INTEGER, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [0, 0], texts: ['a', 'b', 'c'] } }], + ['a member time leaving safe range', { type: 'text-chunks', seq0: 0, time0: Number.MAX_SAFE_INTEGER, data: { turn: 1, step: 1, index: 0, dt: [1], texts: ['a', 'b'] } }], ['a non-numeric turn', { type: 'text-chunks', seq0: 0, time0: 1, data: { turn: 'x', step: 1, index: 0, dt: [], texts: ['a'] } }], ['a tool-call row without id', { type: 'tool-call-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, dt: [], args: ['a'] } }], ['a tool-call row with non-string id', { type: 'tool-call-chunks', seq0: 0, time0: 1, data: { turn: 1, step: 1, index: 0, id: 7, dt: [], args: ['a'] } }], @@ -183,11 +203,19 @@ const boundaryChunkArb: fc.Arbitrary = fc.oneof( fc.record({ type: fc.constant<'finish'>('finish'), reason: fc.constant({ kind: 'stop' as const }) }), ) -/** Batches with contiguous seqs, arbitrary gaps in time, mixed chunk kinds and turn/step placement. */ +/** + * Batches with contiguous seqs, arbitrary timestamps, mixed chunk kinds and + * turn/step placement. Times draw from the FULL safe-integer range (not just + * realistic clocks) so the property exercises the gap-overflow guard: two safe + * endpoints can differ by more than a double subtracts exactly. + */ const batchArb: fc.Arbitrary = fc.array( fc.record({ chunk: fc.oneof({ weight: 4, arbitrary: deltaChunkArb }, { weight: 1, arbitrary: boundaryChunkArb }), - gap: fc.integer({ min: -5, max: 200 }), + time: fc.oneof( + { weight: 4, arbitrary: fc.integer({ min: 995, max: 9000 }) }, + { weight: 1, arbitrary: fc.integer({ min: Number.MIN_SAFE_INTEGER, max: Number.MAX_SAFE_INTEGER }) }, + ), turn: fc.nat(1), step: fc.nat(1), }), @@ -196,7 +224,7 @@ const batchArb: fc.Arbitrary = fc.array( // plain objects real log events are (the log is JSON), so equality compares // values, not prototypes. ).map(entries => JSON.parse(JSON.stringify( - entries.map((entry, k) => chunkEvent(k, 1000 + entry.gap * k, entry.chunk, entry.turn, entry.step)), + entries.map((entry, k) => chunkEvent(k, entry.time, entry.chunk, entry.turn, entry.step)), )) as SessionEvent[]) describe('chunk-row codec properties', () => {