llm retriability via structured status, resolve tools through canonical sandbox path, resume each frame with its own agent config

crates/skald-core/src/chatbot/mod.rs

@@ -2,6 +2,6 @@ pub mod logging;
 
 // Re-export from the independent llm-client crate.
 pub use llm_client::{
-    ChatOptions, ChatResponse, ChatbotClient, LlmRawMeta, LlmTurn, Message, ToolCall,
-    anthropic, lm_studio, ollama, openai,
+    ChatOptions, ChatResponse, ChatbotClient, LlmError, LlmRawMeta, LlmTurn, Message, ToolCall,
+    anthropic, http_status, lm_studio, ollama, openai,
 };

crates/skald-core/src/session/handler/agent_dispatch.rs

@@ -70,64 +70,11 @@ impl ChatSessionHandler {
             Some(parent_tool_call_id),
         ).await?;
 
-        let persisted_grants = stack_mcp_grants::list_for_stack(pool, child.id)
-            .await
-            .unwrap_or_default();
-        let active_mcp_grants: Arc<RwLock<HashSet<String>>> =
-            Arc::new(RwLock::new(persisted_grants.into_iter().collect()));
-
-        let mut child_config = parent_config.for_sub_agent(target_id.to_string(), resolved_client.clone());
-        child_config.active_mcp_grants = Arc::clone(&active_mcp_grants);
-
-        child_config.base_tool_defs.extend(self.tools.openai_definitions_sub_agents_only());
-        child_config.base_tool_defs.push(super::ask_user_clarification_tool_def());
-        // Let the sub-agent dispatch a further sub-agent (e.g. tech-lead → architect/engineer).
-        // `execute_subtask` is intercepted in `run_agent_turn` and routed back here. Only expose it
-        // while the child can still recurse — at the depth limit `dispatch_sub_agent` would reject it.
-        if new_depth < MAX_AGENT_DEPTH {
-            child_config.base_tool_defs.push(super::execute_subtask_tool_def());
-        }
-
-        {
-            let group_id    = self.tool_group_id().await;
-            let gid         = group_id.as_deref().unwrap_or("default");
-            let group_rules = crate::db::approval_rules::list_for_group(
-                pool, Some(gid),
-            ).await.unwrap_or_default();
-            child_config.base_tool_defs.retain(|def| {
-                let name = def["function"]["name"].as_str().unwrap_or("");
-                self.approval.is_tool_visible(&group_rules, name)
-            });
-        }
-
-        {
-            let pool_clone   = Arc::clone(&self.db);
-            let session_id   = self.session_id;
-            let stack_id     = child.id;
-            let mcp_clone    = Arc::clone(&self.mcp);
-            let grants_clone = Arc::clone(&active_mcp_grants);
-
-            let activate_tool = crate::tools::activate_tools::ActivateTools {
-                pool:              pool_clone,
-                session_id,
-                stack_id:          Some(stack_id),
-                mcp:               mcp_clone,
-                active_mcp_grants: grants_clone,
-            };
-            let activate_tool = Arc::new(activate_tool);
-            child_config.interface_tools.push(InterfaceTool {
-                definition: activate_tools_tool_def(),
-                handler: Arc::new(move |args| -> ToolFuture {
-                    use crate::tools::Tool as _;
-                    let tool = Arc::clone(&activate_tool);
-                    Box::pin(async move {
-                        tokio::task::spawn_blocking(move || tool.execute(args))
-                            .await
-                            .map_err(|e| anyhow::anyhow!("activate_tools task panicked: {e}"))?
-                    })
-                }),
-            });
-        }
+        // Single source of the sub-agent's config (base tools + augmentation + grants
+        // + activate_tools), shared with restart recovery so the two can't drift (B3).
+        let child_config = self.build_sub_agent_config(
+            parent_config, target_id, resolved_client.clone(), child.id, new_depth,
+        ).await?;
 
         chat_history::append(pool, child.id, &chat_history::Role::Agent, prompt, false, None).await?;
 
@@ -199,6 +146,108 @@ impl ChatSessionHandler {
         result
     }
 
