Close four measured defects in the config and workspace surfaces

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<bool> {
     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)
         }

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());

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<AppState>) -> 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::*;

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<Vec<crate::agents::ExtensionConfig>> {
+    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<Vec<crate::agents::ExtensionConfig>> {
+    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<Vec<crate::agents::ExtensionConfig>> {
+    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<String> = 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<_>>(),
+            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}");
+    }
+}