feat: implement subagent tool gating and timeout mechanisms for enhanced security and performance
This commit is contained in:
+27
-1
@@ -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 || {
|
||||
|
||||
@@ -894,6 +894,11 @@ fn archive_message(db: &Option<std::sync::Arc<std::sync::Mutex<rusqlite::Connect
|
||||
/// the agent gets stuck in a loop (e.g. an unachievable todo item).
|
||||
const MAX_TURN_STEPS: usize = 10000;
|
||||
|
||||
/// Hard wall-clock timeout per agent turn (5 minutes). Prevents a single
|
||||
/// user turn from running indefinitely even if the step budget isn't
|
||||
/// exhausted (e.g. slow LLM responses, stuck tool calls).
|
||||
const MAX_TURN_TIMEOUT_MS: u64 = 300_000;
|
||||
|
||||
/// Execute one full agent turn: stream the conversation to the LLM,
|
||||
/// handle tool calls, and loop until the LLM produces a non-tool response
|
||||
/// or runs out of unfinished todo items.
|
||||
@@ -924,7 +929,11 @@ fn run_agent_turn(
|
||||
let mut msgs = messages.to_vec();
|
||||
let mut edits_this_turn = 0u32;
|
||||
let mut prev_shaped = false;
|
||||
let turn_start_ms = std::time::Instant::now();
|
||||
|
||||
// Build system prompt components once and cache them for the entire turn
|
||||
// instead of regenerating on every loop iteration (which walks the full
|
||||
// workspace tree and reads all memory files each time).
|
||||
let tree_info = generate_workspace_tree(&tc.workspace_roots);
|
||||
let memory_section = build_memory_section(&tc.ctx.memory_dir);
|
||||
let system_text = format!(
|
||||
@@ -941,6 +950,8 @@ fn run_agent_turn(
|
||||
}
|
||||
|
||||
let mut turn_step = 0usize;
|
||||
let mut todo_retry_count = 0usize;
|
||||
const MAX_TODO_RETRIES: usize = 5;
|
||||
|
||||
loop {
|
||||
turn_step += 1;
|
||||
@@ -951,6 +962,13 @@ fn run_agent_turn(
|
||||
MAX_TURN_STEPS,
|
||||
);
|
||||
}
|
||||
if turn_start_ms.elapsed().as_millis() as u64 > 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;
|
||||
|
||||
+240
-7
@@ -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<String>) {
|
||||
(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<String> {
|
||||
// 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<SubagentEvent>) -> anyhow::Result<String> {
|
||||
let mut output = String::new();
|
||||
let mut messages: Vec<ChatMessage> = 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<SubagentEvent>) -> 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<SubagentEvent>) -> 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<SubagentEvent>) -> 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)),
|
||||
|
||||
@@ -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<String>) -> Self {
|
||||
self.allowed_tools = Some(tools);
|
||||
self
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<dyn Fn(String, AgentStatus) + Send + Sync>;
|
||||
/// `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<u64>,
|
||||
) -> anyhow::Result<String> {
|
||||
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::<anyhow::Result<String>>();
|
||||
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<Vec<String>>);
|
||||
/// `Arc<Mutex<Vec<String>>>` 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<String>` 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<Mutex<Vec<String>>>,
|
||||
timeout_ms: Option<u64>,
|
||||
) -> anyhow::Result<Vec<String>> {
|
||||
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::<String>();
|
||||
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, String>) -> 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<usize>,
|
||||
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();
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user