Skip to content

Commit 8805f7b

Browse files
Ruakijdaniel-lxs
authored andcommitted
Fix #3652: Allow insertContentTool to create new files with content
1 parent 08a0c89 commit 8805f7b

File tree

3 files changed

+264
-23
lines changed

3 files changed

+264
-23
lines changed

src/core/prompts/sections/rules.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ function getEditingInstructions(diffStrategy?: DiffStrategy): string {
1515
availableTools.push("write_to_file (for creating new files or complete file rewrites)")
1616
}
1717

18-
availableTools.push("insert_content (for adding lines to existing files)")
18+
availableTools.push("insert_content (for adding lines to files)")
1919
availableTools.push("search_and_replace (for finding and replacing individual pieces of text)")
2020

2121
// Base editing instruction mentioning all available tools
Lines changed: 243 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,243 @@
1+
import * as path from "path"
2+
import * as fs from "fs/promises"
3+
4+
import { fileExistsAtPath } from "../../../utils/fs"
5+
import { ToolUse, ToolResponse } from "../../../shared/tools"
6+
import { insertContentTool } from "../insertContentTool"
7+
8+
// Mock external dependencies
9+
jest.mock("path", () => {
10+
const originalPath = jest.requireActual("path")
11+
return {
12+
...originalPath,
13+
resolve: jest.fn().mockImplementation((...args) => args.join("/")),
14+
}
15+
})
16+
17+
jest.mock("fs/promises", () => ({
18+
readFile: jest.fn(),
19+
writeFile: jest.fn(),
20+
}))
21+
22+
jest.mock("delay", () => jest.fn())
23+
24+
jest.mock("../../../utils/fs", () => ({
25+
fileExistsAtPath: jest.fn().mockResolvedValue(false),
26+
}))
27+
28+
jest.mock("../../prompts/responses", () => ({
29+
formatResponse: {
30+
toolError: jest.fn((msg) => `Error: ${msg}`),
31+
rooIgnoreError: jest.fn((path) => `Access denied: ${path}`),
32+
createPrettyPatch: jest.fn((_path, original, updated) => `Diff: ${original} -> ${updated}`),
33+
},
34+
}))
35+
36+
jest.mock("../../../utils/path", () => ({
37+
getReadablePath: jest.fn().mockReturnValue("test/path.txt"),
38+
}))
39+
40+
jest.mock("../../ignore/RooIgnoreController", () => ({
41+
RooIgnoreController: class {
42+
initialize() {
43+
return Promise.resolve()
44+
}
45+
validateAccess() {
46+
return true
47+
}
48+
},
49+
}))
50+
51+
// Mock insertGroups from diff/insert-groups
52+
jest.mock("../../diff/insert-groups", () => ({
53+
insertGroups: jest.fn().mockImplementation((lines, groups) => {
54+
let newLines = [...lines]
55+
for (const group of groups) {
56+
const { index, elements } = group
57+
if (index === -1 || index >= newLines.length) {
58+
// Append to end
59+
newLines.push(...elements)
60+
} else if (index < 0) {
61+
// Insert at beginning (index -1 for line 0, but insertGroups expects 0 for beginning)
62+
// This mock simplifies, assuming index -1 is always append.
63+
// For line 1, index is 0.
64+
newLines.splice(0, 0, ...elements)
65+
} else {
66+
newLines.splice(index, 0, ...elements)
67+
}
68+
}
69+
return newLines
70+
}),
71+
}))
72+
73+
describe("insertContentTool", () => {
74+
const testFilePath = "test/file.txt"
75+
const absoluteFilePath = "/test/file.txt"
76+
77+
const mockedFileExistsAtPath = fileExistsAtPath as jest.MockedFunction<typeof fileExistsAtPath>
78+
const mockedFsReadFile = fs.readFile as jest.MockedFunction<typeof fs.readFile>
79+
const mockedPathResolve = path.resolve as jest.MockedFunction<typeof path.resolve>
80+
const mockedInsertGroups = require("../../diff/insert-groups").insertGroups as jest.MockedFunction<any>
81+
82+
let mockCline: any
83+
let mockAskApproval: jest.Mock
84+
let mockHandleError: jest.Mock
85+
let mockPushToolResult: jest.Mock
86+
let mockRemoveClosingTag: jest.Mock
87+
let toolResult: ToolResponse | undefined
88+
89+
beforeEach(() => {
90+
jest.clearAllMocks()
91+
92+
mockedPathResolve.mockReturnValue(absoluteFilePath)
93+
mockedFileExistsAtPath.mockResolvedValue(true) // Assume file exists by default for insert
94+
mockedFsReadFile.mockResolvedValue("") // Default empty file content
95+
96+
mockCline = {
97+
cwd: "/",
98+
consecutiveMistakeCount: 0,
99+
didEditFile: false,
100+
rooIgnoreController: {
101+
validateAccess: jest.fn().mockReturnValue(true),
102+
},
103+
diffViewProvider: {
104+
editType: undefined,
105+
isEditing: false,
106+
originalContent: "",
107+
open: jest.fn().mockResolvedValue(undefined),
108+
update: jest.fn().mockResolvedValue(undefined),
109+
reset: jest.fn().mockResolvedValue(undefined),
110+
revertChanges: jest.fn().mockResolvedValue(undefined),
111+
saveChanges: jest.fn().mockResolvedValue({
112+
newProblemsMessage: "",
113+
userEdits: null,
114+
finalContent: "final content",
115+
}),
116+
scrollToFirstDiff: jest.fn(),
117+
pushToolWriteResult: jest.fn().mockImplementation(async function (
118+
this: any,
119+
task: any,
120+
cwd: string,
121+
isNewFile: boolean,
122+
) {
123+
return "Tool result message"
124+
}),
125+
},
126+
fileContextTracker: {
127+
trackFileContext: jest.fn().mockResolvedValue(undefined),
128+
},
129+
say: jest.fn().mockResolvedValue(undefined),
130+
ask: jest.fn().mockResolvedValue({ response: "yesButtonClicked" }), // Default to approval
131+
recordToolError: jest.fn(),
132+
sayAndCreateMissingParamError: jest.fn().mockResolvedValue("Missing param error"),
133+
}
134+
135+
mockAskApproval = jest.fn().mockResolvedValue(true)
136+
mockHandleError = jest.fn().mockResolvedValue(undefined)
137+
mockRemoveClosingTag = jest.fn((tag, content) => content)
138+
139+
toolResult = undefined
140+
})
141+
142+
async function executeInsertContentTool(
143+
params: Partial<ToolUse["params"]> = {},
144+
options: {
145+
fileExists?: boolean
146+
isPartial?: boolean
147+
accessAllowed?: boolean
148+
fileContent?: string
149+
askApprovalResponse?: "yesButtonClicked" | "noButtonClicked" | string
150+
} = {},
151+
): Promise<ToolResponse | undefined> {
152+
const fileExists = options.fileExists ?? true
153+
const isPartial = options.isPartial ?? false
154+
const accessAllowed = options.accessAllowed ?? true
155+
const fileContent = options.fileContent ?? ""
156+
157+
mockedFileExistsAtPath.mockResolvedValue(fileExists)
158+
mockedFsReadFile.mockResolvedValue(fileContent)
159+
mockCline.rooIgnoreController.validateAccess.mockReturnValue(accessAllowed)
160+
mockCline.ask.mockResolvedValue({ response: options.askApprovalResponse ?? "yesButtonClicked" })
161+
162+
const toolUse: ToolUse = {
163+
type: "tool_use",
164+
name: "insert_content",
165+
params: {
166+
path: testFilePath,
167+
line: "1",
168+
content: "New content",
169+
...params,
170+
},
171+
partial: isPartial,
172+
}
173+
174+
await insertContentTool(
175+
mockCline,
176+
toolUse,
177+
mockAskApproval,
178+
mockHandleError,
179+
(result: ToolResponse) => {
180+
toolResult = result
181+
},
182+
mockRemoveClosingTag,
183+
)
184+
185+
return toolResult
186+
}
187+
188+
describe("new file creation logic", () => {
189+
it("creates a new file and inserts content at line 0 (append)", async () => {
190+
const contentToInsert = "New Line 1\nNew Line 2"
191+
await executeInsertContentTool(
192+
{ line: "0", content: contentToInsert },
193+
{ fileExists: false, fileContent: "" },
194+
)
195+
196+
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
197+
expect(mockedFsReadFile).not.toHaveBeenCalled() // Should not read if file doesn't exist
198+
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true)
199+
expect(mockCline.diffViewProvider.editType).toBe("create")
200+
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
201+
})
202+
203+
it("creates a new file and inserts content at line 1 (beginning)", async () => {
204+
const contentToInsert = "Hello World!"
205+
await executeInsertContentTool(
206+
{ line: "1", content: contentToInsert },
207+
{ fileExists: false, fileContent: "" },
208+
)
209+
210+
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
211+
expect(mockedFsReadFile).not.toHaveBeenCalled()
212+
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith(contentToInsert, true)
213+
expect(mockCline.diffViewProvider.editType).toBe("create")
214+
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
215+
})
216+
217+
it("creates an empty new file if content is empty string", async () => {
218+
await executeInsertContentTool({ line: "1", content: "" }, { fileExists: false, fileContent: "" })
219+
220+
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
221+
expect(mockedFsReadFile).not.toHaveBeenCalled()
222+
expect(mockCline.diffViewProvider.update).toHaveBeenCalledWith("", true)
223+
expect(mockCline.diffViewProvider.editType).toBe("create")
224+
expect(mockCline.diffViewProvider.pushToolWriteResult).toHaveBeenCalledWith(mockCline, mockCline.cwd, true)
225+
})
226+
227+
it("returns an error when inserting content at an arbitrary line number into a new file", async () => {
228+
const contentToInsert = "Arbitrary insert"
229+
const result = await executeInsertContentTool(
230+
{ line: "5", content: contentToInsert },
231+
{ fileExists: false, fileContent: "" },
232+
)
233+
234+
expect(mockedFileExistsAtPath).toHaveBeenCalledWith(absoluteFilePath)
235+
expect(mockedFsReadFile).not.toHaveBeenCalled()
236+
expect(mockCline.consecutiveMistakeCount).toBe(1)
237+
expect(mockCline.recordToolError).toHaveBeenCalledWith("insert_content")
238+
expect(mockCline.say).toHaveBeenCalledWith("error", expect.stringContaining("non-existent file"))
239+
expect(mockCline.diffViewProvider.update).not.toHaveBeenCalled()
240+
expect(mockCline.diffViewProvider.pushToolWriteResult).not.toHaveBeenCalled()
241+
})
242+
})
243+
})

