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

KJ7LNW · 2025-05-16T05:11:21Z

Problem fixed by this PR

This pull request fixes the problem of dumping an entire file into the context under the following condition:

auto approve is turned off for writes
the model proposes a file modification
the user modifies the file before clicking save
user clicks save

After the user clicks save, <final_file_content>[entire file]</final_file_content> is dumped into the conversation even for a one-line change, which is a severe problem because it:

distracts the model from the focus (especially with large files)
bloats context
increases costs

Fixes: #3647

The fix

The change by the user shown in Step-3, above, is provided to the model in the existing implementation using the standard diff format, so it understands the change that was made. All we are doing here is omitting <final_file_content>[entire file]</final_file_content> to prevent context bloat and maintain model focus.

Because this issue exists in all tools that modify files, this pull request is also a refactor to standardize the tool response for file modifications across write_to_file, apply_diff, search_and_replace, and insert_content:

]$ git diff  origin/main... --stat
 src/core/tools/applyDiffTool.ts             | 36 ++++------------
 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, 115 insertions(+), 109 deletions(-)

History

The final_file_content feature was introduced by Saoud Rizwan and copy-pasted across all file modification tools as new tools were added.

write_to_file: Commit cbf942e (Oct 18, 2024) by Saoud Rizwan
apply_diff: Commit c0b070e, PR Improvements to apply_diff #52 (Dec 8, 2024) by @mrubens
search_and_replace and insert_content: Commit 2c97b59 (Jan 21, 2025) by @mrubens

Important

Refactored to centralize response formatting and messaging in DiffViewProvider, adding pushToolWriteResult for XML response and user feedback, affecting multiple tool files.

Refactoring:
- Centralized response formatting and messaging logic in DiffViewProvider.
- Added pushToolWriteResult method to DiffViewProvider for XML response formatting and user feedback messaging.
- Stores results from saveChanges in class properties.
Behavior:
- Tool files (applyDiffTool.ts, insertContentTool.ts, searchAndReplaceTool.ts, writeToFileTool.ts) now call pushToolWriteResult instead of duplicating logic.
- Sends user_feedback_diff only when user edits exist.
- Configures XMLBuilder with no indentation for cleaner output.
Misc:
- Removed addLineNumbers import from applyDiffTool.ts and writeToFileTool.ts.

^{This description was created by}^{for a119ca4. You can customize this summary. It will automatically update as commits are pushed.}

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 <[email protected]>

changeset-bot · 2025-05-16T05:11:26Z

⚠️ No Changeset found

Latest commit: c91d564

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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 <[email protected]>

KJ7LNW · 2025-05-16T05:26:47Z

@mrubens @cte @hannesrudolph

given https://www.reddit.com/r/RooCode/comments/1kn6xwj/pruning_ai_turn_from_context/ :

I think this should be manually tested for regression and then merged quickly.

This is intended to be a simple refactor that centralizes file modification responses to the model and removes final_file_content which has a number of problems described in the OP above.

I have had the model test each tool and various scenarios along with testing user modifications and I have not found any issues with it.

KJ7LNW · 2025-05-16T06:00:36Z

@samhvw8 can you set your multi-file PR to "draft" until this lands?

samhvw8 · 2025-05-16T06:03:12Z

@KJ7LNW i will put multi-write pr into draft mode, i think multi-read not related to this

KJ7LNW · 2025-05-16T06:10:07Z

multi-read not related to this

true

KJ7LNW · 2025-05-17T01:59:53Z

@mrubens @cte

This PR should merge as preparatory standardization for #3342, #3055, #1899, #2955 and it fixes #3647:

Llm fall pretty quickly into local minimum when they get fed their own responses in a multiturn generation, such as those of coding agents [from here]

Thus, final_file_content must go because it impedes model progress. The final_file_content feature was introduced by Saoud Rizwan and copy-pasted across all file modification tools as new tools were added. This PR only affects "user edits" such as when diff view is presented and the user changes something before clicking Roo's "Save" button: instead of dumping the entire huge file to the model, it shows the model a diff what changed.

