mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
app-server: drop legacy profile config surface (#24067)
## Why Legacy `[profiles.<name>]` config tables and the legacy `profile` selector are being retired in favor of profile files selected with `--profile <name>`. After #23886 removed the CLI-side legacy profile plumbing, the app-server config surface still exposed those fields and still carried conversion code for the old protocol shape. ## What changed - Remove `profile`, `profiles`, and `ProfileV2` from the app-server config protocol/schema output so `config/read` no longer returns legacy profile config. - Drop the old v1 `UserSavedConfig` profile conversion path from `config`. - Reject new app-server config writes under `profiles.*` with the same migration direction used for `profile`, while still allowing callers to clear existing legacy profile tables. - Refresh app-server config coverage and the experimental API README example around the remaining `Config` nesting path. ## Verification - Added config-manager coverage that `config/read` omits legacy profile config, `profiles.*` writes are rejected, and existing legacy profile tables can still be cleared. - Updated the v2 config RPC test to cover the rejected `profiles.*` batch-write path.
This commit is contained in:
committed by
GitHub
Unverified
parent
c0b16cfc6b
commit
162a6e746b
@@ -1931,7 +1931,7 @@ reason up through the containing type:
|
||||
|
||||
```rust
|
||||
#[derive(ExperimentalApi)]
|
||||
struct ProfileV2 {
|
||||
struct Config {
|
||||
#[experimental(nested)]
|
||||
approval_policy: Option<AskForApproval>,
|
||||
}
|
||||
|
||||
@@ -125,7 +125,6 @@ impl ConfigManager {
|
||||
};
|
||||
|
||||
let effective = layers.effective_config();
|
||||
|
||||
let effective_config_toml: ConfigToml = effective
|
||||
.try_into()
|
||||
.map_err(|err| ConfigManagerError::toml("invalid configuration", err))?;
|
||||
@@ -238,12 +237,22 @@ 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",
|
||||
));
|
||||
if !value.is_null() {
|
||||
match segments.as_slice() {
|
||||
[segment] if segment == "profile" => {
|
||||
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",
|
||||
));
|
||||
}
|
||||
[segment, ..] if segment == "profiles" => {
|
||||
return Err(ConfigManagerError::write(
|
||||
ConfigWriteErrorCode::ConfigValidationError,
|
||||
"`profiles` contains legacy config profile tables 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| {
|
||||
|
||||
@@ -162,6 +162,38 @@ async fn write_value_rejects_legacy_profile_selector() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_rejects_legacy_profile_table() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let path = tmp.path().join(CONFIG_TOML_FILE);
|
||||
std::fs::write(&path, "")?;
|
||||
|
||||
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: "profiles.work.model".to_string(),
|
||||
value: serde_json::json!("gpt-work"),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect_err("legacy profile table write should fail");
|
||||
|
||||
assert_eq!(
|
||||
error.write_error_code(),
|
||||
Some(ConfigWriteErrorCode::ConfigValidationError)
|
||||
);
|
||||
assert!(
|
||||
error
|
||||
.to_string()
|
||||
.contains("`profiles` contains legacy config profile tables"),
|
||||
"{error}"
|
||||
);
|
||||
assert_eq!(std::fs::read_to_string(&path)?, "");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn batch_write_rejects_legacy_profile_selector() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
@@ -712,52 +744,6 @@ async fn write_value_rejects_feature_requirement_conflict() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_rejects_profile_feature_requirement_conflict() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
std::fs::write(tmp.path().join(CONFIG_TOML_FILE), "").unwrap();
|
||||
|
||||
let service = ConfigManager::new_for_tests(
|
||||
tmp.path().to_path_buf(),
|
||||
vec![],
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
CloudRequirementsLoader::new(async {
|
||||
Ok(Some(ConfigRequirementsToml {
|
||||
feature_requirements: Some(FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("personality".to_string(), true)]),
|
||||
}),
|
||||
..Default::default()
|
||||
}))
|
||||
}),
|
||||
);
|
||||
|
||||
let error = service
|
||||
.write_value(ConfigValueWriteParams {
|
||||
file_path: Some(tmp.path().join(CONFIG_TOML_FILE).display().to_string()),
|
||||
key_path: "profiles.enterprise.features.personality".to_string(),
|
||||
value: serde_json::json!(false),
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect_err("conflicting profile feature write should fail");
|
||||
|
||||
assert_eq!(
|
||||
error.write_error_code(),
|
||||
Some(ConfigWriteErrorCode::ConfigValidationError)
|
||||
);
|
||||
assert!(
|
||||
error.to_string().contains(
|
||||
"invalid value for `features`: `profiles.enterprise.features.personality=false`"
|
||||
),
|
||||
"{error}"
|
||||
);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).unwrap(),
|
||||
""
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_reports_managed_overrides_user_and_session_flags() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
@@ -894,19 +894,14 @@ async fn config_batch_write_applies_multiple_edits() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn config_batch_write_preserves_dotted_profile_names() -> Result<()> {
|
||||
async fn config_batch_write_rejects_legacy_profile_tables() -> Result<()> {
|
||||
let tmp_dir = TempDir::new()?;
|
||||
let codex_home = tmp_dir.path().canonicalize()?;
|
||||
write_config(
|
||||
&tmp_dir,
|
||||
r#"
|
||||
profile = "team.prod"
|
||||
|
||||
[profiles."team.prod"]
|
||||
model = "gpt-5.3-spark"
|
||||
|
||||
[profiles.team.prod]
|
||||
model = "should-stay-put"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
@@ -932,28 +927,30 @@ model = "should-stay-put"
|
||||
reload_user_config: false,
|
||||
})
|
||||
.await?;
|
||||
let batch_resp: JSONRPCResponse = timeout(
|
||||
let err: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(batch_id)),
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(batch_id)),
|
||||
)
|
||||
.await??;
|
||||
let batch_write: ConfigWriteResponse = to_response(batch_resp)?;
|
||||
assert_eq!(batch_write.status, WriteStatus::Ok);
|
||||
let code = err
|
||||
.error
|
||||
.data
|
||||
.as_ref()
|
||||
.and_then(|data| data.get("config_write_error_code"))
|
||||
.and_then(|value| value.as_str());
|
||||
assert_eq!(code, Some("configValidationError"));
|
||||
assert!(
|
||||
err.error.message.contains("`profiles`"),
|
||||
"unexpected error: {err:?}"
|
||||
);
|
||||
|
||||
let config: toml::Value =
|
||||
toml::from_str(&std::fs::read_to_string(codex_home.join("config.toml"))?)?;
|
||||
assert_eq!(
|
||||
config["profiles"]["team.prod"]["model"].as_str(),
|
||||
Some("gpt-5.5")
|
||||
);
|
||||
assert_eq!(
|
||||
config["profiles"]["team"]["prod"]["model"].as_str(),
|
||||
Some("should-stay-put")
|
||||
);
|
||||
assert_eq!(
|
||||
config["items"]["sample@catalog"]["enabled"].as_bool(),
|
||||
Some(true)
|
||||
Some("gpt-5.3-spark")
|
||||
);
|
||||
assert_eq!(config.get("items"), None);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user