input-parser: resolve lone ESC-]/ESC-[ to Alt+]/Alt+[ on legacy terminals; surface dropped keybinding config entries

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();

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());
+    }
+}

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",
+        );
+    }
 }

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<u8>,
+    },
 }
 
 /// 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<Event> {
-        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+<introducer>` 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
         }