diff --git a/apps/backend/internal/agentctl/server/process/git_log.go b/apps/backend/internal/agentctl/server/process/git_log.go index 78d3300249..163ac87fcc 100644 --- a/apps/backend/internal/agentctl/server/process/git_log.go +++ b/apps/backend/internal/agentctl/server/process/git_log.go @@ -387,7 +387,18 @@ func (g *GitOperator) ShowCommit(ctx context.Context, commitSHA string) (*Commit } // Get the diff with stats - diffOutput, err := g.runGitCommand(ctx, "show", "--format=", "--stat", "--numstat", "-p", commitSHA) + // Merge commits must be compared with their first parent so the patch + // matches the branch history shown by the commit list and GitHub. + diffOutput, err := g.runGitCommand( + ctx, + "show", + "--first-parent", + "--format=", + "--stat", + "--numstat", + "-p", + commitSHA, + ) if err != nil { result.Error = fmt.Sprintf("failed to get commit diff: %s", err.Error()) return result, nil diff --git a/apps/backend/internal/agentctl/server/process/git_test.go b/apps/backend/internal/agentctl/server/process/git_test.go index c5a480ab95..456f955243 100644 --- a/apps/backend/internal/agentctl/server/process/git_test.go +++ b/apps/backend/internal/agentctl/server/process/git_test.go @@ -91,6 +91,65 @@ func TestParseCommitDiff_PathsWithSpaces(t *testing.T) { } } +func TestShowCommit_MergeCommitUsesFirstParentDiff(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + log := newTestLogger(t) + ctx := context.Background() + + runGit(t, repoDir, "checkout", "-b", "feature/merge-diff") + writeFile(t, repoDir, "feature-only.txt", "feature branch\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "feature work") + + runGit(t, repoDir, "checkout", "main") + writeFile(t, repoDir, "incoming.txt", "merged branch\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "main work") + + runGit(t, repoDir, "checkout", "feature/merge-diff") + runGit(t, repoDir, "merge", "--no-ff", "-m", "Merge main into feature/merge-diff", "main") + mergeSHA := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + + gitOp := NewGitOperator(repoDir, log, nil) + result, err := gitOp.ShowCommit(ctx, mergeSHA) + if err != nil { + t.Fatalf("ShowCommit error: %v", err) + } + if !result.Success { + t.Fatalf("ShowCommit failed: %+v", result) + } + + if _, ok := result.Files["incoming.txt"]; !ok { + t.Fatalf("merge diff missing incoming first-parent change; files=%v", fileKeys(result.Files)) + } + entry, ok := result.Files["incoming.txt"].(map[string]interface{}) + if !ok { + t.Fatalf("incoming.txt entry type = %T, want map[string]interface{}", result.Files["incoming.txt"]) + } + if status := entry["status"]; status != "added" { + t.Errorf("incoming.txt status = %v, want added", status) + } + if _, ok := result.Files["feature-only.txt"]; ok { + t.Fatalf("merge diff incorrectly included a file already present in the first parent") + } + if result.FilesChanged != 1 { + t.Errorf("FilesChanged = %d, want 1", result.FilesChanged) + } + if result.Insertions != 1 || result.Deletions != 0 { + t.Errorf("stats = +%d/-%d, want +1/-0", result.Insertions, result.Deletions) + } + + diff, skipReason := fileEntryDiff(t, result.Files, "incoming.txt") + if skipReason != "" { + t.Errorf("incoming diff skip reason = %q, want empty", skipReason) + } + if !strings.Contains(diff, "+merged branch") { + t.Errorf("incoming diff = %q, want merged file content", diff) + } +} + func TestGitOperatorCreatePR_UsesAzureCLIForAzureRepos(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("shell wrapper test is Unix-only") diff --git a/docs/plans/merge-commit-details/plan.md b/docs/plans/merge-commit-details/plan.md new file mode 100644 index 0000000000..334e4e0165 --- /dev/null +++ b/docs/plans/merge-commit-details/plan.md @@ -0,0 +1,97 @@ +--- +spec: docs/specs/merge-commit-details/spec.md +created: 2026-08-02 +status: completed +--- + +# Implementation Plan: Repair merge commit details + +## Overview + +Make the agentctl commit-detail path request a first-parent patch for merge +commits, matching GitHub and the existing commit-list interpretation. Prove the +regression with a real temporary Git graph before changing production code, +then run the focused process-package tests. The API response and frontend +consumer remain unchanged. + +## Root cause + +`GitOperator.ShowCommit` invokes `git show --format= --stat --numstat -p `. +For the reported two-parent commit `45f2c92d2a129c06a266f45865ab62d4f63a5d8c`, +Git emits 649 numstat rows but no `diff --git` patch sections under its default +merge-diff behavior. `parseCommitDiff` only constructs file entries from +`diff --git` sections, so it returns an empty map and the frontend renders its +empty state. Running the same command with `--first-parent` emits 649 patch +sections and the expected +45,312/-3,983 first-parent diff. + +## Backend + +### Commit diff selection + +- `apps/backend/internal/agentctl/server/process/git_log.go`: add + `--first-parent` to the patch-producing `git show` invocation in + `GitOperator.ShowCommit`. Keep metadata lookup, SHA validation, parsing, + response fields, and uncapped single-commit behavior unchanged. +- Do not use `-m`, which would produce one diff per parent rather than the + single first-parent view required by the repair spec. + +## Frontend + +No frontend code changes are planned. `useCommitDiff` and +`CommitDetailPanel` already render every file returned by the existing +`CommitDiffResult.files` contract on desktop and mobile. + +## Tests + +- **What:** a clean two-parent merge returns the incoming first-parent file, + patch, and stats instead of an empty file map. + **File:** `apps/backend/internal/agentctl/server/process/git_test.go`. + **How:** add `TestShowCommit_MergeCommitUsesFirstParentDiff` using + `setupTestRepo`, create divergent feature/main commits, merge main into the + feature branch with `--no-ff`, and call the real `GitOperator.ShowCommit`. + The RED run must fail because `result.Files` is empty. The GREEN assertions + must show the incoming main-side file, exclude a feature-only file already + present in the first parent, and verify the parsed additions/deletions. +- **What:** existing ordinary and large single-commit diffs remain present and + uncapped. + **File:** existing tests in + `apps/backend/internal/agentctl/server/process/git_test.go`. + **How:** run the focused `ShowCommit` regression tests, followed by the full + process package. + +## E2E Tests + +No new Playwright test is planned. The defect is fully reproduced at the real +Git process boundary, and the existing commit-click E2E already verifies that a +non-empty `files` response is rendered. This repair changes no browser logic, +layout, interaction, or mobile composition. + +## Verification Results + +- RED: `rtk go test -v -run '^TestShowCommit_MergeCommitUsesFirstParentDiff$' + ./internal/agentctl/server/process` failed as expected with `files=[]` and + `merge diff missing incoming first-parent change`. +- GREEN: the same focused command passed with 1 test passed. +- Package: `rtk go test ./internal/agentctl/server/process` passed with 550 + tests in 1 package (exit code 0). +- Hygiene: `git diff --check` passed. The test repositories are temporary and + removed by `t.Cleanup`; no browser sessions, instances, or external state + were started or modified. + +## Implementation Waves And Parallel Candidates + +Wave 1 (sequential): + +- [x] [Task 01: First-parent merge commit diff](task-01-first-parent-merge-diff.md) — completed. + +There are no parallel candidates: the production change and regression test +share one package and form a single TDD unit. + +## Risks and out of scope + +- First-parent semantics intentionally show what the merge added to the branch + that received it; parent selection and combined-diff UI remain out of scope. +- The response can still be large because `ShowCommit` is intentionally + uncapped; this repair does not change that established contract. +- No public API, persistence, authorization, frontend, or documentation + contract changes are introduced. diff --git a/docs/plans/merge-commit-details/task-01-first-parent-merge-diff.md b/docs/plans/merge-commit-details/task-01-first-parent-merge-diff.md new file mode 100644 index 0000000000..854971109b --- /dev/null +++ b/docs/plans/merge-commit-details/task-01-first-parent-merge-diff.md @@ -0,0 +1,48 @@ +--- +id: "01-first-parent-merge-diff" +title: "First-parent merge commit diff" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/merge-commit-details/spec.md" +--- + +# Task 01: First-parent merge commit diff + +- **Acceptance:** `GitOperator.ShowCommit` returns the first-parent file map, + patch, status, additions, and deletions for a clean two-parent merge; it does + not include a feature-only file already present in the first parent; existing + single-commit behavior and the `CommitDiffResult` contract remain unchanged. +- **Verification:** Follow strict TDD. Add the regression test and first run + `cd apps/backend && go test -v -run '^TestShowCommit_MergeCommitUsesFirstParentDiff$' ./internal/agentctl/server/process`; + confirm it fails because the merge result has no files. After the minimal + fix, rerun that command, then run + `cd apps/backend && go test ./internal/agentctl/server/process` and + `git diff --check` from the repository root. +- **Files likely touched:** + `apps/backend/internal/agentctl/server/process/git_log.go`, + `apps/backend/internal/agentctl/server/process/git_test.go`, this task file, + and `plan.md` for status/results synchronization. +- **Dependencies:** None. +- **Parallelism:** sequential — the test and implementation change the same Go + package and must be completed as one Red-Green-Refactor cycle. +- **Inputs:** repair spec desired behavior and regression scenarios; + `GitOperator.ShowCommit` and `parseCommitDiff` in `git_log.go`; existing + `setupTestRepo`, `runGit`, `TestParseCommitDiff_PathsWithSpaces`, and + `TestShowCommit_NotCapped` patterns. +- **Output contract:** Report the root-cause-preserving minimal change, exact + files changed, RED and GREEN command results, process-package result, + `git diff --check`, remaining risks, and synchronized task/plan statuses. + +## Results + +- RED: `rtk go test -v -run '^TestShowCommit_MergeCommitUsesFirstParentDiff$' + ./internal/agentctl/server/process` failed with the expected assertion: + `merge diff missing incoming first-parent change; files=[]`. +- GREEN: the same focused command passed with 1 test passed. +- Full package: `rtk go test ./internal/agentctl/server/process` passed with 550 + tests in 1 package (exit code 0). +- Hygiene: `git diff --check` passed. `gofmt` ran on the two Go files. Test + repositories were temporary and cleaned by `t.Cleanup`; no external state, + browser session, or live instance was modified. diff --git a/docs/specs/merge-commit-details/spec.md b/docs/specs/merge-commit-details/spec.md new file mode 100644 index 0000000000..bb76c5a93e --- /dev/null +++ b/docs/specs/merge-commit-details/spec.md @@ -0,0 +1,57 @@ +--- +title: Repair merge commit details +status: building +created: 2026-08-02 +owner: kandev +--- + +# Repair merge commit details + +## Problem + +Opening a clean merge commit from the Changes panel can show **No files in this +commit** even when the merge introduced files and GitHub shows a non-empty +commit diff. The commit row can still report the correct aggregate additions +and deletions, leaving the row and detail view visibly inconsistent. + +## Desired behavior + +- Opening a merge commit shows the files changed between that commit and its + first parent, matching GitHub's commit-detail semantics and the branch history + represented by Kandev's commit list. +- Each changed file retains its patch, status, additions, and deletions through + the existing `CommitDiffResult.files` response. +- A merge commit does not duplicate files by returning a separate diff for each + parent or switch to a combined-diff view. +- Ordinary single-parent commits and root commits keep their current diff + behavior. +- A genuinely empty commit continues to return an empty file collection and may + display **No files in this commit**. +- Single-commit details remain uncapped; cumulative-diff budget behavior is + unchanged. + +## Regression scenarios + +- **GIVEN** a feature branch whose first-parent tip does not contain a file from + the branch being merged, **WHEN** the user opens the resulting clean merge + commit, **THEN** the incoming file and its patch are shown in commit details. +- **GIVEN** a file already exists unchanged in the merge commit's first parent, + **WHEN** the user opens the merge commit, **THEN** that first-parent-only file + is not incorrectly presented as introduced by the merge. +- **GIVEN** a normal single-parent or root commit with file changes, **WHEN** the + user opens it, **THEN** its changed files continue to be shown. +- **GIVEN** a commit with no tree change from its parent, **WHEN** the user opens + it, **THEN** the commit details may show the empty-file message. + +## Constraints + +- Preserve the existing `session.commit_diff` WebSocket action and + `CommitDiffResult` response shape. +- Use the merge commit's first parent as the sole comparison base. +- Keep commit metadata lookup and multi-repository routing unchanged. + +## Out of scope + +- Adding parent-selection controls or combined-diff rendering. +- Changing commit-list aggregation, cumulative PR changes, or diff budgets. +- Changing the commit-details UI, empty-state copy, or mobile composition.