Surface GitHub check suites awaiting approval (#6717)

* Surface GitHub check suites awaiting approval to unblock merge - Query the check-suites API endpoint to find suites with an "action_required" conclusion, which are often workflows awaiting "Approve and run" and do not have any associated check runs. - Map the "action_required" status distinctly instead of treating it as a standard failure or omitting it entirely. - Update the UI to render these suites with a warning icon, a dedicated "Action required" label, and a localized hint explaining that manual approval is required on GitHub. - Count "action_required" checks as failed/blocking when deriving overall PR and task statuses so the UI does not report all checks passing. * Enhance visibility and handling of action-required PR check suites * Include check suite IDs in pending approval check names and URLs to allow navigating directly to the specific workflow run. * Add an "action required" count badge to PR dialog and page checks tabs. * Prioritize action-required checks in the checks preview summary. * Use correct check run state for the action-required fallback hint in the right sidebar details panel. * Add translations for the new status across all supported locales.
2026-09-24 16:02:41 +00:00 · 2026-06-29 12:05:57 -07:00
parent 82d275c3ac
commit 1c30d28113
22 changed files with 562 additions and 119 deletions
@@ -92,19 +92,21 @@ describe('getPRChecks', () => {

  it('queries check-runs by PR head SHA when GitHub remote metadata is available', async () => {
    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
-    ghExecFileAsyncMock.mockResolvedValueOnce({
-      stdout: JSON.stringify({
-        check_runs: [
-          {
-            name: 'build',
-            status: 'completed',
-            conclusion: 'success',
-            html_url: 'https://github.com/acme/widgets/actions/runs/1',
-            details_url: null
-          }
-        ]
+    ghExecFileAsyncMock
+      .mockResolvedValueOnce({
+        stdout: JSON.stringify({
+          check_runs: [
+            {
+              name: 'build',
+              status: 'completed',
+              conclusion: 'success',
+              html_url: 'https://github.com/acme/widgets/actions/runs/1',
+              details_url: null
+            }
+          ]
+        })
      })
-    })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })

    const checks = await getPRChecks('/repo-root', 42, 'head-oid')

@@ -123,10 +125,82 @@ describe('getPRChecks', () => {
    ])
  })

-  it('falls back to gh pr checks when the head SHA has no check runs', async () => {
+  it('surfaces an action_required check suite that has no check run', async () => {
+    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
+    ghExecFileAsyncMock
+      .mockResolvedValueOnce({
+        stdout: JSON.stringify({
+          check_runs: [
+            {
+              name: 'track-community-pr',
+              status: 'completed',
+              conclusion: 'success',
+              html_url: 'https://github.com/acme/widgets/actions/runs/1',
+              details_url: null
+            }
+          ]
+        })
+      })
+      .mockResolvedValueOnce({
+        stdout: JSON.stringify({
+          check_suites: [
+            {
+              id: 1000,
+              status: 'completed',
+              conclusion: 'success',
+              app: { name: 'GitHub Actions' }
+            },
+            {
+              id: 1001,
+              status: 'completed',
+              conclusion: 'action_required',
+              app: { name: 'GitHub Actions' }
+            },
+            {
+              id: 1002,
+              status: 'completed',
+              conclusion: 'action_required',
+              app: { name: 'GitHub Actions' }
+            }
+          ]
+        })
+      })
+
+    const checks = await getPRChecks('/repo-root', 42, 'head-oid')
+
+    expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
+      2,
+      ['api', '--cache', '60s', 'repos/acme/widgets/commits/head-oid/check-suites?per_page=100'],
+      { cwd: '/repo-root' }
+    )
+    expect(checks).toEqual([
+      {
+        name: 'track-community-pr',
+        status: 'completed',
+        conclusion: 'success',
+        url: 'https://github.com/acme/widgets/actions/runs/1',
+        workflowRunId: 1
+      },
+      {
+        name: 'GitHub Actions #1001',
+        status: 'completed',
+        conclusion: 'action_required',
+        url: 'https://github.com/acme/widgets/commits/head-oid/checks#check-suite-1001'
+      },
+      {
+        name: 'GitHub Actions #1002',
+        status: 'completed',
+        conclusion: 'action_required',
+        url: 'https://github.com/acme/widgets/commits/head-oid/checks#check-suite-1002'
+      }
+    ])
+  })
+
+  it('falls back to gh pr checks when the head SHA has no check runs or suites', async () => {
    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
    ghExecFileAsyncMock
      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_runs: [] }) })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })
      .mockResolvedValueOnce({
        stdout: JSON.stringify([
          { name: 'verify', state: 'PENDING', link: 'https://example.com/verify' }
@@ -136,7 +210,7 @@ describe('getPRChecks', () => {
    const checks = await getPRChecks('/repo-root', 42, 'head-oid')

    expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(
-      2,
+      3,
      ['pr', 'checks', '42', '--json', 'name,state,link', '--repo', 'acme/widgets'],
      { cwd: '/repo-root' }
    )
@@ -151,39 +225,76 @@ describe('getPRChecks', () => {
    ])
  })

-  it('maps remaining completed GitHub conclusions to failure', async () => {
+  it('maps stale and startup_failure conclusions to failure and action_required to its own state', async () => {
    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
-    ghExecFileAsyncMock.mockResolvedValueOnce({
-      stdout: JSON.stringify({
-        check_runs: [
-          {
-            name: 'needs-approval',
-            status: 'completed',
-            conclusion: 'action_required',
-            html_url: 'https://github.com/acme/widgets/actions/runs/1',
-            details_url: null
-          },
-          {
-            name: 'old-run',
-            status: 'completed',
-            conclusion: 'stale',
-            html_url: 'https://github.com/acme/widgets/actions/runs/2',
-            details_url: null
-          },
-          {
-            name: 'boot',
-            status: 'completed',
-            conclusion: 'startup_failure',
-            html_url: 'https://github.com/acme/widgets/actions/runs/3',
-            details_url: null
-          }
-        ]
+    ghExecFileAsyncMock
+      .mockResolvedValueOnce({
+        stdout: JSON.stringify({
+          check_runs: [
+            {
+              name: 'needs-approval',
+              status: 'completed',
+              conclusion: 'action_required',
+              html_url: 'https://github.com/acme/widgets/actions/runs/1',
+              details_url: null
+            },
+            {
+              name: 'old-run',
+              status: 'completed',
+              conclusion: 'stale',
+              html_url: 'https://github.com/acme/widgets/actions/runs/2',
+              details_url: null
+            },
+            {
+              name: 'boot',
+              status: 'completed',
+              conclusion: 'startup_failure',
+              html_url: 'https://github.com/acme/widgets/actions/runs/3',
+              details_url: null
+            }
+          ]
+        })
      })
-    })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })

    const checks = await getPRChecks('/repo-root', 42, 'head-oid')

-    expect(checks.map((check) => check.conclusion)).toEqual(['failure', 'failure', 'failure'])
+    expect(checks.map((check) => check.conclusion)).toEqual([
+      'action_required',
+      'failure',
+      'failure'
+    ])
+  })
+
+  it('surfaces an action_required suite even when there are zero check runs', async () => {
+    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
+    ghExecFileAsyncMock
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_runs: [] }) })
+      .mockResolvedValueOnce({
+        stdout: JSON.stringify({
+          check_suites: [
+            {
+              id: 1001,
+              status: 'completed',
+              conclusion: 'action_required',
+              app: { name: 'GitHub Actions' }
+            }
+          ]
+        })
+      })
+
+    const checks = await getPRChecks('/repo-root', 42, 'head-oid')
+
+    // Why: must not fall through to `gh pr checks` — the suite is the only signal.
+    expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2)
+    expect(checks).toEqual([
+      {
+        name: 'GitHub Actions #1001',
+        status: 'completed',
+        conclusion: 'action_required',
+        url: 'https://github.com/acme/widgets/commits/head-oid/checks#check-suite-1001'
+      }
+    ])
  })

  it('treats gh pr checks "no checks reported" as an empty check list', async () => {
@@ -191,6 +302,7 @@ describe('getPRChecks', () => {
    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
    ghExecFileAsyncMock
      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_runs: [] }) })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })
      .mockRejectedValueOnce(
        Object.assign(new Error('Command failed: gh pr checks 42'), {
          stderr: "no checks reported on the 'codex/keybindings-toml' branch\n",
@@ -210,6 +322,7 @@ describe('getPRChecks', () => {
    getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
    ghExecFileAsyncMock
      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_runs: [] }) })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })
      .mockRejectedValueOnce(
        Object.assign(new Error('Command failed: gh pr checks 42'), {
          stderr: 'GraphQL: Could not resolve to a PullRequest',
@@ -291,6 +404,7 @@ describe('getPRChecks', () => {
          ]
        })
      })
