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..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) @@ -1644,10 +1649,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 +1788,57 @@ 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; use super::*; 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 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(()) }