diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index b5ee1731..80152f2d 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -7504,6 +7504,9 @@ impl Agent { self.config.biorouter_mode, session.clone(), Arc::clone(&self.hooks_manager), + // D10: the store the judge walks to find the conversation a + // person is watching, when this session is a delegated child. + Some(Arc::clone(&self.config.session_manager)), )) }); diff --git a/crates/biorouter/src/agents/approval_relay.rs b/crates/biorouter/src/agents/approval_relay.rs index 77bd03e4..32195c19 100644 --- a/crates/biorouter/src/agents/approval_relay.rs +++ b/crates/biorouter/src/agents/approval_relay.rs @@ -585,6 +585,46 @@ pub async fn begin_delegated_approval( Delegation::AwaitingHuman { surfaced_in: root } } +/// **D10.** Show a card raised inside an agent-created conversation to the person +/// watching the root of its tree as well, and accept the answer from there. +/// +/// This is the [`crate::pending_user_action`] counterpart of the +/// [`Delegation::AwaitingHuman`] half of [`begin_delegated_approval`], and +/// deliberately *only* that half. Same destination (the root of the delegation +/// tree — every layer between is itself an agent, and the layer with no layer +/// above it is where a person is), same "one ask, two surfaces, one request id" +/// shape, and the same reason: a card that lives only in a conversation nobody is +/// looking at is an unexplained stall. +/// +/// **No ancestor agent is consulted.** [`DelegationPolicy`] is not reached from +/// here, so decisions 30 and 31 keep their single home and this path cannot +/// produce a permission at all — it widens who may *see and click*, never who +/// may decide. Proof of user is equally untouched: +/// `PendingUserActions::resolve_matching` gates an allow on a proof-backed +/// approval by the authority of the answering request, not by the session it came +/// from. +/// +/// Returns the session the card was also surfaced in, for the caller's log: +/// `None` for a root conversation (which is already where the person is), for a +/// chain that reads back empty, and for a card that is no longer parked. +/// +/// ⚠ Callers: this is for a card whose decision **must** reach a person — +/// everything [`crate::pending_user_action::PendingUserActions::park`] raises +/// inside a delegated child. Do not reach for it from a door that has an +/// ancestor-agent path available; that is [`begin_delegated_approval`]. +pub(crate) async fn surface_where_a_person_is_watching( + session_manager: &SessionManager, + session: &Session, + parked: &crate::pending_user_action::PendingUserAction, +) -> Option { + // A root conversation IS where the person is; surfacing into it would show + // that chat the same card twice. + session.parent_session_id.as_ref()?; + let chain = ancestor_chain(session_manager, session).await; + let root = chain.last()?; + parked.also_surface_in(root).then(|| root.clone()) +} + /// Hand a decision to the prompt that is parked on it. Reuses BR-62's routing /// wholesale: `handle_confirmation` finds the `oneshot` `register_confirmation` /// created and `await_confirmation` is already waiting on it, so the turn loop diff --git a/crates/biorouter/src/agents/script_call_gate.rs b/crates/biorouter/src/agents/script_call_gate.rs index 9f1688ad..ad6a4993 100644 --- a/crates/biorouter/src/agents/script_call_gate.rs +++ b/crates/biorouter/src/agents/script_call_gate.rs @@ -53,6 +53,25 @@ //! 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. +//! +//! ⚠ That is about *who decides*, and it says nothing about *where the +//! question appears* — which is where D10 went wrong. A card parks on the +//! session that raised it, so a script's ask inside a **sub-agent** was +//! published to the child's own conversation and nowhere else: a person +//! watching the chat that delegated the work saw the `subagent` tool call stop +//! with no card and no explanation, and a child with no tab at all +//! (`visible: false`, a fan-out past the four-tab cap, a terminal session) had +//! no surface anywhere. The same call made *directly* by the child's model +//! already surfaces in the root chat — `approval_relay` publishes it there +//! when it reports `AwaitingHuman` — so in the shipped Code Execution default, +//! where nearly every call is a script's, the escalation was missing from the +//! path that carries almost all the traffic. +//! [`approval_relay::surface_where_a_person_is_watching`] closes that: the +//! same card, in the root of the delegation tree, answerable from either +//! surface, with no ancestor **agent** consulted and proof of user untouched. +//! +//! [`approval_relay::surface_where_a_person_is_watching`]: +//! crate::agents::approval_relay::surface_where_a_person_is_watching //! * **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 @@ -112,7 +131,8 @@ 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, + PendingUserAction, PendingUserActions, ToolApprovalRequest, UserActionOutcome, + UserActionRequest, }; use crate::permission::tool_risk::ToolRiskRegistry; use crate::permission::Permission; @@ -312,8 +332,18 @@ pub struct ScriptCallGate { /// 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. + /// path arguments, and the home of every ask this judge raises. session: Session, + /// The store the delegation chain above [`Self::session`] is walked in, so a + /// card raised inside a sub-agent can also be shown where a person is + /// watching (D10). + /// + /// `None` only where there is no store to walk — a hand-built test gate. A + /// missing store degrades to today's behaviour (the child's own tab, and + /// nothing else) and says so in a log, rather than failing the call: a + /// script's ask that cannot be *escalated* is still an ask a person may + /// answer in the child's tab. + sessions: Option>, /// For the PreToolUse rewrites this gate's own inspection staged, and the /// PermissionRequest hooks consulted before a card. hooks: Arc, @@ -334,12 +364,14 @@ impl ScriptCallGate { mode: BioRouterMode, session: Session, hooks: Arc, + sessions: Option>, ) -> Self { Self { inspections, mode, session, hooks, + sessions, parking_permit: Mutex::new(None), } } @@ -556,6 +588,11 @@ impl ScriptCallGate { requires_user_proof: false, }); let parked = PendingUserActions::global().park(Some(&self.session.id), None, request); + // D10: a card raised inside a delegated child is also shown to the person + // watching the conversation that delegated the work. Before the wait, so + // a user already looking at that chat cannot answer a card that has not + // been recorded as answerable there yet. + self.escalate_to_a_watching_conversation(&parked).await; // #246 review, finding 2. This wait is up to `approval_ttl()` long // (default 3600 s; `Duration::MAX` when `BIOROUTER_CONFIRMATION_TIMEOUT_SECS=0`), // and it happens INSIDE the `execute_code` tool body, which holds one of @@ -572,6 +609,49 @@ impl ScriptCallGate { self.verdict_for_answer(call, outcome, cancel).await } + /// **D10.** Show a delegated child's card in the conversation a person is + /// actually watching, as well as in the child's own tab. + /// + /// The policy — which conversation, and why the root rather than the + /// immediate parent — lives in + /// [`crate::agents::approval_relay::surface_where_a_person_is_watching`], + /// beside the direct-call escalation it mirrors, so the two cannot pick + /// different destinations. All this adds is the store to walk and the log + /// line. + /// + /// Failure is silent by design: no ancestor, or no store to walk, leaves the + /// ask exactly where it was before this existed — in the child's own tab, + /// which a person may still answer. + async fn escalate_to_a_watching_conversation(&self, parked: &PendingUserAction) { + if self.session.parent_session_id.is_none() { + return; + } + let Some(sessions) = self.sessions.as_deref() else { + tracing::warn!( + session_id = %self.session.id, + "a script's approval card inside a delegated conversation could not be \ + escalated: this judge holds no session store, so only that conversation's \ + own tab can answer it" + ); + return; + }; + if let Some(watching) = crate::agents::approval_relay::surface_where_a_person_is_watching( + sessions, + &self.session, + parked, + ) + .await + { + tracing::debug!( + child_session_id = %self.session.id, + watching_session_id = %watching, + request_id = parked.id(), + "surfaced a delegated script's approval card in the conversation that \ + delegated the work" + ); + } + } + /// 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 @@ -825,12 +905,13 @@ pub(crate) mod test_support { inspections.add_inspector(Box::new(crate::hooks::HookInspector::new(Arc::clone( &hooks, )))); - let session = SessionManager::new(dir.join("sessions")) + let sessions = Arc::new(SessionManager::new(dir.join("sessions"))); + let session = sessions .create_session(dir.to_path_buf(), "gate".into(), SessionType::User) .await .expect("a session"); ( - ScriptCallGate::new(Arc::new(inspections), mode, session, hooks), + ScriptCallGate::new(Arc::new(inspections), mode, session, hooks, Some(sessions)), permissions, ) } @@ -972,7 +1053,18 @@ mod tests { code: &str, cancel: CancellationToken, ) -> tokio::task::JoinHandle<(bool, String)> { - let dispatched = dispatch_script(f, code, cancel).await; + run_script_in(f, &f.session, code, cancel).await + } + + /// As [`run_script`], but for a script the agent dispatches in `session` — + /// a delegated child, say — rather than in the fixture's own chat. + async fn run_script_in( + f: &Fixture, + session: &Session, + code: &str, + cancel: CancellationToken, + ) -> tokio::task::JoinHandle<(bool, String)> { + let dispatched = dispatch_script_in(f, session, code, cancel).await; tokio::spawn(async move { let result = dispatched .result @@ -986,6 +1078,15 @@ mod tests { f: &Fixture, code: &str, cancel: CancellationToken, + ) -> crate::agents::tool_execution::ToolCallResult { + dispatch_script_in(f, &f.session, code, cancel).await + } + + async fn dispatch_script_in( + f: &Fixture, + session: &Session, + code: &str, + cancel: CancellationToken, ) -> crate::agents::tool_execution::ToolCallResult { let call = CallToolRequestParams { task: None, @@ -995,7 +1096,7 @@ mod tests { }; let (_, dispatched) = f .agent - .dispatch_tool_call(call, "outer-execute-code".into(), Some(cancel), &f.session) + .dispatch_tool_call(call, "outer-execute-code".into(), Some(cancel), session) .await; dispatched.expect("execute_code dispatches") } @@ -1053,10 +1154,27 @@ mod tests { } async fn answer(f: &Fixture, card: &Card, permission: Permission) { - let outcome = f - .agent + let outcome = answer_from(f, &f.session.id, card, permission).await; + assert_eq!( + outcome, + crate::agents::ConfirmationOutcome::Delivered, + "the card's decision must reach the parked call" + ); + } + + /// Answer `card` the way `POST /action-required/tool-confirmation` does when + /// the click happened in `from_session_id` — the one thing that distinguishes + /// a decision made in the child's own tab from one made where the card was + /// escalated to. + async fn answer_from( + f: &Fixture, + from_session_id: &str, + card: &Card, + permission: Permission, + ) -> crate::agents::ConfirmationOutcome { + f.agent .handle_confirmation_for_session( - &f.session.id, + from_session_id, card.id.clone(), PermissionConfirmation { principal_type: PrincipalType::Tool, @@ -1064,12 +1182,90 @@ mod tests { }, DecisionAuthority::unproven(), ) - .await; + .await + } + + /// A delegated child of the fixture's own chat: a `SubAgent` row whose + /// `parent_session_id` names the conversation that spawned it, which is the + /// shape `create_subagent_session` writes. + /// + /// Read back from the store rather than mutated in place, because + /// `ScriptCallGate` snapshots the `Session` it is handed — a child whose + /// parent is only in the database is a child the gate cannot see. + async fn delegated_child(f: &Fixture) -> Session { + let sessions = &f.agent.config.session_manager; + let child = sessions + .create_session( + f.dir.path().to_path_buf(), + "delegated".into(), + SessionType::SubAgent, + ) + .await + .expect("a child session"); + sessions + .update(&child.id) + .parent_session_id(Some(f.session.id.clone())) + .apply() + .await + .expect("the child records its parent"); + ActionRequiredManager::global().drain_requests(&child.id); + let child = sessions + .get_session(&child.id, false) + .await + .expect("the child reads back"); assert_eq!( - outcome, - crate::agents::ConfirmationOutcome::Delivered, - "the card's decision must reach the parked call" + child.parent_session_id.as_deref(), + Some(f.session.id.as_str()), + "the fixture only discriminates if the child really is delegated" ); + child + } + + /// The next approval card to reach `watcher`, the bus feed `POST /reply` and + /// `GET /sessions/{id}/events` both drain — i.e. what a person watching that + /// conversation sees. + async fn card_on_bus( + watcher: &mut crate::session_events::Subscription, + within: Duration, + ) -> Option { + let deadline = tokio::time::Instant::now() + within; + loop { + let remaining = deadline.checked_duration_since(tokio::time::Instant::now())?; + let Ok(Ok(event)) = tokio::time::timeout(remaining, watcher.recv()).await else { + return None; + }; + let crate::session_events::SessionBusEvent::Agent(crate::agents::AgentEvent::Message( + message, + )) = event + else { + continue; + }; + for content in &message.content { + let MessageContent::ActionRequired(action) = content else { + continue; + }; + if let ActionRequiredData::ToolConfirmation { + id, + tool_name, + arguments, + prompt, + .. + } = &action.data + { + assert!( + !message.is_agent_visible(), + "an escalated card must stay out of the watching agent's context: \ + the decision is the person's, not the parent model's" + ); + return Some(Card { + id: id.clone(), + tool_name: tool_name.clone(), + arguments: arguments.clone(), + prompt: prompt.clone(), + }); + } + } + } } async fn finish(script: tokio::task::JoinHandle<(bool, String)>) -> (bool, String) { @@ -1779,6 +1975,221 @@ mod tests { ); } + /// **D10.** A card a script raises inside a DELEGATED child must reach the + /// conversation the person is actually watching. + /// + /// Measured in the running app: a script's ask inside a subagent published + /// to the child's session and nowhere else, so a user watching the parent + /// saw the `subagent` tool call sit there with no card, no card anywhere in + /// that chat, and no explanation — and a child running without a tab (a + /// `visible: false` spawn, a fan-out past the four-tab cap, a terminal + /// session) had no surface at all. The card then sat out its full + /// `approval_ttl()` — 3600 s by default — and the run was lost. + /// + /// The same call made DIRECTLY by the child's model already escalates: + /// `approval_relay::begin_delegated_approval` returns + /// `AwaitingHuman { surfaced_in: root }` and `handle_approval_tool_requests` + /// publishes the identical card into that session's bus. This asserts the + /// script path does the same, because in the shipped Code Execution default + /// the script path is how nearly every tool call is made. + /// + /// The bus is the right place to assert: `POST /reply` and + /// `GET /sessions/{id}/events` BOTH drain it (`routes/reply.rs` + + /// `routes/session_events.rs`), so a frame published there is what the + /// parent's tab renders whether the user is driving that chat or observing + /// it. + #[tokio::test] + #[serial_test::serial] + async fn a_subagents_script_ask_surfaces_where_the_person_watching_the_parent_is() { + let f = fixture(BioRouterMode::Approve).await; + let child = delegated_child(&f).await; + // Subscribed BEFORE the script runs: `session_events::publish` is a pure + // lookup that creates no ring, so a card published to a session nobody + // is watching is dropped — subscribing afterwards would measure the + // race, not the behaviour. + let mut watching_the_parent = crate::session_events::subscribe(&f.session.id); + + let mut script = run_script_in( + &f, + &child, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-DELEGATED" }));"#, + CancellationToken::new(), + ) + .await; + + let childs_card = card_or_completion(&child.id, &mut script) + .await + .unwrap_or_else(|(_, output)| panic!("the child's shell call must ask: {output}")); + assert_eq!(childs_card.tool_name, SHELL); + + let escalated = card_on_bus(&mut watching_the_parent, Duration::from_secs(30)) + .await + .expect( + "a script's approval card inside a subagent never reached the conversation the \ + person is watching: the parent's chat shows an unexplained stall while the \ + child parks for its whole time-to-live", + ); + assert_eq!( + escalated.id, childs_card.id, + "the escalated card must be the SAME ask — one decision, two surfaces — not a \ + second question with its own id" + ); + assert_eq!(escalated.tool_name, SHELL); + assert_eq!( + escalated.arguments.get("command").and_then(|v| v.as_str()), + Some("echo SCRIPT-GATE-DELEGATED"), + "the watching person needs the call's own arguments to decide" + ); + assert_eq!( + escalated.prompt, None, + "an ordinary script ask must not look like a security finding — the desktop \ + draws any prompt as a warning banner and withholds Always allow" + ); + + answer(&f, &childs_card, Permission::AllowOnce).await; + let (is_error, output) = finish(script).await; + assert!(!is_error, "the allowed call runs: {output}"); + assert!(output.contains("SCRIPT-GATE-DELEGATED"), "{output}"); + } + + /// The other half of D10, and the half that makes the card worth showing: a + /// person clicking Allow in the PARENT's chat resolves the child's parked + /// call. + /// + /// Publishing without this would be worse than the bug — a card the user can + /// see, click, and watch do nothing, because + /// `PendingUserActions::resolve_in_session` compares the posting session id + /// against the parked entry's and answers `Unknown` for anything else. + /// + /// Note what is NOT relaxed: the decision still comes from a person, through + /// the same `DecisionAuthority` the route samples from the request. Nothing + /// asks the parent AGENT, and `approval_relay`'s ancestor consultation is + /// not reached from here at all. + #[tokio::test] + #[serial_test::serial] + async fn the_person_watching_the_parent_can_answer_the_childs_card() { + let f = fixture(BioRouterMode::Approve).await; + let child = delegated_child(&f).await; + let mut watching_the_parent = crate::session_events::subscribe(&f.session.id); + + let mut script = run_script_in( + &f, + &child, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-ANSWERED-ABOVE" }));"#, + CancellationToken::new(), + ) + .await; + let card = card_or_completion(&child.id, &mut script) + .await + .unwrap_or_else(|(_, output)| panic!("the child's shell call must ask: {output}")); + // Drain the escalated copy so the assertion below is about answering it, + // not about whether it arrived — that is the test above. + card_on_bus(&mut watching_the_parent, Duration::from_secs(30)) + .await + .expect("the escalated card must reach the parent first"); + + let outcome = answer_from(&f, &f.session.id, &card, Permission::AllowOnce).await; + assert_eq!( + outcome, + crate::agents::ConfirmationOutcome::Delivered, + "Allow clicked in the watching conversation must release the child's parked \ + call; anything else leaves the user looking at a card that does nothing" + ); + + let (is_error, output) = finish(script).await; + assert!(!is_error, "the allowed call runs: {output}"); + assert!(output.contains("SCRIPT-GATE-ANSWERED-ABOVE"), "{output}"); + } + + /// A decision may come from the child's own tab or from where it was + /// escalated — and from nowhere else. The escalation widens the answering + /// scope by exactly one session, so an unrelated chat that happens to know + /// the request id still resolves nothing (#40's rule, unchanged). + #[tokio::test] + #[serial_test::serial] + async fn an_unrelated_conversation_still_cannot_answer_the_childs_card() { + let f = fixture(BioRouterMode::Approve).await; + let child = delegated_child(&f).await; + let bystander = f + .agent + .config + .session_manager + .create_session( + f.dir.path().to_path_buf(), + "bystander".into(), + SessionType::User, + ) + .await + .expect("a bystander session"); + + let mut script = run_script_in( + &f, + &child, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-BYSTANDER" }));"#, + CancellationToken::new(), + ) + .await; + let card = card_or_completion(&child.id, &mut script) + .await + .unwrap_or_else(|(_, output)| panic!("the child's shell call must ask: {output}")); + + let refused = answer_from(&f, &bystander.id, &card, Permission::AllowOnce).await; + assert_eq!( + refused, + crate::agents::ConfirmationOutcome::Unknown, + "a session that is neither the child nor an escalation surface must not be able \ + to grant the child's call" + ); + + answer_from(&f, &child.id, &card, Permission::DenyOnce).await; + let (_, output) = finish(script).await; + assert!( + !output.contains("SCRIPT-GATE-BYSTANDER"), + "the bystander's Allow must not have run the command: {output}" + ); + } + + /// A root conversation has nowhere to escalate to, and must not gain a + /// second card: the one the drain yields IS the person's. Guards against an + /// escalation that fires for every session and shows every ordinary chat its + /// own card twice. + #[tokio::test] + #[serial_test::serial] + async fn a_root_chats_script_ask_is_published_once() { + let f = fixture(BioRouterMode::Approve).await; + assert!( + f.session.parent_session_id.is_none(), + "the fixture's chat must be a root for this to measure anything" + ); + let mut watching = crate::session_events::subscribe(&f.session.id); + + let mut script = run_script( + &f, + r#"import { shell } from "developer"; + record_result(shell({ command: "echo SCRIPT-GATE-ROOT-ONCE" }));"#, + 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!( + card_on_bus(&mut watching, Duration::from_millis(750)) + .await + .is_none(), + "a root chat's own ask must not also be published to its bus as an escalation; \ + the drain already yields it into that chat's stream" + ); + + answer(&f, &card, Permission::AllowOnce).await; + let (is_error, output) = finish(script).await; + assert!(!is_error, "{output}"); + } + #[test] fn both_name_forms_of_execute_code_get_a_judge_and_nothing_else_does() { use crate::agents::code_execution_extension::is_execute_code_call; diff --git a/crates/biorouter/src/pending_user_action.rs b/crates/biorouter/src/pending_user_action.rs index 46c209b6..40d3aa4b 100644 --- a/crates/biorouter/src/pending_user_action.rs +++ b/crates/biorouter/src/pending_user_action.rs @@ -313,6 +313,45 @@ struct Entry { owner: Option, request: UserActionRequest, tx: Option>, + /// Other sessions this same card was published into, and from which a + /// decision is therefore accepted (D10). + /// + /// One entry today: the conversation that delegated the work, for a card + /// raised inside a sub-agent. `session_id` is still the card's **home** — + /// the session that owns the parked call and whose tab always shows it — + /// and this list only widens *who may answer*, never who may raise. + /// + /// ⚠ Recording and publishing happen in one operation + /// ([`PendingUserAction::also_surface_in`]) because each half alone is a bug + /// with its own symptom: a recorded surface the card was never published to + /// is an approval nobody can see, and a published card that was never + /// recorded is a card the user clicks and watches do nothing — + /// [`Self::answerable_in`] answers `Unknown` for every session but this one. + /// + /// What that pairing does NOT promise is an audience. `session_events::publish` + /// is best-effort by design: a session nobody is observing drops the frame. + /// So a surface recorded here may still show nothing — for a chat whose turn + /// has ended and which is therefore neither streaming `/reply` nor observing + /// its own bus. That is no worse than not escalating, and it grants nothing: + /// a decision still has to carry the request id, which only a rendered card + /// supplies. + escalated_to: Vec, +} + +impl Entry { + /// May a decision posted from `session_id` release this call? + /// + /// The card's home, plus any session it was escalated into. Deliberately + /// NOT "any session": #40's rule is that an authorization belongs to the + /// exact surfaces that showed it, and a card another conversation could + /// answer merely by knowing the id is a cross-session approval leak. + fn answerable_in(&self, session_id: &str) -> bool { + self.session_id.as_deref() == Some(session_id) + || self + .escalated_to + .iter() + .any(|surface| surface == session_id) + } } /// The process-global registry of parked user actions. @@ -426,6 +465,7 @@ impl PendingUserActions { owner: owner.map(str::to_string), request: request.clone(), tx: Some(tx), + escalated_to: Vec::new(), }, ); @@ -455,7 +495,7 @@ impl PendingUserActions { authority: DecisionAuthority, ) -> ResolveOutcome { self.resolve_matching(id, outcome, authority, |entry| { - entry.session_id.as_deref() == Some(session_id) + entry.answerable_in(session_id) }) } @@ -579,7 +619,7 @@ impl PendingUserActions { pub fn pending_cards_for_session(&self, session_id: &str) -> Vec { self.lock() .iter() - .filter(|(_, entry)| entry.session_id.as_deref() == Some(session_id)) + .filter(|(_, entry)| entry.answerable_in(session_id)) .map(|(id, entry)| request_message(id, &entry.request)) .filter(is_ephemeral_card) .collect() @@ -589,7 +629,7 @@ impl PendingUserActions { /// action. A foreign session learns nothing and cannot satisfy the check. pub fn requires_user_proof_in_session(&self, session_id: &str, id: &str) -> bool { self.lock().get(id).is_some_and(|entry| { - entry.session_id.as_deref() == Some(session_id) + entry.answerable_in(session_id) && matches!( &entry.request, UserActionRequest::ToolApproval(request) if request.requires_user_proof @@ -637,6 +677,30 @@ impl PendingUserActions { released } + /// Also accept a decision for `id` posted from `session_id`. + /// + /// `false` when nothing is parked on `id` (already answered, already gone) + /// or when `session_id` is the card's own home, so the caller can decline + /// to publish a second copy rather than showing a chat the same card twice. + /// Idempotent in the session: two calls add one surface. + fn record_escalation(&self, id: &str, session_id: &str) -> bool { + let mut entries = self.lock(); + let Some(entry) = entries.get_mut(id) else { + return false; + }; + if entry.session_id.as_deref() == Some(session_id) { + return false; + } + if !entry + .escalated_to + .iter() + .any(|surface| surface == session_id) + { + entry.escalated_to.push(session_id.to_string()); + } + true + } + /// Drop the entry for `id` without a decision. Idempotent. fn forget(&self, id: &str) { self.lock().remove(id); @@ -672,6 +736,56 @@ impl PendingUserAction { &self.request } + /// Show this same card in `session_id` as well, and accept the answer from + /// there (D10). + /// + /// The card a sub-agent raises is published to the sub-agent's own session + /// and nowhere else, so a person watching the conversation that *delegated* + /// the work sees a tool call that has silently stopped — and a child running + /// without a tab at all (`visible: false`, a fan-out past the four-tab cap, + /// a terminal session) has no surface anywhere. This is how a caller hands + /// the decision to the conversation a person is actually looking at. + /// + /// **What this does not do.** It does not ask the parent *agent* anything — + /// no [`crate::agents::approval_relay`] consultation, no model-produced + /// permission — and it does not touch proof of user: `resolve_matching` + /// still refuses an *allow* on a proof-backed approval from a surface that + /// cannot prove a person acted, whichever session it was posted from. The + /// only thing that widens is which conversation's card a **person** may + /// click. + /// + /// Published with [`request_message`], the same function `park` published + /// the original with, so the two surfaces carry a byte-identical card — and + /// therefore the same request id, which is what makes them one decision + /// rather than two questions. `user_only`, so the watching agent never reads + /// a question it must not answer. + /// + /// `false` when nothing was registered (`park` declined because no person + /// could be asked), when the call has already been answered, or when + /// `session_id` is this card's own home. Nothing is published in any of + /// those cases. + /// + /// `pub(crate)` on purpose. Widening where a card may be answered is a + /// decision about the delegation tree, so it belongs to the one function + /// that knows the tree — `approval_relay::surface_where_a_person_is_watching`, + /// which is itself crate-private. A door outside this crate that needs the + /// behaviour should go through that, not invent a second destination. + pub(crate) fn also_surface_in(&self, session_id: &str) -> bool { + if self.declined || self.rx.is_none() { + return false; + } + if !self.registry.record_escalation(&self.id, session_id) { + return false; + } + crate::session_events::publish( + session_id, + crate::session_events::SessionBusEvent::Agent(crate::agents::AgentEvent::Message( + request_message(&self.id, &self.request), + )), + ); + true + } + /// Park until a human answers, `ttl` elapses, or `cancel` trips. /// /// `cancel` is the **turn's** token, not one made here: every cancellation @@ -1418,6 +1532,126 @@ mod decision_authority_tests { } } + /// **D10, and the line that must not move.** A card escalated into the + /// conversation a person is watching widens who may SEE and CLICK it. It must + /// not widen what a click without proof may grant. + /// + /// The gate keys on the authority of the answering request, never on the + /// session it came from, so the escalation surface is refused exactly as the + /// card's own home is — and the caller stays parked for a surface that can + /// prove a person. + #[tokio::test] + async fn an_escalated_card_refuses_an_unproven_allow_exactly_as_its_home_does() { + let registry = Arc::new(PendingUserActions::default()); + let parked = registry.park(Some("child"), None, a_proof_backed_approval()); + let id = parked.id().to_string(); + assert!(parked.also_surface_in("parent")); + + assert_eq!( + registry.resolve_in_session("parent", &id, allow(), DecisionAuthority::unproven()), + ResolveOutcome::Unproven, + "escalating a card must not create a door that grants without proof" + ); + assert!(registry.is_pending(&id)); + assert_eq!( + registry.resolve_in_session( + "parent", + &id, + allow(), + DecisionAuthority::for_test_proven() + ), + ResolveOutcome::Delivered, + "a proven person at the escalation surface may still answer" + ); + drop(parked); + } + + /// The route asks `requires_user_proof_in_session` BEFORE it resolves, to + /// choose between a 403 that says "the user decides this" and one that says + /// "this control is unavailable here". Answered for the card's home only, an + /// escalated card would be reported as needing no proof, the route would skip + /// its check, and the person would get a bare `refused` from the gate below + /// with no sentence attached. + #[tokio::test] + async fn the_proof_requirement_is_reported_at_the_escalation_surface_too() { + let registry = Arc::new(PendingUserActions::default()); + let parked = registry.park(Some("child"), None, a_proof_backed_approval()); + let id = parked.id().to_string(); + assert!(parked.also_surface_in("parent")); + + assert!(registry.requires_user_proof_in_session("child", &id)); + assert!(registry.requires_user_proof_in_session("parent", &id)); + assert!( + !registry.requires_user_proof_in_session("bystander", &id), + "a session the card was never shown in must learn nothing about it" + ); + drop(parked); + } + + /// Exactly one session is added, and only a session that was asked for. An + /// escalation is not a licence for any conversation that knows the id (#40). + #[tokio::test] + async fn escalating_widens_the_answering_scope_by_exactly_one_session() { + let registry = Arc::new(PendingUserActions::default()); + let parked = registry.park(Some("child"), None, an_ordinary_approval()); + let id = parked.id().to_string(); + assert!(parked.also_surface_in("parent")); + + assert_eq!( + registry.resolve_in_session("bystander", &id, allow(), DecisionAuthority::unproven()), + ResolveOutcome::Unknown, + "a bystander conversation must not be able to grant this call" + ); + assert!(registry.is_pending(&id)); + assert_eq!( + registry.resolve_in_session("parent", &id, allow(), DecisionAuthority::unproven()), + ResolveOutcome::Delivered + ); + let _ = parked.wait(Duration::from_secs(5), None).await; + } + + /// Two calls add one surface, and a card is never escalated into its own + /// home — otherwise the chat that raised it renders the same ask twice. + #[tokio::test] + async fn escalation_is_idempotent_and_never_targets_the_cards_own_home() { + let registry = Arc::new(PendingUserActions::default()); + let parked = registry.park(Some("child"), None, an_ordinary_approval()); + + assert!( + !parked.also_surface_in("child"), + "the card's home already shows it" + ); + assert!(parked.also_surface_in("parent")); + assert!( + parked.also_surface_in("parent"), + "a second call is a no-op, not a second surface" + ); + assert_eq!( + registry.pending_cards_for_session("parent").len(), + 1, + "one ask, one card at that surface" + ); + drop(parked); + } + + /// A park that registered nothing — `park` declined because no person could + /// be asked — has nothing to escalate, and must publish nothing. Without the + /// guard an unattended run would broadcast a card for a call it is about to + /// refuse anyway. + #[tokio::test] + async fn a_park_nobody_could_answer_is_not_escalated() { + let registry = Arc::new(PendingUserActions::default()); + let parked = crate::user_surface::without_human_surface(async { + registry.park(Some("child"), None, an_ordinary_approval()) + }) + .await; + assert!(!parked.also_surface_in("parent")); + assert!( + registry.pending_cards_for_session("parent").is_empty(), + "a declined park must leave no card anywhere" + ); + } + #[tokio::test] async fn an_ordinary_approval_is_untouched_by_the_gate() { // ⚠ Catches a gate keyed on `ToolApproval(_)` rather than on the flag. diff --git a/docs/agent-loop/subagents.md b/docs/agent-loop/subagents.md index 849c5ae2..b3ac6fd8 100644 --- a/docs/agent-loop/subagents.md +++ b/docs/agent-loop/subagents.md @@ -60,6 +60,19 @@ If you typed into the tab, the parent is told. Its tool result carries `human_in The flag tracks **messages you sent**, so it is Steer that sets it, not Stop. Pressing Stop without typing cancels the run without marking the result as intervened. +### When the child needs your permission + +Even in Completely Autonomous mode some operations always ask — a recursive delete of a directory the session did not create, a write into a credential store, anything the security floor stops. A subagent running one of those has to reach **you**, and a card that lives only in a conversation you are not looking at is the same thing as no card at all. + +So the ask appears in **two** places: the child's own tab, and the conversation *you* started — the top of the chain, if the work was delegated more than once, because every layer below it is itself an agent. It is one decision with one identity: answer it in either place and the child continues. That matters most where the child has no tab to answer in — a `visible: false` spawn, a child past the four-tab fan-out cap, a fan-out you never opened. + +Two things it is not: + +- **The parent agent is not asked.** The card is shown to a person, in the chat a person is watching; the delegating model never sees it and cannot answer it. An approval a security inspector raised may only ever be answered by a person, at any depth. +- **It is not a record.** An approval card exists only while somebody has to answer it, so it is never written into the delegating conversation's history — answered or expired, it leaves nothing behind in a chat it was never part of. + +If nobody answers, nothing is quietly allowed: the call is refused, the child is told the approval expired, and it carries on without it. + ### Visible by default Children are **visible by default** whenever the desktop app is open. To run one silently, ask for it — the agent passes `visible: false` on the spawn — and the child runs exactly as subagents did before, reachable only from History and from the parent's summary.