Skip to content

Commit a82afcc

Browse files
committed
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
1 parent f53fd39 commit a82afcc

File tree

2 files changed

+343
-2
lines changed

2 files changed

+343
-2
lines changed
Lines changed: 338 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,338 @@
1+
import { applyDiffTool } from "../multiApplyDiffTool"
2+
import { EXPERIMENT_IDS } from "../../../shared/experiments"
3+
import * as fs from "fs/promises"
4+
import * as fileUtils from "../../../utils/fs"
5+
import * as pathUtils from "../../../utils/path"
6+
7+
// Mock dependencies
8+
vi.mock("fs/promises")
9+
vi.mock("../../../utils/fs")
10+
vi.mock("../../../utils/path")
11+
vi.mock("../../../utils/xml")
12+
vi.mock("../applyDiffTool", () => ({
13+
applyDiffToolLegacy: vi.fn(),
14+
}))
15+
16+
describe("multiApplyDiffTool", () => {
17+
let mockCline: any
18+
let mockBlock: any
19+
let mockAskApproval: any
20+
let mockHandleError: any
21+
let mockPushToolResult: any
22+
let mockRemoveClosingTag: any
23+
let mockProvider: any
24+
25+
beforeEach(() => {
26+
vi.clearAllMocks()
27+
28+
mockProvider = {
29+
getState: vi.fn().mockResolvedValue({
30+
experiments: {
31+
[EXPERIMENT_IDS.MULTI_FILE_APPLY_DIFF]: true,
32+
},
33+
diagnosticsEnabled: true,
34+
writeDelayMs: 0,
35+
}),
36+
}
37+
38+
mockCline = {
39+
providerRef: {
40+
deref: vi.fn().mockReturnValue(mockProvider),
41+
},
42+
cwd: "/test",
43+
taskId: "test-task",
44+
consecutiveMistakeCount: 0,
45+
consecutiveMistakeCountForApplyDiff: new Map(),
46+
recordToolError: vi.fn(),
47+
say: vi.fn().mockResolvedValue(undefined),
48+
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
49+
ask: vi.fn().mockResolvedValue({ response: "yesButtonClicked", text: "", images: [] }),
50+
diffStrategy: {
51+
applyDiff: vi.fn().mockResolvedValue({
52+
success: true,
53+
content: "modified content",
54+
}),
55+
getProgressStatus: vi.fn(),
56+
},
57+
diffViewProvider: {
58+
reset: vi.fn().mockResolvedValue(undefined),
59+
editType: undefined,
60+
originalContent: undefined,
61+
open: vi.fn().mockResolvedValue(undefined),
62+
update: vi.fn().mockResolvedValue(undefined),
63+
scrollToFirstDiff: vi.fn(),
64+
saveDirectly: vi.fn().mockResolvedValue(undefined),
65+
saveChanges: vi.fn().mockResolvedValue(undefined),
66+
pushToolWriteResult: vi.fn().mockResolvedValue("File modified successfully"),
67+
},
68+
api: {
69+
getModel: vi.fn().mockReturnValue({ id: "test-model" }),
70+
},
71+
rooIgnoreController: {
72+
validateAccess: vi.fn().mockReturnValue(true),
73+
},
74+
rooProtectedController: {
75+
isWriteProtected: vi.fn().mockReturnValue(false),
76+
},
77+
fileContextTracker: {
78+
trackFileContext: vi.fn().mockResolvedValue(undefined),
79+
},
80+
didEditFile: false,
81+
} as any
82+
83+
mockAskApproval = vi.fn().mockResolvedValue(true)
84+
mockHandleError = vi.fn()
85+
mockPushToolResult = vi.fn()
86+
mockRemoveClosingTag = vi.fn((tag, value) => value)
87+
88+
// Mock file system operations
89+
;(fileUtils.fileExistsAtPath as any).mockResolvedValue(true)
90+
;(fs.readFile as any).mockResolvedValue("original content")
91+
;(pathUtils.getReadablePath as any).mockImplementation((cwd: string, path: string) => path)
92+
})
93+
94+
describe("SEARCH block counting with non-string content", () => {
95+
it("should handle diffItem.content being undefined", async () => {
96+
mockBlock = {
97+
params: {
98+
path: "test.ts",
99+
diff: undefined, // This will result in undefined content
100+
},
101+
partial: false,
102+
}
103+
104+
await applyDiffTool(
105+
mockCline,
106+
mockBlock,
107+
mockAskApproval,
108+
mockHandleError,
109+
mockPushToolResult,
110+
mockRemoveClosingTag,
111+
)
112+
113+
// Should complete without throwing an error
114+
expect(mockPushToolResult).toHaveBeenCalled()
115+
expect(mockHandleError).not.toHaveBeenCalled()
116+
})
117+
118+
it("should handle diffItem.content being null", async () => {
119+
mockBlock = {
120+
params: {
121+
args: `<file>
122+
<path>test.ts</path>
123+
<diff>
124+
<content></content>
125+
</diff>
126+
</file>`,
127+
},
128+
partial: false,
129+
}
130+
131+
// Mock parseXml to return null content
132+
const parseXml = await import("../../../utils/xml")
133+
;(parseXml.parseXml as any).mockReturnValue({
134+
file: {
135+
path: "test.ts",
136+
diff: {
137+
content: null,
138+
},
139+
},
140+
})
141+
142+
await applyDiffTool(
143+
mockCline,
144+
mockBlock,
145+
mockAskApproval,
146+
mockHandleError,
147+
mockPushToolResult,
148+
mockRemoveClosingTag,
149+
)
150+
151+
// Should complete without throwing an error
152+
expect(mockPushToolResult).toHaveBeenCalled()
153+
expect(mockHandleError).not.toHaveBeenCalled()
154+
})
155+
156+
it("should handle diffItem.content being a number", async () => {
157+
mockBlock = {
158+
params: {
159+
args: `<file>
160+
<path>test.ts</path>
161+
<diff>
162+
<content>123</content>
163+
</diff>
164+
</file>`,
165+
},
166+
partial: false,
167+
}
168+
169+
// Mock parseXml to return number content
170+
const parseXml = await import("../../../utils/xml")
171+
;(parseXml.parseXml as any).mockReturnValue({
172+
file: {
173+
path: "test.ts",
174+
diff: {
175+
content: 123, // Number instead of string
176+
},
177+
},
178+
})
179+
180+
await applyDiffTool(
181+
mockCline,
182+
mockBlock,
183+
mockAskApproval,
184+
mockHandleError,
185+
mockPushToolResult,
186+
mockRemoveClosingTag,
187+
)
188+
189+
// Should complete without throwing an error
190+
expect(mockPushToolResult).toHaveBeenCalled()
191+
expect(mockHandleError).not.toHaveBeenCalled()
192+
})
193+
194+
it("should correctly count SEARCH blocks when content is a valid string", async () => {
195+
const diffContent = `<<<<<<< SEARCH
196+
old content
197+
=======
198+
new content
199+
>>>>>>> REPLACE
200+
201+
<<<<<<< SEARCH
202+
another old content
203+
=======
204+
another new content
205+
>>>>>>> REPLACE`
206+
207+
mockBlock = {
208+
params: {
209+
path: "test.ts",
210+
diff: diffContent,
211+
},
212+
partial: false,
213+
}
214+
215+
await applyDiffTool(
216+
mockCline,
217+
mockBlock,
218+
mockAskApproval,
219+
mockHandleError,
220+
mockPushToolResult,
221+
mockRemoveClosingTag,
222+
)
223+
224+
// Should complete successfully
225+
expect(mockPushToolResult).toHaveBeenCalled()
226+
const resultCall = mockPushToolResult.mock.calls[0][0]
227+
// Should not include the single block notice since we have 2 blocks
228+
expect(resultCall).not.toContain("Making multiple related changes")
229+
})
230+
231+
it("should show single block notice when only one SEARCH block exists", async () => {
232+
const diffContent = `<<<<<<< SEARCH
233+
old content
234+
=======
235+
new content
236+
>>>>>>> REPLACE`
237+
238+
mockBlock = {
239+
params: {
240+
path: "test.ts",
241+
diff: diffContent,
242+
},
243+
partial: false,
244+
}
245+
246+
await applyDiffTool(
247+
mockCline,
248+
mockBlock,
249+
mockAskApproval,
250+
mockHandleError,
251+
mockPushToolResult,
252+
mockRemoveClosingTag,
253+
)
254+
255+
// Should complete successfully
256+
expect(mockPushToolResult).toHaveBeenCalled()
257+
const resultCall = mockPushToolResult.mock.calls[0][0]
258+
// Should include the single block notice
259+
expect(resultCall).toContain("Making multiple related changes")
260+
})
261+
})
262+
263+
describe("Edge cases for diff content", () => {
264+
it("should handle empty diff array", async () => {
265+
mockBlock = {
266+
params: {
267+
args: `<file>
268+
<path>test.ts</path>
269+
<diff></diff>
270+
</file>`,
271+
},
272+
partial: false,
273+
}
274+
275+
const parseXml = await import("../../../utils/xml")
276+
;(parseXml.parseXml as any).mockReturnValue({
277+
file: {
278+
path: "test.ts",
279+
diff: [],
280+
},
281+
})
282+
283+
await applyDiffTool(
284+
mockCline,
285+
mockBlock,
286+
mockAskApproval,
287+
mockHandleError,
288+
mockPushToolResult,
289+
mockRemoveClosingTag,
290+
)
291+
292+
// Should complete without error
293+
expect(mockPushToolResult).toHaveBeenCalled()
294+
expect(mockHandleError).not.toHaveBeenCalled()
295+
})
296+
297+
it("should handle mixed content types in diff array", async () => {
298+
mockBlock = {
299+
params: {
300+
args: `<file>
301+
<path>test.ts</path>
302+
<diff>
303+
<content>valid string content</content>
304+
</diff>
305+
</file>`,
306+
},
307+
partial: false,
308+
}
309+
310+
const parseXml = await import("../../../utils/xml")
311+
;(parseXml.parseXml as any).mockReturnValue({
312+
file: {
313+
path: "test.ts",
314+
diff: [
315+
{ content: "<<<<<<< SEARCH\ntest\n=======\nreplaced\n>>>>>>> REPLACE" },
316+
{ content: null },
317+
{ content: undefined },
318+
{ content: 42 },
319+
{ content: "<<<<<<< SEARCH\nmore\n=======\nchanges\n>>>>>>> REPLACE" },
320+
],
321+
},
322+
})
323+
324+
await applyDiffTool(
325+
mockCline,
326+
mockBlock,
327+
mockAskApproval,
328+
mockHandleError,
329+
mockPushToolResult,
330+
mockRemoveClosingTag,
331+
)
332+
333+
// Should complete without error and count only valid string SEARCH blocks
334+
expect(mockPushToolResult).toHaveBeenCalled()
335+
expect(mockHandleError).not.toHaveBeenCalled()
336+
})
337+
})
338+
})

src/core/tools/multiApplyDiffTool.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -656,8 +656,11 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
656656
let totalSearchBlocks = 0
657657
for (const operation of operations) {
658658
for (const diffItem of operation.diff) {
659-
const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length
660-
totalSearchBlocks += searchBlocks
659+
// Ensure content is a string before calling match
660+
if (typeof diffItem.content === "string") {
661+
const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length
662+
totalSearchBlocks += searchBlocks
663+
}
661664
}
662665
}
663666

0 commit comments

Comments
 (0)