From 932f72c225889102257493f57460251016cbfdc2 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Fri, 22 May 2026 13:19:47 +0200 Subject: [PATCH] fix: reject legacy profile selectors (#24059) ## Why `--profile` now selects `.config.toml`, so the legacy `profile` selector should not be reintroduced through config write or MCP tool paths. A matching legacy selector in base user config also needs the same migration guard as a matching legacy `[profiles.]` table so profile loading fails with one clear migration error instead of mixing the old and new profile models. ## What - reject non-null app-server config writes to the top-level legacy `profile` selector - make `--profile ` reject base user config that still selects the same legacy `profile = ""` value, alongside the existing matching legacy profile-table guard - reject removed MCP `codex` tool fields such as `profile` by denying unknown tool-call parameters and exposing that restriction in the generated schema - add regression coverage for the app-server write paths, config loader guard, and MCP tool input/schema behavior ## Verification - targeted regression tests cover the new app-server, config loader, and MCP rejection paths --- .../app-server/src/config_manager_service.rs | 7 ++ .../src/config_manager_service_tests.rs | 74 +++++++++++++++++++ codex-rs/config/src/loader/mod.rs | 33 +++++---- codex-rs/config/src/loader/tests.rs | 55 ++++++++++++++ codex-rs/mcp-server/src/codex_tool_config.rs | 27 ++++++- 5 files changed, 180 insertions(+), 16 deletions(-) diff --git a/codex-rs/app-server/src/config_manager_service.rs b/codex-rs/app-server/src/config_manager_service.rs index 4255b83e6..2f3cc5ef9 100644 --- a/codex-rs/app-server/src/config_manager_service.rs +++ b/codex-rs/app-server/src/config_manager_service.rs @@ -238,6 +238,13 @@ impl ConfigManager { let segments = parse_key_path(&key_path).map_err(|message| { ConfigManagerError::write(ConfigWriteErrorCode::ConfigValidationError, message) })?; + if matches!(segments.as_slice(), [segment] if segment == "profile") && !value.is_null() + { + return Err(ConfigManagerError::write( + ConfigWriteErrorCode::ConfigValidationError, + "`profile` is a legacy config selector and can no longer be written; use `--profile ` with `.config.toml` instead", + )); + } let original_value = value_at_path(&user_config, &segments).cloned(); let parsed_value = parse_value(value).map_err(|message| { ConfigManagerError::write(ConfigWriteErrorCode::ConfigValidationError, message) diff --git a/codex-rs/app-server/src/config_manager_service_tests.rs b/codex-rs/app-server/src/config_manager_service_tests.rs index c1a081e02..be35a1977 100644 --- a/codex-rs/app-server/src/config_manager_service_tests.rs +++ b/codex-rs/app-server/src/config_manager_service_tests.rs @@ -130,6 +130,80 @@ async fn clear_missing_nested_config_is_noop() -> Result<()> { Ok(()) } +#[tokio::test] +async fn write_value_rejects_legacy_profile_selector() -> Result<()> { + let tmp = tempdir().expect("tempdir"); + let path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&path, "model = \"gpt-main\"\n")?; + + let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); + let error = service + .write_value(ConfigValueWriteParams { + file_path: Some(path.display().to_string()), + key_path: "profile".to_string(), + value: serde_json::json!("work"), + merge_strategy: MergeStrategy::Replace, + expected_version: None, + }) + .await + .expect_err("legacy profile selector write should fail"); + + assert_eq!( + error.write_error_code(), + Some(ConfigWriteErrorCode::ConfigValidationError) + ); + assert!( + error + .to_string() + .contains("`profile` is a legacy config selector"), + "{error}" + ); + assert_eq!(std::fs::read_to_string(&path)?, "model = \"gpt-main\"\n"); + Ok(()) +} + +#[tokio::test] +async fn batch_write_rejects_legacy_profile_selector() -> Result<()> { + let tmp = tempdir().expect("tempdir"); + let path = tmp.path().join(CONFIG_TOML_FILE); + std::fs::write(&path, "model = \"gpt-main\"\n")?; + + let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf()); + let error = service + .batch_write(ConfigBatchWriteParams { + edits: vec![ + codex_app_server_protocol::ConfigEdit { + key_path: "model".to_string(), + value: serde_json::json!("gpt-work"), + merge_strategy: MergeStrategy::Replace, + }, + codex_app_server_protocol::ConfigEdit { + key_path: "profile".to_string(), + value: serde_json::json!("work"), + merge_strategy: MergeStrategy::Replace, + }, + ], + file_path: Some(path.display().to_string()), + expected_version: None, + reload_user_config: false, + }) + .await + .expect_err("legacy profile selector batch write should fail"); + + assert_eq!( + error.write_error_code(), + Some(ConfigWriteErrorCode::ConfigValidationError) + ); + assert!( + error + .to_string() + .contains("`profile` is a legacy config selector"), + "{error}" + ); + assert_eq!(std::fs::read_to_string(&path)?, "model = \"gpt-main\"\n"); + Ok(()) +} + #[tokio::test] async fn write_value_supports_nested_app_paths() -> Result<()> { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index 528dedaba..e8972ce5a 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -225,21 +225,26 @@ pub async fn load_config_layers_state( ) .await?; if let Some(active_user_profile) = active_user_profile.as_ref() - && base_user_layer.config.as_table().is_some_and(|config| { - config - .get("profiles") - .and_then(TomlValue::as_table) - .is_some_and(|profiles| profiles.contains_key(active_user_profile.as_str())) - }) + && let Some(base_user_config) = base_user_layer.config.as_table() { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - format!( - "--profile `{active_user_profile}` cannot be used while {} contains legacy `[profiles.{active_user_profile}]` config; move those settings into {} or remove `[profiles.{active_user_profile}]`. See https://developers.openai.com/codex/config-advanced#profiles for more information.", - base_user_file.as_path().display(), - active_user_file.as_path().display() - ), - )); + let legacy_profile_is_selected = base_user_config + .get("profile") + .and_then(TomlValue::as_str) + .is_some_and(|profile| profile == active_user_profile.as_str()); + let legacy_profile_table_exists = base_user_config + .get("profiles") + .and_then(TomlValue::as_table) + .is_some_and(|profiles| profiles.contains_key(active_user_profile.as_str())); + if legacy_profile_is_selected || legacy_profile_table_exists { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "--profile `{active_user_profile}` cannot be used while {} contains legacy `profile = \"{active_user_profile}\"` or `[profiles.{active_user_profile}]` config; move those settings into {} and remove the legacy profile selector/table. See https://developers.openai.com/codex/config-advanced#profiles for more information.", + base_user_file.as_path().display(), + active_user_file.as_path().display() + ), + )); + } } layers.push(base_user_layer); diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 812a86e25..2c87e1381 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -137,6 +137,61 @@ model = "gpt-work" ); } +#[tokio::test] +async fn profile_v2_rejects_matching_legacy_profile_selector_in_base_user_config() { + let tmp = tempdir().expect("tempdir"); + let selected_config = tmp.path().join("work.config.toml"); + + std::fs::write( + tmp.path().join(CONFIG_TOML_FILE), + r#" +profile = "work" +model = "gpt-main" +"#, + ) + .expect("write default user config"); + std::fs::write(&selected_config, r#"model = "gpt-work-v2""#) + .expect("write selected user config"); + + let mut overrides = LoaderOverrides::without_managed_config_for_tests(); + overrides.user_config_path = Some(AbsolutePathBuf::resolve_path_against_base( + "work.config.toml", + tmp.path(), + )); + overrides.user_config_profile = Some("work".parse().expect("profile-v2 name")); + + let err = load_config_layers_state( + &TestFileSystem, + tmp.path(), + /*cwd*/ None, + &[], + overrides, + CloudRequirementsLoader::default(), + &crate::NoopThreadConfigLoader, + ) + .await + .expect_err("profile-v2 should reject a matching legacy profile selector"); + + assert_eq!( + err.kind(), + io::ErrorKind::InvalidData, + "a matching legacy profile selector should be a hard config error" + ); + let message = err.to_string(); + assert!( + message.contains("--profile `work` cannot be used"), + "unexpected error message: {message}" + ); + assert!( + message.contains("profile = \"work\""), + "unexpected error message: {message}" + ); + assert!( + message.contains("work.config.toml"), + "unexpected error message: {message}" + ); +} + #[tokio::test] async fn profile_v2_allows_unrelated_legacy_profiles_in_base_user_config() { let tmp = tempdir().expect("tempdir"); diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index d91d261fb..9c9a3da53 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -20,7 +20,8 @@ use std::sync::Arc; /// Client-supplied configuration for a `codex` tool-call. #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, Default)] -#[serde(rename_all = "kebab-case")] +#[serde(rename_all = "kebab-case", deny_unknown_fields)] +#[schemars(deny_unknown_fields)] pub struct CodexToolCallParam { /// The *initial user prompt* to start the Codex conversation. pub prompt: String, @@ -271,7 +272,14 @@ fn create_tool_input_schema( // in case any `$ref` leaks into the generated schema (even though we try // to inline subschemas). let mut input_schema = JsonObject::new(); - for key in ["properties", "required", "type", "$defs", "definitions"] { + for key in [ + "additionalProperties", + "properties", + "required", + "type", + "$defs", + "definitions", + ] { if let Some(value) = schema_object.remove(key) { input_schema.insert(key.to_string(), value); } @@ -303,6 +311,7 @@ mod tests { let expected_tool_json = serde_json::json!({ "description": "Run a Codex session. Accepts configuration parameters matching the Codex Config struct.", "inputSchema": { + "additionalProperties": false, "properties": { "approval-policy": { "description": "Approval policy for shell commands generated by the model: `untrusted`, `on-failure`, `on-request`, `never`.", @@ -379,6 +388,20 @@ mod tests { assert_eq!(expected_tool_json, tool_json); } + #[test] + fn codex_tool_call_param_rejects_removed_profile_field() { + let err = serde_json::from_value::(serde_json::json!({ + "prompt": "hello", + "profile": "work" + })) + .expect_err("removed profile field should fail"); + + assert!( + err.to_string().contains("unknown field `profile`"), + "unexpected error: {err}" + ); + } + #[test] fn verify_codex_tool_reply_json_schema() { let tool = create_tool_for_codex_tool_call_reply_param();