Fix: Allow write_to_file to handle newline-only and empty content #3550
Conversation
|
|
nice. add a test and also check for any |
|
@Githubguy132010 can you please update this to follow and issues first approach? |
There is, see #3533 and @Githubguy132010 mentioned it above ^^ |
Ok. I did that. |
|
I think generally while adding tests for the feature is great, BUT there are so many tests that it is impossible for me to tell what you are testing specific to this pull request. warning: AI tests have a tendency to create so much mocking and that by the time it tests what it expects it basically becomes a very elaborate check for review all tests to make sure that this is not the case |
|
To keep in mind during review: This PR seems to be trying to solve the same issue as PR #3871. |
|
@daniel-lxs LGTM |
|
@KJ7LNW |
|
Thank you @daniel-lxs . I just tested it and the result looks good. |
|
Regarding possible side effects: if there are any, then those are actually bugs elsewhere in the application. From a programming perspective, the parser's job is to parse, and that's it: it should provide exactly what was parsed to any tooling and the tools should do whatever they need to do with the result. If the tool receives something unexpected then it is the tool's job to clean it up, not the parser. That said, this is a production application, so it could be a good idea to run this through a simple eval and see if any breaks catastrophically, but I do not expect it because the model does a pretty good job of using the correct white space and all of the examples I can remember. Do you have a quick way to tell the eval system "run against PR 3550" ? |
|
I'll run some Integration tests on this tomorrow, see if anything pops up. |
|
It seems that as a side effect the files created always have a new line at the beginning, making the 2) Roo Code write_to_file Tool
Should create nested directories when writing file:
AssertionError [ERR_ASSERTION]: File content should match
+ actual - expected
+ '\nFile in nested directory\n'
- 'File in nested directory'
+ expected - actual
-
-File in nested directory
+File in nested directoryI'll check if I can fix this later. |
…erve newlines in content parameters while stripping leading and trailing newlines
) * Fix: Allow write_to_file to handle newline-only and empty content * fix: update writeToFileTool to return early without error on missing or empty parameters * fix: preserve newlines in content parameters and update error handling in writeToFileTool tests * fix: update parseAssistantMessage and parseAssistantMessageV2 to preserve newlines in content parameters while stripping leading and trailing newlines --------- Co-authored-by: Daniel Riccio <ricciodaniel98@gmail.com>
…oCodeInc#3550) * Fix: Allow write_to_file to handle newline-only and empty content * fix: update writeToFileTool to return early without error on missing or empty parameters * fix: preserve newlines in content parameters and update error handling in writeToFileTool tests * fix: update parseAssistantMessage and parseAssistantMessageV2 to preserve newlines in content parameters while stripping leading and trailing newlines --------- Co-authored-by: Daniel Riccio <ricciodaniel98@gmail.com>
Closes #3533
This pull request updates the
writeToFileToolfunction insrc/core/tools/writeToFileTool.tsto improve the handling of thenewContentparameter by explicitly checking forundefinedinstead of falsy values. This ensures more precise validation and avoids potential issues with empty strings or other falsy but valid inputs.Improvements to parameter validation:
ifstatement to explicitly check ifnewContentisundefinedinstead of relying on a general falsy check. This ensures that valid but falsy values (e.g., empty strings) are not incorrectly treated as invalid.ifstatement to usenewContent === undefinedfor consistency and to avoid unintended behavior whennewContentis a valid falsy value.Important
Update
writeToFileToolto explicitly check forundefinedinnewContentto handle empty strings and other falsy values correctly.writeToFileToolinwriteToFileTool.tsto explicitly checknewContent === undefinedinstead of falsy check.ifstatement to usenewContent === undefined.ifstatement for consistency and correct handling of falsy values.This description was created by
for 2fcaffa. You can customize this summary. It will automatically update as commits are pushed.