From 4e6b3618d781a1a15f79917aad0595118b2fe238 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Tue, 30 Jun 2026 16:03:01 -0400 Subject: [PATCH] fix(github): align maintainer digest failing-check detection with readiness classifier The queue digest counted only three lowercase conclusion values and ignored status-carried failures, so PRs with broken checks were mis-ranked for triage. Reuse the canonical isFailingCheckSummary predicate from local-branch readiness. Co-authored-by: Cursor --- src/github/commands.ts | 6 +++++- src/signals/local-branch.ts | 16 ++++++++++++++- test/unit/github-commands.test.ts | 33 +++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/github/commands.ts b/src/github/commands.ts index e1efcbab5e..5b1d051ffb 100644 --- a/src/github/commands.ts +++ b/src/github/commands.ts @@ -17,6 +17,7 @@ import { type QueueHealth, type RepoOutcomePatterns, } from "../signals/engine"; +import { isFailingCheckSummary } from "../signals/local-branch"; import { buildMaintainerNoiseReport, type MaintainerNoiseReport } from "../signals/reward-risk"; const PUBLIC_MENTION_COMMAND_CATALOG = [ @@ -1290,7 +1291,10 @@ function summarizeQueuePullRequest( ): MaintainerQueuePullRequestSummary { const ageDays = daysSince(pr.updatedAt ?? pr.createdAt); const confirmedMiner = Boolean(pr.authorLogin && confirmedMinerLogins.has(normalizeLogin(pr.authorLogin))); - const failedChecks = checks.filter((check) => ["failure", "timed_out", "cancelled"].includes(check.conclusion ?? "")).length; + // Share the readiness path's canonical classifier so the digest counts the SAME failing checks the gate sees: + // a failure carried on `status` (commit-status rows / runs that errored before concluding), startup_failure / + // failed / action_required, and any case variant — not just three lowercase `conclusion` values. + const failedChecks = checks.filter(isFailingCheckSummary).length; const signals: MaintainerQueuePullRequestSummary["signals"] = [ ...(confirmedMiner ? ["confirmed_miner" as const] : []), ...(pr.linkedIssues.length === 0 ? ["missing_linked_issue" as const] : []), diff --git a/src/signals/local-branch.ts b/src/signals/local-branch.ts index a1837633db..b2ce2d1c7d 100644 --- a/src/signals/local-branch.ts +++ b/src/signals/local-branch.ts @@ -737,8 +737,22 @@ function matchingCheckSummaries(pr: PullRequestRecord, checkSummaries: CheckSumm ); } +/** Conclusion/status values that mark a single cached check as failing or attention-needing. */ +const FAILING_CHECK_STATES = ["failure", "failed", "timed_out", "cancelled", "action_required", "startup_failure"]; + +/** + * Canonical "is this ONE cached check failing?" predicate, shared so every surface (readiness, the maintainer + * queue digest) classifies a check identically. A cached check may carry its outcome on `conclusion` (check + * runs) OR only on `status` (commit-status rows and runs that errored before concluding), so fall back to + * `status` when `conclusion` is absent, and case-fold both — GitHub conclusions are lowercase, but cached/commit + * statuses are not guaranteed to be. + */ +export function isFailingCheckSummary(check: CheckSummaryRecord): boolean { + return FAILING_CHECK_STATES.includes((check.conclusion ?? check.status).toLowerCase()); +} + function hasFailingCheck(checks: CheckSummaryRecord[]): boolean { - return checks.some((check) => ["failure", "failed", "timed_out", "cancelled", "action_required", "startup_failure"].includes((check.conclusion ?? check.status).toLowerCase())); + return checks.some(isFailingCheckSummary); } function hasPendingCheck(checks: CheckSummaryRecord[]): boolean { diff --git a/test/unit/github-commands.test.ts b/test/unit/github-commands.test.ts index e691ea4e76..c52301b2b6 100644 --- a/test/unit/github-commands.test.ts +++ b/test/unit/github-commands.test.ts @@ -1517,6 +1517,39 @@ describe("GitHub mention commands", () => { expect(emptyNoise).not.toContain("Suggested triage:"); }); + it("counts failing cached checks with the canonical readiness classifier (status-carried, startup_failure, case-fold)", () => { + const prWithChecks = (number: number, checks: Array<{ status: string; conclusion?: string | null }>) => + buildMaintainerQueueDigest({ + repo: { fullName: "owner/repo", isRegistered: true, registryConfig: { emissionShare: 0.1, issueDiscoveryShare: 0, labelMultipliers: {}, maintainerCut: 0, raw: {}, repo: "owner/repo" } } as any, + issues: [issue(1, "Linked fix")], + pullRequests: [pr(number, "Check signal coverage", "alice", { linkedIssues: [1], updatedAt: "2099-01-01T00:00:00.000Z" })], + checkSummariesByPullNumber: { + [number]: checks.map((check, index) => ({ + id: `check-${number}-${index}`, + repoFullName: "owner/repo", + pullNumber: number, + name: `check-${index}`, + status: check.status, + conclusion: check.conclusion ?? null, + payload: {}, + })), + }, + }); + + const statusCarried = prWithChecks(20, [{ status: "failure", conclusion: null }]).needsAuthorPullRequests[0]; + expect(statusCarried?.signals).toContain("checks_need_attention"); + expect(statusCarried?.reasons).toContain("1 cached check(s) need attention."); + + const startupFailure = prWithChecks(21, [{ status: "completed", conclusion: "startup_failure" }]).needsAuthorPullRequests[0]; + expect(startupFailure?.signals).toContain("checks_need_attention"); + + const mixedCase = prWithChecks(22, [{ status: "completed", conclusion: "FAILURE" }]).needsAuthorPullRequests[0]; + expect(mixedCase?.signals).toContain("checks_need_attention"); + + const clean = prWithChecks(23, [{ status: "completed", conclusion: "success" }]).needsAuthorPullRequests[0]; + expect(clean?.signals ?? []).not.toContain("checks_need_attention"); + }); + it("builds maintainer-only queue digests with safe routing, sorting, and private-detail pointers", () => { const digest = sampleMaintainerDigest(); expect(digest.totals.confirmedMinerPullRequests).toBe(2);