mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-01-26 19:09:52 +00:00
d59885839a
## Summary - Fixes the Claude automated PR review comparing against wrong commits - Updates the comprehensive-pr-review.md command to use `$BASE_SHA` instead of `origin/$BASE_BRANCH` - Resolves issue where Claude was reviewing unrelated changes from other PRs ## Problem As identified in #5651 (comment https://github.com/Comfy-Org/ComfyUI_frontend/pull/5651#issuecomment-3310416767), the Claude automated review was incorrectly analyzing changes that weren't part of the PR being reviewed. The review was mentioning Turkish language removal, linkRenderer changes, and other modifications that weren't in the actual PR diff. ## Root Cause Analysis ### The Issue Explained (from Discord discussion) When Christian Byrne noticed Claude was referencing things from previous reviews on other PRs, we investigated and found: 1. **The backport branch was created from origin/main BEFORE Turkish language support was merged** - Branch state: `main.A` - Backport changes committed: `main.A.Backport` 2. **Turkish language support was then merged into origin/main** - Main branch updated to: `main.A.Turkish` 3. **Claude review workflow checked out `main.A.Backport` and ran git diff against `origin/main`** - This compared: `main.A.Backport <> main.A.Turkish` - The diff showed: `+++Backport` changes and `---Turkish` removal - Because the common parent of both branches was `main.A` ### Why This Happens When using `origin/$BASE_BRANCH`, git resolves to the latest commit on that branch. The diff includes: 1. The PR's actual changes (+++Backport) 2. The reverse of all commits merged to main since the PR was created (---Turkish) This causes Claude to review changes that appear as "removals" of code from other merged PRs, leading to confusing comments about unrelated code. ## Solution Changed the git diff commands to use `$BASE_SHA` directly, which GitHub Actions provides as the exact commit SHA that represents the merge base. This ensures Claude only reviews the actual changes introduced by the PR. ### Before (incorrect): ```bash git diff --name-only "origin/$BASE_BRANCH" # Compares against latest main git diff "origin/$BASE_BRANCH" git diff --name-status "origin/$BASE_BRANCH" ``` ### After (correct): ```bash git diff --name-only "$BASE_SHA" # Compares against merge base git diff "$BASE_SHA" git diff --name-status "$BASE_SHA" ``` ## Technical Details ### GitHub Actions Environment Variables - `BASE_SHA`: The commit SHA of the merge base (where PR branched from main) - `BASE_BRANCH`: Not provided by GitHub Actions (this was the bug) - Using `origin/$BASE_BRANCH` was falling back to comparing against the latest main commit ### Alternative Approaches Considered 1. **Approach 1**: Rebase/update branch before running Claude review - Downside: Changes the PR's commits, not always desirable 2. **Approach 2**: Use BASE_SHA to diff against the merge base ✅ - This is what GitHub's PR diff view does - Shows only the changes introduced by the PR ## Testing The BASE_SHA environment variable is already correctly set in the claude-pr-review.yml workflow (line 88), so this change will work immediately once merged. ## Impact - Claude reviews will now be accurate and only analyze the actual PR changes - No false positives about "removed" code from other PRs - More reliable automated PR review process - Developers won't be confused by comments about code they didn't change ## Verification You can verify this fix by: 1. Creating a PR from an older branch 2. Merging another PR to main 3. Triggering Claude review with the label 4. Claude should only review the PR's changes, not show removals from the newly merged commits ## Credits Thanks to @Christian-Byrne for reporting the issue and @snomiao for the root cause analysis. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com>