+    /// Builds the [`AgentRunConfig`] for a sub-agent stack frame: base tools derived
+    /// from `parent_config`, plus the sub-agent augmentation (sub-agents-only tools,
+    /// `ask_user_clarification`, `execute_subtask` while `depth` still permits
+    /// recursion), the approval-visibility filter, the frame's persisted MCP grants,
+    /// and a stack-scoped `activate_tools`.
+    ///
+    /// The **single** source of a sub-agent's config, shared by live dispatch
+    /// (`dispatch_sub_agent`) and post-restart recovery (`build_recovery_frame_config`),
+    /// so a resumed child runs with the same prompt/tools it had live — never the root
+    /// agent's (bug B3). `depth` is passed explicitly (not `parent.depth + 1`) so
+    /// recovery can build a config for a frame at any depth straight from the root.
+    pub(super) async fn build_sub_agent_config(
+        &self,
+        parent_config: &AgentRunConfig,
+        agent_id:      &str,
+        client_name:   String,
+        stack_id:      i64,
+        depth:         i64,
+    ) -> anyhow::Result<AgentRunConfig> {
+        let persisted_grants = stack_mcp_grants::list_for_stack(&self.db, stack_id)
+            .await
+            .unwrap_or_default();
+        let active_mcp_grants: Arc<RwLock<HashSet<String>>> =
+            Arc::new(RwLock::new(persisted_grants.into_iter().collect()));
+
+        let mut child_config = parent_config.for_sub_agent(agent_id.to_string(), client_name);
+        child_config.depth             = depth;
+        child_config.active_mcp_grants = Arc::clone(&active_mcp_grants);
+
+        child_config.base_tool_defs.extend(self.tools.openai_definitions_sub_agents_only());
+        child_config.base_tool_defs.push(super::ask_user_clarification_tool_def());
+        // Expose `execute_subtask` only while the child can still recurse — at the
+        // depth limit `dispatch_sub_agent` would reject it.
+        if depth < MAX_AGENT_DEPTH {
+            child_config.base_tool_defs.push(super::execute_subtask_tool_def());
+        }
+
+        {
+            let group_id    = self.tool_group_id().await;
+            let gid         = group_id.as_deref().unwrap_or("default");
+            // Registry table — read from the registry pool, not the owner pool
+            // (see the same filter in `config.rs::build_agent_config`).
+            let group_rules = match crate::db::approval_rules::list_for_group(
+                &self.shared_pool, Some(gid),
+            ).await {
+                Ok(rules) => rules,
+                Err(e) => {
+                    tracing::warn!(group = gid, error = %e, "sub-agent approval-rules visibility filter: list_for_group failed; leaving all tools visible");
+                    Vec::new()
+                }
+            };
+            child_config.base_tool_defs.retain(|def| {
+                let name = def["function"]["name"].as_str().unwrap_or("");
+                self.approval.is_tool_visible(&group_rules, name)
+            });
+        }
+
+        {
+            let activate_tool = crate::tools::activate_tools::ActivateTools {
+                pool:              Arc::clone(&self.db),
+                session_id:        self.session_id,
+                stack_id:          Some(stack_id),
+                mcp:               Arc::clone(&self.mcp),
+                active_mcp_grants: Arc::clone(&active_mcp_grants),
+            };
+            let activate_tool = Arc::new(activate_tool);
+            child_config.interface_tools.push(InterfaceTool {
+                definition: activate_tools_tool_def(),
+                handler: Arc::new(move |args| -> ToolFuture {
+                    use crate::tools::Tool as _;
+                    let tool = Arc::clone(&activate_tool);
+                    Box::pin(async move {
+                        tokio::task::spawn_blocking(move || tool.execute(args))
+                            .await
+                            .map_err(|e| anyhow::anyhow!("activate_tools task panicked: {e}"))?
+                    })
+                }),
+            });
+        }
+
+        Ok(child_config)
+    }
+
+    /// Config to re-run a sub-agent frame during app-restart recovery: resolves the
+    /// frame's **own** agent (prompt/meta/client) and builds its sub-agent config, so
+    /// `resume_turn`'s cascade resumes a child as itself, not as the root agent (bug
+    /// B3). The root frame is not passed here — the caller keeps the session's root
+    /// config for it. Base tools derive from `root_config`; the per-dispatch `client`
+    /// override isn't persisted, so the frame's agent meta drives model resolution.
+    pub(super) async fn build_recovery_frame_config(
+        &self,
+        root_config: &AgentRunConfig,
+        frame:       &chat_sessions_stack::SessionStack,
+    ) -> anyhow::Result<AgentRunConfig> {
+        let meta = crate::agents::load_task_meta(&frame.agent_id)
+            .map_err(|e| anyhow::anyhow!("resume: cannot load sub-agent `{}`: {e}", frame.agent_id))?;
+        let (client, _) = self.llm_manager.resolve(
+            meta.client.as_deref(), meta.scope.as_deref(), meta.strength,
+        ).await?;
+        self.build_sub_agent_config(root_config, &frame.agent_id, client.to_string(), frame.id, frame.depth).await
+    }
+
     /// Handles the `update_scratchpad` built-in.
     ///
     /// The scratchpad is a session-scoped shared blackboard (`scratchpad_sid()` is

