diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 871ebc560..c0381041d 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -137,7 +137,21 @@ jobs: - name: Install Linux native build dependencies if: matrix.os == 'ubuntu-latest' - run: sudo apt-get update && sudo apt-get install --yes libdbus-1-dev libxcb1-dev pkg-config + run: | + # ⚠ Drop the runner image's Microsoft apt repositories FIRST. + # `packages.microsoft.com` has repeatedly answered 403 for the + # `azure-cli` and `prod` lists that the hosted image ships + # preinstalled, and `apt-get update` exits 100 on any repository it + # cannot refresh — so an outage on a third-party CDN we do not use + # fails this job with `E: ... is no longer signed`. Nothing here + # installs from them. Removing them makes the update depend only on + # the archives the packages below actually come from. + sudo rm -f /etc/apt/sources.list.d/microsoft-prod.list \ + /etc/apt/sources.list.d/azure-cli.list \ + /etc/apt/sources.list.d/microsoft-prod.sources \ + /etc/apt/sources.list.d/azure-cli.sources + sudo apt-get update + sudo apt-get install --yes libdbus-1-dev libxcb1-dev pkg-config # Start narrow (lib + bins on every OS) so the matrix is green on arrival; # widen as it proves stable. This selector leaves out `tests/`, so the @@ -289,6 +303,18 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Install Linux native build dependencies run: | + # ⚠ Drop the runner image's Microsoft apt repositories FIRST. + # `packages.microsoft.com` has repeatedly answered 403 for the + # `azure-cli` and `prod` lists that the hosted image ships + # preinstalled, and `apt-get update` exits 100 on any repository it + # cannot refresh — so an outage on a third-party CDN we do not use + # fails this job with `E: ... is no longer signed`. Nothing here + # installs from them. Removing them makes the update depend only on + # the archives the packages below actually come from. + sudo rm -f /etc/apt/sources.list.d/microsoft-prod.list \ + /etc/apt/sources.list.d/azure-cli.list \ + /etc/apt/sources.list.d/microsoft-prod.sources \ + /etc/apt/sources.list.d/azure-cli.sources sudo apt-get update -q sudo apt-get install -y --no-install-recommends libdbus-1-dev libxdo-dev diff --git a/CLAUDE.md b/CLAUDE.md index fe5bb5a78..52cc024d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -251,12 +251,23 @@ what did not" section first**; the rest of that document is the design, not the - **The master switch** lives in its own record beside `config.yaml`, **not in it** and **not in an env var** — the agent has `developer__shell`, so a switch it can edit is not a switch. Loaded once per process; a load error resolves to ON. +- **Lineage is NOT a boundary; the tier is the only one.** `may_write` ⇔ `may_read` ⇔ `VIS`, so an + agent may inject a prompt into any conversation it can see — a child, a sibling, an unrelated + chat. R6's old "steer what you spawned, read everything else" rule is retired and + `Lineage`/`lineage_of` are deleted. Two *other* one-hop rules survive and are constantly + confused with it: `McpMeta::workspace_child_scope_only` (which confines an auto-injected + supervision grant's read/close/watch to direct children) and the flat refusal of every + `workspace_*` tool to a `SessionType::SubAgent`. A private→public write raises a + **first-crossing approval showing the payload**, once per (caller, target) pair, in every + permission mode — `agents/workspace_inspector.rs` asks, `privacy/crossing.rs` remembers, + `handle_send_prompt`/`handle_set_tools` record only once the write lands. - **Known gaps, do not assume otherwise.** The general filesystem read-deny (§9.5, DR-14) is DEFERRED — a public chat with a shell still reads ordinary files, which is why the - non-private-model disclosure ships. And §7's cross-session matrix - (`privacy/visibility.rs::may_read`) is **written but wired to nothing**: `workspace_read_conversation` - checks only `session_type == Hidden`, so it still reads a private transcript that `chatrecall` - would refuse. + non-private-model disclosure ships. ⚠ This bullet also claimed §7's cross-session matrix was + "written but wired to nothing" and that `workspace_read_conversation` checked only + `session_type == Hidden`; **both were already false** — the read gate is + `visibility::refuse_unless_readable` and the write gate composes it with `may_write`. Grep the + symbol rather than trusting a summary, including this one. - **Tests:** `cargo test -p biorouter --lib privacy::` and `cargo test -p biorouter-mcp --lib knowledge::tier`, plus five integration binaries that are **spread across three crates** — `-p biorouter`: `--test privacy_toggle`, diff --git a/crates/biorouter-server/src/routes/session_reach.rs b/crates/biorouter-server/src/routes/session_reach.rs index 8111b32d7..761bf5e34 100644 --- a/crates/biorouter-server/src/routes/session_reach.rs +++ b/crates/biorouter-server/src/routes/session_reach.rs @@ -66,8 +66,9 @@ //! target's classification instead of asking for a human. Its refusal answers //! "private" and "no such conversation" in one sentence, for the same reason //! [`SESSION_OUT_OF_REACH`] does. `workspace_close` and `workspace_set_tools` -//! now enforce the same one-hop `may_write` lineage as -//! `workspace_send_prompt`; `workspace_watch` is parent-scoped through the +//! enforce the same `may_write` rule as `workspace_send_prompt` — which is +//! the tier and nothing else, the one-hop lineage clause having been +//! retired; `workspace_watch` is parent-scoped through the //! caller's registered background handles rather than an arbitrary session //! write; //! * and the daemon still has no principal, which is the actual subject of #47. diff --git a/crates/biorouter-server/src/workspace/turn.rs b/crates/biorouter-server/src/workspace/turn.rs index 6bdcbba58..5c6da6fa2 100644 --- a/crates/biorouter-server/src/workspace/turn.rs +++ b/crates/biorouter-server/src/workspace/turn.rs @@ -2778,4 +2778,168 @@ mod tests { (TurnErrorScope::Inference, true, None) ); } + + /// **`workspace_send_prompt mode:"turn"` reflects in an open tab live.** + /// + /// The injected prompt is persisted by `Agent::reply` like any other user + /// message, and `Agent::reply` deliberately does NOT yield a `Message` frame + /// for a user prompt — #66's rule, on the premise that "the client authored + /// it and already holds it". That premise is false for an injection: the + /// target's tab authored nothing, so with no frame carrying the body the + /// message appears only after a reload. `Agent::reply` therefore publishes + /// an agent-injected row straight onto the session bus, at the point it + /// becomes durable. + /// + /// Driven through the real `run_turn` against a provider that RESTORES but + /// cannot infer (`ollama`, naming a model that is not there). Getting past + /// provider restore is what matters: a name the factory does not know fails + /// in setup, before `Agent::reply` runs at all, so the prompt is never + /// persisted and the test would pass for the wrong reason. Failing at + /// inference instead is the point — the publish must already have happened, + /// because that is where the row became durable. + #[tokio::test] + async fn an_injected_turns_prompt_reaches_the_targets_open_tab_live() { + use biorouter::conversation::message::{MessageProvenance, ProvenanceKind}; + + const INJECTED: &str = "br71-injected-turn-live-marker"; + + let state = crate::state::AppState::new().await.unwrap(); + let workdir = tempfile::TempDir::new().unwrap(); + let target = state + .session_manager() + .create_session( + workdir.path().to_path_buf(), + "br71 injected turn live".into(), + SessionType::User, + ) + .await + .unwrap(); + state + .session_manager() + .update(&target.id) + .provider_name("ollama") + .model_config(biorouter::model::ModelConfig::new("br71-turn-model").unwrap()) + .apply() + .await + .unwrap(); + + // An observer, subscribed BEFORE the turn — the bus is a broadcast with + // no replay, so a subscription opened afterwards proves nothing. + let mut observer = biorouter::session_events::subscribe(&target.id); + + let cancel = CancellationToken::new(); + let guard = state + .try_begin_turn_idempotent(&target.id, cancel.clone(), None) + .expect("the target is idle"); + let _ = tokio::time::timeout( + std::time::Duration::from_secs(60), + run_turn( + state, + TurnRequest::new( + target.id.clone(), + Message::user() + .with_text(INJECTED) + .with_provenance(MessageProvenance { + kind: ProvenanceKind::AgentInjection, + from_session_id: Some("br71-injecting-caller".into()), + from_session_name: Some("the other chat".into()), + }), + ), + guard, + cancel, + ), + ) + .await; + + let mut saw_body = false; + while let Ok(event) = observer.try_recv() { + if let SessionBusEvent::Agent(biorouter::agents::AgentEvent::Message(m)) = event { + if !message_text_contains(&m, INJECTED) { + continue; + } + // The DURABLE row, not a pre-write copy: `add_message_adopting_uid` + // stamps the minted uid, and a frame with `id: None` is one the + // renderer cannot reconcile against the stored twin that arrives + // with the next snapshot. + assert!( + m.id.is_some(), + "the injected prompt was published before it was durable" + ); + saw_body = true; + } + } + assert!( + saw_body, + "no Message frame carried the injected prompt, so an open tab would \ + show it only after a reload" + ); + } + + /// The control for the test above, and the thing that keeps it from being + /// vacuous: an ORDINARY user prompt still gets no `Message` frame. #66's + /// ordering rule depends on that — the client's own prompt is named by + /// `MessagesPersisted` and never yielded — so a publish that fired for every + /// user message would be a regression wearing the same green tick. + #[tokio::test] + async fn an_ordinary_user_prompt_is_still_never_published_as_a_message() { + const TYPED: &str = "br71-ordinary-prompt-marker"; + + let state = crate::state::AppState::new().await.unwrap(); + let workdir = tempfile::TempDir::new().unwrap(); + let target = state + .session_manager() + .create_session( + workdir.path().to_path_buf(), + "br71 ordinary prompt".into(), + SessionType::User, + ) + .await + .unwrap(); + state + .session_manager() + .update(&target.id) + .provider_name("ollama") + .model_config(biorouter::model::ModelConfig::new("br71-turn-model").unwrap()) + .apply() + .await + .unwrap(); + + let mut observer = biorouter::session_events::subscribe(&target.id); + + let cancel = CancellationToken::new(); + let guard = state + .try_begin_turn_idempotent(&target.id, cancel.clone(), None) + .expect("the target is idle"); + let _ = tokio::time::timeout( + std::time::Duration::from_secs(60), + run_turn( + state, + TurnRequest::new(target.id.clone(), Message::user().with_text(TYPED)), + guard, + cancel, + ), + ) + .await; + + while let Ok(event) = observer.try_recv() { + if let SessionBusEvent::Agent(biorouter::agents::AgentEvent::Message(m)) = event { + assert!( + !message_text_contains(&m, TYPED), + "an ordinary typed prompt was published as a Message frame; \ + #66's ordering rule assumes it never is" + ); + } + } + } + + fn message_text_contains( + message: &biorouter::conversation::message::Message, + needle: &str, + ) -> bool { + message + .content + .iter() + .filter_map(|c| c.as_text()) + .any(|t| t.contains(needle)) + } } diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index d3fc3d4a4..4520bfc94 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -3654,6 +3654,19 @@ impl Agent { crate::agents::workspace_inspector::WorkspaceMutationInspector, )); + // Issue #56, the first-crossing disclosure: private-capability chat + // writing into a PUBLIC conversation shows the payload once per + // (caller, target) pair. Inert for every tool but + // `workspace_send_prompt` and `workspace_set_tools`, and inert entirely + // for a public-capability caller — which is why it can take the same + // provider handle the permission inspector below does without adding a + // mutex read to an ordinary turn. + tool_inspection_manager.add_inspector(Box::new( + crate::agents::workspace_inspector::WorkspaceCrossingInspector::new(Arc::clone( + &provider, + )), + )); + // Add permission inspector (medium-high priority). BR-18: it reads the // shared risk registry the agent refreshes each turn from the model's // tool list, so `SmartApprove` auto-approves read-only-annotated tools @@ -6359,14 +6372,59 @@ impl Agent { name: Self::SPAWN_EXTENSION.to_string(), description: "Delegate work to subagents".to_string(), bundled: Some(true), - // Delegation plus its child-scoped supervision surface. The bridge - // does not expose workspace_send_prompt, and WorkspaceClient still - // enforces lineage on every read/close/watch call. Enforced on BOTH - // the advertisement path (`filter`/`is_tool_available` in - // `fetch_all_tools`) and dispatch. + // Delegation, its child-scoped supervision surface, and the two + // tools that make cross-chat injection usable: `workspace_list` to + // learn which conversations exist and which are running, and + // `workspace_send_prompt` to write into one. + // + // ⚠ **`workspace_list` is in here on purpose, and it is the entry + // that needs justifying.** Without it `workspace_send_prompt` is + // reachable but unusable for anything except a child, because a + // session id is the one argument it cannot invent — an agent knows + // its children's ids from the spawn results and knows no others. A + // grant that advertises a tool the holder can never supply an + // argument for is worse than not granting it: the model tries. + // What keeps the TIER safe is that `appears_in_list` OMITS private + // rows rather than redacting them, so the discovery surface is + // exactly the set this capability may already read. + // + // ⚠ **The tier is not the whole cost, and the rest is a real + // trade rather than a non-issue.** A row carries the conversation's + // name (LLM-generated from its contents), its working directory, + // its enabled extensions and its GUI placement, and `scope:"all"` + // returns every same-tier conversation on the machine. A user who + // turned on "delegate to subagents" did not knowingly turn on + // "enumerate my chats and their tool grants", and the enumeration is + // what makes an injection *aimable* — a chat can find a + // conversation whose extensions include `developer` and inject text + // that runs there, in that conversation's permission context. + // + // It is included anyway, because the requirement is explicit that a + // chat should be able to see which conversations exist and which are + // running, and because the alternative is worse rather than safer: + // `workspace_send_prompt` without it is a tool whose one required + // argument the holder cannot obtain, so the model tries and fails + // instead of not trying. Two things bound it, and both are load- + // bearing: this tier exists only in `BioRouterMode::Auto` + // (`subagents_enabled`), and every injection is provenance-stamped + // and toasted on the target's tab. + // + // The other four are unchanged, and the two child-scoped ones stay + // child-scoped: `refuse_unless_direct_subagent_child` gates + // read_conversation / close / watch off + // `McpMeta::workspace_child_scope_only`, which + // `ExtensionManager::dispatch_tool_call` sets for exactly this + // auto-injected entry. `handle_list` and `handle_send_prompt` do + // not read that flag — they are gated by the tier alone — which is + // what makes adding them here a real widening rather than a no-op. + // + // Enforced on BOTH the advertisement path (`filter`/ + // `is_tool_available` in `fetch_all_tools`) and dispatch. available_tools: vec![ SUBAGENT_TOOL_NAME.to_string(), + "workspace_list".to_string(), "workspace_read_conversation".to_string(), + "workspace_send_prompt".to_string(), "workspace_close".to_string(), "workspace_watch".to_string(), ], @@ -7087,6 +7145,33 @@ impl Agent { .await?; prestream_persisted.push(injected); } + // A turn started by ANOTHER conversation + // (`workspace_send_prompt mode:"turn"`) has no client that authored its + // prompt, so the premise of the `named_but_never_yielded` comment below + // does not hold for it: nobody holds this text, and without a frame + // carrying the body an open tab shows the injection only after a + // reload. Published straight onto the session bus, AFTER the row is + // durable and with the row's own minted uid. + // + // ⚠ Published rather than *yielded*, deliberately. A yield would put a + // `Message` frame in front of the `MessagesPersisted` id below and break + // #66's ordering rule for every turn in the product, to fix a case that + // is not about this turn's own stream at all — the reader who needs this + // is an observer of the TARGET tab, and the bus is that reader's + // channel. `publish` is a pure lookup and a no-op with no observer. + for persisted in &prestream_persisted { + if persisted.metadata.provenance.as_ref().is_some_and(|p| { + p.kind == crate::conversation::message::ProvenanceKind::AgentInjection + }) { + crate::session_events::publish( + &session_config.id, + crate::session_events::SessionBusEvent::Agent(AgentEvent::Message( + persisted.clone(), + )), + ); + } + } + // #59: published as the stream's first event, before anything else can // be appended to the session, so the client's view of the stored set // starts complete. @@ -12646,9 +12731,25 @@ mod tests { "auto-injection must grant child supervision with {tool}: {names:?}" ); } + // Cross-chat injection, and the discovery half without which its one + // required argument is unobtainable. Both were on the excluded list + // below until the write rule stopped reading lineage; a delegating + // agent that cannot name another conversation cannot inject into one. for tool in [ "workspace__workspace_list", "workspace__workspace_send_prompt", + ] { + assert!( + names.iter().any(|name| name == tool), + "auto-injection must grant cross-chat injection with {tool}: {names:?}" + ); + } + // Still excluded, and each for its own reason rather than as leftovers + // of one rule. `workspace_set_tools` rewrites another conversation's + // provider, extensions and skills — a capability change, not a message, + // and the thing §5's always-confirm inspector exists for. + // `workspace_open` mints sessions and moves the user's tabs. + for tool in [ "workspace__workspace_set_tools", "workspace__workspace_open", ] { @@ -13005,6 +13106,12 @@ mod tests { /// on the call path too (`extension_manager.rs`, the /// `config.is_tool_available` re-check in `dispatch_tool_call`), so a /// remembered tool name cannot reach the handler. + /// + /// Driven with `workspace_set_tools`. It used to be `workspace_send_prompt`, + /// which is now IN the injected set — and the substitution is the point: + /// the allowlist still has teeth, it is just a different list. A + /// capability change to another conversation is the thing a delegating + /// agent is not handed; a message to one is. #[tokio::test] async fn an_auto_injected_session_cannot_dispatch_a_cross_session_tool() { let (agent, session_id) = agent_with_one_extension_for_tests().await; @@ -13015,12 +13122,14 @@ mod tests { &session_id, rmcp::model::CallToolRequestParams { meta: None, - name: "workspace__workspace_send_prompt".into(), + name: "workspace__workspace_set_tools".into(), arguments: Some( - serde_json::json!({ "session_id": "other", "text": "hi", "mode": "note" }) - .as_object() - .unwrap() - .clone(), + serde_json::json!({ + "session_id": "other", "add_extensions": ["developer"] + }) + .as_object() + .unwrap() + .clone(), ), task: None, }, @@ -13032,7 +13141,7 @@ mod tests { // Ok payload) does not compile — match instead. let err = match dispatched { Ok(_) => panic!( - "workspace_send_prompt is outside the auto-injection's \ + "workspace_set_tools is outside the auto-injection's \ available_tools and must not reach a handler" ), Err(e) => e, @@ -13095,10 +13204,9 @@ mod tests { .map(|t| t.name.to_string()) .collect(); assert!( - !names - .iter() - .any(|n| n == "workspace__workspace_send_prompt"), - "precondition: only the spawn tool was injected: {names:?}" + !names.iter().any(|n| n == "workspace__workspace_set_tools"), + "precondition: the restricted injection was applied, not the full \ + surface: {names:?}" ); // Now the user enables Workspace Control in Settings. diff --git a/crates/biorouter/src/agents/code_execution_extension.rs b/crates/biorouter/src/agents/code_execution_extension.rs index 933de5e5d..b042576cc 100644 --- a/crates/biorouter/src/agents/code_execution_extension.rs +++ b/crates/biorouter/src/agents/code_execution_extension.rs @@ -2106,6 +2106,65 @@ impl CodeExecutionClient { } } + /// The refusals this door owes because **no [`ToolInspector`] reaches it**. + /// + /// The JS sandbox hands a script's inner tool calls straight to + /// `ExtensionManager::dispatch_tool_call`, so the whole inspector stack — + /// which is where the global-memory consent gate, the session-store refusal + /// and issue #56's first-crossing disclosure all live — is simply not on + /// this path. Each of the three is therefore re-asked here, against the + /// **already-evaluated** arguments rather than against the script text: a + /// path or a payload the script computed at runtime is fully assembled by + /// the time it arrives, which is what makes these boundary checks strictly + /// stronger than their inspectors rather than copies of them. + /// + /// Returns the metric label and the sentence to answer with. Kept as one + /// function so the loop has one refusal branch: three inline copies is how + /// a fourth boundary gets added to two of them. + /// + /// [`ToolInspector`]: crate::tool_inspection::ToolInspector + async fn uninspected_boundary_refusal( + cap: crate::privacy::CallCapability, + session_id: &str, + tool_name: &str, + evaluated: Option<&rmcp::model::JsonObject>, + ) -> Option<(&'static str, String)> { + const BOUNDARY: crate::security::UninspectedBoundary = + crate::security::UninspectedBoundary::ExecuteCodeScript; + + if let Some(refusal) = crate::security::global_memory::uninspected_boundary_refusal( + tool_name, evaluated, BOUNDARY, + ) { + return Some(("global_memory_consent", refusal)); + } + // Issue #56. The same boundary, the same reason, for the transcript + // store: `SessionStoreInspector`'s literal-path scan of the script text + // is out-computed by exactly one line + // (`const p = home + "/.config/biorouter/sessions/sessions.db"`). Here + // the path has already been assembled, so there is nothing left to + // compute. The store is every conversation on this machine. + if let Some(refusal) = crate::security::session_store::uninspected_boundary_refusal( + tool_name, evaluated, BOUNDARY, + ) { + return Some(("session_store_read", refusal)); + } + // Issue #56, the first-crossing disclosure, and the sharpest of the + // three: an undisclosed write here does not merely escape ONE approval + // card — the handler then records the (caller, target) pair as having + // crossed, so every later, properly-inspected write to that conversation + // is silent too. One script call would permanently disable the + // disclosure for that pair. Narrow by construction: same-tier writes, + // already-approved pairs and payload-free tools all pass through. + if let Some(refusal) = crate::agents::workspace_inspector::uninspected_crossing_refusal( + cap, session_id, tool_name, evaluated, BOUNDARY, + ) + .await + { + return Some(("workspace_tier_crossing", refusal)); + } + None + } + async fn run_tool_handler( session_id: String, cap: crate::privacy::CallCapability, @@ -2152,39 +2211,17 @@ impl CodeExecutionClient { // arguments. A boundary that cannot ask the user refuses. let evaluated = serde_json::from_str::(&arguments).ok(); let evaluated = evaluated.as_ref().and_then(serde_json::Value::as_object); - if let Some(refusal) = crate::security::global_memory::uninspected_boundary_refusal( - &tool_name, - evaluated, - crate::security::UninspectedBoundary::ExecuteCodeScript, - ) { - Self::refuse_sub_call( - &collected_artifacts, - &tool_name, - &arguments, - "global_memory_consent", - refusal, - response_tx, - ) - .await; - continue; - } - // Issue #56. The same boundary, the same reason, for the transcript - // store: `SessionStoreInspector` runs in the agent loop and never - // sees a script's inner calls, and its literal-path scan of the - // script text is out-computed by exactly one line - // (`const p = home + "/.config/biorouter/sessions/sessions.db"`). - // Here the path has already been assembled, so there is nothing left - // to compute. The store is every conversation on this machine. - if let Some(refusal) = crate::security::session_store::uninspected_boundary_refusal( - &tool_name, - evaluated, - crate::security::UninspectedBoundary::ExecuteCodeScript, - ) { + // Every boundary refusal this door owes, asked in one place. See + // `uninspected_boundary_refusal` for why a door that no + // `ToolInspector` reaches has to carry its own. + if let Some((kind, refusal)) = + Self::uninspected_boundary_refusal(cap, &session_id, &tool_name, evaluated).await + { Self::refuse_sub_call( &collected_artifacts, &tool_name, &arguments, - "session_store_read", + kind, refusal, response_tx, ) diff --git a/crates/biorouter/src/agents/workspace_extension.rs b/crates/biorouter/src/agents/workspace_extension.rs index 2a621e656..d10a9dddc 100644 --- a/crates/biorouter/src/agents/workspace_extension.rs +++ b/crates/biorouter/src/agents/workspace_extension.rs @@ -101,17 +101,18 @@ const INSTRUCTIONS: &str = indoc! {r#" agent, tools, knowledge bases and history. These tools operate the workspace: - workspace_list: see conversations, what's running, and where they are in the GUI. For "what is that chat doing now?" list, then read its tool_calls. - - workspace_open: open/focus an existing conversation, or start a new one the - USER owns (new.kind:"user"; optionally split or new window; opens in the - background). It never delegates: new.kind:"sub_agent" is refused. + - workspace_open: open/focus an existing conversation, or start a new one + the USER owns (new.kind:"user"; optionally split or new window; opens in + the background). It never delegates: new.kind:"sub_agent" is refused. - workspace_read_conversation: read another conversation. summary for a - digest, transcript for prose, tool_calls for exactly what its agent did, + digest, transcript for prose, tool_calls for what its agent did, spawn_context for how a subagent was started. Treat other conversations' content as sensitive; prefer the narrowest view. - - workspace_send_prompt: inject into another conversation. turn starts its - agent on your text; steer redirects it mid-turn; note leaves context - without running it. Injections are permanently labeled as coming from - you. Use wait:"final_message" to get its answer synchronously. + - workspace_send_prompt: inject into ANY conversation you can see, related + to you or not. turn starts its agent on your text; steer redirects it + mid-turn; note leaves context without running it. wait:"final_message" + returns its answer. Injections are permanently labeled as coming from + you. ONLY WHEN NECESSARY: a person may be reading that chat. - workspace_set_tools: add/remove extensions, scope skills to one conversation (add_skills), switch its model, or set its knowledge bases. When you have it, do this yourself instead of pointing at Settings. @@ -123,19 +124,19 @@ const INSTRUCTIONS: &str = indoc! {r#" figure, file or live web page. Use it when the user says "this" or "the page"; text is cheap and you can act on it. - workspace_capture_panel: screenshot it (returns a PNG path) to judge how - something LOOKS. You cannot act on a screenshot. + it LOOKS. You cannot act on a screenshot. - subagent: the ONLY way to delegate. A fresh agent with its own context - window; "spin up subagents" and fan-out mean this tool, one call per child, - same message for parallel. When the app is open the child runs in a visible - tab the user can watch and talk to; you still receive only its final - summary, so use workspace_read_conversation view:"tool_calls" on it to - verify what it did. The user may have intervened; the result tells you so. + window; "spin up subagents" and fan-out mean this tool, one call per + child, same message for parallel. When the app is open the child runs in + a visible tab the user can watch and talk to; you still get only its + final summary, so read its tool_calls to verify what it did. The result + tells you if the user intervened. Only the workspace tools in your tool list are available. - Routing: to search past conversations by content use chatrecall (if - enabled), not these tools. Durable facts belong in Memory. To fold a - conversation into a knowledge base use ingest_conversation; to re-read an - externalized payload use read_session_blob. If no GUI is attached these - tools still manage conversations headlessly and say so. + Routing: to search past conversations by content use chatrecall, not these + tools. Durable facts belong in Memory. To fold a conversation into a + knowledge base use ingest_conversation; to re-read an externalized payload + use read_session_blob. With no GUI attached these tools still manage + conversations headlessly and say so. "#}; const PANEL_MOIM_MAX_CHARS: usize = 8_000; @@ -1397,26 +1398,8 @@ impl WorkspaceClient { /// advertised tool surface against the capability table. A hand-maintained /// mirror of this function is the one place either guard can rot silently. pub fn get_tools() -> Vec { - vec![ - Self::tool( - "workspace_read_panel", - "Read what the preview panel is currently showing: the rendered \ - document, figure, file or live web page. **Prefer this over \ - workspace_capture_panel** — text is cheaper and can be acted \ - on, where a screenshot can only be looked at. Returns nothing \ - readable for an image; capture it instead.", - serde_json::to_value(schema_for!(WorkspacePanelParams)).unwrap(), - true, - ), - Self::tool( - "workspace_capture_panel", - "Screenshot the preview panel, saved as a PNG whose path is \ - returned. Use it to judge how something LOOKS — a figure, a \ - rendered page, a layout. You cannot act on a screenshot: to \ - find or change content, use workspace_read_panel.", - serde_json::to_value(schema_for!(WorkspacePanelParams)).unwrap(), - true, - ), + let mut tools = Self::panel_tools(); + tools.extend([ Self::tool( "workspace_list", "List conversations in the workspace: id, name, type, running \ @@ -1435,10 +1418,16 @@ impl WorkspaceClient { ), Self::tool( "workspace_send_prompt", - "Inject a prompt into another conversation. mode turn: start its \ - agent (target idle); steer: redirect mid-turn (target running); \ - note: append context without a turn. Injections are permanently \ - provenance-labeled. wait:\"final_message\" returns its answer.", + "Inject a prompt into ANY conversation you can see — a subagent \ + you spawned or an unrelated chat. mode turn: start its agent \ + (target idle); steer: redirect mid-turn (target running); note: \ + append context without a turn. Injections are permanently \ + provenance-labeled. wait:\"final_message\" returns its answer. \ + ONLY WHEN NECESSARY: a human may be reading that conversation, \ + and this interrupts them. Prefer answering here, or reading the \ + other conversation, over writing into it; do not use it to chat \ + with yourself, to spread a task you could do, or to nudge a \ + conversation that is already working.", serde_json::to_value(schema_for!(WorkspaceSendPromptParams)).unwrap(), false, ), @@ -1497,6 +1486,39 @@ impl WorkspaceClient { serde_json::to_value(schema_for!(WorkspaceOpenParams)).unwrap(), false, ), + ]); + tools + } + + /// The preview-panel pair, split out of [`Self::get_tools`] only because + /// that function outgrew the per-function line baseline. These two belong + /// together — `workspace_read_panel` and `workspace_capture_panel` are the + /// text and the picture of the same surface, and each description tells the + /// model to prefer the other in the case it does not cover — so they are the + /// natural seam. **`get_tools` is still the whole advertised surface**, and + /// `workspace_open_is_advertised_and_completes_the_surface` still holds it + /// against the instruction block name for name. + fn panel_tools() -> Vec { + vec![ + Self::tool( + "workspace_read_panel", + "Read what the preview panel is currently showing: the rendered \ + document, figure, file or live web page. **Prefer this over \ + workspace_capture_panel** — text is cheaper and can be acted \ + on, where a screenshot can only be looked at. Returns nothing \ + readable for an image; capture it instead.", + serde_json::to_value(schema_for!(WorkspacePanelParams)).unwrap(), + true, + ), + Self::tool( + "workspace_capture_panel", + "Screenshot the preview panel, saved as a PNG whose path is \ + returned. Use it to judge how something LOOKS — a figure, a \ + rendered page, a layout. You cannot act on a screenshot: to \ + find or change content, use workspace_read_panel.", + serde_json::to_value(schema_for!(WorkspacePanelParams)).unwrap(), + true, + ), ] } @@ -1564,15 +1586,34 @@ impl WorkspaceClient { .await } + /// WRITE ⇔ VIS: the tier is the only boundary a cross-session mutation has. + /// + /// ⚠ **Lineage is not a boundary, and used to be.** This body used to + /// classify the target as self / child / other and refuse the third, so an + /// agent could steer a conversation it had spawned and only read one it had + /// not. That is retired — an agent may inject into any conversation it can + /// see. What did not change is the half that matters: the read gate still + /// runs first, so a public-capability caller is refused a private target + /// before this function looks at anything, and the refusal is still one + /// sentence for private, unreadable and absent alike. + /// + /// The caller's own id is NOT an argument: nothing in the write decision + /// reads it any more. The one thing still keyed on the (caller, target) + /// pair is the first-crossing disclosure, and that is asked and recorded by + /// the handlers — see [`crate::privacy::crossing`]. + /// + /// Returns the target's classification on success, so the caller can decide + /// whether the write it is about to perform is a first crossing without + /// resolving the same row a third time. `None` is the master opt-out (DR-15) + /// — with enforcement off there is no tier to cross. async fn refuse_unless_writable( &self, cap: crate::privacy::CallCapability, - caller_session_id: &str, target_session_id: &str, - ) -> Result<(), String> { + ) -> Result, String> { self.refuse_unless_visible(cap, target_session_id).await?; if !cap.enforced() { - return Ok(()); + return Ok(None); } let target = self @@ -1581,16 +1622,8 @@ impl WorkspaceClient { .get_session(target_session_id, false) .await .map_err(|_| crate::privacy::refusal::workspace_out_of_reach())?; - let lineage = if target.id == caller_session_id { - crate::privacy::visibility::Lineage::Zelf - } else { - crate::privacy::visibility::lineage_of( - target.parent_session_id.as_deref(), - caller_session_id, - ) - }; - if crate::privacy::visibility::may_write(cap.tier(), target.privacy_tier, lineage) { - Ok(()) + if crate::privacy::visibility::may_write(cap.tier(), target.privacy_tier) { + Ok(Some(target.privacy_tier)) } else { Err(crate::privacy::refusal::workspace_out_of_reach()) } @@ -2468,6 +2501,96 @@ impl WorkspaceClient { } } + /// The RECORD half of the first-crossing disclosure — the half that decides + /// a pair has crossed and may stop being asked about. + /// + /// Three conditions, and the third is the one that is easy to leave out. + /// The write must have landed (the caller checks that); the write must + /// really have been a crossing (`requires_first_crossing_approval` against + /// the classification the gate resolved, so this cannot disagree with the + /// gate); and **there must have been something to disclose**, asked of the + /// same [`crossing_payload`] the inspector asks. + /// + /// ⚠ Without the third, a change the inspector considers payload-free + /// consumes the pair's one disclosure. `workspace_set_tools + /// { set_knowledge_bases: [] }` is exactly that: an accepted change that + /// clears the target's knowledge bases, raises no card from either + /// workspace inspector, and would otherwise mark the pair as crossed — so + /// the caller's next `workspace_send_prompt` into that conversation would + /// ship its payload to a public model in silence, with no approval ever + /// having been shown. + /// + /// [`crossing_payload`]: crate::agents::workspace_inspector::crossing_payload + fn record_crossing_if_disclosed( + cap: crate::privacy::CallCapability, + resolved_tier: Option, + caller_session_id: &str, + target_session_id: &str, + tool_name: &str, + arguments: Option<&JsonObject>, + ) { + let Some(tier) = resolved_tier else { return }; + if !crate::privacy::visibility::requires_first_crossing_approval(cap.tier(), tier) { + return; + } + let Some(args) = arguments else { return }; + if crate::agents::workspace_inspector::crossing_payload(tool_name, args).is_none() { + return; + } + crate::privacy::crossing::record(caller_session_id, target_session_id); + } + + /// **§3c: make the target's open tab render this NOW, not on reload.** + /// + /// The durable row is published on the session bus by whichever code path + /// made it durable — `send_prompt_note` here, the drain loop for `steer`, + /// `Agent::reply` for `turn`. That is necessary and not sufficient: a + /// `publish` is a pure lookup and a no-op when the session has no + /// subscriber, and **a tab the user opened has no subscriber**. Nothing in + /// the renderer attaches an observer to an ordinary tab; it is normally + /// driven by its own `/reply` stream, and an idle tab has nothing to listen + /// to. Cross-chat injection is exactly the case that breaks that assumption. + /// + /// So the tab is asked to attach one. The frame carries no placement, no + /// focus and no annotation: the daemon is not moving the user's tabs, it is + /// telling the window that this conversation is now changing underneath it. + /// + /// ⚠ **This is deliberately NOT ordered against the publish, and must not + /// be made to depend on it.** An observer's first frame is a full + /// `UpdateConversation` snapshot read from the store, so a tab that attaches + /// *after* the publish still renders the injected row. Whichever arrives + /// first, the message shows; requiring a handshake to win a race against a + /// broadcast would be the fragile version of this. + /// + /// Best-effort, like [`Self::notify_target`]: a frame that cannot be + /// delivered never fails the tool. + async fn reflect_in_target_tab(&self, session_id: &str) { + if let Some(services) = workspace_services::get() { + if services.gui_attached() { + // ⚠ `gui_command_near`, not `gui_command`. The plain form + // resolves to `focused_or_recent()` — ONE window, the focused + // one — so a conversation open in a background window while the + // user works in another gets its attach frame delivered to the + // wrong renderer, which answers "no tab in this window" and + // attaches nothing. That is exactly the case this method exists + // for, and exactly the case where a user is least likely to + // notice a conversation being steered. `gui_command_near` + // routes by `bridge_for_session` and falls back to the focused + // window only when no window claims the session. + let _ = services + .gui_command_near( + json!({ + "type": "workspace", "cmd": "observe", + "session_id": session_id, + }), + false, + session_id, + ) + .await; + } + } + } + /// Decision 4, read from the RIGHT place — and read WITHOUT creating an /// agent. /// @@ -2507,6 +2630,10 @@ impl WorkspaceClient { cap: crate::privacy::CallCapability, arguments: Option, ) -> Result, String> { + // Kept before `parse_args` consumes it: the record half of the + // first-crossing disclosure asks `crossing_payload` with the SAME raw + // arguments the inspector asked it with. See the note on that function. + let raw_arguments = arguments.clone(); let args: WorkspaceSendPromptParams = parse_args(arguments)?; if args.session_id == caller_session_id { return Err( @@ -2517,22 +2644,24 @@ impl WorkspaceClient { return Err("text must not be empty".into()); } // Issue #56, design §7 row 5 (`workspace_send_prompt` = ✗ at C=Pub, - // T=Priv, under every lineage). It is on this list as a **reader** as - // well as a writer, and that is the half easy to miss: `mode:"turn"` - // with `wait:"final_message"` parks on the target's turn and returns - // its final assistant message verbatim — a private conversation's - // content, arriving through a tool whose name says "send". + // T=Priv). It is on this list as a **reader** as well as a writer, and + // that is the half easy to miss: `mode:"turn"` with + // `wait:"final_message"` parks on the target's turn and returns its + // final assistant message verbatim — a private conversation's content, + // arriving through a tool whose name says "send". // // Placed after the two pure argument checks above (neither touches the // store, and neither can say anything about the target) and before // `caller_provenance`, which is this handler's first store read. // - self.refuse_unless_writable(cap, caller_session_id, &args.session_id) - .await?; + // The gate no longer asks about lineage: an unrelated conversation is a + // legal target, a private one still is not. + let write_target = self.refuse_unless_writable(cap, &args.session_id).await?; let provenance = self.caller_provenance(caller_session_id).await; let services = workspace_services::get(); + let target_session_id = args.session_id.clone(); - match args.mode.as_str() { + let delivered = match args.mode.as_str() { "note" => self.send_prompt_note(args, provenance).await, "steer" => { self.send_prompt_steer(caller_session_id, args, provenance, services) @@ -2543,7 +2672,25 @@ impl WorkspaceClient { .await } other => Err(format!("unknown mode '{other}' (turn | steer | note)")), + }; + // The disclosure's other half. `WorkspaceCrossingInspector` asked, in + // the caller's own turn, whether this (caller, target) pair had crossed + // yet and raised the payload for approval if it had not; this is the + // record that it now has, taken only once the write has actually + // landed. Recording at the gate instead would let a denied approval — + // or a refusal underneath it — buy silence for the retry. + if delivered.is_ok() { + self.reflect_in_target_tab(&target_session_id).await; + Self::record_crossing_if_disclosed( + cap, + write_target, + caller_session_id, + &target_session_id, + "workspace_send_prompt", + raw_arguments.as_ref(), + ); } + delivered } /// `mode:"note"` — leave context on the target without running it. @@ -2594,6 +2741,25 @@ impl WorkspaceClient { .add_message_adopting_uid(&args.session_id, &mut message) .await .map_err(|e| format!("failed to append note: {e}"))?; + // AFTER the row is durable, and with the row's own identity: an open tab + // renders this the moment it lands instead of on the next reload. + // + // ⚠ The order is the whole point. `add_message_adopting_uid` stamps the + // minted uid onto `message` (#41), so publishing here sends the stored + // row; publishing a pre-write copy would paint a bubble with no `id`, + // which the renderer cannot reconcile against the stored twin that + // arrives with the next snapshot — a message that renders and then + // duplicates or vanishes is worse than one that arrives late. + // + // `publish` is a pure lookup and a no-op when the target has no + // observer, so this costs a hash lookup for a conversation nobody is + // watching, and the durable row is the only thing anyone relies on. + crate::session_events::publish( + &args.session_id, + crate::session_events::SessionBusEvent::Agent(crate::agents::AgentEvent::Message( + message, + )), + ); Ok(vec![Content::text(format!( "Note appended to session {} (no turn started; preserved across \ compaction).", @@ -2810,6 +2976,9 @@ impl WorkspaceClient { cap: crate::privacy::CallCapability, arguments: Option, ) -> Result, String> { + // See `handle_send_prompt`: the record half asks `crossing_payload` + // with the same raw arguments the inspector did. + let raw_arguments = arguments.clone(); let args: WorkspaceSetToolsParams = parse_args(arguments)?; // ⚠ **A conversation may not re-tool ITSELF through this door.** @@ -2848,8 +3017,7 @@ impl WorkspaceClient { // conversation must certainly not re-tool one. FIRST, before any store // read that could answer a question about the target. // - self.refuse_unless_writable(cap, caller_session_id, &args.session_id) - .await?; + let write_target = self.refuse_unless_writable(cap, &args.session_id).await?; // ---- Resolve EVERYTHING before mutating anything, so a bad name is a // clean no-op rather than a half-applied change. ------------------ @@ -2886,51 +3054,10 @@ impl WorkspaceClient { }; if let Some(agent) = &agent { - // **Gate F1's UNLOAD half, at this tool's own door (issue #56, - // finding 14's SECOND door).** `manage_extensions {disable}` has - // asked `assert_extension_manageable` since finding 14 landed; this - // handler reached the very same executor — `Agent::remove_extension`, - // a passthrough to `ExtensionManager::remove_extension` — with no - // privacy decision anywhere on the path. So the capability was gated - // at one entrance and open at the other, and the reachable caller is - // the one finding 14 names: a chat classified Private but bound to a - // public model passes `refuse_unless_visible` for its own row, and - // then unloaded the private connector the public model may not see, - // may not call into, and may not name. - // - // ⚠ **The same predicate as the other door, called by name — not a - // second spelling of it.** `assert_extension_manageable` is - // `assert_extension_reachable(&normalize(name), Some(admitted))` - // verbatim, so all three of its consequences arrive here too, and - // all three are wanted: an unknown name reads Private and is refused - // (which is what stops this refusal being the existence oracle - // `add_extensions` needed a comment to avoid), the name is - // normalized to the key the executor removes under, and a model - // bound to another institution may see a mismatched connector but - // may not unload it. Writing the rule out here in this file's own - // words is exactly how these two doors drifted apart in the first - // place. - // - // ⚠ **Asked on the TARGET's manager**, because it is the target's - // loaded set that is about to change and the tier is a property of - // that entry — while `cap` is the CALLER's, because the caller is - // the one being entitled. Both halves matter when the two - // conversations differ. - // - // ⚠ **BEFORE `apply_extension_changes`, not inside its remove loop**, - // 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. - for name in &args.remove_extensions { - agent - .extension_manager - .assert_extension_manageable(name, cap) - .await - .map_err(|e| e.message.to_string())?; - } applied.extend( - Self::apply_extension_changes( + Self::apply_extension_changes_gated( agent, + cap, &args.session_id, add_configs, &args.remove_extensions, @@ -2981,6 +3108,18 @@ impl WorkspaceClient { ) .await; + // The first-crossing disclosure's record half, exactly as in + // `handle_send_prompt`: taken once the change is applied, never at the + // gate that asked about it. + Self::record_crossing_if_disclosed( + cap, + write_target, + caller_session_id, + &args.session_id, + "workspace_set_tools", + raw_arguments.as_ref(), + ); + let next_turn_note = if applied.iter().any(|a| a.starts_with("model=")) { " The model change applies to this conversation's NEXT turn." } else { @@ -2993,6 +3132,63 @@ impl WorkspaceClient { ))]) } + /// The extension half of `workspace_set_tools`: **Gate F1's unload check, + /// then the apply**. Lifted out of the handler whole — body and reasoning + /// together — when the handler outgrew the per-function line baseline; the + /// 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, + cap: crate::privacy::CallCapability, + session_id: &str, + add_configs: Vec, + remove_extensions: &[String], + ) -> Result, String> { + // **Gate F1's UNLOAD half, at this tool's own door (issue #56, + // finding 14's SECOND door).** `manage_extensions {disable}` has + // asked `assert_extension_manageable` since finding 14 landed; this + // handler reached the very same executor — `Agent::remove_extension`, + // a passthrough to `ExtensionManager::remove_extension` — with no + // privacy decision anywhere on the path. So the capability was gated + // at one entrance and open at the other, and the reachable caller is + // the one finding 14 names: a chat classified Private but bound to a + // public model passes `refuse_unless_visible` for its own row, and + // then unloaded the private connector the public model may not see, + // may not call into, and may not name. + // + // ⚠ **The same predicate as the other door, called by name — not a + // second spelling of it.** `assert_extension_manageable` is + // `assert_extension_reachable(&normalize(name), Some(admitted))` + // verbatim, so all three of its consequences arrive here too, and + // all three are wanted: an unknown name reads Private and is refused + // (which is what stops this refusal being the existence oracle + // `add_extensions` needed a comment to avoid), the name is + // normalized to the key the executor removes under, and a model + // bound to another institution may see a mismatched connector but + // may not unload it. Writing the rule out here in this file's own + // words is exactly how these two doors drifted apart in the first + // place. + // + // ⚠ **Asked on the TARGET's manager**, because it is the target's + // loaded set that is about to change and the tier is a property of + // that entry — while `cap` is the CALLER's, because the caller is + // the one being entitled. Both halves matter when the two + // conversations differ. + // + // ⚠ **BEFORE `apply_extension_changes`, not inside its remove loop**, + // 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. + for name in remove_extensions { + agent + .extension_manager + .assert_extension_manageable(name, cap) + .await + .map_err(|e| e.message.to_string())?; + } + Self::apply_extension_changes(agent, session_id, add_configs, remove_extensions).await + } + /// **Gate F1, at the workspace's own two enable doors** (issue #56, /// finding 4). /// @@ -3409,8 +3605,10 @@ impl WorkspaceClient { self.refuse_unless_direct_subagent_child(caller_session_id, &args.session_id) .await?; } - self.refuse_unless_writable(cap, caller_session_id, &args.session_id) - .await?; + // The target's classification is not needed here: `workspace_close` + // carries no payload, so there is nothing for a first crossing to + // disclose. + self.refuse_unless_writable(cap, &args.session_id).await?; let services = workspace_services::get(); let background = background_subagent_for(caller_session_id, &args.session_id); @@ -5745,45 +5943,21 @@ pub(crate) mod tests { )) } + /// Call a workspace tool as `meta`'s session. + /// + /// ⚠ **This used to LINK the target to the caller first** — it rewrote the + /// target's `parent_session_id` to the caller's id before every + /// `send_prompt` / `set_tools` / `close`, because the write rule was + /// `VIS ∧ L ∈ {self, child}` and a tier assertion could not otherwise get + /// past it. That fabrication is gone with the rule, and its removal is a + /// strengthening rather than a tidy-up: every tier test below now drives an + /// UNRELATED conversation, which is the case the old helper was quietly + /// converting into a related one. async fn call_as( c: &WorkspaceClient, tool: &str, args: serde_json::Value, meta: crate::agents::mcp_client::McpMeta, - ) -> CallToolResult { - if matches!( - tool, - "workspace_send_prompt" | "workspace_set_tools" | "workspace_close" - ) { - if let Some(target) = args.get("session_id").and_then(serde_json::Value::as_str) { - if target != meta.session_id - && c.context - .session_manager - .get_session(target, false) - .await - .is_ok() - { - c.context - .session_manager - .update(target) - .parent_session_id(Some(meta.session_id.clone())) - .apply() - .await - .expect("test target can be linked to its caller"); - } - } - } - let args: rmcp::model::JsonObject = serde_json::from_value(args).unwrap(); - c.call_tool(tool, Some(args), meta, CancellationToken::new()) - .await - .unwrap() - } - - async fn call_as_without_test_lineage( - c: &WorkspaceClient, - tool: &str, - args: serde_json::Value, - meta: crate::agents::mcp_client::McpMeta, ) -> CallToolResult { let args: rmcp::model::JsonObject = serde_json::from_value(args).unwrap(); c.call_tool(tool, Some(args), meta, CancellationToken::new()) @@ -5791,8 +5965,23 @@ pub(crate) mod tests { .unwrap() } + /// **The headline behaviour of the cross-chat injection change**, and the + /// exact inverse of the test it replaces. + /// + /// `workspace_writes_are_limited_to_direct_children` built this same + /// parent / child / grandchild / unrelated fixture and asserted that + /// `send_prompt`, `set_tools` and `close` into the grandchild and the + /// unrelated conversation were REFUSED with `workspace_out_of_reach()`. + /// The write rule no longer reads lineage, so all four targets are + /// reachable. Rewritten rather than deleted: this is where an accidental + /// re-narrowing shows up as a failure. + /// + /// The reachability is asserted as an EFFECT, not as a non-error — the note + /// has to be in the target's conversation afterwards. A handler that + /// reported success and appended nothing would satisfy `is_error != true`. #[tokio::test] - async fn workspace_writes_are_limited_to_direct_children() { + #[serial_test::serial(workspace_services)] + async fn workspace_writes_reach_any_visible_conversation_not_only_children() { use crate::session::session_manager::SessionType; let c = client(); @@ -5834,62 +6023,100 @@ pub(crate) mod tests { crate::privacy::CallCapability::for_test_restricted(), ) }; - let child_result = call_as_without_test_lineage( - &c, - "workspace_send_prompt", - serde_json::json!({ - "session_id": child.id, "text": "direct-child-marker", "mode": "note" - }), - meta(), - ) - .await; - assert_ne!( - child_result.is_error, - Some(true), - "{}", - text_of(&child_result) - ); - for target in [&unrelated.id, &grandchild.id] { - let refused = call_as_without_test_lineage( + // A direct child, a TRANSITIVE grandchild (one hop was the old rule's + // limit) and a conversation with no relationship to the caller at all. + for (label, target) in [ + ("child", &child.id), + ("grandchild", &grandchild.id), + ("unrelated", &unrelated.id), + ] { + let marker = format!("reaches-{label}"); + let sent = call_as( &c, "workspace_send_prompt", serde_json::json!({ - "session_id": target, "text": "must-not-land", "mode": "note" + "session_id": target, "text": marker, "mode": "note" }), meta(), ) .await; - assert_eq!(refused.is_error, Some(true), "{}", text_of(&refused)); + assert_ne!(sent.is_error, Some(true), "{label}: {}", text_of(&sent)); + + let after = text_of( + &call_as( + &c, + "workspace_read_conversation", + serde_json::json!({ "session_id": target }), + meta(), + ) + .await, + ); assert!( - text_of(&refused).contains(&crate::privacy::refusal::workspace_out_of_reach()), - "{}", - text_of(&refused) + after.contains(&marker), + "{label}: the injection reported success and landed nowhere: {after}" ); } - for (tool, args) in [ - ( - "workspace_set_tools", - serde_json::json!({ - "session_id": unrelated.id, "add_extensions": ["unknown-extension"] - }), - ), - ( + // The two other write verbs widened with it. `set_tools` is driven with + // an unknown extension so the assertion is about the GATE and not about + // whether the machine running the test happens to have one installed: + // an out-of-reach target refuses before the name is resolved, so a + // reachable one must fail differently. + let retooled = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ + "session_id": unrelated.id, "add_extensions": ["unknown-extension"] + }), + meta(), + ) + .await; + let retooled = text_of(&retooled); + assert!( + !retooled.contains(&crate::privacy::refusal::workspace_out_of_reach()), + "set_tools still refuses an unrelated conversation as out of reach: {retooled}" + ); + + // `close` is asserted the same way and for a second reason: every scope + // it has needs the daemon, which a unit test does not have, so "not + // out of reach" is the strongest true statement available here. + let closed = text_of( + &call_as( + &c, "workspace_close", - serde_json::json!({ "session_id": unrelated.id, "scope": "everything" }), - ), - ] { - let refused = call_as_without_test_lineage(&c, tool, args, meta()).await; - assert_eq!(refused.is_error, Some(true), "{}", text_of(&refused)); - assert!( - text_of(&refused).contains(&crate::privacy::refusal::workspace_out_of_reach()), - "{tool}: {}", - text_of(&refused) - ); - } + serde_json::json!({ "session_id": unrelated.id, "scope": "turn" }), + meta(), + ) + .await, + ); + assert!( + !closed.contains(&crate::privacy::refusal::workspace_out_of_reach()), + "close still refuses an unrelated conversation as out of reach: {closed}" + ); + } + + /// The tier is still a boundary for a caller that opted the feature OUT, + /// which is a different thing from the lineage rule going away: with + /// enforcement off nothing is refused, and this pins that the widened rule + /// did not accidentally become the only reason writes succeed. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn an_opted_out_caller_still_writes_into_an_unrelated_conversation() { + use crate::session::session_manager::SessionType; - let opted_out = call_as_without_test_lineage( + let c = client(); + let sm = c.context.session_manager.clone(); + let caller = sm + .create_session(std::env::temp_dir(), "parent".into(), SessionType::User) + .await + .unwrap(); + let unrelated = sm + .create_session(std::env::temp_dir(), "unrelated".into(), SessionType::User) + .await + .unwrap(); + + let opted_out = call_as( &c, "workspace_send_prompt", serde_json::json!({ @@ -5945,7 +6172,7 @@ pub(crate) mod tests { ) .with_workspace_child_scope_only(true) }; - let readable = call_as_without_test_lineage( + let readable = call_as( &c, "workspace_read_conversation", serde_json::json!({ "session_id": child.id, "view": "summary" }), @@ -5968,7 +6195,7 @@ pub(crate) mod tests { serde_json::json!({ "session_id": unrelated.id, "scope": "turn" }), ), ] { - let refused = call_as_without_test_lineage(&c, tool, args, meta()).await; + let refused = call_as(&c, tool, args, meta()).await; assert_eq!( refused.is_error, Some(true), @@ -6137,6 +6364,91 @@ pub(crate) mod tests { assert!(ids.contains(&f.public_id), "{ids:?}"); } + /// **`mode:"note"` reflects in an open tab live.** The other two modes reach + /// the bus through the target's own turn (steer via the drain loop, turn via + /// `Agent::reply` at the point the row is persisted); `note` starts no turn, + /// so there is nothing else to carry it and the handler publishes it itself. + /// Before this, an open tab showed an appended note only after a reload. + /// + /// Asserted on the DURABLE row. `add_message_adopting_uid` stamps the minted + /// uid onto the message, so a frame with `id: None` would be one published + /// before the write — a bubble the renderer cannot reconcile against the + /// stored twin arriving with the next snapshot. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn an_appended_note_reaches_the_targets_open_tab_live() { + use crate::session::session_manager::SessionType; + const NOTE: &str = "br71-note-live-marker"; + + let c = client(); + let sm = c.context.session_manager.clone(); + let caller = sm + .create_session(std::env::temp_dir(), "caller".into(), SessionType::User) + .await + .unwrap(); + let target = sm + .create_session(std::env::temp_dir(), "target".into(), SessionType::User) + .await + .unwrap(); + + // Subscribed BEFORE the call: the bus is a broadcast with no replay, and + // `publish` is a pure lookup that is a NO-OP when nobody is listening — + // so a subscription opened afterwards would pass whatever happened. + let mut observer = crate::session_events::subscribe(&target.id); + + let sent = call_as( + &c, + "workspace_send_prompt", + serde_json::json!({ + "session_id": target.id, "text": NOTE, "mode": "note" + }), + crate::agents::mcp_client::McpMeta::new( + caller.id, + crate::privacy::CallCapability::for_test_restricted(), + ), + ) + .await; + assert_ne!(sent.is_error, Some(true), "{}", text_of(&sent)); + + let mut published = None; + while let Ok(event) = observer.try_recv() { + if let SessionBusEvent::Agent(crate::agents::AgentEvent::Message(m)) = event { + if m.content + .iter() + .any(|content| content.as_text().is_some_and(|text| text.contains(NOTE))) + { + published = Some(m); + } + } + } + let published = published.expect( + "the note was appended but never published, so an open tab would show \ + it only after a reload", + ); + assert!( + published.id.is_some(), + "the note was published before it was durable" + ); + + // The published row IS the stored row, not a look-alike. + let stored = sm + .get_session(&target.id, true) + .await + .unwrap() + .conversation + .expect("the target has a conversation"); + assert!( + stored.messages().iter().any(|m| m.id == published.id), + "the published uid names no stored message" + ); + // And the provenance survives onto the frame, so the renderer attributes + // it to the sending conversation rather than drawing it as the user's own. + assert_eq!( + published.metadata.provenance.as_ref().map(|p| p.kind), + Some(crate::conversation::message::ProvenanceKind::AgentInjection), + ); + } + /// §7 row 5. `workspace_send_prompt` is on the gated list as a **reader** as /// well as a writer: `mode:"turn"` with `wait:"final_message"` returns the /// target's final assistant message verbatim. @@ -6146,6 +6458,7 @@ pub(crate) mod tests { /// note must not be in the conversation afterwards, read back by a caller /// that is allowed to look. #[tokio::test] + #[serial_test::serial(workspace_services)] async fn a_public_caller_cannot_inject_into_a_private_conversation() { let f = tier_fixture().await; const INJECTED: &str = "workspace-tier-injection-marker"; @@ -7230,8 +7543,8 @@ pub(crate) mod tests { // §7 column C, at every handler that names another conversation. // Four read-only handler sites call the read gate directly; its helper // contains the fifth occurrence. Three mutating handlers call the - // write gate, which composes the same read gate before its lineage - // check. Together those are read_conversation, open (existing), + // write gate, which composes the same read gate before asking + // `may_write`. Together those are read_conversation, open (existing), // send_prompt, set_tools, close, watch, and the panel pair (which share // one handler). Exact counts make a new door fail this audit until its // gate is chosen deliberately. @@ -7701,6 +8014,7 @@ pub(crate) mod tests { } #[tokio::test] + #[serial_test::serial(workspace_services)] async fn send_prompt_note_appends_with_provenance_without_running_a_turn() { use crate::conversation::message::ProvenanceKind; let c = client(); @@ -8175,15 +8489,37 @@ pub(crate) mod tests { // Bind out of the guard before the early return: a live `MutexGuard` // across the tail of an `async fn` makes the future `!Send`. let failure = self.gui_error.lock().unwrap().clone(); - let queued = self.gui_answers.lock().unwrap().pop_front(); match failure { Some(message) => Err(message), // A fire-and-forget emit never carries an `ok` — the real // `ServerWorkspaceServices` answers `{"sent": true}` — so a // caller that did not wait cannot learn anything, and the fake // must not hand it a verdict it could not have had. + // + // ⚠ **It must not CONSUME one either, and that is a fix rather + // than a tidy-up.** The `pop_front` used to happen above this + // match, so an emit that could not return an answer still took + // one off the queue — directly contradicting the sentence above + // it. Any staged reply was then stolen from the read it was + // staged for, which fell through to the `{"ok": true}` default + // and made a test fail on an assertion about something else + // entirely. `panel_secret_guard_rejects_top_level_and_wrapped_locators` + // is the one that showed it: it stages two secret-bearing panel + // replies and consumes them with two reads, so one stolen answer + // turns its second read into a harmless default and the + // SecretGuard refusal it asserts on never happens. + // + // Every `notify_target` toast is such an emit, and BR-71 §3c + // added a second (`reflect_in_target_tab`, on every accepted + // injection), which is what made a rare pre-existing flake + // frequent enough to catch. None if !wait_result => Ok(serde_json::json!({ "sent": true })), - None => Ok(queued.unwrap_or_else(|| serde_json::json!({ "ok": true }))), + None => Ok(self + .gui_answers + .lock() + .unwrap() + .pop_front() + .unwrap_or_else(|| serde_json::json!({ "ok": true }))), } } } @@ -9179,6 +9515,250 @@ pub(crate) mod tests { ); } + /// **§3c, the half a bus publish cannot do on its own.** Publishing the + /// durable row is necessary and not sufficient: `session_events::publish` is + /// a pure lookup that is a NO-OP when the session has no subscriber, and a + /// tab the *user* opened has no subscriber — nothing in the renderer + /// attaches an observer to an ordinary tab. So every accepted injection also + /// asks the window holding that conversation to attach one. + /// + /// All three modes, because all three change a conversation somebody may be + /// looking at. `note` is the one that would be easiest to skip and the one + /// that needs it most: it starts no turn, so there is no other frame of any + /// kind heading for that tab. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn every_injection_asks_the_targets_tab_for_a_live_feed() { + use crate::agents::agent::TurnId; + let c = client(); + let caller = c + .context + .session_manager + .create_session( + std::env::temp_dir(), + "injector".into(), + crate::session::session_manager::SessionType::User, + ) + .await + .unwrap(); + + // note — no daemon work at all beyond the frame. + let note_target = seeded_target(&c, "note-target").await; + let services = FakeServices::with_gui(true).install(); + let sent = send_prompt( + &c, + &caller.id, + serde_json::json!({ + "session_id": note_target, "text": "context for later", "mode": "note" + }), + ) + .await; + assert_ne!(sent.is_error, Some(true), "{}", text_of(&sent)); + let frame = services + .frame_with_cmd("observe") + .expect("a note must ask its target's tab to attach a live feed"); + assert_eq!(frame["session_id"], serde_json::json!(note_target)); + // No placement, no focus, no annotation: the daemon is not moving the + // user's tabs, only telling the window this conversation is changing. + assert!(frame.get("placement").is_none(), "{frame}"); + assert!(frame.get("focus").is_none(), "{frame}"); + crate::workspace_services::clear_test_override(); + + // steer — a live turn on the target. + let steer_target = seeded_target(&c, "steer-feed-target").await; + let services = FakeServices::with_gui(true).busy(&steer_target).install(); + let manager = crate::execution::manager::AgentManager::instance() + .await + .expect("agent manager"); + let agent = manager + .get_or_create_agent(steer_target.clone()) + .await + .expect("agent"); + agent.open_for_turn(TurnId::new("feed-turn-live")); + let sent = send_prompt( + &c, + &caller.id, + serde_json::json!({ + "session_id": steer_target, "text": "narrow it to 2019", "mode": "steer" + }), + ) + .await; + assert_ne!(sent.is_error, Some(true), "{}", text_of(&sent)); + assert!( + services.frame_with_cmd("observe").is_some(), + "a steer must ask its target's tab to attach a live feed; got {:?}", + services.all_frames() + ); + let _ = agent.drain_soft_interrupts(); + crate::workspace_services::clear_test_override(); + + // turn — a detached turn started on an idle target. + let turn_target = seeded_target(&c, "turn-feed-target").await; + let services = FakeServices::with_gui(true).install(); + let sent = send_prompt( + &c, + &caller.id, + serde_json::json!({ + "session_id": turn_target, "text": "start on the QC pass", "mode": "turn" + }), + ) + .await; + assert_ne!(sent.is_error, Some(true), "{}", text_of(&sent)); + assert!( + services.frame_with_cmd("observe").is_some(), + "a turn must ask its target's tab to attach a live feed; got {:?}", + services.all_frames() + ); + crate::workspace_services::clear_test_override(); + } + + /// **A write with nothing to disclose must not mark the pair as crossed.** + /// + /// The behavioural half of + /// `workspace_inspector::tests::a_change_with_nothing_to_disclose_has_no_payload_to_record`, + /// and the half that can actually fail: that one asserts what + /// `crossing_payload` answers, which stays true however + /// `record_crossing_if_disclosed` is wired. This one drives the real handler + /// and reads the ledger afterwards, so dropping the payload check goes red + /// here and nowhere else. + /// + /// `workspace_set_tools { set_knowledge_bases: [] }` is an accepted change + /// that clears the target's bases and raises no approval card. If it could + /// record a crossing, a private caller would silence a public target's one + /// disclosure with a call the user never saw — and the next + /// `workspace_send_prompt` would ship its payload in silence. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn a_payload_free_change_cannot_silence_the_first_crossing_disclosure() { + use crate::session::session_manager::SessionType; + let c = client(); + let sm = c.context.session_manager.clone(); + let caller = sm + .create_session( + std::env::temp_dir(), + "private lead".into(), + SessionType::User, + ) + .await + .unwrap(); + let target = sm + .create_session( + std::env::temp_dir(), + "public worker".into(), + SessionType::User, + ) + .await + .unwrap(); + let _services = FakeServices::with_gui(true).install(); + + // Precondition: this pair has not crossed, so the disclosure is owed. + crate::privacy::crossing::reset_for_test(); + let owed = || { + crate::privacy::crossing::needs_disclosure( + crate::privacy::ProviderTier::Private, + crate::privacy::SessionClassification::Public, + &caller.id, + &target.id, + ) + }; + assert!(owed(), "precondition: the pair has not crossed yet"); + + let cleared = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ "session_id": target.id, "set_knowledge_bases": [] }), + crate::agents::mcp_client::McpMeta::new( + caller.id.clone(), + crate::privacy::CallCapability::for_test( + crate::privacy::ProviderTier::Private, + true, + ), + ), + ) + .await; + // The change really landed — otherwise this test would pass against a + // handler that refused it, which proves nothing about the ledger. + assert_ne!(cleared.is_error, Some(true), "{}", text_of(&cleared)); + assert!( + text_of(&cleared).contains("kb="), + "the clearing change did not apply, so the ledger was never at risk: {}", + text_of(&cleared) + ); + + assert!( + owed(), + "a payload-free change marked the pair as crossed; the caller's next \ + injection into this conversation would cross to a public model with no \ + approval ever having been shown" + ); + + // The mirror: a change that DOES carry a payload records, so this is not + // "set_tools never records". + let retooled = call_as( + &c, + "workspace_set_tools", + serde_json::json!({ + "session_id": target.id, "set_knowledge_bases": ["ms-cohort"] + }), + crate::agents::mcp_client::McpMeta::new( + caller.id.clone(), + crate::privacy::CallCapability::for_test( + crate::privacy::ProviderTier::Private, + true, + ), + ), + ) + .await; + assert_ne!(retooled.is_error, Some(true), "{}", text_of(&retooled)); + assert!( + !owed(), + "a disclosed change failed to record, so the user would be asked again \ + for a pair they have already approved" + ); + crate::privacy::crossing::reset_for_test(); + crate::workspace_services::clear_test_override(); + } + + /// The control for the test above. A REFUSED injection must not ask for + /// anything: an attach frame for a write that did not happen tells the + /// window a conversation is changing when it is not, and — for a refusal + /// that is about the privacy tier — would confirm to the caller's window + /// that the named conversation exists. + #[tokio::test] + #[serial_test::serial(workspace_services)] + async fn a_refused_injection_asks_for_no_live_feed() { + let c = client(); + let caller = c + .context + .session_manager + .create_session( + std::env::temp_dir(), + "injector".into(), + crate::session::session_manager::SessionType::User, + ) + .await + .unwrap(); + let services = FakeServices::with_gui(true).install(); + + // Idle target, `steer` — refused because there is no turn to redirect. + let target = seeded_target(&c, "idle-target").await; + let refused = send_prompt( + &c, + &caller.id, + serde_json::json!({ + "session_id": target, "text": "too late", "mode": "steer" + }), + ) + .await; + assert_eq!(refused.is_error, Some(true), "{}", text_of(&refused)); + assert!( + services.frame_with_cmd("observe").is_none(), + "a refused injection asked the tab to attach anyway: {:?}", + services.all_frames() + ); + crate::workspace_services::clear_test_override(); + } + /// #69: the server's turn lock and the agent's interrupt queue can disagree, /// and only the queue is authoritative. /// diff --git a/crates/biorouter/src/agents/workspace_inspector.rs b/crates/biorouter/src/agents/workspace_inspector.rs index 7f4e0bb01..32271d36c 100644 --- a/crates/biorouter/src/agents/workspace_inspector.rs +++ b/crates/biorouter/src/agents/workspace_inspector.rs @@ -387,6 +387,254 @@ impl ToolInspector for WorkspaceMutationInspector { // every other call. } +/// The tool families that carry a PAYLOAD into another conversation, and so +/// have something for a first crossing to disclose. `workspace_close` is +/// deliberately absent: it carries no text. +/// +/// ⚠ **Two halves ask this, and they must be the same half.** The inspector +/// below asks it to decide whether to raise the card; `handle_send_prompt` and +/// `handle_set_tools` ask it to decide whether a landed write may mark the pair +/// as having crossed. A record for a change this function calls payload-free +/// would consume the pair's one disclosure without ever having shown one — and +/// that is not hypothetical: `workspace_set_tools { set_knowledge_bases: [] }` +/// is a real, accepted change (it CLEARS the target's bases) that produces no +/// payload here, so a handler recording on "the write succeeded" alone let a +/// caller silence the disclosure with a call the user never saw. Asking one +/// function is what makes the two halves unable to disagree. +pub(crate) fn crossing_payload(tool_name: &str, args: &JsonObject) -> Option<(String, String)> { + let target = args.get("session_id").and_then(serde_json::Value::as_str)?; + let payload = if is_send_prompt_call(tool_name) { + let mode = args + .get("mode") + .and_then(serde_json::Value::as_str) + .unwrap_or("turn"); + let text = args.get("text").and_then(serde_json::Value::as_str)?; + format!("mode {mode} — {text}") + } else if is_set_tools_call(tool_name) { + // Not the prose a model wrote, but still caller-chosen content that + // reconfigures a public conversation: which extensions, skills, model + // and knowledge bases it will hold. + let mut parts: Vec = Vec::new(); + for key in [ + "add_extensions", + "remove_extensions", + "add_skills", + "remove_skills", + "set_knowledge_bases", + ] { + let names = string_list(args, key); + if !names.is_empty() { + parts.push(format!("{key}: {}", names.join(", "))); + } + } + for key in ["provider", "model"] { + if let Some(v) = args.get(key).and_then(serde_json::Value::as_str) { + parts.push(format!("{key}: {v}")); + } + } + if parts.is_empty() { + return None; + } + parts.join("; ") + } else { + return None; + }; + Some((target.to_string(), payload)) +} + +pub(crate) fn is_send_prompt_call(tool_name: &str) -> bool { + tool_name == "workspace_send_prompt" || tool_name == "workspace__workspace_send_prompt" +} + +/// **The first-crossing disclosure** (issue #56, DR-16's `✓!` cells): a +/// private-capability conversation writing into a PUBLIC one shows the user the +/// exact payload, once per (caller, target) pair, in every permission mode. +/// +/// ⚠ **This predicate shipped unwired, and widening the write rule is what made +/// wiring it non-optional.** While WRITE was `VIS ∧ L ∈ {self, child}`, the only +/// public targets a private caller could write into were ones it had spawned +/// itself. It can now write into any public conversation on the machine — one +/// the user opened, is reading, and never connected to this agent — so the +/// moment private-origin text leaves for a public model is a moment the user has +/// to be able to see. `crates/biorouter/tests/privacy_guard_wiring.rs` carried +/// the predicate as `Status::Unwired("OPERATOR DECISION OUTSTANDING")` until +/// this inspector existed. +/// +/// Sibling of [`WorkspaceMutationInspector`] and deliberately a SEPARATE +/// inspector rather than another `reason` arm inside it: that one asks a pure +/// question about the arguments, where this one has to resolve the caller's +/// bound provider and the target's stored classification. Folding an async, +/// I/O-bearing decision into a pure one is how the pure one stops being +/// testable. +/// +/// Like its sibling it has **no mode gate**: `apply_inspection_results_to_permissions` +/// promotes `RequireApproval` over another inspector's `Allow`, so this beats +/// Auto mode's blanket allow. Unlike its sibling it cannot be a unit-pure +/// function, so the decision is split — [`crossing_payload`] and +/// [`crate::privacy::crossing::needs_disclosure`] are both testable without an +/// agent, and this `inspect` is the two of them plus two lookups. +/// The refusal a dispatch boundary that never reaches a [`ToolInspector`] must +/// return for a workspace write that would be a **first crossing**. +/// +/// ⚠ **An inspector is not a gate at every door.** `ExtensionManager::dispatch_tool_call` +/// is reached from four places and only one of them runs the inspector stack; +/// the JS sandbox's tool handler hands a script's inner calls straight to it +/// (`agents/code_execution_extension.rs`), which is why that file already +/// carries boundary refusals for the global memory store and the session +/// database. This is the third, and it is needed for a sharper reason than the +/// other two: skipping the card would not merely let ONE undisclosed write +/// through — the handler would then **record the pair as crossed**, so every +/// later, properly-inspected write to that same conversation would be silent +/// too. One un-inspected call would permanently disable the disclosure. +/// +/// Deliberately narrow. It refuses only what would otherwise have raised a +/// card: a same-tier write is untouched, so is a pair the user has already +/// approved, and so is every tool that carries no payload. A script that needs +/// to make a first crossing is told to make the call where the user can see it. +pub async fn uninspected_crossing_refusal( + cap: crate::privacy::CallCapability, + caller_session_id: &str, + tool_name: &str, + args: Option<&JsonObject>, + boundary: crate::security::UninspectedBoundary, +) -> Option { + if !cap.enforced() || !cap.tier().is_private() { + return None; + } + let (target, _) = crossing_payload(tool_name, args?)?; + let row = crate::session::session_manager::SessionManager::instance() + .get_session(&target, false) + .await + .ok()?; + if !crate::privacy::crossing::needs_disclosure( + cap.tier(), + row.privacy_tier, + caller_session_id, + &target, + ) { + return None; + } + tracing::warn!( + counter.biorouter.workspace_crossing_uninspected_refused = 1, + tool_name = %tool_name, + target_session = %target, + boundary = ?boundary, + "Refused a first-crossing workspace write at a boundary no inspector sees" + ); + Some(format!( + "Refused: this conversation runs on a model hosted inside your institution, and \ + {tool_name} would send text to conversation {target}, which does not. The first \ + time that happens the user has to see the exact payload and approve it — and a \ + call made from inside a script never reaches the approval. Call {tool_name} \ + directly instead of from `execute_code`; after the user approves once, this pair \ + of conversations stops asking." + )) +} + +pub struct WorkspaceCrossingInspector { + provider: crate::agents::types::SharedProvider, +} + +impl WorkspaceCrossingInspector { + pub fn new(provider: crate::agents::types::SharedProvider) -> Self { + Self { provider } + } +} + +#[async_trait] +impl ToolInspector for WorkspaceCrossingInspector { + fn name(&self) -> &'static str { + "workspace_tier_crossing" + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } + + async fn inspect( + &self, + tool_requests: &[ToolRequest], + _messages: &[Message], + _biorouter_mode: BioRouterMode, + session: &Session, + ) -> Result> { + // Cheapest discriminator first: a turn with no workspace write in it + // must not sample the provider mutex or touch the session store. + let candidates: Vec<(&ToolRequest, String, String)> = tool_requests + .iter() + .filter_map(|request| { + let tool_call = request.tool_call.as_ref().ok()?; + let args = tool_call.arguments.as_ref()?; + let (target, payload) = crossing_payload(&tool_call.name, args)?; + Some((request, target, payload)) + }) + .collect(); + if candidates.is_empty() { + return Ok(Vec::new()); + } + + // ONE sample, for the whole batch, at one instant — the same discipline + // `CallCapability` exists to enforce. Re-reading the mutex per request + // would let two calls in one batch gate on two different models. + let cap = crate::privacy::CallCapability::sample(&self.provider).await; + if !cap.enforced() || !cap.tier().is_private() { + return Ok(Vec::new()); + } + + let session_manager = crate::session::session_manager::SessionManager::instance(); + let mut results = Vec::new(); + for (request, target, payload) in candidates { + // Metadata-only: resolving the tier must never be the way to load + // the conversation the disclosure is about. + let Ok(row) = session_manager.get_session(&target, false).await else { + // An unresolvable target is refused by the handler's own gate a + // moment later, in one sentence that does not say whether the + // conversation exists. Escalating here would answer that. + continue; + }; + if !crate::privacy::crossing::needs_disclosure( + cap.tier(), + row.privacy_tier, + &session.id, + &target, + ) { + continue; + } + tracing::warn!( + counter.biorouter.workspace_tier_crossing_disclosed = 1, + tool_request_id = %request.id, + target_session = %target, + "Private-to-public workspace write escalated to approval (issue #56)" + ); + results.push(InspectionResult { + tool_request_id: request.id.clone(), + action: InspectionAction::RequireApproval(Some(format!( + // ⚠ The payload goes LAST, and it is fenced. The card renders + // this string into a single element, so every newline in it + // collapses to a space — measured in the running app, where + // "…the 2019 relapse counts Approving sends this now" read as + // one sentence and the reader could not tell where the + // quoted text stopped and Biorouter's own words resumed. + // That matters more here than in an ordinary confirmation: + // the whole point of the card is that the user can see + // exactly what would leave, so its boundary has to be + // unambiguous even with the whitespace gone. + "🔒 This conversation is on a model hosted inside your institution. \ + It is about to send text to conversation {target}, which is NOT. \ + Approving sends it now, and stops asking for this pair of \ + conversations. This confirmation appears in every permission mode, \ + including Fully Automatic. ⟪WHAT IT WOULD SEND⟫ {payload} ⟪END⟫" + ))), + reason: format!("Private-to-public workspace write into {target}"), + confidence: 1.0, + inspector_name: self.name().to_string(), + finding_id: Some(format!("WSXING-{}", Uuid::new_v4().simple())), + }); + } + Ok(results) + } +} + #[cfg(test)] mod tests { use super::*; @@ -721,4 +969,230 @@ mod tests { }))) .is_none()); } + + /// **The disclosure is REGISTERED.** Every assertion in this module and in + /// `tests/workspace_crossing_disclosure.rs` builds the inspector by hand, so + /// all of them stay green if `create_tool_inspection_manager` stops adding + /// it — which is the failure this campaign has shipped five times under a + /// different name: the mechanism is built, the entry point is never called, + /// and every unit test passes because the unit is correct. + /// + /// A source scan, not a behavioural test, because the thing being asserted + /// is an absence elsewhere. It reads the production half of `agent.rs` (cut + /// at its test module, with a negative control proving the cut landed) and + /// requires the constructor call to be there. + #[test] + fn the_disclosure_inspector_is_registered_in_the_agent_loop() { + const AGENT: &str = include_str!("agent.rs"); + // ⚠ The LAST such module, not the first. `agent.rs` carries a nested + // `#[cfg(test)] mod tests` inside another module some 3,000 lines above + // the file-level one, and cutting at the first match put the whole + // inspector registry on the "tests" side — while the negative control + // below still passed, because its marker sits below both. Measured, not + // reasoned about: the first-match version of this scan failed on a tree + // where the registration was present and correct. + let cut = AGENT + .match_indices("mod tests {") + .filter_map(|(i, _)| { + let before = AGENT.get(..i)?.trim_end(); + let before = before + .strip_suffix("pub(crate)") + .unwrap_or(before) + .trim_end(); + let before = before.strip_suffix("pub").unwrap_or(before).trim_end(); + before + .ends_with("#[cfg(test)]") + .then(|| before.len() - "#[cfg(test)]".len()) + }) + .last() + .expect("agent.rs has a `#[cfg(test)]` test module, so this scan cuts it there"); + let (production, tests) = AGENT.split_at(cut); + + // The control, FIRST — otherwise a cut that landed at the end of the + // file would make the assertion below pass vacuously. + // `agent_with_one_extension_for_tests` is spelled only by the file-level + // test module. + assert!( + !production.contains("agent_with_one_extension_for_tests"), + "the cut did not remove the test module, so the assertion below proves nothing" + ); + assert!( + tests.contains("agent_with_one_extension_for_tests"), + "the cut removed more than the test module" + ); + + assert!( + production.contains("WorkspaceCrossingInspector::new("), + "the first-crossing disclosure has no production registration: the \ + inspector exists, the agent loop never adds it, and every test that \ + constructs one by hand still passes" + ); + } + + // ---------------------------------------------------------------- the + // first-crossing disclosure. `crossing_payload` is the pure half — + // `WorkspaceCrossingInspector::inspect` is the two lookups around it, and + // the ledger it consults has its own tests in `privacy::crossing`. + + #[test] + fn a_send_prompt_payload_is_the_text_and_the_mode() { + let (target, payload) = crossing_payload( + "workspace_send_prompt", + &args(serde_json::json!({ + "session_id": "s-public", + "mode": "steer", + "text": "drop what you are doing and summarise the cohort", + })), + ) + .expect("a send_prompt with text has a payload to disclose"); + assert_eq!(target, "s-public"); + assert!(payload.contains("mode steer"), "{payload}"); + // The WHOLE text, verbatim. A disclosure that showed a summary would be + // the same as no disclosure — the user is being asked to judge exactly + // what leaves for the public model. + assert!( + payload.contains("drop what you are doing and summarise the cohort"), + "{payload}" + ); + } + + #[test] + fn the_prefixed_tool_name_is_recognised_too() { + // Both spellings reach dispatch (extension-advertised tools are prefixed, + // and the loop tolerates models that strip the prefix), so a disclosure + // keyed on one of them is a disclosure with a one-word bypass. + for name in ["workspace_send_prompt", "workspace__workspace_send_prompt"] { + assert!( + crossing_payload( + name, + &args(serde_json::json!({ + "session_id": "s", "mode": "note", "text": "x" + })), + ) + .is_some(), + "{name}" + ); + } + } + + #[test] + fn set_tools_discloses_what_it_would_change_and_close_discloses_nothing() { + let (_, payload) = crossing_payload( + "workspace_set_tools", + &args(serde_json::json!({ + "session_id": "s-public", + "add_skills": ["single-cell"], + "provider": "anthropic", + })), + ) + .expect("a set_tools that changes something has a payload"); + assert!(payload.contains("add_skills: single-cell"), "{payload}"); + assert!(payload.contains("provider: anthropic"), "{payload}"); + + // A no-op set_tools has nothing to disclose, so it must not raise a card + // the user cannot act on. + assert!(crossing_payload( + "workspace_set_tools", + &args(serde_json::json!({ "session_id": "s-public" })), + ) + .is_none()); + + // `workspace_close` carries no text at all. It is a write, and it is + // gated by the tier like the others — but there is nothing for a + // *disclosure* to show, and a card with an empty payload teaches the + // user to click through them. + assert!(crossing_payload( + "workspace_close", + &args(serde_json::json!({ "session_id": "s-public", "scope": "turn" })), + ) + .is_none()); + } + + /// **The ledger must not be poisonable by a write that disclosed nothing.** + /// + /// `workspace_set_tools { set_knowledge_bases: [] }` is an ACCEPTED change — + /// `set_knowledge_bases` is an `Option>`, so `Some(vec![])` is a real + /// request to clear the target's bases, and `handle_set_tools` applies it and + /// reports success. Neither workspace inspector raises a card for it, because + /// there is no payload to show. + /// + /// A record half that fired on "the write succeeded" alone would therefore + /// let a private caller consume a public target's one disclosure with a call + /// the user never saw, and the very next `workspace_send_prompt` into that + /// conversation would ship its payload to the public model in silence. That + /// is why `record_crossing_if_disclosed` asks THIS function rather than + /// re-deriving what counts as a payload. + #[test] + fn a_change_with_nothing_to_disclose_has_no_payload_to_record() { + assert!( + crossing_payload( + "workspace_set_tools", + &args(serde_json::json!({ + "session_id": "s-public", "set_knowledge_bases": [] + })), + ) + .is_none(), + "an empty set_knowledge_bases has no payload, so a write carrying one must \ + not be able to mark a pair as crossed" + ); + // The control: the same tool with something to show is still covered, so + // this is not "the predicate answers None for set_tools". + assert!(crossing_payload( + "workspace_set_tools", + &args(serde_json::json!({ + "session_id": "s-public", "set_knowledge_bases": ["ms-cohort"] + })), + ) + .is_some()); + } + + #[test] + fn a_call_naming_no_target_or_no_text_has_no_payload() { + assert!(crossing_payload( + "workspace_send_prompt", + &args(serde_json::json!({ "mode": "note", "text": "x" })), + ) + .is_none()); + assert!(crossing_payload( + "workspace_send_prompt", + &args(serde_json::json!({ "session_id": "s", "mode": "note" })), + ) + .is_none()); + } + + /// The inspector is INERT for a public-capability caller, and that is a + /// property rather than an optimisation: a public caller cannot write into a + /// private conversation at all (that is `may_write` refusing), so a card + /// here would announce a crossing that is not going to happen. + #[tokio::test] + async fn a_public_capability_caller_raises_no_disclosure() { + let inspector = + WorkspaceCrossingInspector::new(std::sync::Arc::new(tokio::sync::Mutex::new(None))); + let request = ToolRequest { + id: "req-1".into(), + tool_call: Ok(rmcp::model::CallToolRequestParams { + name: "workspace_send_prompt".into(), + arguments: Some(args(serde_json::json!({ + "session_id": "s-public", "mode": "note", "text": "x" + }))), + meta: None, + task: None, + }), + metadata: Default::default(), + tool_meta: Default::default(), + }; + // An unbound provider samples Public — the safe direction for every gate + // that reads a capability, and what this inspector must treat as "not my + // business" rather than as "unknown, so ask". + let results = inspector + .inspect( + std::slice::from_ref(&request), + &[], + BioRouterMode::Auto, + &Session::default(), + ) + .await + .unwrap(); + assert!(results.is_empty(), "{results:?}"); + } } diff --git a/crates/biorouter/src/privacy/crossing.rs b/crates/biorouter/src/privacy/crossing.rs new file mode 100644 index 000000000..a32c7c829 --- /dev/null +++ b/crates/biorouter/src/privacy/crossing.rs @@ -0,0 +1,152 @@ +//! The **first-crossing disclosure**: the state +//! [`visibility::requires_first_crossing_approval`] always needed and never had. +//! +//! A private-capability caller writing into a public conversation is permitted +//! (R4 — a private session may spawn and drive public children, and a rule that +//! lets you spawn one but never prompt it grants nothing). The prompt text is +//! still private-origin content arriving at a public model, so the **first** +//! such write from a given caller into a given target shows the user the exact +//! payload and waits for an answer. +//! +//! ⚠ **This module exists because retiring the lineage clause made the +//! disclosure load-bearing.** While WRITE was `VIS ∧ L ∈ {self, child}`, the +//! public targets a private caller could reach were the ones it had itself +//! spawned, so the crossing was one the caller had already arranged. It can now +//! write into any public conversation on the machine — a chat the user opened, +//! is reading, and never connected to this agent. The predicate had been sitting +//! unwired with an "OPERATOR DECISION OUTSTANDING" note against it; widening the +//! target set is what decided it. +//! +//! # Why the ledger is here and not in the permission store +//! +//! The approval card carries a `prompt`, and both the desktop and the CLI +//! deliberately suppress "Always Allow" whenever one is present +//! (`ToolCallConfirmation.tsx`, `session/mod.rs`'s `prompt_tool_confirmation`). +//! So the permission store can never learn this grant, and "first" has to be +//! remembered somewhere else. It is remembered per **(caller, target) pair**, +//! exactly as the predicate's own doc specifies. +//! +//! # Two properties that are easy to get backwards +//! +//! * **The crossing is recorded when the write LANDS, never when it is asked +//! about.** [`needs_disclosure`] is a question; [`record`] is the answer's +//! consequence. A refusal — the user denying the card, or the tier gate +//! refusing underneath it — records nothing, so the next attempt asks again. +//! Recording at the question would let one denied call buy silence for the +//! next one. +//! * **It is process-local and deliberately not persisted.** The disclosure is +//! about a running agent surprising a watching user, so "again after a +//! restart" is the safe direction; a durable grant would silently outlive the +//! session that earned it. + +use std::collections::HashSet; +use std::sync::{LazyLock, Mutex}; + +use super::{visibility, ProviderTier, SessionClassification}; + +/// Pairs that have already disclosed, as `(caller_session_id, target_session_id)`. +/// +/// Unbounded in principle, bounded in practice by the number of conversations a +/// single process holds; each entry is two session ids. +static CROSSED: LazyLock>> = + LazyLock::new(|| Mutex::new(HashSet::new())); + +/// Whether this write is a first crossing, i.e. whether it must disclose its +/// payload before it happens. +/// +/// `false` for every same-tier write, for a public caller (which cannot reach a +/// private target at all — that is a refusal, not a disclosure), and for a pair +/// that has already crossed. +pub fn needs_disclosure( + caller: ProviderTier, + target: SessionClassification, + caller_session_id: &str, + target_session_id: &str, +) -> bool { + if !visibility::requires_first_crossing_approval(caller, target) { + return false; + } + !already_crossed(caller_session_id, target_session_id) +} + +/// Record that this pair has now crossed. Called by the handler once the write +/// is committed, never by the gate that asks about it. +pub fn record(caller_session_id: &str, target_session_id: &str) { + lock().insert((caller_session_id.to_string(), target_session_id.to_string())); +} + +fn already_crossed(caller_session_id: &str, target_session_id: &str) -> bool { + lock().contains(&(caller_session_id.to_string(), target_session_id.to_string())) +} + +/// A poisoned mutex is not a reason to skip a disclosure, and it is also not a +/// reason to abort a turn: the guard is taken back and the worst case is that +/// the set is stale, which asks again. +fn lock() -> std::sync::MutexGuard<'static, HashSet<(String, String)>> { + CROSSED.lock().unwrap_or_else(|e| e.into_inner()) +} + +/// Test-only: forget every recorded crossing. +#[cfg(test)] +pub fn reset_for_test() { + lock().clear(); +} + +#[cfg(test)] +mod tests { + use super::*; + use ProviderTier::{Private as CPriv, Public as CPub}; + use SessionClassification::{Private as TPriv, Public as TPub}; + + /// Serialized against the process-global ledger, so each test starts clean + /// and two of them cannot interleave on the same pair. + fn guard() -> std::sync::MutexGuard<'static, ()> { + static SERIAL: Mutex<()> = Mutex::new(()); + let g = SERIAL.lock().unwrap_or_else(|e| e.into_inner()); + reset_for_test(); + g + } + + #[test] + fn only_a_private_caller_writing_into_a_public_target_discloses() { + let _g = guard(); + assert!(needs_disclosure(CPriv, TPub, "a", "b")); + // Same tier both ways: nothing is crossing, so nothing is disclosed. + assert!(!needs_disclosure(CPriv, TPriv, "a", "b")); + assert!(!needs_disclosure(CPub, TPub, "a", "b")); + // A public caller cannot reach a private target at all. That is + // `may_write` refusing, and a disclosure here would imply the write was + // about to happen. + assert!(!needs_disclosure(CPub, TPriv, "a", "b")); + } + + #[test] + fn the_disclosure_is_once_per_pair_and_not_once_per_caller() { + let _g = guard(); + assert!(needs_disclosure(CPriv, TPub, "caller", "first")); + record("caller", "first"); + assert!(!needs_disclosure(CPriv, TPub, "caller", "first")); + // A SECOND public target is a second crossing. Keying on the caller + // alone would let one approval cover every public chat on the machine. + assert!(needs_disclosure(CPriv, TPub, "caller", "second")); + // And the pair is ordered: the target having disclosed to someone else + // says nothing about this caller. + assert!(needs_disclosure(CPriv, TPub, "other-caller", "first")); + } + + #[test] + fn asking_does_not_record() { + let _g = guard(); + // The failure this rules out: a denied approval buying silence for the + // retry. `needs_disclosure` is called once per attempt and must answer + // the same way until a write actually lands. + for _ in 0..3 { + assert!( + needs_disclosure(CPriv, TPub, "caller", "target"), + "asking recorded the crossing, so a denial would silence the retry" + ); + } + record("caller", "target"); + assert!(!needs_disclosure(CPriv, TPub, "caller", "target")); + } +} diff --git a/crates/biorouter/src/privacy/mod.rs b/crates/biorouter/src/privacy/mod.rs index 935bcd58b..276e3db4a 100644 --- a/crates/biorouter/src/privacy/mod.rs +++ b/crates/biorouter/src/privacy/mod.rs @@ -21,6 +21,8 @@ pub mod affiliation; pub mod alt_provider; pub mod capability; pub mod config_keys; +// The (caller, target) state behind `visibility::requires_first_crossing_approval`. +pub mod crossing; pub mod declassify; pub mod disclosure; #[cfg(test)] diff --git a/crates/biorouter/src/privacy/visibility.rs b/crates/biorouter/src/privacy/visibility.rs index 072d222ce..63ea4febf 100644 --- a/crates/biorouter/src/privacy/visibility.rs +++ b/crates/biorouter/src/privacy/visibility.rs @@ -2,11 +2,33 @@ //! //! ```text //! VIS(T) <=> T <= C // a public caller sees public only -//! READ <=> VIS // any lineage — R6's read-only floor -//! WRITE <=> VIS && L in {self, child} +//! READ <=> VIS +//! WRITE <=> VIS //! BIND(P->T) <=> WRITE && tier(P) >= T // Gate A, evaluated on the target //! ``` //! +//! **The privacy tier is the only boundary, and lineage is not one.** WRITE used +//! to carry a second clause, `L in {self, child}`, so an agent could steer a +//! conversation it had spawned and only read one it had not. That is retired: +//! an agent may inject into ANY conversation it can see, related or not. What it +//! may never do is cross the tier — a public-capability caller is still refused +//! a private target under every verb, and a private caller writing into a public +//! one still discloses its payload on the first crossing +//! ([`requires_first_crossing_approval`]). +//! +//! The rule that replaced it is narrower than "anything goes" in two ways worth +//! naming, because both are what keep the retirement safe: +//! +//! * READ and WRITE now coincide, so **there is no verb that can reach a +//! conversation the caller could not already read in full**. Widening WRITE to +//! VIS adds no target; it removes a read-only cell. +//! * The delegation-scoped grant is a SEPARATE mechanism and is untouched. +//! `McpMeta::workspace_child_scope_only` still confines an auto-injected +//! supervision surface's read/close/watch to direct children +//! (`workspace_extension::refuse_unless_direct_subagent_child`), and a +//! `SessionType::SubAgent` is still refused every `workspace_*` tool outright. +//! Neither of those is `may_write`, and neither moved. +//! //! Design §7 is a nine-column table over three inputs. Written once, as pure //! functions, it is unit-testable without a database and BR-71's tool handlers //! can call it rather than re-deriving it — which is how one table becomes @@ -52,67 +74,36 @@ //! headless background-child watch. It returns a turn's end *reason*, not the //! conversation. //! * `workspace_close` and `workspace_set_tools` — writes that return no content. -//! §7's write row is `may_write`, i.e. VIS **and** lineage, and this change -//! implements no lineage anywhere; wiring half of it here would make the other -//! half look done. +//! Both go through `refuse_unless_writable`, so they ask [`may_write`]; this +//! bullet records only that neither returns a transcript, so neither needs the +//! read adapter above. use super::{visible_to, ProviderTier, SessionClassification}; -/// The lineage of a target session relative to the caller, as design §7 defines -/// it: **one hop, never transitive**. -/// -/// `Zelf` is spelled with a Z because `Self` is a keyword; it is the design's -/// `self` column. It is not produced by [`lineage_of`] — a caller establishes it -/// by comparing session ids before it ever looks at parentage — and it exists as -/// a variant because the matrix has a column for it and because `self` and -/// `child` behaving identically is a property worth being able to assert. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Lineage { - /// The target *is* the caller's own session. - Zelf, - /// The caller spawned the target directly: `target.parent_session_id == caller`. - Child, - /// Everything else — a sibling, an unrelated session, a transitive - /// grandchild, and any session with a NULL parent. - Other, +/// READ ⇔ VIS. A caller may read any session it can see, whether or not it +/// spawned it. +pub fn may_read(c: ProviderTier, t: SessionClassification) -> bool { + visible_to(c, t) } -/// Classify a target by its stored `parent_session_id` against the caller's own -/// session id. +/// WRITE ⇔ VIS. A caller may steer any session it can see — a child, a sibling, +/// an unrelated conversation — and may steer none that it cannot. /// -/// **One hop.** A grandchild is `Other`: R6 says "sessions the caller *did* -/// spawn", and a grandchild was spawned by the child. BR-71's -/// `workspace_list { parent_session_id: "" }` filter already yields exactly -/// the one-hop set, so no recursive CTE and no new "control my subtree" surface -/// is invented. A leader that needs deeper control asks its child. +/// ⚠ **This deliberately coincides with [`may_read`], and the duplication is the +/// point rather than an oversight.** WRITE used to be `VIS ∧ L ∈ {self, child}`, +/// which made an unrelated conversation a read-only cell; that clause is retired +/// (see the module header). Keeping WRITE as its own named predicate is what +/// gives the write gate somewhere to disagree with the read gate again — a later +/// narrowing lands here and is caught by the matrix test, instead of being +/// hand-written a second time inside whichever handler needed it. Collapsing the +/// two into one function is how one table becomes eight. /// -/// A NULL parent is `Other`, i.e. read-only — the safe direction, and what every -/// session predating `parent_session_id` (Task 6) carries. -/// -/// This function cannot return [`Lineage::Zelf`]: parentage does not encode -/// identity. A handler that has the target's id decides `Zelf` first: -/// `if target.id == caller { Zelf } else { lineage_of(target.parent_session_id, caller) }`. -/// Getting that wrong is not a privilege escalation — `Zelf` and `Child` are -/// merged under every rule of the matrix — but it is worth spelling out. -pub fn lineage_of(target_parent: Option<&str>, caller_session_id: &str) -> Lineage { - match target_parent { - Some(parent) if parent == caller_session_id => Lineage::Child, - _ => Lineage::Other, - } -} - -/// READ ⇔ VIS, under **any** lineage — R6's read-only floor. A caller may read -/// any session it can see, whether or not it spawned it. -pub fn may_read(c: ProviderTier, t: SessionClassification) -> bool { +/// A private-capability caller writing into a public target is permitted and +/// **discloses itself**: see [`requires_first_crossing_approval`]. +pub fn may_write(c: ProviderTier, t: SessionClassification) -> bool { visible_to(c, t) } -/// WRITE ⇔ VIS ∧ L ∈ {self, child}. Seeing a sibling does not license steering -/// it; that is what makes column B a read-only cell rather than a refusal. -pub fn may_write(c: ProviderTier, t: SessionClassification, l: Lineage) -> bool { - visible_to(c, t) && !matches!(l, Lineage::Other) -} - /// `workspace_list` OMITS private rows rather than redacting them: a row /// carries a title, and a session title in this product is LLM-generated from /// the conversation, i.e. content. Omission is one WHERE clause and removes the @@ -134,7 +125,13 @@ pub fn appears_in_list(c: ProviderTier, t: SessionClassification) -> bool { /// /// "First" is per (caller, target) pair and is state the *caller* of this /// predicate keeps; this function is the pure classifier of whether a crossing -/// is happening at all. +/// is happening at all. That state, and the one call to this predicate, live in +/// [`super::crossing`] — for a long time neither existed and the disclosure +/// documented above never fired, which is what +/// `crates/biorouter/tests/privacy_guard_wiring.rs` recorded as an outstanding +/// operator decision. Retiring the lineage clause is what settled it: the set of +/// public targets a private caller can write into is no longer "the ones it +/// spawned". pub fn requires_first_crossing_approval(c: ProviderTier, t: SessionClassification) -> bool { c.is_private() && !t.is_private() } @@ -193,41 +190,46 @@ mod tests { #[test] fn the_capability_matrix_matches_the_design_table_cell_for_cell() { - use Lineage::{Child, Other, Zelf}; - // Columns A..G of design §7. `self` and `child` behave identically under - // every rule and are merged in the table; D and F are what prove it, so - // both are enumerated here rather than assumed. + // Design §7 was a NINE-column table over three inputs (caller tier, + // target classification, lineage). Lineage is retired, so the eight + // lineage columns A/B, D/E and F/G collapse pairwise onto the four rows + // below — and the collapse is the assertion: the `write` column of the + // `Other` half used to read `false` in three of those rows. #[rustfmt::skip] let cases = [ - // caller target lineage read write list-visible - ( CPub, TPub, Zelf, true, true, true ), // A - ( CPub, TPub, Child, true, true, true ), // A - ( CPub, TPub, Other, true, false, true ), // B — R6's read-only floor - ( CPub, TPriv, Zelf, false, false, false), // C — row OMITTED, not redacted - ( CPub, TPriv, Child, false, false, false), // C - ( CPub, TPriv, Other, false, false, false), // C - ( CPriv, TPub, Zelf, true, true, true ), // D - ( CPriv, TPub, Child, true, true, true ), // D - ( CPriv, TPub, Other, true, false, true ), // E - ( CPriv, TPriv, Zelf, true, true, true ), // F - ( CPriv, TPriv, Child, true, true, true ), // F - ( CPriv, TPriv, Other, true, false, true ), // G + // caller target read write list-visible + ( CPub, TPub, true, true, true ), // A+B + ( CPub, TPriv, false, false, false), // C — row OMITTED, not redacted + ( CPriv, TPub, true, true, true ), // D+E, and a first-crossing disclosure + ( CPriv, TPriv, true, true, true ), // F+G ]; - for (c, t, l, read, write, list) in cases { - assert_eq!(may_read(c, t), read, "read {c:?}/{t:?}/{l:?}"); - assert_eq!(may_write(c, t, l), write, "write {c:?}/{t:?}/{l:?}"); - assert_eq!(appears_in_list(c, t), list, "list {c:?}/{t:?}/{l:?}"); + for (c, t, read, write, list) in cases { + assert_eq!(may_read(c, t), read, "read {c:?}/{t:?}"); + assert_eq!(may_write(c, t), write, "write {c:?}/{t:?}"); + assert_eq!(appears_in_list(c, t), list, "list {c:?}/{t:?}"); } } + /// The headline behaviour change, asserted as a property rather than as a + /// row of the table above: **WRITE is exactly READ**. + /// + /// The retired rule differed from READ in precisely the three cells where + /// the caller could see the target but had not spawned it. Stating the + /// equality over the whole cross product is what makes a re-narrowing fail + /// here — a second clause reintroduced on any one cell breaks it, whichever + /// cell it is, without anyone having to remember to add a case. #[test] - fn a_grandchild_is_other_and_a_null_parent_is_other() { - // Lineage is ONE hop: R6 says "sessions the caller DID spawn", and a - // grandchild was spawned by the child. NULL parent is `other` => read-only, - // which is the safe direction and is what every pre-upgrade subagent has. - assert_eq!(lineage_of(Some("me"), "me"), Lineage::Child); - assert_eq!(lineage_of(Some("my-child"), "me"), Lineage::Other); - assert_eq!(lineage_of(None, "me"), Lineage::Other); + fn write_reaches_every_session_the_caller_can_read() { + for c in [CPub, CPriv] { + for t in [TPub, TPriv] { + assert_eq!( + may_write(c, t), + may_read(c, t), + "write and read disagree at {c:?}/{t:?}; the tier is the only \ + boundary, so a caller that may read a conversation may steer it" + ); + } + } } /// **The assertion that would have caught the release blocker: these @@ -336,7 +338,7 @@ mod tests { // lets you spawn one but never send it a prompt makes the permission // useless. The prompt text IS private-origin content crossing into a // public model, so the FIRST crossing per (caller,target) discloses it. - assert!(may_write(CPriv, TPub, Lineage::Zelf)); + assert!(may_write(CPriv, TPub)); assert!(requires_first_crossing_approval(CPriv, TPub)); assert!(!requires_first_crossing_approval(CPriv, TPriv)); assert!(!requires_first_crossing_approval(CPub, TPub)); diff --git a/crates/biorouter/src/providers/tool_turn.rs b/crates/biorouter/src/providers/tool_turn.rs index a60f49602..25c939cb1 100644 --- a/crates/biorouter/src/providers/tool_turn.rs +++ b/crates/biorouter/src/providers/tool_turn.rs @@ -461,6 +461,16 @@ fn workflow_inspectors(tool_risks: Arc) -> ToolInspectionManag use crate::security::security_inspector::SecurityInspector; use crate::security::sensitive_ops::SensitiveOpsInspector; + // ⚠ **Neither workspace inspector is here, and that is a latent gap rather + // than a decision.** `WorkspaceMutationInspector` (§5's always-confirm on a + // cross-session capability change) and `WorkspaceCrossingInspector` (issue + // #56's first-crossing payload disclosure) are registered only in + // `Agent::create_tool_inspection_manager`. It costs nothing today: the one + // production caller of this stack is `knowledge/provider_completer.rs`, + // which supplies a bridge over the knowledge sub-agent's own `kb_*` surface + // and cannot reach a `workspace_*` tool at all. The moment anything routes + // the workspace surface through here, both controls silently do not apply — + // so add them in the same change, do not discover it afterwards. let managed = Arc::new(ManagedPolicy::empty()); let mut manager = ToolInspectionManager::new(); manager.add_inspector(Box::new(ManagedPolicyInspector::new(Arc::clone(&managed)))); diff --git a/crates/biorouter/src/session/session_manager.rs b/crates/biorouter/src/session/session_manager.rs index 73febdeac..43749e179 100644 --- a/crates/biorouter/src/session/session_manager.rs +++ b/crates/biorouter/src/session/session_manager.rs @@ -234,7 +234,11 @@ pub struct Session { /// Id of the parent session that spawned this one as a subagent (BR-71). /// Sibling of `diverged_from` (branch lineage): `diverged_from` records a /// user fork; this records a delegation. `None` for non-subagent sessions. - /// It is also what the §7 capability matrix's `L` axis reads (issue #56). + /// It is what the interface groups tabs by, and what + /// `refuse_unless_direct_subagent_child` reads to keep a delegation-scoped + /// grant pointed at its own children. It is NOT read by the §7 capability + /// matrix any more: that matrix had an `L` axis until lineage stopped being + /// a boundary for writes (issue #56). #[serde(default)] pub parent_session_id: Option, /// How sensitive this session's contents are (issue #56). A permanent diff --git a/crates/biorouter/tests/privacy_capability.rs b/crates/biorouter/tests/privacy_capability.rs index 75916300b..58da810e0 100644 --- a/crates/biorouter/tests/privacy_capability.rs +++ b/crates/biorouter/tests/privacy_capability.rs @@ -133,6 +133,22 @@ const EXPECTED: &[Site] = &[ which is the whole reason this type exists; re-reading per callback \ would be the two-reads race with a process boundary through the middle", }, + Site { + needle: "CallCapability::sample(", + file: "crates/biorouter/src/agents/workspace_inspector.rs", + count: 1, + what: "`WorkspaceCrossingInspector::inspect`, the first-crossing \ + disclosure. A `ToolInspector` runs BEFORE the dispatch that would \ + admit a capability — that is the point of an inspector — so there \ + is none in scope to inherit, and the alternative to sampling here \ + is not inheriting but deciding on `Config::global()`, which is the \ + bug this type exists to prevent. Sampled ONCE per batch rather than \ + per request, because two calls in one batch must not be able to \ + gate on two different models, and only after a cheap name check \ + has established that the batch contains a cross-session write at \ + all: an ordinary turn must not pay a provider-mutex read for a \ + disclosure that cannot apply to it", + }, Site { needle: "CallCapability::sample(", file: "crates/biorouter/src/agents/knowledge_tool.rs", diff --git a/crates/biorouter/tests/privacy_guard_wiring.rs b/crates/biorouter/tests/privacy_guard_wiring.rs index 8071c2b13..24e024733 100644 --- a/crates/biorouter/tests/privacy_guard_wiring.rs +++ b/crates/biorouter/tests/privacy_guard_wiring.rs @@ -233,8 +233,10 @@ const REGISTRY: &[Guard] = &[ Guard { ident: "may_write", defined_in: VISIBILITY, - decides: "WRITE ⇔ VIS ∧ lineage ∈ {self, child}: whether a caller may steer a session, \ - not merely see it", + decides: "WRITE ⇔ VIS: whether a caller may steer a session, which is now every \ + session it may read. The lineage clause it used to carry — steer what you \ + spawned, read everything else — is retired: an agent may inject into any \ + conversation, and the tier is the only boundary", status: Status::Wired, sites: &[Site { file: "crates/biorouter/src/agents/workspace_extension.rs", @@ -243,32 +245,42 @@ const REGISTRY: &[Guard] = &[ what: "the shared writable adapter used by send_prompt, set_tools and close", }], }, - Guard { - ident: "lineage_of", - defined_in: VISIBILITY, - decides: "classifies a target as self / child / other from its stored \ - `parent_session_id`, one hop and never transitive", - status: Status::Wired, - sites: &[Site { - file: "crates/biorouter/src/agents/workspace_extension.rs", - counts: c(1, 0, 0), - kind: SiteKind::Guard, - what: "the shared writable adapter classifies the named target before `may_write`", - }], - }, Guard { ident: "requires_first_crossing_approval", defined_in: VISIBILITY, decides: "whether a write is a downgrade crossing (private caller → public target) and \ so must disclose its payload the first time", - status: Status::Unwired( - "OPERATOR DECISION OUTSTANDING. The documented disclosure (the first \ - `workspace_send_prompt` / `workspace_set_tools` from a given caller into a given \ - public target raises an approval showing the exact payload) never fires. There \ - is no caller and no (caller, target) first-crossing state anywhere in the tree. \ - The predicate is pure and correct; the state it needs was never built.", - ), - sites: &[], + // ⚠ This row read `Status::Unwired("OPERATOR DECISION OUTSTANDING")` for a long + // time, and what settled the decision was widening the write rule. While WRITE + // carried its lineage clause, the only public targets a private caller could write + // into were ones it had spawned itself; now it can write into any public + // conversation on the machine, so the moment private-origin text leaves for a + // public model is one the user has to be able to see. The state the predicate + // always needed is `privacy/crossing.rs`. + status: Status::Wired, + sites: &[ + Site { + file: "crates/biorouter/src/privacy/crossing.rs", + counts: c(1, 0, 0), + kind: SiteKind::Guard, + what: "`needs_disclosure`, which crosses the pure predicate with the \ + (caller, target) ledger the disclosure is keyed on. The inspector \ + that raises the approval — `WorkspaceCrossingInspector` — asks \ + through here rather than asking the predicate itself, so there is \ + one place that knows what 'first' means", + }, + Site { + file: "crates/biorouter/src/agents/workspace_extension.rs", + counts: c(1, 0, 0), + kind: SiteKind::Guard, + what: "`record_crossing_if_disclosed`, the RECORD half, called by both \ + `handle_send_prompt` and `handle_set_tools`. ONE call site for two \ + handlers, deliberately: the pair is marked as crossed only once the \ + write has landed AND only when there was something to disclose, and \ + a second copy of that pair of conditions is how one of them goes \ + missing", + }, + ], }, // ------------------------------------------------------- the HTTP reach // gate. `session_id` is a request parameter, not a credential. diff --git a/crates/biorouter/tests/soft_interrupt_agent_loop.rs b/crates/biorouter/tests/soft_interrupt_agent_loop.rs index 3db7cdeae..1d650dc6f 100644 --- a/crates/biorouter/tests/soft_interrupt_agent_loop.rs +++ b/crates/biorouter/tests/soft_interrupt_agent_loop.rs @@ -333,6 +333,70 @@ async fn an_agent_originated_steer_is_framed_as_untrusted() { assert_eq!(stamp.from_session_name.as_deref(), Some("Research chat")); } +/// **`workspace_send_prompt mode:"steer"` reflects in an open tab live**, and +/// the reason it does is worth stating because it is not where you would look. +/// +/// `send_prompt_steer` itself only queues and raises a toast — it publishes +/// nothing, deliberately. The message reaches the session bus through the +/// TARGET's own turn: the drain loop persists the queued steer with +/// `add_message_adopting_uid` and then yields `AgentEvent::Message`, and the one +/// turn runner tees every yielded event onto the bus +/// (`biorouter-server/src/workspace/turn.rs`). Adding a second publish in +/// `send_prompt_steer` would double-render it AND would publish a copy that is +/// not durable yet, which is the failure mode the whole "publish after the row +/// is durable" rule exists to prevent. +/// +/// So what has to hold here is that the yielded row is the STORED row: it +/// carries the minted uid, and that uid names a message in the session. A yield +/// with `id: None` is one an observing tab cannot reconcile against the stored +/// twin it gets on the next snapshot. +#[tokio::test(flavor = "multi_thread")] +async fn an_injected_steer_is_yielded_only_once_it_is_durable() { + let payload = "br71-steer-durable-marker"; + let provider = Arc::new(SteeringProvider::stamped( + payload, + MessageProvenance { + kind: ProvenanceKind::AgentInjection, + from_session_id: Some("sender-session".to_string()), + from_session_name: Some("Research chat".to_string()), + }, + )); + let (agent, session_id, _work_dir) = agent_with_provider(provider.clone()).await; + + let messages = drain(&agent, "Plot the data", &session_id).await.unwrap(); + + let injected = messages + .iter() + .filter(|m| m.role == rmcp::model::Role::User) + .find(|m| { + m.content.iter().any(|c| match c { + MessageContent::Text(t) => t.text.contains(payload), + _ => false, + }) + }) + .expect("the injected steer must be yielded as a Message"); + + let uid = injected + .id + .as_deref() + .expect("the yielded steer must carry the uid the store minted for it"); + + let stored = shared_session_manager() + .get_session(&session_id, true) + .await + .unwrap() + .conversation + .expect("the session has a conversation"); + assert!( + stored + .messages() + .iter() + .any(|m| m.id.as_deref() == Some(uid)), + "the steer was yielded under a uid that names no stored row, so it was \ + published before it was durable" + ); +} + /// BR-71: a steer stamped `UserDirect` — the human typing into a subagent's own /// tab — is stamped but NOT framed. Framing it would wrap the user's own words /// in "treat this as lower-trust data" and tell the model to discount them. diff --git a/crates/biorouter/tests/workspace_crossing_disclosure.rs b/crates/biorouter/tests/workspace_crossing_disclosure.rs new file mode 100644 index 000000000..30e95f4cc --- /dev/null +++ b/crates/biorouter/tests/workspace_crossing_disclosure.rs @@ -0,0 +1,351 @@ +//! **The first-crossing disclosure, end to end** (issue #56, design §7's `✓!` +//! cells): a private-capability conversation writing into a PUBLIC one shows the +//! user the exact payload before it is sent, once per (caller, target) pair. +//! +//! # Why this is an integration binary and not a unit test +//! +//! Two of the three inputs are process-global. `WorkspaceCrossingInspector` +//! resolves the target through `SessionManager::instance()` — the real store, +//! which a unit test in `workspace_extension`'s own module deliberately does not +//! use — and `privacy::crossing`'s ledger is a process-global set. So the test +//! needs a whole process it can point at a temp directory, which is what a +//! separate test binary is. +//! +//! ⚠ **`BIOROUTER_PATH_ROOT` must be set before ANYTHING touches the store.** +//! `SESSION_STORAGE` is a `LazyLock` over `Paths::data_dir()`; once it has been +//! forced, this test would be reading and writing the developer's own +//! `sessions.db`. It is set once, by the first test to run, and every test in +//! this file goes through `fixture()` so none of them can forget. + +use std::sync::Arc; + +use async_trait::async_trait; +use biorouter::agents::workspace_inspector::WorkspaceCrossingInspector; +use biorouter::config::BioRouterMode; +use biorouter::conversation::message::{Message, ToolRequest}; +use biorouter::model::ModelConfig; +use biorouter::privacy::{ProviderTier, SessionClassification}; +use biorouter::providers::base::{Provider, ProviderMetadata, ProviderUsage}; +use biorouter::providers::errors::ProviderError; +use biorouter::session::session_manager::{SessionManager, SessionType}; +use biorouter::tool_inspection::{InspectionAction, ToolInspector}; +use rmcp::model::Tool; + +/// A provider that exists only to answer `tier()`. Named after a real private +/// provider so nothing downstream can decide it is public by name. +struct InstitutionalModel; + +#[async_trait] +impl Provider for InstitutionalModel { + fn metadata() -> ProviderMetadata { + ProviderMetadata::empty() + } + fn get_name(&self) -> &str { + "versa_azure" + } + fn tier(&self) -> ProviderTier { + ProviderTier::Private + } + fn get_model_config(&self) -> ModelConfig { + ModelConfig::new_or_fail("test-model") + } + async fn complete_with_model( + &self, + _model_config: &ModelConfig, + _system: &str, + _messages: &[Message], + _tools: &[Tool], + ) -> Result<(Message, ProviderUsage), ProviderError> { + unreachable!("this fixture exists only to be asked its tier") + } +} + +fn send_prompt_request(id: &str, target: &str, text: &str) -> ToolRequest { + ToolRequest { + id: id.to_string(), + tool_call: Ok(rmcp::model::CallToolRequestParams { + name: "workspace_send_prompt".into(), + arguments: Some( + serde_json::json!({ "session_id": target, "mode": "note", "text": text }) + .as_object() + .unwrap() + .clone(), + ), + meta: None, + task: None, + }), + metadata: Default::default(), + tool_meta: Default::default(), + } +} + +/// Point the process-global store at a temp directory, once, before anything +/// forces the `LazyLock`. Serialized because `set_var` is, and because the +/// crossing ledger below it is process-global too. +fn store_root() -> &'static tempfile::TempDir { + static ROOT: std::sync::OnceLock = std::sync::OnceLock::new(); + ROOT.get_or_init(|| { + let root = tempfile::TempDir::new().unwrap(); + // SAFETY: inside a `OnceLock` initializer, so exactly one thread runs + // it, and it runs before any test in this binary reads the store. + unsafe { + std::env::set_var("BIOROUTER_PATH_ROOT", root.path()); + } + root + }) +} + +#[tokio::test] +#[serial_test::serial(workspace_crossing)] +async fn a_private_chat_writing_into_a_public_one_discloses_its_payload_exactly_once() { + let root = store_root(); + + const PAYLOAD: &str = "the MS cohort's 2019 relapse counts, verbatim"; + + let sm = SessionManager::instance(); + let caller = sm + .create_session( + root.path().to_path_buf(), + "private lead".into(), + SessionType::User, + ) + .await + .unwrap(); + let public_target = sm + .create_session( + root.path().to_path_buf(), + "public worker".into(), + SessionType::User, + ) + .await + .unwrap(); + let private_target = sm + .create_session( + root.path().to_path_buf(), + "private peer".into(), + SessionType::User, + ) + .await + .unwrap(); + sm.update(&private_target.id) + .raise_privacy( + SessionClassification::Private, + "test:workspace-crossing-disclosure", + ) + .apply() + .await + .unwrap(); + // The ratchet really fired. Without this the "same tier, no disclosure" + // assertion below would pass against a target that is merely public. + assert_eq!( + sm.get_session(&private_target.id, false) + .await + .unwrap() + .privacy_tier, + SessionClassification::Private, + ); + + let provider: biorouter::agents::types::SharedProvider = + Arc::new(tokio::sync::Mutex::new(Some(Arc::new(InstitutionalModel)))); + let inspector = WorkspaceCrossingInspector::new(provider); + + let caller_session = sm.get_session(&caller.id, false).await.unwrap(); + let inspect = |request: ToolRequest| { + let inspector = &inspector; + let session = caller_session.clone(); + async move { + inspector + .inspect(&[request], &[], BioRouterMode::Auto, &session) + .await + .unwrap() + } + }; + + // 1. The crossing. Private caller, public target, first time. + let results = inspect(send_prompt_request("req-1", &public_target.id, PAYLOAD)).await; + assert_eq!(results.len(), 1, "no disclosure was raised: {results:?}"); + let prompt = match &results[0].action { + InspectionAction::RequireApproval(Some(prompt)) => prompt.clone(), + other => panic!("the disclosure must be a RequireApproval carrying text, got {other:?}"), + }; + // The payload, VERBATIM. A card that summarised what was about to be sent + // would leave the user approving something they cannot check — and the whole + // point of this gate is that they can. + assert!( + prompt.contains(PAYLOAD), + "the approval did not show the payload: {prompt}" + ); + assert!( + prompt.contains(&public_target.id), + "the approval did not name the target: {prompt}" + ); + + // 2. Asking again — with no write having landed — asks again. This is the + // denial case: a user who says no must be asked on the retry, not + // silently obeyed. + let again = inspect(send_prompt_request("req-2", &public_target.id, PAYLOAD)).await; + assert_eq!( + again.len(), + 1, + "a second attempt was let through, so denying the first would have bought \ + silence for it" + ); + + // 3. Once the write has landed, the pair has crossed and stops asking. + biorouter::privacy::crossing::record(&caller.id, &public_target.id); + let after = inspect(send_prompt_request("req-3", &public_target.id, PAYLOAD)).await; + assert!( + after.is_empty(), + "the disclosure repeated for a pair that has already crossed: {after:?}" + ); + + // 4. A DIFFERENT public target is a different crossing. Keying the ledger on + // the caller alone would let one approval cover every public conversation + // on the machine. + let other_public = sm + .create_session( + root.path().to_path_buf(), + "another worker".into(), + SessionType::User, + ) + .await + .unwrap(); + let second = inspect(send_prompt_request("req-4", &other_public.id, PAYLOAD)).await; + assert_eq!(second.len(), 1, "a second public target was not disclosed"); + + // 5. A same-tier write crosses nothing, so it discloses nothing. This is the + // control that keeps the assertions above from being "the inspector fires + // for everything". + let same_tier = inspect(send_prompt_request("req-5", &private_target.id, PAYLOAD)).await; + assert!( + same_tier.is_empty(), + "a private→private write raised a crossing disclosure: {same_tier:?}" + ); +} + +/// **A first crossing cannot be made from inside an `execute_code` script.** +/// +/// The disclosure is a `ToolInspector`, and the JS sandbox hands a script's +/// inner tool calls straight to `ExtensionManager::dispatch_tool_call` — a door +/// no inspector sees. That is why `code_execution_extension` already carries +/// boundary refusals for the global memory store and the session database; this +/// is the third. +/// +/// The stakes are higher than one skipped card: the handler records the pair as +/// crossed afterwards, so a single un-inspected script call would silence the +/// disclosure for that pair permanently. +#[tokio::test] +#[serial_test::serial(workspace_crossing)] +async fn a_first_crossing_is_refused_at_the_uninspected_script_boundary() { + use biorouter::agents::workspace_inspector::uninspected_crossing_refusal; + use biorouter::privacy::CallCapability; + use biorouter::security::UninspectedBoundary; + + let root = store_root(); + let sm = SessionManager::instance(); + let public_target = sm + .create_session( + root.path().to_path_buf(), + "script target".into(), + SessionType::User, + ) + .await + .unwrap(); + let private_target = sm + .create_session( + root.path().to_path_buf(), + "script peer".into(), + SessionType::User, + ) + .await + .unwrap(); + sm.update(&private_target.id) + .raise_privacy(SessionClassification::Private, "test:script-boundary") + .apply() + .await + .unwrap(); + + let obj = |v: serde_json::Value| v.as_object().unwrap().clone(); + let send = |target: &str| { + obj(serde_json::json!({ + "session_id": target, "mode": "turn", "text": "the cohort, verbatim" + })) + }; + // Built through the PUBLIC constructor a production caller uses. There is a + // test-only one, but it is `#[cfg(test)]` and so invisible from an + // integration binary — which is the point of the census in + // `tests/privacy_capability.rs`: a capability comes from `sample` or from + // `public_enforced`, and nowhere else. + let institutional: biorouter::agents::types::SharedProvider = + Arc::new(tokio::sync::Mutex::new(Some(Arc::new(InstitutionalModel)))); + let private_caller = CallCapability::sample(&institutional).await; + assert!( + private_caller.tier().is_private() && private_caller.enforced(), + "the fixture caller is not a private, enforced capability, so nothing below \ + is testing the disclosure" + ); + + let refusal = uninspected_crossing_refusal( + private_caller, + "script-caller", + "workspace_send_prompt", + Some(&send(&public_target.id)), + UninspectedBoundary::ExecuteCodeScript, + ) + .await + .expect("a first crossing from a script must be refused, not silently recorded"); + assert!(refusal.contains(&public_target.id), "{refusal}"); + // The refusal has to say what to do instead, or the model retries the same + // call until it gives up. + assert!(refusal.contains("directly"), "{refusal}"); + + // Narrow by construction — each of these must pass straight through. + // 1. Same tier: nothing is crossing. + assert!(uninspected_crossing_refusal( + private_caller, + "script-caller", + "workspace_send_prompt", + Some(&send(&private_target.id)), + UninspectedBoundary::ExecuteCodeScript, + ) + .await + .is_none()); + // 2. A public caller: it cannot reach a private target at all, and a + // public→public write crosses nothing. + assert!(uninspected_crossing_refusal( + CallCapability::public_enforced(), + "script-caller", + "workspace_send_prompt", + Some(&send(&public_target.id)), + UninspectedBoundary::ExecuteCodeScript, + ) + .await + .is_none()); + // 3. A tool with no payload. + assert!(uninspected_crossing_refusal( + private_caller, + "script-caller", + "workspace_close", + Some(&obj(serde_json::json!({ + "session_id": public_target.id, "scope": "turn" + }))), + UninspectedBoundary::ExecuteCodeScript, + ) + .await + .is_none()); + // 4. A pair the user has already approved. This is the one that keeps the + // refusal from becoming "scripts may never touch the workspace". + biorouter::privacy::crossing::record("script-caller", &public_target.id); + assert!( + uninspected_crossing_refusal( + private_caller, + "script-caller", + "workspace_send_prompt", + Some(&send(&public_target.id)), + UninspectedBoundary::ExecuteCodeScript, + ) + .await + .is_none(), + "a pair the user already approved must not be refused for ever after" + ); +} diff --git a/docs/agent-loop/tool-routing.md b/docs/agent-loop/tool-routing.md index b71388c77..9abddcbb6 100644 --- a/docs/agent-loop/tool-routing.md +++ b/docs/agent-loop/tool-routing.md @@ -163,13 +163,14 @@ Two consequences for routing, until that lands: One write is covered, and it is worth knowing which: `workspace_set_tools { provider, model }` calls the same `Agent::update_provider` the model picker does, so **Gate A** applies to it and steering -another chat onto a public model cannot launder a private one. Design §7's *other* write rules — -the lineage conditions on `workspace_send_prompt` and on the rest of `workspace_set_tools` — are -unwired along with the read side. +another chat onto a public model cannot launder a private one. Design §7's write row is `may_write`, and it +is `VIS` — the same predicate as READ. The **lineage conditions** that used to sit on +`workspace_send_prompt` and on the rest of `workspace_set_tools` are **retired**: an agent may +inject into any conversation it can see, and only the privacy tier refuses. The same guidance is mirrored in the extension's own `INSTRUCTIONS` block (`crates/biorouter/src/agents/workspace_extension.rs`), which a unit test holds to -≤2,500 characters and to naming only tools `get_tools()` actually registers. +≤2,800 characters and to naming only tools `get_tools()` actually registers. ## Overlap matrix diff --git a/docs/agent-loop/workspace-control-tools.md b/docs/agent-loop/workspace-control-tools.md index 31fcce25b..871f34f07 100644 --- a/docs/agent-loop/workspace-control-tools.md +++ b/docs/agent-loop/workspace-control-tools.md @@ -187,6 +187,12 @@ When the projection exceeds `max_chars` it is cut and the reply appends `… [cl Writes into another conversation. Three modes with three different blast radii; every injection is provenance-stamped `MessageProvenance { kind: AgentInjection, from_session_id, from_session_name }` and the label is stored, so it survives reload. +**Any conversation, not only a child.** The target may be a subagent this conversation spawned, a sibling, or a chat the user opened and this agent has never touched. Lineage is not a boundary — the write rule is `may_write` ⇔ `may_read`, so the reachable set is exactly the set this conversation may already read in full. What *is* a boundary is the privacy tier: a public-capability chat is refused a private conversation under every verb, and a private-capability chat writing into a public one raises a **first-crossing approval showing the exact payload**, once per (caller, target) pair, in every permission mode including Fully Automatic. + +The tool's own description tells the model to use it **only when necessary**, and the reason is not politeness: the target may be a conversation a person is reading, and an injection interrupts them. + +**All three modes reflect in an open tab live**, without a reload. `note` publishes the stored row onto the session bus itself; `steer` is published by the target's own turn loop when it drains the soft interrupt; `turn`'s injected prompt is published by `Agent::reply` at the point the row becomes durable — which is a case its `MessagesPersisted`-only rule deliberately does not cover, since that rule assumes the client authored the prompt and already holds it. In every case the publish happens **after** the row is durable and carries the row's own minted uid. + ### Arguments | Name | Type | Default | Meaning | @@ -223,7 +229,7 @@ Returns, by path: - `wait: "final_message"`, the turn errored — an **error** result: `turn {turn_id} ended in error: {e}`. - `wait: "final_message"`, timed out — a **success** result: `Turn {turn_id} is still running after {n}s; it continues in the background. Read it later with workspace_read_conversation.` A timeout is not a failure. -Both `steer` and `turn` post a toast on the target's tab naming the calling conversation. +Both `steer` and `turn` post a toast on the target's tab naming the calling conversation. The toast is a notice, not the message — the message itself arrives in the transcript through the session bus (see above). ### When to reach for it diff --git a/docs/agent-loop/workspace-control.md b/docs/agent-loop/workspace-control.md index bd5949bc7..186871135 100644 --- a/docs/agent-loop/workspace-control.md +++ b/docs/agent-loop/workspace-control.md @@ -22,7 +22,9 @@ Everything below is the mechanics behind those three. ## Turning it on -Workspace control ships in two sizes. The small one is automatic: any session allowed to delegate gets the spawn tool (`subagent`) and nothing else. The full surface — reading other conversations, injecting prompts into them, changing their tool sets — is an explicit opt-in, because those tools reach into conversations the agent did not create. +Workspace control ships in two sizes. The small one is automatic: any session allowed to delegate gets `subagent`, the child-scoped supervision tools (`workspace_read_conversation`, `workspace_close`, `workspace_watch`), and the two that make cross-chat injection usable — `workspace_list` to see which conversations exist and which are running, and `workspace_send_prompt` to write into one. The full surface — changing another conversation's tool set with `workspace_set_tools`, opening and moving tabs with `workspace_open`, reading the preview panel — is an explicit opt-in. + +The line between the two is **message versus capability change**: the automatic tier can talk to another conversation, and only the opt-in can re-tool one. In the desktop app, open **Extensions** in the left sidebar and turn on **Workspace Control**. From the terminal, run `biorouter configure`, choose `Toggle Extensions`, and enable `workspace`. The extension is registered `default_enabled: false`; nothing enables it for you. @@ -35,7 +37,7 @@ Jobs 1 and 3 need that full surface. Job 2 does not — but "allowed to delegate All of the following must hold: - **The permission mode is Completely Autonomous** (`auto`). In Manual Approval, Smart Approval and Chat Only — three of the [four modes](../security/permission-modes.md) — there is no spawn tool at all. Autonomous is the default, so most people never meet this; anyone who has turned the mode down will, and the symptom is an ordinary answer where a child conversation was expected. -- **The session is not itself a subagent.** A child cannot spawn grandchildren, which is the same rule that stops a child being granted workspace control. +- **The session is not itself a subagent.** A child cannot spawn grandchildren, which is the same rule that stops a child being granted workspace control. This is a *lineage* rule and it survives; it is not the retired §7 write rule, which no longer looks at lineage at all. - **At least one ordinary extension is loaded.** The auto-injected `workspace` entry is deliberately not counted — otherwise one turn's grant would justify the next one's, and an agent that dropped its last real extension would keep delegating forever off a grant it derived from itself. - **The active model name does not begin with `gemini`.** This is a flat exclusion in the gate, with no rationale recorded in the source, so treat it as observed behaviour rather than a rule with a reason: on a Gemini model, delegation is off whatever your mode says. - **The session is not a BioRouter app that delegates through `consult`.** Apps with worker profiles route delegation through their own mechanism and have the generic tool withdrawn so the two cannot both be offered. diff --git a/docs/extensions/built-in/workspace.md b/docs/extensions/built-in/workspace.md index 0aa3f3ca5..0661cb32c 100644 --- a/docs/extensions/built-in/workspace.md +++ b/docs/extensions/built-in/workspace.md @@ -16,12 +16,12 @@ Workspace Control ships in **two sizes**, and most people only ever meet the sma | Tier | How you get it | What the agent can do | |------|----------------|-----------------------| -| **Delegation only** (default) | Automatic. Any session that may delegate loads the extension with a tool list of exactly `subagent`. | Spawn subagents. Nothing cross-session. | -| **Full workspace control** | You enable the `workspace` extension explicitly. | The seven `workspace_*` tools as well: read other conversations, inject prompts into them, change their tool sets. | +| **Delegation** (default) | Automatic. Any session that may delegate loads the extension with a fixed six-tool list: `subagent`, `workspace_list`, `workspace_read_conversation`, `workspace_send_prompt`, `workspace_close`, `workspace_watch`. | Spawn subagents and supervise them; see which conversations exist and which are running; inject a prompt into one. | +| **Full workspace control** | You enable the `workspace` extension explicitly. | Everything above plus `workspace_set_tools` (change another conversation's extensions, skills, model, knowledge bases), `workspace_open`, and the preview-panel pair. | -The split exists because the two tiers have very different blast radii. Delegation creates a *new* conversation whose contents the agent already owns. The cross-session tools reach into conversations the agent did **not** create — so they are an explicit, informed opt-in, and the capability summary you are agreeing to is the same one the design records: **read other conversations, inject prompts into them, and change their tool sets.** +The split is no longer "your own children versus everyone else's" — an injection may go to any conversation the session can see. What separates the tiers now is **capability change versus message**: the delegation tier can *talk to* another conversation, and only the explicit opt-in can *re-tool* one or mint and move tabs. Three of the delegation tier's six tools stay child-scoped whatever the write rule says — `workspace_read_conversation`, `workspace_close` and `workspace_watch` are confined to direct subagent children by `refuse_unless_direct_subagent_child`, which is a separate mechanism from the privacy matrix and did not move. -Concretely, the extension is registered `default_enabled: false` (like Chat Recall). When a session has any ordinary extension loaded and delegation is permitted by your [permission mode](../../security/permission-modes.md), BioRouter auto-injects `workspace` for the spawn tool alone; that injection is derived state and is dropped again if the reason for it goes away. Enabling `workspace` yourself is what unlocks the rest, and an explicit enable is never downgraded to the injected one. +Concretely, the extension is registered `default_enabled: false` (like Chat Recall). When a session has any ordinary extension loaded and delegation is permitted by your [permission mode](../../security/permission-modes.md), BioRouter auto-injects `workspace` with that six-tool list; the injection is derived state and is dropped again if the reason for it goes away. Enabling `workspace` yourself is what unlocks the rest, and an explicit enable is never downgraded to the injected one. ### Turning on the full surface @@ -59,7 +59,9 @@ Hidden sessions are refused. Reads are recorded as tool calls in the *reading* c ### `workspace_send_prompt` -Injects text into another conversation. `mode: "turn"` starts its agent on your text (it must be idle), `mode: "steer"` redirects it mid-turn (it must be running), `mode: "note"` leaves context without running anything. +Injects text into **any** conversation the agent can see — a subagent it spawned, or a chat you opened that it has never touched. `mode: "turn"` starts its agent on the text (it must be idle), `mode: "steer"` redirects it mid-turn (it must be running), `mode: "note"` leaves context without running anything. The message appears in that conversation's tab **as it is sent**, with no reload. + +The one boundary is privacy: a chat on a public model cannot inject into a conversation marked private, and a chat on your institution's own model injecting into a public one asks you **the first time**, showing the exact text it would send. Read "the first time" literally — the approval is remembered per pair of conversations, not per message, so once you have approved one write from chat A into chat B, later writes on that pair go through without asking. That is the deliberate trade (a card on every message is a card nobody reads), and it is the thing to know before approving one: you are agreeing to the channel, not just to the text in front of you. The tool's instructions also tell the agent to use it only when it genuinely needs to — you may be reading the conversation it interrupts. > "Tell the QC chat to stop at step 3 and summarise." → `workspace_send_prompt { session_id: "…", text: "Stop at step 3 and summarize.", mode: "steer" }` diff --git a/docs/security/privacy-tiers-execution-plan.md b/docs/security/privacy-tiers-execution-plan.md index b0622eb21..827830c01 100644 --- a/docs/security/privacy-tiers-execution-plan.md +++ b/docs/security/privacy-tiers-execution-plan.md @@ -16747,6 +16747,14 @@ unit-testable without a database and BR-71's tool handlers can call it rather th | Create | `crates/biorouter/src/privacy/visibility.rs` | new | | Reference | `crates/biorouter/src/session/session_manager.rs` | `Session.parent_session_id` (Task 6) | +> ⚠ **The Rust in the rest of this task is a HISTORICAL ARTIFACT and no longer +> describes the tree.** It reproduces `visibility.rs` as Task 21 shipped it, when +> the write rule was `WRITE ⇔ VIS ∧ L ∈ {self, child}`. R6 is retired: `may_write` +> takes no lineage, `Lineage` and `lineage_of` are deleted, and the matrix's +> `( CPub, TPub, Other, true, false, true )` row now expects `write: true`. Read +> `crates/biorouter/src/privacy/visibility.rs`, not the blocks below; they are +> kept only so the shape of the original task stays legible. + - [ ] **Step 1: Write the failing test — the design's table, cell for cell** ```rust @@ -22155,7 +22163,7 @@ the implementation is wrong. | **DR-2** | **Two lattices, opposite directions.** CAPABILITY (what a session may DO) = the **least** privileged model bound to it, so a mixed lead/worker config gets public reach. CLASSIFICATION (how sensitive its CONTENTS are) = the **most** sensitive thing it has touched, a permanent ratchet. A session can be classified private while holding only public capability. | | **DR-3** | **A public model must never reach a private session.** Not once, not read-only, not indirectly. The converse is unrestricted: a private model may read anything. | | **DR-4** | **The ratchet fires on the first TURN and on a permitted private-extension dispatch — never on the bind.** Binding is not when content appears, and ratcheting there would privatise a chat on a mis-click while still missing `POST /agent/call_tool`, which dispatches straight into the extension manager without touching the reply path. | -| **DR-5** | **Lineage decides write access.** Sessions the caller spawned get full control; everything else is read-only. Lineage is **one hop** — a grandchild is `other`. | +| **DR-5** | ~~**Lineage decides write access.** Sessions the caller spawned get full control; everything else is read-only. Lineage is **one hop** — a grandchild is `other`.~~ **RETIRED** by the product owner: an agent may inject a prompt into *any* conversation, child or unrelated, provided it does not cross the private/public boundary. WRITE ⇔ VIS. | | **DR-6** | **The BAAM registry is the sole grantor of a private badge, and anything not on BAAM is PUBLIC** (fail-open, by decision). The private set is exactly **`ucsfomopagent`** and **`cdwagent`**. Built-ins, platform servers and in-process app servers are public. Skills carry no classification. | | **DR-7** | **`chatrecall` obeys the barrier** — private models recall from private and public, public models from public only. **Side channels (existence, counts, timing) are explicitly out of scope**: no count padding, no constant-time responses, no decoys. Only content must not cross. | | **DR-8** | **Declassification is the user's alone** — an explicit deprivatise action in History. Nothing automatic, nothing an agent can invoke. Graded by `privacy_reason`: `mcp:*` gets a typed confirmation, `turn:*`-only gets single-click with undo. | @@ -22903,24 +22911,32 @@ workflow — but two implementations of one capability is what produced the hole workspace tool"* — true, and actively misleading, because four of the six doors to a transcript are not workspace tools. -### D2 — the private→public write is refused, like the spawn already is +### D2 — the private→public write ~~is refused, like the spawn already is~~ discloses its payload -The same act gets opposite rulings depending on the tool: `subagent` with a public override is -**refused** (`spawn_downgrade`), while `workspace_send_prompt` into a public chat and -`workspace_open {new:{prompt}}` are **permitted and silent** — the latter minting a permanently -public row holding private-origin text. +⚠ **SUPERSEDED.** This audit ruling — *"refuse the downgrade write"* — was overruled by the product +owner, who requires that an agent be able to inject into any conversation it can see, the tier being +the only boundary. Refusing a private→public write would forbid exactly the private-leader / +public-worker arrangement R2 names, and R4 already permits a private session to spawn public +children. **The disclosure was built instead of the refusal**: the first `workspace_send_prompt` / +`workspace_set_tools` from a given caller into a given public target raises an approval showing the +exact payload (`agents/workspace_inspector.rs`, `privacy/crossing.rs`). -**Ruled: refuse the downgrade write.** One branch, and it removes the contradiction rather than -documenting it. +The audit's underlying observation stands and is worth keeping: the same act had opposite rulings +depending on the tool — `subagent` with a public override is **refused** (`spawn_downgrade`), while +`workspace_send_prompt` into a public chat and `workspace_open {new:{prompt}}` were **permitted and +silent**. What changed is which way the inconsistency was resolved: the silent half became loud +rather than the permitted half becoming refused. ⚠ **Amend `spawn_downgrade`'s advice text in the same change.** It currently reads *"start a new chat on it and give it the task directly"* — which points at exactly the path being closed. This is the audit's sharpest finding: **the refusals were routing cooperative agents into the ungated paths.** A model that reads its refusals carefully was *more* likely to find the bypass than one that gave up. -Deleting `requires_first_crossing_approval`, `may_write` and `lineage_of` is in scope here — refusing -makes all three unnecessary, and shipping a fifth correct-but-uncalled guard is worse than shipping -none. +~~Deleting `requires_first_crossing_approval`, `may_write` and `lineage_of` is in scope here~~ — that +followed from the refusal and does not follow from the disclosure. Of the three, only `lineage_of` +was deleted (with `Lineage`, and because R6 retired, not because of this ruling); `may_write` stayed +and lost its lineage clause; and `requires_first_crossing_approval` is now WIRED, which is the +opposite of the "fifth correct-but-uncalled guard" this paragraph was worried about. ### D3 — private payloads stay out of the logs, and diagnostics stop being cross-session @@ -22967,15 +22983,27 @@ required for this release. message bodies. ⚠ Enumerate the doors — this bug existed because one door was found and four were not. -### Task 63: Refuse the private→public write (D2) - -- [ ] `workspace_send_prompt` and `workspace_open {new:{prompt}}` refuse a private→public downgrade, - reusing `spawn_downgrade`'s predicate and shape. -- [ ] Amend `spawn_downgrade`'s advice text. -- [ ] Delete `requires_first_crossing_approval`, `may_write`, `lineage_of` — refusing makes them dead. -- [ ] **Gate:** the same downgrade is refused through spawn, send-prompt and open, asserted per path. - Plus: no privacy refusal in the tree advises an action another gate forbids — this is the - finding, made mechanical. +### Task 63: ~~Refuse~~ **Disclose** the private→public write (D2) + +⚠ **Rewritten after the owner overruled D2's refusal.** The original checklist is kept struck +through, because a later reader finding only the new one would not know a refusal was ever +considered. + +- [x] `workspace_send_prompt` and `workspace_set_tools` raise a first-crossing approval showing the + exact payload on a private→public write, once per (caller, target) pair, in every permission + mode — `WorkspaceCrossingInspector` + `privacy::crossing`. +- [ ] ~~`workspace_send_prompt` and `workspace_open {new:{prompt}}` refuse a private→public + downgrade, reusing `spawn_downgrade`'s predicate and shape.~~ +- [ ] Amend `spawn_downgrade`'s advice text. **Still open, and now more so:** it advises *"start a + new chat on it and give it the task directly"*, which is a path the widened write rule makes + easier rather than harder. `workspace_open {new:{prompt}}` is still ungated on the model + dimension (§7 open item 2). +- [ ] ~~Delete `requires_first_crossing_approval`, `may_write`, `lineage_of`~~ — see above; the + disclosure needs the first of these and the write gate needs the second. +- [x] **Gate:** the disclosure is raised per path and cannot be bought by a denial (asking never + records the crossing). +- [ ] **Gate:** no privacy refusal in the tree advises an action another gate forbids — this is the + finding, made mechanical. Still open. ### Task 64: Keep private payloads out of logs and bug reports (D3) diff --git a/docs/security/privacy-tiers-implementation-brief.md b/docs/security/privacy-tiers-implementation-brief.md index ba1407556..b769e849f 100644 --- a/docs/security/privacy-tiers-implementation-brief.md +++ b/docs/security/privacy-tiers-implementation-brief.md @@ -261,7 +261,7 @@ read the full row before implementing against it. | **DR-2** | "**Two lattices, opposite directions.** CAPABILITY (what a session may DO) = the **least** privileged model bound to it… CLASSIFICATION (how sensitive its CONTENTS are) = the **most** sensitive thing it has touched, a permanent ratchet." | | **DR-3** | "**A public model must never reach a private session.** Not once, not read-only, not indirectly. The converse is unrestricted: a private model may read anything." | | **DR-4** | "**The ratchet fires on the first TURN and on a permitted private-extension dispatch — never on the bind.**" | -| **DR-5** | "**Lineage decides write access.** Sessions the caller spawned get full control; everything else is read-only. Lineage is **one hop** — a grandchild is `other`." | +| **DR-5** | ~~"**Lineage decides write access.** Sessions the caller spawned get full control; everything else is read-only. Lineage is **one hop** — a grandchild is `other`."~~ **RETIRED.** Superseded by the ruling that an agent may inject a prompt into any conversation, child or unrelated, provided it does not cross the private/public boundary. WRITE ⇔ VIS. | | **DR-6** | "**The BAAM registry is the sole grantor of a private badge, and anything not on BAAM is PUBLIC** (fail-open, by decision). The private set is exactly **`ucsfomopagent`** and **`cdwagent`**." | | **DR-7** | "**`chatrecall` obeys the barrier**… **Side channels (existence, counts, timing) are explicitly out of scope**: no count padding, no constant-time responses, no decoys. Only content must not cross." | | **DR-8** | "**Declassification is the user's alone** — an explicit deprivatise action in History. Nothing automatic, nothing an agent can invoke." | diff --git a/docs/security/privacy-tiers.md b/docs/security/privacy-tiers.md index ffbbfc898..164740953 100644 --- a/docs/security/privacy-tiers.md +++ b/docs/security/privacy-tiers.md @@ -120,13 +120,13 @@ this section is the ledger. plan Tasks 14A–14F are `DEFERRED`, not deleted. A public-capability chat with a shell can still read ordinary files on this machine, including files an earlier private chat wrote outside Biorouter's own storage. This is disclosed to the user rather than mechanised. -- **§7's cross-session capability matrix: READ is wired on all seven workspace tools, WRITE is - wired on none.** ⚠ **This entry was false, and was rewritten against the tree on 2026-08-06.** - It used to claim that `crates/biorouter/src/privacy/visibility.rs` had **no production caller** - and that `workspace_list` and `workspace_read_conversation` *"do not consult `privacy_tier`"* — - and then to name those two tools as the whole of the exposure. Every part of that was wrong by - the time it was read: the predicates have callers, and the still-open set is neither those two - tools nor a set of that size. A security document that understates its coverage costs as much +- **§7's cross-session capability matrix: READ and WRITE are both wired now.** ⚠ This entry has + been wrong twice, and both times in the direction of understating coverage. It first claimed + that `crates/biorouter/src/privacy/visibility.rs` had **no production caller** and that + `workspace_list` and `workspace_read_conversation` *"do not consult `privacy_tier`"* — rewritten + against the tree on 2026-08-06, because every part of it was already false. It then claimed + WRITE was "wired on none", which was false too: `refuse_unless_writable` has composed the read + gate with `may_write` since. A security document that understates its coverage costs as much trust as one that overstates it, so what follows is the state of the tree, symbol by symbol. ⚠ **Re-verify before you rely on this.** This branch is under concurrent repair; between the @@ -177,17 +177,26 @@ this section is the ledger. **Still open:** - 1. **§7's WRITE row is implemented nowhere, so every wired write enforces VIS only.** - `visibility::may_write`, `lineage_of`, `Lineage` and `requires_first_crossing_approval` have - **zero** production callers — a tree-wide search for each returns `visibility.rs` itself and - two doc comments. So R6's lineage floor (columns B, E and G, `✗ R6`) is unenforced: a public - caller may steer, re-tool and close a **public sibling it did not spawn**. And column D's - `✓!` first-crossing approval — the disclosure that is the entire reason a private→public - downgrade write is *permitted* rather than refused — never fires. `workspace_send_prompt` and - `workspace_set_tools` both say so in their own source: *"⚠ This enforces VIS only. §7's write - row is `may_write` … and the lineage half is not implemented anywhere"*. - 2. **`workspace_open { new: … }` implements none of §8.2's spawn matrix.** §7's last row defers - that form to §8.2. The extension dimension is now gated (above), but the **model** dimension + 1. ~~**§7's WRITE row is implemented nowhere, so every wired write enforces VIS only.**~~ + **CLOSED, and in both directions at once.** Half of it was closed by ruling rather than by + code: R6's lineage floor is retired, so "a public caller may steer a public sibling it did + not spawn" is the intended behaviour and `Lineage`/`lineage_of` are deleted. The other half + was a real gap and is now built: column D's `✓!` first-crossing approval fires, through + `agents/workspace_inspector.rs`'s `WorkspaceCrossingInspector` (which asks) and + `privacy/crossing.rs` (which is the per-(caller, target) state the predicate always needed + and never had). Widening the write rule is what settled the operator decision that had been + outstanding against it: the public targets a private caller can reach are no longer only the + ones it spawned itself, so the disclosure went from nice-to-have to load-bearing. + 2. **`workspace_open { new: … }` implements none of §8.2's spawn matrix, and `new.prompt` + is now the sharpest edge of it.** A new session is minted PUBLIC (the schema default), so + a private-capability caller passing `new: { prompt: … }` puts private-origin text in front + of a public model with no first-crossing approval — the disclosure covers + `workspace_send_prompt` and `workspace_set_tools`, both of which name an existing + `session_id`, and a session that does not exist yet has no (caller, target) pair to key on. + `workspace_open` is not in the delegation tier's injected tool list, so this needs the + explicit Workspace Control opt-in. + + The older half of this item stands unchanged. §7's last row defers that form to §8.2. The extension dimension is now gated (above), but the **model** dimension is not: `open_new_session` creates the session through `WorkspaceServices::start_session`, which takes no capability and binds the machine default provider, and then optionally seeds it with a detached turn carrying prompt text the model wrote. §8.2's hard refusal (public parent, @@ -375,7 +384,7 @@ Each verified by reading the code, each fixed as a by-product of this design: | R3 | Classification is a permanent ratchet. | | R4 | A private session may spawn public children; a public session may never gain private reach. | | R5 | Children inherit the parent's model and lead/worker mode unless the user says otherwise. | -| R6 | Lineage decides write access: sessions the caller spawned get full control, everything else is read-only. | +| R6 | ~~Lineage decides write access: sessions the caller spawned get full control, everything else is read-only.~~ **RETIRED.** Superseded by the product owner's ruling that an agent may inject a prompt into *any* conversation — child or unrelated — provided it does not cross the private/public boundary. WRITE ⇔ VIS; the tier is the only boundary (§7). | | R7 | A global opt-out exists, off by default. It is a **master** switch: with it off there is no gate and no ratchet anywhere (§10.6). ⚠ It does **not** switch off R15's disclosure — with enforcement off the exposure is larger, not smaller. | | R8 | A public model must never reach a private session. | | R9 | Only the user can deprivatise a session, from history settings, with a warning. Nothing automatic, nothing agent-invocable. | @@ -857,21 +866,28 @@ deliberately no builder setter that accepts `Public`. - **C** = `capability(caller)` = `least` over the components of the caller's currently-bound provider. `Pub` | `Priv`. - **T** = `target.privacy_tier`, the stored classification. `Pub` | `Priv`. -- **L** = lineage of target relative to caller: `self` · `child` - (`target.parent_session_id == caller_session_id`) · `other` (includes NULL parent and every - transitive descendant). - -**Lineage is one hop.** A grandchild is `other`: R6 says "sessions the caller *did* spawn", and a -grandchild was spawned by the child. BR-71's `workspace_list { parent_session_id: "" }` filter -already yields exactly the one-hop set, so no recursive CTE and no new "control my subtree" surface -is invented. A leader that needs deeper control asks its child. +**Lineage was a third input and is not one any more.** The matrix used to take +`L ∈ {self, child, other}` and make WRITE depend on it, so an agent could steer a conversation it +had spawned and only *read* one it had not (R6). The product owner has since ruled the opposite: +an agent may inject a prompt into **any** conversation — a subagent child or an unrelated chat — +as long as it does not cross the private/public boundary. **The privacy tier is the only boundary.** +R6 is retired, and `Lineage`/`lineage_of` are deleted rather than left as an unread argument. + +Two things make that retirement smaller than it sounds, and both are load-bearing: + +- READ and WRITE now coincide, so **no verb reaches a conversation the caller could not already + read in full**. Widening WRITE removes a read-only cell; it does not add a target. +- The two *other* one-hop rules in this product are untouched, and neither is `may_write`. A + delegation-scoped grant (`McpMeta::workspace_child_scope_only`) still confines an auto-injected + supervision surface's read/close/watch to direct children, and a `SessionType::SubAgent` is + still refused every `workspace_*` tool outright (§8.2). **The three rules.** ``` VIS(T) ⇔ T ≤ C // a public caller sees public only -READ ⇔ VIS // any lineage — R6's read-only floor -WRITE ⇔ VIS ∧ L ∈ {self, child} +READ ⇔ VIS +WRITE ⇔ VIS BIND(P→T) ⇔ WRITE ∧ tier(P) ≥ T // Gate A, evaluated on the target ``` @@ -883,21 +899,22 @@ crossing into a public model, so the first `workspace_send_prompt` / `workspace_ given caller into a given public target raises an approval showing the exact payload. **The matrix.** `✓` allowed · `✓!` allowed, first crossing requires approval showing the payload · -`✗` refused with a teaching message · `∅` omitted from results entirely. `self` and `child` behave -identically under every rule and are merged; columns D and F prove it. - -| BR-71 tool | Class | **A**
C=Pub T=Pub
self/child | **B**
C=Pub T=Pub
other | **C**
C=Pub T=**Priv**
any L | **D**
C=Priv T=Pub
self/child | **E**
C=Priv T=Pub
other | **F**
C=Priv T=Priv
self/child | **G**
C=Priv T=Priv
other | -|---|---|---|---|---|---|---|---|---| -| `workspace_list` | read | ✓ | ✓ | **∅ row omitted** | ✓ | ✓ | ✓ | ✓ | -| `workspace_read_conversation` | read | ✓ | ✓ | ✗ | ✓ | ✓ | ✓ | ✓ | -| `workspace_watch` | read | ✓ | ✓ | ✗ | ✓ | ✓ | ✓ | ✓ | -| `workspace_open` *(existing session)* | read | ✓ | ✓ | ✗ | ✓ | ✓ | ✓ | ✓ | -| `workspace_send_prompt` *(turn / steer / note)* | write | ✓ | ✗ R6 | ✗ | **✓!** | ✗ R6 | ✓ | ✗ R6 | -| `workspace_set_tools` — extensions / skills / KBs | write | ✓ | ✗ R6 | ✗ | **✓!** | ✗ R6 | ✓ | ✗ R6 | -| `workspace_set_tools` — `add_extensions` naming a **private** extension | write | ✗ target is public-capability | ✗ | ✗ | ✗ | ✗ | ✓ | ✗ R6 | -| `workspace_set_tools` — `{ provider, model }` | bind | ✓ if `tier(P) ≥ Pub` (always) | ✗ R6 | ✗ | **✓!** if `tier(P) ≥ Pub` | ✗ R6 | ✓ **only if `tier(P)=Priv`** | ✗ R6 | -| `workspace_close` | write | ✓ | ✗ R6 | ✗ | ✓ | ✗ R6 | ✓ | ✗ R6 | -| `workspace_spawn_subagent` / `workspace_open { new: … }` | spawn | see §8.2 | | | | | | | +`✗` refused with a teaching message · `∅` omitted from results entirely. Nine columns became four: +with lineage gone, A and B collapse, as do D and E, and F and G — and the collapse is exactly where +the behaviour changed, because the `other` half of each pair used to read `✗ R6` on every write row. + +| BR-71 tool | Class | **A**
C=Pub T=Pub | **C**
C=Pub T=**Priv** | **D**
C=Priv T=Pub | **F**
C=Priv T=Priv | +|---|---|---|---|---|---| +| `workspace_list` | read | ✓ | **∅ row omitted** | ✓ | ✓ | +| `workspace_read_conversation` | read | ✓ | ✗ | ✓ | ✓ | +| `workspace_watch` | read | ✓ | ✗ | ✓ | ✓ | +| `workspace_open` *(existing session)* | read | ✓ | ✗ | ✓ | ✓ | +| `workspace_send_prompt` *(turn / steer / note)* | write | ✓ | ✗ | **✓!** | ✓ | +| `workspace_set_tools` — extensions / skills / KBs | write | ✓ | ✗ | **✓!** | ✓ | +| `workspace_set_tools` — `add_extensions` naming a **private** extension | write | ✗ target is public-capability | ✗ | ✗ | ✓ | +| `workspace_set_tools` — `{ provider, model }` | bind | ✓ if `tier(P) ≥ Pub` (always) | ✗ | **✓!** if `tier(P) ≥ Pub` | ✓ **only if `tier(P)=Priv`** | +| `workspace_close` | write | ✓ | ✗ | ✓ | ✓ | +| `workspace_spawn_subagent` / `workspace_open { new: … }` | spawn | see §8.2 | | | | **`workspace_list` omits private rows rather than redacting them.** The operator ruled existence leaks *acceptable*, not *required*, and omission is strictly simpler: a `workspace_list` row @@ -1464,7 +1481,7 @@ is the correct placement call); a stale provider inside the extension manager (t `Arc` is the same one `update_provider` writes through); a mixed composite calling a private MCP server (`least = Public` → refused); a public parent spawning a private child then reading its output; a private parent's public child reading back up (VIS is evaluated on the child's -capability); `workspace_read_conversation` on an ancestor (lineage widens *write*, never *read*); +capability); `workspace_read_conversation` on an ancestor (which was never about lineage, and is even less so now that lineage decides nothing); `chatrecall` SEARCH after Gate D; a stale registry copy downgrading an extension (the union rule holds). @@ -2709,7 +2726,7 @@ See §15.5 and §16 for what the backfill actually does to a real machine on day | Unknown provider | Public | fail-**safe**, not fail-open: Public is the *less* privileged tier | | Unlisted extension | Public | fail-open, **operator ruling R11(ii)**, isolated to the final `ProviderTier::Public` of one function — `classify_extension_entry`, which `classify_extension(name)` now delegates to — with one const and one comment naming the ruling, so reversing it later is a one-line change rather than an audit | | Any gate's lookup fails | refuse | encoded as a refusal inside `Ok(...)`, never as `Err` | -| NULL `parent_session_id` | `other` ⇒ read-only | safe for R6 | +| ~~NULL `parent_session_id`~~ | ~~`other` ⇒ read-only~~ | **moot.** R6 is retired and lineage is no longer an input to any rule, so a NULL parent decides nothing. The column itself remains, for the interface's parent/child grouping. | ### 15.4 Sessions, configs and extensions @@ -2979,9 +2996,10 @@ public caller's list **omits** private rows. Task 4 already amends this projecti **Task 15 (`workspace_set_tools`).** Three constraints, all resolved off lookups the task already performs: `{ provider, model }` **must call `Agent::update_provider`** rather than reimplement the persist; `add_extensions` naming a private extension gains the tier refusal beside the issue-#42 -operator-disabled gate the plan already wires in at `get_extension_entry_by_name`; and lineage gates -the whole tool to `self`/`child`, with a private→public invocation raising the first-crossing -approval. +operator-disabled gate the plan already wires in at `get_extension_entry_by_name`; and a +private→public invocation raises the first-crossing approval. ⚠ This paragraph also said *"lineage +gates the whole tool to `self`/`child`"* — that clause is retired with R6; the tool is gated by the +tier alone. **Tasks 17 / 19 / 13 / 14 / 16 / 24 (the tool surface).** The matrix in §7 covers `workspace_list` (12), `workspace_read_conversation` (13), `workspace_send_prompt` (14), `workspace_set_tools` (15), @@ -2995,7 +3013,9 @@ subagent the whole stretch runs in a detached `tokio::spawn`, so a daemon kill l `SubAgent` row with no provider and no parent. One INSERT closes both windows. **Task 36 (the subagent guard).** The existing refusal (a `SessionType::SubAgent` session may not -call the subagent tool) is the shape and the location; the lineage and tier checks belong beside it. +call the subagent tool) is the shape and the location; the tier check belongs beside it. (This read +"the lineage and tier checks" until R6 was retired. The `SubAgent` refusal itself is a *different* +one-hop rule and stays.) **Tasks 22-28 (GUI).** Tab-bar dots, workspace-row badges, provenance chips, set-tools toasts. diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index 4a5570d55..7da43c536 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -11805,7 +11805,7 @@ }, "parent_session_id": { "type": "string", - "description": "Id of the parent session that spawned this one as a subagent (BR-71).\nSibling of `diverged_from` (branch lineage): `diverged_from` records a\nuser fork; this records a delegation. `None` for non-subagent sessions.\nIt is also what the §7 capability matrix's `L` axis reads (issue #56).", + "description": "Id of the parent session that spawned this one as a subagent (BR-71).\nSibling of `diverged_from` (branch lineage): `diverged_from` records a\nuser fork; this records a delegation. `None` for non-subagent sessions.\nIt is what the interface groups tabs by, and what\n`refuse_unless_direct_subagent_child` reads to keep a delegation-scoped\ngrant pointed at its own children. It is NOT read by the §7 capability\nmatrix any more: that matrix had an `L` axis until lineage stopped being\na boundary for writes (issue #56).", "nullable": true }, "privacy_reason": { diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index 2e9feb9b7..ec796cefe 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -3113,7 +3113,11 @@ export type Session = { * Id of the parent session that spawned this one as a subagent (BR-71). * Sibling of `diverged_from` (branch lineage): `diverged_from` records a * user fork; this records a delegation. `None` for non-subagent sessions. - * It is also what the §7 capability matrix's `L` axis reads (issue #56). + * It is what the interface groups tabs by, and what + * `refuse_unless_direct_subagent_child` reads to keep a delegation-scoped + * grant pointed at its own children. It is NOT read by the §7 capability + * matrix any more: that matrix had an `L` axis until lineage stopped being + * a boundary for writes (issue #56). */ parent_session_id?: string | null; /** diff --git a/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.test.ts b/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.test.ts index 3cdd79590..5962068e1 100644 --- a/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.test.ts +++ b/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.test.ts @@ -23,6 +23,46 @@ function stateWithSessions(ids: string[]): ChatGroupsState { } describe('planWorkspaceCommand', () => { + // BR-71 §3c. `observe` is the frame the daemon sends after writing into a + // conversation from somewhere else. It asks the window holding that + // conversation to attach a live feed and does NOTHING else — no reducer + // action, no annotation, no focus — because the tab is already where the user + // put it. + it('observe plans no state change at all', () => { + const state = stateWithSessions(['s-mine']); + const plan = planWorkspaceCommand( + { type: 'workspace', cmd: 'observe', session_id: 's-mine' }, + state + ); + expect(plan.result.ok).toBe(true); + expect(plan.actions).toEqual([]); + expect(plan.annotate).toBeUndefined(); + expect(plan.notify).toBeUndefined(); + expect(plan.openWindowSessionId).toBeUndefined(); + }); + + it('observe for a session this window has no tab for succeeds, saying so', () => { + // An injection into a conversation nobody has open is the ORDINARY case, so + // it must not read as a failure the daemon should act on. `ok: false` here + // would put a refusal in the injecting agent's tool result for a write that + // worked perfectly. + const plan = planWorkspaceCommand( + { type: 'workspace', cmd: 'observe', session_id: 's-elsewhere' }, + stateWithSessions(['s-mine']) + ); + expect(plan.result.ok).toBe(true); + expect(plan.result.detail).toContain('no tab'); + expect(plan.actions).toEqual([]); + }); + + it('observe without a session_id is refused', () => { + const plan = planWorkspaceCommand( + { type: 'workspace', cmd: 'observe' }, + stateWithSessions(['s-mine']) + ); + expect(plan.result.ok).toBe(false); + }); + it('open_tab focus:false opens then restores the previously active tab', () => { const state = stateWithSessions(['s-mine']); const prevActive = activeTabOf(state)?.tabId; diff --git a/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.ts b/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.ts index 43fea7d3c..fe4382894 100644 --- a/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.ts +++ b/ui/desktop/src/components/chatGroups/workspaceCommandPlanner.ts @@ -131,6 +131,20 @@ export function planWorkspaceCommand( }, }; } + // BR-71 §3c: something was written into this session from elsewhere, so + // whatever tab is showing it needs a live feed. Purely a request to ATTACH + // — no reducer action, no annotation, no focus steal — because the tab is + // already where the user put it and the daemon has no business moving it. + // + // Refusing when the session has no tab is not a failure the caller should + // act on: an injection into a conversation nobody has open is the ordinary + // case. The detail says so rather than reading as an error. + case 'observe': { + if (!cmd.session_id) return refuse('missing session_id'); + const hit = findTabBySession(state, cmd.session_id); + if (!hit) return { result: { ok: true, detail: 'no tab in this window' }, actions: [] }; + return { result: { ok: true }, actions: [] }; + } default: return refuse(`unknown cmd '${(cmd as WorkspaceCommand).cmd}'`); } diff --git a/ui/desktop/src/components/chatGroups/workspaceCommandRegistry.ts b/ui/desktop/src/components/chatGroups/workspaceCommandRegistry.ts index e4a0d6640..bcd2ca37d 100644 --- a/ui/desktop/src/components/chatGroups/workspaceCommandRegistry.ts +++ b/ui/desktop/src/components/chatGroups/workspaceCommandRegistry.ts @@ -19,6 +19,7 @@ export type WorkspaceCommand = { | 'open_window' | 'notify' | 'annotate_tab' + | 'observe' | 'read_panel' | 'capture_panel'; session_id?: string; diff --git a/ui/desktop/src/contexts/ChatGroupsContext.tsx b/ui/desktop/src/contexts/ChatGroupsContext.tsx index ede69a626..53571509e 100644 --- a/ui/desktop/src/contexts/ChatGroupsContext.tsx +++ b/ui/desktop/src/contexts/ChatGroupsContext.tsx @@ -371,7 +371,22 @@ export function ChatGroupsProvider({ children }: { children: React.ReactNode }) const opened = cmd.cmd === 'open_tab'; const annotated = cmd.cmd === 'annotate_tab' && !!findTabBySession(stateRef.current, cmd.session_id); - if (opened || annotated) { + // BR-71 §3c. A tab the USER opened has no observer — nothing in this + // renderer attaches one, because the tab is normally driven by its own + // `/reply` stream and an idle tab has nothing to listen to. That stops + // being true the moment another conversation can write into this one: + // `workspace_send_prompt` makes the session change while this window is + // looking straight at it, and with no observer the transcript sits + // stale until a reload. + // + // The daemon sends this frame after the row is durable, and the + // observer's FIRST frame is a full `UpdateConversation` snapshot from + // the store — so the injected message renders whether or not the bus + // publish beat the attach. That is deliberate: an ordering guarantee + // between a broadcast and a socket handshake is not one worth relying on. + const observed = + cmd.cmd === 'observe' && !!findTabBySession(stateRef.current, cmd.session_id); + if (opened || annotated || observed) { const stream = defaultChatStreamRegistry.getController( cmd.session_id ) as unknown as ObservableStream; diff --git a/ui/desktop/src/contexts/ChatGroupsContext.workspaceCommands.test.tsx b/ui/desktop/src/contexts/ChatGroupsContext.workspaceCommands.test.tsx index 8f4cf5eaa..2bee9e45a 100644 --- a/ui/desktop/src/contexts/ChatGroupsContext.workspaceCommands.test.tsx +++ b/ui/desktop/src/contexts/ChatGroupsContext.workspaceCommands.test.tsx @@ -139,6 +139,55 @@ describe('ChatGroupsProvider — the workspace command executor', () => { expect(mocks.observeSession).toHaveBeenCalled(); }); + // BR-71 §3c: the whole reason the `observe` frame exists. A tab the USER + // opened has no observer stream — nothing attaches one, because an ordinary + // tab is driven by its own `/reply` — so a conversation written into from + // elsewhere sat stale until reload. The daemon sends this frame after the row + // is durable; the observer's first frame is a full snapshot from the store, + // so the injected message renders whether or not the bus publish beat it. + it('an observe frame attaches the live feed to a tab this window already has', async () => { + mount(); + act(() => { + applyWorkspaceCommand(openTab('s-target')); + }); + await waitFor(() => expect(screen.getByTestId('sessions').textContent).toContain('s-target')); + mocks.observeSession.mockClear(); + + let result: WorkspaceCommandResult | undefined; + act(() => { + result = applyWorkspaceCommand({ + type: 'workspace', + cmd: 'observe', + session_id: 's-target', + }) as WorkspaceCommandResult; + }); + expect(result).toEqual(expect.objectContaining({ ok: true })); + expect(mocks.observeSession).toHaveBeenCalled(); + }); + + // The control, and the reason the executor re-checks `findTabBySession` + // itself rather than trusting `plan.result.ok`: `getController` is a + // create-AND-RETAIN, so calling it for a session with no tab both starts a + // stream for a chat that is nowhere on screen and leaks a controller, once + // per frame, on input the daemon fully controls. + it('an observe frame for a session with no tab here attaches nothing', async () => { + mount(); + act(() => { + applyWorkspaceCommand(openTab('s-mine')); + }); + await waitFor(() => expect(screen.getByTestId('sessions').textContent).toContain('s-mine')); + mocks.observeSession.mockClear(); + + act(() => { + applyWorkspaceCommand({ + type: 'workspace', + cmd: 'observe', + session_id: 's-in-another-window', + }); + }); + expect(mocks.observeSession).not.toHaveBeenCalled(); + }); + it('splits a new session into its own pane, from a frame delivered as the socket delivers one', async () => { // NOT wrapped in `act()`, and that is the entire point. `ws.onmessage` hands // the executor a frame from a MACROTASK; React then commits on the diff --git a/ui/desktop/src/hooks/chatStreamStore.observerIdle.test.tsx b/ui/desktop/src/hooks/chatStreamStore.observerIdle.test.tsx new file mode 100644 index 000000000..b0f0480fa --- /dev/null +++ b/ui/desktop/src/hooks/chatStreamStore.observerIdle.test.tsx @@ -0,0 +1,134 @@ +/** + * BR-71 §3c — **a message appended to an IDLE conversation must not make its + * tab claim a running turn.** + * + * Cross-chat injection publishes the stored row onto the target session's bus, + * and the target's tab renders it live through the observer feed. For + * `workspace_send_prompt mode:"note"` that row arrives with **no turn in + * flight**: a note is explicitly "leave context, start nothing". + * + * `applyMessageEvent` derives `ChatState.Streaming` from any message, because + * on the driver path a message is by definition a turn producing output. On the + * observer path with no active turn that is false, and it fails in the worst + * direction: nothing is running, so nothing will ever publish the terminal that + * would retire the state. Measured in the running app before the fix — the + * daemon's `/active_work` empty, the target tab showing "Thinking · 29s" with a + * live stop button, indefinitely. + * + * ⚠ This is the half a jsdom test *can* hold: what the store does with a frame. + * That the frame arrives at all — publish → SSE → observer — is a daemon fact, + * held by `workspace_extension`'s bus tests and by the GUI drive-through. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Message, MessageEvent, TokenState } from '../api'; + +const mocks = vi.hoisted(() => ({ + reply: vi.fn(), + observeSessionEvents: vi.fn(), + resumeAgent: vi.fn(async () => ({ data: null })), + cancelTurn: vi.fn(async () => ({ data: { cancelled: true } })), + getSession: vi.fn(async () => ({ data: null })), + interrupt: vi.fn(), + listApps: vi.fn(async () => ({ data: { apps: [] } })), + listSessions: vi.fn(async () => ({ data: { sessions: [] } })), + updateFromSession: vi.fn(async () => ({ data: {} })), + updateSessionUserWorkflowValues: vi.fn(async () => ({ data: {} })), +})); + +vi.mock('../api', async (importOriginal) => { + const actual = (await importOriginal()) as Record; + return { ...actual, ...mocks }; +}); + +const { ChatStreamRegistry } = await import('./chatStreamStore'); +const { ChatState } = await import('../types/chatState'); + +const tokenState = { + inputTokens: 0, + outputTokens: 0, + totalTokens: 0, + accumulatedInputTokens: 0, + accumulatedOutputTokens: 0, + accumulatedTotalTokens: 0, +} as unknown as TokenState; + +function userMessage(id: string, text: string): Message { + return { + id, + role: 'user', + created: Math.floor(Date.now() / 1000), + content: [{ type: 'text', text }], + metadata: { userVisible: true, agentVisible: true }, + } as unknown as Message; +} + +function messageFrame(id: string, text: string): MessageEvent { + return { type: 'Message', message: userMessage(id, text), token_state: tokenState } as MessageEvent; +} + +const turnStateIdle = { type: 'TurnState', active_turn_id: null } as unknown as MessageEvent; +const turnStateRunning = { + type: 'TurnState', + active_turn_id: 'turn-live', +} as unknown as MessageEvent; + +async function* streamOf(...frames: MessageEvent[]) { + for (const frame of frames) yield frame; +} + +let sessionSeq = 0; + +beforeEach(() => { + mocks.observeSessionEvents.mockReset(); + Object.assign(window, { + electron: { + getUserActionKey: vi.fn(async () => 'proof-of-user'), + showNotification: vi.fn(), + logInfo: vi.fn(), + }, + }); +}); + +/** Drive one observer connection to completion and stop the loop. */ +async function observeOnce(sid: string, frames: MessageEvent[]) { + mocks.observeSessionEvents.mockResolvedValue({ stream: streamOf(...frames) }); + const controller = new ChatStreamRegistry().getController(sid); + const loop = controller.observeSession(); + await vi.waitFor(() => expect(mocks.observeSessionEvents).toHaveBeenCalled()); + await new Promise((r) => setTimeout(r, 20)); + const state = controller.getSnapshot().chatState; + const messages = controller.getSnapshot().messages; + controller.stopObserving(); + await loop; + return { state, messages }; +} + +describe('an injected message on an observer feed', () => { + it('leaves an idle conversation idle', async () => { + const { state, messages } = await observeOnce(`obs-idle-${++sessionSeq}`, [ + turnStateIdle, + messageFrame('m-note', 'INJECTED-NOTE-MARKER'), + ]); + + // The row is rendered — that is the whole point of publishing it. + expect(messages.some((m) => JSON.stringify(m).includes('INJECTED-NOTE-MARKER'))).toBe(true); + // …and the tab does not claim a turn that does not exist. Nothing would + // ever retire it if it did. + expect(state).toBe(ChatState.Idle); + }); + + it('still reflects a real running turn as running', async () => { + // The control, and the reason the guard keys on `activeTurnId` rather than + // on "this is an observer". A genuinely running turn announces itself + // first — the SSE handler sends `TurnState` right after its snapshot — so + // its messages must still raise the running state, or an observed subagent + // would render as idle for its whole run. + const { state, messages } = await observeOnce(`obs-live-${++sessionSeq}`, [ + turnStateRunning, + messageFrame('m-live', 'STREAMED-OUTPUT-MARKER'), + ]); + + expect(messages.some((m) => JSON.stringify(m).includes('STREAMED-OUTPUT-MARKER'))).toBe(true); + expect(state).not.toBe(ChatState.Idle); + }); +}); diff --git a/ui/desktop/src/hooks/chatStreamStore.tsx b/ui/desktop/src/hooks/chatStreamStore.tsx index 163b8970a..c5a776743 100644 --- a/ui/desktop/src/hooks/chatStreamStore.tsx +++ b/ui/desktop/src/hooks/chatStreamStore.tsx @@ -1197,11 +1197,16 @@ class ChatStreamController { * steer retirement, and landed-tool-skeleton removal into one updater, so a * token event costs exactly one snapshot swap. */ + /// `keepIdle` holds the chat's running state where it is: the message is a + /// row appended to a conversation with no turn in flight (an injected note), + /// so nothing is generating and nothing will ever arrive to retire a running + /// state. Only the observer path passes it; see the call site. private applyMessageEvent = ( msg: Message, messages: Message[], tokenState: TokenState, - receivedAt: number + receivedAt: number, + keepIdle = false ): void => { this.messagesRef = messages; // A streamed message is not the row (or rows) it was stored as; see @@ -1219,7 +1224,7 @@ class ChatStreamController { const hasSecretRequest = msg.content.some( (content) => content.type === 'actionRequired' && content.data.actionType === 'secretRequest' ); - const chatState = + const derivedChatState = hasToolConfirmation || hasElicitation || hasSecretRequest ? ChatState.WaitingForUserInput : getCompactingMessage(msg) @@ -1227,6 +1232,11 @@ class ChatStreamController { : getThinkingMessage(msg) ? ChatState.Thinking : ChatState.Streaming; + // A confirmation card still parks the chat even when nothing is running — + // it is genuinely waiting on the person — so `keepIdle` yields to it rather + // than overriding it. + const chatState = + keepIdle && derivedChatState !== ChatState.WaitingForUserInput ? null : derivedChatState; // The authoritative request(s) landed: drop any matching pending skeletons // so the real tool card replaces the placeholder with no flicker or ghost. @@ -1248,7 +1258,8 @@ class ChatStreamController { return { ...prev, messages, - chatState, + // `null` is `keepIdle`: leave the running state exactly where it was. + chatState: chatState ?? prev.chatState, tokenState, lastMessageAt: receivedAt, pendingSteer: steerLanded ? undefined : prev.pendingSteer, @@ -1888,7 +1899,31 @@ class ChatStreamController { currentMessages = pushMessage(currentMessages, msg); // #22 — one snapshot swap (state + tokens + transcript + skeleton // cleanup) per streamed event, not three. - this.applyMessageEvent(msg, currentMessages, event.token_state, Date.now()); + // + // BR-71 §3c: on an OBSERVER feed with no turn in flight, a message + // is a row appended to an IDLE conversation, not a turn producing + // output. `workspace_send_prompt mode:"note"` is exactly that — it + // publishes the stored row and starts nothing — and + // `applyMessageEvent` derives `ChatState.Streaming` from any + // message, so without this the target tab claimed "Thinking…" with + // a stop button, indefinitely, for a turn that did not exist and so + // could never publish a terminal to retire it. Measured in the + // running app: `/active_work` empty, tab at `data-working="true"`. + // + // Narrow on purpose. A real observed turn announces itself first — + // `TurnStarted` sets `activeTurnId` via `applyObservedTurnState`, + // and the SSE handler sends `TurnState` right after its snapshot — + // so a running turn's messages still raise the running state. Only + // the no-turn case is held back, and only for an observer; the + // driver path is untouched. + const appendedWhileIdle = this.observing && !this.activeTurnId; + this.applyMessageEvent( + msg, + currentMessages, + event.token_state, + Date.now(), + appendedWhileIdle + ); break; } case 'Error': diff --git a/ui/desktop/src/toasts.autoClose.test.ts b/ui/desktop/src/toasts.autoClose.test.ts new file mode 100644 index 000000000..2fb78d7d7 --- /dev/null +++ b/ui/desktop/src/toasts.autoClose.test.ts @@ -0,0 +1,52 @@ +/** + * **A notification the user has walked away from is the one that most needs to + * expire**, and until this test it was the one that never did. + * + * `react-toastify` defaults `pauseOnFocusLoss` to true: every dismissal timer + * stops while the window is not frontmost. In an Electron app that is most of + * the time — the user reads a toast, switches to their editor, and comes back + * to a stack still sitting there. Observed on a fresh install: six "Extension + * installed" toasts, minutes old, none of them expiring, which reads as "these + * need dismissing" rather than as the FYI they are. + * + * `pauseOnHover` is deliberately left ON, and the pair is the whole point: + * hovering pauses because the user is reading, which is a reason to wait; + * losing focus is a reason to go. + */ +import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const source = readFileSync( + resolve(dirname(fileURLToPath(import.meta.url)), 'toasts.tsx'), + 'utf8' +); + +/** The shared options block every toast in the app is built from. */ +function commonToastOptionsBlock(): string { + const start = source.indexOf('const commonToastOptions: ToastOptions = {'); + expect(start, 'commonToastOptions was renamed; this test cannot find it').toBeGreaterThan(-1); + const end = source.indexOf('};', start); + return source.slice(start, end); +} + +describe('the shared toast options', () => { + it('do not let an unfocused window freeze every dismissal timer', () => { + expect(commonToastOptionsBlock()).toContain('pauseOnFocusLoss: false'); + }); + + it('still pause while the user is hovering, which is a reason to wait', () => { + expect(commonToastOptionsBlock()).toContain('pauseOnHover: true'); + }); + + it('expire inside the window a person will actually wait', () => { + const ms = /export const TOAST_AUTO_CLOSE_MS = (\d+);/.exec(source); + expect(ms, 'TOAST_AUTO_CLOSE_MS was renamed or removed').not.toBeNull(); + const value = Number(ms![1]); + // Long enough to read a two-line message, short enough that a stack of + // them clears itself rather than becoming a chore. + expect(value).toBeGreaterThanOrEqual(3000); + expect(value).toBeLessThanOrEqual(15000); + }); +}); diff --git a/ui/desktop/src/toasts.tsx b/ui/desktop/src/toasts.tsx index 8b3af32e4..2671b6340 100644 --- a/ui/desktop/src/toasts.tsx +++ b/ui/desktop/src/toasts.tsx @@ -222,6 +222,15 @@ const commonToastOptions: ToastOptions = { closeOnClick: true, pauseOnHover: true, draggable: true, + // ⚠ **Off, and this is what makes `autoClose` mean anything.** + // react-toastify defaults `pauseOnFocusLoss` to true, so every dismissal + // timer stops the moment the window is not frontmost — and a notification + // the user has already walked away from is exactly the one that most needs + // to expire. The observed result was a stack of toasts still sitting there + // minutes later, which reads as "these need dismissing" rather than "these + // were FYI". `pauseOnHover` stays on: that one pauses because the user is + // reading it, which is a reason to wait. + pauseOnFocusLoss: false, }; /** diff --git a/ui/desktop/src/utils/catalogSubscription.test.ts b/ui/desktop/src/utils/catalogSubscription.test.ts index c82f17f15..27c46ceab 100644 --- a/ui/desktop/src/utils/catalogSubscription.test.ts +++ b/ui/desktop/src/utils/catalogSubscription.test.ts @@ -214,6 +214,56 @@ describe('reading a delta', () => { expect(newlyInstalledExtensions(d)).toEqual([{ key: 'bioroffice', name: 'bioroffice' }]); }); + /** + * ⚠ **A first run is not six installs.** The daemon writes its own bundled + * baseline into the catalogue the first time it starts, and every entry + * arrives as `added` + `enabled` — so a brand-new user, who had set nothing + * up, met a stack of "Extension installed. Turn it on for this chat…" toasts + * naming `developer`, `computercontroller`, `autovisualiser`, `memory`, + * `knowledge` and `agent_drafter`. The notification exists for an install + * made somewhere else that this chat can now opt into; Biorouter's own + * baseline is never that. + */ + it('does not announce Biorouter\'s own bundled extensions', () => { + const d = delta(1, ['developer']); + d.changes![0].extensions![0].config = { + name: 'developer', + description: 'Developer tools', + type: 'builtin', + bundled: true, + } as never; + expect(newlyInstalledExtensions(d)).toEqual([]); + + // A platform extension is the same case by a different config shape: only + // Biorouter can register an in-process server. + const p = delta(2, ['workspace']); + p.changes![0].extensions![0].config = { + name: 'workspace', + description: 'Workspace Control', + type: 'platform', + } as never; + expect(newlyInstalledExtensions(p)).toEqual([]); + }); + + it('still announces a third-party install, and one with no config at all', () => { + // The control. Silencing the baseline must not silence the case the + // notification was written for. + const d = delta(3, ['bioroffice']); + d.changes![0].extensions![0].config = { + name: 'bioroffice', + description: 'Office documents', + type: 'stdio', + cmd: 'bioroffice', + args: [], + } as never; + expect(newlyInstalledExtensions(d)).toEqual([{ key: 'bioroffice', name: 'bioroffice' }]); + + // No config: fail towards NOTIFYING. One extra notification is a smaller + // harm than silently swallowing a real third-party install. + const bare = delta(4, ['mystery']); + expect(newlyInstalledExtensions(bare)).toEqual([{ key: 'mystery', name: 'mystery' }]); + }); + /** A toggle is not an install; offering to attach one would be noise. */ it('does not treat an enable as a new install', () => { const d = delta(1, ['bioroffice']); diff --git a/ui/desktop/src/utils/catalogSubscription.ts b/ui/desktop/src/utils/catalogSubscription.ts index b05380fa0..0c87ccbf8 100644 --- a/ui/desktop/src/utils/catalogSubscription.ts +++ b/ui/desktop/src/utils/catalogSubscription.ts @@ -1,5 +1,5 @@ import { catalogChanges } from '../api'; -import type { CatalogChanged, CatalogDelta } from '../api'; +import type { CatalogChanged, CatalogDelta, CatalogExtensionChange } from '../api'; /** * Issue #112. The renderer's ear on the extension catalogue. @@ -166,14 +166,48 @@ export function changedExtensionKeys(delta: CatalogDelta): string[] { return [...keys]; } -/** The extensions a delta reports as newly installed and enabled. */ +/** + * Whether this catalogue entry is something **Biorouter ships**, as opposed to + * something the user installed. + * + * `bundled: true` is the flag the daemon sets on its own baseline; the + * `builtin` and `platform` config shapes are in-process servers that only + * Biorouter can register, so neither can arrive from a marketplace install. + * A missing `config` is treated as NOT built-in — the safe direction here is to + * risk one extra notification, never to silence a real third-party install. + */ +function isShippedWithBiorouter(config: CatalogExtensionChange['config']): boolean { + if (!config) return false; + if ('bundled' in config && config.bundled) return true; + return config.type === 'builtin' || config.type === 'platform'; +} + +/** + * The extensions a delta reports as newly installed and enabled **by the + * user**. + * + * ⚠ **Biorouter's own bundled extensions are excluded, and that is the point.** + * On a first run the daemon writes its baseline — `developer`, + * `computercontroller`, `autovisualiser`, `memory`, `knowledge`, + * `agent_drafter` — into the catalogue, and every one of them arrived here as + * an `added` + `enabled` change. The user then met a fresh install with six + * stacked "Extension installed. Turn it on for this chat…" toasts for things + * they had never heard of and had not asked for, on a screen where they had not + * yet set anything up at all. The notification is worth having for the case it + * was written for — an install made in another window or another terminal, which + * this chat can now opt into — and that case is always a third-party extension. + */ export function newlyInstalledExtensions( delta: CatalogDelta ): Array<{ key: string; name: string }> { const found = new Map(); for (const change of (delta.changes ?? []) as CatalogChanged[]) { for (const extension of change.extensions ?? []) { - if (extension.change === 'added' && extension.enabled) { + if ( + extension.change === 'added' && + extension.enabled && + !isShippedWithBiorouter(extension.config) + ) { found.set(extension.key, { key: extension.key, name: extension.name }); } }