Close four measured defects in the config and workspace surfaces

[Broccolito]

Four measured defects in the config and workspace surfaces. One commit per item, so any one can be reverted alone. Item 4 turned out to be already fixed on main and is not in the diff.

⚠ Item 3 is security-sensitive (a partial credential in an HTTP response) — please review that commit as such, and note it does not conflict with #237.

#	Severity	Item	Status	Commit
1	HIGH-ish	workspace_set_tools wiped a cold chat's extension roster and reported success	Fixed	c0f7a248
2	MEDIUM	/config/set_provider did not write the provider/model pair atomically	Fixed	e4fcafa9
3	LOW, security	/config/read revealed the first ~8 characters of a masked secret	Fixed	f2e8452f
4	MEDIUM	/config/recover's "could not be written" note is sticky after the file heals	Already fixed on main, skipped	—

No route's response shape changed, so no OpenAPI/TS-client regeneration is needed (/config/read still answers { maskedValue: string }).

---

1 — A cold chat's extension roster survives workspace_set_tools (c0f7a248)

handle_set_tools fetched the target's agent with AgentManager::get_or_create_agent, whose miss path mints a bare, extension-less agent and caches it under the target's id. apply_extension_changes then finished with Agent::persist_extension_state — session_extensions::record, a whole-key REPLACE of enabled_extensions.v0 built from the live manager's snapshot. That basis is right for the reply loop (the chat is open, its manager is its roster, and a removal is expressed by an absence) and wrong for a conversation that is not open, where the manager is an agent the handler just created empty.

Fail-before

set_tools_on_a_cold_conversation_keeps_the_extensions_it_did_not_name — a session row seeded with three Stdio extensions, asserted to have no live agent (peek_agent is None), then workspace_set_tools { remove_extensions: ["roster-beta"] } from a private caller:

assertion `left == right` failed: one named removal replaced the whole saved roster
  left: []
 right: ["roster-alpha", "roster-gamma"]

Three extensions in, zero out — and the call answered Applied to session <id>: -roster-beta. add_extensions is the same shape in the other direction (the roster becomes exactly the one added extension). Data loss reported as success is what makes this the worst item here: nothing at the call site can tell.

Two more, both left: Some(false) right: Some(true) on the same revert:

• set_tools_refuses_a_cold_conversation_a_removal_it_has_nothing_for — a cold removal of a name the chat never had answered Applied to session <id>: -ghost-fixture. The live branch has refused this since QA finding F4; the cold branch continued past the existence check, so the phantom was reported as applied and took the rest of the roster with it.
• a_saved_roster_this_build_cannot_read_is_refused_rather_than_replaced — a roster the build cannot parse answered Applied to session <id>: -roster-alpha. and was overwritten with {"extensions": []}.

How the revert was staged (the first attempt measured the wrong thing)

Reverting to Agent::persist_extension_state verbatim fails in a unit test for an unrelated reason: AgentManager::instance() carries SessionManager::instance() while the test's WorkspaceClient carries its own temp store, so the write lands Session not found. In the daemon both are the same on-disk store, which is why the wipe is real in production. The faithful revert asks session_extensions::record (exactly what persist_extension_state calls) through the client's store, removing the harness artifact. The numbers above are from that run.

The fix

1. session_extensions::apply_saved_roster_delta writes stored − remove + add onto the session row instead of snapshotting a manager. Right for an open chat too: this tool knows exactly what it changed, and the rest of the row is state it was never asked to touch. This is also what closes the hole reached the other way — a cold {provider, model} call still mints a bare agent, so a peek_agent-only fix would have had the next call find one and trust it.
2. session_extensions::saved_roster_of separates "nothing saved" from "saved and unreadable". EnabledExtensionsState::from_extension_data ends in .ok() and collapses them; to a writer about to replace the key they are opposites. An unreadable roster now fails the call loudly rather than being overwritten.
3. The handler peeks, never creates for the extension dimension, so a cold conversation is changed where its roster lives and an agent is minted only for the provider switch, which genuinely needs one. The pre-flight's cold branch answers the existence half against the saved roster, below both privacy arms, so the inspector's card and the handler still agree (set_tools_preflight_refusal runs the same function).

Gates preserved, deliberately

Gate F1's unload arm on the cold branch asks manageability_refusal(name, None, cap) — the same predicate with the same None the pre-flight's cold branch asks. Judging a cold removal against the saved roster would have loosened it: an unknown name reading Private is what stops the refusal being an existence oracle over exactly the private names Gate E hides. The pinned-source needle .assert_extension_manageable(name, cap) is intact, and neither repo-grep census counts an ident this touches (manageability_refusal appears in both census files only in prose; the private_or_absent_refusal row counts extension_manager.rs alone). No census row was added or re-counted.

Also deduplicated: the "is not enabled in conversation" refusal now has one definition (Self::nothing_to_remove) asked by both branches instead of two copies — this file's own history of two doors drifting is the reason.

---

2 — The default provider and its model are one write (e4fcafa9)

