Skip to content

Commit 18882b2

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

File tree

2 files changed

+62
-10
lines changed

2 files changed

+62
-10
lines changed

src/core/tools/__tests__/multiApplyDiffTool.spec.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,56 @@ describe("multiApplyDiffTool", () => {
9191
;(pathUtils.getReadablePath as any).mockImplementation((cwd: string, path: string) => path)
9292
})
9393

94+
describe("Early content validation", () => {
95+
it("should filter out non-string content at parse time", async () => {
96+
mockBlock = {
97+
params: {
98+
args: `<file>
99+
<path>test.ts</path>
100+
<diff>
101+
<content>valid string content</content>
102+
</diff>
103+
</file>`,
104+
},
105+
partial: false,
106+
}
107+
108+
// Mock parseXml to return mixed content types
109+
const parseXml = await import("../../../utils/xml")
110+
;(parseXml.parseXml as any).mockReturnValue({
111+
file: {
112+
path: "test.ts",
113+
diff: [
114+
{ content: "<<<<<<< SEARCH\ntest\n=======\nreplaced\n>>>>>>> REPLACE" },
115+
{ content: null },
116+
{ content: undefined },
117+
{ content: 42 },
118+
{ content: "" }, // Empty string should also be filtered
119+
{ content: "<<<<<<< SEARCH\nmore\n=======\nchanges\n>>>>>>> REPLACE" },
120+
],
121+
},
122+
})
123+
124+
await applyDiffTool(
125+
mockCline,
126+
mockBlock,
127+
mockAskApproval,
128+
mockHandleError,
129+
mockPushToolResult,
130+
mockRemoveClosingTag,
131+
)
132+
133+
// Should complete without error and only process valid string content
134+
expect(mockPushToolResult).toHaveBeenCalled()
135+
expect(mockHandleError).not.toHaveBeenCalled()
136+
137+
// Verify that only valid diffs were processed
138+
const resultCall = mockPushToolResult.mock.calls[0][0]
139+
// Should not include the single block notice since we have 2 valid blocks
140+
expect(resultCall).not.toContain("Making multiple related changes")
141+
})
142+
})
143+
94144
describe("SEARCH block counting with non-string content", () => {
95145
it("should handle diffItem.content being undefined", async () => {
96146
mockBlock = {

src/core/tools/multiApplyDiffTool.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,17 @@ export async function applyDiffTool(
132132
let diffContent: string
133133
let startLine: number | undefined
134134

135-
diffContent = diff.content
135+
// Ensure content is a string before storing it
136+
diffContent = typeof diff.content === "string" ? diff.content : ""
136137
startLine = diff.start_line ? parseInt(diff.start_line) : undefined
137138

138-
operationsMap[filePath].diff.push({
139-
content: diffContent,
140-
startLine,
141-
})
139+
// Only add to operations if we have valid content
140+
if (diffContent) {
141+
operationsMap[filePath].diff.push({
142+
content: diffContent,
143+
startLine,
144+
})
145+
}
142146
}
143147
}
144148
} catch (error) {
@@ -656,11 +660,9 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
656660
let totalSearchBlocks = 0
657661
for (const operation of operations) {
658662
for (const diffItem of operation.diff) {
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-
}
663+
// Content is guaranteed to be a string due to early validation
664+
const searchBlocks = (diffItem.content.match(/<<<<<<< SEARCH/g) || []).length
665+
totalSearchBlocks += searchBlocks
664666
}
665667
}
666668

0 commit comments

Comments
 (0)