From 5e50c5543b4cb1c58c612efcd97efa6819cac32f Mon Sep 17 00:00:00 2001 From: KJ7LNW <93454819+KJ7LNW@users.noreply.github.com> Date: Sun, 1 Jun 2025 06:55:40 -0700 Subject: [PATCH] fix: prevent dump of an entire file into the context on user edit (#3654) * refactor: Add pushToolWriteResult method to DiffViewProvider Previously, each tool file contained duplicate code for formatting file write responses and conditionally sending user_feedback_diff messages. This led to inconsistent implementations and made changes difficult to maintain. This refactoring centralizes the response formatting and messaging logic in the DiffViewProvider class, which now: - Stores results from saveChanges() in class properties - Only sends user_feedback_diff when user edits exist - Configures XMLBuilder with no indentation for cleaner output Tool files now make a single method call instead of duplicating logic. Fixes: #3647 Signed-off-by: Eric Wheeler * fix: conditionally show user edits message in file write response Make the 'If the user's edits have addressed part of the task...' message conditional based on whether there are actual user edits. This prevents showing irrelevant guidance when no user edits were made to the file. Signed-off-by: Eric Wheeler --------- Signed-off-by: Eric Wheeler Co-authored-by: Eric Wheeler --- src/core/tools/applyDiffTool.ts | 34 ++------ src/core/tools/insertContentTool.ts | 35 ++------- src/core/tools/searchAndReplaceTool.ts | 35 ++------- src/core/tools/writeToFileTool.ts | 32 ++------ src/integrations/editor/DiffViewProvider.ts | 86 ++++++++++++++++++++- 5 files changed, 114 insertions(+), 108 deletions(-) diff --git a/src/core/tools/applyDiffTool.ts b/src/core/tools/applyDiffTool.ts index 2c637bc219..93c6bc5090 100644 --- a/src/core/tools/applyDiffTool.ts +++ b/src/core/tools/applyDiffTool.ts @@ -9,7 +9,6 @@ import { Task } from "../task/Task" import { ToolUse, RemoveClosingTag, AskApproval, HandleError, PushToolResult } from "../../shared/tools" import { formatResponse } from "../prompts/responses" import { fileExistsAtPath } from "../../utils/fs" -import { addLineNumbers } from "../../integrations/misc/extract-text" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { unescapeHtmlEntities } from "../../utils/text-normalization" @@ -163,7 +162,8 @@ export async function applyDiffTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -178,33 +178,13 @@ export async function applyDiffTool( partFailHint = `But unable to apply all diff parts to file: ${absolutePath}, silently use tool to check newest file version and re-apply diffs\n` } - if (userEdits) { - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), - ) + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - partFailHint + - `The updated content, which includes both your original modifications and the user's edits, has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file, including line numbers:\n\n` + - `\n${addLineNumbers( - finalContent || "", - )}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) + if (partFailHint) { + pushToolResult(partFailHint + message) } else { - pushToolResult( - `Changes successfully applied to ${relPath.toPosix()}:\n\n${newProblemsMessage}\n` + partFailHint, - ) + pushToolResult(message) } await cline.diffViewProvider.reset() diff --git a/src/core/tools/insertContentTool.ts b/src/core/tools/insertContentTool.ts index 910bed5fe4..0963bc78cc 100644 --- a/src/core/tools/insertContentTool.ts +++ b/src/core/tools/insertContentTool.ts @@ -136,7 +136,8 @@ export async function insertContentTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -145,34 +146,14 @@ export async function insertContentTool( cline.didEditFile = true - if (!userEdits) { - pushToolResult( - `The content was successfully inserted in ${relPath.toPosix()} at line ${lineNumber}.${newProblemsMessage}`, - ) - await cline.diffViewProvider.reset() - return - } - - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: "insertContent", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - lineNumber: lineNumber, - } satisfies ClineSayTool), + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult( + cline, + cline.cwd, + false, // Always false for insert_content ) - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - `The updated content has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file:\n\n` + - `\n${finalContent}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) + pushToolResult(message) await cline.diffViewProvider.reset() } catch (error) { diff --git a/src/core/tools/searchAndReplaceTool.ts b/src/core/tools/searchAndReplaceTool.ts index de98fcafea..58d246b133 100644 --- a/src/core/tools/searchAndReplaceTool.ts +++ b/src/core/tools/searchAndReplaceTool.ts @@ -219,7 +219,8 @@ export async function searchAndReplaceTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -228,34 +229,14 @@ export async function searchAndReplaceTool( cline.didEditFile = true - if (!userEdits) { - pushToolResult(`The content was successfully replaced in ${relPath}.${newProblemsMessage}`) - await cline.diffViewProvider.reset() - return - } - - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: "appliedDiff", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult( + cline, + cline.cwd, + false, // Always false for search_and_replace ) - // Format and send response with user's updates - const resultMessage = [ - `The user made the following updates to your content:\n\n${userEdits}\n\n`, - `The updated content has been successfully saved to ${validRelPath.toPosix()}. Here is the full, updated content of the file:\n\n`, - `\n${finalContent}\n\n\n`, - `Please note:\n`, - `1. You do not need to re-write the file with these changes, as they have already been applied.\n`, - `2. Proceed with the task using the updated file content as the new baseline.\n`, - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.`, - newProblemsMessage, - ].join("") - - pushToolResult(resultMessage) + pushToolResult(message) // Record successful tool usage and cleanup cline.recordToolUsage("search_and_replace") diff --git a/src/core/tools/writeToFileTool.ts b/src/core/tools/writeToFileTool.ts index 946e0d63dd..63191acb7e 100644 --- a/src/core/tools/writeToFileTool.ts +++ b/src/core/tools/writeToFileTool.ts @@ -8,7 +8,7 @@ import { formatResponse } from "../prompts/responses" import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "../../shared/tools" import { RecordSource } from "../context-tracking/FileContextTrackerTypes" import { fileExistsAtPath } from "../../utils/fs" -import { addLineNumbers, stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" +import { stripLineNumbers, everyLineHasLineNumbers } from "../../integrations/misc/extract-text" import { getReadablePath } from "../../utils/path" import { isPathOutsideWorkspace } from "../../utils/pathUtils" import { detectCodeOmission } from "../../integrations/editor/detect-omission" @@ -208,7 +208,8 @@ export async function writeToFileTool( return } - const { newProblemsMessage, userEdits, finalContent } = await cline.diffViewProvider.saveChanges() + // Call saveChanges to update the DiffViewProvider properties + await cline.diffViewProvider.saveChanges() // Track file edit operation if (relPath) { @@ -217,31 +218,10 @@ export async function writeToFileTool( cline.didEditFile = true // used to determine if we should wait for busy terminal to update before sending api request - if (userEdits) { - await cline.say( - "user_feedback_diff", - JSON.stringify({ - tool: fileExists ? "editedExistingFile" : "newFileCreated", - path: getReadablePath(cline.cwd, relPath), - diff: userEdits, - } satisfies ClineSayTool), - ) + // Get the formatted response message + const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists) - pushToolResult( - `The user made the following updates to your content:\n\n${userEdits}\n\n` + - `The updated content, which includes both your original modifications and the user's edits, has been successfully saved to ${relPath.toPosix()}. Here is the full, updated content of the file, including line numbers:\n\n` + - `\n${addLineNumbers( - finalContent || "", - )}\n\n\n` + - `Please note:\n` + - `1. You do not need to re-write the file with these changes, as they have already been applied.\n` + - `2. Proceed with the task using this updated file content as the new baseline.\n` + - `3. If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.` + - `${newProblemsMessage}`, - ) - } else { - pushToolResult(`The content was successfully saved to ${relPath.toPosix()}.${newProblemsMessage}`) - } + pushToolResult(message) await cline.diffViewProvider.reset() diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index adc19ff014..3ab0419618 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -3,11 +3,14 @@ import * as path from "path" import * as fs from "fs/promises" import * as diff from "diff" import stripBom from "strip-bom" +import { XMLBuilder } from "fast-xml-parser" import { createDirectoriesForFile } from "../../utils/fs" -import { arePathsEqual } from "../../utils/path" +import { arePathsEqual, getReadablePath } from "../../utils/path" import { formatResponse } from "../../core/prompts/responses" import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics" +import { ClineSayTool } from "../../shared/ExtensionMessage" +import { Task } from "../../core/task/Task" import { DecorationController } from "./DecorationController" @@ -15,6 +18,9 @@ export const DIFF_VIEW_URI_SCHEME = "cline-diff" // TODO: https://github.com/cline/cline/pull/3354 export class DiffViewProvider { + // Properties to store the results of saveChanges + newProblemsMessage?: string + userEdits?: string editType?: "create" | "modify" isEditing = false originalContent: string | undefined @@ -238,13 +244,91 @@ export class DiffViewProvider { normalizedEditedContent, ) + // Store the results as class properties for formatFileWriteResponse to use + this.newProblemsMessage = newProblemsMessage + this.userEdits = userEdits + return { newProblemsMessage, userEdits, finalContent: normalizedEditedContent } } else { // No changes to Roo's edits. + // Store the results as class properties for formatFileWriteResponse to use + this.newProblemsMessage = newProblemsMessage + this.userEdits = undefined + return { newProblemsMessage, userEdits: undefined, finalContent: normalizedEditedContent } } } + /** + * Formats a standardized XML response for file write operations + * + * @param cwd Current working directory for path resolution + * @param isNewFile Whether this is a new file or an existing file being modified + * @returns Formatted message and say object for UI feedback + */ + async pushToolWriteResult(task: Task, cwd: string, isNewFile: boolean): Promise { + if (!this.relPath) { + throw new Error("No file path available in DiffViewProvider") + } + + // Only send user_feedback_diff if userEdits exists + if (this.userEdits) { + // Create say object for UI feedback + const say: ClineSayTool = { + tool: isNewFile ? "newFileCreated" : "editedExistingFile", + path: getReadablePath(cwd, this.relPath), + diff: this.userEdits, + } + + // Send the user feedback + await task.say("user_feedback_diff", JSON.stringify(say)) + } + + // Build XML response + const xmlObj = { + file_write_result: { + path: this.relPath, + operation: isNewFile ? "created" : "modified", + user_edits: this.userEdits ? this.userEdits : undefined, + problems: this.newProblemsMessage || undefined, + notice: { + i: [ + "You do not need to re-read the file, as you have seen all changes", + "Proceed with the task using these changes as the new baseline.", + ...(this.userEdits + ? [ + "If the user's edits have addressed part of the task or changed the requirements, adjust your approach accordingly.", + ] + : []), + ], + }, + }, + } + + const builder = new XMLBuilder({ + format: true, + indentBy: "", + suppressEmptyNode: true, + processEntities: false, + tagValueProcessor: (name, value) => { + if (typeof value === "string") { + // Only escape <, >, and & characters + return value.replace(/&/g, "&").replace(//g, ">") + } + return value + }, + attributeValueProcessor: (name, value) => { + if (typeof value === "string") { + // Only escape <, >, and & characters + return value.replace(/&/g, "&").replace(//g, ">") + } + return value + }, + }) + + return builder.build(xmlObj) + } + async revertChanges(): Promise { if (!this.relPath || !this.activeDiffEditor) { return