Eliminate ~500 lines of duplicate code across 31 files by extracting shared functions, helpers, and consolidating repeated patterns. Highlights: - Toast helpers (toast_info/success/warning/error) on AppStateRest - push_event() helper for turn-event queue (19 callers consolidated) - log_write_edit_tool() shared fn (turn.rs + engine.rs ~50 lines saved) - resolve_api_key() shared fn (spawn.rs + provider.rs) - LSP call_positional() helper on LspClient - lsp_cursor_params() shared schema for 4 tool files -overlay_block() helper for consistent overlay title/border styling - cycle_selected_index(), path_not_found/a_directory() helpers - mark_dirty(), save_settings() on AppStateRest - Remove redundant Err(e) => Err(e) arms in LSP tools - Consolidate generate_workspace_tree (turn.rs → workspace.rs) - Simplify background-review wrapper args in auto/mod.rs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
446 lines
16 KiB
Rust
446 lines
16 KiB
Rust
//! Auto-subagent orchestration: the main agent automatically delegates
|
|
//! review, test-generation, architecture-review, and security-review tasks
|
|
//! to subagents without requiring explicit tool calls from the LLM.
|
|
//!
|
|
//! Two modes:
|
|
//! - **Inline** (`spawn_quick_review`): runs synchronously within the turn
|
|
//! after each write/edit tool call. Results are fed back into the LLM
|
|
//! conversation so the agent can act on feedback immediately.
|
|
//! - **Background** (`spawn_background_*`): runs asynchronously on a
|
|
//! dedicated OS thread at the end of a turn. Reports results via
|
|
//! `TurnEvent::SystemNote`, consumed by the TUI on the next Tick.
|
|
//!
|
|
//! Why inline vs background:
|
|
//! - Inline reviews give the agent an immediate feedback loop ("I just
|
|
//! wrote this file, let me check if it's correct before continuing").
|
|
//! - Background reviews catch broader concerns (missing tests, architectural
|
|
//! drift, security issues) without blocking the main agent's flow.
|
|
pub(crate) mod paths;
|
|
|
|
pub use paths::is_reviewable_path;
|
|
pub(crate) use paths::is_production_code;
|
|
|
|
use crate::app::state::runtime::TurnEvent;
|
|
use crate::app::subagent::context::build_subagent_context;
|
|
use crate::app::subagent::engine::run_subagent;
|
|
use crate::app::subagent::event::SubagentEvent;
|
|
use crate::app::subagent::spawn::{spawn_subagent_with_drain, AgentDefinition};
|
|
use std::collections::VecDeque;
|
|
use std::path::Path;
|
|
use std::sync::atomic::{AtomicBool, Ordering};
|
|
use std::sync::{Arc, Mutex};
|
|
|
|
/// Prevents a second background subagent of the same kind from spawning
|
|
/// while one is already in flight. Without this, a chatty multi-turn edit
|
|
/// session could stack overlapping test-gen/arch/security reviews of
|
|
/// overlapping file sets, none of which could be told apart in the
|
|
/// `SystemNote` toast stream.
|
|
static TEST_GEN_RUNNING: AtomicBool = AtomicBool::new(false);
|
|
static ARCH_REVIEW_RUNNING: AtomicBool = AtomicBool::new(false);
|
|
static SECURITY_REVIEW_RUNNING: AtomicBool = AtomicBool::new(false);
|
|
|
|
/// RAII guard that resets a per-kind overlap flag back to `false` on drop —
|
|
/// including during a panic-triggered unwind inside the spawned thread — so
|
|
/// a background review can never wedge itself permanently disabled for the
|
|
/// rest of the process if the subagent run panics before reaching its
|
|
/// normal completion path.
|
|
struct RunningGuard(&'static AtomicBool);
|
|
|
|
impl Drop for RunningGuard {
|
|
fn drop(&mut self) {
|
|
self.0.store(false, Ordering::SeqCst);
|
|
}
|
|
}
|
|
|
|
/// ─── Helpers ───
|
|
|
|
/// Derive a human-readable message prefix from the internal kind label.
|
|
/// Derive a human-readable message prefix from the internal kind label.
|
|
///
|
|
/// Production callers always pass one of the three known labels
|
|
/// (`"bg-test-gen"`, `"bg-arch-review"`, `"bg-security-review"`).
|
|
fn message_prefix(kind: &str) -> &'static str {
|
|
match kind {
|
|
"bg-test-gen" => "Auto test-gen",
|
|
"bg-arch-review" => "Architecture review",
|
|
"bg-security-review" => "Security review",
|
|
other => {
|
|
// Production callers always use one of the three known labels.
|
|
// This path is a safety net only.
|
|
debug_assert!(false, "unknown background review kind: {other}");
|
|
""
|
|
}
|
|
}
|
|
}
|
|
|
|
/// ─── Inline Quick Review (synchronous, feeds back to LLM) ───
|
|
///
|
|
/// Spawn a lightweight inline code review subagent for the given file.
|
|
///
|
|
/// The subagent reads the file (read-only), checks for common issues,
|
|
/// and returns a concise text verdict. This runs synchronously so the
|
|
/// main agent's `run_agent_turn` can inject the result back into the
|
|
/// LLM conversation for immediate action.
|
|
///
|
|
/// Returns `Ok(verdict)` if the review completed, or an error if the
|
|
/// subagent could not be spawned or failed internally. Callers should
|
|
/// log and swallow errors gracefully — a failed inline review should
|
|
/// never interrupt the main agent's flow.
|
|
pub fn spawn_quick_review(
|
|
file_path: &str,
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
) -> anyhow::Result<String> {
|
|
let prompt = format!(
|
|
"{}\n\nFile to review: {}",
|
|
crate::prompts::AUTO_REVIEWER_PROMPT,
|
|
file_path,
|
|
);
|
|
|
|
let def = AgentDefinition::new("quick-reviewer".to_string(), "reviewer".to_string())
|
|
.with_system_prompt(prompt);
|
|
|
|
let mut ctx = build_subagent_context(&def);
|
|
ctx.session_dir = session_dir.to_path_buf();
|
|
ctx.workspaces = workspaces.to_vec();
|
|
|
|
let (tx, _drain) = spawn_subagent_with_drain(|event| {
|
|
match &event {
|
|
SubagentEvent::ToolCall { tool, .. } => {
|
|
tracing::debug!("[auto-review] tool call: {}", tool);
|
|
}
|
|
SubagentEvent::ToolResult { tool, .. } => {
|
|
tracing::debug!("[auto-review] tool result: {}", tool);
|
|
}
|
|
SubagentEvent::Completed => {
|
|
tracing::debug!("[auto-review] completed");
|
|
}
|
|
_ => {}
|
|
}
|
|
});
|
|
|
|
let verdict = run_subagent(&ctx, &tx)?;
|
|
tracing::info!(
|
|
"[auto-review] quick review for '{}': {}",
|
|
file_path,
|
|
verdict.lines().next().unwrap_or(&verdict),
|
|
);
|
|
Ok(verdict)
|
|
}
|
|
|
|
/// ─── Background Subagent Spawners (async, report via `SystemNote`) ───
|
|
///
|
|
/// Run a subagent built from `def`, retrying once if the first attempt
|
|
/// fails. Background subagents call this instead of running once and
|
|
/// silently swallowing the error into a note string, so a single transient
|
|
/// LLM/tool failure doesn't just disappear.
|
|
///
|
|
/// `abort_flag` is checked before every attempt (including the first) and
|
|
/// forwarded into the subagent's own context, so a cancelled turn stops
|
|
/// retrying immediately instead of burning a second attempt.
|
|
///
|
|
/// Return: `Ok(output)` if either attempt succeeded, `Err(message)`
|
|
/// describing the final failure if both attempts failed, or the literal
|
|
/// message `"aborted by user"` if `abort_flag` was already set before an
|
|
/// attempt could start.
|
|
fn run_subagent_with_retry(
|
|
def: &AgentDefinition,
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
label: &str,
|
|
abort_flag: Option<&Arc<AtomicBool>>,
|
|
) -> Result<String, String> {
|
|
let mut last_err = String::new();
|
|
for attempt in 1..=2 {
|
|
if abort_flag.is_some_and(|f| f.load(Ordering::SeqCst)) {
|
|
return Err("aborted by user".to_string());
|
|
}
|
|
let mut ctx = build_subagent_context(def);
|
|
ctx.session_dir = session_dir.to_path_buf();
|
|
ctx.workspaces = workspaces.to_vec();
|
|
ctx.abort_flag = abort_flag.cloned();
|
|
|
|
let drain_label = label.to_string();
|
|
let (tx, _drain) = spawn_subagent_with_drain(move |event| {
|
|
if let SubagentEvent::StepFailed { step, error } = &event {
|
|
tracing::warn!("[{drain_label}] step {step} failed: {error}");
|
|
}
|
|
});
|
|
|
|
match run_subagent(&ctx, &tx) {
|
|
Ok(output) => return Ok(output),
|
|
Err(e) => {
|
|
tracing::warn!("[{label}] attempt {attempt}/2 failed: {e}");
|
|
last_err = e.to_string();
|
|
}
|
|
}
|
|
}
|
|
Err(format!("failed after 2 attempts: {last_err}"))
|
|
}
|
|
|
|
/// ─── Generic background review spawner ───
|
|
///
|
|
/// Runs a subagent in a background OS thread, gated by `running_flag` so
|
|
/// only one instance of a given kind can be in flight at a time. Reports
|
|
/// completion via a `TurnEvent::SystemNote` pushed to `turn_events`.
|
|
///
|
|
/// `kind` is the internal label used for logging and the `SystemNote` kind
|
|
/// (e.g. `"bg-test-gen"`, `"bg-arch-review"`). The human-readable message
|
|
/// prefix is derived from this label via [`message_prefix`].
|
|
fn spawn_background_review(
|
|
kind: &str,
|
|
running_flag: &'static AtomicBool,
|
|
prompt_constant: &str,
|
|
agent_name: &str,
|
|
agent_role: &str,
|
|
file_paths: Vec<String>,
|
|
session_dir: std::path::PathBuf,
|
|
workspaces: Vec<std::path::PathBuf>,
|
|
turn_events: Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) {
|
|
if file_paths.is_empty() {
|
|
return;
|
|
}
|
|
if running_flag
|
|
.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst)
|
|
.is_err()
|
|
{
|
|
tracing::debug!("[{kind}] skipped — a {kind} run is already in flight");
|
|
return;
|
|
}
|
|
|
|
let sd = session_dir;
|
|
let ws = workspaces;
|
|
let events = turn_events;
|
|
let prompt_text = format!(
|
|
"{}\n\nModified files:\n{}",
|
|
prompt_constant,
|
|
file_paths.join("\n"),
|
|
);
|
|
let label = kind.to_string();
|
|
let agent_name = agent_name.to_string();
|
|
let agent_role = agent_role.to_string();
|
|
let prefix = message_prefix(kind);
|
|
|
|
std::thread::spawn(move || {
|
|
let _running_guard = RunningGuard(running_flag);
|
|
tracing::info!("[{label}] spawning for {} file(s)", file_paths.len());
|
|
|
|
let def = AgentDefinition::new(agent_name, agent_role).with_system_prompt(prompt_text);
|
|
|
|
let result = run_subagent_with_retry(&def, &sd, &ws, &label, Some(&abort_flag));
|
|
let message = match &result {
|
|
Ok(output) => {
|
|
let first = output.lines().next().unwrap_or(output);
|
|
format!("{prefix}: {first}")
|
|
}
|
|
Err(e) if e.contains("aborted") => format!("{prefix} cancelled: {e}"),
|
|
Err(e) => format!("ESCALATED: {prefix} {e}"),
|
|
};
|
|
|
|
if let Ok(mut q) = events.lock() {
|
|
q.push_back(TurnEvent::SystemNote {
|
|
kind: label,
|
|
message,
|
|
});
|
|
}
|
|
});
|
|
}
|
|
|
|
/// Collect the trailing arguments shared by all background-review spawners.
|
|
fn review_args<'a>(
|
|
file_paths: &'a [String],
|
|
session_dir: &'a Path,
|
|
workspaces: &'a [std::path::PathBuf],
|
|
turn_events: &'a Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) -> (Vec<String>, std::path::PathBuf, Vec<std::path::PathBuf>, Arc<Mutex<VecDeque<TurnEvent>>>, Arc<AtomicBool>) {
|
|
(
|
|
file_paths.to_vec(),
|
|
session_dir.to_path_buf(),
|
|
workspaces.to_vec(),
|
|
turn_events.clone(),
|
|
abort_flag,
|
|
)
|
|
}
|
|
|
|
/// Spawn a background subagent that generates tests for modified files.
|
|
pub fn spawn_background_test_gen(
|
|
file_paths: &[String],
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
turn_events: &Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) {
|
|
let (fps, sd, ws, te, af) = review_args(file_paths, session_dir, workspaces, turn_events, abort_flag);
|
|
spawn_background_review(
|
|
"bg-test-gen", &TEST_GEN_RUNNING,
|
|
crate::prompts::TEST_GENERATOR_PROMPT, "test-generator", "coder",
|
|
fps, sd, ws, te, af,
|
|
);
|
|
}
|
|
|
|
/// Spawn a background architecture-review subagent.
|
|
pub fn spawn_background_arch_review(
|
|
file_paths: &[String],
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
turn_events: &Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) {
|
|
let (fps, sd, ws, te, af) = review_args(file_paths, session_dir, workspaces, turn_events, abort_flag);
|
|
spawn_background_review(
|
|
"bg-arch-review", &ARCH_REVIEW_RUNNING,
|
|
crate::prompts::ARCH_REVIEWER_PROMPT, "arch-reviewer", "reviewer",
|
|
fps, sd, ws, te, af,
|
|
);
|
|
}
|
|
|
|
/// Spawn a background security-review subagent.
|
|
///
|
|
/// Only reviews production code files for security — test files and
|
|
/// config files are out of scope for security review.
|
|
pub fn spawn_background_security_review(
|
|
file_paths: &[String],
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
turn_events: &Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) {
|
|
let (_, sd, ws, te, af) = review_args(file_paths, session_dir, workspaces, turn_events, abort_flag);
|
|
let prod_paths: Vec<String> = file_paths
|
|
.iter()
|
|
.filter(|p| is_production_code(p))
|
|
.cloned()
|
|
.collect();
|
|
spawn_background_review(
|
|
"bg-security-review", &SECURITY_REVIEW_RUNNING,
|
|
crate::prompts::SECURITY_REVIEWER_PROMPT, "security-reviewer", "reviewer",
|
|
prod_paths, sd, ws, te, af,
|
|
);
|
|
}
|
|
|
|
/// Convenience: spawn all applicable background subagents for a set of edited
|
|
/// file paths. Called once at the end of a main agent turn.
|
|
///
|
|
/// Flow: always spawns arch-review and security-review if there are
|
|
/// reviewable production files → spawns test-gen only if there are source
|
|
/// files that aren't already tests.
|
|
///
|
|
/// `abort_flag` is cloned and forwarded to all three spawn calls so a
|
|
/// single cancellation source stops every kind of background review.
|
|
pub fn spawn_all_background(
|
|
file_paths: &[String],
|
|
session_dir: &Path,
|
|
workspaces: &[std::path::PathBuf],
|
|
turn_events: &Arc<Mutex<VecDeque<TurnEvent>>>,
|
|
abort_flag: Arc<AtomicBool>,
|
|
) {
|
|
if file_paths.is_empty() {
|
|
return;
|
|
}
|
|
|
|
// Background test-gen: only for non-test source files
|
|
let source_paths: Vec<String> = file_paths
|
|
.iter()
|
|
.filter(|p| is_production_code(p))
|
|
.cloned()
|
|
.collect();
|
|
spawn_background_test_gen(
|
|
&source_paths,
|
|
session_dir,
|
|
workspaces,
|
|
turn_events,
|
|
abort_flag.clone(),
|
|
);
|
|
|
|
// Background arch review: for all files that are reviewable
|
|
let reviewable: Vec<String> = file_paths
|
|
.iter()
|
|
.filter(|p| is_reviewable_path(p))
|
|
.cloned()
|
|
.collect();
|
|
spawn_background_arch_review(
|
|
&reviewable,
|
|
session_dir,
|
|
workspaces,
|
|
turn_events,
|
|
abort_flag.clone(),
|
|
);
|
|
|
|
// Background security review: only production source files
|
|
spawn_background_security_review(
|
|
&source_paths,
|
|
session_dir,
|
|
workspaces,
|
|
turn_events,
|
|
abort_flag,
|
|
);
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
#[test]
|
|
fn reviewable_path_skips_lockfiles_and_known_extensions() {
|
|
assert!(!is_reviewable_path("Cargo.lock"));
|
|
assert!(!is_reviewable_path("package.json"));
|
|
assert!(!is_reviewable_path("logo.svg"));
|
|
}
|
|
|
|
#[test]
|
|
fn reviewable_path_skips_vendored_and_generated_dirs() {
|
|
assert!(!is_reviewable_path("target/debug/build.rs"));
|
|
assert!(!is_reviewable_path("node_modules/foo/index.js"));
|
|
}
|
|
|
|
#[test]
|
|
fn reviewable_path_accepts_ordinary_source_files() {
|
|
assert!(is_reviewable_path("src/main.rs"));
|
|
}
|
|
|
|
#[test]
|
|
fn production_code_excludes_dedicated_test_directories() {
|
|
assert!(!is_production_code("src/tests/foo.rs"));
|
|
assert!(!is_production_code("__tests__/baz.test.ts"));
|
|
}
|
|
|
|
#[test]
|
|
fn production_code_excludes_test_filename_conventions() {
|
|
assert!(!is_production_code("src/foo_test.rs"));
|
|
assert!(!is_production_code("src/test_foo.py"));
|
|
assert!(!is_production_code("src/foo.spec.ts"));
|
|
}
|
|
|
|
#[test]
|
|
fn production_code_does_not_false_positive_on_substring_test() {
|
|
// Regression: a plain `.contains("test")` would wrongly exclude
|
|
// these legitimate production files.
|
|
assert!(is_production_code("src/attestation.rs"));
|
|
assert!(is_production_code("src/latest/foo.rs"));
|
|
}
|
|
|
|
#[test]
|
|
fn production_code_requires_known_source_extension() {
|
|
assert!(!is_production_code("README.md"));
|
|
assert!(is_production_code("src/main.rs"));
|
|
}
|
|
|
|
#[test]
|
|
fn running_guard_resets_flag_on_drop_even_after_panic() {
|
|
static TEST_FLAG: AtomicBool = AtomicBool::new(false);
|
|
TEST_FLAG.store(true, Ordering::SeqCst);
|
|
let result = std::panic::catch_unwind(|| {
|
|
let _guard = RunningGuard(&TEST_FLAG);
|
|
panic!("simulated failure inside guarded region");
|
|
});
|
|
assert!(result.is_err());
|
|
assert!(
|
|
!TEST_FLAG.load(Ordering::SeqCst),
|
|
"guard must reset the flag even when the guarded closure panics"
|
|
);
|
|
}
|
|
}
|