Simplify coding-agent-loop: extract required_str helper, simplify spawn, restrict test-only accessors

- Proposal 7.1: Introduce required_str() helper in tools.rs to replace ~15
  repetitions of the args.get("param").and_then(|v| v.as_str()).ok_or_else(...)
  pattern across tools.rs and subagent.rs. Provides consistent error messages.

- Proposal 4.2: Simplify SubAgentManager::spawn success path by using ? on
  process_input() result directly, eliminating the redundant success variable
  (always true on the Ok path) and the if-let-Err-return pattern.

- Proposal 8.2: Mark SubAgent::depth() and SubAgentManager::get() as
  #[cfg(test)] since they are only used in tests. Remove SubAgent::id()
  entirely as it was unused even in tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

crates/coding-agent-loop/src/subagent.rs

@@ -1,6 +1,7 @@
 use crate::error::AgentError;
 use crate::session::Session;
 use crate::tool_registry::RegisteredTool;
+use crate::tools::required_str;
 use crate::types::Turn;
 use std::collections::{HashMap, VecDeque};
 use std::sync::atomic::{AtomicBool, Ordering};
@@ -17,18 +18,17 @@ pub struct SubAgentResult {
 }
 
 pub struct SubAgent {
+    #[allow(dead_code)]
     id: String,
+    #[allow(dead_code)]
     depth: usize,
     task: Option<tokio::task::JoinHandle<Result<SubAgentResult, AgentError>>>,
     followup_queue: Arc<Mutex<VecDeque<String>>>,
     abort_flag: Arc<AtomicBool>,
 }
 
