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 - #277
Merged
Merged
Conversation
The three shelves each asked `hay.indexOf(q) !== -1` -- whether the WHOLE
lowercased query occurred verbatim inside one card's text. A one-word query
worked and every phrase failed, so the queries a visitor actually types
returned the empty shelf and a "No skills match your search." line:
"R scripting ggplot visualization" -> 0 cards
"SPOKE knowledge graph" -> 0 cards
This is the same defect `crates/biorouter/src/marketplace/search.rs` closed
for the in-app marketplace, and these were its last two live copies. Rather
than write a fifth matcher, `landing/marketplace-search.js` ports that module
rule for rule: split at whitespace and punctuation, lowercase, de-duplicate,
any-term-matched is a hit, terms under three characters restricted to whole
words, filler dropped unless nothing is left, plural falling back to singular.
Three consequences worth naming.
**License is no longer searchable.** The canonical matcher does not offer it
as a field, and the old haystack folded in `data-license` plus the licence
chip via `textContent` -- so "apache" returned cards that say apache nowhere a
visitor can read as a subject. Callers now hand over a weighted field list
(heading, slug line, prose, org, curated tags) instead of `textContent`.
**Facets are untouched.** The chips still match against the old `hay` string:
they are exact values or curated substrings a chip authored, not prose a
person typed, and baam-privacy-facet.test.mjs pins that discrimination.
**The shelves are not reordered.** `rank()` computes the score, but every hit
is shown here, inside curated sections whose order is editorial -- so DOM
order, which is registry order and the matcher's own last tiebreak, is kept.
Fail-before, on the unmodified page (node --test landing/scripts/baam-search.test.mjs):
x a natural-language skills query finds the skills it names
AssertionError: the phrase returned an empty shelf
x a natural-language extensions query finds the extension it names
AssertionError: the phrase returned an empty shelf
x the license chip is not searchable
AssertionError: a card matched on its license alone
After: 16/16 pass, and baam-privacy-facet.test.mjs stays 14/14.
…scarded
`start_agent` creates the chat first and then runs three steps that can still
fail while it exists. Two of them -- binding the provider and preparing the
workflow prompt -- take the error, call `discard_failed_new_session`, and only
then return. The third, applying the workflow's declared knowledge selection,
used a bare `?`, so it returned straight past the cleanup and left the chat in
the session list.
It was not a rare race. A workflow whose `default` names a base that has since
been deleted fails at this exact step on EVERY start, so a stale workflow
minted one orphan chat per press -- a row the user never asked for and cannot
explain, in a list they do not associate with the workflow that made it.
Copies the siblings' shape verbatim. Adds a test that states the invariant
rather than the fix: between one fallible step and the next, the error path
discards the chat. It reads the handler's source, because 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, and a test that creates and deletes chats there is
worse than no test.
Two hazards the test documents, both of which bit while writing it: the
handler is `start_agent`, not `create_session` (the name in the body is the
SessionManager call it makes); and the source anchor needs its leading newline,
because `include_str!` reads this file including the test, so an anchor without
one matches the copy inside its own string literal and silently slices the test
instead of the handler -- which fails, but names the wrong step.
Fail-before (BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter-server --lib
-- knowledge_selection_tests), with the bare `?` restored:
every_failure_before_a_new_chat_is_returned_discards_it ... FAILED
`apply_workflow_knowledge_selection(` can return an error without
discarding the chat it leaves behind
After: 5 passed, 0 failed.
Scope: the `?` sites after this window -- the two `manager.update(...)` calls
and the refetch -- orphan a chat too and are deliberately left alone. Each is
the session store itself failing, where the discard's own `delete_session`
would be failing for the same reason; what to do there is a separate question,
named in the test so the next reader knows they were seen, not missed.
…mpty
An absent `knowledge_bases` and a present-but-empty one are different
statements, and `workflow/runtime.rs` reads them differently.
`apply_knowledge_selection` returns early on `None` -- so every chat the
workflow starts re-derives a selection from the replaying machine and sees
every base -- and calls `set_visible_kbs(session, &[], PrimaryUpdate::Clear)`
on `Some({})`, hiding them all.
The modal emitted the key only when the selection was non-empty, so the one
gesture that says "no knowledge bases" was stored as the one that says
"whatever you have". `service.rs`'s own capture already got this right
(`an_empty_visible_set_is_captured_not_dropped`); this modal was the remaining
half.
The condition it replaces was asking whether the selection is EMPTY. The
question that decides this is whether the selection is KNOWN, which is a
different question, and conflating the two is what also made a failed read
indistinguishable from an empty one. Three independent ways to know, any one
enough: the user edited it (an edit is a statement, even when what it states is
an empty set); the generation carried the daemon's own block; or this modal's
own read landed and did not fail.
The third is read as `knowledgeBaseItems.length > 0 && !knowledgeSelectionUnread`,
and the first half of that is load-bearing: `knowledgeBaseItems` is set
unconditionally at the top of the read's `.then`, BEFORE the `if (!selection)`
bail, so a non-empty list proves the read ran. `!knowledgeSelectionUnread`
alone does not -- a read that never answers at all, because the effect was torn
down first, sets no flag and would have been captured as "nothing selected". A
machine with no bases therefore still captures nothing, which is what
`knowledge_bases_for_session` answers for that case too: there was no selection
to make, so there is no selection to state.
Fail-before (npx vitest run ...CreateWorkflowFromSessionModal.knowledgeCapture.test.tsx),
with the old emit rule restored:
x stores a chat that has every base switched off as an explicitly empty selection
AssertionError: expected undefined to deeply equal { default: null, visible: [] }
After: 31 passed across the new spec and the existing modal suite.
The new tests live in their own file because the existing suite's knowledge
block is `describe('when its selection cannot be read')`, whose whole fixture
is a read that FAILS; what is under test here is the opposite fixture, and the
two cannot share a `beforeEach`. Both are asserted side by side in the new file,
because the distinction between them is the thing that must not be erased.
One gap is stated in the file rather than left to be rediscovered: the same
gesture made through the picker is not covered. It reaches the identical emit
via `resourceEditsRef`, so the rule above decides it either way, but Radix's
Collapsible around Advanced options would not open from a fresh spec in jsdom,
and WorkflowResourcePicker belongs to another change in flight.
… from
`find_esbuild` walked a flat six ancestors looking for
`ui/desktop/node_modules/.bin/esbuild`, 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, and every esbuild-dependent test passed
locally for a reason CI can never have.
That is the hardest shape of "works on my machine" to notice, because nothing
was broken or missing: the tool the tests found was real and working, it was
just not in the tree under test.
The ascent now stops at the nearest ancestor holding a `.git` entry. Two details
that look like style and are not:
* the candidate is tested BEFORE the boundary, because an ordinary clone keeps
`.git` and `ui/` in the same directory -- a bound that fired first would find
nothing anywhere, which reads as "esbuild is missing" on every machine;
* the boundary tests for the `.git` ENTRY, not for a directory, because a git
worktree's `.git` is a FILE. `is_dir()` would look right and stop at nothing.
Discovery moved into `esbuild_in_checkout(start)` so it can be tested at all:
`find_esbuild` reads `current_dir()`, process-global state a parallel test run
cannot set without racing every other test in the binary.
Fail-before (BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter-mcp --lib --
agent_drafter::bundle::tests::esbuild_discovery), with the flat walk restored:
a_worktree_without_its_own_install_finds_nothing ... FAILED
assertion `left == right` failed: a worktree borrowed the outer checkout's bundler
left: Some(".../BioRouter/ui/desktop/node_modules/.bin/esbuild")
right: None
After: 3 passed; the whole `agent_drafter::bundle` module is 50 passed, 0 failed.
CI is unaffected either way: the `test` job never runs `npm ci`, so no checkout
there has an install to find, and the jobs that do (`serve`, apps-smoke) run
from a repository root where `.git` and `ui/` are siblings -- pinned by
`the_root_of_a_checkout_is_searched_before_the_boundary_stops_it`.
Note on a neighbouring flake, so the next reader does not attribute it here:
`a_timed_out_esbuild_reaps_its_whole_process_group` fails intermittently on this
machine under load (its shim has one second to write a pid file). Measured on
pristine origin/main: 1 failure in 6 runs; with this change: 2 in 4. Same test,
same rate, not caused by this commit, and owned elsewhere.
…OCK_* is shared
Three corrections to guidance a future session would otherwise act on.
**The subagent count was 102; measured, it is 197.** A figure this wrong is worse
than none, because this repo's own habit is to assert "pre + N" against it -- and
against 102 a shortfall of ninety-five reads as a pass. Measured 2026-09-11:
BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter --lib -- subagent
test result: ok. 197 passed; 0 failed; 3708 filtered out
The line now carries the date and says to re-measure.
**Seven `cargo test` commands written as instructions could not run.** `cargo
test` takes ONE positional TESTNAME; a second is rejected before any test runs,
so `cargo test -p X --lib a b` exits 1 with a usage line and nothing is tested.
Five are in br71-execution-plan.md, whose own lines 430-437 explain this exact
hazard and then violate it five times further down; two are in
multi-kb-implementation-plan.md. Each gains the `--` it needed.
Left alone deliberately: five passages that quote the broken form in order to
warn about it, and two occurrences under docs/history/, which are records of what
was run rather than instructions to run it.
**`config_keys.rs`: the stale "cannot RAISE a tier" claim was already fixed**, and
correctly -- `tier()` really does follow the resolved endpoint through
`ucsf_gateway_tier`, so an off-site value demotes and deleting it restores
Private. The classification stands on a different argument, which the comment
already states: only the UCSF gateway's own host reads Private, so no write can
make an off-site endpoint look Private, and a raise is always a return to the
institution's gateway. Nothing to change there.
What WAS unstated is the five `BEDROCK_*` keys. The 2026-09-11 namespacing split
`VERSA_BEDROCK_ENDPOINT` and `VERSA_BEDROCK_REGION` out of the public card's
names precisely to stop one setup steering the other -- and these five were left
shared, read by `versa_bedrock.rs` (Private) and `bedrock.rs` /
`formats/bedrock.rs` (Public) alike. Sharing that a reader has to infer from an
absence looks like an oversight, so the reason is now written down: all four
retry keys are read in one place, `load_retry_config`, and become a
`RetryConfig`; `BEDROCK_OPERATION_TIMEOUT_SECS` is read in
`load_operation_timeout_secs` and becomes a deadline. None reaches the resolved
endpoint `tier()` asks about, and none 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.
The row reasons are read as `_why` by both scans, so they are prose, not
assertions. Verified: `cargo test -p biorouter --lib -- privacy::config_keys`
2 passed, 0 failed.
…dels route that answers
Four defects in what a failed or unconfigured provider tells the user, plus the
endpoint whose name and behaviour disagreed.
**1. A turn no retry can fix no longer invites one.** `stop_notice` is reached by
three routes — the error is fatal, the budget is spent, or retries are off — and
it ended with "Please retry if you think this is a transient or recoverable
error" on all three. On the first that contradicts the sentence above it. Measured
on a vendor model rejection, where the text read *"Claude Code 2.1.235 does not
support this model; version 2.1.251 or newer is required."* and the frame beneath
invited a retry of a request that will fail identically forever. The advice now
follows `is_recoverable`, the same predicate the retry decision uses, so the two
cannot drift. The retried count stays on the retryable branch only: Biorouter
never retries a fatal error, so "already retried **it**" there would name a call
that never happened.
**2. Claude Code gets the catalog hint Codex has.** `unknown_model_hint`, mirroring
`codex.rs`'s. The case is stronger here, and `known_models` already documents why:
`claude --model X -p` **accepts an unrecognized id and only warns**, so a typo
neither fails loudly nor gets a pointer — and when the turn does end badly,
nothing in it names the model. Appended at the three structurally identical sites
(`classify`'s detail, the empty-response error, and `resolve_terminal`'s "produced
no result", the most anonymous failure this provider has). Empty for a listed
model, so a genuine outage on a known id reads as it always did.
**3. An unconfigured Versa Bedrock stops saying three contradictory things.** It
rendered:
error: Error VERSA_BEDROCK_ACCESS_KEY_ID is not configured. Add it under
Versa API Bedrock in Settings..
Please check your system keychain and run 'biorouter configure' again.
...
Three defects in one line. The `..` came from `"Error {e}.{}"` appending an
unconditional stop to text that had one — `end_sentence` already solves exactly
this for the chat path and is now `pub` so there is one rule rather than two
spellings. The `error: Error` stutter came from that same format string, since
`render_error` already prints `error:`. And the three keychain lines answer "why
can't the store give me the key I saved?", which is the wrong question here and on
the sibling arm a contradictory one — the store-refused message says *do NOT
re-enter it* while `biorouter configure` says the opposite.
`keyring_advice` now takes the detail and withholds itself when the credential was
never set. That is a wording test, because the `anyhow::Error` leaving `from_env`
has already discarded the `ConfigError` that knew — so `CREDENTIAL_NEVER_SET` was
added beside the tier helpers and BOTH producers (`versa_bedrock`, `versa_azure`)
now format with it. One spelling, shared by construction rather than by hope.
**4. `GET /config/providers/{name}/models` answers with the models.** It returned
`[]` for nine of the twenty-three builtins, because `fetch_supported_models`
returns `Ok(None)` unless a provider overrides it and those nine do not — under a
name, a description and a `200 Models fetched successfully` that all promise the
model list.
**Ruling: change the behaviour, not the name.** Three reasons. The fallback is
already the right answer in two other places: the declarative-provider branch at
the top of this very handler 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 — so the bug was that the correct rule lived
everywhere except the one place the CLI, an agent, or anything reading the
OpenAPI spec could reach. Renaming would regenerate `openapi.json` and the TS
client (the `Generated API contract` check) and still leave every caller holding
an empty list. And the shape is unchanged here — same path, params and
`Vec<String>` body — so nothing needs regenerating; `git diff --name-only` against
main touches no `openapi.json` and no `api/*.gen.ts`.
⚠ **Measure this with `known_models`, not by grepping `with_models`.** That grep
was the first instrument tried and it said 6 of 9, because
`ProviderMetadata::new` also takes a `model_names` list — `snowflake`, `zai` and
`sagemaker_tgi` looked catalogless and are not. Measured off live metadata
(2026-09-11), all nine declare one: azure_openai 12, aws_bedrock 7, versa_azure 9,
versa_bedrock 5, xai 9, snowflake 8, zai 8, xiaomi_mimo 4, sagemaker_tgi 1.
Fail-before, each with the fix reverted and nothing else changed:
a_rejection_no_retry_can_fix_does_not_invite_one ... FAILED
Ran into this error: ... version 2.1.251 or newer is required.
Please retry if you think this is a transient or recoverable error.
an_empty_answer_on_an_unknown_model_says_which_model ... FAILED
Request failed: `claude` returned an empty response
a_credential_that_was_never_set_is_not_a_keychain_problem ... FAILED
an_unconfigured_provider_renders_one_stop_and_no_stutter ... FAILED
left: "Error VERSA_BEDROCK_ACCESS_KEY_ID is not configured. Add it under
Versa API Bedrock in Settings..\nPlease check your system keychain..."
a_provider_with_no_live_fetch_reports_the_models_it_declares ... FAILED
left: [] right: ["gpt-5.5-2026-04-24", ... 12 models]
After: biorouter --lib 98 passed across the four groups, biorouter-server
routes::config_management 20 passed, biorouter-cli session::builder 20 passed.
…kflow-kb-and-docs
…kflow-kb-and-docs # Conflicts: # crates/biorouter-server/src/routes/config_management.rs
…re two counts Follow-up to merging 133 commits of main into this branch. Nothing here changes behaviour a user can see; each item is a claim in the six commits that the merge made false. **The canonical matcher moved, so the port's three citations were dead.** PR #266 renamed `crates/biorouter/src/marketplace/search.rs` to `crates/biorouter/src/catalog_search.rs`. `landing/marketplace-search.js`, `landing/scripts/baam-search.test.mjs` and `landing/baam.html` each named the old path as the module to read for the reasoning -- the one pointer a future reader of a port most needs. All three now name the new path and say when it moved, the way `ui/desktop/src/components/baam/search.ts` already does on main. **And PR #266 did not only move it -- it changed a rule the port mirrors.** The verbatim-phrase bonus became `written_in`: present as a substring AND not buried inside a longer word at either end, with the boundary imposed only by an end that is itself a word character. The port still asked `indexOf(...) !== -1`, so it had drifted from the module it claims to mirror rule for rule. Ported, with that module's own six assertions run against it. Worth being exact about the blast radius, in both directions: the shelves call `matching`, which collapses `rank` to a membership set, so **nothing on the page moves** -- a visitor sees the same cards in the same order either way. That is also why the drift could have sat there indefinitely, and why the rule is now pinned by a test rather than left to the next reader to notice. **`declared_model_names`' own doc contradicted the comment 100 lines below it.** The route's `Ok(None)` arm still said "six of those nine ship a curated `with_models(...)` catalog" -- the figure produced by the instrument the function's doc-comment exists to warn against, which undercounts by three because `ProviderMetadata::new` also takes a `model_names` list. In a commit whose subject is copy that does not contradict itself, that is the wrong comment to leave standing. It now reads nine, measured off `known_models`, and points at the function that explains the measurement. **The subagent count moved 197 -> 198 while this branch waited.** The whole point of that CLAUDE.md line is that a wrong figure is worse than none, because this repo asserts "pre + N" against it. Re-measured on the merged tree: BIOROUTER_DISABLE_KEYRING=true cargo test -p biorouter --lib -- subagent test result: ok. 198 passed; 0 failed; 3794 filtered out so the line carries 198 and today's date, and now also records that it moved by one in a single week -- which is the argument for re-measuring rather than for trusting any figure written here, this one included. Verified: node --test landing/scripts/baam-search.test.mjs 17/17 (the browser half drives the real page, so the baam.html edit is covered); baam-privacy-facet.test.mjs 14/14; build-registry.mjs --check reports all three outputs current, and no registry datum is touched.
`clippy::string_slice` is `warn` in the workspace `Cargo.toml`, so under
`scripts/clippy-lint.sh`'s `-D warnings` the new
`every_failure_before_a_new_chat_is_returned_discards_it` was a hard error:
`&body[start..end]` panics when either bound falls inside a UTF-8 character.
It cannot here -- both bounds come from `str::find` -- but "cannot" is the
argument the lint exists to stop being made in a comment, so the bounds are taken
through `get` and the reasoning moved into the `expect`.
Measured on the merged tree:
cargo test -p biorouter-server --lib -- routes::agent
test result: ok. 63 passed; 0 failed
cargo clippy --all-targets -- -D warnings \
-A clippy::result_large_err -A clippy::string_slice
exit 0, zero findings
⚠ Those two `-A` flags are NOT this branch's debt and must not be read as a
waiver of it. Both fire on files this branch does not touch, and pristine
origin/main was measured failing the same way:
* `result_large_err` at routes/reply.rs:1863 (`authorize_steer`) -- reproduced
on a detached origin/main worktree, `cargo clippy -p biorouter-server --lib
-- -D warnings`, exit 101, same line;
* `string_slice` at commands/agent.rs:480 (`&digest[..62]`) -- the line is
byte-identical on origin/main and absent from this branch's diff.
Allowing exactly those two is what lets the sweep reach every other target and
show that this branch adds nothing. CI is unaffected either way: rust.yml's
clippy step is `cargo clippy --workspace --all-targets --locked` with no
`-D warnings` and is labelled informational, so both are warnings there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six independent fixes that had been finished on a branch with no PR, brought up to
date with 133 commits of
mainand verified on the merged tree. 1,574 insertionsacross Rust, the desktop UI, the website and docs.
Every count below was measured in this run, on this merge, with
BIOROUTER_DISABLE_KEYRING=trueand multiple filters after a bare--.The six fixes
1.
fix(providers)— error copy that does not contradict itself, and a/modelsroute that answers (a2d94fad)Four defects in what a failed or unconfigured provider tells the user.
stop_noticeinvited a retry on all three of its routes, including the onewhere the error is fatal. Measured on a vendor model rejection, where the text
above read "version 2.1.251 or newer is required" and the frame below invited
a retry of a request that will fail identically forever. The advice now follows
is_recoverable, the same predicate the retry decision uses, so the two cannotdrift; the retried count stays on the retryable branch only.
unknown_model_hintCodex already had — the case isstronger here, because
claude --model X -paccepts an unrecognised id and onlywarns, so a typo neither fails loudly nor gets a pointer.
error: Error … in Settings..plus three keychain lines that answer the wrong question (and, on the sibling
arm, contradict it: the store-refused message says do not re-enter it while
biorouter configuresays the opposite).end_sentenceis nowpubso there isone rule, and
CREDENTIAL_NEVER_SETis shared by both producers and theconsumer by construction rather than by hope.
GET /config/providers/{name}/modelsreturned[]for nine of the twenty-threebuiltins. Behaviour changed, not the name: the same fallback is already correct
in the declarative-provider branch of the very same handler and in the desktop's
fetchModelsForProviders, and the shape is unchanged, so no OpenAPI or TS clientregeneration.
Measured:
-p biorouter --lib -- providers:: agents::mistakes privacy::config_keys837 passed, 0 failed;
-p biorouter-server --lib -- routes::510 passed, 0failed;
-p biorouter-cli --lib -- session::builder20 passed, 0 failed.Each of the five named fail-before tests was confirmed present and
okby name inthe run logs.
2.
docs— a measured subagent count, seven runnable filters, and whyBEDROCK_*is shared (ac504b9d)merged tree it is now 198 (main added one in the 133 commits), so the line
carries 198, today's date, and the fact that it moved by one in a week.
cargo testlines written as instructions could not run:cargo testtakes one positional TESTNAME, so
-p X --lib a bexits 1 with a usage line andtests nothing. Five were in
br71-execution-plan.md, whose own lines 430-437explain that exact hazard and then violate it below; two in
multi-kb-implementation-plan.md. Passages that quote the broken form to warnabout it, and occurrences under
docs/history/, are deliberately left alone.privacy/config_keys.rs: the five sharedBEDROCK_*keys now say why sharing iscorrect — all four retry keys become a
RetryConfig, the timeout becomes adeadline, and none reaches the resolved endpoint
tier()asks about or takespart in signing.
Measured:
-p biorouter --lib -- privacy::config_keyspasses inside the 837 above;-- subagent198 passed, 0 failed.3.
fix(agent-drafter)— esbuild discovery stops at the checkout it is run from (ce3b86a7)find_esbuildwalked a flat six ancestors, and six is exactly far enough to leavea worktree — from
<repo>/.claude/worktrees/<name>/crates/biorouter-mcpthe sixthstep is
<repo>. So a worktree with no install of its own silently borrowed themain checkout's bundler, and every esbuild test passed locally for a reason CI can
never have. The hardest shape of "works on my machine": nothing was missing, the
tool was just not in the tree under test.
The ascent now stops at the nearest ancestor holding a
.gitentry — testedafter the candidate, because an ordinary clone keeps
.gitandui/in the samedirectory, and for the entry rather than a directory, because a worktree's
.gitis a file.
Measured:
-p biorouter-mcp --lib -- agent_drafter::273 passed, 0 failed, 2ignored. The neighbouring flake the commit warns about,
a_timed_out_esbuild_reaps_its_whole_process_group, passed in this run.4.
fix(workflows)— an explicitly empty knowledge selection is saved as empty (4dbd0562)An absent
knowledge_basesand a present-but-empty one are different statementsand
workflow/runtime.rsreads them differently. The modal emitted the key onlywhen the selection was non-empty, so the one gesture that says "no knowledge
bases" was stored as the one that says "whatever you have". The condition it
replaces asked whether the selection is empty; the question that decides this is
whether it is known, which is what also made a failed read indistinguishable
from an empty one.
Measured:
npx vitest run src/components/workflows/__tests__/31 passed (thefigure the commit claims), and the whole
src/components/workflows/directory121 passed, 7 files.
5.
fix(workflows)— a new chat that cannot take its knowledge bases is discarded (15a315b5)start_agentcreates the chat and then runs three steps that can still fail whileit exists. Two take the error, call
discard_failed_new_session, then return; thethird used a bare
?and returned straight past the cleanup. Not a rare race — aworkflow whose
defaultnames a deleted base fails here on every start, so astale workflow minted one orphan chat per press.
Measured:
-p biorouter-server --lib -- routes::agent63 passed, 0 failed;every_failure_before_a_new_chat_is_returned_discards_itconfirmedokby name.6.
fix(baam)— the marketplace shelves answer a phrase, not a substring (9105063e)The three shelves asked
hay.indexOf(q) !== -1, so a one-word query worked andevery phrase failed:
"R scripting ggplot visualization"returned 0 cards and a"No skills match your search." line.
landing/marketplace-search.jsports theapp's canonical matcher rule for rule instead of becoming a fifth implementation.
Licence stops being searchable (the old haystack folded in
data-licenseviatextContent); facet chips are untouched, because they are authored values ratherthan prose a person typed; shelves are not reordered, because their order is
editorial.
Measured:
node --test landing/scripts/baam-search.test.mjs17/17 (16 from thecommit plus one added here, below),
baam-privacy-facet.test.mjs14/14,build-registry.test.mjs56/56,check-consistency.mjsexit 0.⚠
landing/**deploys to biorouter.ucsf.edu on merge. No registry datum istouched —
git diff origin/main..HEAD -- landing/registry*is empty andnode landing/scripts/build-registry.mjs --checkreports all three outputscurrent (37 extensions, 129 skills).
Conflicts and how they were resolved
crates/biorouter-server/src/routes/config_management.rs— the only conflict gitreported. Both sides added a different new function at the same point: ours
declared_model_names, main'sprovider_details(extracted from theproviders()body it also rewrote). Not competing implementations, so both arekept, ours first.
providers()had already auto-merged to call main'sprovider_details, anddeclared_model_namesis reached from the models route atone call site plus two tests — all four verified wired after the resolution.
ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx— noconflict, and the two rules are one rule. #256 is still open, so
maincarriesnone of it; the collision is with the PR, not with the branch. Read side by side
they are complementary halves of the same serialization decision and touch
different regions of the file:
defaultholds when a selection is captured: the realprimary if it is in
visible, otherwisenull— nevervisible[0].is known, empty included.
Composed, a chat with every base switched off yields
{ default: null, visible: [] }— exactly what the new test asserts — and a chat with bases but no primaryyields
{ default: null, visible: [ids] }. Measured rather than argued:git merge-treeof this branch againstorigin/claude/great-panini-672c85exits0 with no conflict list. Whichever lands second needs no resolution.
landing/— no conflict.mainhas nolanding/commits since the mergebase, and the licence-as-tag work another session is doing in
ui/desktop/src/components/baam/search.ts/crates/biorouter/src/catalog_search.rshas not landed either. Worth flagging for that session: this PR's port already
excludes licence from the searched fields and pins it with "the license chip is
not searchable", so
landing/needs no licence fix of its own.CLAUDE.md,docs/agent-loop/designs/br71-execution-plan.mdanddocs/knowledge-base/multi-kb-implementation-plan.mdauto-merged; main's text iskept in full and this branch only edits its own lines.
What did not hold up, and was fixed here
Three claims in the six commits stopped being true, all three because of the merge,
plus one that was wrong when written. Each is a separate follow-up commit
(
28dce896,d5d5a7b0) rather than a quiet amend.crates/biorouter/src/marketplace/search.rs→catalog_search.rs.landing/marketplace-search.js,landing/scripts/baam-search.test.mjsandlanding/baam.htmlall cited the dead path — the one pointer a reader of a portmost needs. Re-aimed, the way
search.tsalready does on main.written_in(word-boundary aware); the port still askedindexOf. Ported, andchecked against that module's own six assertions — all six agree. Blast radius
in both directions: the shelves call
matching, which collapses rank to amembership set, so nothing a visitor sees moves — which is also why the
drift could have sat there indefinitely, hence the new test.
declared_model_namescontradicted its own neighbour. The route'sOk(None)arm still read "six of those nine ship a curated
with_models(...)catalog" —the figure produced by the very instrument the function's doc-comment exists to
warn against, which undercounts by three because
ProviderMetadata::newalsotakes a
model_nameslist. In a commit about copy that does not contradictitself, the wrong comment to leave standing.
clippy::string_slice. The new source-read test used&body[start..end],which the workspace lints
warnandscripts/clippy-lint.shturns into anerror. Now
get+expect, with the boundary argument in theexpectratherthan a comment.
Verification
cargo test -p biorouter --lib -- providers:: agents::mistakes privacy::config_keyscargo test -p biorouter --lib -- subagentcargo test -p biorouter-server --lib -- routes::cargo test -p biorouter-server --lib -- routes::agentcargo test -p biorouter-mcp --lib -- agent_drafter::cargo test -p biorouter-cli --lib -- session::builder(isolatedHOME, literalCARGO_HOME/RUSTUP_HOME)cargo fmt --checknode --test landing/scripts/baam-search.test.mjsnode --test landing/scripts/baam-privacy-facet.test.mjsnode --test landing/scripts/build-registry.test.mjsnode landing/scripts/build-registry.mjs --checknode landing/scripts/check-consistency.mjsnpx vitest run src/components/workflows/npx vitest run src/components/workflows/__tests__/npm run lint:checknpx prettier --checkon both touched frontend filescargo clippy --all-targets -- -D warnings -A clippy::result_large_err -A clippy::string_sliceOn clippy, and why two lints are allowed above
./scripts/clippy-lint.shfails on this branch — and on pristinemain, for thesame two reasons, in files this branch does not touch. Measured rather than
assumed:
result_large_erratroutes/reply.rs:1863(authorize_steer). Reproduced on adetached
origin/mainworktree:cargo clippy -p biorouter-server --lib -- -D warnings→ exit 101, same line.string_sliceatcommands/agent.rs:480(&digest[..62]). Byte-identical onorigin/main, absent from this branch's diff.Allowing exactly those two is what lets the sweep reach every other target and show
this branch adds nothing: it went from 2 findings to 1 after fixing ours, then to 0.
Neither is a CI problem —
rust.yml's clippy step iscargo clippy --workspace --all-targets --lockedwith no-D warningsand is labelled informational — butboth are worth someone's follow-up, and neither belongs in this PR.
Check Commit Messageswill pass: noCo-Authored-Bytrailer of any kind appearsin any of the ten commits.
🤖 Generated with Claude Code