If we can push this out soon, then it will simplify development on other fronts. As you can see it substantially reduces the footprint of each tool that modifies files by replacing common call paths with a single function call:

]$ git diff  origin/main... --stat
 src/core/tools/applyDiffTool.ts             | 36 ++++------------
 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, 115 insertions(+), 109 deletions(-)

daniel-lxs

Hey @KJ7LNW, I really like this fix. I think it's crucial to add this to Roo. Everything looks pretty good, I just have a couple of questions. Looking forward to helping get this merged!

Thank you!

daniel-lxs · 2025-05-31T23:15:14Z

src/integrations/editor/DiffViewProvider.ts

+	 *
+	 * @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


Should we have an unit test for this new method?

daniel-lxs · 2025-05-31T23:15:30Z

src/integrations/editor/DiffViewProvider.ts

+				operation: isNewFile ? "created" : "modified",
+				user_edits: this.userEdits ? this.userEdits : undefined,
+				problems: this.newProblemsMessage || undefined,
+				notice: {


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.

mrubens

Thanks for the cleanup here! This has been waiting for a while, so I think it’s ok to discuss the remaining comments as a follow up.

mrubens · 2025-06-01T20:50:54Z

Er, a test is failing in main so I'm going to revert this to be safe.

KJ7LNW · 2025-06-02T20:50:10Z

Er, a test is failing in main so I'm going to revert this to be safe.

understood. I will rebase and see if we can figure out why

KJ7LNW requested review from cte and mrubens as code owners May 16, 2025 05:11

github-project-automation bot added this to Roo Code Roadmap May 16, 2025

github-project-automation bot moved this to New in Roo Code Roadmap May 16, 2025

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 16, 2025

samhvw8 mentioned this pull request May 16, 2025

feat: Enhance apply_diff with XML for multi-file/multi-diff operations & batch UI #3342

Merged

23 tasks

KJ7LNW self-assigned this May 16, 2025

hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 20, 2025

mrubens added this to Roo Code Roadmap May 20, 2025

github-project-automation bot moved this to New in Roo Code Roadmap May 20, 2025

hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 20, 2025

hannesrudolph added the PR - Needs Preliminary Review label May 26, 2025

hannesrudolph moved this from PR [Needs Review] to TEMP in Roo Code Roadmap May 26, 2025

hannesrudolph moved this from TEMP to PR [Needs Review] in Roo Code Roadmap May 26, 2025

KJ7LNW changed the title ~~refactor: Add pushToolWriteResult method to DiffViewProvider~~ fix: prevent dump of an entire file into the context on user edit May 30, 2025

daniel-lxs reviewed May 31, 2025

View reviewed changes

daniel-lxs moved this from PR [Needs Prelim Review] to PR [Changes Requested] in Roo Code Roadmap May 31, 2025

daniel-lxs added PR - Changes Requested and removed PR - Needs Preliminary Review labels May 31, 2025

mrubens approved these changes Jun 1, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 1, 2025

mrubens merged commit 5e50c55 into RooCodeInc:main Jun 1, 2025
18 checks passed

github-project-automation bot moved this from PR [Changes Requested] to Done in Roo Code Roadmap Jun 1, 2025

github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Jun 1, 2025

mrubens mentioned this pull request Jun 1, 2025

Revert "fix: prevent dump of an entire file into the context on user edit " #4211

Merged

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

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

Uh oh!

Conversation

KJ7LNW commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem fixed by this PR

The fix

History

Uh oh!

changeset-bot bot commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

KJ7LNW commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KJ7LNW commented May 16, 2025

Uh oh!

samhvw8 commented May 16, 2025

Uh oh!

KJ7LNW commented May 16, 2025

Uh oh!

KJ7LNW commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daniel-lxs left a comment

Choose a reason for hiding this comment

Uh oh!

daniel-lxs May 31, 2025

Choose a reason for hiding this comment

Uh oh!

daniel-lxs May 31, 2025

Choose a reason for hiding this comment

Uh oh!

mrubens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrubens commented Jun 1, 2025

Uh oh!

KJ7LNW commented Jun 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KJ7LNW commented May 16, 2025 •

edited

Loading

changeset-bot bot commented May 16, 2025 •

edited

Loading

KJ7LNW commented May 16, 2025 •

edited

Loading

KJ7LNW commented May 17, 2025 •

edited

Loading