fix(state): transcript step records duplicate the prior conversation — record per-step deltas (#175)#198
Conversation
…essages duplicated resume context (#175) ai@6's onStepFinish exposes event.response.messages as the CUMULATIVE response list for the whole run so far, not a per-step delta (live evidence: per-record counts [2,4,6], record 0 a strict prefix of record 1). All four transcript recording sites wrote it verbatim, so files grew O(N^2) and reconstructTranscript concatenated overlapping records — the #108 resume path (findResumable -> priorHandle) handed the Worker/CI-fix session a duplicated conversation. - recordStepDeltas(recorder): an onStepFinish handler that tracks the count already recorded and slices the new tail, so each record is a true delta. Wired at all four sites (planner/worker/reviewer/ci-fix); the recorder stays a dumb append. - reconstructTranscript de-overlaps records whose leading run equals the accumulated messages, so transcripts written by prior releases (cumulative) reconstruct duplicate-free too; per-step-delta records don't overlap and append whole. - Tests: recordStepDeltas emits [2,2,2] deltas from cumulative [2,4,6] events (+ a no-new-messages step records nothing); reconstruct de-overlaps a pre-fix cumulative transcript to 4 messages, not 6. Part of #196. Shares its root cause with #176 (compaction sizing).
📝 WalkthroughWalkthroughTranscript step records now contain only newly observed response messages. Transcript reconstruction also removes duplicated prefixes from transcripts written with the previous cumulative-message format. ChangesTranscript delta handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 58 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 50 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 28 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/aitm/src/state/transcript-store.ts (1)
56-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer a real deep-equal over
JSON.stringifycomparison.
JSON.stringifyequality is order-sensitive — two structurally-identical message objects with differently-ordered keys would compare unequal, causingisLeadingRunto (falsely) report no overlap and reintroduce duplication for exactly the legacy transcripts this function targets. Both arrays here come fromJSON.parse, so this mostly holds in practice, but it's a fragile invariant for a correctness-critical dedup check.♻️ Suggested fix using `node:util.isDeepStrictEqual`
+import { isDeepStrictEqual } from 'node:util'; + function isLeadingRun(arr: readonly ModelMessage[], prefix: readonly ModelMessage[]): boolean { if (prefix.length === 0) return true; if (arr.length < prefix.length) return false; - for (let i = 0; i < prefix.length; i++) { - if (JSON.stringify(arr[i]) !== JSON.stringify(prefix[i])) return false; - } - return true; + return prefix.every((m, i) => isDeepStrictEqual(arr[i], m)); }Please confirm
node:util'sisDeepStrictEqualis available under the project's Bun/Node/Deno portability targets before adopting this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/aitm/src/state/transcript-store.ts` around lines 56 - 65, Update isLeadingRun to compare each arr/prefix message pair with a real deep-equality helper instead of JSON.stringify, preserving the existing empty-prefix, length, and early-exit behavior. Verify that node:util isDeepStrictEqual is supported across the project’s Bun, Node, and Deno targets before using it; otherwise reuse an existing portable deep-equality utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/aitm/src/state/transcript-store.ts`:
- Around line 56-65: Update isLeadingRun to compare each arr/prefix message pair
with a real deep-equality helper instead of JSON.stringify, preserving the
existing empty-prefix, length, and early-exit behavior. Verify that node:util
isDeepStrictEqual is supported across the project’s Bun, Node, and Deno targets
before using it; otherwise reuse an existing portable deep-equality utility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: af64d34f-9842-4082-9513-126641caa411
📒 Files selected for processing (4)
packages/aitm/src/loop/run-loop-adapter.test.tspackages/aitm/src/loop/run-loop-adapter.tspackages/aitm/src/state/transcript-store.test.tspackages/aitm/src/state/transcript-store.ts
1 participant
Closes #175. Second of the two P0s from the post-#100 parity verification (#196), same root cause as #176.
Root cause (verified)
ai@6.0.182'sonStepFinishexposesevent.response.messagesas the cumulative response-message list for the whole run so far, not a per-step delta. Confirmed against a live run (sum/working-1.jsonl): per-record counts [2, 4, 6], record 0 a strict prefix of record 1.All four transcript recording sites (
plannerRecorder/recorder×2/ciRecorder) wrote it verbatim:reconstructTranscriptconcatenates records, so a 3-step / 6-message conversation rebuilt to 12 messages. The feat(state): per-subagent transcript persistence — messages JSONL + in-run resume #108 resume path (findResumable→priorHandle) then handed a Worker/CI-fix session duplicated context — inflated tokens, confused model.The unit tests passed because their fakes modelled
response.messagesas per-step deltas — the wrong SDK assumption.Fix
recordStepDeltas(recorder)— anonStepFinishhandler that tracks the count already recorded and appends only the new tail (messages.slice(recorded)), so each record is a true delta. Wired at all four sites; the recorder stays a dumb serialized append (SDK-quirk knowledge lives in one adapter helper, not the store).reconstructTranscriptde-overlaps a record whose leading run equals the accumulated messages → appends only its suffix. This makes transcripts written by prior releases (cumulative) reconstruct duplicate-free too; new per-step-delta records don't overlap and append whole. One rule handles both.Tests
recordStepDeltas: cumulative[2,4,6]events → recorded deltas[2,2,2]; usage forwarded; an unchanged-cumulative step records nothing.reconstructTranscript: a pre-fix cumulative transcript de-overlaps to 4 messages, not 6.test:node(aitm 905 / compat 250) + biome.Same root cause as #176 — fixed separately since it touches different files.
Part of #196.
Summary by CodeRabbit
Bug Fixes
Tests