From a82afccd15b8dbc9a465a0b4dbfddd388eea4073 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sun, 10 Aug 2025 16:41:45 +0000 Subject: [PATCH 1/5] fix: add type check before calling .match() on diffItem.content Fixes #6905 - Error during diff application "v.content.match is not a function" - Added type check to ensure diffItem.content is a string before calling .match() - Added comprehensive tests for handling non-string content values - Prevents runtime errors when content is null, undefined, or other non-string types --- .../__tests__/multiApplyDiffTool.spec.ts | 338 ++++++++++++++++++ src/core/tools/multiApplyDiffTool.ts | 7 +- 2 files changed, 343 insertions(+), 2 deletions(-) create mode 100644 src/core/tools/__tests__/multiApplyDiffTool.spec.ts diff --git a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts new file mode 100644 index 0000000000..f618414611 --- /dev/null +++ b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts @@ -0,0 +1,338 @@ +import { applyDiffTool } from "../multiApplyDiffTool" +import { EXPERIMENT_IDS } from "../../../shared/experiments" +import * as fs from "fs/promises" +import * as fileUtils from "../../../utils/fs" +import * as pathUtils from "../../../utils/path" + +// Mock dependencies +vi.mock("fs/promises") +vi.mock("../../../utils/fs") +vi.mock("../../../utils/path") +vi.mock("../../../utils/xml") +vi.mock("../applyDiffTool", () => ({ + applyDiffToolLegacy: vi.fn(), +})) + +describe("multiApplyDiffTool", () => { + let mockCline: any + let mockBlock: any + let mockAskApproval: any + let mockHandleError: any + let mockPushToolResult: any + let mockRemoveClosingTag: any + let mockProvider: any + + beforeEach(() => { + vi.clearAllMocks() + + mockProvider = { + getState: vi.fn().mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: true, + }, + diagnosticsEnabled: true, + writeDelayMs: 0, + }), + } + + mockCline = { + providerRef: { + deref: vi.fn().mockReturnValue(mockProvider), + }, + cwd: "/test", + taskId: "test-task", + consecutiveMistakeCount: 0, + consecutiveMistakeCountForApplyDiff: new Map(), + recordToolError: vi.fn(), + say: vi.fn().mockResolvedValue(undefined), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), + ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }), + diffStrategy: { + applyDiff: vi.fn().mockResolvedValue({ + success: true, + content: "modified content", + }), + getProgressStatus: vi.fn(), + }, + diffViewProvider: { + reset: vi.fn().mockResolvedValue(undefined), + editType: undefined, + originalContent: undefined, + open: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + scrollToFirstDiff: vi.fn(), + saveDirectly: vi.fn().mockResolvedValue(undefined), + saveChanges: vi.fn().mockResolvedValue(undefined), + pushToolWriteResult: vi.fn().mockResolvedValue("File modified successfully"), + }, + api: { + getModel: vi.fn().mockReturnValue({ id: "test-model" }), + }, + rooIgnoreController: { + validateAccess: vi.fn().mockReturnValue(true), + }, + rooProtectedController: { + isWriteProtected: vi.fn().mockReturnValue(false), + }, + fileContextTracker: { + trackFileContext: vi.fn().mockResolvedValue(undefined), + }, + didEditFile: false, + } as any + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn() + mockPushToolResult = vi.fn() + mockRemoveClosingTag = vi.fn((tag, value) => value) + + // Mock file system operations + ;(fileUtils.fileExistsAtPath as any).mockResolvedValue(true) + ;(fs.readFile as any).mockResolvedValue("original content") + ;(pathUtils.getReadablePath as any).mockImplementation((cwd: string, path: string) => path) + }) + + describe("SEARCH block counting with non-string content", () => { + it("should handle diffItem.content being undefined", async () => { + mockBlock = { + params: { + path: "test.ts", + diff: undefined, // This will result in undefined content + }, + partial: false, + } + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without throwing an error + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("should handle diffItem.content being null", async () => { + mockBlock = { + params: { + args: ` + test.ts + + + + `, + }, + partial: false, + } + + // Mock parseXml to return null content + const parseXml = await import("../../../utils/xml") + ;(parseXml.parseXml as any).mockReturnValue({ + file: { + path: "test.ts", + diff: { + content: null, + }, + }, + }) + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without throwing an error + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("should handle diffItem.content being a number", async () => { + mockBlock = { + params: { + args: ` + test.ts + + 123 + + `, + }, + partial: false, + } + + // Mock parseXml to return number content + const parseXml = await import("../../../utils/xml") + ;(parseXml.parseXml as any).mockReturnValue({ + file: { + path: "test.ts", + diff: { + content: 123, // Number instead of string + }, + }, + }) + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without throwing an error + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("should correctly count SEARCH blocks when content is a valid string", async () => { + const diffContent = `<<<<<<< SEARCH +old content +======= +new content +>>>>>>> REPLACE + +<<<<<<< SEARCH +another old content +======= +another new content +>>>>>>> REPLACE` + + mockBlock = { + params: { + path: "test.ts", + diff: diffContent, + }, + partial: false, + } + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete successfully + expect(mockPushToolResult).toHaveBeenCalled() + const resultCall = mockPushToolResult.mock.calls[0][0] + // Should not include the single block notice since we have 2 blocks + expect(resultCall).not.toContain("Making multiple related changes") + }) + + it("should show single block notice when only one SEARCH block exists", async () => { + const diffContent = `<<<<<<< SEARCH +old content +======= +new content +>>>>>>> REPLACE` + + mockBlock = { + params: { + path: "test.ts", + diff: diffContent, + }, + partial: false, + } + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete successfully + expect(mockPushToolResult).toHaveBeenCalled() + const resultCall = mockPushToolResult.mock.calls[0][0] + // Should include the single block notice + expect(resultCall).toContain("Making multiple related changes") + }) + }) + + describe("Edge cases for diff content", () => { + it("should handle empty diff array", async () => { + mockBlock = { + params: { + args: ` + test.ts + + `, + }, + partial: false, + } + + const parseXml = await import("../../../utils/xml") + ;(parseXml.parseXml as any).mockReturnValue({ + file: { + path: "test.ts", + diff: [], + }, + }) + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without error + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + + it("should handle mixed content types in diff array", async () => { + mockBlock = { + params: { + args: ` + test.ts + + valid string content + + `, + }, + partial: false, + } + + const parseXml = await import("../../../utils/xml") + ;(parseXml.parseXml as any).mockReturnValue({ + file: { + path: "test.ts", + diff: [ + { content: "<<<<<<< SEARCH\ntest\n=======\nreplaced\n>>>>>>> REPLACE" }, + { content: null }, + { content: undefined }, + { content: 42 }, + { content: "<<<<<<< SEARCH\nmore\n=======\nchanges\n>>>>>>> REPLACE" }, + ], + }, + }) + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without error and count only valid string SEARCH blocks + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + }) + }) +}) diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index db514d2b64..af6e196b51 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -656,8 +656,11 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} let totalSearchBlocks = 0 for (const operation of operations) { for (const diffItem of operation.diff) { - const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length - totalSearchBlocks += searchBlocks + // Ensure content is a string before calling match + if (typeof diffItem.content === "string") { + const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length + totalSearchBlocks += searchBlocks + } } } From 18882b293e58f6cda6f6113bcf0658c492f1dc25 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 14 Aug 2025 17:33:12 -0500 Subject: [PATCH 2/5] fix: validate content type earlier in diff parsing - Move type check to where content is first extracted from XML - Remove redundant check since content is now guaranteed to be string - Add test for early content validation --- .../__tests__/multiApplyDiffTool.spec.ts | 50 +++++++++++++++++++ src/core/tools/multiApplyDiffTool.ts | 22 ++++---- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts index f618414611..b7a07bfa4e 100644 --- a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts +++ b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts @@ -91,6 +91,56 @@ describe("multiApplyDiffTool", () => { ;(pathUtils.getReadablePath as any).mockImplementation((cwd: string, path: string) => path) }) + describe("Early content validation", () => { + it("should filter out non-string content at parse time", async () => { + mockBlock = { + params: { + args: ` + test.ts + + valid string content + + `, + }, + partial: false, + } + + // Mock parseXml to return mixed content types + const parseXml = await import("../../../utils/xml") + ;(parseXml.parseXml as any).mockReturnValue({ + file: { + path: "test.ts", + diff: [ + { content: "<<<<<<< SEARCH\ntest\n=======\nreplaced\n>>>>>>> REPLACE" }, + { content: null }, + { content: undefined }, + { content: 42 }, + { content: "" }, // Empty string should also be filtered + { content: "<<<<<<< SEARCH\nmore\n=======\nchanges\n>>>>>>> REPLACE" }, + ], + }, + }) + + await applyDiffTool( + mockCline, + mockBlock, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should complete without error and only process valid string content + expect(mockPushToolResult).toHaveBeenCalled() + expect(mockHandleError).not.toHaveBeenCalled() + + // Verify that only valid diffs were processed + const resultCall = mockPushToolResult.mock.calls[0][0] + // Should not include the single block notice since we have 2 valid blocks + expect(resultCall).not.toContain("Making multiple related changes") + }) + }) + describe("SEARCH block counting with non-string content", () => { it("should handle diffItem.content being undefined", async () => { mockBlock = { diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index af6e196b51..a6aa99096c 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -132,13 +132,17 @@ export async function applyDiffTool( let diffContent: string let startLine: number | undefined - diffContent = diff.content + // Ensure content is a string before storing it + diffContent = typeof diff.content === "string" ? diff.content : "" startLine = diff.start_line ? parseInt(diff.start_line) : undefined - operationsMap[filePath].diff.push({ - content: diffContent, - startLine, - }) + // Only add to operations if we have valid content + if (diffContent) { + operationsMap[filePath].diff.push({ + content: diffContent, + startLine, + }) + } } } } catch (error) { @@ -656,11 +660,9 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} let totalSearchBlocks = 0 for (const operation of operations) { for (const diffItem of operation.diff) { - // Ensure content is a string before calling match - if (typeof diffItem.content === "string") { - const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length - totalSearchBlocks += searchBlocks - } + // Content is guaranteed to be a string due to early validation + const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length + totalSearchBlocks += searchBlocks } } From 418ebbeb13164f5ed1953e1e9f17a174daf49f57 Mon Sep 17 00:00:00 2001 From: Daniel <57051444+daniel-lxs@users.noreply.github.com> Date: Thu, 14 Aug 2025 17:37:15 -0500 Subject: [PATCH 3/5] chore: remove comments --- src/core/tools/multiApplyDiffTool.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/tools/multiApplyDiffTool.ts b/src/core/tools/multiApplyDiffTool.ts index a6aa99096c..5461230ac3 100644 --- a/src/core/tools/multiApplyDiffTool.ts +++ b/src/core/tools/multiApplyDiffTool.ts @@ -660,7 +660,6 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""} let totalSearchBlocks = 0 for (const operation of operations) { for (const diffItem of operation.diff) { - // Content is guaranteed to be a string due to early validation const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length totalSearchBlocks += searchBlocks } From d17fecb06f93d14d1d3a5af2abbc26f594496961 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 14 Aug 2025 17:55:59 -0500 Subject: [PATCH 4/5] fix: add TelemetryService mock to multiApplyDiffTool tests --- src/core/tools/__tests__/multiApplyDiffTool.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts index b7a07bfa4e..080d9531c2 100644 --- a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts +++ b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts @@ -13,6 +13,18 @@ vi.mock("../applyDiffTool", () => ({ applyDiffToolLegacy: vi.fn(), })) +// Mock TelemetryService +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + get instance() { + return { + trackEvent: vi.fn(), + trackError: vi.fn(), + } + }, + }, +})) + describe("multiApplyDiffTool", () => { let mockCline: any let mockBlock: any From 55f0ab13fc8b9fcbfc27aa9fe087dd6a6d043ad9 Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Thu, 14 Aug 2025 18:07:14 -0500 Subject: [PATCH 5/5] fix: add captureDiffApplicationError to TelemetryService mock --- src/core/tools/__tests__/multiApplyDiffTool.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts index 080d9531c2..0bdedb9cd5 100644 --- a/src/core/tools/__tests__/multiApplyDiffTool.spec.ts +++ b/src/core/tools/__tests__/multiApplyDiffTool.spec.ts @@ -20,6 +20,7 @@ vi.mock("@roo-code/telemetry", () => ({ return { trackEvent: vi.fn(), trackError: vi.fn(), + captureDiffApplicationError: vi.fn(), } }, },