diff --git a/crates/fresh-editor/src/input/keybindings.rs b/crates/fresh-editor/src/input/keybindings.rs index 9518469cdf..d41d71dedf 100644 --- a/crates/fresh-editor/src/input/keybindings.rs +++ b/crates/fresh-editor/src/input/keybindings.rs @@ -1662,6 +1662,16 @@ impl BindingSource { } } +/// Log that a configured keybinding entry was dropped because its key name did +/// not parse, so a rejected binding leaves a trace in the log instead of dying +/// silently (issue #1128: `"key": "asterisk"` was ignored with no feedback +/// anywhere). +fn warn_invalid_key(key: &str, action: &str) { + tracing::warn!( + "Invalid keybinding in config: unknown key \"{key}\" for action \"{action}\" (binding ignored)" + ); +} + impl KeybindingResolver { /// Create a new resolver from configuration pub fn new(config: &Config) -> Self { @@ -1732,6 +1742,7 @@ impl KeybindingResolver { sequence.push((key_code, modifiers)); } else { // Invalid key in sequence, skip this binding + warn_invalid_key(&key_press.key, &binding.action); break; } } @@ -1755,6 +1766,8 @@ impl KeybindingResolver { action, &binding.key, ); + } else { + warn_invalid_key(&binding.key, &binding.action); } } } @@ -1824,6 +1837,7 @@ impl KeybindingResolver { sequence.push((key_code, modifiers)); } else { // Invalid key in sequence, skip this binding + warn_invalid_key(&key_press.key, &binding.action); break; } } @@ -1842,6 +1856,8 @@ impl KeybindingResolver { .entry(context) .or_default() .insert((key_code, modifiers), action); + } else { + warn_invalid_key(&binding.key, &binding.action); } } } @@ -3324,6 +3340,64 @@ mod tests { assert_eq!(action.to_qualified_action_str(), "menu_open:Edit"); } + /// Issue #1128: a keybinding whose key name doesn't parse (e.g. + /// "asterisk", "kp_multiply") is rejected — the entry must be dropped + /// (binding nothing) while a `tracing::warn!` at load time names the key + /// and action, and the rest of the config must still load. The warning + /// itself is emitted by `warn_invalid_key`; here we assert the + /// dropped-but-load-continues behavior. + #[test] + fn test_unknown_key_name_entry_is_dropped_but_load_continues() { + let mut config = Config::default(); + config.keybindings.push(crate::config::Keybinding { + key: "asterisk".to_string(), + modifiers: vec!["ctrl".to_string()], + keys: Vec::new(), + action: "duplicate_line".to_string(), + args: HashMap::new(), + when: None, + }); + // Same failure inside a chord sequence. + config.keybindings.push(crate::config::Keybinding { + key: String::new(), + modifiers: Vec::new(), + keys: vec![ + crate::config::KeyPress { + key: "x".to_string(), + modifiers: vec!["ctrl".to_string()], + }, + crate::config::KeyPress { + key: "kp_multiply".to_string(), + modifiers: Vec::new(), + }, + ], + action: "save".to_string(), + args: HashMap::new(), + when: None, + }); + // A valid entry after the bad ones: loading must not abort mid-config. + config.keybindings.push(crate::config::Keybinding { + key: "f6".to_string(), + modifiers: Vec::new(), + keys: Vec::new(), + action: "save".to_string(), + args: HashMap::new(), + when: None, + }); + let resolver = KeybindingResolver::new(&config); + + // Only the valid entry produced custom bindings; both bad entries were + // dropped rather than half-registered. + let custom_single: usize = resolver.bindings.values().map(|m| m.len()).sum(); + assert_eq!(custom_single, 1, "bindings: {:?}", resolver.bindings); + let custom_chords: usize = resolver.chord_bindings.values().map(|m| m.len()).sum(); + assert_eq!(custom_chords, 0, "chords: {:?}", resolver.chord_bindings); + + // The valid entry still resolves. + let event = KeyEvent::new(KeyCode::F(6), KeyModifiers::empty()); + assert_eq!(resolver.resolve(&event, KeyContext::Normal), Action::Save); + } + #[test] fn test_resolve_basic() { let config = Config::default(); diff --git a/crates/fresh-editor/src/server/input_parser.rs b/crates/fresh-editor/src/server/input_parser.rs index e34c34363f..ec53256930 100644 --- a/crates/fresh-editor/src/server/input_parser.rs +++ b/crates/fresh-editor/src/server/input_parser.rs @@ -89,3 +89,49 @@ impl Default for ClientInputParser { Self::new() } } + +#[cfg(test)] +mod tests { + use super::*; + use crossterm::event::{KeyCode, KeyModifiers}; + + /// #2930, session-client path: a legacy terminal's Alt+] arrives as + /// `ESC ]` — identical to the OSC introducer. With no continuation inside + /// the grace window it must resolve to Alt+], not swallow all further + /// input as OSC content. (Alt+[ / `ESC [` is the CSI-introducer twin.) + #[test] + fn lone_osc_and_csi_introducers_resolve_to_alt_brackets_after_grace() { + for (bytes, chr) in [(&b"\x1b]"[..], ']'), (&b"\x1b["[..], '[')] { + let mut p = ClientInputParser::new(); + let before = Instant::now(); + assert!(p.parse(bytes).is_empty()); + // Same tick (grace not elapsed): still ambiguous, nothing emitted. + assert!(p.flush_idle(before).is_empty()); + // Grace elapsed with no continuation: the legacy Alt chord. + let events = p.flush_idle(Instant::now() + ESC_GRACE); + assert!( + matches!( + events.as_slice(), + [Event::Key(k)] + if k.code == KeyCode::Char(chr) && k.modifiers == KeyModifiers::ALT, + ), + "expected Alt+{chr}, got {events:?}", + ); + // Spent: nothing further to flush. + assert!(p.flush_idle(Instant::now() + ESC_GRACE).is_empty()); + } + } + + /// The guard: an OSC reply whose payload arrives (even on a later parse + /// call) before the stream goes idle is committed as a string sequence — + /// no amount of idling may tear it into keystrokes. + #[test] + fn osc_reply_split_across_parses_stays_swallowed() { + let mut p = ClientInputParser::new(); + assert!(p.parse(b"\x1b]").is_empty()); + assert!(p.parse(b"52;c;SGVsbG8=").is_empty()); + assert!(p.flush_idle(Instant::now() + ESC_GRACE).is_empty()); + assert!(p.parse(b"\x1b\\").is_empty()); + assert!(p.flush_idle(Instant::now() + ESC_GRACE).is_empty()); + } +} diff --git a/crates/fresh-editor/src/services/tty_input.rs b/crates/fresh-editor/src/services/tty_input.rs index 9f97f1dee2..865bff6daa 100644 --- a/crates/fresh-editor/src/services/tty_input.rs +++ b/crates/fresh-editor/src/services/tty_input.rs @@ -372,4 +372,67 @@ mod tests { events[0], ); } + + /// #2930: a legacy terminal transmits Alt+] as `ESC ]` and Alt+[ as + /// `ESC [` — byte-identical to the OSC/CSI introducers. With nothing + /// following, the idle flush must resolve them to the Alt chords instead + /// of swallowing all further input (OSC) or misreading the next key as a + /// CSI final byte. + #[test] + fn lone_osc_and_csi_introducers_resolve_to_alt_brackets_on_idle() { + use crossterm::event::KeyModifiers; + for (bytes, chr) in [(&b"\x1b]"[..], ']'), (&b"\x1b["[..], '[')] { + let pipe = Pipe::new(); + let mut reader = TtyReader::for_test(pipe.0); + + pipe.write(bytes); + reader.drain_stdin(); + assert!( + drain_events(&mut reader).is_empty(), + "introducer must stay buffered while a payload could follow", + ); + + // Stream went idle: the introducer is a legacy Alt chord. + reader.flush_pending_escape(); + let events = drain_events(&mut reader); + assert!( + matches!( + events.as_slice(), + [InputEvent::Key(k)] + if k.code == KeyCode::Char(chr) && k.modifiers == KeyModifiers::ALT, + ), + "expected Alt+{chr}, got {events:?}", + ); + + // Typing afterwards works normally (nothing is swallowed). + pipe.write(b"x"); + reader.drain_stdin(); + let events = drain_events(&mut reader); + assert!( + matches!( + events.as_slice(), + [InputEvent::Key(k)] if k.code == KeyCode::Char('x'), + ), + "expected literal 'x', got {events:?}", + ); + } + } + + /// The counterpart guard: an OSC reply whose payload arrives on a later + /// read (no idle in between) is still swallowed whole, never emitted. + #[test] + fn osc_reply_split_across_reads_is_still_swallowed() { + let pipe = Pipe::new(); + let mut reader = TtyReader::for_test(pipe.0); + + pipe.write(b"\x1b]"); + // The payload is already in the pipe when drain_stdin polls, so the + // grace-window read pulls it in and the reply is consumed whole. + pipe.write(b"11;rgb:2e2e/3434/3636\x07"); + reader.drain_stdin(); + assert!( + drain_events(&mut reader).is_empty(), + "OSC reply must be swallowed, not emitted", + ); + } } diff --git a/crates/fresh-input-parser/src/lib.rs b/crates/fresh-input-parser/src/lib.rs index 2bb1a337f6..41ba7588f7 100644 --- a/crates/fresh-input-parser/src/lib.rs +++ b/crates/fresh-input-parser/src/lib.rs @@ -214,7 +214,18 @@ enum State { /// APC (`ESC _`), PM (`ESC ^`) or SOS (`ESC X`). Content is discarded until /// 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 }, + /// + /// `pending_alt` holds the introducer byte for as long as *nothing* has + /// followed it — the window in which the two-byte prefix is still ambiguous + /// between a real string sequence and a legacy-encoded Alt chord (a + /// terminal without the kitty protocol transmits Alt+] as exactly `ESC ]`). + /// The first body byte clears it: from then on this is unambiguously a + /// string sequence and [`InputParser::flush`] must not tear it apart. See + /// [`InputParser::escape_pending`]. + StringSeq { + saw_esc: bool, + pending_alt: Option, + }, } /// Incremental terminal-input parser. @@ -261,32 +272,65 @@ impl InputParser { events } - /// True while a lone `ESC` is buffered, waiting for the next byte to say - /// whether it was the Escape key or the head of an escape sequence. + /// True while an ambiguous escape prefix is buffered — bytes that read as + /// the head of an escape sequence but equally as key press(es) a legacy + /// terminal encodes with an `ESC` prefix. The next byte (or an idle + /// [`flush`](InputParser::flush)) disambiguates. The ambiguous prefixes: + /// + /// * a lone `ESC` — the Escape key, or the head of any sequence; + /// * `ESC [` with no parameter byte yet — Alt+[ on a legacy terminal, or a + /// CSI introducer (sinelaw/fresh#2930); + /// * a string introducer (`ESC ]`, `ESC P`, `ESC _`, `ESC ^`, `ESC X`) + /// with no body byte yet — Alt+] (etc.) on a legacy terminal, or the + /// start of an OSC/DCS/APC/PM/SOS string (sinelaw/fresh#2930). /// /// A caller reading from a live tty uses this to decide when to - /// [`flush`](InputParser::flush): a standalone Escape has no continuation, - /// so once no more input arrives the ambiguity resolves to the key press. + /// [`flush`](InputParser::flush): a genuine control sequence's payload + /// always follows its introducer immediately (same write burst, so at most + /// one read-boundary/grace-window away), while a human Alt chord arrives + /// alone. Once no more input arrives the ambiguity resolves to the key + /// press. As soon as any payload byte follows, this returns `false` and + /// the sequence is committed: it can then only complete or resync, never + /// surface as keystrokes. pub fn escape_pending(&self) -> bool { - self.state == State::Escape + match self.state { + State::Escape => true, + // `ESC [` alone: params live in `buffer` (cleared on entry), so an + // empty buffer means nothing has followed the introducer yet. + State::Csi => self.buffer.is_empty(), + State::StringSeq { + saw_esc: false, + pending_alt: Some(_), + } => true, + _ => false, + } } - /// Resolve a buffered lone `ESC` as a standalone Escape key press. + /// Resolve a buffered ambiguous escape prefix as the key press(es) a + /// legacy terminal means by it: a lone `ESC` is the Escape key, `ESC [` is + /// Alt+[, and a bare string introducer `ESC ]` / `ESC P` / `ESC _` / + /// `ESC ^` / `ESC X` is Alt + that character (sinelaw/fresh#2930). /// - /// Returns the `Esc` key event (and returns to ground) when + /// Returns the resolved 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. + /// otherwise. Only the ambiguous prefixes are flushable — a CSI with + /// parameters, a string sequence with body bytes, or a partial UTF-8 + /// character keeps waiting, since its bytes must never surface as literal + /// keystrokes. pub fn flush(&mut self) -> Vec { - if !self.escape_pending() { - return Vec::new(); - } + let key = match self.state { + State::Escape => KeyEvent::new(KeyCode::Esc, KeyModifiers::empty()), + State::Csi if self.buffer.is_empty() => { + KeyEvent::new(KeyCode::Char('['), KeyModifiers::ALT) + } + State::StringSeq { + saw_esc: false, + pending_alt: Some(introducer), + } => KeyEvent::new(byte_to_keycode(introducer), KeyModifiers::ALT), + _ => return Vec::new(), + }; self.state = State::Ground; - vec![Event::key(KeyEvent::new( - KeyCode::Esc, - KeyModifiers::empty(), - ))] + vec![Event::key(key)] } /// Process a single byte, appending any completed events to `out`. @@ -334,7 +378,7 @@ impl InputParser { continue; } } - State::StringSeq { saw_esc } => { + State::StringSeq { saw_esc, .. } => { if !self.feed_string(byte, saw_esc) { continue; } @@ -383,8 +427,18 @@ impl InputParser { // discarded until `ST`/`BEL`; without this arm the introducer and // its whole payload leaked out as `Alt+` plus literal // characters (e.g. an OSC 52 clipboard or OSC 10/11 colour reply). + // + // The introducer is remembered in `pending_alt` because the prefix + // is still ambiguous until a body byte arrives: a legacy terminal + // transmits Alt+] as exactly `ESC ]`, so a bare introducer with no + // continuation must resolve to the Alt chord on idle `flush()` + // rather than swallow all further input as string content + // (sinelaw/fresh#2930). b'P' | b']' | b'_' | b'^' | b'X' => { - self.state = State::StringSeq { saw_esc: false }; + self.state = State::StringSeq { + saw_esc: false, + pending_alt: Some(byte), + }; } 0x1b => { // First ESC was standalone; stay in Escape for the second one. @@ -610,8 +664,21 @@ impl InputParser { } else { match byte { 0x07 => self.state = State::Ground, // BEL: legacy OSC terminator - 0x1b => self.state = State::StringSeq { saw_esc: true }, - _ => {} // discard content byte + 0x1b => { + self.state = State::StringSeq { + saw_esc: true, + pending_alt: None, + } + } + _ => { + // Discard the content byte. Any body byte commits the + // prefix as a real string sequence: it is no longer + // flushable as an Alt chord. + self.state = State::StringSeq { + saw_esc: false, + pending_alt: None, + }; + } } true } diff --git a/crates/fresh-input-parser/src/tests.rs b/crates/fresh-input-parser/src/tests.rs index c9558a5b7a..d7f93a5ac2 100644 --- a/crates/fresh-input-parser/src/tests.rs +++ b/crates/fresh-input-parser/src/tests.rs @@ -626,6 +626,128 @@ fn flush_never_breaks_up_a_partial_sequence() { } } +// ---- Lone escape prefixes vs. legacy Alt chords (sinelaw/fresh#2930) ---- +// +// A terminal without the kitty keyboard protocol transmits Alt+] as `ESC ]` +// and Alt+[ as `ESC [` — byte-identical to the OSC and CSI introducers. A +// genuine control sequence always has its payload right behind the introducer +// (same write burst), while a human Alt chord arrives alone, so a bare +// introducer with nothing following resolves to the Alt chord on idle flush — +// exactly the rule that already resolved a bare `ESC` to the Escape key. + +#[test] +fn lone_esc_rbracket_flushes_to_alt_rbracket() { + let mut p = InputParser::new(); + // Legacy Alt+]: `ESC ]` with no continuation. + assert!(p.parse(b"\x1b]").is_empty()); + assert!(p.escape_pending(), "bare OSC introducer must be flushable"); + assert_eq!( + keys(&p.flush()), + vec![(KeyCode::Char(']'), KeyModifiers::ALT)] + ); + // Back at ground: flush is spent and typing works normally. + assert!(!p.escape_pending()); + assert!(p.flush().is_empty()); + assert_eq!( + keys(&p.parse(b"a")), + vec![(KeyCode::Char('a'), KeyModifiers::empty())] + ); +} + +#[test] +fn lone_esc_lbracket_flushes_to_alt_lbracket() { + let mut p = InputParser::new(); + // Legacy Alt+[: `ESC [` with no continuation. + assert!(p.parse(b"\x1b[").is_empty()); + assert!(p.escape_pending(), "bare CSI introducer must be flushable"); + assert_eq!( + keys(&p.flush()), + vec![(KeyCode::Char('['), KeyModifiers::ALT)] + ); + // The next typed key stands alone — it must NOT be misread as a CSI + // final byte (`A` after a swallowed `ESC [` used to become the Up key). + assert!(!p.escape_pending()); + assert_eq!( + keys(&p.parse(b"A")), + vec![(KeyCode::Char('A'), KeyModifiers::empty())] + ); +} + +#[test] +fn lone_string_introducers_flush_to_alt_chords() { + // The other legacy Alt chords that collide with string introducers: + // Alt+Shift+P (`ESC P` = DCS), Alt+_ (APC), Alt+^ (PM), Alt+Shift+X (SOS). + for (introducer, chr) in [(b'P', 'P'), (b'_', '_'), (b'^', '^'), (b'X', 'X')] { + let mut p = InputParser::new(); + assert!(p.parse(&[0x1b, introducer]).is_empty()); + assert!(p.escape_pending(), "bare `ESC {chr}` must be flushable"); + assert_eq!( + keys(&p.flush()), + vec![(KeyCode::Char(chr), KeyModifiers::ALT)], + "flush of `ESC {chr}`" + ); + } +} + +#[test] +fn osc_with_payload_in_same_burst_is_not_flushable() { + // A genuine OSC reply whose terminator hasn't arrived yet: the payload + // bytes commit it as a string sequence, so it must keep swallowing — + // never resolve to Alt+] plus leaked text. + let mut p = InputParser::new(); + assert!(p.parse(b"\x1b]52;c;SGVs").is_empty()); + assert!(!p.escape_pending(), "committed OSC must not be flushable"); + assert!(p.flush().is_empty()); + // The rest of the reply arrives on a later read and is swallowed whole. + assert!(p.parse(b"bG8=\x07").is_empty()); + assert_eq!( + keys(&p.parse(b"a")), + vec![(KeyCode::Char('a'), KeyModifiers::empty())] + ); +} + +#[test] +fn osc_split_right_after_introducer_still_swallowed_when_payload_follows() { + // Read boundary lands exactly after `ESC ]` but the payload follows before + // the stream goes idle (so the caller never invokes `flush`): the reply + // must still be swallowed whole. This is the split the idle-flush rule + // must not break: flushing is the caller's idle-time decision, and mere + // parsing of the two prefix bytes must not emit anything by itself. + let mut p = InputParser::new(); + assert!(p.parse(b"\x1b]").is_empty()); + assert!(p.parse(b"11;rgb:2e2e/3434/3636\x07").is_empty()); + assert_eq!( + keys(&p.parse(b"a")), + vec![(KeyCode::Char('a'), KeyModifiers::empty())] + ); +} + +#[test] +fn csi_with_params_pending_is_not_flushable() { + // `ESC [ 1` could still become F5 (`ESC [ 15 ~`) etc. — a parameter byte + // commits the CSI, so flush must stay silent and the sequence completes. + let mut p = InputParser::new(); + assert!(p.parse(b"\x1b[1").is_empty()); + assert!(!p.escape_pending(), "committed CSI must not be flushable"); + assert!(p.flush().is_empty()); + assert_eq!( + keys(&p.parse(b"5~")), + vec![(KeyCode::F(5), KeyModifiers::empty())] + ); +} + +#[test] +fn empty_osc_terminated_in_later_chunk_is_swallowed() { + // `ESC ]` then a lone BEL in the next read: a (degenerate but genuine) + // empty OSC. As long as no flush happened in between, it is swallowed. + let mut p = InputParser::new(); + assert!(p.parse(b"\x1b]").is_empty()); + assert!(p.parse(b"\x07").is_empty()); + // Same for the ST-terminated form. + assert!(p.parse(b"\x1b]").is_empty()); + assert!(p.parse(b"\x1b\\").is_empty()); +} + #[test] fn esc_then_mouse_same_chunk() { let mut p = InputParser::new();