fix(subagents): allow explicit write-role edits

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<Vec<String>>,
     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<Vec<String>>,
         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)?;

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

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: