diff --git a/CLAUDE.md b/CLAUDE.md index d00981305..cccfd777d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -483,6 +483,13 @@ compliance page is required reading before research data goes near either. (`routes/coding_agents.rs`) backs the onboarding card `onboarding/CodingAgentInlineCard.tsx`, wired beside `LlamaServerInlineCard` in `ProviderGuard.tsx`. `CLAUDE_CODE_COMMAND` / `CODEX_COMMAND` override discovery. +- **"Configured" means the key is saved AND the CLI resolves.** `check_provider_configured` + (`routes/utils.rs`) asks `discovery::resolve_configured` — the lookup the status probe + uses — so a command key naming a missing CLI is served `is_configured: false` with + `unavailable_reason`, and `SwitchModelModal` shows that row disabled with the reason rather + than offering a bind `from_env` would refuse. Sign-in is deliberately NOT part of it: learning + it spawns the CLI, and `GET /config/providers` runs for every provider. See + [`docs/desktop-ui/provider-catalog.md`](docs/desktop-ui/provider-catalog.md). - **Tests:** `cargo test -p biorouter --lib providers::coding_agent`, `cargo test -p biorouter-server --test tool_bridge_routes`, and the vitest suite for the onboarding card. The live end-to-end tests need the real vendor diff --git a/crates/biorouter-server/src/routes/config_management.rs b/crates/biorouter-server/src/routes/config_management.rs index 4e923b233..10687a314 100644 --- a/crates/biorouter-server/src/routes/config_management.rs +++ b/crates/biorouter-server/src/routes/config_management.rs @@ -1,4 +1,4 @@ -use crate::routes::utils::check_provider_configured; +use crate::routes::utils::{check_provider_configured, provider_readiness, ProviderReadiness}; use crate::state::AppState; use axum::routing::put; use axum::{ @@ -129,6 +129,20 @@ pub struct ProviderDetails { /// `extensionPairingRefused` documents the same rule on its side. #[serde(default)] pub resolved_tier: Option, + /// Why a provider the user HAS set up cannot run right now: a one-line + /// sentence for the model picker to print on the row it disables. + /// + /// Set only when [`Self::is_configured`] is false for a reason other than a + /// missing key — today, a coding agent whose command key is saved and whose + /// CLI does not resolve (see `routes::utils::provider_readiness`). `None` for + /// every usable provider and for every provider that is simply not set up, + /// which the picker leaves out rather than greys out. + /// + /// ⚠ **Only what can be learned without spawning.** A signed-out CLI is not + /// reported here: finding that out means running it, and this route runs + /// for every provider on every settings open. + #[serde(default)] + pub unavailable_reason: Option, } #[derive(Serialize, ToSchema)] @@ -991,33 +1005,45 @@ pub async fn providers() -> Result>, StatusCode> { // Concurrently, because each row may construct a provider and a serial pass // would add every constructor's latency together on a route the settings // grid blocks on. - let providers_response: Vec = - futures::future::join_all(providers.into_iter().map( - |(metadata, provider_type)| async move { - let is_configured = check_provider_configured(&metadata, provider_type); - // Issue #56, DR-26. Both resolved from the instance, never from - // the name — see `resolve_provider_axes`. - let (resolved_tier, affiliation) = if is_configured { - resolve_provider_axes(&metadata).await - } else { - (None, None) - }; - - ProviderDetails { - name: metadata.name.clone(), - metadata, - is_configured, - provider_type, - affiliation, - resolved_tier, - } - }, - )) - .await; + let providers_response: Vec = futures::future::join_all( + providers + .into_iter() + .map(|(metadata, provider_type)| provider_details(metadata, provider_type)), + ) + .await; Ok(Json(providers_response)) } +/// One row of `GET /config/providers`. +async fn provider_details( + metadata: ProviderMetadata, + provider_type: ProviderType, +) -> ProviderDetails { + let (is_configured, unavailable_reason) = match provider_readiness(&metadata, provider_type) { + ProviderReadiness::Configured => (true, None), + ProviderReadiness::NotConfigured => (false, None), + ProviderReadiness::Unavailable(reason) => (false, Some(reason)), + }; + // Issue #56, DR-26. Both resolved from the instance, never from the name — + // see `resolve_provider_axes`. + let (resolved_tier, affiliation) = if is_configured { + resolve_provider_axes(&metadata).await + } else { + (None, None) + }; + + ProviderDetails { + name: metadata.name.clone(), + metadata, + is_configured, + provider_type, + affiliation, + resolved_tier, + unavailable_reason, + } +} + #[utoipa::path( get, path = "/config/providers/{name}/models", @@ -2202,6 +2228,7 @@ mod affiliation_wire_tests { provider_type: ProviderType::Builtin, affiliation, resolved_tier, + unavailable_reason: None, } } @@ -2382,3 +2409,82 @@ mod privacy_disclosure_tests { ); } } + +/// F6 of the 2026-09-10 provider QA run, at the route: a coding agent whose CLI +/// is missing is served `is_configured: false` WITH the reason the model picker +/// prints on the row it disables — and an ordinary row carries an explicit +/// `null` in the same key. +/// +/// ⚠ Exercised through `provider_details`, the one function `providers()` maps +/// over, rather than through the whole route: `GET /config/providers` builds +/// every configured provider in the developer's real config, which no unit test +/// should do. The command key is pinned through the environment under +/// `env_lock`, so the real config file never decides the outcome. +#[cfg(test)] +mod readiness_wire_tests { + use super::*; + use biorouter::providers::base::Provider; + use biorouter::providers::codex::CodexProvider; + use biorouter::providers::coding_agent::CodingAgentKind; + + #[tokio::test] + async fn a_codex_row_whose_cli_is_missing_is_unconfigured_and_says_why() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("nonexistent").join("codex"); + let _env = env_lock::lock_env([("CODEX_COMMAND", Some(missing.to_str().unwrap()))]); + + let row = provider_details(CodexProvider::metadata(), ProviderType::Builtin).await; + + assert!( + !row.is_configured, + "the badge and the picker both key on this" + ); + assert_eq!( + row.unavailable_reason.as_deref(), + Some(CodingAgentKind::Codex.not_installed_summary().as_str()) + ); + // Nothing was constructed for a provider that cannot be bound. + assert!(row.resolved_tier.is_none() && row.affiliation.is_none()); + + let json = serde_json::to_value(row).unwrap(); + assert_eq!(json["is_configured"], serde_json::json!(false)); + assert_eq!( + json["unavailable_reason"], + serde_json::json!(CodingAgentKind::Codex.not_installed_summary()) + ); + } + + /// The control: a codex row whose CLI resolves is configured and carries no + /// reason — so the test above cannot pass for a route that refuses Codex + /// outright. + #[tokio::test] + async fn a_codex_row_whose_cli_resolves_is_configured_with_no_reason() { + let dir = tempfile::tempdir().unwrap(); + let exe = dir.path().join("codex"); + std::fs::write(&exe, b"#!/bin/sh\n").unwrap(); + let _env = env_lock::lock_env([("CODEX_COMMAND", Some(exe.to_str().unwrap()))]); + + let row = provider_details(CodexProvider::metadata(), ProviderType::Builtin).await; + + assert!(row.is_configured); + assert_eq!(row.unavailable_reason, None); + } + + /// Usable and not-set-up rows alike serve the key as `null`, never omit it: + /// an absent key is indistinguishable from a daemon that predates the field. + #[test] + fn a_row_with_nothing_to_explain_serialises_an_explicit_null() { + let row = ProviderDetails { + name: "openai".to_string(), + metadata: ProviderMetadata::empty(), + is_configured: false, + provider_type: ProviderType::Builtin, + affiliation: None, + resolved_tier: None, + unavailable_reason: None, + }; + let json = serde_json::to_value(row).unwrap(); + assert!(json.as_object().unwrap().contains_key("unavailable_reason")); + assert!(json["unavailable_reason"].is_null()); + } +} diff --git a/crates/biorouter-server/src/routes/utils.rs b/crates/biorouter-server/src/routes/utils.rs index dc4d07338..88490e2f7 100644 --- a/crates/biorouter-server/src/routes/utils.rs +++ b/crates/biorouter-server/src/routes/utils.rs @@ -1,9 +1,64 @@ use biorouter::config::declarative_providers::load_provider; use biorouter::config::Config; use biorouter::providers::base::{ConfigKey, ProviderMetadata, ProviderType}; +use biorouter::providers::coding_agent::discovery::{self, CodingAgentKind}; use std::env; +/// Whether a provider can be used, as `GET /config/providers` reports it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ProviderReadiness { + /// Its keys are saved, and nothing it needs that can be checked cheaply is + /// missing. The only state `is_configured` is true for. + Configured, + /// Not set up: a key it requires has not been saved. + NotConfigured, + /// The user set it up, but something it needs at runtime is missing — the + /// one-line reason. Today that is a coding agent whose command key is saved + /// and whose CLI does not resolve. + /// + /// ⚠ **Only what a `stat` can see.** Signed-out is deliberately NOT a reason + /// here: learning it means spawning the vendor CLI, and this runs for every + /// provider on every `GET /config/providers` (see + /// `coding_agent::discovery`'s module header). The catalog's status pill says + /// it, and a turn that reaches a signed-out CLI fails with the vendor's own + /// login command. + Unavailable(String), +} + pub fn check_provider_configured(metadata: &ProviderMetadata, provider_type: ProviderType) -> bool { + provider_readiness(metadata, provider_type) == ProviderReadiness::Configured +} + +/// [`check_provider_configured`], with the reason when a provider the user set +/// up still cannot run. +pub fn provider_readiness( + metadata: &ProviderMetadata, + provider_type: ProviderType, +) -> ProviderReadiness { + if !keys_are_saved(metadata, provider_type) { + return ProviderReadiness::NotConfigured; + } + + // A coding agent's one key only NAMES a command, and a saved name is not an + // installed CLI. Reporting it configured anyway is how the row read "Not + // installed" and "Configured" on one line, and how the model picker offered + // a provider whose `from_env` would refuse the bind. The same + // `resolve_configured` backs `/coding_agents/status`, so the two answers + // agree by construction rather than by a test remembering to compare them. + if provider_type == ProviderType::Builtin { + if let Some(kind) = CodingAgentKind::from_provider_id(&metadata.name) { + if discovery::resolve_configured(kind).is_none() { + return ProviderReadiness::Unavailable(kind.not_installed_summary()); + } + } + } + + ProviderReadiness::Configured +} + +/// Whether every key the provider requires has been saved — the whole of what +/// "configured" meant before a saved key could fail to be enough. +fn keys_are_saved(metadata: &ProviderMetadata, provider_type: ProviderType) -> bool { let config = Config::global(); if provider_type == ProviderType::Custom || provider_type == ProviderType::Declarative { @@ -89,3 +144,113 @@ pub fn check_provider_configured(metadata: &ProviderMetadata, provider_type: Pro is_set_in_env || is_set_in_config }) } + +#[cfg(test)] +mod tests { + //! Issue F6 of the 2026-09-10 provider QA run: with `CODEX_COMMAND` pointed + //! at a path that does not exist, the Codex row read "Not installed" and + //! "✓ Configured" on one line, and Codex stayed selectable in the model + //! picker. + //! + //! ⚠ **Every case sets the command key through the environment**, under + //! `env_lock`'s one process-wide mutex. `Config::get_param` reads the + //! environment before the config file, so neither half of the check ever + //! reaches the developer's real `~/.config/biorouter` — and a saved + //! `CODEX_COMMAND` there cannot decide what these tests see. + + use super::*; + use biorouter::providers::base::Provider; + use biorouter::providers::claude_code::ClaudeCodeProvider; + use biorouter::providers::codex::CodexProvider; + + /// Pin `kind`'s command key to `value` for the life of the guard. + fn command_pinned_to(kind: CodingAgentKind, value: &str) -> env_lock::EnvGuard<'static> { + env_lock::lock_env([(kind.command_config_key(), Some(value))]) + } + + fn metadata_for(kind: CodingAgentKind) -> ProviderMetadata { + match kind { + CodingAgentKind::ClaudeCode => ClaudeCodeProvider::metadata(), + CodingAgentKind::Codex => CodexProvider::metadata(), + } + } + + /// The reported defect. A saved key naming a CLI that is not there is not a + /// configured provider, and it says why. + #[test] + fn a_coding_agent_whose_cli_is_missing_is_not_configured() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("no-such-dir").join("codex"); + + for kind in CodingAgentKind::all() { + let _env = command_pinned_to(kind, missing.to_str().unwrap()); + let metadata = metadata_for(kind); + + assert!( + !check_provider_configured(&metadata, ProviderType::Builtin), + "{kind:?} pointed at {} must not report is_configured", + missing.display() + ); + assert_eq!( + provider_readiness(&metadata, ProviderType::Builtin), + ProviderReadiness::Unavailable(kind.not_installed_summary()), + "{kind:?}: the reason is the not-installed sentence, not a silent false" + ); + } + } + + /// Without this, the test above passes for a check that simply refuses every + /// coding agent. A command that resolves is configured, with no reason. + #[test] + fn a_coding_agent_whose_cli_resolves_is_configured() { + let dir = tempfile::tempdir().unwrap(); + let exe = dir.path().join("codex"); + std::fs::write(&exe, b"#!/bin/sh\n").unwrap(); + + for kind in CodingAgentKind::all() { + let _env = command_pinned_to(kind, exe.to_str().unwrap()); + let metadata = metadata_for(kind); + + assert!(check_provider_configured(&metadata, ProviderType::Builtin)); + assert_eq!( + provider_readiness(&metadata, ProviderType::Builtin), + ProviderReadiness::Configured + ); + } + } + + /// ⚠ **The row and the status route must agree.** The pill beside the name + /// comes from `/coding_agents/status` (`probe`), the check beside it from this + /// function; F6 was the two disagreeing on one line. A pinned path that does + /// not exist resolves nothing, so the probe returns without spawning. + #[tokio::test] + async fn not_configured_is_exactly_what_the_status_route_calls_not_installed() { + let dir = tempfile::tempdir().unwrap(); + let missing = dir.path().join("codex"); + let _env = command_pinned_to(CodingAgentKind::Codex, missing.to_str().unwrap()); + + let status = discovery::probe(CodingAgentKind::Codex).await; + assert_eq!(status.auth, discovery::AuthState::NotInstalled); + assert!(!check_provider_configured( + &metadata_for(CodingAgentKind::Codex), + ProviderType::Builtin + )); + } + + /// The CLI requirement is the coding agents' alone. A provider with the very + /// same key shape — one required key with a default, which is how + /// `llamacpp` reports configured through `LLAMACPP_PORT` — is still judged + /// on the saved key and nothing else. + #[test] + fn the_cli_requirement_applies_to_the_coding_agents_only() { + let mut metadata = ProviderMetadata::empty(); + metadata.name = "same_shape_as_a_coding_agent_f6".to_string(); + metadata.config_keys = vec![ConfigKey::new("F6_SAME_SHAPE_PORT", true, false, Some("1"))]; + let _env = env_lock::lock_env([("F6_SAME_SHAPE_PORT", Some("11543"))]); + + assert_eq!( + provider_readiness(&metadata, ProviderType::Builtin), + ProviderReadiness::Configured + ); + } +} diff --git a/crates/biorouter/src/providers/coding_agent/discovery.rs b/crates/biorouter/src/providers/coding_agent/discovery.rs index 5920a10df..fe270edb5 100644 --- a/crates/biorouter/src/providers/coding_agent/discovery.rs +++ b/crates/biorouter/src/providers/coding_agent/discovery.rs @@ -82,6 +82,17 @@ impl CodingAgentKind { } } + /// The kind whose provider id is `name`, or `None` for every other provider. + /// + /// The inverse of [`Self::provider_id`], so code that sees every provider the + /// daemon serves — `check_provider_configured` — can ask "is this one of the + /// coding agents?" without keeping a second list of their ids. + pub fn from_provider_id(name: &str) -> Option { + Self::all() + .into_iter() + .find(|kind| kind.provider_id() == name) + } + /// The config key naming the executable. /// /// Each provider declares exactly one **required** key with a **default**. @@ -90,6 +101,13 @@ impl CodingAgentKind { /// in the tree ever writes, so a genuinely zero-key provider would report /// `is_configured: false` forever and never appear in the model picker. /// `llamacpp` solves it the same way with `LLAMACPP_PORT`. + /// + /// ⚠ **A saved key is necessary, not sufficient.** The key only NAMES a + /// command, so `check_provider_configured` also requires that command to + /// resolve ([`resolve_configured`]). Before it did, `CODEX_COMMAND` pointed at + /// a path that did not exist left the row reading "Not installed" and + /// "Configured" side by side, and Codex selectable in the model picker — + /// where the bind then failed in `from_env`. pub const fn command_config_key(self) -> &'static str { match self { Self::ClaudeCode => "CLAUDE_CODE_COMMAND", @@ -124,6 +142,20 @@ impl CodingAgentKind { } } + /// One line saying the CLI cannot be found. + /// + /// The first sentence of [`super::unavailable_error`]'s not-installed + /// message, and — on its own — the reason `GET /config/providers` serves for + /// a row the user set up whose CLI is missing, which the model picker prints + /// on the disabled row. One definition, so the picker and the error a turn + /// would have raised cannot come to say different things. + pub fn not_installed_summary(self) -> String { + format!( + "{} is not installed, or is not on a path Biorouter searches", + self.display_name() + ) + } + pub const fn all() -> [Self; 2] { [Self::ClaudeCode, Self::Codex] } @@ -223,6 +255,19 @@ pub fn configured_command(kind: CodingAgentKind) -> Option { .filter(|s| !s.trim().is_empty()) } +/// [`resolve_binary`] under the command the user configured — "is it installed?" +/// asked the way every consumer must ask it. +/// +/// ⚠ **One question, one function.** [`probe`] (behind `/coding_agents/status`, +/// whose "Not installed" pill the provider row shows) and +/// `check_provider_configured` (behind the "Configured" check on the SAME row, +/// and behind which providers the model picker offers) both call this. Two +/// spellings of it are how the row came to say both things at once. Cheap +/// enough for the provider list: `stat` calls, never a spawn. +pub fn resolve_configured(kind: CodingAgentKind) -> Option { + resolve_binary(kind, configured_command(kind).as_deref()) +} + // --------------------------------------------------------------------------- // The spawning half. Never call these from `from_env` — see the module header. // --------------------------------------------------------------------------- @@ -236,7 +281,7 @@ pub fn configured_command(kind: CodingAgentKind) -> Option { /// `ANTHROPIC_API_KEY` is exported, so probing with the ambient environment /// would describe a credential our own runs will never use. pub async fn probe(kind: CodingAgentKind) -> AgentAvailability { - let path = resolve_binary(kind, configured_command(kind).as_deref()); + let path = resolve_configured(kind); let (version, auth) = match &path { None => (None, AuthState::NotInstalled), @@ -502,4 +547,46 @@ mod tests { ); } } + + /// `from_provider_id` is the exact inverse of `provider_id`, and answers + /// nothing for any other provider — `check_provider_configured` runs it over + /// every provider the daemon serves, and a false match there would hold an + /// API provider to a CLI it has no reason to have. + #[test] + fn from_provider_id_inverts_provider_id_and_nothing_else() { + for kind in CodingAgentKind::all() { + assert_eq!( + CodingAgentKind::from_provider_id(kind.provider_id()), + Some(kind) + ); + } + for other in ["anthropic", "openai", "claude", "Codex", ""] { + assert_eq!(CodingAgentKind::from_provider_id(other), None, "{other:?}"); + } + } + + /// The picker's reason and the turn's error open with the same words. + #[test] + fn the_not_installed_summary_is_the_errors_first_sentence() { + for kind in CodingAgentKind::all() { + let error = super::super::unavailable_error( + kind, + &AgentAvailability { + kind, + provider_id: kind.provider_id().to_string(), + display_name: kind.display_name().to_string(), + path: None, + version: None, + auth: AuthState::NotInstalled, + login_command: kind.login_command().to_string(), + install_hint: kind.install_hint().to_string(), + }, + ) + .to_string(); + assert!( + error.contains(&format!("{}.", kind.not_installed_summary())), + "{kind:?}: {error}" + ); + } + } } diff --git a/crates/biorouter/src/providers/coding_agent/mod.rs b/crates/biorouter/src/providers/coding_agent/mod.rs index 1736022bc..c1e806718 100644 --- a/crates/biorouter/src/providers/coding_agent/mod.rs +++ b/crates/biorouter/src/providers/coding_agent/mod.rs @@ -239,11 +239,11 @@ pub fn unwrap_json_error(raw: &str) -> String { pub fn unavailable_error(kind: CodingAgentKind, availability: &AgentAvailability) -> ProviderError { match &availability.auth { AuthState::NotInstalled => ProviderError::ExecutionError(format!( - "{} is not installed, or is not on a path Biorouter searches.\n\n\ + "{}.\n\n\ Install it with:\n {}\n\n\ If it is already installed somewhere unusual (nvm, volta, bun, asdf), set {} to its \ full path in Settings instead.", - kind.display_name(), + kind.not_installed_summary(), kind.install_hint(), kind.command_config_key(), )), diff --git a/docs/desktop-ui/provider-catalog.md b/docs/desktop-ui/provider-catalog.md index 56d8e9e18..62e71a3d3 100644 --- a/docs/desktop-ui/provider-catalog.md +++ b/docs/desktop-ui/provider-catalog.md @@ -150,6 +150,32 @@ the catalog, and never per row; it runs on mount and on an explicit "Check again a timer. `ProviderCatalog.test.tsx` asserts the call count across a tab change and a minute of fake timers. +### One row, one answer about "installed" + +An agent row carries two statements from two routes: the status pill from +`/coding_agents/status`, and the **Configured** check from `is_configured` on +`/config/providers`. They used to disagree on one line — *"Codex · Not installed ✓ Configured"* +(the 2026-09-10 provider QA run, F6) — because `is_configured` asked only whether the command key +was saved. Three things now keep them together: + +- **The daemon asks the same question twice.** `check_provider_configured` + (`routes/utils.rs`) grants a coding agent `is_configured` only when + `discovery::resolve_configured` finds its CLI, which is the lookup the status probe uses. A + saved key naming a missing CLI is served `is_configured: false` **with** + `unavailable_reason` — the sentence a turn would have failed with. +- **"Check again" re-reads the provider list.** The catalog reads that list once, when the page + opens; a re-check that changed "installed" has changed `is_configured` too, so the hook's + `onRechecked` hands it the page's `refreshProviders`. The mount probe does not — the page + fetched the list at the same moment. +- **The model picker disables the row instead of dropping it.** `SwitchModelModal` lists + every usable provider plus every row with an `unavailable_reason`, the latter as react-select's + own `aria-disabled` option with the reason as its detail line — the private-chat pre-flight's + shape, one level up. A dialog that opens *on* such a provider (the bound one) says why and + will not submit. + +Sign-in is not folded into `is_configured`: learning it spawns the CLI, and +`/config/providers` runs for every provider on every settings open. The pill carries it. + ## Entering without a provider "Explore Biorouter first →" (under the header and repeated at the foot of the first-run @@ -205,8 +231,12 @@ Tests: `ProviderCatalog.test.tsx`, `ProviderCatalog.privacy.test.tsx`, `ProviderCatalog.browserSurface.test.tsx`, `providerOrdering.test.ts`, `ProviderGuard.test.tsx`, `ProviderGuard.browserSurface.test.tsx`, `composerNoProvider.test.ts`, `ChatInput.noProvider.test.tsx`, -`ModelsBottomBar.noProvider.test.tsx`; and on the Rust side -`cargo test -p biorouter --lib -- providers::versa providers::base::type_level_institution`. +`ModelsBottomBar.noProvider.test.tsx`, `SwitchModelModal.unavailable.test.tsx`; and on the Rust +side `cargo test -p biorouter --lib -- providers::versa providers::base::type_level_institution` +and `cargo test -p biorouter-server --lib -- routes::utils routes::config_management`. +⚠ Several filters go **after** `--`: cargo's own `TESTNAME` is a single positional, so +`cargo test … routes::utils routes::config_management` stops at +`unexpected argument 'routes::config_management'` without running anything. ⚠ **Radix's `TabsTrigger` activates on `mousedown`, not on a synthetic `click`.** A `fireEvent.click` alone leaves the panel untouched — and because an unopened panel renders diff --git a/docs/providers/coding-agents/installing-and-signing-in.md b/docs/providers/coding-agents/installing-and-signing-in.md index 4adde0f3f..9bf6c3137 100644 --- a/docs/providers/coding-agents/installing-and-signing-in.md +++ b/docs/providers/coding-agents/installing-and-signing-in.md @@ -126,6 +126,23 @@ Find the real path with `which claude` or `which codex` in the terminal where it > and never appear in the model picker. `llamacpp` solves the same problem the same way with > `LLAMACPP_PORT`. +### What "Configured" means for these two + +A saved key is necessary but not sufficient. `GET /config/providers` reports a coding agent as +configured only when its key is saved **and** the command it names resolves — the same lookup +`GET /coding_agents/status` uses for its **Not installed** state, so the pill and the +**Configured** check on one row can no longer disagree. Point `CODEX_COMMAND` at a path that does +not exist and the row loses its check; the model picker keeps Codex in its list, disabled, with +the reason on the row (*"Codex is not installed, or is not on a path Biorouter searches"*), +instead of offering a provider whose bind would fail. Fix the path, or install the CLI and press +**Check again**, and both come back — the re-check re-reads the provider list as well as the +status. + +Sign-in is deliberately **not** part of "configured". Learning it means running the CLI, and +`GET /config/providers` runs for every provider on every settings open, under the three-second +budget described below. The pill says whether you are signed in; a turn that reaches a signed-out +CLI fails with the vendor's own login command. + ## Verifying from the command line ```bash diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index cc6888530..653e978c0 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -10756,6 +10756,11 @@ } ], "nullable": true + }, + "unavailable_reason": { + "type": "string", + "description": "Why a provider the user HAS set up cannot run right now: a one-line\nsentence for the model picker to print on the row it disables.\n\nSet only when [`Self::is_configured`] is false for a reason other than a\nmissing key — today, a coding agent whose command key is saved and whose\nCLI does not resolve (see `routes::utils::provider_readiness`). `None` for\nevery usable provider and for every provider that is simply not set up,\nwhich the picker leaves out rather than greys out.\n\n⚠ **Only what can be learned without spawning.** A signed-out CLI is not\nreported here: finding that out means running it, and this route runs\nfor every provider on every settings open.", + "nullable": true } } }, diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index b35605218..879961c83 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -2632,6 +2632,21 @@ export type ProviderDetails = { name: string; provider_type: ProviderType; resolved_tier?: ProviderTier | null; + /** + * Why a provider the user HAS set up cannot run right now: a one-line + * sentence for the model picker to print on the row it disables. + * + * Set only when [`Self::is_configured`] is false for a reason other than a + * missing key — today, a coding agent whose command key is saved and whose + * CLI does not resolve (see `routes::utils::provider_readiness`). `None` for + * every usable provider and for every provider that is simply not set up, + * which the picker leaves out rather than greys out. + * + * ⚠ **Only what can be learned without spawning.** A signed-out CLI is not + * reported here: finding that out means running it, and this route runs + * for every provider on every settings open. + */ + unavailable_reason?: string | null; }; export type ProviderEngine = 'openai' | 'ollama' | 'anthropic'; diff --git a/ui/desktop/src/components/onboarding/codingAgentControls.tsx b/ui/desktop/src/components/onboarding/codingAgentControls.tsx index 36276b886..91951aec4 100644 --- a/ui/desktop/src/components/onboarding/codingAgentControls.tsx +++ b/ui/desktop/src/components/onboarding/codingAgentControls.tsx @@ -149,8 +149,19 @@ export interface CodingAgentControls { * would fork processes in the background for as long as the surface is on screen. * Both consumers mount this hook exactly once, which is what keeps that true: * `ProviderCatalog` mounts it at the tab panel, not per row. + * + * `onRechecked` runs after an explicit re-check settles (never after the mount + * probe). The catalog passes its provider-list refresh: the row's "Configured" + * check comes from `GET /config/providers`, which requires the CLI to resolve, + * so a re-check that changed "installed" has changed that answer too — and a + * list read before it would show a check the pill beside it contradicts (F6). + * The catalog also re-checks after any change to an agent's setup, for the + * same reason in the other direction. */ -export function useCodingAgents(onSuccess: (providerId: string) => void): CodingAgentControls { +export function useCodingAgents( + onSuccess: (providerId: string) => void, + onRechecked?: () => void +): CodingAgentControls { const { upsert } = useConfig(); const [agents, setAgents] = useState(null); const [isChecking, setIsChecking] = useState(true); @@ -171,6 +182,15 @@ export function useCodingAgents(onSuccess: (providerId: string) => void): Coding }; }, []); + // ⚠ Through a ref, so `refresh` keeps its empty dependency list. The mount + // effect below re-runs whenever `refresh` changes identity, and a caller's + // fresh closure per render would turn the one mount probe into a probe per + // render — each of which spawns both vendor CLIs. + const onRecheckedRef = useRef(onRechecked); + useEffect(() => { + onRecheckedRef.current = onRechecked; + }, [onRechecked]); + const refresh = useCallback(async (initial: boolean) => { if (initial) { setIsChecking(true); @@ -190,6 +210,9 @@ export function useCodingAgents(onSuccess: (providerId: string) => void): Coding if (mountedRef.current) { setIsChecking(false); setIsRechecking(false); + // Success or not: a probe that failed says nothing about the list, and + // the change that prompted the re-check may still have moved it. + if (!initial) onRecheckedRef.current?.(); } } }, []); diff --git a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx index db0a47719..c3a3052f4 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx @@ -140,6 +140,18 @@ const modelOptionSearchText = (option: ModelOption) => const PUBLIC_MODEL_IN_PRIVATE_CHAT = 'Unavailable: this is a private chat, so only private models may run in it'; +/** + * A provider row in the picker. `unavailableReason` is set for a provider the + * user HAS set up that cannot run right now — the daemon's own sentence, from + * `ProviderDetails.unavailable_reason`. + */ +type ProviderOption = { value: string; label: string; unavailableReason?: string }; + +/** + * Serves a provider row as well as a model row: both carry a `label`, and only a + * model row has a `detail` of its own, so a provider row shows its label alone + * until there is a reason to print beneath it. + */ const renderModelOptionLabel = ( rawOption: unknown, meta: { context: 'menu' | 'value' }, @@ -199,7 +211,7 @@ export const SwitchModelModal = ({ }: SwitchModelModalProps) => { const { getProviders, getProviderModels, read } = useConfig(); const { changeModel, currentModel, currentProvider } = useModelAndProvider(); - const [providerOptions, setProviderOptions] = useState<{ value: string; label: string }[]>([]); + const [providerOptions, setProviderOptions] = useState([]); const [activeProviders, setActiveProviders] = useState([]); const [modelOptionsByProvider, setModelOptionsByProvider] = useState< Record @@ -313,6 +325,30 @@ export const SwitchModelModal = ({ [privacyTier, publicProviderNames] ); + /** + * F6 of the 2026-09-10 provider QA run — the same "pre-flight, not + * post-refusal" rule as {@link blockedReasonFor}, one level up: a whole + * PROVIDER that cannot run. + * + * With `CODEX_COMMAND` pointed at a path that did not exist, Codex stayed + * selectable here, and the bind was then refused by `from_env` with a toast in + * the far corner. The daemon now serves such a row with `is_configured: false` + * and the reason, and the row stays in the list — disabled, with the reason on + * it — rather than vanishing, because a provider the user chose and cannot + * find is exactly the one they need to be told about. The words are the + * daemon's (`CodingAgentKind::not_installed_summary`), the same sentence a turn + * would have failed with. + */ + const unavailableReasonFor = useCallback( + (providerName: string | undefined | null) => { + const reason = providerOptions.find( + (option) => option.value === providerName + )?.unavailableReason; + return reason ? `Unavailable: ${reason}` : null; + }, + [providerOptions] + ); + /** * The form's verdict, DERIVED from the selection on every render — never * computed inside a click (F3, QA of 7c96d796, 2026-09-10). @@ -333,10 +369,19 @@ export const SwitchModelModal = ({ * `attemptedSubmit` only decides whether the "nothing chosen yet" messages * show, because those are prompts, not refusals. `handleSubmit` reads the same * verdict, as the fallback for a submit that arrives some other way. + * + * Two refusals, at two levels, both pre-flight. `blocked` is the MODEL's (a + * public model in a private chat) and shows beside the model field; + * `providerBlocked` is the PROVIDER's (F6: one the user set up that cannot + * run, see {@link unavailableReasonFor}) and shows beside the provider field. + * The same "a disabled row is not a disabled selection" holds one level up: + * the dialog OPENS on such a provider — the bound one, or the one a configure + * form just saved — without anyone picking its disabled row. */ const validation = useMemo(() => { const errors = { provider: '', model: '' }; let blocked: string | null = null; + let providerBlocked: string | null = null; if (usePredefinedModels) { if (!selectedPredefinedModel) { @@ -345,12 +390,18 @@ export const SwitchModelModal = ({ // This branch swaps both selects for a flat radio list and reaches the // same `changeModel`, so it bypasses the option list's pre-flight // exactly the way the custom-model field below does. Guarding only that - // one would leave the identical hole open on the sibling path. - blocked = blockedReasonFor(selectedPredefinedModel.provider); + // one would leave the identical hole open on the sibling path. It has + // no provider field to speak beside, so a provider that cannot run is + // this list's refusal as well. + blocked = + unavailableReasonFor(selectedPredefinedModel.provider) ?? + blockedReasonFor(selectedPredefinedModel.provider); } } else { if (!provider) { errors.provider = 'Select a provider'; + } else { + providerBlocked = unavailableReasonFor(provider); } if (!model) { @@ -362,10 +413,23 @@ export const SwitchModelModal = ({ blocked = blockedReasonFor(provider); } } + if (providerBlocked) errors.provider = providerBlocked; if (blocked) errors.model = blocked; - return { errors, blocked, isValid: !errors.provider && !errors.model }; - }, [model, provider, usePredefinedModels, selectedPredefinedModel, blockedReasonFor]); + return { + errors, + blocked, + providerBlocked, + isValid: !errors.provider && !errors.model, + }; + }, [ + model, + provider, + usePredefinedModels, + selectedPredefinedModel, + blockedReasonFor, + unavailableReasonFor, + ]); // A refusal shows at once, because it is WHY the confirm is disabled; a // prompt to choose something waits for an attempt. One node, rendered under @@ -377,6 +441,18 @@ export const SwitchModelModal = ({ {modelMessage} ) : null; + // The provider field's twin: F6's refusal at once, "Select a provider" only + // after an attempt. The confirm names every refusal in force as its reason. + const providerMessageId = useId(); + const providerMessage = + validation.providerBlocked ?? (attemptedSubmit ? validation.errors.provider : ''); + const confirmDescribedBy = + [ + validation.providerBlocked ? providerMessageId : null, + validation.blocked ? modelMessageId : null, + ] + .filter(Boolean) + .join(' ') || undefined; const handleClose = () => { onClose(); @@ -505,12 +581,19 @@ export const SwitchModelModal = ({ const providersResponse = await getProviders(Boolean(initialProvider)); const activeProviders = providersResponse.filter((provider) => provider.is_configured); setActiveProviders(activeProviders); - // Create provider options and add "Use other provider" option + // Every usable provider, plus every provider the user set up that cannot + // run right now (see `unavailableReasonFor`) — in the daemon's order, so + // a disabled row sits where it always sat instead of sinking to the + // bottom — then "Use other provider". A provider that is simply not set + // up stays out, as before. setProviderOptions([ - ...activeProviders.map(({ metadata, name }) => ({ - value: name, - label: metadata.display_name, - })), + ...providersResponse + .filter((provider) => provider.is_configured || provider.unavailable_reason) + .map(({ metadata, name, is_configured, unavailable_reason }) => ({ + value: name, + label: metadata.display_name, + unavailableReason: is_configured ? undefined : (unavailable_reason ?? undefined), + })), { value: 'configure_providers', label: 'Use other provider', @@ -832,9 +915,31 @@ export const SwitchModelModal = ({ placeholder="Provider, type to search" isClearable isDisabled={hostManaged} + // The private-chat pre-flight's shape, one level up: the row is + // react-select's own `aria-disabled` option, with the reason in + // its detail line — see `unavailableReasonFor`. + formatOptionLabel={(rawOption: unknown, meta) => + renderModelOptionLabel( + rawOption, + meta, + unavailableReasonFor((rawOption as ProviderOption).value) + ) + } + isOptionDisabled={(rawOption: unknown) => + unavailableReasonFor((rawOption as ProviderOption).value) !== null + } /> - {attemptedSubmit && validation.errors.provider && ( -
{validation.errors.provider}
+ {/* Shown before any attempt when the dialog opened ON an + unavailable provider: the reason the button below is inert + has to be readable before the user tries it. */} + {providerMessage && ( +
+ {providerMessage} +
)} {/* Issue #56, DR-26. Whose agreements cover the models under this @@ -879,7 +984,9 @@ export const SwitchModelModal = ({ loadingModels ? 'Loading models…' : 'Select a model, type to search' } isClearable - isDisabled={loadingModels || hostManaged} + isDisabled={ + loadingModels || hostManaged || validation.providerBlocked !== null + } /> {modelMessageNode} @@ -946,7 +1053,8 @@ export const SwitchModelModal = ({ // A barred selection's refusal, beside the field, is this // button's reason for being disabled — so it is also its // description, not merely a sentence that happens to be nearby. - aria-describedby={validation.blocked ? modelMessageId : undefined} + // The provider's (F6) and the model's, when both are in force. + aria-describedby={confirmDescribedBy} > {switching ? 'Switching\u2026' : 'Select model'} diff --git a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.unavailable.test.tsx b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.unavailable.test.tsx new file mode 100644 index 000000000..f6e38c77f --- /dev/null +++ b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.unavailable.test.tsx @@ -0,0 +1,142 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ProviderDetails } from '../../../../api'; +import { SwitchModelModal } from './SwitchModelModal'; + +const mocks = vi.hoisted(() => ({ + getProviders: vi.fn(), + getProviderModels: vi.fn(), + read: vi.fn(), + changeModel: vi.fn(), + currentProvider: 'versa_azure' as string, + currentModel: null as string | null, +})); + +vi.mock('../../../ConfigContext', () => ({ + useConfig: () => ({ + getProviders: mocks.getProviders, + getProviderModels: mocks.getProviderModels, + read: mocks.read, + }), +})); + +vi.mock('../../../ModelAndProviderContext', () => ({ + useModelAndProvider: () => ({ + changeModel: mocks.changeModel, + currentModel: mocks.currentModel, + currentProvider: mocks.currentProvider, + }), +})); + +vi.mock('../predefinedModelsUtils', () => ({ + getPredefinedModelsFromEnv: () => [], + shouldShowPredefinedModels: () => false, +})); + +// ⚠ The REAL react-select, as in `SwitchModelModal.privacy.test.tsx`: +// `role="option"` and `aria-disabled` are react-select's own output, and they +// are exactly what this pre-flight has to produce. The sibling +// `SwitchModelModal.test.tsx` stubs the Select and can see no option at all. + +/** The daemon's sentence for a coding agent whose CLI does not resolve. */ +const NOT_INSTALLED = 'Codex is not installed, or is not on a path Biorouter searches'; + +function provider( + name: string, + displayName: string, + readiness: { is_configured: boolean; unavailable_reason?: string | null } +): ProviderDetails { + return { + name, + provider_type: 'Builtin', + affiliation: null, + resolved_tier: null, + unavailable_reason: null, + ...readiness, + metadata: { + config_keys: [], + default_model: '', + description: '', + display_name: displayName, + known_models: [], + model_doc_link: '', + name, + tier: 'public', + runs_locally: false, + }, + } as ProviderDetails; +} + +/** + * F6 of the 2026-09-10 provider QA run: with `CODEX_COMMAND` pointed at a path + * that did not exist, Codex stayed selectable here. What the daemon now serves + * for that machine: Versa usable, Codex set up but unable to run, OpenAI never + * set up. + */ +const ROWS = [ + provider('versa_azure', 'Versa API Azure', { is_configured: true }), + provider('codex', 'Codex', { is_configured: false, unavailable_reason: NOT_INSTALLED }), + provider('openai', 'OpenAI', { is_configured: false }), +]; + +beforeEach(() => { + vi.clearAllMocks(); + mocks.getProviders.mockResolvedValue(ROWS); + mocks.getProviderModels.mockResolvedValue(['gpt-5.5-2026-04-24']); + mocks.read.mockResolvedValue(''); + mocks.changeModel.mockResolvedValue(true); + mocks.currentProvider = 'versa_azure'; + mocks.currentModel = null; +}); + +/** The provider combobox is the first one; open it from the keyboard. */ +async function openProviderMenu() { + // The bound provider's model loads first, which is also how we know the + // provider list has arrived. + await screen.findByText('gpt-5.5-2026-04-24'); + fireEvent.keyDown(screen.getAllByRole('combobox')[0], { key: 'ArrowDown', code: 'ArrowDown' }); +} + +describe('SwitchModelModal — a provider that cannot run', () => { + it('lists it disabled, with the reason on the row', async () => { + render(); + await openProviderMenu(); + + const codex = await screen.findByRole('option', { name: /Codex/ }); + expect(codex).toHaveAttribute('aria-disabled', 'true'); + expect(codex).toHaveTextContent(`Unavailable: ${NOT_INSTALLED}`); + }); + + // Without this the case above passes for a picker that disables every row. + it('leaves a usable provider selectable, and still omits one never set up', async () => { + render(); + await openProviderMenu(); + + const versa = await screen.findByRole('option', { name: /Versa API Azure/ }); + expect(versa).toHaveAttribute('aria-disabled', 'false'); + expect(versa).not.toHaveTextContent(/Unavailable/); + expect(screen.queryByRole('option', { name: /OpenAI/ })).toBeNull(); + }); + + /** + * The dialog can OPEN on a provider it would never let you pick: the bound + * one, after its CLI went missing. It says why before anything is tried, and + * the switch cannot be submitted — the bind would only be refused by + * `from_env`, with the explanation in a toast in the far corner. + */ + it('explains, and refuses to switch, when it opens on an unavailable provider', async () => { + mocks.currentProvider = 'codex'; + mocks.currentModel = 'gpt-6-astra'; + render(); + + const reason = await screen.findByTestId('switch-model-provider-error'); + expect(reason).toHaveTextContent(`Unavailable: ${NOT_INSTALLED}`); + const confirm = screen.getByRole('button', { name: 'Select model' }); + expect(confirm).toBeDisabled(); + // The F3 pre-flight's contract, kept for this refusal too: the reason beside + // the field is the disabled confirm's description, not a nearby sentence. + expect(confirm.getAttribute('aria-describedby')?.split(' ')).toContain(reason.id); + fireEvent.click(confirm); + expect(mocks.changeModel).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/desktop/src/components/settings/providers/ProviderCatalog.test.tsx b/ui/desktop/src/components/settings/providers/ProviderCatalog.test.tsx index 1f10e466c..3d28ef481 100644 --- a/ui/desktop/src/components/settings/providers/ProviderCatalog.test.tsx +++ b/ui/desktop/src/components/settings/providers/ProviderCatalog.test.tsx @@ -1,4 +1,5 @@ -import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { useState } from 'react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { ProviderDetails, ProviderTier } from '../../../api'; import ProviderCatalog, { defaultCatalogTab, tabFromHint } from './ProviderCatalog'; @@ -11,12 +12,26 @@ const mocks = vi.hoisted(() => ({ ackPrivacyDisclosure: vi.fn(), fetchCodingAgentStatus: vi.fn(), upsert: vi.fn(), + checkProvider: vi.fn(), })); vi.mock('../../../api', async (importOriginal) => ({ ...(await importOriginal>()), getPrivacyDisclosure: mocks.getPrivacyDisclosure, ackPrivacyDisclosure: mocks.ackPrivacyDisclosure, + // The configure form's submit handler validates the saved keys through it. + checkProvider: mocks.checkProvider, +})); +// The configure modal asks which provider is bound before offering "Remove". +vi.mock('../../ModelAndProviderContext', () => ({ + useModelAndProvider: () => ({ + getCurrentModelAndProvider: async () => ({ provider: 'versa_azure', model: 'm' }), + }), +})); +// A successful save opens the model picker; what it shows is its own suite's +// business (`SwitchModelModal.*.test.tsx`), not this one's. +vi.mock('../models/subcomponents/SwitchModelModal', () => ({ + SwitchModelModal: () =>
, })); vi.mock('../../../utils/userAction', () => ({ userActionHeaders: async () => ({ 'X-User-Action': 'test-key' }), @@ -48,6 +63,7 @@ type Backend = { affiliation?: ProviderDetails['affiliation']; resolved_tier?: ProviderTier | null; is_configured?: boolean; + unavailable_reason?: string | null; }; function provider(name: string, backend: Backend = {}, display = name): ProviderDetails { @@ -57,6 +73,7 @@ function provider(name: string, backend: Backend = {}, display = name): Provider provider_type: 'Builtin', affiliation: backend.affiliation, resolved_tier: backend.resolved_tier ?? null, + unavailable_reason: backend.unavailable_reason ?? null, metadata: { config_keys: [], default_model: '', @@ -370,6 +387,163 @@ describe('ProviderCatalog — AI agents', () => { await waitFor(() => expect(mocks.fetchCodingAgentStatus).toHaveBeenCalledTimes(2)); }); + /** + * F6 of the 2026-09-10 provider QA run, as the renderer can reproduce it: the + * row read "Codex · Not installed" and "✓ Configured" on one line. + * + * The check is `is_configured`, and the daemon no longer grants it to a coding + * agent whose CLI does not resolve — but the catalog reads the provider list + * once, when the page opens. Here the list was read while Codex was installed; + * the CLI is then removed and "Check again" says so. Unless the re-check also + * re-reads the list, the stale check sits beside the fresh pill. + */ + it('drops the Configured check when a re-check finds the CLI gone', async () => { + const NOT_INSTALLED = 'Codex is not installed, or is not on a path Biorouter searches'; + mocks.fetchCodingAgentStatus + .mockResolvedValueOnce({ agents: [agent('codex', { state: 'signed_in_subscription' })] }) + .mockResolvedValueOnce({ agents: [agent('codex', { state: 'not_installed' })] }); + + function CatalogWithLiveList() { + const [rows, setRows] = useState([provider('codex', {}, 'Codex')]); + return ( + + setRows([ + provider( + 'codex', + { is_configured: false, unavailable_reason: NOT_INSTALLED }, + 'Codex' + ), + ]) + } + /> + ); + } + + render(); + clickTab('commercial'); + const row = await screen.findByTestId('provider-card-codex'); + await within(row).findByText('Ready · signed in on your subscription'); + expect(within(row).getByText('Configured')).toBeInTheDocument(); + + fireEvent.click(screen.getByTestId('provider-row-toggle-codex')); + fireEvent.click(screen.getByTestId('coding-agent-recheck-codex')); + + await within(row).findByText('Not installed'); + await waitFor(() => expect(within(row).queryByText('Configured')).toBeNull()); + }); + + // The other half: the mount probe must not re-read a list the page fetched + // at the same moment, or opening the catalog costs two provider sweeps. + it('re-reads the provider list only on an explicit re-check', async () => { + const refreshProviders = vi.fn(); + mocks.fetchCodingAgentStatus.mockResolvedValue({ + agents: [agent('codex', { state: 'not_installed' })], + }); + render( + + ); + clickTab('commercial'); + await screen.findByText('Not installed'); + expect(refreshProviders).not.toHaveBeenCalled(); + + fireEvent.click(screen.getByTestId('provider-row-toggle-codex')); + fireEvent.click(screen.getByTestId('coding-agent-recheck-codex')); + await waitFor(() => expect(refreshProviders).toHaveBeenCalledTimes(1)); + }); + + /** + * The same contradiction from the other side, found by driving the running + * app: `CODEX_COMMAND` corrected in the configure form. The save re-read the + * provider list, so the check came back — beside a pill still saying "Not + * installed" from the probe taken when the path was wrong. A change to an + * agent's setup has to re-probe as well. + */ + it('re-probes when an agent’s command key is corrected in the configure form', async () => { + const NOT_INSTALLED = 'Codex is not installed, or is not on a path Biorouter searches'; + const codexKeys = { + config_keys: [{ name: 'CODEX_COMMAND', required: true, secret: false, default: 'codex' }], + }; + const codexRow = (backend: Backend) => { + const row = provider('codex', backend, 'Codex'); + return { ...row, metadata: { ...row.metadata, ...codexKeys } } as ProviderDetails; + }; + mocks.checkProvider.mockResolvedValue({ data: {} }); + mocks.upsert.mockResolvedValue(undefined); + mocks.fetchCodingAgentStatus + .mockResolvedValueOnce({ agents: [agent('codex', { state: 'not_installed' })] }) + .mockResolvedValueOnce({ agents: [agent('codex', { state: 'signed_in_subscription' })] }); + + function CatalogWithLiveList() { + const [rows, setRows] = useState([ + codexRow({ is_configured: false, unavailable_reason: NOT_INSTALLED }), + ]); + return ( + setRows([codexRow({ is_configured: true })])} + /> + ); + } + + render(); + clickTab('commercial'); + const row = await screen.findByTestId('provider-card-codex'); + await within(row).findByText('Not installed'); + + fireEvent.click(within(row).getByRole('button', { name: 'Configure' })); + fireEvent.click(await screen.findByRole('button', { name: 'Save' })); + + await within(row).findByText('Ready · signed in on your subscription'); + expect(within(row).getByText('Configured')).toBeInTheDocument(); + expect(mocks.upsert).toHaveBeenCalledWith('CODEX_COMMAND', 'codex', false); + }); + + // "Use Codex" saves the command key, which is what makes the daemon report it + // configured — so the row behind the picker that opens must re-read the list. + it('re-reads the provider list after "Use" saves an agent’s command key', async () => { + const refreshProviders = vi.fn(); + mocks.upsert.mockResolvedValue(undefined); + mocks.fetchCodingAgentStatus.mockResolvedValue({ + agents: [agent('codex', { state: 'signed_in_subscription' })], + }); + render( + + ); + clickTab('commercial'); + fireEvent.click(await screen.findByTestId('provider-row-toggle-codex')); + fireEvent.click(await screen.findByTestId('coding-agent-connect-codex')); + + await screen.findByTestId('switch-model-modal'); + expect(refreshProviders).toHaveBeenCalledTimes(1); + expect(mocks.upsert).toHaveBeenCalledWith('CODEX_COMMAND', 'codex', false); + }); + + /** The control: a usable agent keeps its check, so the case above is not "never show it". */ + it('keeps the Configured check on an agent that is ready', async () => { + withAgents([agent('claude_code', { state: 'signed_in_subscription' })]); + clickTab('commercial'); + const row = await screen.findByTestId('provider-card-claude_code'); + await within(row).findByText('Ready · signed in on your subscription'); + expect(within(row).getByText('Configured')).toBeInTheDocument(); + }); + it('puts the agents ahead of the pinned API providers, in their fixed order', async () => { withAgents([ // Served the "wrong" way round on purpose: the order is the catalog's, diff --git a/ui/desktop/src/components/settings/providers/ProviderCatalog.tsx b/ui/desktop/src/components/settings/providers/ProviderCatalog.tsx index 06d409582..8b5d92dfe 100644 --- a/ui/desktop/src/components/settings/providers/ProviderCatalog.tsx +++ b/ui/desktop/src/components/settings/providers/ProviderCatalog.tsx @@ -15,6 +15,7 @@ import { SwitchModelModal } from '../models/subcomponents/SwitchModelModal'; import { Tabs, TabsContent, TabsList, TabsTrigger } from '../../ui/tabs'; import type { View } from '../../../utils/navigationUtils'; import { + AI_AGENT_PROVIDER_IDS, getOrderedProviderGroups, type OrderedProviderGroup, type OrderedProviderSection, @@ -250,13 +251,52 @@ export default function ProviderCatalog({ [handleProviderReady, onCommercialSuccess, refreshProviders] ); + const handleAgentConnected = useCallback( + (providerId: string) => { + // "Use " has just saved the agent's command key, which is what + // makes the daemon report it configured. The picker that opens next reads + // the list for itself; the row behind it must not keep the old answer. + refreshProviders?.(); + handleProviderReady(providerId); + }, + [handleProviderReady, refreshProviders] + ); + /** * ⚠ **Mounted once, at the catalog — never per row.** `GET * /coding_agents/status` spawns both vendor CLIs, so one probe per agent row * would fork four processes on every render of this panel. `useCodingAgents` * fetches on mount and on an explicit "Check again" only. + * + * A re-check also re-reads the provider list. The row's "Configured" check is + * `is_configured`, which the daemon only grants a coding agent whose CLI + * resolves, so installing (or removing) a CLI and pressing "Check again" moves + * the check along with the pill — F6 was those two disagreeing on one line. + */ + const agentControls = useCodingAgents(handleAgentConnected, refreshProviders); + const recheckAgents = agentControls.refresh; + + /** + * Re-read everything a change to one provider's setup can have changed. + * + * ⚠ **For a coding agent that is two answers, not one.** Its only config key + * names the CLI, so saving it moves the status pill as well as + * `is_configured`. Re-reading the list alone is what left the row reading + * "Not installed" beside a fresh "✓ Configured" once `CODEX_COMMAND` was + * corrected in the configure form — F6's contradiction, reached from the other + * side, measured in the running app. The explicit re-check re-reads the list + * itself (`onRechecked`), so for an agent it is the one call rather than both. */ - const agentControls = useCodingAgents(handleProviderReady); + const refreshAfterSetupChange = useCallback( + (providerName: string) => { + if (AI_AGENT_PROVIDER_IDS.includes(providerName)) { + void recheckAgents(false); + } else { + refreshProviders?.(); + } + }, + [recheckAgents, refreshProviders] + ); const agentsByProviderId = useMemo(() => { const map = new Map(); for (const agent of agentControls.agents ?? []) map.set(agent.providerId, agent); @@ -362,18 +402,22 @@ export default function ProviderCatalog({ }, []); const onCloseProviderConfig = useCallback(() => { + const closed = configuringProvider?.name; setConfiguringProvider(null); - refreshProviders?.(); - }, [refreshProviders]); + // Closing is not proof nothing changed: a save whose provider check failed + // has still written the key before the error dialog appeared. + if (closed) refreshAfterSetupChange(closed); + else refreshProviders?.(); + }, [configuringProvider, refreshAfterSetupChange, refreshProviders]); const onProviderConfigured = useCallback( (provider: ProviderDetails) => { setConfiguringProvider(null); - refreshProviders?.(); + refreshAfterSetupChange(provider.name); setSwitchModelProvider(provider.name); setShowSwitchModelModal(true); }, - [refreshProviders] + [refreshAfterSetupChange] ); const handleSetView = useCallback( diff --git a/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx b/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx index b735cd2a3..b736f717d 100644 --- a/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx +++ b/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx @@ -44,6 +44,11 @@ export default function ProviderConfigurationModal({ ); const isConfigured = provider.is_configured; + // Something is SAVED for this provider even when it cannot run — a coding + // agent whose command key names a CLI that is not installed is served + // `is_configured: false` with a reason. Removing that saved key is still the + // user's to do; keying "Remove" on `is_configured` alone would strand it. + const hasSavedSetup = isConfigured || Boolean(provider.unavailable_reason); const headerText = showDeleteConfirmation ? `Delete configuration for ${provider.metadata.display_name}` : `Configure ${provider.metadata.display_name}`; @@ -201,7 +206,7 @@ export default function ProviderConfigurationModal({ setIsActiveProvider(false); setShowDeleteConfirmation(false); }} - canDelete={isConfigured && !isActiveProvider} + canDelete={hasSavedSetup && !isActiveProvider} providerName={provider.metadata.display_name} isActiveProvider={isActiveProvider} /> diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx index 12bbb4332..d58a59f02 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx @@ -1,5 +1,6 @@ import React, { useState, useEffect } from 'react'; import { Input } from '../../../../../ui/input'; +import { SecretInput } from '../../../../../ui/secret-input'; import { Select } from '../../../../../ui/Select'; import { Button } from '../../../../../ui/button'; import { SecureStorageNotice } from '../SecureStorageNotice'; @@ -180,9 +181,11 @@ export default function CustomProviderForm({ API Key {!isLocalModel && !initialData && *} - setApiKey(e.target.value)} placeholder={initialData ? 'Leave blank to keep existing key' : 'Your API key'} diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.test.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.test.tsx new file mode 100644 index 000000000..f1466ae57 --- /dev/null +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.test.tsx @@ -0,0 +1,155 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import { useState } from 'react'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ProviderDetails } from '../../../../../../api'; +import DefaultProviderSetupForm, { type ConfigInput } from './DefaultProviderSetupForm'; +import CustomProviderForm from './CustomProviderForm'; + +const mocks = vi.hoisted(() => ({ read: vi.fn() })); + +vi.mock('../../../../../ConfigContext', () => ({ + useConfig: () => ({ read: mocks.read }), +})); + +/** + * F5 of the 2026-09-10 provider QA run: the Versa Bedrock card rendered its + * Secret Access Key as `` with spellcheck on — on screen and + * in the DOM as it was typed. The fixture is that card's real key set + * (`versa_bedrock.rs`): two required secrets, two optional non-secrets with + * defaults behind "Show 2 options". + */ +const bedrock = { + name: 'versa_bedrock', + is_configured: false, + provider_type: 'Builtin', + metadata: { + name: 'versa_bedrock', + display_name: 'Versa API Bedrock', + description: '', + default_model: '', + known_models: [], + model_doc_link: '', + config_keys: [ + { name: 'VERSA_BEDROCK_ACCESS_KEY_ID', required: true, secret: true, default: null }, + { name: 'VERSA_BEDROCK_SECRET_ACCESS_KEY', required: true, secret: true, default: null }, + { + name: 'AWS_ENDPOINT_URL_BEDROCK', + required: false, + secret: false, + default: 'https://unified-api.ucsf.edu/general/awsai', + }, + { name: 'AWS_REGION', required: false, secret: false, default: 'us-west-2' }, + ], + }, +} as unknown as ProviderDetails; + +function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); +} + +/** + * Found by what the QA screenshot shows in each empty field — its placeholder — + * so the same queries run unchanged against the form before the fix, and the + * assertion that fails there is the one about masking rather than a lookup. + */ +const SECRET_ACCESS_KEY = 'VERSA BEDROCK SECRET ACCESS KEY'; +const ACCESS_KEY_ID = 'VERSA BEDROCK ACCESS KEY ID'; + +beforeEach(() => { + vi.clearAllMocks(); + // Nothing stored yet — the state the QA run typed into. + mocks.read.mockResolvedValue(null); +}); + +describe('DefaultProviderSetupForm — secrets are masked', () => { + it('renders a secret parameter as a masked, unchecked, un-autofilled field', async () => { + render(); + + for (const placeholder of [SECRET_ACCESS_KEY, ACCESS_KEY_ID]) { + const input = await screen.findByPlaceholderText(placeholder); + expect(input).toHaveAttribute('type', 'password'); + expect(input).toHaveAttribute('autocomplete', 'off'); + expect(input).toHaveAttribute('spellcheck', 'false'); + } + }); + + // The control: without it, the case above passes for a form that masks + // everything — an endpoint URL nobody can read back is its own defect. + it('leaves a non-secret parameter readable', async () => { + render(); + fireEvent.click(await screen.findByText(/Show 2 options/)); + + expect(screen.getByDisplayValue('us-west-2')).toHaveAttribute('type', 'text'); + expect(screen.getByDisplayValue('https://unified-api.ucsf.edu/general/awsai')).toHaveAttribute( + 'type', + 'text' + ); + }); + + // ⚠ By the config key, never by the words: the reveal toggle's accessible + // name is "Show Secret Access Key", so a query for the words would match the + // button as well as the field. + it('labels each field, so a screen reader names the masked one', async () => { + render(); + const input = await screen.findByPlaceholderText(SECRET_ACCESS_KEY); + expect(screen.getByLabelText(/\(VERSA_BEDROCK_SECRET_ACCESS_KEY\)/)).toBe(input); + }); + + it('stays masked while typing, and reveals only on an explicit toggle', async () => { + render(); + const input = await screen.findByPlaceholderText(SECRET_ACCESS_KEY); + + fireEvent.change(input, { target: { value: 'dummy-not-a-real-secret' } }); + expect(input).toHaveAttribute('type', 'password'); + + const reveal = screen.getByRole('button', { name: 'Show Secret Access Key' }); + expect(reveal).toHaveAttribute('aria-pressed', 'false'); + fireEvent.click(reveal); + expect(input).toHaveAttribute('type', 'text'); + // Revealed is exactly when a spellchecker would otherwise see the value. + expect(input).toHaveAttribute('spellcheck', 'false'); + + fireEvent.click(screen.getByRole('button', { name: 'Hide Secret Access Key' })); + expect(input).toHaveAttribute('type', 'password'); + expect(input).toHaveValue('dummy-not-a-real-secret'); + }); +}); + +describe('the two provider forms mask a key the same way', () => { + // The custom form's "local model" checkbox is Radix Themes', which measures + // itself with a ResizeObserver jsdom does not have. + beforeAll(() => { + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + unobserve() {} + disconnect() {} + } + ); + }); + afterAll(() => { + vi.unstubAllGlobals(); + }); + + it('gives the custom-provider key the same masked field and the same toggle', () => { + const { container } = render( + + ); + const input = container.querySelector('#api-key') as HTMLInputElement; + expect(input).toHaveAttribute('type', 'password'); + expect(input).toHaveAttribute('autocomplete', 'off'); + expect(input).toHaveAttribute('spellcheck', 'false'); + + fireEvent.click(screen.getByRole('button', { name: 'Show API Key' })); + expect(input).toHaveAttribute('type', 'text'); + }); +}); diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx index d3cd29eb0..b809d86ef 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx @@ -1,5 +1,6 @@ import React, { useEffect, useMemo, useState, useCallback } from 'react'; import { Input } from '../../../../../ui/input'; +import { SecretInput } from '../../../../../ui/secret-input'; import { useConfig } from '../../../../../ConfigContext'; import { ProviderDetails, ConfigKey } from '../../../../../../api'; import { Collapsible, CollapsibleContent, CollapsibleTrigger } from '../../../../../ui/collapsible'; @@ -127,7 +128,8 @@ export default function DefaultProviderSetupForm({ .trim(); }; - const getFieldLabel = (parameter: ConfigKey) => { + /** The field's name in words — the label's text, and what a reveal toggle is called. */ + const getFieldName = (parameter: ConfigKey): string => { const name = parameter.name.toLowerCase(); if (name.includes('api_key')) return 'API Key'; if (name.includes('api_url') || name.includes('host')) return 'API Host'; @@ -137,10 +139,20 @@ export default function DefaultProviderSetupForm({ if (parameter_name.startsWith(provider.name.toUpperCase().replace('-', '_'))) { parameter_name = parameter_name.slice(provider.name.length + 1); } - let pretty = envToPrettyName(parameter_name); + return envToPrettyName(parameter_name); + }; + + const getFieldLabel = (parameter: ConfigKey) => { + const name = parameter.name.toLowerCase(); + // The recognised roles are labelled by the role alone; everything else also + // names the config key it writes. + if (['api_key', 'api_url', 'host', 'models'].some((role) => name.includes(role))) { + return getFieldName(parameter); + } + return ( - {pretty} + {getFieldName(parameter)} ({parameter.name}) ); @@ -160,37 +172,49 @@ export default function DefaultProviderSetupForm({ } const renderParametersList = (parameters: ConfigKey[]) => { - return parameters.map((parameter) => ( -
- - ) => { - setConfigValues((prev) => { - const newValue = { ...(prev[parameter.name] || {}), value: e.target.value }; - return { - ...prev, - [parameter.name]: newValue, - }; - }); - }} - placeholder={getPlaceholder(parameter)} - className={`w-full h-9 px-3 rounded-element shadow-none text-sm ${ - validationErrors[parameter.name] - ? 'border-2 border-border-danger' - : 'border border-border-subtle hover:border-border-strong focus:border-border-strong' - } bg-background-default placeholder:text-text-muted text-text-default`} - required={parameter.required} - /> - {validationErrors[parameter.name] && ( -

{validationErrors[parameter.name]}

- )} -
- )); + return parameters.map((parameter) => { + const fieldId = `provider-config-${parameter.name}`; + const fieldProps = { + id: fieldId, + value: getRenderValue(parameter), + onChange: (e: React.ChangeEvent) => { + setConfigValues((prev) => { + const newValue = { ...(prev[parameter.name] || {}), value: e.target.value }; + return { + ...prev, + [parameter.name]: newValue, + }; + }); + }, + placeholder: getPlaceholder(parameter), + className: `w-full h-9 px-3 rounded-element shadow-none text-sm ${ + validationErrors[parameter.name] + ? 'border-2 border-border-danger' + : 'border border-border-subtle hover:border-border-strong focus:border-border-strong' + } bg-background-default placeholder:text-text-muted text-text-default`, + required: parameter.required, + }; + + return ( +
+ + {/* F5: a secret is masked, exactly as the custom-provider form masks + its key — the same primitive, so the two cannot diverge again. A + non-secret parameter (an endpoint, a region) stays readable. */} + {parameter.secret ? ( + + ) : ( + + )} + {validationErrors[parameter.name] && ( +

{validationErrors[parameter.name]}

+ )} +
+ ); + }); }; let aboveFoldParameters = parameters.filter((p) => p.required); diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx index 0758576a0..b0e24ab0e 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx @@ -5,8 +5,9 @@ import { coerceConfigKeyValue } from '../../../configKeyValue'; * Standalone function to submit provider configuration * Useful for components that don't want to use the hook * - * Every value arriving here is a string — the setup form renders each key as an - * ``. `/config/upsert` writes what it is given verbatim, so a + * Every value arriving here is a string — the setup form renders each key as a + * text field (masked for a secret, but a string all the same). `/config/upsert` + * writes what it is given verbatim, so a * string lands in `config.yaml` quoted (`LLAMACPP_PORT: '11543'`) and the * backend's typed `get_param::()` / `get_param::()` cannot read it * back, silently falling through to the default. `coerceConfigKeyValue` turns diff --git a/ui/desktop/src/components/ui/secret-input.tsx b/ui/desktop/src/components/ui/secret-input.tsx new file mode 100644 index 000000000..166aacfea --- /dev/null +++ b/ui/desktop/src/components/ui/secret-input.tsx @@ -0,0 +1,74 @@ +import * as React from 'react'; + +import { cn } from '../../utils'; +import { Eye, EyeOff } from '../icons/app-icons'; +import { Button } from './button'; +import { Input } from './input'; + +type SecretInputProps = Omit, 'type'> & { + /** + * What the field holds, in words — it names the reveal toggle for a screen + * reader ("Show Secret Access Key"), which otherwise hears only an eye. + */ + revealLabel: string; +}; + +/** + * The field a credential is typed into: an API key, a secret access key, a + * token. Masked until the person at the keyboard asks to see it. + * + * ⚠ **The variant lives here, not at the call sites** (design.md P4). The two + * provider forms used to disagree — the custom-provider form masked its key and + * the form every built-in provider shares rendered every parameter, secrets + * included, as plain `type="text"` with spellcheck on, so a Secret Access Key was + * on screen and in the DOM as it was typed. That was a divergence, not a + * decision, and one primitive is what stops it recurring in a third form. + * + * What the primitive guarantees, whatever a caller passes: + * - `type="password"` until the toggle is pressed, and again on every mount — a + * reopened dialog starts masked. + * - `autoComplete="off"` and `spellCheck={false}`, applied AFTER the caller's + * props so neither can be switched back on. Spellcheck matters in the revealed + * state: a checked `text` field hands the value to the platform dictionary. + * + * The toggle only ever reveals what was typed into this field in this session. + * A stored secret is never loaded back into it — the forms show the daemon's + * masked form as a placeholder instead — so there is nothing here that could + * read one back out of the credential store. + */ +const SecretInput = React.forwardRef( + ({ className, revealLabel, disabled, ...props }, ref) => { + const [revealed, setRevealed] = React.useState(false); + + return ( +
+ + +
+ ); + } +); +SecretInput.displayName = 'SecretInput'; + +export { SecretInput };