Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped server memory-retention fix that stops storing full Claude SDK messages while preserving turn counts, IDs, event handling, and native-history rollback. The snapshot shape remains compatible, with only unused in-memory message contents replaced by empty arrays. No code changes detected at You can add or adjust custom eligibility rules. Learn more. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
| turns: context.turns.map((turn) => ({ id: turn.id, items: [] })), | ||
| }; | ||
| }); | ||
|
|
There was a problem hiding this comment.
readThread now returns empty items for completed Claude turns, changing its observable snapshot behavior. Could you add a focused adapter test that completes a turn, calls readThread, and verifies the turn ID remains while its items are empty (and item events still carry the content)?
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Not adding this. Asserting items is empty would only restate the literal items: [] in snapshotThread (ClaudeAdapter.ts:2176). The turn count after completed turns is already checked by readThread at ClaudeAdapter.test.ts:6631 and by the rollback snapshots at :6745, and item content is covered by the existing item event tests. readThread also has no production caller.
There was a problem hiding this comment.
The existing tests don’t cover this observable contract: they verify turn count or events separately, but not that readThread retains the completed turn ID while omitting its items. Since readThread is part of the adapter interface, I still think the focused regression test is warranted.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: 9 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughClaude turn state and session history now store turn IDs without accumulated message items. Thread snapshots return an empty ChangesClaude turn tracking
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change reduces live-session message retention without an identified disruption to thread reading or rollback. It is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ClaudeAdapter pushed every parent assistant message and every tool-result user message (subagent ones too) into the turn state, then copied them into context.turns until the session stopped. Nothing read them: readThread has no production caller, ProviderService discards the rollback snapshot, and rollback uses the native Claude history plus turnStartMessageIds. Keep only the turn ids, so readThread and rollbackThread still report the correct turn count. Same fix as #9684 for OpenCode. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
0929738 to
37bae9d
Compare
|
Closing as superseded by #13718. |
A long-uptime report (T3 Code 0.0.40, 9 days uptime, about 11 live Claude sessions) showed server memory that only a restart freed. One cause:
ClaudeAdapterkept every parent assistant message and every tool-result user message (subagent tool results too) in memory. It copied them intocontext.turnsat the end of each turn, and they stayed there until the session stopped. The session reaper skips sessions with an active turn or background work, so a busy session can hold them for days.Nothing reads these messages.
readThreadhas no production caller.ProviderServicediscards the snapshot thatrollbackThreadreturns. Rollback reads the native Claude history andturnStartMessageIds.Fix
This follows #9684, which made the same fix for OpenCode.
itemsfromClaudeTurnStateand the two sites that pushed SDK messages into it.context.turnsnow keeps only turn ids. A comment says the native Claude session file is the transcript.snapshotThreadmaps each turn to{ id, items: [] }, soreadThreadandrollbackThreadstill report the correct turn count.The change is in
apps/server/src/provider/Layers/ClaudeAdapter.tsonly. We did not measure the memory saved. It depends on how much tool output a live session produces.Verification
vp test run apps/server/src/provider/Layers/ClaudeAdapter.test.ts(140 passed). The existing readThread and rollback tests check the turn count. Removing the turn id push makes 5 rollback tests fail.vp lintandvp fmt --checkonClaudeAdapter.tsvp run --filter t3 typecheckRelated: #9661 tracks performance audit fixes.
Made by Claude Opus 5.5 (1M context) in Claude Code, running in T3 Code.
🤖 Generated with Claude Code
Summary by CodeRabbit