set_biorouter_provider(...) then set_biorouter_model(...) is two set_param calls, hence two save_values calls, so config.yaml held the new provider beside the old model in between — measured at ~55 ms of versa_azure next to gpt-6-astra during one switch. A chat started in that window binds a pair nobody chose, and the provider decides the privacy capability a session starts at, so the mismatch is privacy-relevant and not only cosmetic.

Mechanism: the existing one, extended — deliberately not a second temp-file-and-rename. save_values already stages under a per-process, per-call name (Config::staging_path, whose doc records the shared-config.tmp race and the Windows ERROR_LOCK_VIOLATION it caused) and renames into place. File-level atomicity was never missing; one write carrying both keys was. So Config::set_params holds the same guard across one load, the inserts and one save_values, exactly as set_param does, and set_biorouter_provider_and_model is the pair that only ever means anything together.

All six sites that wrote the two keys in sequence now use it: the route, both biorouter configure paths, biorouter models, and the OpenRouter and Tetrate sign-up flows.

Fail-before

the_default_provider_and_its_model_are_one_config_write asserts the write count, because the count is the mechanism — two writes is a window by construction, one has none. Reading the file back cannot see this: the final state was always correct, which is exactly why the gap survived.

assertion `left == right` failed: the pair was written in more than one pass,
so config.yaml held a mixed provider/model pair in between
  left: 2
 right: 1

---

3 — ⚠ SECURITY: a masked secret reveals none of the secret (f2e8452f)

#237 checked first, as asked. git diff origin/main...origin/claude/sweet-pare-965d18 touches 54 files and neither routes/config_management.rs nor config/base.rs is among them (empty diff for both paths). No conflict with that branch.

mask_secret showed min(len / 2, 8) leading characters, so a 40-character key came back as eight real characters followed by asterisks — {"maskedValue":"Y2EzNTgy********…"} — a partial credential in the one response whose whole purpose is not to contain one, and a prefix long enough to identify which key is stored and to narrow a search for the rest.

It is now a fixed eight-bullet placeholder carrying none of the secret's bytes. The length is fixed for the same reason: how long a stored credential is fingerprints which kind it is. The response shape is unchanged, and the only renderer puts the string straight into a field (DefaultProviderSetupForm.tsx:108), so nothing needed it to resemble the value.

Fail-before

a_masked_secret_reveals_nothing_of_it fails against the old helper. The load-bearing part is the prefix loop (for n in 1..=secret.len()): masked != secret passes against the old code, and so does "contains asterisks" — only asking whether any prefix survives catches this.

Neighbour sweep

mask_secret had exactly one call site and is the only secret-masking helper in crates/; no route or log line emits a partially masked secret. One other partial reveal exists and was left alone on purpose: crates/biorouter/examples/tetrate_auth.rs:28 prints the first 10 characters of a key the developer has just obtained interactively, to their own terminal — not a route, not a log, not shipped. Flagged rather than folded into a security commit.

---

4 — Already fixed on main, no commit

Not a defect on 5181f544; it was closed as finding F2 of the 2026-09-10 QA run, and by the mechanism the brief asks for — the verdict re-evaluates instead of being cached.

Config::outstanding_write_failure() is the only reader and re-checks the recorded failure against the disk as it is now: no record ⇒ no I/O; a config that does not load keeps its record; a config that loads is re-probed through probe_config_write (everything a write does except the rename), and a probe that now succeeds clears the record — only the failure it actually tested (slot.recorded == recorded). last_write_error() is its string view through the same check, and the recover route reads it after the reload (config_management.rs:1466). Existing coverage: a_recovery_after_the_config_was_healed_reports_persisted_with_no_note (F2 by name) and a_recorded_write_failure_is_cleared_once_the_config_loads_and_can_be_written_again.

---

Verification

All run with BIOROUTER_DISABLE_KEYRING=true, on the committed tree:

Command	Result
cargo check -p biorouter -p biorouter-server -p biorouter-cli --lib --tests	ok
cargo test -p biorouter --lib -- config:: agents::workspace agents::session_extensions	327 passed, 0 failed
cargo test -p biorouter-server --lib -- routes::config routes::workspace	24 passed, 0 failed
cargo test -p biorouter-server --tests	ok, 0 failed
cargo test -p biorouter --test privacy_capability --test privacy_guard_wiring	4 + 3 passed, 0 failed
cargo fmt --all && cargo fmt --check	clean
./scripts/clippy-lint.sh	one pre-existing finding only

clippy-lint.sh fails its too_many_lines baseline on send_prompt_turn
(workspace_extension.rs, 101/100), which arrived with e60213c8 (#244) and is
untouched by this branch — git diff origin/main...HEAD contains no line of that
function. It was the only finding; one clippy::string_slice error this branch
did introduce (a byte slice in the new masking test) is fixed in f2e8452f.

Rebased onto 38270a50 after an automation merged origin/main into the branch
mid-review, so that the history stays three commits — one per item, each
revertible alone. The rebased tree is byte-identical to the merged state
(git diff <merge> HEAD empty) and the whole table above was re-run on it.

Six new tests: three cold-roster end-to-end, one pure reader test (an_unreadable_saved_roster_is_not_an_absent_one), one config write-count test, one masking test. Every one of them was demonstrated to fail against the reverted behaviour before being accepted.

🤖 Generated with Claude Code