src/core/tools/insertContentTool.ts

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export async function insertContentTool(
5151
return
5252
}
5353

54-
if (!content) {
54+
if (content === undefined) {
5555
cline.consecutiveMistakeCount++
5656
cline.recordToolError("insert_content")
5757
pushToolResult(await cline.sayAndCreateMissingParamError("insert_content", "content"))
@@ -70,17 +70,6 @@ export async function insertContentTool(
7070
const isWriteProtected = cline.rooProtectedController?.isWriteProtected(relPath) || false
7171

7272
const absolutePath = path.resolve(cline.cwd, relPath)
73-
const fileExists = await fileExistsAtPath(absolutePath)
74-
75-
if (!fileExists) {
76-
cline.consecutiveMistakeCount++
77-
cline.recordToolError("insert_content")
78-
const formattedError = `File does not exist at path: ${absolutePath}\n\n<error_details>\nThe specified file could not be found. Please verify the file path and try again.\n</error_details>`
79-
await cline.say("error", formattedError)
80-
pushToolResult(formattedError)
81-
return
82-
}
83-
8473
const lineNumber = parseInt(line, 10)
8574
if (isNaN(lineNumber) || lineNumber < 0) {
8675
cline.consecutiveMistakeCount++
@@ -89,13 +78,26 @@ export async function insertContentTool(
8978
return
9079
}
9180

81+
const fileExists = await fileExistsAtPath(absolutePath)
82+
let fileContent: string = ""
83+
if (!fileExists) {
84+
if (lineNumber > 1) {
85+
cline.consecutiveMistakeCount++
86+
cline.recordToolError("insert_content")
87+
const formattedError = `Cannot insert content at line ${lineNumber} into a non-existent file. For new files, 'line' must be 0 (to append) or 1 (to insert at the beginning).`
88+
await cline.say("error", formattedError)
89+
pushToolResult(formattedError)
90+
return
91+
}
92+
} else {
93+
fileContent = await fs.readFile(absolutePath, "utf8")
94+
}
95+
9296
cline.consecutiveMistakeCount = 0
9397

94-
// Read the file
95-
const fileContent = await fs.readFile(absolutePath, "utf8")
96-
cline.diffViewProvider.editType = "modify"
98+
cline.diffViewProvider.editType = fileExists ? "modify" : "create"
9799
cline.diffViewProvider.originalContent = fileContent
98-
const lines = fileContent.split("\n")
100+
const lines = fileExists ? fileContent.split("\n") : []
99101

100102
const updatedContent = insertGroups(lines, [
101103
{
@@ -116,7 +118,7 @@ export async function insertContentTool(
116118

117119
const diff = formatResponse.createPrettyPatch(relPath, fileContent, updatedContent)
118120

119-
if (!diff) {
121+
if (fileExists && !diff) {
120122
pushToolResult(`No changes needed for '${relPath}'`)
121123
return
122124
}
@@ -151,11 +153,7 @@ export async function insertContentTool(
151153
cline.didEditFile = true
152154

153155
// Get the formatted response message
154-
const message = await cline.diffViewProvider.pushToolWriteResult(
155-
cline,
156-
cline.cwd,
157-
false, // Always false for insert_content
158-
)
156+
const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists)
159157

160158
pushToolResult(message)
161159

0 commit comments

Comments
 (0)