fix(tools): preserve non-UTF-8 bytes in StrReplaceFile edits - #2594
fix(tools): preserve non-UTF-8 bytes in StrReplaceFile edits#2594686f6c61 wants to merge 4 commits into
Conversation
StrReplaceFile used read_text(errors=replace) and write_text, so any invalid UTF-8 sequence anywhere in the file became U+FFFD (EF BF BD) even when the edit only touched valid text elsewhere (MoonshotAI#2591). Apply old/new as UTF-8 byte substrings on the raw buffer and write_bytes so unrelated regions are bit- identical. Fixes MoonshotAI#2591
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Read raw bytes so non-UTF-8 sequences outside the edit are preserved | ||
| # (#2591 / same whole-file rewrite class as #2191). | ||
| raw = await p.read_bytes() | ||
| original_raw = raw | ||
| edits = [params.edit] if isinstance(params.edit, Edit) else params.edit | ||
|
|
||
| # Apply all edits | ||
| for edit in edits: | ||
| content = self._apply_edit(content, edit) | ||
| raw = self._apply_edit_bytes(raw, edit) |
There was a problem hiding this comment.
🔴 Edits spanning multiple lines stop working on Windows-style text files
The file's stored line endings are now matched literally against the model-supplied text (p.read_bytes() at src/kimi_cli/tools/file/replace.py:153) instead of being normalized first, so any edit spanning more than one line in a file saved with Windows line endings never matches and the edit is rejected.
Impact: Users editing CRLF files (common on Windows or in repos with CRLF checkouts) get "No replacements were made" for every multi-line edit.
Newline normalization lost by switching from text read to raw byte read
Previously the file was read with p.read_text(errors="replace"), which goes through kaos.readtext (packages/kaos/src/kaos/local.py:116-125) using Python's default universal-newlines mode, so \r\n in the file became \n in memory and a model-provided old containing \n matched.
The model only ever sees \n, because ReadFile also iterates lines in text mode (src/kimi_cli/tools/file/read.py:181 uses p.read_lines(...), universal newlines).
Now raw = await p.read_bytes() keeps \r\n, and _apply_edit_bytes encodes old as UTF-8 (src/kimi_cli/tools/file/replace.py:97) and searches the raw stream, so b"foo\nbar" cannot match b"foo\r\nbar"; the tool returns the "No replacements were made" error at src/kimi_cli/tools/file/replace.py:160-165.
The same literal-byte matching also breaks edits containing non-ASCII characters in files stored in a non-UTF-8 encoding (e.g. GBK/latin-1), which the old lossy-decode path could at least partially match.
A fix would be to detect the file's dominant line ending (and/or try a CRLF-normalized variant of old/new) before doing the byte-level search, while still writing back raw bytes.
Prompt for agents
StrReplaceFile now reads the file with read_bytes and matches the model-supplied `old` string as raw UTF-8 bytes (src/kimi_cli/tools/file/replace.py, _apply_edit_bytes). Previously it read with read_text, which performs universal-newline translation, so CRLF files were seen as LF and multi-line `old` strings containing \n matched. ReadFile also exposes file content to the model with universal newlines (src/kimi_cli/tools/file/read.py), so the model always emits \n. As a result, any multi-line edit against a CRLF-terminated file now fails with 'No replacements were made'. Consider detecting the file's line-ending style from the raw bytes and translating `old`/`new` accordingly (e.g. converting \n to \r\n when the target region/file uses CRLF), or falling back to a CRLF-normalized search when the literal byte search finds nothing, while still writing raw bytes so non-UTF-8 regions are preserved.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 473b141: StrReplaceFile now detects the file's dominant line ending (CRLF if any CRLF is present) and rewrites model-supplied old/new (always LF) to match before the byte search, while still writing raw bytes so non-UTF-8 regions stay intact. Added regression tests for multi-line CRLF edits and CRLF + invalid UTF-8.
Byte-level matching preserved non-UTF-8 regions but broke multi-line edits on CRLF files because the model always supplies LF. Detect the file's dominant line ending and rewrite old/new before the byte search.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates StrReplaceFile to perform replacements on raw bytes (instead of decoding/re-encoding the whole file), preserving invalid UTF-8 regions while also supporting multi-line edits in CRLF files.
Changes:
- Apply edits using byte-level search/replace to avoid corrupting non-UTF-8 sequences outside the edited region.
- Detect file line endings and rewrite model-supplied
\nto match on-disk CRLF when performing byte matching. - Add regression tests covering invalid UTF-8 preservation and CRLF multi-line replacements.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/tools/test_str_replace_file.py | Adds regression tests for invalid UTF-8 preservation and CRLF multi-line edit behavior. |
| src/kimi_cli/tools/file/replace.py | Switches replacement logic to byte-based edits and adds newline detection/encoding helpers. |
Suppressed comments (1)
src/kimi_cli/tools/file/replace.py:1
- The docstring says “dominant newline bytes”, but the implementation returns CRLF if any
\\r\\nexists. For mixed-newline files (mostly LF with a stray CRLF), this can mis-encodeold/new(rewriting\\n→\\r\\n) and cause legitimate edits to fail to match. Consider implementing true dominance (e.g., count CRLF vs. lone LF) or update the docstring/behavior to be explicit and consistent.
from collections.abc import Callable
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| p = p.canonical() | ||
|
|
||
| plan_target = inspect_plan_edit_target( | ||
| p, | ||
| plan_mode_checker=self._plan_mode_checker, |
There was a problem hiding this comment.
Fixed in 4d385ef: empty old is rejected with a clear ToolError before the byte path runs, with a regression test (test_replace_rejects_empty_old_string).
|
Addressed the empty- |
Byte-path search no-ops on an empty needle; fail explicitly with a clear ToolError instead of the generic "no replacements" path.
Detect CRLF vs LF by occurrence count instead of "any CRLF wins", so a mostly-LF file with a stray CRLF still matches model-supplied multi-line edits. Addresses Copilot feedback on mixed-newline files.
|
Follow-ups from review:
|
Summary
StrReplaceFiledecoded the whole file witherrors="replace", applied the edit as a string, and re-encoded — so any invalid UTF-8 sequence outside the edit became U+FFFD (EF BF BD) and permanently corrupted the file.old/newas UTF-8 byte substrings on the raw buffer andwrite_bytesso unrelated regions stay bit-identical.Fixes #2591
Test plan
pytest tests/tools/test_str_replace_file.py(16 passed), including:\xffbyte preserved beside a text edit