+#[cfg(test)]
 impl SubAgent {
-    pub fn id(&self) -> &str {
-        &self.id
-    }
-
     pub fn depth(&self) -> usize {
         self.depth
     }
@@ -65,24 +65,16 @@ impl SubAgentManager {
         let abort_flag = session.abort_flag_handle();
 
         let task = tokio::spawn(async move {
-            let result = session.process_input(&task_prompt).await;
+            session.process_input(&task_prompt).await?;
             let turns = session.history().turns();
-            let turns_used = turns.len();
-            let last_text = turns.iter().rev().find_map(|t| {
-                if let Turn::Assistant { content, .. } = t {
-                    Some(content.clone())
-                } else {
-                    None
-                }
+            let last_text = turns.iter().rev().find_map(|t| match t {
+                Turn::Assistant { content, .. } => Some(content.clone()),
+                _ => None,
             });
-            let success = result.is_ok();
-            if let Err(e) = result {
-                return Err(e);
-            }
             Ok(SubAgentResult {
                 output: last_text.unwrap_or_default(),
-                success,
-                turns_used,
+                success: true,
+                turns_used: turns.len(),
             })
         });
 
@@ -145,6 +137,7 @@ impl SubAgentManager {
         Ok(())
     }
 
+    #[cfg(test)]
     pub fn get(&self, agent_id: &str) -> Option<&SubAgent> {
         self.agents.get(agent_id)
     }
@@ -186,10 +179,7 @@ pub fn make_spawn_agent_tool(
             let manager = manager.clone();
             let session_factory = session_factory.clone();
             Box::pin(async move {
-                let task = args
-                    .get("task")
-                    .and_then(|v| v.as_str())
-                    .ok_or_else(|| "Missing required parameter: task".to_string())?;
+                let task = required_str(&args, "task")?;
 
                 // Extract optional max_turns parameter
                 #[allow(clippy::cast_possible_truncation)]
@@ -234,14 +224,8 @@ pub fn make_send_input_tool(
         executor: Arc::new(move |args, _env| {
             let manager = manager.clone();
             Box::pin(async move {
-                let agent_id = args
-                    .get("agent_id")
-                    .and_then(|v| v.as_str())
-                    .ok_or_else(|| "Missing required parameter: agent_id".to_string())?;
-                let message = args
-                    .get("message")
-                    .and_then(|v| v.as_str())
-                    .ok_or_else(|| "Missing required parameter: message".to_string())?;
+                let agent_id = required_str(&args, "agent_id")?;
+                let message = required_str(&args, "message")?;
 
                 let mgr = manager.lock().await;
                 mgr.send_input(agent_id, message)?;
@@ -272,10 +256,7 @@ pub fn make_wait_tool(
         executor: Arc::new(move |args, _env| {
             let manager = manager.clone();
             Box::pin(async move {
-                let agent_id = args
-                    .get("agent_id")
-                    .and_then(|v| v.as_str())
-                    .ok_or_else(|| "Missing required parameter: agent_id".to_string())?;
+                let agent_id = required_str(&args, "agent_id")?;
 
                 let mut mgr = manager.lock().await;
                 let result = mgr.wait(agent_id).await?;
@@ -309,10 +290,7 @@ pub fn make_close_agent_tool(
         executor: Arc::new(move |args, _env| {
             let manager = manager.clone();
             Box::pin(async move {
-                let agent_id = args
-                    .get("agent_id")
-                    .and_then(|v| v.as_str())
-                    .ok_or_else(|| "Missing required parameter: agent_id".to_string())?;
+                let agent_id = required_str(&args, "agent_id")?;
 
                 let mut mgr = manager.lock().await;
                 mgr.close(agent_id)?;

crates/coding-agent-loop/src/tools.rs

@@ -5,6 +5,12 @@ use std::fmt::Write;
 use std::sync::Arc;
 use unified_llm::types::ToolDefinition;
 
+pub(crate) fn required_str<'a>(args: &'a serde_json::Value, key: &str) -> Result<&'a str, String> {
+    args.get(key)
+        .and_then(|v| v.as_str())
+        .ok_or_else(|| format!("Missing required parameter: {key}"))
+}
+
 #[must_use]
 pub fn make_read_file_tool() -> RegisteredTool {
     RegisteredTool {
@@ -23,9 +29,7 @@ pub fn make_read_file_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let file_path = args["file_path"]
-                    .as_str()
-                    .ok_or_else(|| "file_path is required".to_string())?;
+                let file_path = required_str(&args, "file_path")?;
                 let offset = args.get("offset").and_then(serde_json::Value::as_u64);
                 let limit = args.get("limit").and_then(serde_json::Value::as_u64);
 
@@ -58,12 +62,8 @@ pub fn make_write_file_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let file_path = args["file_path"]
-                    .as_str()
-                    .ok_or_else(|| "file_path is required".to_string())?;
-                let content = args["content"]
-                    .as_str()
-                    .ok_or_else(|| "content is required".to_string())?;
+                let file_path = required_str(&args, "file_path")?;
+                let content = required_str(&args, "content")?;
 
                 env.write_file(file_path, content).await?;
                 Ok(format!("Successfully wrote to {file_path}"))
@@ -91,15 +91,9 @@ pub fn make_edit_file_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let file_path = args["file_path"]
-                    .as_str()
-                    .ok_or_else(|| "file_path is required".to_string())?;
-                let old_string = args["old_string"]
-                    .as_str()
-                    .ok_or_else(|| "old_string is required".to_string())?;
-                let new_string = args["new_string"]
-                    .as_str()
-                    .ok_or_else(|| "new_string is required".to_string())?;
+                let file_path = required_str(&args, "file_path")?;
+                let old_string = required_str(&args, "old_string")?;
+                let new_string = required_str(&args, "new_string")?;
                 let replace_all = args
                     .get("replace_all")
                     .and_then(serde_json::Value::as_bool)
@@ -165,9 +159,7 @@ pub fn make_shell_tool_with_config(config: &SessionConfig) -> RegisteredTool {
         },
         executor: Arc::new(move |args, env| {
             Box::pin(async move {
-                let command = args["command"]
-                    .as_str()
-                    .ok_or_else(|| "command is required".to_string())?;
+                let command = required_str(&args, "command")?;
                 let timeout_ms = args
                     .get("timeout_ms")
                     .and_then(serde_json::Value::as_u64)
@@ -213,9 +205,7 @@ pub fn make_grep_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let pattern = args["pattern"]
-                    .as_str()
-                    .ok_or_else(|| "pattern is required".to_string())?;
+                let pattern = required_str(&args, "pattern")?;
                 let path = args
                     .get("path")
                     .and_then(serde_json::Value::as_str)
@@ -261,9 +251,7 @@ pub fn make_glob_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let pattern = args["pattern"]
-                    .as_str()
-                    .ok_or_else(|| "pattern is required".to_string())?;
+                let pattern = required_str(&args, "pattern")?;
                 let path = args
                     .get("path")
                     .and_then(serde_json::Value::as_str);
@@ -336,9 +324,7 @@ pub(crate) fn make_list_dir_tool() -> RegisteredTool {
         },
         executor: Arc::new(|args, env| {
             Box::pin(async move {
-                let path = args["path"]
-                    .as_str()
-                    .ok_or_else(|| "path is required".to_string())?;
+                let path = required_str(&args, "path")?;
                 #[allow(clippy::cast_possible_truncation)]
                 let depth = args
                     .get("depth")