diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7e4a7ce5b9..23d598e85a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -147,7 +147,7 @@ import { unionScopedOverlapClusters, type ContributorProfile, } from "../signals/engine"; -import { buildClosedUnifiedCommentBody, buildUnifiedCommentBody, isUnifiedReviewCommentEnabled } from "../review/unified-comment-bridge"; +import { buildClosedUnifiedCommentBody, buildMergeReadinessFromChecks, buildUnifiedCommentBody, isUnifiedReviewCommentEnabled } from "../review/unified-comment-bridge"; import type { MergeReadiness } from "../review/unified-comment"; import { buildIssueSlopAssessment, buildSlopAssessment, type SlopBand } from "../signals/slop"; import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; @@ -1809,22 +1809,16 @@ async function maybePublishPrPublicSurface( if (unifiedCommentAllowed && gateEvaluation) { const { rows, readinessTotal } = buildPublicPrPanelSignalRows({ repo, pr, profile, detection, queueHealth, collisions, preflight, settings, gate: gateEvaluation }); const unifiedFiles = await listPullRequestFiles(env, repoFullName, pr.number); - // CI + merge-state readiness — a converged enrichment the legacy panel never showed. Maps each cached - // check's conclusion to passed/failed/unverified; any failure (failure/timed_out/cancelled/action_required) - // flips the whole PR to 'failed'. The gate decision stays authoritative for the comment's color (always - // passed here), so these CI chips never spuriously flip the unified status to held/blocked. + // CI + merge-state readiness — a converged enrichment the legacy panel never showed. Only current-head + // check summaries can affect the chip, and pending/in-progress current-head checks remain unverified. + // The gate decision stays authoritative for the comment's color (always passed here), so these CI chips + // never spuriously flip the unified status to held/blocked. const checkSummaries = await listCheckSummaries(env, repoFullName, pr.number); - const failedChecks = checkSummaries.filter((check) => { - const conclusion = (check.conclusion ?? "").toLowerCase(); - return conclusion === "failure" || conclusion === "timed_out" || conclusion === "cancelled" || conclusion === "action_required"; + const mergeReadiness: MergeReadiness = buildMergeReadinessFromChecks({ + checks: checkSummaries, + headSha: pr.headSha, + mergeableState: pr.mergeableState, }); - const anyPassed = checkSummaries.some((check) => (check.conclusion ?? "").toLowerCase() === "success"); - const ciState: MergeReadiness["ciState"] = failedChecks.length > 0 ? "failed" : anyPassed ? "passed" : "unverified"; - const mergeReadiness: MergeReadiness = { - ciState, - ...(pr.mergeableState ? { mergeStateLabel: pr.mergeableState } : {}), - ...(failedChecks.length > 0 ? { failingChecks: failedChecks.map((check) => check.name) } : {}), - }; deterministicBody = buildUnifiedCommentBody({ gate: gateEvaluation, ...(aiReview !== undefined ? { aiReview } : {}), diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index a59064378c..3fc5126b1b 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -18,7 +18,7 @@ // comment path historically did not. This module therefore scrubs Nits itself (see `publicSafeNit` / // `PRIVATE_FORBIDDEN_TERMS`) as defense-in-depth before they reach a public comment. -import type { AdvisoryFinding } from "../types"; +import type { AdvisoryFinding, CheckSummaryRecord } from "../types"; import type { GateCheckConclusion, GateCheckEvaluation } from "../rules/advisory"; import type { PublicPrPanelSignalRow } from "../signals/engine"; // Single-source the panel marker from its canonical home (the upsert reads it there); re-export so existing @@ -56,6 +56,37 @@ export { PR_PANEL_COMMENT_MARKER }; // Mirrors src/rules/advisory.ts CHECK_RUN_FORBIDDEN_TERMS (scrubbed → "[context]") and // src/signals/engine.ts containsPrivatePublicTerm (drop if still present). Kept inline so this module // stays a pure, dependency-light renderer-mapping seam. + +const FAILED_CHECK_CONCLUSIONS = new Set(["failure", "timed_out", "cancelled", "action_required"]); + +function isCompletedSuccessfulCheck(check: CheckSummaryRecord): boolean { + return check.status.toLowerCase() === "completed" && (check.conclusion ?? "").toLowerCase() === "success"; +} + +/** + * Build public merge-readiness from cached check summaries without allowing stale or partial CI to look green. + * Only check summaries for the current PR head SHA are considered, and CI is green only when every current-head + * check is completed successfully. Pending/in-progress checks, missing checks, or an unknown head SHA are + * deliberately rendered as unverified. + */ +export function buildMergeReadinessFromChecks(input: { + checks: readonly CheckSummaryRecord[]; + headSha?: string | null | undefined; + mergeableState?: string | null | undefined; +}): MergeReadiness { + const currentHead = input.headSha?.trim(); + const currentChecks = currentHead ? input.checks.filter((check) => check.headSha === currentHead) : []; + const failedChecks = currentChecks.filter((check) => FAILED_CHECK_CONCLUSIONS.has((check.conclusion ?? "").toLowerCase())); + const ciState: MergeReadiness["ciState"] = + failedChecks.length > 0 ? "failed" : currentChecks.length > 0 && currentChecks.every(isCompletedSuccessfulCheck) ? "passed" : "unverified"; + + return { + ciState, + ...(input.mergeableState ? { mergeStateLabel: input.mergeableState } : {}), + ...(failedChecks.length > 0 ? { failingChecks: failedChecks.map((check) => check.name) } : {}), + }; +} + const PRIVATE_FORBIDDEN_TERMS = /\b(?:rewards?|payouts?|farming|estimated\s+scores?|raw\s+trust\s+scores?|trust\s+scores?|score\s+estimates?|reward\s+estimates?|wallets?|hotkeys?|coldkeys?|reviewability|scoreability|private\s+signals?|likely_duplicate|reviewability\s*\d)\b/gi; const PRIVATE_DROP_TERMS = /\b(?:reward|payout|farming|wallet|hotkey|trust score|raw trust|estimated score|scoreability|likely_duplicate|reviewability\s*\d)\b/i; diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 414decace5..9e306cca8e 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { buildClosedUnifiedCommentBody, buildDualReviewNotes, + buildMergeReadinessFromChecks, buildUnifiedCommentBody, consensusDefectFromFindings, gateConclusionToVerdict, @@ -13,7 +14,7 @@ import { import { PR_PANEL_COMMENT_MARKER as MARKER_FROM_COMMENTS } from "../../src/github/comments"; import { deriveUnifiedStatus, type MergeReadiness, type UnifiedCollapsible, type UnifiedCommentStatus } from "../../src/review/unified-comment"; import type { GateCheckEvaluation } from "../../src/rules/advisory"; -import type { AdvisoryFinding } from "../../src/types"; +import type { AdvisoryFinding, CheckSummaryRecord } from "../../src/types"; import type { PublicPrPanelSignalRow } from "../../src/signals/engine"; function gate(over: Partial = {}): GateCheckEvaluation { @@ -42,6 +43,48 @@ const panelRows: PublicPrPanelSignalRow[] = [ const footer = "💰 **Earn for open-source contributions like this.** Checked by Gittensory."; +describe("buildMergeReadinessFromChecks", () => { + const check = (over: Partial = {}): CheckSummaryRecord => ({ + id: over.id ?? `${over.headSha ?? "head"}-${over.name ?? "test"}`, + repoFullName: "JSONbored/gittensory", + pullNumber: 42, + headSha: "current-head", + name: "test", + status: "completed", + conclusion: "success", + payload: {}, + ...over, + }); + + it("does not report stale or partial cached CI as green", () => { + expect( + buildMergeReadinessFromChecks({ + headSha: "current-head", + checks: [check({ headSha: "old-head", conclusion: "success" }), check({ name: "build", status: "in_progress", conclusion: null })], + }), + ).toEqual({ ciState: "unverified" }); + }); + + it("requires every current-head check to be completed successfully before showing CI green", () => { + expect( + buildMergeReadinessFromChecks({ + headSha: "current-head", + mergeableState: "clean", + checks: [check({ name: "test" }), check({ name: "lint" })], + }), + ).toEqual({ ciState: "passed", mergeStateLabel: "clean" }); + }); + + it("reports current-head failures with failing check names", () => { + expect( + buildMergeReadinessFromChecks({ + headSha: "current-head", + checks: [check({ name: "test", conclusion: "failure" }), check({ name: "lint" })], + }), + ).toEqual({ ciState: "failed", failingChecks: ["test"] }); + }); +}); + describe("gateConclusionToVerdict", () => { it("maps every gate conclusion to its authoritative verdict", () => { expect(gateConclusionToVerdict("success")).toBe("merge");