From e3cb501a67421dcdc0720bf2d2e3668b0b6f6304 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Mon, 15 Dec 2025 11:38:46 -0700 Subject: [PATCH] fix: update applyDiffTool experiment tests for new routing architecture --- .../applyDiffTool.experiment.spec.ts | 149 +++++++++++------- 1 file changed, 93 insertions(+), 56 deletions(-) diff --git a/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts b/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts index 4e0044c5ee..8c6d89c6d8 100644 --- a/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts +++ b/src/core/tools/__tests__/applyDiffTool.experiment.spec.ts @@ -8,17 +8,21 @@ vi.mock("vscode", () => ({ }, })) -// Mock the ApplyDiffTool module -vi.mock("../ApplyDiffTool", () => ({ - applyDiffTool: { - handle: vi.fn(), - }, -})) - // Import after mocking to get the mocked version import { applyDiffTool as multiApplyDiffTool } from "../MultiApplyDiffTool" -import { applyDiffTool as applyDiffToolClass } from "../ApplyDiffTool" +/** + * These tests verify that multiApplyDiffTool properly handles multi-file operations. + * + * NOTE: Routing between single-file and multi-file tools is now done in presentAssistantMessage.ts + * based on nativeArgs format. multiApplyDiffTool is responsible for: + * - Handling XML args format (multi-file XML protocol) + * - Handling nativeArgs.files format (multi-file native protocol) + * - Handling legacy path/diff params (single file, XML protocol) + * + * The routing logic tests (when to use applyDiffTool vs multiApplyDiffTool) should be + * tested in presentAssistantMessage.spec.ts or integration tests. + */ describe("applyDiffTool experiment routing", () => { let mockCline: any let mockBlock: any @@ -68,9 +72,22 @@ describe("applyDiffTool experiment routing", () => { }), }, processQueuedMessages: vi.fn(), + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), + // Required for file access validation + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + }, + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + }, + say: vi.fn().mockResolvedValue(undefined), } as any mockBlock = { + type: "tool_use", + name: "apply_diff", params: { path: "test.ts", diff: "test diff", @@ -84,16 +101,18 @@ describe("applyDiffTool experiment routing", () => { mockRemoveClosingTag = vi.fn((tag, value) => value) }) - it("should use legacy tool when MULTI_FILE_APPLY_DIFF experiment is disabled", async () => { + it("should handle legacy params directly when experiment is disabled", async () => { + // With new architecture, multiApplyDiffTool handles the request directly + // when called with legacy params. Routing to applyDiffTool (single-file) + // is now done in presentAssistantMessage.ts BEFORE calling this function. mockProvider.getState.mockResolvedValue({ experiments: { [EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: false, }, }) - // Mock the class-based tool to resolve successfully - ;(applyDiffToolClass.handle as any).mockResolvedValue(undefined) - + // This will result in an error because the file doesn't exist, + // but it verifies the function processes the request directly await multiApplyDiffTool( mockCline, mockBlock, @@ -103,21 +122,15 @@ describe("applyDiffTool experiment routing", () => { mockRemoveClosingTag, ) - expect(applyDiffToolClass.handle).toHaveBeenCalledWith(mockCline, mockBlock, { - askApproval: mockAskApproval, - handleError: mockHandleError, - pushToolResult: mockPushToolResult, - removeClosingTag: mockRemoveClosingTag, - toolProtocol: "xml", - }) + // Function should process the request - it will push a result (error about missing file) + expect(mockPushToolResult).toHaveBeenCalled() }) - it("should use legacy tool when experiments are not defined", async () => { + it("should handle legacy params directly when experiments are not defined", async () => { mockProvider.getState.mockResolvedValue({}) - // Mock the class-based tool to resolve successfully - ;(applyDiffToolClass.handle as any).mockResolvedValue(undefined) - + // This will result in an error because the file doesn't exist, + // but it verifies the function processes the request directly await multiApplyDiffTool( mockCline, mockBlock, @@ -127,24 +140,19 @@ describe("applyDiffTool experiment routing", () => { mockRemoveClosingTag, ) - expect(applyDiffToolClass.handle).toHaveBeenCalledWith(mockCline, mockBlock, { - askApproval: mockAskApproval, - handleError: mockHandleError, - pushToolResult: mockPushToolResult, - removeClosingTag: mockRemoveClosingTag, - toolProtocol: "xml", - }) + // Function should process the request - it will push a result (error about missing file) + expect(mockPushToolResult).toHaveBeenCalled() }) - it("should use multi-file tool when MULTI_FILE_APPLY_DIFF experiment is enabled and using XML protocol", async () => { + it("should handle multi-file operations when MULTI_FILE_APPLY_DIFF experiment is enabled", async () => { mockProvider.getState.mockResolvedValue({ experiments: { [EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: true, }, }) - // Mock the new tool behavior - it should continue with the multi-file implementation - // Since we're not mocking the entire function, we'll just verify it doesn't call the class-based tool + // This will result in an error because the file doesn't exist, + // but it verifies the function processes the request directly await multiApplyDiffTool( mockCline, mockBlock, @@ -154,45 +162,74 @@ describe("applyDiffTool experiment routing", () => { mockRemoveClosingTag, ) - expect(applyDiffToolClass.handle).not.toHaveBeenCalled() + // Function should process the request directly + expect(mockPushToolResult).toHaveBeenCalled() }) - it("should use class-based tool when model defaults to native protocol", async () => { - // Update model to support native tools and default to native protocol - mockCline.api.getModel = vi.fn().mockReturnValue({ - id: "test-model", - info: { - maxTokens: 4096, - contextWindow: 128000, - supportsPromptCache: false, - supportsNativeTools: true, // Model supports native tools - defaultToolProtocol: "native", // Model defaults to native protocol - }, - }) - + it("should handle native multi-file format (nativeArgs.files)", async () => { + // Test that multiApplyDiffTool properly handles native multi-file format mockProvider.getState.mockResolvedValue({ experiments: { [EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: true, }, }) - ;(applyDiffToolClass.handle as any).mockResolvedValue(undefined) + + const blockWithNativeArgs = { + type: "tool_use" as const, + name: "apply_diff" as const, + params: {}, + partial: false, + nativeArgs: { + files: [ + { path: "test1.ts", diff: "test diff 1" }, + { path: "test2.ts", diff: "test diff 2" }, + ], + }, + } await multiApplyDiffTool( mockCline, - mockBlock, + blockWithNativeArgs, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag, ) - // When native protocol is used, should always use class-based tool - expect(applyDiffToolClass.handle).toHaveBeenCalledWith(mockCline, mockBlock, { - askApproval: mockAskApproval, - handleError: mockHandleError, - pushToolResult: mockPushToolResult, - removeClosingTag: mockRemoveClosingTag, - toolProtocol: "native", + // Function should process the multi-file request + expect(mockPushToolResult).toHaveBeenCalled() + }) + + it("should handle partial messages for native multi-file format", async () => { + mockProvider.getState.mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: true, + }, }) + + const mockAsk = vi.fn().mockResolvedValue({}) + mockCline.ask = mockAsk + + const blockWithNativeArgs = { + type: "tool_use" as const, + name: "apply_diff" as const, + params: {}, + partial: true, + nativeArgs: { + files: [{ path: "test1.ts", diff: "partial diff" }], + }, + } + + await multiApplyDiffTool( + mockCline, + blockWithNativeArgs, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // For partial messages, should call ask with partial=true + expect(mockAsk).toHaveBeenCalledWith("tool", expect.any(String), true) }) })