From 0b2e7b5eb1cfa74e5807a84b291e6c900eeb197d Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Thu, 4 Jun 2026 16:52:10 -0700 Subject: [PATCH 01/10] Improve Windows sandbox setup refresh diagnostics (#26471) ## Why Users have been seeing opaque Windows sandbox setup refresh failures such as `windows sandbox: spawn setup refresh`, including reports in #24391 and #21208. The setup refresh path already runs the Windows sandbox setup helper, but it was not using the same structured `setup_error.json` reporting path that elevated setup uses. As a result, when the helper exited non-zero, Codex only surfaced a generic refresh status instead of the helper's `SetupFailure` code and message. ## What changed - Clear stale `setup_error.json` before non-elevated setup refresh launches the helper. - When the refresh helper exits non-zero, read the helper-written report through the existing `report_helper_failure` path. - Keep a parent-side launch diagnostic for cases where the helper never starts, including the helper path, cwd, sandbox log path, and spawn error. - Clear the setup error report after a successful refresh. - Add regression coverage for report consumption and stale-report avoidance. ## Verification - `cargo test -p codex-windows-sandbox setup::tests::` --- codex-rs/windows-sandbox-rs/src/setup.rs | 110 +++++++++++++++--- .../windows-sandbox-rs/src/setup_error.rs | 2 +- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index c1881b55c8..428b5ff6a1 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -15,6 +15,7 @@ use crate::allow::compute_allow_paths_for_permissions; use crate::helper_materialization::bundled_executable_path_for_exe; use crate::helper_materialization::helper_bin_dir; use crate::identity::sandbox_setup_is_complete; +use crate::logging::current_log_file_path; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::path_normalization::canonicalize_path; @@ -25,7 +26,6 @@ use crate::setup_error::clear_setup_error_report; use crate::setup_error::failure; use crate::setup_error::read_setup_error_report; use crate::ssh_config_dependencies::ssh_config_dependency_paths; -use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use base64::Engine; @@ -210,6 +210,18 @@ fn run_setup_refresh_inner( let json = serde_json::to_vec(&payload)?; let b64 = BASE64_STANDARD.encode(json); let exe = find_setup_exe(); + let sbx_dir = sandbox_dir(request.codex_home); + let log_path = current_log_file_path(&sbx_dir); + let cleared_report = match clear_setup_error_report(request.codex_home) { + Ok(()) => true, + Err(err) => { + log_note( + &format!("setup refresh: failed to clear setup_error.json before launch: {err}"), + Some(&sbx_dir), + ); + false + } + }; // Refresh should never request elevation; ensure verb isn't set and we don't trigger UAC. let mut cmd = Command::new(&exe); cmd.arg(&b64).stdout(Stdio::null()).stderr(Stdio::null()); @@ -221,24 +233,34 @@ fn run_setup_refresh_inner( cwd.display(), b64.len() ), - Some(&sandbox_dir(request.codex_home)), + Some(&sbx_dir), ); - let status = cmd - .status() - .map_err(|e| { - log_note( - &format!("setup refresh: failed to spawn {}: {e}", exe.display()), - Some(&sandbox_dir(request.codex_home)), - ); - e - }) - .context("spawn setup refresh")?; + let status = cmd.status().map_err(|err| { + let message = format!( + "setup refresh failed to launch helper: helper={}, cwd={}, log={}, error={err}", + exe.display(), + cwd.display(), + log_path.display() + ); + log_note(&format!("setup refresh: {message}"), Some(&sbx_dir)); + failure(SetupErrorCode::OrchestratorHelperLaunchFailed, message) + })?; if !status.success() { log_note( &format!("setup refresh: exited with status {status:?}"), - Some(&sandbox_dir(request.codex_home)), + Some(&sbx_dir), + ); + return Err(report_helper_failure( + request.codex_home, + cleared_report, + status.code(), + )); + } + if let Err(err) = clear_setup_error_report(request.codex_home) { + log_note( + &format!("setup refresh: failed to clear setup_error.json after success: {err}"), + Some(&sbx_dir), ); - return Err(anyhow!("setup refresh failed with status {status}")); } Ok(()) } @@ -1087,7 +1109,9 @@ mod tests { use crate::helper_materialization::helper_bin_dir; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; use crate::setup_error::SetupErrorCode; + use crate::setup_error::SetupErrorReport; use crate::setup_error::extract_failure; + use crate::setup_error::write_setup_error_report; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -1146,6 +1170,64 @@ mod tests { ) } + #[test] + fn report_helper_failure_uses_setup_error_report_when_clear_succeeded() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + write_setup_error_report( + codex_home.as_path(), + &SetupErrorReport { + code: super::SetupErrorCode::HelperFirewallPolicyAccessFailed, + message: "firewall policy unavailable".to_string(), + }, + ) + .expect("write setup error report"); + + let err = super::report_helper_failure( + codex_home.as_path(), + /*cleared_report*/ true, + /*exit_code*/ Some(1), + ); + + let failure = extract_failure(&err).expect("structured setup failure"); + assert_eq!( + &super::SetupFailure::new( + super::SetupErrorCode::HelperFirewallPolicyAccessFailed, + "firewall policy unavailable", + ), + failure + ); + } + + #[test] + fn report_helper_failure_ignores_setup_error_report_when_clear_failed() { + let tmp = TempDir::new().expect("tempdir"); + let codex_home = tmp.path().join("codex-home"); + write_setup_error_report( + codex_home.as_path(), + &SetupErrorReport { + code: super::SetupErrorCode::HelperFirewallPolicyAccessFailed, + message: "stale report".to_string(), + }, + ) + .expect("write setup error report"); + + let err = super::report_helper_failure( + codex_home.as_path(), + /*cleared_report*/ false, + /*exit_code*/ Some(1), + ); + + let failure = extract_failure(&err).expect("structured setup failure"); + assert_eq!( + &super::SetupFailure::new( + super::SetupErrorCode::OrchestratorHelperExitNonzero, + "setup helper exited with status Some(1)", + ), + failure + ); + } + #[test] fn setup_refresh_skips_profiles_without_managed_filesystem_permissions() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/setup_error.rs b/codex-rs/windows-sandbox-rs/src/setup_error.rs index 0f759ef879..d9104602bc 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_error.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_error.rs @@ -117,7 +117,7 @@ pub struct SetupErrorReport { pub message: String, } -#[derive(Debug)] +#[derive(Debug, PartialEq, Eq)] pub struct SetupFailure { pub code: SetupErrorCode, pub message: String, From 72d0bfb6ba4b59ad78fbe012052a06a2c0874fc3 Mon Sep 17 00:00:00 2001 From: beggers-openai Date: Thu, 4 Jun 2026 17:47:58 -0700 Subject: [PATCH 02/10] Pull plugin service less frequently (#26431) # Summary Reduce download traffic to `github.com/openai/plugins` while continuing to check for updates on every Codex startup. # Root cause The startup sync replaced the local repository with a fresh shallow clone whenever the remote revision changed. At Codex's global scale, repeatedly downloading the repository created excessive GitHub traffic. # Changes - Run `git ls-remote` on each startup to read the remote HEAD SHA. - Skip all repository downloads when the local and remote SHAs match. - Update existing checkouts with an exact-SHA shallow `git fetch`, followed by reset and clean. - Bootstrap new installations with `git init` plus the same shallow fetch, rather than cloning. - Keep the existing file lock so concurrent Codex processes serialize updates and do not duplicate fetches. - Preserve the existing GitHub HTTP and export archive fallback behavior. # Impact Each startup makes one lightweight remote HEAD check. Repository objects are downloaded only when the revision changes, and existing Git objects are reused during updates. # Validation - `just test -p codex-core-plugins startup_sync` (15 tests passed) - `just test -p codex-core-plugins` (201 tests passed) - `just clippy -p codex-core-plugins` (passes with one pre-existing `large_enum_variant` warning) - Production app-server smoke test against GitHub: - Fresh home: `ls-remote`, `git init`, one exact-SHA shallow fetch - Unchanged restart: `ls-remote` and local `rev-parse` only; no fetch or clone - Bench smoke passed --- codex-rs/core-plugins/src/startup_sync.rs | 141 ++++++- .../core-plugins/src/startup_sync_tests.rs | 349 +++++++++++++++--- 2 files changed, 418 insertions(+), 72 deletions(-) diff --git a/codex-rs/core-plugins/src/startup_sync.rs b/codex-rs/core-plugins/src/startup_sync.rs index f03aff93f6..c5965f2212 100644 --- a/codex-rs/core-plugins/src/startup_sync.rs +++ b/codex-rs/core-plugins/src/startup_sync.rs @@ -1,3 +1,4 @@ +use std::fs::File; use std::path::Path; use std::path::PathBuf; use std::process::Command; @@ -22,8 +23,11 @@ const CURATED_PLUGINS_BACKUP_ARCHIVE_API_URL: &str = "https://chatgpt.com/backend-api/plugins/export/curated"; const OPENAI_PLUGINS_OWNER: &str = "openai"; const OPENAI_PLUGINS_REPO: &str = "plugins"; +const OPENAI_PLUGINS_GIT_URL: &str = "https://github.com/openai/plugins.git"; +const CURATED_PLUGINS_FETCH_REF: &str = "refs/codex/curated-sync"; const CURATED_PLUGINS_RELATIVE_DIR: &str = ".tmp/plugins"; const CURATED_PLUGINS_SHA_FILE: &str = ".tmp/plugins.sha"; +const CURATED_PLUGINS_SYNC_LOCK_FILE: &str = ".tmp/plugins.sync.lock"; const CURATED_PLUGINS_BACKUP_ARCHIVE_FALLBACK_VERSION: &str = "export-backup"; const CURATED_PLUGINS_GIT_TIMEOUT: Duration = Duration::from_secs(30); const CURATED_PLUGINS_HTTP_TIMEOUT: Duration = Duration::from_secs(30); @@ -78,6 +82,8 @@ fn sync_openai_plugins_repo_with_transport_overrides( api_base_url: &str, backup_archive_api_url: &str, ) -> Result { + let _file_guard = lock_curated_plugins_startup_sync(codex_home)?; + match sync_openai_plugins_repo_via_git(codex_home, git_binary) { Ok(remote_sha) => { emit_curated_plugins_startup_sync_metric("git", "success"); @@ -135,6 +141,22 @@ fn sync_openai_plugins_repo_with_transport_overrides( } } +fn lock_curated_plugins_startup_sync(codex_home: &Path) -> Result { + let lock_path = codex_home.join(CURATED_PLUGINS_SYNC_LOCK_FILE); + std::fs::create_dir_all(codex_home.join(".tmp")) + .map_err(|err| format!("failed to create curated plugins sync directory: {err}"))?; + let lock_file = File::options() + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + .map_err(|err| format!("failed to open curated plugins sync lock: {err}"))?; + lock_file + .lock() + .map_err(|err| format!("failed to lock curated plugins sync: {err}"))?; + Ok(lock_file) +} + fn sync_openai_plugins_repo_via_git(codex_home: &Path, git_binary: &str) -> Result { let repo_path = curated_plugins_repo_path(codex_home); let sha_path = codex_home.join(CURATED_PLUGINS_SHA_FILE); @@ -146,23 +168,30 @@ fn sync_openai_plugins_repo_via_git(codex_home: &Path, git_binary: &str) -> Resu } let staged_repo_dir = prepare_curated_repo_parent_and_temp_dir(&repo_path)?; - let clone_output = run_git_command_with_timeout( - Command::new(git_binary) - .env("GIT_OPTIONAL_LOCKS", "0") - .arg("clone") - .arg("--depth") - .arg("1") - .arg("https://github.com/openai/plugins.git") - .arg(staged_repo_dir.path()), - "git clone curated plugins repo", - CURATED_PLUGINS_GIT_TIMEOUT, + run_git_in_repo( + staged_repo_dir.path(), + git_binary, + &["init"], + "git init curated plugins repo", )?; - ensure_git_success(&clone_output, "git clone curated plugins repo")?; - let cloned_sha = git_head_sha(staged_repo_dir.path(), git_binary)?; - if cloned_sha != remote_sha { + if repo_path.join(".git").is_dir() { + fetch_curated_plugins_commit(&repo_path, &remote_sha, git_binary)?; + fetch_curated_plugins_commit_from_source( + staged_repo_dir.path(), + &repo_path, + CURATED_PLUGINS_FETCH_REF, + git_binary, + )?; + } else { + fetch_curated_plugins_commit(staged_repo_dir.path(), &remote_sha, git_binary)?; + } + + reset_curated_plugins_checkout(staged_repo_dir.path(), git_binary)?; + let fetched_sha = git_head_sha(staged_repo_dir.path(), git_binary)?; + if fetched_sha != remote_sha { return Err(format!( - "curated plugins clone HEAD mismatch: expected {remote_sha}, got {cloned_sha}" + "curated plugins fetch HEAD mismatch: expected {remote_sha}, got {fetched_sha}" )); } @@ -172,6 +201,90 @@ fn sync_openai_plugins_repo_via_git(codex_home: &Path, git_binary: &str) -> Resu Ok(remote_sha) } +fn fetch_curated_plugins_commit( + repo_path: &Path, + remote_sha: &str, + git_binary: &str, +) -> Result<(), String> { + fetch_curated_plugins_commit_from( + repo_path, + OPENAI_PLUGINS_GIT_URL.as_ref(), + remote_sha, + git_binary, + "git fetch curated plugins repo", + ) +} + +fn fetch_curated_plugins_commit_from_source( + repo_path: &Path, + source_repo_path: &Path, + remote_sha: &str, + git_binary: &str, +) -> Result<(), String> { + fetch_curated_plugins_commit_from( + repo_path, + source_repo_path, + remote_sha, + git_binary, + "git copy fetched curated plugins commit", + ) +} + +fn fetch_curated_plugins_commit_from( + repo_path: &Path, + source: &Path, + source_revision: &str, + git_binary: &str, + context: &str, +) -> Result<(), String> { + let fetch_refspec = format!("+{source_revision}:{CURATED_PLUGINS_FETCH_REF}"); + let output = run_git_command_with_timeout( + Command::new(git_binary) + .env("GIT_OPTIONAL_LOCKS", "0") + .arg("-C") + .arg(repo_path) + .args(["fetch", "--depth", "1", "--no-tags"]) + .arg(source) + .arg(fetch_refspec), + context, + CURATED_PLUGINS_GIT_TIMEOUT, + )?; + ensure_git_success(&output, context) +} + +fn reset_curated_plugins_checkout(repo_path: &Path, git_binary: &str) -> Result<(), String> { + run_git_in_repo( + repo_path, + git_binary, + &["reset", "--hard", CURATED_PLUGINS_FETCH_REF], + "git reset curated plugins repo", + )?; + run_git_in_repo( + repo_path, + git_binary, + &["clean", "-fdx"], + "git clean curated plugins repo", + ) +} + +fn run_git_in_repo( + repo_path: &Path, + git_binary: &str, + args: &[&str], + context: &str, +) -> Result<(), String> { + let output = run_git_command_with_timeout( + Command::new(git_binary) + .env("GIT_OPTIONAL_LOCKS", "0") + .arg("-C") + .arg(repo_path) + .args(args), + context, + CURATED_PLUGINS_GIT_TIMEOUT, + )?; + ensure_git_success(&output, context) +} + fn sync_openai_plugins_repo_via_http( codex_home: &Path, api_base_url: &str, diff --git a/codex-rs/core-plugins/src/startup_sync_tests.rs b/codex-rs/core-plugins/src/startup_sync_tests.rs index a9388e3fce..f73e4ad9d1 100644 --- a/codex-rs/core-plugins/src/startup_sync_tests.rs +++ b/codex-rs/core-plugins/src/startup_sync_tests.rs @@ -3,6 +3,8 @@ use pretty_assertions::assert_eq; use std::io::Write; use std::path::Path; use std::path::PathBuf; +#[cfg(unix)] +use std::sync::Barrier; use tempfile::tempdir; use wiremock::Mock; use wiremock::MockServer; @@ -95,6 +97,22 @@ fn write_executable_script(path: &Path, contents: &str) { } } +#[cfg(unix)] +fn run_git(repo: &Path, args: &[&str]) -> std::process::Output { + let output = Command::new("git") + .arg("-C") + .arg(repo) + .args(args) + .output() + .expect("run git"); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + output +} + async fn mount_github_repo_and_ref(server: &MockServer, sha: &str) { Mock::given(method("GET")) .and(path("/repos/openai/plugins")) @@ -253,30 +271,42 @@ fn remove_stale_curated_repo_temp_dirs_removes_only_matching_directories() { #[cfg(unix)] #[test] -fn sync_openai_plugins_repo_prefers_git_when_available() { +fn concurrent_syncs_serialize_fetches_without_skipping_remote_checks() { let tmp = tempdir().expect("tempdir"); let bin_dir = tempfile::Builder::new() .prefix("fake-git-") .tempdir() .expect("tempdir"); let git_path = bin_dir.path().join("git"); + let invocation_log = bin_dir.path().join("invocations.log"); let sha = "0123456789abcdef0123456789abcdef01234567"; write_executable_script( &git_path, &format!( r#"#!/bin/sh +printf '%s\n' "$*" >> '{}' if [ "$1" = "ls-remote" ]; then + sleep 1 printf '%s\tHEAD\n' "{sha}" exit 0 fi -if [ "$1" = "clone" ]; then - dest="$5" - mkdir -p "$dest/.git" "$dest/.agents/plugins" "$dest/plugins/gmail/.codex-plugin" - cat > "$dest/.agents/plugins/marketplace.json" <<'EOF' +if [ "$1" = "-C" ] && [ "$3" = "init" ]; then + mkdir -p "$2/.git" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "fetch" ]; then + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "reset" ]; then + mkdir -p "$2/.agents/plugins" "$2/plugins/gmail/.codex-plugin" + cat > "$2/.agents/plugins/marketplace.json" <<'EOF' {{"name":"openai-curated","plugins":[{{"name":"gmail","source":{{"source":"local","path":"./plugins/gmail"}}}}]}} EOF - printf '%s\n' '{{"name":"gmail"}}' > "$dest/plugins/gmail/.codex-plugin/plugin.json" + printf '%s\n' '{{"name":"gmail"}}' > "$2/plugins/gmail/.codex-plugin/plugin.json" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "clean" ]; then exit 0 fi if [ "$1" = "-C" ] && [ "$3" = "rev-parse" ] && [ "$4" = "HEAD" ]; then @@ -285,23 +315,55 @@ if [ "$1" = "-C" ] && [ "$3" = "rev-parse" ] && [ "$4" = "HEAD" ]; then fi echo "unexpected git invocation: $@" >&2 exit 1 -"# +"#, + invocation_log.display() ), ); - let synced_sha = sync_openai_plugins_repo_with_transport_overrides( - tmp.path(), - git_path.to_str().expect("utf8 path"), - "http://127.0.0.1:9", - "http://127.0.0.1:9/backend-api/plugins/export/curated", - ) - .expect("git sync should succeed"); + let barrier = Barrier::new(2); + let results = std::thread::scope(|scope| { + let run_sync = || { + barrier.wait(); + sync_openai_plugins_repo_with_transport_overrides( + tmp.path(), + git_path.to_str().expect("utf8 path"), + "http://127.0.0.1:9", + "http://127.0.0.1:9/backend-api/plugins/export/curated", + ) + }; + let first = scope.spawn(run_sync); + let second = scope.spawn(run_sync); + [ + first.join().expect("first sync thread"), + second.join().expect("second sync thread"), + ] + }); - assert_eq!(synced_sha, sha); + assert_eq!(results, [Ok(sha.to_string()), Ok(sha.to_string())]); let repo_path = curated_plugins_repo_path(tmp.path()); assert!(repo_path.join(".git").is_dir()); assert_curated_gmail_repo(&repo_path); assert_eq!(read_curated_plugins_sha(tmp.path()).as_deref(), Some(sha)); + let invocations = std::fs::read_to_string(invocation_log).expect("read invocation log"); + assert_eq!( + invocations + .lines() + .filter(|invocation| invocation.starts_with("ls-remote ")) + .count(), + 2 + ); + assert_eq!( + invocations + .lines() + .filter(|invocation| invocation.contains(" fetch --depth 1 --no-tags ")) + .count(), + 1 + ); + assert!( + !invocations + .lines() + .any(|invocation| invocation.split_whitespace().any(|arg| arg == "clone")) + ); } #[cfg(unix)] @@ -328,36 +390,20 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() { ) .expect("write plugin manifest"); - let init_status = Command::new("git") - .arg("-C") - .arg(&work_repo) - .arg("init") - .status() - .expect("run git init"); - assert!(init_status.success()); - - let add_status = Command::new("git") - .arg("-C") - .arg(&work_repo) - .arg("add") - .arg(".") - .status() - .expect("run git add"); - assert!(add_status.success()); - - let commit_status = Command::new("git") - .arg("-C") - .arg(&work_repo) - .arg("-c") - .arg("user.name=Codex Test") - .arg("-c") - .arg("user.email=codex@example.com") - .arg("commit") - .arg("-m") - .arg("init") - .status() - .expect("run git commit"); - assert!(commit_status.success()); + run_git(&work_repo, &["init"]); + run_git(&work_repo, &["add", "."]); + run_git( + &work_repo, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "commit", + "-m", + "init", + ], + ); std::fs::create_dir_all(remote_repo.parent().expect("remote parent")) .expect("create remote parent"); @@ -370,14 +416,7 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() { .expect("run git clone --bare"); assert!(clone_status.success()); - let sha_output = Command::new("git") - .arg("-C") - .arg(&work_repo) - .arg("rev-parse") - .arg("HEAD") - .output() - .expect("run git rev-parse"); - assert!(sha_output.status.success()); + let sha_output = run_git(&work_repo, &["rev-parse", "HEAD"]); let sha = String::from_utf8_lossy(&sha_output.stdout) .trim() .to_string(); @@ -397,10 +436,12 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() { .tempdir() .expect("tempdir"); let git_wrapper = bin_dir.path().join("git"); + let invocation_log = bin_dir.path().join("invocations.log"); write_executable_script( &git_wrapper, &format!( - "#!/bin/sh\nGIT_CONFIG_GLOBAL='{}' exec git \"$@\"\n", + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nGIT_CONFIG_GLOBAL='{}' exec git \"$@\"\n", + invocation_log.display(), git_config_path.display() ), ); @@ -416,6 +457,125 @@ fn sync_openai_plugins_repo_via_git_succeeds_with_local_rewritten_remote() { Some(sha.as_str()) ); assert!(!has_plugins_clone_dirs(tmp.path())); + + let first_sync_invocation_count = std::fs::read_to_string(&invocation_log) + .expect("read first sync invocations") + .lines() + .count(); + let first_sync_invocations = + std::fs::read_to_string(&invocation_log).expect("read first sync invocations"); + assert!( + first_sync_invocations + .lines() + .any(|invocation| invocation.contains(" fetch --depth 1 --no-tags ")) + ); + assert!( + !first_sync_invocations + .lines() + .any(|invocation| invocation.split_whitespace().any(|arg| arg == "clone")) + ); + write_openai_curated_marketplace(&work_repo, &["gmail", "linear"]); + run_git(&work_repo, &["add", "."]); + run_git( + &work_repo, + &[ + "-c", + "user.name=Codex Test", + "-c", + "user.email=codex@example.com", + "commit", + "-m", + "update", + ], + ); + let branch_output = run_git(&work_repo, &["symbolic-ref", "--short", "HEAD"]); + let branch = String::from_utf8_lossy(&branch_output.stdout) + .trim() + .to_string(); + let remote_repo = remote_repo.to_str().expect("utf8 remote repo"); + let push_ref = format!("HEAD:refs/heads/{branch}"); + run_git(&work_repo, &["push", remote_repo, &push_ref]); + let updated_sha_output = run_git(&work_repo, &["rev-parse", "HEAD"]); + let updated_sha = String::from_utf8_lossy(&updated_sha_output.stdout) + .trim() + .to_string(); + + let synced_sha = + sync_openai_plugins_repo_via_git(tmp.path(), git_wrapper.to_str().expect("utf8 path")) + .expect("incremental git sync should succeed"); + + assert_eq!(synced_sha, updated_sha); + assert!( + curated_plugins_repo_path(tmp.path()) + .join("plugins/linear/.codex-plugin/plugin.json") + .is_file() + ); + assert_eq!( + read_curated_plugins_sha(tmp.path()).as_deref(), + Some(updated_sha.as_str()) + ); + assert!( + !curated_plugins_repo_path(tmp.path()) + .join(".git/objects/info/alternates") + .exists() + ); + let invocation_log_contents = + std::fs::read_to_string(&invocation_log).expect("read sync invocations"); + let incremental_sync_invocations = invocation_log_contents + .lines() + .skip(first_sync_invocation_count) + .collect::>(); + let curated_repo_path = curated_plugins_repo_path(tmp.path()); + assert!(incremental_sync_invocations.iter().any(|invocation| { + invocation.starts_with(&format!("-C {} fetch ", curated_repo_path.display())) + && invocation.contains(" https://github.com/openai/plugins.git ") + && invocation.contains(updated_sha.as_str()) + && invocation.ends_with(CURATED_PLUGINS_FETCH_REF) + })); + assert!(incremental_sync_invocations.iter().any(|invocation| { + invocation.contains(" fetch --depth 1 --no-tags ") + && invocation.contains(&format!(" {} ", curated_repo_path.display())) + && invocation.ends_with(&format!( + "{CURATED_PLUGINS_FETCH_REF}:{CURATED_PLUGINS_FETCH_REF}" + )) + })); + assert!( + incremental_sync_invocations + .iter() + .any(|invocation| invocation.ends_with(" init")) + ); + assert!( + !incremental_sync_invocations + .iter() + .any(|invocation| invocation.split_whitespace().any(|arg| arg == "clone")) + ); + assert!(!incremental_sync_invocations.iter().any(|invocation| { + invocation.starts_with(&format!("-C {} reset ", curated_repo_path.display())) + || invocation.starts_with(&format!("-C {} clean ", curated_repo_path.display())) + })); + assert!(!has_plugins_clone_dirs(tmp.path())); + + let unchanged_sync_invocation_count = invocation_log_contents.lines().count(); + let synced_sha = + sync_openai_plugins_repo_via_git(tmp.path(), git_wrapper.to_str().expect("utf8 path")) + .expect("unchanged git sync should succeed"); + + assert_eq!(synced_sha, updated_sha); + let invocation_log = std::fs::read_to_string(&invocation_log).expect("read sync invocations"); + let unchanged_sync_invocations = invocation_log + .lines() + .skip(unchanged_sync_invocation_count) + .collect::>(); + assert!( + unchanged_sync_invocations + .iter() + .any(|invocation| invocation.starts_with("ls-remote ")) + ); + assert!( + !unchanged_sync_invocations + .iter() + .any(|invocation| invocation.contains(" fetch ")) + ); } #[tokio::test] @@ -482,7 +642,7 @@ exit 1 #[cfg(unix)] #[test] -fn sync_openai_plugins_repo_via_git_cleans_up_staged_dir_on_clone_failure() { +fn sync_openai_plugins_repo_via_git_cleans_up_staged_dir_on_fetch_failure() { let tmp = tempdir().expect("tempdir"); let bin_dir = tempfile::Builder::new() .prefix("fake-git-partial-fail-") @@ -499,9 +659,11 @@ if [ "$1" = "ls-remote" ]; then printf '%s\tHEAD\n' "{sha}" exit 0 fi -if [ "$1" = "clone" ]; then - dest="$5" - mkdir -p "$dest/.git" +if [ "$1" = "-C" ] && [ "$3" = "init" ]; then + mkdir -p "$2/.git" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "fetch" ]; then echo "fatal: early EOF" >&2 exit 128 fi @@ -518,6 +680,77 @@ exit 1 assert!(!has_plugins_clone_dirs(tmp.path())); } +#[cfg(unix)] +#[test] +fn sync_openai_plugins_repo_via_git_preserves_existing_snapshot_on_validation_failure() { + let tmp = tempdir().expect("tempdir"); + let repo_path = curated_plugins_repo_path(tmp.path()); + write_openai_curated_marketplace(&repo_path, &["gmail"]); + std::fs::create_dir_all(repo_path.join(".git")).expect("create git dir"); + write_curated_plugin_sha(tmp.path()); + + let bin_dir = tempfile::Builder::new() + .prefix("fake-git-invalid-update-") + .tempdir() + .expect("tempdir"); + let git_path = bin_dir.path().join("git"); + let remote_sha = "fedcba9876543210fedcba9876543210fedcba98"; + + write_executable_script( + &git_path, + &format!( + r#"#!/bin/sh +if [ "$1" = "ls-remote" ]; then + printf '%s\tHEAD\n' "{remote_sha}" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$2" = "{}" ] && [ "$3" = "rev-parse" ]; then + printf '%s\n' "{TEST_CURATED_PLUGIN_SHA}" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$2" = "{}" ] && [ "$3" = "fetch" ]; then + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "init" ]; then + mkdir -p "$2/.git" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "fetch" ]; then + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "reset" ]; then + mkdir -p "$2/plugins/linear/.codex-plugin" + printf '%s\n' '{{"name":"linear"}}' > "$2/plugins/linear/.codex-plugin/plugin.json" + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "clean" ]; then + exit 0 +fi +if [ "$1" = "-C" ] && [ "$3" = "rev-parse" ]; then + printf '%s\n' "{remote_sha}" + exit 0 +fi +echo "unexpected git invocation: $@" >&2 +exit 1 +"#, + repo_path.display(), + repo_path.display(), + ), + ); + + let err = sync_openai_plugins_repo_via_git(tmp.path(), git_path.to_str().expect("utf8 path")) + .expect_err("invalid staged checkout should fail"); + + assert!(err.contains("curated plugins archive missing marketplace manifest")); + assert_curated_gmail_repo(&repo_path); + assert!(!repo_path.join("plugins/linear").exists()); + assert_eq!( + read_curated_plugins_sha(tmp.path()).as_deref(), + Some(TEST_CURATED_PLUGIN_SHA) + ); + assert!(!has_plugins_clone_dirs(tmp.path())); +} + #[tokio::test] async fn sync_openai_plugins_repo_via_http_cleans_up_staged_dir_on_extract_failure() { let tmp = tempdir().expect("tempdir"); From 4be1a168fc7ed3313ddc62031da036f0c50209b7 Mon Sep 17 00:00:00 2001 From: Channing Conger Date: Thu, 4 Jun 2026 17:51:13 -0700 Subject: [PATCH 03/10] ci: test windows cross build (#25000) We cross build when using bazel for windows. This causes a couple hiccups in that v8 does a mksnapshot step that is expecting to snapshot on the host arch which wasn't matching when we were doing the crossbuild. This was causing segfault failiures when starting up codemode from a cross built artifact. This changes things such that we cross build the library and then run and link a snapshot on the host machine/arch which is windows. This gives us a functional snapshot and library that can start code-mode on windows. This fixes the build and then fixes two test regressions we had. --- .bazelrc | 14 ++++--- .github/workflows/bazel.yml | 24 +++++------ codex-rs/core/tests/suite/code_mode.rs | 16 +++++-- patches/v8_bazel_rules.patch | 58 +++++++++++++++++++++++++- 4 files changed, 90 insertions(+), 22 deletions(-) diff --git a/.bazelrc b/.bazelrc index e39a3aff22..c8b8682c4b 100644 --- a/.bazelrc +++ b/.bazelrc @@ -105,9 +105,9 @@ common:ci --disk_cache= # Shared config for the main Bazel CI workflow. common:ci-bazel --config=ci common:ci-bazel --build_metadata=TAG_workflow=bazel -# Bazel CI cross-compiles in several legs, and the V8-backed code-mode tests -# are not stable in that setup yet. Keep running the rest of the Rust -# integration suites through the workspace-root launcher. +# Keep code-mode integration cases out of ordinary Bazel legs. The +# Windows-cross config below re-enables them after generating its Windows V8 +# snapshot on the Windows runner. common:ci-bazel --test_env=CODEX_BAZEL_TEST_SKIP_FILTERS=suite::code_mode:: # Shared config for Bazel-backed Rust linting. @@ -186,12 +186,16 @@ common:ci-windows-cross --config=ci-windows common:ci-windows-cross --build_metadata=TAG_windows_cross_compile=true common:ci-windows-cross --host_platform=//:rbe common:ci-windows-cross --strategy=TestRunner=local +# V8 embeds IsolateData offsets in snapshot builtins; Windows snapshots must be +# generated by a Windows mksnapshot binary rather than the Linux RBE host tool. +common:ci-windows-cross --strategy=V8Mksnapshot=local common:ci-windows-cross --local_test_jobs=4 common:ci-windows-cross --test_env=RUST_TEST_THREADS=1 # Native Windows CI still covers the PowerShell tests. The cross-built gnullvm # binaries currently hang in PowerShell AST parser tests when those binaries are -# run on the Windows runner. -common:ci-windows-cross --test_env=CODEX_BAZEL_TEST_SKIP_FILTERS=suite::code_mode::,powershell +# run on the Windows runner. Keep V8-backed code-mode tests enabled except for +# the hidden dynamic-tool callback test, which currently times out on Windows. +common:ci-windows-cross --test_env=CODEX_BAZEL_TEST_SKIP_FILTERS=powershell,suite::code_mode::code_mode_can_call_hidden_dynamic_tools common:ci-windows-cross --platforms=//:windows_x86_64_gnullvm common:ci-windows-cross --extra_execution_platforms=//:rbe,//:windows_x86_64_msvc common:ci-windows-cross --extra_toolchains=//:windows_gnullvm_tests_on_msvc_host_toolchain diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index c6dfd60231..bddbb5adc9 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -20,8 +20,9 @@ jobs: test: # PRs use the sharded Windows cross-compiled test jobs below. Post-merge # pushes to main also run the native Windows test job for broader Windows - # signal without putting PR latency back on the critical path. Cargo CI - # owns V8/code-mode test coverage for now. + # signal without putting PR latency back on the critical path. When + # authenticated RBE is available, the Windows-cross shards exercise the + # source-built V8/code-mode targets. timeout-minutes: 30 strategy: fail-fast: false @@ -91,9 +92,9 @@ jobs: # path. V8 consumers under `//codex-rs/...` still participate # transitively through `//...`. -//third_party/v8:all - # V8-backed code-mode tests are covered by Cargo CI. Bazel CI - # cross-compiles in several legs, and those tests are not stable in - # that setup yet. + # Keep V8-backed code-mode tests out of the ordinary macOS/Linux + # legs; authenticated Windows-cross shards below exercise the + # source-built gnullvm V8 path. -//codex-rs/code-mode:code-mode-unit-tests -//codex-rs/v8-poc:v8-poc-unit-tests ) @@ -135,9 +136,9 @@ jobs: key: ${{ steps.prepare_bazel.outputs.repository-cache-key }} test-windows-shard: - # Split the Windows Bazel test leg across separate Windows - # hosts. Each shard still uses Linux RBE for build actions, but the test - # execution itself happens on its own Windows runner. + # Split the Windows Bazel test leg across separate Windows hosts. Jobs with + # BuildBuddy credentials use Linux RBE for build actions; test execution + # remains on a Windows runner. timeout-minutes: 30 strategy: fail-fast: false @@ -183,7 +184,7 @@ jobs: run: | set -euo pipefail - bazel_test_query='tests(//...) except tests(//third_party/v8:all) except //codex-rs/code-mode:code-mode-unit-tests except //codex-rs/v8-poc:v8-poc-unit-tests except attr(tags, "manual", tests(//...))' + bazel_test_query='tests(//...) except tests(//third_party/v8:all) except attr(tags, "manual", tests(//...))' mapfile -t bazel_targets < <( MSYS2_ARG_CONV_EXCL='*' bazel query --output=label "${bazel_test_query}" \ | LC_ALL=C sort @@ -289,9 +290,8 @@ jobs: # path. V8 consumers under `//codex-rs/...` still participate # transitively through `//...`. -//third_party/v8:all - # Keep this aligned with the main Bazel job. The native Windows - # job preserves broad post-merge coverage, but code-mode/V8 tests - # are covered by Cargo CI rather than Bazel for now. + # Keep this job broad and cheap; authenticated Windows-cross jobs + # add source-built V8-backed code-mode coverage. -//codex-rs/code-mode:code-mode-unit-tests -//codex-rs/v8-poc:v8-poc-unit-tests ) diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index ffe23bff56..bc311d4245 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -375,7 +375,11 @@ async fn code_mode_only_restricts_prompt_tools() -> Result<()> { let first_body = resp_mock.single_request().body_json(); assert_eq!( tool_names(&first_body), - vec!["exec".to_string(), "wait".to_string()] + vec![ + "exec".to_string(), + "wait".to_string(), + "web_search".to_string() + ] ); Ok(()) @@ -457,7 +461,12 @@ if (!tool) { let first_body = resp_mock.single_request().body_json(); assert_eq!( tool_names(&first_body), - vec!["exec".to_string(), "wait".to_string()] + vec![ + "exec".to_string(), + "wait".to_string(), + "web_search".to_string(), + "image_generation".to_string() + ] ); let exec_description = first_body @@ -2898,6 +2907,7 @@ text(JSON.stringify(Object.getOwnPropertyNames(globalThis).sort())); "escape", "exit", "eval", + "generatedImage", "globalThis", "image", "isFinite", @@ -2953,7 +2963,7 @@ text(JSON.stringify(tool)); parsed, serde_json::json!({ "name": "view_image", - "description": "View a local image file from the filesystem when visual inspection is needed. Use this for images already available on disk.\n\nexec tool declaration:\n```ts\ndeclare const tools: { view_image(args: {\n // Local filesystem path to an image file\n path: string;\n}): Promise<{\n // Image detail hint returned by view_image. Returns `high` for default resized behavior or `original` when original resolution is preserved.\n detail: \"high\" | \"original\";\n // Data URL for the loaded image.\n image_url: string;\n}>; };\n```", + "description": "View a local image file from the filesystem when visual inspection is needed. Use this for images already available on disk.\n\nexec tool declaration:\n```ts\ndeclare const tools: { view_image(args: {\n // Local filesystem path to an image file.\n path: string;\n}): Promise<{\n // Image detail hint returned by view_image. Returns `high` for default resized behavior or `original` when original resolution is preserved.\n detail: \"high\" | \"original\";\n // Data URL for the loaded image.\n image_url: string;\n}>; };\n```", }) ); diff --git a/patches/v8_bazel_rules.patch b/patches/v8_bazel_rules.patch index 1c49d06b09..97e07d9d47 100644 --- a/patches/v8_bazel_rules.patch +++ b/patches/v8_bazel_rules.patch @@ -106,6 +106,55 @@ index 9648e4a..88efd41 100644 }) + select({ ":should_add_rdynamic": ["-rdynamic"], "//conditions:default": [], +@@ -459,6 +488,11 @@ + def _mksnapshot(ctx): + prefix = ctx.attr.prefix + suffix = ctx.attr.suffix ++ # Windows cross-builds use Linux for the exec configuration, but the ++ # snapshot generator must match the target ABI and run on the Windows ++ # runner. Action strategies only choose where an action runs; they cannot ++ # change this executable from the exec to the target configuration. ++ tool = ctx.executable.target_tool if ctx.attr.target_os == "win" else ctx.executable.tool + outs = [ + ctx.actions.declare_file(prefix + "/snapshot" + suffix + ".cc"), + ctx.actions.declare_file(prefix + "/embedded" + suffix + ".S"), +@@ -476,7 +510,7 @@ + outs[1].path, + ] + ctx.attr.args, +- executable = ctx.executable.tool, ++ executable = tool, + progress_message = "Running mksnapshot", + ) + return [DefaultInfo(files = depset(outs))] +@@ -491,6 +525,12 @@ + executable = True, + cfg = "exec", + ), ++ "target_tool": attr.label( ++ mandatory = True, ++ allow_files = True, ++ executable = True, ++ cfg = "target", ++ ), + "target_os": attr.string(mandatory = True), + "prefix": attr.string(mandatory = True), + "suffix": attr.string(mandatory = True), +@@ -504,6 +544,7 @@ + args = args, + prefix = "noicu", + tool = ":noicu/mksnapshot" + suffix, ++ target_tool = ":noicu/mksnapshot" + suffix, + suffix = suffix, + target_os = select({ + "@v8//bazel/config:is_macos": "mac", +@@ -516,6 +557,7 @@ + args = args, + prefix = "icu", + tool = ":icu/mksnapshot" + suffix, ++ target_tool = ":icu/mksnapshot" + suffix, + suffix = suffix, + target_os = select({ + "@v8//bazel/config:is_macos": "mac", diff --git a/orig/v8-14.6.202.11/BUILD.bazel b/mod/v8-14.6.202.11/BUILD.bazel index 421ebcd..52283ea 100644 --- a/orig/v8-14.6.202.11/BUILD.bazel @@ -309,7 +358,7 @@ index 421ebcd..52283ea 100644 ], ) -@@ -4772,9 +4784,15 @@ v8_binary( +@@ -4772,9 +4784,20 @@ v8_binary( ":icu/generated_torque_initializers", ":icu/v8_initializers_files", ], @@ -317,11 +366,16 @@ index 421ebcd..52283ea 100644 + # external-reference helpers together while producing the snapshot, the + # final embedder binary may not fold the same pair and startup + # deserialization will reject the snapshot. ++ # GN also increases the x64 Windows stack reserve because constructing the ++ # snapshot overflows the linker's 1 MiB default. linkopts = select({ "@v8//bazel/config:is_android": ["-llog"], - "//conditions:default": [], + "@v8//bazel/config:is_macos": ["-Wl,-no_deduplicate"], -+ "@v8//bazel/config:is_windows": ["/OPT:NOICF"], ++ "@v8//bazel/config:is_windows": [ ++ "-Wl,/OPT:NOICF", ++ "-Wl,/STACK:2097152", ++ ], + "//conditions:default": ["-Wl,--icf=none"], }), noicu_deps = [":v8_libshared_noicu"], From ecae41274091252f454eb5ed0507036ed4e6fc19 Mon Sep 17 00:00:00 2001 From: rreichel3-oai Date: Thu, 4 Jun 2026 20:58:14 -0400 Subject: [PATCH 04/10] [codex] Emit sandbox outcome telemetry event (#25955) ## Summary Adds a dedicated `codex.sandbox_outcome` telemetry event so we can query sandbox edge outcomes without threading sandbox metadata through tool-result output types. This is meant to make sandbox failures and approved escalation retries visible in OTEL while keeping the existing `codex.tool_result` event shape focused on tool completion data. ## What changed - Adds `SessionTelemetry::sandbox_outcome(...)`, which emits `codex.sandbox_outcome` as both a log and trace event. - Records the tool name, call id, sandbox outcome, initial attempt duration, and escalated attempt duration when a retry runs. - Emits `denied` when the sandbox blocks execution and no retry is run. - Emits `timed_out` and `signal` when those sandbox errors surface from tool execution. - Emits `escalated` when the initial sandboxed attempt fails and the approved unsandboxed retry succeeds. - Adds OTEL coverage for the new event payload, including timing fields. ## Validation - `RUST_MIN_STACK=8388608 just test -p codex-core sandbox_outcome_event_records_outcome handle_sandbox_error_user_approves_retry_records_tool_decision` - `just test -p codex-otel otel_export_routing_policy_routes_tool_result_log_and_trace_events runtime_metrics_summary_collects_tool_api_and_streaming_metrics` - `just fix -p codex-core` - `just fix -p codex-otel` --- codex-rs/core/src/tools/orchestrator.rs | 86 +++++++++++++++++-- codex-rs/core/tests/suite/otel.rs | 69 +++++++++++++++ codex-rs/otel/src/events/session_telemetry.rs | 31 +++++++ 3 files changed, 181 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 235f8c4e96..77cbffde32 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -39,6 +39,7 @@ use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; +use std::time::Instant; pub(crate) struct ToolOrchestrator { sandbox: SandboxManager, @@ -256,6 +257,7 @@ impl ToolOrchestrator { network_denial_cancellation_token: None, }; + let initial_attempt_start = Instant::now(); let (first_result, first_deferred_network_approval) = Self::run_attempt( tool, req, @@ -264,6 +266,7 @@ impl ToolOrchestrator { managed_network_active, ) .await; + let initial_duration = initial_attempt_start.elapsed(); match first_result { Ok(out) => { // We have a successful initial result @@ -284,12 +287,26 @@ impl ToolOrchestrator { None }; if network_policy_decision.is_some() && network_approval_context.is_none() { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + "denied", + initial_duration, + /*escalated_duration*/ None, + ); return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, network_policy_decision, }))); } if !tool.escalate_on_failure() { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + "denied", + initial_duration, + /*escalated_duration*/ None, + ); return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, network_policy_decision, @@ -312,6 +329,13 @@ impl ToolOrchestrator { ExecApprovalRequirement::NeedsApproval { .. } ); if !allow_on_request_network_prompt { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + "denied", + initial_duration, + /*escalated_duration*/ None, + ); return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, network_policy_decision, @@ -319,6 +343,13 @@ impl ToolOrchestrator { } } if !unsandboxed_allowed && network_approval_context.is_none() { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + "denied", + initial_duration, + /*escalated_duration*/ None, + ); return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output, network_policy_decision, @@ -400,15 +431,51 @@ impl ToolOrchestrator { }; // Second attempt. + let escalated_attempt_start = Instant::now(); let (retry_result, retry_deferred_network_approval) = Self::run_attempt(tool, req, tool_ctx, &retry_attempt, managed_network_active) .await; - retry_result.map(|output| OrchestratorRunResult { - output, - deferred_network_approval: retry_deferred_network_approval, - }) + let escalated_duration = escalated_attempt_start.elapsed(); + match retry_result { + Ok(output) => { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + "escalated", + initial_duration, + Some(escalated_duration), + ); + Ok(OrchestratorRunResult { + output, + deferred_network_approval: retry_deferred_network_approval, + }) + } + Err(err) => { + if let Some(outcome) = sandbox_outcome_from_tool_error(&err) { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + outcome, + initial_duration, + Some(escalated_duration), + ); + } + Err(err) + } + } + } + Err(err) => { + if let Some(outcome) = sandbox_outcome_from_tool_error(&err) { + otel.sandbox_outcome( + &otel_tn, + otel_ci, + outcome, + initial_duration, + /*escalated_duration*/ None, + ); + } + Err(err) } - Err(err) => Err(err), } } @@ -509,6 +576,15 @@ impl ToolOrchestrator { } } +fn sandbox_outcome_from_tool_error(err: &ToolError) -> Option<&'static str> { + match err { + ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { .. })) => Some("denied"), + ToolError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { .. })) => Some("timed_out"), + ToolError::Codex(CodexErr::Sandbox(SandboxErr::Signal(_))) => Some("signal"), + ToolError::Rejected(_) | ToolError::Codex(_) => None, + } +} + fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { // Keep approval reason terse and stable for UX/tests, but accept the // output so we can evolve heuristics later without touching call sites. diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 7f5fa260cd..d6c7577a7f 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1,11 +1,15 @@ use codex_core::config::Constrained; use codex_features::Feature; +use codex_otel::SessionTelemetry; +use codex_otel::TelemetryAuthMode; +use codex_protocol::ThreadId; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::ReviewDecision; +use codex_protocol::protocol::SessionSource; use codex_protocol::user_input::UserInput; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -27,6 +31,7 @@ use core_test_support::test_codex::TestCodex; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use std::sync::Mutex; +use std::time::Duration; use tracing::Level; use tracing_test::traced_test; @@ -1145,6 +1150,70 @@ fn tool_decision_assertion<'a>( } } +fn sandbox_outcome_assertion<'a>( + call_id: &'a str, + expected_outcome: &'a str, +) -> impl Fn(&[&str]) -> Result<(), String> + 'a { + let call_id = call_id.to_string(); + let expected_outcome = expected_outcome.to_string(); + + move |lines: &[&str]| { + let line = lines + .iter() + .find(|line| { + line.contains("codex.sandbox_outcome") + && line.contains(&format!("call_id={call_id}")) + }) + .ok_or_else(|| format!("missing codex.sandbox_outcome event for {call_id}"))?; + + let lower = line.to_lowercase(); + if !lower.contains("tool_name=shell_command") { + return Err("missing tool_name for shell_command".to_string()); + } + if !lower.contains(&format!("outcome={expected_outcome}")) { + return Err(format!("unexpected sandbox outcome for {call_id}")); + } + if !lower.contains("initial_duration_ms=12") { + return Err("missing initial_duration_ms field".to_string()); + } + if !lower.contains("escalated_duration_ms=34") { + return Err("missing escalated_duration_ms field".to_string()); + } + + Ok(()) + } +} + +#[test] +#[traced_test] +fn sandbox_outcome_event_records_outcome() { + let telemetry = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.5", + "gpt-5.5", + /*account_id*/ None, + /*account_email*/ None, + Some(TelemetryAuthMode::ApiKey), + "Codex_Desktop".to_string(), + /*log_user_prompts*/ false, + "tty".to_string(), + SessionSource::Cli, + ); + + telemetry.sandbox_outcome( + "shell_command", + "sandbox-outcome-call", + "escalated", + Duration::from_millis(/*millis*/ 12), + Some(Duration::from_millis(/*millis*/ 34)), + ); + + logs_assert(sandbox_outcome_assertion( + "sandbox-outcome-call", + "escalated", + )); +} + #[tokio::test] #[traced_test] async fn handle_shell_command_autoapprove_from_config_records_tool_decision() { diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index f4534a0791..0003cb04a5 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -998,6 +998,37 @@ impl SessionTelemetry { ); } + pub fn sandbox_outcome( + &self, + tool_name: &str, + call_id: &str, + outcome: &str, + initial_duration: Duration, + escalated_duration: Option, + ) { + let initial_duration_ms = initial_duration.as_millis().min(i64::MAX as u128) as i64; + let escalated_duration_ms = + escalated_duration.map(|duration| duration.as_millis().min(i64::MAX as u128) as i64); + log_event!( + self, + event.name = "codex.sandbox_outcome", + tool_name = %tool_name, + call_id = %call_id, + outcome = %outcome, + initial_duration_ms = initial_duration_ms, + escalated_duration_ms = escalated_duration_ms, + ); + trace_event!( + self, + event.name = "codex.sandbox_outcome", + tool_name = %tool_name, + call_id = %call_id, + outcome = %outcome, + initial_duration_ms = initial_duration_ms, + escalated_duration_ms = escalated_duration_ms, + ); + } + #[allow(clippy::too_many_arguments)] pub async fn log_tool_result_with_tags( &self, From e0096db6dc8a2f89394986123802a676a1be20c5 Mon Sep 17 00:00:00 2001 From: rka-oai Date: Thu, 4 Jun 2026 18:49:51 -0700 Subject: [PATCH 05/10] [codex] Add use_responses_lite 'override' logic (#26487) ## Summary - add a defaulted `ModelInfo.use_responses_lite` catalog field - support serializing `reasoning.context` while preserving the existing effort and summary path - has not been turned on for any models yet I've added an override to parallel tools if responses_lite is on. I've also forced persistent reasoning when using responses_lite. It would be ideal if we could centralize all the responses_lite plumbing, but I think this is best for now to keep the plumbing & diffs small. ## Testing - `cargo test -p codex-protocol model_info_defaults_availability_nux_to_none_when_omitted` - `RUST_MIN_STACK=8388608 cargo test -p codex-core responses_lite_sets_all_turns_context_and_disables_parallel_tool_calls` - `RUST_MIN_STACK=8388608 cargo test -p codex-core configured_reasoning_summary_is_sent` - `cargo check -p codex-core --tests` - `RUST_MIN_STACK=8388608 cargo clippy -p codex-core --tests` (passes with pre-existing warnings in `codex-code-mode` and `codex-core-plugins`) --- .../app-server/tests/common/models_cache.rs | 1 + codex-rs/codex-api/src/common.rs | 10 +++ codex-rs/codex-api/src/lib.rs | 1 + .../codex-api/tests/models_integration.rs | 1 + codex-rs/core/src/client.rs | 9 ++- codex-rs/core/tests/suite/auto_review.rs | 1 + codex-rs/core/tests/suite/client.rs | 65 +++++++++++++++++++ codex-rs/core/tests/suite/model_switching.rs | 2 + codex-rs/core/tests/suite/models_cache_ttl.rs | 1 + codex-rs/core/tests/suite/personality.rs | 2 + codex-rs/core/tests/suite/remote_models.rs | 3 + codex-rs/core/tests/suite/rmcp_client.rs | 1 + .../tests/suite/spawn_agent_description.rs | 1 + codex-rs/core/tests/suite/view_image.rs | 1 + codex-rs/models-manager/src/model_info.rs | 1 + codex-rs/protocol/src/openai_models.rs | 4 ++ codex-rs/tools/src/tool_config_tests.rs | 1 + 17 files changed, 104 insertions(+), 1 deletion(-) diff --git a/codex-rs/app-server/tests/common/models_cache.rs b/codex-rs/app-server/tests/common/models_cache.rs index 127c14bbb4..8233b1b296 100644 --- a/codex-rs/app-server/tests/common/models_cache.rs +++ b/codex-rs/app-server/tests/common/models_cache.rs @@ -52,6 +52,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/codex-api/src/common.rs b/codex-rs/codex-api/src/common.rs index 50ac2685b4..12f8cc55da 100644 --- a/codex-rs/codex-api/src/common.rs +++ b/codex-rs/codex-api/src/common.rs @@ -110,12 +110,22 @@ pub enum ResponseEvent { ModelsEtag(String), } +#[derive(Debug, Serialize, Clone, PartialEq)] +#[serde(rename_all = "snake_case")] +pub enum ReasoningContext { + Auto, + CurrentTurn, + AllTurns, +} + #[derive(Debug, Serialize, Clone, PartialEq)] pub struct Reasoning { #[serde(skip_serializing_if = "Option::is_none")] pub effort: Option, #[serde(skip_serializing_if = "Option::is_none")] pub summary: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub context: Option, } #[derive(Debug, Serialize, Default, Clone, PartialEq)] diff --git a/codex-rs/codex-api/src/lib.rs b/codex-rs/codex-api/src/lib.rs index 8efebf076c..d2d7679406 100644 --- a/codex-rs/codex-api/src/lib.rs +++ b/codex-rs/codex-api/src/lib.rs @@ -30,6 +30,7 @@ pub use crate::common::OpenAiVerbosity; pub use crate::common::RawMemory; pub use crate::common::RawMemoryMetadata; pub use crate::common::Reasoning; +pub use crate::common::ReasoningContext; pub use crate::common::ResponseCreateWsRequest; pub use crate::common::ResponseEvent; pub use crate::common::ResponseStream; diff --git a/codex-rs/codex-api/tests/models_integration.rs b/codex-rs/codex-api/tests/models_integration.rs index 9ff273211f..4ee6069898 100644 --- a/codex-rs/codex-api/tests/models_integration.rs +++ b/codex-rs/codex-api/tests/models_integration.rs @@ -98,6 +98,7 @@ async fn models_client_hits_models_endpoint() { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 9c133d85e4..ee8ea3619f 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -44,6 +44,7 @@ use codex_api::RawMemory as ApiRawMemory; use codex_api::RealtimeCallClient as ApiRealtimeCallClient; use codex_api::RealtimeSessionConfig as ApiRealtimeSessionConfig; use codex_api::Reasoning; +use codex_api::ReasoningContext; use codex_api::RequestTelemetry; use codex_api::ReqwestTransport; use codex_api::ResponseCreateWsRequest; @@ -609,6 +610,7 @@ impl ModelClient { reasoning: effort.map(|effort| Reasoning { effort: Some(effort), summary: None, + context: None, }), }; @@ -727,6 +729,11 @@ impl ModelClient { } else { Some(summary) }, + // When Responses Lite is disabled, omit context so Responses uses the default, + // which is currently `current_turn`. + context: model_info + .use_responses_lite + .then_some(ReasoningContext::AllTurns), }) } else { None @@ -775,7 +782,7 @@ impl ModelClient { input, tools, tool_choice: "auto".to_string(), - parallel_tool_calls: prompt.parallel_tool_calls, + parallel_tool_calls: prompt.parallel_tool_calls && !model_info.use_responses_lite, reasoning, store: provider.is_azure_responses_endpoint(), stream: true, diff --git a/codex-rs/core/tests/suite/auto_review.rs b/codex-rs/core/tests/suite/auto_review.rs index 570f97008f..64ae10d688 100644 --- a/codex-rs/core/tests/suite/auto_review.rs +++ b/codex-rs/core/tests/suite/auto_review.rs @@ -231,6 +231,7 @@ fn remote_model_with_auto_review_override(slug: &str, review_model: &str) -> Mod input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: Some(review_model.to_string()), tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index ad8ff17cdc..72e415e2be 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1872,6 +1872,71 @@ async fn configured_reasoning_summary_is_sent() -> anyhow::Result<()> { .and_then(|value| value.as_str()), Some("concise") ); + pretty_assertions::assert_eq!( + request_body + .get("reasoning") + .and_then(|reasoning| reasoning.get("context")), + None + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn responses_lite_sets_all_turns_context_and_disables_parallel_tool_calls() +-> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + let server = MockServer::start().await; + + let resp_mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + + let mut model_catalog = bundled_models_response() + .unwrap_or_else(|err| panic!("bundled models.json should parse: {err}")); + let model = model_catalog + .models + .iter_mut() + .find(|model| model.slug == "gpt-5.4") + .expect("gpt-5.4 exists in bundled models.json"); + model.use_responses_lite = true; + model.supports_parallel_tool_calls = true; + + let TestCodex { codex, .. } = test_codex() + .with_model("gpt-5.4") + .with_config(move |config| { + config.model_catalog = Some(model_catalog); + }) + .build(&server) + .await?; + + codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let request_body = resp_mock.single_request().body_json(); + pretty_assertions::assert_eq!( + request_body + .get("reasoning") + .and_then(|reasoning| reasoning.get("context")) + .and_then(|value| value.as_str()), + Some("all_turns") + ); + pretty_assertions::assert_eq!(request_body.get("parallel_tool_calls"), Some(&json!(false))); Ok(()) } diff --git a/codex-rs/core/tests/suite/model_switching.rs b/codex-rs/core/tests/suite/model_switching.rs index 111ce50436..a80c43660c 100644 --- a/codex-rs/core/tests/suite/model_switching.rs +++ b/codex-rs/core/tests/suite/model_switching.rs @@ -112,6 +112,7 @@ fn test_model_info( input_modalities, used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, @@ -932,6 +933,7 @@ async fn model_switch_to_smaller_model_updates_token_context_window() -> Result< input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/models_cache_ttl.rs b/codex-rs/core/tests/suite/models_cache_ttl.rs index 34fc98b59f..d27c43cfc6 100644 --- a/codex-rs/core/tests/suite/models_cache_ttl.rs +++ b/codex-rs/core/tests/suite/models_cache_ttl.rs @@ -370,6 +370,7 @@ fn test_remote_model(slug: &str, priority: i32) -> ModelInfo { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index cd264c0734..bf411d9cd1 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -592,6 +592,7 @@ async fn remote_model_friendly_personality_instructions_with_feature() -> anyhow input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, @@ -705,6 +706,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 608558d365..0ed39216d0 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -478,6 +478,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, @@ -730,6 +731,7 @@ async fn remote_models_apply_remote_base_instructions() -> Result<()> { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, @@ -1216,6 +1218,7 @@ fn test_remote_model_with_policy( input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 36ea7d4b2c..552c4ed529 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -1349,6 +1349,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re input_modalities: vec![InputModality::Text], used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/spawn_agent_description.rs b/codex-rs/core/tests/suite/spawn_agent_description.rs index 3b581c9051..ba29b4f00c 100644 --- a/codex-rs/core/tests/suite/spawn_agent_description.rs +++ b/codex-rs/core/tests/suite/spawn_agent_description.rs @@ -60,6 +60,7 @@ fn test_model_info( input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 2e34475387..19758592c3 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -1355,6 +1355,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an input_modalities: vec![InputModality::Text], used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/models-manager/src/model_info.rs b/codex-rs/models-manager/src/model_info.rs index 58137a3f50..81a5c6e5ed 100644 --- a/codex-rs/models-manager/src/model_info.rs +++ b/codex-rs/models-manager/src/model_info.rs @@ -99,6 +99,7 @@ pub fn model_info_from_slug(slug: &str) -> ModelInfo { input_modalities: default_input_modalities(), used_fallback_model_metadata: true, // this is the fallback model metadata supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 4c04edbb56..1553ef3435 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -404,6 +404,8 @@ pub struct ModelInfo { pub used_fallback_model_metadata: bool, #[serde(default)] pub supports_search_tool: bool, + #[serde(default)] + pub use_responses_lite: bool, #[serde(default, skip_serializing_if = "Option::is_none")] pub auto_review_model_override: Option, #[serde( @@ -674,6 +676,7 @@ mod tests { input_modalities: default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, @@ -936,6 +939,7 @@ mod tests { assert!(!model.supports_image_detail_original); assert_eq!(model.web_search_tool_type, WebSearchToolType::Text); assert!(!model.supports_search_tool); + assert!(!model.use_responses_lite); assert_eq!(model.auto_review_model_override, None); assert_eq!(model.tool_mode, None); } diff --git a/codex-rs/tools/src/tool_config_tests.rs b/codex-rs/tools/src/tool_config_tests.rs index 1b4d789394..0e3917d697 100644 --- a/codex-rs/tools/src/tool_config_tests.rs +++ b/codex-rs/tools/src/tool_config_tests.rs @@ -44,6 +44,7 @@ fn model_with_shell_type(shell_type: ConfigShellToolType) -> ModelInfo { input_modalities: codex_protocol::openai_models::default_input_modalities(), used_fallback_model_metadata: false, supports_search_tool: false, + use_responses_lite: false, auto_review_model_override: None, tool_mode: None, multi_agent_version: None, From 4de7a2b9d8eae19e00ca7f744647fa1aabdc204f Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 4 Jun 2026 19:31:06 -0700 Subject: [PATCH 06/10] fix(rmcp): refresh expired OAuth tokens before startup (#26482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Codex persists OAuth expiry as an absolute `expires_at`, then reconstructs RMCP’s relative `expires_in` when credentials are loaded. For an already-expired token, Codex reconstructed `expires_in` as missing. [RMCP 0.15 treated a missing `expires_in` as zero when a refresh token was present](https://github.com/modelcontextprotocol/rust-sdk/blob/9cfc905a9ef17c8bba6748dc0a9bdd2452681733/crates/rmcp/src/transport/auth.rs#L704-L723), so this still triggered a refresh. [RMCP 1.7 treats missing expiry information as unknown and uses the access token as-is](https://github.com/modelcontextprotocol/rust-sdk/blob/3529c3675ff64db805bd947ca6ece6090809e43d/crates/rmcp/src/transport/auth.rs#L1233-L1265), causing the stale token to be sent during `initialize`. ## What changed - Represent a known-expired persisted token as `expires_in = 0`, preserving `None` for genuinely unknown expiry. - Add Streamable HTTP coverage requiring the token to refresh before the startup handshake. ## Validation - The new regression test fails on RMCP 1.7 before the fix and passes afterward. - The same scenario passes on the commit immediately before the RMCP 1.7 update, using RMCP 0.15. - `just test -p codex-rmcp-client` (63 passed). --- codex-rs/Cargo.lock | 1 + codex-rs/rmcp-client/Cargo.toml | 1 + codex-rs/rmcp-client/src/oauth.rs | 12 +- .../tests/streamable_http_oauth_startup.rs | 155 ++++++++++++++++++ .../tests/streamable_http_test_support.rs | 9 +- 5 files changed, 173 insertions(+), 5 deletions(-) create mode 100644 codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1e79865d43..6da7688435 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3597,6 +3597,7 @@ dependencies = [ "urlencoding", "webbrowser", "which 8.0.0", + "wiremock", ] [[package]] diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index e3417de70e..1006ad74e8 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -70,6 +70,7 @@ codex-utils-cargo-bin = { workspace = true } pretty_assertions = { workspace = true } serial_test = { workspace = true } tempfile = { workspace = true } +wiremock = { workspace = true } [target.'cfg(target_os = "linux")'.dependencies] keyring = { workspace = true, features = ["linux-native-async-persistent"] } diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index e23eee84be..c348460795 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -113,7 +113,13 @@ fn refresh_expires_in_from_timestamp(tokens: &mut StoredOAuthTokens) { tokens.token_response.0.set_expires_in(Some(&duration)); } None => { - tokens.token_response.0.set_expires_in(None); + // RMCP treats a missing expiry as unknown and uses the access token + // as-is. Treat a known-expired timestamp as an explicit zero so + // startup refreshes the token before the first request. + tokens + .token_response + .0 + .set_expires_in(Some(&Duration::ZERO)); } } } @@ -830,7 +836,7 @@ mod tests { } #[test] - fn refresh_expires_in_from_timestamp_clears_expired_tokens() { + fn refresh_expires_in_from_timestamp_marks_expired_tokens() { let mut tokens = sample_tokens(); let now = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -843,7 +849,7 @@ mod tests { super::refresh_expires_in_from_timestamp(&mut tokens); - assert!(tokens.token_response.0.expires_in().is_none()); + assert_eq!(tokens.token_response.0.expires_in(), Some(Duration::ZERO)); } fn assert_tokens_match_without_expiry( diff --git a/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs new file mode 100644 index 0000000000..1c18f2c98b --- /dev/null +++ b/codex-rs/rmcp-client/tests/streamable_http_oauth_startup.rs @@ -0,0 +1,155 @@ +mod streamable_http_test_support; + +use std::time::Duration; + +use codex_config::types::OAuthCredentialsStoreMode; +use codex_exec_server::Environment; +use codex_rmcp_client::RmcpClient; +use codex_rmcp_client::StoredOAuthTokens; +use codex_rmcp_client::WrappedOAuthTokenResponse; +use codex_rmcp_client::save_oauth_tokens; +use oauth2::AccessToken; +use oauth2::RefreshToken; +use oauth2::basic::BasicTokenType; +use rmcp::transport::auth::OAuthTokenResponse; +use rmcp::transport::auth::VendorExtraTokenFields; +use serde_json::Value; +use serde_json::json; +use tempfile::TempDir; +use tokio::process::Command; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::Request; +use wiremock::ResponseTemplate; +use wiremock::matchers::body_string_contains; +use wiremock::matchers::header; +use wiremock::matchers::method; +use wiremock::matchers::path; + +use streamable_http_test_support::initialize_client; + +const SERVER_NAME: &str = "test-streamable-http-oauth-startup"; +const EXPIRED_ACCESS_TOKEN: &str = "expired-access-token"; +const REFRESH_TOKEN: &str = "valid-refresh-token"; +const REFRESHED_ACCESS_TOKEN: &str = "refreshed-access-token"; +const CHILD_SERVER_URL_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SERVER_URL"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn refreshes_expired_persisted_token_before_initialize() -> anyhow::Result<()> { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/.well-known/oauth-authorization-server/mcp")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "authorization_endpoint": format!("{}/oauth/authorize", server.uri()), + "token_endpoint": format!("{}/oauth/token", server.uri()), + "scopes_supported": [""], + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .and(body_string_contains("grant_type=refresh_token")) + .and(body_string_contains(format!( + "refresh_token={REFRESH_TOKEN}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "access_token": REFRESHED_ACCESS_TOKEN, + "token_type": "Bearer", + "expires_in": 7200, + "refresh_token": REFRESH_TOKEN, + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/mcp")) + .and(header( + "authorization", + format!("Bearer {REFRESHED_ACCESS_TOKEN}"), + )) + .respond_with(|request: &Request| { + let body: Value = request.body_json().expect("valid JSON-RPC request"); + match body.get("method").and_then(Value::as_str) { + Some("initialize") => ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": body.get("id").cloned().unwrap_or(Value::Null), + "result": { + "protocolVersion": body + .pointer("/params/protocolVersion") + .cloned() + .unwrap_or_else(|| json!("2025-06-18")), + "capabilities": {}, + "serverInfo": { + "name": "oauth-startup-test", + "version": "0.0.0-test", + }, + }, + })), + Some("notifications/initialized") => ResponseTemplate::new(202), + method => ResponseTemplate::new(400) + .set_body_string(format!("unexpected JSON-RPC method: {method:?}")), + } + }) + .expect(2) + .mount(&server) + .await; + + let codex_home = TempDir::new()?; + let server_url = format!("{}/mcp", server.uri()); + + // Credential storage resolves CODEX_HOME from the process environment. + // Run the client half of the test in an ignored helper test so it can use + // an isolated home without mutating the parent test runner's environment. + let status = Command::new(std::env::current_exe()?) + .args(["oauth_startup_child", "--exact", "--ignored", "--nocapture"]) + .env("CODEX_HOME", codex_home.path()) + .env(CHILD_SERVER_URL_ENV, server_url) + .status() + .await?; + assert!(status.success(), "OAuth startup child failed: {status}"); + server.verify().await; + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[ignore = "spawned by refreshes_expired_persisted_token_before_initialize"] +async fn oauth_startup_child() -> anyhow::Result<()> { + let server_url = std::env::var(CHILD_SERVER_URL_ENV)?; + + // Save an expired access token with a valid refresh token so startup must + // refresh before sending the initialize request. + let mut response = OAuthTokenResponse::new( + AccessToken::new(EXPIRED_ACCESS_TOKEN.to_string()), + BasicTokenType::Bearer, + VendorExtraTokenFields::default(), + ); + response.set_refresh_token(Some(RefreshToken::new(REFRESH_TOKEN.to_string()))); + response.set_expires_in(Some(&Duration::from_secs(7200))); + let tokens = StoredOAuthTokens { + server_name: SERVER_NAME.to_string(), + url: server_url.clone(), + client_id: "test-client-id".to_string(), + token_response: WrappedOAuthTokenResponse(response), + expires_at: Some(0), + }; + save_oauth_tokens(SERVER_NAME, &tokens, OAuthCredentialsStoreMode::File)?; + + // This mirrors create_client's transport and initialization setup, except + // it omits the direct bearer token. Supplying that token would bypass the + // persisted OAuth credentials and the startup refresh under test. + let client = RmcpClient::new_streamable_http_client( + SERVER_NAME, + &server_url, + /*bearer_token*/ None, + /*http_headers*/ None, + /*env_http_headers*/ None, + OAuthCredentialsStoreMode::File, + Environment::default_for_tests().get_http_client(), + /*auth_provider*/ None, + ) + .await?; + + initialize_client(&client).await?; + Ok(()) +} diff --git a/codex-rs/rmcp-client/tests/streamable_http_test_support.rs b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs index 822acef1a2..03ee79392c 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_test_support.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs @@ -86,6 +86,12 @@ pub(crate) async fn create_client(base_url: &str) -> anyhow::Result ) .await?; + initialize_client(&client).await?; + + Ok(client) +} + +pub(crate) async fn initialize_client(client: &RmcpClient) -> anyhow::Result<()> { client .initialize( init_params(), @@ -102,8 +108,7 @@ pub(crate) async fn create_client(base_url: &str) -> anyhow::Result }), ) .await?; - - Ok(client) + Ok(()) } /// Creates a Streamable HTTP RMCP client that sends traffic through the remote From 1d9c9c9f33735223cc564ec942001c9141a11eb1 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Thu, 4 Jun 2026 20:23:37 -0700 Subject: [PATCH 07/10] [codex] Keep Bazel startup options stable across commands (#26256) ## Why `just bazel-clippy` ran target discovery with `--noexperimental_remote_repo_contents_cache`, then ran the build with the workspace default `--experimental_remote_repo_contents_cache`. Bazel therefore killed and restarted its server on each transition, slowing repeated commands and discarding the in-memory analysis cache. An audit found the same class of startup-option variation in several CI command sequences. ## What changed - Keep local lint target-discovery queries on the workspace-default Bazel server, while making CI target discovery explicitly use the CI startup options. - Normalize GitHub Actions launches through the BuildBuddy wrapper to share `BAZEL_OUTPUT_USER_ROOT` and `--noexperimental_remote_repo_contents_cache`. - Route the CI lockfile check and Windows test-shard query through the same startup configuration. - Document the startup-option invariant and add wrapper regression coverage. ## Validation - Confirmed consecutive local clippy target-discovery runs retained the same Bazel server PID. --- .github/scripts/run-bazel-query-ci.sh | 20 +++------ .github/scripts/run_bazel_with_buildbuddy.py | 43 ++++++++++++++++++- .../scripts/test_run_bazel_with_buildbuddy.py | 34 +++++++++++++++ .github/workflows/bazel.yml | 2 +- codex-rs/docs/bazel.md | 5 ++- scripts/check-module-bazel-lock.sh | 3 +- 6 files changed, 89 insertions(+), 18 deletions(-) diff --git a/.github/scripts/run-bazel-query-ci.sh b/.github/scripts/run-bazel-query-ci.sh index f5d4f56f49..39fa04c47d 100755 --- a/.github/scripts/run-bazel-query-ci.sh +++ b/.github/scripts/run-bazel-query-ci.sh @@ -4,7 +4,8 @@ set -euo pipefail # Run target-discovery queries with the same startup settings as the main # build/test invocation so they can reuse the same Bazel server. Queries only -# enumerate labels, so they intentionally do not select CI or remote configs. +# enumerate labels, so they intentionally do not select a CI build/test config +# or remote execution. if [[ $# -lt 2 || "${@: -2:1}" != "--" ]]; then echo "Usage: $0 [...] -- " >&2 @@ -14,21 +15,16 @@ fi query_args=("${@:1:$#-2}") query_expression="${@: -1}" -bazel_startup_args=() -if [[ -n "${BAZEL_OUTPUT_USER_ROOT:-}" ]]; then - bazel_startup_args+=("--output_user_root=${BAZEL_OUTPUT_USER_ROOT}") -fi - run_bazel() { if [[ "${RUNNER_OS:-}" == "Windows" ]]; then - MSYS2_ARG_CONV_EXCL='*' bazel "$@" + MSYS2_ARG_CONV_EXCL='*' "$(dirname "${BASH_SOURCE[0]}")/run_bazel_with_buildbuddy.py" "$@" return fi - bazel "$@" + "$(dirname "${BASH_SOURCE[0]}")/run_bazel_with_buildbuddy.py" "$@" } -bazel_query_args=(--noexperimental_remote_repo_contents_cache query) +bazel_query_args=(query) if [[ -n "${BAZEL_REPO_CONTENTS_CACHE:-}" ]]; then bazel_query_args+=("--repo_contents_cache=${BAZEL_REPO_CONTENTS_CACHE}") @@ -43,8 +39,4 @@ if (( ${#query_args[@]} > 0 )); then fi bazel_query_args+=("$query_expression") -if (( ${#bazel_startup_args[@]} > 0 )); then - run_bazel "${bazel_startup_args[@]}" "${bazel_query_args[@]}" -else - run_bazel "${bazel_query_args[@]}" -fi +run_bazel "${bazel_query_args[@]}" diff --git a/.github/scripts/run_bazel_with_buildbuddy.py b/.github/scripts/run_bazel_with_buildbuddy.py index f9f329d686..4503b4fda3 100755 --- a/.github/scripts/run_bazel_with_buildbuddy.py +++ b/.github/scripts/run_bazel_with_buildbuddy.py @@ -22,6 +22,47 @@ REMOTE_EXECUTION_CONFIGS = { "--config=ci-v8", "--config=ci-windows-cross", } +# Honor either explicit setting so the wrapper never overrides the caller's +# choice when it supplies the CI default below. +REMOTE_REPO_CONTENTS_CACHE_STARTUP_OPTIONS = { + "--experimental_remote_repo_contents_cache", + "--noexperimental_remote_repo_contents_cache", +} + + +def startup_args(args: Sequence[str], env: Mapping[str, str]) -> list[str]: + """Return shared startup options that are missing from a Bazel invocation. + + Bazel startup options must precede the command, and changing them restarts + the server and discards its analysis cache. GitHub Actions invokes Bazel + through several helpers, so normalize their startup options here while + preserving any explicit choice made by the caller. + """ + command_idx = next( + (idx for idx, arg in enumerate(args) if not arg.startswith("-")), + len(args), + ) + configured_startup_args = args[:command_idx] + injected_args = [] + + output_user_root = env.get("BAZEL_OUTPUT_USER_ROOT") + if output_user_root and not any( + arg.startswith("--output_user_root=") for arg in configured_startup_args + ): + injected_args.append(f"--output_user_root={output_user_root}") + + if env.get("GITHUB_ACTIONS") == "true" and not any( + arg in REMOTE_REPO_CONTENTS_CACHE_STARTUP_OPTIONS + for arg in configured_startup_args + ): + # Work around Bazel 9 overlay materialization failures seen in CI. This + # disables only the startup-level repo contents cache; keyed runs still + # use BuildBuddy. + injected_args.append("--noexperimental_remote_repo_contents_cache") + + return injected_args + + # Only authenticated workflow runs executing trusted upstream code may use the # OpenAI BuildBuddy host. A pull request event without proof that its head is # in the upstream repository fails closed to the generic host. @@ -114,7 +155,7 @@ def bazel_args_with_remote_config( def bazel_command(*args: str, env: Mapping[str, str] | None = None) -> list[str]: env = os.environ if env is None else env bazel = env.get("CODEX_BAZEL_BIN", "bazel") - return [bazel, *bazel_args_with_remote_config(args, env)] + return [bazel, *startup_args(args, env), *bazel_args_with_remote_config(args, env)] def main() -> None: diff --git a/.github/scripts/test_run_bazel_with_buildbuddy.py b/.github/scripts/test_run_bazel_with_buildbuddy.py index 0f594b7947..f06e34b895 100644 --- a/.github/scripts/test_run_bazel_with_buildbuddy.py +++ b/.github/scripts/test_run_bazel_with_buildbuddy.py @@ -182,6 +182,38 @@ class RunBazelWithBuildBuddyTest(unittest.TestCase): ["fake-bazel", "info", "execution_root"], ) + def test_bazel_command_normalizes_github_actions_startup_options(self) -> None: + env = { + "BAZEL_OUTPUT_USER_ROOT": "/tmp/bazel-output", + "GITHUB_ACTIONS": "true", + } + + self.assertEqual( + run_bazel_with_buildbuddy.bazel_command("build", "//codex-rs/...", env=env), + [ + "bazel", + "--output_user_root=/tmp/bazel-output", + "--noexperimental_remote_repo_contents_cache", + "build", + "//codex-rs/...", + ], + ) + self.assertEqual( + run_bazel_with_buildbuddy.bazel_command( + "--experimental_remote_repo_contents_cache", + "build", + "//codex-rs/...", + env=env, + ), + [ + "bazel", + "--output_user_root=/tmp/bazel-output", + "--experimental_remote_repo_contents_cache", + "build", + "//codex-rs/...", + ], + ) + def test_main_preserves_spaced_argument_and_child_exit_status(self) -> None: spaced_arg = ( r"--test_env=PATH=C:\Program Files\PowerShell\7;C:\Program Files\Git\bin" @@ -191,7 +223,9 @@ class RunBazelWithBuildBuddyTest(unittest.TestCase): ) env = os.environ.copy() env["CODEX_BAZEL_BIN"] = sys.executable + env.pop("BAZEL_OUTPUT_USER_ROOT", None) env.pop("BUILDBUDDY_API_KEY", None) + env.pop("GITHUB_ACTIONS", None) result = subprocess.run( [ diff --git a/.github/workflows/bazel.yml b/.github/workflows/bazel.yml index bddbb5adc9..cc1ef09245 100644 --- a/.github/workflows/bazel.yml +++ b/.github/workflows/bazel.yml @@ -186,7 +186,7 @@ jobs: bazel_test_query='tests(//...) except tests(//third_party/v8:all) except attr(tags, "manual", tests(//...))' mapfile -t bazel_targets < <( - MSYS2_ARG_CONV_EXCL='*' bazel query --output=label "${bazel_test_query}" \ + ./.github/scripts/run-bazel-query-ci.sh --output=label -- "${bazel_test_query}" \ | LC_ALL=C sort ) diff --git a/codex-rs/docs/bazel.md b/codex-rs/docs/bazel.md index 085c15992f..f12eeefd70 100644 --- a/codex-rs/docs/bazel.md +++ b/codex-rs/docs/bazel.md @@ -81,7 +81,10 @@ GitHub Actions routes Bazel build and output-resolution commands through `.github/scripts/run-bazel-ci.sh` and `.github/scripts/rusty_v8_bazel.py` delegate remote configuration selection to that wrapper. The wrapper reads the GitHub Actions repository and event payload rather than relying on workflow -files to duplicate tenant-selection logic. +files to duplicate tenant-selection logic. It also normalizes GitHub Actions +startup options so all Bazel launches in a job reuse the same server and +in-memory analysis cache. Target-discovery and lockfile helpers delegate to the +same wrapper so their callers do not need to select CI-specific startup options. Loading-phase target-discovery `bazel query` commands run locally because they only enumerate labels and do not need remote caches or execution. diff --git a/scripts/check-module-bazel-lock.sh b/scripts/check-module-bazel-lock.sh index 1a14889643..49ddaaef2b 100755 --- a/scripts/check-module-bazel-lock.sh +++ b/scripts/check-module-bazel-lock.sh @@ -1,7 +1,8 @@ #!/usr/bin/env bash set -euo pipefail -if ! bazel mod deps --lockfile_mode=error; then +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +if ! "${repo_root}/.github/scripts/run_bazel_with_buildbuddy.py" mod deps --lockfile_mode=error; then echo "MODULE.bazel.lock is out of date." echo "Run 'just bazel-lock-update' and commit the updated lockfile." exit 1 From a2f5874b7a2db79b46c60a73858ba4c4b4807c05 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 4 Jun 2026 21:48:45 -0700 Subject: [PATCH 08/10] core: derive exec policy filesystem policy from profile (#26499) ## Why `PermissionProfile` already owns the runtime filesystem sandbox policy through `file_system_sandbox_policy()`. Keeping a separate `FileSystemSandboxPolicy` on exec-policy fallback contexts made it possible for callers and tests to construct split states that the production permission model should not rely on. ## What changed - Removed `file_system_sandbox_policy` from `UnmatchedCommandContext`, `ExecApprovalRequest`, and the intercepted Unix exec-policy context. - Derived filesystem sandbox policy inside unmatched-command decision logic from `PermissionProfile::file_system_sandbox_policy()`. - Simplified shell/unified-exec callers and tests that were only plumbing the duplicate policy through. ## Testing Local tests not run per request; relying on remote CI. --- codex-rs/core/src/exec_policy.rs | 17 +--- codex-rs/core/src/exec_policy_tests.rs | 79 +------------------ .../core/src/exec_policy_windows_tests.rs | 4 - codex-rs/core/src/session/tests.rs | 2 - codex-rs/core/src/tools/handlers/shell.rs | 2 - .../tools/runtimes/shell/unix_escalation.rs | 4 - .../runtimes/shell/unix_escalation_tests.rs | 7 -- .../core/src/unified_exec/process_manager.rs | 2 - 8 files changed, 7 insertions(+), 110 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index a44ffbe3ab..cc2f18c4fb 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -22,7 +22,6 @@ use codex_execpolicy::blocking_append_network_rule; use codex_protocol::approvals::ExecPolicyAmendment; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxKind; -use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_shell_command::is_dangerous_command::command_might_be_dangerous; use codex_shell_command::is_safe_command::is_known_safe_command; @@ -121,7 +120,6 @@ pub(crate) enum ExecPolicyCommandOrigin { pub(crate) struct UnmatchedCommandContext<'a> { pub(crate) approval_policy: AskForApproval, pub(crate) permission_profile: &'a PermissionProfile, - pub(crate) file_system_sandbox_policy: &'a FileSystemSandboxPolicy, pub(crate) sandbox_cwd: &'a Path, pub(crate) sandbox_permissions: SandboxPermissions, pub(crate) used_complex_parsing: bool, @@ -242,7 +240,6 @@ pub(crate) struct ExecApprovalRequest<'a> { pub(crate) command: &'a [String], pub(crate) approval_policy: AskForApproval, pub(crate) permission_profile: PermissionProfile, - pub(crate) file_system_sandbox_policy: &'a FileSystemSandboxPolicy, pub(crate) sandbox_cwd: &'a Path, pub(crate) sandbox_permissions: SandboxPermissions, pub(crate) prefix_rule: Option>, @@ -277,7 +274,6 @@ impl ExecPolicyManager { command, approval_policy, permission_profile, - file_system_sandbox_policy, sandbox_cwd, sandbox_permissions, prefix_rule, @@ -298,7 +294,6 @@ impl ExecPolicyManager { UnmatchedCommandContext { approval_policy, permission_profile: &permission_profile, - file_system_sandbox_policy, sandbox_cwd, sandbox_permissions, used_complex_parsing, @@ -636,12 +631,12 @@ pub(crate) fn render_decision_for_unmatched_command( let UnmatchedCommandContext { approval_policy, permission_profile, - file_system_sandbox_policy, sandbox_cwd, sandbox_permissions, used_complex_parsing, command_origin, } = context; + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); let is_known_safe = match command_origin { ExecPolicyCommandOrigin::Generic => is_known_safe_command(command), #[cfg(windows)] @@ -652,12 +647,8 @@ pub(crate) fn render_decision_for_unmatched_command( // On Windows, ReadOnly sandbox is not a real sandbox, so special-case it // here. - let environment_lacks_sandbox_protections = cfg!(windows) - && profile_is_managed_read_only( - permission_profile, - file_system_sandbox_policy, - sandbox_cwd, - ); + let environment_lacks_sandbox_protections = + cfg!(windows) && profile_is_managed_read_only(permission_profile, sandbox_cwd); if is_known_safe && !used_complex_parsing @@ -751,9 +742,9 @@ pub(crate) fn render_decision_for_unmatched_command( fn profile_is_managed_read_only( permission_profile: &PermissionProfile, - file_system_sandbox_policy: &FileSystemSandboxPolicy, sandbox_cwd: &Path, ) -> bool { + let file_system_sandbox_policy = permission_profile.file_system_sandbox_policy(); matches!(permission_profile, PermissionProfile::Managed { .. }) && matches!( file_system_sandbox_policy.kind, diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 7b0883db2a..1adf6d471b 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -19,6 +19,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; @@ -104,31 +105,6 @@ async fn write_project_trust_config( .await } -fn read_only_file_system_sandbox_policy() -> FileSystemSandboxPolicy { - FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::Root, - }, - access: FileSystemAccessMode::Read, - }]) -} - -fn workspace_write_file_system_sandbox_policy() -> FileSystemSandboxPolicy { - FileSystemSandboxPolicy::workspace_write( - &[], - /*exclude_tmpdir_env_var*/ false, - /*exclude_slash_tmp*/ false, - ) -} - -fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy { - FileSystemSandboxPolicy::unrestricted() -} - -fn external_file_system_sandbox_policy() -> FileSystemSandboxPolicy { - FileSystemSandboxPolicy::external_sandbox() -} - async fn test_config() -> (TempDir, Config) { let home = TempDir::new().expect("create temp dir"); let config = ConfigBuilder::without_managed_config_for_tests() @@ -665,7 +641,6 @@ async fn evaluates_bash_lc_inner_commands() { ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -761,7 +736,6 @@ async fn evaluates_heredoc_script_against_prefix_rules() { command, approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -785,7 +759,6 @@ async fn omits_auto_amendment_for_heredoc_fallback_prompts() { ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -809,7 +782,6 @@ async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_wont_mat ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: Some(vec![ "python3".to_string(), @@ -837,7 +809,6 @@ async fn drops_requested_amendment_for_heredoc_fallback_prompts_when_it_matches( ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: Some(vec!["python3".to_string()]), }, @@ -862,7 +833,6 @@ async fn heredoc_with_variable_assignment_is_not_reduced_to_allowed_prefix() { ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -893,7 +863,6 @@ EOF"# ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::workspace_write(), - file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -927,7 +896,6 @@ EOF"# ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::workspace_write(), - file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }, @@ -967,7 +935,6 @@ prefix_rule( ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -986,7 +953,6 @@ async fn exec_approval_requirement_prefers_execpolicy_match() { command: vec!["rm".to_string()], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1014,7 +980,6 @@ prefix_rule(pattern=["git"], decision="allow") command: vec![git_path, "status".to_string()], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1048,7 +1013,6 @@ prefix_rule(pattern=["git"], decision="prompt") command: vec![disallowed_git_path.clone(), "status".to_string()], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1075,7 +1039,6 @@ async fn requested_prefix_rule_can_approve_absolute_path_commands() { ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), }, @@ -1098,7 +1061,6 @@ async fn exec_approval_requirement_respects_approval_policy() { command: vec!["rm".to_string()], approval_policy: AskForApproval::Never, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1126,7 +1088,6 @@ fn unmatched_granular_policy_still_prompts_for_restricted_sandbox_escalation() { mcp_elicitations: true, }), permission_profile: &PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, used_complex_parsing: false, @@ -1137,9 +1098,8 @@ fn unmatched_granular_policy_still_prompts_for_restricted_sandbox_escalation() { } #[test] -fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() { +fn unmatched_on_request_uses_permission_profile_file_system_policy_for_escalation_prompts() { let command = vec!["madeup-cmd".to_string()]; - let restricted_file_system_policy = FileSystemSandboxPolicy::restricted(vec![]); assert_eq!( Decision::Prompt, @@ -1147,8 +1107,7 @@ fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() { &command, UnmatchedCommandContext { approval_policy: AskForApproval::OnRequest, - permission_profile: &PermissionProfile::Disabled, - file_system_sandbox_policy: &restricted_file_system_policy, + permission_profile: &PermissionProfile::read_only(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, used_complex_parsing: false, @@ -1169,7 +1128,6 @@ fn known_safe_on_request_still_prompts_for_restricted_sandbox_escalation() { UnmatchedCommandContext { approval_policy: AskForApproval::OnRequest, permission_profile: &PermissionProfile::workspace_write(), - file_system_sandbox_policy: &workspace_write_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, used_complex_parsing: false, @@ -1202,7 +1160,6 @@ fn managed_cwd_write_profile_is_not_read_only() { assert!(!profile_is_managed_read_only( &permission_profile, - &file_system_sandbox_policy, Path::new("/tmp/project") )); } @@ -1233,7 +1190,6 @@ fn managed_unresolvable_write_profile_is_still_read_only() { assert!(profile_is_managed_read_only( &permission_profile, - &file_system_sandbox_policy, Path::new("/tmp/project") )); } @@ -1250,7 +1206,6 @@ async fn exec_approval_requirement_prompts_for_inline_additional_permissions_und ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, prefix_rule: None, }, @@ -1273,7 +1228,6 @@ async fn exec_approval_requirement_prompts_for_known_safe_escalation_under_on_re command: vec!["echo".to_string(), "hello".to_string()], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::workspace_write(), - file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }, @@ -1303,7 +1257,6 @@ async fn exec_approval_requirement_rejects_known_safe_escalation_when_granular_s mcp_elicitations: true, }), permission_profile: PermissionProfile::workspace_write(), - file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }, @@ -1329,7 +1282,6 @@ async fn exec_approval_requirement_rejects_unmatched_sandbox_escalation_when_gra mcp_elicitations: true, }), permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, }, @@ -1365,7 +1317,6 @@ async fn mixed_rule_and_sandbox_prompt_prioritizes_rule_for_rejection_decision() mcp_elicitations: true, }), permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, @@ -1403,7 +1354,6 @@ async fn mixed_rule_and_sandbox_prompt_rejects_when_granular_rules_are_disabled( mcp_elicitations: true, }), permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: None, @@ -1428,7 +1378,6 @@ async fn exec_approval_requirement_falls_back_to_heuristics() { command: &command, approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, @@ -1454,7 +1403,6 @@ async fn empty_bash_lc_script_falls_back_to_original_command() { command: &command, approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, @@ -1484,7 +1432,6 @@ async fn whitespace_bash_lc_script_falls_back_to_original_command() { command: &command, approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, @@ -1514,7 +1461,6 @@ async fn request_rule_uses_prefix_rule() { command: &command, approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), @@ -1547,7 +1493,6 @@ async fn request_rule_falls_back_when_prefix_rule_does_not_approve_all_commands( command: &command, approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::RequireEscalated, prefix_rule: Some(vec!["cargo".to_string(), "install".to_string()]), @@ -1587,7 +1532,6 @@ async fn heuristics_apply_when_other_commands_match_policy() { command: &command, approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: &unrestricted_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, @@ -1663,7 +1607,6 @@ async fn proposed_execpolicy_amendment_is_present_for_single_command_without_pol command: command.clone(), approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1683,7 +1626,6 @@ async fn proposed_execpolicy_amendment_is_omitted_when_policy_prompts() { command: vec!["rm".to_string()], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1707,7 +1649,6 @@ async fn proposed_execpolicy_amendment_is_present_for_multi_command_scripts() { ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1737,7 +1678,6 @@ async fn proposed_execpolicy_amendment_uses_first_no_match_in_multi_command_scri command, approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1761,7 +1701,6 @@ async fn proposed_execpolicy_amendment_is_present_when_heuristics_allow() { command: command.clone(), approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1785,7 +1724,6 @@ async fn proposed_execpolicy_amendment_is_suppressed_when_policy_matches_allow() ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1816,7 +1754,6 @@ prefix_rule(pattern=["cat"], decision="allow") command: command.clone(), approval_policy, permission_profile: PermissionProfile::workspace_write(), - file_system_sandbox_policy: workspace_write_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -1848,7 +1785,6 @@ prefix_rule(pattern=["bash"], decision="allow") ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -2014,7 +1950,6 @@ async fn dangerous_rm_rf_requires_approval_in_danger_full_access() { command: command.clone(), approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -2078,7 +2013,6 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() { command: &sneaky_command, approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: permissions, prefix_rule: None, @@ -2103,7 +2037,6 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() { command: &dangerous_command, approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: permissions, prefix_rule: None, @@ -2124,7 +2057,6 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() { command: &dangerous_command, approval_policy: AskForApproval::Never, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: permissions, prefix_rule: None, @@ -2146,7 +2078,6 @@ async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() { permission_profile: PermissionProfile::External { network: NetworkSandboxPolicy::Restricted, }, - file_system_sandbox_policy: external_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -2171,7 +2102,6 @@ async fn dangerous_command_forbidden_in_external_sandbox_when_policy_matches() { permission_profile: PermissionProfile::External { network: NetworkSandboxPolicy::Restricted, }, - file_system_sandbox_policy: external_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -2188,7 +2118,6 @@ struct ExecApprovalRequirementScenario { command: Vec, approval_policy: AskForApproval, permission_profile: PermissionProfile, - file_system_sandbox_policy: FileSystemSandboxPolicy, sandbox_permissions: SandboxPermissions, prefix_rule: Option>, } @@ -2212,7 +2141,6 @@ async fn exec_approval_requirement_for_command( command, approval_policy, permission_profile, - file_system_sandbox_policy, sandbox_permissions, prefix_rule, } = test; @@ -2224,7 +2152,6 @@ async fn exec_approval_requirement_for_command( command: &command, approval_policy, permission_profile, - file_system_sandbox_policy: &file_system_sandbox_policy, sandbox_cwd: Path::new("/tmp"), sandbox_permissions, prefix_rule, diff --git a/codex-rs/core/src/exec_policy_windows_tests.rs b/codex-rs/core/src/exec_policy_windows_tests.rs index 1d14d93813..3fba240b41 100644 --- a/codex-rs/core/src/exec_policy_windows_tests.rs +++ b/codex-rs/core/src/exec_policy_windows_tests.rs @@ -15,7 +15,6 @@ async fn evaluates_powershell_inner_commands_against_prompt_rules() { ], approval_policy: AskForApproval::Never, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -39,7 +38,6 @@ async fn evaluates_powershell_inner_commands_against_allow_rules() { ], approval_policy: AskForApproval::UnlessTrusted, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: read_only_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, @@ -81,7 +79,6 @@ fn unmatched_safe_powershell_words_are_allowed() { UnmatchedCommandContext { approval_policy: AskForApproval::UnlessTrusted, permission_profile: &PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: Path::new("/tmp"), sandbox_permissions: SandboxPermissions::UseDefault, used_complex_parsing: false, @@ -110,7 +107,6 @@ async fn unmatched_dangerous_powershell_inner_commands_require_approval() { ], approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::Disabled, - file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), sandbox_permissions: SandboxPermissions::UseDefault, prefix_rule: None, }, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index c5e37fca0e..a2023ea378 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -10868,7 +10868,6 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { let turn_context_mut = Arc::get_mut(&mut turn_context).expect("unique thread settings Arc"); turn_context_mut.permission_profile = PermissionProfile::Disabled; - let file_system_sandbox_policy = turn_context.file_system_sandbox_policy(); let command = session.user_shell().derive_exec_args( command_script, turn_context.config.permissions.allow_login_shell, @@ -10880,7 +10879,6 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() { command: &command, approval_policy: turn_context.approval_policy.value(), permission_profile: turn_context.permission_profile(), - file_system_sandbox_policy: &file_system_sandbox_policy, #[allow(deprecated)] sandbox_cwd: turn_context.cwd.as_path(), sandbox_permissions: SandboxPermissions::UseDefault, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 504653cf76..b3c955595a 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -160,7 +160,6 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result Result { approval_policy: AskForApproval, permission_profile: PermissionProfile, - file_system_sandbox_policy: &'a FileSystemSandboxPolicy, sandbox_cwd: &'a Path, sandbox_permissions: SandboxPermissions, enable_shell_wrapper_parsing: bool, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 6e7a1ced54..ecdf015fd5 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -522,7 +522,6 @@ fn evaluate_intercepted_exec_policy_uses_wrapper_command_when_shell_wrapper_pars InterceptedExecPolicyContext { approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: SandboxPermissions::UseDefault, enable_shell_wrapper_parsing: enable_intercepted_exec_policy_shell_wrapper_parsing, @@ -575,7 +574,6 @@ fn evaluate_intercepted_exec_policy_matches_inner_shell_commands_when_enabled() InterceptedExecPolicyContext { approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: SandboxPermissions::UseDefault, enable_shell_wrapper_parsing: enable_intercepted_exec_policy_shell_wrapper_parsing, @@ -619,7 +617,6 @@ host_executable(name = "git", paths = ["{git_path_literal}"]) InterceptedExecPolicyContext { approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: SandboxPermissions::UseDefault, enable_shell_wrapper_parsing: false, @@ -747,7 +744,6 @@ fn intercepted_exec_policy_treats_preapproved_additional_permissions_as_default( let argv = ["printf".to_string(), "hello".to_string()]; let approval_policy = AskForApproval::OnRequest; let permission_profile = PermissionProfile::workspace_write(); - let file_system_sandbox_policy = read_only_file_system_sandbox_policy(); let sandbox_cwd = test_sandbox_cwd(); let preapproved = evaluate_intercepted_exec_policy( @@ -757,7 +753,6 @@ fn intercepted_exec_policy_treats_preapproved_additional_permissions_as_default( InterceptedExecPolicyContext { approval_policy, permission_profile: permission_profile.clone(), - file_system_sandbox_policy: &file_system_sandbox_policy, sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: super::approval_sandbox_permissions( SandboxPermissions::WithAdditionalPermissions, @@ -773,7 +768,6 @@ fn intercepted_exec_policy_treats_preapproved_additional_permissions_as_default( InterceptedExecPolicyContext { approval_policy, permission_profile, - file_system_sandbox_policy: &file_system_sandbox_policy, sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: SandboxPermissions::WithAdditionalPermissions, enable_shell_wrapper_parsing: false, @@ -808,7 +802,6 @@ host_executable(name = "git", paths = ["{allowed_git_literal}"]) InterceptedExecPolicyContext { approval_policy: AskForApproval::OnRequest, permission_profile: PermissionProfile::read_only(), - file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), sandbox_cwd: sandbox_cwd.as_path(), sandbox_permissions: SandboxPermissions::UseDefault, enable_shell_wrapper_parsing: false, diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 66b8b49a80..2c86deafaf 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1011,7 +1011,6 @@ impl UnifiedExecProcessManager { }; let mut orchestrator = ToolOrchestrator::new(); let mut runtime = UnifiedExecRuntime::new(self, request.shell_mode.clone()); - let file_system_sandbox_policy = context.turn.file_system_sandbox_policy(); let exec_approval_requirement = context .session .services @@ -1020,7 +1019,6 @@ impl UnifiedExecProcessManager { command: &request.command, approval_policy: context.turn.approval_policy.value(), permission_profile: context.turn.permission_profile(), - file_system_sandbox_policy: &file_system_sandbox_policy, // The process cwd may be model-controlled. Policy resolution // stays anchored to the selected turn environment cwd instead. sandbox_cwd: request.sandbox_cwd.as_path(), From 64e0829cab9dccb14b8273a2ebea2eee9e06c12c Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Thu, 4 Jun 2026 22:12:23 -0700 Subject: [PATCH 09/10] feat(remote-control): allow pairing while disabled (#26215) ## Why `remoteControl/pairing/start` creates authorization for future remote-control connections, so it should not require the live websocket to already be enabled. Requiring enable first made pairing depend on presence instead of the persisted server enrollment that pairing actually uses. Pairing also needs to recover when that persisted server row is stale. If `/server/pair` returns `404`, making the first pairing attempt fail forces a manual retry even though the client can clear the stale row and create a replacement enrollment immediately. ## What Changed - Allow `remoteControl/pairing/start` to reuse or create the persisted remote-control server enrollment while remote control is disabled. - Keep the selected in-memory enrollment across disable and share it with websocket connect so a later enable uses the same selected server. - Thread the app-server client name through pairing so stdio persistence keeps using the websocket-owned enrollment key. - Recover pairing server-token auth failures through the existing refresh/auth-recovery path. - Recover stale pairing enrollment on `/server/pair` `404` by clearing the stale selected enrollment, re-enrolling once, and retrying pairing once. - Add focused disabled-pairing and stale-pairing recovery coverage. ## Verification - `remote_control_pairing_start_returns_pairing_artifacts_while_disabled` exercises pairing before enable. - `remote_control_handle_reenrolls_after_stale_pairing_enrollment` exercises stale `/server/pair` `404` recovery without a manual retry. Related: N/A --- .../src/transport/remote_control/mod.rs | 353 +++++++++++++++--- .../src/transport/remote_control/tests.rs | 30 +- .../remote_control/tests/clients_tests.rs | 5 +- .../remote_control/tests/pairing_tests.rs | 156 +++++++- .../src/transport/remote_control/websocket.rs | 348 +++++++++-------- codex-rs/app-server/src/message_processor.rs | 2 +- .../remote_control_processor.rs | 3 +- .../remote_control_processor_tests.rs | 5 +- .../tests/suite/v2/remote_control.rs | 44 ++- 9 files changed, 702 insertions(+), 244 deletions(-) diff --git a/codex-rs/app-server-transport/src/transport/remote_control/mod.rs b/codex-rs/app-server-transport/src/transport/remote_control/mod.rs index 38e34e6d43..443dcc897e 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/mod.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/mod.rs @@ -9,7 +9,10 @@ mod websocket; use self::auth::load_remote_control_auth; use self::auth::recover_remote_control_auth; use self::enroll::RemoteControlEnrollment; +use self::enroll::enroll_remote_control_server; +use self::enroll::load_persisted_remote_control_enrollment; use self::enroll::refresh_remote_control_server; +use self::enroll::update_persisted_remote_control_enrollment; use crate::transport::remote_control::websocket::RemoteControlChannels; use crate::transport::remote_control::websocket::RemoteControlStatusPublisher; use crate::transport::remote_control::websocket::RemoteControlWebsocket; @@ -36,9 +39,13 @@ use gethostname::gethostname; use std::error::Error; use std::fmt; use std::io; +use std::ops::Deref; +use std::ops::DerefMut; use std::panic::AssertUnwindSafe; use std::sync::Arc; use std::sync::Mutex as StdMutex; +use tokio::sync::Semaphore; +use tokio::sync::SemaphorePermit; use tokio::sync::mpsc; use tokio::sync::oneshot; use tokio::sync::watch; @@ -65,12 +72,81 @@ pub struct RemoteControlHandle { enabled_tx: Arc>, status_tx: Arc>, state_db_available: bool, + state_db: Option>, remote_control_url: String, current_enrollment: CurrentRemoteControlEnrollment, + pairing_persistence_key: RemoteControlPairingPersistenceKey, + pairing_persistence_key_required: bool, auth_manager: Arc, } -type CurrentRemoteControlEnrollment = Arc>>; +// Pairing and websocket connect share one selected server so they cannot enroll or clear +// different persisted rows while either path is awaiting backend I/O. +type CurrentRemoteControlEnrollment = Arc; +type RemoteControlPairingPersistenceKey = watch::Sender>; + +struct RemoteControlEnrollmentState { + enrollment: StdMutex>, + lock: Semaphore, +} + +impl RemoteControlEnrollmentState { + fn new(enrollment: Option) -> Self { + Self { + enrollment: StdMutex::new(enrollment), + lock: Semaphore::new(1), + } + } + + async fn lock(&self) -> RemoteControlEnrollmentLease<'_> { + let permit = match self.lock.acquire().await { + Ok(permit) => permit, + Err(_) => unreachable!("remote control enrollment lock should stay open"), + }; + RemoteControlEnrollmentLease { + state: self, + enrollment: self.snapshot(), + _permit: permit, + } + } + + fn snapshot(&self) -> Option { + self.enrollment + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone() + } +} + +struct RemoteControlEnrollmentLease<'a> { + state: &'a RemoteControlEnrollmentState, + enrollment: Option, + _permit: SemaphorePermit<'a>, +} + +impl Deref for RemoteControlEnrollmentLease<'_> { + type Target = Option; + + fn deref(&self) -> &Self::Target { + &self.enrollment + } +} + +impl DerefMut for RemoteControlEnrollmentLease<'_> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.enrollment + } +} + +impl Drop for RemoteControlEnrollmentLease<'_> { + fn drop(&mut self) { + *self + .state + .enrollment + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) = self.enrollment.take(); + } +} #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct RemoteControlUnavailable; @@ -126,8 +202,6 @@ impl RemoteControlHandle { *state = false; changed }); - clear_current_enrollment(&self.current_enrollment); - let status = self.status(); info!( enabled_changed, @@ -151,28 +225,30 @@ impl RemoteControlHandle { pub async fn start_pairing( &self, params: RemoteControlPairingStartParams, + app_server_client_name: Option<&str>, ) -> io::Result { - if !*self.enabled_tx.borrow() { - return Err(Self::pairing_disabled_error()); - } let mut auth = load_remote_control_auth(&self.auth_manager) .await .map_err(|_| pairing_unavailable_error())?; - let mut enrollment = { - let current_enrollment = self - .current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - current_enrollment - .as_ref() - .filter(|enrollment| enrollment.account_id == auth.account_id) - .cloned() - } - .ok_or_else(pairing_unavailable_error)?; - let installation_id = self.status().installation_id; + let status = self.status(); + let installation_id = status.installation_id; + let app_server_client_name = self.pairing_persistence_key(app_server_client_name)?; + let app_server_client_name = app_server_client_name.as_deref(); + let mut current_enrollment = self.current_enrollment.lock().await; + let mut enrollment = self + .load_or_enroll_pairing_server( + &mut current_enrollment, + &mut auth, + &installation_id, + &status.server_name, + app_server_client_name, + ) + .await?; if enrollment.should_refresh_server_token() { refresh_pairing_enrollment( - &self.current_enrollment, + &mut current_enrollment, + self.state_db.as_deref(), + app_server_client_name, &self.auth_manager, &mut auth, &installation_id, @@ -185,9 +261,11 @@ impl RemoteControlHandle { }; let pairing_response = match enrollment.start_pairing(pairing_request()).await { Err(err) if err.kind() == io::ErrorKind::PermissionDenied => { - clear_pairing_server_token(&self.current_enrollment, &mut enrollment)?; + clear_pairing_server_token(&mut current_enrollment, &mut enrollment)?; refresh_pairing_enrollment( - &self.current_enrollment, + &mut current_enrollment, + self.state_db.as_deref(), + app_server_client_name, &self.auth_manager, &mut auth, &installation_id, @@ -196,24 +274,46 @@ impl RemoteControlHandle { .await?; enrollment.start_pairing(pairing_request()).await } + Err(err) if err.kind() == io::ErrorKind::NotFound => { + clear_pairing_enrollment( + &mut current_enrollment, + self.state_db.as_deref(), + app_server_client_name, + &enrollment, + ) + .await; + enrollment = self + .load_or_enroll_pairing_server( + &mut current_enrollment, + &mut auth, + &installation_id, + &status.server_name, + app_server_client_name, + ) + .await?; + enrollment.start_pairing(pairing_request()).await + } pairing_response => pairing_response, }; if let Err(err) = &pairing_response { match err.kind() { io::ErrorKind::NotFound => { - clear_current_enrollment_if_matches(&self.current_enrollment, &enrollment); + clear_pairing_enrollment( + &mut current_enrollment, + self.state_db.as_deref(), + app_server_client_name, + &enrollment, + ) + .await; return Err(pairing_unavailable_error()); } io::ErrorKind::PermissionDenied => { - clear_pairing_server_token(&self.current_enrollment, &mut enrollment)?; + clear_pairing_server_token(&mut current_enrollment, &mut enrollment)?; return Err(pairing_unavailable_error()); } _ => {} } } - if !*self.enabled_tx.borrow() { - return Err(Self::pairing_disabled_error()); - } let current_auth = load_remote_control_auth(&self.auth_manager) .await .map_err(|_| pairing_unavailable_error())?; @@ -223,6 +323,74 @@ impl RemoteControlHandle { pairing_response } + async fn load_or_enroll_pairing_server( + &self, + current_enrollment: &mut Option, + auth: &mut auth::RemoteControlConnectionAuth, + installation_id: &str, + server_name: &str, + app_server_client_name: Option<&str>, + ) -> io::Result { + if let Some(enrollment) = current_enrollment + .as_ref() + .filter(|enrollment| enrollment.account_id == auth.account_id) + .cloned() + { + return Ok(enrollment); + } + + let remote_control_target = normalize_remote_control_url(&self.remote_control_url)?; + let state_db = self + .state_db + .as_deref() + .ok_or_else(pairing_unavailable_error)?; + if let Some(mut enrollment) = load_persisted_remote_control_enrollment( + Some(state_db), + &remote_control_target, + &auth.account_id, + app_server_client_name, + ) + .await? + { + enrollment.server_name = server_name.to_string(); + publish_current_enrollment(current_enrollment, &enrollment); + return Ok(enrollment); + } + + let enrollment = enroll_pairing_server( + &self.auth_manager, + auth, + &remote_control_target, + installation_id, + server_name, + ) + .await?; + update_persisted_remote_control_enrollment( + Some(state_db), + &remote_control_target, + &auth.account_id, + app_server_client_name, + Some(&enrollment), + ) + .await?; + publish_current_enrollment(current_enrollment, &enrollment); + Ok(enrollment) + } + + fn pairing_persistence_key( + &self, + app_server_client_name: Option<&str>, + ) -> io::Result> { + if self.pairing_persistence_key_required && self.pairing_persistence_key.borrow().is_none() + { + let app_server_client_name = + app_server_client_name.ok_or_else(pairing_unavailable_error)?; + self.pairing_persistence_key + .send_replace(Some(app_server_client_name.to_string())); + } + Ok(self.pairing_persistence_key.borrow().clone()) + } + pub async fn list_clients( &self, params: RemoteControlClientsListParams, @@ -239,13 +407,6 @@ impl RemoteControlHandle { .await } - fn pairing_disabled_error() -> io::Error { - io::Error::new( - io::ErrorKind::InvalidInput, - "remote control pairing requires remote control to be enabled", - ) - } - fn publish_status( &self, connection_status: RemoteControlConnectionStatus, @@ -277,8 +438,36 @@ impl RemoteControlHandle { } } +async fn enroll_pairing_server( + auth_manager: &Arc, + auth: &mut auth::RemoteControlConnectionAuth, + remote_control_target: &protocol::RemoteControlTarget, + installation_id: &str, + server_name: &str, +) -> io::Result { + match enroll_remote_control_server(remote_control_target, auth, installation_id, server_name) + .await + { + Ok(enrollment) => return Ok(enrollment), + Err(err) if err.kind() == io::ErrorKind::PermissionDenied => { + let mut auth_recovery = auth_manager.unauthorized_recovery(); + let mut auth_change_rx = auth_manager.auth_change_receiver(); + if !recover_remote_control_auth(&mut auth_recovery, &mut auth_change_rx).await { + return Err(err); + } + *auth = load_remote_control_auth(auth_manager) + .await + .map_err(|_| pairing_unavailable_error())?; + } + Err(err) => return Err(err), + } + enroll_remote_control_server(remote_control_target, auth, installation_id, server_name).await +} + async fn refresh_pairing_enrollment( - current_enrollment: &CurrentRemoteControlEnrollment, + current_enrollment: &mut Option, + state_db: Option<&StateRuntime>, + app_server_client_name: Option<&str>, auth_manager: &Arc, auth: &mut auth::RemoteControlConnectionAuth, installation_id: &str, @@ -286,7 +475,14 @@ async fn refresh_pairing_enrollment( ) -> io::Result<()> { if let Err(err) = refresh_remote_control_server(auth, installation_id, enrollment).await { if err.kind() != io::ErrorKind::PermissionDenied { - return handle_pairing_refresh_error(current_enrollment, enrollment, err); + return handle_pairing_refresh_error( + current_enrollment, + state_db, + app_server_client_name, + enrollment, + err, + ) + .await; } let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); @@ -300,7 +496,14 @@ async fn refresh_pairing_enrollment( return Err(pairing_unavailable_error()); } if let Err(err) = refresh_remote_control_server(auth, installation_id, enrollment).await { - return handle_pairing_refresh_error(current_enrollment, enrollment, err); + return handle_pairing_refresh_error( + current_enrollment, + state_db, + app_server_client_name, + enrollment, + err, + ) + .await; } } if replace_current_enrollment(current_enrollment, enrollment) { @@ -310,21 +513,54 @@ async fn refresh_pairing_enrollment( } } -fn handle_pairing_refresh_error( - current_enrollment: &CurrentRemoteControlEnrollment, +async fn handle_pairing_refresh_error( + current_enrollment: &mut Option, + state_db: Option<&StateRuntime>, + app_server_client_name: Option<&str>, enrollment: &RemoteControlEnrollment, err: io::Error, ) -> io::Result<()> { if err.kind() == io::ErrorKind::NotFound { - clear_current_enrollment_if_matches(current_enrollment, enrollment); + clear_pairing_enrollment( + current_enrollment, + state_db, + app_server_client_name, + enrollment, + ) + .await; Err(pairing_unavailable_error()) } else { Err(err) } } +async fn clear_pairing_enrollment( + current_enrollment: &mut Option, + state_db: Option<&StateRuntime>, + app_server_client_name: Option<&str>, + enrollment: &RemoteControlEnrollment, +) { + if !clear_current_enrollment_if_matches(current_enrollment, enrollment) { + return; + } + let Some(state_db) = state_db else { + return; + }; + if let Err(err) = update_persisted_remote_control_enrollment( + Some(state_db), + &enrollment.remote_control_target, + &enrollment.account_id, + app_server_client_name, + /*enrollment*/ None, + ) + .await + { + warn!("failed to clear stale pairing enrollment: {err}"); + } +} + fn clear_pairing_server_token( - current_enrollment: &CurrentRemoteControlEnrollment, + current_enrollment: &mut Option, enrollment: &mut RemoteControlEnrollment, ) -> io::Result<()> { enrollment.clear_server_token(); @@ -359,27 +595,16 @@ fn remote_control_status_with_connection_status( } fn publish_current_enrollment( - current_enrollment: &CurrentRemoteControlEnrollment, + current_enrollment: &mut Option, enrollment: &RemoteControlEnrollment, ) { - *current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) = Some(enrollment.clone()); -} - -fn clear_current_enrollment(current_enrollment: &CurrentRemoteControlEnrollment) { - *current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) = None; + *current_enrollment = Some(enrollment.clone()); } fn replace_current_enrollment( - current_enrollment: &CurrentRemoteControlEnrollment, + current_enrollment: &mut Option, enrollment: &RemoteControlEnrollment, ) -> bool { - let mut current_enrollment = current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); if !current_enrollment .as_ref() .is_some_and(|current| same_remote_control_enrollment(current, enrollment)) @@ -391,17 +616,17 @@ fn replace_current_enrollment( } fn clear_current_enrollment_if_matches( - current_enrollment: &CurrentRemoteControlEnrollment, + current_enrollment: &mut Option, enrollment: &RemoteControlEnrollment, -) { - let mut current_enrollment = current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); +) -> bool { if current_enrollment .as_ref() .is_some_and(|current| same_remote_control_enrollment(current, enrollment)) { *current_enrollment = None; + true + } else { + false } } @@ -438,9 +663,13 @@ pub async fn start_remote_control( }; let (enabled_tx, enabled_rx) = watch::channel(initial_enabled); - let current_enrollment = Arc::new(StdMutex::new(None)); + let current_enrollment = Arc::new(RemoteControlEnrollmentState::new(/*enrollment*/ None)); let websocket_current_enrollment = current_enrollment.clone(); + let pairing_persistence_key_required = app_server_client_name_rx.is_some(); + let (pairing_persistence_key, _pairing_persistence_key_rx) = watch::channel(None); + let websocket_pairing_persistence_key = pairing_persistence_key.clone(); let handle_auth_manager = auth_manager.clone(); + let handle_state_db = state_db.clone(); let server_name = gethostname().to_string_lossy().trim().to_string(); let remote_control_url = config.remote_control_url; let installation_id = config.installation_id; @@ -490,6 +719,7 @@ pub async fn start_remote_control( transport_event_tx, status_publisher, current_enrollment: websocket_current_enrollment, + pairing_persistence_key: websocket_pairing_persistence_key, }, shutdown_token, enabled_rx, @@ -534,8 +764,11 @@ pub async fn start_remote_control( enabled_tx: Arc::new(enabled_tx), status_tx: Arc::new(status_tx), state_db_available, + state_db: handle_state_db, remote_control_url: handle_remote_control_url, current_enrollment, + pairing_persistence_key, + pairing_persistence_key_required, auth_manager: handle_auth_manager, }, )) diff --git a/codex-rs/app-server-transport/src/transport/remote_control/tests.rs b/codex-rs/app-server-transport/src/transport/remote_control/tests.rs index a1a284da9b..3e9a911b2d 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/tests.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/tests.rs @@ -40,7 +40,6 @@ use pretty_assertions::assert_eq; use serde_json::json; use std::collections::BTreeMap; use std::sync::Arc; -use std::sync::Mutex as StdMutex; use tempfile::TempDir; use time::OffsetDateTime; use tokio::io::AsyncBufReadExt; @@ -148,24 +147,29 @@ fn remote_control_handle_with_current_enrollment( }); let remote_control_target = normalize_remote_control_url(remote_control_url) .expect("remote control target should normalize"); - let current_enrollment = Arc::new(StdMutex::new(Some(RemoteControlEnrollment { - remote_control_target, - account_id: "account_id".to_string(), - environment_id: "env_test".to_string(), - server_id: "srv_e_test".to_string(), - server_name: test_server_name(), - remote_control_token: Some(TEST_REMOTE_CONTROL_SERVER_TOKEN.to_string()), - expires_at: Some( - OffsetDateTime::from_unix_timestamp(33_336_362_096) - .expect("future timestamp should parse"), - ), - }))); + let current_enrollment = Arc::new(RemoteControlEnrollmentState::new(Some( + RemoteControlEnrollment { + remote_control_target, + account_id: "account_id".to_string(), + environment_id: "env_test".to_string(), + server_id: "srv_e_test".to_string(), + server_name: test_server_name(), + remote_control_token: Some(TEST_REMOTE_CONTROL_SERVER_TOKEN.to_string()), + expires_at: Some( + OffsetDateTime::from_unix_timestamp(33_336_362_096) + .expect("future timestamp should parse"), + ), + }, + ))); RemoteControlHandle { enabled_tx: Arc::new(enabled_tx), status_tx: Arc::new(status_tx), state_db_available: true, + state_db: None, remote_control_url: remote_control_url.to_string(), current_enrollment, + pairing_persistence_key: watch::channel(None).0, + pairing_persistence_key_required: false, auth_manager, } } diff --git a/codex-rs/app-server-transport/src/transport/remote_control/tests/clients_tests.rs b/codex-rs/app-server-transport/src/transport/remote_control/tests/clients_tests.rs index 22aa290762..060063909c 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/tests/clients_tests.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/tests/clients_tests.rs @@ -24,8 +24,11 @@ fn client_management_handle( enabled_tx: Arc::new(enabled_tx), status_tx: Arc::new(status_tx), state_db_available: false, + state_db: None, remote_control_url, - current_enrollment: Arc::new(StdMutex::new(None)), + current_enrollment: Arc::new(RemoteControlEnrollmentState::new(/*enrollment*/ None)), + pairing_persistence_key: watch::channel(None).0, + pairing_persistence_key_required: false, auth_manager, } } diff --git a/codex-rs/app-server-transport/src/transport/remote_control/tests/pairing_tests.rs b/codex-rs/app-server-transport/src/transport/remote_control/tests/pairing_tests.rs index 176aeb4ed5..7586f0b4d1 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/tests/pairing_tests.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/tests/pairing_tests.rs @@ -131,13 +131,16 @@ async fn remote_control_handle_starts_pairing_before_websocket_connects() { remote_handle .current_enrollment .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) + .await .as_mut() .expect("current enrollment should exist") .expires_at = Some(OffsetDateTime::now_utc() + time::Duration::seconds(29)); let response = remote_handle - .start_pairing(RemoteControlPairingStartParams { manual_code: true }) + .start_pairing( + RemoteControlPairingStartParams { manual_code: true }, + /*app_server_client_name*/ None, + ) .await .expect("pairing should use the current server before websocket connect"); server_task.await.expect("server task should finish"); @@ -219,7 +222,10 @@ async fn remote_control_handle_refreshes_after_pairing_auth_failure() { ); let response = remote_handle - .start_pairing(RemoteControlPairingStartParams::default()) + .start_pairing( + RemoteControlPairingStartParams::default(), + /*app_server_client_name*/ None, + ) .await .expect("pairing should refresh after server token auth failure"); server_task.await.expect("server task should finish"); @@ -332,13 +338,16 @@ async fn remote_control_handle_recovers_auth_before_refreshing_pairing() { remote_handle .current_enrollment .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) + .await .as_mut() .expect("current enrollment should exist") .expires_at = Some(OffsetDateTime::now_utc() + time::Duration::seconds(29)); let response = remote_handle - .start_pairing(RemoteControlPairingStartParams::default()) + .start_pairing( + RemoteControlPairingStartParams::default(), + /*app_server_client_name*/ None, + ) .await .expect("pairing should refresh after auth recovery"); server_task.await.expect("server task should finish"); @@ -414,21 +423,137 @@ async fn start_remote_control_pairing_preserves_expiry_parse_error_context() { } #[tokio::test] -async fn remote_control_handle_disable_clears_current_enrollment() { +async fn remote_control_handle_disable_keeps_current_enrollment() { let remote_handle = remote_control_handle_with_current_enrollment( TEST_REMOTE_CONTROL_URL, remote_control_auth_manager(), ); remote_handle.disable(); - remote_handle.enable().expect("enable should succeed"); + assert!( + remote_handle.current_enrollment.lock().await.is_some(), + "disabled remote control should keep the selected pairing server" + ); +} + +#[tokio::test] +async fn remote_control_handle_reenrolls_after_stale_pairing_enrollment() { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("listener should bind"); + let remote_control_url = remote_control_url_for_listener(&listener); + let codex_home = TempDir::new().expect("temp dir should create"); + let state_db = remote_control_state_runtime(&codex_home).await; + let mut remote_handle = remote_control_handle_with_current_enrollment( + &remote_control_url, + remote_control_auth_manager_with_home(&codex_home), + ); + remote_handle.state_db = Some(state_db.clone()); + remote_handle.disable(); + let stale_enrollment = remote_handle + .current_enrollment + .lock() + .await + .clone() + .expect("current enrollment should exist"); + let remote_control_target = stale_enrollment.remote_control_target.clone(); + let refreshed_enrollment = RemoteControlEnrollment { + remote_control_target: remote_control_target.clone(), + account_id: "account_id".to_string(), + environment_id: "env_refreshed".to_string(), + server_id: "srv_e_refreshed".to_string(), + server_name: test_server_name(), + remote_control_token: None, + expires_at: None, + }; + update_persisted_remote_control_enrollment( + Some(state_db.as_ref()), + &remote_control_target, + "account_id", + /*app_server_client_name*/ None, + Some(&stale_enrollment), + ) + .await + .expect("stale enrollment should save"); + let server_refreshed_enrollment = refreshed_enrollment.clone(); + let server_task = tokio::spawn(async move { + let stale_pairing_request = accept_http_request(&listener).await; + assert_eq!( + stale_pairing_request.request_line, + "POST /backend-api/wham/remote/control/server/pair HTTP/1.1" + ); + assert_eq!( + stale_pairing_request.headers.get("authorization"), + Some(&format!("Bearer {TEST_REMOTE_CONTROL_SERVER_TOKEN}")) + ); + respond_with_status(stale_pairing_request.stream, "404 Not Found", "").await; + + let enroll_request = accept_http_request(&listener).await; + assert_eq!( + enroll_request.request_line, + "POST /backend-api/wham/remote/control/server/enroll HTTP/1.1" + ); + respond_with_json( + enroll_request.stream, + remote_control_server_token_response( + &server_refreshed_enrollment.server_id, + &server_refreshed_enrollment.environment_id, + TEST_REFRESHED_REMOTE_CONTROL_SERVER_TOKEN, + ), + ) + .await; + + let refreshed_pairing_request = accept_http_request(&listener).await; + assert_eq!( + refreshed_pairing_request.request_line, + "POST /backend-api/wham/remote/control/server/pair HTTP/1.1" + ); + assert_eq!( + refreshed_pairing_request.headers.get("authorization"), + Some(&format!( + "Bearer {TEST_REFRESHED_REMOTE_CONTROL_SERVER_TOKEN}" + )) + ); + respond_with_json( + refreshed_pairing_request.stream, + json!({ + "pairing_code": "pairing-code", + "manual_pairing_code": "ABCD-EFGH", + "server_id": server_refreshed_enrollment.server_id, + "environment_id": server_refreshed_enrollment.environment_id, + "expires_at": "3026-05-22T12:34:56Z", + }), + ) + .await; + }); + let response = remote_handle + .start_pairing( + RemoteControlPairingStartParams::default(), + /*app_server_client_name*/ None, + ) + .await + .expect("pairing should re-enroll after stale enrollment"); + server_task.await.expect("server task should finish"); + assert_eq!( - remote_handle - .start_pairing(RemoteControlPairingStartParams::default()) - .await - .expect_err("re-enabled remote control should wait for a current server") - .to_string(), - "remote control pairing is unavailable until enrollment completes" + response, + RemoteControlPairingStartResponse { + pairing_code: "pairing-code".to_string(), + manual_pairing_code: Some("ABCD-EFGH".to_string()), + environment_id: "env_refreshed".to_string(), + expires_at: 33_336_362_096, + } + ); + assert_eq!( + load_persisted_remote_control_enrollment( + Some(state_db.as_ref()), + &remote_control_target, + "account_id", + /*app_server_client_name*/ None, + ) + .await + .expect("refreshed enrollment should load"), + Some(refreshed_enrollment) ); } @@ -458,7 +583,10 @@ async fn remote_control_handle_discards_pairing_response_after_auth_change() { let remote_handle = remote_handle.clone(); async move { remote_handle - .start_pairing(RemoteControlPairingStartParams::default()) + .start_pairing( + RemoteControlPairingStartParams::default(), + /*app_server_client_name*/ None, + ) .await } }); diff --git a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs index 60792df54c..43fce8ed36 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs @@ -1,13 +1,13 @@ use super::CurrentRemoteControlEnrollment; -use super::clear_current_enrollment; +use super::RemoteControlPairingPersistenceKey; use super::protocol::ClientEnvelope; use super::protocol::ClientEvent; use super::protocol::ClientId; use super::protocol::RemoteControlTarget; use super::protocol::ServerEnvelope; use super::protocol::StreamId; -use super::publish_current_enrollment; use super::remote_control_status_with_connection_status; +use super::same_remote_control_enrollment; use super::segment::ClientSegmentObservation; use super::segment::ClientSegmentReassembler; use super::segment::REMOTE_CONTROL_SEGMENT_MAX_BYTES; @@ -55,6 +55,9 @@ use tokio_tungstenite::connect_async; use tokio_tungstenite::tungstenite; use tokio_tungstenite::tungstenite::client::IntoClientRequest; use tokio_util::sync::CancellationToken; + +#[cfg(test)] +use super::RemoteControlEnrollmentState; use tracing::error; use tracing::info; use tracing::warn; @@ -252,10 +255,10 @@ pub(crate) struct RemoteControlWebsocket { status_publisher: RemoteControlStatusPublisher, shutdown_token: CancellationToken, reconnect_attempt: u64, - enrollment: Option, auth_recovery: UnauthorizedRecovery, auth_change_rx: watch::Receiver, current_enrollment: CurrentRemoteControlEnrollment, + pairing_persistence_key: RemoteControlPairingPersistenceKey, client_tracker: Arc>, state: Arc>, server_event_rx: Arc>>, @@ -294,6 +297,7 @@ pub(super) struct RemoteControlChannels { pub(super) transport_event_tx: mpsc::Sender, pub(super) status_publisher: RemoteControlStatusPublisher, pub(super) current_enrollment: CurrentRemoteControlEnrollment, + pub(super) pairing_persistence_key: RemoteControlPairingPersistenceKey, } #[derive(Clone)] @@ -407,10 +411,10 @@ impl RemoteControlWebsocket { status_publisher: channels.status_publisher, shutdown_token, reconnect_attempt: 0, - enrollment: None, auth_recovery, auth_change_rx, current_enrollment: channels.current_enrollment, + pairing_persistence_key: channels.pairing_persistence_key, client_tracker: Arc::new(Mutex::new(client_tracker)), state: Arc::new(Mutex::new(WebsocketState { outbound_buffer, @@ -457,6 +461,8 @@ impl RemoteControlWebsocket { return; } }; + self.pairing_persistence_key + .send_replace(app_server_client_name.clone()); loop { if !self.wait_until_enabled().await { @@ -579,15 +585,15 @@ impl RemoteControlWebsocket { loop { let subscribe_cursor = self.state.lock().await.subscribe_cursor.clone(); - let enrollment = self.enrollment.as_ref(); + let enrollment = self.current_enrollment.snapshot(); info!( websocket_url = %remote_control_target.websocket_url, installation_id = %self.installation_id, server_name = %self.server_name, reconnect_attempt = self.reconnect_attempt.saturating_add(1), has_enrollment = enrollment.is_some(), - server_id = ?enrollment.map(|enrollment| enrollment.server_id.as_str()), - environment_id = ?enrollment.map(|enrollment| enrollment.environment_id.as_str()), + server_id = ?enrollment.as_ref().map(|enrollment| enrollment.server_id.as_str()), + environment_id = ?enrollment.as_ref().map(|enrollment| enrollment.environment_id.as_str()), subscribe_cursor_present = subscribe_cursor.is_some(), app_server_client_name = ?app_server_client_name, "connecting to app-server remote control websocket" @@ -611,15 +617,17 @@ impl RemoteControlWebsocket { } return ConnectOutcome::Disabled; } - connect_result = connect_remote_control_websocket( - &remote_control_target, - self.state_db.as_deref(), - auth_context, - &mut self.enrollment, - connect_options, - &self.status_publisher, - &self.current_enrollment, - ) => connect_result, + connect_result = async { + connect_remote_control_websocket( + &remote_control_target, + self.state_db.as_deref(), + auth_context, + &self.current_enrollment, + connect_options, + &self.status_publisher, + ) + .await + } => connect_result, }; match connect_result { @@ -631,13 +639,13 @@ impl RemoteControlWebsocket { self.auth_recovery = self.auth_manager.unauthorized_recovery(); self.status_publisher .publish_status(RemoteControlConnectionStatus::Connected); - let enrollment = self.enrollment.as_ref(); + let enrollment = self.current_enrollment.snapshot(); info!( websocket_url = %remote_control_target.websocket_url, installation_id = %self.installation_id, server_name = %self.server_name, - server_id = ?enrollment.map(|enrollment| enrollment.server_id.as_str()), - environment_id = ?enrollment.map(|enrollment| enrollment.environment_id.as_str()), + server_id = ?enrollment.as_ref().map(|enrollment| enrollment.server_id.as_str()), + environment_id = ?enrollment.as_ref().map(|enrollment| enrollment.environment_id.as_str()), subscribe_cursor_present = subscribe_cursor.is_some(), response_headers = %format_headers(response.headers()), "connected to app-server remote control websocket" @@ -656,7 +664,7 @@ impl RemoteControlWebsocket { let reconnect_attempt = self.reconnect_attempt.saturating_add(1); let (reconnect_delay, reconnect_backoff_reset) = next_reconnect_delay(&mut self.reconnect_attempt); - let enrollment = self.enrollment.as_ref(); + let enrollment = self.current_enrollment.snapshot(); warn!( websocket_url = %remote_control_target.websocket_url, installation_id = %self.installation_id, @@ -667,8 +675,8 @@ impl RemoteControlWebsocket { reconnect_delay = ?reconnect_delay, reconnect_backoff_reset, has_enrollment = enrollment.is_some(), - server_id = ?enrollment.map(|enrollment| enrollment.server_id.as_str()), - environment_id = ?enrollment.map(|enrollment| enrollment.environment_id.as_str()), + server_id = ?enrollment.as_ref().map(|enrollment| enrollment.server_id.as_str()), + environment_id = ?enrollment.as_ref().map(|enrollment| enrollment.environment_id.as_str()), subscribe_cursor_present = subscribe_cursor.is_some(), "failed to connect to app-server remote control websocket" ); @@ -1189,19 +1197,108 @@ pub(super) async fn connect_remote_control_websocket( remote_control_target: &RemoteControlTarget, state_db: Option<&StateRuntime>, mut auth_context: RemoteControlAuthContext<'_>, - enrollment: &mut Option, + current_enrollment: &CurrentRemoteControlEnrollment, connect_options: RemoteControlConnectOptions<'_>, status_publisher: &RemoteControlStatusPublisher, - current_enrollment: &CurrentRemoteControlEnrollment, ) -> io::Result<( WebSocketStream>, tungstenite::http::Response<()>, )> { ensure_rustls_crypto_provider(); + let (auth, enrollment) = { + let mut current_enrollment = current_enrollment.lock().await; + let auth = prepare_remote_control_enrollment( + remote_control_target, + state_db, + &mut auth_context, + &mut current_enrollment, + connect_options, + status_publisher, + ) + .await?; + let enrollment = current_enrollment.as_ref().cloned().ok_or_else(|| { + io::Error::other("missing remote control enrollment after enrollment step") + })?; + (auth, enrollment) + }; + let request = build_remote_control_websocket_request( + &remote_control_target.websocket_url, + &enrollment, + connect_options.installation_id, + connect_options.subscribe_cursor, + )?; + + let websocket_connect_result = tokio::time::timeout( + REMOTE_CONTROL_WEBSOCKET_CONNECT_TIMEOUT, + connect_async(request), + ) + .await + .map_err(|_| { + io::Error::new( + ErrorKind::TimedOut, + format!( + "timed out connecting to remote control websocket at `{}` after {:?}", + remote_control_target.websocket_url, REMOTE_CONTROL_WEBSOCKET_CONNECT_TIMEOUT + ), + ) + })?; + + match websocket_connect_result { + Ok((websocket_stream, response)) => Ok((websocket_stream, response.map(|_| ()))), + Err(err) => { + match &err { + tungstenite::Error::Http(response) if response.status().as_u16() == 404 => { + info!( + "remote control websocket returned HTTP 404; clearing stale enrollment before re-enrolling: websocket_url={}, account_id={}, server_id={}, environment_id={}", + remote_control_target.websocket_url, + auth.account_id, + enrollment.server_id, + enrollment.environment_id + ); + clear_remote_control_enrollment_if_matches( + state_db, + remote_control_target, + &auth.account_id, + connect_options.app_server_client_name, + current_enrollment, + &enrollment, + status_publisher, + ) + .await; + } + tungstenite::Error::Http(response) + if matches!(response.status().as_u16(), 401 | 403) => + { + clear_remote_control_server_token_if_matches(current_enrollment, &enrollment) + .await?; + return Err(io::Error::other(format!( + "remote control websocket auth failed with HTTP {}; refreshing server token before reconnect", + response.status() + ))); + } + _ => {} + } + Err(io::Error::other( + format_remote_control_websocket_connect_error( + &remote_control_target.websocket_url, + &err, + ), + )) + } + } +} + +async fn prepare_remote_control_enrollment( + remote_control_target: &RemoteControlTarget, + state_db: Option<&StateRuntime>, + auth_context: &mut RemoteControlAuthContext<'_>, + enrollment: &mut Option, + connect_options: RemoteControlConnectOptions<'_>, + status_publisher: &RemoteControlStatusPublisher, +) -> io::Result { let Some(state_db) = state_db else { *enrollment = None; - clear_current_enrollment(current_enrollment); return Err(io::Error::new( ErrorKind::NotFound, "remote control requires sqlite state db", @@ -1214,7 +1311,6 @@ pub(super) async fn connect_remote_control_websocket( if err.kind() == ErrorKind::PermissionDenied { *enrollment = None; status_publisher.publish_environment_id(/*environment_id*/ None); - clear_current_enrollment(current_enrollment); } return Err(err); } @@ -1231,7 +1327,6 @@ pub(super) async fn connect_remote_control_websocket( ); *enrollment = None; status_publisher.publish_environment_id(/*environment_id*/ None); - clear_current_enrollment(current_enrollment); } if let Some(enrollment) = enrollment.as_mut() { enrollment.remote_control_target = remote_control_target.clone(); @@ -1262,7 +1357,7 @@ pub(super) async fn connect_remote_control_websocket( remote_control_target, state_db, &auth, - &mut auth_context, + auth_context, enrollment, connect_options, status_publisher, @@ -1307,14 +1402,13 @@ pub(super) async fn connect_remote_control_websocket( connect_options.app_server_client_name, enrollment, status_publisher, - current_enrollment, ) .await; enroll_remote_control_server_if_missing( remote_control_target, state_db, &auth, - &mut auth_context, + auth_context, enrollment, connect_options, status_publisher, @@ -1337,82 +1431,7 @@ pub(super) async fn connect_remote_control_websocket( } } - let enrollment_ref = enrollment.as_ref().ok_or_else(|| { - io::Error::other("missing remote control enrollment after enrollment step") - })?; - publish_current_enrollment(current_enrollment, enrollment_ref); - let request = build_remote_control_websocket_request( - &remote_control_target.websocket_url, - enrollment_ref, - connect_options.installation_id, - connect_options.subscribe_cursor, - )?; - - let websocket_connect_result = tokio::time::timeout( - REMOTE_CONTROL_WEBSOCKET_CONNECT_TIMEOUT, - connect_async(request), - ) - .await - .map_err(|_| { - io::Error::new( - ErrorKind::TimedOut, - format!( - "timed out connecting to remote control websocket at `{}` after {:?}", - remote_control_target.websocket_url, REMOTE_CONTROL_WEBSOCKET_CONNECT_TIMEOUT - ), - ) - })?; - - match websocket_connect_result { - Ok((websocket_stream, response)) => Ok((websocket_stream, response.map(|_| ()))), - Err(err) => { - match &err { - tungstenite::Error::Http(response) if response.status().as_u16() == 404 => { - info!( - "remote control websocket returned HTTP 404; clearing stale enrollment before re-enrolling: websocket_url={}, account_id={}, server_id={}, environment_id={}", - remote_control_target.websocket_url, - auth.account_id, - enrollment_ref.server_id, - enrollment_ref.environment_id - ); - clear_remote_control_enrollment( - state_db, - remote_control_target, - &auth.account_id, - connect_options.app_server_client_name, - enrollment, - status_publisher, - current_enrollment, - ) - .await; - } - tungstenite::Error::Http(response) - if matches!(response.status().as_u16(), 401 | 403) => - { - enrollment - .as_mut() - .ok_or_else(|| { - io::Error::other( - "missing remote control enrollment after websocket auth failure", - ) - })? - .clear_server_token(); - clear_current_enrollment(current_enrollment); - return Err(io::Error::other(format!( - "remote control websocket auth failed with HTTP {}; refreshing server token before reconnect", - response.status() - ))); - } - _ => {} - } - Err(io::Error::other( - format_remote_control_websocket_connect_error( - &remote_control_target.websocket_url, - &err, - ), - )) - } - } + Ok(auth) } async fn clear_remote_control_enrollment( @@ -1422,7 +1441,6 @@ async fn clear_remote_control_enrollment( app_server_client_name: Option<&str>, enrollment: &mut Option, status_publisher: &RemoteControlStatusPublisher, - current_enrollment: &CurrentRemoteControlEnrollment, ) { if let Err(clear_err) = update_persisted_remote_control_enrollment( Some(state_db), @@ -1437,7 +1455,51 @@ async fn clear_remote_control_enrollment( } *enrollment = None; status_publisher.publish_environment_id(/*environment_id*/ None); - clear_current_enrollment(current_enrollment); +} + +async fn clear_remote_control_enrollment_if_matches( + state_db: Option<&StateRuntime>, + remote_control_target: &RemoteControlTarget, + account_id: &str, + app_server_client_name: Option<&str>, + current_enrollment: &CurrentRemoteControlEnrollment, + enrollment: &RemoteControlEnrollment, + status_publisher: &RemoteControlStatusPublisher, +) { + let Some(state_db) = state_db else { + return; + }; + let mut current_enrollment = current_enrollment.lock().await; + if !current_enrollment + .as_ref() + .is_some_and(|current| same_remote_control_enrollment(current, enrollment)) + { + return; + } + clear_remote_control_enrollment( + state_db, + remote_control_target, + account_id, + app_server_client_name, + &mut current_enrollment, + status_publisher, + ) + .await; +} + +async fn clear_remote_control_server_token_if_matches( + current_enrollment: &CurrentRemoteControlEnrollment, + enrollment: &RemoteControlEnrollment, +) -> io::Result<()> { + let mut current_enrollment = current_enrollment.lock().await; + current_enrollment + .as_mut() + .filter(|current| same_remote_control_enrollment(current, enrollment)) + .ok_or_else(|| { + io::Error::other("missing remote control enrollment after websocket auth failure") + })? + .clear_server_token(); + Ok(()) } async fn enroll_remote_control_server_if_missing( @@ -1585,8 +1647,10 @@ mod tests { } } - fn test_current_enrollment() -> CurrentRemoteControlEnrollment { - Arc::new(std::sync::Mutex::new(None)) + fn test_current_enrollment( + enrollment: Option, + ) -> CurrentRemoteControlEnrollment { + Arc::new(RemoteControlEnrollmentState::new(enrollment)) } #[test] @@ -1739,10 +1803,9 @@ mod tests { let auth_manager = remote_control_auth_manager(); let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = Some(remote_control_enrollment(Some( + let current_enrollment = test_current_enrollment(Some(remote_control_enrollment(Some( TEST_REMOTE_CONTROL_SERVER_TOKEN, - ))); - let current_enrollment = test_current_enrollment(); + )))); let (status_publisher, status_rx) = remote_control_status_channel(); let err = match connect_remote_control_websocket( @@ -1753,7 +1816,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -1761,7 +1824,6 @@ mod tests { app_server_client_name: None, }, &status_publisher, - ¤t_enrollment, ) .await { @@ -1771,12 +1833,7 @@ mod tests { server_task.await.expect("server task should succeed"); assert_eq!(err.to_string(), expected_error); - assert!( - current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .is_some() - ); + assert!(current_enrollment.lock().await.is_some()); assert_eq!( status_rx.borrow().clone(), RemoteControlStatusChangedNotification { @@ -1801,10 +1858,9 @@ mod tests { let auth_manager = remote_control_auth_manager(); let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = Some(remote_control_enrollment(Some( + let current_enrollment = test_current_enrollment(Some(remote_control_enrollment(Some( TEST_REMOTE_CONTROL_SERVER_TOKEN, - ))); - let current_enrollment = test_current_enrollment(); + )))); let (status_publisher, status_rx) = remote_control_status_channel(); let server_task = tokio::spawn(async move { @@ -1824,7 +1880,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -1832,7 +1888,6 @@ mod tests { app_server_client_name: None, }, &status_publisher, - ¤t_enrollment, ) .await .expect_err("unauthorized response should fail the websocket connect"); @@ -1853,13 +1908,7 @@ mod tests { ); let mut expected_enrollment = remote_control_enrollment(/*remote_control_token*/ None); expected_enrollment.remote_control_target = remote_control_target; - assert_eq!(enrollment, Some(expected_enrollment)); - assert!( - current_enrollment - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .is_none() - ); + assert_eq!(*current_enrollment.lock().await, Some(expected_enrollment)); } #[tokio::test] @@ -1896,7 +1945,7 @@ mod tests { .await; let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = None; + let current_enrollment = test_current_enrollment(/*enrollment*/ None); let (status_publisher, status_rx) = remote_control_status_channel(); save_auth( codex_home.path(), @@ -1913,7 +1962,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -1921,7 +1970,6 @@ mod tests { app_server_client_name: None, }, &status_publisher, - &test_current_enrollment(), ) .await .expect_err("unauthorized enrollment should fail the websocket connect"); @@ -1989,9 +2037,9 @@ mod tests { .await; let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = Some(remote_control_enrollment( + let current_enrollment = test_current_enrollment(Some(remote_control_enrollment( /*remote_control_token*/ None, - )); + ))); let (status_publisher, status_rx) = remote_control_status_channel(); save_auth( codex_home.path(), @@ -2008,7 +2056,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -2016,7 +2064,6 @@ mod tests { app_server_client_name: None, }, &status_publisher, - &test_current_enrollment(), ) .await .expect_err("unauthorized refresh should fail the websocket connect"); @@ -2061,9 +2108,9 @@ mod tests { let auth_manager = remote_control_auth_manager(); let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = Some(remote_control_enrollment(Some( + let current_enrollment = test_current_enrollment(Some(remote_control_enrollment(Some( TEST_REMOTE_CONTROL_SERVER_TOKEN, - ))); + )))); let (status_publisher, _status_rx) = remote_control_status_channel(); let err = connect_remote_control_websocket( @@ -2074,7 +2121,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -2082,14 +2129,13 @@ mod tests { app_server_client_name: None, }, &status_publisher, - &test_current_enrollment(), ) .await .expect_err("missing sqlite state db should fail remote control"); assert_eq!(err.kind(), ErrorKind::NotFound); assert_eq!(err.to_string(), "remote control requires sqlite state db"); - assert_eq!(enrollment, None); + assert_eq!(*current_enrollment.lock().await, None); } #[tokio::test] @@ -2107,9 +2153,9 @@ mod tests { .await; let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut auth_change_rx = auth_manager.auth_change_receiver(); - let mut enrollment = Some(remote_control_enrollment(Some( + let current_enrollment = test_current_enrollment(Some(remote_control_enrollment(Some( TEST_REMOTE_CONTROL_SERVER_TOKEN, - ))); + )))); let (status_publisher, mut status_rx) = remote_control_status_channel(); status_publisher.publish_environment_id(Some("env_test".to_string())); status_rx @@ -2125,7 +2171,7 @@ mod tests { auth_recovery: &mut auth_recovery, auth_change_rx: &mut auth_change_rx, }, - &mut enrollment, + ¤t_enrollment, RemoteControlConnectOptions { installation_id: TEST_INSTALLATION_ID, server_name: "test-server", @@ -2133,7 +2179,6 @@ mod tests { app_server_client_name: None, }, &status_publisher, - &test_current_enrollment(), ) .await .expect_err("missing auth should fail remote control"); @@ -2143,7 +2188,7 @@ mod tests { err.to_string(), "remote control requires ChatGPT authentication" ); - assert_eq!(enrollment, None); + assert_eq!(*current_enrollment.lock().await, None); assert_eq!( status_rx.borrow().clone(), RemoteControlStatusChangedNotification { @@ -2185,7 +2230,8 @@ mod tests { RemoteControlChannels { transport_event_tx, status_publisher, - current_enrollment: test_current_enrollment(), + current_enrollment: test_current_enrollment(/*enrollment*/ None), + pairing_persistence_key: watch::channel(None).0, }, shutdown_token, enabled_rx, diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index d89f0b0645..6c578ce8e5 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -918,7 +918,7 @@ impl MessageProcessor { .map(|response| Some(response.into())), ClientRequest::RemoteControlPairingStart { params, .. } => self .remote_control_processor - .pairing_start(params) + .pairing_start(params, app_server_client_name.as_deref()) .await .map(|response| Some(response.into())), ClientRequest::RemoteControlClientsList { params, .. } => self diff --git a/codex-rs/app-server/src/request_processors/remote_control_processor.rs b/codex-rs/app-server/src/request_processors/remote_control_processor.rs index 804339c17a..22223687bc 100644 --- a/codex-rs/app-server/src/request_processors/remote_control_processor.rs +++ b/codex-rs/app-server/src/request_processors/remote_control_processor.rs @@ -52,9 +52,10 @@ impl RemoteControlRequestProcessor { pub(crate) async fn pairing_start( &self, params: RemoteControlPairingStartParams, + app_server_client_name: Option<&str>, ) -> Result { self.handle()? - .start_pairing(params) + .start_pairing(params, app_server_client_name) .await .map_err(map_pairing_start_error) } diff --git a/codex-rs/app-server/src/request_processors/remote_control_processor/remote_control_processor_tests.rs b/codex-rs/app-server/src/request_processors/remote_control_processor/remote_control_processor_tests.rs index 4381e48959..e336491cd3 100644 --- a/codex-rs/app-server/src/request_processors/remote_control_processor/remote_control_processor_tests.rs +++ b/codex-rs/app-server/src/request_processors/remote_control_processor/remote_control_processor_tests.rs @@ -6,7 +6,10 @@ use pretty_assertions::assert_eq; #[tokio::test] async fn pairing_start_returns_internal_error_when_remote_control_is_unavailable() { let err = RemoteControlRequestProcessor::new(/*remote_control_handle*/ None) - .pairing_start(RemoteControlPairingStartParams::default()) + .pairing_start( + RemoteControlPairingStartParams::default(), + /*app_server_client_name*/ None, + ) .await .expect_err("missing remote control should fail pairing"); diff --git a/codex-rs/app-server/tests/suite/v2/remote_control.rs b/codex-rs/app-server/tests/suite/v2/remote_control.rs index b5384d3bad..c48ffda0f6 100644 --- a/codex-rs/app-server/tests/suite/v2/remote_control.rs +++ b/codex-rs/app-server/tests/suite/v2/remote_control.rs @@ -193,6 +193,42 @@ async fn remote_control_pairing_start_returns_pairing_artifacts() -> Result<()> Ok(()) } +#[tokio::test] +async fn remote_control_pairing_start_returns_pairing_artifacts_while_disabled() -> Result<()> { + let codex_home = TempDir::new()?; + let mut backend = PairingRemoteControlBackend::start(codex_home.path()).await?; + let mut mcp = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_remote_control_pairing_start_request(RemoteControlPairingStartParams { + manual_code: true, + }) + .await?; + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!( + timeout(DEFAULT_TIMEOUT, backend.wait_for_enroll_request()).await??, + "POST /backend-api/wham/remote/control/server/enroll HTTP/1.1" + ); + assert_eq!(response.result.get("serverId"), None); + let received: RemoteControlPairingStartResponse = to_response(response)?; + + assert_eq!( + received, + RemoteControlPairingStartResponse { + pairing_code: "pairing-code".to_string(), + manual_pairing_code: Some("ABCD-EFGH".to_string()), + environment_id: "environment-id".to_string(), + expires_at: 33_336_362_096, + } + ); + Ok(()) +} + #[tokio::test] async fn remote_control_client_management_works_while_disabled() -> Result<()> { let codex_home = TempDir::new()?; @@ -376,8 +412,12 @@ impl PairingRemoteControlBackend { ) .await?; - let _websocket_request = read_http_request(&listener).await?; - let pair_http_request = read_http_request(&listener).await?; + let request_after_enroll = read_http_request(&listener).await?; + let pair_http_request = if request_after_enroll.request_line.starts_with("GET ") { + read_http_request(&listener).await? + } else { + request_after_enroll + }; respond_with_json( pair_http_request.reader.into_inner(), serde_json::json!({ From 6a6a5f925e97bee23d5d31938b5f9f9f38becb8a Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Thu, 4 Jun 2026 22:36:25 -0700 Subject: [PATCH 10/10] [codex] Add environment shell info (#26480) ## Why Shell detection needs to be available through the `Environment` abstraction so callers can ask the selected local or remote environment for shell metadata without adding a separate HTTP endpoint or parallel info-source path. This keeps shell metadata shaped like the existing environment-owned filesystem capability and lets remote environments answer through exec-server JSON-RPC. ## What changed - Added `environment/info` to the exec-server protocol/client/server and exposed `Environment::info()`. - Added local and remote environment info providers on `Environment`, following the existing capability-provider pattern used for filesystem access. - Moved the shared shell detection logic into `codex-shell-command` and kept core shell APIs as wrappers around that implementation. - Returned shell metadata as `EnvironmentInfo { shell: ShellInfo }` using the existing shell detection path. - Added a remote environment test that calls `Environment::info()` through an exec-server-backed environment. ## Validation - `git diff --check` - `just test -p codex-shell-command` - `just test -p codex-core -E 'test(/shell::tests::/)'`\n- `just test -p codex-exec-server environment` --- codex-rs/Cargo.lock | 2 + codex-rs/core/src/lib.rs | 1 - codex-rs/core/src/shell.rs | 328 +---------------- codex-rs/core/src/shell_detect.rs | 24 -- codex-rs/core/src/shell_snapshot.rs | 8 +- .../src/tools/handlers/shell/shell_command.rs | 2 +- .../core/src/tools/handlers/unified_exec.rs | 2 +- .../core/src/unified_exec/process_manager.rs | 2 +- codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/client.rs | 12 + codex-rs/exec-server/src/environment.rs | 62 ++++ codex-rs/exec-server/src/lib.rs | 2 + codex-rs/exec-server/src/protocol.rs | 18 + codex-rs/exec-server/src/server/handler.rs | 6 + codex-rs/exec-server/src/server/registry.rs | 5 + codex-rs/exec-server/tests/health.rs | 15 + codex-rs/shell-command/Cargo.toml | 1 + codex-rs/shell-command/src/bash.rs | 2 +- codex-rs/shell-command/src/lib.rs | 2 +- codex-rs/shell-command/src/powershell.rs | 2 +- codex-rs/shell-command/src/shell_detect.rs | 346 +++++++++++++++++- 21 files changed, 488 insertions(+), 355 deletions(-) delete mode 100644 codex-rs/core/src/shell_detect.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 6da7688435..b899222384 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2789,6 +2789,7 @@ dependencies = [ "codex-file-system", "codex-protocol", "codex-sandboxing", + "codex-shell-command", "codex-test-binary-support", "codex-utils-absolute-path", "codex-utils-pty", @@ -3692,6 +3693,7 @@ dependencies = [ "base64 0.22.1", "codex-protocol", "codex-utils-absolute-path", + "libc", "once_cell", "pretty_assertions", "regex", diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index b77f96cdb1..4dc19de7e6 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -83,7 +83,6 @@ mod sandbox_tags; pub mod sandboxing; mod session_prefix; mod session_startup_prewarm; -mod shell_detect; pub mod skills; pub(crate) use skills::SkillInjections; pub(crate) use skills::SkillLoadOutcome; diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index 48c760d667..9843d4cea5 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -1,19 +1,12 @@ -use crate::shell_detect::detect_shell_type; use crate::shell_snapshot::ShellSnapshot; +use codex_shell_command::shell_detect::DetectedShell; use serde::Deserialize; use serde::Serialize; use std::path::PathBuf; use std::sync::Arc; use tokio::sync::watch; -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub enum ShellType { - Zsh, - Bash, - PowerShell, - Sh, - Cmd, -} +pub use codex_shell_command::shell_detect::ShellType; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Shell { @@ -29,13 +22,7 @@ pub struct Shell { impl Shell { pub fn name(&self) -> &'static str { - match self.shell_type { - ShellType::Zsh => "zsh", - ShellType::Bash => "bash", - ShellType::PowerShell => "powershell", - ShellType::Sh => "sh", - ShellType::Cmd => "cmd", - } + self.shell_type.name() } /// Takes a string of shell and returns the full list of command args to @@ -88,319 +75,36 @@ impl PartialEq for Shell { impl Eq for Shell {} -#[cfg(unix)] -fn get_user_shell_path() -> Option { - let uid = unsafe { libc::getuid() }; - use std::ffi::CStr; - use std::mem::MaybeUninit; - use std::ptr; - - let mut passwd = MaybeUninit::::uninit(); - - // We cannot use getpwuid here: it returns pointers into libc-managed - // storage, which is not safe to read concurrently on all targets (the musl - // static build used by the CLI can segfault when parallel callers race on - // that buffer). getpwuid_r keeps the passwd data in caller-owned memory. - let suggested_buffer_len = unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) }; - let buffer_len = usize::try_from(suggested_buffer_len) - .ok() - .filter(|len| *len > 0) - .unwrap_or(1024); - let mut buffer = vec![0; buffer_len]; - - loop { - let mut result = ptr::null_mut(); - let status = unsafe { - libc::getpwuid_r( - uid, - passwd.as_mut_ptr(), - buffer.as_mut_ptr().cast(), - buffer.len(), - &mut result, - ) - }; - - if status == 0 { - if result.is_null() { - return None; - } - - let passwd = unsafe { passwd.assume_init_ref() }; - if passwd.pw_shell.is_null() { - return None; - } - - let shell_path = unsafe { CStr::from_ptr(passwd.pw_shell) } - .to_string_lossy() - .into_owned(); - return Some(PathBuf::from(shell_path)); - } - - if status != libc::ERANGE { - return None; - } - - // Retry with a larger buffer until libc can materialize the passwd entry. - let new_len = buffer.len().checked_mul(2)?; - if new_len > 1024 * 1024 { - return None; - } - buffer.resize(new_len, 0); - } -} - -#[cfg(not(unix))] -fn get_user_shell_path() -> Option { - None -} - -fn file_exists(path: &PathBuf) -> Option { - if std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) { - Some(PathBuf::from(path)) - } else { - None - } -} - -fn get_shell_path( - shell_type: ShellType, - provided_path: Option<&PathBuf>, - binary_name: &str, - fallback_paths: &[&str], -) -> Option { - // If exact provided path exists, use it - if provided_path.and_then(file_exists).is_some() { - return provided_path.cloned(); - } - - // Check if the shell we are trying to load is user's default shell - // if just use it - let default_shell_path = get_user_shell_path(); - if let Some(default_shell_path) = default_shell_path - && detect_shell_type(&default_shell_path) == Some(shell_type) - && file_exists(&default_shell_path).is_some() - { - return Some(default_shell_path); - } - - if let Ok(path) = which::which(binary_name) { - return Some(path); - } - - for path in fallback_paths { - //check exists - if let Some(path) = file_exists(&PathBuf::from(path)) { - return Some(path); +impl From for Shell { + fn from(detected: DetectedShell) -> Self { + Self { + shell_type: detected.shell_type, + shell_path: detected.shell_path, + shell_snapshot: empty_shell_snapshot_receiver(), } } - - None -} - -const ZSH_FALLBACK_PATHS: &[&str] = &["/bin/zsh"]; - -fn get_zsh_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Zsh, path, "zsh", ZSH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Zsh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -const BASH_FALLBACK_PATHS: &[&str] = &["/bin/bash"]; - -fn get_bash_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Bash, path, "bash", BASH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Bash, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -const SH_FALLBACK_PATHS: &[&str] = &["/bin/sh"]; - -fn get_sh_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Sh, path, "sh", SH_FALLBACK_PATHS); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Sh, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -// Note the `pwsh` and `powershell` fallback paths are where the respective -// shells are commonly installed on GitHub Actions Windows runners, but may not -// be present on all Windows machines: -// https://docs.github.com/en/actions/tutorials/build-and-test-code/powershell - -#[cfg(windows)] -const PWSH_FALLBACK_PATHS: &[&str] = &[r#"C:\Program Files\PowerShell\7\pwsh.exe"#]; -#[cfg(not(windows))] -const PWSH_FALLBACK_PATHS: &[&str] = &["/usr/local/bin/pwsh"]; - -#[cfg(windows)] -const POWERSHELL_FALLBACK_PATHS: &[&str] = - &[r#"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"#]; -#[cfg(not(windows))] -const POWERSHELL_FALLBACK_PATHS: &[&str] = &[]; - -fn get_powershell_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::PowerShell, path, "pwsh", PWSH_FALLBACK_PATHS) - .or_else(|| { - get_shell_path( - ShellType::PowerShell, - path, - "powershell", - POWERSHELL_FALLBACK_PATHS, - ) - }); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::PowerShell, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) -} - -fn get_cmd_shell(path: Option<&PathBuf>) -> Option { - let shell_path = get_shell_path(ShellType::Cmd, path, "cmd", &[]); - - shell_path.map(|shell_path| Shell { - shell_type: ShellType::Cmd, - shell_path, - shell_snapshot: empty_shell_snapshot_receiver(), - }) } +#[cfg(all(test, unix))] fn ultimate_fallback_shell() -> Shell { - if cfg!(windows) { - Shell { - shell_type: ShellType::Cmd, - shell_path: PathBuf::from("cmd.exe"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } else { - Shell { - shell_type: ShellType::Sh, - shell_path: PathBuf::from("/bin/sh"), - shell_snapshot: empty_shell_snapshot_receiver(), - } - } + codex_shell_command::shell_detect::ultimate_fallback_shell().into() } pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> Shell { - detect_shell_type(shell_path) - .and_then(|shell_type| get_shell(shell_type, Some(shell_path))) - .unwrap_or(ultimate_fallback_shell()) + codex_shell_command::shell_detect::get_shell_by_model_provided_path(shell_path).into() } pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { - match shell_type { - ShellType::Zsh => get_zsh_shell(path), - ShellType::Bash => get_bash_shell(path), - ShellType::PowerShell => get_powershell_shell(path), - ShellType::Sh => get_sh_shell(path), - ShellType::Cmd => get_cmd_shell(path), - } + codex_shell_command::shell_detect::get_shell(shell_type, path).map(Into::into) } pub fn default_user_shell() -> Shell { - default_user_shell_from_path(get_user_shell_path()) + codex_shell_command::shell_detect::default_user_shell().into() } +#[cfg(all(test, target_os = "macos"))] fn default_user_shell_from_path(user_shell_path: Option) -> Shell { - if cfg!(windows) { - get_shell(ShellType::PowerShell, /*path*/ None).unwrap_or(ultimate_fallback_shell()) - } else { - let user_default_shell = user_shell_path - .and_then(|shell| detect_shell_type(&shell)) - .and_then(|shell_type| get_shell(shell_type, /*path*/ None)); - - let shell_with_fallback = if cfg!(target_os = "macos") { - user_default_shell - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - } else { - user_default_shell - .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) - .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) - }; - - shell_with_fallback.unwrap_or(ultimate_fallback_shell()) - } -} - -#[cfg(test)] -mod detect_shell_type_tests { - use super::*; - - #[test] - fn test_detect_shell_type() { - assert_eq!( - detect_shell_type(&PathBuf::from("zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell")), - Some(ShellType::PowerShell) - ); - assert_eq!(detect_shell_type(&PathBuf::from("fish")), None); - assert_eq!(detect_shell_type(&PathBuf::from("other")), None); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/zsh")), - Some(ShellType::Zsh) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/bash")), - Some(ShellType::Bash) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("powershell.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from(if cfg!(windows) { - "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" - } else { - "/usr/local/bin/pwsh" - })), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("pwsh.exe")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/usr/local/bin/pwsh")), - Some(ShellType::PowerShell) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("/bin/sh")), - Some(ShellType::Sh) - ); - assert_eq!(detect_shell_type(&PathBuf::from("sh")), Some(ShellType::Sh)); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd")), - Some(ShellType::Cmd) - ); - assert_eq!( - detect_shell_type(&PathBuf::from("cmd.exe")), - Some(ShellType::Cmd) - ); - } + codex_shell_command::shell_detect::default_user_shell_from_path(user_shell_path).into() } #[cfg(test)] diff --git a/codex-rs/core/src/shell_detect.rs b/codex-rs/core/src/shell_detect.rs deleted file mode 100644 index 3595ab3469..0000000000 --- a/codex-rs/core/src/shell_detect.rs +++ /dev/null @@ -1,24 +0,0 @@ -use crate::shell::ShellType; -use std::path::Path; -use std::path::PathBuf; - -pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option { - match shell_path.as_os_str().to_str() { - Some("zsh") => Some(ShellType::Zsh), - Some("sh") => Some(ShellType::Sh), - Some("cmd") => Some(ShellType::Cmd), - Some("bash") => Some(ShellType::Bash), - Some("pwsh") => Some(ShellType::PowerShell), - Some("powershell") => Some(ShellType::PowerShell), - _ => { - let shell_name = shell_path.file_stem(); - if let Some(shell_name) = shell_name { - let shell_name_path = Path::new(shell_name); - if shell_name_path != Path::new(shell_path) { - return detect_shell_type(&shell_name_path.to_path_buf()); - } - } - None - } - } -} diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index b328a977d7..1cbd538667 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -151,9 +151,7 @@ impl ShellSnapshot { }); // Make the new snapshot. - if let Err(err) = - write_shell_snapshot(shell.shell_type.clone(), &temp_path, session_cwd).await - { + if let Err(err) = write_shell_snapshot(shell.shell_type, &temp_path, session_cwd).await { tracing::warn!( "Failed to create shell snapshot for {}: {err:?}", shell.name() @@ -203,7 +201,7 @@ async fn write_shell_snapshot( if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { bail!("Shell snapshot not supported yet for {shell_type:?}"); } - let shell = get_shell(shell_type.clone(), /*path*/ None) + let shell = get_shell(shell_type, /*path*/ None) .with_context(|| format!("No available shell for {shell_type:?}"))?; let raw_snapshot = capture_snapshot(&shell, cwd).await?; @@ -225,7 +223,7 @@ async fn write_shell_snapshot( } async fn capture_snapshot(shell: &Shell, cwd: &AbsolutePathBuf) -> Result { - let shell_type = shell.shell_type.clone(); + let shell_type = shell.shell_type; match shell_type { ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script(), cwd).await, ShellType::Bash => run_shell_script(shell, &bash_snapshot_script(), cwd).await, diff --git a/codex-rs/core/src/tools/handlers/shell/shell_command.rs b/codex-rs/core/src/tools/handlers/shell/shell_command.rs index b4b60997d6..7c492b3646 100644 --- a/codex-rs/core/src/tools/handlers/shell/shell_command.rs +++ b/codex-rs/core/src/tools/handlers/shell/shell_command.rs @@ -182,7 +182,7 @@ impl ToolExecutor for ShellCommandHandler { session.thread_id, turn.config.permissions.allow_login_shell, )?; - let shell_type = Some(session.user_shell().shell_type.clone()); + let shell_type = Some(session.user_shell().shell_type); run_exec_like(RunExecLikeArgs { tool_name, exec_params, diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 6fa2e40685..ca6b2a4b3e 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -122,7 +122,7 @@ pub(crate) fn get_command( let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); Ok(ResolvedCommand { command: shell.derive_exec_args(&args.cmd, use_login_shell), - shell_type: shell.shell_type.clone(), + shell_type: shell.shell_type, }) } UnifiedExecShellMode::ZshFork(zsh_fork_config) => { diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 2c86deafaf..a4493cf114 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1032,7 +1032,7 @@ impl UnifiedExecProcessManager { .await; let req = UnifiedExecToolRequest { command: request.command.clone(), - shell_type: request.shell_type.clone(), + shell_type: request.shell_type, hook_command: request.hook_command.clone(), process_id: request.process_id, cwd, diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index d842094a16..eadab83fe7 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -22,6 +22,7 @@ codex-client = { workspace = true } codex-file-system = { workspace = true } codex-protocol = { workspace = true } codex-sandboxing = { workspace = true } +codex-shell-command = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-rustls-provider = { workspace = true } diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index b6c56e9f5e..8cf18e7d1f 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -29,6 +29,7 @@ use crate::connection::JsonRpcConnection; use crate::process::ExecProcessEvent; use crate::process::ExecProcessEventLog; use crate::process::ExecProcessEventReceiver; +use crate::protocol::ENVIRONMENT_INFO_METHOD; use crate::protocol::EXEC_CLOSED_METHOD; use crate::protocol::EXEC_EXITED_METHOD; use crate::protocol::EXEC_METHOD; @@ -36,6 +37,7 @@ use crate::protocol::EXEC_OUTPUT_DELTA_METHOD; use crate::protocol::EXEC_READ_METHOD; use crate::protocol::EXEC_TERMINATE_METHOD; use crate::protocol::EXEC_WRITE_METHOD; +use crate::protocol::EnvironmentInfo; use crate::protocol::ExecClosedNotification; use crate::protocol::ExecExitedNotification; use crate::protocol::ExecOutputDeltaNotification; @@ -279,6 +281,12 @@ impl HttpClient for LazyRemoteExecServerClient { } } +impl LazyRemoteExecServerClient { + pub(crate) async fn environment_info(&self) -> Result { + self.get().await?.environment_info().await + } +} + #[derive(Debug, thiserror::Error)] pub enum ExecServerError { #[error("failed to spawn exec-server: {0}")] @@ -363,6 +371,10 @@ impl ExecServerClient { self.call(EXEC_METHOD, ¶ms).await } + pub async fn environment_info(&self) -> Result { + self.call(ENVIRONMENT_INFO_METHOD, &()).await + } + pub async fn read(&self, params: ReadParams) -> Result { self.call(EXEC_READ_METHOD, ¶ms).await } diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 55dc031273..1dfd49d3fd 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -2,6 +2,9 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::RwLock; +use futures::FutureExt; +use futures::future::BoxFuture; + use crate::ExecServerError; use crate::ExecServerRuntimePaths; use crate::ExecutorFileSystem; @@ -18,8 +21,11 @@ use crate::environment_toml::environment_provider_from_codex_home; use crate::local_file_system::LocalFileSystem; use crate::local_process::LocalProcess; use crate::process::ExecBackend; +use crate::protocol::EnvironmentInfo; +use crate::protocol::ShellInfo; use crate::remote_file_system::RemoteFileSystem; use crate::remote_process::RemoteProcess; +use codex_shell_command::shell_detect::DetectedShell; pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; @@ -286,18 +292,49 @@ impl EnvironmentManager { pub struct Environment { exec_server_url: Option, remote_transport: Option, + info_provider: Arc, exec_backend: Arc, filesystem: Arc, http_client: Arc, local_runtime_paths: Option, } +/// Provides environment metadata from either a local environment or a remote exec-server. +trait EnvironmentInfoProvider: Send + Sync { + fn info(&self) -> BoxFuture<'_, Result>; +} + +struct LocalEnvironmentInfoProvider; + +impl EnvironmentInfoProvider for LocalEnvironmentInfoProvider { + fn info(&self) -> BoxFuture<'_, Result> { + std::future::ready(Ok(EnvironmentInfo::local())).boxed() + } +} + +struct RemoteEnvironmentInfoProvider { + client: LazyRemoteExecServerClient, +} + +impl RemoteEnvironmentInfoProvider { + fn new(client: LazyRemoteExecServerClient) -> Self { + Self { client } + } +} + +impl EnvironmentInfoProvider for RemoteEnvironmentInfoProvider { + fn info(&self) -> BoxFuture<'_, Result> { + async move { self.client.environment_info().await }.boxed() + } +} + impl Environment { /// Builds a test-only local environment without configured sandbox helper paths. pub fn default_for_tests() -> Self { Self { exec_server_url: None, remote_transport: None, + info_provider: Arc::new(LocalEnvironmentInfoProvider), exec_backend: Arc::new(LocalProcess::default()), filesystem: Arc::new(LocalFileSystem::unsandboxed()), http_client: Arc::new(ReqwestHttpClient), @@ -354,6 +391,7 @@ impl Environment { Self { exec_server_url: None, remote_transport: None, + info_provider: Arc::new(LocalEnvironmentInfoProvider), exec_backend: Arc::new(LocalProcess::default()), filesystem: Arc::new(LocalFileSystem::with_runtime_paths( local_runtime_paths.clone(), @@ -392,6 +430,7 @@ impl Environment { Self { exec_server_url, remote_transport: Some(remote_transport), + info_provider: Arc::new(RemoteEnvironmentInfoProvider::new(client.clone())), exec_backend, filesystem, http_client: Arc::new(client), @@ -412,6 +451,11 @@ impl Environment { self.local_runtime_paths.as_ref() } + /// Returns environment information from the selected execution/filesystem environment. + pub async fn info(&self) -> Result { + self.info_provider.info().await + } + pub fn get_exec_backend(&self) -> Arc { Arc::clone(&self.exec_backend) } @@ -425,6 +469,23 @@ impl Environment { } } +impl EnvironmentInfo { + pub(crate) fn local() -> Self { + Self { + shell: codex_shell_command::shell_detect::default_user_shell().into(), + } + } +} + +impl From for ShellInfo { + fn from(shell: DetectedShell) -> Self { + Self { + name: shell.name().to_string(), + path: shell.shell_path.to_string_lossy().into_owned(), + } + } +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -458,6 +519,7 @@ mod tests { assert_eq!(environment.exec_server_url(), None); assert!(!environment.is_remote()); + assert!(environment.info().await.is_ok()); } #[tokio::test] diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index dea039c288..e306ec395e 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -57,6 +57,7 @@ pub use process::ExecProcessEvent; pub use process::ExecProcessEventReceiver; pub use process::StartedExecProcess; pub use process_id::ProcessId; +pub use protocol::EnvironmentInfo; pub use protocol::ExecClosedNotification; pub use protocol::ExecEnvPolicy; pub use protocol::ExecExitedNotification; @@ -94,6 +95,7 @@ pub use protocol::InitializeResponse; pub use protocol::ProcessOutputChunk; pub use protocol::ReadParams; pub use protocol::ReadResponse; +pub use protocol::ShellInfo; pub use protocol::TerminateParams; pub use protocol::TerminateResponse; pub use protocol::WriteParams; diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 9ac80c188e..6eeeeefdd7 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -19,6 +19,7 @@ pub const EXEC_TERMINATE_METHOD: &str = "process/terminate"; pub const EXEC_OUTPUT_DELTA_METHOD: &str = "process/output"; pub const EXEC_EXITED_METHOD: &str = "process/exited"; pub const EXEC_CLOSED_METHOD: &str = "process/closed"; +pub const ENVIRONMENT_INFO_METHOD: &str = "environment/info"; pub const FS_READ_FILE_METHOD: &str = "fs/readFile"; pub const FS_WRITE_FILE_METHOD: &str = "fs/writeFile"; pub const FS_CREATE_DIRECTORY_METHOD: &str = "fs/createDirectory"; @@ -64,6 +65,23 @@ pub struct InitializeResponse { pub session_id: String, } +/// Information about an execution/filesystem environment. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct EnvironmentInfo { + pub shell: ShellInfo, +} + +/// Shell detected for an execution/filesystem environment. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ShellInfo { + /// Stable shell name, for example `zsh`, `bash`, `powershell`, `sh`, or `cmd`. + pub name: String, + /// Path the exec server would use for that shell. + pub path: String, +} + #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ExecParams { diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 5456ce41c0..b8934705d2 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -14,6 +14,7 @@ use tokio_util::task::TaskTracker; use crate::ExecServerRuntimePaths; use crate::client::http_client::PendingReqwestHttpBodyStream; use crate::client::http_client::ReqwestHttpRequestRunner; +use crate::protocol::EnvironmentInfo; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCanonicalizeParams; @@ -147,6 +148,11 @@ impl ExecServerHandler { session.process().exec(params).await } + pub(crate) fn environment_info(&self) -> Result { + self.require_initialized_for("environment info")?; + Ok(EnvironmentInfo::local()) + } + pub(crate) async fn exec_read( &self, params: ReadParams, diff --git a/codex-rs/exec-server/src/server/registry.rs b/codex-rs/exec-server/src/server/registry.rs index 74adf90580..26d2876c63 100644 --- a/codex-rs/exec-server/src/server/registry.rs +++ b/codex-rs/exec-server/src/server/registry.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use crate::protocol::ENVIRONMENT_INFO_METHOD; use crate::protocol::EXEC_METHOD; use crate::protocol::EXEC_READ_METHOD; use crate::protocol::EXEC_TERMINATE_METHOD; @@ -60,6 +61,10 @@ pub(crate) fn build_router() -> RpcRouter { EXEC_METHOD, |handler: Arc, params: ExecParams| async move { handler.exec(params).await }, ); + router.request( + ENVIRONMENT_INFO_METHOD, + |handler: Arc, _params: ()| async move { handler.environment_info() }, + ); router.request( EXEC_READ_METHOD, |handler: Arc, params: ReadParams| async move { diff --git a/codex-rs/exec-server/tests/health.rs b/codex-rs/exec-server/tests/health.rs index 91b3806a22..1fcfb049b5 100644 --- a/codex-rs/exec-server/tests/health.rs +++ b/codex-rs/exec-server/tests/health.rs @@ -2,6 +2,7 @@ mod common; +use codex_exec_server::Environment; use common::exec_server::exec_server; use pretty_assertions::assert_eq; @@ -19,3 +20,17 @@ async fn exec_server_serves_readyz_alongside_websocket_endpoint() -> anyhow::Res server.shutdown().await?; Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_environment_fetches_info_from_exec_server() -> anyhow::Result<()> { + let mut server = exec_server().await?; + let environment = Environment::create_for_tests(Some(server.websocket_url().to_string()))?; + assert!(environment.is_remote()); + + let remote_info = environment.info().await?; + let local_info = Environment::default_for_tests().info().await?; + assert_eq!(remote_info, local_info); + + server.shutdown().await?; + Ok(()) +} diff --git a/codex-rs/shell-command/Cargo.toml b/codex-rs/shell-command/Cargo.toml index cc33d3621c..e918488f82 100644 --- a/codex-rs/shell-command/Cargo.toml +++ b/codex-rs/shell-command/Cargo.toml @@ -11,6 +11,7 @@ workspace = true base64 = { workspace = true } codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } +libc = { workspace = true } once_cell = { workspace = true } regex = { workspace = true } serde = { workspace = true, features = ["derive"] } diff --git a/codex-rs/shell-command/src/bash.rs b/codex-rs/shell-command/src/bash.rs index 60ee5c420c..b25d3fd37c 100644 --- a/codex-rs/shell-command/src/bash.rs +++ b/codex-rs/shell-command/src/bash.rs @@ -100,7 +100,7 @@ pub fn extract_bash_command(command: &[String]) -> Option<(&str, &str)> { }; if !matches!(flag.as_str(), "-lc" | "-c") || !matches!( - detect_shell_type(&PathBuf::from(shell)), + detect_shell_type(PathBuf::from(shell)), Some(ShellType::Zsh) | Some(ShellType::Bash) | Some(ShellType::Sh) ) { diff --git a/codex-rs/shell-command/src/lib.rs b/codex-rs/shell-command/src/lib.rs index 1d9e302a4e..947de5e645 100644 --- a/codex-rs/shell-command/src/lib.rs +++ b/codex-rs/shell-command/src/lib.rs @@ -1,6 +1,6 @@ //! Command parsing and safety utilities shared across Codex crates. -mod shell_detect; +pub mod shell_detect; pub mod bash; pub(crate) mod command_safety; diff --git a/codex-rs/shell-command/src/powershell.rs b/codex-rs/shell-command/src/powershell.rs index e8c9a500c4..9730439bea 100644 --- a/codex-rs/shell-command/src/powershell.rs +++ b/codex-rs/shell-command/src/powershell.rs @@ -47,7 +47,7 @@ pub fn extract_powershell_command(command: &[String]) -> Option<(&str, &str)> { let shell = &command[0]; if !matches!( - detect_shell_type(&PathBuf::from(shell)), + detect_shell_type(PathBuf::from(shell)), Some(ShellType::PowerShell) ) { return None; diff --git a/codex-rs/shell-command/src/shell_detect.rs b/codex-rs/shell-command/src/shell_detect.rs index 34322a06d0..7068463830 100644 --- a/codex-rs/shell-command/src/shell_detect.rs +++ b/codex-rs/shell-command/src/shell_detect.rs @@ -1,8 +1,10 @@ -use std::path::Path; use std::path::PathBuf; -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -pub(crate) enum ShellType { +use serde::Deserialize; +use serde::Serialize; + +#[derive(Debug, PartialEq, Eq, Clone, Copy, Serialize, Deserialize)] +pub enum ShellType { Zsh, Bash, PowerShell, @@ -10,7 +12,32 @@ pub(crate) enum ShellType { Cmd, } -pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option { +impl ShellType { + pub fn name(self) -> &'static str { + match self { + Self::Zsh => "zsh", + Self::Bash => "bash", + Self::PowerShell => "powershell", + Self::Sh => "sh", + Self::Cmd => "cmd", + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct DetectedShell { + pub shell_type: ShellType, + pub shell_path: PathBuf, +} + +impl DetectedShell { + pub fn name(&self) -> &'static str { + self.shell_type.name() + } +} + +pub fn detect_shell_type(shell_path: impl AsRef) -> Option { + let shell_path = shell_path.as_ref(); match shell_path.as_os_str().to_str() { Some("zsh") => Some(ShellType::Zsh), Some("sh") => Some(ShellType::Sh), @@ -21,12 +48,317 @@ pub(crate) fn detect_shell_type(shell_path: &PathBuf) -> Option { _ => { let shell_name = shell_path.file_stem(); if let Some(shell_name) = shell_name { - let shell_name_path = Path::new(shell_name); - if shell_name_path != Path::new(shell_path) { - return detect_shell_type(&shell_name_path.to_path_buf()); + let shell_name_path = std::path::Path::new(shell_name); + if shell_name_path != shell_path { + return detect_shell_type(shell_name_path); } } None } } } + +#[cfg(unix)] +fn get_user_shell_path() -> Option { + let uid = unsafe { libc::getuid() }; + use std::ffi::CStr; + use std::mem::MaybeUninit; + use std::ptr; + + let mut passwd = MaybeUninit::::uninit(); + + // We cannot use getpwuid here: it returns pointers into libc-managed + // storage, which is not safe to read concurrently on all targets (the musl + // static build used by the CLI can segfault when parallel callers race on + // that buffer). getpwuid_r keeps the passwd data in caller-owned memory. + let suggested_buffer_len = unsafe { libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX) }; + let buffer_len = usize::try_from(suggested_buffer_len) + .ok() + .filter(|len| *len > 0) + .unwrap_or(1024); + let mut buffer = vec![0; buffer_len]; + + loop { + let mut result = ptr::null_mut(); + let status = unsafe { + libc::getpwuid_r( + uid, + passwd.as_mut_ptr(), + buffer.as_mut_ptr().cast(), + buffer.len(), + &mut result, + ) + }; + + if status == 0 { + if result.is_null() { + return None; + } + + let passwd = unsafe { passwd.assume_init_ref() }; + if passwd.pw_shell.is_null() { + return None; + } + + let shell_path = unsafe { CStr::from_ptr(passwd.pw_shell) } + .to_string_lossy() + .into_owned(); + return Some(PathBuf::from(shell_path)); + } + + if status != libc::ERANGE { + return None; + } + + // Retry with a larger buffer until libc can materialize the passwd entry. + let new_len = buffer.len().checked_mul(2)?; + if new_len > 1024 * 1024 { + return None; + } + buffer.resize(new_len, 0); + } +} + +#[cfg(not(unix))] +fn get_user_shell_path() -> Option { + None +} + +fn file_exists(path: &std::path::Path) -> Option { + if std::fs::metadata(path).is_ok_and(|metadata| metadata.is_file()) { + Some(PathBuf::from(path)) + } else { + None + } +} + +fn get_shell_path( + shell_type: ShellType, + provided_path: Option<&PathBuf>, + binary_name: &str, + fallback_paths: &[&str], +) -> Option { + if let Some(path) = provided_path.and_then(|path| file_exists(path)) { + return Some(path); + } + + let default_shell_path = get_user_shell_path(); + if let Some(default_shell_path) = default_shell_path + && detect_shell_type(&default_shell_path) == Some(shell_type) + && file_exists(&default_shell_path).is_some() + { + return Some(default_shell_path); + } + + if let Ok(path) = which::which(binary_name) { + return Some(path); + } + + for path in fallback_paths { + if let Some(path) = file_exists(std::path::Path::new(path)) { + return Some(path); + } + } + + None +} + +const ZSH_FALLBACK_PATHS: &[&str] = &["/bin/zsh"]; + +fn get_zsh_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Zsh, path, "zsh", ZSH_FALLBACK_PATHS); + + shell_path.map(|shell_path| DetectedShell { + shell_type: ShellType::Zsh, + shell_path, + }) +} + +const BASH_FALLBACK_PATHS: &[&str] = &["/bin/bash"]; + +fn get_bash_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Bash, path, "bash", BASH_FALLBACK_PATHS); + + shell_path.map(|shell_path| DetectedShell { + shell_type: ShellType::Bash, + shell_path, + }) +} + +const SH_FALLBACK_PATHS: &[&str] = &["/bin/sh"]; + +fn get_sh_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Sh, path, "sh", SH_FALLBACK_PATHS); + + shell_path.map(|shell_path| DetectedShell { + shell_type: ShellType::Sh, + shell_path, + }) +} + +// Note the `pwsh` and `powershell` fallback paths are where the respective +// shells are commonly installed on GitHub Actions Windows runners, but may not +// be present on all Windows machines: +// https://docs.github.com/en/actions/tutorials/build-and-test-code/powershell + +#[cfg(windows)] +const PWSH_FALLBACK_PATHS: &[&str] = &[r#"C:\Program Files\PowerShell\7\pwsh.exe"#]; +#[cfg(not(windows))] +const PWSH_FALLBACK_PATHS: &[&str] = &["/usr/local/bin/pwsh"]; + +#[cfg(windows)] +const POWERSHELL_FALLBACK_PATHS: &[&str] = + &[r#"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe"#]; +#[cfg(not(windows))] +const POWERSHELL_FALLBACK_PATHS: &[&str] = &[]; + +fn get_powershell_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::PowerShell, path, "pwsh", PWSH_FALLBACK_PATHS) + .or_else(|| { + get_shell_path( + ShellType::PowerShell, + path, + "powershell", + POWERSHELL_FALLBACK_PATHS, + ) + }); + + shell_path.map(|shell_path| DetectedShell { + shell_type: ShellType::PowerShell, + shell_path, + }) +} + +fn get_cmd_shell(path: Option<&PathBuf>) -> Option { + let shell_path = get_shell_path(ShellType::Cmd, path, "cmd", &[]); + + shell_path.map(|shell_path| DetectedShell { + shell_type: ShellType::Cmd, + shell_path, + }) +} + +pub fn ultimate_fallback_shell() -> DetectedShell { + if cfg!(windows) { + DetectedShell { + shell_type: ShellType::Cmd, + shell_path: PathBuf::from("cmd.exe"), + } + } else { + DetectedShell { + shell_type: ShellType::Sh, + shell_path: PathBuf::from("/bin/sh"), + } + } +} + +pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> DetectedShell { + detect_shell_type(shell_path) + .and_then(|shell_type| get_shell(shell_type, Some(shell_path))) + .unwrap_or_else(ultimate_fallback_shell) +} + +pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { + match shell_type { + ShellType::Zsh => get_zsh_shell(path), + ShellType::Bash => get_bash_shell(path), + ShellType::PowerShell => get_powershell_shell(path), + ShellType::Sh => get_sh_shell(path), + ShellType::Cmd => get_cmd_shell(path), + } +} + +pub fn default_user_shell() -> DetectedShell { + default_user_shell_from_path(get_user_shell_path()) +} + +pub fn default_user_shell_from_path(user_shell_path: Option) -> DetectedShell { + if cfg!(windows) { + get_shell(ShellType::PowerShell, /*path*/ None).unwrap_or_else(ultimate_fallback_shell) + } else { + let user_default_shell = user_shell_path + .and_then(|shell| detect_shell_type(&shell)) + .and_then(|shell_type| get_shell(shell_type, /*path*/ None)); + + let shell_with_fallback = if cfg!(target_os = "macos") { + user_default_shell + .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) + .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) + } else { + user_default_shell + .or_else(|| get_shell(ShellType::Bash, /*path*/ None)) + .or_else(|| get_shell(ShellType::Zsh, /*path*/ None)) + }; + + shell_with_fallback.unwrap_or_else(ultimate_fallback_shell) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn test_detect_shell_type() { + assert_eq!( + detect_shell_type(PathBuf::from("zsh")), + Some(ShellType::Zsh) + ); + assert_eq!( + detect_shell_type(PathBuf::from("bash")), + Some(ShellType::Bash) + ); + assert_eq!( + detect_shell_type(PathBuf::from("pwsh")), + Some(ShellType::PowerShell) + ); + assert_eq!( + detect_shell_type(PathBuf::from("powershell")), + Some(ShellType::PowerShell) + ); + assert_eq!(detect_shell_type(PathBuf::from("fish")), None); + assert_eq!(detect_shell_type(PathBuf::from("other")), None); + assert_eq!( + detect_shell_type(PathBuf::from("/bin/zsh")), + Some(ShellType::Zsh) + ); + assert_eq!( + detect_shell_type(PathBuf::from("/bin/bash")), + Some(ShellType::Bash) + ); + assert_eq!( + detect_shell_type(PathBuf::from("powershell.exe")), + Some(ShellType::PowerShell) + ); + assert_eq!( + detect_shell_type(PathBuf::from(if cfg!(windows) { + "C:\\windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe" + } else { + "/usr/local/bin/pwsh" + })), + Some(ShellType::PowerShell) + ); + assert_eq!( + detect_shell_type(PathBuf::from("pwsh.exe")), + Some(ShellType::PowerShell) + ); + assert_eq!( + detect_shell_type(PathBuf::from("/usr/local/bin/pwsh")), + Some(ShellType::PowerShell) + ); + assert_eq!( + detect_shell_type(PathBuf::from("/bin/sh")), + Some(ShellType::Sh) + ); + assert_eq!(detect_shell_type(PathBuf::from("sh")), Some(ShellType::Sh)); + assert_eq!( + detect_shell_type(PathBuf::from("cmd")), + Some(ShellType::Cmd) + ); + assert_eq!( + detect_shell_type(PathBuf::from("cmd.exe")), + Some(ShellType::Cmd) + ); + } +}