From 5e1a2f0aef0ece5ab6e0709c562eb6b16170ad8d Mon Sep 17 00:00:00 2001 From: Xiao Wang <24860335+xwang233@users.noreply.github.com> Date: Fri, 21 Nov 2025 21:56:29 -0800 Subject: [PATCH 1/2] Fix critical GitHub API pagination bugs in auto-merge workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/auto-merge.yml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/auto-merge.yml b/.github/workflows/auto-merge.yml index 9f7f4921093..de4bb076b67 100644 --- a/.github/workflows/auto-merge.yml +++ b/.github/workflows/auto-merge.yml @@ -61,19 +61,19 @@ jobs: pr = data; } else { // For check_run events, we need to find the PR - const prs = await github.rest.pulls.list({ + const prsData = await github.paginate(github.rest.pulls.list, { owner: context.repo.owner, repo: context.repo.repo, state: 'open', head: `${context.repo.owner}:${context.payload.check_run?.head_branch}`, }); - if (prs.data.length === 0) { + if (prsData.length === 0) { core.info('No open PR found for this commit'); return { should_skip: true }; } - pr = prs.data[0]; + pr = prsData[0]; } // Defensive checks: skip if PR is from a fork @@ -137,7 +137,7 @@ jobs: }; // 1. Get all commit statuses (for nvfuser-ci and other status checks) - const { data: statuses } = await github.rest.repos.listCommitStatusesForRef({ + const statuses = await github.paginate(github.rest.repos.listCommitStatusesForRef, { owner: context.repo.owner, repo: context.repo.repo, ref: sha, @@ -170,11 +170,12 @@ jobs: ); // 2. Get all check runs - const { data: checkRuns } = await github.rest.checks.listForRef({ + const checkRunsData = await github.paginate(github.rest.checks.listForRef, { owner: context.repo.owner, repo: context.repo.repo, ref: sha, }); + const checkRuns = { check_runs: checkRunsData }; core.info(`Found ${checkRuns.check_runs.length} check runs`); @@ -345,7 +346,7 @@ jobs: const statusText = statusLines.join('\n'); // Find the comment with placeholders - const { data: comments } = await github.rest.issues.listComments({ + const comments = await github.paginate(github.rest.issues.listComments, { owner: context.repo.owner, repo: context.repo.repo, issue_number: pr_number, From ae5a5289c2b2e847ee12f86f78fa727004e8fce5 Mon Sep 17 00:00:00 2001 From: Xiao Wang <24860335+xwang233@users.noreply.github.com> Date: Fri, 21 Nov 2025 22:04:10 -0800 Subject: [PATCH 2/2] Simplify auto-merge logic by removing confusing pr_approved check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/auto-merge.yml | 46 +++++++------------------------- 1 file changed, 10 insertions(+), 36 deletions(-) diff --git a/.github/workflows/auto-merge.yml b/.github/workflows/auto-merge.yml index de4bb076b67..6cbb38afa04 100644 --- a/.github/workflows/auto-merge.yml +++ b/.github/workflows/auto-merge.yml @@ -132,8 +132,7 @@ jobs: no_failed_checks_reason: '', pr_mergeable: false, pr_mergeable_reason: '', - pr_approved: false, - pr_approved_reason: '', + pr_mergeable_state: '', }; // 1. Get all commit statuses (for nvfuser-ci and other status checks) @@ -237,6 +236,9 @@ jobs: core.info('Refetched PR to get mergeable status'); } + // Store mergeable_state for display in status comment + checks.pr_mergeable_state = pr.mergeable_state; + if (pr.mergeable === false) { checks.pr_mergeable = false; checks.pr_mergeable_reason = 'has merge conflicts'; @@ -251,35 +253,10 @@ jobs: core.info('✅ PR is mergeable'); } - // 5. Check approval status based on mergeable_state - // mergeable_state values: clean, blocked, unstable, behind, dirty, unknown, draft - // - 'blocked': branch protection rules not satisfied (missing approvals or required checks) - // - 'clean': all requirements met, ready to merge - // - 'unstable'/'behind': mergeable but pending/failed non-required checks - // Note: We check approvals separately from check completion status - if (pr.mergeable_state === 'blocked') { - // Blocked means branch protection rules aren't satisfied - // This could be missing approvals OR required checks not passing - checks.pr_approved = false; - checks.pr_approved_reason = 'branch protection rules not satisfied'; - core.info('❌ PR approval: blocked by branch protection'); - } else if (pr.mergeable_state === 'clean' || pr.mergeable_state === 'unstable' || pr.mergeable_state === 'behind') { - // These states mean the PR has required approvals (not blocked) - // Check completion is validated separately in no_failed_checks - checks.pr_approved = true; - core.info('✅ PR is approved'); - } else { - // Other states (dirty, unknown, draft, etc.) - checks.pr_approved = false; - checks.pr_approved_reason = `mergeable_state: ${pr.mergeable_state}`; - core.info(`❌ PR approval: ${checks.pr_approved_reason}`); - } - // Determine if ready to merge const ready = checks.internal_ci_passed && checks.no_failed_checks && - checks.pr_mergeable && - checks.pr_approved; + checks.pr_mergeable; return { ready: ready, @@ -311,14 +288,6 @@ jobs: const checks = checkResult.checks; const statusLines = []; - // PR approved - if (checks.pr_approved) { - statusLines.push('✅ PR is approved'); - } else { - const reason = checks.pr_approved_reason ? ` (${checks.pr_approved_reason})` : ''; - statusLines.push(`❌ PR is approved${reason}`); - } - // Internal CI if (checks.internal_ci_passed) { statusLines.push('✅ Internal CI is finished'); @@ -343,6 +312,11 @@ jobs: statusLines.push(`❌ PR is mergeable${reason}`); } + // Show mergeable_state for transparency + if (checks.pr_mergeable_state) { + statusLines.push(`ℹī¸ PR mergeable_state: \`${checks.pr_mergeable_state}\``); + } + const statusText = statusLines.join('\n'); // Find the comment with placeholders