From 1039f67c12749c6b2c93e3ab7037feded8ab01c6 Mon Sep 17 00:00:00 2001 From: asepharyana Date: Tue, 14 Jul 2026 09:59:58 +0700 Subject: [PATCH] fix(subagent): perbaiki filter is_production_code berbasis substring dan tambah pembatalan/anti-tumpang-tindih pada background review Co-Authored-By: Claude Sonnet 5 --- src/app/runtime/actions/mod.rs | 2 + src/app/subagent/auto.rs | 147 ++++++++++++++++++++++++++++++--- 2 files changed, 137 insertions(+), 12 deletions(-) diff --git a/src/app/runtime/actions/mod.rs b/src/app/runtime/actions/mod.rs index a5f35b3..3f1aeb9 100644 --- a/src/app/runtime/actions/mod.rs +++ b/src/app/runtime/actions/mod.rs @@ -1509,12 +1509,14 @@ fn run_agent_turn( let bg_session_dir = tc.edit_log_session_dir.clone(); let bg_workspaces = tc.workspace_roots.clone(); let bg_events = events_q.clone(); + let bg_abort = tc.abort_flag.clone(); std::thread::spawn(move || { crate::app::subagent::auto::spawn_all_background( &bg_paths, &bg_session_dir, &bg_workspaces, &bg_events, + bg_abort, ); }); } diff --git a/src/app/subagent/auto.rs b/src/app/subagent/auto.rs index 66201ea..b051b55 100644 --- a/src/app/subagent/auto.rs +++ b/src/app/subagent/auto.rs @@ -18,6 +18,7 @@ use std::path::Path; use std::sync::{Arc, Mutex}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::collections::VecDeque; use crate::app::state::runtime::TurnEvent; use crate::app::subagent::context::build_subagent_context; @@ -37,9 +38,24 @@ const SKIP_REVIEW_FILES: &[&str] = &[ ".gitignore", ".env", ".env.example", ]; +/// 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); + /// ─── Helpers ─── /// /// Check whether a file path is worth auto-reviewing (not config/lock/data). +/// +/// Vendored/generated directories are matched by path *segment* rather than +/// a `/target/`-style substring check — the substring form misses paths +/// where the directory is the first component (e.g. `target/debug/build.rs`, +/// which has no leading slash), the same class of bug fixed in +/// `is_production_code` below. pub fn is_reviewable_path(path: &str) -> bool { let lower = path.to_lowercase(); if SKIP_REVIEW_FILES.iter().any(|f| lower.ends_with(f)) { @@ -49,9 +65,14 @@ pub fn is_reviewable_path(path: &str) -> bool { return false; } // Skip paths that are clearly generated or vendored - if lower.contains("/target/") || lower.contains("/node_modules/") - || lower.contains("/.git/") || lower.contains("/vendor/") - { + let in_vendored_dir = std::path::Path::new(&lower).components().any(|c| { + matches!( + c, + std::path::Component::Normal(seg) + if matches!(seg.to_str(), Some("target" | "node_modules" | ".git" | "vendor")) + ) + }); + if in_vendored_dir { return false; } true @@ -60,15 +81,40 @@ pub fn is_reviewable_path(path: &str) -> bool { /// Determine whether a file change looks like it modifies production logic /// (vs. tests, config, or documentation) — used to decide if a test-gen /// or security-review background subagent should fire. +/// +/// Matches test-ness by path *segment* (a directory literally named +/// "test"/"tests"/"__tests__") or by filename convention +/// (`foo_test.rs`, `foo.test.ts`, `test_foo.py`, `foo_spec.rb`), not by a +/// raw substring check — a plain `.contains("test")` would wrongly exclude +/// legitimate production files like `src/attestation.rs` or +/// `src/latest/foo.rs`. fn is_production_code(path: &str) -> bool { let lower = path.to_lowercase(); - // Skip test files — they don't need test-gen from another agent - if lower.contains("test") || lower.contains("spec") || lower.contains("_test.") { + let path_obj = std::path::Path::new(&lower); + + let in_test_dir = path_obj.components().any(|c| { + matches!( + c, + std::path::Component::Normal(seg) + if matches!(seg.to_str(), Some("test") | Some("tests") | Some("__tests__")) + ) + }); + + let file_stem = path_obj.file_stem().and_then(|s| s.to_str()).unwrap_or(""); + let is_test_filename = file_stem.starts_with("test_") + || file_stem.ends_with("_test") + || file_stem.ends_with(".test") + || file_stem == "spec" + || file_stem.ends_with("_spec") + || file_stem.ends_with(".spec"); + + if in_test_dir || is_test_filename { return false; } + // Only source files — use Path::extension() to avoid clippy // case_sensitive_file_extension_comparisons lint - std::path::Path::new(&lower) + path_obj .extension() .and_then(|ext| ext.to_str()) .is_some_and(|ext| { @@ -155,12 +201,17 @@ fn run_subagent_with_retry( session_dir: &Path, workspaces: &[std::path::PathBuf], label: &str, + abort_flag: Option<&Arc>, ) -> Result { 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 (tx, mut rx) = tokio::sync::mpsc::channel(32); let drain_label = label.to_string(); @@ -193,10 +244,15 @@ pub fn spawn_background_test_gen( session_dir: &Path, workspaces: &[std::path::PathBuf], turn_events: &Arc>>, + abort_flag: Arc, ) { if file_paths.is_empty() { return; } + if TEST_GEN_RUNNING.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst).is_err() { + tracing::debug!("[bg-test-gen] skipped — a test-gen run is already in flight"); + return; + } let paths = file_paths.to_vec(); let sd = session_dir.to_path_buf(); @@ -224,12 +280,13 @@ pub fn spawn_background_test_gen( .with_system_prompt(prompt) ; - let result = run_subagent_with_retry(&def, &sd, &ws, "bg-test-gen"); + let result = run_subagent_with_retry(&def, &sd, &ws, "bg-test-gen", Some(&abort_flag)); let message = match &result { Ok(output) => { let first = output.lines().next().unwrap_or(output); format!("Auto test-gen: {first}") } + Err(e) if e.contains("aborted") => format!("Auto test-gen cancelled: {e}"), Err(e) => format!("ESCALATED: Auto test-gen {e}"), }; @@ -239,6 +296,7 @@ pub fn spawn_background_test_gen( message, }); } + TEST_GEN_RUNNING.store(false, Ordering::SeqCst); }); } @@ -252,10 +310,15 @@ pub fn spawn_background_arch_review( session_dir: &Path, workspaces: &[std::path::PathBuf], turn_events: &Arc>>, + abort_flag: Arc, ) { if file_paths.is_empty() { return; } + if ARCH_REVIEW_RUNNING.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst).is_err() { + tracing::debug!("[bg-arch-review] skipped — an arch-review run is already in flight"); + return; + } let paths = file_paths.to_vec(); let sd = session_dir.to_path_buf(); @@ -277,12 +340,13 @@ pub fn spawn_background_arch_review( .with_system_prompt(prompt) ; - let result = run_subagent_with_retry(&def, &sd, &ws, "bg-arch-review"); + let result = run_subagent_with_retry(&def, &sd, &ws, "bg-arch-review", Some(&abort_flag)); let message = match &result { Ok(output) => { let first = output.lines().next().unwrap_or(output); format!("Architecture review: {first}") } + Err(e) if e.contains("aborted") => format!("Architecture review cancelled: {e}"), Err(e) => format!("ESCALATED: Architecture review {e}"), }; @@ -292,6 +356,7 @@ pub fn spawn_background_arch_review( message, }); } + ARCH_REVIEW_RUNNING.store(false, Ordering::SeqCst); }); } @@ -304,6 +369,7 @@ pub fn spawn_background_security_review( session_dir: &Path, workspaces: &[std::path::PathBuf], turn_events: &Arc>>, + abort_flag: Arc, ) { if file_paths.is_empty() { return; @@ -320,6 +386,10 @@ pub fn spawn_background_security_review( if prod_paths.is_empty() { return; } + if SECURITY_REVIEW_RUNNING.compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst).is_err() { + tracing::debug!("[bg-security-review] skipped — a security-review run is already in flight"); + return; + } let paths = prod_paths; let sd = session_dir.to_path_buf(); @@ -341,12 +411,13 @@ pub fn spawn_background_security_review( .with_system_prompt(prompt) ; - let result = run_subagent_with_retry(&def, &sd, &ws, "bg-security-review"); + let result = run_subagent_with_retry(&def, &sd, &ws, "bg-security-review", Some(&abort_flag)); let message = match &result { Ok(output) => { let first = output.lines().next().unwrap_or(output); format!("Security review: {first}") } + Err(e) if e.contains("aborted") => format!("Security review cancelled: {e}"), Err(e) => format!("ESCALATED: Security review {e}"), }; @@ -356,6 +427,7 @@ pub fn spawn_background_security_review( message, }); } + SECURITY_REVIEW_RUNNING.store(false, Ordering::SeqCst); }); } @@ -370,6 +442,7 @@ pub fn spawn_all_background( session_dir: &Path, workspaces: &[std::path::PathBuf], turn_events: &Arc>>, + abort_flag: Arc, ) { if file_paths.is_empty() { return; @@ -381,7 +454,7 @@ pub fn spawn_all_background( .filter(|p| is_production_code(p)) .cloned() .collect(); - spawn_background_test_gen(&source_paths, session_dir, workspaces, turn_events); + 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 = file_paths @@ -389,8 +462,58 @@ pub fn spawn_all_background( .filter(|p| is_reviewable_path(p)) .cloned() .collect(); - spawn_background_arch_review(&reviewable, session_dir, workspaces, turn_events); + 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); + 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")); + } }