Repository navigation
Conversation
github-actions
Bot
added
needs:compliance
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
github-actions
Bot
removed
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not ready: the fix only covers one of three read paths, and it breaks reading valid UTF-8 files larger than 256KB.
How I checked: a temporary test calling ReadToolFileSystem.read on real files, on base and PR.
- A small file (under 50KB) with invalid bytes and no offset/limit now gives
BinaryFileError, as intended. - The same file read with
limit, or any file between 50KB and 256KB, still comes back with U+FFFD. Those reads go throughtextPage's lossy decoder, which the PR doesn't touch. - A valid 400KB UTF-8 file whose 256KB read boundary splits a multi-byte character now fails with
BinaryFileErrorwhen read with an offset. On base it reads correctly.
The existing test reads malformed UTF-8 lossily and still rejects null-byte binary content in test/tool-read-filesystem.test.ts now fails. It shows that lossy decoding is deliberate today. Rejecting these files instead means some files that read today (Latin-1 source, files with a stray bad byte) would fail completely. That is a behaviour change a maintainer should agree to first, and the test needs updating to match. Typecheck passes.
The mime.ts change is the same as #55946. In Read it has no effect, because Mime.detect there only picks out images and PDFs. I'd drop it here and leave it to that PR.
Public API: unchanged.
| const length = Math.min(leaf.summary.bytes, consumed - checked) | ||
| checked += leaf.summary.bytes | ||
| return length > 0 && leaf.bytes.subarray(0, length).includes(0) | ||
| if (length > 0 && leaf.bytes.subarray(0, length).includes(0)) return true |
There was a problem hiding this comment.
Leaves are raw 256KB reads, so a leaf boundary can split a multi-byte character. Decoding each leaf on its own with fatal: true then throws for valid UTF-8. I reproduced this: a valid 400KB file of é lines, shifted one byte and read with offset: 3000, returns BinaryFileError here, but reads fine on base. If invalid UTF-8 should count as binary, the check should decode the concatenated bytes (e.g. one streaming fatal decoder fed each leaf with stream: true), not each leaf on its own.
| const paged = first.info.size > MAX_READ_BYTES || page.offset !== undefined || page.limit !== undefined | ||
| if (!paged) { | ||
| if (first.bytes.includes(0)) return yield* new BinaryFileError({ resource }) | ||
| let decoded: string |
There was a problem hiding this comment.
This only covers the small-file path with no offset/limit. Any read with offset/limit, and any file between 50KB and 256KB, goes through textPage(), which still uses new TextDecoder() and returns U+FFFD. I checked both cases on this branch. The behaviour should be the same across read paths. Also, the existing test reads malformed UTF-8 lossily... asserts the lossy behaviour on purpose and now fails, so this needs a maintainer decision and an updated test.
Issue for this PR
Closes #57837
Type of change
What does this PR do?
The Read tool's non-paged path decoded file bytes with a non-fatal TextDecoder, silently replacing invalid UTF-8 sequences with U+FFFD and losing the original bytes. Also, Mime.detect's isText() streamed a chunk without flushing, so truncated trailing multi-byte UTF-8 sequences were accepted as valid text instead of being classified as binary. Both now use a fatal decoder (and isText flushes it), so invalid/truncated UTF-8 is treated as binary rather than corrupting the bytes the model sees.
How did you verify your code works?
Reviewed the changed decode paths; mime/read tests unchanged. Local sanity check of invalid-UTF-8 input now returns binary.
Screenshots / recordings
N/A
Checklist