Skip to content

fix: prevent dump of an entire file into the context on user edit #3654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 1, 2025
Merged

fix: prevent dump of an entire file into the context on user edit #3654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a119ca4
refactor: Add pushToolWriteResult method to DiffViewProvider
May 16, 2025
c91d564
fix: conditionally show user edits message in file write response
May 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 8 additions & 28 deletions src/core/tools/applyDiffTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { ToolUse, RemoveClosingTag } from "../../shared/tools"
import { formatResponse } from "../prompts/responses"
import { AskApproval, HandleError, PushToolResult } from "../../shared/tools"
import { fileExistsAtPath } from "../../utils/fs"
import { addLineNumbers } from "../../integrations/misc/extract-text"
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
import { telemetryService } from "../../services/telemetry/TelemetryService"
import { unescapeHtmlEntities } from "../../utils/text-normalization"
Expand Down Expand Up @@ -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) {
Expand All @@ -178,33 +178,13 @@ export async function applyDiffTool(
partFailHint = `But unable to apply all diff parts to file: ${absolutePath}, silently use <read_file> 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),
)

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` +
`<final_file_content path="${relPath.toPosix()}">\n${addLineNumbers(
finalContent || "",
)}\n</final_file_content>\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}`,
)
// Get the formatted response message
const message = await cline.diffViewProvider.pushToolWriteResult(cline, cline.cwd, !fileExists)

if (partFailHint) {
pushToolResult(partFailHint + message)
} else {
pushToolResult(
`Changes successfully applied to ${relPath.toPosix()}:\n\n${newProblemsMessage}\n` + partFailHint,
)
pushToolResult(message)
}

await cline.diffViewProvider.reset()
Expand Down
35 changes: 8 additions & 27 deletions src/core/tools/insertContentTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,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) {
Expand All @@ -137,34 +138,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` +
`<final_file_content path="${relPath.toPosix()}">\n${finalContent}\n</final_file_content>\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) {
Expand Down
35 changes: 8 additions & 27 deletions src/core/tools/searchAndReplaceTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,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) {
Expand All @@ -220,34 +221,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`,
`<final_file_content path="${validRelPath.toPosix()}">\n${finalContent}\n</final_file_content>\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")
Expand Down
32 changes: 6 additions & 26 deletions src/core/tools/writeToFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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` +
`<final_file_content path="${relPath.toPosix()}">\n${addLineNumbers(
finalContent || "",
)}\n</final_file_content>\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()

Expand Down
86 changes: 85 additions & 1 deletion src/integrations/editor/DiffViewProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,24 @@ 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"

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
Expand Down Expand Up @@ -235,13 +241,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
Copy link
Member

@daniel-lxs daniel-lxs May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have an unit test for this new method?

*/
async pushToolWriteResult(task: Task, cwd: string, isNewFile: boolean): Promise<string> {
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: {
Copy link
Member

@daniel-lxs daniel-lxs May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the use of an array of i tags here for the notice. Is there a specific reason for structuring it this way instead of just using the content directly within <notice> tags? Just wondering if the i tags serve a particular purpose in how the model processes this information, or if we could simplify this structure.

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, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;")
}
return value
},
attributeValueProcessor: (name, value) => {
if (typeof value === "string") {
// Only escape <, >, and & characters
return value.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;")
}
return value
},
})

return builder.build(xmlObj)
}

async revertChanges(): Promise<void> {
if (!this.relPath || !this.activeDiffEditor) {
return
Expand Down
Loading