From 8e891fbced4c0d5fa94c65c64753d228a69ccbb7 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Tue, 21 Jul 2026 22:26:29 +0100 Subject: [PATCH] llm retriability via structured status, resolve tools through canonical sandbox path, resume each frame with its own agent config --- CLAUDE.md | 9 +- crates/llm-client/src/lib.rs | 41 +++++ crates/llm-client/src/openai.rs | 11 +- crates/skald-core/src/chatbot/mod.rs | 4 +- .../src/session/handler/agent_dispatch.rs | 165 ++++++++++++------ .../src/session/handler/approval.rs | 24 ++- .../skald-core/src/session/handler/config.rs | 16 +- .../src/session/handler/llm_call.rs | 60 +++++-- .../skald-core/src/session/handler/resume.rs | 66 +++++-- crates/skald-core/src/tools/exec.rs | 98 ++--------- 10 files changed, 321 insertions(+), 173 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9d131ab..4aca404 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,7 +74,7 @@ Two rules keep the boundary real, and both are enforced by the compiler: | `crates/skald-core/src/chat_hub/` | `ChatHub`: broadcast events to all connected WS clients | | `crates/skald-core/src/chat_event_bus.rs` | Global async bus for cross-session events | | `crates/skald-core/src/agents.rs` | Discovers agents from `agents/*/`, loads meta + system prompt | -| `crates/skald-core/src/tools/` | Built-in tools: `exec` (**runs inside the caller's per-user Docker container** via `docker exec`, as the non-root host uid — `sudo` for system installs — with a robust /stop that reaps the command's process-group; see `container/`), `restart`, `list_agents`, `fs/*` (route `user-memory/`/`shared-memory/` to `memory_docs`, and every other **physical** path through `ctx.fs` to the caller's per-user host workspace — see DB tables + container), `notify`, `ast_outline`, `image_generate`, MCP tools, plugin tools, cron tools | +| `crates/skald-core/src/tools/` | Built-in tools: `exec` (**runs inside the caller's per-user Docker container** via `docker exec`, as the non-root host uid — `sudo` for system installs — with a robust /stop that reaps the command's process-group; see `container/`; the only live path is `run_with` (needs `ToolContext`) — the context-free `Tool::execute`/`execute_async` now **error** (`HOST_PATH_ERROR`) instead of the old host `sh -c`, so nothing can run a command outside the sandbox), `restart`, `list_agents`, `fs/*` (route `user-memory/`/`shared-memory/` to `memory_docs`, and every other **physical** path through `ctx.fs` to the caller's per-user host workspace — see DB tables + container), `notify`, `ast_outline`, `image_generate`, MCP tools, plugin tools, cron tools | | `crates/skald-core/src/container/` | `ContainerManager` (§6): per-user Docker containers (the execution sandbox). Docker is a **hard requirement** — `check_docker()` fails `Skald::new` (→ shell exits) if the daemon is unreachable. Builds our own `skald-runtime` image (python+node+**sudo**; tag is **versioned** `skald-runtime:v2` so a `Dockerfile` change forces a rebuild) once from the embedded `Dockerfile`, then `reconcile_all()` at boot ensures one running container `skald-{userid}` per active user. Each container runs as the **host `uid:gid`** (`--user`, §6 UID coherence) with `--init` (tini reaps zombies); `ensure()` **self-heals** a container whose `--user` is stale (e.g. an old root one) by recreating it, and injects a passwd/shadow entry post-create so `sudo` (NOPASSWD, in the image) resolves the arbitrary uid. `build_user_fs()` assembles a user's `UserFs` (home `{WD}/homes/{userid}` → `/root`, plus each `shared/{name}` they belong to). Shells the `docker` CLI (no client crate) | | `crates/skald-core/src/tool_catalog.rs` | `ToolCatalog`: unified tool listing façade (wraps ToolRegistry + McpManager) | | `crates/skald-core/src/events.rs` | `ServerEvent` enum streamed over WebSocket to the frontend | @@ -90,7 +90,7 @@ Two rules keep the boundary real, and both are enforced by the compiler: | `crates/skald-core/src/clarification/` | `ClarificationManager`: background-session question/answer | | `crates/skald-core/src/elicitation/` | `ElicitationManager` + bridge: MCP server-initiated input (`elicitation/create`), surfaced in the Inbox; secrets never logged/persisted | | `crates/skald-core/src/inbox.rs` | `Inbox`: unified façade for pending approvals + clarifications + elicitations (wraps ApprovalManager, ClarificationManager, ElicitationManager) | -| `crates/skald-core/src/llm/` | LLM client abstraction (OpenAI-compat, Anthropic, Ollama…). OpenAI-compatible provider *types* are runtime data, not code: `providers/declared.rs` loads `providers.yaml` at boot (see Config); only non-OpenAI-compatible or bespoke providers (anthropic, ollama, openai, openrouter) stay native | +| `crates/skald-core/src/llm/` | LLM client abstraction (OpenAI-compat, Anthropic, Ollama…). OpenAI-compatible provider *types* are runtime data, not code: `providers/declared.rs` loads `providers.yaml` at boot (see Config); only non-OpenAI-compatible or bespoke providers (anthropic, ollama, openai, openrouter) stay native. **Retriability** (`llm_call.rs::is_retriable_llm_error`) keys on the real HTTP status via `llm_client::http_status` (a structured `LlmError { status }` from the client, else a `reqwest::Error` in the chain), **not** a substring of the message — a model id/token count containing "404"/"401" no longer mis-classifies; 401/403/404/422 don't retry, 400/429/5xx/network do | | `crates/skald-core/src/transcribe/` | Transcription providers | | `crates/skald-core/src/image_generate/` | Image generation providers | | `crates/skald-core/src/memory/` | Agent memory tools | @@ -198,6 +198,7 @@ At context-build time (`MessageBuilder`), attachments of the **current turn** (t - **The parent's resolved client is NOT inherited.** Passing a concrete model name to `resolve()` bypasses strength/scope checks; sub-agents always auto-select unless overridden explicitly. - `list_agents` is a plain tool; returns JSON of **task** agents only (excludes `chat`/`system` agents like the `assistant` entry agent). - `resume_turn` (+ its cascade) is kept only for: app-restart recovery of an active child stack, async task result injection (`inject_async_result`), and the WS resume message — not for the normal sync dispatch. +- **The cascade runs each frame with ITS OWN agent's config, not the session root's.** `resume_turn` builds the root config from `self.agent_id`, but for any non-root frame (deepest seed + each parent it walks up) it derives a per-frame config via `build_recovery_frame_config` → `build_sub_agent_config` (keyed on `frame.agent_id`), so a resumed sub-agent runs with its own prompt/tools/client — not the root's (it would otherwise resume e.g. a `researcher` as the `assistant`). `build_sub_agent_config` is the **single** source of a sub-agent's config, shared by live `dispatch_sub_agent` and this recovery path so they can't drift; the per-dispatch `client` override isn't persisted, so recovery re-resolves the model from the frame's agent meta. ## Cancellation (stop) @@ -208,7 +209,9 @@ At context-build time (`MessageBuilder`), attachments of the **current turn** (t The rule engine `ApprovalManager::check` returns `Allow`/`Deny`/`Require` per tool call (default rules seeded on first boot; the catch-all `* require @999999` gates anything not explicitly allowed — e.g. `execute_cmd`, `restart`, `execute_task`, writes outside whitelisted paths). A `Require` registers a `oneshot` in the in-memory `pending` map keyed by `request_id` and emits an approval event over WS. -Resolution is **source-agnostic**: the WS + Inbox paths resolve by `request_id`; the inline chat card resolves by the durable `tool_call_id` via `POST /api/tools/:tool_call_id/resolve` (`resolve_tool` in `src/frontend/api/sessions.rs`), which derives the owning session from the tool call's own stack row — never a hardcoded source. Live pending cards fire the `oneshot`; post-restart they execute directly on the owning session. See `docs/approval/`. +Resolution is **source-agnostic**: the WS + Inbox paths resolve by `request_id`; the inline chat card resolves by the durable `tool_call_id` via `POST /api/tools/:tool_call_id/resolve` (`resolve_tool` in `src/frontend/api/sessions.rs`), which derives the owning session from the tool call's own stack row — never a hardcoded source. Live pending cards fire the `oneshot`; post-restart a simple tool runs directly on the owning session via `ChatSessionHandler::execute_tool`, which now goes through the **same canonical path as the live loop** — `build_execution` (owner pool + per-user container `ToolContext`) driven by `drive_execution` — so a resolved `write_file`/`execute_cmd` acts on the user's workspace/container, never the server cwd/host (was a §6 escape; sub-agent + `restart` tools are still handled by their own branches earlier in `resolve_tool`). See `docs/approval/`. + +The **diff preview** in a `PendingWrite` event (`handler/approval.rs::read_current_content`) routes exactly like the fs-tools: `user-memory/`/`shared-memory/` → `memory_docs` on the right pool, every other agent path → the caller's host workspace via `resolve_host_path(&self.fs, …)`. It must never use the cwd-relative `fs::resolve` — that showed a bogus "new file" on overwrites (or the diff of a same-named cwd file), so the user would approve the wrong diff. **Tool visibility in the Security-groups UI** (`GET /api/approval/tools`): tools injected outside the `ToolRegistry` (interface/plugin/provider tools) would otherwise be un-configurable. `ToolCatalog::list_all()` covers registry tools + a static `synthetic_tools()` list of core interface tools; everything else is captured by `crates/skald-core/src/tool_discovery.rs` (`ToolDiscovery`), which taps `all_tool_defs()` in `llm_loop.rs` each round and upserts every offered tool into the `known_tools` table (in-memory seen-set guard → background DB write). `list_tools` merges `known_tools` (deduped, `category: "dynamic"`) so any tool offered at least once becomes gate-able. Drift-proof by construction; core never hardcodes plugin tool names. diff --git a/crates/llm-client/src/lib.rs b/crates/llm-client/src/lib.rs index 2973876..2cd9aa9 100644 --- a/crates/llm-client/src/lib.rs +++ b/crates/llm-client/src/lib.rs @@ -31,3 +31,44 @@ pub fn redact_key(key: &str) -> String { "***".to_string() } } + +/// A structured LLM call failure carrying the HTTP `status` of the response. +/// +/// Clients that read the status themselves (rather than via `error_for_status`) +/// return this so callers can classify retriability on the numeric code instead of +/// substring-matching a formatted message — which mis-fires when a model id, token +/// count or URL merely contains "401"/"404"/… (bug B6). Non-HTTP failures (network, +/// JSON parse, cancellation) stay ordinary `anyhow` errors with no status. +#[derive(Debug)] +pub struct LlmError { + /// HTTP status code, when the failure came from an HTTP response. + pub status: Option, + /// Human-readable detail (provider tag + body), used for logs and the UI. + pub message: String, +} + +impl std::fmt::Display for LlmError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + +impl std::error::Error for LlmError {} + +/// Extracts the HTTP status of an LLM failure, if any: a structured +/// [`LlmError::status`] first, else any `reqwest::Error` in the source chain (the +/// clients that fail via `error_for_status()?`). Returns `None` for a non-HTTP +/// error (network, parse, cancellation), which callers should treat as retriable. +pub fn http_status(err: &anyhow::Error) -> Option { + for cause in err.chain() { + if let Some(le) = cause.downcast_ref::() { + return le.status; + } + if let Some(re) = cause.downcast_ref::() { + if let Some(s) = re.status() { + return Some(s.as_u16()); + } + } + } + None +} diff --git a/crates/llm-client/src/openai.rs b/crates/llm-client/src/openai.rs index d44225d..d8c0b9f 100644 --- a/crates/llm-client/src/openai.rs +++ b/crates/llm-client/src/openai.rs @@ -176,10 +176,13 @@ impl ChatbotClient for OpenAiClient { let resp_text = http_resp.text().await?; if !status.is_success() { - return Err(anyhow::anyhow!( - "openai: HTTP {status} from {url}\nbody: {resp_text}", - url = self.url(), - )); + return Err(crate::LlmError { + status: Some(status.as_u16()), + message: format!( + "openai: HTTP {status} from {url}\nbody: {resp_text}", + url = self.url(), + ), + }.into()); } let resp: Value = serde_json::from_str(&resp_text) diff --git a/crates/skald-core/src/chatbot/mod.rs b/crates/skald-core/src/chatbot/mod.rs index aa983fe..b3d04c5 100644 --- a/crates/skald-core/src/chatbot/mod.rs +++ b/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, }; diff --git a/crates/skald-core/src/session/handler/agent_dispatch.rs b/crates/skald-core/src/session/handler/agent_dispatch.rs index 64de518..928dbef 100644 --- a/crates/skald-core/src/session/handler/agent_dispatch.rs +++ b/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>> = - 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 { + let persisted_grants = stack_mcp_grants::list_for_stack(&self.db, stack_id) + .await + .unwrap_or_default(); + let active_mcp_grants: Arc>> = + 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 { + 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 diff --git a/crates/skald-core/src/session/handler/approval.rs b/crates/skald-core/src/session/handler/approval.rs index 8505051..a931d2d 100644 --- a/crates/skald-core/src/session/handler/approval.rs +++ b/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 { - 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() } diff --git a/crates/skald-core/src/session/handler/config.rs b/crates/skald-core/src/session/handler/config.rs index d0314fb..d975e41 100644 --- a/crates/skald-core/src/session/handler/config.rs +++ b/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) diff --git a/crates/skald-core/src/session/handler/llm_call.rs b/crates/skald-core/src/session/handler/llm_call.rs index 2895f35..cc23963 100644 --- a/crates/skald-core/src/session/handler/llm_call.rs +++ b/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)); + } +} diff --git a/crates/skald-core/src/session/handler/resume.rs b/crates/skald-core/src/session/handler/resume.rs index 0e0f7f0..d10b053 100644 --- a/crates/skald-core/src/session/handler/resume.rs +++ b/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 { - 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; } diff --git a/crates/skald-core/src/tools/exec.rs b/crates/skald-core/src/tools/exec.rs index 640432e..23d3420 100644 --- a/crates/skald-core/src/tools/exec.rs +++ b/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 { - 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 { + 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> + 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> + 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 { - let (command, workdir, timeout_secs) = parse_args(args)?; - run(command, workdir, timeout_secs).await -} - -fn parse_args(args: &Value) -> Result<(String, Option, 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, timeout_secs: u64) -> Result { - // 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 { let mut child = cmd.spawn()?;