Skip to content

fix(tools): preserve non-UTF-8 bytes in EditFileTool - #7316

Open
1wos wants to merge 1 commit into
google:mainfrom
1wos:fix/editfile-preserve-non-utf8-bytes
Open

1wos wants to merge 1 commit into
google:mainfrom
1wos:fix/editfile-preserve-non-utf8-bytes

Conversation

@1wos

@1wos 1wos commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

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):

2. Or, if no issue exists, describe the change:

Problem:

EditFileTool reads the target file with errors='replace' at
src/google/adk/tools/environment/_edit_file_tool.py:104 and writes the whole
decoded 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.

before: b'# caf\xe9 note\nOLD\n'
result: {'status': 'ok', 'message': 'Edited notes.py'}
after : b'# caf\xef\xbf\xbd note\nNEW\n'

The write-back fails a second way. open(path, 'w') truncates the file before the
encode raises, so a new_string that cannot be encoded leaves the file empty:

before: b'# caf\xe9 note\nOLD\n'
raised: UnicodeEncodeError
after : b''

ReadFileTool decodes the same lossy way, but ReadFileTool only changes what the
model is shown and leaves the file alone. EditFileTool is the only tool that
writes the lossy decode back.

Solution:

src/google/adk/tools/environment/_edit_file_tool.py:

  • Decode with errors='surrogateescape' so undecodable bytes round-trip instead of
    collapsing to U+FFFD.
  • Encode before calling write_file, so a failed encode cannot truncate the file.
  • Return the tool's own {'status': 'error'} dict when the encode fails, instead of
    raising out of run_async. A lone surrogate reaches the tool from ordinary model
    output: json.loads('{"x": "\ud800"}') produces one, and json is what parses a
    tool call.

BaseEnvironment.write_file is already typed content: str | bytes
(_base_environment.py:125) and LocalEnvironment._sync_write already branches to
open(path, 'wb') (_local_environment.py:236-243), so this adds no dependency and
no new name. DaytonaEnvironment converts str to bytes before upload and
E2BEnvironment passes content straight through, so bytes is in contract for all
three implementations.

-      content = data_bytes.decode('utf-8', errors='replace')
+      content = data_bytes.decode('utf-8', errors='surrogateescape')

     new_content = re.sub(pattern, lambda m: new_string, content, count=1)
-    await self._environment.write_file(path, new_content)
+    try:
+      # Encode before opening the file: a failure here must not truncate it.
+      data = new_content.encode('utf-8', errors='surrogateescape')
+    except UnicodeEncodeError:
+      return {
+          'status': 'error',
+          'error': (
+              '`new_string` contains characters that cannot be encoded. '
+              'The file was not modified.'
+          ),
+      }
+    await self._environment.write_file(path, data)

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

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 — edits OLD to NEW in
    b"# header \xe9\nOLD\n" and asserts the file is b"# header \xe9\nNEW\n".
  • test_edit_file_reports_unencodable_new_string — passes "\ud800" as
    new_string and asserts status: error and that the file is byte-identical.

Reverting the decode fails one of them; reverting the encode fails both.

$ pytest tests/unittests/tools/environment/test_edit_file_tool.py -v
============================= test session starts ==============================
platform darwin -- Python 3.12.12, pytest-9.1.1, pluggy-1.6.0
collected 7 items

test_edit_file_handles_line_breaks_linux_file_windows_search PASSED      [ 14%]
test_edit_file_handles_line_breaks_windows_file_linux_search PASSED      [ 28%]
test_edit_file_fails_on_multiple_matches PASSED                          [ 42%]
test_edit_file_exact_match_works PASSED                                  [ 57%]
test_edit_file_handles_special_regex_chars PASSED                        [ 71%]
test_edit_file_preserves_non_utf8_bytes PASSED                           [ 85%]
test_edit_file_reports_unencodable_new_string PASSED                     [100%]

============================== 7 passed in 0.53s ===============================

Wider run, to confirm nothing else moved:

$ pytest tests/unittests/tools/ tests/unittests/environment/ -q
2544 passed, 1 skipped in 38.14s

The two pre-existing CRLF round-trip tests still pass, which is what shows the
open('w', newline='') to open('wb') switch changed no line-ending behavior.

Manual End-to-End (E2E) Tests:

Ran the tool directly against a LocalEnvironment on a temp directory, on main and
on this branch.

import asyncio, tempfile
from pathlib import Path
from google.adk.environment._local_environment import LocalEnvironment
from google.adk.tools.environment._edit_file_tool import EditFileTool

async def main():
    with tempfile.TemporaryDirectory() as tmp:
        env = LocalEnvironment(working_dir=Path(tmp))
        await env.initialize()
        target = Path(tmp) / "notes.py"
        target.write_bytes(b"# caf\xe9 note\nOLD\n")
        print("before:", target.read_bytes())
        print("result:", await EditFileTool(env).run_async(
            args={"path": "notes.py", "old_string": "OLD", "new_string": "NEW"},
            tool_context=None))
        print("after :", target.read_bytes())
        await env.close()

asyncio.run(main())

On main:

before: b'# caf\xe9 note\nOLD\n'
result: {'status': 'ok', 'message': 'Edited notes.py'}
after : b'# caf\xef\xbf\xbd note\nNEW\n'

On this branch:

before: b'# caf\xe9 note\nOLD\n'
result: {'status': 'ok', 'message': 'Edited notes.py'}
after : b'# caf\xe9 note\nNEW\n'

With new_string="\ud800", main raises UnicodeEncodeError and leaves the file at
b''; this branch returns {'status': 'error', ...} and leaves the file unchanged.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

pre-commit run passes on both edited files, including ruff, isort, pyink,
codespell and the ADK compliance checks. The two pylint C0301 findings in
_edit_file_tool.py are 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:346 and
:369; 1cc298ed moved it into _edit_file_tool.py unchanged.

No overlap with the open PRs in this area. #7133 and #7176 touch
_read_file_tool.py alongside _base_environment.py and _local_environment.py,
but not _edit_file_tool.py.

_write_file_tool.py:83 passes a model-supplied str to write_file and has the
same truncate-before-encode exposure. I left it out to keep this PR to one concern
and can follow up separately.

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
1wos force-pushed the fix/editfile-preserve-non-utf8-bytes branch from b6acb37 to a92d1d5 Compare September 27, 2026 14:14
@iarjunganesh

Copy link
Copy Markdown
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EditFileTool permanently destroys non-UTF-8 bytes outside the edited region

3 participants