diff --git a/codex-rs/.config/nextest.toml b/codex-rs/.config/nextest.toml index 8a095942dd..6d87360563 100644 --- a/codex-rs/.config/nextest.toml +++ b/codex-rs/.config/nextest.toml @@ -5,6 +5,11 @@ slow-timeout = { period = "30s", terminate-after = 2 } retries = 1 +[[profile.default.overrides]] +# These cases cold-start a daemon before confirming a second launch with new settings. +filter = 'package(codex-cli) & binary(daemon_startup) & test(requires_confirmation_to_disable_)' +slow-timeout = { period = "1m", terminate-after = 2 } + [[profile.default.overrides]] # This case validates and copies both the initial and replacement debug package. filter = 'package(codex-cli) & binary(app_server_daemon) & test(=packaged_daemon_start_and_explicit_replacement)' diff --git a/codex-rs/app-server-daemon/src/launch.rs b/codex-rs/app-server-daemon/src/launch.rs index 2a06017280..04d58401a6 100644 --- a/codex-rs/app-server-daemon/src/launch.rs +++ b/codex-rs/app-server-daemon/src/launch.rs @@ -1,13 +1,15 @@ -//! Invocation feature overrides apply only when starting a missing daemon. -//! The lifecycle lock protects their persistence against concurrent starts and updates. +//! Invocation overrides apply on fresh starts or explicitly confirmed managed restarts. +//! The lifecycle lock serializes persistence against other clients and updates. use crate::Daemon; use crate::LifecycleOutput; use crate::ensure_supported_platform; use anyhow::Result; +use anyhow::ensure; use std::collections::BTreeMap; -/// Start a missing daemon with these features, or leave a running daemon unchanged. +/// Start a missing daemon with these features, preserving other saved overrides, +/// or leave a running daemon unchanged. /// Callers must check the running server's configuration before using it. pub async fn start_with_features(features: &BTreeMap) -> Result { ensure_supported_platform()?; @@ -16,5 +18,44 @@ pub async fn start_with_features(features: &BTreeMap) -> Result
  • ) -> Result { + ensure_supported_platform()?; + #[cfg(windows)] + crate::backend::windows::ensure_not_elevated()?; + let daemon = Daemon::from_environment()?; + let _operation_lock = daemon.acquire_operation_lock().await?; + let selected = daemon.current_installation()?; + Box::pin(selected.restart_with_features_locked(features)).await +} + +impl Daemon { + pub(super) async fn restart(&self) -> Result { + self.restart_with_settings(self.load_settings().await?) + .await + } + + pub(super) async fn restart_with_features_locked( + &self, + features: &BTreeMap, + ) -> Result { + let mut settings = self.load_settings().await?; + ensure!( + self.running_backend_instance(&settings).await?.is_some(), + "no running managed daemon; rerun the command to check the current server" + ); + let previous = settings.feature_overrides.clone(); + settings.feature_overrides.extend(features.clone()); + if settings.feature_overrides == previous { + return self.start(&previous).await; + } + self.restart_with_settings(settings).await + } } diff --git a/codex-rs/app-server-daemon/src/lib.rs b/codex-rs/app-server-daemon/src/lib.rs index fbeff0988e..755a0ebd82 100644 --- a/codex-rs/app-server-daemon/src/lib.rs +++ b/codex-rs/app-server-daemon/src/lib.rs @@ -6,6 +6,7 @@ use backend::windows::try_lock_file; mod client; mod install_lock; mod launch; +pub use launch::restart_with_features; pub use launch::start_with_features; mod managed_install; mod prepare_install; @@ -437,8 +438,7 @@ impl Daemon { .await) } - async fn restart(&self) -> Result { - let settings = self.load_settings().await?; + async fn restart_with_settings(&self, settings: DaemonSettings) -> Result { if client::probe(&self.socket_path).await.is_ok() && self.running_backend(&settings).await?.is_none() { @@ -465,6 +465,11 @@ impl Daemon { .await?; } + // Persist changed launch settings only after the old process has stopped. + // A failed or interrupted drain must not make an unapplied change look current. + if self.load_settings().await? != settings { + settings.save(&self.settings_file).await?; + } let pid = managed.start_managed_backend(&settings).await?; let info = self.wait_until_ready().await?; if let Err(err) = managed.ensure_managed_updater(&settings).await { diff --git a/codex-rs/app-server-daemon/src/update_loop_tests.rs b/codex-rs/app-server-daemon/src/update_loop_tests.rs index 198cbcea4b..1602c080a4 100644 --- a/codex-rs/app-server-daemon/src/update_loop_tests.rs +++ b/codex-rs/app-server-daemon/src/update_loop_tests.rs @@ -617,6 +617,74 @@ async fn daemon_start_and_restart_preserve_launch_features() { } } +#[cfg(unix)] +#[tokio::test] +async fn confirmed_feature_restart_preserves_ownership_and_skips_matching_settings() { + use crate::LifecycleStatus; + use std::collections::BTreeMap; + + for managed in [true, false] { + let home = TempDir::new().unwrap(); + let (daemon, _) = manual_update_daemon(&home); + std::fs::write(&daemon.settings_file, + r#"{"featureOverrides":{"auth_elicitation":true,"api_key_model_discovery":true},"updater":{"autoUpdateEnabled":false},"shutdownGraceSeconds":0}"# + ).unwrap(); + let original = daemon.load_settings().await.unwrap(); + if managed { + daemon.start_managed_backend(&original).await.unwrap(); + } + let server = test_control_server(&daemon, home.path()).await; + let _lock = daemon.acquire_operation_lock().await.unwrap(); + let requested = BTreeMap::from([ + ("api_key_model_discovery".to_string(), false), + ("mcp_oauth_refresh_coordination".to_string(), true), + ]); + if managed { + // Hide the selection without removing the script the spawned shell still needs. + let selected_package = daemon.managed_codex_bin.parent().unwrap(); + let saved_package = selected_package.with_extension("saved"); + std::fs::rename(selected_package, &saved_package).unwrap(); + let error = daemon + .restart_with_features_locked(&requested) + .await + .unwrap_err(); + std::fs::rename(saved_package, selected_package).unwrap(); + assert!( + error.to_string().contains("daemon executable not found"), + "{error:#}" + ); + assert_eq!(daemon.load_settings().await.unwrap(), original); + } + let result = daemon.restart_with_features_locked(&requested).await; + if managed { + assert_eq!(result.unwrap().status, LifecycleStatus::Restarted); + let pid = std::fs::read(&daemon.pid_file).unwrap(); + let mut expected = original; + expected.feature_overrides.extend(requested.clone()); + assert_eq!(daemon.load_settings().await.unwrap(), expected); + assert_eq!( + daemon + .restart_with_features_locked(&requested) + .await + .unwrap() + .status, + LifecycleStatus::AlreadyRunning + ); + assert_eq!(std::fs::read(&daemon.pid_file).unwrap(), pid); + daemon.stop().await.unwrap(); + } else { + assert!( + result + .unwrap_err() + .to_string() + .contains("no running managed daemon") + ); + assert_eq!(daemon.load_settings().await.unwrap(), original); + } + server.abort(); + } +} + #[cfg(unix)] #[tokio::test] async fn manual_update_restarts_local_daemon_with_automatic_updates_disabled() { diff --git a/codex-rs/cli/tests/daemon_startup.rs b/codex-rs/cli/tests/daemon_startup.rs index 6a235149f0..e00fe1d584 100644 --- a/codex-rs/cli/tests/daemon_startup.rs +++ b/codex-rs/cli/tests/daemon_startup.rs @@ -14,6 +14,36 @@ async fn auto_daemon_start_attaches_to_shared_server() -> Result<()> { daemon_startup("start").await } +#[tokio::test] +#[cfg(unix)] +async fn incompatible_daemon_can_cancel() -> Result<()> { + daemon_startup("mismatch-cancel").await +} + +#[tokio::test] +#[cfg(unix)] +async fn incompatible_daemon_can_run_embedded() -> Result<()> { + daemon_startup("mismatch-embedded").await +} + +#[tokio::test] +#[cfg(unix)] +async fn incompatible_daemon_can_restart_with_confirmed_settings() -> Result<()> { + daemon_startup("mismatch-restart").await +} + +#[tokio::test] +#[cfg(unix)] +async fn fresh_daemon_requires_confirmation_to_disable_shared_features() -> Result<()> { + daemon_startup("mismatch-disable").await +} + +#[tokio::test] +#[cfg(unix)] +async fn stopped_daemon_requires_confirmation_to_disable_persisted_features() -> Result<()> { + daemon_startup("mismatch-disable-persisted").await +} + #[tokio::test] async fn daemon_exclusion_survives_resume_picker() -> Result<()> { daemon_startup("resume").await @@ -55,6 +85,10 @@ async fn daemon_startup(command: &str) -> Result<()> { serde_json::to_string(&workspace_path)?, ), )?; + let mismatch = command.starts_with("mismatch-"); + let persisted = command == "mismatch-disable-persisted"; + let disabling = matches!(command, "mismatch-disable" | "mismatch-disable-persisted"); + let restart = command == "mismatch-restart" || disabling; let bedrock_onboarding = matches!(command, "bedrock" | "bedrock-running"); if !bedrock_onboarding { fs::write( @@ -83,7 +117,7 @@ async fn daemon_startup(command: &str) -> Result<()> { env.insert("TERM".into(), "xterm-256color".into()); let mut args = vec!["--no-alt-screen".to_string()]; let mut steps: VecDeque<(&str, &[u8])> = VecDeque::new(); - if matches!(command, "start" | "bedrock-running") { + if matches!(command, "start" | "bedrock-running") || mismatch { // A selected package with a stopped daemon avoids installing a release. let managed = home .path() @@ -93,12 +127,16 @@ async fn daemon_startup(command: &str) -> Result<()> { fs::create_dir(home.path().join("app-server-daemon"))?; fs::write( home.path().join("app-server-daemon/settings.json"), - r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false}}"#, + if persisted { + r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false},"featureOverrides":{"api_key_model_discovery":true}}"# + } else { + r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false}}"# + }, )?; } let pid_file = home.path().join("app-server-daemon/daemon.pid"); let result = async { - let existing_daemon = if command == "bedrock-running" { + let mut existing_daemon = if command == "bedrock-running" || (mismatch && !disabling) { let started = Command::new(&codex) .env_clear() .envs(&env) @@ -118,6 +156,27 @@ async fn daemon_startup(command: &str) -> Result<()> { // The draft header is visible before the session's command composer is ready. steps.push_back(("GPT-5.6-Terra", b"/status\r")); "Server:Localbackgroundserver" + } else if mismatch { + args.extend(if persisted { + ["--disable".into(), "api_key_model_discovery".into()] + } else if disabling { + ["--disable".into(), "auth_elicitation".into()] + } else { + ["--enable".into(), "api_key_model_discovery".into()] + }); + let input: &[u8] = match command { + "mismatch-cancel" => b"\x03", + "mismatch-embedded" => b"1", + "mismatch-restart" | "mismatch-disable" | "mismatch-disable-persisted" => b"2\r", + _ => unreachable!(), + }; + steps.push_back(("Backgroundserverhasincompatiblefeaturesettings", input)); + if command == "mismatch-cancel" { + "--no-daemon" + } else { + steps.push_back(("GPT-5.6-Terra", b"/status\r")); + if restart { "Server:Localbackgroundserver" } else { "Model:" } + } } else if bedrock_onboarding { "UseAmazonBedrock" } else { @@ -139,6 +198,7 @@ async fn daemon_startup(command: &str) -> Result<()> { &[], ) .await?; + let exit = spawned.exit_rx; let session = spawned.session; let writer = session.writer_sender(); let mut stdout = spawned.stdout_rx; @@ -153,7 +213,9 @@ async fn daemon_startup(command: &str) -> Result<()> { ("\x1b]10;?", b"\x1b]10;rgb:ffff/ffff/ffff\x1b\\"), ("\x1b]11;?", b"\x1b]11;rgb:0000/0000/0000\x1b\\"), ]; - let result = tokio::time::timeout(Duration::from_secs(/*secs*/ 45), async { + // A fresh disable request includes both cold daemon startup and its confirmed restart. + let timeout = Duration::from_secs(if disabling { 90 } else { 45 }); + let result = tokio::time::timeout(timeout, async { while let Some(bytes) = stdout.recv().await { output.push_str(&String::from_utf8_lossy(&bytes)); screen.process(&bytes); @@ -172,11 +234,29 @@ async fn daemon_startup(command: &str) -> Result<()> { if let Some((ready, input)) = steps.front() && text.contains(ready) { + if disabling && *ready == "Backgroundserverhasincompatiblefeaturesettings" { + existing_daemon = Some(fs::read(&pid_file)?); + let settings: serde_json::Value = serde_json::from_slice(&fs::read(home.path().join("app-server-daemon/settings.json"))?)?; + if persisted { + ensure!(settings["featureOverrides"]["api_key_model_discovery"] == true); + } else { + ensure!(settings["featureOverrides"]["auth_elicitation"].is_null()); + } + } writer.send(input.to_vec()).await?; steps.pop_front(); output.clear(); } else if steps.is_empty() && text.contains(expected) { - if command == "start" { + if mismatch { + let previous_pid = existing_daemon.as_ref().context("missing original daemon PID")?; + ensure!((fs::read(&pid_file)? != *previous_pid) == restart); + if command == "mismatch-cancel" { + ensure!(text.contains("Cannotusethesharedbackgroundserver:Thissessionrequiresapi_key_model_discoverytobeenabled.")); + } else { + ensure!(text.contains("Server:Localbackgroundserver") == restart); + } + } else if command == "start" { + ensure!(text.contains("Server:Localbackgroundserver")); ensure!(home.path().join("app-server-daemon/daemon.pid").exists()); } else if let Some(existing_daemon) = &existing_daemon { ensure!(fs::read(&pid_file)? == *existing_daemon); @@ -192,9 +272,13 @@ async fn daemon_startup(command: &str) -> Result<()> { ) }) .await; + if command == "mismatch-cancel" && matches!(result, Ok(Ok(()))) { + let status = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), exit).await??; + ensure!(status != 0, "incompatible daemon launch must fail"); + } Ok::<_, anyhow::Error>(( session, - result.with_context(|| format!("{command} timed out: {}", screen.screen().contents())), + result.with_context(|| format!("{command} timed out waiting for {}: {}", steps.front().map_or(expected, |(ready, _)| *ready), screen.screen().contents())), )) } .await; diff --git a/codex-rs/tui/src/daemon_recovery.rs b/codex-rs/tui/src/daemon_recovery.rs new file mode 100644 index 0000000000..0a9e8faf09 --- /dev/null +++ b/codex-rs/tui/src/daemon_recovery.rs @@ -0,0 +1,181 @@ +//! Explicit recovery from required-daemon incompatibility. Restart is managed-only, +//! requires a fresh confirmation, and is followed by one compatibility check, never a loop. + +use crate::AppServerTarget; +use crate::app_event::AppEvent; +use crate::app_event_sender::AppEventSender; +use crate::bottom_pane::BottomPaneView; +use crate::bottom_pane::ListSelectionView; +use crate::bottom_pane::SelectionItem; +use crate::bottom_pane::SelectionViewParams; +use crate::daemon_startup; +use crate::daemon_startup::CompatibilityError; +use crate::keymap::RuntimeKeymap; +use crate::legacy_core::config::Config; +use crate::render::renderable::ColumnRenderable; +use crate::render::renderable::Renderable; +use crate::startup_draft::StartupDraft; +use crossterm::event::KeyCode; +use crossterm::event::KeyEventKind; +use crossterm::event::KeyModifiers; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::widgets::Paragraph; +use ratatui::widgets::Wrap; +use std::io; +use std::io::IsTerminal; +use tokio::sync::mpsc::unbounded_channel; +use tokio_stream::StreamExt; + +pub(super) async fn check( + startup: &mut StartupDraft, + target: &AppServerTarget, + config: &Config, + managed_daemon: bool, +) -> io::Result> { + let issue = match startup + .run_until(daemon_startup::compatibility_warning(target, config)) + .await? + { + Ok(warning) => return Ok(warning), + Err(issue) => issue, + }; + if !(io::stdin().is_terminal() && io::stdout().is_terminal()) { + return Err(io::Error::other(issue)); + } + startup.flush_pending_events().await?; + let keymap = RuntimeKeymap::from_config(&config.tui_keymap).map_err(io::Error::other)?; + let mut view = recovery_view(&issue, managed_daemon, &keymap); + let mut chord_matcher = crate::keymap::KeyChordMatcher::default(); + let tui = startup.tui_mut(); + tui.discard_pending_input_before_interactive_screen()?; + let selection = { + let events = tui.event_stream(); + tokio::pin!(events); + loop { + let height = view.desired_height(tui.terminal.size()?.width); + tui.draw(height, |frame| { + view.render(frame.area(), frame.buffer_mut()) + })?; + let Some(event) = events.next().await else { + break None; + }; + tui.screen_size_for_event(&event)?; + if let crate::tui::TuiEvent::Key(key) = event + && matches!(key.kind, KeyEventKind::Press | KeyEventKind::Repeat) + { + if key.modifiers.contains(KeyModifiers::CONTROL) + && matches!(key.code, KeyCode::Char('c' | 'd')) + { + break None; + } + let key = match chord_matcher.advance( + key, + &keymap.chords, + crate::keymap::KeymapContextSet::new(crate::keymap::KeymapContext::List), + ) { + crate::keymap::KeyChordMatch::PassThrough => key, + crate::keymap::KeyChordMatch::Completed(key) => key, + crate::keymap::KeyChordMatch::Pending(_) + | crate::keymap::KeyChordMatch::Cancelled + | crate::keymap::KeyChordMatch::Ignored => continue, + }; + view.handle_key_event(key); + } + if view.is_complete() { + break view.take_last_selected_index(); + } + } + }; + tui.terminal.clear()?; + match (selection, issue.restart_features.as_ref()) { + (Some(0), _) => Ok(Some(format!( + "Running without the shared background server: {}.", + issue.reason + ))), + (Some(1), Some(features)) if managed_daemon => { + tui.with_restored(|| async { + crossterm::terminal::disable_raw_mode()?; + codex_app_server_daemon::restart_with_features(features) + .await + .map_err(|err| { + io::Error::other(format!("{err:#}\n{}", daemon_startup::FAILURE_HINT)) + }) + }) + .await?; + startup + .run_until(daemon_startup::compatibility_warning(target, config)) + .await? + .map_err(io::Error::other) + } + (Some(_) | None, _) => Err(io::Error::other(issue)), + } +} + +fn recovery_view( + issue: &CompatibilityError, + managed_daemon: bool, + keymap: &RuntimeKeymap, +) -> ListSelectionView { + let mut header = ColumnRenderable::new(); + header.push(Line::from( + if issue.restart_features.is_some() { + "Background server has incompatible feature settings" + } else { + "Cannot use the background server" + } + .bold(), + )); + header.push(Paragraph::new(issue.reason.clone()).wrap(Wrap { trim: false })); + if let Some(features) = &issue.restart_features { + header.push(Line::from( + "Restart will use these shared feature settings:", + )); + for (name, enabled) in features { + header.push(Line::from(format!(" {name} = {enabled}").dim())); + } + header.push(Paragraph::new( + "These settings persist and can disable functionality for other clients. Restart may interrupt active or queued work." + ).wrap(Wrap { trim: false })); + } + let (tx, _rx) = unbounded_channel::(); + ListSelectionView::new( + SelectionViewParams { + header: Box::new(header), + initial_selected_idx: Some(2), + items: vec![ + SelectionItem { + name: "Run without daemon this time".to_string(), + dismiss_on_select: true, + ..Default::default() + }, + SelectionItem { + name: "Restart with these settings".to_string(), + dismiss_on_select: true, + require_explicit_confirmation: true, + is_disabled: !managed_daemon || issue.restart_features.is_none(), + disabled_reason: if !managed_daemon { + Some("This server is not managed by Codex.".to_string()) + } else if issue.restart_features.is_none() { + Some("Restart cannot resolve this compatibility check.".to_string()) + } else { + None + }, + ..Default::default() + }, + SelectionItem { + name: "Cancel".to_string(), + dismiss_on_select: true, + ..Default::default() + }, + ], + ..SelectionViewParams::picker() + }, + AppEventSender::new(tx), + keymap.list.clone(), + ) +} + +#[cfg(test)] +#[path = "daemon_recovery_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/daemon_recovery_tests.rs b/codex-rs/tui/src/daemon_recovery_tests.rs new file mode 100644 index 0000000000..5c283e4112 --- /dev/null +++ b/codex-rs/tui/src/daemon_recovery_tests.rs @@ -0,0 +1,67 @@ +use super::*; +use crossterm::event::KeyCode; +use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; +use pretty_assertions::assert_eq; +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use std::collections::BTreeMap; + +#[test] +fn daemon_recovery_requires_explicit_restart_and_defaults_to_cancel() { + let issue = CompatibilityError { + reason: "This session requires api_key_model_discovery to be enabled".to_string(), + restart_features: Some(BTreeMap::from([ + ("api_key_model_discovery".to_string(), true), + ("mcp_oauth_refresh_coordination".to_string(), false), + ])), + }; + let non_restartable = CompatibilityError { + reason: "code-mode host fallback policy requires embedded mode".to_string(), + restart_features: None, + }; + for (issue, managed, snapshot) in [ + (&issue, true, "daemon_recovery_menu"), + (&issue, false, "unmanaged_daemon_recovery_menu"), + ( + &non_restartable, + true, + "non_restartable_daemon_recovery_menu", + ), + ] { + let mut view = recovery_view(issue, managed, &RuntimeKeymap::defaults()); + let area = Rect::new( + /*x*/ 0, + /*y*/ 0, + /*width*/ 90, + view.desired_height(/*width*/ 90), + ); + let mut buffer = Buffer::empty(area); + view.render(area, &mut buffer); + let text = buffer + .content + .chunks(90) + .map(|row| { + row.iter() + .map(ratatui::buffer::Cell::symbol) + .collect::() + .trim_end() + .to_string() + }) + .collect::>() + .join("\n"); + insta::assert_snapshot!(snapshot, text); + view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert_eq!(view.take_last_selected_index(), Some(2)); + + let mut view = recovery_view(issue, managed, &RuntimeKeymap::defaults()); + view.handle_key_event(KeyEvent::new(KeyCode::Char('2'), KeyModifiers::NONE)); + if managed && issue.restart_features.is_some() { + assert!(!view.is_complete()); + view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + assert_eq!(view.take_last_selected_index(), Some(1)); + } else { + assert_eq!(view.take_last_selected_index(), Some(2)); + } + } +} diff --git a/codex-rs/tui/src/daemon_startup.rs b/codex-rs/tui/src/daemon_startup.rs index 0e76635c3b..9ce454fc9e 100644 --- a/codex-rs/tui/src/daemon_startup.rs +++ b/codex-rs/tui/src/daemon_startup.rs @@ -1,6 +1,6 @@ //! Local daemon launch policy. Explicit embedded launches never discover or start a daemon; -//! incompatible feature settings use embedded mode. Compatible automatic launches -//! require a successful shared-server connection. +//! optional attachment may fall back to embedded mode, while automatic launches +//! require a compatible shared server and a successful connection. use super::*; use std::collections::BTreeMap; @@ -14,6 +14,13 @@ const SERVER_FEATURES: [Feature; 4] = [ pub(super) const FAILURE_HINT: &str = "To work without the background server, rerun the same command with --no-daemon (including resume or fork and its arguments)."; +#[derive(Debug, thiserror::Error)] +#[error("Cannot use the shared background server: {reason}.\n{FAILURE_HINT}")] +pub(super) struct CompatibilityError { + pub reason: String, + pub restart_features: Option>, +} + pub(super) fn exclusion( cli: &Cli, cli_kv_overrides: &[(String, toml::Value)], @@ -54,7 +61,10 @@ pub(super) fn config_exclusion( .all(|(key, value)| match key.as_str() { "suppress_unstable_features_warning" | "tui.fullscreen_transcript" => value.is_bool(), "tui" => value.as_table().is_some_and(|tui| { - tui.len() == 1 && tui.get("fullscreen_transcript").is_some_and(toml::Value::is_bool) + tui.len() == 1 + && tui + .get("fullscreen_transcript") + .is_some_and(toml::Value::is_bool) }), "features" => value.as_table().is_some_and(|features| { !features.is_empty() @@ -64,9 +74,6 @@ pub(super) fn config_exclusion( }), _ => key.strip_prefix("features.").is_some_and(allowed_feature) && value.is_bool(), }) - // Older clients cannot check compatibility before attaching. Do not disable - // shared services they may rely on through a new daemon's CLI overrides. - || server_features(cli_kv_overrides).values().any(|enabled| !enabled) { Some("command-line configuration overrides (-c, --enable, --disable, or --search)") } else if !loader_overrides_are_default(loader_overrides) { @@ -111,17 +118,22 @@ pub(super) fn server_features(overrides: &[(String, toml::Value)]) -> BTreeMap Option { - if !matches!(target, AppServerTarget::LocalDaemon { .. }) { - return None; - } - // 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 Some("Running without the shared background server: code-mode host fallback policy requires embedded mode.".to_string()); - } +) -> Result, CompatibilityError> { + let AppServerTarget::LocalDaemon { + allow_embedded_fallback, + .. + } = target + else { + return Ok(None); + }; + 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())?; @@ -146,13 +158,29 @@ pub(super) async fn compatibility_warning( .is_some_and(|feature| feature.enabled) != enabled { - return Err(format!("daemon does not report features.{name}={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}")); } } Ok::<(), String>(()) } .await; - check - .err() - .map(|reason| format!("Running without the shared background server: {reason}.")) + match check { + Ok(()) => Ok(None), + Err(reason) if *allow_embedded_fallback => Ok(Some(format!( + "Running without the shared background server: {reason}." + ))), + Err(reason) => Err(CompatibilityError { + reason, + restart_features, + }), + } } diff --git a/codex-rs/tui/src/daemon_startup_tests.rs b/codex-rs/tui/src/daemon_startup_tests.rs index 15c62b7d08..201ec0fde0 100644 --- a/codex-rs/tui/src/daemon_startup_tests.rs +++ b/codex-rs/tui/src/daemon_startup_tests.rs @@ -17,11 +17,11 @@ fn audited_overrides_allow_daemon_without_allowing_arbitrary_config() { ("features={worktrees=true}", true), ( "features={worktrees=true,api_key_model_discovery=false}", - false, + true, ), - ("features.auth_elicitation=false", false), - ("features.code_mode_host=false", false), - ("features.mcp_oauth_refresh_coordination=false", false), + ("features.auth_elicitation=false", true), + ("features.code_mode_host=false", true), + ("features.mcp_oauth_refresh_coordination=false", true), ("suppress_unstable_features_warning=true", true), ("suppress_unstable_features_warning='true'", false), ("features={worktrees=true,shell_tool=false}", false), @@ -138,6 +138,119 @@ fn daemon_launch_telemetry_records_once_on_connection_or_early_return() { } } +#[tokio::test] +async fn daemon_feature_compatibility_respects_required_and_optional_attachment() +-> color_eyre::Result<()> { + use codex_app_server_protocol::JSONRPCMessage; + use futures::SinkExt; + use futures::StreamExt; + use serde_json::json; + use tokio_tungstenite::tungstenite::Message; + + for allow_embedded_fallback in [false, true] { + for scenario in [ + "matching", + "stale overrides", + "conflicting client", + "unsupported RPC", + ] { + let home = TempDir::new()?; + let mut config = ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .build() + .await?; + config.features.enable(Feature::ApiKeyModelDiscovery)?; + if scenario == "conflicting client" { + config.features.disable(Feature::ApiKeyModelDiscovery)?; + } + let features = [ + Feature::ApiKeyModelDiscovery, + Feature::CodeModeHost, + Feature::AuthElicitation, + Feature::McpOAuthRefreshCoordination, + ].map(|feature| { + let enabled = if feature == Feature::ApiKeyModelDiscovery && scenario == "stale overrides" { + false + } else if feature == Feature::ApiKeyModelDiscovery && scenario == "conflicting client" { + true + } else { + config.features.enabled(feature) + }; + json!({"name": feature.key(), "stage": "beta", "enabled": enabled, "defaultEnabled": false}) + }); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let target = AppServerTarget::LocalDaemon { + endpoint: RemoteAppServerEndpoint::WebSocket { + websocket_url: format!("ws://{}", listener.local_addr()?), + auth_token: None, + }, + allow_embedded_fallback, + }; + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await.unwrap(); + let mut socket = tokio_tungstenite::accept_async(stream).await.unwrap(); + while let Some(Ok(Message::Text(text))) = socket.next().await { + let JSONRPCMessage::Request(request) = serde_json::from_str(&text).unwrap() + 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" { + json!({"id": request.id, "error": {"code": -32601, "message": "method not found"}}) + } else { + json!({"id": request.id, "result": {"data": features, "nextCursor": null}}) + } + }; + socket + .send(Message::Text(response.to_string().into())) + .await + .unwrap(); + } + }); + let result = daemon_startup::compatibility_warning(&target, &config).await; + server.await?; + if scenario == "matching" { + assert_eq!(result?, None); + continue; + } + let reason = match scenario { + "stale overrides" => "This session requires api_key_model_discovery to be enabled", + "conflicting client" => { + "This session requires api_key_model_discovery to be disabled" + } + "unsupported RPC" => "Experimental feature request failed", + _ => unreachable!(), + }; + if allow_embedded_fallback { + assert_eq!( + result?, + Some(format!( + "Running without the shared background server: {reason}." + )) + ); + } else { + let error = result.unwrap_err().to_string(); + assert_eq!( + error, + format!( + "Cannot use the shared background server: {reason}.\n{}", + daemon_startup::FAILURE_HINT + ) + ); + if scenario == "stale overrides" { + insta::assert_snapshot!("daemon_feature_mismatch_error", error); + } + } + } + } + Ok(()) +} + #[cfg(windows)] #[tokio::test] async fn daemon_connection_rejects_unprotected_socket_before_handshake() -> color_eyre::Result<()> { diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 83f5c1a43f..0aed8985ae 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -2246,6 +2246,7 @@ fn should_show_bedrock_setup_wizard( .is_login_method_allowed(ForcedLoginMethod::Api) } +mod daemon_recovery; mod daemon_startup; mod daemon_telemetry; diff --git a/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__daemon_recovery_menu.snap b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__daemon_recovery_menu.snap new file mode 100644 index 0000000000..37ea3281af --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__daemon_recovery_menu.snap @@ -0,0 +1,17 @@ +--- +source: tui/src/daemon_recovery_tests.rs +expression: text +--- + + Background server has incompatible feature settings + This session requires api_key_model_discovery to be enabled + Restart will use these shared feature settings: + api_key_model_discovery = true + mcp_oauth_refresh_coordination = false + These settings persist and can disable functionality for other clients. Restart may + interrupt active or queued work. + + + 1. Run without daemon this time + 2. Restart with these settings +› 3. Cancel diff --git a/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__non_restartable_daemon_recovery_menu.snap b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__non_restartable_daemon_recovery_menu.snap new file mode 100644 index 0000000000..fae81afd18 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__non_restartable_daemon_recovery_menu.snap @@ -0,0 +1,13 @@ +--- +source: tui/src/daemon_recovery_tests.rs +expression: text +--- + + Cannot use the background server + code-mode host fallback policy requires embedded mode + + + 1. Run without daemon this time + Restart with these settings (disabled) Restart cannot resolve this compatibility + check. +› 2. Cancel diff --git a/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__unmanaged_daemon_recovery_menu.snap b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__unmanaged_daemon_recovery_menu.snap new file mode 100644 index 0000000000..0f2d96a004 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__daemon_recovery__tests__unmanaged_daemon_recovery_menu.snap @@ -0,0 +1,17 @@ +--- +source: tui/src/daemon_recovery_tests.rs +expression: text +--- + + Background server has incompatible feature settings + This session requires api_key_model_discovery to be enabled + Restart will use these shared feature settings: + api_key_model_discovery = true + mcp_oauth_refresh_coordination = false + These settings persist and can disable functionality for other clients. Restart may + interrupt active or queued work. + + + 1. Run without daemon this time + Restart with these settings (disabled) This server is not managed by Codex. +› 2. Cancel diff --git a/codex-rs/tui/src/snapshots/codex_tui__daemon_startup_tests__daemon_feature_mismatch_error.snap b/codex-rs/tui/src/snapshots/codex_tui__daemon_startup_tests__daemon_feature_mismatch_error.snap new file mode 100644 index 0000000000..e53dce73b8 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__daemon_startup_tests__daemon_feature_mismatch_error.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/daemon_startup_tests.rs +expression: error +--- +Cannot use the shared background server: This session requires api_key_model_discovery to be enabled. +To work without the background server, rerun the same command with --no-daemon (including resume or fork and its arguments). diff --git a/codex-rs/tui/src/startup_orchestration.rs b/codex-rs/tui/src/startup_orchestration.rs index d25281e88e..633be7780e 100644 --- a/codex-rs/tui/src/startup_orchestration.rs +++ b/codex-rs/tui/src/startup_orchestration.rs @@ -491,7 +491,10 @@ pub(super) async fn run_main_inner( daemon_exclusion = Some("Bedrock sign-in"); app_server_target = AppServerTarget::Embedded; } - let daemon_features = daemon_startup::server_features(&cli_kv_overrides); + let mut daemon_features = daemon_startup::server_features(&cli_kv_overrides); + // Disabling shared services requires confirmation, even on a fresh auto-start. + daemon_features.retain(|_, enabled| *enabled); + let mut managed_daemon = false; if auto_start_daemon && daemon_exclusion.is_none() { startup_draft.flush_pending_events().await?; let output = startup_draft @@ -506,6 +509,7 @@ pub(super) async fn run_main_inner( }) }) .await?; + managed_daemon = output.backend.is_some(); app_server_target = AppServerTarget::LocalDaemon { endpoint: RemoteAppServerEndpoint::UnixSocket { socket_path: AbsolutePathBuf::from_absolute_path_checked(output.socket_path)?, @@ -517,12 +521,13 @@ pub(super) async fn run_main_inner( let compatibility_warning = if cli.agents_overview { None } else { - startup_draft - .run_until(daemon_startup::compatibility_warning( - &app_server_target, - &config, - )) - .await? + daemon_recovery::check( + &mut startup_draft, + &app_server_target, + &config, + managed_daemon, + ) + .await? }; if compatibility_warning.is_some() { app_server_target = AppServerTarget::Embedded; diff --git a/codex-rs/tui/tests/suite/daemon_compatibility.rs b/codex-rs/tui/tests/suite/daemon_compatibility.rs index a7a87a63e4..5e6ea64585 100644 --- a/codex-rs/tui/tests/suite/daemon_compatibility.rs +++ b/codex-rs/tui/tests/suite/daemon_compatibility.rs @@ -71,11 +71,11 @@ async fn incompatible_daemon_falls_back_for_default_and_explicit_features() -> R let (snapshot, warning_end) = match scenario { "default" => ( "daemon_feature_mismatch", - "features.api_key_model_discovery=false.", + "api_key_model_discovery to be disabled.", ), "explicit" => ( "daemon_override_mismatch", - "features.api_key_model_discovery=true.", + "api_key_model_discovery to be enabled.", ), "host policy" => ("daemon_host_policy_mismatch", "requires embedded mode."), _ => unreachable!(), diff --git a/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_feature_mismatch.snap b/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_feature_mismatch.snap index 1e53f4552a..60251d073c 100644 --- a/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_feature_mismatch.snap +++ b/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_feature_mismatch.snap @@ -2,4 +2,4 @@ source: tui/tests/suite/daemon_compatibility.rs expression: warning --- -⚠ Running without the shared background server: daemon does not report features.api_key_model_discovery=false. +⚠ Running without the shared background server: This session requires api_key_model_discovery to be disabled. diff --git a/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_override_mismatch.snap b/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_override_mismatch.snap index d809ddedd4..f4f7d9ab20 100644 --- a/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_override_mismatch.snap +++ b/codex-rs/tui/tests/suite/snapshots/all__suite__daemon_compatibility__daemon_override_mismatch.snap @@ -2,4 +2,4 @@ source: tui/tests/suite/daemon_compatibility.rs expression: warning --- -⚠ Running without the shared background server: daemon does not report features.api_key_model_discovery=true. +⚠ Running without the shared background server: This session requires api_key_model_discovery to be enabled.