fix(review): custom provider login comments, doc placement, subagent scheme test

The subagent auth-scheme lookup is now a function with a test. The failed-login
span carries the same method id as the successful one. README order matches the
code: the provider is saved, then the key is checked.
This commit is contained in:
雷电芽衣 committed 2026-09-29 16:50:03 -04:00
1 parent c4ef8a3779
commit c9855c4bb6
14 files changed
+77 -52

No files matched your search

+2 -2
View File
@@ -155,8 +155,8 @@ Anything that speaks the OpenAI Chat Completions API or the Anthropic Messages
API works: a gateway, a proxy, Ollama, LM Studio, vLLM. In `kigi`, open `/login` API works: a gateway, a proxy, Ollama, LM Studio, vLLM. In `kigi`, open `/login`
(or the welcome login screen) and pick **Custom provider (OpenAI compatible)** (or the welcome login screen) and pick **Custom provider (OpenAI compatible)**
or **(Anthropic compatible)**, then enter a base URL, a name, and an API key. or **(Anthropic compatible)**, then enter a base URL, a name, and an API key.
Kigi checks the key against Kigi saves the provider,
`GET {base}/models`, saves the provider, and `/model` lists what the endpoint checks the key against `GET {base}/models`, and `/model` lists what the endpoint
serves as `name/model`. serves as `name/model`.
```toml ```toml
+1 -1
View File
@@ -165,7 +165,7 @@ pub fn normalize_base_url(raw: &str) -> Result<String, CustomProviderError> {
Ok(url.to_owned()) Ok(url.to_owned())
} }
/// A valid provider name from a base URL's host, for a login default. /// A valid provider name derived from a host.
pub fn default_name(base_url: &str) -> String { pub fn default_name(base_url: &str) -> String {
let host = base_url let host = base_url
.split("://") .split("://")
@@ -599,7 +599,7 @@ mod tests {
); );
} }
/// `custom:<name>` is an API-key login; the picker rows are not loginable. /// `custom:<name>` logs in; picker rows cannot.
#[test] #[test]
fn custom_provider_method_ids_classify_as_api_key() { fn custom_provider_method_ids_classify_as_api_key() {
let kind = AuthMethodKind::from_id(&acp::AuthMethodId::new("custom:my-proxy")); let kind = AuthMethodKind::from_id(&acp::AuthMethodId::new("custom:my-proxy"));
@@ -3960,8 +3960,7 @@ pub fn resolve_model_auth_facts(model_id: &str) -> ModelAuthFacts {
}, },
}) })
} }
/// Auth facts of a catalog entry the caller already holds. A fetched entry /// Auth facts of a held live-catalog entry.
/// exists only in the live catalog, never in a disk reload.
pub(crate) fn auth_facts_of(entry: &ModelEntry) -> ModelAuthFacts { pub(crate) fn auth_facts_of(entry: &ModelEntry) -> ModelAuthFacts {
ModelAuthFacts { ModelAuthFacts {
byok: if entry.has_own_credentials() { byok: if entry.has_own_credentials() {
@@ -5286,7 +5285,7 @@ reasoning_effort = "low"
auth_scheme: Default::default(), auth_scheme: Default::default(),
} }
} }
/// A fetched entry's scheme and BYOK status come from the entry itself. /// Scheme and BYOK status come from the entry.
#[test] #[test]
fn auth_facts_come_from_the_held_entry() { fn auth_facts_come_from_the_held_entry() {
let mut model = test_model_entry("claude-x", "https://gw.example/v1", None, None, None); let mut model = test_model_entry("claude-x", "https://gw.example/v1", None, None, None);
@@ -9,7 +9,7 @@ const PICKER_PREFIX: &str = "custom-";
const LOGIN_PREFIX: &str = "custom:"; const LOGIN_PREFIX: &str = "custom:";
const ALL_APIS: [CustomApi; 2] = [CustomApi::OpenAi, CustomApi::Anthropic]; const ALL_APIS: [CustomApi; 2] = [CustomApi::OpenAi, CustomApi::Anthropic];
/// The picker row that starts the custom provider flow for one wire. /// The picker row for one wire.
pub fn picker_method(api: CustomApi) -> acp::AuthMethod { pub fn picker_method(api: CustomApi) -> acp::AuthMethod {
acp::AuthMethod::Agent( acp::AuthMethod::Agent(
acp::AuthMethodAgent::new( acp::AuthMethodAgent::new(
@@ -41,7 +41,7 @@ pub fn login_provider_name(id: &str) -> Option<&str> {
id.strip_prefix(LOGIN_PREFIX) id.strip_prefix(LOGIN_PREFIX)
} }
/// Probes the listing endpoint with the key; the error text never holds the key. /// Probes the listing endpoint; errors never hold the key.
pub(crate) async fn validate_key(credentialed: &CredentialedProvider) -> Result<(), String> { pub(crate) async fn validate_key(credentialed: &CredentialedProvider) -> Result<(), String> {
let provider = &credentialed.provider; let provider = &credentialed.provider;
let url = provider.models_url(); let url = provider.models_url();
@@ -553,7 +553,7 @@ impl MvpAgent {
tracing::warn!(provider = name, %message, "custom provider login failed"); tracing::warn!(provider = name, %message, "custom provider login failed");
emit_login_span( emit_login_span(
false, false,
"custom-provider", method_id.0.as_ref(),
None, None,
Some("custom_provider_invalid_or_missing"), Some("custom_provider_invalid_or_missing"),
); );
@@ -561,7 +561,7 @@ impl MvpAgent {
} }
Ok(self.finish_api_key_login(method_id).await) Ok(self.finish_api_key_login(method_id).await)
} }
/// Shared tail of an API-key login: reload config, refresh the catalog. /// Shared tail of an API-key login.
async fn finish_api_key_login(&self, method_id: acp::AuthMethodId) -> AuthenticateResponse { async fn finish_api_key_login(&self, method_id: acp::AuthMethodId) -> AuthenticateResponse {
// Rebuild the catalog from the on-disk config: the rebuild freshly // Rebuild the catalog from the on-disk config: the rebuild freshly
// resolves platform keys (env > auth.json > config), so the key just // resolves platform keys (env > auth.json > config), so the key just
@@ -879,6 +879,20 @@ fn log_subagent_model_resolution(
)), )),
); );
} }
/// The parent model's auth scheme: its live catalog entry, else a disk lookup.
fn parent_auth_scheme(
available: &indexmap::IndexMap<String, crate::agent::config::ModelEntry>,
catalog_id: &acp::ModelId,
slug: &str,
) -> kigi_sampler::AuthScheme {
crate::agent::models::entry_for_slug(available, Some(catalog_id.0.as_ref()), slug)
.map(|entry| crate::agent::config::auth_facts_of(entry).auth_scheme)
.or_else(|| {
crate::agent::config::try_resolve_model_credentials(slug, None).map(|r| r.auth_scheme)
})
.unwrap_or_default()
}
/// Read the parent session's actual current sampling config. /// Read the parent session's actual current sampling config.
/// ///
/// Prefers the live state from `ChatStateHandle` (authoritative). Falls back /// Prefers the live state from `ChatStateHandle` (authoritative). Falls back
@@ -897,17 +911,7 @@ async fn read_parent_sampling_config(
creds.alpha_test_key.as_deref(), creds.alpha_test_key.as_deref(),
&cfg.base_url, &cfg.base_url,
); );
let auth_scheme = crate::agent::models::entry_for_slug( let auth_scheme = parent_auth_scheme(&ctx.available_models, &ctx.model_id, &cfg.model);
&ctx.available_models,
Some(ctx.model_id.0.as_ref()),
&cfg.model,
)
.map(|entry| crate::agent::config::auth_facts_of(entry).auth_scheme)
.or_else(|| {
crate::agent::config::try_resolve_model_credentials(&cfg.model, None)
.map(|r| r.auth_scheme)
})
.unwrap_or_default();
// Claude Pro/Max OAuth Messages adaptation inherits from the parent // Claude Pro/Max OAuth Messages adaptation inherits from the parent
// model's platform (claude-pro-max → true); every other platform, // model's platform (claude-pro-max → true); every other platform,
// and BYOK, → false, so the API-key paths stay byte-identical. // and BYOK, → false, so the API-key paths stay byte-identical.
@@ -2667,3 +2671,32 @@ pub(crate) fn reconcile_orphaned_subagents(
} }
#[cfg(test)] #[cfg(test)]
mod tests; mod tests;
#[cfg(test)]
mod parent_auth_scheme_tests {
use super::*;
/// A fetched Messages model exists only in the live catalog.
#[test]
fn a_fetched_entry_gives_its_own_scheme_to_the_child() {
let wire: kigi_models::WireModel =
serde_json::from_value(serde_json::json!({ "id": "claude-x" })).unwrap();
let cfg = crate::agent::models_fetch::wire_model_to_entry(
"gw/claude-x".into(),
kigi_models::PlatformWireApi::Messages,
kigi_models::PlatformKeyHeader::XApiKey,
None,
true,
wire,
"https://gw.example/v1",
);
let available = indexmap::IndexMap::from([(
"gw/claude-x".to_owned(),
crate::agent::config::ModelEntry::from_config_entry(&cfg),
)]);
let scheme = parent_auth_scheme(&available, &acp::ModelId::new("gw/claude-x"), "claude-x");
assert_eq!(scheme, kigi_sampler::AuthScheme::XApiKey);
}
}
+3 -4
View File
@@ -570,9 +570,9 @@ pub enum Action {
/// under the platform-id scope, then authenticate with the platform's /// under the platform-id scope, then authenticate with the platform's
/// method id. The key must never be logged. /// method id. The key must never be logged.
SubmitPlatformApiKey(String), SubmitPlatformApiKey(String),
/// A custom provider row was picked: start the URL, name, key steps. /// Custom provider row picked: open the URL step.
BeginCustomProviderEntry(kigi_shell::models::custom::CustomApi), BeginCustomProviderEntry(kigi_shell::models::custom::CustomApi),
/// Enter in a custom provider step, with the trimmed input. /// Enter in a custom provider step.
SubmitCustomProviderInput(String), SubmitCustomProviderInput(String),
/// Copy the auth URL to the clipboard during authentication. /// Copy the auth URL to the clipboard during authentication.
CopyAuthUrl, CopyAuthUrl,
@@ -1618,8 +1618,7 @@ pub enum Effect {
target: crate::app::app_view::PlatformLogin, target: crate::app::app_view::PlatformLogin,
key: String, key: String,
}, },
/// Save a custom provider (config.toml and auth.json), then send /// Saves the provider, then authenticates; never log the key.
/// AuthenticateRequest with `custom:<name>`. SECURITY: never log the key.
PersistCustomProviderAndAuthenticate { PersistCustomProviderAndAuthenticate {
request_seq: u64, request_seq: u64,
provider: kigi_shell::models::custom::CustomProvider, provider: kigi_shell::models::custom::CustomProvider,
+1 -1
View File
@@ -262,7 +262,7 @@ pub enum AuthMode {
/// Open-platform API-key entry: paste box for a Moonshot key selected /// Open-platform API-key entry: paste box for a Moonshot key selected
/// from the welcome login picker. Esc returns to the picker (no quit). /// from the welcome login picker. Esc returns to the picker (no quit).
ApiKeyEntry(PlatformLogin), ApiKeyEntry(PlatformLogin),
/// Custom provider login: one field per step; Esc returns to the picker. /// Custom provider login, one field per step.
CustomProviderEntry(crate::app::custom_entry::CustomEntryStep), CustomProviderEntry(crate::app::custom_entry::CustomEntryStep),
} }
/// API-key platform login target, selected from the welcome picker. Wraps a /// API-key platform login target, selected from the welcome picker. Wraps a
@@ -1,4 +1,4 @@
//! The base URL, name, and key steps of the custom provider login. //! Custom provider login steps: base URL, name, key.
use kigi_shell::models::custom::{ use kigi_shell::models::custom::{
CustomApi, CustomProvider, default_name, normalize_base_url, validate_name, CustomApi, CustomProvider, default_name, normalize_base_url, validate_name,
@@ -26,7 +26,7 @@ impl CustomEntryStep {
} }
} }
/// Only the key is a secret; URL and name show as typed. /// Only the key is masked.
pub fn masks_input(self) -> bool { pub fn masks_input(self) -> bool {
self.field == CustomEntryField::Key self.field == CustomEntryField::Key
} }
@@ -43,9 +43,9 @@ impl CustomEntryStep {
/// What one Enter press did. /// What one Enter press did.
#[derive(Debug, PartialEq, Eq)] #[derive(Debug, PartialEq, Eq)]
pub enum CustomEntryOutcome { pub enum CustomEntryOutcome {
/// The input stays; `CustomEntryDraft::error` says why, if it does. /// The input stays; the draft error says why.
Stay, Stay,
/// Move to the next field with this text in the box. /// Next field, with this text in the box.
Next { Next {
field: CustomEntryField, field: CustomEntryField,
prefill: String, prefill: String,
@@ -326,7 +326,7 @@ pub(super) fn dispatch_begin_platform_key_entry(
vec![] vec![]
} }
/// A custom provider row was selected: open the first step (base URL). /// Custom provider row selected: open the URL step.
pub(super) fn dispatch_begin_custom_provider_entry( pub(super) fn dispatch_begin_custom_provider_entry(
app: &mut AppView, app: &mut AppView,
api: kigi_shell::models::custom::CustomApi, api: kigi_shell::models::custom::CustomApi,
@@ -344,7 +344,7 @@ pub(super) fn dispatch_begin_custom_provider_entry(
vec![] vec![]
} }
/// Enter in a custom provider step: advance, stay with a reason, or save. /// Enter in a custom provider step.
pub(super) fn dispatch_submit_custom_provider_input( pub(super) fn dispatch_submit_custom_provider_input(
app: &mut AppView, app: &mut AppView,
input: String, input: String,
@@ -582,8 +582,7 @@ fn custom_field(app: &AppView) -> crate::app::custom_entry::CustomEntryField {
} }
} }
/// The custom provider flow: URL, then name (prefilled from the host), then /// URL, name, key; the last Enter emits one persist effect.
/// key; the last Enter emits ONE persist+authenticate effect for the provider.
#[test] #[test]
fn custom_provider_steps_end_in_one_persist_effect() { fn custom_provider_steps_end_in_one_persist_effect() {
use crate::app::custom_entry::CustomEntryField; use crate::app::custom_entry::CustomEntryField;
@@ -649,7 +648,7 @@ fn custom_provider_steps_end_in_one_persist_effect() {
assert!(app.auth_code_input.is_empty()); assert!(app.auth_code_input.is_empty());
} }
/// A refused field keeps the user on it and does not emit an effect. /// A refused field keeps the user on it.
#[test] #[test]
fn custom_provider_refusals_stay_on_the_field() { fn custom_provider_refusals_stay_on_the_field() {
use crate::app::custom_entry::CustomEntryField; use crate::app::custom_entry::CustomEntryField;
@@ -687,7 +686,7 @@ fn custom_provider_refusals_stay_on_the_field() {
assert!(app.custom_entry.error.is_some()); assert!(app.custom_entry.error.is_some());
} }
/// Esc at any step returns to the picker and drops the draft. /// Esc returns to the picker and drops the draft.
#[test] #[test]
fn cancel_custom_provider_entry_returns_to_picker() { fn cancel_custom_provider_entry_returns_to_picker() {
use kigi_shell::models::custom::CustomApi; use kigi_shell::models::custom::CustomApi;
@@ -1294,7 +1294,7 @@ fn render_welcome_authenticating(
} }
AuthMode::ApiKeyEntry(target) => { AuthMode::ApiKeyEntry(target) => {
// Platform API-key paste box: the key comes from the platform console. // Platform API-key paste box.
let instruction = format!( let instruction = format!(
"Paste your {} API key (from {})", "Paste your {} API key (from {})",
target.vendor(), target.vendor(),
@@ -1938,8 +1938,7 @@ pub(crate) fn render_session_picker(
) )
} }
/// Render the auth token input box (loopback mode). /// One text field under the logo.
/// One text field under the logo: instruction, optional error, input box, hints.
struct EntryBox<'a> { struct EntryBox<'a> {
instruction: &'a str, instruction: &'a str,
error: Option<&'a str>, error: Option<&'a str>,
@@ -2035,6 +2034,7 @@ fn render_entry_box(
Paragraph::new(hints).render(hint_area, buf); Paragraph::new(hints).render(hint_area, buf);
} }
/// Render the auth token input box (loopback mode).
fn render_auth_input_box( fn render_auth_input_box(
area: Rect, area: Rect,
buf: &mut Buffer, buf: &mut Buffer,
@@ -2393,8 +2393,6 @@ mod tests {
assert!(!text.contains("Moonshot"), "{text}"); assert!(!text.contains("Moonshot"), "{text}");
} }
/// The Moonshot API-key entry arm renders the platform copy, the paste
/// box, and the esc-back hint — and no OAuth-URL affordances.
fn render_custom_entry( fn render_custom_entry(
step: crate::app::custom_entry::CustomEntryStep, step: crate::app::custom_entry::CustomEntryStep,
draft: &crate::app::custom_entry::CustomEntryDraft, draft: &crate::app::custom_entry::CustomEntryDraft,
@@ -2417,7 +2415,7 @@ mod tests {
buffer_text(&buf) buffer_text(&buf)
} }
/// The URL and name steps show typed text; only the key step masks it. /// Only the key step masks its input.
#[test] #[test]
fn custom_entry_arm_masks_only_the_key() { fn custom_entry_arm_masks_only_the_key() {
use crate::app::custom_entry::{CustomEntryDraft, CustomEntryField, CustomEntryStep}; use crate::app::custom_entry::{CustomEntryDraft, CustomEntryField, CustomEntryStep};
@@ -2447,7 +2445,7 @@ mod tests {
assert!(empty.contains("provider name"), "{empty}"); assert!(empty.contains("provider name"), "{empty}");
} }
/// A refusal shows under the instruction so the user sees what to fix. /// A refusal shows under the instruction.
#[test] #[test]
fn custom_entry_arm_shows_the_refusal() { fn custom_entry_arm_shows_the_refusal() {
use crate::app::custom_entry::{CustomEntryDraft, CustomEntryStep}; use crate::app::custom_entry::{CustomEntryDraft, CustomEntryStep};
@@ -2460,6 +2458,8 @@ mod tests {
assert!(text.contains("must start with http"), "{text}"); assert!(text.contains("must start with http"), "{text}");
} }
/// The Moonshot API-key entry arm renders the platform copy, the paste
/// box, and the esc-back hint — and no OAuth-URL affordances.
#[test] #[test]
fn api_key_entry_arm_shows_platform_copy_and_paste_box() { fn api_key_entry_arm_shows_platform_copy_and_paste_box() {
let area = Rect::new(0, 0, 80, 40); let area = Rect::new(0, 0, 80, 40);
@@ -57,10 +57,7 @@ async fn run_turn(content: &ContentController) -> PtyHarness {
harness harness
} }
/// A declared OpenAI-compatible provider supplies the catalog and the turn: /// An OpenAI provider lists models and uses its own key.
/// the request goes to `/chat/completions` for the fetched model with the
/// provider's own key. The house key, which is set in this environment on the
/// same loopback URL as the session's coding endpoint, never rides it.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[ignore] #[ignore]
async fn custom_openai_provider_lists_models_and_uses_its_own_key() { async fn custom_openai_provider_lists_models_and_uses_its_own_key() {
@@ -94,8 +91,7 @@ async fn custom_openai_provider_lists_models_and_uses_its_own_key() {
harness.quit().expect("clean quit"); harness.quit().expect("clean quit");
} }
/// An Anthropic-compatible provider is reached on `/messages` with /// An Anthropic provider uses `/messages` with `x-api-key`.
/// `x-api-key` and `anthropic-version`, and no Bearer header.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[ignore] #[ignore]
async fn custom_anthropic_provider_uses_messages_and_x_api_key() { async fn custom_anthropic_provider_uses_messages_and_x_api_key() {
@@ -128,8 +124,7 @@ async fn custom_anthropic_provider_uses_messages_and_x_api_key() {
harness.quit().expect("clean quit"); harness.quit().expect("clean quit");
} }
/// A saved provider is a complete login: with no house key and no session, the /// A saved provider alone is a complete login.
/// pager starts authenticated and the turn reaches the provider.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
#[ignore] #[ignore]
async fn custom_provider_alone_authenticates_at_startup() { async fn custom_provider_alone_authenticates_at_startup() {