From 3048471ae1657d4125aa5bcecf13038d2dc8262f Mon Sep 17 00:00:00 2001 From: Shawn <5414767+playcations@users.noreply.github.com> Date: Mon, 1 Sep 2025 00:12:06 -0400 Subject: [PATCH] Ignore empty tool usage Added more test cases missing and removed cases in case nothing was changed. and made it so those get ignored by the fco --- src/core/checkpoints/__tests__/index.spec.ts | 25 +- .../file-changes/FCOMessageHandler.ts | 122 +++++---- .../__tests__/FCOMessageHandler.test.ts | 244 ++++++++++++++++-- 3 files changed, 307 insertions(+), 84 deletions(-) diff --git a/src/core/checkpoints/__tests__/index.spec.ts b/src/core/checkpoints/__tests__/index.spec.ts index 20605ba3fa..97090b7296 100644 --- a/src/core/checkpoints/__tests__/index.spec.ts +++ b/src/core/checkpoints/__tests__/index.spec.ts @@ -13,6 +13,17 @@ vitest.mock("../../../services/checkpoints", () => ({ }, })) +// Mock the TelemetryService to prevent unhandled rejections +vitest.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vitest.fn(), + captureCheckpointRestored: vitest.fn(), + captureCheckpointDiffed: vitest.fn(), + }, + }, +})) + import { describe, it, expect, beforeEach, afterEach, vitest } from "vitest" import * as path from "path" import * as fs from "fs/promises" @@ -130,6 +141,12 @@ describe("getCheckpointService orchestration", () => { }) return Promise.resolve() }) + mockService.saveCheckpoint = vitest.fn(() => { + return Promise.resolve({ + commit: "mock-checkpoint-hash", + message: "Mock checkpoint", + }) + }) // Mock the service creation ;(RepoPerTaskCheckpointService.create as any).mockReturnValue(mockService) @@ -147,7 +164,7 @@ describe("getCheckpointService orchestration", () => { hasExistingCheckpoints: false, }) - const service = getCheckpointService(task) + const service = await getCheckpointService(task) console.log("Service returned:", service) expect(service).toBe(mockService) expect(RepoPerTaskCheckpointService.create).toHaveBeenCalledWith({ @@ -167,7 +184,7 @@ describe("getCheckpointService orchestration", () => { // Set existing checkpoint service task.checkpointService = mockService - const service = getCheckpointService(task) + const service = await getCheckpointService(task) expect(service).toBe(mockService) // Should not create a new service @@ -181,7 +198,7 @@ describe("getCheckpointService orchestration", () => { enableCheckpoints: false, }) - const service = getCheckpointService(task) + const service = await getCheckpointService(task) expect(service).toBeUndefined() }) }) @@ -193,7 +210,7 @@ describe("getCheckpointService orchestration", () => { hasExistingCheckpoints: false, }) - const service = getCheckpointService(task) + const service = await getCheckpointService(task) expect(service).toBe(mockService) // initShadowGit should be called diff --git a/src/services/file-changes/FCOMessageHandler.ts b/src/services/file-changes/FCOMessageHandler.ts index 22fa1b8653..3ad4690828 100644 --- a/src/services/file-changes/FCOMessageHandler.ts +++ b/src/services/file-changes/FCOMessageHandler.ts @@ -50,11 +50,16 @@ export class FCOMessageHandler { task.taskId, task.fileContextTracker, ) - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: filteredChangeset.files.length > 0 ? filteredChangeset : undefined, - }) + // Only send update if there are actual changes + if (filteredChangeset.files.length > 0) { + this.provider.postMessageToWebview({ + type: "filesChanged", + filesChanged: filteredChangeset, + }) + } + // If no changes, don't send anything - keep FCO in current state } + // If can't filter, don't send anything - keep FCO in current state break } @@ -189,15 +194,18 @@ export class FCOMessageHandler { if (message.uri && acceptFileChangeManager && task?.taskId && task?.fileContextTracker) { await acceptFileChangeManager.acceptChange(message.uri) - // Send updated state with LLM-only filtering + // Send updated state with LLM-only filtering only if there are remaining changes const updatedChangeset = await acceptFileChangeManager.getLLMOnlyChanges( task.taskId, task.fileContextTracker, ) - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined, - }) + if (updatedChangeset.files.length > 0) { + this.provider.postMessageToWebview({ + type: "filesChanged", + filesChanged: updatedChangeset, + }) + } + // If no remaining changes, don't send anything - keep FCO in current state } } @@ -239,28 +247,27 @@ export class FCOMessageHandler { // Remove from tracking since the file has been reverted await rejectFileChangeManager.rejectChange(message.uri) - // Send updated state with LLM-only filtering + // Send updated state with LLM-only filtering only if there are remaining changes if (currentTask?.taskId && currentTask?.fileContextTracker) { const updatedChangeset = await rejectFileChangeManager.getLLMOnlyChanges( currentTask.taskId, currentTask.fileContextTracker, ) - console.log(`[FCO] After rejection, sending ${updatedChangeset.files.length} LLM-only files to webview`) - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined, - }) + console.log(`[FCO] After rejection, found ${updatedChangeset.files.length} remaining LLM-only files`) + if (updatedChangeset.files.length > 0) { + this.provider.postMessageToWebview({ + type: "filesChanged", + filesChanged: updatedChangeset, + }) + } + // If no remaining changes, don't send anything - keep FCO in current state } } catch (error) { console.error(`[FCO] Error reverting file ${message.uri}:`, error) // Fall back to old behavior (just remove from display) if reversion fails await rejectFileChangeManager.rejectChange(message.uri) - const updatedChangeset = rejectFileChangeManager.getChanges() - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined, - }) + // Don't send fallback message - just log the error and keep FCO in current state } } @@ -271,7 +278,7 @@ export class FCOMessageHandler { } await acceptAllFileChangeManager?.acceptAll() - // Clear state + // Clear FCO state - this is the one case where we DO want to clear the UI this.provider.postMessageToWebview({ type: "filesChanged", filesChanged: undefined, @@ -345,40 +352,40 @@ export class FCOMessageHandler { fileChangeManager = await this.provider.ensureFileChangeManager() } - if (fileChangeManager && task?.checkpointService) { - const changeset = fileChangeManager.getChanges() - + if (fileChangeManager) { // Handle message file changes if provided if (message.fileChanges) { const fileChanges = message.fileChanges.map((fc: any) => ({ uri: fc.uri, type: fc.type, - fromCheckpoint: task.checkpointService?.baseHash || "base", + fromCheckpoint: task?.checkpointService?.baseHash || "base", toCheckpoint: "current", })) fileChangeManager.setFiles(fileChanges) } - // Get filtered changeset and send to webview - const filteredChangeset = fileChangeManager.getChanges() - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: filteredChangeset.files.length > 0 ? filteredChangeset : undefined, - }) - } else { - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: undefined, - }) + // Get LLM-only filtered changeset and send to webview only if there are changes + if (task?.taskId && task?.fileContextTracker) { + const filteredChangeset = await fileChangeManager.getLLMOnlyChanges( + task.taskId, + task.fileContextTracker, + ) + // Only send update if there are actual changes + if (filteredChangeset.files.length > 0) { + this.provider.postMessageToWebview({ + type: "filesChanged", + filesChanged: filteredChangeset, + }) + } + // If no changes, don't send anything - keep FCO in current state + } + // If can't filter, don't send anything - keep FCO in current state } + // If no fileChangeManager, don't send anything - keep FCO in current state } catch (error) { console.error("FCOMessageHandler: Error handling filesChangedRequest:", error) - // Send empty response to prevent FCO from hanging - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: undefined, - }) + // Don't send anything on error - keep FCO in current state } } @@ -393,24 +400,27 @@ export class FCOMessageHandler { // Update baseline to the specified checkpoint await fileChangeManager.updateBaseline(message.baseline) - // Send updated state - const updatedChangeset = fileChangeManager.getChanges() - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: updatedChangeset.files.length > 0 ? updatedChangeset : undefined, - }) - } else { - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: undefined, - }) + // Send updated state with LLM-only filtering only if there are changes + if (task.taskId && task.fileContextTracker) { + const updatedChangeset = await fileChangeManager.getLLMOnlyChanges( + task.taskId, + task.fileContextTracker, + ) + // Only send update if there are actual changes + if (updatedChangeset.files.length > 0) { + this.provider.postMessageToWebview({ + type: "filesChanged", + filesChanged: updatedChangeset, + }) + } + // If no changes, don't send anything - keep FCO in current state + } + // If can't filter, don't send anything - keep FCO in current state } + // If conditions not met, don't send anything - keep FCO in current state } catch (error) { console.error("FCOMessageHandler: Failed to update baseline:", error) - this.provider.postMessageToWebview({ - type: "filesChanged", - filesChanged: undefined, - }) + // Don't send anything on error - keep FCO in current state } } diff --git a/src/services/file-changes/__tests__/FCOMessageHandler.test.ts b/src/services/file-changes/__tests__/FCOMessageHandler.test.ts index 34669ff5a0..ae1058a31b 100644 --- a/src/services/file-changes/__tests__/FCOMessageHandler.test.ts +++ b/src/services/file-changes/__tests__/FCOMessageHandler.test.ts @@ -205,7 +205,7 @@ describe("FCOMessageHandler", () => { expect(mockProvider.ensureFileChangeManager).toHaveBeenCalled() }) - it("should send undefined when no LLM changes exist", async () => { + it("should not send message when no LLM changes exist", async () => { const emptyChangeset = { baseCheckpoint: "base123", files: [], @@ -215,10 +215,8 @@ describe("FCOMessageHandler", () => { await handler.handleMessage({ type: "webviewReady" } as WebviewMessage) - expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ - type: "filesChanged", - filesChanged: undefined, - }) + // Should not send any message when no changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("should handle missing task gracefully", async () => { @@ -227,6 +225,8 @@ describe("FCOMessageHandler", () => { await handler.handleMessage({ type: "webviewReady" } as WebviewMessage) expect(mockFileChangeManager.getLLMOnlyChanges).not.toHaveBeenCalled() + // Should not send any message when no task context + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) }) @@ -355,7 +355,7 @@ describe("FCOMessageHandler", () => { }) }) - it("should send undefined when no files remain after accept", async () => { + it("should not send message when no files remain after accept", async () => { mockFileChangeManager.getLLMOnlyChanges.mockResolvedValue({ baseCheckpoint: "base123", files: [], @@ -363,10 +363,8 @@ describe("FCOMessageHandler", () => { await handler.handleMessage(mockMessage) - expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ - type: "filesChanged", - filesChanged: undefined, - }) + // Should not send any message when no remaining changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("should handle missing FileChangeManager", async () => { @@ -397,7 +395,7 @@ describe("FCOMessageHandler", () => { mockCheckpointService.getContent.mockResolvedValue("original content") }) - it("should revert file and update changeset", async () => { + it("should revert file and not send message when no remaining changes", async () => { const updatedChangeset = { baseCheckpoint: "base123", files: [], @@ -410,10 +408,8 @@ describe("FCOMessageHandler", () => { expect(mockCheckpointService.getContent).toHaveBeenCalledWith("base123", "/test/workspace/test.txt") expect(fs.writeFile).toHaveBeenCalledWith("/test/workspace/test.txt", "original content", "utf8") expect(mockFileChangeManager.rejectChange).toHaveBeenCalledWith("test.txt") - expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ - type: "filesChanged", - filesChanged: undefined, - }) + // Should not send any message when no remaining changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("should delete newly created files", async () => { @@ -505,10 +501,8 @@ describe("FCOMessageHandler", () => { await handler.handleMessage(mockMessage) expect(mockFileChangeManager.setFiles).not.toHaveBeenCalled() - expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ - type: "filesChanged", - filesChanged: undefined, - }) + // Should not send any message when no changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("should handle errors gracefully", async () => { @@ -520,11 +514,215 @@ describe("FCOMessageHandler", () => { await handler.handleMessage(mockMessage) + // Should not send any message on error + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should not send message when task context is missing", async () => { + // Mock task without taskId + mockProvider.getCurrentTask.mockReturnValue({ + fileContextTracker: mockFileContextTracker, + checkpointService: mockCheckpointService, + // Missing taskId + }) + + const mockMessage = { + type: "filesChangedRequest" as const, + } + + await handler.handleMessage(mockMessage) + + // Should not call getLLMOnlyChanges when taskId is missing + expect(mockFileChangeManager.getLLMOnlyChanges).not.toHaveBeenCalled() + // Should not send any message when task context is missing + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should not send message when fileContextTracker is missing", async () => { + // Mock task without fileContextTracker + mockProvider.getCurrentTask.mockReturnValue({ + taskId: "test-task-id", + checkpointService: mockCheckpointService, + // Missing fileContextTracker + }) + + const mockMessage = { + type: "filesChangedRequest" as const, + } + + await handler.handleMessage(mockMessage) + + // Should not call getLLMOnlyChanges when fileContextTracker is missing + expect(mockFileChangeManager.getLLMOnlyChanges).not.toHaveBeenCalled() + // Should not send any message when fileContextTracker is missing + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + }) + + describe("filesChangedBaselineUpdate", () => { + it("should update baseline and send LLM-only changes", async () => { + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + const updatedChangeset = { + baseCheckpoint: "new-baseline-123", + files: [ + { + uri: "updated.txt", + type: "edit" as const, + fromCheckpoint: "new-baseline-123", + toCheckpoint: "current", + linesAdded: 3, + linesRemoved: 1, + }, + ], + } + + mockFileChangeManager.getLLMOnlyChanges.mockResolvedValue(updatedChangeset) + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).toHaveBeenCalledWith("new-baseline-123") + expect(mockFileChangeManager.getLLMOnlyChanges).toHaveBeenCalledWith("test-task-id", mockFileContextTracker) expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "filesChanged", - filesChanged: undefined, + filesChanged: updatedChangeset, }) }) + + it("should not send message when no LLM changes remain after baseline update", async () => { + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + mockFileChangeManager.getLLMOnlyChanges.mockResolvedValue({ + baseCheckpoint: "new-baseline-123", + files: [], + }) + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).toHaveBeenCalledWith("new-baseline-123") + // Should not send any message when no changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should not send message when task context is missing", async () => { + // Mock task without taskId + mockProvider.getCurrentTask.mockReturnValue({ + fileContextTracker: mockFileContextTracker, + checkpointService: mockCheckpointService, + // Missing taskId + }) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).toHaveBeenCalledWith("new-baseline-123") + // Should not call getLLMOnlyChanges when taskId is missing + expect(mockFileChangeManager.getLLMOnlyChanges).not.toHaveBeenCalled() + // Should not send any message when task context is missing + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should not send message when fileContextTracker is missing", async () => { + // Mock task without fileContextTracker + mockProvider.getCurrentTask.mockReturnValue({ + taskId: "test-task-id", + checkpointService: mockCheckpointService, + // Missing fileContextTracker + }) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).toHaveBeenCalledWith("new-baseline-123") + // Should not call getLLMOnlyChanges when fileContextTracker is missing + expect(mockFileChangeManager.getLLMOnlyChanges).not.toHaveBeenCalled() + // Should not send any message when fileContextTracker is missing + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should handle missing FileChangeManager", async () => { + mockProvider.getFileChangeManager.mockReturnValue(null) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + expect(mockProvider.ensureFileChangeManager).toHaveBeenCalled() + }) + + it("should not send message when no baseline provided", async () => { + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + // No baseline property + } + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).not.toHaveBeenCalled() + // Should not send any message when no baseline provided + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should not send message when task is missing", async () => { + mockProvider.getCurrentTask.mockReturnValue(null) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).not.toHaveBeenCalled() + // Should not send any message when task is missing + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should handle updateBaseline errors gracefully", async () => { + mockFileChangeManager.updateBaseline.mockRejectedValue(new Error("Baseline update failed")) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + // Should not throw and not send any message on error + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) + + it("should handle getLLMOnlyChanges errors gracefully", async () => { + mockFileChangeManager.getLLMOnlyChanges.mockRejectedValue(new Error("Filter error")) + + const mockMessage = { + type: "filesChangedBaselineUpdate" as const, + baseline: "new-baseline-123", + } + + await handler.handleMessage(mockMessage) + + expect(mockFileChangeManager.updateBaseline).toHaveBeenCalledWith("new-baseline-123") + // Should not send any message when filtering fails + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() + }) }) describe("LLM Filtering Edge Cases", () => { @@ -540,10 +738,8 @@ describe("FCOMessageHandler", () => { await handler.handleMessage({ type: "webviewReady" } as WebviewMessage) - expect(mockProvider.postMessageToWebview).toHaveBeenCalledWith({ - type: "filesChanged", - filesChanged: undefined, - }) + // Should not send any message when no changes + expect(mockProvider.postMessageToWebview).not.toHaveBeenCalled() }) it("should handle mixed LLM and user-edited files", async () => {