Repository navigation
Conversation
Detect file encoding per read (BOM, UTF-8 validity, then a lossless decode/encode round-trip among common legacy encodings) and write files back in the encoding they were detected with. Files whose encoding cannot be identified are refused for mutation instead of being corrupted, and reads keep the existing lossy UTF-8 behavior for those. Port of anomalyco#49881 to the v2 core tool architecture, rebased onto the Environment/Files + FileMutation services. New files are created as UTF-8; no configuration option is added.
github-actions
Bot
added
the
needs:compliance
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This works for GBK/GB18030 and UTF-16 files, but the detection doesn't hold up for the other encodings it claims to support. Writes into a legacy encoding can also lose characters without any error. Requesting changes.
I checked it by running the new and existing edit/write/patch/read/file-mutation tests (all pass) and bun run typecheck (clean). I also ran a script against Encoding.detect and encodeText with Shift_JIS, Big5, EUC-KR and Windows-1252 samples. Shift_JIS, Big5 and EUC-KR files are all detected as gb18030:
- Shift_JIS
こんにちは、世界was decoded as偙傫偵偪偼丄悽奅. - Big5
繁體中文測試was decoded as羉砰いゅ代刚. - EUC-KR
안녕하세요was decoded as救崇窍技夸.
Bytes the model doesn't touch do survive, so the claim that a misdetected file can never be corrupted only holds for those bytes. Any text the model adds to such a file is encoded as GB18030, which leaves a Shift_JIS/Big5/EUC-KR file with mixed encodings. That is arguably worse than today's behaviour, because the model can't even see the problem in the text it reads. The tests only cover GBK and UTF-16LE, so this isn't caught.
Public API: none of the SDK, server, plugin, config or CLI surface changes. The changes are internal to packages/core: FileMutation.writeTextPreservingBom is replaced by writeText (which takes a source), syncTextBom becomes syncTextEncoding, readText now returns the detected encoding fields, and iconv-lite is added as a runtime dependency. The edit, write, patch and read tool descriptions the model sees also change.
The concerns are in the inline comments, most important first: candidate detection, writes that silently turn characters into ?, and the separate whole-file read path for legacy encodings.
| // Tried in order after BOM and UTF-8 checks. gb18030 first: it round-trips | ||
| // nearly every CJK byte stream, including its GBK subset, which makes it the | ||
| // safest guess; a misdetected CJK variant still round-trips byte-exactly. | ||
| const CANDIDATES = ["gb18030", "big5", "shift_jis", "euc-kr", "windows-1252"] as const |
There was a problem hiding this comment.
With gb18030 first, big5, shift_jis and euc-kr are effectively unreachable. Their byte ranges mostly fall inside GB18030's valid double-byte range, so the lossless round-trip succeeds and detection stops there. I ran Shift_JIS こんにちは、世界, Big5 繁體中文測試 and EUC-KR 안녕하세요 through detect: all came back as gb18030 and decoded to CJK mojibake (偙傫偵偪偼丄悽奅, 羉砰いゅ代刚, 救崇窍技夸). A lossless round-trip shows that a decoding is possible, not that it is right. When the model then edits such a file, the text it adds is encoded as GB18030, which leaves the file with mixed encodings. It would need a real way to tell these encodings apart, such as frequency or statistical scoring, or a user/config hint. Failing that, limit the claim to GB18030 and refuse the others. Please also add Shift_JIS/Big5/EUC-KR cases to the tests.
| if (source.encoding === "utf-16le" || source.encoding === "utf-16be") | ||
| return Effect.succeed(encodeUtf16(next.text, source.encoding)) | ||
| if (source.encoding === "utf-8") return Effect.succeed(encodeUtf8(next.text, source.bom)) | ||
| return Effect.promise(() => loadIconv().then((mod) => mod.encode(next.text, source.encoding))) |
There was a problem hiding this comment.
iconv-lite quietly turns characters the target encoding can't hold into ?. Encoding 中文 é 😀 ✓ gives 中文 ? ? ? as shift_jis and ?? é ?? ? as windows-1252. So an edit or write that adds such characters (emoji, CJK in a Windows-1252 file, and so on) loses data without any error, which is the kind of silent corruption this change is meant to prevent. Please check that decode(encode(text)) === text, and fail the tool with a clear error if it doesn't.
| const buffer = Buffer.from(bytes) | ||
| // A truncated multi-byte sequence at the sample end can decode losslessly | ||
| // after dropping up to three trailing bytes, so retry before rejecting. | ||
| for (let cut = 0; cut <= 3; cut++) { |
There was a problem hiding this comment.
Dropping up to 3 trailing bytes makes sense when sampling the first chunk for read. But FileMutation.readText runs the same check on the whole file. A file whose last byte is a dangling lead byte is then marked valid, and decodeText turns that byte into U+FFFD. The next edit writes the replacement back, so the last bytes change. For mutations, detection should require the full buffer to round-trip.
| } | ||
| } | ||
|
|
||
| const detected = yield* Encoding.detect(first.bytes) |
There was a problem hiding this comment.
Two concerns with this branch. First, it duplicates the paging, line-splitting and offset logic of the existing text path rather than decoding into it, so the two paths will drift apart. Second, it loads the whole file and fails with MediaIngestLimitError once the file is over MAX_READ_BYTES * 4. Large legacy-encoded files could be read page by page before (lossily) and now can't be read at all. Detection here also uses only the first chunk, while the mutation tools detect on the whole file, so read and edit can disagree on the same file. Could the existing paged path decode with the detected encoding instead?
jschardet now leads detection so big5, shift_jis, and euc-kr stop collapsing into gb18030; the lossless round-trip only confirms the guess. Encoding verifies decode(encode(text)) === text and fails the tool when content is not representable in the file's encoding, whole-file detection no longer tolerates a dangling trailing lead byte, and reads page legacy encodings through the existing byte-rope pager instead of a capped whole-file branch (UTF-16 still decodes whole because its newline units span two bytes).
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
Two of my earlier points are fixed. Shift_JIS, Big5, EUC-KR and GBK samples are now detected correctly and decode cleanly. Unrepresentable content now fails with UnrepresentableError instead of being written as ?. I checked both with a script against Encoding. The read pager also handles legacy encodings now, without the size cap. The 8 nearest test files pass (195 tests) and bun run typecheck is clean.
Public API: unchanged. The changes stay inside packages/core, which now also depends on jschardet.
The dangling-byte fix doesn't reach the UTF-8 check. validUtf8 still ignores up to 3 trailing bytes even when checking the whole file. A GBK 注释\n and a Windows-1252 café\n file are both reported as valid UTF-8 and read as ע�� and caf�. Edit and write accept them instead of refusing, and the next edit writes the replacement characters back. Any legacy file with a non-ASCII character in its last three bytes is affected. See the inline comment.
The statistical guess can also pick the wrong single-byte code page: my Windows-1252 sample came back as windows-1250 (naïve read as naďve). Bytes still round-trip, but the model reads wrong characters, and a ď it writes back is stored as Windows-1250 bytes in a Windows-1252 file.
packages/core/src/encoding.ts:145:sampleonly affects the legacy candidates.validUtf8still ignores up to 3 trailing bytes when checking the whole file. GBK注释\n(D7 A2 CA CD 0A) and Windows-1252café\nboth come back as{ encoding: "utf-8", valid: true }and decode toע��andcaf�. Edit and write then accept the file and write U+FFFD back, which is exactly the refusal case this change is meant to catch.validUtf8should only tolerate truncation whenoptions.sampleis set.
| // Single-byte encodings round-trip almost any byte stream, so only the | ||
| // statistical label separates them; accept any iconv-supported label and let | ||
| // the round-trip gate confirm it. BOM-less UTF is never guessed here. | ||
| const SINGLE_BYTE = /^(windows-125\d|iso-8859-\d+|koi8-[ru]|ibm866)$/ |
There was a problem hiding this comment.
Taking any single-byte label from jschardet, confirmed only by a round-trip, lets the wrong code page through. Nearly every single-byte code page round-trips almost any input. A Windows-1252 Résumé: café naïve façade über straße came back as windows-1250 and reads as naďve. When the model then writes a character that differs between the two code pages, it is stored as Windows-1250 bytes in a Windows-1252 file. Consider accepting only a small, known set (e.g. windows-1252) or requiring a minimum jschardet confidence, and refusing otherwise.
jschardet cannot reliably tell the Latin code pages apart at low confidence (a windows-1252 file labeled windows-1250 read as mojibake and later writes would store wrong-code-page bytes). Collapse the Latin-script single-byte labels onto windows-1252 and refuse every other single-byte label instead of decoding through a wrong code page.
There was a problem hiding this comment.
The wrong-code-page case from my last review is fixed. I ran Windows-1252 Résumé: café naïve… through Encoding.detect and it is now detected as windows-1252 and reads correctly. Central European (Windows-1250 / ISO-8859-2), cp850 and Cyrillic files are now reported as invalid, so edit and write refuse them, which is safe. The edit/write/patch/read/file-mutation/encoding tests pass (196) and bun run typecheck is clean. The public API is unchanged.
The point about trailing bytes in validUtf8 is still open. validUtf8 still ignores up to 3 trailing bytes when checking the whole file. A GBK 注释\n file is still reported as valid UTF-8 and reads as ע��, so the next edit writes U+FFFD back.
New: mapping Turkish onto Windows-1252 gives wrong letters, see the inline comment.
| const SINGLE_BYTE_LABELS: Record<string, string> = { | ||
| "windows-1250": "windows-1252", | ||
| "windows-1252": "windows-1252", | ||
| "windows-1254": "windows-1252", |
There was a problem hiding this comment.
Mapping Turkish (windows-1254, and iso-8859-9 below) onto windows-1252 round-trips, but it gives the wrong letters. A Windows-1254 file şehir, ığdır, İstanbul reads as þehir, ýðdýr, Ýstanbul. Writes are protected by the unrepresentable check, but the model sees wrong text and any edit that adds ş/ğ/ı/İ fails. Since jschardet named a Turkish code page, refusing it (as you now do for Central European and Cyrillic code pages) would be safer than decoding it as Western.
A windows-1254 label means Turkish letters are present; decoding them as windows-1252 reads s/ğ/ı/İ as wrong Latin letters and edits adding them fail anyway, so refuse the label like the other language-specific code pages.
There was a problem hiding this comment.
The Turkish point is fixed: I checked that a Windows-1254 file is now refused rather than read as Western. Windows-1252 still reads correctly, and the edit/write/patch/read/encoding tests (196) and bun run typecheck pass. The public API is unchanged.
The point about trailing bytes in validUtf8 is still open. A short GBK file like 注释\n is still reported as valid UTF-8 (ע��), so the next edit writes U+FFFD back.
validUtf8 retried without up to 3 trailing bytes even for whole-file detection, so a windows-1252 cafe or GBK 注释 file whose bytes look UTF-8-ish after dropping the tail was accepted as UTF-8, decoded with replacement characters, and the next edit wrote them back. Whole-file detection now requires an exact UTF-8 decode (sampled prefixes still tolerate a cut sequence), reads detect strictly when the first chunk holds the whole file so read and edit agree, C1 control characters count against plausibility (they only appear through lossy mappings of undefined byte positions, e.g. shift_jis 0x80), and the iso-8859-3 label joins the western collapse onto windows-1252.
There was a problem hiding this comment.
The remaining points are addressed. Detecting a whole file no longer ignores trailing bytes, so short legacy files like GBK 注释\n and Windows-1252 café\n now detect and decode correctly instead of being taken as UTF-8. read now detects the same way as edit when the file fits in its first chunk. I re-checked GBK, Big5, Shift_JIS, Windows-1252, iso-8859-3 and plain UTF-8 samples against Encoding.detect. The edit/write/patch/read/encoding/file-mutation tests (197) and bun run typecheck pass.
The public API is unchanged. The changes are internal to packages/core, which gains the runtime dependencies iconv-lite and jschardet.
No further concerns.
Resolves the conflicts with the updates-map refactor: add and move hunks now register their encoding source alongside the updates entry, so a later hunk that skips re-reading still writes in the right encoding.
Issue for this PR
Port of #49881 to the V2 architecture, as requested when the original PR was closed ("please port it over and open it for V2").
Type of change
What does this PR do?
edit/write/patchpreviously decoded every file as UTF-8 with a non-fatal decoder and always wrote back UTF-8, which permanently corrupted files stored in legacy encodings (GBK, Big5, Shift_JIS, ...). This makes the file tools encoding-aware end to end:Encoding.detect(packages/core/src/encoding.ts) identifies the encoding per read: a BOM (UTF-8 / UTF-16LE / UTF-16BE) is honored first, valid UTF-8 is used as-is, and anything else is identified by a statistical guess (jschardet) that must be confirmed by a lossless decode/encode round-trip (plus a control-character plausibility gate). A confirmed encoding always reproduces the original bytes exactly, so mutations are byte-safe even in the rare case a CJK variant is misidentified.0x0Apage through the existing byte-rope pager — each selected byte range starts after a newline byte, so it is independently decodable. UTF-16 (BOM-marked) decodes whole because its newline units span two bytes. There is no size cap beyond the existing media one.edit/write/patchwrite back in the encoding the file was loaded with, so unchanged regions round-trip byte-exactly. New files are created as UTF-8.Encoding.UnrepresentableErrorinstead of iconv's silent?replacement, because encoding verifiesdecode(encode(text)) === text.iconv-liteandjschardetload lazily on first non-native use, so UTF-8/UTF-16 paths never pay for them. No configuration option: detection is fully automatic.Why this works: byte-safety never depends on the guess being right — a candidate is only accepted when re-encoding the decode reproduces the input bytes exactly, and writes verify their own round-trip. The guess only decides which correct-looking decoding the model sees.
How did you verify your code works?
packages/core/test/tool-encoding.test.ts(18 tests): detection units for BOM/UTF-8/GBK/Big5/Shift_JIS/EUC-KR/undecodable/dangling-tail, big5-vs-shift_jis-vs-euc-kr separation, unrepresentable-content failure, edit-in-place on GBK / Big5 / Shift_JIS / UTF-16LE files, refusal cases leaving bytes untouched, write-back preserving GBK, patch on GBK, and paged + whole reads of GBK and UTF-16LE files through the filesystem reader.packages/core/test/file-mutation.test.tsupdated for the newwriteTextAPI.bun run typecheckand the encoding / file-mutation / edit / write / patch / read suites pass. (Shell/session-shell suites fail on this Windows machine both with and without the change — pre-existing environment failures; CI on Linux is the source of truth for those.)Screenshots / recordings
Not a UI change.
Checklist