From 373f2bfb566e67f6a463f1f34cf9533c45e1cd32 Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Fri, 5 Jun 2026 00:47:04 +0000 Subject: [PATCH] fix rebase conflict resolution attribution --- src/authorship/conflict_resolution.rs | 269 +++++++++++++ src/authorship/mod.rs | 1 + src/authorship/post_commit.rs | 24 ++ src/authorship/rewrite.rs | 45 ++- src/commands/checkpoint_agent/orchestrator.rs | 51 +-- src/daemon.rs | 107 ++--- .../integration/checkpoint_explicit_paths.rs | 14 +- tests/integration/checkpoint_unit.rs | 61 ++- tests/integration/pull_rebase_ff.rs | 368 ++++++++++++++++++ 9 files changed, 785 insertions(+), 155 deletions(-) create mode 100644 src/authorship/conflict_resolution.rs diff --git a/src/authorship/conflict_resolution.rs b/src/authorship/conflict_resolution.rs new file mode 100644 index 0000000000..75d533b863 --- /dev/null +++ b/src/authorship/conflict_resolution.rs @@ -0,0 +1,269 @@ +use std::collections::{HashMap, HashSet}; + +use crate::authorship::authorship_log::LineRange; +use crate::authorship::authorship_log_serialization::{AttestationEntry, AuthorshipLog}; +use crate::authorship::imara_diff_utils::{DiffOp, capture_diff_slices}; +use crate::git::repository::Repository; + +fn normalize_line_ranges(ranges: &[LineRange]) -> Vec { + let mut lines: Vec = ranges.iter().flat_map(LineRange::expand).collect(); + lines.sort_unstable(); + lines.dedup(); + LineRange::compress_lines(&lines) +} + +fn subtract_line_ranges(ranges: &[LineRange], covered: &[LineRange]) -> Vec { + let mut remaining = ranges.to_vec(); + for covered_range in covered { + remaining = remaining + .iter() + .flat_map(|range| range.remove(covered_range)) + .collect(); + if remaining.is_empty() { + break; + } + } + normalize_line_ranges(&remaining) +} + +fn line_coverage_by_file(log: &AuthorshipLog) -> HashMap> { + let mut coverage: HashMap> = HashMap::new(); + for attestation in &log.attestations { + let file_coverage = coverage.entry(attestation.file_path.clone()).or_default(); + for entry in &attestation.entries { + file_coverage.extend(entry.line_ranges.clone()); + } + } + for ranges in coverage.values_mut() { + *ranges = normalize_line_ranges(ranges); + } + coverage +} + +fn attestation_metadata_key(hash: &str) -> &str { + hash.split("::").next().unwrap_or(hash) +} + +fn retain_referenced_metadata(log: &mut AuthorshipLog) { + let mut prompt_keys = HashSet::new(); + let mut human_keys = HashSet::new(); + let mut session_keys = HashSet::new(); + + for attestation in &log.attestations { + for entry in &attestation.entries { + let key = attestation_metadata_key(&entry.hash).to_string(); + if key.starts_with("h_") { + human_keys.insert(key); + } else if key.starts_with("s_") { + session_keys.insert(key); + } else { + prompt_keys.insert(key); + } + } + } + + log.metadata + .prompts + .retain(|key, _| prompt_keys.contains(key)); + log.metadata + .humans + .retain(|key, _| human_keys.contains(key)); + log.metadata + .sessions + .retain(|key, _| session_keys.contains(key)); +} + +fn filter_resolution_log_to_uncovered_lines( + mut resolution_log: AuthorshipLog, + shifted_log: &AuthorshipLog, +) -> AuthorshipLog { + let shifted_coverage = line_coverage_by_file(shifted_log); + + for attestation in &mut resolution_log.attestations { + let covered = shifted_coverage + .get(&attestation.file_path) + .map(Vec::as_slice) + .unwrap_or(&[]); + for entry in &mut attestation.entries { + entry.line_ranges = subtract_line_ranges(&entry.line_ranges, covered); + } + attestation + .entries + .retain(|entry| !entry.line_ranges.is_empty()); + } + + resolution_log + .attestations + .retain(|attestation| !attestation.entries.is_empty()); + retain_referenced_metadata(&mut resolution_log); + resolution_log +} + +fn merge_file_attestations(target: &mut AuthorshipLog, source: &AuthorshipLog) { + for source_attestation in &source.attestations { + let target_attestation = target.get_or_create_file(&source_attestation.file_path); + for source_entry in &source_attestation.entries { + if let Some(target_entry) = target_attestation + .entries + .iter_mut() + .find(|entry| entry.hash == source_entry.hash) + { + target_entry + .line_ranges + .extend(source_entry.line_ranges.clone()); + target_entry.line_ranges = normalize_line_ranges(&target_entry.line_ranges); + } else { + let mut entry = source_entry.clone(); + entry.line_ranges = normalize_line_ranges(&entry.line_ranges); + target_attestation.entries.push(entry); + } + } + } +} + +fn merge_authorship_metadata(target: &mut AuthorshipLog, source: &AuthorshipLog) { + for (key, record) in &source.metadata.prompts { + target + .metadata + .prompts + .entry(key.clone()) + .or_insert_with(|| record.clone()); + } + for (key, record) in &source.metadata.humans { + target + .metadata + .humans + .entry(key.clone()) + .or_insert_with(|| record.clone()); + } + for (key, record) in &source.metadata.sessions { + target + .metadata + .sessions + .entry(key.clone()) + .or_insert_with(|| record.clone()); + } +} + +fn equal_line_mapping_between_commits( + repo: &Repository, + source_sha: &str, + destination_sha: &str, + file_path: &str, +) -> Option> { + let source_content = + String::from_utf8(repo.get_file_content(file_path, source_sha).ok()?).ok()?; + let destination_content = + String::from_utf8(repo.get_file_content(file_path, destination_sha).ok()?).ok()?; + let source_lines: Vec = source_content.lines().map(str::to_string).collect(); + let destination_lines: Vec = destination_content.lines().map(str::to_string).collect(); + let diff_ops = capture_diff_slices(&source_lines, &destination_lines); + + let mut mapping = HashMap::new(); + for op in diff_ops { + if let DiffOp::Equal { + old_index, + new_index, + len, + } = op + { + for offset in 0..len { + mapping.insert( + (old_index + offset + 1) as u32, + (new_index + offset + 1) as u32, + ); + } + } + } + Some(mapping) +} + +fn recover_exact_source_lines_from_mapping( + repo: &Repository, + target: &mut AuthorshipLog, + source_sha: &str, + destination_sha: &str, +) { + let Some(source_raw) = crate::git::notes_api::read_note(repo, source_sha) else { + return; + }; + let Ok(source_log) = AuthorshipLog::deserialize_from_string(&source_raw) else { + return; + }; + + let mut recovered_log = AuthorshipLog::new(); + recovered_log.metadata = source_log.metadata.clone(); + let mut target_coverage = line_coverage_by_file(target); + + for source_attestation in &source_log.attestations { + let Some(line_mapping) = equal_line_mapping_between_commits( + repo, + source_sha, + destination_sha, + &source_attestation.file_path, + ) else { + continue; + }; + + for source_entry in &source_attestation.entries { + let mut mapped_lines = Vec::new(); + for source_line in source_entry.line_ranges.iter().flat_map(LineRange::expand) { + if let Some(destination_line) = line_mapping.get(&source_line) { + mapped_lines.push(*destination_line); + } + } + + if mapped_lines.is_empty() { + continue; + } + + mapped_lines.sort_unstable(); + mapped_lines.dedup(); + let mapped_ranges = LineRange::compress_lines(&mapped_lines); + let current_coverage = target_coverage + .get(&source_attestation.file_path) + .map(Vec::as_slice) + .unwrap_or(&[]); + let missing_ranges = subtract_line_ranges(&mapped_ranges, current_coverage); + if missing_ranges.is_empty() { + continue; + } + + target_coverage + .entry(source_attestation.file_path.clone()) + .or_default() + .extend(missing_ranges.clone()); + let file = recovered_log.get_or_create_file(&source_attestation.file_path); + file.add_entry(AttestationEntry::new( + source_entry.hash.clone(), + missing_ranges, + )); + } + } + + recovered_log + .attestations + .retain(|attestation| !attestation.entries.is_empty()); + retain_referenced_metadata(&mut recovered_log); + merge_file_attestations(target, &recovered_log); + merge_authorship_metadata(target, &recovered_log); +} + +pub fn merge_conflict_resolution_authorship( + repo: &Repository, + existing_shifted_log: Option, + resolution_log: AuthorshipLog, + source_sha: Option<&str>, + commit_sha: &str, +) -> AuthorshipLog { + let mut merged = existing_shifted_log.unwrap_or_default(); + if let Some(source_sha) = source_sha { + recover_exact_source_lines_from_mapping(repo, &mut merged, source_sha, commit_sha); + } + let resolution_log = filter_resolution_log_to_uncovered_lines(resolution_log, &merged); + + merge_file_attestations(&mut merged, &resolution_log); + merge_authorship_metadata(&mut merged, &resolution_log); + merged.metadata.base_commit_sha = commit_sha.to_string(); + merged +} diff --git a/src/authorship/mod.rs b/src/authorship/mod.rs index ae3da53f2d..9351280a2c 100644 --- a/src/authorship/mod.rs +++ b/src/authorship/mod.rs @@ -3,6 +3,7 @@ pub mod attribution_tracker; pub mod authorship_log; pub mod authorship_log_serialization; pub mod background_agent; +pub mod conflict_resolution; pub mod diff_ai_accepted; pub mod git_ai_hooks; pub mod hunk_shift; diff --git a/src/authorship/post_commit.rs b/src/authorship/post_commit.rs index 92a3f89e02..bb15e95628 100644 --- a/src/authorship/post_commit.rs +++ b/src/authorship/post_commit.rs @@ -74,6 +74,27 @@ pub fn post_commit_from_working_log( human_author: String, supress_output: bool, ) -> Result<(String, AuthorshipLog), GitAiError> { + post_commit_from_working_log_with_transform( + repo, + base_commit, + commit_sha, + human_author, + supress_output, + Ok, + ) +} + +pub fn post_commit_from_working_log_with_transform( + repo: &Repository, + base_commit: Option, + commit_sha: String, + human_author: String, + supress_output: bool, + transform: F, +) -> Result<(String, AuthorshipLog), GitAiError> +where + F: FnOnce(AuthorshipLog) -> Result, +{ // Use base_commit parameter if provided, otherwise use "initial" for empty repos // This matches the convention in checkpoint.rs let parent_sha = base_commit.unwrap_or_else(|| "initial".to_string()); @@ -157,6 +178,9 @@ pub fn post_commit_from_working_log( } } + authorship_log = transform(authorship_log)?; + authorship_log.metadata.base_commit_sha = commit_sha.clone(); + // Long-lived daemon processes should read a fresh config snapshot. // Always use Config::fresh() to support runtime config updates // (especially important for daemon mode, but also good for consistency) diff --git a/src/authorship/rewrite.rs b/src/authorship/rewrite.rs index eff5672c6f..a78760c91e 100644 --- a/src/authorship/rewrite.rs +++ b/src/authorship/rewrite.rs @@ -40,26 +40,7 @@ pub fn handle_rewrite_event(repo: &Repository, event: RewriteEvent) -> Result<() ref old_tip, ref new_tip, ref onto, - } => { - let result = derive_mappings_from_range_diff(repo, old_tip, new_tip, onto.as_deref())?; - match result { - RangeDiffResult::Squash { base } => { - handle_squash_merge(repo, old_tip, new_tip, &base) - } - RangeDiffResult::Mappings(mappings) => { - if mappings.is_empty() { - return Ok(()); - } - let source_shas: Vec = - mappings.iter().map(|(src, _)| src.clone()).collect(); - crate::git::sync_authorship::fetch_missing_notes_for_commits( - repo, - &source_shas, - ); - shift_authorship_notes(repo, &mappings) - } - } - } + } => handle_non_fast_forward_rewrite(repo, old_tip, new_tip, onto.as_deref()).map(|_| ()), RewriteEvent::CherryPickComplete { sources, new_commits, @@ -75,6 +56,30 @@ pub fn handle_rewrite_event(repo: &Repository, event: RewriteEvent) -> Result<() } } +pub fn handle_non_fast_forward_rewrite( + repo: &Repository, + old_tip: &str, + new_tip: &str, + onto: Option<&str>, +) -> Result, GitAiError> { + let result = derive_mappings_from_range_diff(repo, old_tip, new_tip, onto)?; + match result { + RangeDiffResult::Squash { base } => { + handle_squash_merge(repo, old_tip, new_tip, &base)?; + Ok(Vec::new()) + } + RangeDiffResult::Mappings(mappings) => { + if mappings.is_empty() { + return Ok(Vec::new()); + } + let source_shas: Vec = mappings.iter().map(|(src, _)| src.clone()).collect(); + crate::git::sync_authorship::fetch_missing_notes_for_commits(repo, &source_shas); + shift_authorship_notes(repo, &mappings)?; + Ok(mappings) + } + } +} + fn handle_squash_merge( repo: &Repository, source_head: &str, diff --git a/src/commands/checkpoint_agent/orchestrator.rs b/src/commands/checkpoint_agent/orchestrator.rs index 2b4997a722..ddf5810bce 100644 --- a/src/commands/checkpoint_agent/orchestrator.rs +++ b/src/commands/checkpoint_agent/orchestrator.rs @@ -7,9 +7,7 @@ use crate::commands::checkpoint_agent::presets::{ use crate::config; use crate::daemon::checkpoint::PreparedPathRole; use crate::error::GitAiError; -use crate::git::repo_state::{ - git_dir_for_worktree, read_head_state_for_worktree, worktree_root_for_path, -}; +use crate::git::repo_state::{read_head_state_for_worktree, worktree_root_for_path}; use crate::git::repository::discover_repository_in_path_no_git_exec; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -55,39 +53,10 @@ struct CheckpointDebugLogEntry<'a> { struct RepoContext { repo_work_dir: PathBuf, base_commit: BaseCommit, - unmerged_paths: std::collections::HashSet, } const MAX_CHECKPOINT_FILES: usize = 1000; -fn has_active_merge_state(git_dir: &Path) -> bool { - git_dir.join("MERGE_HEAD").exists() - || git_dir.join("CHERRY_PICK_HEAD").exists() - || git_dir.join("rebase-merge").exists() - || git_dir.join("rebase-apply").exists() -} - -fn get_unmerged_paths_via_git(repo_work_dir: &Path) -> std::collections::HashSet { - use crate::git::repository::exec_git_allow_nonzero; - let args = vec![ - "-C".to_string(), - repo_work_dir.to_string_lossy().to_string(), - "ls-files".to_string(), - "-u".to_string(), - ]; - let output = match exec_git_allow_nonzero(&args) { - Ok(o) => o, - Err(_) => return std::collections::HashSet::new(), - }; - let stdout = String::from_utf8_lossy(&output.stdout); - stdout - .lines() - .filter(|l| !l.is_empty()) - .filter_map(|l| l.split('\t').nth(1)) - .map(|path| repo_work_dir.join(path)) - .collect() -} - fn build_checkpoint_files(file_paths: &[PathBuf]) -> Result, GitAiError> { let perf = std::env::var("GIT_AI_DEBUG_PERFORMANCE").is_ok_and(|v| !v.is_empty() && v != "0"); @@ -130,22 +99,11 @@ fn build_checkpoint_files(file_paths: &[PathBuf]) -> Result, }; let head_ms = t_head.elapsed().as_secs_f64() * 1000.0; - let t_unmerged = std::time::Instant::now(); - let unmerged_paths = if let Some(git_dir) = git_dir_for_worktree(&repo_work_dir) - && has_active_merge_state(&git_dir) - { - get_unmerged_paths_via_git(&repo_work_dir) - } else { - std::collections::HashSet::new() - }; - let unmerged_ms = t_unmerged.elapsed().as_secs_f64() * 1000.0; - if perf { eprintln!( - "[perf] build_checkpoint_files: discover={:.1}ms head={:.1}ms unmerged={:.1}ms (repo={})", + "[perf] build_checkpoint_files: discover={:.1}ms head={:.1}ms (repo={})", t_discover.elapsed().as_secs_f64() * 1000.0, head_ms, - unmerged_ms, repo_work_dir.display(), ); } @@ -156,17 +114,12 @@ fn build_checkpoint_files(file_paths: &[PathBuf]) -> Result, RepoContext { repo_work_dir: repo_work_dir.clone(), base_commit, - unmerged_paths, }, ); } repo_cache.get(&repo_work_dir).unwrap() }; - if ctx.unmerged_paths.contains(path) { - continue; - } - let t_read = std::time::Instant::now(); let content = if path.exists() { fs::read_to_string(path).ok() diff --git a/src/daemon.rs b/src/daemon.rs index f4d23155f0..5c43929c7d 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1,3 +1,4 @@ +use crate::authorship::authorship_log_serialization::AuthorshipLog; use crate::config; use crate::daemon::git_backend::GitBackend; use crate::error::GitAiError; @@ -744,9 +745,14 @@ fn resolve_stash_sha(cmd: &crate::daemon::domain::NormalizedCommand) -> Option<& } /// After a rebase completes, check if any newly-rebased commits were created -/// from conflict resolution with AI checkpoints. If so, run post_commit on -/// those commits to incorporate the AI attribution from the working log. -fn process_conflict_resolution_working_logs(repo: &Repository, new_tip: &str, onto: Option<&str>) { +/// from conflict resolution with AI checkpoints. If so, merge those resolution +/// checkpoints into the already-shifted source authorship note for the new commit. +fn process_conflict_resolution_working_logs( + repo: &Repository, + new_tip: &str, + onto: Option<&str>, + source_mappings: &[(String, String)], +) { let onto_sha = match onto { Some(s) if !s.is_empty() => s, _ => return, @@ -764,50 +770,54 @@ fn process_conflict_resolution_working_logs(repo: &Repository, new_tip: &str, on Err(_) => return, }; let log_output = String::from_utf8_lossy(&output.stdout); + let source_by_destination: HashMap = source_mappings + .iter() + .map(|(source, destination)| (destination.clone(), source.clone())) + .collect(); for line in log_output.lines() { let parts: Vec<&str> = line.split_whitespace().collect(); if parts.len() < 2 { continue; } - let commit_sha = parts[0]; - let parent_sha = parts[1]; // first parent + let commit_sha = parts[0].to_string(); + let parent_sha = parts[1].to_string(); // first parent - if !repo.storage.has_working_log(parent_sha) { + if !repo.storage.has_working_log(&parent_sha) { continue; } - // There's a working log at this parent — conflict resolution happened here. - // Save the existing shifted note so we can restore it if the working log - // produces a worse result (fewer attestation entries). - let existing_note_raw = crate::git::notes_api::read_note(repo, commit_sha); - let existing_entry_count = existing_note_raw - .as_ref() - .and_then(|raw| { - crate::authorship::authorship_log_serialization::AuthorshipLog::deserialize_from_string(raw).ok() - }) - .map(|log| log.attestations.iter().map(|a| a.entries.len()).sum::()) - .unwrap_or(0); - + let existing_shifted_log = crate::git::notes_api::read_note(repo, &commit_sha) + .and_then(|raw| AuthorshipLog::deserialize_from_string(&raw).ok()); let author = repo.git_author_identity().formatted_or_unknown(); - let _ = crate::authorship::post_commit::post_commit_from_working_log( - repo, - Some(parent_sha.to_string()), - commit_sha.to_string(), - author, - true, - ); - - // If the working log produced a worse note, restore the shifted one - if existing_entry_count > 0 - && let Ok(new_log) = crate::git::notes_api::read_authorship_v3(repo, commit_sha) + let source_sha = source_by_destination.get(&commit_sha).cloned(); + let commit_for_transform = commit_sha.clone(); + let commit_for_log = commit_sha.clone(); + if let Err(err) = + crate::authorship::post_commit::post_commit_from_working_log_with_transform( + repo, + Some(parent_sha), + commit_sha, + author, + true, + move |resolution_log| { + Ok( + crate::authorship::conflict_resolution::merge_conflict_resolution_authorship( + repo, + existing_shifted_log, + resolution_log, + source_sha.as_deref(), + &commit_for_transform, + ), + ) + }, + ) { - let new_count: usize = new_log.attestations.iter().map(|a| a.entries.len()).sum(); - if new_count < existing_entry_count - && let Some(raw) = existing_note_raw - { - let _ = crate::git::notes_api::write_note(repo, commit_sha, &raw); - } + tracing::debug!( + "failed to merge rebase conflict resolution authorship for {}: {}", + commit_for_log, + err + ); } } } @@ -3682,16 +3692,19 @@ impl ActorDaemonCoordinator { && original_head != new_tip && !is_ancestor_commit(&repo, &original_head, &new_tip) { - crate::authorship::rewrite::handle_rewrite_event( + let mappings = crate::authorship::rewrite::handle_non_fast_forward_rewrite( &repo, - crate::authorship::rewrite::RewriteEvent::NonFastForward { - old_tip: original_head.clone(), - new_tip: new_tip.clone(), - onto: rebase_onto.clone(), - }, + &original_head, + &new_tip, + rebase_onto.as_deref(), )?; let _ = repo.storage.rename_working_log(&original_head, &new_tip); - process_conflict_resolution_working_logs(&repo, &new_tip, rebase_onto.as_deref()); + process_conflict_resolution_working_logs( + &repo, + &new_tip, + rebase_onto.as_deref(), + &mappings, + ); } return Ok(()); } @@ -3706,13 +3719,11 @@ impl ActorDaemonCoordinator { continue; } - crate::authorship::rewrite::handle_rewrite_event( + let _ = crate::authorship::rewrite::handle_non_fast_forward_rewrite( &repo, - crate::authorship::rewrite::RewriteEvent::NonFastForward { - old_tip: old_tip.to_string(), - new_tip: new_tip.to_string(), - onto: onto_hint.clone(), - }, + old_tip, + new_tip, + onto_hint.as_deref(), )?; let _ = repo.storage.rename_working_log(old_tip, new_tip); } diff --git a/tests/integration/checkpoint_explicit_paths.rs b/tests/integration/checkpoint_explicit_paths.rs index d33e8df9b8..a5cd730eb8 100644 --- a/tests/integration/checkpoint_explicit_paths.rs +++ b/tests/integration/checkpoint_explicit_paths.rs @@ -49,7 +49,7 @@ fn test_explicit_path_checkpoint_only_tracks_the_explicit_file() { } #[test] -fn test_explicit_path_checkpoint_skips_conflicted_files() { +fn test_explicit_path_checkpoint_records_conflicted_files() { let repo = TestRepo::new(); let conflict_path = repo.path().join("conflict.txt"); fs::write(&conflict_path, "base\n").expect("failed to write conflict.txt"); @@ -84,15 +84,21 @@ fn test_explicit_path_checkpoint_skips_conflicted_files() { ); repo.git_ai(&["checkpoint", "mock_ai", "conflict.txt"]) - .expect("explicit conflict checkpoint should succeed without recording entries"); + .expect("explicit conflict checkpoint should succeed and record entries"); let checkpoints = repo .current_working_logs() .read_all_checkpoints() .expect("checkpoints should be readable"); + let latest = checkpoints + .last() + .expect("explicit conflict checkpoint should be recorded"); assert!( - checkpoints.is_empty(), - "explicit-path checkpoints should skip conflicted files entirely" + latest + .entries + .iter() + .any(|entry| entry.file == "conflict.txt"), + "explicit-path checkpoints should record conflicted files" ); } diff --git a/tests/integration/checkpoint_unit.rs b/tests/integration/checkpoint_unit.rs index 2895aa9fbd..548fd4d567 100644 --- a/tests/integration/checkpoint_unit.rs +++ b/tests/integration/checkpoint_unit.rs @@ -322,7 +322,7 @@ fn test_ai_checkpoint_without_agent_id_is_rejected() { } #[test] -fn test_checkpoint_skips_conflicted_files() { +fn test_checkpoint_records_conflicted_files() { // Create a repo with an initial commit let (repo, lines_file, _) = setup_repo_with_base_commit(); @@ -374,23 +374,18 @@ fn test_checkpoint_skips_conflicted_files() { repo.git_ai(&["checkpoint", "mock_known_human", &lines_file]) .unwrap(); - // Checkpoint should skip conflicted files — either no new checkpoint is created, - // or the new checkpoint has 0 entries. Both outcomes mean conflicted files were skipped. + // Checkpoints record conflicted files so conflict-resolution attribution can be + // merged into the eventual rebase/merge commit. let checkpoints_after = working_log.read_all_checkpoints().unwrap(); - if checkpoints_after.len() > count_before { - let latest = checkpoints_after.last().unwrap(); - assert_eq!( - latest.entries.len(), - 0, - "Should have 0 entries (conflicted file should be skipped)" - ); - } else { - assert_eq!( - checkpoints_after.len(), - count_before, - "No new checkpoint should be created for conflicted files" - ); - } + assert!( + checkpoints_after.len() > count_before, + "Should create a checkpoint for conflicted files" + ); + let latest = checkpoints_after.last().unwrap(); + assert!( + latest.entries.iter().any(|entry| entry.file == lines_file), + "Should record an entry for the conflicted file" + ); } #[test] @@ -566,7 +561,8 @@ fn test_checkpoint_works_after_conflict_resolution_maintains_authorship() { let has_conflicts = output.is_err(); assert!(has_conflicts, "Should have merge conflicts"); - // While there are conflicts, checkpoint should skip the file + // While there are conflicts, checkpoint should still record the file so the + // eventual resolution can carry explicit attribution. let gitai_repo = find_repository_in_path(repo.path().to_str().unwrap()).unwrap(); let base_commit = repo .git_og(&["rev-parse", "HEAD"]) @@ -583,23 +579,20 @@ fn test_checkpoint_works_after_conflict_resolution_maintains_authorship() { repo.git_ai(&["checkpoint", "mock_known_human", &lines_file]) .unwrap(); - // Checkpoint should skip conflicted files — either no new checkpoint is created, - // or the new checkpoint has 0 entries. + // Checkpoint should record conflicted files during the conflict. let checkpoints_after_conflict_checkpoint = working_log.read_all_checkpoints().unwrap(); - if checkpoints_after_conflict_checkpoint.len() > count_before { - let checkpoint_during_conflict = checkpoints_after_conflict_checkpoint.last().unwrap(); - assert_eq!( - checkpoint_during_conflict.entries.len(), - 0, - "Should skip conflicted files during conflict" - ); - } else { - assert_eq!( - checkpoints_after_conflict_checkpoint.len(), - count_before, - "No new checkpoint should be created for conflicted files" - ); - } + assert!( + checkpoints_after_conflict_checkpoint.len() > count_before, + "Should create a checkpoint for conflicted files" + ); + let checkpoint_during_conflict = checkpoints_after_conflict_checkpoint.last().unwrap(); + assert!( + checkpoint_during_conflict + .entries + .iter() + .any(|entry| entry.file == lines_file), + "Should record conflicted files during conflict" + ); // Resolve the conflict by choosing "ours" (base branch) repo.git_og(&["checkout", "--ours", &lines_file]).unwrap(); diff --git a/tests/integration/pull_rebase_ff.rs b/tests/integration/pull_rebase_ff.rs index 76cf9c3d95..5d25e16620 100644 --- a/tests/integration/pull_rebase_ff.rs +++ b/tests/integration/pull_rebase_ff.rs @@ -2,6 +2,7 @@ use git_ai::authorship::authorship_log_serialization::AuthorshipLog; use crate::repos::test_file::ExpectedLineExt; use crate::repos::test_repo::TestRepo; +use std::collections::BTreeSet; /// Helper struct that provides a local repo with an upstream containing seeded commits. /// The local repo is initially behind the upstream (no divergence — fast-forward possible). @@ -791,6 +792,8 @@ struct RegularRebaseConflictSetup { repo: TestRepo, /// SHA of the AI commit on the feature branch feature_ai_commit_sha: String, + /// SHA of the conflicting commit on the default branch + main_conflict_commit_sha: String, /// Name of the default branch default_branch: String, } @@ -829,6 +832,11 @@ fn setup_regular_rebase_conflict() -> RegularRebaseConflictSetup { main_file.set_contents(vec!["line 1".human(), "main change line 2".human()]); repo.stage_all_and_commit("main conflicting change") .expect("main commit should succeed"); + let main_conflict_commit_sha = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); // Switch back to feature repo.git(&["checkout", "feature"]) @@ -837,10 +845,85 @@ fn setup_regular_rebase_conflict() -> RegularRebaseConflictSetup { RegularRebaseConflictSetup { repo, feature_ai_commit_sha: feature_sha, + main_conflict_commit_sha, default_branch, } } +fn setup_regular_rebase_conflict_with_trailing_newlines() -> RegularRebaseConflictSetup { + use std::fs; + + let repo = TestRepo::new(); + let shared_path = repo.path().join("shared.txt"); + + fs::write(&shared_path, "line 1\nline 2\n").expect("write initial file"); + repo.git_ai(&["checkpoint", "mock_known_human", "shared.txt"]) + .expect("initial known-human checkpoint should succeed"); + repo.stage_all_and_commit("initial commit") + .expect("initial commit should succeed"); + + let default_branch = repo.current_branch(); + + repo.git(&["checkout", "-b", "feature"]) + .expect("checkout -b feature should succeed"); + + fs::write(&shared_path, "line 1\nAI feature line 2\n").expect("write feature file"); + repo.git_ai(&["checkpoint", "mock_ai", "shared.txt"]) + .expect("feature AI checkpoint should succeed"); + repo.stage_all_and_commit("AI feature changes") + .expect("AI feature commit should succeed"); + + let feature_sha = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + + repo.git(&["checkout", &default_branch]) + .expect("checkout main should succeed"); + + fs::write(&shared_path, "line 1\nmain change line 2\n").expect("write main file"); + repo.git_ai(&["checkpoint", "mock_known_human", "shared.txt"]) + .expect("main known-human checkpoint should succeed"); + repo.stage_all_and_commit("main conflicting change") + .expect("main commit should succeed"); + let main_conflict_commit_sha = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + + repo.git(&["checkout", "feature"]) + .expect("checkout feature should succeed"); + + RegularRebaseConflictSetup { + repo, + feature_ai_commit_sha: feature_sha, + main_conflict_commit_sha, + default_branch, + } +} + +fn session_keys(log: &AuthorshipLog) -> BTreeSet { + log.metadata.sessions.keys().cloned().collect() +} + +fn attestation_author_keys(log: &AuthorshipLog, path: &str) -> BTreeSet { + log.attestations + .iter() + .filter(|attestation| attestation.file_path == path) + .flat_map(|attestation| attestation.entries.iter()) + .map(|entry| { + entry + .hash + .split("::") + .next() + .unwrap_or(&entry.hash) + .to_string() + }) + .collect() +} + #[test] fn test_regular_rebase_with_conflict_preserves_ai_notes() { let setup = setup_regular_rebase_conflict(); @@ -917,6 +1000,287 @@ fn test_regular_rebase_with_conflict_preserves_ai_notes() { ); } +#[test] +fn test_regular_rebase_conflict_ai_resolution_preserves_original_and_resolution_sessions() { + use std::fs; + + let setup = setup_regular_rebase_conflict(); + let repo = setup.repo; + let original_note = repo + .read_authorship_note(&setup.feature_ai_commit_sha) + .expect("feature AI commit should have authorship note"); + let original_log = + AuthorshipLog::deserialize_from_string(&original_note).expect("parse original note"); + let original_sessions = session_keys(&original_log); + assert!( + !original_sessions.is_empty(), + "precondition: original feature note should contain session metadata" + ); + + let rebase_result = repo.git(&["rebase", &setup.default_branch]); + assert!( + rebase_result.is_err(), + "rebase should fail due to conflict on shared.txt" + ); + + repo.git_ai(&["checkpoint", "human", "shared.txt"]) + .expect("pre-resolution checkpoint should succeed"); + fs::write( + repo.path().join("shared.txt"), + "line 1\nmain change line 2\nAI resolved line 2", + ) + .expect("write AI conflict resolution"); + repo.git_ai(&["checkpoint", "mock_ai", "shared.txt"]) + .expect("AI resolution checkpoint should succeed"); + + repo.git(&["add", "shared.txt"]) + .expect("staging resolved file should succeed"); + repo.git_with_env(&["rebase", "--continue"], &[("GIT_EDITOR", "true")], None) + .expect("rebase --continue should succeed"); + + let rebased_head = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + assert_ne!( + rebased_head, setup.feature_ai_commit_sha, + "HEAD should have a new SHA after rebase" + ); + + let rebased_note = repo + .read_authorship_note(&rebased_head) + .expect("rebased commit should have authorship note"); + let rebased_log = + AuthorshipLog::deserialize_from_string(&rebased_note).expect("parse rebased note"); + let rebased_sessions = session_keys(&rebased_log); + let resolution_sessions = rebased_sessions + .difference(&original_sessions) + .cloned() + .collect::>(); + + assert!( + original_sessions.is_subset(&rebased_sessions), + "rebased note should preserve original feature session metadata; original={:?}, rebased={:?}; note={}", + original_sessions, + rebased_sessions, + rebased_note + ); + assert!( + !resolution_sessions.is_empty(), + "rebased note should contain a new AI conflict-resolution session; original={:?}, rebased={:?}", + original_sessions, + rebased_sessions + ); + + let shared_authors = attestation_author_keys(&rebased_log, "shared.txt"); + assert!( + !shared_authors.is_empty(), + "AI resolution should create shared.txt attribution" + ); + assert!( + shared_authors + .iter() + .any(|author| resolution_sessions.contains(author)), + "shared.txt attribution should belong to resolution session; authors={:?}, resolution_sessions={:?}", + shared_authors, + resolution_sessions + ); + assert!( + shared_authors.is_disjoint(&original_sessions), + "original conflict-hunk attribution should be dropped, not carried as file attribution; authors={:?}, original_sessions={:?}", + shared_authors, + original_sessions + ); + + let mut final_file = repo.filename("shared.txt"); + final_file.assert_committed_lines(crate::lines![ + "line 1".human(), + "main change line 2".human(), + "AI resolved line 2".ai(), + ]); +} + +#[test] +fn test_regular_rebase_conflict_keep_feature_side_preserves_feature_attribution() { + use std::fs; + + let setup = setup_regular_rebase_conflict(); + let repo = setup.repo; + let original_note = repo + .read_authorship_note(&setup.feature_ai_commit_sha) + .expect("feature AI commit should have authorship note"); + let original_log = + AuthorshipLog::deserialize_from_string(&original_note).expect("parse original note"); + let original_sessions = session_keys(&original_log); + assert!( + !original_sessions.is_empty(), + "precondition: original feature note should contain session metadata" + ); + + let rebase_result = repo.git(&["rebase", &setup.default_branch]); + assert!( + rebase_result.is_err(), + "rebase should fail due to conflict on shared.txt" + ); + + fs::write(repo.path().join("shared.txt"), "line 1\nAI feature line 2") + .expect("write feature-side conflict resolution"); + repo.git(&["add", "shared.txt"]) + .expect("staging resolved file should succeed"); + repo.git_with_env(&["rebase", "--continue"], &[("GIT_EDITOR", "true")], None) + .expect("rebase --continue should succeed"); + + let rebased_head = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + let rebased_note = repo + .read_authorship_note(&rebased_head) + .expect("rebased commit should have authorship note"); + let rebased_log = + AuthorshipLog::deserialize_from_string(&rebased_note).expect("parse rebased note"); + let shared_authors = attestation_author_keys(&rebased_log, "shared.txt"); + assert!( + shared_authors + .iter() + .any(|author| original_sessions.contains(author)), + "feature-side resolution should preserve feature attribution; authors={:?}, original_sessions={:?}; note={}", + shared_authors, + original_sessions, + rebased_note + ); + + let mut final_file = repo.filename("shared.txt"); + final_file.assert_committed_lines(crate::lines!["line 1".human(), "AI feature line 2".ai(),]); +} + +#[test] +fn test_regular_rebase_conflict_keep_both_sides_preserves_each_original_source() { + use std::fs; + + let setup = setup_regular_rebase_conflict_with_trailing_newlines(); + let repo = setup.repo; + let original_note = repo + .read_authorship_note(&setup.feature_ai_commit_sha) + .expect("feature AI commit should have authorship note"); + let original_log = + AuthorshipLog::deserialize_from_string(&original_note).expect("parse original note"); + let original_sessions = session_keys(&original_log); + assert!( + !original_sessions.is_empty(), + "precondition: original feature note should contain session metadata" + ); + + let rebase_result = repo.git(&["rebase", &setup.default_branch]); + assert!( + rebase_result.is_err(), + "rebase should fail due to conflict on shared.txt" + ); + + fs::write( + repo.path().join("shared.txt"), + "line 1\nmain change line 2\nAI feature line 2\n", + ) + .expect("write keep-both conflict resolution"); + repo.git(&["add", "shared.txt"]) + .expect("staging resolved file should succeed"); + repo.git_with_env(&["rebase", "--continue"], &[("GIT_EDITOR", "true")], None) + .expect("rebase --continue should succeed"); + + let rebased_head = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + let rebased_note = repo + .read_authorship_note(&rebased_head) + .expect("rebased commit should have authorship note"); + let rebased_log = + AuthorshipLog::deserialize_from_string(&rebased_note).expect("parse rebased note"); + let shared_authors = attestation_author_keys(&rebased_log, "shared.txt"); + assert!( + shared_authors + .iter() + .any(|author| original_sessions.contains(author)), + "keep-both resolution should preserve feature-side attribution; authors={:?}, original_sessions={:?}; note={}", + shared_authors, + original_sessions, + rebased_note + ); + + let blame = repo + .git(&["blame", "--line-porcelain", "-L", "2,2", "--", "shared.txt"]) + .expect("git blame should succeed"); + let blamed_commit = blame + .lines() + .next() + .and_then(|line| line.split_whitespace().next()) + .expect("blame should include commit sha"); + assert_eq!( + blamed_commit, setup.main_conflict_commit_sha, + "main-side kept line should blame to the original main conflict commit" + ); + + let mut final_file = repo.filename("shared.txt"); + final_file.assert_committed_lines(crate::lines![ + "line 1".human(), + "main change line 2".human(), + "AI feature line 2".ai(), + ]); +} + +#[test] +fn test_regular_rebase_conflict_keep_main_side_preserves_main_attribution() { + use std::fs; + + let setup = setup_regular_rebase_conflict(); + let repo = setup.repo; + + let rebase_result = repo.git(&["rebase", &setup.default_branch]); + assert!( + rebase_result.is_err(), + "rebase should fail due to conflict on shared.txt" + ); + + fs::write(repo.path().join("shared.txt"), "line 1\nmain change line 2") + .expect("write main-side conflict resolution"); + repo.git(&["add", "shared.txt"]) + .expect("staging resolved file should succeed"); + repo.git(&["rebase", "--skip"]) + .expect("main-side resolution makes the feature commit empty, so rebase should skip it"); + + let head = repo + .git(&["rev-parse", "HEAD"]) + .expect("rev-parse should succeed") + .trim() + .to_string(); + assert_eq!( + head, setup.main_conflict_commit_sha, + "keeping the main side should leave feature at the original main conflict commit" + ); + + let blame = repo + .git(&["blame", "--line-porcelain", "-L", "2,2", "--", "shared.txt"]) + .expect("git blame should succeed"); + let blamed_commit = blame + .lines() + .next() + .and_then(|line| line.split_whitespace().next()) + .expect("blame should include commit sha"); + assert_eq!( + blamed_commit, setup.main_conflict_commit_sha, + "main-side line should blame to the original main conflict commit" + ); + + let mut final_file = repo.filename("shared.txt"); + final_file.assert_committed_lines(crate::lines![ + "line 1".human(), + "main change line 2".human(), + ]); +} + #[test] fn test_regular_rebase_with_conflict_abort_preserves_original_notes() { let setup = setup_regular_rebase_conflict(); @@ -973,5 +1337,9 @@ crate::reuse_tests_in_worktree!( test_pull_rebase_with_conflict_preserves_ai_notes, test_pull_rebase_with_conflict_abort_preserves_original_notes, test_regular_rebase_with_conflict_preserves_ai_notes, + test_regular_rebase_conflict_ai_resolution_preserves_original_and_resolution_sessions, + test_regular_rebase_conflict_keep_feature_side_preserves_feature_attribution, + test_regular_rebase_conflict_keep_both_sides_preserves_each_original_source, + test_regular_rebase_conflict_keep_main_side_preserves_main_attribution, test_regular_rebase_with_conflict_abort_preserves_original_notes, );