From 6c51bf0c7c4b14fb0be97e7454df1cfa9b3f5b1f Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 27 Apr 2026 09:51:12 -0700 Subject: [PATCH 01/11] Hide rewind preview when no user message exists (#19510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Fixes #19508. In a fresh TUI session, pressing `Esc` twice entered the rewind transcript overlay even though there was no user message to rewind to. That produced an empty header-only transcript view and exposed a rewind flow that could not select a valid target. ## What changed The backtrack flow now checks whether a user-message rewind target exists before opening the transcript preview. If no target exists, Codex stays in the main TUI and shows `No previous message to edit.` instead of opening an empty overlay. The same guard applies when starting rewind preview from the transcript overlay, and the first `Esc` no longer advertises the “edit previous message” hint when there is no previous message available. Snapshot coverage was added for the unavailable rewind info message, along with a small target-detection test. --- codex-rs/tui/src/app_backtrack.rs | 78 ++++++++++++++++++- ...ack_unavailable_info_message_snapshot.snap | 5 ++ 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__app_backtrack__tests__backtrack_unavailable_info_message_snapshot.snap diff --git a/codex-rs/tui/src/app_backtrack.rs b/codex-rs/tui/src/app_backtrack.rs index da1f82e626..2afcaa3b53 100644 --- a/codex-rs/tui/src/app_backtrack.rs +++ b/codex-rs/tui/src/app_backtrack.rs @@ -10,7 +10,8 @@ //! //! Backtrack operates as a small state machine: //! - The first `Esc` in the main view "primes" the feature and captures a base thread id. -//! - A subsequent `Esc` opens the transcript overlay (`Ctrl+T`) and highlights a user message. +//! - A subsequent `Esc` opens the transcript overlay (`Ctrl+T`) and highlights a user message when +//! there is a rewind target. //! - `Enter` requests a rollback from core and records a `pending_rollback` guard. //! - On `EventMsg::ThreadRolledBack`, we either finish an in-flight backtrack request or queue a //! rollback trim so it runs in event order with transcript inserts. @@ -44,6 +45,8 @@ use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; +const NO_PREVIOUS_MESSAGE_TO_EDIT: &str = "No previous message to edit."; + /// Aggregates all backtrack-related state used by the App. #[derive(Default)] pub(crate) struct BacktrackState { @@ -266,11 +269,21 @@ impl App { self.backtrack.primed = true; self.backtrack.nth_user_message = usize::MAX; self.backtrack.base_id = self.chat_widget.thread_id(); - self.chat_widget.show_esc_backtrack_hint(); + if has_backtrack_target(&self.transcript_cells) { + self.chat_widget.show_esc_backtrack_hint(); + } } /// Open overlay and begin backtrack preview flow (first step + highlight). fn open_backtrack_preview(&mut self, tui: &mut tui::Tui) { + if !has_backtrack_target(&self.transcript_cells) { + self.reset_backtrack_state(); + self.chat_widget + .add_info_message(NO_PREVIOUS_MESSAGE_TO_EDIT.to_string(), /*hint*/ None); + tui.frame_requester().schedule_frame(); + return; + } + self.open_transcript_overlay(tui); self.backtrack.overlay_preview_active = true; // Composer is hidden by overlay; clear its hint. @@ -280,6 +293,14 @@ impl App { /// When overlay is already open, begin preview mode and select latest user message. fn begin_overlay_backtrack_preview(&mut self, tui: &mut tui::Tui) { + if !has_backtrack_target(&self.transcript_cells) { + self.close_transcript_overlay(tui); + self.chat_widget + .add_info_message(NO_PREVIOUS_MESSAGE_TO_EDIT.to_string(), /*hint*/ None); + tui.frame_requester().schedule_frame(); + return; + } + self.backtrack.primed = true; self.backtrack.base_id = self.chat_widget.thread_id(); self.backtrack.overlay_preview_active = true; @@ -613,6 +634,10 @@ pub(crate) fn user_count(cells: &[Arc]) -> user_positions_iter(cells).count() } +fn has_backtrack_target(cells: &[Arc]) -> bool { + user_count(cells) > 0 +} + fn nth_user_position( cells: &[Arc], nth: usize, @@ -674,9 +699,22 @@ mod tests { use super::*; use crate::history_cell::AgentMessageCell; use crate::history_cell::HistoryCell; + use pretty_assertions::assert_eq; use ratatui::prelude::Line; use std::sync::Arc; + fn render_lines(lines: &[Line<'static>]) -> Vec { + lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect() + } + #[test] fn trim_transcript_for_first_user_drops_user_and_newer_cells() { let mut cells: Vec> = vec![ @@ -885,4 +923,40 @@ mod tests { assert_eq!(agent_group_count(&cells), 2); } + + #[test] + fn backtrack_target_requires_user_message() { + let mut cells: Vec> = vec![ + Arc::new(AgentMessageCell::new( + vec![Line::from("assistant")], + /*is_first_line*/ true, + )) as Arc, + Arc::new(crate::history_cell::new_info_event( + "Context compacted".to_string(), + /*hint*/ None, + )) as Arc, + ]; + + assert!(!has_backtrack_target(&cells)); + + cells.push(Arc::new(UserHistoryCell { + message: "hello".to_string(), + text_elements: Vec::new(), + local_image_paths: Vec::new(), + remote_image_urls: Vec::new(), + }) as Arc); + + assert!(has_backtrack_target(&cells)); + } + + #[test] + fn backtrack_unavailable_info_message_snapshot() { + let cell = crate::history_cell::new_info_event( + NO_PREVIOUS_MESSAGE_TO_EDIT.to_string(), + /*hint*/ None, + ); + let rendered = render_lines(&cell.display_lines(/*width*/ 80)).join("\n"); + + insta::assert_snapshot!(rendered); + } } diff --git a/codex-rs/tui/src/snapshots/codex_tui__app_backtrack__tests__backtrack_unavailable_info_message_snapshot.snap b/codex-rs/tui/src/snapshots/codex_tui__app_backtrack__tests__backtrack_unavailable_info_message_snapshot.snap new file mode 100644 index 0000000000..8e896e7d7c --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app_backtrack__tests__backtrack_unavailable_info_message_snapshot.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/app_backtrack.rs +expression: rendered +--- +• No previous message to edit. From 0e2300c02c72c8da0ea3e6976d84c48b980399c0 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 27 Apr 2026 09:54:25 -0700 Subject: [PATCH 02/11] Persist shell mode commands in prompt history (#19618) ## Why `!` shell commands are currently surfaced as "Bash mode", which is misleading for users running shells such as PowerShell or zsh. Those commands also bypass the persistent prompt history path, so they cannot be recalled after starting a new session. Fixes #19613. ## What changed - Rename the TUI footer label and related test wording from "Bash mode" to "Shell mode". - Persist accepted `!` shell commands to prompt history with the leading `!`, so recall restores the composer into shell mode across sessions. - Add coverage for immediate and queued shell-command submissions emitting the prompt-history update. ## Verification - `cargo test -p codex-tui bang_shell` - `cargo test -p codex-tui shell_command_uses_shell_accent_style` - `cargo test -p codex-tui footer_mode_snapshots` - `cargo insta pending-snapshots --manifest-path tui/Cargo.toml` Manually verified fix after confirming presence of bug prior to fix. --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 14 ++++++------- ...ooter_mode_shell_command_absorbs_bang.snap | 2 +- codex-rs/tui/src/chatwidget.rs | 21 +++++++++++++++++-- .../tui/src/chatwidget/tests/exec_flow.rs | 4 ++++ .../src/chatwidget/tests/slash_commands.rs | 9 +++++++- 5 files changed, 39 insertions(+), 11 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 9b67e29b57..1a0e39d9c3 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -1160,7 +1160,7 @@ impl ChatComposer { /// Convert canonical composer text into the textarea's internal representation. /// - /// Bash mode stores the leading `!` as prompt state instead of editable text, + /// Shell mode stores the leading `!` as prompt state instead of editable text, /// so full-buffer imports must absorb that prefix before rebuilding the textarea. fn imported_text_for_textarea( &mut self, @@ -2934,7 +2934,7 @@ impl ChatComposer { fn shell_mode_footer_line(&self) -> Option> { self.is_bang_shell_command() .then_some(()) - .map(|_| Line::from(vec![Span::from("Bash mode").light_red()])) + .map(|_| Line::from(vec![Span::from("Shell mode").light_red()])) } /// Applies any due `PasteBurst` flush at time `now`. @@ -4660,7 +4660,7 @@ mod tests { } #[test] - fn shell_command_uses_bash_accent_style() { + fn shell_command_uses_shell_accent_style() { let (tx, _rx) = unbounded_channel::(); let sender = AppEventSender::new(tx); let mut composer = ChatComposer::new( @@ -4688,11 +4688,11 @@ mod tests { let footer_text = (0..area.width) .map(|x| buf[(x, footer_y)].symbol().chars().next().unwrap_or(' ')) .collect::(); - let bash_label_x = footer_text - .find("Bash mode") - .expect("expected bash mode footer label"); + let shell_label_x = footer_text + .find("Shell mode") + .expect("expected shell mode footer label"); assert_eq!( - buf[(bash_label_x as u16, footer_y)].style().fg, + buf[(shell_label_x as u16, footer_y)].style().fg, Some(Color::LightRed) ); } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shell_command_absorbs_bang.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shell_command_absorbs_bang.snap index 2b44670df0..0b09860b59 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shell_command_absorbs_bang.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__footer_mode_shell_command_absorbs_bang.snap @@ -10,4 +10,4 @@ expression: terminal.backend() " " " " " " -" gpt-5.4 high fast · ~/code/codex-1 · Context 0% used Bash mode " +" gpt-5.4 high fast · ~/code/codex-1 · Context 0% used Shell mode " diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 1c08c77486..f5454c2bf3 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -6151,9 +6151,26 @@ impl ChatWidget { } } + fn submit_shell_command_with_history( + &mut self, + command: &str, + history_text: &str, + ) -> QueueDrain { + let drain = self.submit_shell_command(command); + if drain == QueueDrain::Stop { + self.submit_op(Op::AddToHistory { + text: history_text.to_string(), + }); + } + drain + } + fn submit_queued_shell_prompt(&mut self, user_message: UserMessage) -> QueueDrain { match user_message.text.strip_prefix('!') { - Some(command) => self.submit_shell_command(command), + Some(command) => { + let history_text = user_message.text.clone(); + self.submit_shell_command_with_history(command, &history_text) + } None => { self.submit_user_message(user_message); QueueDrain::Stop @@ -6249,7 +6266,7 @@ impl ChatWidget { if shell_escape_policy == ShellEscapePolicy::Allow && let Some(stripped) = text.strip_prefix('!') { - let app_command = match self.submit_shell_command(stripped) { + let app_command = match self.submit_shell_command_with_history(stripped, &text) { QueueDrain::Continue => None, QueueDrain::Stop => Some(AppCommand::run_user_shell_command( stripped.trim().to_string(), diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index 206564d06a..84d7cd721e 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -1038,6 +1038,10 @@ async fn bang_shell_enter_while_task_running_submits_run_user_shell_command() { Ok(Op::RunUserShellCommand { command }) => assert_eq!(command, "echo hi"), other => panic!("expected RunUserShellCommand op, got {other:?}"), } + assert_matches!( + op_rx.try_recv(), + Ok(Op::AddToHistory { text }) if text == "!echo hi" + ); assert_matches!(rx.try_recv(), Err(TryRecvError::Empty)); } diff --git a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs index 6da41423f2..f88846be94 100644 --- a/codex-rs/tui/src/chatwidget/tests/slash_commands.rs +++ b/codex-rs/tui/src/chatwidget/tests/slash_commands.rs @@ -210,6 +210,10 @@ async fn queued_bang_shell_dispatches_after_active_turn() { Ok(Op::RunUserShellCommand { command }) => assert_eq!(command, "echo hi"), other => panic!("expected queued shell command op, got {other:?}"), } + assert_matches!( + op_rx.try_recv(), + Ok(Op::AddToHistory { text }) if text == "!echo hi" + ); assert!(chat.queued_user_messages.is_empty()); } @@ -287,7 +291,10 @@ async fn queued_bang_shell_waits_for_user_shell_completion_before_next_input() { Ok(Op::RunUserShellCommand { command }) => assert_eq!(command, "echo hi"), other => panic!("expected queued shell command op, got {other:?}"), } - assert_matches!(op_rx.try_recv(), Err(TryRecvError::Empty)); + assert_matches!( + op_rx.try_recv(), + Ok(Op::AddToHistory { text }) if text == "!echo hi" + ); assert_eq!(chat.queued_user_messages.len(), 1); let begin = begin_exec_with_source( From 48dd7b58f09d35022d9163c720b6cef1e6d77c6a Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 27 Apr 2026 10:07:15 -0700 Subject: [PATCH 03/11] Render delegated patch approval details (#19709) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Fixes #19632. When a delegated agent requests approval for an in-progress file change, the parent TUI handles that request from an inactive thread. The app server already sent the `FileChange` item with the proposed diff, but the inactive-thread approval path was not recovering and rendering it the same way as the active-thread path. The result was an inconsistent approval prompt: main-thread edits show a normal patch preview history item before the approval modal, while delegated edits did not show that preview in the transcript flow. ## What Changed - Recover buffered or historical `FileChange` item changes when building inactive-thread file-change approval requests. - Reuse the app-server file-change conversion helper for both live transcript rendering and inactive-thread approvals. - Render recovered delegated patches as a normal patch preview history cell before the approval modal. - Keep apply-patch approval modals focused on the decision prompt and optional metadata; they do not render a synthetic command line or embed the diff body. ## Manual Repro And Verification I manually reproduced the issue using a file under `~/Desktop` so the write would require approval. Before the fix: 1. Ask the main thread: `Use apply_patch, not shell redirection or Python, to create ~/Desktop/bug1.txt with three short lines.` 2. Observe the expected TUI shape: the transcript shows a normal patch preview such as `• Added ~/Desktop/bug1.txt (+N -0)` above the approval modal, and the modal contains only the approval prompt/options without a synthetic command line. 3. Ask for the delegated path: `Spawn a worker. Have it use apply_patch, not shell redirection or Python, to create ~/Desktop/bug1.txt with four short lines.` 4. Observe the delegated approval is inconsistent: the parent view does not render the proposed patch as the normal transcript preview before the modal, so the diff context is missing from the stream or appears inside the modal instead of in the history flow. After the fix: 1. Repeat the delegated worker prompt with `apply_patch`. 2. Confirm the parent view renders the same normal patch preview history cell (`• Added ~/Desktop/bug1.txt (+N -0)` plus the diff) immediately before the approval modal. 3. Confirm the approval modal remains focused on the decision prompt. For delegated approvals it may show the worker thread label, but it should not show a `$ apply_patch` command line or embed the diff body in the modal. --- codex-rs/tui/src/app/tests.rs | 74 +++++++++++++++++++ codex-rs/tui/src/app/thread_events.rs | 48 ++++++++++++ codex-rs/tui/src/app/thread_routing.rs | 35 ++++++++- .../src/app_server_approval_conversions.rs | 51 +++++++++++++ .../tui/src/bottom_pane/approval_overlay.rs | 35 +++++++-- codex-rs/tui/src/chatwidget.rs | 35 +-------- ...atwidget__tests__approval_modal_patch.snap | 11 +-- .../tui/src/chatwidget/tests/app_server.rs | 19 ----- .../tui/src/chatwidget/tests/exec_flow.rs | 61 +++------------ 9 files changed, 253 insertions(+), 116 deletions(-) diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index f7d24e1e0e..d9d3b0c026 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -30,6 +30,9 @@ use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::AgentMessageDeltaNotification; use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::ConfigWarningNotification; +use codex_app_server_protocol::FileChangeRequestApprovalParams; +use codex_app_server_protocol::FileUpdateChange; +use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::McpServerStartupState; use codex_app_server_protocol::McpServerStatusUpdatedNotification; @@ -38,6 +41,7 @@ use codex_app_server_protocol::NetworkApprovalProtocol as AppServerNetworkApprov use codex_app_server_protocol::NetworkPolicyAmendment as AppServerNetworkPolicyAmendment; use codex_app_server_protocol::NetworkPolicyRuleAction as AppServerNetworkPolicyRuleAction; use codex_app_server_protocol::NonSteerableTurnKind as AppServerNonSteerableTurnKind; +use codex_app_server_protocol::PatchChangeKind; use codex_app_server_protocol::PermissionsRequestApprovalParams; use codex_app_server_protocol::RequestId as AppServerRequestId; use codex_app_server_protocol::ServerNotification; @@ -70,6 +74,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::protocol::RolloutItem; @@ -2522,6 +2527,75 @@ async fn inactive_thread_exec_approval_splits_shell_wrapped_command() { ); } +#[tokio::test] +async fn inactive_thread_file_change_approval_recovers_buffered_changes() { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let thread_id = ThreadId::new(); + app.enqueue_thread_notification( + thread_id, + ServerNotification::ItemStarted(ItemStartedNotification { + thread_id: thread_id.to_string(), + turn_id: "turn-approval".to_string(), + item: ThreadItem::FileChange { + id: "patch-approval".to_string(), + changes: vec![FileUpdateChange { + path: "README.md".to_string(), + kind: PatchChangeKind::Add, + diff: "hello\n".to_string(), + }], + status: codex_app_server_protocol::PatchApplyStatus::InProgress, + }, + }), + ) + .await + .expect("enqueue file change item"); + + let request = ServerRequest::FileChangeRequestApproval { + request_id: AppServerRequestId::Integer(9), + params: FileChangeRequestApprovalParams { + thread_id: thread_id.to_string(), + turn_id: "turn-approval".to_string(), + item_id: "patch-approval".to_string(), + reason: Some("command failed; retry without sandbox?".to_string()), + grant_root: None, + }, + }; + + let request = app + .interactive_request_for_thread_request(thread_id, &request) + .await + .expect("expected file change approval request"); + + let ThreadInteractiveRequest::Approval(ApprovalRequest::ApplyPatch { + changes, reason, .. + }) = &request + else { + panic!("expected apply-patch approval request"); + }; + assert_eq!( + changes, + &HashMap::from([( + PathBuf::from("README.md"), + FileChange::Add { + content: "hello\n".to_string(), + }, + )]) + ); + assert_eq!( + reason, + &Some("command failed; retry without sandbox?".to_string()) + ); + + app.push_thread_interactive_request(request); + let cell = match app_event_rx.try_recv() { + Ok(AppEvent::InsertHistoryCell(cell)) => cell, + other => panic!("expected patch preview history cell, saw {other:?}"), + }; + let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80)); + assert!(rendered.contains("• Added README.md (+1 -0)")); + assert!(rendered.contains("1 +hello")); +} + #[tokio::test] async fn inactive_thread_permissions_approval_preserves_file_system_permissions() { let app = make_test_app().await; diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index daf743b467..9bbb41fbd1 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -157,6 +157,40 @@ impl ThreadEventStore { .collect() } + pub(super) fn file_change_changes( + &self, + turn_id: &str, + item_id: &str, + ) -> Option> { + self.buffer + .iter() + .rev() + .find_map(|event| match event { + ThreadBufferedEvent::Notification(ServerNotification::ItemStarted( + notification, + )) if turn_id_matches(turn_id, ¬ification.turn_id) => { + file_change_item_changes(¬ification.item, item_id) + } + ThreadBufferedEvent::Notification(ServerNotification::ItemCompleted( + notification, + )) if turn_id_matches(turn_id, ¬ification.turn_id) => { + file_change_item_changes(¬ification.item, item_id) + } + ThreadBufferedEvent::Request(_) + | ThreadBufferedEvent::Notification(_) + | ThreadBufferedEvent::HistoryEntryResponse(_) + | ThreadBufferedEvent::FeedbackSubmission(_) => None, + }) + .or_else(|| { + self.turns + .iter() + .rev() + .filter(|turn| turn_id_matches(turn_id, &turn.id)) + .flat_map(|turn| turn.items.iter().rev()) + .find_map(|item| file_change_item_changes(item, item_id)) + }) + } + pub(super) fn apply_thread_rollback(&mut self, response: &ThreadRollbackResponse) { self.turns = response.thread.turns.clone(); self.buffer.clear(); @@ -231,6 +265,20 @@ impl ThreadEventStore { } } +fn turn_id_matches(request_turn_id: &str, candidate_turn_id: &str) -> bool { + request_turn_id.is_empty() || request_turn_id == candidate_turn_id +} + +fn file_change_item_changes( + item: &ThreadItem, + item_id: &str, +) -> Option> { + match item { + ThreadItem::FileChange { id, changes, .. } if id == item_id => Some(changes.clone()), + _ => None, + } +} + #[derive(Debug)] pub(super) struct ThreadEventChannel { pub(super) sender: mpsc::Sender, diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 5f0f52c2c7..345e560d41 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -194,6 +194,17 @@ impl App { store.session.as_ref().map(|session| session.cwd.clone()) } + async fn thread_file_change_changes( + &self, + thread_id: ThreadId, + turn_id: &str, + item_id: &str, + ) -> Option> { + let channel = self.thread_event_channels.get(&thread_id)?; + let store = channel.store.lock().await; + store.file_change_changes(turn_id, item_id) + } + pub(super) async fn interactive_request_for_thread_request( &self, thread_id: ThreadId, @@ -264,7 +275,11 @@ impl App { .thread_cwd(thread_id) .await .unwrap_or_else(|| self.config.cwd.clone()), - changes: HashMap::new(), + changes: self + .thread_file_change_changes(thread_id, ¶ms.turn_id, ¶ms.item_id) + .await + .map(crate::app_server_approval_conversions::file_update_changes_to_core) + .unwrap_or_default(), }), ), ServerRequest::McpServerElicitationRequest { request_id, params } => { @@ -311,6 +326,7 @@ impl App { pub(super) fn push_thread_interactive_request(&mut self, request: ThreadInteractiveRequest) { match request { ThreadInteractiveRequest::Approval(request) => { + self.render_inactive_patch_preview(&request); self.chat_widget.push_approval_request(request); } ThreadInteractiveRequest::McpServerElicitation(request) => { @@ -320,6 +336,23 @@ impl App { } } + fn render_inactive_patch_preview(&mut self, request: &ApprovalRequest) { + let ApprovalRequest::ApplyPatch { + thread_label, + cwd, + changes, + .. + } = request + else { + return; + }; + if thread_label.is_none() || changes.is_empty() { + return; + } + self.chat_widget + .add_to_history(history_cell::new_patch_event(changes.clone(), cwd)); + } + pub(super) async fn pending_inactive_thread_requests(&self) -> Vec<(ThreadId, ServerRequest)> { let channels: Vec<(ThreadId, Arc>)> = self .thread_event_channels diff --git a/codex-rs/tui/src/app_server_approval_conversions.rs b/codex-rs/tui/src/app_server_approval_conversions.rs index a0d86db7d6..894bd36ed7 100644 --- a/codex-rs/tui/src/app_server_approval_conversions.rs +++ b/codex-rs/tui/src/app_server_approval_conversions.rs @@ -1,9 +1,14 @@ use codex_app_server_protocol::AdditionalNetworkPermissions; +use codex_app_server_protocol::FileUpdateChange; use codex_app_server_protocol::GrantedPermissionProfile; use codex_app_server_protocol::NetworkApprovalContext as AppServerNetworkApprovalContext; +use codex_app_server_protocol::PatchChangeKind; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; +use std::collections::HashMap; +use std::path::PathBuf; pub(crate) fn network_approval_context_to_core( value: AppServerNetworkApprovalContext, @@ -38,21 +43,50 @@ pub(crate) fn granted_permission_profile_from_request( } } +pub(crate) fn file_update_changes_to_core( + changes: Vec, +) -> HashMap { + changes + .into_iter() + .map(|change| { + let path = PathBuf::from(change.path); + let file_change = match change.kind { + PatchChangeKind::Add => FileChange::Add { + content: change.diff, + }, + PatchChangeKind::Delete => FileChange::Delete { + content: change.diff, + }, + PatchChangeKind::Update { move_path } => FileChange::Update { + unified_diff: change.diff, + move_path, + }, + }; + (path, file_change) + }) + .collect() +} + #[cfg(test)] mod tests { + use super::file_update_changes_to_core; use super::granted_permission_profile_from_request; use super::network_approval_context_to_core; + use codex_app_server_protocol::FileUpdateChange; + use codex_app_server_protocol::PatchChangeKind; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSpecialPath; + use codex_protocol::protocol::FileChange; use codex_protocol::protocol::NetworkApprovalContext; use codex_protocol::protocol::NetworkApprovalProtocol; use codex_protocol::request_permissions::RequestPermissionProfile as CoreRequestPermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; + use std::collections::HashMap; use std::path::PathBuf; fn absolute_path(path: &str) -> AbsolutePathBuf { @@ -73,6 +107,23 @@ mod tests { ); } + #[test] + fn converts_file_update_changes_to_core() { + assert_eq!( + file_update_changes_to_core(vec![FileUpdateChange { + path: "foo.txt".to_string(), + kind: PatchChangeKind::Add, + diff: "hello\n".to_string(), + }]), + HashMap::from([( + PathBuf::from("foo.txt"), + FileChange::Add { + content: "hello\n".to_string(), + }, + )]) + ); + } + #[test] fn converts_request_permissions_into_granted_permissions() { assert_eq!( diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 57d819e560..ce3b04b33b 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -9,7 +9,6 @@ use crate::bottom_pane::CancellationEvent; use crate::bottom_pane::list_selection_view::ListSelectionView; use crate::bottom_pane::list_selection_view::SelectionItem; use crate::bottom_pane::list_selection_view::SelectionViewParams; -use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; use crate::key_hint; @@ -633,8 +632,6 @@ fn build_header(request: &ApprovalRequest) -> Box { ApprovalRequest::ApplyPatch { thread_label, reason, - cwd, - changes, .. } => { let mut header: Vec> = Vec::new(); @@ -643,11 +640,13 @@ fn build_header(request: &ApprovalRequest) -> Box { "Thread: ".into(), thread_label.clone().bold(), ]))); - header.push(Box::new(Line::from(""))); } if let Some(reason) = reason && !reason.is_empty() { + if !header.is_empty() { + header.push(Box::new(Line::from(""))); + } header.push(Box::new( Paragraph::new(Line::from_iter([ "Reason: ".into(), @@ -655,9 +654,7 @@ fn build_header(request: &ApprovalRequest) -> Box { ])) .wrap(Wrap { trim: false }), )); - header.push(Box::new(Line::from(""))); } - header.push(DiffSummary::new(changes.clone(), cwd.clone()).into()); Box::new(ColumnRenderable::with(header)) } ApprovalRequest::McpElicitation { @@ -1556,6 +1553,32 @@ mod tests { ); } + #[test] + fn apply_patch_prompt_with_thread_label_omits_command_line() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut changes = HashMap::new(); + changes.insert( + PathBuf::from("bug1.txt"), + FileChange::Add { + content: "one\ntwo\nthree\n".to_string(), + }, + ); + let request = ApprovalRequest::ApplyPatch { + thread_id: ThreadId::new(), + thread_label: Some("Banach [worker]".to_string()), + id: "test".to_string(), + reason: None, + cwd: absolute_path("/tmp"), + changes, + }; + let view = ApprovalOverlay::new(request, tx, Features::with_defaults()); + let rendered = render_overlay_lines(&view, /*width*/ 120); + assert!(rendered.contains("Thread: Banach [worker]")); + assert!(rendered.contains("o to open thread")); + assert!(!rendered.contains("$ apply_patch")); + } + #[test] fn network_exec_prompt_title_includes_host() { let (tx, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index f5454c2bf3..8513fe8816 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -47,6 +47,7 @@ use self::realtime::PendingSteerCompareKey; use crate::app::app_server_requests::ResolvedAppServerRequest; use crate::app_command::AppCommand; use crate::app_event::RealtimeAudioDeviceKind; +use crate::app_server_approval_conversions::file_update_changes_to_core; use crate::app_server_approval_conversions::network_approval_context_to_core; use crate::app_server_session::ThreadSessionState; #[cfg(not(target_os = "linux"))] @@ -1774,36 +1775,6 @@ fn patch_approval_request_from_params( } } -fn app_server_patch_changes_to_core( - changes: Vec, -) -> HashMap { - changes - .into_iter() - .map(|change| { - let path = PathBuf::from(change.path); - let file_change = match change.kind { - codex_app_server_protocol::PatchChangeKind::Add => { - codex_protocol::protocol::FileChange::Add { - content: change.diff, - } - } - codex_app_server_protocol::PatchChangeKind::Delete => { - codex_protocol::protocol::FileChange::Delete { - content: change.diff, - } - } - codex_app_server_protocol::PatchChangeKind::Update { move_path } => { - codex_protocol::protocol::FileChange::Update { - unified_diff: change.diff, - move_path, - } - } - }; - (path, file_change) - }) - .collect() -} - fn app_server_collab_thread_id_to_core(thread_id: &str) -> Option { match ThreadId::from_string(thread_id) { Ok(thread_id) => Some(thread_id), @@ -6797,7 +6768,7 @@ impl ChatWidget { status, codex_app_server_protocol::PatchApplyStatus::Failed ), - changes: app_server_patch_changes_to_core(changes), + changes: file_update_changes_to_core(changes), status: match status { codex_app_server_protocol::PatchApplyStatus::Completed => { codex_protocol::protocol::PatchApplyStatus::Completed @@ -7356,7 +7327,7 @@ impl ChatWidget { call_id: id, turn_id: notification.turn_id, auto_approved: false, - changes: app_server_patch_changes_to_core(changes), + changes: file_update_changes_to_core(changes), }); } ThreadItem::McpToolCall { diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index e394605dcc..8635b66682 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -1,18 +1,11 @@ --- -source: tui/src/chatwidget/tests.rs -expression: terminal.backend().vt100().screen().contents() +source: tui/src/chatwidget/tests/exec_flow.rs +expression: contents --- - - Would you like to make the following edits? Reason: The model wants to apply changes - README.md (+2 -0) - - 1 +hello - 2 +world - › 1. Yes, proceed (y) 2. Yes, and don't ask again for these files (a) 3. No, and tell Codex what to do differently (esc) diff --git a/codex-rs/tui/src/chatwidget/tests/app_server.rs b/codex-rs/tui/src/chatwidget/tests/app_server.rs index b9ec1d871b..d4fa2df451 100644 --- a/codex-rs/tui/src/chatwidget/tests/app_server.rs +++ b/codex-rs/tui/src/chatwidget/tests/app_server.rs @@ -349,25 +349,6 @@ async fn live_app_server_command_execution_strips_shell_wrapper() { ); } -#[test] -fn app_server_patch_changes_to_core_preserves_diffs() { - let changes = app_server_patch_changes_to_core(vec![FileUpdateChange { - path: "foo.txt".to_string(), - kind: PatchChangeKind::Add, - diff: "hello\n".to_string(), - }]); - - assert_eq!( - changes, - HashMap::from([( - PathBuf::from("foo.txt"), - FileChange::Add { - content: "hello\n".to_string(), - }, - )]) - ); -} - #[tokio::test] async fn live_app_server_collab_wait_items_render_history() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index 84d7cd721e..4d87914aa4 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -1321,10 +1321,9 @@ async fn approval_modal_patch_snapshot() -> anyhow::Result<()> { terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw patch approval modal"); - assert_chatwidget_snapshot!( - "approval_modal_patch", - terminal.backend().vt100().screen().contents() - ); + let contents = terminal.backend().vt100().screen().contents(); + assert!(!contents.contains("$ apply_patch")); + assert_chatwidget_snapshot!("approval_modal_patch", contents); Ok(()) } @@ -1469,28 +1468,11 @@ async fn apply_patch_events_emit_history_cells() { id: "s1".into(), msg: EventMsg::ApplyPatchApprovalRequest(ev), }); - let cells = drain_insert_history(&mut rx); assert!( - cells.is_empty(), + drain_insert_history(&mut rx).is_empty(), "expected approval request to surface via modal without emitting history cells" ); - let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); - let mut buf = ratatui::buffer::Buffer::empty(area); - chat.render(area, &mut buf); - let mut saw_summary = false; - for y in 0..area.height { - let mut row = String::new(); - for x in 0..area.width { - row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); - } - if row.contains("foo.txt (+1 -0)") { - saw_summary = true; - break; - } - } - assert!(saw_summary, "expected approval modal to show diff summary"); - // 2) Begin apply -> per-file apply block cell (no global header) let mut changes2 = HashMap::new(); changes2.insert( @@ -1820,7 +1802,7 @@ async fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> { } #[tokio::test] -async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { +async fn apply_patch_request_omits_diff_summary_from_modal() -> anyhow::Result<()> { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; // Ensure we are in OnRequest so an approval is surfaced @@ -1849,43 +1831,24 @@ async fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { }), }); - // No history entries yet; the modal should contain the diff summary - let cells = drain_insert_history(&mut rx); assert!( - cells.is_empty(), + drain_insert_history(&mut rx).is_empty(), "expected approval request to render via modal instead of history" ); let area = Rect::new(0, 0, 80, chat.desired_height(/*width*/ 80)); let mut buf = ratatui::buffer::Buffer::empty(area); chat.render(area, &mut buf); - - let mut saw_header = false; - let mut saw_line1 = false; - let mut saw_line2 = false; + let mut contents = String::new(); for y in 0..area.height { - let mut row = String::new(); for x in 0..area.width { - row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); - } - if row.contains("README.md (+2 -0)") { - saw_header = true; - } - if row.contains("+line one") { - saw_line1 = true; - } - if row.contains("+line two") { - saw_line2 = true; - } - if saw_header && saw_line1 && saw_line2 { - break; + contents.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); } + contents.push('\n'); } - assert!(saw_header, "expected modal to show diff header with totals"); - assert!( - saw_line1 && saw_line2, - "expected modal to show per-line diff summary" - ); + assert!(!contents.contains("README.md (+2 -0)")); + assert!(!contents.contains("+line one")); + assert!(!contents.contains("+line two")); Ok(()) } From 4ed22fc7d2bd090ffe308662b780bb712fe29773 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 27 Apr 2026 10:18:25 -0700 Subject: [PATCH 04/11] Streamline plugin, apps, and skills handlers (#19490) ## Why The plugin, app, and skills handlers had a lot of repeated `send_error`/`return` branches that made the success path hard to scan. This slice keeps behavior the same while moving fallible steps into local response-producing helpers, so the request boundary can send one result. ## What Changed - Converted plugin list/install/uninstall handlers in `codex-rs/app-server/src/codex_message_processor/plugins.rs` to return `Result<*Response, JSONRPCErrorError>` from helper methods and call `send_result` once. - Added local error-mapping helpers for plugin install/uninstall and marketplace failures. - Applied the same mechanical shape to app list, skills list/config, and marketplace add/remove/upgrade handlers in `codex-rs/app-server/src/codex_message_processor.rs`. ## Verification - `cargo check -p codex-app-server` - `cargo test -p codex-app-server --test all v2::app_list -- --test-threads=1` - `cargo test -p codex-app-server --test all v2::plugin_ -- --test-threads=1` - `cargo test -p codex-app-server --test all v2::skills_list -- --test-threads=1` --- .../app-server/src/codex_message_processor.rs | 367 ++++-------- .../src/codex_message_processor/plugins.rs | 559 +++++++----------- 2 files changed, 333 insertions(+), 593 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index c86d414359..563a87c3bd 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -7,6 +7,7 @@ use crate::error_code::INPUT_TOO_LARGE_ERROR_CODE; use crate::error_code::INTERNAL_ERROR_CODE; use crate::error_code::INVALID_PARAMS_ERROR_CODE; use crate::error_code::INVALID_REQUEST_ERROR_CODE; +use crate::error_code::invalid_params; use crate::fuzzy_file_search::FuzzyFileSearchSession; use crate::fuzzy_file_search::run_fuzzy_file_search; use crate::fuzzy_file_search::start_fuzzy_file_search_session; @@ -6323,6 +6324,15 @@ impl CodexMessageProcessor { self.outgoing.send_error(request_id, error).await; } + async fn send_internal_error(&self, request_id: ConnectionRequestId, message: String) { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message, + data: None, + }; + self.outgoing.send_error(request_id, error).await; + } + fn input_too_large_error(actual_chars: usize) -> JSONRPCErrorError { JSONRPCErrorError { code: INVALID_PARAMS_ERROR_CODE, @@ -6345,41 +6355,6 @@ impl CodexMessageProcessor { Ok(()) } - async fn send_internal_error(&self, request_id: ConnectionRequestId, message: String) { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message, - data: None, - }; - self.outgoing.send_error(request_id, error).await; - } - - async fn send_marketplace_error( - &self, - request_id: ConnectionRequestId, - err: MarketplaceError, - action: &str, - ) { - match err { - MarketplaceError::MarketplaceNotFound { .. } => { - self.send_invalid_request_error(request_id, err.to_string()) - .await; - } - MarketplaceError::Io { .. } => { - self.send_internal_error(request_id, format!("failed to {action}: {err}")) - .await; - } - MarketplaceError::InvalidMarketplaceFile { .. } - | MarketplaceError::PluginNotFound { .. } - | MarketplaceError::PluginNotAvailable { .. } - | MarketplaceError::PluginsDisabled - | MarketplaceError::InvalidPlugin(_) => { - self.send_invalid_request_error(request_id, err.to_string()) - .await; - } - } - } - async fn wait_for_thread_shutdown(thread: &Arc) -> ThreadShutdownResult { match tokio::time::timeout(Duration::from_secs(10), thread.shutdown_and_wait()).await { Ok(Ok(())) => ThreadShutdownResult::Complete, @@ -6458,34 +6433,33 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: ThreadUnsubscribeParams, ) { - let thread_id = match ThreadId::from_string(¶ms.thread_id) { - Ok(id) => id, - Err(err) => { - self.send_invalid_request_error(request_id, format!("invalid thread id: {err}")) - .await; - return; - } - }; + let result = self + .thread_unsubscribe_response(params, request_id.connection_id) + .await; + self.outgoing.send_result(request_id, result).await; + } + + async fn thread_unsubscribe_response( + &self, + params: ThreadUnsubscribeParams, + connection_id: ConnectionId, + ) -> Result { + let thread_id = ThreadId::from_string(¶ms.thread_id) + .map_err(|err| invalid_request(format!("invalid thread id: {err}")))?; if self.thread_manager.get_thread(thread_id).await.is_err() { // Reconcile stale app-server bookkeeping when the thread has already been // removed from the core manager. This keeps loaded-status/subscription state // consistent with the source of truth before reporting NotLoaded. self.finalize_thread_teardown(thread_id).await; - self.outgoing - .send_response( - request_id, - ThreadUnsubscribeResponse { - status: ThreadUnsubscribeStatus::NotLoaded, - }, - ) - .await; - return; + return Ok(ThreadUnsubscribeResponse { + status: ThreadUnsubscribeStatus::NotLoaded, + }); }; let was_subscribed = self .thread_state_manager - .unsubscribe_connection_from_thread(thread_id, request_id.connection_id) + .unsubscribe_connection_from_thread(thread_id, connection_id) .await; let status = if was_subscribed { @@ -6493,9 +6467,7 @@ impl CodexMessageProcessor { } else { ThreadUnsubscribeStatus::NotSubscribed }; - self.outgoing - .send_response(request_id, ThreadUnsubscribeResponse { status }) - .await; + Ok(ThreadUnsubscribeResponse { status }) } async fn prepare_thread_for_archive(&self, thread_id: ThreadId) { @@ -6589,6 +6561,16 @@ impl CodexMessageProcessor { config: Config, environment_manager: Arc, ) { + let result = Self::apps_list_response(&outgoing, params, config, environment_manager).await; + outgoing.send_result(request_id, result).await; + } + + async fn apps_list_response( + outgoing: &Arc, + params: AppsListParams, + config: Config, + environment_manager: Arc, + ) -> Result { let AppsListParams { cursor, limit, @@ -6598,15 +6580,7 @@ impl CodexMessageProcessor { let start = match cursor { Some(cursor) => match cursor.parse::() { Ok(idx) => idx, - Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid cursor: {cursor}"), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; - } + Err(_) => return Err(invalid_request(format!("invalid cursor: {cursor}"))), }, None => 0, }; @@ -6660,7 +6634,7 @@ impl CodexMessageProcessor { accessible_loaded, all_loaded, ) { - apps_list_helpers::send_app_list_updated_notification(&outgoing, merged.clone()) + apps_list_helpers::send_app_list_updated_notification(outgoing, merged.clone()) .await; last_notified_apps = Some(merged); } @@ -6670,25 +6644,13 @@ impl CodexMessageProcessor { let result = match tokio::time::timeout_at(app_list_deadline, rx.recv()).await { Ok(Some(result)) => result, Ok(None) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: "failed to load app lists".to_string(), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; + return Err(internal_error("failed to load app lists")); } Err(_) => { let timeout_seconds = APP_LIST_LOAD_TIMEOUT.as_secs(); - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!( - "timed out waiting for app lists after {timeout_seconds} seconds" - ), - data: None, - }; - outgoing.send_error(request_id, error).await; - return; + return Err(internal_error(format!( + "timed out waiting for app lists after {timeout_seconds} seconds" + ))); } }; @@ -6698,26 +6660,14 @@ impl CodexMessageProcessor { accessible_loaded = true; } AppListLoadResult::Accessible(Err(err)) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: err, - data: None, - }; - outgoing.send_error(request_id, error).await; - return; + return Err(internal_error(err)); } AppListLoadResult::Directory(Ok(connectors)) => { all_connectors = Some(connectors); all_loaded = true; } AppListLoadResult::Directory(Err(err)) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: err, - data: None, - }; - outgoing.send_error(request_id, error).await; - return; + return Err(internal_error(err)); } } @@ -6747,27 +6697,26 @@ impl CodexMessageProcessor { all_loaded, ) && last_notified_apps.as_ref() != Some(&merged) { - apps_list_helpers::send_app_list_updated_notification(&outgoing, merged.clone()) + apps_list_helpers::send_app_list_updated_notification(outgoing, merged.clone()) .await; last_notified_apps = Some(merged.clone()); } if accessible_loaded && all_loaded { - match apps_list_helpers::paginate_apps(merged.as_slice(), start, limit) { - Ok(response) => { - outgoing.send_response(request_id, response).await; - return; - } - Err(error) => { - outgoing.send_error(request_id, error).await; - return; - } - } + return apps_list_helpers::paginate_apps(merged.as_slice(), start, limit); } } } async fn skills_list(&self, request_id: ConnectionRequestId, params: SkillsListParams) { + let result = self.skills_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn skills_list_response( + &self, + params: SkillsListParams, + ) -> Result { let SkillsListParams { cwds, force_reload, @@ -6792,17 +6741,13 @@ impl CodexMessageProcessor { let mut valid_extra_roots = Vec::new(); for root in entry.extra_user_roots { - let Ok(root) = AbsolutePathBuf::from_absolute_path_checked(root.as_path()) else { - self.send_invalid_request_error( - request_id, - format!( + let root = + AbsolutePathBuf::from_absolute_path_checked(root.as_path()).map_err(|_| { + invalid_request(format!( "skills/list perCwdExtraUserRoots extraUserRoots paths must be absolute: {}", root.display() - ), - ) - .await; - return; - }; + )) + })?; valid_extra_roots.push(root); } extra_roots_by_cwd @@ -6811,13 +6756,7 @@ impl CodexMessageProcessor { .extend(valid_extra_roots); } - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(error) => { - self.outgoing.send_error(request_id, error).await; - return; - } - }; + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; let auth = self.auth_manager.auth().await; let workspace_codex_plugins_enabled = self .workspace_codex_plugins_enabled(&config, auth.as_ref()) @@ -6896,9 +6835,7 @@ impl CodexMessageProcessor { errors, }); } - self.outgoing - .send_response(request_id, SkillsListResponse { data }) - .await; + Ok(SkillsListResponse { data }) } async fn marketplace_remove( &self, @@ -6911,27 +6848,16 @@ impl CodexMessageProcessor { marketplace_name: params.marketplace_name, }, ) - .await; - - match result { - Ok(outcome) => { - self.outgoing - .send_response( - request_id, - MarketplaceRemoveResponse { - marketplace_name: outcome.marketplace_name, - installed_root: outcome.removed_installed_root, - }, - ) - .await; - } - Err(MarketplaceRemoveError::InvalidRequest(message)) => { - self.send_invalid_request_error(request_id, message).await; - } - Err(MarketplaceRemoveError::Internal(message)) => { - self.send_internal_error(request_id, message).await; - } - } + .await + .map(|outcome| MarketplaceRemoveResponse { + marketplace_name: outcome.marketplace_name, + installed_root: outcome.removed_installed_root, + }) + .map_err(|err| match err { + MarketplaceRemoveError::InvalidRequest(message) => invalid_request(message), + MarketplaceRemoveError::Internal(message) => internal_error(message), + }); + self.outgoing.send_result(request_id, result).await; } async fn marketplace_upgrade( @@ -6939,53 +6865,38 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: MarketplaceUpgradeParams, ) { - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; + let result = self.marketplace_upgrade_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn marketplace_upgrade_response( + &self, + params: MarketplaceUpgradeParams, + ) -> Result { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; let plugins_manager = self.thread_manager.plugins_manager(); let MarketplaceUpgradeParams { marketplace_name } = params; - let result = tokio::task::spawn_blocking(move || { + let outcome = tokio::task::spawn_blocking(move || { plugins_manager .upgrade_configured_marketplaces_for_config(&config, marketplace_name.as_deref()) }) - .await; + .await + .map_err(|err| internal_error(format!("failed to upgrade marketplaces: {err}")))? + .map_err(invalid_request)?; - match result { - Ok(Ok(outcome)) => { - self.outgoing - .send_response( - request_id, - MarketplaceUpgradeResponse { - selected_marketplaces: outcome.selected_marketplaces, - upgraded_roots: outcome.upgraded_roots, - errors: outcome - .errors - .into_iter() - .map(|err| MarketplaceUpgradeErrorInfo { - marketplace_name: err.marketplace_name, - message: err.message, - }) - .collect(), - }, - ) - .await; - } - Ok(Err(message)) => { - self.send_invalid_request_error(request_id, message).await; - } - Err(err) => { - self.send_internal_error( - request_id, - format!("failed to upgrade marketplaces: {err}"), - ) - .await; - } - } + Ok(MarketplaceUpgradeResponse { + selected_marketplaces: outcome.selected_marketplaces, + upgraded_roots: outcome.upgraded_roots, + errors: outcome + .errors + .into_iter() + .map(|err| MarketplaceUpgradeErrorInfo { + marketplace_name: err.marketplace_name, + message: err.message, + }) + .collect(), + }) } async fn marketplace_add(&self, request_id: ConnectionRequestId, params: MarketplaceAddParams) { @@ -6997,28 +6908,17 @@ impl CodexMessageProcessor { sparse_paths: params.sparse_paths.unwrap_or_default(), }, ) - .await; - - match result { - Ok(outcome) => { - self.outgoing - .send_response( - request_id, - MarketplaceAddResponse { - marketplace_name: outcome.marketplace_name, - installed_root: outcome.installed_root, - already_added: outcome.already_added, - }, - ) - .await; - } - Err(MarketplaceAddError::InvalidRequest(message)) => { - self.send_invalid_request_error(request_id, message).await; - } - Err(MarketplaceAddError::Internal(message)) => { - self.send_internal_error(request_id, message).await; - } - } + .await + .map(|outcome| MarketplaceAddResponse { + marketplace_name: outcome.marketplace_name, + installed_root: outcome.installed_root, + already_added: outcome.already_added, + }) + .map_err(|err| match err { + MarketplaceAddError::InvalidRequest(message) => invalid_request(message), + MarketplaceAddError::Internal(message) => internal_error(message), + }); + self.outgoing.send_result(request_id, result).await; } async fn skills_config_write( @@ -7026,6 +6926,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: SkillsConfigWriteParams, ) { + let result = self.skills_config_write_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn skills_config_write_response( + &self, + params: SkillsConfigWriteParams, + ) -> Result { let SkillsConfigWriteParams { path, name, @@ -7040,43 +6948,24 @@ impl CodexMessageProcessor { ConfigEdit::SetSkillConfigByName { name, enabled } } _ => { - let error = JSONRPCErrorError { - code: INVALID_PARAMS_ERROR_CODE, - message: "skills/config/write requires exactly one of path or name".to_string(), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_params( + "skills/config/write requires exactly one of path or name", + )); } }; let edits = vec![edit]; - let result = ConfigEditsBuilder::new(&self.config.codex_home) + ConfigEditsBuilder::new(&self.config.codex_home) .with_edits(edits) .apply() - .await; - - match result { - Ok(()) => { + .await + .map(|()| { self.thread_manager.plugins_manager().clear_cache(); self.thread_manager.skills_manager().clear_cache(); - self.outgoing - .send_response( - request_id, - SkillsConfigWriteResponse { - effective_enabled: enabled, - }, - ) - .await; - } - Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to update skill settings: {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - } - } + SkillsConfigWriteResponse { + effective_enabled: enabled, + } + }) + .map_err(|err| internal_error(format!("failed to update skill settings: {err}"))) } async fn turn_start( diff --git a/codex-rs/app-server/src/codex_message_processor/plugins.rs b/codex-rs/app-server/src/codex_message_processor/plugins.rs index 8f0f4dea9a..e1d0fffad3 100644 --- a/codex-rs/app-server/src/codex_message_processor/plugins.rs +++ b/codex-rs/app-server/src/codex_message_processor/plugins.rs @@ -1,4 +1,6 @@ use super::*; +use crate::error_code::internal_error; +use crate::error_code::invalid_request; use codex_app_server_protocol::PluginInstallPolicy; impl CodexMessageProcessor { @@ -7,46 +9,33 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: PluginListParams, ) { + let result = self.plugin_list_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn plugin_list_response( + &self, + params: PluginListParams, + ) -> Result { let plugins_manager = self.thread_manager.plugins_manager(); let PluginListParams { cwds } = params; let roots = cwds.unwrap_or_default(); - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + let empty_response = || PluginListResponse { + marketplaces: Vec::new(), + marketplace_load_errors: Vec::new(), + featured_plugin_ids: Vec::new(), }; if !config.features.enabled(Feature::Plugins) { - self.outgoing - .send_response( - request_id, - PluginListResponse { - marketplaces: Vec::new(), - marketplace_load_errors: Vec::new(), - featured_plugin_ids: Vec::new(), - }, - ) - .await; - return; + return Ok(empty_response()); } let auth = self.auth_manager.auth().await; if !self .workspace_codex_plugins_enabled(&config, auth.as_ref()) .await { - self.outgoing - .send_response( - request_id, - PluginListResponse { - marketplaces: Vec::new(), - marketplace_load_errors: Vec::new(), - featured_plugin_ids: Vec::new(), - }, - ) - .await; - return; + return Ok(empty_response()); } plugins_manager.maybe_start_non_curated_plugin_cache_refresh(&roots); @@ -100,18 +89,11 @@ impl CodexMessageProcessor { .await { Ok(Ok(outcome)) => outcome, - Ok(Err(err)) => { - self.send_marketplace_error(request_id, err, "list marketplace plugins") - .await; - return; - } + Ok(Err(err)) => return Err(Self::marketplace_error(err, "list marketplace plugins")), Err(err) => { - self.send_internal_error( - request_id, - format!("failed to list marketplace plugins: {err}"), - ) - .await; - return; + return Err(internal_error(format!( + "failed to list marketplace plugins: {err}" + ))); } }; @@ -174,16 +156,11 @@ impl CodexMessageProcessor { Vec::new() }; - self.outgoing - .send_response( - request_id, - PluginListResponse { - marketplaces: data, - marketplace_load_errors, - featured_plugin_ids, - }, - ) - .await; + Ok(PluginListResponse { + marketplaces: data, + marketplace_load_errors, + featured_plugin_ids, + }) } pub(super) async fn plugin_read( @@ -191,6 +168,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: PluginReadParams, ) { + let result = self.plugin_read_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn plugin_read_response( + &self, + params: PluginReadParams, + ) -> Result { let plugins_manager = self.thread_manager.plugins_manager(); let PluginReadParams { marketplace_path, @@ -201,30 +186,16 @@ impl CodexMessageProcessor { (Some(marketplace_path), None) => Ok(marketplace_path), (None, Some(remote_marketplace_name)) => Err(remote_marketplace_name), (Some(_), Some(_)) | (None, None) => { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "plugin/read requires exactly one of marketplacePath or remoteMarketplaceName".to_string(), - data: None, - }, - ) - .await; - return; + return Err(invalid_request( + "plugin/read requires exactly one of marketplacePath or remoteMarketplaceName", + )); } }; let config_cwd = read_source.as_ref().ok().and_then(|marketplace_path| { marketplace_path.as_path().parent().map(Path::to_path_buf) }); - let config = match self.load_latest_config(config_cwd).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; + let config = self.load_latest_config(config_cwd).await?; let plugin = match read_source { Ok(marketplace_path) => { @@ -232,17 +203,10 @@ impl CodexMessageProcessor { plugin_name, marketplace_path, }; - let outcome = match plugins_manager + let outcome = plugins_manager .read_plugin_for_config(&config, &request) .await - { - Ok(outcome) => outcome, - Err(err) => { - self.send_marketplace_error(request_id, err, "read plugin details") - .await; - return; - } - }; + .map_err(|err| Self::marketplace_error(err, "read plugin details"))?; let environment_manager = self.thread_manager.environment_manager(); let app_summaries = plugin_app_helpers::load_plugin_app_summaries( &config, @@ -287,19 +251,9 @@ impl CodexMessageProcessor { if !config.features.enabled(Feature::Plugins) || !config.features.enabled(Feature::RemotePlugin) { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!( - "remote plugin read is not enabled for marketplace {remote_marketplace_name}" - ), - data: None, - }, - ) - .await; - return; + return Err(invalid_request(format!( + "remote plugin read is not enabled for marketplace {remote_marketplace_name}" + ))); } let auth = self.auth_manager.auth().await; let remote_plugin_service_config = RemotePluginServiceConfig { @@ -310,36 +264,20 @@ impl CodexMessageProcessor { .chars() .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '~') { - self.send_invalid_request_error( - request_id, - "invalid remote plugin id: only ASCII letters, digits, `_`, `-`, and `~` are allowed" - .to_string(), - ) - .await; - return; + return Err(invalid_request( + "invalid remote plugin id: only ASCII letters, digits, `_`, `-`, and `~` are allowed", + )); } - let remote_detail = match codex_core_plugins::remote::fetch_remote_plugin_detail( + let remote_detail = codex_core_plugins::remote::fetch_remote_plugin_detail( &remote_plugin_service_config, auth.as_ref(), &remote_marketplace_name, &plugin_name, ) .await - { - Ok(remote_detail) => remote_detail, - Err(err) => { - self.outgoing - .send_error( - request_id, - remote_plugin_catalog_error_to_jsonrpc( - err, - "read remote plugin details", - ), - ) - .await; - return; - } - }; + .map_err(|err| { + remote_plugin_catalog_error_to_jsonrpc(err, "read remote plugin details") + })?; let plugin_apps = remote_detail .app_ids .iter() @@ -357,9 +295,7 @@ impl CodexMessageProcessor { } }; - self.outgoing - .send_response(request_id, PluginReadResponse { plugin }) - .await; + Ok(PluginReadResponse { plugin }) } pub(super) async fn plugin_install( @@ -367,6 +303,14 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: PluginInstallParams, ) { + let result = self.plugin_install_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn plugin_install_response( + &self, + params: PluginInstallParams, + ) -> Result { let PluginInstallParams { marketplace_path, remote_marketplace_name, @@ -375,44 +319,27 @@ impl CodexMessageProcessor { let marketplace_path = match (marketplace_path, remote_marketplace_name) { (Some(marketplace_path), None) => marketplace_path, (None, Some(remote_marketplace_name)) => { - self.remote_plugin_install(request_id, remote_marketplace_name, plugin_name) + return self + .remote_plugin_install_response(remote_marketplace_name, plugin_name) .await; - return; } (Some(_), Some(_)) | (None, None) => { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "plugin/install requires exactly one of marketplacePath or remoteMarketplaceName".to_string(), - data: None, - }, - ) - .await; - return; + return Err(invalid_request( + "plugin/install requires exactly one of marketplacePath or remoteMarketplaceName", + )); } }; let config_cwd = marketplace_path.as_path().parent().map(Path::to_path_buf); - let config = match self.load_latest_config(config_cwd.clone()).await { - Ok(config) => config, - Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; - } - }; + let config = self.load_latest_config(config_cwd.clone()).await?; let auth = self.auth_manager.auth().await; if !self .workspace_codex_plugins_enabled(&config, auth.as_ref()) .await { - self.send_invalid_request_error( - request_id, - "Codex plugins are disabled for this workspace".to_string(), - ) - .await; - return; + return Err(invalid_request( + "Codex plugins are disabled for this workspace", + )); } let plugins_manager = self.thread_manager.plugins_manager(); @@ -421,197 +348,103 @@ impl CodexMessageProcessor { marketplace_path, }; - let install_result = plugins_manager.install_plugin(request).await; - - match install_result { - Ok(result) => { - let config = match self.load_latest_config(config_cwd).await { - Ok(config) => config, - Err(err) => { - warn!( - "failed to reload config after plugin install, using current config: {err:?}" - ); - config - } - }; - - self.clear_plugin_related_caches(); - - let plugin_mcp_servers = - load_plugin_mcp_servers(result.installed_path.as_path()).await; - - if !plugin_mcp_servers.is_empty() { - if let Err(err) = self.queue_mcp_server_refresh_for_config(&config).await { - warn!( - plugin = result.plugin_id.as_key(), - "failed to queue MCP refresh after plugin install: {err:?}" - ); - } - self.start_plugin_mcp_oauth_logins(&config, plugin_mcp_servers) - .await; - } - - let plugin_apps = load_plugin_apps(result.installed_path.as_path()).await; - let auth = self.auth_manager.auth().await; - let apps_needing_auth = self - .plugin_apps_needing_auth_for_install( - &config, - auth.as_ref().is_some_and(CodexAuth::is_chatgpt_auth), - &result.plugin_id.as_key(), - &plugin_apps, - ) - .await; - - self.outgoing - .send_response( - request_id, - PluginInstallResponse { - auth_policy: result.auth_policy.into(), - apps_needing_auth, - }, - ) - .await; - } - Err(err) => { - if err.is_invalid_request() { - self.send_invalid_request_error(request_id, err.to_string()) - .await; - return; - } - - match err { - CorePluginInstallError::Marketplace(err) => { - self.send_marketplace_error(request_id, err, "install plugin") - .await; - } - CorePluginInstallError::Config(err) => { - self.send_internal_error( - request_id, - format!("failed to persist installed plugin config: {err}"), - ) - .await; - } - CorePluginInstallError::Remote(err) => { - self.send_internal_error( - request_id, - format!("failed to enable remote plugin: {err}"), - ) - .await; - } - CorePluginInstallError::Join(err) => { - self.send_internal_error( - request_id, - format!("failed to install plugin: {err}"), - ) - .await; - } - CorePluginInstallError::Store(err) => { - self.send_internal_error( - request_id, - format!("failed to install plugin: {err}"), - ) - .await; - } - } - } - } - } - - async fn remote_plugin_install( - &self, - request_id: ConnectionRequestId, - remote_marketplace_name: String, - plugin_name: String, - ) { - let config = match self.load_latest_config(/*fallback_cwd*/ None).await { + let result = plugins_manager + .install_plugin(request) + .await + .map_err(Self::plugin_install_error)?; + let config = match self.load_latest_config(config_cwd).await { Ok(config) => config, Err(err) => { - self.outgoing.send_error(request_id, err).await; - return; + warn!( + "failed to reload config after plugin install, using current config: {err:?}" + ); + config } }; + + self.clear_plugin_related_caches(); + + let plugin_mcp_servers = load_plugin_mcp_servers(result.installed_path.as_path()).await; + + if !plugin_mcp_servers.is_empty() { + if let Err(err) = self.queue_mcp_server_refresh_for_config(&config).await { + warn!( + plugin = result.plugin_id.as_key(), + "failed to queue MCP refresh after plugin install: {err:?}" + ); + } + self.start_plugin_mcp_oauth_logins(&config, plugin_mcp_servers) + .await; + } + + let plugin_apps = load_plugin_apps(result.installed_path.as_path()).await; + let auth = self.auth_manager.auth().await; + let apps_needing_auth = self + .plugin_apps_needing_auth_for_install( + &config, + auth.as_ref().is_some_and(CodexAuth::is_chatgpt_auth), + &result.plugin_id.as_key(), + &plugin_apps, + ) + .await; + + Ok(PluginInstallResponse { + auth_policy: result.auth_policy.into(), + apps_needing_auth, + }) + } + + async fn remote_plugin_install_response( + &self, + remote_marketplace_name: String, + plugin_name: String, + ) -> Result { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; if !config.features.enabled(Feature::Plugins) || !config.features.enabled(Feature::RemotePlugin) { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!( - "remote plugin install is not enabled for marketplace {remote_marketplace_name}" - ), - data: None, - }, - ) - .await; - return; + return Err(invalid_request(format!( + "remote plugin install is not enabled for marketplace {remote_marketplace_name}" + ))); } if plugin_name.is_empty() || !plugin_name .chars() .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '~') { - self.send_invalid_request_error( - request_id, - "invalid remote plugin id: only ASCII letters, digits, `_`, `-`, and `~` are allowed" - .to_string(), - ) - .await; - return; + return Err(invalid_request( + "invalid remote plugin id: only ASCII letters, digits, `_`, `-`, and `~` are allowed", + )); } let auth = self.auth_manager.auth().await; let remote_plugin_service_config = RemotePluginServiceConfig { chatgpt_base_url: config.chatgpt_base_url.clone(), }; - let remote_detail = match codex_core_plugins::remote::fetch_remote_plugin_detail( + let remote_detail = codex_core_plugins::remote::fetch_remote_plugin_detail( &remote_plugin_service_config, auth.as_ref(), &remote_marketplace_name, &plugin_name, ) .await - { - Ok(remote_detail) => remote_detail, - Err(err) => { - self.outgoing - .send_error( - request_id, - remote_plugin_catalog_error_to_jsonrpc( - err, - "read remote plugin details before install", - ), - ) - .await; - return; - } - }; + .map_err(|err| { + remote_plugin_catalog_error_to_jsonrpc(err, "read remote plugin details before install") + })?; if remote_detail.summary.install_policy == PluginInstallPolicy::NotAvailable { - self.send_invalid_request_error( - request_id, - format!("remote plugin {plugin_name} is not available for install"), - ) - .await; - return; + return Err(invalid_request(format!( + "remote plugin {plugin_name} is not available for install" + ))); } - if let Err(err) = codex_core_plugins::remote::install_remote_plugin( + codex_core_plugins::remote::install_remote_plugin( &remote_plugin_service_config, auth.as_ref(), &remote_marketplace_name, &plugin_name, ) .await - { - self.outgoing - .send_error( - request_id, - remote_plugin_catalog_error_to_jsonrpc(err, "install remote plugin"), - ) - .await; - return; - } + .map_err(|err| remote_plugin_catalog_error_to_jsonrpc(err, "install remote plugin"))?; self.clear_plugin_related_caches(); @@ -629,15 +462,10 @@ impl CodexMessageProcessor { ) .await; - self.outgoing - .send_response( - request_id, - PluginInstallResponse { - auth_policy: remote_detail.summary.auth_policy, - apps_needing_auth, - }, - ) - .await; + Ok(PluginInstallResponse { + auth_policy: remote_detail.summary.auth_policy, + apps_needing_auth, + }) } async fn plugin_apps_needing_auth_for_install( @@ -709,59 +537,82 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: PluginUninstallParams, ) { + let result = self.plugin_uninstall_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn plugin_uninstall_response( + &self, + params: PluginUninstallParams, + ) -> Result { let PluginUninstallParams { plugin_id } = params; let plugins_manager = self.thread_manager.plugins_manager(); - let uninstall_result = plugins_manager.uninstall_plugin(plugin_id).await; + plugins_manager + .uninstall_plugin(plugin_id) + .await + .map_err(Self::plugin_uninstall_error)?; + self.clear_plugin_related_caches(); + Ok(PluginUninstallResponse {}) + } - match uninstall_result { - Ok(()) => { - self.clear_plugin_related_caches(); - self.outgoing - .send_response(request_id, PluginUninstallResponse {}) - .await; - } - Err(err) => { - if err.is_invalid_request() { - self.send_invalid_request_error(request_id, err.to_string()) - .await; - return; - } + fn plugin_install_error(err: CorePluginInstallError) -> JSONRPCErrorError { + if err.is_invalid_request() { + return invalid_request(err.to_string()); + } - match err { - CorePluginUninstallError::Config(err) => { - self.send_internal_error( - request_id, - format!("failed to clear plugin config: {err}"), - ) - .await; - } - CorePluginUninstallError::Remote(err) => { - self.send_internal_error( - request_id, - format!("failed to uninstall remote plugin: {err}"), - ) - .await; - } - CorePluginUninstallError::Join(err) => { - self.send_internal_error( - request_id, - format!("failed to uninstall plugin: {err}"), - ) - .await; - } - CorePluginUninstallError::Store(err) => { - self.send_internal_error( - request_id, - format!("failed to uninstall plugin: {err}"), - ) - .await; - } - CorePluginUninstallError::InvalidPluginId(_) => { - unreachable!("invalid plugin ids are handled above"); - } - } + match err { + CorePluginInstallError::Marketplace(err) => { + Self::marketplace_error(err, "install plugin") } + CorePluginInstallError::Config(err) => { + internal_error(format!("failed to persist installed plugin config: {err}")) + } + CorePluginInstallError::Remote(err) => { + internal_error(format!("failed to enable remote plugin: {err}")) + } + CorePluginInstallError::Join(err) => { + internal_error(format!("failed to install plugin: {err}")) + } + CorePluginInstallError::Store(err) => { + internal_error(format!("failed to install plugin: {err}")) + } + } + } + + fn plugin_uninstall_error(err: CorePluginUninstallError) -> JSONRPCErrorError { + if err.is_invalid_request() { + return invalid_request(err.to_string()); + } + + match err { + CorePluginUninstallError::Config(err) => { + internal_error(format!("failed to clear plugin config: {err}")) + } + CorePluginUninstallError::Remote(err) => { + internal_error(format!("failed to uninstall remote plugin: {err}")) + } + CorePluginUninstallError::Join(err) => { + internal_error(format!("failed to uninstall plugin: {err}")) + } + CorePluginUninstallError::Store(err) => { + internal_error(format!("failed to uninstall plugin: {err}")) + } + CorePluginUninstallError::InvalidPluginId(_) => { + unreachable!("invalid plugin ids are handled above"); + } + } + } + + fn marketplace_error(err: MarketplaceError, action: &str) -> JSONRPCErrorError { + match err { + MarketplaceError::MarketplaceNotFound { .. } + | MarketplaceError::InvalidMarketplaceFile { .. } + | MarketplaceError::PluginNotFound { .. } + | MarketplaceError::PluginNotAvailable { .. } + | MarketplaceError::PluginsDisabled + | MarketplaceError::InvalidPlugin(_) => invalid_request(err.to_string()), + MarketplaceError::Io { .. } => internal_error(format!("failed to {action}: {err}")), } } } From 2009f6e894d3141020a4651a2f14d4bf5f899e30 Mon Sep 17 00:00:00 2001 From: efrazer-oai Date: Mon, 27 Apr 2026 11:00:27 -0700 Subject: [PATCH 05/11] refactor: make auth loading async (#19762) ## Summary Auth loading used to expose synchronous construction helpers in several places even though some auth sources now need async work. This PR makes the auth-loading surface async and updates the callers to await it. This is intentionally only plumbing. It does not change how AgentIdentity tokens are decoded, how task runtime ids are allocated, or how JWT signatures are verified. ## Stack 1. **This PR:** [refactor: make auth loading async](https://github.com/openai/codex/pull/19762) 2. [refactor: load AgentIdentity runtime eagerly](https://github.com/openai/codex/pull/19763) 3. [feat: verify AgentIdentity JWTs with JWKS](https://github.com/openai/codex/pull/19764) ## Important call sites | Area | Change | | --- | --- | | `codex-login` auth loading | `CodexAuth` and `AuthManager` construction paths now await auth loading. | | app-server startup | Auth manager construction is awaited during initialization. | | CLI/TUI/exec/MCP/chatgpt callers | Existing auth-loading calls now await the same behavior. | | cloud requirements storage loader | The loader becomes async so it can share the same auth construction path. | | auth tests | Tests that load auth now run in async contexts. | ## Testing Tests: targeted Rust auth test compilation, formatter, scoped Clippy fix, and Bazel lock check. --- .../app-server/src/codex_message_processor.rs | 8 +- codex-rs/app-server/src/in_process.rs | 3 +- codex-rs/app-server/src/lib.rs | 6 +- .../src/message_processor/tracing_tests.rs | 6 +- .../src/transport/remote_control/tests.rs | 6 +- .../src/transport/remote_control/websocket.rs | 10 +- codex-rs/chatgpt/src/chatgpt_client.rs | 2 +- codex-rs/chatgpt/src/connectors.rs | 4 +- codex-rs/cli/src/login.rs | 4 +- codex-rs/cli/src/main.rs | 2 +- codex-rs/cloud-requirements/src/lib.rs | 159 ++++++++++-------- codex-rs/cloud-tasks/src/util.rs | 15 +- codex-rs/core/src/connectors.rs | 6 +- codex-rs/core/src/prompt_debug.rs | 2 +- codex-rs/core/tests/suite/client.rs | 3 +- codex-rs/exec/src/lib.rs | 7 +- codex-rs/login/src/auth/auth_tests.rs | 70 +++++--- codex-rs/login/src/auth/manager.rs | 83 +++++---- codex-rs/login/tests/suite/auth_refresh.rs | 69 ++++---- codex-rs/login/tests/suite/logout.rs | 3 +- codex-rs/mcp-server/src/lib.rs | 3 +- codex-rs/mcp-server/src/message_processor.rs | 5 +- codex-rs/models-manager/src/manager_tests.rs | 5 +- codex-rs/tui/src/lib.rs | 10 +- codex-rs/tui/src/onboarding/auth.rs | 3 +- 25 files changed, 291 insertions(+), 203 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 563a87c3bd..814fee9d8c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1354,7 +1354,7 @@ impl CodexMessageProcessor { self.config.cli_auth_credentials_store_mode, ) { Ok(()) => { - self.auth_manager.reload(); + self.auth_manager.reload().await; Ok(()) } Err(err) => Err(JSONRPCErrorError { @@ -1505,7 +1505,7 @@ impl CodexMessageProcessor { .await; if success { - auth_manager.reload(); + auth_manager.reload().await; config_manager.replace_cloud_requirements_loader( auth_manager.clone(), chatgpt_base_url, @@ -1613,7 +1613,7 @@ impl CodexMessageProcessor { .await; if success { - auth_manager.reload(); + auth_manager.reload().await; config_manager.replace_cloud_requirements_loader( auth_manager.clone(), chatgpt_base_url, @@ -1749,7 +1749,7 @@ impl CodexMessageProcessor { self.outgoing.send_error(request_id, error).await; return; } - self.auth_manager.reload(); + self.auth_manager.reload().await; self.config_manager.replace_cloud_requirements_loader( self.auth_manager.clone(), self.config.chatgpt_base_url.clone(), diff --git a/codex-rs/app-server/src/in_process.rs b/codex-rs/app-server/src/in_process.rs index cc4e22e923..73332394f4 100644 --- a/codex-rs/app-server/src/in_process.rs +++ b/codex-rs/app-server/src/in_process.rs @@ -391,7 +391,8 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle { let processor_outgoing = Arc::clone(&outgoing_message_sender); let auth_manager = - AuthManager::shared_from_config(args.config.as_ref(), args.enable_codex_api_key_env); + AuthManager::shared_from_config(args.config.as_ref(), args.enable_codex_api_key_env) + .await; let config_manager = ConfigManager::new( args.config.codex_home.to_path_buf(), args.cli_overrides, diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 52b6de0e0c..de89608101 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -469,7 +469,7 @@ pub async fn run_main_with_transport_options( config_manager .replace_thread_config_loader(Arc::clone(&discovered_thread_config_loader)); let auth_manager = - AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; config_manager.replace_cloud_requirements_loader(auth_manager, config.chatgpt_base_url); } Err(err) => { @@ -631,7 +631,7 @@ pub async fn run_main_with_transport_options( } let auth_manager = - AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; let remote_control_enabled = config.features.enabled(Feature::RemoteControl); if transport_accept_handles.is_empty() && !remote_control_enabled { @@ -712,7 +712,7 @@ pub async fn run_main_with_transport_options( let outgoing_message_sender = Arc::new(OutgoingMessageSender::new(outgoing_tx)); let outbound_control_tx = outbound_control_tx; let auth_manager = - AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; let processor = Arc::new(MessageProcessor::new(MessageProcessorArgs { outgoing: outgoing_message_sender, arg0_paths, diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index 507d7865b9..850d05c04e 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -127,7 +127,7 @@ impl TracingHarness { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; let config = Arc::new(build_test_config(codex_home.path(), &server.uri()).await?); - let (processor, outgoing_rx) = build_test_processor(config); + let (processor, outgoing_rx) = build_test_processor(config).await; let tracing = init_test_tracing(); tracing.exporter.reset(); tracing::callsite::rebuild_interest_cache(); @@ -257,7 +257,7 @@ async fn build_test_config(codex_home: &Path, server_uri: &str) -> Result, ) -> ( Arc, @@ -266,7 +266,7 @@ fn build_test_processor( let (outgoing_tx, outgoing_rx) = mpsc::channel(16); let outgoing = Arc::new(OutgoingMessageSender::new(outgoing_tx)); let auth_manager = - AuthManager::shared_from_config(config.as_ref(), /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config.as_ref(), /*enable_codex_api_key_env*/ false).await; let config_manager = ConfigManager::new( config.codex_home.to_path_buf(), Vec::new(), diff --git a/codex-rs/app-server/src/transport/remote_control/tests.rs b/codex-rs/app-server/src/transport/remote_control/tests.rs index 6b0051f8db..82e02ae5de 100644 --- a/codex-rs/app-server/src/transport/remote_control/tests.rs +++ b/codex-rs/app-server/src/transport/remote_control/tests.rs @@ -497,7 +497,8 @@ async fn remote_control_start_allows_missing_auth_when_enabled() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; let (transport_event_tx, _transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); @@ -1085,7 +1086,8 @@ async fn remote_control_waits_for_account_id_before_enrolling() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; let expected_server_name = gethostname().to_string_lossy().trim().to_string(); let expected_enrollment = RemoteControlEnrollment { account_id: "account_id".to_string(), diff --git a/codex-rs/app-server/src/transport/remote_control/websocket.rs b/codex-rs/app-server/src/transport/remote_control/websocket.rs index 464832e34a..4cd078455a 100644 --- a/codex-rs/app-server/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server/src/transport/remote_control/websocket.rs @@ -706,7 +706,7 @@ pub(crate) async fn load_remote_control_auth( "remote control requires ChatGPT authentication", )); } - auth_manager.reload(); + auth_manager.reload().await; reloaded = true; continue; }; @@ -714,7 +714,7 @@ pub(crate) async fn load_remote_control_auth( break auth; } if auth.get_account_id().is_none() && !reloaded { - auth_manager.reload(); + auth_manager.reload().await; reloaded = true; continue; } @@ -1090,7 +1090,8 @@ mod tests { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut enrollment = Some(RemoteControlEnrollment { account_id: "account_id".to_string(), @@ -1172,7 +1173,8 @@ mod tests { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; let mut auth_recovery = auth_manager.unauthorized_recovery(); let mut enrollment = None; save_auth( diff --git a/codex-rs/chatgpt/src/chatgpt_client.rs b/codex-rs/chatgpt/src/chatgpt_client.rs index 42aac41138..05d8186686 100644 --- a/codex-rs/chatgpt/src/chatgpt_client.rs +++ b/codex-rs/chatgpt/src/chatgpt_client.rs @@ -21,7 +21,7 @@ pub(crate) async fn chatgpt_get_request_with_timeout( ) -> anyhow::Result { let chatgpt_base_url = &config.chatgpt_base_url; let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; let auth = auth_manager .auth() .await diff --git a/codex-rs/chatgpt/src/connectors.rs b/codex-rs/chatgpt/src/connectors.rs index 62e8040940..9dba71ce3a 100644 --- a/codex-rs/chatgpt/src/connectors.rs +++ b/codex-rs/chatgpt/src/connectors.rs @@ -26,7 +26,7 @@ const DIRECTORY_CONNECTORS_TIMEOUT: Duration = Duration::from_secs(60); async fn apps_enabled(config: &Config) -> bool { let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; let auth = auth_manager.auth().await; config .features @@ -35,7 +35,7 @@ async fn apps_enabled(config: &Config) -> bool { async fn connector_auth(config: &Config) -> anyhow::Result { let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; let auth = auth_manager .auth() .await diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index 4fa7272ae4..5c60c20cbf 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -362,7 +362,9 @@ pub async fn run_login_with_device_code_fallback_to_browser( pub async fn run_login_status(cli_config_overrides: CliConfigOverrides) -> ! { let config = load_config_or_exit(cli_config_overrides).await; - match CodexAuth::from_auth_storage(&config.codex_home, config.cli_auth_credentials_store_mode) { + match CodexAuth::from_auth_storage(&config.codex_home, config.cli_auth_credentials_store_mode) + .await + { Ok(Some(auth)) => match auth.auth_mode() { AuthMode::ApiKey => match auth.get_token() { Ok(api_key) => { diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 9f465521da..852dff616d 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -1384,7 +1384,7 @@ async fn run_debug_models_command( .map_err(anyhow::Error::msg)?; let config = Config::load_with_cli_overrides(cli_overrides).await?; let auth_manager = - AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ true); + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ true).await; let models_manager = build_models_manager(&config, auth_manager, CollaborationModesConfig::default()); models_manager diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index 86a12e7d17..d38fbb0846 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -727,7 +727,7 @@ pub fn cloud_requirements_loader( }) } -pub fn cloud_requirements_loader_for_storage( +pub async fn cloud_requirements_loader_for_storage( codex_home: PathBuf, enable_codex_api_key_env: bool, credentials_store_mode: AuthCredentialsStoreMode, @@ -738,7 +738,8 @@ pub fn cloud_requirements_loader_for_storage( enable_codex_api_key_env, credentials_store_mode, Some(chatgpt_base_url.clone()), - ); + ) + .await; cloud_requirements_loader(auth_manager, chatgpt_base_url, codex_home) } @@ -853,7 +854,7 @@ mod tests { Ok(()) } - fn auth_manager_with_api_key() -> Arc { + async fn auth_manager_with_api_key() -> Arc { let tmp = tempdir().expect("tempdir"); let auth_json = json!({ "OPENAI_API_KEY": "sk-test-key", @@ -861,15 +862,18 @@ mod tests { "last_refresh": null, }); write_auth_json(tmp.path(), auth_json).expect("write auth"); - Arc::new(AuthManager::new( - tmp.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )) + Arc::new( + AuthManager::new( + tmp.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ) } - fn auth_manager_with_plan_and_identity( + async fn auth_manager_with_plan_and_identity( plan_type: &str, chatgpt_user_id: Option<&str>, account_id: Option<&str>, @@ -886,12 +890,15 @@ mod tests { ), ) .expect("write auth"); - Arc::new(AuthManager::new( - tmp.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )) + Arc::new( + AuthManager::new( + tmp.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ) } fn chatgpt_auth_json( @@ -975,7 +982,7 @@ mod tests { manager: Arc, } - fn managed_auth_context( + async fn managed_auth_context( plan_type: &str, chatgpt_user_id: Option<&str>, account_id: Option<&str>, @@ -995,18 +1002,22 @@ mod tests { ) .expect("write auth"); ManagedAuthContext { - manager: Arc::new(AuthManager::new( - home.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )), + manager: Arc::new( + AuthManager::new( + home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ), _home: home, } } - fn auth_manager_with_plan(plan_type: &str) -> Arc { + async fn auth_manager_with_plan(plan_type: &str) -> Arc { auth_manager_with_plan_and_identity(plan_type, Some("user-12345"), Some("account-12345")) + .await } fn parse_for_fetch(contents: Option<&str>) -> Option { @@ -1118,7 +1129,7 @@ mod tests { #[tokio::test] async fn fetch_cloud_requirements_skips_non_chatgpt_auth() { - let auth_manager = auth_manager_with_api_key(); + let auth_manager = auth_manager_with_api_key().await; let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( auth_manager, @@ -1134,7 +1145,7 @@ mod tests { async fn fetch_cloud_requirements_skips_non_business_or_enterprise_plan() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("pro"), + auth_manager_with_plan("pro").await, Arc::new(StaticFetcher { contents: None }), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1147,7 +1158,7 @@ mod tests { async fn fetch_cloud_requirements_skips_team_like_usage_based_plan() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("self_serve_business_usage_based"), + auth_manager_with_plan("self_serve_business_usage_based").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1161,7 +1172,7 @@ mod tests { async fn fetch_cloud_requirements_allows_business_plan() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1193,7 +1204,7 @@ mod tests { async fn fetch_cloud_requirements_allows_business_like_usage_based_plan() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("enterprise_cbp_usage_based"), + auth_manager_with_plan("enterprise_cbp_usage_based").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1225,7 +1236,7 @@ mod tests { async fn fetch_cloud_requirements_allows_hc_plan_as_enterprise() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("hc"), + auth_manager_with_plan("hc").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1329,7 +1340,7 @@ enabled = false #[tokio::test(start_paused = true)] async fn fetch_cloud_requirements_times_out() { - let auth_manager = auth_manager_with_plan("enterprise"); + let auth_manager = auth_manager_with_plan("enterprise").await; let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( auth_manager, @@ -1356,7 +1367,7 @@ enabled = false ])); let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1405,12 +1416,15 @@ enabled = false ), ) .expect("write initial auth"); - let auth_manager = Arc::new(AuthManager::new( - auth_home.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )); + let auth_manager = Arc::new( + AuthManager::new( + auth_home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ); write_auth_json( auth_home.path(), @@ -1479,12 +1493,15 @@ enabled = false ), ) .expect("write initial auth"); - let auth_manager = Arc::new(AuthManager::new( - auth_home.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )); + let auth_manager = Arc::new( + AuthManager::new( + auth_home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ); write_auth_json( auth_home.path(), @@ -1559,7 +1576,8 @@ enabled = false Some("account-12345"), "stale-access-token", "test-refresh-token", - ); + ) + .await; write_auth_json( auth._home.path(), chatgpt_auth_json( @@ -1611,12 +1629,15 @@ enabled = false ), ) .expect("write auth"); - let auth_manager = Arc::new(AuthManager::new( - auth_home.path().to_path_buf(), - /*enable_codex_api_key_env*/ false, - AuthCredentialsStoreMode::File, - /*chatgpt_base_url*/ None, - )); + let auth_manager = Arc::new( + AuthManager::new( + auth_home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + /*chatgpt_base_url*/ None, + ) + .await, + ); let fetcher = Arc::new(UnauthorizedFetcher { message: @@ -1653,7 +1674,7 @@ enabled = false ])); let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1678,7 +1699,7 @@ enabled = false ))])); let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, fetcher, codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1700,7 +1721,7 @@ enabled = false async fn fetch_cloud_requirements_uses_cache_when_valid() { let codex_home = tempdir().expect("tempdir"); let prime_service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1711,7 +1732,7 @@ enabled = false let fetcher = Arc::new(SequenceFetcher::new(vec![Err(request_error())])); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1747,7 +1768,8 @@ enabled = false "business", /*chatgpt_user_id*/ None, Some("account-12345"), - ), + ) + .await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1790,7 +1812,7 @@ enabled = false async fn fetch_cloud_requirements_does_not_use_cache_when_auth_identity_is_incomplete() { let codex_home = tempdir().expect("tempdir"); let prime_service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1807,7 +1829,8 @@ enabled = false "business", /*chatgpt_user_id*/ None, Some("account-12345"), - ), + ) + .await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1843,7 +1866,8 @@ enabled = false "business", Some("user-12345"), Some("account-12345"), - ), + ) + .await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1860,7 +1884,8 @@ enabled = false "business", Some("user-99999"), Some("account-12345"), - ), + ) + .await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1892,7 +1917,7 @@ enabled = false async fn fetch_cloud_requirements_ignores_tampered_cache() { let codex_home = tempdir().expect("tempdir"); let prime_service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -1917,7 +1942,7 @@ enabled = false "allowed_approval_policies = [\"never\"]".to_string(), ))])); let service = CloudRequirementsService::new( - auth_manager_with_plan("enterprise"), + auth_manager_with_plan("enterprise").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -1975,7 +2000,7 @@ enabled = false "allowed_approval_policies = [\"never\"]".to_string(), ))])); let service = CloudRequirementsService::new( - auth_manager_with_plan("enterprise"), + auth_manager_with_plan("enterprise").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -2007,7 +2032,7 @@ enabled = false async fn fetch_cloud_requirements_writes_signed_cache() { let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, Arc::new(StaticFetcher { contents: Some("allowed_approval_policies = [\"never\"]".to_string()), }), @@ -2070,7 +2095,7 @@ enabled = false let fetcher = Arc::new(SequenceFetcher::new(vec![Ok(None), Err(request_error())])); let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("enterprise"), + auth_manager_with_plan("enterprise").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -2088,7 +2113,7 @@ enabled = false ])); let codex_home = tempdir().expect("tempdir"); let service = CloudRequirementsService::new( - auth_manager_with_plan("enterprise"), + auth_manager_with_plan("enterprise").await, fetcher.clone(), codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, @@ -2121,7 +2146,7 @@ enabled = false )), ])); let service = CloudRequirementsService::new( - auth_manager_with_plan("business"), + auth_manager_with_plan("business").await, fetcher, codex_home.path().to_path_buf(), CLOUD_REQUIREMENTS_TIMEOUT, diff --git a/codex-rs/cloud-tasks/src/util.rs b/codex-rs/cloud-tasks/src/util.rs index e433b892e5..9a5056aa66 100644 --- a/codex-rs/cloud-tasks/src/util.rs +++ b/codex-rs/cloud-tasks/src/util.rs @@ -44,12 +44,15 @@ pub fn normalize_base_url(input: &str) -> String { pub async fn load_auth_manager(chatgpt_base_url: Option) -> Option { // TODO: pass in cli overrides once cloud tasks properly support them. let config = Config::load_with_cli_overrides(Vec::new()).await.ok()?; - Some(AuthManager::new( - config.codex_home.to_path_buf(), - /*enable_codex_api_key_env*/ false, - config.cli_auth_credentials_store_mode, - chatgpt_base_url.or(Some(config.chatgpt_base_url)), - )) + Some( + AuthManager::new( + config.codex_home.to_path_buf(), + /*enable_codex_api_key_env*/ false, + config.cli_auth_credentials_store_mode, + chatgpt_base_url.or(Some(config.chatgpt_base_url)), + ) + .await, + ) } /// Build headers for ChatGPT-backed requests: `User-Agent`, optional `Authorization`, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 456f3a7eac..42cd415217 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -144,7 +144,7 @@ pub async fn list_cached_accessible_connectors_from_mcp_tools( config: &Config, ) -> Option> { let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; let auth = auth_manager.auth().await; if !config .features @@ -216,7 +216,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( environment_manager: &EnvironmentManager, ) -> anyhow::Result { let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; let auth = auth_manager.auth().await; if !config .features @@ -434,7 +434,7 @@ async fn list_directory_connectors_for_tool_suggest_with_auth( Some(auth) } else { let auth_manager = - AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; loaded_auth = auth_manager.auth().await; loaded_auth.as_ref() }; diff --git a/codex-rs/core/src/prompt_debug.rs b/codex-rs/core/src/prompt_debug.rs index b6c66410a1..f782c0d232 100644 --- a/codex-rs/core/src/prompt_debug.rs +++ b/codex-rs/core/src/prompt_debug.rs @@ -29,7 +29,7 @@ pub async fn build_prompt_input( config.ephemeral = true; let auth_manager = - AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false); + AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false).await; let local_runtime_paths = ExecServerRuntimePaths::from_optional_paths( config.codex_self_exe.clone(), diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 8886568435..7bd5cee686 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -1091,7 +1091,8 @@ async fn prefers_apikey_when_config_prefers_apikey_even_with_chatgpt_tokens() { config.model_provider = model_provider; let auth_manager = - match CodexAuth::from_auth_storage(codex_home.path(), AuthCredentialsStoreMode::File) { + match CodexAuth::from_auth_storage(codex_home.path(), AuthCredentialsStoreMode::File).await + { Ok(Some(auth)) => codex_core::test_support::auth_manager_from_auth(auth), Ok(None) => panic!("No CodexAuth found in codex_home"), Err(e) => panic!("Failed to load CodexAuth: {e}"), diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 334e4001d5..9e9d9bc148 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -348,7 +348,8 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result /*enable_codex_api_key_env*/ false, config_toml.cli_auth_credentials_store.unwrap_or_default(), chatgpt_base_url, - ); + ) + .await; let run_cli_overrides = cli_kv_overrides.clone(); let run_loader_overrides = loader_overrides.clone(); let run_cloud_requirements = cloud_requirements.clone(); @@ -438,7 +439,9 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result auth_credentials_store_mode: config.cli_auth_credentials_store_mode, forced_login_method: config.forced_login_method, forced_chatgpt_workspace_id: config.forced_chatgpt_workspace_id.clone(), - }) { + }) + .await + { eprintln!("{err}"); std::process::exit(1); } diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index b38a40b6f0..b6c86761ef 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -116,10 +116,11 @@ fn login_with_agent_identity_rejects_invalid_jwt() { ); } -#[test] -fn missing_auth_json_returns_none() { +#[tokio::test] +async fn missing_auth_json_returns_none() { let dir = tempdir().unwrap(); let auth = CodexAuth::from_auth_storage(dir.path(), AuthCredentialsStoreMode::File) + .await .expect("call should succeed"); assert_eq!(auth, None); } @@ -143,6 +144,7 @@ async fn pro_account_with_no_api_key_uses_chatgpt_auth() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .unwrap() .unwrap(); assert_eq!(None, auth.api_key()); @@ -196,6 +198,7 @@ async fn loads_api_key_from_auth_json() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .unwrap() .unwrap(); assert_eq!(auth.auth_mode(), AuthMode::ApiKey); @@ -222,15 +225,16 @@ fn logout_removes_auth_file() -> Result<(), std::io::Error> { Ok(()) } -#[test] -fn unauthorized_recovery_reports_mode_and_step_names() { +#[tokio::test] +async fn unauthorized_recovery_reports_mode_and_step_names() { let dir = tempdir().unwrap(); let manager = AuthManager::shared( dir.path().to_path_buf(), /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; let managed = UnauthorizedRecovery { manager: Arc::clone(&manager), step: UnauthorizedRecoveryStep::Reload, @@ -250,8 +254,8 @@ fn unauthorized_recovery_reports_mode_and_step_names() { assert_eq!(external.step_name(), "external_refresh"); } -#[test] -fn refresh_failure_is_scoped_to_the_matching_auth_snapshot() { +#[tokio::test] +async fn refresh_failure_is_scoped_to_the_matching_auth_snapshot() { let codex_home = tempdir().unwrap(); write_auth_file( AuthFileParams { @@ -268,6 +272,7 @@ fn refresh_failure_is_scoped_to_the_matching_auth_snapshot() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); let mut updated_auth_dot_json = auth @@ -284,6 +289,7 @@ fn refresh_failure_is_scoped_to_the_matching_auth_snapshot() { updated_auth_dot_json, AuthCredentialsStoreMode::File, ) + .await .expect("updated auth should parse"); let manager = AuthManager::from_auth_for_testing(auth.clone()); @@ -619,9 +625,9 @@ impl Drop for EnvVarGuard { } } -#[test] +#[tokio::test] #[serial(codex_auth_env)] -fn load_auth_reads_agent_identity_from_env() { +async fn load_auth_reads_agent_identity_from_env() { let codex_home = tempdir().unwrap(); let expected_record = agent_identity_record("account-123"); let agent_identity = fake_agent_identity_jwt(&expected_record).expect("fake agent identity"); @@ -632,6 +638,7 @@ fn load_auth_reads_agent_identity_from_env() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("env auth should load") .expect("env auth should be present"); @@ -645,9 +652,9 @@ fn load_auth_reads_agent_identity_from_env() { ); } -#[test] +#[tokio::test] #[serial(codex_auth_env)] -fn load_auth_keeps_codex_api_key_env_precedence() { +async fn load_auth_keeps_codex_api_key_env_precedence() { let codex_home = tempdir().unwrap(); let record = agent_identity_record("account-123"); let agent_identity = fake_agent_identity_jwt(&record).expect("fake agent identity"); @@ -659,6 +666,7 @@ fn load_auth_keeps_codex_api_key_env_precedence() { /*enable_codex_api_key_env*/ true, AuthCredentialsStoreMode::File, ) + .await .expect("env auth should load") .expect("env auth should be present"); @@ -679,8 +687,9 @@ async fn enforce_login_restrictions_logs_out_for_method_mismatch() { ) .await; - let err = - super::enforce_login_restrictions(&config).expect_err("expected method mismatch to error"); + let err = super::enforce_login_restrictions(&config) + .await + .expect_err("expected method mismatch to error"); assert!(err.to_string().contains("ChatGPT login is required")); assert!( !codex_home.path().join("auth.json").exists(), @@ -710,6 +719,7 @@ async fn enforce_login_restrictions_logs_out_for_workspace_mismatch() { .await; let err = super::enforce_login_restrictions(&config) + .await .expect_err("expected workspace mismatch to error"); assert!(err.to_string().contains("workspace org_mine")); assert!( @@ -739,7 +749,9 @@ async fn enforce_login_restrictions_allows_matching_workspace() { ) .await; - super::enforce_login_restrictions(&config).expect("matching workspace should succeed"); + super::enforce_login_restrictions(&config) + .await + .expect("matching workspace should succeed"); assert!( codex_home.path().join("auth.json").exists(), "auth.json should remain when restrictions pass" @@ -761,7 +773,9 @@ async fn enforce_login_restrictions_allows_api_key_if_login_method_not_set_but_f ) .await; - super::enforce_login_restrictions(&config).expect("matching workspace should succeed"); + super::enforce_login_restrictions(&config) + .await + .expect("matching workspace should succeed"); assert!( codex_home.path().join("auth.json").exists(), "auth.json should remain when restrictions pass" @@ -782,6 +796,7 @@ async fn enforce_login_restrictions_blocks_env_api_key_when_chatgpt_required() { .await; let err = super::enforce_login_restrictions(&config) + .await .expect_err("environment API key should not satisfy forced ChatGPT login"); assert!( err.to_string() @@ -818,8 +833,8 @@ fn fake_agent_identity_jwt(record: &AgentIdentityAuthRecord) -> std::io::Result< Ok(format!("{header_b64}.{payload_b64}.{signature_b64}")) } -#[test] -fn plan_type_maps_known_plan() { +#[tokio::test] +async fn plan_type_maps_known_plan() { let codex_home = tempdir().unwrap(); let _jwt = write_auth_file( AuthFileParams { @@ -836,14 +851,15 @@ fn plan_type_maps_known_plan() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); pretty_assertions::assert_eq!(auth.account_plan_type(), Some(AccountPlanType::Pro)); } -#[test] -fn plan_type_maps_self_serve_business_usage_based_plan() { +#[tokio::test] +async fn plan_type_maps_self_serve_business_usage_based_plan() { let codex_home = tempdir().unwrap(); let _jwt = write_auth_file( AuthFileParams { @@ -860,6 +876,7 @@ fn plan_type_maps_self_serve_business_usage_based_plan() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); @@ -869,8 +886,8 @@ fn plan_type_maps_self_serve_business_usage_based_plan() { ); } -#[test] -fn plan_type_maps_enterprise_cbp_usage_based_plan() { +#[tokio::test] +async fn plan_type_maps_enterprise_cbp_usage_based_plan() { let codex_home = tempdir().unwrap(); let _jwt = write_auth_file( AuthFileParams { @@ -887,6 +904,7 @@ fn plan_type_maps_enterprise_cbp_usage_based_plan() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); @@ -896,8 +914,8 @@ fn plan_type_maps_enterprise_cbp_usage_based_plan() { ); } -#[test] -fn plan_type_maps_unknown_to_unknown() { +#[tokio::test] +async fn plan_type_maps_unknown_to_unknown() { let codex_home = tempdir().unwrap(); let _jwt = write_auth_file( AuthFileParams { @@ -914,14 +932,15 @@ fn plan_type_maps_unknown_to_unknown() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); pretty_assertions::assert_eq!(auth.account_plan_type(), Some(AccountPlanType::Unknown)); } -#[test] -fn missing_plan_type_maps_to_unknown() { +#[tokio::test] +async fn missing_plan_type_maps_to_unknown() { let codex_home = tempdir().unwrap(); let _jwt = write_auth_file( AuthFileParams { @@ -938,6 +957,7 @@ fn missing_plan_type_maps_to_unknown() { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, ) + .await .expect("load auth") .expect("auth available"); diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index 85c9b6f024..ad6d644493 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -193,7 +193,7 @@ impl From for std::io::Error { } impl CodexAuth { - fn from_auth_dot_json( + async fn from_auth_dot_json( codex_home: &Path, auth_dot_json: AuthDotJson, auth_credentials_store_mode: AuthCredentialsStoreMode, @@ -234,7 +234,7 @@ impl CodexAuth { } } - pub fn from_auth_storage( + pub async fn from_auth_storage( codex_home: &Path, auth_credentials_store_mode: AuthCredentialsStoreMode, ) -> std::io::Result> { @@ -243,6 +243,7 @@ impl CodexAuth { /*enable_codex_api_key_env*/ false, auth_credentials_store_mode, ) + .await } pub fn from_agent_identity_jwt(jwt: &str) -> std::io::Result { @@ -522,6 +523,7 @@ pub async fn logout_with_revoke( auth_credentials_store_mode, /*chatgpt_base_url*/ None, ) + .await .logout_with_revoke() .await } @@ -609,12 +611,13 @@ pub struct AuthConfig { pub forced_chatgpt_workspace_id: Option, } -pub fn enforce_login_restrictions(config: &AuthConfig) -> std::io::Result<()> { +pub async fn enforce_login_restrictions(config: &AuthConfig) -> std::io::Result<()> { let Some(auth) = load_auth( &config.codex_home, /*enable_codex_api_key_env*/ true, config.auth_credentials_store_mode, - )? + ) + .await? else { return Ok(()); }; @@ -714,15 +717,11 @@ fn logout_all_stores( Ok(removed_ephemeral || removed_managed) } -fn load_auth( +async fn load_auth( codex_home: &Path, enable_codex_api_key_env: bool, auth_credentials_store_mode: AuthCredentialsStoreMode, ) -> std::io::Result> { - let build_auth = |auth_dot_json: AuthDotJson, storage_mode| { - CodexAuth::from_auth_dot_json(codex_home, auth_dot_json, storage_mode) - }; - // API key via env var takes precedence over any other auth method. if enable_codex_api_key_env && let Some(api_key) = read_codex_api_key_from_env() { return Ok(Some(CodexAuth::from_api_key(api_key.as_str()))); @@ -735,7 +734,12 @@ fn load_auth( AuthCredentialsStoreMode::Ephemeral, ); if let Some(auth_dot_json) = ephemeral_storage.load()? { - let auth = build_auth(auth_dot_json, AuthCredentialsStoreMode::Ephemeral)?; + let auth = CodexAuth::from_auth_dot_json( + codex_home, + auth_dot_json, + AuthCredentialsStoreMode::Ephemeral, + ) + .await?; return Ok(Some(auth)); } @@ -755,7 +759,9 @@ fn load_auth( None => return Ok(None), }; - let auth = build_auth(auth_dot_json, auth_credentials_store_mode)?; + let auth = + CodexAuth::from_auth_dot_json(codex_home, auth_dot_json, auth_credentials_store_mode) + .await?; Ok(Some(auth)) } @@ -1169,6 +1175,7 @@ impl UnauthorizedRecovery { match self .manager .reload_if_account_id_matches(self.expected_account_id.as_deref()) + .await { ReloadOutcome::ReloadedChanged => { self.step = UnauthorizedRecoveryStep::RefreshToken; @@ -1279,7 +1286,7 @@ impl AuthManager { /// preferred auth method. Errors loading auth are swallowed; `auth()` will /// simply return `None` in that case so callers can treat it as an /// unauthenticated state. - pub fn new( + pub async fn new( codex_home: PathBuf, enable_codex_api_key_env: bool, auth_credentials_store_mode: AuthCredentialsStoreMode, @@ -1290,6 +1297,7 @@ impl AuthManager { enable_codex_api_key_env, auth_credentials_store_mode, ) + .await .ok() .flatten(); Self { @@ -1402,13 +1410,16 @@ impl AuthManager { /// Force a reload of the auth information from auth.json. Returns /// whether the auth value changed. - pub fn reload(&self) -> bool { + pub async fn reload(&self) -> bool { tracing::info!("Reloading auth"); - let new_auth = self.load_auth_from_storage(); + let new_auth = self.load_auth_from_storage().await; self.set_cached_auth(new_auth) } - fn reload_if_account_id_matches(&self, expected_account_id: Option<&str>) -> ReloadOutcome { + async fn reload_if_account_id_matches( + &self, + expected_account_id: Option<&str>, + ) -> ReloadOutcome { let expected_account_id = match expected_account_id { Some(account_id) => account_id, None => { @@ -1417,7 +1428,7 @@ impl AuthManager { } }; - let new_auth = self.load_auth_from_storage(); + let new_auth = self.load_auth_from_storage().await; let new_account_id = new_auth.as_ref().and_then(CodexAuth::get_account_id); if new_account_id.as_deref() != Some(expected_account_id) { @@ -1488,12 +1499,13 @@ impl AuthManager { } } - fn load_auth_from_storage(&self) -> Option { + async fn load_auth_from_storage(&self) -> Option { load_auth( &self.codex_home, self.enable_codex_api_key_env, self.auth_credentials_store_mode, ) + .await .ok() .flatten() } @@ -1557,22 +1569,25 @@ impl AuthManager { } /// Convenience constructor returning an `Arc` wrapper. - pub fn shared( + pub async fn shared( codex_home: PathBuf, enable_codex_api_key_env: bool, auth_credentials_store_mode: AuthCredentialsStoreMode, chatgpt_base_url: Option, ) -> Arc { - Arc::new(Self::new( - codex_home, - enable_codex_api_key_env, - auth_credentials_store_mode, - chatgpt_base_url, - )) + Arc::new( + Self::new( + codex_home, + enable_codex_api_key_env, + auth_credentials_store_mode, + chatgpt_base_url, + ) + .await, + ) } /// Convenience constructor returning an `Arc` wrapper from resolved config. - pub fn shared_from_config( + pub async fn shared_from_config( config: &impl AuthManagerConfig, enable_codex_api_key_env: bool, ) -> Arc { @@ -1581,7 +1596,8 @@ impl AuthManager { enable_codex_api_key_env, config.cli_auth_credentials_store_mode(), Some(config.chatgpt_base_url()), - ); + ) + .await; auth_manager.set_forced_chatgpt_workspace_id(config.forced_chatgpt_workspace_id()); auth_manager } @@ -1647,7 +1663,10 @@ impl AuthManager { .as_ref() .and_then(CodexAuth::get_account_id); - match self.reload_if_account_id_matches(expected_account_id.as_deref()) { + match self + .reload_if_account_id_matches(expected_account_id.as_deref()) + .await + { ReloadOutcome::ReloadedChanged => { tracing::info!("Skipping token refresh because auth changed after guarded reload."); Ok(()) @@ -1714,10 +1733,10 @@ impl AuthManager { /// if a file was removed, Ok(false) if no auth file existed. On success, /// reloads the in‑memory auth cache so callers immediately observe the /// unauthenticated state. - pub fn logout(&self) -> std::io::Result { + pub async fn logout(&self) -> std::io::Result { let removed = logout_all_stores(&self.codex_home, self.auth_credentials_store_mode)?; // Always reload to clear any cached auth (even if file absent). - self.reload(); + self.reload().await; Ok(removed) } @@ -1730,7 +1749,7 @@ impl AuthManager { } let result = logout_all_stores(&self.codex_home, self.auth_credentials_store_mode)?; // Always reload to clear any cached auth (even if file absent). - self.reload(); + self.reload().await; Ok(result) } @@ -1826,7 +1845,7 @@ impl AuthManager { AuthCredentialsStoreMode::Ephemeral, ) .map_err(RefreshTokenError::Transient)?; - self.reload(); + self.reload().await; Ok(()) } @@ -1846,7 +1865,7 @@ impl AuthManager { refresh_response.refresh_token, ) .map_err(RefreshTokenError::from)?; - self.reload(); + self.reload().await; Ok(()) } diff --git a/codex-rs/login/tests/suite/auth_refresh.rs b/codex-rs/login/tests/suite/auth_refresh.rs index 3ae0eb5fa1..afdf466d09 100644 --- a/codex-rs/login/tests/suite/auth_refresh.rs +++ b/codex-rs/login/tests/suite/auth_refresh.rs @@ -46,7 +46,7 @@ async fn refresh_token_succeeds_updates_storage() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -56,7 +56,7 @@ async fn refresh_token_succeeds_updates_storage() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; ctx.auth_manager .refresh_token_from_authority() @@ -110,7 +110,7 @@ async fn refresh_token_refreshes_when_auth_is_unchanged() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -120,7 +120,7 @@ async fn refresh_token_refreshes_when_auth_is_unchanged() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; ctx.auth_manager .refresh_token() @@ -164,7 +164,7 @@ async fn refresh_token_skips_refresh_when_auth_changed() -> Result<()> { skip_if_no_network!(Ok(())); let server = MockServer::start().await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); @@ -175,7 +175,7 @@ async fn refresh_token_skips_refresh_when_auth_changed() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); let disk_auth = AuthDotJson { @@ -230,7 +230,7 @@ async fn refresh_token_errors_on_account_mismatch() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -240,7 +240,7 @@ async fn refresh_token_errors_on_account_mismatch() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let mut disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); disk_tokens.account_id = Some("other-account".to_string()); @@ -299,7 +299,7 @@ async fn returns_fresh_tokens_as_is() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let stale_refresh = Utc::now() - Duration::days(9); let fresh_access_token = access_token_with_expiration(Utc::now() + Duration::hours(1)); let initial_tokens = build_tokens(&fresh_access_token, INITIAL_REFRESH_TOKEN); @@ -310,7 +310,7 @@ async fn returns_fresh_tokens_as_is() -> Result<()> { last_refresh: Some(stale_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let cached_auth = ctx .auth_manager @@ -347,7 +347,7 @@ async fn refreshes_token_when_access_token_is_expired() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let fresh_refresh = Utc::now() - Duration::days(1); let expired_access_token = access_token_with_expiration(Utc::now() - Duration::hours(1)); let initial_tokens = build_tokens(&expired_access_token, INITIAL_REFRESH_TOKEN); @@ -358,7 +358,7 @@ async fn refreshes_token_when_access_token_is_expired() -> Result<()> { last_refresh: Some(fresh_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let cached_auth = ctx .auth_manager @@ -398,7 +398,7 @@ async fn auth_reloads_disk_auth_when_cached_auth_is_stale() -> Result<()> { let server = MockServer::start().await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let stale_refresh = Utc::now() - Duration::days(9); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -408,7 +408,7 @@ async fn auth_reloads_disk_auth_when_cached_auth_is_stale() -> Result<()> { last_refresh: Some(stale_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let fresh_refresh = Utc::now() - Duration::days(1); let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); @@ -461,7 +461,7 @@ async fn auth_reloads_disk_auth_without_calling_expired_refresh_token() -> Resul .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let stale_refresh = Utc::now() - Duration::days(9); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -471,7 +471,7 @@ async fn auth_reloads_disk_auth_without_calling_expired_refresh_token() -> Resul last_refresh: Some(stale_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let fresh_refresh = Utc::now() - Duration::days(1); let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); @@ -522,7 +522,7 @@ async fn refresh_token_returns_permanent_error_for_expired_refresh_token() -> Re .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -532,7 +532,7 @@ async fn refresh_token_returns_permanent_error_for_expired_refresh_token() -> Re last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let err = ctx .auth_manager @@ -575,7 +575,7 @@ async fn refresh_token_does_not_retry_after_permanent_failure() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -585,7 +585,7 @@ async fn refresh_token_does_not_retry_after_permanent_failure() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let first_err = ctx .auth_manager @@ -642,7 +642,7 @@ async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result< .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -652,7 +652,7 @@ async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result< last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let first_err = ctx .auth_manager @@ -723,7 +723,7 @@ async fn refresh_token_returns_transient_error_on_server_failure() -> Result<()> .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -733,7 +733,7 @@ async fn refresh_token_returns_transient_error_on_server_failure() -> Result<()> last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let err = ctx .auth_manager @@ -776,7 +776,7 @@ async fn unauthorized_recovery_reloads_then_refreshes_tokens() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -786,7 +786,7 @@ async fn unauthorized_recovery_reloads_then_refreshes_tokens() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); let disk_auth = AuthDotJson { @@ -870,7 +870,7 @@ async fn unauthorized_recovery_errors_on_account_mismatch() -> Result<()> { .mount(&server) .await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); let initial_auth = AuthDotJson { @@ -880,7 +880,7 @@ async fn unauthorized_recovery_errors_on_account_mismatch() -> Result<()> { last_refresh: Some(initial_last_refresh), agent_identity: None, }; - ctx.write_auth(&initial_auth)?; + ctx.write_auth(&initial_auth).await?; let mut disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); disk_tokens.account_id = Some("other-account".to_string()); @@ -941,7 +941,7 @@ async fn unauthorized_recovery_requires_chatgpt_auth() -> Result<()> { skip_if_no_network!(Ok(())); let server = MockServer::start().await; - let ctx = RefreshTokenTestContext::new(&server)?; + let ctx = RefreshTokenTestContext::new(&server).await?; let auth = AuthDotJson { auth_mode: Some(AuthMode::ApiKey), openai_api_key: Some("sk-test".to_string()), @@ -949,7 +949,7 @@ async fn unauthorized_recovery_requires_chatgpt_auth() -> Result<()> { last_refresh: None, agent_identity: None, }; - ctx.write_auth(&auth)?; + ctx.write_auth(&auth).await?; let mut recovery = ctx.auth_manager.unauthorized_recovery(); assert!(!recovery.has_next()); @@ -974,7 +974,7 @@ struct RefreshTokenTestContext { } impl RefreshTokenTestContext { - fn new(server: &MockServer) -> Result { + async fn new(server: &MockServer) -> Result { let codex_home = TempDir::new()?; let endpoint = format!("{}/oauth/token", server.uri()); @@ -985,7 +985,8 @@ impl RefreshTokenTestContext { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; Ok(Self { codex_home, @@ -1000,13 +1001,13 @@ impl RefreshTokenTestContext { .context("auth.json should exist") } - fn write_auth(&self, auth_dot_json: &AuthDotJson) -> Result<()> { + async fn write_auth(&self, auth_dot_json: &AuthDotJson) -> Result<()> { save_auth( self.codex_home.path(), auth_dot_json, AuthCredentialsStoreMode::File, )?; - self.auth_manager.reload(); + self.auth_manager.reload().await; Ok(()) } } diff --git a/codex-rs/login/tests/suite/logout.rs b/codex-rs/login/tests/suite/logout.rs index e703b15eb1..2364ee7f54 100644 --- a/codex-rs/login/tests/suite/logout.rs +++ b/codex-rs/login/tests/suite/logout.rs @@ -143,7 +143,8 @@ async fn auth_manager_logout_with_revoke_uses_cached_auth() -> Result<()> { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, /*chatgpt_base_url*/ None, - ); + ) + .await; save_auth( codex_home.path(), &chatgpt_auth_with_refresh_token("newer-disk-refresh-token"), diff --git a/codex-rs/mcp-server/src/lib.rs b/codex-rs/mcp-server/src/lib.rs index 1d904e4577..95fc4341ee 100644 --- a/codex-rs/mcp-server/src/lib.rs +++ b/codex-rs/mcp-server/src/lib.rs @@ -141,7 +141,8 @@ pub async fn run_main( arg0_paths, Arc::new(config), environment_manager, - ); + ) + .await; async move { while let Some(msg) = incoming_rx.recv().await { match msg { diff --git a/codex-rs/mcp-server/src/message_processor.rs b/codex-rs/mcp-server/src/message_processor.rs index e0a1e77f68..32d05d1f93 100644 --- a/codex-rs/mcp-server/src/message_processor.rs +++ b/codex-rs/mcp-server/src/message_processor.rs @@ -49,7 +49,7 @@ pub(crate) struct MessageProcessor { impl MessageProcessor { /// Create a new `MessageProcessor`, retaining a handle to the outgoing /// `Sender` so handlers can enqueue messages to be written to stdout. - pub(crate) fn new( + pub(crate) async fn new( outgoing: OutgoingMessageSender, arg0_paths: Arg0DispatchPaths, config: Arc, @@ -59,7 +59,8 @@ impl MessageProcessor { let auth_manager = AuthManager::shared_from_config( config.as_ref(), /*enable_codex_api_key_env*/ false, - ); + ) + .await; let thread_manager = Arc::new(ThreadManager::new( config.as_ref(), auth_manager, diff --git a/codex-rs/models-manager/src/manager_tests.rs b/codex-rs/models-manager/src/manager_tests.rs index 4046b7565f..57c42be015 100644 --- a/codex-rs/models-manager/src/manager_tests.rs +++ b/codex-rs/models-manager/src/manager_tests.rs @@ -206,7 +206,7 @@ fn static_manager_for_tests(model_catalog: ModelsResponse) -> StaticModelsManage ) } -fn chatgpt_auth_tokens_for_tests(codex_home: &Path) -> CodexAuth { +async fn chatgpt_auth_tokens_for_tests(codex_home: &Path) -> CodexAuth { let auth_dot_json = codex_login::AuthDotJson { auth_mode: Some(AuthMode::ChatgptAuthTokens), openai_api_key: None, @@ -232,6 +232,7 @@ c2ln", .expect("auth.json should be written"); CodexAuth::from_auth_storage(codex_home, AuthCredentialsStoreMode::File) + .await .expect("auth should load") .expect("auth should be present") } @@ -685,7 +686,7 @@ async fn refresh_available_models_fetches_with_chatgpt_auth_tokens() { "ChatGPT Auth Tokens", /*priority*/ 1, )]]); - let auth = chatgpt_auth_tokens_for_tests(codex_home.path()); + let auth = chatgpt_auth_tokens_for_tests(codex_home.path()).await; let manager = openai_manager_for_tests_with_auth( codex_home.path().to_path_buf(), endpoint.clone(), diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 46b2fd1d21..6dd527c713 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -795,7 +795,8 @@ pub async fn run_main( /*enable_codex_api_key_env*/ false, config_toml.cli_auth_credentials_store.unwrap_or_default(), chatgpt_base_url, - ); + ) + .await; let model_provider_override = if cli.oss { let resolved = resolve_oss_provider( @@ -895,7 +896,9 @@ pub async fn run_main( auth_credentials_store_mode: config.cli_auth_credentials_store_mode, forced_login_method: config.forced_login_method, forced_chatgpt_workspace_id: config.forced_chatgpt_workspace_id.clone(), - }) { + }) + .await + { eprintln!("{err}"); std::process::exit(1); } @@ -1166,7 +1169,8 @@ async fn run_ratatui_app( /*enable_codex_api_key_env*/ false, initial_config.cli_auth_credentials_store_mode, initial_config.chatgpt_base_url.clone(), - ); + ) + .await; } // If the user made an explicit trust decision, or we showed the login flow, reload config diff --git a/codex-rs/tui/src/onboarding/auth.rs b/codex-rs/tui/src/onboarding/auth.rs index 1e55b5c5d2..cbe1a1fe8b 100644 --- a/codex-rs/tui/src/onboarding/auth.rs +++ b/codex-rs/tui/src/onboarding/auth.rs @@ -986,7 +986,8 @@ mod tests { /*enable_codex_api_key_env*/ false, AuthCredentialsStoreMode::File, "https://chatgpt.com/backend-api/".to_string(), - ), + ) + .await, feedback: codex_feedback::CodexFeedback::new(), log_db: None, environment_manager: Arc::new( From c2084552d9457625f52166d71b9382602622a375 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 27 Apr 2026 11:32:48 -0700 Subject: [PATCH 06/11] ci: pin npm staging smoke test to a recent rust-release run (#19854) ## Why The `build-test` workflow stages a representative `codex` npm tarball by asking `scripts/stage_npm_packages.py` to look up a past `rust-release` run for a hardcoded release version. That started failing in CI because the representative version in `.github/workflows/ci.yml` was stale: - the workflow was still using `0.115.0` - `stage_npm_packages.py` resolves native artifacts by looking for a `rust-release` run on the `rust-v` branch - that lookup no longer found a matching run for `rust-v0.115.0`, so the smoke test failed before it could stage the package This PR makes that smoke test depend on a known-good recent release run instead of an older branch lookup that is no longer reliable. ## What Changed - Updated the representative release version in `.github/workflows/ci.yml` from `0.115.0` to `0.125.0`. - Added an explicit `WORKFLOW_URL` pointing at a recent successful `rust-release` run: `https://github.com/openai/codex/actions/runs/24901475298`. - Passed that URL to `scripts/stage_npm_packages.py` via `--workflow-url` so the job can reuse the expected native artifacts directly instead of relying on `gh run list --branch rust-v` to discover them. That keeps the npm staging smoke test representative while making it less sensitive to older release branch history disappearing from the GitHub Actions lookup path. ## Verification - Inspected the failing CI log from `build-test` and confirmed the failure came from `scripts/stage_npm_packages.py` being unable to resolve `rust-v0.115.0`. - Confirmed that `https://github.com/openai/codex/actions/runs/24901475298` is a successful `rust-release` run for `rust-v0.125.0`. --- .github/workflows/ci.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c745106cb..bb5ed5b5bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,11 +45,16 @@ jobs: GH_TOKEN: ${{ github.token }} run: | set -euo pipefail - # Use a rust-release version that includes all native binaries. - CODEX_VERSION=0.115.0 + # Use a recent successful rust-release run that published the full + # cross-platform native payload required by the npm package layout. + # Passing the workflow URL directly avoids relying on old rust-v* + # branches remaining discoverable via `gh run list --branch ...`. + CODEX_VERSION=0.125.0 + WORKFLOW_URL="https://github.com/openai/codex/actions/runs/24901475298" OUTPUT_DIR="${RUNNER_TEMP}" python3 ./scripts/stage_npm_packages.py \ --release-version "$CODEX_VERSION" \ + --workflow-url "$WORKFLOW_URL" \ --package codex \ --output-dir "$OUTPUT_DIR" PACK_OUTPUT="${OUTPUT_DIR}/codex-npm-${CODEX_VERSION}.tgz" From cafe717dca8ca24ebac0a850ef8cf169ca51d030 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 27 Apr 2026 11:37:30 -0700 Subject: [PATCH 07/11] ci: migrate Bazel setup away from archived setup-bazelisk (#19851) ## Why All Bazel CI jobs are currently blocked in the `setup-bazelisk` step while trying to download Bazelisk. [`bazelbuild/setup-bazelisk`](https://github.com/bazelbuild/setup-bazelisk) is archived, and its README now recommends migrating to [`bazel-contrib/setup-bazel`](https://github.com/bazel-contrib/setup-bazel), so leaving our workflows on the archived action leaves CI exposed to exactly this sort of outage. Because `v8-canary` now consumes the shared local `setup-bazel-ci` action, that workflow also needs to trigger when the action changes. Without that follow-up, Bazel bootstrap regressions specific to the V8 canary path could be skipped by the workflow path filters. ## What Changed - Switched `.github/actions/setup-bazel-ci/action.yml` from `bazelbuild/setup-bazelisk` to `bazel-contrib/setup-bazel`, pinned to `0.19.0`. - Left `bazelisk-version` unset so GitHub-hosted runners can use their preinstalled Bazelisk instead of downloading `1.x` at job start. - Updated `.github/workflows/rusty-v8-release.yml` and `.github/workflows/v8-canary.yml` to use the shared `setup-bazel-ci` action instead of referencing `setup-bazelisk` directly. - Added `.github/actions/setup-bazel-ci/**` to the `pull_request` and `push` path filters in `.github/workflows/v8-canary.yml` so changes to the shared Bazel setup action still run the canary workflow. - Kept the existing repository-cache and Windows-specific Bazel setup logic intact. This keeps Bazel version selection anchored by `.bazelversion` while removing the failing dependency on the archived setup action. ## Verification - Searched `.github/` to confirm there are no remaining `setup-bazelisk` references. - Parsed the updated workflow and action YAML locally with Ruby's `YAML.load_file`. --- .github/actions/setup-bazel-ci/action.yml | 2 +- .github/workflows/rusty-v8-release.yml | 4 +++- .github/workflows/v8-canary.yml | 6 +++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/actions/setup-bazel-ci/action.yml b/.github/actions/setup-bazel-ci/action.yml index 881209fd81..bb757aab91 100644 --- a/.github/actions/setup-bazel-ci/action.yml +++ b/.github/actions/setup-bazel-ci/action.yml @@ -33,7 +33,7 @@ runs: run: Copy-Item (Get-Command dotslash).Source -Destination "$env:LOCALAPPDATA\Microsoft\WindowsApps\dotslash.exe" - name: Set up Bazel - uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3 + uses: bazel-contrib/setup-bazel@c5acdfb288317d0b5c0bbd7a396a3dc868bb0f86 # 0.19.0 - name: Configure Bazel repository cache id: configure_bazel_repository_cache diff --git a/.github/workflows/rusty-v8-release.yml b/.github/workflows/rusty-v8-release.yml index 29e7b3b1ae..ee92eff4fa 100644 --- a/.github/workflows/rusty-v8-release.yml +++ b/.github/workflows/rusty-v8-release.yml @@ -78,7 +78,9 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 - name: Set up Bazel - uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3 + uses: ./.github/actions/setup-bazel-ci + with: + target: ${{ matrix.target }} - name: Set up Python uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6 diff --git a/.github/workflows/v8-canary.yml b/.github/workflows/v8-canary.yml index f5aa1d7c67..119d042275 100644 --- a/.github/workflows/v8-canary.yml +++ b/.github/workflows/v8-canary.yml @@ -3,6 +3,7 @@ name: v8-canary on: pull_request: paths: + - ".github/actions/setup-bazel-ci/**" - ".github/scripts/rusty_v8_bazel.py" - ".github/workflows/rusty-v8-release.yml" - ".github/workflows/v8-canary.yml" @@ -16,6 +17,7 @@ on: branches: - main paths: + - ".github/actions/setup-bazel-ci/**" - ".github/scripts/rusty_v8_bazel.py" - ".github/workflows/rusty-v8-release.yml" - ".github/workflows/v8-canary.yml" @@ -75,7 +77,9 @@ jobs: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 - name: Set up Bazel - uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3 + uses: ./.github/actions/setup-bazel-ci + with: + target: ${{ matrix.target }} - name: Set up Python uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6 From e5709db6dc575b722953aa397a4856203711f4e5 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 27 Apr 2026 12:03:49 -0700 Subject: [PATCH 08/11] Streamline account and command handlers (#19491) ## Why Account login/logout and command exec handlers were doing local error sends in the middle of each handler. That made these request flows branch heavily even though most of the logic is validate, perform the operation, and return the response. ## What Changed - Converted ChatGPT/API-key login, login cancel, logout, rate-limit, and add-credit handlers in `codex-rs/app-server/src/codex_message_processor.rs` to compute `Result` values and send them once at the request boundary. - Applied the same shape to command exec start/write/resize/terminate handlers. - Kept side-effect notifications in the same places after successful request handling. ## Verification - `cargo check -p codex-app-server` - `cargo test -p codex-app-server --test all v2::account -- --test-threads=1` - `cargo test -p codex-app-server --test all v2::command_exec -- --test-threads=1` --- .../app-server/src/codex_message_processor.rs | 833 ++++++++---------- 1 file changed, 361 insertions(+), 472 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 814fee9d8c..d982fd5331 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1366,31 +1366,16 @@ impl CodexMessageProcessor { } async fn login_api_key_v2(&self, request_id: ConnectionRequestId, params: LoginApiKeyParams) { - match self.login_api_key_common(¶ms).await { - Ok(()) => { - let response = codex_app_server_protocol::LoginAccountResponse::ApiKey {}; - self.outgoing.send_response(request_id, response).await; + let result = self + .login_api_key_common(¶ms) + .await + .map(|()| LoginAccountResponse::ApiKey {}); + let logged_in = result.is_ok(); + self.outgoing.send_result(request_id, result).await; - let payload_login_completed = AccountLoginCompletedNotification { - login_id: None, - success: true, - error: None, - }; - self.outgoing - .send_server_notification(ServerNotification::AccountLoginCompleted( - payload_login_completed, - )) - .await; - - self.outgoing - .send_server_notification(ServerNotification::AccountUpdated( - self.current_account_updated_notification(), - )) - .await; - } - Err(error) => { - self.outgoing.send_error(request_id, error).await; - } + if logged_in { + self.send_login_success_notifications(/*login_id*/ None) + .await; } } @@ -1453,202 +1438,143 @@ impl CodexMessageProcessor { } async fn login_chatgpt_v2(&self, request_id: ConnectionRequestId) { - match self.login_chatgpt_common().await { - Ok(opts) => match run_login_server(opts) { - Ok(server) => { - let login_id = Uuid::new_v4(); - let shutdown_handle = server.cancel_handle(); + let result = self.login_chatgpt_response().await; + self.outgoing.send_result(request_id, result).await; + } - // Replace active login if present. - { - let mut guard = self.active_login.lock().await; - if let Some(existing) = guard.take() { - drop(existing); - } - *guard = Some(ActiveLogin::Browser { - shutdown_handle: shutdown_handle.clone(), - login_id, - }); - } + async fn login_chatgpt_response(&self) -> Result { + let opts = self.login_chatgpt_common().await?; + let server = run_login_server(opts) + .map_err(|err| internal_error(format!("failed to start login server: {err}")))?; + let login_id = Uuid::new_v4(); + let shutdown_handle = server.cancel_handle(); - // Spawn background task to monitor completion. - let outgoing_clone = self.outgoing.clone(); - let active_login = self.active_login.clone(); - let auth_manager = self.auth_manager.clone(); - let config_manager = self.config_manager.clone(); - let chatgpt_base_url = self.config.chatgpt_base_url.clone(); - let auth_url = server.auth_url.clone(); - tokio::spawn(async move { - let (success, error_msg) = match tokio::time::timeout( - LOGIN_CHATGPT_TIMEOUT, - server.block_until_done(), - ) - .await - { - Ok(Ok(())) => (true, None), - Ok(Err(err)) => (false, Some(format!("Login server error: {err}"))), - Err(_elapsed) => { - shutdown_handle.shutdown(); - (false, Some("Login timed out".to_string())) - } - }; - - let payload_v2 = AccountLoginCompletedNotification { - login_id: Some(login_id.to_string()), - success, - error: error_msg, - }; - outgoing_clone - .send_server_notification(ServerNotification::AccountLoginCompleted( - payload_v2, - )) - .await; - - if success { - auth_manager.reload().await; - config_manager.replace_cloud_requirements_loader( - auth_manager.clone(), - chatgpt_base_url, - ); - config_manager - .sync_default_client_residency_requirement() - .await; - - // Notify clients with the actual current auth mode. - let auth = auth_manager.auth_cached(); - let payload_v2 = AccountUpdatedNotification { - auth_mode: auth.as_ref().map(CodexAuth::api_auth_mode), - plan_type: auth.as_ref().and_then(CodexAuth::account_plan_type), - }; - outgoing_clone - .send_server_notification(ServerNotification::AccountUpdated( - payload_v2, - )) - .await; - } - - // Clear the active login if it matches this attempt. It may have been replaced or cancelled. - let mut guard = active_login.lock().await; - if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { - *guard = None; - } - }); - - let response = codex_app_server_protocol::LoginAccountResponse::Chatgpt { - login_id: login_id.to_string(), - auth_url, - }; - self.outgoing.send_response(request_id, response).await; - } - Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to start login server: {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - } - }, - Err(err) => { - self.outgoing.send_error(request_id, err).await; + // Replace active login if present. + { + let mut guard = self.active_login.lock().await; + if let Some(existing) = guard.take() { + drop(existing); } + *guard = Some(ActiveLogin::Browser { + shutdown_handle: shutdown_handle.clone(), + login_id, + }); } + + let outgoing_clone = self.outgoing.clone(); + let active_login = self.active_login.clone(); + let auth_manager = self.auth_manager.clone(); + let config_manager = self.config_manager.clone(); + let chatgpt_base_url = self.config.chatgpt_base_url.clone(); + let auth_url = server.auth_url.clone(); + tokio::spawn(async move { + let (success, error_msg) = match tokio::time::timeout( + LOGIN_CHATGPT_TIMEOUT, + server.block_until_done(), + ) + .await + { + Ok(Ok(())) => (true, None), + Ok(Err(err)) => (false, Some(format!("Login server error: {err}"))), + Err(_elapsed) => { + shutdown_handle.shutdown(); + (false, Some("Login timed out".to_string())) + } + }; + + Self::send_chatgpt_login_completion_notifications( + &outgoing_clone, + auth_manager, + config_manager, + chatgpt_base_url, + login_id, + success, + error_msg, + ) + .await; + + // Clear the active login if it matches this attempt. It may have been replaced or cancelled. + let mut guard = active_login.lock().await; + if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { + *guard = None; + } + }); + + Ok(LoginAccountResponse::Chatgpt { + login_id: login_id.to_string(), + auth_url, + }) } async fn login_chatgpt_device_code_v2(&self, request_id: ConnectionRequestId) { - match self.login_chatgpt_common().await { - Ok(opts) => match request_device_code(&opts).await { - Ok(device_code) => { - let login_id = Uuid::new_v4(); - let cancel = CancellationToken::new(); + let result = self.login_chatgpt_device_code_response().await; + self.outgoing.send_result(request_id, result).await; + } - { - let mut guard = self.active_login.lock().await; - if let Some(existing) = guard.take() { - drop(existing); - } - *guard = Some(ActiveLogin::DeviceCode { - cancel: cancel.clone(), - login_id, - }); - } + async fn login_chatgpt_device_code_response( + &self, + ) -> Result { + let opts = self.login_chatgpt_common().await?; + let device_code = request_device_code(&opts) + .await + .map_err(Self::login_chatgpt_device_code_start_error)?; + let login_id = Uuid::new_v4(); + let cancel = CancellationToken::new(); - let verification_url = device_code.verification_url.clone(); - let user_code = device_code.user_code.clone(); - let response = - codex_app_server_protocol::LoginAccountResponse::ChatgptDeviceCode { - login_id: login_id.to_string(), - verification_url, - user_code, - }; - self.outgoing.send_response(request_id, response).await; - - let outgoing_clone = self.outgoing.clone(); - let active_login = self.active_login.clone(); - let auth_manager = self.auth_manager.clone(); - let config_manager = self.config_manager.clone(); - let chatgpt_base_url = self.config.chatgpt_base_url.clone(); - tokio::spawn(async move { - let (success, error_msg) = tokio::select! { - _ = cancel.cancelled() => { - (false, Some("Login was not completed".to_string())) - } - r = complete_device_code_login(opts, device_code) => { - match r { - Ok(()) => (true, None), - Err(err) => (false, Some(err.to_string())), - } - } - }; - - let payload_v2 = AccountLoginCompletedNotification { - login_id: Some(login_id.to_string()), - success, - error: error_msg, - }; - outgoing_clone - .send_server_notification(ServerNotification::AccountLoginCompleted( - payload_v2, - )) - .await; - - if success { - auth_manager.reload().await; - config_manager.replace_cloud_requirements_loader( - auth_manager.clone(), - chatgpt_base_url, - ); - config_manager - .sync_default_client_residency_requirement() - .await; - - let auth = auth_manager.auth_cached(); - let payload_v2 = AccountUpdatedNotification { - auth_mode: auth.as_ref().map(CodexAuth::api_auth_mode), - plan_type: auth.as_ref().and_then(CodexAuth::account_plan_type), - }; - outgoing_clone - .send_server_notification(ServerNotification::AccountUpdated( - payload_v2, - )) - .await; - } - - let mut guard = active_login.lock().await; - if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { - *guard = None; - } - }); - } - Err(err) => { - let error = Self::login_chatgpt_device_code_start_error(err); - self.outgoing.send_error(request_id, error).await; - } - }, - Err(err) => { - self.outgoing.send_error(request_id, err).await; + { + let mut guard = self.active_login.lock().await; + if let Some(existing) = guard.take() { + drop(existing); } + *guard = Some(ActiveLogin::DeviceCode { + cancel: cancel.clone(), + login_id, + }); } + + let verification_url = device_code.verification_url.clone(); + let user_code = device_code.user_code.clone(); + + let outgoing_clone = self.outgoing.clone(); + let active_login = self.active_login.clone(); + let auth_manager = self.auth_manager.clone(); + let config_manager = self.config_manager.clone(); + let chatgpt_base_url = self.config.chatgpt_base_url.clone(); + tokio::spawn(async move { + let (success, error_msg) = tokio::select! { + _ = cancel.cancelled() => { + (false, Some("Login was not completed".to_string())) + } + r = complete_device_code_login(opts, device_code) => { + match r { + Ok(()) => (true, None), + Err(err) => (false, Some(err.to_string())), + } + } + }; + + Self::send_chatgpt_login_completion_notifications( + &outgoing_clone, + auth_manager, + config_manager, + chatgpt_base_url, + login_id, + success, + error_msg, + ) + .await; + + let mut guard = active_login.lock().await; + if guard.as_ref().map(ActiveLogin::login_id) == Some(login_id) { + *guard = None; + } + }); + + Ok(LoginAccountResponse::ChatgptDeviceCode { + login_id: login_id.to_string(), + verification_url, + user_code, + }) } async fn cancel_login_chatgpt_common( @@ -1671,25 +1597,22 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: CancelLoginAccountParams, ) { + let result = self.cancel_login_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn cancel_login_response( + &self, + params: CancelLoginAccountParams, + ) -> Result { let login_id = params.login_id; - match Uuid::parse_str(&login_id) { - Ok(uuid) => { - let status = match self.cancel_login_chatgpt_common(uuid).await { - Ok(()) => CancelLoginAccountStatus::Canceled, - Err(CancelLoginError::NotFound) => CancelLoginAccountStatus::NotFound, - }; - let response = CancelLoginAccountResponse { status }; - self.outgoing.send_response(request_id, response).await; - } - Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid login id: {login_id}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - } - } + let uuid = Uuid::parse_str(&login_id) + .map_err(|_| invalid_request(format!("invalid login id: {login_id}")))?; + let status = match self.cancel_login_chatgpt_common(uuid).await { + Ok(()) => CancelLoginAccountStatus::Canceled, + Err(CancelLoginError::NotFound) => CancelLoginAccountStatus::NotFound, + }; + Ok(CancelLoginAccountResponse { status }) } async fn login_chatgpt_auth_tokens( @@ -1699,18 +1622,31 @@ impl CodexMessageProcessor { chatgpt_account_id: String, chatgpt_plan_type: Option, ) { + let result = self + .login_chatgpt_auth_tokens_response(access_token, chatgpt_account_id, chatgpt_plan_type) + .await; + let logged_in = result.is_ok(); + self.outgoing.send_result(request_id, result).await; + + if logged_in { + self.send_login_success_notifications(/*login_id*/ None) + .await; + } + } + + async fn login_chatgpt_auth_tokens_response( + &self, + access_token: String, + chatgpt_account_id: String, + chatgpt_plan_type: Option, + ) -> Result { if matches!( self.config.forced_login_method, Some(ForcedLoginMethod::Api) ) { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "External ChatGPT auth is disabled. Use API key login instead." - .to_string(), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request( + "External ChatGPT auth is disabled. Use API key login instead.", + )); } // Cancel any active login attempt to avoid persisting managed auth state. @@ -1724,31 +1660,18 @@ impl CodexMessageProcessor { if let Some(expected_workspace) = self.config.forced_chatgpt_workspace_id.as_deref() && chatgpt_account_id != expected_workspace { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!( - "External auth must use workspace {expected_workspace}, but received {chatgpt_account_id:?}." - ), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request(format!( + "External auth must use workspace {expected_workspace}, but received {chatgpt_account_id:?}." + ))); } - if let Err(err) = login_with_chatgpt_auth_tokens( + login_with_chatgpt_auth_tokens( &self.config.codex_home, &access_token, &chatgpt_account_id, chatgpt_plan_type.as_deref(), - ) { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to set external auth: {err}"), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; - } + ) + .map_err(|err| internal_error(format!("failed to set external auth: {err}")))?; self.auth_manager.reload().await; self.config_manager.replace_cloud_requirements_loader( self.auth_manager.clone(), @@ -1758,12 +1681,12 @@ impl CodexMessageProcessor { .sync_default_client_residency_requirement() .await; - self.outgoing - .send_response(request_id, LoginAccountResponse::ChatgptAuthTokens {}) - .await; + Ok(LoginAccountResponse::ChatgptAuthTokens {}) + } + async fn send_login_success_notifications(&self, login_id: Option) { let payload_login_completed = AccountLoginCompletedNotification { - login_id: None, + login_id: login_id.map(|id| id.to_string()), success: true, error: None, }; @@ -1780,6 +1703,43 @@ impl CodexMessageProcessor { .await; } + async fn send_chatgpt_login_completion_notifications( + outgoing: &OutgoingMessageSender, + auth_manager: Arc, + config_manager: ConfigManager, + chatgpt_base_url: String, + login_id: Uuid, + success: bool, + error_msg: Option, + ) { + let payload_v2 = AccountLoginCompletedNotification { + login_id: Some(login_id.to_string()), + success, + error: error_msg, + }; + outgoing + .send_server_notification(ServerNotification::AccountLoginCompleted(payload_v2)) + .await; + + if success { + auth_manager.reload().await; + config_manager + .replace_cloud_requirements_loader(auth_manager.clone(), chatgpt_base_url); + config_manager + .sync_default_client_residency_requirement() + .await; + + let auth = auth_manager.auth_cached(); + let payload_v2 = AccountUpdatedNotification { + auth_mode: auth.as_ref().map(CodexAuth::api_auth_mode), + plan_type: auth.as_ref().and_then(CodexAuth::account_plan_type), + }; + outgoing + .send_server_notification(ServerNotification::AccountUpdated(payload_v2)) + .await; + } + } + async fn logout_common(&self) -> std::result::Result, JSONRPCErrorError> { // Cancel any active login attempt. { @@ -1809,23 +1769,24 @@ impl CodexMessageProcessor { } async fn logout_v2(&self, request_id: ConnectionRequestId) { - match self.logout_common().await { - Ok(current_auth_method) => { - self.outgoing - .send_response(request_id, LogoutAccountResponse {}) - .await; - - let payload_v2 = AccountUpdatedNotification { - auth_mode: current_auth_method, + let result = self.logout_common().await; + let account_updated = + result + .as_ref() + .ok() + .cloned() + .map(|auth_mode| AccountUpdatedNotification { + auth_mode, plan_type: None, - }; - self.outgoing - .send_server_notification(ServerNotification::AccountUpdated(payload_v2)) - .await; - } - Err(error) => { - self.outgoing.send_error(request_id, error).await; - } + }); + self.outgoing + .send_result(request_id, result.map(|_| LogoutAccountResponse {})) + .await; + + if let Some(payload) = account_updated { + self.outgoing + .send_server_notification(ServerNotification::AccountUpdated(payload)) + .await; } } @@ -1908,6 +1869,14 @@ impl CodexMessageProcessor { } async fn get_account(&self, request_id: ConnectionRequestId, params: GetAccountParams) { + let result = self.get_account_response(params).await; + self.outgoing.send_result(request_id, result).await; + } + + async fn get_account_response( + &self, + params: GetAccountParams, + ) -> Result { let do_refresh = params.refresh_token; self.refresh_token_if_requested(do_refresh).await; @@ -1919,43 +1888,35 @@ impl CodexMessageProcessor { let account_state = match provider.account_state() { Ok(account_state) => account_state, Err(ProviderAccountError::MissingChatgptAccountDetails) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "email and plan type are required for chatgpt authentication" - .to_string(), - data: None, - }; - self.outgoing.send_error(request_id, error).await; - return; + return Err(invalid_request( + "email and plan type are required for chatgpt authentication", + )); } }; let account = account_state.account.map(Account::from); - let response = GetAccountResponse { + Ok(GetAccountResponse { account, requires_openai_auth: account_state.requires_openai_auth, - }; - self.outgoing.send_response(request_id, response).await; + }) } async fn get_account_rate_limits(&self, request_id: ConnectionRequestId) { - match self.fetch_account_rate_limits().await { - Ok((rate_limits, rate_limits_by_limit_id)) => { - let response = GetAccountRateLimitsResponse { - rate_limits: rate_limits.into(), - rate_limits_by_limit_id: Some( - rate_limits_by_limit_id - .into_iter() - .map(|(limit_id, snapshot)| (limit_id, snapshot.into())) - .collect(), - ), - }; - self.outgoing.send_response(request_id, response).await; - } - Err(error) => { - self.outgoing.send_error(request_id, error).await; - } - } + let result = + self.fetch_account_rate_limits() + .await + .map( + |(rate_limits, rate_limits_by_limit_id)| GetAccountRateLimitsResponse { + rate_limits: rate_limits.into(), + rate_limits_by_limit_id: Some( + rate_limits_by_limit_id + .into_iter() + .map(|(limit_id, snapshot)| (limit_id, snapshot.into())) + .collect(), + ), + }, + ); + self.outgoing.send_result(request_id, result).await; } async fn send_add_credits_nudge_email( @@ -1963,16 +1924,11 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: SendAddCreditsNudgeEmailParams, ) { - match self.send_add_credits_nudge_email_inner(params).await { - Ok(status) => { - self.outgoing - .send_response(request_id, SendAddCreditsNudgeEmailResponse { status }) - .await; - } - Err(error) => { - self.outgoing.send_error(request_id, error).await; - } - } + let result = self + .send_add_credits_nudge_email_inner(params) + .await + .map(|status| SendAddCreditsNudgeEmailResponse { status }); + self.outgoing.send_result(request_id, result).await; } async fn send_add_credits_nudge_email_inner( @@ -2100,18 +2056,24 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: CommandExecParams, ) { + let result = self + .exec_one_off_command_inner(request_id.clone(), params) + .await + .map(|()| None::); + self.send_optional_result(request_id, result).await; + } + + async fn exec_one_off_command_inner( + &self, + request_id: ConnectionRequestId, + params: CommandExecParams, + ) -> Result<(), JSONRPCErrorError> { tracing::debug!("ExecOneOffCommand params: {params:?}"); let request = request_id.clone(); if params.command.is_empty() { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: "command must not be empty".to_string(), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(invalid_request("command must not be empty")); } let CommandExecParams { @@ -2131,43 +2093,25 @@ impl CodexMessageProcessor { permission_profile, } = params; if sandbox_policy.is_some() && permission_profile.is_some() { - self.send_invalid_request_error( - request_id, - "`permissionProfile` cannot be combined with `sandboxPolicy`".to_string(), - ) - .await; - return; + return Err(invalid_request( + "`permissionProfile` cannot be combined with `sandboxPolicy`", + )); } if size.is_some() && !tty { - let error = JSONRPCErrorError { - code: INVALID_PARAMS_ERROR_CODE, - message: "command/exec size requires tty: true".to_string(), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(invalid_params("command/exec size requires tty: true")); } if disable_output_cap && output_bytes_cap.is_some() { - let error = JSONRPCErrorError { - code: INVALID_PARAMS_ERROR_CODE, - message: "command/exec cannot set both outputBytesCap and disableOutputCap" - .to_string(), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(invalid_params( + "command/exec cannot set both outputBytesCap and disableOutputCap", + )); } if disable_timeout && timeout_ms.is_some() { - let error = JSONRPCErrorError { - code: INVALID_PARAMS_ERROR_CODE, - message: "command/exec cannot set both timeoutMs and disableTimeout".to_string(), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(invalid_params( + "command/exec cannot set both timeoutMs and disableTimeout", + )); } let cwd = cwd.map_or_else(|| self.config.cwd.clone(), |cwd| self.config.cwd.join(cwd)); @@ -2191,15 +2135,9 @@ impl CodexMessageProcessor { Some(timeout_ms) => match u64::try_from(timeout_ms) { Ok(timeout_ms) => Some(timeout_ms), Err(_) => { - let error = JSONRPCErrorError { - code: INVALID_PARAMS_ERROR_CODE, - message: format!( - "command/exec timeoutMs must be non-negative, got {timeout_ms}" - ), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(invalid_params(format!( + "command/exec timeoutMs must be non-negative, got {timeout_ms}" + ))); } }, None => None, @@ -2219,13 +2157,9 @@ impl CodexMessageProcessor { { Ok(started) => Some(started), Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to start managed network proxy: {err}"), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; + return Err(internal_error(format!( + "failed to start managed network proxy: {err}" + ))); } }, None => None, @@ -2290,68 +2224,33 @@ impl CodexMessageProcessor { &file_system_sandbox_policy, network_sandbox_policy, ); - match self - .config + self.config .permissions .permission_profile .can_set(&effective_permission_profile) - { - Ok(()) => effective_permission_profile, - Err(err) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid permission profile: {err}"), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; - } - } + .map_err(|err| invalid_request(format!("invalid permission profile: {err}")))?; + effective_permission_profile } else if let Some(policy) = sandbox_policy.map(|policy| policy.to_core()) { - match self - .config + self.config .permissions .can_set_legacy_sandbox_policy(&policy, &sandbox_cwd) - { - Ok(()) => { - let file_system_sandbox_policy = - codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, &sandbox_cwd); - let network_sandbox_policy = - codex_protocol::permissions::NetworkSandboxPolicy::from(&policy); - let permission_profile = - codex_protocol::models::PermissionProfile::from_runtime_permissions_with_enforcement( - codex_protocol::models::SandboxEnforcement::from_legacy_sandbox_policy( - &policy, - ), - &file_system_sandbox_policy, - network_sandbox_policy, - ); - if let Err(err) = self - .config - .permissions - .permission_profile - .can_set(&permission_profile) - { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid sandbox policy: {err}"), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; - } - permission_profile - } - Err(err) => { - let error = JSONRPCErrorError { - code: INVALID_REQUEST_ERROR_CODE, - message: format!("invalid sandbox policy: {err}"), - data: None, - }; - self.outgoing.send_error(request, error).await; - return; - } - } + .map_err(|err| invalid_request(format!("invalid sandbox policy: {err}")))?; + let file_system_sandbox_policy = + codex_protocol::permissions::FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, &sandbox_cwd); + let network_sandbox_policy = + codex_protocol::permissions::NetworkSandboxPolicy::from(&policy); + let permission_profile = + codex_protocol::models::PermissionProfile::from_runtime_permissions_with_enforcement( + codex_protocol::models::SandboxEnforcement::from_legacy_sandbox_policy(&policy), + &file_system_sandbox_policy, + network_sandbox_policy, + ); + self.config + .permissions + .permission_profile + .can_set(&permission_profile) + .map_err(|err| invalid_request(format!("invalid sandbox policy: {err}")))?; + permission_profile } else { self.config.permissions.permission_profile() }; @@ -2363,49 +2262,32 @@ impl CodexMessageProcessor { let use_legacy_landlock = self.config.features.use_legacy_landlock(); let size = match size.map(crate::command_exec::terminal_size_from_protocol) { Some(Ok(size)) => Some(size), - Some(Err(error)) => { - self.outgoing.send_error(request, error).await; - return; - } + Some(Err(error)) => return Err(error), None => None, }; - match codex_core::exec::build_exec_request( + let exec_request = codex_core::exec::build_exec_request( exec_params, &effective_permission_profile, &sandbox_cwd, &codex_linux_sandbox_exe, use_legacy_landlock, - ) { - Ok(exec_request) => { - if let Err(error) = self - .command_exec_manager - .start(StartCommandExecParams { - outgoing, - request_id: request_for_task, - process_id, - exec_request, - started_network_proxy: started_network_proxy_for_task, - tty, - stream_stdin, - stream_stdout_stderr, - output_bytes_cap, - size, - }) - .await - { - self.outgoing.send_error(request, error).await; - } - } - Err(err) => { - let error = JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("exec failed: {err}"), - data: None, - }; - self.outgoing.send_error(request, error).await; - } - } + ) + .map_err(|err| internal_error(format!("exec failed: {err}")))?; + self.command_exec_manager + .start(StartCommandExecParams { + outgoing, + request_id: request_for_task, + process_id, + exec_request, + started_network_proxy: started_network_proxy_for_task, + tty, + stream_stdin, + stream_stdout_stderr, + output_bytes_cap, + size, + }) + .await } fn preserve_configured_deny_read_restrictions( @@ -2421,14 +2303,11 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: CommandExecWriteParams, ) { - match self + let result = self .command_exec_manager .write(request_id.clone(), params) - .await - { - Ok(response) => self.outgoing.send_response(request_id, response).await, - Err(error) => self.outgoing.send_error(request_id, error).await, - } + .await; + self.outgoing.send_result(request_id, result).await; } async fn command_exec_resize( @@ -2436,14 +2315,11 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: CommandExecResizeParams, ) { - match self + let result = self .command_exec_manager .resize(request_id.clone(), params) - .await - { - Ok(response) => self.outgoing.send_response(request_id, response).await, - Err(error) => self.outgoing.send_error(request_id, error).await, - } + .await; + self.outgoing.send_result(request_id, result).await; } async fn command_exec_terminate( @@ -2451,14 +2327,11 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: CommandExecTerminateParams, ) { - match self + let result = self .command_exec_manager .terminate(request_id.clone(), params) - .await - { - Ok(response) => self.outgoing.send_response(request_id, response).await, - Err(error) => self.outgoing.send_error(request_id, error).await, - } + .await; + self.outgoing.send_result(request_id, result).await; } async fn thread_start( @@ -6315,6 +6188,22 @@ impl CodexMessageProcessor { }); } + async fn send_optional_result( + &self, + request_id: ConnectionRequestId, + result: Result, JSONRPCErrorError>, + ) where + T: serde::Serialize, + { + match result { + Ok(Some(response)) => self.outgoing.send_response(request_id, response).await, + Ok(None) => {} + Err(error) => { + self.outgoing.send_error(request_id, error).await; + } + } + } + async fn send_invalid_request_error(&self, request_id: ConnectionRequestId, message: String) { let error = JSONRPCErrorError { code: INVALID_REQUEST_ERROR_CODE, From 85c1500569f4d72a06c4aac173ec4b59a85a05b3 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Mon, 27 Apr 2026 12:09:02 -0700 Subject: [PATCH 09/11] fix: filter dynamic deferred tools from model_visible_specs (#19771) fixes #19486 ### Problem Right now dynamic deferred tools are filtered at normal-turn prompt building time, rather than upstream while building the `ToolRouter` itself. This causes issues because dynamic deferred tools are then wrongly included in the router's `model_visible_specs`, which is what the compaction request-building flow relies on. ### Fix Move the dynamic deferred tool filtering to `ToolRouter` creation time to solve this problem for every request that relies on `ToolRouter` for `model_visible_specs`, which solves the issue generically. ### Tests Added unit + integration tests to ensure dynamic deferred tools are omitted from `model_visible_specs` and compaction request respectively. Tested against live `/compact` endpoint; raw deferred dynamic tools without `tool_search` returned `400` (current bug), while the filtered payload (this fix) returns `200`. --- codex-rs/core/src/session/turn.rs | 49 +------ codex-rs/core/src/tools/router.rs | 70 +++++++--- codex-rs/core/src/tools/router_tests.rs | 88 ++++++++++++ codex-rs/core/tests/suite/compact_remote.rs | 143 ++++++++++++++++++++ codex-rs/core/tests/suite/search_tool.rs | 12 +- 5 files changed, 293 insertions(+), 69 deletions(-) diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 6383b892ec..ff3d37912d 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -95,9 +95,7 @@ use codex_protocol::protocol::ReasoningRawContentDeltaEvent; use codex_protocol::protocol::TurnDiffEvent; use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; -use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; -use codex_tools::ToolSpec; use codex_tools::filter_tool_suggest_discoverable_tools_for_client; use codex_utils_stream_parser::AssistantTextChunk; use codex_utils_stream_parser::AssistantTextStreamParser; @@ -946,25 +944,9 @@ pub(crate) fn build_prompt( turn_context: &TurnContext, base_instructions: BaseInstructions, ) -> Prompt { - let deferred_dynamic_tools = turn_context - .dynamic_tools - .iter() - .filter(|tool| tool.defer_loading) - .map(|tool| ToolName::new(tool.namespace.clone(), tool.name.clone())) - .collect::>(); - let tools = if deferred_dynamic_tools.is_empty() { - router.model_visible_specs() - } else { - router - .model_visible_specs() - .into_iter() - .filter_map(|spec| filter_deferred_dynamic_tool_spec(spec, &deferred_dynamic_tools)) - .collect() - }; - Prompt { input, - tools, + tools: router.model_visible_specs(), parallel_tool_calls: turn_context.model_info.supports_parallel_tool_calls, base_instructions, personality: turn_context.personality, @@ -975,35 +957,6 @@ pub(crate) fn build_prompt( } } -fn filter_deferred_dynamic_tool_spec( - spec: ToolSpec, - deferred_dynamic_tools: &HashSet, -) -> Option { - match spec { - ToolSpec::Function(tool) => { - if deferred_dynamic_tools.contains(&ToolName::plain(tool.name.as_str())) { - None - } else { - Some(ToolSpec::Function(tool)) - } - } - ToolSpec::Namespace(mut namespace) => { - let namespace_name = namespace.name.clone(); - namespace.tools.retain(|tool| match tool { - ResponsesApiNamespaceTool::Function(tool) => !deferred_dynamic_tools.contains( - &ToolName::namespaced(namespace_name.as_str(), tool.name.as_str()), - ), - }); - if namespace.tools.is_empty() { - None - } else { - Some(ToolSpec::Namespace(namespace)) - } - } - spec => Some(spec), - } -} - #[allow(clippy::too_many_arguments)] #[instrument(level = "trace", skip_all, diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 1f0dec6925..aeba3b0556 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -71,23 +71,26 @@ impl ToolRouter { dynamic_tools, ); let (specs, registry) = builder.build(); - let model_visible_specs = if config.code_mode_only_enabled { - specs - .iter() - .filter_map(|configured_tool| { - if !codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) { - Some(configured_tool.spec.clone()) - } else { - None - } - }) - .collect() - } else { - specs - .iter() - .map(|configured_tool| configured_tool.spec.clone()) - .collect() - }; + let deferred_dynamic_tools = dynamic_tools + .iter() + .filter(|tool| tool.defer_loading) + .map(|tool| ToolName::new(tool.namespace.clone(), tool.name.clone())) + .collect::>(); + let model_visible_specs = specs + .iter() + .filter_map(|configured_tool| { + if config.code_mode_only_enabled + && codex_code_mode::is_code_mode_nested_tool(configured_tool.name()) + { + return None; + } + + filter_deferred_dynamic_tool_spec( + configured_tool.spec.clone(), + &deferred_dynamic_tools, + ) + }) + .collect(); Self { registry, @@ -293,6 +296,39 @@ impl ToolRouter { self.registry.dispatch_any(invocation).await } } + +fn filter_deferred_dynamic_tool_spec( + spec: ToolSpec, + deferred_dynamic_tools: &HashSet, +) -> Option { + if deferred_dynamic_tools.is_empty() { + return Some(spec); + } + + match spec { + ToolSpec::Function(tool) => { + if deferred_dynamic_tools.contains(&ToolName::plain(tool.name.as_str())) { + None + } else { + Some(ToolSpec::Function(tool)) + } + } + ToolSpec::Namespace(mut namespace) => { + let namespace_name = namespace.name.clone(); + namespace.tools.retain(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => !deferred_dynamic_tools.contains( + &ToolName::namespaced(namespace_name.as_str(), tool.name.as_str()), + ), + }); + if namespace.tools.is_empty() { + None + } else { + Some(ToolSpec::Namespace(namespace)) + } + } + spec => Some(spec), + } +} #[cfg(test)] #[path = "router_tests.rs"] mod tests; diff --git a/codex-rs/core/src/tools/router_tests.rs b/codex-rs/core/src/tools/router_tests.rs index e8c098c41e..ac859d9aa6 100644 --- a/codex-rs/core/src/tools/router_tests.rs +++ b/codex-rs/core/src/tools/router_tests.rs @@ -3,8 +3,13 @@ use std::sync::Arc; use crate::session::tests::make_session_and_context; use crate::tools::context::ToolPayload; +use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::models::ResponseItem; +use codex_tools::ResponsesApiNamespaceTool; use codex_tools::ToolName; +use codex_tools::ToolSpec; +use pretty_assertions::assert_eq; +use serde_json::json; use super::ToolCall; use super::ToolRouter; @@ -133,3 +138,86 @@ async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> Ok(()) } + +#[tokio::test] +async fn model_visible_specs_filter_deferred_dynamic_tools() -> anyhow::Result<()> { + let (_, turn) = make_session_and_context().await; + let hidden_tool = "hidden_dynamic_tool"; + let visible_tool = "visible_dynamic_tool"; + let dynamic_tools = vec![ + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: hidden_tool.to_string(), + description: "Hidden until discovered.".to_string(), + input_schema: json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }), + defer_loading: true, + }, + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: visible_tool.to_string(), + description: "Visible immediately.".to_string(), + input_schema: json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }), + defer_loading: false, + }, + ]; + + let router = ToolRouter::from_config( + &turn.tools_config, + ToolRouterParams { + deferred_mcp_tools: None, + mcp_tools: None, + unavailable_called_tools: Vec::new(), + parallel_mcp_server_names: HashSet::new(), + discoverable_tools: None, + dynamic_tools: &dynamic_tools, + }, + ); + + assert!( + router + .find_spec(&ToolName::namespaced("codex_app", hidden_tool)) + .is_some() + ); + assert_eq!( + namespace_function_names(&router.specs(), "codex_app"), + vec![hidden_tool.to_string(), visible_tool.to_string()] + ); + assert_eq!( + namespace_function_names(&router.model_visible_specs(), "codex_app"), + vec![visible_tool.to_string()] + ); + + Ok(()) +} + +fn namespace_function_names(specs: &[ToolSpec], namespace_name: &str) -> Vec { + specs + .iter() + .find_map(|spec| match spec { + ToolSpec::Namespace(namespace) if namespace.name == namespace_name => Some( + namespace + .tools + .iter() + .map(|tool| match tool { + ResponsesApiNamespaceTool::Function(tool) => tool.name.clone(), + }) + .collect(), + ), + ToolSpec::Function(_) + | ToolSpec::Freeform(_) + | ToolSpec::ToolSearch { .. } + | ToolSpec::LocalShell {} + | ToolSpec::ImageGeneration { .. } + | ToolSpec::WebSearch { .. } + | ToolSpec::Namespace(_) => None, + }) + .unwrap_or_default() +} diff --git a/codex-rs/core/tests/suite/compact_remote.rs b/codex-rs/core/tests/suite/compact_remote.rs index b70cc6f38e..24e86881b9 100644 --- a/codex-rs/core/tests/suite/compact_remote.rs +++ b/codex-rs/core/tests/suite/compact_remote.rs @@ -6,6 +6,7 @@ use std::path::PathBuf; use anyhow::Result; use codex_core::compact::SUMMARY_PREFIX; use codex_login::CodexAuth; +use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::items::TurnItem; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; @@ -36,6 +37,7 @@ use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; use pretty_assertions::assert_eq; +use serde_json::Value; use serde_json::json; use tokio::time::Duration; use wiremock::ResponseTemplate; @@ -55,6 +57,53 @@ fn estimate_compact_payload_tokens(request: &responses::ResponsesRequest) -> i64 .saturating_add(approx_token_count(&request.instructions_text())) } +fn assert_tools_payload_does_not_defer(body: &Value) { + if let Some(tools) = body.get("tools") { + assert!( + !contains_defer_loading(tools), + "model-visible tools should not include deferred declarations: {tools}" + ); + } +} + +fn namespace_child_tool_names(body: &Value, namespace: &str) -> Vec { + body.get("tools") + .and_then(Value::as_array) + .and_then(|tools| { + tools.iter().find_map(|tool| { + if tool.get("type").and_then(Value::as_str) == Some("namespace") + && tool.get("name").and_then(Value::as_str) == Some(namespace) + { + tool.get("tools").and_then(Value::as_array).map(|children| { + children + .iter() + .filter_map(|child| { + child + .get("name") + .and_then(Value::as_str) + .map(str::to_string) + }) + .collect() + }) + } else { + None + } + }) + }) + .unwrap_or_default() +} + +fn contains_defer_loading(value: &Value) -> bool { + match value { + Value::Object(map) => { + map.get("defer_loading").and_then(Value::as_bool) == Some(true) + || map.values().any(contains_defer_loading) + } + Value::Array(values) => values.iter().any(contains_defer_loading), + Value::Null | Value::Bool(_) | Value::Number(_) | Value::String(_) => false, + } +} + const PRETURN_CONTEXT_DIFF_CWD: &str = "/tmp/PRETURN_CONTEXT_DIFF_CWD"; const DUMMY_FUNCTION_NAME: &str = "test_tool"; const REMOTE_COMPACT_TURN_COMPLETE_TIMEOUT: Duration = Duration::from_secs(30); @@ -358,6 +407,100 @@ async fn remote_compact_replaces_history_for_followups() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_compact_filters_deferred_dynamic_tools() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let mut builder = test_codex().with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()); + let mut test = builder.build(&server).await?; + let hidden_tool = "hidden_dynamic_tool"; + let visible_tool = "visible_dynamic_tool"; + let input_schema = json!({ + "type": "object", + "properties": {}, + "additionalProperties": false, + }); + let dynamic_tools = vec![ + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: hidden_tool.to_string(), + description: "Hidden until discovered.".to_string(), + input_schema: input_schema.clone(), + defer_loading: true, + }, + DynamicToolSpec { + namespace: Some("codex_app".to_string()), + name: visible_tool.to_string(), + description: "Visible immediately.".to_string(), + input_schema, + defer_loading: false, + }, + ]; + let new_thread = test + .thread_manager + .start_thread_with_tools( + test.config.clone(), + dynamic_tools, + /*persist_extended_history*/ false, + ) + .await?; + test.codex = new_thread.thread; + test.session_configured = new_thread.session_configured; + let codex = test.codex.clone(); + + let responses_mock = mount_sse_once( + &server, + sse(vec![ + responses::ev_assistant_message("m1", "FIRST_REMOTE_REPLY"), + responses::ev_completed("resp-1"), + ]), + ) + .await; + let compact_mock = responses::mount_compact_json_once( + &server, + serde_json::json!({ + "output": compacted_summary_only_output("compact summary"), + }), + ) + .await; + + codex + .submit(Op::UserInput { + environments: None, + items: vec![UserInput::Text { + text: "hello remote compact".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + }) + .await?; + wait_for_turn_complete(&codex).await; + + codex.submit(Op::Compact).await?; + wait_for_turn_complete(&codex).await; + + let first_response_body = responses_mock.single_request().body_json(); + let compact_body = compact_mock.single_request().body_json(); + assert_eq!( + compact_body["tools"], first_response_body["tools"], + "compact requests should send the same model-visible tools payload as /v1/responses" + ); + assert_tools_payload_does_not_defer(&first_response_body); + assert_tools_payload_does_not_defer(&compact_body); + assert_eq!( + namespace_child_tool_names(&first_response_body, "codex_app"), + vec![visible_tool.to_string()] + ); + assert_eq!( + namespace_child_tool_names(&compact_body, "codex_app"), + vec![visible_tool.to_string()] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn remote_compact_runs_automatically() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index fa2eca9b22..1794f2fd3f 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -605,7 +605,8 @@ async fn tool_search_returns_deferred_tools_without_follow_up_tool_injection() - "apps tools/call should include turn metadata turn_id: {apps_tool_call:?}" ); - let first_request_tools = tool_names(&requests[0].body_json()); + let first_request_body = requests[0].body_json(); + let first_request_tools = tool_names(&first_request_body); assert!( first_request_tools .iter() @@ -823,7 +824,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - let requests = mock.requests(); assert_eq!(requests.len(), 3); - let first_request_tools = tool_names(&requests[0].body_json()); + let first_request_body = requests[0].body_json(); + let first_request_tools = tool_names(&first_request_body); assert!( first_request_tools .iter() @@ -853,7 +855,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - })] ); - let second_request_tools = tool_names(&requests[1].body_json()); + let second_request_body = requests[1].body_json(); + let second_request_tools = tool_names(&second_request_body); assert!( !second_request_tools.iter().any(|name| name == tool_name), "follow-up request should rely on tool_search_output history, not tool injection: {second_request_tools:?}" @@ -870,7 +873,8 @@ async fn tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call() - FunctionCallOutputPayload::from_text("dynamic-search-ok".to_string()) ); - let third_request_tools = tool_names(&requests[2].body_json()); + let third_request_body = requests[2].body_json(); + let third_request_tools = tool_names(&third_request_body); assert!( !third_request_tools.iter().any(|name| name == tool_name), "post-tool follow-up should rely on tool_search_output history, not tool injection: {third_request_tools:?}" From 215d5a8f7c37ceb8f6c5ddf16390e907ce70f056 Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Mon, 27 Apr 2026 15:29:19 -0400 Subject: [PATCH 10/11] [codex-analytics] remove ga flag (#19863) --- .../app-server/src/codex_message_processor.rs | 107 +++++++----------- codex-rs/app-server/src/message_processor.rs | 19 ++-- .../app-server/tests/suite/v2/analytics.rs | 18 --- .../app-server/tests/suite/v2/thread_fork.rs | 21 +--- .../tests/suite/v2/thread_resume.rs | 41 ++----- .../app-server/tests/suite/v2/thread_start.rs | 66 +---------- .../app-server/tests/suite/v2/turn_start.rs | 74 +----------- .../app-server/tests/suite/v2/turn_steer.rs | 8 +- codex-rs/core/config.schema.json | 6 - codex-rs/core/src/agent/control.rs | 1 - codex-rs/core/src/codex_delegate.rs | 22 ++-- codex-rs/core/src/compact.rs | 7 -- codex-rs/core/src/guardian/review.rs | 10 -- codex-rs/core/src/memories/phase2.rs | 20 ++-- codex-rs/core/src/session/turn.rs | 4 - codex-rs/features/src/lib.rs | 8 -- codex-rs/features/src/tests.rs | 6 - 17 files changed, 87 insertions(+), 351 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index d982fd5331..6edd11c691 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -540,7 +540,6 @@ struct ListenerTaskContext { outgoing: Arc, pending_thread_unloads: Arc>>, analytics_events_client: AnalyticsEventsClient, - general_analytics_enabled: bool, thread_watch_manager: ThreadWatchManager, fallback_model_provider: String, codex_home: PathBuf, @@ -727,14 +726,12 @@ impl CodexMessageProcessor { error: &JSONRPCErrorError, error_type: Option, ) { - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_error_response( - request_id.connection_id.0, - request_id.request_id.clone(), - error.clone(), - error_type, - ); - } + self.analytics_events_client.track_error_response( + request_id.connection_id.0, + request_id.request_id.clone(), + error.clone(), + error_type, + ); } async fn load_thread( @@ -2410,7 +2407,6 @@ impl CodexMessageProcessor { outgoing: Arc::clone(&self.outgoing), pending_thread_unloads: Arc::clone(&self.pending_thread_unloads), analytics_events_client: self.analytics_events_client.clone(), - general_analytics_enabled: self.config.features.enabled(Feature::GeneralAnalytics), thread_watch_manager: self.thread_watch_manager.clone(), fallback_model_provider: self.config.model_provider_id.clone(), codex_home: self.config.codex_home.to_path_buf(), @@ -2749,17 +2745,15 @@ impl CodexMessageProcessor { permission_profile, reasoning_effort: config_snapshot.reasoning_effort, }; - if listener_task_context.general_analytics_enabled { - listener_task_context - .analytics_events_client - .track_response( - request_id.connection_id.0, - ClientResponse::ThreadStart { - request_id: request_id.request_id.clone(), - response: response.clone(), - }, - ); - } + listener_task_context + .analytics_events_client + .track_response( + request_id.connection_id.0, + ClientResponse::ThreadStart { + request_id: request_id.request_id.clone(), + response: response.clone(), + }, + ); listener_task_context .outgoing @@ -4565,15 +4559,13 @@ impl CodexMessageProcessor { permission_profile, reasoning_effort: session_configured.reasoning_effort, }; - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_response( - request_id.connection_id.0, - ClientResponse::ThreadResume { - request_id: request_id.request_id.clone(), - response: response.clone(), - }, - ); - } + self.analytics_events_client.track_response( + request_id.connection_id.0, + ClientResponse::ThreadResume { + request_id: request_id.request_id.clone(), + response: response.clone(), + }, + ); let connection_id = request_id.connection_id; let token_usage_thread = include_turns.then(|| response.thread.clone()); @@ -5296,15 +5288,13 @@ impl CodexMessageProcessor { permission_profile, reasoning_effort: session_configured.reasoning_effort, }; - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_response( - request_id.connection_id.0, - ClientResponse::ThreadFork { - request_id: request_id.request_id.clone(), - response: response.clone(), - }, - ); - } + self.analytics_events_client.track_response( + request_id.connection_id.0, + ClientResponse::ThreadFork { + request_id: request_id.request_id.clone(), + response: response.clone(), + }, + ); let connection_id = request_id.connection_id; let token_usage_thread = include_turns.then(|| response.thread.clone()); @@ -7038,15 +7028,13 @@ impl CodexMessageProcessor { }; let response = TurnStartResponse { turn }; - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_response( - request_id.connection_id.0, - ClientResponse::TurnStart { - request_id: request_id.request_id.clone(), - response: response.clone(), - }, - ); - } + self.analytics_events_client.track_response( + request_id.connection_id.0, + ClientResponse::TurnStart { + request_id: request_id.request_id.clone(), + response: response.clone(), + }, + ); self.outgoing.send_response(request_id, response).await; } Err(err) => { @@ -7172,15 +7160,13 @@ impl CodexMessageProcessor { { Ok(turn_id) => { let response = TurnSteerResponse { turn_id }; - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_response( - request_id.connection_id.0, - ClientResponse::TurnSteer { - request_id: request_id.request_id.clone(), - response: response.clone(), - }, - ); - } + self.analytics_events_client.track_response( + request_id.connection_id.0, + ClientResponse::TurnSteer { + request_id: request_id.request_id.clone(), + response: response.clone(), + }, + ); self.outgoing.send_response(request_id, response).await; } Err(err) => { @@ -7813,7 +7799,6 @@ impl CodexMessageProcessor { outgoing: Arc::clone(&self.outgoing), pending_thread_unloads: Arc::clone(&self.pending_thread_unloads), analytics_events_client: self.analytics_events_client.clone(), - general_analytics_enabled: self.config.features.enabled(Feature::GeneralAnalytics), thread_watch_manager: self.thread_watch_manager.clone(), fallback_model_provider: self.config.model_provider_id.clone(), codex_home: self.config.codex_home.to_path_buf(), @@ -7931,7 +7916,6 @@ impl CodexMessageProcessor { outgoing: Arc::clone(&self.outgoing), pending_thread_unloads: Arc::clone(&self.pending_thread_unloads), analytics_events_client: self.analytics_events_client.clone(), - general_analytics_enabled: self.config.features.enabled(Feature::GeneralAnalytics), thread_watch_manager: self.thread_watch_manager.clone(), fallback_model_provider: self.config.model_provider_id.clone(), codex_home: self.config.codex_home.to_path_buf(), @@ -7980,7 +7964,6 @@ impl CodexMessageProcessor { thread_state_manager, pending_thread_unloads, analytics_events_client: _, - general_analytics_enabled: _, thread_watch_manager, fallback_model_provider, codex_home, @@ -8056,9 +8039,7 @@ impl CodexMessageProcessor { conversation_id, conversation.clone(), thread_manager.clone(), - listener_task_context - .general_analytics_enabled - .then(|| listener_task_context.analytics_events_client.clone()), + Some(listener_task_context.analytics_events_client.clone()), thread_outgoing, thread_state.clone(), thread_watch_manager.clone(), diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 6cdb939364..071f69eb6a 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -653,14 +653,12 @@ impl MessageProcessor { } } } - if self.config.features.enabled(Feature::GeneralAnalytics) { - self.analytics_events_client.track_initialize( - connection_id.0, - analytics_initialize_params, - originator, - self.rpc_transport, - ); - } + self.analytics_events_client.track_initialize( + connection_id.0, + analytics_initialize_params, + originator, + self.rpc_transport, + ); set_default_client_residency_requirement(self.config.enforce_residency.value()); if let Ok(mut suffix) = USER_AGENT_SUFFIX.lock() { *suffix = Some(user_agent_suffix); @@ -716,9 +714,8 @@ impl MessageProcessor { return Err(invalid_request(experimental_required_message(reason))); } let connection_id = connection_request_id.connection_id; - if self.config.features.enabled(Feature::GeneralAnalytics) - && let ClientRequest::TurnStart { request_id, .. } - | ClientRequest::TurnSteer { request_id, .. } = &codex_request + if let ClientRequest::TurnStart { request_id, .. } + | ClientRequest::TurnSteer { request_id, .. } = &codex_request { self.analytics_events_client.track_request( connection_id.0, diff --git a/codex-rs/app-server/tests/suite/v2/analytics.rs b/codex-rs/app-server/tests/suite/v2/analytics.rs index a3ecdbc1f4..862721a154 100644 --- a/codex-rs/app-server/tests/suite/v2/analytics.rs +++ b/codex-rs/app-server/tests/suite/v2/analytics.rs @@ -79,24 +79,6 @@ async fn app_server_default_analytics_enabled_with_flag() -> Result<()> { Ok(()) } -pub(crate) async fn enable_analytics_capture(server: &MockServer, codex_home: &Path) -> Result<()> { - let config_path = codex_home.join("config.toml"); - let config_toml = std::fs::read_to_string(&config_path)?; - if !config_toml.contains("[features]") { - std::fs::write( - &config_path, - format!("{config_toml}\n[features]\ngeneral_analytics = true\n"), - )?; - } else if !config_toml.contains("general_analytics") { - std::fs::write( - &config_path, - config_toml.replace("[features]\n", "[features]\ngeneral_analytics = true\n"), - )?; - } - - mount_analytics_capture(server, codex_home).await -} - pub(crate) async fn mount_analytics_capture(server: &MockServer, codex_home: &Path) -> Result<()> { Mock::given(method("POST")) .and(path("/codex/analytics-events/events")) diff --git a/codex-rs/app-server/tests/suite/v2/thread_fork.rs b/codex-rs/app-server/tests/suite/v2/thread_fork.rs index 6c43ebd626..fd773f2e30 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_fork.rs @@ -42,7 +42,7 @@ use wiremock::matchers::method; use wiremock::matchers::path; use super::analytics::assert_basic_thread_initialized_event; -use super::analytics::enable_analytics_capture; +use super::analytics::mount_analytics_capture; use super::analytics::thread_initialized_event; use super::analytics::wait_for_analytics_payload; @@ -385,13 +385,8 @@ async fn thread_fork_tracks_thread_initialized_analytics() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml_with_chatgpt_base_url( - codex_home.path(), - &server.uri(), - &server.uri(), - /*general_analytics_enabled*/ true, - )?; - enable_analytics_capture(&server, codex_home.path()).await?; + create_config_toml_with_chatgpt_base_url(codex_home.path(), &server.uri(), &server.uri())?; + mount_analytics_capture(&server, codex_home.path()).await?; let conversation_id = create_fake_rollout( codex_home.path(), @@ -496,7 +491,6 @@ async fn thread_fork_surfaces_cloud_requirements_load_errors() -> Result<()> { codex_home.path(), &model_server.uri(), &chatgpt_base_url, - /*general_analytics_enabled*/ false, )?; write_chatgpt_auth( codex_home.path(), @@ -793,13 +787,7 @@ fn create_config_toml_with_chatgpt_base_url( codex_home: &Path, server_uri: &str, chatgpt_base_url: &str, - general_analytics_enabled: bool, ) -> std::io::Result<()> { - let general_analytics_toml = if general_analytics_enabled { - "\ngeneral_analytics = true".to_string() - } else { - "\ngeneral_analytics = false".to_string() - }; let config_toml = codex_home.join("config.toml"); std::fs::write( config_toml, @@ -812,9 +800,6 @@ chatgpt_base_url = "{chatgpt_base_url}" model_provider = "mock_provider" -[features] -{general_analytics_toml} - [model_providers.mock_provider] name = "Mock provider for test" base_url = "{server_uri}/v1" diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index 5044fcd11a..d9f5f039de 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -84,7 +84,7 @@ use wiremock::matchers::method; use wiremock::matchers::path; use super::analytics::assert_basic_thread_initialized_event; -use super::analytics::enable_analytics_capture; +use super::analytics::mount_analytics_capture; use super::analytics::thread_initialized_event; use super::analytics::wait_for_analytics_payload; @@ -185,10 +185,7 @@ async fn thread_goal_get_rejects_unmaterialized_thread() -> Result<()> { let config = std::fs::read_to_string(&config_path)?; std::fs::write( &config_path, - config.replace( - "general_analytics = true\n", - "general_analytics = true\ngoals = true\n", - ), + config.replace("personality = true\n", "personality = true\ngoals = true\n"), )?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; @@ -238,13 +235,8 @@ async fn thread_resume_tracks_thread_initialized_analytics() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml_with_chatgpt_base_url( - codex_home.path(), - &server.uri(), - &server.uri(), - /*general_analytics_enabled*/ true, - )?; - enable_analytics_capture(&server, codex_home.path()).await?; + create_config_toml_with_chatgpt_base_url(codex_home.path(), &server.uri(), &server.uri())?; + mount_analytics_capture(&server, codex_home.path()).await?; let conversation_id = create_fake_rollout_with_text_elements( codex_home.path(), @@ -400,10 +392,7 @@ async fn thread_resume_emits_active_goal_update_before_continuation() -> Result< let config = std::fs::read_to_string(&config_path)?; std::fs::write( &config_path, - config.replace( - "general_analytics = true\n", - "general_analytics = true\ngoals = true\n", - ), + config.replace("personality = true\n", "personality = true\ngoals = true\n"), )?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; @@ -507,10 +496,7 @@ async fn thread_goal_set_preserves_budget_limited_same_objective() -> Result<()> let config = std::fs::read_to_string(&config_path)?; std::fs::write( &config_path, - config.replace( - "general_analytics = true\n", - "general_analytics = true\ngoals = true\n", - ), + config.replace("personality = true\n", "personality = true\ngoals = true\n"), )?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; @@ -608,10 +594,7 @@ async fn thread_goal_clear_deletes_goal_and_notifies() -> Result<()> { let config = std::fs::read_to_string(&config_path)?; std::fs::write( &config_path, - config.replace( - "general_analytics = true\n", - "general_analytics = true\ngoals = true\n", - ), + config.replace("personality = true\n", "personality = true\ngoals = true\n"), )?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; @@ -2414,7 +2397,6 @@ async fn thread_resume_surfaces_cloud_requirements_load_errors() -> Result<()> { codex_home.path(), &model_server.uri(), &chatgpt_base_url, - /*general_analytics_enabled*/ false, )?; write_chatgpt_auth( codex_home.path(), @@ -2859,7 +2841,6 @@ model_provider = "mock_provider" [features] personality = true -general_analytics = true [model_providers.mock_provider] name = "Mock provider for test" @@ -2890,7 +2871,6 @@ model_provider = "mock_provider" [features] personality = true -general_analytics = true [model_providers.mock_provider] name = "Mock provider for test" @@ -2907,13 +2887,7 @@ fn create_config_toml_with_chatgpt_base_url( codex_home: &std::path::Path, server_uri: &str, chatgpt_base_url: &str, - general_analytics_enabled: bool, ) -> std::io::Result<()> { - let general_analytics_toml = if general_analytics_enabled { - "\ngeneral_analytics = true".to_string() - } else { - "\ngeneral_analytics = false".to_string() - }; let config_toml = codex_home.join("config.toml"); std::fs::write( config_toml, @@ -2928,7 +2902,6 @@ model_provider = "mock_provider" [features] personality = true -{general_analytics_toml} [model_providers.mock_provider] name = "Mock provider for test" diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index f521d5509c..d8a50b88a4 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -34,7 +34,6 @@ use serde_json::Value; use serde_json::json; use std::path::Path; use std::path::PathBuf; -use std::time::Duration; use tempfile::TempDir; use tokio::time::timeout; use wiremock::Mock; @@ -265,12 +264,7 @@ async fn thread_start_tracks_thread_initialized_analytics() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; let codex_home = TempDir::new()?; - create_config_toml_with_chatgpt_base_url( - codex_home.path(), - &server.uri(), - &server.uri(), - /*general_analytics_enabled*/ true, - )?; + create_config_toml_with_chatgpt_base_url(codex_home.path(), &server.uri(), &server.uri())?; mount_analytics_capture(&server, codex_home.path()).await?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; @@ -293,54 +287,6 @@ async fn thread_start_tracks_thread_initialized_analytics() -> Result<()> { Ok(()) } -#[tokio::test] -async fn thread_start_does_not_track_thread_initialized_analytics_without_feature() -> Result<()> { - let server = create_mock_responses_server_repeating_assistant("Done").await; - - let codex_home = TempDir::new()?; - create_config_toml_with_chatgpt_base_url( - codex_home.path(), - &server.uri(), - &server.uri(), - /*general_analytics_enabled*/ false, - )?; - mount_analytics_capture(&server, codex_home.path()).await?; - - let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; - timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - - let req_id = mcp - .send_thread_start_request(ThreadStartParams::default()) - .await?; - let resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(req_id)), - ) - .await??; - let _ = to_response::(resp)?; - - assert_no_thread_initialized_analytics(&server, Duration::from_millis(250)).await?; - Ok(()) -} - -async fn assert_no_thread_initialized_analytics( - server: &MockServer, - wait_duration: Duration, -) -> Result<()> { - tokio::time::sleep(wait_duration).await; - let requests = server.received_requests().await.unwrap_or_default(); - for request in requests.iter().filter(|request| { - request.method == "POST" && request.url.path() == "/codex/analytics-events/events" - }) { - let payload: Value = serde_json::from_slice(&request.body)?; - assert!( - thread_initialized_event(&payload).is_err(), - "thread analytics should be gated off when general_analytics is disabled; payload={payload}" - ); - } - Ok(()) -} - #[tokio::test] async fn thread_start_respects_project_config_from_cwd() -> Result<()> { let server = create_mock_responses_server_repeating_assistant("Done").await; @@ -643,7 +589,6 @@ async fn thread_start_surfaces_cloud_requirements_load_errors() -> Result<()> { codex_home.path(), &model_server.uri(), &chatgpt_base_url, - /*general_analytics_enabled*/ false, )?; write_chatgpt_auth( codex_home.path(), @@ -966,13 +911,7 @@ fn create_config_toml_with_chatgpt_base_url( codex_home: &Path, server_uri: &str, chatgpt_base_url: &str, - general_analytics_enabled: bool, ) -> std::io::Result<()> { - let general_analytics_toml = if general_analytics_enabled { - "\ngeneral_analytics = true".to_string() - } else { - "\ngeneral_analytics = false".to_string() - }; let config_toml = codex_home.join("config.toml"); std::fs::write( config_toml, @@ -985,9 +924,6 @@ chatgpt_base_url = "{chatgpt_base_url}" model_provider = "mock_provider" -[features] -{general_analytics_toml} - [model_providers.mock_provider] name = "Mock provider for test" base_url = "{server_uri}/v1" diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 6d66edd3d3..f81ec5f168 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -78,7 +78,6 @@ use std::path::Path; use tempfile::TempDir; use tokio::time::timeout; -use super::analytics::enable_analytics_capture; use super::analytics::mount_analytics_capture; use super::analytics::wait_for_analytics_event; @@ -463,7 +462,7 @@ async fn turn_start_tracks_turn_event_analytics() -> Result<()> { &server.uri(), &server.uri(), )?; - enable_analytics_capture(&server, codex_home.path()).await?; + mount_analytics_capture(&server, codex_home.path()).await?; let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -538,77 +537,6 @@ async fn turn_start_tracks_turn_event_analytics() -> Result<()> { Ok(()) } -#[tokio::test] -async fn turn_start_does_not_track_turn_event_analytics_without_feature() -> Result<()> { - let responses = vec![create_final_assistant_message_sse_response("Done")?]; - let server = create_mock_responses_server_sequence_unchecked(responses).await; - - let codex_home = TempDir::new()?; - write_mock_responses_config_toml_with_chatgpt_base_url( - codex_home.path(), - &server.uri(), - &server.uri(), - )?; - let config_path = codex_home.path().join("config.toml"); - let config_toml = std::fs::read_to_string(&config_path)?; - std::fs::write( - &config_path, - format!("{config_toml}\n[features]\ngeneral_analytics = false\n"), - )?; - mount_analytics_capture(&server, codex_home.path()).await?; - - let mut mcp = McpProcess::new_without_managed_config(codex_home.path()).await?; - timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - - let thread_req = mcp - .send_thread_start_request(ThreadStartParams { - model: Some("mock-model".to_string()), - ..Default::default() - }) - .await?; - let thread_resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), - ) - .await??; - let ThreadStartResponse { thread, .. } = to_response::(thread_resp)?; - - let turn_req = mcp - .send_turn_start_request(TurnStartParams { - thread_id: thread.id, - input: vec![V2UserInput::Text { - text: "hello".to_string(), - text_elements: Vec::new(), - }], - ..Default::default() - }) - .await?; - let turn_resp: JSONRPCResponse = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), - ) - .await??; - let _ = to_response::(turn_resp)?; - - timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_notification_message("turn/completed"), - ) - .await??; - - let turn_event = wait_for_analytics_event( - &server, - std::time::Duration::from_millis(250), - "codex_turn_event", - ) - .await; - assert!( - turn_event.is_err(), - "turn analytics should be gated off when general_analytics is disabled" - ); - Ok(()) -} - #[tokio::test] async fn turn_start_accepts_text_at_limit_with_mention_item() -> Result<()> { let responses = vec![create_final_assistant_message_sse_response("Done")?]; diff --git a/codex-rs/app-server/tests/suite/v2/turn_steer.rs b/codex-rs/app-server/tests/suite/v2/turn_steer.rs index 16e28d6cc5..a92b2db528 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_steer.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_steer.rs @@ -24,7 +24,7 @@ use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS; use tempfile::TempDir; use tokio::time::timeout; -use super::analytics::enable_analytics_capture; +use super::analytics::mount_analytics_capture; use super::analytics::wait_for_analytics_event; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); @@ -41,7 +41,7 @@ async fn turn_steer_requires_active_turn() -> Result<()> { &server.uri(), &server.uri(), )?; - enable_analytics_capture(&server, &codex_home).await?; + mount_analytics_capture(&server, &codex_home).await?; let mut mcp = McpProcess::new_without_managed_config(&codex_home).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -125,7 +125,7 @@ async fn turn_steer_rejects_oversized_text_input() -> Result<()> { &server.uri(), &server.uri(), )?; - enable_analytics_capture(&server, &codex_home).await?; + mount_analytics_capture(&server, &codex_home).await?; let mut mcp = McpProcess::new_without_managed_config(&codex_home).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -234,7 +234,7 @@ async fn turn_steer_returns_active_turn_id() -> Result<()> { &server.uri(), &server.uri(), )?; - enable_analytics_capture(&server, &codex_home).await?; + mount_analytics_capture(&server, &codex_home).await?; let mut mcp = McpProcess::new_without_managed_config(&codex_home).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 5727a4bdcf..e4d156c540 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -421,9 +421,6 @@ "fast_mode": { "type": "boolean" }, - "general_analytics": { - "type": "boolean" - }, "goals": { "type": "boolean" }, @@ -2631,9 +2628,6 @@ "fast_mode": { "type": "boolean" }, - "general_analytics": { - "type": "boolean" - }, "goals": { "type": "boolean" }, diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index d4ec6858d1..daf1acf8f3 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -262,7 +262,6 @@ impl AgentControl { parent_thread_id, .. }, )) = notification_source.as_ref() - && new_thread.thread.enabled(Feature::GeneralAnalytics) { let client_metadata = match state.get_thread(*parent_thread_id).await { Ok(parent_thread) => { diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 1fb2f42f2e..f3df3cd4c6 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -104,18 +104,16 @@ pub(crate) async fn run_codex_thread_interactive( })) .or_cancel(&cancel_token) .await??; - if parent_session.enabled(codex_features::Feature::GeneralAnalytics) { - let thread_config = codex.thread_config_snapshot().await; - let client_metadata = parent_session.app_server_client_metadata().await; - emit_subagent_session_started( - &parent_session.services.analytics_events_client, - client_metadata, - codex.session.conversation_id, - Some(parent_session.conversation_id), - thread_config, - subagent_source, - ); - } + let thread_config = codex.thread_config_snapshot().await; + let client_metadata = parent_session.app_server_client_metadata().await; + emit_subagent_session_started( + &parent_session.services.analytics_events_client, + client_metadata, + codex.session.conversation_id, + Some(parent_session.conversation_id), + thread_config, + subagent_source, + ); let codex = Arc::new(codex); // Use a child token so parent cancel cascades but we can scope it to this task diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index e9218ae7f0..ed7a95b96e 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -18,7 +18,6 @@ use codex_analytics::CompactionStatus; use codex_analytics::CompactionStrategy; use codex_analytics::CompactionTrigger; use codex_analytics::now_unix_seconds; -use codex_features::Feature; use codex_protocol::error::CodexErr; use codex_protocol::error::Result as CodexResult; use codex_protocol::items::ContextCompactionItem; @@ -295,7 +294,6 @@ async fn run_compact_task_inner_impl( } pub(crate) struct CompactionAnalyticsAttempt { - enabled: bool, thread_id: String, turn_id: String, trigger: CompactionTrigger, @@ -316,10 +314,8 @@ impl CompactionAnalyticsAttempt { implementation: CompactionImplementation, phase: CompactionPhase, ) -> Self { - let enabled = sess.enabled(Feature::GeneralAnalytics); let active_context_tokens_before = sess.get_total_token_usage().await; Self { - enabled, thread_id: sess.conversation_id.to_string(), turn_id: turn_context.sub_id.clone(), trigger, @@ -338,9 +334,6 @@ impl CompactionAnalyticsAttempt { status: CompactionStatus, error: Option, ) { - if !self.enabled { - return; - } let active_context_tokens_after = sess.get_total_token_usage().await; sess.services .analytics_events_client diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 2635d641d5..850d84dd2a 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -6,7 +6,6 @@ use codex_analytics::GuardianReviewDecision; use codex_analytics::GuardianReviewFailureReason; use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTrackContext; -use codex_features::Feature; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -161,13 +160,9 @@ pub(crate) fn is_guardian_reviewer_source( fn track_guardian_review( session: &Session, - turn: &TurnContext, tracking: &GuardianReviewTrackContext, result: GuardianReviewAnalyticsResult, ) { - if !turn.config.features.enabled(Feature::GeneralAnalytics) { - return; - } session .services .analytics_events_client @@ -279,7 +274,6 @@ async fn run_guardian_review( { track_guardian_review( session.as_ref(), - turn.as_ref(), &review_tracking, GuardianReviewAnalyticsResult { decision: GuardianReviewDecision::Aborted, @@ -325,7 +319,6 @@ async fn run_guardian_review( let approved = matches!(assessment.outcome, GuardianAssessmentOutcome::Allow); track_guardian_review( session.as_ref(), - turn.as_ref(), &review_tracking, GuardianReviewAnalyticsResult { decision: if approved { @@ -356,7 +349,6 @@ async fn run_guardian_review( .to_string(); track_guardian_review( session.as_ref(), - turn.as_ref(), &review_tracking, GuardianReviewAnalyticsResult { decision: GuardianReviewDecision::Denied, @@ -395,7 +387,6 @@ async fn run_guardian_review( GuardianReviewError::Cancelled => { track_guardian_review( session.as_ref(), - turn.as_ref(), &review_tracking, GuardianReviewAnalyticsResult { decision: GuardianReviewDecision::Aborted, @@ -437,7 +428,6 @@ async fn run_guardian_review( let rationale = format!("Automatic approval review failed: {message}"); track_guardian_review( session.as_ref(), - turn.as_ref(), &review_tracking, GuardianReviewAnalyticsResult { decision: GuardianReviewDecision::Denied, diff --git a/codex-rs/core/src/memories/phase2.rs b/codex-rs/core/src/memories/phase2.rs index d156d1dee5..bcad738319 100644 --- a/codex-rs/core/src/memories/phase2.rs +++ b/codex-rs/core/src/memories/phase2.rs @@ -163,17 +163,15 @@ pub(super) async fn run(session: &Arc, config: Arc) { .get_agent_config_snapshot(thread_id) .await { - if session.enabled(Feature::GeneralAnalytics) { - let client_metadata = session.app_server_client_metadata().await; - emit_subagent_session_started( - &session.services.analytics_events_client, - client_metadata, - thread_id, - /*parent_thread_id*/ None, - thread_config, - SubAgentSource::MemoryConsolidation, - ); - } + let client_metadata = session.app_server_client_metadata().await; + emit_subagent_session_started( + &session.services.analytics_events_client, + client_metadata, + thread_id, + /*parent_thread_id*/ None, + thread_config, + SubAgentSource::MemoryConsolidation, + ); } else { warn!("failed to load memory consolidation thread config for analytics: {thread_id}"); } diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index ff3d37912d..41195490b6 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -662,10 +662,6 @@ async fn track_turn_resolved_config_analytics( turn_context: &TurnContext, input: &[UserInput], ) { - if !sess.enabled(Feature::GeneralAnalytics) { - return; - } - let thread_config = { let state = sess.state.lock().await; state.session_configuration.thread_config_snapshot() diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 6a2a2bc717..4a4b12be35 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -128,8 +128,6 @@ pub enum Feature { CodexGitCommit, /// Enable runtime metrics snapshots via a manual reader. RuntimeMetrics, - /// Enable thread lifecycle analytics emitted via the app-server analytics pipeline. - GeneralAnalytics, /// Persist rollout metadata to a local SQLite database. Sqlite, /// Enable startup memory extraction and file-backed memory consolidation. @@ -712,12 +710,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::UnderDevelopment, default_enabled: false, }, - FeatureSpec { - id: Feature::GeneralAnalytics, - key: "general_analytics", - stage: Stage::Stable, - default_enabled: true, - }, FeatureSpec { id: Feature::Sqlite, key: "sqlite", diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index ca05d72d2d..b8ddf53506 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -156,12 +156,6 @@ fn browser_controls_are_stable_and_enabled_by_default() { assert_eq!(feature_for_key("computer_use"), Some(Feature::ComputerUse)); } -#[test] -fn general_analytics_is_stable_and_enabled_by_default() { - assert_eq!(Feature::GeneralAnalytics.stage(), Stage::Stable); - assert_eq!(Feature::GeneralAnalytics.default_enabled(), true); -} - #[test] fn use_linux_sandbox_bwrap_is_a_removed_feature_key() { assert_eq!( From 277186ec85da6b1aac9782c9bc8ec1ac31427caa Mon Sep 17 00:00:00 2001 From: Curtis 'Fjord' Hawthorne Date: Mon, 27 Apr 2026 12:39:24 -0700 Subject: [PATCH 11/11] Cap original-detail image token estimates (#19865) Clamp original-detail image patch estimates to the current 10k patch budget so large images cannot inflate local context accounting without bound. Add regression coverage for an over-budget image. Fixes openai/codex#19806. --- codex-rs/core/src/context_manager/history.rs | 5 +++ .../core/src/context_manager/history_tests.rs | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index c4bdc916ff..7e66a5b703 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -519,6 +519,10 @@ const RESIZED_IMAGE_BYTES_ESTIMATE: i64 = 7373; // Use a direct 32px patch count only for `detail: "original"`; // all other image inputs continue to use `RESIZED_IMAGE_BYTES_ESTIMATE`. const ORIGINAL_IMAGE_PATCH_SIZE: u32 = 32; +// See https://platform.openai.com/docs/guides/images-vision#model-sizing-behavior. +// Keep this hard-coded for now; move it into model capabilities if the patch +// budget starts changing often across model releases. +const ORIGINAL_IMAGE_MAX_PATCHES: usize = 10_000; const ORIGINAL_IMAGE_ESTIMATE_CACHE_SIZE: usize = 32; static ORIGINAL_IMAGE_ESTIMATE_CACHE: LazyLock>> = @@ -621,6 +625,7 @@ fn estimate_original_image_bytes(image_url: &str) -> Option { let patches_high = height.saturating_add(patch_size.saturating_sub(1)) / patch_size; let patch_count = patches_wide.saturating_mul(patches_high); let patch_count = usize::try_from(patch_count).unwrap_or(usize::MAX); + let patch_count = patch_count.min(ORIGINAL_IMAGE_MAX_PATCHES); Some(i64::try_from(approx_bytes_for_tokens(patch_count)).unwrap_or(i64::MAX)) }) } diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index f5c30af575..ad67deb544 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -26,6 +26,7 @@ use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::truncate_text; use image::ImageBuffer; use image::ImageFormat; +use image::Luma; use image::Rgba; use pretty_assertions::assert_eq; use regex_lite::Regex; @@ -1900,6 +1901,38 @@ fn original_detail_images_scale_with_dimensions() { assert_eq!(estimated, expected); } +#[test] +fn original_detail_images_are_capped_at_max_patch_count() { + // 3201x3201 at 32px patches yields 101 * 101 = 10,201 patches, + // which exceeds the original-detail patch budget. + let width = 3201; + let height = 3201; + let image = ImageBuffer::from_pixel(width, height, Luma([12u8])); + let mut bytes = std::io::Cursor::new(Vec::new()); + image + .write_to(&mut bytes, ImageFormat::Png) + .expect("encode png"); + let payload = BASE64_STANDARD.encode(bytes.get_ref()); + let image_url = format!("data:image/png;base64,{payload}"); + let item = ResponseItem::FunctionCallOutput { + call_id: "call-original-capped".to_string(), + output: FunctionCallOutputPayload::from_content_items(vec![ + FunctionCallOutputContentItem::InputImage { + image_url, + detail: Some(ImageDetail::Original), + }, + ]), + }; + + let raw_len = serde_json::to_string(&item).unwrap().len() as i64; + let estimated = estimate_response_item_model_visible_bytes(&item); + let capped_original_detail_image_bytes = + i64::try_from(approx_bytes_for_tokens(ORIGINAL_IMAGE_MAX_PATCHES)).unwrap(); + let expected = raw_len - payload.len() as i64 + capped_original_detail_image_bytes; + + assert_eq!(estimated, expected); +} + #[test] fn original_detail_webp_images_scale_with_dimensions() { // Same dimensions as the PNG case above, so the patch-based replacement cost is the same.