diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 4c0063d687..17fb04336f 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -16,6 +16,7 @@ import type { AnsiOutput, AnsiOutputDisplay, Config, + TodoResultDisplay, } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../../../config/settings.js'; import { getScreenBuffer } from '../../selection/screen-buffer.js'; @@ -141,6 +142,14 @@ vi.mock('./ToolConfirmationMessage.js', () => ({ }, })); +vi.mock('../TodoDisplay.js', () => ({ + TodoDisplay: ({ + todos, + }: { + todos: Array<{ content: string; status: string }>; + }) => {todos.map((t) => t.content).join(', ')}, +})); + // Mock settings const mockSettings: LoadedSettings = { merged: { @@ -851,6 +860,57 @@ describe('', () => { expect(lastFrame()).toMatch(/MockDiff:--- a\/file\.txt/); }); + it('suppresses todo panel when resultDisplay has unchanged flag', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Idle, + ); + const output = lastFrame() ?? ''; + expect(output).toContain('TodoWrite'); + // TodoDisplay should NOT render when unchanged is true + expect(output).not.toContain('Task A'); + expect(output).not.toContain('Task B'); + expect(output).not.toContain('in_progress'); + }); + + it('renders todo panel normally when unchanged flag is absent', () => { + const { lastFrame } = renderWithContext( + , + StreamingState.Idle, + ); + const output = lastFrame() ?? ''; + expect(output).toContain('TodoWrite'); + expect(output).toContain('Task A'); + expect(output).toContain('Task B'); + }); + it('diff results are not collapsed for completed collapsible tools (bypass shouldCollapseResult)', () => { const diffResult = { fileDiff: '--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new', diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 7bc57249a5..c104695604 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -334,7 +334,12 @@ const useResultDisplayRenderer = ( */ const TodoResultRenderer: React.FC<{ data: TodoResultDisplay }> = ({ data, -}) => ; +}) => { + if (data.unchanged) { + return null; + } + return ; +}; const PlanResultRenderer: React.FC<{ data: PlanResultDisplay; diff --git a/packages/cli/src/ui/utils/todoSnapshot.test.ts b/packages/cli/src/ui/utils/todoSnapshot.test.ts index e8bb2be1d4..e27b74be06 100644 --- a/packages/cli/src/ui/utils/todoSnapshot.test.ts +++ b/packages/cli/src/ui/utils/todoSnapshot.test.ts @@ -466,4 +466,79 @@ describe('sticky todo layout helpers', () => { expect(getStickyTodoMaxVisibleItemsForMode(8, true)).toBe(1); }); }); + + describe('unchanged snapshot guard', () => { + it('falls back to previous snapshot when the latest committed snapshot is unchanged', () => { + const history = [ + makeCustomTodoToolGroup( + [{ id: '1', content: 'Old Task', status: 'in_progress' }], + 1, + ), + makeGeminiHistoryItem('Response', 2), + makeGeminiHistoryItem('Response 2', 3), + { + type: 'tool_group' as const, + tools: [ + { + callId: 'todo-unchanged', + name: 'TodoWrite', + description: 'Update todos', + resultDisplay: { + type: 'todo_list' as const, + todos: [ + { id: '1', content: 'Old Task', status: 'in_progress' }, + ], + unchanged: true, + }, + status: ToolCallStatus.Success, + confirmationDetails: undefined, + }, + ], + id: 4, + }, + ] as HistoryItem[]; + + // It should skip the unchanged snapshot and return the previous valid one + expect(getStickyTodos(history, [])).toEqual([ + { id: '1', content: 'Old Task', status: 'in_progress' }, + ]); + }); + + it('allows history snapshot to be used when pending snapshot is unchanged', () => { + const history = [ + makeCustomTodoToolGroup( + [{ id: '1', content: 'History Task', status: 'pending' }], + 1, + ), + makeGeminiHistoryItem('Response', 2), + makeGeminiHistoryItem('Response 2', 3), + ] as HistoryItem[]; + + const pendingHistoryItems = [ + { + type: 'tool_group' as const, + tools: [ + { + callId: 'todo-pending-unchanged', + name: 'TodoWrite', + description: 'Update todos', + resultDisplay: { + type: 'todo_list' as const, + todos: [ + { id: '1', content: 'History Task', status: 'pending' }, + ], + unchanged: true, + }, + status: ToolCallStatus.Success, + confirmationDetails: undefined, + }, + ], + }, + ] as HistoryItemWithoutId[]; + + expect(getStickyTodos(history, pendingHistoryItems)).toEqual([ + { id: '1', content: 'History Task', status: 'pending' }, + ]); + }); + }); }); diff --git a/packages/cli/src/ui/utils/todoSnapshot.ts b/packages/cli/src/ui/utils/todoSnapshot.ts index 8cba854e66..af1e7e32d5 100644 --- a/packages/cli/src/ui/utils/todoSnapshot.ts +++ b/packages/cli/src/ui/utils/todoSnapshot.ts @@ -62,6 +62,9 @@ function extractTodosFromResultDisplay( candidate['type'] === 'todo_list' && Array.isArray(candidate['todos']) ) { + if (candidate['unchanged'] === true) { + return null; + } return candidate['todos'] as TodoItem[]; } } diff --git a/packages/core/src/tools/todoWrite.test.ts b/packages/core/src/tools/todoWrite.test.ts index bc3ab1bdb4..31827a5873 100644 --- a/packages/core/src/tools/todoWrite.test.ts +++ b/packages/core/src/tools/todoWrite.test.ts @@ -322,19 +322,136 @@ describe('TodoWriteTool', () => { todos: [{ id: '1', content: 'Done', status: 'completed' }], }), ); - mockFs.mkdir.mockResolvedValue(undefined); - mockAtomicWrite.mockResolvedValue(undefined); + + const result = await promptIdContext.run('todo-prompt', () => + tool + .build({ + todos: [{ id: '1', content: 'Done', status: 'completed' }], + }) + .execute(mockAbortSignal), + ); + + // Identical todos → no-op short-circuit, no write + expect(mockAtomicWrite).not.toHaveBeenCalled(); + expect(result.returnDisplay).toMatchObject({ + planId: 'finished-plan', + unchanged: true, + }); + expect(mockConfig.setActiveTodoReminder).toHaveBeenCalledWith( + 'todo-prompt', + undefined, + ); + }); + + it('should short-circuit with unchanged flag when todos are identical', async () => { + const existingTodos: TodoItem[] = [ + { id: '1', content: 'Task 1', status: 'in_progress' }, + { id: '2', content: 'Task 2', status: 'pending' }, + ]; + + mockFs.readFile.mockResolvedValue( + JSON.stringify({ planId: 'plan-abc', todos: existingTodos }), + ); + + const result = await promptIdContext.run('todo-prompt', () => + tool.build({ todos: existingTodos }).execute(mockAbortSignal), + ); + + // No file write or hooks should fire + expect(mockAtomicWrite).not.toHaveBeenCalled(); + expect(mockFs.mkdir).not.toHaveBeenCalled(); + + // Display signals unchanged to UI layer + expect(result.returnDisplay).toMatchObject({ + type: 'todo_list', + planId: 'plan-abc', + todos: existingTodos, + changes: { created: [], completed: [] }, + unchanged: true, + }); + + // LLM content tells model no change occurred + expect(result.llmContent).toContain('already up to date'); + expect(result.llmContent).toContain('No changes were needed'); + expect(result.llmContent).not.toContain('modified successfully'); + + expect(mockConfig.setActiveTodoReminder).toHaveBeenCalledWith( + 'todo-prompt', + expect.stringContaining('Task 1'), + ); + }); + + it('should not fire hooks on no-op todo_write', async () => { + const existingTodos: TodoItem[] = [ + { id: '1', content: 'Task 1', status: 'pending' }, + ]; + + const mockHookSystem = { + fireTodoCreatedEvent: vi.fn(), + fireTodoCompletedEvent: vi.fn(), + }; + mockConfig = { + getSessionId: () => 'test-session-123', + getHookSystem: () => mockHookSystem, + } as unknown as Config; + tool = new TodoWriteTool(mockConfig); + + mockFs.readFile.mockResolvedValue( + JSON.stringify({ todos: existingTodos }), + ); + + await tool.build({ todos: existingTodos }).execute(mockAbortSignal); + + expect(mockHookSystem.fireTodoCreatedEvent).not.toHaveBeenCalled(); + expect(mockHookSystem.fireTodoCompletedEvent).not.toHaveBeenCalled(); + }); + + it('should short-circuit with unchanged flag when modified_by_user yields identical todos', async () => { + const existingTodos: TodoItem[] = [ + { id: '1', content: 'Task 1', status: 'in_progress' }, + ]; + mockFs.readFile.mockResolvedValue( + JSON.stringify({ planId: 'plan-abc', todos: existingTodos }), + ); + + const modifiedContent = JSON.stringify({ todos: existingTodos }); const result = await tool .build({ - todos: [{ id: '1', content: 'Done', status: 'completed' }], + todos: [], + modified_by_user: true, + modified_content: modifiedContent, }) .execute(mockAbortSignal); - expect(result.returnDisplay).toMatchObject({ planId: 'finished-plan' }); - expect( - JSON.parse(mockAtomicWrite.mock.calls[0][1] as string), - ).toMatchObject({ planId: 'finished-plan' }); + expect(mockAtomicWrite).not.toHaveBeenCalled(); + expect(result.returnDisplay).toMatchObject({ unchanged: true }); + }); + + it('should return an error result if modified_content has invalid parsed todos', async () => { + const existingTodos: TodoItem[] = [ + { id: '1', content: 'Task 1', status: 'in_progress' }, + ]; + mockFs.readFile.mockResolvedValue( + JSON.stringify({ planId: 'plan-abc', todos: existingTodos }), + ); + + // Parsing an invalid todo list (empty content) + const modifiedContent = JSON.stringify({ + todos: [{ id: '1', content: '', status: 'pending' }], + }); + + const result = await tool + .build({ + todos: [], + modified_by_user: true, + modified_content: modifiedContent, + }) + .execute(mockAbortSignal); + + expect(mockAtomicWrite).not.toHaveBeenCalled(); + // execute catches validation errors and returns an error string + expect(result.returnDisplay).toContain('non-empty "content"'); }); it('should start a new plan after the previous plan completed', async () => { diff --git a/packages/core/src/tools/todoWrite.ts b/packages/core/src/tools/todoWrite.ts index 72f09baa1c..ed3277ef65 100644 --- a/packages/core/src/tools/todoWrite.ts +++ b/packages/core/src/tools/todoWrite.ts @@ -270,6 +270,26 @@ class TodoWriteToolInvocation extends BaseToolInvocation< this.operationType = operationType; } + private refreshActiveTodoReminder(todos: TodoItem[]): void { + const promptId = promptIdContext.getStore(); + if (!promptId) return; + + const unfinishedTodos = todos.filter((todo) => todo.status !== 'completed'); + const serializedTodos = escapeSystemReminderTags( + unfinishedTodos + .map((todo) => `- [${todo.status}] ${todo.content}`) + .join('\n'), + ); + const todoContext = serializedTodos.slice(0, MAX_ACTIVE_TODO_CONTEXT_CHARS); + + this.config.setActiveTodoReminder( + promptId, + unfinishedTodos.length > 0 + ? `\nThe current task still has unfinished todo items:\n${todoContext}${serializedTodos.length > todoContext.length ? '\n[truncated]' : ''}\nKeep the todo list current and continue the task. Do not treat a successful intermediate tool call as task completion.\n` + : undefined, + ); + } + getDescription(): string { return this.operationType === 'create' ? 'Create todos' : 'Update todos'; } @@ -298,6 +318,31 @@ class TodoWriteToolInvocation extends BaseToolInvocation< if (validationError) throw new Error(validationError); const finalTodos = candidateTodos as TodoItem[]; + if (isDeepStrictEqual(oldTodos, finalTodos)) { + debugLogger.debug( + '[TodoWriteTool] No-op: todos unchanged, skipping write/hooks', + ); + + this.refreshActiveTodoReminder(finalTodos); + + const todoResultDisplay = { + type: 'todo_list' as const, + ...(previousPlan.planId ? { planId: previousPlan.planId } : {}), + todos: finalTodos, + changes: { created: [], completed: [] }, + unchanged: true, + }; + + return { + llmContent: `Todo list is already up to date. No changes were needed. + + +Your todo list was not modified because it is already current. Continue with your existing tasks. +`, + returnDisplay: todoResultDisplay, + }; + } + // 2. Detect changes const changes = detectTodoChanges(oldTodos, finalTodos); const oldTodosMap = new Map(oldTodos.map((t) => [t.id, t])); @@ -382,27 +427,7 @@ class TodoWriteToolInvocation extends BaseToolInvocation< // 4. Write new todos AFTER all validation passes await writeTodosToFile(finalTodos, activePlanId, sessionId); - const unfinishedTodos = finalTodos.filter( - (todo) => todo.status !== 'completed', - ); - const promptId = promptIdContext.getStore(); - if (promptId) { - const serializedTodos = escapeSystemReminderTags( - unfinishedTodos - .map((todo) => `- [${todo.status}] ${todo.content}`) - .join('\n'), - ); - const todoContext = serializedTodos.slice( - 0, - MAX_ACTIVE_TODO_CONTEXT_CHARS, - ); - this.config.setActiveTodoReminder( - promptId, - unfinishedTodos.length > 0 - ? `\nThe current task still has unfinished todo items:\n${todoContext}${serializedTodos.length > todoContext.length ? '\n[truncated]' : ''}\nKeep the todo list current and continue the task. Do not treat a successful intermediate tool call as task completion.\n` - : undefined, - ); - } + this.refreshActiveTodoReminder(finalTodos); // 5. POST-WRITE PHASE: Execute hooks for side effects (logging, HTTP sync, etc.) // These hooks can now safely perform side effects knowing data is persisted diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 7ef06653c8..0295eb4c01 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -890,6 +890,7 @@ export interface TodoResultDisplay { status: 'pending' | 'in_progress' | 'completed'; blockedBy?: string[]; }>; + unchanged?: boolean; } export interface PlanResultDisplay { diff --git a/packages/core/src/utils/toolResultDisplayCompaction.test.ts b/packages/core/src/utils/toolResultDisplayCompaction.test.ts index 295a70f16b..103e8bd804 100644 --- a/packages/core/src/utils/toolResultDisplayCompaction.test.ts +++ b/packages/core/src/utils/toolResultDisplayCompaction.test.ts @@ -67,6 +67,17 @@ describe('toolResultDisplayCompaction', () => { expect(compacted).toContain('truncated from'); }); + it('should preserve the unchanged flag through compaction', () => { + const display = { + type: 'todo_list' as const, + todos: [{ id: '1', content: 'Task', status: 'pending' as const }], + changes: { created: [], completed: [] }, + unchanged: true, + }; + const compacted = compactToolResultDisplayForHistory(display); + expect((compacted as TodoResultDisplay).unchanged).toBe(true); + }); + it('uses saved session wording when compacting recording strings', () => { const value = `start-${'x'.repeat( MAX_RETAINED_TOOL_RESULT_DISPLAY_CHARS,