Skip to content

fix(environment): prevent truncation on invalid file content - #7319

Open
iarjunganesh wants to merge 1 commit into
google:mainfrom
iarjunganesh:fix/local-write-validate-before-open
Open

iarjunganesh wants to merge 1 commit into
google:mainfrom
iarjunganesh:fix/local-write-validate-before-open

Conversation

@iarjunganesh

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

Related: #7316 addresses a separate EditFileTool decode/write path. This PR
fixes the LocalEnvironment write path used by WriteFileTool and other callers.

Problem: LocalEnvironment._sync_write opens an existing file before Python
encodes model-supplied text as UTF-8. A lone surrogate raises
UnicodeEncodeError after the file has been truncated. An unsupported content
type can similarly fail after the binary file is opened. WriteFileTool reports
an error, but the original file bytes are already gone.

Reproduction: Write b'keep these bytes\n' to an existing file, then call
write_file with "replacement \ud800". Before the fix, the call raises and
the file reads back as b''. The new regression test fails this way on main.

Expected behavior: Invalid write content reports an error without modifying
the existing file.

Solution: Encode text and validate the str | bytes content type before
opening the target. Write the prepared bytes in binary mode. Existing tests
cover text, raw bytes, and explicit CRLF sequences.

Testing Plan

Unit tests:

  • Added tests for unencodable text and an unsupported content type. Both assert
    that existing file bytes remain unchanged.
  • python -m pytest tests/unittests/environment/test_local_environment.py tests/unittests/tools/environment/test_edit_file_tool.py -q — 18 passed, 2 skipped.
  • Full tox -e py312 suite, run serially on the same code tree: 16,235 passed,
    85 skipped, 26 xfailed, 2 xpassed.
  • Also attempted tox -p 5 across Python 3.10–3.14. Each version passed more
    than 16,200 tests, but that parallel run was not green for unrelated test
    environment reasons described below.

Manual end-to-end test:

Called WriteFileTool.run_async against a real LocalEnvironment containing
b'original\n', with content "bad\ud800". The tool returned status: error
and the file still contained b'original\n'.

Checks: Applicable pre-commit hooks passed. The Windows runner could not
launch the bash-only check-new-py-prefix wrapper (/bin/bash absent); its
underlying python scripts/check_new_py_files.py --new-dir . check passed.

The initial temporary Linux archive gave check_new_py_files.sh CRLF endings
even though its Git blob is LF. Its two shell-wrapper tests passed in all five
Python versions after restoring the exact Git blob. Parallel contention caused
intermittent deployment-test failures; the complete deployment test file then
passed serially in Python 3.10, 3.11, and 3.13 (68 tests in each). Ubuntu's
system Python 3.14 preloads sitecustomize, causing two import allowlist test
failures; the full import-loading file passed with uv-managed Python 3.14.7
(15 tests).

Checklist

Validate content type and encode text before opening the target so invalid model output cannot truncate an existing file.

Signed-off-by: Arjun Ganesh <iarjunganesh@gmail.com>
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.

2 participants