From 6c379e30fddfe1d84836a50a96862e4709d6e7fd Mon Sep 17 00:00:00 2001 From: hanafish <1106510024@qq.com> Date: Tue, 4 Aug 2026 21:47:58 +0800 Subject: [PATCH] fix(key-vault): make Codex model discovery reliable --- .../CodexSetup.md | 40 +++++ src-tauri/Cargo.lock | 1 + src-tauri/crates/key-vault/Cargo.toml | 1 + .../key-vault/src/key_store/service/keys.rs | 10 +- .../src/key_store/service/persistence.rs | 6 +- .../crates/key-vault/src/key_store/store.rs | 5 +- .../key-vault/src/key_store/tests/tests.rs | 82 ++++++++++ .../crates/key-vault/src/key_store/types.rs | 16 +- .../key-vault/src/providers/codex/mod.rs | 136 +++++++++++++++-- .../forkModelFallback.test.ts | 8 + .../TeamCollaboration/forkModelFallback.ts | 16 +- src/hooks/housekeeper/useHousekeeperConfig.ts | 6 +- .../models/useModelAccountLookup.test.ts | 21 +++ src/hooks/models/useModelAccountLookup.ts | 17 ++- .../components/cliManagedConfigUtils.test.ts | 17 ++- .../components/cliManagedConfigUtils.ts | 10 +- .../hooks/useWorkflowModelOptions.ts | 12 +- .../useUnifiedModelPaletteItems.test.ts | 74 +++++++++ .../useUnifiedModelPaletteItems.ts | 144 +++++++++++------- .../KeyVault/components/AgentSetupRouter.tsx | 4 + .../variants/KeyVault/components/ApiSetup.tsx | 1 + .../KeyVault/components/setup/CodexSetup.tsx | 71 ++++++++- .../components/setup/__tests__/TEST_CASES.md | 51 +++++++ .../KeyVault/components/setup/types.ts | 2 + .../credentialDetectionState.test.ts | 126 +++++++++++++++ .../hooks/credentialDetectionState.ts | 93 +++++++++++ .../KeyVault/hooks/keyHelpers.test.ts | 38 ++++- .../variants/KeyVault/hooks/keyHelpers.ts | 16 +- .../variants/KeyVault/hooks/useApiSetup.ts | 17 ++- .../hooks/useApiSetupTokenDetection.ts | 131 +++++++++++++--- 30 files changed, 1041 insertions(+), 131 deletions(-) create mode 100644 docs/frontend-ui-audit-2026-08-04/CodexSetup.md create mode 100644 src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.test.ts create mode 100644 src/scaffold/WizardSystem/variants/KeyVault/components/setup/__tests__/TEST_CASES.md create mode 100644 src/scaffold/WizardSystem/variants/KeyVault/hooks/__tests__/credentialDetectionState.test.ts create mode 100644 src/scaffold/WizardSystem/variants/KeyVault/hooks/credentialDetectionState.ts diff --git a/docs/frontend-ui-audit-2026-08-04/CodexSetup.md b/docs/frontend-ui-audit-2026-08-04/CodexSetup.md new file mode 100644 index 000000000..a3e96d872 --- /dev/null +++ b/docs/frontend-ui-audit-2026-08-04/CodexSetup.md @@ -0,0 +1,40 @@ +# Frontend UI Audit — CodexSetup + +**File:** `src/scaffold/WizardSystem/variants/KeyVault/components/setup/CodexSetup.tsx` (269 LOC) +**Date:** 2026-08-04 +**Auditor:** Codex session + +## D1 — Raw HTML vs Design System + +| Line | Element | Verdict | Reason | Suggested change | +| ---- | ----------- | ------- | ---------------------------------------------------------------------------------------------- | ---------------- | +| — | No findings | — | The changed interaction uses the existing `Button` and `InlineAlert` design-system components. | — | + +## D2 — Arbitrary Tailwind Value vs Token + +| Line | Value | Verdict | Reason | Suggested change | +| ---- | ----------- | ------- | -------------------------------------------------------------------------------------- | ---------------- | +| — | No findings | — | The feedback change introduces no arbitrary CSS variables, colors, or Tailwind values. | — | + +## D3 — Hardcoded Sizes / Colors + +| Line | Value | Verdict | Reason | Suggested change | +| ---- | ----------- | ------- | ------------------------------------------------------------------- | ---------------- | +| — | No findings | — | The feedback change introduces no hardcoded size or color literals. | — | + +## D4 — Accessibility + +| Line | Element | Verdict | Reason | Suggested change | +| ---- | ------------------------- | ---------------- | ----------------------------------------------------------------------------------------------------------------------------- | ---------------- | +| 178 | Detect `Button` | keep with reason | The design-system button has a visible localized label, native keyboard semantics, and is disabled while detection is active. | — | +| 230 | Detection feedback region | keep with reason | Progress and success use a polite status region; failures use an assertive alert region and retain a dismiss control. | — | + +## D5 — Visual Patterns Observed + +- No new repeated visual pattern. The implementation reuses the existing inline-alert feedback pattern. + +## Summary + +- 0 fixes recommended +- 2 kept with documented reason +- 0 abstract candidates (>= 3 occurrences) diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index 8f06dc307..339df4443 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -3922,6 +3922,7 @@ dependencies = [ "dirs", "insta", "integrations", + "libc", "log", "nucleo-matcher", "orgtrack_core", diff --git a/src-tauri/crates/key-vault/Cargo.toml b/src-tauri/crates/key-vault/Cargo.toml index b6da901af..5f0b61f12 100644 --- a/src-tauri/crates/key-vault/Cargo.toml +++ b/src-tauri/crates/key-vault/Cargo.toml @@ -40,6 +40,7 @@ reqwest = { workspace = true } regex = "1.9.5" log = "0.4" tracing = "0.1.37" +libc = "0.2" uuid = { version = "1", features = ["v4"] } dirs = "6.0" sha2 = "0.10" diff --git a/src-tauri/crates/key-vault/src/key_store/service/keys.rs b/src-tauri/crates/key-vault/src/key_store/service/keys.rs index 927934173..9897d1b37 100644 --- a/src-tauri/crates/key-vault/src/key_store/service/keys.rs +++ b/src-tauri/crates/key-vault/src/key_store/service/keys.rs @@ -86,11 +86,7 @@ impl KeyService { /// Save or update a key pub fn save_key(&self, key: ModelKey) -> Result { - self.update_store(|store| { - let entry = key.clone(); - store.set(key); - entry - }) + self.update_store(|store| store.set(key)) } /// Record behaviorally-observed reasoning capability for `model` on key @@ -236,6 +232,10 @@ impl KeyService { if let Some(enabled) = enabled_models { entry.enabled_models = enabled; } + // A successful model refresh is authoritative. Removed models + // must not survive in enabled_models when callers omit that + // optional field, and caller-provided lists are normalized too. + entry.normalize_enabled_models(); if let Some(quota) = quota_info { entry.quota_info = Some(quota); } diff --git a/src-tauri/crates/key-vault/src/key_store/service/persistence.rs b/src-tauri/crates/key-vault/src/key_store/service/persistence.rs index 507bab19a..a6ff684a2 100644 --- a/src-tauri/crates/key-vault/src/key_store/service/persistence.rs +++ b/src-tauri/crates/key-vault/src/key_store/service/persistence.rs @@ -171,7 +171,11 @@ fn deserialize_key_store(contents: &str) -> Result(raw.clone()) { - Ok(key) => { + Ok(mut key) => { + // Older catalog refreshes could leave removed model ids in + // enabled_models. Normalize on hydration so every reader sees + // a consistent account even before the next persisted write. + key.normalize_enabled_models(); store.keys.insert(storage_id, key); } Err(error) => invalid_credentials.push(InvalidStoredCredential { diff --git a/src-tauri/crates/key-vault/src/key_store/store.rs b/src-tauri/crates/key-vault/src/key_store/store.rs index c9594b2e2..878e23f8b 100644 --- a/src-tauri/crates/key-vault/src/key_store/store.rs +++ b/src-tauri/crates/key-vault/src/key_store/store.rs @@ -65,10 +65,13 @@ impl KeyStore { } /// Save or update a key - pub fn set(&mut self, mut key: ModelKey) { + pub fn set(&mut self, mut key: ModelKey) -> ModelKey { + key.normalize_enabled_models(); key.updated_at = Utc::now(); + let saved = key.clone(); self.keys.insert(key.id.clone(), key); self.updated_at = Utc::now(); + saved } /// Delete key by agent type and optional ID diff --git a/src-tauri/crates/key-vault/src/key_store/tests/tests.rs b/src-tauri/crates/key-vault/src/key_store/tests/tests.rs index 9fc922235..22d731762 100644 --- a/src-tauri/crates/key-vault/src/key_store/tests/tests.rs +++ b/src-tauri/crates/key-vault/src/key_store/tests/tests.rs @@ -58,6 +58,86 @@ fn test_key_crud() { assert_eq!(empty.len(), 0); } +#[test] +fn test_save_key_normalizes_enabled_models_to_available_catalog() { + let temp_dir = tempdir().unwrap(); + let service = KeyService::new(Some(temp_dir.path().to_path_buf())); + + let mut key = ModelKey::new(ModelType::Codex); + key.available_models = vec!["gpt-5.5".to_string(), "gpt-5.4".to_string()]; + key.enabled_models = vec![ + "gpt-5.6-sol".to_string(), + "gpt-5.5".to_string(), + "gpt-5.5".to_string(), + ]; + + let saved = service.save_key(key).unwrap(); + + assert_eq!(saved.enabled_models, vec!["gpt-5.5".to_string()]); + assert_eq!( + service.get_key_by_id(&saved.id).unwrap().enabled_models, + vec!["gpt-5.5".to_string()] + ); + let persisted: serde_json::Value = serde_json::from_str( + &std::fs::read_to_string(temp_dir.path().join("credentials.json")).unwrap(), + ) + .unwrap(); + assert_eq!( + persisted["credentials"][&saved.id]["enabled_models"], + serde_json::json!(["gpt-5.5"]), + "serialized credentials must not retain unavailable enabled models" + ); +} + +#[test] +fn test_load_normalizes_legacy_stale_enabled_models() { + let temp_dir = tempdir().unwrap(); + let service = KeyService::new(Some(temp_dir.path().to_path_buf())); + + let mut key = ModelKey::new(ModelType::Codex); + let key_id = key.id.clone(); + key.available_models = vec!["gpt-5.5".to_string()]; + key.enabled_models = vec!["gpt-5.6-sol".to_string(), "gpt-5.5".to_string()]; + + let mut raw_store = KeyStore::default(); + raw_store.keys.insert(key_id.clone(), key); + std::fs::write( + temp_dir.path().join("credentials.json"), + serde_json::to_string_pretty(&raw_store).unwrap(), + ) + .unwrap(); + + let loaded = service.get_key_by_id(&key_id).unwrap(); + assert_eq!(loaded.enabled_models, vec!["gpt-5.5".to_string()]); +} + +#[test] +fn test_model_refresh_removes_enabled_models_missing_from_new_catalog() { + let temp_dir = tempdir().unwrap(); + let service = KeyService::new(Some(temp_dir.path().to_path_buf())); + + let mut key = ModelKey::new(ModelType::Codex); + key.available_models = vec!["gpt-5.6-sol".to_string(), "gpt-5.5".to_string()]; + key.enabled_models = key.available_models.clone(); + let saved = service.save_key(key).unwrap(); + + service + .update_key_health( + &saved.id, + HealthStatus::Valid, + None, + Some(vec!["gpt-5.5".to_string()]), + None, + None, + None, + ) + .unwrap(); + + let refreshed = service.get_key_by_id(&saved.id).unwrap(); + assert_eq!(refreshed.available_models, vec!["gpt-5.5".to_string()]); + assert_eq!(refreshed.enabled_models, vec!["gpt-5.5".to_string()]); +} + #[test] fn retired_gemini_cli_credentials_do_not_corrupt_the_vault() { let temp_dir = tempdir().unwrap(); @@ -1181,6 +1261,7 @@ fn test_cross_type_env_zenmux_as_claude_code_uses_anthropic_endpoint() { let mut zenmux_key = ModelKey::new(ModelType::ZenmuxApi); zenmux_key.api_key = Some("sk-zenmux-test123".to_string()); + zenmux_key.available_models = vec!["claude-sonnet-4-20250514".to_string()]; zenmux_key.enabled_models = vec!["claude-sonnet-4-20250514".to_string()]; let key_id = zenmux_key.id.clone(); service.save_key(zenmux_key).unwrap(); @@ -1235,6 +1316,7 @@ fn test_cross_type_env_atlascloud_as_claude_code_uses_anthropic_endpoint() { let mut atlas_key = ModelKey::new(ModelType::AtlascloudApi); atlas_key.api_key = Some("atlas-test-key".to_string()); + atlas_key.available_models = vec!["zai-org/glm-5.1".to_string()]; atlas_key.enabled_models = vec!["zai-org/glm-5.1".to_string()]; // The stored /v1 URL is OpenAI-protocol; the Anthropic export must // ignore it and use the bare host instead. diff --git a/src-tauri/crates/key-vault/src/key_store/types.rs b/src-tauri/crates/key-vault/src/key_store/types.rs index 84806c308..7f528795c 100644 --- a/src-tauri/crates/key-vault/src/key_store/types.rs +++ b/src-tauri/crates/key-vault/src/key_store/types.rs @@ -1,7 +1,7 @@ use chrono::{DateTime, NaiveDateTime, Utc}; use serde::{Deserialize, Deserializer, Serialize, Serializer}; // Deserializer is used by the flexible_datetime modules below -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use uuid::Uuid; // Custom serde for flexible datetime parsing (naive timestamps without timezone) @@ -544,6 +544,20 @@ impl ModelKey { } } + /// Restore the persisted model-selection invariant after catalog changes: + /// every enabled model must still exist in the provider's available-model + /// catalog. Preserve user ordering while also removing duplicate rows. + /// + /// This is deliberately owned by `ModelKey`, rather than individual UI or + /// runtime consumers, so old credentials and every write path converge on + /// the same representation. + pub fn normalize_enabled_models(&mut self) { + let available: HashSet<&str> = self.available_models.iter().map(String::as_str).collect(); + let mut seen = HashSet::new(); + self.enabled_models + .retain(|model| available.contains(model.as_str()) && seen.insert(model.clone())); + } + /// Mask sensitive data for display pub fn mask_api_key(&self) -> Option { self.api_key.as_ref().map(|key| { diff --git a/src-tauri/crates/key-vault/src/providers/codex/mod.rs b/src-tauri/crates/key-vault/src/providers/codex/mod.rs index e816a4f4c..397597917 100644 --- a/src-tauri/crates/key-vault/src/providers/codex/mod.rs +++ b/src-tauri/crates/key-vault/src/providers/codex/mod.rs @@ -13,7 +13,7 @@ use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; use std::path::PathBuf; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; -use tokio::process::Command; +use tokio::process::{Child, Command}; /// ChatGPT usage API endpoint const USAGE_API_URL: &str = "https://chatgpt.com/backend-api/wham/usage"; @@ -21,6 +21,7 @@ const CODEX_MODELS_API_URL: &str = "https://chatgpt.com/backend-api/codex/models const CODEX_MODELS_CLIENT_VERSION: &str = "0.124.0"; const CODEX_USER_AGENT: &str = "codex_cli_rs/0.124.0 (orgii, cli)"; const APP_SERVER_TIMEOUT_SECS: u64 = 10; +const APP_SERVER_SHUTDOWN_TIMEOUT_SECS: u64 = 2; #[derive(Debug, Deserialize)] struct CodexModelsResponse { @@ -750,12 +751,20 @@ async fn run_codex_app_server_rpc( .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()); + // The npm `codex` entry point forks the native Codex binary. Give the + // wrapper and every descendant a dedicated process group so timeout and + // normal completion can terminate the whole tree instead of orphaning the + // native app-server with our stdout/stderr pipes still open. + #[cfg(unix)] + child.process_group(0); + #[cfg(windows)] child.creation_flags(app_platform::CREATE_NO_WINDOW); let mut child = child .spawn() .map_err(|err| format!("Failed to start Codex app-server via {codex_binary}: {err}"))?; + let child_pid = child.id(); let mut stdin = child .stdin @@ -808,28 +817,90 @@ async fn run_codex_app_server_rpc( Err(_) => Err(format!("Codex app-server {operation} timed out")), }; - if let Err(err) = child.kill().await { - log::debug!( - "[CodexAppServer] Failed to kill Codex app-server after {}: {}", - operation, - err, - ); - } - let _ = child.wait().await; + terminate_codex_app_server_tree(&mut child, child_pid, operation).await; - if let Some(task) = stderr_task { - if let Ok(stderr_output) = task.await { - if let Err(ref error_message) = result { - if !stderr_output.trim().is_empty() { - result = Err(format!("{error_message}: {}", stderr_output.trim())); + if let Some(mut task) = stderr_task { + match tokio::time::timeout( + std::time::Duration::from_secs(APP_SERVER_SHUTDOWN_TIMEOUT_SECS), + &mut task, + ) + .await + { + Ok(Ok(stderr_output)) => { + if let Err(ref error_message) = result { + if !stderr_output.trim().is_empty() { + result = Err(format!("{error_message}: {}", stderr_output.trim())); + } } } + Ok(Err(err)) => { + log::debug!( + "[CodexAppServer] stderr reader failed after {}: {}", + operation, + err + ); + } + Err(_) => { + task.abort(); + log::warn!( + "[CodexAppServer] stderr pipe did not close after {}; reader aborted", + operation + ); + } } } result } +async fn terminate_codex_app_server_tree( + child: &mut Child, + child_pid: Option, + operation: &str, +) { + #[cfg(unix)] + if let Some(pid) = child_pid { + // SAFETY: the child was spawned as leader of a dedicated process + // group. A stale PID returns ESRCH; no Rust memory invariants apply. + unsafe { + libc::kill(-(pid as libc::pid_t), libc::SIGKILL); + } + } + + #[cfg(windows)] + if let Some(pid) = child_pid { + let mut taskkill = Command::new("taskkill"); + taskkill.args(["/PID", &pid.to_string(), "/T", "/F"]); + taskkill.creation_flags(app_platform::CREATE_NO_WINDOW); + let _ = tokio::time::timeout( + std::time::Duration::from_secs(APP_SERVER_SHUTDOWN_TIMEOUT_SECS), + taskkill.output(), + ) + .await; + } + + if let Err(err) = child.kill().await { + log::debug!( + "[CodexAppServer] Direct child already stopped after {}: {}", + operation, + err + ); + } + if tokio::time::timeout( + std::time::Duration::from_secs(APP_SERVER_SHUTDOWN_TIMEOUT_SECS), + child.wait(), + ) + .await + .is_err() + { + log::warn!( + "[CodexAppServer] Direct child did not exit after {} within {}s", + operation, + APP_SERVER_SHUTDOWN_TIMEOUT_SECS + ); + } +} + async fn write_json_rpc_request( stdin: &mut tokio::process::ChildStdin, id: u64, @@ -1201,4 +1272,41 @@ mod model_discovery_tests { assert_eq!(models[0].supported_efforts, vec!["low", "high", "max"]); assert!(models[0].is_default); } + + #[cfg(unix)] + #[tokio::test] + async fn app_server_shutdown_terminates_wrapper_descendants() { + let mut command = Command::new("sh"); + command + .args(["-c", "sleep 60 & helper=$!; echo $helper; wait"]) + .process_group(0) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .kill_on_drop(true); + let mut child = command.spawn().expect("spawn wrapper process"); + let child_pid = child.id(); + let stdout = child.stdout.take().expect("wrapper stdout"); + let mut reader = BufReader::new(stdout).lines(); + let helper_pid: i32 = reader + .next_line() + .await + .expect("read helper pid") + .expect("helper pid line") + .trim() + .parse() + .expect("numeric helper pid"); + + terminate_codex_app_server_tree(&mut child, child_pid, "test shutdown").await; + + let mut helper_alive = true; + for _ in 0..20 { + // SAFETY: signal 0 performs an existence check only. + helper_alive = unsafe { libc::kill(helper_pid, 0) == 0 }; + if !helper_alive { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(25)).await; + } + assert!(!helper_alive, "descendant process {helper_pid} survived"); + } } diff --git a/src/features/TeamCollaboration/forkModelFallback.test.ts b/src/features/TeamCollaboration/forkModelFallback.test.ts index 17e56658e..06ef5fd22 100644 --- a/src/features/TeamCollaboration/forkModelFallback.test.ts +++ b/src/features/TeamCollaboration/forkModelFallback.test.ts @@ -28,6 +28,14 @@ describe("isModelRunnableLocally", () => { expect(isModelRunnableLocally("gpt-5.6-sol", [makeKey()])).toBe(false); }); + it("rejects a stale enabled model removed from the available catalog", () => { + expect( + isModelRunnableLocally("gpt-5.6-sol", [ + makeKey({ enabled_models: ["gpt-5.6-sol"] }), + ]) + ).toBe(false); + }); + it("rejects when the only matching key is disabled", () => { expect( isModelRunnableLocally("deepseek-v4-pro", [makeKey({ enabled: false })]) diff --git a/src/features/TeamCollaboration/forkModelFallback.ts b/src/features/TeamCollaboration/forkModelFallback.ts index 2f430595b..98acf1adc 100644 --- a/src/features/TeamCollaboration/forkModelFallback.ts +++ b/src/features/TeamCollaboration/forkModelFallback.ts @@ -22,10 +22,14 @@ export function isModelRunnableWithAccount( ): boolean { const key = keys.find((candidate) => candidate.id === accountId); if (!key || !keyIsUsable(key)) return false; + const available = new Set(key.available_models ?? []); const enabled = new Set(key.enabled_models ?? []); - if (enabled.has(model)) return true; + if (available.has(model) && enabled.has(model)) return true; return (key.model_variants ?? []).some( - (variant) => variant.model === model && enabled.has(variant.base_model) + (variant) => + variant.model === model && + available.has(variant.base_model) && + enabled.has(variant.base_model) ); } @@ -36,10 +40,14 @@ export function isModelRunnableLocally( if (isOrgiiTierModel(model)) return true; return keys.some((key) => { if (!keyIsUsable(key)) return false; + const available = new Set(key.available_models ?? []); const enabled = new Set(key.enabled_models ?? []); - if (enabled.has(model)) return true; + if (available.has(model) && enabled.has(model)) return true; return (key.model_variants ?? []).some( - (variant) => variant.model === model && enabled.has(variant.base_model) + (variant) => + variant.model === model && + available.has(variant.base_model) && + enabled.has(variant.base_model) ); }); } diff --git a/src/hooks/housekeeper/useHousekeeperConfig.ts b/src/hooks/housekeeper/useHousekeeperConfig.ts index 51651cfba..819bd3446 100644 --- a/src/hooks/housekeeper/useHousekeeperConfig.ts +++ b/src/hooks/housekeeper/useHousekeeperConfig.ts @@ -20,8 +20,10 @@ export function getHousekeeperModelCandidates( ): string[] { if (!account) return []; const candidates: string[] = []; - for (const model of account.enabledModels ?? []) - pushUnique(candidates, model); + const available = new Set(account.availableModels ?? []); + for (const model of account.enabledModels ?? []) { + if (available.has(model)) pushUnique(candidates, model); + } for (const model of account.availableModels ?? []) pushUnique(candidates, model); for (const variant of account.modelVariants ?? []) { diff --git a/src/hooks/models/useModelAccountLookup.test.ts b/src/hooks/models/useModelAccountLookup.test.ts index d7240234d..9911db2b4 100644 --- a/src/hooks/models/useModelAccountLookup.test.ts +++ b/src/hooks/models/useModelAccountLookup.test.ts @@ -48,6 +48,16 @@ describe("accountHasModel", () => { expect(accountHasModel(account, "claude-opus-4-8-high")).toBe(false); }); + it("rejects enabled ids that are no longer in the available catalog", () => { + const account = claudeAccount({ + availableModels: ["claude-opus-4-7"], + enabledModels: ["claude-opus-4-8"], + }); + + expect(accountHasModel(account, "claude-opus-4-8")).toBe(false); + expect(accountHasModel(account, "claude-opus-4-8-high")).toBe(false); + }); + it("rejects unknown ids and disabled accounts", () => { expect(accountHasModel(claudeAccount(), "claude-opus-4-8-xhigh")).toBe( false @@ -74,4 +84,15 @@ describe("buildAccountLookup", () => { const lookup = buildAccountLookup([claudeAccount({ enabledModels: [] })]); expect(lookup.has("claude-opus-4-8-high")).toBe(false); }); + + it("does not count stale enabled ids from a different account catalog", () => { + const stale = claudeAccount({ + id: "stale-key", + availableModels: ["claude-opus-4-7"], + enabledModels: ["claude-opus-4-8"], + }); + const lookup = buildAccountLookup([claudeAccount(), stale]); + + expect(lookup.get("claude-opus-4-8")?.totalKeys).toBe(1); + }); }); diff --git a/src/hooks/models/useModelAccountLookup.ts b/src/hooks/models/useModelAccountLookup.ts index 13b3f1e66..84d373b9f 100644 --- a/src/hooks/models/useModelAccountLookup.ts +++ b/src/hooks/models/useModelAccountLookup.ts @@ -5,12 +5,13 @@ import { type KeyVaultAccount, useKeyVault } from "@src/hooks/keyVault"; import type { ModelAccountInfo } from "./types"; /** - * Returns true if `account` has `modelId` enabled. + * Returns true if `account` currently exposes and has enabled `modelId`. * - * Two ways an id counts as enabled: - * - it is in `enabledModels` directly, or + * Two ways an id counts as selectable: + * - it is present in both `availableModels` and `enabledModels`, or * - it is a variant row from `modelVariants` (e.g. a backend-synthesized - * effort rung like `claude-opus-4-8-high`) whose BASE model is enabled — + * effort rung like `claude-opus-4-8-high`) whose BASE model is available + * and enabled — * variant ids never appear in enabledModels themselves, so gating on * enabledModels alone hides every synthesized effort ladder from the * picker's variant-edit affordance. @@ -23,10 +24,14 @@ export function accountHasModel( modelId: string ): boolean { if (!account.enabled) return false; + const available = new Set(account.availableModels ?? []); const enabled = new Set(account.enabledModels ?? []); - if (enabled.has(modelId)) return true; + if (available.has(modelId) && enabled.has(modelId)) return true; return (account.modelVariants ?? []).some( - (variant) => variant.model === modelId && enabled.has(variant.base_model) + (variant) => + variant.model === modelId && + available.has(variant.base_model) && + enabled.has(variant.base_model) ); } diff --git a/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.test.ts b/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.test.ts index eb0557ebe..ec25fd5af 100644 --- a/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.test.ts +++ b/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.test.ts @@ -46,7 +46,7 @@ describe("modelIdsFor", () => { it("prefers enabled models and removes duplicates", () => { const value = account("key", { enabledModels: ["gpt-5", "gpt-5", "gpt-5-mini"], - availableModels: ["ignored"], + availableModels: ["gpt-5", "gpt-5-mini", "ignored"], }); expect(modelIdsFor(value)).toEqual(["gpt-5", "gpt-5-mini"]); @@ -65,8 +65,14 @@ describe("modelIdsFor", () => { describe("getManagedProxyDraftSelection", () => { it("does not carry a stale model onto a different compatible key", () => { const accounts = [ - account("openai", { enabledModels: ["gpt-5"] }), - account("anthropic", { enabledModels: ["claude-sonnet"] }), + account("openai", { + availableModels: ["gpt-5"], + enabledModels: ["gpt-5"], + }), + account("anthropic", { + availableModels: ["claude-sonnet"], + enabledModels: ["claude-sonnet"], + }), ]; expect( @@ -76,7 +82,10 @@ describe("getManagedProxyDraftSelection", () => { it("keeps a saved model only when it belongs to the saved key", () => { const accounts = [ - account("openai", { enabledModels: ["gpt-5", "gpt-5-mini"] }), + account("openai", { + availableModels: ["gpt-5", "gpt-5-mini"], + enabledModels: ["gpt-5", "gpt-5-mini"], + }), ]; expect( diff --git a/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.ts b/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.ts index bdca964c3..9c26880bb 100644 --- a/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.ts +++ b/src/modules/MainApp/AgentOrgs/components/cliManagedConfigUtils.ts @@ -2,10 +2,12 @@ import type { KeyVaultAccount } from "@src/hooks/keyVault"; export function modelIdsFor(account: KeyVaultAccount | undefined): string[] { if (!account) return []; - const models = - account.enabledModels && account.enabledModels.length > 0 - ? account.enabledModels - : (account.availableModels ?? []); + const available = account.availableModels ?? []; + const availableSet = new Set(available); + const enabledAvailable = (account.enabledModels ?? []).filter((model) => + availableSet.has(model) + ); + const models = enabledAvailable.length > 0 ? enabledAvailable : available; return Array.from(new Set(models.filter(Boolean))); } diff --git a/src/modules/MainApp/AgentOrgs/hooks/useWorkflowModelOptions.ts b/src/modules/MainApp/AgentOrgs/hooks/useWorkflowModelOptions.ts index 4ebbda349..284ea5766 100644 --- a/src/modules/MainApp/AgentOrgs/hooks/useWorkflowModelOptions.ts +++ b/src/modules/MainApp/AgentOrgs/hooks/useWorkflowModelOptions.ts @@ -18,7 +18,10 @@ */ import { useMemo } from "react"; -import { useModelAccountLookup } from "@src/hooks/models"; +import { + accountHasModel, + useModelAccountLookup, +} from "@src/hooks/models/useModelAccountLookup"; import { formatModelNameFull } from "@src/util/formatModelName"; import { useAgentDefinitions } from "./useAgentDefinitions"; @@ -38,10 +41,9 @@ export function useWorkflowModelOptions(): WorkflowSelectOption[] { return useMemo(() => { const enabled = new Set(); - for (const acc of accounts) { - if (!acc.enabled) continue; - for (const modelId of acc.enabledModels ?? []) { - if (modelId) enabled.add(modelId); + for (const account of accounts) { + for (const modelId of account.enabledModels ?? []) { + if (accountHasModel(account, modelId)) enabled.add(modelId); } } return Array.from(enabled) diff --git a/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.test.ts b/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.test.ts new file mode 100644 index 000000000..efd85a4c4 --- /dev/null +++ b/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "vitest"; + +import type { KeyVaultAccount } from "@src/hooks/keyVault/types"; + +import { resolveCurrentModelEntry } from "./useUnifiedModelPaletteItems"; + +function accountWithStaleEnabledModel(): KeyVaultAccount { + return { + id: "codex-key", + hasLocalKey: true, + isListed: false, + modelType: "codex", + name: "OpenAIne w", + status: "ready", + hasKey: true, + hasApiKey: false, + hasSessionToken: true, + authMethod: "oauth", + enabled: true, + availableModels: ["gpt-5.5", "gpt-5.4"], + enabledModels: ["gpt-5.6-sol", "gpt-5.5", "gpt-5.4"], + }; +} + +describe("useUnifiedModelPaletteItems", () => { + it("does not synthesize an actionable recent row for a stale active pair", () => { + const accounts = [accountWithStaleEnabledModel()]; + const entry = resolveCurrentModelEntry({ + activeModelId: "gpt-5.6-sol", + advancedConfig: { + keySource: "own_key", + model: "gpt-5.6-sol", + selectedAccountId: "codex-key", + selectedSourceLabel: "OpenAIne w", + selectedSourceModelType: "codex", + }, + compatibleRecentEntries: [], + groupByModel: new Map(), + compatibilityContext: { + accounts, + orgiiPoolEnabled: true, + orgiiModelSet: new Map(), + orgiiCategoryIds: new Set(), + }, + }); + + expect(entry).toBeNull(); + }); + + it("keeps a current pair that remains available and enabled", () => { + const account = accountWithStaleEnabledModel(); + const entry = resolveCurrentModelEntry({ + activeModelId: "gpt-5.5", + advancedConfig: { + keySource: "own_key", + model: "gpt-5.5", + selectedAccountId: "codex-key", + }, + compatibleRecentEntries: [], + groupByModel: new Map([["gpt-5.5", ["gpt-5.5"]]]), + compatibilityContext: { + accounts: [account], + orgiiPoolEnabled: true, + orgiiModelSet: new Map(), + orgiiCategoryIds: new Set(), + }, + }); + + expect(entry).toMatchObject({ + modelId: "gpt-5.5", + accountId: "codex-key", + }); + }); +}); diff --git a/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.ts b/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.ts index 7f8f789df..a1e37c88d 100644 --- a/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.ts +++ b/src/scaffold/GlobalSpotlight/palettes/UnifiedModelPalette/useUnifiedModelPaletteItems.ts @@ -4,7 +4,10 @@ import { KEY_SOURCE } from "@src/api/tauri/session"; import { ORGII_ORCHESTRATOR } from "@src/assets/providers"; import type { AdvancedConfig } from "@src/features/SessionCreator/types"; import type { KeyVaultAccount } from "@src/hooks/keyVault/types"; -import { isPairCompatible } from "@src/hooks/models/modelPairCompatibility"; +import { + type PairCompatibilityContext, + isPairCompatible, +} from "@src/hooks/models/modelPairCompatibility"; import { accountHasModel } from "@src/hooks/models/useModelAccountLookup"; import type { RecentModelEntry } from "@src/store/session/recentModelEntriesAtom"; import { recentEntriesEquivalent } from "@src/store/session/recentModelEntriesAtom"; @@ -56,6 +59,69 @@ interface UseUnifiedModelPaletteItemsParams { tCommon: (key: string) => string; } +interface ResolveCurrentModelEntryParams { + activeModelId: string | undefined; + advancedConfig: AdvancedConfig; + compatibleRecentEntries: RecentModelEntry[]; + groupByModel: Map; + compatibilityContext: PairCompatibilityContext; +} + +export function resolveCurrentModelEntry({ + activeModelId, + advancedConfig, + compatibleRecentEntries, + groupByModel, + compatibilityContext, +}: ResolveCurrentModelEntryParams): RecentModelEntry | null { + if (!activeModelId) return null; + + const fromRecents = compatibleRecentEntries.find((entry) => + entryMatchesActiveConfig(entry, advancedConfig) + ); + if (fromRecents) return fromRecents; + + const { accounts } = compatibilityContext; + const selectedAccount = advancedConfig.selectedAccountId + ? accounts.find((entry) => entry.id === advancedConfig.selectedAccountId) + : undefined; + const activeModelFamily = groupByModel.get(activeModelId) ?? [activeModelId]; + const inferredAccount = + selectedAccount ?? + accounts.find((account) => { + const selectedModelType = + advancedConfig.selectedSourceModelType ?? + advancedConfig.listingModelType; + if (selectedModelType && account.modelType !== selectedModelType) { + return false; + } + if ( + advancedConfig.selectedSourceLabel && + account.name !== advancedConfig.selectedSourceLabel + ) { + return false; + } + return activeModelFamily.some((modelId) => + accountHasModel(account, modelId) + ); + }); + + const candidate: RecentModelEntry = { + modelId: activeModelId, + sourceType: advancedConfig.keySource ?? KEY_SOURCE.OWN, + accountId: inferredAccount?.id ?? advancedConfig.selectedAccountId, + accountName: advancedConfig.selectedSourceLabel ?? inferredAccount?.name, + modelType: + advancedConfig.selectedSourceModelType ?? + advancedConfig.listingModelType ?? + inferredAccount?.modelType ?? + ORGII_ORCHESTRATOR, + cliAgentType: advancedConfig.cliAgentType, + }; + + return isPairCompatible(candidate, compatibilityContext) ? candidate : null; +} + export function useUnifiedModelPaletteItems({ advancedConfig, accounts, @@ -117,59 +183,31 @@ export function useUnifiedModelPaletteItems({ const MAX_RECENT_ITEMS = 3; - const currentModelEntry = useMemo((): RecentModelEntry | null => { - if (!activeModelId) return null; - - const fromRecents = compatibleRecentEntries.find((entry) => - entryMatchesActiveConfig(entry, advancedConfig) - ); - if (fromRecents) return fromRecents; - - const selectedAccount = advancedConfig.selectedAccountId - ? accounts.find((entry) => entry.id === advancedConfig.selectedAccountId) - : undefined; - const activeModelFamily = groupByModel.get(activeModelId) ?? [ + const currentModelEntry = useMemo( + () => + resolveCurrentModelEntry({ + activeModelId, + advancedConfig, + compatibleRecentEntries, + groupByModel, + compatibilityContext: { + accounts, + orgiiPoolEnabled, + orgiiModelSet, + orgiiCategoryIds, + }, + }), + [ activeModelId, - ]; - const inferredAccount = - selectedAccount ?? - accounts.find((account) => { - const selectedModelType = - advancedConfig.selectedSourceModelType ?? - advancedConfig.listingModelType; - if (selectedModelType && account.modelType !== selectedModelType) { - return false; - } - if ( - advancedConfig.selectedSourceLabel && - account.name !== advancedConfig.selectedSourceLabel - ) { - return false; - } - return activeModelFamily.some((modelId) => - accountHasModel(account, modelId) - ); - }); - - return { - modelId: activeModelId, - sourceType: advancedConfig.keySource ?? KEY_SOURCE.OWN, - accountId: inferredAccount?.id ?? advancedConfig.selectedAccountId, - accountName: advancedConfig.selectedSourceLabel ?? inferredAccount?.name, - modelType: - advancedConfig.selectedSourceModelType ?? - advancedConfig.listingModelType ?? - inferredAccount?.modelType ?? - ORGII_ORCHESTRATOR, - cliAgentType: advancedConfig.cliAgentType, - }; - }, [ - activeModelId, - advancedConfig, - compatibleRecentEntries, - accounts, - groupByModel, - ]); + advancedConfig, + compatibleRecentEntries, + accounts, + groupByModel, + orgiiCategoryIds, + orgiiModelSet, + orgiiPoolEnabled, + ] + ); const recentEntriesForDisplay = useMemo((): RecentModelEntry[] => { const entries: RecentModelEntry[] = []; diff --git a/src/scaffold/WizardSystem/variants/KeyVault/components/AgentSetupRouter.tsx b/src/scaffold/WizardSystem/variants/KeyVault/components/AgentSetupRouter.tsx index b95bd2b09..6e7600f25 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/components/AgentSetupRouter.tsx +++ b/src/scaffold/WizardSystem/variants/KeyVault/components/AgentSetupRouter.tsx @@ -10,6 +10,7 @@ import { getOAuthModelCatalog } from "@src/api/services/keyValidation"; import { CLI_AGENT } from "@src/api/tauri/rpc/schemas/validation"; import { LOCAL_MODEL_PROVIDER } from "@src/api/types/keys"; +import type { CredentialDetectionState } from "../hooks/credentialDetectionState"; import { ApiKeyProviderSetup } from "./setup/ApiKeyProviderSetup"; import { ClaudeCodeSetup } from "./setup/ClaudeCodeSetup"; import { CodexSetup } from "./setup/CodexSetup"; @@ -35,6 +36,7 @@ interface AgentSetupRouterProps extends AgentSetupProps { setTokenDetected: (detected: boolean) => void; detectingToken: boolean; tokenError: string | null; + credentialDetection: CredentialDetectionState; setTokenError: (error: string | null) => void; clearTokenError: () => void; useGuidedSetup: boolean; @@ -62,6 +64,7 @@ export const AgentSetupRouter: React.FC = ({ setTokenDetected, detectingToken, tokenError, + credentialDetection, setTokenError, clearTokenError, useGuidedSetup, @@ -115,6 +118,7 @@ export const AgentSetupRouter: React.FC = ({ tokenDetected={tokenDetected} detectingToken={detectingToken} tokenError={tokenError} + credentialDetection={credentialDetection} onDetectToken={sharedProps.onAutoDetect ?? (() => {})} onClearTokenError={clearTokenError} preselectedMethod={isComplex ? setupMethod : undefined} diff --git a/src/scaffold/WizardSystem/variants/KeyVault/components/ApiSetup.tsx b/src/scaffold/WizardSystem/variants/KeyVault/components/ApiSetup.tsx index 796a9c4b9..afc3404d0 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/components/ApiSetup.tsx +++ b/src/scaffold/WizardSystem/variants/KeyVault/components/ApiSetup.tsx @@ -419,6 +419,7 @@ const ApiSetup: React.FC = ({ setTokenDetected={hook.setTokenDetected} detectingToken={hook.detectingToken} tokenError={hook.tokenError} + credentialDetection={hook.credentialDetection} setTokenError={hook.setTokenError} clearTokenError={hook.clearTokenError} useGuidedSetup={hook.useGuidedSetup} diff --git a/src/scaffold/WizardSystem/variants/KeyVault/components/setup/CodexSetup.tsx b/src/scaffold/WizardSystem/variants/KeyVault/components/setup/CodexSetup.tsx index d87c9bd51..4ce33722e 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/components/setup/CodexSetup.tsx +++ b/src/scaffold/WizardSystem/variants/KeyVault/components/setup/CodexSetup.tsx @@ -31,6 +31,7 @@ const CodexSetup: React.FC = ({ tokenDetected, detectingToken, tokenError, + credentialDetection, onDetectToken, onClearTokenError, onSessionCaptured, @@ -61,6 +62,50 @@ const CodexSetup: React.FC = ({ const selectedMethod = (data.setup_method ?? "signin") as CodexMethod; const hideSelector = !!preselectedMethod; + const detectionAlert = useMemo(() => { + switch (credentialDetection.phase) { + case "detecting_credentials": + return { + type: "info" as const, + title: t("keyVault.quickActions.detectingKeys"), + body: t("keyVault.codexAutodetectDesc"), + }; + case "loading_catalog": + return { + type: "info" as const, + title: t("keyVault.quickActions.detectingValidating"), + body: undefined, + }; + case "selecting_credential": + return { + type: "info" as const, + title: t("keyVault.quickActions.multipleKeysFound"), + body: t("keyVault.quickActions.keysFoundMessage", { + count: credentialDetection.credentialCount ?? 0, + provider: "Codex", + }), + }; + case "success": + return { + type: "success" as const, + title: t("keyVault.codexConnected"), + body: t("keyVault.quickActions.modelsAvailable", { + count: credentialDetection.modelCount ?? 0, + }), + }; + case "error": + return { + type: "danger" as const, + title: + credentialDetection.message ?? + tokenError ?? + t("keyVault.failedToDetectKeys"), + body: t("keyVault.codexDetectErrorHint"), + }; + case "idle": + return null; + } + }, [credentialDetection, t, tokenError]); const handleCredentialChange = useCallback( (value: string) => { @@ -182,11 +227,31 @@ const CodexSetup: React.FC = ({ )} - {(tokenDetected || data.validated) && selectedMethod !== "signin" && ( - {t("keyVault.codexConnected")} + {selectedMethod === "autodetect" && detectionAlert && ( +
+ + {detectionAlert.body} + +
)} - {tokenError && selectedMethod !== "signin" && ( + {(tokenDetected || data.validated) && + selectedMethod === "enter_token" && ( + + {t("keyVault.codexConnected")} + + )} + + {tokenError && selectedMethod === "enter_token" && ( void; onClearTokenError?: () => void; onSessionCaptured?: (values: CodexSessionValues) => void; diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/__tests__/credentialDetectionState.test.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/__tests__/credentialDetectionState.test.ts new file mode 100644 index 000000000..f17ee7b40 --- /dev/null +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/__tests__/credentialDetectionState.test.ts @@ -0,0 +1,126 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { + INITIAL_CREDENTIAL_DETECTION_STATE, + credentialDetectionReducer, + getCredentialDetectionErrorMessage, + isCredentialDetectionPending, + withCredentialDetectionTimeout, +} from "../credentialDetectionState"; + +describe("credentialDetectionState", () => { + afterEach(() => { + vi.useRealTimers(); + }); + it("tracks the successful credential and model-catalog flow", () => { + const detecting = credentialDetectionReducer( + INITIAL_CREDENTIAL_DETECTION_STATE, + { type: "begin" } + ); + const loadingCatalog = credentialDetectionReducer(detecting, { + type: "catalog_requested", + }); + const success = credentialDetectionReducer(loadingCatalog, { + type: "succeeded", + modelCount: 7, + }); + + expect(isCredentialDetectionPending(detecting)).toBe(true); + expect(isCredentialDetectionPending(loadingCatalog)).toBe(true); + expect(success).toEqual({ phase: "success", modelCount: 7 }); + expect(isCredentialDetectionPending(success)).toBe(false); + }); + + it("retains the failure reason until the next attempt", () => { + const failed = credentialDetectionReducer( + INITIAL_CREDENTIAL_DETECTION_STATE, + { type: "failed", message: "Model catalog unavailable" } + ); + + expect(failed).toEqual({ + phase: "error", + message: "Model catalog unavailable", + }); + expect(credentialDetectionReducer(failed, { type: "begin" })).toEqual({ + phase: "detecting_credentials", + }); + }); + + it("reports multiple credentials before a selection is applied", () => { + const selecting = credentialDetectionReducer( + INITIAL_CREDENTIAL_DETECTION_STATE, + { type: "credentials_found", count: 2 } + ); + + expect(selecting).toEqual({ + phase: "selecting_credential", + credentialCount: 2, + }); + expect(isCredentialDetectionPending(selecting)).toBe(false); + }); + + it("preserves a successful detection when the model catalog is empty", () => { + const success = credentialDetectionReducer( + INITIAL_CREDENTIAL_DETECTION_STATE, + { type: "succeeded", modelCount: 0 } + ); + + expect(success).toEqual({ phase: "success", modelCount: 0 }); + }); + + it("returns to an idle state when feedback is dismissed", () => { + const failed = { + phase: "error" as const, + message: "No credentials found", + }; + + expect(credentialDetectionReducer(failed, { type: "reset" })).toBe( + INITIAL_CREDENTIAL_DETECTION_STATE + ); + }); + + it("preserves Error and string rejection reasons with a safe fallback", () => { + expect( + getCredentialDetectionErrorMessage( + new Error("OAuth refresh failed"), + "Detection failed" + ) + ).toBe("OAuth refresh failed"); + expect( + getCredentialDetectionErrorMessage( + " Tauri command unavailable ", + "Detection failed" + ) + ).toBe("Tauri command unavailable"); + expect(getCredentialDetectionErrorMessage(null, "Detection failed")).toBe( + "Detection failed" + ); + }); + + it("rejects a hung operation at the configured deadline", async () => { + vi.useFakeTimers(); + const operation = new Promise(() => {}); + const bounded = withCredentialDetectionTimeout( + operation, + 30_000, + "Request timed out" + ); + + const assertion = expect(bounded).rejects.toThrow("Request timed out"); + await vi.advanceTimersByTimeAsync(30_000); + await assertion; + }); + + it("clears the deadline when the operation settles first", async () => { + vi.useFakeTimers(); + + await expect( + withCredentialDetectionTimeout( + Promise.resolve("catalog"), + 30_000, + "Request timed out" + ) + ).resolves.toBe("catalog"); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/credentialDetectionState.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/credentialDetectionState.ts new file mode 100644 index 000000000..cd91af440 --- /dev/null +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/credentialDetectionState.ts @@ -0,0 +1,93 @@ +export type CredentialDetectionPhase = + | "idle" + | "detecting_credentials" + | "loading_catalog" + | "selecting_credential" + | "success" + | "error"; + +export interface CredentialDetectionState { + phase: CredentialDetectionPhase; + credentialCount?: number; + modelCount?: number; + message?: string; +} + +export type CredentialDetectionEvent = + | { type: "begin" } + | { type: "credentials_found"; count: number } + | { type: "catalog_requested" } + | { type: "succeeded"; modelCount: number } + | { type: "failed"; message: string } + | { type: "reset" }; + +export const INITIAL_CREDENTIAL_DETECTION_STATE: CredentialDetectionState = { + phase: "idle", +}; + +export function credentialDetectionReducer( + _state: CredentialDetectionState, + event: CredentialDetectionEvent +): CredentialDetectionState { + switch (event.type) { + case "begin": + return { phase: "detecting_credentials" }; + case "credentials_found": + return { + phase: "selecting_credential", + credentialCount: event.count, + }; + case "catalog_requested": + return { phase: "loading_catalog" }; + case "succeeded": + return { phase: "success", modelCount: event.modelCount }; + case "failed": + return { phase: "error", message: event.message }; + case "reset": + return INITIAL_CREDENTIAL_DETECTION_STATE; + } +} + +export function isCredentialDetectionPending( + state: CredentialDetectionState +): boolean { + return ( + state.phase === "detecting_credentials" || state.phase === "loading_catalog" + ); +} + +export function getCredentialDetectionErrorMessage( + error: unknown, + fallback: string +): string { + if (error instanceof Error && error.message.trim()) { + return error.message.trim(); + } + if (typeof error === "string" && error.trim()) { + return error.trim(); + } + return fallback; +} + +export function withCredentialDetectionTimeout( + operation: Promise, + timeoutMs: number, + timeoutMessage: string +): Promise { + return new Promise((resolve, reject) => { + const timeoutId = setTimeout(() => { + reject(new Error(timeoutMessage)); + }, timeoutMs); + + operation.then( + (value) => { + clearTimeout(timeoutId); + resolve(value); + }, + (error: unknown) => { + clearTimeout(timeoutId); + reject(error); + } + ); + }); +} diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.test.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.test.ts index 68afeb799..040ab2539 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.test.ts +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.test.ts @@ -23,7 +23,7 @@ describe("keyHelpers", () => { validated: true, }; - applyKey(detectedKey, { + const result = applyKey(detectedKey, { onChange: (update) => updates.push(update), setTokenDetected: (value) => { tokenDetected = value; @@ -47,6 +47,7 @@ describe("keyHelpers", () => { expect(cursorSessionToken).toBe("cursor-native-token"); expect(tokenError).toBeNull(); expect(showKeySelection).toBe(false); + expect(result).toEqual({ applied: true, modelCount: 2 }); expect(updates).toEqual([ { auth_method: "oauth", @@ -61,6 +62,41 @@ describe("keyHelpers", () => { ]); }); + it("returns the validation failure used by detection feedback", () => { + let tokenError: string | null = null; + + const result = applyKey( + { + id: "expired-codex-oauth", + name: "OpenAI", + auth_method: "oauth", + session_token: "expired-token", + validated: false, + validation_message: "OAuth session expired", + }, + { + onChange: () => {}, + setTokenDetected: () => {}, + setCursorSessionToken: () => {}, + setTokenError: (value) => { + tokenError = value; + }, + setShowKeySelection: () => {}, + isCursor: false, + isOAuthAgent: true, + noValidTokenMsg: "No valid token", + validationFailedMsg: "Validation failed", + } + ); + + expect(result).toEqual({ + applied: false, + modelCount: 0, + error: "OAuth session expired", + }); + expect(tokenError).toBe("OAuth session expired"); + }); + it("clears stale extracted api key and base URL when applying an OAuth detection", () => { const updates: Partial[] = []; diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.ts index 47ca25aa4..47cab4d15 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.ts +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/keyHelpers.ts @@ -35,6 +35,12 @@ export interface ApplyKeyCallbacks { validationFailedMsg: string; } +export interface ApplyKeyResult { + applied: boolean; + modelCount: number; + error?: string; +} + export function normalizeDetectedQuotaInfo( quotaInfo: DetectedKey["quota_info"] ): WizardData["quota_info"] | undefined { @@ -58,7 +64,7 @@ export function normalizeDetectedQuotaInfo( export function applyKey( cred: DetectedKey, callbacks: ApplyKeyCallbacks -): void { +): ApplyKeyResult { const { onChange, setTokenDetected, @@ -75,8 +81,9 @@ export function applyKey( } = callbacks; if (!cred.validated) { - setTokenError(cred.validation_message || validationFailedMsg); - return; + const error = cred.validation_message || validationFailedMsg; + setTokenError(error); + return { applied: false, modelCount: 0, error }; } const sessionToken = cred.session_token || cred.api_key; @@ -110,7 +117,7 @@ export function applyKey( if (!sessionToken) { setTokenError(noValidTokenMsg); - return; + return { applied: false, modelCount: 0, error: noValidTokenMsg }; } setTokenDetected(true); @@ -173,6 +180,7 @@ export function applyKey( } setShowKeySelection(false); + return { applied: true, modelCount: modelsAvailable.length }; } export interface ExtractCallbacks { diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetup.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetup.ts index 23bf0bc18..297914f61 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetup.ts +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetup.ts @@ -1,4 +1,4 @@ -import { useState } from "react"; +import { useReducer, useState } from "react"; import { useTranslation } from "react-i18next"; import type { DetectedKey } from "@src/api/types/keys"; @@ -14,6 +14,10 @@ import { getApiSetupProceedState, getResolvedCursorSessionToken, } from "./apiSetupDerived"; +import { + INITIAL_CREDENTIAL_DETECTION_STATE, + credentialDetectionReducer, +} from "./credentialDetectionState"; import { useApiSetupCursorToken } from "./useApiSetupCursorToken"; import { useApiSetupExtraction } from "./useApiSetupExtraction"; import { useApiSetupHealthCheck } from "./useApiSetupHealthCheck"; @@ -45,6 +49,10 @@ export function useApiSetup({ data, onChange }: UseApiSetupOptions) { const [detectingToken, setDetectingToken] = useState(false); const [tokenDetected, setTokenDetected] = useState(false); const [tokenError, setTokenError] = useState(null); + const [credentialDetection, dispatchCredentialDetection] = useReducer( + credentialDetectionReducer, + INITIAL_CREDENTIAL_DETECTION_STATE + ); const [inputMode, setInputMode] = useState<"direct" | "natural">("direct"); const [extracting, setExtracting] = useState(false); const [extractError, setExtractError] = useState(null); @@ -130,6 +138,7 @@ export function useApiSetup({ data, onChange }: UseApiSetupOptions) { setShowKeySelection, setDetectedKeys, setSelectedCredentialIndex, + dispatchCredentialDetection, }); const handleExtract = useApiSetupExtraction({ @@ -175,6 +184,7 @@ export function useApiSetup({ data, onChange }: UseApiSetupOptions) { detectingToken, tokenDetected, tokenError, + credentialDetection, setTokenError, inputMode, setInputMode, @@ -207,7 +217,10 @@ export function useApiSetup({ data, onChange }: UseApiSetupOptions) { handleAutoDetectToken, handleExtract, canProceed, - clearTokenError: () => setTokenError(null), + clearTokenError: () => { + setTokenError(null); + dispatchCredentialDetection({ type: "reset" }); + }, clearExtractError: () => setExtractError(null), }; } diff --git a/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetupTokenDetection.ts b/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetupTokenDetection.ts index 068371503..9be45879a 100644 --- a/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetupTokenDetection.ts +++ b/src/scaffold/WizardSystem/variants/KeyVault/hooks/useApiSetupTokenDetection.ts @@ -1,5 +1,6 @@ import type { TFunction } from "i18next"; -import { useCallback, useMemo } from "react"; +import type { Dispatch } from "react"; +import { useCallback, useMemo, useRef } from "react"; import { autoDetectKey, @@ -16,6 +17,11 @@ import { resolveSelectedEndpoint, } from "../config/providerEndpoints"; import type { WizardData } from "../types"; +import { + type CredentialDetectionEvent, + getCredentialDetectionErrorMessage, + withCredentialDetectionTimeout, +} from "./credentialDetectionState"; import { applyKey } from "./keyHelpers"; import { useProviderConfig } from "./useProviderConfig"; @@ -23,6 +29,7 @@ const log = createLogger("ApiSetup"); /** Matches the `zen` entry of `OPENCODE_ENDPOINTS` in `provider_config.rs`. */ const OPENCODE_ZEN_ENDPOINT_ID = "zen"; +const OAUTH_MODEL_CATALOG_TIMEOUT_MS = 30_000; /** * A Zen key authenticates against both OpenCode endpoints, but a Go key is @@ -74,6 +81,7 @@ interface UseApiSetupTokenDetectionOptions { setShowKeySelection: (value: boolean) => void; setDetectedKeys: (value: DetectedKey[]) => void; setSelectedCredentialIndex: (value: number) => void; + dispatchCredentialDetection: Dispatch; } export function useApiSetupTokenDetection({ @@ -93,7 +101,9 @@ export function useApiSetupTokenDetection({ setShowKeySelection, setDetectedKeys, setSelectedCredentialIndex, + dispatchCredentialDetection, }: UseApiSetupTokenDetectionOptions) { + const detectionInFlightRef = useRef(false); // OpenCode's Zen/Go endpoints come from the Rust provider registry, same as // every other provider's — autodetect must not re-hardcode their URLs. const { config: openCodeConfig } = useProviderConfig(CLI_AGENT.OPENCODE); @@ -106,7 +116,7 @@ export function useApiSetupTokenDetection({ async (cred: DetectedKey) => { if (data.agent_type === "opencode" && cred.api_key) { const models = cred.available_models ?? []; - applyKey(cred, { + const result = applyKey(cred, { onChange, setTokenDetected, setCursorSessionToken, @@ -118,23 +128,40 @@ export function useApiSetupTokenDetection({ noValidTokenMsg: t("keyVault.noValidTokenFound"), validationFailedMsg: t("keyVault.quickActions.keyValidationFailed"), }); - return; + dispatchCredentialDetection( + result.applied + ? { type: "succeeded", modelCount: result.modelCount } + : { + type: "failed", + message: + result.error ?? + t("keyVault.quickActions.keyValidationFailed"), + } + ); + return result; } + if (isClaudeCode || isCodex) { + dispatchCredentialDetection({ type: "catalog_requested" }); + } const catalog = isClaudeCode || isCodex - ? await getOAuthModelCatalog( - isClaudeCode ? CLI_AGENT.CLAUDE_CODE : CLI_AGENT.CODEX, - { - accessToken: cred.session_token ?? cred.api_key ?? undefined, - refreshToken: isClaudeCode - ? cred.env_vars?.CLAUDE_CODE_REFRESH_TOKEN - : cred.env_vars?.OPENAI_REFRESH_TOKEN, - idToken: cred.env_vars?.OPENAI_ID_TOKEN, - } + ? await withCredentialDetectionTimeout( + getOAuthModelCatalog( + isClaudeCode ? CLI_AGENT.CLAUDE_CODE : CLI_AGENT.CODEX, + { + accessToken: cred.session_token ?? cred.api_key ?? undefined, + refreshToken: isClaudeCode + ? cred.env_vars?.CLAUDE_CODE_REFRESH_TOKEN + : cred.env_vars?.OPENAI_REFRESH_TOKEN, + idToken: cred.env_vars?.OPENAI_ID_TOKEN, + } + ), + OAUTH_MODEL_CATALOG_TIMEOUT_MS, + t("common:errors.api.messages.timeout") ) : undefined; - applyKey(cred, { + const result = applyKey(cred, { onChange, setTokenDetected, setCursorSessionToken, @@ -146,9 +173,20 @@ export function useApiSetupTokenDetection({ noValidTokenMsg: t("keyVault.noValidTokenFound"), validationFailedMsg: t("keyVault.quickActions.keyValidationFailed"), }); + dispatchCredentialDetection( + result.applied + ? { type: "succeeded", modelCount: result.modelCount } + : { + type: "failed", + message: + result.error ?? t("keyVault.quickActions.keyValidationFailed"), + } + ); + return result; }, [ data.agent_type, + dispatchCredentialDetection, isClaudeCode, isCodex, isOAuthAgent, @@ -163,15 +201,20 @@ export function useApiSetupTokenDetection({ ); const handleAutoDetectToken = useCallback(async () => { + if (detectionInFlightRef.current) return; + detectionInFlightRef.current = true; setDetectingToken(true); setTokenError(null); setTokenDetected(false); + dispatchCredentialDetection({ type: "begin" }); try { const result = await autoDetectKey(data.agent_type); if (!result.success) { - setTokenError(result.message || t("keyVault.couldNotDetectKeys")); + const message = result.message || t("keyVault.couldNotDetectKeys"); + setTokenError(message); + dispatchCredentialDetection({ type: "failed", message }); return; } @@ -200,7 +243,9 @@ export function useApiSetupTokenDetection({ : keys; if (candidateKeys.length === 0) { - setTokenError(t("keyVault.couldNotDetectKeys")); + const message = t("keyVault.couldNotDetectKeys"); + setTokenError(message); + dispatchCredentialDetection({ type: "failed", message }); return; } @@ -225,14 +270,24 @@ export function useApiSetupTokenDetection({ : 0 ); setShowKeySelection(true); + dispatchCredentialDetection({ + type: "credentials_found", + count: candidateKeys.length, + }); return; } - applySelectedKey(candidateKeys[0]); + await applySelectedKey(candidateKeys[0]); } catch (err) { log.error("[ApiSetup] Failed to auto-detect credentials:", err); - setTokenError(t("keyVault.failedToDetectKeys")); + const message = getCredentialDetectionErrorMessage( + err, + t("keyVault.failedToDetectKeys") + ); + setTokenError(message); + dispatchCredentialDetection({ type: "failed", message }); } finally { + detectionInFlightRef.current = false; setDetectingToken(false); } }, [ @@ -240,6 +295,7 @@ export function useApiSetupTokenDetection({ data.extracted_base_url, openCodeEndpoints, applySelectedKey, + dispatchCredentialDetection, isClaudeCode, setDetectedKeys, setDetectingToken, @@ -250,12 +306,45 @@ export function useApiSetupTokenDetection({ t, ]); - const handleConfirmKeySelection = useCallback(() => { + const handleConfirmKeySelection = useCallback(async () => { + if (detectionInFlightRef.current) return; const selected = detectedKeys[selectedCredentialIndex]; - if (selected) { - applySelectedKey(selected); + if (!selected) { + const message = t("keyVault.quickActions.noValidKeys"); + setTokenError(message); + setShowKeySelection(false); + dispatchCredentialDetection({ type: "failed", message }); + return; + } + + detectionInFlightRef.current = true; + setShowKeySelection(false); + setDetectingToken(true); + setTokenError(null); + try { + await applySelectedKey(selected); + } catch (err) { + log.error("[ApiSetup] Failed to apply detected credential:", err); + const message = getCredentialDetectionErrorMessage( + err, + t("keyVault.failedToDetectKeys") + ); + setTokenError(message); + dispatchCredentialDetection({ type: "failed", message }); + } finally { + detectionInFlightRef.current = false; + setDetectingToken(false); } - }, [detectedKeys, selectedCredentialIndex, applySelectedKey]); + }, [ + applySelectedKey, + detectedKeys, + dispatchCredentialDetection, + selectedCredentialIndex, + setDetectingToken, + setShowKeySelection, + setTokenError, + t, + ]); return { handleAutoDetectToken, handleConfirmKeySelection }; }