From 819efd72732adf600c833c6b026fcfbd1e8968b7 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Thu, 1 Oct 2026 06:49:59 +0000 Subject: [PATCH] Preserve validation errors for invalid TUI keybindings (#49910) ## Why Deserializing keybindings through an untagged enum hides the specific validation error, making invalid `tui.keymap` settings harder to diagnose. ## What changed Deserialize single keybinding strings and lists by value type so validation errors reach the user. Errors now retain the reason a binding is invalid and the affected configuration key, including when loading configuration for `codex mcp list`. ## Testing Add TOML regression tests for unknown keys, invalid chord strokes, and overly long chords in both string and list forms. Add a CLI snapshot test verifying that `codex mcp list` reports the invalid keybinding reason and configuration key. GitOrigin-RevId: 26e3dea93fc43db21e993ea5cf93df53ce93a43e --- codex-rs/cli/tests/features.rs | 25 +++++++++++ ...ist_reports_invalid_keybinding_reason.snap | 10 +++++ codex-rs/config/src/tui_keymap.rs | 44 ++++++++++++++++++- codex-rs/config/src/tui_keymap_chord_tests.rs | 17 +++++++ 4 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 codex-rs/cli/tests/snapshots/features__mcp_list_reports_invalid_keybinding_reason.snap diff --git a/codex-rs/cli/tests/features.rs b/codex-rs/cli/tests/features.rs index b4e00afe96..9ce0314e32 100644 --- a/codex-rs/cli/tests/features.rs +++ b/codex-rs/cli/tests/features.rs @@ -38,6 +38,31 @@ fn strict_config_rejects_unknown_config_override() -> Result<()> { Ok(()) } +#[test] +fn mcp_list_reports_invalid_keybinding_reason() -> Result<()> { + for bindings in ["'backslash'", "['backslash']"] { + let codex_home = TempDir::new()?; + std::fs::write( + codex_home.path().join("config.toml"), + format!("[tui.keymap.editor]\ninsert_newline = {bindings}\n"), + )?; + + let output = codex_command(codex_home.path())? + .current_dir(codex_home.path()) + .args(["mcp", "list"]) + .assert() + .failure() + .get_output() + .stderr + .clone(); + let output = String::from_utf8(output)?; + insta::allow_duplicates! { + insta::assert_snapshot!(output); + } + } + Ok(()) +} + #[test] fn interactive_validates_config_before_requiring_terminal() -> Result<()> { let cases: &[(&[&str], &str, &str, &str)] = &[ diff --git a/codex-rs/cli/tests/snapshots/features__mcp_list_reports_invalid_keybinding_reason.snap b/codex-rs/cli/tests/snapshots/features__mcp_list_reports_invalid_keybinding_reason.snap new file mode 100644 index 0000000000..3fbb6017fc --- /dev/null +++ b/codex-rs/cli/tests/snapshots/features__mcp_list_reports_invalid_keybinding_reason.snap @@ -0,0 +1,10 @@ +--- +source: cli/tests/features.rs +expression: output +--- +Error: failed to load bootstrap configuration + +Caused by: + unknown key `backslash` in keybinding `backslash`. Use a printable character (for example `a`), function keys (`f1`-`f24`), or one of: enter, tab, backspace, esc, delete, arrows, home/end, page-up/page-down, space, minus. + See the Codex keymap documentation for supported actions and examples. + in `tui.keymap.editor.insert_newline` diff --git a/codex-rs/config/src/tui_keymap.rs b/codex-rs/config/src/tui_keymap.rs index d840967852..146857bbc8 100644 --- a/codex-rs/config/src/tui_keymap.rs +++ b/codex-rs/config/src/tui_keymap.rs @@ -21,7 +21,10 @@ use serde::Deserialize; use serde::Deserializer; use serde::Serialize; use serde::de::Error as SerdeError; +use serde::de::SeqAccess; +use serde::de::Visitor; use std::collections::BTreeMap; +use std::fmt; /// Highest function key supported by portable TUI keymap configuration. pub const MAX_FUNCTION_KEY: u8 = 24; @@ -67,13 +70,52 @@ impl<'de> Deserialize<'de> for KeybindingSpec { /// An empty list explicitly unbinds the action in that scope. Because an /// explicit empty list is still a configured value, runtime resolution must not /// fall through to global or built-in defaults for that action. -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[derive(Serialize, Debug, Clone, PartialEq, Eq, JsonSchema)] #[serde(untagged)] pub enum KeybindingsSpec { One(KeybindingSpec), Many(Vec), } +impl<'de> Deserialize<'de> for KeybindingsSpec { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + // Dispatch by value type so untagged enum matching cannot hide validation errors. + struct KeybindingsVisitor; + + impl<'de> Visitor<'de> for KeybindingsVisitor { + type Value = KeybindingsSpec; + + fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + formatter.write_str("a keybinding string or a list of keybinding strings") + } + + fn visit_str(self, value: &str) -> Result + where + E: SerdeError, + { + let normalized = normalize_keybinding_spec(value).map_err(E::custom)?; + Ok(KeybindingsSpec::One(KeybindingSpec(normalized))) + } + + fn visit_seq(self, mut sequence: A) -> Result + where + A: SeqAccess<'de>, + { + let mut bindings = Vec::new(); + while let Some(binding) = sequence.next_element::()? { + bindings.push(binding); + } + Ok(KeybindingsSpec::Many(bindings)) + } + } + + deserializer.deserialize_any(KeybindingsVisitor) + } +} + impl KeybindingsSpec { /// Returns all configured key specs for one action in declaration order. /// diff --git a/codex-rs/config/src/tui_keymap_chord_tests.rs b/codex-rs/config/src/tui_keymap_chord_tests.rs index 87f34588d9..d43602ea8a 100644 --- a/codex-rs/config/src/tui_keymap_chord_tests.rs +++ b/codex-rs/config/src/tui_keymap_chord_tests.rs @@ -74,3 +74,20 @@ fn rejects_an_invalid_second_chord_stroke() { assert!(error.contains("unknown key"), "{error}"); } + +#[test] +fn preserves_keybinding_validation_errors_when_deserializing_toml() { + for invalid in ["backslash", "ctrl-x ctrl-unknown", "ctrl-x ctrl-s ctrl-t"] { + for bindings in [format!("'{invalid}'"), format!("['ctrl-j', '{invalid}']")] { + let error = + toml::from_str::(&format!("[editor]\ninsert_newline = {bindings}\n")) + .expect_err("invalid bindings should retain the validation error"); + + assert_eq!( + error.message(), + normalize_keybinding_spec(invalid).unwrap_err() + ); + assert!(error.to_string().contains("insert_newline"), "{error}"); + } + } +}