diff --git a/crates/codegen/kigi-tui/src/app/actions.rs b/crates/codegen/kigi-tui/src/app/actions.rs index 1e6dcc1..abccd2e 100644 --- a/crates/codegen/kigi-tui/src/app/actions.rs +++ b/crates/codegen/kigi-tui/src/app/actions.rs @@ -562,6 +562,11 @@ pub enum Action { SwitchAccount, /// User pressed login on the welcome screen. Login, + /// User chose a specific OAuth provider row on the login picker. Routes + /// the flow to THAT advertised method — `Login` alone resolves to the + /// first interactive method, which sent every provider row to the Kimi + /// device flow. + LoginWith(acp::AuthMethodId), /// Cancel an in-progress login that was started from inside a session /// (`/login` or a 401 re-auth prompt) and return to the previous view. /// Distinct from `Quit`: abandoning a mid-session re-auth must not exit diff --git a/crates/codegen/kigi-tui/src/app/app_view.rs b/crates/codegen/kigi-tui/src/app/app_view.rs index f0bb44d..bdd87f2 100644 --- a/crates/codegen/kigi-tui/src/app/app_view.rs +++ b/crates/codegen/kigi-tui/src/app/app_view.rs @@ -298,8 +298,12 @@ impl PlatformLogin { /// One row of the unauthenticated welcome menu (the login picker). #[derive(Debug, Clone, PartialEq, Eq)] pub enum PendingMenuItem { - /// Interactive OAuth login (the existing `kimi-code` device flow). + /// Interactive OAuth login. `method_id` names the advertised method this + /// row starts (`kimi-code`, `xai-grok`, `claude-pro-max`, …) — `None` + /// only on the no-interactive-method fallback row, whose `Action::Login` + /// surfaces the proper error. Login { + method_id: Option, label: String, }, /// Open-platform API-key entry. @@ -321,7 +325,7 @@ impl PendingMenuItem { } pub fn label(&self) -> &str { match self { - Self::Login { label } | Self::ApiKey { label, .. } => label, + Self::Login { label, .. } | Self::ApiKey { label, .. } => label, Self::Quit => "Quit", } } @@ -347,12 +351,14 @@ pub fn pending_menu_items( }); } else if AuthMethodKind::from_id(method.id()).needs_interactive_login() { items.push(PendingMenuItem::Login { + method_id: Some(method.id().clone()), label: format!("{} (OAuth)", method.name()), }); } } if items.is_empty() { items.push(PendingMenuItem::Login { + method_id: None, label: format!("Login with {}", login_label.unwrap_or("kimi.com")), }); } @@ -2827,7 +2833,15 @@ fn handle_menu_nav( /// Rows come from [`pending_menu_items`]: interactive methods + Quit. fn dispatch_pending_menu_action(items: &[PendingMenuItem], index: usize) -> InputOutcome { match items.get(index) { - Some(PendingMenuItem::Login { .. }) => InputOutcome::Action(Action::Login), + // The row's own method id rides along — every provider must start + // ITS OWN flow, not the first advertised (Kimi) one. + Some(PendingMenuItem::Login { + method_id: Some(id), + .. + }) => InputOutcome::Action(Action::LoginWith(id.clone())), + Some(PendingMenuItem::Login { + method_id: None, .. + }) => InputOutcome::Action(Action::Login), Some(PendingMenuItem::ApiKey { target, .. }) => { InputOutcome::Action(Action::BeginPlatformKeyEntry(*target)) } @@ -6916,34 +6930,41 @@ pub(crate) mod tests { ) .methods } + /// Assert an OAuth picker row carries BOTH its label and its own method + /// id — the id is what routes the flow to that provider (a row without + /// it fell back to the first advertised method: the Kimi device flow). + fn assert_login_row(item: &PendingMenuItem, method_id: &str, label: &str) { + match item { + PendingMenuItem::Login { + method_id: Some(id), + label: got, + } => { + assert_eq!(id.0.as_ref(), method_id, "row must route to its own method"); + assert_eq!(got, label); + } + other => panic!("expected the {method_id} login row, got {other:?}"), + } + } #[test] fn pending_menu_items_lists_interactive_methods_plus_quit() { let items = pending_menu_items(&fresh_user_auth_methods(), None); assert_eq!(items.len(), 30, "29 login rows + Quit, got {items:?}"); - assert!( - matches!(&items[0], PendingMenuItem::Login { label } if label == "Kimi Code (OAuth)"), - "row 0 must be the OAuth login, got {:?}", - items[0] + assert_login_row(&items[0], "kimi-code", "Kimi Code (OAuth)"); + assert_login_row(&items[1], "xai-grok", "xAI Grok (subscription) (OAuth)"); + assert_login_row( + &items[2], + "claude-pro-max", + "Claude Pro/Max (subscription) (OAuth)", ); - assert!( - matches!(&items[1], PendingMenuItem::Login { label } if label == "xAI Grok (subscription) (OAuth)"), - "row 1 must be the xai-grok OAuth login (interactive, after kimi-code), got {:?}", - items[1] + assert_login_row( + &items[3], + "github-copilot", + "GitHub Copilot (subscription) (OAuth)", ); - assert!( - matches!(&items[2], PendingMenuItem::Login { label } if label == "Claude Pro/Max (subscription) (OAuth)"), - "row 2 must be the claude-pro-max OAuth login (after xai-grok), got {:?}", - items[2] - ); - assert!( - matches!(&items[3], PendingMenuItem::Login { label } if label == "GitHub Copilot (subscription) (OAuth)"), - "row 3 must be the github-copilot OAuth login (after claude-pro-max), got {:?}", - items[3] - ); - assert!( - matches!(&items[4], PendingMenuItem::Login { label } if label == "ChatGPT Plus/Pro (Codex) (OAuth)"), - "row 4 must be the openai-codex OAuth login (after github-copilot), got {:?}", - items[4] + assert_login_row( + &items[4], + "openai-codex", + "ChatGPT Plus/Pro (Codex) (OAuth)", ); assert_eq!( items[5], @@ -7168,9 +7189,39 @@ pub(crate) mod tests { "Enter on the moonshot-cn row must open its key entry, got {outcome:?}" ); // 'l' is muscle-memory for the first (OAuth) row regardless of the - // arrow selection. + // arrow selection — and it must carry that row's own method id. let outcome = app.handle_input(&key_event(KeyCode::Char('l'), KeyModifiers::NONE)); - assert!(matches!(outcome, InputOutcome::Action(Action::Login))); + assert!( + matches!( + &outcome, + InputOutcome::Action(Action::LoginWith(id)) if id.0.as_ref() == "kimi-code" + ), + "'l' must start the kimi-code flow, got {outcome:?}" + ); + } + /// Selecting a subscription-OAuth row must dispatch `LoginWith` carrying + /// THAT provider's method id. Regression: every Login row collapsed to + /// the id-less `Action::Login`, which resolved to the FIRST interactive + /// method — sending Grok/Claude/Copilot/Codex logins to kimi.com. + #[test] + fn welcome_pending_oauth_rows_route_to_their_own_provider() { + let items = pending_menu_items(&fresh_user_auth_methods(), None); + for (index, expected) in [ + (0, "kimi-code"), + (1, "xai-grok"), + (2, "claude-pro-max"), + (3, "github-copilot"), + (4, "openai-codex"), + ] { + let outcome = dispatch_pending_menu_action(&items, index); + assert!( + matches!( + &outcome, + InputOutcome::Action(Action::LoginWith(id)) if id.0.as_ref() == expected + ), + "row {index} must dispatch LoginWith({expected}), got {outcome:?}" + ); + } } #[test] fn welcome_api_key_entry_esc_returns_to_picker() { diff --git a/crates/codegen/kigi-tui/src/app/dispatch/auth.rs b/crates/codegen/kigi-tui/src/app/dispatch/auth.rs index 4fb685d..d319d14 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/auth.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/auth.rs @@ -175,6 +175,44 @@ pub(super) fn strip_trailing_auth_error_blocks(agent: &mut AgentView) { /// restored once auth completes or is cancelled. Without this, `/login` /// with an external auth provider configured appeared to do nothing. pub(super) fn dispatch_login(app: &mut AppView) -> Vec { + dispatch_login_with(app, None) +} + +/// Start an interactive login flow with an explicitly chosen method (a +/// provider row on the login picker). `None` keeps the historical behavior: +/// re-use the current method or resolve the first interactive one. +/// +/// The explicit id is resolved against the shell-advertised `auth_methods` +/// and FAILS CLOSED when absent — silently falling back to the first method +/// is exactly the bug that sent every provider row to the Kimi flow. +pub(super) fn dispatch_login_with( + app: &mut AppView, + method_id: Option, +) -> Vec { + if let Some(id) = method_id { + let Some(method) = app.auth_methods.iter().find(|m| *m.id() == id) else { + app.auth_state = AuthState::Pending { + error: Some(format!("Login method not available: {}", id.0)), + }; + return vec![]; + }; + // Mirror `find_interactive_login_method`: external auth providers + // start in Command mode, everything else Pending (the mode firms up + // when the auth URL arrives). + let is_provider = method + .meta() + .as_ref() + .and_then(|v| v.get("external_provider")) + .and_then(|v| v.as_bool()) + .unwrap_or(false); + app.login_label = Some(method.name().to_string()); + app.login_method_id = Some(method.id().clone()); + app.auth_start_mode = if is_provider { + AuthMode::Command + } else { + AuthMode::Pending + }; + } ensure_login_method(app); let Some(method_id) = app.login_method_id.clone() else { app.auth_state = AuthState::Pending { diff --git a/crates/codegen/kigi-tui/src/app/dispatch/router.rs b/crates/codegen/kigi-tui/src/app/dispatch/router.rs index 405f318..479df9b 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/router.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/router.rs @@ -1,8 +1,8 @@ //! Top-level action router: maps actions and action results to handlers. use super::auth::{ dispatch_begin_platform_key_entry, dispatch_cancel_login, dispatch_cancel_platform_key_entry, - dispatch_login, dispatch_logout, dispatch_submit_auth_code, dispatch_submit_platform_api_key, - dispatch_switch_account, + dispatch_login, dispatch_login_with, dispatch_logout, dispatch_submit_auth_code, + dispatch_submit_platform_api_key, dispatch_switch_account, }; use super::ctx::{ active_agent_session_id, get_active_agent_mut, navigate_clearing_selection, @@ -876,6 +876,7 @@ pub(crate) fn dispatch(action: Action, app: &mut AppView) -> Vec { vec![] } Action::Login => dispatch_login(app), + Action::LoginWith(method_id) => dispatch_login_with(app, Some(method_id)), Action::CancelLogin => dispatch_cancel_login(app), Action::SubmitAuthCode(code) => dispatch_submit_auth_code(app, code), Action::BeginPlatformKeyEntry(target) => dispatch_begin_platform_key_entry(app, target), diff --git a/crates/codegen/kigi-tui/src/app/dispatch/tests/auth.rs b/crates/codegen/kigi-tui/src/app/dispatch/tests/auth.rs index e0a322f..5ab7c2f 100644 --- a/crates/codegen/kigi-tui/src/app/dispatch/tests/auth.rs +++ b/crates/codegen/kigi-tui/src/app/dispatch/tests/auth.rs @@ -435,3 +435,74 @@ fn auth_complete_preserves_show_resolved_model_when_absent() { assert!(!app.show_resolved_model); } + +/// `LoginWith` must authenticate with EXACTLY the chosen method and adopt +/// its label — even when a different method was resolved earlier. +/// Regression: the picker collapsed every row to the id-less `Login`, +/// which resolved to the first interactive method (kimi-code), so the +/// Grok/Claude/Copilot/Codex rows all opened the Kimi device flow. +#[test] +fn login_with_routes_to_the_chosen_method() { + let mut app = test_app(); + app.auth_state = AuthState::Pending { error: None }; + app.auth_methods = kigi_shell::agent::auth_method::build_auth_methods( + kigi_shell::agent::auth_method::AuthMethodsBuildInputs { + has_external_api_key: false, + has_cached_token: false, + login_label: None, + }, + ) + .methods; + // A previous login resolved kimi-code; the explicit choice must win. + app.login_method_id = Some(acp::AuthMethodId::new("kimi-code")); + app.login_label = Some("Kimi Code".into()); + + let effects = dispatch( + Action::LoginWith(acp::AuthMethodId::new("claude-pro-max")), + &mut app, + ); + + let authenticated_with = effects.iter().find_map(|e| match e { + Effect::Authenticate { method_id, .. } => Some(method_id.0.to_string()), + _ => None, + }); + assert_eq!( + authenticated_with.as_deref(), + Some("claude-pro-max"), + "the chosen provider's own method must ride the Authenticate effect" + ); + assert_eq!( + app.login_method_id.as_ref().map(|id| id.0.as_ref()), + Some("claude-pro-max") + ); + assert_eq!( + app.login_label.as_deref(), + Some("Claude Pro/Max (subscription)"), + "the picker label must follow the chosen method" + ); + assert!(matches!(app.auth_state, AuthState::Authenticating { .. })); +} + +/// A `LoginWith` id that is not among the shell-advertised methods fails +/// closed with an error on the picker — silently falling back to the +/// first method is exactly the routing bug this guards against. +#[test] +fn login_with_unknown_method_fails_closed() { + let mut app = test_app(); + app.auth_state = AuthState::Pending { error: None }; + + let effects = dispatch( + Action::LoginWith(acp::AuthMethodId::new("no-such-provider")), + &mut app, + ); + + assert!(effects.is_empty(), "must not start any auth flow"); + assert!( + matches!( + &app.auth_state, + AuthState::Pending { error: Some(e) } if e.contains("no-such-provider") + ), + "must surface the unknown method, got {:?}", + app.auth_state + ); +}