8000 Fix: Ensure write_to_file creates empty files when content is empty by Ruakij · Pull Request #3871 · RooCodeInc/Roo-Code · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix: Ensure write_to_file creates empty files when content is empty #3871

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
Merged
merged 2 commits into from
May 29, 2025
Merged

Fix: Ensure write_to_file creates empty files when content is empty #3871

merged 2 commits into from
May 29, 2025

Conversation

Ruakij
Copy link
@Ruakij Ruakij commented May 22, 2025 •
edited
Loading

Early check for partial-block
Small typo
Missing path normalization

Related GitHub Issue

Closes: #3650

Description

This PR fixes a bug where the write_to_file tool would appear to succeed but fail to create files when the content parameter is empty. The tool now properly creates empty files when requested, ensuring consistent behavior regardless of content length.

Key changes:

  • Modified the file writing logic to handle empty content correctly
  • Enhanced error handling for edge cases
  • Refactor file for simplification and reduced code-duplication
  • Add Unit-Tests for writeToFileTool

Test Procedure

  • Unit-tests
  • Ask model to create an empty file

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • ✨ New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Documentation Updates

  • No documentation updates are required.

Get in Touch

Discord: ruakij

Important

Fixes writeToFileTool to handle empty content correctly, adds error handling, refactors for clarity, and introduces comprehensive unit tests.

  • Behavior:
    • Fixes writeToFileTool to create files even when content is empty in writeToFileTool.ts.
    • Adds early return for missing path or content in partial blocks.
    • Improves error handling for missing parameters and line count discrepancies.
  • Refactoring:
    • Introduces helper functions: validateAccess, checkFileExists, preprocessContent, createMessageProps, and handleDiffViewUpdate in writeToFileTool.ts.
    • Refactors file writing logic for clarity and reduced duplication.
  • Testing:
    • Adds writeToFileTool.test.ts with unit tests for parameter validation, access control, file existence, content preprocessing, file operations, partial block handling, user interaction, and error handling.

This description was created by Ellipsis for 06d8bbb. You can customize this summary. It will automatically update as commits are pushed.

@Ruakij Ruakij requested review from mrubens and cte as code owners May 22, 2025 21:40
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 23, 2025
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to TEMP in Roo Code Roadmap May 26, 2025
@daniel-lxs daniel-lxs mentioned this pull request May 27, 2025
Open
@daniel-lxs
Copy link
Collaborator

To keep in mind during review: This PR seems to be trying to solve the same issue as PR #3550.

@daniel-lxs daniel-lxs mentioned this pull request May 27, 2025
Closed
@daniel-lxs daniel-lxs moved this from TEMP to PR [Needs Review] in Roo Code Roadmap May 27, 2025
@KJ7LNW KJ7LNW self-requested a review May 27, 2025 17:00
@KJ7LNW
Copy link
Collaborator
KJ7LNW commented May 27, 2025

Pull requests to fix bugs need to be small and targeted, but looking at the differences this is huge and more than necessary to solve the problem.

@hannesrudolph hannesrudolph added the Followup Needs followup from support or code team label May 27, 2025
@daniel-lxs
Copy link
Collaborator

Hey @Ruakij, thank you for the contribution, I noticed that this PR introduces a lot of changes that might be outside of the scope of the issue you linked.

Would it make sense to split this PR and create new issues for the other improvements you implemented?

@daniel-lxs daniel-lxs added PR - Changes Requested and removed PR - Needs Preliminary Review Followup Needs followup from support or code team labels May 28, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Preliminary Review] to PR [Changes Requested] in Roo Code Roadmap May 28, 2025
@Ruakij Ruakij force-pushed the fix/3650/write_to_file-with-empty-content branch from 06d8bbb to 158c06c Compare May 29, 2025 07:58
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label May 29, 2025
@Ruakij Ruakij force-pushed the fix/3650/write_to_file-with-empty-content branch from 158c06c to 8d37e71 Compare May 29, 2025 08:07
Ruakij added 2 commits May 29, 2025 13:32
@Ruakij
Fix RooCodeInc#3650: Allow empty content in write_to_file tool
8e384a0 
Change validation from !newContent to newContent === undefined
to allow empty strings while still rejecting undefined values.
@Ruakij
Add unit tests for writeToFileTool
ccf8938
@Ruakij Ruakij force-pushed the fix/3650/write_to_file-with-empty-content branch from 8d37e71 to ccf8938 Compare May 29, 2025 11:40
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels May 29, 2025
@Ruakij
Copy link
Author
Ruakij commented May 29, 2025 •
edited
Loading

I minimized the PR.
Though there is at least another issues i had fixed before, when path is empty or missing or content is missing, no error is returned, because processing will simply stop on line 29.
I have found this through a unit-test. So i feel like this minimize-approach isnt very optimized.. I neither could add the test, nor fix the issue without a new writeup.
Though i do agree adding the refactor to the fix wasnt good.

@daniel-lxs
Copy link
Collaborator

@Ruakij
Thank you, It looks great, I will take a look right away

@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Prelim Review] in Roo Code Roadmap May 29, 2025
@daniel-lxs
Copy link
Collaborator

Looks good to me, the requested changes were properly addressed.

I'm aware that #3550 does pretty much the same thing however that PR currently has requested changes.

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap May 29, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 29, 2025
@mrubens mrubens merged commit 201fb3b into RooCodeInc:main May 29, 2025
17 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap May 29, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap May 29, 2025
@Ruakij Ruakij mentioned this pull request May 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrubens mrubens mrubens approved these changes

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

@KJ7LNW KJ7LNW Awaiting requested review from KJ7LNW

Assignees
No one assigned
Labels
lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Roo Code Roadmap
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: write_to_file with empty content appears to succeed but no file is created
5 participants
@Ruakij @daniel-lxs @KJ7LNW @mrubens @hannesrudolph
0