Conversation
EditFileTool read the target file with errors='replace' and wrote the whole decoded string back, so editing one ASCII line permanently rewrote every non-UTF-8 byte elsewhere in the file as U+FFFD while reporting status: ok. The write-back also truncated the file before encoding, so a new_string that could not be encoded left the file at zero bytes. Decode with errors='surrogateescape', encode before calling write_file, and return the tool's own error dict when the encode fails. write_file already accepts str | bytes and _sync_write already has a 'wb' branch, so no new dependency or error path is introduced. Fixes google#7314
1wos
force-pushed
the
fix/editfile-preserve-non-utf8-bytes
branch
from
September 27, 2026 14:14
b6acb37 to
a92d1d5
Compare
Contributor
|
Thanks for the careful EditFileTool fix. I noticed the related LocalEnvironment._sync_write path can truncate an existing file when model-supplied text contains a character UTF-8 cannot encode. I can prepare a small follow-up that encodes before opening the file and tests that its bytes remain unchanged on failure. Are you already planning to cover that path? |
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
EditFileToolpermanently destroys non-UTF-8 bytes outside the edited region #73142. Or, if no issue exists, describe the change:
Problem:
EditFileToolreads the target file witherrors='replace'atsrc/google/adk/tools/environment/_edit_file_tool.py:104and writes the wholedecoded string back at line 133. Editing one ASCII line therefore rewrites every
non-UTF-8 byte elsewhere in the file as U+FFFD on disk, and the tool returns
status: ok, so neither the model nor the user learns that bytes were replaced.The write-back fails a second way.
open(path, 'w')truncates the file before theencode raises, so a
new_stringthat cannot be encoded leaves the file empty:ReadFileTooldecodes the same lossy way, butReadFileToolonly changes what themodel is shown and leaves the file alone.
EditFileToolis the only tool thatwrites the lossy decode back.
Solution:
src/google/adk/tools/environment/_edit_file_tool.py:errors='surrogateescape'so undecodable bytes round-trip instead ofcollapsing to U+FFFD.
write_file, so a failed encode cannot truncate the file.{'status': 'error'}dict when the encode fails, instead ofraising out of
run_async. A lone surrogate reaches the tool from ordinary modeloutput:
json.loads('{"x": "\ud800"}')produces one, andjsonis what parses atool call.
BaseEnvironment.write_fileis already typedcontent: str | bytes(
_base_environment.py:125) andLocalEnvironment._sync_writealready branches toopen(path, 'wb')(_local_environment.py:236-243), so this adds no dependency andno new name.
DaytonaEnvironmentconvertsstrto bytes before upload andE2BEnvironmentpasses content straight through, so bytes is in contract for allthree implementations.
Testing Plan
Unit Tests:
Added two tests to
tests/unittests/tools/environment/test_edit_file_tool.py,following the byte-level assertions the existing CRLF tests already use:
test_edit_file_preserves_non_utf8_bytes— editsOLDtoNEWinb"# header \xe9\nOLD\n"and asserts the file isb"# header \xe9\nNEW\n".test_edit_file_reports_unencodable_new_string— passes"\ud800"asnew_stringand assertsstatus: errorand that the file is byte-identical.Reverting the decode fails one of them; reverting the encode fails both.
Wider run, to confirm nothing else moved:
The two pre-existing CRLF round-trip tests still pass, which is what shows the
open('w', newline='')toopen('wb')switch changed no line-ending behavior.Manual End-to-End (E2E) Tests:
Ran the tool directly against a
LocalEnvironmenton a temp directory, on main andon this branch.
On main:
On this branch:
With
new_string="\ud800", main raisesUnicodeEncodeErrorand leaves the file atb''; this branch returns{'status': 'error', ...}and leaves the file unchanged.Checklist
Additional context
pre-commit runpasses on both edited files, includingruff,isort,pyink,codespelland the ADK compliance checks. The twopylintC0301 findings in_edit_file_tool.pyare pre-existing, on lines this PR does not touch.Not a regression: the decode and write-back pair has been in place since the
environment toolset was added in
9082b9e3(2026-03-27), at_tools.py:346and:369;1cc298edmoved it into_edit_file_tool.pyunchanged.No overlap with the open PRs in this area. #7133 and #7176 touch
_read_file_tool.pyalongside_base_environment.pyand_local_environment.py,but not
_edit_file_tool.py._write_file_tool.py:83passes a model-suppliedstrtowrite_fileand has thesame truncate-before-encode exposure. I left it out to keep this PR to one concern
and can follow up separately.