From 347d071117a33dfcc3d592277b6794fb5b37856c Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 20:21:38 -0700 Subject: [PATCH 1/3] fix(workspace): a cold chat's extension roster survives workspace_set_tools `workspace_set_tools` on a conversation with no live agent replaced its whole saved extension list with just the one change, and reported success. The handler fetched the target's agent with `get_or_create_agent`, whose miss path mints a bare, extension-less agent and caches it under the target's id. `Agent::persist_extension_state` then wrote that empty manager's snapshot as the conversation's entire roster -- the write is a whole-key REPLACE, which is correct for the reply loop (a removal is expressed by an absence) and wrong for a chat that is not open. Measured on 5181f544, on a cold chat holding three extensions: `remove_extensions: ["roster-beta"]` answered `Applied to session ...: -roster-beta.` and left the saved roster holding NONE. `add_extensions` left it holding one. Data loss, announced as a change applied, with nothing at the call site able to tell. Three changes: * `session_extensions::apply_saved_roster_delta` writes `stored - remove + add` onto the session row instead of snapshotting a manager. It is the right answer for an open chat too: this tool knows exactly what it changed, and everything else in the row is state it was never asked to touch. This also closes the same hole reached the other way -- a cold `{provider, model}` call mints a bare agent, so the NEXT call's `peek_agent` would have found one and trusted it. * `session_extensions::saved_roster_of` tells "nothing saved" apart 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, so an unreadable roster now refuses the call loudly rather than being overwritten. * the handler peeks instead of creating for the extension dimension, so a cold conversation is changed where its roster actually lives and an agent is minted only for the provider switch, which genuinely needs one. Gate F1's unload arm asks the same `manageability_refusal(name, None, cap)` the pre-flight's cold branch asks -- judging a cold removal against the saved roster would LOOSEN it, because an unknown name reading Private is what stops the refusal being an existence oracle. The pre-flight's cold branch also answers the existence half now, against the saved roster and below both privacy arms. It used to `continue` past it, so a cold removal of a name the chat never had came back as `-name` -- and then took the rest of the roster with it. --- .../src/agents/session_extensions.rs | 166 ++++++++ .../src/agents/workspace_extension.rs | 373 +++++++++++++++--- 2 files changed, 494 insertions(+), 45 deletions(-) diff --git a/crates/biorouter/src/agents/session_extensions.rs b/crates/biorouter/src/agents/session_extensions.rs index f4c232994..0a538e927 100644 --- a/crates/biorouter/src/agents/session_extensions.rs +++ b/crates/biorouter/src/agents/session_extensions.rs @@ -87,3 +87,169 @@ pub async fn record( } Ok(()) } + +/// This conversation's SAVED extension roster, telling "nothing saved" apart +/// from "saved and unreadable". +/// +/// [`EnabledExtensionsState::from_extension_data`] collapses both into `None` +/// — it ends in `.ok()` — and for a caller about to REPLACE the key those two +/// answers could not be further apart. An absent key has nothing to lose. An +/// unreadable one is a roster this build cannot see, and overwriting it is the +/// data loss, not the repair. +pub fn saved_roster_of( + extension_data: &crate::session::extension_data::ExtensionData, + session_id: &str, +) -> Result> { + let Some(value) = extension_data.get_extension_state( + EnabledExtensionsState::EXTENSION_NAME, + EnabledExtensionsState::VERSION, + ) else { + return Ok(Vec::new()); + }; + Ok(EnabledExtensionsState::from_value(value) + .map_err(|e| { + anyhow!( + "conversation {session_id} has a saved extension roster this build cannot \ + read ({e}); refusing to replace it with anything" + ) + })? + .extensions) +} + +/// [`saved_roster_of`] for a session id. +pub async fn saved_roster( + session_manager: &SessionManager, + session_id: &str, +) -> Result> { + let session = session_manager.get_session(session_id, false).await?; + saved_roster_of(&session.extension_data, session_id) +} + +/// Apply ONE change to the conversation's saved roster: the stored set, minus +/// `remove`, plus `add`. +/// +/// ⚠ **Not [`record`], and the difference is the whole point.** `record` +/// snapshots the LIVE manager, which is right for the reply loop — the chat is +/// open, its manager is its roster, and a removal is expressed by an absence. +/// `workspace_set_tools` writes into conversations that are **not open**, where +/// the live manager is an empty agent `get_or_create_agent` has just minted: +/// snapshotting that wrote the one change as the conversation's entire roster +/// and reported success. Measured on a cold chat holding three extensions — one +/// `add_extensions` left one, one `remove_extensions` left none. +/// +/// So this writes a DELTA on the durable state instead of a snapshot of a +/// volatile one, which is also the right answer for a chat that *is* open: the +/// caller knows exactly what it changed, and everything else in the row is +/// state it was never asked to touch. An unreadable roster fails loudly here +/// rather than being replaced (see [`saved_roster_of`]). +pub async fn apply_saved_roster_delta( + session_manager: &SessionManager, + session_id: &str, + add: &[crate::agents::ExtensionConfig], + remove: &[String], +) -> Result> { + use crate::agents::extension_manager::normalize; + + let session = session_manager.get_session(session_id, false).await?; + // The same refusal `record` makes, for the same reason: a subagent's grant + // is runtime-profile authority, not a preference. + if session.session_type == SessionType::SubAgent { + return Err(anyhow!( + "subagent extension grants are immutable runtime-profile authority" + )); + } + + let mut roster = saved_roster_of(&session.extension_data, session_id)?; + let dropped: Vec = remove.iter().map(|name| normalize(name)).collect(); + roster.retain(|config| !dropped.contains(&normalize(&config.name()))); + for config in add { + let name = normalize(&config.name()); + // Re-adding replaces rather than duplicates: two entries under one name + // is a roster whose meaning depends on iteration order. + roster.retain(|existing| normalize(&existing.name()) != name); + roster.push(config.clone()); + } + + let value = EnabledExtensionsState::new(roster.clone()) + .to_value() + .map_err(|e| anyhow!("Extension state serialization failed: {}", e))?; + let written = session_manager + .update_extension_state( + session_id, + EnabledExtensionsState::EXTENSION_NAME, + EnabledExtensionsState::VERSION, + move |_| Ok(value), + ) + .await?; + if written.is_none() { + return Err(anyhow!( + "cannot record extension state: no session {session_id}" + )); + } + Ok(roster) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::session::extension_data::ExtensionData; + + fn stdio(name: &str) -> crate::agents::ExtensionConfig { + crate::agents::ExtensionConfig::Stdio { + name: name.to_string(), + description: String::new(), + cmd: "true".to_string(), + args: Vec::new(), + envs: Default::default(), + env_keys: Vec::new(), + timeout: None, + bundled: None, + available_tools: Vec::new(), + } + } + + /// The three answers a reader has to tell apart, and the one + /// `EnabledExtensionsState::from_extension_data` collapses. + /// + /// It ends in `.ok()`, so "no roster saved" and "a roster this build cannot + /// parse" are both `None` there. A caller about to REPLACE the key needs + /// them apart: the first has nothing to lose, the second is the data loss. + #[test] + fn an_unreadable_saved_roster_is_not_an_absent_one() { + let mut absent = ExtensionData::new(); + absent.set_extension_state("todo", "v0", serde_json::json!({ "content": "" })); + assert!( + saved_roster_of(&absent, "s1").unwrap().is_empty(), + "no roster saved is an empty roster, not an error" + ); + + let mut readable = ExtensionData::new(); + readable.set_extension_state( + EnabledExtensionsState::EXTENSION_NAME, + EnabledExtensionsState::VERSION, + EnabledExtensionsState::new(vec![stdio("a"), stdio("b")]) + .to_value() + .unwrap(), + ); + assert_eq!( + saved_roster_of(&readable, "s1") + .unwrap() + .iter() + .map(|c| c.name()) + .collect::>(), + vec!["a".to_string(), "b".to_string()] + ); + + let mut unreadable = ExtensionData::new(); + unreadable.set_extension_state( + EnabledExtensionsState::EXTENSION_NAME, + EnabledExtensionsState::VERSION, + serde_json::json!({ "extensions": "written by a newer build" }), + ); + let err = saved_roster_of(&unreadable, "s1") + .expect_err("an unreadable roster must not read as an empty one") + .to_string(); + assert!(err.contains("cannot read"), "{err}"); + assert!(err.contains("refusing to replace it"), "{err}"); + } +} diff --git a/crates/biorouter/src/agents/workspace_extension.rs b/crates/biorouter/src/agents/workspace_extension.rs index 5c5559419..c6d5bb995 100644 --- a/crates/biorouter/src/agents/workspace_extension.rs +++ b/crates/biorouter/src/agents/workspace_extension.rs @@ -3591,26 +3591,43 @@ impl WorkspaceClient { // `target_mode_requires_approval` documents at length. A skills-only or // KB-only call must not pay that price for a target the user has not // opened. - let needs_agent = - !add_configs.is_empty() || !args.remove_extensions.is_empty() || new_provider.is_some(); - let agent = if needs_agent { + // + // ⚠ **The EXTENSION dimension must not pay it either, which is why the + // peek comes first.** A bare agent's manager holds nothing, and + // `persist_extension_state` was a whole-key REPLACE of that snapshot — + // so a change to a conversation with no live agent wrote the one change + // as its ENTIRE roster and reported success. Measured on a cold chat + // holding three extensions: one `add_extensions` left it holding one, + // and one `remove_extensions` left it holding none. Data loss, + // announced as a change applied. A conversation with no live agent is + // now changed where its roster actually lives — the session row — and + // an agent is minted only for the provider switch, which genuinely + // needs one. + let touches_extensions = !add_configs.is_empty() || !args.remove_extensions.is_empty(); + let (live, agent) = if touches_extensions || new_provider.is_some() { let agent_manager = crate::execution::manager::AgentManager::instance() .await .map_err(|e| e.to_string())?; - Some( - agent_manager - .get_or_create_agent(args.session_id.clone()) - .await - .map_err(|e| e.to_string())?, - ) + let live = agent_manager.peek_agent(&args.session_id).await; + let agent = match (&live, new_provider.is_some()) { + (Some(live), _) => Some(std::sync::Arc::clone(live)), + (None, true) => Some( + agent_manager + .get_or_create_agent(args.session_id.clone()) + .await + .map_err(|e| e.to_string())?, + ), + (None, false) => None, + }; + (live, agent) } else { - None + (None, None) }; - if let Some(agent) = &agent { + if touches_extensions { applied.extend( - Self::apply_extension_changes_gated( - agent, + self.apply_extension_changes_gated( + live.as_ref(), cap, &args.session_id, add_configs, @@ -3788,7 +3805,13 @@ impl WorkspaceClient { // F4: removals are judged HERE, before any approval — they used to be // judged only once the handler had fetched the target's agent, which is // after the card the user had already been asked to approve. - Self::preflight_extension_removals(cap, &args.session_id, &args.remove_extensions).await?; + Self::preflight_extension_removals( + self.context.session_manager.as_ref(), + cap, + &args.session_id, + &args.remove_extensions, + ) + .await?; // Model/provider (decision b): resolve and validate here; apply later. let new_provider = Self::resolve_provider_switch(&args.provider, &args.model).await?; if let (Some((_, _, provider)), Some(classification)) = (&new_provider, write_target) { @@ -3837,7 +3860,21 @@ impl WorkspaceClient { /// "loaded" and "not loaded" stay the one sentence finding 13 requires. /// /// [`manageability_refusal`]: crate::agents::extension_manager::manageability_refusal + /// The one sentence for "that conversation has nothing under this name". + /// + /// Two branches answer it — a live conversation's loaded manager and a cold + /// one's saved roster — and two copies of one refusal is how the two + /// extension doors in this file drifted apart the last time. + fn nothing_to_remove(name: &str, target_session_id: &str) -> String { + format!( + "`{name}` is not enabled in conversation {target_session_id}, so there is \ + nothing to remove. Nothing was changed; check the conversation's extensions \ + with workspace_list." + ) + } + async fn preflight_extension_removals( + session_manager: &crate::session::SessionManager, cap: crate::privacy::CallCapability, target_session_id: &str, names: &[String], @@ -3849,6 +3886,19 @@ impl WorkspaceClient { Ok(manager) => manager.peek_agent(target_session_id).await, Err(_) => None, }; + // A conversation with no live agent still HAS a roster — in its session + // row — and this used to skip the existence half entirely for one, + // so a cold removal of a name the chat never had came back as `-name`. + // Read once, before anything is applied, so an unreadable roster refuses + // the call here rather than at the write (`saved_roster_of`). + let saved = match &live { + Some(_) => Vec::new(), + None => { + crate::agents::session_extensions::saved_roster(session_manager, target_session_id) + .await + .map_err(|e| e.to_string())? + } + }; for name in names { let Some(agent) = &live else { if let Some(refusal) = @@ -3856,6 +3906,16 @@ impl WorkspaceClient { { return Err(refusal.message.to_string()); } + // The same sentence the live branch answers with — by name, not a + // second copy of it — and BELOW the privacy arm for the same + // reason: reachable only by a caller already entitled to see what + // that conversation has loaded. + if !saved.iter().any(|config| { + crate::agents::extension_manager::normalize(&config.name()) + == crate::agents::extension_manager::normalize(name) + }) { + return Err(Self::nothing_to_remove(name, target_session_id)); + } continue; }; agent @@ -3872,11 +3932,7 @@ impl WorkspaceClient { .is_extension_enabled(&crate::agents::extension_manager::normalize(name)) .await { - return Err(format!( - "`{name}` is not enabled in conversation {target_session_id}, so there is \ - nothing to remove. Nothing was changed; check the conversation's extensions \ - with workspace_list." - )); + return Err(Self::nothing_to_remove(name, target_session_id)); } } Ok(()) @@ -3934,7 +3990,8 @@ impl WorkspaceClient { /// order it encodes (every removal entitled BEFORE anything is applied) is /// the part that must not be rearranged, and it is argued for inline. async fn apply_extension_changes_gated( - agent: &std::sync::Arc, + &self, + live: Option<&std::sync::Arc>, cap: crate::privacy::CallCapability, session_id: &str, add_configs: Vec, @@ -3975,14 +4032,33 @@ impl WorkspaceClient { // so a refused removal cannot land after that function has already // applied the adds — the "resolve everything before mutating // anything" rule the add half states above, held across both halves. + // + // ⚠ **`live: None` asks the SAME predicate with the same `None`** the + // pre-flight's cold branch asks it with. A conversation with no live + // agent has nothing loaded, and judging its removals against the SAVED + // roster here would LOOSEN this gate rather than tighten it: an unknown + // name reads Private, and that inverted default is what stops the + // refusal being an existence oracle over exactly the private names + // Gate E hides. Existence is answered separately, below both privacy + // arms, in `preflight_extension_removals`. for name in remove_extensions { - agent - .extension_manager - .assert_extension_manageable(name, cap) - .await - .map_err(|e| e.message.to_string())?; + match live { + Some(agent) => agent + .extension_manager + .assert_extension_manageable(name, cap) + .await + .map_err(|e| e.message.to_string())?, + None => { + if let Some(refusal) = + crate::agents::extension_manager::manageability_refusal(name, None, cap) + { + return Err(refusal.message.to_string()); + } + } + } } - Self::apply_extension_changes(agent, session_id, add_configs, remove_extensions).await + self.apply_extension_changes(live, session_id, add_configs, remove_extensions) + .await } /// **Gate F1, at the workspace's own two enable doors** (issue #56, @@ -4228,9 +4304,14 @@ impl WorkspaceClient { } } - /// The exact /agent/add_extension handler path (routes/agent.rs:744-767): - /// add on the live agent, persist only after a successful load. Returns the - /// `applied` labels for the extensions that changed. + /// Change the live agent's loaded set when there IS one, and the + /// conversation's saved roster either way. Returns the `applied` labels for + /// the extensions that changed. + /// + /// The live half is /agent/add_extension's handler path (routes/agent.rs): + /// add or remove on the agent, persist only after a successful load. The + /// persist half is deliberately NOT that handler's — see the delta note + /// below. /// /// ⚠ **This function decides nothing about privacy, and it has exactly one /// caller for that reason.** Both of its halves are gated at @@ -4244,35 +4325,57 @@ impl WorkspaceClient { /// how this one came to be one. If you need this here, carry both gates with /// it or move them inside. async fn apply_extension_changes( - agent: &crate::agents::Agent, + &self, + live: Option<&std::sync::Arc>, session_id: &str, add_configs: Vec, remove_extensions: &[String], ) -> Result, String> { let mut applied = Vec::new(); let mut extensions_changed = false; - for config in add_configs { - let name = config.name().to_string(); - agent - .add_extension(config) - .await - .map_err(|e| format!("failed to add '{name}': {e}"))?; - applied.push(format!("+{name}")); + // The live half: only a conversation that is actually open has a + // manager to change. A cold one is changed on disk alone, below — it + // has no tool surface to keep in step, and `load_extensions_from_session` + // spawns the roster it finds the next time it is opened. + if let Some(agent) = live { + for config in &add_configs { + let name = config.name(); + agent + .add_extension(config.clone()) + .await + .map_err(|e| format!("failed to add '{name}': {e}"))?; + } + for name in remove_extensions { + agent + .remove_extension(name) + .await + .map_err(|e| format!("failed to remove '{name}': {e}"))?; + } + } + for config in &add_configs { + applied.push(format!("+{}", config.name())); extensions_changed = true; } for name in remove_extensions { - agent - .remove_extension(name) - .await - .map_err(|e| format!("failed to remove '{name}': {e}"))?; applied.push(format!("-{name}")); extensions_changed = true; } if extensions_changed { - agent - .persist_extension_state(session_id) - .await - .map_err(|e| format!("failed to persist extension state: {e}"))?; + // ⚠ **A DELTA on the saved roster, never a snapshot of the live + // manager.** `Agent::persist_extension_state` snapshots the manager, + // which is right for the reply loop and wrong here: this tool writes + // into conversations that are not open, whose manager is whatever + // bare agent happens to be cached under their id. See + // `session_extensions::apply_saved_roster_delta`, which also refuses + // loudly rather than replacing a roster it cannot read. + crate::agents::session_extensions::apply_saved_roster_delta( + self.context.session_manager.as_ref(), + session_id, + &add_configs, + remove_extensions, + ) + .await + .map_err(|e| format!("failed to persist extension state: {e}"))?; // `workspace__workspace_set_tools` is NOT in `tool_catalog_mutation`, // so the reply loop's post-batch refresh never covered it: this tool // changes ANOTHER chat's extension set, and until now no consumer of @@ -12188,6 +12291,186 @@ pub(crate) mod tests { ); } + /// A conversation with a SAVED extension roster and no live agent — the + /// state every chat the user has not opened this session is in, and the one + /// `live_target` cannot produce (its `add_inprocess_server` fixtures are + /// filtered out of `get_extension_configs`, so they never reach a row). + async fn cold_target_with_roster(c: &WorkspaceClient, label: &str, names: &[&str]) -> String { + let target = seeded_target(c, label).await; + let configs: Vec = names + .iter() + .map(|name| crate::agents::ExtensionConfig::Stdio { + name: (*name).to_string(), + description: String::new(), + cmd: "true".to_string(), + args: Vec::new(), + envs: Default::default(), + env_keys: Vec::new(), + timeout: None, + bundled: None, + available_tools: Vec::new(), + }) + .collect(); + let value = serde_json::to_value( + crate::session::extension_data::EnabledExtensionsState::new(configs), + ) + .unwrap(); + c.context + .session_manager + .update_extension_state(&target, "enabled_extensions", "v0", move |_| Ok(value)) + .await + .unwrap() + .expect("the seeded session exists"); + assert!( + crate::execution::manager::AgentManager::instance() + .await + .unwrap() + .peek_agent(&target) + .await + .is_none(), + "the premise of every test below: this conversation has no live agent" + ); + target + } + + async fn saved_names(c: &WorkspaceClient, target: &str) -> Vec { + let mut names: Vec = + crate::agents::session_extensions::saved_roster(&c.context.session_manager, target) + .await + .unwrap() + .iter() + .map(|config| config.name()) + .collect(); + names.sort(); + names + } + + /// **A conversation the user has not opened keeps the extensions the call + /// did not name.** + /// + /// The handler fetched the target's agent with `get_or_create_agent`, whose + /// miss path mints a BARE one, and then persisted that empty manager's + /// snapshot as the conversation's entire roster — + /// `Agent::persist_extension_state` is a whole-key REPLACE by design. + /// Measured on `main`: a cold chat holding three extensions, one + /// `remove_extensions` naming one of them, and the saved roster came back + /// holding NONE. The two untouched extensions were gone and the call + /// answered `Applied to session …: -roster-beta.` — data loss reported as + /// success, which is why this is the worst shape in the batch: nothing at + /// the call site can tell. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn set_tools_on_a_cold_conversation_keeps_the_extensions_it_did_not_name() { + crate::workspace_services::set_for_tests(None); + let c = client(); + let target = cold_target_with_roster( + &c, + "cold-roster", + &["roster-alpha", "roster-beta", "roster-gamma"], + ) + .await; + + let result = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ "session_id": target, "remove_extensions": ["roster-beta"] }), + private_caller(), + ) + .await; + let text = text_of(&result); + crate::workspace_services::clear_test_override(); + assert_ne!(result.is_error, Some(true), "{text}"); + + assert_eq!( + saved_names(&c, &target).await, + vec!["roster-alpha".to_string(), "roster-gamma".to_string()], + "one named removal replaced the whole saved roster" + ); + } + + /// The same door, the other direction: a cold removal of a name the + /// conversation does not have is refused, and nothing is written. + /// + /// The live branch has answered this since F4; the cold branch used to + /// `continue` past it, so the phantom came back as `-name` — and then took + /// the rest of the roster with it. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn set_tools_refuses_a_cold_conversation_a_removal_it_has_nothing_for() { + crate::workspace_services::set_for_tests(None); + let c = client(); + let target = cold_target_with_roster(&c, "cold-phantom", &["roster-alpha"]).await; + + let refused = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ "session_id": target, "remove_extensions": ["ghost-fixture"] }), + private_caller(), + ) + .await; + let text = text_of(&refused); + crate::workspace_services::clear_test_override(); + + assert_eq!(refused.is_error, Some(true), "{text}"); + assert!( + text.contains("`ghost-fixture` is not enabled in conversation"), + "{text}" + ); + assert_eq!( + saved_names(&c, &target).await, + vec!["roster-alpha".to_string()], + "a refused call still wrote" + ); + } + + /// A saved roster this build cannot parse is refused, not replaced. + /// + /// `EnabledExtensionsState::from_extension_data` ends in `.ok()`, so an + /// unreadable roster and an absent one are the same `None` to every reader + /// — and to a writer about to REPLACE the key they are opposites. A write + /// that destroys state must never report success. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn a_saved_roster_this_build_cannot_read_is_refused_rather_than_replaced() { + crate::workspace_services::set_for_tests(None); + let c = client(); + let target = seeded_target(&c, "cold-unreadable").await; + let unreadable = serde_json::json!({ "extensions": "written by a newer build" }); + let stored = unreadable.clone(); + c.context + .session_manager + .update_extension_state(&target, "enabled_extensions", "v0", move |_| Ok(stored)) + .await + .unwrap() + .expect("the seeded session exists"); + + let refused = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ "session_id": target, "remove_extensions": ["roster-alpha"] }), + private_caller(), + ) + .await; + let text = text_of(&refused); + crate::workspace_services::clear_test_override(); + + assert_eq!(refused.is_error, Some(true), "{text}"); + assert!(text.contains("cannot read"), "{text}"); + let session = c + .context + .session_manager + .get_session(&target, false) + .await + .unwrap(); + assert_eq!( + session + .extension_data + .get_extension_state("enabled_extensions", "v0"), + Some(&unreadable), + "the roster it could not read was overwritten anyway" + ); + } + /// Knowledge bases used to be validated LAST — after extensions, skills /// and the provider had already been applied — and an id with no base was /// dropped without a word. Now: a missing base, or a write target outside From 2b92d92a98aada402314db66075b42a23e91e27f Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 20:21:55 -0700 Subject: [PATCH 2/3] fix(config): the default provider and its model are one write `POST /config/set_provider` called `set_biorouter_provider` and then `set_biorouter_model`. Those are two `save_values` calls, so between them `config.yaml` held the new provider beside the OLD model: 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 on this product the PROVIDER decides the privacy capability a session starts at, so the mismatch is privacy-relevant and not only cosmetic. `Config::set_params` writes several non-secret keys under one `guard` and one `save_values`, 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. Deliberately NOT a second temp-file-and-rename: `save_values` already stages under a per-process, per-call name (see `Config::staging_path`, and the shared-`config.tmp` race its doc records) and that is where the atomicity lives. The fix is to make one write carry both keys, not to invent another writer. The test asserts the write COUNT, because that is the mechanism: two writes is a window by construction and one has none. Reading the file back cannot see this -- the final state was always correct, which is why the gap survived. --- .../biorouter-cli/src/commands/configure.rs | 6 +- crates/biorouter-cli/src/commands/models.rs | 3 +- .../src/routes/config_management.rs | 14 ++- crates/biorouter/src/config/base.rs | 94 +++++++++++++++++++ .../src/config/signup_openrouter/mod.rs | 3 +- .../src/config/signup_tetrate/mod.rs | 3 +- 6 files changed, 109 insertions(+), 14 deletions(-) diff --git a/crates/biorouter-cli/src/commands/configure.rs b/crates/biorouter-cli/src/commands/configure.rs index cdbc89218..056aecd04 100644 --- a/crates/biorouter-cli/src/commands/configure.rs +++ b/crates/biorouter-cli/src/commands/configure.rs @@ -169,8 +169,7 @@ async fn handle_local_llamacpp_setup(config: &Config) -> anyhow::Result<()> { match test_provider_configuration("llamacpp", model, false, None).await { Ok(()) => { spin.stop(style("Llama Server is ready").green()); - config.set_biorouter_provider("llamacpp")?; - config.set_biorouter_model(model)?; + config.set_biorouter_provider_and_model("llamacpp", model)?; print_config_file_saved()?; Ok(()) } @@ -817,8 +816,7 @@ pub async fn configure_provider_dialog() -> anyhow::Result { match test_provider_configuration(provider_name, &model, toolshim_enabled, toolshim_model).await { Ok(()) => { - config.set_biorouter_provider(provider_name)?; - config.set_biorouter_model(&model)?; + config.set_biorouter_provider_and_model(provider_name, &model)?; print_config_file_saved()?; Ok(true) } diff --git a/crates/biorouter-cli/src/commands/models.rs b/crates/biorouter-cli/src/commands/models.rs index 1a1a83de6..3c4723927 100644 --- a/crates/biorouter-cli/src/commands/models.rs +++ b/crates/biorouter-cli/src/commands/models.rs @@ -233,8 +233,7 @@ pub async fn handle_models_set(provider_name: String, model: String) -> Result<( } let config = Config::global(); - config.set_biorouter_provider(provider_name.clone())?; - config.set_biorouter_model(&model)?; + config.set_biorouter_provider_and_model(provider_name.clone(), &model)?; println!("Model configuration updated"); println!(" provider: {}", style(provider_name).cyan()); diff --git a/crates/biorouter-server/src/routes/config_management.rs b/crates/biorouter-server/src/routes/config_management.rs index 4e923b233..89a2d2b49 100644 --- a/crates/biorouter-server/src/routes/config_management.rs +++ b/crates/biorouter-server/src/routes/config_management.rs @@ -1644,10 +1644,15 @@ pub async fn set_config_provider( create_with_default_model(&provider) .await .and_then(|_| { - let config = Config::global(); - config - .set_biorouter_provider(provider) - .and_then(|_| config.set_biorouter_model(model)) + // ⚠ ONE write, not two. `set_biorouter_provider` followed by + // `set_biorouter_model` left `config.yaml` holding the new provider + // beside the old model — measured at ~55 ms of `versa_azure` next + // to `gpt-6-astra` — and a chat started in that window binds a pair + // that was never chosen. The provider decides the session's privacy + // capability, so a mismatched pair is a privacy-relevant outcome, + // not only a cosmetic one. + Config::global() + .set_biorouter_provider_and_model(provider, model) .map_err(|e| anyhow::anyhow!(e)) }) .map_err(|err| (StatusCode::BAD_REQUEST, err.to_string()))?; @@ -1778,6 +1783,7 @@ pub fn routes(state: Arc) -> Router { #[cfg(test)] mod tests { + use http::HeaderMap; use super::*; diff --git a/crates/biorouter/src/config/base.rs b/crates/biorouter/src/config/base.rs index 2397a8abf..5905f6faf 100644 --- a/crates/biorouter/src/config/base.rs +++ b/crates/biorouter/src/config/base.rs @@ -1862,6 +1862,50 @@ impl Config { self.save_values(values) } + /// Write several non-secret configuration values as **one** config write. + /// + /// Two `set_param` calls are two writes, and between them the file holds + /// one new value beside one stale one. For `BIOROUTER_PROVIDER` and + /// `BIOROUTER_MODEL` that window is not cosmetic: a chat started inside it + /// binds a provider to a model that is not its own, and on this product the + /// PROVIDER is what decides the privacy capability a session starts at. + /// Measured at ~55 ms of `versa_azure` beside `gpt-6-astra` during one + /// switch through `POST /config/set_provider`. + /// + /// ⚠ **The same mechanism as [`Self::set_param`], not a second one.** + /// `guard` is held across the read and the write, exactly one + /// [`Self::save_values`] runs, and that is where the atomicity actually + /// lives — staged under a per-process, per-call name and renamed into + /// place (see [`Self::staging_path`], and the shared-`config.tmp` race its + /// doc records). Reaching for a second temp-file-and-rename here would + /// reintroduce exactly that. + pub fn set_params(&self, pairs: &[(&str, V)]) -> Result<(), ConfigError> { + let _guard = self.guard.lock().unwrap(); + let mut values = self.load()?; + for (key, value) in pairs { + values.insert(serde_yaml::to_value(key)?, serde_yaml::to_value(value)?); + } + self.save_values(values) + } + + /// The default provider and the model it is to be used with, written + /// together or not at all. + /// + /// The generated `set_biorouter_provider` / `set_biorouter_model` pair is + /// still there and still correct on its own; what is wrong is calling both + /// in sequence, which every one of the six switch sites used to do. Use + /// this instead — the two keys only ever mean anything as a pair. + pub fn set_biorouter_provider_and_model( + &self, + provider: impl Into, + model: impl Into, + ) -> Result<(), ConfigError> { + self.set_params(&[ + ("BIOROUTER_PROVIDER", provider.into()), + ("BIOROUTER_MODEL", model.into()), + ]) + } + /// Atomically read, mutate, and persist one non-secret configuration value /// with respect to every writer in this process. pub(crate) fn update_param(&self, key: &str, update: F) -> Result @@ -3637,6 +3681,56 @@ mod tests { ); } + /// **The provider and its model reach the file together or not at all.** + /// + /// `set_biorouter_provider` followed by `set_biorouter_model` is two + /// `save_values` calls, and between them `config.yaml` holds the new + /// provider beside the OLD model — measured at ~55 ms of `versa_azure` + /// next to `gpt-6-astra` during one switch through + /// `POST /config/set_provider`. A chat started in that window binds a pair + /// nobody chose, and the provider is what decides the privacy capability a + /// session starts at, so the mismatch is not cosmetic. + /// + /// The write COUNT is the assertion, because it is the mechanism: two + /// writes is a window by construction and one write has none. Reading the + /// file back cannot see this — the final state was always correct, which + /// is exactly why the gap survived. + #[test] + fn the_default_provider_and_its_model_are_one_config_write() { + let dir = tempfile::tempdir().unwrap(); + let config = Config::new_with_file_secrets( + dir.path().join("config.yaml"), + dir.path().join("secrets.yaml"), + ) + .unwrap(); + config.set_param("A_KEY_ALREADY_HERE", "keep me").unwrap(); + + let before = config.io_probe.config_writes(); + config + .set_biorouter_provider_and_model("versa_azure", "gpt-6-astra") + .unwrap(); + assert_eq!( + config.io_probe.config_writes() - before, + 1, + "the pair was written in more than one pass, so config.yaml held a mixed \ + provider/model pair in between" + ); + + assert_eq!( + config.get_biorouter_provider().unwrap(), + "versa_azure".to_string() + ); + assert_eq!( + config.get_biorouter_model().unwrap(), + "gpt-6-astra".to_string() + ); + assert_eq!( + config.get_param::("A_KEY_ALREADY_HERE").unwrap(), + "keep me".to_string(), + "one write for two keys must still be a read-modify-write of the whole file" + ); + } + /// A recorded write failure is cleared the moment a write succeeds. /// /// ⚠ Without this the record is permanent, and it is read by a **user-facing** diff --git a/crates/biorouter/src/config/signup_openrouter/mod.rs b/crates/biorouter/src/config/signup_openrouter/mod.rs index 934f143b0..1a387307c 100644 --- a/crates/biorouter/src/config/signup_openrouter/mod.rs +++ b/crates/biorouter/src/config/signup_openrouter/mod.rs @@ -165,7 +165,6 @@ use crate::config::Config; pub fn configure_openrouter(config: &Config, api_key: String) -> Result<()> { config.set_secret("OPENROUTER_API_KEY", &api_key)?; - config.set_biorouter_provider("openrouter")?; - config.set_biorouter_model(OPENROUTER_DEFAULT_MODEL)?; + config.set_biorouter_provider_and_model("openrouter", OPENROUTER_DEFAULT_MODEL)?; Ok(()) } diff --git a/crates/biorouter/src/config/signup_tetrate/mod.rs b/crates/biorouter/src/config/signup_tetrate/mod.rs index 31b541e2f..58182fdcb 100644 --- a/crates/biorouter/src/config/signup_tetrate/mod.rs +++ b/crates/biorouter/src/config/signup_tetrate/mod.rs @@ -166,7 +166,6 @@ use crate::config::Config; pub fn configure_tetrate(config: &Config, api_key: String) -> Result<()> { config.set_secret("TETRATE_API_KEY", &api_key)?; - config.set_biorouter_provider("tetrate")?; - config.set_biorouter_model(TETRATE_DEFAULT_MODEL)?; + config.set_biorouter_provider_and_model("tetrate", TETRATE_DEFAULT_MODEL)?; Ok(()) } From b28521227b73700fb1b50c2c7e12729db5e51bc5 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 20:22:17 -0700 Subject: [PATCH 3/3] fix(security): a masked secret reveals none of the secret `POST /config/read` with `is_secret: true` answered `{"maskedValue":"Y2EzNTgy********..."}`. `mask_secret` showed the first `min(len / 2, 8)` characters, so roughly eight characters of the real credential survived the mask -- a partial secret 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. The mask is now a fixed eight-bullet placeholder carrying none of the secret's bytes. Its LENGTH is fixed for the same reason: how long a stored credential is fingerprints which kind it is. Nothing renders it as anything but placeholder text (`DefaultProviderSetupForm.tsx` puts it straight into a field), so no caller needed it to resemble the value, and the response shape is unchanged -- still `{ maskedValue: string }`, so the generated client is untouched. The test's fail-before is the prefix loop: `masked != secret` passes against the old helper, and so does "contains asterisks". Swept the neighbours. `mask_secret` had exactly one call site and is the only secret-masking helper in the crates; no route or log line emits a partial secret. The one other partial reveal is `crates/biorouter/examples/tetrate_auth.rs:28`, which 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. Left alone and recorded here rather than folded into a security fix. --- .../src/routes/config_management.rs | 83 +++++++++++++++---- 1 file changed, 69 insertions(+), 14 deletions(-) diff --git a/crates/biorouter-server/src/routes/config_management.rs b/crates/biorouter-server/src/routes/config_management.rs index 89a2d2b49..53f05ff4f 100644 --- a/crates/biorouter-server/src/routes/config_management.rs +++ b/crates/biorouter-server/src/routes/config_management.rs @@ -665,20 +665,25 @@ pub async fn remove_config( } } -const SECRET_MASK_SHOW_LEN: usize = 8; - -fn mask_secret(secret: Value) -> String { - let as_string = match secret { - Value::String(s) => s, - _ => serde_json::to_string(&secret).unwrap_or_else(|_| secret.to_string()), - }; - - let chars: Vec<_> = as_string.chars().collect(); - let show_len = std::cmp::min(chars.len() / 2, SECRET_MASK_SHOW_LEN); - let visible: String = chars.iter().take(show_len).collect(); - let mask = "*".repeat(chars.len() - show_len); +/// The one string `POST /config/read` serves in place of a secret. +/// +/// Fixed, and carrying **none** of the secret's own bytes. It used to reveal +/// the first `min(len / 2, 8)` characters, so a 40-character key came back as +/// eight real characters followed by asterisks — a partial credential inside +/// the one response whose entire purpose is not to contain one, and a prefix +/// long enough to identify the key and to narrow a search for the rest. +/// +/// The LENGTH is fixed for the same reason the bytes are: how long a stored +/// credential is fingerprints which kind it is. Nothing renders this as +/// anything but placeholder text — `DefaultProviderSetupForm.tsx` puts it +/// straight into a field — so there is no caller that needs it to resemble +/// the value. +const SECRET_MASK: &str = "\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}\u{2022}"; - format!("{}{}", visible, mask) +/// See [`SECRET_MASK`]. The secret is taken and deliberately not looked at: +/// this is the shape a masking helper has to have to be one. +fn mask_secret(_secret: &Value) -> String { + SECRET_MASK.to_string() } #[utoipa::path( @@ -727,7 +732,7 @@ pub async fn read_config( Ok(value) => { if query.is_secret { ConfigValueResponse::MaskedValue(MaskedSecret { - masked_value: mask_secret(value), + masked_value: mask_secret(&value), }) } else { ConfigValueResponse::Value(value) @@ -1783,6 +1788,56 @@ pub fn routes(state: Arc) -> Router { #[cfg(test)] mod tests { + /// **A masked secret carries none of the secret.** + /// + /// `POST /config/read` with `is_secret: true` answered + /// `{"maskedValue":"Y2EzNTgy********…"}` — `min(len / 2, 8)` real + /// characters of the credential, in the one response whose whole purpose is + /// not to contain one. Eight characters is enough to identify which key is + /// stored and to narrow a search for the rest. + /// + /// The prefix loop is the fail-before: a `!= secret` assertion passes + /// against the old helper, and so does "contains asterisks". + #[test] + fn a_masked_secret_reveals_nothing_of_it() { + for secret in [ + "ca3582deadbeefcafe0123456789abcdef01234567", + "sk-proj-AAAABBBBCCCCDDDDEEEEFFFF", + "short", + "x", + ] { + let masked = super::mask_secret(&serde_json::json!(secret)); + // `chars().take(n)`, not `&secret[..n]`: a byte slice of a string is + // `clippy::string_slice`, and the property under test is about + // characters anyway. + for n in 1..=secret.chars().count() { + let prefix: String = secret.chars().take(n).collect(); + assert!( + !masked.contains(&prefix), + "the mask carries the first {n} characters of the secret: {masked}" + ); + } + assert!( + !masked.chars().any(|c| secret.contains(c)), + "the mask shares characters with the secret: {masked}" + ); + } + + // …and it is the same length whatever it hides: how long a stored + // credential is fingerprints which kind it is. + assert_eq!( + super::mask_secret(&serde_json::json!("x")), + super::mask_secret(&serde_json::json!( + "ca3582deadbeefcafe0123456789abcdef01234567" + )), + "the mask's length still leaks the secret's" + ); + // A non-string secret is masked too, not serialized into the response. + assert_eq!( + super::mask_secret(&serde_json::json!({ "token": "abc123" })), + super::SECRET_MASK + ); + } use http::HeaderMap;