From 5c11252cf18f769e1f86c28b09efb78bc0b8849e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 14:19:43 +1000 Subject: [PATCH 1/8] feat(sessions): reassociate commits with their sessions after a rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rebase gives every commit on the branch a new SHA, which orphaned each `commits` row keyed by the old one: the timeline lost the authoring session, the head commit got mis-attributed to the mechanical "Rebase branch" session, and reviews keyed by `commit_sha` disappeared behind `review_is_visible_in_timeline`. The mapping is recoverable from the DB plus git alone, with no pre-rebase capture — `git rebase` preserves author email, author date, and subject, and the old objects stay resolvable — so the reconciler also survives an app restart mid-pipeline. New `commit_reassociation` module reads the old identities back from the orphaned SHAs, pairs them against the rewritten commits (oldest-to-oldest when identities collide, skipping SHAs a row already owns), and `Store::remap_commit_shas` repoints the rows and their reviews in one transaction. Both rebase completion paths call it before the pending row is resolved, so the existing "target SHA already owned" branch drops the duplicate pending row instead of stealing the head commit. Covered by matcher unit tests, store tests for the remap and its collision guard, and integration tests driving a real `git rebase --signoff` through each completion path. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/branches.rs | 2 +- .../src-tauri/src/commit_reassociation.rs | 393 ++++++++++++++++++ apps/staged/src-tauri/src/lib.rs | 1 + apps/staged/src-tauri/src/session_runner.rs | 212 +++++++++- apps/staged/src-tauri/src/store/commits.rs | 52 +++ apps/staged/src-tauri/src/store/tests.rs | 69 +++ 6 files changed, 727 insertions(+), 2 deletions(-) create mode 100644 apps/staged/src-tauri/src/commit_reassociation.rs diff --git a/apps/staged/src-tauri/src/branches.rs b/apps/staged/src-tauri/src/branches.rs index 9330d8f96..c4d29ca3a 100644 --- a/apps/staged/src-tauri/src/branches.rs +++ b/apps/staged/src-tauri/src/branches.rs @@ -347,7 +347,7 @@ pub(crate) fn resolve_branch_clone_dir( } pub(crate) fn resolve_branch_workspace_subpath( - store: &Arc, + store: &Store, branch: &store::Branch, ) -> Result, String> { let Some(repo_id) = branch.project_repo_id.as_deref() else { diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs new file mode 100644 index 000000000..5f1b334dd --- /dev/null +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -0,0 +1,393 @@ +//! Reattach commit metadata to rewritten SHAs after a rebase. +//! +//! Staged links each commit to the session that authored it by SHA. A rebase +//! gives every commit on the branch a new SHA, which orphans every one of +//! those rows: the timeline's SHA lookup misses, the commits lose their +//! session, and their reviews get hidden by `review_is_visible_in_timeline`. +//! +//! The mapping is recoverable from the DB plus git alone — no pre-rebase +//! capture, so this also survives an app restart mid-pipeline. `git rebase` +//! preserves author email, author date, and subject; only the SHA and the +//! committer fields change (conflict resolution doesn't touch author metadata, +//! and `--signoff` only appends a body trailer). The orphaned rows still hold +//! the old SHAs, and the old commit objects stay resolvable in the repo, so we +//! can read the old metadata back and match it against the rewritten commits. + +use std::collections::{HashMap, HashSet, VecDeque}; +use std::path::Path; + +use crate::store::Store; + +/// The commit metadata `git rebase` carries across a rewrite. Two commits with +/// the same identity are the same commit before and after a rebase. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct CommitIdentity { + pub author_email: String, + /// Author date as git's raw `%at` (unix seconds), compared as text. + pub author_timestamp: String, + pub subject: String, +} + +/// A `commits` row whose SHA is no longer on the branch. +#[derive(Debug, Clone)] +pub struct OrphanedRow { + pub row_id: String, + pub old_sha: String, + pub identity: CommitIdentity, +} + +/// A commit currently on the branch, as a candidate for an orphaned row. +#[derive(Debug, Clone)] +pub struct RewrittenCommit { + pub sha: String, + pub identity: CommitIdentity, + /// Whether a `commits` row already owns this SHA. Claimed commits are + /// never handed to an orphaned row — the existing row wins. + pub claimed: bool, +} + +/// A row to repoint, produced by [`match_rewritten_commits`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ShaRemap { + pub row_id: String, + pub old_sha: String, + pub new_sha: String, +} + +/// `%H|%ae|%at|%s` — subject last, since it's the only field that can contain +/// the delimiter. Deliberately not `CommitInfo`'s format, which carries `%ct` +/// (committer time), the one timestamp a rebase rewrites. +const REASSOCIATION_LOG_FORMAT: &str = "--format=%H|%ae|%at|%s"; + +/// Pair orphaned rows with the commits that replaced them. +/// +/// Both inputs must be in branch order, oldest first: when several commits +/// share an identity (two `wip` commits authored in the same second, say), +/// they're paired oldest-to-oldest. Unmatched rows are simply left out — the +/// caller leaves them orphaned, which is what happens today anyway. +pub fn match_rewritten_commits( + orphans: &[OrphanedRow], + rewritten: &[RewrittenCommit], +) -> Vec { + let mut available: HashMap<&CommitIdentity, VecDeque<&str>> = HashMap::new(); + for commit in rewritten.iter().filter(|c| !c.claimed) { + available + .entry(&commit.identity) + .or_default() + .push_back(&commit.sha); + } + + let mut remaps = Vec::new(); + for orphan in orphans { + let Some(candidates) = available.get_mut(&orphan.identity) else { + continue; + }; + let Some(new_sha) = candidates.pop_front() else { + continue; + }; + remaps.push(ShaRemap { + row_id: orphan.row_id.clone(), + old_sha: orphan.old_sha.clone(), + new_sha: new_sha.to_string(), + }); + } + remaps +} + +/// Repoint a branch's orphaned commit rows (and their reviews) at the SHAs a +/// rebase rewrote them into. Returns how many rows were remapped. +/// +/// Safe to call when nothing was rewritten: with no orphaned rows it stops +/// after listing the branch and returns 0. +pub fn reassociate_after_rebase( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) -> Result { + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + + let repo_subpath = match workspace_name { + Some(_) => crate::branches::resolve_branch_workspace_subpath(store, &branch)?, + None => None, + }; + let git = |args: &[&str]| -> Result { + match workspace_name { + Some(ws_name) => { + crate::branches::run_workspace_git(ws_name, repo_subpath.as_deref(), args) + .map_err(|e| e.to_string()) + } + None => crate::git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), + } + }; + + let base_ref = crate::git::origin_ref_for_branch(&branch.base_branch); + let on_branch = list_branch_commits(&git, &base_ref)?; + + // `list_commits_for_branch` orders by `created_at`, which is the order the + // sessions authored them — i.e. branch order, as the matcher requires. + let rows = store + .list_commits_for_branch(branch_id) + .map_err(|e| e.to_string())?; + let owned: HashSet<&str> = rows.iter().filter_map(|row| row.sha.as_deref()).collect(); + + let rewritten: Vec = on_branch + .into_iter() + .map(|(sha, identity)| RewrittenCommit { + claimed: owned.contains(sha.as_str()), + sha, + identity, + }) + .collect(); + + let still_on_branch: HashSet<&str> = rewritten.iter().map(|c| c.sha.as_str()).collect(); + let orphan_shas: Vec = owned + .iter() + .filter(|sha| !still_on_branch.contains(*sha)) + .map(|sha| (*sha).to_string()) + .collect(); + if orphan_shas.is_empty() { + return Ok(0); + } + + let mut old_identities = lookup_commit_identities(&git, &orphan_shas)?; + let orphans: Vec = rows + .iter() + .filter_map(|row| { + let old_sha = row.sha.clone()?; + // GC-pruned objects drop out here, leaving their row orphaned. + let identity = old_identities.remove(&old_sha)?; + Some(OrphanedRow { + row_id: row.id.clone(), + old_sha, + identity, + }) + }) + .collect(); + + let remaps = match_rewritten_commits(&orphans, &rewritten); + if remaps.is_empty() { + return Ok(0); + } + + let pairs: Vec<(&str, &str, &str)> = remaps + .iter() + .map(|r| (r.row_id.as_str(), r.old_sha.as_str(), r.new_sha.as_str())) + .collect(); + store + .remap_commit_shas(branch_id, &pairs) + .map_err(|e| e.to_string()) +} + +/// List the branch's commits as `(sha, identity)` pairs, oldest first. +fn list_branch_commits(git: &F, base_ref: &str) -> Result, String> +where + F: Fn(&[&str]) -> Result, +{ + // Fall back to the bare base ref the way the timeline does, so a repo + // without a shared history with `origin/{base}` still reports something. + let range = match git(&["merge-base", base_ref, "HEAD"]) { + Ok(output) if !output.trim().is_empty() => format!("{}..HEAD", output.trim()), + _ => format!("{base_ref}..HEAD"), + }; + let output = git(&["log", REASSOCIATION_LOG_FORMAT, &range, "--"])?; + + // `git log` is newest-first; the matcher wants branch order, oldest first. + Ok(output + .lines() + .filter_map(parse_identity_line) + .rev() + .collect()) +} + +/// Batch-read metadata for commits that are no longer on any branch. The old +/// objects survive a rebase (the reflog keeps them alive), and +/// `--ignore-missing` silently drops any that have since been GC-pruned. +fn lookup_commit_identities( + git: &F, + shas: &[String], +) -> Result, String> +where + F: Fn(&[&str]) -> Result, +{ + // Guard the empty case: `git log --no-walk` with no revisions defaults to + // HEAD, which would hand back metadata for a commit nobody asked about. + if shas.is_empty() { + return Ok(HashMap::new()); + } + + let mut args = vec![ + "log", + "--no-walk=unsorted", + "--ignore-missing", + REASSOCIATION_LOG_FORMAT, + ]; + args.extend(shas.iter().map(String::as_str)); + args.push("--"); + let output = git(&args)?; + + Ok(output.lines().filter_map(parse_identity_line).collect()) +} + +/// Parse one `%H|%ae|%at|%s` line. The subject is the remainder, so subjects +/// containing `|` survive intact. +fn parse_identity_line(line: &str) -> Option<(String, CommitIdentity)> { + let mut parts = line.splitn(4, '|'); + let sha = parts.next()?; + let author_email = parts.next()?; + let author_timestamp = parts.next()?; + let subject = parts.next()?; + if sha.is_empty() { + return None; + } + Some(( + sha.to_string(), + CommitIdentity { + author_email: author_email.to_string(), + author_timestamp: author_timestamp.to_string(), + subject: subject.to_string(), + }, + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn identity(email: &str, timestamp: &str, subject: &str) -> CommitIdentity { + CommitIdentity { + author_email: email.to_string(), + author_timestamp: timestamp.to_string(), + subject: subject.to_string(), + } + } + + fn orphan(row_id: &str, old_sha: &str, identity: CommitIdentity) -> OrphanedRow { + OrphanedRow { + row_id: row_id.to_string(), + old_sha: old_sha.to_string(), + identity, + } + } + + fn rewritten(sha: &str, identity: CommitIdentity) -> RewrittenCommit { + RewrittenCommit { + sha: sha.to_string(), + identity, + claimed: false, + } + } + + #[test] + fn matches_rewritten_commits_by_author_identity() { + let parser = identity("a@example.com", "100", "feat: parser"); + let lexer = identity("b@example.com", "200", "fix: lexer"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-1", "abc111", parser.clone()), + orphan("row-2", "abc222", lexer.clone()), + ], + &[rewritten("def444", parser), rewritten("def555", lexer)], + ); + + assert_eq!( + remaps, + vec![ + ShaRemap { + row_id: "row-1".into(), + old_sha: "abc111".into(), + new_sha: "def444".into() + }, + ShaRemap { + row_id: "row-2".into(), + old_sha: "abc222".into(), + new_sha: "def555".into() + }, + ] + ); + } + + /// A conflict-resolved commit has different content but the same author + /// metadata, so it still matches — that's the whole point of the key. + #[test] + fn matches_commit_whose_content_changed_during_conflict_resolution() { + let resolved = identity("a@example.com", "100", "feat: parser"); + let remaps = match_rewritten_commits( + &[orphan("row-1", "abc111", resolved.clone())], + &[rewritten("def444", resolved)], + ); + assert_eq!(remaps.len(), 1); + assert_eq!(remaps[0].new_sha, "def444"); + } + + /// Two commits authored in the same second with the same subject are + /// indistinguishable by key, so they pair up in branch order. + #[test] + fn matches_duplicate_identities_oldest_to_oldest() { + let wip = identity("a@example.com", "100", "wip"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-old", "abc111", wip.clone()), + orphan("row-new", "abc222", wip.clone()), + ], + &[rewritten("def444", wip.clone()), rewritten("def555", wip)], + ); + + assert_eq!(remaps[0].row_id, "row-old"); + assert_eq!(remaps[0].new_sha, "def444"); + assert_eq!(remaps[1].row_id, "row-new"); + assert_eq!(remaps[1].new_sha, "def555"); + } + + /// A commit the rebase dropped (it became empty) has no counterpart; its + /// row stays orphaned rather than stealing a neighbour's SHA. + #[test] + fn leaves_dropped_commit_unmatched() { + let kept = identity("a@example.com", "100", "feat: parser"); + let dropped = identity("a@example.com", "200", "chore: already upstream"); + + let remaps = match_rewritten_commits( + &[ + orphan("row-kept", "abc111", kept.clone()), + orphan("row-dropped", "abc222", dropped), + ], + &[rewritten("def444", kept)], + ); + + assert_eq!(remaps.len(), 1); + assert_eq!(remaps[0].row_id, "row-kept"); + } + + /// A rewritten commit that already has a row of its own is off-limits — + /// e.g. the rebase session's own pending row once it has landed. + #[test] + fn skips_rewritten_commits_that_already_have_a_row() { + let parser = identity("a@example.com", "100", "feat: parser"); + let mut claimed = rewritten("def444", parser.clone()); + claimed.claimed = true; + + let remaps = match_rewritten_commits(&[orphan("row-1", "abc111", parser)], &[claimed]); + + assert!(remaps.is_empty()); + } + + #[test] + fn parses_subject_containing_the_delimiter() { + let (sha, identity) = + parse_identity_line("abc111|a@example.com|100|chore: rename a|b to c").unwrap(); + assert_eq!(sha, "abc111"); + assert_eq!(identity.subject, "chore: rename a|b to c"); + assert_eq!(identity.author_timestamp, "100"); + } + + #[test] + fn ignores_malformed_log_lines() { + assert!(parse_identity_line("").is_none()); + assert!(parse_identity_line("abc111|a@example.com|100").is_none()); + } +} diff --git a/apps/staged/src-tauri/src/lib.rs b/apps/staged/src-tauri/src/lib.rs index 9f9305c04..6ff8a109e 100644 --- a/apps/staged/src-tauri/src/lib.rs +++ b/apps/staged/src-tauri/src/lib.rs @@ -11,6 +11,7 @@ pub mod agent; pub mod background_sync; pub mod blox; pub mod branches; +pub(crate) mod commit_reassociation; pub mod diff_cache; pub mod diff_commands; pub mod doctor; diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 264705d57..b5dda004a 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -35,7 +35,7 @@ use std::collections::HashMap; use std::io; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Output, Stdio}; use std::sync::{Arc, OnceLock}; use std::time::Duration; @@ -1430,6 +1430,44 @@ fn current_pipeline_head(config: &PipelineConfig) -> Result { } } +/// Whether a session's persisted pipeline is a rebase. Read from the session +/// row rather than an in-memory config so it still answers correctly for an AI +/// handoff that outlived the pipeline that started it. +fn session_is_rebase_pipeline(store: &Store, session_id: &str) -> bool { + store + .get_session(session_id) + .ok() + .flatten() + .and_then(|session| session.pipeline) + .and_then(|pipeline| pipeline.kind) + .is_some_and(|kind| kind == PipelineKind::Rebase) +} + +/// Reattach a branch's commit metadata (and reviews) to the SHAs a rebase +/// rewrote them into. Best-effort: a failure here only leaves the rows +/// orphaned, which is what happened before reassociation existed. +fn reassociate_rebased_commits( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) { + match crate::commit_reassociation::reassociate_after_rebase( + store, + branch_id, + working_dir, + workspace_name, + ) { + Ok(0) => {} + Ok(count) => { + log::info!("Reassociated {count} rebased commit(s) on branch {branch_id}") + } + Err(e) => { + log::warn!("Failed to reassociate rebased commits on branch {branch_id}: {e}") + } + } +} + fn resolve_pipeline_artifacts_without_ai(config: &PipelineConfig, store: &Store, completed: bool) { match config.pipeline.kind.as_ref() { Some(PipelineKind::Rebase) if completed => { @@ -1486,6 +1524,20 @@ fn finalize_rebase_pipeline_without_ai(config: &PipelineConfig, store: &Store) { return; } + // The rebase rewrote every SHA on the branch, orphaning the metadata rows + // that pointed at the old ones. Reattach them *before* claiming the new + // HEAD: once the pre-existing head row owns it, `complete_pending_commit_sha` + // takes its "target SHA already owned" branch and drops this pipeline's + // pending row — so the top commit keeps its authoring session instead of + // the mechanical "Rebase branch" one. A head commit authored outside + // Staged has no prior row, so the rebase session keeps it, as before. + reassociate_rebased_commits( + store, + &commit.branch_id, + &config.working_dir, + config.workspace_name.as_deref(), + ); + match store.complete_pending_commit_sha(&commit.id, &commit.branch_id, ¤t_head) { Ok(true) => log::info!( "Rebase pipeline session {} updated pending commit to {}", @@ -2324,6 +2376,20 @@ fn run_post_completion_hooks( &pre_sha[..7.min(pre_sha.len())], ¤t_head[..7.min(current_head.len())] ); + // A rebase pipeline that handed off to AI (conflicts, a + // failed fetch) lands here instead of + // `finalize_rebase_pipeline_without_ai`, but rewrote SHAs + // just the same. Reattach the orphaned rows before the + // pending row below claims the new HEAD — see that function + // for why the ordering matters. + if session_is_rebase_pipeline(store, session_id) { + reassociate_rebased_commits( + store, + &commit.branch_id, + working_dir, + workspace_name, + ); + } let recorded = if commit.sha.is_none() { match store.complete_pending_commit_sha( &commit.id, @@ -3655,6 +3721,150 @@ mod tests { let _ = std::fs::remove_dir_all(repo); } + /// A branch whose two commits were authored by Staged sessions (with a + /// review on the head one) and have just been rebased onto a moved base by + /// a rebase-pipeline session, leaving every row orphaned. + struct RebasedBranch { + repo: crate::test_utils::TempGitRepo, + store: Arc, + /// `(session_id, commit_row_id)` for the authoring sessions, oldest first. + authored: Vec<(String, String)>, + pending_id: String, + review_id: String, + rebase_session_id: String, + old_head: String, + new_head: String, + new_first: String, + } + + fn rebased_branch() -> RebasedBranch { + use crate::store::{Commit, Review, ReviewScope, Session}; + + let repo = crate::test_utils::TempGitRepo::new(); + repo.write_file("base.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + let old_first = repo.commit("feat: parser"); + repo.write_file("lexer.txt", "lexer\n"); + let old_head = repo.commit("fix: lexer"); + + let store = Arc::new(Store::in_memory().unwrap()); + let project = crate::store::Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = crate::store::Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + // The two sessions that authored the branch, oldest first. + let mut authored = Vec::new(); + for (index, (prompt, sha)) in [("Add parser", &old_first), ("Fix lexer", &old_head)] + .into_iter() + .enumerate() + { + let session = Session::new_running(prompt, repo.path()); + store.create_session(&session).unwrap(); + let mut row = Commit::new_with_sha(&branch.id, sha).with_session(&session.id); + row.created_at = 1_000 + index as i64; + row.updated_at = row.created_at; + store.create_commit(&row).unwrap(); + authored.push((session.id, row.id)); + } + let review = Review::new(&branch.id, &old_head, ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let mut rebase_session = Session::new_running("Rebase branch", repo.path()); + rebase_session.pipeline = + Some(PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Rebase)); + store.create_session(&rebase_session).unwrap(); + let mut pending = Commit::new_pending(&branch.id).with_session(&rebase_session.id); + pending.created_at = 2_000; + pending.updated_at = pending.created_at; + store.create_commit(&pending).unwrap(); + + // Move the base out from under the branch, then really rebase onto it. + repo.run_git(&["checkout", "main"]); + repo.write_file("moved.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + repo.run_git(&["rebase", "--signoff", "origin/main"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + let new_first = repo.run_git(&["rev-parse", "HEAD~1"]).trim().to_string(); + assert_ne!(new_head, old_head, "the rebase must rewrite the SHAs"); + + RebasedBranch { + repo, + store, + authored, + pending_id: pending.id, + review_id: review.id, + rebase_session_id: rebase_session.id, + old_head, + new_head, + new_first, + } + } + + fn assert_reassociated(fixture: &RebasedBranch) { + let store = &fixture.store; + + let first = store.get_commit(&fixture.authored[0].1).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some(fixture.new_first.as_str())); + let head = store.get_commit(&fixture.authored[1].1).unwrap().unwrap(); + assert_eq!(head.sha.as_deref(), Some(fixture.new_head.as_str())); + assert_eq!( + head.session_id.as_deref(), + Some(fixture.authored[1].0.as_str()), + "the head commit must keep its authoring session, not the rebase one" + ); + + // The rebase session's pending row loses the race for the new HEAD and + // is dropped, exactly as it is for a no-op rebase today. + assert!(store.get_commit(&fixture.pending_id).unwrap().is_none()); + + // The review followed its commit, so it stays visible in the timeline. + let review = store.get_review(&fixture.review_id).unwrap().unwrap(); + assert_eq!(review.commit_sha, fixture.new_head); + } + + /// A rebase rewrites every SHA on the branch. Finalizing the pipeline must + /// move the pre-existing commit rows (and their reviews) onto the new SHAs. + #[test] + fn rebase_pipeline_completion_reassociates_pre_existing_commits() { + let fixture = rebased_branch(); + + finalize_rebase_pipeline_without_ai( + &rebase_pipeline_config( + &fixture.rebase_session_id, + fixture.repo.path(), + &fixture.old_head, + ), + &fixture.store, + ); + + assert_reassociated(&fixture); + } + + /// When the pipeline hands off to AI (conflicts, a failed fetch), the agent + /// finishes the rebase and the post-completion hooks run instead — they + /// have to reassociate too. + #[test] + fn rebase_handoff_post_completion_reassociates_pre_existing_commits() { + let fixture = rebased_branch(); + + run_post_completion_hooks( + &fixture.rebase_session_id, + fixture.repo.path(), + Some(&fixture.old_head), + None, + &fixture.store, + ); + + assert_reassociated(&fixture); + } + // ── find_closing_fence ────────────────────────────────────────────── #[test] diff --git a/apps/staged/src-tauri/src/store/commits.rs b/apps/staged/src-tauri/src/store/commits.rs index adc592b57..117376c06 100644 --- a/apps/staged/src-tauri/src/store/commits.rs +++ b/apps/staged/src-tauri/src/store/commits.rs @@ -137,6 +137,58 @@ impl Store { Ok(rows > 0) } + /// Repoint commit rows at the SHAs a history rewrite (e.g. a rebase) gave + /// them, carrying each row's reviews along so they don't drop out of the + /// timeline with their old commit. + /// + /// `remaps` is a list of `(row_id, old_sha, new_sha)`, applied in one + /// transaction. Each pair re-checks the `(branch_id, sha)` unique index + /// inside the transaction and is skipped on collision, so a row another + /// writer already attached to `new_sha` keeps it. Returns how many rows + /// were repointed. + pub fn remap_commit_shas( + &self, + branch_id: &str, + remaps: &[(&str, &str, &str)], + ) -> Result { + let mut conn = self.conn.lock().unwrap(); + let tx = conn.transaction()?; + let now = now_timestamp(); + let mut remapped = 0; + + for (row_id, old_sha, new_sha) in remaps { + let owner = tx + .query_row( + "SELECT id FROM commits WHERE branch_id = ?1 AND sha = ?2", + params![branch_id, new_sha], + |row| row.get::<_, String>(0), + ) + .optional()?; + if owner.is_some_and(|owner| owner != *row_id) { + continue; + } + + let rows = tx.execute( + "UPDATE commits SET sha = ?1, updated_at = ?2 + WHERE id = ?3 AND branch_id = ?4 AND sha = ?5", + params![new_sha, now, row_id, branch_id, old_sha], + )?; + if rows == 0 { + continue; + } + + tx.execute( + "UPDATE reviews SET commit_sha = ?1, updated_at = ?2 + WHERE branch_id = ?3 AND commit_sha = ?4", + params![new_sha, now, branch_id, old_sha], + )?; + remapped += 1; + } + + tx.commit()?; + Ok(remapped) + } + /// Delete a linked pending commit row if it has not landed. pub fn delete_pending_commit_for_session(&self, session_id: &str) -> Result { let conn = self.conn.lock().unwrap(); diff --git a/apps/staged/src-tauri/src/store/tests.rs b/apps/staged/src-tauri/src/store/tests.rs index 0a973ccfe..4a301f818 100644 --- a/apps/staged/src-tauri/src/store/tests.rs +++ b/apps/staged/src-tauri/src/store/tests.rs @@ -1694,6 +1694,75 @@ fn test_complete_pending_commit_sha_updates_pending_row() { assert_eq!(commit.sha.as_deref(), Some("bbb222")); } +#[test] +fn test_remap_commit_shas_moves_rows_and_their_reviews() { + 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 first = Commit::new_with_sha(&branch.id, "old111"); + let second = Commit::new_with_sha(&branch.id, "old222"); + store.create_commit(&first).unwrap(); + store.create_commit(&second).unwrap(); + let review = Review::new(&branch.id, "old222", ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let remapped = store + .remap_commit_shas( + &branch.id, + &[ + (first.id.as_str(), "old111", "new111"), + (second.id.as_str(), "old222", "new222"), + ], + ) + .unwrap(); + + assert_eq!(remapped, 2); + let first = store.get_commit(&first.id).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some("new111")); + let second = store.get_commit(&second.id).unwrap().unwrap(); + assert_eq!(second.sha.as_deref(), Some("new222")); + let review = store.get_review(&review.id).unwrap().unwrap(); + assert_eq!(review.commit_sha, "new222"); +} + +/// The `(branch_id, sha)` unique index is re-checked inside the transaction, +/// so a target SHA another row already owns is skipped rather than blowing up +/// the whole remap. +#[test] +fn test_remap_commit_shas_skips_target_owned_by_another_row() { + 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 orphan = Commit::new_with_sha(&branch.id, "old111"); + let owner = Commit::new_with_sha(&branch.id, "new111"); + let movable = Commit::new_with_sha(&branch.id, "old222"); + store.create_commit(&orphan).unwrap(); + store.create_commit(&owner).unwrap(); + store.create_commit(&movable).unwrap(); + + let remapped = store + .remap_commit_shas( + &branch.id, + &[ + (orphan.id.as_str(), "old111", "new111"), + (movable.id.as_str(), "old222", "new222"), + ], + ) + .unwrap(); + + assert_eq!(remapped, 1); + let orphan = store.get_commit(&orphan.id).unwrap().unwrap(); + assert_eq!(orphan.sha.as_deref(), Some("old111")); + let movable = store.get_commit(&movable.id).unwrap().unwrap(); + assert_eq!(movable.sha.as_deref(), Some("new222")); +} + #[test] fn test_delete_branch_cascades_commits() { let store = Store::in_memory().unwrap(); From feb99da91b890bb572ed116d692b9a44f984fdbf Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 14:48:28 +1000 Subject: [PATCH 2/8] feat(timeline): sort commits by author date so notes stay interleaved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reassociating commits with their sessions after a rebase (f5103d9a) restored each commit's ownership but not its place in the timeline. The branch timeline is one list sorted by timestamp, and the two clocks it merges diverge under a rebase: notes, reviews, and images carry DB times, while commits carried git's committer date, which a rebase rewrites to "now". So a timeline that read commit A -> plan note -> commit B came back as plan note -> A' -> B', with every commit sunk below every note. Author date is the rebase-stable key the reassociation matcher already trusts, so extend it from ownership to position: `git log` now carries both clocks (`BRANCH_COMMIT_LOG_FORMAT`, with a six-field fallback so a stray old-format producer still parses), and `CommitTimelineItem` takes its timestamp from `%at`. Every item on the timeline now sorts by a key a rebase never touches. `CommitInfo.timestamp` stays committer time for `latest_git_commit_ms`, where a rebase *should* read as new activity. Session transcripts get the same `%ct` -> `%at` switch on both the local and remote paths, where the prefix is purely an ordering key. Author dates aren't monotonic along a branch the way committer dates are — a cherry-pick keeps its week-old date — so `build_branch_timeline` clamps them with a running max in branch order, keeping the rendered order matching `git log`. Repo-browse listings and parent-branch commits stay on committer time; they don't interleave with notes. No schema or frontend change. Covered by parse tests for both field counts, clamp unit tests, and an integration test driving a real `git rebase --signoff` over a commit/note/commit branch and asserting the interleaving survives. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/git/mod.rs | 9 +- apps/staged/src-tauri/src/git/worktree.rs | 73 +++++- apps/staged/src-tauri/src/lib.rs | 4 + apps/staged/src-tauri/src/session_commands.rs | 11 +- apps/staged/src-tauri/src/timeline.rs | 219 +++++++++++++++++- 5 files changed, 295 insertions(+), 21 deletions(-) diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index ab65d3be6..e522adcfa 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -52,8 +52,9 @@ pub use worktree::{ create_worktree_for_existing_branch_at_path, create_worktree_from_pr, create_worktree_from_pr_at_path, discard_worktree_changes, fetch_pr_head_sha, get_commits_since_base, get_full_commit_log, get_head_sha, get_parent_commit, - has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_worktree_status_paths, - project_worktree_path_for, project_worktree_root_for, remote_branch_exists, remove_worktree, - reset_to_commit, set_upstream_to_origin, switch_branch, update_branch_from_pr, - worktree_path_for, CommitInfo, UpdateFromPrResult, WorktreeChangePaths, + has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_author_timestamp, + parse_worktree_status_paths, project_worktree_path_for, project_worktree_root_for, + remote_branch_exists, remove_worktree, reset_to_commit, set_upstream_to_origin, switch_branch, + update_branch_from_pr, worktree_path_for, CommitInfo, UpdateFromPrResult, WorktreeChangePaths, + BRANCH_COMMIT_LOG_FORMAT, }; diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index 305ee6e64..1b4337299 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -299,6 +299,23 @@ pub fn get_head_sha(worktree: &Path) -> Result { Ok(output.trim().to_string()) } +/// `git log` format for the branch-timeline commit producers. +/// +/// Carries both clocks: `%ct` (committer time) is what a rebase rewrites, so +/// it answers "did the branch change since?"; `%at` (author time) is what a +/// rebase preserves, so it answers "when was this commit written" and is what +/// the timeline sorts on. +pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H|%h|%s|%an|%ae|%ct|%at"; + +/// Read the author timestamp from a [`BRANCH_COMMIT_LOG_FORMAT`] line's +/// trailing field, falling back to the committer timestamp for six-field lines +/// from a producer still on the older format. +pub fn parse_author_timestamp(field: Option<&str>, committer_timestamp: i64) -> i64 { + field + .and_then(|s| s.parse().ok()) + .unwrap_or(committer_timestamp) +} + /// Get commits on a branch since it diverged from base. /// Returns commits in reverse chronological order (newest first). #[derive(Debug, Clone)] @@ -308,7 +325,10 @@ pub struct CommitInfo { pub subject: String, pub author: String, pub author_email: String, + /// Committer time (`%ct`), in unix seconds. Rewritten by a rebase. pub timestamp: i64, + /// Author time (`%at`), in unix seconds. Preserved by a rebase. + pub author_timestamp: i64, /// Position in git's topological order (0 = oldest on the branch). /// Used as a tiebreaker when multiple commits share the same second-level timestamp. pub order: i64, @@ -329,25 +349,32 @@ pub fn get_commits_since_base(worktree: &Path, base: &str) -> Result Vec { let mut commits = Vec::new(); for line in output.lines() { if line.is_empty() { continue; } - let parts: Vec<&str> = line.splitn(6, '|').collect(); + let parts: Vec<&str> = line.splitn(7, '|').collect(); if parts.len() >= 6 { + let timestamp = parts[5].parse().unwrap_or(0); commits.push(CommitInfo { sha: parts[0].to_string(), short_sha: parts[1].to_string(), subject: parts[2].to_string(), author: parts[3].to_string(), author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), + timestamp, + author_timestamp: parse_author_timestamp(parts.get(6).copied(), timestamp), order: 0, // placeholder, assigned below }); } @@ -359,7 +386,7 @@ pub fn get_commits_since_base(worktree: &Path, base: &str) -> Result Result Result, branch_id: &str) -> i64 { Ok(c) => c, Err(_) => return 0, }; - // CommitInfo.timestamp is in seconds; convert to milliseconds. + // `timestamp` is committer time, not the author time the timeline sorts + // on: a rebase *should* read as new activity here. In seconds, so convert + // to milliseconds. commits.iter().map(|c| c.timestamp).max().unwrap_or(0) * 1000 } @@ -3719,7 +3721,10 @@ fn build_remote_branch_context( "git", "log", "--reverse", - "--format=%x00%ct%x01commit %H%nAuthor: %an%nDate: %ci%n%n%B", + // %at (author time) rather than %ct: the prefix only orders the + // interleave, and author time survives a rebase — see + // `git::get_full_commit_log`, the local counterpart. + "--format=%x00%at%x01commit %H%nAuthor: %an%nDate: %ci%n%n%B", &range, ], ) { @@ -4147,7 +4152,7 @@ fn render_timeline(mut timeline: Vec, error: Option) -> S /// Parse a timestamped git log into timeline entries. /// -/// Expects the format produced by `--format=%x00%ct%x01commit %H…`: +/// Expects the format produced by `--format=%x00%at%x01commit %H…`: /// `\0\x01` per commit. fn parse_timestamped_log(output: &str) -> Vec { let mut entries = Vec::new(); diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index bfa5aeb7a..92030131f 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -176,7 +176,7 @@ fn parse_parent_commit_lines(lines: &[String]) -> Vec { commits } -/// Parse `%H|%h|%s|%an|%ae|%ct` formatted commit lines into timeline items, +/// Parse [`git::BRANCH_COMMIT_LOG_FORMAT`] commit lines into timeline items, /// looking up DB metadata for session linkage. fn parse_commit_lines( store: &Arc, @@ -185,12 +185,13 @@ fn parse_commit_lines( ) -> Vec { let mut commits = Vec::new(); for line in lines { - let parts: Vec<&str> = line.splitn(6, '|').collect(); + let parts: Vec<&str> = line.splitn(7, '|').collect(); if parts.len() >= 6 { let sha = parts[0].to_string(); let our_commit = store.get_commit_by_sha(branch_id, &sha).unwrap_or(None); let resolved = store .resolve_session_status(our_commit.as_ref().and_then(|c| c.session_id.as_deref())); + let committer_timestamp = parts[5].parse().unwrap_or(0); commits.push(CommitTimelineItem { id: our_commit.as_ref().map(|c| c.id.clone()), sha, @@ -198,7 +199,7 @@ fn parse_commit_lines( subject: parts[2].to_string(), author: parts[3].to_string(), author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), + timestamp: git::parse_author_timestamp(parts.get(6).copied(), committer_timestamp), order: 0, session_id: resolved.session_id, session_status: resolved.status, @@ -230,9 +231,12 @@ fn fetch_remote_commits( } else { format!("{base_ref}..HEAD") }; - let format_arg = "--format=%H|%h|%s|%an|%ae|%ct"; - let output = branches::run_workspace_git(ws_name, repo_subpath, &["log", format_arg, &range]) - .map_err(|e| format!("Failed to load commits from workspace: {e}"))?; + let output = branches::run_workspace_git( + ws_name, + repo_subpath, + &["log", git::BRANCH_COMMIT_LOG_FORMAT, &range], + ) + .map_err(|e| format!("Failed to load commits from workspace: {e}"))?; let lines: Vec = output .lines() .filter(|l| !l.is_empty()) @@ -260,7 +264,7 @@ fn map_local_commits( subject: gc.subject.clone(), author: gc.author.clone(), author_email: gc.author_email.clone(), - timestamp: gc.timestamp, + timestamp: gc.author_timestamp, order: gc.order, session_id: resolved.session_id, session_status: resolved.status, @@ -285,6 +289,25 @@ fn non_empty_acp_title(resolved: &ResolvedSession) -> Option { .map(str::to_string) } +/// Clamp commit timestamps so they never decrease in branch order. +/// +/// Committer dates are naturally non-decreasing along a branch; the author +/// dates the timeline sorts on aren't — a cherry-pick keeps its original +/// author date, and an interactive rebase can reorder commits — so a commit +/// could otherwise sort above one that precedes it in `git log`. Walking +/// oldest-first with a running max pins each commit to at least its +/// predecessor's effective time, keeping the rendered order the same as git's. +/// +/// `commits` arrives in `git log` order (newest first), as both producers emit +/// it, so the walk runs in reverse. +fn clamp_commit_timestamps_monotonic(commits: &mut [CommitTimelineItem]) { + let mut floor = i64::MIN; + for commit in commits.iter_mut().rev() { + floor = commit.timestamp.max(floor); + commit.timestamp = floor; + } +} + /// Public wrapper for `build_branch_timeline` for use by the web server. pub fn build_branch_timeline_public( store: &Arc, @@ -413,6 +436,8 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result CommitTimelineItem { + let store = Arc::new(Store::in_memory().unwrap()); + let commits = parse_commit_lines(&store, "branch-1", &[line.to_string()]); + commits.into_iter().next().unwrap() + } + + #[test] + fn parse_commit_lines_takes_its_timestamp_from_author_time() { + let commit = parsed_commit("abc123|abc123a|feat: parser|Test|test@example.com|9100|1100"); + + assert_eq!(commit.timestamp, 1100); + } + + #[test] + fn parse_commit_lines_falls_back_to_committer_time_for_six_field_lines() { + let commit = parsed_commit("abc123|abc123a|feat: parser|Test|test@example.com|9100"); + + assert_eq!(commit.timestamp, 9100); + } + + fn commit_at(subject: &str, timestamp: i64, order: i64) -> CommitTimelineItem { + CommitTimelineItem { + id: None, + sha: format!("sha-{order}"), + short_sha: format!("sha-{order}"), + subject: subject.to_string(), + author: "Test".to_string(), + author_email: "test@example.com".to_string(), + timestamp, + order, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + } + } + + /// A cherry-picked commit keeps its original author date, which would sort + /// it above the commit it actually follows. The clamp pins it down instead. + #[test] + fn clamp_keeps_out_of_order_author_dates_in_branch_order() { + // Newest-first, as `git log` emits. + let mut commits = vec![ + commit_at("fix: lexer", 300, 2), + commit_at("chore: cherry-picked", 100, 1), + commit_at("feat: parser", 200, 0), + ]; + + clamp_commit_timestamps_monotonic(&mut commits); + + assert_eq!( + commits.iter().map(|c| c.timestamp).collect::>(), + vec![300, 200, 200] + ); + } + + #[test] + fn clamp_leaves_already_increasing_author_dates_alone() { + let mut commits = vec![commit_at("fix: lexer", 300, 1), commit_at("feat", 200, 0)]; + + clamp_commit_timestamps_monotonic(&mut commits); + + assert_eq!( + commits.iter().map(|c| c.timestamp).collect::>(), + vec![300, 200] + ); + } + + /// Commit the working tree with a fixed author date, leaving the committer + /// date at "now" — the same split a rebase creates. + fn commit_authored_at(repo: &TempGitRepo, message: &str, author_epoch: i64) -> String { + repo.run_git(&["add", "."]); + repo.run_git(&[ + "commit", + "--date", + &format!("@{author_epoch} +0000"), + "-m", + message, + ]); + repo.run_git(&["rev-parse", "HEAD"]).trim().to_string() + } + + fn committer_time(repo: &TempGitRepo, sha: &str) -> i64 { + repo.run_git(&["show", "-s", "--format=%ct", sha]) + .trim() + .parse() + .unwrap() + } + + /// The timeline interleaves commits with notes by timestamp. A rebase + /// rewrites every committer date to "now" while notes keep their DB times, + /// so sorting on committer time would sink every commit below every note. + /// Author dates survive the rewrite, so the interleaving does too. + #[test] + fn build_branch_timeline_keeps_notes_interleaved_across_a_rebase() { + const FIRST_AUTHORED_AT: i64 = 1_700_000_000; + const NOTE_WRITTEN_AT: i64 = 1_700_000_100; + const SECOND_AUTHORED_AT: i64 = 1_700_000_200; + + let repo = TempGitRepo::new(); + repo.write_file("base.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + commit_authored_at(&repo, "feat: parser", FIRST_AUTHORED_AT); + repo.write_file("lexer.txt", "lexer\n"); + let old_head = commit_authored_at(&repo, "fix: lexer", SECOND_AUTHORED_AT); + + let (store, branch) = store_with_branch(&repo); + let mut note = Note::new(&branch.id, "Plan", "the plan"); + note.created_at = NOTE_WRITTEN_AT * 1000; + note.updated_at = note.created_at; + note.completed_at = Some(note.created_at); + store.create_note(¬e).unwrap(); + + let before = build_branch_timeline(&store, &branch.id).unwrap(); + assert_eq!( + timeline_order(&before), + vec!["feat: parser", "Plan", "fix: lexer"], + "the note was written between the two commits" + ); + + // Move the base out from under the branch, then really rebase onto it. + repo.run_git(&["checkout", "main"]); + repo.write_file("moved.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + repo.run_git(&["rebase", "--signoff", "origin/main"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + assert_ne!(new_head, old_head, "the rebase must rewrite the SHAs"); + + let after = build_branch_timeline(&store, &branch.id).unwrap(); + + assert_eq!( + timeline_order(&after), + vec!["feat: parser", "Plan", "fix: lexer"], + "the note must stay between the two commits after the rebase" + ); + assert_eq!( + after + .commits + .iter() + .map(|c| c.timestamp) + .collect::>(), + vec![SECOND_AUTHORED_AT, FIRST_AUTHORED_AT], + "the rewritten commits keep their original author dates" + ); + // The committer dates really are elsewhere — a `%ct` sort would put + // both commits after the note rather than around it. + assert!( + committer_time(&repo, &new_head) > NOTE_WRITTEN_AT, + "committer time is 'now', long after the note was written" + ); + } + + /// Commit subjects and note titles, merged and sorted the way the frontend + /// does it (`BranchTimeline.svelte`): ascending timestamp, `order` breaking + /// ties between commits. + fn timeline_order(timeline: &BranchTimeline) -> Vec<&str> { + let mut items: Vec<(i64, i64, &str)> = timeline + .commits + .iter() + .map(|c| (c.timestamp, c.order, c.subject.as_str())) + .chain(timeline.notes.iter().map(|n| { + ( + n.completed_at.unwrap_or(n.created_at) / 1000, + 0, + n.title.as_str(), + ) + })) + .collect(); + items.sort_by_key(|(timestamp, order, _)| (*timestamp, *order)); + items.into_iter().map(|(_, _, label)| label).collect() + } } From 959e4537452e75907b6d9e8ca748eb85f3c135aa Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 16:22:23 +1000 Subject: [PATCH 3/8] fix(timeline): close the four gaps review e4f7f39 found in the rebase-stable sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sorting commits by author date (aa8220f1) kept notes interleaved across a rebase, but review e4f7f39 found the ordering key leaking into places it shouldn't and the reassociation trusting a HEAD it shouldn't. `BRANCH_COMMIT_LOG_FORMAT` still kept `%s` mid-line, so a subject containing `|` shifted every field after it — a `%at` parse failure that the new clamp then masked by pulling the commit up to its predecessor's time. Subject moves last, as `commit_reassociation`'s format already had it, and one shared `parse_branch_commit_line` takes it as the remainder. That replaces four hand-rolled index parsers (branch commits, remote commits, parent-branch commits, repo-browse) and the six-field fallback: the last old-format producer, `BATCH_FAST_SCRIPT`, now emits the shared field list too, with a test tying the script to the const. The `%ct` -> `%at` switch had landed in `build_branch_context` without the monotonic clamp, so a cherry-picked commit whose author date predates its parent's could reorder the agent-facing section titled "Branch History (oldest first)" (`order` only breaks ties, so it can't recover an inverted timestamp). The clamp is now `timeline::clamp_timestamps_ monotonic`, shared by both timelines. Reassociation fired on any HEAD move for a rebase-pipeline session, with nothing checking that the rebase had finished. A turn that ends with conflicts unresolved leaves HEAD detached on a partially applied commit, and repointing every row plus its reviews there is worse than leaving them orphaned — `git rebase --abort` restores the originals and the rows would then name commits on no branch at all. `reassociate_after_rebase` now returns 0 unless HEAD is attached to the branch it was asked about, which is the same question as "is the rebase over?" since a rebase only moves the branch ref at the end. Finally the clamp overwrote the field the frontend renders, so a week-old cherry-pick displayed as its successor's time. `CommitTimeline Item` carries `sortTimestamp` for order and keeps `timestamp` truthful; the merge, the commit anchors, `BranchCard`'s latest-item check and the hashtag list sort on the former, the rendered date reads the latter. Adding the field is the reviewer's own suggestion, per AGENTS.md. Covered by parse tests for the new field order and a piped subject, a clamp test asserting the rendered dates stay put, a branch-history clamp test, and an integration test driving a real `git rebase` into a conflict and asserting the stopped rebase leaves every row and review untouched (it repoints them onto the throwaway SHAs without the guard). Signed-off-by: Matt Toohey --- .../src-tauri/src/commit_reassociation.rs | 52 ++++++- apps/staged/src-tauri/src/git/mod.rs | 6 +- apps/staged/src-tauri/src/git/state.rs | 16 ++- apps/staged/src-tauri/src/git/worktree.rs | 128 ++++++++++++------ apps/staged/src-tauri/src/lib.rs | 66 +++++---- apps/staged/src-tauri/src/session_commands.rs | 29 ++++ apps/staged/src-tauri/src/session_runner.rs | 88 ++++++++++++ apps/staged/src-tauri/src/test_utils.rs | 19 ++- apps/staged/src-tauri/src/timeline.rs | 107 ++++++++------- apps/staged/src-tauri/src/web_server.rs | 51 +++---- .../lib/features/branches/BranchCard.svelte | 2 +- .../features/sessions/hashtagItems.test.ts | 3 + .../src/lib/features/sessions/hashtagItems.ts | 2 +- .../features/timeline/BranchTimeline.svelte | 5 +- apps/staged/src/lib/types.ts | 7 +- 15 files changed, 428 insertions(+), 153 deletions(-) diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs index 5f1b334dd..5f0bc6552 100644 --- a/apps/staged/src-tauri/src/commit_reassociation.rs +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -98,7 +98,9 @@ pub fn match_rewritten_commits( /// rebase rewrote them into. Returns how many rows were remapped. /// /// Safe to call when nothing was rewritten: with no orphaned rows it stops -/// after listing the branch and returns 0. +/// after listing the branch and returns 0. Also safe to call while a rebase is +/// still in flight — it returns 0 without touching a row (see +/// [`head_is_on_branch`]). pub fn reassociate_after_rebase( store: &Store, branch_id: &str, @@ -124,6 +126,15 @@ pub fn reassociate_after_rebase( } }; + let branch_name = crate::git::branch_name_without_origin(&branch.branch_name); + if !head_is_on_branch(&git, branch_name) { + log::warn!( + "Skipping commit reassociation on branch {branch_id}: HEAD isn't on {branch_name} \ + (rebase still in progress?)" + ); + return Ok(0); + } + let base_ref = crate::git::origin_ref_for_branch(&branch.base_branch); let on_branch = list_branch_commits(&git, &base_ref)?; @@ -182,6 +193,30 @@ pub fn reassociate_after_rebase( .map_err(|e| e.to_string()) } +/// Whether HEAD is the branch we're about to reassociate, rather than a +/// detached commit. +/// +/// This is how we tell a finished rebase from one still in flight: a rebase +/// detaches HEAD for the whole rewrite and only moves the branch ref at the +/// end. If the agent stops with conflicts unresolved — or its turn simply ends +/// — HEAD sits on a partially applied commit, and repointing rows there would +/// be strictly worse than leaving them orphaned: a later `git rebase --abort` +/// restores the original SHAs, and the rows (plus their reviews) would then +/// name commits that are on no branch at all. Any other detached or +/// wrong-branch HEAD is skipped for the same reason — `merge-base..HEAD` isn't +/// the branch's history, so nothing it lists can be trusted as a rewrite of it. +fn head_is_on_branch(git: &F, branch_name: &str) -> bool +where + F: Fn(&[&str]) -> Result, +{ + // Exits non-zero on a detached HEAD; a failure for any other reason also + // reads as "don't touch anything", which is the safe direction. + match git(&["symbolic-ref", "--quiet", "--short", "HEAD"]) { + Ok(head) => head.trim() == branch_name, + Err(_) => false, + } +} + /// List the branch's commits as `(sha, identity)` pairs, oldest first. fn list_branch_commits(git: &F, base_ref: &str) -> Result, String> where @@ -390,4 +425,19 @@ mod tests { assert!(parse_identity_line("").is_none()); assert!(parse_identity_line("abc111|a@example.com|100").is_none()); } + + /// Mid-rebase, `symbolic-ref` exits non-zero because HEAD is detached; a + /// checkout of some other branch answers with its name. Neither is the + /// branch we were asked to reassociate. + #[test] + fn head_is_on_branch_requires_an_attached_matching_head() { + let on_feature = |_: &[&str]| -> Result { Ok("feature\n".to_string()) }; + assert!(head_is_on_branch(&on_feature, "feature")); + assert!(!head_is_on_branch(&on_feature, "other")); + + let detached = |_: &[&str]| -> Result { + Err("fatal: ref HEAD is not a symbolic ref".to_string()) + }; + assert!(!head_is_on_branch(&detached, "feature")); + } } diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index e522adcfa..c840220eb 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -52,9 +52,9 @@ pub use worktree::{ create_worktree_for_existing_branch_at_path, create_worktree_from_pr, create_worktree_from_pr_at_path, discard_worktree_changes, fetch_pr_head_sha, get_commits_since_base, get_full_commit_log, get_head_sha, get_parent_commit, - has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_author_timestamp, + has_unpushed_commits, list_worktree_change_paths, list_worktrees, parse_branch_commit_line, parse_worktree_status_paths, project_worktree_path_for, project_worktree_root_for, remote_branch_exists, remove_worktree, reset_to_commit, set_upstream_to_origin, switch_branch, - update_branch_from_pr, worktree_path_for, CommitInfo, UpdateFromPrResult, WorktreeChangePaths, - BRANCH_COMMIT_LOG_FORMAT, + update_branch_from_pr, worktree_path_for, BranchCommitFields, CommitInfo, UpdateFromPrResult, + WorktreeChangePaths, BRANCH_COMMIT_LOG_FIELDS, BRANCH_COMMIT_LOG_FORMAT, }; diff --git a/apps/staged/src-tauri/src/git/state.rs b/apps/staged/src-tauri/src/git/state.rs index 277398d2f..ce3c3ff7b 100644 --- a/apps/staged/src-tauri/src/git/state.rs +++ b/apps/staged/src-tauri/src/git/state.rs @@ -1141,7 +1141,10 @@ const BATCH_FAST_SCRIPT: &str = concat!( " range=\"$2..HEAD\"\n", "fi\n", "echo COMMITS_START\n", - "git log --format='%H|%h|%s|%an|%ae|%ct' \"$range\" 2>/dev/null || true\n", + // Same fields as `BRANCH_COMMIT_LOG_FIELDS`, inlined because this is a + // script rather than an argument list; `fast_script_emits_the_shared_commit_fields` + // keeps the two from drifting. + "git log --format='%H|%h|%an|%ae|%ct|%at|%s' \"$range\" 2>/dev/null || true\n", "echo COMMITS_END\n", "exit 0\n", ); @@ -1517,6 +1520,17 @@ pub fn update_repo_fetch_cache(repo_path: &Path) { mod tests { use super::*; + /// The fast script's commit lines are parsed by the same code as every + /// other producer's, so its field list has to match theirs exactly. + #[test] + fn fast_script_emits_the_shared_commit_fields() { + assert!( + BATCH_FAST_SCRIPT.contains(super::super::BRANCH_COMMIT_LOG_FIELDS), + "BATCH_FAST_SCRIPT must log {}", + super::super::BRANCH_COMMIT_LOG_FIELDS + ); + } + fn assert_worktree( input: &str, dirty: bool, diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index 1b4337299..11924ba90 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -299,21 +299,59 @@ pub fn get_head_sha(worktree: &Path) -> Result { Ok(output.trim().to_string()) } -/// `git log` format for the branch-timeline commit producers. +/// The `git log` field list behind [`BRANCH_COMMIT_LOG_FORMAT`], for the one +/// producer that inlines it into a shell script (`state::BATCH_FAST_SCRIPT`) +/// instead of passing it as an argument. +pub const BRANCH_COMMIT_LOG_FIELDS: &str = "%H|%h|%an|%ae|%ct|%at|%s"; + +/// `git log` format for every commit producer that feeds a +/// [`CommitTimelineItem`](crate::CommitTimelineItem). /// /// Carries both clocks: `%ct` (committer time) is what a rebase rewrites, so /// it answers "did the branch change since?"; `%at` (author time) is what a /// rebase preserves, so it answers "when was this commit written" and is what -/// the timeline sorts on. -pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H|%h|%s|%an|%ae|%ct|%at"; +/// the branch timeline sorts on. +/// +/// The subject goes last because it's the only field that can contain the +/// delimiter, so [`parse_branch_commit_line`] can take it as the remainder — +/// the same shape as `commit_reassociation`'s format. +pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H|%h|%an|%ae|%ct|%at|%s"; -/// Read the author timestamp from a [`BRANCH_COMMIT_LOG_FORMAT`] line's -/// trailing field, falling back to the committer timestamp for six-field lines -/// from a producer still on the older format. -pub fn parse_author_timestamp(field: Option<&str>, committer_timestamp: i64) -> i64 { - field - .and_then(|s| s.parse().ok()) - .unwrap_or(committer_timestamp) +/// One [`BRANCH_COMMIT_LOG_FORMAT`] line, borrowed from the log output. +#[derive(Debug, Clone)] +pub struct BranchCommitFields<'a> { + pub sha: &'a str, + pub short_sha: &'a str, + pub author: &'a str, + pub author_email: &'a str, + /// Committer time (`%ct`), in unix seconds. Rewritten by a rebase. + pub committer_timestamp: i64, + /// Author time (`%at`), in unix seconds. Preserved by a rebase. + pub author_timestamp: i64, + pub subject: &'a str, +} + +/// Parse one [`BRANCH_COMMIT_LOG_FORMAT`] line. The subject is the remainder, +/// so subjects containing `|` survive intact. Returns `None` for a line that +/// doesn't carry every field — a blank line, or output from some other format. +pub fn parse_branch_commit_line(line: &str) -> Option> { + let mut parts = line.splitn(7, '|'); + let sha = parts.next().filter(|sha| !sha.is_empty())?; + let short_sha = parts.next()?; + let author = parts.next()?; + let author_email = parts.next()?; + let committer_timestamp = parts.next()?; + let author_timestamp = parts.next()?; + let subject = parts.next()?; + Some(BranchCommitFields { + sha, + short_sha, + author, + author_email, + committer_timestamp: committer_timestamp.parse().unwrap_or(0), + author_timestamp: author_timestamp.parse().unwrap_or(0), + subject, + }) } /// Get commits on a branch since it diverged from base. @@ -359,26 +397,20 @@ pub fn get_commits_since_base(worktree: &Path, base: &str) -> Result Vec { - let mut commits = Vec::new(); - for line in output.lines() { - if line.is_empty() { - continue; - } - let parts: Vec<&str> = line.splitn(7, '|').collect(); - if parts.len() >= 6 { - let timestamp = parts[5].parse().unwrap_or(0); - commits.push(CommitInfo { - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp, - author_timestamp: parse_author_timestamp(parts.get(6).copied(), timestamp), - order: 0, // placeholder, assigned below - }); - } - } + let mut commits: Vec = output + .lines() + .filter_map(parse_branch_commit_line) + .map(|fields| CommitInfo { + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + timestamp: fields.committer_timestamp, + author_timestamp: fields.author_timestamp, + order: 0, // placeholder, assigned below + }) + .collect(); // git log returns newest-first; assign order so that 0 = oldest. let len = commits.len() as i64; @@ -798,16 +830,26 @@ pub fn has_unpushed_commits(worktree: &Path, branch: &str) -> Result = output .lines() - .filter(|l| !l.is_empty()) .enumerate() .filter_map(|(i, line)| { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - Some(CommitTimelineItem { - id: None, - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - order: (max_count - 1 - i) as i64, - session_id: None, - session_status: None, - completion_reason: None, - is_own_commit: false, - }) - } else { - None - } + let fields = git::parse_branch_commit_line(line)?; + Some(CommitTimelineItem { + id: None, + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time: this listing never interleaves with notes, + // so it has no reason to prefer the rebase-stable clock. + timestamp: fields.committer_timestamp, + sort_timestamp: fields.committer_timestamp, + order: (max_count - 1 - i) as i64, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + }) }) .collect(); diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 74aaae021..0d5006c7c 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -4154,6 +4154,11 @@ fn render_timeline(mut timeline: Vec, error: Option) -> S /// /// Expects the format produced by `--format=%x00%at%x01commit %H…`: /// `\0\x01` per commit. +/// +/// The prefix is author time, which a rebase preserves — but author dates +/// aren't monotonic along a branch (a cherry-pick keeps its old one), so the +/// entries are clamped the same way the branch card's are, keeping +/// "Branch History (oldest first)" in `git log` order. fn parse_timestamped_log(output: &str) -> Vec { let mut entries = Vec::new(); // The log is produced with --reverse (oldest-first), so index 0 = oldest. @@ -4174,6 +4179,7 @@ fn parse_timestamped_log(output: &str) -> Vec { } } } + crate::timeline::clamp_timestamps_monotonic(entries.iter_mut().map(|e| &mut e.timestamp)); entries } @@ -6763,6 +6769,29 @@ mod tests { assert!(entries[0].content.contains("user kept this review alive")); } + /// The log's prefix is author time, so a cherry-picked commit can carry a + /// date older than the commit it follows. "Branch History (oldest first)" + /// sorts on that prefix, so the entries have to be clamped into branch + /// order the same way the branch card's commits are. + #[test] + fn parse_timestamped_log_clamps_out_of_order_author_dates() { + // --reverse output: oldest first, with a cherry-pick in the middle. + let log = "\u{0}200\u{1}commit aaa\nfirst\ + \u{0}100\u{1}commit bbb\ncherry-picked\ + \u{0}300\u{1}commit ccc\nthird"; + + let entries = parse_timestamped_log(log); + + assert_eq!( + entries.iter().map(|e| e.timestamp).collect::>(), + vec![200, 200, 300] + ); + assert_eq!( + entries.iter().map(|e| e.order).collect::>(), + vec![0, 1, 2] + ); + } + #[test] fn parse_commit_shas_extracts_full_shas() { let log = "\u{0}1700000000\u{1}commit abc123def456\nAuthor: A\nDate: d\n\nfirst\ diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index b5dda004a..680e13b8f 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -3865,6 +3865,94 @@ mod tests { assert_reassociated(&fixture); } + /// The handoff also runs when the agent's turn ends with the rebase still + /// stopped on a conflict. HEAD is detached on a partially applied rewrite + /// there, and those SHAs only survive until someone runs `git rebase + /// --abort` — which restores the originals — so reassociating onto them + /// would leave every row, and its reviews, naming commits on no branch at + /// all. The rows have to stay put until the rebase finishes. + #[test] + fn rebase_stopped_on_a_conflict_leaves_the_rows_alone() { + use crate::store::{Commit, Review, ReviewScope, Session}; + + let repo = crate::test_utils::TempGitRepo::new(); + repo.write_file("shared.txt", "base\n"); + let base_sha = repo.commit("chore: base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &base_sha]); + + // Two commits: the first rebases cleanly, the second conflicts — so the + // stopped rebase leaves a rewritten commit on a detached HEAD. + repo.run_git(&["checkout", "-b", "feature"]); + repo.write_file("parser.txt", "parser\n"); + let old_first = repo.commit("feat: parser"); + repo.write_file("shared.txt", "feature\n"); + let old_head = repo.commit("fix: lexer"); + + let store = Arc::new(Store::in_memory().unwrap()); + let project = crate::store::Project::new("test-owner/test-repo"); + store.create_project(&project).unwrap(); + let branch = crate::store::Branch::new(&project.id, "feature", "main"); + store.create_branch(&branch).unwrap(); + + let mut rows = Vec::new(); + for (index, sha) in [&old_first, &old_head].into_iter().enumerate() { + let session = Session::new_running("Author a commit", repo.path()); + store.create_session(&session).unwrap(); + let mut row = Commit::new_with_sha(&branch.id, sha).with_session(&session.id); + row.created_at = 1_000 + index as i64; + row.updated_at = row.created_at; + store.create_commit(&row).unwrap(); + rows.push(row.id); + } + let review = Review::new(&branch.id, &old_head, ReviewScope::Commit); + store.create_review(&review).unwrap(); + + let mut rebase_session = Session::new_running("Rebase branch", repo.path()); + rebase_session.pipeline = + Some(PipelineExecution::from_steps(&[]).with_kind(PipelineKind::Rebase)); + store.create_session(&rebase_session).unwrap(); + let pending = Commit::new_pending(&branch.id).with_session(&rebase_session.id); + store.create_commit(&pending).unwrap(); + + // Move the base with a conflicting change, then rebase into the conflict. + repo.run_git(&["checkout", "main"]); + repo.write_file("shared.txt", "moved\n"); + let moved_base = repo.commit("chore: move base"); + repo.run_git(&["update-ref", "refs/remotes/origin/main", &moved_base]); + repo.run_git(&["checkout", "feature"]); + assert!( + repo.try_run_git(&["rebase", "origin/main"]).is_err(), + "the rebase must stop on the conflict" + ); + assert_ne!( + repo.run_git(&["rev-parse", "HEAD"]).trim(), + old_head, + "the stopped rebase must have moved HEAD off the branch" + ); + + run_post_completion_hooks( + &rebase_session.id, + repo.path(), + Some(&old_head), + None, + &store, + ); + + assert_eq!( + store.get_commit(&rows[0]).unwrap().unwrap().sha.as_deref(), + Some(old_first.as_str()), + "the first commit's row must keep the SHA an abort would restore" + ); + assert_eq!( + store.get_commit(&rows[1]).unwrap().unwrap().sha.as_deref(), + Some(old_head.as_str()) + ); + assert_eq!( + store.get_review(&review.id).unwrap().unwrap().commit_sha, + old_head + ); + } + // ── find_closing_fence ────────────────────────────────────────────── #[test] diff --git a/apps/staged/src-tauri/src/test_utils.rs b/apps/staged/src-tauri/src/test_utils.rs index 4fe6a9fe8..37fdd6f04 100644 --- a/apps/staged/src-tauri/src/test_utils.rs +++ b/apps/staged/src-tauri/src/test_utils.rs @@ -39,6 +39,14 @@ impl TempGitRepo { } pub fn run_git(&self, args: &[&str]) -> String { + self.try_run_git(args) + .unwrap_or_else(|stderr| panic!("git {args:?} failed: {stderr}")) + } + + /// Run git and report the exit status instead of asserting on it, for + /// commands whose failure is the point — a `git rebase` that stops on a + /// conflict, say. `Err` carries stderr. + pub fn try_run_git(&self, args: &[&str]) -> Result { let mut command = Command::new("git"); command .arg("-c") @@ -50,14 +58,11 @@ impl TempGitRepo { let output = command.output().unwrap(); - assert!( - output.status.success(), - "git {:?} failed: {}", - args, - String::from_utf8_lossy(&output.stderr) - ); + if !output.status.success() { + return Err(String::from_utf8_lossy(&output.stderr).into_owned()); + } - String::from_utf8(output.stdout).unwrap() + Ok(String::from_utf8(output.stdout).unwrap()) } } diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index 92030131f..662fdd8eb 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -159,21 +159,21 @@ pub struct ParentBranchCommit { } fn parse_parent_commit_lines(lines: &[String]) -> Vec { - let mut commits = Vec::new(); - for line in lines { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - commits.push(ParentBranchCommit { - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - }); - } - } - commits + lines + .iter() + .filter_map(|line| git::parse_branch_commit_line(line)) + .map(|fields| ParentBranchCommit { + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time: these commits are listed on their own, never + // interleaved with notes, so there's nothing for a rebase-stable + // clock to line up with. + timestamp: fields.committer_timestamp, + }) + .collect() } /// Parse [`git::BRANCH_COMMIT_LOG_FORMAT`] commit lines into timeline items, @@ -185,21 +185,20 @@ fn parse_commit_lines( ) -> Vec { let mut commits = Vec::new(); for line in lines { - let parts: Vec<&str> = line.splitn(7, '|').collect(); - if parts.len() >= 6 { - let sha = parts[0].to_string(); + if let Some(fields) = git::parse_branch_commit_line(line) { + let sha = fields.sha.to_string(); let our_commit = store.get_commit_by_sha(branch_id, &sha).unwrap_or(None); let resolved = store .resolve_session_status(our_commit.as_ref().and_then(|c| c.session_id.as_deref())); - let committer_timestamp = parts[5].parse().unwrap_or(0); commits.push(CommitTimelineItem { id: our_commit.as_ref().map(|c| c.id.clone()), sha, - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: git::parse_author_timestamp(parts.get(6).copied(), committer_timestamp), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + timestamp: fields.author_timestamp, + sort_timestamp: fields.author_timestamp, order: 0, session_id: resolved.session_id, session_status: resolved.status, @@ -265,6 +264,7 @@ fn map_local_commits( author: gc.author.clone(), author_email: gc.author_email.clone(), timestamp: gc.author_timestamp, + sort_timestamp: gc.author_timestamp, order: gc.order, session_id: resolved.session_id, session_status: resolved.status, @@ -289,7 +289,7 @@ fn non_empty_acp_title(resolved: &ResolvedSession) -> Option { .map(str::to_string) } -/// Clamp commit timestamps so they never decrease in branch order. +/// Clamp commit sort keys so they never decrease in branch order. /// /// Committer dates are naturally non-decreasing along a branch; the author /// dates the timeline sorts on aren't — a cherry-pick keeps its original @@ -298,13 +298,26 @@ fn non_empty_acp_title(resolved: &ResolvedSession) -> Option { /// oldest-first with a running max pins each commit to at least its /// predecessor's effective time, keeping the rendered order the same as git's. /// +/// Only `sort_timestamp` moves; `timestamp` keeps the real author date, which +/// is what the UI renders. +/// /// `commits` arrives in `git log` order (newest first), as both producers emit /// it, so the walk runs in reverse. -fn clamp_commit_timestamps_monotonic(commits: &mut [CommitTimelineItem]) { +fn clamp_commit_sort_timestamps(commits: &mut [CommitTimelineItem]) { + clamp_timestamps_monotonic(commits.iter_mut().rev().map(|c| &mut c.sort_timestamp)); +} + +/// Raise each timestamp to at least its predecessor's, in iteration order. +/// +/// Shared by the two timelines that interleave git commits with DB-timed items +/// and so have to sort on author date: this module's branch timeline and the +/// agent-facing branch history in `session_commands`. Callers pass their +/// timestamps in branch order, oldest first. +pub(crate) fn clamp_timestamps_monotonic<'a>(timestamps: impl Iterator) { let mut floor = i64::MIN; - for commit in commits.iter_mut().rev() { - floor = commit.timestamp.max(floor); - commit.timestamp = floor; + for timestamp in timestamps { + floor = (*timestamp).max(floor); + *timestamp = floor; } } @@ -436,7 +449,7 @@ fn build_branch_timeline(store: &Arc, branch_id: &str) -> Result, branch_id: &str) -> Result CommitTimelineItem { @@ -1873,6 +1882,7 @@ mod tests { author: "Test".to_string(), author_email: "test@example.com".to_string(), timestamp, + sort_timestamp: timestamp, order, session_id: None, session_status: None, @@ -1892,22 +1902,27 @@ mod tests { commit_at("feat: parser", 200, 0), ]; - clamp_commit_timestamps_monotonic(&mut commits); + clamp_commit_sort_timestamps(&mut commits); assert_eq!( - commits.iter().map(|c| c.timestamp).collect::>(), + commits.iter().map(|c| c.sort_timestamp).collect::>(), vec![300, 200, 200] ); + assert_eq!( + commits.iter().map(|c| c.timestamp).collect::>(), + vec![300, 100, 200], + "the rendered author dates stay untouched" + ); } #[test] fn clamp_leaves_already_increasing_author_dates_alone() { let mut commits = vec![commit_at("fix: lexer", 300, 1), commit_at("feat", 200, 0)]; - clamp_commit_timestamps_monotonic(&mut commits); + clamp_commit_sort_timestamps(&mut commits); assert_eq!( - commits.iter().map(|c| c.timestamp).collect::>(), + commits.iter().map(|c| c.sort_timestamp).collect::>(), vec![300, 200] ); } @@ -2003,13 +2018,13 @@ mod tests { } /// Commit subjects and note titles, merged and sorted the way the frontend - /// does it (`BranchTimeline.svelte`): ascending timestamp, `order` breaking - /// ties between commits. + /// does it (`BranchTimeline.svelte`): ascending sort timestamp, `order` + /// breaking ties between commits. fn timeline_order(timeline: &BranchTimeline) -> Vec<&str> { let mut items: Vec<(i64, i64, &str)> = timeline .commits .iter() - .map(|c| (c.timestamp, c.order, c.subject.as_str())) + .map(|c| (c.sort_timestamp, c.order, c.subject.as_str())) .chain(timeline.notes.iter().map(|n| { ( n.completed_at.unwrap_or(n.created_at) / 1000, diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 283136046..22009fd17 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -995,34 +995,37 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result = output .lines() - .filter(|l| !l.is_empty()) .enumerate() .filter_map(|(i, line)| { - let parts: Vec<&str> = line.splitn(6, '|').collect(); - if parts.len() >= 6 { - Some(crate::CommitTimelineItem { - id: None, - sha: parts[0].to_string(), - short_sha: parts[1].to_string(), - subject: parts[2].to_string(), - author: parts[3].to_string(), - author_email: parts[4].to_string(), - timestamp: parts[5].parse().unwrap_or(0), - order: (max_count - 1 - i) as i64, - session_id: None, - session_status: None, - completion_reason: None, - is_own_commit: false, - }) - } else { - None - } + let fields = crate::git::parse_branch_commit_line(line)?; + Some(crate::CommitTimelineItem { + id: None, + sha: fields.sha.to_string(), + short_sha: fields.short_sha.to_string(), + subject: fields.subject.to_string(), + author: fields.author.to_string(), + author_email: fields.author_email.to_string(), + // Committer time, as in the Tauri command this mirrors. + timestamp: fields.committer_timestamp, + sort_timestamp: fields.committer_timestamp, + order: (max_count - 1 - i) as i64, + session_id: None, + session_status: None, + completion_reason: None, + is_own_commit: false, + }) }) .collect(); Ok(crate::RepoDefaultBranchTimeline { diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index eba9597af..1b288fba1 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -410,7 +410,7 @@ const all: AnyCandidate[] = [...candidates]; for (const commit of timeline.commits) { if (!commit.sha) continue; // skip pending - all.push({ kind: 'commit', timestamp: commit.timestamp }); + all.push({ kind: 'commit', timestamp: commit.sortTimestamp }); } if (all.length === 0) return empty; diff --git a/apps/staged/src/lib/features/sessions/hashtagItems.test.ts b/apps/staged/src/lib/features/sessions/hashtagItems.test.ts index 20d0e9a5c..0b51cd07c 100644 --- a/apps/staged/src/lib/features/sessions/hashtagItems.test.ts +++ b/apps/staged/src/lib/features/sessions/hashtagItems.test.ts @@ -92,6 +92,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 1, + sortTimestamp: 1, order: 0, sessionId: null, sessionStatus: null, @@ -149,6 +150,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 2000, + sortTimestamp: 2000, order: 0, sessionId: null, sessionStatus: null, @@ -163,6 +165,7 @@ describe('timelineToHashtagItems', () => { authorEmail: 'test@example.com', isOwnCommit: true, timestamp: 6000, + sortTimestamp: 6000, order: 1, sessionId: null, sessionStatus: null, diff --git a/apps/staged/src/lib/features/sessions/hashtagItems.ts b/apps/staged/src/lib/features/sessions/hashtagItems.ts index bb683ba92..2c70a5c9f 100644 --- a/apps/staged/src/lib/features/sessions/hashtagItems.ts +++ b/apps/staged/src/lib/features/sessions/hashtagItems.ts @@ -225,7 +225,7 @@ function timelineToSortableHashtagItems( repoSubpath, branchId, projectId, - sortTimestamp: commit.timestamp, + sortTimestamp: commit.sortTimestamp, sortOrder: commit.order, }); } diff --git a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte index 23f649792..211a9d159 100644 --- a/apps/staged/src/lib/features/timeline/BranchTimeline.svelte +++ b/apps/staged/src/lib/features/timeline/BranchTimeline.svelte @@ -217,6 +217,7 @@ secondaryMeta?: string; tertiaryMeta?: string; deleting?: boolean; + /** Sort key in unix seconds. Commits pass their clamped `sortTimestamp`; displayed times come from `meta`. */ timestamp: number; /** Position in git's topological order (0 = oldest). Tiebreaker for same-second timestamps. */ order: number; @@ -552,7 +553,7 @@ secondaryMeta: isDeleting || isRunning ? undefined : commit.shortSha || undefined, tertiaryMeta: showAuthor ? commit.author : undefined, deleting: isDeleting, - timestamp: commit.timestamp, + timestamp: commit.sortTimestamp, order: commit.order, sessionId: commit.sessionId ?? undefined, commitSha: commit.sha || undefined, @@ -568,7 +569,7 @@ if (type === 'commit' && commit.sha) { commitAnchors.set(commit.sha, { - timestamp: commit.timestamp, + timestamp: commit.sortTimestamp, order: commit.order, shortSha: commit.shortSha || undefined, }); diff --git a/apps/staged/src/lib/types.ts b/apps/staged/src/lib/types.ts index ac8d71022..14f9f4164 100644 --- a/apps/staged/src/lib/types.ts +++ b/apps/staged/src/lib/types.ts @@ -126,8 +126,13 @@ export interface CommitTimelineItem { subject: string; author: string; authorEmail: string; - /** Unix timestamp in seconds */ + /** Unix timestamp in seconds — author time for branch commits, so it survives a rebase. */ timestamp: number; + /** + * Unix timestamp in seconds to sort on, clamped so it can't decrease in + * branch order. Order only — render `timestamp`. + */ + sortTimestamp: number; /** Position in git's topological order (0 = oldest). Tiebreaker for same-second timestamps. */ order: number; sessionId: string | null; From 137b61633cf37a6364851540b53957fe43a1ac26 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 4 Aug 2026 17:00:32 +1000 Subject: [PATCH 4/8] fix(git): delimit commit-log fields with %x1f, which git can't emit inside them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review d051cfc0 on dba9dad5 pointed out that "the only field that can contain the delimiter" wasn't airtight: subjects were the only field *expected* to contain `|`, but git also permits it in `user.name` (and technically in emails). An author configured as `Foo | Bar` shifted every field after it — `committer_timestamp` tried to parse the email (0 via `unwrap_or`), `author_timestamp` silently picked up the `%ct` value, and the monotonic clamp then hid the damage — the same masking the piped-subject fix in dba9dad5 closed. Since the field order is a documented invariant shared by four producers, close the whole class as the reviewer suggested: separate fields with `%x1f` (the unit separator) instead of relying on field order to keep `|` harmless. `BRANCH_COMMIT_LOG_FIELDS` and the four producers behind it pick the change up from the one const; `BATCH_FAST_SCRIPT`'s inlined copy is updated in kind and stays ASCII-safe because git expands `%x1f` itself, with the existing drift test still tying the script to the const. `commit_reassociation`'s sibling format gets the same treatment — its `%ae` sits directly before `%at`, exactly the shape the review's failure corrupts, and there a shifted timestamp breaks identity matching, not just ordering. The subject still goes last, so even the separator byte itself in a subject can't reach the timestamps. Covered by the reworked parse tests: a fixture with `|` in the name, the email, and the subject asserting nothing shifts, plus the separator-in-subject remainder case, in both parsers. Signed-off-by: Matt Toohey --- .../src-tauri/src/commit_reassociation.rs | 28 +++++---- apps/staged/src-tauri/src/git/state.rs | 2 +- apps/staged/src-tauri/src/git/worktree.rs | 58 ++++++++++++++----- apps/staged/src-tauri/src/timeline.rs | 4 +- 4 files changed, 63 insertions(+), 29 deletions(-) diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs index 5f0bc6552..e97f93b25 100644 --- a/apps/staged/src-tauri/src/commit_reassociation.rs +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -54,10 +54,12 @@ pub struct ShaRemap { pub new_sha: String, } -/// `%H|%ae|%at|%s` — subject last, since it's the only field that can contain -/// the delimiter. Deliberately not `CommitInfo`'s format, which carries `%ct` -/// (committer time), the one timestamp a rebase rewrites. -const REASSOCIATION_LOG_FORMAT: &str = "--format=%H|%ae|%at|%s"; +/// `%H`, `%ae`, `%at`, `%s`, separated by `%x1f` (the unit separator, as in +/// `BRANCH_COMMIT_LOG_FORMAT`) since git technically permits `|` in emails; +/// the subject still goes last as the remainder. Deliberately not +/// `CommitInfo`'s format, which carries `%ct` (committer time), the one +/// timestamp a rebase rewrites. +const REASSOCIATION_LOG_FORMAT: &str = "--format=%H%x1f%ae%x1f%at%x1f%s"; /// Pair orphaned rows with the commits that replaced them. /// @@ -267,10 +269,10 @@ where Ok(output.lines().filter_map(parse_identity_line).collect()) } -/// Parse one `%H|%ae|%at|%s` line. The subject is the remainder, so subjects -/// containing `|` survive intact. +/// Parse one [`REASSOCIATION_LOG_FORMAT`] line. The subject is the remainder, +/// so even a subject containing the separator byte survives intact. fn parse_identity_line(line: &str) -> Option<(String, CommitIdentity)> { - let mut parts = line.splitn(4, '|'); + let mut parts = line.splitn(4, '\x1f'); let sha = parts.next()?; let author_email = parts.next()?; let author_timestamp = parts.next()?; @@ -411,19 +413,23 @@ mod tests { assert!(remaps.is_empty()); } + /// A `|` is ordinary text in every field now that the separator is + /// `%x1f`; only a separator byte in the subject needs the remainder rule. #[test] - fn parses_subject_containing_the_delimiter() { + fn parses_pipes_and_trailing_separators_intact() { let (sha, identity) = - parse_identity_line("abc111|a@example.com|100|chore: rename a|b to c").unwrap(); + parse_identity_line("abc111\x1fa|b@example.com\x1f100\x1fchore: rename a\x1fb to c") + .unwrap(); assert_eq!(sha, "abc111"); - assert_eq!(identity.subject, "chore: rename a|b to c"); + assert_eq!(identity.author_email, "a|b@example.com"); + assert_eq!(identity.subject, "chore: rename a\x1fb to c"); assert_eq!(identity.author_timestamp, "100"); } #[test] fn ignores_malformed_log_lines() { assert!(parse_identity_line("").is_none()); - assert!(parse_identity_line("abc111|a@example.com|100").is_none()); + assert!(parse_identity_line("abc111\x1fa@example.com\x1f100").is_none()); } /// Mid-rebase, `symbolic-ref` exits non-zero because HEAD is detached; a diff --git a/apps/staged/src-tauri/src/git/state.rs b/apps/staged/src-tauri/src/git/state.rs index ce3c3ff7b..b48657b6e 100644 --- a/apps/staged/src-tauri/src/git/state.rs +++ b/apps/staged/src-tauri/src/git/state.rs @@ -1144,7 +1144,7 @@ const BATCH_FAST_SCRIPT: &str = concat!( // Same fields as `BRANCH_COMMIT_LOG_FIELDS`, inlined because this is a // script rather than an argument list; `fast_script_emits_the_shared_commit_fields` // keeps the two from drifting. - "git log --format='%H|%h|%an|%ae|%ct|%at|%s' \"$range\" 2>/dev/null || true\n", + "git log --format='%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s' \"$range\" 2>/dev/null || true\n", "echo COMMITS_END\n", "exit 0\n", ); diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index 11924ba90..153287080 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -302,7 +302,7 @@ pub fn get_head_sha(worktree: &Path) -> Result { /// The `git log` field list behind [`BRANCH_COMMIT_LOG_FORMAT`], for the one /// producer that inlines it into a shell script (`state::BATCH_FAST_SCRIPT`) /// instead of passing it as an argument. -pub const BRANCH_COMMIT_LOG_FIELDS: &str = "%H|%h|%an|%ae|%ct|%at|%s"; +pub const BRANCH_COMMIT_LOG_FIELDS: &str = "%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s"; /// `git log` format for every commit producer that feeds a /// [`CommitTimelineItem`](crate::CommitTimelineItem). @@ -312,10 +312,12 @@ pub const BRANCH_COMMIT_LOG_FIELDS: &str = "%H|%h|%an|%ae|%ct|%at|%s"; /// rebase preserves, so it answers "when was this commit written" and is what /// the branch timeline sorts on. /// -/// The subject goes last because it's the only field that can contain the -/// delimiter, so [`parse_branch_commit_line`] can take it as the remainder — -/// the same shape as `commit_reassociation`'s format. -pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H|%h|%an|%ae|%ct|%at|%s"; +/// Fields are separated by `%x1f` (the unit separator) because no printable +/// delimiter is safe: git permits `|` in author names — and technically in +/// emails — and a delimiter inside a field shifts every field after it. The +/// subject still goes last so [`parse_branch_commit_line`] can take it as the +/// remainder — the same shape as `commit_reassociation`'s format. +pub const BRANCH_COMMIT_LOG_FORMAT: &str = "--format=%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s"; /// One [`BRANCH_COMMIT_LOG_FORMAT`] line, borrowed from the log output. #[derive(Debug, Clone)] @@ -332,10 +334,11 @@ pub struct BranchCommitFields<'a> { } /// Parse one [`BRANCH_COMMIT_LOG_FORMAT`] line. The subject is the remainder, -/// so subjects containing `|` survive intact. Returns `None` for a line that -/// doesn't carry every field — a blank line, or output from some other format. +/// so even a subject containing the separator byte survives intact. Returns +/// `None` for a line that doesn't carry every field — a blank line, or output +/// from some other format. pub fn parse_branch_commit_line(line: &str) -> Option> { - let mut parts = line.splitn(7, '|'); + let mut parts = line.splitn(7, '\x1f'); let sha = parts.next().filter(|sha| !sha.is_empty())?; let short_sha = parts.next()?; let author = parts.next()?; @@ -841,8 +844,8 @@ mod tests { #[test] fn parses_both_clocks_and_orders_from_the_oldest_commit() { let commits = parse_commit_info_lines(concat!( - "def456|def456a|Test|test@example.com|9200|1200|fix: lexer\n", - "abc123|abc123a|Test|test@example.com|9100|1100|feat: parser\n", + "def456\x1fdef456a\x1fTest\x1ftest@example.com\x1f9200\x1f1200\x1ffix: lexer\n", + "abc123\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100\x1f1100\x1ffeat: parser\n", )); assert_eq!(commits.len(), 2); @@ -857,26 +860,49 @@ mod tests { assert_eq!(commits[1].order, 0); } - /// The subject is the trailing field precisely so a `|` in it can't shift - /// the timestamps out from under the parse. + /// git permits `|` in `user.name` (and technically in emails) and it's + /// common in subjects, which is why the separator is `%x1f` — none of + /// these shift the fields after them. #[test] - fn keeps_a_subject_containing_the_delimiter_intact() { + fn keeps_pipes_in_names_emails_and_subjects_intact() { let commits = parse_commit_info_lines( - "abc123|abc123a|Test|test@example.com|9100|1100|feat: parse a|b unions\n", + "abc123\x1fabc123a\x1fFoo | Bar\x1fa|b@example.com\x1f9100\x1f1100\x1ffeat: parse a|b unions\n", ); assert_eq!(commits.len(), 1); + assert_eq!(commits[0].author, "Foo | Bar"); + assert_eq!(commits[0].author_email, "a|b@example.com"); assert_eq!(commits[0].subject, "feat: parse a|b unions"); assert_eq!(commits[0].timestamp, 9100); assert_eq!(commits[0].author_timestamp, 1100); } + /// The subject is the trailing field so even a separator byte in it can't + /// shift the timestamps out from under the parse. + #[test] + fn keeps_a_subject_containing_the_separator_intact() { + let commits = parse_commit_info_lines( + "abc123\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100\x1f1100\x1ffeat: a\x1fb\n", + ); + + assert_eq!(commits.len(), 1); + assert_eq!(commits[0].subject, "feat: a\x1fb"); + assert_eq!(commits[0].timestamp, 9100); + assert_eq!(commits[0].author_timestamp, 1100); + } + #[test] fn skips_lines_that_are_missing_fields() { assert!(parse_branch_commit_line("").is_none()); - assert!(parse_branch_commit_line("abc123|abc123a|Test|test@example.com|9100").is_none()); + assert!( + parse_branch_commit_line("abc123\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100") + .is_none() + ); // A record with no SHA isn't a commit. - assert!(parse_branch_commit_line("|abc123a|Test|test@example.com|9100|1100|s").is_none()); + assert!(parse_branch_commit_line( + "\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100\x1f1100\x1fs" + ) + .is_none()); } #[test] diff --git a/apps/staged/src-tauri/src/timeline.rs b/apps/staged/src-tauri/src/timeline.rs index 662fdd8eb..8ce0993ca 100644 --- a/apps/staged/src-tauri/src/timeline.rs +++ b/apps/staged/src-tauri/src/timeline.rs @@ -1866,7 +1866,9 @@ mod tests { #[test] fn parse_commit_lines_takes_its_timestamp_from_author_time() { - let commit = parsed_commit("abc123|abc123a|Test|test@example.com|9100|1100|feat: parser"); + let commit = parsed_commit( + "abc123\x1fabc123a\x1fTest\x1ftest@example.com\x1f9100\x1f1100\x1ffeat: parser", + ); assert_eq!(commit.subject, "feat: parser"); assert_eq!(commit.timestamp, 1100); From 3257ebd32132b20d027898b7284f9a54f91d6558 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 11:22:05 +1000 Subject: [PATCH 5/8] refactor(git): delete the FastGitState family, dead since PR #700 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 0a5af850 on 9ffc9311 flagged that the commit before it had spent effort keeping a dead shell script in sync: `BATCH_FAST_SCRIPT`'s inlined `git log` format was rewritten to the `%x1f` delimiter and pinned with a drift test, for a script nothing runs. Note be1f03ab traced when it died. Commit 9318b974 ("fix: eliminate timeline disruption when adopting auto-review", PR #700, May 2026) removed fetching from timeline loading, which retired the two-stream split the family implemented: a fast local-only stream (HEAD, branch, status, commits — no fetch) that emitted a partial timeline with a placeholder git state, front-running a slow stream that blocked on `git fetch`. Once the build never fetches there is no latency to front-run, so that commit deleted every call site, in Rust and in the frontend's `timeline-partial` handling, and left the definitions behind. Nothing has called them in the fifteen months since. So delete the whole family from `git/state.rs`: `FastGitState` and `into_placeholder_git_state`, `compute_fast_local_git_state`, `complete_local_git_state`, `BATCH_FAST_SCRIPT` with `BatchFastOutput` / `parse_batch_fast_output` / `into_fast_git_state` / `compute_fast_git_state_batched`, plus the two-stream scheduling helpers `needs_fetch` (whose doc names the path it chose between) and `local_git_state_cache_key`, which 9318b974 orphaned in the same edit. The `git/mod.rs` re-exports go with them. `BRANCH_COMMIT_LOG_FIELDS` goes too: it existed only so the script could inline the field list that `BRANCH_COMMIT_LOG_FORMAT` passes as an argument, so with the script gone its sole remaining consumer was the test asserting the two match. Keeping it would leave behind a fresh instance of exactly what the review objected to. The producer count for the shared commit-log format drops from four to three — worktree.rs's doc comment naming the script is gone, and the three surviving producers all take the format as an argument. No behaviour change; nothing referenced any of this. Verified with `cargo check --all-targets` (no new warnings), `cargo clippy --all-targets` (only the pre-existing warnings in test_utils.rs, store/tests.rs and the acp_stream_probe example), `cargo fmt --check`, and `cargo test --lib git::` — 82 passed. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/git/mod.rs | 11 +- apps/staged/src-tauri/src/git/state.rs | 341 ---------------------- apps/staged/src-tauri/src/git/worktree.rs | 13 - 3 files changed, 5 insertions(+), 360 deletions(-) diff --git a/apps/staged/src-tauri/src/git/mod.rs b/apps/staged/src-tauri/src/git/mod.rs index c840220eb..c8ea5d63c 100644 --- a/apps/staged/src-tauri/src/git/mod.rs +++ b/apps/staged/src-tauri/src/git/mod.rs @@ -40,11 +40,10 @@ pub use refs::{ origin_ref_for_branch, prune_remote, resolve_ref, BranchRef, }; pub use state::{ - complete_local_git_state, compute_branch_git_state, compute_branch_git_state_batched, - compute_fast_git_state_batched, compute_fast_local_git_state, compute_local_branch_git_state, - ensure_fast_forward_pullable, fast_forward_to_ref, local_git_state_cache_key, needs_fetch, - update_repo_fetch_cache, BaseGitState, BranchGitState, FastGitState, FetchGitState, FetchMode, - FetchStatus, UpstreamGitState, UpstreamRelation, WorktreeGitState, WorktreeStatusScope, + compute_branch_git_state, compute_branch_git_state_batched, compute_local_branch_git_state, + ensure_fast_forward_pullable, fast_forward_to_ref, update_repo_fetch_cache, BaseGitState, + BranchGitState, FetchGitState, FetchMode, FetchStatus, UpstreamGitState, UpstreamRelation, + WorktreeGitState, WorktreeStatusScope, }; pub use types::*; pub use worktree::{ @@ -56,5 +55,5 @@ pub use worktree::{ parse_worktree_status_paths, project_worktree_path_for, project_worktree_root_for, remote_branch_exists, remove_worktree, reset_to_commit, set_upstream_to_origin, switch_branch, update_branch_from_pr, worktree_path_for, BranchCommitFields, CommitInfo, UpdateFromPrResult, - WorktreeChangePaths, BRANCH_COMMIT_LOG_FIELDS, BRANCH_COMMIT_LOG_FORMAT, + WorktreeChangePaths, BRANCH_COMMIT_LOG_FORMAT, }; diff --git a/apps/staged/src-tauri/src/git/state.rs b/apps/staged/src-tauri/src/git/state.rs index b48657b6e..02ab00b78 100644 --- a/apps/staged/src-tauri/src/git/state.rs +++ b/apps/staged/src-tauri/src/git/state.rs @@ -128,58 +128,6 @@ pub enum FetchStatus { Failed, } -/// Fast (local-only) git state — no fetch, no ref comparisons. -/// Used for the fast stream of the two-stream timeline split. -#[derive(Debug, Clone, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct FastGitState { - pub head_sha: Option, - pub current_branch: Option, - pub detached_head: bool, - pub expected_branch_matches: bool, - pub worktree: WorktreeGitState, -} - -impl FastGitState { - /// Convert to a full BranchGitState with placeholder upstream/base/fetch fields. - /// Used to build the partial timeline before the slow stream (fetch + refs) completes. - pub fn into_placeholder_git_state( - self, - branch_name: &str, - base_branch: &str, - ) -> BranchGitState { - let upstream_ref = origin_ref_for_branch(branch_name); - let base_ref = origin_ref_for_branch(base_branch); - BranchGitState { - head_sha: self.head_sha, - current_branch: self.current_branch, - detached_head: self.detached_head, - expected_branch_matches: self.expected_branch_matches, - worktree: self.worktree, - upstream: UpstreamGitState { - r#ref: upstream_ref, - exists: false, - sha: None, - relation: UpstreamRelation::Missing, - ahead: 0, - behind: 0, - merge_base_sha: None, - behind_base: 0, - }, - base: BaseGitState { - r#ref: base_ref, - sha: None, - commits_since_fork: 0, - }, - fetch: FetchGitState { - status: FetchStatus::Stale, - fetched_at: None, - error: None, - }, - } - } -} - #[derive(Debug, Clone)] struct FetchCacheEntry { fetched_at: i64, @@ -729,137 +677,6 @@ pub fn compute_local_branch_git_state( ) } -/// Check whether a fetch is needed for the given cache key and mode. -/// Used by timeline to decide whether to use the two-stream path. -/// -/// For local keys this consults the repo-level cache so the decision is -/// consistent with `refresh_refs_if_needed` — if another branch on the same -/// repo recently fetched, this returns `false`. -pub fn needs_fetch(cache_key: &str, fetch_mode: FetchMode) -> bool { - let now = now_ms(); - - // For local keys, check the repo-level cache first. - if let Some(repo_key) = repo_key_from_local_cache_key(cache_key) { - return match fetch_mode { - FetchMode::Never => false, - FetchMode::Force => true, - FetchMode::Ttl => { - let repo_fresh = repo_fetch_cache() - .lock() - .ok() - .and_then(|cache| cache.get(&repo_key).cloned()) - .map(|entry| now.saturating_sub(entry.fetched_at) <= FETCH_TTL_MS) - .unwrap_or(false); - // Even if the repo is fresh, we might still need a narrow - // fetch for uncovered refspecs — but that's fast enough that - // we don't need the two-stream split for it. - !repo_fresh - } - }; - } - - // Remote / non-local keys: fall back to per-branch cache. - let previous = fetch_cache() - .lock() - .ok() - .and_then(|cache| cache.get(cache_key).cloned()); - - match (fetch_mode, &previous) { - (FetchMode::Never, _) => false, - (FetchMode::Force, _) => true, - (FetchMode::Ttl, Some(entry)) => now.saturating_sub(entry.fetched_at) > FETCH_TTL_MS, - (FetchMode::Ttl, None) => true, - } -} - -/// Compute fast (local-only) git state for a local branch. -/// Returns HEAD, branch name, and worktree status without any fetch. -pub fn compute_fast_local_git_state( - repo: &Path, - branch_name: &str, - worktree_scope: WorktreeStatusScope, -) -> FastGitState { - let run_git = |args: &[&str]| -> Result { - cli::run(repo, args).map_err(|e| e.to_string()) - }; - let (head_sha, branch, worktree) = std::thread::scope(|s| { - let h = s.spawn(|| { - run_git(&["rev-parse", "HEAD"]) - .ok() - .and_then(trim_non_empty) - }); - let b = s.spawn(|| current_branch(&run_git)); - let w = s.spawn(|| compute_worktree_state(&run_git, worktree_scope)); - ( - h.join().expect("head thread panicked"), - b.join().expect("branch thread panicked"), - w.join().expect("worktree thread panicked"), - ) - }); - let expected = branch_name_without_origin(branch_name); - FastGitState { - detached_head: head_sha.is_some() && branch.is_none(), - expected_branch_matches: branch.as_deref().map(|c| c == expected).unwrap_or(false), - head_sha, - current_branch: branch, - worktree, - } -} - -/// Complete a local branch git state: runs fetch + ref comparisons, combining -/// with a pre-computed `FastGitState`. Used by the slow stream after the -/// partial timeline has been emitted. -pub fn complete_local_git_state( - repo: &Path, - fast: &FastGitState, - branch_name: &str, - base_branch: &str, - fetch_mode: FetchMode, -) -> BranchGitState { - let cache_key = format!("local:{}:{}:{}", repo.display(), branch_name, base_branch); - let run_git = |args: &[&str]| -> Result { - cli::run(repo, args).map_err(|e| e.to_string()) - }; - - let refresh = - refresh_refs_if_needed(&cache_key, &run_git, branch_name, base_branch, fetch_mode); - let upstream_ref = origin_ref_for_branch(branch_name); - let base_ref = origin_ref_for_branch(base_branch); - let base_ref_for_upstream = base_ref.clone(); - - let (upstream, base) = std::thread::scope(|s| { - let u = s.spawn(|| { - compute_upstream_state( - &run_git, - upstream_ref, - &base_ref_for_upstream, - fast.head_sha.as_deref(), - refresh.upstream_known_missing, - ) - }); - let b = s.spawn(|| compute_base_state(&run_git, base_ref, fast.head_sha.as_deref())); - ( - u.join().expect("upstream thread panicked"), - b.join().expect("base thread panicked"), - ) - }); - - BranchGitState { - head_sha: fast.head_sha.clone(), - current_branch: fast.current_branch.clone(), - detached_head: fast.detached_head, - expected_branch_matches: fast.expected_branch_matches, - worktree: fast.worktree.clone(), - fetch: refresh.fetch, - upstream, - base, - } -} - -pub fn local_git_state_cache_key(repo: &Path, branch_name: &str, base_branch: &str) -> String { - format!("local:{}:{}:{}", repo.display(), branch_name, base_branch) -} - // --------------------------------------------------------------------------- // Batched computation for remote projects // --------------------------------------------------------------------------- @@ -1107,153 +924,6 @@ fn parse_worktree_from_status(status_output: &str) -> WorktreeGitState { state } -// --------------------------------------------------------------------------- -// Fast script for remote two-stream split -// --------------------------------------------------------------------------- -// -// When a fetch is needed, the timeline uses two concurrent round-trips: -// 1. BATCH_FAST_SCRIPT — local state + commits (no fetch, returns immediately) -// 2. BATCH_GIT_STATE_SCRIPT — fetch + full ref comparisons (blocks on fetch) -// -// The fast script's output is used to emit a partial timeline event so -// commits + worktree rows appear before the slow stream completes. - -/// Fast local-only script for remote projects. -/// -/// Arguments: -/// $1 = repo_path -/// $2 = base_ref (e.g., "origin/main") — used for merge-base + git log -/// $3 = "uno" (no untracked enumeration) or "uall" (full enumeration) -const BATCH_FAST_SCRIPT: &str = concat!( - "cd \"$1\" || exit 1\n", - "head_sha=$(git rev-parse HEAD 2>/dev/null || true)\n", - "printf 'HEAD=%s\\n' \"$head_sha\"\n", - "printf 'BRANCH=%s\\n' \"$(git branch --show-current 2>/dev/null || true)\"\n", - "if [ \"$3\" = 'uno' ]; then ut_flag='--untracked-files=no'; else ut_flag='--untracked-files=all'; fi\n", - "echo STATUS_START\n", - "git status --porcelain=1 \"$ut_flag\" 2>/dev/null || true\n", - "echo STATUS_END\n", - // Commits using locally-cached refs - "mb=$(git merge-base \"$2\" HEAD 2>/dev/null || true)\n", - "if [ -n \"$mb\" ]; then\n", - " range=\"${mb}..HEAD\"\n", - "else\n", - " range=\"$2..HEAD\"\n", - "fi\n", - "echo COMMITS_START\n", - // Same fields as `BRANCH_COMMIT_LOG_FIELDS`, inlined because this is a - // script rather than an argument list; `fast_script_emits_the_shared_commit_fields` - // keeps the two from drifting. - "git log --format='%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s' \"$range\" 2>/dev/null || true\n", - "echo COMMITS_END\n", - "exit 0\n", -); - -/// Parsed output from the fast local-only script. -pub struct BatchFastOutput { - pub head_sha: Option, - pub branch: Option, - pub status_lines: String, - pub commit_lines: Vec, -} - -pub fn parse_batch_fast_output(raw: &str) -> BatchFastOutput { - let mut head_sha = None; - let mut branch = None; - let mut status_lines = String::new(); - let mut commit_lines = Vec::new(); - let mut in_status = false; - let mut in_commits = false; - - for line in raw.lines() { - if line == "STATUS_START" { - in_status = true; - continue; - } - if line == "STATUS_END" { - in_status = false; - continue; - } - if line == "COMMITS_START" { - in_commits = true; - continue; - } - if line == "COMMITS_END" { - in_commits = false; - continue; - } - if in_status { - if !status_lines.is_empty() { - status_lines.push('\n'); - } - status_lines.push_str(line); - continue; - } - if in_commits { - if !line.is_empty() { - commit_lines.push(line.to_string()); - } - continue; - } - if let Some(val) = line.strip_prefix("HEAD=") { - let v = val.trim(); - if !v.is_empty() { - head_sha = Some(v.to_string()); - } - } else if let Some(val) = line.strip_prefix("BRANCH=") { - let v = val.trim(); - if !v.is_empty() { - branch = Some(v.to_string()); - } - } - } - - BatchFastOutput { - head_sha, - branch, - status_lines, - commit_lines, - } -} - -impl BatchFastOutput { - /// Convert to FastGitState. - pub fn into_fast_git_state(self, branch_name: &str) -> (FastGitState, Vec) { - let worktree = parse_worktree_from_status(&self.status_lines); - let expected = branch_name_without_origin(branch_name); - let fast = FastGitState { - detached_head: self.head_sha.is_some() && self.branch.is_none(), - expected_branch_matches: self - .branch - .as_deref() - .map(|c| c == expected) - .unwrap_or(false), - head_sha: self.head_sha, - current_branch: self.branch, - worktree, - }; - (fast, self.commit_lines) - } -} - -/// Run the fast local-only script on a remote workspace and return parsed output. -pub fn compute_fast_git_state_batched( - run_script: &F, - repo_path: &str, - base_branch: &str, - worktree_scope: WorktreeStatusScope, -) -> Result -where - F: Fn(&str, &[&str]) -> Result, -{ - let base_ref = origin_ref_for_branch(base_branch); - let raw = run_script( - BATCH_FAST_SCRIPT, - &[repo_path, &base_ref, worktree_scope.script_arg()], - )?; - Ok(parse_batch_fast_output(&raw)) -} - /// Compute branch git state using a single batched shell script. /// /// This is the remote-optimised counterpart of `compute_branch_git_state`. @@ -1520,17 +1190,6 @@ pub fn update_repo_fetch_cache(repo_path: &Path) { mod tests { use super::*; - /// The fast script's commit lines are parsed by the same code as every - /// other producer's, so its field list has to match theirs exactly. - #[test] - fn fast_script_emits_the_shared_commit_fields() { - assert!( - BATCH_FAST_SCRIPT.contains(super::super::BRANCH_COMMIT_LOG_FIELDS), - "BATCH_FAST_SCRIPT must log {}", - super::super::BRANCH_COMMIT_LOG_FIELDS - ); - } - fn assert_worktree( input: &str, dirty: bool, diff --git a/apps/staged/src-tauri/src/git/worktree.rs b/apps/staged/src-tauri/src/git/worktree.rs index 153287080..a7cf975ad 100644 --- a/apps/staged/src-tauri/src/git/worktree.rs +++ b/apps/staged/src-tauri/src/git/worktree.rs @@ -299,11 +299,6 @@ pub fn get_head_sha(worktree: &Path) -> Result { Ok(output.trim().to_string()) } -/// The `git log` field list behind [`BRANCH_COMMIT_LOG_FORMAT`], for the one -/// producer that inlines it into a shell script (`state::BATCH_FAST_SCRIPT`) -/// instead of passing it as an argument. -pub const BRANCH_COMMIT_LOG_FIELDS: &str = "%H%x1f%h%x1f%an%x1f%ae%x1f%ct%x1f%at%x1f%s"; - /// `git log` format for every commit producer that feeds a /// [`CommitTimelineItem`](crate::CommitTimelineItem). /// @@ -833,14 +828,6 @@ pub fn has_unpushed_commits(worktree: &Path, branch: &str) -> Result Date: Wed, 5 Aug 2026 12:01:12 +1000 Subject: [PATCH 6/8] fix(sessions): defer commit detection when a rebase handoff ends mid-rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review d1db4957 on 0837bb07 flagged the last open gap on this branch: reassociation was guarded against a mid-rebase HEAD (dba9dad5), but the pending-row completion right below it wasn't. A rebase-handoff turn that ends with conflicts unresolved leaves HEAD detached on a partially applied commit; `current_head != pre_sha` fired, and `complete_pending_commit_sha` handed that throwaway SHA to the rebase session's pending row. If the rebase was later finished with `--continue`, the pending row owned the rewritten first commit, so the matcher skipped it as claimed and the authoring session's row stayed orphaned — the exact mis-attribution this branch set out to fix, moved one commit down. If it was aborted, the row named a commit on no branch at all. Either way `recorded` also triggered the auto-review follow-up on a mid-rebase state. Implement plan note 2c4270b8: extend the mid-rebase skip from the reassociation call to the whole detection arm. `commit_reassociation` gains a public `head_is_attached_to_branch` — the same attached-HEAD question `reassociate_after_rebase` already answers internally, now sharing its branch-resolution and git-runner prologue via `branch_git_runner` (errors read as "not attached", the safe direction). `run_post_completion_hooks` asks it once for rebase-pipeline sessions and, mid-rebase, skips reassociation, the pending claim, the amend path, and `committed_branch_id`, so no row is touched and no auto-review fires. Deferring needs no cleanup pass: resumed sessions re-capture `pre_head_sha` at turn start, so the next turn lands back in the hooks with HEAD attached. After `--continue` finishes, reassociation repoints the authored rows first and the pending row drops in `complete_pending_commit_sha`'s duplicate-SHA branch — identical to the no-conflict pipeline path; after `--abort`, the restored head hits the same branch. A session never resumed leaves the pending row `sha IS NULL`, an ordinary failed commit attempt. The conflict test's setup moves into a shared `conflicted_rebase` fixture, and the test now asserts what the review noted it didn't: the pending row stays unclaimed and the hooks return no branch to review. Two new integration tests drive the deferred row through both exits — resolve-and-`--continue` (rows reassociated, review follows, pending dropped as a duplicate) and `--abort` (everything back on the original SHAs, pending dropped). Still out of scope, as the plan note records: a final handoff turn whose HEAD ends where that turn's snapshot started (e.g. `git rebase --skip` dropping the conflicted commit as the last act) takes the HEAD-unchanged arm, so earlier rewritten commits wait for reassociation until the next head-moving turn. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings), `cargo fmt --check`, and `cargo test --lib` — 546 passed. Signed-off-by: Matt Toohey --- .../src-tauri/src/commit_reassociation.rs | 76 +++- apps/staged/src-tauri/src/session_runner.rs | 340 +++++++++++++----- 2 files changed, 317 insertions(+), 99 deletions(-) diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs index e97f93b25..8239480fc 100644 --- a/apps/staged/src-tauri/src/commit_reassociation.rs +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -109,24 +109,7 @@ pub fn reassociate_after_rebase( working_dir: &Path, workspace_name: Option<&str>, ) -> Result { - let branch = store - .get_branch(branch_id) - .map_err(|e| e.to_string())? - .ok_or_else(|| format!("Branch not found: {branch_id}"))?; - - let repo_subpath = match workspace_name { - Some(_) => crate::branches::resolve_branch_workspace_subpath(store, &branch)?, - None => None, - }; - let git = |args: &[&str]| -> Result { - match workspace_name { - Some(ws_name) => { - crate::branches::run_workspace_git(ws_name, repo_subpath.as_deref(), args) - .map_err(|e| e.to_string()) - } - None => crate::git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), - } - }; + let (branch, git) = branch_git_runner(store, branch_id, working_dir, workspace_name)?; let branch_name = crate::git::branch_name_without_origin(&branch.branch_name); if !head_is_on_branch(&git, branch_name) { @@ -195,6 +178,63 @@ pub fn reassociate_after_rebase( .map_err(|e| e.to_string()) } +/// The git runner [`branch_git_runner`] hands back. Boxed rather than an `impl +/// Fn`, since a named type keeps the return signature readable and every +/// consumer here is generic over `Fn` anyway. +type GitRunner<'a> = Box Result + 'a>; + +/// Resolve the branch row and build the git runner the entry points share: +/// local commands run in `working_dir`, remote ones through the branch's +/// workspace (and its repo subpath). +fn branch_git_runner<'a>( + store: &Store, + branch_id: &str, + working_dir: &'a Path, + workspace_name: Option<&'a str>, +) -> Result<(crate::store::Branch, GitRunner<'a>), String> { + let branch = store + .get_branch(branch_id) + .map_err(|e| e.to_string())? + .ok_or_else(|| format!("Branch not found: {branch_id}"))?; + + let repo_subpath = match workspace_name { + Some(_) => crate::branches::resolve_branch_workspace_subpath(store, &branch)?, + None => None, + }; + let git = move |args: &[&str]| -> Result { + match workspace_name { + Some(ws_name) => { + crate::branches::run_workspace_git(ws_name, repo_subpath.as_deref(), args) + .map_err(|e| e.to_string()) + } + None => crate::git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), + } + }; + Ok((branch, Box::new(git))) +} + +/// Whether HEAD is attached to this branch — i.e. no rebase is in flight +/// (see [`head_is_on_branch`] for why that's the same question). For callers +/// that need the answer *before* touching any rows, like the post-completion +/// commit detection. Errors read as "not attached", the safe direction. +pub fn head_is_attached_to_branch( + store: &Store, + branch_id: &str, + working_dir: &Path, + workspace_name: Option<&str>, +) -> bool { + match branch_git_runner(store, branch_id, working_dir, workspace_name) { + Ok((branch, git)) => head_is_on_branch( + &git, + crate::git::branch_name_without_origin(&branch.branch_name), + ), + Err(e) => { + log::warn!("Failed to check whether HEAD is attached to branch {branch_id}: {e}"); + false + } + } +} + /// Whether HEAD is the branch we're about to reassociate, rather than a /// detached commit. /// diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 680e13b8f..83d9528c2 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -2371,79 +2371,110 @@ fn run_post_completion_hooks( match current_head_result { Ok(current_head) if current_head != pre_sha => { - log::info!( - "Session {session_id}: new commit detected ({} → {})", - &pre_sha[..7.min(pre_sha.len())], - ¤t_head[..7.min(current_head.len())] - ); // A rebase pipeline that handed off to AI (conflicts, a // failed fetch) lands here instead of - // `finalize_rebase_pipeline_without_ai`, but rewrote SHAs - // just the same. Reattach the orphaned rows before the - // pending row below claims the new HEAD — see that function - // for why the ordering matters. - if session_is_rebase_pipeline(store, session_id) { - reassociate_rebased_commits( + // `finalize_rebase_pipeline_without_ai`. If its turn ended + // with the rebase still stopped on a conflict, HEAD is + // detached on a partially applied commit — a SHA `git + // rebase --abort` erases — so nothing may claim it: not + // the pending row, not an amend, and no auto-review via + // `committed_branch_id`. Skip the whole arm; the rows + // self-resolve on a later turn, because resumed sessions + // re-capture `pre_head_sha` and land back here once HEAD + // is attached again (after `--continue` finishes or + // `--abort` restores, reassociation plus the duplicate-SHA + // branch of `complete_pending_commit_sha` settle every + // row), while a turn that never comes leaves the pending + // row `sha IS NULL` — an ordinary failed commit attempt. + let rebase_pipeline = session_is_rebase_pipeline(store, session_id); + if rebase_pipeline + && !crate::commit_reassociation::head_is_attached_to_branch( store, &commit.branch_id, working_dir, workspace_name, + ) + { + log::info!( + "Session {session_id}: rebase still in flight (HEAD detached), \ + leaving commit detection for a later turn" ); - } - let recorded = if commit.sha.is_none() { - match store.complete_pending_commit_sha( - &commit.id, - &commit.branch_id, - ¤t_head, - ) { - Ok(recorded) => recorded, - Err(e) => { - log::error!("Failed to update pending commit SHA: {e}"); - false - } - } } else { - match store.get_commit_by_sha(&commit.branch_id, ¤t_head) { - Ok(Some(existing)) if existing.id != commit.id => { - log::warn!( - "Session {session_id}: target commit SHA already has metadata row {}, skipping update", - existing.id - ); - false - } - Ok(_) => { - if let Err(e) = store.update_commit_sha(&commit.id, ¤t_head) { - log::error!("Failed to update commit SHA: {e}"); + log::info!( + "Session {session_id}: new commit detected ({} → {})", + &pre_sha[..7.min(pre_sha.len())], + ¤t_head[..7.min(current_head.len())] + ); + // The rebase rewrote SHAs just the same as the no-AI + // path. Reattach the orphaned rows before the pending + // row below claims the new HEAD — see + // `finalize_rebase_pipeline_without_ai` for why the + // ordering matters. + if rebase_pipeline { + reassociate_rebased_commits( + store, + &commit.branch_id, + working_dir, + workspace_name, + ); + } + let recorded = if commit.sha.is_none() { + match store.complete_pending_commit_sha( + &commit.id, + &commit.branch_id, + ¤t_head, + ) { + Ok(recorded) => recorded, + Err(e) => { + log::error!("Failed to update pending commit SHA: {e}"); false - } else { - true } } - Err(e) => { - log::error!("Failed to check existing commit SHA: {e}"); - false + } else { + match store.get_commit_by_sha(&commit.branch_id, ¤t_head) { + Ok(Some(existing)) if existing.id != commit.id => { + log::warn!( + "Session {session_id}: target commit SHA already has metadata row {}, skipping update", + existing.id + ); + false + } + Ok(_) => { + if let Err(e) = + store.update_commit_sha(&commit.id, ¤t_head) + { + log::error!("Failed to update commit SHA: {e}"); + false + } else { + true + } + } + Err(e) => { + log::error!("Failed to check existing commit SHA: {e}"); + false + } } - } - }; + }; - if recorded { - committed_branch_id = Some(commit.branch_id.clone()); - - // Spawn background diff caching for remote branches. - if let Some(ws_name) = workspace_name { - let commit_shas: Vec = store - .list_commits_for_branch(&commit.branch_id) - .unwrap_or_default() - .into_iter() - .filter_map(|c| c.sha) - .collect(); - crate::diff_cache::spawn_cache_branch_diff( - Arc::clone(store), - commit.branch_id.clone(), - ws_name.to_string(), - current_head.clone(), - commit_shas, - ); + if recorded { + committed_branch_id = Some(commit.branch_id.clone()); + + // Spawn background diff caching for remote branches. + if let Some(ws_name) = workspace_name { + let commit_shas: Vec = store + .list_commits_for_branch(&commit.branch_id) + .unwrap_or_default() + .into_iter() + .filter_map(|c| c.sha) + .collect(); + crate::diff_cache::spawn_cache_branch_diff( + Arc::clone(store), + commit.branch_id.clone(), + ws_name.to_string(), + current_head.clone(), + commit_shas, + ); + } } } } @@ -3865,14 +3896,22 @@ mod tests { assert_reassociated(&fixture); } - /// The handoff also runs when the agent's turn ends with the rebase still - /// stopped on a conflict. HEAD is detached on a partially applied rewrite - /// there, and those SHAs only survive until someone runs `git rebase - /// --abort` — which restores the originals — so reassociating onto them - /// would leave every row, and its reviews, naming commits on no branch at - /// all. The rows have to stay put until the rebase finishes. - #[test] - fn rebase_stopped_on_a_conflict_leaves_the_rows_alone() { + /// A branch like [`RebasedBranch`], except the rebase is still in flight: + /// the first commit rebased cleanly, the second stopped on a conflict, + /// leaving HEAD detached on the partially applied rewrite. + struct ConflictedRebase { + repo: crate::test_utils::TempGitRepo, + store: Arc, + /// `(session_id, commit_row_id)` for the authoring sessions, oldest first. + authored: Vec<(String, String)>, + pending_id: String, + review_id: String, + rebase_session_id: String, + old_first: String, + old_head: String, + } + + fn conflicted_rebase() -> ConflictedRebase { use crate::store::{Commit, Review, ReviewScope, Session}; let repo = crate::test_utils::TempGitRepo::new(); @@ -3894,7 +3933,7 @@ mod tests { let branch = crate::store::Branch::new(&project.id, "feature", "main"); store.create_branch(&branch).unwrap(); - let mut rows = Vec::new(); + let mut authored = Vec::new(); for (index, sha) in [&old_first, &old_head].into_iter().enumerate() { let session = Session::new_running("Author a commit", repo.path()); store.create_session(&session).unwrap(); @@ -3902,7 +3941,7 @@ mod tests { row.created_at = 1_000 + index as i64; row.updated_at = row.created_at; store.create_commit(&row).unwrap(); - rows.push(row.id); + authored.push((session.id, row.id)); } let review = Review::new(&branch.id, &old_head, ReviewScope::Commit); store.create_review(&review).unwrap(); @@ -3930,26 +3969,165 @@ mod tests { "the stopped rebase must have moved HEAD off the branch" ); + ConflictedRebase { + repo, + store, + authored, + pending_id: pending.id, + review_id: review.id, + rebase_session_id: rebase_session.id, + old_first, + old_head, + } + } + + /// End the handoff turn with the rebase still stopped on the conflict. + /// Returns the hooks' `committed_branch_id`, which must be `None` — a + /// mid-rebase state must not trigger the auto-review follow-up. + fn end_turn_mid_rebase(fixture: &ConflictedRebase) -> Option { run_post_completion_hooks( - &rebase_session.id, - repo.path(), - Some(&old_head), + &fixture.rebase_session_id, + fixture.repo.path(), + Some(&fixture.old_head), None, - &store, - ); + &fixture.store, + ) + } + fn assert_untouched(fixture: &ConflictedRebase) { + let store = &fixture.store; assert_eq!( - store.get_commit(&rows[0]).unwrap().unwrap().sha.as_deref(), - Some(old_first.as_str()), + store + .get_commit(&fixture.authored[0].1) + .unwrap() + .unwrap() + .sha + .as_deref(), + Some(fixture.old_first.as_str()), "the first commit's row must keep the SHA an abort would restore" ); assert_eq!( - store.get_commit(&rows[1]).unwrap().unwrap().sha.as_deref(), - Some(old_head.as_str()) + store + .get_commit(&fixture.authored[1].1) + .unwrap() + .unwrap() + .sha + .as_deref(), + Some(fixture.old_head.as_str()) ); assert_eq!( - store.get_review(&review.id).unwrap().unwrap().commit_sha, - old_head + store + .get_review(&fixture.review_id) + .unwrap() + .unwrap() + .commit_sha, + fixture.old_head + ); + } + + /// The handoff also runs when the agent's turn ends with the rebase still + /// stopped on a conflict. HEAD is detached on a partially applied rewrite + /// there, and those SHAs only survive until someone runs `git rebase + /// --abort` — which restores the originals — so neither the authored rows + /// nor the rebase session's pending row may take one. The rows have to + /// stay put until the rebase finishes. + #[test] + fn rebase_stopped_on_a_conflict_leaves_the_rows_alone() { + let fixture = conflicted_rebase(); + + assert!(end_turn_mid_rebase(&fixture).is_none()); + + assert_untouched(&fixture); + let pending = fixture + .store + .get_commit(&fixture.pending_id) + .unwrap() + .unwrap(); + assert!( + pending.sha.is_none(), + "the pending row must not claim the detached mid-rebase SHA" + ); + } + + /// The deferred pending row resolves on the next turn: the resumed session + /// re-captures HEAD (now the detached mid-rebase commit), the conflict is + /// resolved, and `--continue` finishes the rebase. The authored rows claim + /// the rewritten SHAs first, so the pending row drops as a duplicate — + /// the same end state as a rebase that never conflicted. + #[test] + fn rebase_resumed_and_finished_resolves_the_deferred_pending_row() { + let fixture = conflicted_rebase(); + assert!(end_turn_mid_rebase(&fixture).is_none()); + + let repo = &fixture.repo; + let detached_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + repo.write_file("shared.txt", "resolved\n"); + repo.run_git(&["add", "shared.txt"]); + repo.run_git(&["-c", "core.editor=true", "rebase", "--continue"]); + let new_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + let new_first = repo.run_git(&["rev-parse", "HEAD~1"]).trim().to_string(); + + run_post_completion_hooks( + &fixture.rebase_session_id, + repo.path(), + Some(&detached_head), + None, + &fixture.store, + ); + + let store = &fixture.store; + let first = store.get_commit(&fixture.authored[0].1).unwrap().unwrap(); + assert_eq!(first.sha.as_deref(), Some(new_first.as_str())); + let head = store.get_commit(&fixture.authored[1].1).unwrap().unwrap(); + assert_eq!(head.sha.as_deref(), Some(new_head.as_str())); + assert_eq!( + head.session_id.as_deref(), + Some(fixture.authored[1].0.as_str()), + "the head commit must keep its authoring session, not the rebase one" + ); + assert_eq!( + store + .get_review(&fixture.review_id) + .unwrap() + .unwrap() + .commit_sha, + new_head + ); + assert!( + store.get_commit(&fixture.pending_id).unwrap().is_none(), + "the deferred pending row must drop as a duplicate of the reclaimed head" + ); + } + + /// The other way out of the conflict: `--abort` restores the original + /// SHAs. The next turn sees HEAD attached again, reassociation finds no + /// orphans, and the pending row's claim on the old head hits the same + /// duplicate-resolution branch — dropped cleanly, rows untouched. + #[test] + fn rebase_resumed_and_aborted_drops_the_deferred_pending_row() { + let fixture = conflicted_rebase(); + assert!(end_turn_mid_rebase(&fixture).is_none()); + + let repo = &fixture.repo; + let detached_head = repo.run_git(&["rev-parse", "HEAD"]).trim().to_string(); + repo.run_git(&["rebase", "--abort"]); + + run_post_completion_hooks( + &fixture.rebase_session_id, + repo.path(), + Some(&detached_head), + None, + &fixture.store, + ); + + assert_untouched(&fixture); + assert!( + fixture + .store + .get_commit(&fixture.pending_id) + .unwrap() + .is_none(), + "the deferred pending row must drop as a duplicate of the restored head" ); } From 8c211003e509fc77fda24482581545887647a507 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 14:31:43 +1000 Subject: [PATCH 7/8] refactor(sessions): fold the mid-rebase gate into reassociation, one call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 5b5ce229 on b2e5df3d flagged duplicated work on the turn-completion path. A rebase-handoff turn whose HEAD moved made two back-to-back calls into `commit_reassociation`: `head_is_attached_to_branch` to decide whether the rebase was over, then — if it was — `reassociate_after_rebase`, which re-resolved the branch and re-ran the same `symbolic-ref` before doing its real work. On a Blox workspace that second `symbolic-ref` is a full `ws exec` round trip to a cloud workstation, on every successful handoff turn. (One correction to the review's cost estimate: `resolve_branch_workspace_subpath` is DB-only, so the duplicated remote call was exactly one, not each of the three.) The review suggested an internal entry point taking the already-built `(branch, git)` pair. That shape forks badly: keep its own attached-HEAD check and the duplicate `symbolic-ref` remains; drop it and the dba9dad5 invariant "no rows are touched unless HEAD is attached" stops being enforced by the module. Unnecessary, because the gate and the reassociation are 1:1 — every gate-true reassociates, every gate-false skips, and the other caller `finalize_rebase_pipeline_without_ai` wants check-then-reassociate too. So merge them: `reassociate_after_rebase` now returns a `Reassociation` (`MidRebase` | `Done { remapped }`), resolving once, checking once, and reporting which way it went. The check still happens exactly once inside the module — now immediately before rows are touched rather than two calls earlier, holding the reviewer's safety property more tightly. `head_is_attached_to_branch` goes; the gate was its only caller. `run_post_completion_hooks` replaces its gate plus its inner reassociation with the single call, and `mid_rebase` keeps gating the whole arm (pending claim, amend path, auto-review) as before. The load-bearing ordering — reassociate before `complete_pending_commit_sha` claims the new HEAD — is now true by construction. `branch_git_runner` failures adopt the gate's old error mapping and read as `MidRebase`, the safe direction, and the unattached-HEAD log drops from `warn` to `info` since a mid-rebase handoff turn hits it routinely. `Err` is reserved for failures after the check passed, so the `session_runner` helper folds it into `Done` — detection must proceed, exactly today's best-effort semantics. Remote cost for the attached case: two branch resolves and two `symbolic-ref` calls become one each. No behaviour change, so no new tests: nothing called either function directly, and the existing suite is the regression net — both completion paths' reassociation tests and the three `conflicted_rebase` tests (mid-rebase defers everything; `--continue` and `--abort` still settle every row). A "symbolic-ref runs once" assertion would need injection machinery worth more than the check; the win is structural, the second call site no longer exists. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings), `cargo fmt --check`, and `cargo test --lib` — 593 passed. Signed-off-by: Matt Toohey --- .../src-tauri/src/commit_reassociation.rs | 79 ++++++++++--------- apps/staged/src-tauri/src/session_runner.rs | 63 +++++++++------ 2 files changed, 80 insertions(+), 62 deletions(-) diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs index 8239480fc..e122b1ee6 100644 --- a/apps/staged/src-tauri/src/commit_reassociation.rs +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -96,28 +96,54 @@ pub fn match_rewritten_commits( remaps } +/// Which way [`reassociate_after_rebase`] went — it answers "is the rebase +/// over?" on its way in, so callers that must not touch rows mid-rebase can +/// read the answer off the same call instead of asking again. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Reassociation { + /// HEAD isn't attached to the branch — or that couldn't be verified + /// (errors read as "not attached", the safe direction). No row was + /// touched; commit detection must defer to a later turn. + MidRebase, + /// HEAD is attached to the branch; `remapped` rows (possibly 0) were + /// repointed. + Done { remapped: usize }, +} + /// Repoint a branch's orphaned commit rows (and their reviews) at the SHAs a -/// rebase rewrote them into. Returns how many rows were remapped. +/// rebase rewrote them into. /// /// Safe to call when nothing was rewritten: with no orphaned rows it stops -/// after listing the branch and returns 0. Also safe to call while a rebase is -/// still in flight — it returns 0 without touching a row (see -/// [`head_is_on_branch`]). +/// after listing the branch and reports `Done { remapped: 0 }`. Also safe to +/// call while a rebase is still in flight — it reports +/// [`MidRebase`](Reassociation::MidRebase) without touching a row (see +/// [`head_is_on_branch`] for why an unattached HEAD is the same question as "is +/// the rebase still going?"). That check happens before anything else here, so a +/// `MidRebase` answer is a promise that no row moved, and `Err` is reserved for +/// failures *after* it passed. pub fn reassociate_after_rebase( store: &Store, branch_id: &str, working_dir: &Path, workspace_name: Option<&str>, -) -> Result { - let (branch, git) = branch_git_runner(store, branch_id, working_dir, workspace_name)?; +) -> Result { + let (branch, git) = match branch_git_runner(store, branch_id, working_dir, workspace_name) { + Ok(pair) => pair, + Err(e) => { + log::warn!("Failed to resolve branch {branch_id} for commit reassociation: {e}"); + return Ok(Reassociation::MidRebase); + } + }; let branch_name = crate::git::branch_name_without_origin(&branch.branch_name); if !head_is_on_branch(&git, branch_name) { - log::warn!( + // Routine on a rebase-handoff turn that ended with conflicts + // unresolved, so an expected state rather than an anomaly. + log::info!( "Skipping commit reassociation on branch {branch_id}: HEAD isn't on {branch_name} \ (rebase still in progress?)" ); - return Ok(0); + return Ok(Reassociation::MidRebase); } let base_ref = crate::git::origin_ref_for_branch(&branch.base_branch); @@ -146,7 +172,7 @@ pub fn reassociate_after_rebase( .map(|sha| (*sha).to_string()) .collect(); if orphan_shas.is_empty() { - return Ok(0); + return Ok(Reassociation::Done { remapped: 0 }); } let mut old_identities = lookup_commit_identities(&git, &orphan_shas)?; @@ -166,16 +192,17 @@ pub fn reassociate_after_rebase( let remaps = match_rewritten_commits(&orphans, &rewritten); if remaps.is_empty() { - return Ok(0); + return Ok(Reassociation::Done { remapped: 0 }); } let pairs: Vec<(&str, &str, &str)> = remaps .iter() .map(|r| (r.row_id.as_str(), r.old_sha.as_str(), r.new_sha.as_str())) .collect(); - store + let remapped = store .remap_commit_shas(branch_id, &pairs) - .map_err(|e| e.to_string()) + .map_err(|e| e.to_string())?; + Ok(Reassociation::Done { remapped }) } /// The git runner [`branch_git_runner`] hands back. Boxed rather than an `impl @@ -183,9 +210,9 @@ pub fn reassociate_after_rebase( /// consumer here is generic over `Fn` anyway. type GitRunner<'a> = Box Result + 'a>; -/// Resolve the branch row and build the git runner the entry points share: -/// local commands run in `working_dir`, remote ones through the branch's -/// workspace (and its repo subpath). +/// Resolve the branch row and build the git runner [`reassociate_after_rebase`] +/// works through: local commands run in `working_dir`, remote ones through the +/// branch's workspace (and its repo subpath). fn branch_git_runner<'a>( store: &Store, branch_id: &str, @@ -213,28 +240,6 @@ fn branch_git_runner<'a>( Ok((branch, Box::new(git))) } -/// Whether HEAD is attached to this branch — i.e. no rebase is in flight -/// (see [`head_is_on_branch`] for why that's the same question). For callers -/// that need the answer *before* touching any rows, like the post-completion -/// commit detection. Errors read as "not attached", the safe direction. -pub fn head_is_attached_to_branch( - store: &Store, - branch_id: &str, - working_dir: &Path, - workspace_name: Option<&str>, -) -> bool { - match branch_git_runner(store, branch_id, working_dir, workspace_name) { - Ok((branch, git)) => head_is_on_branch( - &git, - crate::git::branch_name_without_origin(&branch.branch_name), - ), - Err(e) => { - log::warn!("Failed to check whether HEAD is attached to branch {branch_id}: {e}"); - false - } - } -} - /// Whether HEAD is the branch we're about to reassociate, rather than a /// detached commit. /// diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index 83d9528c2..fbe2c01c5 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -49,6 +49,7 @@ use acp_client::{AgentRunOutcome, McpServer, McpServerHttp}; use crate::actions::{ActionExecutor, ActionRegistry}; use crate::agent::{AcpDriver, AgentDriver, MessageWriter}; +use crate::commit_reassociation::Reassociation; use crate::git::Span; use crate::shell_env::ShellEnvCache; use crate::store::{ @@ -1444,26 +1445,36 @@ fn session_is_rebase_pipeline(store: &Store, session_id: &str) -> bool { } /// Reattach a branch's commit metadata (and reviews) to the SHAs a rebase -/// rewrote them into. Best-effort: a failure here only leaves the rows -/// orphaned, which is what happened before reassociation existed. +/// rewrote them into, and report whether the rebase had finished — callers that +/// must not touch a row mid-rebase gate on the returned +/// [`Reassociation::MidRebase`]. +/// +/// Best-effort: a failure here only leaves the rows orphaned, which is what +/// happened before reassociation existed, so an `Err` folds into `Done`. It can +/// only be raised once HEAD was seen attached, so the caller's detection should +/// carry on regardless — `MidRebase` is the only answer that means "defer". fn reassociate_rebased_commits( store: &Store, branch_id: &str, working_dir: &Path, workspace_name: Option<&str>, -) { +) -> Reassociation { match crate::commit_reassociation::reassociate_after_rebase( store, branch_id, working_dir, workspace_name, ) { - Ok(0) => {} - Ok(count) => { - log::info!("Reassociated {count} rebased commit(s) on branch {branch_id}") + Ok(Reassociation::Done { remapped: 0 }) => Reassociation::Done { remapped: 0 }, + Ok(Reassociation::Done { remapped }) => { + log::info!("Reassociated {remapped} rebased commit(s) on branch {branch_id}"); + Reassociation::Done { remapped } } + // Already logged inside the module, with the branch name it wanted. + Ok(Reassociation::MidRebase) => Reassociation::MidRebase, Err(e) => { - log::warn!("Failed to reassociate rebased commits on branch {branch_id}: {e}") + log::warn!("Failed to reassociate rebased commits on branch {branch_id}: {e}"); + Reassociation::Done { remapped: 0 } } } } @@ -1531,7 +1542,13 @@ fn finalize_rebase_pipeline_without_ai(config: &PipelineConfig, store: &Store) { // pending row — so the top commit keeps its authoring session instead of // the mechanical "Rebase branch" one. A head commit authored outside // Staged has no prior row, so the rebase session keeps it, as before. - reassociate_rebased_commits( + // + // This path runs when the pipeline drove the rebase to completion itself, + // and its own HEAD-moved check above already established there's something + // to claim, so the returned outcome isn't a gate here: a `MidRebase` answer + // (an unattached HEAD, or a branch that wouldn't resolve) just means the + // rows stay orphaned while the claim below proceeds as it always has. + let _ = reassociate_rebased_commits( store, &commit.branch_id, &config.working_dir, @@ -2386,15 +2403,24 @@ fn run_post_completion_hooks( // branch of `complete_pending_commit_sha` settle every // row), while a turn that never comes leaves the pending // row `sha IS NULL` — an ordinary failed commit attempt. + // + // Asking is the same call that does the work: the rebase + // rewrote SHAs just the same as the no-AI path, and + // reassociation has to check the attached HEAD anyway + // before it repoints anything, so it reports which way it + // went. That also keeps the load-bearing ordering — reattach + // the orphaned rows before the pending row below claims the + // new HEAD, see `finalize_rebase_pipeline_without_ai` for + // why — true by construction. let rebase_pipeline = session_is_rebase_pipeline(store, session_id); - if rebase_pipeline - && !crate::commit_reassociation::head_is_attached_to_branch( + let mid_rebase = rebase_pipeline + && reassociate_rebased_commits( store, &commit.branch_id, working_dir, workspace_name, - ) - { + ) == Reassociation::MidRebase; + if mid_rebase { log::info!( "Session {session_id}: rebase still in flight (HEAD detached), \ leaving commit detection for a later turn" @@ -2405,19 +2431,6 @@ fn run_post_completion_hooks( &pre_sha[..7.min(pre_sha.len())], ¤t_head[..7.min(current_head.len())] ); - // The rebase rewrote SHAs just the same as the no-AI - // path. Reattach the orphaned rows before the pending - // row below claims the new HEAD — see - // `finalize_rebase_pipeline_without_ai` for why the - // ordering matters. - if rebase_pipeline { - reassociate_rebased_commits( - store, - &commit.branch_id, - working_dir, - workspace_name, - ); - } let recorded = if commit.sha.is_none() { match store.complete_pending_commit_sha( &commit.id, From fabc47e84e6868a34af5a83c74f4ae5925a7226b Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 5 Aug 2026 16:08:03 +1000 Subject: [PATCH 8/8] fix(sessions): resolve every remote HEAD read to the branch's clone dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan note f6c9e34c: Staged read git HEAD on a Blox workspace two different ways, and on a multi-repo workspace they answered about two different checkouts. Every HEAD snapshot went through a bare `sq blox ws exec -- git rev-parse HEAD`, which runs at whatever cwd a bare exec lands in, while `commit_reassociation`'s runner went through `run_workspace_git`, which prepends `-C /home/bloxer/[/]` from the branch row. One project gets one workspace and additional repos are cloned into it as siblings, so at most one clone can be the bare-exec cwd and every branch on the others was read at the wrong checkout. The mid-rebase guard (b2e5df3d) is where that disagreement first became load-bearing: `head_is_on_branch` returns false on a wrong-branch answer or any error, both of which a foreign checkout can produce, so the hooks would take the defer arm forever — the pending row never gets its SHA and no auto-review fires, with one `log::info!` per turn as the only trace. The inverse holds too: a clean primary repo masks a genuinely mid-rebase branch repo. But the divergence predates the guard. `pre_head_sha` and `current_head` are consistent with each other and both about the wrong repo, so an unrelated commit landing in the primary repo makes `current_head != pre_sha` fire for a session that committed nothing, while a real commit on the branch repo reads as "HEAD unchanged". The resolved form is what the rest of the remote path already uses — the rebase itself (`run_remote_pipeline_command` cds into `remote_working_dir`), the agent's cwd, timeline commit deletion, diff collection — so the bare reads were the outliers, not the guard. `branches.rs` gains the runner next to the resolution it needs: `GitRunner`, `git_runner` for an already-resolved directory, and `branch_git_runner`, which resolves one from the branch row (the same call `remote_working_dir` is built from, so the two agree byte for byte). `head_sha` reads HEAD through a runner so no caller hand-rolls `rev-parse`, and `branch_head_sha` wraps it for async callers. Both `ws_exec` argv builders now share `workspace_git_args`, extracted pure so the one rule everything here rests on is testable without a workstation. `commit_reassociation` sheds the workspace knowledge: its runner moves out and `reassociate_after_rebase` takes `(store, &Branch, &GitRunner)`. Both callers hold a config carrying `remote_working_dir`, so — one deviation from the plan — the branch-id wrapper it proposed keeping would have had no callers, and a second entry point would have forced `reassociate_rebased_commits` to duplicate its logging per shape. The attached-HEAD check still runs exactly once, inside the module, immediately before any row is touched. `run_post_completion_hooks` takes `remote_working_dir` and builds one runner that the HEAD read and the reassociation share, dropping the second branch resolution the reviewer flagged; a branch row that won't load now reads as `MidRebase`, which is where the old resolution failure already landed. `current_pipeline_head` reads `config.remote_working_dir` through the same primitive, with no signature churn at its three call sites, and `finalize_rebase_pipeline_without_ai` shares one runner the same way. The capture sites move in the same commit, because they must: if `current_head` resolves and `pre_head_sha` doesn't, every remote commit session on a multi-repo workspace compares two unrelated SHAs and reports a phantom commit. That's the resume path, the start path, the queued path and the web-server start path, all via `branch_head_sha`. The three review-anchor sites go too — strictly beyond the commit-detection problem, but the identical one-line change through the identical helper, and a bare-read SHA stored as `reviews.commit_sha` matches nothing in `review_is_visible_in_timeline`, so the review stays hidden. Each site's own error split (a remote failure degrades, a local one fails the command) is preserved in `commit_pre_head_sha` / `review_tip_sha` rather than hand-rolled four more times. `diff_cache` needed nothing: it already resolves the clone dir itself and inherits the corrected SHA. `session_commands::run_blox_blocking` had no callers left and is deleted. Branches with `project_repo_id: None` still resolve to `None` and run bare, byte-identical to today. The local path only swaps `cli::run` for `cli_run_smart` — same cwd, one lite-env attempt before the captured one. There is no remote test harness, so the new coverage is the three `workspace_git_args` tests pinning the rule: a branch with a repo yields `git -C /home/bloxer// …`, one without runs bare, and an absolute `remote_working_dir` resolves to the same `-C` as the `home:` form. The rebase suite (both completion paths' reassociation tests, the three `conflicted_rebase` exits) all run with `workspace_name: None` and stays green unchanged as the regression net for the working path. Not done: the plan's `sq blox ws exec -- pwd` check, which would have told us whether the bare cwd is the primary clone (so single-repo projects see no change) or `$HOME` (so remote commit detection has been a silent no-op throughout). `sq blox ws list` reports no workstations on this machine, so there was nothing live to ask. Both readings point at this same fix; they differ only in how much it turns on. Verified with `cargo check --all-targets`, `cargo clippy --all-targets` (only the three pre-existing warnings in test_utils.rs, store/tests.rs and the acp_stream_probe example), `cargo fmt --check`, and `cargo test --lib` — 596 passed. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/branches.rs | 190 +++++++++++++++++- .../src-tauri/src/commit_reassociation.rs | 64 ++---- apps/staged/src-tauri/src/session_commands.rs | 132 ++++-------- apps/staged/src-tauri/src/session_runner.rs | 105 ++++++---- apps/staged/src-tauri/src/web_server.rs | 10 +- 5 files changed, 304 insertions(+), 197 deletions(-) diff --git a/apps/staged/src-tauri/src/branches.rs b/apps/staged/src-tauri/src/branches.rs index c4d29ca3a..74d1ec886 100644 --- a/apps/staged/src-tauri/src/branches.rs +++ b/apps/staged/src-tauri/src/branches.rs @@ -205,12 +205,19 @@ pub(crate) fn repo_name_from_github_repo(github_repo: &str) -> String { } } -pub(crate) fn run_workspace_git( +/// Build the argv every workspace git call hands to `ws_exec`. +/// +/// A repo subpath becomes `git -C …`, pinning the command to that +/// clone; without one the command runs bare, at whatever directory the +/// workspace hands a bare exec. That one rule is what every remote read of a +/// branch's HEAD rests on, so it lives in a pure function that can be tested +/// without a workstation. +fn workspace_git_args( workspace_name: &str, repo_subpath: Option<&str>, git_args: &[&str], -) -> Result { - let mut owned = Vec::::new(); +) -> Result, blox::BloxError> { + let mut owned = Vec::::with_capacity(3 + git_args.len()); owned.push("git".to_string()); if let Some(subpath) = repo_subpath.map(str::trim).filter(|s| !s.is_empty()) { let resolved = resolve_workspace_repo_path(workspace_name, subpath)?; @@ -218,6 +225,15 @@ pub(crate) fn run_workspace_git( owned.push(resolved); } owned.extend(git_args.iter().map(|arg| (*arg).to_string())); + Ok(owned) +} + +pub(crate) fn run_workspace_git( + workspace_name: &str, + repo_subpath: Option<&str>, + git_args: &[&str], +) -> Result { + let owned = workspace_git_args(workspace_name, repo_subpath, git_args)?; let borrowed = owned.iter().map(String::as_str).collect::>(); blox::ws_exec(workspace_name, &borrowed) } @@ -247,14 +263,7 @@ pub(crate) fn run_workspace_git_bytes( repo_subpath: Option<&str>, git_args: &[&str], ) -> Result, blox::BloxError> { - let mut owned = Vec::::new(); - owned.push("git".to_string()); - if let Some(subpath) = repo_subpath.map(str::trim).filter(|s| !s.is_empty()) { - let resolved = resolve_workspace_repo_path(workspace_name, subpath)?; - owned.push("-C".to_string()); - owned.push(resolved); - } - owned.extend(git_args.iter().map(|arg| (*arg).to_string())); + let owned = workspace_git_args(workspace_name, repo_subpath, git_args)?; let borrowed = owned.iter().map(String::as_str).collect::>(); blox::ws_exec_bytes(workspace_name, &borrowed) } @@ -368,6 +377,94 @@ pub(crate) fn resolve_branch_workspace_subpath( Ok(Some(workspace_path)) } +// ── Per-branch git runners ────────────────────────────────────────────────── +// +// Any read *about* a branch has to happen *in* the branch's own checkout. On a +// Blox workspace that is not the same thing as a bare exec: one project gets +// one workspace (`resolve_project_workspace_name`) and every additional repo +// is cloned into it as a sibling (`clone_repo_into_workspace`), so at most one +// clone can be whatever directory a bare `sq blox ws exec … git rev-parse +// HEAD` lands in. These runners resolve the same directory the rebase +// (`run_remote_pipeline_command`), the remote agent, and the diff collector +// already run in. + +/// A git command runner bound to one checkout: local commands run in +/// `working_dir`, remote ones in `remote_dir` on `workspace_name`. +/// +/// Boxed rather than an `impl Fn`, since a named type keeps the return +/// signatures readable and every consumer is generic over `Fn` anyway. +pub(crate) type GitRunner<'a> = Box Result + 'a>; + +/// Build a runner for an already-resolved remote directory. +/// +/// `remote_dir` takes either form the codebase produces — the `home:` +/// string from [`resolve_branch_workspace_subpath`] or the absolute path a +/// session config carries as `remote_working_dir` — because +/// [`resolve_workspace_repo_path`] maps both to the same `-C` argument. `None` +/// runs bare git, which is correct for a branch with no project repo (and is +/// what every remote read did before). It is owned rather than borrowed +/// because callers that resolve it from a branch row produce it on the fly. +pub(crate) fn git_runner<'a>( + working_dir: &'a Path, + workspace_name: Option<&'a str>, + remote_dir: Option, +) -> GitRunner<'a> { + Box::new(move |args: &[&str]| match workspace_name { + Some(ws_name) => { + run_workspace_git(ws_name, remote_dir.as_deref(), args).map_err(|e| e.to_string()) + } + None => git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), + }) +} + +/// The same runner, resolving the remote directory from the branch row. +/// +/// For callers that hold a `Branch` but no session config. The resolution is +/// the one such a config's `remote_working_dir` was built from, so the two +/// agree byte for byte. +pub(crate) fn branch_git_runner<'a>( + store: &Store, + branch: &store::Branch, + working_dir: &'a Path, + workspace_name: Option<&'a str>, +) -> Result, String> { + let remote_dir = match workspace_name { + Some(_) => resolve_branch_workspace_subpath(store, branch)?, + None => None, + }; + Ok(git_runner(working_dir, workspace_name, remote_dir)) +} + +/// Read HEAD through a runner, trimmed. +pub(crate) fn head_sha(git: &GitRunner<'_>) -> Result { + git(&["rev-parse", "HEAD"]).map(|sha| sha.trim().to_string()) +} + +/// A branch's HEAD, read off the checkout that branch lives in. +/// +/// Goes to a blocking thread because on a remote branch it is a `ws exec` +/// round trip to a cloud workstation. +pub(crate) async fn branch_head_sha( + store: &Arc, + branch: &store::Branch, + working_dir: &Path, +) -> Result { + let store = Arc::clone(store); + let branch = branch.clone(); + let working_dir = working_dir.to_path_buf(); + tauri::async_runtime::spawn_blocking(move || { + let git = branch_git_runner( + &store, + &branch, + &working_dir, + branch.workspace_name.as_deref(), + )?; + head_sha(&git) + }) + .await + .map_err(|e| format!("HEAD lookup task failed: {e}"))? +} + pub(crate) fn normalize_branch_ref(branch: &str) -> String { branch.strip_prefix("origin/").unwrap_or(branch).to_string() } @@ -2644,6 +2741,77 @@ mod tests { use super::*; use crate::test_utils::TempGitRepo; + /// The rule every remote read of a branch's HEAD rests on: a branch with + /// a project repo is pinned to that repo's clone directory, so it can't + /// answer about a sibling clone that happens to be the bare exec cwd. + #[test] + fn workspace_git_pins_a_branch_with_a_repo_to_its_clone_dir() { + let store = Store::in_memory().unwrap(); + let project = store::Project::new("squareup/g2"); + store.create_project(&project).unwrap(); + let repo = store::ProjectRepo::new( + &project.id, + "block/builderbot", + "feature", + Some("apps/staged".to_string()), + ); + store.create_project_repo(&repo).unwrap(); + let branch = store::Branch::new_remote(&project.id, "feature", "main", "ws-1") + .with_project_repo(&repo.id); + + let subpath = resolve_branch_workspace_subpath(&store, &branch) + .unwrap() + .unwrap(); + assert_eq!( + workspace_git_args("ws-1", Some(&subpath), &["rev-parse", "HEAD"]).unwrap(), + vec![ + "git", + "-C", + "/home/bloxer/builderbot/apps/staged", + "rev-parse", + "HEAD" + ] + ); + } + + /// A branch with no project repo (a pre-Staged-repo branch) has no clone + /// dir to resolve, so it runs bare — byte-identical to a plain `ws_exec`. + #[test] + fn workspace_git_runs_bare_without_a_repo() { + let store = Store::in_memory().unwrap(); + let project = store::Project::new("squareup/g2"); + store.create_project(&project).unwrap(); + let branch = store::Branch::new_remote(&project.id, "feature", "main", "ws-1"); + + assert!(resolve_branch_workspace_subpath(&store, &branch) + .unwrap() + .is_none()); + assert_eq!( + workspace_git_args("ws-1", None, &["rev-parse", "HEAD"]).unwrap(), + vec!["git", "rev-parse", "HEAD"] + ); + } + + /// A session config's already-absolute `remote_working_dir` resolves to the + /// same `-C` as the `home:` form, so the two runners can't disagree. + #[test] + fn workspace_git_accepts_an_already_resolved_remote_dir() { + assert_eq!( + workspace_git_args( + "ws-1", + Some("/home/bloxer/builderbot/apps/staged"), + &["rev-parse", "HEAD"] + ) + .unwrap(), + workspace_git_args( + "ws-1", + Some("home:builderbot/apps/staged"), + &["rev-parse", "HEAD"] + ) + .unwrap() + ); + } + #[test] fn apply_branch_prefix_joins_with_slash() { assert_eq!( diff --git a/apps/staged/src-tauri/src/commit_reassociation.rs b/apps/staged/src-tauri/src/commit_reassociation.rs index e122b1ee6..695c2f3a2 100644 --- a/apps/staged/src-tauri/src/commit_reassociation.rs +++ b/apps/staged/src-tauri/src/commit_reassociation.rs @@ -14,9 +14,9 @@ //! can read the old metadata back and match it against the rewritten commits. use std::collections::{HashMap, HashSet, VecDeque}; -use std::path::Path; -use crate::store::Store; +use crate::branches::GitRunner; +use crate::store::{Branch, Store}; /// The commit metadata `git rebase` carries across a rewrite. Two commits with /// the same identity are the same commit before and after a rebase. @@ -113,6 +113,11 @@ pub enum Reassociation { /// Repoint a branch's orphaned commit rows (and their reviews) at the SHAs a /// rebase rewrote them into. /// +/// `git` must be bound to the branch's own checkout — see +/// [`crate::branches::branch_git_runner`]. Every read here is relative to +/// wherever it points, so a runner aimed at a sibling clone on a shared +/// workspace would compare one repo's rows against another repo's commits. +/// /// Safe to call when nothing was rewritten: with no orphaned rows it stops /// after listing the branch and reports `Done { remapped: 0 }`. Also safe to /// call while a rebase is still in flight — it reports @@ -123,20 +128,12 @@ pub enum Reassociation { /// failures *after* it passed. pub fn reassociate_after_rebase( store: &Store, - branch_id: &str, - working_dir: &Path, - workspace_name: Option<&str>, + branch: &Branch, + git: &GitRunner<'_>, ) -> Result { - let (branch, git) = match branch_git_runner(store, branch_id, working_dir, workspace_name) { - Ok(pair) => pair, - Err(e) => { - log::warn!("Failed to resolve branch {branch_id} for commit reassociation: {e}"); - return Ok(Reassociation::MidRebase); - } - }; - + let branch_id = branch.id.as_str(); let branch_name = crate::git::branch_name_without_origin(&branch.branch_name); - if !head_is_on_branch(&git, branch_name) { + if !head_is_on_branch(git, branch_name) { // Routine on a rebase-handoff turn that ended with conflicts // unresolved, so an expected state rather than an anomaly. log::info!( @@ -147,7 +144,7 @@ pub fn reassociate_after_rebase( } let base_ref = crate::git::origin_ref_for_branch(&branch.base_branch); - let on_branch = list_branch_commits(&git, &base_ref)?; + let on_branch = list_branch_commits(git, &base_ref)?; // `list_commits_for_branch` orders by `created_at`, which is the order the // sessions authored them — i.e. branch order, as the matcher requires. @@ -175,7 +172,7 @@ pub fn reassociate_after_rebase( return Ok(Reassociation::Done { remapped: 0 }); } - let mut old_identities = lookup_commit_identities(&git, &orphan_shas)?; + let mut old_identities = lookup_commit_identities(git, &orphan_shas)?; let orphans: Vec = rows .iter() .filter_map(|row| { @@ -205,41 +202,6 @@ pub fn reassociate_after_rebase( Ok(Reassociation::Done { remapped }) } -/// The git runner [`branch_git_runner`] hands back. Boxed rather than an `impl -/// Fn`, since a named type keeps the return signature readable and every -/// consumer here is generic over `Fn` anyway. -type GitRunner<'a> = Box Result + 'a>; - -/// Resolve the branch row and build the git runner [`reassociate_after_rebase`] -/// works through: local commands run in `working_dir`, remote ones through the -/// branch's workspace (and its repo subpath). -fn branch_git_runner<'a>( - store: &Store, - branch_id: &str, - working_dir: &'a Path, - workspace_name: Option<&'a str>, -) -> Result<(crate::store::Branch, GitRunner<'a>), String> { - let branch = store - .get_branch(branch_id) - .map_err(|e| e.to_string())? - .ok_or_else(|| format!("Branch not found: {branch_id}"))?; - - let repo_subpath = match workspace_name { - Some(_) => crate::branches::resolve_branch_workspace_subpath(store, &branch)?, - None => None, - }; - let git = move |args: &[&str]| -> Result { - match workspace_name { - Some(ws_name) => { - crate::branches::run_workspace_git(ws_name, repo_subpath.as_deref(), args) - .map_err(|e| e.to_string()) - } - None => crate::git::cli_run_smart(working_dir, args).map_err(|e| e.to_string()), - } - }; - Ok((branch, Box::new(git))) -} - /// Whether HEAD is the branch we're about to reassociate, rather than a /// detached commit. /// diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 0d5006c7c..190f54fab 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -85,17 +85,6 @@ pub(crate) fn resolve_branch_repo_slug( project.primary_repo().map(|s| s.to_string()) } -pub(crate) async fn run_blox_blocking(op: F) -> Result -where - T: Send + 'static, - F: FnOnce() -> Result + Send + 'static, -{ - tauri::async_runtime::spawn_blocking(op) - .await - .map_err(|e| format!("blox task failed: {e}"))? - .map_err(|e| e.to_string()) -} - fn bundled_pikchr_grammar_path(app_handle: &tauri::AppHandle) -> Option { if let Ok(path) = app_handle .path() @@ -1165,17 +1154,9 @@ pub(crate) async fn resume_session_for_store( if let Some(ref branch) = linked_branch { let ws_name = branch.workspace_name.clone(); let head = if linked_commit.is_some() { - if let Some(ref ws) = ws_name { - let ws = ws.clone(); - run_blox_blocking(move || { - crate::blox::ws_exec(&ws, &["git", "rev-parse", "HEAD"]) - }) + crate::branches::branch_head_sha(&store, branch, &working_dir) .await - .map(|s| s.trim().to_string()) .ok() - } else { - crate::git::get_head_sha(&working_dir).ok() - } } else { None }; @@ -2267,6 +2248,44 @@ fn resolve_branch_session_provider( } } +/// A commit session's `pre_head_sha`, read off the branch's own checkout. +/// +/// A remote lookup failure only costs commit detection for this session, so it +/// degrades to `None`; a local one means the worktree is unusable, so it fails +/// the command — the split each capture site hand-rolled before. +async fn commit_pre_head_sha( + store: &Arc, + branch: &store::Branch, + working_dir: &Path, +) -> Result, String> { + match crate::branches::branch_head_sha(store, branch, working_dir).await { + Ok(sha) => Ok(Some(sha)), + Err(e) if branch.workspace_name.is_some() => { + log::warn!( + "Failed to get remote HEAD SHA for branch {}: {e}", + branch.id + ); + Ok(None) + } + Err(e) => Err(format!("Failed to get HEAD SHA: {e}")), + } +} + +/// The tip SHA a review anchors to, read off the branch's own checkout. +/// +/// `reviews.commit_sha` is matched against the branch's commits by +/// `review_is_visible_in_timeline`, so a SHA from a sibling clone would hide +/// the review. Failure degrades the same way as [`commit_pre_head_sha`]. +async fn review_tip_sha( + store: &Arc, + branch: &store::Branch, + working_dir: &Path, +) -> Result { + Ok(commit_pre_head_sha(store, branch, working_dir) + .await? + .unwrap_or_else(|| "unknown".to_string())) +} + #[allow(clippy::too_many_arguments)] async fn prepare_branch_session_start( store: &Arc, @@ -2356,40 +2375,13 @@ async fn prepare_branch_session_start( }; let pre_head_sha = if matches!(session_type, BranchSessionType::Commit) { - if is_remote { - let workspace_name = branch.workspace_name.as_deref().unwrap().to_string(); - match run_blox_blocking(move || { - blox::ws_exec(&workspace_name, &["git", "rev-parse", "HEAD"]) - }) - .await - { - Ok(sha) => Some(sha.trim().to_string()), - Err(e) => { - log::warn!("Failed to get remote HEAD SHA via ws_exec: {e}"); - None - } - } - } else { - Some( - git::get_head_sha(&working_dir) - .map_err(|e| format!("Failed to get HEAD SHA: {e}"))?, - ) - } + commit_pre_head_sha(store, &branch, &working_dir).await? } else { None }; let review_tip_sha = if matches!(session_type, BranchSessionType::Review) { - let tip_sha = if is_remote { - let workspace_name = branch.workspace_name.as_deref().unwrap().to_string(); - run_blox_blocking(move || blox::ws_exec(&workspace_name, &["git", "rev-parse", "HEAD"])) - .await - .map(|s| s.trim().to_string()) - .unwrap_or_else(|_| "unknown".to_string()) - } else { - git::get_head_sha(&working_dir).map_err(|e| format!("Failed to get HEAD SHA: {e}"))? - }; - Some(tip_sha) + Some(review_tip_sha(store, &branch, &working_dir).await?) } else { None }; @@ -3042,15 +3034,7 @@ async fn start_queued_session_for_branch( // At queue time, reviews are created with an empty commit_sha since the // workspace may not exist yet. if let Some(ref review_id) = schedule.review_id { - let tip_sha = if is_remote { - let workspace_name = branch.workspace_name.as_deref().unwrap().to_string(); - run_blox_blocking(move || blox::ws_exec(&workspace_name, &["git", "rev-parse", "HEAD"])) - .await - .map(|s| s.trim().to_string()) - .unwrap_or_else(|_| "unknown".to_string()) - } else { - git::get_head_sha(&working_dir).map_err(|e| format!("Failed to get HEAD SHA: {e}"))? - }; + let tip_sha = review_tip_sha(&store, &branch, &working_dir).await?; store .update_review_commit_sha(review_id, &tip_sha) .map_err(|e| e.to_string())?; @@ -3058,27 +3042,7 @@ async fn start_queued_session_for_branch( // Compute pre-head SHA for commit sessions. let pre_head_sha = match session_type { - BranchSessionType::Commit => { - if is_remote { - let workspace_name = branch.workspace_name.as_deref().unwrap().to_string(); - match run_blox_blocking(move || { - blox::ws_exec(&workspace_name, &["git", "rev-parse", "HEAD"]) - }) - .await - { - Ok(sha) => Some(sha.trim().to_string()), - Err(e) => { - log::warn!("Failed to get remote HEAD SHA via ws_exec: {e}"); - None - } - } - } else { - Some( - git::get_head_sha(&working_dir) - .map_err(|e| format!("Failed to get HEAD SHA: {e}"))?, - ) - } - } + BranchSessionType::Commit => commit_pre_head_sha(&store, &branch, &working_dir).await?, _ => None, }; @@ -3414,15 +3378,7 @@ pub async fn trigger_auto_review( }; // Get the current tip SHA for the review anchor - let tip_sha = if is_remote { - let workspace_name = branch.workspace_name.as_deref().unwrap().to_string(); - run_blox_blocking(move || blox::ws_exec(&workspace_name, &["git", "rev-parse", "HEAD"])) - .await - .map(|s| s.trim().to_string()) - .unwrap_or_else(|_| "unknown".to_string()) - } else { - git::get_head_sha(&working_dir).map_err(|e| format!("Failed to get HEAD SHA: {e}"))? - }; + let tip_sha = review_tip_sha(&store, &branch, &working_dir).await?; // Build the full prompt (reuse Review prompt) let prompt = "Review the latest changes on this branch.".to_string(); diff --git a/apps/staged/src-tauri/src/session_runner.rs b/apps/staged/src-tauri/src/session_runner.rs index fbe2c01c5..af4502025 100644 --- a/apps/staged/src-tauri/src/session_runner.rs +++ b/apps/staged/src-tauri/src/session_runner.rs @@ -35,7 +35,7 @@ use std::collections::HashMap; use std::io; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use std::process::{Command, Output, Stdio}; use std::sync::{Arc, OnceLock}; use std::time::Duration; @@ -834,6 +834,10 @@ pub fn start_session( &config.working_dir, config.pre_head_sha.as_deref(), config.workspace_name.as_deref(), + config + .remote_working_dir + .as_deref() + .and_then(|dir| dir.to_str()), &store_for_status, ) } else { @@ -1421,14 +1425,24 @@ fn pre_head_for_pipeline_handoff(config: &PipelineConfig) -> Option { } } +/// A git runner for the checkout this pipeline runs in. `remote_working_dir` +/// is the literal directory `run_remote_pipeline_command` `cd`s into, so every +/// read through this runner is about the same repo the pipeline's own commands +/// were — not whatever sibling clone a bare `ws exec` happens to land in. +fn pipeline_git_runner(config: &PipelineConfig) -> crate::branches::GitRunner<'_> { + crate::branches::git_runner( + &config.working_dir, + config.workspace_name.as_deref(), + config + .remote_working_dir + .as_deref() + .and_then(|dir| dir.to_str()) + .map(str::to_string), + ) +} + fn current_pipeline_head(config: &PipelineConfig) -> Result { - if let Some(ws_name) = config.workspace_name.as_deref() { - crate::blox::ws_exec(ws_name, &["git", "rev-parse", "HEAD"]) - .map(|s| s.trim().to_string()) - .map_err(|e| e.to_string()) - } else { - crate::git::get_head_sha(&config.working_dir).map_err(|e| e.to_string()) - } + crate::branches::head_sha(&pipeline_git_runner(config)) } /// Whether a session's persisted pipeline is a rebase. Read from the session @@ -1449,22 +1463,33 @@ fn session_is_rebase_pipeline(store: &Store, session_id: &str) -> bool { /// must not touch a row mid-rebase gate on the returned /// [`Reassociation::MidRebase`]. /// +/// `git` is the caller's own runner, so the reassociation reads the checkout +/// the caller just read HEAD from rather than resolving a second one. +/// /// Best-effort: a failure here only leaves the rows orphaned, which is what /// happened before reassociation existed, so an `Err` folds into `Done`. It can /// only be raised once HEAD was seen attached, so the caller's detection should /// carry on regardless — `MidRebase` is the only answer that means "defer". +/// A branch row that won't load is the exception: nothing was checked, so it +/// reads as `MidRebase`, the same "don't touch anything" direction. fn reassociate_rebased_commits( store: &Store, branch_id: &str, - working_dir: &Path, - workspace_name: Option<&str>, + git: &crate::branches::GitRunner<'_>, ) -> Reassociation { - match crate::commit_reassociation::reassociate_after_rebase( - store, - branch_id, - working_dir, - workspace_name, - ) { + let branch = match store.get_branch(branch_id) { + Ok(Some(branch)) => branch, + Ok(None) => { + log::warn!("Branch {branch_id} not found for commit reassociation"); + return Reassociation::MidRebase; + } + Err(e) => { + log::warn!("Failed to load branch {branch_id} for commit reassociation: {e}"); + return Reassociation::MidRebase; + } + }; + + match crate::commit_reassociation::reassociate_after_rebase(store, &branch, git) { Ok(Reassociation::Done { remapped: 0 }) => Reassociation::Done { remapped: 0 }, Ok(Reassociation::Done { remapped }) => { log::info!("Reassociated {remapped} rebased commit(s) on branch {branch_id}"); @@ -1514,7 +1539,10 @@ fn finalize_rebase_pipeline_without_ai(config: &PipelineConfig, store: &Store) { } }; - let current_head = match current_pipeline_head(config) { + // One runner for both reads below, so the HEAD this claims and the commits + // reassociation lists can't come from different checkouts. + let git = pipeline_git_runner(config); + let current_head = match crate::branches::head_sha(&git) { Ok(head) => head, Err(e) => { log::error!( @@ -1548,12 +1576,7 @@ fn finalize_rebase_pipeline_without_ai(config: &PipelineConfig, store: &Store) { // to claim, so the returned outcome isn't a gate here: a `MidRebase` answer // (an unattached HEAD, or a branch that wouldn't resolve) just means the // rows stay orphaned while the claim below proceeds as it always has. - let _ = reassociate_rebased_commits( - store, - &commit.branch_id, - &config.working_dir, - config.workspace_name.as_deref(), - ); + let _ = reassociate_rebased_commits(store, &commit.branch_id, &git); match store.complete_pending_commit_sha(&commit.id, &commit.branch_id, ¤t_head) { Ok(true) => log::info!( @@ -2368,6 +2391,7 @@ fn run_post_completion_hooks( working_dir: &std::path::Path, pre_head_sha: Option<&str>, workspace_name: Option<&str>, + remote_working_dir: Option<&str>, store: &Arc, ) -> Option { let mut committed_branch_id: Option = None; @@ -2377,16 +2401,18 @@ fn run_post_completion_hooks( // Look for any commit linked to this session — not just pending (sha IS NULL) // ones — so we also detect amended commits on resumed sessions. if let Ok(Some(commit)) = store.get_commit_by_session(session_id) { - // Get current HEAD — either from local worktree or remote workspace. - let current_head_result = if let Some(ws_name) = workspace_name { - crate::blox::ws_exec(ws_name, &["git", "rev-parse", "HEAD"]) - .map(|s| s.trim().to_string()) - .map_err(|e| format!("{e}")) - } else { - crate::git::get_head_sha(working_dir).map_err(|e| format!("{e}")) - }; + // Get current HEAD from the checkout the session ran in — the + // branch's clone directory on a remote workspace, which is not + // where a bare `ws exec` lands when the workspace holds more than + // one repo. The same runner is handed to reassociation below, so + // the two can't end up describing different repos. + let git = crate::branches::git_runner( + working_dir, + workspace_name, + remote_working_dir.map(str::to_string), + ); - match current_head_result { + match crate::branches::head_sha(&git) { Ok(current_head) if current_head != pre_sha => { // A rebase pipeline that handed off to AI (conflicts, a // failed fetch) lands here instead of @@ -2414,12 +2440,8 @@ fn run_post_completion_hooks( // why — true by construction. let rebase_pipeline = session_is_rebase_pipeline(store, session_id); let mid_rebase = rebase_pipeline - && reassociate_rebased_commits( - store, - &commit.branch_id, - working_dir, - workspace_name, - ) == Reassociation::MidRebase; + && reassociate_rebased_commits(store, &commit.branch_id, &git) + == Reassociation::MidRebase; if mid_rebase { log::info!( "Session {session_id}: rebase still in flight (HEAD detached), \ @@ -3903,6 +3925,7 @@ mod tests { fixture.repo.path(), Some(&fixture.old_head), None, + None, &fixture.store, ); @@ -4003,6 +4026,7 @@ mod tests { fixture.repo.path(), Some(&fixture.old_head), None, + None, &fixture.store, ) } @@ -4085,6 +4109,7 @@ mod tests { repo.path(), Some(&detached_head), None, + None, &fixture.store, ); @@ -4130,6 +4155,7 @@ mod tests { repo.path(), Some(&detached_head), None, + None, &fixture.store, ); @@ -4362,6 +4388,7 @@ Solid changes with minor nit std::path::Path::new("/tmp"), None, None, + None, &store, ); @@ -4390,6 +4417,7 @@ Solid changes with minor nit std::path::Path::new("/tmp"), None, None, + None, &store, ); @@ -4410,6 +4438,7 @@ Solid changes with minor nit std::path::Path::new("/tmp"), None, None, + None, &store, ); diff --git a/apps/staged/src-tauri/src/web_server.rs b/apps/staged/src-tauri/src/web_server.rs index 22009fd17..7a1206de5 100644 --- a/apps/staged/src-tauri/src/web_server.rs +++ b/apps/staged/src-tauri/src/web_server.rs @@ -2971,17 +2971,9 @@ async fn dispatch(command: &str, args: Value, state: &WebAppState) -> Result