diff --git a/src/app/subagent/auto.rs b/src/app/subagent/auto.rs index b051b55..1b20717 100644 --- a/src/app/subagent/auto.rs +++ b/src/app/subagent/auto.rs @@ -47,6 +47,19 @@ 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 ─── /// /// Check whether a file path is worth auto-reviewing (not config/lock/data). @@ -194,8 +207,14 @@ pub fn spawn_quick_review( /// 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. +/// 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, @@ -239,6 +258,11 @@ fn run_subagent_with_retry( /// Uses the test-generator prompt and has read-write access so it can /// create test files. Runs in a separate OS thread and reports completion /// via `TurnEvent::SystemNote { kind: "bg-test-gen" }`. +/// +/// Skipped (no-op) if a test-gen run is already in flight (guarded by +/// `TEST_GEN_RUNNING`) — prevents a chatty multi-turn edit session from +/// stacking overlapping runs. `abort_flag` is forwarded to +/// `run_subagent_with_retry` so the run can be cancelled if the turn aborts. pub fn spawn_background_test_gen( file_paths: &[String], session_dir: &Path, @@ -260,6 +284,7 @@ pub fn spawn_background_test_gen( let events = turn_events.clone(); std::thread::spawn(move || { + let _running_guard = RunningGuard(&TEST_GEN_RUNNING); tracing::info!( "[bg-test-gen] spawning for {} file(s): {:?}", paths.len(), @@ -296,7 +321,6 @@ pub fn spawn_background_test_gen( message, }); } - TEST_GEN_RUNNING.store(false, Ordering::SeqCst); }); } @@ -305,6 +329,10 @@ pub fn spawn_background_test_gen( /// Inspects the modified files for architectural consistency (layering, /// coupling, module boundaries). Reports via /// `TurnEvent::SystemNote { kind: "bg-arch-review" }`. +/// +/// Skipped (no-op) if an arch-review run is already in flight (guarded by +/// `ARCH_REVIEW_RUNNING`). `abort_flag` is forwarded to +/// `run_subagent_with_retry` so the run can be cancelled if the turn aborts. pub fn spawn_background_arch_review( file_paths: &[String], session_dir: &Path, @@ -326,6 +354,7 @@ pub fn spawn_background_arch_review( let events = turn_events.clone(); std::thread::spawn(move || { + let _running_guard = RunningGuard(&ARCH_REVIEW_RUNNING); let file_list = paths.join("\n"); let prompt = format!( "{}\n\nModified files for architecture review:\n{}", @@ -356,7 +385,6 @@ pub fn spawn_background_arch_review( message, }); } - ARCH_REVIEW_RUNNING.store(false, Ordering::SeqCst); }); } @@ -364,6 +392,10 @@ pub fn spawn_background_arch_review( /// /// Checks modified files for security vulnerabilities. Reports via /// `TurnEvent::SystemNote { kind: "bg-security-review" }`. +/// +/// Skipped (no-op) if a security-review run is already in flight (guarded by +/// `SECURITY_REVIEW_RUNNING`). `abort_flag` is forwarded to +/// `run_subagent_with_retry` so the run can be cancelled if the turn aborts. pub fn spawn_background_security_review( file_paths: &[String], session_dir: &Path, @@ -397,6 +429,7 @@ pub fn spawn_background_security_review( let events = turn_events.clone(); std::thread::spawn(move || { + let _running_guard = RunningGuard(&SECURITY_REVIEW_RUNNING); let file_list = paths.join("\n"); let prompt = format!( "{}\n\nModified files for security review:\n{}", @@ -427,7 +460,6 @@ pub fn spawn_background_security_review( message, }); } - SECURITY_REVIEW_RUNNING.store(false, Ordering::SeqCst); }); } @@ -437,6 +469,9 @@ pub fn spawn_background_security_review( /// 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, @@ -516,4 +551,19 @@ mod tests { 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" + ); + } }