From 1dcabe509ac2aab4d8e9170a1a7a7208157854ec Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 8 Apr 2026 11:02:48 +1000 Subject: [PATCH 1/2] fix(staged): cancel auto reviews before manual sessions --- apps/staged/src-tauri/src/session_commands.rs | 138 ++++++++++++++++++ .../BranchCardSessionManager.svelte.ts | 4 +- 2 files changed, 140 insertions(+), 2 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 004b61202..9ada2cef9 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -626,6 +626,13 @@ pub async fn start_branch_session( ) -> Result { let store = get_store(&store)?; + if matches!( + session_type, + BranchSessionType::Commit | BranchSessionType::Review + ) { + cancel_in_flight_auto_review_for_branch(&store, ®istry, &branch_id)?; + } + // Resolve branch → project let branch = store .get_branch(&branch_id) @@ -835,6 +842,7 @@ pub async fn start_branch_session( #[tauri::command(rename_all = "camelCase")] pub fn queue_branch_session( store: tauri::State<'_, Mutex>>>, + registry: tauri::State<'_, Arc>, branch_id: String, prompt: String, session_type: BranchSessionType, @@ -843,6 +851,13 @@ pub fn queue_branch_session( ) -> Result { let store = get_store(&store)?; + if matches!( + session_type, + BranchSessionType::Commit | BranchSessionType::Review + ) { + cancel_in_flight_auto_review_for_branch(&store, ®istry, &branch_id)?; + } + // Validate that the branch exists. let _branch = store .get_branch(&branch_id) @@ -1385,6 +1400,48 @@ fn latest_git_commit_ms(store: &Arc, branch_id: &str) -> i64 { commits.iter().map(|c| c.timestamp).max().unwrap_or(0) * 1000 } +fn cancel_in_flight_auto_review_for_branch( + store: &Arc, + registry: &session_runner::SessionRegistry, + branch_id: &str, +) -> Result { + let git_ts = latest_git_commit_ms(store, branch_id); + let Some(review) = store + .find_fresh_auto_review(branch_id, git_ts) + .map_err(|e| e.to_string())? + else { + return Ok(false); + }; + + let Some(session_id) = review.session_id.as_deref() else { + return Ok(false); + }; + + let Some(session) = store.get_session(session_id).map_err(|e| e.to_string())? else { + return Ok(false); + }; + + if !matches!( + session.status, + store::SessionStatus::Running | store::SessionStatus::Queued + ) { + return Ok(false); + } + + registry.cancel(session_id); + store + .update_session_status( + session_id, + store::SessionStatus::Cancelled, + None, + Some(&store::CompletionReason::Interrupted), + ) + .map_err(|e| e.to_string())?; + store.delete_review(&review.id).map_err(|e| e.to_string())?; + + Ok(true) +} + /// Find an auto review created after all commits on a branch. #[tauri::command(rename_all = "camelCase")] pub async fn find_fresh_auto_review( @@ -2377,6 +2434,40 @@ Rules: #[cfg(test)] mod tests { use super::*; + use std::path::Path; + use std::sync::Arc; + + fn setup_branch_store() -> (Arc, store::Branch) { + let store = Arc::new(Store::in_memory().unwrap()); + let project = store::Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = store::Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + (store, branch) + } + + fn create_auto_review( + store: &Arc, + branch_id: &str, + status: store::SessionStatus, + ) -> (store::Session, store::Review) { + let session = match status { + store::SessionStatus::Queued => store::Session::new_queued("auto review"), + _ => store::Session::new_running("auto review", Path::new("/tmp")), + }; + store.create_session(&session).unwrap(); + if status != store::SessionStatus::Running && status != store::SessionStatus::Queued { + store + .update_session_status(&session.id, status, None, None) + .unwrap(); + } + + let review = store::Review::new(branch_id, "abc123", store::ReviewScope::Branch) + .with_session(&session.id) + .with_auto(); + store.create_review(&review).unwrap(); + (session, review) + } #[test] fn infer_branch_resume_session_type_detects_pr_prompts() { @@ -2434,4 +2525,51 @@ mod tests { )); assert!(prompt.contains("do not output any preamble, commentary, or thinking before it")); } + + #[test] + fn cancel_in_flight_auto_review_cancels_running_review() { + let (store, branch) = setup_branch_store(); + let (session, review) = + create_auto_review(&store, &branch.id, store::SessionStatus::Running); + let registry = session_runner::SessionRegistry::new(); + + let cancelled = + cancel_in_flight_auto_review_for_branch(&store, ®istry, &branch.id).unwrap(); + + assert!(cancelled); + assert!(store.get_session(&session.id).unwrap().is_none()); + assert!(store.get_review(&review.id).unwrap().is_none()); + } + + #[test] + fn cancel_in_flight_auto_review_cancels_queued_review() { + let (store, branch) = setup_branch_store(); + let (session, review) = + create_auto_review(&store, &branch.id, store::SessionStatus::Queued); + let registry = session_runner::SessionRegistry::new(); + + let cancelled = + cancel_in_flight_auto_review_for_branch(&store, ®istry, &branch.id).unwrap(); + + assert!(cancelled); + assert!(store.get_session(&session.id).unwrap().is_none()); + assert!(store.get_review(&review.id).unwrap().is_none()); + } + + #[test] + fn cancel_in_flight_auto_review_leaves_completed_review_available_for_adoption() { + let (store, branch) = setup_branch_store(); + let (session, review) = + create_auto_review(&store, &branch.id, store::SessionStatus::Completed); + let registry = session_runner::SessionRegistry::new(); + + let cancelled = + cancel_in_flight_auto_review_for_branch(&store, ®istry, &branch.id).unwrap(); + + assert!(!cancelled); + let session = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(session.status, store::SessionStatus::Completed); + let review = store.get_review(&review.id).unwrap().unwrap(); + assert!(review.is_auto); + } } diff --git a/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts b/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts index afe4eda2d..825abaea7 100644 --- a/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts +++ b/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts @@ -212,7 +212,7 @@ export default class BranchCardSessionManager { const branch = this.getBranch(); const isRemote = this.getIsRemote(); - if (this.autoReviewSessionId && mode !== 'review') { + if (this.autoReviewSessionId && mode !== 'note') { this.cancelAutoReview(); } @@ -270,7 +270,7 @@ export default class BranchCardSessionManager { const branch = this.getBranch(); const isRemote = this.getIsRemote(); - if (this.autoReviewSessionId && mode !== 'review') { + if (this.autoReviewSessionId && mode !== 'note') { this.cancelAutoReview(); } From 01ee28e09ecaff6d98d6565e1719cb0c9ebfb3f4 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 8 Apr 2026 11:13:39 +1000 Subject: [PATCH 2/2] fix(staged): resolve auto-review cancellation review comments --- apps/staged/src-tauri/src/session_commands.rs | 23 ++++++++++-- apps/staged/src-tauri/src/store/sessions.rs | 26 ++++++++++++++ apps/staged/src-tauri/src/store/tests.rs | 35 +++++++++++++++++++ .../BranchCardSessionManager.svelte.ts | 4 --- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 9ada2cef9..437442ecc 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -1429,14 +1429,25 @@ fn cancel_in_flight_auto_review_for_branch( } registry.cancel(session_id); - store - .update_session_status( + let cancelled = store + .transition_from_active( session_id, store::SessionStatus::Cancelled, None, Some(&store::CompletionReason::Interrupted), ) .map_err(|e| e.to_string())?; + if !cancelled { + let current = store.get_session(session_id).map_err(|e| e.to_string())?; + return match current.map(|session| session.status) { + None => Ok(true), + Some(store::SessionStatus::Cancelled) => { + store.delete_review(&review.id).map_err(|e| e.to_string())?; + Ok(true) + } + _ => Ok(false), + }; + } store.delete_review(&review.id).map_err(|e| e.to_string())?; Ok(true) @@ -2452,8 +2463,14 @@ mod tests { status: store::SessionStatus, ) -> (store::Session, store::Review) { let session = match status { + store::SessionStatus::Running => { + store::Session::new_running("auto review", Path::new("/tmp")) + } store::SessionStatus::Queued => store::Session::new_queued("auto review"), - _ => store::Session::new_running("auto review", Path::new("/tmp")), + store::SessionStatus::Completed => { + store::Session::new_running("auto review", Path::new("/tmp")) + } + other => panic!("unsupported auto review test status: {}", other.as_str()), }; store.create_session(&session).unwrap(); if status != store::SessionStatus::Running && status != store::SessionStatus::Queued { diff --git a/apps/staged/src-tauri/src/store/sessions.rs b/apps/staged/src-tauri/src/store/sessions.rs index 7eb045a6a..228df03ba 100644 --- a/apps/staged/src-tauri/src/store/sessions.rs +++ b/apps/staged/src-tauri/src/store/sessions.rs @@ -90,6 +90,32 @@ impl Store { Ok(rows > 0) } + /// Transition session status only if it is currently `queued` or `running`. + /// + /// Returns `true` if the row was updated, `false` if the session already + /// moved to another state or didn't exist. This is the safe path for + /// cancelling work that may still be in the queue. + pub fn transition_from_active( + &self, + id: &str, + new_status: SessionStatus, + error_message: Option<&str>, + completion_reason: Option<&CompletionReason>, + ) -> Result { + let conn = self.conn.lock().unwrap(); + let error_msg = if new_status == SessionStatus::Error { + error_message + } else { + None + }; + let rows = conn.execute( + "UPDATE sessions SET status = ?1, error_message = ?2, completion_reason = ?3, updated_at = ?4 + WHERE id = ?5 AND status IN ('queued', 'running')", + params![new_status.as_str(), error_msg, completion_reason.map(|r| r.as_str()), now_timestamp(), id], + )?; + Ok(rows > 0) + } + /// Atomically transition a session to `Running`, but only if it is NOT /// already running. Returns `true` if the row was updated, `false` if /// the session was already running (or didn't exist). diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index 9263b5530..22e05d2b2 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -358,6 +358,41 @@ fn test_transition_from_running_succeeds_when_running() { assert_eq!(final_state.status, SessionStatus::Completed); } +#[test] +fn test_transition_from_active_succeeds_when_queued() { + let store = Store::in_memory().unwrap(); + + let session = Session::new_queued("queued"); + store.create_session(&session).unwrap(); + + let transitioned = store + .transition_from_active(&session.id, SessionStatus::Cancelled, None, None) + .unwrap(); + assert!(transitioned); + + let final_state = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(final_state.status, SessionStatus::Cancelled); +} + +#[test] +fn test_transition_from_active_does_not_overwrite_completed_session() { + let store = Store::in_memory().unwrap(); + + let session = Session::new_running("completed first", Path::new("/tmp")); + store.create_session(&session).unwrap(); + store + .update_session_status(&session.id, SessionStatus::Completed, None, None) + .unwrap(); + + let transitioned = store + .transition_from_active(&session.id, SessionStatus::Cancelled, None, None) + .unwrap(); + assert!(!transitioned); + + let final_state = store.get_session(&session.id).unwrap().unwrap(); + assert_eq!(final_state.status, SessionStatus::Completed); +} + #[test] fn test_completion_reason_round_trips() { let store = Store::in_memory().unwrap(); diff --git a/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts b/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts index 825abaea7..9b1d90f9a 100644 --- a/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts +++ b/apps/staged/src/lib/features/branches/BranchCardSessionManager.svelte.ts @@ -382,10 +382,6 @@ export default class BranchCardSessionManager { return; } - if (data.mode === 'review' && this.autoReviewSessionId) { - this.cancelAutoReview(); - } - const prompt = data.prompt || (data.mode === 'review' ? 'Review the code changes on this branch.' : ''); void this.startOrQueueSession(data.mode, prompt, data.imageIds);