From b38257adc74b0e8ea4494e4c264eb73aef3b4fa0 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 1 Apr 2026 18:23:59 +0000 Subject: [PATCH] fix: correctly attribute AI lines in merge commit stats Fixes https://github.com/git-ai-project/git-ai/issues/910 When AI resolves a merge conflict, `git ai blame` correctly attributes lines to the AI, but `git ai stats head` was incorrectly showing 100% human / 0% AI additions. Three root causes: 1. `accepted_lines_from_attestations()` had an early return that always returned (0, empty) for merge commits, skipping attestation matching. 2. `stats_for_commit_stats()` set `added_lines_by_file` to an empty HashMap for merge commits, so even without the early return there were no lines to match against attestations. 3. `git show --numstat` uses the combined-diff format for merge commits, which only shows files that differ from ALL parents (conflict resolutions). This misses cleanly-merged changes. The fix uses `git diff commit^1 commit --numstat` to diff against the first parent instead. Changes: - Remove `is_merge_commit` parameter and early return from `accepted_lines_from_attestations()` - Always compute `added_lines_by_file` by diffing against the first parent (works for both regular and merge commits) - Add `get_git_diff_stats_first_parent()` helper for merge commit numstat via `git diff commit^1 commit --numstat` - Extract `parse_numstat_output()` to share parsing logic between the two numstat code paths - Add two TestRepo-based integration tests that replicate the bug - Update unit tests to reflect the corrected behavior Co-Authored-By: Sasha Varlamov --- src/authorship/stats.rs | 106 +++++++++++++++++++++----------- tests/integration/stats.rs | 123 +++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 37 deletions(-) diff --git a/src/authorship/stats.rs b/src/authorship/stats.rs index 815865968..6c9faf13b 100644 --- a/src/authorship/stats.rs +++ b/src/authorship/stats.rs @@ -535,29 +535,30 @@ pub fn stats_for_commit_stats( ) -> Result { let commit_obj = repo.revparse_single(commit_sha)?.peel_to_commit()?; - // Step 1: get the diff between this commit and its parent ON refname (if more than one parent) - // If initial than everything is additions - // We want the count here git shows +111 -55 - let (git_diff_added_lines, git_diff_deleted_lines) = - get_git_diff_stats(repo, commit_sha, ignore_patterns)?; + // Step 1: get the diff between this commit and its parent. + // For merge commits, diff against the first parent to see what the merge introduced. + // For regular commits, use git show --numstat. + let parent_count = commit_obj.parent_count()?; + let is_merge_commit = parent_count > 1; + let (git_diff_added_lines, git_diff_deleted_lines) = if is_merge_commit { + get_git_diff_stats_first_parent(repo, commit_sha, ignore_patterns)? + } else { + get_git_diff_stats(repo, commit_sha, ignore_patterns)? + }; // Step 2: get the authorship log for this commit let authorship_log = get_authorship(repo, commit_sha); // Step 3: get line numbers added by this specific commit, then intersect with attestations. // This keeps accepted stats scoped to the target commit while avoiding expensive blame traversal. - let parent_count = commit_obj.parent_count()?; - let is_merge_commit = parent_count > 1; - let mut added_lines_by_file: HashMap> = if is_merge_commit { - HashMap::new() + // For merge commits, diff against the first parent to capture conflict resolution changes. + let from_ref = if parent_count == 0 { + "4b825dc642cb6eb9a060e54bf8d69288fbee4904".to_string() } else { - let from_ref = if parent_count == 0 { - "4b825dc642cb6eb9a060e54bf8d69288fbee4904".to_string() - } else { - commit_obj.parent(0)?.id() - }; - repo.diff_added_lines(&from_ref, commit_sha, None)? + commit_obj.parent(0)?.id() }; + let mut added_lines_by_file: HashMap> = + repo.diff_added_lines(&from_ref, commit_sha, None)?; let ignore_matcher = build_ignore_matcher(ignore_patterns); added_lines_by_file .retain(|file_path, _| !should_ignore_file_with_matcher(file_path, &ignore_matcher)); @@ -567,11 +568,8 @@ pub fn stats_for_commit_stats( } // Step 4: derive accepted lines directly from note attestations for lines added in this commit. - let (ai_accepted, ai_accepted_by_tool) = accepted_lines_from_attestations( - authorship_log.as_ref(), - &added_lines_by_file, - is_merge_commit, - ); + let (ai_accepted, ai_accepted_by_tool) = + accepted_lines_from_attestations(authorship_log.as_ref(), &added_lines_by_file); // Step 5: Calculate stats from authorship log Ok(stats_from_authorship_log( @@ -586,12 +584,7 @@ pub fn stats_for_commit_stats( fn accepted_lines_from_attestations( authorship_log: Option<&crate::authorship::authorship_log_serialization::AuthorshipLog>, added_lines_by_file: &HashMap>, - is_merge_commit: bool, ) -> (u32, BTreeMap) { - if is_merge_commit { - return (0, BTreeMap::new()); - } - let mut total_ai_accepted = 0u32; let mut per_tool_model = BTreeMap::new(); @@ -657,6 +650,36 @@ pub fn get_git_diff_stats( let output = exec_git_with_profile(&args, InternalGitProfile::NumstatParse)?; let stdout = String::from_utf8_lossy(&output.stdout); + parse_numstat_output(&stdout, ignore_patterns) +} + +/// Get git diff statistics for a merge commit by diffing against first parent. +/// `git show --numstat` on merge commits uses the combined diff format, which only shows +/// files that differ from ALL parents (i.e. conflict resolutions). This misses changes +/// that were cleanly merged. Instead, we diff against the first parent to see everything +/// the merge introduced relative to the branch it was merged into. +fn get_git_diff_stats_first_parent( + repo: &Repository, + commit_sha: &str, + ignore_patterns: &[String], +) -> Result<(u32, u32), GitAiError> { + let mut args = repo.global_args_for_exec(); + args.push("diff".to_string()); + args.push("--numstat".to_string()); + args.push(format!("{}^1", commit_sha)); + args.push(commit_sha.to_string()); + + let output = exec_git_with_profile(&args, InternalGitProfile::NumstatParse)?; + let stdout = String::from_utf8_lossy(&output.stdout); + + parse_numstat_output(&stdout, ignore_patterns) +} + +/// Parse numstat output lines into (added, deleted) totals. +fn parse_numstat_output( + stdout: &str, + ignore_patterns: &[String], +) -> Result<(u32, u32), GitAiError> { let mut added_lines = 0u32; let mut deleted_lines = 0u32; let ignore_matcher = build_ignore_matcher(ignore_patterns); @@ -1281,14 +1304,16 @@ mod tests { #[test] fn test_accepted_lines_no_authorship_log() { let added_lines: HashMap> = HashMap::new(); - let (accepted, per_tool) = accepted_lines_from_attestations(None, &added_lines, false); + let (accepted, per_tool) = accepted_lines_from_attestations(None, &added_lines); assert_eq!(accepted, 0); assert!(per_tool.is_empty()); } #[test] - fn test_accepted_lines_merge_commit() { - // Even with a real authorship log, merge commits should short-circuit to (0, empty) + fn test_accepted_lines_merge_commit_now_counts_ai() { + // After fixing issue #910, merge commits should correctly count AI accepted lines + // when added_lines_by_file is populated (which stats_for_commit_stats now does + // by diffing against the first parent). let mut log = crate::authorship::authorship_log_serialization::AuthorshipLog::new(); let agent_id = crate::authorship::working_log::AgentId { tool: "cursor".to_string(), @@ -1328,9 +1353,9 @@ mod tests { let mut added_lines: HashMap> = HashMap::new(); added_lines.insert("foo.rs".to_string(), vec![1, 2, 3]); - let (accepted, per_tool) = accepted_lines_from_attestations(Some(&log), &added_lines, true); - assert_eq!(accepted, 0); - assert!(per_tool.is_empty()); + let (accepted, per_tool) = accepted_lines_from_attestations(Some(&log), &added_lines); + assert_eq!(accepted, 3); + assert_eq!(per_tool.get("cursor::claude-3-sonnet"), Some(&3)); } #[test] @@ -1375,8 +1400,7 @@ mod tests { let mut added_lines: HashMap> = HashMap::new(); added_lines.insert("bar.rs".to_string(), vec![1, 2, 3]); - let (accepted, per_tool) = - accepted_lines_from_attestations(Some(&log), &added_lines, false); + let (accepted, per_tool) = accepted_lines_from_attestations(Some(&log), &added_lines); assert_eq!(accepted, 0); assert!(per_tool.is_empty()); } @@ -1422,8 +1446,7 @@ mod tests { let mut added_lines: HashMap> = HashMap::new(); added_lines.insert("foo.rs".to_string(), vec![1, 2, 3]); - let (accepted, per_tool) = - accepted_lines_from_attestations(Some(&log), &added_lines, false); + let (accepted, per_tool) = accepted_lines_from_attestations(Some(&log), &added_lines); assert_eq!(accepted, 3); // Verify per-tool breakdown contains the right key @@ -1471,7 +1494,12 @@ mod tests { } #[test] - fn test_stats_for_merge_commit_skips_ai_acceptance() { + fn test_stats_for_merge_commit_no_notes_on_merge() { + // Non-conflicting merge where the AI authorship notes live on the feature commit, + // not on the merge commit itself. The merge commit has no authorship log, so + // ai_accepted should be 0. However, the merge introduces lines relative to the + // first parent, so git_diff_added_lines > 0 (unlike the old combined-diff path + // which returned 0 for non-conflicting merges). let tmp_repo = TmpRepo::new().unwrap(); tmp_repo.write_file("test.txt", "base\n", true).unwrap(); @@ -1504,8 +1532,12 @@ mod tests { let merge_sha = tmp_repo.get_head_commit_sha().unwrap(); let stats = stats_for_commit_stats(tmp_repo.gitai_repo(), &merge_sha, &[]).unwrap(); + // No authorship log on the merge commit → ai_accepted stays 0 assert_eq!(stats.ai_accepted, 0); - assert_eq!(stats.ai_additions, stats.mixed_additions); + // The merge introduced 1 line ("feature line") relative to the first parent + assert_eq!(stats.git_diff_added_lines, 1); + // With no AI notes, the added line is attributed to human + assert_eq!(stats.human_additions, 1); } #[test] diff --git a/tests/integration/stats.rs b/tests/integration/stats.rs index 755337ac0..d183b4374 100644 --- a/tests/integration/stats.rs +++ b/tests/integration/stats.rs @@ -744,6 +744,127 @@ fn test_post_commit_large_ignored_files_do_not_trigger_skip_warning() { assert_eq!(stats.human_additions, 0); } +/// Test that merge commits with AI-resolved conflicts correctly show AI stats. +/// Regression test for https://github.com/git-ai-project/git-ai/issues/910 +/// +/// When AI resolves a merge conflict, `git ai blame` correctly attributes lines to AI, +/// but `git ai stats head` was incorrectly showing 100% human / 0% AI because +/// stats_for_commit_stats() skipped AI acceptance counting for all merge commits. +/// +/// The test simulates the real-world flow: +/// 1. Start merge (conflicts occur, auto-resolved with -X theirs but not committed) +/// 2. AI checkpoint marks the conflict resolution as AI-authored +/// 3. Commit the merge (hooks write authorship notes from the working log) +#[test] +fn test_stats_merge_commit_with_ai_conflict_resolution() { + let repo = TestRepo::new(); + let mut file = repo.filename("test.txt"); + + // Create base file + file.set_contents(crate::lines!["Line 1", "Line 2", "Line 3"]); + repo.stage_all_and_commit("Initial commit").unwrap(); + + let default_branch = repo.current_branch(); + + // Create feature branch with changes that will conflict + repo.git(&["checkout", "-b", "feature"]).unwrap(); + file.replace_at(1, "AI FEATURE VERSION".ai()); + repo.stage_all_and_commit("Feature change").unwrap(); + + // Go back to default branch and make conflicting human changes + repo.git(&["checkout", &default_branch]).unwrap(); + file = repo.filename("test.txt"); + file.replace_at(1, "HUMAN MAIN VERSION"); + repo.stage_all_and_commit("Human main change").unwrap(); + + // Merge feature branch: resolve conflicts with theirs but don't commit yet. + // This simulates starting a merge that an AI tool will resolve. + repo.git(&["merge", "feature", "--no-commit", "-X", "theirs"]) + .unwrap(); + + // Run AI checkpoint on the resolved file — this is what happens in practice when + // an AI tool (Cursor, Copilot, etc.) resolves the conflict and git-ai tracks it. + repo.git_ai(&["checkpoint", "mock_ai", "test.txt"]).unwrap(); + + // Commit the merge. The hooks read the working log (which now contains the AI + // checkpoint) and write authorship notes with attestations on the merge commit. + repo.stage_all_and_commit("Merge feature with AI conflict resolution") + .unwrap(); + + // Verify blame correctly shows AI attribution + file = repo.filename("test.txt"); + file.assert_lines_and_blame(crate::lines![ + "Line 1".human(), + "AI FEATURE VERSION".ai(), + "Line 3".human(), + ]); + + // Verify stats correctly show AI additions (this is the bug from issue #910) + let stats = repo.stats().unwrap(); + + // The merge commit introduces 1 line change vs first parent (AI FEATURE VERSION + // replacing HUMAN MAIN VERSION). That line was authored by AI, so stats should + // reflect the AI contribution. + assert!( + stats.ai_accepted > 0, + "Merge commit with AI-resolved conflict should have ai_accepted > 0, got: ai_accepted={}, human_additions={}, ai_additions={}", + stats.ai_accepted, + stats.human_additions, + stats.ai_additions, + ); +} + +/// Test that non-conflicting merge commits with AI involvement also show correct AI stats. +/// This simulates a scenario where AI assists during a non-conflicting merge (e.g., AI +/// review/modification of merged files before committing). +#[test] +fn test_stats_merge_commit_non_conflicting_ai_changes() { + let repo = TestRepo::new(); + let mut file = repo.filename("test.txt"); + + // Create base file + file.set_contents(crate::lines!["Line 1", "Line 2", "Line 3"]); + repo.stage_all_and_commit("Initial commit").unwrap(); + + let default_branch = repo.current_branch(); + + // Create feature branch with AI additions in a separate file (no conflict) + repo.git(&["checkout", "-b", "feature"]).unwrap(); + let mut ai_file = repo.filename("ai_feature.txt"); + ai_file.set_contents(crate::lines!["AI Line 1".ai(), "AI Line 2".ai()]); + repo.stage_all_and_commit("AI feature additions").unwrap(); + + // Go back to default branch and make human changes to a different file + repo.git(&["checkout", &default_branch]).unwrap(); + file = repo.filename("test.txt"); + file.replace_at(1, "HUMAN CHANGE"); + repo.stage_all_and_commit("Human main change").unwrap(); + + // Merge feature branch without committing + repo.git(&["merge", "feature", "--no-commit"]).unwrap(); + + // Run AI checkpoint on the new AI file — simulates AI involvement in the merge + repo.git_ai(&["checkpoint", "mock_ai", "ai_feature.txt"]) + .unwrap(); + + // Commit the merge + repo.stage_all_and_commit("Merge feature with AI additions") + .unwrap(); + + // Stats for the merge commit + let stats = repo.stats().unwrap(); + + // The merge introduces ai_feature.txt (2 AI lines) relative to the first parent. + // With the AI checkpoint, the authorship notes should attribute them to AI. + assert!( + stats.ai_accepted > 0, + "Non-conflicting merge with AI changes should have ai_accepted > 0, got: ai_accepted={}, human_additions={}, git_diff_added_lines={}", + stats.ai_accepted, + stats.human_additions, + stats.git_diff_added_lines, + ); +} + crate::reuse_tests_in_worktree!( test_authorship_log_stats, test_stats_cli_range, @@ -763,4 +884,6 @@ crate::reuse_tests_in_worktree!( test_stats_ignore_flag_is_additive_to_defaults, test_stats_range_uses_default_ignores, test_post_commit_large_ignored_files_do_not_trigger_skip_warning, + test_stats_merge_commit_with_ai_conflict_resolution, + test_stats_merge_commit_non_conflicting_ai_changes, );