crates/skald-core/src/session/handler/approval.rs

@@ -53,9 +53,29 @@ impl ChatSessionHandler {
         }
     }
 
-    /// Reads the current content of a file from disk (for diff generation in PendingWrite events).
+    /// Reads the current content of a file for the diff in a `PendingWrite` event.
+    ///
+    /// Routes **exactly like the fs-tools** (blueprint §6), so the diff the user
+    /// approves reflects the real target — not the server's cwd:
+    /// - `user-memory/…` / `shared-memory/…` → the `memory_docs` note on the right
+    ///   pool (owner vs `system.db`), never disk;
+    /// - every other agent path → the caller's per-user host workspace via `self.fs`,
+    ///   containment-checked by `resolve_host_path`.
+    ///
+    /// A resolve failure or a missing note/file yields `None` (rendered as "new file").
+    /// The old cwd-relative `fs::resolve` was wrong for every agent path: it showed a
+    /// bogus "new file" on overwrites and, worse, the diff of a same-named cwd file.
     pub(super) async fn read_current_content(&self, path: &str) -> Option<String> {
-        let abs = crate::tools::fs::resolve(path).ok()?;
+        use crate::tools::fs::{classify_memory, resolve_host_path, MemScope};
+        if let Some(m) = classify_memory(path) {
+            let pool = match m.scope {
+                MemScope::User   => &self.db,
+                MemScope::Shared => &self.shared_pool,
+            };
+            return crate::db::memory_docs::get(pool, &m.rel)
+                .await.ok().flatten().map(|d| d.content);
+        }
+        let abs = resolve_host_path(&self.fs.load(), path).ok()?;
         tokio::fs::read_to_string(&abs).await.ok()
     }
 

crates/skald-core/src/session/handler/config.rs

