8000 fix: prevent dump of an entire file into the context on user edit by KJ7LNW · Pull Request #3654 · RooCodeInc/Roo-Code · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from

Conversation

KJ7LNW
Copy link
Collaborator
@KJ7LNW KJ7LNW commented May 16, 2025 •
edited
Loading

Problem fixed by this PR

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

  1. auto approve is turned off for writes
  2. the model proposes a file modification
  3. the user modifies the file before clicking save
  4. 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:

  1. distracts the model from the focus (especially with large files)
  2. bloats context
  3. 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.

  1. write_to_file: Commit cbf942e (Oct 18, 2024) by Saoud Rizwan
  2. apply_diff: Commit c0b070e, PR Improvements to apply_diff #52 (Dec 8, 2024) by @mrubens
  3. 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 Ellipsis for a119ca4. You can customize this summary. It will automatically update as commits are pushed.

refactor: Add pushToolWriteResult method to DiffViewProvider
a119ca4 
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: cline#3647
Signed-off-by: Eric Wheeler <roo-code@z.ewheeler.org>
@KJ7LNW KJ7LNW requested review from mrubens and cte as code owners May 16, 2025 05:11
@changeset-bot changeset-bot
Copy link
changeset-bot bot commented May 16, 2025 •
edited
Loading

⚠️ 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

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 16, 2025
fix: conditionally show user edits message in file write response
c91d564 
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 <roo-code@z.ewheeler.org>
@KJ7LNW
Copy link
Collaborator Author
KJ7LNW commented May 16, 2025 •
edited
Loading

@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
Copy link
Collaborator Author
KJ7LNW commented May 16, 2025

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

@samhvw8
Copy link
Collaborator
samhvw8 commented May 16, 2025

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

@samhvw8 samhvw8 mentioned this pull request May 16, 2025
Draft
23 tasks
@KJ7LNW KJ7LNW self-assigned this May 16, 2025
@KJ7LNW
Copy link
Collaborator Author
KJ7LNW commented May 16, 2025

multi-read not related to this

true

@KJ7LNW
Copy link
Collaborator Author
KJ7LNW commented May 17, 2025 •
edited
Loading

@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(-)

@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 20, 2025
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 20, 2025
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to TEMP in Roo Code Roadmap May 26, 2025
@hannesrudolph hannesrudolph moved this from TEMP to PR [Needs Review] in Roo Code Roadmap May 26, 2025
@KJ7LNW 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrubens mrubens Awaiting requested review from mrubens mrubens is a code owner

@cte cte Awaiting requested review from cte cte is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@KJ7LNW KJ7LNW

Labels
PR - Needs Preliminary Review size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Roo Code Roadmap
Status: PR [Needs Prelim Review]
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Roo dumps entire file into the context when a user edits during apply_diff before clicking save
3 participants
@KJ7LNW @samhvw8 @hannesrudolph
0