From 870639eb3e6052b8554fda843885b13ab9a08aed Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 15:05:24 +1000 Subject: [PATCH 1/9] feat(staged): queue rebase/squash behind running branch sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebase and Squash were disabled whenever the branch had a queued or running session ("Session in progress"), even though the backend already knew how to enqueue them: `start_or_queue_commit_pipeline_for_branch` creates a queued session plus pending-commit artifact that `drain_queued_sessions_for_branch` picks up in FIFO order. Only the frontend stood in the way. Phase 1 of the queue-git-actions plan removes that block and makes the queued state legible. Backend: - `rebase_branch`/`squash_commits` now return `BranchPipelineResponse` (`sessionId` + `sessionStatus`) instead of a bare session id, so the caller can tell a started pipeline from a queued one. Mirrored in the `web_server.rs` dispatch arms. - The queue-vs-run decision, the dedupe scan, and the insert all run under `branch_session_launch_lock_for`, replacing a racy check-then-insert. Clicking Rebase twice now reuses the queued session rather than stacking a second one. Dedupe keys on kind *and* persisted rebase target so "onto base" and "onto origin" stay distinct. - `web_server.rs` also forwards `target`, so web mode no longer silently downgrades "Rebase onto Origin" to a base rebase. Frontend: - Dropped `branchSessionBusy` from the `startBranchCommandPipeline` guard and from `branchCommandDisabledReason`; identity warnings (detached HEAD, wrong branch) still disable both actions. - A queued response now renders a `queued-commit` stub labelled with the pipeline name and "Queued — waiting for current session…", matching the persisted row instead of flashing "Rebasing…" for work that hasn't started. - `BranchTimeline`'s `rebaseBranchDisabledReason` gated push, force-push, and reset-to-origin rather than rebase. Renamed it to `immediateGitActionDisabledReason` and fed it a reason that keeps the busy check, so those three stay blocked while sessions are in flight — their queueing is Phases 2 and 3, and their handlers still guard on `branchSessionBusy`. Without the split, removing the busy arm would have made them look clickable while silently doing nothing. - `BranchCardActionsBar`'s `newCommitDisabled` only ever gated the Rebase/Squash menu items, so it is now `rebaseSquashDisabled`. Tests: seven `prs.rs` cases cover run-when-idle, queue-when-busy, the queued label and pending-commit link, repeat-click dedupe, per-kind and per-target separation, and a queued pipeline alone keeping the branch busy. A `commands.test.ts` case pins the new return shapes. `just fmt-check`, `just lint`, `just typecheck`, `just test` (559), and `pnpm test` (482) all pass. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/prs.rs | 352 ++++++++++++++++-- apps/staged/src-tauri/src/session_commands.rs | 2 +- apps/staged/src-tauri/src/web_server.rs | 13 +- apps/staged/src/lib/commands.test.ts | 27 ++ apps/staged/src/lib/commands.ts | 14 +- .../lib/features/branches/BranchCard.svelte | 60 +-- .../branches/BranchCardActionsBar.svelte | 10 +- .../features/timeline/BranchTimeline.svelte | 19 +- apps/staged/src/lib/types.ts | 11 + 9 files changed, 426 insertions(+), 82 deletions(-) diff --git a/apps/staged/src-tauri/src/prs.rs b/apps/staged/src-tauri/src/prs.rs index fd0563c3f..bbf9457a1 100644 --- a/apps/staged/src-tauri/src/prs.rs +++ b/apps/staged/src-tauri/src/prs.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; use crate::git; +use crate::session_commands::BranchSessionLaunchStatus; use crate::session_runner; use crate::store::{self, FailureStrategy, PipelineExecution, PipelineKind, PipelineStep, Store}; @@ -49,6 +50,34 @@ pub(crate) struct PrStatusEvent { // Pipeline session helper // ============================================================================= +/// Result of a start-or-queue branch pipeline command. +/// +/// Rebase and squash can be requested while the branch already has work in +/// flight, so the caller needs to know whether the returned session started +/// running or is waiting its turn on the branch queue. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[serde(rename_all = "camelCase")] +pub struct BranchPipelineResponse { + pub session_id: String, + pub session_status: BranchSessionLaunchStatus, +} + +impl BranchPipelineResponse { + fn running(session_id: String) -> Self { + Self { + session_id, + session_status: BranchSessionLaunchStatus::Running, + } + } + + fn queued(session_id: String) -> Self { + Self { + session_id, + session_status: BranchSessionLaunchStatus::Queued, + } + } +} + /// Resolved context for a branch, ready to start a pipeline session. struct BranchPipelineContext { branch: store::Branch, @@ -213,6 +242,46 @@ fn commit_pipeline_prompt(kind: &PipelineKind) -> &'static str { } } +/// The rebase target to persist on the pipeline, or `None` when the pipeline +/// targets the branch's configured base. +/// +/// Only "rebase onto origin" needs a persisted target; a base rebase re-derives +/// it from the branch on dequeue. Keeping this in one place means the queued and +/// running paths agree, which the same-kind dedupe check relies on. +fn persisted_rebase_target(target: Option<&str>, rebase_ref: &str) -> Option { + matches!(target, Some("origin")).then(|| rebase_ref.to_string()) +} + +/// Find a queued pipeline session on this branch that already performs exactly +/// the work being requested, so a second click doesn't stack a duplicate. +/// +/// `rebase_target` must be the *persisted* target so that "rebase onto base" and +/// "rebase onto origin" stay distinct — they are different operations even +/// though they share a [`PipelineKind`]. +fn find_queued_commit_pipeline( + store: &Arc, + branch_id: &str, + kind: &PipelineKind, + rebase_target: Option<&str>, +) -> Result, String> { + let queued = store + .get_queued_sessions_for_branch(branch_id) + .map_err(|e| e.to_string())?; + + for session in queued { + let Some(pipeline) = session.pipeline.as_ref() else { + continue; + }; + if pipeline.kind.as_ref() == Some(kind) + && pipeline.rebase_target.as_deref() == rebase_target + { + return Ok(Some(session.id)); + } + } + + Ok(None) +} + const HTTPS_FALLBACK_CONFIG: &str = "url.https://github.com/.insteadOf=git@github.com:"; fn git_fetch_with_fallback(refspec: &str) -> String { @@ -392,6 +461,69 @@ async fn start_running_commit_pipeline_for_branch( Ok(session.id) } +/// Queue a rebase/squash pipeline when the branch has work in flight. +/// +/// Returns the queued session id — either a freshly created one, or an existing +/// queued pipeline that already covers this request — and `None` when the branch +/// is idle so the caller should start the pipeline immediately. +/// +/// The busy check, the dedupe scan, and the insert all run under the branch +/// launch lock, so two rapid clicks (or a click racing a session start) cannot +/// both observe an idle branch or both miss the same queued pipeline. This stays +/// synchronous on purpose: the lock must not be held across an await. +fn queue_commit_pipeline_if_branch_busy( + store: &Arc, + branch_id: &str, + kind: &PipelineKind, + provider: Option<&str>, + target: Option<&str>, +) -> Result, String> { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); + let _guard = launch_lock.lock().unwrap(); + + let branch_busy = store + .has_running_session_for_branch(branch_id) + .map_err(|e| e.to_string())? + || !store + .get_queued_sessions_for_branch(branch_id) + .map_err(|e| e.to_string())? + .is_empty(); + if !branch_busy { + return Ok(None); + } + + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + let base_branch = base_branch_name(&branch); + let rebase_ref = rebase_ref_for_target(&branch, target); + let rebase_target = persisted_rebase_target(target, &rebase_ref); + + if let Some(existing) = + find_queued_commit_pipeline(store, branch_id, kind, rebase_target.as_deref())? + { + return Ok(Some(existing)); + } + + let steps = build_commit_pipeline_steps(kind, base_branch, &rebase_ref); + let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind.clone()); + if let Some(target) = rebase_target { + pipeline = pipeline.with_rebase_target(target); + } + let mut session = store::Session::new_queued(commit_pipeline_prompt(kind)); + if let Some(p) = provider { + session = session.with_provider(p); + } + session.pipeline = Some(pipeline); + store.create_session(&session).map_err(|e| e.to_string())?; + + let commit = store::Commit::new_pending(branch_id).with_session(&session.id); + store.create_commit(&commit).map_err(|e| e.to_string())?; + + Ok(Some(session.id)) +} + #[allow(clippy::too_many_arguments)] pub(crate) async fn start_or_queue_commit_pipeline_for_branch( store: Arc, @@ -401,60 +533,36 @@ pub(crate) async fn start_or_queue_commit_pipeline_for_branch( kind: PipelineKind, provider: Option, target: Option, -) -> Result { - let prompt = commit_pipeline_prompt(&kind); - - let branch_has_running_session = store - .has_running_session_for_branch(&branch_id) - .map_err(|e| e.to_string())?; - let branch_has_queued_session = !store - .get_queued_sessions_for_branch(&branch_id) - .map_err(|e| e.to_string())? - .is_empty(); - - if branch_has_running_session || branch_has_queued_session { - let branch = store - .get_branch(&branch_id) - .map_err(|e| e.to_string())? - .ok_or_else(|| format!("Branch not found: {branch_id}"))?; - let base_branch = base_branch_name(&branch); - let rebase_ref = rebase_ref_for_target(&branch, target.as_deref()); - let steps = build_commit_pipeline_steps(&kind, base_branch, &rebase_ref); - let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind); - if matches!(target.as_deref(), Some("origin")) { - pipeline = pipeline.with_rebase_target(rebase_ref.clone()); - } - let mut session = store::Session::new_queued(prompt); - if let Some(ref p) = provider { - session = session.with_provider(p); - } - session.pipeline = Some(pipeline); - store.create_session(&session).map_err(|e| e.to_string())?; - - let commit = store::Commit::new_pending(&branch_id).with_session(&session.id); - store.create_commit(&commit).map_err(|e| e.to_string())?; - - return Ok(session.id); +) -> Result { + if let Some(session_id) = queue_commit_pipeline_if_branch_busy( + &store, + &branch_id, + &kind, + provider.as_deref(), + target.as_deref(), + )? { + return Ok(BranchPipelineResponse::queued(session_id)); } let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; let base_branch = base_branch_name(&ctx.branch).to_string(); let rebase_ref = rebase_ref_for_target(&ctx.branch, target.as_deref()); let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref); - let persisted_rebase_target = - matches!(target.as_deref(), Some("origin")).then(|| rebase_ref.clone()); + let rebase_target = persisted_rebase_target(target.as_deref(), &rebase_ref); - start_running_commit_pipeline_for_branch( + let session_id = start_running_commit_pipeline_for_branch( ctx, kind, steps, - persisted_rebase_target, + rebase_target, provider, store, &app_handle, ®istry, ) - .await + .await?; + + Ok(BranchPipelineResponse::running(session_id)) } pub(crate) async fn start_queued_commit_pipeline_for_branch( @@ -1281,6 +1389,9 @@ pub async fn push_branch( /// (the default behaviour used by the base-moved row and the `…` menu). /// When `target` is `"origin"`, rebases onto `origin/{branch_name}` so that /// the local branch incorporates remote-only commits (used by the diverged row). +/// +/// Queues behind in-flight branch work instead of failing, so the response +/// reports whether the rebase started or is waiting on the branch queue. #[tauri::command(rename_all = "camelCase")] pub async fn rebase_branch( store: tauri::State<'_, Mutex>>>, @@ -1289,7 +1400,7 @@ pub async fn rebase_branch( branch_id: String, provider: Option, target: Option, -) -> Result { +) -> Result { let store = get_store(&store)?; start_or_queue_commit_pipeline_for_branch( store, @@ -1310,6 +1421,9 @@ pub async fn rebase_branch( /// branch's own commits into staged changes. /// Hands off to AI to write a single conventional-commit message using the /// original commit history as context. +/// +/// Queues behind in-flight branch work instead of failing, so the response +/// reports whether the squash started or is waiting on the branch queue. #[tauri::command(rename_all = "camelCase")] pub async fn squash_commits( store: tauri::State<'_, Mutex>>>, @@ -1317,7 +1431,7 @@ pub async fn squash_commits( app_handle: tauri::AppHandle, branch_id: String, provider: Option, -) -> Result { +) -> Result { let store = get_store(&store)?; start_or_queue_commit_pipeline_for_branch( store, @@ -1356,6 +1470,162 @@ mod tests { } } + 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) + } + + /// Make the branch busy by linking a running note session to it, which is + /// what `has_running_session_for_branch` looks for. + fn start_running_note_session(store: &Arc, branch_id: &str) { + let session = store::Session::new_running("write a note", Path::new("/tmp/staged-test")); + store.create_session(&session).unwrap(); + let note = store::Note::new(branch_id, "note", "").with_session(&session.id); + store.create_note(¬e).unwrap(); + } + + fn queue_pipeline( + store: &Arc, + branch_id: &str, + kind: PipelineKind, + target: Option<&str>, + ) -> Option { + queue_commit_pipeline_if_branch_busy(store, branch_id, &kind, None, target).unwrap() + } + + #[test] + fn idle_branch_runs_pipeline_immediately_instead_of_queueing() { + let (store, branch) = setup_branch_store(); + + assert_eq!( + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None), + None + ); + assert!(store + .list_commits_for_branch(&branch.id) + .unwrap() + .is_empty()); + } + + #[test] + fn busy_branch_queues_pipeline_with_label_and_pending_commit() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let session_id = queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None) + .expect("rebase should queue behind the running note session"); + + let session = store.get_session(&session_id).unwrap().unwrap(); + assert_eq!(session.status, store::SessionStatus::Queued); + // The timeline renders queued pipeline rows from this prompt, so it has + // to read as the git action rather than a bare "Pending commit". + assert_eq!(session.prompt, "Rebase branch"); + let pipeline = session.pipeline.unwrap(); + assert_eq!(pipeline.kind, Some(PipelineKind::Rebase)); + assert_eq!(pipeline.rebase_target, None); + + let pending: Vec<_> = store + .list_commits_for_branch(&branch.id) + .unwrap() + .into_iter() + .filter(|c| c.sha.is_none()) + .collect(); + assert_eq!(pending.len(), 1); + assert_eq!(pending[0].session_id.as_deref(), Some(session_id.as_str())); + } + + #[test] + fn queued_squash_uses_squash_label() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let session_id = queue_pipeline(&store, &branch.id, PipelineKind::Squash, None).unwrap(); + + let session = store.get_session(&session_id).unwrap().unwrap(); + assert_eq!(session.prompt, "Squash commits"); + assert_eq!(session.pipeline.unwrap().kind, Some(PipelineKind::Squash)); + } + + #[test] + fn repeated_click_reuses_the_already_queued_pipeline() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let first = queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + let second = queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + + assert_eq!(first, second); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 1 + ); + } + + #[test] + fn different_pipeline_kinds_queue_separately() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let rebase = queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + let squash = queue_pipeline(&store, &branch.id, PipelineKind::Squash, None).unwrap(); + + assert_ne!(rebase, squash); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 2 + ); + } + + #[test] + fn rebase_onto_origin_is_not_deduped_against_rebase_onto_base() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let onto_base = queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + let onto_origin = + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, Some("origin")).unwrap(); + + assert_ne!(onto_base, onto_origin); + let origin_session = store.get_session(&onto_origin).unwrap().unwrap(); + assert_eq!( + origin_session.pipeline.unwrap().rebase_target.as_deref(), + Some("feature") + ); + + // Clicking the diverged row's rebase again still dedupes. + assert_eq!( + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, Some("origin")), + Some(onto_origin) + ); + } + + #[test] + fn queued_pipeline_alone_keeps_the_branch_busy() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + + // The note session finishes, but the still-queued rebase must keep a + // newly requested squash on the queue rather than running it now. + for session in store.get_running_sessions().unwrap() { + store + .update_session_status(&session.id, store::SessionStatus::Completed, None, None) + .unwrap(); + } + + assert!(queue_pipeline(&store, &branch.id, PipelineKind::Squash, None).is_some()); + } + #[tokio::test] async fn collect_branch_refresh_results_tolerates_partial_task_failure() { let tasks = vec![ diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 7a38670f9..fd461d83b 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -1912,7 +1912,7 @@ fn branch_session_launch_locks() -> &'static Mutex LOCKS.get_or_init(|| Mutex::new(HashMap::new())) } -fn branch_session_launch_lock_for(branch_id: &str) -> Arc> { +pub(crate) fn branch_session_launch_lock_for(branch_id: &str) -> Arc> { let mut locks = branch_session_launch_locks().lock().unwrap(); Arc::clone( locks diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 283136046..589555ba1 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -3391,23 +3391,26 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = opt_arg(&args, "provider")?; - let session_id = crate::prs::start_or_queue_commit_pipeline_for_branch( + // Forwarded so web mode honours "Rebase onto Origin" instead of + // silently downgrading it to a base rebase. + let target: Option = opt_arg(&args, "target")?; + let response = crate::prs::start_or_queue_commit_pipeline_for_branch( store, Arc::clone(session_registry), app_handle.clone(), branch_id, store::PipelineKind::Rebase, provider, - None, + target, ) .await?; - Ok(serde_json::to_value(session_id).unwrap()) + Ok(serde_json::to_value(response).unwrap()) } "squash_commits" => { let store = get_store(store_mutex)?; let branch_id: String = arg(&args, "branchId")?; let provider: Option = opt_arg(&args, "provider")?; - let session_id = crate::prs::start_or_queue_commit_pipeline_for_branch( + let response = crate::prs::start_or_queue_commit_pipeline_for_branch( store, Arc::clone(session_registry), app_handle.clone(), @@ -3417,7 +3420,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { let store = get_store(store_mutex)?; diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index 9db0ed4dd..c2eff5bab 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -259,6 +259,33 @@ describe('browser-native command wrappers', () => { }); }); + it('surfaces the queued-vs-running status returned by rebase and squash', async () => { + const invokeCommand = vi + .fn() + .mockResolvedValueOnce({ sessionId: 'session-1', sessionStatus: 'queued' }) + .mockResolvedValueOnce({ sessionId: 'session-2', sessionStatus: 'running' }); + vi.doMock('./transport', () => ({ + invokeCommand, + isTauri: true, + })); + + const { rebaseBranch, squashCommits } = await import('./commands'); + + await expect(rebaseBranch('branch-1', 'codex', 'origin')).resolves.toEqual({ + sessionId: 'session-1', + sessionStatus: 'queued', + }); + await expect(squashCommits('branch-1', 'codex')).resolves.toEqual({ + sessionId: 'session-2', + sessionStatus: 'running', + }); + + expect(invokeCommand.mock.calls).toEqual([ + ['rebase_branch', { branchId: 'branch-1', provider: 'codex', target: 'origin' }], + ['squash_commits', { branchId: 'branch-1', provider: 'codex' }], + ]); + }); + it('forwards ACP config selection when resuming a session', async () => { const invokeCommand = vi.fn().mockResolvedValue(undefined); vi.doMock('./transport', () => ({ diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index 2663936ae..b3bf0a015 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -21,6 +21,7 @@ import type { Branch, BranchTimeline, BranchRef, + BranchPipelineResponse, BranchSessionLaunchContext, BranchSessionType, BranchSessionResponse, @@ -1281,12 +1282,13 @@ export interface GitHubCommentResult { /** Rebase a branch via a pipeline. * When target is 'base' (default), rebases onto origin/{base_branch}. * When target is 'origin', rebases onto origin/{branch_name}. - * Returns the session ID so the frontend can track progress. */ + * Queues behind in-flight branch sessions, so the response reports whether the + * returned session is running or waiting on the branch queue. */ export function rebaseBranch( branchId: string, provider?: string, target?: 'base' | 'origin' -): Promise { +): Promise { return invokeCommand('rebase_branch', { branchId, provider: provider ?? null, @@ -1296,8 +1298,12 @@ export function rebaseBranch( /** Squash all commits on a branch into a single commit via a pipeline. * Uses git reset --soft then hands off to AI to write the commit message. - * Returns the session ID so the frontend can track progress. */ -export function squashCommits(branchId: string, provider?: string): Promise { + * Queues behind in-flight branch sessions, so the response reports whether the + * returned session is running or waiting on the branch queue. */ +export function squashCommits( + branchId: string, + provider?: string +): Promise { return invokeCommand('squash_commits', { branchId, provider: provider ?? null, diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index eba9597af..0b22df613 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -74,6 +74,7 @@ addPendingSession, getPendingSessionItems, prunePendingSessionItems, + queuedSessionMeta, } from './branchSessionLaunch.svelte'; import RemoteWorkspaceStatusBadge from './RemoteWorkspaceStatusBadge.svelte'; import RemoteWorkspaceStatusView from './RemoteWorkspaceStatusView.svelte'; @@ -329,26 +330,38 @@ let commandPipelinePending = $state(false); let branchSessionBusy = $derived(timeline ? hasActiveSessions(timeline) : false); + /** Timeline label the backend gives a queued rebase/squash pipeline session. */ + const branchCommandLabels = { rebase: 'Rebase branch', squash: 'Squash commits' } as const; + async function startBranchCommandPipeline( kind: 'rebase' | 'squash', rebaseTarget?: 'base' | 'origin' ) { - if (commandPipelinePending || branchSessionBusy) return; + // Deliberately not gated on `branchSessionBusy`: the backend decides + // between running now and queueing behind in-flight branch work, and it + // dedupes repeat clicks of the same action. + if (commandPipelinePending) return; commandPipelinePending = true; const agents = isRemote ? REMOTE_AGENTS : agentState.providers; const provider = getPreferredAgent(agents) ?? undefined; const pendingKey = `pipeline-${kind}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; try { - let sessionId: string; - if (kind === 'rebase') { - sessionId = await commands.rebaseBranch(branch.id, provider, rebaseTarget); - } else { - sessionId = await commands.squashCommits(branch.id, provider); - } + const result = + kind === 'rebase' + ? await commands.rebaseBranch(branch.id, provider, rebaseTarget) + : await commands.squashCommits(branch.id, provider); // Add a pending session item so the session stub appears instantly - // instead of waiting for the full timeline refresh. - const title = kind === 'rebase' ? 'Rebasing…' : 'Squashing…'; - addPendingSession(branch.id, { key: pendingKey, type: 'pending-commit', title, sessionId }); + // instead of waiting for the full timeline refresh. Queued pipelines get + // the same label the persisted row will show, so the stub doesn't flash a + // progress title for work that hasn't started. + const queued = result.sessionStatus === 'queued'; + addPendingSession(branch.id, { + key: pendingKey, + type: queued ? 'queued-commit' : 'pending-commit', + title: queued ? branchCommandLabels[kind] : kind === 'rebase' ? 'Rebasing…' : 'Squashing…', + secondaryMeta: queued ? queuedSessionMeta(timeline) : undefined, + sessionId: result.sessionId, + }); await loadTimeline(); } catch (e) { notifyError(kind === 'rebase' ? 'Rebase failed' : 'Squash failed', e); @@ -604,13 +617,21 @@ let branchIdentityWarning = $derived(gitIdentityWarning(timeline?.gitState)); let gitUnsafeActionsDisabled = $derived(!!branchIdentityWarning); + /** + * Gate for Rebase/Squash. In-flight sessions are not a reason to disable: + * both queue on the branch session queue and drain when the branch frees up. + * Only identity problems (detached HEAD, wrong branch) make them unsafe. + */ let branchCommandDisabledReason = $derived( - branchIdentityWarning ?? - (commandPipelinePending - ? 'Command in progress' - : branchSessionBusy - ? 'Session in progress' - : null) + branchIdentityWarning ?? (commandPipelinePending ? 'Command in progress' : null) + ); + /** + * Gate for git actions that still execute immediately (push, force-push, + * reset to origin). Those have no queue support yet, so a busy branch has to + * keep blocking them. + */ + let immediateGitActionDisabledReason = $derived( + branchCommandDisabledReason ?? (branchSessionBusy ? 'Session in progress' : null) ); // ========================================================================= @@ -1727,10 +1748,7 @@ onNoteCreated={() => loadTimeline()} onRebaseBranch={() => startBranchCommandPipeline('rebase')} onSquashCommits={() => startBranchCommandPipeline('squash')} - newCommitDisabled={sessionMgr.isNewSessionDisabled || - commandPipelinePending || - branchSessionBusy || - gitUnsafeActionsDisabled} + rebaseSquashDisabled={!!branchCommandDisabledReason} {commitCount} /> @@ -1831,7 +1849,7 @@ ? openForcePushSession : undefined} {forcePushingOrigin} - rebaseBranchDisabledReason={branchCommandDisabledReason} + {immediateGitActionDisabledReason} onViewWorktreeDiff={isLocal ? () => openDiffDetail({ diff --git a/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte b/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte index 297abbd5d..5a2d5518f 100644 --- a/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardActionsBar.svelte @@ -90,7 +90,9 @@ onNoteCreated?: () => void; onRebaseBranch?: () => void; onSquashCommits?: () => void; - newCommitDisabled?: boolean; + /** Rebase/Squash queue behind running sessions, so this covers only the + * cases where they can't run at all (detached HEAD, wrong branch). */ + rebaseSquashDisabled?: boolean; commitCount?: number; } @@ -106,7 +108,7 @@ onNoteCreated, onRebaseBranch, onSquashCommits, - newCommitDisabled = false, + rebaseSquashDisabled = false, commitCount = 0, }: Props = $props(); @@ -923,11 +925,11 @@ Rename Branch - onRebaseBranch?.()}> + onRebaseBranch?.()}> Rebase Branch {#if commitCount >= 2} - onSquashCommits?.()}> + onSquashCommits?.()}> Squash Commits {/if} diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 23f649792..9e36f6441 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -95,7 +95,14 @@ onOpenForcePushSession?: () => void; forcePushingOrigin?: boolean; onOpenPushSession?: () => void; - rebaseBranchDisabledReason?: string | null; + /** + * Why push, force-push, and reset-to-origin can't run right now. + * + * These git actions still execute immediately rather than queueing, so + * unlike Rebase/Squash they stay disabled while the branch has sessions in + * flight. + */ + immediateGitActionDisabledReason?: string | null; onViewWorktreeDiff?: () => void; onCommitWorktreeChanges?: () => void; onDiscardWorktreeChanges?: () => void; @@ -151,7 +158,7 @@ onOpenForcePushSession, forcePushingOrigin = false, onOpenPushSession, - rebaseBranchDisabledReason, + immediateGitActionDisabledReason, onViewWorktreeDiff, onCommitWorktreeChanges, onDiscardWorktreeChanges, @@ -388,7 +395,7 @@ const summary = `is ${plural(state.upstream.ahead, 'commit')} behind`; const disabledReason = pushingOrigin ? undefined // button is clickable during push (opens session) - : (rebaseBranchDisabledReason ?? undefined); + : (immediateGitActionDisabledReason ?? undefined); rows.push({ key: 'git-local-ahead', type: 'git-push', @@ -437,7 +444,7 @@ : forcePushingOrigin ? 'Push in progress' : onResetToOrigin - ? (rebaseBranchDisabledReason ?? undefined) + ? (immediateGitActionDisabledReason ?? undefined) : undefined; rows.push({ key: 'git-diverged', @@ -448,13 +455,13 @@ order: placement.order, onForcePush: forcePushingOrigin ? onOpenForcePushSession - : rebaseBranchDisabledReason + : immediateGitActionDisabledReason ? undefined : onForcePush, forcePushDisabledReason: forcePushingOrigin ? undefined : onForcePush - ? (rebaseBranchDisabledReason ?? undefined) + ? (immediateGitActionDisabledReason ?? undefined) : undefined, forcePushing: forcePushingOrigin, onResetToOrigin: resetToOriginReason ? undefined : onResetToOrigin, diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index ac8d71022..796b60ee9 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -490,6 +490,17 @@ export interface BranchSessionResponse { sessionStatus: BranchSessionLaunchStatus; } +/** + * Result of a branch git pipeline command (rebase, squash). + * + * These can be requested while the branch already has sessions in flight, in + * which case the backend queues them and reports `'queued'`. + */ +export interface BranchPipelineResponse { + sessionId: string; + sessionStatus: BranchSessionLaunchStatus; +} + // ============================================================================= // Session status event payload // ============================================================================= From 41a8130a42c00b383a9af2cc1c2d2a779e0abc07 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 16:08:53 +1000 Subject: [PATCH 2/9] feat(staged): queue push/force-push behind running branch sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of the queued git actions plan: push and force push now join the per-branch session queue instead of being disabled while the branch is busy, and a push in flight blocks other branch work the way a commit does. Backend: - Migration 0021 adds nullable `sessions.branch_id`. Push pipelines create no commit/note/review, so the artifact joins could not see them; `get_queued_sessions_for_branch`, `has_running_session_for_branch` and the branch/project resolvers now also match on that column. - `PipelineKind::Push` plus a persisted `push_force` flag, so a queued push re-derives the same command on dequeue and a queued normal push is distinguishable from a queued force push. - New `GitPipeline` schedule kind: exclusive and queue-blocking. Queued pushes drain through `start_queued_git_pipeline_for_branch`, which rebuilds the steps from the branch's current name rather than replaying the queued ones. - `push_branch` returns `BranchPipelineResponse` (running vs queued); queue-vs-run, dedupe on `(kind, push_force)`, and the insert all happen under the branch launch lock, mirroring rebase/squash. - Bonus fix: because a running push resolves to a `GitPipeline` schedule, it now blocks new notes/commits/reviews. Previously a push was invisible to the queue and blocked nothing. Frontend: - `pushState` gains a `queued` state; the PR button and the git-push / diverged timeline rows show it and cancel the queued session on click. The queued entry flips to `pushing` on the drain's "running" event, with the 5s poller as a fallback. - Push and force-push no longer check `branchSessionBusy` — the backend owns that decision. Reset to origin still does: it is validated against a point-in-time preview of what would be discarded, so it stays immediate-only, as does discard changes. `BranchTimeline` therefore takes a separate `queueableGitActionDisabledReason` alongside the existing `immediateGitActionDisabledReason`. Known gap: the queued badge lives only in the frontend store, so after an app restart a still-queued push drains without the UI showing it as queued first. Also note that per AGENTS.md the new `Session.branch_id` and `PipelineExecution.push_force` fields are data-model additions that want human review; they are the shape the plan note specified. Tests: migration schema version/column, branch-linked queued and running push queries, drain-scan blocking and FIFO ordering for pushes, queue-vs-run and dedupe for push vs force push, step rebuilding, and the new `pushBranch` response shape. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/prs.rs | 548 +++++++++++++++--- apps/staged/src-tauri/src/session_commands.rs | 208 ++++++- apps/staged/src-tauri/src/session_runner.rs | 7 +- .../src-tauri/src/store/migration_tests.rs | 9 +- .../0022-add-session-branch-id/up.sql | 8 + apps/staged/src-tauri/src/store/models.rs | 71 ++- apps/staged/src-tauri/src/store/sessions.rs | 36 +- apps/staged/src-tauri/src/store/tests.rs | 75 +++ apps/staged/src-tauri/src/web_server.rs | 4 +- apps/staged/src/lib/commands.test.ts | 27 + apps/staged/src/lib/commands.ts | 9 +- .../lib/features/branches/BranchCard.svelte | 55 +- .../branches/BranchCardPrButton.svelte | 60 +- .../features/timeline/BranchTimeline.svelte | 81 ++- .../lib/features/timeline/TimelineRow.svelte | 23 +- .../lib/listeners/sessionStatusListener.ts | 5 + .../staged/src/lib/stores/pushState.svelte.ts | 48 +- apps/staged/src/lib/types.ts | 4 +- 18 files changed, 1086 insertions(+), 192 deletions(-) create mode 100644 apps/staged/src-tauri/src/store/migrations/0022-add-session-branch-id/up.sql diff --git a/apps/staged/src-tauri/src/prs.rs b/apps/staged/src-tauri/src/prs.rs index bbf9457a1..2cabd2ad5 100644 --- a/apps/staged/src-tauri/src/prs.rs +++ b/apps/staged/src-tauri/src/prs.rs @@ -235,10 +235,20 @@ fn rebase_ref_for_target(branch: &store::Branch, target: Option<&str>) -> String } } -fn commit_pipeline_prompt(kind: &PipelineKind) -> &'static str { +const PUSH_PROMPT: &str = "Push the current branch to the remote with a normal push. If the push fails for a recoverable reason, diagnose and fix it, then retry with a normal push. Do not force push."; +const FORCE_PUSH_PROMPT: &str = "Force push the current branch to the remote"; + +/// Prompt for a pipeline session, which doubles as its timeline label. +/// +/// `push_force` only matters for [`PipelineKind::Push`]; the commit kinds ignore +/// it. Queued and running paths share this so a session's label doesn't change +/// when it is drained. +fn pipeline_prompt(kind: &PipelineKind, push_force: bool) -> &'static str { match kind { PipelineKind::Rebase => "Rebase branch", PipelineKind::Squash => "Squash commits", + PipelineKind::Push if push_force => FORCE_PUSH_PROMPT, + PipelineKind::Push => PUSH_PROMPT, } } @@ -255,31 +265,37 @@ fn persisted_rebase_target(target: Option<&str>, rebase_ref: &str) -> Option, branch_id: &str, - kind: &PipelineKind, - rebase_target: Option<&str>, + matches: impl Fn(&PipelineExecution) -> bool, ) -> Result, String> { let queued = store .get_queued_sessions_for_branch(branch_id) .map_err(|e| e.to_string())?; - for session in queued { - let Some(pipeline) = session.pipeline.as_ref() else { - continue; - }; - if pipeline.kind.as_ref() == Some(kind) - && pipeline.rebase_target.as_deref() == rebase_target - { - return Ok(Some(session.id)); - } - } + Ok(queued + .into_iter() + .find(|session| session.pipeline.as_ref().is_some_and(&matches)) + .map(|session| session.id)) +} - Ok(None) +/// Whether the branch already has work in flight, and a new request therefore +/// has to join the queue instead of starting now. +/// +/// Callers must hold the branch launch lock: the answer is only meaningful for +/// as long as no session can start or be enqueued underneath them. +fn branch_has_work_in_flight(store: &Arc, branch_id: &str) -> Result { + Ok(store + .has_running_session_for_branch(branch_id) + .map_err(|e| e.to_string())? + || !store + .get_queued_sessions_for_branch(branch_id) + .map_err(|e| e.to_string())? + .is_empty()) } const HTTPS_FALLBACK_CONFIG: &str = "url.https://github.com/.insteadOf=git@github.com:"; @@ -306,12 +322,17 @@ fn git_push_with_fallback(args: &str) -> String { /// the branch's own name for "rebase onto origin" (used when local has /// diverged from `origin/{branch}`). Only the rebase variant consults this /// value; squash always operates against the base branch. +/// +/// Errors for [`PipelineKind::Push`], which produces no commit and belongs to +/// the git pipeline path — the only caller that reads the kind from the database +/// checks it first, so this guards against a misrouted queued session rather +/// than a reachable input. fn build_commit_pipeline_steps( kind: &PipelineKind, base_branch: &str, rebase_target: &str, -) -> Vec { - match kind { +) -> Result, String> { + let steps = match kind { PipelineKind::Rebase => { let target_note = if base_branch == rebase_target { String::new() @@ -398,7 +419,50 @@ Here is the context from the prior steps: .to_string(), }, ], - } + PipelineKind::Push => { + return Err("Push is not a commit pipeline".to_string()); + } + }; + + Ok(steps) +} + +/// Build the single step that pushes the branch to its remote. +/// +/// Rebuilt from the persisted `push_force` flag on dequeue rather than replayed +/// from the queued pipeline, so a queued push always pushes the branch's current +/// name with the command the user asked for. +fn build_push_pipeline_steps(branch_name: &str, force: bool) -> Vec { + let push_command = if force { + git_push_with_fallback(&format!("-u origin {branch_name} --force-with-lease")) + } else { + git_push_with_fallback(&format!("-u origin {branch_name}")) + }; + + let on_failure = if force { + FailureStrategy::HandoffToAi { + prompt_template: + "The force push failed. Diagnose and fix the issue, then retry the force push.\n\n{step_outputs}" + .to_string(), + } + } else { + // For normal push, abort on non-fast-forward so the frontend can show + // the force-push dialog. The marker matches git's actual stderr output + // (e.g. "! [rejected] main -> main (non-fast-forward)"). + // + // If the push fails for a *different* reason (e.g. auth error, network + // timeout), the marker won't match and the pipeline falls through to an + // AI handoff for generic diagnosis — this is intentional. + FailureStrategy::Abort { + marker: Some("non-fast-forward".to_string()), + } + }; + + vec![PipelineStep::Command { + label: "Push to remote".to_string(), + command: push_command, + on_failure, + }] } #[allow(clippy::too_many_arguments)] @@ -412,7 +476,7 @@ async fn start_running_commit_pipeline_for_branch( app_handle: &tauri::AppHandle, registry: &Arc, ) -> Result { - let prompt = commit_pipeline_prompt(&kind); + let prompt = pipeline_prompt(&kind, false); let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind); if let Some(target) = rebase_target { pipeline = pipeline.with_rebase_target(target); @@ -481,14 +545,7 @@ fn queue_commit_pipeline_if_branch_busy( let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); let _guard = launch_lock.lock().unwrap(); - let branch_busy = store - .has_running_session_for_branch(branch_id) - .map_err(|e| e.to_string())? - || !store - .get_queued_sessions_for_branch(branch_id) - .map_err(|e| e.to_string())? - .is_empty(); - if !branch_busy { + if !branch_has_work_in_flight(store, branch_id)? { return Ok(None); } @@ -500,18 +557,19 @@ fn queue_commit_pipeline_if_branch_busy( let rebase_ref = rebase_ref_for_target(&branch, target); let rebase_target = persisted_rebase_target(target, &rebase_ref); - if let Some(existing) = - find_queued_commit_pipeline(store, branch_id, kind, rebase_target.as_deref())? - { + if let Some(existing) = find_queued_pipeline(store, branch_id, |pipeline| { + pipeline.kind.as_ref() == Some(kind) + && pipeline.rebase_target.as_deref() == rebase_target.as_deref() + })? { return Ok(Some(existing)); } - let steps = build_commit_pipeline_steps(kind, base_branch, &rebase_ref); + let steps = build_commit_pipeline_steps(kind, base_branch, &rebase_ref)?; let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind.clone()); if let Some(target) = rebase_target { pipeline = pipeline.with_rebase_target(target); } - let mut session = store::Session::new_queued(commit_pipeline_prompt(kind)); + let mut session = store::Session::new_queued(pipeline_prompt(kind, false)); if let Some(p) = provider { session = session.with_provider(p); } @@ -547,7 +605,7 @@ pub(crate) async fn start_or_queue_commit_pipeline_for_branch( let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; let base_branch = base_branch_name(&ctx.branch).to_string(); let rebase_ref = rebase_ref_for_target(&ctx.branch, target.as_deref()); - let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref); + let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref)?; let rebase_target = persisted_rebase_target(target.as_deref(), &rebase_ref); let session_id = start_running_commit_pipeline_for_branch( @@ -588,8 +646,8 @@ pub(crate) async fn start_queued_commit_pipeline_for_branch( let rebase_ref = queued_rebase_target .clone() .unwrap_or_else(|| base_branch.clone()); - let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref); - let prompt = commit_pipeline_prompt(&kind); + let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref)?; + let prompt = pipeline_prompt(&kind, false); let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind); if let Some(target) = queued_rebase_target { pipeline = pipeline.with_rebase_target(target); @@ -647,6 +705,194 @@ pub(crate) async fn start_queued_commit_pipeline_for_branch( Ok(true) } +/// Start a push pipeline that runs right now. +/// +/// Unlike [`start_pipeline_for_branch`], the session records its pipeline kind +/// and `branch_id`. A push creates no artifact, so without that link the branch +/// queue could not see it and a commit session could start mid-push. +fn start_running_push_pipeline_for_branch( + ctx: BranchPipelineContext, + force: bool, + steps: Vec, + provider: Option, + store: Arc, + app_handle: &tauri::AppHandle, + registry: &Arc, +) -> Result { + let prompt = pipeline_prompt(&PipelineKind::Push, force); + let pipeline = PipelineExecution::from_steps(&steps) + .with_kind(PipelineKind::Push) + .with_push_force(force); + + let branch_id = ctx.branch.id.clone(); + let project_id = ctx.branch.project_id.clone(); + + let mut session = store::Session::new_running(prompt, &ctx.working_dir).with_branch(&branch_id); + if let Some(ref p) = provider { + session = session.with_provider(p); + } + session.pipeline = Some(pipeline.clone()); + store.create_session(&session).map_err(|e| e.to_string())?; + + // Emit "running" before returning so the global session listener registers + // this session atomically, as `start_pipeline_for_branch` does. + session_runner::emit_session_running(app_handle, &session.id, &branch_id, &project_id, "push"); + + session_runner::start_pipeline_session( + session_runner::PipelineConfig { + session_id: session.id.clone(), + prompt: prompt.to_string(), + steps, + pipeline, + working_dir: ctx.working_dir, + pre_head_sha: None, + provider, + workspace_name: ctx.workspace_name, + remote_working_dir: ctx.remote_working_dir, + branch_id: Some(branch_id), + project_id: Some(project_id), + }, + store, + app_handle.clone(), + Arc::clone(registry), + )?; + + Ok(session.id) +} + +/// Queue a push pipeline when the branch has work in flight. +/// +/// Returns the queued session id — either a freshly created one, or an existing +/// queued push that already covers this request — and `None` when the branch is +/// idle so the caller should push immediately. +/// +/// Mirrors [`queue_commit_pipeline_if_branch_busy`]: the busy check, the dedupe +/// scan, and the insert all run under the branch launch lock, and stay +/// synchronous because the lock must not be held across an await. A push and a +/// force push dedupe separately — they are different operations, so a queued +/// normal push must not swallow a force push request. +fn queue_push_pipeline_if_branch_busy( + store: &Arc, + branch_id: &str, + provider: Option<&str>, + force: bool, +) -> Result, String> { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); + let _guard = launch_lock.lock().unwrap(); + + if !branch_has_work_in_flight(store, branch_id)? { + return Ok(None); + } + + if let Some(existing) = find_queued_pipeline(store, branch_id, |pipeline| { + pipeline.kind.as_ref() == Some(&PipelineKind::Push) && pipeline.push_force == force + })? { + return Ok(Some(existing)); + } + + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + + let steps = build_push_pipeline_steps(&branch.branch_name, force); + let pipeline = PipelineExecution::from_steps(&steps) + .with_kind(PipelineKind::Push) + .with_push_force(force); + // No artifact row: a push produces no commit, and a pending-commit stub + // would render as a failed commit once the push finishes without a new sha. + // `branch_id` is what keeps this session on the branch queue instead. + let mut session = store::Session::new_queued(pipeline_prompt(&PipelineKind::Push, force)) + .with_branch(branch_id); + if let Some(p) = provider { + session = session.with_provider(p); + } + session.pipeline = Some(pipeline); + store.create_session(&session).map_err(|e| e.to_string())?; + + Ok(Some(session.id)) +} + +/// Start a queued push pipeline that reached the front of the branch queue. +/// +/// Steps are rebuilt from the persisted `push_force` flag rather than replayed +/// from the queued pipeline, matching [`start_queued_commit_pipeline_for_branch`] +/// so a branch renamed while the push waited still pushes the right ref. +pub(crate) async fn start_queued_git_pipeline_for_branch( + store: Arc, + registry: Arc, + app_handle: tauri::AppHandle, + branch_id: String, + session: store::Session, + provider: Option, +) -> Result { + let queued_pipeline = session + .pipeline + .as_ref() + .ok_or_else(|| format!("Queued session {} has no pipeline", session.id))?; + let kind = queued_pipeline + .kind + .clone() + .ok_or_else(|| format!("Queued session {} has no pipeline kind", session.id))?; + if kind != PipelineKind::Push { + return Err(format!( + "Queued git pipeline session {} has non-git kind {kind:?}", + session.id + )); + } + let force = queued_pipeline.push_force; + + let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; + let steps = build_push_pipeline_steps(&ctx.branch.branch_name, force); + let prompt = pipeline_prompt(&kind, force); + let pipeline = PipelineExecution::from_steps(&steps) + .with_kind(kind) + .with_push_force(force); + let effective_provider = session.provider.clone().or(provider); + + let transitioned = store + .transition_queued_to_running(&session.id) + .map_err(|e| e.to_string())?; + if !transitioned { + return Ok(false); + } + + // No `mark_session_artifact_started` call: a push has no queued artifact stub + // whose timestamp needs restamping when the work actually starts. + store + .prepare_queued_session(&session.id, &ctx.working_dir.to_string_lossy(), prompt) + .map_err(|e| e.to_string())?; + store + .update_session_pipeline(&session.id, &pipeline) + .map_err(|e| e.to_string())?; + + let branch_id = ctx.branch.id.clone(); + let project_id = ctx.branch.project_id.clone(); + + session_runner::emit_session_running(&app_handle, &session.id, &branch_id, &project_id, "push"); + + session_runner::start_pipeline_session( + session_runner::PipelineConfig { + session_id: session.id.clone(), + prompt: prompt.to_string(), + steps, + pipeline, + working_dir: ctx.working_dir, + pre_head_sha: None, + provider: effective_provider, + workspace_name: ctx.workspace_name, + remote_working_dir: ctx.remote_working_dir, + branch_id: Some(branch_id), + project_id: Some(project_id), + }, + store, + app_handle, + Arc::clone(®istry), + )?; + + Ok(true) +} + fn create_pr_handoff_prompt( pr_type: &str, base_branch: &str, @@ -1297,71 +1543,44 @@ pub(crate) async fn has_unpushed_commits_impl( .map_err(|e| format!("has_unpushed_commits task failed: {e}"))? } -/// Push a branch to its remote by kicking off an agent session. -pub(crate) async fn start_push_branch_pipeline_for_branch( +/// Push a branch to its remote by kicking off an agent session, or queue the +/// push behind whatever the branch is already doing. +pub(crate) async fn start_or_queue_push_pipeline_for_branch( store: Arc, registry: Arc, app_handle: tauri::AppHandle, branch_id: String, provider: Option, force: Option, -) -> Result { - let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; - +) -> Result { let force = force.unwrap_or(false); - let push_command = if force { - git_push_with_fallback(&format!( - "-u origin {} --force-with-lease", - ctx.branch.branch_name - )) - } else { - git_push_with_fallback(&format!("-u origin {}", ctx.branch.branch_name)) - }; - - let on_failure = if force { - FailureStrategy::HandoffToAi { - prompt_template: - "The force push failed. Diagnose and fix the issue, then retry the force push.\n\n{step_outputs}" - .to_string(), - } - } else { - // For normal push, abort on non-fast-forward so the frontend can show - // the force-push dialog. The marker matches git's actual stderr output - // (e.g. "! [rejected] main -> main (non-fast-forward)"). - // - // If the push fails for a *different* reason (e.g. auth error, network - // timeout), the marker won't match and the pipeline falls through to an - // AI handoff for generic diagnosis — this is intentional. - FailureStrategy::Abort { - marker: Some("non-fast-forward".to_string()), - } - }; - - let steps = vec![PipelineStep::Command { - label: "Push to remote".to_string(), - command: push_command, - on_failure, - }]; + if let Some(session_id) = + queue_push_pipeline_if_branch_busy(&store, &branch_id, provider.as_deref(), force)? + { + return Ok(BranchPipelineResponse::queued(session_id)); + } - let prompt = if force { - "Force push the current branch to the remote".to_string() - } else { - "Push the current branch to the remote with a normal push. If the push fails for a recoverable reason, diagnose and fix it, then retry with a normal push. Do not force push.".to_string() - }; + let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; + let steps = build_push_pipeline_steps(&ctx.branch.branch_name, force); - start_pipeline_for_branch( + let session_id = start_running_push_pipeline_for_branch( ctx, + force, steps, - &prompt, - "push", provider, store, &app_handle, ®istry, - ) + )?; + + Ok(BranchPipelineResponse::running(session_id)) } +/// Push a branch to its remote. +/// +/// Queues behind in-flight branch work instead of failing, so the response +/// reports whether the push started or is waiting on the branch queue. #[tauri::command(rename_all = "camelCase")] pub async fn push_branch( store: tauri::State<'_, Mutex>>>, @@ -1370,9 +1589,9 @@ pub async fn push_branch( branch_id: String, provider: Option, force: Option, -) -> Result { +) -> Result { let store = get_store(&store)?; - start_push_branch_pipeline_for_branch( + start_or_queue_push_pipeline_for_branch( store, Arc::clone(®istry), app_handle, @@ -1626,6 +1845,149 @@ mod tests { assert!(queue_pipeline(&store, &branch.id, PipelineKind::Squash, None).is_some()); } + fn queue_push(store: &Arc, branch_id: &str, force: bool) -> Option { + queue_push_pipeline_if_branch_busy(store, branch_id, None, force).unwrap() + } + + #[test] + fn idle_branch_pushes_immediately_instead_of_queueing() { + let (store, branch) = setup_branch_store(); + + assert_eq!(queue_push(&store, &branch.id, false), None); + assert!(store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .is_empty()); + } + + #[test] + fn busy_branch_queues_push_with_branch_link_and_no_artifact() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let session_id = queue_push(&store, &branch.id, false) + .expect("push should queue behind the running note session"); + + let session = store.get_session(&session_id).unwrap().unwrap(); + assert_eq!(session.status, store::SessionStatus::Queued); + assert_eq!(session.prompt, PUSH_PROMPT); + // The branch link is what keeps an artifact-less push on the queue. + assert_eq!(session.branch_id.as_deref(), Some(branch.id.as_str())); + let pipeline = session.pipeline.unwrap(); + assert_eq!(pipeline.kind, Some(PipelineKind::Push)); + assert!(!pipeline.push_force); + + // A pending commit would render as a failed commit once the push ends. + assert!(store + .list_commits_for_branch(&branch.id) + .unwrap() + .is_empty()); + } + + #[test] + fn repeated_push_click_reuses_the_already_queued_push() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let first = queue_push(&store, &branch.id, false).unwrap(); + let second = queue_push(&store, &branch.id, false).unwrap(); + + assert_eq!(first, second); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 1 + ); + } + + #[test] + fn force_push_is_not_deduped_against_a_queued_normal_push() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let push = queue_push(&store, &branch.id, false).unwrap(); + let force_push = queue_push(&store, &branch.id, true).unwrap(); + + assert_ne!(push, force_push); + let forced = store.get_session(&force_push).unwrap().unwrap(); + assert_eq!(forced.prompt, FORCE_PUSH_PROMPT); + assert!(forced.pipeline.unwrap().push_force); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 2 + ); + + // Clicking force push again still dedupes. + assert_eq!(queue_push(&store, &branch.id, true), Some(force_push)); + } + + #[test] + fn queued_push_alone_keeps_the_branch_busy() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + queue_push(&store, &branch.id, false).unwrap(); + + // The note session finishes, but the still-queued push must keep a newly + // requested squash on the queue rather than running it now. + for session in store.get_running_sessions().unwrap() { + store + .update_session_status(&session.id, store::SessionStatus::Completed, None, None) + .unwrap(); + } + + assert!(queue_pipeline(&store, &branch.id, PipelineKind::Squash, None).is_some()); + } + + #[test] + fn queued_rebase_keeps_a_later_push_on_the_queue() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).unwrap(); + + for session in store.get_running_sessions().unwrap() { + store + .update_session_status(&session.id, store::SessionStatus::Completed, None, None) + .unwrap(); + } + + assert!(queue_push(&store, &branch.id, false).is_some()); + } + + #[test] + fn push_steps_are_rebuilt_from_the_current_branch_name_and_force_flag() { + let normal = build_push_pipeline_steps("feature", false); + let (label, command, on_failure) = command_at(&normal, 0); + assert_eq!(label, "Push to remote"); + assert!(command.contains("-u origin feature")); + assert!(!command.contains("--force-with-lease")); + assert!(matches!( + on_failure, + FailureStrategy::Abort { marker } if marker.as_deref() == Some("non-fast-forward") + )); + + // The drain path re-derives the ref, so a branch renamed while the push + // waited pushes the new name rather than the queued one. + let forced = build_push_pipeline_steps("feature-renamed", true); + let (_, forced_command, forced_on_failure) = command_at(&forced, 0); + assert!(forced_command.contains("-u origin feature-renamed --force-with-lease")); + assert!(matches!( + forced_on_failure, + FailureStrategy::HandoffToAi { .. } + )); + } + + #[test] + fn commit_pipeline_steps_reject_the_push_kind() { + let err = build_commit_pipeline_steps(&PipelineKind::Push, "main", "main").unwrap_err(); + + assert_eq!(err, "Push is not a commit pipeline"); + } + #[tokio::test] async fn collect_branch_refresh_results_tolerates_partial_task_failure() { let tasks = vec![ @@ -1717,7 +2079,7 @@ mod tests { #[test] fn rebase_pipeline_uses_signoff() { - let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "main"); + let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "main").unwrap(); let (label, command, _) = command_at(&steps, 0); assert_eq!(label, "Fetch latest base"); @@ -1733,7 +2095,8 @@ mod tests { #[test] fn rebase_pipeline_targets_origin_branch_when_target_differs() { - let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "feature-branch"); + let steps = + build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "feature-branch").unwrap(); let (label, command, _) = command_at(&steps, 0); assert_eq!(label, "Fetch origin/feature-branch"); @@ -1749,7 +2112,8 @@ mod tests { #[test] fn rebase_pipeline_prompt_mentions_base_when_target_differs() { - let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "feature-branch"); + let steps = + build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "feature-branch").unwrap(); let fetch_failure = match &steps[0] { PipelineStep::Command { @@ -1776,7 +2140,7 @@ mod tests { #[test] fn rebase_pipeline_prompt_omits_target_note_when_target_matches_base() { - let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "main"); + let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "main").unwrap(); for step in &steps { if let PipelineStep::Command { @@ -1792,7 +2156,7 @@ mod tests { #[test] fn squash_pipeline_prompt_requires_signoff() { - let steps = build_commit_pipeline_steps(&PipelineKind::Squash, "main", "main"); + let steps = build_commit_pipeline_steps(&PipelineKind::Squash, "main", "main").unwrap(); let (_, prompt) = ai_prompt_at(&steps, 3); assert!(prompt.contains("Use the user's global git identity")); diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index fd461d83b..a9da0c3b2 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -1757,13 +1757,19 @@ enum BranchSessionScheduleKind { Note, Review, CommitPipeline, + /// A git command pipeline that mutates the branch without producing a + /// commit (push / force push). Exclusive: it rewrites the remote from the + /// current worktree, so nothing else may touch the branch while it runs. + GitPipeline, } impl BranchSessionScheduleKind { fn is_exclusive(self) -> bool { matches!( self, - BranchSessionScheduleKind::Commit | BranchSessionScheduleKind::CommitPipeline + BranchSessionScheduleKind::Commit + | BranchSessionScheduleKind::CommitPipeline + | BranchSessionScheduleKind::GitPipeline ) } @@ -1776,7 +1782,9 @@ impl BranchSessionScheduleKind { BranchSessionScheduleKind::Commit => Some(BranchSessionType::Commit), BranchSessionScheduleKind::Note => Some(BranchSessionType::Note), BranchSessionScheduleKind::Review => Some(BranchSessionType::Review), - BranchSessionScheduleKind::CommitPipeline => None, + BranchSessionScheduleKind::CommitPipeline | BranchSessionScheduleKind::GitPipeline => { + None + } } } } @@ -1819,7 +1827,9 @@ fn review_session_schedule(review: &store::Review) -> BranchSessionSchedule { } } -fn commit_session_schedule(kind: BranchSessionScheduleKind) -> BranchSessionSchedule { +/// Schedule for the kinds that take the branch exclusively (commit sessions and +/// command pipelines): they always block the queue and carry no review. +fn exclusive_session_schedule(kind: BranchSessionScheduleKind) -> BranchSessionSchedule { BranchSessionSchedule { kind, review_id: None, @@ -1827,12 +1837,21 @@ fn commit_session_schedule(kind: BranchSessionScheduleKind) -> BranchSessionSche } } -fn is_commit_pipeline_session(session: &store::Session) -> bool { - session - .pipeline - .as_ref() - .and_then(|pipeline| pipeline.kind.as_ref()) - .is_some() +/// How a command-pipeline session is tied to the branch it belongs to. +enum PipelineBranchLink { + /// Rebase/squash: found through the pending-commit artifact they create. + Commit, + /// Push: creates no artifact, so it records `sessions.branch_id` instead. + Branch, +} + +fn pipeline_branch_link(session: &store::Session) -> Option { + match session.pipeline.as_ref()?.kind.as_ref()? { + store::PipelineKind::Rebase | store::PipelineKind::Squash => { + Some(PipelineBranchLink::Commit) + } + store::PipelineKind::Push => Some(PipelineBranchLink::Branch), + } } fn resolve_branch_session_schedule( @@ -1841,21 +1860,37 @@ fn resolve_branch_session_schedule( session: &store::Session, require_artifact: bool, ) -> Result, String> { - if is_commit_pipeline_session(session) { - let commit = store - .get_commit_by_session(&session.id) - .map_err(|e| e.to_string())?; - return match commit { - Some(commit) if commit.branch_id == branch_id => Ok(Some(commit_session_schedule( - BranchSessionScheduleKind::CommitPipeline, - ))), - Some(_) => Ok(None), - None if require_artifact => Err(format!( - "Queued pipeline session {} has no linked commit", - session.id - )), - None => Ok(None), - }; + match pipeline_branch_link(session) { + Some(PipelineBranchLink::Commit) => { + let commit = store + .get_commit_by_session(&session.id) + .map_err(|e| e.to_string())?; + return match commit { + Some(commit) if commit.branch_id == branch_id => Ok(Some( + exclusive_session_schedule(BranchSessionScheduleKind::CommitPipeline), + )), + Some(_) => Ok(None), + None if require_artifact => Err(format!( + "Queued pipeline session {} has no linked commit", + session.id + )), + None => Ok(None), + }; + } + Some(PipelineBranchLink::Branch) => { + return match session.branch_id.as_deref() { + Some(linked) if linked == branch_id => Ok(Some(exclusive_session_schedule( + BranchSessionScheduleKind::GitPipeline, + ))), + Some(_) => Ok(None), + None if require_artifact => Err(format!( + "Queued git pipeline session {} has no linked branch", + session.id + )), + None => Ok(None), + }; + } + None => {} } if let Some(commit) = store @@ -1863,7 +1898,7 @@ fn resolve_branch_session_schedule( .map_err(|e| e.to_string())? { return Ok((commit.branch_id == branch_id) - .then(|| commit_session_schedule(BranchSessionScheduleKind::Commit))); + .then(|| exclusive_session_schedule(BranchSessionScheduleKind::Commit))); } if let Some(note) = store @@ -2909,6 +2944,13 @@ async fn start_queued_session_for_branch( .await; } + if matches!(schedule.kind, BranchSessionScheduleKind::GitPipeline) { + return crate::prs::start_queued_git_pipeline_for_branch( + store, registry, app_handle, branch_id, session, provider, + ) + .await; + } + let session_type = schedule.kind.branch_session_type().ok_or_else(|| { format!( "Queued session {} cannot start as an agent session", @@ -5180,6 +5222,28 @@ mod tests { session } + /// A push pipeline session, linked to the branch without any artifact row. + fn create_branch_push_pipeline_session( + store: &Arc, + branch_id: &str, + status: store::SessionStatus, + force: bool, + ) -> store::Session { + let mut session = match status { + store::SessionStatus::Queued => store::Session::new_queued("push"), + store::SessionStatus::Running => store::Session::new_running("push", Path::new("/tmp")), + other => panic!("unsupported scheduler test status: {}", other.as_str()), + } + .with_branch(branch_id); + session.pipeline = Some( + store::PipelineExecution::from_steps(&[]) + .with_kind(store::PipelineKind::Push) + .with_push_force(force), + ); + store.create_session(&session).unwrap(); + session + } + fn schedule(kind: BranchSessionScheduleKind) -> BranchSessionSchedule { BranchSessionSchedule { kind, @@ -5382,6 +5446,100 @@ mod tests { )); } + #[test] + fn running_push_pipeline_blocks_queued_note_review_and_commit() { + let (store, branch) = setup_branch_store(); + create_branch_push_pipeline_session( + &store, + &branch.id, + store::SessionStatus::Running, + false, + ); + + let active = running_branch_session_kinds(&store, &branch.id).unwrap(); + + assert!(active.contains(&BranchSessionScheduleKind::GitPipeline)); + for kind in [ + BranchSessionScheduleKind::Note, + BranchSessionScheduleKind::Review, + BranchSessionScheduleKind::Commit, + ] { + assert!(!can_start_with_active_branch_sessions(kind, &active)); + } + } + + #[test] + fn running_push_pipeline_on_other_branch_does_not_block() { + let (store, branch) = setup_branch_store(); + let other = store::Branch::new(&branch.project_id, "other", "main"); + store.create_branch(&other).unwrap(); + create_branch_push_pipeline_session(&store, &other.id, store::SessionStatus::Running, true); + + let active = running_branch_session_kinds(&store, &branch.id).unwrap(); + + assert!(active.is_empty()); + } + + #[test] + fn branch_start_decision_queues_all_user_modes_behind_queued_push() { + let (store, branch) = setup_branch_store_with_workdir(); + create_branch_push_pipeline_session(&store, &branch.id, store::SessionStatus::Queued, true); + + for session_type in [ + BranchSessionType::Note, + BranchSessionType::Review, + BranchSessionType::Commit, + ] { + assert!(should_queue_branch_session_start(&store, &branch.id, &session_type).unwrap()); + } + } + + #[test] + fn queued_push_acts_as_fifo_barrier() { + let mut active = HashSet::new(); + let queued = vec![ + ( + "note-1".to_string(), + schedule(BranchSessionScheduleKind::Note), + ), + ( + "push".to_string(), + schedule(BranchSessionScheduleKind::GitPipeline), + ), + ( + "note-2".to_string(), + schedule(BranchSessionScheduleKind::Note), + ), + ]; + + let drainable = drainable_session_ids_for_active_set(&queued, &mut active); + + assert_eq!(drainable, vec!["note-1".to_string()]); + } + + #[test] + fn drain_scan_starts_oldest_queued_push_alone() { + let mut active = HashSet::new(); + let queued = vec![ + ( + "push".to_string(), + schedule(BranchSessionScheduleKind::GitPipeline), + ), + ( + "note-1".to_string(), + schedule(BranchSessionScheduleKind::Note), + ), + ( + "commit".to_string(), + schedule(BranchSessionScheduleKind::Commit), + ), + ]; + + let drainable = drainable_session_ids_for_active_set(&queued, &mut active); + + assert_eq!(drainable, vec!["push".to_string()]); + } + #[test] fn queued_commit_acts_as_fifo_barrier() { let mut active = HashSet::new(); diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 264705d57..fab100a79 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -1416,7 +1416,9 @@ fn pre_head_for_pipeline_handoff(config: &PipelineConfig) -> Option { None } }, - None => None, + // A push never rewrites local history, so an AI handoff has no + // pre-pipeline HEAD to compare against. + Some(PipelineKind::Push) | None => None, } } @@ -1443,7 +1445,8 @@ fn resolve_pipeline_artifacts_without_ai(config: &PipelineConfig, store: &Store, ); } } - None => {} + // Push pipelines create no artifact, so there is nothing to resolve. + Some(PipelineKind::Push) | None => {} } } diff --git a/apps/staged/src-tauri/src/store/migration_tests.rs b/apps/staged/src-tauri/src/store/migration_tests.rs index bc55d2065..e0083da1e 100644 --- a/apps/staged/src-tauri/src/store/migration_tests.rs +++ b/apps/staged/src-tauri/src/store/migration_tests.rs @@ -145,7 +145,7 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { ) .unwrap(); - assert_eq!(version, 21); + assert_eq!(version, 22); assert_eq!(app_version, super::APP_VERSION); assert!(table_exists(&conn, "projects")); assert!(table_exists(&conn, "project_notes")); @@ -160,6 +160,7 @@ fn test_store_bootstraps_fresh_database_with_baseline_migration() { )); assert!(column_exists(&conn, "sessions", "acp_config_selection")); assert!(column_exists(&conn, "sessions", "acp_title")); + assert!(column_exists(&conn, "sessions", "branch_id")); let trigger_count: i64 = conn .query_row( @@ -223,10 +224,11 @@ fn test_store_repairs_github_comment_tracking_user_version() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 21); + assert_eq!(version, 22); assert!(column_exists(&conn, "sessions", "pipeline")); assert!(column_exists(&conn, "sessions", "acp_config_selection")); assert!(column_exists(&conn, "sessions", "acp_title")); + assert!(column_exists(&conn, "sessions", "branch_id")); assert!(column_exists( &conn, "session_messages", @@ -282,7 +284,7 @@ fn test_store_repairs_pipeline_user_version() { let version: i64 = conn .query_row("PRAGMA user_version", [], |row| row.get(0)) .unwrap(); - assert_eq!(version, 21); + assert_eq!(version, 22); assert!(column_exists(&conn, "comments", "github_comment_id")); assert!(column_exists(&conn, "comments", "github_comment_type")); assert!(column_exists(&conn, "comments", "github_comment_stale")); @@ -294,6 +296,7 @@ fn test_store_repairs_pipeline_user_version() { assert!(table_exists(&conn, "queued_session_messages")); assert!(column_exists(&conn, "sessions", "acp_config_selection")); assert!(column_exists(&conn, "sessions", "acp_title")); + assert!(column_exists(&conn, "sessions", "branch_id")); cleanup_db(&path); } diff --git a/apps/staged/src-tauri/src/store/migrations/0022-add-session-branch-id/up.sql b/apps/staged/src-tauri/src/store/migrations/0022-add-session-branch-id/up.sql new file mode 100644 index 000000000..8f67e2a61 --- /dev/null +++ b/apps/staged/src-tauri/src/store/migrations/0022-add-session-branch-id/up.sql @@ -0,0 +1,8 @@ +-- Link branch-scoped sessions that create no artifact directly to their branch. +-- +-- Queued/running sessions are normally found for a branch through their commit, +-- note, or review row. Push pipelines produce none of those, so they were +-- invisible to the branch queue. Existing rows keep resolving via artifacts; +-- only sessions with no artifact need this column populated. +ALTER TABLE sessions ADD COLUMN branch_id TEXT DEFAULT NULL REFERENCES branches(id) ON DELETE CASCADE; +CREATE INDEX idx_sessions_branch ON sessions(branch_id); diff --git a/apps/staged/src-tauri/src/store/models.rs b/apps/staged/src-tauri/src/store/models.rs index cb9dd0364..afe2d5d17 100644 --- a/apps/staged/src-tauri/src/store/models.rs +++ b/apps/staged/src-tauri/src/store/models.rs @@ -531,6 +531,14 @@ pub struct Session { /// Used as an interim display name while the session is running. #[serde(default)] pub acp_title: Option, + /// Branch this session belongs to, for sessions that create no artifact. + /// + /// Branch-scoped sessions are normally found through their commit, note, or + /// review row. Push pipelines have none of those, so they record the branch + /// here to stay visible to the branch queue. `None` for artifact-backed + /// sessions and for project-level sessions. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub branch_id: Option, } /// Persistent follow-up message waiting to be sent to an existing session. @@ -621,6 +629,7 @@ impl Session { pipeline: None, acp_config_selection: None, acp_title: None, + branch_id: None, } } @@ -644,6 +653,7 @@ impl Session { pipeline: None, acp_config_selection: None, acp_title: None, + branch_id: None, } } @@ -652,6 +662,12 @@ impl Session { self } + /// Link an artifact-less session to its branch so the branch queue sees it. + pub fn with_branch(mut self, branch_id: &str) -> Self { + self.branch_id = Some(branch_id.to_string()); + self + } + pub fn with_agent(mut self, agent_id: &str) -> Self { self.agent_id = Some(agent_id.to_string()); self @@ -1436,6 +1452,13 @@ pub struct PipelineExecution { /// rather than silently downgrading to a base rebase. #[serde(default, skip_serializing_if = "Option::is_none")] pub rebase_target: Option, + /// Whether the push variant force-pushes (`--force-with-lease`). + /// + /// Recorded so a queued push re-derives the same command on dequeue, and so + /// the queue can tell a pending push from a pending force push. Always + /// `false` for non-push pipelines. + #[serde(default, skip_serializing_if = "is_false")] + pub push_force: bool, pub steps: Vec, pub current_step: usize, /// Set when pipeline completes without needing AI. @@ -1472,6 +1495,7 @@ impl PipelineExecution { Self { kind: None, rebase_target: None, + push_force: false, steps: step_statuses, current_step: 0, completed_without_ai: false, @@ -1487,14 +1511,29 @@ impl PipelineExecution { self.rebase_target = Some(target); self } + + pub fn with_push_force(mut self, force: bool) -> Self { + self.push_force = force; + self + } +} + +/// `skip_serializing_if` predicate so non-push pipelines persist no push flag. +fn is_false(value: &bool) -> bool { + !*value } -/// Durable identity for commit-producing command pipelines. +/// Durable identity for command pipelines that the branch queue schedules. +/// +/// `Rebase` and `Squash` produce a commit and are linked to their branch through +/// a pending-commit artifact. `Push` produces no artifact and is linked through +/// `Session::branch_id` instead. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum PipelineKind { Rebase, Squash, + Push, } #[cfg(test)] @@ -1534,6 +1573,36 @@ mod pipeline_tests { assert_eq!(execution.rebase_target, None); } + #[test] + fn pipeline_push_force_is_optional_for_legacy_pipeline_json() { + let execution: PipelineExecution = + serde_json::from_str(r#"{"steps":[],"currentStep":0,"completedWithoutAi":false}"#) + .unwrap(); + + assert!(!execution.push_force); + } + + #[test] + fn pipeline_push_force_round_trips_and_stays_out_of_non_push_json() { + let execution = PipelineExecution::from_steps(&[]) + .with_kind(PipelineKind::Push) + .with_push_force(true); + let json = serde_json::to_string(&execution).unwrap(); + + assert!(json.contains("\"kind\":\"push\"")); + assert!(json.contains("\"pushForce\":true")); + assert!( + serde_json::from_str::(&json) + .unwrap() + .push_force + ); + + let rebase = PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Rebase); + assert!(!serde_json::to_string(&rebase) + .unwrap() + .contains("pushForce")); + } + #[test] fn pipeline_rebase_target_round_trips() { let execution: PipelineExecution = serde_json::from_str( diff --git a/apps/staged/src-tauri/src/store/sessions.rs b/apps/staged/src-tauri/src/store/sessions.rs index d6397631a..9581f7950 100644 --- a/apps/staged/src-tauri/src/store/sessions.rs +++ b/apps/staged/src-tauri/src/store/sessions.rs @@ -19,8 +19,8 @@ impl Store { let acp_config_selection_json = serialize_acp_config_selection(session.acp_config_selection.as_ref())?; conn.execute( - "INSERT INTO sessions (id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14)", + "INSERT INTO sessions (id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title, branch_id) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15)", params![ session.id, session.prompt, @@ -36,6 +36,7 @@ impl Store { pipeline_json, acp_config_selection_json, session.acp_title, + session.branch_id, ], )?; Ok(()) @@ -44,7 +45,7 @@ impl Store { pub fn get_session(&self, id: &str) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); conn.query_row( - "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title + "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title, branch_id FROM sessions WHERE id = ?1", params![id], Self::row_to_session, @@ -223,7 +224,7 @@ impl Store { pub fn get_running_sessions(&self) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); let mut stmt = conn.prepare( - "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title + "SELECT id, prompt, status, working_dir, provider, agent_id, error_message, completion_reason, created_at, updated_at, owner_pid, pipeline, acp_config_selection, acp_title, branch_id FROM sessions WHERE status = 'running'", )?; let sessions = stmt @@ -287,20 +288,22 @@ impl Store { /// Get all queued sessions for a branch, ordered by creation time (oldest first). /// - /// Sessions are linked to branches through artifacts (commits, notes, reviews). - /// This query joins across all three artifact tables to find every queued session - /// belonging to the given branch. + /// Sessions are linked to branches through artifacts (commits, notes, reviews), + /// or directly via `sessions.branch_id` for artifact-less work such as queued + /// push pipelines. This query covers both so a queued push takes its place in + /// the branch queue alongside artifact-backed sessions. pub fn get_queued_sessions_for_branch( &self, branch_id: &str, ) -> Result, StoreError> { let conn = self.conn.lock().unwrap(); let mut stmt = conn.prepare( - "SELECT s.id, s.prompt, s.status, s.working_dir, s.provider, s.agent_id, s.error_message, s.completion_reason, s.created_at, s.updated_at, s.owner_pid, s.pipeline, s.acp_config_selection, s.acp_title + "SELECT s.id, s.prompt, s.status, s.working_dir, s.provider, s.agent_id, s.error_message, s.completion_reason, s.created_at, s.updated_at, s.owner_pid, s.pipeline, s.acp_config_selection, s.acp_title, s.branch_id FROM sessions s WHERE s.status = 'queued' AND ( - EXISTS (SELECT 1 FROM commits c WHERE c.session_id = s.id AND c.branch_id = ?1) + s.branch_id = ?1 + OR EXISTS (SELECT 1 FROM commits c WHERE c.session_id = s.id AND c.branch_id = ?1) OR EXISTS (SELECT 1 FROM notes n WHERE n.session_id = s.id AND n.branch_id = ?1) OR EXISTS (SELECT 1 FROM reviews r WHERE r.session_id = s.id AND r.branch_id = ?1) ) @@ -315,14 +318,17 @@ impl Store { /// Check whether a branch already has a running session. /// /// Auto-reviews (`is_auto = 1`) are excluded because they run in the - /// background and should never block user-initiated sessions. + /// background and should never block user-initiated sessions. Sessions + /// linked through `sessions.branch_id` (running push pipelines) count, so a + /// push in flight blocks new branch work the same way a commit does. pub fn has_running_session_for_branch(&self, branch_id: &str) -> Result { let conn = self.conn.lock().unwrap(); let count: i64 = conn.query_row( "SELECT COUNT(*) FROM sessions s WHERE s.status = 'running' AND ( - EXISTS (SELECT 1 FROM commits c WHERE c.session_id = s.id AND c.branch_id = ?1) + s.branch_id = ?1 + OR EXISTS (SELECT 1 FROM commits c WHERE c.session_id = s.id AND c.branch_id = ?1) OR EXISTS (SELECT 1 FROM notes n WHERE n.session_id = s.id AND n.branch_id = ?1) OR EXISTS (SELECT 1 FROM reviews r WHERE r.session_id = s.id AND r.branch_id = ?1 AND r.is_auto = 0) )", @@ -332,7 +338,8 @@ impl Store { Ok(count > 0) } - /// Resolve the branch that owns a session through its linked artifact. + /// Resolve the branch that owns a session through its linked artifact, or + /// through `sessions.branch_id` for artifact-less branch work (pushes). /// /// Project-note sessions do not belong to a branch and therefore return `None`. /// This assumes all branch-linked artifacts for a session point at the same @@ -345,6 +352,8 @@ impl Store { let conn = self.conn.lock().unwrap(); conn.query_row( "SELECT branch_id FROM ( + SELECT branch_id FROM sessions WHERE id = ?1 AND branch_id IS NOT NULL + UNION ALL SELECT branch_id FROM commits WHERE session_id = ?1 UNION ALL SELECT branch_id FROM notes WHERE session_id = ?1 @@ -382,6 +391,8 @@ impl Store { conn.query_row( "SELECT b.project_id FROM branches b INNER JOIN ( + SELECT branch_id FROM sessions WHERE id = ?1 AND branch_id IS NOT NULL + UNION ALL SELECT branch_id FROM commits WHERE session_id = ?1 UNION ALL SELECT branch_id FROM notes WHERE session_id = ?1 @@ -479,6 +490,7 @@ impl Store { pipeline, acp_config_selection, acp_title: row.get(13)?, + branch_id: row.get(14)?, }) } } diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index 0a973ccfe..0f4b36e11 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -947,6 +947,81 @@ fn test_queued_pipeline_commit_owns_branch_queue_position() { ); } +#[test] +fn test_queued_push_pipeline_is_found_through_session_branch_link() { + let store = Store::in_memory().unwrap(); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + let other_branch = Branch::new(&project.id, "other", "main"); + store.create_branch(&other_branch).unwrap(); + + // No commit/note/review artifact: the branch link is the only way in. + let mut session = Session::new_queued("Push the current branch").with_branch(&branch.id); + session.pipeline = Some( + PipelineExecution::from_steps(&[]) + .with_kind(PipelineKind::Push) + .with_push_force(true), + ); + store.create_session(&session).unwrap(); + + let queued = store.get_queued_sessions_for_branch(&branch.id).unwrap(); + assert_eq!(queued.len(), 1); + assert_eq!(queued[0].id, session.id); + assert_eq!(queued[0].branch_id.as_deref(), Some(branch.id.as_str())); + assert_eq!( + queued[0].pipeline.as_ref().and_then(|p| p.kind.as_ref()), + Some(&PipelineKind::Push) + ); + assert!(queued[0].pipeline.as_ref().unwrap().push_force); + + assert!(store + .get_queued_sessions_for_branch(&other_branch.id) + .unwrap() + .is_empty()); +} + +#[test] +fn test_running_push_pipeline_marks_branch_busy_and_resolves_branch() { + let store = Store::in_memory().unwrap(); + let project = Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let mut session = + Session::new_running("Push the current branch", Path::new("/tmp")).with_branch(&branch.id); + session.pipeline = Some(PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Push)); + store.create_session(&session).unwrap(); + + assert!(store.has_running_session_for_branch(&branch.id).unwrap()); + assert_eq!( + store + .get_branch_id_for_session(&session.id) + .unwrap() + .as_deref(), + Some(branch.id.as_str()) + ); + assert_eq!( + store + .get_project_id_for_session(&session.id) + .unwrap() + .as_deref(), + Some(project.id.as_str()) + ); + // The drain scan resolves schedules from `get_running_sessions`, so that + // query has to carry the branch link too. + let running = store.get_running_sessions().unwrap(); + assert_eq!( + running + .iter() + .find(|running| running.id == session.id) + .and_then(|running| running.branch_id.as_deref()), + Some(branch.id.as_str()) + ); +} + #[test] fn test_mark_session_artifact_started_restamps_empty_note_and_preserves_session_created_at() { let store = Store::in_memory().unwrap(); diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 589555ba1..9bd00d99c 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -3376,7 +3376,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = opt_arg(&args, "provider")?; let force: Option = opt_arg(&args, "force")?; - let session_id = crate::prs::start_push_branch_pipeline_for_branch( + let response = crate::prs::start_or_queue_push_pipeline_for_branch( store, Arc::clone(session_registry), app_handle.clone(), @@ -3385,7 +3385,7 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { let store = get_store(store_mutex)?; diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index c2eff5bab..ee211c03b 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -286,6 +286,33 @@ describe('browser-native command wrappers', () => { ]); }); + it('surfaces the queued-vs-running status returned by push and force push', async () => { + const invokeCommand = vi + .fn() + .mockResolvedValueOnce({ sessionId: 'session-1', sessionStatus: 'running' }) + .mockResolvedValueOnce({ sessionId: 'session-2', sessionStatus: 'queued' }); + vi.doMock('./transport', () => ({ + invokeCommand, + isTauri: true, + })); + + const { pushBranch } = await import('./commands'); + + await expect(pushBranch('branch-1', 'codex', false)).resolves.toEqual({ + sessionId: 'session-1', + sessionStatus: 'running', + }); + await expect(pushBranch('branch-1', 'codex', true)).resolves.toEqual({ + sessionId: 'session-2', + sessionStatus: 'queued', + }); + + expect(invokeCommand.mock.calls).toEqual([ + ['push_branch', { branchId: 'branch-1', provider: 'codex', force: false }], + ['push_branch', { branchId: 'branch-1', provider: 'codex', force: true }], + ]); + }); + it('forwards ACP config selection when resuming a session', async () => { const invokeCommand = vi.fn().mockResolvedValue(undefined); vi.doMock('./transport', () => ({ diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index b3bf0a015..54cce02d1 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -1255,8 +1255,13 @@ export function hasUnpushedCommits(branchId: string): Promise { /** Push a branch to its remote via an agent session. * The agent runs git push and can fix pre-push hook failures. - * Returns the session ID so the frontend can track progress. */ -export function pushBranch(branchId: string, provider?: string, force?: boolean): Promise { + * Queues behind in-flight branch sessions, so the response reports whether the + * returned session is pushing now or waiting on the branch queue. */ +export function pushBranch( + branchId: string, + provider?: string, + force?: boolean +): Promise { return invokeCommand('push_branch', { branchId, provider: provider ?? null, diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index 0b22df613..6de1374f7 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -618,17 +618,18 @@ let branchIdentityWarning = $derived(gitIdentityWarning(timeline?.gitState)); let gitUnsafeActionsDisabled = $derived(!!branchIdentityWarning); /** - * Gate for Rebase/Squash. In-flight sessions are not a reason to disable: - * both queue on the branch session queue and drain when the branch frees up. - * Only identity problems (detached HEAD, wrong branch) make them unsafe. + * Gate for the queueable git actions (Rebase/Squash, push, force-push). + * In-flight sessions are not a reason to disable: they all queue on the branch + * session queue and drain when the branch frees up. Only identity problems + * (detached HEAD, wrong branch) make them unsafe. */ let branchCommandDisabledReason = $derived( branchIdentityWarning ?? (commandPipelinePending ? 'Command in progress' : null) ); /** - * Gate for git actions that still execute immediately (push, force-push, - * reset to origin). Those have no queue support yet, so a busy branch has to - * keep blocking them. + * Gate for reset to origin, which still executes immediately: it is validated + * against a point-in-time preview of what would be discarded, so a busy branch + * has to keep blocking it. */ let immediateGitActionDisabledReason = $derived( branchCommandDisabledReason ?? (branchSessionBusy ? 'Session in progress' : null) @@ -1077,24 +1078,49 @@ // a 5s polling fallback in BranchCardPrButton. let storePushState = $derived(pushStateStore.getPushState(branch.id)); let pushingOrigin = $derived(storePushState?.state === 'pushing'); + /** Push is waiting on the branch session queue rather than running. */ + let pushQueuedOrigin = $derived(storePushState?.state === 'queued'); let pushSessionId = $derived(storePushState?.sessionId ?? null); let forcePushingOrigin = $derived(pushingOrigin); let forcePushSessionId = $derived(pushSessionId); async function handlePushOrigin() { - if (pushingOrigin || commandPipelinePending || branchSessionBusy) return; + // Deliberately not gated on `branchSessionBusy`: the backend decides + // between pushing now and queueing behind in-flight branch work, and it + // dedupes repeat clicks of the same action. + if (pushingOrigin || pushQueuedOrigin || commandPipelinePending) return; const agents = isRemote ? REMOTE_AGENTS : agentState.providers; const provider = getPreferredAgent(agents) ?? undefined; pushStateStore.setPushing(branch.id, '__pending__'); try { - const sessionId = await commands.pushBranch(branch.id, provider, false); - pushStateStore.setPushing(branch.id, sessionId); + const response = await commands.pushBranch(branch.id, provider, false); + pushStateStore.setPushLaunch(branch.id, response); } catch (e) { pushStateStore.setPushError(branch.id, e instanceof Error ? e.message : String(e)); notifyError('Push failed', e); } } + /** + * Drop a push that is still waiting on the branch queue. + * + * The store entry is cleared here rather than by the completion listener: a + * session that never ran isn't in the session registry, so its cancellation + * event carries no push session type to match on. + */ + async function cancelQueuedPush() { + const sessionId = pushSessionId; + if (!pushQueuedOrigin || !sessionId || sessionId === '__pending__') return; + pushStateStore.clearPushState(branch.id); + try { + await commands.cancelSession(sessionId); + commands.invalidateBranchTimeline(branch.id); + await loadTimeline(); + } catch (e) { + notifyError('Could not cancel queued push', e); + } + } + function openPushSession() { if (pushSessionId && pushSessionId !== '__pending__') { sessionMgr.openSessionId = pushSessionId; @@ -1108,7 +1134,9 @@ } async function confirmForcePush() { - if (forcePushingOrigin || commandPipelinePending || branchSessionBusy) { + // Like `handlePushOrigin`, not gated on `branchSessionBusy` — the backend + // queues the force push behind in-flight branch work. + if (forcePushingOrigin || pushQueuedOrigin || commandPipelinePending) { // Another operation is in progress — keep the dialog open so the user // understands why the action didn't proceed. return; @@ -1118,8 +1146,8 @@ const provider = getPreferredAgent(agents) ?? undefined; pushStateStore.setPushing(branch.id, '__pending__'); try { - const sessionId = await commands.pushBranch(branch.id, provider, true); - pushStateStore.setPushing(branch.id, sessionId); + const response = await commands.pushBranch(branch.id, provider, true); + pushStateStore.setPushLaunch(branch.id, response); } catch (e) { pushStateStore.setPushError(branch.id, e instanceof Error ? e.message : String(e)); notifyError('Force push failed', e); @@ -1848,8 +1876,11 @@ onOpenForcePushSession={forcePushSessionId && forcePushSessionId !== '__pending__' ? openForcePushSession : undefined} + onCancelQueuedPush={cancelQueuedPush} {forcePushingOrigin} + {pushQueuedOrigin} {immediateGitActionDisabledReason} + queueableGitActionDisabledReason={branchCommandDisabledReason} onViewWorktreeDiff={isLocal ? () => openDiffDetail({ diff --git a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte index a08a3d2fe..74708cabb 100644 --- a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte @@ -11,6 +11,7 @@ import GitPullRequestDraft from '@lucide/svelte/icons/git-pull-request-draft'; import GitMerge from '@lucide/svelte/icons/git-merge'; import AlertCircle from '@lucide/svelte/icons/alert-circle'; + import Clock from '@lucide/svelte/icons/clock'; import Spinner from '../../shared/Spinner.svelte'; import * as AlertDialog from '$lib/components/ui/alert-dialog'; import { Button } from '$lib/components/ui/button'; @@ -229,15 +230,22 @@ return () => clearInterval(interval); }); - // Fallback polling for push session + // Fallback polling for push session. Queued pushes are polled too: if the + // branch queue drains one while the "running" event is missed, this is what + // moves the button off "Queued". $effect(() => { - if (pushState !== 'pushing' || !pushSessionId) return; + if ((pushState !== 'pushing' && pushState !== 'queued') || !pushSessionId) return; const sid = pushSessionId; const interval = setInterval(async () => { try { const session = await commands.getSession(sid); - if (session && session.status !== 'running') { + if (session?.status === 'queued') return; + if (session?.status === 'running') { + pushStateStore.markQueuedPushStarted(branch.id, sid); + return; + } + if (session) { handlePushSessionComplete(session.status, session); } } catch (err) { @@ -337,7 +345,7 @@ function getPrStatusIndicator(): 'success' | 'warning' | 'error' | 'neutral' | 'pending' | null { if (prState === 'creating') return null; - if (pushState === 'pushing') return null; + if (pushState === 'pushing' || pushState === 'queued') return null; if (pushState === 'error' || prState === 'error') return 'error'; if (!branch.prNumber) return null; @@ -363,6 +371,7 @@ let prStatusIndicator = $derived(getPrStatusIndicator()); function getPrButtonActionTitle(): string { + if (pushState === 'queued') return 'Push queued behind branch work — click to cancel'; if (pushState === 'pushing') return 'Pushing… (click to view)'; if (pushState === 'error') return 'Push failed — click for details'; if (prState === 'created' && hasUnpushed) { @@ -478,7 +487,7 @@ // ========================================================================= function handlePush(force = false) { - if (pushState === 'pushing') return; + if (pushState === 'pushing' || pushState === 'queued') return; pushStateStore.setPushing(branch.id, '__pending__'); @@ -487,17 +496,35 @@ commands .pushBranch(branch.id, provider, force) - .then((sessionId) => { - // Session is already registered by the global listener via the - // backend's "running" event — just update the local store with the - // real session ID so the fallback poller can track it. - pushStateStore.setPushing(branch.id, sessionId); + .then((response) => { + // A running session is already registered by the global listener via + // the backend's "running" event — this just records the real session ID + // (and whether the push is waiting on the branch queue) so the fallback + // poller can track it. + pushStateStore.setPushLaunch(branch.id, response); }) .catch((e) => { pushStateStore.setPushError(branch.id, e instanceof Error ? e.message : String(e)); }); } + /** + * Drop a push that is still waiting on the branch queue. + * + * The store entry is cleared here rather than in the completion handler: a + * session that never ran isn't in the session registry, so the cancellation + * event carries no push session type for `sessionStatusListener` to match. + */ + function cancelQueuedPush() { + const sid = pushSessionId; + if (pushState !== 'queued' || !sid || sid === '__pending__') return; + + pushStateStore.clearPushState(branch.id); + commands.cancelSession(sid).catch((e) => { + pushStateStore.setPushError(branch.id, e instanceof Error ? e.message : String(e)); + }); + } + let pushCompletionInFlight = false; async function classifyPushSessionOutcome( @@ -602,6 +629,10 @@ } return; } + if (pushState === 'queued') { + cancelQueuedPush(); + return; + } if (pushState === 'pushing' && pushSessionId) { onOpenSession?.(pushSessionId); return; @@ -676,12 +707,15 @@ (prState === 'error' || pushState === 'error') && 'border-destructive text-destructive hover:bg-[var(--ui-danger-bg)] hover:text-destructive', pushState === 'pushing' && 'cursor-default border-[var(--border-muted)]', + pushState === 'queued' && 'border-[var(--border-muted)]', prState === 'created' && prStatusState === 'MERGED' && '[&_svg]:text-[var(--status-added)]', ]} onclick={handlePrButtonClick} disabled={showPushErrorDialog || showForcePushDialog || showPrErrorDialog} > - {#if pushState === 'pushing'} + {#if pushState === 'queued'} + + {:else if pushState === 'pushing'} {:else if pushState === 'error'} @@ -699,7 +733,9 @@ {/if} - {#if pushState === 'pushing'} + {#if pushState === 'queued'} + Push queued + {:else if pushState === 'pushing'} Pushing… {:else if pushState === 'error'} Push failed diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 9e36f6441..db53168d1 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -95,14 +95,26 @@ onOpenForcePushSession?: () => void; forcePushingOrigin?: boolean; onOpenPushSession?: () => void; + /** Cancel a push/force-push that is still waiting on the branch queue. */ + onCancelQueuedPush?: () => void; + /** Push is queued behind in-flight branch work. */ + pushQueuedOrigin?: boolean; /** - * Why push, force-push, and reset-to-origin can't run right now. + * Why reset-to-origin can't run right now. * - * These git actions still execute immediately rather than queueing, so - * unlike Rebase/Squash they stay disabled while the branch has sessions in - * flight. + * Reset still executes immediately rather than queueing — it is validated + * against a point-in-time preview — so unlike Rebase/Squash/Push it stays + * disabled while the branch has sessions in flight. */ immediateGitActionDisabledReason?: string | null; + /** + * Why push and force-push can't run right now. + * + * These queue on the branch session queue like Rebase/Squash, so in-flight + * sessions are not a reason to disable them — only branch identity problems + * are. + */ + queueableGitActionDisabledReason?: string | null; onViewWorktreeDiff?: () => void; onCommitWorktreeChanges?: () => void; onDiscardWorktreeChanges?: () => void; @@ -158,7 +170,10 @@ onOpenForcePushSession, forcePushingOrigin = false, onOpenPushSession, + onCancelQueuedPush, + pushQueuedOrigin = false, immediateGitActionDisabledReason, + queueableGitActionDisabledReason, onViewWorktreeDiff, onCommitWorktreeChanges, onDiscardWorktreeChanges, @@ -251,6 +266,8 @@ resetToOriginDisabledReason?: string; resettingToOrigin?: boolean; pushing?: boolean; + pushQueued?: boolean; + forcePushQueued?: boolean; onViewDiff?: () => void; onCommitChanges?: () => void; commitChangesDisabledReason?: string; @@ -393,19 +410,28 @@ 2 ); const summary = `is ${plural(state.upstream.ahead, 'commit')} behind`; - const disabledReason = pushingOrigin - ? undefined // button is clickable during push (opens session) - : (immediateGitActionDisabledReason ?? undefined); + const disabledReason = + pushingOrigin || pushQueuedOrigin + ? undefined // button stays clickable: it opens the session, or cancels the queued push + : (queueableGitActionDisabledReason ?? undefined); rows.push({ key: 'git-local-ahead', type: 'git-push', title: `origin ${summary}`, titleHtml: `origin ${escapeHtml(summary)}`, + meta: pushQueuedOrigin ? 'Push queued' : undefined, timestamp: placement.timestamp, order: placement.order, - onPush: pushingOrigin ? onOpenPushSession : disabledReason ? undefined : onPushOrigin, + onPush: pushQueuedOrigin + ? onCancelQueuedPush + : pushingOrigin + ? onOpenPushSession + : disabledReason + ? undefined + : onPushOrigin, pushDisabledReason: disabledReason, pushing: pushingOrigin, + pushQueued: pushQueuedOrigin, }); } break; @@ -441,29 +467,36 @@ const divergedTitleHtml = `origin diverges here and has ${escapeHtml(plural(behindCount, 'more commit'))}${escapeHtml(baseSummary)}`; const resetToOriginReason = resettingToOrigin ? 'Resetting...' - : forcePushingOrigin - ? 'Push in progress' - : onResetToOrigin - ? (immediateGitActionDisabledReason ?? undefined) - : undefined; + : pushQueuedOrigin + ? 'Push queued' + : forcePushingOrigin + ? 'Push in progress' + : onResetToOrigin + ? (immediateGitActionDisabledReason ?? undefined) + : undefined; rows.push({ key: 'git-diverged', type: 'git-merge-warning', title: divergedTitle, titleHtml: divergedTitleHtml, + meta: pushQueuedOrigin ? 'Push queued' : undefined, timestamp: placement.timestamp, order: placement.order, - onForcePush: forcePushingOrigin - ? onOpenForcePushSession - : immediateGitActionDisabledReason + onForcePush: pushQueuedOrigin + ? onCancelQueuedPush + : forcePushingOrigin + ? onOpenForcePushSession + : queueableGitActionDisabledReason + ? undefined + : onForcePush, + forcePushDisabledReason: + forcePushingOrigin || pushQueuedOrigin ? undefined - : onForcePush, - forcePushDisabledReason: forcePushingOrigin - ? undefined - : onForcePush - ? (immediateGitActionDisabledReason ?? undefined) - : undefined, + : onForcePush + ? (queueableGitActionDisabledReason ?? undefined) + : undefined, forcePushing: forcePushingOrigin, + forcePushQueued: pushQueuedOrigin, onResetToOrigin: resetToOriginReason ? undefined : onResetToOrigin, resetToOriginDisabledReason: resetToOriginReason, resettingToOrigin, @@ -941,6 +974,8 @@ resetToOriginDisabledReason={item.resetToOriginDisabledReason} resettingToOrigin={item.resettingToOrigin} pushing={item.pushing} + pushQueued={item.pushQueued} + forcePushQueued={item.forcePushQueued} onViewDiffClick={item.onViewDiff} onCommitChangesClick={item.onCommitChanges} commitChangesDisabledReason={item.commitChangesDisabledReason} @@ -1028,6 +1063,8 @@ resetToOriginDisabledReason={item.resetToOriginDisabledReason} resettingToOrigin={item.resettingToOrigin} pushing={item.pushing} + pushQueued={item.pushQueued} + forcePushQueued={item.forcePushQueued} onViewDiffClick={item.onViewDiff} onCommitChangesClick={item.onCommitChanges} commitChangesDisabledReason={item.commitChangesDisabledReason} diff --git a/apps/staged/src/lib/features/timeline/TimelineRow.svelte b/apps/staged/src/lib/features/timeline/TimelineRow.svelte index 49ba87681..a5224a154 100644 --- a/apps/staged/src/lib/features/timeline/TimelineRow.svelte +++ b/apps/staged/src/lib/features/timeline/TimelineRow.svelte @@ -83,6 +83,10 @@ resetToOriginDisabledReason?: string; resettingToOrigin?: boolean; pushing?: boolean; + /** Push is waiting on the branch queue, so its button cancels instead. */ + pushQueued?: boolean; + /** Force push is waiting on the branch queue, so its button cancels instead. */ + forcePushQueued?: boolean; onViewDiffClick?: () => void; onCommitChangesClick?: () => void; commitChangesDisabledReason?: string; @@ -124,6 +128,8 @@ resetToOriginDisabledReason, resettingToOrigin = false, pushing = false, + pushQueued = false, + forcePushQueued = false, onViewDiffClick, onCommitChangesClick, commitChangesDisabledReason, @@ -176,10 +182,17 @@ let isClickable = $derived(!!onItemClick && !isPending && !isFailed); let hasSession = $derived(!!sessionId && !deleting); let pullTitle = $derived(pullDisabledReason ?? 'Pull'); - let pushTitle = $derived(pushDisabledReason ?? (pushing ? 'View push session' : 'Push')); + let pushTitle = $derived( + pushDisabledReason ?? + (pushQueued ? 'Cancel queued push' : pushing ? 'View push session' : 'Push') + ); let forcePushTitle = $derived( forcePushDisabledReason ?? - (forcePushing ? 'View push session' : 'Force push local branch to origin') + (forcePushQueued + ? 'Cancel queued push' + : forcePushing + ? 'View push session' + : 'Force push local branch to origin') ); let resetToOriginTitle = $derived( resetToOriginDisabledReason ?? @@ -458,7 +471,7 @@ aria-label={pushTitle} class="h-[22px] rounded-md border-[var(--border-subtle)] bg-transparent text-[var(--text-muted)] shadow-none hover:border-[var(--border-muted)] hover:bg-[var(--bg-hover)] hover:text-foreground" > - {pushing ? 'Pushing\u2026' : 'Push'} + {pushQueued ? 'Cancel' : pushing ? 'Pushing\u2026' : 'Push'} {/if} @@ -493,12 +506,12 @@ aria-label={forcePushTitle} class={[ 'h-[22px] rounded-md bg-transparent shadow-none', - forcePushing + forcePushing || forcePushQueued ? 'border-[var(--border-subtle)] text-[var(--text-muted)] hover:border-[var(--border-muted)] hover:bg-[var(--bg-hover)] hover:text-foreground' : 'border-[var(--ui-danger-bg)] font-medium text-[var(--ui-danger)] hover:border-[var(--ui-danger)] hover:bg-[var(--ui-danger-bg)] hover:text-[var(--ui-danger)]', ]} > - {forcePushing ? 'Pushing\u2026' : 'Force Push'} + {forcePushQueued ? 'Cancel' : forcePushing ? 'Pushing\u2026' : 'Force Push'} {/if} diff --git a/apps/staged/src/lib/listeners/sessionStatusListener.ts b/apps/staged/src/lib/listeners/sessionStatusListener.ts index 350de1612..46a38e192 100644 --- a/apps/staged/src/lib/listeners/sessionStatusListener.ts +++ b/apps/staged/src/lib/listeners/sessionStatusListener.ts @@ -46,6 +46,11 @@ export function listenForSessionStatus(): UnlistenFn { eventBranchId ); projectStateStore.addRunningSession(eventProjectId, sessionId); + // A push that was queued behind other branch work starts running when the + // branch queue drains it; this event is the only signal of that. + if (sessionType === 'push' && eventBranchId) { + pushStateStore.markQueuedPushStarted(eventBranchId, sessionId); + } return; } diff --git a/apps/staged/src/lib/stores/pushState.svelte.ts b/apps/staged/src/lib/stores/pushState.svelte.ts index 10dd63176..130f4833f 100644 --- a/apps/staged/src/lib/stores/pushState.svelte.ts +++ b/apps/staged/src/lib/stores/pushState.svelte.ts @@ -14,9 +14,14 @@ * Session lookups are delegated to the unified sessionRegistry */ +import type { BranchPipelineResponse } from '../types'; import { sessionRegistry } from './sessionRegistry.svelte'; -export type PushState = 'idle' | 'pushing' | 'error' | 'done'; +/** + * `queued` means the backend put the push on the branch session queue because + * the branch was busy; it becomes `pushing` when the queue drains it. + */ +export type PushState = 'idle' | 'queued' | 'pushing' | 'error' | 'done'; interface BranchPushState { state: PushState; @@ -68,6 +73,47 @@ class PushStateStore { this.version++; } + setPushQueued(branchId: string, sessionId: string): void { + this.maybeCleanup(); + this.states.set(branchId, { + state: 'queued', + sessionId, + error: null, + rejectedNonFastForward: false, + timestamp: Date.now(), + }); + this.version++; + } + + /** + * Record the backend's queued-vs-running verdict for a push request. + * + * The backend owns that decision (it holds the branch launch lock), so both + * push entry points route their response through here rather than predicting + * it from the timeline. + */ + setPushLaunch(branchId: string, response: BranchPipelineResponse): void { + if (response.sessionStatus === 'queued') { + this.setPushQueued(branchId, response.sessionId); + } else { + this.setPushing(branchId, response.sessionId); + } + } + + /** + * Flip a queued push to `pushing` once the branch queue drains it. + * + * Guarded on the session ID so an unrelated push event can't revive a stale + * queued entry (e.g. one the user cancelled and re-requested). + */ + markQueuedPushStarted(branchId: string, sessionId: string): void { + const existing = this.states.get(branchId); + if (existing?.state !== 'queued' || existing.sessionId !== sessionId) { + return; + } + this.setPushing(branchId, sessionId); + } + setPushDone(branchId: string): void { this.maybeCleanup(); this.states.set(branchId, { diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index 796b60ee9..eb3dca95d 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -409,7 +409,7 @@ export interface QueuedSessionMessage { export type StepStatus = 'pending' | 'running' | 'succeeded' | 'failed' | 'skipped'; export type StepType = 'command' | 'ai_handoff'; -export type PipelineKind = 'rebase' | 'squash'; +export type PipelineKind = 'rebase' | 'squash' | 'push'; export interface PipelineStepStatus { label: string; @@ -423,6 +423,8 @@ export interface PipelineStepStatus { export interface PipelineExecution { kind?: PipelineKind | null; + /** Whether a `push` pipeline force-pushes. Absent for the other kinds. */ + pushForce?: boolean; steps: PipelineStepStatus[]; currentStep: number; completedWithoutAi: boolean; From 2a0e8bfac3ccc4a931f5c954521fd20fc2757e9d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 16:38:56 +1000 Subject: [PATCH 3/9] feat(staged): queue pull behind running branch sessions Phase 3 of the branch git-action queue: pull now follows the same start-or-queue path as rebase/squash (Phase 1) and push/force-push (Phase 2). When the branch is idle the pull still runs as an instant direct git operation; when the branch has work in flight it enqueues a `PipelineKind::Pull` session that the branch drainer runs headlessly in FIFO order. Backend: - Add `PipelineKind::Pull` and route it through `PipelineBranchLink::Branch` (no commit artifact), so it schedules as `BranchSessionScheduleKind::GitPipeline`. - Replace the `pull_branch_ff_only` command with `pull_or_queue_branch` in `prs.rs`, which takes the branch launch lock, dedupes against an existing queued pull, and otherwise falls through to `pull_branch_ff_only_impl`. It returns the queued session id, or null when the pull ran immediately. - Generalize `start_queued_git_pipeline_for_branch` over push/pull and add `build_pull_pipeline_steps` (fetch, then `merge --ff-only origin/`, both aborting rather than handing off to an agent). - Surface drained-pull failures: an aborted pull now ends the session in `error` with the failing step's label and output, while an aborted push still completes so the UI can offer force push. Frontend: - Add a `pullState` store mirroring `pushState`, badge the origin-ahead timeline row with "Pull queued", and turn its button into Cancel while queued. - Loosen the dirty-worktree gate: pull moves to `queueableGitActionDisabledReason`, so agent-transient dirt no longer disables it while the branch is busy. - Toast queued-pull failures from the session-status listener, and teach the session registry / labels about the `pull` session type. Tests: pull coverage in the git-pipeline scheduling tests, new `aborted_pipeline_error` unit tests (pull with and without step output, push unaffected), and a `commands.test.ts` case distinguishing an immediate pull from a queued one. Data-model note for review: `PipelineKind::Pull` is a new persisted enum variant. The queued badge lives in frontend state only, matching Phase 2, so a badge is lost across an app restart (re-clicking Pull hits the backend dedupe and restores it). Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/lib.rs | 2 +- apps/staged/src-tauri/src/prs.rs | 295 ++++++++++++++++-- apps/staged/src-tauri/src/session_commands.rs | 97 +++--- apps/staged/src-tauri/src/session_runner.rs | 170 +++++++++- apps/staged/src-tauri/src/store/models.rs | 5 +- apps/staged/src-tauri/src/timeline.rs | 19 +- apps/staged/src-tauri/src/web_server.rs | 12 +- apps/staged/src/lib/commands.test.ts | 19 ++ apps/staged/src/lib/commands.ts | 7 +- .../lib/features/branches/BranchCard.svelte | 50 ++- .../features/timeline/BranchTimeline.svelte | 41 ++- .../lib/features/timeline/TimelineRow.svelte | 7 +- .../lib/listeners/sessionStatusListener.ts | 47 ++- apps/staged/src/lib/shared/utils.ts | 10 +- .../staged/src/lib/stores/pullState.svelte.ts | 93 ++++++ .../src/lib/stores/sessionRegistry.svelte.ts | 4 +- apps/staged/src/lib/types.ts | 2 +- 17 files changed, 755 insertions(+), 125 deletions(-) create mode 100644 apps/staged/src/lib/stores/pullState.svelte.ts diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 9f9305c04..ec2c14e27 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -2269,7 +2269,6 @@ pub fn run() { timeline::get_branch_timeline, timeline::refresh_branch_git_state, timeline::list_parent_branch_commits, - timeline::pull_branch_ff_only, timeline::reset_branch_to_remote, // Notes note_commands::create_note, @@ -2311,6 +2310,7 @@ pub fn run() { prs::refresh_all_pr_statuses, prs::has_unpushed_commits, prs::push_branch, + prs::pull_or_queue_branch, prs::rebase_branch, prs::squash_commits, prs::clear_branch_pr_status, diff --git a/apps/staged/src-tauri/src/prs.rs b/apps/staged/src-tauri/src/prs.rs index 2cabd2ad5..ac549f91f 100644 --- a/apps/staged/src-tauri/src/prs.rs +++ b/apps/staged/src-tauri/src/prs.rs @@ -237,10 +237,13 @@ fn rebase_ref_for_target(branch: &store::Branch, target: Option<&str>) -> String const PUSH_PROMPT: &str = "Push the current branch to the remote with a normal push. If the push fails for a recoverable reason, diagnose and fix it, then retry with a normal push. Do not force push."; const FORCE_PUSH_PROMPT: &str = "Force push the current branch to the remote"; +/// A pull never hands off to an agent (every step aborts), so this is purely the +/// session's label. +const PULL_PROMPT: &str = "Pull from origin"; /// Prompt for a pipeline session, which doubles as its timeline label. /// -/// `push_force` only matters for [`PipelineKind::Push`]; the commit kinds ignore +/// `push_force` only matters for [`PipelineKind::Push`]; the other kinds ignore /// it. Queued and running paths share this so a session's label doesn't change /// when it is drained. fn pipeline_prompt(kind: &PipelineKind, push_force: bool) -> &'static str { @@ -249,6 +252,7 @@ fn pipeline_prompt(kind: &PipelineKind, push_force: bool) -> &'static str { PipelineKind::Squash => "Squash commits", PipelineKind::Push if push_force => FORCE_PUSH_PROMPT, PipelineKind::Push => PUSH_PROMPT, + PipelineKind::Pull => PULL_PROMPT, } } @@ -323,10 +327,10 @@ fn git_push_with_fallback(args: &str) -> String { /// diverged from `origin/{branch}`). Only the rebase variant consults this /// value; squash always operates against the base branch. /// -/// Errors for [`PipelineKind::Push`], which produces no commit and belongs to -/// the git pipeline path — the only caller that reads the kind from the database -/// checks it first, so this guards against a misrouted queued session rather -/// than a reachable input. +/// Errors for [`PipelineKind::Push`] and [`PipelineKind::Pull`], which produce no +/// commit and belong to the git pipeline path — the only caller that reads the +/// kind from the database checks it first, so this guards against a misrouted +/// queued session rather than a reachable input. fn build_commit_pipeline_steps( kind: &PipelineKind, base_branch: &str, @@ -419,8 +423,8 @@ Here is the context from the prior steps: .to_string(), }, ], - PipelineKind::Push => { - return Err("Push is not a commit pipeline".to_string()); + PipelineKind::Push | PipelineKind::Pull => { + return Err(format!("{kind:?} is not a commit pipeline")); } }; @@ -465,6 +469,30 @@ fn build_push_pipeline_steps(branch_name: &str, force: bool) -> Vec Vec { + vec![ + PipelineStep::Command { + label: format!("Fetch origin/{branch_name}"), + command: git_fetch_with_fallback(branch_name), + on_failure: FailureStrategy::Abort { marker: None }, + }, + PipelineStep::Command { + label: format!("Fast-forward to origin/{branch_name}"), + command: format!("git merge --ff-only origin/{branch_name}"), + on_failure: FailureStrategy::Abort { marker: None }, + }, + ] +} + #[allow(clippy::too_many_arguments)] async fn start_running_commit_pipeline_for_branch( ctx: BranchPipelineContext, @@ -813,11 +841,13 @@ fn queue_push_pipeline_if_branch_busy( Ok(Some(session.id)) } -/// Start a queued push pipeline that reached the front of the branch queue. +/// Start a queued push or pull pipeline that reached the front of the branch +/// queue. /// -/// Steps are rebuilt from the persisted `push_force` flag rather than replayed -/// from the queued pipeline, matching [`start_queued_commit_pipeline_for_branch`] -/// so a branch renamed while the push waited still pushes the right ref. +/// Steps are rebuilt from the branch's current name and the persisted +/// `push_force` flag rather than replayed from the queued pipeline, matching +/// [`start_queued_commit_pipeline_for_branch`] so a branch renamed while the work +/// waited still acts on the right ref. pub(crate) async fn start_queued_git_pipeline_for_branch( store: Arc, registry: Arc, @@ -834,16 +864,22 @@ pub(crate) async fn start_queued_git_pipeline_for_branch( .kind .clone() .ok_or_else(|| format!("Queued session {} has no pipeline kind", session.id))?; - if kind != PipelineKind::Push { - return Err(format!( - "Queued git pipeline session {} has non-git kind {kind:?}", - session.id - )); - } let force = queued_pipeline.push_force; let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; - let steps = build_push_pipeline_steps(&ctx.branch.branch_name, force); + let (steps, session_type) = match kind { + PipelineKind::Push => ( + build_push_pipeline_steps(&ctx.branch.branch_name, force), + "push", + ), + PipelineKind::Pull => (build_pull_pipeline_steps(&ctx.branch.branch_name), "pull"), + PipelineKind::Rebase | PipelineKind::Squash => { + return Err(format!( + "Queued git pipeline session {} has non-git kind {kind:?}", + session.id + )); + } + }; let prompt = pipeline_prompt(&kind, force); let pipeline = PipelineExecution::from_steps(&steps) .with_kind(kind) @@ -857,8 +893,9 @@ pub(crate) async fn start_queued_git_pipeline_for_branch( return Ok(false); } - // No `mark_session_artifact_started` call: a push has no queued artifact stub - // whose timestamp needs restamping when the work actually starts. + // No `mark_session_artifact_started` call: a git pipeline has no queued + // artifact stub whose timestamp needs restamping when the work actually + // starts. store .prepare_queued_session(&session.id, &ctx.working_dir.to_string_lossy(), prompt) .map_err(|e| e.to_string())?; @@ -869,7 +906,13 @@ pub(crate) async fn start_queued_git_pipeline_for_branch( let branch_id = ctx.branch.id.clone(); let project_id = ctx.branch.project_id.clone(); - session_runner::emit_session_running(&app_handle, &session.id, &branch_id, &project_id, "push"); + session_runner::emit_session_running( + &app_handle, + &session.id, + &branch_id, + &project_id, + session_type, + ); session_runner::start_pipeline_session( session_runner::PipelineConfig { @@ -893,6 +936,89 @@ pub(crate) async fn start_queued_git_pipeline_for_branch( Ok(true) } +/// Queue a fast-forward pull when the branch has work in flight. +/// +/// Returns the queued session id — either a freshly created one, or the queued +/// pull that already covers this request — and `None` when the branch is idle so +/// the caller should pull immediately. +/// +/// Mirrors [`queue_push_pipeline_if_branch_busy`], including running the busy +/// check, the dedupe scan, and the insert under the branch launch lock. A pull has +/// no variants, so the dedupe keys on the kind alone. +/// +/// No provider is recorded: every pull step aborts on failure, so the pipeline +/// never hands off to an agent. +fn queue_pull_pipeline_if_branch_busy( + store: &Arc, + branch_id: &str, +) -> Result, String> { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); + let _guard = launch_lock.lock().unwrap(); + + if !branch_has_work_in_flight(store, branch_id)? { + return Ok(None); + } + + if let Some(existing) = find_queued_pipeline(store, branch_id, |pipeline| { + pipeline.kind.as_ref() == Some(&PipelineKind::Pull) + })? { + return Ok(Some(existing)); + } + + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + + let steps = build_pull_pipeline_steps(&branch.branch_name); + let pipeline = PipelineExecution::from_steps(&steps).with_kind(PipelineKind::Pull); + // Like a queued push, this session carries no artifact — `branch_id` is what + // keeps it on the branch queue. + let mut session = store::Session::new_queued(PULL_PROMPT).with_branch(branch_id); + session.pipeline = Some(pipeline); + store.create_session(&session).map_err(|e| e.to_string())?; + + Ok(Some(session.id)) +} + +/// Fast-forward the branch to origin now, or queue the pull behind in-flight +/// branch work. +/// +/// An idle branch pulls directly (no session row for what is usually an instant +/// operation), which is why this returns an `Option` rather than the +/// [`BranchPipelineResponse`] the pipeline-only actions use: `None` means the +/// pull already happened, `Some(session_id)` means it is waiting its turn on the +/// branch queue. +pub(crate) async fn pull_or_queue_branch_for_branch( + store: Arc, + branch_id: String, +) -> Result, String> { + if let Some(session_id) = queue_pull_pipeline_if_branch_busy(&store, &branch_id)? { + return Ok(Some(session_id)); + } + + tauri::async_runtime::spawn_blocking(move || { + crate::timeline::pull_branch_ff_only_impl(&store, &branch_id) + }) + .await + .map_err(|e| format!("Pull task failed: {e}"))??; + + Ok(None) +} + +/// Pull origin's new commits into the branch. +/// +/// Returns the queued session id when the pull had to join the branch queue, and +/// `None` when it ran immediately. +#[tauri::command(rename_all = "camelCase")] +pub async fn pull_or_queue_branch( + store: tauri::State<'_, Mutex>>>, + branch_id: String, +) -> Result, String> { + let store = get_store(&store)?; + pull_or_queue_branch_for_branch(store, branch_id).await +} + fn create_pr_handoff_prompt( pr_type: &str, base_branch: &str, @@ -1982,10 +2108,131 @@ mod tests { } #[test] - fn commit_pipeline_steps_reject_the_push_kind() { - let err = build_commit_pipeline_steps(&PipelineKind::Push, "main", "main").unwrap_err(); + fn commit_pipeline_steps_reject_the_git_pipeline_kinds() { + let push_err = + build_commit_pipeline_steps(&PipelineKind::Push, "main", "main").unwrap_err(); + let pull_err = + build_commit_pipeline_steps(&PipelineKind::Pull, "main", "main").unwrap_err(); + + assert_eq!(push_err, "Push is not a commit pipeline"); + assert_eq!(pull_err, "Pull is not a commit pipeline"); + } + + fn queue_pull(store: &Arc, branch_id: &str) -> Option { + queue_pull_pipeline_if_branch_busy(store, branch_id).unwrap() + } - assert_eq!(err, "Push is not a commit pipeline"); + #[test] + fn idle_branch_pulls_immediately_instead_of_queueing() { + let (store, branch) = setup_branch_store(); + + assert_eq!(queue_pull(&store, &branch.id), None); + assert!(store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .is_empty()); + } + + #[test] + fn busy_branch_queues_pull_with_branch_link_and_no_artifact() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let session_id = + queue_pull(&store, &branch.id).expect("pull should queue behind the running note"); + + let session = store.get_session(&session_id).unwrap().unwrap(); + assert_eq!(session.status, store::SessionStatus::Queued); + assert_eq!(session.prompt, PULL_PROMPT); + // Like a queued push, the branch link is what keeps an artifact-less + // pull on the queue. + assert_eq!(session.branch_id.as_deref(), Some(branch.id.as_str())); + assert_eq!( + session.pipeline.unwrap().kind, + Some(store::PipelineKind::Pull) + ); + assert!(store + .list_commits_for_branch(&branch.id) + .unwrap() + .is_empty()); + } + + #[test] + fn repeated_pull_click_reuses_the_already_queued_pull() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let first = queue_pull(&store, &branch.id).unwrap(); + let second = queue_pull(&store, &branch.id).unwrap(); + + assert_eq!(first, second); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 1 + ); + } + + #[test] + fn pull_and_push_queue_separately() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + + let pull = queue_pull(&store, &branch.id).unwrap(); + let push = queue_push(&store, &branch.id, false).unwrap(); + + assert_ne!(pull, push); + assert_eq!( + store + .get_queued_sessions_for_branch(&branch.id) + .unwrap() + .len(), + 2 + ); + } + + #[test] + fn queued_pull_alone_keeps_the_branch_busy() { + let (store, branch) = setup_branch_store(); + start_running_note_session(&store, &branch.id); + queue_pull(&store, &branch.id).unwrap(); + + // The note session finishes, but the still-queued pull must keep a newly + // requested push on the queue rather than running it now. + for session in store.get_running_sessions().unwrap() { + store + .update_session_status(&session.id, store::SessionStatus::Completed, None, None) + .unwrap(); + } + + assert!(queue_push(&store, &branch.id, false).is_some()); + } + + #[test] + fn pull_steps_fetch_then_fast_forward_and_never_hand_off() { + // The drain path re-derives the ref, so a branch renamed while the pull + // waited fast-forwards the new name rather than the queued one. + let steps = build_pull_pipeline_steps("feature-renamed"); + + let (fetch_label, fetch_command, fetch_on_failure) = command_at(&steps, 0); + assert_eq!(fetch_label, "Fetch origin/feature-renamed"); + assert!(fetch_command.contains("git fetch origin feature-renamed")); + assert!(matches!( + fetch_on_failure, + FailureStrategy::Abort { marker: None } + )); + + let (merge_label, merge_command, merge_on_failure) = command_at(&steps, 1); + assert_eq!(merge_label, "Fast-forward to origin/feature-renamed"); + assert_eq!(merge_command, "git merge --ff-only origin/feature-renamed"); + // A diverged branch is the user's call to make, not an agent's. + assert!(matches!( + merge_on_failure, + FailureStrategy::Abort { marker: None } + )); + assert_eq!(steps.len(), 2); } #[tokio::test] diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index a9da0c3b2..b108a15e6 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -1758,8 +1758,9 @@ enum BranchSessionScheduleKind { Review, CommitPipeline, /// A git command pipeline that mutates the branch without producing a - /// commit (push / force push). Exclusive: it rewrites the remote from the - /// current worktree, so nothing else may touch the branch while it runs. + /// commit (push / force push / pull). Exclusive: it rewrites the remote from + /// the current worktree, or moves the worktree's HEAD, so nothing else may + /// touch the branch while it runs. GitPipeline, } @@ -1841,7 +1842,7 @@ fn exclusive_session_schedule(kind: BranchSessionScheduleKind) -> BranchSessionS enum PipelineBranchLink { /// Rebase/squash: found through the pending-commit artifact they create. Commit, - /// Push: creates no artifact, so it records `sessions.branch_id` instead. + /// Push/pull: create no artifact, so they record `sessions.branch_id` instead. Branch, } @@ -1850,7 +1851,7 @@ fn pipeline_branch_link(session: &store::Session) -> Option store::PipelineKind::Rebase | store::PipelineKind::Squash => { Some(PipelineBranchLink::Commit) } - store::PipelineKind::Push => Some(PipelineBranchLink::Branch), + store::PipelineKind::Push | store::PipelineKind::Pull => Some(PipelineBranchLink::Branch), } } @@ -5222,24 +5223,24 @@ mod tests { session } - /// A push pipeline session, linked to the branch without any artifact row. - fn create_branch_push_pipeline_session( + /// A push or pull pipeline session, linked to the branch without any artifact + /// row — which is what makes it resolve as a `GitPipeline` schedule. + fn create_branch_git_pipeline_session( store: &Arc, branch_id: &str, status: store::SessionStatus, - force: bool, + kind: store::PipelineKind, ) -> store::Session { + let prompt = format!("{kind:?}").to_lowercase(); let mut session = match status { - store::SessionStatus::Queued => store::Session::new_queued("push"), - store::SessionStatus::Running => store::Session::new_running("push", Path::new("/tmp")), + store::SessionStatus::Queued => store::Session::new_queued(&prompt), + store::SessionStatus::Running => { + store::Session::new_running(&prompt, Path::new("/tmp")) + } other => panic!("unsupported scheduler test status: {}", other.as_str()), } .with_branch(branch_id); - session.pipeline = Some( - store::PipelineExecution::from_steps(&[]) - .with_kind(store::PipelineKind::Push) - .with_push_force(force), - ); + session.pipeline = Some(store::PipelineExecution::from_steps(&[]).with_kind(kind)); store.create_session(&session).unwrap(); session } @@ -5447,24 +5448,26 @@ mod tests { } #[test] - fn running_push_pipeline_blocks_queued_note_review_and_commit() { - let (store, branch) = setup_branch_store(); - create_branch_push_pipeline_session( - &store, - &branch.id, - store::SessionStatus::Running, - false, - ); + fn running_git_pipeline_blocks_queued_note_review_and_commit() { + for kind in [store::PipelineKind::Push, store::PipelineKind::Pull] { + let (store, branch) = setup_branch_store(); + create_branch_git_pipeline_session( + &store, + &branch.id, + store::SessionStatus::Running, + kind, + ); - let active = running_branch_session_kinds(&store, &branch.id).unwrap(); + let active = running_branch_session_kinds(&store, &branch.id).unwrap(); - assert!(active.contains(&BranchSessionScheduleKind::GitPipeline)); - for kind in [ - BranchSessionScheduleKind::Note, - BranchSessionScheduleKind::Review, - BranchSessionScheduleKind::Commit, - ] { - assert!(!can_start_with_active_branch_sessions(kind, &active)); + assert!(active.contains(&BranchSessionScheduleKind::GitPipeline)); + for blocked in [ + BranchSessionScheduleKind::Note, + BranchSessionScheduleKind::Review, + BranchSessionScheduleKind::Commit, + ] { + assert!(!can_start_with_active_branch_sessions(blocked, &active)); + } } } @@ -5473,7 +5476,12 @@ mod tests { let (store, branch) = setup_branch_store(); let other = store::Branch::new(&branch.project_id, "other", "main"); store.create_branch(&other).unwrap(); - create_branch_push_pipeline_session(&store, &other.id, store::SessionStatus::Running, true); + create_branch_git_pipeline_session( + &store, + &other.id, + store::SessionStatus::Running, + store::PipelineKind::Push, + ); let active = running_branch_session_kinds(&store, &branch.id).unwrap(); @@ -5481,16 +5489,25 @@ mod tests { } #[test] - fn branch_start_decision_queues_all_user_modes_behind_queued_push() { - let (store, branch) = setup_branch_store_with_workdir(); - create_branch_push_pipeline_session(&store, &branch.id, store::SessionStatus::Queued, true); + fn branch_start_decision_queues_all_user_modes_behind_queued_git_pipeline() { + for kind in [store::PipelineKind::Push, store::PipelineKind::Pull] { + let (store, branch) = setup_branch_store_with_workdir(); + create_branch_git_pipeline_session( + &store, + &branch.id, + store::SessionStatus::Queued, + kind, + ); - for session_type in [ - BranchSessionType::Note, - BranchSessionType::Review, - BranchSessionType::Commit, - ] { - assert!(should_queue_branch_session_start(&store, &branch.id, &session_type).unwrap()); + for session_type in [ + BranchSessionType::Note, + BranchSessionType::Review, + BranchSessionType::Commit, + ] { + assert!( + should_queue_branch_session_start(&store, &branch.id, &session_type).unwrap() + ); + } } } diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index fab100a79..c86c42055 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -1315,25 +1315,35 @@ pub fn start_pipeline_session( } } } - PipelineOutcome::Aborted { .. } => { + PipelineOutcome::Aborted { step_index } => { // Pipeline aborted (e.g. non-fast-forward). Mark as completed so - // the frontend can inspect the pipeline steps for the failure. + // the frontend can inspect the pipeline steps for the failure — + // except for a pull, which has no such affordance and, when it + // was drained off the branch queue, no one watching it. See + // `aborted_pipeline_error`. resolve_pipeline_artifacts_without_ai(&config, &store_for_status, false); + let error = aborted_pipeline_error(&config, &store_for_status, step_index); + // The pipeline ran to its conclusion either way; only the + // outcome differs, so the completion reason stays the same. let reason = CompletionReason::TurnComplete; + let status = if error.is_some() { + SessionStatus::Error + } else { + SessionStatus::Completed + }; registry.deregister(&session_id); let transitioned = store_for_status - .transition_from_running( - &session_id, - SessionStatus::Completed, - None, - Some(&reason), - ) + .transition_from_running(&session_id, status, error.as_deref(), Some(&reason)) .unwrap_or(false); emit_status( &app_handle, &session_id, - "completed", - None, + if error.is_some() { + "error" + } else { + "completed" + }, + error, Some(&reason), config.branch_id.clone(), config.project_id.clone(), @@ -1406,6 +1416,47 @@ fn finish_failed_pipeline_handoff_start( .unwrap_or(false) } +/// Error message for an aborted pipeline, or `None` when the abort is an expected +/// outcome the session should still complete with. +/// +/// Only a pull produces an error. A push rejected as non-fast-forward completes so +/// the frontend can read the steps and offer a force push, but a pull that can't +/// fast-forward is a dead end whose fix is a user decision (rebase onto origin, or +/// reset to origin) — and a pull drained off the branch queue runs headless, so +/// the error status is the only way the user hears about it. +/// +/// The failing step's output is read back from the persisted pipeline, which +/// `run_pipeline` writes before it returns `Aborted`. +fn aborted_pipeline_error( + config: &PipelineConfig, + store: &Store, + step_index: usize, +) -> Option { + if config.pipeline.kind.as_ref() != Some(&PipelineKind::Pull) { + return None; + } + + let step = store + .get_session(&config.session_id) + .ok() + .flatten() + .and_then(|session| session.pipeline) + .and_then(|pipeline| pipeline.steps.into_iter().nth(step_index)); + let Some(step) = step else { + return Some("Pull failed".to_string()); + }; + + let detail = step + .output + .as_deref() + .map(str::trim) + .filter(|output| !output.is_empty()); + Some(match detail { + Some(detail) => format!("{} failed:\n\n{detail}", step.label), + None => format!("{} failed", step.label), + }) +} + fn pre_head_for_pipeline_handoff(config: &PipelineConfig) -> Option { match config.pipeline.kind.as_ref() { Some(PipelineKind::Rebase) => config.pre_head_sha.clone(), @@ -1416,9 +1467,9 @@ fn pre_head_for_pipeline_handoff(config: &PipelineConfig) -> Option { None } }, - // A push never rewrites local history, so an AI handoff has no - // pre-pipeline HEAD to compare against. - Some(PipelineKind::Push) | None => None, + // A push never rewrites local history and a pull never hands off, so + // neither has a pre-pipeline HEAD to compare against. + Some(PipelineKind::Push | PipelineKind::Pull) | None => None, } } @@ -1445,8 +1496,9 @@ fn resolve_pipeline_artifacts_without_ai(config: &PipelineConfig, store: &Store, ); } } - // Push pipelines create no artifact, so there is nothing to resolve. - Some(PipelineKind::Push) | None => {} + // Push and pull pipelines create no artifact, so there is nothing to + // resolve. + Some(PipelineKind::Push | PipelineKind::Pull) | None => {} } } @@ -3634,6 +3686,94 @@ mod tests { let _ = std::fs::remove_dir_all(repo); } + /// A session whose pipeline recorded `step_index` as failed, mirroring what + /// `run_pipeline` persists before it returns `Aborted`. + fn store_with_aborted_pipeline( + kind: PipelineKind, + steps: &[PipelineStep], + step_index: usize, + output: Option<&str>, + ) -> (Store, PipelineConfig) { + let store = Store::in_memory().unwrap(); + let mut pipeline = PipelineExecution::from_steps(steps).with_kind(kind.clone()); + pipeline.steps[step_index].status = StepStatus::Failed; + pipeline.steps[step_index].output = output.map(str::to_string); + let mut session = + crate::store::Session::new_running("git pipeline", std::path::Path::new("/tmp")); + session.pipeline = Some(pipeline.clone()); + store.create_session(&session).unwrap(); + + let config = PipelineConfig { + session_id: session.id, + prompt: "git pipeline".to_string(), + steps: steps.to_vec(), + pipeline, + working_dir: PathBuf::from("/tmp"), + pre_head_sha: None, + provider: None, + workspace_name: None, + remote_working_dir: None, + branch_id: None, + project_id: None, + }; + (store, config) + } + + fn abort_step(label: &str) -> PipelineStep { + PipelineStep::Command { + label: label.to_string(), + command: "true".to_string(), + on_failure: FailureStrategy::Abort { marker: None }, + } + } + + #[test] + fn aborted_pull_reports_the_failing_step_and_its_output() { + let steps = vec![ + abort_step("Fetch origin/feature"), + abort_step("Fast-forward"), + ]; + let (store, config) = store_with_aborted_pipeline( + PipelineKind::Pull, + &steps, + 1, + Some(" fatal: Not possible to fast-forward, aborting.\n"), + ); + + let error = aborted_pipeline_error(&config, &store, 1).unwrap(); + + assert_eq!( + error, + "Fast-forward failed:\n\nfatal: Not possible to fast-forward, aborting." + ); + } + + #[test] + fn aborted_pull_without_step_output_still_names_the_step() { + let steps = vec![abort_step("Fetch origin/feature")]; + let (store, config) = store_with_aborted_pipeline(PipelineKind::Pull, &steps, 0, None); + + assert_eq!( + aborted_pipeline_error(&config, &store, 0).unwrap(), + "Fetch origin/feature failed" + ); + } + + #[test] + fn aborted_push_completes_without_an_error_message() { + // A push rejected as non-fast-forward is an expected outcome: the session + // completes so the frontend can offer a force push. + let steps = vec![abort_step("Push to remote")]; + let (store, config) = store_with_aborted_pipeline( + PipelineKind::Push, + &steps, + 0, + Some("! [rejected] feature -> feature (non-fast-forward)"), + ); + + assert_eq!(aborted_pipeline_error(&config, &store, 0), None); + } + #[test] fn rebase_pipeline_completion_deletes_noop_pending_commit() { let repo = make_git_repo("rebase-noop"); diff --git a/apps/staged/src-tauri/src/store/models.rs b/apps/staged/src-tauri/src/store/models.rs index afe2d5d17..1325ae8fb 100644 --- a/apps/staged/src-tauri/src/store/models.rs +++ b/apps/staged/src-tauri/src/store/models.rs @@ -1526,14 +1526,15 @@ fn is_false(value: &bool) -> bool { /// Durable identity for command pipelines that the branch queue schedules. /// /// `Rebase` and `Squash` produce a commit and are linked to their branch through -/// a pending-commit artifact. `Push` produces no artifact and is linked through -/// `Session::branch_id` instead. +/// a pending-commit artifact. `Push` and `Pull` produce no artifact and are +/// linked through `Session::branch_id` instead. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "lowercase")] pub enum PipelineKind { Rebase, Squash, Push, + Pull, } #[cfg(test)] diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index bfa5aeb7a..88238f690 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -796,20 +796,11 @@ pub(crate) fn list_parent_branch_commits_impl( Ok(parse_parent_commit_lines(&lines)) } -#[tauri::command(rename_all = "camelCase")] -pub async fn pull_branch_ff_only( - store: tauri::State<'_, Mutex>>>, - branch_id: String, -) -> Result<(), String> { - let store = crate::get_store(&store)?; - - tauri::async_runtime::spawn_blocking(move || pull_branch_ff_only_impl(&store, &branch_id)) - .await - .map_err(|e| format!("Pull task failed: {e}"))? -} - -/// Synchronous body of [`pull_branch_ff_only`], shared verbatim with the -/// web-mode `dispatch()` arm. Callers run it inside `spawn_blocking`. +/// Fast-forward the branch to its upstream, right now. +/// +/// The immediate half of `prs::pull_or_queue_branch`, which owns the decision +/// between pulling now and queueing behind in-flight branch sessions. Callers run +/// this inside `spawn_blocking`. pub(crate) fn pull_branch_ff_only_impl(store: &Arc, branch_id: &str) -> Result<(), String> { let branch = store .get_branch(branch_id) diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 9bd00d99c..8a2158525 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2322,15 +2322,13 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { + "pull_or_queue_branch" => { let store = get_store(store_mutex)?; let branch_id: String = arg(&args, "branchId")?; - tauri::async_runtime::spawn_blocking(move || { - crate::timeline::pull_branch_ff_only_impl(&store, &branch_id) - }) - .await - .map_err(|e| format!("Pull task failed: {e}"))??; - Ok(Value::Null) + // Returns the queued session id, or null when the pull ran now. + let queued_session_id = + crate::prs::pull_or_queue_branch_for_branch(store, branch_id).await?; + Ok(serde_json::to_value(queued_session_id).unwrap()) } "reset_branch_to_remote" => { let store = get_store(store_mutex)?; diff --git a/apps/staged/src/lib/commands.test.ts b/apps/staged/src/lib/commands.test.ts index ee211c03b..f6b2d63a5 100644 --- a/apps/staged/src/lib/commands.test.ts +++ b/apps/staged/src/lib/commands.test.ts @@ -313,6 +313,25 @@ describe('browser-native command wrappers', () => { ]); }); + it('distinguishes an immediate pull from a queued one', async () => { + const invokeCommand = vi.fn().mockResolvedValueOnce(null).mockResolvedValueOnce('session-1'); + vi.doMock('./transport', () => ({ + invokeCommand, + isTauri: true, + })); + + const { pullOrQueueBranch } = await import('./commands'); + + // null: the branch was idle, so the pull already fast-forwarded. + await expect(pullOrQueueBranch('branch-1')).resolves.toBeNull(); + await expect(pullOrQueueBranch('branch-1')).resolves.toBe('session-1'); + + expect(invokeCommand.mock.calls).toEqual([ + ['pull_or_queue_branch', { branchId: 'branch-1' }], + ['pull_or_queue_branch', { branchId: 'branch-1' }], + ]); + }); + it('forwards ACP config selection when resuming a session', async () => { const invokeCommand = vi.fn().mockResolvedValue(undefined); vi.doMock('./transport', () => ({ diff --git a/apps/staged/src/lib/commands.ts b/apps/staged/src/lib/commands.ts index 54cce02d1..81ba745d4 100644 --- a/apps/staged/src/lib/commands.ts +++ b/apps/staged/src/lib/commands.ts @@ -547,8 +547,11 @@ export function invalidateProjectBranchTimelines(branchIds: string[]): void { window.dispatchEvent(new CustomEvent('timeline-invalidated', { detail: { branchIds } })); } -export function pullBranchFastForward(branchId: string): Promise { - return invokeCommand('pull_branch_ff_only', { branchId }); +/** Fast-forward a branch to origin, or queue the pull behind in-flight branch + * sessions. The backend owns that decision: it resolves to the queued session's + * ID when the pull has to wait, and to `null` when the pull already ran. */ +export function pullOrQueueBranch(branchId: string): Promise { + return invokeCommand('pull_or_queue_branch', { branchId }); } export function resetBranchToRemote(branchId: string): Promise { diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index 6de1374f7..0c92e3fb6 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -84,6 +84,7 @@ import { timelineToHashtagItems, projectNotesToHashtagItems } from '../sessions/hashtagItems'; import { getPreferredAgent } from '../settings/preferences.svelte'; import { agentState, REMOTE_AGENTS } from '../agents/agent.svelte'; + import { pullStateStore } from '../../stores/pullState.svelte'; import { pushStateStore } from '../../stores/pushState.svelte'; import { onBranchGitStateUpdated, @@ -1016,11 +1017,31 @@ } } + // A queued pull outlives this component (it drains when the branch frees up), + // so its state lives in the global pullStateStore alongside pushState. The + // immediate pull stays local: it is awaited right here. + let storePullState = $derived(pullStateStore.getPullState(branch.id)); + /** Pull is waiting on the branch session queue rather than running. */ + let pullQueuedOrigin = $derived(storePullState?.state === 'queued'); + let pullSessionId = $derived(storePullState?.sessionId ?? null); + /** True for both the immediate pull and a drained one still running. */ + let pullInFlight = $derived(pullingOrigin || storePullState?.state === 'pulling'); + + /** + * Pull origin's new commits, or queue the pull behind in-flight branch work. + * + * The backend decides which — an idle branch fast-forwards immediately, with no + * session row for what is usually an instant operation — and it dedupes repeat + * clicks, so this is deliberately not gated on `branchSessionBusy`. + */ async function handlePullOrigin() { - if (pullingOrigin) return; + if (pullInFlight || pullQueuedOrigin) return; pullingOrigin = true; try { - await commands.pullBranchFastForward(branch.id); + const queuedSessionId = await commands.pullOrQueueBranch(branch.id); + if (queuedSessionId) { + pullStateStore.setPullQueued(branch.id, queuedSessionId); + } await loadTimeline(); } catch (e) { notifyError('Pull failed', e); @@ -1029,6 +1050,26 @@ } } + /** + * Drop a pull that is still waiting on the branch queue. + * + * The store entry is cleared here rather than by the completion listener: a + * session that never ran isn't in the session registry, so its cancellation + * event carries no pull session type to match on. + */ + async function cancelQueuedPull() { + const sessionId = pullSessionId; + if (!pullQueuedOrigin || !sessionId) return; + pullStateStore.clearPullState(branch.id); + try { + await commands.cancelSession(sessionId); + commands.invalidateBranchTimeline(branch.id); + await loadTimeline(); + } catch (e) { + notifyError('Could not cancel queued pull', e); + } + } + function formatCommitCount(count: number, noun = 'commit'): string { return `${count} ${noun}${count === 1 ? '' : 's'}`; } @@ -1877,8 +1918,11 @@ ? openForcePushSession : undefined} onCancelQueuedPush={cancelQueuedPush} + onCancelQueuedPull={cancelQueuedPull} {forcePushingOrigin} {pushQueuedOrigin} + {pullQueuedOrigin} + {branchSessionBusy} {immediateGitActionDisabledReason} queueableGitActionDisabledReason={branchCommandDisabledReason} onViewWorktreeDiff={isLocal @@ -1895,7 +1939,7 @@ onDiscardWorktreeChanges={handleDiscardWorktreeChanges} onNewSessionReferring={(ref) => sessionMgr.openNewSessionReferring(ref)} newSessionDisabled={sessionMgr.isNewSessionDisabled || gitUnsafeActionsDisabled} - {pullingOrigin} + pullingOrigin={pullInFlight} {pushingOrigin} {resettingToOrigin} {discardingWorktreeChanges} diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index db53168d1..5f5fde30e 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -97,13 +97,23 @@ onOpenPushSession?: () => void; /** Cancel a push/force-push that is still waiting on the branch queue. */ onCancelQueuedPush?: () => void; + /** Cancel a pull that is still waiting on the branch queue. */ + onCancelQueuedPull?: () => void; /** Push is queued behind in-flight branch work. */ pushQueuedOrigin?: boolean; + /** Pull is queued behind in-flight branch work. */ + pullQueuedOrigin?: boolean; + /** + * True when the branch has queued or running sessions, so the backend will + * queue a git action rather than run it now. Only loosens the clean-worktree + * requirement on Pull — see `pullDisabledReason`. + */ + branchSessionBusy?: boolean; /** * Why reset-to-origin can't run right now. * * Reset still executes immediately rather than queueing — it is validated - * against a point-in-time preview — so unlike Rebase/Squash/Push it stays + * against a point-in-time preview — so unlike Rebase/Squash/Push/Pull it stays * disabled while the branch has sessions in flight. */ immediateGitActionDisabledReason?: string | null; @@ -112,7 +122,7 @@ * * These queue on the branch session queue like Rebase/Squash, so in-flight * sessions are not a reason to disable them — only branch identity problems - * are. + * are. Pull is queueable too, but computes its own reason from the git state. */ queueableGitActionDisabledReason?: string | null; onViewWorktreeDiff?: () => void; @@ -171,7 +181,10 @@ forcePushingOrigin = false, onOpenPushSession, onCancelQueuedPush, + onCancelQueuedPull, pushQueuedOrigin = false, + pullQueuedOrigin = false, + branchSessionBusy = false, immediateGitActionDisabledReason, queueableGitActionDisabledReason, onViewWorktreeDiff, @@ -268,6 +281,7 @@ pushing?: boolean; pushQueued?: boolean; forcePushQueued?: boolean; + pullQueued?: boolean; onViewDiff?: () => void; onCommitChanges?: () => void; commitChangesDisabledReason?: string; @@ -349,13 +363,25 @@ return detail ? `${title}: ${detail}` : title; } + /** + * Why Pull can't run right now. + * + * A dirty worktree only blocks the *immediate* pull: `git merge --ff-only` + * needs a clean tree. When the branch is busy the pull queues instead, and the + * dirt is almost always an agent's work in progress that its commit session + * clears before the pull drains — so the requirement is dropped there rather + * than disabling an action the backend would happily queue. + * + * Detached HEAD, the wrong branch, and a diverged upstream stay hard disables: + * none of them resolve by waiting. + */ function pullDisabledReason(state: BranchGitState): string | undefined { if (pullingOrigin) return 'Pulling...'; if (state.detachedHead) return 'Detached HEAD'; if (!state.expectedBranchMatches) { return state.currentBranch ? `Checked out ${state.currentBranch}` : 'Wrong branch'; } - if (state.worktree.dirty) return 'Clean worktree required'; + if (state.worktree.dirty && !branchSessionBusy) return 'Clean worktree required'; if (state.upstream.relation !== 'originAhead') return 'Not fast-forwardable'; return undefined; } @@ -436,16 +462,19 @@ } break; case 'originAhead': { - const disabledReason = pullDisabledReason(state); + // A queued pull keeps the button live so it can cancel the queued session. + const disabledReason = pullQueuedOrigin ? undefined : pullDisabledReason(state); rows.push({ key: 'git-origin-ahead', type: 'git-pull', title: `Origin has ${plural(state.upstream.behind, 'new commit')}`, + meta: pullQueuedOrigin ? 'Pull queued' : undefined, timestamp: bottomTimestamp, order: 1, placement: 'git-footer', - onPull: disabledReason ? undefined : onPullOrigin, + onPull: pullQueuedOrigin ? onCancelQueuedPull : disabledReason ? undefined : onPullOrigin, pullDisabledReason: disabledReason, + pullQueued: pullQueuedOrigin, }); break; } @@ -976,6 +1005,7 @@ pushing={item.pushing} pushQueued={item.pushQueued} forcePushQueued={item.forcePushQueued} + pullQueued={item.pullQueued} onViewDiffClick={item.onViewDiff} onCommitChangesClick={item.onCommitChanges} commitChangesDisabledReason={item.commitChangesDisabledReason} @@ -1065,6 +1095,7 @@ pushing={item.pushing} pushQueued={item.pushQueued} forcePushQueued={item.forcePushQueued} + pullQueued={item.pullQueued} onViewDiffClick={item.onViewDiff} onCommitChangesClick={item.onCommitChanges} commitChangesDisabledReason={item.commitChangesDisabledReason} diff --git a/apps/staged/src/lib/features/timeline/TimelineRow.svelte b/apps/staged/src/lib/features/timeline/TimelineRow.svelte index a5224a154..69c4edf54 100644 --- a/apps/staged/src/lib/features/timeline/TimelineRow.svelte +++ b/apps/staged/src/lib/features/timeline/TimelineRow.svelte @@ -87,6 +87,8 @@ pushQueued?: boolean; /** Force push is waiting on the branch queue, so its button cancels instead. */ forcePushQueued?: boolean; + /** Pull is waiting on the branch queue, so its button cancels instead. */ + pullQueued?: boolean; onViewDiffClick?: () => void; onCommitChangesClick?: () => void; commitChangesDisabledReason?: string; @@ -130,6 +132,7 @@ pushing = false, pushQueued = false, forcePushQueued = false, + pullQueued = false, onViewDiffClick, onCommitChangesClick, commitChangesDisabledReason, @@ -181,7 +184,7 @@ ); let isClickable = $derived(!!onItemClick && !isPending && !isFailed); let hasSession = $derived(!!sessionId && !deleting); - let pullTitle = $derived(pullDisabledReason ?? 'Pull'); + let pullTitle = $derived(pullDisabledReason ?? (pullQueued ? 'Cancel queued pull' : 'Pull')); let pushTitle = $derived( pushDisabledReason ?? (pushQueued ? 'Cancel queued push' : pushing ? 'View push session' : 'Push') @@ -456,7 +459,7 @@ aria-label={pullTitle} class="h-[22px] rounded-md border-[var(--border-subtle)] bg-transparent text-[var(--text-muted)] shadow-none hover:border-[var(--border-muted)] hover:bg-[var(--bg-hover)] hover:text-foreground" > - Pull + {pullQueued ? 'Cancel' : 'Pull'} {/if} diff --git a/apps/staged/src/lib/listeners/sessionStatusListener.ts b/apps/staged/src/lib/listeners/sessionStatusListener.ts index 46a38e192..d6bb73047 100644 --- a/apps/staged/src/lib/listeners/sessionStatusListener.ts +++ b/apps/staged/src/lib/listeners/sessionStatusListener.ts @@ -1,14 +1,16 @@ /** * Global listener for `session-status-changed` Tauri events. * - * Updates three independent state stores on session completion: + * Updates four independent state stores on session completion: * 1. projectState — aggregate view of all sessions in a project (project tiles) * 2. prState — branch-specific PR creation workflow state (PR buttons) * 3. pushState — branch-specific push workflow state (push operations) + * 4. pullState — branch-specific queued-pull state (pull footer row) * * Session lookups are delegated to the unified sessionRegistry for consistency. */ +import { toast } from 'svelte-sonner'; import { listenToEvent, type UnlistenFn } from '../transport'; import { invalidateBranchTimeline } from '../commands'; import * as commands from '../api/commands'; @@ -20,6 +22,7 @@ import { import { navigation } from '../features/layout/navigation.svelte'; import { projectStateStore } from '../stores/projectState.svelte'; import { prStateStore } from '../stores/prState.svelte'; +import { pullStateStore } from '../stores/pullState.svelte'; import { pushStateStore } from '../stores/pushState.svelte'; import { sessionRegistry, type SessionType } from '../stores/sessionRegistry.svelte'; import type { SessionStatus, SessionStatusPayload } from '../types'; @@ -29,6 +32,7 @@ export function listenForSessionStatus(): UnlistenFn { const { sessionId, status, + errorMessage, branchId: eventBranchId, projectId: eventProjectId, sessionType, @@ -46,11 +50,14 @@ export function listenForSessionStatus(): UnlistenFn { eventBranchId ); projectStateStore.addRunningSession(eventProjectId, sessionId); - // A push that was queued behind other branch work starts running when the - // branch queue drains it; this event is the only signal of that. + // A push or pull that was queued behind other branch work starts running + // when the branch queue drains it; this event is the only signal of that. if (sessionType === 'push' && eventBranchId) { pushStateStore.markQueuedPushStarted(eventBranchId, sessionId); } + if (sessionType === 'pull' && eventBranchId) { + pullStateStore.markQueuedPullStarted(eventBranchId, sessionId); + } return; } @@ -59,7 +66,7 @@ export function listenForSessionStatus(): UnlistenFn { if (eventBranchId) { invalidateBranchTimeline(eventBranchId); } - handleSessionEnd(sessionId, status); + handleSessionEnd(sessionId, status, errorMessage); } }); } @@ -68,7 +75,11 @@ export function listenForSessionStatus(): UnlistenFn { // Completion sub-handlers // --------------------------------------------------------------------------- -async function handleSessionEnd(sessionId: string, status: SessionStatus) { +async function handleSessionEnd( + sessionId: string, + status: SessionStatus, + errorMessage?: string | null +) { const sessionProjectId = sessionRegistry.getProjectId(sessionId); const sessionType = sessionRegistry.getType(sessionId); const branchId = sessionRegistry.getBranchId(sessionId); @@ -93,10 +104,36 @@ async function handleSessionEnd(sessionId: string, status: SessionStatus) { pushStateStore.clearSessionTracking(branchId); } + if (sessionType === 'pull' && branchId) { + handlePullCompletion(branchId, status, errorMessage); + } + // Remove running state from projectStateStore and unregister from the registry. sessionRegistry.cleanupSession(sessionId); } +/** + * Release the pull row and report a failed pull. + * + * A queued pull is drained headless, so the status event is the only place its + * failure surfaces: the backend ends an unpullable session in `error` with the + * failing step's output (see `session_runner::aborted_pipeline_error`), and the + * usual fix — rebase onto origin, or reset to origin — is the user's call. On + * success the row simply disappears, since the branch is no longer behind. + */ +function handlePullCompletion( + branchId: string, + status: SessionStatus, + errorMessage?: string | null +) { + pullStateStore.clearPullState(branchId); + if (status !== 'error') return; + toast.error('Pull failed', { + description: errorMessage ?? 'The queued pull could not fast-forward this branch.', + duration: Infinity, + }); +} + async function handlePrCompletion(sessionId: string, branchId: string, status: SessionStatus) { if (status === 'completed') { try { diff --git a/apps/staged/src/lib/shared/utils.ts b/apps/staged/src/lib/shared/utils.ts index 7db84b428..516ce1265 100644 --- a/apps/staged/src/lib/shared/utils.ts +++ b/apps/staged/src/lib/shared/utils.ts @@ -113,6 +113,8 @@ function sessionTypeLabel(type: SessionType): string { return 'PR'; case 'push': return 'push'; + case 'pull': + return 'pull'; case 'other': return 'task'; } @@ -179,12 +181,16 @@ export function projectActivity( counts.set(type, (counts.get(type) ?? 0) + 1); } - // Special-case "push" to read more naturally: "pushing changes" instead of "making a push" + // Special-case "push"/"pull" to read more naturally: "pushing changes" instead + // of "making a push" if (counts.size === 1 && counts.has('push')) { return 'pushing changes'; } + if (counts.size === 1 && counts.has('pull')) { + return 'pulling changes'; + } - const displayOrder: SessionType[] = ['commit', 'note', 'review', 'pr', 'push', 'other']; + const displayOrder: SessionType[] = ['commit', 'note', 'review', 'pr', 'push', 'pull', 'other']; const parts: string[] = []; for (const type of displayOrder) { diff --git a/apps/staged/src/lib/stores/pullState.svelte.ts b/apps/staged/src/lib/stores/pullState.svelte.ts new file mode 100644 index 000000000..7e7f1450b --- /dev/null +++ b/apps/staged/src/lib/stores/pullState.svelte.ts @@ -0,0 +1,93 @@ +/** + * Global pull state store. + * + * Only queued pulls live here. An immediate pull is a direct git operation the + * branch card awaits inline, but a queued one outlives the click: it waits on the + * branch session queue, then runs when the queue drains it. Keeping it in a store + * (rather than in BranchCard state) means the "Queued" badge survives the remount + * that happens when the user switches projects and back — the same reason + * `pushState` exists. + * + * Like `pushState`, this is frontend-only: a pull that is still queued when the + * app restarts drains without ever having been badged as queued. + * + * The Map is wrapped in $state, but $state(Map) does not give fine-grained + * reactivity for .get()/.set(), so a version counter is bumped on every mutation + * and read by `getPullState` to establish the dependency. + */ + +/** `queued` flips to `pulling` when the branch queue drains the session. */ +export type PullState = 'queued' | 'pulling'; + +interface BranchPullState { + state: PullState; + sessionId: string; + timestamp: number; +} + +const MAX_STORE_SIZE = 100; +const STATE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours +const CLEANUP_THRESHOLD = 0.8; + +class PullStateStore { + private states = $state>(new Map()); + private version = $state(0); + + getPullState(branchId: string): BranchPullState | null { + // Read the counter so $derived callers re-evaluate on any mutation. + this.version; + return this.states.get(branchId) ?? null; + } + + /** Record a pull the backend put on the branch queue. */ + setPullQueued(branchId: string, sessionId: string): void { + this.set(branchId, { state: 'queued', sessionId, timestamp: Date.now() }); + } + + /** + * Flip a queued pull to `pulling` once the branch queue drains it. + * + * Guarded on the session ID so an unrelated pull event can't revive a stale + * queued entry (e.g. one the user cancelled and re-requested). + */ + markQueuedPullStarted(branchId: string, sessionId: string): void { + const existing = this.states.get(branchId); + if (existing?.state !== 'queued' || existing.sessionId !== sessionId) { + return; + } + this.set(branchId, { state: 'pulling', sessionId, timestamp: Date.now() }); + } + + clearPullState(branchId: string): void { + this.states.delete(branchId); + this.version++; + } + + private set(branchId: string, next: BranchPullState): void { + if (this.states.size >= MAX_STORE_SIZE * CLEANUP_THRESHOLD) { + this.cleanup(); + } + this.states.set(branchId, next); + this.version++; + } + + /** Drop stale entries so a long-running app doesn't accumulate them. */ + private cleanup(): void { + const now = Date.now(); + for (const [branchId, state] of this.states.entries()) { + if (now - state.timestamp > STATE_TTL_MS) { + this.states.delete(branchId); + } + } + + if (this.states.size > MAX_STORE_SIZE) { + const entries = Array.from(this.states.entries()); + entries.sort((a, b) => a[1].timestamp - b[1].timestamp); + for (const [branchId] of entries.slice(0, entries.length - MAX_STORE_SIZE)) { + this.states.delete(branchId); + } + } + } +} + +export const pullStateStore = new PullStateStore(); diff --git a/apps/staged/src/lib/stores/sessionRegistry.svelte.ts b/apps/staged/src/lib/stores/sessionRegistry.svelte.ts index b7089015b..24346e1d9 100644 --- a/apps/staged/src/lib/stores/sessionRegistry.svelte.ts +++ b/apps/staged/src/lib/stores/sessionRegistry.svelte.ts @@ -19,12 +19,12 @@ import { projectStateStore } from './projectState.svelte'; -export type SessionType = 'commit' | 'pr' | 'push' | 'note' | 'review' | 'other'; +export type SessionType = 'commit' | 'pr' | 'push' | 'pull' | 'note' | 'review' | 'other'; interface SessionMetadata { sessionId: string; projectId: string; - branchId?: string; // Optional: only PR and push sessions are tied to a specific branch + branchId?: string; // Optional: only PR, push, and pull sessions are tied to a specific branch type: SessionType; timestamp: number; // When the session was registered } diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index eb3dca95d..4d484d728 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -409,7 +409,7 @@ export interface QueuedSessionMessage { export type StepStatus = 'pending' | 'running' | 'succeeded' | 'failed' | 'skipped'; export type StepType = 'command' | 'ai_handoff'; -export type PipelineKind = 'rebase' | 'squash' | 'push'; +export type PipelineKind = 'rebase' | 'squash' | 'push' | 'pull'; export interface PipelineStepStatus { label: string; From afcfb2ac305ca56775fd8062bdb8b1ab47dd30d5 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 17:13:25 +1000 Subject: [PATCH 4/9] fix(staged): close the run-now races the git-action queue left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the queued git actions work (review 5368736f). The queue was correct once a session existed; the gaps were all in the moment before one did, plus two frontend signals that could disagree with the backend. Backend: - The rebase/squash and push run-now paths checked `branch_has_work_in_flight` under the branch launch lock, then released it to resolve the pipeline context and inserted the running session with no second look. Two near-simultaneous actions could both see an idle branch and both start. Both paths now re-check and insert under the lock, mirroring `start_or_queue_branch_session_for_store`: `queue_*_pipeline_if_branch_busy` splits into a locking wrapper plus a `_locked` body, and `start_running_*_pipeline_for_branch` splits into an `insert_running_*_pipeline_session` (under the lock, writes the rows that make the branch look busy) and a shared `launch_running_pipeline_session` (after it, emits + hands off to the runner). The pre-flight check stays so an already-busy branch still queues without resolving a context it may not be able to. - An immediate pull created no session row, so for a network fetch plus `merge --ff-only` the branch looked idle to `has_running_session_for_branch` and the drain scan — the one mutating git op the queue could not see. `claim_or_queue_pull_for_branch` now decides queue-vs-run and records either outcome under one lock: a queued pull as before, or a running `PipelineKind::Pull` session linked to the branch (no artifact, like a push) that marks the branch busy for the pull's duration. It emits no status event, because the caller still awaits the pull and reports the outcome itself; `finish_immediate_pull_session` ends it, then the branch queue is drained since the marker bypassed the runner that normally would. `pull_or_queue_branch` therefore takes the registry and app handle now. Frontend: - `branchSessionBusy` was purely timeline-derived, and a push or pull creates no artifact, so a mid-push branch read as idle: Pull stayed disabled by a dirty worktree even though the click would have queued. The push/pull store reads move above it and fold in through a new tested `isGitActionInFlight` helper. Reset to origin and discard changes now also stay blocked during a push or pull, which they should have been. - Queued pulls had no polling fallback, so a missed `session-status-changed` left the "Pull queued" badge stuck until the user hit Cancel and lost the failure toast. BranchCard polls the pull session every 5s, like BranchCardPrButton does for pushes. - `pullDisabledReason`'s doc comment claimed the dirt clears before the pull drains; it now states the actual accepted failure mode (a stale timeline means the pull runs immediately and fails with "Cannot pull with uncommitted changes"). `BranchPipelineResponse`'s doc mentions push and calls out pull's different return shape. No data-model changes: the marker session reuses `PipelineKind::Pull` and `sessions.branch_id`. `PullDisposition` and `RunningPipelineSession` are in-memory only. Tests: the marker session's shape, that it blocks a concurrent rebase, push and pull, and that finishing it frees the branch and records the failure message; that inserting a running commit or push pipeline is what the re-check reads; and `isGitActionInFlight` cases. `just fmt-check`, `just lint`, `just typecheck`, `just test` (590), and `pnpm test` (487) pass. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/prs.rs | 525 ++++++++++++++---- apps/staged/src-tauri/src/web_server.rs | 9 +- .../lib/features/branches/BranchCard.svelte | 89 ++- .../branches/branchCardHelpers.test.ts | 22 + .../features/branches/branchCardHelpers.ts | 24 + .../features/timeline/BranchTimeline.svelte | 8 + apps/staged/src/lib/types.ts | 6 +- 7 files changed, 568 insertions(+), 115 deletions(-) diff --git a/apps/staged/src-tauri/src/prs.rs b/apps/staged/src-tauri/src/prs.rs index ac549f91f..f8a78ce77 100644 --- a/apps/staged/src-tauri/src/prs.rs +++ b/apps/staged/src-tauri/src/prs.rs @@ -493,50 +493,85 @@ fn build_pull_pipeline_steps(branch_name: &str) -> Vec { ] } -#[allow(clippy::too_many_arguments)] -async fn start_running_commit_pipeline_for_branch( - ctx: BranchPipelineContext, +/// The rows a run-now pipeline needs before the session runner can pick it up. +/// +/// Inserted under the branch launch lock and consumed by +/// [`launch_running_pipeline_session`] once the lock is released. +struct RunningPipelineSession { + session_id: String, + pipeline: PipelineExecution, + prompt: &'static str, +} + +/// Insert the session and pending-commit rows for a rebase/squash that runs now. +/// +/// Callers must hold the branch launch lock: these rows are what make the branch +/// look busy to a concurrent launch, so the busy check and this insert have to be +/// atomic. Synchronous for the same reason — the lock must not be held across an +/// await. +fn insert_running_commit_pipeline_session( + store: &Arc, + ctx: &BranchPipelineContext, kind: PipelineKind, - steps: Vec, + steps: &[PipelineStep], rebase_target: Option, - provider: Option, - store: Arc, - app_handle: &tauri::AppHandle, - registry: &Arc, -) -> Result { + provider: Option<&str>, +) -> Result { let prompt = pipeline_prompt(&kind, false); - let mut pipeline = PipelineExecution::from_steps(&steps).with_kind(kind); + let mut pipeline = PipelineExecution::from_steps(steps).with_kind(kind); if let Some(target) = rebase_target { pipeline = pipeline.with_rebase_target(target); } let mut session = store::Session::new_running(prompt, &ctx.working_dir); - if let Some(ref p) = provider { + if let Some(p) = provider { session = session.with_provider(p); } session.pipeline = Some(pipeline.clone()); store.create_session(&session).map_err(|e| e.to_string())?; + let commit = store::Commit::new_pending(&ctx.branch.id).with_session(&session.id); + store.create_commit(&commit).map_err(|e| e.to_string())?; + + Ok(RunningPipelineSession { + session_id: session.id, + pipeline, + prompt, + }) +} + +/// Announce a run-now pipeline session and hand it to the session runner. +/// +/// Runs after the branch launch lock is released: the session row already exists, +/// so anything racing this already sees the branch as busy. +#[allow(clippy::too_many_arguments)] +fn launch_running_pipeline_session( + ctx: BranchPipelineContext, + running: RunningPipelineSession, + steps: Vec, + session_type: &str, + provider: Option, + store: Arc, + app_handle: &tauri::AppHandle, + registry: &Arc, +) -> Result { let branch_id = ctx.branch.id.clone(); let project_id = ctx.branch.project_id.clone(); - let commit = store::Commit::new_pending(&branch_id).with_session(&session.id); - store.create_commit(&commit).map_err(|e| e.to_string())?; - session_runner::emit_session_running( app_handle, - &session.id, + &running.session_id, &branch_id, &project_id, - "commit", + session_type, ); session_runner::start_pipeline_session( session_runner::PipelineConfig { - session_id: session.id.clone(), - prompt: prompt.to_string(), + session_id: running.session_id.clone(), + prompt: running.prompt.to_string(), steps, - pipeline, + pipeline: running.pipeline, working_dir: ctx.working_dir, pre_head_sha: None, provider, @@ -550,7 +585,7 @@ async fn start_running_commit_pipeline_for_branch( Arc::clone(registry), )?; - Ok(session.id) + Ok(running.session_id) } /// Queue a rebase/squash pipeline when the branch has work in flight. @@ -572,7 +607,18 @@ fn queue_commit_pipeline_if_branch_busy( ) -> Result, String> { let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); let _guard = launch_lock.lock().unwrap(); + queue_commit_pipeline_locked(store, branch_id, kind, provider, target) +} +/// The body of [`queue_commit_pipeline_if_branch_busy`], for the run-now path, +/// which re-checks while already holding the branch launch lock. +fn queue_commit_pipeline_locked( + store: &Arc, + branch_id: &str, + kind: &PipelineKind, + provider: Option<&str>, + target: Option<&str>, +) -> Result, String> { if !branch_has_work_in_flight(store, branch_id)? { return Ok(None); } @@ -620,6 +666,9 @@ pub(crate) async fn start_or_queue_commit_pipeline_for_branch( provider: Option, target: Option, ) -> Result { + // Pre-flight check: a branch that is already busy queues without resolving a + // pipeline context, which for a remote branch can depend on a running + // workspace the queued work will only need later. if let Some(session_id) = queue_commit_pipeline_if_branch_busy( &store, &branch_id, @@ -636,17 +685,45 @@ pub(crate) async fn start_or_queue_commit_pipeline_for_branch( let steps = build_commit_pipeline_steps(&kind, &base_branch, &rebase_ref)?; let rebase_target = persisted_rebase_target(target.as_deref(), &rebase_ref); - let session_id = start_running_commit_pipeline_for_branch( + // Re-check and insert under the lock, mirroring + // `session_commands::start_or_queue_branch_session_for_store`: the pre-flight + // check released the lock to resolve the context above, so without a second + // look two near-simultaneous actions could both have seen an idle branch and + // both start running — exactly what git-pipeline exclusivity exists to stop. + let running = { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(&branch_id); + let _guard = launch_lock.lock().unwrap(); + + if let Some(session_id) = queue_commit_pipeline_locked( + &store, + &branch_id, + &kind, + provider.as_deref(), + target.as_deref(), + )? { + return Ok(BranchPipelineResponse::queued(session_id)); + } + + insert_running_commit_pipeline_session( + &store, + &ctx, + kind, + &steps, + rebase_target, + provider.as_deref(), + )? + }; + + let session_id = launch_running_pipeline_session( ctx, - kind, + running, steps, - rebase_target, + "commit", provider, store, &app_handle, ®istry, - ) - .await?; + )?; Ok(BranchPipelineResponse::running(session_id)) } @@ -733,59 +810,39 @@ pub(crate) async fn start_queued_commit_pipeline_for_branch( Ok(true) } -/// Start a push pipeline that runs right now. +/// Insert the session row for a push that runs right now. /// /// Unlike [`start_pipeline_for_branch`], the session records its pipeline kind /// and `branch_id`. A push creates no artifact, so without that link the branch /// queue could not see it and a commit session could start mid-push. -fn start_running_push_pipeline_for_branch( - ctx: BranchPipelineContext, +/// +/// Like [`insert_running_commit_pipeline_session`], callers must hold the branch +/// launch lock: this row is what makes the branch look busy. +fn insert_running_push_pipeline_session( + store: &Arc, + ctx: &BranchPipelineContext, force: bool, - steps: Vec, - provider: Option, - store: Arc, - app_handle: &tauri::AppHandle, - registry: &Arc, -) -> Result { + steps: &[PipelineStep], + provider: Option<&str>, +) -> Result { let prompt = pipeline_prompt(&PipelineKind::Push, force); - let pipeline = PipelineExecution::from_steps(&steps) + let pipeline = PipelineExecution::from_steps(steps) .with_kind(PipelineKind::Push) .with_push_force(force); - let branch_id = ctx.branch.id.clone(); - let project_id = ctx.branch.project_id.clone(); - - let mut session = store::Session::new_running(prompt, &ctx.working_dir).with_branch(&branch_id); - if let Some(ref p) = provider { + let mut session = + store::Session::new_running(prompt, &ctx.working_dir).with_branch(&ctx.branch.id); + if let Some(p) = provider { session = session.with_provider(p); } session.pipeline = Some(pipeline.clone()); store.create_session(&session).map_err(|e| e.to_string())?; - // Emit "running" before returning so the global session listener registers - // this session atomically, as `start_pipeline_for_branch` does. - session_runner::emit_session_running(app_handle, &session.id, &branch_id, &project_id, "push"); - - session_runner::start_pipeline_session( - session_runner::PipelineConfig { - session_id: session.id.clone(), - prompt: prompt.to_string(), - steps, - pipeline, - working_dir: ctx.working_dir, - pre_head_sha: None, - provider, - workspace_name: ctx.workspace_name, - remote_working_dir: ctx.remote_working_dir, - branch_id: Some(branch_id), - project_id: Some(project_id), - }, - store, - app_handle.clone(), - Arc::clone(registry), - )?; - - Ok(session.id) + Ok(RunningPipelineSession { + session_id: session.id, + pipeline, + prompt, + }) } /// Queue a push pipeline when the branch has work in flight. @@ -807,7 +864,17 @@ fn queue_push_pipeline_if_branch_busy( ) -> Result, String> { let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); let _guard = launch_lock.lock().unwrap(); + queue_push_pipeline_locked(store, branch_id, provider, force) +} +/// The body of [`queue_push_pipeline_if_branch_busy`], for the run-now path, +/// which re-checks while already holding the branch launch lock. +fn queue_push_pipeline_locked( + store: &Arc, + branch_id: &str, + provider: Option<&str>, + force: bool, +) -> Result, String> { if !branch_has_work_in_flight(store, branch_id)? { return Ok(None); } @@ -936,33 +1003,50 @@ pub(crate) async fn start_queued_git_pipeline_for_branch( Ok(true) } -/// Queue a fast-forward pull when the branch has work in flight. +/// What the branch queue decided to do with a pull request. +enum PullDisposition { + /// Waiting its turn: either a freshly queued session, or the queued pull that + /// already covers this request. + Queued(String), + /// The branch was idle, so the pull runs now. The id is the session that marks + /// the branch busy for its duration. + RunningNow(String), +} + +/// Decide between pulling now and queueing behind in-flight branch work, and +/// record that decision. /// -/// Returns the queued session id — either a freshly created one, or the queued -/// pull that already covers this request — and `None` when the branch is idle so -/// the caller should pull immediately. +/// Mirrors [`queue_push_pipeline_if_branch_busy`]: the busy check, the dedupe +/// scan, and the insert all run under the branch launch lock, and stay +/// synchronous because the lock must not be held across an await. A pull has no +/// variants, so the dedupe keys on the kind alone. /// -/// Mirrors [`queue_push_pipeline_if_branch_busy`], including running the busy -/// check, the dedupe scan, and the insert under the branch launch lock. A pull has -/// no variants, so the dedupe keys on the kind alone. +/// The run-now case still gets a session row — a running `PipelineKind::Pull` +/// linked to the branch with no artifact, exactly like a running push. It is what +/// makes the fetch-and-merge visible to `has_running_session_for_branch` and the +/// drain scan; without it the one mutating git operation that skips the pipeline +/// runner would look idle for its whole duration, and a commit session or another +/// git action could start against the same worktree mid-merge. No +/// `session-status-changed` event is emitted for it: the caller awaits the pull and +/// reports the outcome itself, so an event would double-report a failure and spin +/// the project tile for what is usually an instant operation. /// -/// No provider is recorded: every pull step aborts on failure, so the pipeline -/// never hands off to an agent. -fn queue_pull_pipeline_if_branch_busy( +/// No provider is recorded either: every pull step aborts on failure, so the +/// pipeline never hands off to an agent. +fn claim_or_queue_pull_for_branch( store: &Arc, branch_id: &str, -) -> Result, String> { +) -> Result { let launch_lock = crate::session_commands::branch_session_launch_lock_for(branch_id); let _guard = launch_lock.lock().unwrap(); - if !branch_has_work_in_flight(store, branch_id)? { - return Ok(None); - } - - if let Some(existing) = find_queued_pipeline(store, branch_id, |pipeline| { - pipeline.kind.as_ref() == Some(&PipelineKind::Pull) - })? { - return Ok(Some(existing)); + let busy = branch_has_work_in_flight(store, branch_id)?; + if busy { + if let Some(existing) = find_queued_pipeline(store, branch_id, |pipeline| { + pipeline.kind.as_ref() == Some(&PipelineKind::Pull) + })? { + return Ok(PullDisposition::Queued(existing)); + } } let branch = store @@ -972,38 +1056,112 @@ fn queue_pull_pipeline_if_branch_busy( let steps = build_pull_pipeline_steps(&branch.branch_name); let pipeline = PipelineExecution::from_steps(&steps).with_kind(PipelineKind::Pull); - // Like a queued push, this session carries no artifact — `branch_id` is what + // Like a push session, this one carries no artifact — `branch_id` is what // keeps it on the branch queue. - let mut session = store::Session::new_queued(PULL_PROMPT).with_branch(branch_id); + let mut session = if busy { + store::Session::new_queued(PULL_PROMPT) + } else { + store::Session::new_running(PULL_PROMPT, &immediate_pull_working_dir(store, branch_id)) + } + .with_branch(branch_id); session.pipeline = Some(pipeline); store.create_session(&session).map_err(|e| e.to_string())?; - Ok(Some(session.id)) + Ok(if busy { + PullDisposition::Queued(session.id) + } else { + PullDisposition::RunningNow(session.id) + }) +} + +/// Best-effort working directory for the session row of an immediate pull. +/// +/// The pull resolves its own path (a remote branch fast-forwards through its +/// workspace shell), so this only decides what the session row displays — not +/// where anything runs, and not whether the pull can proceed. +fn immediate_pull_working_dir(store: &Arc, branch_id: &str) -> PathBuf { + store + .get_workdir_for_branch(branch_id) + .ok() + .flatten() + .map(|workdir| PathBuf::from(workdir.path)) + .unwrap_or_default() +} + +/// Release the session that marked the branch busy for an immediate pull. +/// +/// The marker never went through the session runner, so nothing else will end it. +/// The completion reason matches the pipeline path: the work ran to its conclusion +/// either way, only the outcome differs. +fn finish_immediate_pull_session(store: &Arc, session_id: &str, error: Option<&str>) { + let status = if error.is_some() { + store::SessionStatus::Error + } else { + store::SessionStatus::Completed + }; + + if let Err(e) = store.transition_from_running( + session_id, + status, + error, + Some(&store::CompletionReason::TurnComplete), + ) { + log::warn!("[prs] Failed to finish immediate pull session {session_id}: {e}"); + } } /// Fast-forward the branch to origin now, or queue the pull behind in-flight /// branch work. /// -/// An idle branch pulls directly (no session row for what is usually an instant -/// operation), which is why this returns an `Option` rather than the -/// [`BranchPipelineResponse`] the pipeline-only actions use: `None` means the -/// pull already happened, `Some(session_id)` means it is waiting its turn on the -/// branch queue. +/// An idle branch pulls directly rather than through the pipeline runner, which is +/// why this returns an `Option` rather than the [`BranchPipelineResponse`] the +/// pipeline-only actions use: `None` means the pull already happened (and its +/// failure, if any, is this call's error), `Some(session_id)` means it is waiting +/// its turn on the branch queue. pub(crate) async fn pull_or_queue_branch_for_branch( store: Arc, + registry: Arc, + app_handle: tauri::AppHandle, branch_id: String, ) -> Result, String> { - if let Some(session_id) = queue_pull_pipeline_if_branch_busy(&store, &branch_id)? { - return Ok(Some(session_id)); - } + let session_id = match claim_or_queue_pull_for_branch(&store, &branch_id)? { + PullDisposition::Queued(session_id) => return Ok(Some(session_id)), + PullDisposition::RunningNow(session_id) => session_id, + }; - tauri::async_runtime::spawn_blocking(move || { - crate::timeline::pull_branch_ff_only_impl(&store, &branch_id) + let pull_store = Arc::clone(&store); + let pull_branch_id = branch_id.clone(); + let pulled = tauri::async_runtime::spawn_blocking(move || { + crate::timeline::pull_branch_ff_only_impl(&pull_store, &pull_branch_id) }) .await - .map_err(|e| format!("Pull task failed: {e}"))??; + .map_err(|e| format!("Pull task failed: {e}"))?; - Ok(None) + finish_immediate_pull_session( + &store, + &session_id, + pulled.as_ref().err().map(String::as_str), + ); + + // Anything the user requested while the pull held the branch is queued behind + // the marker session, and the marker bypassed the session runner that would + // normally drain it. Spawned rather than awaited so the pull's own result + // isn't held up by starting someone else's work. + tauri::async_runtime::spawn(async move { + if let Err(e) = crate::session_commands::drain_queued_sessions_for_branch( + store, + registry, + app_handle, + branch_id.clone(), + None, + ) + .await + { + log::warn!("[prs] Failed to drain queued sessions after pulling {branch_id}: {e}"); + } + }); + + pulled.map(|()| None) } /// Pull origin's new commits into the branch. @@ -1013,10 +1171,12 @@ pub(crate) async fn pull_or_queue_branch_for_branch( #[tauri::command(rename_all = "camelCase")] pub async fn pull_or_queue_branch( store: tauri::State<'_, Mutex>>>, + registry: tauri::State<'_, Arc>, + app_handle: tauri::AppHandle, branch_id: String, ) -> Result, String> { let store = get_store(&store)?; - pull_or_queue_branch_for_branch(store, branch_id).await + pull_or_queue_branch_for_branch(store, Arc::clone(®istry), app_handle, branch_id).await } fn create_pr_handoff_prompt( @@ -1681,6 +1841,8 @@ pub(crate) async fn start_or_queue_push_pipeline_for_branch( ) -> Result { let force = force.unwrap_or(false); + // Pre-flight check, then a re-check at insert time — see + // `start_or_queue_commit_pipeline_for_branch` for why both are needed. if let Some(session_id) = queue_push_pipeline_if_branch_busy(&store, &branch_id, provider.as_deref(), force)? { @@ -1690,10 +1852,24 @@ pub(crate) async fn start_or_queue_push_pipeline_for_branch( let ctx = resolve_branch_pipeline_context(&store, &branch_id)?; let steps = build_push_pipeline_steps(&ctx.branch.branch_name, force); - let session_id = start_running_push_pipeline_for_branch( + let running = { + let launch_lock = crate::session_commands::branch_session_launch_lock_for(&branch_id); + let _guard = launch_lock.lock().unwrap(); + + if let Some(session_id) = + queue_push_pipeline_locked(&store, &branch_id, provider.as_deref(), force)? + { + return Ok(BranchPipelineResponse::queued(session_id)); + } + + insert_running_push_pipeline_session(&store, &ctx, force, &steps, provider.as_deref())? + }; + + let session_id = launch_running_pipeline_session( ctx, - force, + running, steps, + "push", provider, store, &app_handle, @@ -2084,6 +2260,78 @@ mod tests { assert!(queue_push(&store, &branch.id, false).is_some()); } + fn pipeline_context(branch: &store::Branch) -> BranchPipelineContext { + BranchPipelineContext { + branch: branch.clone(), + working_dir: PathBuf::from("/tmp/staged-test"), + workspace_name: None, + remote_working_dir: None, + } + } + + #[test] + fn inserting_a_running_pipeline_is_what_makes_the_branch_busy() { + let (store, branch) = setup_branch_store(); + let steps = build_commit_pipeline_steps(&PipelineKind::Rebase, "main", "main").unwrap(); + + // The pre-flight check sees an idle branch and lets the rebase run now. + assert_eq!( + queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None), + None + ); + insert_running_commit_pipeline_session( + &store, + &pipeline_context(&branch), + PipelineKind::Rebase, + &steps, + None, + None, + ) + .unwrap(); + + // The rows that insert wrote are what the re-check at insert time reads, so + // an action that raced it queues instead of also starting. + assert!(queue_commit_pipeline_locked( + &store, + &branch.id, + &PipelineKind::Squash, + None, + None + ) + .unwrap() + .is_some()); + assert!(queue_push(&store, &branch.id, false).is_some()); + } + + #[test] + fn inserting_a_running_push_is_what_makes_the_branch_busy() { + let (store, branch) = setup_branch_store(); + let steps = build_push_pipeline_steps(&branch.branch_name, false); + + insert_running_push_pipeline_session( + &store, + &pipeline_context(&branch), + false, + &steps, + None, + ) + .unwrap(); + + // A push has no artifact, so `branch_id` is what the re-check keys on. + assert!(queue_push_pipeline_locked(&store, &branch.id, None, true) + .unwrap() + .is_some()); + assert!(queue_commit_pipeline_locked( + &store, + &branch.id, + &PipelineKind::Rebase, + None, + None + ) + .unwrap() + .is_some()); + } + #[test] fn push_steps_are_rebuilt_from_the_current_branch_name_and_force_flag() { let normal = build_push_pipeline_steps("feature", false); @@ -2118,8 +2366,22 @@ mod tests { assert_eq!(pull_err, "Pull is not a commit pipeline"); } + /// The queued session id, or `None` when the branch was idle and the pull + /// claimed it to run now. fn queue_pull(store: &Arc, branch_id: &str) -> Option { - queue_pull_pipeline_if_branch_busy(store, branch_id).unwrap() + match claim_or_queue_pull_for_branch(store, branch_id).unwrap() { + PullDisposition::Queued(session_id) => Some(session_id), + PullDisposition::RunningNow(_) => None, + } + } + + fn claim_pull(store: &Arc, branch_id: &str) -> String { + match claim_or_queue_pull_for_branch(store, branch_id).unwrap() { + PullDisposition::RunningNow(session_id) => session_id, + PullDisposition::Queued(session_id) => { + panic!("expected an immediate pull, got queued session {session_id}") + } + } } #[test] @@ -2133,6 +2395,65 @@ mod tests { .is_empty()); } + #[test] + fn immediate_pull_claims_the_branch_with_a_running_marker_session() { + let (store, branch) = setup_branch_store(); + + let session_id = claim_pull(&store, &branch.id); + + let session = store.get_session(&session_id).unwrap().unwrap(); + assert_eq!(session.status, store::SessionStatus::Running); + // The branch link and pipeline kind are what make the pull visible to the + // queue; it creates no artifact, exactly like a push. + assert_eq!(session.branch_id.as_deref(), Some(branch.id.as_str())); + assert_eq!( + session.pipeline.unwrap().kind, + Some(store::PipelineKind::Pull) + ); + assert!(store + .list_commits_for_branch(&branch.id) + .unwrap() + .is_empty()); + assert!(store.has_running_session_for_branch(&branch.id).unwrap()); + } + + #[test] + fn a_pull_in_flight_keeps_other_branch_work_off_the_worktree() { + let (store, branch) = setup_branch_store(); + claim_pull(&store, &branch.id); + + // A commit pipeline or a second git action requested mid-pull has to wait + // rather than race the fast-forward in the same worktree. + assert!(queue_pipeline(&store, &branch.id, PipelineKind::Rebase, None).is_some()); + assert!(queue_push(&store, &branch.id, false).is_some()); + assert!(queue_pull(&store, &branch.id).is_some()); + } + + #[test] + fn finishing_an_immediate_pull_frees_the_branch_and_records_the_failure() { + let (store, branch) = setup_branch_store(); + + let ok = claim_pull(&store, &branch.id); + finish_immediate_pull_session(&store, &ok, None); + let completed = store.get_session(&ok).unwrap().unwrap(); + assert_eq!(completed.status, store::SessionStatus::Completed); + assert!(!store.has_running_session_for_branch(&branch.id).unwrap()); + + let failed_id = claim_pull(&store, &branch.id); + finish_immediate_pull_session( + &store, + &failed_id, + Some("Cannot pull with uncommitted changes"), + ); + let failed = store.get_session(&failed_id).unwrap().unwrap(); + assert_eq!(failed.status, store::SessionStatus::Error); + assert_eq!( + failed.error_message.as_deref(), + Some("Cannot pull with uncommitted changes") + ); + assert!(!store.has_running_session_for_branch(&branch.id).unwrap()); + } + #[test] fn busy_branch_queues_pull_with_branch_link_and_no_artifact() { let (store, branch) = setup_branch_store(); diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 8a2158525..d53454bbb 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2326,8 +2326,13 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result { diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index 0c92e3fb6..47d9b9de1 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -63,6 +63,7 @@ import { fileNameFromPath, formatBaseBranch, + isGitActionInFlight, isMaybeTextFile, isImageFile, } from './branchCardHelpers'; @@ -329,7 +330,35 @@ ); } let commandPipelinePending = $state(false); - let branchSessionBusy = $derived(timeline ? hasActiveSessions(timeline) : false); + + // Push and pull state is sourced from the global stores so it survives the + // BranchCard remount that happens when the user switches projects and back. + // Both are read up here, above the handlers that use them, because a push or + // pull is branch work `hasActiveSessions` cannot see: neither creates a + // timeline artifact. + let storePushState = $derived(pushStateStore.getPushState(branch.id)); + let storePullState = $derived(pullStateStore.getPullState(branch.id)); + /** A git action of our own is running or waiting on the branch queue. */ + let gitPipelineInFlight = $derived( + isGitActionInFlight({ + push: storePushState, + pull: storePullState, + immediatePull: pullingOrigin, + }) + ); + /** + * True when the branch has work in flight that a new action queues behind. + * + * Folding the git actions in matters for the gates that read this: without them + * the frontend reads a mid-push branch as idle and keeps disabling actions the + * backend would happily queue. The opposite skew — a timeline that still shows a + * session which has just finished — is unavoidable from here; it costs an + * immediate pull that fails with "Cannot pull with uncommitted changes" instead + * of a disabled button. + */ + let branchSessionBusy = $derived( + (timeline ? hasActiveSessions(timeline) : false) || gitPipelineInFlight + ); /** Timeline label the backend gives a queued rebase/squash pipeline session. */ const branchCommandLabels = { rebase: 'Rebase branch', squash: 'Squash commits' } as const; @@ -1018,9 +1047,9 @@ } // A queued pull outlives this component (it drains when the branch frees up), - // so its state lives in the global pullStateStore alongside pushState. The - // immediate pull stays local: it is awaited right here. - let storePullState = $derived(pullStateStore.getPullState(branch.id)); + // so its state lives in the global pullStateStore (read as `storePullState` + // above, alongside pushState). The immediate pull stays local: it is awaited + // right here. /** Pull is waiting on the branch session queue rather than running. */ let pullQueuedOrigin = $derived(storePullState?.state === 'queued'); let pullSessionId = $derived(storePullState?.sessionId ?? null); @@ -1070,6 +1099,49 @@ } } + /** + * Fallback polling for a pull on the branch queue, mirroring the push poller in + * BranchCardPrButton. + * + * A queued pull is drained headless, so `session-status-changed` is the only + * thing that moves it off "Pull queued" and reports a failure. If that event is + * missed the badge sticks until the user clicks Cancel and a failed pull loses + * its toast, so poll the session too. The effect tracks `storePullState`, so + * whichever of the two gets there first tears the other down. + */ + $effect(() => { + const sessionId = storePullState?.sessionId; + if (!sessionId) return; + + const interval = setInterval(async () => { + try { + const session = await commands.getSession(sessionId); + if (session?.status === 'queued') return; + if (session?.status === 'running') { + pullStateStore.markQueuedPullStarted(branch.id, sessionId); + return; + } + pullStateStore.clearPullState(branch.id); + if (session?.status === 'error') { + notifyError( + 'Pull failed', + session.errorMessage ?? 'The queued pull could not fast-forward this branch.' + ); + } + commands.invalidateBranchTimeline(branch.id); + await loadTimeline(); + } catch (e) { + console.error( + `[BranchCard] Lost track of pull session ${sessionId} for branch ${branch.id}:`, + e + ); + pullStateStore.clearPullState(branch.id); + } + }, 5_000); + + return () => clearInterval(interval); + }); + function formatCommitCount(count: number, noun = 'commit'): string { return `${count} ${noun}${count === 1 ? '' : 's'}`; } @@ -1112,12 +1184,9 @@ } } - // Push state is sourced from the global pushStateStore so it survives the - // BranchCard remount that happens when the user switches projects and back. - // The store is shared with BranchCardPrButton (single entry per branch.id), - // updated by the global sessionStatusListener on completion, and covered by - // a 5s polling fallback in BranchCardPrButton. - let storePushState = $derived(pushStateStore.getPushState(branch.id)); + // `storePushState` (read above) is shared with BranchCardPrButton (single entry + // per branch.id), updated by the global sessionStatusListener on completion, and + // covered by a 5s polling fallback in BranchCardPrButton. let pushingOrigin = $derived(storePushState?.state === 'pushing'); /** Push is waiting on the branch session queue rather than running. */ let pushQueuedOrigin = $derived(storePushState?.state === 'queued'); diff --git a/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts b/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts index c8313f0fa..b3e6d4173 100644 --- a/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts +++ b/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts @@ -4,6 +4,7 @@ import { classifyPipelinePushCompletion, extractPrNumber, extractPrUrl, + isGitActionInFlight, isImageFile, isMaybeTextFile, isPushRejectedNonFastForward, @@ -212,3 +213,24 @@ describe('classifyPipelinePushCompletion', () => { ).toBe('succeeded'); }); }); + +describe('isGitActionInFlight', () => { + it('reports a push or pull that is running or waiting on the branch queue', () => { + expect(isGitActionInFlight({ push: { state: 'pushing' } })).toBe(true); + expect(isGitActionInFlight({ push: { state: 'queued' } })).toBe(true); + expect(isGitActionInFlight({ pull: { state: 'pulling' } })).toBe(true); + expect(isGitActionInFlight({ pull: { state: 'queued' } })).toBe(true); + expect(isGitActionInFlight({ immediatePull: true })).toBe(true); + }); + + it('ignores finished push state, which no longer blocks anything', () => { + expect(isGitActionInFlight({ push: { state: 'done' } })).toBe(false); + expect(isGitActionInFlight({ push: { state: 'error' } })).toBe(false); + expect(isGitActionInFlight({ push: { state: 'idle' } })).toBe(false); + }); + + it('reports an idle branch when neither store has an entry', () => { + expect(isGitActionInFlight({})).toBe(false); + expect(isGitActionInFlight({ push: null, pull: null, immediatePull: false })).toBe(false); + }); +}); diff --git a/apps/staged/src/lib/features/branches/branchCardHelpers.ts b/apps/staged/src/lib/features/branches/branchCardHelpers.ts index 041a1c247..d9d80cc8c 100644 --- a/apps/staged/src/lib/features/branches/branchCardHelpers.ts +++ b/apps/staged/src/lib/features/branches/branchCardHelpers.ts @@ -1,8 +1,32 @@ import type { ProjectAction } from '../../api/commands'; +import type { PullState } from '../../stores/pullState.svelte'; +import type { PushState } from '../../stores/pushState.svelte'; import type { PipelineExecution } from '../../types'; const IMAGE_EXTENSIONS = ['.png', '.jpg', '.jpeg', '.gif', '.webp']; +/** + * Whether a push or pull of the branch's own is running or waiting on the branch + * session queue. + * + * Neither creates a timeline artifact, so a timeline-derived "has active sessions" + * check reads the branch as idle for their whole duration. That skew matters for + * the gates that ask whether the backend would queue a new action: mid-push, an + * action they only disable for an *immediate* run (Pull on a dirty worktree) would + * otherwise stay disabled even though the click would just queue. + */ +export function isGitActionInFlight(args: { + push?: { state: PushState } | null; + pull?: { state: PullState } | null; + /** An immediate pull the card is awaiting inline; it has no store entry. */ + immediatePull?: boolean; +}): boolean { + if (args.immediatePull) return true; + const push = args.push?.state; + const pull = args.pull?.state; + return push === 'pushing' || push === 'queued' || pull === 'pulling' || pull === 'queued'; +} + export function groupActionsByType(actions: ProjectAction[]): Record { const groups: Record = { prerun: [], diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 5f5fde30e..41de7a37a 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -372,6 +372,14 @@ * clears before the pull drains — so the requirement is dropped there rather * than disabling an action the backend would happily queue. * + * `branchSessionBusy` is the frontend's read of that, and it can disagree with + * the lock-held decision the backend makes on the click: on a timeline that + * still shows a session which has just finished, the pull runs immediately + * against the dirty worktree and fails with "Cannot pull with uncommitted + * changes" rather than being disabled up front. That is the failure mode this + * trade accepts; the reverse skew is covered by BranchCard folding in-flight + * push/pull sessions (which have no timeline artifact) into the signal. + * * Detached HEAD, the wrong branch, and a diverged upstream stay hard disables: * none of them resolve by waiting. */ diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index 4d484d728..49e73b68a 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -493,10 +493,14 @@ export interface BranchSessionResponse { } /** - * Result of a branch git pipeline command (rebase, squash). + * Result of a branch git pipeline command (rebase, squash, push, force push). * * These can be requested while the branch already has sessions in flight, in * which case the backend queues them and reports `'queued'`. + * + * Pull is the odd one out: an idle branch fast-forwards without going through the + * pipeline runner, so `pullOrQueueBranch` returns `string | null` — the queued + * session id, or `null` for a pull that already happened. */ export interface BranchPipelineResponse { sessionId: string; From 31dd5f0f46809da6413fcd2c5addb14601ceb2f8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 10:29:23 +1000 Subject: [PATCH 5/9] fix(staged): don't leak the pull marker session when the pull task dies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review d8635210 on the run-now race fixes: the immediate-pull path joined its spawn_blocking task with `?`, so a JoinError (the pull closure panicking, or runtime shutdown) returned before finish_immediate_pull_session ran. The marker session is exactly what makes the branch look busy and nothing else ever ends it, so every later git action and commit session on the branch would queue behind a session that never finishes, until an app restart's owner_pid recovery cleared it. The join result now folds into the pull's own error path with unwrap_or_else, so both arms — a pull that ran and a task that died — flow through finish_immediate_pull_session (which records the error and frees the branch) and the queue drain, and the caller still gets the failure as the command's error. No new test: constructing the JoinError requires driving the real async runtime plus an AppHandle for the drain, which the unit tests don't build, and the behavior the fold guarantees (finishing the marker frees the branch and records the message) is already pinned by finishing_an_immediate_pull_frees_the_branch_and_records_the_failure. `just fmt-check`, `just lint`, and `just test` (590) pass. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/prs.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/staged/src-tauri/src/prs.rs b/apps/staged/src-tauri/src/prs.rs index f8a78ce77..f747761b0 100644 --- a/apps/staged/src-tauri/src/prs.rs +++ b/apps/staged/src-tauri/src/prs.rs @@ -1131,11 +1131,14 @@ pub(crate) async fn pull_or_queue_branch_for_branch( let pull_store = Arc::clone(&store); let pull_branch_id = branch_id.clone(); + // A join failure (the pull task panicking, or runtime shutdown) folds into the + // pull's own error path rather than returning early: the marker session below + // is what marks the branch busy, and nothing else ever finishes it. let pulled = tauri::async_runtime::spawn_blocking(move || { crate::timeline::pull_branch_ff_only_impl(&pull_store, &pull_branch_id) }) .await - .map_err(|e| format!("Pull task failed: {e}"))?; + .unwrap_or_else(|e| Err(format!("Pull task failed: {e}"))); finish_immediate_pull_session( &store, From 163f287a9e518dbaa8dbf8603bf2cc4c75397458 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 11:34:01 +1000 Subject: [PATCH 6/9] fix(staged): keep a queued pull visible after the branch diverges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's second PR #902 comment (note ae57c7a6): the "Pull queued" badge and its Cancel button rendered only inside the `originAhead` arm of the upstream switch, but the queued session they describe is relation-independent. Queue a pull while origin is ahead, let the session ahead of it in the branch queue land a local commit, and the relation flips to `diverged` — the `git-origin-ahead` row disappears and takes the badge and the only Cancel affordance with it. The session still drains, into a `merge --ff-only` that cannot fast-forward, so the user gets a failure toast for work they could no longer see or call off. A queued pull now gets a footer row of its own whenever the relation isn't `originAhead`: same `git-footer` slot and order the origin-ahead row occupied, titled "Pull from origin" with the "Pull queued" meta and the Cancel button wired to `onCancelQueuedPull`. The `originAhead` arm is unchanged, so exactly one of the two renders. The decision is `standaloneQueuedPullRowCopy` in a new `queuedPullRow.ts`, which also covers `inSync`/`localAhead`/`missing` (no upstream row at all) and a null relation. The row is therefore built outside the `if (timeline.gitState)` block in the item derivation rather than inside `gitStateRows`, so a timeline that comes back without a git state — an unprovisioned or removed worktree — still leaves the queued session cancellable. That required lifting the `git-footer` timestamp to a `gitFooterTimestamp` const shared by both. Scope note: a *running* drained pull is still only surfaced on the origin-ahead row, so it can also go unseen on a diverged branch. Left alone deliberately — it is transient and ends in a toast either way, and covering it would flash a footer row every time an immediate pull succeeds (the timeline reloads to `inSync` before `pullingOrigin` clears). Frontend only; no backend, data-model, or store changes. Five `queuedPullRow.test.ts` cases pin which relations get the standalone row. `pnpm test` (492), `pnpm run check`, and `prettier --check` pass. Signed-off-by: Matt Toohey --- .../features/timeline/BranchTimeline.svelte | 42 ++++++++++++++++++- .../features/timeline/queuedPullRow.test.ts | 31 ++++++++++++++ .../lib/features/timeline/queuedPullRow.ts | 30 +++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 apps/staged/src/lib/features/timeline/queuedPullRow.test.ts create mode 100644 apps/staged/src/lib/features/timeline/queuedPullRow.ts diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 41de7a37a..b4352431b 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -19,6 +19,7 @@ BranchGitState, BranchTimeline as BranchTimelineData, HashtagItem, + UpstreamRelation, } from '../../types'; import type { NoteClickInfo } from '../sessions/noteFreshness'; import TimelineRow from './TimelineRow.svelte'; @@ -40,6 +41,7 @@ type PendingHintItemType, } from './liveSessionHints'; import { isEmptyFailedReview } from './reviewState'; + import { standaloneQueuedPullRowCopy } from './queuedPullRow'; import { failedArtifactSubtitle } from './sessionFailureCopy'; import { stripXmlTags } from '../sessions/sessionModalHelpers'; @@ -345,6 +347,9 @@ liveSessionHintPoller.destroy(); }); + /** Timestamp that sorts the git status rows below every timeline artifact. */ + const gitFooterTimestamp = Number.MAX_SAFE_INTEGER - 1000; + function plural(count: number, noun: string): string { return `${count} ${noun}${count === 1 ? '' : 's'}`; } @@ -418,7 +423,7 @@ ): DisplayItem[] { const rows: DisplayItem[] = []; const topTimestamp = 0; - const bottomTimestamp = Number.MAX_SAFE_INTEGER - 1000; + const bottomTimestamp = gitFooterTimestamp; const commitChangesDisabledReason = gitActionDisabledReason ? gitActionDisabledReason : newSessionDisabled @@ -470,7 +475,9 @@ } break; case 'originAhead': { - // A queued pull keeps the button live so it can cancel the queued session. + // A queued pull keeps the button live so it can cancel the queued + // session. If the relation moves off `originAhead` while the pull is + // still queued, the standalone row below takes over. const disabledReason = pullQueuedOrigin ? undefined : pullDisabledReason(state); rows.push({ key: 'git-origin-ahead', @@ -570,6 +577,32 @@ return rows; } + /** + * Footer row for a queued pull the upstream rows above can no longer host. + * + * A pull is only startable while origin is ahead, so that row owns the badge + * and the Cancel button — but the queued session outlives the relation it was + * created in: whatever sits ahead of it in the branch queue can land a local + * commit before the pull drains. This row takes over in the slot the + * origin-ahead row occupied, so the queued pull stays visible and cancellable + * instead of draining into a surprise `merge --ff-only` failure. + */ + function queuedPullFooterRow(relation: UpstreamRelation | null): DisplayItem | null { + const copy = standaloneQueuedPullRowCopy({ pullQueued: pullQueuedOrigin, relation }); + if (!copy) return null; + return { + key: 'git-queued-pull', + type: 'git-pull', + title: copy.title, + meta: copy.meta, + timestamp: gitFooterTimestamp, + order: 1, + placement: 'git-footer', + onPull: onCancelQueuedPull, + pullQueued: true, + }; + } + // Merge commits, notes, and reviews into a single sorted list let items = $derived.by(() => { const nowMs = minuteNow.now(); @@ -792,6 +825,11 @@ all.push(...gitStateRows(timeline.gitState, commitAnchors)); } + // Outside the git-state block: a queued pull has to stay cancellable even + // when the timeline came back without a git state to hang it on. + const queuedPull = queuedPullFooterRow(timeline.gitState?.upstream.relation ?? null); + if (queuedPull) all.push(queuedPull); + // Provisioning row appears at the very start of the timeline if (provisioningLabel) { all.unshift({ diff --git a/apps/staged/src/lib/features/timeline/queuedPullRow.test.ts b/apps/staged/src/lib/features/timeline/queuedPullRow.test.ts new file mode 100644 index 000000000..4125f0ac3 --- /dev/null +++ b/apps/staged/src/lib/features/timeline/queuedPullRow.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { standaloneQueuedPullRowCopy } from './queuedPullRow'; + +describe('standaloneQueuedPullRowCopy', () => { + it('leaves the badge on the origin-ahead row that already hosts it', () => { + expect(standaloneQueuedPullRowCopy({ pullQueued: true, relation: 'originAhead' })).toBeNull(); + }); + + it('keeps a queued pull visible once the branch diverges', () => { + expect(standaloneQueuedPullRowCopy({ pullQueued: true, relation: 'diverged' })).toEqual({ + title: 'Pull from origin', + meta: 'Pull queued', + }); + }); + + it('keeps a queued pull visible for relations that render no upstream row', () => { + for (const relation of ['inSync', 'localAhead', 'missing'] as const) { + expect(standaloneQueuedPullRowCopy({ pullQueued: true, relation })).not.toBeNull(); + } + }); + + it('keeps a queued pull visible when the timeline has no git state', () => { + expect(standaloneQueuedPullRowCopy({ pullQueued: true, relation: null })).not.toBeNull(); + }); + + it('renders nothing when no pull is queued', () => { + for (const relation of ['originAhead', 'diverged', 'inSync', null] as const) { + expect(standaloneQueuedPullRowCopy({ pullQueued: false, relation })).toBeNull(); + } + }); +}); diff --git a/apps/staged/src/lib/features/timeline/queuedPullRow.ts b/apps/staged/src/lib/features/timeline/queuedPullRow.ts new file mode 100644 index 000000000..c2db91f24 --- /dev/null +++ b/apps/staged/src/lib/features/timeline/queuedPullRow.ts @@ -0,0 +1,30 @@ +import type { UpstreamRelation } from '../../types'; + +/** Title and badge for a queued pull rendered on a row of its own. */ +export type QueuedPullRowCopy = { title: string; meta: string }; + +/** + * Copy for a standalone queued-pull footer row, or null when the upstream + * relation already has a row to host the badge. + * + * The "Pull queued" badge and its Cancel button normally ride on the + * `originAhead` row, since that is the only relation Pull can be started from. + * But a queued pull outlives that relation: it drains whenever the branch frees + * up, and a session ahead of it in the queue can land a local commit first, + * flipping the branch to `diverged` — or to `inSync`/`localAhead`, neither of + * which renders an upstream row at all. The queued session itself is + * relation-independent, so without a row of its own it becomes invisible and + * uncancellable until it drains, which on a diverged branch means a + * `merge --ff-only` failure the user can no longer call off. + * + * `null` relation means the timeline came back without a git state at all (an + * unprovisioned or removed worktree); the queued session still exists, so it + * still gets a row. + */ +export function standaloneQueuedPullRowCopy(args: { + pullQueued: boolean; + relation: UpstreamRelation | null; +}): QueuedPullRowCopy | null { + if (!args.pullQueued || args.relation === 'originAhead') return null; + return { title: 'Pull from origin', meta: 'Pull queued' }; +} From 744d2710574898cbe771ff0a15f960c8e1e22fc1 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 11:58:00 +1000 Subject: [PATCH 7/9] fix(staged): tolerate transient session-poll failures instead of dropping the badge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 5af0ac18 (note ad5e8094): the queued-pull poller in BranchCard treated any `getSession` rejection as "session lost" and cleared the pullStateStore entry on the first failure. Since the backend command returns `Ok(None)` for an unknown or deleted session, a rejection is only ever a transport or dispatch failure — a web-mode network blip, the backend restarting, a laptop waking from sleep. So one blip permanently removed the "Pull queued" badge and its Cancel button while the backend still held the queued session, which then drained invisibly; the completion toast survived only via the `session-status-changed` event that this poller exists to back up. `createPollFailureTracker` in branchCardHelpers gives a poller a budget of consecutive failures (3 by default, so ~15s at the 5s cadence) that a success resets. Below the budget the catch arm warns and keeps polling; once exhausted it does exactly what it did before. Extracted rather than an inline counter per effect so the three call sites stay consistent and the reset-on-success decision is testable. "Never clear" was rejected: a genuinely unreachable backend would leave the badge up forever with a Cancel button that cannot work. Applied to the push and PR-creation pollers in BranchCardPrButton too. The review suggested matching the push poller, describing it as logging and continuing, but it also gives up on the first failure — and worse, `setPushError` flips the button to "Push failed" for a push that is still queued. Fixing all three is what actually removes the inconsistency the comment was pointing at. A session cancelled elsewhere still clears promptly: that is the `null` success path, untouched. Effect re-runs get a fresh tracker, which is correct — the budget is per polling episode. Out of scope (its own fix): the review's second comment, that `cancelQueuedPull`/`cancelQueuedPush` clear the store before `cancelSession` resolves, so a failed cancel loses the affordance. Tests: four `branchCardHelpers.test.ts` cases pin the budget, the reset-on-success semantics, a custom `maxFailures`, and the default. The `$effect` intervals stay untested, as they were. Frontend-only; no backend, store, or data-model changes. `pnpm test` (496), `pnpm run check`, and `prettier --check` pass. Signed-off-by: Matt Toohey --- .../lib/features/branches/BranchCard.svelte | 15 ++++++++ .../branches/BranchCardPrButton.svelte | 29 +++++++++++++-- .../branches/branchCardHelpers.test.ts | 36 +++++++++++++++++++ .../features/branches/branchCardHelpers.ts | 31 ++++++++++++++++ 4 files changed, 108 insertions(+), 3 deletions(-) diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index 47d9b9de1..b5eb22551 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -61,6 +61,7 @@ import * as AlertDialog from '$lib/components/ui/alert-dialog'; import { Button } from '$lib/components/ui/button'; import { + createPollFailureTracker, fileNameFromPath, formatBaseBranch, isGitActionInFlight, @@ -1108,14 +1109,21 @@ * missed the badge sticks until the user clicks Cancel and a failed pull loses * its toast, so poll the session too. The effect tracks `storePullState`, so * whichever of the two gets there first tears the other down. + * + * A cancelled or deleted session comes back as `null` from `getSession`, so a + * rejection only ever means the backend was unreachable. Those are tolerated for + * a few consecutive attempts — dropping the badge on the first blip would strand + * a still-queued pull with no Cancel button and no signal to re-click. */ $effect(() => { const sessionId = storePullState?.sessionId; if (!sessionId) return; + const failures = createPollFailureTracker(); const interval = setInterval(async () => { try { const session = await commands.getSession(sessionId); + failures.recordSuccess(); if (session?.status === 'queued') return; if (session?.status === 'running') { pullStateStore.markQueuedPullStarted(branch.id, sessionId); @@ -1131,6 +1139,13 @@ commands.invalidateBranchTimeline(branch.id); await loadTimeline(); } catch (e) { + if (!failures.recordFailure()) { + console.warn( + `[BranchCard] Could not poll pull session ${sessionId} for branch ${branch.id}, retrying:`, + e + ); + return; + } console.error( `[BranchCard] Lost track of pull session ${sessionId} for branch ${branch.id}:`, e diff --git a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte index 74708cabb..f5bffe220 100644 --- a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte @@ -26,6 +26,7 @@ import { classifyCompletedPushSession, classifyPipelinePushCompletion, + createPollFailureTracker, extractPrNumber, extractPrUrl, type CompletedPushOutcome, @@ -210,18 +211,29 @@ }; }); - // Fallback polling for PR session + // Fallback polling for PR session. A rejection means the backend was + // unreachable (a missing session resolves to `null`), so give it a few + // consecutive attempts before declaring the session lost. $effect(() => { if (prState !== 'creating' || !prSessionId) return; const sid = prSessionId; + const failures = createPollFailureTracker(); const interval = setInterval(async () => { try { const session = await commands.getSession(sid); + failures.recordSuccess(); if (session && session.status !== 'running') { handlePrSessionComplete(session.status); } - } catch { + } catch (err) { + if (!failures.recordFailure()) { + console.warn( + `[BranchCardPrButton] Could not poll PR session ${sid} for branch ${branch.id}, retrying:`, + err + ); + return; + } prStateStore.setPrError(branch.id, 'Lost track of PR creation session.'); prStateStore.clearSessionTracking(branch.id); } @@ -232,14 +244,18 @@ // Fallback polling for push session. Queued pushes are polled too: if the // branch queue drains one while the "running" event is missed, this is what - // moves the button off "Queued". + // moves the button off "Queued". Transient poll failures are tolerated for a + // few attempts, so a single blip can't flip a still-queued push to "Push + // failed". $effect(() => { if ((pushState !== 'pushing' && pushState !== 'queued') || !pushSessionId) return; const sid = pushSessionId; + const failures = createPollFailureTracker(); const interval = setInterval(async () => { try { const session = await commands.getSession(sid); + failures.recordSuccess(); if (session?.status === 'queued') return; if (session?.status === 'running') { pushStateStore.markQueuedPushStarted(branch.id, sid); @@ -249,6 +265,13 @@ handlePushSessionComplete(session.status, session); } } catch (err) { + if (!failures.recordFailure()) { + console.warn( + `[BranchCardPrButton] Could not poll push session ${sid} for branch ${branch.id}, retrying:`, + err + ); + return; + } console.error( `[BranchCardPrButton] Lost track of push session ${sid} for branch ${branch.id}:`, err diff --git a/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts b/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts index b3e6d4173..371b55205 100644 --- a/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts +++ b/apps/staged/src/lib/features/branches/branchCardHelpers.test.ts @@ -2,12 +2,14 @@ import { describe, expect, it } from 'vitest'; import { classifyCompletedPushSession, classifyPipelinePushCompletion, + createPollFailureTracker, extractPrNumber, extractPrUrl, isGitActionInFlight, isImageFile, isMaybeTextFile, isPushRejectedNonFastForward, + MAX_CONSECUTIVE_POLL_FAILURES, } from './branchCardHelpers'; import type { PipelineExecution } from '../../types'; @@ -234,3 +236,37 @@ describe('isGitActionInFlight', () => { expect(isGitActionInFlight({ push: null, pull: null, immediatePull: false })).toBe(false); }); }); + +describe('createPollFailureTracker', () => { + it('tolerates failures until the budget is exhausted', () => { + const tracker = createPollFailureTracker(3); + expect(tracker.recordFailure()).toBe(false); + expect(tracker.recordFailure()).toBe(false); + expect(tracker.recordFailure()).toBe(true); + }); + + it('resets the count on a success, so intermittent failures never give up', () => { + const tracker = createPollFailureTracker(3); + for (let attempt = 0; attempt < 10; attempt += 1) { + expect(tracker.recordFailure()).toBe(false); + expect(tracker.recordFailure()).toBe(false); + tracker.recordSuccess(); + } + }); + + it('honors a custom failure budget', () => { + expect(createPollFailureTracker(1).recordFailure()).toBe(true); + + const patient = createPollFailureTracker(5); + expect([1, 2, 3, 4].map(() => patient.recordFailure())).toEqual([false, false, false, false]); + expect(patient.recordFailure()).toBe(true); + }); + + it('defaults to the shared consecutive-failure budget', () => { + const tracker = createPollFailureTracker(); + for (let attempt = 1; attempt < MAX_CONSECUTIVE_POLL_FAILURES; attempt += 1) { + expect(tracker.recordFailure()).toBe(false); + } + expect(tracker.recordFailure()).toBe(true); + }); +}); diff --git a/apps/staged/src/lib/features/branches/branchCardHelpers.ts b/apps/staged/src/lib/features/branches/branchCardHelpers.ts index d9d80cc8c..32772f52f 100644 --- a/apps/staged/src/lib/features/branches/branchCardHelpers.ts +++ b/apps/staged/src/lib/features/branches/branchCardHelpers.ts @@ -27,6 +27,37 @@ export function isGitActionInFlight(args: { return push === 'pushing' || push === 'queued' || pull === 'pulling' || pull === 'queued'; } +export const MAX_CONSECUTIVE_POLL_FAILURES = 3; + +export type PollFailureTracker = { + recordSuccess(): void; + /** Records one failure; true when the budget is exhausted and the poller should give up. */ + recordFailure(): boolean; +}; + +/** + * Failure budget for a session poller. + * + * `getSession` resolves `null` for a session that no longer exists, so a rejection + * is always a transport or dispatch failure — the kind that a retry fixes. Tolerate + * those until several happen in a row; a success resets the budget, so only sustained + * unreachability, not an isolated blip, makes a poller give up its store entry. + */ +export function createPollFailureTracker( + maxFailures = MAX_CONSECUTIVE_POLL_FAILURES +): PollFailureTracker { + let consecutive = 0; + return { + recordSuccess(): void { + consecutive = 0; + }, + recordFailure(): boolean { + consecutive += 1; + return consecutive >= maxFailures; + }, + }; +} + export function groupActionsByType(actions: ProjectAction[]): Record { const groups: Record = { prerun: [], From 8b7fcba517e0aaf07ace26cb2fe2690bada7eded Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 13:07:41 +1000 Subject: [PATCH 8/9] fix(staged): clear the queued push/pull badge only after the cancel confirms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review dc6c3fa5, and the gap commit 744d2710 explicitly deferred: all three "cancel a queued git action" handlers cleared the frontend store entry *before* `cancelSession` resolved. `cancel_session` answers `Ok` for an unknown, already-finished, or already-cancelled session, so a rejection only ever means the request never reached the backend — a web-mode network blip, the backend restarting, a laptop waking — which is exactly when the queued session still exists. The badge and its Cancel button were gone anyway, so the push/pull went on to drain invisibly, potentially into the `merge --ff-only` failure the standalone queued-pull row was added to surface. `createQueuedSessionCanceller` in branchCardHelpers inverts the order: await the cancel, clear on success, and on failure report the error and keep the state so the affordance survives and re-clicking retries. It also carries an in-flight flag, because leaving the state set means the call sites' own `pullQueuedOrigin` / `pushState !== 'queued'` guards no longer stop a second click. Extracted rather than inlined three times, following `createPollFailureTracker`, so the ordering decision is testable and the sites stay consistent. BranchCard's `cancelQueuedPull` and `cancelQueuedPush` now delegate and only invalidate/reload the timeline on success. BranchCardPrButton's `cancelQueuedPush` additionally drops `setPushError` for a toast: on a failed cancel it flipped the PR button to "Push failed" for a push that was still queued, replacing Cancel with a retry dialog. Races considered, no further changes: both clears are idempotent `Map.delete`s, so the 5s fallback poller racing the helper is harmless, and a genuinely unreachable backend still clears the badge via the poller's 3-failure budget with the cancel toast explaining why. A drain that flips the entry to pulling/pushing before the click no-ops the existing guards. Frontend-only: two components plus branchCardHelpers. No backend, store-shape, or data-model changes. Four `branchCardHelpers.test.ts` cases pin the clear-after-confirm ordering, state survival on rejection, re-entrancy, and retry-after-failure. `pnpm test` (512), `pnpm run check`, and `prettier --check` pass. Signed-off-by: Matt Toohey --- .../lib/features/branches/BranchCard.svelte | 31 +++++--- .../branches/BranchCardPrButton.svelte | 25 +++++-- .../branches/branchCardHelpers.test.ts | 75 ++++++++++++++++++- .../features/branches/branchCardHelpers.ts | 38 ++++++++++ 4 files changed, 151 insertions(+), 18 deletions(-) diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index b5eb22551..d51271712 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -62,6 +62,7 @@ import { Button } from '$lib/components/ui/button'; import { createPollFailureTracker, + createQueuedSessionCanceller, fileNameFromPath, formatBaseBranch, isGitActionInFlight, @@ -1085,18 +1086,21 @@ * * The store entry is cleared here rather than by the completion listener: a * session that never ran isn't in the session registry, so its cancellation - * event carries no pull session type to match on. + * event carries no pull session type to match on. It waits for the backend to + * confirm the cancellation — see `createQueuedSessionCanceller`. */ + const runCancelQueuedPull = createQueuedSessionCanceller({ + cancel: (sessionId) => commands.cancelSession(sessionId), + clearState: () => pullStateStore.clearPullState(branch.id), + onError: (e) => notifyError('Could not cancel queued pull', e), + }); + async function cancelQueuedPull() { const sessionId = pullSessionId; if (!pullQueuedOrigin || !sessionId) return; - pullStateStore.clearPullState(branch.id); - try { - await commands.cancelSession(sessionId); + if (await runCancelQueuedPull(sessionId)) { commands.invalidateBranchTimeline(branch.id); await loadTimeline(); - } catch (e) { - notifyError('Could not cancel queued pull', e); } } @@ -1231,18 +1235,21 @@ * * The store entry is cleared here rather than by the completion listener: a * session that never ran isn't in the session registry, so its cancellation - * event carries no push session type to match on. + * event carries no push session type to match on. It waits for the backend to + * confirm the cancellation — see `createQueuedSessionCanceller`. */ + const runCancelQueuedPush = createQueuedSessionCanceller({ + cancel: (sessionId) => commands.cancelSession(sessionId), + clearState: () => pushStateStore.clearPushState(branch.id), + onError: (e) => notifyError('Could not cancel queued push', e), + }); + async function cancelQueuedPush() { const sessionId = pushSessionId; if (!pushQueuedOrigin || !sessionId || sessionId === '__pending__') return; - pushStateStore.clearPushState(branch.id); - try { - await commands.cancelSession(sessionId); + if (await runCancelQueuedPush(sessionId)) { commands.invalidateBranchTimeline(branch.id); await loadTimeline(); - } catch (e) { - notifyError('Could not cancel queued push', e); } } diff --git a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte index f5bffe220..7b0b6f936 100644 --- a/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte +++ b/apps/staged/src/lib/features/branches/BranchCardPrButton.svelte @@ -6,6 +6,7 @@ -->