MoonshotAI / MoonshotAI/kimi-cli

StrReplaceFile corrupts undecodable bytes outside the edited region

Open
#2,591 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
11.4k
Forks
1.3k
Avg merge
9h 47m
Merged PRs (30d)
2

Description

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 #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 #2191 / #1953.

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/kimi_cli/tools/file/replace.py, then read tests_ai/tests_ai/test_encoding_error_handling.md and the sibling write.py implementation. Compare the listed handling options and trace how content reaches build_diff_blocks and approval display; done means a regression test covers an unrelated undecodable byte without silently rewriting it, with the chosen behavior documented and verified.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.