mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Reject unknown TUI keys in strict config validation (#50525)
## Why Flattened notification settings prevent `serde_ignored` from detecting unknown keys in `[tui]`, allowing them to pass `--strict-config` validation. ## What changed Detect unknown TUI keys by comparing configured keys with a serialized `Tui`, keeping accepted keys derived from the configuration type. Apply this check to config files and `-c`/`--config` overrides so keys such as `tui.bogus_key_xyz` produce an unknown-field error while valid notification settings remain accepted. ## Testing Add regression coverage for unknown TUI keys, diagnostic source ranges, valid notification settings, and generic validation behavior. Extend CLI tests to cover TUI overrides and config validation before terminal checks. GitOrigin-RevId: b04467523442183e7a54415d82c3f632f97ef319
This commit is contained in:
@@ -27,13 +27,19 @@ fn codex_command(codex_home: &Path) -> Result<assert_cmd::Command> {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn strict_config_rejects_unknown_config_override() -> Result<()> {
|
fn strict_config_rejects_unknown_config_override() -> Result<()> {
|
||||||
let codex_home = TempDir::new()?;
|
for (config_override, ignored_path) in [
|
||||||
|
("foo=bar", "foo"),
|
||||||
let mut cmd = codex_command(codex_home.path())?;
|
("tui.bogus_key_xyz=1", "tui.bogus_key_xyz"),
|
||||||
cmd.args(["--strict-config", "-c", "foo=bar", "exec", "hello"])
|
] {
|
||||||
.assert()
|
let codex_home = TempDir::new()?;
|
||||||
.failure()
|
let mut cmd = codex_command(codex_home.path())?;
|
||||||
.stderr(contains("unknown configuration field"));
|
cmd.args(["--strict-config", "-c", config_override, "exec", "hello"])
|
||||||
|
.assert()
|
||||||
|
.failure()
|
||||||
|
.stderr(contains(format!(
|
||||||
|
"unknown configuration field `{ignored_path}`"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
@@ -73,6 +79,12 @@ fn interactive_validates_config_before_requiring_terminal() -> Result<()> {
|
|||||||
"unknown_key = true",
|
"unknown_key = true",
|
||||||
"unknown configuration field",
|
"unknown configuration field",
|
||||||
),
|
),
|
||||||
|
(
|
||||||
|
&["--strict-config"],
|
||||||
|
"config.toml",
|
||||||
|
"[tui]\nbogus_key_xyz = 1",
|
||||||
|
"unknown configuration field `tui.bogus_key_xyz`",
|
||||||
|
),
|
||||||
(
|
(
|
||||||
&["--strict-config", "-c", "foo=bar"],
|
&["--strict-config", "-c", "foo=bar"],
|
||||||
"config.toml",
|
"config.toml",
|
||||||
|
|||||||
@@ -45,6 +45,7 @@ use crate::strict_config::config_error_from_ignored_toml_value_fields;
|
|||||||
use crate::strict_config::ignored_config_warning;
|
use crate::strict_config::ignored_config_warning;
|
||||||
use crate::strict_config::ignored_toml_value_fields;
|
use crate::strict_config::ignored_toml_value_fields;
|
||||||
use crate::strict_config::unknown_feature_toml_value_field;
|
use crate::strict_config::unknown_feature_toml_value_field;
|
||||||
|
use crate::strict_config::unknown_tui_toml_value_path;
|
||||||
use crate::thread_config::ThreadConfigContext;
|
use crate::thread_config::ThreadConfigContext;
|
||||||
use crate::thread_config::ThreadConfigLoader;
|
use crate::thread_config::ThreadConfigLoader;
|
||||||
use codex_file_system::ExecutorFileSystem;
|
use codex_file_system::ExecutorFileSystem;
|
||||||
@@ -653,16 +654,13 @@ fn validate_cli_overrides_strictly(
|
|||||||
base_dir: &Path,
|
base_dir: &Path,
|
||||||
) -> io::Result<()> {
|
) -> io::Result<()> {
|
||||||
let _guard = AbsolutePathBufGuard::new(base_dir);
|
let _guard = AbsolutePathBufGuard::new(base_dir);
|
||||||
if let Some(path) = ignored_toml_value_fields::<ConfigToml>(cli_overrides_layer.clone()).first()
|
let ignored_path = ignored_toml_value_fields::<ConfigToml>(cli_overrides_layer.clone())
|
||||||
{
|
.into_iter()
|
||||||
let ignored_path = path.join(".");
|
.chain(unknown_tui_toml_value_path(cli_overrides_layer))
|
||||||
return Err(io::Error::new(
|
.next()
|
||||||
io::ErrorKind::InvalidData,
|
.map(|path| path.join("."))
|
||||||
format!("unknown configuration field `{ignored_path}` in -c/--config override"),
|
.or_else(|| unknown_feature_toml_value_field(cli_overrides_layer));
|
||||||
));
|
if let Some(ignored_path) = ignored_path {
|
||||||
}
|
|
||||||
|
|
||||||
if let Some(ignored_path) = unknown_feature_toml_value_field(cli_overrides_layer) {
|
|
||||||
return Err(io::Error::new(
|
return Err(io::Error::new(
|
||||||
io::ErrorKind::InvalidData,
|
io::ErrorKind::InvalidData,
|
||||||
format!("unknown configuration field `{ignored_path}` in -c/--config override"),
|
format!("unknown configuration field `{ignored_path}` in -c/--config override"),
|
||||||
|
|||||||
@@ -14,15 +14,17 @@ use crate::diagnostics::span_for_toml_key_path;
|
|||||||
use crate::diagnostics::text_range_from_span;
|
use crate::diagnostics::text_range_from_span;
|
||||||
use crate::format_config_layer_source;
|
use crate::format_config_layer_source;
|
||||||
use crate::requirements_layers::strip_cloud_auth_requirements;
|
use crate::requirements_layers::strip_cloud_auth_requirements;
|
||||||
|
use crate::types::Tui;
|
||||||
use codex_features::is_known_feature_key;
|
use codex_features::is_known_feature_key;
|
||||||
use codex_utils_absolute_path::AbsolutePathBufGuard;
|
use codex_utils_absolute_path::AbsolutePathBufGuard;
|
||||||
use serde::de::DeserializeOwned;
|
use serde::de::DeserializeOwned;
|
||||||
|
use std::any::TypeId;
|
||||||
use std::collections::BTreeSet;
|
use std::collections::BTreeSet;
|
||||||
use std::fmt::Write;
|
use std::fmt::Write;
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
use toml::Value as TomlValue;
|
use toml::Value as TomlValue;
|
||||||
|
|
||||||
pub fn config_error_from_ignored_toml_fields<T: DeserializeOwned>(
|
pub fn config_error_from_ignored_toml_fields<T: DeserializeOwned + 'static>(
|
||||||
path: impl AsRef<Path>,
|
path: impl AsRef<Path>,
|
||||||
contents: &str,
|
contents: &str,
|
||||||
) -> Option<ConfigError> {
|
) -> Option<ConfigError> {
|
||||||
@@ -35,7 +37,7 @@ pub fn config_error_from_ignored_toml_fields<T: DeserializeOwned>(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn config_error_from_ignored_toml_value_fields<T: DeserializeOwned>(
|
pub(crate) fn config_error_from_ignored_toml_value_fields<T: DeserializeOwned + 'static>(
|
||||||
path: impl AsRef<Path>,
|
path: impl AsRef<Path>,
|
||||||
contents: &str,
|
contents: &str,
|
||||||
value: TomlValue,
|
value: TomlValue,
|
||||||
@@ -47,7 +49,9 @@ pub(crate) fn config_error_from_ignored_toml_value_fields<T: DeserializeOwned>(
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name<T: DeserializeOwned>(
|
pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name<
|
||||||
|
T: DeserializeOwned + 'static,
|
||||||
|
>(
|
||||||
source_name: &str,
|
source_name: &str,
|
||||||
contents: &str,
|
contents: &str,
|
||||||
value: TomlValue,
|
value: TomlValue,
|
||||||
@@ -59,12 +63,17 @@ pub(crate) fn config_error_from_ignored_toml_value_fields_for_source_name<T: Des
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
fn config_error_from_ignored_toml_value_fields_for_source<T: DeserializeOwned>(
|
fn config_error_from_ignored_toml_value_fields_for_source<T: DeserializeOwned + 'static>(
|
||||||
source: ConfigDiagnosticSource<'_>,
|
source: ConfigDiagnosticSource<'_>,
|
||||||
contents: &str,
|
contents: &str,
|
||||||
value: TomlValue,
|
value: TomlValue,
|
||||||
) -> Option<ConfigError> {
|
) -> Option<ConfigError> {
|
||||||
let unknown_feature_paths = unknown_feature_toml_value_path(&value);
|
let unknown_feature_paths = unknown_feature_toml_value_path(&value);
|
||||||
|
let unknown_tui_paths = if TypeId::of::<T>() == TypeId::of::<ConfigToml>() {
|
||||||
|
unknown_tui_toml_value_path(&value)
|
||||||
|
} else {
|
||||||
|
Vec::new()
|
||||||
|
};
|
||||||
let mut ignored_paths = Vec::new();
|
let mut ignored_paths = Vec::new();
|
||||||
let mut ignored_callback = |ignored_path: serde_ignored::Path<'_>| {
|
let mut ignored_callback = |ignored_path: serde_ignored::Path<'_>| {
|
||||||
let path_segments = ignored_path_segments(&ignored_path);
|
let path_segments = ignored_path_segments(&ignored_path);
|
||||||
@@ -77,6 +86,7 @@ fn config_error_from_ignored_toml_value_fields_for_source<T: DeserializeOwned>(
|
|||||||
|
|
||||||
match result {
|
match result {
|
||||||
Ok(_) => unknown_field_error_from_paths(source, contents, ignored_paths)
|
Ok(_) => unknown_field_error_from_paths(source, contents, ignored_paths)
|
||||||
|
.or_else(|| unknown_field_error_from_paths(source, contents, unknown_tui_paths))
|
||||||
.or_else(|| unknown_field_error_from_paths(source, contents, unknown_feature_paths)),
|
.or_else(|| unknown_field_error_from_paths(source, contents, unknown_feature_paths)),
|
||||||
Err(err) => {
|
Err(err) => {
|
||||||
let path_hint = err.path().clone();
|
let path_hint = err.path().clone();
|
||||||
@@ -94,6 +104,29 @@ fn config_error_from_ignored_toml_value_fields_for_source<T: DeserializeOwned>(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub(crate) fn unknown_tui_toml_value_path(value: &TomlValue) -> Vec<Vec<String>> {
|
||||||
|
// `serde_ignored` cannot observe keys consumed by the flattened notification settings.
|
||||||
|
// Round-trip the TUI config so accepted input keys stay derived from `Tui` itself.
|
||||||
|
let Some(tui_value) = value.as_table().and_then(|root| root.get("tui")) else {
|
||||||
|
return Vec::new();
|
||||||
|
};
|
||||||
|
let Some(configured_tui) = tui_value.as_table() else {
|
||||||
|
return Vec::new();
|
||||||
|
};
|
||||||
|
let Ok(tui) = tui_value.clone().try_into::<Tui>() else {
|
||||||
|
return Vec::new();
|
||||||
|
};
|
||||||
|
let Ok(TomlValue::Table(serialized_tui)) = TomlValue::try_from(tui) else {
|
||||||
|
return Vec::new();
|
||||||
|
};
|
||||||
|
|
||||||
|
configured_tui
|
||||||
|
.keys()
|
||||||
|
.filter(|key| !serialized_tui.contains_key(*key))
|
||||||
|
.map(|key| vec!["tui".to_string(), key.clone()])
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
pub(crate) fn ignored_toml_value_fields<T: DeserializeOwned>(value: TomlValue) -> Vec<Vec<String>> {
|
pub(crate) fn ignored_toml_value_fields<T: DeserializeOwned>(value: TomlValue) -> Vec<Vec<String>> {
|
||||||
let mut ignored_paths = Vec::new();
|
let mut ignored_paths = Vec::new();
|
||||||
let result: Result<T, _> = serde_ignored::deserialize(value, |ignored_path| {
|
let result: Result<T, _> = serde_ignored::deserialize(value, |ignored_path| {
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ use crate::config_toml::ConfigToml;
|
|||||||
use crate::diagnostics::TextPosition;
|
use crate::diagnostics::TextPosition;
|
||||||
use crate::diagnostics::TextRange;
|
use crate::diagnostics::TextRange;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
|
use std::collections::BTreeMap;
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
@@ -91,6 +92,82 @@ foo = true"#;
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn strict_config_rejects_unknown_tui_key() {
|
||||||
|
let path = Path::new("/tmp/config.toml");
|
||||||
|
let contents = r#"
|
||||||
|
[tui]
|
||||||
|
bogus_key_xyz = 1"#;
|
||||||
|
|
||||||
|
let error = config_error_from_ignored_toml_fields::<ConfigToml>(path, contents)
|
||||||
|
.expect("unknown TUI key error");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
error,
|
||||||
|
ConfigError::new(
|
||||||
|
path.to_path_buf(),
|
||||||
|
TextRange {
|
||||||
|
start: TextPosition { line: 3, column: 1 },
|
||||||
|
end: TextPosition {
|
||||||
|
line: 3,
|
||||||
|
column: 13,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
"unknown configuration field `tui.bogus_key_xyz`",
|
||||||
|
)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn strict_config_accepts_tui_notification_settings() {
|
||||||
|
let path = Path::new("/tmp/config.toml");
|
||||||
|
|
||||||
|
for contents in [
|
||||||
|
"[tui]\nnotifications = false\n",
|
||||||
|
"[tui]\nnotification_method = \"bel\"\n",
|
||||||
|
"[tui]\nnotification_condition = \"always\"\n",
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
config_error_from_ignored_toml_fields::<ConfigToml>(path, contents),
|
||||||
|
None
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn generic_validation_does_not_apply_config_toml_tui_fields() {
|
||||||
|
let path = Path::new("/tmp/config.toml");
|
||||||
|
let contents = "[tui]\nplugin_key = 1\n";
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
config_error_from_ignored_toml_fields::<BTreeMap<String, BTreeMap<String, i32>>>(
|
||||||
|
path, contents,
|
||||||
|
),
|
||||||
|
None
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn wrapped_config_toml_still_rejects_unknown_feature_key() {
|
||||||
|
let path = Path::new("/tmp/config.toml");
|
||||||
|
let contents = "[features]\nfoo = true\n";
|
||||||
|
|
||||||
|
let error = config_error_from_ignored_toml_fields::<Box<ConfigToml>>(path, contents)
|
||||||
|
.expect("unknown feature error");
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
error,
|
||||||
|
ConfigError::new(
|
||||||
|
path.to_path_buf(),
|
||||||
|
TextRange {
|
||||||
|
start: TextPosition { line: 2, column: 1 },
|
||||||
|
end: TextPosition { line: 2, column: 3 },
|
||||||
|
},
|
||||||
|
"unknown configuration field `features.foo`",
|
||||||
|
)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn strict_config_accepts_tool_registry_config() {
|
fn strict_config_accepts_tool_registry_config() {
|
||||||
let path = Path::new("/tmp/config.toml");
|
let path = Path::new("/tmp/config.toml");
|
||||||
|
|||||||
Reference in New Issue
Block a user