fix(subagent): perbaiki filter is_production_code berbasis substring dan tambah pembatalan/anti-tumpang-tindih pada background review

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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,
                 );
             });
         }

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<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 (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<Mutex<VecDeque<TurnEvent>>>,
+    abort_flag: Arc<AtomicBool>,
 ) {
     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<Mutex<VecDeque<TurnEvent>>>,
+    abort_flag: Arc<AtomicBool>,
 ) {
     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<Mutex<VecDeque<TurnEvent>>>,
+    abort_flag: Arc<AtomicBool>,
 ) {
     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<Mutex<VecDeque<TurnEvent>>>,
+    abort_flag: Arc<AtomicBool>,
 ) {
     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<String> = 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"));
+    }
 }