fix(StrReplaceFile): refuse to edit files that are not valid UTF-8 - #2595
fix(StrReplaceFile): refuse to edit files that are not valid UTF-8#2595shoemoney wants to merge 3 commits into
Conversation
StrReplaceFile decodes the whole file with errors="replace", applies the edit to the string, and writes the whole string back. Any byte in the file that is not valid UTF-8 — including bytes nowhere near the edit — comes back as U+FFFD and is written out as EF BF BD, so the file changes outside the requested edit and the approval diff cannot show it, because the diff is built from the already-lossy string. Detect the lossy decode and return a ToolError instead of writing. The check runs before the approval request, so a corrupting edit is never offered for approval in the first place. U+FFFD in the decoded text is only a symptom: the file may legitimately contain one. The raw bytes are re-read and strictly decoded to tell the two apart, and only when a U+FFFD is present, so a file with no U+FFFD — the overwhelming majority — still costs exactly one read as before. The strict decode is deliberate and is caught rather than propagated, so it cannot panic on malformed UTF-8, which is what the errors="replace" convention in tests_ai/test_encoding_error_handling.md exists to prevent. Fixes MoonshotAI#2591
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens StrReplaceFile to avoid silent file corruption by refusing to edit files that are not valid UTF-8, preventing lossy errors="replace" round-trips from rewriting undecodable bytes outside the requested edit.
Changes:
- Add a pre-approval guard in
StrReplaceFilethat detects lossy UTF-8 decoding and returns aToolErrorinstead of writing. - Add tests covering: refusal on invalid UTF-8 bytes, allowing a real U+FFFD character, and avoiding false positives on CRLF files.
- Add a changelog entry describing the behavior change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/kimi_cli/tools/file/replace.py |
Detects undecodable UTF-8 bytes before requesting approval; refuses the edit to prevent corruption. |
tests/tools/test_str_replace_file.py |
Adds regression tests for undecodable bytes, legitimate U+FFFD, and CRLF files. |
CHANGELOG.md |
Documents the new refusal behavior for StrReplaceFile on non-UTF-8 files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if "�" in content: | ||
| try: | ||
| (await p.read_bytes()).decode("utf-8") | ||
| except UnicodeDecodeError as decode_error: | ||
| return ToolError( | ||
| message=( | ||
| f"`{params.path}` is not valid UTF-8 " | ||
| f"(byte 0x{decode_error.object[decode_error.start]:02x} at offset " | ||
| f"{decode_error.start}). Editing it with StrReplaceFile would " | ||
| "replace that byte, and every other undecodable byte in the file, " | ||
| "with U+FFFD. No changes were made." | ||
| ), | ||
| brief="File is not valid UTF-8", | ||
| ) |
| original_content = "alpha\nbeta � gamma\ndelta\n" | ||
| await file_path.write_text(original_content) | ||
|
|
||
| result = await str_replace_file_tool( | ||
| Params(path=str(file_path), edit=Edit(old="alpha", new="ALPHA")) | ||
| ) | ||
|
|
||
| assert not result.is_error | ||
| assert await file_path.read_text() == "ALPHA\nbeta � gamma\ndelta\n" |
Matches the existing convention in tests/ui_and_conv/test_prompt_history.py:67 and test_prompt_placeholders.py:153, which both write the sentinel as an escape rather than a literal glyph. Binding read_bytes() to a name also makes it obvious the strict decode and the error message read the same buffer.
|
Thanks — both Copilot comments checked against the source. Adopted one, and the other does not reproduce. Adopted: Not adopted: the b"abc\xe2\x82".decode("utf-8")
# UnicodeDecodeError: ... invalid continuation byte / unexpected end of data
# e.start == 3, len(e.object) == 5 -> e.object[3] is fineI brute-forced it rather than argue from the docs: 47,883 distinct Happy to add a guard anyway if you would rather not depend on that, but it would be unreachable code and I would rather not add it silently. |
|
Worth recording here since it came up on #2591: this PR does not use a The gate here is narrower: it only looks at the raw bytes when U+FFFD is already present in the decoded text, and then rejects solely on Still happy to rewrite as direction 1 or the byte-level splice if a maintainer prefers. |
|
This has been open since Aug 6 and no CI has ever run on the head commit. The change makes |
Related Issue
Resolve #2591
Description
StrReplaceFiledecodes the whole file witherrors="replace", applies the edit to the string, and writes the whole string back. Any byte that is not valid UTF-8, including bytes nowhere near the edit, comes back as U+FFFD and is written out asEF BF BD. The file changes outside the requested edit, and the approval diff cannot show it because the diff is built from the already-lossy string.This PR detects the lossy decode and returns a
ToolErrorinstead of writing. The check runs before the approval request, so a corrupting edit is never offered for approval.This is option 2 from #2591, refuse the edit.
surrogateescapeon both ends needs theerrorstype inkaos.pathwidened and is an exception to the convention intests_ai/test_encoding_error_handling.md; byte-level splicing is a rewrite of how the tool applies edits. Refusing is small, cannot panic, and turns silent corruption into an error the user can see. The cost is that the tool now declines edits it currently performs by corrupting the file. If you would rather have option 1 or 3, say so and I will rewrite this.Two design notes:
errors="replace".content.encode() != raw_bytes.kaos.local.readtextopens withoutnewline="", so reads translate CRLF to LF, whilewritetextpassesnewline="". That comparison would reject every CRLF file.test_replace_allows_crlf_filelocks this down. The write normalizing those endings to LF is the separate bug in [Windows] StrReplaceFile silently converts entire file from CRLF to LF, forcing Agent to abandon native tools for Python workarounds #2191 and is untouched here.Not in this PR:
replace.py:170writes witherrors="replace", which the project's own rule says is not needed on writes and whichwrite.py:158does not do. After this guard it is a no-op, since the content is known to round-trip. Happy to drop it in a follow-up.Testing
Three tests added to
tests/tools/test_str_replace_file.py:test_replace_refuses_file_with_undecodable_bytesmain(the edit succeeds and the file grows by two bytes).test_replace_allows_file_containing_real_replacement_charactertest_replace_allows_crlf_fileFull suite: 2933 passed on this branch vs 2930 on
cbc15c07, with the same 26 pre-existing failures (tests/ui/test_usage.py,tests/utils/test_editor.py) before and after.ruff check,ruff format --check, andpyrightare clean.Checklist
## Unreleased;make gen-changelogshells out tokimiitself, which I did not want to run against your repo).make gen-docsto update the user documentation. No user-facing docs describe this behavior, so there was nothing to regenerate.