+      .mockResolvedValueOnce({ stdout: JSON.stringify({ check_suites: [] }) })
      .mockResolvedValueOnce({
        stdout: JSON.stringify([
          {
@@ -3057,15 +3057,28 @@ export async function getPRChecks(
            details_url: string | null
          }[]
        }
-        if (data.check_runs.length > 0) {
-          return data.check_runs.map((d) => ({
-            name: d.name,
-            status: mapCheckRunRESTStatus(d.status),
-            conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion),
-            url: d.details_url || d.html_url || null,
-            ...(typeof d.id === 'number' ? { checkRunId: d.id } : {}),
-            workflowRunId: parseActionsRunId(d.details_url || d.html_url || null)
-          }))
+        const checkRuns: PRCheckDetail[] = data.check_runs.map((d) => ({
+          name: d.name,
+          status: mapCheckRunRESTStatus(d.status),
+          conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion),
+          url: d.details_url || d.html_url || null,
+          ...(typeof d.id === 'number' ? { checkRunId: d.id } : {}),
+          workflowRunId: parseActionsRunId(d.details_url || d.html_url || null)
+        }))
+        // Why: a workflow awaiting "Approve and run" produces a check SUITE with
+        // no check run, and is absent from statusCheckRollup — so without this
+        // the panel shows "no checks"/"all passing" while auto-merge fails with
+        // "unstable status". Surface them as their own check rows. Fetch even
+        // when there are zero check runs, since that is the exact case GitHub
+        // leaves the PR unstable with nothing to show.
+        const pendingApprovalChecks = await getPendingApprovalCheckSuites(
+          ownerRepo,
+          headSha,
+          ghOptions,
+          options?.noCache
+        )
+        if (checkRuns.length > 0 || pendingApprovalChecks.length > 0) {
+          return [...checkRuns, ...pendingApprovalChecks]
        }
      } catch (err) {
        // Why: a PR can outlive the cached head SHA after force-pushes or remote
@@ -3086,6 +3099,88 @@ export async function getPRChecks(
  }
 }

+/**
+ * Fetch check SUITES that need manual action (e.g. a workflow awaiting approval).
+ * These have no check run and are absent from statusCheckRollup, yet they keep a
+ * PR in "unstable status" and block auto-merge. We surface one synthetic check
+ * row per such suite so both check panels show it.
+ */
+async function getPendingApprovalCheckSuites(
+  ownerRepo: OwnerRepo,
+  headSha: string,
+  ghOptions: GhExecOptions,
+  noCache?: boolean
+): Promise<PRCheckDetail[]> {
+  const cacheArgs = noCache ? [] : ['--cache', '60s']
+  try {
+    const { stdout } = await ghExecFileAsync(
+      [
+        'api',
+        ...cacheArgs,
+        `repos/${ownerRepo.owner}/${ownerRepo.repo}/commits/${encodeURIComponent(headSha)}/check-suites?per_page=100`
+      ],
+      ghOptions
+    )
+    noteRateLimitSpend('core')
+    const data = JSON.parse(stdout) as {
+      check_suites?: {
+        id?: number | null
+        status: string | null
+        conclusion: string | null
+        app?: { name?: string | null; slug?: string | null } | null
+      }[]
+    }
+    return (data.check_suites ?? [])
+      .filter((suite) => suite.conclusion?.toLowerCase() === 'action_required')
+      .map((suite, index) => ({
+        name: getPendingApprovalCheckSuiteName(suite, headSha, index),
+        status: 'completed' as const,
+        conclusion: 'action_required' as const,
+        // Why: check suites expose no per-PR details URL; the checks tab is the
+        // closest actionable destination for approving the run.
+        url: getPendingApprovalCheckSuiteUrl(ownerRepo, headSha, suite.id)
+      }))
+  } catch (err) {
+    // Why: this is a best-effort enrichment; a failed suites lookup must not
+    // blank out the check runs we already fetched successfully.
+    console.warn('getPendingApprovalCheckSuites failed:', err)
+    return []
+  }
+}
+
+function getPendingApprovalCheckSuiteName(
+  suite: {
+    id?: number | null
+    app?: { name?: string | null; slug?: string | null } | null
+  },
+  headSha: string,
+  index: number
+): string {
+  const appName = suite.app?.name ?? suite.app?.slug ?? null
+  const suiteId = typeof suite.id === 'number' && Number.isFinite(suite.id) ? `#${suite.id}` : null
+  if (appName && suiteId) {
+    return `${appName} ${suiteId}`
+  }
+  if (appName) {
+    return appName
+  }
+  if (suiteId) {
+    return suiteId
+  }
+  return `${headSha.slice(0, 12)}:${index + 1}`
+}
+
+function getPendingApprovalCheckSuiteUrl(
+  ownerRepo: OwnerRepo,
+  headSha: string,
+  suiteId: number | null | undefined
+): string {
+  const base = `https://github.com/${ownerRepo.owner}/${ownerRepo.repo}/commits/${headSha}/checks`
+  return typeof suiteId === 'number' && Number.isFinite(suiteId)
+    ? `${base}#check-suite-${suiteId}`
+    : base
+}
+
 function nullableString(value: unknown): string | null {
  return typeof value === 'string' && value.length > 0 ? value : null
 }
@@ -22,7 +22,7 @@ const conclusionMap: Record<string, PRCheckDetail['conclusion']> = {
  timed_out: 'timed_out',
  skipped: 'skipped',
  neutral: 'neutral',
-  action_required: 'failure',
+  action_required: 'action_required',
  stale: 'failure',
  startup_failure: 'failure'
 }
@@ -61,7 +61,10 @@ export function mapCheckConclusion(state: string): PRCheckDetail['conclusion'] {
  if (s === 'FAILURE' || s === 'FAIL') {
    return 'failure'
  }
-  if (s === 'ACTION_REQUIRED' || s === 'STALE' || s === 'STARTUP_FAILURE') {
+  if (s === 'ACTION_REQUIRED') {
+    return 'action_required'
+  }
+  if (s === 'STALE' || s === 'STARTUP_FAILURE') {
    return 'failure'
  }
  if (s === 'CANCELLED') {
@@ -134,6 +137,9 @@ export function deriveCheckStatus(rollup: unknown[] | null | undefined): CheckSt
      conclusion === 'FAILURE' ||
      conclusion === 'TIMED_OUT' ||
      conclusion === 'CANCELLED' ||
+      // Why: action_required (e.g. an unapproved workflow run) blocks merge until
+      // someone acts; treat it as needs-attention rather than a silent pass.
+      conclusion === 'ACTION_REQUIRED' ||
      state === 'FAILURE' ||
      state === 'ERROR'
    ) {