From b9915a0ea5b329e7e1b248037901f10659c5e983 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:12:54 -0700 Subject: [PATCH 01/11] test(code-execution): pin that a script's tool calls face their own permission decision (F7, fail-before) QA finding F7 (composer-driven run on 7c96d796): in Manual mode, with `code_execution__execute_code` on the user's always-allow list, a script's `developer__shell` and `developer__analyze` calls ran with no approval card. The dispatched tool was `execute_code`, which was allowed; the calls inside the script were never judged. Four tests through the agent's real `dispatch_tool_call`, the real `developer` and `code_execution` extensions and a private permission table. Against this commit (no fix yet): manual_mode_asks_for_a_scripts_shell_call_under_the_inner_tools_name FAILED "ran with no approval card ... Result: \"SCRIPT-GATE-ALLOWED\n\"" a_denied_card_is_a_catchable_tool_error_and_the_script_goes_on FAILED "ran with no approval card: Result: { caught: null, continued: true }" always_deny_on_the_inner_tool_refuses_it_and_the_script_continues FAILED "must come back into the script as a tool error: { caught: null, ... }" auto_mode_runs_a_scripts_shell_call_with_no_card ok --- crates/biorouter/src/agents/mod.rs | 3 + .../biorouter/src/agents/script_call_gate.rs | 363 ++++++++++++++++++ 2 files changed, 366 insertions(+) create mode 100644 crates/biorouter/src/agents/script_call_gate.rs diff --git a/crates/biorouter/src/agents/mod.rs b/crates/biorouter/src/agents/mod.rs index 430b9c16e..d0ae91892 100644 --- a/crates/biorouter/src/agents/mod.rs +++ b/crates/biorouter/src/agents/mod.rs @@ -46,6 +46,9 @@ pub(crate) mod reply_parts; pub mod resource_refs; pub mod retry; mod schedule_tool; +// QA finding F7: every tool call a Code Execution script makes faces the same +// permission decision it would face as a direct call. +pub(crate) mod script_call_gate; mod session_blob_tool; // The session-row write for `enabled_extensions.v0`, plus the classifier that // says which catalog tools require it. `pub` because `agents::agent` is diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs new file mode 100644 index 000000000..f7f2667cd --- /dev/null +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -0,0 +1,363 @@ +//! The permission decision for every tool call a Code Execution script makes +//! (QA finding F7, 2026-09-10). + +#[cfg(test)] +mod tests { + use std::sync::Arc; + use std::time::Duration; + + use rmcp::model::{CallToolRequestParams, CallToolResult, JsonObject, RawContent}; + use rmcp::object; + use tokio_util::sync::CancellationToken; + + use crate::action_required_manager::ActionRequiredManager; + use crate::agents::extension::ExtensionConfig; + use crate::agents::{Agent, AgentConfig}; + use crate::config::permission::{PermissionLevel, PermissionManager}; + use crate::config::BioRouterMode; + use crate::conversation::message::{ActionRequiredData, MessageContent}; + use crate::pending_user_action::DecisionAuthority; + use crate::permission::permission_confirmation::PrincipalType; + use crate::permission::{Permission, PermissionConfirmation}; + use crate::session::session_manager::SessionType; + use crate::session::{Session, SessionManager}; + + const EXECUTE_CODE: &str = "code_execution__execute_code"; + const SHELL: &str = "developer__shell"; + + /// A real agent — its own inspector stack, its own permission table — with + /// the two extensions the QA run used: `developer` and `code_execution`. + /// + /// The permission table is private to the fixture and pre-seeded exactly as + /// the QA sandbox's was in the one entry that matters: the SCRIPT is always + /// allowed. Everything the script calls must then stand on its own name. + struct Fixture { + agent: Arc, + session: Session, + permissions: Arc, + dir: tempfile::TempDir, + } + + async fn fixture(mode: BioRouterMode) -> Fixture { + let dir = tempfile::TempDir::new().expect("a scratch directory"); + let sessions = Arc::new(SessionManager::new(dir.path().join("sessions"))); + let permissions = Arc::new(PermissionManager::new(dir.path().join("config"))); + let agent = Arc::new(Agent::with_config(AgentConfig::new( + Arc::clone(&sessions), + Arc::clone(&permissions), + None, + mode, + ))); + agent + .add_extension(ExtensionConfig::Builtin { + name: "developer".into(), + description: "developer".into(), + display_name: Some("Developer".into()), + timeout: Some(300), + bundled: Some(true), + available_tools: vec![], + }) + .await + .expect("enable developer"); + agent + .add_extension(ExtensionConfig::Platform { + name: "code_execution".into(), + description: "code execution".into(), + bundled: Some(true), + available_tools: vec![], + }) + .await + .expect("enable code_execution"); + let session = sessions + .create_session( + dir.path().to_path_buf(), + "script gate".into(), + SessionType::User, + ) + .await + .expect("a session"); + permissions.update_user_permission(EXECUTE_CODE, PermissionLevel::AlwaysAllow); + // A card left over from another test that minted the same session id + // (they are `YYYYMMDD_N` per database) must not be read as ours. + ActionRequiredManager::global().drain_requests(&session.id); + Fixture { + agent, + session, + permissions, + dir, + } + } + + fn text_of(result: &CallToolResult) -> String { + result + .content + .iter() + .filter_map(|content| match &content.raw { + RawContent::Text(text) => Some(text.text.clone()), + _ => None, + }) + .collect::>() + .join("\n") + } + + /// Dispatch `execute_code` through [`Agent::dispatch_tool_call`] — the one + /// function every model-initiated call reaches, approved or not — and drive + /// the script's body on its own task, the way the reply loop's batch does. + /// + /// ⚠ Deliberately NOT `ExtensionManager::dispatch_tool_call`: that is the + /// path a PERSON drives (`POST /agent/call_tool`), and it is the agent's + /// dispatch that has to carry the script's judge down to its calls. + async fn run_script( + f: &Fixture, + code: &str, + cancel: CancellationToken, + ) -> tokio::task::JoinHandle<(bool, String)> { + let call = CallToolRequestParams { + task: None, + meta: None, + name: EXECUTE_CODE.into(), + arguments: Some(object!({ "code": code })), + }; + let (_, dispatched) = f + .agent + .dispatch_tool_call(call, "outer-execute-code".into(), Some(cancel), &f.session) + .await; + let dispatched = dispatched.expect("execute_code dispatches"); + tokio::spawn(async move { + let result = dispatched + .result + .await + .expect("execute_code returns a result"); + (result.is_error.unwrap_or(false), text_of(&result)) + }) + } + + struct Card { + id: String, + tool_name: String, + arguments: JsonObject, + prompt: Option, + } + + /// The next approval card published for this session, or `None` if the + /// script finished without one. + /// + /// Raced against the script itself rather than a bare timeout: a script + /// that runs its call without asking finishes, and waiting out a clock + /// after that proves nothing but patience. + async fn card_or_completion( + session_id: &str, + script: &mut tokio::task::JoinHandle<(bool, String)>, + ) -> Result { + let deadline = tokio::time::Instant::now() + Duration::from_secs(60); + loop { + for message in ActionRequiredManager::global().drain_requests(session_id) { + for content in &message.content { + let MessageContent::ActionRequired(action) = content else { + continue; + }; + if let ActionRequiredData::ToolConfirmation { + id, + tool_name, + arguments, + prompt, + .. + } = &action.data + { + return Ok(Card { + id: id.clone(), + tool_name: tool_name.clone(), + arguments: arguments.clone(), + prompt: prompt.clone(), + }); + } + } + } + if script.is_finished() { + return Err(script.await.expect("the script task completes")); + } + assert!( + tokio::time::Instant::now() < deadline, + "neither an approval card nor a finished script within 60s" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } + } + + async fn answer(f: &Fixture, card: &Card, permission: Permission) { + let outcome = f + .agent + .handle_confirmation_for_session( + &f.session.id, + card.id.clone(), + PermissionConfirmation { + principal_type: PrincipalType::Tool, + permission, + }, + DecisionAuthority::unproven(), + ) + .await; + assert_eq!( + outcome, + crate::agents::ConfirmationOutcome::Delivered, + "the card's decision must reach the parked call" + ); + } + + async fn finish(script: tokio::task::JoinHandle<(bool, String)>) -> (bool, String) { + tokio::time::timeout(Duration::from_secs(60), script) + .await + .expect("the script finishes once answered") + .expect("the script task completes") + } + + /// F7's headline, measured the way the QA run measured it: Manual mode, the + /// script itself on the user's always-allow list, and a shell call inside + /// it. The shell call is not on that list, so it must be put to the user — + /// on a card that names `developer__shell` and carries the command — and it + /// must run once they allow it. + #[tokio::test] + #[serial_test::serial] + async fn manual_mode_asks_for_a_scripts_shell_call_under_the_inner_tools_name() { + let f = fixture(BioRouterMode::Approve).await; + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-ALLOWED" }));"#, + CancellationToken::new(), + ) + .await; + + let card = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => card, + Err((_, output)) => panic!( + "the script's developer__shell call ran with no approval card, because the \ + always-allowed script vouched for it: {output}" + ), + }; + assert_eq!( + card.tool_name, SHELL, + "the card must name the tool the script called, not the script" + ); + assert_eq!( + card.arguments.get("command").and_then(|v| v.as_str()), + Some("echo SCRIPT-GATE-ALLOWED"), + "the card must carry the call's own evaluated arguments" + ); + + answer(&f, &card, Permission::AllowOnce).await; + let (is_error, output) = finish(script).await; + assert!(!is_error, "an allowed call runs: {output}"); + assert!( + output.contains("SCRIPT-GATE-ALLOWED"), + "the allowed shell call's output reaches the script: {output}" + ); + } + + /// The other answer on the same card: a denial comes back INTO the script + /// as a tool error it can catch, and the script goes on — it is not ended + /// silently, and the command never runs. + #[tokio::test] + #[serial_test::serial] + async fn a_denied_card_is_a_catchable_tool_error_and_the_script_goes_on() { + let f = fixture(BioRouterMode::Approve).await; + let marker = f.dir.path().join("denied-marker"); + let code = format!( + r#"import {{ shell }} from "developer"; + let caught = null; + try {{ shell({{ command: "touch '{marker}'" }}); }} + catch (e) {{ caught = String(e); }} + record_result({{ caught, continued: true }});"#, + marker = marker.display() + ); + let mut script = run_script(&f, &code, CancellationToken::new()).await; + + let card = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => card, + Err((_, output)) => { + panic!("the script's developer__shell call ran with no approval card: {output}") + } + }; + assert_eq!(card.tool_name, SHELL); + answer(&f, &card, Permission::DenyOnce).await; + + let (is_error, output) = finish(script).await; + assert!(!is_error, "the script caught the refusal and finished: {output}"); + assert!( + output.contains("\"continued\":true"), + "the script must go on past a refused call: {output}" + ); + assert!( + output.contains(SHELL) && output.contains("declined"), + "the error the script caught must name the refused tool and say why: {output}" + ); + assert!(!marker.exists(), "a denied command must not have run"); + } + + /// `always_deny` is keyed by the INNER tool's name, exactly as it is for a + /// direct call: the refusal needs no card, it is an error the script can + /// catch, and the script's next call is judged on its own name. + #[tokio::test] + #[serial_test::serial] + async fn always_deny_on_the_inner_tool_refuses_it_and_the_script_continues() { + let f = fixture(BioRouterMode::Approve).await; + f.permissions + .update_user_permission(SHELL, PermissionLevel::NeverAllow); + f.permissions + .update_user_permission("developer__text_editor", PermissionLevel::AlwaysAllow); + let marker = f.dir.path().join("never-marker"); + // The developer server's path jail is the process working directory + // here, which `cargo test` sets to this crate's root — so the next call + // reads a file that is certainly inside it. + let code = format!( + r#"import {{ shell, text_editor }} from "developer"; + let caught = null; + try {{ shell({{ command: "touch '{marker}'" }}); }} + catch (e) {{ caught = String(e); }} + const after = text_editor({{ command: "view", path: "Cargo.toml" }}); + record_result({{ caught, after }});"#, + marker = marker.display(), + ); + let mut script = run_script(&f, &code, CancellationToken::new()).await; + + let (is_error, output) = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => panic!( + "a call the user always denies must be refused without a card, got one for {}", + card.tool_name + ), + Err(finished) => finished, + }; + assert!(!is_error, "the script caught the refusal and finished: {output}"); + assert!( + output.contains(SHELL) && output.contains("declined"), + "the always-denied call must come back into the script as a tool error: {output}" + ); + assert!( + output.contains("[package]"), + "the script's next call must still run: {output}" + ); + assert!(!marker.exists(), "an always-denied command must not have run"); + } + + /// Auto mode is unchanged: the same script call runs, and no card is + /// raised for it. + #[tokio::test] + #[serial_test::serial] + async fn auto_mode_runs_a_scripts_shell_call_with_no_card() { + let f = fixture(BioRouterMode::Auto).await; + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-AUTO" }));"#, + CancellationToken::new(), + ) + .await; + + let (is_error, output) = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => panic!("Auto mode raised a card for {}", card.tool_name), + Err(finished) => finished, + }; + assert!(!is_error, "{output}"); + assert!(output.contains("SCRIPT-GATE-AUTO"), "{output}"); + } +} From 3987832ad8c362b6ee7605ae1f4bbcb0a71c331d Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:31:47 -0700 Subject: [PATCH 02/11] fix(code-execution): judge every call a script makes as the call it is (F7) QA finding F7: with `code_execution__execute_code` always-allowed, a script's inner calls ran with no card in Manual mode, because the only call the permission system judged was the one the agent loop dispatched. The script's own calls went straight from the JS sandbox to `ExtensionManager::dispatch_tool_call`, where no inspector runs. Now each call a script makes faces the decision a direct call would: - `Agent::dispatch_tool_call` builds a `ScriptCallGate` (the agent's own inspector stack, mode, session and hooks) for an `execute_code` call and runs the tool's BODY inside a task-local scope; `execute_code` reads it and hands it to the task that dispatches the script's calls. - Every call is inspected on its evaluated arguments by the same inspectors (repetition excluded: a script's loop is not the model repeating itself), on the capability `execute_code` was admitted on (threaded, never resampled). PreToolUse rewrites are applied and re-judged; staged hook context is dropped because a running script has no channel for it (the bridge's reasoning). - The permission inspector grades a script's call from the script's own catalogue (`inspect_graded`), because the agent's registry is graded from the model's roster, which in Code Execution mode holds none of these tools. - A denial throws into the script as a tool error it can catch; an ask parks the same card a direct call gets, naming the inner tool and its arguments, after PermissionRequest hooks; Always allow / Always deny are recorded under the inner tool's name. - The uninspected-boundary refusals still run first, so nothing they refuse can become a card; `execute_code` itself is judged exactly as before. - `no_human_surface` is carried into the spawned sub-call task, so an ask in a scheduled run is refused at once instead of parking for its TTL. `handle_denied_tools`' inline text match moves to `denied_response_text` so a refusal reads the same for a direct call and a script's call. Tests: the four F7 tests from the previous commit now pass, plus Smart-mode catalogue grading, always-allow-by-inner-name, Always Allow recorded under the inner name, nobody-to-ask refused at once, and a boundary refusal never becoming a card. --- crates/biorouter/src/agents/agent.rs | 73 +- .../src/agents/code_execution_extension.rs | 254 ++++- .../biorouter/src/agents/script_call_gate.rs | 886 +++++++++++++++++- crates/biorouter/src/agents/tool_execution.rs | 58 +- .../src/permission/permission_inspector.rs | 77 +- crates/biorouter/src/tool_inspection.rs | 99 +- 6 files changed, 1315 insertions(+), 132 deletions(-) diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index f471c0ea3..ef89b8510 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -20,7 +20,7 @@ use super::platform_tools; /// a classifier that silently stops matching, and a scheduled run that stops /// matching is one that reports success for having done nothing. pub(crate) use super::tool_execution::EXPIRED_RESPONSE; -use super::tool_execution::{ToolCallResult, CHAT_MODE_TOOL_SKIPPED_RESPONSE, DECLINED_RESPONSE}; +use super::tool_execution::{ToolCallResult, CHAT_MODE_TOOL_SKIPPED_RESPONSE}; use super::turn_abort::TurnAbortCode; use crate::action_required_manager::ActionRequiredManager; use crate::agents::budget::{BudgetAction, BudgetTracker, ReplyBudget}; @@ -6369,51 +6369,8 @@ impl Agent { ) { for request in &permission_check_result.denied { if let Some(response_msg) = request_to_response_map.get(&request.id) { - // When an inspector denied this call, tell the model why so it - // can adjust instead of blindly retrying. The always-on - // catastrophic-command block (security inspector) and hook denials - // carry a reason; surface it verbatim / with context. - let deny_reason = inspection_results.iter().find(|result| { - result.tool_request_id == request.id - && result.action == InspectionAction::Deny - && !result.reason.trim().is_empty() - }); - let response_text = match deny_reason { - Some(result) - if result.inspector_name - == crate::hooks::inspector::HOOK_INSPECTOR_NAME => - { - format!("{DECLINED_RESPONSE}\n\nHook feedback: {}", result.reason) - } - // Non-bypassable safety block: the user did not decline, the - // command is refused outright, so return the reason directly. - Some(result) if result.inspector_name == "security" => result.reason.clone(), - // BR-29/BR-31: a loop guard tripped — the call repeated - // itself, or the tool has been failing the same way over and - // over. The user did not decline anything; telling the model - // they did (the old DECLINED_RESPONSE) is actively misleading - // and leaves it unable to diagnose the stop. Return the real - // reason. - Some(result) - if result.inspector_name - == crate::tool_monitor::REPETITION_INSPECTOR_NAME => - { - result.reason.clone() - } - // #63: a cross-session memory shape Biorouter refuses (the - // whole-store global read). Same reasoning as the loop - // guards above — the user declined nothing, and the reason - // is the only thing that tells the model the itemised call - // still works. `DECLINED_RESPONSE` here would be both untrue - // and unactionable, and would read as the feature being off. - Some(result) - if result.inspector_name - == crate::security::global_memory::GLOBAL_MEMORY_INSPECTOR_NAME => - { - result.reason.clone() - } - _ => DECLINED_RESPONSE.to_string(), - }; + let response_text = + super::tool_execution::denied_response_text(&request.id, inspection_results); let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( request.id.clone(), @@ -7469,6 +7426,23 @@ impl Agent { let exec_tool_name = tool_call.name.to_string(); let exec_request_id = request_id.clone(); + // QA finding F7: the calls a Code Execution script makes are judged by + // this agent's own inspector stack, in this agent's mode, exactly as its + // direct calls are — see `script_call_gate`. Built here, where `self` + // still is, and installed below around the tool's BODY: a scope around + // this function alone would be gone before the script ran. + let script_gate = super::code_execution_extension::is_execute_code_call( + tool_call.name.as_ref(), + ) + .then(|| { + Arc::new(super::script_call_gate::ScriptCallGate::new( + Arc::clone(&self.tool_inspection_manager), + self.config.biorouter_mode, + session.clone(), + Arc::clone(&self.hooks_manager), + )) + }); + ( request_id, Ok(ToolCallResult { @@ -7507,7 +7481,12 @@ impl Agent { id = %exec_request_id, "TOOL_EXEC_START" ); - let inner_result = inner.await; + let inner_result = match script_gate { + Some(gate) => { + super::script_call_gate::judging_script_calls(gate, inner).await + } + None => inner.await, + }; let dur_ms = exec_started.elapsed().as_millis() as u64; debug!( name = %exec_tool_name, diff --git a/crates/biorouter/src/agents/code_execution_extension.rs b/crates/biorouter/src/agents/code_execution_extension.rs index 499e9f53f..2d74bd55a 100644 --- a/crates/biorouter/src/agents/code_execution_extension.rs +++ b/crates/biorouter/src/agents/code_execution_extension.rs @@ -90,6 +90,35 @@ type ToolCallRequest = ( tokio::sync::oneshot::Sender>, ); +/// Whether a dispatched name reaches this extension's `execute_code`: the +/// prefixed form, or the bare one `ExtensionManager::prefixed_tool_name` +/// resolves, because models strip prefixes. The agent loop asks this to decide +/// which of its calls need a judge for the calls they make (QA finding F7). +pub(crate) fn is_execute_code_call(name: &str) -> bool { + name == "execute_code" + || name + .strip_prefix(EXTENSION_NAME) + .and_then(|rest| rest.strip_prefix("__")) + == Some("execute_code") +} + +/// QA finding F7: the agent's judge for one script's calls, and the grades of +/// the script's own catalogue it judges them with. See `script_call_gate`. +struct ScriptJudge { + gate: Arc, + risks: crate::permission::tool_risk::ToolRiskRegistry, +} + +/// A script's call refused before dispatch, by whichever check owed it. +struct PreDispatchRefusal { + /// The executed-calls record's failure class. + kind: &'static str, + /// User-safe text for the executed-calls view; `None` shows only `kind`. + user_note: Option<&'static str>, + /// What the script is told. + error: String, +} + struct SandboxHooks; impl HostHooks for SandboxHooks { @@ -1776,6 +1805,20 @@ impl CodeExecutionClient { &self, admitted: Option, ) -> Vec { + self.get_catalogue(admitted) + .await + .iter() + .filter_map(ToolInfo::from_mcp_tool) + .collect() + } + + /// The catalogue as MCP tools, annotations included — what + /// [`Self::get_tool_infos`] renders into import bindings, and what a script's + /// calls are risk-graded from (QA finding F7, `script_call_gate`). + async fn get_catalogue( + &self, + admitted: Option, + ) -> Vec { let Some(manager) = self .context .extension_manager @@ -1785,15 +1828,10 @@ impl CodeExecutionClient { return Vec::new(); }; - match manager + manager .get_prefixed_tools_excluding(EXTENSION_NAME, admitted) .await - { - Ok(tools) if !tools.is_empty() => { - tools.iter().filter_map(ToolInfo::from_mcp_tool).collect() - } - _ => Vec::new(), - } + .unwrap_or_default() } async fn handle_execute_code( @@ -1810,10 +1848,30 @@ impl CodeExecutionClient { .ok_or("Missing required parameter: code")? .to_string(); - let tools = self.get_tool_infos(Some(cap)).await; + let catalogue = self.get_catalogue(Some(cap)).await; + let tools: Vec = catalogue + .iter() + .filter_map(ToolInfo::from_mcp_tool) + .collect(); + // QA finding F7: the judge the agent loop installed around this call, + // if it was the agent loop that dispatched it. Read HERE, on the task + // the scope covers — the handler below is spawned, and a task-local does + // not follow a spawn. See `script_call_gate`. + let judge = crate::agents::script_call_gate::current().map(|gate| { + let risks = crate::permission::tool_risk::ToolRiskRegistry::new(); + // Graded from the exact list the script's imports are built from, + // so every call it can make has its own tool's grade. + risks.refresh_from_tools(&catalogue); + ScriptJudge { gate, risks } + }); + // …and whether a person can be asked at all. Also a task-local, also + // lost across the spawn: without this a scheduled run's script would + // park an ask nobody can answer until its time-to-live, where every other + // ask in that run is refused at once (`user_surface`). + let no_human_surface = crate::user_surface::no_human_surface(); let collected_artifacts = Arc::new(Mutex::new(CollectedArtifacts::default())); let (call_tx, call_rx) = mpsc::unbounded_channel(); - let tool_handler = tokio::spawn(Self::run_tool_handler( + let handler = Self::run_tool_handler( session_id.to_string(), // Issue #56: the capability this `execute_code` call was admitted // on, carried down to every sub-call the script makes. The bridge @@ -1821,11 +1879,19 @@ impl CodeExecutionClient { // is nothing here it could sample even if it wanted to — which is // the point: a script's tool call inherits the script's permission. cap, + judge, call_rx, self.context.extension_manager.clone(), Arc::clone(&collected_artifacts), cancellation_token.clone(), - )); + ); + let tool_handler = tokio::spawn(async move { + if no_human_surface { + crate::user_surface::without_human_surface(handler).await; + } else { + handler.await; + } + }); let js_task = tokio::task::spawn_blocking(move || run_js_module(&code, &tools, call_tx)); let js_result = tokio::select! { @@ -2204,15 +2270,20 @@ impl CodeExecutionClient { /// Refuse one sub-call before it is dispatched: record the failure for /// telemetry and hand the script its error. /// - /// The two pre-dispatch guards (the tool-call limit and the global-memory - /// consent boundary) do the same three things in the same order, and both - /// must record *before* answering the script — a refusal the record misses - /// is a call the transparency view never shows. + /// The pre-dispatch guards (the tool-call limit, the uninspected-boundary + /// refusals and the permission judge) do the same three things in the same + /// order, and all must record *before* answering the script — a refusal the + /// record misses is a call the transparency view never shows. + /// + /// `user_note` is what the executed-calls view shows. It must already be safe + /// to show the user — see [`ToolCallRecord::failed`]; `None` shows only the + /// failure class. async fn refuse_sub_call( collected_artifacts: &Arc>, tool_name: &str, arguments: &str, failure_kind: &'static str, + user_note: Option<&str>, error: String, response_tx: tokio::sync::oneshot::Sender>, ) { @@ -2222,12 +2293,55 @@ impl CodeExecutionClient { .push_tool_call(ToolCallRecord::failed( tool_name, arguments, - None, + user_note, failure_kind, )); let _ = response_tx.send(Err(error)); } + /// QA finding F7: put one script call to the agent's judge, if there is one. + /// + /// `Ok` is the arguments to dispatch — the script's own, or a PreToolUse + /// hook's rewrite of them, because what runs is what was judged. `Err` is the + /// refusal the script gets instead, as the same sentence a direct call gets. + /// + /// No judge means no agent loop dispatched this script (see + /// `script_call_gate::current`), and the call proceeds as it always has. + async fn judged_arguments( + judge: Option<&ScriptJudge>, + cap: crate::privacy::CallCapability, + tool_name: &str, + arguments: String, + cancellation_token: &CancellationToken, + ) -> Result { + use crate::agents::script_call_gate::ScriptCallVerdict; + + let Some(judge) = judge else { + return Ok(arguments); + }; + // Parsed exactly as `dispatch_sub_call` parses them, so the call that is + // judged is the call that would be dispatched. + let parsed: Option = serde_json::from_str(&arguments).ok(); + let call = CallToolRequestParams { + task: None, + name: tool_name.to_string().into(), + arguments: parsed.clone(), + meta: None, + }; + match judge + .gate + .judge(call, cap, &judge.risks, cancellation_token) + .await + { + ScriptCallVerdict::Run(approved) if approved.arguments == parsed => Ok(arguments), + ScriptCallVerdict::Run(approved) => Ok(serde_json::to_string( + &approved.arguments.unwrap_or_default(), + ) + .unwrap_or(arguments)), + ScriptCallVerdict::Refuse(refusal) => Err(refusal), + } + } + /// Dispatch one sub-call and report how it went. /// /// Returns the script-facing result separately from user-visible telemetry. @@ -2385,9 +2499,65 @@ impl CodeExecutionClient { None } + /// Every check a script's call passes before it is dispatched, in order. + /// + /// `Ok` is the arguments to dispatch. `Err` is the one refusal the loop + /// answers with, from whichever check owed it — one refusal branch, however + /// many checks feed it. + async fn admit_sub_call( + session_id: &str, + cap: crate::privacy::CallCapability, + judge: Option<&ScriptJudge>, + tool_name: &str, + arguments: &str, + cancellation_token: &CancellationToken, + ) -> Result { + // Issue #63 review, finding 3. A script's tool calls go straight to + // the extension manager below, so no `ToolInspector` — the + // global-memory consent gate included — ever sees them. The gate + // compensated by scanning the *script text* for an embedded memory + // call, which a runtime-assembled call walks past + // (`is_global: flag`). This is the same decision taken where there + // is nothing left to compute: the dispatched name and the evaluated + // arguments. A boundary that cannot ask the user refuses. + let evaluated = serde_json::from_str::(arguments).ok(); + let evaluated = evaluated.as_ref().and_then(serde_json::Value::as_object); + // Every boundary refusal this door owes, asked in one place. See + // `uninspected_boundary_refusal` for why a door that no + // `ToolInspector` reaches has to carry its own. + if let Some((kind, refusal)) = + Self::uninspected_boundary_refusal(cap, session_id, tool_name, evaluated).await + { + return Err(PreDispatchRefusal { + kind, + user_note: None, + error: refusal, + }); + } + // QA finding F7: the call faces the permission decision it would face + // as a direct call — the agent's inspectors, its mode, and the user's + // allow/deny entries under THIS tool's name — and an ask goes to the + // person on a card naming it. After the boundary refusals, so nothing + // they refuse becomes something a card can allow. + Self::judged_arguments( + judge, + cap, + tool_name, + arguments.to_string(), + cancellation_token, + ) + .await + .map_err(|refusal| PreDispatchRefusal { + kind: refusal.kind, + user_note: Some(refusal.user_note), + error: attribute_sub_call_error(tool_name, refusal.message), + }) + } + async fn run_tool_handler( session_id: String, cap: crate::privacy::CallCapability, + judge: Option, mut call_rx: mpsc::UnboundedReceiver, extension_manager: Option>, collected_artifacts: Arc>, @@ -2415,39 +2585,38 @@ impl CodeExecutionClient { &tool_name, &arguments, "call_limit", + None, format!("JavaScript exceeded the {MAX_JS_TOOL_CALLS} tool-call limit"), response_tx, ) .await; continue; } - // Issue #63 review, finding 3. A script's tool calls go straight to - // the extension manager below, so no `ToolInspector` — the - // global-memory consent gate included — ever sees them. The gate - // compensated by scanning the *script text* for an embedded memory - // call, which a runtime-assembled call walks past - // (`is_global: flag`). This is the same decision taken where there - // is nothing left to compute: the dispatched name and the evaluated - // arguments. A boundary that cannot ask the user refuses. - let evaluated = serde_json::from_str::(&arguments).ok(); - let evaluated = evaluated.as_ref().and_then(serde_json::Value::as_object); - // Every boundary refusal this door owes, asked in one place. See - // `uninspected_boundary_refusal` for why a door that no - // `ToolInspector` reaches has to carry its own. - if let Some((kind, refusal)) = - Self::uninspected_boundary_refusal(cap, &session_id, &tool_name, evaluated).await + let arguments = match Self::admit_sub_call( + &session_id, + cap, + judge.as_ref(), + &tool_name, + &arguments, + &cancellation_token, + ) + .await { - Self::refuse_sub_call( - &collected_artifacts, - &tool_name, - &arguments, - kind, - refusal, - response_tx, - ) - .await; - continue; - } + Ok(admitted) => admitted, + Err(refusal) => { + Self::refuse_sub_call( + &collected_artifacts, + &tool_name, + &arguments, + refusal.kind, + refusal.user_note, + refusal.error, + response_tx, + ) + .await; + continue; + } + }; let (result, mut failure_kind, user_error, todo_task) = Self::dispatch_sub_call( &session_id, cap, @@ -3421,6 +3590,7 @@ mod tests { let handler = tokio::spawn(CodeExecutionClient::run_tool_handler( "cancel-session".to_string(), crate::privacy::CallCapability::for_test_restricted(), + None, call_rx, None, Arc::clone(&collected), @@ -3442,6 +3612,7 @@ mod tests { let handler = tokio::spawn(CodeExecutionClient::run_tool_handler( "telemetry-session".to_string(), crate::privacy::CallCapability::for_test_restricted(), + None, call_rx, None, Arc::clone(&collected), @@ -3567,6 +3738,7 @@ mod tests { let handler = tokio::spawn(CodeExecutionClient::run_tool_handler( session.id, crate::privacy::CallCapability::for_test_restricted(), + None, call_rx, Some(Arc::downgrade(&manager)), Arc::clone(&collected), diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index f7f2667cd..645878295 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -1,5 +1,593 @@ //! The permission decision for every tool call a Code Execution script makes //! (QA finding F7, 2026-09-10). +//! +//! # The defect +//! +//! In Code Execution mode — the shipped default — the model's directly callable +//! roster collapses to `code_execution__*` and a handful of exemptions +//! (`reply_parts::survives_code_execution_filter`); every other tool is reached +//! by writing JavaScript inside `execute_code`. The permission system judged the +//! call the agent loop DISPATCHED, which was `execute_code`, and nothing below +//! it: the script's own calls went from the JS sandbox straight to +//! `ExtensionManager::dispatch_tool_call`, where no [`ToolInspector`] runs. So +//! one approval of the script — or one `always_allow` entry for +//! `code_execution__execute_code` — covered every call it made, in every mode. +//! Measured in Manual mode: `echo` through `developer__shell` and a +//! `developer__analyze` that was on no allow list both ran without a card. +//! +//! # The rule +//! +//! **A call a script makes faces the decision it would face as a direct call.** +//! Concretely, for each call the sandbox hands over: +//! +//! 1. every inspector the agent loop runs, on the call's *evaluated* arguments, +//! in the agent's own mode and on the capability the `execute_code` call was +//! admitted on — managed policy, the security floor, sensitive operations, +//! the memory / session-store / knowledge-delete gates, workspace mutation, +//! the user's PreToolUse hooks (with their rewrites re-judged) and the +//! permission inspector, whose `always_allow` / `never_allow` / scoped grants +//! are keyed by the INNER tool's name; +//! 2. a denial comes back into the script as a tool error it can catch; +//! 3. an ask goes to the person on the same card a direct call gets — naming the +//! inner tool and carrying its arguments — after the user's PermissionRequest +//! hooks have had the chance to answer it, exactly as for a direct call; and +//! "Always allow" / "Always deny" on that card are recorded under the inner +//! tool's name. +//! +//! `code_execution__execute_code` itself is judged exactly as before, so nothing +//! that asked before stops asking: an `always_allow` entry for it still means +//! "do not ask me before running a script". What it no longer means is "…and +//! allow everything the script calls". +//! +//! # What is deliberately different from a direct call +//! +//! * **No loop guard.** The repetition inspector watches the MODEL's call stream +//! for a model repeating itself; a script's loop is the script doing its job, +//! and the `execute_code` call that contains it already passed the guard. +//! * **No conversation history.** The one inspector that reads history is +//! sensitive-ops' criterion-5 provenance, and it reads it only to EXEMPT a +//! repository this session demonstrably created. Without history it exempts +//! nothing, so a script's recursive delete of such a repository asks where the +//! same direct call might not — the direction this gate may err in. +//! * **No approval delegation** (`approval_relay`). A direct call in an +//! agent-created session may be answered by the agent that created it; a +//! script's ask always goes to a person. Asking a person instead of an agent +//! is never the weaker answer. +//! * **A hook's context is dropped, not injected.** A PreToolUse or +//! PermissionRequest hook's `additionalContext` / `systemMessage` has no +//! channel into a script that is still running, and left staged it would leak +//! into a later turn as context about a call that finished long ago — the +//! coding-agent bridge's reasoning, verbatim (`BridgeGrant::call`). +//! +//! # How the judge reaches the script +//! +//! [`Agent::dispatch_tool_call`] builds a [`ScriptCallGate`] for an +//! `execute_code` call and runs the TOOL BODY — the future it returns, not the +//! dispatch that builds it — inside [`judging_script_calls`]. `execute_code` +//! reads it with [`current`] and hands it to the task that dispatches the +//! script's calls. Absent means the caller is not the agent loop: `POST +//! /agent/call_tool`, which a person drives and which bypasses every inspector +//! for the outer call too. There is no model decision to gate there, so a +//! script run that way behaves exactly as it always has. +//! +//! [`ToolInspector`]: crate::tool_inspection::ToolInspector +//! [`Agent::dispatch_tool_call`]: crate::agents::Agent::dispatch_tool_call + +use std::sync::Arc; +use std::time::Duration; + +use rmcp::model::{CallToolRequestParams, JsonObject}; +use serde_json::Value; +use tokio_util::sync::CancellationToken; + +use crate::config::permission::PermissionLevel; +use crate::config::BioRouterMode; +use crate::conversation::message::ToolRequest; +use crate::conversation::tool_preview::ToolPreview; +use crate::hooks::{HookDecision, HookEvent, HookPayload, HooksManager}; +use crate::pending_user_action::{ + PendingUserActions, ToolApprovalRequest, UserActionOutcome, UserActionRequest, +}; +use crate::permission::tool_risk::ToolRiskRegistry; +use crate::permission::Permission; +use crate::privacy::CallCapability; +use crate::session::Session; +use crate::tool_inspection::{InspectionResult, ToolInspectionManager}; + +use super::tool_execution::{ + denied_response_text, CANCELLED_RESPONSE, DECLINED_RESPONSE, EXPIRED_RESPONSE, +}; + +tokio::task_local! { + /// The judge for the calls a script makes, installed around the body of the + /// `execute_code` call that runs it. See the module header. + static SCRIPT_CALL_GATE: Arc; +} + +/// Run `tool_body` with `gate` judging any call a script makes inside it. +/// +/// ⚠ Wrap the tool's BODY. `dispatch_tool_call` returns a future, and a scope +/// around the dispatch alone is gone before the script runs — the shape of the +/// #160 hang, and of every scope bug in that family. +pub(crate) async fn judging_script_calls( + gate: Arc, + tool_body: F, +) -> F::Output { + SCRIPT_CALL_GATE.scope(gate, tool_body).await +} + +/// The gate installed around the tool body running on this task, if any. +pub(crate) fn current() -> Option> { + SCRIPT_CALL_GATE.try_with(Arc::clone).ok() +} + +/// Inspectors that do not judge a script's calls. See the module header. +const NOT_FOR_SCRIPT_CALLS: &[&str] = &[crate::tool_monitor::REPETITION_INSPECTOR_NAME]; + +/// …plus the hook inspector, when a PreToolUse rewrite is being re-judged: +/// re-running it would execute the user's hook commands a second time and let a +/// rewrite trigger another rewrite (BR-19, as on the agent's own path). +const NOT_FOR_A_REWRITE: &[&str] = &[ + crate::tool_monitor::REPETITION_INSPECTOR_NAME, + crate::hooks::inspector::HOOK_INSPECTOR_NAME, +]; + +/// What the script's call gets. +#[derive(Debug)] +pub(crate) enum ScriptCallVerdict { + /// Dispatch exactly this call. It may differ from what the script asked + /// for: a PreToolUse hook may have rewritten its arguments, and what runs is + /// what was judged. + Run(CallToolRequestParams), + /// Do not dispatch it. + Refuse(ScriptCallRefusal), +} + +/// A call the gate did not let through. +#[derive(Debug)] +pub(crate) struct ScriptCallRefusal { + /// The telemetry label on the script's executed-calls record. + pub(crate) kind: &'static str, + /// What the script (and, if the script does not catch it, the model) is + /// told. The same sentence a direct call would get. + pub(crate) message: String, + /// What the user's executed-calls view says, in their terms. Never the + /// arguments and never a hook's free-form text. + pub(crate) user_note: &'static str, +} + +impl ScriptCallRefusal { + fn new(kind: &'static str, message: impl Into, user_note: &'static str) -> Self { + Self { + kind, + message: message.into(), + user_note, + } + } +} + +/// Everything one `execute_code` call's judge needs, taken from the agent that +/// dispatched it. +/// +/// A snapshot rather than a handle back to the `Agent`, for the reason the +/// coding-agent bridge's grant is one: the script's calls are dispatched from a +/// task the agent does not own, and a judge that outlived its call would be an +/// authority with no owner. +pub struct ScriptCallGate { + /// The agent's own inspector stack — the same `Arc` its direct calls use, + /// so a user's "Always allow" recorded here is the one they will see there. + inspections: Arc, + /// The agent's mode. `Agent::config.biorouter_mode` is the value the reply + /// loop hands its own inspectors, so a script's calls and the loop's cannot + /// be judged in two different modes. + mode: BioRouterMode, + /// The session whose turn dispatched the script: its working directory for + /// path arguments, and its id as the only surface an ask may be put on. + session: Session, + /// For the PreToolUse rewrites this gate's own inspection staged, and the + /// PermissionRequest hooks consulted before a card. + hooks: Arc, +} + +impl ScriptCallGate { + pub(crate) fn new( + inspections: Arc, + mode: BioRouterMode, + session: Session, + hooks: Arc, + ) -> Self { + Self { + inspections, + mode, + session, + hooks, + } + } + + /// Decide one call a script made. + /// + /// `capability` is the one the `execute_code` call was admitted on, threaded + /// down rather than sampled (issue #56). `risks` grades the script's own + /// catalogue. `cancel` is the turn's token: Stop releases a parked ask. + pub(crate) async fn judge( + &self, + call: CallToolRequestParams, + capability: CallCapability, + risks: &ToolRiskRegistry, + cancel: &CancellationToken, + ) -> ScriptCallVerdict { + // Minted here so every exit below — refusals included — goes through the + // drain, because every exit ran the user's hooks. + let request_id = format!("script_call_{}", uuid::Uuid::new_v4()); + let verdict = self + .judge_one(request_id.clone(), call, capability, risks, cancel) + .await; + self.discard_staged_hook_context(&request_id); + verdict + } + + async fn judge_one( + &self, + request_id: String, + call: CallToolRequestParams, + capability: CallCapability, + risks: &ToolRiskRegistry, + cancel: &CancellationToken, + ) -> ScriptCallVerdict { + let name = call.name.to_string(); + let mut requests = vec![ToolRequest { + id: request_id, + tool_call: Ok(call), + metadata: None, + tool_meta: None, + }]; + + let mut inspections = match self + .inspections + .inspect_script_calls( + NOT_FOR_SCRIPT_CALLS, + &requests, + &[], + self.mode, + &self.session, + capability, + risks, + ) + .await + { + Ok(inspections) => inspections, + Err(error) => return unjudged(&name, &error.to_string()), + }; + if let Err(error) = self + .collect_hook_rewrites(&mut requests, &mut inspections, capability, risks) + .await + { + return unjudged(&name, &error.to_string()); + } + + // No permission decision must never read as approval. + let Some(decision) = self + .inspections + .process_inspection_results_with_permission_inspector(&requests, &inspections) + else { + return unjudged(&name, "no permission decision was reached"); + }; + + if let Some(denied) = decision.denied.first() { + return ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "permission_denied", + denied_response_text(&denied.id, &inspections), + "Not run: refused by your tool permissions", + )); + } + // What runs is what was judged — taken out of the verdict, not out of + // the script's request, so a hook rewrite cannot be undone here. + if let Some(approved) = decision.approved.into_iter().next() { + return match approved.tool_call { + Ok(call) => ScriptCallVerdict::Run(call), + Err(error) => unjudged(&name, &error.to_string()), + }; + } + let Some(pending) = decision.needs_approval.into_iter().next() else { + return unjudged(&name, "the call was neither allowed, denied nor put to you"); + }; + self.ask_a_person(pending, &inspections, risks, cancel) + .await + } + + /// BR-19, on the script's path: apply what the user's PreToolUse hooks asked + /// to rewrite, and judge the rewritten call again. + /// + /// Scoped to this call's own request id, never the session's whole buffer: + /// a turn can run several scripts at once, and a session-wide take would + /// steal a sibling's rewrite (the bridge learned this — see + /// `BridgeGrant::collect_hook_rewrites`). + async fn collect_hook_rewrites( + &self, + requests: &mut [ToolRequest], + inspections: &mut Vec, + capability: CallCapability, + risks: &ToolRiskRegistry, + ) -> anyhow::Result<()> { + let ids: Vec = requests.iter().map(|request| request.id.clone()).collect(); + let rewrites = self + .hooks + .take_tool_input_rewrites_for(&self.session.id, &ids); + if rewrites.is_empty() || crate::hooks::apply_tool_input_rewrites(requests, &rewrites) == 0 + { + return Ok(()); + } + let mut revalidated = self + .inspections + .inspect_script_calls( + NOT_FOR_A_REWRITE, + requests, + &[], + self.mode, + &self.session, + capability, + risks, + ) + .await?; + inspections + .retain(|result| result.inspector_name == crate::hooks::inspector::HOOK_INSPECTOR_NAME); + inspections.append(&mut revalidated); + Ok(()) + } + + /// Put an ask to the person, the way the agent loop puts a direct call's. + async fn ask_a_person( + &self, + pending: ToolRequest, + inspections: &[InspectionResult], + risks: &ToolRiskRegistry, + cancel: &CancellationToken, + ) -> ScriptCallVerdict { + let call = match pending.tool_call { + Ok(call) => call, + Err(error) => return unjudged("the call", &error.to_string()), + }; + let name = call.name.to_string(); + let arguments: JsonObject = call.arguments.clone().unwrap_or_default(); + + if let Some(answered) = self + .answered_by_permission_request_hooks(&pending.id, &call, &arguments, inspections) + .await + { + return answered; + } + + // An approval is an authorization, so it needs the exact session that + // will display it and accept the answer — never an unscoped queue + // another session could claim (the bridge's rule, #40). + if self.session.id.is_empty() { + return ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_unavailable", + format!( + "`{name}` needs your approval, and this script is not running in a \ + conversation that can show an approval card. It was not run." + ), + "Not run: no conversation to ask in", + )); + } + self.notify_permission_prompt(&name); + + let request = UserActionRequest::ToolApproval(ToolApprovalRequest { + tool_name: name.clone(), + arguments: arguments.clone(), + prompt: Some(card_prompt( + crate::tool_inspection::approval_prompt_for_request(&pending.id, inspections), + )), + risk: Some(risks.risk_for(&name)), + preview: ToolPreview::for_tool_call(&name, &arguments), + // The same answer a direct call's card takes: any surface of this + // session may give it. Nothing about a script's call makes it an + // authorization a model must be proven unable to grant. + requires_user_proof: false, + }); + let parked = PendingUserActions::global().park(Some(&self.session.id), None, request); + let outcome = parked.wait(approval_ttl(), Some(cancel)).await; + self.verdict_for_answer(call, outcome, cancel).await + } + + /// The user's PermissionRequest hooks answer before any card, as they do for + /// a direct call (`handle_approval_tool_requests`) — except where a security + /// inspector raised the ask, which only a person may answer. `None` means + /// the ask still goes to the person. + async fn answered_by_permission_request_hooks( + &self, + request_id: &str, + call: &CallToolRequestParams, + arguments: &JsonObject, + inspections: &[InspectionResult], + ) -> Option { + let hook = self + .hooks + .permission_request( + &self.session.id, + &self.session.working_dir, + &call.name, + &Value::Object(arguments.clone()), + ) + .await; + let requires_a_human = + crate::tool_inspection::approval_requires_a_human(request_id, inspections); + match hook.decision { + Some(HookDecision::Allow { .. }) if requires_a_human => { + tracing::warn!( + counter.biorouter.non_delegable_approval_hook_ignored = 1, + tool_name = %call.name, + "PermissionRequest hook tried to auto-approve a security-raised approval \ + inside a script; asking the user instead" + ); + None + } + Some(HookDecision::Allow { .. }) => Some(ScriptCallVerdict::Run(call.clone())), + Some(HookDecision::Deny { reason }) => { + Some(ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "hook_denied", + format!("{DECLINED_RESPONSE}\n\nHook feedback: {reason}"), + "Not run: refused by your PermissionRequest hook", + ))) + } + Some(HookDecision::Ask { .. }) | None => None, + } + } + + /// Tell the user's Notification hooks a permission prompt is waiting — the + /// direct path does, and a desktop notification is how a user away from the + /// window learns a script is parked on them. + fn notify_permission_prompt(&self, name: &str) { + let mut payload = HookPayload::new( + HookEvent::Notification, + &self.session.id, + self.session.working_dir.to_string_lossy(), + ); + payload.message = Some(format!("Permission required for {name}")); + self.hooks.fire( + HookEvent::Notification, + Some("permission_prompt".to_string()), + payload, + self.session.working_dir.clone(), + ); + } + + /// What the person's answer — or the lack of one — means for the call. + async fn verdict_for_answer( + &self, + call: CallToolRequestParams, + outcome: UserActionOutcome, + cancel: &CancellationToken, + ) -> ScriptCallVerdict { + let name = call.name.to_string(); + match outcome { + UserActionOutcome::Approved { .. } if cancel.is_cancelled() => { + ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "cancelled", + CANCELLED_RESPONSE, + "Not run: the turn was stopped", + )) + } + UserActionOutcome::Approved { permission } => { + if permission == Permission::AlwaysAllow { + self.inspections + .update_permission_manager(&name, PermissionLevel::AlwaysAllow) + .await; + } + ScriptCallVerdict::Run(call) + } + UserActionOutcome::Denied { permission } => { + if permission == Permission::AlwaysDeny { + self.inspections + .update_permission_manager(&name, PermissionLevel::NeverAllow) + .await; + } + ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_declined", + DECLINED_RESPONSE, + "Not run: you declined it", + )) + } + UserActionOutcome::TimedOut => ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_expired", + EXPIRED_RESPONSE, + "Not run: the approval expired", + )), + UserActionOutcome::Cancelled if cancel.is_cancelled() => { + ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "cancelled", + CANCELLED_RESPONSE, + "Not run: the turn was stopped", + )) + } + // `park` answers Cancelled at once where nobody could ever answer — + // a scheduled run, say. Say so, rather than "cancelled", which reads + // as though someone decided. + UserActionOutcome::Cancelled if crate::user_surface::no_human_surface() => { + ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_unavailable", + format!( + "`{name}` needs a person's approval, and nobody can be asked in this \ + run, so it was not run. Do not retry it here; say what you needed it \ + for." + ), + "Not run: nobody could be asked", + )) + } + UserActionOutcome::Cancelled => ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_dismissed", + CANCELLED_RESPONSE, + "Not run: the approval was dismissed", + )), + // `Provided` / `SecretsConfigured` cannot answer a tool approval — + // `PendingUserActions` refuses them — so reaching here is `Failed`. + other => ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "approval_unavailable", + format!( + "`{name}` needed your approval, and the request {}. It was not run.", + other.refusal_detail() + ), + "Not run: the approval could not be shown", + )), + } + } + + /// A hook's staged context for this call has nowhere to go. See the module + /// header; logged at debug so "my hook's additionalContext never appeared" + /// has an answer. + fn discard_staged_hook_context(&self, request_id: &str) { + let dropped = self + .hooks + .drain_tool_hook_context_for(&self.session.id, &[request_id.to_string()]); + for staged in dropped { + if staged.additional_context.is_empty() && staged.system_messages.is_empty() { + continue; + } + tracing::debug!( + tool = %staged.tool_name, + context = staged.additional_context.len(), + messages = staged.system_messages.len(), + "a hook returned context for a tool call inside a Code Execution script; \ + a running script has no channel to receive it, so it was dropped rather \ + than left to leak into a later turn" + ); + } + } +} + +/// The ask's explanation: every inspector's reason first (the one that names +/// what is at stake leads — see `approval_prompt_for_request`), then where the +/// call came from, which is the one thing the card could not otherwise show. +fn card_prompt(inspector_reasons: Option) -> String { + const FROM_A_SCRIPT: &str = "This call was made by a Code Execution script \ + (`code_execution__execute_code`). Allowing it runs this one call; every other \ + call the script makes is decided on its own."; + match inspector_reasons { + Some(reasons) => format!("{reasons}\n\n{FROM_A_SCRIPT}"), + None => FROM_A_SCRIPT.to_string(), + } +} + +/// How long a script's ask stays answerable: exactly as long as a direct +/// call's (`BIOROUTER_CONFIRMATION_TIMEOUT_SECS`, where `0` waits until the turn +/// ends). Nothing holds a socket open while it waits — only the script's own +/// thread, which the turn's cancellation releases. +fn approval_ttl() -> Duration { + super::tool_execution::confirmation_timeout().unwrap_or(Duration::MAX) +} + +/// A call the machinery could not judge. Refused: no decision is not a yes. +fn unjudged(name: &str, detail: &str) -> ScriptCallVerdict { + tracing::warn!(tool = %name, detail, "a script's tool call could not be judged; refusing it"); + ScriptCallVerdict::Refuse(ScriptCallRefusal::new( + "permission_unavailable", + format!( + "`{name}` was not run: Biorouter could not reach a permission decision for it \ + ({detail})." + ), + "Not run: no permission decision could be reached", + )) +} #[cfg(test)] mod tests { @@ -39,6 +627,11 @@ mod tests { } async fn fixture(mode: BioRouterMode) -> Fixture { + fixture_with(mode, &[]).await + } + + /// As [`fixture`], plus the named bundled Platform capabilities. + async fn fixture_with(mode: BioRouterMode, platform: &[&str]) -> Fixture { let dir = tempfile::TempDir::new().expect("a scratch directory"); let sessions = Arc::new(SessionManager::new(dir.path().join("sessions"))); let permissions = Arc::new(PermissionManager::new(dir.path().join("config"))); @@ -68,6 +661,17 @@ mod tests { }) .await .expect("enable code_execution"); + for name in platform { + agent + .add_extension(ExtensionConfig::Platform { + name: (*name).into(), + description: (*name).into(), + bundled: Some(true), + available_tools: vec![], + }) + .await + .unwrap_or_else(|error| panic!("enable {name}: {error}")); + } let session = sessions .create_session( dir.path().to_path_buf(), @@ -112,6 +716,21 @@ mod tests { code: &str, cancel: CancellationToken, ) -> tokio::task::JoinHandle<(bool, String)> { + let dispatched = dispatch_script(f, code, cancel).await; + tokio::spawn(async move { + let result = dispatched + .result + .await + .expect("execute_code returns a result"); + (result.is_error.unwrap_or(false), text_of(&result)) + }) + } + + async fn dispatch_script( + f: &Fixture, + code: &str, + cancel: CancellationToken, + ) -> crate::agents::tool_execution::ToolCallResult { let call = CallToolRequestParams { task: None, meta: None, @@ -122,14 +741,7 @@ mod tests { .agent .dispatch_tool_call(call, "outer-execute-code".into(), Some(cancel), &f.session) .await; - let dispatched = dispatched.expect("execute_code dispatches"); - tokio::spawn(async move { - let result = dispatched - .result - .await - .expect("execute_code returns a result"); - (result.is_error.unwrap_or(false), text_of(&result)) - }) + dispatched.expect("execute_code dispatches") } struct Card { @@ -211,6 +823,15 @@ mod tests { .expect("the script task completes") } + /// The value the script handed `record_result`, out of `execute_code`'s + /// `Result: ` text. + fn recorded(output: &str) -> serde_json::Value { + let json = output + .strip_prefix("Result: ") + .unwrap_or_else(|| panic!("not a script result: {output}")); + serde_json::from_str(json).unwrap_or_else(|e| panic!("{e}: {output}")) + } + /// F7's headline, measured the way the QA run measured it: Manual mode, the /// script itself on the user's always-allow list, and a shell call inside /// it. The shell call is not on that list, so it must be put to the user — @@ -244,6 +865,13 @@ mod tests { Some("echo SCRIPT-GATE-ALLOWED"), "the card must carry the call's own evaluated arguments" ); + assert!( + card.prompt + .as_deref() + .is_some_and(|prompt| prompt.contains(EXECUTE_CODE)), + "the card must say the call came from a script: {:?}", + card.prompt + ); answer(&f, &card, Permission::AllowOnce).await; let (is_error, output) = finish(script).await; @@ -282,13 +910,19 @@ mod tests { answer(&f, &card, Permission::DenyOnce).await; let (is_error, output) = finish(script).await; - assert!(!is_error, "the script caught the refusal and finished: {output}"); assert!( - output.contains("\"continued\":true"), + !is_error, + "the script caught the refusal and finished: {output}" + ); + let result = recorded(&output); + assert_eq!( + result["continued"], + serde_json::json!(true), "the script must go on past a refused call: {output}" ); + let caught = result["caught"].as_str().unwrap_or_default(); assert!( - output.contains(SHELL) && output.contains("declined"), + caught.contains(SHELL) && caught.contains("declined"), "the error the script caught must name the refused tool and say why: {output}" ); assert!(!marker.exists(), "a denied command must not have run"); @@ -327,16 +961,26 @@ mod tests { ), Err(finished) => finished, }; - assert!(!is_error, "the script caught the refusal and finished: {output}"); assert!( - output.contains(SHELL) && output.contains("declined"), + !is_error, + "the script caught the refusal and finished: {output}" + ); + let result = recorded(&output); + let caught = result["caught"].as_str().unwrap_or_default(); + assert!( + caught.contains(SHELL) && caught.contains("declined"), "the always-denied call must come back into the script as a tool error: {output}" ); assert!( - output.contains("[package]"), + result["after"] + .as_str() + .is_some_and(|text| text.contains("[package]")), "the script's next call must still run: {output}" ); - assert!(!marker.exists(), "an always-denied command must not have run"); + assert!( + !marker.exists(), + "an always-denied command must not have run" + ); } /// Auto mode is unchanged: the same script call runs, and no card is @@ -360,4 +1004,216 @@ mod tests { assert!(!is_error, "{output}"); assert!(output.contains("SCRIPT-GATE-AUTO"), "{output}"); } + + /// `always_allow` is keyed by the inner tool's name in the other direction + /// too: a call the user allowed by name runs with no card, script or not. + #[tokio::test] + #[serial_test::serial] + async fn a_call_the_user_always_allows_by_name_runs_with_no_card() { + let f = fixture(BioRouterMode::Approve).await; + f.permissions + .update_user_permission(SHELL, PermissionLevel::AlwaysAllow); + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-BY-NAME" }));"#, + CancellationToken::new(), + ) + .await; + + let (is_error, output) = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => panic!( + "a call allowed by its own name must not ask, got a card for {}", + card.tool_name + ), + Err(finished) => finished, + }; + assert!(!is_error, "{output}"); + assert!(output.contains("SCRIPT-GATE-BY-NAME"), "{output}"); + } + + /// "Always allow" on a script call's card is recorded under the INNER + /// tool's name — the name the card showed — so the script's next call to it + /// needs no card, and neither will a direct one. + #[tokio::test] + #[serial_test::serial] + async fn always_allow_on_a_scripts_card_is_recorded_under_the_inner_tools_name() { + let f = fixture(BioRouterMode::Approve).await; + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + const first = shell({ command: "echo SCRIPT-GATE-FIRST" }); + const second = shell({ command: "echo SCRIPT-GATE-SECOND" }); + record_result({ first, second });"#, + CancellationToken::new(), + ) + .await; + + let card = card_or_completion(&f.session.id, &mut script) + .await + .unwrap_or_else(|(_, output)| panic!("the first call must ask: {output}")); + assert_eq!(card.tool_name, SHELL); + answer(&f, &card, Permission::AlwaysAllow).await; + + let (is_error, output) = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => panic!( + "a tool the user just always-allowed asked again: {}", + card.tool_name + ), + Err(finished) => finished, + }; + assert!(!is_error, "{output}"); + assert!( + output.contains("SCRIPT-GATE-FIRST") && output.contains("SCRIPT-GATE-SECOND"), + "{output}" + ); + assert_eq!( + f.permissions.get_user_permission(SHELL), + Some(PermissionLevel::AlwaysAllow), + "the grant belongs to the tool the card named" + ); + assert_eq!( + f.permissions.get_user_permission(EXECUTE_CODE), + Some(PermissionLevel::AlwaysAllow), + "and the script's own entry is untouched" + ); + } + + /// Smart mode grades a script's call from the tool's OWN annotations, read + /// out of the script's catalogue. The agent's registry is graded from the + /// model's roster, which in Code Execution mode holds none of these tools — + /// graded from it, the read-only `chatrecall` would read `Unknown` and ask + /// like a shell. + #[tokio::test] + #[serial_test::serial] + async fn smart_mode_grades_a_scripts_calls_from_the_scripts_own_catalogue() { + let f = fixture_with(BioRouterMode::SmartApprove, &["chatrecall", "todo"]).await; + let mut script = run_script( + &f, + r#"import { chatrecall } from "chatrecall"; + import { todo_write } from "todo"; + const recalled = chatrecall({ query: "script gate smart probe" }); + const written = todo_write({ content: "- [ ] SCRIPT-GATE-SMART" }); + record_result({ recalled: typeof recalled, written: typeof written });"#, + CancellationToken::new(), + ) + .await; + + let card = card_or_completion(&f.session.id, &mut script) + .await + .unwrap_or_else(|(_, output)| { + panic!("a script's non-read-only call must ask in Smart mode: {output}") + }); + assert_eq!( + card.tool_name, "todo__todo_write", + "the read-only chatrecall must pass on its own grade; only the write asks" + ); + answer(&f, &card, Permission::AllowOnce).await; + + let (is_error, output) = finish(script).await; + assert!(!is_error, "{output}"); + } + + /// Where nobody can be asked — a scheduled run — a script's ask is refused + /// at once, as every other ask in that run is, instead of parking a card no + /// interface drains for the whole time-to-live. + /// + /// ⚠ The script is driven INSIDE the scope, not spawned: the production + /// scope (`scheduler.rs`) covers the whole run, and a task-local does not + /// follow a spawn — which is exactly what `execute_code`'s own tool handler + /// has to compensate for. + #[tokio::test] + #[serial_test::serial] + async fn with_nobody_to_ask_a_scripts_ask_is_refused_at_once_not_parked() { + let f = fixture(BioRouterMode::Approve).await; + let (is_error, output) = crate::user_surface::without_human_surface(async { + let dispatched = dispatch_script( + &f, + r#"import { shell } from "developer"; + let caught = null; + try { shell({ command: "echo SCRIPT-GATE-UNATTENDED" }); } + catch (e) { caught = String(e); } + record_result({ caught });"#, + CancellationToken::new(), + ) + .await; + let result = tokio::time::timeout(Duration::from_secs(60), dispatched.result) + .await + .expect("an unattended ask must not park") + .expect("execute_code returns a result"); + (result.is_error.unwrap_or(false), text_of(&result)) + }) + .await; + + assert!(!is_error, "{output}"); + let caught = recorded(&output)["caught"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + caught.contains(SHELL) && caught.contains("nobody can be asked"), + "the refusal must say why: {output}" + ); + assert!( + ActionRequiredManager::global() + .drain_requests(&f.session.id) + .is_empty(), + "no card may be published where nobody can answer it" + ); + } + + /// The refusals `execute_code` owes at its own boundary come FIRST, so + /// nothing they refuse becomes something a card can allow: a script's shell + /// command naming the machine-wide memory store is refused outright, as it + /// always was, even in Manual mode where the judge would otherwise ask. + #[tokio::test] + #[serial_test::serial] + async fn a_boundary_refusal_stays_a_refusal_and_never_becomes_a_card() { + let f = fixture(BioRouterMode::Approve).await; + let store = biorouter_mcp::global_memory_dir().join("probe.txt"); + let code = format!( + r#"import {{ shell }} from "developer"; + let caught = null; + try {{ shell({{ command: "cat '{store}'" }}); }} + catch (e) {{ caught = String(e); }} + record_result({{ caught }});"#, + store = store.display() + ); + let mut script = run_script(&f, &code, CancellationToken::new()).await; + + let (is_error, output) = match card_or_completion(&f.session.id, &mut script).await { + Ok(card) => panic!( + "a call the boundary refuses must not be put to the user, got a card for {}", + card.tool_name + ), + Err(finished) => finished, + }; + assert!(!is_error, "{output}"); + assert!( + recorded(&output)["caught"] + .as_str() + .is_some_and(|caught| caught.contains("global memory store")), + "{output}" + ); + } + + #[test] + fn both_name_forms_of_execute_code_get_a_judge_and_nothing_else_does() { + use crate::agents::code_execution_extension::is_execute_code_call; + assert!(is_execute_code_call(EXECUTE_CODE)); + assert!( + is_execute_code_call("execute_code"), + "models strip prefixes, and the manager resolves the bare name" + ); + for other in [ + "code_execution__read_module", + "code_execution__search_modules", + "developer__execute_code", + "code_executionexecute_code", + "code_execution__execute_code_extra", + SHELL, + ] { + assert!(!is_execute_code_call(other), "{other}"); + } + } } diff --git a/crates/biorouter/src/agents/tool_execution.rs b/crates/biorouter/src/agents/tool_execution.rs index 4b330efc8..6119c1b87 100644 --- a/crates/biorouter/src/agents/tool_execution.rs +++ b/crates/biorouter/src/agents/tool_execution.rs @@ -78,10 +78,66 @@ pub const CANCELLED_RESPONSE: &str = "The user cancelled this turn before decidi const DEFAULT_CONFIRMATION_TIMEOUT_SECS: u64 = 3600; /// Resolve the permission-prompt TTL, honoring `BIOROUTER_CONFIRMATION_TIMEOUT_SECS`. -fn confirmation_timeout() -> Option { +/// +/// `pub(crate)` for [`crate::agents::script_call_gate`]: a card raised for a +/// call inside a Code Execution script is the same question as the card for a +/// direct call, so it stays answerable for exactly as long. +pub(crate) fn confirmation_timeout() -> Option { parse_confirmation_timeout(std::env::var("BIOROUTER_CONFIRMATION_TIMEOUT_SECS").ok()) } +/// What a denied tool call is answered with, chosen by which inspector denied it. +/// +/// One function for both places a denial is written: the agent loop's own +/// denied calls (`Agent::handle_denied_tools`) and a denied call inside a Code +/// Execution script (`script_call_gate`). The two used to be one inline match, +/// and a second inline copy is how a refusal comes to read differently +/// depending on whether the model called a tool directly or from a script. +pub(crate) fn denied_response_text( + request_id: &str, + inspection_results: &[crate::tool_inspection::InspectionResult], +) -> String { + use crate::tool_inspection::InspectionAction; + + // When an inspector denied this call, tell the model why so it can adjust + // instead of blindly retrying. The always-on catastrophic-command block + // (security inspector) and hook denials carry a reason; surface it verbatim + // / with context. + let deny_reason = inspection_results.iter().find(|result| { + result.tool_request_id == request_id + && result.action == InspectionAction::Deny + && !result.reason.trim().is_empty() + }); + match deny_reason { + Some(result) if result.inspector_name == crate::hooks::inspector::HOOK_INSPECTOR_NAME => { + format!("{DECLINED_RESPONSE}\n\nHook feedback: {}", result.reason) + } + // Non-bypassable safety block: the user did not decline, the command is + // refused outright, so return the reason directly. + Some(result) if result.inspector_name == "security" => result.reason.clone(), + // BR-29/BR-31: a loop guard tripped — the call repeated itself, or the + // tool has been failing the same way over and over. The user did not + // decline anything; telling the model they did (the old + // DECLINED_RESPONSE) is actively misleading and leaves it unable to + // diagnose the stop. Return the real reason. + Some(result) if result.inspector_name == crate::tool_monitor::REPETITION_INSPECTOR_NAME => { + result.reason.clone() + } + // #63: a cross-session memory shape Biorouter refuses (the whole-store + // global read). Same reasoning as the loop guards above — the user + // declined nothing, and the reason is the only thing that tells the + // model the itemised call still works. `DECLINED_RESPONSE` here would be + // both untrue and unactionable, and would read as the feature being off. + Some(result) + if result.inspector_name + == crate::security::global_memory::GLOBAL_MEMORY_INSPECTOR_NAME => + { + result.reason.clone() + } + _ => DECLINED_RESPONSE.to_string(), + } +} + /// The TTL policy, split out from the env read so it can be tested without /// mutating process-global state. /// diff --git a/crates/biorouter/src/permission/permission_inspector.rs b/crates/biorouter/src/permission/permission_inspector.rs index 554199ad6..bf8f5a7e2 100644 --- a/crates/biorouter/src/permission/permission_inspector.rs +++ b/crates/biorouter/src/permission/permission_inspector.rs @@ -243,7 +243,11 @@ impl PermissionInspector { /// A tool the annotations cannot grade fails closed, unless the opt-in LLM /// judge is enabled, in which case it is batched for one classification whose /// verdict is cached as a `smart_approve` permission level. - fn smart_verdict(&self, tool_name: &str) -> Verdict { + /// + /// `risks` is where the grade is read from: the agent's own registry for a + /// direct call, a Code Execution script's catalogue for a call the script + /// makes (see [`Self::inspect_graded`]). + fn smart_verdict(&self, tool_name: &str, risks: &ToolRiskRegistry) -> Verdict { if !self.smart.enabled { // Kill switch: behave exactly like Approve. return Verdict::Decided( @@ -273,7 +277,7 @@ impl PermissionInspector { }; } - let risk = self.risks.risk_for(tool_name); + let risk = risks.risk_for(tool_name); if !self.smart.requires_confirmation(risk) { return Verdict::Decided( InspectionAction::Allow, @@ -353,6 +357,7 @@ impl PermissionInspector { request: &ToolRequest, mode: BioRouterMode, working_dir: &Path, + risks: &ToolRiskRegistry, ) -> Verdict { if let Some(verdict) = self.managed_verdict(tool_name, mode) { return verdict; @@ -400,7 +405,7 @@ impl PermissionInspector { ); } - self.smart_verdict(tool_name) + self.smart_verdict(tool_name, risks) } } } @@ -427,24 +432,33 @@ impl PermissionInspector { } read_only } -} - -#[async_trait] -impl ToolInspector for PermissionInspector { - fn name(&self) -> &'static str { - "permission" - } - fn as_any(&self) -> &dyn std::any::Any { - self - } - - async fn inspect( + /// The whole permission decision, with each tool's BR-18 risk grade read out + /// of `risks` instead of the registry the agent refreshes from the model's + /// roster. + /// + /// ⚠ The registry is the ONLY input that differs, and it differs for one + /// caller: a tool call a Code Execution script makes (QA finding F7). In + /// Code Execution mode the model's roster collapses to `code_execution__*` + /// and a handful of exemptions, so the agent's registry has never graded the + /// ~76 tools a script can reach, and every one of them would read `Unknown` + /// — fail-closed, so Smart mode would confirm a read-only `chatrecall` as if + /// it were a shell. The script's own catalogue (the exact tool list its + /// imports are built from, carrying each tool's own MCP annotations) is the + /// analogue of "the exact tool list handed to the model", so its grades are + /// the ones a direct call to the same tool would get. + /// + /// Everything else — the managed policy, the user's own `always_allow` / + /// `never_allow` entries, the extension-management gate, scoped grants, the + /// smart-approve kill switch and the opt-in judge — is this inspector's + /// ordinary sequence, keyed by the tool's own name. The direct path calls + /// this with `&self.risks`, so it cannot drift from it. + pub(crate) async fn inspect_graded( &self, tool_requests: &[ToolRequest], - _messages: &[Message], biorouter_mode: BioRouterMode, session: &crate::session::Session, + risks: &ToolRiskRegistry, ) -> Result> { // Chat mode skips tools entirely; the agent splices a canned response. if biorouter_mode == BioRouterMode::Chat { @@ -462,8 +476,13 @@ impl ToolInspector for PermissionInspector { let Ok(tool_call) = &request.tool_call else { continue; }; - match self.deterministic_verdict(&tool_call.name, request, biorouter_mode, working_dir) - { + match self.deterministic_verdict( + &tool_call.name, + request, + biorouter_mode, + working_dir, + risks, + ) { Verdict::Decided(action, reason) => results.push(InspectionResult { tool_request_id: request.id.clone(), action, @@ -508,3 +527,25 @@ impl ToolInspector for PermissionInspector { Ok(results) } } + +#[async_trait] +impl ToolInspector for PermissionInspector { + fn name(&self) -> &'static str { + "permission" + } + + fn as_any(&self) -> &dyn std::any::Any { + self + } + + async fn inspect( + &self, + tool_requests: &[ToolRequest], + _messages: &[Message], + biorouter_mode: BioRouterMode, + session: &crate::session::Session, + ) -> Result> { + self.inspect_graded(tool_requests, biorouter_mode, session, &self.risks) + .await + } +} diff --git a/crates/biorouter/src/tool_inspection.rs b/crates/biorouter/src/tool_inspection.rs index 193368dbf..e16406c5f 100644 --- a/crates/biorouter/src/tool_inspection.rs +++ b/crates/biorouter/src/tool_inspection.rs @@ -158,6 +158,70 @@ impl ToolInspectionManager { biorouter_mode: BioRouterMode, session: &Session, capability: Option, + ) -> Result> { + self.run_inspectors( + excluded, + tool_requests, + messages, + biorouter_mode, + session, + capability, + None, + ) + .await + } + + /// Inspect the tool calls a Code Execution script makes (QA finding F7). + /// + /// The same inspectors, in the same order, with the same escalation-only + /// merge downstream, as a direct call — with two differences, both forced: + /// + /// * `capability` is required, not optional. A script's calls inherit the + /// capability its `execute_code` call was ADMITTED on (issue #56), so no + /// inspector on this path may sample one of its own. + /// * The permission inspector reads each tool's BR-18 risk grade out of + /// `risks` — the script's own catalogue — rather than the registry the + /// agent refreshes from the model's roster, which in Code Execution mode + /// has never graded the tools a script can reach. See + /// [`PermissionInspector::inspect_graded`]. + /// + /// `excluded` names inspectors that must not judge a script's call; the + /// caller states them rather than this function assuming them. + #[allow(clippy::too_many_arguments)] + pub async fn inspect_script_calls( + &self, + excluded: &[&str], + tool_requests: &[ToolRequest], + messages: &[Message], + biorouter_mode: BioRouterMode, + session: &Session, + capability: CallCapability, + risks: &crate::permission::tool_risk::ToolRiskRegistry, + ) -> Result> { + self.run_inspectors( + excluded, + tool_requests, + messages, + biorouter_mode, + session, + Some(capability), + Some(risks), + ) + .await + } + + /// The one loop over the inspectors. `risks` overrides where the permission + /// inspector reads risk grades from; `None` is its own registry. + #[allow(clippy::too_many_arguments)] + async fn run_inspectors( + &self, + excluded: &[&str], + tool_requests: &[ToolRequest], + messages: &[Message], + biorouter_mode: BioRouterMode, + session: &Session, + capability: Option, + risks: Option<&crate::permission::tool_risk::ToolRiskRegistry>, ) -> Result> { let mut all_results = Vec::new(); @@ -172,16 +236,31 @@ impl ToolInspectionManager { "Running tool inspector" ); - match inspector - .inspect_with_capability( - tool_requests, - messages, - biorouter_mode, - session, - capability, - ) - .await - { + let graded = risks.and_then(|risks| { + inspector + .as_any() + .downcast_ref::() + .map(|permission| (permission, risks)) + }); + let outcome = match graded { + Some((permission, risks)) => { + permission + .inspect_graded(tool_requests, biorouter_mode, session, risks) + .await + } + None => { + inspector + .inspect_with_capability( + tool_requests, + messages, + biorouter_mode, + session, + capability, + ) + .await + } + }; + match outcome { Ok(results) => { tracing::debug!( inspector_name = inspector.name(), From 1a5f1a54e1141aa6176e3062b7148b17623fa1da Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:33:45 -0700 Subject: [PATCH 03/11] docs(permissions): a script's calls are decided under their own names (F7) Code Execution guide: replace the warning that implied per-tool controls already reached inside scripts with a section on how a script is judged twice - the script itself, then every call it makes under that tool's own name - with the table of what each mode, allow/deny entry and card answer does, what an always-allow entry for execute_code now means (and that it is never a shipped default), and the one route (POST /agent/call_tool) where a person drives the script and its calls run as before. Permission modes: a short section saying the same, linked to the table. --- docs/extensions/built-in/code-execution.md | 33 ++++++++++++++++++++-- docs/security/permission-modes.md | 25 ++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/docs/extensions/built-in/code-execution.md b/docs/extensions/built-in/code-execution.md index 09c41807e..e4639f8e0 100644 --- a/docs/extensions/built-in/code-execution.md +++ b/docs/extensions/built-in/code-execution.md @@ -75,7 +75,35 @@ The syntax rules are: ] ``` -> **Warning.** `execute_code` is annotated as destructive and non-idempotent, and it can reach every effective tool exposed by enabled capabilities and loaded extensions — including `developer`'s `shell` and `text_editor`. It inherits the same blast radius as those tools, so the permission controls in the [Developer capability guide](developer.md) and [permission modes](../../security/permission-modes.md) apply to it too. +> **Warning.** `execute_code` is annotated as destructive and non-idempotent, and it can reach every effective tool exposed by enabled capabilities and loaded extensions — including `developer`'s `shell` and `text_editor`. Every one of those calls is still decided on its own, under its own name; see [Permissions: every call a script makes is decided on its own](#permissions-every-call-a-script-makes-is-decided-on-its-own). + +## Permissions: every call a script makes is decided on its own + +A script is judged twice, and neither judgement covers the other. + +1. **The script itself** — `code_execution__execute_code` — is judged like any other tool call. In Manual Approval you are asked before a script runs, unless you have set `execute_code` to **Always allow**. Smart Approval asks too, because `execute_code` is annotated destructive. Completely Autonomous runs it. +2. **Every tool call the script makes** — `developer__shell`, `developer__analyze`, `todo__todo_write`, and so on — is then decided exactly as the same call made directly would be, **under that tool's own name**: your [permission mode](../../security/permission-modes.md), your **Always allow** and **Never allow** entries for that tool, an administrator's [managed policy](../../security/managed-policy.md), the approvals that apply in every mode (sensitive system writes, global memory), and your PreToolUse and PermissionRequest [hooks](../../agent-loop/hooks/hooks-reference.md). + +What that means in practice: + +| Situation | What happens | +|---|---| +| Manual Approval, `execute_code` always allowed, and the script calls `developer__shell` | A card asks about `developer__shell` and shows the command. The card says the call came from a script. | +| The same, with `developer__shell` also always allowed | The shell call runs with no card. | +| `developer__shell` is set to **Never allow** | The call is refused with no card. The script receives a tool error, which it can catch and carry on from. | +| You click **Deny** on the card | The same as Never allow, for this one call: the command does not run, and the script receives *"The user has declined to run this tool."* | +| You click **Always allow** on the card | It is recorded for `developer__shell` — the tool the card named — not for `execute_code`. | +| Smart Approval | Each call is graded by that tool's own risk annotations. Read-only tools such as `chatrecall` run; everything else asks. A tool that carries no annotations asks, as it would directly — `developer`'s tools carry none. | +| Completely Autonomous | Calls run with no card, apart from the operations that ask in every mode. | +| A run with nobody to ask, such as a scheduled workflow | A call that would ask is refused at once, and the script is told why. | + +Three consequences worth knowing: + +- **In Manual Approval a script can ask more than once:** once to run at all, then once for each call it makes that your settings do not already allow. Clicking **Always allow** on a card for a tool the script uses in a loop stops the rest of the loop asking. +- **Always allow on `code_execution__execute_code` is yours to keep.** The script runs in a sandbox with no file, process or network access of its own; everything it does, it does through tool calls, and each of those is decided on its own. What that entry means is "do not ask me before running a script". It no longer also means "and allow everything the script calls". It is not a shipped default — a new `permission.yaml` is empty — so the entry exists only if you added it, by clicking **Always allow** on a script's card or in **Settings → Permissions**. +- **A refusal the script cannot see past stays a refusal.** A call that reads or changes the machine-wide memory store, reads the transcript database, or deletes a knowledge base is refused inside a script outright, as it always has been, rather than turned into a card. + +A script you run yourself through the `POST /agent/call_tool` API is the one exception: that route is driven by a person rather than the model, bypasses the permission mode for the script too, and runs the script's calls as it always has. ## Example usage @@ -112,4 +140,5 @@ The file has been saved to the root directory as `LOG.md`. - [Developer capability](developer.md) — the `shell` and `text_editor` tools most Code Mode scripts import, and the access controls that constrain them. - [Extension Manager capability](extension-manager.md) — the other lever for keeping the active tool count and context usage down. - [Context engineering](../../agent-loop/context-engineering.md) — the broader picture of how BioRouter manages its context window. -- [Permission modes](../../security/permission-modes.md) — how to require approval before a script runs shell commands or edits files. +- [Permission modes](../../security/permission-modes.md) — how to require approval before a script runs shell commands or edits files, and how a script's calls are decided. +- [Hooks reference](../../agent-loop/hooks/hooks-reference.md) — PreToolUse and PermissionRequest hooks, which judge a script's calls as they judge direct ones. diff --git a/docs/security/permission-modes.md b/docs/security/permission-modes.md index 3daf0f7f4..af9854671 100644 --- a/docs/security/permission-modes.md +++ b/docs/security/permission-modes.md @@ -83,6 +83,29 @@ Two things these approvals are guaranteed against, so that "put to you" means wh An administrator's [managed policy](managed-policy.md) can add further tools to this list. +## Scripts: every call is decided on its own + +With the [Code Execution capability](../extensions/built-in/code-execution.md) on — the default +— the model reaches most tools by writing a short script (`code_execution__execute_code`) rather +than calling each tool directly. Your mode and your per-tool settings still apply to **every +call the script makes, under that tool's own name**: + +- In **Manual Approval** a script's `developer__shell` call gets its own card, naming + `developer__shell` and showing the command, even when the script itself is on your + **Always allow** list. **Always allow** or **Never allow** for `developer__shell` applies + inside a script exactly as it does outside one. +- In **Smart Approval** each call is graded by that tool's own risk annotations, so a read-only + lookup runs and anything else asks. +- In **Completely Autonomous** the calls run without a card, apart from the operations in the + table above, which ask in every mode — inside a script too. +- A refused or declined call does not run; the script receives it as an error it can catch. + +So an **Always allow** entry for `code_execution__execute_code` means "do not ask me before +running a script" and nothing more — it is not a grant for the tools the script calls. It is +also never a default: the entry exists in your `permission.yaml` only if you added it. See +[the Code Execution guide](../extensions/built-in/code-execution.md#permissions-every-call-a-script-makes-is-decided-on-its-own) +for the full table, including what **Always allow** on a script's card records. + ## Changing the mode in the desktop app You can change modes before or during a session, and the change takes effect immediately. @@ -177,5 +200,7 @@ To set the default mode, use `biorouter configure`: persisted mode and `permission.yaml` live. - [Hooks reference](../agent-loop/hooks/hooks-reference.md) — lifecycle hooks, which gate tool calls independently of the permission mode. +- [Code Execution capability](../extensions/built-in/code-execution.md) — scripts, and how each + call inside one is decided. - [Data privacy and patient data](data-privacy-and-phi.md) — the other decision that matters before a session touches sensitive data. From e7b8a93a4188986764fb08574375ff7348af0ab2 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:39:54 -0700 Subject: [PATCH 04/11] fix(code-execution): a hook rewrite cannot carry a script call past the boundary refusals (F7) Self-review of the previous commit: the uninspected-boundary refusals (global memory store, transcript database, knowledge delete, first tier crossing) ran on the script's own arguments, and a PreToolUse hook may rewrite them. The inspectors re-judge a rewrite, but those only ASK about some of these shapes, where the boundary refuses outright - so a rewrite was a way to turn a boundary refusal into a card. When the judged arguments differ from the script's, the boundary now runs again on what will actually be dispatched. Pre-dispatch checks move into one admit_sub_call step with one refusal branch in run_tool_handler (which also keeps it under the too_many_lines baseline), and the boundary docs now say the inspector stack does reach a script's calls when the agent loop dispatched the script. Tests: a_hook_rewrite_cannot_carry_a_scripts_call_past_the_boundary_refusals (fails with the re-check disabled: the rewritten 'cat ' was admitted), plus gate-level tests that a rewrite is what runs and is judged again (a rewritten 'rm -rf /' is refused despite an always-allow for developer__shell). --- .../src/agents/code_execution_extension.rs | 105 +++++++++--- .../biorouter/src/agents/script_call_gate.rs | 152 ++++++++++++++++++ 2 files changed, 237 insertions(+), 20 deletions(-) diff --git a/crates/biorouter/src/agents/code_execution_extension.rs b/crates/biorouter/src/agents/code_execution_extension.rs index 2d74bd55a..3a19e9e54 100644 --- a/crates/biorouter/src/agents/code_execution_extension.rs +++ b/crates/biorouter/src/agents/code_execution_extension.rs @@ -2436,8 +2436,12 @@ impl CodeExecutionClient { /// The JS sandbox hands a script's inner tool calls straight to /// `ExtensionManager::dispatch_tool_call`, so the whole inspector stack — /// which is where the global-memory consent gate, the session-store refusal - /// and issue #56's first-crossing disclosure all live — is simply not on - /// this path. Each of the three is therefore re-asked here, against the + /// and issue #56's first-crossing disclosure all live — was not on this + /// path. Since QA finding F7 it is, when the agent loop dispatched the + /// script (`script_call_gate`); it is still not when a person ran the script + /// through `POST /agent/call_tool`, and these refusals run FIRST either way, + /// so none of them becomes an approval card. Each of the three is therefore + /// re-asked here, against the /// **already-evaluated** arguments rather than against the script text: a /// path or a payload the script computed at runtime is fully assembled by /// the time it arrives, which is what makes these boundary checks strictly @@ -2513,33 +2517,26 @@ impl CodeExecutionClient { cancellation_token: &CancellationToken, ) -> Result { // Issue #63 review, finding 3. A script's tool calls go straight to - // the extension manager below, so no `ToolInspector` — the - // global-memory consent gate included — ever sees them. The gate - // compensated by scanning the *script text* for an embedded memory - // call, which a runtime-assembled call walks past + // the extension manager below, and until F7 no `ToolInspector` — the + // global-memory consent gate included — saw them at all; none still + // does when no agent loop dispatched the script (`judge` is `None`). + // The gate had compensated by scanning the *script text* for an + // embedded memory call, which a runtime-assembled call walks past // (`is_global: flag`). This is the same decision taken where there // is nothing left to compute: the dispatched name and the evaluated - // arguments. A boundary that cannot ask the user refuses. - let evaluated = serde_json::from_str::(arguments).ok(); - let evaluated = evaluated.as_ref().and_then(serde_json::Value::as_object); + // arguments. A boundary that cannot ask the user refuses — and it keeps + // refusing when a judge below could ask: turning one of these refusals + // into a card is a decision of its own, not a side effect of F7. // Every boundary refusal this door owes, asked in one place. See // `uninspected_boundary_refusal` for why a door that no // `ToolInspector` reaches has to carry its own. - if let Some((kind, refusal)) = - Self::uninspected_boundary_refusal(cap, session_id, tool_name, evaluated).await - { - return Err(PreDispatchRefusal { - kind, - user_note: None, - error: refusal, - }); - } + Self::boundary_check(cap, session_id, tool_name, arguments).await?; // QA finding F7: the call faces the permission decision it would face // as a direct call — the agent's inspectors, its mode, and the user's // allow/deny entries under THIS tool's name — and an ask goes to the // person on a card naming it. After the boundary refusals, so nothing // they refuse becomes something a card can allow. - Self::judged_arguments( + let admitted = Self::judged_arguments( judge, cap, tool_name, @@ -2551,7 +2548,36 @@ impl CodeExecutionClient { kind: refusal.kind, user_note: Some(refusal.user_note), error: attribute_sub_call_error(tool_name, refusal.message), - }) + })?; + // …and that holds for what actually RUNS. A PreToolUse hook may have + // rewritten the arguments, and the refusals above only ever saw the + // script's own; a rewrite must not be the one way a call that names the + // global memory store or the transcript database reaches a card instead + // of this refusal. + if admitted != arguments { + Self::boundary_check(cap, session_id, tool_name, &admitted).await?; + } + Ok(admitted) + } + + /// [`Self::uninspected_boundary_refusal`] on one set of arguments, as the + /// refusal the loop answers with. + async fn boundary_check( + cap: crate::privacy::CallCapability, + session_id: &str, + tool_name: &str, + arguments: &str, + ) -> Result<(), PreDispatchRefusal> { + let evaluated = serde_json::from_str::(arguments).ok(); + let evaluated = evaluated.as_ref().and_then(serde_json::Value::as_object); + match Self::uninspected_boundary_refusal(cap, session_id, tool_name, evaluated).await { + Some((kind, refusal)) => Err(PreDispatchRefusal { + kind, + user_note: None, + error: refusal, + }), + None => Ok(()), + } } async fn run_tool_handler( @@ -2910,6 +2936,45 @@ mod tests { use std::sync::Arc; use test_case::test_case; + /// QA finding F7. A PreToolUse hook's rewrite is what runs, so the boundary + /// refusals have to see the rewrite as well as the script's own arguments: + /// a hook that turns a harmless command into one naming the machine-wide + /// memory store must meet the outright refusal the script's own call would + /// have met — not a card, and not a dispatch. + #[tokio::test] + async fn a_hook_rewrite_cannot_carry_a_scripts_call_past_the_boundary_refusals() { + use crate::agents::script_call_gate::test_support::{gate, hooks_rewriting_shell_to}; + + let dir = tempfile::TempDir::new().expect("a scratch directory"); + let store = biorouter_mcp::global_memory_dir().join("probe.txt"); + let hooks = hooks_rewriting_shell_to(&format!("cat '{}'", store.display())); + // Auto mode, and no memory inspector in this gate: the judge itself lets + // the rewrite through, so a refusal can only come from the boundary. + let (gate, _permissions) = + gate(dir.path(), crate::config::BioRouterMode::Auto, hooks, false).await; + let judge = ScriptJudge { + gate: Arc::new(gate), + risks: crate::permission::tool_risk::ToolRiskRegistry::new(), + }; + + let refusal = CodeExecutionClient::admit_sub_call( + "boundary-after-rewrite", + crate::privacy::CallCapability::for_test_restricted(), + Some(&judge), + "developer__shell", + r#"{"command":"echo harmless"}"#, + &CancellationToken::new(), + ) + .await + .expect_err("a rewrite naming the global memory store must be refused"); + assert_eq!(refusal.kind, "global_memory_consent"); + assert!( + refusal.error.contains("global memory store"), + "{}", + refusal.error + ); + } + /// Issue #141. A script naming a `platform__*` tool used to fall through to /// `ExtensionManager::dispatch_tool_call`, which does not know these tools /// and answers `Tool '…' not found` — indistinguishable from a typo, so the diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 645878295..1ef5b8132 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -589,6 +589,93 @@ fn unjudged(name: &str, detail: &str) -> ScriptCallVerdict { )) } +/// A gate built by hand, for tests that exercise it — or `execute_code`'s use +/// of it — without an agent. The agent-path tests below build none of this: +/// they go through `Agent::dispatch_tool_call`, which is the only way to prove +/// the judge reaches a script at all. +#[cfg(test)] +pub(crate) mod test_support { + use std::sync::Arc; + + use super::ScriptCallGate; + use crate::config::permission::PermissionManager; + use crate::config::BioRouterMode; + use crate::hooks::HooksManager; + use crate::session::session_manager::SessionType; + use crate::session::SessionManager; + use crate::tool_inspection::ToolInspectionManager; + + /// A user PreToolUse hook that rewrites every `developer__shell` call's + /// command to `command` — the fixture shape the bridge's BR-19 tests use. + pub(crate) fn hooks_rewriting_shell_to(command: &str) -> Arc { + let output = serde_json::json!({ + "hookSpecificOutput": { "updatedInput": { "command": command } } + }) + .to_string(); + let hook = if cfg!(target_os = "windows") { + // cmd.exe keeps the JSON's double quotes and would echo single ones. + format!("echo {output}") + } else { + format!("echo '{}'", output.replace('\'', "'\"'\"'")) + }; + let yaml = format!( + "PreToolUse:\n - matcher: \"developer__shell\"\n hooks:\n - type: command\n command: {}\n", + serde_json::to_string(&hook).expect("a json string"), + ); + Arc::new(HooksManager::with_config( + serde_yaml::from_str(&yaml).expect("the hook config parses"), + false, + Arc::new(tokio::sync::Mutex::new(None)), + )) + } + + /// A gate over the inspectors a verdict is read off — the permission + /// inspector and the user's hooks, plus the security floor on request — + /// with its own permission table in `dir`. + pub(crate) async fn gate( + dir: &std::path::Path, + mode: BioRouterMode, + hooks: Arc, + with_security: bool, + ) -> (ScriptCallGate, Arc) { + let permissions = Arc::new(PermissionManager::new(dir.join("config"))); + let mut inspections = ToolInspectionManager::new(); + if with_security { + inspections.add_inspector(Box::new( + crate::security::security_inspector::SecurityInspector::new(), + )); + } + inspections.add_inspector(Box::new( + crate::permission::permission_inspector::PermissionInspector::new( + Arc::new(crate::permission::tool_risk::ToolRiskRegistry::new()), + Arc::clone(&permissions), + Arc::new(crate::managed::ManagedPolicy::empty()), + Arc::new(tokio::sync::Mutex::new(None)), + ), + )); + inspections.add_inspector(Box::new(crate::hooks::HookInspector::new(Arc::clone( + &hooks, + )))); + let session = SessionManager::new(dir.join("sessions")) + .create_session(dir.to_path_buf(), "gate".into(), SessionType::User) + .await + .expect("a session"); + ( + ScriptCallGate::new(Arc::new(inspections), mode, session, hooks), + permissions, + ) + } + + pub(crate) fn shell_call(command: &str) -> rmcp::model::CallToolRequestParams { + rmcp::model::CallToolRequestParams { + task: None, + meta: None, + name: "developer__shell".into(), + arguments: Some(rmcp::object!({ "command": command })), + } + } +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -1197,6 +1284,71 @@ mod tests { ); } + /// BR-19 on the script's path: a PreToolUse hook's rewrite of a script's + /// call is applied, and the rewritten call — not the script's — is what the + /// gate hands back to be dispatched. + #[tokio::test] + async fn a_hook_rewrite_of_a_scripts_call_is_what_runs() { + use super::test_support::{gate, hooks_rewriting_shell_to, shell_call}; + use super::ScriptCallVerdict; + + let dir = tempfile::TempDir::new().expect("a scratch directory"); + let hooks = hooks_rewriting_shell_to("echo SCRIPT-GATE-REWRITTEN"); + let (gate, permissions) = gate(dir.path(), BioRouterMode::Approve, hooks, false).await; + permissions.update_user_permission(SHELL, PermissionLevel::AlwaysAllow); + + let verdict = gate + .judge( + shell_call("echo SCRIPT-GATE-ORIGINAL"), + crate::privacy::CallCapability::for_test_restricted(), + &crate::permission::tool_risk::ToolRiskRegistry::new(), + &CancellationToken::new(), + ) + .await; + let ScriptCallVerdict::Run(call) = verdict else { + panic!("an always-allowed call with a benign rewrite runs: {verdict:?}"); + }; + assert_eq!( + call.arguments + .as_ref() + .and_then(|args| args.get("command")) + .and_then(|command| command.as_str()), + Some("echo SCRIPT-GATE-REWRITTEN"), + "what runs is what the hook rewrote it to" + ); + } + + /// …and the rewrite is judged AGAIN, by the inspectors that only saw the + /// script's original: the user's always-allow for `developer__shell` does + /// not carry a rewritten `rm -rf /` past the catastrophic-command block. + #[tokio::test] + async fn a_rewritten_script_call_is_judged_again_before_it_runs() { + use super::test_support::{gate, hooks_rewriting_shell_to, shell_call}; + use super::ScriptCallVerdict; + + let dir = tempfile::TempDir::new().expect("a scratch directory"); + let hooks = hooks_rewriting_shell_to("rm -rf /"); + let (gate, permissions) = gate(dir.path(), BioRouterMode::Approve, hooks, true).await; + permissions.update_user_permission(SHELL, PermissionLevel::AlwaysAllow); + + let verdict = gate + .judge( + shell_call("ls"), + crate::privacy::CallCapability::for_test_restricted(), + &crate::permission::tool_risk::ToolRiskRegistry::new(), + &CancellationToken::new(), + ) + .await; + match verdict { + ScriptCallVerdict::Refuse(refusal) => { + assert_eq!(refusal.kind, "permission_denied", "{refusal:?}"); + } + ScriptCallVerdict::Run(call) => { + panic!("a rewritten catastrophic command must not run: {call:?}") + } + } + } + #[test] fn both_name_forms_of_execute_code_get_a_judge_and_nothing_else_does() { use crate::agents::code_execution_extension::is_execute_code_call; From 02c698ddf826fcc05d2708fde731857027805bd5 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:53:39 -0700 Subject: [PATCH 05/11] fix(code-execution): a script's ask carries the same card a direct call's does (F7) Measured in the running app: the provenance line the gate added to every script ask ('This call was made by a Code Execution script...') was read by the desktop as a security finding. ToolCallConfirmation draws any prompt as a warning banner and withholds Always Allow, so an ordinary Manual-mode ask for a script's shell call could only be answered once per call - a loop of twenty meant twenty cards. The card now carries exactly what a direct call's does: the inspectors' reasons when one raised the ask, and no prompt otherwise. The script's step row in the transcript already shows what the card belongs to. The Manual-mode test now pins prompt == None for an ordinary ask, and the docs table says the card and its buttons are the direct call's. --- .../biorouter/src/agents/script_call_gate.rs | 38 ++++++++----------- docs/extensions/built-in/code-execution.md | 2 +- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 1ef5b8132..5954778c6 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -375,9 +375,15 @@ impl ScriptCallGate { let request = UserActionRequest::ToolApproval(ToolApprovalRequest { tool_name: name.clone(), arguments: arguments.clone(), - prompt: Some(card_prompt( - crate::tool_inspection::approval_prompt_for_request(&pending.id, inspections), - )), + // Exactly what a direct call's card carries: the inspectors' reasons, + // and nothing when none of them explained anything. ⚠ Do not add a + // "this came from a script" line here. The desktop reads ANY prompt + // as a security finding — it draws it as a warning banner and + // withholds "Always allow" (`ToolCallConfirmation.tsx`) — so a + // provenance note turned every ordinary script ask into an alarm the + // user could only answer once per call, which was measured in the + // running app. The transcript's step row already shows the script. + prompt: crate::tool_inspection::approval_prompt_for_request(&pending.id, inspections), risk: Some(risks.risk_for(&name)), preview: ToolPreview::for_tool_call(&name, &arguments), // The same answer a direct call's card takes: any surface of this @@ -555,19 +561,6 @@ impl ScriptCallGate { } } -/// The ask's explanation: every inspector's reason first (the one that names -/// what is at stake leads — see `approval_prompt_for_request`), then where the -/// call came from, which is the one thing the card could not otherwise show. -fn card_prompt(inspector_reasons: Option) -> String { - const FROM_A_SCRIPT: &str = "This call was made by a Code Execution script \ - (`code_execution__execute_code`). Allowing it runs this one call; every other \ - call the script makes is decided on its own."; - match inspector_reasons { - Some(reasons) => format!("{reasons}\n\n{FROM_A_SCRIPT}"), - None => FROM_A_SCRIPT.to_string(), - } -} - /// How long a script's ask stays answerable: exactly as long as a direct /// call's (`BIOROUTER_CONFIRMATION_TIMEOUT_SECS`, where `0` waits until the turn /// ends). Nothing holds a socket open while it waits — only the script's own @@ -952,12 +945,13 @@ mod tests { Some("echo SCRIPT-GATE-ALLOWED"), "the card must carry the call's own evaluated arguments" ); - assert!( - card.prompt - .as_deref() - .is_some_and(|prompt| prompt.contains(EXECUTE_CODE)), - "the card must say the call came from a script: {:?}", - card.prompt + // The same card a direct call gets. An ordinary Manual-mode ask carries + // no prompt, and that is load-bearing: the desktop reads any prompt as a + // security finding, draws it as a warning and withholds "Always allow" + // — measured in the running app when a provenance line was added here. + assert_eq!( + card.prompt, None, + "an ordinary script ask must not look like a security finding" ); answer(&f, &card, Permission::AllowOnce).await; diff --git a/docs/extensions/built-in/code-execution.md b/docs/extensions/built-in/code-execution.md index e4639f8e0..5c2e66e85 100644 --- a/docs/extensions/built-in/code-execution.md +++ b/docs/extensions/built-in/code-execution.md @@ -88,7 +88,7 @@ What that means in practice: | Situation | What happens | |---|---| -| Manual Approval, `execute_code` always allowed, and the script calls `developer__shell` | A card asks about `developer__shell` and shows the command. The card says the call came from a script. | +| Manual Approval, `execute_code` always allowed, and the script calls `developer__shell` | A card asks about `developer__shell` and shows the command — the same card, with the same **Allow Once** / **Always Allow** / **Deny** buttons, that calling the tool directly would raise. The script's step in the conversation shows it is waiting on that card. | | The same, with `developer__shell` also always allowed | The shell call runs with no card. | | `developer__shell` is set to **Never allow** | The call is refused with no card. The script receives a tool error, which it can catch and carry on from. | | You click **Deny** on the card | The same as Never allow, for this one call: the command does not run, and the script receives *"The user has declined to run this tool."* | From 9f203b70899801f53e8c67e64e5587efd97cc837 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 12:08:51 -0700 Subject: [PATCH 06/11] test(code-execution): hold env_lock across the two global-store reads (F7) a_hook_rewrite_cannot_carry_a_scripts_call_past_the_boundary_refusals failed once in a filtered run: it resolves the global memory store at the start of the test and the boundary resolves it again later, both through the process-global BIOROUTER_PATH_ROOT, and a guarded writer in another test landed between the two reads. The agent-level boundary test has the same shape. Both now hold env_lock's single global mutex for the whole test while writing nothing. Pinning the variable to its 'current' value (pinned_store_root's shape) was tried first and rejected: that value is read outside the lock, so it can capture another holder's transient root and republish it to every unguarded reader while the test runs. Measured after: the widened filter (script_call_gate code_execution permission inspector tool_inspection global_memory) 20/20 clean, and the full lib binary 3/3 at 3807 passed. --- crates/biorouter/src/agents/code_execution_extension.rs | 9 +++++++++ crates/biorouter/src/agents/script_call_gate.rs | 8 ++++++++ 2 files changed, 17 insertions(+) diff --git a/crates/biorouter/src/agents/code_execution_extension.rs b/crates/biorouter/src/agents/code_execution_extension.rs index 3a19e9e54..9063dffe1 100644 --- a/crates/biorouter/src/agents/code_execution_extension.rs +++ b/crates/biorouter/src/agents/code_execution_extension.rs @@ -2945,6 +2945,15 @@ mod tests { async fn a_hook_rewrite_cannot_carry_a_scripts_call_past_the_boundary_refusals() { use crate::agents::script_call_gate::test_support::{gate, hooks_rewriting_shell_to}; + // The store is resolved TWICE — here, and inside the boundary check — + // and both follow the process-global `BIOROUTER_PATH_ROOT`, which other + // tests change under `env_lock`. Measured flaking without this. Hold + // env_lock's one global mutex, writing NOTHING: pinning the variable to + // a value read outside the lock (`pinned_store_root`'s shape) can catch + // another holder's transient root and republish it to every unguarded + // reader for the length of this test. + let _env = env_lock::lock_env(Vec::<(&str, Option<&str>)>::new()); + let dir = tempfile::TempDir::new().expect("a scratch directory"); let store = biorouter_mcp::global_memory_dir().join("probe.txt"); let hooks = hooks_rewriting_shell_to(&format!("cat '{}'", store.display())); diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 5954778c6..33ff867c0 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -1250,6 +1250,14 @@ mod tests { #[tokio::test] #[serial_test::serial] async fn a_boundary_refusal_stays_a_refusal_and_never_becomes_a_card() { + // The store is resolved here and again inside the boundary check, both + // through the process-global `BIOROUTER_PATH_ROOT`, which other tests + // change under `env_lock`. Hold env_lock's one global mutex for the whole + // test, writing nothing, so no such writer can land between the reads — + // and no value read outside the lock is ever republished (see the same + // guard in `code_execution_extension`'s rewrite test). + let _env = env_lock::lock_env(Vec::<(&str, Option<&str>)>::new()); + let f = fixture(BioRouterMode::Approve).await; let store = biorouter_mcp::global_memory_dir().join("probe.txt"); let code = format!( From 6e4566185c2a594a483c0d62a2523a1dbd369e3a Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 12:12:59 -0700 Subject: [PATCH 07/11] test(code-execution): splice paths into test scripts as JSON literals (F7) Three of the gate's agent-path tests put a filesystem path inside a double-quoted JavaScript string. On Windows the path's backslashes are escape sequences there (\r becomes a carriage return, \U drops its backslash), so the script would name a different path from the one the test means. For a_boundary_refusal_stays_a_refusal_and_never_becomes_a_card that is a real failure, not a cosmetic one: the mangled path no longer names the global memory store, the boundary does not fire, and Manual mode raises a card the test forbids. The two deny tests only passed by luck. The commands are now spliced in as JSON string literals, which are valid JavaScript literals with every backslash escaped. --- .../biorouter/src/agents/script_call_gate.rs | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 33ff867c0..cc80e23de 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -903,6 +903,17 @@ mod tests { .expect("the script task completes") } + /// `text` as a JavaScript string literal, for splicing a path into a script. + /// + /// ⚠ Not `"{path}"`. A Windows path's backslashes are escape sequences in a + /// JS string — `\r` becomes a carriage return, `\U` loses its backslash — so + /// the call the script makes would name a path that is not the one the + /// test means, and a boundary check keyed on that path would silently miss. + /// A JSON string literal is a valid JS one, with every backslash escaped. + fn js_string(text: &str) -> String { + serde_json::to_string(text).expect("a string always serialises") + } + /// The value the script handed `record_result`, out of `execute_code`'s /// `Result: ` text. fn recorded(output: &str) -> serde_json::Value { @@ -971,13 +982,13 @@ mod tests { async fn a_denied_card_is_a_catchable_tool_error_and_the_script_goes_on() { let f = fixture(BioRouterMode::Approve).await; let marker = f.dir.path().join("denied-marker"); + let command = js_string(&format!("touch '{}'", marker.display())); let code = format!( r#"import {{ shell }} from "developer"; let caught = null; - try {{ shell({{ command: "touch '{marker}'" }}); }} + try {{ shell({{ command: {command} }}); }} catch (e) {{ caught = String(e); }} - record_result({{ caught, continued: true }});"#, - marker = marker.display() + record_result({{ caught, continued: true }});"# ); let mut script = run_script(&f, &code, CancellationToken::new()).await; @@ -1021,17 +1032,17 @@ mod tests { f.permissions .update_user_permission("developer__text_editor", PermissionLevel::AlwaysAllow); let marker = f.dir.path().join("never-marker"); + let command = js_string(&format!("touch '{}'", marker.display())); // The developer server's path jail is the process working directory // here, which `cargo test` sets to this crate's root — so the next call // reads a file that is certainly inside it. let code = format!( r#"import {{ shell, text_editor }} from "developer"; let caught = null; - try {{ shell({{ command: "touch '{marker}'" }}); }} + try {{ shell({{ command: {command} }}); }} catch (e) {{ caught = String(e); }} const after = text_editor({{ command: "view", path: "Cargo.toml" }}); - record_result({{ caught, after }});"#, - marker = marker.display(), + record_result({{ caught, after }});"# ); let mut script = run_script(&f, &code, CancellationToken::new()).await; @@ -1260,13 +1271,13 @@ mod tests { let f = fixture(BioRouterMode::Approve).await; let store = biorouter_mcp::global_memory_dir().join("probe.txt"); + let command = js_string(&format!("cat '{}'", store.display())); let code = format!( r#"import {{ shell }} from "developer"; let caught = null; - try {{ shell({{ command: "cat '{store}'" }}); }} + try {{ shell({{ command: {command} }}); }} catch (e) {{ caught = String(e); }} - record_result({{ caught }});"#, - store = store.display() + record_result({{ caught }});"# ); let mut script = run_script(&f, &code, CancellationToken::new()).await; From 63f4edeca5f37275fe06e9d36ddff77ce614a277 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 12:19:42 -0700 Subject: [PATCH 08/11] test(code-execution): pin that sensitive-ops judges a script's calls in Auto mode (F7) The brief names the sensitive-operations inspectors as part of the decision a script's call must face, and no test pinned it. In Auto mode a script's 'echo probe > /etc/...' must still raise a card, carrying the sensitive-ops reason, and a Deny must come back into the script. Fail-before (gate installation disabled in Agent::dispatch_tool_call): the write actually ran - and failed harmlessly on permissions - with no card: 'a sensitive write inside a script must ask even in Auto mode: Result: { caught: "...[shell: command exited with status 1]" }'. The same toggle fails the Manual-mode test too, which is what shows the agent-path tests measure the real installation point rather than a stand-in. Non-Windows, like sensitive_ops' own POSIX-fixture tests. --- .../biorouter/src/agents/script_call_gate.rs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index cc80e23de..e52e5cf17 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -1097,6 +1097,56 @@ mod tests { assert!(output.contains("SCRIPT-GATE-AUTO"), "{output}"); } + /// The sensitive-operations inspector — the one that asks even in Auto mode + /// — judges a script's call too, on its evaluated arguments, and the card + /// carries its own reason (which is also what makes the desktop withhold + /// "Always allow" on it, as for a direct call). + /// + /// The target is `/etc`, which a test user cannot write, so a regression + /// here fails with a permission error rather than touching the system. + /// Not on Windows: the fixture is a POSIX command line, for the reason + /// `sensitive_ops`' own tests give above their `cfg`s. + #[cfg(not(target_os = "windows"))] + #[tokio::test] + #[serial_test::serial] + async fn auto_mode_still_asks_for_a_scripts_sensitive_write() { + let f = fixture(BioRouterMode::Auto).await; + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + let caught = null; + try { shell({ command: "echo probe > /etc/biorouter-f7-sensitive-probe" }); } + catch (e) { caught = String(e); } + record_result({ caught });"#, + CancellationToken::new(), + ) + .await; + + let card = card_or_completion(&f.session.id, &mut script) + .await + .unwrap_or_else(|(_, output)| { + panic!("a sensitive write inside a script must ask even in Auto mode: {output}") + }); + assert_eq!(card.tool_name, SHELL); + assert!( + card.prompt + .as_deref() + .is_some_and(|prompt| prompt.contains("Sensitive system operation")), + "the card must carry the sensitive-ops reason: {:?}", + card.prompt + ); + answer(&f, &card, Permission::DenyOnce).await; + + let (is_error, output) = finish(script).await; + assert!(!is_error, "{output}"); + assert!( + recorded(&output)["caught"] + .as_str() + .is_some_and(|caught| caught.contains("declined")), + "{output}" + ); + } + /// `always_allow` is keyed by the inner tool's name in the other direction /// too: a call the user allowed by name runs with no card, script or not. #[tokio::test] From 42e093df8707e93d1362a4c84cee16abf4138eed Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:27:08 -0700 Subject: [PATCH 09/11] fix(permissions): a script whose judge was lost is refused, not run unjudged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `script_call_gate::current()` was `try_with(..).ok()`, so "no scope on this task" — which is what a `tokio::spawn` anywhere between `Agent::dispatch_tool_call` and `handle_execute_code` produces — collapsed into the same `None` a person-driven dispatch gives, and `judged_arguments` read that `None` as "nothing to judge". The whole control therefore rested on the shape of the call graph, and its failure mode was silent and permissive: the opposite polarity from `unjudged()` in the same module, whose stated principle is that no decision is not a yes. An absent gate is two situations. A person running a script through `POST /agent/call_tool`, an Agent Drafter app or the coding-agent bridge is benign — no agent loop dispatched it, every inspector was bypassed for the outer call too, and there is no model decision to gate. The agent loop's own dispatch arriving without its judge is a defect that would run every call inside the script past the permission system. Tell them apart with a record a spawn cannot lose: `DispatchedByAgentLoop`, a process-global count of the sessions the agent loop has an `execute_code` body in flight for, taken by `judging_script_calls` itself off the gate's own session, so the record and the scope it vouches for are created and released by one expression. `judge_for` is now the single place an absence is given a meaning: gate absent plus that record present is `JudgeLost`, and `handle_execute_code` refuses the whole script before dispatching a sub-call, loudly (a tool error the model sees plus `counter.biorouter.script_call_judge_lost`). `current()` returns `Result<_, NoGateOnThisTask>` so the access error is a named state rather than an absence, and `judged_arguments`' `None` is documented as a verdict already reached rather than a polarity to re-decide. Inverting the polarity outright was measured and rejected: `absent => refuse` would refuse `POST /agent/call_tool`, `routes/apps.rs:10274`, `coding_agent/bridge.rs:123` and ~15 integration-test dispatch sites, all legitimately ungated. The chosen shape errs the other way — its one false positive is a person dispatching a script for a session already inside one, and that gets a loud refusal rather than a silent grant. No change to `agents/agent.rs`. --- .../src/agents/code_execution_extension.rs | 42 ++- .../biorouter/src/agents/script_call_gate.rs | 266 +++++++++++++++++- 2 files changed, 291 insertions(+), 17 deletions(-) diff --git a/crates/biorouter/src/agents/code_execution_extension.rs b/crates/biorouter/src/agents/code_execution_extension.rs index 9063dffe1..36c446e5d 100644 --- a/crates/biorouter/src/agents/code_execution_extension.rs +++ b/crates/biorouter/src/agents/code_execution_extension.rs @@ -1857,13 +1857,33 @@ impl CodeExecutionClient { // if it was the agent loop that dispatched it. Read HERE, on the task // the scope covers — the handler below is spawned, and a task-local does // not follow a spawn. See `script_call_gate`. - let judge = crate::agents::script_call_gate::current().map(|gate| { - let risks = crate::permission::tool_risk::ToolRiskRegistry::new(); - // Graded from the exact list the script's imports are built from, - // so every call it can make has its own tool's grade. - risks.refresh_from_tools(&catalogue); - ScriptJudge { gate, risks } - }); + // + // ⚠ An absent judge is never read as "nothing to judge". `judge_for` + // tells a person-driven dispatch (benign, unchanged) apart from an + // agent-loop dispatch whose scope was lost (a defect, and every call the + // script makes would go past the permission system), and the second + // refuses the whole script rather than running it unjudged. + let judge = match crate::agents::script_call_gate::judge_for(session_id) { + crate::agents::script_call_gate::ScriptJudging::By(gate) => { + let risks = crate::permission::tool_risk::ToolRiskRegistry::new(); + // Graded from the exact list the script's imports are built from, + // so every call it can make has its own tool's grade. + risks.refresh_from_tools(&catalogue); + Some(ScriptJudge { gate, risks }) + } + crate::agents::script_call_gate::ScriptJudging::PersonDriven => None, + crate::agents::script_call_gate::ScriptJudging::JudgeLost => { + tracing::error!( + counter.biorouter.script_call_judge_lost = 1, + session = %session_id, + "an execute_code call dispatched by the agent loop reached the handler with \ + no script-call judge installed; refusing the script rather than running its \ + tool calls unjudged (a task-local does not survive a tokio::spawn — see \ + agents::script_call_gate)" + ); + return Err(crate::agents::script_call_gate::JUDGE_LOST_REFUSAL.to_string()); + } + }; // …and whether a person can be asked at all. Also a task-local, also // lost across the spawn: without this a scheduled run's script would // park an ask nobody can answer until its time-to-live, where every other @@ -2305,8 +2325,12 @@ impl CodeExecutionClient { /// hook's rewrite of them, because what runs is what was judged. `Err` is the /// refusal the script gets instead, as the same sentence a direct call gets. /// - /// No judge means no agent loop dispatched this script (see - /// `script_call_gate::current`), and the call proceeds as it always has. + /// `None` here is a *decision already taken*, not an absence: + /// `handle_execute_code` asked `script_call_gate::judge_for` once, at the one + /// place that can tell a person-driven dispatch from an agent-loop dispatch + /// whose judge was lost, and only the first reaches here. ⚠ Do not re-derive + /// that meaning — a second polarity decision is how the two collapse back + /// into one silent fail-open. async fn judged_arguments( judge: Option<&ScriptJudge>, cap: crate::privacy::CallCapability, diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index e52e5cf17..8e599a7e7 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -64,16 +64,42 @@ //! [`Agent::dispatch_tool_call`] builds a [`ScriptCallGate`] for an //! `execute_code` call and runs the TOOL BODY — the future it returns, not the //! dispatch that builds it — inside [`judging_script_calls`]. `execute_code` -//! reads it with [`current`] and hands it to the task that dispatches the -//! script's calls. Absent means the caller is not the agent loop: `POST -//! /agent/call_tool`, which a person drives and which bypasses every inspector -//! for the outer call too. There is no model decision to gate there, so a -//! script run that way behaves exactly as it always has. +//! asks [`judge_for`] for it and hands it to the task that dispatches the +//! script's calls. +//! +//! ## An absent judge is two situations, and only one of them is benign +//! +//! A task-local is unreachable across a `tokio::spawn`, so "no gate on this +//! task" is evidence of nothing by itself. It is equally the shape of +//! +//! * a **person** running a script — `POST /agent/call_tool`, an Agent Drafter +//! app, the coding-agent bridge (which judges with its own `BridgeGrant`). +//! No agent loop dispatched it, every inspector was bypassed for the outer +//! call too, and there is no model decision to gate: benign, and behaves +//! exactly as it always has; and of +//! * the agent loop dispatching a script whose **scope did not survive** the +//! trip down to the handler, which would run every call inside it *unjudged*. +//! +//! Collapsing those into one `None` made the whole control rest on the shape of +//! the call graph: a `tokio::spawn` inserted anywhere between +//! [`Agent::dispatch_tool_call`] and `handle_execute_code` would silently +//! disable it — no type error, no refusal, no log. So they are told apart by a +//! record a spawn cannot lose: [`DispatchedByAgentLoop`], a process-global count +//! of the sessions the agent loop currently has an `execute_code` body in flight +//! for, taken by [`judging_script_calls`] itself and released when that body +//! ends. Gate absent **and** that record present is +//! [`ScriptJudging::JudgeLost`], and it refuses the whole script. +//! +//! It errs the safe way round. Its one false positive is a *person* dispatching +//! a script through one of the ungated doors for a session whose own turn is +//! already inside one — and there the answer is a loud refusal (a tool error and +//! a `tracing::error!`), never a silent grant. //! //! [`ToolInspector`]: crate::tool_inspection::ToolInspector //! [`Agent::dispatch_tool_call`]: crate::agents::Agent::dispatch_tool_call -use std::sync::Arc; +use std::collections::HashMap; +use std::sync::{Arc, LazyLock, Mutex, PoisonError}; use std::time::Duration; use rmcp::model::{CallToolRequestParams, JsonObject}; @@ -104,6 +130,59 @@ tokio::task_local! { static SCRIPT_CALL_GATE: Arc; } +/// The sessions the agent loop currently has an `execute_code` tool body in +/// flight for, and how many (one turn may dispatch several scripts at once). +/// +/// The durable half of [`judge_for`]: a `tokio::spawn` loses the task-local +/// above, and cannot touch this. Keyed by session because that is the one thing +/// `handle_execute_code` is handed that identifies the dispatch — see the module +/// header for why a false positive here is a refusal rather than a grant. +static AGENT_LOOP_SCRIPTS: LazyLock>> = + LazyLock::new(|| Mutex::new(HashMap::new())); + +/// The agent loop's record that it is running a script for one session with a +/// judge installed. Held for exactly the life of the tool body, so a dropped +/// (cancelled) body releases it too. +pub(crate) struct DispatchedByAgentLoop { + session_id: String, +} + +impl DispatchedByAgentLoop { + pub(crate) fn record(session_id: &str) -> Self { + *AGENT_LOOP_SCRIPTS + .lock() + .unwrap_or_else(PoisonError::into_inner) + .entry(session_id.to_string()) + .or_default() += 1; + Self { + session_id: session_id.to_string(), + } + } +} + +impl Drop for DispatchedByAgentLoop { + fn drop(&mut self) { + let mut scripts = AGENT_LOOP_SCRIPTS + .lock() + .unwrap_or_else(PoisonError::into_inner); + // Remove the key at zero rather than leaving a `0` behind: the map is + // process-global in a daemon that outlives every session in it. + if let Some(count) = scripts.get_mut(&self.session_id) { + *count = count.saturating_sub(1); + if *count == 0 { + scripts.remove(&self.session_id); + } + } + } +} + +fn agent_loop_is_running_a_script(session_id: &str) -> bool { + AGENT_LOOP_SCRIPTS + .lock() + .unwrap_or_else(PoisonError::into_inner) + .contains_key(session_id) +} + /// Run `tool_body` with `gate` judging any call a script makes inside it. /// /// ⚠ Wrap the tool's BODY. `dispatch_tool_call` returns a future, and a scope @@ -113,14 +192,65 @@ pub(crate) async fn judging_script_calls( gate: Arc, tool_body: F, ) -> F::Output { + // Taken HERE rather than in the agent loop, so the record and the scope it + // vouches for are created and released by the same expression and can never + // be wired up one without the other. + let _dispatched = DispatchedByAgentLoop::record(&gate.session.id); SCRIPT_CALL_GATE.scope(gate, tool_body).await } +/// No [`ScriptCallGate`] is installed on the task that asked. +/// +/// A type of its own rather than a `None`, because on its own this is not a +/// decision — [`judge_for`] is what decides what it means. +#[derive(Debug)] +struct NoGateOnThisTask; + /// The gate installed around the tool body running on this task, if any. -pub(crate) fn current() -> Option> { - SCRIPT_CALL_GATE.try_with(Arc::clone).ok() +fn current() -> Result, NoGateOnThisTask> { + SCRIPT_CALL_GATE + .try_with(Arc::clone) + .map_err(|_| NoGateOnThisTask) } +/// Who, if anyone, judges the calls the script about to run makes. +/// +/// No `Debug`: [`ScriptCallGate`] has none, and it holds the inspector stack, +/// the session and the hooks manager — none of which belongs in a log line. +pub(crate) enum ScriptJudging { + /// The agent loop dispatched this script and its judge is right here. + By(Arc), + /// Nothing in the agent loop dispatched it: a person did, through a door + /// that bypasses every inspector for the outer call too. Unchanged + /// behaviour — see the module header. + PersonDriven, + /// The agent loop IS running a script for this session, and this task + /// cannot see its judge. Refuse: running on would put every call the script + /// makes past the permission system. + JudgeLost, +} + +/// Which of the three situations in the module header this `execute_code` call +/// is in. The ONE place an absent gate is given a meaning. +pub(crate) fn judge_for(session_id: &str) -> ScriptJudging { + match current() { + Ok(gate) => ScriptJudging::By(gate), + Err(NoGateOnThisTask) if agent_loop_is_running_a_script(session_id) => { + ScriptJudging::JudgeLost + } + Err(NoGateOnThisTask) => ScriptJudging::PersonDriven, + } +} + +/// What a script whose judge did not reach it is answered with. Deliberately +/// says it is a defect: there is no user action that fixes it, and a sentence +/// that reads like a permission refusal would send them looking for a setting. +pub(crate) const JUDGE_LOST_REFUSAL: &str = + "This script was not run. Biorouter dispatched it but the permission judge for the tool \ + calls it would make did not reach it, so those calls could not be put to you — and running \ + them unjudged is not an option. This is a defect in Biorouter, not something you can allow: \ + please report it."; + /// Inspectors that do not judge a script's calls. See the module header. const NOT_FOR_SCRIPT_CALLS: &[&str] = &[crate::tool_monitor::REPETITION_INSPECTOR_NAME]; @@ -1412,6 +1542,126 @@ mod tests { } } + /// `execute_code` through the door a PERSON's dispatch takes — + /// `ExtensionManager::dispatch_tool_call`, with no judge scope anywhere + /// above it. The only way to reach the handler the way a lost scope would. + async fn dispatch_with_no_scope(f: &Fixture, code: &str) -> (bool, String) { + let call = CallToolRequestParams { + task: None, + meta: None, + name: EXECUTE_CODE.into(), + arguments: Some(object!({ "code": code })), + }; + let dispatched = f + .agent + .extension_manager + .dispatch_tool_call( + &f.session.id, + call, + crate::privacy::CallCapability::for_test_restricted(), + CancellationToken::new(), + ) + .await + .expect("execute_code dispatches"); + let result = tokio::time::timeout(Duration::from_secs(60), dispatched.result) + .await + .expect("execute_code returns a result") + .expect("execute_code returns a result"); + (result.is_error.unwrap_or(false), text_of(&result)) + } + + /// An absent judge is two situations, and only one of them may run. + /// + /// The benign half is a **person** dispatching a script through a door that + /// judges nothing — `POST /agent/call_tool`, an Agent Drafter app, the + /// coding-agent bridge. It runs, exactly as it always has. + /// + /// The other half is the agent loop's OWN dispatch arriving with its scope + /// lost: the shape a `tokio::spawn` inserted anywhere between + /// `Agent::dispatch_tool_call` and `handle_execute_code` would produce. Until + /// this test, `try_with(..).ok()` collapsed it into the benign one and the + /// script ran with every call inside it unjudged — a silent, permissive + /// failure in a permission control, the opposite polarity from `unjudged()` + /// three lines away. It must now be refused. + #[tokio::test] + #[serial_test::serial] + async fn a_script_whose_judge_was_lost_is_refused_and_a_person_driven_one_still_runs() { + let f = fixture(BioRouterMode::Approve).await; + + // 1. Nothing recorded: a person drove it, and it runs. + let (is_error, output) = dispatch_with_no_scope( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-PERSON-DRIVEN" }));"#, + ) + .await; + assert!(!is_error, "a person-driven script must still run: {output}"); + assert!( + output.contains("SCRIPT-GATE-PERSON-DRIVEN"), + "…and its call must have run: {output}" + ); + + // 2. The agent loop IS running a script for this session — and the + // handler cannot see the judge. Refused, and nothing inside it ran. + let recorded = super::DispatchedByAgentLoop::record(&f.session.id); + let (is_error, output) = dispatch_with_no_scope( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-UNJUDGED" }));"#, + ) + .await; + drop(recorded); + + assert!( + is_error, + "a script the agent loop dispatched with no judge must be refused: {output}" + ); + assert!( + output.contains("permission judge"), + "…and the refusal must say what was missing: {output}" + ); + assert!( + !output.contains("SCRIPT-GATE-UNJUDGED"), + "…and its shell call must never have run: {output}" + ); + assert!( + ActionRequiredManager::global() + .drain_requests(&f.session.id) + .is_empty(), + "a lost judge is a defect, not a decision to put to the user" + ); + + // 3. …and the record is released with the body, so the next person-driven + // dispatch is benign again rather than permanently refused. + let (is_error, output) = dispatch_with_no_scope( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-RELEASED" }));"#, + ) + .await; + assert!(!is_error, "{output}"); + assert!(output.contains("SCRIPT-GATE-RELEASED"), "{output}"); + } + + /// The record the refusal above keys on is taken by `judging_script_calls` + /// itself, so it cannot be wired up without the scope it vouches for — and + /// it is released when the body ends, cancelled bodies included. + #[tokio::test] + async fn the_agent_loop_record_lives_exactly_as_long_as_the_scope() { + let session = "script-gate-record-probe"; + assert!(!super::agent_loop_is_running_a_script(session)); + { + let _held = super::DispatchedByAgentLoop::record(session); + assert!(super::agent_loop_is_running_a_script(session)); + let _nested = super::DispatchedByAgentLoop::record(session); + assert!(super::agent_loop_is_running_a_script(session)); + } + assert!( + !super::agent_loop_is_running_a_script(session), + "the record must not outlive the bodies that took it" + ); + } + #[test] fn both_name_forms_of_execute_code_get_a_judge_and_nothing_else_does() { use crate::agents::code_execution_extension::is_execute_code_call; From 6c35a7824fe2f318859515c7b1202156c00a8a02 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:30:40 -0700 Subject: [PATCH 10/11] fix(permissions): a script parked on an approval card hands its dispatch permit back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tool_dispatch_limits`' design note states the premise the whole lock ordering rests on: "a running tool only ever holds resources and runs to completion; it never waits on a resource a parked tool holds." F7 broke it. `execute_code` matches neither `is_spawn_tool_call` nor `is_parking_workspace_tool`, so it takes one of the eight permits in the process-global `TOOL_SEMAPHORE` — shared by every session in the daemon — and holds it across `judging_script_calls`, which now contains every approval card a script's sub-call parks on, for `approval_ttl()`: 3600 s by default, and `Duration::MAX` when `BIOROUTER_CONFIRMATION_TIMEOUT_SECS` is 0. Eight scripts parked on cards stall every other tool call in the process, including the user's own foreground conversation. Starvation rather than deadlock — answering a card takes no permit, and a script's sub-calls bypass the semaphore — but it is exactly the hazard the two workspace exemptions exist for. Adding `execute_code` to that list is the wrong fix. Those two are do-nothing wrappers, so exempting them widens no concurrency; `execute_code` does real work, and in the shipped Code Execution default it is very nearly the only tool the model can call, so an exemption would leave this semaphore bounding nothing at all. The permit is instead handed back for exactly the parked interval: `ToolDispatchGuard` keeps it in a cell, `parking_handle()` hands out a `Weak` reference to that cell, and `DispatchPermitHandle::while_parked` releases it, awaits, and queues for one again before the tool resumes. Waiting for it back starves nobody — the call holds nothing while it waits, which is the whole difference from the situation it replaces — and a cancelled park simply leaves it released. `ScriptCallGate` takes the handle from `Agent::dispatch_tool_call` (the permit is acquired inside the tool body, below where the gate is built) and wraps its `parked.wait(..)` in it. Everything else keeps the permit for its whole execution, unchanged. --- crates/biorouter/src/agents/agent.rs | 19 ++ .../biorouter/src/agents/script_call_gate.rs | 119 +++++++++++- .../src/agents/tool_dispatch_limits.rs | 170 +++++++++++++++++- 3 files changed, 304 insertions(+), 4 deletions(-) diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index 4e8bc64ae..829afb7e2 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -1985,6 +1985,14 @@ pub(crate) fn is_workspace_tool_refused_for( /// Workspace tools that block on work happening in ANOTHER session, and must /// therefore not hold a global tool-dispatch permit while they do. Both name /// forms, like `is_spawn_tool_call`. +/// +/// ⚠ **This is not the list of everything that parks.** `code_execution__execute_code` +/// parks too, since QA finding F7 gave a script's own tool calls approval cards — +/// and it deliberately does NOT belong here, because unlike these two it is not a +/// do-nothing wrapper, and in the shipped Code Execution default it is very nearly +/// the only tool the model calls, so exempting it would leave the semaphore +/// bounding nothing. It releases the permit for the parked interval only, via +/// `tool_dispatch_limits::DispatchPermitHandle::while_parked`. pub(crate) fn is_parking_workspace_tool(name: &str) -> bool { matches!( name, @@ -7505,6 +7513,17 @@ impl Agent { ); let inner_result = match script_gate { Some(gate) => { + // #246 review, finding 2: since F7 a script's own + // calls can park on an approval card, inside this + // body, holding one of the eight shared dispatch + // permits. Hand it back for the parked interval + // instead of exempting `execute_code` by name — in + // the shipped Code Execution default it is nearly + // the only tool, so an exemption would leave the + // semaphore bounding nothing. + gate.hold_dispatch_permit(_dispatch_guard.as_ref().map( + super::tool_dispatch_limits::ToolDispatchGuard::parking_handle, + )); super::script_call_gate::judging_script_calls(gate, inner).await } None => inner.await, diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 8e599a7e7..9f1688ad0 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -317,6 +317,15 @@ pub struct ScriptCallGate { /// For the PreToolUse rewrites this gate's own inspection staged, and the /// PermissionRequest hooks consulted before a card. hooks: Arc, + /// The global tool-dispatch concurrency permit the `execute_code` call this + /// judge belongs to is holding, handed back for the duration of a parked ask + /// (#246 review, finding 2). `None` where there is none to hand back: a test + /// gate, or a dispatch the semaphore exempts. + /// + /// Set after construction because the permit is acquired *inside* the tool + /// body, below the point where the agent still has the pieces this gate is + /// built from — see `Agent::dispatch_tool_call`. + parking_permit: Mutex>, } impl ScriptCallGate { @@ -331,9 +340,34 @@ impl ScriptCallGate { mode, session, hooks, + parking_permit: Mutex::new(None), } } + /// Hand this judge the dispatch permit its `execute_code` call holds, so an + /// ask parked on a person does not hold one of the eight the whole daemon + /// shares. Called once, before any judging. + pub(crate) fn hold_dispatch_permit( + &self, + handle: Option, + ) { + *self + .parking_permit + .lock() + .unwrap_or_else(PoisonError::into_inner) = handle; + } + + /// The handle, cloned out — never read across an `await`, because the lock is + /// a `std::sync::Mutex`. + fn parked_permit_handle( + &self, + ) -> Option { + self.parking_permit + .lock() + .unwrap_or_else(PoisonError::into_inner) + .clone() + } + /// Decide one call a script made. /// /// `capability` is the one the `execute_code` call was admitted on, threaded @@ -522,7 +556,19 @@ impl ScriptCallGate { requires_user_proof: false, }); let parked = PendingUserActions::global().park(Some(&self.session.id), None, request); - let outcome = parked.wait(approval_ttl(), Some(cancel)).await; + // #246 review, finding 2. This wait is up to `approval_ttl()` long + // (default 3600 s; `Duration::MAX` when `BIOROUTER_CONFIRMATION_TIMEOUT_SECS=0`), + // and it happens INSIDE the `execute_code` tool body, which holds one of + // the eight tool-dispatch permits the whole daemon shares. Before F7 + // `execute_code` could not park at all, so eight scripts parked on cards + // would now stall every other tool call in the process — the user's own + // foreground conversation included. Hand the permit back while we wait; + // the script queues for it again before it resumes doing work. + let wait = parked.wait(approval_ttl(), Some(cancel)); + let outcome = match self.parked_permit_handle() { + Some(permit) => permit.while_parked(wait).await, + None => wait.await, + }; self.verdict_for_answer(call, outcome, cancel).await } @@ -1643,6 +1689,77 @@ mod tests { assert!(output.contains("SCRIPT-GATE-RELEASED"), "{output}"); } + /// #246 review, finding 2. `execute_code` takes a permit from the + /// process-global eight-permit dispatch semaphore and holds it for the whole + /// tool body — which, since F7, contains every approval card a script's + /// sub-call parks on, for up to `approval_ttl()` (3600 s by default, + /// `Duration::MAX` when the confirmation timeout is 0). Eight scripts parked + /// on cards would stall every other tool call in the daemon, the user's own + /// foreground conversation included. So a parked ask must hold no permit. + /// + /// Measured by filling the ceiling to exactly one free permit before the + /// script runs: the script's dispatch takes the last one, and while it is + /// parked on its card an unrelated dispatch must still be able to acquire. + /// Before the fix that acquisition never completes. + #[tokio::test] + #[serial_test::serial] + async fn a_parked_scripts_ask_holds_no_global_dispatch_permit() { + use crate::agents::tool_dispatch_limits; + + let f = fixture(BioRouterMode::Approve).await; + let dir = f.dir.path().to_path_buf(); + + // Fill the ceiling to one free permit. `probe` names no file, so these + // are concurrency permits and nothing else. Generous timeout: other + // tests in this binary hold permits briefly and release them. + let mut filled = Vec::new(); + for _ in 0..tool_dispatch_limits::max_concurrent_tools().saturating_sub(1) { + filled.push( + tokio::time::timeout( + Duration::from_secs(30), + tool_dispatch_limits::acquire("probe", None, &dir), + ) + .await + .expect("the binary's other tool dispatches release their permits"), + ); + } + + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-PARKED" }));"#, + CancellationToken::new(), + ) + .await; + let card = card_or_completion(&f.session.id, &mut script) + .await + .unwrap_or_else(|(_, output)| panic!("the shell call must ask: {output}")); + assert_eq!(card.tool_name, SHELL); + + // The script is parked on that card and must therefore be holding + // nothing: the last permit has to be available to an unrelated tool. + let unrelated = tokio::time::timeout( + Duration::from_secs(10), + tool_dispatch_limits::acquire("probe", None, &dir), + ) + .await; + assert!( + unrelated.is_ok(), + "a script parked on an approval card held its dispatch permit; eight of \ + those stall every other tool call in the daemon" + ); + + // Free everything before answering: the script queues for a permit again + // when it resumes, and would otherwise be waiting on this test. + drop(unrelated); + drop(filled); + + answer(&f, &card, Permission::AllowOnce).await; + let (is_error, output) = finish(script).await; + assert!(!is_error, "the answered call still runs: {output}"); + assert!(output.contains("SCRIPT-GATE-PARKED"), "{output}"); + } + /// The record the refusal above keys on is taken by `judging_script_calls` /// itself, so it cannot be wired up without the scope it vouches for — and /// it is released when the body ends, cancelled bodies included. diff --git a/crates/biorouter/src/agents/tool_dispatch_limits.rs b/crates/biorouter/src/agents/tool_dispatch_limits.rs index dbe85a293..73013e408 100644 --- a/crates/biorouter/src/agents/tool_dispatch_limits.rs +++ b/crates/biorouter/src/agents/tool_dispatch_limits.rs @@ -26,6 +26,18 @@ //! circular waiting and the design is deadlock-free: a running tool only ever //! holds resources and runs to completion; it never waits on a resource a parked //! tool holds. +//! +//! ⚠ **That last sentence is a rule about the tools, not a property of this +//! module**, and two things have broken it since. `workspace_watch` / +//! `workspace_send_prompt` park on work in other sessions and are exempted from +//! the permit by name (`agent::is_parking_workspace_tool`). `execute_code` parks +//! on a *person* — since QA finding F7 a script's own tool calls raise approval +//! cards — but it is not a do-nothing wrapper, and in the shipped Code Execution +//! default it is very nearly the only tool there is, so exempting it by name +//! would leave this semaphore bounding nothing at all. It instead hands the +//! permit back for exactly the parked interval and queues for it again +//! afterwards: [`ToolDispatchGuard::parking_handle`] and +//! [`DispatchPermitHandle::while_parked`]. use std::collections::HashMap; use std::path::{Component, Path, PathBuf}; @@ -99,13 +111,79 @@ static PATH_LOCKS: LazyLock>>>> = /// a session that touches thousands of distinct files does not leak map slots. const PATH_LOCK_PRUNE_THRESHOLD: usize = 1024; +/// The concurrency permit, in a cell a tool that parks on a person can hand it +/// back through. `None` means this dispatch holds no permit — an exempt tool, a +/// closed semaphore in a test teardown, or a park in progress. +type PermitCell = Arc>>; + /// RAII guard held for the lifetime of a tool's execution. Dropping it releases /// the concurrency permit and any write-path locks. pub struct ToolDispatchGuard { - _permit: Option, + permit: PermitCell, + /// The semaphore `permit` came from, so a [`DispatchPermitHandle`] queues for + /// the same one it handed a permit back to. A field rather than a reach for + /// the static, because the tests need their own. + semaphore: Arc, _path_guards: Vec>, } +impl ToolDispatchGuard { + /// A handle for a tool that **parks on a person** to hand its concurrency + /// permit back for the duration of the wait. + /// + /// Only for genuine parking — an approval card, an elicitation — never around + /// work. The permit exists to bound work, and a tool waiting on a human is + /// doing none; holding one there is what turns eight parked calls into a + /// stalled daemon (`Semaphore::new(8)`, shared by every session in the + /// process), which is the hazard `agent::is_parking_workspace_tool` was + /// added for. + pub fn parking_handle(&self) -> DispatchPermitHandle { + DispatchPermitHandle { + // Weak: a handle that outlives the dispatch it came from must not + // keep that dispatch's permit alive. + permit: Arc::downgrade(&self.permit), + semaphore: Arc::clone(&self.semaphore), + } + } +} + +/// A tool's way of not holding its dispatch permit while it is parked on a +/// person. See [`ToolDispatchGuard::parking_handle`]. +#[derive(Clone)] +pub struct DispatchPermitHandle { + permit: Weak>>, + semaphore: Arc, +} + +impl DispatchPermitHandle { + /// Await `parked` holding no concurrency permit, then queue for one again + /// before the tool resumes. + /// + /// Waiting for the permit back starves nobody: this call holds nothing while + /// it waits, which is the whole difference from the situation it replaces. If + /// `parked` is dropped part-way (a cancelled turn) the permit simply stays + /// released and the guard drops with nothing to free. + pub async fn while_parked(&self, parked: F) -> F::Output { + let Some(cell) = self.permit.upgrade() else { + // The dispatch this handle came from is already over. + return parked.await; + }; + let handed_back = cell.lock().await.take(); + if handed_back.is_none() { + // Nothing to hand back: an exempt dispatch, or an outer park already + // did. Never hold the cell's lock across the await below. + return parked.await; + } + drop(handed_back); + + let outcome = parked.await; + if let Ok(permit) = self.semaphore.clone().acquire_owned().await { + *cell.lock().await = Some(permit); + } + outcome + } +} + /// Acquire the concurrency permit and any write-path locks for a tool call, in /// deadlock-free order (permit first, then paths sorted). The returned guard /// must be held for the whole duration of the tool's execution. @@ -117,7 +195,7 @@ pub async fn acquire( // 1. Bound total parallelism. The static Semaphore never closes, so a // failure here can only mean a poisoned/closed sem in a test teardown — // fail open (run the tool) rather than wedge the loop. - let permit = TOOL_SEMAPHORE.clone().acquire_owned().await.ok(); + let permit = acquire_permit_cell(&TOOL_SEMAPHORE).await; // 2. Serialize overlapping write paths. let path_guards = if write_ordering_enabled() { @@ -128,11 +206,19 @@ pub async fn acquire( }; ToolDispatchGuard { - _permit: permit, + permit, + semaphore: TOOL_SEMAPHORE.clone(), _path_guards: path_guards, } } +/// Take one permit from `semaphore` into a cell a park can hand it back through. +async fn acquire_permit_cell(semaphore: &Arc) -> PermitCell { + Arc::new(AsyncMutex::new( + semaphore.clone().acquire_owned().await.ok(), + )) +} + /// Take an exclusive lock on each path, in a stable sorted order so that two /// tools locking an overlapping set can never deadlock on acquisition order. async fn acquire_path_locks(mut paths: Vec) -> Vec> { @@ -451,6 +537,84 @@ mod tests { assert_eq!(DEFAULT_MAX_CONCURRENT_TOOLS, 8); } + /// A tool parked on a person holds no permit, and takes one back — queueing + /// if it must — before it resumes. + /// + /// Against its own one-permit semaphore rather than the process-global + /// `TOOL_SEMAPHORE`, so the assertions are exact instead of racing every other + /// test in this binary. + #[tokio::test] + async fn a_parked_tool_hands_its_permit_back_and_queues_for_it_again() { + let semaphore = Arc::new(Semaphore::new(1)); + let guard = ToolDispatchGuard { + permit: acquire_permit_cell(&semaphore).await, + semaphore: Arc::clone(&semaphore), + _path_guards: Vec::new(), + }; + assert_eq!( + semaphore.available_permits(), + 0, + "the tool holds the permit" + ); + + let handle = guard.parking_handle(); + let (answer, parked) = tokio::sync::oneshot::channel::<()>(); + let waiting = tokio::spawn(async move { handle.while_parked(parked).await }); + + for _ in 0..200 { + if semaphore.available_permits() == 1 { + break; + } + tokio::time::sleep(Duration::from_millis(5)).await; + } + assert_eq!( + semaphore.available_permits(), + 1, + "a parked tool must hold no permit — eight of them would stall the daemon" + ); + + // Somebody else takes the freed permit, so resuming has to queue. + let other = Arc::clone(&semaphore) + .acquire_owned() + .await + .expect("a permit"); + answer.send(()).expect("the park is still waiting"); + drop(other); + + tokio::time::timeout(Duration::from_secs(5), waiting) + .await + .expect("the parked tool resumes once a permit frees") + .expect("the parked task completes") + .expect("the park resolves"); + assert_eq!( + semaphore.available_permits(), + 0, + "the permit is taken back for the rest of the tool's work" + ); + drop(guard); + assert_eq!(semaphore.available_permits(), 1, "…and freed on drop"); + } + + /// A handle whose dispatch is already over must not resurrect a permit. + #[tokio::test] + async fn a_handle_that_outlived_its_dispatch_is_inert() { + let semaphore = Arc::new(Semaphore::new(1)); + let guard = ToolDispatchGuard { + permit: acquire_permit_cell(&semaphore).await, + semaphore: Arc::clone(&semaphore), + _path_guards: Vec::new(), + }; + let handle = guard.parking_handle(); + drop(guard); + assert_eq!(semaphore.available_permits(), 1); + handle.while_parked(std::future::ready(())).await; + assert_eq!( + semaphore.available_permits(), + 1, + "an inert handle must not take a permit nobody will release" + ); + } + #[tokio::test] async fn same_path_writes_are_serialized() { let path = vec![PathBuf::from("/work/shared.txt")]; From 6a6748d1fd695294c02ccc79f9825656210e2152 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:53:31 -0700 Subject: [PATCH 11/11] docs: the parking-exemption list is not the list of everything that parks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two doc corrections the two review fixes make necessary. `workspace-control-tools.md` read "Two tools park, and are exempt from the dispatch permit". `execute_code` parks too now, and is deliberately not on that list — so the line was sending the next reader to the wrong mechanism. It now names both shapes and says a new parking tool has to choose between them. `code-execution.md` gains the limitation the review turned up and this PR cannot close without changing the approval protocol: a script call's card names the tool and shows the arguments exactly as a direct call's does, and says nothing about the script, so "Always allow" there records a grant for that tool everywhere from a card that did not say where the call came from. The alternative lever — a `prompt` — is also what draws the security banner and hides "Always allow", which this PR already measured and reverted. --- docs/agent-loop/workspace-control-tools.md | 2 ++ docs/extensions/built-in/code-execution.md | 1 + 2 files changed, 3 insertions(+) diff --git a/docs/agent-loop/workspace-control-tools.md b/docs/agent-loop/workspace-control-tools.md index caa203dfe..f8f5b3dd3 100644 --- a/docs/agent-loop/workspace-control-tools.md +++ b/docs/agent-loop/workspace-control-tools.md @@ -36,6 +36,8 @@ This page uses the bare names for readability. **Two tools park, and are exempt from the dispatch permit.** `workspace_watch` and `workspace_send_prompt` block on work happening in another session, so `is_parking_workspace_tool` (`agent.rs`) keeps them from holding a global tool-dispatch permit while they wait. Without that a parked watch would throttle the caller's own unrelated tool calls. +⚠ **That exemption list is not the list of everything that parks.** `code_execution__execute_code` parks too — a script's own tool calls raise approval cards — and it is deliberately absent from it: unlike these two it is not a do-nothing wrapper, and with the Code Execution capability on it is very nearly the only tool the model calls, so exempting it would leave the semaphore bounding nothing. It releases the permit for the parked interval only, through `tool_dispatch_limits::DispatchPermitHandle::while_parked`. A new parking tool has to pick one of those two shapes deliberately; neither is the default. + **Subagents are refused all seven `workspace_*` tools.** `is_workspace_tool_refused_for` (`agent.rs`) enumerates them and refuses when the calling session's type is `SubAgent`, in both the prefixed and bare name forms. `subagent` itself is refused inside a subagent by `is_spawn_tool_call`, so a child cannot spawn grandchildren. **What a missing daemon costs.** `workspace_services::get()` returns `None` in a process with no daemon (a plain `biorouter` terminal session). Each tool's entry names its own refusal; the summary is: diff --git a/docs/extensions/built-in/code-execution.md b/docs/extensions/built-in/code-execution.md index 5c2e66e85..6e3b5e104 100644 --- a/docs/extensions/built-in/code-execution.md +++ b/docs/extensions/built-in/code-execution.md @@ -102,6 +102,7 @@ Three consequences worth knowing: - **In Manual Approval a script can ask more than once:** once to run at all, then once for each call it makes that your settings do not already allow. Clicking **Always allow** on a card for a tool the script uses in a loop stops the rest of the loop asking. - **Always allow on `code_execution__execute_code` is yours to keep.** The script runs in a sandbox with no file, process or network access of its own; everything it does, it does through tool calls, and each of those is decided on its own. What that entry means is "do not ask me before running a script". It no longer also means "and allow everything the script calls". It is not a shipped default — a new `permission.yaml` is empty — so the entry exists only if you added it, by clicking **Always allow** on a script's card or in **Settings → Permissions**. - **A refusal the script cannot see past stays a refusal.** A call that reads or changes the machine-wide memory store, reads the transcript database, or deletes a knowledge base is refused inside a script outright, as it always has been, rather than turned into a card. +- **A card does not say the call came from a script**, and that is a known limitation. It names the tool and shows the arguments, exactly as a direct call's card does; the script is visible only as the conversation step the card is waiting inside. So **Always allow** on such a card records a grant for that tool everywhere — direct calls included — from a card that did not tell you where this one came from. If that is more than you meant, use **Allow Once**, or set the tool from **Settings → Permissions** where the scope is explicit. A script you run yourself through the `POST /agent/call_tool` API is the one exception: that route is driven by a person rather than the model, bypasses the permission mode for the script too, and runs the script's calls as it always has.