diff --git a/CLAUDE.md b/CLAUDE.md index d00981305..ef38a8d92 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1187,7 +1187,7 @@ Test the gate where it is: the unit tests in `agents/agent.rs` prints a URL. The daemon serves the SPA **on its own origin**, so nothing is proxied. This replaced a standalone `biorouter-headless` binary and its Linux tarball, both deleted 2026-08-23; release assets went 11 → 10. Design and reasoning: -[`docs/deployment/serve-decisions.md`](docs/deployment/serve-decisions.md) (SD-1..SD-9), +[`docs/deployment/serve-decisions.md`](docs/deployment/serve-decisions.md) (SD-1..SD-9, SD-11), [`serve-architecture.md`](docs/deployment/serve-architecture.md), [`browser-access.md`](docs/deployment/browser-access.md). @@ -1211,6 +1211,20 @@ replaced a standalone `biorouter-headless` binary and its Linux tarball, both de operator-pinned-off extension and its ordinary path needs none. ⚠ Not a security change: nothing that was refused becomes permitted. The availability flag is sampled ONCE per roster and threaded, so a roster can never half-believe a person is reachable. +- **Stop answers to the reach gate on a keyless daemon; steering does not** (SD-11). + `/agent/cancel` and the two `/agent/continuation/*` routes take the proof on a daemon that + holds a key, and on one that holds none gate through `authorize_agent_control` — the *same + call* `/agent/stop` makes — via `reply.rs::authorize_turn_control`. Tightening that gate + tightens who may press Stop in a browser. ⚠ **`/interrupt` is NOT one of them.** It keeps the + proof on both kinds of daemon: the keyless arm's whole argument is that the caller already + reaches the same effect through `/agent/stop` and `/reply`, and `/reply` is refused `409` by + the BR-33 single-turn lock in the exact state where a steer lands — so admitting it would add + silent mid-turn injection into a turn already in flight, which nothing else there can do + (`reply.rs::authorize_steer`). Its keyless refusal carries `STEER_NO_KEY` and is **never an + empty 403**, because an empty turn-control 403 is how `biorouter session attach` recognises a + daemon that holds a key and asks the person for it. A subagent's tab stays refused throughout. + ⚠ Keyless behaviour can only be tested in its own binary (the digest is a process-global + `OnceLock`): `cargo test -p biorouter-server --test turn_control_no_user_key`. - **Proof of a person is checked at the resolution choke point, not at one route.** Every door that answers a parked decision — the HTTP route, an Agent Drafter app's WebSocket, ACP, the CLI prompt, the TUI modal, an ancestor agent's relay — passes a `DecisionAuthority` into diff --git a/crates/biorouter-server/src/commands/agent.rs b/crates/biorouter-server/src/commands/agent.rs index c2fa7dfbe..d683d5b9e 100644 --- a/crates/biorouter-server/src/commands/agent.rs +++ b/crates/biorouter-server/src/commands/agent.rs @@ -109,6 +109,89 @@ fn watch_parent(expected: u32) -> CancellationToken { CancellationToken::new() } +/// Why this daemon came up holding no user-action digest. +/// +/// Four causes, kept apart because they mean very different things and only one +/// [`read_user_action_digest`] returns is a *mistake*. Before SD-11 they were one +/// `None`, which was survivable while a keyless daemon simply refused every +/// control that needed the proof: the failure was loud at the first click. Now +/// three of the four turn-control routes fall back to the reach gate there +/// (`routes::reply::authorize_turn_control`), so a desktop launcher that misses +/// the 2 s window comes up **quietly** weaker than the one the user installed +/// rather than visibly broken. Naming the cause is what keeps that from being a +/// silent degradation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum NoUserActionKey { + /// stdin is a terminal: a person started this daemon at a prompt + /// (`just run-server`, `biorouterd agent` by hand). Expected. + HandStarted, + /// stdin closed with nothing on it. `biorouter serve` spawns its daemon with + /// `Stdio::null()` precisely so that this happens (SD-7). Expected. + NoneOffered, + /// ⚠ A writer held the pipe open and put no digest on it inside the bound. + /// Nothing Biorouter ships does that on purpose, so this is the arm that + /// means a launcher is broken or the machine was too loaded to make the + /// window — the one case where a keyless daemon is an accident. + TimedOut, + /// ⚠ A line arrived and was not 32 bytes of hex. Also a launcher fault. + Malformed, +} + +impl NoUserActionKey { + /// One sentence saying what happened and, always, what it costs. + /// + /// Every arm names **both** consequences, because a reader who has just + /// learnt their daemon is keyless needs to know what that daemon now does + /// differently, not only what it refuses. `unit_tests` below asserts it of + /// each arm rather than leaving it to whoever edits one of them. + fn warning(self) -> String { + let cause = match self { + Self::HandStarted => { + "no user-action key: stdin is a terminal, so this daemon was started by hand" + } + Self::NoneOffered => { + "no user-action key: stdin closed without one, which is how `biorouter serve` \ + starts its daemon" + } + Self::TimedOut => { + "no user-action key: something held stdin open and wrote no digest within 2s. \ + If this is the desktop application's daemon, its launcher FAILED to hand the \ + key over and this daemon is weaker than the one you installed — restart it" + } + Self::Malformed => { + "no user-action key: the line on stdin was not a 32-byte hex digest. If this is \ + the desktop application's daemon, its launcher is broken — restart it" + } + }; + format!( + "{cause}. This daemon cannot verify that a request came from the person at the \ + keyboard, so it will refuse every request that raises a session's privacy \ + capability, including one made by that person; and Stop, Stop-and-Send and the \ + continuation routes answer to the reach gate instead of to the proof (serve \ + decision SD-11), which admits any caller holding the daemon secret to the chats \ + that gate admits. Mid-turn steering stays refused." + ) + } +} + +/// The digest, or why there is none, from the line stdin produced — `None` for a +/// read that did not finish inside the bound. +/// +/// Split out from the I/O so the mapping is testable: the whole point of the +/// four arms is that they are told apart, and a classification that lives inside +/// an `async fn` reading real stdin is one nothing can check. +fn classify_digest_line(line: Option) -> Result<[u8; 32], NoUserActionKey> { + let Some(line) = line else { + return Err(NoUserActionKey::TimedOut); + }; + let line = line.trim(); + if line.is_empty() { + return Err(NoUserActionKey::NoneOffered); + } + let bytes = hex::decode(line).map_err(|_| NoUserActionKey::Malformed)?; + <[u8; 32]>::try_from(bytes.as_slice()).map_err(|_| NoUserActionKey::Malformed) +} + /// Read the launcher's SHA-256 user-action digest off stdin, as one hex line /// (issue #56, DR-16). /// @@ -117,12 +200,16 @@ fn watch_parent(expected: u32) -> CancellationToken { /// by a child, and the raw key was never there to begin with. /// /// It must **never block a hand-started daemon**, so it is guarded twice. -async fn read_user_action_digest() -> Option<[u8; 32]> { +/// +/// ⚠ The 2 s bound is unchanged. It is not raised here because nothing measured +/// says the desktop launcher misses it; what changed is that missing it is now +/// *reported* rather than folded into the three expected ways of holding no key. +async fn read_user_action_digest() -> Result<[u8; 32], NoUserActionKey> { use std::io::IsTerminal; // (1) A terminal is a human at a prompt, not a launcher with a key. Reading // it would hang `just run-server` forever waiting for a line. if std::io::stdin().is_terminal() { - return None; + return Err(NoUserActionKey::HandStarted); } // (2) And a pipe whose writer never closes would hang just as hard, so the // read is bounded. 2s is far longer than a local `write` + `end`. @@ -144,12 +231,15 @@ async fn read_user_action_digest() -> Option<[u8; 32]> { // The receiver is gone on the timeout path; nothing to report to. let _ = tx.send(read); }); + // A timeout, a dropped sender and a failed `read_line` are all "no line + // arrived inside the bound", which is the one arm that means a launcher + // wrote nothing it promised. let line = tokio::time::timeout(std::time::Duration::from_secs(2), rx) .await - .ok()? - .ok()??; - let bytes = hex::decode(line.trim()).ok()?; - <[u8; 32]>::try_from(bytes.as_slice()).ok() + .ok() + .and_then(Result::ok) + .flatten(); + classify_digest_line(line) } pub async fn run(exit_with_parent: Option) -> Result<()> { @@ -209,13 +299,18 @@ pub async fn run(exit_with_parent: Option) -> Result<()> { // tool that reads a caller-named path (`/proc/self/environ`) or, on macOS, // by `sysctl(KERN_PROCARGS2)`, which is not a path at all and which no // sandbox profile can gate. - let user_action_digest = read_user_action_digest().await; - if user_action_digest.is_none() { - tracing::warn!( - "no user-action key on stdin: this daemon will refuse every request that raises a \ - session's privacy capability, including one made by the person at the keyboard" - ); - } + let user_action_digest = match read_user_action_digest().await { + Ok(digest) => Some(digest), + Err(reason) => { + // ⚠ One WARN, and it names the SD-11 consequence as well as the + // privacy one. Before SD-11 a keyless desktop daemon announced + // itself at the first click — Stop answered 403 and the user + // complained. Now Stop works there, so the same misconfiguration is + // silent unless this line says so. + tracing::warn!("{}", reason.warning()); + None + } + }; // A tool whose approval can never be granted must not be offered. `serve` // spawns this daemon with `Stdio::null()`, so it holds no key and every // proof-backed approval refuses forever — the install and delete tools take @@ -344,6 +439,86 @@ pub async fn run(exit_with_parent: Option) -> Result<()> { Ok(()) } +/// The keyless-startup report, on every platform (unlike the `unix`-only module +/// below). +#[cfg(test)] +mod keyless_report_tests { + use super::{classify_digest_line, NoUserActionKey}; + + /// The four causes are told apart. They were one `None` until SD-11 made a + /// keyless daemon behave differently rather than merely refuse more, at + /// which point a launcher that misses the window stops being visible. + #[test] + fn the_four_ways_of_holding_no_key_are_distinguishable() { + let digest = "a".repeat(64); + assert_eq!( + classify_digest_line(Some(format!("{digest}\n"))), + Ok([0xaa; 32]) + ); + // Nothing arrived inside the bound: a writer held the pipe open. The + // only arm that means something is wrong. + assert_eq!(classify_digest_line(None), Err(NoUserActionKey::TimedOut)); + // `Stdio::null()`, which is how `biorouter serve` starts its daemon: the + // read succeeds at EOF and yields nothing. + for empty in ["", "\n", " \n"] { + assert_eq!( + classify_digest_line(Some(empty.to_string())), + Err(NoUserActionKey::NoneOffered), + "{empty:?}" + ); + } + // Present and wrong, which a launcher fault also looks like. + for bad in ["not-hex", "abcd", &digest[..62], &format!("{digest}aa")] { + assert_eq!( + classify_digest_line(Some(bad.to_string())), + Err(NoUserActionKey::Malformed), + "{bad:?}" + ); + } + } + + /// Every arm names BOTH consequences — what this daemon refuses, and what it + /// now admits instead (SD-11) — and the two launcher faults say they are + /// faults. A reader who has just learnt their daemon is keyless needs the + /// second half as much as the first. + #[test] + fn every_warning_names_what_a_keyless_daemon_does_differently() { + for reason in [ + NoUserActionKey::HandStarted, + NoUserActionKey::NoneOffered, + NoUserActionKey::TimedOut, + NoUserActionKey::Malformed, + ] { + let warning = reason.warning(); + assert!( + warning.contains("privacy capability"), + "{reason:?} does not name what it refuses: {warning}" + ); + assert!( + warning.contains("SD-11") && warning.contains("reach gate"), + "{reason:?} does not name what it admits instead: {warning}" + ); + assert!( + warning.contains("Mid-turn steering stays refused"), + "{reason:?} does not say steering is still refused: {warning}" + ); + } + // The two that mean a launcher is broken say so, and say what to do. + for fault in [NoUserActionKey::TimedOut, NoUserActionKey::Malformed] { + let warning = fault.warning(); + assert!( + warning.contains("restart it"), + "{fault:?} is a misconfiguration and must be actionable: {warning}" + ); + } + // …and the two expected ones do not, so the WARN cannot cry wolf on + // every `biorouter serve` start. + for expected in [NoUserActionKey::HandStarted, NoUserActionKey::NoneOffered] { + assert!(!expected.warning().contains("restart it"), "{expected:?}"); + } + } +} + /// Only [`until_orphaned`], never [`watch_parent`]: the latter arms a /// `process::exit`, which would take the test binary down with it. #[cfg(all(test, unix))] diff --git a/crates/biorouter-server/src/routes/agent.rs b/crates/biorouter-server/src/routes/agent.rs index e9f761dc9..dbbe25fa9 100644 --- a/crates/biorouter-server/src/routes/agent.rs +++ b/crates/biorouter-server/src/routes/agent.rs @@ -70,17 +70,50 @@ pub struct UpdateFromSessionRequest { const SUBAGENT_USER_ACTION_REQUIRED: &str = "Changing or resuming a subagent from its tab requires proof that the request came from the person at the keyboard."; +/// …and when the daemon holds no user-action key at all. +/// +/// A separate sentence, per Task 18A's open question 23 and SD-8: telling a +/// person at a `biorouter serve` page that their request "requires proof" sends +/// them hunting for a permission this daemon can never grant anyone. It names +/// the daemon as the reason, in the register `CROSS_AFFILIATION_GRANT_NO_KEY` +/// and `session_reach::SESSION_REACH_NO_KEY` already use. Since SD-11 this is +/// also what a keyless daemon answers a Stop aimed at a subagent's turn, because +/// that route gates through [`authorize_agent_control`] there. A *steer* at the +/// same turn is refused one step earlier, by `reply::authorize_steer`, which +/// never reads the row — so the two sentences differ, and both open by naming +/// this daemon rather than the caller. +const SUBAGENT_CONTROL_NO_KEY: &str = + "This daemon was started without a user-action key, so it cannot verify that a request came \ + from the person at the keyboard, and changing, resuming, stopping or steering a subagent from \ + its tab requires that proof. Nothing was changed. This control is unavailable on this \ + daemon; use the desktop app."; + +/// A 424 in the shape every other refusal in this file has, so that gating a +/// route which used to answer bare status codes does not change the body a client +/// sees for the failures it already handled. +fn agent_not_initialized(message: &str) -> ErrorResponse { + ErrorResponse { + message: message.to_string(), + status: StatusCode::FAILED_DEPENDENCY, + } +} + fn refuse_subagent_unless_user( session: &Session, headers: &HeaderMap, ) -> Result<(), ErrorResponse> { - if session.session_type == SessionType::SubAgent && !is_user_action(headers) { - return Err(ErrorResponse { - message: SUBAGENT_USER_ACTION_REQUIRED.to_string(), - status: StatusCode::FORBIDDEN, - }); + if session.session_type != SessionType::SubAgent { + return Ok(()); } - Ok(()) + let message = match user_action_proof(headers) { + UserActionProof::Proven => return Ok(()), + UserActionProof::Unproven => SUBAGENT_USER_ACTION_REQUIRED, + UserActionProof::NoKeyInstalled => SUBAGENT_CONTROL_NO_KEY, + }; + Err(ErrorResponse { + message: message.to_string(), + status: StatusCode::FORBIDDEN, + }) } #[async_trait::async_trait] @@ -146,7 +179,16 @@ async fn read_update_session( /// Authorize an HTTP control-plane operation before it can touch an agent or a /// queued child handle. The daemon bearer proves only that the caller reached /// this process; it does not prove that a person chose to mutate a subagent. -async fn authorize_agent_control( +/// +/// ⚠ **Also the turn-control gate on a daemon with no user-action key** (SD-11): +/// `routes::reply`'s `authorize_turn_control` calls this for `/agent/cancel` and +/// the two continuation routes there, so that stopping a turn admits exactly the +/// callers `/agent/stop` admits. Tightening this therefore tightens those three +/// too, which is the point — but it is a change to who may press Stop in a +/// browser, and `tests/turn_control_no_user_key.rs` will say so. `/interrupt` is +/// NOT among them: `reply::authorize_steer` keeps the proof on every daemon, +/// because the dominance argument that admits a Stop does not reach a steer. +pub(crate) async fn authorize_agent_control( state: &AppState, session_id: &str, headers: &HeaderMap, @@ -1248,22 +1290,48 @@ async fn get_tools( responses( (status = 200, description = "Model-visible callable tool count", body = CallableToolCountResponse), (status = 401, description = "Unauthorized - invalid secret key"), + (status = 403, description = "Refused by a privacy boundary (issue #56 Task 58 / #47): \ + the named chat is private (or absent, and an unproven caller \ + is told the same thing for both) and the request carried \ + neither a capability that covers it nor proof it came from \ + the user"), (status = 424, description = "Agent not initialized") ) )] async fn get_callable_tool_count( State(state): State>, + // Before `Query`, which is fine either way here, but keeps the extractor + // order the rest of this file uses. + headers: axum::http::HeaderMap, Query(query): Query, -) -> Result, StatusCode> { +) -> Result, ErrorResponse> { let session_id = query.session_id; + // Issue #56 Task 58 / #47. FIRST, before the agent is fetched, for the reason + // `agent_add_extension` states at length: `get_agent_for_route` CREATES an + // agent for a session that has none, so a gate below it would let an unproven + // caller materialise one for a chat it may not address — and this route's own + // 424 would then tell it what it had found. `session_id` is a request + // parameter, not a credential; see `routes::session_reach`. + // + // ⚠ This route had NO gate of any kind, and PR #260's renderer merely stopped + // calling it for a subagent's chat, which left the route exactly as open as + // it was. Routing a client around an ungated route does not gate it. + crate::routes::session_reach::session_reach(state.session_manager(), &session_id, &headers) + .await?; let child_initializing = biorouter::agents::subagent_handle::is_child_initializing(&session_id); let agent = if child_initializing { state .peek_agent(&session_id) .await - .ok_or(StatusCode::FAILED_DEPENDENCY)? + .ok_or_else(|| agent_not_initialized("that chat's runtime is not ready yet"))? } else { - state.get_agent_for_route(session_id.clone()).await? + state + .get_agent_for_route(session_id.clone()) + .await + .map_err(|status| ErrorResponse { + message: "could not load that chat".to_string(), + status, + })? }; // This endpoint drives a model-context warning. Count the final model-facing @@ -1274,7 +1342,7 @@ async fn get_callable_tool_count( let count = agent .callable_tool_count(&session_id) .await - .map_err(|_| StatusCode::FAILED_DEPENDENCY)?; + .map_err(|_| agent_not_initialized("that chat's tools could not be counted"))?; Ok(Json(CallableToolCountResponse { count })) } @@ -2233,7 +2301,9 @@ pub(crate) async fn apply_working_dir_update( (status = 403, description = "Refused by a privacy boundary (issue #56 Task 58 / #47): \ the named chat is private (or absent, and an unproven caller \ is told the same thing for both) and the request carried no \ - proof it came from the user"), + proof it came from the user; or the named chat is a delegated \ + subagent's, whose working directory only the person at the \ + keyboard may repoint (SD-8)"), (status = 404, description = "Session not found"), ( status = 409, @@ -2258,6 +2328,46 @@ async fn update_working_dir( crate::routes::session_reach::session_reach(state.session_manager(), &session_id, &headers) .await?; + // SD-8, and the one write to a subagent's chat this daemon did not refuse. + // This route repoints the named chat at a directory of the caller's choosing + // and restarts its agent there, which for a delegated child is a change to + // the thing the parent is being told about. Every other write to a + // subagent's chat already asks for the proof — `/reply` inline, + // `/agent/resume` through `read_resume_session`, and provider, extension, + // stop and restart through `authorize_agent_control` — and this one did not, + // which made the shipped SD-8 claim that "the daemon refuses every write to + // it" false. + // + // ⚠ **`session_reach` above does not cover it, and cannot.** That gate is the + // PRIVACY slice and is deliberately inert for a public session; a delegated + // subagent's chat is normally public. The two 409s below are not the boundary + // either: `try_update_working_dir_if_empty` refuses a chat that has messages + // and the turn lock refuses one that is busy, and a just-spawned or queued + // child is neither — which is exactly the window in which a subagent's tab + // is interesting. + // + // AFTER the reach gate and BEFORE the turn lock, so the three refusals stay + // in the order the rest of this file uses: a chat this caller may not reach + // is refused without disclosing that it is a subagent's, and a subagent's is + // refused without disclosing whether it is busy. + // + // Not `authorize_agent_control`, which would call `session_reach` a second + // time, and not `read_update_session`, whose read failure is a 500 — this + // route documents (and the desktop handles) a 404 for a session that is not + // there. + let target = state + .session_manager() + .get_session(&session_id, false) + .await + .map_err(|error| { + error!("Failed to get session before working dir update: {}", error); + ErrorResponse { + message: format!("Failed to get session: {}", error), + status: StatusCode::NOT_FOUND, + } + })?; + refuse_subagent_unless_user(&target, &headers)?; + // Serialize with `/reply`'s per-session turn lock (BR-33) by claiming the // turn slot for the whole update + restart. Without it, a first message // accepted (but not yet persisted) by an in-flight reply could pass the @@ -3164,17 +3274,24 @@ mod resume_update_security_tests { fn openapi_describes_the_agent_route_failures_clients_must_handle() { let schema: serde_json::Value = serde_json::from_str(&crate::openapi::generate_schema()).unwrap(); - for (path, statuses) in [ - ("/agent/start", &["409"][..]), - ("/agent/resume", &["403", "404"][..]), - ("/agent/update_from_session", &["403", "500"][..]), - ("/agent/restart", &["424"][..]), + for (method, path, statuses) in [ + ("post", "/agent/start", &["409"][..]), + ("post", "/agent/resume", &["403", "404"][..]), + ("post", "/agent/update_from_session", &["403", "500"][..]), + ("post", "/agent/restart", &["424"][..]), + // The two the SD-8 review gated. A client that has only ever seen a + // 200/424 from the tool count, or a 400/404/409 from the working-dir + // switch, now has a 403 to handle, and the generated TS client is + // where it has to be visible. + ("post", "/agent/update_working_dir", &["403"][..]), + ("get", "/agent/callable_tool_count", &["403", "424"][..]), ] { - let responses = &schema["paths"][path]["post"]["responses"]; + let responses = &schema["paths"][path][method]["responses"]; for status in statuses { assert!( responses.get(*status).is_some(), - "POST {path} is missing its {status} OpenAPI response" + "{} {path} is missing its {status} OpenAPI response", + method.to_uppercase() ); } } @@ -3387,6 +3504,13 @@ mod resume_update_security_tests { "/agent/stop" | "/agent/restart" => serde_json::json!({ "session_id": session_id, }), + // An existing directory, deliberately: a path that does not exist is + // rejected with a 400 before the boundary is ever consulted, so a + // test written with one passes against an ungated route. + "/agent/update_working_dir" => serde_json::json!({ + "session_id": session_id, + "working_dir": std::env::temp_dir().to_string_lossy(), + }), _ => panic!("unexpected route in test: {path}"), }; let mut request = Request::builder() @@ -3407,6 +3531,61 @@ mod resume_update_security_tests { .status() } + /// `GET /agent/callable_tool_count?session_id=`, with the status AND the body: + /// the body is what separates a gate from a 424 that happens to look like one. + async fn get_callable_tool_count_response( + state: Arc, + session_id: &str, + user_action: Option<&str>, + ) -> (StatusCode, String) { + let mut request = Request::builder().method("GET").uri(format!( + "/agent/callable_tool_count?session_id={session_id}" + )); + if let Some(key) = user_action { + request = request.header("X-User-Action", key); + } + let response = routes(state) + .oneshot(request.body(Body::empty()).unwrap()) + .await + .unwrap(); + let status = response.status(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + (status, String::from_utf8_lossy(&body).into_owned()) + } + + /// One agent route, one session id, no proof — status and body, so a test can + /// compare two refusals for sameness rather than merely for their code. + async fn post_agent_route_response( + state: Arc, + path: &str, + session_id: &str, + ) -> (StatusCode, String) { + let response = routes(state) + .oneshot( + Request::builder() + .method("POST") + .uri(path) + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_vec(&serde_json::json!({ + "session_id": session_id, + "load_model_and_extensions": false, + })) + .unwrap(), + )) + .unwrap(), + ) + .await + .unwrap(); + let status = response.status(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + (status, String::from_utf8_lossy(&body).into_owned()) + } + async fn get_agent_tools(state: Arc, session_id: &str) -> StatusCode { routes(state) .oneshot( @@ -3635,6 +3814,179 @@ mod resume_update_security_tests { } } + /// SD-8 review finding 1. `/agent/update_working_dir` is a WRITE to the named + /// chat — it repoints the session at a directory of the caller's choosing and + /// restarts its agent there — and it used to consult only `session_reach`, + /// which is **deliberately inert for a public session**. A delegated + /// subagent's chat is normally public, so a caller holding nothing but the + /// daemon secret could repoint a child, while the shipped SD-8 record said + /// the daemon "refuses every write" to a subagent's chat. + /// + /// ⚠ **The public arm is the one that fails without the gate.** The private + /// arm was already refused, by `session_reach`, for a reason that has nothing + /// to do with subagents — so a test written on a private child alone passes + /// against the hole. + /// + /// The directory is read back rather than trusting the status code: the two + /// 409s this route already had (a chat with messages, a turn in flight) are + /// not the subagent boundary and must not be mistaken for it. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn bearer_only_cannot_repoint_a_subagents_working_directory() { + install_test_user_action_key(); + let state = AppState::new().await.unwrap(); + + for private in [false, true] { + let child = seed(&state, SessionType::SubAgent, private).await; + let before = state + .session_manager() + .get_session(child.id(), false) + .await + .unwrap() + .working_dir; + + assert_eq!( + post_agent_route( + Arc::clone(&state), + "/agent/update_working_dir", + child.id(), + None, + ) + .await, + StatusCode::FORBIDDEN, + "/agent/update_working_dir accepted bearer-only repointing of a {} child", + if private { "private" } else { "public" } + ); + assert_eq!( + state + .session_manager() + .get_session(child.id(), false) + .await + .unwrap() + .working_dir, + before, + "the refused request still moved the {} child's working directory", + if private { "private" } else { "public" } + ); + assert!( + state.peek_agent(child.id()).await.is_none(), + "/agent/update_working_dir restarted an agent before refusing the request" + ); + } + } + + /// SD-8 review finding 2. `/agent/callable_tool_count` had no gate of any + /// kind: not `session_reach`, not the subagent refusal. It is a read of the + /// named session's model-facing tool surface, and it answers through + /// `get_agent_for_route`, which **creates** an agent for a session that has + /// none — the same hazard `agent_add_extension` gates against and says so. + /// + /// PR #260's renderer stopped calling it for a subagent's chat, which left + /// the route exactly as open as it was. Gated the way its tier-bearing + /// siblings are (`GET /sessions/{id}`, `POST /agent/resume`): the privacy + /// reach gate, first, before the agent is fetched. + /// + /// The `peek_agent` assertion is not decoration — a gate placed below the + /// fetch would pass the status assertion and still mint the agent. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn callable_tool_count_refuses_an_unproven_caller_naming_a_private_chat() { + install_test_user_action_key(); + let state = AppState::new().await.unwrap(); + let private = seed(&state, SessionType::User, true).await; + + let (status, body) = + get_callable_tool_count_response(Arc::clone(&state), private.id(), None).await; + assert_eq!( + status, + StatusCode::FORBIDDEN, + "/agent/callable_tool_count answered an unproven caller about a private chat: {body}" + ); + assert!( + state.peek_agent(private.id()).await.is_none(), + "/agent/callable_tool_count materialized an agent for a chat it may not address" + ); + + // And the proof is sufficient, so the desktop's own tool-count alert is + // unaffected: whatever this answers, it is not the reach refusal. + let (proven, _) = get_callable_tool_count_response( + Arc::clone(&state), + private.id(), + Some(TEST_USER_ACTION_KEY), + ) + .await; + assert_ne!( + proven, + StatusCode::FORBIDDEN, + "/agent/callable_tool_count refused a request carrying the user-action proof" + ); + } + + /// SD-8 review finding 4, recorded as a MEASUREMENT rather than closed as a + /// bug — see `docs/deployment/serve-decisions.md`. + /// + /// The refusal bodies do differ: a **public** subagent is told it is a + /// subagent, an unknown id is told only that it is out of reach. That pair + /// would be an existence oracle for subagent ids if it were the only way to + /// learn the fact. It is not, and the fourth row here is why: the same + /// unproven caller is answered **200** for a public chat that is not a + /// subagent's, and `GET /sessions/{id}` — inert for public chats by the same + /// rule — hands it the whole row, `session_type` included. The differing body + /// discloses nothing that a 200 next door does not. + /// + /// What must hold, and is asserted here, is that the pair collapses wherever + /// the 200 is *not* available: for a **private** subagent the two refusals are + /// identical, because `session_reach` fires first and its one sentence answers + /// "private" and "no such chat" alike. + /// + /// ⚠ With privacy tiers OFF `session_reach` returns `Ok` before its store + /// read, so the private row joins the public one and the whole pair separates + /// again. That is the master switch's pre-existing blast radius, not this + /// route's, and it is not closed here. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn a_private_subagent_and_an_unknown_id_are_refused_in_the_same_words() { + install_test_user_action_key(); + let state = AppState::new().await.unwrap(); + let private_child = seed(&state, SessionType::SubAgent, true).await; + let public_child = seed(&state, SessionType::SubAgent, false).await; + let public_chat = seed(&state, SessionType::User, false).await; + + let (unknown_status, unknown_body) = + post_agent_route_response(Arc::clone(&state), "/agent/resume", "19700101_404").await; + let (private_status, private_body) = + post_agent_route_response(Arc::clone(&state), "/agent/resume", private_child.id()) + .await; + assert_eq!(unknown_status, StatusCode::FORBIDDEN); + assert_eq!(private_status, StatusCode::FORBIDDEN); + assert_eq!( + private_body, unknown_body, + "a private subagent is distinguishable from a nonexistent id by its refusal" + ); + assert!( + unknown_body.contains(crate::routes::session_reach::SESSION_OUT_OF_REACH), + "the shared refusal is no longer the reach sentence: {unknown_body}" + ); + + // The dominating disclosure, measured in the same run so the argument + // above cannot rot into an assumption. + let (public_child_status, public_child_body) = + post_agent_route_response(Arc::clone(&state), "/agent/resume", public_child.id()).await; + assert_eq!(public_child_status, StatusCode::FORBIDDEN); + assert!( + public_child_body.contains(SUBAGENT_USER_ACTION_REQUIRED), + "a public subagent is no longer told why it is refused: {public_child_body}" + ); + let (public_chat_status, _) = + post_agent_route_response(Arc::clone(&state), "/agent/resume", public_chat.id()).await; + assert_eq!( + public_chat_status, + StatusCode::OK, + "an unproven caller is refused an ordinary public chat, which would make the \ + subagent body the only existence signal and turn finding 4 into a real oracle" + ); + } + #[tokio::test(flavor = "multi_thread")] #[serial] async fn proven_user_cannot_drift_child_grants_and_can_stop_public_or_private_subagents() { diff --git a/crates/biorouter-server/src/routes/reply.rs b/crates/biorouter-server/src/routes/reply.rs index ed2823699..e8fdb7783 100644 --- a/crates/biorouter-server/src/routes/reply.rs +++ b/crates/biorouter-server/src/routes/reply.rs @@ -16,6 +16,10 @@ use biorouter::conversation::Conversation; use biorouter::privacy::SessionClassification; use biorouter::session::session_manager::ReplaceOutcome; use biorouter::session::SessionManager; +// Through the LIB path, as every route names the user-action digest: `src/routes/` +// is compiled into the `biorouterd` binary too, where `crate::auth` does not +// exist, and the digest must be the lib's one static. +use biorouter_server::auth::{user_action_proof, UserActionProof}; use bytes::Bytes; use futures::Stream; use rmcp::model::ServerNotification; @@ -1753,6 +1757,121 @@ pub struct InterruptAccepted { pub turn_id: String, } +/// The refusal `POST /interrupt` gives on a daemon that holds no user-action key. +/// +/// In the voice of `session_reach::SESSION_REACH_NO_KEY` and `routes::agent`'s +/// `SUBAGENT_CONTROL_NO_KEY`, and for their reason (SD-8): it is this daemon's +/// situation, not the caller's, and a person at a `biorouter serve` page must +/// not be sent hunting for a permission no one can be granted here. +/// +/// ⚠ **Never empty, and that is load-bearing.** `biorouter session attach` tells +/// a daemon that wants the proof apart from one that cannot check it by whether +/// a turn-control 403 carries a body (`session_watch::key_verdict`): an empty +/// body means "this daemon holds a key and wants the proof", and the terminal +/// prompts for one. A keyless daemon has no key to be typed, so its steer +/// refusal says so in words instead of being mistaken for a missing credential. +pub const STEER_NO_KEY: &str = + "This daemon was started without a user-action key, so it cannot verify that a request came \ + from the person at the keyboard, and steering a turn that is already running requires that \ + proof: it changes what the model is doing, in place, without the person watching it having \ + asked. Nothing was queued and the turn was not touched. Stop the turn and send the message \ + instead, or use the desktop app."; + +/// May this request stop or settle a turn in the chat it names? +/// +/// The gate of `POST /agent/cancel`, `POST /agent/continuation/abandon` and +/// `POST /agent/continuation/recover`, asked before any of them touches the +/// turn. **`POST /interrupt` is deliberately not one of them** — see +/// [`authorize_steer`], which says why the argument below does not reach it. +/// +/// * **A daemon that holds a user-action key** — the desktop application's — +/// takes the proof and nothing else, exactly as before; `Unproven` is the +/// empty 403 these routes have always answered. +/// * **A daemon that holds none** — the one `biorouter serve` starts (SD-7), or +/// a `biorouterd` started by hand — asks what `/agent/stop` asks there: +/// `routes::agent::authorize_agent_control`, the reach gate and then the +/// subagent rule. The same call, not a copy of it, so the two routes can never +/// disagree about who may stop a turn. +/// +/// ⚠ **Why the keyless arm no longer refuses** (serve decision SD-11). On such a +/// daemon the proof can only refuse everyone, the person at the browser +/// included: measured 2026-09-11, the Stop button answered 403 on every chat of +/// a `serve` host. And the refusal protected nothing. The proof is here so that +/// a model holding the daemon secret, which AR-11 found recoverable, cannot stop +/// another chat's turn; on a keyless daemon the same caller already stops that +/// turn through `/agent/stop` — literally this function's own keyless arm — or, +/// as a model, through `workspace_close { scope: "turn" }`. `/agent/stop` is +/// also strictly the more destructive of the two: it cancels the in-flight turn +/// **and** evicts the agent. Same authority, same scope, weaker effect, so +/// admitting these three gives no caller a capability it lacked. +/// +/// ⚠ **Why a daemon that holds a key is not relaxed with it.** There the proof +/// costs the person nothing — the renderer attaches it to every request — and it +/// is what licenses the `UserDirect` stamp a steer carries. +async fn authorize_turn_control( + state: &AppState, + session_id: &str, + headers: &HeaderMap, +) -> Result<(), axum::response::Response> { + match user_action_proof(headers) { + UserActionProof::Proven => Ok(()), + UserActionProof::Unproven => Err(StatusCode::FORBIDDEN.into_response()), + UserActionProof::NoKeyInstalled => { + crate::routes::agent::authorize_agent_control(state, session_id, headers) + .await + .map(|_| ()) + .map_err(IntoResponse::into_response) + } + } +} + +/// May this request put text into a turn that is **already running**? +/// +/// The gate of `POST /interrupt` alone, and the one place the four turn-control +/// routes part company. It asks for the user-action proof on every daemon, which +/// is what `main` asked before SD-11 and what this route keeps asking. +/// +/// ⚠ **Why the steer does not move with the Stop.** [`authorize_turn_control`]'s +/// keyless arm rests on a dominance argument: the caller already stops that turn +/// through `/agent/stop`, and already puts text in front of that chat's model +/// through `/reply`. The second half is false here. `/reply` takes the BR-33 +/// single-turn lock (`try_begin_turn_idempotent_with_continuation`) and answers +/// `409 CONFLICT` when a *different* turn is already running in that chat; +/// `/interrupt` answers `409` when none is. The two preconditions are disjoint, +/// so in the exact state where a steer lands, the route said to dominate it is +/// refused. What admitting it would add is therefore genuinely new, and worth +/// naming precisely: attacker-chosen text injected into a turn already in +/// flight, **without cancelling it**, which the person watching sees as their own +/// turn changing direction. Cancel-then-reply — the nearest thing a caller +/// holding only the daemon secret already has — kills the turn first, and is +/// therefore visible. This is a capability asymmetry, not a tier crossing: +/// `session_reach` still refuses a private chat to a public caller and +/// `refuse_subagent_unless_user` still refuses every subagent's chat. +/// +/// ⚠ **Why the keyless refusal is [`STEER_NO_KEY`] rather than an empty 403.** +/// `biorouter session attach` tells the two kinds of daemon apart by whether a +/// turn-control 403 carries a body (`session_watch::key_verdict`): an empty one +/// means "this daemon holds a key and wants the proof", and the terminal prompts +/// for it. A keyless daemon has no key to be typed, so refusing here with an +/// empty body would send a `biorouter serve` user hunting for a credential that +/// does not exist and then tell them it was the wrong one. The sentence keeps +/// the empty 403 meaning exactly what that reading needs it to mean. +/// +/// It takes no session id and asks nothing about the chat, so the refusal is +/// byte-for-byte the same for every chat — which keeps a route no proof can ever +/// satisfy from becoming a per-id oracle. +fn authorize_steer(headers: &HeaderMap) -> Result<(), axum::response::Response> { + match user_action_proof(headers) { + UserActionProof::Proven => Ok(()), + UserActionProof::Unproven => Err(StatusCode::FORBIDDEN.into_response()), + UserActionProof::NoKeyInstalled => Err(( + StatusCode::FORBIDDEN, + Json(serde_json::json!({ "message": STEER_NO_KEY })), + ) + .into_response()), + } +} + /// Soft interrupt: queue a user message to be injected into the session's /// running turn at the next safe loop boundary, instead of cancelling the turn /// and re-sending the whole context. Returns 202 Accepted; the message surfaces @@ -1779,7 +1898,9 @@ pub struct InterruptAccepted { responses( (status = 202, description = "Message queued for injection into the running turn", body = InterruptAccepted), (status = 400, description = "Empty message text"), - (status = 403, description = "The request was not proven to come from the user"), + (status = 403, description = "The request was not proven to come from the user; on a daemon \ + that holds no user-action key, steering is unavailable and the \ + refusal says so (SD-11)"), (status = 409, description = "No turn is accepting interrupts for this session"), (status = 500, description = "Internal server error") ) @@ -1788,12 +1909,10 @@ pub async fn interrupt( State(state): State>, headers: HeaderMap, Json(req): Json, -) -> Result<(StatusCode, Json), StatusCode> { - if !biorouter_server::auth::is_user_action(&headers) { - return Err(StatusCode::FORBIDDEN); - } +) -> Result<(StatusCode, Json), axum::response::Response> { + authorize_steer(&headers)?; if req.text.trim().is_empty() { - return Err(StatusCode::BAD_REQUEST); + return Err(StatusCode::BAD_REQUEST.into_response()); } if let Some(turn_id) = req.turn_id.clone() { let queued_message = crate::workspace::turn::stamp_user_direct_if_subagent( @@ -1817,18 +1936,22 @@ pub async fn interrupt( // Cheap early-out only: it avoids constructing an agent for an idle session. // It is no longer the guard — see `try_queue_soft_interrupt` below. if !state.is_turn_active(&req.session_id) { - return Err(StatusCode::CONFLICT); + return Err(StatusCode::CONFLICT.into_response()); } - // User-action authentication above is the authority for this attribution. - // Keep it independent of the session store: a live agent can legitimately - // outlast or race its durable row, but an accepted human steer must never - // lose its provenance because that auxiliary lookup failed. + // `authorize_steer` above is the authority for this attribution, and it + // admits none but `Proven` — which is why the stamp is unconditional here on + // every daemon. Keep it independent of the session store: a live agent can + // legitimately outlast or race its durable row, but an accepted human steer + // must never lose its provenance because that auxiliary lookup failed. let provenance = Some(biorouter::conversation::message::MessageProvenance { kind: biorouter::conversation::message::ProvenanceKind::UserDirect, from_session_id: None, from_session_name: None, }); - let agent = state.get_agent_for_route(req.session_id).await?; + let agent = state + .get_agent_for_route(req.session_id) + .await + .map_err(IntoResponse::into_response)?; match agent.try_queue_soft_interrupt(req.text, provenance) { Ok(turn_id) => Ok(( StatusCode::ACCEPTED, @@ -1838,7 +1961,7 @@ pub async fn interrupt( )), // #69: the turn the caller addressed has ended. Refusing is the honest // answer — queueing for whatever runs next is the bug this replaces. - Err(InterruptRefused::TurnEnded) => Err(StatusCode::CONFLICT), + Err(InterruptRefused::TurnEnded) => Err(StatusCode::CONFLICT.into_response()), } } @@ -2052,7 +2175,9 @@ const CANCEL_SETTLEMENT_TIMEOUT: Duration = Duration::from_secs(30); (status = 200, description = "Cancel processed; `cancelled` reports whether a turn was running", body = CancelTurnResponse), (status = 400, description = "Stop-and-Send requires an exact turn id and a valid stable continuation owner id"), (status = 401, description = "Unauthorized - invalid secret key"), - (status = 403, description = "The request was not proven to come from the user"), + (status = 403, description = "The request was not proven to come from the user; on a daemon \ + that holds no user-action key, the chat is out of the caller's \ + reach or is a subagent's (SD-11)"), (status = 409, description = "A different turn generation is active, another client owns the continuation, or its admission is still settling", body = CancelTurnConflict), (status = 504, description = "The cancelled turn did not release the session lock before the safety bound"), (status = 500, description = "Internal server error") @@ -2063,8 +2188,8 @@ pub async fn cancel_turn( headers: HeaderMap, Json(req): Json, ) -> axum::response::Response { - if !biorouter_server::auth::is_user_action(&headers) { - return StatusCode::FORBIDDEN.into_response(); + if let Err(refusal) = authorize_turn_control(&state, &req.session_id, &headers).await { + return refusal; } match cancel_turn_bounded(&state, &req, CANCEL_SETTLEMENT_TIMEOUT).await { Ok(response) => Json(response).into_response(), @@ -2079,7 +2204,9 @@ pub async fn cancel_turn( request_body = AbandonContinuationLeaseRequest, responses( (status = 200, description = "The continuation lease is abandoned or was already resolved", body = AbandonContinuationLeaseResponse), - (status = 403, description = "The request was not proven to come from the user"), + (status = 403, description = "The request was not proven to come from the user; on a daemon \ + that holds no user-action key, the chat is out of the caller's \ + reach or is a subagent's (SD-11)"), (status = 409, description = "The lease is invalid or belongs to another session", body = ContinuationLeaseErrorResponse) ) )] @@ -2088,8 +2215,8 @@ pub async fn abandon_continuation_lease( headers: HeaderMap, Json(req): Json, ) -> axum::response::Response { - if !biorouter_server::auth::is_user_action(&headers) { - return StatusCode::FORBIDDEN.into_response(); + if let Err(refusal) = authorize_turn_control(&state, &req.session_id, &headers).await { + return refusal; } match state.abandon_continuation_lease(&req.session_id, &req.continuation_lease) { Ok(resolution) => { @@ -2123,7 +2250,9 @@ fn valid_continuation_owner_id(owner_id: &str) -> bool { responses( (status = 200, description = "The exact pending continuation was taken over or its whole claim group was abandoned", body = RecoverContinuationResponse), (status = 400, description = "The continuation owner id is missing or invalid"), - (status = 403, description = "The session is out of reach or the request was not proven to come from the user"), + (status = 403, description = "The session is out of reach or the request was not proven to \ + come from the user; on a daemon that holds no user-action key, \ + the chat is out of the caller's reach or is a subagent's (SD-11)"), (status = 409, description = "The exact continuation generation was already resolved", body = ContinuationLeaseErrorResponse) ) )] @@ -2132,12 +2261,16 @@ pub async fn recover_continuation( headers: HeaderMap, Json(req): Json, ) -> axum::response::Response { - if !biorouter_server::auth::is_user_action(&headers) { - return StatusCode::FORBIDDEN.into_response(); + if let Err(refusal) = authorize_turn_control(&state, &req.session_id, &headers).await { + return refusal; } if !valid_continuation_owner_id(&req.continuation_owner_id) { return StatusCode::BAD_REQUEST.into_response(); } + // Asked again on a keyless daemon, where `authorize_turn_control` has already + // asked it: this is the route's own reach gate on EVERY daemon, and a second + // answer to the same question is the same answer. It stays where the + // ordering census (`session_reach::every_gated_route_resolves_…`) pins it. if let Err(refusal) = crate::routes::session_reach::session_reach( state.session_manager(), &req.session_id, diff --git a/crates/biorouter-server/src/routes/session_reach.rs b/crates/biorouter-server/src/routes/session_reach.rs index 09dbb875e..c665936fa 100644 --- a/crates/biorouter-server/src/routes/session_reach.rs +++ b/crates/biorouter-server/src/routes/session_reach.rs @@ -23,8 +23,11 @@ //! it can name — that is not a privacy boundary and this gate is deliberately //! inert there; //! * it still reaches every session-addressing route NOT on -//! [the gated list](self#the-gated-list). `POST /interrupt` and `POST -//! /agent/cancel` now require user-action proof; `GET +//! [the gated list](self#the-gated-list). `POST /agent/cancel` requires +//! user-action proof on a daemon that holds a key and is on the list on one +//! that does not (SD-11); `POST /interrupt` requires the proof on **either** +//! kind and so is on neither (SD-11a — `routes::reply::authorize_steer` says +//! why the steer did not move with the Stop); `GET //! /sessions/{id}/extensions`, `GET /sessions/{id}/usage`, `PUT //! /sessions/{id}/name`, `PUT /sessions/{id}/user_workflow_values` and //! `DELETE /sessions/{id}` remain open, as do `GET /active_work` and `POST @@ -146,18 +149,21 @@ //! | `POST /agent/add_extension` | Attaches tools to the session. | //! | `GET|POST /knowledge/active` | Reads or repoints the session's knowledge bases and write target. | //! | `POST /agent/resume` | Loads the session's stored conversation into a live agent. Gates directly, like the rows above. | +//! | `GET /agent/callable_tool_count` | Counts the named session's MODEL-FACING tools, and answers through `get_or_create_agent` — so it CREATES an agent for a session that has none. Added 2026-09-12 by the SD-8 review of #260, which found it with no gate of any kind; the renderer had merely stopped calling it. ⚠ Its sibling `GET /agent/tools` is **deliberately** not here: that one is the unfiltered permission-editor surface, so a person can administer private tools a public model cannot see. | //! | `POST /agent/continuation/recover` | Resumes a parked continuation in the named session. Gates directly. | //! | `POST /agent/update_from_session` | Adopts another session's provider configuration. Gates directly. | //! | `POST /agent/update_provider` · `restart` · `stop` · `remove_extension` | Gate through [`authorize_agent_control`](../agent/fn.authorize_agent_control.html), which calls [`session_reach`] and then reads the row. | +//! | `POST /agent/cancel` · `/agent/continuation/abandon` | Stop and settle the named session's turn. **On a daemon that holds no user-action key only** (serve decision SD-11): there `routes::reply::authorize_turn_control` gates them through the same `authorize_agent_control` as the row above, so a Stop admits exactly the callers `/agent/stop` does. A daemon that holds a key asks them for the proof instead, which reaches every chat. `POST /interrupt` is NOT here: it asks for the proof on both kinds of daemon, so it never reaches this gate — see `routes::reply::authorize_steer`. | //! -//! ⚠ **Two spellings, one list.** The last row reaches the gate through a helper -//! rather than by naming it, which is why a scan for the literal `session_reach(` -//! reports those four as ungated and why the ordering test below uses two of them -//! as over-read controls. They are NOT exempt — measured live, each answers 403 -//! without the capability header and proceeds with it. A future sweep that greps -//! for the call must follow `authorize_agent_control` too, or it will "discover" -//! four holes that are not there and, worse, trust the same grep when it reports -//! a real one. +//! ⚠ **Two spellings, one list.** The last two rows reach the gate through a +//! helper rather than by naming it, which is why a scan for the literal +//! `session_reach(` reports those six as ungated and why the ordering test +//! below uses two of them as over-read controls. They are NOT exempt — measured +//! live, each of the first four answers 403 without the capability header and +//! proceeds with it, and `tests/turn_control_no_user_key.rs` measures the other +//! two on a keyless daemon. A future sweep that greps for the call must follow +//! `authorize_agent_control` too, or it will "discover" six holes that are not +//! there and, worse, trust the same grep when it reports a real one. //! //! # Why `X-User-Action` and not a new mechanism, for the proof half //! @@ -1063,11 +1069,13 @@ mod tests { /// unit test — `AppState::new()` opens the developer's REAL session /// database. Every route on the list is also driven over HTTP by /// [`super::bypass_tests`] except `POST /agent/add_extension` (whose admitted - /// arm mints a real agent) and `GET|POST /knowledge/active` (a middleware, which + /// arm mints a real agent), `GET|POST /knowledge/active` (a middleware, which /// a body scan cannot see and /// [`super::bypass_tests::the_knowledge_active_gate_is_actually_wired`] - /// drives instead); this is what holds the ORDERING, which no status code - /// can show. + /// drives instead) and the two SD-11 turn-control routes, which are gated + /// only on a daemon with no user-action key and so are driven by their own + /// keyless binary, `tests/turn_control_no_user_key.rs`; this is what holds + /// the ORDERING, which no status code can show. /// /// ⚠ **Every route added to the gated list gets a row here.** `/export`, /// `/events` and `/diagnostics` each shipped a gate that this table did not @@ -1095,6 +1103,31 @@ mod tests { "recover_continuation_for_owner(", "the pending continuation ownership state", ), + // SD-11: the turn-control routes. Their gate is a helper, because on + // a daemon that holds a key the answer is the proof and on one that + // holds none it is `authorize_agent_control` — the ordering is the + // same either way, and it is what is asserted here. + ( + reply_rs, + "pub async fn recover_continuation", + "authorize_turn_control(", + "recover_continuation_for_owner(", + "the pending continuation ownership state", + ), + ( + reply_rs, + "pub async fn cancel_turn(", + "authorize_turn_control(", + "cancel_turn_bounded(", + "the turn registry, whose answer says whether this chat is busy", + ), + ( + reply_rs, + "pub async fn abandon_continuation_lease", + "authorize_turn_control(", + "state.abandon_continuation_lease(", + "the continuation registry", + ), ( session_rs, "async fn get_session(", @@ -1166,15 +1199,20 @@ mod tests { // are controls for the EXTRACTOR, not exemptions from the gate, and the // comment here said otherwise until 2026-09-04. `interrupt` and // `get_session_extensions` are the genuinely ungated pair: `interrupt` - // requires the user's proof instead, and `get_session_extensions` is on - // the module header's open residual. + // requires the user's proof instead — on a keyless daemon too, which is + // the one way it differs from the Stop beside it (SD-11a, + // `reply::authorize_steer`) — and `get_session_extensions` is on the + // module header's open residual. Two more reply.rs controls sit on + // either side of the five rows that file contributes. // // BOTH sides in `agent.rs`: `agent_remove_extension` sits after the two // gated handlers' neighbourhood and `update_agent_provider` before it, // and a control on one side only passes against an extractor that // over-reads towards the other. for (src, control) in [ + (reply_rs, "fn attach_names_a_missing_turn("), (reply_rs, "pub async fn interrupt"), + (reply_rs, "pub fn routes("), (session_rs, "async fn get_session_extensions"), (agent_rs, "async fn agent_remove_extension"), (agent_rs, "async fn update_agent_provider"), @@ -1187,11 +1225,16 @@ mod tests { (status_rs, "async fn system_info("), (status_rs, "pub fn routes("), ] { - assert!( - !body_of(src, control).contains("session_reach("), - "the body scan is over-reading: {control} is not on the gated list and \ - reported the gate" - ); + // Both spellings the rows above use, so a control is a control for + // every row it could be over-reading into. + let body = body_of(src, control); + for gate in ["session_reach(", "authorize_turn_control("] { + assert!( + !body.contains(gate), + "the body scan is over-reading: {control} is not on the gated list and \ + reported the gate (`{gate}`)" + ); + } } } diff --git a/crates/biorouter-server/tests/turn_control_no_user_key.rs b/crates/biorouter-server/tests/turn_control_no_user_key.rs new file mode 100644 index 000000000..d9b026d07 --- /dev/null +++ b/crates/biorouter-server/tests/turn_control_no_user_key.rs @@ -0,0 +1,618 @@ +//! SD-11: on a daemon that holds no proof-of-user key — the one `biorouter +//! serve` starts (SD-7), or a `biorouterd` started by hand — stopping and +//! settling a turn admit exactly the callers `POST /agent/stop` already admits +//! there: a chat the caller can reach, and never a subagent's. +//! +//! Measured on 2026-09-11 from a browser page on a real `biorouter serve`: +//! `POST /agent/cancel` and `POST /interrupt` both answered 403 with an empty +//! body, capability header and all, because each began with an unconditional +//! `is_user_action` check that a keyless daemon can never pass. The browser's +//! Stop button and mid-turn steering could not work on any `serve` host, one +//! configured with a public model included. +//! +//! ⚠ **Three of those four moved; `POST /interrupt` did not** (SD-11a, the +//! security review's correction). The keyless arm rests on a dominance +//! argument — the same caller already stops that turn through `/agent/stop`, +//! and already puts text in front of that chat's model through `/reply` — and +//! for the steer that argument is false: `/reply` is refused `409` by the BR-33 +//! single-turn lock in the exact state where a steer is meaningful, so the +//! dominating route cannot reach it. `/interrupt` therefore still asks for the +//! proof on every daemon, and a keyless one refuses it **in words**, which is +//! what keeps `biorouter session attach` from asking for a key that does not +//! exist. Both halves are pinned below. +//! +//! ⚠ **Its own test binary on purpose**, for the reason `approval_no_user_key.rs` +//! gives: the installed digest is a process-global `OnceLock`, the lib's tests +//! install one, and inside that binary the keyless state is unreachable once the +//! first of them wins. Nothing here installs a digest — which is exactly how +//! `biorouter serve` starts its daemon. What a daemon that DOES hold a key does +//! with the same requests is pinned by the lib's own tests +//! (`routes::reply`'s `cancel_without_user_action_proof_cannot_stop_another_turn` +//! and `interrupt_without_user_action_proof_cannot_forge_human_steering`), and +//! this change leaves it alone. + +// Redirects this binary's Biorouter data/config/state dirs at a throwaway root +// before `main`, so nothing here can open the developer's real `sessions.db`. +#[path = "../src/test_sandbox.rs"] +mod test_sandbox; + +use std::sync::Arc; +use std::time::Duration; + +use axum::body::Body; +use axum::http::{HeaderMap, Request, StatusCode}; +use axum::Router; +use biorouter::agents::{Drained, TurnId}; +use biorouter::model::ModelConfig; +use biorouter::privacy::SessionClassification; +use biorouter::session::session_manager::SessionType; +use biorouter_server::auth::{user_action_proof, UserActionProof}; +use biorouter_server::routes::reply::STEER_NO_KEY; +use biorouter_server::routes::session_reach::SESSION_REACH_NO_KEY; +use biorouter_server::state::AppState; +use serde_json::{json, Value}; +use serial_test::serial; +use tokio_util::sync::CancellationToken; +use tower::ServiceExt; + +/// Every test here stands on this: the daemon under test holds no key. +fn assert_the_daemon_is_keyless() { + assert_eq!( + user_action_proof(&HeaderMap::new()), + UserActionProof::NoKeyInstalled, + "something in this binary installed a user-action digest, so these tests would be \ + measuring a desktop daemon rather than a `biorouter serve` one" + ); + assert!( + biorouter::privacy::privacy_tiers_enabled(), + "privacy tiers are off, so the reach gate below would admit everything either way" + ); +} + +/// The chats these tests stop and steer. Every one is a real row, because the +/// keyless arm reads the row (reach, then the subagent rule) before it touches +/// the turn — unlike the lib's tests, which name ids that were never created. +#[derive(Clone, Copy, Debug)] +enum Chat { + /// An ordinary chat on a public model: what a browser on a `serve` host + /// configured with a public model opens. + Public, + /// An ordinary chat a private model's turn has ratcheted. + Private, + /// A delegated child's session. + Subagent, +} + +async fn seed(state: &Arc, chat: Chat) -> String { + let manager = state.session_manager(); + let session_type = match chat { + Chat::Subagent => SessionType::SubAgent, + Chat::Public | Chat::Private => SessionType::User, + }; + let session = manager + .create_session( + std::env::temp_dir(), + format!("SD-11 {chat:?} (test fixture)"), + session_type, + ) + .await + .unwrap(); + if matches!(chat, Chat::Private) { + // Raised the way a real chat gets there — a turn on a private provider — + // rather than by writing the column, so what is refused here is the + // state a user's own chat reaches. + manager + .update(&session.id) + .provider_name("versa_azure") + .model_config(ModelConfig::new("gpt-4o").unwrap()) + .raise_privacy(SessionClassification::Private, "turn:versa_azure") + .apply() + .await + .unwrap(); + } + session.id +} + +async fn discard(state: &Arc, session_id: &str) { + // The tests run serially, so every cached agent is this test's. + state.clear_cached_agents().await; + let _ = state.session_manager().delete_session(session_id).await; +} + +/// The headers a browser tab sends on a `serve` daemon: the page's shim answers +/// the user-action key with an empty string, and a caller that runs under a +/// private model states it in `X-Caller-Provider`, as `biorouter session` does +/// from a terminal. +fn post(uri: &str, body: Value, caller_provider: Option<&str>) -> Request { + let mut request = Request::builder() + .uri(uri) + .method("POST") + .header("content-type", "application/json") + .header("X-User-Action", ""); + if let Some(provider) = caller_provider { + request = request.header("X-Caller-Provider", provider); + } + request.body(Body::from(body.to_string())).unwrap() +} + +async fn send(app: Router, request: Request) -> (StatusCode, String) { + let response = app.oneshot(request).await.unwrap(); + let status = response.status(); + let bytes = tokio::time::timeout( + Duration::from_secs(60), + axum::body::to_bytes(response.into_body(), usize::MAX), + ) + .await + .expect("the response body did not finish within a minute") + .unwrap(); + (status, String::from_utf8_lossy(&bytes).into_owned()) +} + +fn reply_routes(state: &Arc) -> Router { + biorouter_server::routes::reply::routes(Arc::clone(state)) +} + +fn json_of(body: &str) -> Value { + serde_json::from_str(body).unwrap_or_else(|_| panic!("not JSON: {body}")) +} + +/// Begin a turn on `session_id` the way `/reply` does, holding its lock. +fn begin_turn( + state: &Arc, + session_id: &str, +) -> (biorouter_server::state::TurnGuard, CancellationToken) { + let token = CancellationToken::new(); + let guard = state + .try_begin_turn_idempotent(session_id, token.clone(), None) + .expect("the turn lock is free"); + (guard, token) +} + +/// A Stop, in the plain form a script sends. The browser's store +/// (`requestExactTurnSettlement` in `chatStreamStore.tsx`) also names the exact +/// generation and waits for it to settle; the Stop-and-Send test below sends +/// that fuller body. +fn stop_request(session_id: &str, caller_provider: Option<&str>) -> Request { + post( + "/agent/cancel", + json!({ "session_id": session_id }), + caller_provider, + ) +} + +fn steer_request(session_id: &str, text: &str, caller_provider: Option<&str>) -> Request { + post( + "/interrupt", + json!({ "session_id": session_id, "text": text, "turn_id": "browser-steer-1" }), + caller_provider, + ) +} + +/// The failure the QA run measured, as its own regression test: the browser's +/// Stop, on an ordinary public chat, on a daemon with no key. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_keyless_daemon_stops_a_turn_in_a_chat_the_caller_can_reach() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Public).await; + let (guard, token) = begin_turn(&state, &id); + + let (status, body) = send(reply_routes(&state), stop_request(&id, None)).await; + + assert_eq!( + status, + StatusCode::OK, + "the browser's Stop was refused on a keyless daemon: {body}" + ); + let body = json_of(&body); + assert_eq!(body["cancelled"], json!(true), "{body}"); + assert_eq!(body["turn_id"], json!(guard.turn_id()), "{body}"); + assert!(token.is_cancelled(), "a 200 that did not trip the turn"); + + drop(guard); + discard(&state, &id).await; +} + +/// **The one of the four that did not move, and the shape of its refusal.** +/// +/// ⚠ **Why the steer is excluded.** SD-11's keyless arm is admitted on a +/// dominance argument: the caller already stops that turn through `/agent/stop` +/// and already puts text in front of that chat's model through `/reply`. The +/// second half does not hold here. `/reply` takes the BR-33 single-turn lock +/// (`try_begin_turn_idempotent_with_continuation`) and answers `409` for a +/// *different* turn while one is running; `/interrupt` answers `409` when none +/// is. The two preconditions are disjoint, so in the exact state where a steer +/// lands, the route said to dominate it is refused. What admitting it would add +/// is genuinely new: attacker-chosen text injected into a turn already in +/// flight, without cancelling it, indistinguishable in the transcript from what +/// the person watching typed. Cancel-then-reply — the nearest thing a caller +/// holding only the daemon secret already has — kills the turn first, and is +/// therefore visible. +/// +/// ⚠ **And why the refusal carries a sentence rather than being empty.** +/// `biorouter session attach` tells a daemon that wants the proof apart from one +/// that cannot check it by whether a turn-control 403 has a body +/// (`session_watch::key_verdict`): empty means "this daemon holds a key", and it +/// prompts for one. A keyless daemon has no key to be typed, so an empty refusal +/// here would send a `serve` user hunting for a credential that does not exist. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_keyless_daemon_refuses_the_steer_it_admits_the_stop_for() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Public).await; + let (guard, token) = begin_turn(&state, &id); + // #69: acceptance is the agent loop's to give, so the agent must be in the + // state a running loop puts it in; the turn lock alone is not enough. + let agent = state.get_agent(id.clone()).await.unwrap(); + agent.open_for_turn(TurnId::new("keyless-agent-turn")); + + let (status, body) = send( + reply_routes(&state), + steer_request(&id, "actually, use R", None), + ) + .await; + + assert_eq!( + status, + StatusCode::FORBIDDEN, + "a keyless daemon injected text into a running turn: {body}" + ); + assert!( + body.contains(STEER_NO_KEY), + "the steer refusal must say it is this daemon that cannot check a proof, and must not \ + be empty — an empty turn-control 403 is how `biorouter session attach` decides to \ + prompt for a user-action key: {body:?}" + ); + assert!( + !agent.has_soft_interrupts(), + "a refused steer reached the agent's queue" + ); + assert!(matches!(agent.close_and_drain(), Drained::Empty)); + + // …while the Stop the same caller aims at the same turn is admitted. This + // pairing is the finding: three of the four routes moved and this one did + // not, so the difference is asserted in one place rather than inferred from + // two tests that could drift apart. + let (status, body) = send(reply_routes(&state), stop_request(&id, None)).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert!(token.is_cancelled(), "a 200 that did not trip the turn"); + + drop(guard); + discard(&state, &id).await; +} + +/// The steer refusal is the SAME refusal for every chat, because the proof is +/// asked for before anything reads the row. +/// +/// Worth its own assertion rather than being left implicit: `session_reach` +/// takes care to answer a private chat and a nonexistent one identically so that +/// a refusal is not a per-id oracle, and a steer gate that refused three kinds of +/// chat in three different ways would rebuild exactly that oracle on a route +/// where no proof can ever be offered. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_keyless_steer_refusal_says_the_same_thing_about_every_chat() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let mut answers = Vec::new(); + for chat in [Chat::Public, Chat::Private, Chat::Subagent] { + let id = seed(&state, chat).await; + let (guard, token) = begin_turn(&state, &id); + for caller in [None, Some("versa_azure")] { + let (status, body) = send( + reply_routes(&state), + steer_request(&id, "pretend the user said this", caller), + ) + .await; + assert!(!token.is_cancelled(), "a refused steer reached the turn"); + answers.push((format!("{chat:?}/{caller:?}"), status, body)); + } + drop(guard); + discard(&state, &id).await; + } + // A chat that was never created, as the control: reach answers this one and + // a private chat identically, and so must the steer gate. + let (status, body) = send( + reply_routes(&state), + steer_request("no-such-session", "pretend the user said this", None), + ) + .await; + answers.push(("absent/None".to_string(), status, body)); + + let (_, first_status, first_body) = &answers[0]; + for (label, status, body) in &answers { + assert_eq!(status, first_status, "{label}: {body}"); + assert_eq!(body, first_body, "{label}"); + } + assert_eq!(*first_status, StatusCode::FORBIDDEN); + assert!(first_body.contains(STEER_NO_KEY), "{first_body}"); +} + +/// Stop-and-Send mints a continuation lease, and a live lease blocks every turn +/// that does not present it. So a keyless daemon that let the cancel through and +/// refused the abandon would wedge the chat the first time a user removed a +/// queued message — this is why the settle routes move with the cancel. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn stop_and_send_settles_and_its_lease_can_be_abandoned_on_a_keyless_daemon() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Public).await; + let (guard, token) = begin_turn(&state, &id); + let turn_id = guard.turn_id().to_string(); + + let cancel = tokio::spawn(send( + reply_routes(&state), + post( + "/agent/cancel", + json!({ + "session_id": id, + "expected_turn_id": turn_id, + "wait_for_idle": true, + "continuation_pending": true, + "continuation_owner_id": "browser-window-a", + }), + None, + ), + )); + tokio::time::timeout(Duration::from_secs(10), async { + while !token.is_cancelled() && !cancel.is_finished() { + tokio::task::yield_now().await; + } + }) + .await + .expect("the Stop-and-Send request neither tripped the turn nor answered"); + // The turn unwinds: its guard retires, which is what settles the cancel. + drop(guard); + let (status, body) = cancel.await.unwrap(); + assert_eq!( + status, + StatusCode::OK, + "Stop-and-Send was refused on a keyless daemon: {body}" + ); + let body = json_of(&body); + assert_eq!(body["settled"], json!(true), "{body}"); + let lease = body["continuation_lease"] + .as_str() + .unwrap_or_else(|| panic!("Stop-and-Send returned no lease: {body}")) + .to_string(); + + assert!( + state + .try_begin_turn_idempotent(&id, CancellationToken::new(), None) + .is_err(), + "a live continuation lease must hold the chat for its replacement" + ); + + let (status, body) = send( + reply_routes(&state), + post( + "/agent/continuation/abandon", + json!({ "session_id": id, "continuation_lease": lease }), + None, + ), + ) + .await; + assert_eq!( + status, + StatusCode::OK, + "the lease a keyless cancel minted could not be abandoned, so the chat is wedged: {body}" + ); + assert_eq!(json_of(&body)["resolution"], json!("abandoned")); + let successor = state + .try_begin_turn_idempotent(&id, CancellationToken::new(), None) + .expect("an abandoned lease must release the chat"); + + drop(successor); + discard(&state, &id).await; +} + +/// A reload between the cancel and the replacement: the window asks for its +/// pending continuation back by its owner id, or gives the group up. Refusing +/// this on a keyless daemon strands the same live lease the abandon route +/// above releases. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_keyless_daemon_hands_a_pending_continuation_back_to_its_window() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Public).await; + // A generation that has already retired mints its lease at once, with no + // settlement to wait for — the reload case, where the turn is long gone. + let (retired, _token) = begin_turn(&state, &id); + let retired_id = retired.turn_id().to_string(); + drop(retired); + let (status, body) = send( + reply_routes(&state), + post( + "/agent/cancel", + json!({ + "session_id": id, + "expected_turn_id": retired_id, + "wait_for_idle": true, + "continuation_pending": true, + "continuation_owner_id": "browser-window-b", + }), + None, + ), + ) + .await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert!(json_of(&body)["continuation_lease"].is_string(), "{body}"); + + let recover = |action: &str| { + post( + "/agent/continuation/recover", + json!({ + "session_id": id, + "superseded_turn_id": retired_id, + "continuation_owner_id": "browser-window-b", + "action": action, + }), + None, + ) + }; + let (status, body) = send(reply_routes(&state), recover("take_over")).await; + assert_eq!( + status, + StatusCode::OK, + "a reloaded window could not take its continuation back on a keyless daemon: {body}" + ); + let body = json_of(&body); + assert_eq!(body["resolution"], json!("taken_over"), "{body}"); + assert!(body["continuation_lease"].is_string(), "{body}"); + + let (status, body) = send(reply_routes(&state), recover("abandon")).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(json_of(&body)["resolution"], json!("abandoned")); + let successor = state + .try_begin_turn_idempotent(&id, CancellationToken::new(), None) + .expect("an abandoned group must release the chat"); + + drop(successor); + discard(&state, &id).await; +} + +/// Reach decides a private chat, exactly as it decides reading one: a caller +/// whose stated capability covers it is admitted, one that states nothing is +/// refused with the keyless daemon's own sentence, and the refused request +/// touches neither the turn nor the agent's queue. +/// +/// Stop only. The steer never reaches reach on this daemon — its own gate +/// refuses it first, identically for every chat, which is +/// `a_keyless_steer_refusal_says_the_same_thing_about_every_chat` above. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_private_chat_is_stopped_only_by_a_caller_whose_capability_covers_it() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Private).await; + let (guard, token) = begin_turn(&state, &id); + let agent = state.get_agent(id.clone()).await.unwrap(); + agent.open_for_turn(TurnId::new("private-agent-turn")); + + for request in [ + stop_request(&id, None), + // A tier is not a provider: the daemon resolves the NAME against its own + // registry, and a spelled-out tier resolves Public. + stop_request(&id, Some("private")), + ] { + let (status, body) = send(reply_routes(&state), request).await; + assert_eq!(status, StatusCode::FORBIDDEN, "{body}"); + assert!( + body.contains(SESSION_REACH_NO_KEY), + "refused, but not by the reach gate's keyless sentence: {body}" + ); + } + assert!(!token.is_cancelled(), "a refused Stop reached the turn"); + assert!( + !agent.has_soft_interrupts(), + "a refused request reached the agent's queue" + ); + + let (status, body) = send(reply_routes(&state), stop_request(&id, Some("versa_azure"))).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert!(token.is_cancelled()); + + drop(guard); + discard(&state, &id).await; +} + +/// A subagent's tab stays the person's, on this daemon as on every other: the +/// parent is told when a human intervened in its child, and only the proof can +/// establish that one did. `/reply` and `/agent/stop` refuse the same caller +/// there already. The refusal names this daemon's situation rather than +/// telling a person at the keyboard to go and prove they are one (SD-8). +/// +/// The Stop is refused by the subagent rule inside `authorize_agent_control`; +/// the steer is refused one step earlier, by its own gate, which refuses every +/// chat on a keyless daemon. Both sentences open the same way, which is what +/// this asserts — a person is told about the daemon either way. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_subagents_turn_is_still_refused_and_the_refusal_says_why() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + let id = seed(&state, Chat::Subagent).await; + let (guard, token) = begin_turn(&state, &id); + let agent = state.get_agent(id.clone()).await.unwrap(); + agent.open_for_turn(TurnId::new("child-agent-turn")); + + for request in [ + stop_request(&id, None), + steer_request(&id, "pretend the user said this", None), + stop_request(&id, Some("versa_azure")), + ] { + let (status, body) = send(reply_routes(&state), request).await; + assert_eq!(status, StatusCode::FORBIDDEN, "{body}"); + assert!( + body.contains("without a user-action key"), + "a keyless daemon's refusal must say it is this daemon, not the caller, that \ + cannot prove a person acted: {body}" + ); + } + assert!( + !token.is_cancelled(), + "a refused Stop reached a child's turn" + ); + assert!( + !agent.has_soft_interrupts(), + "a refused steer reached a child's queue" + ); + + drop(guard); + discard(&state, &id).await; +} + +/// **The premise SD-11 stands on, pinned.** On a keyless daemon `/agent/stop` +/// already cancels a running turn for exactly the callers turn control now +/// admits — reach, and never a subagent's chat — so admitting them to +/// `/agent/cancel` gives nothing holding the daemon secret a capability it did +/// not have. If `/agent/stop` is ever tightened on such a daemon, this test +/// fails, and the ruling has to be re-argued rather than silently outlived. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_keyless_cancel_admits_exactly_the_callers_agent_stop_already_admits() { + assert_the_daemon_is_keyless(); + let state = AppState::new().await.unwrap(); + for (chat, caller, admitted) in [ + (Chat::Public, None, true), + (Chat::Private, None, false), + (Chat::Private, Some("versa_azure"), true), + (Chat::Subagent, None, false), + (Chat::Subagent, Some("versa_azure"), false), + ] { + let mut verdicts = Vec::new(); + for route in ["/agent/stop", "/agent/cancel"] { + let id = seed(&state, chat).await; + // `/agent/stop` evicts the agent, and answers 404 when there is none + // to evict; give it one so its answer is about the gate alone. + state.get_agent(id.clone()).await.unwrap(); + let (guard, token) = begin_turn(&state, &id); + let app = if route == "/agent/stop" { + biorouter_server::routes::agent::routes(Arc::clone(&state)) + } else { + reply_routes(&state) + }; + let (status, body) = send(app, post(route, json!({ "session_id": id }), caller)).await; + assert_eq!( + status.is_success(), + token.is_cancelled(), + "{route} on a {chat:?} chat answered {status} but the turn's cancellation \ + disagrees: {body}" + ); + verdicts.push((route, status, token.is_cancelled())); + drop(guard); + discard(&state, &id).await; + } + for (route, status, cancelled) in &verdicts { + assert_eq!( + *cancelled, admitted, + "{route} on a {chat:?} chat with caller {caller:?} answered {status}; expected \ + admitted={admitted}. All verdicts: {verdicts:?}" + ); + } + } +} diff --git a/crates/biorouter/tests/privacy_guard_wiring.rs b/crates/biorouter/tests/privacy_guard_wiring.rs index bb42dea5c..127aa4131 100644 --- a/crates/biorouter/tests/privacy_guard_wiring.rs +++ b/crates/biorouter/tests/privacy_guard_wiring.rs @@ -318,11 +318,24 @@ const REGISTRY: &[Guard] = &[ // read as refs-only and stand out. Site { file: "crates/biorouter-server/src/routes/agent.rs", - counts: c(4, 4, 0), + counts: c(5, 5, 0), kind: SiteKind::Guard, - what: "`POST /agent/resume`, `POST /agent/update_from_session`, and `POST \ - /agent/update_working_dir`, plus the shared `authorize_agent_control` \ - gate used by provider, extension, stop, and restart mutations", + what: "`POST /agent/resume`, `POST /agent/update_from_session`, `POST \ + /agent/update_working_dir` and `GET /agent/callable_tool_count`, plus \ + the shared `authorize_agent_control` gate used by provider, extension, \ + stop, and restart mutations. On a \ + daemon that holds no user-action key that same gate is also the \ + turn-control gate of `/agent/cancel` and the two continuation routes \ + (SD-11), reached from `routes::reply`'s `authorize_turn_control` by \ + name — so SD-11 added no call here. `/interrupt` keeps the proof on \ + every daemon and reaches neither gate. \ + ⚠ The count went 4 → 5 on 2026-09-12, and the justification is a \ + ROUTE that had no gate at all rather than a second gate on a guarded \ + one: an SD-8 review of #260 found `callable_tool_count` answering any \ + caller that could name a chat, through `get_or_create_agent`, which \ + CREATES an agent for a session that has none — so the fix is the same \ + reach gate, placed before the fetch for the reason \ + `agent_add_extension` states at its own", }, Site { file: "crates/biorouter-server/src/routes/mod.rs", @@ -337,7 +350,11 @@ const REGISTRY: &[Guard] = &[ kind: SiteKind::Guard, what: "`POST /reply`, which runs an agent turn with tools inside the named \ session, plus the explicit continuation takeover and group-abandon \ - recovery mutation", + recovery mutation. `/agent/cancel` and `/agent/continuation/abandon` reach \ + the gate only on a keyless daemon, and through `authorize_agent_control` \ + in `routes/agent.rs` (SD-11), which is why they add nothing to this count; \ + `/interrupt` keeps the user-action proof on every daemon and reaches no \ + reach gate at all", }, Site { file: "crates/biorouter-server/src/routes/session.rs", diff --git a/docs/agent-loop/subagents.md b/docs/agent-loop/subagents.md index faf6ca9d4..2d7d8c993 100644 --- a/docs/agent-loop/subagents.md +++ b/docs/agent-loop/subagents.md @@ -52,6 +52,8 @@ Three things you can do from that tab: - **Steer.** Type into the tab's ordinary composer. While the child's turn is running your message is injected as a mid-turn correction ("stop at step 3 and summarise"); between turns it starts a new turn or leaves a note. Either way it is labelled in the transcript as a **direct user message**, permanently. - **Stop.** The header's Stop control cancels the child's turn. The parent's tool call then resolves promptly, carrying whatever the child had produced — it is not left hanging. What it resolves *as* depends on how far the child got: a child stopped mid-tool-call, with no text to show for it, comes back `incomplete`; a child that had already written a summary returns that summary and can still be labelled `completed`, because the envelope is classified from the transcript rather than from the fact of the cancellation. Do not read `completed` as proof the child finished on its own. +> **Watch is the only one of the three in a browser.** On a page served by `biorouter serve` the tab has no composer and no Stop: steering and stopping a subagent need proof that a person acted, which only the desktop application holds. The tab says so in place of both controls rather than refusing on click — see [Browser access](../deployment/browser-access.md#what-a-browser-can-and-cannot-do). + **Closing the tab never kills the child.** That is the same rule as every other tab in BioRouter: closing is a view operation. Stop is the only kill switch, and a child whose tab you closed is still reachable from History. If you typed into the tab, the parent is told. Its tool result carries `human_intervened` and gains a line — *"Note: the user intervened directly in this subagent's tab during the run."* — so it weighs the child's self-report accordingly instead of assuming an untouched run. Nothing is said when you did not: silence there would read as a claim that someone checked. diff --git a/docs/deployment/browser-access.md b/docs/deployment/browser-access.md index 44526d471..17801317c 100644 --- a/docs/deployment/browser-access.md +++ b/docs/deployment/browser-access.md @@ -214,6 +214,9 @@ differs: |---|---| | Chat, sessions, history, extensions, skills, knowledge bases, workflows | Work as they do in the desktop application. | | Workspace control, several conversations at once, live app agents | Work — these are WebSocket-backed daemon routes, reached on the same origin. | +| Stopping a response, Stop and send | Work in an ordinary chat ([SD-11](serve-decisions.md#sd-11--stop-works-on-a-daemon-with-no-key-steering-does-not-and-a-subagents-tab-stays-the-persons)). | +| Steering a response while it runs | **Not available.** Injecting text into a turn that is already running needs proof that a person acted, which only the desktop application holds. What you type is queued instead and sent when the turn ends, so nothing is lost — it simply does not redirect the answer in flight. | +| A delegated subagent's own tab | **Read-only, and it says so before you try.** The tab shows the subagent's conversation and follows it while it works, but has no composer and no Stop button: sending to a subagent, steering it and stopping it need proof that a person acted, which only the desktop application holds. A note takes the composer's place, and one line takes the Stop button's ([SD-8](serve-decisions.md#sd-8--a-control-that-can-never-work-here-says-so-rather-than-failing-on-click)). | | Model and provider selection | **Not available.** See [The model is fixed before you start](#the-model-is-fixed-before-you-start). | | File and folder pickers | No native dialog. You type a path, and it is a path **on the machine running the daemon**, not on the machine holding the browser. | | Artifacts and diagnostics bundles | The artifact side panel works as usual. Opening an artifact outside the panel opens a new tab; a diagnostics bundle downloads as a file. | diff --git a/docs/deployment/programmatic-session-access.md b/docs/deployment/programmatic-session-access.md index 7d976e883..281f8954a 100644 --- a/docs/deployment/programmatic-session-access.md +++ b/docs/deployment/programmatic-session-access.md @@ -175,6 +175,7 @@ one of them resolves the target's tier **before** it touches the session, so a r | `POST /agent/update_working_dir` | Repoints the session at a directory. | | `POST /agent/add_extension` · `remove_extension` | Attaches or detaches tools. | | `GET`/`POST /knowledge/active` | Reads or repoints the session's knowledge bases. | +| `POST /agent/cancel` · `POST /agent/continuation/abandon` · `recover` | Stops or settles the session's running turn — **on a daemon that holds no user-action key only**, such as `biorouter serve` or a hand-run `biorouterd` ([SD-11](serve-decisions.md#sd-11--stop-works-on-a-daemon-with-no-key-steering-does-not-and-a-subagents-tab-stays-the-persons)). There they admit exactly the callers `POST /agent/stop` admits, a subagent's session excepted. `POST /interrupt` is **not** among them: it takes the proof on either kind of daemon, because injecting text into a turn already running is the one thing no other route on a keyless daemon can do. | ## What the header does *not* cover @@ -186,7 +187,8 @@ it would be wrong: | Route | What guards it instead | |---|---| -| `POST /interrupt`, `POST /agent/cancel`, `POST /agent/continuation/abandon` | `X-User-Action` — steering a turn is the user's decision, not a capability. | +| `POST /interrupt` | `X-User-Action` on **every** daemon — steering a turn that is already running is the user's decision, not a capability, and no other route on a keyless daemon reaches into a turn in flight. A keyless daemon's refusal says so in words rather than with an empty `403`. | +| `POST /agent/cancel`, `POST /agent/continuation/abandon` · `recover` | On a daemon that holds a user-action key — the desktop application's — `X-User-Action` and nothing else. A daemon that holds no key cannot check the proof at all, so these routes honour the header there instead (the table above). | | `POST /sessions/{id}/declassify`, `POST /sessions/{id}/diverge`, `POST /sessions/{id}/edit_message` | `X-User-Action` — these change or copy a classification, which no model may decide. | | `POST /agent/cross_affiliation_grant`, `POST /action-required/tool-confirmation` | `X-User-Action`, plus a decision-authority check on the resolving surface. | | `POST /knowledge/bases/{id}/ingest-conversation` | Its own Gate G: capability is derived from the model named in the request body, and every selected conversation is checked against it before a transcript is rendered. | @@ -233,8 +235,11 @@ not evidence the session exists. **`403` saying the daemon was started without a user-action key.** A different state: this daemon holds no key with which to verify a human, which is normal for `just run-server`, a hand-run `biorouterd agent`, and `biorouter serve`. The capability header is unaffected and still admits a -private chat on that daemon — a capable caller never reaches this message, because capability is -checked before proof. If you are seeing it, your caller is public. +private chat on that daemon — a capable caller never reaches the reach gate's version of this +message, because capability is checked before proof. If you are seeing it, your caller is public, +or the session is a delegated subagent's: changing, stopping or steering a subagent from its tab +needs proof that a person acted, whatever the capability, and that refusal says so in its own +words. **The header worked yesterday and now does not.** The tier came from the daemon's own registry, so a provider removed from `config.yaml`, or renamed, now resolves public. diff --git a/docs/deployment/serve-decisions.md b/docs/deployment/serve-decisions.md index 24b74e3a5..c62d8832b 100644 --- a/docs/deployment/serve-decisions.md +++ b/docs/deployment/serve-decisions.md @@ -18,7 +18,8 @@ This page records the decisions that replaced that arrangement. They were taken several of them only make sense as a set: the reason a browser session cannot switch models (SD-1) is also the reason it needs no proof-of-user mechanism, which is the reason the daemon can be spawned with a closed stdin (SD-7) — and the reason every control that needs that proof -must say so before the user reaches for it (SD-8). Read [the architecture](serve-architecture.md) +must say so before the user reaches for it (SD-8), and the reason the controls that stop and settle +a turn answer to the reach gate there instead (SD-11). Read [the architecture](serve-architecture.md) for how the result is built, and [browser access](browser-access.md) for how to use it. Records are identified `SD-n` — *serve decision*. The numbering is stable; a superseded record @@ -222,6 +223,91 @@ Allow/Deny buttons; and the agent is not offered tools whose only path runs thro approval — the three skill mutations and the extension manager's install and delete are withheld from the advertised roster when no person is reachable. +**A delegated subagent's tab is the same case** (2026-09-11; SD-11 recorded it as open). A +subagent's chat is where the proof decides everything — a message there is recorded as a person +intervening, and the parent is told so — and the daemon refuses these writes to it from a caller +that cannot prove a person acted: `POST /reply`, `POST /agent/cancel` and the two continuation +routes SD-11 admits elsewhere, `POST /interrupt` (which asks for the proof on every daemon, so it +refuses here for its own reason rather than SD-11's), `POST /agent/stop`, the extension routes, +`POST /agent/update_working_dir`, and `POST /agent/resume` itself. On a `serve` daemon that is every caller, so from the tab's own controls +there is nothing to do but read. + +⚠ **That is an enumeration, and it must not be read as "every write".** It said "every write" for +one day and was wrong on its own terms: `POST /agent/update_working_dir` — which repoints a chat at +a directory of the caller's choosing and restarts its agent there — consulted only +[`session_reach`](../security/privacy-tiers.md), the privacy gate, which is *deliberately inert for +a public session*, and a delegated child's chat is normally public. It is on the list above because +it was gated (2026-09-12); the claim is stated as a list because the sweep that found it found +eight more session-addressing writes that reach a child's row and ask nothing about it — `DELETE +/sessions/{id}` (which cancels the child's in-flight turn before deleting, so it is a Stop by +another name), `PUT /sessions/{id}/name`, `PUT /sessions/{id}/user_workflow_values`, `POST +/sessions/{id}/edit_message`, `POST /sessions/{id}/diverge`, `POST /agent/call_tool` and `POST +/agent/read_resource`. All predate this record. What SD-8 is about is the tab's own controls; a +sentence claiming the API surface as a whole is closed would be [#47](https://github.com/BaranziniLab/biorouter/issues/47)'s +claim to make, and #47 is open. + +**`GET /agent/callable_tool_count` was gated in the same pass**, and for a reason worth separating +from the tab: the renderer stopped *calling* it for a subagent's chat (below), and a client that +avoids an ungated route leaves it ungated. It answers through `get_or_create_agent`, so an unproven +caller naming any chat could mint an agent for it, and the route's own 424 would then report what +it had found. It now consults `session_reach` before the agent is fetched, like `GET +/sessions/{id}` and `POST /agent/resume`. + +⚠ **The two 403 bodies differ, and that is recorded rather than smoothed over.** A public +subagent's chat is told it is a subagent; an id that does not exist is told only that it is out of +reach. In isolation the pair is an existence oracle for subagent ids. It is dominated, and the +measurement is in +`routes::agent::resume_update_security_tests::a_private_subagent_and_an_unknown_id_are_refused_in_the_same_words`: +for a **private** subagent the two refusals are byte-identical, because `session_reach` fires first +and its one sentence answers "private" and "no such chat" alike; and for a **public** one the same +unproven caller is answered **200** by `/agent/resume` on an ordinary public chat and **200** by +`GET /sessions/{id}` on the subagent's, `session_type` included — so the body discloses nothing the +route next door does not hand over outright. ⚠ With the privacy master switch **OFF** +`session_reach` returns `Ok` before its store read, so the private row joins the public one and the +pair separates for every chat on the machine. That is the switch's pre-existing blast radius +(DR-17), not this route's, and it is not closed here. + +It could not even be read. The renderer loaded every chat through `/agent/resume`, so a subagent's +tab rendered *"Could not load this chat"* over the daemon's refusal — including the tab the daemon +itself opens to show a subagent it has just spawned. Measured against a real `biorouter serve`: +`POST /agent/resume` answered 403 for the child while `GET /sessions/{id}` and +`GET /sessions/{id}/events` answered 200 for the same chat. In a browser the tab now loads through +those two reads, and never asks for the agent — not `/agent/resume` again, not the rejoin (which +re-POSTs `/reply`), and nothing that reads AGENT state, because `/agent/callable_tool_count` +answers through `get_or_create_agent` and would mint a bare placeholder agent under the child's +session id. A note takes the composer's place, one line takes the header Stop's, and the +transcript's "still working" nudge stops pointing at a composer that is not there +(`ui/desktop/src/components/subagent/subagentReadOnly.ts`). + +⚠ **The tab decides this at mount, and the badge the daemon's own workspace frame put on it is +not enough on its own.** In a browser the two reads queue behind the page's open event streams — +six connections per origin, one stream per observed tab — and with a subagent running the refused +resume alone took 4.8 s, with the session read still pending five seconds later. All of that is +the running window, which is exactly when the ordinary composer was offering a Stop that could +only be refused. The badge is the one source known at mount, so it was added first; but +`tabAnnotations` is ordinary renderer state written only from live daemon frames, while the tab +LAYOUT is persisted per window — so a page **reload** restores a subagent's tab with no badge, and +a tab reached from History never had one. Every source then read "no", and "no" mounted the +composer: the decision **failed open on reload**, which is the opposite of SD-8's promise. + +So the decision has **three** states, not two (`subagentComposerKind`): a subagent's chat, a chat +that is definitely not one, and *not yet known*. A boolean reported the third as the second. In a +browser the third **withholds** the composer rather than mounting one that may have to be taken +back; on the desktop, which holds the key, it changes nothing. The cost is close to invisible, +because the transcript of a browser chat does not paint until that same read lands either — and +the two states that could turn withholding into a lockout are resolved deliberately: a tab with no +session id yet (the empty tab before a first message) and a chat the store could not load at all +both count as "not a subagent". + +**Two writes the missing composer never reached**, both inside the transcript rather than under it, +and so both still live on a read-only tab until they were withheld by the same flag: an +**elicitation card**, which posts its answer through `/reply` exactly as the composer does, and +**artifact auto-repair**, which needs no click at all — a figure that fails to render is the +trigger, and `shouldAutoRepairArtifact` is satisfied by a subagent's chat because that chat really +is live. Both are withheld by passing no callback, which is how the read-only transcript surfaces +already withhold them: `BioRouterMessage` renders the elicitation form only when handed a submit +callback, and `ArtifactViewer` installs its `postMessage` listener only when handed `onRenderError`. + **Why.** SD-1 already required that *"the interface must explain the refusal rather than appear broken"*, and stated it about the model picker. The same argument covers every proof-backed control, and an approval card is the worst case: three buttons that look live, a bare 403 on @@ -284,6 +370,184 @@ behaviour, so changing it means revisiting this record, not making a quiet fix. --- +## SD-11 — Stop works on a daemon with no key; steering does not, and a subagent's tab stays the person's + +**Ruling.** On a daemon that holds no user-action key — the one `biorouter serve` starts (SD-7), or +a `biorouterd` started by hand — **three** of the four routes that control a running turn admit +exactly the callers `POST /agent/stop` already admits there: + +| Route | What it does | +|---|---| +| `POST /agent/cancel` | Stop, and the first half of Stop-and-Send. | +| `POST /agent/continuation/abandon` | Gives up a Stop-and-Send replacement. | +| `POST /agent/continuation/recover` | Takes a Stop-and-Send replacement back after a reload, or gives it up. | + +That gate is `authorize_agent_control` in `routes/agent.rs`, called by name rather than written +again: the reach rule — every public chat, and a private one only for a caller whose stated +capability covers it — and then no subagent's chat. + +**`POST /interrupt` — mid-turn steering — is excluded, and refuses in words.** It asks for the +user-action proof on **both** kinds of daemon, so on a keyless one it is unavailable to everybody, +the person at the browser included. Its refusal is a `403` carrying +`reply.rs::STEER_NO_KEY`, a sentence naming this daemon as what cannot check a proof rather than +telling a person to go and prove they are one (SD-8) — and never an empty body, because an empty +turn-control `403` is how `biorouter session attach` recognises a daemon that *does* hold a key and +decides to ask the person for it. *Why* is the section below. + +Two things hold beside all four: + +- A daemon that holds a key — the desktop application's — is unchanged. These routes take the proof + there and nothing else, and a steer admitted there is stamped `UserDirect`, exactly as before. +- A subagent's chat is refused on a keyless daemon, as `/reply` and `/agent/stop` already refuse it + there, and the refusal now names the daemon, not the caller, as what cannot prove a person acted + (SD-8). + +**Why.** The four routes asked for proof of a person before anything else, and on a daemon that +holds no key that proof can only refuse everyone, the person at the browser included. Measured on +2026-09-11 against a real `biorouter serve`, from the page, with the daemon secret and the host's +provider stated: `POST /agent/cancel` and `POST /interrupt` both answered `403` with an empty body. +The Stop button could stop nothing on any `serve` host, including one configured with a public +model. `crates/biorouter-server/tests/turn_control_no_user_key.rs` reproduces it on the code this +record changed: on a keyless daemon and an ordinary public chat, `/agent/stop` answered `200` and +cancelled the turn while `/agent/cancel` answered `403`. Stop is the control that ends a runaway +turn, and a chat that cannot be stopped from the only interface it has is less safe, not more. + +And the refusal protected nothing. The proof is on these routes so that a model holding the daemon +secret, which AR-11 found recoverable, cannot stop another chat's turn or put words in a person's +mouth. On a keyless daemon that caller already does each of those next door: + +| What the refusal withheld | Where the same caller already does it on a keyless daemon | Guarded by | +|---|---|---| +| Cancel the running turn of a chat it can reach | `POST /agent/stop`, which trips the same turn and evicts its agent besides | `authorize_agent_control` — the gate this record adopts | +| The same, as a model holding no secret at all | `workspace_close { scope: "turn" }` | `refuse_unless_writable`: the tier, and nothing else | +| Put text in front of the chat's model as the user, **before or after a turn** | `POST /reply`, which a non-subagent chat accepts, unstamped, from any caller that reaches it | `session_reach` | +| Give up a chat's pending Stop-and-Send | `workspace_close { scope: "turn" }`, which abandons every pending continuation of the chat it names | `refuse_unless_writable` | + +So admitting a caller to those three gives nothing that holds the secret a capability it lacked. +The person gains the Stop button and Stop-and-Send, which the desktop application has always had. + +⚠ **The third row of that table is the whole argument for the steer, and it does not hold.** +`POST /reply` takes the BR-33 single-turn lock and answers `409 CONFLICT` whenever a *different* +turn is already running in that chat; `POST /interrupt` is meaningful only while one is, and +answers `409` when none is. The two preconditions are **disjoint**: in the exact state where a +steer lands, the route said to dominate it is refused. So admitting the steer would hand a caller +holding only the daemon secret something genuinely new — attacker-chosen text injected into a turn +already in flight, *without cancelling it*, which the person watching sees as their own turn +changing direction. The nearest thing that caller already has is cancel-then-reply, and that is +**visible**: the turn dies first. It is not a tier crossing — `session_reach` still refuses a +private chat to a public caller, and the subagent rule still refuses every child's chat — but it is +a capability asymmetry, and the dominance argument is the only thing this record had to offer for +it. Hence the exclusion. (Found in the security review of this change, before it merged; +`reply.rs::authorize_steer` carries the same reasoning at the code.) + +**Why the other three routes move together.** Stop-and-Send cancels with a continuation, and the +continuation mints a lease that holds the chat for its replacement: until the lease is used or +given up, every other turn in that chat is refused. A daemon that let the cancel through and +refused the abandon or the recover would wedge the chat the first time a person removed a queued +message or reloaded the page. The test binary above measures both. + +**What a browser user loses, and what happens instead.** On a keyless daemon the browser's steer +is refused, and the renderer's `steer()` already treats any refusal as "fall back to an ordinary +send" (`chatStreamStore.tsx`): the text is queued and delivered when the turn ends, rather than +injected into it or lost. That is the cost of the exclusion, and it is a delay rather than a +capability the person no longer has. + +**Why not stamp the keyless steer `UserDirect` and admit it?** This was the shape the record took +before the review. `UserDirect` is a claim that a person typed the text, and the subagent machinery +acts on it, so the steer would have had to arrive unstamped — which is what `/reply` gives the same +caller's message. But the stamp was never the problem: the *injection into a running turn* is, and +an unstamped steer still redirects the model mid-flight and still appears in the transcript beside +the person's own messages. + +**Why a subagent's tab is not included.** That tab is where `UserDirect` means something, and +`/reply` and `/agent/stop` already refuse an unproven caller there, on every daemon. Admitting turn +control alone would give that tab a Stop that works beside a composer that cannot send, and the +rule for the tab is a decision about the whole tab rather than one of its buttons. + +**Why not on every daemon.** On a daemon that holds a key the proof costs the person nothing — the +renderer attaches it to every request — and it still refuses a caller that cannot present it. +Relaxing it there buys the person nothing, and would admit an unstamped steer where the desktop has +always stamped one. + +**Who can do this, and what else reaches the same place.** The two questions every privacy control +answers in writing ([privacy tiers §3.1](../security/privacy-tiers.md#31-the-review-checklist--two-questions-every-control-answers-in-writing)): + +- *Who can initiate it.* On a keyless daemon, anything holding the daemon secret: the person in the + browser and, indistinguishably, a model running in a chat on that daemon that has recovered the + secret. Both reach the chats the reach gate admits them to, less every subagent's. On a daemon + that holds a key, only a request carrying the proof. +- *What else reaches the same place:* + + | Other entry point to the same capability | Reachable by | Guarded by | Where that guard is called | + |---|---|---|---| + | `POST /agent/stop` — cancels the running turn | user; any holder of the daemon secret | `authorize_agent_control`: reach, then the subagent rule | `routes/agent.rs::stop_agent` | + | `workspace_close { scope: "turn" \| "agent" }` — cancels the turn, abandons its pending continuations | model | `refuse_unless_writable` (the tier) | `agents/workspace_extension.rs::handle_close` | + | `POST /active_work/{id}/cancel` — cancels a running subagent or background job by registry id | user; any holder of the daemon secret | **nothing**: the id is not a session id, so the reach gate cannot be applied | `routes/active_work.rs::cancel_active_work`; an open residual in `session_reach.rs` | + | `POST /reply` — puts the caller's text in front of the chat's model | user; any holder of the daemon secret | `session_reach`, then the subagent rule | `routes/reply.rs::reply` | + | `workspace_send_prompt { mode: "steer" \| "turn" }` — the same, from another chat | model | `refuse_unless_writable`; the text arrives framed as `AgentInjection`, and a private-to-public write raises a first-crossing approval | `agents/workspace_extension.rs::handle_send_prompt` | + | `biorouter session attach` and `session cancel` → these routes | user at a terminal | the proof, which the CLI demands before it sends anything | `commands/session_watch.rs::build_user_action_post_request` | + + The third row is a finding rather than a guard: a subagent's work can be cancelled on any daemon + by a caller that holds the secret and reads the id from `GET /active_work`. It is the residual + `session_reach.rs` already records, and this record does not widen it. + +**Displaced alternatives.** + +- *Keep the refusal, and explain it before the click (SD-8).* Rejected. SD-8 is for a control whose + absence is safe to explain; this one is how a person ends a turn they did not want. A Stop button + that says "unavailable here" is honest and still leaves the turn running. +- *Point the browser's Stop at `/agent/stop`.* Rejected. It cancels whatever turn is running rather + than the generation the person saw, so a late click can kill a successor; it does not wait for the + turn to settle and has no Stop-and-Send; and it evicts the agent. It would give the browser a + blunter Stop than the desktop's, to route around a refusal that guards nothing. +- *Relax the routes on every daemon.* Rejected; see *Why not on every daemon*. +- *Admit `/interrupt` too, with the steer arriving unstamped.* Rejected in review; see the ⚠ + paragraph under *Why* and *Why not stamp the keyless steer `UserDirect` and admit it?*. +- *Admit `/interrupt` and refuse it with an empty `403` like the others.* Rejected. `biorouter + session attach` reads an empty turn-control `403` as "this daemon holds a key" and prompts the + person for one; on a keyless daemon that prompt asks for a credential that does not exist and + then reports it as the wrong key. The refusal carries `STEER_NO_KEY` instead. +- *Admit plain Stop and refuse Stop-and-Send.* Rejected. The queue's "Stop and send" and a typed + "stop" in a busy composer both cancel with a continuation, so this would leave half of the Stop + controls refused, and a lease a person can mint must be a lease they can give up. + +**Consequence to accept.** On a keyless daemon, a model running in a chat on that daemon that has +recovered the daemon secret can stop or settle a turn in any chat the reach gate admits it to — a +public chat always, and a private one by stating a private provider, which the header does not +authenticate (`session_reach.rs` records as much). It could already stop those turns through +`/agent/stop` or `workspace_close`, and put text in front of those chats through `/reply`; what it +gains over that is the exact generation semantics and the Stop-and-Send lease, not a new reach. It +is recorded rather than closed: on a daemon that cannot tell a person from a model, closing it +means a Stop button nobody can press. **Steering a running turn is the line this stops at**, for +the reason above. + +**A daemon that is keyless by accident says so at startup.** Before this record, a desktop daemon +that came up without its key announced itself at the first click: Stop answered `403` and the user +complained. Now Stop works there, so the same misconfiguration is silent — a *weaker* daemon rather +than a visibly broken one. `read_user_action_digest` therefore reports **why** it holds no key +rather than returning one undifferentiated "none": stdin was a terminal, stdin closed with nothing +on it (`serve`'s `Stdio::null()`), a writer held the pipe open and wrote nothing inside the 2 s +bound, or the line was not a 32-byte hex digest. The last two are launcher faults and nothing +Biorouter ships does either on purpose, so their warning says so and says to restart. Every arm +names both consequences — what this daemon refuses, and what it now admits instead. The bound is +unchanged: nothing measured says the desktop launcher misses it, and what was missing was the +report, not the time. + +**Not decided here.** An ordinary browser chat's steer control is still offered on a keyless daemon +and still refuses — its text falls back to the send queue, so nothing is lost, but SD-8's rule would +have it say so first. The CLI's `session cancel` and `attach` steering still demand a user-action key +from the terminal before they send anything, so against a keyless daemon they refuse locally a Stop +the daemon would now admit. + +**Decided since, under SD-8.** This record left a subagent's tab in a browser offering a composer, +a steer and a Stop that all refuse, and said SD-8 required them to say so before the click. +Measuring it found the tab worse off than that — it did not open at all — and the whole case, +with what the interface now does instead, is written up in +[SD-8](#sd-8--a-control-that-can-never-work-here-says-so-rather-than-failing-on-click). Nothing +about the refusals above changed. + +--- + ## Related documentation - [Architecture of the serving path](serve-architecture.md) — how the decisions above are built. diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index cc6888530..e747f7ed6 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -266,6 +266,9 @@ "401": { "description": "Unauthorized - invalid secret key" }, + "403": { + "description": "Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried neither a capability that covers it nor proof it came from the user" + }, "424": { "description": "Agent not initialized" } @@ -308,7 +311,7 @@ "description": "Unauthorized - invalid secret key" }, "403": { - "description": "The request was not proven to come from the user" + "description": "The request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11)" }, "409": { "description": "A different turn generation is active, another client owns the continuation, or its admission is still settling", @@ -357,7 +360,7 @@ } }, "403": { - "description": "The request was not proven to come from the user" + "description": "The request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11)" }, "409": { "description": "The lease is invalid or belongs to another session", @@ -403,7 +406,7 @@ "description": "The continuation owner id is missing or invalid" }, "403": { - "description": "The session is out of reach or the request was not proven to come from the user" + "description": "The session is out of reach or the request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11)" }, "409": { "description": "The exact continuation generation was already resolved", @@ -926,7 +929,7 @@ "description": "Unauthorized - invalid secret key" }, "403": { - "description": "Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried no proof it came from the user" + "description": "Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried no proof it came from the user; or the named chat is a delegated subagent's, whose working directory only the person at the keyboard may repoint (SD-8)" }, "404": { "description": "Session not found" @@ -1864,7 +1867,7 @@ "description": "Empty message text" }, "403": { - "description": "The request was not proven to come from the user" + "description": "The request was not proven to come from the user; on a daemon that holds no user-action key, steering is unavailable and the refusal says so (SD-11)" }, "409": { "description": "No turn is accepting interrupts for this session" diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index b35605218..54837cd0c 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -4388,6 +4388,10 @@ export type GetCallableToolCountErrors = { * Unauthorized - invalid secret key */ 401: unknown; + /** + * Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried neither a capability that covers it nor proof it came from the user + */ + 403: unknown; /** * Agent not initialized */ @@ -4420,7 +4424,7 @@ export type CancelTurnErrors = { */ 401: unknown; /** - * The request was not proven to come from the user + * The request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11) */ 403: unknown; /** @@ -4457,7 +4461,7 @@ export type AbandonContinuationLeaseData = { export type AbandonContinuationLeaseErrors = { /** - * The request was not proven to come from the user + * The request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11) */ 403: unknown; /** @@ -4490,7 +4494,7 @@ export type RecoverContinuationErrors = { */ 400: unknown; /** - * The session is out of reach or the request was not proven to come from the user + * The session is out of reach or the request was not proven to come from the user; on a daemon that holds no user-action key, the chat is out of the caller's reach or is a subagent's (SD-11) */ 403: unknown; /** @@ -4920,7 +4924,7 @@ export type UpdateWorkingDirErrors = { */ 401: unknown; /** - * Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried no proof it came from the user + * Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried no proof it came from the user; or the named chat is a delegated subagent's, whose working directory only the person at the keyboard may repoint (SD-8) */ 403: unknown; /** @@ -5652,7 +5656,7 @@ export type InterruptErrors = { */ 400: unknown; /** - * The request was not proven to come from the user + * The request was not proven to come from the user; on a daemon that holds no user-action key, steering is unavailable and the refusal says so (SD-11) */ 403: unknown; /** diff --git a/ui/desktop/src/components/BaseChat.subagentReadOnly.test.ts b/ui/desktop/src/components/BaseChat.subagentReadOnly.test.ts new file mode 100644 index 000000000..f151794c5 --- /dev/null +++ b/ui/desktop/src/components/BaseChat.subagentReadOnly.test.ts @@ -0,0 +1,109 @@ +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * SD-8 on a delegated subagent's tab, the half that lives in BaseChat. + * + * BaseChat cannot be mounted in jsdom (see `BaseChat.privacy.test.tsx`), so its + * obligation is pinned against its source, as the other BaseChat suites do. + * The slot's own decision — the composer on the desktop, the reason in its + * place in a browser — is exercised by `subagent/SubagentComposerSlot.test.tsx`; + * what that suite cannot see is whether BaseChat routes the real composer + * THROUGH the slot. A composer mounted beside it instead would keep every + * refused control on screen while the slot's tests stayed green. + */ + +/** vitest runs with `ui/desktop` as its root — the idiom the other suites use. */ +const source = readFileSync(path.join(process.cwd(), 'src', 'components', 'BaseChat.tsx'), 'utf8'); + +/** The body of `renderChatInput`, from its arrow to the next top-level const. */ +function renderChatInputBody(): string { + const start = source.indexOf('const renderChatInput = () => ('); + expect(start, 'BaseChat no longer defines renderChatInput').toBeGreaterThan(-1); + const end = source.indexOf('\n );\n', start); + expect(end).toBeGreaterThan(start); + return source.slice(start, end); +} + +describe("BaseChat — a subagent's composer in a browser", () => { + it('mounts ChatInput only inside the read-only slot', () => { + const body = renderChatInputBody(); + const open = body.indexOf(''); + const close = body.indexOf(''); + const composer = body.indexOf(' { + // Its Take over and Abandon post the continuation routes, which SD-11 + // refuses for a subagent's chat on a keyless daemon, exactly like Stop. + const body = renderChatInputBody(); + const open = body.indexOf('') + ); + }); + + it('hands every source of the fact to one three-valued decision', () => { + const decision = /const subagentChatKind = subagentComposerKind\(\{([\s\S]*?)\}\);/.exec( + source + ); + expect(decision, 'BaseChat no longer computes subagentChatKind').not.toBeNull(); + // ⚠ The badge is the half that is known at MOUNT. The two reads are ordinary + // requests, and in a browser they queue behind every open event stream (six + // connections per origin) — measured at five seconds and more, all of it + // with the ordinary composer and its Stop on screen. + expect(decision![1]).toMatch(/tabAnnotations\?\.\[sessionId\]\?\.badge/); + expect(decision![1]).toMatch(/loadedSessionType: session\?\.session_type/); + expect(decision![1]).toMatch(/hookSaysSubagent: subagent\.isSubagent/); + // The review's finding 3: the badge does not survive a browser reload, so + // the decision must be able to say "not yet" as well as "no". These two + // fields are what let it. + expect(decision![1]).toMatch(/loadedSessionId: session\?\.id/); + expect(decision![1]).toMatch(/loadFailed:/); + }); + + it('keys the read-only consequences off the slot, not off subagent-ness', () => { + // Finding 3. `isSubagentChat` is false while the answer is in flight, so a + // consequence keyed off it is live for exactly the window the composer is + // withheld in. + expect(source).toMatch( + /const subagentTabReadOnly = composerSlotMode\(subagentChatKind\) !== 'composer';/ + ); + }); + + it('withholds the two writes a read-only tab can still make', () => { + // The PR author's own follow-up. Neither is reachable from the composer, so + // removing the composer never closed them. + // + // 5.1 — an elicitation card lives INSIDE the transcript and posts `/reply`. + // `BioRouterMessage` renders the form only when handed a submit callback. + const list = //.exec(source); + expect(list, 'BaseChat no longer renders ProgressiveMessageList').not.toBeNull(); + expect(list![0]).toMatch( + /submitElicitationResponse=\{\s*subagentTabReadOnly \? undefined : submitElicitationResponse\s*\}/ + ); + // 5.2 — artifact auto-repair feeds a broken figure back to the child on a + // render error, with no click at all. `ArtifactViewer` installs the + // postMessage listener only when handed the callback. + const viewer = //.exec(source); + expect(viewer, 'BaseChat no longer renders ArtifactViewer').not.toBeNull(); + expect(viewer![0]).toMatch( + /onRenderError=\{subagentTabReadOnly \? undefined : handleArtifactRenderError\}/ + ); + }); + + it("tells the transcript's activity nudge that this tab cannot stop the turn", () => { + // With the composer gone, "You can stop the turn from the composer" would + // send the reader to a control that is not there. + const list = //.exec(source); + expect(list, 'BaseChat no longer renders ProgressiveMessageList').not.toBeNull(); + expect(list![0]).toMatch(/canStopTurn=\{!subagentTabReadOnly\}/); + }); +}); diff --git a/ui/desktop/src/components/BaseChat.tsx b/ui/desktop/src/components/BaseChat.tsx index 252e4f8b7..2f4b641db 100644 --- a/ui/desktop/src/components/BaseChat.tsx +++ b/ui/desktop/src/components/BaseChat.tsx @@ -105,6 +105,8 @@ import type { ResourceContents, } from '../api'; import { SIDEBAR_COMPACT_WIDTH as SIDEBAR_COMPACT_TITLE_WIDTH } from './Layout/yieldLadder'; +import { SubagentComposerSlot } from './subagent/SubagentComposerSlot'; +import { composerSlotMode, subagentComposerKind } from './subagent/subagentReadOnly'; import { SubagentTabHeader } from './subagent/SubagentTabHeader'; import { extractKnowledgeBases, useSubagentSession } from './subagent/useSubagentSession'; import { useChatGroups } from '../contexts/ChatGroupsContext'; @@ -1416,6 +1418,47 @@ function BaseChatContent({ // keeps the standalone mounts (which have no tab strip to open a parent into) // from crashing. const chatGroups = useChatGroups(); + // Is this a delegated subagent's chat — and if the answer is not in yet, say + // so rather than saying no. Three sources, and `subagentComposerKind` is + // where the three-way decision lives (with its own unit tests): + // + // - the badge the daemon's workspace frame put on the tab when it opened it + // for a subagent it had just spawned — the same annotation the tab strip + // draws the robot glyph from, and the only source known at MOUNT; + // - the chat store's row, which in a browser is the only way a subagent's + // chat loads at all (`loadReadOnlySubagentChat`); + // - the header hook's own read, which is positive-only. + // + // ⚠ Neither read is prompt. They are ordinary requests, and in a browser they + // queue behind every open event stream: the page holds one per observed tab + // and the browser allows six connections per origin. Measured on 2026-09-11 + // with a subagent running, the store's `/agent/resume` took 4.8 s to be + // refused and its session read was still pending five seconds later — all of + // it the running window, which is exactly when the ordinary composer offered + // a Stop that could only be refused. + // + // ⚠ And the badge does NOT close that window on its own, which is what made + // this a three-state decision rather than a boolean. `tabAnnotations` is + // ordinary React state written only from live daemon frames; the tab LAYOUT is + // persisted to `localStorage` per window and the annotations are not. So a + // reloaded page — or a tab opened from History, which never had a frame — + // restores the subagent's tab with no badge, both reads start from nothing, + // and every source reads `false`. A boolean reported that as "not a + // subagent", and mounted the composer. + const subagentChatKind = subagentComposerKind({ + badge: chatGroups?.tabAnnotations?.[sessionId]?.badge, + sessionId, + loadedSessionId: session?.id, + loadedSessionType: session?.session_type, + hookSaysSubagent: subagent.isSubagent, + loadFailed: sessionLoadError !== undefined, + }); + // SD-8: in a browser such a chat can only be read (see `subagentReadOnly.ts`), + // and so can one whose kind is not settled YET — the composer is withheld in + // both. So everything that keys off "there is a composer to use" keys off the + // slot's own decision, never off "this is a subagent's chat": the two differ + // for exactly the window this fix is about. + const subagentTabReadOnly = composerSlotMode(subagentChatKind) !== 'composer'; const canDivergeSession = useMemo( () => messages.some((message) => message.role === 'assistant'), @@ -2055,54 +2098,75 @@ function BaseChatContent({ 'biorouter-composer-view-transition' )} > - {pendingContinuation && ( -
- - {pendingContinuation.ownership === 'owned' - ? 'A previous Stop & send is ready. Re-enter the message you want to send; Biorouter will not guess or resend lost composer text.' - : pendingContinuation.ownership === 'settling' - ? 'A previous Stop & send is still settling. Recover it explicitly or abandon the stopped-turn continuation.' - : 'Another window owns a pending Stop & send. Take it over here or abandon the stopped-turn continuation before sending.'} - -
- {pendingContinuation.ownership !== 'owned' && ( + {/* + H3 (2026-09-10 security test drive) — privacy tiers are OFF, where the + switch is recorded, and whether the app recorded turning it off. Same + slot, same rails and the same unconditional mount as the note below, + and first of the two: it is about the whole machine, that one about + this chat. It renders nothing while the tiers are on. + + ⚠ OUTSIDE the read-only slot below, and that is the point of its own + "no dismiss control" rule: the condition is standing, so the statement + of it is too. A subagent's tab in a browser loses its composer, not the + notice that every gate on this machine is off. + */} + + {/* + SD-8: in a browser, a delegated subagent's chat gets the reason it has + no composer IN PLACE of everything below — the continuation banner's + buttons and every write `ChatInput` holds are refused there. Everywhere + else this renders its children untouched. The shell div stays so the + composer's motion ref and layout slot are the same either way. + */} + + {pendingContinuation && ( +
+ + {pendingContinuation.ownership === 'owned' + ? 'A previous Stop & send is ready. Re-enter the message you want to send; Biorouter will not guess or resend lost composer text.' + : pendingContinuation.ownership === 'settling' + ? 'A previous Stop & send is still settling. Recover it explicitly or abandon the stopped-turn continuation.' + : 'Another window owns a pending Stop & send. Take it over here or abandon the stopped-turn continuation before sending.'} + +
+ {pendingContinuation.ownership !== 'owned' && ( + + )} - )} - +
-
- )} - {/* + )} + {/* Issue #56 Gate B. Above the composer, on the composer's own rails, in the same slot the Stop-and-send banner already uses — so it sits with the control it is about rather than in the transcript, where it would @@ -2116,61 +2180,54 @@ function BaseChatContent({ Mounted unconditionally — it renders nothing when there is nothing to say, which is almost always. */} - {/* - H3 (2026-09-10 security test drive) — privacy tiers are OFF, where the - switch is recorded, and whether the app recorded turning it off. Same - slot, same rails and the same unconditional mount as the note below, - and first of the two: it is about the whole machine, that one about - this chat. It renders nothing while the tiers are on. - */} - - - setDroppedFiles([])} // Clear dropped files after processing - messagesLength={messages.length} - workingDirLocked={workingDirLocked} - disableAnimation={disableAnimation} - sessionCosts={sessionCosts} - modelCostRows={modelRows} - workflow={workflow} - workflowAccepted={!hasNotAcceptedWorkflow} - initialPrompt={initialPrompt} - toolCount={toolCount || 0} - supportsVisionOverride={session ? (sessionSupportsVision ?? false) : undefined} - supportedInputMimeTypesOverride={sessionSupportedInputMimeTypes} - // #39 — capture a pre-session directory choice so the first message - // creates the session in it. Before the customChatInputProps spread, - // so callers can still override. - onWorkingDirChange={setPendingWorkingDir} - {...customChatInputProps} - /> + + setDroppedFiles([])} // Clear dropped files after processing + messagesLength={messages.length} + workingDirLocked={workingDirLocked} + disableAnimation={disableAnimation} + sessionCosts={sessionCosts} + modelCostRows={modelRows} + workflow={workflow} + workflowAccepted={!hasNotAcceptedWorkflow} + initialPrompt={initialPrompt} + toolCount={toolCount || 0} + supportsVisionOverride={session ? (sessionSupportsVision ?? false) : undefined} + supportedInputMimeTypesOverride={sessionSupportedInputMimeTypes} + // #39 — capture a pre-session directory choice so the first message + // creates the session in it. Before the customChatInputProps spread, + // so callers can still override. + onWorkingDirChange={setPendingWorkingDir} + {...customChatInputProps} + /> +
); @@ -2458,9 +2515,21 @@ function BaseChatContent({ turnStartedAt={turnStartedAt} lastMessageAt={lastMessageAt} pendingSteer={pendingSteer} + canStopTurn={!subagentTabReadOnly} onRenderingComplete={handleRenderingComplete} onMessageUpdate={onMessageUpdate} - submitElicitationResponse={submitElicitationResponse} + // Finding 5.1 (the PR author's own follow-up). + // `ElicitationRequest` posts its answer through + // `/reply` — the same write the composer makes — + // and it lives INSIDE the transcript, so removing + // the composer never reached it. + // `BioRouterMessage` renders the form only when it + // is handed a submit callback, so withholding the + // callback withholds the control rather than + // leaving a Submit that 403s. + submitElicitationResponse={ + subagentTabReadOnly ? undefined : submitElicitationResponse + } onOpenArtifact={handleOpenArtifact} onRunInTerminal={handleRunInTerminal} workingDir={sessionWorkingDir} @@ -2550,7 +2619,16 @@ function BaseChatContent({ // Chat-only, and the reason the panel's repair listener exists at // all: a read-only transcript passes nothing here, so // ArtifactViewer never installs the postMessage listener. - onRenderError={handleArtifactRenderError} + // + // Finding 5.2 (the PR author's own follow-up). A subagent's tab in + // a browser is a LIVE chat by every other measure, so + // `shouldAutoRepairArtifact` would happily fire inside the child's + // running turn and feed the broken figure back to it through + // `/reply`. That is the one write the composer's removal could not + // reach, because nobody clicks it — a figure failing to render is + // the trigger. Same instrument as the read-only transcripts: pass + // nothing, and the listener is never installed. + onRenderError={subagentTabReadOnly ? undefined : handleArtifactRenderError} onLiveBrowserShareChange={setLiveBrowserShare} onFilePreviewRevisionChange={setFilePreviewRevision} refreshRevision={artifactRefreshRevision} diff --git a/ui/desktop/src/components/ProgressiveMessageList.test.tsx b/ui/desktop/src/components/ProgressiveMessageList.test.tsx index 41bef3c30..a80eb7815 100644 --- a/ui/desktop/src/components/ProgressiveMessageList.test.tsx +++ b/ui/desktop/src/components/ProgressiveMessageList.test.tsx @@ -138,6 +138,35 @@ describe('ProgressiveMessageList trailing activity indicator', () => { expect(indicatorIndex).toBeGreaterThan(lastMessageIndex); }); + it("hands the indicator whether this tab can stop the turn, so the nudge can't lie", () => { + // Past the 45 s nudge threshold on a live turn. Desktop (the default) + // points at the composer; a subagent's tab in a browser has none (SD-8). + const longAgo = Date.now() - 46_000; + const { rerender } = render( + + ); + expect(screen.getByText(/stop the turn from the composer/)).toBeInTheDocument(); + + rerender( + + ); + expect(screen.getByText('Still working.')).toBeInTheDocument(); + expect(screen.queryByText(/stop the turn from the composer/)).toBeNull(); + }); + it('shows no indicator while the assistant is streaming visible prose', () => { const prose: Message = { id: 'assistant-2', diff --git a/ui/desktop/src/components/ProgressiveMessageList.tsx b/ui/desktop/src/components/ProgressiveMessageList.tsx index 7d73f14e2..5d24f6587 100644 --- a/ui/desktop/src/components/ProgressiveMessageList.tsx +++ b/ui/desktop/src/components/ProgressiveMessageList.tsx @@ -63,6 +63,12 @@ interface ProgressiveMessageListProps { lastMessageAt?: number; /** BR-61: a soft interrupt awaiting the agent, shown as a trailing chip. */ pendingSteer?: PendingSteer; + /** + * Whether the reader can stop the running turn from this tab. Only a + * delegated subagent's tab in a browser answers false: it has no composer + * (SD-8), so the trailing indicator's nudge must not point at one. + */ + canStopTurn?: boolean; } export default function ProgressiveMessageList({ @@ -85,6 +91,7 @@ export default function ProgressiveMessageList({ turnStartedAt, lastMessageAt, pendingSteer, + canStopTurn = true, }: ProgressiveMessageListProps) { const [renderedCount, setRenderedCount] = useState(() => { // Initialize with either all messages (if small) or first batch (if large) @@ -344,7 +351,7 @@ export default function ProgressiveMessageList({ rhythm matches exactly. */} {trailingActivity && (
- +
)} diff --git a/ui/desktop/src/components/TurnActivityIndicator.test.tsx b/ui/desktop/src/components/TurnActivityIndicator.test.tsx index 25c41d045..00dd6f36b 100644 --- a/ui/desktop/src/components/TurnActivityIndicator.test.tsx +++ b/ui/desktop/src/components/TurnActivityIndicator.test.tsx @@ -58,6 +58,29 @@ describe('TurnActivityIndicator', () => { expect(screen.queryByText(/Still working/)).toBeNull(); }); + it('points at the composer only where there is one to stop from', () => { + // A delegated subagent's tab in a browser has no composer (SD-8), so the + // half of the nudge that sends the reader there is dropped, not left + // pointing at nothing. The reassurance stays. + const now = Date.now(); + vi.setSystemTime(now); + render( + + ); + expect(screen.getByText('Still working.')).toBeInTheDocument(); + expect(screen.queryByText(/stop the turn from the composer/)).toBeNull(); + }); + + it('still names the composer by default', () => { + renderAt(46_000); + expect( + screen.getByText('Still working. You can stop the turn from the composer.') + ).toBeInTheDocument(); + }); + it('exposes a polite live region and hides the ticking chip from screen readers', () => { renderAt(5000); const status = screen.getByRole('status'); diff --git a/ui/desktop/src/components/TurnActivityIndicator.tsx b/ui/desktop/src/components/TurnActivityIndicator.tsx index aaa1f3f82..403540ff2 100644 --- a/ui/desktop/src/components/TurnActivityIndicator.tsx +++ b/ui/desktop/src/components/TurnActivityIndicator.tsx @@ -11,6 +11,12 @@ export const NUDGE_MS = 45000; interface TurnActivityIndicatorProps { activity: TrailingActivity; className?: string; + /** + * Whether the reader can stop this turn from this tab. False on a delegated + * subagent's tab in a browser, which has no composer at all (SD-8) — so the + * nudge must not send the reader to one. + */ + canStopHere?: boolean; } /** @@ -30,7 +36,11 @@ interface TurnActivityIndicatorProps { * for everything; the pulse degrades to a static dot. Do not add a per-component * check here — that fights the global rule. */ -export default function TurnActivityIndicator({ activity, className }: TurnActivityIndicatorProps) { +export default function TurnActivityIndicator({ + activity, + className, + canStopHere = true, +}: TurnActivityIndicatorProps) { const elapsedMs = useElapsedMs(activity.since); const showElapsed = elapsedMs !== null && elapsedMs >= ELAPSED_REVEAL_MS; const showNudge = elapsedMs !== null && elapsedMs >= NUDGE_MS; @@ -103,7 +113,9 @@ export default function TurnActivityIndicator({ activity, className }: TurnActiv {showNudge && (
- Still working. You can stop the turn from the composer. + {canStopHere + ? 'Still working. You can stop the turn from the composer.' + : 'Still working.'}
)} diff --git a/ui/desktop/src/components/subagent/SubagentComposerSlot.test.tsx b/ui/desktop/src/components/subagent/SubagentComposerSlot.test.tsx new file mode 100644 index 000000000..afbab9251 --- /dev/null +++ b/ui/desktop/src/components/subagent/SubagentComposerSlot.test.tsx @@ -0,0 +1,204 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { BROWSER_SURFACE_MARKER } from '../../utils/surface'; +import { SubagentComposerSlot } from './SubagentComposerSlot'; +import { + SUBAGENT_TAB_READ_ONLY_REASON, + composerSlotMode, + isReadOnlySubagentChat, + subagentComposerKind, + subagentTabReadOnlyReason, +} from './subagentReadOnly'; + +/** + * SD-8 for a delegated subagent's tab: on a `biorouter serve` page the daemon + * refuses every write to a subagent's chat (SD-7, SD-11), so the composer's + * place says why instead of offering a composer that fails on click. + * + * A stand-in composer rather than `ChatInput`: what is asserted is the slot's + * one decision — mount the children, or mount the reason in their place — and + * `ChatInput` needs a dozen providers to render at all. That BaseChat puts the + * real composer inside this slot is pinned against its source in + * `BaseChat.subagentReadOnly.test.ts`. + */ +function Composer() { + return ( +
+