fix(tui): route each OAuth picker row to its own provider, not kimi.com
Root cause: PendingMenuItem::Login carried only a label, so every provider row collapsed to the id-less Action::Login, and dispatch_login resolved the FIRST advertised interactive method — the Kimi device flow. Selecting Grok/Claude/Copilot/Codex all opened kimi.com. (The shell side was already correct: authenticate() dispatches each method id to its own OAuth flow.) - PendingMenuItem::Login now carries the advertised method id (None only on the no-interactive-method fallback row). - New Action::LoginWith(AuthMethodId); the picker dispatches it with the row's own id. Action::Login keeps its meaning (resolved/default method) for /login, auto-login, and re-auth. - dispatch_login_with resolves the id against the advertised methods and FAILS CLOSED on an unknown id — no silent first-method fallback — then adopts the method's label and start mode. Tests: picker rows pinned to their method ids; LoginWith(claude-pro-max) must authenticate with claude-pro-max even when kimi-code was previously resolved; unknown ids surface an error and start nothing. Verified: kigi-tui 6867 tests green, clippy clean.
This commit is contained in:
@@ -562,6 +562,11 @@ pub enum Action {
|
|||||||
SwitchAccount,
|
SwitchAccount,
|
||||||
/// User pressed login on the welcome screen.
|
/// User pressed login on the welcome screen.
|
||||||
Login,
|
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
|
/// 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.
|
/// (`/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
|
/// Distinct from `Quit`: abandoning a mid-session re-auth must not exit
|
||||||
|
|||||||
@@ -298,8 +298,12 @@ impl PlatformLogin {
|
|||||||
/// One row of the unauthenticated welcome menu (the login picker).
|
/// One row of the unauthenticated welcome menu (the login picker).
|
||||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||||
pub enum PendingMenuItem {
|
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 {
|
Login {
|
||||||
|
method_id: Option<acp::AuthMethodId>,
|
||||||
label: String,
|
label: String,
|
||||||
},
|
},
|
||||||
/// Open-platform API-key entry.
|
/// Open-platform API-key entry.
|
||||||
@@ -321,7 +325,7 @@ impl PendingMenuItem {
|
|||||||
}
|
}
|
||||||
pub fn label(&self) -> &str {
|
pub fn label(&self) -> &str {
|
||||||
match self {
|
match self {
|
||||||
Self::Login { label } | Self::ApiKey { label, .. } => label,
|
Self::Login { label, .. } | Self::ApiKey { label, .. } => label,
|
||||||
Self::Quit => "Quit",
|
Self::Quit => "Quit",
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -347,12 +351,14 @@ pub fn pending_menu_items(
|
|||||||
});
|
});
|
||||||
} else if AuthMethodKind::from_id(method.id()).needs_interactive_login() {
|
} else if AuthMethodKind::from_id(method.id()).needs_interactive_login() {
|
||||||
items.push(PendingMenuItem::Login {
|
items.push(PendingMenuItem::Login {
|
||||||
|
method_id: Some(method.id().clone()),
|
||||||
label: format!("{} (OAuth)", method.name()),
|
label: format!("{} (OAuth)", method.name()),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if items.is_empty() {
|
if items.is_empty() {
|
||||||
items.push(PendingMenuItem::Login {
|
items.push(PendingMenuItem::Login {
|
||||||
|
method_id: None,
|
||||||
label: format!("Login with {}", login_label.unwrap_or("kimi.com")),
|
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.
|
/// Rows come from [`pending_menu_items`]: interactive methods + Quit.
|
||||||
fn dispatch_pending_menu_action(items: &[PendingMenuItem], index: usize) -> InputOutcome {
|
fn dispatch_pending_menu_action(items: &[PendingMenuItem], index: usize) -> InputOutcome {
|
||||||
match items.get(index) {
|
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, .. }) => {
|
Some(PendingMenuItem::ApiKey { target, .. }) => {
|
||||||
InputOutcome::Action(Action::BeginPlatformKeyEntry(*target))
|
InputOutcome::Action(Action::BeginPlatformKeyEntry(*target))
|
||||||
}
|
}
|
||||||
@@ -6916,34 +6930,41 @@ pub(crate) mod tests {
|
|||||||
)
|
)
|
||||||
.methods
|
.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]
|
#[test]
|
||||||
fn pending_menu_items_lists_interactive_methods_plus_quit() {
|
fn pending_menu_items_lists_interactive_methods_plus_quit() {
|
||||||
let items = pending_menu_items(&fresh_user_auth_methods(), None);
|
let items = pending_menu_items(&fresh_user_auth_methods(), None);
|
||||||
assert_eq!(items.len(), 30, "29 login rows + Quit, got {items:?}");
|
assert_eq!(items.len(), 30, "29 login rows + Quit, got {items:?}");
|
||||||
assert!(
|
assert_login_row(&items[0], "kimi-code", "Kimi Code (OAuth)");
|
||||||
matches!(&items[0], PendingMenuItem::Login { label } if label == "Kimi Code (OAuth)"),
|
assert_login_row(&items[1], "xai-grok", "xAI Grok (subscription) (OAuth)");
|
||||||
"row 0 must be the OAuth login, got {:?}",
|
assert_login_row(
|
||||||
items[0]
|
&items[2],
|
||||||
|
"claude-pro-max",
|
||||||
|
"Claude Pro/Max (subscription) (OAuth)",
|
||||||
);
|
);
|
||||||
assert!(
|
assert_login_row(
|
||||||
matches!(&items[1], PendingMenuItem::Login { label } if label == "xAI Grok (subscription) (OAuth)"),
|
&items[3],
|
||||||
"row 1 must be the xai-grok OAuth login (interactive, after kimi-code), got {:?}",
|
"github-copilot",
|
||||||
items[1]
|
"GitHub Copilot (subscription) (OAuth)",
|
||||||
);
|
);
|
||||||
assert!(
|
assert_login_row(
|
||||||
matches!(&items[2], PendingMenuItem::Login { label } if label == "Claude Pro/Max (subscription) (OAuth)"),
|
&items[4],
|
||||||
"row 2 must be the claude-pro-max OAuth login (after xai-grok), got {:?}",
|
"openai-codex",
|
||||||
items[2]
|
"ChatGPT Plus/Pro (Codex) (OAuth)",
|
||||||
);
|
|
||||||
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_eq!(
|
assert_eq!(
|
||||||
items[5],
|
items[5],
|
||||||
@@ -7168,9 +7189,39 @@ pub(crate) mod tests {
|
|||||||
"Enter on the moonshot-cn row must open its key entry, got {outcome:?}"
|
"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
|
// '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));
|
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]
|
#[test]
|
||||||
fn welcome_api_key_entry_esc_returns_to_picker() {
|
fn welcome_api_key_entry_esc_returns_to_picker() {
|
||||||
|
|||||||
@@ -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`
|
/// restored once auth completes or is cancelled. Without this, `/login`
|
||||||
/// with an external auth provider configured appeared to do nothing.
|
/// with an external auth provider configured appeared to do nothing.
|
||||||
pub(super) fn dispatch_login(app: &mut AppView) -> Vec<Effect> {
|
pub(super) fn dispatch_login(app: &mut AppView) -> Vec<Effect> {
|
||||||
|
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<agent_client_protocol::AuthMethodId>,
|
||||||
|
) -> Vec<Effect> {
|
||||||
|
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);
|
ensure_login_method(app);
|
||||||
let Some(method_id) = app.login_method_id.clone() else {
|
let Some(method_id) = app.login_method_id.clone() else {
|
||||||
app.auth_state = AuthState::Pending {
|
app.auth_state = AuthState::Pending {
|
||||||
|
|||||||
@@ -1,8 +1,8 @@
|
|||||||
//! Top-level action router: maps actions and action results to handlers.
|
//! Top-level action router: maps actions and action results to handlers.
|
||||||
use super::auth::{
|
use super::auth::{
|
||||||
dispatch_begin_platform_key_entry, dispatch_cancel_login, dispatch_cancel_platform_key_entry,
|
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_login, dispatch_login_with, dispatch_logout, dispatch_submit_auth_code,
|
||||||
dispatch_switch_account,
|
dispatch_submit_platform_api_key, dispatch_switch_account,
|
||||||
};
|
};
|
||||||
use super::ctx::{
|
use super::ctx::{
|
||||||
active_agent_session_id, get_active_agent_mut, navigate_clearing_selection,
|
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<Effect> {
|
|||||||
vec![]
|
vec![]
|
||||||
}
|
}
|
||||||
Action::Login => dispatch_login(app),
|
Action::Login => dispatch_login(app),
|
||||||
|
Action::LoginWith(method_id) => dispatch_login_with(app, Some(method_id)),
|
||||||
Action::CancelLogin => dispatch_cancel_login(app),
|
Action::CancelLogin => dispatch_cancel_login(app),
|
||||||
Action::SubmitAuthCode(code) => dispatch_submit_auth_code(app, code),
|
Action::SubmitAuthCode(code) => dispatch_submit_auth_code(app, code),
|
||||||
Action::BeginPlatformKeyEntry(target) => dispatch_begin_platform_key_entry(app, target),
|
Action::BeginPlatformKeyEntry(target) => dispatch_begin_platform_key_entry(app, target),
|
||||||
|
|||||||
@@ -435,3 +435,74 @@ fn auth_complete_preserves_show_resolved_model_when_absent() {
|
|||||||
|
|
||||||
assert!(!app.show_resolved_model);
|
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
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user