mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Preserve multi-agent settings across config representations (#35656)
## Why `features.multi_agent_v2` can be represented as either a legacy boolean toggle or a table with an `enabled` field and nested settings. Layering or editing configs that mix these forms could replace one form with the other and discard the enabled state or nested settings. ## What changed - Normalize boolean toggles to the table's `enabled` field when merging config layers, applying CLI overrides, and editing user or profile config. - Preserve nested multi-agent settings when toggling the feature, while keeping ordinary replacement semantics for unrelated paths. - Attribute normalized `enabled` values to the layer that supplied the boolean toggle so config write results report overrides correctly. ## Testing Added coverage for layered config, CLI overrides, config edits, app-server writes, and origin metadata using both root and profile feature paths. GitOrigin-RevId: 38b248c949b9ea5d6340a73d754f91c1834ac486
This commit is contained in:
committed by
copyberry
parent
6b23635a7e
commit
2f19a57704
@@ -583,10 +583,33 @@ fn apply_merge(
|
||||
));
|
||||
};
|
||||
|
||||
if matches!(strategy, MergeStrategy::Upsert)
|
||||
&& (shell_environment_policy_representation_switch(root, segments, value)
|
||||
|| (matches!(value_at_path(root, segments), Some(TomlValue::Table(_)))
|
||||
&& matches!(value, TomlValue::Table(_))))
|
||||
let multi_agent_v2_feature_depth = match segments {
|
||||
[features, feature, ..] if features == "features" && feature == "multi_agent_v2" => Some(2),
|
||||
[profiles, _, features, feature, ..]
|
||||
if profiles == "profiles" && features == "features" && feature == "multi_agent_v2" =>
|
||||
{
|
||||
Some(4)
|
||||
}
|
||||
_ => None,
|
||||
};
|
||||
let preserves_multi_agent_v2_feature_config =
|
||||
multi_agent_v2_feature_depth.is_some_and(|feature_depth| {
|
||||
match value_at_path(root, &segments[..feature_depth]) {
|
||||
Some(TomlValue::Boolean(_)) => {
|
||||
segments.len() > feature_depth || matches!(value, TomlValue::Table(_))
|
||||
}
|
||||
Some(TomlValue::Table(_)) => {
|
||||
segments.len() == feature_depth && matches!(value, TomlValue::Boolean(_))
|
||||
}
|
||||
_ => false,
|
||||
}
|
||||
});
|
||||
|
||||
if preserves_multi_agent_v2_feature_config
|
||||
|| matches!(strategy, MergeStrategy::Upsert)
|
||||
&& (shell_environment_policy_representation_switch(root, segments, value)
|
||||
|| (matches!(value_at_path(root, segments), Some(TomlValue::Table(_)))
|
||||
&& matches!(value, TomlValue::Table(_))))
|
||||
{
|
||||
let overlay = sparse_overlay(segments, value);
|
||||
merge_toml_values(root, &overlay);
|
||||
@@ -741,6 +764,24 @@ fn value_at_semantic_path<'a>(root: &'a TomlValue, segments: &[String]) -> Optio
|
||||
shell_environment_filter_entry(root, segments)
|
||||
.map(|(_, value)| value)
|
||||
.or_else(|| value_at_path(root, segments))
|
||||
.or_else(|| {
|
||||
let (field, parents) = segments.split_last()?;
|
||||
if field != "enabled" {
|
||||
return None;
|
||||
}
|
||||
let is_multi_agent_v2_feature = match parents {
|
||||
[features, feature] => features == "features" && feature == "multi_agent_v2",
|
||||
[profiles, _, features, feature] => {
|
||||
profiles == "profiles" && features == "features" && feature == "multi_agent_v2"
|
||||
}
|
||||
_ => false,
|
||||
};
|
||||
if !is_multi_agent_v2_feature {
|
||||
return None;
|
||||
}
|
||||
let feature = value_at_path(root, parents)?;
|
||||
matches!(feature, TomlValue::Boolean(_)).then_some(feature)
|
||||
})
|
||||
}
|
||||
|
||||
fn override_message(layer: &ConfigLayerSource) -> String {
|
||||
|
||||
@@ -1037,6 +1037,63 @@ async fn write_value_reports_managed_override() {
|
||||
assert_eq!(overridden.effective_value, serde_json::json!("never"));
|
||||
}
|
||||
|
||||
/// Legacy managed feature toggles own their normalized enabled origin and override metadata.
|
||||
#[tokio::test]
|
||||
async fn multi_agent_v2_boolean_layer_owns_enabled_origin_and_overrides() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let user_path = tmp.path().join(CONFIG_TOML_FILE);
|
||||
std::fs::write(
|
||||
&user_path,
|
||||
"[features.multi_agent_v2]\nenabled = true\nsubagent_usage_hint_text = \"keep\"\n",
|
||||
)
|
||||
.expect("user config");
|
||||
|
||||
let managed_path = tmp.path().join("managed_config.toml");
|
||||
std::fs::write(&managed_path, "[features]\nmulti_agent_v2 = false\n").expect("managed config");
|
||||
let managed_file = AbsolutePathBuf::try_from(managed_path.clone()).expect("managed file");
|
||||
let service = ConfigManager::new_for_tests(
|
||||
tmp.path().to_path_buf(),
|
||||
vec![],
|
||||
LoaderOverrides::with_managed_config_path_for_tests(managed_path),
|
||||
CloudConfigBundleLoader::default(),
|
||||
);
|
||||
|
||||
let read = service
|
||||
.read(ConfigReadParams {
|
||||
include_layers: false,
|
||||
cwd: None,
|
||||
})
|
||||
.await
|
||||
.expect("read config");
|
||||
assert_eq!(
|
||||
read.origins
|
||||
.get("features.multi_agent_v2.enabled")
|
||||
.expect("enabled origin")
|
||||
.name,
|
||||
ApiConfigLayerSource::LegacyManagedConfigTomlFromFile {
|
||||
file: managed_file.clone(),
|
||||
},
|
||||
);
|
||||
|
||||
let result = service
|
||||
.write_value(ConfigValueWriteParams {
|
||||
file_path: Some(user_path.display().to_string()),
|
||||
key_path: "features.multi_agent_v2.enabled".to_string(),
|
||||
value: serde_json::json!(true),
|
||||
merge_strategy: MergeStrategy::Upsert,
|
||||
expected_version: None,
|
||||
})
|
||||
.await
|
||||
.expect("write config");
|
||||
assert_eq!(result.status, WriteStatus::OkOverridden);
|
||||
let overridden = result.overridden_metadata.expect("overridden metadata");
|
||||
assert_eq!(
|
||||
overridden.overriding_layer.name,
|
||||
ApiConfigLayerSource::LegacyManagedConfigTomlFromFile { file: managed_file }
|
||||
);
|
||||
assert_eq!(overridden.effective_value, serde_json::json!(false));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn upsert_merges_tables_replace_overwrites() -> Result<()> {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
@@ -1229,6 +1286,69 @@ no_memories_if_mcp_or_web_search = false
|
||||
serde_json::json!({"disable_on_external_context": true}),
|
||||
r#"[memories]
|
||||
disable_on_external_context = true
|
||||
"#,
|
||||
),
|
||||
(
|
||||
r#"[features]
|
||||
multi_agent_v2 = true
|
||||
"#,
|
||||
"features.multi_agent_v2.subagent_usage_hint_text",
|
||||
serde_json::json!("Delegate carefully."),
|
||||
r#"[features.multi_agent_v2]
|
||||
enabled = true
|
||||
subagent_usage_hint_text = "Delegate carefully."
|
||||
"#,
|
||||
),
|
||||
(
|
||||
r#"[features]
|
||||
multi_agent_v2 = true
|
||||
"#,
|
||||
"features.multi_agent_v2",
|
||||
serde_json::json!({"subagent_usage_hint_text": "Delegate carefully."}),
|
||||
r#"[features.multi_agent_v2]
|
||||
enabled = true
|
||||
subagent_usage_hint_text = "Delegate carefully."
|
||||
"#,
|
||||
),
|
||||
(
|
||||
r#"[features.multi_agent_v2]
|
||||
enabled = true
|
||||
subagent_usage_hint_text = "Delegate carefully."
|
||||
"#,
|
||||
"features.multi_agent_v2",
|
||||
serde_json::json!(false),
|
||||
r#"[features.multi_agent_v2]
|
||||
enabled = false
|
||||
subagent_usage_hint_text = "Delegate carefully."
|
||||
"#,
|
||||
),
|
||||
(
|
||||
r#"[features.multi_agent_v2]
|
||||
enabled = true
|
||||
subagent_usage_hint_text = "Delegate carefully."
|
||||
"#,
|
||||
"features.multi_agent_v2",
|
||||
serde_json::Value::Null,
|
||||
"",
|
||||
),
|
||||
(
|
||||
r#"[desktop.features.multi_agent_v2]
|
||||
custom = true
|
||||
"#,
|
||||
"desktop.features.multi_agent_v2",
|
||||
serde_json::json!(false),
|
||||
r#"[desktop.features]
|
||||
multi_agent_v2 = false
|
||||
"#,
|
||||
),
|
||||
(
|
||||
r#"[desktop.features]
|
||||
multi_agent_v2 = true
|
||||
"#,
|
||||
"desktop.features.multi_agent_v2",
|
||||
serde_json::json!({"custom": true}),
|
||||
r#"[desktop.features.multi_agent_v2]
|
||||
custom = true
|
||||
"#,
|
||||
),
|
||||
];
|
||||
|
||||
Reference in New Issue
Block a user