mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
fix: reject legacy profile selectors (#24059)
## Why `--profile` now selects `<name>.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.<name>]` 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 <name>` reject base user config that still selects the same legacy `profile = "<name>"` 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
This commit is contained in:
committed by
GitHub
Unverified
parent
47476e8a8a
commit
932f72c225
@@ -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 <name>` with `<name>.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)
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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);
|
||||
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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::<CodexToolCallParam>(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();
|
||||
|
||||
Reference in New Issue
Block a user