Fix critical GitHub API pagination bugs in auto-merge workflow#5580
Fix critical GitHub API pagination bugs in auto-merge workflow#5580
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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
|
|
!build |
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.