mirror of
https://github.com/openai/codex.git
synced 2026-05-04 03:16:31 +00:00
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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 ...
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
311
codex-rs/tui/src/chatwidget/tests/approval_requests.rs
Normal file
311
codex-rs/tui/src/chatwidget/tests/approval_requests.rs
Normal file
@@ -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)
|
||||
);
|
||||
}
|
||||
@@ -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...
|
||||
@@ -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 ...
|
||||
@@ -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
|
||||
@@ -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,
|
||||
]
|
||||
}
|
||||
Reference in New Issue
Block a user