diff --git a/CLAUDE.md b/CLAUDE.md index 194e52d7a..87e60e5af 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1209,7 +1209,10 @@ Test the gate where it is: the unit tests in `agents/agent.rs` (`subagents_enabled_injects_the_workspace_extension_with_the_spawn_tool_only`, `an_explicit_workspace_entry_still_hides_the_spawn_tool_when_delegation_is_off`, `subagents_disabled_injects_nothing`), via -`cargo test -p biorouter --lib -- subagent` (102 tests). +`cargo test -p biorouter --lib -- subagent` (**198 tests, measured 2026-09-12** — this +line said 102 for long enough that a "pre + N" assertion against it would have read a +shortfall of ninety-six as a pass; re-measure rather than trusting the figure, which +moved 197 → 198 between this line being written and the branch carrying it landing). ### Browser access (`biorouter serve`) diff --git a/crates/biorouter-cli/src/session/builder.rs b/crates/biorouter-cli/src/session/builder.rs index b288c14b2..6b37a5498 100644 --- a/crates/biorouter-cli/src/session/builder.rs +++ b/crates/biorouter-cli/src/session/builder.rs @@ -850,9 +850,16 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession { let new_provider = match create(&provider_name, model_config).await { Ok(provider) => provider, Err(e) => { + // `render_error` already prints `error:`, and `end_sentence` already + // knows not to add a second full stop — this line used to do neither, + // so an unconfigured Versa Bedrock read + // `error: Error VERSA_BEDROCK_ACCESS_KEY_ID is not configured. Add it + // under Versa API Bedrock in Settings..` + let detail = e.to_string(); output::render_error(&format!( - "Error {e}.{}", - keyring_advice(&provider_name).await + "{}{}", + biorouter::agents::mistakes::end_sentence(&detail), + keyring_advice(&provider_name, &detail).await )); close_ephemeral_store_with_manager(&session_manager, ephemeral_store_dir).await; process::exit(1); @@ -1198,11 +1205,30 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession { /// has never heard of, which is one of the ways `create` fails — keeps the /// advice: it may well have secrets, and an unnecessary paragraph is a much /// smaller failure than withholding the one that would have helped. -async fn keyring_advice(provider_name: &str) -> &'static str { +/// ⚠ **And it is withheld when the credential was never set at all**, which the +/// provider says in `detail`. Three lines about the system keychain answer the +/// question "why can't the store give me the key I saved?" — they are the wrong +/// answer, and in one case a contradictory one, to "there is no key". An +/// unconfigured Versa Bedrock printed *"VERSA_BEDROCK_ACCESS_KEY_ID is not +/// configured. Add it under Versa API Bedrock in Settings."* and then told the +/// reader to check their keychain and re-run `biorouter configure` — two +/// remedies for a problem that has one, and a third voice after a message that +/// had already named the fix. Worse on the sibling arm: the store-refused +/// message says *do NOT re-enter it*, and "run 'biorouter configure' again" says +/// the opposite. +/// +/// The test is on the wording, via `providers::says_credential_never_set`, +/// because the `anyhow::Error` leaving `from_env` has already discarded the +/// `ConfigError` that knew. That constant is what both ends share so the two +/// cannot drift apart. +async fn keyring_advice(provider_name: &str, detail: &str) -> &'static str { const ADVICE: &str = "\n\ Please check your system keychain and run 'biorouter configure' again.\n\ If your system is unable to use the keyring, please try setting secret key(s) via environment variables.\n\ For more info, see: https://BaranziniLab.github.io/biorouter/docs/troubleshooting/#keychainkeyring-errors"; + if biorouter::providers::says_credential_never_set(detail) { + return ""; + } let has_secrets = biorouter::providers::providers() .await .into_iter() @@ -1252,13 +1278,52 @@ mod tests { async fn a_provider_with_no_secrets_is_not_told_to_check_its_keychain() { for provider in ["claude_code", "codex"] { assert_eq!( - keyring_advice(provider).await, + keyring_advice(provider, "could not find the `claude` command").await, "", "{provider} stores no secret" ); } } + /// A credential that was never set is not a keychain failure, and the three + /// lines that answer one are the wrong answer to it — the provider's own + /// message has already named the fix. + /// + /// The fixture is the message `versa_bedrock::from_env` really produces on + /// its `ConfigError::NotFound` arm, assembled from the same constant the + /// provider formats with, so a reword moves both ends together. + #[tokio::test] + async fn a_credential_that_was_never_set_is_not_a_keychain_problem() { + let detail = format!( + "VERSA_BEDROCK_ACCESS_KEY_ID {}. Add it under Versa API Bedrock in Settings.", + biorouter::providers::CREDENTIAL_NEVER_SET + ); + assert_eq!( + keyring_advice("versa_bedrock", &detail).await, + "", + "the message already says what to do: {detail}" + ); + } + + /// The rendered line, end to end: one full stop and no `error: Error …` + /// stutter. Both were visible on an unconfigured Versa Bedrock, whose text + /// ends in a stop of its own and then met an unconditional one. + #[tokio::test] + async fn an_unconfigured_provider_renders_one_stop_and_no_stutter() { + let detail = format!( + "VERSA_BEDROCK_ACCESS_KEY_ID {}. Add it under Versa API Bedrock in Settings.", + biorouter::providers::CREDENTIAL_NEVER_SET + ); + let rendered = format!( + "{}{}", + biorouter::agents::mistakes::end_sentence(&detail), + keyring_advice("versa_bedrock", &detail).await + ); + assert_eq!(rendered, detail, "nothing should be added to it"); + assert!(!rendered.contains(".."), "{rendered}"); + assert!(!rendered.starts_with("Error "), "{rendered}"); + } + /// …and the advice is kept for everything that does hold one, including a /// provider the registry cannot describe: an unnecessary paragraph is a far /// smaller failure than withholding the one that would have helped. @@ -1266,7 +1331,9 @@ mod tests { async fn a_provider_with_secrets_still_gets_the_keychain_advice() { for provider in ["anthropic", "openai", "no_such_provider_exists"] { assert!( - keyring_advice(provider).await.contains("system keychain"), + keyring_advice(provider, "the credential store refused the read") + .await + .contains("system keychain"), "{provider} must keep the advice" ); } diff --git a/crates/biorouter-mcp/src/agent_drafter/bundle.rs b/crates/biorouter-mcp/src/agent_drafter/bundle.rs index 00bd24893..e1022c1f1 100644 --- a/crates/biorouter-mcp/src/agent_drafter/bundle.rs +++ b/crates/biorouter-mcp/src/agent_drafter/bundle.rs @@ -941,6 +941,39 @@ fn npx_cache_dir() -> PathBuf { std::env::temp_dir().join(format!("biorouter-npx-cache-{}", std::process::id())) } +/// The `ui/desktop/node_modules/.bin/esbuild` of the checkout `start` sits in, +/// if that checkout has one. +/// +/// ⚠ **The ascent stops at the checkout root** — the nearest ancestor holding a +/// `.git` entry — and that bound is why this is a named function rather than a +/// loop inside `find_esbuild`. It used to walk a flat six ancestors, and six is +/// exactly far enough to leave a worktree: from +/// `/.claude/worktrees//crates/biorouter-mcp` the sixth step is +/// `` itself, so a worktree with no install of its own silently borrowed +/// the MAIN checkout's bundler. Every esbuild-dependent test then passed locally +/// for a reason CI can never have, which is the shape of "works on my machine" +/// that is hardest to see: the tool was real, it was just not in the tree under +/// test. +/// +/// ⚠ A git worktree's `.git` is a FILE, not a directory, so the bound tests for +/// the ENTRY. Asking `is_dir()` would look right and stop at nothing. +fn esbuild_in_checkout(start: &Path) -> Option { + let mut dir = Some(start); + while let Some(d) = dir { + let candidate = d.join("ui/desktop/node_modules/.bin/esbuild"); + if candidate.exists() { + return Some(candidate); + } + // Checked after the candidate, so the root's own install still counts — + // an ordinary checkout keeps `.git` and `ui/` in the same directory. + if d.join(".git").exists() { + return None; + } + dir = d.parent(); + } + None +} + /// Locate an esbuild executable. Returns `(program, leading_args)` so the caller /// can support both a direct binary and `npx esbuild`. fn find_esbuild() -> Option<(String, Vec)> { @@ -949,17 +982,11 @@ fn find_esbuild() -> Option<(String, Vec)> { return Some((bin, vec![])); } } - // Dev tree: ui/desktop/node_modules/.bin/esbuild, discovered relative to CWD - // and a couple of ancestors (tests/CLI may run from a subdir). + // Dev tree: the install belonging to THIS checkout, found from the CWD + // upwards because a test or the CLI may run from a subdirectory. if let Ok(cwd) = std::env::current_dir() { - let mut dir: Option<&Path> = Some(cwd.as_path()); - for _ in 0..6 { - let Some(d) = dir else { break }; - let cand = d.join("ui/desktop/node_modules/.bin/esbuild"); - if cand.exists() { - return Some((cand.to_string_lossy().to_string(), vec![])); - } - dir = d.parent(); + if let Some(found) = esbuild_in_checkout(&cwd) { + return Some((found.to_string_lossy().to_string(), vec![])); } } if which("esbuild") { @@ -2960,4 +2987,81 @@ document.getElementById("edit")!.addEventListener("click", () => { } } } + + /// The bundler discovery is confined to the checkout it is run from. + /// + /// ⚠ This is the "passes locally, for a reason CI never has" defect, in the + /// shape that is hardest to see: the tool the tests found was real and + /// working, it was simply not in the tree under test. `find_esbuild` walked a + /// flat six ancestors, and six is exactly far enough to leave a worktree — + /// from `/.claude/worktrees//crates/biorouter-mcp` the sixth step + /// is `` itself. So every esbuild-dependent test in a worktree with no + /// install of its own quietly borrowed the main checkout's bundler, and no + /// failure anywhere said so. + /// + /// Tested through `esbuild_in_checkout` rather than `find_esbuild`, because + /// the latter reads `current_dir()` — process-global state that a parallel + /// test run cannot set without racing every other test in this binary. + mod esbuild_discovery { + use super::super::esbuild_in_checkout; + use tempfile::TempDir; + + /// An outer checkout that HAS an install, and a worktree inside it that + /// does not — the real layout, with `.claude/worktrees/` two levels + /// down and a `.git` FILE rather than a directory. + fn nested_checkouts() -> (TempDir, std::path::PathBuf) { + let root = TempDir::new().unwrap(); + let outer = root.path().join("BioRouter"); + let bin = outer.join("ui/desktop/node_modules/.bin"); + std::fs::create_dir_all(&bin).unwrap(); + std::fs::write(bin.join("esbuild"), "#!/bin/sh\n").unwrap(); + std::fs::write(outer.join(".git"), "gitdir: elsewhere\n").unwrap(); + + let worktree = outer.join(".claude/worktrees/a-worktree"); + std::fs::create_dir_all(worktree.join("crates/biorouter-mcp")).unwrap(); + std::fs::write(worktree.join(".git"), "gitdir: elsewhere\n").unwrap(); + (root, worktree) + } + + #[test] + fn a_worktree_without_its_own_install_finds_nothing() { + let (_root, worktree) = nested_checkouts(); + assert_eq!( + esbuild_in_checkout(&worktree.join("crates/biorouter-mcp")), + None, + "a worktree borrowed the outer checkout's bundler" + ); + } + + #[test] + fn a_checkouts_own_install_is_found_from_a_subdirectory() { + let (_root, worktree) = nested_checkouts(); + let bin = worktree.join("ui/desktop/node_modules/.bin"); + std::fs::create_dir_all(&bin).unwrap(); + std::fs::write(bin.join("esbuild"), "#!/bin/sh\n").unwrap(); + assert_eq!( + esbuild_in_checkout(&worktree.join("crates/biorouter-mcp")), + Some(bin.join("esbuild")), + "the worktree's own install must still be found from a crate directory" + ); + } + + #[test] + fn the_root_of_a_checkout_is_searched_before_the_boundary_stops_it() { + // An ordinary clone keeps `.git` and `ui/` in the same directory, so + // a bound that fired before testing the candidate would find nothing + // anywhere — which would look like "esbuild is missing" on every + // developer machine. + let (_root, worktree) = nested_checkouts(); + let outer = worktree + .parent() + .and_then(|p| p.parent()) + .and_then(|p| p.parent()) + .unwrap(); + assert_eq!( + esbuild_in_checkout(outer), + Some(outer.join("ui/desktop/node_modules/.bin/esbuild")) + ); + } + } } diff --git a/crates/biorouter-server/src/routes/agent.rs b/crates/biorouter-server/src/routes/agent.rs index dbbe25fa9..ea3caf183 100644 --- a/crates/biorouter-server/src/routes/agent.rs +++ b/crates/biorouter-server/src/routes/agent.rs @@ -749,7 +749,18 @@ async fn start_agent( }; if let Some(workflow) = original_workflow.as_ref() { - apply_workflow_knowledge_selection(&state.knowledge_service, &session.id, workflow)?; + // The siblings' shape, and for the same reason. A bare `?` here left the + // chat this function had just created sitting in the session list — a + // row the user never asked for and cannot explain. It is not a rare + // race, either: a workflow whose `default` names a base that has since + // been deleted fails here on EVERY start, so a stale workflow minted one + // orphan per press. + if let Err(error) = + apply_workflow_knowledge_selection(&state.knowledge_service, &session.id, workflow) + { + discard_failed_new_session(&state, &session.id).await; + return Err(error); + } } let workflow_extensions = original_workflow @@ -5711,4 +5722,79 @@ mod knowledge_selection_tests { ); assert_eq!(selection.primary_kb.as_deref(), Some("alpha")); } + + /// Every step that can fail while the new chat already exists, but before it + /// is returned, discards it. An orphan chat is a row the user never asked + /// for and cannot explain, and the knowledge apply was the one step that + /// left one: it used a bare `?` where its two siblings take the error, call + /// `discard_failed_new_session`, and only then return. + /// + /// ⚠ It was not a rare race. A workflow whose `default` names a base that + /// has since been deleted fails here on EVERY start, so a stale workflow + /// minted one orphan per press. + /// + /// **A source read, deliberately.** Reaching the real handler needs an + /// `AppState`, and `AppState::new` calls `AgentManager::instance()` and + /// `KnowledgeService::new_default()` — both of which resolve the developer's + /// own `~/.config/biorouter`. A test that creates and deletes chats there is + /// worse than no test. The shape is what the defect was, so the shape is + /// what is asserted. + /// + /// ⚠ **Scope.** The `?` sites *after* this window — the two + /// `manager.update(...)` calls and the refetch — orphan a chat too and are + /// deliberately not covered: each of those is the session store itself + /// failing, where the discard's own `delete_session` would be failing for + /// the same reason, and deciding what to do there is a separate question. + /// Named here so the next reader knows they were seen, not missed. + #[test] + fn every_failure_before_a_new_chat_is_returned_discards_it() { + let source = include_str!("agent.rs"); + // ⚠ The leading newline is load-bearing: `include_str!` reads this file + // including this test, so an anchor without it matches the copy inside + // this very string literal and slices the test instead of the handler. + let body = source + .split("\nasync fn start_agent(") + .nth(1) + .and_then(|rest| rest.split("\n}\n").next()) + .expect("start_agent production body"); + + // In source order. Each step's window runs to the next one, so the + // assertion is "between one fallible step and the next, the error path + // discards the chat" rather than a count that a fourth step could pass + // without being looked at. + const STEPS: [&str; 3] = [ + "bind_new_session_provider(", + "runtime::prepare_prompt(", + "apply_workflow_knowledge_selection(", + ]; + + let at = |needle: &str| { + body.find(needle) + .unwrap_or_else(|| panic!("`{needle}` is a step of start_agent")) + }; + for (index, step) in STEPS.iter().enumerate() { + let start = at(step); + let end = STEPS.get(index + 1).map_or(body.len(), |next| at(next)); + // `get`, not `&body[start..end]`: the workspace denies + // `clippy::string_slice`, and it is right to — a slice that is not on + // a char boundary panics. Both bounds come from `find`, so they are + // boundaries and this never fires. + let window = body + .get(start..end) + .expect("both bounds come from `find`, so both are char boundaries"); + let discarded = window + .find("discard_failed_new_session") + .unwrap_or_else(|| { + panic!( + "`{step}` can return an error without discarding the chat it leaves behind" + ) + }); + if let Some(returned) = window.find("return Err") { + assert!( + discarded < returned, + "`{step}` returns its error before discarding the chat" + ); + } + } + } } diff --git a/crates/biorouter-server/src/routes/config_management.rs b/crates/biorouter-server/src/routes/config_management.rs index 0c2861e06..ede336862 100644 --- a/crates/biorouter-server/src/routes/config_management.rs +++ b/crates/biorouter-server/src/routes/config_management.rs @@ -1020,6 +1020,32 @@ pub async fn providers() -> Result>, StatusCode> { Ok(Json(providers_response)) } +/// The models a provider declares, for the case where it has no live fetch. +/// +/// Named and separate because it is the answer the route gives most often, and +/// it used to be `Vec::new()`. +/// +/// Measured 2026-09-11 over the 23 registered builtins: **9 do not override +/// `fetch_supported_models`**, so `base.rs`'s `Ok(None)` default is what this +/// route receives for every one of them — and **all nine declare a catalog** the +/// settings grid renders on screen: `azure_openai` 12, `aws_bedrock` 7, +/// `versa_azure` 9, `versa_bedrock` 5, `xai` 9, `snowflake` 8, `zai` 8, +/// `xiaomi_mimo` 4, `sagemaker_tgi` 1. So the route reported "no models" for nine +/// providers that have between one and twelve, under a `200 Models fetched +/// successfully`. +/// +/// ⚠ Do not measure this by grepping for `with_models`. That was the first +/// instrument tried here and it gave 6 of 9, because `ProviderMetadata::new` also +/// takes a `model_names` list — `snowflake`, `zai` and `sagemaker_tgi` looked +/// catalogless and are not. Read `known_models` off the live metadata instead. +fn declared_model_names(metadata: &biorouter::providers::base::ProviderMetadata) -> Vec { + metadata + .known_models + .iter() + .map(|model| model.name.clone()) + .collect() +} + /// One row of `GET /config/providers`. async fn provider_details( metadata: ProviderMetadata, @@ -1104,7 +1130,33 @@ pub async fn get_provider_models( match models_result { Ok(Some(models)) => Ok(Json(models)), - Ok(None) => Ok(Json(Vec::new())), + // ⚠ **`None` means "this provider has no LIVE fetch", not "this provider + // has no models"** — and answering `[]` said the second. Nine of the + // twenty-three builtins do not override `fetch_supported_models`, so its + // `Ok(None)` default reached here; ALL NINE declare a catalog the + // settings grid visibly renders — measured off `known_models`, not by + // grepping `with_models`, which undercounts by three (see + // `declared_model_names`). The + // route was therefore reporting an empty model list for a provider whose + // models were on screen, under a name and a `200 Models fetched + // successfully` that both promise the model list. + // + // Answering from the declared catalog is not a new policy, it is the one + // already in force twice over. The declarative-provider branch at the top + // of this very function returns `config.models` with no live fetch at all; + // and the desktop's `fetchModelsForProviders` prefers + // `metadata.known_models` and only falls back to this route when a + // provider has none. So the fallback WAS the right answer, implemented in + // the one place that could not help the CLI, an agent, or anything reading + // the OpenAPI spec. + // + // Naming was the alternative, and it was rejected: renaming or + // redescribing the route regenerates `openapi.json` and the TS client (the + // `Generated API contract` check), and would still leave every caller + // holding an empty list for a provider that has models. The shape is + // unchanged here — same path, same params, same `Vec` body — so no + // client needs regenerating. + Ok(None) => Ok(Json(declared_model_names(&metadata))), Err(provider_error) => { let status_code = match provider_error { // Permanent misconfigurations - client should fix configuration @@ -1869,6 +1921,74 @@ mod tests { use super::*; + /// `GET /config/providers/{name}/models` is named and documented as the model + /// list, and for nine builtins it answered `[]`. + /// + /// ⚠ The cause is a default, not a failure: `fetch_supported_models` returns + /// `Ok(None)` unless a provider overrides it, and 9 of the 23 registered + /// builtins do not override it. Every one of those nine declares a catalog the + /// settings grid renders, so the route reported "no models" for a provider + /// whose models the user could see on screen — under a `200 Models fetched + /// successfully`. + /// + /// The nine are named rather than derived, because deriving them needs a live + /// instance of each: credentials, and a network call for the ones that do + /// fetch. If one grows a live fetch later it leaves the `Ok(None)` arm and its + /// row here becomes redundant rather than wrong. + #[tokio::test] + async fn a_provider_with_no_live_fetch_reports_the_models_it_declares() { + let all = biorouter::providers::providers().await; + let mut checked = 0; + for name in [ + "azure_openai", + "aws_bedrock", + "versa_azure", + "versa_bedrock", + "xai", + "xiaomi_mimo", + "snowflake", + "zai", + "sagemaker_tgi", + ] { + let Some((metadata, _)) = all.iter().find(|(m, _)| m.name == name) else { + // `aws_bedrock`, `versa_bedrock` and `sagemaker_tgi` are behind + // the `aws-providers` feature. + continue; + }; + assert!( + !metadata.known_models.is_empty(), + "{name} declares a catalog — that is what made `[]` a false answer" + ); + assert_eq!( + declared_model_names(metadata), + metadata + .known_models + .iter() + .map(|model| model.name.clone()) + .collect::>(), + "{name} must report exactly what it declares, in order" + ); + checked += 1; + } + assert!(checked >= 6, "only {checked} of the nine were reachable"); + } + + /// …and nothing is invented for a provider that declares nothing. `litellm` + /// and `ollama` are the two builtins with an empty catalog; both have a live + /// fetch, so neither reaches the arm above — but the projection has to be + /// faithful in that direction too, or the fix reads as "always non-empty". + #[tokio::test] + async fn an_empty_catalog_projects_to_an_empty_list() { + let all = biorouter::providers::providers().await; + for name in ["litellm", "ollama"] { + let Some((metadata, _)) = all.iter().find(|(m, _)| m.name == name) else { + continue; + }; + assert!(metadata.known_models.is_empty(), "{name} declares none"); + assert!(declared_model_names(metadata).is_empty(), "{name}"); + } + } + /// A recovery that could not write says so in BOTH halves of its answer. /// /// The route reported plain success for a config it had just failed to diff --git a/crates/biorouter/src/agents/mistakes.rs b/crates/biorouter/src/agents/mistakes.rs index 2f43c8d75..0990d563f 100644 --- a/crates/biorouter/src/agents/mistakes.rs +++ b/crates/biorouter/src/agents/mistakes.rs @@ -421,18 +421,43 @@ fn recovery_notice(error: &ProviderError, attempt: u32, limit: u32) -> String { ) } -/// The user-facing message when the turn ends on a provider error. The first -/// sentence is unchanged from before BR-66 — only the retry count is new, so the -/// user is not told to "retry" a call Biorouter already silently retried. +/// The user-facing message when the turn ends on a provider error. +/// +/// ⚠ **The retry invitation is only offered for an error a retry could survive.** +/// This notice is reached by three different routes — the error is not +/// recoverable, the budget is spent, or retries are switched off — and it used to +/// end with "Please retry if you think this is a transient or recoverable error" +/// on all three. On the first route that sentence contradicts the one above it: +/// an authentication failure, an unsupported operation or a rejected model name +/// will fail identically forever, and [`is_recoverable`]'s own doc says so. The +/// user is then told, by the same paragraph, that the thing that cannot work +/// might. Measured on a vendor model rejection, where the text above read "no +/// retry will fix it" and the frame below invited one anyway. +/// +/// So the advice follows the same predicate the retry decision does, and cannot +/// drift from it. +/// +/// The retried count stays on the retryable branch only. Biorouter never retries +/// a fatal error, so on the other branch "already retried **it**" would name a +/// call that never happened — the retries it counts were of earlier, different +/// errors in the same turn, and that is exactly the kind of near-true sentence +/// this function is being cleaned of. fn stop_notice(error: &ProviderError, retried: u32) -> String { - let retried_clause = match retried { - 0 => String::new(), - 1 => " Biorouter already retried it once.".to_string(), - n => format!(" Biorouter already retried it {n} times."), + let advice = if is_recoverable(error) { + let retried_clause = match retried { + 0 => String::new(), + 1 => " Biorouter already retried it once.".to_string(), + n => format!(" Biorouter already retried it {n} times."), + }; + format!( + "Please retry if you think this is a transient or recoverable \ + error.{retried_clause}" + ) + } else { + "Retrying will not help: this one returns the same way until its cause changes.".to_string() }; format!( - "Ran into this error: {}\n\nPlease retry if you think this is a transient or \ - recoverable error.{retried_clause}", + "Ran into this error: {}\n\n{advice}", end_sentence(&error.to_string()) ) } @@ -447,7 +472,12 @@ fn stop_notice(error: &ProviderError, retried: u32) -> String { /// /// Deliberately conservative: only `.`, `!`, `?` and a closing quote or bracket /// after one of them count as an ending. Anything else gets the period it needs. -fn end_sentence(text: &str) -> String { +/// +/// `pub` because the CLI needs the same rule: `session --provider` printed +/// `Error .` with an unconditional stop of its own, and produced the +/// identical `…in Settings..` on an unconfigured provider. One rule, not two +/// spellings of it. +pub fn end_sentence(text: &str) -> String { let trimmed = text.trim_end(); let ends = trimmed .chars() @@ -789,6 +819,71 @@ mod tests { assert!(!notice.contains(".."), "{notice}"); } + /// The same measured failure as the test above, read for the other defect it + /// carried. `does not support this model` is a 400, so + /// `classify_provider_details` reads `InvalidRequest` and [`is_recoverable`] + /// says false: Biorouter will not retry it, and neither should the user. The + /// frame invited one anyway — in the paragraph directly beneath text that had + /// just named the two things to run instead. + #[test] + fn a_rejection_no_retry_can_fix_does_not_invite_one() { + let config = MistakeConfig::default(); + let mut tracker = MistakeTracker::default(); + let error = ProviderError::RequestFailed( + "API Error: 400 Claude Code 2.1.235 does not support this model; version 2.1.251 or \ + newer is required." + .to_string(), + ); + assert!(!is_recoverable(&error), "the fixture must be fatal"); + + let ProviderErrorAction::Stop { notice } = tracker.observe_provider_error(&config, &error) + else { + panic!("a fatal error ends the turn"); + }; + assert!( + !notice.contains("Please retry"), + "a turn that cannot be retried must not invite one: {notice}" + ); + assert!( + notice.contains("Retrying will not help"), + "it should say so instead: {notice}" + ); + // The vendor's own text, and its instructions, are still there in full. + assert!( + notice.contains("version 2.1.251 or newer is required."), + "{notice}" + ); + } + + /// The other branch, unchanged: a blip still invites the retry, and still + /// reports the ones Biorouter already spent so the user is not told to retry + /// a call it silently retried three times. + #[test] + fn a_transient_error_still_invites_a_retry_and_names_the_ones_already_spent() { + let config = MistakeConfig::default(); + let mut tracker = MistakeTracker::default(); + let error = ProviderError::ServerError("502".to_string()); + + let mut notice = None; + // Burn the budget, then read the notice the exhausted retry produces. + for _ in 0..=config.provider_error_retries { + if let ProviderErrorAction::Stop { notice: text } = + tracker.observe_provider_error(&config, &error) + { + notice = Some(text); + } + } + let notice = notice.expect("the budget runs out and the turn stops"); + assert!( + notice.contains("Please retry if you think this is a transient"), + "{notice}" + ); + assert!( + notice.contains("Biorouter already retried it"), + "the count survives on this branch: {notice}" + ); + } + #[test] fn end_sentence_only_supplies_a_stop_that_is_missing() { assert_eq!(end_sentence("Server error: 502"), "Server error: 502."); diff --git a/crates/biorouter/src/privacy/config_keys.rs b/crates/biorouter/src/privacy/config_keys.rs index 7d5fbe719..152040652 100644 --- a/crates/biorouter/src/privacy/config_keys.rs +++ b/crates/biorouter/src/privacy/config_keys.rs @@ -98,11 +98,42 @@ pub const NOT_CAPABILITY_CONFIG_KEYS: &[(&str, &str)] = &[ "VERSA_BEDROCK_REGION", "SigV4 signing region; the endpoint, not the region, decides where a request goes", ), - ("BEDROCK_MAX_RETRIES", "retry policy"), - ("BEDROCK_INITIAL_RETRY_INTERVAL_MS", "retry policy"), - ("BEDROCK_BACKOFF_MULTIPLIER", "retry policy"), - ("BEDROCK_MAX_RETRY_INTERVAL_MS", "retry policy"), - ("BEDROCK_OPERATION_TIMEOUT_SECS", "transport timeout"), + // ⚠ These five are the `BEDROCK_*` keys the 2026-09-11 namespacing did NOT + // split, and the fact that they are still SHARED deserves saying rather + // than being inferred from their absence above. `versa_bedrock.rs` (Private) + // and `bedrock.rs` / `formats/bedrock.rs` (Public) all read the same five + // names, so one write tunes both cards at once. That is the exact shape of + // the cross-card bleed `VERSA_BEDROCK_ENDPOINT` and `VERSA_BEDROCK_REGION` + // were namespaced to end — so the reason these were left shared has to be + // a positive one, not an oversight. + // + // It is that they reach nothing a tier depends on. All four retry keys are + // read in one place, `load_retry_config`, and go into a `RetryConfig`; + // `BEDROCK_OPERATION_TIMEOUT_SECS` is read in `load_operation_timeout_secs` + // and becomes a deadline. None of them contributes to the resolved endpoint + // `tier()` asks about, and none of them takes part in signing or + // credentials. They decide how patiently a request is retried and how long + // it may take — not where it goes or who it claims to be. + ( + "BEDROCK_MAX_RETRIES", + "retry policy, shared with the public card", + ), + ( + "BEDROCK_INITIAL_RETRY_INTERVAL_MS", + "retry policy, shared with the public card", + ), + ( + "BEDROCK_BACKOFF_MULTIPLIER", + "retry policy, shared with the public card", + ), + ( + "BEDROCK_MAX_RETRY_INTERVAL_MS", + "retry policy, shared with the public card", + ), + ( + "BEDROCK_OPERATION_TIMEOUT_SECS", + "transport timeout, shared with the public card", + ), ]; /// The files whose `get_param` reads the scan covers: every provider file Task diff --git a/crates/biorouter/src/providers/claude_code.rs b/crates/biorouter/src/providers/claude_code.rs index 596f3fe3b..c411386df 100644 --- a/crates/biorouter/src/providers/claude_code.rs +++ b/crates/biorouter/src/providers/claude_code.rs @@ -134,6 +134,40 @@ pub const CLAUDE_CODE_DEFAULT_MODEL: &str = "claude-fable-5-1"; pub const CLAUDE_CODE_DOC_URL: &str = "https://code.claude.com/docs/en/headless"; +/// The sentence to append to a failed turn when the model name is the likely +/// cause. Codex's twin, `codex::unknown_model_hint`, and written to the same +/// rules; read that one for the reasoning about hinting on the failure path +/// rather than refusing before the call. +/// +/// The case for it is *stronger* here than next door, and the reason is recorded +/// at length in `known_models` above: `claude --model X -p` **accepts an unknown +/// id and merely warns** — +/// +/// "X" is not a model this version of Claude Code recognizes, so auto-compact +/// will keep this session within 200k tokens +/// +/// — so a typo neither fails loudly nor gets a pointer. When the turn does then +/// end badly, nothing in the message names the model, and the frame above it +/// invites a retry that cannot come true. Codex got this hint; the structurally +/// identical failures here did not. +/// +/// ⚠ **Only ever additive to text the vendor already produced.** It never +/// replaces a real explanation, and it is empty for a listed model, so a genuine +/// outage on a known id reads exactly as it did before. +fn unknown_model_hint(model: &str) -> String { + let known = known_models(); + if known.iter().any(|m| m.name == model) { + return String::new(); + } + let names: Vec<&str> = known.iter().map(|m| m.name.as_str()).collect(); + format!( + " — and `{model}` is not one of the models this build knows Claude Code to \ + offer ({}). `claude` accepts an unrecognized name with only a warning, so \ + a typo fails here rather than at the point it was made", + names.join(", ") + ) +} + /// Models advertised in the picker, with each id's measured context window. /// /// `ProviderMetadata::with_models` is used rather than `::new` because `::new` @@ -609,6 +643,7 @@ impl ClaudeCodeProvider { .or_else(|| Some(stderr.trim().to_string())) .filter(|s| !s.is_empty()) .unwrap_or_else(|| "`claude` reported an error".into()); + let detail = format!("{detail}{}", unknown_model_hint(model)); let category = result .get("subtype") .and_then(Value::as_str) @@ -622,9 +657,10 @@ impl ClaudeCodeProvider { .unwrap_or_default() .to_string(); if text.trim().is_empty() { - return Err(ProviderError::RequestFailed( - "`claude` returned an empty response".into(), - )); + return Err(ProviderError::RequestFailed(format!( + "`claude` returned an empty response{}", + unknown_model_hint(model) + ))); } let usage = parse_usage(result.get("usage")); @@ -1267,7 +1303,7 @@ async fn pump_claude_stdout(inputs: PumpInputs) { // The authoritative usage (and any failure) goes last, so it is the // snapshot the agent keeps. - let terminal = resolve_terminal(terminal, stderr_task).await; + let terminal = resolve_terminal(terminal, stderr_task, &model_name).await; let _ = out_tx.send(terminal.map(|usage| (None, Some(usage), None))); } @@ -1279,6 +1315,7 @@ async fn pump_claude_stdout(inputs: PumpInputs) { async fn resolve_terminal( terminal: Option>, stderr_task: tokio::task::JoinHandle, + model: &str, ) -> Result { match terminal { Some(terminal) => terminal, @@ -1288,11 +1325,17 @@ async fn resolve_terminal( None => { let detail = stderr_task.await.unwrap_or_default(); let detail = detail.trim(); - Err(ProviderError::RequestFailed(if detail.is_empty() { + let base = if detail.is_empty() { "`claude` produced no result".to_string() } else { format!("`claude` produced no result: {detail}") - })) + }; + // The most anonymous failure this provider has — a child that said + // nothing at all — so it is the one that most needs the model named. + Err(ProviderError::RequestFailed(format!( + "{base}{}", + unknown_model_hint(model) + ))) } } } @@ -2133,6 +2176,77 @@ mod tests { assert_eq!(parse_usage(None).total_tokens, None); } + /// Codex's `a_failed_turn_names_an_unknown_model_and_the_ones_that_exist`, + /// for the provider that needs it more. `claude` accepts an unrecognized + /// `--model` with only a warning, so a typo produces a turn that fails with + /// nothing in it naming the model — and this provider is the only thing in + /// the stack that knows which names it believes Claude Code offers. + #[test] + fn a_failed_turn_names_an_unknown_model_and_the_ones_that_exist() { + let hint = unknown_model_hint("claude-opus-99"); + assert!( + hint.contains("claude-opus-99"), + "the hint must name the model that was asked for: {hint}" + ); + // ⚠ Assert against the parenthesised catalog only, never the whole hint. + // The hint embeds the id that was asked for, and a plausible typo shares + // a prefix with a real id — so a bare `hint.contains("claude-opus")` + // passes on a hint that lists no models at all. Codex's twin carries the + // same warning for the same reason. + let catalog = hint + .split_once('(') + .unwrap_or_else(|| panic!("the hint must carry a parenthesised catalog: {hint}")) + .1; + for expected in known_models().iter().map(|m| m.name.clone()) { + assert!( + catalog.contains(&expected), + "the fix has to be in the message: {expected} is missing from {hint}" + ); + } + } + + /// ⚠ And it must stay SILENT for a model that is known, or every unrelated + /// failure — a rate limit, a dropped connection — gains a paragraph about + /// model names and sends the reader after the wrong thing. + #[test] + fn a_known_model_adds_nothing_to_a_failure() { + for m in known_models() { + assert_eq!( + unknown_model_hint(&m.name), + "", + "{} is a declared model and must not be second-guessed", + m.name + ); + } + } + + /// The hint reaches the message a user actually sees. An empty answer from + /// the child is the most anonymous failure this provider has, and it named + /// nothing at all before. + #[test] + fn an_empty_answer_on_an_unknown_model_says_which_model() { + let lines = vec![ + r#"{"type":"result","subtype":"success","is_error":false,"result":"","usage":{}}"# + .to_string(), + ]; + let error = provider() + .parse_result_object("claude-opus-99", &lines, "", exit_ok()) + .expect_err("an empty answer is a failure"); + let text = error.to_string(); + assert!(text.contains("claude-opus-99"), "{text}"); + assert!(text.contains("only a warning"), "{text}"); + + // And a listed model's identical failure is untouched. + let known = provider() + .parse_result_object(CLAUDE_CODE_DEFAULT_MODEL, &lines, "", exit_ok()) + .expect_err("an empty answer is a failure") + .to_string(); + assert!( + known.ends_with("`claude` returned an empty response"), + "a known model's failure must read as it always did: {known}" + ); + } + /// A real captured `result` frame parses, and the usage row is attributed to /// this provider rather than left for the model name to decide. #[test] diff --git a/crates/biorouter/src/providers/mod.rs b/crates/biorouter/src/providers/mod.rs index 5e2a39b63..8a52ceea1 100644 --- a/crates/biorouter/src/providers/mod.rs +++ b/crates/biorouter/src/providers/mod.rs @@ -136,6 +136,26 @@ pub(crate) fn is_loopback_host(url: &str) -> bool { } } +/// The phrase a provider uses when a credential was **never set**, as distinct +/// from "the credential store refused to hand it over". The two need opposite +/// responses from the user — add the key, versus do NOT re-enter it and answer +/// the Keychain prompt — and `versa_bedrock::from_env` carries a comment saying +/// exactly that about its own two arms. +/// +/// ⚠ **It exists so the wording has ONE spelling.** The `anyhow::Error` that +/// leaves `from_env` has already discarded the `ConfigError` behind it, so a +/// caller further out — `biorouter-cli`'s `keyring_advice`, which decides whether +/// to print three lines about the system keychain — has nothing but the text to +/// go on. A literal repeated at both ends is a literal that drifts at one end; +/// the producers format with this constant and the consumer matches on it. +pub const CREDENTIAL_NEVER_SET: &str = "is not configured"; + +/// Whether `text` is a provider saying a credential was never set. See +/// [`CREDENTIAL_NEVER_SET`] for why this is a wording check and not a type one. +pub fn says_credential_never_set(text: &str) -> bool { + text.contains(CREDENTIAL_NEVER_SET) +} + /// The tier of a provider that reaches the UCSF gateway and nothing else. /// /// Demotion only, never promotion: each Versa provider's endpoint is diff --git a/crates/biorouter/src/providers/versa_azure.rs b/crates/biorouter/src/providers/versa_azure.rs index 7c681196d..95d119d3e 100644 --- a/crates/biorouter/src/providers/versa_azure.rs +++ b/crates/biorouter/src/providers/versa_azure.rs @@ -415,7 +415,9 @@ impl VersaAzureProvider { VersaAzureCredentialSource::ApiKey => { let key = config .get_secret::("VERSA_AZURE_API_KEY") - .map_err(|_| anyhow::anyhow!("VERSA_AZURE_API_KEY is not configured"))?; + .map_err(|_| { + anyhow::anyhow!("VERSA_AZURE_API_KEY {}", super::CREDENTIAL_NEVER_SET) + })?; anyhow::ensure!(!key.trim().is_empty(), "VERSA_AZURE_API_KEY is empty"); Some(key) } diff --git a/crates/biorouter/src/providers/versa_bedrock.rs b/crates/biorouter/src/providers/versa_bedrock.rs index 0a78d9c76..03f4b90d5 100644 --- a/crates/biorouter/src/providers/versa_bedrock.rs +++ b/crates/biorouter/src/providers/versa_bedrock.rs @@ -183,7 +183,8 @@ impl VersaBedrockProvider { match config.get_secret::(name) { Ok(value) => Ok(value), Err(crate::config::ConfigError::NotFound(_)) => Err(anyhow::anyhow!( - "{name} is not configured. Add it under Versa API Bedrock in Settings." + "{name} {}. Add it under Versa API Bedrock in Settings.", + super::CREDENTIAL_NEVER_SET )), Err(error) => Err(anyhow::anyhow!( "Could not read {name} from the credential store: {error}\n\n\ diff --git a/docs/agent-loop/designs/br71-execution-plan.md b/docs/agent-loop/designs/br71-execution-plan.md index b1a73f3f2..43a979ce3 100644 --- a/docs/agent-loop/designs/br71-execution-plan.md +++ b/docs/agent-loop/designs/br71-execution-plan.md @@ -1490,7 +1490,7 @@ with no error anywhere. ```bash cargo test -p biorouter --lib session::session_manager -cargo test -p biorouter --lib agents::knowledge_tool knowledge::conversation_ingest +cargo test -p biorouter --lib -- agents::knowledge_tool knowledge::conversation_ingest ``` Expected: **PASS**, including the pre-existing migration tests. Two of them name a @@ -3705,7 +3705,7 @@ does need adding is `async-trait`, in Task 9; see there.) - [ ] **Step 5: Run tests** -Run: `cargo test -p biorouter-server --lib workspace::turn state::tests::turn_guard_exposes_its_turn_id` +Run: `cargo test -p biorouter-server --lib -- workspace::turn state::tests::turn_guard_exposes_its_turn_id` Expected: `test result: ok. 6 passed` (three lifecycle tests, the seed-is-not-a-write test, the abort classifier, and the `TurnGuard::turn_id` accessor). @@ -5235,7 +5235,7 @@ just the new file) ```bash cargo test -p biorouter-server --lib routes::reply -cargo test -p biorouter-server --lib routes::session_events workspace:: +cargo test -p biorouter-server --lib -- routes::session_events workspace:: cargo test -p biorouter-server --lib # every server unit test cargo test -p biorouter --lib agents::agent # the agent side of the turn contract @@ -11547,7 +11547,7 @@ for Task 24's `workspace_open` line. - [ ] **Step 6: Run tests** -Run: `cargo test -p biorouter --lib agents::agent agents::workspace_extension agents::extension_manager` +Run: `cargo test -p biorouter --lib -- agents::agent agents::workspace_extension agents::extension_manager` Expected: PASS (5 new agent tests, including the persistence exclusion; the `available_tools` tests at `extension_manager.rs:2456-2545` still green — this task relies on them, it does not change them). @@ -11773,7 +11773,7 @@ five fields used above.) - [ ] **Step 3: Run to verify failure** -Run: `BIOROUTER_PATH_ROOT=$(mktemp -d) cargo test -p biorouter --lib agents::workspace_extension agents::subagent_tool agents::agent` +Run: `BIOROUTER_PATH_ROOT=$(mktemp -d) cargo test -p biorouter --lib -- agents::workspace_extension agents::subagent_tool agents::agent` Expected: FAILURES — but **not** "no `subagent` tool on the extension": Task 18 Step 4 already appended `create_subagent_tool(&[])` to `get_tools()`, so `the_workspace_extension_advertises_the_spawn_tool_under_its_existing_name`'s diff --git a/docs/knowledge-base/multi-kb-implementation-plan.md b/docs/knowledge-base/multi-kb-implementation-plan.md index 8221e014b..34d4f58cb 100644 --- a/docs/knowledge-base/multi-kb-implementation-plan.md +++ b/docs/knowledge-base/multi-kb-implementation-plan.md @@ -411,7 +411,7 @@ with: - [ ] **Step 2: Run the tests — see them fail** ```bash -cargo test -p biorouter-mcp --lib knowledge::service::tests::session_hidden_override_can_be_explicitly_empty knowledge::service::tests::hidden_kbs_can_be_scoped_per_session +cargo test -p biorouter-mcp --lib -- knowledge::service::tests::session_hidden_override_can_be_explicitly_empty knowledge::service::tests::hidden_kbs_can_be_scoped_per_session ``` Expected: a compile error for the missing method. @@ -3682,7 +3682,7 @@ git commit -m "docs(knowledge): describe the merged set-plus-primary model" cargo test -p biorouter-mcp --lib knowledge:: cargo test -p biorouter-mcp --test knowledge_macros_e2e --test knowledge_registered --test knowledge_revert_integration cargo test -p biorouter-server --test knowledge_routes -cargo test -p biorouter-server --lib routes::apps routes::agent routes::knowledge +cargo test -p biorouter-server --lib -- routes::apps routes::agent routes::knowledge cargo test -p biorouter-cli cargo test -p biorouter --lib knowledge:: --lib agents::knowledge_tool cargo test -p biorouter --test knowledge_e2e diff --git a/landing/baam.html b/landing/baam.html index c15aff88d..5bf21ae6f 100644 --- a/landing/baam.html +++ b/landing/baam.html @@ -7,6 +7,9 @@ + +