fix(agent-core-v2): unify text/binary classification for UTF-8 multibyte files - #2972
Conversation
🦋 Changeset detectedLatest commit: 5d35cca The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ca841694b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (cp === 9 || cp === 10 || cp === 13) continue; | ||
| if (cp < 32 || (cp >= 0x7f && cp <= 0x9f)) nonPrintable++; |
There was a problem hiding this comment.
When a NUL occurs after byte 511, sniffTextEncoding does not see it because its parity scan is limited to 512 bytes, and this loop merely counts it toward the 30% control-character threshold. Consequently, an otherwise ASCII 4 KiB sample containing a sparse NUL is classified as UTF-8 text, whereas the previous classifier treated any NUL as binary; WorkspaceFs read/download and kap-server fs:content can therefore expose such binary files as text. Preserve the unconditional NUL check across the full classification sample after ruling out UTF-16.
Useful? React with 👍 / 👎.
| for (let i = Math.max(0, sample.length - 3); i < sample.length; i++) { | ||
| const b = sample[i]!; | ||
| const expected = b < 0x80 ? 1 : b < 0xc2 ? 0 : b < 0xe0 ? 2 : b < 0xf0 ? 3 : 4; | ||
| if (expected > 0 && i + expected > sample.length) { | ||
| end = i; | ||
| break; |
There was a problem hiding this comment.
When the final three bytes contain any apparent multibyte lead whose nominal width extends past the sample, this code discards the entire suffix without validating the lead or the continuation bytes. Thus samples ending in an impossible UTF-8 lead such as 0xff, or a provably malformed suffix such as 0xe4 0x41, are classified as text and served as such by the WorkspaceFs and fs:content paths. Only trim a suffix when it is a valid prefix of a legal UTF-8 sequence; otherwise retain it so the fatal decoder rejects the sample.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
9ca8416 to
5d35cca
Compare
1 participant
Related Issue
None — the problem is explained below.
Problem
Opening a UTF-8 log/text file containing Chinese or emoji in the web file preview shows "Binary file · application/octet-stream · preview unavailable" instead of the text content.
Root cause: two independent classifiers disagreed. The byte-level binary heuristic counts every byte ≥ 0x7F as non-printable, so a UTF-8 sample full of multibyte characters easily crosses the 30% threshold and is deemed binary. The separate encoding detector only ran on the fs read path (and only helped UTF-16), so the preview (
fs:content) and download paths had no encoding awareness at all — and even utf-8 reads of such files were rejected withFS_IS_BINARY.What changed
detectBinaryanddetectTextEncodingare now thin derived views over that classifier, and the read path's manual "trust encoding detection over the binary heuristic" patch block is deleted — read,fs:content, and download now share a single conclusion.fs:content, download resolution) labeltext/plainonly when the sample classifies as UTF-8 text; UTF-16 files keep their previousapplication/octet-streambehavior on those routes since the raw bytes are served untranscoded.FS_IS_BINARY;is_binary=false,mime='text/plain'), download mime for the same file, andfs:contentserving a UTF-8 Chinese.logastext/plain.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.