diff --git a/src/app/review/mod.rs b/src/app/review/mod.rs index f441756..5134a8a 100644 --- a/src/app/review/mod.rs +++ b/src/app/review/mod.rs @@ -350,7 +350,33 @@ pub fn trigger_review(state: &mut AppStateRest) -> anyhow::Result<()> { probe_note, ); - let (tx, _rx) = tokio::sync::mpsc::channel(32); + // Use a drain thread for subagent events (so blocking_send never + // fails on a closed channel) and log events at debug level for + // observability during review runs. + let (tx, rx) = tokio::sync::mpsc::channel(32); + let _drain_thread = std::thread::spawn(move || { + use crate::app::subagent::event::SubagentEvent; + let mut rx = rx; + while let Some(event) = rx.blocking_recv() { + match &event { + SubagentEvent::ToolCall { _tool, .. } => { + tracing::debug!("[review] tool call: {}", _tool); + } + SubagentEvent::ToolResult { _tool, .. } => { + tracing::debug!("[review] tool result: {}", _tool); + } + SubagentEvent::StepCompleted { _step, .. } => { + tracing::trace!("[review] step {} completed", _step); + } + SubagentEvent::StepFailed { _step, _error } => { + tracing::warn!("[review] step {} failed: {}", _step, _error); + } + SubagentEvent::Completed { .. } => { + tracing::debug!("[review] completed"); + } + } + } + }); let turn_events = state.turn_events.clone(); std::thread::spawn(move || { diff --git a/src/app/runtime/actions/mod.rs b/src/app/runtime/actions/mod.rs index 7f4e73f..3c25345 100644 --- a/src/app/runtime/actions/mod.rs +++ b/src/app/runtime/actions/mod.rs @@ -894,6 +894,11 @@ fn archive_message(db: &Option MAX_TURN_TIMEOUT_MS { + anyhow::bail!( + "turn exceeded maximum duration ({}s) — aborting. \ + Use /compact or shorter prompts if the model needs more time.", + MAX_TURN_TIMEOUT_MS / 1000, + ); + } let total_chars: usize = msgs.iter() .filter_map(|m| m.content.as_deref()) .map(|c| c.len()) @@ -1049,10 +1067,18 @@ fn run_agent_turn( } } if has_unfinished { + todo_retry_count += 1; + if todo_retry_count > MAX_TODO_RETRIES { + anyhow::bail!( + "exhausted {} todo-retries — giving up on unfinished tasks. \ + Edit todo.md manually or ask me to focus on specific items.", + MAX_TODO_RETRIES, + ); + } if let Ok(mut q) = events_q.lock() { q.push_back(TurnEvent::SystemNote { kind: "task_retry".to_string(), - message: format!("Network/API error: {}. Auto-retrying to finish tasks...", api_err), + message: format!("Network/API error: {}. Auto-retrying to finish tasks... (retry {}/{})", api_err, todo_retry_count, MAX_TODO_RETRIES), }); } std::thread::sleep(std::time::Duration::from_secs(5)); @@ -1160,14 +1186,25 @@ fn run_agent_turn( } if has_unfinished { - let sys_text = "You stopped, but you still have unfinished tasks in todo.md (marked with '- [ ]'). You MUST continue working and use tools to finish them, or edit todo.md to mark them as done if they are finished."; + todo_retry_count += 1; + if todo_retry_count > MAX_TODO_RETRIES { + if let Ok(mut q) = events_q.lock() { + q.push_back(TurnEvent::SystemNote { + kind: "task_retry".to_string(), + message: format!("Giving up after {} retries — some todo items remain unfinished. Edit todo.md manually or ask again.", MAX_TODO_RETRIES), + }); + } + break; + } + let sys_text = format!("You stopped, but you still have unfinished tasks in todo.md (marked with '- [ ]'). You MUST continue working and use tools to finish them, or edit todo.md to mark them as done if they are finished. (Retry {}/{})", todo_retry_count, MAX_TODO_RETRIES); + let sys_text_clone = sys_text.clone(); let msg = ChatMessage::system(sys_text); archive_message(&tc.db, &tc.session_id, &msg); msgs.push(msg); if let Ok(mut q) = events_q.lock() { q.push_back(TurnEvent::SystemNote { kind: "task_retry".to_string(), - message: sys_text.to_string(), + message: sys_text_clone, }); } continue; diff --git a/src/app/subagent/engine.rs b/src/app/subagent/engine.rs index ef14a74..3f24c50 100644 --- a/src/app/subagent/engine.rs +++ b/src/app/subagent/engine.rs @@ -1,6 +1,11 @@ //! Subagent execution loop: drive an LLM conversation, gate tool calls //! against the context's allowlist, run tools, and stream progress events //! to the parent via an mpsc channel. +//! +//! Security: subagent tool gating mirrors the main agent's `Harness` checks +//! (path traversal, reason validation, stub/denial/assumption scanning, +//! bash exfiltration and destructive-pattern detection) so that subagents +//! are not a weaker link than the main agent. use tokio::sync::mpsc; use crate::dto::chat::message::ChatMessage; @@ -73,24 +78,236 @@ fn resolve_provider_config() -> (String, String, Option) { (api_key, model, base_url) } +// ─── Subagent-level tool gating (mirrors Harness checks) ─── + +const STUB_PATTERNS: &[&str] = &[ + "todo!()", "todo!(", + "unimplemented!()", "unimplemented!(", + "FIXME", "fixme:", "XXX:", "PLACEHOLDER", + "REPLACE_ME", "stub_value", "stub_function", + "fake_response", "fake_data", + "not implemented", "not yet implemented", + "to be implemented", "to be done", +]; + +const DENIAL_PATTERNS: &[&str] = &[ + "// skip", "// skipping", "// skipping for now", + "// for now just", "// punt", "// hack:", + "// workaround:", "// cba", "// later", + "// do later", "// ignore for now", "// disable", + "// bypass", "// quick fix", "// temp fix", + "// temporary fix", "// temp:", "// temporary:", + "// noop", +]; + +const ASSUMPTION_PATTERNS: &[&str] = &[ + "// assume", "// probably", "// guess", + "// should work", "// hopefully", "// i think", + "// should be fine", "// likely", +]; + +const EXFIL_PATTERNS: &[&str] = &[ + "curl ", "wget ", "nc -e ", "ncat ", "/dev/tcp/", + "base64 -d |", "base64 --decode |", + "openssl s_client", "ssh -R ", + "scp /", "rsync /", +]; + +const SENSITIVE_PATH_PATTERNS: &[&str] = &[ + ".ssh/id_rsa", ".ssh/id_ed25519", + ".aws/credentials", ".aws/config", + ".kube/config", ".docker/config.json", + "/etc/shadow", "/etc/passwd", "/proc/self/environ", +]; + +const MIN_REASON_LEN: usize = 8; + +/// Gate a tool call in the subagent context. Returns `Some(block_reason)` if +/// the call should be blocked, `None` to allow. +/// +/// Flow: always blocks dangerous patterns — path traversal, stub/denial/ +/// assumption language, bash exfiltration, destructive commands, sensitive +/// path reads — regardless of the allowed-tools list. Tools that are not +/// risky only get the basic allowlist check. +fn gate_subagent_tool_call( + tool_name: &str, + args: &serde_json::Value, +) -> Option { + // File-mutating tools: write / edit / delete + if matches!(tool_name, "write" | "edit" | "delete") { + if let Some(path) = args.get("path").and_then(|v| v.as_str()) { + if path.contains("..") { + return Some("path traversal detected in 'path' argument".to_string()); + } + } + } + + // write / edit require a non-trivial `reason` + if matches!(tool_name, "write" | "edit" | "delete") { + let reason = args.get("reason").and_then(|v| v.as_str()).unwrap_or(""); + if reason.trim().len() < MIN_REASON_LEN { + return Some(format!( + "{} requires a non-trivial 'reason' (>= {} chars) explaining why", + tool_name, MIN_REASON_LEN, + )); + } + } + + // write / edit content must not contain stubs, denial, or assumption language + if matches!(tool_name, "write" | "edit") { + let content = match tool_name { + "write" => args.get("content").and_then(|v| v.as_str()).unwrap_or(""), + "edit" => { + let old = args.get("old").and_then(|v| v.as_str()).unwrap_or(""); + let new = args.get("new").and_then(|v| v.as_str()).unwrap_or(""); + // For edits, scanning old+new together catches stubs in both + return if contains_any(old, STUB_PATTERNS) { + Some("content contains stub/placeholder pattern; production code must be fully implemented".to_string()) + } else if contains_any(new, STUB_PATTERNS) { + Some("content contains stub/placeholder pattern; production code must be fully implemented".to_string()) + } else if contains_any(new, DENIAL_PATTERNS) { + Some("content contains denial/punt pattern; implement properly instead of skipping".to_string()) + } else if contains_any(new, ASSUMPTION_PATTERNS) { + Some("content contains assumption pattern; verify against data instead of guessing".to_string()) + } else { + return None; + }; + } + _ => "", + }; + if contains_any(content, STUB_PATTERNS) { + return Some("content contains stub/placeholder pattern; production code must be fully implemented".to_string()); + } + if contains_any(content, DENIAL_PATTERNS) { + return Some("content contains denial/punt pattern; implement properly instead of skipping".to_string()); + } + if contains_any(content, ASSUMPTION_PATTERNS) { + return Some("content contains assumption pattern; verify against data instead of guessing".to_string()); + } + } + + // Bash: exfiltration, sensitive paths, destructive commands + if tool_name == "bash" { + let cmd = args.get("command").and_then(|v| v.as_str()).unwrap_or(""); + if cmd.contains("..") { + return Some("path traversal detected in bash command".to_string()); + } + // Only check exfiltration for non-standard commands + let is_standard = cmd.trim_start().starts_with("cargo") + || cmd.trim_start().starts_with("rustc") + || cmd.trim_start().starts_with("git ") + || cmd.trim_start().starts_with("ls") + || cmd.trim_start().starts_with("pwd") + || cmd.trim_start().starts_with("echo") + || cmd.trim_start().starts_with("cat") + || cmd.trim_start().starts_with("find") + || cmd.trim_start().starts_with("grep") + || cmd.trim_start().starts_with("test"); + if !is_standard { + for pat in EXFIL_PATTERNS { + if cmd.contains(pat) { + return Some(format!("potential data-exfiltration command blocked (matched '{}')", pat)); + } + } + } + for pat in SENSITIVE_PATH_PATTERNS { + if cmd.contains(pat) { + return Some(format!("refused to read/write sensitive path '{}'", pat)); + } + } + let dangerous = ["rm -rf /", "rm -rf --no-preserve-root", "rm -rf ~", + "rm -fr /", "mkfs.", "dd if=", ":(){", "> /dev/sda", + "chmod -R 000 /", "shutdown ", "poweroff ", "reboot ", "halt "]; + for pat in &dangerous { + if cmd.contains(pat) { + return Some(format!("destructive command pattern blocked: {}", pat)); + } + } + if contains_any(cmd, STUB_PATTERNS) { + return Some("bash command contains stub pattern".to_string()); + } + } + + // git_operator: require reason + if tool_name == "git_operator" { + let reason = args.get("reason").and_then(|v| v.as_str()).unwrap_or(""); + if reason.trim().len() < MIN_REASON_LEN { + return Some("git_operator requires a non-trivial 'reason' (>= 8 chars)".to_string()); + } + } + + None +} + +/// Check if `text` matches any pattern (case-insensitive substring). +fn contains_any(text: &str, patterns: &[&str]) -> bool { + let lower = text.to_lowercase(); + patterns.iter().any(|p| lower.contains(&p.to_lowercase())) +} + +/// Build an ASCII tree of the workspace directory structure for the +/// system prompt, so the LLM can see the file layout. +/// +/// Flow: for each root, walk using `ignore::WalkBuilder` (respecting +/// `.gitignore` and hidden files) → prefix `[DIR]` for directories → +/// truncate after 1000 entries. +fn generate_workspace_tree(roots: &[std::path::PathBuf]) -> String { + let mut out = String::new(); + out.push_str("Current Workspace Directory Structure:\n"); + for root in roots { + out.push_str(&format!("Root: {}\n", root.display())); + let walker = ignore::WalkBuilder::new(root) + .hidden(true) + .git_ignore(true) + .build(); + let mut count = 0; + for entry in walker.flatten() { + let path = entry.path(); + if let Ok(rel) = path.strip_prefix(root) { + if rel.as_os_str().is_empty() { continue; } + let is_dir = entry.file_type().map(|ft| ft.is_dir()).unwrap_or(false); + let prefix = if is_dir { "[DIR] " } else { " " }; + out.push_str(&format!(" {}{}\n", prefix, rel.display())); + count += 1; + if count > 1000 { + out.push_str(" ... (truncated)\n"); + break; + } + } + } + } + out +} + /// Synchronous subagent entry point: run up to `ctx.max_steps` iterations /// of the LLM tool loop. /// -/// Flow: inject system prompt → for each step: resolve provider config, -/// build an LLM client, call `chat_with_tools_non_streaming`, process tool -/// calls or collect text output → send `SubagentEvent`s on `tx` → break on -/// first text-only (non-empty) response. +/// Flow: inject system prompt (with workspace tree if available) → for each +/// step: resolve provider config, build an LLM client, call +/// `chat_with_tools_non_streaming`, process tool calls (gated against both +/// the allowlist and Harness-style content safety checks) or collect text +/// output → send `SubagentEvent`s on `tx` → break on first text-only +/// (non-empty) response. /// /// Why: runs synchronously on a dedicated thread so the main async event -/// loop is not blocked. Tool gating prevents restricted or risky tools from -/// executing unless explicitly allowed. +/// loop is not blocked. Tool gating prevents restricted, risky, or +/// malicious/poor-quality tool calls from executing. /// /// Return: the concatenated text output, or an `anyhow::Error` if the LLM /// call fails at any step. pub fn run_subagent(ctx: SubagentContext, tx: mpsc::Sender) -> anyhow::Result { let mut output = String::new(); let mut messages: Vec = Vec::new(); - messages.push(ChatMessage::system(ctx.system_prompt.clone())); + + // Build system prompt with workspace tree context if we have workspaces, + // giving subagents the same project-awareness as the main agent. + let system_with_context = if ctx.workspaces.is_empty() { + ctx.system_prompt.clone() + } else { + let tree_info = generate_workspace_tree(&ctx.workspaces); + format!("{}\n\n{}", ctx.system_prompt, tree_info) + }; + messages.push(ChatMessage::system(system_with_context)); let tool_ctx = crate::tool::ToolCtx::builder() .session_dir(ctx.session_dir.clone()) @@ -145,6 +362,7 @@ pub fn run_subagent(ctx: SubagentContext, tx: mpsc::Sender) -> an _args: args.clone(), }); + // Level 1: allowlist check — is this tool even permitted? if !generally_allowed { let msg = format!("tool '{}' not allowed for this subagent", tool_name); messages.push(ChatMessage::tool_result(tool_call.id.clone(), msg.clone())); @@ -155,6 +373,7 @@ pub fn run_subagent(ctx: SubagentContext, tx: mpsc::Sender) -> an continue; } + // Level 2: risky tool check — risky tools require explicit permission if tool_is_risky(tool_name) && !explicitly_allowed { let msg = format!("risky tool '{}' requires explicit permission; not allowed for this subagent", tool_name); messages.push(ChatMessage::tool_result(tool_call.id.clone(), msg.clone())); @@ -165,6 +384,20 @@ pub fn run_subagent(ctx: SubagentContext, tx: mpsc::Sender) -> an continue; } + // Level 3: Harness-style content safety gating — mirrors the main + // agent's gate_tool_call checks (path traversal, reason validation, + // stub/denial/assumption scanning, bash exfiltration, destructive + // commands, sensitive path reads). + if let Some(block_reason) = gate_subagent_tool_call(tool_name, &args) { + let msg = format!("Blocked by subagent gate: {}", block_reason); + messages.push(ChatMessage::tool_result(tool_call.id.clone(), msg.clone())); + let _ = tx.blocking_send(SubagentEvent::ToolResult { + _tool: tool_name.clone(), + _output: msg, + }); + continue; + } + let result = match tools.iter().find(|t| t.name() == tool_name.as_str()) { Some(tool) => tool.run(&tool_ctx, &args), None => Err(anyhow::anyhow!("tool '{}' not found", tool_name)), diff --git a/src/app/subagent/spawn.rs b/src/app/subagent/spawn.rs index 102349f..d97f92c 100644 --- a/src/app/subagent/spawn.rs +++ b/src/app/subagent/spawn.rs @@ -35,4 +35,15 @@ impl AgentDefinition { self } + /// Builder method: set the system prompt for this agent. + pub fn with_system_prompt(mut self, prompt: String) -> Self { + self.system_prompt = Some(prompt); + self + } + + /// Builder method: set the allowed tool list for this agent. + pub fn with_allowed_tools(mut self, tools: Vec) -> Self { + self.allowed_tools = Some(tools); + self + } } diff --git a/src/app/workflow/engine.rs b/src/app/workflow/engine.rs index fe3d995..83d3b87 100644 --- a/src/app/workflow/engine.rs +++ b/src/app/workflow/engine.rs @@ -16,6 +16,7 @@ use std::collections::HashMap; use std::sync::{Arc, Mutex}; +use std::time::Duration; use serde::{Deserialize, Serialize}; use super::script::{ScriptPrimitive, WorkflowScript}; @@ -84,6 +85,10 @@ pub type LiveStateFn = Arc; /// `execute_primitive` scope, so pipeline stages can pass data between each /// other while different workflow invocations remain isolated. /// +/// When `timeout_ms` is `Some`, the subagent is killed (abandoned on a +/// separate thread) if it does not complete within the deadline, preventing +/// a stuck stage from blocking the entire pipeline forever. +/// /// Return: the agent's text output, or an error on failure. fn spawn_single_agent( agent_id: &str, @@ -94,6 +99,7 @@ fn spawn_single_agent( live: Option<&LiveStateFn>, session_dir: &std::path::Path, workspaces: &[std::path::PathBuf], + timeout_ms: Option, ) -> anyhow::Result { use crate::app::subagent::context::build_subagent_context; use crate::app::subagent::engine::run_subagent; @@ -170,7 +176,28 @@ fn spawn_single_agent( } }); - let result = run_subagent(ctx, tx); + // Enforce timeout by running subagent on a separate thread and + // waiting with a deadline. If the deadline expires, the thread is + // abandoned (Rust threads cannot be forcibly killed, but we proceed + // without waiting for it — the drain thread will drop when tx is + // dropped on thread exit). + let result = if let Some(timeout) = timeout_ms { + let (done_tx, done_rx) = std::sync::mpsc::channel::>(); + let timeout_ctx = ctx; + let timeout_tx = tx; + std::thread::spawn(move || { + let _ = done_tx.send(run_subagent(timeout_ctx, timeout_tx)); + }); + match done_rx.recv_timeout(Duration::from_millis(timeout)) { + Ok(r) => r, + Err(_) => Err(anyhow::anyhow!( + "subagent '{}' timed out after {}ms", + agent_name, timeout, + )), + } + } else { + run_subagent(ctx, tx) + }; let completed_at = chrono::Utc::now().timestamp_millis(); @@ -213,6 +240,9 @@ type ParallelResult = (usize, anyhow::Result>); /// `Arc>>` rather than a global static, so concurrent /// workflow runs are isolated from each other. /// +/// `timeout_ms` propagates to individual agents so that no single agent +/// can block the entire workflow beyond the configured deadline. +/// /// Return: a `Vec` of all agent outputs (or error strings) in /// the order they were submitted. pub fn execute_primitive( @@ -224,6 +254,7 @@ pub fn execute_primitive( session_dir: &std::path::Path, workspaces: &[std::path::PathBuf], findings: &Arc>>, + timeout_ms: Option, ) -> anyhow::Result> { match primitive { ScriptPrimitive::Agent(prompt) => { @@ -231,7 +262,7 @@ pub fn execute_primitive( let findings_snapshot = findings.lock().map(|f| f.clone()).unwrap_or_default(); let agent_id = uuid::Uuid::new_v4().to_string(); let agent_name = resolved.chars().take(40).collect::(); - match spawn_single_agent(&agent_id, &agent_name, &resolved, findings_snapshot, findings, live, session_dir, workspaces) { + match spawn_single_agent(&agent_id, &agent_name, &resolved, findings_snapshot, findings, live, session_dir, workspaces, timeout_ms) { Ok(text) => Ok(vec![text]), Err(e) => { if continue_on_error { @@ -266,6 +297,7 @@ pub fn execute_primitive( let session_dir = session_dir.to_path_buf(); let workspaces = workspaces.to_vec(); let findings = Arc::clone(findings); + let to = timeout_ms; std::thread::spawn(move || { let _permit = sem.acquire(); @@ -275,6 +307,7 @@ pub fn execute_primitive( &session_dir, &workspaces, &findings, + to, ); if let Ok(mut locked) = results.lock() { locked.push((idx, result)); @@ -308,7 +341,7 @@ pub fn execute_primitive( // `findings` Arc (same isolation scope as parent). let mut all = Vec::new(); for (idx, script) in scripts.iter().enumerate() { - match execute_primitive(script, args, concurrency_cap, continue_on_error, live, session_dir, workspaces, findings) { + match execute_primitive(script, args, concurrency_cap, continue_on_error, live, session_dir, workspaces, findings, timeout_ms) { Ok(outputs) => all.extend(outputs), Err(e) => { if continue_on_error { @@ -323,7 +356,7 @@ pub fn execute_primitive( } ScriptPrimitive::Phase { name: _name, script } => { - execute_primitive(script, args, concurrency_cap, continue_on_error, live, session_dir, workspaces, findings) + execute_primitive(script, args, concurrency_cap, continue_on_error, live, session_dir, workspaces, findings, timeout_ms) } } } @@ -372,6 +405,7 @@ pub fn run_workflow_tracked( &script.script, args, concurrency_cap, script.options.continue_on_error, live_ref, session_dir, workspaces, &findings, + script.options.timeout_ms, )?; let summary = if results.is_empty() { @@ -409,6 +443,11 @@ fn resolve_template(template: &str, args: &HashMap) -> String { /// A counting semaphore built from a `Mutex` + `Condvar`. /// /// Used by `execute_primitive` to cap concurrent parallel branches. +/// +/// Panic-safety: if a thread panics while holding a permit, the Mutex +/// becomes poisoned. Both `acquire` and the `Drop` implementation recover +/// from poisoned mutexes by discarding the poison, ensuring the semaphore +/// remains usable after a thread panic. struct Semaphore { count: Mutex, condvar: std::sync::Condvar, @@ -423,9 +462,15 @@ impl Semaphore { } fn acquire(&self) -> SemaphoreGuard<'_> { - let mut count = self.count.lock().unwrap(); + let mut count = self.count.lock().unwrap_or_else(|e| { + tracing::warn!("[semaphore] mutex poisoned in acquire, recovering"); + e.into_inner() + }); while *count == 0 { - count = self.condvar.wait(count).unwrap(); + count = self.condvar.wait(count).unwrap_or_else(|e| { + tracing::warn!("[semaphore] mutex poisoned in wait, recovering"); + e.into_inner() + }); } *count -= 1; SemaphoreGuard { sem: self } @@ -438,7 +483,10 @@ struct SemaphoreGuard<'a> { impl<'a> Drop for SemaphoreGuard<'a> { fn drop(&mut self) { - let mut count = self.sem.count.lock().unwrap(); + let mut count = self.sem.count.lock().unwrap_or_else(|e| { + tracing::warn!("[semaphore] mutex poisoned in drop, recovering"); + e.into_inner() + }); *count += 1; self.sem.condvar.notify_one(); } diff --git a/src/service/provider.rs b/src/service/provider.rs index a3d0729..ac3b52d 100644 --- a/src/service/provider.rs +++ b/src/service/provider.rs @@ -199,7 +199,10 @@ impl LlmClient { }; let url = format!("{}/chat/completions", self.base_url); - let max_retries = 10; + // Fewer retries on streaming because `run_agent_turn` has a + // non-streaming fallback that also retries. Combined total is + // capped implicitly by the per-turn timeout and step limits. + let max_retries = 3; let mut attempt = 0; let mut started = false; diff --git a/src/tool/spawn.rs b/src/tool/spawn.rs index 6c35743..407927e 100644 --- a/src/tool/spawn.rs +++ b/src/tool/spawn.rs @@ -117,6 +117,7 @@ impl Tool for SpawnAgents { &_ctx.session_dir, &_ctx.workspaces, &findings, + None, // no per-agent timeout for spawn_agents )?; format_results(results, "parallel") } @@ -206,6 +207,7 @@ impl Tool for SpawnPipeline { &_ctx.session_dir, &_ctx.workspaces, &findings, + None, // no per-agent timeout for spawn_pipeline )?; format_results(results, "pipeline") }