diff --git a/src/acp_client.rs b/src/acp_client.rs index c782216..5afae0f 100644 --- a/src/acp_client.rs +++ b/src/acp_client.rs @@ -19,6 +19,7 @@ use tokio_tungstenite::{connect_async, tungstenite::Message}; use crate::ServerChannelMsg; use crate::acp_state::{AcpAppEvent, AcpModelsMetaInfo, AcpSessionUpdate}; +use crate::domain::auth::{OAuthFlow, OAuthResult, OAuthResultStatus}; use crate::domain::model::ModelEntry; use crate::domain::profile::{AgentInfo, ProfileInfo}; use crate::domain::session::{ @@ -27,7 +28,7 @@ use crate::domain::session::{ }; use crate::protocol::{ AuthProvidersData, ClientMsg, MeshInviteCreatedInfo, MeshNodesInfo, MeshStatusInfo, - OAuthFlowData, OAuthResultData, RedoResultData, RemoteSessionAttachInfo, RemoteSessionListInfo, + OAuthFlowDto, OAuthResultDto, RedoResultData, RemoteSessionAttachInfo, RemoteSessionListInfo, SessionListRequest, UndoResultData, UndoStackFrame, }; @@ -1220,10 +1221,12 @@ async fn handle_client_msg( json!({ "provider": provider }), ) .await?; - if let Ok(flow) = - serde_json::from_value::(ext_payload(&response).clone()) + if let Ok(flow) = serde_json::from_value::(ext_payload(&response).clone()) { - send_acp(srv_tx, AcpAppEvent::OAuthFlowStarted(flow)); + send_acp( + srv_tx, + AcpAppEvent::OAuthFlowStarted(oauth_flow_from_wire(flow)), + ); } } ClientMsg::CompleteOAuthLogin { flow_id, response } => { @@ -1234,9 +1237,12 @@ async fn handle_client_msg( ) .await?; if let Ok(result) = - serde_json::from_value::(ext_payload(&response).clone()) + serde_json::from_value::(ext_payload(&response).clone()) { - send_acp(srv_tx, AcpAppEvent::OAuthResult(result)); + send_acp( + srv_tx, + AcpAppEvent::OAuthResult(oauth_result_from_wire(result)), + ); } } ClientMsg::DisconnectOAuth { provider } => { @@ -1247,9 +1253,12 @@ async fn handle_client_msg( ) .await?; if let Ok(result) = - serde_json::from_value::(ext_payload(&response).clone()) + serde_json::from_value::(ext_payload(&response).clone()) { - send_acp(srv_tx, AcpAppEvent::OAuthResult(result)); + send_acp( + srv_tx, + AcpAppEvent::OAuthResult(oauth_result_from_wire(result)), + ); } } ClientMsg::ElicitationResponse { @@ -1518,6 +1527,27 @@ fn load_session_cwd(cwd: Option<&str>, default_cwd: PathBuf) -> PathBuf { .unwrap_or(default_cwd) } +fn oauth_flow_from_wire(flow: OAuthFlowDto) -> OAuthFlow { + OAuthFlow { + flow_id: flow.flow_id, + provider: flow.provider, + authorization_url: flow.authorization_url, + flow_kind: flow.flow_kind, + } +} + +fn oauth_result_from_wire(result: OAuthResultDto) -> OAuthResult { + OAuthResult { + provider: result.provider, + status: if result.success { + OAuthResultStatus::Success + } else { + OAuthResultStatus::Failure + }, + message: result.message, + } +} + fn undo_stack_snapshot_from_wire(frames: Vec) -> UndoStackSnapshot { UndoStackSnapshot { message_ids: frames.into_iter().map(|frame| frame.message_id).collect(), @@ -2884,6 +2914,45 @@ mod tests { .collect() } + #[test] + fn oauth_flow_from_wire_preserves_semantic_fields() { + let flow = oauth_flow_from_wire(OAuthFlowDto { + flow_id: "flow-123".into(), + provider: "openai".into(), + authorization_url: "https://auth.example.com/authorize".into(), + flow_kind: crate::domain::auth::OAuthFlowKind::DevicePoll, + }); + + assert_eq!(flow.flow_id, "flow-123"); + assert_eq!(flow.provider, "openai"); + assert_eq!(flow.authorization_url, "https://auth.example.com/authorize"); + assert_eq!( + flow.flow_kind, + crate::domain::auth::OAuthFlowKind::DevicePoll + ); + } + + #[test] + fn oauth_result_from_wire_maps_status_and_preserves_provider() { + let success = oauth_result_from_wire(OAuthResultDto { + provider: "openai".into(), + success: true, + message: "connected".into(), + }); + assert_eq!(success.provider, "openai"); + assert_eq!(success.status, OAuthResultStatus::Success); + assert_eq!(success.message, "connected"); + + let failure = oauth_result_from_wire(OAuthResultDto { + provider: "anthropic".into(), + success: false, + message: "authorization denied".into(), + }); + assert_eq!(failure.provider, "anthropic"); + assert_eq!(failure.status, OAuthResultStatus::Failure); + assert_eq!(failure.message, "authorization denied"); + } + #[test] fn undo_stack_snapshot_from_wire_preserves_message_order() { let snapshot = undo_stack_snapshot_from_wire(wire_stack(&["message-2", "message-1"])); diff --git a/src/acp_state.rs b/src/acp_state.rs index 43693e6..d1852d7 100644 --- a/src/acp_state.rs +++ b/src/acp_state.rs @@ -9,7 +9,7 @@ use crate::app::{LogLevel, POPUP_SESSION_PAGE_TARGET, Popup, Screen}; use crate::domain::activity::{ ActivityState, DelegateChildState, DelegateEntry, DelegateStats, DelegateStatus, }; -use crate::domain::auth::AuthProviderEntry; +use crate::domain::auth::{AuthProviderEntry, OAuthFlow, OAuthResult}; use crate::domain::chat::ChatEntry; use crate::domain::elicitation::ElicitationState; use crate::domain::model::ModelEntry; @@ -20,8 +20,8 @@ use crate::domain::session::{ }; use crate::domain::tool::ToolDetail; use crate::protocol::{ - ClientMsg, MeshInviteCreatedInfo, MeshNodesInfo, MeshStatusInfo, OAuthFlowData, - OAuthResultData, RemoteSessionAttachInfo, RemoteSessionListInfo, SessionListRequest, + ClientMsg, MeshInviteCreatedInfo, MeshNodesInfo, MeshStatusInfo, RemoteSessionAttachInfo, + RemoteSessionListInfo, SessionListRequest, }; use crate::tool_detail; @@ -166,8 +166,8 @@ pub(crate) enum AcpAppEvent { RedoResult(RedoResult), ForkResult(ForkResult), AuthProviders(Vec), - OAuthFlowStarted(OAuthFlowData), - OAuthResult(OAuthResultData), + OAuthFlowStarted(OAuthFlow), + OAuthResult(OAuthResult), InfoLog { target: &'static str, message: String, @@ -214,6 +214,7 @@ impl crate::app::App { if let Some(effort) = reasoning_effort { self.reasoning_effort = effort; } + self.auth_ui_notice = None; self.set_status(LogLevel::Info, "connection", "connected"); vec![] } @@ -530,18 +531,21 @@ impl crate::app::App { self.auth_panel = crate::app::AuthPanel::OAuthFlow; self.auth_oauth_response.clear(); self.auth_oauth_response_cursor = 0; - self.auth_result_message = None; + self.auth_last_result = None; + self.auth_ui_notice = None; vec![] } AcpAppEvent::OAuthResult(result) => { - let level = if result.success { + let is_success = result.is_success(); + let level = if is_success { LogLevel::Info } else { LogLevel::Warn }; self.push_log(level, "auth", &result.message); - self.auth_result_message = Some((result.success, result.message)); - if result.success { + self.auth_ui_notice = None; + self.auth_last_result = Some(result); + if is_success { self.auth_oauth_flow = None; self.auth_panel = crate::app::AuthPanel::List; } diff --git a/src/app.rs b/src/app.rs index 36be58c..0bf5320 100644 --- a/src/app.rs +++ b/src/app.rs @@ -8,7 +8,7 @@ use crate::domain::activity::{ ActivityState, DelegateChildState, DelegateEntry, DelegateStats, PendingDelegateToolCall, SessionActivity, SessionOp, SessionStatsLite, }; -use crate::domain::auth::AuthProviderEntry; +use crate::domain::auth::{AuthProviderEntry, OAuthFlow, OAuthResult}; use crate::domain::chat::{ChatEntry, format_outcome_labels}; use crate::domain::elicitation::ElicitationState; use crate::domain::model::{DelegateModelPreference, ModelEntry}; @@ -537,6 +537,13 @@ pub enum ModelPopupItem { }, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AuthUiNotice { + pub provider: Option, + pub success: bool, + pub message: String, +} + /// Which sub-panel is active in the provider auth popup. #[derive(Debug, Clone, PartialEq, Eq, Default)] pub enum AuthPanel { @@ -740,10 +747,11 @@ pub struct App { pub auth_api_key_input: String, pub auth_api_key_cursor: usize, pub auth_api_key_masked: bool, - pub auth_oauth_flow: Option, + pub auth_oauth_flow: Option, pub auth_oauth_response: String, pub auth_oauth_response_cursor: usize, - pub auth_result_message: Option<(bool, String)>, + pub auth_last_result: Option, + pub auth_ui_notice: Option, /// When clipboard copy fails, store the URL here for a fallback display popup. pub auth_clipboard_fallback: Option, @@ -968,7 +976,8 @@ impl App { auth_oauth_flow: None, auth_oauth_response: String::new(), auth_oauth_response_cursor: 0, - auth_result_message: None, + auth_last_result: None, + auth_ui_notice: None, auth_clipboard_fallback: None, delegate_entries: Vec::new(), delegate_cursor: 0, @@ -1136,7 +1145,8 @@ impl App { self.auth_oauth_flow = None; self.auth_oauth_response.clear(); self.auth_oauth_response_cursor = 0; - self.auth_result_message = None; + self.auth_last_result = None; + self.auth_ui_notice = None; self.auth_clipboard_fallback = None; } @@ -1149,10 +1159,27 @@ impl App { self.auth_oauth_flow = None; self.auth_oauth_response.clear(); self.auth_oauth_response_cursor = 0; - self.auth_result_message = None; + self.auth_last_result = None; + self.auth_ui_notice = None; self.auth_clipboard_fallback = None; } + pub fn auth_feedback_for_provider(&self, provider: &str) -> Option<(bool, &str)> { + if let Some(notice) = self.auth_ui_notice.as_ref().filter(|notice| { + notice + .provider + .as_deref() + .is_none_or(|notice_provider| notice_provider == provider) + }) { + return Some((notice.success, notice.message.as_str())); + } + + self.auth_last_result + .as_ref() + .filter(|result| result.provider == provider) + .map(|result| (result.is_success(), result.message.as_str())) + } + pub fn profile_by_id(&self, profile_id: &str) -> Option<&ProfileInfo> { self.profiles .iter() diff --git a/src/domain/auth.rs b/src/domain/auth.rs index 0a0350f..cc3185c 100644 --- a/src/domain/auth.rs +++ b/src/domain/auth.rs @@ -129,6 +129,33 @@ pub enum OAuthFlowKind { DevicePoll, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OAuthFlow { + pub flow_id: String, + pub provider: String, + pub authorization_url: String, + pub flow_kind: OAuthFlowKind, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OAuthResultStatus { + Success, + Failure, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OAuthResult { + pub provider: String, + pub status: OAuthResultStatus, + pub message: String, +} + +impl OAuthResult { + pub fn is_success(&self) -> bool { + matches!(self.status, OAuthResultStatus::Success) + } +} + #[cfg(test)] mod tests { use super::*; @@ -300,6 +327,19 @@ mod tests { ); } + #[test] + fn oauth_result_reports_success_from_semantic_status() { + let mut result = OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Success, + message: "connected".into(), + }; + assert!(result.is_success()); + + result.status = OAuthResultStatus::Failure; + assert!(!result.is_success()); + } + #[test] fn auth_method_display_labels_match_ui_contract() { assert_eq!(AuthMethod::OAuth.to_string(), "OAuth"); diff --git a/src/handlers.rs b/src/handlers.rs index 6006215..ab1d580 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -1,7 +1,7 @@ use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseEvent, MouseEventKind}; use tokio::sync::mpsc; -use crate::app::{self, App, CommandPaletteAction, LogLevel, Popup, Screen}; +use crate::app::{self, App, AuthUiNotice, CommandPaletteAction, LogLevel, Popup, Screen}; use crate::domain::activity::{ActivityState, SessionOp}; use crate::domain::auth::{OAuthFlowKind, OAuthStatus}; use crate::domain::chat::{ChatEntry, format_outcome_labels}; @@ -2220,7 +2220,8 @@ pub(crate) fn handle_auth_popup_key( let filtered = app.filtered_auth_providers(); if let Some(&(real_idx, _)) = filtered.get(app.auth_cursor) { let provider = &app.auth_providers[real_idx]; - app.auth_result_message = None; + app.auth_last_result = None; + app.auth_ui_notice = None; if provider.is_unconfigurable() { app.auth_selected = Some(real_idx); // Stay in list — the draw fn shows the info message @@ -2266,6 +2267,7 @@ pub(crate) fn handle_auth_popup_key( if let Some(&(real_idx, _)) = filtered.get(app.auth_cursor) { let provider = &app.auth_providers[real_idx]; if provider.env_var_name.is_some() || provider.has_stored_api_key { + app.auth_ui_notice = None; app.auth_selected = Some(real_idx); app.auth_panel = AuthPanel::ApiKeyInput; app.auth_api_key_input.clear(); @@ -2279,6 +2281,7 @@ pub(crate) fn handle_auth_popup_key( if let Some(&(real_idx, _)) = filtered.get(app.auth_cursor) { let provider = &app.auth_providers[real_idx]; if provider.supports_oauth { + app.auth_ui_notice = None; app.auth_selected = Some(real_idx); let provider_id = provider.provider.clone(); cmd_tx.send(ClientMsg::StartOAuthLogin { @@ -2364,8 +2367,9 @@ pub(crate) fn handle_auth_popup_key( KeyCode::Char('y') if key.modifiers.contains(KeyModifiers::CONTROL) => { // Copy authorization URL to clipboard (C-y to avoid global C-c quit) if let Some(ref flow) = app.auth_oauth_flow { + let provider = flow.provider.clone(); let url = flow.authorization_url.clone(); - try_copy_to_clipboard(app, &url); + try_copy_to_clipboard(app, &provider, &url); } } KeyCode::Enter => { @@ -2448,10 +2452,17 @@ fn copy_text_to_clipboard(text: &str) -> bool { false } -fn try_copy_to_clipboard(app: &mut App, text: &str) { +fn try_copy_to_clipboard(app: &mut App, provider: &str, text: &str) { + app.auth_ui_notice = None; + app.auth_clipboard_fallback = None; if copy_text_to_clipboard(text) { - app.auth_result_message = Some((true, "Copied to clipboard".into())); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some(provider.to_string()), + success: true, + message: "Copied to clipboard".into(), + }); } else { + app.auth_ui_notice = None; app.auth_clipboard_fallback = Some(text.to_string()); } } diff --git a/src/protocol.rs b/src/protocol.rs index 503fbe0..95b852b 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -1397,7 +1397,7 @@ pub struct ErrorData { // ── Auth / token types ──────────────────────────────────────────────────────── #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] -pub struct OAuthFlowData { +pub struct OAuthFlowDto { pub flow_id: String, pub provider: String, pub authorization_url: String, @@ -1405,7 +1405,7 @@ pub struct OAuthFlowData { } #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] -pub struct OAuthResultData { +pub struct OAuthResultDto { pub provider: String, pub success: bool, pub message: String, @@ -1418,10 +1418,76 @@ pub struct AuthProvidersData { #[cfg(test)] mod auth_data_tests { - use super::AuthProvidersData; - use crate::domain::auth::{AuthMethod, OAuthStatus}; + use super::{AuthProvidersData, OAuthFlowDto, OAuthResultDto}; + use crate::domain::auth::{AuthMethod, OAuthFlowKind, OAuthStatus}; use serde_json::json; + #[test] + fn oauth_flow_dto_deserializes_both_flow_kinds_and_requires_fields() { + for (wire_kind, expected_kind) in [ + ("redirect_code", OAuthFlowKind::RedirectCode), + ("device_poll", OAuthFlowKind::DevicePoll), + ] { + let flow: OAuthFlowDto = serde_json::from_value(json!({ + "flow_id": "flow-123", + "provider": "openai", + "authorization_url": "https://example.test/authorize", + "flow_kind": wire_kind + })) + .unwrap(); + assert_eq!(flow.flow_id, "flow-123"); + assert_eq!(flow.provider, "openai"); + assert_eq!(flow.authorization_url, "https://example.test/authorize"); + assert_eq!(flow.flow_kind, expected_kind); + } + + let complete = json!({ + "flow_id": "flow-123", + "provider": "openai", + "authorization_url": "https://example.test/authorize", + "flow_kind": "redirect_code" + }); + for field in ["flow_id", "provider", "authorization_url", "flow_kind"] { + let mut missing = complete.clone(); + missing.as_object_mut().unwrap().remove(field); + assert!(serde_json::from_value::(missing).is_err()); + } + } + + #[test] + fn oauth_result_dto_deserializes_success_failure_and_requires_fields() { + let success: OAuthResultDto = serde_json::from_value(json!({ + "provider": "openai", + "success": true, + "message": "connected" + })) + .unwrap(); + assert_eq!(success.provider, "openai"); + assert!(success.success); + assert_eq!(success.message, "connected"); + + let failure: OAuthResultDto = serde_json::from_value(json!({ + "provider": "anthropic", + "success": false, + "message": "authorization denied" + })) + .unwrap(); + assert_eq!(failure.provider, "anthropic"); + assert!(!failure.success); + assert_eq!(failure.message, "authorization denied"); + + let complete = json!({ + "provider": "openai", + "success": true, + "message": "connected" + }); + for field in ["provider", "success", "message"] { + let mut missing = complete.clone(); + missing.as_object_mut().unwrap().remove(field); + assert!(serde_json::from_value::(missing).is_err()); + } + } + #[test] fn auth_providers_data_deserializes_mixed_providers() { let data: AuthProvidersData = serde_json::from_value(json!({ diff --git a/src/runtime/mod.rs b/src/runtime/mod.rs index 067f2e8..d410faf 100644 --- a/src/runtime/mod.rs +++ b/src/runtime/mod.rs @@ -3351,9 +3351,12 @@ mod runtime_tests { #[cfg(test)] mod auth_tests { use super::*; - use crate::domain::auth::{AuthProviderEntry, OAuthFlowKind, OAuthStatus}; + use crate::app::AuthUiNotice; + use crate::domain::auth::{ + AuthProviderEntry, OAuthFlow, OAuthFlowKind, OAuthResult, OAuthResultStatus, OAuthStatus, + }; use crate::handlers::*; - use crate::protocol::{ClientMsg, OAuthFlowData, OAuthResultData}; + use crate::protocol::ClientMsg; use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; fn key(code: KeyCode) -> KeyEvent { @@ -3420,12 +3423,24 @@ mod auth_tests { app.auth_filter = "test".into(); app.auth_selected = Some(2); app.auth_api_key_input = "secret".into(); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "old result".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); app.open_auth_popup(); assert_eq!(app.popup, app::Popup::ProviderAuth); assert_eq!(app.auth_cursor, 0); assert!(app.auth_filter.is_empty()); assert!(app.auth_selected.is_none()); assert!(app.auth_api_key_input.is_empty()); + assert!(app.auth_last_result.is_none()); + assert!(app.auth_ui_notice.is_none()); assert!(app.auth_api_key_masked); assert_eq!(app.auth_panel, app::AuthPanel::List); } @@ -3452,10 +3467,78 @@ mod auth_tests { app.auth_selected = Some(1); app.auth_panel = app::AuthPanel::ApiKeyInput; app.auth_api_key_input = "secret".into(); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Success, + message: "connected".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: None, + success: true, + message: "saved".into(), + }); app.auth_close_detail(); assert!(app.auth_selected.is_none()); assert_eq!(app.auth_panel, app::AuthPanel::List); assert!(app.auth_api_key_input.is_empty()); + assert!(app.auth_last_result.is_none()); + assert!(app.auth_ui_notice.is_none()); + } + + #[test] + fn auth_feedback_scopes_oauth_result_to_its_provider() { + let mut app = App::new(); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "authorization denied".into(), + }); + + assert_eq!(app.auth_feedback_for_provider("anthropic"), None); + assert_eq!( + app.auth_feedback_for_provider("openai"), + Some((false, "authorization denied")) + ); + } + + #[test] + fn auth_feedback_scopes_ui_notice_and_takes_precedence() { + let mut app = App::new(); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "authorization denied".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "Copied to clipboard".into(), + }); + + assert_eq!(app.auth_feedback_for_provider("anthropic"), None); + assert_eq!( + app.auth_feedback_for_provider("openai"), + Some((true, "Copied to clipboard")) + ); + } + + #[test] + fn auth_feedback_supports_generic_ui_notice() { + let mut app = App::new(); + app.auth_ui_notice = Some(AuthUiNotice { + provider: None, + success: false, + message: "Clipboard unavailable".into(), + }); + + assert_eq!( + app.auth_feedback_for_provider("openai"), + Some((false, "Clipboard unavailable")) + ); + assert_eq!( + app.auth_feedback_for_provider("anthropic"), + Some((false, "Clipboard unavailable")) + ); } // ── Key handler tests: List panel ───────────────────────────────────────── @@ -3472,10 +3555,22 @@ mod auth_tests { fn auth_list_esc_clears_selection_when_selected() { let mut app = make_app_with_providers(vec![make_provider("OpenAI")]); app.auth_selected = Some(0); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "old result".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, key(KeyCode::Esc), &tx).unwrap(); assert_eq!(app.popup, app::Popup::ProviderAuth); assert!(app.auth_selected.is_none()); + assert!(app.auth_last_result.is_none()); + assert!(app.auth_ui_notice.is_none()); } #[test] @@ -3500,29 +3595,53 @@ mod auth_tests { #[test] fn auth_list_enter_on_api_key_only_opens_api_key_panel() { let mut app = make_app_with_providers(vec![make_api_key_only("Groq")]); + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "old result".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, key(KeyCode::Enter), &tx).unwrap(); assert_eq!(app.auth_panel, app::AuthPanel::ApiKeyInput); assert_eq!(app.auth_selected, Some(0)); + assert!(app.auth_last_result.is_none()); + assert!(app.auth_ui_notice.is_none()); } #[test] fn auth_list_enter_on_oauth_only_starts_flow() { let mut app = make_app_with_providers(vec![make_oauth_only("Codex")]); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("codex".into()), + success: true, + message: "old notice".into(), + }); let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, key(KeyCode::Enter), &tx).unwrap(); assert_eq!(app.auth_selected, Some(0)); let msg = rx.try_recv().expect("message sent"); assert!(matches!(msg, ClientMsg::StartOAuthLogin { provider } if provider == "codex")); + assert!(app.auth_ui_notice.is_none()); } #[test] fn auth_list_enter_on_multi_method_selects_provider() { let mut app = make_app_with_providers(vec![make_provider("OpenAI")]); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, key(KeyCode::Enter), &tx).unwrap(); assert_eq!(app.auth_selected, Some(0)); assert_eq!(app.auth_panel, app::AuthPanel::List); + assert!(app.auth_ui_notice.is_none()); } #[test] @@ -3546,19 +3665,31 @@ mod auth_tests { #[test] fn auth_list_ctrl_k_opens_api_key_panel() { let mut app = make_app_with_providers(vec![make_provider("OpenAI")]); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, ctrl('k'), &tx).unwrap(); assert_eq!(app.auth_panel, app::AuthPanel::ApiKeyInput); assert_eq!(app.auth_selected, Some(0)); + assert!(app.auth_ui_notice.is_none()); } #[test] fn auth_list_ctrl_o_starts_oauth() { let mut app = make_app_with_providers(vec![make_provider("OpenAI")]); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, ctrl('o'), &tx).unwrap(); let msg = rx.try_recv().expect("message sent"); assert!(matches!(msg, ClientMsg::StartOAuthLogin { provider } if provider == "openai")); + assert!(app.auth_ui_notice.is_none()); } // ── Key handler tests: API Key panel ────────────────────────────────────── @@ -3655,7 +3786,7 @@ mod auth_tests { let mut app = make_app_with_providers(vec![make_oauth_only("Codex")]); app.auth_selected = Some(0); app.auth_panel = app::AuthPanel::OAuthFlow; - app.auth_oauth_flow = Some(OAuthFlowData { + app.auth_oauth_flow = Some(OAuthFlow { flow_id: "f1".into(), provider: "codex".into(), authorization_url: "https://example.com".into(), @@ -3673,7 +3804,7 @@ mod auth_tests { let mut app = make_app_with_providers(vec![make_oauth_only("Codex")]); app.auth_selected = Some(0); app.auth_panel = app::AuthPanel::OAuthFlow; - app.auth_oauth_flow = Some(OAuthFlowData { + app.auth_oauth_flow = Some(OAuthFlow { flow_id: "f1".into(), provider: "codex".into(), authorization_url: "https://example.com".into(), @@ -3701,7 +3832,7 @@ mod auth_tests { let mut app = make_app_with_providers(vec![make_oauth_only("Codex")]); app.auth_selected = Some(0); app.auth_panel = app::AuthPanel::OAuthFlow; - app.auth_oauth_flow = Some(OAuthFlowData { + app.auth_oauth_flow = Some(OAuthFlow { flow_id: "f1".into(), provider: "codex".into(), authorization_url: "https://example.com/device".into(), @@ -3720,6 +3851,28 @@ mod auth_tests { // ── Native ACP event handling tests ─────────────────────────────────────── + #[test] + fn native_initialized_event_clears_auth_ui_notice() { + let mut app = App::new(); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); + + let cmds = app.handle_acp_event(AcpAppEvent::Initialized { + agent_id: "agent-1".into(), + agent_name: "Agent".into(), + profiles: Vec::new(), + active_profile_id: None, + agent_mode: None, + reasoning_effort: None, + }); + + assert!(cmds.is_empty()); + assert!(app.auth_ui_notice.is_none()); + } + #[test] fn native_auth_providers_event_populates_list() { let mut app = App::new(); @@ -3743,7 +3896,18 @@ mod auth_tests { let mut app = App::new(); app.popup = app::Popup::ProviderAuth; - let cmds = app.handle_acp_event(AcpAppEvent::OAuthFlowStarted(OAuthFlowData { + app.auth_last_result = Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "old result".into(), + }); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "old notice".into(), + }); + + let cmds = app.handle_acp_event(AcpAppEvent::OAuthFlowStarted(OAuthFlow { flow_id: "flow-123".into(), provider: "openai".into(), authorization_url: "https://auth.example.com/authorize".into(), @@ -3757,12 +3921,14 @@ mod auth_tests { assert_eq!(flow.provider, "openai"); assert_eq!(flow.flow_kind, OAuthFlowKind::RedirectCode); assert_eq!(app.auth_panel, app::AuthPanel::OAuthFlow); + assert!(app.auth_last_result.is_none()); + assert!(app.auth_ui_notice.is_none()); } #[test] fn native_oauth_result_success_clears_flow() { let mut app = App::new(); - app.auth_oauth_flow = Some(OAuthFlowData { + app.auth_oauth_flow = Some(OAuthFlow { flow_id: "f1".into(), provider: "openai".into(), authorization_url: "https://example.com".into(), @@ -3770,24 +3936,70 @@ mod auth_tests { }); app.auth_panel = app::AuthPanel::OAuthFlow; - let cmds = app.handle_acp_event(AcpAppEvent::OAuthResult(OAuthResultData { + let cmds = app.handle_acp_event(AcpAppEvent::OAuthResult(OAuthResult { provider: "openai".into(), - success: true, + status: OAuthResultStatus::Success, message: "Connected successfully".into(), })); - assert!( - cmds.iter() - .any(|c| matches!(c, ClientMsg::ListAuthProviders)) - ); + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], ClientMsg::ListAuthProviders)); assert!(app.auth_oauth_flow.is_none()); assert_eq!(app.auth_panel, app::AuthPanel::List); assert_eq!( - app.auth_result_message, - Some((true, "Connected successfully".into())) + app.auth_last_result, + Some(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Success, + message: "Connected successfully".into(), + }) ); } + #[test] + fn native_oauth_result_failure_preserves_flow_and_refreshes_providers() { + let mut app = App::new(); + let flow = OAuthFlow { + flow_id: "f1".into(), + provider: "anthropic".into(), + authorization_url: "https://example.com".into(), + flow_kind: OAuthFlowKind::RedirectCode, + }; + app.auth_oauth_flow = Some(flow.clone()); + app.auth_panel = app::AuthPanel::OAuthFlow; + + let result = OAuthResult { + provider: "anthropic".into(), + status: OAuthResultStatus::Failure, + message: "Authorization denied".into(), + }; + let cmds = app.handle_acp_event(AcpAppEvent::OAuthResult(result.clone())); + + assert_eq!(cmds.len(), 1); + assert!(matches!(cmds[0], ClientMsg::ListAuthProviders)); + assert_eq!(app.auth_oauth_flow, Some(flow)); + assert_eq!(app.auth_panel, app::AuthPanel::OAuthFlow); + assert_eq!(app.auth_last_result, Some(result)); + } + + #[test] + fn native_oauth_result_clears_auth_ui_notice() { + let mut app = App::new(); + app.auth_ui_notice = Some(AuthUiNotice { + provider: Some("openai".into()), + success: true, + message: "Copied to clipboard".into(), + }); + + app.handle_acp_event(AcpAppEvent::OAuthResult(OAuthResult { + provider: "openai".into(), + status: OAuthResultStatus::Failure, + message: "Authorization denied".into(), + })); + + assert!(app.auth_ui_notice.is_none()); + } + // ── Disconnect / clear credential tests (C-d in List panel) ───────────── #[test] @@ -3868,7 +4080,7 @@ mod auth_tests { let mut app = make_app_with_providers(vec![make_oauth_only("Codex")]); app.auth_selected = Some(0); app.auth_panel = app::AuthPanel::OAuthFlow; - app.auth_oauth_flow = Some(OAuthFlowData { + app.auth_oauth_flow = Some(OAuthFlow { flow_id: "f1".into(), provider: "codex".into(), authorization_url: "https://auth.example.com/authorize".into(), @@ -3877,12 +4089,24 @@ mod auth_tests { let (tx, _rx) = tokio::sync::mpsc::unbounded_channel(); handle_auth_popup_key(&mut app, ctrl('y'), &tx).unwrap(); - // In CI there's no clipboard tool, so it falls back to the URL display + + let expected_notice = AuthUiNotice { + provider: Some("codex".into()), + success: true, + message: "Copied to clipboard".into(), + }; + let expected_url = "https://auth.example.com/authorize"; assert!( - app.auth_clipboard_fallback.is_some() - || app.auth_result_message == Some((true, "Copied to clipboard".into())), - "C-y should attempt clipboard copy" + matches!( + (&app.auth_ui_notice, &app.auth_clipboard_fallback), + (Some(notice), None) if notice == &expected_notice + ) || matches!( + (&app.auth_ui_notice, &app.auth_clipboard_fallback), + (None, Some(url)) if url == expected_url + ), + "C-y should show the provider notice or exact fallback URL" ); + assert!(app.auth_last_result.is_none()); } #[test] diff --git a/src/ui/popups.rs b/src/ui/popups.rs index 2e7b904..e89a825 100644 --- a/src/ui/popups.rs +++ b/src/ui/popups.rs @@ -2724,7 +2724,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { ))); } - if let Some((success, ref msg)) = app.auth_result_message { + if let Some((success, message)) = app.auth_feedback_for_provider(&provider.provider) { let style = if success { ratatui::style::Style::default() .fg(Theme::ok()) @@ -2734,7 +2734,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { .fg(Theme::err()) .bg(Theme::bg_dim()) }; - lines.push(Line::from(Span::styled(format!(" {msg}"), style))); + lines.push(Line::from(Span::styled(format!(" {message}"), style))); } for (i, line) in lines.into_iter().enumerate() { @@ -2807,7 +2807,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { Span::styled(input_text, input_style), ])); - if let Some((success, ref msg)) = app.auth_result_message { + if let Some((success, message)) = app.auth_feedback_for_provider(&provider.provider) { let style = if success { ratatui::style::Style::default() .fg(Theme::ok()) @@ -2817,7 +2817,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { .fg(Theme::err()) .bg(Theme::bg_dim()) }; - lines.push(Line::from(Span::styled(format!(" {msg}"), style))); + lines.push(Line::from(Span::styled(format!(" {message}"), style))); } for (i, line) in lines.into_iter().enumerate() { @@ -2897,7 +2897,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { ))); } - if let Some((success, ref msg)) = app.auth_result_message { + if let Some((success, message)) = app.auth_feedback_for_provider(&provider.provider) { let style = if success { ratatui::style::Style::default() .fg(Theme::ok()) @@ -2907,7 +2907,7 @@ fn draw_auth_detail_panel(f: &mut Frame, app: &App, area: Rect) { .fg(Theme::err()) .bg(Theme::bg_dim()) }; - lines.push(Line::from(Span::styled(format!(" {msg}"), style))); + lines.push(Line::from(Span::styled(format!(" {message}"), style))); } for (i, line) in lines.into_iter().enumerate() {