fix: show files for merge commit details (#2173)
This commit is contained in:
Carlos Florêncio
@@ -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
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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 <sha>`.
|
||||
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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user