fix: contain and patch the vt100 row-erase panic#75
Merged
Conversation
#73) vt100 0.16.2 has a real upstream bug (doy/vt100-rust#28, open PR #30): Grid::set_size shrinks rows via Row::resize, which doesn't clean up a double-width character's "wide" flag when its continuation cell gets truncated away. The orphaned wide cell then panics the next time it's erased (Row::clear_wide indexes one past the row's new length) — this matches the exact panic signature and line from the reported incidents. hcom's existing global panic hook logs the panic but doesn't stop it from unwinding, so it killed the whole PTY wrapper thread and dropped the wrapped agent session (observed as repeated "stopped by pty: closed" on real Codex sessions). Wrap the two vt100 entry points (Parser::process, Screen::set_size) in catch_unwind, and rebuild the parser from scratch on a caught panic rather than continuing to use a parser that may have panicked mid-mutation and be left in an inconsistent state. This drops the current screen contents on the rare occasions it triggers, but keeps the PTY wrapper and wrapped agent process alive; the next output chunk repopulates the screen. Includes a regression test reproducing the exact upstream repro (wide CJK char, downward resize, erase-in-line) to confirm the panic no longer escapes process()/resize(). This is a containment fix, not a root-cause fix — the actual vt100 bug still exists upstream (unmerged as of this writing). Vendoring vt100 with the upstream PR #30 patch would fix the root cause and is a reasonable follow-up, but is a separate, heavier change (pinned git dependency) that this PR intentionally doesn't take on.
) Pin vt100 to Junyi-99/vt100-rust@131a75e (the commit backing the still-open doy/vt100-rust#30) instead of relying solely on the catch_unwind containment added in the previous commit. That patch fixes the real bug: Row::clear_wide now bounds-checks before indexing the continuation cell, and Row::resize clears a wide flag left dangling by a downward resize, instead of the previous behavior of orphaning it and panicking on the next erase. Verified independently (outside hcom, against a plain clone of the patched fork) that the standalone repro from the previous commit's regression test no longer panics with this patch applied. The catch_unwind containment stays as defense-in-depth for any other vt100 edge case, not just this one. This dependency pin is a stopgap until vt100 ships a release containing the fix (or the PR merges) — at that point this patch entry should be removed and the crates.io version requirement bumped instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #73. vt100 0.16.2 has a real upstream bug (doy/vt100-rust#28, open fix in PR #30):
Grid::set_sizeshrinks rows viaRow::resize, which doesn't clean up a double-width character's "wide" flag when its continuation cell is truncated away by a downward resize. The orphaned wide cell then panics the next time it's erased (Row::clear_wideindexes one past the row's new length) — this matches the exact panic signature and line (row.rs:89) from the reported incidents.hcom's global panic hook logs the panic but doesn't stop it from unwinding, so it killed the whole PTY wrapper thread and dropped the wrapped agent session (observed as repeated
stopped by pty: closedon real Codex sessions, sometimes two in the same second).Two changes, in order:
Parser::process,Screen::set_size) incatch_unwindand rebuilds the parser from scratch on a caught panic — a parser that panicked mid-mutation may be left in an inconsistent internal state, so it's discarded rather than reused. This is defense-in-depth for any other vt100 edge case, not just this one.Junyi-99/vt100-rust@131a75e, the commit backing the still-open PR fix: handle UUID identities in instance_not_found_error_for #30 above) so the underlying bug doesn't happen in the first place. Verified independently against a plain clone of the patched fork, outside hcom's own containment code, that the panic no longer occurs. This is a stopgap until vt100 ships a release with the fix — at that point the[patch.crates-io]entry inCargo.tomlshould come out and the version requirement bumped normally.Test plan
process()/resize()cargo clippy --all-targetscleanGenerated by Claude Code