-
Notifications
You must be signed in to change notification settings - Fork 74
Fix critical GitHub API pagination bugs in auto-merge workflow #5580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This fixes a severe bug where the auto-merge workflow only checked the first 30 commit statuses, causing it to miss failures and incorrectly merge PRs with failing checks. Root cause analysis: - PR #5578 had 2 failed GB200 tests at 23:23-23:25 UTC - By 03:29 UTC, 27+ new successful statuses pushed failures past position 30 - Workflow only fetched first page (30 items), saw 0 failures, and merged Fixed 4 critical pagination issues: 1. listCommitStatusesForRef (line 140) - CRITICAL: Only saw 30 of 57 statuses 2. checks.listForRef (line 173) - Could miss failed checks if >30 exist 3. issues.listComments (line 349) - Wouldn't find status comment if >30 comments 4. pulls.list (line 64) - Could miss PR if >30 open PRs on branch All API calls now use github.paginate() to retrieve complete results. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The pr_approved check was derived from mergeable_state using complex logic that was often misleading. Some PRs showed "PR is approved" when they weren't actually approved, because the logic conflated approval status with other mergeable_state values. Changes: - Removed pr_approved and pr_approved_reason from checks object - Removed complex approval logic based on mergeable_state interpretation - Removed pr_approved from merge ready condition - Removed "✅/❌ PR is approved" from status comment - Added "ℹ️ PR mergeable_state: `value`" to status comment for transparency The workflow now relies on GitHub's mergeable_state directly through the pr_mergeable check, which already validates merge conflicts and branch protection rules. Users can see the actual mergeable_state value in the status comment to understand PR status. This makes the logic cleaner and more reliable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Description
|
| Relevant files | |||
|---|---|---|---|
| Bug fix |
|
PR Reviewer Guide
Here are some key observations to aid the review process:
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
| ⚡ Recommended focus areas for review |
Logic Simplification
pr_approved check logic that was deriving approval status from mergeable_state. While this simplifies the code and removes potential confusion, it should be verified that the simplified logic (checking only internal_ci_passed, no_failed_checks, and pr_mergeable) provides equivalent or better accuracy for determining when a PR is ready to merge. The removed logic was specifically handling branch protection rules and approval requirements. |
Greptile OverviewGreptile SummaryFixed critical bug where auto-merge workflow only checked first 30 GitHub API results, causing it to miss failed checks and incorrectly merge PRs. Key Changes:
Impact: Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant GH as GitHub Event
participant WF as Auto-merge Workflow
participant API as GitHub API
participant PR as Pull Request
GH->>WF: Trigger (label/review/check_run/dispatch)
Note over WF: Step 1: Get PR Details
WF->>API: Get PR info
alt check_run event
WF->>API: paginate(pulls.list) - find PR by branch
end
API-->>WF: PR data (number, sha, mergeable_state)
Note over WF: Step 2: Check All Conditions
WF->>API: paginate(listCommitStatusesForRef, sha)
API-->>WF: All commit statuses (nvfuser-ci, etc.)
WF->>API: paginate(checks.listForRef, sha)
API-->>WF: All check runs
WF->>API: getCombinedStatusForRef(sha)
API-->>WF: Combined status
Note over WF: Validate all checks
alt nvfuser-ci passed
WF->>WF: ✅ internal_ci_passed
else nvfuser-ci failed/pending
WF->>WF: ❌ internal_ci_passed
end
alt no failed statuses/checks
WF->>WF: ✅ no_failed_checks
else has failures
WF->>WF: ❌ no_failed_checks
end
alt mergeable and clean/unstable
WF->>WF: ✅ pr_mergeable
else conflicts or blocked
WF->>WF: ❌ pr_mergeable
end
Note over WF: Step 3: Update Status Comment
WF->>API: paginate(issues.listComments, pr_number)
API-->>WF: All comments
WF->>API: updateComment(status)
API-->>PR: Comment updated with status
Note over WF: Step 4: Merge if Ready
alt all checks passed
WF->>API: pulls.merge(squash)
API-->>PR: PR merged
WF->>API: removeLabel('enable-auto-merge')
else checks failed
WF->>WF: Skip merge
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, no comments
|
!build |
crcrpar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks clinical to me
Summary
Fixes a critical bug where the auto-merge workflow only fetched the first 30 results from GitHub API list operations, causing it to miss failed checks and incorrectly merge PRs.
Root Cause
PR #5578 had 2 failed GB200 tests that occurred early in the CI run. By the time the auto-merge action ran 4+ hours later, 27 newer successful statuses had been created. Since the workflow used unpaginated API calls (default limit: 30 items), the failed statuses were pushed beyond the first page and never detected.
Changes
Fixed 4 GitHub API calls to use
github.paginate():listCommitStatusesForRef- Was only checking 30 of 57+ statuseschecks.listForRef- Could miss failed checks if >30 existissues.listComments- Could miss status comment if >30 commentspulls.list- Could miss PR if >30 open PRs on branchAlso simplified the
pr_approvedcheck logic which was deriving approval status frommergeable_statein a confusing way. The workflow now shows the actualmergeable_statevalue in status comments for transparency.Impact
The auto-merge workflow will now correctly detect ALL failures regardless of how many statuses exist, preventing incorrect merges like #5578.