@@ -114,9 +114,19 @@ impl ChatSessionHandler {
         {
             let group_id   = self.tool_group_id().await;
             let gid        = group_id.as_deref().unwrap_or("default");
-            let group_rules = crate::db::approval_rules::list_for_group(
-                &self.db, Some(gid),
-            ).await.unwrap_or_default();
+            // `approval_rules` is a registry table (`create_registry_tables`), so it
+            // must be read from the registry pool, not the per-user owner pool — the
+            // latter has no such table, the query errors, and `unwrap_or_default()`
+            // would silently yield an empty ruleset (→ every tool "visible").
+            let group_rules = match crate::db::approval_rules::list_for_group(
+                &self.shared_pool, Some(gid),
+            ).await {
+                Ok(rules) => rules,
+                Err(e) => {
+                    tracing::warn!(group = gid, error = %e, "approval-rules visibility filter: list_for_group failed; leaving all tools visible");
+                    Vec::new()
+                }
+            };
             let visible = |def: &Value| {
                 let name = def["function"]["name"].as_str().unwrap_or("");
                 self.approval.is_tool_visible(&group_rules, name)

crates/skald-core/src/session/handler/llm_call.rs

@@ -145,20 +145,58 @@ impl ChatSessionHandler {
 }
 
 /// Whether an LLM error is worth retrying on a different model.
+///
+/// Classifies on the real HTTP status ([`crate::chatbot::http_status`]), not a
+/// substring of the message — a model id or token count containing "404"/"401" no
+/// longer mis-classifies (bug B6). A non-HTTP failure (network, parse) has no status
+/// and is retriable, matching the previous default.
 fn is_retriable_llm_error(e: &anyhow::Error) -> bool {
-    let msg = e.to_string().to_lowercase();
-    // Never retry client errors — the request itself is malformed or unauthorized.
-    // 400 is excluded: some providers reject valid requests that others accept
-    // (e.g. DeepSeek requires reasoning_content echo, OpenAI does not), so
-    // retrying on a different model can succeed.
-    for code in ["401", "403", "404", "422"] {
-        if msg.contains(code) {
-            return false;
-        }
-    }
-    true
+    // Never retry these client errors — the request itself is unauthorized, not
+    // found, or unprocessable. 400 is intentionally NOT listed: some providers
+    // reject valid requests that others accept (e.g. DeepSeek requires a
+    // reasoning_content echo, OpenAI does not), so retrying elsewhere can succeed.
+    // 429 and 5xx stay retriable (a different model / provider may serve the call).
+    !matches!(crate::chatbot::http_status(e), Some(401 | 403 | 404 | 422))
 }
 
 fn first_line(s: &str) -> String {
     s.lines().next().unwrap_or(s).to_string()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::is_retriable_llm_error;
+    use crate::chatbot::LlmError;
+
+    fn http_err(status: u16, message: &str) -> anyhow::Error {
+        LlmError { status: Some(status), message: message.to_string() }.into()
+    }
+
+    #[test]
+    fn client_errors_are_not_retried() {
+        for code in [401, 403, 404, 422] {
+            assert!(!is_retriable_llm_error(&http_err(code, "nope")), "{code} must not retry");
+        }
+    }
+
+    #[test]
+    fn server_rate_limit_and_400_retry() {
+        for code in [400, 429, 500, 502, 503] {
+            assert!(is_retriable_llm_error(&http_err(code, "retry")), "{code} must retry");
+        }
+    }
+
+    #[test]
+    fn non_http_errors_retry() {
+        assert!(is_retriable_llm_error(&anyhow::anyhow!("connection reset by peer")));
+    }
+
+    #[test]
+    fn status_digits_in_the_message_do_not_mislead() {
+        // Regression for B6: the old substring check read any "404"/"401" in the text
+        // as a client error. A 500 whose body mentions "1401 tokens" / "code 404" must
+        // still retry — classification keys on the structured status, not the string.
+        let e = http_err(500, "provider error: too many (1401) tokens, see code 404 in docs");
+        assert!(is_retriable_llm_error(&e));
+    }
+}

crates/skald-core/src/session/handler/resume.rs

@@ -5,7 +5,7 @@ use tracing::{error, info, warn};
 
 use crate::db::{chat_history, chat_llm_tools, chat_sessions_stack};
 use crate::events::ServerEvent;
-use crate::tools::{ToolDescriptionLength, ToolResult, tool_names as tn};
+use crate::tools::{drive_execution, ExecutionOutcome, ToolDescriptionLength, ToolResult, tool_names as tn};
 
 use super::{ChatSessionHandler, TurnOutcome};
 use super::emitter::TurnEmitter;
@@ -14,14 +14,35 @@ use super::outcome::RecordFlow;
 use super::interface_tools::{AgentRunConfig, InterfaceTool};
 
 impl ChatSessionHandler {
-    /// Dispatches a single tool call by name+args without going through the LLM loop.
-    /// Used by the REST `resolve` endpoint and by `resume_pending_tools`.
-    /// Does NOT update the DB — caller is responsible for `complete` / `fail`.
+    /// Dispatches a single already-approved tool call by name+args, without running
+    /// the LLM loop. The sole caller is the REST `resolve` endpoint's post-restart
+    /// "simple tools" branch (no live oneshot to unblock; sub-agent and `restart`
+    /// tools are handled earlier there). Does NOT touch the DB — the caller records
+    /// `complete`/`fail`.
+    ///
+    /// Runs through the **same canonical path as the live loop** — `build_execution`
+    /// (which constructs the [`ToolContext`]: owner pool + per-user container fs)
+    /// driven by `drive_execution`. The previous `self.tools.dispatch(name, args)`
+    /// bypassed the context entirely, so a resolved `write_file` landed in the server
+    /// cwd (no containment, memory paths hit disk) and `execute_cmd` ran on the host —
+    /// a blueprint §6 sandbox escape (bug B1). MCP tools are covered by
+    /// `build_execution` too, so no name special-casing is needed here.
     pub async fn execute_tool(&self, name: &str, args: Value) -> anyhow::Result<ToolResult> {
-        if let Some((srv, mcp_tool)) = crate::mcp::parse_mcp_tool_name(name) {
-            return self.mcp.call(srv, mcp_tool, args).await;
+        // No interface tools post-restart: a pending-approval tool is a built-in /
+        // memory / MCP call, never a per-interface closure like `activate_tools`.
+        let config = self.build_agent_config(
+            None, None, None, Vec::new(), std::collections::HashMap::new(),
+        ).await?;
+        let exec = self.build_execution(name, args, &config)
+            .ok_or_else(|| anyhow::anyhow!("unknown tool: {name}"))?;
+        // A resolve is a one-shot; nothing wires /stop to it, so a fresh (never
+        // cancelled) token satisfies the driver contract.
+        let token = CancellationToken::new();
+        match drive_execution(exec.as_ref(), &token).await {
+            ExecutionOutcome::Completed(result) => Ok(result),
+            ExecutionOutcome::Failed(msg)       => Err(anyhow::anyhow!(msg)),
+            ExecutionOutcome::Cancelled         => Err(anyhow::anyhow!("tool execution cancelled")),
         }
-        self.tools.dispatch(name, args).await.map(ToolResult::Text)
     }
 
     /// Resumes the LLM loop for the current session WITHOUT appending a new user message.
@@ -63,8 +84,22 @@ impl ChatSessionHandler {
 
         info!(session_id = self.session_id, stack_id = stack.id, depth = stack.depth, "resume_turn start");
 
+        // B3: resume each frame with ITS OWN agent's config (prompt/tools/client), not
+        // the session root's. After a restart the deepest active frame may be a
+        // sub-agent; running it under `config` would resume e.g. a `researcher` as the
+        // `assistant`. The root frame keeps `config`; a sub-agent frame gets a freshly
+        // built sub-agent config for its own agent (deferred-init so the root path
+        // borrows `config` and the sub-agent path borrows the owned value).
+        let seed_frame_config;
+        let seed_config: &AgentRunConfig = if stack.parent_tool_call_id.is_none() {
+            &config
+        } else {
+            seed_frame_config = self.build_recovery_frame_config(&config, &stack).await?;
+            &seed_frame_config
+        };
+
         // Resume pending/interrupted tools before running the LLM loop.
-        let had_pending = self.resume_pending_tools(stack.id, &config, &token, &tx).await?;
+        let had_pending = self.resume_pending_tools(stack.id, seed_config, &token, &tx).await?;
 
         // Seed the cascade. Normally we (re)run the deepest active frame's LLM loop
         // (live injection only applies to a fresh interactive turn from handle_message).
@@ -99,7 +134,7 @@ impl ChatSessionHandler {
                     }
                 }
             }
-            (self.run_agent_turn(stack.id, &config, &token, &tx, None).await?, stack)
+            (self.run_agent_turn(stack.id, seed_config, &token, &tx, None).await?, stack)
         };
 
         // Cascade completion upward through parent stacks (handles app-restart recovery
@@ -156,8 +191,17 @@ impl ChatSessionHandler {
                 "resume_turn: cascading to parent stack"
             );
 
-            self.resume_pending_tools(parent_stack.id, &config, &token, &tx).await?;
-            current_outcome = self.run_agent_turn(parent_stack.id, &config, &token, &tx, None).await?;
+            // B3: run the parent under its own agent's config (the root keeps `config`).
+            let parent_frame_config;
+            let parent_run_config: &AgentRunConfig = if parent_stack.parent_tool_call_id.is_none() {
+                &config
+            } else {
+                parent_frame_config = self.build_recovery_frame_config(&config, &parent_stack).await?;
+                &parent_frame_config
+            };
+
+            self.resume_pending_tools(parent_stack.id, parent_run_config, &token, &tx).await?;
+            current_outcome = self.run_agent_turn(parent_stack.id, parent_run_config, &token, &tx, None).await?;
             current_stack = parent_stack;
 
         }

crates/skald-core/src/tools/exec.rs

@@ -1,4 +1,3 @@
-use std::path::PathBuf;
 use std::process::Stdio;
 use std::time::Duration;
 
@@ -15,6 +14,14 @@ const DEFAULT_TIMEOUT_SECS: u64 = 120;
 const MAX_TIMEOUT_SECS:     u64 = 600;
 const MAX_OUTPUT_BYTES:     usize = 100_000;
 
+/// Returned by the context-free `Tool` entry points (`execute`/`execute_async`).
+/// `execute_cmd` only ever runs through `run_with`, which carries the caller's
+/// `ToolContext` and dispatches into the per-user container. There is no safe
+/// host fallback (blueprint §6): running on the host would execute the command
+/// in the Skald process itself, outside the sandbox the user approved.
+const HOST_PATH_ERROR: &str =
+    "execute_cmd requires the per-user container (ToolContext); it cannot run on the host";
+
 pub struct ExecuteCmd;
 
 impl Tool for ExecuteCmd {
@@ -76,18 +83,18 @@ impl Tool for ExecuteCmd {
         }
     }
 
-    fn execute(&self, args: Value) -> Result<String> {
-        tokio::task::block_in_place(|| {
-            tokio::runtime::Handle::current().block_on(run_from_args(&args))
-        })
+    /// Context-free entry point — deliberately unreachable for real work. Without a
+    /// `ToolContext` there is no per-user container to target, so this must NOT fall
+    /// back to a host `sh -c` (blueprint §6 sandbox). Any dispatch that lands here
+    /// (e.g. a REST resolve that bypasses the tool loop) is a caller bug: fail loud
+    /// rather than escape the sandbox. The live path is `run_with`.
+    fn execute(&self, _args: Value) -> Result<String> {
+        anyhow::bail!(HOST_PATH_ERROR)
     }
 
-    /// Genuinely async so the unified `ToolExecution` path can race it against the
-    /// /stop token: on cancel the `SimpleExecution` drops this future and
-    /// `kill_on_drop(true)` kills the spawned shell process. (The sync `execute`
-    /// above — which blocks a worker thread — would not be cancellable.)
-    fn execute_async<'a>(&'a self, args: Value) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<String>> + Send + 'a>> {
-        Box::pin(async move { run_from_args(&args).await })
+    /// See [`Self::execute`]: no container without a `ToolContext`, so no host fallback.
+    fn execute_async<'a>(&'a self, _args: Value) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<String>> + Send + 'a>> {
+        Box::pin(async move { anyhow::bail!(HOST_PATH_ERROR) })
     }
 
     /// The real entry point (blueprint §6): the command runs **inside the caller's
