From d69d2f4f7ec693e3ece25efd68cac7a0fcc82b86 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:42:31 -0700 Subject: [PATCH] fix(checks): stop the Tasks-grid pill contradicting its own verdict The checks pill's label, tone and icon all read one ProviderCheckSummary, but getChecksLabel short-circuited on the raw `neutral` counter while the tone and icon key off `state`. After the classification fix a PR with 19 success + 1 neutral renders an emerald CheckCircle2 pill that reads "1 unresolved", and mobile's own label (which keys off `state`) reads "19/20 passed" for the same summary. Move the label into src/shared/provider-check-summary.ts so desktop and mobile cannot fork it again, and key it off `state`. Also covers deriveWorkItemCheckSummary, the desktop-main producer of the summary that reaches the Tasks grid and the relay-paired mobile client. It was rewritten here with no test at all; the parity table stands in derivePRCheckStatusFromRollup, which is a different normalizer. The new main-process test drives getWorkItem with a real statusCheckRollup fixture, pinning the StatusContext `state` fallback that would otherwise be deletable with the whole suite still green. Co-authored-by: Orca --- .../src/tasks/mobile-hosted-check-status.ts | 18 +- .../client-work-item-check-summary.test.ts | 155 ++++++++++++++++++ src/renderer/src/components/TaskPage.tsx | 19 +-- ...ovider-check-classification-parity.test.ts | 14 +- src/shared/provider-check-summary.ts | 19 +++ 5 files changed, 191 insertions(+), 34 deletions(-) create mode 100644 src/main/github/client-work-item-check-summary.test.ts diff --git a/mobile/src/tasks/mobile-hosted-check-status.ts b/mobile/src/tasks/mobile-hosted-check-status.ts index 341d71bc814..c1b6076c969 100644 --- a/mobile/src/tasks/mobile-hosted-check-status.ts +++ b/mobile/src/tasks/mobile-hosted-check-status.ts @@ -1,4 +1,5 @@ import type { ProviderCheckSummary, PRMergeableState } from '../../../src/shared/types' +import { getProviderChecksLabel } from '../../../src/shared/provider-check-summary' export type MobileHostedReviewStatus = { checksSummary?: ProviderCheckSummary @@ -39,22 +40,7 @@ export function getHostedMergeLabel(item: MobileHostedReviewStatus): string { } export function getHostedChecksLabel(item: { checksSummary?: ProviderCheckSummary }): string { - const summary = item.checksSummary - if (!summary) { - return 'Checks' - } - if (summary.total === 0) { - return 'No checks' - } - if (summary.failed > 0) { - return `${summary.failed} failing` - } - if (summary.pending > 0) { - return `${summary.pending} pending` - } - return summary.state === 'neutral' - ? 'Unresolved checks' - : `${summary.passed}/${summary.total} passed` + return getProviderChecksLabel(item.checksSummary) } export function getHostedReviewSignalTone( diff --git a/src/main/github/client-work-item-check-summary.test.ts b/src/main/github/client-work-item-check-summary.test.ts new file mode 100644 index 00000000000..2543fa3a288 --- /dev/null +++ b/src/main/github/client-work-item-check-summary.test.ts @@ -0,0 +1,155 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { ghExecFileAsyncMock, getOwnerRepoMock, rateLimitGuardMock } = vi.hoisted(() => ({ + ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + rateLimitGuardMock: vi.fn(() => ({ blocked: false })) +})) + +vi.mock('./gh-utils', () => ({ + execFileAsync: vi.fn(), + ghExecFileAsync: ghExecFileAsyncMock, + githubRepoContext: (repoPath: string, connectionId?: string | null) => ({ + repoPath, + connectionId: connectionId ?? null + }), + ghRepoExecOptions: (context: { repoPath: string }) => ({ cwd: context.repoPath }), + getOwnerRepo: getOwnerRepoMock, + getIssueOwnerRepo: vi.fn(), + getOwnerRepoForRemote: ( + repoPath: string, + remoteName: string, + connectionId?: string | null, + localGitOptions?: unknown + ) => + remoteName === 'origin' + ? getOwnerRepoMock(repoPath, connectionId, localGitOptions) + : Promise.resolve(null), + resolveIssueSource: vi.fn(), + extractExecError: vi.fn((err: unknown) => ({ stderr: String(err), stdout: '' })), + acquire: vi.fn(), + release: vi.fn(), + _resetOwnerRepoCache: vi.fn(), + classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }), + classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr }) +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: vi.fn() +})) + +vi.mock('./rate-limit', () => ({ + rateLimitGuard: rateLimitGuardMock, + noteRateLimitSpend: vi.fn(), + getRateLimit: vi.fn(async () => ({ ok: false, error: 'not probed in tests' })), + repositoryRateLimitGuard: vi.fn(() => ({ blocked: false })), + noteRepositoryRateLimitSpend: vi.fn(), + spendsSharedGitHubComQuota: vi.fn(() => true) +})) + +import { getWorkItem, _resetMergeQueueCacheForTests, _resetOwnerRepoCache } from './client' +import { _resetOriginGitHubApiRepositoryCache } from './github-api-repository' + +// Why: only `pr view` is fixtured; the merge-metadata fan-out is best-effort and must not mask the summary. +function mockPullRequestDetail(payload: Record): void { + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] !== 'pr' || args[1] !== 'view') { + throw new Error('unexpected gh call') + } + return { stdout: JSON.stringify(payload) } + }) +} + +describe('work item checksSummary', () => { + beforeEach(() => { + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + rateLimitGuardMock.mockReset() + rateLimitGuardMock.mockReturnValue({ blocked: false }) + _resetOwnerRepoCache() + _resetOriginGitHubApiRepositoryCache() + _resetMergeQueueCacheForTests() + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + }) + + it('counts a skipped run as passing and reads a StatusContext state as its conclusion', async () => { + mockPullRequestDetail({ + number: 42, + title: 'Add feature', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/42', + updatedAt: '2026-04-01T00:00:00Z', + statusCheckRollup: [ + { __typename: 'CheckRun', status: 'COMPLETED', conclusion: 'SKIPPED' }, + // Why: StatusContext carries no status/conclusion — only `state`. + { __typename: 'StatusContext', state: 'SUCCESS' }, + { __typename: 'CheckRun', status: 'COMPLETED', conclusion: 'NEUTRAL' } + ] + }) + + const item = await getWorkItem('/repo-root', 42, 'pr', null, {}, 'origin') + + expect(item?.checksSummary).toEqual({ + state: 'success', + total: 3, + passed: 2, + failed: 0, + pending: 0, + neutral: 1 + }) + }) + + it('reports a still-running check as pending', async () => { + mockPullRequestDetail({ + number: 43, + title: 'Add feature', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/43', + updatedAt: '2026-04-01T00:00:00Z', + statusCheckRollup: [ + { __typename: 'CheckRun', status: 'COMPLETED', conclusion: 'SUCCESS' }, + { __typename: 'CheckRun', status: 'IN_PROGRESS', conclusion: null } + ] + }) + + const item = await getWorkItem('/repo-root', 43, 'pr', null, {}, 'origin') + + expect(item?.checksSummary).toEqual({ + state: 'pending', + total: 2, + passed: 1, + failed: 0, + pending: 1, + neutral: 0 + }) + }) + + it('fails the summary on a failing run and leaves an empty rollup with no state', async () => { + mockPullRequestDetail({ + number: 44, + title: 'Add feature', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/44', + updatedAt: '2026-04-01T00:00:00Z', + statusCheckRollup: [ + { __typename: 'CheckRun', status: 'COMPLETED', conclusion: 'SUCCESS' }, + { __typename: 'CheckRun', status: 'COMPLETED', conclusion: 'FAILURE' } + ] + }) + await expect(getWorkItem('/repo-root', 44, 'pr', null, {}, 'origin')).resolves.toMatchObject({ + checksSummary: { state: 'failure', total: 2, passed: 1, failed: 1, pending: 0, neutral: 0 } + }) + + mockPullRequestDetail({ + number: 45, + title: 'Add feature', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/45', + updatedAt: '2026-04-01T00:00:00Z', + statusCheckRollup: [] + }) + await expect(getWorkItem('/repo-root', 45, 'pr', null, {}, 'origin')).resolves.toMatchObject({ + checksSummary: { state: 'none', total: 0, passed: 0, failed: 0, pending: 0, neutral: 0 } + }) + }) +}) diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index f9dac6c9fa3..4261ea69e9a 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -209,6 +209,7 @@ import { taskPageToGitHubApiPage } from '@/components/task-page-work-item-pagination' import { sortWorkItemsByNumber } from '../../../shared/work-items' +import { getProviderChecksLabel } from '../../../shared/provider-check-summary' import LinearIssueAttributeFilterDropdowns from '@/components/linear-issue-attribute-filter-dropdowns' import { resolveLinearIssueAttributeFilterPrimaryTeam } from '@/components/linear-issue-attribute-filter-primary-team' import { @@ -2013,23 +2014,7 @@ function GHAssigneesCell({ } function getChecksLabel(item: GitHubWorkItem): string { - const summary = item.checksSummary - if (!summary) { - return 'Checks' - } - if (summary.total === 0) { - return 'No checks' - } - if (summary.failed > 0) { - return `${summary.failed} failing` - } - if (summary.pending > 0) { - return `${summary.pending} pending` - } - if (summary.neutral > 0) { - return `${summary.neutral} unresolved` - } - return `${summary.passed}/${summary.total} passed` + return getProviderChecksLabel(item.checksSummary) } function getChecksPillTone(item: GitHubWorkItem): string { diff --git a/src/renderer/src/components/provider-check-classification-parity.test.ts b/src/renderer/src/components/provider-check-classification-parity.test.ts index f43ad64bfb0..7388252979e 100644 --- a/src/renderer/src/components/provider-check-classification-parity.test.ts +++ b/src/renderer/src/components/provider-check-classification-parity.test.ts @@ -2,7 +2,10 @@ import { describe, expect, it } from 'vitest' import { deriveTaskPagePRCheckSummary } from './task-page-pr-check-summary' import { gitLabPipelineJobsToPRChecks } from '../../../shared/gitlab-pipeline-checks' import { derivePRCheckStatus, derivePRCheckStatusFromRollup } from '../../../shared/pr-check-status' -import { summarizeProviderChecks } from '../../../shared/provider-check-summary' +import { + getProviderChecksLabel, + summarizeProviderChecks +} from '../../../shared/provider-check-summary' import type { GitLabPipelineJob } from '../../../shared/gitlab-types' import type { PRCheckDetail, ProviderCheckSummary } from '../../../shared/types' @@ -110,6 +113,15 @@ describe('provider check classification parity', () => { expect(deriveTaskPagePRCheckSummary(checks)).toEqual(summary) expect(derivePRCheckStatus(checks)).toBe(expected.state) expect(derivePRCheckStatusFromRollup(checks.map(toGraphQLRollup))).toBe(expected.state) + // Why: the pill's label, tone and icon all read this one summary, so a green pill must never say "unresolved". + expect(getProviderChecksLabel(summary).includes('Unresolved')).toBe( + expected.state === 'neutral' + ) } ) + + it('labels a green PR carrying one neutral check as passing', () => { + const checks = [...Array.from({ length: 19 }, () => completed('success')), completed('neutral')] + expect(getProviderChecksLabel(summarizeProviderChecks(checks))).toBe('19/20 passed') + }) }) diff --git a/src/shared/provider-check-summary.ts b/src/shared/provider-check-summary.ts index 3c1b3eed2a1..a3fe8813458 100644 --- a/src/shared/provider-check-summary.ts +++ b/src/shared/provider-check-summary.ts @@ -80,3 +80,22 @@ export function summarizeProviderChecks( neutral } } + +/** The one checks-pill label; it keys off `state` so the text can never contradict the pill's tone or icon. */ +export function getProviderChecksLabel(summary: ProviderCheckSummary | undefined): string { + if (!summary) { + return 'Checks' + } + if (summary.total === 0) { + return 'No checks' + } + if (summary.failed > 0) { + return `${summary.failed} failing` + } + if (summary.pending > 0) { + return `${summary.pending} pending` + } + return summary.state === 'neutral' + ? 'Unresolved checks' + : `${summary.passed}/${summary.total} passed` +}