From cb9ef06eccb5ccf514cc07eb77f93ef21f74b89a Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 1 Apr 2026 22:00:27 -0600 Subject: [PATCH] Fix TUI app-server permission profile conversions (#16284) Addresses #16283 Problem: TUI app-server permission approvals could drop filesystem grants because request and response payloads were round-tripped through mismatched camelCase and snake_case JSON shapes. Solution: Replace the lossy JSON round-trips with typed app-server/core permission conversions so requested and granted permission profiles, including filesystem paths and scope, are preserved end to end. --- codex-rs/tui/src/app.rs | 93 ++++-- codex-rs/tui/src/app/app_server_requests.rs | 59 +++- .../src/app_server_approval_conversions.rs | 101 ++++++ codex-rs/tui/src/chatwidget.rs | 20 +- ...pproval_history_decision_aborted_long.snap | 3 +- ...al_history_decision_aborted_multiline.snap | 3 +- ...roval_history_decision_approved_short.snap | 3 +- ...dget__tests__exec_approval_modal_exec.snap | 3 +- codex-rs/tui/src/chatwidget/tests.rs | 10 + .../src/chatwidget/tests/approval_requests.rs | 311 ++++++++++++++++++ ...pproval_history_decision_aborted_long.snap | 6 + ...al_history_decision_aborted_multiline.snap | 5 + ...roval_history_decision_approved_short.snap | 5 + ...al_requests__exec_approval_modal_exec.snap | 39 +++ codex-rs/tui/src/lib.rs | 1 + 15 files changed, 602 insertions(+), 60 deletions(-) create mode 100644 codex-rs/tui/src/app_server_approval_conversions.rs create mode 100644 codex-rs/tui/src/chatwidget/tests/approval_requests.rs create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_long.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_multiline.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_approved_short.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_modal_exec.snap diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 75286b8d6..7e1a0abdd 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -8,6 +8,7 @@ use crate::app_event::RealtimeAudioDeviceKind; #[cfg(target_os = "windows")] use crate::app_event::WindowsSandboxEnableMode; use crate::app_event_sender::AppEventSender; +use crate::app_server_approval_conversions::network_approval_context_to_core; use crate::app_server_session::AppServerSession; use crate::app_server_session::AppServerStartedThread; use crate::app_server_session::ThreadSessionState; @@ -235,16 +236,6 @@ fn collab_receiver_thread_ids(notification: &ServerNotification) -> Option<&[Str } } -fn convert_via_json(value: T) -> Option -where - T: serde::Serialize, - U: serde::de::DeserializeOwned, -{ - serde_json::to_value(value) - .ok() - .and_then(|value| serde_json::from_value(value).ok()) -} - fn default_exec_approval_decisions( network_approval_context: Option<&codex_protocol::protocol::NetworkApprovalContext>, proposed_execpolicy_amendment: Option<&codex_protocol::approvals::ExecPolicyAmendment>, @@ -1702,11 +1693,8 @@ impl App { let network_approval_context = params .network_approval_context .clone() - .and_then(convert_via_json); - let additional_permissions = params - .additional_permissions - .clone() - .and_then(convert_via_json); + .map(network_approval_context_to_core); + let additional_permissions = params.additional_permissions.clone().map(Into::into); let proposed_execpolicy_amendment = params .proposed_execpolicy_amendment .clone() @@ -1801,10 +1789,7 @@ impl App { thread_label, call_id: params.item_id.clone(), reason: params.reason.clone(), - permissions: serde_json::from_value( - serde_json::to_value(¶ms.permissions).ok()?, - ) - .ok()?, + permissions: params.permissions.clone().into(), }), ), _ => None, @@ -6164,6 +6149,7 @@ mod tests { use crate::multi_agents::AgentPickerThreadEntry; use assert_matches::assert_matches; + use codex_app_server_protocol::AdditionalFileSystemPermissions; use codex_app_server_protocol::AdditionalNetworkPermissions; use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::AgentMessageDeltaNotification; @@ -6185,6 +6171,7 @@ mod tests { use codex_app_server_protocol::NetworkPolicyAmendment as AppServerNetworkPolicyAmendment; use codex_app_server_protocol::NetworkPolicyRuleAction as AppServerNetworkPolicyRuleAction; use codex_app_server_protocol::NonSteerableTurnKind as AppServerNonSteerableTurnKind; + use codex_app_server_protocol::PermissionsRequestApprovalParams; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::ServerRequest; @@ -6211,6 +6198,7 @@ mod tests { use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Settings; use codex_protocol::mcp::Tool; + use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; use codex_protocol::openai_models::ModelAvailabilityNux; @@ -6226,8 +6214,10 @@ mod tests { use codex_protocol::protocol::SessionConfiguredEvent; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::TurnContextItem; + use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::user_input::TextElement; use codex_protocol::user_input::UserInput; + use codex_utils_absolute_path::AbsolutePathBuf; use crossterm::event::KeyModifiers; use insta::assert_snapshot; use pretty_assertions::assert_eq; @@ -6238,6 +6228,10 @@ mod tests { use tempfile::tempdir; use tokio::time; + fn test_absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(PathBuf::from(path)).expect("absolute test path") + } + #[test] fn normalize_harness_overrides_resolves_relative_add_dirs() -> Result<()> { let temp_dir = tempdir()?; @@ -8314,13 +8308,16 @@ guardian_approval = true }; params.network_approval_context = Some(AppServerNetworkApprovalContext { host: "example.com".to_string(), - protocol: AppServerNetworkApprovalProtocol::Https, + protocol: AppServerNetworkApprovalProtocol::Socks5Tcp, }); params.additional_permissions = Some(AdditionalPermissionProfile { network: Some(AdditionalNetworkPermissions { enabled: Some(true), }), - file_system: None, + file_system: Some(AdditionalFileSystemPermissions { + read: Some(vec![test_absolute_path("/tmp/read-only")]), + write: Some(vec![test_absolute_path("/tmp/write")]), + }), }); params.proposed_network_policy_amendments = Some(vec![AppServerNetworkPolicyAmendment { host: "example.com".to_string(), @@ -8343,7 +8340,7 @@ guardian_approval = true network_approval_context, Some(NetworkApprovalContext { host: "example.com".to_string(), - protocol: NetworkApprovalProtocol::Https, + protocol: NetworkApprovalProtocol::Socks5Tcp, }) ); assert_eq!( @@ -8352,7 +8349,10 @@ guardian_approval = true network: Some(NetworkPermissions { enabled: Some(true), }), - file_system: None, + file_system: Some(FileSystemPermissions { + read: Some(vec![test_absolute_path("/tmp/read-only")]), + write: Some(vec![test_absolute_path("/tmp/write")]), + }), }) ); assert_eq!( @@ -8406,6 +8406,53 @@ guardian_approval = true ); } + #[tokio::test] + async fn inactive_thread_permissions_approval_preserves_file_system_permissions() { + let app = make_test_app().await; + let thread_id = ThreadId::new(); + let request = ServerRequest::PermissionsRequestApproval { + request_id: AppServerRequestId::Integer(7), + params: PermissionsRequestApprovalParams { + thread_id: thread_id.to_string(), + turn_id: "turn-approval".to_string(), + item_id: "call-approval".to_string(), + reason: Some("Need access to .git".to_string()), + permissions: codex_app_server_protocol::RequestPermissionProfile { + network: Some(AdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: Some(AdditionalFileSystemPermissions { + read: Some(vec![test_absolute_path("/tmp/read-only")]), + write: Some(vec![test_absolute_path("/tmp/write")]), + }), + }, + }, + }; + + let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Permissions { + permissions, + .. + })) = app + .interactive_request_for_thread_request(thread_id, &request) + .await + else { + panic!("expected permissions approval request"); + }; + + assert_eq!( + permissions, + RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![test_absolute_path("/tmp/read-only")]), + write: Some(vec![test_absolute_path("/tmp/write")]), + }), + } + ); + } + #[tokio::test] async fn inactive_thread_approval_badge_clears_after_turn_completion_notification() -> Result<()> { diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 45db3857a..03449b902 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -2,10 +2,10 @@ use std::collections::HashMap; use crate::app_command::AppCommand; use crate::app_command::AppCommandView; +use crate::app_server_approval_conversions::granted_permission_profile_from_request; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeRequestApprovalResponse; -use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::McpServerElicitationAction; use codex_app_server_protocol::McpServerElicitationRequestResponse; use codex_app_server_protocol::PermissionsRequestApprovalResponse; @@ -153,14 +153,9 @@ impl PendingAppServerRequests { Ok::(AppServerRequestResolution { request_id, result: serde_json::to_value(PermissionsRequestApprovalResponse { - permissions: serde_json::from_value::( - serde_json::to_value(&response.permissions).map_err(|err| { - format!("failed to encode granted permissions: {err}") - })?, - ) - .map_err(|err| { - format!("failed to decode granted permissions for app-server: {err}") - })?, + permissions: granted_permission_profile_from_request( + response.permissions.clone(), + ), scope: response.scope.into(), }) .map_err(|err| { @@ -276,6 +271,8 @@ fn file_change_decision(decision: &ReviewDecision) -> Result(permissions.result) .expect("permissions response should decode"), PermissionsRequestApprovalResponse { - permissions: serde_json::from_value(json!({ - "network": { "enabled": null } - })) - .expect("valid permissions"), + permissions: codex_app_server_protocol::GrantedPermissionProfile { + network: Some(AdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: Some(AdditionalFileSystemPermissions { + read: Some(vec![absolute_path(read_path)]), + write: Some(vec![absolute_path(write_path)]), + }), + }, scope: PermissionGrantScope::Session, } ); diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs new file mode 100644 index 000000000..8d2335eb7 --- /dev/null +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -0,0 +1,101 @@ +use codex_app_server_protocol::AdditionalFileSystemPermissions; +use codex_app_server_protocol::AdditionalNetworkPermissions; +use codex_app_server_protocol::GrantedPermissionProfile; +use codex_app_server_protocol::NetworkApprovalContext as AppServerNetworkApprovalContext; +use codex_protocol::protocol::NetworkApprovalContext; +use codex_protocol::protocol::NetworkApprovalProtocol; +use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; + +pub(crate) fn network_approval_context_to_core( + value: AppServerNetworkApprovalContext, +) -> NetworkApprovalContext { + NetworkApprovalContext { + host: value.host, + protocol: match value.protocol { + codex_app_server_protocol::NetworkApprovalProtocol::Http => { + NetworkApprovalProtocol::Http + } + codex_app_server_protocol::NetworkApprovalProtocol::Https => { + NetworkApprovalProtocol::Https + } + codex_app_server_protocol::NetworkApprovalProtocol::Socks5Tcp => { + NetworkApprovalProtocol::Socks5Tcp + } + codex_app_server_protocol::NetworkApprovalProtocol::Socks5Udp => { + NetworkApprovalProtocol::Socks5Udp + } + }, + } +} + +pub(crate) fn granted_permission_profile_from_request( + value: CoreRequestPermissionProfile, +) -> GrantedPermissionProfile { + GrantedPermissionProfile { + network: value.network.map(|network| AdditionalNetworkPermissions { + enabled: network.enabled, + }), + file_system: value + .file_system + .map(|file_system| AdditionalFileSystemPermissions { + read: file_system.read, + write: file_system.write, + }), + } +} + +#[cfg(test)] +mod tests { + use super::granted_permission_profile_from_request; + use super::network_approval_context_to_core; + use codex_protocol::models::FileSystemPermissions; + use codex_protocol::models::NetworkPermissions; + use codex_protocol::protocol::NetworkApprovalContext; + use codex_protocol::protocol::NetworkApprovalProtocol; + use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + use std::path::PathBuf; + + fn absolute_path(path: &str) -> AbsolutePathBuf { + AbsolutePathBuf::try_from(PathBuf::from(path)).expect("path must be absolute") + } + + #[test] + fn converts_app_server_network_approval_context_to_core() { + assert_eq!( + network_approval_context_to_core(codex_app_server_protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_app_server_protocol::NetworkApprovalProtocol::Socks5Tcp, + }), + NetworkApprovalContext { + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Socks5Tcp, + } + ); + } + + #[test] + fn converts_request_permissions_into_granted_permissions() { + assert_eq!( + granted_permission_profile_from_request(CoreRequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![absolute_path("/tmp/read-only")]), + write: Some(vec![absolute_path("/tmp/write")]), + }), + }), + codex_app_server_protocol::GrantedPermissionProfile { + network: Some(codex_app_server_protocol::AdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: Some(codex_app_server_protocol::AdditionalFileSystemPermissions { + read: Some(vec![absolute_path("/tmp/read-only")]), + write: Some(vec![absolute_path("/tmp/write")]), + }), + } + ); + } +} diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 446d934ee..2b245428c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -43,6 +43,7 @@ use url::Url; use self::realtime::PendingSteerCompareKey; use crate::app_command::AppCommand; use crate::app_event::RealtimeAudioDeviceKind; +use crate::app_server_approval_conversions::network_approval_context_to_core; use crate::app_server_session::ThreadSessionState; #[cfg(not(target_os = "linux"))] use crate::audio_device::list_realtime_audio_device_names; @@ -1283,16 +1284,6 @@ fn thread_session_state_to_legacy_event( } } -fn convert_via_json(value: T) -> Option -where - T: serde::Serialize, - U: serde::de::DeserializeOwned, -{ - serde_json::to_value(value) - .ok() - .and_then(|value| serde_json::from_value(value).ok()) -} - fn hook_output_entry_from_notification( entry: codex_app_server_protocol::HookOutputEntry, ) -> codex_protocol::protocol::HookOutputEntry { @@ -1371,8 +1362,8 @@ fn exec_approval_request_from_params( reason: params.reason, network_approval_context: params .network_approval_context - .and_then(convert_via_json), - additional_permissions: params.additional_permissions.and_then(convert_via_json), + .map(network_approval_context_to_core), + additional_permissions: params.additional_permissions.map(Into::into), turn_id: params.turn_id, approval_id: params.approval_id, proposed_execpolicy_amendment: params @@ -1559,10 +1550,7 @@ fn request_permissions_from_params( turn_id: params.turn_id, call_id: params.item_id, reason: params.reason, - permissions: serde_json::from_value( - serde_json::to_value(params.permissions).unwrap_or(serde_json::Value::Null), - ) - .unwrap_or_default(), + permissions: params.permissions.into(), } } diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap index 44ed1f401..c9f305093 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap @@ -1,5 +1,6 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/exec_flow.rs +assertion_line: 217 expression: lines_to_single_string(&aborted_long) --- ✗ You canceled the request to run echo diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap index 5aee3b284..4280a39a3 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap @@ -1,5 +1,6 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/exec_flow.rs +assertion_line: 183 expression: lines_to_single_string(&aborted_multi) --- ✗ You canceled the request to run echo line1 ... diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap index 2f0f1412a..0a474df75 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap @@ -1,5 +1,6 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/exec_flow.rs +assertion_line: 47 expression: lines_to_single_string(&decision) --- ✔ You approved codex to run echo hello world this time diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap index 055a6292f..3086586b1 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -1,5 +1,6 @@ --- -source: tui/src/chatwidget/tests.rs +source: tui/src/chatwidget/tests/exec_flow.rs +assertion_line: 40 expression: "format!(\"{buf:?}\")" --- Buffer { diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 7925011be..e87634aea 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -20,6 +20,9 @@ pub(super) use crate::test_support::PathBufExt; pub(super) use crate::test_support::test_path_display; pub(super) use crate::tui::FrameRequester; pub(super) use assert_matches::assert_matches; +pub(super) use codex_app_server_protocol::AdditionalFileSystemPermissions as AppServerAdditionalFileSystemPermissions; +pub(super) use codex_app_server_protocol::AdditionalNetworkPermissions as AppServerAdditionalNetworkPermissions; +pub(super) use codex_app_server_protocol::AdditionalPermissionProfile as AppServerAdditionalPermissionProfile; pub(super) use codex_app_server_protocol::AppSummary; pub(super) use codex_app_server_protocol::CollabAgentState as AppServerCollabAgentState; pub(super) use codex_app_server_protocol::CollabAgentStatus as AppServerCollabAgentStatus; @@ -55,6 +58,7 @@ pub(super) use codex_app_server_protocol::McpServerStartupState; pub(super) use codex_app_server_protocol::McpServerStatusUpdatedNotification; pub(super) use codex_app_server_protocol::PatchApplyStatus as AppServerPatchApplyStatus; pub(super) use codex_app_server_protocol::PatchChangeKind; +pub(super) use codex_app_server_protocol::PermissionsRequestApprovalParams as AppServerPermissionsRequestApprovalParams; pub(super) use codex_app_server_protocol::PluginAuthPolicy; pub(super) use codex_app_server_protocol::PluginDetail; pub(super) use codex_app_server_protocol::PluginInstallPolicy; @@ -109,7 +113,10 @@ pub(super) use codex_protocol::items::AgentMessageItem; pub(super) use codex_protocol::items::PlanItem; pub(super) use codex_protocol::items::TurnItem; pub(super) use codex_protocol::items::UserMessageItem; +pub(super) use codex_protocol::models::FileSystemPermissions; pub(super) use codex_protocol::models::MessagePhase; +pub(super) use codex_protocol::models::NetworkPermissions; +pub(super) use codex_protocol::models::PermissionProfile; pub(super) use codex_protocol::openai_models::ModelPreset; pub(super) use codex_protocol::openai_models::ReasoningEffortPreset; pub(super) use codex_protocol::openai_models::default_input_modalities; @@ -175,6 +182,7 @@ pub(super) use codex_protocol::protocol::UndoCompletedEvent; pub(super) use codex_protocol::protocol::UndoStartedEvent; pub(super) use codex_protocol::protocol::ViewImageToolCallEvent; pub(super) use codex_protocol::protocol::WarningEvent; +pub(super) use codex_protocol::request_permissions::RequestPermissionProfile; pub(super) use codex_protocol::request_user_input::RequestUserInputEvent; pub(super) use codex_protocol::request_user_input::RequestUserInputQuestion; pub(super) use codex_protocol::request_user_input::RequestUserInputQuestionOption; @@ -188,6 +196,7 @@ pub(super) use codex_utils_approval_presets::builtin_approval_presets; pub(super) use crossterm::event::KeyCode; pub(super) use crossterm::event::KeyEvent; pub(super) use crossterm::event::KeyModifiers; +pub(super) use insta::assert_snapshot; #[cfg(target_os = "windows")] pub(super) use serial_test::serial; pub(super) use std::collections::BTreeMap; @@ -228,6 +237,7 @@ macro_rules! assert_chatwidget_snapshot { } mod app_server; +mod approval_requests; mod background_events; mod composer_submission; mod exec_flow; diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs new file mode 100644 index 000000000..ee686e004 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -0,0 +1,311 @@ +//! Approval-request focused tests extracted from the main chatwidget test file +//! to keep the primary module under blob-size policy limits. + +use super::*; +use pretty_assertions::assert_eq; + +#[tokio::test] +async fn exec_approval_emits_proposed_command_and_decision_history() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + // Trigger an exec approval request with a short, single-line command. + let ev = ExecApprovalRequestEvent { + call_id: "call-short".into(), + approval_id: Some("call-short".into()), + turn_id: "turn-short".into(), + command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + available_decisions: None, + parsed_cmd: vec![], + }; + chat.handle_codex_event(Event { + id: "sub-short".into(), + msg: EventMsg::ExecApprovalRequest(ev), + }); + + let proposed_cells = drain_insert_history(&mut rx); + assert!( + proposed_cells.is_empty(), + "expected approval request to render via modal without emitting history cells" + ); + + let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + chat.render(area, &mut buf); + assert_snapshot!("exec_approval_modal_exec", format!("{buf:?}")); + + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "exec_approval_history_decision_approved_short", + lines_to_single_string(&decision) + ); +} + +#[test] +fn app_server_exec_approval_request_splits_shell_wrapped_command() { + let script = r#"python3 -c 'print("Hello, world!")'"#; + let request = + exec_approval_request_from_params(AppServerCommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + approval_id: Some("approval-1".to_string()), + reason: None, + network_approval_context: None, + command: Some( + shlex::try_join(["/bin/zsh", "-lc", script]) + .expect("round-trippable shell wrapper"), + ), + cwd: Some(PathBuf::from("/tmp")), + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: None, + }); + + assert_eq!( + request.command, + vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + script.to_string(), + ] + ); +} + +#[test] +fn app_server_exec_approval_request_preserves_permissions_context() { + let read_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/read-only"))) + .expect("absolute read path"); + let write_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/write"))) + .expect("absolute write path"); + let request = + exec_approval_request_from_params(AppServerCommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + approval_id: Some("approval-1".to_string()), + reason: None, + network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_app_server_protocol::NetworkApprovalProtocol::Socks5Tcp, + }), + command: Some("ls".to_string()), + cwd: Some(PathBuf::from("/tmp")), + command_actions: None, + additional_permissions: Some(AppServerAdditionalPermissionProfile { + network: Some(AppServerAdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: Some(AppServerAdditionalFileSystemPermissions { + read: Some(vec![read_path.clone()]), + write: Some(vec![write_path.clone()]), + }), + }), + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: None, + }); + + assert_eq!( + request.network_approval_context, + Some(codex_protocol::protocol::NetworkApprovalContext { + host: "example.com".to_string(), + protocol: codex_protocol::protocol::NetworkApprovalProtocol::Socks5Tcp, + }) + ); + assert_eq!( + request.additional_permissions, + Some(PermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![read_path]), + write: Some(vec![write_path]), + }), + }) + ); +} + +#[test] +fn app_server_request_permissions_preserves_file_system_permissions() { + let read_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/read-only"))) + .expect("absolute read path"); + let write_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/write"))) + .expect("absolute write path"); + + let request = request_permissions_from_params(AppServerPermissionsRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + reason: Some("Select a workspace root".to_string()), + permissions: codex_app_server_protocol::RequestPermissionProfile { + network: Some(AppServerAdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: Some(AppServerAdditionalFileSystemPermissions { + read: Some(vec![read_path.clone()]), + write: Some(vec![write_path.clone()]), + }), + }, + }); + + assert_eq!( + request.permissions, + RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + file_system: Some(FileSystemPermissions { + read: Some(vec![read_path]), + write: Some(vec![write_path]), + }), + } + ); +} + +#[tokio::test] +async fn exec_approval_uses_approval_id_when_present() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + chat.handle_codex_event(Event { + id: "sub-short".into(), + msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + call_id: "call-parent".into(), + approval_id: Some("approval-subcommand".into()), + turn_id: "turn-short".into(), + command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + available_decisions: None, + parsed_cmd: vec![], + }), + }); + + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let mut found = false; + while let Ok(app_ev) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::ExecApproval { id, decision, .. }, + .. + } = app_ev + { + assert_eq!(id, "approval-subcommand"); + assert_matches!(decision, codex_protocol::protocol::ReviewDecision::Approved); + found = true; + break; + } + } + assert!(found, "expected ExecApproval op to be sent"); +} + +#[tokio::test] +async fn exec_approval_decision_truncates_multiline_and_long_commands() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + + let ev_multi = ExecApprovalRequestEvent { + call_id: "call-multi".into(), + approval_id: Some("call-multi".into()), + turn_id: "turn-multi".into(), + command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + available_decisions: None, + parsed_cmd: vec![], + }; + chat.handle_codex_event(Event { + id: "sub-multi".into(), + msg: EventMsg::ExecApprovalRequest(ev_multi), + }); + let proposed_multi = drain_insert_history(&mut rx); + assert!( + proposed_multi.is_empty(), + "expected multiline approval request to render via modal without emitting history cells" + ); + + let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + chat.render(area, &mut buf); + let mut saw_first_line = false; + for y in 0..area.height { + let mut row = String::new(); + for x in 0..area.width { + row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); + } + if row.contains("echo line1") { + saw_first_line = true; + break; + } + } + assert!( + saw_first_line, + "expected modal to show first line of multiline snippet" + ); + + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_multi = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (multiline)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_multiline", + lines_to_single_string(&aborted_multi) + ); + + let long = format!("echo {}", "a".repeat(200)); + let ev_long = ExecApprovalRequestEvent { + call_id: "call-long".into(), + approval_id: Some("call-long".into()), + turn_id: "turn-long".into(), + command: vec!["bash".into(), "-lc".into(), long], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: None, + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + available_decisions: None, + parsed_cmd: vec![], + }; + chat.handle_codex_event(Event { + id: "sub-long".into(), + msg: EventMsg::ExecApprovalRequest(ev_long), + }); + let proposed_long = drain_insert_history(&mut rx); + assert!( + proposed_long.is_empty(), + "expected long approval request to avoid emitting history cells before decision" + ); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_long = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (long)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_long", + lines_to_single_string(&aborted_long) + ); +} diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_long.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_long.snap new file mode 100644 index 000000000..44ed1f401 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_long.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_long) +--- +✗ You canceled the request to run echo + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_multiline.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_multiline.snap new file mode 100644 index 000000000..5aee3b284 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_aborted_multiline.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_multi) +--- +✗ You canceled the request to run echo line1 ... diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_approved_short.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_approved_short.snap new file mode 100644 index 000000000..2f0f1412a --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_history_decision_approved_short.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex to run echo hello world this time diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_modal_exec.snap new file mode 100644 index 000000000..055a6292f --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__exec_approval_modal_exec.snap @@ -0,0 +1,39 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: "format!(\"{buf:?}\")" +--- +Buffer { + area: Rect { x: 0, y: 0, width: 80, height: 13 }, + content: [ + " ", + " ", + " Would you like to run the following command? ", + " ", + " Reason: this is a test reason such as one that would be produced by the ", + " model ", + " ", + " $ echo hello world ", + " ", + "› 1. Yes, proceed (y) ", + " 2. No, and tell Codex what to do differently (esc) ", + " ", + " Press enter to confirm or esc to cancel ", + ], + styles: [ + x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD, + x: 46, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 10, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: ITALIC, + x: 73, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: ITALIC, + x: 7, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 4, y: 7, fg: Rgb(137, 180, 250), bg: Reset, underline: Reset, modifier: NONE, + x: 8, y: 7, fg: Rgb(205, 214, 244), bg: Reset, underline: Reset, modifier: NONE, + x: 20, y: 7, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 0, y: 9, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD, + x: 21, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 48, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 51, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + ] +} diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index df52f28fa..6cf864751 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -77,6 +77,7 @@ mod app_backtrack; mod app_command; mod app_event; mod app_event_sender; +mod app_server_approval_conversions; mod app_server_session; mod ascii_animation; #[cfg(not(target_os = "linux"))]