Skip to content

Commit a73403a

Browse files
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`
1 parent 87d0cf1 commit a73403a

2 files changed

Lines changed: 30 additions & 8 deletions

File tree

codex-rs/app-server/src/config_manager_service.rs

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -244,10 +244,6 @@ impl ConfigManager {
244244

245245
apply_merge(&mut user_config, &segments, parsed_value.as_ref(), strategy).map_err(
246246
|err| match err {
247-
MergeError::PathNotFound => ConfigManagerError::write(
248-
ConfigWriteErrorCode::ConfigPathNotFound,
249-
"Path not found",
250-
),
251247
MergeError::Validation(message) => ConfigManagerError::write(
252248
ConfigWriteErrorCode::ConfigValidationError,
253249
message,
@@ -413,7 +409,6 @@ fn parse_key_path(path: &str) -> Result<Vec<String>, String> {
413409

414410
#[derive(Debug)]
415411
enum MergeError {
416-
PathNotFound,
417412
Validation(String),
418413
}
419414

@@ -485,14 +480,17 @@ fn clear_path(root: &mut TomlValue, segments: &[String]) -> Result<bool, MergeEr
485480
for segment in parents {
486481
match current {
487482
TomlValue::Table(table) => {
488-
current = table.get_mut(segment).ok_or(MergeError::PathNotFound)?;
483+
let Some(next) = table.get_mut(segment) else {
484+
return Ok(false);
485+
};
486+
current = next;
489487
}
490-
_ => return Err(MergeError::PathNotFound),
488+
_ => return Ok(false),
491489
}
492490
}
493491

494492
let Some(parent) = current.as_table_mut() else {
495-
return Err(MergeError::PathNotFound);
493+
return Ok(false);
496494
};
497495

498496
Ok(parent.remove(last).is_some())

codex-rs/app-server/src/config_manager_service_tests.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,30 @@ personality = true
106106
Ok(())
107107
}
108108

109+
#[tokio::test]
110+
async fn clear_missing_nested_config_is_noop() -> Result<()> {
111+
let tmp = tempdir().expect("tempdir");
112+
let path = tmp.path().join(CONFIG_TOML_FILE);
113+
std::fs::write(&path, "")?;
114+
115+
let service = ConfigManager::without_managed_config_for_tests(tmp.path().to_path_buf());
116+
let response = service
117+
.write_value(ConfigValueWriteParams {
118+
file_path: Some(path.display().to_string()),
119+
key_path: "features.personality".to_string(),
120+
value: serde_json::Value::Null,
121+
merge_strategy: MergeStrategy::Replace,
122+
expected_version: None,
123+
})
124+
.await
125+
.expect("clear missing config succeeds");
126+
127+
assert_eq!(response.status, WriteStatus::Ok);
128+
assert_eq!(response.overridden_metadata, None);
129+
assert_eq!(std::fs::read_to_string(&path)?, "");
130+
Ok(())
131+
}
132+
109133
#[tokio::test]
110134
async fn write_value_supports_nested_app_paths() -> Result<()> {
111135
let tmp = tempdir().expect("tempdir");

0 commit comments

Comments
 (0)