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. |
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 in the file 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's length and contents change 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 in the first place.Why this direction
#2591 laid out three options. This is option 2 — refuse the edit. @ayaangazali independently confirmed the bug at
cbc15c07and narrowed it to the same two candidates, so to be explicit about why this one:surrogateescapeon both ends (lossless) needserrorswidened inkaos.path, where it is typedLiteral["strict", "ignore", "replace"], and is an exception to the documented convention intests_ai/test_encoding_error_handling.md. It also puts lone surrogates intobuild_diff_blocksand out to the display layer, which is the failure mode UnicodeEncodeError - Surrogates Not Allowed When Writing History #420 already hit. That is a maintainer's call, not a drive-by PR's.If you would rather have option 1 or 3, say so and I will rewrite this.
Two things I checked so a later change does not undo them
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 an original U+FFFD from a failed decode. That second read only happens when a U+FFFD is present, so a file without one — the overwhelming majority — still costs exactly one read, as before.
Detection cannot be a byte comparison against the decoded text.
kaos.local.readtextopens withoutnewline="", so reads translate CRLF to LF, whilewritetextpassesnewline="". Any check of the formcontent.encode() != raw_bytestherefore rejects every Windows-line-ending file.test_replace_allows_crlf_filelocks that down. (That the write then normalizes those endings to LF is the separate bug in #2191, and is deliberately untouched here.)The strict decode is deliberate and is caught, not propagated, so it cannot panic on malformed UTF-8 — which is what the
errors="replace"convention exists to prevent. The read that produces the content the tool actually uses still specifieserrors="replace", so the convention holds as written.Not in this PR
replace.py:170writes witherrors="replace", which is inconsistent with the project's own rule ("Writing files and encoding Python strings to bytes do not requireerrors="replace"") and with the sibling tool atwrite.py:158. After this guard it is provably a no-op, since the content is now known to round-trip. I left it alone to keep this to one concern — happy to drop it in a follow-up if you want it gone.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 with this branch vs 2930 passed on
cbc15c07— the same 26 pre-existing failures (tests/ui/test_usage.py,tests/utils/test_editor.py) before and after, exactly +3 from this PR.ruff check,ruff format --check, andpyrightare all 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.