From aba2f18d84bcbbfb33265d78213c0957725d36af Mon Sep 17 00:00:00 2001 From: Ogulcan Celik Date: Tue, 6 Oct 2026 01:25:54 +0300 Subject: [PATCH] fix: wait for split input sequences instead of typing their fragments --- src/client/input.rs | 40 +++--- src/client/shell/tests/input_conformance.rs | 55 ++++++--- src/raw_input.rs | 128 +++++++++++++++++--- 3 files changed, 162 insertions(+), 61 deletions(-) diff --git a/src/client/input.rs b/src/client/input.rs index 08254155d..914b63baa 100644 --- a/src/client/input.rs +++ b/src/client/input.rs @@ -376,17 +376,7 @@ fn idle_flush_timeout_ms( framer: &crate::raw_input::RawInputByteFramer, host_mouse_capture_active: bool, ) -> i32 { - if !host_mouse_capture_active { - return crate::raw_input::RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS; - } - if framer.has_pending_lone_escape() || framer.has_pending_incomplete_mouse_sequence() { - crate::raw_input::MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS - } else if framer.has_pending_csi_introducer() { - // A mouse report split after ESC[ is still ambiguous with legacy Alt+[. - crate::raw_input::MOUSE_ACTIVE_CSI_INTRODUCER_FLUSH_TIMEOUT_MS - } else { - crate::raw_input::RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS - } + framer.idle_flush_timeout_ms(host_mouse_capture_active) } #[cfg(windows)] @@ -1058,7 +1048,7 @@ mod tests { } #[test] - fn mouse_active_escape_sequences_get_longer_reassembly_window() { + fn only_ambiguous_escape_prefixes_get_the_short_keyboard_window() { let mut escape = crate::raw_input::RawInputByteFramer::default(); assert!(escape.push(b"\x1b").is_empty()); let mut csi = crate::raw_input::RawInputByteFramer::default(); @@ -1070,26 +1060,30 @@ mod tests { let mut unrelated = crate::raw_input::RawInputByteFramer::default(); assert!(unrelated.push(b"\x1b[49:33;2:").is_empty()); - for framer in [&escape, &csi, &sgr_mouse, &default_mouse, &unrelated] { + // A lone ESC or ESC[ may still be a key (Escape, Alt+[). + for framer in [&escape, &csi] { assert_eq!( idle_flush_timeout_ms(framer, false), crate::raw_input::RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS ); } - for framer in [&escape, &sgr_mouse, &default_mouse] { - assert_eq!( - idle_flush_timeout_ms(framer, true), - crate::raw_input::MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS - ); - } + assert_eq!( + idle_flush_timeout_ms(&escape, true), + crate::raw_input::MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS + ); assert_eq!( idle_flush_timeout_ms(&csi, true), crate::raw_input::MOUSE_ACTIVE_CSI_INTRODUCER_FLUSH_TIMEOUT_MS ); - assert_eq!( - idle_flush_timeout_ms(&unrelated, true), - crate::raw_input::RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS - ); + // Anything further inside a sequence cannot be a key: wait for the rest. + for framer in [&sgr_mouse, &default_mouse, &unrelated] { + for mouse_capture in [false, true] { + assert_eq!( + idle_flush_timeout_ms(framer, mouse_capture), + crate::raw_input::INCOMPLETE_SEQUENCE_FLUSH_TIMEOUT_MS + ); + } + } let mouse_timeout_ms = std::hint::black_box(crate::raw_input::MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS); diff --git a/src/client/shell/tests/input_conformance.rs b/src/client/shell/tests/input_conformance.rs index 92cab31bd..f97843e6e 100644 --- a/src/client/shell/tests/input_conformance.rs +++ b/src/client/shell/tests/input_conformance.rs @@ -596,10 +596,12 @@ fn print_report(report: &Report) { // Ratchet: failure counts may only go down. Lower them when a change fixes // cases; a rise means a regression in Herdr's input transparency. -const KEYBOARD_FAILURES_BASELINE: usize = 120; +// Remaining: Alt+], Alt+Shift+P/X, Alt+^, Alt+_ behind legacy hosts. Their bytes +// also start host replies, so they are never forwarded (#344). +const KEYBOARD_FAILURES_BASELINE: usize = 50; const MOUSE_FAILURES_BASELINE: usize = 96; -const SPLIT_IDLE_MISMATCH_BASELINE: usize = 6_730; -const REPLY_IDLE_MISMATCH_BASELINE: usize = 77; +// Remaining: legacy hosts split right after ESC[, which is also Alt+[. +const SPLIT_IDLE_MISMATCH_BASELINE: usize = 207; // --------------------------------------------------------------------------- // Mouse @@ -961,14 +963,20 @@ const HOST_REPLIES: &[&[u8]] = &[ b"\x1b[?997;2n", ]; -fn framed_events(host: HostProfile, pieces: &[&[u8]], idle_between: bool) -> String { - framed_events_with(host, pieces, idle_between, false) +/// A network hiccup between two reads of one sequence (SSH, slow links). +const SPLIT_PAUSE_MS: i32 = 50; + +fn framed_events(host: HostProfile, pieces: &[&[u8]], pause_ms: Option) -> String { + framed_events_with(host, pieces, pause_ms, false) } +/// Feed `pieces` as separate reads. With `pause_ms`, the reads are that far +/// apart: like the client loop, the framer is flushed only when the pause +/// outlasts the wait it asked for. fn framed_events_with( host: HostProfile, pieces: &[&[u8]], - idle_between: bool, + pause_ms: Option, awaiting_replies: bool, ) -> String { let mut framer = HerdrPath::fresh_framer(host); @@ -980,9 +988,18 @@ fn framed_events_with( framer.host_cell_size_query_sent(); } let mut chunks = Vec::new(); - for piece in pieces { + for (index, piece) in pieces.iter().enumerate() { chunks.extend(framer.push(piece)); - if idle_between { + let Some(pause_ms) = pause_ms.filter(|_| index + 1 < pieces.len()) else { + continue; + }; + let mut waited = 0; + for _ in 0..3 { + let wait = framer.idle_flush_timeout_ms(true); + if !framer.has_pending_input() || waited + wait > pause_ms { + break; + } + waited += wait; chunks.extend(framer.flush_timeout()); } } @@ -1006,11 +1023,11 @@ fn split_read_robustness() { let mut burst_mismatch = Vec::new(); let mut idle_mismatch = Vec::new(); for (host, bytes) in &corpus { - let whole = framed_events(*host, &[bytes], false); + let whole = framed_events(*host, &[bytes], None); for cut in 1..bytes.len() { splits += 1; let pieces = [&bytes[..cut], &bytes[cut..]]; - if framed_events(*host, &pieces, false) != whole { + if framed_events(*host, &pieces, None) != whole { burst_mismatch.push(format!( "{}\t{}|{}", host.name(), @@ -1018,7 +1035,7 @@ fn split_read_robustness() { show(pieces[1]) )); } - if framed_events(*host, &pieces, true) != whole { + if framed_events(*host, &pieces, Some(SPLIT_PAUSE_MS)) != whole { idle_mismatch.push(format!( "{}\t{}|{}", host.name(), @@ -1032,21 +1049,22 @@ fn split_read_robustness() { let mut reply_burst_mismatch = Vec::new(); let mut reply_idle_mismatch = Vec::new(); for reply in HOST_REPLIES { - let whole = framed_events_with(HostProfile::Kitty, &[reply], false, true); + let whole = framed_events_with(HostProfile::Kitty, &[reply], None, true); for cut in 1..reply.len() { reply_splits += 1; let pieces = [&reply[..cut], &reply[cut..]]; let line = format!("reply\t{}|{}", show(pieces[0]), show(pieces[1])); - if framed_events_with(HostProfile::Kitty, &pieces, false, true) != whole { + if framed_events_with(HostProfile::Kitty, &pieces, None, true) != whole { reply_burst_mismatch.push(line.clone()); } - if framed_events_with(HostProfile::Kitty, &pieces, true, true) != whole { + if framed_events_with(HostProfile::Kitty, &pieces, Some(SPLIT_PAUSE_MS), true) != whole + { reply_idle_mismatch.push(line); } } } println!( - "\nhost replies: {} replies, {} splits\n burst (no pause): {} differ\n pause between: {} differ", + "\nhost replies: {} replies, {} splits\n burst (no pause): {} differ\n {SPLIT_PAUSE_MS}ms pause: {} differ", HOST_REPLIES.len(), reply_splits, reply_burst_mismatch.len(), @@ -1055,7 +1073,7 @@ fn split_read_robustness() { let pct = |bad: usize| 100.0 * (splits - bad) as f64 / splits.max(1) as f64; println!( - "\nsplit reads: {} sequences, {} splits\n burst (no pause): {:.1}% identical ({} differ)\n pause between: {:.1}% identical ({} differ)", + "\nsplit reads: {} sequences, {} splits\n burst (no pause): {:.1}% identical ({} differ)\n {SPLIT_PAUSE_MS}ms pause: {:.1}% identical ({} differ)", corpus.len(), splits, pct(burst_mismatch.len()), @@ -1087,9 +1105,8 @@ fn split_read_robustness() { idle_mismatch.len() ); assert!( - reply_idle_mismatch.len() <= REPLY_IDLE_MISMATCH_BASELINE, - "host-reply split robustness regressed: {} > {REPLY_IDLE_MISMATCH_BASELINE}", - reply_idle_mismatch.len() + reply_idle_mismatch.is_empty(), + "split host replies changed after a {SPLIT_PAUSE_MS}ms pause: {reply_idle_mismatch:?}" ); } diff --git a/src/raw_input.rs b/src/raw_input.rs index c66d75840..5b74931b5 100644 --- a/src/raw_input.rs +++ b/src/raw_input.rs @@ -21,16 +21,20 @@ use crate::terminal_theme::{ }; const ESC: u8 = 0x1b; -#[cfg(unix)] +#[cfg(any(unix, test))] pub(crate) const RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS: i32 = 10; -#[cfg(unix)] +#[cfg(any(unix, test))] pub(crate) const MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS: i32 = 150; /// Covers the 33 ms split in #4630 without gluing legacy Alt+[ to the next key. -#[cfg(unix)] +#[cfg(any(unix, test))] pub(crate) const MOUSE_ACTIVE_CSI_INTRODUCER_FLUSH_TIMEOUT_MS: i32 = 50; /// Covers the 350 ms mouse tail delay in #3480. Other input ends it early (#4751). #[cfg(unix)] const DISAMBIGUATED_MOUSE_TAIL_FLUSH_TIMEOUT_MS: i32 = 500; +/// Bytes already inside a sequence cannot be a key, so wait long enough for a +/// split read over a slow link before giving up on them. +#[cfg(any(unix, test))] +pub(crate) const INCOMPLETE_SEQUENCE_FLUSH_TIMEOUT_MS: i32 = 500; pub(crate) const GHOSTTY_COLOR_SCHEME_DARK_REPORT: &[u8] = b"\x1b[?997;1n"; pub(crate) const GHOSTTY_COLOR_SCHEME_LIGHT_REPORT: &[u8] = b"\x1b[?997;2n"; const BRACKETED_PASTE_START: &[u8] = b"\x1b[200~"; @@ -166,6 +170,12 @@ impl RawInputFramer { .with_vt_bytes(chunk), )); } + // The byte framer released this introducer as a key on its own. + if is_alt_key_introducer(&chunk) { + let text = std::str::from_utf8(&chunk).ok()?; + return parse_terminal_key_sequence(text) + .map(|key| RawInputEvent::Key(key.with_vt_bytes(chunk.clone()))); + } extract_one_event(&chunk).map(|(event, _consumed)| { tracing::debug!(raw_bytes = ?chunk, event = ?event, "raw input event parsed"); event @@ -304,6 +314,44 @@ impl RawInputByteFramer { self.host_escape_disambiguation_active = active; } + /// Pending bytes that could still be a key on their own: a lone ESC or ESC + /// plus one introducer. Legacy hosts send those for Escape and Alt+key, and + /// host text bindings (e.g. Ghostty `text:\x1bO`) send them even when the + /// host disambiguates escapes. + #[cfg(any(unix, test))] + fn pending_could_be_escape_key(&self) -> bool { + is_escape_key_prefix(&self.buffer) + } + + /// How long to wait for more input before `flush_timeout`. Only bytes that + /// could still be a key get the short keyboard window; anything already + /// inside a sequence waits long enough for split reads over slow links. + #[cfg(any(unix, test))] + pub(crate) fn idle_flush_timeout_ms(&self, host_mouse_capture_active: bool) -> i32 { + if self.buffer.is_empty() { + return RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS; + } + // A reply Herdr asked for can split right after its introducer. + if self.awaiting_host_reply() && matches!(self.buffer.as_slice(), b"\x1b[" | b"\x1b]") { + return INCOMPLETE_SEQUENCE_FLUSH_TIMEOUT_MS; + } + if self.pending_could_be_escape_key() { + if !host_mouse_capture_active { + return RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS; + } + return match self.buffer.as_slice() { + [ESC] => MOUSE_ACTIVE_ESCAPE_SEQUENCE_FLUSH_TIMEOUT_MS, + // A mouse report split after ESC[ is still ambiguous with legacy Alt+[. + b"\x1b[" => MOUSE_ACTIVE_CSI_INTRODUCER_FLUSH_TIMEOUT_MS, + _ => RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS, + }; + } + if self.buffer.first() == Some(&ESC) || starts_with_incomplete_utf8_char(&self.buffer) { + return INCOMPLETE_SEQUENCE_FLUSH_TIMEOUT_MS; + } + RAW_INPUT_IDLE_FLUSH_TIMEOUT_MS + } + /// How long to keep holding input after a first idle flush held it. #[cfg(unix)] pub(crate) fn held_input_flush_timeout_ms(&self) -> i32 { @@ -314,22 +362,6 @@ impl RawInputByteFramer { } } - #[cfg(unix)] - pub(crate) fn has_pending_lone_escape(&self) -> bool { - self.buffer.as_slice() == [ESC] - } - - #[cfg(unix)] - pub(crate) fn has_pending_csi_introducer(&self) -> bool { - self.buffer.as_slice() == b"\x1b[" - } - - #[cfg(unix)] - pub(crate) fn has_pending_incomplete_mouse_sequence(&self) -> bool { - starts_with_incomplete_sgr_mouse_sequence(&self.buffer) - || starts_with_incomplete_default_mouse_sequence(&self.buffer) - } - #[cfg(any(windows, test))] pub(crate) fn has_pending_bracketed_paste(&self) -> bool { self.buffer.starts_with(BRACKETED_PASTE_START) @@ -392,8 +424,11 @@ impl RawInputByteFramer { #[cfg(not(unix))] let mouse_wait_served = false; #[cfg(unix)] + // Prefixes already inside a sequence waited the long window; only the + // ambiguous ESC / ESC[ prefixes need this extra mouse-tail hold. if !mouse_wait_served && self.host_escape_disambiguation_active + && self.pending_could_be_escape_key() && could_continue_as_mouse_report(&self.buffer) { self.awaiting_mouse_tail_after = Some(self.buffer.len()); @@ -515,6 +550,26 @@ impl RawInputByteFramer { return chunks; } + // A legacy host sends Alt+[ and Alt+O as ESC plus that byte. When nothing + // followed and no reply is expected, it was the key. String introducers + // (OSC, DCS, APC, PM, SOS) also start host replies, so they keep the + // control-string path and are never forwarded as Alt keys (#344). + if is_alt_key_introducer(&self.buffer) && !self.awaiting_host_reply() { + chunks.push(std::mem::take(&mut self.buffer)); + return chunks; + } + // A lone string introducer with no reply expected is a legacy Alt chord + // that cannot be forwarded safely. Drop just these bytes; entering + // control-string discard would swallow the user's next keystrokes. + if is_escape_key_prefix(&self.buffer) + && self.buffer.len() == 2 + && !self.awaiting_host_reply() + { + tracing::debug!(bytes = ?self.buffer, "dropping lone string introducer"); + self.buffer.clear(); + return chunks; + } + if let Some(ControlString::Incomplete { family }) = control_string(&self.buffer) { tracing::debug!( len = self.buffer.len(), @@ -991,6 +1046,18 @@ fn starts_with_incomplete_host_cell_size_report(buffer: &[u8]) -> bool { && !(height.is_some_and(<[u8]>::is_empty) && width.is_some()) } +fn is_alt_key_introducer(buffer: &[u8]) -> bool { + matches!(buffer, [ESC, b'[' | b'O']) +} + +fn is_escape_key_prefix(buffer: &[u8]) -> bool { + match buffer { + [ESC] => true, + [ESC, introducer] => matches!(introducer, b'[' | b'O' | b'P' | b']' | b'X' | b'^' | b'_'), + _ => false, + } +} + fn control_string(buffer: &[u8]) -> Option { let family = match buffer.get(..2)? { b"\x1b]" => ControlStringFamily::Osc, @@ -1121,6 +1188,7 @@ fn could_continue_as_mouse_report(buffer: &[u8]) -> bool { } #[cfg(any(unix, windows, test))] +#[cfg(any(windows, test))] fn starts_with_incomplete_default_mouse_sequence(buffer: &[u8]) -> bool { buffer.starts_with(b"\x1b[M") && buffer.len() < 6 } @@ -2632,6 +2700,7 @@ mod tests { #[test] fn raw_input_byte_framer_discards_split_control_string_after_timeout() { let mut framer = RawInputByteFramer::default(); + framer.host_color_query_sent(); assert!(framer.push(b"\x1b]").is_empty()); assert!(framer.flush_timeout().is_empty()); @@ -2642,6 +2711,7 @@ mod tests { #[test] fn raw_input_byte_framer_keeps_discarding_tail_across_timeout() { let mut framer = RawInputByteFramer::default(); + framer.host_color_query_sent(); assert!(framer.push(b"\x1b]").is_empty()); assert!(framer.flush_timeout().is_empty()); @@ -2654,6 +2724,7 @@ mod tests { #[test] fn raw_input_byte_framer_releases_discard_on_implausible_tail() { let mut framer = RawInputByteFramer::default(); + framer.host_color_query_sent(); assert!(framer.push(b"\x1b]").is_empty()); assert!(framer.flush_timeout().is_empty()); @@ -2662,6 +2733,25 @@ mod tests { assert_eq!(framer.push(b"b"), vec![b"b".to_vec()]); } + #[test] + fn csi_and_ss3_introducers_without_expected_reply_are_alt_keys() { + for (bytes, code) in [ + (&b"\x1b["[..], KeyCode::Char('[')), + (b"\x1bO", KeyCode::Char('O')), + ] { + let mut framer = RawInputFramer::default(); + assert!(framer.push(bytes).is_empty()); + let events = framer.flush_timeout(); + assert!( + matches!(&events[..], [RawInputEvent::Key(key)] + if key.code == code && key.modifiers.contains(KeyModifiers::ALT)), + "{bytes:?}: {events:?}" + ); + // The next keystroke is not swallowed as a sequence tail. + assert_eq!(framer.push(b"a").len(), 1, "{bytes:?}"); + } + } + #[test] fn parse_raw_input_bytes_sync_does_not_parse_incomplete_strings_as_alt_keys() { for bytes in [