mirror of
https://github.com/openai/codex.git
synced 2026-10-17 17:08:55 +00:00
Limit daemon feature compatibility checks to explicit CLI overrides (#52648)
## Why
A running shared background server owns its feature settings until it restarts. Differences from a client's configuration files or defaults should not trigger a compatibility warning or a restart request.
## What changed
- Check shared-service features only when explicitly overridden on the command line.
- Exclude overrides superseded by active legacy managed configuration or feature requirements, while supporting older servers without `configRequirements/read`.
- Pass only the remaining explicit feature overrides to a requested daemon restart.
## Testing
Update compatibility tests for stale daemon overrides, explicit enable/disable requests, managed configuration, and older servers. Extend CLI startup tests to verify the feature overrides persisted after a restart.
GitOrigin-RevId: ac92c9c02cd70fa233203ebef1182fd119325dd1
(cherry picked from commit d3f9b22b74)
This commit is contained in:
@@ -364,6 +364,17 @@ async fn daemon_startup(command: &str) -> Result<()> {
|
||||
ensure!(text.contains("Cannotusethesharedbackgroundserver:Thissessionrequiresapi_key_model_discoverytobedisabled."));
|
||||
} else {
|
||||
ensure!(text.contains("Server:Localbackgroundserver") == restart);
|
||||
if restart {
|
||||
let settings: serde_json::Value = serde_json::from_slice(
|
||||
&fs::read(home.path().join("app-server-daemon/settings.json"))?
|
||||
)?;
|
||||
let flag = if disabling && !persisted {
|
||||
"auth_elicitation"
|
||||
} else {
|
||||
"api_key_model_discovery"
|
||||
};
|
||||
assert_eq!(settings["featureOverrides"], serde_json::json!({(flag): false}));
|
||||
}
|
||||
}
|
||||
} else if command == "start" {
|
||||
ensure!(text.contains("Server:Localbackgroundserver"));
|
||||
|
||||
@@ -31,10 +31,15 @@ pub(super) async fn check(
|
||||
startup: &mut StartupDraft,
|
||||
target: &AppServerTarget,
|
||||
config: &Config,
|
||||
cli_kv_overrides: &[(String, toml::Value)],
|
||||
managed_daemon: bool,
|
||||
) -> io::Result<Option<String>> {
|
||||
let issue = match startup
|
||||
.run_until(daemon_startup::compatibility_warning(target, config))
|
||||
.run_until(daemon_startup::compatibility_warning(
|
||||
target,
|
||||
config,
|
||||
cli_kv_overrides,
|
||||
))
|
||||
.await?
|
||||
{
|
||||
Ok(warning) => return Ok(warning),
|
||||
@@ -104,7 +109,11 @@ pub(super) async fn check(
|
||||
})
|
||||
.await?;
|
||||
startup
|
||||
.run_until(daemon_startup::compatibility_warning(target, config))
|
||||
.run_until(daemon_startup::compatibility_warning(
|
||||
target,
|
||||
config,
|
||||
cli_kv_overrides,
|
||||
))
|
||||
.await?
|
||||
.map_err(io::Error::other)
|
||||
}
|
||||
|
||||
@@ -5,6 +5,10 @@
|
||||
//! Windows sessions use explicit embedded behavior before discovery or startup.
|
||||
|
||||
use super::*;
|
||||
use codex_app_server_client::TypedRequestError;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ConfigRequirementsReadResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
const SERVER_FEATURES: [Feature; 4] = [
|
||||
@@ -155,6 +159,7 @@ pub(super) fn server_features(overrides: &[(String, toml::Value)]) -> BTreeMap<S
|
||||
pub(super) async fn compatibility_warning(
|
||||
target: &AppServerTarget,
|
||||
config: &Config,
|
||||
cli_kv_overrides: &[(String, toml::Value)],
|
||||
) -> Result<Option<String>, CompatibilityError> {
|
||||
let AppServerTarget::LocalDaemon {
|
||||
allow_embedded_fallback,
|
||||
@@ -165,15 +170,61 @@ pub(super) async fn compatibility_warning(
|
||||
};
|
||||
let mut restart_features = None;
|
||||
let check = async {
|
||||
// The feature-list RPC cannot report this process-scoped structured setting.
|
||||
if !config.features.enabled(Feature::CodeModeHost)
|
||||
&& config.code_mode.disable_in_process_fallback
|
||||
{
|
||||
return Err("code-mode host fallback policy requires embedded mode".to_string());
|
||||
}
|
||||
let client = app_server_connection::connect(target)
|
||||
.await
|
||||
.map_err(|_| "could not connect to check daemon feature settings".to_string())?;
|
||||
// File settings and defaults belong to the daemon until it restarts.
|
||||
// Only explicit invocation flags can request a shared-service restart.
|
||||
let mut requested = server_features(cli_kv_overrides);
|
||||
if requested.is_empty() {
|
||||
let _ = client.shutdown().await;
|
||||
return Ok(());
|
||||
}
|
||||
// Both legacy managed config and current requirements override CLI flags.
|
||||
let layers = crate::config_update::read_effective_config_if_supported(
|
||||
client.request_handle(),
|
||||
config.cwd.as_path(),
|
||||
)
|
||||
.await
|
||||
.map_err(|_| "could not read daemon configuration".to_string())?
|
||||
.and_then(|config| config.layers)
|
||||
.unwrap_or_default();
|
||||
for layer in layers.iter().filter(|layer| {
|
||||
layer["disabledReason"].is_null()
|
||||
&& matches!(
|
||||
layer["name"]["type"].as_str(),
|
||||
Some("legacyManagedConfigTomlFromFile" | "legacyManagedConfigTomlFromMdm")
|
||||
)
|
||||
}) {
|
||||
requested.retain(|name, _| layer["config"]["features"].get(name).is_none());
|
||||
}
|
||||
match client
|
||||
.request_handle()
|
||||
.request_typed::<ConfigRequirementsReadResponse>(
|
||||
ClientRequest::ConfigRequirementsRead {
|
||||
request_id: RequestId::String("tui-daemon-requirements".to_string()),
|
||||
params: None,
|
||||
},
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(response) => {
|
||||
if let Some(required) = response.requirements.and_then(|r| r.feature_requirements) {
|
||||
requested.retain(|name, _| !required.contains_key(name));
|
||||
}
|
||||
}
|
||||
Err(TypedRequestError::Server { source, .. })
|
||||
if source.code == -32601
|
||||
|| source.code == -32600
|
||||
&& source.message.contains("configRequirements/read")
|
||||
&& (source.message.contains("unknown variant")
|
||||
|| source.message.contains("unknown method")) => {}
|
||||
Err(_) => return Err("could not check daemon configuration requirements".to_string()),
|
||||
}
|
||||
if requested.is_empty() {
|
||||
let _ = client.shutdown().await;
|
||||
return Ok(());
|
||||
}
|
||||
let (tx, rx) = tokio::sync::oneshot::channel();
|
||||
crate::experimental_features::fetch(
|
||||
client.request_handle(),
|
||||
@@ -184,27 +235,17 @@ pub(super) async fn compatibility_warning(
|
||||
let result = rx.await;
|
||||
let _ = client.shutdown().await;
|
||||
let features = result.map_err(|_| "daemon feature check was interrupted".to_string())??;
|
||||
// A previous client may have launched this daemon with overrides, even if
|
||||
// this client has none. Check effective values, including defaults.
|
||||
for feature in SERVER_FEATURES {
|
||||
let name = feature.key();
|
||||
let enabled = config.features.enabled(feature);
|
||||
for (name, enabled) in &requested {
|
||||
if features
|
||||
.iter()
|
||||
.find(|feature| feature.name == name)
|
||||
.find(|feature| feature.name == *name)
|
||||
.is_some_and(|feature| feature.enabled)
|
||||
!= enabled
|
||||
!= *enabled
|
||||
{
|
||||
restart_features = Some(
|
||||
SERVER_FEATURES
|
||||
.into_iter()
|
||||
.map(|feature| {
|
||||
(feature.key().to_string(), config.features.enabled(feature))
|
||||
})
|
||||
.collect(),
|
||||
);
|
||||
let state = if enabled { "enabled" } else { "disabled" };
|
||||
return Err(format!("This session requires {name} to be {state}"));
|
||||
let state = if *enabled { "enabled" } else { "disabled" };
|
||||
let reason = format!("This session requires {name} to be {state}");
|
||||
restart_features = Some(requested);
|
||||
return Err(reason);
|
||||
}
|
||||
}
|
||||
Ok::<(), String>(())
|
||||
|
||||
@@ -152,6 +152,10 @@ async fn daemon_feature_compatibility_respects_required_and_optional_attachment(
|
||||
"matching",
|
||||
"stale overrides",
|
||||
"conflicting client",
|
||||
"explicit enable",
|
||||
"managed",
|
||||
"legacy managed",
|
||||
"old requirements server",
|
||||
"unsupported RPC",
|
||||
] {
|
||||
let home = TempDir::new()?;
|
||||
@@ -161,19 +165,21 @@ async fn daemon_feature_compatibility_respects_required_and_optional_attachment(
|
||||
.build()
|
||||
.await?;
|
||||
config.features.enable(Feature::ApiKeyModelDiscovery)?;
|
||||
if scenario == "conflicting client" {
|
||||
config.features.disable(Feature::ApiKeyModelDiscovery)?;
|
||||
}
|
||||
let cli_overrides = match scenario {
|
||||
"matching" | "stale overrides" => Vec::new(),
|
||||
_ => vec![(
|
||||
"features.api_key_model_discovery".to_string(),
|
||||
toml::Value::Boolean(scenario == "explicit enable"),
|
||||
)],
|
||||
};
|
||||
let features = [
|
||||
Feature::ApiKeyModelDiscovery,
|
||||
Feature::CodeModeHost,
|
||||
Feature::AuthElicitation,
|
||||
Feature::McpOAuthRefreshCoordination,
|
||||
].map(|feature| {
|
||||
let enabled = if feature == Feature::ApiKeyModelDiscovery && scenario == "stale overrides" {
|
||||
let enabled = if feature == Feature::ApiKeyModelDiscovery && matches!(scenario, "stale overrides" | "explicit enable") {
|
||||
false
|
||||
} else if feature == Feature::ApiKeyModelDiscovery && scenario == "conflicting client" {
|
||||
true
|
||||
} else {
|
||||
config.features.enabled(feature)
|
||||
};
|
||||
@@ -195,16 +201,34 @@ async fn daemon_feature_compatibility_respects_required_and_optional_attachment(
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
let response = if request.method == "initialize" {
|
||||
json!({"id": request.id, "result": {"userAgent": "daemon-test"}})
|
||||
} else {
|
||||
assert_eq!(request.method, "experimentalFeature/list");
|
||||
assert_eq!(request.params.unwrap()["threadId"], json!(null));
|
||||
if scenario == "unsupported RPC" {
|
||||
let response = match request.method.as_str() {
|
||||
"initialize" => {
|
||||
json!({"id": request.id, "result": {"userAgent": "daemon-test"}})
|
||||
}
|
||||
"config/read" => {
|
||||
json!({"id": request.id, "result": {"config": {}, "layers": [{
|
||||
"name": {"type": "legacyManagedConfigTomlFromFile"}, "config":
|
||||
if scenario == "legacy managed" {
|
||||
json!({"features": {"api_key_model_discovery": true}})
|
||||
} else { json!({}) }
|
||||
}]}})
|
||||
}
|
||||
"configRequirements/read" if scenario == "old requirements server" => {
|
||||
json!({"id": request.id, "error": {"code": -32600, "message": "Invalid request: unknown variant `configRequirements/read`"}})
|
||||
}
|
||||
"configRequirements/read" => json!({"id": request.id, "result": {
|
||||
"requirements": if scenario == "managed" {
|
||||
json!({"featureRequirements": {"api_key_model_discovery": true}})
|
||||
} else { json!(null) }
|
||||
}}),
|
||||
"experimentalFeature/list" if scenario == "unsupported RPC" => {
|
||||
json!({"id": request.id, "error": {"code": -32601, "message": "method not found"}})
|
||||
} else {
|
||||
}
|
||||
"experimentalFeature/list" => {
|
||||
assert_eq!(request.params.unwrap()["threadId"], json!(null));
|
||||
json!({"id": request.id, "result": {"data": features, "nextCursor": null}})
|
||||
}
|
||||
_ => panic!("unexpected request: {}", request.method),
|
||||
};
|
||||
socket
|
||||
.send(Message::Text(response.to_string().into()))
|
||||
@@ -212,15 +236,19 @@ async fn daemon_feature_compatibility_respects_required_and_optional_attachment(
|
||||
.unwrap();
|
||||
}
|
||||
});
|
||||
let result = daemon_startup::compatibility_warning(&target, &config).await;
|
||||
let result =
|
||||
daemon_startup::compatibility_warning(&target, &config, &cli_overrides).await;
|
||||
server.await?;
|
||||
if scenario == "matching" {
|
||||
if matches!(
|
||||
scenario,
|
||||
"matching" | "stale overrides" | "managed" | "legacy managed"
|
||||
) {
|
||||
assert_eq!(result?, None);
|
||||
continue;
|
||||
}
|
||||
let reason = match scenario {
|
||||
"stale overrides" => "This session requires api_key_model_discovery to be enabled",
|
||||
"conflicting client" => {
|
||||
"explicit enable" => "This session requires api_key_model_discovery to be enabled",
|
||||
"conflicting client" | "old requirements server" => {
|
||||
"This session requires api_key_model_discovery to be disabled"
|
||||
}
|
||||
"unsupported RPC" => "Experimental feature request failed",
|
||||
@@ -242,7 +270,7 @@ async fn daemon_feature_compatibility_respects_required_and_optional_attachment(
|
||||
daemon_startup::FAILURE_HINT
|
||||
)
|
||||
);
|
||||
if scenario == "stale overrides" {
|
||||
if scenario == "explicit enable" {
|
||||
insta::assert_snapshot!("daemon_feature_mismatch_error", error);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -585,6 +585,7 @@ pub(super) async fn run_main_inner(
|
||||
&mut startup_draft,
|
||||
&app_server_target,
|
||||
&config,
|
||||
&cli_kv_overrides,
|
||||
managed_daemon,
|
||||
)
|
||||
.await?
|
||||
|
||||
@@ -8,27 +8,16 @@ use anyhow::ensure;
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use futures::SinkExt;
|
||||
use futures::StreamExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use tokio::net::UnixListener;
|
||||
use tokio_tungstenite::tungstenite::Message;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> Result<()> {
|
||||
for scenario in ["default", "explicit", "host policy"] {
|
||||
async fn incompatible_daemon_falls_back_for_explicit_features() -> Result<()> {
|
||||
for scenario in ["explicit enable", "explicit disable"] {
|
||||
let cwd = codex_utils_cargo_bin::repo_root()?;
|
||||
let home = tempfile::tempdir_in("/tmp")?;
|
||||
write_test_config(home.path(), &cwd)?;
|
||||
if scenario == "host policy" {
|
||||
let path = home.path().join("config.toml");
|
||||
let contents = std::fs::read_to_string(&path)?;
|
||||
std::fs::write(
|
||||
path,
|
||||
format!(
|
||||
"features.code_mode_host = {{enabled=false, disable_in_process_fallback=true}}\n{contents}"
|
||||
),
|
||||
)?;
|
||||
}
|
||||
let socket = codex_app_server_client::app_server_control_socket_path(home.path())?;
|
||||
std::fs::create_dir_all(socket.parent().unwrap())?;
|
||||
let listener = UnixListener::bind(socket.as_path())?;
|
||||
@@ -43,15 +32,22 @@ async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> R
|
||||
let JSONRPCMessage::Request(request) = serde_json::from_str(&text)? else {
|
||||
continue;
|
||||
};
|
||||
let response = if request.method == "initialize" {
|
||||
json!({"id": request.id, "result": {"userAgent": "daemon-test/0.0.0"}})
|
||||
} else {
|
||||
assert_eq!(request.method, "experimentalFeature/list");
|
||||
json!({"id": request.id, "result": {"data": [{
|
||||
let response = match request.method.as_str() {
|
||||
"initialize" => {
|
||||
json!({"id": request.id, "result": {"userAgent": "daemon-test/0.0.0"}})
|
||||
}
|
||||
"config/read" => {
|
||||
json!({"id": request.id, "result": {"config": {}, "layers": []}})
|
||||
}
|
||||
"configRequirements/read" => {
|
||||
json!({"id": request.id, "result": {"requirements": null}})
|
||||
}
|
||||
"experimentalFeature/list" => json!({"id": request.id, "result": {"data": [{
|
||||
"name": "api_key_model_discovery", "stage": "stable",
|
||||
"displayName": null, "description": null, "announcement": null,
|
||||
"enabled": scenario == "explicit", "defaultEnabled": true,
|
||||
}], "nextCursor": null}})
|
||||
"enabled": scenario == "explicit disable", "defaultEnabled": true,
|
||||
}], "nextCursor": null}}),
|
||||
_ => panic!("unexpected request: {}", request.method),
|
||||
};
|
||||
socket
|
||||
.send(Message::Text(response.to_string().into()))
|
||||
@@ -59,25 +55,24 @@ async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> R
|
||||
}
|
||||
Ok::<_, anyhow::Error>(())
|
||||
});
|
||||
let args = if scenario == "explicit" {
|
||||
let args = if scenario == "explicit disable" {
|
||||
vec!["-c", "features.api_key_model_discovery=false"]
|
||||
} else {
|
||||
vec![]
|
||||
vec!["-c", "features.api_key_model_discovery=true"]
|
||||
};
|
||||
let mut terminal = PtyCodex::start(&cwd, home, &args)?;
|
||||
terminal.wait_for_startup()?;
|
||||
terminal.wait_for_screen("warning")?;
|
||||
terminal.write_input(b"\x14")?;
|
||||
let (snapshot, warning_end) = match scenario {
|
||||
"default" => (
|
||||
"explicit enable" => (
|
||||
"daemon_feature_mismatch",
|
||||
"api_key_model_discovery to be enabled.",
|
||||
),
|
||||
"explicit" => (
|
||||
"explicit disable" => (
|
||||
"daemon_override_mismatch",
|
||||
"api_key_model_discovery to be disabled.",
|
||||
),
|
||||
"host policy" => ("daemon_host_policy_mismatch", "requires embedded mode."),
|
||||
_ => unreachable!(),
|
||||
};
|
||||
terminal.wait_for_screen(warning_end)?;
|
||||
@@ -94,10 +89,6 @@ async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> R
|
||||
terminal.write_input(b"\r")?;
|
||||
terminal.wait_for_screen("Model:")?;
|
||||
ensure!(!terminal.screen_contains("unix://"));
|
||||
if scenario == "host policy" {
|
||||
server.abort();
|
||||
continue;
|
||||
}
|
||||
tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), server).await???;
|
||||
}
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user