From 79034a9e01ad8c3cba6258537d63866b8beaa2e6 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 7 Jul 2026 09:29:46 -0300 Subject: [PATCH] fix(tui): preserve browser network access in auto review --- codex-rs/tui/src/app/config_persistence.rs | 59 ++++++++------ codex-rs/tui/src/app/tests.rs | 59 ++++++++++++++ codex-rs/tui/src/app_server_session.rs | 8 +- codex-rs/tui/src/chatwidget.rs | 9 +++ .../tui/src/chatwidget/permission_popups.rs | 77 +++++++++++++------ codex-rs/tui/src/chatwidget/tests.rs | 1 - .../tui/src/chatwidget/tests/permissions.rs | 34 ++++---- .../src/chatwidget/windows_sandbox_prompts.rs | 12 ++- codex-rs/tui/src/terminal_browser/network.rs | 9 --- .../tui/src/terminal_browser/network_tests.rs | 6 -- codex-rs/tui/src/terminal_browser/tools.rs | 3 +- 11 files changed, 187 insertions(+), 90 deletions(-) diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index bb3ee385a3..bc6fa8f030 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -431,9 +431,9 @@ impl App { } if feature == Feature::GuardianApproval && effective_enabled { // The feature flag alone is not enough for the live session. - // We also align approval policy + sandbox to the Auto-review - // preset so enabling the experiment immediately - // makes guardian review observable in the current thread. + // We also align the approval policy with Auto-review. Preserve + // an existing workspace-shaped profile so enabling the reviewer + // does not discard configured roots or network access. if !self.try_set_approval_policy_on_config( &mut feature_config, auto_review_preset.approval_policy, @@ -442,30 +442,34 @@ impl App { ) { continue; } - let Some(permission_profile) = self - .try_set_builtin_active_permission_profile_on_config( - &mut feature_config, - auto_review_preset.active_permission_profile.clone(), - "Failed to enable Approve for me", - "failed to set auto-review permission profile on staged config", - ) - else { - continue; - }; - feature_edits.extend([ - crate::config_update::replace_config_value( - "approval_policy", - serde_json::json!("on-request"), - ), - crate::config_update::replace_config_value( + feature_edits.push(crate::config_update::replace_config_value( + "approval_policy", + serde_json::json!("on-request"), + )); + let preserve_workspace_profile = ChatWidget::workspace_profile_matches_auto_mode( + feature_config.permissions.permission_profile(), + feature_config.cwd.as_path(), + ); + if !preserve_workspace_profile { + let Some(permission_profile) = self + .try_set_builtin_active_permission_profile_on_config( + &mut feature_config, + auto_review_preset.active_permission_profile.clone(), + "Failed to enable Approve for me", + "failed to set auto-review permission profile on staged config", + ) + else { + continue; + }; + feature_edits.push(crate::config_update::replace_config_value( "sandbox_mode", serde_json::json!("workspace-write"), - ), - ]); + )); + permission_profile_override = Some(permission_profile); + active_permission_profile_override = + Some(auto_review_preset.active_permission_profile.clone()); + } approval_policy_override = Some(auto_review_preset.approval_policy); - permission_profile_override = Some(permission_profile); - active_permission_profile_override = - Some(auto_review_preset.active_permission_profile.clone()); } next_config = feature_config; feature_updates_to_apply.push((feature, effective_enabled)); @@ -882,6 +886,13 @@ impl App { return; } + if ChatWidget::workspace_profile_matches_auto_mode( + self.config.permissions.permission_profile(), + self.config.cwd.as_path(), + ) { + return; + } + let auto_review_preset = auto_review_mode(); let mut config = self.config.clone(); let Some(permission_profile) = self.try_set_builtin_active_permission_profile_on_config( diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index c69ffa26a5..cda8fdf134 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -95,6 +95,7 @@ use codex_protocol::models::ActivePermissionProfile; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::MAX_THREAD_GOAL_OBJECTIVE_CHARS; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::user_input::TextElement; @@ -2062,6 +2063,64 @@ async fn update_feature_flags_enabling_guardian_selects_auto_review() -> Result< Ok(()) } +#[tokio::test] +async fn terminal_browser_enabling_guardian_preserves_network_enabled_workspace_profile() +-> Result<()> { + let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + let codex_home = tempdir()?; + app.config.codex_home = codex_home.path().to_path_buf().abs(); + let permission_profile = PermissionProfile::workspace_write_with( + &[], + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + app.config + .permissions + .set_permission_profile(permission_profile.clone())?; + app.chat_widget + .set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy( + permission_profile.clone(), + ))?; + let mut app_server = start_config_write_test_app_server(&app).await?; + + app.update_feature_flags(&mut app_server, vec![(Feature::GuardianApproval, true)]) + .await; + + assert_eq!( + app.config.permissions.permission_profile(), + &permission_profile + ); + assert_eq!( + app.chat_widget + .config_ref() + .permissions + .permission_profile(), + &permission_profile + ); + assert_eq!( + op_rx.try_recv(), + Ok(Op::OverrideTurnContext { + cwd: None, + approval_policy: Some(AskForApproval::OnRequest), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), + permission_profile: None, + active_permission_profile: None, + windows_sandbox_level: None, + model: None, + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + }) + ); + let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?; + assert!(!config.contains("sandbox_mode =")); + app_server.shutdown().await?; + Ok(()) +} + #[tokio::test] async fn update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default() -> Result<()> { diff --git a/codex-rs/tui/src/app_server_session.rs b/codex-rs/tui/src/app_server_session.rs index 5cf23a6f53..447b28f2bf 100644 --- a/codex-rs/tui/src/app_server_session.rs +++ b/codex-rs/tui/src/app_server_session.rs @@ -14,7 +14,6 @@ use crate::session_state::SessionNetworkProxyRuntime; use crate::session_state::ThreadSessionState; use crate::status::StatusAccountDisplay; use crate::status::plan_type_display_name; -use crate::terminal_browser::TerminalBrowserNetworkAvailability; use crate::terminal_browser::dynamic_tool_specs; use crate::terminal_visualization_instructions::with_terminal_visualization_instructions; use codex_app_server_client::AppServerClient; @@ -1411,8 +1410,7 @@ fn thread_start_params_from_config( ) -> ThreadStartParams { let dynamic_tools = (matches!(thread_params_mode, ThreadParamsMode::Embedded) && cfg!(any(target_os = "macos", target_os = "linux")) - && config.features.enabled(Feature::TerminalBrowser) - && TerminalBrowserNetworkAvailability::dynamic_tools_supported(config)) + && config.features.enabled(Feature::TerminalBrowser)) .then(dynamic_tool_specs); let permissions = permissions_selection_from_config(config, thread_params_mode); let sandbox = permissions @@ -1968,7 +1966,7 @@ mod tests { #[cfg(any(target_os = "macos", target_os = "linux"))] #[tokio::test] - async fn terminal_browser_dynamic_tools_allow_non_mitm_managed_network() { + async fn terminal_browser_dynamic_tools_stay_visible_when_network_is_restricted() { let direct_config = build_terminal_browser_config(/*managed_network*/ false).await; let managed_config = build_terminal_browser_config(/*managed_network*/ true).await; assert_eq!( @@ -2016,7 +2014,7 @@ mod tests { "network restricted", &restricted_config, ThreadParamsMode::Embedded, - false, + true, ), ( "managed network", diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index fd5c2d75a4..a94b467f10 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -480,6 +480,15 @@ const DEFAULT_OPENAI_BASE_URL: &str = "https://api.openai.com/v1"; const DEFAULT_STATUS_LINE_ITEMS: [&str; 2] = ["model-with-reasoning", "current-dir"]; const MAX_AGENT_COPY_HISTORY: usize = 32; +#[derive(Clone)] +enum PermissionPresetProfileUpdate { + Preserve, + Replace { + permission_profile: PermissionProfile, + active_permission_profile: ActivePermissionProfile, + }, +} + /// Common initialization parameters shared by all `ChatWidget` constructors. pub(crate) struct ChatWidgetInit { pub(crate) config: Config, diff --git a/codex-rs/tui/src/chatwidget/permission_popups.rs b/codex-rs/tui/src/chatwidget/permission_popups.rs index 1283a9e490..b15144b0fb 100644 --- a/codex-rs/tui/src/chatwidget/permission_popups.rs +++ b/codex-rs/tui/src/chatwidget/permission_popups.rs @@ -240,18 +240,27 @@ impl ChatWidget { pub(super) fn approval_preset_actions( approval: AskForApproval, - permission_profile: PermissionProfile, - active_permission_profile: ActivePermissionProfile, + profile_update: PermissionPresetProfileUpdate, label: String, approvals_reviewer: ApprovalsReviewer, ) -> Vec { vec![Box::new(move |tx| { + let (permission_profile, active_permission_profile) = match &profile_update { + PermissionPresetProfileUpdate::Preserve => (None, None), + PermissionPresetProfileUpdate::Replace { + permission_profile, + active_permission_profile, + } => ( + Some(permission_profile.clone()), + Some(active_permission_profile.clone()), + ), + }; tx.send(AppEvent::CodexOp(AppCommand::override_turn_context( /*cwd*/ None, Some(approval), Some(approvals_reviewer), - Some(permission_profile.clone()), - Some(active_permission_profile.clone()), + permission_profile, + active_permission_profile, /*windows_sandbox_level*/ None, /*model*/ None, /*effort*/ None, @@ -261,9 +270,15 @@ impl ChatWidget { /*personality*/ None, ))); tx.send(AppEvent::UpdateAskForApprovalPolicy(approval)); - tx.send(AppEvent::UpdateActivePermissionProfile( - active_permission_profile.clone(), - )); + if let PermissionPresetProfileUpdate::Replace { + active_permission_profile, + .. + } = &profile_update + { + tx.send(AppEvent::UpdateActivePermissionProfile( + active_permission_profile.clone(), + )); + } tx.send(AppEvent::UpdateApprovalsReviewer(approvals_reviewer)); tx.send(AppEvent::InsertHistoryCell(Box::new( history_cell::new_info_event( @@ -290,13 +305,24 @@ impl ChatWidget { profile_selection: Option, return_to_permissions: bool, ) -> Vec { + let preserve_workspace_profile = preset.id == "auto" + && Self::workspace_profile_matches_auto_mode( + self.config.permissions.permission_profile(), + self.config.cwd.as_path(), + ); let apply_actions = || { profile_selection.clone().map_or_else( || { Self::approval_preset_actions( AskForApproval::from(preset.approval), - preset.permission_profile.clone(), - preset.active_permission_profile.clone(), + if preserve_workspace_profile { + PermissionPresetProfileUpdate::Preserve + } else { + PermissionPresetProfileUpdate::Replace { + permission_profile: preset.permission_profile.clone(), + active_permission_profile: preset.active_permission_profile.clone(), + } + }, label.clone(), approvals_reviewer, ) @@ -390,20 +416,21 @@ impl ChatWidget { && current_permission_profile.network_sandbox_policy() == preset.permission_profile.network_sandbox_policy() } - "auto" => { - let file_system_policy = current_permission_profile.file_system_sandbox_policy(); - matches!( - current_permission_profile, - PermissionProfile::Managed { .. } - ) && file_system_policy.can_write_path_with_cwd(cwd, cwd) - && !file_system_policy.has_full_disk_write_access() - && current_permission_profile.network_sandbox_policy() - == preset.permission_profile.network_sandbox_policy() - } + "auto" => Self::workspace_profile_matches_auto_mode(current_permission_profile, cwd), _ => current_permission_profile == &preset.permission_profile, } } + pub(crate) fn workspace_profile_matches_auto_mode( + permission_profile: &PermissionProfile, + cwd: &std::path::Path, + ) -> bool { + let file_system_policy = permission_profile.file_system_sandbox_policy(); + matches!(permission_profile, PermissionProfile::Managed { .. }) + && file_system_policy.can_write_path_with_cwd(cwd, cwd) + && !file_system_policy.has_full_disk_write_access() + } + pub(crate) fn open_full_access_confirmation( &mut self, preset: ApprovalPreset, @@ -430,8 +457,10 @@ impl ChatWidget { || { Self::approval_preset_actions( approval, - preset.permission_profile.clone(), - preset.active_permission_profile.clone(), + PermissionPresetProfileUpdate::Replace { + permission_profile: preset.permission_profile.clone(), + active_permission_profile: preset.active_permission_profile.clone(), + }, selected_name.clone(), ApprovalsReviewer::User, ) @@ -446,8 +475,10 @@ impl ChatWidget { || { Self::approval_preset_actions( approval, - preset.permission_profile, - preset.active_permission_profile, + PermissionPresetProfileUpdate::Replace { + permission_profile: preset.permission_profile, + active_permission_profile: preset.active_permission_profile, + }, selected_name, ApprovalsReviewer::User, ) diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 24ef4d5761..c3fbed4411 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -144,7 +144,6 @@ pub(super) use codex_protocol::config_types::Personality; pub(super) use codex_protocol::config_types::SERVICE_TIER_DEFAULT_REQUEST_VALUE; pub(super) use codex_protocol::config_types::ServiceTier; pub(super) use codex_protocol::models::ActivePermissionProfile; -pub(super) use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_WORKSPACE; pub(super) use codex_protocol::models::FileSystemPermissions; pub(super) use codex_protocol::models::MessagePhase; pub(super) use codex_protocol::models::NetworkPermissions; diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index ebac595671..8e4231a900 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -1061,7 +1061,7 @@ async fn permissions_selection_can_disable_auto_review() { } #[tokio::test] -async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context() { +async fn terminal_browser_auto_review_preserves_network_enabled_workspace_profile() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { @@ -1075,9 +1075,15 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context .approval_policy .set(AskForApproval::OnRequest.to_core()) .expect("set approval policy"); + let permission_profile = PermissionProfile::workspace_write_with( + &[], + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); chat.config .permissions - .set_permission_profile(PermissionProfile::workspace_write()) + .set_permission_profile(permission_profile.clone()) .expect("set permission profile"); chat.set_approvals_reviewer(ApprovalsReviewer::User); @@ -1113,10 +1119,8 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context cwd: None, approval_policy: Some(AskForApproval::OnRequest), approvals_reviewer: Some(ApprovalsReviewer::AutoReview), - permission_profile: Some(PermissionProfile::workspace_write()), - active_permission_profile: Some(ActivePermissionProfile::new( - BUILT_IN_PERMISSION_PROFILE_WORKSPACE, - )), + permission_profile: None, + active_permission_profile: None, windows_sandbox_level: None, model: None, effort: None, @@ -1127,18 +1131,14 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context } ); - let active_permission_profile_update = std::iter::from_fn(|| rx.try_recv().ok()) - .find_map(|event| match event { - AppEvent::UpdateActivePermissionProfile(active_permission_profile) => { - Some(active_permission_profile) - } - _ => None, - }) - .expect("expected UpdateActivePermissionProfile event"); - assert_eq!( - active_permission_profile_update, - ActivePermissionProfile::new(BUILT_IN_PERMISSION_PROFILE_WORKSPACE) + chat.config.permissions.permission_profile(), + &permission_profile + ); + assert!( + std::iter::from_fn(|| rx.try_recv().ok()) + .all(|event| !matches!(event, AppEvent::UpdateActivePermissionProfile(_))), + "reviewer-only changes must not replace the effective workspace profile" ); } diff --git a/codex-rs/tui/src/chatwidget/windows_sandbox_prompts.rs b/codex-rs/tui/src/chatwidget/windows_sandbox_prompts.rs index 89ea3a9bc5..69c06f83ce 100644 --- a/codex-rs/tui/src/chatwidget/windows_sandbox_prompts.rs +++ b/codex-rs/tui/src/chatwidget/windows_sandbox_prompts.rs @@ -153,8 +153,10 @@ impl ChatWidget { ) { accept_actions.extend(Self::approval_preset_actions( approval, - permission_profile, - active_permission_profile, + PermissionPresetProfileUpdate::Replace { + permission_profile, + active_permission_profile, + }, mode_label.to_string(), ApprovalsReviewer::User, )); @@ -173,8 +175,10 @@ impl ChatWidget { { accept_and_remember_actions.extend(Self::approval_preset_actions( approval, - permission_profile, - active_permission_profile, + PermissionPresetProfileUpdate::Replace { + permission_profile, + active_permission_profile, + }, mode_label.to_string(), ApprovalsReviewer::User, )); diff --git a/codex-rs/tui/src/terminal_browser/network.rs b/codex-rs/tui/src/terminal_browser/network.rs index 7e3757831d..73bbdcc44f 100644 --- a/codex-rs/tui/src/terminal_browser/network.rs +++ b/codex-rs/tui/src/terminal_browser/network.rs @@ -44,15 +44,6 @@ impl TerminalBrowserNetworkAvailability { } } - pub(crate) fn dynamic_tools_supported(config: &Config) -> bool { - config.permissions.network_sandbox_policy().is_enabled() - && config - .permissions - .network - .as_ref() - .is_none_or(|network| !network.mitm_enabled()) - } - pub(crate) fn unavailable_message(self) -> Option<&'static str> { match self { Self::Direct | Self::ManagedProxy { .. } => None, diff --git a/codex-rs/tui/src/terminal_browser/network_tests.rs b/codex-rs/tui/src/terminal_browser/network_tests.rs index bd35001d69..0fedaaa41c 100644 --- a/codex-rs/tui/src/terminal_browser/network_tests.rs +++ b/codex-rs/tui/src/terminal_browser/network_tests.rs @@ -53,18 +53,12 @@ async fn direct_network_does_not_require_runtime_proxy_details() { ), TerminalBrowserNetworkAvailability::Direct ); - assert!(TerminalBrowserNetworkAvailability::dynamic_tools_supported( - &config - )); } #[tokio::test] async fn managed_network_requires_a_non_mitm_loopback_runtime_proxy() { let config = terminal_browser_config(/*managed_network*/ true).await; assert!(config.permissions.network.is_some()); - assert!(TerminalBrowserNetworkAvailability::dynamic_tools_supported( - &config - )); assert_eq!( TerminalBrowserNetworkAvailability::from_config_and_runtime( diff --git a/codex-rs/tui/src/terminal_browser/tools.rs b/codex-rs/tui/src/terminal_browser/tools.rs index 25304ec024..9abf543243 100644 --- a/codex-rs/tui/src/terminal_browser/tools.rs +++ b/codex-rs/tui/src/terminal_browser/tools.rs @@ -13,7 +13,8 @@ pub(crate) const TERMINAL_BROWSER_NAMESPACE: &str = "terminal_browser"; pub(crate) fn dynamic_tool_specs() -> Vec { vec![DynamicToolSpec::Namespace(DynamicToolNamespaceSpec { name: TERMINAL_BROWSER_NAMESPACE.to_string(), - description: "Control a browser rendered inside the terminal.".to_string(), + description: "Control the browser rendered inside the terminal. Use these tools when the user asks for the terminal browser; do not substitute shell text browsers." + .to_string(), tools: vec![ tool( "open",