@@ -243,75 +250,8 @@ async fn reap_container_group(container: &str, pidfile: &str) {
         .await;
 }
 
-/// Parse + run a shell command from tool arguments, as an awaitable future.
-///
-/// Driven by `ExecuteCmd::execute_async` through the unified `ToolExecution`
-/// path: on /stop the `SimpleExecution` drops this future and `kill_on_drop(true)`
-/// kills the child process. `Tool::execute` runs it synchronously via
-/// `block_in_place` only as a non-cancellable fallback.
-pub async fn run_from_args(args: &Value) -> Result<String> {
-    let (command, workdir, timeout_secs) = parse_args(args)?;
-    run(command, workdir, timeout_secs).await
-}
-
-fn parse_args(args: &Value) -> Result<(String, Option<PathBuf>, u64)> {
-    let command = args["command"].as_str()
-        .ok_or_else(|| anyhow::anyhow!("Missing required argument: command"))?
-        .to_string();
-
-    let workdir = match args["workdir"].as_str() {
-        Some(p) => {
-            let path = PathBuf::from(p);
-            if !path.is_absolute() {
-                anyhow::bail!("workdir must be an absolute path, got: {p}");
-            }
-            if !path.is_dir() {
-                anyhow::bail!("workdir does not exist or is not a directory: {p}");
-            }
-            Some(path)
-        }
-        None => None,
-    };
-
-    let timeout_secs = args["timeout"].as_u64()
-        .unwrap_or(DEFAULT_TIMEOUT_SECS)
-        .clamp(1, MAX_TIMEOUT_SECS);
-
-    Ok((command, workdir, timeout_secs))
-}
-
-async fn run(command: String, workdir: Option<PathBuf>, timeout_secs: u64) -> Result<String> {
-    // Audit log: record every shell command before it runs. Auto-approved
-    // commands (approval bypass active) otherwise leave no trace, so a command
-    // that kills the process — or misbehaves — can't be reconstructed.
-    let workdir_display = workdir
-        .as_deref()
-        .map(|p| p.display().to_string())
-        .unwrap_or_else(|| ".".to_string());
-    tracing::info!(
-        command = %command,
-        workdir = %workdir_display,
-        timeout_secs,
-        "execute_cmd: running shell command"
-    );
-
-    let mut cmd = tokio::process::Command::new("sh");
-    cmd.arg("-c")
-        .arg(&command)
-        .stdout(Stdio::piped())
-        .stderr(Stdio::piped())
-        .stdin(Stdio::null())
-        .kill_on_drop(true);
-
-    if let Some(dir) = workdir {
-        cmd.current_dir(dir);
-    }
-
-    capture(cmd, timeout_secs, &command).await
-}
-
 /// Spawns a prepared command, capturing stdout+stderr under a single timeout, and
-/// formats the result. Shared by the host `sh -c` path and the `docker exec` path.
+/// formats the result. Used by the `docker exec` path (`run_in_container`).
 async fn capture(mut cmd: tokio::process::Command, timeout_secs: u64, command: &str) -> Result<String> {
     let mut child = cmd.spawn()?;