fix(rewind): correct API history truncation; abort on compression#200
Merged
Conversation
Adopts the LLM-history mapping logic from upstream QwenLM/qwen-code PR QwenLM#3441 to fix our rewind feature. The bug: our `applyConversationRewind` walked the API history counting `role === 'user'` entries as user turns, which conflated three distinct classes of entries: 1. Real user prompts (what we wanted to count) 2. Tool result entries (`role: 'user'` with `functionResponse` parts) 3. The startup context pair (env preamble + "Got it" model ack) It also miscounted the UI side: slash-command items (`/help`, `/stats`) have `type: 'user'` in our UI history but never reach the API, so they threw off the alignment between UI turn count and API user-content count. The combination meant the LLM history was sliced at the wrong index, which left orphan tool-result entries or stale context attached to the wrong turn — and on the next prompt the model produced output that no longer corresponded to the visible UI scrollback. Fix: - New `historyMapping.ts` utility with `computeApiTruncationIndex` and `isRealUserTurn` (adapted from upstream). - `applyConversationRewind` now uses `computeApiTruncationIndex` for the LLM slice and aborts cleanly when the target turn was absorbed by compression (returns -1 instead of truncating to a wrong index). - After truncating, calls `stripThoughtsFromHistory()` so stale thinking blocks don't leak into the next request. - Compression-abort path surfaces an error instead of silently rewinding to the wrong turn. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThe PR adds API-history mapping utilities to support compression-aware conversation rewinding. A new ChangesConversation Rewind with Compression Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 20 minutes and 41 seconds. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/ui/components/DialogManager.tsx (3)
435-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFiles are rewound before compression-abort is known, causing partial rewind despite error.
In
handleRestoreFilesAndConversation, files are restored first, then compression can abort conversation rewind (Line 446). That leaves file state changed even though the user is told rewind couldn’t be done.Suggested fix
const handleRestoreFilesAndConversation = (promptId: string) => { - // 1. Restore files to pre-turn state - try { - checkpointStore.rewindToCheckpoint(promptId); - } catch { - // checkpoint may have no file snapshots — safe to ignore - } - // 2. Restore conversation (LLM history + UI history) + // 1. Restore conversation (LLM history + UI history) const { slicedUiHistory, originalPrompt, compressionAbort } = applyConversationRewind(promptId); uiActions.closeRewindDialog(); if (compressionAbort) { addItem( { type: 'error', text: 'Cannot rewind to a turn that was compressed. Try a more recent turn.', }, Date.now(), ); return; } + // 2. Restore files to pre-turn state (only after conversation rewind is valid) + try { + checkpointStore.rewindToCheckpoint(promptId); + } catch { + // checkpoint may have no file snapshots — safe to ignore + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/DialogManager.tsx` around lines 435 - 455, handleRestoreFilesAndConversation currently restores files via checkpointStore.rewindToCheckpoint before checking applyConversationRewind’s compressionAbort, which can leave files changed when conversation rewind fails; fix by calling applyConversationRewind(promptId) first, inspect compressionAbort and return early (and show the error) before calling checkpointStore.rewindToCheckpoint, then proceed to uiActions.closeRewindDialog/addItem and the rest; keep the existing try/catch around checkpointStore.rewindToCheckpoint to safely ignore missing snapshots.
524-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSummarize-from-here ignores
compressionAbortand can summarize without rewinding.This path calls
applyConversationRewindbut does not checkcompressionAbort. If target turn is compressed, conversation remains unchanged yet summarization still runs, producing misleading “rewound and summarized” outcomes.Suggested fix
const handleSummarizeFromHere = async (promptId: string) => { // 1. Rewind conversation to the selected checkpoint - const { slicedUiHistory } = applyConversationRewind(promptId); + const { slicedUiHistory, compressionAbort } = applyConversationRewind(promptId); uiActions.closeRewindDialog(); + if (compressionAbort) { + addItem( + { + type: 'error', + text: 'Cannot rewind to a turn that was compressed. Try a more recent turn.', + }, + Date.now(), + ); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/DialogManager.tsx` around lines 524 - 541, handleSummarizeFromHere currently calls applyConversationRewind and proceeds to compress/summarize even when applyConversationRewind signals a failed rewind via compressionAbort; update handleSummarizeFromHere to inspect the result from applyConversationRewind (e.g., check compressionAbort or an equivalent flag returned with slicedUiHistory) immediately after the call, and if compressionAbort is true, call uiActions.closeRewindDialog() (or keep it closed per UX), add an error item via addItem explaining the rewind failed, and return early so no summarization or calls to config.getGeminiClient() / summarization logic run; this prevents summarizing when the conversation was not actually rewound.
370-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheckpoint-to-UI turn mapping still counts slash-command items, which can target the wrong turn.
Line 374 currently treats every
type === 'user'item as a real prompt. If slash/help entries exist,cpIndexdrifts andtargetUserItemcan point to the wrong turn, causing incorrect rewind truncation.Suggested fix
-import { computeApiTruncationIndex } from '../utils/historyMapping.js'; +import { + computeApiTruncationIndex, + isRealUserTurn, +} from '../utils/historyMapping.js'; ... let usersSeen = 0; for (let i = 0; i < uiState.history.length; i++) { - if (uiState.history[i].type === 'user') { + if (isRealUserTurn(uiState.history[i])) { if (usersSeen === cpIndex) { return i; // slice(0, i) keeps everything before this turn } usersSeen++; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/DialogManager.tsx` around lines 370 - 379, The loop in DialogManager that maps cpIndex to a UI history index currently counts every item with type === 'user', which includes slash-command/help entries and misaligns cpIndex; update the condition inside the for-loop to only increment usersSeen and match cpIndex when the history entry is an actual user prompt (e.g., check a distinguishing flag/property on the entry such as historyItem.isPrompt === true or historyItem.isCommand !== true or historyItem.source !== 'slash' depending on your model), so change the if (uiState.history[i].type === 'user') branch to first filter out slash/command/help items before counting and returning the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/ui/components/DialogManager.tsx`:
- Around line 435-455: handleRestoreFilesAndConversation currently restores
files via checkpointStore.rewindToCheckpoint before checking
applyConversationRewind’s compressionAbort, which can leave files changed when
conversation rewind fails; fix by calling applyConversationRewind(promptId)
first, inspect compressionAbort and return early (and show the error) before
calling checkpointStore.rewindToCheckpoint, then proceed to
uiActions.closeRewindDialog/addItem and the rest; keep the existing try/catch
around checkpointStore.rewindToCheckpoint to safely ignore missing snapshots.
- Around line 524-541: handleSummarizeFromHere currently calls
applyConversationRewind and proceeds to compress/summarize even when
applyConversationRewind signals a failed rewind via compressionAbort; update
handleSummarizeFromHere to inspect the result from applyConversationRewind
(e.g., check compressionAbort or an equivalent flag returned with
slicedUiHistory) immediately after the call, and if compressionAbort is true,
call uiActions.closeRewindDialog() (or keep it closed per UX), add an error item
via addItem explaining the rewind failed, and return early so no summarization
or calls to config.getGeminiClient() / summarization logic run; this prevents
summarizing when the conversation was not actually rewound.
- Around line 370-379: The loop in DialogManager that maps cpIndex to a UI
history index currently counts every item with type === 'user', which includes
slash-command/help entries and misaligns cpIndex; update the condition inside
the for-loop to only increment usersSeen and match cpIndex when the history
entry is an actual user prompt (e.g., check a distinguishing flag/property on
the entry such as historyItem.isPrompt === true or historyItem.isCommand !==
true or historyItem.source !== 'slash' depending on your model), so change the
if (uiState.history[i].type === 'user') branch to first filter out
slash/command/help items before counting and returning the index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c68e5cf-59a9-4e7e-9a2c-482f653b229f
📒 Files selected for processing (3)
packages/cli/src/ui/components/DialogManager.tsxpackages/cli/src/ui/utils/historyMapping.test.tspackages/cli/src/ui/utils/historyMapping.ts
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User reported: "the ui doesn't properly rewind in our implementation."
Root cause: our
applyConversationRewindwalked the API history countingrole === 'user'entries as user turns, which conflated three classes of entries that should be counted differently:role: 'user'withfunctionResponseparts. Should be skipped.It also miscounted on the UI side: slash-command items (
/help,/stats) are stored withtype: 'user'in our UI history but never reach the API, so they threw off the alignment between UI turn count and API user-content count.The combination meant the LLM history was sliced at the wrong index. The next prompt sent to the model included orphan tool-result entries or stale model context attached to the wrong turn — and the model's response no longer corresponded to the visible UI scrollback.
What changed
Adopts the LLM-history mapping logic from upstream QwenLM/qwen-code #3441:
packages/cli/src/ui/utils/historyMapping.ts—computeApiTruncationIndexandisRealUserTurn. Adapted from upstream; same algorithm but documented for our fork.DialogManager.tsx applyConversationRewind— usescomputeApiTruncationIndexfor the LLM slice instead of the buggy user-count walk. Aborts cleanly when the target turn was absorbed by chat compression (returns-1instead of truncating to a wrong index).geminiClient.stripThoughtsFromHistory()so stale thinking blocks don't leak into the next request.Why a focused port and not full QwenLM#3441
Our fork already has its own complete rewind feature (commits
bd68124b,73166e2d,c17e875e,0929fb7e,062b04a2, etc.) including a dedicatedRewindPicker, file-snapshot restore viacheckpointStore, and three-way action menu (files+conversation, conversation only, files only). Porting upstream's whole rewind would conflict with and likely regress that.This PR adopts only the LLM-history mapping piece — the part our implementation got wrong.
Test plan
computeApiTruncationIndexandisRealUserTurncovering: empty history, with/without startup context, tool-call interleaving, compression fallback, slash-command-only UI items.npm run typecheckclean across all workspaces./rewindto a turn before the tool call. Verify (a) UI scrollback truncates, (b) next user prompt produces a coherent response that doesn't reference removed turns, (c) model isn't confused by leftover tool-result entries./rewindto a turn that was absorbed by compression. Expect "Cannot rewind to a turn that was compressed" error rather than silent wrong-turn rewind.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests