diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index 4c950c090..829afb7e2 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -20,7 +20,11 @@ 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}; +// The denial text itself is written by `tool_execution::denied_response_text` +// now; the tests below still assert against the constant. +#[cfg(test)] +use super::tool_execution::DECLINED_RESPONSE; use super::turn_abort::TurnAbortCode; use crate::action_required_manager::ActionRequiredManager; use crate::agents::budget::{BudgetAction, BudgetTracker, ReplyBudget}; @@ -1981,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, @@ -6379,62 +6391,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() - } - // F4: a `workspace_set_tools` change that cannot be made — - // a built-in capability named for removal, a knowledge base - // that does not exist — refused BEFORE any card. Nobody was - // asked, so "the user has declined" would be false, and the - // reason is the handler's own sentence. - Some(result) - if result.inspector_name - == crate::agents::workspace_inspector::WORKSPACE_MUTATION_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(), @@ -7498,6 +7456,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 { @@ -7536,7 +7511,23 @@ impl Agent { id = %exec_request_id, "TOOL_EXEC_START" ); - let inner_result = inner.await; + 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, + }; 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..36c446e5d 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,50 @@ 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`. + // + // ⚠ 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 + // 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 +1899,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 +2290,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 +2313,59 @@ 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. + /// + /// `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, + 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. @@ -2322,8 +2460,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 @@ -2385,9 +2527,87 @@ 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, 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 — 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. + 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. + let admitted = 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), + })?; + // …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( session_id: String, cap: crate::privacy::CallCapability, + judge: Option, mut call_rx: mpsc::UnboundedReceiver, extension_manager: Option>, collected_artifacts: Arc>, @@ -2415,39 +2635,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, @@ -2741,6 +2960,54 @@ 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}; + + // 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())); + // 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 @@ -3421,6 +3688,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 +3710,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 +3836,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/mod.rs b/crates/biorouter/src/agents/mod.rs index 91c159312..b35cbc7cc 100644 --- a/crates/biorouter/src/agents/mod.rs +++ b/crates/biorouter/src/agents/mod.rs @@ -50,6 +50,9 @@ pub(crate) mod reply_parts; pub mod resource_refs; pub mod retry; pub(crate) 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..9f1688ad0 --- /dev/null +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -0,0 +1,1801 @@ +//! 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` +//! 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::collections::HashMap; +use std::sync::{Arc, LazyLock, Mutex, PoisonError}; +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; +} + +/// 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 +/// 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 { + // 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. +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]; + +/// …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, + /// 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 { + pub(crate) fn new( + inspections: Arc, + mode: BioRouterMode, + session: Session, + hooks: Arc, + ) -> Self { + Self { + inspections, + 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 + /// 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(), + // 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 + // 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); + // #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 + } + + /// 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" + ); + } + } +} + +/// 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", + )) +} + +/// 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; + 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 { + 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"))); + 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"); + 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(), + "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 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, + 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; + dispatched.expect("execute_code dispatches") + } + + 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") + } + + /// `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 { + 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 — + /// 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" + ); + // 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; + 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 command = js_string(&format!("touch '{}'", marker.display())); + let code = format!( + r#"import {{ shell }} from "developer"; + let caught = null; + try {{ shell({{ command: {command} }}); }} + catch (e) {{ caught = String(e); }} + record_result({{ caught, continued: true }});"# + ); + 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}" + ); + 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!( + 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"); + } + + /// `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"); + 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: {command} }}); }} + catch (e) {{ caught = String(e); }} + const after = text_editor({{ command: "view", path: "Cargo.toml" }}); + record_result({{ caught, after }});"# + ); + 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}" + ); + 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!( + 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" + ); + } + + /// 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}"); + } + + /// 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] + #[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() { + // 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 command = js_string(&format!("cat '{}'", store.display())); + let code = format!( + r#"import {{ shell }} from "developer"; + let caught = null; + try {{ shell({{ command: {command} }}); }} + catch (e) {{ caught = String(e); }} + record_result({{ caught }});"# + ); + 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}" + ); + } + + /// 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:?}") + } + } + } + + /// `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}"); + } + + /// #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. + #[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; + 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_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")]; diff --git a/crates/biorouter/src/agents/tool_execution.rs b/crates/biorouter/src/agents/tool_execution.rs index 4b330efc8..88df0009c 100644 --- a/crates/biorouter/src/agents/tool_execution.rs +++ b/crates/biorouter/src/agents/tool_execution.rs @@ -78,10 +78,76 @@ 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() + } + // F4: a `workspace_set_tools` change that cannot be made — a built-in + // capability named for removal, a knowledge base that does not exist — + // refused BEFORE any card. Nobody was asked, so "the user has declined" + // would be false, and the reason is the handler's own sentence. + Some(result) + if result.inspector_name + == crate::agents::workspace_inspector::WORKSPACE_MUTATION_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(), 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 09c41807e..6e3b5e104 100644 --- a/docs/extensions/built-in/code-execution.md +++ b/docs/extensions/built-in/code-execution.md @@ -75,7 +75,36 @@ 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 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."* | +| 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 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. ## Example usage @@ -112,4 +141,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.