diff --git a/crates/fresh-editor/plugins/config-schema.json b/crates/fresh-editor/plugins/config-schema.json index 44eb62243b..cd76927bca 100644 --- a/crates/fresh-editor/plugins/config-schema.json +++ b/crates/fresh-editor/plugins/config-schema.json @@ -132,6 +132,7 @@ "auto_recovery_save_interval_secs": 2, "auto_revert_poll_interval_ms": 2000, "keyboard_disambiguate_escape_codes": true, + "keyboard_escape_time_ms": 50, "keyboard_report_event_types": false, "keyboard_report_alternate_keys": true, "keyboard_report_all_keys_as_escape_codes": false, @@ -887,6 +888,14 @@ "default": true, "x-section": "Keyboard" }, + "keyboard_escape_time_ms": { + "description": "How long a lone Escape byte waits for a continuation before it is treated\nas the Escape key, in milliseconds.\nIn the legacy encoding `ESC` is both the Escape key and the prefix of every\nescape sequence, so this is a guess on a timer: too low and a mouse report\nsplit by a slow ssh/mosh link is torn apart mid-sequence, too high and the\nEscape key feels sluggish. Raise it on high-latency connections\n(tmux's equivalent, `escape-time`, defaults to 500).\nIgnored entirely on terminals that confirm the kitty keyboard protocol's\n\"disambiguate escape codes\" mode, where Escape is unambiguous and no\nguessing is needed.\nDefault: 50ms", + "type": "integer", + "format": "uint64", + "minimum": 0, + "default": 50, + "x-section": "Keyboard" + }, "keyboard_report_event_types": { "description": "Enable keyboard enhancement: report key event types (repeat/release).\nAdds extra events when keys are autorepeated or released.\nRequires terminal support (kitty keyboard protocol).\nDefault: false", "type": "boolean", diff --git a/crates/fresh-editor/src/config.rs b/crates/fresh-editor/src/config.rs index 2d81913bbc..571e0f5d90 100644 --- a/crates/fresh-editor/src/config.rs +++ b/crates/fresh-editor/src/config.rs @@ -1685,6 +1685,21 @@ pub struct EditorConfig { #[schemars(extend("x-section" = "Keyboard"))] pub keyboard_disambiguate_escape_codes: bool, + /// How long a lone Escape byte waits for a continuation before it is treated + /// as the Escape key, in milliseconds. + /// In the legacy encoding `ESC` is both the Escape key and the prefix of every + /// escape sequence, so this is a guess on a timer: too low and a mouse report + /// split by a slow ssh/mosh link is torn apart mid-sequence, too high and the + /// Escape key feels sluggish. Raise it on high-latency connections + /// (tmux's equivalent, `escape-time`, defaults to 500). + /// Ignored entirely on terminals that confirm the kitty keyboard protocol's + /// "disambiguate escape codes" mode, where Escape is unambiguous and no + /// guessing is needed. + /// Default: 50ms + #[serde(default = "default_keyboard_escape_time")] + #[schemars(extend("x-section" = "Keyboard"))] + pub keyboard_escape_time_ms: u64, + /// Enable keyboard enhancement: report key event types (repeat/release). /// Adds extra events when keys are autorepeated or released. /// Requires terminal support (kitty keyboard protocol). @@ -1815,6 +1830,15 @@ fn default_quick_suggestions_delay() -> u64 { 150 // 150ms — fast enough to feel responsive, slow enough to not interrupt typing } +fn default_keyboard_escape_time() -> u64 { + // 50ms, matching Neovim's `ttimeoutlen` and libtermkey's waittime — the + // established floor for reassembling sequences split across reads. The + // previous 15ms was the most aggressive of any comparable tool (tmux's + // `escape-time` defaults to 500ms) and left normal ssh jitter inside the + // window where a split mouse report is torn apart (sinelaw/fresh#2793). + 50 +} + fn default_scroll_offset() -> usize { 3 } @@ -1917,6 +1941,7 @@ impl Default for EditorConfig { rainbow_brackets: true, cursor_style: CursorStyle::default(), keyboard_disambiguate_escape_codes: true, + keyboard_escape_time_ms: default_keyboard_escape_time(), keyboard_report_event_types: false, keyboard_report_alternate_keys: true, keyboard_report_all_keys_as_escape_codes: false, diff --git a/crates/fresh-editor/src/main.rs b/crates/fresh-editor/src/main.rs index 9f2021110b..cfd34b405d 100644 --- a/crates/fresh-editor/src/main.rs +++ b/crates/fresh-editor/src/main.rs @@ -4736,7 +4736,9 @@ fn run_event_loop( // Host input is read raw and parsed by fresh's own state machine (see // `services::tty_input`), not crossterm — this is what prevents mouse // reports from leaking into focused terminals (#2745). - let mut reader = fresh::services::tty_input::TtyReader::new(); + let mut reader = fresh::services::tty_input::TtyReader::with_escape_grace( + std::time::Duration::from_millis(editor.config().editor.keyboard_escape_time_ms), + ); run_event_loop_common( editor, terminal, @@ -4859,7 +4861,9 @@ fn run_event_loop( // Host input is read raw and parsed by fresh's own state machine (see // `services::tty_input`), not crossterm — this is what prevents mouse // reports from leaking into focused terminals (#2745). - let mut reader = fresh::services::tty_input::TtyReader::new(); + let mut reader = fresh::services::tty_input::TtyReader::with_escape_grace( + std::time::Duration::from_millis(editor.config().editor.keyboard_escape_time_ms), + ); run_event_loop_common( editor, terminal, diff --git a/crates/fresh-editor/src/partial_config.rs b/crates/fresh-editor/src/partial_config.rs index 5246d6486a..bddb76463e 100644 --- a/crates/fresh-editor/src/partial_config.rs +++ b/crates/fresh-editor/src/partial_config.rs @@ -199,6 +199,7 @@ pub struct PartialEditorConfig { pub rainbow_brackets: Option, pub cursor_style: Option, pub keyboard_disambiguate_escape_codes: Option, + pub keyboard_escape_time_ms: Option, pub keyboard_report_event_types: Option, pub keyboard_report_alternate_keys: Option, pub keyboard_report_all_keys_as_escape_codes: Option, @@ -310,6 +311,8 @@ impl Merge for PartialEditorConfig { self.cursor_style.merge_from(&other.cursor_style); self.keyboard_disambiguate_escape_codes .merge_from(&other.keyboard_disambiguate_escape_codes); + self.keyboard_escape_time_ms + .merge_from(&other.keyboard_escape_time_ms); self.keyboard_report_event_types .merge_from(&other.keyboard_report_event_types); self.keyboard_report_alternate_keys @@ -654,6 +657,7 @@ impl From<&crate::config::EditorConfig> for PartialEditorConfig { rainbow_brackets: Some(cfg.rainbow_brackets), cursor_style: Some(cfg.cursor_style), keyboard_disambiguate_escape_codes: Some(cfg.keyboard_disambiguate_escape_codes), + keyboard_escape_time_ms: Some(cfg.keyboard_escape_time_ms), keyboard_report_event_types: Some(cfg.keyboard_report_event_types), keyboard_report_alternate_keys: Some(cfg.keyboard_report_alternate_keys), keyboard_report_all_keys_as_escape_codes: Some( @@ -808,6 +812,9 @@ impl PartialEditorConfig { keyboard_disambiguate_escape_codes: self .keyboard_disambiguate_escape_codes .unwrap_or(defaults.keyboard_disambiguate_escape_codes), + keyboard_escape_time_ms: self + .keyboard_escape_time_ms + .unwrap_or(defaults.keyboard_escape_time_ms), keyboard_report_event_types: self .keyboard_report_event_types .unwrap_or(defaults.keyboard_report_event_types), diff --git a/crates/fresh-editor/src/server/editor_server.rs b/crates/fresh-editor/src/server/editor_server.rs index e64b6ad359..3e848254b9 100644 --- a/crates/fresh-editor/src/server/editor_server.rs +++ b/crates/fresh-editor/src/server/editor_server.rs @@ -1124,7 +1124,13 @@ impl EditorServer { term_size: hello.term_size, env: hello.env, id: client_id, - input_parser: ClientInputParser::new(), + // Same escape grace as the direct tty path (#2793): the session + // path used to flush a lone `ESC` after a single 15ms window while + // the tty path allowed two, so `fresh -a` tore split sequences + // apart twice as readily. + input_parser: ClientInputParser::with_escape_grace(std::time::Duration::from_millis( + self.config.editor_config.editor.keyboard_escape_time_ms, + )), needs_full_render: true, wait_id: None, cmd_token: hello.cmd_token, diff --git a/crates/fresh-editor/src/server/input_parser.rs b/crates/fresh-editor/src/server/input_parser.rs index b1f6e60a76..a3496f8496 100644 --- a/crates/fresh-editor/src/server/input_parser.rs +++ b/crates/fresh-editor/src/server/input_parser.rs @@ -14,12 +14,16 @@ use crossterm::event::Event; pub use fresh_input_parser::InputParser; -/// How long a lone `ESC` stays buffered before it is resolved as the Escape -/// key. Mirrors the tty reader's grace window: long enough that a control -/// sequence split across two socket reads still arrives as one sequence, short -/// enough that Escape feels immediate. Two keystrokes can never land this close -/// together, so nothing a user types is coalesced by it. -const ESC_GRACE: Duration = Duration::from_millis(15); +/// Default grace for a lone `ESC` on the session path, used when no configured +/// value is supplied. +/// +/// Mirrors the tty reader's default (and `editor.keyboard_escape_time_ms`): long +/// enough that a control sequence split across two socket reads still arrives as +/// one sequence, short enough that Escape feels immediate. The two paths must +/// agree — this window was 15ms while the tty path effectively allowed ~30ms, +/// which made `fresh -a` twice as easy to tear a split mouse report apart on +/// (sinelaw/fresh#2793). +pub const DEFAULT_ESC_GRACE: Duration = Duration::from_millis(50); /// [`InputParser`] plus the idle-flush rule the session path needs. /// @@ -39,13 +43,22 @@ pub struct ClientInputParser { /// When the currently-buffered lone `ESC` was first observed. `None` /// whenever the parser is not sitting on one. escape_pending_since: Option, + /// How long that `ESC` may stay buffered before resolving to the Escape key. + esc_grace: Duration, } impl ClientInputParser { pub fn new() -> Self { + Self::with_escape_grace(DEFAULT_ESC_GRACE) + } + + /// As [`ClientInputParser::new`], with the grace from + /// `editor.keyboard_escape_time_ms`. + pub fn with_escape_grace(esc_grace: Duration) -> Self { Self { parser: InputParser::new(), escape_pending_since: None, + esc_grace, } } @@ -58,7 +71,7 @@ impl ClientInputParser { } /// Resolve a buffered lone `ESC` as the Escape key once it has been pending - /// for [`ESC_GRACE`] without a continuation. Returns the events to inject + /// for the escape grace without a continuation. Returns the events to inject /// (empty when nothing is pending or the grace window has not elapsed). /// /// `now` is a parameter so tests can drive the window without sleeping. @@ -66,7 +79,7 @@ impl ClientInputParser { let Some(since) = self.escape_pending_since else { return Vec::new(); }; - if now.duration_since(since) < ESC_GRACE { + if now.duration_since(since) < self.esc_grace { return Vec::new(); } let events = self.parser.flush(); @@ -78,7 +91,9 @@ impl ClientInputParser { /// the parser has moved on (the continuation arrived, or the escape was /// already flushed). fn sync_escape_timer(&mut self, now: Instant) { - if self.parser.escape_pending() { + // A terminal that confirmed unambiguous Escape encoding never needs the + // timer: a lone `ESC` there is only ever the head of a sequence. + if self.parser.escape_pending() && !self.parser.escape_unambiguous() { self.escape_pending_since.get_or_insert(now); } else { self.escape_pending_since = None; diff --git a/crates/fresh-editor/src/services/terminal_modes.rs b/crates/fresh-editor/src/services/terminal_modes.rs index 970539410b..d52a613e47 100644 --- a/crates/fresh-editor/src/services/terminal_modes.rs +++ b/crates/fresh-editor/src/services/terminal_modes.rs @@ -199,6 +199,23 @@ impl TerminalModes { "Pushed keyboard enhancement flags optimistically: {:?}", flags ); + // Ask what actually took effect. Pushing optimistically leaves us + // unable to tell a terminal that honoured the flags from one that + // ignored them, and that distinction decides whether a lone `ESC` + // can ever be the Escape key: with "disambiguate escape codes" + // active it arrives as `CSI 27 u`, so a bare `0x1b` is always the + // head of a sequence and must never be resolved on a timer + // (sinelaw/fresh#2793). + // + // Unlike `crossterm::supports_keyboard_enhancement` this costs + // nothing on terminals that don't implement the protocol: we do + // not wait for the answer. `CSI ? u` is silently ignored by them, + // while terminals that do implement it reply on stdin, where the + // input parser picks the reply up as part of normal reading + // (`InputParser::take_keyboard_flags_reply`). + if let Err(e) = write!(stdout(), "\x1b[?u").and_then(|()| stdout().flush()) { + tracing::info!("Failed to query keyboard flags: {}", e); + } } } else { tracing::debug!("Keyboard enhancement disabled by config"); diff --git a/crates/fresh-editor/src/services/tty_input.rs b/crates/fresh-editor/src/services/tty_input.rs index ae97ec831b..c4fc7d98e1 100644 --- a/crates/fresh-editor/src/services/tty_input.rs +++ b/crates/fresh-editor/src/services/tty_input.rs @@ -31,20 +31,11 @@ use std::time::Duration; use crossterm::event::{Event as CrosstermEvent, MouseEventKind}; use fresh_input_parser::InputParser; -/// How long a buffered lone `ESC` waits for a continuation before it is -/// resolved as the Escape key. This bounds two waits: the in-`drain_stdin` -/// window that lets a sequence split across a read boundary complete as one -/// event, and the idle wait in [`TtyReader::poll`] before a genuinely lone -/// `ESC` is emitted as the Escape key. It must stay well below human -/// key-repeat latency so Escape still registers promptly. +/// Default grace for a buffered lone `ESC`, when no configured value is given +/// (tests, and callers that predate the config plumbing). /// -/// Crucially the grace is only ever a *wait*, never — as it once was — a -/// deadline that flushes the `ESC` mid-stream: a continuation arriving after -/// the window elapses (a mouse report split across a slow pty/socket boundary) -/// must still be parsed as the control sequence it is, not torn into an Escape -/// key plus literal keystrokes that fresh forwards into a focused embedded -/// terminal (sinelaw/fresh#2793, a residue of #2745). -const ESC_GRACE: Duration = Duration::from_millis(15); +/// Matches `editor.keyboard_escape_time_ms`; see that setting for why 50ms. +pub const DEFAULT_ESC_GRACE: Duration = Duration::from_millis(50); /// Set to true by the `SIGWINCH` handler; consumed by [`TtyReader::take_resize`]. static SIGWINCH_PENDING: AtomicBool = AtomicBool::new(false); @@ -101,17 +92,29 @@ pub struct TtyReader { parser: InputParser, queue: VecDeque, stdin_fd: RawFd, + /// How long a lone `ESC` may wait for a continuation before it resolves to + /// the Escape key. Only consulted while the terminal has *not* confirmed + /// unambiguous Escape encoding. + esc_grace: Duration, } impl TtyReader { - /// Install the `SIGWINCH` handler and take ownership of stdin input. + /// Install the `SIGWINCH` handler and take ownership of stdin input, using + /// the default escape grace. pub fn new() -> Self { + Self::with_escape_grace(DEFAULT_ESC_GRACE) + } + + /// As [`TtyReader::new`], with the grace from + /// `editor.keyboard_escape_time_ms`. + pub fn with_escape_grace(esc_grace: Duration) -> Self { install_sigwinch_handler(); RAW_INPUT_ACTIVE.store(true, Ordering::Relaxed); Self { parser: InputParser::new(), queue: VecDeque::new(), stdin_fd: std::io::stdin().as_raw_fd(), + esc_grace, } } @@ -137,33 +140,43 @@ impl TtyReader { /// promptly without blocking. /// /// A lone trailing `ESC` is ambiguous — the Escape key, or the head of a - /// sequence split across reads. If a continuation arrives within - /// [`ESC_GRACE`] it is pulled in so the sequence completes as one event; - /// otherwise the `ESC` is left *buffered* (not emitted) and only resolved - /// as the Escape key by [`flush_pending_escape`](Self::flush_pending_escape) - /// once stdin actually goes idle. Flushing it here — as a previous version - /// did on grace expiry — tore a slowly-split control sequence into an - /// Escape key followed by its remainder as literal keystrokes, which fresh - /// then forwarded verbatim into a focused embedded terminal + /// sequence split across reads. If a continuation arrives within the escape + /// grace it is pulled in so the sequence completes as one event; otherwise + /// the `ESC` is left *buffered* (not emitted) and only resolved as the + /// Escape key by [`flush_pending_escape`](Self::flush_pending_escape) once + /// stdin actually goes idle. Flushing it here — as a previous version did on + /// grace expiry — tore a slowly-split control sequence into an Escape key + /// followed by its remainder as literal keystrokes, which fresh then + /// forwarded verbatim into a focused embedded terminal /// (sinelaw/fresh#2793). pub fn drain_stdin(&mut self) { while self.read_once() { - if !self.parser.escape_pending() || !poll_readable(self.stdin_fd, ESC_GRACE) { + if !self.parser.flush_pending() || !poll_readable(self.stdin_fd, self.esc_grace) { break; } } } - /// Resolve a buffered lone `ESC` as the Escape key press, queueing the - /// event. A no-op when no `ESC` is pending. + /// Resolve a buffered lone `ESC` as the Escape key press (or release a `[` + /// held after an earlier flush), queueing the event. A no-op when nothing is + /// pending. /// /// The caller invokes this only when stdin has gone idle — a blocking - /// [`poll`](Self::poll) that timed out with no further bytes. At that point - /// a pending `ESC` has no continuation in flight, so it is unambiguously the - /// Escape key. Keeping the decision here (rather than at the end of every - /// [`drain_stdin`](Self::drain_stdin)) is what makes the leak in - /// sinelaw/fresh#2793 structurally impossible: while bytes are still - /// arriving the `ESC` stays buffered and combines with its continuation. + /// [`poll`](Self::poll) that timed out with no further bytes. Deferring the + /// decision to here rather than to the end of every + /// [`drain_stdin`](Self::drain_stdin) widens the window in which a split + /// sequence still reassembles from one grace period to two. + /// + /// It does **not** eliminate the leak, and earlier revisions of this comment + /// wrongly claimed it did: a continuation that arrives after both windows + /// have elapsed still finds the `ESC` gone. That is inherent to the legacy + /// encoding — `0x1b` is both the Escape key and a sequence prefix, so every + /// implementation guesses on a timer (tmux `escape-time`, Neovim + /// `ttimeoutlen`). What bounds the damage is elsewhere: the parser resyncs a + /// mouse report whose `ESC` was flushed instead of spraying its bytes as + /// keystrokes, and on terminals that confirm the kitty protocol's + /// disambiguate mode the guess never happens at all + /// (`InputParser::set_escape_unambiguous`). See sinelaw/fresh#2793. pub fn flush_pending_escape(&mut self) { for ev in self.parser.flush() { self.push_coalesced(ev); @@ -188,9 +201,36 @@ impl TtyReader { for ev in events { self.push_coalesced(ev); } + self.adopt_keyboard_flags_reply(); true } + /// Act on a kitty keyboard-flags reply (`CSI ? u`) if one arrived in + /// the bytes just parsed — the answer to the `CSI ? u` query + /// `TerminalModes::enable` sends after pushing its enhancement flags. + /// + /// Bit 0 is "disambiguate escape codes". When the terminal confirms it, the + /// Escape key arrives as `CSI 27 u`, so a bare `0x1b` is *always* the head of + /// a sequence and must never be resolved on a timer; telling the parser so + /// retires the guess entirely on those terminals (sinelaw/fresh#2793). A + /// reply with the bit clear (or no reply at all, from a terminal that + /// ignored both the push and the query) leaves the timer in charge. + fn adopt_keyboard_flags_reply(&mut self) { + let Some(flags) = self.parser.take_keyboard_flags_reply() else { + return; + }; + let unambiguous = flags & 0b1 != 0; + if unambiguous != self.parser.escape_unambiguous() { + tracing::info!( + "Terminal reported keyboard flags {flags:#b}; \ + Escape is {}ambiguous, escape timer {}", + if unambiguous { "un" } else { "" }, + if unambiguous { "retired" } else { "in use" }, + ); + } + self.parser.set_escape_unambiguous(unambiguous); + } + /// Queue an event, collapsing a run of mouse-move events down to the latest /// one (a motion flood produces one Moved event per read batch), matching /// the coalescing the crossterm path did in `coalesce_mouse_moves`. @@ -216,13 +256,17 @@ impl TtyReader { if let Some(ev) = self.take_resize() { return Ok(Some(ev)); } - // While a lone `ESC` is buffered, cap the wait to `ESC_GRACE`: if a - // continuation arrives it completes the sequence, and if the stream - // stays idle we resolve the `ESC` as the Escape key promptly instead of - // blocking for the caller's full timeout. When nothing is pending the - // caller's timeout is honoured as before. - let wait = if self.parser.escape_pending() { - timeout.min(ESC_GRACE) + // While a lone `ESC` (or a `[` held after a flush) is buffered, cap the + // wait to the escape grace: if a continuation arrives it completes the + // sequence, and if the stream stays idle we resolve the pending byte + // promptly instead of blocking for the caller's full timeout. When + // nothing is pending the caller's timeout is honoured as before. + // + // A terminal that confirmed unambiguous Escape encoding needs no cap: a + // lone `ESC` there is never a key press, so there is nothing to resolve + // and the parser simply waits for the rest of the sequence. + let wait = if self.parser.flush_pending() && !self.parser.escape_unambiguous() { + timeout.min(self.esc_grace) } else { timeout }; @@ -265,6 +309,7 @@ impl TtyReader { parser: InputParser::new(), queue: VecDeque::new(), stdin_fd: fd, + esc_grace: DEFAULT_ESC_GRACE, } } } @@ -314,7 +359,13 @@ mod tests { /// must arrive as a single `Mouse` event, never the Escape key followed by /// its remainder (`[ M C H 4`) as literal keystrokes. Before the fix /// `drain_stdin` flushed the `ESC` as soon as no continuation arrived within - /// `ESC_GRACE`, so this split leaked six key events and zero mouse events. + /// the escape grace, so this split leaked six key events and zero mouse + /// events. + /// + /// Note this case never reaches the flush at all — it covers the window in + /// which the `ESC` is merely *buffered*. The leak the issue reports happens + /// once the flush has fired; that is + /// `flushed_escape_does_not_leak_a_split_mouse_report_as_keys` below. #[test] fn split_x10_mouse_across_reads_is_one_mouse_event_not_leaked_keys() { let pipe = Pipe::new(); @@ -372,4 +423,94 @@ mod tests { events[0], ); } + + /// #2793, the leak as actually reported: the split gap outlasts every grace + /// window, so the caller has already flushed the `ESC` as the Escape key + /// (stdin went idle) when the continuation finally arrives. The remainder + /// must not become `[ M C H 4` keystrokes — those get forwarded verbatim into + /// the focused embedded terminal's child pty and print `^[[MCH4` at the + /// user's shell prompt. + /// + /// Drives the same call sequence the event loop uses (`drain_stdin`, then + /// `flush_pending_escape` on an idle poll, then `drain_stdin` again) rather + /// than the buffered-only window, so it exercises the path that leaked. No + /// timing involved: the idle flush is invoked directly, exactly as a + /// timed-out `poll` would. + #[test] + fn flushed_escape_does_not_leak_a_split_mouse_report_as_keys() { + let pipe = Pipe::new(); + let mut reader = TtyReader::for_test(pipe.0); + + pipe.write(b"\x1b"); + reader.drain_stdin(); + + // stdin went idle for the whole grace: the caller resolves the Escape. + reader.flush_pending_escape(); + let flushed = drain_events(&mut reader); + assert!( + matches!(flushed.first(), Some(CrosstermEvent::Key(k)) if k.code == KeyCode::Esc), + "expected the Escape key on idle, got {flushed:?}", + ); + + // The continuation arrives late. It is still a mouse report. + pipe.write(b"[MCH4"); + reader.drain_stdin(); + let events = drain_events(&mut reader); + assert!( + !events.iter().any(|e| matches!( + e, + CrosstermEvent::Key(k) if matches!(k.code, KeyCode::Char(_)) + )), + "mouse report leaked as literal keystrokes: {events:?}", + ); + assert_eq!( + events.len(), + 1, + "expected just the mouse event, got {events:?}" + ); + assert!( + matches!(events[0], CrosstermEvent::Mouse(_)), + "expected the late continuation to decode as a mouse event, got {:?}", + events[0], + ); + } + + /// A terminal that confirms the kitty protocol's disambiguate mode (`CSI ? 1 u` + /// in reply to fresh's `CSI ? u` query) encodes Escape as `CSI 27 u`, so a + /// bare `ESC` is always the head of a sequence. The reader must then stop + /// resolving it on idle altogether — the guess is retired, not merely widened. + #[test] + fn confirmed_disambiguate_mode_retires_the_escape_guess() { + let pipe = Pipe::new(); + let mut reader = TtyReader::for_test(pipe.0); + + pipe.write(b"\x1b[?1u"); // the flags reply + reader.drain_stdin(); + assert!( + drain_events(&mut reader).is_empty(), + "the flags reply must not surface as input", + ); + + pipe.write(b"\x1b"); + reader.drain_stdin(); + reader.flush_pending_escape(); + assert!( + drain_events(&mut reader).is_empty(), + "a lone ESC must not resolve to the Escape key on such terminals", + ); + + pipe.write(b"[MCH4"); + reader.drain_stdin(); + let events = drain_events(&mut reader); + assert_eq!( + events.len(), + 1, + "expected just the mouse event, got {events:?}" + ); + assert!( + matches!(events[0], CrosstermEvent::Mouse(_)), + "expected a mouse event, got {:?}", + events[0], + ); + } } diff --git a/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs b/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs index ee4d56632f..362ce5b1ac 100644 --- a/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs +++ b/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs @@ -37,10 +37,16 @@ use std::time::{Duration, Instant}; const WIDTH: u16 = 120; const HEIGHT: u16 = 30; -/// Longer than the parser's escape grace window, so `flush_idle` resolves a -/// buffered `ESC` the way an idle socket does on the server. Passed as an -/// explicit `Instant` rather than slept, so the test stays time-insensitive. -const PAST_ESC_GRACE: Duration = Duration::from_millis(50); +/// Comfortably longer than the parser's escape grace window, so `flush_idle` +/// resolves a buffered `ESC` the way an idle socket does on the server. Passed +/// as an explicit `Instant` rather than slept, so the test stays +/// time-insensitive. +/// +/// Derived from the grace rather than hard-coded: it sat at exactly the default +/// window, so raising that default (as #2793 did) left this test passing only on +/// the `>=` boundary. +const PAST_ESC_GRACE: Duration = + fresh::server::input_parser::DEFAULT_ESC_GRACE.saturating_add(Duration::from_millis(10)); /// Raw bytes for the keys the report uses. A session client forwards exactly /// these; `Alt+A` is `ESC a`, Escape is a lone `ESC`, `Ctrl+P` is `0x10`. diff --git a/crates/fresh-input-parser/src/lib.rs b/crates/fresh-input-parser/src/lib.rs index f134ec7cab..42221036c6 100644 --- a/crates/fresh-input-parser/src/lib.rs +++ b/crates/fresh-input-parser/src/lib.rs @@ -93,6 +93,16 @@ enum State { /// the terminator: `ST` (`ESC \`) or a legacy `BEL`. `saw_esc` records that /// the previous byte was an `ESC`, so the next byte can complete an `ST`. StringSeq { saw_esc: bool }, + /// A buffered lone `ESC` was just resolved as the Escape key by + /// [`InputParser::flush`], but the stream may yet prove it was the head of a + /// sequence split across the flush. Only `[` is interesting here: it moves + /// to [`State::FlushedCsi`] to attempt a resync. Any other byte is + /// reprocessed from ground, exactly as if the flush had returned there. + EscapeFlushed, + /// Holding the `[` that followed a flushed `ESC`, deciding whether the + /// sequence continues into a mouse report (resync) or was really a literal + /// `[` keystroke (emit it and carry on). + FlushedCsi, } /// Incremental terminal-input parser. @@ -108,6 +118,13 @@ pub struct InputParser { /// Accumulated bracketed-paste content (including the trailing end marker /// until it is recognised and stripped). paste: Vec, + /// Flags from the most recent kitty keyboard-flags reply (`CSI ? u`), + /// awaiting collection by [`InputParser::take_keyboard_flags_reply`]. + keyboard_flags_reply: Option, + /// Whether the terminal has confirmed that it encodes the Escape key + /// unambiguously, so a lone `ESC` byte is never a key press. See + /// [`InputParser::set_escape_unambiguous`]. + escape_unambiguous: bool, } impl Default for InputParser { @@ -122,6 +139,8 @@ impl InputParser { state: State::Ground, buffer: Vec::with_capacity(32), paste: Vec::new(), + keyboard_flags_reply: None, + escape_unambiguous: false, } } @@ -149,22 +168,96 @@ impl InputParser { self.state == State::Escape } - /// Resolve a buffered lone `ESC` as a standalone Escape key press. + /// Whether [`flush`](InputParser::flush) has anything to resolve: a lone + /// `ESC` awaiting its continuation, or a `[` held back after a flushed + /// `ESC` while the parser waits to see whether a mouse report follows. /// - /// Returns the `Esc` key event (and returns to ground) when - /// [`escape_pending`](InputParser::escape_pending) is true; empty - /// otherwise. Only the `Escape` state is flushable — a partial CSI or - /// UTF-8 sequence keeps waiting, since its bytes must never surface as - /// literal keystrokes. + /// Callers cap their idle wait on this (not on + /// [`escape_pending`](InputParser::escape_pending) alone), so a held `[` + /// cannot sit in the parser for longer than one grace window. + pub fn flush_pending(&self) -> bool { + matches!( + self.state, + State::Escape | State::EscapeFlushed | State::FlushedCsi + ) + } + + /// Record whether the terminal encodes the Escape key unambiguously, i.e. + /// the kitty keyboard protocol's "disambiguate escape codes" mode is active + /// and confirmed, so Escape arrives as `CSI 27 u` rather than a bare `0x1b`. + /// + /// This is the only *structural* escape from the ambiguity that makes + /// [`flush`](InputParser::flush) necessary at all: while it holds, a lone + /// `ESC` byte can only ever be the head of a sequence — never a key press — + /// so the parser waits for the continuation indefinitely instead of guessing + /// on a timer. Callers must set it only on a *confirmed* reply (`CSI ? u` + /// with the disambiguate bit set), never on an optimistic push: a terminal + /// that silently ignored the push still sends a bare `0x1b` for Escape, and + /// on those the timer is the only option (sinelaw/fresh#2793). + pub fn set_escape_unambiguous(&mut self, unambiguous: bool) { + self.escape_unambiguous = unambiguous; + } + + /// Whether the terminal has confirmed unambiguous Escape encoding. + pub fn escape_unambiguous(&self) -> bool { + self.escape_unambiguous + } + + /// Take the flags from the most recent kitty keyboard-flags reply + /// (`CSI ? u`), if one arrived since the last call. + /// + /// Bit 0 (`0b1`) is "disambiguate escape codes"; a caller that queried with + /// `CSI ? u` uses this to decide [`set_escape_unambiguous`](InputParser::set_escape_unambiguous). + pub fn take_keyboard_flags_reply(&mut self) -> Option { + self.keyboard_flags_reply.take() + } + + /// Resolve whatever [`flush_pending`](InputParser::flush_pending) reports: + /// a buffered lone `ESC` becomes a standalone Escape key press, and a `[` + /// held after an earlier flush becomes the literal `[` keystroke it turned + /// out to be. Empty when nothing is pending. + /// + /// Only these states are flushable — a partial CSI or UTF-8 sequence keeps + /// waiting, since its bytes must never surface as literal keystrokes. + /// + /// A lone `ESC` is *not* flushed once the terminal has confirmed + /// unambiguous Escape encoding + /// ([`set_escape_unambiguous`](InputParser::set_escape_unambiguous)): there + /// the byte cannot be a key press, so flushing it could only tear a split + /// sequence apart. pub fn flush(&mut self) -> Vec { - if !self.escape_pending() { - return Vec::new(); + match self.state { + State::Escape => { + if self.escape_unambiguous { + return Vec::new(); + } + // Emitted now for a responsive Escape, but the stream may still + // reveal a continuation: park in `EscapeFlushed` so a mouse + // report split across the flush can be resynced rather than + // sprayed into a focused embedded terminal as literal keys. + self.state = State::EscapeFlushed; + vec![Event::Key(KeyEvent::new( + KeyCode::Esc, + KeyModifiers::empty(), + ))] + } + // The stream went idle again without a continuation: the `ESC` was + // genuinely the Escape key (already emitted) and nothing is held. + State::EscapeFlushed => { + self.state = State::Ground; + Vec::new() + } + // A `[` was held to see whether a mouse report followed; it did + // not, so it was a literal `[` keystroke after all. + State::FlushedCsi => { + self.state = State::Ground; + vec![Event::Key(KeyEvent::new( + KeyCode::Char('['), + KeyModifiers::empty(), + ))] + } + _ => Vec::new(), } - self.state = State::Ground; - vec![Event::Key(KeyEvent::new( - KeyCode::Esc, - KeyModifiers::empty(), - ))] } /// Process a single byte, appending any completed events to `out`. @@ -217,11 +310,73 @@ impl InputParser { continue; } } + State::EscapeFlushed => { + if !self.feed_escape_flushed(byte) { + continue; + } + } + State::FlushedCsi => { + if !self.feed_flushed_csi(byte, out) { + continue; + } + } } break; } } + /// `EscapeFlushed` state: a lone `ESC` was flushed as the Escape key and + /// this is the next byte to arrive. `[` means the flush may have split a + /// control sequence, so hold it and let [`Self::feed_flushed_csi`] decide; + /// anything else means the Escape really was standalone, so the byte is + /// reprocessed from ground (returning `false`). + fn feed_escape_flushed(&mut self, byte: u8) -> bool { + if byte == b'[' { + self.state = State::FlushedCsi; + true + } else { + self.state = State::Ground; + false + } + } + + /// `FlushedCsi` state: decide what the `[` after a flushed `ESC` belonged to. + /// + /// A mouse introducer (`M` for legacy X10, `<` for SGR) means the flush tore + /// a mouse report in half: hand the rest to the normal mouse machinery so it + /// decodes into the `Mouse` event the terminal meant to send, instead of + /// reaching a focused embedded terminal's child pty as `^[[M…` garbage + /// (sinelaw/fresh#2793). The spurious Escape key was already emitted — that + /// much is unavoidable once the timer has fired — but the report's own bytes + /// never surface as literal input. + /// + /// Anything else was a literal `[` keystroke that happened to follow an + /// Escape: emit it and reprocess this byte from ground. Deliberately narrow + /// (guessing at arbitrary CSI sequences would swallow real keystrokes); the + /// mouse case is the one that floods. + fn feed_flushed_csi(&mut self, byte: u8, out: &mut Vec) -> bool { + match byte { + b'M' => { + self.state = State::X10 { + buf: [0; 3], + have: 0, + }; + true + } + b'<' => { + self.buffer.clear(); + self.buffer.push(b'<'); + self.state = State::Csi; + true + } + _ => { + out.push(key(KeyCode::Char('['), KeyModifiers::empty())); + self.state = State::Ground; + false + } + } + } + /// Ground state. Returns `false` if the caller should reprocess `byte` /// (never happens here, but keeps the control-flow uniform). fn feed_ground(&mut self, byte: u8, out: &mut Vec) -> bool { @@ -548,6 +703,21 @@ impl InputParser { // rather than misdecoding it (the flags reply used to surface as a NUL // key). `<` is the SGR mouse introducer and is handled in the match. if matches!(params.first(), Some(b'?') | Some(b'>')) { + // The kitty keyboard-flags reply is the one device reply this parser + // acts on rather than merely discarding: it is the only way to know + // whether "disambiguate escape codes" is really active, and thus + // whether a lone `ESC` can ever be the Escape key. Recorded for + // `take_keyboard_flags_reply`; still never emitted as a key event. + if final_byte == b'u' && params.first() == Some(&b'?') { + if let Ok(flags) = std::str::from_utf8(¶ms[1..]) + .unwrap_or("") + .trim() + .parse::() + { + tracing::debug!("InputParser: kitty keyboard flags reply: {flags:#b}"); + self.keyboard_flags_reply = Some(flags); + } + } tracing::trace!( "InputParser: discarding CSI reply, final {:#04x}", final_byte diff --git a/crates/fresh-input-parser/src/tests.rs b/crates/fresh-input-parser/src/tests.rs index a54679c141..5704fea1c2 100644 --- a/crates/fresh-input-parser/src/tests.rs +++ b/crates/fresh-input-parser/src/tests.rs @@ -1329,3 +1329,141 @@ fn utf8_lead_bytes_above_rfc3629_are_not_starts() { let ev = p.parse(&[0xf5, b'a']); assert_eq!(keys(&ev), vec![(KeyCode::Char('a'), KeyModifiers::empty())]); } + +// ---- #2793: the lone-`ESC` ambiguity ---- + +#[test] +fn flushed_escape_resyncs_a_split_x10_mouse_report() { + // The reported leak: a well-formed X10 report split right after its + // introducing `ESC`, with the two halves far enough apart that the caller's + // grace window expires in between and it flushes the `ESC` as the Escape + // key. The remainder must NOT become literal `[ M C H 4` keystrokes (which + // fresh forwards into a focused embedded terminal's child pty) — it is the + // tail of a mouse report and must decode as one. + let mut p = InputParser::new(); + assert!(p.parse(b"\x1b").is_empty(), "lone ESC surfaced early"); + let flushed = p.flush(); + assert_eq!( + keys(&flushed), + vec![(KeyCode::Esc, KeyModifiers::empty())], + "the flush must still deliver a responsive Escape key", + ); + + let ev = p.parse(b"[MCH4"); + assert!( + !has_char_key(&ev), + "mouse bytes leaked as characters: {ev:?}" + ); + assert_eq!(ev.len(), 1, "expected exactly the mouse event, got {ev:?}",); + assert!( + matches!(ev[0], Event::Mouse(_)), + "expected the split report to decode as a mouse event, got {:?}", + ev[0], + ); +} + +#[test] +fn flushed_escape_resyncs_a_split_sgr_mouse_report() { + // Same, for the SGR encoding (`ESC [ < …M`). + let mut p = InputParser::new(); + p.parse(b"\x1b"); + p.flush(); + let ev = p.parse(b"[<35;41;20M"); + assert!( + !has_char_key(&ev), + "mouse bytes leaked as characters: {ev:?}" + ); + assert_eq!(ev.len(), 1, "expected exactly the mouse event, got {ev:?}"); + assert!( + matches!(ev[0], Event::Mouse(_)), + "expected a mouse event, got {:?}", + ev[0], + ); +} + +#[test] +fn literal_bracket_after_a_flushed_escape_still_arrives() { + // The resync must not eat a real `[` keystroke that happens to follow an + // Escape press. `[` is held only until the next byte disproves a mouse + // report, then emitted in order. + let mut p = InputParser::new(); + p.parse(b"\x1b"); + p.flush(); + let ev = p.parse(b"[a"); + assert_eq!( + keys(&ev), + vec![ + (KeyCode::Char('['), KeyModifiers::empty()), + (KeyCode::Char('a'), KeyModifiers::empty()), + ], + "literal `[` after Escape was dropped or reordered", + ); +} + +#[test] +fn held_bracket_is_released_when_the_stream_goes_idle() { + // `[` as the very last byte before the stream goes quiet: the caller's next + // idle flush must release it, so typing `[` right after Escape cannot be + // swallowed indefinitely. + let mut p = InputParser::new(); + p.parse(b"\x1b"); + p.flush(); + let ev = p.parse(b"["); + assert!( + ev.is_empty(), + "`[` surfaced before its fate was known: {ev:?}" + ); + assert!(p.flush_pending(), "a held `[` must keep the flush armed"); + assert_eq!( + keys(&p.flush()), + vec![(KeyCode::Char('['), KeyModifiers::empty())], + "the held `[` was never released", + ); +} + +#[test] +fn unambiguous_escape_terminals_never_flush_a_lone_esc() { + // With the kitty protocol's "disambiguate escape codes" confirmed active, + // Escape arrives as `CSI 27 u`, so a bare `0x1b` can only be the head of a + // split sequence. Flushing it there could only tear the sequence apart, so + // the parser waits for the continuation however long it takes — the leak + // becomes structurally impossible rather than merely rarer. + let mut p = InputParser::new(); + p.set_escape_unambiguous(true); + assert!(p.parse(b"\x1b").is_empty()); + assert!( + p.flush().is_empty(), + "a lone ESC must not resolve to the Escape key on such terminals", + ); + let ev = p.parse(b"[MCH4"); + assert_eq!(ev.len(), 1, "expected just the mouse event, got {ev:?}"); + assert!(matches!(ev[0], Event::Mouse(_)), "got {:?}", ev[0]); + + // And the real Escape key still arrives, as `CSI 27 u`. + let esc = p.parse(b"\x1b[27u"); + assert_eq!(keys(&esc), vec![(KeyCode::Esc, KeyModifiers::empty())]); +} + +#[test] +fn kitty_keyboard_flags_reply_is_reported_to_the_caller() { + // `CSI ? u` is still never a key event, but its flags are now + // readable so the caller can confirm disambiguate mode (bit 0) instead of + // assuming its optimistic `CSI > 1 u` push took effect. + let mut p = InputParser::new(); + assert!( + p.take_keyboard_flags_reply().is_none(), + "nothing queried yet" + ); + let ev = p.parse(b"\x1b[?1u"); + assert!(ev.is_empty(), "flags reply leaked as input: {ev:?}"); + assert_eq!(p.take_keyboard_flags_reply(), Some(1)); + assert_eq!( + p.take_keyboard_flags_reply(), + None, + "the reply must be consumed exactly once", + ); + + // A reply with the disambiguate bit clear (e.g. only "report event types"). + p.parse(b"\x1b[?2u"); + assert_eq!(p.take_keyboard_flags_reply(), Some(2)); +}