mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
fix: make edit_file matching more resilient (#10585)
This commit is contained in:
Hannes Rudolph
GitHub
parent
168cfcaba5
commit
e39abbffa1
4 changed files with 456 additions and 87 deletions
|
|
@ -4,6 +4,8 @@ const EDIT_FILE_DESCRIPTION = `Use this tool to replace text in an existing file
|
|||
|
||||
This tool performs literal string replacement with support for multiple occurrences.
|
||||
|
||||
To be resilient to minor formatting drift, the tool normalizes line endings (CRLF/LF) for matching and may fall back to deterministic matching strategies when an exact literal match fails (exact → whitespace-tolerant match → token-based match). The original file's line endings are preserved when writing.
|
||||
|
||||
USAGE PATTERNS:
|
||||
|
||||
1. MODIFY EXISTING FILE (default):
|
||||
|
|
@ -18,10 +20,10 @@ USAGE PATTERNS:
|
|||
|
||||
CRITICAL REQUIREMENTS:
|
||||
|
||||
1. EXACT MATCHING: The old_string must match the file contents EXACTLY, including:
|
||||
- All whitespace (spaces, tabs, newlines)
|
||||
- All indentation
|
||||
- All punctuation and special characters
|
||||
1. EXACT MATCHING (BEST): The old_string should match the file contents EXACTLY, including:
|
||||
- All whitespace (spaces, tabs, newlines)
|
||||
- All indentation
|
||||
- All punctuation and special characters
|
||||
|
||||
2. CONTEXT FOR UNIQUENESS: For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text to ensure uniqueness.
|
||||
|
||||
|
|
|
|||
|
|
@ -354,6 +354,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
consecutiveMistakeCount: number = 0
|
||||
consecutiveMistakeLimit: number
|
||||
consecutiveMistakeCountForApplyDiff: Map<string, number> = new Map()
|
||||
consecutiveMistakeCountForEditFile: Map<string, number> = new Map()
|
||||
consecutiveNoToolUseCount: number = 0
|
||||
consecutiveNoAssistantMessagesCount: number = 0
|
||||
toolUsage: ToolUsage = {}
|
||||
|
|
|
|||
|
|
@ -22,6 +22,8 @@ interface EditFileParams {
|
|||
expected_replacements?: number
|
||||
}
|
||||
|
||||
type LineEnding = "\r\n" | "\n"
|
||||
|
||||
/**
|
||||
* Count occurrences of a substring in a string.
|
||||
* @param str The string to search in
|
||||
|
|
@ -65,35 +67,75 @@ function safeLiteralReplace(str: string, oldString: string, newString: string):
|
|||
return str.replaceAll(oldString, escapedNewString)
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply a replacement operation.
|
||||
*
|
||||
* @param currentContent The current file content (null if file doesn't exist)
|
||||
* @param oldString The string to replace
|
||||
* @param newString The replacement string
|
||||
* @param isNewFile Whether this is creating a new file
|
||||
* @returns The resulting content
|
||||
*/
|
||||
function applyReplacement(
|
||||
currentContent: string | null,
|
||||
oldString: string,
|
||||
newString: string,
|
||||
isNewFile: boolean,
|
||||
): string {
|
||||
if (isNewFile) {
|
||||
return newString
|
||||
}
|
||||
// If oldString is empty and it's not a new file, do not modify the content
|
||||
if (oldString === "" || currentContent === null) {
|
||||
return currentContent ?? ""
|
||||
function detectLineEnding(content: string): LineEnding {
|
||||
return content.includes("\r\n") ? "\r\n" : "\n"
|
||||
}
|
||||
|
||||
function normalizeToLF(content: string): string {
|
||||
return content.replace(/\r\n/g, "\n")
|
||||
}
|
||||
|
||||
function restoreLineEnding(contentLF: string, eol: LineEnding): string {
|
||||
if (eol === "\n") return contentLF
|
||||
return contentLF.replace(/\n/g, "\r\n")
|
||||
}
|
||||
|
||||
function escapeRegExp(input: string): string {
|
||||
return input.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")
|
||||
}
|
||||
|
||||
function buildWhitespaceTolerantRegex(oldLF: string): RegExp {
|
||||
if (oldLF === "") {
|
||||
// Never match empty string
|
||||
return new RegExp("(?!)", "g")
|
||||
}
|
||||
|
||||
return safeLiteralReplace(currentContent, oldString, newString)
|
||||
const parts = oldLF.match(/(\s+|\S+)/g) ?? []
|
||||
const whitespacePatternForRun = (run: string): string => {
|
||||
// If the whitespace run includes a newline, allow matching any whitespace (including newlines)
|
||||
// to tolerate wrapping changes across lines.
|
||||
if (run.includes("\n")) {
|
||||
return "\\s+"
|
||||
}
|
||||
|
||||
// Otherwise, limit matching to horizontal whitespace so we don't accidentally consume
|
||||
// line breaks that precede indentation.
|
||||
return "[\\t ]+"
|
||||
}
|
||||
|
||||
const pattern = parts
|
||||
.map((part) => {
|
||||
if (/^\s+$/.test(part)) {
|
||||
return whitespacePatternForRun(part)
|
||||
}
|
||||
return escapeRegExp(part)
|
||||
})
|
||||
.join("")
|
||||
|
||||
return new RegExp(pattern, "g")
|
||||
}
|
||||
|
||||
function buildTokenRegex(oldLF: string): RegExp {
|
||||
const tokens = oldLF.split(/\s+/).filter(Boolean)
|
||||
if (tokens.length === 0) {
|
||||
return new RegExp("(?!)", "g")
|
||||
}
|
||||
|
||||
const pattern = tokens.map(escapeRegExp).join("\\s+")
|
||||
return new RegExp(pattern, "g")
|
||||
}
|
||||
|
||||
function countRegexMatches(content: string, regex: RegExp): number {
|
||||
const stable = new RegExp(regex.source, regex.flags)
|
||||
return Array.from(content.matchAll(stable)).length
|
||||
}
|
||||
|
||||
export class EditFileTool extends BaseTool<"edit_file"> {
|
||||
readonly name = "edit_file" as const
|
||||
|
||||
private didSendPartialToolAsk = false
|
||||
private partialToolAskRelPath: string | undefined
|
||||
|
||||
parseLegacy(params: Partial<Record<string, string>>): EditFileParams {
|
||||
return {
|
||||
file_path: params.file_path || "",
|
||||
|
|
@ -106,14 +148,54 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
}
|
||||
|
||||
async execute(params: EditFileParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
|
||||
const { file_path, old_string, new_string, expected_replacements = 1 } = params
|
||||
// Coerce old_string/new_string to handle malformed native tool calls where they could be non-strings.
|
||||
// In native mode, malformed calls can pass numbers/objects; normalize those to "" to avoid later crashes.
|
||||
const file_path = params.file_path
|
||||
const old_string = typeof params.old_string === "string" ? params.old_string : ""
|
||||
const new_string = typeof params.new_string === "string" ? params.new_string : ""
|
||||
const expected_replacements = params.expected_replacements ?? 1
|
||||
const { askApproval, handleError, pushToolResult, toolProtocol } = callbacks
|
||||
let relPathForErrorHandling: string | undefined
|
||||
let operationPreviewForErrorHandling: string | undefined
|
||||
|
||||
const finalizePartialToolAskIfNeeded = async (relPath: string): Promise<void> => {
|
||||
if (!this.didSendPartialToolAsk) {
|
||||
return
|
||||
}
|
||||
|
||||
if (this.partialToolAskRelPath && this.partialToolAskRelPath !== relPath) {
|
||||
return
|
||||
}
|
||||
|
||||
const absolutePath = path.resolve(task.cwd, relPath)
|
||||
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
|
||||
|
||||
const sharedMessageProps: ClineSayTool = {
|
||||
tool: "appliedDiff",
|
||||
path: getReadablePath(task.cwd, relPath),
|
||||
diff: operationPreviewForErrorHandling,
|
||||
isOutsideWorkspace,
|
||||
}
|
||||
|
||||
// Finalize the existing partial tool ask row so the UI doesn't get stuck in a spinner state.
|
||||
await task.ask("tool", JSON.stringify(sharedMessageProps), false).catch(() => {})
|
||||
}
|
||||
|
||||
const recordFailureForPathAndMaybeEscalate = async (relPath: string, formattedError: string): Promise<void> => {
|
||||
const currentCount = (task.consecutiveMistakeCountForEditFile.get(relPath) || 0) + 1
|
||||
task.consecutiveMistakeCountForEditFile.set(relPath, currentCount)
|
||||
|
||||
if (currentCount >= 2) {
|
||||
await task.say("diff_error", formattedError)
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
// Validate required parameters
|
||||
if (!file_path) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file")
|
||||
task.didToolFailInCurrentTurn = true
|
||||
pushToolResult(await task.sayAndCreateMissingParamError("edit_file", "file_path"))
|
||||
return
|
||||
}
|
||||
|
|
@ -125,10 +207,22 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
} else {
|
||||
relPath = file_path
|
||||
}
|
||||
relPathForErrorHandling = relPath
|
||||
|
||||
operationPreviewForErrorHandling =
|
||||
old_string === ""
|
||||
? "creating new file"
|
||||
: (() => {
|
||||
const preview = old_string.length > 50 ? old_string.substring(0, 50) + "..." : old_string
|
||||
return `replacing: "${preview}"`
|
||||
})()
|
||||
|
||||
const accessAllowed = task.rooIgnoreController?.validateAccess(relPath)
|
||||
|
||||
if (!accessAllowed) {
|
||||
// Finalize the partial tool preview before emitting any say() messages.
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
task.didToolFailInCurrentTurn = true
|
||||
await task.say("rooignore_error", relPath)
|
||||
pushToolResult(formatResponse.rooIgnoreError(relPath, toolProtocol))
|
||||
return
|
||||
|
|
@ -141,30 +235,38 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
const fileExists = await fileExistsAtPath(absolutePath)
|
||||
|
||||
let currentContent: string | null = null
|
||||
let currentContentLF: string | null = null
|
||||
let originalEol: LineEnding = "\n"
|
||||
let isNewFile = false
|
||||
|
||||
// Read file or determine if creating new
|
||||
if (fileExists) {
|
||||
try {
|
||||
currentContent = await fs.readFile(absolutePath, "utf8")
|
||||
// Normalize line endings to LF
|
||||
currentContent = currentContent.replace(/\r\n/g, "\n")
|
||||
originalEol = detectLineEnding(currentContent)
|
||||
// Normalize line endings to LF for matching
|
||||
currentContentLF = normalizeToLF(currentContent)
|
||||
} catch (error) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file")
|
||||
const errorMessage = `Failed to read file '${relPath}'. Please verify file permissions and try again.`
|
||||
await task.say("error", errorMessage)
|
||||
pushToolResult(formatResponse.toolError(errorMessage, toolProtocol))
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const errorDetails = error instanceof Error ? error.message : String(error)
|
||||
const formattedError = `Failed to read file: ${absolutePath}\n\n<error_details>\nRead error: ${errorDetails}\n\nRecovery suggestions:\n1. Verify the file exists and is readable\n2. Check file permissions\n3. If the file may have changed, use read_file to confirm its current contents\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
|
||||
// Check if trying to create a file that already exists
|
||||
if (old_string === "") {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file")
|
||||
const errorMessage = `File '${relPath}' already exists. Cannot create a new file with empty old_string when file exists.`
|
||||
await task.say("error", errorMessage)
|
||||
pushToolResult(formatResponse.toolError(errorMessage, toolProtocol))
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `File already exists: ${absolutePath}\n\n<error_details>\nYou provided an empty old_string, which indicates file creation, but the target file already exists.\n\nRecovery suggestions:\n1. To modify an existing file, provide a non-empty old_string that matches the current file contents\n2. Use read_file to confirm the exact text to match\n3. If you intended to overwrite the entire file, use write_to_file instead\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
} else {
|
||||
|
|
@ -175,67 +277,111 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
} else {
|
||||
// Trying to replace in non-existent file
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file")
|
||||
const errorMessage = `File not found: ${relPath}. Cannot perform replacement on a non-existent file. Use an empty old_string to create a new file.`
|
||||
await task.say("error", errorMessage)
|
||||
pushToolResult(formatResponse.toolError(errorMessage, toolProtocol))
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `File does not exist at path: ${absolutePath}\n\n<error_details>\nThe specified file could not be found, so the replacement could not be performed.\n\nRecovery suggestions:\n1. Verify the file path is correct\n2. If you intended to create a new file, set old_string to an empty string\n3. Use list_files or read_file to confirm the correct path\n</error_details>`
|
||||
// Match apply_diff behavior: surface missing file via the generic error channel.
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await task.say("error", formattedError)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
const oldLF = normalizeToLF(old_string)
|
||||
const newLF = normalizeToLF(new_string)
|
||||
const expectedReplacements = Math.max(1, expected_replacements)
|
||||
|
||||
// Validate replacement operation
|
||||
if (!isNewFile && currentContent !== null) {
|
||||
// Check occurrence count
|
||||
const occurrences = countOccurrences(currentContent, old_string)
|
||||
|
||||
if (occurrences === 0) {
|
||||
if (!isNewFile && currentContentLF !== null) {
|
||||
// Validate that old_string and new_string are different (normalized for EOL)
|
||||
if (oldLF === newLF) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file", "no_match")
|
||||
pushToolResult(
|
||||
formatResponse.toolError(
|
||||
`No match found for the specified 'old_string'. Please ensure it matches the file contents exactly, including all whitespace and indentation.`,
|
||||
toolProtocol,
|
||||
),
|
||||
)
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `No changes to apply for file: ${absolutePath}\n\n<error_details>\nThe provided old_string and new_string are identical (after normalizing line endings), so there is nothing to change.\n\nRecovery suggestions:\n1. Update new_string to the intended replacement text\n2. If you intended to verify file state only, use read_file instead\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
|
||||
if (occurrences !== expected_replacements) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file", "occurrence_mismatch")
|
||||
pushToolResult(
|
||||
formatResponse.toolError(
|
||||
`Expected ${expected_replacements} occurrence(s) but found ${occurrences}. Please adjust your old_string to match exactly ${expected_replacements} occurrence(s), or set expected_replacements to ${occurrences}.`,
|
||||
toolProtocol,
|
||||
),
|
||||
)
|
||||
return
|
||||
}
|
||||
const wsRegex = buildWhitespaceTolerantRegex(oldLF)
|
||||
const tokenRegex = buildTokenRegex(oldLF)
|
||||
|
||||
// Validate that old_string and new_string are different
|
||||
if (old_string === new_string) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("edit_file")
|
||||
pushToolResult(
|
||||
formatResponse.toolError(
|
||||
"No changes to apply. The old_string and new_string are identical.",
|
||||
toolProtocol,
|
||||
),
|
||||
)
|
||||
return
|
||||
// Strategy 1: exact literal match
|
||||
const exactOccurrences = countOccurrences(currentContentLF, oldLF)
|
||||
if (exactOccurrences === expectedReplacements) {
|
||||
// Apply literal replacement on LF-normalized content
|
||||
currentContentLF = safeLiteralReplace(currentContentLF, oldLF, newLF)
|
||||
} else {
|
||||
// Strategy 2: whitespace-tolerant regex
|
||||
const wsOccurrences = countRegexMatches(currentContentLF, wsRegex)
|
||||
if (wsOccurrences === expectedReplacements) {
|
||||
currentContentLF = currentContentLF.replace(wsRegex, () => newLF)
|
||||
} else {
|
||||
// Strategy 3: token-based regex
|
||||
const tokenOccurrences = countRegexMatches(currentContentLF, tokenRegex)
|
||||
if (tokenOccurrences === expectedReplacements) {
|
||||
currentContentLF = currentContentLF.replace(tokenRegex, () => newLF)
|
||||
} else {
|
||||
// Error reporting
|
||||
const anyMatches = exactOccurrences > 0 || wsOccurrences > 0 || tokenOccurrences > 0
|
||||
if (!anyMatches) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `No match found in file: ${absolutePath}\n\n<error_details>\nThe provided old_string could not be found using exact, whitespace-tolerant, or token-based matching.\n\nRecovery suggestions:\n1. Use read_file to confirm the file's current contents\n2. Ensure old_string matches exactly (including whitespace/indentation and line endings)\n3. Provide more surrounding context in old_string to make the match unique\n4. If the file has changed since you constructed old_string, re-read and retry\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
|
||||
// If exact matching finds occurrences but doesn't match expected, keep the existing message
|
||||
if (exactOccurrences > 0) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `Occurrence count mismatch in file: ${absolutePath}\n\n<error_details>\nExpected ${expectedReplacements} occurrence(s) but found ${exactOccurrences} exact match(es).\n\nRecovery suggestions:\n1. Provide a more specific old_string so it matches exactly once\n2. If you intend to replace all occurrences, set expected_replacements to ${exactOccurrences}\n3. Use read_file to confirm the exact text and counts\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
|
||||
task.consecutiveMistakeCount++
|
||||
task.didToolFailInCurrentTurn = true
|
||||
const formattedError = `Occurrence count mismatch in file: ${absolutePath}\n\n<error_details>\nExpected ${expectedReplacements} occurrence(s), but matching found ${wsOccurrences} (whitespace-tolerant) and ${tokenOccurrences} (token-based).\n\nRecovery suggestions:\n1. Provide more surrounding context in old_string to make the match unique\n2. If multiple replacements are intended, adjust expected_replacements to the intended count\n3. Use read_file to confirm the current file contents and refine the match\n</error_details>`
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
await recordFailureForPathAndMaybeEscalate(relPath, formattedError)
|
||||
task.recordToolError("edit_file", formattedError)
|
||||
pushToolResult(formattedError)
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Apply the replacement
|
||||
const newContent = applyReplacement(currentContent, old_string, new_string, isNewFile)
|
||||
const newContent = isNewFile
|
||||
? new_string
|
||||
: restoreLineEnding(currentContentLF ?? currentContent ?? "", originalEol)
|
||||
|
||||
// Check if any changes were made
|
||||
if (!isNewFile && newContent === currentContent) {
|
||||
if (relPathForErrorHandling) {
|
||||
task.consecutiveMistakeCount = 0
|
||||
task.consecutiveMistakeCountForEditFile.delete(relPathForErrorHandling)
|
||||
}
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
pushToolResult(`No changes needed for '${relPath}'`)
|
||||
return
|
||||
}
|
||||
|
||||
task.consecutiveMistakeCount = 0
|
||||
task.consecutiveMistakeCountForEditFile.delete(relPath)
|
||||
|
||||
// Initialize diff view
|
||||
task.diffViewProvider.editType = isNewFile ? "create" : "modify"
|
||||
|
|
@ -244,6 +390,9 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
// Generate and validate diff
|
||||
const diff = formatResponse.createPrettyPatch(relPath, currentContent || "", newContent)
|
||||
if (!diff && !isNewFile) {
|
||||
task.consecutiveMistakeCount = 0
|
||||
task.consecutiveMistakeCountForEditFile.delete(relPath)
|
||||
await finalizePartialToolAskIfNeeded(relPath)
|
||||
pushToolResult(`No changes needed for '${relPath}'`)
|
||||
await task.diffViewProvider.reset()
|
||||
return
|
||||
|
|
@ -333,8 +482,15 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
// Process any queued messages after file edit completes
|
||||
task.processQueuedMessages()
|
||||
} catch (error) {
|
||||
if (relPathForErrorHandling) {
|
||||
await finalizePartialToolAskIfNeeded(relPathForErrorHandling)
|
||||
}
|
||||
await handleError("edit_file", error as Error)
|
||||
await task.diffViewProvider.reset()
|
||||
task.didToolFailInCurrentTurn = true
|
||||
} finally {
|
||||
this.didSendPartialToolAsk = false
|
||||
this.partialToolAskRelPath = undefined
|
||||
this.resetPartialState()
|
||||
}
|
||||
}
|
||||
|
|
@ -363,6 +519,8 @@ export class EditFileTool extends BaseTool<"edit_file"> {
|
|||
if (path.isAbsolute(relPath)) {
|
||||
relPath = path.relative(task.cwd, relPath)
|
||||
}
|
||||
this.didSendPartialToolAsk = true
|
||||
this.partialToolAskRelPath = relPath
|
||||
|
||||
const absolutePath = path.resolve(task.cwd, relPath)
|
||||
const isOutsideWorkspace = isPathOutsideWorkspace(absolutePath)
|
||||
|
|
|
|||
|
|
@ -106,7 +106,9 @@ describe("editFileTool", () => {
|
|||
|
||||
mockTask.cwd = "/"
|
||||
mockTask.consecutiveMistakeCount = 0
|
||||
mockTask.consecutiveMistakeCountForEditFile = new Map()
|
||||
mockTask.didEditFile = false
|
||||
mockTask.didToolFailInCurrentTurn = false
|
||||
mockTask.providerRef = {
|
||||
deref: vi.fn().mockReturnValue({
|
||||
getState: vi.fn().mockResolvedValue({
|
||||
|
|
@ -211,6 +213,7 @@ describe("editFileTool", () => {
|
|||
expect(result).toBe("Missing param error")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith("edit_file")
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
})
|
||||
|
||||
it("treats undefined new_string as empty string (deletion)", async () => {
|
||||
|
|
@ -237,8 +240,95 @@ describe("editFileTool", () => {
|
|||
new_string: "same",
|
||||
})
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("No changes to apply")
|
||||
expect(result).toContain("<error_details>")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
})
|
||||
|
||||
describe("native tool mode coercion", () => {
|
||||
/**
|
||||
* Helper to execute edit_file with native tool args (simulating native protocol)
|
||||
*/
|
||||
async function executeWithNativeArgs(
|
||||
nativeArgs: Record<string, unknown>,
|
||||
options: { fileExists?: boolean; fileContent?: string } = {},
|
||||
): Promise<ToolResponse | undefined> {
|
||||
const fileExists = options.fileExists ?? true
|
||||
const fileContent = options.fileContent ?? testFileContent
|
||||
|
||||
mockedFileExistsAtPath.mockResolvedValue(fileExists)
|
||||
mockedFsReadFile.mockResolvedValue(fileContent)
|
||||
mockTask.rooIgnoreController.validateAccess.mockReturnValue(true)
|
||||
|
||||
const toolUse: ToolUse = {
|
||||
type: "tool_use",
|
||||
name: "edit_file",
|
||||
params: {},
|
||||
partial: false,
|
||||
nativeArgs: nativeArgs as any,
|
||||
}
|
||||
|
||||
let capturedResult: ToolResponse | undefined
|
||||
const localPushToolResult = vi.fn((result: ToolResponse) => {
|
||||
capturedResult = result
|
||||
})
|
||||
|
||||
await editFileTool.handle(mockTask, toolUse as ToolUse<"edit_file">, {
|
||||
askApproval: mockAskApproval,
|
||||
handleError: mockHandleError,
|
||||
pushToolResult: localPushToolResult,
|
||||
removeClosingTag: mockRemoveClosingTag,
|
||||
toolProtocol: "native",
|
||||
})
|
||||
|
||||
return capturedResult
|
||||
}
|
||||
|
||||
it("coerces undefined old_string to empty string in native mode (file creation)", async () => {
|
||||
await executeWithNativeArgs(
|
||||
{ file_path: testFilePath, old_string: undefined, new_string: "New content" },
|
||||
{ fileExists: false },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockTask.diffViewProvider.editType).toBe("create")
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("coerces undefined new_string to empty string in native mode (deletion)", async () => {
|
||||
await executeWithNativeArgs(
|
||||
{ file_path: testFilePath, old_string: "Line 2", new_string: undefined },
|
||||
{ fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("handles both old_string and new_string as undefined in native mode", async () => {
|
||||
await executeWithNativeArgs(
|
||||
{ file_path: testFilePath, old_string: undefined, new_string: undefined },
|
||||
{ fileExists: false },
|
||||
)
|
||||
|
||||
// Both undefined means: old_string = "" (create file), new_string = "" (empty file)
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockTask.diffViewProvider.editType).toBe("create")
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("handles null values as strings in native mode", async () => {
|
||||
await executeWithNativeArgs(
|
||||
{ file_path: testFilePath, old_string: null, new_string: "New content" },
|
||||
{ fileExists: false },
|
||||
)
|
||||
|
||||
// null is coerced to "" via ?? operator
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockTask.diffViewProvider.editType).toBe("create")
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -246,9 +336,10 @@ describe("editFileTool", () => {
|
|||
it("returns error when file does not exist and old_string is not empty", async () => {
|
||||
const result = await executeEditFileTool({}, { fileExists: false })
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("File not found")
|
||||
expect(result).toContain("File does not exist")
|
||||
expect(result).toContain("<error_details>")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
})
|
||||
|
||||
it("returns error when access is denied", async () => {
|
||||
|
|
@ -265,10 +356,21 @@ describe("editFileTool", () => {
|
|||
{ fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("No match found")
|
||||
expect(result).toContain("<error_details>")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith("edit_file", "no_match")
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith(
|
||||
"edit_file",
|
||||
expect.stringContaining("No match found"),
|
||||
)
|
||||
})
|
||||
|
||||
it("emits diff_error on the 2nd consecutive failure for the same file", async () => {
|
||||
await executeEditFileTool({ old_string: "NonExistent" }, { fileContent: "Line 1\nLine 2\nLine 3" })
|
||||
await executeEditFileTool({ old_string: "NonExistent" }, { fileContent: "Line 1\nLine 2\nLine 3" })
|
||||
|
||||
expect(mockTask.say).toHaveBeenCalledWith("diff_error", expect.stringContaining("No match found"))
|
||||
})
|
||||
|
||||
it("returns error when occurrence count does not match expected_replacements", async () => {
|
||||
|
|
@ -277,10 +379,14 @@ describe("editFileTool", () => {
|
|||
{ fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("Expected 1 occurrence(s) but found 3")
|
||||
expect(result).toContain("<error_details>")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith("edit_file", "occurrence_mismatch")
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
expect(mockTask.recordToolError).toHaveBeenCalledWith(
|
||||
"edit_file",
|
||||
expect.stringContaining("Occurrence count mismatch"),
|
||||
)
|
||||
})
|
||||
|
||||
it("succeeds when occurrence count matches expected_replacements", async () => {
|
||||
|
|
@ -314,8 +420,8 @@ describe("editFileTool", () => {
|
|||
{ fileContent: "Line 1\nLine 2\nLine 3\nLine 4" },
|
||||
)
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("Expected 1 occurrence(s) but found 4")
|
||||
expect(result).toContain("<error_details>")
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -334,9 +440,11 @@ describe("editFileTool", () => {
|
|||
{ fileExists: true, fileContent: "Existing content" },
|
||||
)
|
||||
|
||||
expect(result).toContain("Error:")
|
||||
expect(result).toContain("File already exists")
|
||||
expect(result).toContain("<error_details>")
|
||||
expect(result).toContain("already exists")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -379,6 +487,43 @@ describe("editFileTool", () => {
|
|||
|
||||
expect(mockTask.ask).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("finalizes a partial tool preview row on failure (no stuck spinner)", async () => {
|
||||
// Path stabilization requires two consecutive calls with the same path
|
||||
await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true })
|
||||
await executeEditFileTool({ old_string: "NonExistent" }, { isPartial: true })
|
||||
|
||||
await executeEditFileTool(
|
||||
{ old_string: "NonExistent" },
|
||||
{ isPartial: false, fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
const askCalls = mockTask.ask.mock.calls
|
||||
const hasFinalToolAsk = askCalls.some((call: any[]) => call[0] === "tool" && call[2] === false)
|
||||
expect(hasFinalToolAsk).toBe(true)
|
||||
})
|
||||
|
||||
it("finalizes a partial tool preview row on no-op success (no changes needed)", async () => {
|
||||
// Path stabilization requires two consecutive calls with the same path
|
||||
await executeEditFileTool(
|
||||
{ old_string: " Line 2", new_string: "Line 2" },
|
||||
{ isPartial: true, fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
await executeEditFileTool(
|
||||
{ old_string: " Line 2", new_string: "Line 2" },
|
||||
{ isPartial: true, fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
const result = await executeEditFileTool(
|
||||
{ old_string: " Line 2", new_string: "Line 2" },
|
||||
{ isPartial: false, fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(result).toContain("No changes needed")
|
||||
const askCalls = mockTask.ask.mock.calls
|
||||
const hasFinalToolAsk = askCalls.some((call: any[]) => call[0] === "tool" && call[2] === false)
|
||||
expect(hasFinalToolAsk).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("error handling", () => {
|
||||
|
|
@ -409,9 +554,10 @@ describe("editFileTool", () => {
|
|||
toolProtocol: "native",
|
||||
})
|
||||
|
||||
expect(capturedResult).toContain("Error:")
|
||||
expect(capturedResult).toContain("Failed to read file")
|
||||
expect(capturedResult).toContain("<error_details>")
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockTask.didToolFailInCurrentTurn).toBe(true)
|
||||
})
|
||||
|
||||
it("handles general errors and resets diff view", async () => {
|
||||
|
|
@ -433,7 +579,7 @@ describe("editFileTool", () => {
|
|||
})
|
||||
|
||||
describe("CRLF normalization", () => {
|
||||
it("normalizes CRLF to LF when reading file", async () => {
|
||||
it("preserves CRLF line endings on output", async () => {
|
||||
const contentWithCRLF = "Line 1\r\nLine 2\r\nLine 3"
|
||||
|
||||
await executeEditFileTool(
|
||||
|
|
@ -443,6 +589,68 @@ describe("editFileTool", () => {
|
|||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith("Line 1\r\nModified Line 2\r\nLine 3", true)
|
||||
})
|
||||
|
||||
it("normalizes CRLF in old_string for matching against LF file content", async () => {
|
||||
await executeEditFileTool(
|
||||
{
|
||||
old_string: "Line 1\r\nLine 2\r\nLine 3",
|
||||
new_string: "Line 1\r\nModified Line 2\r\nLine 3",
|
||||
},
|
||||
{ fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith("Line 1\nModified Line 2\nLine 3", true)
|
||||
})
|
||||
})
|
||||
|
||||
describe("deterministic fallback matching", () => {
|
||||
it("recovers from whitespace/indentation mismatch (whitespace-tolerant regex)", async () => {
|
||||
await executeEditFileTool(
|
||||
{
|
||||
old_string: "start\nif (true) {\n return 1\n}\nend",
|
||||
new_string: "start\nif (true) {\n return 2\n}\nend",
|
||||
},
|
||||
{ fileContent: "start\nif (true) {\n\treturn 1\n}\nend" },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith(
|
||||
"start\nif (true) {\n return 2\n}\nend",
|
||||
true,
|
||||
)
|
||||
})
|
||||
|
||||
it("keeps $ literal under regex fallback replacement", async () => {
|
||||
await executeEditFileTool(
|
||||
{
|
||||
old_string: "Line 1\n Line 2\nLine 3",
|
||||
new_string: "Line 1\n Cost: $100\nLine 3",
|
||||
},
|
||||
{ fileContent: "Line 1\n\tLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith("Line 1\n Cost: $100\nLine 3", true)
|
||||
})
|
||||
|
||||
it("falls back to token-based regex when whitespace-tolerant regex cannot match", async () => {
|
||||
await executeEditFileTool(
|
||||
{
|
||||
old_string: " Line 2",
|
||||
new_string: "Row 2",
|
||||
},
|
||||
{ fileContent: "Line 1\nLine 2\nLine 3" },
|
||||
)
|
||||
|
||||
expect(mockTask.consecutiveMistakeCount).toBe(0)
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
expect(mockTask.diffViewProvider.update).toHaveBeenCalledWith("Line 1\nRow 2\nLine 3", true)
|
||||
})
|
||||
})
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue