fix(sandbox): keep split UTF-8 characters intact across PTY output windows - #4724
fix(sandbox): keep split UTF-8 characters intact across PTY output windows#4724ayaangazali wants to merge 4 commits into
Conversation
…ndows collect_pty_output decodes each window with errors="replace". PTY output is collected in repeated windows over one persistent chunk deque, so a multi-byte character whose bytes land either side of a window boundary is decoded as two partial sequences and both halves become U+FFFD. The original bytes are destroyed at that first decode, so no later window can recover them. Hold an unfinished trailing sequence back on the deque for the next window instead. Once is_done() reports the producer has closed, nothing can complete the sequence, so the existing replacement behaviour still applies there. The helper is shared by the unix_local, docker, blaxel, daytona and modal backends, so all of them lose the character today.
![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: a9e76be54c
ℹ️ 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".
| byte = data[-back] | ||
| if byte < 0x80: | ||
| return 0 | ||
| if byte >= 0xC0: |
There was a problem hiding this comment.
When a running PTY's current output ends with an invalid UTF-8 byte such as 0xC0, 0xC1, or 0xF5–0xFF, this condition misclassifies it as the start of an incomplete character. The collector therefore requeues the byte and returns no replacement character; repeated polls keep requeuing it indefinitely until the process emits more output or exits. This regresses the existing errors="replace" handling for arbitrary PTY bytes, particularly for an interactive process that prints a non-UTF-8 prompt and then waits for input. Only valid lead-byte ranges (C2–DF, E0–EF, and F0–F4) should be eligible for carry-over.
Useful? React with 👍 / 👎.
The carry-over check treated any byte at or above 0xC0 as a lead byte, but 0xC0, 0xC1 and 0xF5 to 0xFF never start a valid sequence. A running PTY that emitted one of those as its last byte had it requeued every poll and never replaced, so an interactive process printing a non-UTF-8 prompt and then waiting for input withheld that byte indefinitely. Restrict carry-over to the real lead ranges C2 to DF, E0 to EF and F0 to F4. Everything else falls through to the existing replacement behaviour.
|
Good catch, this was a real regression in my change and it is fixed in You are right that Your interactive-prompt example is the case that made this matter: the byte was withheld while the process sat waiting for input, so the prompt never reached the caller at all. That is worse than the replacement character it was meant to avoid. Carry-over is now restricted to Full verification stack passes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb683f3199
ℹ️ 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".
| else: | ||
| return 0 |
There was a problem hiding this comment.
When a collection window ends after one or more continuation bytes—for example, a three-byte character split as E2 82 | AC—the scan examines 0x82 first and immediately returns zero because it is not a lead byte. The first window therefore decodes E2 82 as U+FFFD and the next decodes AC separately, so this fix still corrupts three- and four-byte characters at most possible split points. The scan needs to traverse trailing continuation bytes back to their valid lead byte before deciding how much to carry.
Useful? React with 👍 / 👎.
… byte The previous commit returned zero as soon as the scan saw a byte that was not a lead byte, but continuation bytes are exactly what a split three or four byte character leaves at the end of a window. A character split as E2 82 | AC was therefore still decoded as two replacement characters, so only two byte characters were actually protected. Skip continuation bytes and keep walking back to the lead byte, scanning up to four bytes since that is the longest sequence. Four trailing bytes with no lead byte cannot be completed, so they fall through to replacement as before. The helper is now covered directly across every split position of every sequence length, which is what the earlier window level test missed.
|
Right again, and this one was worse than it looked: my previous commit only ever protected two-byte characters. The scan returned zero the moment it saw a byte that was not a lead byte, and a continuation byte is exactly what a split three or four byte character leaves at the end of a window. So Fixed in The real lesson is that my test only covered a two-byte character split after one byte, which is the single case that happened to work. The helper is now tested directly across every split position of every sequence length: Plus a window-level test parametrized over both split points of a three-byte character. On the previous commit the Full verification stack passes. One unrelated flake, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65a333b3df
ℹ️ 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".
| elif 0xE0 <= byte <= 0xEF: | ||
| needed = 3 | ||
| elif 0xF0 <= byte <= 0xF4: | ||
| needed = 4 |
There was a problem hiding this comment.
The revised range checks reject bad lead bytes, but they still carry prefixes that can never become valid, such as E0 80, ED A0, F0 80, or F4 90; UTF-8 restricts the second byte for these lead bytes. If an interactive process prints one of these malformed prefixes and then waits for input, every poll drains and requeues the same bytes without emitting the existing U+FFFD replacements. Validate the restricted second-byte ranges before classifying the suffix as incomplete.
Useful? React with 👍 / 👎.
| async with output_lock: | ||
| output_chunks.appendleft(tail) |
There was a problem hiding this comment.
When a Unix child has exited but _watch_process_exit is still awaiting its pump tasks before setting output_closed (unix_local.py:503-508), this code treats a truncated final sequence as resumable and moves it back into the deque. _finalize_pty_update then removes the entry as soon as process.returncode is set (unix_local.py:554-563), so no next window can drain the tail and the final byte is silently lost instead of producing U+FFFD. Coordinate the carry with completion and entry ownership at this mutation boundary, with controlled exit/pump ordering coverage.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
Summary
collect_pty_outputdecodes each collection window witherrors="replace":PTY output is not collected once. It is collected in repeated windows over one persistent chunk deque:
pty_exec_starttakes a window, then eachpty_write_stdintakes another from the sameentry.output_chunks. So a multi-byte character whose bytes land either side of a window boundary is decoded as two separate partial sequences, and both halves become U+FFFD. The bytes are destroyed at that first decode, so no later window can put the character back together.Reproduced against
mainwith one persistent deque, the producer still running for the first window:After the change the same sequence round trips, with the first window ending on
b"h"and holding the partial byte back:The fix holds an unfinished trailing sequence back on the deque for the next window. It needs no new state, because the deque already outlives a single window and is owned by the PTY entry. When
is_done()reports the producer has closed, nothing can complete the sequence, so the existing replacement behaviour is kept for that case.This is not backend-specific.
collect_pty_outputis the shared helper behind theunix_local,docker,blaxel,daytonaandmodalPTY paths, so any of them silently mangles a character that straddles a window today. It is the same class as #4707, which fixed a UTF-8 boundary in the Cloudflare SSE reader; this is the boundary that the shared PTY collector has.Note on overlap: #4572 also edits this function, adding a final drain immediately above the decode line. The two changes are independent and compose, but they will touch adjacent lines, so this may need a trivial rebase depending on merge order.
Test plan
Two tests in
tests/sandbox/test_pty_output.py:test_collect_pty_output_holds_split_utf8_character_for_the_next_windowsplits a two-byte character across two windows on one deque, asserts the first window ends onb"h"rather than emitting a replacement character, and asserts the two windows concatenate back to the original text.test_collect_pty_output_replaces_partial_utf8_once_the_producer_is_donepins the deliberate exception: afteris_done()is true a truncated sequence still becomes U+FFFD and is not left stranded on the deque.Verified the first fails without the source change by reverting
src/: it fails on theb"h"assertion, which is the corruption itself. The second passes either way, since it pins existing behaviour..agents/skills/code-change-verification/scripts/run.shpasses end to end: format, lint, typecheck and the full suite.Issue number
None. Found while sibling-checking #4707.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRI went looking for this straight after #4707 landed, on the theory that a UTF-8 boundary bug in one reader usually has a sibling wherever else the SDK decodes bytes it did not accumulate itself. The judgement call I would most like checked is keeping
errors="replace"once the producer is done, since holding bytes back forever would be worse than losing them. I'm a freshman in college and the PTY collector is a path I only learned recently, so please push back if the carry-back belongs in the backends rather than the shared helper.