From e49739355235bb1ac27df799327227be23e87e94 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 18 Sep 2026 02:54:39 +0000 Subject: [PATCH] Allow worktree sessions to use an existing local daemon (#46498) ## Why `--worktree` and command-line worktree feature overrides previously excluded sessions from using the local daemon, even though worktree allocation is client-owned and thread requests already forward the feature. ## What changed - Allow `--worktree` and boolean `features.worktrees` overrides to remain eligible for daemon connections, while preserving exclusions for other configuration overrides and `--no-daemon`. - Skip daemon auto-start for `--worktree` launches. ## Testing Add daemon eligibility tests for worktree options and overrides. Run the existing worktree startup and fork test scenarios against both embedded and daemon backends, checking ownership before the first turn and confirming daemon connections through `/status`. GitOrigin-RevId: b3f4782e83d8e20974fb4a831442cae013d58e56 --- codex-rs/cli/tests/worktree.rs | 144 ++++++++++++++++++---- codex-rs/tui/src/daemon_startup.rs | 14 ++- codex-rs/tui/src/daemon_startup_tests.rs | 34 ++++- codex-rs/tui/src/startup_orchestration.rs | 1 + 4 files changed, 164 insertions(+), 29 deletions(-) diff --git a/codex-rs/cli/tests/worktree.rs b/codex-rs/cli/tests/worktree.rs index 7e71ee5fc7..401caed1ab 100644 --- a/codex-rs/cli/tests/worktree.rs +++ b/codex-rs/cli/tests/worktree.rs @@ -9,6 +9,7 @@ use std::io::Write as _; use std::path::Path; use std::process::Command; use std::time::Duration; +#[cfg(not(unix))] use tempfile::TempDir; use wiremock::Mock; use wiremock::MockServer; @@ -133,6 +134,23 @@ async fn rejected_start( #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn interactive_worktree_start_and_fork_bind_owner_before_turn() -> anyhow::Result<()> { + worktree_start_and_fork("embedded").await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg_attr( + windows, + ignore = "requires a non-elevated terminal with daemon breakaway support" +)] +async fn daemon_worktree_start_and_fork_bind_owner_before_turn() -> anyhow::Result<()> { + worktree_start_and_fork("daemon").await +} + +async fn worktree_start_and_fork(backend: &str) -> anyhow::Result<()> { + // Leave room for the local control socket on Unix platforms with short sun_path limits. + #[cfg(unix)] + let root = tempfile::Builder::new().tempdir_in("/tmp")?; + #[cfg(not(unix))] let root = TempDir::new()?; let root = codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(root.path())? .canonicalize()? @@ -165,8 +183,9 @@ async fn interactive_worktree_start_and_fork_bind_owner_before_turn() -> anyhow: r#" cli_auth_credentials_store = "file" chatgpt_base_url = "{}/source/backend-api" -features.worktrees = true +analytics.enabled = false check_for_update_on_startup = false +features.daemon_auto_start = true model_provider = "local" model = "test-model" sandbox_mode = "workspace-write" @@ -229,6 +248,38 @@ trust_level = "trusted" ] { env.remove(key); } + // Keep the managed package local; no installer or updater participates in this test. + struct StopDaemon(Command); + impl Drop for StopDaemon { + fn drop(&mut self) { + let _ = self.0.output(); + } + } + let _daemon = if backend == "daemon" { + let bin = home.join("packages/app-server-daemon/current/bin"); + fs::create_dir_all(&bin)?; + let managed = bin.join(if cfg!(windows) { "codex.exe" } else { "codex" }); + fs::hard_link(&program, &managed).or_else(|_| fs::copy(&program, &managed).map(|_| ()))?; + fs::create_dir(home.join("app-server-daemon"))?; + fs::write( + home.join("app-server-daemon/settings.json"), + r#"{"shutdownGraceSeconds":0,"updater":{"autoUpdateEnabled":false}}"#, + )?; + let mut daemon_stop = Command::new(&program); + daemon_stop + .envs(&env) + .args(["app-server", "daemon", "stop"]); + Some(StopDaemon(daemon_stop)) + } else { + None + }; + // Every launch, including rejection cases, exercises the same CLI feature opt-in. + let worktree_args: Vec = vec![ + "--worktree".into(), + "--enable".into(), + "worktrees".into(), + "--no-alt-screen".into(), + ]; let mut owner = None; let mut previous: Vec = Vec::new(); for (fork, explicit_cd, analytics, auth_failure) in [ @@ -307,7 +358,19 @@ trust_level = "trusted" }) .mount(&server) .await; - let mut args = vec!["--worktree".to_owned(), "--no-alt-screen".to_owned()]; + if backend == "daemon" && previous.is_empty() { + // Policy mocks must be ready before the daemon loads its startup configuration. + let started = Command::new(&program) + .envs(&env) + .args(["app-server", "daemon", "start"]) + .output()?; + anyhow::ensure!( + started.status.success(), + "daemon startup failed: {}", + String::from_utf8_lossy(&started.stderr) + ); + } + let mut args = worktree_args.clone(); if fork { args.extend([ "--add-dir".into(), @@ -396,13 +459,12 @@ trust_level = "trusted" assert!(output.contains("The checkout was kept"), "{output}"); continue; } - let renamed = if !fork && matches!(observed, Ok(Ok(_))) { + let renamed = if !fork && let Ok(Ok((_, _, metadata))) = &observed { tokio::time::timeout(Duration::from_secs(/*secs*/ 10), async { session .writer_sender() .send(b"/rename managed-source\r".to_vec()) .await?; - let (_, _, metadata) = observed.as_ref().unwrap().as_ref().unwrap(); let thread_id = codex_protocol::ThreadId::from_string( metadata["ownerThreadId"].as_str().context("owner id")?, )?; @@ -431,6 +493,25 @@ trust_level = "trusted" } else { Ok(()) }; + if backend == "daemon" && !analytics && matches!(observed, Ok(Ok(_))) { + session.writer_sender().send(b"/status\r".to_vec()).await?; + tokio::time::timeout(Duration::from_secs(/*secs*/ 10), async { + while !output.contains("app-server-control.sock") { + let bytes = stdout + .recv() + .await + .context("TUI exited before daemon status")?; + output.push_str(&String::from_utf8_lossy(&bytes)); + let replies = queries.feed(&bytes); + if !replies.is_empty() { + session.writer_sender().send(replies).await?; + } + } + Ok::<_, anyhow::Error>(()) + }) + .await + .with_context(|| format!("daemon status timed out: {output}"))??; + } if analytics { tokio::time::timeout(Duration::from_secs(/*secs*/ 10), metric_rx.recv()) .await? @@ -446,7 +527,7 @@ trust_level = "trusted" ); } if matches!(startup_result, Ok(Ok(()))) && renamed.is_ok() { - if !fork { + if !fork && backend == "embedded" { for (input, expected) in [ ("/daemon\r", "Install latest public stable"), ("\r", "Update and exit"), @@ -500,12 +581,14 @@ trust_level = "trusted" .flat_map(|resource| resource["scopeMetrics"].as_array().into_iter().flatten()) .flat_map(|scope| scope["metrics"].as_array().into_iter().flatten()) .collect::>(); - let update = exported - .iter() - .find(|metric| metric["name"] == "codex.daemon.update") - .context("handoff metric")?; - let update_point = &update["sum"]["dataPoints"][0]; - assert_eq!(update_point["asInt"], 1); + if backend == "embedded" { + let update = exported + .iter() + .find(|metric| metric["name"] == "codex.daemon.update") + .context("handoff metric")?; + let update_point = &update["sum"]["dataPoints"][0]; + assert_eq!(update_point["asInt"], 1); + } let point = exported .iter() .filter(|metric| metric["name"] == "codex.tui.start") @@ -525,20 +608,25 @@ trust_level = "trusted" )) }) .collect::>>()?; - assert_eq!( - tags, - HashMap::from([ - ("app_server_mode", "in_process"), - ("terminal_name", "unknown"), - ("multiplexer", "none"), - ("daemon_selection_reason", "incompatible_option"), - ("daemon_auto_start", "disabled"), - ("auto_update", "enabled"), - ("auto_update_setting", "default"), - ("update_interval_setting", "default"), - ("shutdown_grace_setting", "default"), - ]) - ); + let mut expected_tags = HashMap::from([ + ("app_server_mode", "in_process"), + ("terminal_name", "unknown"), + ("multiplexer", "none"), + ("daemon_selection_reason", "incompatible_option"), + ("daemon_auto_start", "enabled"), + ("auto_update", "enabled"), + ("auto_update_setting", "default"), + ("update_interval_setting", "default"), + ("shutdown_grace_setting", "default"), + ]); + if backend == "daemon" { + expected_tags.extend([ + ("auto_update", "disabled"), + ("auto_update_setting", "configured"), + ("shutdown_grace_setting", "configured"), + ]); + } + assert_eq!(tags, expected_tags); } else { assert!(metrics.is_empty(), "analytics disabled"); } @@ -636,6 +724,8 @@ trust_level = "trusted" &program, &[ "--worktree".into(), + "--enable".into(), + "worktrees".into(), "--no-alt-screen".into(), "fork".into(), next_owner.clone(), @@ -669,6 +759,8 @@ trust_level = "trusted" &program, &[ "--worktree".into(), + "--enable".into(), + "worktrees".into(), "--no-alt-screen".into(), "--cd".into(), source.join(".codex").display().to_string(), @@ -730,6 +822,8 @@ trust_level = "trusted" &program, &[ "--worktree".into(), + "--enable".into(), + "worktrees".into(), "--no-alt-screen".into(), "--cd".into(), source.display().to_string(), diff --git a/codex-rs/tui/src/daemon_startup.rs b/codex-rs/tui/src/daemon_startup.rs index d37bce9407..a7e3bf65e1 100644 --- a/codex-rs/tui/src/daemon_startup.rs +++ b/codex-rs/tui/src/daemon_startup.rs @@ -14,8 +14,6 @@ pub(super) fn exclusion( ) -> Option<&'static str> { if cli.no_daemon { Some("--no-daemon") - } else if cli.shared.worktree { - Some("--worktree") } else if cli.oss { Some("--oss") } else if workload_identity_selected { @@ -42,7 +40,17 @@ pub(super) fn config_exclusion( strict_config: bool, bypass_hook_trust: bool, ) -> Option<&'static str> { - if !cli_kv_overrides.is_empty() { + // Worktree allocation is client-owned, and thread requests already forward this feature. + if !cli_kv_overrides + .iter() + .all(|(key, value)| match key.as_str() { + "features.worktrees" => value.is_bool(), + "features" => value.as_table().is_some_and(|features| { + features.len() == 1 && features.get("worktrees").is_some_and(toml::Value::is_bool) + }), + _ => false, + }) + { Some("command-line configuration overrides (-c, --enable, --disable, or --search)") } else if !loader_overrides_are_default(loader_overrides) { Some("custom configuration loader") diff --git a/codex-rs/tui/src/daemon_startup_tests.rs b/codex-rs/tui/src/daemon_startup_tests.rs index db2eb55a52..000ea8302b 100644 --- a/codex-rs/tui/src/daemon_startup_tests.rs +++ b/codex-rs/tui/src/daemon_startup_tests.rs @@ -5,6 +5,37 @@ use crate::legacy_core::config::ConfigBuilder; use pretty_assertions::assert_eq; use tempfile::TempDir; +#[test] +fn worktree_feature_override_allows_daemon_without_allowing_other_features() { + for (raw, eligible) in [ + ("features.worktrees=true", true), + ("features.worktrees=false", true), + ("features={worktrees=true}", true), + ("features={worktrees=true,shell_tool=false}", false), + ("features.shell_tool=false", false), + ("features.worktrees.enabled=true", false), + ("features={}", false), + ("model='test'", false), + ] { + let overrides = codex_utils_cli::CliConfigOverrides { + raw_overrides: vec![raw.to_string()], + } + .parse_overrides() + .unwrap(); + assert_eq!( + daemon_startup::config_exclusion( + &overrides, + &LoaderOverrides::default(), + /*strict_config*/ false, + /*bypass_hook_trust*/ false, + ) + .is_none(), + eligible, + "{raw}" + ); + } +} + #[test] fn daemon_launch_telemetry_records_once_on_connection_or_early_return() { for connected in [false, true] { @@ -122,7 +153,8 @@ fn daemon_eligibility_preserves_launch_options_and_explains_exclusions() { use clap::Parser; for (args, expected) in [ ("--no-daemon", Some("--no-daemon")), - ("--worktree", Some("--worktree")), + ("--worktree", None), + ("--worktree --no-daemon", Some("--no-daemon")), ("--oss", Some("--oss")), ("--profile test", Some("--profile")), ("--strict-config", Some("--strict-config")), diff --git a/codex-rs/tui/src/startup_orchestration.rs b/codex-rs/tui/src/startup_orchestration.rs index ccefde1548..eb1ded23c4 100644 --- a/codex-rs/tui/src/startup_orchestration.rs +++ b/codex-rs/tui/src/startup_orchestration.rs @@ -400,6 +400,7 @@ pub(super) async fn run_main_inner( )) .await?; let auto_start_daemon = config.features.enabled(Feature::DaemonAutoStart) + && !cli.shared.worktree && !cli.agents_overview && !cli.no_daemon && !app_server_target.uses_remote_workspace();