From 5a02962519d1b373d300f72eee6b6ab0010d7a77 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Thu, 14 May 2026 14:33:54 -0300 Subject: [PATCH 1/6] fix(tui): render network approval history by target (#22229) ## Why Network approval prompts are rendered without a command string on the app-server path. After the user approves one of those prompts, the TUI history cell previously fell back to command-oriented copy and produced malformed lines such as: ```text You approved codex to run every time this session ``` That hid the network target the user actually approved and left a visibly broken transcript entry. ## What changed - Preserve the approval subject as either a command or a network target when recording TUI approval decisions. - Render target-aware history copy for network approval outcomes: - approve once - approve for the current session - cancel - Include the approval protocol and preserve the managed-proxy `network-access` target when present, including non-default ports such as `https://example.com:8443`. - Fall back to formatting the network approval context as `protocol://host` when no generated target command is available. - Keep ordinary command approval history, Guardian approval history, and persisted network-rule history behavior unchanged. - Add focused regression coverage and snapshots for the three network-history cases. ## How to Test 1. Start Codex in a flow that triggers a network approval prompt. 2. Approve network access only for the current conversation. 3. Confirm the transcript records the approved network target, for example: - `You approved codex network access to https://example.com:8443 every time this session` 4. Trigger the prompt again and verify the one-time approval and cancel paths also record target-specific history text instead of an empty command gap. Targeted automated coverage: - `cargo test -p codex-tui network_exec_approval_history` ## Additional verification - `cargo insta pending-snapshots` - `git diff --check` - `just fix -p codex-tui` - `just argument-comment-lint` ## Known unrelated local test noise A full `cargo test -p codex-tui` run still hits a pre-existing stack overflow outside this change: - `tests::fork_last_filters_latest_session_by_cwd_unless_show_all` aborts with a stack overflow --- .../tui/src/bottom_pane/approval_overlay.rs | 133 +++++++++- .../src/chatwidget/tests/approval_requests.rs | 123 +++++++++ ...pproval_history_canceled_host_request.snap | 6 + ...roval_history_one_time_host_allowance.snap | 5 + ...proval_history_session_host_allowance.snap | 6 + codex-rs/tui/src/chatwidget/tool_requests.rs | 6 +- codex-rs/tui/src/history_cell.rs | 239 +++++++++++++----- codex-rs/tui/src/pager_overlay.rs | 2 +- 8 files changed, 449 insertions(+), 71 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap create mode 100644 codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index aece933ff1..22d9f842f1 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -47,6 +47,7 @@ use codex_app_server_protocol::FileSystemSandboxEntry; use codex_app_server_protocol::FileSystemSpecialPath; use codex_app_server_protocol::McpServerElicitationAction; use codex_app_server_protocol::NetworkApprovalContext; +use codex_app_server_protocol::NetworkApprovalProtocol; use codex_app_server_protocol::NetworkPolicyRuleAction; use codex_app_server_protocol::RequestId; use codex_features::Features; @@ -354,8 +355,25 @@ impl ApprovalOverlay { return; }; if request.thread_label().is_none() { + let subject = match request { + ApprovalRequest::Exec { + network_approval_context: Some(network_approval_context), + .. + } => history_cell::ApprovalDecisionSubject::NetworkAccess { + target: network_approval_target(network_approval_context, command), + }, + _ => { + if let Some(target) = network_approval_command_target(command) { + history_cell::ApprovalDecisionSubject::NetworkAccess { + target: target.to_string(), + } + } else { + history_cell::ApprovalDecisionSubject::Command(command.to_vec()) + } + } + }; let cell = history_cell::new_approval_decision_cell( - command.to_vec(), + subject, command_decision_to_review_decision(&decision), history_cell::ApprovalDecisionActor::User, ); @@ -623,6 +641,35 @@ fn approval_footer_hint( Line::from(spans) } +fn network_approval_target( + network_approval_context: &NetworkApprovalContext, + command: &[String], +) -> String { + if let Some(target) = network_approval_command_target(command) { + return target.to_string(); + } + + let scheme = match network_approval_context.protocol { + NetworkApprovalProtocol::Http => "http", + NetworkApprovalProtocol::Https => "https", + NetworkApprovalProtocol::Socks5Tcp => "socks5-tcp", + NetworkApprovalProtocol::Socks5Udp => "socks5-udp", + }; + format!("{scheme}://{}", network_approval_context.host) +} + +fn network_approval_command_target(command: &[String]) -> Option<&str> { + match command { + [program, target] if program == "network-access" && !target.is_empty() => { + Some(target.as_str()) + } + [command] => command + .strip_prefix("network-access ") + .filter(|target| !target.is_empty()), + _ => None, + } +} + fn build_header(request: &ApprovalRequest) -> Box { match request { ApprovalRequest::Exec { @@ -1102,6 +1149,21 @@ mod tests { .join("\n") } + fn render_history_cell_lines( + cell: &dyn crate::history_cell::HistoryCell, + width: u16, + ) -> Vec { + cell.display_lines(width) + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect() + } + fn normalize_snapshot_paths(rendered: String) -> String { [ (absolute_path("/tmp/readme.txt"), "/tmp/readme.txt"), @@ -2111,7 +2173,7 @@ mod tests { "git add tui/src/render/mod.rs tui/src/render/renderable.rs".into(), ]; let cell = history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), ReviewDecision::Approved, history_cell::ApprovalDecisionActor::User, ); @@ -2134,6 +2196,73 @@ mod tests { assert_eq!(rendered, expected); } + #[test] + fn exec_history_cell_does_not_render_blank_action_for_empty_command() { + let approved = history_cell::new_approval_decision_cell( + history_cell::ApprovalDecisionSubject::Command(Vec::new()), + ReviewDecision::Approved, + history_cell::ApprovalDecisionActor::User, + ); + assert_eq!( + render_history_cell_lines(approved.as_ref(), /*width*/ 80), + vec!["✔ You approved this request this time".to_string()] + ); + + let approved_for_session = history_cell::new_approval_decision_cell( + history_cell::ApprovalDecisionSubject::Command(Vec::new()), + ReviewDecision::ApprovedForSession, + history_cell::ApprovalDecisionActor::User, + ); + assert_eq!( + render_history_cell_lines(approved_for_session.as_ref(), /*width*/ 80), + vec!["✔ You approved this request every time this session".to_string()] + ); + } + + #[test] + fn network_access_command_history_uses_target_without_structured_context() { + let (tx_raw, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = make_overlay( + ApprovalRequest::Exec { + thread_id: ThreadId::new(), + thread_label: None, + id: "test".into(), + command: vec![ + "network-access".to_string(), + "https://example.com:8443".to_string(), + ], + reason: None, + available_decisions: vec![ + CommandExecutionApprovalDecision::Accept, + CommandExecutionApprovalDecision::Cancel, + ], + network_approval_context: None, + additional_permissions: None, + }, + tx, + Features::with_defaults(), + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let mut decision = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + decision = Some(cell); + break; + } + } + let decision = decision.expect("expected decision cell in history"); + assert_eq!( + render_history_cell_lines(decision.as_ref(), /*width*/ 80), + vec![ + "✔ You approved codex network access to https://example.com:8443 this time" + .to_string(), + ] + ); + } + #[test] fn esc_cancels_mcp_elicitation() { let (tx_raw, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 85c8fc6c03..d732177ce4 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -145,6 +145,129 @@ fn app_server_exec_approval_request_preserves_permissions_context() { ); } +#[tokio::test] +async fn network_exec_approval_history_describes_session_host_allowance() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + 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(), + started_at_ms: 0, + 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::Https, + }), + command: Some("network-access https://example.com:8443".to_string()), + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::AcceptForSession, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE)); + + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "network_exec_approval_history_session_host_allowance", + lines_to_single_string(&decision) + ); +} + +#[tokio::test] +async fn network_exec_approval_history_describes_one_time_host_allowance() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + 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(), + started_at_ms: 0, + 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::Http, + }), + command: None, + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + 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!( + "network_exec_approval_history_one_time_host_allowance", + lines_to_single_string(&decision) + ); +} + +#[tokio::test] +async fn network_exec_approval_history_describes_canceled_host_request() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + 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(), + started_at_ms: 0, + 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("network-access socks5-tcp://example.com:1080".to_string()), + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: Some(vec![ + codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, + codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, + ]), + }, + &test_path_buf("/tmp").abs(), + ); + + handle_exec_approval_request(&mut chat, "sub-network", request); + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "network_exec_approval_history_canceled_host_request", + lines_to_single_string(&decision) + ); +} + #[test] fn app_server_request_permissions_preserves_file_system_permissions() { let read_path = AbsolutePathBuf::try_from(PathBuf::from(test_path_display("/tmp/read-only"))) diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap new file mode 100644 index 0000000000..3d09cd7087 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_canceled_host_request.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✗ You canceled the request for codex network access to + socks5-tcp://example.com:1080 diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap new file mode 100644 index 0000000000..4b56b187aa --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_one_time_host_allowance.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex network access to http://example.com this time diff --git a/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap new file mode 100644 index 0000000000..e5197e9e71 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/tests/snapshots/codex_tui__chatwidget__tests__approval_requests__network_exec_approval_history_session_host_allowance.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests/approval_requests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex network access to https://example.com:8443 every time this + session diff --git a/codex-rs/tui/src/chatwidget/tool_requests.rs b/codex-rs/tui/src/chatwidget/tool_requests.rs index 7ec567325b..de8fe6a704 100644 --- a/codex-rs/tui/src/chatwidget/tool_requests.rs +++ b/codex-rs/tui/src/chatwidget/tool_requests.rs @@ -149,7 +149,7 @@ impl ChatWidget { if ev.status == GuardianAssessmentStatus::Approved { let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::Approved, history_cell::ApprovalDecisionActor::Guardian, ) @@ -169,7 +169,7 @@ impl ChatWidget { if ev.status == GuardianAssessmentStatus::TimedOut { let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::TimedOut, history_cell::ApprovalDecisionActor::Guardian, ) @@ -213,7 +213,7 @@ impl ChatWidget { self.review.recent_auto_review_denials.push(ev.clone()); let cell = if let Some(command) = guardian_command(&ev.action) { history_cell::new_approval_decision_cell( - command, + history_cell::ApprovalDecisionSubject::Command(command), crate::history_cell::ReviewDecision::Denied, history_cell::ApprovalDecisionActor::Guardian, ) diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 4521fe9a7b..7b97151edb 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -1089,6 +1089,11 @@ fn exec_snippet(command: &[String]) -> String { truncate_exec_snippet(&full_cmd) } +fn non_empty_exec_snippet(command: &[String]) -> Option { + let snippet = exec_snippet(command); + (!snippet.is_empty()).then_some(snippet) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum ReviewDecision { Approved, @@ -1104,8 +1109,14 @@ pub(crate) enum ReviewDecision { Abort, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ApprovalDecisionSubject { + Command(Vec), + NetworkAccess { target: String }, +} + pub fn new_approval_decision_cell( - command: Vec, + subject: ApprovalDecisionSubject, decision: ReviewDecision, actor: ApprovalDecisionActor, ) -> Box { @@ -1113,19 +1124,37 @@ pub fn new_approval_decision_cell( use codex_protocol::approvals::NetworkPolicyRuleAction; let (symbol, summary): (Span<'static>, Vec>) = match decision { - Approved => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + Approved => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "approved".bold(), + " codex to run ".into(), + Span::from(snippet).dim(), + " this time".bold(), + ] + } else { + vec![ + actor.subject().into(), + "approved".bold(), + " this request".into(), + " this time".bold(), + ] + }; + ("✔ ".green(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✔ ".green(), vec![ actor.subject().into(), "approved".bold(), - " codex to run ".into(), - snippet, + " codex network access to ".into(), + Span::from(target).dim(), " this time".bold(), ], - ) - } + ), + }, ApprovedExecpolicyAmendment { proposed_execpolicy_amendment, } => { @@ -1140,84 +1169,164 @@ pub fn new_approval_decision_cell( ], ) } - ApprovedForSession => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ApprovedForSession => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "approved".bold(), + " codex to run ".into(), + Span::from(snippet).dim(), + " every time this session".bold(), + ] + } else { + vec![ + actor.subject().into(), + "approved".bold(), + " this request".into(), + " every time this session".bold(), + ] + }; + ("✔ ".green(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✔ ".green(), vec![ actor.subject().into(), "approved".bold(), - " codex to run ".into(), - snippet, - " every time this session".bold(), - ], - ) - } - NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => ( - "✔ ".green(), - vec![ - actor.subject().into(), - "persisted".bold(), - " Codex network access to ".into(), - Span::from(network_policy_amendment.host).dim(), - ], - ), - NetworkPolicyRuleAction::Deny => ( - "✗ ".red(), - vec![ - actor.subject().into(), - "denied".bold(), " codex network access to ".into(), - Span::from(network_policy_amendment.host).dim(), - " and saved that rule".into(), + Span::from(target).dim(), + " every time this session".bold(), ], ), }, - Denied => { - let snippet = Span::from(exec_snippet(&command)).dim(); - let summary = match actor { - ApprovalDecisionActor::User => vec![ + NetworkPolicyAmendment { + network_policy_amendment, + } => { + let target = match subject { + ApprovalDecisionSubject::NetworkAccess { target } => target, + ApprovalDecisionSubject::Command(_) => network_policy_amendment.host, + }; + match network_policy_amendment.action { + NetworkPolicyRuleAction::Allow => ( + "✔ ".green(), + vec![ + actor.subject().into(), + "persisted".bold(), + " Codex network access to ".into(), + Span::from(target).dim(), + ], + ), + NetworkPolicyRuleAction::Deny => ( + "✗ ".red(), + vec![ + actor.subject().into(), + "denied".bold(), + " codex network access to ".into(), + Span::from(target).dim(), + " and saved that rule".into(), + ], + ), + } + } + Denied => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + let snippet = Span::from(snippet).dim(); + match actor { + ApprovalDecisionActor::User => vec![ + actor.subject().into(), + "did not approve".bold(), + " codex to run ".into(), + snippet, + ], + ApprovalDecisionActor::Guardian => vec![ + "Request ".into(), + "denied".bold(), + " for codex to run ".into(), + snippet, + ], + } + } else { + match actor { + ApprovalDecisionActor::User => vec![ + actor.subject().into(), + "did not approve".bold(), + " this request".into(), + ], + ApprovalDecisionActor::Guardian => { + vec!["Request ".into(), "denied".bold()] + } + } + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( + "✗ ".red(), + vec![ actor.subject().into(), "did not approve".bold(), - " codex to run ".into(), - snippet, + " codex network access to ".into(), + Span::from(target).dim(), ], - ApprovalDecisionActor::Guardian => vec![ - "Request ".into(), - "denied".bold(), - " for codex to run ".into(), - snippet, - ], - }; - ("✗ ".red(), summary) - } - TimedOut => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ), + }, + TimedOut => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + "Review ".into(), + "timed out".bold(), + " before codex could run ".into(), + Span::from(snippet).dim(), + ] + } else { + vec![ + "Review ".into(), + "timed out".bold(), + " before this request could be approved".into(), + ] + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✗ ".red(), vec![ "Review ".into(), "timed out".bold(), - " before codex could run ".into(), - snippet, + " before codex could access ".into(), + Span::from(target).dim(), ], - ) - } - Abort => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( + ), + }, + Abort => match subject { + ApprovalDecisionSubject::Command(command) => { + let summary = if let Some(snippet) = non_empty_exec_snippet(&command) { + vec![ + actor.subject().into(), + "canceled".bold(), + " the request to run ".into(), + Span::from(snippet).dim(), + ] + } else { + vec![ + actor.subject().into(), + "canceled".bold(), + " this request".into(), + ] + }; + ("✗ ".red(), summary) + } + ApprovalDecisionSubject::NetworkAccess { target } => ( "✗ ".red(), vec![ actor.subject().into(), "canceled".bold(), - " the request to run ".into(), - snippet, + " the request for codex network access to ".into(), + Span::from(target).dim(), ], - ) - } + ), + }, }; Box::new(PrefixedWrappedHistoryCell::new( diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index be8629542e..c301700eaa 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -1133,7 +1133,7 @@ mod tests { cells.push(apply_begin_cell); let apply_end_cell: Arc = history_cell::new_approval_decision_cell( - vec!["ls".into()], + history_cell::ApprovalDecisionSubject::Command(vec!["ls".into()]), ReviewDecision::Approved, history_cell::ApprovalDecisionActor::User, ) From f13e21ef432cdb61c8c7653890b1316ac84ebea2 Mon Sep 17 00:00:00 2001 From: Rajeev Nayak Date: Thu, 14 May 2026 13:45:49 -0400 Subject: [PATCH 2/6] Prefer the model list fetched from the backend for SIWC users (#22547) ## Summary - For SIWC users, update the model list merging logic to prefer the model list fetched from the backend over the bundled model list (this is needed for special cases where users have a more limited set of models they're allowed to use) - Add or update tests covering the revised cache behavior ## Testing - Added/updated unit tests in `codex-rs/models-manager/src/manager_tests.rs` - Not run (not requested) --- .../app-server/tests/suite/v2/model_list.rs | 103 ++++++++++++ codex-rs/models-manager/src/manager.rs | 19 +++ codex-rs/models-manager/src/manager_tests.rs | 154 ++++++++++++++++++ 3 files changed, 276 insertions(+) diff --git a/codex-rs/app-server/tests/suite/v2/model_list.rs b/codex-rs/app-server/tests/suite/v2/model_list.rs index e2039d333a..d22bdd2a18 100644 --- a/codex-rs/app-server/tests/suite/v2/model_list.rs +++ b/codex-rs/app-server/tests/suite/v2/model_list.rs @@ -1,8 +1,10 @@ use std::time::Duration; use anyhow::Result; +use app_test_support::ChatGptAuthFixture; use app_test_support::McpProcess; use app_test_support::to_response; +use app_test_support::write_chatgpt_auth; use app_test_support::write_models_cache; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; @@ -13,10 +15,16 @@ use codex_app_server_protocol::ModelServiceTier; use codex_app_server_protocol::ModelUpgradeInfo; use codex_app_server_protocol::ReasoningEffortOption; use codex_app_server_protocol::RequestId; +use codex_config::types::AuthCredentialsStoreMode; +use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::openai_models::ModelsResponse; +use core_test_support::responses::mount_models_once; use pretty_assertions::assert_eq; +use serde_json::json; use tempfile::TempDir; use tokio::time::timeout; +use wiremock::MockServer; const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); const INVALID_REQUEST_ERROR_CODE: i64 = -32600; @@ -148,6 +156,101 @@ async fn list_models_includes_hidden_models() -> Result<()> { Ok(()) } +#[tokio::test] +async fn list_models_uses_chatgpt_remote_catalog_as_source_of_truth() -> Result<()> { + let server = MockServer::start().await; + let remote_model: ModelInfo = serde_json::from_value(json!({ + "slug": "chatgpt-remote-only", + "display_name": "ChatGPT Remote Only", + "description": "Remote-only model for app-server model/list coverage", + "default_reasoning_level": "medium", + "supported_reasoning_levels": [ + {"effort": "low", "description": "low"}, + {"effort": "medium", "description": "medium"} + ], + "shell_type": "shell_command", + "visibility": "list", + "minimal_client_version": [0, 1, 0], + "supported_in_api": true, + "priority": 0, + "upgrade": null, + "base_instructions": "base instructions", + "supports_reasoning_summaries": false, + "support_verbosity": false, + "default_verbosity": null, + "apply_patch_tool_type": null, + "truncation_policy": {"mode": "bytes", "limit": 10_000}, + "supports_parallel_tool_calls": false, + "supports_image_detail_original": false, + "context_window": 272_000, + "max_context_window": 272_000, + "experimental_supported_tools": [], + }))?; + let models_mock = mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model.clone()], + }, + ) + .await; + + let codex_home = TempDir::new()?; + let server_uri = server.uri(); + std::fs::write( + codex_home.path().join("config.toml"), + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" +openai_base_url = "{server_uri}/v1" +"# + ), + )?; + write_chatgpt_auth( + codex_home.path(), + ChatGptAuthFixture::new("chatgpt-access-token").plan_type("pro"), + AuthCredentialsStoreMode::File, + )?; + + let mut mcp = McpProcess::new_with_env(codex_home.path(), &[("OPENAI_API_KEY", None)]).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_list_models_request(ModelListParams { + limit: Some(100), + cursor: None, + include_hidden: None, + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + + let ModelListResponse { + data: items, + next_cursor, + } = to_response::(response)?; + let mut expected_presets: Vec = vec![remote_model.into()]; + ModelPreset::mark_default_by_picker_visibility(&mut expected_presets); + let expected_items = expected_presets + .iter() + .map(model_from_preset) + .collect::>(); + + assert_eq!(items, expected_items); + assert!(next_cursor.is_none()); + assert_eq!( + models_mock.requests().len(), + 1, + "expected a single /models request" + ); + Ok(()) +} + #[tokio::test] async fn list_models_pagination_works() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/models-manager/src/manager.rs b/codex-rs/models-manager/src/manager.rs index 517bb0abbb..af510c8d73 100644 --- a/codex-rs/models-manager/src/manager.rs +++ b/codex-rs/models-manager/src/manager.rs @@ -3,11 +3,13 @@ use crate::collaboration_mode_presets::builtin_collaboration_mode_presets; use crate::config::ModelsManagerConfig; use crate::model_info; use async_trait::async_trait; +use codex_app_server_protocol::AuthMode; use codex_login::AuthManager; use codex_protocol::config_types::CollaborationModeMask; use codex_protocol::error::Result as CoreResult; use codex_protocol::openai_models::ModelInfo; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; use std::fmt; use std::path::PathBuf; @@ -319,6 +321,23 @@ impl OpenAiModelsManager { /// Replace the cached remote models and rebuild the derived presets list. async fn apply_remote_models(&self, models: Vec) { + // Use the remote models list as the source of truth if it contains at least one + // non-hidden model and the user is using ChatGPT auth. + let should_use_remote_models_only = !models.is_empty() + && models + .iter() + .any(|model| model.visibility == ModelVisibility::List) + && self.auth_manager.as_ref().is_some_and(|auth_manager| { + matches!( + auth_manager.auth_mode(), + Some(AuthMode::Chatgpt | AuthMode::ChatgptAuthTokens) + ) + }); + if should_use_remote_models_only { + *self.remote_models.write().await = models; + return; + } + let mut existing_models = load_remote_models_from_file().unwrap_or_default(); for model in models { if let Some(existing_index) = existing_models diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index 9df9fb09c9..ede7cfe795 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -366,6 +366,160 @@ async fn refresh_available_models_sorts_by_priority() { assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); } +#[tokio::test] +async fn refresh_available_models_uses_remote_only_catalog_for_chatgpt_auth() { + let remote_models = vec![remote_model( + "chatgpt-visible-source-of-truth", + "ChatGPT Visible", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![remote_models.clone()]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint.clone()); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, remote_models); + assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); +} + +#[tokio::test] +async fn refresh_available_models_uses_cached_remote_only_catalog_for_chatgpt_auth() { + let remote_models = vec![remote_model( + "chatgpt-cached-source-of-truth", + "ChatGPT Cached", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let fetch_endpoint = TestModelsEndpoint::new(vec![remote_models.clone()]); + let fetch_manager = + openai_manager_for_tests(codex_home.path().to_path_buf(), fetch_endpoint.clone()); + + fetch_manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("initial refresh succeeds"); + + let cache_endpoint = TestModelsEndpoint::new(Vec::new()); + let cache_manager = + openai_manager_for_tests(codex_home.path().to_path_buf(), cache_endpoint.clone()); + + cache_manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("cached refresh succeeds"); + + assert_eq!(cache_manager.get_remote_models().await, remote_models); + assert_eq!( + cache_endpoint.fetch_count(), + 0, + "fresh cache should avoid a model fetch" + ); +} + +#[tokio::test] +async fn get_model_info_uses_fallback_for_bundled_models_when_chatgpt_remote_is_authoritative() { + let remote_models = vec![remote_model( + "chatgpt-authoritative-model-info", + "ChatGPT Model Info", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![remote_models]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let bundled_slug = load_remote_models_from_file() + .expect("bundled models should parse") + .first() + .expect("bundled models should contain at least one model") + .slug + .clone(); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + let model_info = manager + .get_model_info(&bundled_slug, &ModelsManagerConfig::default()) + .await; + + assert_eq!(model_info.slug, bundled_slug); + assert!(model_info.used_fallback_model_metadata); +} + +#[tokio::test] +async fn refresh_available_models_preserves_bundled_catalog_for_empty_chatgpt_remote() { + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![Vec::new()]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let expected = load_remote_models_from_file().expect("bundled models should parse"); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); +} + +#[tokio::test] +async fn refresh_available_models_merges_hidden_only_chatgpt_remote_with_bundled_catalog() { + let hidden_remote = remote_model_with_visibility( + "chatgpt-hidden-only", + "ChatGPT Hidden", + /*priority*/ 0, + "hide", + ); + let codex_home = tempdir().expect("temp dir"); + let endpoint = TestModelsEndpoint::new(vec![vec![hidden_remote.clone()]]); + let manager = openai_manager_for_tests(codex_home.path().to_path_buf(), endpoint); + let mut expected = load_remote_models_from_file().expect("bundled models should parse"); + expected.push(hidden_remote); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); +} + +#[tokio::test] +async fn refresh_available_models_keeps_merging_for_api_auth() { + let remote_models = vec![remote_model( + "api-auth-visible-remote", + "API Auth Visible", + /*priority*/ 0, + )]; + let codex_home = tempdir().expect("temp dir"); + let endpoint = Arc::new(TestModelsEndpoint { + has_command_auth: true, + uses_codex_backend: false, + responses: Mutex::new(vec![remote_models.clone()].into()), + fetch_count: AtomicUsize::new(0), + }); + let manager = openai_manager_for_tests_with_auth( + codex_home.path().to_path_buf(), + endpoint.clone(), + Some(AuthManager::from_auth_for_testing(CodexAuth::from_api_key( + "test-api-key", + ))), + ); + let mut expected = load_remote_models_from_file().expect("bundled models should parse"); + expected.extend(remote_models); + + manager + .refresh_available_models(RefreshStrategy::OnlineIfUncached) + .await + .expect("refresh succeeds"); + + assert_eq!(manager.get_remote_models().await, expected); + assert_eq!(endpoint.fetch_count(), 1, "expected a single model fetch"); +} + #[tokio::test] async fn refresh_available_models_uses_cache_when_fresh() { let remote_models = vec![remote_model("cached", "Cached", /*priority*/ 5)]; From 4a1f1df8ce2686fc067d0cd84b85c5b2a229a0a0 Mon Sep 17 00:00:00 2001 From: Casey Chow Date: Thu, 14 May 2026 11:41:04 -0700 Subject: [PATCH 3/6] [codex] fix plugin CLI active user layer compile (#22666) ## Why PR #21396 merged after #17141 removed the old `ConfigLayerStack::get_user_layer()` API. The new plugin CLI call sites still used that stale API, which caused `main` to fail compilation. ## What Changed - update `codex plugin marketplace list` to read configured marketplaces through `get_active_user_layer()` - update the plugin snapshot validation helper to use `get_active_user_layer()` This preserves the intended active writable user-layer behavior from the profile-aware config API while fixing the stale call sites. ## Validation - `cargo check -p codex-cli` - `cargo test -p codex-cli --test plugin_cli` - `git diff --check` --- codex-rs/cli/src/marketplace_cmd.rs | 2 +- codex-rs/cli/src/plugin_cmd.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/cli/src/marketplace_cmd.rs b/codex-rs/cli/src/marketplace_cmd.rs index b4132a0127..2ec948eacb 100644 --- a/codex-rs/cli/src/marketplace_cmd.rs +++ b/codex-rs/cli/src/marketplace_cmd.rs @@ -152,7 +152,7 @@ async fn run_list(overrides: Vec<(String, toml::Value)>) -> Result<()> { .context("failed to load configuration")?; let configured_marketplaces = config .config_layer_stack - .get_user_layer() + .get_active_user_layer() .and_then(|layer| layer.config.get("marketplaces")) .and_then(toml::Value::as_table); let Some(configured_marketplaces) = configured_marketplaces else { diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index dba06313e5..593fe644b2 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -349,7 +349,7 @@ fn configured_marketplace_snapshot_issues( load_errors: &[MarketplaceListError], marketplace_name: Option<&str>, ) -> Vec { - let Some(user_layer) = plugins_input.config_layer_stack.get_user_layer() else { + let Some(user_layer) = plugins_input.config_layer_stack.get_active_user_layer() else { return Vec::new(); }; let Some(configured_marketplaces) = user_layer From d8ddeb68696dd7b90fe41cd3809821d2ddf7d1b0 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 14 May 2026 11:52:43 -0700 Subject: [PATCH 4/6] Support explicit MCP OAuth client IDs (#22575) ## Why Some MCP OAuth providers require a pre-registered public client ID and cannot rely on dynamic client registration. Codex already supports MCP OAuth, but it had no way to supply that client ID from config into the PKCE flow. ## What changed - add `oauth.client_id` under `[mcp_servers.]` config, including config editing and schema generation - thread the configured client ID through CLI, app-server, plugin login, and MCP skill dependency OAuth entrypoints - configure RMCP authorization with the explicit client when present, while preserving the existing dynamic-registration path when it is absent - add focused coverage for config parsing/serialization and OAuth URL generation ## Verification - `cargo test -p codex-config -p codex-rmcp-client -p codex-mcp -p codex-core-plugins` - `cargo test -p codex-core blocking_replace_mcp_servers_round_trips --lib` - `cargo test -p codex-core replace_mcp_servers_streamable_http_serializes_oauth_resource --lib` - `cargo test -p codex-core config_schema_matches_fixture --lib` ## Notes Broader local package runs still hit unrelated pre-existing stack overflows in: - `codex-app-server::in_process_start_clamps_zero_channel_capacity` - `codex-core::resume_agent_from_rollout_uses_edge_data_when_descendant_metadata_source_is_stale` --- .../src/request_processors/mcp_processor.rs | 1 + .../src/request_processors/plugins.rs | 3 + codex-rs/cli/src/mcp_cmd.rs | 6 + .../codex-mcp/src/connection_manager_tests.rs | 2 + codex-rs/codex-mcp/src/mcp/mod.rs | 1 + codex-rs/codex-mcp/src/mcp/mod_tests.rs | 2 + codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/mcp_edit.rs | 9 ++ codex-rs/config/src/mcp_edit_tests.rs | 62 +++++++++ codex-rs/config/src/mcp_types.rs | 26 ++++ codex-rs/config/src/mcp_types_tests.rs | 34 +++++ codex-rs/config/src/types.rs | 1 + codex-rs/core-plugins/src/loader.rs | 22 ++- codex-rs/core-plugins/src/manager_tests.rs | 7 + codex-rs/core/config.schema.json | 19 +++ codex-rs/core/src/config/config_tests.rs | 23 ++++ codex-rs/core/src/config/edit.rs | 9 ++ codex-rs/core/src/config/edit_tests.rs | 13 ++ codex-rs/core/src/mcp_skill_dependencies.rs | 5 + codex-rs/core/tests/suite/code_mode.rs | 1 + codex-rs/core/tests/suite/hooks_mcp.rs | 1 + codex-rs/core/tests/suite/rmcp_client.rs | 1 + codex-rs/core/tests/suite/search_tool.rs | 3 + codex-rs/core/tests/suite/sqlite_state.rs | 1 + codex-rs/core/tests/suite/truncation.rs | 3 + .../rmcp-client/src/perform_oauth_login.rs | 129 +++++++++++++++++- 26 files changed, 374 insertions(+), 11 deletions(-) diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 812108a15c..548bb5be87 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -160,6 +160,7 @@ impl McpRequestProcessor { http_headers, env_http_headers, &resolved_scopes.scopes, + server.oauth_client_id(), server.oauth_resource.as_deref(), timeout_secs, config.mcp_oauth_callback_port, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index ea1ff3ead4..c290d2091a 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -1346,6 +1346,7 @@ impl PluginRequestProcessor { let notification_name = name.clone(); tokio::spawn(async move { + let oauth_client_id = server.oauth_client_id(); let first_attempt = perform_oauth_login_silent( &name, &oauth_config.url, @@ -1353,6 +1354,7 @@ impl PluginRequestProcessor { oauth_config.http_headers.clone(), oauth_config.env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), @@ -1368,6 +1370,7 @@ impl PluginRequestProcessor { oauth_config.http_headers, oauth_config.env_http_headers, &[], + oauth_client_id, server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index af75999163..5c66a27d84 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -199,6 +199,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers: Option>, env_http_headers: Option>, resolved_scopes: &ResolvedMcpOAuthScopes, + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -210,6 +211,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers.clone(), env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -226,6 +228,7 @@ async fn perform_oauth_login_retry_without_scopes( http_headers, env_http_headers, &[], + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -309,6 +312,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }; @@ -338,6 +342,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re oauth_config.http_headers, oauth_config.env_http_headers, &resolved_scopes, + /*oauth_client_id*/ None, /*oauth_resource*/ None, config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -431,6 +436,7 @@ async fn run_login(config_overrides: &CliConfigOverrides, login_args: LoginArgs) http_headers, env_http_headers, &resolved_scopes, + server.oauth_client_id(), server.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 976bd4c768..50b1bf6c12 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -893,6 +893,7 @@ fn mcp_init_error_display_prompts_for_github_pat() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }), @@ -945,6 +946,7 @@ fn mcp_init_error_display_reports_generic_errors() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }), diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index b8fb9a080c..0d24718ba9 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -446,6 +446,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 0d0afc6c48..286c191b29 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -278,6 +278,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -302,6 +303,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/config/src/lib.rs b/codex-rs/config/src/lib.rs index 371c3ffba0..3b49db638f 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -96,6 +96,7 @@ pub use mcp_types::AppToolApproval; pub use mcp_types::McpServerConfig; pub use mcp_types::McpServerDisabledReason; pub use mcp_types::McpServerEnvVar; +pub use mcp_types::McpServerOAuthConfig; pub use mcp_types::McpServerToolConfig; pub use mcp_types::McpServerTransportConfig; pub use mcp_types::RawMcpServerConfig; diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index f5881a1257..aaad3444fd 100644 --- a/codex-rs/config/src/mcp_edit.rs +++ b/codex-rs/config/src/mcp_edit.rs @@ -212,6 +212,15 @@ fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem { { entry["scopes"] = array_from_strings(scopes); } + if let Some(oauth) = &config.oauth + && let Some(client_id) = &oauth.client_id + && !client_id.is_empty() + { + let mut oauth_table = TomlTable::new(); + oauth_table.set_implicit(false); + oauth_table["client_id"] = value(client_id.clone()); + entry["oauth"] = TomlItem::Table(oauth_table); + } if let Some(resource) = &config.oauth_resource && !resource.is_empty() { diff --git a/codex-rs/config/src/mcp_edit_tests.rs b/codex-rs/config/src/mcp_edit_tests.rs index cfcd73c3e5..0215e7b808 100644 --- a/codex-rs/config/src/mcp_edit_tests.rs +++ b/codex-rs/config/src/mcp_edit_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::McpServerOAuthConfig; use crate::McpServerToolConfig; use pretty_assertions::assert_eq; use std::collections::HashMap; @@ -33,6 +34,7 @@ async fn replace_mcp_servers_serializes_per_tool_approval_overrides() -> anyhow: enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::from([ ( @@ -82,3 +84,63 @@ approval_mode = "approve" Ok(()) } + +#[tokio::test] +async fn replace_mcp_servers_serializes_oauth_client_id() -> anyhow::Result<()> { + let unique_suffix = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos(); + let codex_home = std::env::temp_dir().join(format!( + "codex-config-mcp-oauth-edit-test-{}-{unique_suffix}", + std::process::id() + )); + let servers = BTreeMap::from([( + "maas_outlook".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "https://example.com/mcp".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + experimental_environment: None, + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), + oauth_resource: None, + tools: HashMap::new(), + }, + )]); + + ConfigEditsBuilder::new(&codex_home) + .replace_mcp_servers(&servers) + .apply() + .await?; + + let config_path = codex_home.join(CONFIG_TOML_FILE); + let serialized = std::fs::read_to_string(&config_path)?; + assert_eq!( + serialized, + r#"[mcp_servers.maas_outlook] +url = "https://example.com/mcp" + +[mcp_servers.maas_outlook.oauth] +client_id = "eci-prd-pub-codex-123" +"# + ); + + let loaded = load_global_mcp_servers(&codex_home).await?; + assert_eq!(loaded, servers); + + std::fs::remove_dir_all(&codex_home)?; + + Ok(()) +} diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index d642d9fc57..8dfceec37e 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -114,6 +114,15 @@ impl AsRef for McpServerEnvVar { } } +/// OAuth client settings used when Codex launches an MCP OAuth flow. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct McpServerOAuthConfig { + /// Explicit OAuth client identifier to present during authorization and token exchange. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub client_id: Option, +} + #[derive(Serialize, Debug, Clone, PartialEq)] pub struct McpServerConfig { #[serde(flatten)] @@ -167,6 +176,10 @@ pub struct McpServerConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub scopes: Option>, + /// Optional OAuth client settings for MCP login. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub oauth: Option, + /// Optional OAuth resource parameter to include during MCP login (RFC 8707). #[serde(default, skip_serializing_if = "Option::is_none")] pub oauth_resource: Option, @@ -176,6 +189,14 @@ pub struct McpServerConfig { pub tools: HashMap, } +impl McpServerConfig { + pub fn oauth_client_id(&self) -> Option<&str> { + self.oauth + .as_ref() + .and_then(|oauth| oauth.client_id.as_deref()) + } +} + /// Raw MCP config shape used for deserialization and supported-field JSON /// Schema generation. /// @@ -233,6 +254,8 @@ pub struct RawMcpServerConfig { #[serde(default)] pub scopes: Option>, #[serde(default)] + pub oauth: Option, + #[serde(default)] pub oauth_resource: Option, /// Legacy display-name field accepted for backward compatibility. #[serde(default, rename = "name")] @@ -267,6 +290,7 @@ impl TryFrom for McpServerConfig { enabled_tools, disabled_tools, scopes, + oauth, oauth_resource, _name: _, tools, @@ -297,6 +321,7 @@ impl TryFrom for McpServerConfig { throw_if_set("stdio", "bearer_token", bearer_token.as_ref())?; throw_if_set("stdio", "http_headers", http_headers.as_ref())?; throw_if_set("stdio", "env_http_headers", env_http_headers.as_ref())?; + throw_if_set("stdio", "oauth", oauth.as_ref())?; throw_if_set("stdio", "oauth_resource", oauth_resource.as_ref())?; let env_vars = env_vars.unwrap_or_default(); for env_var in &env_vars { @@ -338,6 +363,7 @@ impl TryFrom for McpServerConfig { enabled_tools, disabled_tools, scopes, + oauth, oauth_resource, tools: tools.unwrap_or_default(), }) diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 50f705d0f8..f397115425 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -283,6 +283,26 @@ fn deserialize_streamable_http_server_config_with_oauth_resource() { ); } +#[test] +fn deserialize_streamable_http_server_config_with_oauth_client_id() { + let cfg: McpServerConfig = toml::from_str( + r#" + url = "https://example.com/mcp" + + [oauth] + client_id = "eci-prd-pub-codex-123" + "#, + ) + .expect("should deserialize http config with oauth client id"); + + assert_eq!( + cfg.oauth, + Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }) + ); +} + #[test] fn deserialize_server_config_with_tool_filters() { let cfg: McpServerConfig = toml::from_str( @@ -393,6 +413,7 @@ fn deserialize_ignores_unknown_server_fields() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -439,6 +460,19 @@ fn deserialize_rejects_headers_for_stdio() { ) .expect_err("should reject env_http_headers for stdio transport"); + let err = toml::from_str::( + r#" + command = "echo" + oauth = { client_id = "eci-prd-pub-codex-123" } + "#, + ) + .expect_err("should reject oauth for stdio transport"); + + assert!( + err.to_string().contains("oauth is not supported for stdio"), + "unexpected error: {err}" + ); + let err = toml::from_str::( r#" command = "echo" diff --git a/codex-rs/config/src/types.rs b/codex-rs/config/src/types.rs index 74e3394f93..c7d0a5876b 100644 --- a/codex-rs/config/src/types.rs +++ b/codex-rs/config/src/types.rs @@ -7,6 +7,7 @@ pub use crate::mcp_types::AppToolApproval; pub use crate::mcp_types::McpServerConfig; pub use crate::mcp_types::McpServerDisabledReason; pub use crate::mcp_types::McpServerEnvVar; +pub use crate::mcp_types::McpServerOAuthConfig; pub use crate::mcp_types::McpServerToolConfig; pub use crate::mcp_types::McpServerTransportConfig; pub use crate::mcp_types::RawMcpServerConfig; diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index b26f1c21b2..fc4193aa67 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1053,13 +1053,21 @@ fn normalize_plugin_mcp_server_value( } } - if let Some(JsonValue::Object(oauth)) = object.remove("oauth") - && oauth.contains_key("callbackPort") - { - warn!( - plugin = %plugin_root.display(), - "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" - ); + if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { + if oauth.remove("callbackPort").is_some() { + warn!( + plugin = %plugin_root.display(), + "plugin MCP server OAuth callbackPort is ignored; Codex uses global MCP OAuth callback settings" + ); + } + + if let Some(client_id) = oauth.remove("clientId") { + oauth.entry("client_id".to_string()).or_insert(client_id); + } + + if !oauth.is_empty() { + object.insert("oauth".to_string(), JsonValue::Object(oauth)); + } } if let Some(JsonValue::String(cwd)) = object.get("cwd") diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index e0b22c0040..449c87b186 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -22,6 +22,7 @@ use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; use codex_config::McpServerConfig; +use codex_config::McpServerOAuthConfig; use codex_config::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_login::CodexAuth; @@ -230,6 +231,9 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("client-id".to_string()), + }), oauth_resource: None, tools: HashMap::new(), }, @@ -694,6 +698,7 @@ async fn load_plugins_uses_manifest_configured_component_paths() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -805,6 +810,7 @@ async fn load_plugins_ignores_manifest_component_paths_without_dot_slash() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -966,6 +972,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }; diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index d8c64a0d13..9fc95349d2 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1200,6 +1200,17 @@ } ] }, + "McpServerOAuthConfig": { + "additionalProperties": false, + "description": "OAuth client settings used when Codex launches an MCP OAuth flow.", + "properties": { + "client_id": { + "description": "Explicit OAuth client identifier to present during authorization and token exchange.", + "type": "string" + } + }, + "type": "object" + }, "McpServerToolConfig": { "additionalProperties": false, "description": "Per-tool approval settings for a single MCP server tool.", @@ -2124,6 +2135,14 @@ "description": "Legacy display-name field accepted for backward compatibility.", "type": "string" }, + "oauth": { + "allOf": [ + { + "$ref": "#/definitions/McpServerOAuthConfig" + } + ], + "default": null + }, "oauth_resource": { "default": null, "type": "string" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 27a67da68d..ff28e50436 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -36,6 +36,7 @@ use codex_config::types::BundledSkillsConfig; use codex_config::types::FeedbackConfigToml; use codex_config::types::HistoryPersistence; use codex_config::types::McpServerEnvVar; +use codex_config::types::McpServerOAuthConfig; use codex_config::types::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_config::types::MemoriesConfig; @@ -124,6 +125,7 @@ fn stdio_mcp(command: &str) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -148,6 +150,7 @@ fn http_mcp(url: &str) -> McpServerConfig { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), } @@ -4428,6 +4431,7 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4721,6 +4725,7 @@ async fn replace_mcp_servers_serializes_env_sorted() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4797,6 +4802,7 @@ async fn replace_mcp_servers_serializes_env_vars() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4858,6 +4864,7 @@ async fn replace_mcp_servers_serializes_sourced_env_vars() -> anyhow::Result<()> enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4909,6 +4916,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -4963,6 +4971,7 @@ async fn replace_mcp_servers_streamable_http_serializes_bearer_token() -> anyhow enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5033,6 +5042,7 @@ async fn replace_mcp_servers_streamable_http_serializes_custom_headers() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5115,6 +5125,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5150,6 +5161,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5220,6 +5232,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5245,6 +5258,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5333,6 +5347,7 @@ async fn replace_mcp_servers_serializes_disabled_flag() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5383,6 +5398,7 @@ async fn replace_mcp_servers_serializes_required_flag() -> anyhow::Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5433,6 +5449,7 @@ async fn replace_mcp_servers_serializes_tool_filters() -> anyhow::Result<()> { enabled_tools: Some(vec!["allowed".to_string()]), disabled_tools: Some(vec!["blocked".to_string()]), scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -5487,6 +5504,9 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh enabled_tools: None, disabled_tools: None, scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), oauth_resource: Some("https://resource.example.com".to_string()), tools: HashMap::new(), }, @@ -5500,6 +5520,8 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh let config_path = codex_home.path().join(CONFIG_TOML_FILE); let serialized = std::fs::read_to_string(&config_path)?; + assert!(serialized.contains("[mcp_servers.docs.oauth]")); + assert!(serialized.contains(r#"client_id = "eci-prd-pub-codex-123""#)); assert!(serialized.contains(r#"oauth_resource = "https://resource.example.com""#)); let loaded = load_global_mcp_servers(codex_home.path()).await?; @@ -5508,6 +5530,7 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh docs.oauth_resource.as_deref(), Some("https://resource.example.com") ); + assert_eq!(docs.oauth_client_id(), Some("eci-prd-pub-codex-123")); Ok(()) } diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index 4e5b46e182..80bf4d9bf1 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -341,6 +341,15 @@ mod document_helpers { { entry["scopes"] = array_from_iter(scopes.iter().cloned()); } + if let Some(oauth) = &config.oauth + && let Some(client_id) = &oauth.client_id + && !client_id.is_empty() + { + let mut oauth_table = TomlTable::new(); + oauth_table.set_implicit(false); + oauth_table["client_id"] = value(client_id.clone()); + entry["oauth"] = TomlItem::Table(oauth_table); + } if let Some(resource) = &config.oauth_resource && !resource.is_empty() { diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index ec7342c779..751411afdf 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -1,5 +1,6 @@ use super::*; use codex_config::types::AppToolApproval; +use codex_config::types::McpServerOAuthConfig; use codex_config::types::McpServerToolConfig; use codex_config::types::McpServerTransportConfig; use codex_config::types::SessionPickerViewMode; @@ -940,6 +941,7 @@ fn blocking_replace_mcp_servers_round_trips() { enabled_tools: Some(vec!["one".to_string(), "two".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -969,6 +971,9 @@ fn blocking_replace_mcp_servers_round_trips() { enabled_tools: None, disabled_tools: Some(vec!["forbidden".to_string()]), scopes: None, + oauth: Some(McpServerOAuthConfig { + client_id: Some("eci-prd-pub-codex-123".to_string()), + }), oauth_resource: Some("https://resource.example.com".to_string()), tools: HashMap::new(), }, @@ -994,6 +999,9 @@ oauth_resource = \"https://resource.example.com\" [mcp_servers.http.http_headers] Z-Header = \"z\" +[mcp_servers.http.oauth] +client_id = \"eci-prd-pub-codex-123\" + [mcp_servers.stdio] command = \"cmd\" args = [\"--flag\"] @@ -1035,6 +1043,7 @@ fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::from([( "search".to_string(), @@ -1099,6 +1108,7 @@ foo = { command = "cmd" } enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1153,6 +1163,7 @@ foo = { command = "cmd" } # keep me enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1206,6 +1217,7 @@ foo = { command = "cmd", args = ["--flag"] } # keep me enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -1260,6 +1272,7 @@ foo = { command = "cmd" } enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index 44764e0064..b8552596b3 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -151,6 +151,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( server_config.scopes.clone(), oauth_config.discovered_scopes.clone(), ); + let oauth_client_id = server_config.oauth_client_id(); let first_attempt = perform_oauth_login( &name, &oauth_config.url, @@ -158,6 +159,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( oauth_config.http_headers.clone(), oauth_config.env_http_headers.clone(), &resolved_scopes.scopes, + oauth_client_id, server_config.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -173,6 +175,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( oauth_config.http_headers, oauth_config.env_http_headers, &[], + oauth_client_id, server_config.oauth_resource.as_deref(), config.mcp_oauth_callback_port, config.mcp_oauth_callback_url.as_deref(), @@ -369,6 +372,7 @@ fn mcp_dependency_to_server_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }); @@ -398,6 +402,7 @@ fn mcp_dependency_to_server_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }); diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index 3bcb37e7b2..af63d092f8 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -244,6 +244,7 @@ async fn run_code_mode_turn_with_rmcp_config( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 96ed732695..80290e31ba 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -193,6 +193,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 1a7062bdb1..d1973d97a9 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -332,6 +332,7 @@ fn insert_mcp_server( enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index b3cf2e3bca..bb4ca84b29 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -999,6 +999,7 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { enabled_tools: Some(vec!["echo".to_string(), "image".to_string()]), disabled_tools: Some(vec!["image".to_string()]), scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), @@ -1127,6 +1128,7 @@ async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result< enabled_tools: Some(vec!["echo".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), @@ -1272,6 +1274,7 @@ async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_descripti enabled_tools: Some(vec!["echo".to_string()]), disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, supports_parallel_tool_calls: false, tools: HashMap::new(), diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index b2a2778047..0cc7d07b16 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -386,6 +386,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index 0fd072a70f..447bca6731 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -398,6 +398,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -497,6 +498,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, @@ -779,6 +781,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { enabled_tools: None, disabled_tools: None, scopes: None, + oauth: None, oauth_resource: None, tools: HashMap::new(), }, diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index f42786c652..eb6c7906a8 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -11,6 +11,9 @@ use base64::Engine; use base64::engine::general_purpose::URL_SAFE_NO_PAD; use reqwest::ClientBuilder; use reqwest::Url; +use rmcp::transport::AuthorizationManager; +use rmcp::transport::AuthorizationSession; +use rmcp::transport::auth::OAuthClientConfig; use rmcp::transport::auth::OAuthState; use sha2::Digest; use sha2::Sha256; @@ -81,6 +84,7 @@ pub async fn perform_oauth_login( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -92,6 +96,7 @@ pub async fn perform_oauth_login( http_headers, env_http_headers, scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -108,6 +113,7 @@ pub async fn perform_oauth_login_silent( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -119,6 +125,7 @@ pub async fn perform_oauth_login_silent( http_headers, env_http_headers, scopes, + oauth_client_id, oauth_resource, callback_port, callback_url, @@ -135,6 +142,7 @@ async fn perform_oauth_login_with_browser_output( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, @@ -150,6 +158,7 @@ async fn perform_oauth_login_with_browser_output( store_mode, headers, scopes, + oauth_client_id, oauth_resource, /*launch_browser*/ true, callback_port, @@ -169,6 +178,7 @@ pub async fn perform_oauth_login_return_url( http_headers: Option>, env_http_headers: Option>, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, timeout_secs: Option, callback_port: Option, @@ -184,6 +194,7 @@ pub async fn perform_oauth_login_return_url( store_mode, headers, scopes, + oauth_client_id, oauth_resource, /*launch_browser*/ false, callback_port, @@ -436,6 +447,7 @@ impl OauthLoginFlow { store_mode: OAuthCredentialsStoreMode, headers: OauthHeaders, scopes: &[String], + oauth_client_id: Option<&str>, oauth_resource: Option<&str>, launch_browser: bool, callback_port: Option, @@ -471,11 +483,15 @@ impl OauthLoginFlow { let default_headers = build_default_headers(http_headers, env_http_headers)?; let http_client = apply_default_headers(ClientBuilder::new(), &default_headers).build()?; - let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?; let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect(); - oauth_state - .start_authorization(&scope_refs, &redirect_uri, Some("Codex")) - .await?; + let oauth_state = start_authorization( + server_url, + http_client, + &scope_refs, + &redirect_uri, + oauth_client_id, + ) + .await?; let auth_url = append_query_param( &oauth_state.get_authorization_url().await?, "resource", @@ -585,6 +601,41 @@ impl OauthLoginFlow { } } +async fn start_authorization( + server_url: &str, + http_client: reqwest::Client, + scopes: &[&str], + redirect_uri: &str, + oauth_client_id: Option<&str>, +) -> Result { + let Some(oauth_client_id) = oauth_client_id.filter(|client_id| !client_id.trim().is_empty()) + else { + let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?; + oauth_state + .start_authorization(scopes, redirect_uri, Some("Codex")) + .await?; + return Ok(oauth_state); + }; + + let mut auth_manager = AuthorizationManager::new(server_url).await?; + auth_manager.with_client(http_client)?; + let metadata = auth_manager.discover_metadata().await?; + auth_manager.set_metadata(metadata); + auth_manager.configure_client(OAuthClientConfig { + client_id: oauth_client_id.to_string(), + client_secret: None, + scopes: scopes.iter().map(|scope| (*scope).to_string()).collect(), + redirect_uri: redirect_uri.to_string(), + })?; + let auth_url = auth_manager.get_authorization_url(scopes).await?; + + Ok(OAuthState::Session(AuthorizationSession { + auth_manager, + auth_url, + redirect_uri: redirect_uri.to_string(), + })) +} + fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String { let Some(value) = value else { return url.to_string(); @@ -604,7 +655,13 @@ fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String { #[cfg(test)] mod tests { + use axum::Json; + use axum::Router; + use axum::routing::get; use pretty_assertions::assert_eq; + use reqwest::Url; + use serde_json::json; + use tokio::net::TcpListener; use super::CallbackOutcome; use super::OAuthProviderError; @@ -613,6 +670,70 @@ mod tests { use super::callback_id_from_server_url; use super::callback_path_from_redirect_uri; use super::parse_oauth_callback; + use super::start_authorization; + + async fn spawn_oauth_metadata_server() -> String { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("bind metadata listener"); + let addr = listener.local_addr().expect("read metadata listener addr"); + let base_url = format!("http://{addr}"); + let metadata = json!({ + "authorization_endpoint": format!("{base_url}/oauth/authorize"), + "token_endpoint": format!("{base_url}/oauth/token"), + "scopes_supported": [""], + }); + let path_scoped_metadata = metadata.clone(); + let app = Router::new() + .route( + "/.well-known/oauth-authorization-server/mcp", + get(move || { + let metadata = path_scoped_metadata.clone(); + async move { Json(metadata) } + }), + ) + .route( + "/.well-known/oauth-authorization-server", + get(move || { + let metadata = metadata.clone(); + async move { Json(metadata) } + }), + ); + + tokio::spawn(async move { + axum::serve(listener, app) + .await + .expect("serve oauth metadata"); + }); + + base_url + } + + #[tokio::test] + async fn start_authorization_uses_configured_client_id() { + let base_url = spawn_oauth_metadata_server().await; + let oauth_state = start_authorization( + &format!("{base_url}/mcp"), + reqwest::Client::new(), + &[], + "http://127.0.0.1/callback", + Some("eci-prd-pub-codex-123"), + ) + .await + .expect("start oauth authorization"); + + let authorization_url = oauth_state + .get_authorization_url() + .await + .expect("read authorization url"); + let auth_url = Url::parse(&authorization_url).expect("authorization url should parse"); + let client_id = auth_url + .query_pairs() + .find(|(key, _)| key == "client_id") + .map(|(_, value)| value.into_owned()); + + assert_eq!(client_id.as_deref(), Some("eci-prd-pub-codex-123")); + } #[test] fn parse_oauth_callback_accepts_default_path() { From e8969d940d62775f14373cdfad3a4a4aeaacb7bc Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 14 May 2026 12:14:20 -0700 Subject: [PATCH 5/6] test: isolate exec review policy config test (#22512) ## Why `thread_start_params_include_review_policy_when_review_policy_is_manual_only` builds a `Config` with a temporary `CODEX_HOME`, but `ConfigBuilder::default()` can still load host-managed configuration. On local macOS machines with enterprise-managed Codex config, that host state can leak into the test and change the resulting config, even though CI does not have the same managed config source. This makes the test environment-dependent: it can pass in CI while failing locally for developers who have managed configuration installed. ## What Changed - Updated `codex-rs/exec/src/lib_tests.rs` so the test calls `LoaderOverrides::without_managed_config_for_tests()` through `ConfigBuilder::loader_overrides(...)`. - Left the rest of the test setup intact, including the temporary `CODEX_HOME`, temporary cwd, and explicit `approvals_reviewer` harness override. ## Verification ```shell cargo test -p codex-exec thread_start_params_include_review_policy_when_review_policy_is_manual_only ``` --- codex-rs/exec/src/lib_tests.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/exec/src/lib_tests.rs b/codex-rs/exec/src/lib_tests.rs index c12b483e89..321f4b8f9e 100644 --- a/codex-rs/exec/src/lib_tests.rs +++ b/codex-rs/exec/src/lib_tests.rs @@ -409,6 +409,7 @@ async fn thread_start_params_include_review_policy_when_review_policy_is_manual_ let codex_home = tempdir().expect("create temp codex home"); let cwd = tempdir().expect("create temp cwd"); let config = ConfigBuilder::default() + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { approvals_reviewer: Some(ApprovalsReviewer::User), From 255748638cc91a315250345a17296a1a3765b600 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 14 May 2026 12:40:01 -0700 Subject: [PATCH 6/6] Fix remote environment test fixtures (#22572) ## Why The Docker remote-env coverage was failing before it reached the behavior those tests are meant to exercise. The remote-aware test fixture only registered the remote environment, so tests that intentionally select both `local` and `remote` could not start a turn. After that was fixed, two tests exposed stale fixtures: the approval test was auto-approving under workspace-write, and the remote `view_image` test was writing invalid PNG bytes. ## What Changed - Added `EnvironmentManager::create_for_tests_with_local(...)` so tests can keep the provider default while also selecting `local` explicitly. - Updated `build_remote_aware()` to use that test-only manager when a remote exec-server URL is present. - Changed the remote apply-patch approval helper to use `SandboxPolicy::new_read_only_policy()` so the test actually exercises approval caching per environment. - Replaced the hardcoded remote `view_image` PNG blob with the existing `png_bytes(...)` helper so the test uses a valid image fixture. ## Validation Ran these isolated Docker remote-env tests on the devbox with `$remote-tests` setup: - `suite::remote_env::apply_patch_freeform_routes_to_selected_remote_environment` - `suite::remote_env::apply_patch_approvals_are_remembered_per_environment` - `suite::remote_env::apply_patch_intercepted_exec_command_routes_to_selected_remote_environment` - `suite::remote_env::exec_command_routes_to_selected_remote_environment` - `suite::view_image::view_image_routes_to_selected_remote_environment` All five pass. --- codex-rs/core/tests/common/test_codex.rs | 68 +++++++++++++++---- codex-rs/core/tests/suite/agents_md.rs | 2 +- codex-rs/core/tests/suite/apply_patch_cli.rs | 2 +- .../core/tests/suite/hierarchical_agents.rs | 4 +- codex-rs/core/tests/suite/remote_env.rs | 8 +-- codex-rs/core/tests/suite/rmcp_client.rs | 28 ++++---- codex-rs/core/tests/suite/skills.rs | 2 +- codex-rs/core/tests/suite/unified_exec.rs | 48 ++++++------- codex-rs/core/tests/suite/view_image.rs | 30 ++++---- codex-rs/exec-server/src/environment.rs | 14 ++++ 10 files changed, 129 insertions(+), 77 deletions(-) diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index e7db3f4044..fd53e828a9 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -296,11 +296,14 @@ impl TestCodexBuilder { }; let base_url = format!("{}/v1", server.uri()); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await } - pub async fn build_remote_aware( + pub async fn build_with_remote_env( &mut self, server: &wiremock::MockServer, ) -> anyhow::Result { @@ -310,8 +313,28 @@ impl TestCodexBuilder { }; let base_url = format!("{}/v1", server.uri()); let test_env = test_env().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await + } + + pub async fn build_with_remote_and_local_env( + &mut self, + server: &wiremock::MockServer, + ) -> anyhow::Result { + let home = match self.home.clone() { + Some(home) => home, + None => Arc::new(TempDir::new()?), + }; + let base_url = format!("{}/v1", server.uri()); + let test_env = test_env().await?; + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ true, + )) + .await } pub async fn build_with_streaming_server( @@ -329,6 +352,7 @@ impl TestCodexBuilder { home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, )) .await } @@ -350,8 +374,11 @@ impl TestCodexBuilder { config.realtime.version = RealtimeWsVersion::V1; })); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, /*resume_from*/ None, test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, home, /*resume_from*/ None, test_env, + /*include_local_environment*/ false, + )) + .await } pub async fn resume( @@ -362,8 +389,14 @@ impl TestCodexBuilder { ) -> anyhow::Result { let base_url = format!("{}/v1", server.uri()); let test_env = TestEnv::local().await?; - Box::pin(self.build_with_home_and_base_url(base_url, home, Some(rollout_path), test_env)) - .await + Box::pin(self.build_with_home_and_base_url( + base_url, + home, + Some(rollout_path), + test_env, + /*include_local_environment*/ false, + )) + .await } async fn build_with_home_and_base_url( @@ -372,6 +405,7 @@ impl TestCodexBuilder { home: Arc, resume_from: Option, test_env: TestEnv, + include_local_environment: bool, ) -> anyhow::Result { let (config, fallback_cwd) = self .prepare_config(base_url, &home, test_env.cwd().clone()) @@ -391,13 +425,19 @@ impl TestCodexBuilder { std::env::current_exe()?, codex_linux_sandbox_exe, )?; - let environment_manager = Arc::new( + let environment_manager = Arc::new(if include_local_environment { + codex_exec_server::EnvironmentManager::create_for_tests_with_local( + exec_server_url, + local_runtime_paths, + ) + .await + } else { codex_exec_server::EnvironmentManager::create_for_tests( exec_server_url, local_runtime_paths, ) - .await, - ); + .await + }); let file_system = test_env.environment().get_filesystem(); let mut workspace_setups = vec![]; swap(&mut self.workspace_setups, &mut workspace_setups); @@ -788,9 +828,9 @@ impl TestCodexHarness { Ok(Self { server, test }) } - pub async fn with_remote_aware_builder(mut builder: TestCodexBuilder) -> Result { + pub async fn with_remote_env_builder(mut builder: TestCodexBuilder) -> Result { let server = start_mock_server().await; - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; Ok(Self { server, test }) } diff --git a/codex-rs/core/tests/suite/agents_md.rs b/codex-rs/core/tests/suite/agents_md.rs index 724b852396..276fa4bb51 100644 --- a/codex-rs/core/tests/suite/agents_md.rs +++ b/codex-rs/core/tests/suite/agents_md.rs @@ -16,7 +16,7 @@ async fn agents_instructions(mut builder: TestCodexBuilder) -> Result { ) .await; - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; test.submit_turn("hello").await?; let request = resp_mock.single_request(); diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index e42057215a..b692d32568 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -64,7 +64,7 @@ async fn apply_patch_harness_with( }); // Box harness construction so apply_patch_cli tests do not inline the // full test-thread startup path into each test future. - Box::pin(TestCodexHarness::with_remote_aware_builder(builder)).await + Box::pin(TestCodexHarness::with_remote_env_builder(builder)).await } async fn submit_without_wait(harness: &TestCodexHarness, prompt: &str) -> Result<()> { diff --git a/codex-rs/core/tests/suite/hierarchical_agents.rs b/codex-rs/core/tests/suite/hierarchical_agents.rs index 212303ed0e..751807623b 100644 --- a/codex-rs/core/tests/suite/hierarchical_agents.rs +++ b/codex-rs/core/tests/suite/hierarchical_agents.rs @@ -32,7 +32,7 @@ async fn hierarchical_agents_appends_to_project_doc_in_user_instructions() { Ok::<(), anyhow::Error>(()) }); let test = builder - .build_remote_aware(&server) + .build_with_remote_env(&server) .await .expect("build test codex"); @@ -76,7 +76,7 @@ async fn hierarchical_agents_emits_when_no_project_doc() { .expect("test config should allow feature update"); }); let test = builder - .build_remote_aware(&server) + .build_with_remote_env(&server) .await .expect("build test codex"); diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index dc6d5cefc5..9c3414baeb 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -58,7 +58,7 @@ async fn unified_exec_test(server: &wiremock::MockServer) -> Result { "unified exec should enable for test: {result:?}", ); }); - builder.build_remote_aware(server).await + builder.build_with_remote_and_local_env(server).await } async fn submit_turn_with_approval_and_environments( @@ -77,7 +77,7 @@ async fn submit_turn_with_approval_and_environments( cwd: test.cwd.path().to_path_buf(), approval_policy: AskForApproval::OnRequest, approvals_reviewer: Some(ApprovalsReviewer::User), - sandbox_policy: SandboxPolicy::new_workspace_write_policy(), + sandbox_policy: SandboxPolicy::new_read_only_policy(), permission_profile: None, model: test.session_configured.model.clone(), effort: None, @@ -350,7 +350,7 @@ async fn apply_patch_freeform_routes_to_selected_remote_environment() -> Result< let mut builder = test_codex().with_config(|config| { config.include_apply_patch_tool = true; }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; let file_name = "apply_patch_remote_freeform.txt"; let remote_cwd = PathBuf::from(format!( @@ -439,7 +439,7 @@ async fn apply_patch_approvals_are_remembered_per_environment() -> Result<()> { config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.approvals_reviewer = ApprovalsReviewer::User; }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; let remote_cwd = PathBuf::from(format!( "/tmp/codex-remote-apply-patch-approval-cwd-{}", diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index d1973d97a9..7ead0364dc 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -480,7 +480,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -603,7 +603,7 @@ async fn stdio_server_uses_configured_cwd_before_runtime_fallback() -> anyhow::R }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; let expected_cwd = expected_cwd @@ -656,7 +656,7 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() - }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; let expected_cwd = expected_cwd @@ -775,7 +775,7 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -873,7 +873,7 @@ async fn stdio_mcp_parallel_tool_calls_default_false_runs_serially() -> anyhow:: }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -990,7 +990,7 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -1071,7 +1071,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -1203,7 +1203,7 @@ async fn stdio_image_responses_preserve_original_detail_metadata() -> anyhow::Re }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; wait_for_mcp_server(&fixture, server_name).await?; @@ -1338,7 +1338,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1440,7 +1440,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture .codex @@ -1558,7 +1558,7 @@ async fn stdio_server_propagates_explicit_local_env_var_source() -> anyhow::Resu }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1650,7 +1650,7 @@ async fn remote_stdio_env_var_source_does_not_copy_local_env() -> anyhow::Result }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; fixture @@ -1833,7 +1833,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; // Phase 4: submit the user turn that should trigger the MCP tool call. fixture @@ -2019,7 +2019,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { }, ); }) - .build_remote_aware(&server) + .build_with_remote_env(&server) .await?; // Phase 5: wait for MCP startup before the turn is submitted, which keeps // failures tied to server startup/discovery. diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 894110a6fb..6434f9c533 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -50,7 +50,7 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let mut builder = test_codex().with_workspace_setup(move |cwd, fs| async move { write_repo_skill(cwd, fs, "demo", "demo skill", skill_body).await }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let skill_path = test .config diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 76ed9cdcbf..231d14f770 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -379,7 +379,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let cwd = test.config.cwd.to_path_buf(); let call_id = "uexec-begin-event"; @@ -438,7 +438,7 @@ async fn unified_exec_resolves_relative_workdir() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let workdir_rel = std::path::PathBuf::from("uexec_relative_workdir"); let workdir = create_workspace_directory(&test, &workdir_rel).await?; @@ -507,7 +507,7 @@ async fn unified_exec_respects_workdir_override() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let workdir = create_workspace_directory(&test, "uexec_workdir_test").await?; @@ -572,7 +572,7 @@ async fn unified_exec_emits_exec_command_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-end-event"; let args = json!({ @@ -645,7 +645,7 @@ async fn unified_exec_emits_output_delta_for_exec_command() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-delta-1"; let args = json!({ @@ -703,7 +703,7 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-full-lifecycle"; // This timing force the long-standing PTY @@ -910,7 +910,7 @@ allow_local_binding = true PermissionProfile::from_legacy_sandbox_policy(&sandbox_policy_for_config), ); }); - let test = builder.build_remote_aware(server).await?; + let test = builder.build_with_remote_env(server).await?; assert!( test.config.permissions.network.is_some(), "expected managed network proxy config to be present" @@ -991,7 +991,7 @@ async fn unified_exec_emits_terminal_interaction_for_write_stdin() -> Result<()> .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-open"; let open_args = json!({ @@ -1074,7 +1074,7 @@ async fn unified_exec_terminal_interaction_captures_delayed_output() -> Result<( .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-delayed-open"; let open_args = json!({ @@ -1253,7 +1253,7 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let open_call_id = "uexec-open-session"; let open_args = json!({ @@ -1367,7 +1367,7 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-metadata"; let args = serde_json::json!({ @@ -1462,7 +1462,7 @@ async fn exec_command_clamps_model_requested_max_output_tokens_to_policy() -> Re .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-clamped-max-output"; let args = serde_json::json!({ @@ -1524,7 +1524,7 @@ async fn write_stdin_clamps_model_requested_max_output_tokens_to_policy() -> Res .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-stdin-clamp-start"; let start_args = serde_json::json!({ @@ -1611,7 +1611,7 @@ async fn unified_exec_defaults_to_pipe() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-default-pipe"; let args = serde_json::json!({ @@ -1680,7 +1680,7 @@ async fn unified_exec_can_enable_tty() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-tty-enabled"; let args = serde_json::json!({ @@ -1746,7 +1746,7 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec-early-exit"; let args = serde_json::json!({ @@ -1829,7 +1829,7 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-cat-start"; let send_call_id = "uexec-cat-send"; @@ -1983,7 +1983,7 @@ async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<() .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let start_call_id = "uexec-end-on-exit-start"; let start_args = serde_json::json!({ @@ -2266,7 +2266,7 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let first_call_id = "uexec-start"; let first_args = serde_json::json!({ @@ -2365,7 +2365,7 @@ async fn unified_exec_streams_after_lagged_output() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let script = r#"python3 - <<'PY' import sys @@ -2485,7 +2485,7 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let first_call_id = "uexec-timeout"; let first_args = serde_json::json!({ @@ -2574,7 +2574,7 @@ async fn unified_exec_formats_large_output_summary() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let script = r#"python3 - <<'PY' import sys @@ -3002,7 +3002,7 @@ async fn unified_exec_runs_on_all_platforms() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let call_id = "uexec"; let args = serde_json::json!({ @@ -3066,7 +3066,7 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { .enable(Feature::UnifiedExec) .expect("test config should allow feature update"); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; const MAX_SESSIONS_FOR_TEST: i32 = 64; const FILLER_SESSIONS: i32 = MAX_SESSIONS_FOR_TEST - 1; diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 4ff2062b40..5d5673b011 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -174,7 +174,7 @@ async fn assert_user_turn_local_image_resizes_to( let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -266,7 +266,7 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -562,7 +562,7 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<() let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_and_local_env(&server).await?; let local_cwd = TempDir::new()?; fs::write(local_cwd.path().join("remote.png"), b"not a remote image")?; let local_selection = TurnEnvironmentSelection { @@ -582,9 +582,7 @@ async fn view_image_routes_to_selected_remote_environment() -> anyhow::Result<() /*sandbox*/ None, ) .await?; - let png = BASE64_STANDARD.decode( - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=", - )?; + let png = png_bytes(/*width*/ 1, /*height*/ 1, [0, 255, 0, 255])?; test.fs() .write_file(&image_path, png, /*sandbox*/ None) .await?; @@ -664,7 +662,7 @@ async fn view_image_tool_can_preserve_original_resolution_when_requested_on_gpt5 let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -755,7 +753,7 @@ async fn view_image_tool_errors_clearly_for_unsupported_detail_values() -> anyho let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -833,7 +831,7 @@ async fn view_image_tool_treats_null_detail_as_omitted() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -923,7 +921,7 @@ async fn view_image_tool_resizes_when_model_lacks_original_detail_support() -> a let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.2"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1017,7 +1015,7 @@ async fn view_image_tool_does_not_force_original_resolution_with_capability_only let server = start_mock_server().await; let mut builder = test_codex().with_model("gpt-5.3-codex"); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1108,7 +1106,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1178,7 +1176,7 @@ async fn view_image_tool_errors_for_non_image_files() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, @@ -1255,7 +1253,7 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { let server = start_mock_server().await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, config, @@ -1385,7 +1383,7 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an .with_config(|config| { config.model = Some(model_slug.to_string()); }); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, .. } = &test; let rel_path = "assets/example.png"; @@ -1472,7 +1470,7 @@ async fn replaces_invalid_local_image_after_bad_request() -> anyhow::Result<()> let completion_mock = responses::mount_sse_once(&server, success_response).await; let mut builder = test_codex(); - let test = builder.build_remote_aware(&server).await?; + let test = builder.build_with_remote_env(&server).await?; let TestCodex { codex, session_configured, diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 7e4a3fb056..1d7d6e75f5 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -135,6 +135,20 @@ impl EnvironmentManager { } } + /// Builds a test-only manager that keeps the provider default while also + /// allowing tests to select the local environment explicitly. + pub async fn create_for_tests_with_local( + exec_server_url: Option, + local_runtime_paths: ExecServerRuntimePaths, + ) -> Self { + let mut snapshot = DefaultEnvironmentProvider::new(exec_server_url).snapshot_inner(); + snapshot.include_local = true; + match Self::from_provider_snapshot(snapshot, local_runtime_paths) { + Ok(manager) => manager, + Err(err) => panic!("test provider with local should create valid environments: {err}"), + } + } + /// Builds a manager from a provider-supplied startup snapshot. pub async fn from_provider

( provider: &P,