From 3515d9b03451778ced1bd94c85f4bcc95d4e90a2 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 14 Mar 2026 15:19:19 -0600 Subject: [PATCH] codex: address PR review feedback (#14710) --- codex-rs/tui/src/app.rs | 2 +- codex-rs/tui/src/app/app_server_adapter.rs | 16 ++++++- codex-rs/tui/src/onboarding/account_login.rs | 1 + codex-rs/tui/src/onboarding/auth.rs | 46 +++++++++++++++++--- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 6cd64041a7..f3abb0b037 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -703,7 +703,7 @@ pub(crate) struct App { active_turn_ids: HashMap, pending_exec_approval_request_ids: HashMap, pending_patch_approval_request_ids: HashMap, - pending_elicitation_request_ids: HashMap<(String, String), RequestId>, + pending_elicitation_request_ids: HashMap<(String, RequestId), RequestId>, pending_permissions_request_ids: HashMap, pending_user_input_request_ids: HashMap, pending_dynamic_tool_request_ids: HashMap, diff --git a/codex-rs/tui/src/app/app_server_adapter.rs b/codex-rs/tui/src/app/app_server_adapter.rs index f408a548b7..3061239e58 100644 --- a/codex-rs/tui/src/app/app_server_adapter.rs +++ b/codex-rs/tui/src/app/app_server_adapter.rs @@ -553,7 +553,10 @@ impl App { content, meta, } => { - let key = (server_name.clone(), request_id.to_string()); + let key = ( + server_name.clone(), + mcp_request_id_to_app_server_request_id(&request_id), + ); let Some(server_request_id) = self.pending_elicitation_request_ids.remove(&key) else { return Err(format!( @@ -769,7 +772,7 @@ impl App { } ServerRequest::McpServerElicitationRequest { request_id, params } => { self.pending_elicitation_request_ids.insert( - (params.server_name.clone(), request_id.to_string()), + (params.server_name.clone(), request_id.clone()), request_id.clone(), ); } @@ -1439,6 +1442,15 @@ fn review_decision_to_file_change_decision( } } +fn mcp_request_id_to_app_server_request_id( + request_id: &codex_protocol::mcp::RequestId, +) -> RequestId { + match request_id { + codex_protocol::mcp::RequestId::String(value) => RequestId::String(value.clone()), + codex_protocol::mcp::RequestId::Integer(value) => RequestId::Integer(*value), + } +} + fn permission_grant_scope_to_api( scope: PermissionGrantScope, ) -> codex_app_server_protocol::PermissionGrantScope { diff --git a/codex-rs/tui/src/onboarding/account_login.rs b/codex-rs/tui/src/onboarding/account_login.rs index e7b5b32067..abaf2ce50f 100644 --- a/codex-rs/tui/src/onboarding/account_login.rs +++ b/codex-rs/tui/src/onboarding/account_login.rs @@ -13,6 +13,7 @@ use serde::de::DeserializeOwned; use crate::LoginStatus; +#[derive(Debug, PartialEq)] pub(crate) enum AuthCommand { StartApiKey { api_key: String }, StartChatgpt, diff --git a/codex-rs/tui/src/onboarding/auth.rs b/codex-rs/tui/src/onboarding/auth.rs index 398062eb06..2e5d94e5d6 100644 --- a/codex-rs/tui/src/onboarding/auth.rs +++ b/codex-rs/tui/src/onboarding/auth.rs @@ -721,12 +721,24 @@ impl AuthModeWidget { } pub(crate) fn apply_chatgpt_login_started(&mut self, login_id: String, auth_url: String) { + let mut sign_in_state = self.sign_in_state.write().unwrap(); + let still_waiting_for_login = matches!( + &*sign_in_state, + SignInState::ChatGptContinueInBrowser(ContinueInBrowserState { login_id: None, .. }) + ); + if !still_waiting_for_login { + drop(sign_in_state); + let _ = self + .auth_command_tx + .send(AuthCommand::CancelChatgpt { login_id }); + return; + } + self.error = None; - *self.sign_in_state.write().unwrap() = - SignInState::ChatGptContinueInBrowser(ContinueInBrowserState { - auth_url, - login_id: Some(login_id), - }); + *sign_in_state = SignInState::ChatGptContinueInBrowser(ContinueInBrowserState { + auth_url, + login_id: Some(login_id), + }); self.request_frame.schedule_frame(); } @@ -930,6 +942,30 @@ mod tests { assert_eq!(found, url, "OSC 8 hyperlink should cover the full URL"); } + #[test] + fn chatgpt_login_start_after_cancel_requests_server_cancel_without_reopening_ui() { + let (auth_command_tx, mut auth_command_rx) = tokio::sync::mpsc::unbounded_channel(); + let mut widget = auth_widget(Some(ForcedLoginMethod::Chatgpt), SignInOption::ChatGpt); + widget.auth_command_tx = auth_command_tx; + *widget.sign_in_state.write().unwrap() = SignInState::PickMode; + + widget.apply_chatgpt_login_started( + "login-123".to_string(), + "https://auth.example.com/login?state=abc123".to_string(), + ); + + assert!(matches!( + &*widget.sign_in_state.read().unwrap(), + SignInState::PickMode + )); + assert_eq!( + auth_command_rx.try_recv().unwrap(), + AuthCommand::CancelChatgpt { + login_id: "login-123".to_string() + } + ); + } + #[test] fn device_code_login_pending_snapshot() { let mut widget = auth_widget(None, SignInOption::DeviceCode);