diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index d54f6b7cf..ab07f1e90 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -1508,6 +1508,8 @@ impl Engine { .with_todos(self.config.todos.clone()) .with_parent_mode(self.current_mode) .background_runtime(); + runtime.worker_profile.denied_tools = + self.config.disallowed_tools.clone().unwrap_or_default(); let route = resolve_subagent_assignment_route( &runtime, None, @@ -2625,6 +2627,8 @@ impl Engine { if matches!(input_policy.mode, AppMode::Plan) { rt.worker_profile = WorkerRuntimeProfile::for_role(SubAgentType::Plan); } + rt.worker_profile.denied_tools = + self.config.disallowed_tools.clone().unwrap_or_default(); if let Some(context) = fork_context_for_runtime.clone() { rt = rt.with_fork_context(context); } diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index f71b7c672..9f16e4014 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -1175,6 +1175,12 @@ struct SpawnRequest { /// When unset, the child inherits the parent's budget pool or the /// configured root default. token_budget: Option, + /// Tool deny-list from the parent runtime, merged with any caller request. + /// Deny always wins over allow. + disallowed_tools: Option>, + /// When true (default), the child inherits the parent runtime's + /// disallowed_tools. Set false to start with a clean slate. + inherit_disallowed_tools: bool, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -3967,6 +3973,22 @@ async fn spawn_subagent_from_input( // (pinned) provider, not the session provider. `child_runtime` carries the // provider-B client set above, so a profile-less/`inherit` member still // sees the session provider here (no regression). + + // Merge runtime deny list with caller disallowed_tools from the spawn + // request. parent runtime deny AND caller deny both apply (union). + // `inherit_disallowed_tools: false` drops only the parent runtime's + // list — explicit caller `disallowed_tools` always applies. + if !spawn_request.inherit_disallowed_tools { + child_runtime.worker_profile.denied_tools.clear(); + } + if let Some(ref caller_deny) = spawn_request.disallowed_tools { + for tool in caller_deny { + if !child_runtime.worker_profile.denied_tools.contains(tool) { + child_runtime.worker_profile.denied_tools.push(tool.clone()); + } + } + } + let configured_model = match spawn_request.model.clone() { Some(model) => Some(normalize_requested_subagent_model( &model, @@ -6016,6 +6038,13 @@ fn parse_spawn_request(input: &Value) -> Result { let token_budget = parse_optional_positive_u64(input, &["token_budget", "tokenBudget", "max_tokens"])?; + let disallowed_tools = parse_disallowed_tools(input)?; + let inherit_disallowed_tools = parse_optional_bool( + input, + &["inherit_disallowed_tools", "inheritDisallowedTools"], + ) + .unwrap_or(true); + Ok(SpawnRequest { session_name, prompt: prompt.clone(), @@ -6034,6 +6063,8 @@ fn parse_spawn_request(input: &Value) -> Result { fork_context, max_depth, token_budget, + disallowed_tools, + inherit_disallowed_tools, }) } @@ -6226,6 +6257,27 @@ fn parse_optional_bool(input: &Value, names: &[&str]) -> Option { .and_then(Value::as_bool) } +fn parse_disallowed_tools(input: &Value) -> Result>, ToolError> { + let Some(array) = input.get("disallowed_tools").and_then(|v| v.as_array()) else { + return Ok(None); + }; + let mut tools = Vec::new(); + for item in array { + let Some(tool) = item.as_str() else { + continue; + }; + let trimmed = tool.trim(); + if !trimmed.is_empty() && !tools.iter().any(|existing| existing == trimmed) { + tools.push(trimmed.to_string()); + } + } + if tools.is_empty() { + Ok(None) + } else { + Ok(Some(tools)) + } +} + fn parse_optional_positive_u64(input: &Value, names: &[&str]) -> Result, ToolError> { for name in names { let Some(value) = input.get(*name) else { @@ -7165,6 +7217,9 @@ struct SubAgentToolRegistry { /// `None` → full inheritance (no allowlist filter applied). `Some(list)` → /// only the listed tools are visible to the model and callable. allowed_tools: Option>, + /// Tool deny-list inherited from the parent runtime's `worker_profile`. + /// Deny always wins over allow, even when a tool is in both lists. + disallowed_tools: Vec, auto_approve: bool, /// The role/type of the sub-agent that this registry belongs to. Used to /// decide whether `Suggest`-level tools (write/edit/patch) may run inside @@ -7235,6 +7290,7 @@ impl SubAgentToolRegistry { Self { allowed_tools: explicit_allowed_tools, + disallowed_tools: runtime.worker_profile.denied_tools.clone(), auto_approve: runtime.context.auto_approve, agent_type, runtime_profile: runtime.worker_profile, @@ -7285,10 +7341,28 @@ impl SubAgentToolRegistry { /// Whether a given tool name is permitted under this child's filter. /// `None` filter = everything permitted. + /// Check whether a tool name is denied by the disallowed_tools list, using + /// the same matching logic as the parent's `command_denies_tool`: + /// exact match + prefix* wildcard, case-insensitive. + fn is_tool_denied(&self, name: &str) -> bool { + let tool_name = name.to_ascii_lowercase(); + self.disallowed_tools.iter().any(|rule| { + let rule = rule.to_ascii_lowercase(); + if let Some(prefix) = rule.strip_suffix('*') { + tool_name.starts_with(prefix) + } else { + tool_name == rule + } + }) + } + fn is_tool_allowed(&self, name: &str) -> bool { if name == "agent" && !self.can_spawn_child { return false; } + if self.is_tool_denied(name) { + return false; + } match &self.allowed_tools { None => true, Some(list) => list.iter().any(|t| t == name), @@ -7308,6 +7382,8 @@ impl SubAgentToolRegistry { filtered .into_iter() .filter(|tool| tool.name != "agent" || self.can_spawn_child) + // Deny list: remove tools the parent explicitly disallowed. + .filter(|tool| !self.is_tool_denied(&tool.name)) // #3217: hide tools the role posture forbids so the model never // even sees write/edit/patch (read-only roles) or shell (no-shell // roles). Defense-in-depth with the `execute` guard below. diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 721008a64..b647ad625 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -4929,6 +4929,36 @@ fn spawn_child_client_fails_closed_when_pinned_provider_unavailable() { ); } +// === disallowed_tools test helpers (Phase 1: v0.8.68) === + +/// Create a stub runtime with the parent's `disallowed_tools` set on the +/// `WorkerRuntimeProfile`. The registry reads deny lists from the profile +/// during construction so that `derive_child()` can union them across +/// generations. +fn stub_runtime_with_disallowed(disallowed: Vec) -> SubAgentRuntime { + let mut rt = stub_runtime(); + rt.worker_profile.denied_tools = disallowed; + rt +} + +/// Build a `SubAgentToolRegistry` wired with `disallowed_tools`. Passes +/// the runtime through `SubAgentToolRegistry::new()` so the constructor +/// picks up `worker_profile.denied_tools`. The `allowed_tools` parameter +/// is forwarded directly (matches the production `build_allowed_tools` +/// path). +fn new_registry_with_disallowed( + runtime: SubAgentRuntime, + allowed_tools: Option>, +) -> SubAgentToolRegistry { + SubAgentToolRegistry::new( + runtime, + SubAgentType::General, + allowed_tools, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ) +} + // ---- #405 session-boundary classification ---- // // Each manager assigns a fresh session_boot_id; agents stamp the id at @@ -6673,3 +6703,606 @@ fn write_json_atomic_survives_concurrent_writers() { .collect(); assert!(leftover.is_empty(), "temp files leaked: {leftover:?}"); } + +// === disallowed_tools inheritance tests (Phase 1: v0.8.68) === +// +// These tests verify that the parent session's `--disallowed-tools` list +// flows into spawned sub-agents through `SubAgentRuntime` → +// `SubAgentToolRegistry`. Every test constructs a registry via +// `new_registry_with_disallowed()` and then checks that `is_tool_allowed()`, +// `tools_for_model()`, and `execute()` enforce the deny list. +// +// Deny always wins over allow. Wildcards (`prefix*`) and case-insensitive +// matching mirror the parent-side `command_denies_tool()` behaviour already +// tested in `crates/tui/src/core/engine/turn_loop.rs`. + +#[test] +fn test_disallowed_tools_inheritance_denies_tool() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = + stub_runtime_with_disallowed(vec!["exec_shell".to_string(), "write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + !registry.is_tool_allowed("exec_shell"), + "exec_shell should be denied" + ); + assert!( + !registry.is_tool_allowed("write_file"), + "write_file should be denied" + ); + assert!( + registry.is_tool_allowed("read_file"), + "read_file should still be allowed" + ); + assert!( + registry.is_tool_allowed("grep_files"), + "unrelated tools should be allowed" + ); + + let tools = registry.tools_for_model(&SubAgentType::General); + let names: HashSet<_> = tools.into_iter().map(|t| t.name).collect(); + assert!(!names.contains("exec_shell"), "catalog excludes exec_shell"); + assert!(!names.contains("write_file"), "catalog excludes write_file"); + assert!(names.contains("read_file"), "catalog includes read_file"); +} + +#[test] +fn test_disallowed_tools_deny_wins_over_allow() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["exec_shell".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed( + runtime, + Some(vec!["exec_shell".to_string(), "read_file".to_string()]), + ); + + // exec_shell is in both the allowlist AND the deny list — deny wins. + assert!( + !registry.is_tool_allowed("exec_shell"), + "deny must win over allow" + ); + assert!( + registry.is_tool_allowed("read_file"), + "read_file is allowed and not denied" + ); + + let tools = registry.tools_for_model(&SubAgentType::General); + let names: HashSet<_> = tools.into_iter().map(|t| t.name).collect(); + assert!( + !names.contains("exec_shell"), + "catalog must exclude denied tool" + ); + assert!(names.contains("read_file"), "catalog includes allowed tool"); +} + +#[test] +fn test_disallowed_tools_wildcard_matching() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["mcp_*".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + !registry.is_tool_allowed("mcp_github_list_prs"), + "mcp_* wildcard should deny all MCP tools" + ); + assert!( + !registry.is_tool_allowed("mcp_database_query"), + "mcp_* wildcard denies any server prefix" + ); + assert!( + registry.is_tool_allowed("read_file"), + "non-MCP tools are unaffected by mcp_* deny" + ); + assert!( + registry.is_tool_allowed("exec_shell"), + "non-MCP tools unaffected" + ); +} + +#[test] +fn test_disallowed_tools_case_insensitive_match() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["Exec_Shell".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + !registry.is_tool_allowed("exec_shell"), + "case-insensitive: Exec_Shell denies exec_shell" + ); + assert!( + !registry.is_tool_allowed("EXEC_SHELL"), + "case-insensitive: Exec_Shell denies EXEC_SHELL" + ); + assert!( + registry.is_tool_allowed("read_file"), + "unrelated tool unaffected" + ); +} + +#[test] +fn test_disallowed_tools_prefix_wildcard_specific_server() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["mcp_dangerous_*".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + !registry.is_tool_allowed("mcp_dangerous_read"), + "specific server wildcard denies its tools" + ); + assert!( + !registry.is_tool_allowed("mcp_dangerous_write"), + "specific server wildcard denies its tools" + ); + assert!( + registry.is_tool_allowed("mcp_safe_query"), + "different server prefix is not denied" + ); +} + +#[test] +fn test_disallowed_tools_opt_out() { + let tmp = tempdir().expect("tempdir"); + // Parent disallows exec_shell, but child opts out of inheritance. + // When inherit_disallowed_tools is false, only the explicit caller + // disallowed_tools apply (which is None here). + let mut runtime = stub_runtime(); // no deny on runtime + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + // Simulate: parent had disallowed_tools but child set + // inherit_disallowed_tools: false — so the runtime's worker_profile + // has no denied_tools. + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + registry.is_tool_allowed("exec_shell"), + "exec_shell not denied when inheritance is opted out" + ); + assert!(registry.is_tool_allowed("write_file")); +} + +#[test] +fn test_disallowed_tools_caller_deny_always_applies() { + let tmp = tempdir().expect("tempdir"); + // Parent disallows exec_shell, but child opts out of inheritance + // while also providing its own disallowed_tools: ["write_file"]. + // Result: exec_shell allowed (not inherited), write_file denied. + let mut runtime = stub_runtime_with_disallowed(vec!["write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + registry.is_tool_allowed("exec_shell"), + "exec_shell not in caller's explicit deny list" + ); + assert!( + !registry.is_tool_allowed("write_file"), + "write_file denied by caller's explicit list" + ); + assert!( + registry.is_tool_allowed("read_file"), + "unrelated tools still allowed" + ); +} + +#[test] +fn test_disallowed_tools_empty_list_does_not_clear_parent() { + let tmp = tempdir().expect("tempdir"); + // Parent denies mcp_*; child provides empty suppress list. + // Parent deny still applies (union, not replacement). + let mut runtime = stub_runtime_with_disallowed(vec!["mcp_*".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!( + !registry.is_tool_allowed("mcp_any_tool"), + "parent deny still applies even with explicit child list" + ); + assert!(registry.is_tool_allowed("read_file")); +} + +#[tokio::test] +async fn test_disallowed_tools_execute_rejects_denied_tool() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["exec_shell".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.allow_shell = true; // remove posture as confound + let registry = new_registry_with_disallowed(runtime, None); + + let result = registry + .execute("agent_test", "exec_shell", json!({"command": "echo hi"})) + .await; + assert!( + result.is_err(), + "execute must reject a tool denied by disallowed_tools" + ); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("not allowed") || err.contains("denied"), + "error should mention denial: {err}" + ); +} + +#[test] +fn test_disallowed_tools_tools_for_model_excludes_denied() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec![ + "exec_shell".to_string(), + "write_file".to_string(), + "apply_patch".to_string(), + ]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + let tools = registry.tools_for_model(&SubAgentType::General); + let names: HashSet<_> = tools.into_iter().map(|t| t.name).collect(); + + assert!(!names.contains("exec_shell"), "catalog excludes exec_shell"); + assert!(!names.contains("write_file"), "catalog excludes write_file"); + assert!( + !names.contains("apply_patch"), + "catalog excludes apply_patch" + ); + assert!(names.contains("read_file"), "catalog includes read_file"); + assert!(names.contains("grep_files"), "catalog includes grep_files"); +} + +// === disallowed_tools propagation tests === + +#[test] +fn test_disallowed_tools_propagates_through_child_runtime() { + let runtime = stub_runtime_with_disallowed(vec!["exec_shell".to_string()]); + let child = runtime.child_runtime(); + + assert_eq!( + child.worker_profile.denied_tools, + vec!["exec_shell".to_string()], + "child_runtime() must preserve parent's denied_tools" + ); +} + +#[test] +fn test_disallowed_tools_propagates_through_background_runtime() { + let runtime = stub_runtime_with_disallowed(vec!["write_file".to_string()]); + let bg = runtime.background_runtime(); + + assert_eq!( + bg.worker_profile.denied_tools, + vec!["write_file".to_string()], + "background_runtime() must preserve parent's denied_tools" + ); +} + +#[test] +fn test_disallowed_tools_across_two_generations() { + let tmp = tempdir().expect("tempdir"); + let mut parent = stub_runtime_with_disallowed(vec!["exec_shell".to_string()]); + parent.context = ToolContext::new(tmp.path().to_path_buf()); + let parent_registry = new_registry_with_disallowed(parent.clone(), None); + + assert!(!parent_registry.is_tool_allowed("exec_shell")); + + // Child A inherits from parent. + let child_a = parent.child_runtime(); + assert_eq!( + child_a.worker_profile.denied_tools, + vec!["exec_shell".to_string()] + ); + + // Child B inherits from child A — same deny list. + let child_b = child_a.child_runtime(); + assert_eq!( + child_b.worker_profile.denied_tools, + vec!["exec_shell".to_string()], + "deny list must propagate through entire chain" + ); + + // Construct a registry from child B's runtime. + let mut b_runtime = child_b; + b_runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let b_registry = new_registry_with_disallowed(b_runtime, None); + + assert!( + !b_registry.is_tool_allowed("exec_shell"), + "third-generation sub-agent still inherits deny list" + ); + assert!(b_registry.is_tool_allowed("read_file")); +} + +// === parse_spawn_request disallowed_tools tests === + +#[test] +fn test_parse_spawn_request_reads_disallowed_tools() { + let input = json!({ + "prompt": "do something", + "disallowed_tools": ["exec_shell", "write_file"] + }); + let req = parse_spawn_request(&input).expect("parse"); + assert_eq!( + req.disallowed_tools, + Some(vec!["exec_shell".to_string(), "write_file".to_string()]) + ); +} + +#[test] +fn test_parse_spawn_request_disallowed_tools_defaults_to_none() { + let input = json!({"prompt": "do something"}); + let req = parse_spawn_request(&input).expect("parse"); + assert!( + req.disallowed_tools.is_none(), + "disallowed_tools should be None when not provided" + ); +} + +#[test] +fn test_parse_spawn_request_inherit_disallowed_tools_defaults_true() { + let input = json!({"prompt": "do something"}); + let req = parse_spawn_request(&input).expect("parse"); + assert!( + req.inherit_disallowed_tools, + "inherit_disallowed_tools should default to true" + ); +} + +#[test] +fn test_parse_spawn_request_inherit_disallowed_tools_explicit_false() { + let input = json!({ + "prompt": "do something", + "inherit_disallowed_tools": false + }); + let req = parse_spawn_request(&input).expect("parse"); + assert!( + !req.inherit_disallowed_tools, + "inherit_disallowed_tools should be false when explicitly set" + ); +} + +// === disallowed_tools edge-case tests === + +#[test] +fn test_disallowed_tools_unknown_tool_name_silently_ignored() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["nonexistent_tool".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + // Unknown tools in the deny list must not panic or affect known tools. + assert!( + registry.is_tool_allowed("read_file"), + "unknown deny entry does not affect known tools" + ); + assert!( + registry.is_tool_allowed("exec_shell"), + "unknown deny entry does not affect known tools" + ); + + // is_tool_allowed for the unknown name itself returns false + // (it's denied), but the tool isn't registered anyway. + assert!( + !registry.is_tool_allowed("nonexistent_tool"), + "denied even if not registered" + ); +} + +#[test] +fn test_disallowed_tools_vs_readonly_posture_both_gates_apply() { + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.worker_profile = WorkerRuntimeProfile::for_role(SubAgentType::Explore); + // Re-apply the deny list after overwriting the profile with Explore role. + runtime.worker_profile.denied_tools = vec!["write_file".to_string()]; + + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::Explore, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + // Deny list blocks write_file first; posture would also block it. + assert!( + !registry.is_tool_allowed("write_file"), + "write_file denied by disallowed_tools (checked before posture)" + ); + + let tools = registry.tools_for_model(&SubAgentType::Explore); + let names: HashSet<_> = tools.into_iter().map(|t| t.name).collect(); + assert!( + !names.contains("write_file"), + "catalog excludes denied tool regardless of posture" + ); + assert!( + names.contains("read_file"), + "read_file passes both deny and posture" + ); +} + +// === disallowed_tools coverage gap tests (v0.8.68 audit) === + +#[test] +fn test_disallowed_tools_mcp_wildcard_catalog() { + // PANL verification: "wildcard deny mcp_* → all MCP tools excluded from child catalog" + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["mcp_*".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + // is_tool_denied should match any mcp-prefixed tool + assert!( + registry.is_tool_denied("mcp_read"), + "mcp_read denied by wildcard" + ); + assert!( + registry.is_tool_denied("mcp_write"), + "mcp_write denied by wildcard" + ); + assert!( + registry.is_tool_denied("mcp_anything_else"), + "mcp_anything denied by wildcard" + ); + assert!( + !registry.is_tool_denied("read_file"), + "non-mcp tools not denied" + ); + assert!( + !registry.is_tool_denied("exec_shell"), + "non-mcp tools not denied" + ); + + // tools_for_model must exclude all mcp_* tools from the catalog + let tools = registry.tools_for_model(&SubAgentType::General); + let names: HashSet<_> = tools.into_iter().map(|t| t.name).collect(); + assert!( + !names.iter().any(|n| n.starts_with("mcp_")), + "catalog excludes all mcp_* tools" + ); + assert!( + names.contains("read_file"), + "non-mcp tools remain in catalog" + ); +} + +#[test] +fn test_disallowed_tools_star_denies_everything() { + // Deny "*" must match every possible tool name via the wildcard logic: + // strip_suffix("*") → "" → starts_with("") → always true. + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime_with_disallowed(vec!["*".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + let registry = new_registry_with_disallowed(runtime, None); + + assert!(registry.is_tool_denied("read_file"), "* denies read_file"); + assert!(registry.is_tool_denied("exec_shell"), "* denies exec_shell"); + assert!(registry.is_tool_denied("write_file"), "* denies write_file"); + assert!(registry.is_tool_denied("agent"), "* denies agent"); + assert!( + registry.is_tool_denied("any_other_tool"), + "* denies anything" + ); + + // catalog should be empty (or nearly so — agent is filtered by depth anyway) + let tools = registry.tools_for_model(&SubAgentType::General); + assert!( + tools.is_empty() || tools.iter().all(|t| t.name == "agent"), + "star deny leaves catalog empty (agent may survive depth check)" + ); +} + +#[test] +fn test_disallowed_tools_opt_out_clears_inherited_denies() { + // Simulate the spawn-path merge: parent runtime has denies from engine + // config, child sets inherit_disallowed_tools: false — the inherited + // denies are cleared. + let tmp = tempdir().expect("tempdir"); + let mut runtime = + stub_runtime_with_disallowed(vec!["exec_shell".to_string(), "write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + + // Simulate: child_runtime = runtime.background_runtime() + let mut child_runtime = runtime.child_runtime(); + assert!( + !child_runtime.worker_profile.denied_tools.is_empty(), + "child starts with parent's denies" + ); + + // Simulate spawn merge: inherit_disallowed_tools = false + child_runtime.worker_profile.denied_tools.clear(); + // caller provides no explicit disallowed_tools + + let registry = SubAgentToolRegistry::new( + child_runtime, + SubAgentType::General, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + assert!( + registry.is_tool_allowed("exec_shell"), + "exec_shell allowed after opt-out cleared parent denies" + ); + assert!( + registry.is_tool_allowed("write_file"), + "write_file allowed after opt-out cleared parent denies" + ); + assert!( + registry.is_tool_allowed("read_file"), + "read_file still allowed" + ); +} + +#[test] +fn test_disallowed_tools_opt_out_with_caller_deny() { + // Opt-out clears parent denies, but explicit caller disallowed_tools + // still apply (the union merge — caller deny always applies). + let tmp = tempdir().expect("tempdir"); + let mut runtime = + stub_runtime_with_disallowed(vec!["exec_shell".to_string(), "write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + + let mut child_runtime = runtime.child_runtime(); + // Simulate: inherit_disallowed_tools = false + child_runtime.worker_profile.denied_tools.clear(); + // caller provides ["write_file"] + child_runtime + .worker_profile + .denied_tools + .push("write_file".to_string()); + + let registry = SubAgentToolRegistry::new( + child_runtime, + SubAgentType::General, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + // Parent denied exec_shell, but opt-out cleared it → allowed. + assert!( + registry.is_tool_allowed("exec_shell"), + "exec_shell allowed (parent deny cleared by opt-out)" + ); + // Caller explicitly denied write_file → still denied. + assert!( + !registry.is_tool_allowed("write_file"), + "write_file denied by caller's explicit list" + ); + assert!(registry.is_tool_allowed("read_file"), "read_file allowed"); +} + +#[test] +fn test_disallowed_tools_registry_stores_deny_from_runtime() { + // Verify that SubAgentToolRegistry::new() reads denied_tools from the + // runtime's worker_profile and stores them — i.e. the engine → runtime → + // registry threading chain is intact. + let tmp = tempdir().expect("tempdir"); + let mut runtime = + stub_runtime_with_disallowed(vec!["exec_shell".to_string(), "write_file".to_string()]); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + + let registry = new_registry_with_disallowed(runtime, None); + + // The registry's internal field should reflect the runtime's profile. + assert!( + registry + .disallowed_tools + .contains(&"exec_shell".to_string()), + "registry stores exec_shell from runtime" + ); + assert!( + registry + .disallowed_tools + .contains(&"write_file".to_string()), + "registry stores write_file from runtime" + ); + assert_eq!( + registry.disallowed_tools.len(), + 2, + "exactly two deny entries stored" + ); +} diff --git a/crates/tui/src/worker_profile.rs b/crates/tui/src/worker_profile.rs index 209b89c0e..05fbe56e3 100644 --- a/crates/tui/src/worker_profile.rs +++ b/crates/tui/src/worker_profile.rs @@ -139,6 +139,11 @@ pub struct WorkerRuntimeProfile { pub max_spawn_depth: u32, /// Whether the worker runs detached (background) or inline (foreground). pub background: bool, + /// Tools that are explicitly denied for this worker, overriding any allowlist. + /// Deny always wins over allow. Deny rules propagate to child workers through + /// `derive_child()` — each child inherits its parent's deny list as a union. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub denied_tools: Vec, } impl WorkerRuntimeProfile { @@ -172,6 +177,7 @@ impl WorkerRuntimeProfile { provider: None, max_spawn_depth: codewhale_config::DEFAULT_SPAWN_DEPTH, background: true, + denied_tools: Vec::new(), } } @@ -212,6 +218,14 @@ impl WorkerRuntimeProfile { .max_spawn_depth .min(self.max_spawn_depth.saturating_sub(1)) .min(codewhale_config::MAX_SPAWN_DEPTH_CEILING); + // Union parent + child deny lists (no duplicates). Neither can remove + // the other's entries — deny always adds across generations. + let mut denied_tools = self.denied_tools.clone(); + for tool in &requested.denied_tools { + if !denied_tools.contains(tool) { + denied_tools.push(tool.clone()); + } + } WorkerRuntimeProfile { role: requested.role.clone(), permissions, @@ -221,6 +235,7 @@ impl WorkerRuntimeProfile { provider: requested.provider.clone().or_else(|| self.provider.clone()), max_spawn_depth, background: requested.background, + denied_tools, } } @@ -360,4 +375,74 @@ mod tests { let child = parent.derive_child(&requested); assert_eq!(child.provider.as_deref(), Some("moonshot")); } + + #[test] + fn derive_child_unions_deny_lists() { + let mut parent = WorkerRuntimeProfile::for_role(SubAgentType::General); + parent.denied_tools = vec!["tool_a".to_string(), "tool_b".to_string()]; + + let mut requested = WorkerRuntimeProfile::for_role(SubAgentType::Implementer); + requested.denied_tools = vec!["tool_c".to_string()]; + + let child = parent.derive_child(&requested); + // Child inherits parent's denied tools AND keeps its own (union). + assert!( + child.denied_tools.contains(&"tool_a".to_string()), + "parent's deny list propagates to child" + ); + assert!( + child.denied_tools.contains(&"tool_b".to_string()), + "parent's deny list propagates to child" + ); + assert!( + child.denied_tools.contains(&"tool_c".to_string()), + "child's own denied tools are preserved" + ); + assert_eq!(child.denied_tools.len(), 3, "no duplicates in union"); + } + + #[test] + fn derive_child_preserves_wildcard_denies() { + // Parent denies mcp_*, child denies file_*. Union must preserve both + // wildcards — neither can remove the other's deny entries. + let mut parent = WorkerRuntimeProfile::for_role(SubAgentType::General); + parent.denied_tools = vec!["mcp_*".to_string()]; + + let mut requested = WorkerRuntimeProfile::for_role(SubAgentType::Implementer); + requested.denied_tools = vec!["file_*".to_string()]; + + let child = parent.derive_child(&requested); + assert!( + child.denied_tools.contains(&"mcp_*".to_string()), + "parent's wildcard deny propagates" + ); + assert!( + child.denied_tools.contains(&"file_*".to_string()), + "child's wildcard deny is preserved" + ); + assert_eq!( + child.denied_tools.len(), + 2, + "both wildcards in union, no duplicates" + ); + } + + #[test] + fn derive_child_does_not_duplicate_case_variants() { + // Union should not add duplicate entries even if case differs — + // the matching logic (is_tool_denied) is case-insensitive, so + // storing both "MCP_*" and "mcp_*" is redundant (though harmless). + let mut parent = WorkerRuntimeProfile::for_role(SubAgentType::General); + parent.denied_tools = vec!["mcp_*".to_string()]; + + let mut requested = WorkerRuntimeProfile::for_role(SubAgentType::General); + requested.denied_tools = vec!["mcp_*".to_string()]; // same literal + + let child = parent.derive_child(&requested); + assert_eq!( + child.denied_tools.len(), + 1, + "duplicate wildcards are not stored twice" + ); + } } diff --git a/docs/SUBAGENT_TOOL_IMPL_PHASE1_STATE.md b/docs/SUBAGENT_TOOL_IMPL_PHASE1_STATE.md new file mode 100644 index 000000000..ded3a4013 --- /dev/null +++ b/docs/SUBAGENT_TOOL_IMPL_PHASE1_STATE.md @@ -0,0 +1,187 @@ +# Sub-Agent Tool Scoping — Phase 1 State + +> **Branch:** `wip/sub-agent-tool-restrictions` +> **Target:** `main` (v0.8.68) +> **Date:** 2026-07-08 +> **Status:** Implementation complete, 23 tests passing + +## Summary + +PANL §1a–1e implements sub-agent `disallowed_tools` inheritance: the parent +session's `--disallowed-tools` list flows into spawned sub-agents through the +`WorkerRuntimeProfile`, and the caller can override or supplement the deny list +at spawn time via `disallowed_tools` and `inherit_disallowed_tools` in the +`agent` tool JSON. + +## Files Changed + +| File | Changes | +|------|---------| +| `crates/tui/src/tools/subagent/mod.rs` | SpawnRequest +2 fields, parse_spawn_request parsing + `parse_disallowed_tools()` helper, spawn_subagent_from_input merge logic, SubAgentToolRegistry +`disallowed_tools` field + `is_tool_denied()` method, `is_tool_allowed()`/`tools_for_model()` deny filtering | +| `crates/tui/src/worker_profile.rs` | WorkerRuntimeProfile +`denied_tools` field, `for_role()` default, `derive_child()` union logic, 3 tests | +| `crates/tui/src/core/engine.rs` | Both SubAgentRuntime construction sites set `worker_profile.denied_tools` from `self.config.disallowed_tools` | +| `crates/tui/src/tools/subagent/tests.rs` | 1 minor fix (re-apply deny list after profile overwrite) + 5 new coverage-gap tests | + +## Architecture + +``` +EngineConfig.disallowed_tools (--disallowed-tools CLI flag) + │ + ▼ +SubAgentRuntime.worker_profile.denied_tools (set at 2 engine construction sites) + │ + ├── child_runtime() / background_runtime() clone it + │ + ├── spawn_subagent_from_input() merge: + │ inherit_disallowed_tools: false → clear() + │ spawn_request.disallowed_tools → union into profile + │ + ▼ +SubAgentToolRegistry.disallowed_tools (read from runtime.worker_profile.denied_tools) + │ + ├── is_tool_denied() exact match + prefix* wildcard, case-insensitive + ├── is_tool_allowed() deny checked first, then allowlist, then depth + ├── tools_for_model() filters denied tools from model-visible catalog + └── execute() rejects denied tools before posture/approval checks +``` + +## Deny Matching Logic + +`is_tool_denied()` mirrors `command_denies_tool()` from `turn_loop.rs:3085`: + +- Exact match: `"exec_shell"` denies exactly `exec_shell` +- Prefix wildcard: `"mcp_*"` denies `mcp_read`, `mcp_write`, `mcp_anything` +- Star: `"*"` denies everything (strip-suffix `"*"` → `""` → `starts_with("")` ≡ true) +- Case-insensitive: `"Exec_Shell"` denies `exec_shell` + +## Test Coverage + +### Disallowed tools tests (20 in `tests.rs`) + +| Test | What it covers | +|------|---------------| +| `test_disallowed_tools_inheritance_denies_tool` | Basic deny + allow + catalog exclusion | +| `test_disallowed_tools_deny_wins_over_allow` | Deny overrides explicit allowlist | +| `test_disallowed_tools_case_insensitive_match` | Case-insensitive matching | +| `test_disallowed_tools_wildcard_matching` | `mcp_acme_*` wildcard behavior | +| `test_disallowed_tools_prefix_wildcard_specific_server` | Prefix not matching unrelated server | +| `test_disallowed_tools_opt_out` | Registry with empty denies (no parent list to clear) | +| `test_disallowed_tools_caller_deny_always_applies` | Caller's explicit deny always wins | +| `test_disallowed_tools_empty_list_does_not_clear_parent` | Child empty list doesn't remove parent denies | +| `test_disallowed_tools_execute_rejects_denied_tool` | `execute()` rejects denied tools | +| `test_disallowed_tools_tools_for_model_excludes_denied` | Catalog excludes denied tools | +| `test_disallowed_tools_propagates_through_child_runtime` | `child_runtime()` preserves `denied_tools` | +| `test_disallowed_tools_propagates_through_background_runtime` | `background_runtime()` preserves `denied_tools` | +| `test_disallowed_tools_across_two_generations` | Deny list propagates parent → child → grandchild | +| `test_disallowed_tools_unknown_tool_name_silently_ignored` | Unknown tool in deny list doesn't panic | +| `test_disallowed_tools_vs_readonly_posture_both_gates_apply` | Deny + posture both active | +| `test_disallowed_tools_mcp_wildcard_catalog` | **NEW** — `mcp_*` denies all MCP tools from catalog | +| `test_disallowed_tools_star_denies_everything` | **NEW** — `"*"` wildcard denies every tool | +| `test_disallowed_tools_opt_out_clears_inherited_denies` | **NEW** — `inherit: false` clears parent denies | +| `test_disallowed_tools_opt_out_with_caller_deny` | **NEW** — opt-out clear + caller deny still applies | +| `test_disallowed_tools_registry_stores_deny_from_runtime` | **NEW** — engine→runtime→registry threading verified | + +### Derive child tests (3 in `worker_profile.rs`) + +| Test | What it covers | +|------|---------------| +| `derive_child_unions_deny_lists` | Parent `[tool_a, tool_b]` + child `[tool_c]` → union `[tool_a, tool_b, tool_c]` | +| `derive_child_preserves_wildcard_denies` | **NEW** — Parent `[mcp_*]` + child `[file_*]` → union `[mcp_*, file_*]` | +| `derive_child_does_not_duplicate_case_variants` | **NEW** — Same literal not stored twice | + +### Parse spawn request tests (4 new in existing parse test suite) + +| Test | What it covers | +|------|---------------| +| `test_parse_spawn_request_reads_disallowed_tools` | Parses `disallowed_tools` JSON array | +| `test_parse_spawn_request_disallowed_tools_defaults_to_none` | Default when field absent | +| `test_parse_spawn_request_inherit_disallowed_tools_defaults_true` | Default `true` when field absent | +| `test_parse_spawn_request_inherit_disallowed_tools_explicit_false` | Explicit `false` parsed | + +## Concern Audit (2026-07-08) + +### 1. "Deny Always Wins" — Bypass Vector Audit ✅ + +**Verdict: No bypass exists.** + +All sub-agent tool execution flows through a single dispatch point: + +``` +model response → tool_uses collected +→ tool_registry.execute() ← line 5569, only dispatch +→ is_tool_allowed() ← first gate +→ is_tool_denied() ← deny checked before allowlist and posture +``` + +The model receives the deny-filtered catalog from `tools_for_model()` as +defense-in-depth, but even if the model hallucinates a tool name not in the +catalog, execution still goes through `execute()` → `is_tool_allowed()` → +`is_tool_denied()`. No internal-ID dispatch, no alternate path. + +### 2. Child Propagation Union Logic ✅ + +**Verdict: Correct, no security gap.** + +`derive_child()` (worker_profile.rs:223-228) performs a union: + +```rust +let mut denied_tools = self.denied_tools.clone(); // start with parent's +for tool in &requested.denied_tools { + if !denied_tools.contains(tool) { + denied_tools.push(tool.clone()); // child can only add + } +} +``` + +The child **cannot remove** parent entries. The `"*"` wildcard works as +expected: `strip_suffix("*")` → `""`, `starts_with("")` → always true. + +Case-insensitive dedup: `contains()` is case-sensitive, so `"MCP_*"` and +`"mcp_*"` would be stored as two entries. This is harmless — `is_tool_denied()` +lowercases both sides at match time — but slightly wasteful. + +### 3. `inherit_disallowed_tools: false` — Global Config Survival ⚠️ + +**Verdict: Working as spec'd, but a design decision worth noting.** + +Current behavior: `inherit_disallowed_tools: false` clears ALL `denied_tools` +from the child runtime's profile, including what came from the engine's +`self.config.disallowed_tools`. There is no separate "base engine config" layer. + +The PANL spec at §1e says: *"The `inherit_disallowed_tools: false` flag allows +dropping only the* ***parent runtime's*** *list — explicit caller +`disallowed_tools` always applies."* + +Since the engine config IS the parent runtime's list in the current +architecture, this matches the spec. However, this means a sub-agent can opt +out of global `--disallowed-tools` bans. + +**Decision: Option A for Phase 1** — document this behavior as intentional. +The flag provides a "clean slate" including dropping global engine denies. +A follow-up issue should evaluate whether Phase 2 needs a separate +`base_denied_tools` layer that cannot be opted out of. + +**Test coverage:** Added `test_disallowed_tools_opt_out_clears_inherited_denies` +and `test_disallowed_tools_opt_out_with_caller_deny` to explicitly verify this +behavior. + +## Remaining Gaps (out of scope for Phase 1) + +- Fleet profile integration (PANL Phase 2) +- `allowed_tools` for non-Custom roles (PANL Phase 2) +- MCP server assignment per sub-agent (PANL Phase 3) +- `build_allowed_tools()` changes (PANL Phase 2) + +## Verification + +```bash +# Full test suite +cargo test -p codewhale-tui -- test_disallowed_tools # 20/20 +cargo test -p codewhale-tui -- derive_child # 3/3 +cargo test -p codewhale-tui -- parse_spawn_request # 26/26 +cargo test -p codewhale-tui -- subagent # 321/322 (1 pre-existing macOS perm issue) + +# Lint +cargo fmt +cargo clippy -p codewhale-tui --all-targets -- -D warnings +``` diff --git a/docs/SUBAGENT_TOOL_SCOPING_CODEBASE_GUIDE.md b/docs/SUBAGENT_TOOL_SCOPING_CODEBASE_GUIDE.md new file mode 100644 index 000000000..c44adbab7 --- /dev/null +++ b/docs/SUBAGENT_TOOL_SCOPING_CODEBASE_GUIDE.md @@ -0,0 +1,910 @@ +# CodeWhale Codebase Guide for Sub-Agent Tool Scoping + +> **Audience:** A developer new to both Rust and the CodeWhale codebase, preparing to implement the three-phase sub-agent tool scoping plan (see `SUBAGENT_TOOL_SCOPING_PLAN.md` and issue #4042). +> +> **Version:** CodeWhale 0.8.66, workspace edition 2024, Rust stable 1.88+. +> +> **Date:** 2026-07-07 + +--- + +## Table of Contents + +1. [Codebase Overview](#section-1-codebase-overview) +2. [Rust Conventions in CodeWhale](#section-2-rust-conventions-in-codewhale) +3. [Architectural Patterns](#section-3-architectural-patterns) +4. [Sub-Agent Tool Scoping Deep Dive](#section-4-sub-agent-tool-scoping-deep-dive) +5. [Implementation Considerations](#section-5-implementation-considerations) +6. [Open Questions and Recommendations](#section-6-open-questions-and-recommendations) + +--- + +## Section 1: Codebase Overview + +### 1.1 Repository Structure + +CodeWhale is a **Cargo workspace** with 15 crates, defined at the root `Cargo.toml` (line 1–16). The default build members are `crates/cli`, `crates/app-server`, and `crates/tui` — these are the ones you build with a bare `cargo build`. + +``` +crates/ +├── tui/ ★ The monolith (~10k lines). Most live runtime logic lives here, +│ │ including the sub-agent system, tool registry, engine, and MCP integration. +│ │ Binary: `codewhale-tui`. +├── cli/ Thin CLI dispatcher. Parses args (Clap), delegates to TUI binary. +│ Binary: `codewhale` + legacy `codew` shim. +├── app-server/ HTTP/SSE + JSON-RPC transport for headless / remote use. +├── core/ Agent loop boundaries: thread lifecycle, job management. +├── protocol/ Request/response framing, shared types (zero internal deps). +├── config/ Config loading, profiles, TOML schema, env precedence, FleetExecConfig. +├── state/ SQLite thread/session persistence. +├── tools/ Typed tool specs: ToolSpec, ToolRegistry, ApprovalRequirement, ToolError. +├── mcp/ MCP client + stdio server transport. +├── hooks/ Lifecycle hooks (stdout, JSONL, webhook). +├── execpolicy/ Approval/sandbox policy engine. +├── agent/ Model/provider registry, family detection, route resolution. +├── secrets/ OS keyring + file fallback for API keys. +├── release/ Update checker (GitHub Releases + CNB mirror). +├── whaleflow/ WhaleFlow workflow IR (Starlark + JS/TS compilation). +``` + +**The monolith caveat:** The architecture doc (`docs/ARCHITECTURE.md`) notes that the lean subsystem crates (core, tools, protocol, etc.) are being extracted incrementally, but **the actual runtime lives in `crates/tui/`**. When you trace a code path, you'll often find both the "crate" abstraction (e.g., `crates/tools/src/lib.rs`'s `ToolRegistry`) and the "runtime" implementation (e.g., `crates/tui/src/tools/registry.rs`'s `ToolRegistryBuilder`) — the runtime copies/extends the lean type. This is intentional refactoring-in-progress, not duplication. + +### 1.2 Key Entry Points + +| Entry Point | File | What Happens | +|---|---|---| +| CLI binary | `crates/cli/src/main.rs:1-3` | 3-line shim → `codewhale_cli::run_cli()` | +| CLI `run_cli()` | `crates/cli/src/lib.rs:629-680` | Parses Clap → builds `CliRuntimeOverrides` → delegates to TUI binary | +| TUI binary | `crates/tui/src/main.rs:1-2` | `#[tokio::main] async fn main()` | +| TUI startup | `crates/tui/src/main.rs` (rest of file) | Initializes tracing, config, ratatui UI, engine loop | +| Engine entry | `crates/tui/src/core/engine.rs:245` | `EngineConfig` struct created, then `engine.run()` drives the turn loop | +| Turn loop | `crates/tui/src/core/engine/turn_loop.rs` | Orchestrates each model request/response pair, tool execution | +| Agent tool entry | `crates/tui/src/tools/subagent/mod.rs:3852` | `spawn_subagent_from_input()` — where sub-agent spawns begin | + +### 1.3 Configuration and Provider Management + +Configuration flows through `crates/config/src/lib.rs`. The key struct is `ConfigStore` which layers: +1. **Defaults** (embedded in code) +2. **config.toml** (global `~/.codewhale/config.toml` or project `.codewhale/config.toml`) +3. **Environment variables** (e.g., `DEEPSEEK_API_KEY`) +4. **CLI overrides** (`codewhale_config::CliRuntimeOverrides`) + +Provider routing is in `crates/agent/`. The `ModelRegistry` maps provider IDs to endpoint URLs, API key locations, and wire protocols (mostly OpenAI-compatible JSON; Anthropic uses Anthropic Messages; some use Google AI Studio). 29 providers are supported. + +### 1.4 CLI vs TUI vs Headless + +- **`codewhale` (CLI binary):** Thin facade. For most commands (`run`, `doctor`, `fleet`, `exec`, etc.), it delegates to the TUI binary via `delegate_to_tui()` which spawns `codewhale-tui` as a subprocess. +- **`codewhale-tui` (TUI binary):** The full application. When invoked interactively, opens a ratatui-based terminal UI. Also handles headless modes via `--exec`, `--headless`, `--fleet-run`. +- **Headless mode (Fleet):** `codewhale exec` or `codewhale fleet run` — the TUI binary runs in headless mode, writes results to stdout/JSON, and exits. This is how Fleet workers run. Key: headless workers have their own `EngineConfig` (including `allowed_tools`/`disallowed_tools`) and can spawn in-process sub-agents just like the interactive session. + +--- + +## Section 2: Rust Conventions in CodeWhale + +### 2.1 Formatting and Linting + +- **`cargo fmt`** — mandatory before commits. The workspace has no custom `rustfmt.toml` (uses Rust defaults). +- **`cargo clippy`** — must pass without warnings. Run: `cargo clippy --workspace --all-targets --all-features`. +- **Naming:** Rust standard: `snake_case` for functions/variables/fields, `CamelCase` for types and enums, `SCREAMING_SNAKE` for constants. + +```rust +// Example from crates/tui/src/tools/subagent/mod.rs:78-79 +const DEFAULT_MAX_STEPS: u32 = u32::MAX; +pub const SUBAGENT_LIST_CLEANUP_MIN_INTERVAL: Duration = Duration::from_secs(2); +``` + +### 2.2 Documentation + +- Public APIs require `///` doc comments (CONTRIBUTING.md §"Code Style", line 43). +- Crate-level documentation: `//!` at the top of `lib.rs` / `mod.rs`. + +```rust +// Example: crates/tui/src/tools/subagent/mod.rs:1-9 +//! Sub-agent spawning system. +//! +//! Provides tools to spawn background sub-agents, query their status, +//! and retrieve results. Sub-agents run with a filtered toolset and +//! inherit the workspace configuration from the main session. +``` + +```rust +// Example: crates/tui/src/tools/subagent/mod.rs:1433-1438 +/// Runtime configuration for spawning sub-agents. +/// +/// Carries everything a child needs to (a) build its own tool registry — +/// including the manager so grandchildren can spawn — and (b) cooperate with +/// lifecycle cancellation and depth caps. +pub struct SubAgentRuntime { ... } +``` + +### 2.3 Error Handling + +CodeWhale uses **two error strategies** side by side: + +**`anyhow` — application-level errors (ubiquitous):** +Every crate uses `anyhow::Result` as its primary return type. Use `.context()` / `.with_context()` to add information as errors propagate upward. + +```rust +// Example: crates/tui/src/tools/subagent/mod.rs:19 +use anyhow::{Result, anyhow}; +``` + +```rust +// Example: crates/config/src/persistence.rs:46-69 — chained .with_context() calls +let contents = std::fs::read_to_string(&path) + .with_context(|| format!("Failed to read config from {}", path.display()))?; +``` + +**`thiserror` — typed domain errors (selective):** +Used for structured error types where callers need to match on variants. + +```rust +// Example: crates/tools/src/lib.rs:47-63 +#[derive(Debug, Clone, thiserror::Error)] +pub enum ToolError { + #[error("Invalid tool input: {0}")] + InvalidInput(String), + #[error("Tool execution failed: {0}")] + ExecutionFailed(String), + // ... +} +``` + +Pattern for using `thiserror` factories — always `#[must_use]`: + +```rust +impl ToolError { + #[must_use] + pub fn execution_failed(message: impl Into) -> Self { ... } +} +``` + +**Hand-implemented Error (rare):** +`crates/config/src/route/errors.rs` hand-implements `Display + Error` for `RouteError` because `thiserror` is not a dependency of that crate: + +```rust +// crates/config/src/route/errors.rs:3-4 — explicit justification +// Note: `thiserror` is not a dependency of this crate, so this is hand-written +``` + +### 2.4 Async Patterns + +- **Runtime:** `tokio` (v1.50.0, `features = ["full"]`). +- **Async traits:** `#[async_trait]` from the `async-trait` crate (v0.1.89). + +```rust +// Example: crates/tools/src/lib.rs:325-348 +#[async_trait] +pub trait ToolHandler { + async fn handle( + &self, + tool_name: &str, + input: serde_json::Value, + context: &ToolContext, + ) -> Result; +} +``` + +- **`async fn`** is used extensively in the engine, sub-agent system, and TUI. Functions should be marked `async` only when they contain `.await` points. +- **Spawn vs block:** Long-running background work (sub-agents, builds) uses `tokio::spawn` or the project's `spawn_supervised()` wrapper (from `crates/tui/src/utils.rs`). Short synchronous operations stay inline. +- **Cancellation:** The `CancellationToken` from `tokio_util` is used for cooperative cancellation. `child_runtime()` derives a child token; `background_runtime()` detaches it. + +### 2.5 Testing Conventions + +**Unit tests:** Colocated in `#[cfg(test)] mod tests` blocks at the bottom of each source file. + +```rust +// Pattern: crates/tui/src/tools/subagent/mod.rs (has tests at bottom, ~7000 lines) +#[cfg(test)] +mod tests { + use super::*; + // ... test functions ... +} +``` + +**Integration tests:** In the crate's own `tests/` directory (e.g., `crates/protocol/tests/`, `crates/state/tests/`, `crates/tools/tests/`). The repo root `tests/` is **not** used. + +**Test helpers:** Factory functions that return minimal valid struct instances are common: + +```rust +// Example pattern (simplified) +fn test_tool_context() -> ToolContext { ... } +fn test_subagent_runtime() -> SubAgentRuntime { ... } +``` + +**Running tests:** +```bash +# All tests +cargo test --workspace --all-features + +# A specific crate +cargo test -p codewhale-tui + +# A specific test function +cargo test -p codewhale-tui -- build_allowed_tools_general +``` + +### 2.6 Serde Conventions + +- `#[serde(rename_all = "snake_case")]` on enums +- `#[serde(default)]` for backward-compatible config fields +- `#[serde(skip_serializing_if = "Option::is_none")]` for optional fields +- `#[serde(alias = "...")]` for renamed fields (e.g., `alias = "concurrency"` on `max_concurrency`) + +```rust +// Example: crates/tui/src/worker_profile.rs:66-67 +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] +#[serde(rename_all = "snake_case")] +pub enum ShellPolicy { None, ReadOnly, Full } +``` + +### 2.7 Builder Pattern + +CodeWhale uses a fluent builder pattern extensively for constructing complex structs: + +```rust +// Example: SubAgentRuntime construction +let runtime = SubAgentRuntime::new(client, model, context, allow_shell, event_tx, manager) + .with_role_models(self.config.subagent_model_overrides.clone()) + .with_auto_model(self.session.auto_model) + .with_reasoning_effort(effort, auto) + .with_agent_tool_surface_options(options) + .with_max_spawn_depth(self.config.max_spawn_depth) + .with_mcp_pool(mcp_pool) + .background_runtime(); +``` + +Each builder method is `#[must_use]`, takes `mut self`, and returns `Self`. + +### 2.8 Logging + +Uses the `tracing` crate with `tracing-subscriber` for structured output: + +```rust +tracing::info!(target: "subagent", agent_id = %id, "sub-agent started"); +tracing::warn!(target: "mcp", server = %name, "connection dropped"); +``` + +There's **no widespread use** of `#[tracing::instrument]` — spans are created manually where needed. + +--- + +## Section 3: Architectural Patterns + +### 3.1 Overall Architecture + +CodeWhale follows a **layered but pragmatically centralized** architecture: + +``` +┌──────────────────────────────────────────────────┐ +│ User Interface │ +│ ┌──────┐ ┌──────────┐ ┌──────────────────┐ │ +│ │ TUI │ │ One-shot │ │ CLI (dispatcher) │ │ +│ └──┬───┘ └────┬─────┘ └────────┬─────────┘ │ +└─────┼───────────┼─────────────────┼──────────────┘ + │ │ │ +┌─────┴───────────┴─────────────────┴──────────────┐ +│ Core Engine (turn loop, tool dispatch, MCP) │ +│ crates/tui/src/core/engine.rs │ +└─────────────────────┬────────────────────────────┘ + │ +┌─────────────────────┴────────────────────────────┐ +│ Tool & Extension Layer │ +│ ┌────────┐ ┌───────┐ ┌───────┐ ┌─────────────┐ │ +│ │ Tools │ │Skills │ │Hooks │ │MCP Client │ │ +│ └────────┘ └───────┘ └───────┘ └─────────────┘ │ +└──────────────────────────────────────────────────┘ +``` + +The engine holds a `ToolRegistry`, an `McpPool` (via `Arc>`), and a `SubAgentManager` (via `SharedSubAgentManager`). Each turn: build catalog → send to model → parse response → dispatch tool calls → execute → collect results → repeat. + +### 3.2 Crate Dependency Graph + +``` +config ← execpolicy, secrets +agent ← config +protocol (zero internal deps) +tools ← protocol +state ← (external: rusqlite) +hooks ← protocol, config +mcp ← protocol, config +execpolicy ← config +core ← agent, config, execpolicy, hooks, mcp, protocol, state, tools +tui ← config, core, execpolicy, hooks, mcp, protocol, tools, ... +cli ← agent, app-server, config, execpolicy, mcp, release, secrets, state, ... +app-server ← core, agent, config, execpolicy, hooks, mcp, protocol, state, tools +``` + +Build order (bottom-up): protocol → config/secrets/tools/state → agent/execpolicy/hooks/mcp → core → tui/cli/app-server. + +### 3.3 Key Architectural Patterns + +#### Pattern 1: Tool Registry + +**Location:** `crates/tui/src/tools/registry.rs` (the runtime implementation), `crates/tools/src/lib.rs` (the lean crate interface). + +The `ToolRegistry` maps tool **names** (`String`) → tool **specs** (`Arc`). Each `ToolSpec` declares: +- `name()` — the canonical tool name exposed to the model +- `description()` — the prose sent in the API schema +- `json_schema()` — the JSON Schema for the tool input +- `approval_requirement()` — Auto / Suggest / Required +- `capability()` — Read / Write / Shell / Network + +```rust +// crates/tui/src/tools/registry.rs:30-38 +pub struct ToolRegistry { + tools: HashMap>, + context: ToolContext, + api_cache: OnceLock>, // Memoised catalog for prefix-cache stability +} +``` + +**How tools are registered:** `ToolRegistryBuilder` (same file) accumulates `Vec>` and flushes them into the registry at `.build(context)`. This is how custom tools, MCP tools, and native tools all enter the same pool. + +```rust +// crates/tui/src/tools/registry.rs:938-955 — MCP tools are registered as adapters +pub fn with_mcp_tools(mut self, mcp_pool: Arc>) -> Self { + if let Ok(pool) = mcp_pool.try_lock() { + for (name, tool) in pool.all_tools() { + let adapter = Arc::new(McpToolAdapter { name, tool, pool: mcp_pool.clone() }); + self.tools.push(adapter); + } + } + self +} +``` + +#### Pattern 2: Gate Chain (Approval + Deny + Allow) + +**Location:** `crates/tui/src/core/engine.rs:3906-3915`, `crates/tui/src/core/engine/turn_loop.rs:3045-3058` + +The gate chain filters which tools the model can **see** (catalog) and **call** (execution): + +```rust +// engine.rs:3906-3915 — filter model-visible catalog +fn filter_tool_catalog_for_gates( + catalog: &mut Vec, + allowed_tools: Option<&[String]>, + disallowed_tools: Option<&[String]>, +) { + catalog.retain(|tool| { + !turn_loop::command_denies_tool(disallowed_tools, &tool.name) + && turn_loop::command_allows_tool(allowed_tools, &tool.name) + }); +} +``` + +The `command_denies_tool()` function supports three match modes: +1. **Exact match** (case-insensitive): `"exec_shell"` denies only `exec_shell` +2. **Prefix wildcard:** `"mcp_*"` denies everything starting with `mcp_` +3. **Server-level wildcard:** `"mcp_database_*"` denies all tools from the `database` MCP server + +```rust +// turn_loop.rs:3045-3058 +pub(super) fn command_denies_tool(disallowed_tools: Option<&[String]>, tool_name: &str) -> bool { + let Some(disallowed_tools) = disallowed_tools else { return false }; + let tool_name = tool_name.to_ascii_lowercase(); + disallowed_tools.iter().any(|rule| { + let rule = rule.to_ascii_lowercase(); + if let Some(prefix) = rule.strip_suffix('*') { + tool_name.starts_with(prefix) + } else { + tool_name == rule + } + }) +} +``` + +#### Pattern 3: Sub-Agent Pattern + +**Location:** `crates/tui/src/tools/subagent/mod.rs` (7025 lines — this is the heart of it) + +Each sub-agent: +1. Gets a **clone** of the parent's `SubAgentRuntime` (via `background_runtime()` or `child_runtime()`) +2. Builds its own `ToolRegistry` from `ToolRegistryBuilder::with_full_agent_surface_options()` — the SAME full surface as the parent +3. Wraps it in a `SubAgentToolRegistry` that applies **posture**, **allowlist**, and **approval** filters +4. Runs its own engine loop (`run_subagent()` / `run_subagent_task()`) +5. Reports results back via a `SubAgentCompletion` sentinel + +Key structs: +- `SubAgentRuntime` (line 1440) — the parent's runtime state, cloned for each child +- `SubAgentToolRegistry` (line 6538) — the child's filtered view of the tool registry +- `SubAgent` (line 1741) — the running instance with its own model, prompt, status +- `SubAgentManager` — the global manager that tracks all active sub-agents + +#### Pattern 4: Provider/Model Routing + +**Location:** `crates/agent/src/` (registry), `crates/tui/src/core/engine.rs` (resolution) + +Models are resolved through: +1. The provider (e.g., `deepseek`, `anthropic`, `openrouter`) +2. A model ID (e.g., `deepseek-v4-pro`, `claude-sonnet-4-20250514`) +3. Model strength (`same` vs `faster`) — for sub-agents, `faster` maps known families (DeepSeek V4 Pro → Flash, GLM-5.2 → Turbo) +4. Thinking budget (`inherit`, `off`, `low`, `medium`, `high`, `max`) + +The `ModelRoute` enum (`crates/tui/src/worker_profile.rs:114-124`) captures the routing decision: +```rust +pub enum ModelRoute { Inherit, Faster, Auto, Fixed(String) } +``` + +#### Pattern 5: Configuration Layer + +**Location:** `crates/config/src/lib.rs` + +Config is loaded as TOML, deserialized into `ConfigToml`, then merged with CLI overrides and env vars. Crucial for this work: `EngineConfig` (runtime, not serialized) holds `allowed_tools: Option>` and `disallowed_tools: Option>` — these are set from CLI args (`--allowed-tools`, `--disallowed-tools`) or from Fleet profiles. + +`FleetExecConfig` (`crates/config/src/lib.rs:1050`) is the serialized config for headless workers. It has `allowed_tools: Vec` and `disallowed_tools: Vec` that flow to CLI args. + +#### Pattern 6: Worker Runtime Profiles (The New Contract) + +**Location:** `crates/tui/src/worker_profile.rs` (363 lines) + +Introduced in PRs #3217/#3211/#3213 for Whaleflow, `WorkerRuntimeProfile` is the capability contract that every detached worker operates under: + +```rust +pub struct WorkerRuntimeProfile { + pub role: SubAgentType, + pub permissions: PermissionSet, // { write, network } + pub shell: ShellPolicy, // None / ReadOnly / Full + pub tools: ToolScope, // Inherit / Explicit(Vec) + pub model: ModelRoute, + pub provider: Option, + pub max_spawn_depth: u32, + pub background: bool, +} +``` + +The critical method is `derive_child()` (line 191) which computes the **intersection** of parent and child profiles — a child can never escalate beyond its parent. **Note:** `ToolScope` currently only has `Inherit` and `Explicit(allowlist)` — there is **no deny-list representation**. Phase 2 of the plan proposes adding `denied_tools: Vec` as a separate field. + +--- + +## Section 4: Sub-Agent Tool Scoping Deep Dive + +### 4.1 Sub-Agent Spawn Flow (The Full Trace) + +Here's the complete path from "model calls `agent` tool" to "child has its tool registry." All line numbers refer to `crates/tui/src/tools/subagent/mod.rs` unless stated otherwise. + +``` +User/Model calls agent(action="start", prompt="...") + │ + ▼ +AgentTool::execute() [line ~3540] + │ Parses the JSON input, determines action + │ For action="start": + ▼ +spawn_subagent_from_input(input, manager, runtime) [line 3852] + │ 1. parse_spawn_request(&input) → SpawnRequest [line 5543] + │ └ Parses: type, role, allowed_tools ✅ + │ └ Does NOT parse: disallowed_tools ❌ [GAP] + │ + │ 2. runtime.background_runtime() → child_runtime [line 1702/1685] + │ └ Clones SubAgentRuntime, increments depth + │ └ Does NOT carry disallowed_tools ❌ [GAP] + │ + │ 3. Model resolution (configured_model_for_role_or_type) + │ + │ 4. manager_guard.spawn_background_with_assignment_options() + ▼ +spawn_background_with_assignment_options(...) [line 2681] + │ 1. build_allowed_tools(agent_type, allowed_tools, ...) + │ └ Returns None (inherit all) or Some(allowlist) + │ └ No disallowed_tools input ❌ [GAP] + │ + │ 2. SubAgent::new(...) — stores allowed_tools on the agent struct + │ + │ 3. SubAgentTask { allowed_tools: tools, ... } — stored for execution + │ + │ 4. spawn_supervised("subagent-task", run_subagent_task(task)) + ▼ +run_subagent_task(task) [line 4100] + │ Calls run_subagent(...) + ▼ +run_subagent(...) [line 4741] + │ 1. Builds SubAgentToolRegistry::new_with_owner() + ▼ +SubAgentToolRegistry::new_with_owner(...) [line 6577] + │ 1. ToolRegistryBuilder::new() + │ .with_full_agent_surface_options(...) ← builds FULL parent surface + │ .with_mcp_tools(Arc::clone(pool)) ← clones ENTIRE MCP pool + │ .build(context) + │ + │ 2. Stores allowed_tools filter (Option>) + │ └ No disallowed_tools field ❌ [GAP] + │ + │ 3. The child's SubAgentToolRegistry now operates: + │ - is_tool_allowed(name) → checks allowlist only + │ - tools_for_model() → filters model catalog by allowlist + posture + │ - execute(name, input) → checks allowlist + posture + approval + │ └ None of these check a deny list ❌ [GAP] +``` + +**The Gap Summary:** At **five** distinct points in the spawn flow, `disallowed_tools` is absent: +1. `SpawnRequest` struct (line 1247) — no field +2. `parse_spawn_request()` (line 5543) — doesn't extract it from JSON +3. `SubAgentRuntime` (line 1440) — no field, no `with_disallowed_tools()` builder +4. `build_allowed_tools()` (line 6791) — no parameter +5. `SubAgentToolRegistry` (line 6538) — no field, no `is_tool_denied()` method + +### 4.2 Tool Registry and Gate Chain + +**Parent side (works correctly):** +``` +EngineConfig.disallowed_tools [engine.rs:356] + │ + ├─→ filter_tool_catalog_for_gates() [engine.rs:3906] + │ └ Filters model-visible catalog: deny wins + │ + └─→ command_denies_tool() [turn_loop.rs:1608] + └ Blocks at execution time: deny wins +``` + +**Fleet headless workers (works correctly):** +``` +FleetExecConfig.disallowed_tools [config/src/lib.rs:1055] + │ + └─→ build_worker_exec_command_from_prompt() [fleet/executor.rs:94-97] + └ Serialized to --disallowed-tools CLI arg + │ + └─→ Worker picks up into its EngineConfig [verified] +``` + +**Sub-agent side (broken):** +``` +EngineConfig.disallowed_tools [engine.rs:356] + │ + └─→ ??? [NO CONNECTION] ??? [GAP] + └ SubAgentRuntime has no field + └ SubAgentToolRegistry has no field + └ Child tools_for_model() includes denied tools + └ Child execute() allows denied tools +``` + +### 4.3 Custom Role and allowed_tools + +**`SubAgentType` enum** (`subagent/mod.rs:367-389`): +Seven variants: `General` (default), `Explore`, `Plan`, `Review`, `Implementer`, `Verifier`, `Custom`. + +**`Custom` is special:** It starts locked down — `WorkerRuntimeProfile::for_role(Custom)` returns `PermissionSet::read_only()` + `ShellPolicy::None` (worker_profile.rs:164). The caller must provide an explicit `allowed_tools` list. + +**`build_allowed_tools()`** (line 6791): +- Non-Custom + no explicit list → `None` (full parent inheritance) +- Non-Custom + explicit list → `Some(deduped_list)` +- Custom + non-empty list → `Some(deduped_list)` +- Custom + None or empty → **error** ("Custom sub-agent requires a non-empty allowed_tools list") + +**Per-type deprecated lists** (line 437-446): The old `SubAgentType::allowed_tools()` method that returned per-type static lists (e.g., `Explore` got `[list_dir, read_file, grep_files, ...]`) is `#[deprecated since = "0.6.6"]`. Children now inherit the full parent registry. + +**`allowed_tools` on the model-facing `agent` tool:** +The `agent` tool's JSON schema already accepts `allowed_tools` (array of strings). This is parsed in `parse_spawn_request()` at line 5602. It flows into `build_allowed_tools()` and ultimately into `SubAgentToolRegistry.allowed_tools`. + +**What's missing:** There's **no** `disallowed_tools` field on the `agent` tool schema, no parsing for it, and no mechanism to express a deny list at the spawn call site. The `Custom` role requires an allowlist but cannot express a deny list. + +### 4.4 MCP Server Registration and Tool Exposure + +**MCP tool naming convention:** `mcp_{server}_{tool}` (confirmed: `crates/tui/src/mcp.rs:1696`). + +**`McpPool` struct** (`crates/tui/src/mcp.rs:1454`): +```rust +pub struct McpPool { + connections: HashMap, + config: McpConfig, + // ... +} +``` + +**`McpPool::all_tools()`** (line 1688): +```rust +pub fn all_tools(&self) -> Vec<(String, &McpTool)> { + for (server, conn) in &self.connections { + for tool in conn.tools() { + if !conn.config().is_tool_enabled(&tool.name) { continue; } + tools.push((format!("mcp_{}_{}", server, tool.name), tool)); + } + } +} +``` + +Each MCP server connection has its own `enabled_tools` / `disabled_tools` lists (per-server config in `mcp.json`), checked via `is_tool_enabled()` (line 367). This is **per-server per-tool**, not cross-server. + +**How MCP tools reach sub-agents:** +1. Engine creates `McpPool` once, wraps in `Arc>` +2. Passed to `SubAgentRuntime::with_mcp_pool(pool)` → stored on runtime +3. `SubAgentToolRegistry::new_with_owner()` clones the Arc and calls `with_mcp_tools(Arc::clone(pool))` (line 6604-6606) +4. `ToolRegistryBuilder::with_mcp_tools()` snapshots `pool.all_tools()` and wraps each as `McpToolAdapter` + +**The MCP scoping gap:** +- The **entire** MCP pool is cloned into every sub-agent +- There is **no server-level filtering** — all connected MCP servers are exposed to all sub-agents +- There is **no `McpPool::filtered_view(&["server1", "server2"])`** method +- Phase 3 proposes adding `mcp_servers: Vec` to scope which servers are visible + +### 4.5 Fleet Worker Integration + +**`FleetExecConfig`** (`crates/config/src/lib.rs:1050`): +```rust +pub struct FleetExecConfig { + pub allowed_tools: Vec, + pub disallowed_tools: Vec, + pub max_turns: u32, + pub max_spawn_depth: u32, + pub append_system_prompt: String, + pub output_format: String, +} +``` + +**How it reaches workers:** +1. `FleetExecConfig` serialized to `--disallowed-tools` CLI arg in `fleet/executor.rs:94-97` +2. Worker process picks it up into its own `EngineConfig.disallowed_tools` +3. **Note:** The worker's `allow_shell` is set to `false` by default and is **not** derived from `FleetExecConfig` + +**`FleetProfile`** (line 1109): has `slot`, `role`, `loadout`, `model`, `permissions` (allow_shell, trust, approval_required), `delegation` hints. **No tool-scoping fields** beyond `allow_shell`. + +**`FleetRolePreset`** (line 1384): has `tool_profile`, `tools`, `capabilities`. **No disallowed_tools field.** + +**The Fleet-to-sub-agent gap:** +A Fleet worker runs as `codewhale exec` — it gets its own `EngineConfig` with `disallowed_tools`. If that worker spawns an in-process sub-agent (via the `agent` tool), the deny list is **still lost** — same Phase 1 gap. The Phase 1 fix (threading `EngineConfig.disallowed_tools` through `SubAgentRuntime`) closes this for ALL in-process children, whether the parent is interactive or a Fleet worker. + +--- + +## Section 5: Implementation Considerations + +### Phase 1: Fix Sub-Agent Inheritance (Immediate) + +**Problem:** `--disallowed-tools exec_shell,write_file` on the parent → sub-agents see all tools. The parent's deny list is silently dropped. + +**Files that need changes (from the plan):** +| File | Change | +|---|---| +| `crates/tui/src/tools/subagent/mod.rs` | `SpawnRequest` + `disallowed_tools`/`inherit_disallowed_tools` fields; `SubAgentToolRegistry` + `disallowed_tools` field + `is_tool_denied()`; update `is_tool_allowed()` + `tools_for_model()` + `execute()` | +| `crates/tui/src/core/engine.rs` | Thread `self.config.disallowed_tools` into `SubAgentRuntime` via new `with_disallowed_tools()` builder | +| `crates/tui/src/tools/subagent/tests.rs` | Tests for deny inheritance, deny-wins-over-allow, wildcard, opt-out flag | + +**Approach A: Thread `disallowed_tools` through SubAgentRuntime (recommended by the plan)** + +This follows the existing pattern — `SubAgentRuntime` already carries `allow_shell`, `agent_tool_surface_options`, `mcp_pool`, etc. via builder methods. Adding `disallowed_tools` is consistent. + +1. Add `disallowed_tools: Option>` to `SubAgentRuntime` (line 1440) +2. Add `with_disallowed_tools(mut self, tools: Option>) -> Self` builder +3. In `child_runtime()` (line 1702), clone the field: `disallowed_tools: self.disallowed_tools.clone()` +4. In engine construction (lines 1451, 2532), add `.with_disallowed_tools(self.config.disallowed_tools.clone())` +5. Add `disallowed_tools: Option>` to `SpawnRequest` (line 1247) +6. Add `inherit_disallowed_tools: bool` (default `true`) to `SpawnRequest` +7. In `parse_spawn_request()` (line 5543), parse `disallowed_tools` from JSON input +8. Add `disallowed_tools: Vec` to `SubAgentToolRegistry` (line 6538) +9. Add `is_tool_denied(&self, name: &str) -> bool` — exact + `*` wildcard matching (reuse `command_denies_tool` logic) +10. Modify `is_tool_allowed()` (line 6662): check deny list FIRST, then allow list ("deny wins") +11. Modify `tools_for_model()` (line 6672): filter out denied tools from API catalog +12. Modify `execute()` (line 6703): check deny list before execution + +**Rust-specific considerations for Phase 1:** +- `disallowed_tools` should be `Vec` on `SubAgentToolRegistry` (owned, not borrowed) because the registry outlives the runtime +- `command_denies_tool()` is in `crates/tui/src/core/engine/turn_loop.rs` — either move it to a shared utility module or duplicate the logic in the subagent module +- The `is_tool_denied` check at line 6662 (where `is_tool_allowed` is called) is synchronous — no async complications +- `tools_for_model()` builds the model-visible catalog; filtering there ensures the model never even sees denied tools (defense in depth) + +**Pros of this approach:** +- Consistent with existing patterns (builder, field threading) +- Follows principle of least surprise: deny lists are a security boundary +- The `inherit_disallowed_tools: false` flag preserves opt-out flexibility + +**Risks:** +- If `command_denies_tool` logic is duplicated instead of shared, future wildcard enhancements could drift +- `parse_spawn_request` already handles many optional fields (line 5543-5696); adding more increases the parsing surface + +--- + +### Phase 2: Per-Sub-Agent Tool Scoping + +**Problem (builds on Phase 1):** The `agent` tool only supports `allowed_tools` for `Custom` roles. There's no caller-facing `disallowed_tools` parameter, and no way to attach tool restrictions to Fleet profiles. + +**Files that need changes:** +| File | Change | +|---|---| +| `crates/tui/src/tools/subagent/mod.rs` | `SpawnRequest` already gets `disallowed_tools` in Phase 1 — Phase 2 adds parsing from the `agent` tool schema | +| `crates/tui/src/tools/subagent/mod.rs` | `agent` tool schema — add `disallowed_tools` to the JSON input schema | +| `crates/tui/src/worker_profile.rs` | Add `denied_tools: Vec` to `WorkerRuntimeProfile` | +| `crates/config/src/lib.rs` | Add `allowed_tools`/`disallowed_tools` to `FleetProfile` and `FleetRolePreset` | + +**Key design decisions:** + +**Q1: `allowed_tools` vs `disallowed_tools` vs both?** + +The recommendation is **both**: `allowed_tools` for positive scoping (narrow roles like "you can only use read_file and grep_files"), `disallowed_tools` for negative scoping ("you inherit everything except exec_shell"). They're orthogonal. The `Custom` role already requires an allowlist; Phase 2 extends other roles to optionally accept either or both. + +**Q2: How to integrate with `WorkerRuntimeProfile`?** + +The plan says: add a **separate** `denied_tools: Vec` field to `WorkerRuntimeProfile` rather than overloading the `ToolScope` enum. This is cleaner because: +- Deny semantics (union of parent + child) are different from allow semantics (intersection) +- `derive_child()` already handles allowlist intersection — a separate field for deny lists makes the merge logic obvious +- Backward compatible: existing profiles without the field default to empty deny list + +```rust +// Proposed addition to worker_profile.rs: +pub struct WorkerRuntimeProfile { + // ... existing fields ... + pub denied_tools: Vec, // NEW +} +``` + +**Configuration surface:** + +For Fleet profiles, the user sets them in `config.toml`: +```toml +[fleet.profiles.code-reviewer] +role = "reviewer" +allowed_tools = ["read_file", "grep_files", "file_search"] +disallowed_tools = ["exec_shell"] + +[fleet.profiles.worker] +role = "builder" +disallowed_tools = ["mcp_production_*"] +``` + +For the `agent` tool call, the model passes: +```json +{ + "action": "start", + "type": "general", + "prompt": "...", + "disallowed_tools": ["exec_shell", "mcp_database_*"] +} +``` + +**Rust-specific considerations for Phase 2:** +- Adding fields to `WorkerRuntimeProfile` is straightforward — it's `Clone + Serialize + Deserialize` +- `derive_child()` union semantics for deny lists: `child_denied = parent.denied_tools ∪ child.denied_tools` (both apply) +- The `WorkerRuntimeProfile` is already threaded through the spawn flow (line 2773 in `spawn_background_with_assignment_options`) — no new threading needed + +--- + +### Phase 3: Per-Sub-Agent MCP Server Assignment + +**Problem (builds on Phase 1 & 2):** Every sub-agent gets the entire MCP pool. There's no way to say "this sub-agent can use the `github` MCP server but not `production_database`." + +**This is a separate concern from tool deny-lists.** Phase 1's `disallowed_tools: ["mcp_production_*"]` already works as coarse MCP denial through the naming convention. Phase 3 adds **positive server-level allowlisting** as a dedicated mechanism. + +**Files that need changes:** +| File | Change | +|---|---| +| `crates/tui/src/tools/subagent/mod.rs` | `SpawnRequest` + `mcp_servers` field; `SubAgentToolRegistry` — scoped MCP registration | +| `crates/tui/src/mcp.rs` | `McpPool::filtered_view(&[String])` — returns a subset of connections | +| `crates/tui/src/tools/registry.rs` | `with_mcp_tools_scoped()` variant that accepts a filtered pool view | +| `crates/config/src/lib.rs` | Add `mcp_servers` to `FleetProfile` | + +**Approach options:** + +**Option A: ScopedMcpPool — create a filtered wrapper** + +```rust +// New type in mcp.rs +pub struct ScopedMcpPool { + pool: Arc>, + allowed_servers: Vec, +} + +impl ScopedMcpPool { + pub fn all_tools(&self) -> Vec<(String, &McpTool)> { + // Only iterates connections in allowed_servers + } +} +``` + +Pro: Clean separation of concerns. Con: New type to maintain, and the locked pool snapshot pattern needs to be replicated. + +**Option B: McpPool::filtered_view() — add a method to McpPool** + +```rust +impl McpPool { + pub fn filtered_view(&self, servers: &[String]) -> Vec<(String, &McpTool)> { + self.connections + .iter() + .filter(|(name, _)| servers.contains(name)) + .flat_map(|(server, conn)| { + conn.tools().iter() + .filter(|tool| conn.config().is_tool_enabled(&tool.name)) + .map(|tool| (format!("mcp_{}_{}", server, tool.name), tool)) + }) + .collect() + } +} +``` + +Pro: Simpler, no new type. Con: Callers must remember to use `filtered_view` instead of `all_tools`; the pool itself has no "current scope" concept. + +**Integration with the spawn flow:** +1. `SpawnRequest` gains `mcp_servers: Option>` +2. In `SubAgentToolRegistry::new_with_owner()`, instead of `pool.all_tools()`, call `pool.filtered_view(&mcp_servers)` when `mcp_servers` is `Some` +3. If `mcp_servers` is `None`, inherit the full pool (current behavior) + +**Rust-specific considerations for Phase 3:** +- `McpPool` is behind `Arc>` — snapshotting for `filtered_view` needs the lock (same as current `all_tools`) +- The lifecycle: MCP tools are snapshot at registry build time, not dynamically re-scanned — consistent with current behavior +- Error handling: if `mcp_servers` references a server that doesn't exist, it should be a warning, not a hard error (servers may be temporarily disconnected) + +--- + +## Section 6: Open Questions and Recommendations + +### 6.1 Open Design Questions (from the plan) + +**Q1: Should sub-agents inherit `--disallowed-tools` by default or opt-in?** + +**Recommendation: Inherit by default, with opt-out.** The parent's deny list is a security boundary — escaping it via sub-agent spawn is privilege escalation. The `WorkerRuntimeProfile::derive_child()` precedent already establishes capability intersection as the safe default. + +The `inherit_disallowed_tools: false` flag on the `agent` call allows a parent to explicitly spawn a child with a clean slate (e.g., a sandboxed worker with its own policy). The flag only controls the *parent runtime's* deny list — any explicit `disallowed_tools` on the `agent` call itself always applies. + +**Q2: Should MCP scoping be part of custom role or a separate mechanism?** + +**Recommendation: Separate mechanism.** The `mcp_servers` field operates on server names; `allowed_tools`/`disallowed_tools` operate on tool names. Mixing them creates ambiguity: does `allowed_tools: ["mcp_github_*"]` mean "allow the github server" or "allow the literal tool named `mcp_github_*`"? Phase 3's `mcp_servers` is the positive allowlist for servers; Phase 1's `disallowed_tools: ["mcp_database_*"]` is the negative path through the naming convention. Both are useful and orthogonal. + +**Q3: How should Fleet workers interact with sub-agent tool restrictions?** + +Three separate sub-concerns, none of which are MCP-specific: +1. **Headless Fleet CLI path** (already works): `FleetExecConfig.disallowed_tools` → `--disallowed-tools` CLI arg → worker's `EngineConfig` +2. **In-process sub-agents from Fleet workers** (Phase 1 fix): The worker's `EngineConfig.disallowed_tools` threads into `SubAgentRuntime` — same Phase 1 inheritance chain. Covers ALL tools. +3. **Fleet profile resolution** (Phase 2): `FleetProfile.allowed_tools`/`disallowed_tools` resolve into `FleetExecConfig`, feeding the headless CLI path. Covers ALL tools. + +**Q4: `ToolScope` representation?** + +**Recommendation: Add separate `denied_tools: Vec` to `WorkerRuntimeProfile`** rather than overloading the `ToolScope` enum. This keeps allow/deny semantics clear (deny always wins), is backward-compatible, and the `derive_child()` union semantics (parent deny + child deny both apply) are simpler than nested enum variants. + +### 6.2 Risk Assessment + +| Risk | Likelihood | Impact | Mitigation | +|---|---|---|---| +| Phase 1 breaks existing sub-agent behavior | Low | High | Comprehensive tests + backward-compatible defaults | +| Wildcard matching diverges between parent and child | Medium | Medium | Share `command_denies_tool` logic between `turn_loop.rs` and subagent module | +| Phase 2 `inherited` Fleet profiles work differently than direct profiles | Medium | Medium | Clear documentation; the `ToolScope::Inherited` → cannot filter comment in `filter_tool_profile()` already flags this | +| Phase 3 scoped MCP pool breaks existing MCP tool registration | Low | Medium | Keep `None` mcp_servers → full pool as the default | + +### 6.3 Testing Strategy Summary + +**Phase 1 tests (minimum):** +- Parent with `--disallowed-tools exec_shell` → child `tools_for_model()` excludes `exec_shell` +- Parent disallow + child explicit allow → deny wins +- Wildcard deny `mcp_*` → all MCP tools excluded from child catalog +- `inherit_disallowed_tools: false` → parent deny list not applied, but caller deny list is +- Confirm existing tests pass unchanged + +**Phase 2 tests:** +- `agent` call with `disallowed_tools` → child's `is_tool_allowed` rejects denied tools +- `FleetProfile` with `allowed_tools` + `disallowed_tools` → `FleetExecConfig` has both +- `derive_child()` with parent `denied_tools` + child `denied_tools` → union (both apply) + +**Phase 3 tests:** +- Sub-agent with `mcp_servers: ["github"]` → only github MCP tools visible +- Sub-agent with `mcp_servers: []` → no MCP tools visible +- Sub-agent without `mcp_servers` → full pool (backward compatible) + +### 6.4 PR Structure Recommendation + +The plan explicitly states these are separate workstreams that must ship as separate commits/PRs: + +``` +PR 1: Phase 1 — Thread disallowed_tools through SubAgentRuntime → SubAgentToolRegistry +PR 2: Phase 2 — Per-sub-agent allowed_tools/disallowed_tools on agent call + Fleet profiles +PR 3: Phase 3 — mcp_servers field for per-server MCP allowlisting +``` + +Each PR should be: rebased onto `main`, focused on one behavior boundary, backed by tests. + +--- + +## Quick Reference: Key Files + +| File | What's There | +|---|---| +| `crates/tui/src/tools/subagent/mod.rs` | **The sub-agent system:** `SubAgentType`, `SubAgentRuntime`, `SubAgentToolRegistry`, `parse_spawn_request`, `spawn_subagent_from_input`, `spawn_background_with_assignment_options`, `build_allowed_tools`, `run_subagent` | +| `crates/tui/src/core/engine.rs` | **The engine:** `EngineConfig` (with `disallowed_tools`), `filter_tool_catalog_for_gates`, SubAgentRuntime construction sites | +| `crates/tui/src/core/engine/turn_loop.rs` | **The turn loop:** `command_denies_tool`, `command_allows_tool`, tool execution gating | +| `crates/tui/src/worker_profile.rs` | **Worker capability contracts:** `WorkerRuntimeProfile`, `derive_child()`, `ToolScope`, `ShellPolicy`, `PermissionSet` | +| `crates/tui/src/tools/registry.rs` | **Tool registry builder:** `ToolRegistryBuilder`, `with_mcp_tools`, `with_full_agent_surface_options` | +| `crates/tui/src/mcp.rs` | **MCP integration:** `McpPool`, `all_tools()`, `is_tool_enabled()`, MCP naming convention | +| `crates/config/src/lib.rs` | **Config types:** `FleetExecConfig`, `FleetProfile`, `FleetRolePreset` | +| `crates/tui/src/fleet/worker_runtime.rs` | **Fleet hardening:** `apply_exec_hardening()`, `filter_tool_profile()` | +| `crates/tui/src/fleet/executor.rs` | **Fleet execution:** `build_worker_exec_command_from_prompt()` — serializes to CLI args | +| `crates/tools/src/lib.rs` | **Lean tool types:** `ToolSpec`, `ToolError`, `ApprovalRequirement` | + +--- + +*This guide was compiled from live code on the `main` branch, workspace version 0.8.66, plus the existing `SUBAGENT_TOOL_SCOPING_PLAN.md` (2026-07-07). All line numbers are accurate as of this writing but may shift with future changes.* diff --git a/docs/SUBAGENT_TOOL_SCOPING_PANL.md b/docs/SUBAGENT_TOOL_SCOPING_PANL.md new file mode 100644 index 000000000..4f7227f3f --- /dev/null +++ b/docs/SUBAGENT_TOOL_SCOPING_PANL.md @@ -0,0 +1,505 @@ +# Sub-Agent Tool Scoping: Phased Implementation Plan + +- **Date:** 2026-07-07 +- **Repo state:** `main`, workspace version 0.8.67 +- **Author:** CodeWhale architecture analysis (deepseek-v4-pro, thinking=high) + +## Separation of Concerns + +The plan addresses two **independent** workstreams that must ship as separate +commits/PRs: + +| Workstream | Phases | Scope | Mechanism | +|---|---|---|---| +| **Tool deny-list inheritance** | Phase 1, Phase 2 | **All tools** (native + MCP) | `disallowed_tools` field threaded through `SubAgentRuntime` → `SubAgentToolRegistry`; deny-list matching on tool *names* (including `mcp_*` wildcards) | +| **MCP server assignment** | Phase 3 | **MCP tools only** | `mcp_servers` field on `agent` tool + Fleet profiles; server-level positive allowlisting via a filtered pool view | + +The boundary: MCP tool names follow the convention `mcp_{server}_{tool}`. Phase +1's wildcard deny (`disallowed_tools: ["mcp_database_*"]`) already provides +coarse per-server denial through the naming convention — that's a *side effect* +of the naming scheme, not an MCP-specific feature. Phase 3's `mcp_servers` field +is the dedicated, intentional mechanism for server-level scoping. + +--- + +## Current State (Evidence Summary) + +### 1. Sub-agent spawn flow — where tool registry inheritance happens + +**Entry point:** `spawn_subagent_from_input()` at +`crates/tui/src/tools/subagent/mod.rs:3852` + +The flow is: + +1. `parse_spawn_request(input)` → `SpawnRequest` (line 5543), which extracts + `allowed_tools: Option>` (line 5602) but **never extracts a + `disallowed_tools` field** — it does not exist on `SpawnRequest`. +2. `spawn_background_with_assignment_options()` (line 2681) calls + `build_allowed_tools(agent_type, allowed_tools, allow_shell)` (line 2713). +3. `build_allowed_tools()` (line 6791) returns `None` for non-Custom types + (full parent inheritance) or `Some(vec)` for explicit lists. +4. The result is passed into `SubAgentToolRegistry::new_with_owner()` (line + 6577), which builds the full child tool registry via + `ToolRegistryBuilder::new().with_full_agent_surface_options(...)` — identical + to the parent surface (minus `agent` if depth is exhausted). +5. MCP pool is inherited wholesale: `registry.with_mcp_tools(Arc::clone(pool))` + (line 6604-6606). + +**Gap:** `disallowed_tools` does not appear in any of `SpawnRequest`, +`build_allowed_tools()`, `SubAgentToolRegistry`, or the spawn pipeline. The +parent's deny list is silently dropped. + +### 2. The `--disallowed-tools` gate chain + +**Parent side** (works correctly): + +- `EngineConfig.disallowed_tools: Option>` — + `crates/tui/src/core/engine.rs:356` +- Applied at turn-loop tool execution time: + `command_denies_tool(self.config.disallowed_tools.as_deref(), &tool_name)` — + `turn_loop.rs:1608` +- Applied to the model-visible catalog: + `filter_tool_catalog_for_gates()` — `engine.rs:3906` +- `command_denies_tool()` at `turn_loop.rs:3045` supports exact match, + case-insensitive, and `prefix*` wildcards. + +**Fleet side** (works correctly for headless workers): + +- `FleetExecConfig.disallowed_tools` serialized to `--disallowed-tools` CLI arg + in `fleet/executor.rs:94-97` +- The worker process picks it up into its own `EngineConfig`. + +**Sub-agent side** (broken): No path from `EngineConfig.disallowed_tools` into +the child `SubAgentRuntime` or `SubAgentToolRegistry`. + +### 3. Custom role and `allowed_tools` mechanism + +**`SubAgentType::Custom`** (line 387): requires an explicit `allowed_tools` array +from the caller. `build_allowed_tools()` (line 6791) rejects `Custom` without +one. + +**Per-type allowlists deprecated** since v0.6.6 (line 439-446). The +`SubAgentType::allowed_tools()` method is `#[deprecated]` — children now inherit +the full parent registry. + +**Runtime filtering** lives in `SubAgentToolRegistry`: + +- `is_tool_allowed(name)` — line 6662: checks the `allowed_tools: Option>` filter +- `tools_for_model()` — line 6672: filters the API catalog sent to the model +- `posture_permits_tool()` — line 6637: role-posture enforcement +- `execute()` — line 6703: combines allowlist + posture + approval gating + +**No equivalent for `disallowed_tools`** exists anywhere in this chain. + +### 4. MCP server registration and tool exposure + +**MCP tool naming convention:** `mcp_{server}_{tool}` — +`crates/tui/src/mcp.rs:1696` + +**`McpPool.all_tools()`** (line 1688) iterates all connected servers, filters by +per-tool enable flags, returns `Vec<(String, &McpTool)>` with server-prefixed +names. + +**Sub-agent inheritance:** The entire pool is cloned (`Arc::clone(pool)`) and +registered via `registry.with_mcp_tools(pool)` at `subagent/mod.rs:6604-6606`. +There is **no server-level filtering** — every connected MCP server is exposed +to every sub-agent. + +### 5. Fleet worker configuration and sub-agent interaction + +**`FleetExecConfig`** (`config/src/lib.rs:1050`): has `allowed_tools`, +`disallowed_tools`, `max_turns`, `max_spawn_depth`, `append_system_prompt`, +`output_format`. + +**`apply_exec_hardening()`** (`fleet/worker_runtime.rs:441`): applies +`FleetExecConfig` to an `AgentWorkerSpec` — caps steps, filters tool profile via +`filter_tool_profile()` (line 475), appends system prompt. + +**Fleet profiles** (`FleetProfile`, line 1109): have `slot`, `role`, `loadout`, +`model`, `permissions` (allow_shell, trust, approval_required), `delegation` +hints. **No tool-scoping fields** except `allow_shell`. + +**`FleetRolePreset`** (line 1384): has `tool_profile` +(read-only/read-write/custom), `tools` (explicit tool list), `capabilities`. +**No `disallowed_tools` field.** + +**Fleet-to-sub-agent bridge:** Fleet workers run as `codewhale exec` +subprocesses with CLI args. The in-process sub-agent path (`agent` tool) has no +equivalent bridge to `FleetExecConfig`. + +### 6. Worker profile infrastructure (already in place) + +**`WorkerRuntimeProfile`** (`crates/tui/src/worker_profile.rs:128`): capability +contract with: + +- `permissions: PermissionSet` (write + network booleans) +- `shell: ShellPolicy` (None / ReadOnly / Full) +- `tools: ToolScope` (Inherit / Explicit(Vec\)) +- `model: ModelRoute`, `max_spawn_depth`, `background` + +**`derive_child()`** (line 191): already computes parent × child intersection +for permissions, shell, tools, and depth — the non-escalation primitive. **But +`ToolScope` only has `Inherit` and `Explicit(allowlist)` — no deny-list +representation.** + +**Integration note (v0.8.67):** `SubAgentRuntime` now carries +`worker_profile: WorkerRuntimeProfile` (line 1456), and +`SubAgentToolRegistry` reads it via `new_with_owner()` at line 6614 to +set `runtime_profile`. A deny-list added to `WorkerRuntimeProfile` +would automatically flow through the existing spawn pipeline without +a separate `with_disallowed_tools()` builder on `SubAgentRuntime`. + +--- + +## Phase 1: Sub-agent `disallowed_tools` inheritance (Immediate) + +### Problem + +When `--disallowed-tools exec_shell,write_file` is set on the parent, sub-agents +receive the full tool registry with no filtering. The deny list is silently +dropped. + +### Implementation approach + +#### 1a. Add `disallowed_tools` to `SpawnRequest` + +**File:** `crates/tui/src/tools/subagent/mod.rs` + +```rust +struct SpawnRequest { + // ... existing fields unchanged ... + allowed_tools: Option>, + /// Tool deny-list from the parent runtime, merged with any caller request. + /// Deny always wins over allow. + disallowed_tools: Option>, // NEW + /// When true (default), the child inherits the parent runtime's + /// disallowed_tools. Set false to start with a clean slate. + inherit_disallowed_tools: bool, // NEW, default true +} +``` + +#### 1b. Plumb through the spawn pipeline + +1. `spawn_subagent_from_input()` (line 3852) — pass + `spawn_request.disallowed_tools` and `inherit_disallowed_tools` into + `spawn_background_with_assignment_options()` +2. `spawn_background_with_assignment_options()` signature (line 2681) — add + `disallowed_tools: Option>` parameter +3. Pass into `SubAgentTask` (line 2813) and then into `run_subagent_task()` +4. Pass into `SubAgentToolRegistry::new_with_owner()` (line 6577) + +#### 1c. Add deny-list filtering to `SubAgentToolRegistry` + +Add `disallowed_tools: Vec` field to the struct. Add method: + +```rust +fn is_tool_denied(&self, name: &str) -> bool { + let tool_name = name.to_ascii_lowercase(); + self.disallowed_tools.iter().any(|rule| { + let rule = rule.to_ascii_lowercase(); + if let Some(prefix) = rule.strip_suffix('*') { + tool_name.starts_with(prefix) + } else { + rule == tool_name + } + }) +} +``` + +Modify `is_tool_allowed()` (line 6662) — check deny list first: + +```rust +fn is_tool_allowed(&self, name: &str) -> bool { + if name == "agent" && !self.can_spawn_child { return false; } + if self.is_tool_denied(name) { return false; } // NEW + match &self.allowed_tools { + None => true, + Some(list) => list.iter().any(|t| t == name), + } +} +``` + +Modify `tools_for_model()` (line 6672) — filter denied tools from the +model-visible catalog. + +#### 1d. Thread `disallowed_tools` from parent engine into child runtime + +**File:** `crates/tui/src/core/engine.rs` + +Add `disallowed_tools: Option>` to `SubAgentRuntime` with a builder: + +```rust +pub fn with_disallowed_tools(mut self, tools: Option>) -> Self { + self.disallowed_tools = tools; + self +} +``` + +At the two engine construction sites (lines 1451 and 2532), add: + +```rust +.with_disallowed_tools(self.config.disallowed_tools.clone()) +``` + +The `child_runtime()` and `background_runtime()` methods must propagate +`disallowed_tools`. + +#### 1e. Merge runtime + caller deny lists + +In `spawn_subagent_from_input()`, after building `SpawnRequest`, merge: + +```rust +let effective_disallowed = merge_deny_lists( + spawn_request.inherit_disallowed_tools.then(|| runtime.disallowed_tools.clone()).flatten(), + spawn_request.disallowed_tools, +); +``` + +Union logic: parent runtime deny list AND caller deny list both apply. Neither +can remove the other's entries. The `inherit_disallowed_tools: false` flag +allows dropping only the *parent runtime's* list — explicit caller +`disallowed_tools` always applies. + +### Files touched + +| File | Change | +|------|--------| +| `crates/tui/src/tools/subagent/mod.rs` | `SpawnRequest` + fields, `SubAgentTask` + field, `SubAgentToolRegistry` + field + `is_tool_denied()`, updates to `is_tool_allowed()` and `tools_for_model()`, `spawn_background_with_assignment_options` signature, `parse_spawn_request` | +| `crates/tui/src/core/engine.rs` | Thread `self.config.disallowed_tools` into `SubAgentRuntime`; add `with_disallowed_tools()` builder | +| `crates/tui/src/tools/subagent/tests.rs` | Tests for deny-list inheritance, deny-wins-over-allow, wildcard matching, opt-out flag, `tools_for_model()` catalog filtering | + +### Verification + +- New test: parent with `--disallowed-tools exec_shell` → child `tools_for_model()` excludes `exec_shell` +- New test: parent disallow + child explicit allow → deny wins +- New test: wildcard deny `mcp_*` → all MCP tools excluded from child catalog +- New test: `inherit_disallowed_tools: false` → parent deny list not applied, but caller deny list is +- Confirm existing tests pass unchanged + +--- + +## Phase 2: Per-sub-agent tool scoping (Builds on Phase 1) + +### Problem + +Currently, `allowed_tools` on the model-facing `agent` tool only affects +`Custom` roles. There's no caller-facing way to scope tools for non-Custom +roles, express a deny list at the spawn call site, or attach tool restrictions to +Fleet profiles. + +### Implementation approach + +#### 2a. Extend `agent` tool schema to accept `disallowed_tools` + +Add `disallowed_tools` to the JSON schema of the `agent` tool and parse it in +`parse_spawn_request()` alongside `allowed_tools`. The model can express "spawn +an agent that can do anything except X." + +Also parse `inherit_disallowed_tools: true/false`. + +#### 2b. Allow `allowed_tools` for non-Custom roles + +Change `build_allowed_tools()` logic: + +- If `allowed_tools` is provided and non-empty → use it for **any** role type + (not just `Custom`) +- If `allowed_tools` is `None` or empty for `Custom` → error as before +- If `allowed_tools` is `None` for non-`Custom` roles → full inheritance as + before + +#### 2c. Add tool scoping fields to Fleet profiles + +**File:** `crates/config/src/lib.rs` + +Add to `FleetProfile`: + +```rust +pub struct FleetProfile { + // ... existing fields unchanged ... + /// Optional tool allowlist for workers using this profile. + /// Applies to ALL tools — native and MCP. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub allowed_tools: Vec, + /// Optional tool deny-list for workers using this profile. + /// Applies to ALL tools — native and MCP. Deny wins. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub disallowed_tools: Vec, +} +``` + +Resolution path: `FleetProfile.allowed_tools`/`disallowed_tools` → merge into +`FleetExecConfig` → CLI args → worker's `EngineConfig`. This covers **all** +tools — MCP tool names like `mcp_github_*` are just strings in the list. + +#### 2d. Add `denied_tools` to `WorkerRuntimeProfile` + +**File:** `crates/tui/src/worker_profile.rs` + +Add alongside existing `tools: ToolScope`: + +```rust +pub struct WorkerRuntimeProfile { + // ... existing fields unchanged ... + /// Tools explicitly denied to this worker. Deny always wins over allow. + /// Merged (union) during parent→child derivation. + pub denied_tools: Vec, +} +``` + +Update `derive_child()` to union deny lists (parent deny + child deny both +apply). + +### Fleet workers and sub-agent tool restrictions — three separate concerns + +| Concern | Status | Fixed by | +|---|---|---| +| Headless Fleet worker gets `--disallowed-tools` | Already works | `build_worker_exec_command()` passes CLI args → worker's own `EngineConfig` | +| In-process sub-agent spawned FROM a Fleet worker doesn't inherit | Broken | Phase 1: `EngineConfig.disallowed_tools` → `SubAgentRuntime` → child registry | +| Fleet profiles don't carry tool restrictions | Not implemented | Phase 2c: `FleetProfile.allowed_tools`/`disallowed_tools` → `FleetExecConfig` | + +All three cover **all** tools — MCP tool names are just strings. The +MCP-specific `mcp_servers` field is Phase 3. + +### Files touched + +| File | Change | +|------|--------| +| `crates/tui/src/tools/subagent/mod.rs` | `parse_spawn_request` — parse `disallowed_tools`, `inherit_disallowed_tools`; `build_allowed_tools` — accept explicit lists for any role | +| `crates/config/src/lib.rs` | Add `allowed_tools`, `disallowed_tools` to `FleetProfile` | +| `crates/tui/src/worker_profile.rs` | Add `denied_tools: Vec` to `WorkerRuntimeProfile`; update `derive_child()` | +| `crates/tui/src/fleet/worker_runtime.rs` | Resolve profile tool scoping into `FleetExecConfig` | + +--- + +## Phase 3: Per-sub-agent MCP server assignment (Depends on Phase 1 + 2) + +### Problem + +All connected MCP servers are exposed to every sub-agent. There is no way to give +a sub-agent access to only the `github` MCP server but not `database`, or to +prevent a sub-agent from accessing any MCP tools at all. + +Note: Phase 1's `disallowed_tools: ["mcp_database_*"]` already provides coarse +per-server denial through the naming convention. Phase 3 adds the dedicated, +intentional mechanism for positive server-level allowlisting. + +### Implementation approach + +#### 3a. Add `mcp_servers` field to the `agent` tool schema + +```json +{ + "action": "start", + "prompt": "...", + "mcp_servers": ["github", "filesystem"] +} +``` + +Parse in `parse_spawn_request()` into `SpawnRequest`. `None` / absent = all +servers (current behavior). `Some(["github"])` = only the github server. + +#### 3b. Create a filtered MCP pool wrapper + +```rust +// crates/tui/src/mcp.rs (or subagent/mod.rs) +struct ScopedMcpPool { + inner: Arc>, + allowed_servers: Option>, // None = all servers +} +``` + +`ScopedMcpPool::tools()` delegates to `inner.all_tools()` but filters by server +prefix, only returning tools from allowed servers. + +#### 3c. Apply MCP scoping in `SubAgentToolRegistry::new_with_owner()` + +Replace the wholesale clone with a scoped variant: + +```rust +if let Some(pool) = runtime.mcp_pool.as_ref() { + let scoped = ScopedMcpPool::new(Arc::clone(pool), spawn_request.mcp_servers); + registry = registry.with_mcp_tools_scoped(scoped); +} +``` + +#### 3d. Add MCP server scoping to Fleet profiles + +```rust +pub struct FleetProfile { + // ... + /// MCP servers available to workers using this profile. + /// None/empty = all connected servers. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub mcp_servers: Vec, +} +``` + +### Files touched + +| File | Change | +|------|--------| +| `crates/tui/src/tools/subagent/mod.rs` | `SpawnRequest` + `mcp_servers` field; `SubAgentToolRegistry` — scoped MCP registration; `parse_spawn_request` — parse `mcp_servers` | +| `crates/tui/src/mcp.rs` | `ScopedMcpPool` or `McpPool::filtered_view()` | +| `crates/tui/src/tools/registry.rs` | `with_mcp_tools_scoped()` variant that accepts a filtered view | +| `crates/config/src/lib.rs` | Add `mcp_servers` to `FleetProfile` | + +--- + +## Open Design Questions + +### 1. Should sub-agents inherit `--disallowed-tools` by default, or opt-in? + +**Inherit by default, with an opt-out flag.** Rationale: the parent's deny list +is a security boundary — escaping it via sub-agent spawn is privilege +escalation. The `WorkerRuntimeProfile::derive_child()` precedent already +establishes capability intersection as the safe default. + +The `inherit_disallowed_tools: false` flag on the `agent` call allows a parent +to explicitly spawn a child with a clean slate (e.g., a sandboxed worker with +its own policy). The flag only controls the *parent runtime's* deny list — any +explicit `disallowed_tools` on the `agent` call itself always applies. + +### 2. Should MCP scoping be part of custom role or a separate mechanism? + +**Separate mechanism.** The `mcp_servers` field operates on server names; +`allowed_tools`/`disallowed_tools` operate on tool names. Mixing them creates +ambiguity: does `allowed_tools: ["mcp_github_*"]` mean "allow the github server" +or "allow the literal tool named `mcp_github_*`"? Phase 3's `mcp_servers` is the +positive allowlist for servers; Phase 1's `disallowed_tools: ["mcp_database_*"]` +is the negative path through the naming convention. Both are useful and +orthogonal. + +### 3. How should Fleet workers interact with sub-agent tool restrictions? + +Three separate sub-concerns, none of which are MCP-specific: + +- **Headless Fleet worker CLI path** (already works): + `FleetExecConfig.disallowed_tools` → `--disallowed-tools` CLI arg → worker's + `EngineConfig`. Covers **all** tools including MCP names. +- **In-process sub-agents spawned FROM Fleet workers** (Phase 1 fix): the + worker's `EngineConfig.disallowed_tools` threads into `SubAgentRuntime`, then + into child registries via Phase 1's inheritance chain. Covers **all** tools. +- **Fleet profile resolution** (Phase 2): `FleetProfile.allowed_tools` / + `disallowed_tools` resolve into `FleetExecConfig`, feeding the headless CLI + path. Covers **all** tools — not just MCP. + +Phase 3 adds `FleetProfile.mcp_servers` for the MCP-specific positive +allowlist, but that's a separate concern and a separate commit. + +### 4. `ToolScope` representation? + +**Add a separate `denied_tools: Vec` to `WorkerRuntimeProfile`** rather +than overloading the `ToolScope` enum. This keeps allow/deny semantics clear +(deny always wins), is backward-compatible, and the `derive_child()` union +semantics (parent deny + child deny both apply) are simpler than nested enum +variants. + +--- + +## Summary + +| Phase | Scope | Key deliverable | Ships with | +|---|---|---|---| +| 1 | **All tools** | `disallowed_tools` inherits to in-process sub-agents; deny-list enforcement in `SubAgentToolRegistry` | Phase 1 only | +| 2 | **All tools** | Per-sub-agent `allowed_tools`/`disallowed_tools` on `agent` call; Fleet profile tool fields; `WorkerRuntimeProfile.denied_tools` | Phase 2 only | +| 3 | **MCP only** | `mcp_servers` field for per-server positive allowlisting; `ScopedMcpPool`; Fleet profile `mcp_servers` | Phase 3 only |