Harden PTY lifecycle: resize upgrade pane, buffer OSC52 across chunks#32
Closed
Junyi-99 wants to merge 1 commit into
Closed
Harden PTY lifecycle: resize upgrade pane, buffer OSC52 across chunks#32Junyi-99 wants to merge 1 commit into
Junyi-99 wants to merge 1 commit into
Conversation
Two fixes from review of `src/app/pty.rs`: - `resize_pty` now also resizes the upgrade PTY and its vt100 parser. Previously a terminal resize while the Homebrew upgrade pane was visible left that pane's PTY and parser stuck at the old size. Extract a `resize_one` helper so the three resize call sites (main, plugins, upgrade) share a single source of truth for the `pixel_width`/`pixel_height` constants. - Replace the stateless `forward_osc52(data: &[u8])` with an `Osc52Forwarder` state machine on `App`. PTY reads can split an OSC 52 sequence anywhere -- inside the `\x1b]52;` marker, inside the payload, or between the ESC and `\\` of an ST terminator -- so the previous single-chunk scan would silently drop split clipboard updates. The forwarder buffers the in-progress sequence (capped at 1 MiB so a runaway program cannot grow it without bound) and emits each complete sequence to stdout once a BEL or ST terminator arrives. Single-chunk behavior is preserved; only the split case changes. Tests cover: single-chunk BEL, single-chunk ST, two-chunk split in payload, three-chunk split through marker / payload / ST, multiple sequences in one chunk, oversize-then-recover, garbage interleaved, embedded ESC inside payload, and one-byte-at-a-time delivery.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens PTY handling by resizing the upgrade PTY alongside the main/plugin PTYs and replacing stateless OSC 52 forwarding with an incremental buffered forwarder.
Changes:
- Adds
resize_one()and applies it to main, plugin, and upgrade PTYs. - Adds an
Osc52Forwarderstate machine with sink injection and tests. - Stores an OSC 52 forwarder on
Appand routes main PTY output through it.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/app/pty.rs |
Refactors PTY resize logic and includes the upgrade PTY in resize handling. |
src/app/osc52.rs |
Adds buffered OSC 52 parsing/forwarding with bounded state. |
src/app/mod.rs |
Wires the OSC 52 forwarder into app state and main PTY output processing. |
tests/unit/app/osc52.rs |
Adds unit coverage for chunked, terminated, overflow, and interleaved OSC 52 sequences. |
Comments suppressed due to low confidence (1)
src/app/osc52.rs:133
- The ST terminator path also bypasses the size guard: when
buf.len() >= MAX_BUF_LEN,ESC \\is still appended and forwarded before the overflow check below can run. Oversized OSC 52 sequences terminated with ST should be dropped consistently with the configured cap.
if b == b'\\' {
// ST terminator: \x1b\\
self.buf.push(ESC);
self.buf.push(b);
self.sink.write(&self.buf);
self.reset_scan();
} else if self.buf.len() >= MAX_BUF_LEN {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+114
to
+132
| if b == BEL { | ||
| self.buf.push(b); | ||
| self.sink.write(&self.buf); | ||
| self.reset_scan(); | ||
| } else if b == ESC { | ||
| self.state = State::SawEsc; | ||
| } else if self.buf.len() >= MAX_BUF_LEN { | ||
| self.state = State::Overflow; | ||
| } else { | ||
| self.buf.push(b); | ||
| } | ||
| } | ||
| State::SawEsc => { | ||
| if b == b'\\' { | ||
| // ST terminator: \x1b\\ | ||
| self.buf.push(ESC); | ||
| self.buf.push(b); | ||
| self.sink.write(&self.buf); | ||
| self.reset_scan(); |
| update_checker: Option<crate::update::UpdateChecker>, | ||
| upgrade_instance: Option<PluginInstance>, | ||
| last_update_request: Option<Instant>, | ||
| osc52: Osc52Forwarder, |
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
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
Two PTY bugs from codex review, plus a small helper refactor.
Bug fix — terminal resize was skipping the upgrade-page PTY.
resize_pty()only resized the main PTY andplugin_instances, leavingupgrade_instancestuck at the old dimensions. New helperresize_one(parser, pty, rows, cols)is the sole owner ofPtySizeconstruction and is called for main, every plugin instance, and the upgrade instance.Bug fix —
forward_osc52()was stateless and lost sequences split across PTY read chunks.PTY read boundaries are arbitrary; OSC 52 (clipboard) sequences can have their marker, payload, or terminator split across chunks. New
Osc52Forwarderstate machine onAppbuffers partial sequences and emits on terminator (BEL orESC \\). Capped at 1 MiB to bound worst-case memory if a remote program emits\x1b]52;and never terminates.Sink injection via a tiny
Osc52Sinktrait so tests can capture forwarded bytes without writing to stdout.Test plan
cargo test— 186 passed (was 177; +9 newOsc52Forwardertests covering single chunk, split-in-payload, split-in-marker, split-across-three-chunks, multiple sequences in one chunk, oversize drop, garbage interleave, ESC-not-followed-by-backslash, marker-byte-at-a-time)🤖 Generated with Claude Code