mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
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 <help@stably.ai>
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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<string, unknown>): 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 }
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -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 {
|
||||
|
||||
@@ -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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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`
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user