diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index fbf27f26a..9f9e5e9f1 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -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([ { diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 3b2e605a5..a2b9c598e 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -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 { + 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 } diff --git a/src/main/github/mappers.ts b/src/main/github/mappers.ts index 54235a434..f42935f90 100644 --- a/src/main/github/mappers.ts +++ b/src/main/github/mappers.ts @@ -22,7 +22,7 @@ const conclusionMap: Record = { 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' ) { diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index b6a051a66..d5c14dc76 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -4284,6 +4284,7 @@ function CommentReplyForm({ const CHECK_SORT_ORDER: Record = { failure: 0, timed_out: 0, + action_required: 0, cancelled: 1, pending: 2, neutral: 3, @@ -4309,6 +4310,9 @@ function getCheckStatusLabel(check: PRCheckDetail): string { if (conclusion === 'timed_out') { return 'Timed out' } + if (conclusion === 'action_required') { + return 'Action required' + } if (conclusion === 'neutral') { return 'Neutral' } @@ -4327,6 +4331,7 @@ function getCheckStatusLabel(check: PRCheckDetail): string { function getCheckCounts(checks: PRCheckDetail[]): { passing: number failing: number + needsAction: number pending: number skipped: number neutral: number @@ -4336,6 +4341,8 @@ function getCheckCounts(checks: PRCheckDetail[]): { const conclusion = getCheckConclusion(check) if (conclusion === 'success') { counts.passing += 1 + } else if (conclusion === 'action_required') { + counts.needsAction += 1 } else if (['failure', 'cancelled', 'timed_out'].includes(conclusion)) { counts.failing += 1 } else if (conclusion === 'skipped') { @@ -4347,7 +4354,7 @@ function getCheckCounts(checks: PRCheckDetail[]): { } return counts }, - { passing: 0, failing: 0, pending: 0, skipped: 0, neutral: 0 } + { passing: 0, failing: 0, needsAction: 0, pending: 0, skipped: 0, neutral: 0 } ) } @@ -4359,6 +4366,11 @@ function getChecksSummaryLabel(checks: PRCheckDetail[]): string { if (counts.failing > 0) { return `${counts.failing} ${counts.failing === 1 ? 'check' : 'checks'} failing` } + // Why: action_required (e.g. a workflow awaiting approval) blocks merge but is + // not a failure; call it out distinctly so users know a manual step is needed. + if (counts.needsAction > 0) { + return `${counts.needsAction} ${counts.needsAction === 1 ? 'check needs' : 'checks need'} action` + } if (counts.pending > 0) { return `${counts.pending} ${counts.pending === 1 ? 'check' : 'checks'} pending` } @@ -4436,19 +4448,23 @@ function ChecksTab({ const SummaryIcon = counts.failing > 0 ? CHECK_ICON.failure - : counts.pending > 0 - ? CHECK_ICON.pending - : list.length > 0 - ? CHECK_ICON.success - : CircleDashed + : counts.needsAction > 0 + ? CHECK_ICON.action_required + : counts.pending > 0 + ? CHECK_ICON.pending + : list.length > 0 + ? CHECK_ICON.success + : CircleDashed const summaryColor = counts.failing > 0 ? CHECK_COLOR.failure - : counts.pending > 0 - ? CHECK_COLOR.pending - : list.length > 0 - ? CHECK_COLOR.success - : 'text-muted-foreground' + : counts.needsAction > 0 + ? CHECK_COLOR.action_required + : counts.pending > 0 + ? CHECK_COLOR.pending + : list.length > 0 + ? CHECK_COLOR.success + : 'text-muted-foreground' const canFixBrokenChecks = Boolean((repoId ?? item.repoId) && failedChecks.length > 0) const handleRefresh = useCallback(async (): Promise => { @@ -5052,10 +5068,15 @@ function ChecksTab({ {!state?.error && !hasOutput && !hasAnnotations && !hasJobs && (
- {translate( - 'auto.components.GitHubItemDialog.744197c84d', - 'No inline output is available for this check.' - )} + {getCheckConclusion(check) === 'action_required' + ? translate( + 'auto.components.GitHubItemDialog.checkActionRequiredHint', + 'Needs a manual action on GitHub (e.g. approving the run) to unblock merging.' + ) + : translate( + 'auto.components.GitHubItemDialog.744197c84d', + 'No inline output is available for this check.' + )}
)} @@ -5141,6 +5162,18 @@ function ChecksTab({ className: CHECK_COLOR.failure }) } + if (counts.needsAction > 0) { + countChips.push({ + label: translate( + 'auto.components.GitHubItemDialog.checksNeedActionChip', + '{{value0}} action required', + { + value0: counts.needsAction + } + ), + className: CHECK_COLOR.action_required + }) + } if (counts.pending > 0) { countChips.push({ label: translate('auto.components.GitHubItemDialog.18f80e1329', '{{value0}} pending', { diff --git a/src/renderer/src/components/PullRequestPage.tsx b/src/renderer/src/components/PullRequestPage.tsx index 29fc73ebe..6c9dace94 100644 --- a/src/renderer/src/components/PullRequestPage.tsx +++ b/src/renderer/src/components/PullRequestPage.tsx @@ -4288,6 +4288,7 @@ function CommentReplyForm({ const CHECK_SORT_ORDER: Record = { failure: 0, timed_out: 0, + action_required: 0, cancelled: 1, pending: 2, neutral: 3, @@ -4313,6 +4314,9 @@ function getCheckStatusLabel(check: PRCheckDetail): string { if (conclusion === 'timed_out') { return 'Timed out' } + if (conclusion === 'action_required') { + return 'Action required' + } if (conclusion === 'neutral') { return 'Neutral' } @@ -4331,6 +4335,7 @@ function getCheckStatusLabel(check: PRCheckDetail): string { function getCheckCounts(checks: PRCheckDetail[]): { passing: number failing: number + needsAction: number pending: number skipped: number neutral: number @@ -4340,6 +4345,8 @@ function getCheckCounts(checks: PRCheckDetail[]): { const conclusion = getCheckConclusion(check) if (conclusion === 'success') { counts.passing += 1 + } else if (conclusion === 'action_required') { + counts.needsAction += 1 } else if (['failure', 'cancelled', 'timed_out'].includes(conclusion)) { counts.failing += 1 } else if (conclusion === 'skipped') { @@ -4351,7 +4358,7 @@ function getCheckCounts(checks: PRCheckDetail[]): { } return counts }, - { passing: 0, failing: 0, pending: 0, skipped: 0, neutral: 0 } + { passing: 0, failing: 0, needsAction: 0, pending: 0, skipped: 0, neutral: 0 } ) } @@ -4363,6 +4370,11 @@ function getChecksSummaryLabel(checks: PRCheckDetail[]): string { if (counts.failing > 0) { return `${counts.failing} ${counts.failing === 1 ? 'check' : 'checks'} failing` } + // Why: action_required (e.g. a workflow awaiting approval) blocks merge but is + // not a failure; call it out distinctly so users know a manual step is needed. + if (counts.needsAction > 0) { + return `${counts.needsAction} ${counts.needsAction === 1 ? 'check needs' : 'checks need'} action` + } if (counts.pending > 0) { return `${counts.pending} ${counts.pending === 1 ? 'check' : 'checks'} pending` } @@ -4535,19 +4547,23 @@ function ChecksTab({ const SummaryIcon = counts.failing > 0 ? CHECK_ICON.failure - : counts.pending > 0 - ? CHECK_ICON.pending - : list.length > 0 - ? CHECK_ICON.success - : CircleDashed + : counts.needsAction > 0 + ? CHECK_ICON.action_required + : counts.pending > 0 + ? CHECK_ICON.pending + : list.length > 0 + ? CHECK_ICON.success + : CircleDashed const summaryColor = counts.failing > 0 ? CHECK_COLOR.failure - : counts.pending > 0 - ? CHECK_COLOR.pending - : list.length > 0 - ? CHECK_COLOR.success - : 'text-muted-foreground' + : counts.needsAction > 0 + ? CHECK_COLOR.action_required + : counts.pending > 0 + ? CHECK_COLOR.pending + : list.length > 0 + ? CHECK_COLOR.success + : 'text-muted-foreground' const canFixBrokenChecks = Boolean((repoId ?? item.repoId) && failedChecks.length > 0) const handleRefresh = useCallback(async (): Promise => { @@ -5145,10 +5161,15 @@ function ChecksTab({ {!state?.error && !hasOutput && !hasAnnotations && !hasJobs && (
- {translate( - 'auto.components.PullRequestPage.1550675e5f', - 'No inline output is available for this check.' - )} + {getCheckConclusion(check) === 'action_required' + ? translate( + 'auto.components.PullRequestPage.checkActionRequiredHint', + 'Needs a manual action on GitHub (e.g. approving the run) to unblock merging.' + ) + : translate( + 'auto.components.PullRequestPage.1550675e5f', + 'No inline output is available for this check.' + )}
)} @@ -5270,6 +5291,18 @@ function ChecksTab({ className: CHECK_COLOR.failure }) } + if (counts.needsAction > 0) { + countChips.push({ + label: translate( + 'auto.components.PullRequestPage.checksNeedActionChip', + '{{value0}} action required', + { + value0: counts.needsAction + } + ), + className: CHECK_COLOR.action_required + }) + } if (counts.pending > 0) { countChips.push({ label: translate('auto.components.PullRequestPage.88267924d5', '{{value0}} pending', { diff --git a/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx b/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx index 80752aeee..2a9a8c4ec 100644 --- a/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx +++ b/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx @@ -42,6 +42,11 @@ function getCheckStatusLabel(check: CheckStatusLike): string { return translate('auto.components.editor.CheckRunDetailsPanel.91a4c7e2b0', 'Cancelled') case 'timed_out': return translate('auto.components.editor.CheckRunDetailsPanel.2f6d8a1c45', 'Timed out') + case 'action_required': + return translate( + 'auto.components.editor.CheckRunDetailsPanel.actionRequired', + 'Action required' + ) case 'skipped': return translate('auto.components.editor.CheckRunDetailsPanel.7b3e9d4f12', 'Skipped') case 'neutral': diff --git a/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx b/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx index e7ee2002d..f5785a214 100644 --- a/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx @@ -210,4 +210,22 @@ describe('ChecksList expanded check details', () => { expect(stickyBar?.textContent).toContain('View full details') expect(stickyBar?.textContent).not.toContain('View full logs') }) + + it('uses resolved details when showing the action-required fallback hint', async () => { + renderChecksList({ + onLoadCheckDetails: async () => ({ + ...checkDetails, + conclusion: 'action_required', + title: null, + jobs: [] + }) + }) + + await act(async () => { + await Promise.resolve() + }) + + expect(container.textContent).toContain('manual action on GitHub') + expect(container.textContent).not.toContain('No inline details are available') + }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx index 988856a82..62268e292 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx @@ -118,7 +118,8 @@ export const CHECK_ICON: Record = { @@ -128,7 +129,8 @@ export const CHECK_COLOR: Record = { neutral: 'text-muted-foreground', skipped: 'text-muted-foreground/60', cancelled: 'text-muted-foreground/60', - timed_out: 'text-rose-500' + timed_out: 'text-rose-500', + action_required: 'text-amber-500' } type ConflictReview = { @@ -504,6 +506,7 @@ export function ConflictTriageStrip({ const CHECK_SORT_ORDER: Record = { failure: 0, timed_out: 0, + action_required: 0, cancelled: 1, pending: 2, neutral: 3, @@ -539,7 +542,12 @@ function getCheckConclusion(check: PRCheckDetail): NonNullable - {translate( - 'auto.components.right.sidebar.checks.panel.content.e15a8b77ef', - 'No inline details are available for this check.' - )} + {getCheckConclusion(detailsStatusCheck) === 'action_required' + ? translate( + 'auto.components.right.sidebar.checks.panel.content.actionRequiredHint', + 'Needs a manual action on GitHub (e.g. approving the run) to unblock merging.' + ) + : translate( + 'auto.components.right.sidebar.checks.panel.content.e15a8b77ef', + 'No inline details are available for this check.' + )} )} @@ -971,10 +987,7 @@ export function ChecksList({ [checkDetailsContextKey, sorted] ) const passingCount = checks.filter((c) => c.conclusion === 'success').length - const failingCount = checks.filter( - (c) => - c.conclusion === 'failure' || c.conclusion === 'cancelled' || c.conclusion === 'timed_out' - ).length + const failingCount = checks.filter((c) => isFailedCheck(c)).length const pendingCount = checks.filter( (c) => c.conclusion === 'pending' || c.conclusion === null ).length diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts index 4840cadfd..5471b89a5 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts @@ -273,4 +273,41 @@ describe('buildParentPrChecksProjection', () => { expect(projection.rows[0]?.detailNames).toEqual(['build']) expect(projection.rows[0]?.status).toBe('failing') }) + + it('prioritizes action-required check detail names before truncating the preview', () => { + const repo = makeRepo() + const worktree = makeWorktree({ id: 'repo-1::/feature' }) + const review = makeReview({ status: 'failure', headSha: 'abc123' }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + const checksKey = getGitHubRepoCacheKey( + repo.path, + repo.id, + prChecksCacheSuffix(12, null, 'abc123'), + settings + ) + + const projection = makeProjection({ + worktree, + repo, + hostedReviewCache: { [hostedKey]: { data: review, fetchedAt: 1 } }, + checksCache: { + [checksKey]: { + data: [ + { name: 'build', status: 'completed', conclusion: 'failure', url: null }, + { name: 'lint', status: 'in_progress', conclusion: null, url: null }, + { + name: 'GitHub Actions #1001', + status: 'completed', + conclusion: 'action_required', + url: null + } + ], + fetchedAt: 1, + headSha: 'abc123' + } + } + }) + + expect(projection.rows[0]?.detailNames).toEqual(['GitHub Actions #1001', 'build']) + }) }) diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts index dca4da1e6..be8f3bdb2 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts @@ -204,12 +204,17 @@ function getCheckDetailNames(checks: readonly PRCheckDetail[]): string[] { check.conclusion === 'failure' || check.conclusion === 'timed_out' || check.conclusion === 'cancelled' || + check.conclusion === 'action_required' || check.conclusion === 'pending' || check.conclusion === null || check.status === 'queued' || check.status === 'in_progress' ) - return interesting.slice(0, 2).map((check) => check.name) + const ordered = [ + ...interesting.filter((check) => check.conclusion === 'action_required'), + ...interesting.filter((check) => check.conclusion !== 'action_required') + ] + return ordered.slice(0, 2).map((check) => check.name) } function getGitHubChecksEntry( diff --git a/src/renderer/src/components/task-page-pr-check-summary.test.ts b/src/renderer/src/components/task-page-pr-check-summary.test.ts index dda2528b6..bf774bcf7 100644 --- a/src/renderer/src/components/task-page-pr-check-summary.test.ts +++ b/src/renderer/src/components/task-page-pr-check-summary.test.ts @@ -55,4 +55,19 @@ describe('deriveTaskPagePRCheckSummary', () => { pending: 0 }) }) + + it('counts action_required as failed so a blocked PR never reads as passing', () => { + expect( + deriveTaskPagePRCheckSummary([ + check({ conclusion: 'success' }), + check({ conclusion: 'action_required' }) + ]) + ).toEqual({ + state: 'failure', + total: 2, + passed: 1, + failed: 1, + pending: 0 + }) + }) }) diff --git a/src/renderer/src/components/task-page-pr-check-summary.ts b/src/renderer/src/components/task-page-pr-check-summary.ts index db9fa0ee0..4f9fd2be5 100644 --- a/src/renderer/src/components/task-page-pr-check-summary.ts +++ b/src/renderer/src/components/task-page-pr-check-summary.ts @@ -28,7 +28,10 @@ export function deriveTaskPagePRCheckSummary(checks: PRCheckDetail[]): GitHubPRC } else if ( conclusion === 'failure' || conclusion === 'timed_out' || - conclusion === 'cancelled' + conclusion === 'cancelled' || + // Why: action_required (e.g. a workflow awaiting approval) blocks merge; it + // must count as failed so the summary never reads "passing" while blocked. + conclusion === 'action_required' ) { failed += 1 } else if (isPendingCheck(check)) { diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index a0dffdb2c..43f4f3cfe 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -710,6 +710,7 @@ "d23bbb6416": "{{value0}} skipped", "18f80e1329": "{{value0}} pending", "b1ac991806": "{{value0}} failing", + "checksNeedActionChip": "{{value0}} action required", "311d0cee55": "{{value0}} passing", "e52bed9264": "No checks reported yet", "90020cc1f3": "This pull request has no reported checks yet.", @@ -858,7 +859,8 @@ "to": "to", "activity": "Activity", "noActivity": "No activity yet." - } + }, + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "GitLabItemDialog": { "65e784c1f1": "Reopen", @@ -1183,6 +1185,7 @@ "e6ad0a8d06": "{{value0}} skipped", "88267924d5": "{{value0}} pending", "ae2a34c7b8": "{{value0}} failing", + "checksNeedActionChip": "{{value0}} action required", "7c5035931a": "{{value0}} passing", "a18d01cda3": "No checks reported yet", "3912daf310": "This pull request has no reported checks yet.", @@ -1317,7 +1320,8 @@ "commentTooLarge": "Comment is too large to submit safely.", "8ff5ae8866": "Assignees", "82c87eceb9": "Edit assignees", - "1ff5d979df": "No one assigned" + "1ff5d979df": "No one assigned", + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "QuickOpen": { "1dbd3f59ff": "Move", @@ -9115,7 +9119,8 @@ "8a621a2c4f": "Grouped", "b13f85d75c": "Timeline", "f5cf324efa": "Comment display options", - "5e6e5a13fa": "View" + "5e6e5a13fa": "View", + "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "empty": { "state": { @@ -11209,7 +11214,8 @@ "b3e7f9a1c2": "Check fix context unavailable", "d5a8c2f1b9": "Start the default AI agent to fix this check", "e2b4d7c8a1": "Choose an agent for this check", - "f1c9e3a6d4": "Choose agent to fix check" + "f1c9e3a6d4": "Choose agent to fix check", + "actionRequired": "Action required" }, "CheckRunJobs": { "1c0a4d7e02": "succeeded", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 63e7e444f..e090a378e 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -710,6 +710,7 @@ "d23bbb6416": "{{value0}} omitido", "18f80e1329": "{{value0}} pendiente", "b1ac991806": "{{value0}} fallando", + "checksNeedActionChip": "{{value0}} requiere acción", "311d0cee55": "{{value0}} pasando", "e52bed9264": "Aún no se han reportado controles", "90020cc1f3": "Esta solicitud de extracción aún no tiene comprobaciones reportadas.", @@ -858,7 +859,8 @@ "to": "to", "activity": "Activity", "noActivity": "No activity yet." - } + }, + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "GitLabItemDialog": { "65e784c1f1": "Reabrir", @@ -1183,6 +1185,7 @@ "e6ad0a8d06": "{{value0}} omitido", "88267924d5": "{{value0}} pendiente", "ae2a34c7b8": "{{value0}} fallando", + "checksNeedActionChip": "{{value0}} requiere acción", "7c5035931a": "{{value0}} pasando", "a18d01cda3": "Aún no se han reportado controles", "3912daf310": "Esta solicitud de extracción aún no tiene comprobaciones reportadas.", @@ -1317,7 +1320,8 @@ "commentTooLarge": "El comentario es demasiado grande para enviarlo de forma segura.", "8ff5ae8866": "Responsables", "82c87eceb9": "Editar responsables", - "1ff5d979df": "Nadie asignado" + "1ff5d979df": "Nadie asignado", + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "QuickOpen": { "1dbd3f59ff": "Mover", @@ -9115,7 +9119,8 @@ "8a621a2c4f": "Grouped", "b13f85d75c": "Timeline", "f5cf324efa": "Comment display options", - "5e6e5a13fa": "View" + "5e6e5a13fa": "View", + "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "empty": { "state": { @@ -11209,7 +11214,8 @@ "d5a8c2f1b9": "Iniciar el agente de IA predeterminado para corregir esta comprobación", "e2b4d7c8a1": "Elige un agente para esta comprobación", "f1c9e3a6d4": "Elige un agente para corregir la comprobación", - "5e2a9c3f88": "Open file at this line" + "5e2a9c3f88": "Open file at this line", + "actionRequired": "Action required" }, "check": { "run": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 3d697e34e..9d2083c15 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -710,6 +710,7 @@ "d23bbb6416": "{{value0}} はスキップされました", "18f80e1329": "{{value0}} 保留中", "b1ac991806": "{{value0}} は失敗しました", + "checksNeedActionChip": "{{value0}} 件の対応が必要", "311d0cee55": "{{value0}} 通過", "e52bed9264": "まだチェックは報告されていません", "90020cc1f3": "この PR にはまだチェックが報告されていません。", @@ -858,7 +859,8 @@ "to": "to", "activity": "Activity", "noActivity": "No activity yet." - } + }, + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "GitLabItemDialog": { "65e784c1f1": "再度開く", @@ -1183,6 +1185,7 @@ "e6ad0a8d06": "{{value0}} はスキップされました", "88267924d5": "{{value0}} 保留中", "ae2a34c7b8": "{{value0}} は失敗しました", + "checksNeedActionChip": "{{value0}} 件の対応が必要", "7c5035931a": "{{value0}} 通過", "a18d01cda3": "まだチェックは報告されていません", "3912daf310": "この PR にはまだチェックが報告されていません。", @@ -1317,7 +1320,8 @@ "commentTooLarge": "コメントが大きすぎるため安全に送信できません。", "8ff5ae8866": "担当者", "82c87eceb9": "担当者を編集", - "1ff5d979df": "担当者なし" + "1ff5d979df": "担当者なし", + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "QuickOpen": { "1dbd3f59ff": "移動", @@ -9115,7 +9119,8 @@ "8a621a2c4f": "Grouped", "b13f85d75c": "Timeline", "f5cf324efa": "Comment display options", - "5e6e5a13fa": "View" + "5e6e5a13fa": "View", + "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "empty": { "state": { @@ -11209,7 +11214,8 @@ "d5a8c2f1b9": "既定の AI エージェントでこのチェックを修正", "e2b4d7c8a1": "このチェックのエージェントを選択", "f1c9e3a6d4": "チェックを修正するエージェントを選択", - "5e2a9c3f88": "Open file at this line" + "5e2a9c3f88": "Open file at this line", + "actionRequired": "Action required" }, "check": { "run": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index e83d9dbd1..fad4fe9f2 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -710,6 +710,7 @@ "d23bbb6416": "{{value0}} 건너뜀", "18f80e1329": "{{value0}} 보류 중", "b1ac991806": "{{value0}} 실패", + "checksNeedActionChip": "{{value0}}개 조치 필요", "311d0cee55": "{{value0}} 통과", "e52bed9264": "아직 보고된 체크가 없습니다.", "90020cc1f3": "이 PR에는 아직 보고된 체크가 없습니다.", @@ -858,7 +859,8 @@ "to": "to", "activity": "Activity", "noActivity": "No activity yet." - } + }, + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "GitLabItemDialog": { "65e784c1f1": "다시 열기", @@ -1183,6 +1185,7 @@ "e6ad0a8d06": "{{value0}} 건너뛰었습니다", "88267924d5": "{{value0}} 보류 중", "ae2a34c7b8": "{{value0}} 실패", + "checksNeedActionChip": "{{value0}}개 조치 필요", "7c5035931a": "{{value0}} 통과", "a18d01cda3": "아직 보고된 체크가 없습니다.", "3912daf310": "이 PR에는 아직 보고된 체크가 없습니다.", @@ -1317,7 +1320,8 @@ "commentTooLarge": "댓글이 너무 커서 안전하게 제출할 수 없습니다.", "8ff5ae8866": "담당자", "82c87eceb9": "담당자 편집", - "1ff5d979df": "할당된 사람이 없음" + "1ff5d979df": "할당된 사람이 없음", + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "QuickOpen": { "1dbd3f59ff": "이동", @@ -9115,7 +9119,8 @@ "8a621a2c4f": "Grouped", "b13f85d75c": "Timeline", "f5cf324efa": "Comment display options", - "5e6e5a13fa": "View" + "5e6e5a13fa": "View", + "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "empty": { "state": { @@ -11209,7 +11214,8 @@ "d5a8c2f1b9": "이 체크를 수정할 기본 AI agent 시작", "e2b4d7c8a1": "이 체크를 처리할 agent 선택", "f1c9e3a6d4": "체크를 수정할 agent 선택", - "5e2a9c3f88": "Open file at this line" + "5e2a9c3f88": "Open file at this line", + "actionRequired": "Action required" }, "check": { "run": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 3c91ead99..8f1465f85 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -710,6 +710,7 @@ "d23bbb6416": "{{value0}} 已跳过", "18f80e1329": "{{value0}} 待处理", "b1ac991806": "{{value0}} 失败", + "checksNeedActionChip": "{{value0}} 个需要操作", "311d0cee55": "{{value0}} 通过", "e52bed9264": "尚未报告任何检查", "90020cc1f3": "此PR尚未报告检查。", @@ -858,7 +859,8 @@ "to": "to", "activity": "Activity", "noActivity": "No activity yet." - } + }, + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "GitLabItemDialog": { "65e784c1f1": "重新打开", @@ -1183,6 +1185,7 @@ "e6ad0a8d06": "{{value0}} 已跳过", "88267924d5": "{{value0}} 待处理", "ae2a34c7b8": "{{value0}} 失败", + "checksNeedActionChip": "{{value0}} 个需要操作", "7c5035931a": "{{value0}} 通过", "a18d01cda3": "尚未报告任何检查", "3912daf310": "此PR尚未报告检查。", @@ -1317,7 +1320,8 @@ "commentTooLarge": "评论过长,无法安全提交。", "8ff5ae8866": "负责人", "82c87eceb9": "编辑负责人", - "1ff5d979df": "未分配任何人" + "1ff5d979df": "未分配任何人", + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "QuickOpen": { "1dbd3f59ff": "手机", @@ -9115,7 +9119,8 @@ "8a621a2c4f": "已分组", "b13f85d75c": "时间线", "f5cf324efa": "评论显示选项", - "5e6e5a13fa": "查看" + "5e6e5a13fa": "查看", + "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." }, "empty": { "state": { @@ -11209,7 +11214,8 @@ "d5a8c2f1b9": "启动默认 AI 智能体来修复此检查", "e2b4d7c8a1": "为此检查选择智能体", "f1c9e3a6d4": "选择智能体修复检查", - "5e2a9c3f88": "Open file at this line" + "5e2a9c3f88": "Open file at this line", + "actionRequired": "Action required" }, "check": { "run": { diff --git a/src/renderer/src/store/slices/github-checks.test.ts b/src/renderer/src/store/slices/github-checks.test.ts index a67a6b7c3..240521b8b 100644 --- a/src/renderer/src/store/slices/github-checks.test.ts +++ b/src/renderer/src/store/slices/github-checks.test.ts @@ -1,6 +1,21 @@ import { describe, expect, it } from 'vitest' -import { syncPRChecksStatus, normalizeBranchName } from './github-checks' +import { + deriveCheckStatusFromChecks, + syncPRChecksStatus, + normalizeBranchName +} from './github-checks' import type { AppState } from '../types' +import type { PRCheckDetail } from '../../../../shared/types' + +describe('deriveCheckStatusFromChecks', () => { + it('treats an action_required check as failure so it is not a silent pass', () => { + const checks: PRCheckDetail[] = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null }, + { name: 'approval', status: 'completed', conclusion: 'action_required', url: null } + ] + expect(deriveCheckStatusFromChecks(checks)).toBe('failure') + }) +}) describe('normalizeBranchName', () => { it('strips refs/heads/ prefix', () => { diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index 8d53226a7..13e60a2c8 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -17,7 +17,10 @@ export function deriveCheckStatusFromChecks(checks: PRCheckDetail[]): CheckStatu if ( check.conclusion === 'failure' || check.conclusion === 'timed_out' || - check.conclusion === 'cancelled' + check.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. + check.conclusion === 'action_required' ) { return 'failure' } diff --git a/src/shared/hosted-review-github.test.ts b/src/shared/hosted-review-github.test.ts index d01c9307b..640a87016 100644 --- a/src/shared/hosted-review-github.test.ts +++ b/src/shared/hosted-review-github.test.ts @@ -91,6 +91,17 @@ describe('hostedReviewSummaryFromGitHubPRInfo', () => { expect(summary.checksStatus).toBe('failure') }) + it('treats action_required checks as failed so auto-merge sees the block', () => { + const summary = hostedReviewSummaryFromGitHubPRInfo({ + pr: { ...pr, checksStatus: 'success' }, + owner: 'acme', + repo: 'orca', + checks: [{ name: 'approval', status: 'completed', conclusion: 'action_required', url: null }] + }) + + expect(summary.checksStatus).toBe('failure') + }) + it('distinguishes loaded empty comments from unknown comments', () => { expect( hostedReviewSummaryFromGitHubPRInfo({ diff --git a/src/shared/hosted-review-github.ts b/src/shared/hosted-review-github.ts index b332c1ed3..1e3760032 100644 --- a/src/shared/hosted-review-github.ts +++ b/src/shared/hosted-review-github.ts @@ -39,7 +39,10 @@ function deriveChecksStatus( (check) => check.conclusion === 'failure' || check.conclusion === 'timed_out' || - check.conclusion === 'cancelled' + check.conclusion === 'cancelled' || + // Why: action_required (e.g. a workflow awaiting approval) blocks merge; + // treat it as failure so the review queue doesn't report a clean PR. + check.conclusion === 'action_required' ) if (hasFailure) { return 'failure' diff --git a/src/shared/types.ts b/src/shared/types.ts index 789d65190..f6f0a5d72 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1231,6 +1231,10 @@ export type PRCheckDetail = { | 'neutral' | 'skipped' | 'pending' + // Why: a check suite needing manual action (e.g. a workflow awaiting "Approve + // and run") has no check run and is absent from statusCheckRollup, yet blocks + // auto-merge (GitHub returns "unstable status"). Surface it as its own state. + | 'action_required' | null url: string | null checkRunId?: number