Six finished fixes: provider error copy and a /models route that answers, two workflow knowledge-selection defects, esbuild discovery leaving its worktree, phrase search on the marketplace shelves, and three doc corrections

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`)
 

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,21 +1278,62 @@ 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.
     #[tokio::test]
     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"
             );
         }

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
+/// `<repo>/.claude/worktrees/<name>/crates/biorouter-mcp` the sixth step is
+/// `<repo>` 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<PathBuf> {
+    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<String>)> {
@@ -949,17 +982,11 @@ fn find_esbuild() -> Option<(String, Vec<String>)> {
             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 `<repo>/.claude/worktrees/<name>/crates/biorouter-mcp` the sixth step
+    /// is `<repo>` 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/<name>` 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"))
+            );
+        }
+    }
 }

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"
+                );
+            }
+        }
+    }
 }