Repository navigation
Conversation
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.
The fix works. I ran the #58354 reproduction from source with a simulated strict provider. On the base, every request after the shell call contains 003dsha HEAD\u0000multi_ack. With this change the stored tool result is 003dsha HEADmulti_ack and no request contains a NUL. The shell tool tests pass and tsc on packages/core is clean.
The public API is unchanged.
The fix only cleans tool results at the moment they are stored (see the inline comment), so a few ways a NUL reaches the provider remain:
- A session that already has a NUL in its history keeps failing on every request. That is the state the reporter's session is in.
- Output from user-run
!shell commands is replayed frommessage.output.output(session/runner/to-llm-message.ts, theshellcase) without going throughnormalizeContent. - The error text of a failed tool (
tool.state.error) isn't cleaned either.
Removing NULs at request time, next to sanitizeSurrogates in packages/ai/src/utils/sanitize.ts, would cover all of these and every provider in one place. It could replace this change or sit alongside it.
Related: #51351
| // Tool output is text content, not a byte channel: NUL bytes are rejected outright by some | ||
| // providers ("String must not contain null bytes"), which permanently bricks the session since | ||
| // the output is replayed from history on every later request. | ||
| const stripNul = (text: string) => text.replaceAll("\0", "") |
There was a problem hiding this comment.
This only cleans tool results when they are first stored. History is replayed from storage as-is, so a session that already has a NUL (like the one in #58354) still fails on every request. User ! shell output (to-llm-message.ts, shell case) and tool error messages don't go through normalizeContent at all. A NUL pass in the request-time sanitizer (packages/ai/src/utils/sanitize.ts, which already walks every request string for lone surrogates) would cover all of these and all providers.
There was a problem hiding this comment.
Agreed, and thanks — this covers all the entry paths you listed and fixes the reporter's already-poisoned session too, which store-time cleaning can never do. Stripping now happens in sanitizeSurrogates (packages/ai/src/utils/sanitize.ts) next to the surrogate pass, and the store-time change is reverted. New request-time test in packages/ai/test/sanitize.test.ts; commit 777da50.
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.
The new commit only merges the latest dev. The fix itself is unchanged, and the #58354 reproduction still shows no NUL in any request on the new head. The open point about cleaning NULs only when tool output is stored is not addressed yet: sessions that already contain a NUL, user ! shell output and tool error text are still sent unchanged. The public API is unchanged.
0d5f4e2 to
777da50
Compare
nik875
changed the title
There was a problem hiding this comment.
The new commit addresses my earlier point. NULs are now removed in the request-time sanitizer, which runs on every outgoing request (route/client.ts, route/media.ts). That covers replayed history, so sessions that already contain a NUL work again. It also covers user ! shell output, tool error text and every provider.
I re-ran the #58354 reproduction: the base still sends the NUL on every later request, and with this change no request contains one. The packages/ai tests pass and tsc is clean. The public API is unchanged. I have no further concerns.
|
Took your suggestion in the force-push ( |
|
Reviewing from #58384 (my two-site capture/relay fix of the same issue, closed in favor of this): This is the right layer — One thing worth confirming (it held in my investigation of the same bug): the sanitizer runs on the outgoing request only, so the stored session history keeps its NUL bytes and every request re-sanitizes — that's fine for providers, but Also +1 to the escaped |
|
Tested this fix on Windows 11 x64 running OpenCode 2.1.4 against Venice AI ( Can confirm that stripping NUL bytes in A complementary observation regarding @OrangerXu's point on stored session history: Having both (store-time sanitization for clean persistence + |
|
Looking forward to having this fixed! 🥳 |
Issue for this PR
Closes #58354
Type of change
What does this PR do?
Shell output containing a NUL byte gets stored in session history and replayed to the provider on every later request. Venice's OpenAI-compatible endpoint rejects such strings with a 400 ("String must not contain null bytes"), and since the byte is in history, every retry rebuilds the same invalid request — the session can't recover.
Stripping NULs in
sanitizeSurrogates(packages/ai/src/utils/sanitize.ts), which already walks every outgoing request string for lone surrogates, covers every entry path — stored history, user!shell output, failed-tool error text — and every provider in one place, and it also un-bricks sessions whose history already contains a NUL byte. Strip rather than escape, since escaping would inject text the tool never printed; if a NUL byte matters for the task, it's easy to check for explicitly (grep -c,od), in the same spirit as the read tool refusing binary files. Strings without NUL bytes are returned untouched.How did you verify your code works?
New test in
packages/ai/test/sanitize.test.tscovering user text and tool output containing NUL (fails without the fix). Fullpackages/aisuite passes andbun run checkis clean.Screenshots / recordings
n/a
Checklist
Added by the opencode bot (AI-generated).
Related: #51351