mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Make missing config clears no-ops (#20334)
## Why Fixes #20145. `config/value/write` treats a JSON `null` value as a request to clear the config key. Clearing a key that is already absent should be idempotent, but clearing a nested key such as `features.personality` from an empty `config.toml` returned `configPathNotFound` because `clear_path` treated the missing `features` parent table as an error. That makes app-server reset flows brittle because clients have to read first and avoid sending a clear request unless the parent path already exists. ## What Changed - Updated app-server config clearing so missing intermediate tables, or non-table parents, are treated as an unchanged no-op. - Removed the now-unreachable `MergeError::PathNotFound` path from config write merging. - Added a regression test covering `features.personality = null` against an empty user config. ## Verification - `cargo test -p codex-app-server clear_missing_nested_config_is_noop` - `cargo test -p codex-app-server` was run; the config manager unit suite passed, but one unrelated integration test failed because `turn_start_emits_thread_scoped_warning_notification_for_trimmed_skills` expected `7` trimmed skills and observed `8`. - `just fix -p codex-app-server`
This commit is contained in:
committed by
GitHub
Unverified
parent
87d0cf1a62
commit
a73403a890
@@ -244,10 +244,6 @@ impl ConfigManager {
|
||||
|
||||
apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy).map_err(
|
||||
|err| match err {
|
||||
MergeError::PathNotFound => ConfigManagerError::write(
|
||||
ConfigWriteErrorCode::ConfigPathNotFound,
|
||||
"Path not found",
|
||||
),
|
||||
MergeError::Validation(message) => ConfigManagerError::write(
|
||||
ConfigWriteErrorCode::ConfigValidationError,
|
||||
message,
|
||||
@@ -413,7 +409,6 @@ fn parse_key_path(path: &str) -> Result<Vec<String>, String> {
|
||||
|
||||
#[derive(Debug)]
|
||||
enum MergeError {
|
||||
PathNotFound,
|
||||
Validation(String),
|
||||
}
|
||||
|
||||
@@ -485,14 +480,17 @@ fn clear_path(root: &mut TomlValue, segments: &[String]) -> Result<bool, MergeEr
|
||||
for segment in parents {
|
||||
match current {
|
||||
TomlValue::Table(table) => {
|
||||
current = table.get_mut(segment).ok_or(MergeError::PathNotFound)?;
|
||||
let Some(next) = table.get_mut(segment) else {
|
||||
return Ok(false);
|
||||
};
|
||||
current = next;
|
||||
}
|
||||
_ => return Err(MergeError::PathNotFound),
|
||||
_ => return Ok(false),
|
||||
}
|
||||
}
|
||||
|
||||
let Some(parent) = current.as_table_mut() else {
|
||||
return Err(MergeError::PathNotFound);
|
||||
return Ok(false);
|
||||
};
|
||||
|
||||
Ok(parent.remove(last).is_some())
|
||||
|
||||
@@ -106,6 +106,30 @@ personality = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn clear_missing_nested_config_is_noop() -> 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 response = service
|
||||
.write_value(ConfigValueWriteParams {
|
||||
file_path: Some(path.display().to_string()),
|
||||
key_path: "features.personality".to_string(),
|
||||
value: serde_json::Value::Null,
|
||||
merge_strategy: MergeStrategy::Replace,
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect("clear missing config succeeds");
|
||||
|
||||
assert_eq!(response.status, WriteStatus::Ok);
|
||||
assert_eq!(response.overridden_metadata, None);
|
||||
assert_eq!(std::fs::read_to_string(&path)?, "");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_value_supports_nested_app_paths() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user