From 4b01f0f50a640192e37c492464e4f76e9cc80fb6 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 16 Oct 2025 11:23:38 -0700 Subject: [PATCH] fix: tui default trusted settings should respect workspace write config (#3341) ## Summary When using the trusted state during tui startup, we created a new WorkspaceWrite policy without checking the config.toml for a `sandbox_workspace_write` field. This would result in us setting the sandbox_mode as workspace-write, but ignoring the field if the user had set `sandbox_workspace_write` without also setting `sandbox_mode` in the config.toml. This PR adds support for respecting `sandbox_workspace_write` setting in config.toml in the trusted directory flow, and adds tests to cover this case. ## Testing - [x] Added unit tests --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/config.rs | 195 +++++++++++++++++++++++++++--------- codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/lib.rs | 151 ++++++++++------------------ 4 files changed, 203 insertions(+), 145 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 5576b7cb..70514dd5 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1422,6 +1422,7 @@ dependencies = [ "textwrap 0.16.2", "tokio", "tokio-stream", + "toml", "tracing", "tracing-appender", "tracing-subscriber", diff --git a/codex-rs/core/src/config.rs b/codex-rs/core/src/config.rs index f422e4ab..47456a64 100644 --- a/codex-rs/core/src/config.rs +++ b/codex-rs/core/src/config.rs @@ -42,6 +42,7 @@ use codex_protocol::config_types::Verbosity; use codex_rmcp_client::OAuthCredentialsStoreMode; use dirs::home_dir; use serde::Deserialize; +use similar::DiffableStr; use std::collections::BTreeMap; use std::collections::HashMap; use std::io::ErrorKind; @@ -100,6 +101,10 @@ pub struct Config { pub sandbox_policy: SandboxPolicy, + /// True if the user passed in an override or set a value in config.toml + /// for either of approval_policy or sandbox_mode. + pub did_user_set_custom_approval_policy_or_sandbox_mode: bool, + pub shell_environment_policy: ShellEnvironmentPolicy, /// When `true`, `AgentReasoning` events emitted by the backend will be @@ -230,6 +235,10 @@ pub struct Config { /// The active profile name used to derive this `Config` (if any). pub active_profile: Option, + /// The currently active project config, resolved by checking if cwd: + /// is (1) part of a git repo, (2) a git worktree, or (3) just using the cwd + pub active_project: ProjectConfig, + /// Tracks whether the Windows onboarding screen has been acknowledged. pub windows_wsl_setup_acknowledged: bool, @@ -859,6 +868,15 @@ pub struct ProjectConfig { pub trust_level: Option, } +impl ProjectConfig { + pub fn is_trusted(&self) -> bool { + match &self.trust_level { + Some(trust_level) => trust_level == "trusted", + None => false, + } + } +} + #[derive(Deserialize, Debug, Clone, Default, PartialEq)] pub struct ToolsToml { #[serde(default, alias = "web_search_request")] @@ -880,9 +898,23 @@ impl From for Tools { impl ConfigToml { /// Derive the effective sandbox policy from the configuration. - fn derive_sandbox_policy(&self, sandbox_mode_override: Option) -> SandboxPolicy { + fn derive_sandbox_policy( + &self, + sandbox_mode_override: Option, + resolved_cwd: &Path, + ) -> SandboxPolicy { let resolved_sandbox_mode = sandbox_mode_override .or(self.sandbox_mode) + .or_else(|| { + // if no sandbox_mode is set, but user has marked directory as trusted, use WorkspaceWrite + self.get_active_project(resolved_cwd).and_then(|p| { + if p.is_trusted() { + Some(SandboxMode::WorkspaceWrite) + } else { + None + } + }) + }) .unwrap_or_default(); match resolved_sandbox_mode { SandboxMode::ReadOnly => SandboxPolicy::new_read_only_policy(), @@ -904,30 +936,26 @@ impl ConfigToml { } } - pub fn is_cwd_trusted(&self, resolved_cwd: &Path) -> bool { + /// Resolves the cwd to an existing project, or returns None if ConfigToml + /// does not contain a project corresponding to cwd or a git repo for cwd + pub fn get_active_project(&self, resolved_cwd: &Path) -> Option { let projects = self.projects.clone().unwrap_or_default(); - let is_path_trusted = |path: &Path| { - let path_str = path.to_string_lossy().to_string(); - projects - .get(&path_str) - .map(|p| p.trust_level.as_deref() == Some("trusted")) - .unwrap_or(false) - }; - - // Fast path: exact cwd match - if is_path_trusted(resolved_cwd) { - return true; + if let Some(project_config) = projects.get(&resolved_cwd.to_string_lossy().to_string()) { + return Some(project_config.clone()); } - // If cwd lives inside a git worktree, check whether the root git project + // If cwd lives inside a git repo/worktree, check whether the root git project // (the primary repository working directory) is trusted. This lets // worktrees inherit trust from the main project. - if let Some(root_project) = resolve_root_git_project_for_trust(resolved_cwd) { - return is_path_trusted(&root_project); + if let Some(repo_root) = resolve_root_git_project_for_trust(resolved_cwd) + && let Some(project_config_for_root) = + projects.get(&repo_root.to_string_lossy().to_string_lossy().to_string()) + { + return Some(project_config_for_root.clone()); } - false + None } pub fn get_config_profile( @@ -986,7 +1014,7 @@ impl Config { model, review_model: override_review_model, cwd, - approval_policy, + approval_policy: approval_policy_override, sandbox_mode, model_provider, config_profile: config_profile_key, @@ -1026,7 +1054,47 @@ impl Config { let features = Features::from_config(&cfg, &config_profile, feature_overrides); - let sandbox_policy = cfg.derive_sandbox_policy(sandbox_mode); + let resolved_cwd = { + use std::env; + + match cwd { + None => { + tracing::info!("cwd not set, using current dir"); + env::current_dir()? + } + Some(p) if p.is_absolute() => p, + Some(p) => { + // Resolve relative path against the current working directory. + tracing::info!("cwd is relative, resolving against current dir"); + let mut current = env::current_dir()?; + current.push(p); + current + } + } + }; + let active_project = cfg + .get_active_project(&resolved_cwd) + .unwrap_or(ProjectConfig { trust_level: None }); + + let sandbox_policy = cfg.derive_sandbox_policy(sandbox_mode, &resolved_cwd); + let mut approval_policy = approval_policy_override + .or(config_profile.approval_policy) + .or(cfg.approval_policy) + .unwrap_or_else(|| { + if active_project.is_trusted() { + // If no explicit approval policy is set, but we trust cwd, default to OnRequest + AskForApproval::OnRequest + } else { + AskForApproval::default() + } + }); + let did_user_set_custom_approval_policy_or_sandbox_mode = approval_policy_override + .is_some() + || config_profile.approval_policy.is_some() + || cfg.approval_policy.is_some() + // TODO(#3034): profile.sandbox_mode is not implemented + || sandbox_mode.is_some() + || cfg.sandbox_mode.is_some(); let mut model_providers = built_in_model_providers(); // Merge user-defined providers into the built-in list. @@ -1050,25 +1118,6 @@ impl Config { let shell_environment_policy = cfg.shell_environment_policy.into(); - let resolved_cwd = { - use std::env; - - match cwd { - None => { - tracing::info!("cwd not set, using current dir"); - env::current_dir()? - } - Some(p) if p.is_absolute() => p, - Some(p) => { - // Resolve relative path against the current working directory. - tracing::info!("cwd is relative, resolving against current dir"); - let mut current = env::current_dir()?; - current.push(p); - current - } - } - }; - let history = cfg.history.unwrap_or_default(); let include_plan_tool_flag = features.enabled(Feature::PlanTool); @@ -1125,11 +1174,6 @@ impl Config { .or(cfg.review_model) .unwrap_or_else(default_review_model); - let mut approval_policy = approval_policy - .or(config_profile.approval_policy) - .or(cfg.approval_policy) - .unwrap_or_else(AskForApproval::default); - if features.enabled(Feature::ApproveAll) { approval_policy = AskForApproval::OnRequest; } @@ -1146,6 +1190,7 @@ impl Config { cwd: resolved_cwd, approval_policy, sandbox_policy, + did_user_set_custom_approval_policy_or_sandbox_mode, shell_environment_policy, notify: cfg.notify, user_instructions, @@ -1200,6 +1245,7 @@ impl Config { include_view_image_tool: include_view_image_tool_flag, features, active_profile: active_profile_name, + active_project, windows_wsl_setup_acknowledged: cfg.windows_wsl_setup_acknowledged.unwrap_or(false), disable_paste_burst: cfg.disable_paste_burst.unwrap_or(false), tui_notifications: cfg @@ -1395,7 +1441,8 @@ network_access = false # This should be ignored. let sandbox_mode_override = None; assert_eq!( SandboxPolicy::DangerFullAccess, - sandbox_full_access_cfg.derive_sandbox_policy(sandbox_mode_override) + sandbox_full_access_cfg + .derive_sandbox_policy(sandbox_mode_override, &PathBuf::from("/tmp/test")) ); let sandbox_read_only = r#" @@ -1410,7 +1457,8 @@ network_access = true # This should be ignored. let sandbox_mode_override = None; assert_eq!( SandboxPolicy::ReadOnly, - sandbox_read_only_cfg.derive_sandbox_policy(sandbox_mode_override) + sandbox_read_only_cfg + .derive_sandbox_policy(sandbox_mode_override, &PathBuf::from("/tmp/test")) ); let sandbox_workspace_write = r#" @@ -1434,7 +1482,36 @@ exclude_slash_tmp = true exclude_tmpdir_env_var: true, exclude_slash_tmp: true, }, - sandbox_workspace_write_cfg.derive_sandbox_policy(sandbox_mode_override) + sandbox_workspace_write_cfg + .derive_sandbox_policy(sandbox_mode_override, &PathBuf::from("/tmp/test")) + ); + + let sandbox_workspace_write = r#" +sandbox_mode = "workspace-write" + +[sandbox_workspace_write] +writable_roots = [ + "/my/workspace", +] +exclude_tmpdir_env_var = true +exclude_slash_tmp = true + +[projects."/tmp/test"] +trust_level = "trusted" +"#; + + let sandbox_workspace_write_cfg = toml::from_str::(sandbox_workspace_write) + .expect("TOML deserialization should succeed"); + let sandbox_mode_override = None; + assert_eq!( + SandboxPolicy::WorkspaceWrite { + writable_roots: vec![PathBuf::from("/my/workspace")], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }, + sandbox_workspace_write_cfg + .derive_sandbox_policy(sandbox_mode_override, &PathBuf::from("/tmp/test")) ); } @@ -2221,6 +2298,7 @@ model_verbosity = "high" model_provider: fixture.openai_provider.clone(), approval_policy: AskForApproval::Never, sandbox_policy: SandboxPolicy::new_read_only_policy(), + did_user_set_custom_approval_policy_or_sandbox_mode: true, shell_environment_policy: ShellEnvironmentPolicy::default(), user_instructions: None, notify: None, @@ -2250,6 +2328,7 @@ model_verbosity = "high" include_view_image_tool: true, features: Features::with_defaults(), active_profile: Some("o3".to_string()), + active_project: ProjectConfig { trust_level: None }, windows_wsl_setup_acknowledged: false, disable_paste_burst: false, tui_notifications: Default::default(), @@ -2285,6 +2364,7 @@ model_verbosity = "high" model_provider: fixture.openai_chat_completions_provider.clone(), approval_policy: AskForApproval::UnlessTrusted, sandbox_policy: SandboxPolicy::new_read_only_policy(), + did_user_set_custom_approval_policy_or_sandbox_mode: true, shell_environment_policy: ShellEnvironmentPolicy::default(), user_instructions: None, notify: None, @@ -2314,6 +2394,7 @@ model_verbosity = "high" include_view_image_tool: true, features: Features::with_defaults(), active_profile: Some("gpt3".to_string()), + active_project: ProjectConfig { trust_level: None }, windows_wsl_setup_acknowledged: false, disable_paste_burst: false, tui_notifications: Default::default(), @@ -2364,6 +2445,7 @@ model_verbosity = "high" model_provider: fixture.openai_provider.clone(), approval_policy: AskForApproval::OnFailure, sandbox_policy: SandboxPolicy::new_read_only_policy(), + did_user_set_custom_approval_policy_or_sandbox_mode: true, shell_environment_policy: ShellEnvironmentPolicy::default(), user_instructions: None, notify: None, @@ -2393,6 +2475,7 @@ model_verbosity = "high" include_view_image_tool: true, features: Features::with_defaults(), active_profile: Some("zdr".to_string()), + active_project: ProjectConfig { trust_level: None }, windows_wsl_setup_acknowledged: false, disable_paste_burst: false, tui_notifications: Default::default(), @@ -2429,6 +2512,7 @@ model_verbosity = "high" model_provider: fixture.openai_provider.clone(), approval_policy: AskForApproval::OnFailure, sandbox_policy: SandboxPolicy::new_read_only_policy(), + did_user_set_custom_approval_policy_or_sandbox_mode: true, shell_environment_policy: ShellEnvironmentPolicy::default(), user_instructions: None, notify: None, @@ -2458,6 +2542,7 @@ model_verbosity = "high" include_view_image_tool: true, features: Features::with_defaults(), active_profile: Some("gpt5".to_string()), + active_project: ProjectConfig { trust_level: None }, windows_wsl_setup_acknowledged: false, disable_paste_burst: false, tui_notifications: Default::default(), @@ -2469,6 +2554,24 @@ model_verbosity = "high" Ok(()) } + #[test] + fn test_did_user_set_custom_approval_policy_or_sandbox_mode_defaults_no() -> anyhow::Result<()> + { + let fixture = create_test_fixture()?; + + let config = Config::load_from_base_config_with_overrides( + fixture.cfg.clone(), + ConfigOverrides { + ..Default::default() + }, + fixture.codex_home(), + )?; + + assert!(config.did_user_set_custom_approval_policy_or_sandbox_mode); + + Ok(()) + } + #[test] fn test_set_project_trusted_writes_explicit_tables() -> anyhow::Result<()> { let project_dir = Path::new("/some/path"); diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 2c6f32a2..1ba3f086 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -78,6 +78,7 @@ tokio = { workspace = true, features = [ "signal", ] } tokio-stream = { workspace = true } +toml = { workspace = true } tracing = { workspace = true, features = ["log"] } tracing-appender = { workspace = true } tracing-subscriber = { workspace = true, features = ["env-filter"] } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 01b1bd97..7447ef5f 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -13,12 +13,8 @@ use codex_core::INTERACTIVE_SESSION_SOURCES; use codex_core::RolloutRecorder; use codex_core::config::Config; use codex_core::config::ConfigOverrides; -use codex_core::config::ConfigToml; -use codex_core::config::find_codex_home; -use codex_core::config::load_config_as_toml_with_cli_overrides; use codex_core::find_conversation_path_by_id_str; use codex_core::protocol::AskForApproval; -use codex_core::protocol::SandboxPolicy; use codex_ollama::DEFAULT_OSS_MODEL; use codex_protocol::config_types::SandboxMode; use opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge; @@ -192,52 +188,9 @@ pub async fn run_main( } }; - let mut config = { - // Load configuration and support CLI overrides. - - #[allow(clippy::print_stderr)] - match Config::load_with_cli_overrides(cli_kv_overrides.clone(), overrides).await { - Ok(config) => config, - Err(err) => { - eprintln!("Error loading configuration: {err}"); - std::process::exit(1); - } - } - }; - - // we load config.toml here to determine project state. - #[allow(clippy::print_stderr)] - let config_toml = { - let codex_home = match find_codex_home() { - Ok(codex_home) => codex_home, - Err(err) => { - eprintln!("Error finding codex home: {err}"); - std::process::exit(1); - } - }; - - match load_config_as_toml_with_cli_overrides(&codex_home, cli_kv_overrides).await { - Ok(config_toml) => config_toml, - Err(err) => { - eprintln!("Error loading config.toml: {err}"); - std::process::exit(1); - } - } - }; - - let cli_profile_override = cli.config_profile.clone(); - let active_profile = cli_profile_override - .clone() - .or_else(|| config_toml.profile.clone()); - - let should_show_trust_screen = determine_repo_trust_state( - &mut config, - &config_toml, - approval_policy, - sandbox_mode, - cli_profile_override, - )?; + let config = load_config_or_exit(cli_kv_overrides.clone(), overrides.clone()).await; + let active_profile = config.active_profile.clone(); let log_dir = codex_core::config::log_dir(&config)?; std::fs::create_dir_all(&log_dir)?; // Open (or create) your log file, appending to it. @@ -303,18 +256,18 @@ pub async fn run_main( let _ = tracing_subscriber::registry().with(file_layer).try_init(); }; - run_ratatui_app(cli, config, active_profile, should_show_trust_screen) + run_ratatui_app(cli, config, overrides, cli_kv_overrides, active_profile) .await .map_err(|err| std::io::Error::other(err.to_string())) } async fn run_ratatui_app( cli: Cli, - config: Config, + initial_config: Config, + overrides: ConfigOverrides, + cli_kv_overrides: Vec<(String, toml::Value)>, active_profile: Option, - should_show_trust_screen: bool, ) -> color_eyre::Result { - let mut config = config; color_eyre::install()?; // Forward panic reports through tracing so they appear in the UI status @@ -337,7 +290,7 @@ async fn run_ratatui_app( let skip_update_prompt = cli.prompt.as_ref().is_some_and(|prompt| !prompt.is_empty()); if !skip_update_prompt { - match update_prompt::run_update_prompt_if_needed(&mut tui, &config).await? { + match update_prompt::run_update_prompt_if_needed(&mut tui, &initial_config).await? { UpdatePromptOutcome::Continue => {} UpdatePromptOutcome::RunUpdate(action) => { crate::tui::restore()?; @@ -354,7 +307,7 @@ async fn run_ratatui_app( // Show update banner in terminal history (instead of stderr) so it is visible // within the TUI scrollback. Building spans keeps styling consistent. #[cfg(not(debug_assertions))] - if let Some(latest_version) = updates::get_upgrade_version(&config) { + if let Some(latest_version) = updates::get_upgrade_version(&initial_config) { use crate::history_cell::padded_emoji; use crate::history_cell::with_border_with_inner_width; use ratatui::style::Stylize as _; @@ -402,27 +355,29 @@ async fn run_ratatui_app( } // Initialize high-fidelity session event logging if enabled. - session_log::maybe_init(&config); + session_log::maybe_init(&initial_config); - let auth_manager = AuthManager::shared(config.codex_home.clone(), false); - let login_status = get_login_status(&config); + let auth_manager = AuthManager::shared(initial_config.codex_home.clone(), false); + let login_status = get_login_status(&initial_config); + let should_show_trust_screen = should_show_trust_screen(&initial_config); let should_show_windows_wsl_screen = - cfg!(target_os = "windows") && !config.windows_wsl_setup_acknowledged; + cfg!(target_os = "windows") && !initial_config.windows_wsl_setup_acknowledged; let should_show_onboarding = should_show_onboarding( login_status, - &config, + &initial_config, should_show_trust_screen, should_show_windows_wsl_screen, ); - if should_show_onboarding { + + let config = if should_show_onboarding { let onboarding_result = run_onboarding_app( OnboardingScreenArgs { + show_login_screen: should_show_login_screen(login_status, &initial_config), show_windows_wsl_screen: should_show_windows_wsl_screen, - show_login_screen: should_show_login_screen(login_status, &config), show_trust_screen: should_show_trust_screen, login_status, auth_manager: auth_manager.clone(), - config: config.clone(), + config: initial_config.clone(), }, &mut tui, ) @@ -440,14 +395,20 @@ async fn run_ratatui_app( update_action: None, }); } - if should_show_windows_wsl_screen { - config.windows_wsl_setup_acknowledged = true; + // if the user acknowledged windows or made an explicit decision ato trust the directory, reload the config accordingly + if should_show_windows_wsl_screen + || onboarding_result + .directory_trust_decision + .map(|d| d == TrustDirectorySelection::Trust) + .unwrap_or(false) + { + load_config_or_exit(cli_kv_overrides, overrides).await + } else { + initial_config } - if let Some(TrustDirectorySelection::Trust) = onboarding_result.directory_trust_decision { - config.approval_policy = AskForApproval::OnRequest; - config.sandbox_policy = SandboxPolicy::new_workspace_write_policy(); - } - } + } else { + initial_config + }; // Determine resume behavior: explicit id, then resume last, then picker. let resume_selection = if let Some(id_str) = cli.resume_session_id.as_deref() { @@ -588,39 +549,31 @@ fn get_login_status(config: &Config) -> LoginStatus { } } -/// Determine if user has configured a sandbox / approval policy, -/// or if the current cwd project is trusted, and updates the config -/// accordingly. -fn determine_repo_trust_state( - config: &mut Config, - config_toml: &ConfigToml, - approval_policy_overide: Option, - sandbox_mode_override: Option, - config_profile_override: Option, -) -> std::io::Result { - let config_profile = config_toml.get_config_profile(config_profile_override)?; +async fn load_config_or_exit( + cli_kv_overrides: Vec<(String, toml::Value)>, + overrides: ConfigOverrides, +) -> Config { + #[allow(clippy::print_stderr)] + match Config::load_with_cli_overrides(cli_kv_overrides, overrides).await { + Ok(config) => config, + Err(err) => { + eprintln!("Error loading configuration: {err}"); + std::process::exit(1); + } + } +} - if approval_policy_overide.is_some() || sandbox_mode_override.is_some() { +/// Determine if user has configured a sandbox / approval policy, +/// or if the current cwd project is already trusted. If not, we need to +/// show the trust screen. +fn should_show_trust_screen(config: &Config) -> bool { + if config.did_user_set_custom_approval_policy_or_sandbox_mode { // if the user has overridden either approval policy or sandbox mode, // skip the trust flow - Ok(false) - } else if config_profile.approval_policy.is_some() { - // if the user has specified settings in a config profile, skip the trust flow - // todo: profile sandbox mode? - Ok(false) - } else if config_toml.approval_policy.is_some() || config_toml.sandbox_mode.is_some() { - // if the user has specified either approval policy or sandbox mode in config.toml - // skip the trust flow - Ok(false) - } else if config_toml.is_cwd_trusted(&config.cwd) { - // if the current cwd project is trusted and no config has been set - // skip the trust flow and set the approval policy and sandbox mode - config.approval_policy = AskForApproval::OnRequest; - config.sandbox_policy = SandboxPolicy::new_workspace_write_policy(); - Ok(false) + false } else { - // if none of the above conditions are met, show the trust screen - Ok(true) + // otherwise, skip iff the active project is trusted + !config.active_project.is_trusted() } }