mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-04-28 11:41:04 +00:00
fix(rewind): close slash-command, compression, and IDE bypass holes
Three bugs found by Codex review: 1. P1: `/rewind` slash command bypassed the IDE-mode guard because `slashCommandActions.openRewindSelector` called `setIsRewindSelectorOpen` directly. Fixed by introducing a ref bridge (`openRewindSelectorRef`) that delegates to the guarded callback. 2. P1: Slash-command invocations (`/help`, `/stats`, etc.) are stored as `type: 'user'` in UI history but never reach the API or recording service. The turn-index counter in `handleRewindConfirm` and `computeApiTruncationIndex` counted them, producing off-by-N errors. Added `isRealUserTurn()` helper that excludes items starting with `/` or `?`, applied in all three counting sites (AppContainer, historyMapping, RewindSelector). 3. P2: After chat compression, `computeApiTruncationIndex` returned `apiHistory.length` when the target turn was unreachable, silently keeping the full API history while the UI was truncated. Changed to return `-1`; `handleRewindConfirm` now aborts with an error message when the target turn was absorbed by compression. Tests: 14 unit tests for historyMapping (including slash-command and compression cases), full suite 616/616 passed. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
This commit is contained in:
parent
436842638e
commit
8767f0956e
4 changed files with 88 additions and 11 deletions
|
|
@ -75,7 +75,10 @@ import { useResumeCommand } from './hooks/useResumeCommand.js';
|
||||||
import { useDeleteCommand } from './hooks/useDeleteCommand.js';
|
import { useDeleteCommand } from './hooks/useDeleteCommand.js';
|
||||||
import { useSlashCommandProcessor } from './hooks/slashCommandProcessor.js';
|
import { useSlashCommandProcessor } from './hooks/slashCommandProcessor.js';
|
||||||
import { useDoublePress } from './hooks/useDoublePress.js';
|
import { useDoublePress } from './hooks/useDoublePress.js';
|
||||||
import { computeApiTruncationIndex } from './utils/historyMapping.js';
|
import {
|
||||||
|
computeApiTruncationIndex,
|
||||||
|
isRealUserTurn,
|
||||||
|
} from './utils/historyMapping.js';
|
||||||
import { useVimMode } from './contexts/VimModeContext.js';
|
import { useVimMode } from './contexts/VimModeContext.js';
|
||||||
import { CompactModeProvider } from './contexts/CompactModeContext.js';
|
import { CompactModeProvider } from './contexts/CompactModeContext.js';
|
||||||
import { useTerminalSize } from './hooks/useTerminalSize.js';
|
import { useTerminalSize } from './hooks/useTerminalSize.js';
|
||||||
|
|
@ -634,6 +637,12 @@ export const AppContainer = (props: AppContainerProps) => {
|
||||||
const { isHooksDialogOpen, openHooksDialog, closeHooksDialog } =
|
const { isHooksDialogOpen, openHooksDialog, closeHooksDialog } =
|
||||||
useHooksDialog();
|
useHooksDialog();
|
||||||
|
|
||||||
|
// Ref bridge: the guarded openRewindSelector callback is defined later
|
||||||
|
// (after useDoublePress), but slashCommandActions needs it now. The ref
|
||||||
|
// lets the useMemo capture a stable function pointer whose implementation
|
||||||
|
// is swapped in once the real callback exists.
|
||||||
|
const openRewindSelectorRef = useRef<() => void>(() => {});
|
||||||
|
|
||||||
const slashCommandActions = useMemo(
|
const slashCommandActions = useMemo(
|
||||||
() => ({
|
() => ({
|
||||||
openAuthDialog,
|
openAuthDialog,
|
||||||
|
|
@ -662,7 +671,7 @@ export const AppContainer = (props: AppContainerProps) => {
|
||||||
openMcpDialog,
|
openMcpDialog,
|
||||||
openHooksDialog,
|
openHooksDialog,
|
||||||
openResumeDialog,
|
openResumeDialog,
|
||||||
openRewindSelector: () => setIsRewindSelectorOpen(true),
|
openRewindSelector: () => openRewindSelectorRef.current(),
|
||||||
handleResume,
|
handleResume,
|
||||||
openDeleteDialog,
|
openDeleteDialog,
|
||||||
}),
|
}),
|
||||||
|
|
@ -1591,6 +1600,7 @@ export const AppContainer = (props: AppContainerProps) => {
|
||||||
if (!hasUserTurns) return;
|
if (!hasUserTurns) return;
|
||||||
setIsRewindSelectorOpen(true);
|
setIsRewindSelectorOpen(true);
|
||||||
}, [streamingState, config, historyManager.history]);
|
}, [streamingState, config, historyManager.history]);
|
||||||
|
openRewindSelectorRef.current = openRewindSelector;
|
||||||
|
|
||||||
const closeRewindSelector = useCallback(() => {
|
const closeRewindSelector = useCallback(() => {
|
||||||
setIsRewindSelectorOpen(false);
|
setIsRewindSelectorOpen(false);
|
||||||
|
|
@ -1608,7 +1618,7 @@ export const AppContainer = (props: AppContainerProps) => {
|
||||||
let targetTurnIndex = 0;
|
let targetTurnIndex = 0;
|
||||||
for (const h of originalHistory) {
|
for (const h of originalHistory) {
|
||||||
if (h.id === userItem.id) break;
|
if (h.id === userItem.id) break;
|
||||||
if (h.type === 'user') targetTurnIndex++;
|
if (isRealUserTurn(h)) targetTurnIndex++;
|
||||||
}
|
}
|
||||||
|
|
||||||
// 2. Compute API truncation point
|
// 2. Compute API truncation point
|
||||||
|
|
@ -1619,6 +1629,19 @@ export const AppContainer = (props: AppContainerProps) => {
|
||||||
apiHistory,
|
apiHistory,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Abort if the target turn is unreachable (e.g., absorbed by compression)
|
||||||
|
if (apiTruncateIndex < 0) {
|
||||||
|
historyManager.addItem(
|
||||||
|
{
|
||||||
|
type: 'error',
|
||||||
|
text: 'Cannot rewind to a turn that was compressed. Try a more recent turn.',
|
||||||
|
},
|
||||||
|
Date.now(),
|
||||||
|
);
|
||||||
|
setIsRewindSelectorOpen(false);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
// 3. Truncate API history and strip stale thinking blocks
|
// 3. Truncate API history and strip stale thinking blocks
|
||||||
geminiClient.truncateHistory(apiTruncateIndex);
|
geminiClient.truncateHistory(apiTruncateIndex);
|
||||||
geminiClient.stripThoughtsFromHistory();
|
geminiClient.stripThoughtsFromHistory();
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,7 @@ import { theme } from '../semantic-colors.js';
|
||||||
import { useTerminalSize } from '../hooks/useTerminalSize.js';
|
import { useTerminalSize } from '../hooks/useTerminalSize.js';
|
||||||
import { useKeypress } from '../hooks/useKeypress.js';
|
import { useKeypress } from '../hooks/useKeypress.js';
|
||||||
import { truncateText } from '../utils/sessionPickerUtils.js';
|
import { truncateText } from '../utils/sessionPickerUtils.js';
|
||||||
|
import { isRealUserTurn } from '../utils/historyMapping.js';
|
||||||
import { t } from '../../i18n/index.js';
|
import { t } from '../../i18n/index.js';
|
||||||
|
|
||||||
export interface RewindSelectorProps {
|
export interface RewindSelectorProps {
|
||||||
|
|
@ -25,7 +26,7 @@ const MAX_VISIBLE_ITEMS = 7;
|
||||||
* Extract user-type items from UI history for the rewind pick list.
|
* Extract user-type items from UI history for the rewind pick list.
|
||||||
*/
|
*/
|
||||||
function getUserTurns(history: HistoryItem[]): HistoryItem[] {
|
function getUserTurns(history: HistoryItem[]): HistoryItem[] {
|
||||||
return history.filter((item) => item.type === 'user');
|
return history.filter(isRealUserTurn);
|
||||||
}
|
}
|
||||||
|
|
||||||
interface TurnItemViewProps {
|
interface TurnItemViewProps {
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,7 @@
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import { describe, it, expect } from 'vitest';
|
import { describe, it, expect } from 'vitest';
|
||||||
import { computeApiTruncationIndex } from './historyMapping.js';
|
import { computeApiTruncationIndex, isRealUserTurn } from './historyMapping.js';
|
||||||
import type { HistoryItem } from '../types.js';
|
import type { HistoryItem } from '../types.js';
|
||||||
import type { Content, Part } from '@google/genai';
|
import type { Content, Part } from '@google/genai';
|
||||||
|
|
||||||
|
|
@ -169,7 +169,7 @@ describe('computeApiTruncationIndex', () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('compression fallback', () => {
|
describe('compression fallback', () => {
|
||||||
it('returns apiHistory.length when not enough user prompts found', () => {
|
it('returns -1 when not enough user prompts found', () => {
|
||||||
const ui: HistoryItem[] = [
|
const ui: HistoryItem[] = [
|
||||||
userItem(1),
|
userItem(1),
|
||||||
geminiItem(2),
|
geminiItem(2),
|
||||||
|
|
@ -185,7 +185,28 @@ describe('computeApiTruncationIndex', () => {
|
||||||
modelContent('response 5'),
|
modelContent('response 5'),
|
||||||
];
|
];
|
||||||
// Rewind to turn 5 → 2 user turns before it, but API only has 1 user text
|
// Rewind to turn 5 → 2 user turns before it, but API only has 1 user text
|
||||||
expect(computeApiTruncationIndex(ui, 5, api)).toBe(api.length);
|
expect(computeApiTruncationIndex(ui, 5, api)).toBe(-1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('with slash-command items in UI history', () => {
|
||||||
|
it('ignores slash-command items when counting user turns', () => {
|
||||||
|
const ui: HistoryItem[] = [
|
||||||
|
userItem(1, 'hello'),
|
||||||
|
geminiItem(2),
|
||||||
|
userItem(3, '/help'), // slash command — should be skipped
|
||||||
|
userItem(5, 'world'),
|
||||||
|
geminiItem(6),
|
||||||
|
];
|
||||||
|
const api: Content[] = [
|
||||||
|
userContent('hello'),
|
||||||
|
modelContent('response 1'),
|
||||||
|
userContent('world'),
|
||||||
|
modelContent('response 2'),
|
||||||
|
];
|
||||||
|
// Rewind to 'world' (id=5): 1 real user turn before it (id=1)
|
||||||
|
// Slash '/help' (id=3) should not be counted
|
||||||
|
expect(computeApiTruncationIndex(ui, 5, api)).toBe(2);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|
@ -200,3 +221,23 @@ describe('computeApiTruncationIndex', () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('isRealUserTurn', () => {
|
||||||
|
it('returns true for normal user prompts', () => {
|
||||||
|
expect(isRealUserTurn(userItem(1, 'hello world'))).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false for slash commands', () => {
|
||||||
|
expect(isRealUserTurn(userItem(1, '/help'))).toBe(false);
|
||||||
|
expect(isRealUserTurn(userItem(1, '/rewind'))).toBe(false);
|
||||||
|
expect(isRealUserTurn(userItem(1, '/stats'))).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false for ? commands', () => {
|
||||||
|
expect(isRealUserTurn(userItem(1, '?help'))).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns false for non-user items', () => {
|
||||||
|
expect(isRealUserTurn(geminiItem(1))).toBe(false);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,17 @@
|
||||||
import type { HistoryItem } from '../types.js';
|
import type { HistoryItem } from '../types.js';
|
||||||
import type { Content } from '@google/genai';
|
import type { Content } from '@google/genai';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Returns true when the history item represents a real user prompt that was
|
||||||
|
* sent to the model, as opposed to a slash-command invocation (`/help`,
|
||||||
|
* `/stats`, …) which is stored with `type: 'user'` in the UI but never
|
||||||
|
* reaches the API history or `turnParentUuids`.
|
||||||
|
*/
|
||||||
|
export function isRealUserTurn(item: HistoryItem): boolean {
|
||||||
|
if (item.type !== 'user' || !item.text) return false;
|
||||||
|
return !item.text.startsWith('/') && !item.text.startsWith('?');
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The well-known startup context model acknowledgment.
|
* The well-known startup context model acknowledgment.
|
||||||
* Used to identify the startup context pair in the API history.
|
* Used to identify the startup context pair in the API history.
|
||||||
|
|
@ -67,7 +78,8 @@ function hasStartupContext(apiHistory: Content[]): boolean {
|
||||||
* @param uiHistory The full UI history array
|
* @param uiHistory The full UI history array
|
||||||
* @param targetUserItemId The ID of the user HistoryItem to rewind to
|
* @param targetUserItemId The ID of the user HistoryItem to rewind to
|
||||||
* @param apiHistory The current API Content[] array
|
* @param apiHistory The current API Content[] array
|
||||||
* @returns The number of Content entries to keep in the API history
|
* @returns The number of Content entries to keep, or -1 if the target turn
|
||||||
|
* could not be located (e.g., it was absorbed by chat compression).
|
||||||
*/
|
*/
|
||||||
export function computeApiTruncationIndex(
|
export function computeApiTruncationIndex(
|
||||||
uiHistory: HistoryItem[],
|
uiHistory: HistoryItem[],
|
||||||
|
|
@ -80,7 +92,7 @@ export function computeApiTruncationIndex(
|
||||||
if (item.id === targetUserItemId) {
|
if (item.id === targetUserItemId) {
|
||||||
break;
|
break;
|
||||||
}
|
}
|
||||||
if (item.type === 'user') {
|
if (isRealUserTurn(item)) {
|
||||||
uiUserTurnCount++;
|
uiUserTurnCount++;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -109,6 +121,6 @@ export function computeApiTruncationIndex(
|
||||||
}
|
}
|
||||||
|
|
||||||
// If we didn't find enough user prompts (e.g., after compression),
|
// If we didn't find enough user prompts (e.g., after compression),
|
||||||
// return the full API history length
|
// signal that the target turn is unreachable.
|
||||||
return apiHistory.length;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue