From 9744c3f08ce836bd86fc707b877138b3dda113a6 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:08:46 -0700 Subject: [PATCH] fix(checks): stop skipped and manual checks reporting as failures Route every check-classification surface through one shared helper so desktop renderer, desktop main and mobile agree on the same verdict. - GitLab `manual` jobs and pipelines are neutral again, not action_required/failure - `skipped` counts as passed everywhere, including mobile - a neutral check no longer demotes a summary that has passing checks --- mobile/src/tasks/github-check-summary.test.ts | 91 +++++++++++++- mobile/src/tasks/github-check-summary.ts | 59 ++------- mobile/src/tasks/gitlab-check-summary.ts | 40 ++---- src/main/github/client.ts | 52 +++----- src/main/gitlab/mappers.test.ts | 15 ++- src/main/gitlab/mappers.ts | 6 +- .../task-page-pr-check-summary.test.ts | 2 +- .../components/task-page-pr-check-summary.ts | 53 +------- src/shared/gitlab-pipeline-checks.test.ts | 2 +- src/shared/gitlab-pipeline-checks.ts | 7 +- src/shared/pr-check-severity-order.test.ts | 3 +- src/shared/pr-check-status.ts | 35 +----- src/shared/provider-check-summary.test.ts | 115 ++++++++++++++++++ src/shared/provider-check-summary.ts | 82 +++++++++++++ 14 files changed, 347 insertions(+), 215 deletions(-) create mode 100644 src/shared/provider-check-summary.test.ts create mode 100644 src/shared/provider-check-summary.ts diff --git a/mobile/src/tasks/github-check-summary.test.ts b/mobile/src/tasks/github-check-summary.test.ts index a173663d9a5..3eafcc7661d 100644 --- a/mobile/src/tasks/github-check-summary.test.ts +++ b/mobile/src/tasks/github-check-summary.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest' -import { buildGitHubCheckSummary } from './github-check-summary' +import { buildGitHubCheckSummary, type GitHubCheckLike } from './github-check-summary' import { buildGitLabCheckSummary } from './gitlab-check-summary' +import { summarizeProviderChecks } from '../../../src/shared/provider-check-summary' +import type { ProviderCheckSummary } from '../../../src/shared/types' describe('buildGitHubCheckSummary', () => { it('returns none for empty check lists', () => { @@ -31,14 +33,14 @@ describe('buildGitHubCheckSummary', () => { }) }) - it('keeps neutral and unknown terminal conclusions out of passed', () => { + it('keeps neutral and unknown terminal conclusions out of passed without demoting the PR', () => { expect( buildGitHubCheckSummary([ { status: 'completed', conclusion: 'success' }, { status: 'completed', conclusion: 'neutral' } ]) ).toEqual({ - state: 'neutral', + state: 'success', total: 2, passed: 1, failed: 0, @@ -49,7 +51,7 @@ describe('buildGitHubCheckSummary', () => { it('rolls up GitLab jobs with unknown terminal statuses as neutral', () => { expect(buildGitLabCheckSummary([{ status: 'success' }, { status: 'future_status' }])).toEqual({ - state: 'neutral', + state: 'success', total: 2, passed: 1, failed: 0, @@ -58,3 +60,84 @@ describe('buildGitHubCheckSummary', () => { }) }) }) + +type ParityCase = { + name: string + checks: GitHubCheckLike[] + expected: Omit +} + +const completed = (conclusion: string): GitHubCheckLike => ({ status: 'completed', conclusion }) + +const PARITY_CASES: ParityCase[] = [ + { + name: 'all success', + checks: [completed('success'), completed('success')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'success plus skipped', + checks: [completed('success'), completed('skipped')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'all skipped', + checks: [completed('skipped'), completed('skipped')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'success plus neutral', + checks: [completed('success'), completed('neutral')], + expected: { state: 'success', passed: 1, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'all neutral', + checks: [completed('neutral')], + expected: { state: 'neutral', passed: 0, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'success plus failure', + checks: [completed('success'), completed('failure')], + expected: { state: 'failure', passed: 1, failed: 1, pending: 0, neutral: 0 } + }, + { + name: 'success plus running', + checks: [completed('success'), { status: 'in_progress', conclusion: null }], + expected: { state: 'pending', passed: 1, failed: 0, pending: 1, neutral: 0 } + }, + { + name: 'genuine action_required', + checks: [completed('success'), completed('action_required')], + expected: { state: 'failure', passed: 1, failed: 1, pending: 0, neutral: 0 } + } +] + +describe('mobile / desktop check classification parity', () => { + it.each(PARITY_CASES)('$name matches the shared desktop classifier', ({ checks, expected }) => { + const summary = { ...expected, total: checks.length } + expect(buildGitHubCheckSummary(checks)).toEqual(summary) + expect(summarizeProviderChecks(checks)).toEqual(summary) + }) + + it.each([ + { + name: 'GitLab manual gate only', + statuses: ['manual'], + expected: { state: 'neutral', passed: 0, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'GitLab manual gate alongside a green pipeline', + statuses: ['manual', 'success'], + expected: { state: 'success', passed: 1, failed: 0, pending: 0, neutral: 1 } + } + ] satisfies { + name: string + statuses: string[] + expected: Omit + }[])('$name never reads as failing', ({ statuses, expected }) => { + expect(buildGitLabCheckSummary(statuses.map((status) => ({ status })))).toEqual({ + ...expected, + total: statuses.length + }) + }) +}) diff --git a/mobile/src/tasks/github-check-summary.ts b/mobile/src/tasks/github-check-summary.ts index a0006efb95d..8a69db32291 100644 --- a/mobile/src/tasks/github-check-summary.ts +++ b/mobile/src/tasks/github-check-summary.ts @@ -1,60 +1,15 @@ +import { summarizeProviderChecks } from '../../../src/shared/provider-check-summary' +import type { ProviderCheckSummary } from '../../../src/shared/types' + export type GitHubCheckLike = { status: string conclusion?: string | null } -export type GitHubCheckSummary = { - state: 'success' | 'failure' | 'pending' | 'neutral' | 'none' - total: number - passed: number - failed: number - pending: number - neutral: number -} - -function isFailedCheck(check: GitHubCheckLike): boolean { - return ( - check.conclusion === 'failure' || - check.conclusion === 'timed_out' || - check.conclusion === 'cancelled' - ) -} - -function isPendingCheck(check: GitHubCheckLike): boolean { - return ( - check.status === 'queued' || check.status === 'in_progress' || check.conclusion === 'pending' - ) -} +export type GitHubCheckSummary = ProviderCheckSummary +// Why: reuse the desktop classifier verbatim — a second copy is what let mobile call `skipped` +// unresolved while desktop called the same PR green. export function buildGitHubCheckSummary(checks: GitHubCheckLike[]): GitHubCheckSummary { - let failed = 0 - let pending = 0 - let neutral = 0 - - for (const check of checks) { - if (isFailedCheck(check) || check.conclusion === 'action_required') { - failed += 1 - } else if (isPendingCheck(check)) { - pending += 1 - } else if (check.conclusion === 'success') { - continue - } else { - neutral += 1 - } - } - - const total = checks.length - const passed = total - failed - pending - neutral - const state = - total === 0 - ? 'none' - : failed > 0 - ? 'failure' - : pending > 0 - ? 'pending' - : neutral > 0 - ? 'neutral' - : 'success' - - return { state, total, passed, failed, pending, neutral } + return summarizeProviderChecks(checks) } diff --git a/mobile/src/tasks/gitlab-check-summary.ts b/mobile/src/tasks/gitlab-check-summary.ts index d984b6e4283..b5834a1d8b9 100644 --- a/mobile/src/tasks/gitlab-check-summary.ts +++ b/mobile/src/tasks/gitlab-check-summary.ts @@ -1,37 +1,17 @@ -import { buildGitHubCheckSummary, type GitHubCheckLike } from './github-check-summary' +import { + mapGitLabPipelineJobStatusToCheckStatus, + mapGitLabPipelineJobStatusToConclusion +} from '../../../src/shared/gitlab-pipeline-checks' +import { summarizeProviderChecks } from '../../../src/shared/provider-check-summary' import type { ProviderCheckSummary } from '../../../src/shared/types' type GitLabPipelineJobLike = { status: string } export function buildGitLabCheckSummary(jobs: GitLabPipelineJobLike[]): ProviderCheckSummary { - return buildGitHubCheckSummary( - jobs.map((job): GitHubCheckLike => { - const status = job.status.toLowerCase() - const nonterminal = [ - 'created', - 'pending', - 'scheduled', - 'waiting_for_callback', - 'waiting_for_resource', - 'preparing' - ].includes(status) - return { - status: status === 'running' ? 'in_progress' : nonterminal ? 'queued' : 'completed', - conclusion: - status === 'success' - ? 'success' - : status === 'failed' - ? 'failure' - : ['canceled', 'canceling'].includes(status) - ? 'cancelled' - : status === 'skipped' - ? 'skipped' - : ['manual', 'action_required'].includes(status) - ? 'action_required' - : status === 'running' || nonterminal - ? 'pending' - : null - } - }) + return summarizeProviderChecks( + jobs.map((job) => ({ + status: mapGitLabPipelineJobStatusToCheckStatus(job.status), + conclusion: mapGitLabPipelineJobStatusToConclusion(job.status) + })) ) } diff --git a/src/main/github/client.ts b/src/main/github/client.ts index d68c77ad263..ff602bc4dbe 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -27,6 +27,7 @@ import { normalizeHostedReviewHeadRef } from '../../shared/hosted-review-refs' import { normalizeGitHubPRMergeMethodSettings } from '../../shared/github-pr-merge-methods' +import { summarizeProviderChecks } from '../../shared/provider-check-summary' import { isGitHubWorkItemsQueryTooLarge } from '../../shared/github-work-items-query-bounds' import { classifyGitHubUnavailable } from '../../shared/github-api-availability' import { parseTaskQuery, type ParsedTaskQuery } from '../../shared/task-query' @@ -656,44 +657,19 @@ function checkRollupEntries(value: unknown): unknown[] { } function deriveWorkItemCheckSummary(value: unknown): GitHubWorkItem['checksSummary'] { - const entries = checkRollupEntries(value) - if (entries.length === 0) { - return { state: 'none', total: 0, passed: 0, failed: 0, pending: 0, neutral: 0 } - } - let passed = 0 - let failed = 0 - let pending = 0 - let neutral = 0 - for (const entry of entries) { - if (typeof entry !== 'object' || entry === null) { - pending += 1 - continue - } - const raw = entry as Record - const conclusion = String(raw.conclusion ?? raw.state ?? '').toUpperCase() - const status = String(raw.status ?? '').toUpperCase() - if (['SUCCESS', 'SKIPPED'].includes(conclusion)) { - passed += 1 - } else if ( - ['FAILURE', 'ERROR', 'TIMED_OUT', 'CANCELLED', 'ACTION_REQUIRED', 'STARTUP_FAILURE'].includes( - conclusion - ) - ) { - failed += 1 - } else if (status === 'COMPLETED') { - neutral += 1 - } else { - pending += 1 - } - } - return { - state: failed > 0 ? 'failure' : pending > 0 ? 'pending' : neutral > 0 ? 'neutral' : 'success', - total: entries.length, - passed, - failed, - pending, - neutral - } + return summarizeProviderChecks( + checkRollupEntries(value).map((entry) => { + if (typeof entry !== 'object' || entry === null) { + return { status: '', conclusion: '' } + } + const raw = entry as Record + // Why: StatusContext reports `state` and carries no `status`; CheckRun reports both. + return { + status: String(raw.status ?? ''), + conclusion: String(raw.conclusion ?? raw.state ?? '') + } + }) + ) } function mapPullRequestWorkItem( diff --git a/src/main/gitlab/mappers.test.ts b/src/main/gitlab/mappers.test.ts index d96da3eb9be..8b6e5def2f7 100644 --- a/src/main/gitlab/mappers.test.ts +++ b/src/main/gitlab/mappers.test.ts @@ -38,8 +38,8 @@ describe('mapPipelineJobStatusToConclusion', () => { expect(mapPipelineJobStatusToConclusion('skipped')).toBe('skipped') }) - it('maps manual and action-required jobs to an actionable conclusion', () => { - expect(mapPipelineJobStatusToConclusion('manual')).toBe('action_required') + it('keeps manual gates neutral while action-required stays actionable', () => { + expect(mapPipelineJobStatusToConclusion('manual')).toBe('neutral') expect(mapPipelineJobStatusToConclusion('action_required')).toBe('action_required') }) @@ -228,7 +228,8 @@ describe('derivePipelineStatus', () => { expect(derivePipelineStatus('success')).toBe('success') expect(derivePipelineStatus('failed')).toBe('failure') expect(derivePipelineStatus('running')).toBe('pending') - expect(derivePipelineStatus('manual')).toBe('failure') + // Why: a blocked pipeline is waiting on a human trigger, not broken. + expect(derivePipelineStatus('manual')).toBe('neutral') }) it('rolls up an array of jobs', () => { @@ -239,7 +240,13 @@ describe('derivePipelineStatus', () => { it('failure beats pending in the rollup', () => { expect(derivePipelineStatus([{ status: 'failed' }, { status: 'running' }])).toBe('failure') - expect(derivePipelineStatus([{ status: 'manual' }, { status: 'success' }])).toBe('failure') + expect(derivePipelineStatus([{ status: 'action_required' }, { status: 'success' }])).toBe( + 'failure' + ) + }) + + it('keeps a manual deploy gate from failing an otherwise green pipeline', () => { + expect(derivePipelineStatus([{ status: 'manual' }, { status: 'success' }])).toBe('success') }) it('handles a single object with status', () => { diff --git a/src/main/gitlab/mappers.ts b/src/main/gitlab/mappers.ts index 7b45c9b64f0..9d0f747bdc7 100644 --- a/src/main/gitlab/mappers.ts +++ b/src/main/gitlab/mappers.ts @@ -289,9 +289,13 @@ function classifyPipelineString(status: string): CheckStatus { if (s === 'success') { return 'success' } - if (s === 'failed' || s === 'manual' || s === 'action_required') { + if (s === 'failed' || s === 'action_required') { return 'failure' } + // Why: a `manual` pipeline is blocked on a human trigger, not broken — never paint the card red. + if (s === 'manual') { + return 'neutral' + } if ( s === 'created' || s === 'pending' || 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 2258d89fdbb..ccd63d94c04 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 @@ -50,7 +50,7 @@ describe('deriveTaskPagePRCheckSummary', () => { check({ conclusion: 'skipped' }) ]) ).toEqual({ - state: 'neutral', + state: 'success', total: 3, passed: 2, failed: 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 b0bc88091ff..c7531040923 100644 --- a/src/renderer/src/components/task-page-pr-check-summary.ts +++ b/src/renderer/src/components/task-page-pr-check-summary.ts @@ -1,55 +1,6 @@ +import { summarizeProviderChecks } from '../../../shared/provider-check-summary' import type { PRCheckDetail, ProviderCheckSummary } from '../../../shared/types' -function getCheckConclusion(check: PRCheckDetail): NonNullable { - return check.conclusion ?? 'pending' -} - -function isPendingCheck(check: PRCheckDetail): boolean { - return ( - check.status === 'queued' || - check.status === 'in_progress' || - getCheckConclusion(check) === 'pending' - ) -} - export function deriveTaskPagePRCheckSummary(checks: PRCheckDetail[]): ProviderCheckSummary { - if (checks.length === 0) { - return { state: 'none', total: 0, passed: 0, failed: 0, pending: 0, neutral: 0 } - } - - let passed = 0 - let failed = 0 - let pending = 0 - let neutral = 0 - - for (const check of checks) { - const conclusion = getCheckConclusion(check) - if (conclusion === 'success' || conclusion === 'skipped') { - passed += 1 - } else if (conclusion === 'neutral') { - neutral += 1 - } else if ( - conclusion === 'failure' || - conclusion === 'timed_out' || - 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)) { - pending += 1 - } else { - neutral += 1 - } - } - - return { - state: failed > 0 ? 'failure' : pending > 0 ? 'pending' : neutral > 0 ? 'neutral' : 'success', - total: checks.length, - passed, - failed, - pending, - neutral - } + return summarizeProviderChecks(checks) } diff --git a/src/shared/gitlab-pipeline-checks.test.ts b/src/shared/gitlab-pipeline-checks.test.ts index 48faafc38e0..673fcf3cd16 100644 --- a/src/shared/gitlab-pipeline-checks.test.ts +++ b/src/shared/gitlab-pipeline-checks.test.ts @@ -49,7 +49,7 @@ describe('gitLabPipelineJobsToPRChecks', () => { { name: 'deploy: deploy', status: 'completed', - conclusion: 'action_required', + conclusion: 'neutral', url: null }, { diff --git a/src/shared/gitlab-pipeline-checks.ts b/src/shared/gitlab-pipeline-checks.ts index a988967fa32..f993be157a3 100644 --- a/src/shared/gitlab-pipeline-checks.ts +++ b/src/shared/gitlab-pipeline-checks.ts @@ -35,7 +35,12 @@ export function mapGitLabPipelineJobStatusToConclusion( if (s === 'skipped') { return 'skipped' } - if (s === 'manual' || s === 'action_required') { + // Why: manual GitLab jobs are intentionally waiting for a human trigger; calling them pending + // would make the Checks tab look stuck forever, and action_required flags green MRs as failing. + if (s === 'manual') { + return 'neutral' + } + if (s === 'action_required') { return 'action_required' } if ( diff --git a/src/shared/pr-check-severity-order.test.ts b/src/shared/pr-check-severity-order.test.ts index 126ab012723..685886513aa 100644 --- a/src/shared/pr-check-severity-order.test.ts +++ b/src/shared/pr-check-severity-order.test.ts @@ -116,10 +116,11 @@ describe('PR check severity order', () => { check('pass-b', 'success') ] + // A manual GitLab gate is neutral, so it sinks below the passing checks a reviewer reads first. expect(sortChecksBySeverity(checks).map((item) => item.name)).toEqual([ - 'manual', 'pass-a', 'pass-b', + 'manual', 'future' ]) }) diff --git a/src/shared/pr-check-status.ts b/src/shared/pr-check-status.ts index bf7dd5f085c..4d47d08d22d 100644 --- a/src/shared/pr-check-status.ts +++ b/src/shared/pr-check-status.ts @@ -1,38 +1,11 @@ +import { summarizeProviderChecks } from './provider-check-summary' import type { CheckStatus, PRCheckDetail } from './types' /** Derives the review status from the normalized check contract. */ export function derivePRCheckStatus(checks: readonly PRCheckDetail[]): CheckStatus { - if (checks.length === 0) { - return 'neutral' - } - - let hasPending = false - let hasSuccess = false - for (const check of checks) { - if ( - check.conclusion === 'failure' || - check.conclusion === 'timed_out' || - check.conclusion === 'cancelled' || - check.conclusion === 'action_required' - ) { - return 'failure' - } - if ( - check.status === 'queued' || - check.status === 'in_progress' || - check.conclusion === 'pending' - ) { - hasPending = true - } - if (check.conclusion === 'success') { - hasSuccess = true - } - } - - if (hasPending) { - return 'pending' - } - return hasSuccess ? 'success' : 'neutral' + const { state } = summarizeProviderChecks(checks) + // Why: CheckStatus has no 'none'; an empty rollup carries the same "nothing to report" meaning. + return state === 'none' ? 'neutral' : state } type RawCheckRollup = { status?: unknown; conclusion?: unknown; state?: unknown } diff --git a/src/shared/provider-check-summary.test.ts b/src/shared/provider-check-summary.test.ts new file mode 100644 index 00000000000..8e2d9848e74 --- /dev/null +++ b/src/shared/provider-check-summary.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from 'vitest' +import { deriveTaskPagePRCheckSummary } from '../renderer/src/components/task-page-pr-check-summary' +import { gitLabPipelineJobsToPRChecks } from './gitlab-pipeline-checks' +import { derivePRCheckStatus, derivePRCheckStatusFromRollup } from './pr-check-status' +import { summarizeProviderChecks } from './provider-check-summary' +import type { GitLabPipelineJob } from './gitlab-types' +import type { PRCheckDetail, ProviderCheckSummary } from './types' + +function completed(conclusion: string): PRCheckDetail { + return { + name: conclusion, + status: 'completed', + conclusion: conclusion as PRCheckDetail['conclusion'], + url: null + } +} + +function gitLabJobs(...statuses: string[]): PRCheckDetail[] { + return gitLabPipelineJobsToPRChecks( + statuses.map( + (status, index): GitLabPipelineJob => ({ + id: index, + name: status, + stage: 'deploy', + status, + webUrl: '', + duration: null + }) + ) + ) +} + +// Why: GraphQL rollups arrive upper-cased and status-first; the main process must land on the +// same verdict as the renderer for the same checks. +function toGraphQLRollup(check: PRCheckDetail): { status: string; conclusion: string | null } { + return { + status: check.status.toUpperCase(), + conclusion: check.conclusion ? check.conclusion.toUpperCase() : null + } +} + +type ParityCase = { + name: string + checks: PRCheckDetail[] + expected: Omit +} + +const PARITY_CASES: ParityCase[] = [ + { + name: 'all success', + checks: [completed('success'), completed('success')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'success plus skipped', + checks: [completed('success'), completed('skipped')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'all skipped', + checks: [completed('skipped'), completed('skipped')], + expected: { state: 'success', passed: 2, failed: 0, pending: 0, neutral: 0 } + }, + { + name: 'success plus neutral', + checks: [completed('success'), completed('neutral')], + expected: { state: 'success', passed: 1, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'all neutral', + checks: [completed('neutral')], + expected: { state: 'neutral', passed: 0, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'success plus failure', + checks: [completed('success'), completed('failure')], + expected: { state: 'failure', passed: 1, failed: 1, pending: 0, neutral: 0 } + }, + { + name: 'success plus running', + checks: [ + completed('success'), + { name: 'ci', status: 'in_progress', conclusion: null, url: null } + ], + expected: { state: 'pending', passed: 1, failed: 0, pending: 1, neutral: 0 } + }, + { + name: 'GitLab manual gate only', + checks: gitLabJobs('manual'), + expected: { state: 'neutral', passed: 0, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'GitLab manual gate alongside a green pipeline', + checks: gitLabJobs('manual', 'success'), + expected: { state: 'success', passed: 1, failed: 0, pending: 0, neutral: 1 } + }, + { + name: 'genuine action_required', + checks: [completed('success'), completed('action_required')], + expected: { state: 'failure', passed: 1, failed: 1, pending: 0, neutral: 0 } + } +] + +describe('provider check classification parity', () => { + it.each(PARITY_CASES)( + '$name resolves identically on every desktop surface', + ({ checks, expected }) => { + const summary = { ...expected, total: checks.length } + expect(summarizeProviderChecks(checks)).toEqual(summary) + expect(deriveTaskPagePRCheckSummary(checks)).toEqual(summary) + expect(derivePRCheckStatus(checks)).toBe(expected.state) + expect(derivePRCheckStatusFromRollup(checks.map(toGraphQLRollup))).toBe(expected.state) + } + ) +}) diff --git a/src/shared/provider-check-summary.ts b/src/shared/provider-check-summary.ts new file mode 100644 index 00000000000..3c1b3eed2a1 --- /dev/null +++ b/src/shared/provider-check-summary.ts @@ -0,0 +1,82 @@ +import type { ProviderCheckSummary } from './types' + +export type CheckOutcome = 'passed' | 'failed' | 'pending' | 'neutral' + +export type CheckOutcomeInput = { status?: string | null; conclusion?: string | null } + +// Why: a skipped job is a deliberate "not applicable", not an unresolved signal — every surface +// must count it as passing or the same PR reads green on desktop and grey on mobile. +const PASSED_CONCLUSIONS = new Set(['success', 'skipped']) + +// Why: these block the merge. GitLab `manual` is deliberately absent — it waits on a human. +const FAILED_CONCLUSIONS = new Set([ + 'failure', + 'error', + 'startup_failure', + 'timed_out', + 'cancelled', + 'action_required' +]) + +/** The single provider-neutral verdict for one check; every check surface must route through it. */ +export function classifyCheckOutcome(check: CheckOutcomeInput): CheckOutcome { + const conclusion = (check.conclusion ?? '').toLowerCase() + const status = (check.status ?? '').toLowerCase() + if (FAILED_CONCLUSIONS.has(conclusion)) { + return 'failed' + } + if (PASSED_CONCLUSIONS.has(conclusion)) { + return 'passed' + } + // Why: anything that has not reached a terminal status is still running, whatever it calls itself. + if (conclusion === 'pending' || status !== 'completed') { + return 'pending' + } + return 'neutral' +} + +/** Rolls up counted outcomes; passing checks win over neutral ones so one neutral cannot demote a green PR. */ +export function resolveProviderCheckState( + counts: Pick +): ProviderCheckSummary['state'] { + if (counts.total === 0) { + return 'none' + } + if (counts.failed > 0) { + return 'failure' + } + if (counts.pending > 0) { + return 'pending' + } + return counts.passed > 0 ? 'success' : 'neutral' +} + +export function summarizeProviderChecks( + checks: readonly CheckOutcomeInput[] +): ProviderCheckSummary { + let passed = 0 + let failed = 0 + let pending = 0 + let neutral = 0 + for (const check of checks) { + const outcome = classifyCheckOutcome(check) + if (outcome === 'passed') { + passed += 1 + } else if (outcome === 'failed') { + failed += 1 + } else if (outcome === 'pending') { + pending += 1 + } else { + neutral += 1 + } + } + const total = checks.length + return { + state: resolveProviderCheckState({ total, passed, failed, pending }), + total, + passed, + failed, + pending, + neutral + } +}