From 6248dc0508d4c48bacdfebb36098736f0f1fd26f Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Fri, 8 May 2026 14:30:14 -0500 Subject: [PATCH] fix(security): preserve sub-agent approval boundaries --- crates/tui/src/tools/subagent/mod.rs | 52 ++++++++++++++++---------- crates/tui/src/tools/subagent/tests.rs | 36 ++++++++++++++++-- 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 3922309b..e2ef02d8 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -747,18 +747,17 @@ impl SubAgentRuntime { } /// Build a child runtime cloning this one, incrementing `spawn_depth`, - /// deriving a child cancellation token, and forcing `auto_approve` on - /// the child's `ToolContext`. Used at spawn entry to construct the - /// runtime the new sub-agent will see. + /// and deriving a child cancellation token. Used at spawn entry to + /// construct the runtime the new sub-agent will see. /// - /// The `auto_approve` override is deliberate: spawning IS the approval. - /// Per-tool prompts inside a child would break delegation, so children - /// inherit a YOLO-equivalent context regardless of the parent's mode. - /// The workspace boundary + sandbox profile still apply. + /// Children inherit the parent's approval state. A non-auto parent can + /// still delegate read-only investigation, but approval-gated child tools + /// are blocked by the sub-agent registry instead of being silently run + /// without a prompt. #[must_use] pub fn child_runtime(&self) -> Self { let mut child_context = self.context.clone(); - child_context.auto_approve = true; + child_context.auto_approve = self.context.auto_approve; Self { client: self.client.clone(), model: self.model.clone(), @@ -798,7 +797,8 @@ pub struct SubAgent { pub result: Option, pub steps_taken: u32, pub started_at: Instant, - /// `None` = full registry inheritance (v0.6.6 default). + /// `None` = full registry inheritance, with approval-gated tools still + /// blocked unless the parent runtime is auto-approved. /// `Some(list)` = explicit narrow allowlist (Custom agents, legacy). pub allowed_tools: Option>, /// Stable id of the manager that spawned this agent (#405). Compared @@ -1634,7 +1634,7 @@ impl ToolSpec for AgentSpawnTool { "allowed_tools": { "type": "array", "items": { "type": "string" }, - "description": "Explicit tool allowlist (required for custom type). Default behavior is full registry inheritance from the parent." + "description": "Explicit tool allowlist (required for custom type). Default behavior is full registry inheritance from the parent; approval-gated tools still require an auto-approved parent." }, "model": { "type": "string", @@ -1712,9 +1712,9 @@ impl ToolSpec for AgentSpawnTool { }; // Derive the child's runtime as a durable background job: it keeps - // its own cancellation token, forces auto_approve, and optionally - // overrides cwd if the caller passed one (used for the parallel- - // worktree pattern). + // its own cancellation token, inherits the parent approval state, and + // optionally overrides cwd if the caller passed one (used for the + // parallel-worktree pattern). let mut child_runtime = self.runtime.background_runtime(); if let Some(cwd) = validated_cwd { child_runtime.context.workspace = cwd; @@ -2705,6 +2705,7 @@ struct SubAgentTask { prompt: String, assignment: SubAgentAssignment, /// `None` = full registry inheritance. `Some(list)` = explicit narrow. + /// Approval-gated tools still require an auto-approved parent runtime. allowed_tools: Option>, fork_context: bool, started_at: Instant, @@ -3775,14 +3776,16 @@ fn emit_agent_progress( /// Two modes: /// - **Full inheritance** (`allowed_tools = None`): the child sees the same /// tool surface as the parent's Agent mode — every tool family including -/// `with_subagent_tools` (so it can recurse). This is the v0.6.6 default. +/// `with_subagent_tools` (so it can recurse). Approval-gated tools are +/// callable only when the parent runtime is auto-approved. /// - **Explicit narrow** (`allowed_tools = Some(list)`): legacy / Custom /// path. The registry still builds the full surface, but only the listed /// tool names are visible to the model and callable. struct SubAgentToolRegistry { - /// `None` → full inheritance (no filter applied). `Some(list)` → + /// `None` → full inheritance (no allowlist filter applied). `Some(list)` → /// only the listed tools are visible to the model and callable. allowed_tools: Option>, + auto_approve: bool, registry: ToolRegistry, } @@ -3812,6 +3815,7 @@ impl SubAgentToolRegistry { Self { allowed_tools: explicit_allowed_tools, + auto_approve: runtime.context.auto_approve, registry, } } @@ -3851,6 +3855,16 @@ impl SubAgentToolRegistry { if !self.is_tool_allowed(name) { return Err(anyhow!("Tool {name} not allowed for this sub-agent")); } + if !self.auto_approve { + let Some(spec) = self.registry.get(name) else { + return Err(anyhow!("Tool {name} is not registered")); + }; + if spec.approval_requirement() != ApprovalRequirement::Auto { + return Err(anyhow!( + "Tool {name} requires approval and cannot run inside this sub-agent unless the parent session is auto-approved" + )); + } + } reject_subagent_terminal_takeover(name, &input)?; self.registry .execute(name, input) @@ -3914,10 +3928,10 @@ fn build_allowed_tools( )); } - // Default: full registry inheritance from the parent. The child sees - // every tool the parent has, including the sub-agent management family - // (so it can recurse). Sandbox + workspace + depth cap remain the - // safety net. + // Default: full registry inheritance from the parent. The child sees every + // tool the parent has, including the sub-agent management family. The + // registry execution guard still blocks approval-gated tools unless the + // parent runtime is auto-approved. Ok(None) } diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 6bf6cfb2..f32f95c6 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -1066,18 +1066,46 @@ fn would_exceed_depth_at_boundary() { } #[test] -fn child_runtime_increments_depth_and_forces_auto_approve() { +fn child_runtime_increments_depth_and_preserves_auto_approve() { let mut parent = stub_runtime(); parent.spawn_depth = 1; parent.context.auto_approve = false; // parent in suggest mode let child = parent.child_runtime(); assert_eq!(child.spawn_depth, 2, "child depth = parent + 1"); assert!( - child.context.auto_approve, - "child must auto-approve regardless of parent mode (spawning IS the approval)" + !child.context.auto_approve, + "child must inherit parent approval state" ); - // Parent mode is unchanged — the override is on the child only. assert!(!parent.context.auto_approve); + + parent.context.auto_approve = true; + let auto_child = parent.child_runtime(); + assert!( + auto_child.context.auto_approve, + "auto-approved parents should still create auto-approved children" + ); +} + +#[tokio::test] +async fn subagent_registry_blocks_approval_tools_without_parent_auto_approve() { + let mut runtime = stub_runtime(); + runtime.context.auto_approve = false; + let registry = SubAgentToolRegistry::new( + runtime, + Some(vec!["exec_shell".to_string()]), + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + let err = registry + .execute("agent_test", "exec_shell", json!({"command": "echo hi"})) + .await + .expect_err("approval-gated child tool should be blocked"); + + assert!( + err.to_string().contains("requires approval"), + "unexpected error: {err}" + ); } #[test]