From 920a13fefd0720f7f08232aa892a86c9d8e13ef2 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 14 Mar 2026 17:21:44 -0600 Subject: [PATCH] codex: address PR review feedback (#14710) --- codex-rs/tui/src/app.rs | 40 +++++++++++++++++++ codex-rs/tui/src/app/app_server_adapter.rs | 8 ++-- codex-rs/tui/src/onboarding/auth.rs | 31 ++++++++++++++ .../tui/src/onboarding/onboarding_screen.rs | 2 +- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 44ef4a2d86..2ea82c2368 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -7550,6 +7550,46 @@ smart_approvals = true .expect("shutdown should complete"); } + #[tokio::test] + async fn override_turn_context_and_reload_user_config_use_runtime_path() { + let mut app = make_test_app().await; + let app_server = start_test_app_server(app.config.clone()).await; + let event = app + .thread_start_via_app_server(&app_server, &app.config.clone()) + .await + .expect("thread/start should succeed"); + + assert!( + app.submit_app_server_op( + &app_server, + event.session_id, + Op::OverrideTurnContext { + cwd: None, + approval_policy: Some(AskForApproval::OnFailure), + approvals_reviewer: None, + sandbox_policy: None, + windows_sandbox_level: None, + model: None, + effort: None, + summary: None, + service_tier: None, + collaboration_mode: None, + personality: None, + } + ) + .await + ); + assert!( + app.submit_app_server_op(&app_server, event.session_id, Op::ReloadUserConfig) + .await + ); + + app_server + .shutdown() + .await + .expect("shutdown should complete"); + } + #[tokio::test] async fn clear_only_ui_reset_preserves_chat_session_state() { let mut app = make_test_app().await; diff --git a/codex-rs/tui/src/app/app_server_adapter.rs b/codex-rs/tui/src/app/app_server_adapter.rs index 3061239e58..f6fd7306a8 100644 --- a/codex-rs/tui/src/app/app_server_adapter.rs +++ b/codex-rs/tui/src/app/app_server_adapter.rs @@ -657,10 +657,6 @@ impl App { .await?; let _ = config; } - Op::ReloadUserConfig | Op::OverrideTurnContext { .. } => { - // TODO(app-server): add a thread-scoped override/context refresh API so the TUI - // does not need to treat these as local-only state updates. - } Op::Compact => { let _: ThreadCompactStartResponse = send_request_with_response( app_server_client, @@ -728,9 +724,11 @@ impl App { | Op::Undo | Op::DropMemories | Op::UpdateMemories + | Op::ReloadUserConfig | Op::RunUserShellCommand { .. } | Op::GetHistoryEntryRequest { .. } - | Op::ListMcpTools => { + | Op::ListMcpTools + | Op::OverrideTurnContext { .. } => { // TODO(app-server): migrate these legacy-only TUI features once app-server grows // equivalent APIs. Until then, keep routing the still-emitted TUI ops through the // shared in-process thread runtime so existing behavior does not regress. diff --git a/codex-rs/tui/src/onboarding/auth.rs b/codex-rs/tui/src/onboarding/auth.rs index 2e5d94e5d6..4402095a4c 100644 --- a/codex-rs/tui/src/onboarding/auth.rs +++ b/codex-rs/tui/src/onboarding/auth.rs @@ -799,6 +799,11 @@ impl AuthModeWidget { self.request_frame.schedule_frame(); } + pub(crate) fn show_continue_in_browser_error(&mut self, message: String) { + self.error = Some(message); + self.request_frame.schedule_frame(); + } + pub(crate) fn show_device_code_login_error(&mut self, message: String) { *self.sign_in_state.write().unwrap() = SignInState::PickMode; self.error = Some(message); @@ -966,6 +971,32 @@ mod tests { ); } + #[test] + fn continue_in_browser_error_keeps_active_login_state() { + let mut widget = auth_widget(Some(ForcedLoginMethod::Chatgpt), SignInOption::ChatGpt); + *widget.sign_in_state.write().unwrap() = + SignInState::ChatGptContinueInBrowser(ContinueInBrowserState { + auth_url: String::new(), + login_id: None, + }); + widget.apply_chatgpt_login_started( + "login-123".to_string(), + "https://auth.example.com/login?state=abc123".to_string(), + ); + + widget.show_continue_in_browser_error("Failed to open browser".to_string()); + + assert_eq!(widget.error.as_deref(), Some("Failed to open browser")); + assert!(matches!( + &*widget.sign_in_state.read().unwrap(), + SignInState::ChatGptContinueInBrowser(ContinueInBrowserState { + auth_url, + login_id: Some(login_id), + }) if auth_url == "https://auth.example.com/login?state=abc123" + && login_id == "login-123" + )); + } + #[test] fn device_code_login_pending_snapshot() { let mut widget = auth_widget(None, SignInOption::DeviceCode); diff --git a/codex-rs/tui/src/onboarding/onboarding_screen.rs b/codex-rs/tui/src/onboarding/onboarding_screen.rs index 9456a54d58..ca6c09282f 100644 --- a/codex-rs/tui/src/onboarding/onboarding_screen.rs +++ b/codex-rs/tui/src/onboarding/onboarding_screen.rs @@ -555,7 +555,7 @@ async fn handle_auth_command( Ok(LoginAccountResponse::Chatgpt { login_id, auth_url }) => { auth_widget.apply_chatgpt_login_started(login_id, auth_url.clone()); if let Err(err) = webbrowser::open(&auth_url) { - auth_widget.show_login_request_error(format!( + auth_widget.show_continue_in_browser_error(format!( "Failed to open browser for {auth_url}: {err}" )); }