diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 8de4c327..6bac69ad 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -3389,6 +3389,7 @@ async fn run_subagent( }); let tool_registry = SubAgentToolRegistry::new( runtime_for_tools, + agent_type.clone(), allowed_tools.clone(), Arc::new(Mutex::new(TodoList::new())), Arc::new(Mutex::new(PlanState::default())), @@ -4416,7 +4417,9 @@ fn emit_agent_progress( /// - **Full inheritance** (`allowed_tools = None`): the child sees the same /// tool surface as the parent's Agent mode — every tool family including /// `with_subagent_tools` (so it can recurse). Approval-gated tools are -/// callable only when the parent runtime is auto-approved. +/// callable only when the parent runtime is auto-approved or, for explicit +/// write-capable roles (`implementer`, `custom`), when the tool's approval +/// requirement is `Suggest`. /// - **Explicit narrow** (`allowed_tools = Some(list)`): legacy / Custom /// path. The registry still builds the full surface, but only the listed /// tool names are visible to the model and callable. @@ -4425,12 +4428,17 @@ struct SubAgentToolRegistry { /// only the listed tools are visible to the model and callable. allowed_tools: Option>, 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 + /// the child without the parent runtime being auto-approved (#1828, #1833). + agent_type: SubAgentType, registry: ToolRegistry, } impl SubAgentToolRegistry { fn new( runtime: SubAgentRuntime, + agent_type: SubAgentType, explicit_allowed_tools: Option>, todo_list: SharedTodoList, plan_state: SharedPlanState, @@ -4455,10 +4463,22 @@ impl SubAgentToolRegistry { Self { allowed_tools: explicit_allowed_tools, auto_approve: runtime.context.auto_approve, + agent_type, registry, } } + /// Whether this role is allowed to use `Suggest`-level tools (write_file, + /// edit_file, apply_patch, ...) without the parent runtime being + /// auto-approved. Read-only stances (`explore`, `plan`, `review`, + /// `verifier`) stay blocked so they can't quietly mutate the workspace + /// while a non-auto parent is delegating bounded investigation. + /// `Required`-level tools (shell, etc.) still need parent auto-approve + /// regardless of role (#1828, #1833). + fn role_can_delegate_writes(agent_type: &SubAgentType) -> bool { + matches!(agent_type, SubAgentType::Implementer | SubAgentType::Custom) + } + /// Whether a given tool name is permitted under this child's filter. /// `None` filter = everything permitted. fn is_tool_allowed(&self, name: &str) -> bool { @@ -4511,10 +4531,30 @@ impl SubAgentToolRegistry { let Some(spec) = self.registry.get(name) else { return Err(anyhow!("Tool {name} is not registered")); }; - if spec.approval_requirement() != ApprovalRequirement::Auto { - return Err(anyhow!( - "Tool {name} requires approval and cannot run inside this sub-agent unless the parent session is auto-approved" - )); + match spec.approval_requirement() { + ApprovalRequirement::Auto => {} + ApprovalRequirement::Suggest => { + // Write/edit/patch tools land here. Explicit + // write-capable roles (`implementer`, `custom`) may run them + // without parent auto-approve so that delegated work + // can actually land file changes; the previous + // behavior blocked every write under `suggest` mode + // even for the role explicitly chartered to write + // (#1828, #1833). Read-only roles still bounce so + // exploration/review/planning/verifier children + // can't mutate the workspace behind the parent's back. + if !Self::role_can_delegate_writes(&self.agent_type) { + return Err(anyhow!( + "Tool {name} requires approval and is not delegated to {role} sub-agents; rerun the parent with auto approval or pick a write-capable role", + role = self.agent_type.as_str() + )); + } + } + ApprovalRequirement::Required => { + return Err(anyhow!( + "Tool {name} requires approval and cannot run inside this sub-agent unless the parent session is auto-approved" + )); + } } } reject_subagent_terminal_takeover(name, &input)?; diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 3f4f13e3..54c7dd07 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -713,6 +713,7 @@ fn test_subagent_tool_registry_reports_unavailable_tools() { runtime.allow_shell = false; let registry = SubAgentToolRegistry::new( runtime, + SubAgentType::Explore, Some(vec!["read_file".to_string(), "missing_tool".to_string()]), Arc::new(Mutex::new(TodoList::new())), Arc::new(Mutex::new(PlanState::default())), @@ -731,6 +732,7 @@ fn test_review_agent_tools_exclude_agent_spawn() { // None = full parent tool inheritance (the default for builtin types). let registry = SubAgentToolRegistry::new( runtime, + SubAgentType::Review, None, Arc::new(Mutex::new(TodoList::new())), Arc::new(Mutex::new(PlanState::default())), @@ -1197,6 +1199,7 @@ async fn subagent_registry_blocks_approval_tools_without_parent_auto_approve() { runtime.context.auto_approve = false; let registry = SubAgentToolRegistry::new( runtime, + SubAgentType::General, Some(vec!["exec_shell".to_string()]), Arc::new(Mutex::new(TodoList::new())), Arc::new(Mutex::new(PlanState::default())), @@ -1213,6 +1216,173 @@ async fn subagent_registry_blocks_approval_tools_without_parent_auto_approve() { ); } +#[tokio::test] +async fn implementer_delegation_allows_suggest_write_without_parent_auto_approve() { + // Issue #1828: implementer agents could not write files even when their + // whole job is to land code changes, because the registry blocked every + // approval-gated tool when the parent ran in `suggest` mode. The + // hardened gate (#1833) delegates `Suggest`-level tools (write_file, + // edit_file, apply_patch) to write-capable roles. + let tmp = tempdir().expect("tempdir"); + let workspace = tmp.path().to_path_buf(); + let mut runtime = stub_runtime(); + runtime.context = ToolContext::new(workspace.clone()); + runtime.context.auto_approve = false; + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::Implementer, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + let result = registry + .execute( + "agent_test", + "write_file", + json!({"path": "delegated.txt", "content": "hello"}), + ) + .await + .expect("delegated write should be allowed for implementer"); + + let written = std::fs::read_to_string(workspace.join("delegated.txt")) + .expect("file should exist after delegated write"); + assert_eq!(written, "hello"); + assert!( + !result.contains("requires approval"), + "successful write should not look like an approval error: {result}" + ); +} + +#[tokio::test] +async fn general_delegation_still_blocks_suggest_write_without_parent_auto_approve() { + let tmp = tempdir().expect("tempdir"); + let workspace = tmp.path().to_path_buf(); + let mut runtime = stub_runtime(); + runtime.context = ToolContext::new(workspace.clone()); + runtime.context.auto_approve = false; + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::General, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + let err = registry + .execute( + "agent_test", + "write_file", + json!({"path": "general.txt", "content": "ok"}), + ) + .await + .expect_err("general agent should not silently gain write permission"); + let msg = err.to_string(); + assert!( + msg.contains("not delegated to general sub-agents"), + "general writes should be rejected with a role-aware message: {msg}" + ); + + assert!( + !workspace.join("general.txt").exists(), + "general write must not land without parent auto-approve" + ); +} + +#[tokio::test] +async fn explore_role_still_blocks_suggest_writes_without_parent_auto_approve() { + // Read-only stances (explore, plan, review, verifier) must not gain + // write capabilities via delegation — otherwise a parent that asked + // for "just look at the code" could find files mutated behind its back. + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime(); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.context.auto_approve = false; + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::Explore, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + let err = registry + .execute( + "agent_test", + "write_file", + json!({"path": "should_not_appear.txt", "content": "denied"}), + ) + .await + .expect_err("explore agents must not write"); + let msg = err.to_string(); + assert!( + msg.contains("not delegated to explore sub-agents"), + "explore writes should be rejected with a role-aware message: {msg}" + ); + assert!( + !tmp.path().join("should_not_appear.txt").exists(), + "file must not have been written" + ); +} + +#[tokio::test] +async fn delegated_write_role_still_blocks_required_tools() { + // Required-level tools (exec_shell, etc.) remain gated behind parent + // auto-approve regardless of role. Implementer can write files, but it + // still can't bypass shell approval just because it's a "write" role. + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime(); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.context.auto_approve = false; + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::Implementer, + Some(vec!["exec_shell".to_string()]), + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + let err = registry + .execute("agent_test", "exec_shell", json!({"command": "echo hi"})) + .await + .expect_err("Required-level shell must still need parent auto-approve"); + assert!( + err.to_string().contains( + "cannot run inside this sub-agent unless the parent session is auto-approved" + ), + "expected Required-level approval message, got: {err}" + ); +} + +#[tokio::test] +async fn auto_approved_parent_runs_required_tools_in_subagent() { + // Baseline: when the parent runtime IS auto-approved, every approval + // class is permitted (same as before the delegation hardening). + let tmp = tempdir().expect("tempdir"); + let mut runtime = stub_runtime(); + runtime.context = ToolContext::new(tmp.path().to_path_buf()); + runtime.context.auto_approve = true; + let registry = SubAgentToolRegistry::new( + runtime, + SubAgentType::General, + None, + Arc::new(Mutex::new(TodoList::new())), + Arc::new(Mutex::new(PlanState::default())), + ); + + // Calling exec_shell with interactive=true is what we block via the + // separate terminal-takeover guard; pick the simpler write-file path + // to assert that approval gating is off when auto_approve is set. + registry + .execute( + "agent_test", + "write_file", + json!({"path": "auto.txt", "content": "auto"}), + ) + .await + .expect("auto-approved parent should allow writes"); +} + #[test] fn child_cancellation_cascades_from_parent() { let parent = stub_runtime(); @@ -1757,6 +1927,46 @@ fn child_runtime_propagates_completion_tx_for_gating() { ); } +#[test] +fn subagent_runtime_default_step_api_timeout_is_legacy_120s() { + // The legacy hardcoded constant is now the default field value so existing + // call sites and tests that construct a runtime without explicit timeout + // wiring keep their old behavior (#1806, #1808). + let runtime = stub_runtime(); + assert_eq!(runtime.step_api_timeout, DEFAULT_STEP_API_TIMEOUT); + assert_eq!( + DEFAULT_STEP_API_TIMEOUT, + std::time::Duration::from_secs(crate::config::DEFAULT_SUBAGENT_API_TIMEOUT_SECS) + ); +} + +#[test] +fn with_step_api_timeout_overrides_runtime_field() { + let runtime = stub_runtime().with_step_api_timeout(std::time::Duration::from_secs(900)); + assert_eq!(runtime.step_api_timeout.as_secs(), 900); +} + +#[test] +fn child_runtime_preserves_step_api_timeout() { + // Real sub-agents spawn through `child_runtime()` / `background_runtime()`; + // forgetting to clone the timeout would silently drop the user's config + // override and resurrect the 120 s default for every child step. + let parent = stub_runtime().with_step_api_timeout(std::time::Duration::from_secs(900)); + let child = parent.child_runtime(); + let background = parent.background_runtime(); + + assert_eq!( + child.step_api_timeout.as_secs(), + 900, + "child_runtime must preserve parent's per-step timeout" + ); + assert_eq!( + background.step_api_timeout.as_secs(), + 900, + "background_runtime (detached) must also preserve the parent's timeout" + ); +} + #[test] fn subagent_completion_payload_carries_existing_sentinel_format() { // The payload format is the same one already documented in diff --git a/docs/SUBAGENTS.md b/docs/SUBAGENTS.md index 791a881f..60ecbddf 100644 --- a/docs/SUBAGENTS.md +++ b/docs/SUBAGENTS.md @@ -110,6 +110,23 @@ cancelled records persist for inspection but don't occupy a slot. Agents that lost their `task_handle` (e.g. across a process restart) also don't count against the cap. +## Per-step API timeout (#1806, #1808) + +Each sub-agent step wraps its DeepSeek `create_message` call in a +per-step timeout so a single stuck request can't pin the parent's +completion wakeup channel indefinitely. The default is `120` seconds, +which matches the legacy hardcoded value. Long-thinking children that +legitimately exceed that, for example heavy plan or review work behind +`agent_open`, can extend the timeout in `~/.deepseek/config.toml`: + +```toml +[subagents] +api_timeout_secs = 900 # 15 minutes; clamped to 1..=1800 +``` + +Values are clamped to `1..=1800`. `0` and `unset` keep the legacy +`120` second default, so existing installs see no behavior change. + ## Lifecycle Each opened session produces a record that progresses through: