fix(pulls): merge-commit parent diff for merged PRs (collins-platform #12)#236
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR extends PR diff and emptiness detection to handle merged PRs by computing diffs via merge-commit parents. It adds ChangesMerged PR Support
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Real-world hit: collins-platform PR #12 (123,773 additions, merged). After #234 landed, cube prv ran the local-git path but git diff --quiet origin/main...origin/feat/... returned exit 0 (no diff), so the empty-PR bail fired. False-empty. Root cause: three-dot origin/<base>...origin/<head> diffs from the merge-base. For OPEN PRs, merge-base = where head branched off base. For MERGED PRs, base has absorbed head — so merge-base IS head itself, and merge-base..head is empty. GitHub's diff endpoint dodges this by diffing the merge commit's two parents (^1 = base-before-merge, ^2 = head). cube does the same locally now: - fetch_pr requests mergeCommit alongside other PR fields - PRData.merge_commit_oid carries the SHA - has_changes() / get_diff() prefer git diff <merge>^1 <merge>^2 when the SHA is set. Fetches by SHA so this works even when the head branch was deleted post-merge. - Falls back to three-dot, then to API, then to conservative True. Smoke test on the real PR: has_changes True (was: False, silently bailed) get_diff 130k+ lines (was: empty) Tests: 2 new (16 total). Full suite: 508/508. mypy clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jacsamell
force-pushed
the
fix/merged-pr-diff-via-merge-commit
branch
from
e844a38 to
d717fce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Real-world hit:
cube prv 12on collins-platform PR #12 (123,773 additions, merged) bailed with "PR has no diff - nothing to review". The new local-git path in #234 returned empty for merged PRs.Root cause
Three-dot
origin/<base>...origin/<head>diffs from the merge-base. For OPEN PRs, merge-base = where head branched off base. For MERGED PRs, base has absorbed head → merge-base IS head itself →base...headis empty. The PR's actual changes are gone from three-dot's view.GitHub's diff endpoint dodges this by diffing the merge commit's two parents:
^1= base-before-merge,^2= PR head. cube does the same locally now.Changes
fetch_prrequestsmergeCommitalongside other PR fieldsPRData.merge_commit_oidcarries the SHAhas_changes()andget_diff()prefergit diff <merge>^1 <merge>^2when the SHA is set. Fetches by SHA so this works even when the head branch was deleted post-merge.Smoke test
Against the real PR #12 (cwd = collins-platform clone):
has_changes→True(was:False, silently bailed)get_diff→ 130k+ lines of actual PR diff (was: empty string)Test plan
🤖 Generated with Claude Code
Summary
Problem: Merged PRs produced empty local diffs because the previous three-dot local-git diff (origin/...origin/) can compare from a merge-base equal to the head when the base absorbed the head.
Solution: When a PR has a merge commit, compute the diff between the merge commit’s two parents (git diff ^1 ^2) — matching GitHub’s behaviour — falling back to the existing three-dot local diff, the API diff, or a conservative True if resolution fails.
Changes
python/cube/github/pulls.py
merge_commit_oid: strtoPRData.fetch_pr()now requestsmergeCommitvia gh pr view and stores itsoid.has_changes()andget_diff()prefer a merge-commit parent diff whenmerge_commit_oidis available (non–cross-repo), with fallbacks to three-dot local diff → API diff → conservative True._try_local_diff_via_merge_commit()and clarified docs: three-dot logic targets OPEN PRs; merged-PR logic uses merge commit parents.tests/cli/test_pulls_local_diff.py
_gh_view_stub()to model merged PRs (state,merged_at,merge_commit_oid).test_merged_pr_uses_merge_commit_parents_for_has_changestest_merged_pr_get_diff_uses_merge_commit_parentsResults
#12) now returns ~130k+ lines and has_changes() = True (previously empty/False).