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
This commit is contained in:
Neil
2026-07-30 22:08:46 -07:00
parent a2d0e77143
commit 9744c3f08c
14 changed files with 347 additions and 215 deletions
+87 -4
View File
@@ -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<ProviderCheckSummary, 'total'>
}
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<ProviderCheckSummary, 'total'>
}[])('$name never reads as failing', ({ statuses, expected }) => {
expect(buildGitLabCheckSummary(statuses.map((status) => ({ status })))).toEqual({
...expected,
total: statuses.length
})
})
})
+7 -52
View File
@@ -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)
}
+10 -30
View File
@@ -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)
}))
)
}
+14 -38
View File
@@ -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<string, unknown>
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<string, unknown>
// Why: StatusContext reports `state` and carries no `status`; CheckRun reports both.
return {
status: String(raw.status ?? ''),
conclusion: String(raw.conclusion ?? raw.state ?? '')
}
})
)
}
function mapPullRequestWorkItem(
+11 -4
View File
@@ -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', () => {
+5 -1
View File
@@ -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' ||
@@ -50,7 +50,7 @@ describe('deriveTaskPagePRCheckSummary', () => {
check({ conclusion: 'skipped' })
])
).toEqual({
state: 'neutral',
state: 'success',
total: 3,
passed: 2,
failed: 0,
@@ -1,55 +1,6 @@
import { summarizeProviderChecks } from '../../../shared/provider-check-summary'
import type { PRCheckDetail, ProviderCheckSummary } from '../../../shared/types'
function getCheckConclusion(check: PRCheckDetail): NonNullable<PRCheckDetail['conclusion']> {
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)
}
+1 -1
View File
@@ -49,7 +49,7 @@ describe('gitLabPipelineJobsToPRChecks', () => {
{
name: 'deploy: deploy',
status: 'completed',
conclusion: 'action_required',
conclusion: 'neutral',
url: null
},
{
+6 -1
View File
@@ -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 (
+2 -1
View File
@@ -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'
])
})
+4 -31
View File
@@ -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 }
+115
View File
@@ -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<ProviderCheckSummary, 'total'>
}
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)
}
)
})
+82
View File
@@ -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, 'total' | 'passed' | 'failed' | 'pending'>
): 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
}
}