diff --git a/crates/biorouter-server/src/routes/tool_bridge.rs b/crates/biorouter-server/src/routes/tool_bridge.rs index d834057e6..0d6558855 100644 --- a/crates/biorouter-server/src/routes/tool_bridge.rs +++ b/crates/biorouter-server/src/routes/tool_bridge.rs @@ -123,6 +123,10 @@ async fn call_tool( Some(_) => return rpc_error(id, -32602, "tools/call arguments must be an object"), }; + // The child's own id for this call. Not forwarded to the tool — `meta` stays + // `None` — but it is what lets the transcript pair the full result the grant + // keeps with the frame on which the child reports the call. + let child_call_id = bridge::child_call_id(params.get("_meta")); let call = CallToolRequestParams { name: name.to_string().into(), arguments, @@ -130,7 +134,9 @@ async fn call_tool( task: None, }; - match grant.call(call).await { + // The child is answered with the model's view of the result: the blocks a + // model is sent, unannotated (`bridge::child_view`, QA-E F4). + match grant.call_for_child(call, child_call_id).await { Ok(result) => rpc_ok( id, serde_json::to_value(result).unwrap_or_else(|e| { diff --git a/crates/biorouter-server/tests/tool_bridge_routes.rs b/crates/biorouter-server/tests/tool_bridge_routes.rs index 0d1228029..5de00341b 100644 --- a/crates/biorouter-server/tests/tool_bridge_routes.rs +++ b/crates/biorouter-server/tests/tool_bridge_routes.rs @@ -84,6 +84,142 @@ fn no_hooks() -> Arc { )) } +/// A dispatcher that answers every call with one fixed result. +/// +/// Stands in for an extension when a test needs to know that a call really ran +/// on Biorouter's side (a random marker only this returns) or needs a tool's +/// exact result shape. +struct FixedResultDispatch { + result: rmcp::model::CallToolResult, +} + +#[async_trait::async_trait] +impl bridge::BridgeToolDispatch for FixedResultDispatch { + async fn dispatch( + &self, + _session_id: &str, + _call: rmcp::model::CallToolRequestParams, + _capability: CallCapability, + _cancel: tokio_util::sync::CancellationToken, + ) -> Result { + Ok(self.result.clone()) + } +} + +/// A grant over [`advertised_tool`] whose calls are approved (Auto mode, a real +/// permission inspector) and answered with `result`. +fn fixed_result_grant(result: rmcp::model::CallToolResult) -> bridge::BridgeGrant { + use biorouter::config::permission::PermissionManager; + use biorouter::managed::ManagedPolicy; + use biorouter::permission::permission_inspector::PermissionInspector; + use biorouter::permission::tool_risk::ToolRiskRegistry; + + let risks = Arc::new(ToolRiskRegistry::new()); + let mut inspections = ToolInspectionManager::new(); + inspections.add_inspector(Box::new(PermissionInspector::new( + Arc::clone(&risks), + PermissionManager::instance(), + Arc::new(ManagedPolicy::empty()), + Arc::new(tokio::sync::Mutex::new(None)), + ))); + bridge::BridgeGrant::new( + Session::default(), + BioRouterMode::Auto, + Arc::new(FixedResultDispatch { result }), + Arc::new(inspections), + CallCapability::public_enforced(), + vec![advertised_tool()], + Conversation::new_unvalidated(vec![]), + None, + no_hooks(), + None, + risks, + ) +} + +/// A grant whose one tool answers with `marker=` and nothing else. +fn marker_grant(marker: &str) -> bridge::BridgeGrant { + fixed_result_grant(rmcp::model::CallToolResult::success(vec![ + rmcp::model::Content::text(format!("marker={marker}")), + ])) +} + +/// QA-E F4 at the wire, through the real router: a child's `tools/call` is +/// answered with the model's view of the result — the assistant's block, no +/// annotations — and the full result is kept under the child's own call id, for +/// each CLI's `_meta` spelling (measured: claude 2.1.266, codex-cli 0.153.4). +/// +/// The shape is `developer__shell`'s. Handing the child both blocks made it read +/// every result twice, and the user block's `priority: 0.0` made codex-cli fail +/// the call outright with "Unexpected response type". +#[tokio::test] +#[serial_test::serial] +async fn the_child_is_answered_with_the_models_view_and_the_full_result_is_kept() { + use axum::body::Body; + use axum::http::Request; + use tower::ServiceExt; + + bridge::publish_base_url("http://127.0.0.1:65535"); + let shell = rmcp::model::CallToolResult::success(vec![ + rmcp::model::Content::text("Thu Sep 11").with_audience(vec![rmcp::model::Role::Assistant]), + rmcp::model::Content::text("Thu Sep 11") + .with_audience(vec![rmcp::model::Role::User]) + .with_priority(0.0), + ]); + let lease = bridge::issue(fixed_result_grant(shell.clone())).expect("issued"); + let nonce = lease.url().rsplit('/').next().expect("a nonce").to_string(); + + for (meta, child_call_id) in [ + ( + json!({ "claudecode/toolUseId": "toolu_wire", "progressToken": 2 }), + "toolu_wire", + ), + ( + json!({ "callId": "exec-wire", "threadId": "t", "progressToken": 1 }), + "exec-wire", + ), + ] { + let request = json!({ + "jsonrpc": "2.0", "id": 1, "method": "tools/call", + "params": { + "name": "spokeagent__query_knowledge_graph", + "arguments": { "cypher": "MATCH (n) RETURN n LIMIT 1" }, + "_meta": meta, + } + }); + let response = biorouter_server::routes::tool_bridge::routes() + .oneshot( + Request::builder() + .method("POST") + .uri(format!("/tool_bridge/{nonce}")) + .header("content-type", "application/json") + .body(Body::from(request.to_string())) + .expect("a request"), + ) + .await + .expect("the route answers"); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .expect("a body"); + let body: serde_json::Value = serde_json::from_slice(&bytes).expect("JSON"); + + assert_eq!( + body["result"]["content"], + json!([{ "type": "text", "text": "Thu Sep 11" }]), + "the child gets the model's block, unannotated: {body}" + ); + assert!( + !body.to_string().contains("priority"), + "codex-cli cannot parse a `priority` annotation: {body}" + ); + assert_eq!( + bridge::take_recorded_result(lease.url(), child_call_id), + Some(shell.clone()), + "the full result is kept for the transcript under {child_call_id}" + ); + } +} + /// The whole lifecycle in one test, because the assertions are sequential: a grant /// is reachable, serves its own tool set, and stops existing when its lease drops. #[tokio::test] @@ -341,8 +477,17 @@ async fn the_real_codex_provider_reaches_biorouters_tools_over_the_bridge() { use biorouter::providers::base::Provider; use biorouter::providers::codex::CodexProvider; + // ⚠ QA-E F1: this used to issue a grant over an EMPTY extension manager and + // accept any answer that NAMED the tool, on the argument that a refusal "can + // only have come from Biorouter's side of the bridge". It could also come from + // Codex's own side — "MCP tool call requires approval, but approval policy is + // never" names the tool too — so on codex-cli 0.148+ this passed while every + // bridged call was refused inside the CLI. The tool now answers with a random + // marker that exists only in Biorouter's dispatcher, so only a call that + // really crossed the bridge and ran can put it in the answer. + let marker = format!("CODEXBRIDGE{:016x}", rand::random::()); serve_real_bridge().await; - let lease = bridge::issue(grant().await).expect("the base URL is published"); + let lease = bridge::issue(marker_grant(&marker)).expect("the base URL is published"); // Drive the PROVIDER, not the CLI directly. `codex exec` cannot answer an // approval request, so an MCP tool call there fails with "user cancelled MCP @@ -356,7 +501,7 @@ async fn the_real_codex_provider_reaches_biorouters_tools_over_the_bridge() { let messages = vec![Message::user().with_text( "Call the spokeagent__query_knowledge_graph tool with cypher='MATCH (n) RETURN n LIMIT 1'. \ - Then report, in one line, the exact text the tool returned.", + Then reply with ONLY the marker value the tool returned, and nothing else.", )]; let outcome = bridge::ACTIVE_BRIDGE_URL @@ -373,14 +518,14 @@ async fn the_real_codex_provider_reaches_biorouters_tools_over_the_bridge() { match outcome { Ok((message, usage)) => { let text = message.as_concat_text(); - // The grant's ExtensionManager holds no real extension, so the call is - // refused by the gate stack rather than executed — and that refusal is - // the proof: it can only have come from Biorouter's side of the bridge. - // A child that never reached the bridge would report a missing tool - // instead. assert!( - text.contains("spokeagent__query_knowledge_graph"), - "the model should have reached Biorouter's tool; it said: {text}" + !text.contains("approval policy"), + "Codex refused the call itself instead of asking Biorouter: {text}" + ); + assert!( + text.contains(&marker), + "the bridged tool never ran: {marker} exists only in Biorouter's \ + dispatcher, and the answer was: {text}" ); assert_eq!( usage.provider.as_deref(), diff --git a/crates/biorouter/src/providers/claude_code.rs b/crates/biorouter/src/providers/claude_code.rs index 3292c2cf9..596f3fe3b 100644 --- a/crates/biorouter/src/providers/claude_code.rs +++ b/crates/biorouter/src/providers/claude_code.rs @@ -752,10 +752,16 @@ const PENDING_ARGS_CHARS: usize = 200; /// reached Biorouter over the tool bridge and ran behind its inspectors, /// permission mode, `.biorouterignore`, vault and privacy Gate C. The mark is /// what stops the agent loop dispatching it a second time. +/// +/// `bridge_url` is this turn's bridge, whose grant kept Biorouter's own result +/// for each call: the `Result` arm stores that rather than Claude Code's echo, +/// which has lost every annotation (QA-E F4 — see +/// [`mirror::stored_bridged_result`]). fn emit_tool_event( event: claude_stream::ToolBlockEvent, partial_args: &mut std::collections::HashMap, out: &tokio::sync::mpsc::UnboundedSender>, + bridge_url: Option<&str>, ) -> bool { let send = |item: ProviderStreamItem| out.send(Ok(item)).is_ok(); @@ -817,12 +823,24 @@ fn emit_tool_event( } claude_stream::ToolBlockEvent::Result { results } => { for result in results { - let message = mirror::response_message( + let message = match mirror::stored_bridged_result( + bridge_url, &result.tool_use_id, - mirror::content_from_value(&result.content), + &result.content, result.is_error, - mirror::Execution::Bridged, - ); + ) { + Some(recorded) => mirror::response_message_with_result( + &result.tool_use_id, + recorded, + mirror::Execution::Bridged, + ), + None => mirror::response_message( + &result.tool_use_id, + mirror::content_from_value(&result.content), + result.is_error, + mirror::Execution::Bridged, + ), + }; if !send((Some(message), None, None)) { return false; } @@ -849,6 +867,9 @@ struct PumpInputs { initial_prompt: transcript::Prompt, steering: Option, model_name: String, + /// Captured when the stream is built: the task-local is gone by the time + /// this task reads frames. + bridge_url: Option, out_tx: tokio::sync::mpsc::UnboundedSender>, } @@ -1011,6 +1032,7 @@ fn route_claude_frame( partial_args: &mut std::collections::HashMap, model_name: &str, out_tx: &tokio::sync::mpsc::UnboundedSender>, + bridge_url: Option<&str>, ) -> ClaudeFrameOutcome where S: futures::Stream>, @@ -1025,7 +1047,9 @@ where } claude_stream::RoutedFrame::Tool(event) => { // Everything the decoder already produced belongs before this card. - if !drain_ready(decoded, out_tx) || !emit_tool_event(event, partial_args, out_tx) { + if !drain_ready(decoded, out_tx) + || !emit_tool_event(event, partial_args, out_tx, bridge_url) + { ClaudeFrameOutcome::ConsumerClosed } else { ClaudeFrameOutcome::Continue @@ -1076,6 +1100,7 @@ struct ClaudeFrameContext<'a, S> { out_tx: &'a tokio::sync::mpsc::UnboundedSender>, completed_usage: &'a mut Option, outstanding_turns: &'a mut usize, + bridge_url: Option<&'a str>, } fn apply_claude_frame( @@ -1105,6 +1130,7 @@ where context.partial_args, context.model_name, context.out_tx, + context.bridge_url, ) { ClaudeFrameOutcome::Continue => ClaudeLoopOutcome::Continue, ClaudeFrameOutcome::ConsumerClosed => ClaudeLoopOutcome::Stop(None), @@ -1140,6 +1166,7 @@ async fn pump_claude_stdout(inputs: PumpInputs) { initial_prompt, mut steering, model_name, + bridge_url, out_tx, } = inputs; let (line_tx, line_rx) = tokio::sync::mpsc::unbounded_channel::>(); @@ -1210,6 +1237,7 @@ async fn pump_claude_stdout(inputs: PumpInputs) { out_tx: &out_tx, completed_usage: &mut completed_usage, outstanding_turns: &mut outstanding_turns, + bridge_url: bridge_url.as_deref(), }; match apply_claude_frame(router.push_line(&line), &mut context) { ClaudeLoopOutcome::Continue => {} @@ -1359,6 +1387,7 @@ impl ClaudeCodeProvider { })?; let bridge_config = bridge_mcp_config()?; + let bridge_url = bridge::active_bridge_url(); let model_config = self.model.clone(); let model_name = model_config.model_name.clone(); @@ -1415,6 +1444,7 @@ impl ClaudeCodeProvider { initial_prompt: prompt, steering, model_name, + bridge_url, out_tx, })); @@ -1569,6 +1599,93 @@ impl Provider for ClaudeCodeProvider { } } +#[cfg(test)] +mod bridged_result_tests { + use super::*; + use rmcp::model::{CallToolResult, Content}; + + const DATE: &str = "Thu Sep 11 01:00:00 PDT 2026"; + + fn shell_shaped(output: &str) -> CallToolResult { + CallToolResult::success(vec![ + Content::text(output).with_audience(vec![Role::Assistant]), + Content::text(output) + .with_audience(vec![Role::User]) + .with_priority(0.0), + ]) + } + + /// Feed one `tool_result` frame through the real handler and return the + /// `ToolResponse` it stored. + fn store( + tool_use_id: &str, + echo: Value, + is_error: bool, + bridge_url: Option<&str>, + ) -> crate::conversation::message::ToolResponse { + let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); + let mut partial_args = std::collections::HashMap::new(); + assert!(emit_tool_event( + claude_stream::ToolBlockEvent::Result { + results: vec![claude_stream::ToolResultBlock { + tool_use_id: tool_use_id.to_string(), + content: echo, + is_error, + detail: None, + }], + }, + &mut partial_args, + &tx, + bridge_url, + )); + let Ok(Ok((Some(message), _, _))) = rx.try_recv() else { + panic!("the handler must emit the response message"); + }; + let MessageContent::ToolResponse(response) = &message.content[0] else { + panic!("expected a tool response"); + }; + assert_eq!( + mirror::response_execution(response), + Some(mirror::Execution::Bridged) + ); + response.clone() + } + + /// QA-E F4 through the real handler. Claude Code echoes a shell result with + /// every annotation gone, so storing the echo made "2 results" of one shell + /// call and put the output twice into the next turn's prompt. What is stored + /// is the result the bridge kept for that call — both blocks, audiences + /// intact: one for the model, one for the card. + #[test] + fn a_bridged_result_is_stored_as_the_bridge_recorded_it() { + let recorded = shell_shaped(DATE); + let lease = bridge::lease_holding_for_test("toolu_F4", recorded.clone()); + // Claude Code 2.1.266's echo of the view the bridge handed it. + let echo = serde_json::json!([{ "type": "text", "text": DATE }]); + + let stored = store("toolu_F4", echo, false, Some(lease.url())); + + assert_eq!(stored.tool_result.as_ref().ok(), Some(&recorded)); + } + + /// A child that timed out waiting never saw Biorouter's result, and its + /// card says what it did see. + #[test] + fn a_child_that_timed_out_is_stored_as_it_saw_it() { + let lease = bridge::lease_holding_for_test("toolu_late", shell_shaped(DATE)); + let echo = serde_json::json!("The operation timed out"); + + let stored = store("toolu_late", echo, true, Some(lease.url())); + + let result = stored.tool_result.as_ref().expect("a successful transport"); + assert_eq!(result.is_error, Some(true)); + assert_eq!( + result.content[0].as_text().map(|t| t.text.as_str()), + Some("The operation timed out") + ); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/biorouter/src/providers/codex.rs b/crates/biorouter/src/providers/codex.rs index 4552124d1..f935a4adf 100644 --- a/crates/biorouter/src/providers/codex.rs +++ b/crates/biorouter/src/providers/codex.rs @@ -30,7 +30,9 @@ //! local model-controlled tools. Biorouter's own //! tools reach it over the one MCP bridge, and execute in Biorouter's dispatcher //! where every existing gate still fires. An unexpected approval request for a -//! command or file change is refused rather than rubber-stamped. +//! command or file change is refused rather than rubber-stamped; the thread's +//! approval policy refuses those inside the CLI and lets only the MCP tool-call +//! approval through, which Biorouter accepts for its own bridge alone. use std::path::{Path, PathBuf}; use std::time::Duration; @@ -420,13 +422,7 @@ impl CodexProvider { let server = AppServer::spawn_with_home(command, Some(home)).await?; match server - .request( - "initialize", - json!({ - "clientInfo": { "name": "biorouter", "version": env!("CARGO_PKG_VERSION") }, - "capabilities": { "experimentalApi": true } - }), - ) + .request("initialize", Self::initialize_params()) .await { Ok(_) => { @@ -462,6 +458,53 @@ impl CodexProvider { )) } + /// `initialize` parameters. + /// + /// Identity is declared here: Biorouter says who it is rather than + /// impersonating the vendor's own first-party client. + fn initialize_params() -> Value { + json!({ + "clientInfo": { "name": "biorouter", "version": env!("CARGO_PKG_VERSION") }, + "capabilities": { "experimentalApi": true } + }) + } + + /// The approval policy every Codex thread runs under. + /// + /// ⚠ **Not `"never"`, and the reason is a vendor change rather than a + /// preference.** An MCP tool call asks for approval unless its server + /// pre-approved it, and `never` auto-approves that ask only when the sandbox + /// has full disk write — which Biorouter's read-only child never has. From + /// codex-cli 0.148.0 the CLI then answers the ask itself, with + /// `MCP tool call requires approval, but approval policy is never`, before any + /// request reaches Biorouter. So on 0.153.4 every bridged tool failed on its + /// first call (QA-E F1, 2026-09-10); 0.147.0, which predates the check, sent + /// the ask to the host and worked. + /// + /// `granular` is the vendor's per-category form of the same policy: a `false` + /// category is "automatically rejected instead of shown to the user", which is + /// what `never` does, so the child's own command, rule, skill and permission + /// requests stay refused inside the CLI with no round trip. Only + /// `mcp_elicitations` is let through, because that is the channel the + /// tool-call approval arrives on — see [`Self::answer_elicitation`] for which + /// of those are then accepted. + /// + /// ⚠ The variant is `#[experimental("askForApproval.granular")]` in the + /// app-server protocol, so it parses only for a client that declared + /// `experimentalApi` in [`Self::initialize_params`]. Its shape is identical in + /// the 0.147.0 and 0.153.4 protocol. + fn approval_policy() -> Value { + json!({ + "granular": { + "sandbox_approval": false, + "rules": false, + "skill_approval": false, + "request_permissions": false, + "mcp_elicitations": true, + } + }) + } + /// `thread/start` parameters. /// /// `ephemeral` keeps Codex from writing its own session files: Biorouter owns @@ -480,7 +523,7 @@ impl CodexProvider { "cwd": cwd, "ephemeral": true, "sandbox": "read-only", - "approvalPolicy": "never", + "approvalPolicy": Self::approval_policy(), "baseInstructions": system, }); if !model.trim().is_empty() { @@ -698,16 +741,19 @@ impl CodexProvider { /// model-controlled tools are disabled when the app server starts, so an /// approval request here means the CLI has exposed an unexpected capability /// or is reaching for authority it was not given. The honest answer is no. - /// Elicitation is accepted because that is how an MCP tool call Biorouter - /// itself is serving gets its go-ahead — and those run - /// in Biorouter's dispatcher, behind Biorouter's gates. - /// ⚠ **Each of these five methods wants a DIFFERENT response shape**, and - /// they are not interchangeable. Every one of them used to be answered with + /// The one request accepted is Codex asking whether it may call a tool on + /// Biorouter's own bridge — see [`Self::answer_elicitation`]. + /// + /// ⚠ **Each method wants a DIFFERENT response shape**, and they are not + /// interchangeable. Every one of them used to be answered with /// `{"decision": "denied"}`, and `denied` is not a valid value for any of - /// them — verified against `codex app-server generate-json-schema` (0.147.0): + /// them — verified against `codex app-server generate-json-schema` (0.147.0, + /// and unchanged in 0.153.4): /// /// | Method | Response type | Refusal | /// |---|---|---| + /// | `mcpServer/elicitation/request` | `McpServerElicitationRequestResponse` | `{"action": "decline"}` | + /// | `item/tool/requestUserInput` | `ToolRequestUserInputResponse` — `{answers: {: …}}` | no answers: `{"answers": {}}` | /// | `item/commandExecution/requestApproval` | `CommandExecutionApprovalDecision` | `"decline"` (`accept`/`acceptForSession`/`acceptWithExecpolicyAmendment`/`applyNetworkPolicyAmendment`/`decline`/`cancel`) | /// | `item/fileChange/requestApproval` | `FileChangeApprovalDecision` | `"decline"` | /// | `item/permissions/requestApproval` | **not a decision at all** — `{permissions, scope?, strictAutoReview?}` | an empty `GrantedPermissionProfile`: grant nothing | @@ -716,9 +762,15 @@ impl CodexProvider { /// `decline` rather than `cancel`, and `denied` rather than `abort`, on /// purpose: both refuse the action while letting the turn continue, so the /// child can say why it could not proceed instead of the turn dying silently. - fn decide(method: &str) -> Value { + fn decide(method: &str, params: &Value) -> Value { match method { - "mcpServer/elicitation/request" => json!({ "action": "accept", "content": {} }), + "mcpServer/elicitation/request" => Self::answer_elicitation(params), + // Where Codex sends the MCP tool-call approval when its + // `tool_call_mcp_elicitation` feature is off (and where its own + // `request_user_input` tool asks). The parameters name no server, so + // the request cannot be scoped to the bridge and is not accepted; no + // answers is the refusal, which Codex reads as a cancel. + "item/tool/requestUserInput" => json!({ "answers": {} }), "item/commandExecution/requestApproval" | "item/fileChange/requestApproval" => { json!({ "decision": "decline" }) } @@ -739,6 +791,42 @@ impl CodexProvider { } } + /// Answer `mcpServer/elicitation/request` — which is how codex-cli asks + /// whether it may make an MCP tool call, not only how an MCP server asks a + /// person a question. + /// + /// The tool-call approval has no method of its own. Codex sends it here, + /// marked `_meta.codex_approval_kind: "mcp_tool_call"`, with an empty + /// `requestedSchema` (feature `tool_call_mcp_elicitation`, stable and on by + /// default). Measured against 0.153.4 — the captured request is the fixture + /// of `a_tool_call_approval_for_the_bridge_is_accepted_once`. An accept with + /// empty content and no `persist` choice is Codex's one-time "Allow", so the + /// next call is asked again rather than remembered in the child. + /// + /// It is accepted only for the bridge ([`BRIDGE_SERVER`]): that call is + /// inspected, permission-checked and privacy-gated again on Biorouter's side, + /// so Codex's own ask adds nothing Biorouter would say no to. A tool call to + /// any other server is an isolation regression — the child's `CODEX_HOME` + /// holds no other — and is declined, which Codex reports to its model as a + /// rejected call and the turn continues. + /// + /// Everything else here is declined too. With `mcp_elicitations` allowed by + /// [`Self::approval_policy`], an MCP server's own form or URL elicitation now + /// reaches Biorouter instead of being declined inside Codex, and nobody here + /// can fill one in: an empty accept would submit a form no person saw. + fn answer_elicitation(params: &Value) -> Value { + let tool_call_approval = params + .pointer("/_meta/codex_approval_kind") + .and_then(Value::as_str) + == Some("mcp_tool_call"); + let for_bridge = params.get("serverName").and_then(Value::as_str) == Some(BRIDGE_SERVER); + if tool_call_approval && for_bridge { + json!({ "action": "accept", "content": {} }) + } else { + json!({ "action": "decline" }) + } + } + /// Fold one notification into the turn's outcome. Returns true when the turn /// is over. fn absorb(outcome: &mut TurnOutcome, method: &str, params: &Value) -> bool { @@ -1001,7 +1089,9 @@ impl CodexProvider { turn_id_tx.send_replace(Some(turn_id)); Ok::<(), ProviderError>(()) }; - let pump = Self::stream_pump(server, model, &thread_id, turn_id_rx, tx, steering); + let pump = Self::stream_pump( + server, model, &thread_id, turn_id_rx, tx, steering, bridge_url, + ); let (started, pumped) = coding_agent::await_turn( async { tokio::join!(start, pump) }, @@ -1019,6 +1109,9 @@ impl CodexProvider { } /// Read notifications, decode them, and forward each decoded event. + /// + /// `bridge_url` is this turn's bridge, whose grant kept Biorouter's own + /// result for each bridged call — see [`emit_codex_tool_event`]. async fn stream_pump( server: &AppServer, model: &ModelConfig, @@ -1026,6 +1119,7 @@ impl CodexProvider { mut turn_id_rx: tokio::sync::watch::Receiver>, tx: &tokio::sync::mpsc::UnboundedSender>, mut steering: Option, + bridge_url: Option<&str>, ) -> Result<(), ProviderError> { let mut decoder = codex_stream::CodexDecoder::new(); let mut streamed_anything = false; @@ -1065,8 +1159,8 @@ impl CodexProvider { break; }; match message { - Inbound::Request { id, method, .. } => { - server.respond(&id, Self::decide(&method)).await?; + Inbound::Request { id, method, params } => { + server.respond(&id, Self::decide(&method, ¶ms)).await?; } Inbound::Notification { method, params } => { match Self::emit_stream_notification( @@ -1076,6 +1170,7 @@ impl CodexProvider { ¶ms, tx, &mut streamed_anything, + bridge_url, )? { StreamPumpEvent::Continue => {} StreamPumpEvent::ConsumerClosed => return Ok(()), @@ -1211,6 +1306,7 @@ impl CodexProvider { params: &Value, tx: &tokio::sync::mpsc::UnboundedSender>, streamed_anything: &mut bool, + bridge_url: Option<&str>, ) -> Result { for event in decoder.push(method, params) { match event { @@ -1252,7 +1348,7 @@ impl CodexProvider { return Ok(StreamPumpEvent::Terminal); } codex_stream::CodexEvent::Tool(event) => { - if !emit_codex_tool_event(*event, tx) { + if !emit_codex_tool_event(*event, tx, bridge_url) { return Ok(StreamPumpEvent::ConsumerClosed); } } @@ -1322,8 +1418,8 @@ impl CodexProvider { let mut outcome = TurnOutcome::default(); while let Some(message) = server.next_inbound().await { match message { - Inbound::Request { id, method, .. } => { - server.respond(&id, Self::decide(&method)).await?; + Inbound::Request { id, method, params } => { + server.respond(&id, Self::decide(&method, ¶ms)).await?; } Inbound::Notification { method, params } => { if Self::absorb(&mut outcome, &method, ¶ms) { @@ -1398,10 +1494,15 @@ fn codex_tool_identity(kind: &codex_stream::CodexToolKind) -> (String, Value, mi /// /// `item/started` raises the skeleton card; `item/completed` mints the marked /// request/response pair that settles it. The pairing id is the Codex item id, -/// which both halves carry. +/// which both halves carry — and which is also the `callId` Codex sent on the +/// `tools/call`, so for a bridged call it names the result the grant behind +/// `bridge_url` kept. That result is stored rather than Codex's echo, which +/// carries only the view the bridge handed the child (QA-E F4 — see +/// [`mirror::stored_bridged_result`]). fn emit_codex_tool_event( event: codex_stream::CodexToolEvent, tx: &tokio::sync::mpsc::UnboundedSender>, + bridge_url: Option<&str>, ) -> bool { let (name, base_args, exec) = codex_tool_identity(&event.kind); @@ -1440,6 +1541,22 @@ fn emit_codex_tool_event( == Some(true); let is_error = event.error.is_some() || failed || bad_exit || declined || tool_failed; + if exec == mirror::Execution::Bridged { + // What the child got back: its transport error, else the result. + let echoed = match (&event.error, &event.result) { + (Some(error), _) => Value::String(error.clone()), + (None, Some(result)) => result.clone(), + (None, None) => Value::Null, + }; + if let Some(mut recorded) = + mirror::stored_bridged_result(bridge_url, &event.id, &echoed, is_error) + { + recorded.is_error = Some(is_error); + let response = mirror::response_message_with_result(&event.id, recorded, exec); + return tx.send(Ok((Some(response), None, None))).is_ok(); + } + } + if event.error.is_none() && event.aggregated_output.is_none() && !declined { if let Some(mut result) = event.result.as_ref().and_then(|value| { serde_json::from_value::(value.clone()).ok() @@ -2072,7 +2189,7 @@ for line in sys.stdin: p["ephemeral"], true, "Codex must not persist its own transcript" ); - assert_eq!(p["approvalPolicy"], "never"); + assert_eq!(p["approvalPolicy"], CodexProvider::approval_policy()); assert_eq!( p["baseInstructions"], "SYSTEM", "Biorouter's prompt replaces Codex's own preamble" @@ -2081,6 +2198,58 @@ for line in sys.stdin: assert_eq!(p["model"], "gpt-5.5"); } + /// QA-E F1: under `"never"`, codex-cli ≥ 0.148.0 refuses every MCP tool call + /// inside the CLI ("MCP tool call requires approval, but approval policy is + /// never") before Biorouter is asked anything, so the bridge was dead on + /// 0.153.4. The policy must let exactly the MCP-elicitation category through + /// — that is the channel the tool-call approval arrives on — and keep every + /// child-local category refused, which is what `false` means in `granular`. + #[test] + fn the_approval_policy_lets_only_mcp_elicitations_through() { + let policy = CodexProvider::approval_policy(); + assert_ne!( + policy, "never", + "`never` makes codex-cli refuse every bridged MCP tool call itself" + ); + let granular = policy + .get("granular") + .and_then(Value::as_object) + .unwrap_or_else(|| panic!("expected the granular form, got {policy}")); + assert_eq!(granular.get("mcp_elicitations"), Some(&json!(true))); + // Every child-local category is refused without a round trip, exactly as + // `never` refused it. Named explicitly, including the two the schema + // defaults to false, so a future default flip cannot open one silently. + for refused in [ + "sandbox_approval", + "rules", + "skill_approval", + "request_permissions", + ] { + assert_eq!( + granular.get(refused), + Some(&json!(false)), + "{refused} must stay refused: the child's own command execution and \ + file changes are not Biorouter's to approve" + ); + } + assert_eq!( + granular.len(), + 5, + "an unknown category is a new approval surface: {policy}" + ); + } + + /// `granular` is gated behind `#[experimental("askForApproval.granular")]` in + /// the app-server protocol, so it is only accepted from a client that + /// declared the experimental API at `initialize`. Dropping that capability + /// would make every `thread/start` fail — pinned here so the two cannot drift. + #[test] + fn initialize_declares_the_experimental_api_the_policy_needs() { + let params = CodexProvider::initialize_params(); + assert_eq!(params["capabilities"]["experimentalApi"], true); + assert_eq!(params["clientInfo"]["name"], "biorouter"); + } + /// An empty model means "whatever Codex defaults to", which must be expressed /// by omitting the key rather than sending an empty string. #[test] @@ -2290,8 +2459,106 @@ for line in sys.stdin: ); } - /// Every approval that would let the child act on the machine is refused; - /// elicitation — how a Biorouter-served MCP tool call is cleared — is accepted. + /// The MCP tool-call approval exactly as codex-cli 0.153.4 sent it, captured + /// from a live `codex app-server` under the granular policy on 2026-09-11 + /// (ids shortened). It is NOT a method of its own: it rides + /// `mcpServer/elicitation/request`, marked by `_meta.codex_approval_kind`. + fn captured_tool_call_approval(server: &str) -> Value { + json!({ + "threadId": "01a08f7e-f25c", "turnId": "01a08f7e-f313", + "serverName": server, "mode": "form", + "_meta": { + "codex_approval_kind": "mcp_tool_call", + "persist": ["session", "always"], + "tool_description": "Echo the given text back.", + "tool_params": {"text": "hi"}, + "tool_params_display": [{"name": "text", "value": "hi", "display_name": "text"}] + }, + "message": "Allow the biorouter MCP server to run tool \"echo\"?", + "requestedSchema": {"type": "object", "properties": {}} + }) + } + + /// QA-E F1: the approval Codex asks before calling a bridged tool is + /// accepted — the call then runs behind Biorouter's own inspectors, + /// permission mode, `.biorouterignore`, vault and privacy Gate C. + /// + /// The answer carries no `persist` choice on purpose: Codex reads an accept + /// with none as a one-time `Approved`, so nothing is remembered in the child + /// and every later call is asked again (and so gated again on our side). + #[test] + fn a_tool_call_approval_for_the_bridge_is_accepted_once() { + let answer = CodexProvider::decide( + "mcpServer/elicitation/request", + &captured_tool_call_approval(BRIDGE_SERVER), + ); + assert_eq!(answer["action"], "accept", "got {answer}"); + assert_eq!(answer["content"], json!({})); + assert!( + answer.get("_meta").is_none(), + "no `persist` choice: `session`/`always` would let the child skip the ask \ + for the rest of the turn (got {answer})" + ); + } + + /// Only the bridge is Biorouter's to vouch for. The child runs under an + /// isolated `CODEX_HOME` with no other MCP server, so a tool call to one is + /// an isolation regression, and the answer to it is no. + #[test] + fn a_tool_call_approval_for_any_other_server_is_declined() { + let answer = CodexProvider::decide( + "mcpServer/elicitation/request", + &captured_tool_call_approval("personal-clinical-db"), + ); + assert_eq!(answer["action"], "decline", "got {answer}"); + } + + /// With `mcp_elicitations` allowed, an elicitation that is NOT a tool-call + /// approval can now reach Biorouter too (under `never` Codex declined those + /// itself). None of them is something Biorouter can answer for a person, so + /// each is declined — in particular an empty accept must not be sent to a + /// form that asked for fields, or to a URL flow. + #[test] + fn an_elicitation_that_is_not_a_tool_call_approval_is_declined() { + let form = json!({ + "threadId": "t", "serverName": BRIDGE_SERVER, "mode": "form", + "message": "Which cohort?", + "requestedSchema": {"type": "object", "properties": {"cohort": {"type": "string"}}} + }); + let url = json!({ + "threadId": "t", "serverName": BRIDGE_SERVER, "mode": "url", + "elicitationId": "e1", "message": "Sign in", "url": "https://example.invalid/" + }); + let mut other_kind = captured_tool_call_approval(BRIDGE_SERVER); + other_kind["_meta"]["codex_approval_kind"] = json!("tool_suggestion"); + for params in [form, url, other_kind] { + let answer = CodexProvider::decide("mcpServer/elicitation/request", ¶ms); + assert_eq!(answer["action"], "decline", "{params} got {answer}"); + } + } + + /// `item/tool/requestUserInput` is where Codex sends the tool-call approval + /// when its `tool_call_mcp_elicitation` feature is off, and where its own + /// `request_user_input` tool would ask. Its response is + /// `{answers: {: …}}`; the catch-all's `{decision}` is not + /// that shape at all. No answers is the valid refusal: the approval parser + /// reads it as a cancel. + #[test] + fn a_user_input_request_is_refused_in_its_own_shape() { + let params = json!({ + "threadId": "t", "turnId": "u", "itemId": "exec-1", "isBlocking": true, + "questions": [{ + "id": "mcp_tool_call_approval_exec-1", + "header": "Approve app tool call?", + "question": "Allow the biorouter MCP server to run tool \"echo\"?", + "options": [{"label": "Allow", "description": "Run the tool and continue."}] + }] + }); + let answer = CodexProvider::decide("item/tool/requestUserInput", ¶ms); + assert_eq!(answer, json!({ "answers": {} })); + } + + /// Every approval that would let the child act on the machine is refused. /// /// ⚠ This test used to assert `decision == "denied"` for all five methods. /// That is not a valid value for **any** of them, so the refusals were being @@ -2300,12 +2567,7 @@ for line in sys.stdin: /// response schema defines (`codex app-server generate-json-schema`, 0.147.0) /// — which is three different shapes, not one. #[test] - fn only_elicitation_is_accepted() { - assert_eq!( - CodexProvider::decide("mcpServer/elicitation/request")["action"], - "accept" - ); - + fn child_local_approvals_are_refused() { // `*ApprovalDecision`: a plain string. `decline` refuses the action and // lets the turn continue; `cancel` would kill the turn. for refused in [ @@ -2313,7 +2575,7 @@ for line in sys.stdin: "item/fileChange/requestApproval", ] { assert_eq!( - CodexProvider::decide(refused)["decision"], + CodexProvider::decide(refused, &Value::Null)["decision"], "decline", "{refused} takes a *ApprovalDecision, whose refusal is `decline`" ); @@ -2321,7 +2583,7 @@ for line in sys.stdin: // Not a decision at all: the response IS the permission grant, so // granting nothing is how it is refused. - let permissions = CodexProvider::decide("item/permissions/requestApproval"); + let permissions = CodexProvider::decide("item/permissions/requestApproval", &Value::Null); assert!( permissions .get("permissions") @@ -2335,7 +2597,7 @@ for line in sys.stdin: // Legacy `ReviewDecision`: the refusal that continues the turn is the // OBJECT form. The bare string `denied` is not in the enum. for legacy in ["applyPatchApproval", "execCommandApproval"] { - let answer = CodexProvider::decide(legacy); + let answer = CodexProvider::decide(legacy, &Value::Null); assert!( answer["decision"]["denied"]["rejection"].is_string(), "{legacy} takes a ReviewDecision, whose continue-the-turn refusal \ @@ -2354,7 +2616,7 @@ for line in sys.stdin: /// forever waiting for one. #[test] fn an_unknown_request_is_still_answered() { - let answer = CodexProvider::decide("some/future/request"); + let answer = CodexProvider::decide("some/future/request", &Value::Null); assert!( answer.is_object() && !answer.as_object().unwrap().is_empty(), "an unanswered server request blocks the turn indefinitely" @@ -3022,7 +3284,7 @@ for line in sys.stdin: let (sender, mut receiver) = tokio::sync::mpsc::unbounded_channel(); for event in events { if let codex_stream::CodexEvent::Tool(event) = event { - assert!(emit_codex_tool_event(*event, &sender)); + assert!(emit_codex_tool_event(*event, &sender, None)); } } let mut messages = Vec::new(); @@ -3037,6 +3299,74 @@ for line in sys.stdin: ); } + /// The `ToolResponse` a completed `mcpToolCall` item is stored as. + fn stored_response(item: Value, bridge_url: Option<&str>) -> rmcp::model::CallToolResult { + let mut decoder = codex_stream::CodexDecoder::new(); + let (sender, mut receiver) = tokio::sync::mpsc::unbounded_channel(); + for event in decoder.push("item/completed", &json!({ "item": item })) { + if let codex_stream::CodexEvent::Tool(event) = event { + assert!(emit_codex_tool_event(*event, &sender, bridge_url)); + } + } + let mut results = Vec::new(); + while let Ok(Ok((Some(message), _, _))) = receiver.try_recv() { + for content in message.content { + if let MessageContent::ToolResponse(response) = content { + results.push(response.tool_result.expect("a successful transport")); + } + } + } + assert_eq!(results.len(), 1, "one response per completed call"); + results.remove(0) + } + + fn shell_shaped(output: &str) -> rmcp::model::CallToolResult { + rmcp::model::CallToolResult::success(vec![ + rmcp::model::Content::text(output).with_audience(vec![Role::Assistant]), + rmcp::model::Content::text(output) + .with_audience(vec![Role::User]) + .with_priority(0.0), + ]) + } + + /// QA-E F4: Codex's echo carries only the view the bridge handed the child, + /// so the stored result is the one the bridge kept for that `callId` — both + /// blocks, audiences intact. + #[test] + fn a_bridged_result_is_stored_as_the_bridge_recorded_it() { + let recorded = shell_shaped("Thu Sep 11"); + let lease = bridge::lease_holding_for_test("exec-f4", recorded.clone()); + let item = json!({ + "id": "exec-f4", "type": "mcpToolCall", "server": "biorouter", + "tool": "developer__shell", "status": "completed", + "arguments": {"command": "date"}, + "result": {"content": [{"type": "text", "text": "Thu Sep 11"}], + "structuredContent": null, "_meta": null} + }); + + assert_eq!(stored_response(item, Some(lease.url())), recorded); + } + + /// A call the child never got an answer to is stored as the failure the + /// child saw, not as the result Biorouter eventually produced. + #[test] + fn a_bridged_call_the_child_never_got_is_stored_as_it_failed() { + let lease = bridge::lease_holding_for_test("exec-late", shell_shaped("Thu Sep 11")); + let item = json!({ + "id": "exec-late", "type": "mcpToolCall", "server": "biorouter", + "tool": "developer__shell", "status": "failed", "arguments": {}, + "result": null, "error": {"message": "tool call error: request timed out"} + }); + + let stored = stored_response(item, Some(lease.url())); + + assert_eq!(stored.is_error, Some(true)); + assert_eq!( + stored.content[0].as_text().map(|t| t.text.as_str()), + Some("tool call error: request timed out") + ); + } + #[test] fn completed_mcp_transport_preserves_typed_content_and_result_metadata() { let expected = json!({ @@ -3060,7 +3390,7 @@ for line in sys.stdin: }}), ) { if let codex_stream::CodexEvent::Tool(event) = event { - assert!(emit_codex_tool_event(*event, &sender)); + assert!(emit_codex_tool_event(*event, &sender, None)); } } let mut results = Vec::new(); diff --git a/crates/biorouter/src/providers/coding_agent/bridge.rs b/crates/biorouter/src/providers/coding_agent/bridge.rs index 0a9ac2689..3ba471dc0 100644 --- a/crates/biorouter/src/providers/coding_agent/bridge.rs +++ b/crates/biorouter/src/providers/coding_agent/bridge.rs @@ -52,7 +52,7 @@ //! capability than the daemon's REST API — one session's tools, for one turn. use std::collections::HashMap; -use std::sync::{Arc, LazyLock, RwLock}; +use std::sync::{Arc, LazyLock, Mutex, RwLock}; use std::time::Duration; use rmcp::model::{CallToolRequestParams, CallToolResult, Tool}; @@ -67,6 +67,7 @@ use crate::pending_user_action::{ }; use crate::permission::tool_risk::ToolRiskRegistry; use crate::privacy::CallCapability; +use crate::providers::formats::audience; use crate::session::session_manager::Session; use crate::tool_inspection::ToolInspectionManager; @@ -194,8 +195,22 @@ pub struct BridgeGrant { /// is what stops a panicking turn leaving a child blocked on an HTTP response /// nobody will ever answer. nonce: String, + /// Biorouter's own result for each call the child made, under the child's + /// own id for the call, until the provider mirrors that call into the + /// transcript. The child is handed only [`child_view`] of a result, and its + /// echo of even that is lossy, so this is where the transcript gets what + /// the tool actually returned. See [`take_recorded_result`]. + recorded: Mutex>, } +/// How many results one grant holds for the transcript at a time. +/// +/// Each is taken the moment its call is mirrored, so in practice the map holds +/// only calls between the bridge answering and the child reporting. The cap +/// bounds a child that never reports (one that crashed mid-turn): past it a +/// call is not recorded, and its transcript entry falls back to the echo. +const MAX_RECORDED_RESULTS: usize = 64; + /// The per-call MCP deadline Biorouter asks each child CLI to apply (#110). /// /// Both CLIs apply a hard per-call wall clock and abandon the request when it @@ -370,6 +385,7 @@ impl BridgeGrant { vault, tool_risks, nonce: String::new(), + recorded: Mutex::new(HashMap::new()), } } @@ -432,6 +448,35 @@ impl BridgeGrant { outcome } + /// [`Self::call`], answered the way the child must receive it. + /// + /// The child gets [`child_view`] of the result: what a model is sent. The + /// full result is kept under `child_call_id` — the child's own id for the + /// call, see [`child_call_id`] — so the transcript can store what the tool + /// actually returned rather than the child's echo of the view + /// ([`take_recorded_result`]). + pub async fn call_for_child( + &self, + call: CallToolRequestParams, + child_call_id: Option, + ) -> Result { + let result = self.call(call).await?; + if let Some(id) = child_call_id { + self.record(id, &result); + } + Ok(child_view(&result)) + } + + /// Keep `result` for the transcript, under the child's id for the call. + fn record(&self, child_call_id: String, result: &CallToolResult) { + let Ok(mut recorded) = self.recorded.lock() else { + return; + }; + if recorded.len() < MAX_RECORDED_RESULTS || recorded.contains_key(&child_call_id) { + recorded.insert(child_call_id, result.clone()); + } + } + /// The body of one bridged call, from the path jail to the tool's result. /// /// Split out of [`Self::call`] only so that the staged-context drain there @@ -950,6 +995,118 @@ pub fn advertised_tool_names(bridge_url: &str) -> Vec { .unwrap_or_default() } +/// What the child is handed: the blocks a model would be sent, unannotated. +/// +/// A Biorouter tool can address the same output to two readers — +/// `developer__shell` returns it once with `audience: ["assistant"]` and once, +/// reformatted, with `audience: ["user"]` — and every provider formatter sends +/// the model only the blocks addressed to it ([`audience::is_for_model`]). The +/// bridge is the coding agents' formatter: whatever it returns, the child's +/// model reads, and neither CLI filters by audience. It returned every block, +/// so the child read each result twice (QA-E F4). +/// +/// Annotations are removed from what remains. After the filter they tell a +/// model nothing, and codex-cli cannot parse a result whose block carries +/// `priority`: measured on 0.153.4, `priority: 0.0` alone fails the call with +/// "Unexpected response type" while `audience` alone does not — and the +/// shell's user block carries `priority: 0.0`. +/// +/// Nothing is lost to the transcript: [`BridgeGrant::call_for_child`] keeps the +/// full result for [`take_recorded_result`]. +pub fn child_view(result: &CallToolResult) -> CallToolResult { + let mut view = result.clone(); + view.content.retain(audience::is_for_model); + for block in &mut view.content { + block.annotations = None; + } + view +} + +/// The `_meta` keys under which each CLI names its own `tools/call`. +/// +/// Measured on 2026-09-11: `claude` 2.1.266 sends `claudecode/toolUseId`, the +/// same id as the stream's `tool_use` / `tool_result`; codex-cli 0.153.4 sends +/// `callId`, the same id as the `mcpToolCall` item. +const CHILD_CALL_ID_KEYS: [&str; 2] = ["claudecode/toolUseId", "callId"]; + +/// The child's own id for a `tools/call`, read from the request's `_meta`. +/// +/// It is what pairs the result Biorouter keeps with the frame on which the +/// child later reports the call. `None` when the CLI sent neither key; the +/// transcript then falls back to the child's echo. +pub fn child_call_id(meta: Option<&serde_json::Value>) -> Option { + let meta = meta?; + CHILD_CALL_ID_KEYS + .iter() + .find_map(|key| meta.get(*key).and_then(serde_json::Value::as_str)) + .map(str::to_string) +} + +/// Take Biorouter's own result for one of the child's calls, if the grant +/// behind `bridge_url` kept it. Taken, not read: each call is mirrored once. +pub fn take_recorded_result(bridge_url: &str, child_call_id: &str) -> Option { + let nonce = bridge_url.trim_end_matches('/').rsplit('/').next()?; + let grant = lookup(nonce)?; + let mut recorded = grant.recorded.lock().ok()?; + recorded.remove(child_call_id) +} + +/// A dispatcher for grants that exist only to hold results. +/// +/// ⚠ Deliberately not an `ExtensionManager`: building one reaches +/// `SessionManager::instance()`, whose sqlx pool panics outside a Tokio +/// runtime — and a panic inside that process-global `LazyLock` poisons it for +/// every later test in the binary, which then all fail as "previously +/// poisoned" far from the cause. +#[cfg(test)] +struct InertDispatch; + +#[cfg(test)] +#[async_trait::async_trait] +impl BridgeToolDispatch for InertDispatch { + async fn dispatch( + &self, + _session_id: &str, + _call: CallToolRequestParams, + _capability: CallCapability, + _cancel: CancellationToken, + ) -> Result { + Err("this test grant dispatches nothing".to_string()) + } +} + +/// A grant that dispatches nothing and needs no runtime to build. +#[cfg(test)] +pub(crate) fn inert_grant_for_test() -> BridgeGrant { + BridgeGrant::new( + Session::default(), + BioRouterMode::Auto, + Arc::new(InertDispatch), + Arc::new(ToolInspectionManager::new()), + CallCapability::public_enforced(), + Vec::new(), + Conversation::new_unvalidated(vec![]), + None, + Arc::new(crate::hooks::HooksManager::with_config( + Default::default(), + false, + Arc::new(tokio::sync::Mutex::new(None)), + )), + None, + Arc::new(ToolRiskRegistry::new()), + ) +} + +/// A live lease whose grant already holds `result` for `child_call_id`, as if +/// the child had just made that call — for the providers' mirror tests. +#[cfg(test)] +pub(crate) fn lease_holding_for_test(child_call_id: &str, result: CallToolResult) -> BridgeLease { + publish_base_url("http://127.0.0.1:65535"); + let grant = inert_grant_for_test(); + grant.record(child_call_id.to_string(), &result); + issue(grant).expect("a base URL was just published") +} + /// How many grants are live. /// /// Diagnostic rather than test-facing: `GRANTS` is process-global, so a count is @@ -1082,6 +1239,135 @@ mod tests { assert!(recorder.calls.lock().unwrap().is_empty()); } + /// The developer shell's result shape (`rmcp_developer.rs`): the output for + /// the assistant, and a copy for the user marked low priority. + fn shell_shaped_result(output: &str) -> CallToolResult { + CallToolResult::success(vec![ + rmcp::model::Content::text(output).with_audience(vec![rmcp::model::Role::Assistant]), + rmcp::model::Content::text(format!("user copy: {output}")) + .with_audience(vec![rmcp::model::Role::User]) + .with_priority(0.0), + ]) + } + + struct FixedResultDispatch(CallToolResult); + + #[async_trait::async_trait] + impl BridgeToolDispatch for FixedResultDispatch { + async fn dispatch( + &self, + _session_id: &str, + _call: CallToolRequestParams, + _capability: CallCapability, + _cancel: CancellationToken, + ) -> Result { + Ok(self.0.clone()) + } + } + + /// QA-E F4: the child is handed what a model is sent — the assistant's + /// block — and nothing a model does not read. Every block used to go, so + /// the child read the shell's output twice; and the user block's `priority` + /// alone made codex-cli 0.153.4 fail the whole call. + #[test] + fn the_child_is_handed_only_the_model_facing_block_unannotated() { + let view = child_view(&shell_shaped_result("Thu Sep 11")); + assert_eq!(view.content.len(), 1, "one block for the model: {view:?}"); + assert_eq!( + view.content[0].as_text().map(|t| t.text.as_str()), + Some("Thu Sep 11") + ); + assert!( + view.content[0].annotations.is_none(), + "annotations must not reach the child: {view:?}" + ); + let wire = serde_json::to_string(&view).expect("a serialisable result"); + assert!(!wire.contains("priority"), "codex cannot parse it: {wire}"); + } + + /// Held to the same five-case fixture every provider formatter is. + #[test] + fn the_child_view_filters_exactly_as_a_formatter_does() { + let view = child_view(&CallToolResult::success(audience::every_audience_case())); + let texts: Vec<&str> = view + .content + .iter() + .filter_map(|c| c.as_text().map(|t| t.text.as_str())) + .collect(); + assert_eq!(texts, audience::MODEL_VISIBLE.to_vec()); + } + + /// The error flag and structured content are the tool's answer, not + /// display hints, and survive the view. + #[test] + fn the_child_view_keeps_the_error_flag_and_structured_content() { + let mut result = CallToolResult::error(vec![rmcp::model::Content::text("boom")]); + result.structured_content = Some(serde_json::json!({ "code": 7 })); + let view = child_view(&result); + assert_eq!(view.is_error, Some(true)); + assert_eq!( + view.structured_content, + Some(serde_json::json!({ "code": 7 })) + ); + } + + /// Each CLI's own `_meta` key, as measured on 2026-09-11. + #[test] + fn each_cli_names_its_call_under_its_own_meta_key() { + let claude = serde_json::json!({ "claudecode/toolUseId": "toolu_01", "progressToken": 2 }); + let codex = serde_json::json!({ "callId": "exec-1", "threadId": "t", "itemId": "ctc_1" }); + assert_eq!(child_call_id(Some(&claude)).as_deref(), Some("toolu_01")); + assert_eq!(child_call_id(Some(&codex)).as_deref(), Some("exec-1")); + assert_eq!( + child_call_id(Some(&serde_json::json!({ "progressToken": 2 }))), + None + ); + assert_eq!(child_call_id(None), None); + } + + /// The bridge answers with the view and keeps the full result under the + /// child's id — once, because each call is mirrored once. + #[tokio::test] + async fn a_call_for_the_child_keeps_the_full_result_under_its_id() { + publish_base_url("http://127.0.0.1:65535"); + let mut grant = dummy_grant(); + grant.dispatcher = Arc::new(FixedResultDispatch(shell_shaped_result("out"))); + grant.inspections = Arc::new(inspections_with(&grant.hooks, false)); + let lease = issue(grant).expect("a base URL is published"); + let grant = lookup(lease.url().rsplit('/').next().unwrap()).unwrap(); + + let answered = grant + .call_for_child( + CallToolRequestParams { + name: "developer__shell".into(), + arguments: Some(serde_json::Map::new()), + meta: None, + task: None, + }, + Some("toolu_7".to_string()), + ) + .await + .expect("approved in Auto mode"); + + assert_eq!(answered, child_view(&shell_shaped_result("out"))); + assert_eq!( + take_recorded_result(lease.url(), "toolu_7"), + Some(shell_shaped_result("out")), + "the full result, annotations and all, is kept for the transcript" + ); + assert_eq!(take_recorded_result(lease.url(), "toolu_7"), None); + } + + /// A child that never reports its calls cannot grow the grant without bound. + #[test] + fn a_grant_keeps_a_bounded_number_of_results() { + let grant = inert_grant_for_test(); + for i in 0..MAX_RECORDED_RESULTS + 5 { + grant.record(format!("call-{i}"), &CallToolResult::success(vec![])); + } + assert_eq!(grant.recorded.lock().unwrap().len(), MAX_RECORDED_RESULTS); + } + struct NestedApprovalDispatch; #[async_trait::async_trait] diff --git a/crates/biorouter/src/providers/coding_agent/mirror.rs b/crates/biorouter/src/providers/coding_agent/mirror.rs index 4e0670a4c..820b63cf9 100644 --- a/crates/biorouter/src/providers/coding_agent/mirror.rs +++ b/crates/biorouter/src/providers/coding_agent/mirror.rs @@ -57,6 +57,7 @@ use crate::conversation::message::{ Message, MessageContent, ProviderMetadata, ToolRequest, ToolResponse, }; +use crate::providers::formats::audience; /// The reserved `ProviderMetadata` key. Namespaced, because the map is shared /// with whatever a provider chooses to record there. @@ -583,6 +584,234 @@ pub fn content_from_value(value: &serde_json::Value) -> Vec Vec { + use serde_json::Value; + match value { + Value::String(text) => vec![text.clone()], + Value::Array(blocks) => blocks + .iter() + .filter_map(|block| match block.get("type").and_then(Value::as_str) { + Some("text") => block.get("text").and_then(Value::as_str), + Some("resource") => block.pointer("/resource/text").and_then(Value::as_str), + _ => None, + }) + .map(str::to_string) + .collect(), + Value::Object(result) => result.get("content").map(echoed_texts).unwrap_or_default(), + _ => Vec::new(), + } +} + +/// Biorouter's own result for a bridged call, when the child demonstrably got it. +/// +/// The bridge hands the child only [`super::bridge::child_view`] — the blocks a +/// model reads, with their annotations removed — and keeps the full result. The +/// vendor's echo of that view is lossy on top: Claude Code drops every +/// annotation and rewrites an embedded resource as +/// `[Resource from biorouter at ] `. So the transcript stores the full +/// result instead, and the card counts its user-facing block and the next +/// turn's prompt its model-facing one, exactly as for any other provider's call +/// (QA-E F4). +/// +/// Only when the echo shows the child received it, though. A child whose call +/// timed out (#110), or whose CLI truncated a large result, worked from +/// something else, and the transcript records what the child actually saw: the +/// error flag must agree, and every text Biorouter sent must appear in what the +/// child echoed. +#[must_use] +pub fn recorded_if_received( + recorded: rmcp::model::CallToolResult, + echoed_texts: &[String], + echoed_is_error: bool, +) -> Option { + if recorded.is_error.unwrap_or(false) != echoed_is_error { + return None; + } + let echoed = echoed_texts.join("\n"); + let received = super::bridge::child_view(&recorded) + .content + .iter() + .filter_map(audience::flattened_text) + .all(|sent| echoed.contains(sent.as_str())); + received.then_some(recorded) +} + +/// The result to store for a bridged call: Biorouter's own when the bridge +/// kept it and the child's echo shows the child got it, else `None` — the +/// caller then stores the echo, as it always did. +#[must_use] +pub fn stored_bridged_result( + bridge_url: Option<&str>, + child_call_id: &str, + echoed: &serde_json::Value, + echoed_is_error: bool, +) -> Option { + let recorded = super::bridge::take_recorded_result(bridge_url?, child_call_id)?; + recorded_if_received(recorded, &echoed_texts(echoed), echoed_is_error) +} + +#[cfg(test)] +mod recorded_result_tests { + use super::*; + use rmcp::model::{CallToolResult, Content, Role}; + use serde_json::json; + + const DATE: &str = "Thu Sep 11 01:00:00 PDT 2026"; + + /// The developer shell's result shape (`rmcp_developer.rs`): the output for + /// the assistant, and a copy for the user marked low priority. + fn shell_shaped(output: &str) -> CallToolResult { + CallToolResult::success(vec![ + Content::text(output).with_audience(vec![Role::Assistant]), + Content::text(output) + .with_audience(vec![Role::User]) + .with_priority(0.0), + ]) + } + + /// Blocks the card shows: no audience, or one naming the user. + fn user_visible(content: &[Content]) -> usize { + content + .iter() + .filter(|c| c.audience().is_none_or(|a| a.contains(&Role::User))) + .count() + } + + fn stored_result(message: &Message) -> &CallToolResult { + let MessageContent::ToolResponse(response) = &message.content[0] else { + panic!("expected a tool response"); + }; + response + .tool_result + .as_ref() + .expect("a successful transport") + } + + /// QA-E F4, the case the finding names: `developer__shell {"command":"date"}` + /// over Claude Code stored two identical, unlabelled blocks — "2 results" on + /// the card, and the output twice in the child's next prompt. The same + /// two-block annotated result must mirror to ONE model-facing block (and one + /// user-facing block, so the card reads "1 result"), audiences preserved. + #[test] + fn a_two_block_annotated_result_mirrors_to_one_model_facing_block_with_audience_preserved() { + let recorded = shell_shaped(DATE); + // Claude Code's echo in the QA run: both blocks, annotations gone. + let echo = json!([{"type": "text", "text": DATE}, {"type": "text", "text": DATE}]); + + let stored = recorded_if_received(recorded.clone(), &echoed_texts(&echo), false) + .expect("the child demonstrably received the result"); + let message = response_message_with_result("toolu_1", stored, Execution::Bridged); + let content = &stored_result(&message).content; + + let model_facing: Vec<&Content> = content + .iter() + .filter(|c| audience::is_for_model(c)) + .collect(); + assert_eq!( + model_facing.len(), + 1, + "one block for the model: {content:?}" + ); + assert_eq!(model_facing[0].audience(), Some(&vec![Role::Assistant])); + assert_eq!(user_visible(content), 1, "the card must read \"1 result\""); + assert_eq!( + stored_result(&message), + &recorded, + "stored exactly as the tool returned it" + ); + } + + /// After the fix the child is handed only the model's block, and echoes one. + #[test] + fn the_echo_of_the_model_view_adopts_the_full_result() { + let echo = json!([{"type": "text", "text": DATE}]); + let recorded = shell_shaped(DATE); + assert_eq!( + recorded_if_received(recorded.clone(), &echoed_texts(&echo), false), + Some(recorded) + ); + } + + /// A child whose call timed out (#110) never saw Biorouter's result, and + /// the transcript records what it did see. + #[test] + fn a_child_that_timed_out_keeps_its_own_echo() { + let echo = json!("The operation timed out"); + assert!(recorded_if_received(shell_shaped(DATE), &echoed_texts(&echo), true).is_none()); + } + + /// Same when the flags disagree the other way round, or the CLI truncated + /// a large result before the child read it. + #[test] + fn an_echo_that_is_not_what_biorouter_sent_keeps_the_echo() { + let echo = json!([{"type": "text", "text": DATE}]); + assert!( + recorded_if_received(shell_shaped(DATE), &echoed_texts(&echo), true).is_none(), + "the child says its call failed" + ); + + let long = "x".repeat(1_000); + let truncated = json!([{"type": "text", "text": "x".repeat(200)}]); + assert!( + recorded_if_received(shell_shaped(&long), &echoed_texts(&truncated), false).is_none(), + "the child saw only part of the output" + ); + } + + /// Claude Code rewrites an embedded text resource as prose with a prefix + /// (measured, 2.1.266); the file's text is still what the child received. + #[test] + fn a_resource_echoed_as_prefixed_text_still_counts_as_received() { + let recorded = CallToolResult::success(audience::text_editor_view_result()); + let echo = json!([{ + "type": "text", + "text": format!("[Resource from biorouter at str:///notes.rs] {}", audience::VIEW_FOR_MODEL) + }]); + assert_eq!( + recorded_if_received(recorded.clone(), &echoed_texts(&echo), false), + Some(recorded) + ); + } + + #[test] + fn echoed_texts_reads_every_vendor_shape() { + assert_eq!(echoed_texts(&json!("plain")), vec!["plain"]); + assert_eq!( + echoed_texts(&json!([ + {"type": "text", "text": "a"}, + {"type": "image", "source": {"type": "base64", "data": "AA=="}}, + {"type": "resource", "resource": {"uri": "str:///f", "text": "r"}} + ])), + vec!["a", "r"] + ); + assert_eq!( + echoed_texts(&json!({"content": [{"type": "text", "text": "c"}], "isError": false})), + vec!["c"] + ); + assert!(echoed_texts(&serde_json::Value::Null).is_empty()); + } + + /// No bridge, or nothing recorded for the call: the caller keeps the echo. + #[test] + fn with_nothing_recorded_the_echo_is_kept() { + assert!(stored_bridged_result(None, "toolu_1", &json!("x"), false).is_none()); + assert!(stored_bridged_result( + Some("http://127.0.0.1:1/tool_bridge/00000000000000000000000000000000"), + "toolu_1", + &json!("x"), + false + ) + .is_none()); + } +} + #[cfg(test)] mod content_tests { use super::*; diff --git a/crates/biorouter/src/providers/coding_agent/transcript.rs b/crates/biorouter/src/providers/coding_agent/transcript.rs index 14f49ad48..e5d0a0206 100644 --- a/crates/biorouter/src/providers/coding_agent/transcript.rs +++ b/crates/biorouter/src/providers/coding_agent/transcript.rs @@ -23,6 +23,7 @@ //! a stable prefix, so the marginal cost is far below the naive reading. use crate::conversation::message::{Message, MessageContent}; +use crate::providers::formats::audience; use rmcp::model::Role; #[derive(Clone, Debug, PartialEq, Eq)] @@ -85,10 +86,17 @@ fn render_content(content: &MessageContent) -> Option { } MessageContent::ToolResponse(r) => Some(match &r.tool_result { Ok(result) => { + // What a model is sent and nothing else: the filter every provider + // formatter applies, reading text resources as well as text. A + // block a tool addressed only to the user — the shell's reformatted + // copy, a `ui://` figure — is not the model's, and passing both + // copies of an annotated result is how the child came to re-read + // every tool's output twice (QA-E F4). let body = result .content .iter() - .filter_map(|c| c.as_text().map(|t| t.text.as_str())) + .filter(|c| audience::is_for_model(c)) + .filter_map(audience::flattened_text) .collect::>() .join("\n"); format!( @@ -306,6 +314,44 @@ mod tests { ); } + /// QA-E F4: a result a tool addressed to two readers is flattened as a model + /// is sent it — the assistant's block once — not with the user's copy as + /// well. Both copies used to go in, so the child re-read every shell result + /// twice on every later turn. + #[test] + fn an_annotated_tool_result_reaches_the_prompt_once() { + use rmcp::model::{CallToolResult, Content}; + let msg = Message::user().with_tool_response( + "call-1", + Ok(CallToolResult::success(vec![ + Content::text("MODEL-COPY").with_audience(vec![Role::Assistant]), + Content::text("USER-COPY") + .with_audience(vec![Role::User]) + .with_priority(0.0), + ])), + ); + let out = flatten(&[user("first"), msg, user("now answer")]).unwrap(); + assert_eq!(out.matches("MODEL-COPY").count(), 1, "{out}"); + assert!(!out.contains("USER-COPY"), "{out}"); + } + + /// `text_editor view` hands the model the file as an embedded resource and + /// the user a rendering. Filtering by audience must not leave the child with + /// the rendering, or with nothing: it reads the file, as every formatter does. + #[test] + fn a_file_view_reaches_the_prompt_as_the_file_not_its_rendering() { + use crate::providers::formats::audience; + let msg = Message::user().with_tool_response( + "call-1", + Ok(rmcp::model::CallToolResult::success( + audience::text_editor_view_result(), + )), + ); + let out = flatten(&[user("first"), msg, user("now answer")]).unwrap(); + assert!(out.contains(audience::VIEW_FOR_MODEL), "{out}"); + assert!(!out.contains(audience::VIEW_FOR_USER), "{out}"); + } + /// Thinking blocks are dropped: they are the *previous* model's private /// reasoning, they are often signed, and replaying them as text into a /// different vendor's agent is noise at best. diff --git a/docs/providers/coding-agents/README.md b/docs/providers/coding-agents/README.md index bcfbc9bdd..de7b90758 100644 --- a/docs/providers/coding-agents/README.md +++ b/docs/providers/coding-agents/README.md @@ -43,6 +43,28 @@ without a confirmation step. That combination — a coding agent, full Developer and Auto mode — is the one to think about before pointing these providers at a machine holding credentials you care about. +⚠ **On Codex, none of that reached the child from `codex-cli` 0.148.0 until +2026-09-11** (QA-E F1, measured on 0.153.4 with `claude` 2.1.266 alongside). +Every bridged call failed on its first attempt with the vendor's own sentence, +`MCP tool call requires approval, but approval policy is never`: the CLI began +answering its MCP tool-call approval itself under the `never` policy BioRouter +passed, so BioRouter was never asked. The thread now runs under Codex's granular +policy, which still refuses every child-local category inside the CLI and lets +only that one approval through, and BioRouter accepts it for its own bridge +alone — see +[why the policy is not `never`](child-agent-isolation.md#why-the-policy-is-not-never-qa-e-f1). + +⚠ **Fixing the policy alone would not have brought the Codex shell back, and +both children read every tool result twice** (QA-E F4, same date and CLI +versions). The bridge handed the child every content block a tool returned, +including the copy a tool addresses only to the user: neither CLI filters by +audience, so the child's model read each shell result twice, and codex-cli +cannot parse the `priority` annotation on the shell's user copy, so every +`developer__shell` and `text_editor view` call failed there with `Unexpected +response type`. The child is now handed only what a model is sent, unannotated, +and the transcript stores the full result the bridge kept — see +[what the child is handed](tool-bridge.md#what-the-child-is-handed-and-what-the-transcript-keeps-qa-e-f4). + ## Documents | Document | What it covers | diff --git a/docs/providers/coding-agents/child-agent-isolation.md b/docs/providers/coding-agents/child-agent-isolation.md index 711a0490d..8b53f96c7 100644 --- a/docs/providers/coding-agents/child-agent-isolation.md +++ b/docs/providers/coding-agents/child-agent-isolation.md @@ -138,7 +138,7 @@ defaults, and each is pinned by a test. | Parameter | Value | Why | | --- | --- | --- | | `sandbox` | `"read-only"` | The child cannot change anything on the machine. | -| `approvalPolicy` | `"never"` | It must not try to negotiate its way out; approvals are answered by BioRouter's own policy, not by prompting. | +| `approvalPolicy` | `{"granular": {…}}` — `mcp_elicitations: true`, and `sandbox_approval`, `rules`, `skill_approval`, `request_permissions` all `false` | Codex's own per-category form of `never`. A `false` category is rejected inside the CLI with no round trip, exactly as `never` rejects it, so the child still cannot negotiate its way out. The one category let through is the channel on which Codex asks whether it may call a **BioRouter** tool — and `"never"` itself stopped working for that on codex-cli 0.148.0; see [why the policy is not `never`](#why-the-policy-is-not-never-qa-e-f1). | | `ephemeral` | `true` | No Codex session files. BioRouter owns the transcript, for the same reason as `--no-session-persistence` above. | | `baseInstructions` | BioRouter's system prompt | Replaces Codex's own preamble, which measured ~15k input tokens on a trivial prompt. | | `config.mcp_servers.biorouter.url` | The bridge URL, when the turn has one | The streamable-HTTP MCP form, which needs no second process. | @@ -149,22 +149,78 @@ defaults, and each is pinned by a test. ### Every child-local approval request is refused `codex app-server` routes requests back to the host as server-originated messages that block the -turn. BioRouter accepts only the MCP elicitation used by its own gated bridge and refuses every -child-local command, file, patch, or permission escalation in one small decision function: +turn. BioRouter accepts exactly one kind — Codex asking whether it may call a tool on BioRouter's own +gated bridge — and refuses every child-local command, file, patch, or permission escalation in one +small decision function, `CodexProvider::decide`: | Server request | Answer | | --- | --- | -| `mcpServer/elicitation/request` | **Accept.** This is how an MCP tool call BioRouter is itself serving gets its go-ahead, and those run in BioRouter's dispatcher behind BioRouter's gates. | -| `item/commandExecution/requestApproval` | Denied | -| `item/fileChange/requestApproval` | Denied | -| `item/permissions/requestApproval` | Denied | -| `applyPatchApproval`, `execCommandApproval` | Denied | +| `mcpServer/elicitation/request` marked `_meta.codex_approval_kind: "mcp_tool_call"`, `serverName: "biorouter"` | **Accept, once** — `{"action":"accept","content":{}}` with no `persist` choice, which Codex reads as a one-time `Approved`. The call then runs in BioRouter's dispatcher behind BioRouter's inspectors, permission mode, `.biorouterignore`, vault and privacy Gate C, so Codex's own ask adds nothing BioRouter would refuse. | +| The same approval for any **other** server | Declined. The child's isolated `CODEX_HOME` holds no other MCP server, so one appearing is an isolation regression. | +| Any other elicitation — an MCP server's form, a URL flow, another Codex approval kind | Declined. There is nobody on this side to fill a form in, and an empty accept would submit one nobody saw. | +| `item/tool/requestUserInput` | Refused in its own shape, `{"answers": {}}`. This is where Codex sends the tool-call approval when its `tool_call_mcp_elicitation` feature is off; its parameters name no server, so it cannot be scoped to the bridge and is not accepted. | +| `item/commandExecution/requestApproval` | Denied (`decline`) | +| `item/fileChange/requestApproval` | Denied (`decline`) | +| `item/permissions/requestApproval` | Denied (an empty grant) | +| `applyPatchApproval`, `execCommandApproval` | Denied (`{"denied": {…}}`) | | Anything unrecognised | **Denied.** An unanswered request stalls the turn forever, so refusing beats guessing. | The child is configured with a read-only sandbox and no tools of its own, so a command or file-change approval request means it is reaching for authority it was not given. The honest answer is no. +### Why the policy is not `never` (QA-E F1) + +Until 2026-09-11 the thread ran under `approvalPolicy: "never"`, and on a current Codex that made +**every** bridged tool fail — deterministically, on the first call — with a sentence that comes from +the vendor binary, not from BioRouter: + +```text +MCP tool call requires approval, but approval policy is never +``` + +The mechanism, read from the codex-cli source at the matching tags and then measured against a live +`codex app-server` 0.153.4: + +- An MCP tool call asks for approval unless its server pre-approved it, or its annotations say + `readOnlyHint: true`. An unannotated tool counts as destructive, and most of BioRouter's are + unannotated. +- `never` auto-approves that ask **only when the sandbox has full disk write**. BioRouter's child is + read-only, so the ask is real. +- From **0.148.0**, an ask under `never` is answered inside the CLI with the refusal above, before + any request is sent. 0.147.0 has no such check; it sent the ask to the host, where the elicitation + arm accepted it, which is why the bridge worked when it was built. + +So no answer BioRouter gave could have helped: under `never` the question never arrived. The fix is +the granular policy in the table above, and the question then arrives in this exact shape (captured +from 0.153.4, ids shortened): + +```json +{"method": "mcpServer/elicitation/request", "id": 0, "params": { + "threadId": "01a08f7e-…", "turnId": "01a08f7e-…", "serverName": "biorouter", "mode": "form", + "_meta": {"codex_approval_kind": "mcp_tool_call", "persist": ["session", "always"], + "tool_params": {"text": "hi"}, "tool_params_display": [ … ]}, + "message": "Allow the biorouter MCP server to run tool \"echo\"?", + "requestedSchema": {"type": "object", "properties": {}}}} +``` + +⚠ **Two couplings to know before touching this.** + +- `granular` is `#[experimental("askForApproval.granular")]` in the app-server protocol, so it only + parses for a client that declared `capabilities.experimentalApi: true` at `initialize`. BioRouter + does; `initialize_declares_the_experimental_api_the_policy_needs` pins it. The variant is + identical in the 0.147.0 and 0.153.4 protocol, so the change does not strand an older CLI. +- The tool-call approval arrives as an elicitation only while Codex's `tool_call_mcp_elicitation` + feature is on — stable and on by default, and not in `DISABLED_CHILD_FEATURES`. Disabling it would + route the approval to `item/tool/requestUserInput`, which is refused, and every bridged call would + fail again as `user cancelled MCP tool call`. + +Why no test caught it: the recorded Codex fixtures under `tests/fixtures/coding_agent/codex/` were +captured with `sandbox: dangerFullAccess`, the one configuration in which `never` still auto-approves +an MCP call, and none of them contains an MCP tool call at all. The live bridge test that drives the +real Codex asserted only that the answer *named* the tool — which a refusal also does. It now +requires output only a tool that really ran could produce. + ## What the child still has Isolation is not a sandbox, and this section is the honest statement of the boundary. diff --git a/docs/providers/coding-agents/how-it-works.md b/docs/providers/coding-agents/how-it-works.md index 8e93cfaf0..31237fdf2 100644 --- a/docs/providers/coding-agents/how-it-works.md +++ b/docs/providers/coding-agents/how-it-works.md @@ -194,7 +194,11 @@ wants it to have. `codex app-server` speaks newline-delimited JSON-RPC 2.0 over stdio and routes every approval back to the host as a **server-originated request** that blocks the turn until it is answered. -That is the shape BioRouter needs, because the decision stays here. The transport is therefore +That is the shape BioRouter needs, because the decision stays here — ⚠ **provided the thread's +approval policy lets the request out at all.** Under `never`, codex-cli 0.148.0 and newer answer an +MCP tool-call approval inside the CLI with a refusal, so BioRouter's bridge was unreachable until +the policy changed; see +[why the policy is not `never`](child-agent-isolation.md#why-the-policy-is-not-never-qa-e-f1). The transport is therefore genuinely bidirectional: a client that only reads responses deadlocks the first time the agent wants to do anything. Inbound messages are classified exactly as the protocol defines them — an `id` with no `method` is a response, a `method` with an `id` is a request that must be answered, diff --git a/docs/providers/coding-agents/tool-bridge.md b/docs/providers/coding-agents/tool-bridge.md index 2cd5f0fd3..02cb5bfbf 100644 --- a/docs/providers/coding-agents/tool-bridge.md +++ b/docs/providers/coding-agents/tool-bridge.md @@ -163,8 +163,11 @@ staging directory remained. Spoke stayed installed. The chat retained its inert is not a zero-residue purge of prior preferences or records. Codex's standard MCP result object is decoded as a complete `CallToolResult`, not serialized -into a text block containing another result. This preserves content types, audience annotations, -structured data, error state and display metadata. To Do update results carry the updated task's +into a text block containing another result. This preserves content types, structured data, error +state and display metadata. ⚠ Since QA-E F4 it is the *fallback*: the child is handed only the +model's view of a result, so where the bridge kept its own full record of the call, that record — +audience annotations included — is what gets stored; see +[what the child is handed](#what-the-child-is-handed-and-what-the-transcript-keeps-qa-e-f4). To Do update results carry the updated task's id, text and status so the activity row can name the work rather than only its numeric id. The subsequent live read-only checklist showed “Starting ‘Confirm the primary knowledge base’” and “Marking ‘Examine its page index read-only’ complete”, alongside “Listing pages in Soul”. @@ -194,7 +197,12 @@ which opens Electron's welcome window rather than BioRouter. Verified against a 60-tool surface — both CLIs accepted a 73-character prefixed tool name, a schema using `$defs`/`$ref`/`oneOf`, an image result, and a `ui://` embedded resource, all passed through -unchanged. +unchanged. ⚠ Two things that probe did not exercise have since been measured, and both changed what +the bridge returns (QA-E F4, 2026-09-11): neither CLI filters a result by audience, and codex-cli +0.153.4 cannot parse a block carrying `priority`. A block addressed only to the user — a `ui://` +figure, the shell's reformatted copy — is therefore no longer handed to the child at all, and the +annotations are stripped from what is; the transcript keeps the full result. See +[what the child is handed](#what-the-child-is-handed-and-what-the-transcript-keeps-qa-e-f4). ## What still fires on a bridged call @@ -429,6 +437,51 @@ cosmetic. ⚠ **The GUI does not yet draw a label separating the two** — the m persisted metadata, but a card reading `exec` looks like any other card today. Until that label lands, read `exec` and `apply_patch` cards on a Codex turn as child-executed. +### What the child is handed, and what the transcript keeps (QA-E F4) + +A BioRouter tool can return the same output twice, addressed to two readers: `developer__shell` +returns one text block with `audience: ["assistant"]` and a second, reformatted one with +`audience: ["user"]` and `priority: 0.0`; `text_editor view` returns the file to the assistant as an +embedded resource and a numbered rendering to the user; an Auto Visualiser figure is a `ui://` +resource for the user beside a one-line label for the assistant. Every provider formatter sends the +model only the blocks addressed to it (`providers::formats::audience::is_for_model`), and the GUI +shows only the ones addressed to the user. + +The bridge used to hand the child **every** block, and three things followed, all measured on +2026-09-11 against `claude` 2.1.266 and `codex-cli` 0.153.4: + +| What | Consequence | +| --- | --- | +| Neither CLI filters by audience | The child's model read every shell result twice, in the live turn. | +| Claude Code drops every annotation when it echoes a result (`tool_result.content` and `tool_use_result` alike), and rewrites an embedded resource as `[Resource from biorouter at ] ` | The mirror stored two unlabelled blocks, so the card read "2 results" and the next turn's transcript repeated the output again. | +| codex-cli cannot parse a result whose block carries `priority` — `priority: 0.0` alone fails the call with `Unexpected response type`; `audience` alone does not | Once F1 was fixed, every `developer__shell` and `text_editor view` call on Codex still failed. | + +So the bridge now answers the child with `bridge::child_view` of the result: the model-facing blocks +only, with their annotations removed — after the filter they tell a model nothing, and removing them +is what makes the result parse on Codex. The bridge is, in effect, these two providers' formatter. + +The full result is not thrown away. `BridgeGrant::call_for_child` keeps it, keyed by the child's own +id for the call, which both CLIs put in the `tools/call` request's `_meta` and repeat on the frame that +reports the result: + +| CLI | `_meta` key | Same id as | +| --- | --- | --- | +| Claude Code 2.1.266 | `claudecode/toolUseId` | the stream's `tool_use` / `tool_result` id | +| codex-cli 0.153.4 | `callId` | the `mcpToolCall` item id | + +When the provider mirrors the call, it takes that record (`bridge::take_recorded_result`) and stores +**it** rather than the child's lossy echo — so the transcript holds exactly what the tool returned, +annotations included, the card counts one user-facing result, and the next turn's prompt carries one +model-facing copy. The transcript flattener applies the same `is_for_model` filter the formatters do, +reading text resources as well as text, so an annotated result is flattened once. + +⚠ **The record is stored only when the echo shows the child received it** +(`mirror::recorded_if_received`): the error flag must agree, and every text BioRouter sent must +appear in what the child echoed. A child whose call timed out (#110), or whose CLI truncated a large +result, worked from something else, and the transcript records what the child actually saw. A CLI +that stops sending its call id degrades the same way — to the echo of the model-facing view — rather +than failing. + ### The mirror and the approval card are different things The mirror draws what *happened*: a `ToolRequest`/`ToolResponse` pair, green or red, after the fact. @@ -573,11 +626,11 @@ outlasts stream consumption. | Concern | File | | --- | --- | -| Grants, leases, the nonce, the task-locals, the transport budget | [`crates/biorouter/src/providers/coding_agent/bridge.rs`](../../../crates/biorouter/src/providers/coding_agent/bridge.rs) | +| Grants, leases, the nonce, the task-locals, the transport budget; the child's view of a result (`child_view`) and the full result kept for the transcript (`call_for_child`, `take_recorded_result`) | [`crates/biorouter/src/providers/coding_agent/bridge.rs`](../../../crates/biorouter/src/providers/coding_agent/bridge.rs) | | Running one provider turn with Biorouter tools, from anywhere | [`crates/biorouter/src/providers/tool_turn.rs`](../../../crates/biorouter/src/providers/tool_turn.rs) | | Parking a call on a person: approval, elicitation, secret-safe credentials | [`crates/biorouter/src/pending_user_action.rs`](../../../crates/biorouter/src/pending_user_action.rs) | | The queue the card is published on, and the loop's wake seam | [`crates/biorouter/src/action_required_manager.rs`](../../../crates/biorouter/src/action_required_manager.rs) | -| The mirror marker, and the request/response pair builders | [`crates/biorouter/src/providers/coding_agent/mirror.rs`](../../../crates/biorouter/src/providers/coding_agent/mirror.rs) | +| The mirror marker, the request/response pair builders, and when the kept result is stored instead of the child's echo (`recorded_if_received`) | [`crates/biorouter/src/providers/coding_agent/mirror.rs`](../../../crates/biorouter/src/providers/coding_agent/mirror.rs) | | The loop branch that persists a mirrored pair without dispatching it | [`crates/biorouter/src/agents/agent.rs`](../../../crates/biorouter/src/agents/agent.rs) | | The HTTP/JSON-RPC endpoint | [`crates/biorouter-server/src/routes/tool_bridge.rs`](../../../crates/biorouter-server/src/routes/tool_bridge.rs) | | Handing the URL to Claude Code | [`crates/biorouter/src/providers/claude_code.rs`](../../../crates/biorouter/src/providers/claude_code.rs) |