From f6cae4df0587dbd61e39fa9a34fdc510fac7b388 Mon Sep 17 00:00:00 2001 From: Xudong Zhang Date: Tue, 13 May 2025 12:38:36 +0800 Subject: [PATCH] fix(task): temporary fix for the ask error (#3471) Co-authored-by: cte --- src/core/checkpoints/index.ts | 12 ++- src/core/task/Task.ts | 80 ++++++++++++------- src/core/tools/__tests__/readFileTool.test.ts | 40 ++++------ src/core/tools/applyDiffTool.ts | 2 +- src/core/tools/attemptCompletionTool.ts | 6 +- src/core/tools/insertContentTool.ts | 2 +- src/core/tools/listCodeDefinitionNamesTool.ts | 2 +- src/core/tools/readFileTool.ts | 2 +- src/core/tools/searchAndReplaceTool.ts | 2 +- src/core/tools/writeToFileTool.ts | 2 +- 10 files changed, 81 insertions(+), 69 deletions(-) diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index 8c5dd5203f..3a5c2dde45 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -93,10 +93,14 @@ export function getCheckpointService(cline: Task) { try { provider?.postMessageToWebview({ type: "currentCheckpointUpdated", text: to }) - cline.say("checkpoint_saved", to, undefined, undefined, { isFirst, from, to }).catch((err) => { - log("[Cline#getCheckpointService] caught unexpected error in say('checkpoint_saved')") - console.error(err) - }) + cline + .say("checkpoint_saved", to, undefined, undefined, { isFirst, from, to }, undefined, { + isNonInteractive: true, + }) + .catch((err) => { + log("[Cline#getCheckpointService] caught unexpected error in say('checkpoint_saved')") + console.error(err) + }) } catch (err) { log("[Cline#getCheckpointService] caught unexpected error in on('checkpoint'), disabling checkpoints") console.error(err) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 0f5029be9a..bdc041ef57 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -164,7 +164,7 @@ export class Task extends EventEmitter { consecutiveMistakeCount: number = 0 consecutiveMistakeLimit: number consecutiveMistakeCountForApplyDiff: Map = new Map() - private toolUsage: ToolUsage = {} + toolUsage: ToolUsage = {} // Checkpoints enableCheckpoints: boolean @@ -488,6 +488,9 @@ export class Task extends EventEmitter { partial?: boolean, checkpoint?: Record, progressStatus?: ToolProgressStatus, + options: { + isNonInteractive?: boolean + } = {}, ): Promise { if (this.abort) { throw new Error(`[Cline#say] task ${this.taskId}.${this.instanceId} aborted`) @@ -495,49 +498,71 @@ export class Task extends EventEmitter { if (partial !== undefined) { const lastMessage = this.clineMessages.at(-1) + const isUpdatingPreviousPartial = lastMessage && lastMessage.partial && lastMessage.type === "say" && lastMessage.say === type + if (partial) { if (isUpdatingPreviousPartial) { - // existing partial message, so update it + // Existing partial message, so update it. lastMessage.text = text lastMessage.images = images lastMessage.partial = partial lastMessage.progressStatus = progressStatus this.updateClineMessage(lastMessage) } else { - // this is a new partial message, so add it with partial state + // This is a new partial message, so add it with partial state. const sayTs = Date.now() - this.lastMessageTs = sayTs + + if (!options.isNonInteractive) { + this.lastMessageTs = sayTs + } + await this.addToClineMessages({ ts: sayTs, type: "say", say: type, text, images, partial }) } } else { // New now have a complete version of a previously partial message. + // This is the complete version of a previously partial + // message, so replace the partial with the complete version. if (isUpdatingPreviousPartial) { - // This is the complete version of a previously partial - // message, so replace the partial with the complete version. - this.lastMessageTs = lastMessage.ts - // lastMessage.ts = sayTs + if (!options.isNonInteractive) { + this.lastMessageTs = lastMessage.ts + } + lastMessage.text = text lastMessage.images = images lastMessage.partial = false lastMessage.progressStatus = progressStatus + // Instead of streaming partialMessage events, we do a save // and post like normal to persist to disk. await this.saveClineMessages() - // More performant than an entire postStateToWebview. + + // More performant than an entire `postStateToWebview`. this.updateClineMessage(lastMessage) } else { // This is a new and complete message, so add it like normal. const sayTs = Date.now() - this.lastMessageTs = sayTs + + if (!options.isNonInteractive) { + this.lastMessageTs = sayTs + } + await this.addToClineMessages({ ts: sayTs, type: "say", say: type, text, images }) } } } else { - // this is a new non-partial message, so add it like normal + // This is a new non-partial message, so add it like normal. const sayTs = Date.now() - this.lastMessageTs = sayTs + + // A "non-interactive" message is a message is one that the user + // does not need to respond to. We don't want these message types + // to trigger an update to `lastMessageTs` since they can be created + // asynchronously and could interrupt a pending ask. + if (!options.isNonInteractive) { + this.lastMessageTs = sayTs + } + await this.addToClineMessages({ ts: sayTs, type: "say", say: type, text, images, checkpoint }) } } @@ -555,8 +580,12 @@ export class Task extends EventEmitter { // Start / Abort / Resume private async startTask(task?: string, images?: string[]): Promise { - // conversationHistory (for API) and clineMessages (for webview) need to be in sync - // if the extension process were killed, then on restart the clineMessages might not be empty, so we need to set it to [] when we create a new Cline client (otherwise webview would show stale messages from previous session) + // `conversationHistory` (for API) and `clineMessages` (for webview) + // need to be in sync. + // If the extension process were killed, then on restart the + // `clineMessages` might not be empty, so we need to set it to [] when + // we create a new Cline client (otherwise webview would show stale + // messages from previous session). this.clineMessages = [] this.apiConversationHistory = [] await this.providerRef.deref()?.postStateToWebview() @@ -578,28 +607,25 @@ export class Task extends EventEmitter { } public async resumePausedTask(lastMessage: string) { - // release this Cline instance from paused state + // Release this Cline instance from paused state. this.isPaused = false this.emit("taskUnpaused") - // fake an answer from the subtask that it has completed running and this is the result of what it has done - // add the message to the chat history and to the webview ui + // Fake an answer from the subtask that it has completed running and + // this is the result of what it has done add the message to the chat + // history and to the webview ui. try { await this.say("subtask_result", lastMessage) await this.addToApiConversationHistory({ role: "user", - content: [ - { - type: "text", - text: `[new_task completed] Result: ${lastMessage}`, - }, - ], + content: [{ type: "text", text: `[new_task completed] Result: ${lastMessage}` }], }) } catch (error) { this.providerRef .deref() ?.log(`Error failed to add reply from subtast into conversation of parent task, error: ${error}`) + throw error } } @@ -1629,17 +1655,9 @@ export class Task extends EventEmitter { } } - public getToolUsage() { - return this.toolUsage - } - // Getters public get cwd() { return this.workspacePath } - - public getFileContextTracker(): FileContextTracker { - return this.fileContextTracker - } } diff --git a/src/core/tools/__tests__/readFileTool.test.ts b/src/core/tools/__tests__/readFileTool.test.ts index fab395d285..f0b3600a26 100644 --- a/src/core/tools/__tests__/readFileTool.test.ts +++ b/src/core/tools/__tests__/readFileTool.test.ts @@ -33,15 +33,15 @@ let mockInputContent = "" jest.mock("../../../integrations/misc/extract-text", () => { const actual = jest.requireActual("../../../integrations/misc/extract-text") - // Create a spy on the actual addLineNumbers function + // Create a spy on the actual addLineNumbers function. const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers") return { ...actual, - // Expose the spy so tests can access it + // Expose the spy so tests can access it. __addLineNumbersSpy: addLineNumbersSpy, extractTextFromFile: jest.fn().mockImplementation((_filePath) => { - // Use the actual addLineNumbers function + // Use the actual addLineNumbers function. const content = mockInputContent return Promise.resolve(actual.addLineNumbers(content)) }), @@ -87,7 +87,6 @@ describe("read_file tool with maxReadFileLine setting", () => { const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction const mockedPathResolve = path.resolve as jest.MockedFunction - // Mock instances const mockCline: any = {} let mockProvider: any let toolResult: ToolResponse | undefined @@ -95,31 +94,26 @@ describe("read_file tool with maxReadFileLine setting", () => { beforeEach(() => { jest.clearAllMocks() - // Setup path resolution mockedPathResolve.mockReturnValue(absoluteFilePath) - - // Setup mocks for file operations mockedIsBinaryFile.mockResolvedValue(false) - // Set the default content for the mock mockInputContent = fileContent - // Setup the extractTextFromFile mock implementation with the current mockInputContent + // Setup the extractTextFromFile mock implementation with the current + // mockInputContent. mockedExtractTextFromFile.mockImplementation((_filePath) => { const actual = jest.requireActual("../../../integrations/misc/extract-text") return Promise.resolve(actual.addLineNumbers(mockInputContent)) }) // No need to setup the extractTextFromFile mock implementation here - // as it's already defined at the module level + // as it's already defined at the module level. - // Setup mock provider mockProvider = { getState: jest.fn(), deref: jest.fn().mockReturnThis(), } - // Setup Cline instance with mock methods mockCline.cwd = "/" mockCline.task = "Test" mockCline.providerRef = mockProvider @@ -129,12 +123,14 @@ describe("read_file tool with maxReadFileLine setting", () => { mockCline.say = jest.fn().mockResolvedValue(undefined) mockCline.ask = jest.fn().mockResolvedValue(true) mockCline.presentAssistantMessage = jest.fn() - mockCline.getFileContextTracker = jest.fn().mockReturnValue({ + + mockCline.fileContextTracker = { trackFileContext: jest.fn().mockResolvedValue(undefined), - }) + } + mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined) mockCline.recordToolError = jest.fn().mockReturnValue(undefined) - // Reset tool result + toolResult = undefined }) @@ -433,22 +429,16 @@ describe("read_file tool XML output structure", () => { beforeEach(() => { jest.clearAllMocks() - // Setup path resolution mockedPathResolve.mockReturnValue(absoluteFilePath) - - // Setup mocks for file operations mockedIsBinaryFile.mockResolvedValue(false) - // Set the default content for the mock mockInputContent = fileContent - // Setup mock provider mockProvider = { getState: jest.fn().mockResolvedValue({ maxReadFileLine: 500 }), deref: jest.fn().mockReturnThis(), } - // Setup Cline instance with mock methods mockCline.cwd = "/" mockCline.task = "Test" mockCline.providerRef = mockProvider @@ -459,14 +449,14 @@ describe("read_file tool XML output structure", () => { mockCline.ask = jest.fn().mockResolvedValue(true) mockCline.presentAssistantMessage = jest.fn() mockCline.sayAndCreateMissingParamError = jest.fn().mockResolvedValue("Missing required parameter") - // Add mock for getFileContextTracker method - mockCline.getFileContextTracker = jest.fn().mockReturnValue({ + + mockCline.fileContextTracker = { trackFileContext: jest.fn().mockResolvedValue(undefined), - }) + } + mockCline.recordToolUsage = jest.fn().mockReturnValue(undefined) mockCline.recordToolError = jest.fn().mockReturnValue(undefined) - // Reset tool result toolResult = undefined }) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index 19cb874be8..0fc9d9796c 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -164,7 +164,7 @@ export async function applyDiffTool( // Track file edit operation if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "roo_edited" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } // Used to determine if we should wait for busy terminal to update before sending api request diff --git a/src/core/tools/attemptCompletionTool.ts b/src/core/tools/attemptCompletionTool.ts index 998101fc4a..a5e469c77f 100644 --- a/src/core/tools/attemptCompletionTool.ts +++ b/src/core/tools/attemptCompletionTool.ts @@ -46,7 +46,7 @@ export async function attemptCompletionTool( await cline.say("completion_result", removeClosingTag("result", result), undefined, false) telemetryService.captureTaskCompleted(cline.taskId) - cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.getToolUsage()) + cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.toolUsage) await cline.ask("command", removeClosingTag("command", command), block.partial).catch(() => {}) } @@ -72,7 +72,7 @@ export async function attemptCompletionTool( // Haven't sent a command message yet so first send completion_result then command. await cline.say("completion_result", result, undefined, false) telemetryService.captureTaskCompleted(cline.taskId) - cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.getToolUsage()) + cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.toolUsage) } // Complete command message. @@ -97,7 +97,7 @@ export async function attemptCompletionTool( } else { await cline.say("completion_result", result, undefined, false) telemetryService.captureTaskCompleted(cline.taskId) - cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.getToolUsage()) + cline.emit("taskCompleted", cline.taskId, cline.getTokenUsage(), cline.toolUsage) } if (cline.parentTask) { diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index 33c3f88675..8c15f96a8b 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -132,7 +132,7 @@ export async function insertContentTool( // Track file edit operation if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "roo_edited" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } cline.didEditFile = true diff --git a/src/core/tools/listCodeDefinitionNamesTool.ts b/src/core/tools/listCodeDefinitionNamesTool.ts index 8c9ae74fcf..f0cdde01eb 100644 --- a/src/core/tools/listCodeDefinitionNamesTool.ts +++ b/src/core/tools/listCodeDefinitionNamesTool.ts @@ -64,7 +64,7 @@ export async function listCodeDefinitionNamesTool( } if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "read_tool" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) } pushToolResult(result) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index c2e2bf8466..fade9b2d11 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -245,7 +245,7 @@ export async function readFileTool( // Track file read operation if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "read_tool" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "read_tool" as RecordSource) } // Format the result into the required XML structure diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index 4871d246d7..48910b9a2b 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -215,7 +215,7 @@ export async function searchAndReplaceTool( // Track file edit operation if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "roo_edited" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } cline.didEditFile = true diff --git a/src/core/tools/writeToFileTool.ts b/src/core/tools/writeToFileTool.ts index 78f72b50f8..d2f22040c6 100644 --- a/src/core/tools/writeToFileTool.ts +++ b/src/core/tools/writeToFileTool.ts @@ -211,7 +211,7 @@ export async function writeToFileTool( // Track file edit operation if (relPath) { - await cline.getFileContextTracker().trackFileContext(relPath, "roo_edited" as RecordSource) + await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource) } cline.didEditFile = true // used to determine if we should wait for busy terminal to update before sending api request