StrReplaceFile corrupts undecodable bytes outside the edited region

[shoemoney]

Summary

StrReplaceFile decodes the whole file with errors="replace", edits the string, and writes the whole string back. Any byte in the file that isn't valid UTF-8 — anywhere, including far from the edit — is replaced by U+FFFD and written to disk as EF BF BD. The file's length and contents change outside the requested edit, permanently, and the approval diff can't show it because the diff is computed from the already-lossy string.

This is the same root cause as #2191 / #1952 (whole-file round-trip silently rewriting content the edit never touched), but for undecodable bytes rather than line endings.

Where

src/kimi_cli/tools/file/replace.py

# line 132
content = await p.read_text(errors="replace")
...
# line 170
await p.write_text(content, errors="replace")

Reproduction

A file with a single invalid byte, unrelated to the edit:

orig = b"alpha\nbeta \xff gamma\ndelta\n"        # 25 bytes
content = orig.decode("utf-8", errors="replace")
content = content.replace("alpha", "ALPHA")      # the requested edit
out = content.encode("utf-8")

before: b'beta \xff gamma'      25 bytes
after:  b'beta \xef\xbf\xbd gamma'   27 bytes

The \xff is gone and the file grew by two bytes, on an edit that only asked to touch alpha.

One thing worth flagging before anyone fixes this

Dropping errors="replace" from the write at line 170 does not fix it. I checked — U+FFFD is a perfectly valid character to encode, so it still writes EF BF BD:

read=replace  write=replace  ->  b'beta \xef\xbf\xbd gamma'   25->27
read=replace  write=strict   ->  b'beta \xef\xbf\xbd gamma'   25->27   (identical)

The loss happens at the read. That said, line 170 does look inconsistent with the project's own rule in tests_ai/test_encoding_error_handling.md — "Writing files and encoding Python strings to bytes do not require errors="replace"" — and with the sibling tool, write.py:158, which writes without it. Worth tidying either way, just not as the fix.

write.py can't hit this bug: it reads with errors="replace" only to build a display diff, and writes params.content (fresh model-supplied text). StrReplaceFile is the only file tool that reads, edits, and writes back the same content.

Possible directions

I didn't open a PR because the sensible fix is a design decision that's yours to make, and each option has a real cost:

1. surrogateescape on both ends — byte-exact round-trip (verified: 25 → 25 bytes, \xff preserved). But lone surrogates raise UnicodeEncodeError when the string is later encoded, which this codebase has already been bitten by once in UnicodeEncodeError - Surrogates Not Allowed When Writing History #420. The edited content flows into build_diff_blocks and the approval display, so that risk is live here. It would also need errors widening in kaos.path, where it's typed Literal["strict", "ignore", "replace"].

2. Refuse the edit — read strict, and return a ToolError when the file isn't valid UTF-8. Small, can't crash, and arguably right for a text-editing tool. Cost: it declines edits the tool currently performs (badly).

3. Splice at the byte level — apply the replacement to the original bytes and only rewrite the changed span. Most correct, most work, and would also fix the CRLF class in [Windows] StrReplaceFile silently converts entire file from CRLF to LF, forcing Agent to abandon native tools for Python workarounds #2191 / fix(file): preserve CRLF during file edits #1953.

Happy to send a PR for whichever direction you'd prefer.

[ayaangazali]

Confirmed at HEAD (cbc15c0): replace.py:132 reads with errors="replace" and :170 writes the whole string back, so the round trip is lossy for any undecodable byte in the file. Your point that the loss happens at the read, and that touching line 170 alone changes nothing, matches what I see.

One thing that might matter before picking a fix: the obvious lossless approach, reading and writing with errors="surrogateescape", would contradict this repo's own written rule. tests_ai/test_encoding_error_handling.md says that for everything under src/kimi_cli/tools/ except load_desc, decoding user-provided content must specify errors="replace". So surrogateescape is not just a code change here, it is an exception to a documented convention, which probably needs a maintainer to sign off rather than a drive-by PR.

As far as I can tell that leaves roughly two directions:

1. Allow surrogateescape for this specific read/write pair and carve out an exception in the convention doc. Lossless, but lone surrogates then flow into build_diff_blocks and out to the wire/display layer, so that path would need checking.
2. Keep the decode as is, but detect that the file did not round trip (attempt a strict decode of the bytes) and refuse the edit with a clear error instead of writing a corrupted file. Not lossless, but it turns silent permanent corruption into something the user can see and act on.

Happy to implement either one if a maintainer says which they prefer. I did not want to guess at the convention exception myself.

[shoemoney]

Thanks for confirming at HEAD, @ayaangazali — and for spotting that the convention doc makes surrogateescape a maintainer decision rather than a drive-by. That narrowed it usefully.

I've sent #2595 for direction 2, since it's the one that needs no exception carved out: detect that the file didn't round trip and refuse, before the approval request, so a corrupting edit is never even offered for approval. If a maintainer would rather have direction 1 or the byte-level splice, I'll rewrite it — this was the version that could be reviewed today rather than after a convention call.

Two findings from writing it that are worth having in the thread either way:

Detection can't be a byte comparison. kaos.local.readtext opens without newline="", so reads translate CRLF to LF, while writetext passes newline="". Any check shaped like content.encode() != raw_bytes rejects every Windows-line-ending file. There's a test pinning that down.

U+FFFD in the decoded text isn't sufficient evidence — the file may legitimately contain one, so the raw bytes need a strict decode to tell an original from a failed one. Doing that only when a U+FFFD is present keeps the common path at one read.

Also worth noting the CRLF translation above is the mechanism behind #2191: reads normalize line endings, writes don't, so the same whole-file round trip rewrites them. Same root cause as this bug, different lossy step. A byte-level splice would close both at once, which is the argument for direction 3 whenever someone has the appetite for it.

[ayaangazali]

Confirmed the newline asymmetry independently, since your detection logic depends on it: LocalKaos.readtext opens with aiofiles.open(local_path, encoding=encoding, errors=errors) (packages/kaos/src/kaos/local.py:124), so universal newlines apply and CRLF becomes LF on read. writetext passes newline="" (local.py:155), so nothing is translated back. A content.encode() != raw check would indeed reject every CRLF file.

Not opening a competing PR for direction 2 since #2595 covers it. Deferring to a maintainer on whether the byte-level splice is worth it instead.