mirror of
https://github.com/openai/codex.git
synced 2026-10-19 08:16:48 +00:00
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
This commit is contained in:
@@ -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)] = &[
|
||||
|
||||
+10
@@ -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`
|
||||
@@ -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<KeybindingSpec>),
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for KeybindingsSpec {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
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<E>(self, value: &str) -> Result<Self::Value, E>
|
||||
where
|
||||
E: SerdeError,
|
||||
{
|
||||
let normalized = normalize_keybinding_spec(value).map_err(E::custom)?;
|
||||
Ok(KeybindingsSpec::One(KeybindingSpec(normalized)))
|
||||
}
|
||||
|
||||
fn visit_seq<A>(self, mut sequence: A) -> Result<Self::Value, A::Error>
|
||||
where
|
||||
A: SeqAccess<'de>,
|
||||
{
|
||||
let mut bindings = Vec::new();
|
||||
while let Some(binding) = sequence.next_element::<KeybindingSpec>()? {
|
||||
bindings.push(binding);
|
||||
}
|
||||
Ok(KeybindingsSpec::Many(bindings))
|
||||
}
|
||||
}
|
||||
|
||||
deserializer.deserialize_any(KeybindingsVisitor)
|
||||
}
|
||||
}
|
||||
|
||||
impl KeybindingsSpec {
|
||||
/// Returns all configured key specs for one action in declaration order.
|
||||
///
|
||||
|
||||
@@ -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::<TuiKeymap>(&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}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user