diff --git a/src/daemon.rs b/src/daemon.rs index 73cb41cde..004fb9b5e 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -2669,22 +2669,33 @@ fn strict_cherry_pick_mappings_from_command( context, new_head ))); } - let mut source_commits = pending_source_commits; - if source_commits.is_empty() { - source_commits = cherry_pick_source_commits_from_command(cmd); - } - if source_commits.is_empty() { - return Err(GitAiError::Generic(format!( - "{} missing cherry-pick source commits", - context - ))); - } let worktree = cmd.worktree.as_deref().ok_or_else(|| { GitAiError::Generic(format!( "{} missing worktree for cherry-pick mapping new={}", context, new_head )) })?; + // Resolve source commits: prefer pending (cached from start event), fall + // back to parsing command args. Either path may contain short SHAs or + // symbolic refs, so resolve them to full OIDs via git rev-parse. This + // runs in the async side-effect path, not the daemon critical path. + let mut source_refs = pending_source_commits; + if source_refs.is_empty() { + source_refs = cherry_pick_source_refs_from_command(cmd); + } + if source_refs.is_empty() { + return Err(GitAiError::Generic(format!( + "{} missing cherry-pick source commits", + context + ))); + } + let source_commits = resolve_cherry_pick_source_refs(&source_refs, worktree, context)?; + if source_commits.is_empty() { + return Err(GitAiError::Generic(format!( + "{} cherry-pick source refs resolved to no valid commits", + context + ))); + } let (original_head, new_commits) = resolve_linear_head_commit_chain_for_worktree( worktree, new_head, @@ -2703,13 +2714,12 @@ fn strict_cherry_pick_mappings_from_command( Ok((original_head, source_commits, new_commits)) } -fn append_unique_oid(target: &mut Vec, value: &str) { - if is_valid_oid(value) && !is_zero_oid(value) && !target.iter().any(|seen| seen == value) { - target.push(value.to_string()); - } -} - -fn cherry_pick_source_commits_from_command( +/// Collect positional arguments from a cherry-pick command as potential commit +/// references. Unlike the full-OID-only `is_valid_oid` check, this accepts short SHA prefixes and +/// symbolic refs (e.g. branch names) that git would resolve on the command line. +/// Resolution to full OIDs happens later in `resolve_cherry_pick_source_refs` +/// which runs in the async side-effect path. +fn cherry_pick_source_refs_from_command( cmd: &crate::daemon::domain::NormalizedCommand, ) -> Vec { let mut out = Vec::new(); @@ -2733,11 +2743,45 @@ fn cherry_pick_source_commits_from_command( if arg.starts_with('-') { continue; } - append_unique_oid(&mut out, arg); + if !arg.is_empty() && !out.iter().any(|seen: &String| seen == arg) { + out.push(arg.to_string()); + } } out } +/// Resolve cherry-pick source refs (which may be short SHAs, branch names, or +/// full OIDs) to full commit OIDs. This calls `git rev-parse` and MUST only be +/// invoked from the async side-effect path, never the daemon critical path. +fn resolve_cherry_pick_source_refs( + source_refs: &[String], + worktree: &Path, + context: &str, +) -> Result, GitAiError> { + let mut resolved = Vec::new(); + let repo = find_repository_in_path(worktree.to_string_lossy().as_ref())?; + for src in source_refs { + if is_valid_oid(src) && !is_zero_oid(src) { + resolved.push(src.clone()); + } else { + let obj = repo.revparse_single(src).map_err(|err| { + GitAiError::Generic(format!( + "{} failed to resolve cherry-pick source ref '{}': {}", + context, src, err + )) + })?; + let oid = obj + .peel_to_commit() + .map(|c| c.id()) + .unwrap_or_else(|_| obj.id()); + if is_valid_oid(&oid) && !is_zero_oid(&oid) { + resolved.push(oid); + } + } + } + Ok(resolved) +} + fn rebase_is_control_mode(cmd: &crate::daemon::domain::NormalizedCommand) -> bool { summarize_rebase_args(&cmd.invoked_args).is_control_mode } @@ -5972,8 +6016,8 @@ impl ActorDaemonCoordinator { if cmd.invoked_args.iter().any(|arg| arg == "--abort") { self.clear_pending_cherry_pick_sources_for_worktree(worktree)?; } else if cmd.exit_code != 0 { - let source_commits = cherry_pick_source_commits_from_command(cmd); - self.set_pending_cherry_pick_sources_for_worktree(worktree, source_commits)?; + let source_refs = cherry_pick_source_refs_from_command(cmd); + self.set_pending_cherry_pick_sources_for_worktree(worktree, source_refs)?; } } return Ok(()); diff --git a/tests/daemon_mode.rs b/tests/daemon_mode.rs index 94f18beb5..55501350e 100644 --- a/tests/daemon_mode.rs +++ b/tests/daemon_mode.rs @@ -2557,6 +2557,242 @@ fn daemon_pure_trace_socket_cherry_pick_continue_emits_complete_event() { ); } +#[test] +#[serial] +fn daemon_pure_trace_socket_rebase_with_short_sha_emits_complete_event() { + let repo = TestRepo::new_with_mode(GitTestMode::Wrapper); + let _daemon = DaemonGuard::start(&repo); + let trace_socket = daemon_trace_socket_path(&repo); + let env = git_trace_env(&trace_socket); + let env_refs = [(env[0].0, env[0].1.as_str()), (env[1].0, env[1].1.as_str())]; + let default_branch = repo.current_branch(); + let completion_baseline = repo.daemon_total_completion_count(); + let mut expected_top_level_completions = 0u64; + + // Create base commit on default branch + fs::write(repo.path().join("rebase-short.txt"), "base\n").expect("failed to write base"); + traced_git_with_env( + &repo, + &["add", "rebase-short.txt"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("add should succeed"); + traced_git_with_env( + &repo, + &["commit", "-m", "base"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("base commit should succeed"); + + // Create feature branch with a commit + traced_git_with_env( + &repo, + &["checkout", "-b", "feature-rebase-short"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("feature branch checkout should succeed"); + fs::write(repo.path().join("feature-only.txt"), "feature content\n") + .expect("failed to write feature file"); + traced_git_with_env( + &repo, + &["add", "feature-only.txt"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("feature add should succeed"); + traced_git_with_env( + &repo, + &["commit", "-m", "feature change"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("feature commit should succeed"); + + // Go back to default branch and add a non-conflicting commit + traced_git_with_env( + &repo, + &["checkout", default_branch.as_str()], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("checkout default should succeed"); + fs::write(repo.path().join("main-only.txt"), "main content\n") + .expect("failed to write main file"); + traced_git_with_env( + &repo, + &["add", "main-only.txt"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("main add should succeed"); + traced_git_with_env( + &repo, + &["commit", "-m", "main advance"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("main commit should succeed"); + + // Get the short SHA of the latest main commit + let main_full_sha = repo + .git(&["rev-parse", "HEAD"]) + .expect("HEAD rev-parse should succeed") + .trim() + .to_string(); + let main_short_sha = &main_full_sha[..7]; + + // Switch to feature branch and rebase onto main using SHORT SHA + traced_git_with_env( + &repo, + &["checkout", "feature-rebase-short"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("checkout feature should succeed"); + traced_git_with_env( + &repo, + &["rebase", main_short_sha], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("rebase with short SHA should succeed"); + + wait_for_expected_top_level_completions( + &repo, + completion_baseline, + expected_top_level_completions, + ); + + let rewrite_log_path = git_common_dir(&repo).join("ai").join("rewrite_log"); + let rewrite_log = fs::read_to_string(&rewrite_log_path) + .expect("rewrite log should exist after rebase with short SHA"); + assert!( + rewrite_log + .lines() + .any(|line| line.contains("\"rebase_complete\"")), + "daemon should emit rebase_complete even when rebase uses a short SHA, rewrite_log: {}", + rewrite_log + ); +} + +#[test] +#[serial] +fn daemon_pure_trace_socket_cherry_pick_with_short_sha_emits_complete_event() { + let repo = TestRepo::new_with_mode(GitTestMode::Wrapper); + let _daemon = DaemonGuard::start(&repo); + let trace_socket = daemon_trace_socket_path(&repo); + let env = git_trace_env(&trace_socket); + let env_refs = [(env[0].0, env[0].1.as_str()), (env[1].0, env[1].1.as_str())]; + let default_branch = repo.current_branch(); + let completion_baseline = repo.daemon_total_completion_count(); + let mut expected_top_level_completions = 0u64; + + // Create base commit + fs::write(repo.path().join("short-sha-test.txt"), "base\n").expect("failed to write base"); + traced_git_with_env( + &repo, + &["add", "short-sha-test.txt"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("add should succeed"); + traced_git_with_env( + &repo, + &["commit", "-m", "base"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("base commit should succeed"); + + // Create topic branch with a commit + traced_git_with_env( + &repo, + &["checkout", "-b", "topic-short-sha"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("topic branch checkout should succeed"); + fs::write(repo.path().join("short-sha-test.txt"), "topic content\n") + .expect("failed to write topic change"); + traced_git_with_env( + &repo, + &["add", "short-sha-test.txt"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("topic add should succeed"); + traced_git_with_env( + &repo, + &["commit", "-m", "topic change"], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("topic commit should succeed"); + + // Get the full SHA and derive a short (7-char) prefix + let topic_full_sha = repo + .git(&["rev-parse", "topic-short-sha"]) + .expect("topic rev-parse should succeed") + .trim() + .to_string(); + let topic_short_sha = &topic_full_sha[..7]; + + // Switch back to default branch + traced_git_with_env( + &repo, + &["checkout", default_branch.as_str()], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("checkout default branch should succeed"); + + // Cherry-pick using the SHORT SHA -- this is the key part of the test + traced_git_with_env( + &repo, + &["cherry-pick", topic_short_sha], + &env_refs, + &mut expected_top_level_completions, + ) + .expect("cherry-pick with short SHA should succeed"); + + wait_for_expected_top_level_completions( + &repo, + completion_baseline, + expected_top_level_completions, + ); + + let rewrite_log_path = git_common_dir(&repo).join("ai").join("rewrite_log"); + let rewrite_log = fs::read_to_string(&rewrite_log_path) + .expect("rewrite log should exist after cherry-pick with short SHA"); + assert!( + rewrite_log + .lines() + .any(|line| line.contains("\"cherry_pick_complete\"")), + "daemon should emit cherry_pick_complete even when cherry-pick uses a short SHA, rewrite_log: {}", + rewrite_log + ); + + // Verify the source commits in the event contain the FULL SHA, not the short one + for line in rewrite_log.lines() { + if line.contains("\"cherry_pick_complete\"") { + assert!( + line.contains(&topic_full_sha), + "cherry_pick_complete event should contain full resolved SHA {}, got: {}", + topic_full_sha, + line + ); + assert!( + !line.contains(&format!("\"{}\"", topic_short_sha)) + || line.contains(&topic_full_sha), + "cherry_pick_complete should not contain unresolved short SHA" + ); + } + } +} + #[test] #[serial] fn daemon_pure_trace_socket_switch_tracks_success_and_conflict_failure() {