From d38ee70ab4edbd1a8b9021e3b402750a62236dff Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:35:07 -0400 Subject: [PATCH 1/3] fix(prs): stop rendering "CI passed" when nothing verified the commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADE rendered "CI passed · 3 jobs" for PR #988, where zero CI jobs ran. Three third-party apps reported `success` — CodeRabbit while rate-limited, Vercel while cancelled by an ignored-build step, and a comment bot — and GitHub Actions registered no check suite at all. The rollup asked "did anything succeed?" rather than "was this code verified?", so absence rendered as success. That is the failure mode a CI indicator exists to prevent: strictly worse than showing nothing, because it asserts a fact that is false. The derivation moves to shared/prChecksRollup.ts, with three rules: 1. State mapping delegates to prPipelineState, so `skipped`/`neutral` can no longer masquerade as success and the rollup can no longer disagree with the per-job rows rendered beneath it. 2. A green requires a CI producer — GitHub Actions or a legacy commit status (Buildkite/CircleCI/Jenkins). Preview, review and comment apps still render as rows but cannot carry a green on their own. 3. Required contexts that never reported hold the rollup back. Sourced in three tiers because credentials differ in what GitHub will show them: `/rules/branches/{branch}` (rulesets, read access only, works for all five credential sources), classic branch protection (admin-only), and `mergeStateStatus === blocked` as corroboration. Unreadable means unknown, never "none required". Both ingestion paths inherit this: ingestGithubWebhook uses its payload only to resolve which PR changed, then re-derives through computeStatus. New `not_run` state, distinct from `none`: `none` is a repo with no CI and stays quiet, `not_run` means something was expected and nothing verified the commit. It renders as a hollow dashed ring — an empty slot, not an alarm — on desktop, iOS, the Work-chat card, Lanes, the graph, and the TUI. The sweep found four more places `not_run` would have gone green: * adeRpcServer summarizePrChecks initialised `overall = "passing"`, so zero checks reported passing. The same bug, in the CLI's own rollup. * ChatPrPane returned an emerald "3/3 checks" from row counts alone. * ChatGitToolbar bucketed `skipped` into the passed counter. * getPrEdgeColor painted a not_run PR green whenever it had an approval. Display-only by design. No merge button, action or automation is gated — including iOS's merge gate, where only the "All checks green" sentence changes and the tone that feeds canMerge is deliberately left alone. Regression tests at the prChecksRollup layer so every surface inherits them, using the real #988 payload as a fixture. Fixes ADE-135 Co-Authored-By: Claude Opus 5 --- apps/ade-cli/src/adeRpcServer.ts | 8 +- .../__tests__/rightPaneFormatters.test.ts | 28 ++ .../src/tuiClient/rightPaneFormatters.ts | 33 ++- .../export-browser-mock-ade-snapshot.mjs | 8 + .../src/main/services/prs/prChatCards.test.ts | 93 ++++++- .../src/main/services/prs/prChatCards.ts | 180 +++++++++---- .../src/main/services/prs/prService.test.ts | 5 +- .../src/main/services/prs/prService.ts | 193 ++++++++++---- .../main/services/prs/requiredChecks.test.ts | 172 ++++++++++++ .../src/main/services/prs/requiredChecks.ts | 206 +++++++++++++++ apps/desktop/src/main/services/state/kvDb.ts | 6 + apps/desktop/src/renderer/browserMock.ts | 22 +- .../src/renderer/components/app/AppShell.tsx | 2 +- .../app/prToastPresentation.test.ts | 15 ++ .../components/app/prToastPresentation.ts | 27 +- .../components/chat/ChatGitToolbar.tsx | 26 +- .../renderer/components/chat/ChatPrPane.tsx | 38 ++- .../components/graph/WorkspaceGraphPage.tsx | 11 +- .../renderer/components/graph/graphHelpers.ts | 11 + .../components/graph/graphNodes/LaneNode.tsx | 4 + .../renderer/components/graph/graphPrData.ts | 4 + .../renderer/components/graph/graphTypes.ts | 2 + .../components/lanes/LanePrBadgePopover.tsx | 18 +- .../components/lanes/lanePageModel.ts | 4 + .../components/prs/shared/GitHubTabPrRow.tsx | 30 ++- .../prs/shared/PrChecksCard.test.tsx | 24 ++ .../components/prs/shared/PrChecksCard.tsx | 37 ++- .../prs/shared/PrDetailRightMetadataRail.tsx | 1 + .../components/prs/shared/prVisuals.test.ts | 17 +- .../components/prs/shared/prVisuals.tsx | 8 + .../prs/tabs/GitHubTabRowsAndMapping.test.tsx | 29 ++ .../components/terminals/useLanePrs.ts | 3 + .../src/shared/__fixtures__/pr988CheckRuns.ts | 37 +++ .../desktop/src/shared/prChecksRollup.test.ts | 237 +++++++++++++++++ apps/desktop/src/shared/prChecksRollup.ts | 248 ++++++++++++++++++ apps/desktop/src/shared/types/prs.ts | 27 +- apps/ios/ADE/Models/RemoteModels.swift | 24 +- apps/ios/ADE/Services/Database.swift | 50 +++- .../ios/ADE/Views/PRs/PrDetailChecksTab.swift | 163 +++++++++++- .../Views/PRs/PrDetailHeaderComponents.swift | 14 +- .../Views/PRs/PrDetailOverviewPreviews.swift | 88 +++++++ apps/ios/ADE/Views/PRs/PrDetailScreen.swift | 8 +- apps/ios/ADE/Views/PRs/PrHelpers.swift | 5 + apps/ios/ADE/Views/PRs/PrMergeGateCard.swift | 13 +- apps/ios/ADE/Views/PRs/PrRowCard.swift | 64 ++++- .../ios/ADE/Views/PRs/PrRowCardPreviews.swift | 34 +++ .../ADE/Views/PRs/PrsRootScreenPreviews.swift | 29 ++ apps/ios/ADE/Views/Work/WorkChatPrViews.swift | 70 ++++- 48 files changed, 2205 insertions(+), 171 deletions(-) create mode 100644 apps/desktop/src/main/services/prs/requiredChecks.test.ts create mode 100644 apps/desktop/src/main/services/prs/requiredChecks.ts create mode 100644 apps/desktop/src/shared/__fixtures__/pr988CheckRuns.ts create mode 100644 apps/desktop/src/shared/prChecksRollup.test.ts create mode 100644 apps/desktop/src/shared/prChecksRollup.ts diff --git a/apps/ade-cli/src/adeRpcServer.ts b/apps/ade-cli/src/adeRpcServer.ts index 948835994..929ea7002 100644 --- a/apps/ade-cli/src/adeRpcServer.ts +++ b/apps/ade-cli/src/adeRpcServer.ts @@ -1860,12 +1860,16 @@ function requirePrService(runtime: AdeRuntime): NonNullable check.conclusion === "success").length; const failing = checks.filter((check) => check.conclusion === "failure").length; const pending = checks.filter((check) => check.status !== "completed").length; - let overall: "failing" | "pending" | "passing" = "passing"; + // ADE-135: this started at "passing" and only moved off it for a failure or + // an in-flight run, so zero checks — or a suite that was entirely skipped — + // reported green. Nothing succeeding means nothing was verified, which is + // "not_run", not a pass. + let overall: "failing" | "pending" | "passing" | "not_run" = passing > 0 ? "passing" : "not_run"; if (failing > 0) overall = "failing"; else if (pending > 0) overall = "pending"; diff --git a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts index ce9711e20..9df8ec8e3 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts +++ b/apps/ade-cli/src/tuiClient/__tests__/rightPaneFormatters.test.ts @@ -206,6 +206,31 @@ describe("rightPaneFormatters", () => { expect(body).toContain("WAIT lint"); }); + // ADE-135: three third-party successes and no CI producer used to summarize + // as "3 passing". The rollup's verdict outranks the row count. + it("renders a not-run rollup honestly even when every row is green", () => { + const body = formatPrChecks({ + checksStatus: "not_run", + checksReason: "3 checks reported, none from a CI provider. CI has not run on this commit.", + checks: [ + { name: "CodeRabbit", status: "completed", conclusion: "success" }, + { name: "Vercel — Preview", status: "completed", conclusion: "success" }, + { name: "changeset-bot", status: "completed", conclusion: "success" }, + ], + }); + + expect(body).toContain("CI: not run"); + expect(body).not.toContain("3 passing"); + }); + + it("reports a fully skipped suite as not run", () => { + const body = formatPrChecks([ + { name: "ci / unit", status: "completed", conclusion: "skipped" }, + ]); + + expect(body).toContain("CI: not run"); + }); + it("summarizes PR review comments and threads", () => { const body = formatPrComments({ summary: { checksStatus: "passing", actionableComments: 2 }, @@ -222,6 +247,9 @@ describe("rightPaneFormatters", () => { }); expect(body).toContain("PR comments · passing · 2 actionable"); + expect( + formatPrComments({ summary: { checksStatus: "not_run", actionableComments: 0 } }), + ).toContain("CI: not run"); expect(body).toContain("open src/index.ts:12"); expect(body).toContain("reviewer: Please handle the loading state."); expect(body).not.toContain("\"reviewThreads\""); diff --git a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts index 1e7d48620..e2750b9e0 100644 --- a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts +++ b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts @@ -112,6 +112,16 @@ function statusWord(status: unknown, conclusion?: unknown): "OK" | "FAIL" | "WAI return raw.toUpperCase(); } +/** + * Human rendering of a `PrChecksStatus` in a header line. Only `not_run` is + * rewritten: it is the one value a reader would otherwise see as a raw enum, + * and "not run" is the whole point — nothing verified the commit (ADE-135). + */ +function checksStatusWord(status: string | null): string | null { + if (!status) return null; + return status === "not_run" ? "CI: not run" : status; +} + function formatCount(noun: string, count: number): string { return `${count} ${noun}${count === 1 ? "" : "s"}`; } @@ -328,8 +338,15 @@ export function formatPrMergeState(value: unknown): string { } export function formatPrChecks(value: unknown): string { + const root = unwrapStructured(value); + const rollup = isRecord(root) ? pickString(root, ["checksStatus"]) : null; + const reason = isRecord(root) ? pickString(root, ["checksReason"]) : null; const checks = firstRecordArray(value, ["checks", "items", "results"]); - if (!checks.length) return "No PR checks."; + if (!checks.length) { + return rollup === "not_run" + ? `CI: not run — ${reason ?? "No CI has run on this commit."}` + : "No PR checks."; + } let ok = 0; let fail = 0; let wait = 0; @@ -339,9 +356,15 @@ export function formatPrChecks(value: unknown): string { else if (status === "FAIL") fail += 1; else if (status === "WAIT") wait += 1; } - const summary = [ok ? `${ok} passing` : null, fail ? `${fail} failing` : null, wait ? `${wait} pending` : null] - .filter(Boolean) - .join(" · ") || `${checks.length} check${checks.length === 1 ? "" : "s"}`; + // ADE-135: rows can all be green and still verify nothing (third-party apps + // only, or an all-skipped suite). The rollup is the authority when it says + // so; otherwise fall back to "nothing passed" as the same signal. + const notRun = rollup === "not_run" || (ok === 0 && fail === 0 && wait === 0); + const summary = notRun + ? `CI: not run${reason ? ` — ${reason}` : ""}` + : [ok ? `${ok} passing` : null, fail ? `${fail} failing` : null, wait ? `${wait} pending` : null] + .filter(Boolean) + .join(" · ") || `${checks.length} check${checks.length === 1 ? "" : "s"}`; return [ `PR checks · ${summary}`, "", @@ -396,7 +419,7 @@ export function formatPrComments(value: unknown): string { const threads = firstRecordArray(root, ["reviewThreads", "threads"]); const comments = firstRecordArray(root, ["comments", "issueComments"]); const headerParts = [ - summary ? pickString(summary, ["checksStatus"]) : null, + summary ? checksStatusWord(pickString(summary, ["checksStatus"])) : null, summary ? `${asString(summary.actionableComments) ?? "0"} actionable` : null, ].filter(Boolean); const lines = [`PR comments${headerParts.length ? ` · ${headerParts.join(" · ")}` : ""}`]; diff --git a/apps/desktop/scripts/export-browser-mock-ade-snapshot.mjs b/apps/desktop/scripts/export-browser-mock-ade-snapshot.mjs index 63a27872b..60e67f6cc 100644 --- a/apps/desktop/scripts/export-browser-mock-ade-snapshot.mjs +++ b/apps/desktop/scripts/export-browser-mock-ade-snapshot.mjs @@ -390,6 +390,14 @@ function rowToPr(row) { baseBranch: String(row.base_branch ?? "main"), headBranch: String(row.head_branch ?? ""), checksStatus: row.checks_status ?? "none", + // ADE-135: the rollup's explanation and the required contexts that never + // reported travel with the status, so an exported "not_run" can be + // rendered with its reason instead of a bare muted badge. + checksReason: row.checks_reason ?? null, + checksMissingRequired: (() => { + const parsed = safeJson(row.checks_missing_required, []); + return Array.isArray(parsed) ? parsed.map(String) : []; + })(), reviewStatus: row.review_status ?? "none", additions: Number(row.additions ?? 0), deletions: Number(row.deletions ?? 0), diff --git a/apps/desktop/src/main/services/prs/prChatCards.test.ts b/apps/desktop/src/main/services/prs/prChatCards.test.ts index 1ec194d77..20058b6c2 100644 --- a/apps/desktop/src/main/services/prs/prChatCards.test.ts +++ b/apps/desktop/src/main/services/prs/prChatCards.test.ts @@ -75,7 +75,7 @@ function run(overrides: Partial = {}): PrActionRun { }; } -function check(name: string): PrCheck { +function check(name: string, overrides: Partial = {}): PrCheck { return { name, status: "completed", @@ -83,6 +83,10 @@ function check(name: string): PrCheck { detailsUrl: null, startedAt: null, completedAt: null, + // Unattributed by default: that is what a preview/review bot looks like to + // the card, and it must never land in the CI group. + appSlug: null, + ...overrides, }; } @@ -126,18 +130,51 @@ describe("PR chat cards", () => { progress: { passed: 0, failed: 1, running: 1, queued: 0 }, navTarget: { kind: "pr", detailTab: "checks", prNumber: 7 }, }); - expect(card.rows?.map((row) => row.text)).toEqual(["lint", "test"]); + expect(card.rows?.map((row) => row.text)).toEqual(["lint", "test", "external"]); + expect(card.rows?.map((row) => row.detail)).toEqual([ + "CI · failed", + "CI · running", + "Other · passed", + ]); }); - it("falls back to check rows when Actions jobs are unavailable", () => { + it("keeps a third-party check out of the CI group and out of CI's counters", () => { const card = buildPrCiCard({ - pr: pr({ checksStatus: "passing" }), + pr: pr({ checksStatus: "not_run" }), runs: [], - checks: [check("Vercel")], + checks: [check("Vercel"), check("CodeRabbit"), check("coverage-bot")], }); expect(card.state).toBe("terminal"); + expect(card.title).toBe("CI has not run"); + expect(card.progress).toEqual({ passed: 0, failed: 0, running: 0, queued: 0 }); + expect(card.metrics).toEqual([ + { label: "CI checks", value: "0", tone: "neutral" }, + { label: "other checks", value: "3", tone: "neutral" }, + ]); + expect(card.rows?.[0]).toMatchObject({ text: "No CI checks reported on this commit" }); + expect(card.rows?.slice(1).map((row) => row.detail)).toEqual(["Other · passed", "Other · passed"]); + expect(card.rowsTruncated).toBe(1); + }); + + it("counts a legacy commit status as CI — Buildkite and CircleCI report that way", () => { + const card = buildPrCiCard({ + pr: pr({ checksStatus: "passing" }), + runs: [], + checks: [check("buildkite/ci", { id: null, appSlug: "commit_status" }), check("Vercel")], + }); expect(card.progress).toEqual({ passed: 1, failed: 0, running: 0, queued: 0 }); - expect(card.rows?.[0]?.text).toBe("Vercel"); + expect(card.metrics).toContainEqual({ label: "other checks", value: "1", tone: "neutral" }); + expect(card.rows?.map((row) => row.detail)).toEqual(["CI · passed", "Other · passed"]); + }); + + it("counts an Actions check run as CI when only the checks endpoint answered", () => { + const card = buildPrCiCard({ + pr: pr({ checksStatus: "passing" }), + runs: [], + checks: [check("test-desktop", { appSlug: "github-actions" })], + }); + expect(card.progress).toEqual({ passed: 1, failed: 0, running: 0, queued: 0 }); + expect(card.rows?.[0]).toMatchObject({ text: "test-desktop", detail: "CI · passed" }); }); it("does not report neutral, skipped, or indeterminate checks as passed", () => { @@ -172,11 +209,51 @@ describe("PR chat cards", () => { expect(card.progress).toEqual({ passed: 0, failed: 0, running: 0, queued: 0 }); expect(card.metrics).toContainEqual({ label: "other", value: "2", tone: "neutral" }); expect(card.rows).toMatchObject([ - { text: "unknown", icon: "info", detail: "unknown", tone: "neutral" }, - { text: "optional", icon: "skipped", detail: "skipped", tone: "neutral" }, + { text: "unknown", icon: "info", detail: "CI · unknown", tone: "neutral" }, + { text: "optional", icon: "skipped", detail: "CI · skipped", tone: "neutral" }, ]); }); + // ADE-135: PR #988 rendered "CI passed" while GitHub Actions never registered + // a suite. The headline must follow the CI group, and an absence is neutral — + // it is a statement about what we know, not an alarm. + it("never claims a run for a PR nothing verified", () => { + const notRun = buildPrCiCard({ + pr: pr({ + checksStatus: "not_run", + checksReason: "3 checks reported, none from a CI provider. CI has not run on this commit.", + }), + runs: [], + checks: [check("Vercel"), check("CodeRabbit")], + }); + expect(notRun.title).toBe("CI has not run"); + expect(notRun.subtitle).toContain("none from a CI provider"); + expect(notRun.metrics?.every((metric) => metric.tone !== "warning")).toBe(true); + expect(notRun.fallbackText).toContain("ci has not run"); + + const none = buildPrCiCard({ pr: pr({ checksStatus: "none" }), runs: [], checks: [] }); + expect(none.title).toBe("No checks reported"); + expect(none.metrics).toEqual([{ label: "status", value: "none", tone: "neutral" }]); + expect(none.rows).toEqual([]); + }); + + it("names the required checks that never reported, truncating the shard list", () => { + const shards = Array.from({ length: 8 }, (_, index) => `test-desktop (${index + 1}/8)`); + const card = buildPrCiCard({ + pr: pr({ + checksStatus: "not_run", + checksMissingRequired: shards, + }), + runs: [], + checks: [], + }); + const required = card.rows?.find((row) => row.detail === "required"); + expect(required?.text).toBe( + "Required checks with no result: test-desktop (1/8), test-desktop (2/8), test-desktop (3/8), +5 more", + ); + expect(required?.tone).toBe("neutral"); + }); + it("summarizes only the newest run for each workflow", () => { const card = buildPrCiCard({ pr: pr({ checksStatus: "passing" }), diff --git a/apps/desktop/src/main/services/prs/prChatCards.ts b/apps/desktop/src/main/services/prs/prChatCards.ts index 3e7942cb7..40f2f1b1a 100644 --- a/apps/desktop/src/main/services/prs/prChatCards.ts +++ b/apps/desktop/src/main/services/prs/prChatCards.ts @@ -15,6 +15,7 @@ import type { PrSummary, } from "../../../shared/types"; import { pipelineStateOf } from "../../../shared/prPipelineState"; +import { isCiProducerCheck } from "../../../shared/prChecksRollup"; import { latestRunsByWorkflow } from "./workflowGraph"; export type PrCardChange = { @@ -95,52 +96,82 @@ function compactCardText(value: string, maxChars = 480): string { return compact.length <= maxChars ? compact : `${compact.slice(0, maxChars - 1).trimEnd()}…`; } -/** Rows a CI card shows before it starts counting. Kept small; the rest is `+N more`. */ +/** Rows the CI group shows before it starts counting. The rest is `+N more`. */ const CI_ROW_CAP = 3; +/** The non-CI group is context, not the headline, so it gets fewer rows. */ +const OTHER_ROW_CAP = 2; +/** Required contexts named inline before the list collapses into `+N more`. */ +const MISSING_REQUIRED_CAP = 3; -function ciRows(runs: PrActionRun[], checks: PrCheck[]): { - rows: AdeCardRow[]; - progress: AdeCardProgress; - other: number; - truncated: number; -} { - const jobs = runs.flatMap((run) => run.jobs); - const items: Array = jobs.length > 0 ? jobs : checks; - const progress: AdeCardProgress = { - passed: 0, - failed: 0, - running: 0, - queued: 0, - }; +type RankedItem = { item: PrActionJob | PrCheck; bucket: JobBucket }; - const ranked = items +function rankItems(items: Array): RankedItem[] { + return items .map((item) => ({ item, bucket: itemBucket(item) })) .sort((left, right) => ( jobPriority(left.bucket) - jobPriority(right.bucket) || left.item.name.localeCompare(right.item.name) )); - for (const entry of ranked) { +} + +function rowTone(bucket: JobBucket): AdeCardRow["tone"] { + if (bucket === "failed") return "warning"; + if (bucket === "passed") return "success"; + if (bucket === "running" || bucket === "queued") return "accent"; + return "neutral"; +} + +function toRows(entries: RankedItem[], group: "CI" | "Other"): AdeCardRow[] { + return entries.map(({ item, bucket }) => ({ + icon: jobIcon(bucket), + text: item.name, + // The group name travels in `detail` so every surface — desktop, TUI, iOS — + // shows the split without needing a new payload field to render headers. + detail: `${group} · ${bucket}`, + tone: rowTone(bucket), + })); +} + +function countBuckets(entries: RankedItem[]): { progress: AdeCardProgress; other: number } { + const progress: AdeCardProgress = { passed: 0, failed: 0, running: 0, queued: 0 }; + for (const entry of entries) { if (entry.bucket in progress) progress[entry.bucket as keyof AdeCardProgress] += 1; } return { progress, - other: ranked.filter((entry) => entry.bucket === "skipped" || entry.bucket === "unknown").length, - truncated: Math.max(0, ranked.length - CI_ROW_CAP), - rows: ranked.slice(0, CI_ROW_CAP).map(({ item, bucket }) => ({ - icon: jobIcon(bucket), - text: item.name, - detail: bucket, - tone: bucket === "failed" - ? "warning" - : bucket === "passed" - ? "success" - : bucket === "running" || bucket === "queued" - ? "accent" - : "neutral", - })), + other: entries.filter((entry) => entry.bucket === "skipped" || entry.bucket === "unknown").length, }; } +/** + * Split everything we fetched into the CI group and the everything-else group. + * + * Actions jobs are CI by construction — they only exist because a workflow run + * produced them. Check rows are classified by `isCiProducerCheck`, the same + * predicate the rollup uses, so the headline and the rows can never go back to + * disagreeing about what counts as CI. A check with no `appSlug` (older row, or + * a producer GitHub did not name) lands in "Other": an unattributable success + * is not evidence that CI ran. Any check whose name a job already covers is + * dropped so the same work is not counted twice once both endpoints answered. + */ +function groupCheckItems(runs: PrActionRun[], checks: PrCheck[]): { + ci: RankedItem[]; + other: RankedItem[]; +} { + const jobs = runs.flatMap((run) => run.jobs); + const jobNames = new Set(jobs.map((job) => job.name)); + const remaining = checks.filter((check) => !jobNames.has(check.name)); + return { + ci: rankItems([...jobs, ...remaining.filter((check) => isCiProducerCheck(check.appSlug))]), + other: rankItems(remaining.filter((check) => !isCiProducerCheck(check.appSlug))), + }; +} + +function formatContextList(contexts: readonly string[], limit = MISSING_REQUIRED_CAP): string { + if (contexts.length <= limit) return contexts.join(", "); + return `${contexts.slice(0, limit).join(", ")}, +${contexts.length - limit} more`; +} + export function selectPrCardSession( sessions: AgentChatSessionSummary[], ): AgentChatSessionSummary | null { @@ -176,40 +207,97 @@ export function buildPrCiCard(args: { || run?.headSha?.trim() || `${pr.githubPrNumber}:unknown-head`; const episode = `${episodeHead}:${attempt}`; - const { rows, progress, other, truncated } = ciRows(latestRuns, checks); + const groups = groupCheckItems(latestRuns, checks); + // The headline is the CI group's story. `progress` follows it for the same + // reason: mixing producers is what let three third-party successes render as + // "CI passed · 3 jobs" on PR #988 while GitHub Actions never ran. + const { progress, other } = countBuckets(groups.ci); + const ciTotal = groups.ci.length; + const otherTotal = groups.other.length; const state = pr.checksStatus === "pending" ? "live" : "terminal"; const title = pr.checksStatus === "passing" ? "CI passed" : pr.checksStatus === "failing" ? "CI failed" - : "CI is running"; + : pr.checksStatus === "pending" + ? "CI is running" + : pr.checksStatus === "not_run" + // Checks exist or are expected, but nothing verified this commit. The + // old fallback said "CI is running" here, which was its own lie. + ? "CI has not run" + : "No checks reported"; + // `not_run`/`none` are neutral on purpose: the card reports an absence, and + // an absence is not an alarm. Only a real red job earns the amber treatment. const tone = pr.checksStatus === "failing" ? "warning" : pr.checksStatus === "passing" ? "success" - : "accent"; - const total = progress.passed + progress.failed + progress.running + progress.queued + other; + : pr.checksStatus === "pending" + ? "accent" + : "neutral"; + const reason = pr.checksReason?.trim() || null; + const missingRequired = (pr.checksMissingRequired ?? []).map((c) => c.trim()).filter(Boolean); // A partial response is still degraded: the surviving endpoint's rows remain // useful, but they are not the complete job inventory. Keep the warning and // Retry action alongside those rows instead of presenting them as complete. const degraded = fetchError != null; - return { - cardId: `pr-ci:${pr.id}:${episode}`, - variant: "pr_ci", - state, - title, - subtitle: `PR #${pr.githubPrNumber}${run?.name ? ` · ${run.name}` : ""}`, - metrics: total > 0 + const rows: AdeCardRow[] = [ + ...(missingRequired.length > 0 + ? [{ + icon: "queued" as const, + text: `${missingRequired.length === 1 ? "Required check" : "Required checks"} with no result: ${formatContextList(missingRequired)}`, + detail: "required", + tone: "neutral" as const, + }] + : []), + ...toRows(groups.ci.slice(0, CI_ROW_CAP), "CI"), + // Say the quiet part out loud when the other group is carrying the card: + // without this row, three green third-party rows read as three green CI rows. + ...(ciTotal === 0 && !degraded && (otherTotal > 0 || pr.checksStatus === "not_run") + ? [{ + icon: "info" as const, + text: "No CI checks reported on this commit", + detail: "CI", + tone: "neutral" as const, + }] + : []), + ...toRows(groups.other.slice(0, OTHER_ROW_CAP), "Other"), + ]; + const truncated = Math.max(0, ciTotal - CI_ROW_CAP) + Math.max(0, otherTotal - OTHER_ROW_CAP); + + const metrics: AdeCardPayload["metrics"] = degraded && ciTotal === 0 && otherTotal === 0 + ? [] + : ciTotal > 0 ? [ { label: "passed", value: String(progress.passed), tone: "success" }, { label: "failed", value: String(progress.failed), tone: "warning" }, { label: "active", value: String(progress.running + progress.queued), tone: "accent" }, ...(other > 0 ? [{ label: "other", value: String(other), tone: "neutral" as const }] : []), + ...(otherTotal > 0 + ? [{ label: "other checks", value: String(otherTotal), tone: "neutral" as const }] + : []), ] - : degraded - ? [] - : [{ label: "status", value: pr.checksStatus, tone }], + : otherTotal > 0 + // The whole point of the split: "CI 0 / other checks 3" is the honest + // reading of a PR that only preview and review apps reported on. + ? [ + { label: "CI checks", value: "0", tone: "neutral" as const }, + { label: "other checks", value: String(otherTotal), tone: "neutral" as const }, + ] + : [{ label: "status", value: pr.checksStatus, tone }]; + + return { + cardId: `pr-ci:${pr.id}:${episode}`, + variant: "pr_ci", + state, + title, + // The reason explains the headline, so it outranks the run name for the one + // line of subtitle the card gets. + subtitle: reason + ? `PR #${pr.githubPrNumber} · ${compactCardText(reason, 160)}` + : `PR #${pr.githubPrNumber}${run?.name ? ` · ${run.name}` : ""}`, + metrics, rows, progress, ...(truncated > 0 ? { rowsTruncated: truncated } : {}), @@ -222,7 +310,7 @@ export function buildPrCiCard(args: { navTarget: prNavTarget(pr, "checks"), fallbackText: degraded ? `PR #${pr.githubPrNumber} checks are ${pr.checksStatus}; job detail unavailable in part (${fetchError}).` - : `PR #${pr.githubPrNumber} ${title.toLowerCase()}.`, + : `PR #${pr.githubPrNumber} ${title.toLowerCase()}.${reason ? ` ${compactCardText(reason, 160)}` : ""}`, }; } diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index 85411a4c0..e6971e99f 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -3336,7 +3336,10 @@ describe("prService.refresh", () => { const initialUpsert = db.run.mock.calls.find(([sql]: [unknown]) => String(sql).includes("update pull_requests") && String(sql).includes("created_at = ?") ); - expect(initialUpsert?.[1]?.[15]).toBe(githubCreatedAt); + // Positional, so it moves whenever upsertRow gains a column. The four + // ADE-135 checks_reason/checks_missing_required params sit between + // checks_status and review_status, pushing created_at from 15 to 19. + expect(initialUpsert?.[1]?.[19]).toBe(githubCreatedAt); db.run.mockClear(); merged = true; diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 2a03d6200..1bc0756f3 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -155,6 +155,9 @@ import type { IssueTracker } from "../cto/issueTracker"; import type { LinearLiveStatusService } from "../cto/linearLiveStatusService"; import { publishLinearPrCard } from "../cto/linearLaneCardService"; import { parseSyntheticGithubPrId, syntheticGithubPrId } from "../../../shared/types/prs"; +import { rollupChecks } from "../../../shared/prChecksRollup"; +import type { ChecksRollupCheckRun, ChecksRollupCommitStatus } from "../../../shared/prChecksRollup"; +import { createRequiredChecksResolver } from "./requiredChecks"; import { spawn } from "node:child_process"; import { runGit, runGitMergeTree, runGitOrThrow } from "../git/git"; import { shouldAttemptAdminMergeForRestError } from "./resolverUtils"; @@ -234,6 +237,10 @@ type PullRequestRow = { merge_method?: string | null; commit_count?: number | null; changed_files?: number | null; + /** ADE-135: why the rollup is not green, when that is not self-evident. */ + checks_reason?: string | null; + /** ADE-135: JSON array of required contexts that never reported. */ + checks_missing_required?: string | null; }; type PrAutoLinkIgnoreRow = { @@ -984,44 +991,42 @@ function toChecksStatus(state: string | null | undefined): PrChecksStatus { return "none"; } -function toChecksStatusFromCheckRuns(checkRuns: any[]): PrChecksStatus | null { - if (!Array.isArray(checkRuns) || checkRuns.length === 0) return null; - - let hasPending = false; - let hasFailure = false; - let hasSuccessLike = false; - for (const run of checkRuns) { - const status = asString(run?.status).toLowerCase(); - const conclusion = asString(run?.conclusion).toLowerCase(); - if (status && status !== "completed") { - // A check can have a conclusion (e.g. "skipped") even when its status - // hasn't flipped to "completed". Treat it as finished if a terminal - // conclusion is present; otherwise it's genuinely pending. - if (!conclusion || (conclusion !== "success" && conclusion !== "neutral" && conclusion !== "skipped" && conclusion !== "failure" && conclusion !== "cancelled" && conclusion !== "timed_out" && conclusion !== "action_required" && conclusion !== "stale")) { - hasPending = true; - continue; - } - } - if (!conclusion) continue; - if (conclusion === "success" || conclusion === "neutral" || conclusion === "skipped") { - hasSuccessLike = true; - continue; - } - if ( - conclusion === "failure" || - conclusion === "cancelled" || - conclusion === "timed_out" || - conclusion === "action_required" || - conclusion === "stale" - ) { - hasFailure = true; - } +/** Row storage for `checksMissingRequired` is JSON; tolerate anything else. */ +function parseMissingRequired(raw: string | null | undefined): string[] { + if (!raw) return []; + try { + const parsed = JSON.parse(raw); + return Array.isArray(parsed) ? parsed.filter((entry): entry is string => typeof entry === "string") : []; + } catch { + return []; } +} - if (hasPending) return "pending"; - if (hasFailure) return "failing"; - if (hasSuccessLike) return "passing"; - return "none"; +/** Normalize a raw check-runs payload into the rollup's input shape. */ +function toRollupCheckRuns(checkRuns: any[]): ChecksRollupCheckRun[] { + if (!Array.isArray(checkRuns)) return []; + return checkRuns.map((run) => ({ + name: asString(run?.name), + status: asString(run?.status).toLowerCase(), + conclusion: asString(run?.conclusion).toLowerCase() || null, + appSlug: asString(run?.app?.slug).toLowerCase() || null, + })); +} + +function toRollupCommitStatuses( + statuses: Array<{ context: string; state: string }> | undefined, +): ChecksRollupCommitStatus[] { + if (!Array.isArray(statuses)) return []; + return statuses.map((status) => ({ + context: asString(status?.context), + state: asString(status?.state).toLowerCase(), + })); +} + +function commitAgeMs(timestamp: string | null | undefined, nowMs: number): number | null { + if (!timestamp) return null; + const parsed = Date.parse(timestamp); + return Number.isFinite(parsed) ? Math.max(0, nowMs - parsed) : null; } function computeReviewStatus(args: { @@ -1055,6 +1060,8 @@ function rowToSummary(row: PullRequestRow): PrSummary { baseBranch: row.base_branch, headBranch: row.head_branch, checksStatus: (row.checks_status as PrChecksStatus) ?? "none", + checksReason: row.checks_reason ?? null, + checksMissingRequired: parseMissingRequired(row.checks_missing_required), reviewStatus: (row.review_status as PrReviewStatus) ?? "none", additions: Number(row.additions ?? 0), deletions: Number(row.deletions ?? 0), @@ -1199,6 +1206,10 @@ function compareBackgroundRefreshPriority(left: PullRequestRow, right: PullReque function hasMaterialSummaryChange(row: PullRequestRow, summary: PrSummary): boolean { return row.state !== summary.state || row.checks_status !== summary.checksStatus + // ADE-135: the reason is what the tooltip and chat card render, so a change + // in *why* CI is not green is a material change even when the state holds. + || (Object.prototype.hasOwnProperty.call(summary, "checksReason") + && (row.checks_reason ?? null) !== (summary.checksReason ?? null)) || row.review_status !== summary.reviewStatus || (row.title ?? "") !== summary.title || row.base_branch !== summary.baseBranch @@ -1442,7 +1453,8 @@ export function createPrService({ checks_status, review_status, additions, deletions, last_synced_at, created_at, updated_at, merged_at, creation_strategy, merge_conflicts, behind_base_by, head_sha, detached_at, detached_lane_name, detached_lane_color, detached_provenance, - merged_by_login, merged_by_avatar_url, merge_method, commit_count, changed_files`; + merged_by_login, merged_by_avatar_url, merge_method, commit_count, changed_files, + checks_reason, checks_missing_required`; /** * Lane-scoped "what is this lane working on" lookups must ignore detached rows. * A detached row is history: its lane was deleted, or the lane moved to another @@ -2465,6 +2477,15 @@ export function createPrService({ const hasBehindBaseBy = Object.prototype.hasOwnProperty.call(summary, "behindBaseBy"); const behindBaseByValue = normalizeBehindBaseBy(summary.behindBaseBy); const hasMergedAt = Object.prototype.hasOwnProperty.call(summary, "mergedAt"); + // ADE-135. Guarded like mergeConflicts/behindBaseBy: partial summaries flow + // through here constantly, and an unguarded write would wipe a good reason + // every time a caller upserted without one. + const hasChecksReason = Object.prototype.hasOwnProperty.call(summary, "checksReason"); + const checksReasonValue = summary.checksReason ?? null; + const hasChecksMissingRequired = Object.prototype.hasOwnProperty.call(summary, "checksMissingRequired"); + const checksMissingRequiredValue = Array.isArray(summary.checksMissingRequired) + ? JSON.stringify(summary.checksMissingRequired) + : null; // By default we only adopt an existing row that is already associated with // this lane. Callers like `linkToLane`/`refreshOne` must not silently // reassign an existing PR row from another lane just because the repo/PR @@ -2523,6 +2544,8 @@ export function createPrService({ base_branch = ?, head_branch = ?, checks_status = ?, + checks_reason = case when ? then ? else checks_reason end, + checks_missing_required = case when ? then ? else checks_missing_required end, review_status = ?, additions = ?, deletions = ?, @@ -2547,6 +2570,10 @@ export function createPrService({ summary.baseBranch, summary.headBranch, summary.checksStatus, + hasChecksReason ? 1 : 0, + checksReasonValue, + hasChecksMissingRequired ? 1 : 0, + checksMissingRequiredValue, summary.reviewStatus, summary.additions, summary.deletions, @@ -2609,8 +2636,10 @@ export function createPrService({ merged_at, creation_strategy, merge_conflicts, - behind_base_by - ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + behind_base_by, + checks_reason, + checks_missing_required + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `, [ summary.id, @@ -2635,7 +2664,9 @@ export function createPrService({ summary.mergedAt ?? null, summary.creationStrategy ?? null, mergeConflictsValue, - behindBaseByValue + behindBaseByValue, + checksReasonValue, + checksMissingRequiredValue ] ); return summary.id; @@ -4662,6 +4693,44 @@ export function createPrService({ return Array.isArray(data?.check_runs) ? data.check_runs : []; }; + const requiredChecksResolver = createRequiredChecksResolver({ + apiRequest: (options) => githubService.apiRequest(options), + logger, + }); + + /** + * ADE-135: the single checks derivation. Both the poller and the webhook path + * land here — `ingestGithubWebhook` uses its payload only to resolve which PR + * changed and then re-derives through `computeStatus`, so there is exactly one + * place where "is this commit verified?" gets answered. + */ + const deriveChecksRollup = async (args: { + repo: GitHubRepoRef; + baseBranch: string; + checkRuns: any[]; + combinedStatus: { state: string; statuses: Array<{ context: string; state: string }> }; + mergeStateBlocked: boolean; + /** + * PR `updated_at`. An approximation of head-commit age: GitHub bumps it on + * every `synchronize`, which is precisely the event that resets CI. Only + * used to choose between "has not run yet" and "has not run", so being a + * few seconds off changes wording, never state. + */ + headActivityAt: string | null; + }) => { + const required = await requiredChecksResolver + .resolve(args.repo, args.baseBranch, args.mergeStateBlocked) + .catch(() => ({ contexts: null, source: "unavailable" as const })); + return rollupChecks({ + checkRuns: toRollupCheckRuns(args.checkRuns), + commitStatuses: toRollupCommitStatuses(args.combinedStatus?.statuses), + requiredContexts: required.contexts, + requiredSource: required.source, + mergeStateBlocked: args.mergeStateBlocked, + headCommitAgeMs: commitAgeMs(args.headActivityAt, Date.now()), + }); + }; + const fetchCompare = async (repo: GitHubRepoRef, baseSha: string, headSha: string): Promise<{ behindBy: number }> => { const { data } = await githubService.apiRequest({ method: "GET", @@ -4725,9 +4794,23 @@ export function createPrService({ if (review.state === "changes_requested") reviewStatesByUser.set(review.reviewer, "CHANGES_REQUESTED"); } - const checksStatus = shouldFetchLiveStatus - ? toChecksStatusFromCheckRuns(checkRuns) ?? toChecksStatus(combinedStatus.state) - : (row.checks_status as PrChecksStatus | null) ?? "none"; + const checksRollup = shouldFetchLiveStatus + ? await deriveChecksRollup({ + repo, + baseBranch: asString(pr?.base?.ref) || row.base_branch, + checkRuns, + combinedStatus, + // No GraphQL merge box on this path; the rollup treats the absence as + // "no corroboration", never as "not blocked". + mergeStateBlocked: false, + headActivityAt: asString(pr?.updated_at) || row.updated_at || null, + }) + : { + status: (row.checks_status as PrChecksStatus | null) ?? "none", + reason: row.checks_reason ?? null, + missingRequiredContexts: parseMissingRequired(row.checks_missing_required), + }; + const checksStatus = checksRollup.status; const reviewStatus = shouldFetchLiveStatus ? computeReviewStatus({ requestedReviewers, requestedTeams, reviewStatesByUser }) : (row.review_status as PrReviewStatus | null) ?? "none"; @@ -4750,6 +4833,8 @@ export function createPrService({ baseBranch, headBranch, checksStatus, + checksReason: checksRollup.reason, + checksMissingRequired: checksRollup.missingRequiredContexts, reviewStatus, additions, deletions, @@ -5031,7 +5116,15 @@ export function createPrService({ draft: Boolean(pr?.draft), mergedAt: asString(pr?.merged_at) || null }); - const checksStatus = toChecksStatusFromCheckRuns(checkRuns) ?? toChecksStatus(combinedStatus.state); + const checksRollup = await deriveChecksRollup({ + repo, + baseBranch: asString(pr?.base?.ref), + checkRuns, + combinedStatus, + mergeStateBlocked: (mergeState?.mergeStateStatus ?? "").toLowerCase() === "blocked", + headActivityAt: asString(pr?.updated_at) || null, + }); + const checksStatus = checksRollup.status; const reviewStatus = computeReviewStatus({ requestedReviewers, requestedTeams, reviewStatesByUser }); const behindBaseBy = compare.behindBy; @@ -5057,6 +5150,8 @@ export function createPrService({ prId, state: nextState, checksStatus, + checksReason: checksRollup.reason, + checksMissingRequired: checksRollup.missingRequiredContexts, reviewStatus, isMergeable, mergeConflicts: mergeConflicts === true, @@ -5080,6 +5175,8 @@ export function createPrService({ ...summary, state: status.state, checksStatus: status.checksStatus, + checksReason: status.checksReason ?? null, + checksMissingRequired: status.checksMissingRequired ?? [], reviewStatus: status.reviewStatus, additions: Number(pr?.additions ?? summary.additions), deletions: Number(pr?.deletions ?? summary.deletions), @@ -5141,7 +5238,8 @@ export function createPrService({ conclusion, detailsUrl: asString(run?.details_url) || asString(run?.html_url) || null, startedAt: asString(run?.started_at) || null, - completedAt: asString(run?.completed_at) || null + completedAt: asString(run?.completed_at) || null, + appSlug: asString(run?.app?.slug).toLowerCase() || null }); } @@ -5158,7 +5256,10 @@ export function createPrService({ conclusion: s.state === "success" ? "success" : s.state === "failure" || s.state === "error" ? "failure" : null, detailsUrl: s.target_url ?? null, startedAt: s.created_at ?? null, - completedAt: s.updated_at ?? null + completedAt: s.updated_at ?? null, + // Legacy commit statuses are how Jenkins/Buildkite/CircleCI report, so + // they count as CI even though they carry no app slug. + appSlug: "commit_status" }); } diff --git a/apps/desktop/src/main/services/prs/requiredChecks.test.ts b/apps/desktop/src/main/services/prs/requiredChecks.test.ts new file mode 100644 index 000000000..b623dbb60 --- /dev/null +++ b/apps/desktop/src/main/services/prs/requiredChecks.test.ts @@ -0,0 +1,172 @@ +import { describe, expect, it, vi } from "vitest"; +import { createRequiredChecksResolver } from "./requiredChecks"; + +const repo = { owner: "arul28", name: "ADE" }; + +/** + * The real shape of `GET /repos/arul28/ADE/rules/branches/main`, captured live. + * Only the `required_status_checks` rule carries contexts; the rest are noise + * the resolver must skip. + */ +const ADE_MAIN_RULES = [ + { type: "deletion", ruleset_id: 13910754 }, + { type: "non_fast_forward", ruleset_id: 13910754 }, + { type: "pull_request", parameters: { required_approving_review_count: 1 }, ruleset_id: 13910754 }, + { + type: "required_status_checks", + parameters: { + strict_required_status_checks_policy: true, + required_status_checks: [{ context: "ci-pass" }], + }, + ruleset_id: 13910754, + }, +]; + +function resolver(handler: (path: string) => Promise<{ data: unknown }>) { + const apiRequest = vi.fn(async (options: { method: "GET"; path: string }) => handler(options.path)); + return { + apiRequest, + instance: createRequiredChecksResolver({ apiRequest: apiRequest as never }), + }; +} + +describe("createRequiredChecksResolver", () => { + it("reads required contexts from the rulesets tier", async () => { + const { instance } = resolver(async (path) => { + if (path.includes("/rules/branches/")) return { data: ADE_MAIN_RULES }; + throw new Error("unexpected"); + }); + + expect(await instance.resolve(repo, "main", false)).toEqual({ + contexts: ["ci-pass"], + source: "rulesets", + }); + }); + + it("falls back to classic branch protection when rulesets are empty", async () => { + // Repos that never migrated to rulesets only answer on the admin endpoint. + const { instance } = resolver(async (path) => { + if (path.includes("/rules/branches/")) return { data: [] }; + return { data: { required_status_checks: { contexts: ["build", "test"] } } }; + }); + + expect(await instance.resolve(repo, "main", false)).toEqual({ + contexts: ["build", "test"], + source: "branch_protection", + }); + }); + + it("reads the modern `checks` shape of branch protection too", async () => { + const { instance } = resolver(async (path) => { + if (path.includes("/rules/branches/")) return { data: [] }; + return { data: { required_status_checks: { checks: [{ context: "ci-pass", app_id: 15368 }] } } }; + }); + + const result = await instance.resolve(repo, "main", false); + expect(result.contexts).toEqual(["ci-pass"]); + }); + + it("returns unknown — not 'none required' — when no credential can read either tier", async () => { + // A PAT without admin 403s on protection. Claiming "nothing is required" + // there would hand back exactly the false green this ticket is about. + const { instance } = resolver(async () => { + throw new Error("403 Forbidden"); + }); + + expect(await instance.resolve(repo, "main", false)).toEqual({ + contexts: null, + source: "unavailable", + }); + }); + + it("distinguishes a readable branch that requires nothing from an unreadable one", async () => { + const { instance } = resolver(async (path) => { + if (path.includes("/rules/branches/")) return { data: [] }; + return { data: {} }; + }); + + const result = await instance.resolve(repo, "main", false); + expect(result.contexts).toEqual([]); + expect(result.source).not.toBe("unavailable"); + }); + + it("labels the unknown case with merge_state when GitHub reports the merge blocked", async () => { + const { instance } = resolver(async () => { + throw new Error("404"); + }); + + expect(await instance.resolve(repo, "main", true)).toEqual({ + contexts: null, + source: "merge_state", + }); + }); + + it("applies the merge_state overlay to cached results too", async () => { + // Two PRs on the same blocked branch must not disagree on cache timing. + const { instance } = resolver(async () => { + throw new Error("403 Forbidden"); + }); + + expect((await instance.resolve(repo, "main", true)).source).toBe("merge_state"); + expect((await instance.resolve(repo, "main", true)).source).toBe("merge_state"); + expect((await instance.resolve(repo, "main", false)).source).toBe("unavailable"); + }); + + it("caches per branch so a webhook storm does not re-ask GitHub", async () => { + const { apiRequest, instance } = resolver(async (path) => + path.includes("/rules/branches/") ? { data: ADE_MAIN_RULES } : { data: {} }, + ); + + await instance.resolve(repo, "main", false); + await instance.resolve(repo, "main", false); + await instance.resolve(repo, "main", false); + + expect(apiRequest).toHaveBeenCalledTimes(1); + }); + + it("does not serve one branch's requirements to another", async () => { + const { instance } = resolver(async (path) => { + if (!path.includes("/rules/branches/")) return { data: {} }; + return { data: path.endsWith("main") ? ADE_MAIN_RULES : [] }; + }); + + expect((await instance.resolve(repo, "main", false)).contexts).toEqual(["ci-pass"]); + expect((await instance.resolve(repo, "release", false)).contexts).toEqual([]); + }); + + it("re-reads after invalidate", async () => { + const { apiRequest, instance } = resolver(async (path) => + path.includes("/rules/branches/") ? { data: ADE_MAIN_RULES } : { data: {} }, + ); + + await instance.resolve(repo, "main", false); + instance.invalidate(repo, "main"); + await instance.resolve(repo, "main", false); + + expect(apiRequest).toHaveBeenCalledTimes(2); + }); + + it("encodes branch names containing slashes", async () => { + const { apiRequest, instance } = resolver(async () => ({ data: [] })); + await instance.resolve(repo, "release/2026-08", false); + + expect(apiRequest.mock.calls[0]![0].path).toContain("release%2F2026-08"); + }); + + it("unions contexts across multiple applicable rulesets, first-seen order kept", async () => { + const { instance } = resolver(async (path) => { + if (!path.includes("/rules/branches/")) return { data: {} }; + return { + data: [ + { type: "required_status_checks", parameters: { required_status_checks: [{ context: "ci-pass" }] } }, + { + type: "required_status_checks", + parameters: { required_status_checks: [{ context: "ci-pass" }, { context: "security" }] }, + }, + ], + }; + }); + + expect((await instance.resolve(repo, "main", false)).contexts).toEqual(["ci-pass", "security"]); + }); +}); diff --git a/apps/desktop/src/main/services/prs/requiredChecks.ts b/apps/desktop/src/main/services/prs/requiredChecks.ts new file mode 100644 index 000000000..11c11716e --- /dev/null +++ b/apps/desktop/src/main/services/prs/requiredChecks.ts @@ -0,0 +1,206 @@ +import type { GitHubRepoRef } from "../../../shared/types/git"; +import type { RequiredContextSource } from "../../../shared/prChecksRollup"; + +/** + * Resolve the required status-check contexts for a base branch (ADE-135). + * + * Three tiers, because which one answers depends entirely on the credential + * ADE happens to hold — it may be a GitHub App user token, a PAT, the gh CLI's + * oauth token, or an environment token, and those differ in what GitHub will + * show them: + * + * 1. `/rules/branches/{branch}` — repository rulesets. Needs only *read* + * access, so it works for every credential source we support. This is the + * tier that actually answers for most repos, including ADE's own, where + * it reports `ci-pass` as the single required context. + * 2. `/branches/{branch}/protection` — classic branch protection. Requires + * *admin*, so it 403s for most contributors, but it is the only source for + * repos that never migrated to rulesets. + * 3. `mergeStateStatus === "blocked"` — already fetched for the merge box. + * It cannot name contexts and it conflates "required checks missing" with + * "review required" and "branch out of date", so it is corroborating + * evidence only: enough to say something was expected, never enough to + * contradict a genuine pass. + * + * When all three come up empty we return `null` contexts, which means + * "unknown" — never "none required". The rollup then falls back to the + * producer rule alone rather than inventing certainty it does not have. + */ + +export type RequiredChecksResult = { + /** Required contexts, or null when no credential we hold could read them. */ + contexts: string[] | null; + source: RequiredContextSource; +}; + +type CacheEntry = { value: RequiredChecksResult; expiresAt: number }; + +export type RequiredChecksResolver = { + resolve: ( + repo: GitHubRepoRef, + baseBranch: string, + mergeStateBlocked: boolean, + ) => Promise; + invalidate: (repo: GitHubRepoRef, baseBranch: string) => void; +}; + +/** + * Branch protection changes on a human timescale, and this sits on the PR + * refresh path which runs per webhook delivery. Five minutes keeps a busy + * repo's rollups correct without re-asking GitHub for every check_run event. + */ +const DEFAULT_TTL_MS = 5 * 60 * 1000; + +function cacheKey(repo: GitHubRepoRef, baseBranch: string): string { + return `${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}#${baseBranch}`; +} + +function asRecord(value: unknown): Record | null { + return value && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : null; +} + +function contextFromEntry(entry: unknown): string | null { + if (typeof entry === "string") return entry.trim() || null; + const record = asRecord(entry); + if (!record) return null; + const context = record.context; + return typeof context === "string" ? context.trim() || null : null; +} + +/** Union of contexts across every applicable ruleset, first-seen order kept. */ +function contextsFromRules(data: unknown): string[] { + if (!Array.isArray(data)) return []; + const contexts: string[] = []; + const seen = new Set(); + for (const rule of data) { + const record = asRecord(rule); + if (!record || record.type !== "required_status_checks") continue; + const parameters = asRecord(record.parameters); + const required = parameters?.required_status_checks; + if (!Array.isArray(required)) continue; + for (const entry of required) { + const context = contextFromEntry(entry); + if (!context || seen.has(context)) continue; + seen.add(context); + contexts.push(context); + } + } + return contexts; +} + +function contextsFromProtection(data: unknown): string[] { + const record = asRecord(data); + const required = asRecord(record?.required_status_checks); + if (!required) return []; + const contexts: string[] = []; + const seen = new Set(); + // `checks` is the modern shape (context + app id); `contexts` is the legacy + // string array. GitHub still returns both, so read whichever is populated. + for (const source of [required.checks, required.contexts]) { + if (!Array.isArray(source)) continue; + for (const entry of source) { + const context = contextFromEntry(entry); + if (!context || seen.has(context)) continue; + seen.add(context); + contexts.push(context); + } + } + return contexts; +} + +export function createRequiredChecksResolver(args: { + apiRequest: (options: { method: "GET"; path: string }) => Promise<{ data: T }>; + logger?: { warn: (event: string, data?: Record) => void }; + ttlMs?: number; + now?: () => number; +}): RequiredChecksResolver { + const ttlMs = args.ttlMs ?? DEFAULT_TTL_MS; + const now = args.now ?? (() => Date.now()); + const cache = new Map(); + + const readRules = async (repo: GitHubRepoRef, baseBranch: string): Promise => { + try { + const { data } = await args.apiRequest({ + method: "GET", + path: `/repos/${repo.owner}/${repo.name}/rules/branches/${encodeURIComponent(baseBranch)}`, + }); + return contextsFromRules(data); + } catch (error) { + args.logger?.warn("prs.required_checks.rules_failed", { + repo: `${repo.owner}/${repo.name}`, + baseBranch, + error: error instanceof Error ? error.message : String(error), + }); + return null; + } + }; + + const readProtection = async (repo: GitHubRepoRef, baseBranch: string): Promise => { + try { + const { data } = await args.apiRequest({ + method: "GET", + path: `/repos/${repo.owner}/${repo.name}/branches/${encodeURIComponent(baseBranch)}/protection`, + }); + return contextsFromProtection(data); + } catch { + // 403 for non-admins is the expected case, not an incident. The rules + // tier above already logged if it also failed, so stay quiet here. + return null; + } + }; + + const resolve = async ( + repo: GitHubRepoRef, + baseBranch: string, + mergeStateBlocked: boolean, + ): Promise => { + const branch = baseBranch.trim(); + if (!repo.owner || !repo.name || !branch) { + return { contexts: null, source: "unavailable" }; + } + + // Tier 3 is derived per-PR while the cache is per-branch, so the overlay is + // applied on the way out of both the cached and the freshly-read path. + // Applying it only to fresh reads made two PRs on the same blocked branch + // disagree purely on cache timing. + const withMergeStateOverlay = (value: RequiredChecksResult): RequiredChecksResult => + value.contexts === null && mergeStateBlocked ? { contexts: null, source: "merge_state" } : value; + + const key = cacheKey(repo, branch); + const cached = cache.get(key); + if (cached && cached.expiresAt > now()) { + return withMergeStateOverlay(cached.value); + } + + let result: RequiredChecksResult; + const fromRules = await readRules(repo, branch); + if (fromRules && fromRules.length > 0) { + result = { contexts: fromRules, source: "rulesets" }; + } else { + const fromProtection = await readProtection(repo, branch); + if (fromProtection && fromProtection.length > 0) { + result = { contexts: fromProtection, source: "branch_protection" }; + } else if (fromRules !== null || fromProtection !== null) { + // A readable source that lists no required checks is a real answer: + // this branch requires nothing. Distinct from "we could not look". + result = { contexts: [], source: fromRules !== null ? "rulesets" : "branch_protection" }; + } else { + result = { contexts: null, source: "unavailable" }; + } + } + + cache.set(key, { value: result, expiresAt: now() + ttlMs }); + + // Tier 3 never overrides a real answer; it only labels the unknown case so + // the rollup can say *something* was expected here. + return withMergeStateOverlay(result); + }; + + const invalidate = (repo: GitHubRepoRef, baseBranch: string): void => { + cache.delete(cacheKey(repo, baseBranch.trim())); + }; + + return { resolve, invalidate }; +} diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index b38aba43b..ab09fb3c7 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -2419,6 +2419,12 @@ function migrate(db: MigrationDb, rawDb: DatabaseSyncType) { // Denormalized so the merged view survives the snapshot purge on detach. safeAddColumn(db, "alter table pull_requests add column commit_count integer"); safeAddColumn(db, "alter table pull_requests add column changed_files integer"); + // ADE-135: one sentence explaining a non-obvious checks rollup, so every + // surface can say *why* CI is not green without re-deriving the logic. + safeAddColumn(db, "alter table pull_requests add column checks_reason text"); + // Required contexts observed to have never reported, JSON-encoded, used to + // render ghost rows in the PR detail check list. + safeAddColumn(db, "alter table pull_requests add column checks_missing_required text"); db.run("drop table if exists github_pr_cache"); diff --git a/apps/desktop/src/renderer/browserMock.ts b/apps/desktop/src/renderer/browserMock.ts index ed9eb69e0..67d7fa572 100644 --- a/apps/desktop/src/renderer/browserMock.ts +++ b/apps/desktop/src/renderer/browserMock.ts @@ -1285,8 +1285,15 @@ const NORMAL_PRS: any[] = [ 153, "Onboarding wizard with step-by-step project setup", { + // ADE-135 fixture: the only PR here whose CI never ran. Three + // third-party apps reported success (see MOCK_CHECKS_BY_PR["pr-5"]) and + // GitHub Actions registered nothing, which is exactly the shape that + // used to render "CI passed · 3 jobs". state: "open", - checksStatus: "passing", + checksStatus: "not_run", + checksReason: + "3 checks reported, none from a CI provider. CI has not run on this commit.", + checksMissingRequired: ["ci / build"], reviewStatus: "none", additions: 620, deletions: 80, @@ -1623,9 +1630,11 @@ const MOCK_CHECKS_BY_PR: Record = { completedAt: now, }, ], + // Three green rows, zero CI: a review bot, a preview deploy, and a comment + // bot. The rollup calls this "not_run" — the rows are real, the pass is not. "pr-5": [ { - name: "CI / Build", + name: "CodeRabbit", status: "completed", conclusion: "success", detailsUrl: "#", @@ -1633,7 +1642,7 @@ const MOCK_CHECKS_BY_PR: Record = { completedAt: now, }, { - name: "CI / Lint", + name: "Vercel — Preview", status: "completed", conclusion: "success", detailsUrl: "#", @@ -1641,7 +1650,7 @@ const MOCK_CHECKS_BY_PR: Record = { completedAt: now, }, { - name: "CI / Unit Tests", + name: "changeset-bot", status: "completed", conclusion: "success", detailsUrl: "#", @@ -1823,7 +1832,10 @@ const MOCK_STATUS_BY_PR: Record = { "pr-5": { prId: "pr-5", state: "open", - checksStatus: "passing", + checksStatus: "not_run", + checksReason: + "3 checks reported, none from a CI provider. CI has not run on this commit.", + checksMissingRequired: ["ci / build"], reviewStatus: "none", isMergeable: true, mergeConflicts: false, diff --git a/apps/desktop/src/renderer/components/app/AppShell.tsx b/apps/desktop/src/renderer/components/app/AppShell.tsx index 3dd1bb4e4..382f71c85 100644 --- a/apps/desktop/src/renderer/components/app/AppShell.tsx +++ b/apps/desktop/src/renderer/components/app/AppShell.tsx @@ -1457,7 +1457,7 @@ export function AppShell({ children }: { children: React.ReactNode }) { const toastLane = lanes.find((lane) => lane.id === toast.event.laneId) ?? null; const laneName = toastLane?.name ?? toast.event.laneId; const laneColor = toastLane?.color ?? null; - const tone = getPrToastTone(toast.event.kind); + const tone = getPrToastTone(toast.event.kind, toast.event.checksStatus); const toneClasses = getPrToastToneClasses(tone); const Icon = getPrToastIcon(toast.event.kind); const headline = getPrToastHeadline(toast.event); diff --git a/apps/desktop/src/renderer/components/app/prToastPresentation.test.ts b/apps/desktop/src/renderer/components/app/prToastPresentation.test.ts index 438444086..79d5c1d94 100644 --- a/apps/desktop/src/renderer/components/app/prToastPresentation.test.ts +++ b/apps/desktop/src/renderer/components/app/prToastPresentation.test.ts @@ -38,6 +38,16 @@ describe("getPrToastTone", () => { expect(getPrToastTone("merge_ready")).toBe("success"); }); + // ADE-135: a green "ready to merge" toast is what a human — or a /ship loop — + // reads as "the suite is fine". It must not fire when nothing ran. + it("does not celebrate merge_ready when no CI ran on the commit", () => { + expect(getPrToastTone("merge_ready", "not_run")).toBe("info"); + }); + + it("keeps merge_ready green when checks actually passed", () => { + expect(getPrToastTone("merge_ready", "passing")).toBe("success"); + }); + it("returns info for unknown kinds", () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any expect(getPrToastTone("some_future_kind" as any)).toBe("info"); @@ -80,6 +90,11 @@ describe("getPrToastSummary", () => { expect(getPrToastSummary(baseEvent)).toBe("One or more required CI checks failed on this pull request."); }); + it("names the missing CI in a merge-ready summary", () => { + const event = { ...baseEvent, kind: "merge_ready" as const, checksStatus: "not_run" as const, message: "Ready to merge." }; + expect(getPrToastSummary(event)).toBe("Ready to merge. No CI has run on this commit."); + }); + it("falls back to default text when message is empty", () => { const event = { ...baseEvent, message: "" }; expect(getPrToastSummary(event)).toBe("Pull request status changed."); diff --git a/apps/desktop/src/renderer/components/app/prToastPresentation.ts b/apps/desktop/src/renderer/components/app/prToastPresentation.ts index 5752f4f94..a71199c4f 100644 --- a/apps/desktop/src/renderer/components/app/prToastPresentation.ts +++ b/apps/desktop/src/renderer/components/app/prToastPresentation.ts @@ -1,4 +1,4 @@ -import type { PrEventPayload } from "../../../shared/types"; +import type { PrChecksStatus, PrEventPayload } from "../../../shared/types"; type PrNotificationEvent = Extract; @@ -9,10 +9,22 @@ function compactLabel(value: string | null | undefined): string | null { return trimmed.length ? trimmed : null; } -export function getPrToastTone(kind: PrNotificationEvent["kind"]): PrToastTone { +/** + * ADE-135: `not_run` means nothing verified the head commit. The only + * checks-derived success tone is `merge_ready`, so that is the one that must + * never go green on an unverified commit — a green toast is exactly the signal + * a human (or a `/ship` loop) reads as "the suite is fine". It drops to `info` + * rather than `danger`: absence is a finding, not a failure. The lifecycle + * kinds (opened / reopened / merged) say nothing about CI and keep their tone. + */ +export function getPrToastTone( + kind: PrNotificationEvent["kind"], + checksStatus?: PrChecksStatus | null, +): PrToastTone { if (kind === "checks_failing" || kind === "changes_requested") return "danger"; if (kind === "review_requested") return "warning"; - if (kind === "merge_ready" || kind === "merged" || kind === "opened" || kind === "reopened") return "success"; + if (kind === "merge_ready") return checksStatus === "not_run" ? "info" : "success"; + if (kind === "merged" || kind === "opened" || kind === "reopened") return "success"; return "info"; } @@ -21,7 +33,14 @@ export function getPrToastHeadline(event: PrNotificationEvent): string { } export function getPrToastSummary(event: PrNotificationEvent): string { - return compactLabel(event.message) ?? "Pull request status changed."; + const message = compactLabel(event.message) ?? "Pull request status changed."; + // The merge-ready copy reads as an all-clear. When nothing verified the head + // commit, say so in the same breath rather than letting the reader infer a + // green suite from a toast that never mentioned CI. + if (event.kind === "merge_ready" && event.checksStatus === "not_run") { + return `${message} No CI has run on this commit.`; + } + return message; } export function getPrToastMeta(event: PrNotificationEvent, laneName: string | null): string[] { diff --git a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx index be645ff5f..66474f1ad 100644 --- a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx @@ -6,6 +6,7 @@ import { CheckCircle, XCircle, Clock, + MinusCircle, CaretRight, GithubLogo, Copy, @@ -55,6 +56,11 @@ function checksIcon(status: PrSummary["checksStatus"], state: PrSummary["state"] return ; case "pending": return ; + // ADE-135: rendered, but muted and never green — nothing verified this + // commit. Returning null here (the old `default`) hid the finding entirely + // and left the pill looking identical to a repo with no CI. + case "not_run": + return ; default: return null; } @@ -92,10 +98,16 @@ function formatRelativeTime(iso: string | null | undefined): string | null { return new Date(ts).toLocaleDateString(undefined, { month: "short", day: "numeric" }); } -function summarizeChecks(checks: PrCheck[]): { passed: number; failed: number; running: number; total: number } { +function summarizeChecks( + checks: PrCheck[], +): { passed: number; failed: number; running: number; skipped: number; total: number } { let passed = 0; let failed = 0; let running = 0; + // ADE-135: `skipped` used to be counted as `passed`, so a PR whose whole + // suite was skipped rendered a green "3" here. A skipped job verified + // nothing; it gets its own bucket and never colours the pill green. + let skipped = 0; for (const c of checks) { switch (pipelineStateOf(c)) { case "running": @@ -103,15 +115,17 @@ function summarizeChecks(checks: PrCheck[]): { passed: number; failed: number; r running += 1; break; case "passed": - case "skipped": passed += 1; break; + case "skipped": + skipped += 1; + break; case "failed": failed += 1; break; } } - return { passed, failed, running, total: checks.length }; + return { passed, failed, running, skipped, total: checks.length }; } // --------------------------------------------------------------------------- @@ -461,7 +475,11 @@ export const ChatGitToolbar = React.memo(function ChatGitToolbar({ ) : null} {summary.passed === 0 && summary.failed === 0 && summary.running === 0 ? ( - {summary.total} check{summary.total === 1 ? "" : "s"} + // Every row settled without verifying anything (all skipped or + // neutral). Name that instead of a bare count that reads neutral. + + {summary.skipped === summary.total ? "not run" : `${summary.total} check${summary.total === 1 ? "" : "s"}`} + ) : null} ) : ( diff --git a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx index d2fd33a66..8a14bffdc 100644 --- a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx @@ -12,6 +12,7 @@ import { GithubLogo, GitPullRequest, Lightning, + MinusCircle, Sparkle, X, XCircle, @@ -165,9 +166,29 @@ function liveDot(pr: PrSummary, relay: RelayState): { dot: string; label: string : { dot: "bg-amber-400/70", label, title: `Last ${via} ${label} ago` }; } -type ChecksView = { icon: React.ReactNode; text: string; tone: string }; +type ChecksView = { icon: React.ReactNode; text: string; tone: string; title?: string }; -function checksView(checks: PrCheck[] | null, fallback: PrSummary["checksStatus"]): ChecksView | null { +const NOT_RUN_TONE = "text-fg/45"; + +function notRunView(reason: string | null | undefined): ChecksView { + return { + icon: , + text: "CI not run", + tone: NOT_RUN_TONE, + title: reason ?? "No CI has run on this commit.", + }; +} + +function checksView( + checks: PrCheck[] | null, + fallback: PrSummary["checksStatus"], + reason?: string | null, +): ChecksView | null { + // ADE-135: the rollup knows two things the per-job rows cannot show — which + // app produced each run, and which required contexts never reported at all. + // When it says nothing verified this commit, that verdict outranks any count + // of green rows; PR #988 had three third-party successes and zero CI. + if (fallback === "not_run") return notRunView(reason); if (checks && checks.length > 0) { const total = checks.length; const failing = checks.filter((check) => pipelineStateOf(check) === "failed").length; @@ -179,6 +200,9 @@ function checksView(checks: PrCheck[] | null, fallback: PrSummary["checksStatus" if (running > 0) { return { icon: , text: `${passing}/${total} checks running`, tone: "text-amber-300/80" }; } + // Every row settled and not one of them succeeded (all skipped/neutral/ + // cancelled). "0/3 checks" in green read as a pass; it is an absence. + if (passing === 0) return notRunView(reason); return { icon: , text: `${passing}/${total} checks`, tone: "text-emerald-300/80" }; } switch (fallback) { @@ -264,7 +288,13 @@ function PrDetails({ }) { const tone = stateTone(pr.state); const live = liveDot(pr, relay); - const checksInfo = pr.state === "open" || pr.state === "draft" ? checksView(checks, pr.checksStatus) : null; + // The live status wins over the stored summary when we have it, and its + // reason must travel with the status it explains. + const checksStatus = status?.checksStatus ?? pr.checksStatus; + const checksReason = (status ? status.checksReason : pr.checksReason) ?? null; + const checksInfo = pr.state === "open" || pr.state === "draft" + ? checksView(checks, checksStatus, checksReason) + : null; const reviewInfo = reviewView(reviews, pr.reviewStatus); const mergeReady = isMergeReady(pr, status); const pulseHeader = deltaVisible && Boolean(delta) && delta!.kind !== "commit"; @@ -330,7 +360,7 @@ function PrDetails({
{checksInfo ? ( - + {checksInfo.icon} {checksInfo.text} diff --git a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx index 667370162..451b8de52 100644 --- a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx +++ b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx @@ -100,7 +100,8 @@ import { nodeDimensions, branchNameFromRef, globToRegExp, - collectDescendants + collectDescendants, + prChecksLabel } from "./graphHelpers"; import { buildDefaultFilter, @@ -2092,6 +2093,7 @@ function GraphInner({ active = true }: { active?: boolean }) { url: pr.githubUrl, state: pr.state, checksStatus: pr.checksStatus, + checksReason: pr.checksReason ?? null, reviewStatus: pr.reviewStatus, lastSyncedAt: pr.lastSyncedAt ?? null, lastActivityAt: pr.updatedAt, @@ -3291,7 +3293,12 @@ function GraphInner({ active = true }: { active?: boolean }) { const pr = data?.pr ?? null; const prLines = pr ? [ - `PR #${pr.number} · ${pr.state} · checks: ${pr.checksStatus} · reviews: ${pr.reviewStatus}`, + `PR #${pr.number} · ${pr.state} · checks: ${prChecksLabel(pr.checksStatus)} · reviews: ${pr.reviewStatus}`, + // Only shown when the rollup has something to explain, which + // in practice means a not-run or a held-back pending. + pr.checksStatus === "not_run" + ? pr.checksReason ?? "No CI has run on this commit." + : null, `${pr.reviewCount} reviews · ${pr.commentCount} comments${pr.behindBaseBy != null ? ` · behind ${pr.behindBaseBy}` : ""}`, pr.title ? pr.title : null, pr.lastActivityAt ? `activity ${toRelativeTime(pr.lastActivityAt)}` : null, diff --git a/apps/desktop/src/renderer/components/graph/graphHelpers.ts b/apps/desktop/src/renderer/components/graph/graphHelpers.ts index 0004f4eb7..5711cc413 100644 --- a/apps/desktop/src/renderer/components/graph/graphHelpers.ts +++ b/apps/desktop/src/renderer/components/graph/graphHelpers.ts @@ -153,6 +153,17 @@ export function prCiDotColor(pr: GraphPrOverlay): string { return getPrCiDotColor({ checksStatus: pr.checksStatus, ciRunning: pr.pendingCheckCount > 0 }); } +/** + * Human label for a checks rollup in graph tooltips. Exists so `not_run` never + * reaches a user as the raw enum, and so it reads as absence ("not run") + * rather than as a state that could be mistaken for a result. + */ +export function prChecksLabel(status: GraphPrOverlay["checksStatus"]): string { + if (status === "not_run") return "not run"; + if (status === "none") return "none"; + return status; +} + export function iconGlyph(icon: LaneIcon): React.ReactNode { const match = ICON_OPTIONS.find((opt) => opt.key === icon); return match?.icon ?? null; diff --git a/apps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsx b/apps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsx index 4f58c2e83..aa3b7aa7a 100644 --- a/apps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsx +++ b/apps/desktop/src/renderer/components/graph/graphNodes/LaneNode.tsx @@ -72,6 +72,10 @@ export function GraphLaneNode({ data, selected }: NodeProps> if (pr.state === "closed") return { label: `Closed PR #${pr.number}`, className: "text-muted-fg" }; if (pr.reviewStatus === "changes_requested") return { label: `PR #${pr.number} needs changes`, className: "text-amber-300" }; if (pr.checksStatus === "failing") return { label: `PR #${pr.number} checks failing`, className: "text-red-300" }; + // ADE-135: without this, an approved PR whose CI never ran fell past the + // "ready" test into the generic sky-blue "open" badge — the absence became + // invisible. It is muted, not red: nothing failed, nothing verified. + if (pr.checksStatus === "not_run") return { label: `PR #${pr.number} CI not run`, className: "text-muted-fg" }; if (pr.reviewStatus === "approved" && pr.checksStatus === "passing") return { label: `PR #${pr.number} ready`, className: "text-emerald-300" }; if (pr.pendingCheckCount > 0) return { label: `PR #${pr.number} checks running`, className: "text-sky-300" }; return { label: `PR #${pr.number} open`, className: "text-sky-300" }; diff --git a/apps/desktop/src/renderer/components/graph/graphPrData.ts b/apps/desktop/src/renderer/components/graph/graphPrData.ts index d41466a39..34d875b4c 100644 --- a/apps/desktop/src/renderer/components/graph/graphPrData.ts +++ b/apps/desktop/src/renderer/components/graph/graphPrData.ts @@ -57,6 +57,10 @@ export function buildGraphPrOverlay(args: { url: pr.githubUrl, state: liveStatus?.state ?? pr.state, checksStatus: liveStatus?.checksStatus ?? pr.checksStatus, + // Reason travels with the status it explains: taking one from the live + // status and the other from the stored summary would caption a `not_run` + // with a stale sentence about a state it no longer is. + checksReason: (liveStatus ? liveStatus.checksReason : pr.checksReason) ?? null, reviewStatus: liveStatus?.reviewStatus ?? pr.reviewStatus, lastSyncedAt: pr.lastSyncedAt ?? null, lastActivityAt, diff --git a/apps/desktop/src/renderer/components/graph/graphTypes.ts b/apps/desktop/src/renderer/components/graph/graphTypes.ts index 3560f9d81..fff10127c 100644 --- a/apps/desktop/src/renderer/components/graph/graphTypes.ts +++ b/apps/desktop/src/renderer/components/graph/graphTypes.ts @@ -67,6 +67,8 @@ export type GraphPrOverlay = { url: string; state: PrState; checksStatus: PrStatus["checksStatus"]; + /** Rollup explanation for a non-obvious `checksStatus` (see PrStatus). */ + checksReason: string | null; reviewStatus: PrReviewStatus; lastSyncedAt: string | null; lastActivityAt: string | null; diff --git a/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx b/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx index 6be41095f..67c280c70 100644 --- a/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx +++ b/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx @@ -44,6 +44,11 @@ function checksCaption(status: PrChecksStatus): string { return "Checks failing"; case "pending": return "Checks running"; + // ADE-135: "not_run" is a finding — checks were expected and nothing + // verified the commit — where "none" is the quiet no-CI-here case. They + // share the muted dot, so only the caption tells them apart. + case "not_run": + return "CI not run"; default: return "No checks"; } @@ -169,7 +174,18 @@ export function LanePrBadgePopover({ ); })()} - + {checksCaption(pr.checksStatus!)} diff --git a/apps/desktop/src/renderer/components/lanes/lanePageModel.ts b/apps/desktop/src/renderer/components/lanes/lanePageModel.ts index 27ffe8c8b..97db9604d 100644 --- a/apps/desktop/src/renderer/components/lanes/lanePageModel.ts +++ b/apps/desktop/src/renderer/components/lanes/lanePageModel.ts @@ -38,6 +38,8 @@ export type LaneTabPrTag = { baseBranch?: string | null; headBranch?: string | null; checksStatus?: PrChecksStatus; + /** Why the rollup landed where it did; only set for non-obvious states. */ + checksReason?: string | null; reviewStatus?: PrReviewStatus; additions?: number; deletions?: number; @@ -286,6 +288,7 @@ function toLaneTabPrTagFromPrSummary(pr: PrSummary): LaneTabPrTag { baseBranch: pr.baseBranch, headBranch: pr.headBranch, checksStatus: pr.checksStatus, + checksReason: pr.checksReason ?? null, reviewStatus: pr.reviewStatus, additions: pr.additions, deletions: pr.deletions, @@ -328,6 +331,7 @@ function mergeLaneTabPrTags(base: LaneTabPrTag, secondary: LaneTabPrTag | null): baseBranch: base.baseBranch ?? secondary.baseBranch, headBranch: base.headBranch ?? secondary.headBranch, checksStatus: base.checksStatus ?? secondary.checksStatus, + checksReason: base.checksReason ?? secondary.checksReason ?? null, reviewStatus: base.reviewStatus ?? secondary.reviewStatus, additions: base.additions ?? secondary.additions, deletions: base.deletions ?? secondary.deletions, diff --git a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx index 9f83e7194..21f329efe 100644 --- a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { ArrowSquareOut, ChatText, CheckCircle, GitBranch, XCircle } from "@phosphor-icons/react"; +import { ArrowSquareOut, ChatText, CheckCircle, CircleDashed, GitBranch, XCircle } from "@phosphor-icons/react"; import type { GitHubPrListItem, PrSummary } from "../../../../shared/types/prs"; import { COLORS, MONO_FONT, SANS_FONT, inlineBadge } from "../../lanes/laneDesignTokens"; @@ -50,7 +50,16 @@ function stateBadgeStyle(item: GitHubPrListItem): React.CSSProperties { }; } -function PrRowCiStatus({ status }: { status: PrSummary["checksStatus"] | null }) { +/** Copy used when the producer had no more specific reason to offer. */ +const NO_CI_REASON = "No CI has run on this commit."; + +function PrRowCiStatus({ + status, + reason, +}: { + status: PrSummary["checksStatus"] | null; + reason?: string | null; +}) { switch (status) { case "passing": return ( @@ -70,6 +79,19 @@ function PrRowCiStatus({ status }: { status: PrSummary["checksStatus"] | null }) ); + // ADE-135: nothing verified this commit. The glyph is a hollow dashed ring + // in the muted tone — an empty slot where a result should be, deliberately + // not the danger colour: this is absence, not failure. + case "not_run": + return ( + + + + ); default: return null; } @@ -391,7 +413,9 @@ export function GitHubTabPrRow({ }}> {item.title} - {terminal ? null : } + {terminal ? null : ( + + )} {ago ? ( {ago} diff --git a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx index fdf045867..142ac910c 100644 --- a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx @@ -153,4 +153,28 @@ describe("PrChecksCard summary + bucketing", () => { fireEvent.click(screen.getByRole("button", { name: "Re-run CI / e2e" })); expect(onRerunChecks).toHaveBeenCalledWith({ actionJobIds: [77] }); }); + + // ADE-135: a required job that never reported is the finding. It has to be + // visible in the same list as the results that did arrive, in GitHub's order. + it("renders missing required contexts as ghost rows ahead of real checks", () => { + render( + , + ); + + const ghosts = screen.getAllByTestId("pr-checks-card-ghost-row"); + expect(ghosts.map((row) => (row.textContent ?? "").trim())).toEqual([ + "CI / buildrequired · not reported", + "CI / lintrequired · not reported", + ]); + expect(rowNames()).toEqual(["e2e"]); + }); + + it("renders ghost rows even when nothing at all reported", () => { + render(); + expect(screen.getAllByTestId("pr-checks-card-ghost-row")).toHaveLength(1); + }); }); diff --git a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx index db8a7be2f..e0294ce4e 100644 --- a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx @@ -3,6 +3,7 @@ import { ArrowClockwise, ArrowSquareOut, CheckCircle, + CircleDashed, CircleNotch, MinusCircle, XCircle, @@ -34,6 +35,13 @@ export type PrChecksCardProps = { * done by `buildUnifiedChecks`). */ fill?: boolean; + /** + * ADE-135: required contexts that never reported on this commit, in the order + * GitHub declared them. Rendered as dimmed placeholder rows in the same list + * as the real checks so a job that is simply absent reads as an unfilled slot + * rather than as nothing at all. + */ + missingRequired?: readonly string[] | null; }; type Bucket = "pass" | "fail" | "pending" | "skip"; @@ -91,7 +99,10 @@ export const PrChecksCard = memo(function PrChecksCard({ onRerunChecks, actionBusy = false, fill = false, + missingRequired, }: PrChecksCardProps) { + // Order is meaningful (GitHub's declaration order), so this is never sorted. + const ghosts = missingRequired ?? []; const items = useMemo(() => buildUnifiedChecks(checks, actionRuns), [checks, actionRuns]); const { passing, failing, pending, total } = useMemo(() => { @@ -145,12 +156,36 @@ export const PrChecksCard = memo(function PrChecksCard({ ) : null}
- {attention.length > 0 ? ( + {attention.length > 0 || ghosts.length > 0 ? (
+ {/* Ghosts lead the list: a slot that was never filled outranks the + results that did arrive. */} + {ghosts.map((context) => ( +
+ + + {context} + + + required · not reported + +
+ ))} {attention.map((item) => { const bucket = bucketOf(item); const rerunTarget: PrRerunChecksTarget | null = item.source === "actions_job" && item.jobId != null diff --git a/apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx b/apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx index cc174454c..76c238917 100644 --- a/apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/PrDetailRightMetadataRail.tsx @@ -441,6 +441,7 @@ export const PrDetailRightMetadataRail = memo(function PrDetailRightMetadataRail this column, so they take the slack and scroll internally. */} { afterEach(() => { @@ -64,6 +70,15 @@ describe("prVisuals", () => { expect(getPrEdgeColor({ state: "open", checksStatus: "passing", reviewStatus: "changes_requested", ciRunning: true })).toBe(COLORS.danger); }); + // ADE-135: an approved PR whose commit nothing verified used to inherit the + // success edge from the approval alone, which is exactly the "CI passed" + // illusion this ticket exists to kill. + it("never paints a not_run PR green", () => { + expect(getPrEdgeColor({ state: "open", checksStatus: "not_run", reviewStatus: "approved" })).toBe(COLORS.textMuted); + expect(getPrCiDotColor({ checksStatus: "not_run" })).toBe(COLORS.textMuted); + expect(getPrChecksBadge("not_run").color).toBe(COLORS.textMuted); + }); + describe("formatCompactCount", () => { it("returns the number as a string for values under 1000", () => { expect(formatCompactCount(0)).toBe("0"); diff --git a/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx b/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx index b57fe8189..a7e6fdced 100644 --- a/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx @@ -27,6 +27,10 @@ export function getPrChecksBadge(status: PrChecksStatus): PrBadgeSpec { if (status === "passing") return { label: "CI", ...colorBadge(COLORS.success) }; if (status === "failing") return { label: "CI", ...colorBadge(COLORS.danger) }; if (status === "pending") return { label: "CI", ...colorBadge(COLORS.warning) }; + // ADE-135: `not_run` is absence, not failure — nothing verified the commit, + // so it reads muted like the quiet default rather than borrowing the danger + // colour. It is separated from `none` only so the branch is explicit here. + if (status === "not_run") return { label: "CI", ...colorBadge(COLORS.textMuted) }; return { label: "CI", ...colorBadge(COLORS.textMuted) }; } @@ -49,6 +53,9 @@ export function getPrEdgeColor(args: { if (args.ciRunning || args.checksStatus === "pending") return COLORS.info; if (args.reviewStatus === "requested" || args.reviewStatus === "none") return COLORS.warning; if (args.checksStatus === "failing") return COLORS.danger; + // ADE-135: an approved PR whose commit nothing verified must not wear the + // success edge — the approval is real, the verification is not. + if (args.checksStatus === "not_run") return COLORS.textMuted; if (args.checksStatus === "passing" || args.reviewStatus === "approved") return COLORS.success; return COLORS.textMuted; } @@ -59,6 +66,7 @@ export function getPrCiDotColor(args: { }): string { if (args.ciRunning || args.checksStatus === "pending") return COLORS.info; if (args.checksStatus === "failing") return COLORS.danger; + if (args.checksStatus === "not_run") return COLORS.textMuted; if (args.checksStatus === "passing") return COLORS.success; return COLORS.textMuted; } diff --git a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx index a4d3acdb0..c5ea18a91 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/GitHubTabRowsAndMapping.test.tsx @@ -8,6 +8,7 @@ import type { CreateLaneFromPrBranchResult, CreateLaneFromPrBranchPreflightResult, GitHubPrSnapshot, + PrSummary, } from "../../../../shared/types"; vi.mock("react-resizable-panels", async () => { @@ -83,6 +84,34 @@ describe("GitHubTab rows and mapping", () => { expect(onSelect).toHaveBeenCalledTimes(1); }); + // ADE-135: `not_run` means nothing verified the commit. The row must show the + // hollow ring and say why, rather than rendering nothing (which is how three + // bot successes came to read as "CI passed"). + it("shows a hollow ring carrying the rollup reason when no CI ran", () => { + const item = makeGitHubPr({}); + const linkedPr = { + checksStatus: "not_run", + checksReason: "No CI has run on this commit — 3 apps reported, none of them CI.", + reviewStatus: "approved", + } as unknown as PrSummary; + + render(); + + expect(screen.getByLabelText("No CI has run").title).toBe( + "No CI has run on this commit — 3 apps reported, none of them CI.", + ); + }); + + it("falls back to generic copy when the rollup gave no reason", () => { + const linkedPr = { checksStatus: "not_run", checksReason: null } as unknown as PrSummary; + + render( + , + ); + + expect(screen.getByLabelText("No CI has run").title).toBe("No CI has run on this commit."); + }); + it("shows a running CI indicator for PR cards with pending checks", async () => { renderTab(); diff --git a/apps/desktop/src/renderer/components/terminals/useLanePrs.ts b/apps/desktop/src/renderer/components/terminals/useLanePrs.ts index a9c690415..ee5ff9e61 100644 --- a/apps/desktop/src/renderer/components/terminals/useLanePrs.ts +++ b/apps/desktop/src/renderer/components/terminals/useLanePrs.ts @@ -21,6 +21,9 @@ function githubItemToLanePr(item: GitHubPrListItem, laneId: string): PrSummary { state: item.isDraft ? "draft" : item.state, baseBranch: item.baseBranch ?? "", headBranch: item.headBranch ?? "", + // "none", not "not_run": the GitHub list endpoint carries no check data at + // all, so we have observed nothing rather than observed an absence. Only a + // rollup that actually looked at the commit may claim "not_run" (ADE-135). checksStatus: "none", reviewStatus: "none", additions: 0, diff --git a/apps/desktop/src/shared/__fixtures__/pr988CheckRuns.ts b/apps/desktop/src/shared/__fixtures__/pr988CheckRuns.ts new file mode 100644 index 000000000..e144323a0 --- /dev/null +++ b/apps/desktop/src/shared/__fixtures__/pr988CheckRuns.ts @@ -0,0 +1,37 @@ +/** + * The check-runs GitHub returned for PR #988 (`ade/pr-merged-lane-mapping`) at + * head `d56fbb420` — the payload that made ADE render "CI passed · 3 jobs". + * + * Provenance: `Vercel Preview Comments` (success) and `Mintlify Deployment` + * (skipped) were recovered live from + * `GET /repos/arul28/ADE/commits/d56fbb420/check-runs`; GitHub had already + * pruned the rest by the time this was written. The remaining rows are + * transcribed from the ADE-135 report, which recorded `gh pr checks 988` in + * full: + * + * CodeRabbit pass 0 "Review rate limited" + * Vercel pass 0 "Canceled by Ignored Build Step" + * Vercel Preview Comments pass 0 + * + * The essential facts this fixture preserves: + * - Three checks report `success`, and not one of them verified the code. + * CodeRabbit was rate-limited, so its review never happened. Vercel was + * cancelled by an ignored-build step, so its build never ran. + * - `github-actions` registered NO check run at all. Every required job in + * ci.yml — install, test-desktop 1-8, test-ade-cli, and the ci-pass gate — + * is absent, not failing. + * - Absence is exactly what the old rollup could not represent. + */ +export const PR988_CHECK_RUNS = [ + { name: "CodeRabbit", status: "completed", conclusion: "success", appSlug: "coderabbitai" }, + { name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }, + { name: "Vercel Preview Comments", status: "completed", conclusion: "success", appSlug: "vercel" }, + { name: "Mintlify Deployment", status: "completed", conclusion: "skipped", appSlug: "mintlify" }, +] as const; + +/** + * `main`'s required contexts, read live from + * `GET /repos/arul28/ADE/rules/branches/main` — the rulesets tier, which needs + * only read access. None of these reported on #988's head. + */ +export const ADE_MAIN_REQUIRED_CONTEXTS = ["ci-pass"] as const; diff --git a/apps/desktop/src/shared/prChecksRollup.test.ts b/apps/desktop/src/shared/prChecksRollup.test.ts new file mode 100644 index 000000000..20faa5b39 --- /dev/null +++ b/apps/desktop/src/shared/prChecksRollup.test.ts @@ -0,0 +1,237 @@ +import { describe, expect, it } from "vitest"; +import { rollupChecks, isCiProducerAppSlug, CI_PENDING_GRACE_MS } from "./prChecksRollup"; +import type { ChecksRollupCheckRun, ChecksRollupInput } from "./prChecksRollup"; +import { ADE_MAIN_REQUIRED_CONTEXTS, PR988_CHECK_RUNS } from "./__fixtures__/pr988CheckRuns"; + +function input(overrides: Partial = {}): ChecksRollupInput { + return { + checkRuns: [], + commitStatuses: [], + requiredContexts: null, + requiredSource: "unavailable", + mergeStateBlocked: false, + headCommitAgeMs: null, + ...overrides, + }; +} + +const actionsRun = ( + name: string, + conclusion: string | null, + status = "completed", +): ChecksRollupCheckRun => ({ name, status, conclusion, appSlug: "github-actions" }); + +describe("rollupChecks — ADE-135 regression", () => { + it("does not report PR #988 as passing", () => { + // The reported bug, verbatim: three third-party successes, zero Actions + // runs, rendered as "CI passed · 3 jobs". + const result = rollupChecks(input({ checkRuns: [...PR988_CHECK_RUNS] })); + + expect(result.status).not.toBe("passing"); + expect(result.status).toBe("not_run"); + expect(result.reason).toContain("none from a CI provider"); + }); + + it("names the required check that never ran on #988", () => { + // `main` requires `ci-pass`, and nothing on that head reported it. + const result = rollupChecks( + input({ + checkRuns: [...PR988_CHECK_RUNS], + requiredContexts: [...ADE_MAIN_REQUIRED_CONTEXTS], + requiredSource: "rulesets", + headCommitAgeMs: 6 * 60 * 60 * 1000, + }), + ); + + expect(result.status).not.toBe("passing"); + expect(result.missingRequiredContexts).toEqual(["ci-pass"]); + expect(result.reason).toContain("ci-pass"); + }); +}); + +describe("rollupChecks — skipped and neutral are not success", () => { + it("does not go green when every CI check was skipped", () => { + const result = rollupChecks( + input({ checkRuns: [actionsRun("build", "skipped"), actionsRun("test", "skipped")] }), + ); + + expect(result.status).toBe("not_run"); + expect(result.reason).toContain("skipped"); + }); + + it("does not go green on neutral alone", () => { + const result = rollupChecks(input({ checkRuns: [actionsRun("lint", "neutral")] })); + expect(result.status).not.toBe("passing"); + }); + + it("still goes green when a skipped check sits beside a real pass", () => { + // Skipping an irrelevant job is normal and must not block a genuine green. + const result = rollupChecks( + input({ checkRuns: [actionsRun("test", "success"), actionsRun("deploy", "skipped")] }), + ); + + expect(result.status).toBe("passing"); + expect(result.reason).toBeNull(); + }); +}); + +describe("rollupChecks — producer awareness", () => { + it("treats a legacy commit status as CI", () => { + // Jenkins/Buildkite/CircleCI report this way and are genuinely CI. + const result = rollupChecks( + input({ commitStatuses: [{ context: "buildkite/build", state: "success" }] }), + ); + + expect(result.status).toBe("passing"); + }); + + it("never lets a third-party app carry a green on its own", () => { + const result = rollupChecks( + input({ checkRuns: [{ name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }] }), + ); + + expect(result.status).toBe("not_run"); + }); + + it("treats an unknown producer as non-CI rather than assuming the best", () => { + const result = rollupChecks( + input({ checkRuns: [{ name: "mystery", status: "completed", conclusion: "success", appSlug: null }] }), + ); + + expect(result.status).toBe("not_run"); + }); + + it("recognizes only github-actions as a CI app slug", () => { + expect(isCiProducerAppSlug("github-actions")).toBe(true); + expect(isCiProducerAppSlug("GitHub-Actions")).toBe(true); + expect(isCiProducerAppSlug("vercel")).toBe(false); + expect(isCiProducerAppSlug(null)).toBe(false); + }); +}); + +describe("rollupChecks — required contexts", () => { + it("holds back a green when a required context never reported", () => { + const result = rollupChecks( + input({ + checkRuns: [actionsRun("install", "success")], + requiredContexts: ["install", "ci-pass"], + requiredSource: "rulesets", + }), + ); + + expect(result.status).toBe("pending"); + expect(result.missingRequiredContexts).toEqual(["ci-pass"]); + }); + + it("goes green once every required context has reported", () => { + const result = rollupChecks( + input({ + checkRuns: [actionsRun("install", "success"), actionsRun("ci-pass", "success")], + requiredContexts: ["install", "ci-pass"], + requiredSource: "rulesets", + }), + ); + + expect(result.status).toBe("passing"); + }); + + it("keeps required order rather than sorting", () => { + const result = rollupChecks( + input({ + requiredContexts: ["test-desktop (2)", "install", "test-desktop (1)"], + requiredSource: "rulesets", + }), + ); + + expect(result.missingRequiredContexts).toEqual([ + "test-desktop (2)", + "install", + "test-desktop (1)", + ]); + }); + + it("reports a real failure ahead of a missing required check", () => { + // The actionable fact is the red job, not the absent one. + const result = rollupChecks( + input({ + checkRuns: [actionsRun("test", "failure")], + requiredContexts: ["ci-pass"], + requiredSource: "rulesets", + }), + ); + + expect(result.status).toBe("failing"); + }); + + it("treats unreadable required contexts as unknown, not as none required", () => { + // A restrictive token must not manufacture certainty in either direction. + const result = rollupChecks( + input({ + checkRuns: [actionsRun("test", "success")], + requiredContexts: null, + requiredSource: "unavailable", + }), + ); + + expect(result.status).toBe("passing"); + expect(result.missingRequiredContexts).toEqual([]); + }); +}); + +describe("rollupChecks — quiet when nothing is expected", () => { + it("stays `none` for a repo with no CI and no signal", () => { + // Not every repo has CI; inventing a warning for them would be noise. + expect(rollupChecks(input()).status).toBe("none"); + }); + + it("escalates to not_run when GitHub says the merge is blocked", () => { + const result = rollupChecks(input({ mergeStateBlocked: true })); + + expect(result.status).toBe("not_run"); + expect(result.reason).toContain("blocked"); + }); + + it("never downgrades a genuine pass on mergeStateBlocked", () => { + // `blocked` also means "needs review", so it must not touch a green. + const result = rollupChecks( + input({ checkRuns: [actionsRun("test", "success")], mergeStateBlocked: true }), + ); + + expect(result.status).toBe("passing"); + }); +}); + +describe("rollupChecks — age-aware wording", () => { + it("says CI has not run *yet* on a fresh commit", () => { + const result = rollupChecks( + input({ + checkRuns: [{ name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }], + headCommitAgeMs: 30_000, + }), + ); + + expect(result.reason).toContain("not run yet"); + }); + + it("drops the hedge once the commit is old enough that CI should have started", () => { + const result = rollupChecks( + input({ + checkRuns: [{ name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }], + headCommitAgeMs: CI_PENDING_GRACE_MS + 1, + }), + ); + + expect(result.reason).toContain("not run on this commit"); + expect(result.reason).not.toContain("yet"); + }); +}); + +describe("rollupChecks — in-flight", () => { + it("reports pending while an Actions job is still running", () => { + const result = rollupChecks( + input({ checkRuns: [actionsRun("test", null, "in_progress"), actionsRun("install", "success")] }), + ); + + expect(result.status).toBe("pending"); + }); +}); diff --git a/apps/desktop/src/shared/prChecksRollup.ts b/apps/desktop/src/shared/prChecksRollup.ts new file mode 100644 index 000000000..133362688 --- /dev/null +++ b/apps/desktop/src/shared/prChecksRollup.ts @@ -0,0 +1,248 @@ +import type { PrChecksStatus } from "./types/prs"; +import { pipelineStateOf, worstPipelineState } from "./prPipelineState"; +import type { PrPipelineState } from "./types/prs"; + +/** + * Canonical PR checks rollup. + * + * ADE-135: this used to be `toChecksStatusFromCheckRuns` inside prService, and + * it answered "did anything succeed?" rather than "was this code verified?". + * On PR #988 three third-party apps reported `success` — CodeRabbit while + * rate-limited, Vercel while cancelled by an ignored-build step, and a comment + * bot — GitHub Actions registered no suite at all, and the card read + * "CI passed · 3 jobs". Absence rendered as success. + * + * Three rules now govern a green rollup: + * + * 1. State mapping is delegated to `prPipelineState`, so `skipped`/`neutral` + * can no longer masquerade as success and the rollup can no longer + * disagree with the per-job rows rendered beneath it. + * 2. Green requires a *CI producer* — GitHub Actions or a legacy commit + * status. Preview/review/comment apps still render, but cannot carry a + * green on their own. + * 3. Required contexts that never reported hold the rollup back, so a missing + * job is visible rather than silently absent. + * + * When none of that can be established we return `not_run`, which is a claim + * about our own knowledge rather than about the code. + */ + +/** GitHub App slugs whose successes constitute actual verification. */ +const CI_PRODUCER_APP_SLUGS = new Set(["github-actions"]); + +/** + * Below this age a missing CI run is more likely "hasn't started" than + * "never happened", and the copy softens accordingly. GitHub typically + * registers a check suite within seconds; five minutes is generous. + */ +export const CI_PENDING_GRACE_MS = 5 * 60 * 1000; + +export type ChecksRollupCheckRun = { + name: string; + status: string; + conclusion: string | null; + /** `app.slug` from the check-runs payload; null when GitHub omitted it. */ + appSlug: string | null; +}; + +export type ChecksRollupCommitStatus = { + context: string; + /** Legacy commit-status state: success | failure | error | pending. */ + state: string; +}; + +/** Which tier of the required-context lookup answered. */ +export type RequiredContextSource = + | "rulesets" + | "branch_protection" + | "merge_state" + | "unavailable"; + +export type ChecksRollupInput = { + checkRuns: readonly ChecksRollupCheckRun[]; + commitStatuses: readonly ChecksRollupCommitStatus[]; + /** + * Required check contexts for the base branch, or null when no credential + * we hold could read them. Null means "unknown", never "none required". + */ + requiredContexts: readonly string[] | null; + requiredSource: RequiredContextSource; + /** + * GraphQL `mergeStateStatus === "blocked"`. Corroborating only: it conflates + * missing checks with review-required and out-of-date branches, so it may + * strengthen a not-run finding but must never downgrade a genuine pass. + */ + mergeStateBlocked: boolean; + /** Age of the head commit, used only to choose between "yet" and "never". */ + headCommitAgeMs: number | null; +}; + +export type ChecksRollup = { + status: PrChecksStatus; + /** + * One sentence explaining a non-obvious rollup, surfaced in the row tooltip, + * the chat card subtitle, and support triage. Null when the state speaks for + * itself (a clean pass, a plain failure). + */ + reason: string | null; + /** Required contexts with no observed run, in the order the API declared. */ + missingRequiredContexts: string[]; +}; + +/** + * Shared so every surface groups by the same notion of "real CI producer". + * The chat card (`prChatCards.ts`) splits its rows with this predicate, and a + * second copy of the slug list is exactly how the rollup and the card would + * drift back into disagreeing about what "CI" means. + */ +export function isCiProducerAppSlug(slug: string | null | undefined): boolean { + const value = (slug ?? "").trim().toLowerCase(); + // A missing slug is treated as non-CI: GitHub always populates it for + // Actions, so an absent one means we cannot vouch for the producer. + return CI_PRODUCER_APP_SLUGS.has(value); +} + +/** + * Sentinel `appSlug` prService stamps on legacy combined-status contexts, which + * are produced by no GitHub App at all. + */ +export const COMMIT_STATUS_APP_SLUG = "commit_status"; + +/** + * CI membership for a flattened `PrCheck` row, where check runs and legacy + * commit statuses arrive in one list. + * + * The rollup keeps those two apart (`checkRuns` vs `commitStatuses`) and counts + * both as CI producers — Jenkins, Buildkite and CircleCI still report through + * the commit-status API. Surfaces that only have the merged list use this. + * Anything unattributed stays out: an unknown producer cannot carry a green. + */ +export function isCiProducerCheck(appSlug: string | null | undefined): boolean { + const value = (appSlug ?? "").trim().toLowerCase(); + return value === COMMIT_STATUS_APP_SLUG || isCiProducerAppSlug(value); +} + +function isCiProducer(run: ChecksRollupCheckRun): boolean { + return isCiProducerAppSlug(run.appSlug); +} + +function commitStatusState(state: string): PrPipelineState { + const value = state.trim().toLowerCase(); + if (value === "success") return "passed"; + if (value === "failure" || value === "error") return "failed"; + if (value === "pending") return "running"; + return "unknown"; +} + +function pluralize(count: number, singular: string, plural: string): string { + return `${count} ${count === 1 ? singular : plural}`; +} + +function formatContexts(contexts: readonly string[], limit = 3): string { + if (contexts.length <= limit) return contexts.join(", "); + const shown = contexts.slice(0, limit).join(", "); + return `${shown}, +${contexts.length - limit} more`; +} + +/** + * Derive the rollup. Pure: every input is supplied by the caller so both the + * webhook and polling paths, and the tests, exercise identical logic. + */ +export function rollupChecks(input: ChecksRollupInput): ChecksRollup { + const ciRuns = input.checkRuns.filter(isCiProducer); + const otherRuns = input.checkRuns.filter((run) => !isCiProducer(run)); + + const ciStates: PrPipelineState[] = [ + ...ciRuns.map((run) => + pipelineStateOf({ + status: + run.status === "queued" || run.status === "in_progress" || run.status === "completed" + ? run.status + : "completed", + conclusion: run.conclusion, + }), + ), + ...input.commitStatuses.map((status) => commitStatusState(status.state)), + ]; + + const observedContexts = new Set([ + ...input.checkRuns.map((run) => run.name.trim()).filter(Boolean), + ...input.commitStatuses.map((status) => status.context.trim()).filter(Boolean), + ]); + const missingRequiredContexts = (input.requiredContexts ?? []).filter( + (context) => !observedContexts.has(context.trim()), + ); + + const ciProducerCount = ciStates.length; + const hasFailure = ciStates.some((state) => state === "failed"); + const hasInFlight = ciStates.some((state) => state === "running" || state === "queued"); + const hasPass = ciStates.some((state) => state === "passed"); + const worst = worstPipelineState(ciStates); + + // A real failure outranks everything, including missing required checks: + // the actionable fact is the red job, not the absent one. + if (hasFailure) { + return { status: "failing", reason: null, missingRequiredContexts }; + } + + if (hasInFlight) { + return { + status: "pending", + reason: + missingRequiredContexts.length > 0 + ? `Waiting on ${pluralize(missingRequiredContexts.length, "required check", "required checks")}: ${formatContexts(missingRequiredContexts)}.` + : null, + missingRequiredContexts, + }; + } + + // Required checks are known and some never reported. Something is expected + // that has not arrived, so the rollup stays open rather than going green. + if (missingRequiredContexts.length > 0) { + const stale = (input.headCommitAgeMs ?? 0) >= CI_PENDING_GRACE_MS; + return { + status: hasPass || ciProducerCount > 0 ? "pending" : "not_run", + reason: `${pluralize(missingRequiredContexts.length, "required check has", "required checks have")} not reported${stale ? "" : " yet"}: ${formatContexts(missingRequiredContexts)}.`, + missingRequiredContexts, + }; + } + + if (hasPass) { + return { status: "passing", reason: null, missingRequiredContexts }; + } + + // CI producers reported, but every one of them was skipped or neutral. + // Nothing was verified, so this is not a pass. + if (ciProducerCount > 0) { + return { + status: "not_run", + reason: + worst === "skipped" + ? `Every CI check was skipped, so nothing verified this commit.` + : `No CI check reported a result for this commit.`, + missingRequiredContexts, + }; + } + + // No CI producer at all. If other apps reported, or GitHub says the merge is + // blocked, then something was expected here and its absence is the finding. + const stale = (input.headCommitAgeMs ?? 0) >= CI_PENDING_GRACE_MS; + if (otherRuns.length > 0) { + return { + status: "not_run", + reason: `${pluralize(otherRuns.length, "check", "checks")} reported, none from a CI provider. CI has ${stale ? "not run on this commit" : "not run yet"}.`, + missingRequiredContexts, + }; + } + if (input.mergeStateBlocked) { + return { + status: "not_run", + reason: `No CI reported on this commit, and GitHub reports the merge as blocked.`, + missingRequiredContexts, + }; + } + + // Genuinely nothing anywhere, and nothing told us to expect anything. Stay + // quiet rather than inventing a warning for repos that simply have no CI. + return { status: "none", reason: null, missingRequiredContexts }; +} diff --git a/apps/desktop/src/shared/types/prs.ts b/apps/desktop/src/shared/types/prs.ts index 28b8755a6..c342f8441 100644 --- a/apps/desktop/src/shared/types/prs.ts +++ b/apps/desktop/src/shared/types/prs.ts @@ -14,7 +14,13 @@ import type { GitHubRepoRef } from "./git"; import type { LaneSummary, RebaseTargetCommit } from "./lanes"; export type PrState = "draft" | "open" | "merged" | "closed"; -export type PrChecksStatus = "pending" | "passing" | "failing" | "none"; +/** + * `none` means we observed nothing at all and nothing led us to expect + * anything — a repo without CI stays quiet. `not_run` is the ADE-135 state: + * checks exist, or required contexts are known, but nothing actually verified + * the commit. The two are distinct because only the second is a finding. + */ +export type PrChecksStatus = "pending" | "passing" | "failing" | "none" | "not_run"; export type PrReviewStatus = "none" | "requested" | "approved" | "changes_requested"; export type MergeMethod = "merge" | "squash" | "rebase"; export type PrNotificationKind = @@ -68,6 +74,14 @@ export type PrSummary = { baseBranch: string; headBranch: string; checksStatus: PrChecksStatus; + /** + * One sentence explaining a non-obvious rollup, e.g. "3 checks reported, none + * from a CI provider." Null when the state speaks for itself. Every surface + * reads this instead of re-deriving the explanation. + */ + checksReason?: string | null; + /** Required contexts that never reported, in the order GitHub declared them. */ + checksMissingRequired?: string[] | null; reviewStatus: PrReviewStatus; additions: number; deletions: number; @@ -125,6 +139,10 @@ export type PrStatus = { prId: string; state: PrState; checksStatus: PrChecksStatus; + /** See `PrSummary.checksReason`. */ + checksReason?: string | null; + /** See `PrSummary.checksMissingRequired`. */ + checksMissingRequired?: string[] | null; reviewStatus: PrReviewStatus; isMergeable: boolean; mergeConflicts: boolean; @@ -171,6 +189,13 @@ export type PrCheck = { detailsUrl: string | null; startedAt: string | null; completedAt: string | null; + /** + * ADE-135: `app.slug` of the GitHub App that produced this check run, so + * surfaces can tell a CI job from a preview/review bot. `"commit_status"` for + * legacy combined-status contexts, which are CI by convention. Null when + * GitHub omitted it or the row predates this field. + */ + appSlug?: string | null; }; export type PrReview = { diff --git a/apps/ios/ADE/Models/RemoteModels.swift b/apps/ios/ADE/Models/RemoteModels.swift index 1bdfb9eb0..346a8aaa9 100644 --- a/apps/ios/ADE/Models/RemoteModels.swift +++ b/apps/ios/ADE/Models/RemoteModels.swift @@ -4018,6 +4018,12 @@ struct PrSummary: Codable, Identifiable, Equatable { var creationStrategy: String? = nil /// Native GitHub stack membership. Nil against hosts before stacked PR support. var stack: GitHubPrStackMembership? = nil + /// ADE-135. One sentence explaining a non-obvious checks rollup, e.g. "3 checks + /// reported, none from a CI provider." Nil when the state speaks for itself, and + /// on hosts that predate the rollup. + var checksReason: String? = nil + /// Required check contexts that never reported, in the order GitHub declared them. + var checksMissingRequired: [String]? = nil } struct PullRequestListItem: Codable, Identifiable, Equatable { @@ -4055,6 +4061,10 @@ struct PullRequestListItem: Codable, Identifiable, Equatable { var mergeMethod: String? = nil var commitCount: Int? = nil var changedFiles: Int? = nil + /// See `PrSummary.checksReason`. + var checksReason: String? = nil + /// See `PrSummary.checksMissingRequired`. + var checksMissingRequired: [String]? = nil } struct PrGroupMemberSummary: Codable, Identifiable, Equatable { @@ -4113,6 +4123,10 @@ struct PrStatus: Codable, Equatable { var prId: String var state: String var checksStatus: String + /// See `PrSummary.checksReason`. + var checksReason: String? + /// See `PrSummary.checksMissingRequired`. + var checksMissingRequired: [String]? var reviewStatus: String var isMergeable: Bool var mergeConflicts: Bool @@ -4136,7 +4150,8 @@ struct PrStatus: Codable, Equatable { /// new GitHub enum value never fails the whole snapshot decode. All other /// fields use synthesized decoding via `decodeIfPresent` semantics. private enum CodingKeys: String, CodingKey { - case prId, state, checksStatus, reviewStatus, isMergeable, mergeConflicts, behindBaseBy + case prId, state, checksStatus, checksReason, checksMissingRequired + case reviewStatus, isMergeable, mergeConflicts, behindBaseBy case mergeStateStatus, reviewDecision, approvalsCount, requiredApprovals case mergeabilityComputing, canBypass, headSha } @@ -4146,6 +4161,9 @@ struct PrStatus: Codable, Equatable { prId = try c.decode(String.self, forKey: .prId) state = try c.decode(String.self, forKey: .state) checksStatus = try c.decode(String.self, forKey: .checksStatus) + // ADE-135 additions: absent on hosts that predate the rollup, never fatal. + checksReason = try c.decodeIfPresent(String.self, forKey: .checksReason) + checksMissingRequired = try c.decodeIfPresent([String].self, forKey: .checksMissingRequired) reviewStatus = try c.decode(String.self, forKey: .reviewStatus) isMergeable = try c.decode(Bool.self, forKey: .isMergeable) mergeConflicts = try c.decode(Bool.self, forKey: .mergeConflicts) @@ -4170,6 +4188,8 @@ struct PrStatus: Codable, Equatable { prId: String, state: String, checksStatus: String, + checksReason: String? = nil, + checksMissingRequired: [String]? = nil, reviewStatus: String, isMergeable: Bool, mergeConflicts: Bool, @@ -4185,6 +4205,8 @@ struct PrStatus: Codable, Equatable { self.prId = prId self.state = state self.checksStatus = checksStatus + self.checksReason = checksReason + self.checksMissingRequired = checksMissingRequired self.reviewStatus = reviewStatus self.isMergeable = isMergeable self.mergeConflicts = mergeConflicts diff --git a/apps/ios/ADE/Services/Database.swift b/apps/ios/ADE/Services/Database.swift index 8610ef833..e4ce489cd 100644 --- a/apps/ios/ADE/Services/Database.swift +++ b/apps/ios/ADE/Services/Database.swift @@ -1397,8 +1397,8 @@ final class DatabaseService { insert into pull_requests( id, project_id, lane_id, repo_owner, repo_name, github_pr_number, github_url, github_node_id, title, state, base_branch, head_branch, checks_status, review_status, additions, deletions, - last_synced_at, created_at, updated_at, merged_at - ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + last_synced_at, created_at, updated_at, merged_at, checks_reason, checks_missing_required + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) on conflict(id) do update set project_id = excluded.project_id, lane_id = excluded.lane_id, @@ -1418,7 +1418,11 @@ final class DatabaseService { last_synced_at = excluded.last_synced_at, created_at = excluded.created_at, updated_at = excluded.updated_at, - merged_at = coalesce(excluded.merged_at, merged_at) + merged_at = coalesce(excluded.merged_at, merged_at), + -- Plain assignment, like checks_status: the reason legitimately clears + -- when a later rollup no longer needs to explain itself. + checks_reason = excluded.checks_reason, + checks_missing_required = excluded.checks_missing_required """) { statement in try bindText(pr.id, to: statement, index: 1) try bindText(projectId, to: statement, index: 2) @@ -1452,6 +1456,12 @@ final class DatabaseService { } else { sqlite3_bind_null(statement, 20) } + if let checksReason = pr.checksReason { + try bindText(checksReason, to: statement, index: 21) + } else { + sqlite3_bind_null(statement, 21) + } + try bindOptionalJson(pr.checksMissingRequired, to: statement, index: 22) } _ = try execute(""" @@ -2254,7 +2264,8 @@ final class DatabaseService { select pr.id, pr.lane_id, pr.project_id, pr.repo_owner, pr.repo_name, pr.github_pr_number, pr.github_url, pr.github_node_id, title, state, base_branch, head_branch, checks_status, review_status, additions, deletions, - last_synced_at, created_at, updated_at, merged_at, stack_snapshot.stack_json + last_synced_at, created_at, updated_at, merged_at, stack_snapshot.stack_json, + pr.checks_reason, pr.checks_missing_required from pull_requests pr left join pull_request_stack_snapshots stack_snapshot on stack_snapshot.pr_id = pr.id where pr.project_id = ? @@ -2287,7 +2298,9 @@ final class DatabaseService { stack: decodeJson( stringValue(statement, index: 20), as: GitHubPrStackMembership.self - ) + ), + checksReason: stringValue(statement, index: 21), + checksMissingRequired: decodeJson(stringValue(statement, index: 22), as: [String].self) ) } } @@ -2386,6 +2399,8 @@ final class DatabaseService { pr.created_at, pr.updated_at, stack_snapshot.stack_json, + pr.checks_reason, + pr.checks_missing_required, \(prGroupSelect) \(integrationSelect) from pull_requests pr @@ -2429,14 +2444,14 @@ final class DatabaseService { lastSyncedAt: stringValue(statement, index: 16), createdAt: stringValue(statement, index: 17) ?? "", updatedAt: stringValue(statement, index: 18) ?? "", - groupId: stringValue(statement, index: 20), - groupType: stringValue(statement, index: 21), - groupName: stringValue(statement, index: 22), - groupPosition: columnIsNull(statement, index: 23) ? nil : Int(sqlite3_column_int64(statement, 23)), - groupCount: Int(sqlite3_column_int64(statement, 24)), - workflowDisplayState: stringValue(statement, index: 25), - cleanupState: stringValue(statement, index: 26), - linkedWorkflowGroupId: stringValue(statement, index: 27) + groupId: stringValue(statement, index: 22), + groupType: stringValue(statement, index: 23), + groupName: stringValue(statement, index: 24), + groupPosition: columnIsNull(statement, index: 25) ? nil : Int(sqlite3_column_int64(statement, 25)), + groupCount: Int(sqlite3_column_int64(statement, 26)), + workflowDisplayState: stringValue(statement, index: 27), + cleanupState: stringValue(statement, index: 28), + linkedWorkflowGroupId: stringValue(statement, index: 29) ) let adeKind: String? @@ -2479,7 +2494,9 @@ final class DatabaseService { stack: decodeJson( stringValue(statement, index: 19), as: GitHubPrStackMembership.self - ) + ), + checksReason: stringValue(statement, index: 20), + checksMissingRequired: decodeJson(stringValue(statement, index: 21), as: [String].self) ) } } @@ -3019,6 +3036,11 @@ final class DatabaseService { try ensureColumn(tableName: "pull_requests", columnName: "merge_method", definition: "text") try ensureColumn(tableName: "pull_requests", columnName: "commit_count", definition: "integer") try ensureColumn(tableName: "pull_requests", columnName: "changed_files", definition: "integer") + // ADE-135: the checks rollup now carries its own explanation and the required + // contexts that never reported, so "CI passed" can no longer stand in for + // "nothing ran". `checks_missing_required` is a JSON array of context names. + try ensureColumn(tableName: "pull_requests", columnName: "checks_reason", definition: "text") + try ensureColumn(tableName: "pull_requests", columnName: "checks_missing_required", definition: "text") try exec(""" create table if not exists pull_request_snapshots ( pr_id text primary key, diff --git a/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift b/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift index 99d1dcecb..0d5e33dcc 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift @@ -30,6 +30,10 @@ func prChecksSummaryStats(checks: [PrCheck], overallChecksStatus: String?) -> Pr return .init(fail: 0, pending: 1, pass: 0, total: 1) case "passing", "success", "passed": return .init(fail: 0, pending: 0, pass: 1, total: 1) + // ADE-135: nothing verified the commit, so there is no synthetic row to invent + // in any bucket — least of all pass. + case "not_run": + return .init(fail: 0, pending: 0, pass: 0, total: 0) default: return .init(fail: 0, pending: 0, pass: 0, total: 0) } @@ -40,8 +44,18 @@ func prChecksHasFailedSignal(checks: [PrCheck], overallChecksStatus: String?) -> || prChecksSummaryStats(checks: checks, overallChecksStatus: overallChecksStatus).fail > 0 } -func prChecksEmptyStateCopy(overallChecksStatus: String?) -> (title: String, message: String) { +func prChecksEmptyStateCopy( + overallChecksStatus: String?, + checksReason: String? = nil +) -> (title: String, message: String) { switch overallChecksStatus?.lowercased() { + // ADE-135. Distinct from the default "No CI checks": there nothing was + // expected, here something was and it never arrived. + case "not_run": + return ( + "No CI ran on this commit", + checksReason ?? "No CI has run on this commit." + ) case "failing", "failure", "failed": return ( "Checks failing", @@ -68,6 +82,11 @@ func prChecksEmptyStateCopy(overallChecksStatus: String?) -> (title: String, mes struct PrChecksTab: View { let checks: [PrCheck] let overallChecksStatus: String? + /// Host-supplied explanation for a non-obvious rollup, e.g. "3 checks reported, + /// none from a CI provider." + let checksReason: String? + /// Required contexts that never reported, in the order GitHub declared them. + let missingRequired: [String] let actionRuns: [PrActionRun] let deployments: [PrDeployment] let canRerunChecks: Bool @@ -77,6 +96,8 @@ struct PrChecksTab: View { init( checks: [PrCheck], overallChecksStatus: String? = nil, + checksReason: String? = nil, + missingRequired: [String] = [], actionRuns: [PrActionRun], deployments: [PrDeployment] = [], canRerunChecks: Bool, @@ -85,6 +106,8 @@ struct PrChecksTab: View { ) { self.checks = checks self.overallChecksStatus = overallChecksStatus + self.checksReason = checksReason + self.missingRequired = missingRequired self.actionRuns = actionRuns self.deployments = deployments self.canRerunChecks = canRerunChecks @@ -105,7 +128,13 @@ struct PrChecksTab: View { } private var emptyStateCopy: (title: String, message: String) { - prChecksEmptyStateCopy(overallChecksStatus: overallChecksStatus) + prChecksEmptyStateCopy(overallChecksStatus: overallChecksStatus, checksReason: checksReason) + } + + /// True when the rollup itself is the finding: nothing verified the commit, so + /// the reason banner leads even if unrelated check rows did sync. + private var showsNotRunBanner: Bool { + overallChecksStatus?.lowercased() == "not_run" && !checks.isEmpty } var body: some View { @@ -115,6 +144,10 @@ struct PrChecksTab: View { } PrChecksStatStrip(stats: stats) + if showsNotRunBanner { + PrChecksNotRunBanner(reason: checksReason ?? "No CI has run on this commit.") + } + if checks.isEmpty { ADEEmptyStateView( symbol: "checklist", @@ -127,6 +160,10 @@ struct PrChecksTab: View { } } + if !missingRequired.isEmpty { + PrChecksMissingRequiredCard(contexts: missingRequired) + } + // Outline rerun button. PrChecksRerunButton( canRerun: canRerunChecks && isLive && hasFailedChecks, @@ -272,6 +309,128 @@ private struct PrChecksStatTile: View { } } +// MARK: - Not-run banner + +/// ADE-135. Shown when checks synced but none of them verified the commit — the +/// case that used to render as "CI passed". Muted, not red: this is a gap in what +/// we know, not a failing build. +private struct PrChecksNotRunBanner: View { + let reason: String + + var body: some View { + HStack(alignment: .top, spacing: 10) { + Circle() + .strokeBorder( + ADEColor.textSecondary, + style: StrokeStyle(lineWidth: 1.3, lineCap: .round, dash: [2.2, 2.6]) + ) + .frame(width: 15, height: 15) + .padding(.top, 1) + + VStack(alignment: .leading, spacing: 3) { + Text("No CI ran on this commit") + .font(.footnote.weight(.semibold)) + .foregroundStyle(ADEColor.textPrimary) + Text(reason) + .font(.system(size: 11)) + .foregroundStyle(ADEColor.textSecondary) + .fixedSize(horizontal: false, vertical: true) + } + + Spacer(minLength: 0) + } + .padding(.horizontal, 12) + .padding(.vertical, 11) + .background(ADEColor.glassBackground, in: RoundedRectangle(cornerRadius: 14, style: .continuous)) + .overlay( + RoundedRectangle(cornerRadius: 14, style: .continuous) + .strokeBorder(ADEColor.glassBorder, lineWidth: 0.5) + ) + .accessibilityElement(children: .combine) + } +} + +// MARK: - Missing required contexts + +/// Required contexts GitHub declared that never reported. Rendered as dimmed +/// ghost rows in the order the API gave them (no sorting — that order is how the +/// ruleset reads), so a job that never ran is visible as an empty slot rather +/// than as nothing at all. +private struct PrChecksMissingRequiredCard: View { + let contexts: [String] + + var body: some View { + VStack(alignment: .leading, spacing: 4) { + HStack(alignment: .firstTextBaseline, spacing: 8) { + Text("REQUIRED · NOT REPORTED") + .font(.system(size: 11, weight: .semibold, design: .monospaced)) + .tracking(1.2) + .foregroundColor(ADEColor.textSecondary) + Spacer(minLength: 12) + Text("\(contexts.count) missing") + .font(.system(size: 11, weight: .semibold, design: .monospaced)) + .foregroundStyle(ADEColor.textMuted) + } + .padding(.horizontal, 4) + .padding(.vertical, 4) + + VStack(spacing: 0) { + ForEach(Array(contexts.enumerated()), id: \.offset) { index, context in + if index > 0 { + Divider().overlay(ADEColor.glassBorder) + } + PrChecksMissingRequiredRow(context: context) + } + } + .background(ADEColor.glassBackground, in: RoundedRectangle(cornerRadius: 14, style: .continuous)) + .overlay( + RoundedRectangle(cornerRadius: 14, style: .continuous) + .strokeBorder(ADEColor.glassBorder, lineWidth: 0.5) + ) + } + } +} + +private struct PrChecksMissingRequiredRow: View { + let context: String + + var body: some View { + HStack(alignment: .center, spacing: 10) { + // Same hollow dashed ring as the PR row card: an empty slot where a result + // should be. + ZStack { + RoundedRectangle(cornerRadius: 6, style: .continuous) + .fill(ADEColor.textSecondary.opacity(0.08)) + Circle() + .strokeBorder( + ADEColor.textSecondary, + style: StrokeStyle(lineWidth: 1.2, lineCap: .round, dash: [2.0, 2.4]) + ) + .frame(width: 12, height: 12) + } + .frame(width: 22, height: 22) + + VStack(alignment: .leading, spacing: 2) { + Text(context) + .font(.system(.footnote, design: .monospaced).weight(.semibold)) + .foregroundStyle(ADEColor.textSecondary) + .lineLimit(1) + Text("required · not reported") + .font(.system(size: 10, design: .monospaced)) + .foregroundStyle(ADEColor.textMuted) + .lineLimit(1) + } + + Spacer(minLength: 0) + } + .padding(.horizontal, 12) + .padding(.vertical, 10) + .opacity(0.72) + .accessibilityElement(children: .combine) + .accessibilityLabel("\(context), required, not reported") + } +} + // MARK: - Groups private enum PrCheckGroupKind: String, CaseIterable { diff --git a/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift b/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift index d2e999333..8222501aa 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift @@ -82,6 +82,18 @@ struct PrDetailSummarySection: View { private var state: String { snapshot?.status?.state ?? pr.state } private var stateTint: Color { prStateTint(state) } private var checksStatus: String { snapshot?.status?.checksStatus ?? pr.checksStatus } + private var checksReason: String? { snapshot?.status?.checksReason ?? pr.checksReason } + + /// ADE-135. The merge gate counts *observed* failures, so a PR that nothing + /// ever verified still reaches its "All checks green" subline. Absence outranks + /// that copy here — the gate itself is left alone, this only changes what the + /// header says. + private var subline: String { + if checksStatus == "not_run" { + return checksReason ?? "No CI has run on this commit." + } + return mergeGate.subline + } private var files: [PrFile] { snapshot?.files ?? [] } private var commits: [PrCommit] { snapshot?.commits ?? [] } @@ -97,7 +109,7 @@ struct PrDetailSummarySection: View { VStack(alignment: .leading, spacing: 0) { HStack(alignment: .center, spacing: 8) { PrTagChip(label: state.isEmpty ? "unknown" : state, color: stateTint) - Text(mergeGate.subline) + Text(subline) .font(.system(size: 12.5)) .foregroundStyle(ADEColor.textSecondary) .lineLimit(2) diff --git a/apps/ios/ADE/Views/PRs/PrDetailOverviewPreviews.swift b/apps/ios/ADE/Views/PRs/PrDetailOverviewPreviews.swift index 88ad345ba..211b058ce 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailOverviewPreviews.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailOverviewPreviews.swift @@ -188,6 +188,46 @@ private enum PrDetailPreviewFixtures { commits: commits ) + // ADE-135 fixture: reproduces PR #988 — three third-party apps reported + // `success`, GitHub Actions registered no suite, and one required context never + // reported. This used to render as "CI passed". + static let notRunReason = "3 checks reported, none from a CI provider. CI has not run on this commit." + + static let notRunMissingRequired = ["CI / build", "CI / test (ubuntu-latest)"] + + static var notRunPr: PullRequestListItem { + var item = pr + item.checksStatus = "not_run" + item.checksReason = notRunReason + item.checksMissingRequired = notRunMissingRequired + return item + } + + static var notRunSnapshot: PullRequestSnapshot { + PullRequestSnapshot( + detail: detail, + status: PrStatus( + prId: pr.id, + state: "open", + checksStatus: "not_run", + checksReason: notRunReason, + checksMissingRequired: notRunMissingRequired, + reviewStatus: "requested", + isMergeable: true, + mergeConflicts: false, + behindBaseBy: 0, + reviewDecision: .reviewRequired, + approvalsCount: 0, + requiredApprovals: 1 + ), + checks: [], + reviews: reviews, + comments: [], + files: files, + commits: commits + ) + } + static let unresolvedThread = PrReviewThread( id: "thread-1", isResolved: false, @@ -372,6 +412,54 @@ private struct PrGitHubStackCardPreviewScreen: View { } } +/// ADE-135. Header + checks tab for a PR nothing verified: the summary section's +/// subline carries the reason instead of the gate's green copy, and the checks +/// tab lists the required contexts that never reported as ghost rows. +private struct PrDetailNotRunPreviewScreen: View { + @State private var commitsExpanded = false + + var body: some View { + ScrollView { + VStack(spacing: 14) { + PrDetailSummarySection( + pr: PrDetailPreviewFixtures.notRunPr, + snapshot: PrDetailPreviewFixtures.notRunSnapshot, + // Deliberately the green gate: the gate counts observed failures and + // finds none, which is exactly the state the header must override. + mergeGate: PrMergeGateInfo(tone: .green, subline: "All checks green", target: .overview), + commitsExpanded: $commitsExpanded, + onChecksTap: {}, + onFilesTap: {}, + onCommitTap: { _ in } + ) + + PrChecksTab( + checks: [], + overallChecksStatus: "not_run", + checksReason: PrDetailPreviewFixtures.notRunReason, + missingRequired: PrDetailPreviewFixtures.notRunMissingRequired, + actionRuns: [], + canRerunChecks: true, + isLive: true, + onRerun: {} + ) + } + .padding(16) + } + .background(prLiquidGlassBackdrop().ignoresSafeArea()) + } +} + +#Preview("PR detail · Checks not run") { + PrDetailNotRunPreviewScreen() + .preferredColorScheme(.dark) +} + +#Preview("PR detail · Checks not run · light") { + PrDetailNotRunPreviewScreen() + .preferredColorScheme(.light) +} + #Preview("PR detail · Overview thread") { PrDetailOverviewPreviewScreen() .preferredColorScheme(.dark) diff --git a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift index 1b52abb33..5eaf74671 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift @@ -296,7 +296,9 @@ struct PrDetailView: View { linkedGroupCount: 0, workflowDisplayState: githubItem?.workflowDisplayState, cleanupState: githubItem?.cleanupState, - stack: githubItem?.stack + stack: githubItem?.stack, + checksReason: status?.checksReason, + checksMissingRequired: status?.checksMissingRequired ) } @@ -629,6 +631,10 @@ struct PrDetailView: View { PrChecksTab( checks: snapshot?.checks ?? [], overallChecksStatus: snapshot?.status?.checksStatus ?? currentPr.checksStatus, + checksReason: snapshot?.status?.checksReason ?? currentPr.checksReason, + missingRequired: snapshot?.status?.checksMissingRequired + ?? currentPr.checksMissingRequired + ?? [], actionRuns: actionRuns, deployments: deployments, canRerunChecks: canRerunChecks, diff --git a/apps/ios/ADE/Views/PRs/PrHelpers.swift b/apps/ios/ADE/Views/PRs/PrHelpers.swift index ff2284097..97c5923ca 100644 --- a/apps/ios/ADE/Views/PRs/PrHelpers.swift +++ b/apps/ios/ADE/Views/PRs/PrHelpers.swift @@ -908,6 +908,10 @@ func prChecksTint(_ status: String) -> Color { return ADEColor.danger case "pending", "queued", "in_progress": return ADEColor.warning + // ADE-135. `not_run` means nothing verified the commit. It reads as an empty + // slot, not an alarm, so it stays in the muted tone rather than danger red. + case "not_run": + return ADEColor.textSecondary default: return ADEColor.textSecondary } @@ -931,6 +935,7 @@ func prChecksLabel(_ status: String) -> String { case "passing": return "Passing" case "failing": return "Failing" case "pending": return "Pending" + case "not_run": return "Not run" default: return titleCase(status) } } diff --git a/apps/ios/ADE/Views/PRs/PrMergeGateCard.swift b/apps/ios/ADE/Views/PRs/PrMergeGateCard.swift index a0c3364c8..a809ebc1b 100644 --- a/apps/ios/ADE/Views/PRs/PrMergeGateCard.swift +++ b/apps/ios/ADE/Views/PRs/PrMergeGateCard.swift @@ -283,8 +283,19 @@ func prComputeMergeGate( return PrMergeGateInfo(tone: .amber, subline: subline, target: .overview) } + // ADE-135. Nothing here is failing or pending, but "green" is a claim about + // observed results and `not_run` means there were none: third-party apps + // only, or an all-skipped suite. The tone stays green deliberately — it feeds + // merge enablement at PrDetailScreen's `canMerge`, and this fix is not + // allowed to gate anyone's merge. Only the sentence changes, because the + // sentence is the part that was false. + let summarySaysNotRun = normalizedSummaryChecksStatus == "not_run" let subline: String - if reviewsNeeded > 0 || reviewsHave > 0 { + if summarySaysNotRun { + subline = reviewsNeeded > 0 || reviewsHave > 0 + ? "\(approvalsText) · no CI has run on this commit" + : "No CI has run on this commit" + } else if reviewsNeeded > 0 || reviewsHave > 0 { subline = "\(approvalsText) · all checks green" } else { subline = "All checks green" diff --git a/apps/ios/ADE/Views/PRs/PrRowCard.swift b/apps/ios/ADE/Views/PRs/PrRowCard.swift index 785df5bed..614c27209 100644 --- a/apps/ios/ADE/Views/PRs/PrRowCard.swift +++ b/apps/ios/ADE/Views/PRs/PrRowCard.swift @@ -223,9 +223,7 @@ struct PrRowCard: View { } if !data.isTerminal, let ci = data.ciIndicator { - Image(systemName: ci.symbol) - .foregroundStyle(ci.color) - .accessibilityLabel(ci.title) + PrRowCiGlyph(indicator: ci) } if !data.isTerminal, let review = data.reviewIndicator { @@ -264,6 +262,33 @@ struct PrRowCard: View { } } +/// Renders the row's CI signal. Symbol states track the ambient caption font so +/// they stay aligned with the review glyph beside them; the `not_run` ring is +/// drawn at a fixed 13pt, which matches a filled `.caption2` symbol optically. +private struct PrRowCiGlyph: View { + let indicator: PrRowCard.Data.CIIndicator + + var body: some View { + switch indicator.glyph { + case let .symbol(name): + Image(systemName: name) + .foregroundStyle(indicator.color) + .accessibilityLabel(indicator.title) + case .hollowRing: + Circle() + .strokeBorder( + indicator.color, + style: StrokeStyle(lineWidth: 1.3, lineCap: .round, dash: [2.2, 2.6]) + ) + .frame(width: 13, height: 13) + // Shapes are not accessibility elements by default, so the ring has to be + // promoted to one or the finding is invisible to VoiceOver. + .accessibilityElement() + .accessibilityLabel(indicator.title) + } + } +} + struct PrRowCardSkeleton: View { var body: some View { HStack(alignment: .top, spacing: 11) { @@ -342,6 +367,9 @@ extension PrRowCard { let isUnmapped: Bool let laneLabel: String? let checksStatus: String? + /// Host-supplied explanation for a non-obvious rollup. Surfaced verbatim on + /// the CI indicator's accessibility label so the "why" travels with the glyph. + let checksReason: String? let reviewStatus: String? let warnMessage: String? let stackGroupId: String? @@ -385,7 +413,15 @@ extension PrRowCard { } struct CIIndicator { - let symbol: String + /// ADE-135. `not_run` has no honest SF Symbol — every circle-with-a-mark + /// reads as a verdict, and this state is the absence of one. It draws a + /// hollow dashed ring instead: an empty slot where a result should be. + enum Glyph: Equatable { + case symbol(String) + case hollowRing + } + + let glyph: Glyph let color: Color let title: String } @@ -398,11 +434,19 @@ extension PrRowCard { var ciIndicator: CIIndicator? { switch checksStatus { case "passing": - return CIIndicator(symbol: "checkmark.circle.fill", color: PrsGlass.openTop, title: "CI passing") + return CIIndicator(glyph: .symbol("checkmark.circle.fill"), color: PrsGlass.openTop, title: "CI passing") case "failing": - return CIIndicator(symbol: "xmark.circle.fill", color: PrsGlass.closedTop, title: "CI failing") + return CIIndicator(glyph: .symbol("xmark.circle.fill"), color: PrsGlass.closedTop, title: "CI failing") case "pending": - return CIIndicator(symbol: "clock.fill", color: PrsGlass.draftTop, title: "CI pending") + return CIIndicator(glyph: .symbol("clock.fill"), color: PrsGlass.draftTop, title: "CI pending") + case "not_run": + // Checks exist or are required, but nothing verified this commit. Muted, + // never the failure red — this is a gap, not a red build. + return CIIndicator( + glyph: .hollowRing, + color: PrsGlass.textMuted, + title: checksReason ?? "No CI has run on this commit." + ) default: return nil } @@ -437,7 +481,10 @@ extension PrRowCard { self.isExternal = false self.isUnmapped = false self.laneLabel = pr.laneName ?? pr.laneId + // Only "none" (nothing observed, nothing expected) is silent. "not_run" is a + // finding and must survive to `ciIndicator`. self.checksStatus = pr.checksStatus == "none" ? nil : pr.checksStatus + self.checksReason = pr.checksReason self.reviewStatus = pr.reviewStatus == "none" ? nil : pr.reviewStatus self.warnMessage = Self.warnMessage( workflowDisplayState: pr.workflowDisplayState, @@ -476,6 +523,7 @@ extension PrRowCard { self.isUnmapped = unmapped self.laneLabel = item.linkedLaneName ?? item.linkedLaneId ?? linkedPr?.laneName ?? linkedPr?.laneId self.checksStatus = linkedPr?.checksStatus == "none" ? nil : linkedPr?.checksStatus + self.checksReason = linkedPr?.checksReason self.reviewStatus = linkedPr?.reviewStatus == "none" ? nil : linkedPr?.reviewStatus self.warnMessage = unmapped ? nil @@ -518,6 +566,8 @@ extension PrRowCard { if checksStatus == "failing" { return "CI failing" } + // "not_run" deliberately produces no warn banner: the hollow ring already + // states it, and this row is reserved for things the user must act on. return nil } } diff --git a/apps/ios/ADE/Views/PRs/PrRowCardPreviews.swift b/apps/ios/ADE/Views/PRs/PrRowCardPreviews.swift index e58e53dd6..74ae278ef 100644 --- a/apps/ios/ADE/Views/PRs/PrRowCardPreviews.swift +++ b/apps/ios/ADE/Views/PRs/PrRowCardPreviews.swift @@ -130,10 +130,35 @@ private enum PrRowCardPreviewData { commentCount: 0 ) + // ADE-135. The PR nothing verified: third-party apps reported success, GitHub + // Actions never registered a suite. The row must show a hollow dashed ring, not + // the green checkmark this used to render. + static let linkedPr988NotRun: PullRequestListItem = { + var item = linkedPr559 + item.id = "pr-988" + item.githubPrNumber = 988 + item.title = "GitHub Rate Limit Fallback" + item.checksStatus = "not_run" + item.checksReason = "3 checks reported, none from a CI provider. CI has not run on this commit." + item.checksMissingRequired = ["CI / build"] + return item + }() + + static let github988NotRun: GitHubPrListItem = { + var item = github559 + item.id = "gh-988" + item.githubPrNumber = 988 + item.githubUrl = "https://github.com/arul28/ADE/pull/988" + item.title = "GitHub Rate Limit Fallback" + item.linkedPrId = linkedPr988NotRun.id + return item + }() + static var githubListRows: some View { ScrollView { VStack(spacing: 0) { PrRowCard(item: github559, linkedPr: linkedPr559) + PrRowCard(item: github988NotRun, linkedPr: linkedPr988NotRun) PrRowCard(item: github346) PrRowCard(item: github425) } @@ -162,6 +187,15 @@ private enum PrRowCardPreviewData { .background(PrsLiquidBackdrop()) } +#Preview("CI not run · PR #988") { + PrRowCard( + item: PrRowCardPreviewData.github988NotRun, + linkedPr: PrRowCardPreviewData.linkedPr988NotRun + ) + .padding(.horizontal, 16) + .background(PrsLiquidBackdrop()) +} + #Preview("Unmapped bot PR #346") { PrRowCard( item: PrRowCardPreviewData.github346 diff --git a/apps/ios/ADE/Views/PRs/PrsRootScreenPreviews.swift b/apps/ios/ADE/Views/PRs/PrsRootScreenPreviews.swift index 8c9503863..c121e11ac 100644 --- a/apps/ios/ADE/Views/PRs/PrsRootScreenPreviews.swift +++ b/apps/ios/ADE/Views/PRs/PrsRootScreenPreviews.swift @@ -160,6 +160,29 @@ private enum PrsRootPreviewData { commentCount: 0 ) + // ADE-135. Sits directly under the passing row so the hollow dashed ring can be + // compared against the green checkmark it used to be mistaken for. + static let linkedPr988NotRun: PullRequestListItem = { + var item = linkedPr559 + item.id = "pr-988" + item.githubPrNumber = 988 + item.title = "GitHub Rate Limit Fallback" + item.checksStatus = "not_run" + item.checksReason = "3 checks reported, none from a CI provider. CI has not run on this commit." + item.checksMissingRequired = ["CI / build"] + return item + }() + + static let github988NotRun: GitHubPrListItem = { + var item = github559 + item.id = "gh-988" + item.githubPrNumber = 988 + item.githubUrl = "https://github.com/arul28/ADE/pull/988" + item.title = "GitHub Rate Limit Fallback" + item.linkedPrId = linkedPr988NotRun.id + return item + }() + static let categoryCounts = PrGitHubCategoryCounts(open: 21, merged: 0, closed: 0) } @@ -200,6 +223,12 @@ private struct PrsGitHubRootPreviewScreen: View { ) .prListRowCard() + PrRowCard( + item: PrsRootPreviewData.github988NotRun, + linkedPr: PrsRootPreviewData.linkedPr988NotRun + ) + .prListRowCard() + PrRowCard( item: PrsRootPreviewData.github346 ) diff --git a/apps/ios/ADE/Views/Work/WorkChatPrViews.swift b/apps/ios/ADE/Views/Work/WorkChatPrViews.swift index 5eabbe30c..8e0e2b6f5 100644 --- a/apps/ios/ADE/Views/Work/WorkChatPrViews.swift +++ b/apps/ios/ADE/Views/Work/WorkChatPrViews.swift @@ -5,6 +5,8 @@ struct WorkChatPrBadgeModel: Equatable { let title: String let state: String let checksStatus: String? + /// Host-supplied explanation for a non-obvious checks rollup (ADE-135). + let checksReason: String? let reviewStatus: String? let updatedAt: String let stack: GitHubPrStackMembership? @@ -17,6 +19,7 @@ func workChatPrBadgeModel(tag: LanePrTag?, pr: PullRequestListItem?, summary: Pr title: tag.title, state: tag.state, checksStatus: pr?.checksStatus ?? summary?.checksStatus, + checksReason: pr?.checksReason ?? summary?.checksReason, reviewStatus: pr?.reviewStatus ?? summary?.reviewStatus, updatedAt: tag.updatedAt, stack: tag.stack ?? pr?.stack ?? summary?.stack @@ -31,14 +34,23 @@ struct WorkChatPrActivePopup: View { lanePullRequestTint(badge.state) } - private var ciSymbol: String? { + /// ADE-135: `notRun` draws a hollow dashed ring rather than a symbol, so an + /// absent CI result never borrows the vocabulary of a pass or a failure. + private enum CiGlyph: Equatable { + case symbol(String) + case notRun + } + + private var ciGlyph: CiGlyph? { switch badge.checksStatus { case "passing": - return "checkmark.circle.fill" + return .symbol("checkmark.circle.fill") case "failing": - return "xmark.circle.fill" + return .symbol("xmark.circle.fill") case "pending": - return "clock.fill" + return .symbol("clock.fill") + case "not_run": + return .notRun default: return nil } @@ -46,7 +58,9 @@ struct WorkChatPrActivePopup: View { private var accessibilityText: String { var parts = [badge.label, lanePrStateLabel(badge.state)] - if let checksStatus = badge.checksStatus, !checksStatus.isEmpty { + if badge.checksStatus == "not_run" { + parts.append(badge.checksReason ?? "No CI has run on this commit.") + } else if let checksStatus = badge.checksStatus, !checksStatus.isEmpty { parts.append("checks \(checksStatus)") } if let reviewStatus = badge.reviewStatus, !reviewStatus.isEmpty, reviewStatus != "none" { @@ -73,9 +87,19 @@ struct WorkChatPrActivePopup: View { if let stack = badge.stack { GitHubStackPositionBadge(stack: stack, compact: true) } - if let ciSymbol { - Image(systemName: ciSymbol) - .font(.system(size: 10, weight: .bold)) + if let ciGlyph { + switch ciGlyph { + case let .symbol(name): + Image(systemName: name) + .font(.system(size: 10, weight: .bold)) + case .notRun: + Circle() + .strokeBorder( + ADEColor.textSecondary, + style: StrokeStyle(lineWidth: 1.2, lineCap: .round, dash: [2.0, 2.4]) + ) + .frame(width: 11, height: 11) + } } } } @@ -109,6 +133,10 @@ struct WorkChatPrDetailsSheet: View { snapshot?.status?.checksStatus ?? pr?.checksStatus ?? summary?.checksStatus } + private var checksReason: String? { + snapshot?.status?.checksReason ?? pr?.checksReason ?? summary?.checksReason + } + private var additions: Int { pr?.additions ?? summary?.additions ?? 0 } @@ -199,7 +227,7 @@ struct WorkChatPrDetailsSheet: View { HStack(spacing: 10) { WorkChatPrChangesMetricCard(additions: additions, deletions: deletions) - WorkChatPrChecksMetricCard(status: checksStatus) + WorkChatPrChecksMetricCard(status: checksStatus, reason: checksReason) } if let errorMessage, !errorMessage.isEmpty { @@ -384,6 +412,7 @@ private struct WorkChatPrChangesMetricCard: View { private struct WorkChatPrChecksMetricCard: View { let status: String? + let reason: String? private var normalized: String { status?.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() ?? "" @@ -393,6 +422,13 @@ private struct WorkChatPrChecksMetricCard: View { workChatPrChecksTint(normalized) } + /// "Not run" alone invites the reader to assume a transient state, so ADE-135 + /// carries the host's one-line explanation with it. + private var detail: String? { + guard normalized == "not_run" else { return nil } + return reason ?? "No CI has run on this commit." + } + var body: some View { VStack(alignment: .leading, spacing: 7) { Label("Checks", systemImage: workChatPrChecksSymbol(normalized)) @@ -404,6 +440,13 @@ private struct WorkChatPrChecksMetricCard: View { .foregroundStyle(tint) .lineLimit(1) .minimumScaleFactor(0.78) + + if let detail { + Text(detail) + .font(.system(size: 10.5)) + .foregroundStyle(ADEColor.textMuted) + .fixedSize(horizontal: false, vertical: true) + } } .frame(maxWidth: .infinity, minHeight: 70, alignment: .leading) .padding(12) @@ -498,6 +541,10 @@ private func workChatPrChecksSymbol(_ status: String) -> String { return "xmark.circle.fill" case "pending", "queued", "running", "in_progress": return "clock.fill" + // ADE-135: dashed circle, matching the hollow ring the PR row draws. Nothing + // verified the commit, so the slot reads as empty rather than as a verdict. + case "not_run": + return "circle.dashed" default: return "circle" } @@ -511,6 +558,9 @@ private func workChatPrChecksTint(_ status: String) -> Color { return ADEColor.danger case "pending", "queued", "running", "in_progress": return ADEColor.warning + // Muted, never danger red: an absent result is a gap, not a red build. + case "not_run": + return ADEColor.textSecondary default: return ADEColor.textSecondary } @@ -520,6 +570,8 @@ private func workChatPrChecksLabel(_ status: String) -> String { switch status { case "", "none", "unknown": return "None" + case "not_run": + return "Not run" case "passing", "passed", "success": return "Passing" case "failing", "failed", "failure", "error": From aff8c99e9be53213c15eec42b5e032e11dfbe334 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sun, 2 Aug 2026 03:23:08 -0400 Subject: [PATCH 2/3] fix(prs): harden the checks rollup across every surface, and prove it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up work on the same ticket, from /quality (36 findings), /test (parity + regression coverage), and the ship-loop revalidation. The original fix corrected the rollup but left the same disease on other surfaces. Fourteen more places rendered a pass from producer-blind row counts; each is now gated on the canonical rollup: * the merge checklist, desktop and iOS, which said "All 3 checks passed" for PR #988's exact payload — the ticket's own bug on the surface a reader trusts most; * adeRpcServer.summarizePrChecks, which initialised its verdict to "passing" so *zero* checks reported green, on the surface an agent reads before deciding to merge; * PrChecksCard's header, PrDetailPane's pill, PrChecksTab's strip, the ade code drawer and right pane, ChatPrPane, ChatGitToolbar, rightPaneFormatters, LaneNode, getPrEdgeColor, iOS's stat strip and group cards, and PrLaneSummary. Two design corrections to the original fix: * The CI-producer rule was an Actions-only allowlist. CircleCI, Buildkite and Azure Pipelines report through the Checks API with their own app slugs, so that rule marked every such repo permanently unverified — the same bug pointing the other way. It is now a denylist of known non-CI apps, plus a branch-protection override: if every required context reported and passed, the commit was verified whoever ran it. The override is ordered after the in-flight check so it cannot claim a pass while a job is still running. * The grace-window clock read PR `updated_at`, which GitHub bumps on every comment — including the comments posted by the very bots whose presence is the finding. It now reads the earliest check `started_at`, which a comment cannot move. Also: a failed /check-runs fetch preserves the previous rollup instead of persisting a false not_run (bestEffort returns [] on a 403, which is indistinguishable from "no checks"); check runs carrying a terminal conclusion before their status flips no longer stick at pending forever; and pr_get_checks returns the persisted verdict rather than a row tally that cannot see required contexts. Tests: regression coverage at the prChecksRollup and requiredChecks layers so every surface inherits it, plus named tests for the merge checklist, the agent-facing RPC, the TUI's bare-array path, the fetch-failure preservation, and lane summaries. The real #988 payload is committed as a fixture. Docs, iOS, the ade CLI and the TUI are updated in lockstep. Co-Authored-By: Claude Opus 5 --- apps/ade-cli/README.md | 2 +- apps/ade-cli/src/adeRpcServer.test.ts | 36 +++ apps/ade-cli/src/adeRpcServer.ts | 77 ++++-- apps/ade-cli/src/cli.test.ts | 47 ++++ apps/ade-cli/src/cli.ts | 43 +++- .../src/tuiClient/__tests__/Drawer.test.tsx | 30 +++ .../tuiClient/__tests__/RightPane.test.tsx | 29 +++ .../__tests__/rightPaneFormatters.test.ts | 45 ++++ apps/ade-cli/src/tuiClient/app.tsx | 38 ++- .../src/tuiClient/components/Drawer.tsx | 29 ++- .../src/tuiClient/components/RightPane.tsx | 33 ++- .../src/tuiClient/rightPaneFormatters.ts | 50 ++-- apps/ade-cli/src/tuiClient/types.ts | 5 + .../src/main/services/prs/prChatCards.test.ts | 15 +- .../src/main/services/prs/prChatCards.ts | 55 +++-- .../src/main/services/prs/prService.test.ts | 99 ++++++++ .../src/main/services/prs/prService.ts | 113 +++++++-- .../components/app/prToastPresentation.ts | 8 +- .../components/chat/ChatGitToolbar.tsx | 63 ++--- .../renderer/components/chat/ChatPrPane.tsx | 3 +- .../components/graph/WorkspaceGraphPage.tsx | 3 +- .../components/lanes/LanePrBadgePopover.tsx | 7 +- .../components/prs/detail/PrChecksTab.tsx | 18 +- .../components/prs/detail/PrDetailPane.tsx | 27 ++- .../components/prs/shared/GitHubTabPrRow.tsx | 4 +- .../prs/shared/PrChecksCard.test.tsx | 21 +- .../components/prs/shared/PrChecksCard.tsx | 26 +- .../prs/shared/PrDetailRightMetadataRail.tsx | 1 + .../prs/shared/prMergeRailUtils.test.ts | 18 ++ .../components/prs/shared/prMergeRailUtils.ts | 22 ++ .../components/prs/shared/prVisuals.tsx | 11 +- .../desktop/src/shared/prChecksRollup.test.ts | 169 ++++++++++++- apps/desktop/src/shared/prChecksRollup.ts | 225 ++++++++++++++++-- apps/desktop/src/shared/types/prs.ts | 10 + .../ios/ADE/Views/PRs/PrDetailChecksTab.swift | 157 ++++++++---- .../Views/PRs/PrDetailHeaderComponents.swift | 2 +- apps/ios/ADE/Views/PRs/PrDetailScreen.swift | 3 +- apps/ios/ADE/Views/PRs/PrHelpers.swift | 6 + apps/ios/ADE/Views/PRs/PrMergeChecklist.swift | 16 +- apps/ios/ADE/Views/PRs/PrRowCard.swift | 2 +- apps/ios/ADE/Views/Work/WorkChatPrViews.swift | 21 +- apps/ios/ADETests/ADETests.swift | 191 ++++++++++++++- docs/ARCHITECTURE.md | 6 +- docs/features/ade-code/README.md | 4 +- docs/features/pull-requests/README.md | 121 +++++++++- .../sync-and-multi-device/ios-companion.md | 19 ++ docs/features/workspace-graph/README.md | 12 +- 47 files changed, 1684 insertions(+), 258 deletions(-) diff --git a/apps/ade-cli/README.md b/apps/ade-cli/README.md index f3247ae3b..fabcde19d 100644 --- a/apps/ade-cli/README.md +++ b/apps/ade-cli/README.md @@ -409,7 +409,7 @@ ade prs create --lane lane-id --base main --close-linear-issue-on-merge ade prs list-open --text ade prs github-snapshot --include-external-closed --history-page-limit 4 ade prs github-snapshot --include-state-counts --no-revalidate -ade prs checks pr-id --text +ade prs checks pr-id --text # header carries the canonical rollup (checksStatus/checksCounts); "not run" means nothing verified the commit, whatever the rows say ade prs comments pr-id --text ade shell start --lane lane-id -- npm test ade terminal list --lane lane-id --text diff --git a/apps/ade-cli/src/adeRpcServer.test.ts b/apps/ade-cli/src/adeRpcServer.test.ts index 6c3d34f5e..e721e6643 100644 --- a/apps/ade-cli/src/adeRpcServer.test.ts +++ b/apps/ade-cli/src/adeRpcServer.test.ts @@ -852,6 +852,42 @@ function createFakePathExecutable(dir: string, name: string): string { } describe("adeRpcServer", () => { + it("does not report a green rollup to agents when only third-party bots reported", async () => { + // ADE-135: `summarizePrChecks` initialised `overall` to "passing" and was + // producer-blind, so PR #988's three bot successes came back as green on + // the exact surface an autonomous agent reads before deciding to merge. + const { runtime } = createRuntime(); + runtime.prService.getChecks = vi.fn(async () => [ + { name: "CodeRabbit", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "coderabbitai" }, + { name: "Vercel", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "vercel" }, + { name: "Vercel Preview Comments", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "vercel" }, + ]); + const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" }); + await initialize(handler, { role: "agent", chatSessionId: "session-1" }); + + const result = await callTool(handler, "pr_get_checks", { prId: "pr-1" }); + const payload = result.structuredContent ?? result; + + expect(payload.checksStatus).toBe("not_run"); + expect(payload.checksCounts.passing).toBe(0); + expect(payload.checksCounts.total).toBe(3); + expect(payload.checks[0].appSlug).toBe("coderabbitai"); + }); + + it("reports zero checks as none rather than defaulting to passing", async () => { + const { runtime } = createRuntime(); + runtime.prService.getChecks = vi.fn(async () => []); + const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" }); + await initialize(handler, { role: "agent", chatSessionId: "session-1" }); + + const result = await callTool(handler, "pr_get_checks", { prId: "pr-1" }); + const payload = result.structuredContent ?? result; + + expect(payload.checksStatus).not.toBe("passing"); + expect(payload.checksStatus).toBe("none"); + expect(payload.checksCounts.total).toBe(0); + }); + it("exposes direct PTY RPC methods with enriched create/list responses", async () => { const { runtime } = createRuntime(); const handler = createAdeRpcRequestHandler({ runtime, serverVersion: "test" }); diff --git a/apps/ade-cli/src/adeRpcServer.ts b/apps/ade-cli/src/adeRpcServer.ts index 929ea7002..8a8b4788d 100644 --- a/apps/ade-cli/src/adeRpcServer.ts +++ b/apps/ade-cli/src/adeRpcServer.ts @@ -25,6 +25,7 @@ import { getDefaultModelDescriptor } from "../../desktop/src/shared/modelRegistr import { buildAdeCliInlineGuidance } from "../../desktop/src/shared/adeCliGuidance"; import { buildDeeplink, isValidCommitSha, isValidRepoRelativePath } from "../../desktop/src/shared/deeplinks"; import { resolveStableLaneBaseBranch } from "../../desktop/src/shared/laneBaseResolution"; +import { rollupPrChecks } from "../../desktop/src/shared/prChecksRollup"; import { ADE_AGENT_SKILLS_DIRS_ENV, getAdeAgentSkillRootsForPrompt, @@ -41,7 +42,7 @@ import { type MergeMethod, type AppNavigationRequest, } from "../../desktop/src/shared/types"; -import type { PrCheck, PrComment, PrReviewThread } from "../../desktop/src/shared/types/prs"; +import type { PrCheck, PrChecksStatus, PrComment, PrReviewThread } from "../../desktop/src/shared/types/prs"; import type { CtoLinearQuickView } from "../../desktop/src/shared/types/cto"; import type { LinearConnectionStatus } from "../../desktop/src/shared/types/linearSync"; import { resolveAdeLayout } from "../../desktop/src/shared/adeLayout"; @@ -1860,24 +1861,43 @@ function requirePrService(runtime: AdeRuntime): NonNullable check.conclusion === "success").length; - const failing = checks.filter((check) => check.conclusion === "failure").length; - const pending = checks.filter((check) => check.status !== "completed").length; - - // ADE-135: this started at "passing" and only moved off it for a failure or - // an in-flight run, so zero checks — or a suite that was entirely skipped — - // reported green. Nothing succeeding means nothing was verified, which is - // "not_run", not a pass. - let overall: "failing" | "pending" | "passing" | "not_run" = passing > 0 ? "passing" : "not_run"; - if (failing > 0) overall = "failing"; - else if (pending > 0) overall = "pending"; - - return { overall, counts: { passing, failing, pending, total: checks.length } }; +function summarizePrChecks(checks: PrCheck[]): { + overall: PrChecksStatus; + counts: { passing: number; failing: number; pending: number; total: number }; +} { + // ADE-135: this used to carry its own pass/fail rule and got it wrong twice + // over — it initialised `overall` to "passing", so zero checks read green, + // and it was producer-blind, so a single rate-limited CodeRabbit `success` + // also read green. That is the ticket's bug on the surface agents read. + // The shared rollup is the only authority now. + const { status, counts } = rollupPrChecks(checks); + return { + overall: status, + counts: { + passing: counts.passing, + failing: counts.failing, + pending: counts.pending, + total: counts.total, + }, + }; } -function mapCheckToSummary(check: PrCheck): { name: string; status: string; conclusion: string | null; url: string | null } { - return { name: check.name, status: check.status, conclusion: check.conclusion, url: check.detailsUrl }; +function mapCheckToSummary(check: PrCheck): { + name: string; + status: string; + conclusion: string | null; + url: string | null; + appSlug: string | null; +} { + // ADE-135: without the producer, an agent reading three rows of + // `conclusion: "success"` has no way to know that not one of them is CI. + return { + name: check.name, + status: check.status, + conclusion: check.conclusion, + url: check.detailsUrl, + appSlug: check.appSlug ?? null, + }; } function summarizePrReviewComments( @@ -5195,9 +5215,32 @@ async function runTool(args: { const prId = assertNonEmptyString(toolArgs.prId, "prId"); const prSvc = requirePrService(runtime); const checks = await prSvc.getChecks(prId); + // The aggregate travels with the rows so an agent does not have to + // re-derive "was this verified?" and get it wrong, which is the bug this + // ticket exists to fix. + // + // The PERSISTED verdict wins when we have it. A row-level rollup only sees + // the flattened checks: it cannot see required contexts that never + // reported, the merge-state corroboration, or the grace window, all of + // which live in `computeStatus`. Reporting the row tally here would let + // this tool say "passing" while every human surface says "not run" — on + // precisely the surface an autonomous agent reads before merging. + const { overall, counts } = summarizePrChecks(checks); + // Best-effort: `listAll` is a local DB read, but a degraded runtime may not + // expose it. Falling back to the row tally is still better than throwing. + let summary: { checksStatus?: string; checksReason?: string | null; checksMissingRequired?: string[] | null } | null = null; + try { + summary = prSvc.listAll?.().find((entry) => entry.id === prId) ?? null; + } catch { + summary = null; + } return { success: true, prId, + checksStatus: summary?.checksStatus ?? overall, + checksReason: summary?.checksReason ?? null, + checksMissingRequired: summary?.checksMissingRequired ?? [], + checksCounts: counts, checks: checks.map(mapCheckToSummary), }; } diff --git a/apps/ade-cli/src/cli.test.ts b/apps/ade-cli/src/cli.test.ts index 0be15f9e4..ca3065e5f 100644 --- a/apps/ade-cli/src/cli.test.ts +++ b/apps/ade-cli/src/cli.test.ts @@ -2784,6 +2784,53 @@ describe("ADE CLI", () => { }); }); + // ADE-135: `ade prs checks` printed nothing but the row table, so three + // third-party bot rows each rendering `OK` read as a passing commit. The + // canonical rollup now travels with the rows and leads the output. + it("leads `prs checks --text` with the canonical rollup, not the row tally", () => { + const opts = { + ...baseResolveOpts(), + projectRoot: null, + workspaceRoot: null, + text: true, + }; + const notRun = formatOutput( + { + success: true, + prId: "pr-988", + checksStatus: "not_run", + checksCounts: { passing: 0, failing: 0, pending: 0, total: 3 }, + checks: [ + { name: "CodeRabbit", status: "completed", conclusion: "success", appSlug: "coderabbitai" }, + { name: "Vercel — Preview", status: "completed", conclusion: "success", appSlug: "vercel" }, + { name: "changeset-bot", status: "completed", conclusion: "success", appSlug: "changeset-bot" }, + ], + }, + opts, + "pr-checks", + ); + expect(notRun).toContain("ADE PR checks - not run"); + expect(notRun).toContain("3 checks reported"); + // The raw enum must never reach the reader — the phrase is the point. + expect(notRun).not.toContain("not_run"); + + const passing = formatOutput( + { + success: true, + prId: "pr-42", + checksStatus: "passing", + checksCounts: { passing: 2, failing: 0, pending: 0, total: 2 }, + checks: [ + { name: "ci / unit", status: "completed", conclusion: "success", appSlug: "github-actions" }, + { name: "ci / lint", status: "completed", conclusion: "success", appSlug: "github-actions" }, + ], + }, + opts, + "pr-checks", + ); + expect(passing).toContain("ADE PR checks - passing (2 passing"); + }); + describe("chat create parent lineage", () => { const savedParentEnv = process.env.ADE_CHAT_SESSION_ID; afterEach(() => { diff --git a/apps/ade-cli/src/cli.ts b/apps/ade-cli/src/cli.ts index 25e7d711b..7ae172881 100644 --- a/apps/ade-cli/src/cli.ts +++ b/apps/ade-cli/src/cli.ts @@ -1608,7 +1608,7 @@ const HELP_BY_COMMAND: Record = { $ ade prs create --lane --base main Open and map a GitHub PR; prints GitHub + ADE URLs $ ade prs create --lane --close-linear-issue-on-merge $ ade prs link --lane --url Map an existing GitHub PR to a lane - $ ade prs checks --text Show check status + $ ade prs checks --text Show the CI rollup + per-check rows ("not run" = nothing verified the commit) $ ade prs comments --text Show unresolved review work $ ade prs github-snapshot --include-external-closed --history-page-limit 4 Include bounded closed PR history in the GitHub snapshot @@ -17599,6 +17599,10 @@ function statusWord(value: unknown): string { return "FAIL"; if (["pending", "running", "in_progress", "queued", "active"].includes(raw)) return "WAIT"; + // ADE-135: `not_run` is a checks rollup, not a job conclusion, and it must + // never read as a raw enum — the sentence it stands for is "nothing verified + // this commit". + if (raw === "not_run") return "NOT RUN"; return raw.toUpperCase(); } @@ -17715,10 +17719,45 @@ function formatPrCreate(value: unknown): string { ]); } +/** + * Header verdict for `ade prs checks`. + * + * ADE-135: the table alone is the bug. Three third-party bot rows each render + * `OK`, and a reader — human or agent — concludes the commit passed CI. The + * canonical rollup now travels with the rows as `checksStatus`/`checksCounts`, + * so the verdict leads. `not_run` is spelled out rather than leaked as a raw + * enum: it is the one status whose whole point is that nothing verified the + * commit. + */ +function formatPrChecksVerdict(value: unknown): string | null { + if (!isRecord(value)) return null; + const status = asString(value.checksStatus); + if (!status) return null; + // `none` means nothing reported and nothing was expected. Printing the bare + // enum beside an empty table reads as a value, not a sentence — the same + // complaint that motivates the `not_run` relabel below. + if (status === "none") return null; + const label = status === "not_run" ? "not run" : status; + const counts = isRecord(value.checksCounts) ? value.checksCounts : null; + const parts: string[] = []; + for (const noun of ["passing", "failing", "pending"] as const) { + const count = counts ? counts[noun] : null; + if (typeof count === "number" && count > 0) parts.push(`${count} ${noun}`); + } + const total = counts ? counts.total : null; + if (typeof total === "number" && total > 0) { + parts.push(`${total} check${total === 1 ? "" : "s"} reported`); + } + return parts.length > 0 ? `${label} (${parts.join(", ")})` : label; +} + function formatPrChecks(value: unknown): string { const checks = firstArray(value, ["checks", "items"]); const summary = isRecord(value) ? value.summary : null; - const header = summary + const verdict = formatPrChecksVerdict(value); + const header = verdict + ? `ADE PR checks - ${verdict}` + : summary ? `ADE PR checks - ${cell(summary, 80)}` : "ADE PR checks"; return `${header}\n${renderTable( diff --git a/apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx b/apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx index 3dcac25cb..69b398a3e 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx +++ b/apps/ade-cli/src/tuiClient/__tests__/Drawer.test.tsx @@ -633,6 +633,36 @@ describe("Drawer PR pill", () => { expect(frame).toContain("[#168 ≋2/3 ·4/6]"); }); + // ADE-135: the counts are producer-blind, so a PR whose only checks are + // preview/review bots arrives here as N/N. The pill must not spend a number + // on that at all — "no ci" is the fact. + it("renders no-ci in the PR pill when the rollup says nothing verified the commit", () => { + const frame = stripAnsi(render( + , + ).lastFrame() ?? ""); + + expect(frame).toContain("[#988 ·no ci]"); + expect(frame).not.toContain("3/3"); + }); + it("does not render closed or merged PR pills", () => { for (const state of ["closed", "merged"] as const) { const frame = stripAnsi(render( diff --git a/apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx b/apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx index 98803e1a9..396698406 100644 --- a/apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx +++ b/apps/ade-cli/src/tuiClient/__tests__/RightPane.test.tsx @@ -1041,6 +1041,35 @@ describe("RightPane lane-details", () => { expect(frame).not.toContain("RUN"); }); + // ADE-135: with three bot rows the counts read 0 passing / 0 failing / + // 0 pending / 3 total, and the old line called that "checks passing". + it("does not claim checks are passing when the rollup says CI never ran", () => { + const result = render( + , + ); + const frame = stripAnsi(result.lastFrame() ?? ""); + + expect(frame).toContain("CI not run"); + expect(frame).not.toContain("checks passing"); + expect(frame).not.toContain("passing"); + }); + it("shows the PR GitHub link when the PR row is selected", () => { const result = render( { expect(body).not.toContain("\"title\""); }); + // ADE-135: `/pr` leads with this summary, so the rollup has to be visible + // here — otherwise the reader's only checks signal is the row table, which is + // exactly what read green while nothing verified the commit. + it("surfaces the checks rollup and its reason in the PR summary", () => { + const body = formatPrSummary({ + id: "pr-988", + number: 988, + title: "Ship the thing", + state: "open", + checksStatus: "not_run", + checksReason: "3 checks reported, none from a CI provider. CI has not run on this commit.", + }); + + expect(body).toContain("checks not run"); + expect(body).toContain("none from a CI provider"); + expect(body).not.toContain("not_run"); + }); + it("renders a detached PR's lane as history, not as a live mapping", () => { const body = formatPrSummary({ id: "pr-9", @@ -223,6 +241,33 @@ describe("rightPaneFormatters", () => { expect(body).not.toContain("3 passing"); }); + it("refuses a green from third-party rows when the payload carries no rollup", () => { + // The production call site is `conn.actionList("pr","getChecks")`, which + // returns a BARE ARRAY — so the `checksStatus` field the test above relies + // on is always absent there. The row tally itself has to be producer-aware + // or the TUI prints "3 passing" for a commit nothing verified. + const body = formatPrChecks([ + { name: "CodeRabbit", status: "completed", conclusion: "success", appSlug: "coderabbitai" }, + { name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }, + { name: "Vercel Preview Comments", status: "completed", conclusion: "success", appSlug: "vercel" }, + ]); + + expect(body).toContain("CI: not run"); + expect(body).not.toContain("3 passing"); + expect(body).toContain("PR checks"); + }); + + it("still reports real CI as passing from a bare array", () => { + // The guard against over-correcting: a genuine Actions run must stay green + // on the same bare-array path. + const body = formatPrChecks([ + { name: "ci-pass", status: "completed", conclusion: "success", appSlug: "github-actions" }, + ]); + + expect(body).toContain("1 passing"); + expect(body).not.toContain("CI: not run"); + }); + it("reports a fully skipped suite as not run", () => { const body = formatPrChecks([ { name: "ci / unit", status: "completed", conclusion: "skipped" }, diff --git a/apps/ade-cli/src/tuiClient/app.tsx b/apps/ade-cli/src/tuiClient/app.tsx index 2385a4b35..65b79e624 100644 --- a/apps/ade-cli/src/tuiClient/app.tsx +++ b/apps/ade-cli/src/tuiClient/app.tsx @@ -57,6 +57,8 @@ import type { FeedbackPreparedDraft, FeedbackSubmission } from "../../../desktop import type { ProjectSecretsListResult, ProjectSecretValueResult } from "../../../desktop/src/shared/types/projectSecrets"; import type { SearchQueryResult, SearchResultItem } from "../../../desktop/src/shared/types/search"; import type { ChatTerminalPreviewResult, ChatTerminalSession, UsageSnapshot } from "../../../desktop/src/shared/types"; +import { rollupPrChecks } from "../../../desktop/src/shared/prChecksRollup"; +import type { PrChecksStatus } from "../../../desktop/src/shared/types/prs"; import { DEFAULT_CODEX_REASONING_EFFORT, approveToolUse, @@ -6071,6 +6073,7 @@ export function AdeCodeApp({ project, forceEmbedded, requireSocket, socketPath, checksPassed: number; checksTotal: number; checksPending: number; + checksStatus?: PrChecksStatus; checksFailed: number; } | null = null; if (activePr) { @@ -6092,17 +6095,30 @@ export function AdeCodeApp({ project, forceEmbedded, requireSocket, socketPath, let checksTotal = 0; let checksPending = 0; let checksFailed = 0; + let checksStatus: PrChecksStatus = "none"; if (prId) { - const checks = await conn.actionList>("pr", "getChecks", [prId]).catch(() => null); + const checks = await conn.actionList>("pr", "getChecks", [prId]).catch(() => null); if (!cancelled && Array.isArray(checks)) { - checksTotal = checks.length; - checksPassed = checks.filter((check) => check.status === "completed" && check.conclusion === "success").length; - checksFailed = checks.filter((check) => check.conclusion === "failure").length; - checksPending = checks.filter((check) => check.status !== "completed").length; + // ADE-135: this hand-rolled the tally the rest of the branch + // centralised, and it was producer-blind — three third-party + // successes rendered `3/3` in the green theme colour, which is + // the ticket's exact lie on the TUI. + const rollup = rollupPrChecks( + checks.map((check) => ({ + status: check.status ?? "", + conclusion: check.conclusion ?? null, + appSlug: check.appSlug ?? null, + })), + ); + checksTotal = rollup.counts.total; + checksPassed = rollup.counts.passing; + checksFailed = rollup.counts.failing; + checksPending = rollup.counts.pending; + checksStatus = rollup.status; } } if (number != null && url) { - pr = { number, state, url, checksPassed, checksTotal, checksPending, checksFailed }; + pr = { number, state, url, checksPassed, checksTotal, checksPending, checksFailed, checksStatus }; } } @@ -8464,6 +8480,15 @@ export function AdeCodeApp({ project, forceEmbedded, requireSocket, socketPath, let unsubscribe: (() => void) | null = null; const refreshPrsByLane = async () => { try { + // ADE-135: `PrLaneSummary` now carries the service's canonical + // `checksStatus` (set by `summaryToLanePrSummary`), so the pill gates on + // that instead of inferring a pass from `checksPassed === checksTotal`. + // An earlier revision joined a second unscoped `pr listAll` call for the + // same field: redundant, an extra whole-history serialization on a 30s + // refresh, and strictly less correct — projection-backed and detached + // lanes are absent from `pull_requests`, so their status came back + // undefined and fell through to exactly the producer-blind green this + // ticket exists to remove. const prs = await listPrsByLane(connection); if (cancelled) return; const next: Record = {}; @@ -8473,6 +8498,7 @@ export function AdeCodeApp({ project, forceEmbedded, requireSocket, socketPath, state: pr.state, checksPassed: pr.checksPassed, checksTotal: pr.checksTotal, + checksStatus: pr.checksStatus, stack: pr.stack ?? null, }; } diff --git a/apps/ade-cli/src/tuiClient/components/Drawer.tsx b/apps/ade-cli/src/tuiClient/components/Drawer.tsx index ddd72f867..7107394ec 100644 --- a/apps/ade-cli/src/tuiClient/components/Drawer.tsx +++ b/apps/ade-cli/src/tuiClient/components/Drawer.tsx @@ -23,6 +23,7 @@ import { } from "../drawerLayout"; import { Rail, statusGlyph, type StatusKind } from "./designKit"; import { sessionLifecycleMarker, type SessionLifecycleMarker } from "../sessionLifecycle"; +import type { PrChecksStatus } from "../../../../desktop/src/shared/types/prs"; export { visibleDrawerChatCount, visibleDrawerLaneCount }; @@ -38,6 +39,8 @@ export type DrawerPrSummary = { state: "open" | "closed" | "merged"; checksPassed: number; checksTotal: number; + /** ADE-135 canonical rollup; `passed === total` is not proof of a pass. */ + checksStatus?: PrChecksStatus; stack?: GitHubPrStackMembership | null; }; @@ -672,11 +675,23 @@ function LaneCard({ function formatPrPillText(pr: DrawerPrSummary): string { const stack = pr.stack ? ` ≋${pr.stack.position}/${pr.stack.size}` : ""; - return `[#${pr.number}${stack} ·${pr.checksPassed}/${pr.checksTotal}]`; + // ADE-135: the text twin of `PrPill` — it feeds the mouse hit-test width, so + // it has to say the same thing the pill renders, "no ci" included. A bare + // `3/3` here would put the ticket's lie back in the drawer. + const checks = pr.checksStatus === "not_run" + ? "no ci" + : `${pr.checksPassed}/${pr.checksTotal}`; + return `[#${pr.number}${stack} ·${checks}]`; } function PrPill({ pr }: { pr: DrawerPrSummary }) { - const checksColor = pr.checksPassed === pr.checksTotal ? theme.color.running : theme.color.attention; + // ADE-135: producer-blind counts can read N/N while nothing verified the + // commit, so the rollup gates the green. + const checksColor = pr.checksStatus === "not_run" + ? theme.color.t4 + : pr.checksPassed === pr.checksTotal + ? theme.color.running + : theme.color.attention; return ( [# @@ -689,8 +704,14 @@ function PrPill({ pr }: { pr: DrawerPrSummary }) { ) : null} · - {pr.checksPassed} - /{pr.checksTotal} + {pr.checksStatus === "not_run" ? ( + no ci + ) : ( + <> + {pr.checksPassed} + /{pr.checksTotal} + + )} ] ); diff --git a/apps/ade-cli/src/tuiClient/components/RightPane.tsx b/apps/ade-cli/src/tuiClient/components/RightPane.tsx index ef375082c..42e113a1f 100644 --- a/apps/ade-cli/src/tuiClient/components/RightPane.tsx +++ b/apps/ade-cli/src/tuiClient/components/RightPane.tsx @@ -237,6 +237,10 @@ type LaneDetailsPr = NonNullable 0) return theme.color.running; if (pr.checksFailed > 0) return theme.color.error; + // ADE-135: an unverified commit is not a quiet neutral state — it is the + // finding — so it takes the attention colour rather than the muted one a + // clean pass gets. + if (pr.checksStatus === "not_run") return theme.color.attention; return theme.color.t3; } @@ -254,9 +258,21 @@ function formatPrActivity(pr: LaneDetailsPr): string { if (pr.checksFailed > 0) { return `${pr.checksFailed} check${pr.checksFailed === 1 ? "" : "s"} failing`; } - if (pr.checksTotal > 0) { + // ADE-135: this said "checks passing" for any non-empty row list, which is + // exactly the ticket — on PR #988 three bot rows made `checksTotal` 3 while + // `checksPassed` stayed 0, and the line claimed a pass nothing had earned. + // The rollup, not the row count, decides. + if (pr.checksStatus === "not_run") { + return pr.checksTotal > 0 + ? `CI not run · ${pr.checksTotal} check${pr.checksTotal === 1 ? "" : "s"}, none from CI` + : "CI not run"; + } + if (pr.checksPassed > 0) { return "checks passing"; } + if (pr.checksTotal > 0) { + return `${pr.checksTotal} check${pr.checksTotal === 1 ? "" : "s"}`; + } if (pr.state === "merged") return "merged"; if (pr.state === "closed") return "closed"; return "open"; @@ -607,7 +623,11 @@ function LaneDetailsPane({ {content.pr.checksTotal > 0 ? ( - {content.pr.checksPassed}/{content.pr.checksTotal} passing + {/* ADE-135: "0/3 passing" would still invite the reader to + treat the 3 as CI. Say what the rollup found instead. */} + {content.pr.checksStatus === "not_run" + ? `${content.pr.checksTotal} check${content.pr.checksTotal === 1 ? "" : "s"}, no CI` + : `${content.pr.checksPassed}/${content.pr.checksTotal} passing`} ) : null} @@ -1256,7 +1276,10 @@ function ChatInfoPrBlock({ info, brandColor, width }: { info: ChatInfoSnapshot; : pr.state === "merged" ? theme.color.violet : theme.color.t4; - const checksColor = pr.checksTotal === 0 + // ADE-135: `passed === total` is not proof of a pass — on a PR whose only + // checks are preview/review bots the counts are producer-blind. The rollup + // decides whether green is earned. + const checksColor = pr.checksTotal === 0 || pr.checksStatus === "not_run" ? theme.color.t4 : pr.checksPassed === pr.checksTotal ? theme.color.running @@ -1269,7 +1292,9 @@ function ChatInfoPrBlock({ info, brandColor, width }: { info: ChatInfoSnapshot; {pr.checksTotal > 0 ? ( <> {" · checks "} - {`${pr.checksPassed}/${pr.checksTotal}`} + + {pr.checksStatus === "not_run" ? "not run" : `${pr.checksPassed}/${pr.checksTotal}`} + ) : null} diff --git a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts index e2750b9e0..fbaba231e 100644 --- a/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts +++ b/apps/ade-cli/src/tuiClient/rightPaneFormatters.ts @@ -1,5 +1,6 @@ import { buildDeeplink } from "../../../desktop/src/shared/deeplinks"; import { buildWebClientUrl } from "../../../desktop/src/shared/webClientUrl"; +import { NO_CI_REASON, rollupPrChecks } from "../../../desktop/src/shared/prChecksRollup"; type JsonRecord = Record; @@ -117,9 +118,14 @@ function statusWord(status: unknown, conclusion?: unknown): "OK" | "FAIL" | "WAI * rewritten: it is the one value a reader would otherwise see as a raw enum, * and "not run" is the whole point — nothing verified the commit (ADE-135). */ -function checksStatusWord(status: string | null): string | null { +function checksStatusLabel(status: string | null): string | null { if (!status) return null; - return status === "not_run" ? "CI: not run" : status; + return status === "not_run" ? "not run" : status; +} + +function checksStatusWord(status: string | null): string | null { + const label = checksStatusLabel(status); + return label && status === "not_run" ? `CI: ${label}` : label; } function formatCount(noun: string, count: number): string { @@ -188,6 +194,15 @@ export function formatPrSummary(value: unknown): string { ?? pickString(pr, ["adeUrl", "adePrUrl"]) ?? derivedAdeUrl; const mergeable = pickString(pr, ["mergeable", "mergeStateStatus"]); + // ADE-135: the PR summary is the first thing `/pr` prints, and it used to say + // nothing at all about CI — so the reader's only checks signal was the row + // table below it, which is precisely what read green while nothing verified + // the commit. The service's rollup (and its one-sentence reason) belongs here. + const checksStatus = checksStatusLabel(pickString(pr, ["checksStatus"])); + const checksReason = pickString(pr, ["checksReason"]); + const checks = checksStatus + ? `${checksStatus}${checksReason ? ` — ${checksReason}` : ""}` + : null; const rows = [ `#${number ?? id ?? "?"} · ${state}${draft}`, title, @@ -196,6 +211,7 @@ export function formatPrSummary(value: unknown): string { lane ? `lane ${lane}` : null, head || base ? `branch ${head ?? "unknown"}${base ? ` -> ${base}` : ""}` : null, mergeable ? `merge ${mergeable}` : null, + checks ? `checks ${checks}` : null, githubUrl ? `github ${githubUrl}` : null, fallbackUrl && fallbackUrl !== githubUrl ? `url ${fallbackUrl}` : null, adeUrl ? `ade ${adeUrl}` : null, @@ -344,22 +360,24 @@ export function formatPrChecks(value: unknown): string { const checks = firstRecordArray(value, ["checks", "items", "results"]); if (!checks.length) { return rollup === "not_run" - ? `CI: not run — ${reason ?? "No CI has run on this commit."}` + ? `CI: not run — ${reason ?? NO_CI_REASON}` : "No PR checks."; } - let ok = 0; - let fail = 0; - let wait = 0; - for (const check of checks) { - const status = statusWord(check.status, check.conclusion); - if (status === "OK") ok += 1; - else if (status === "FAIL") fail += 1; - else if (status === "WAIT") wait += 1; - } - // ADE-135: rows can all be green and still verify nothing (third-party apps - // only, or an all-skipped suite). The rollup is the authority when it says - // so; otherwise fall back to "nothing passed" as the same signal. - const notRun = rollup === "not_run" || (ok === 0 && fail === 0 && wait === 0); + // ADE-135: counting the rows directly is producer-blind — three third-party + // successes tallied as "3 passing". `rollupPrChecks` applies the same CI + // producer rule as every other surface. The payload's own `checksStatus` + // still wins when present, since it also knows about required contexts, + // which these rows cannot see. + const rows = checks.map((check) => ({ + status: pickString(check, ["status"]) ?? "", + conclusion: pickString(check, ["conclusion"]), + appSlug: pickString(check, ["appSlug"]), + })); + const rowRollup = rollupPrChecks(rows); + const ok = rowRollup.counts.passing; + const fail = rowRollup.counts.failing; + const wait = rowRollup.counts.pending; + const notRun = rollup === "not_run" || rowRollup.status === "not_run"; const summary = notRun ? `CI: not run${reason ? ` — ${reason}` : ""}` : [ok ? `${ok} passing` : null, fail ? `${fail} failing` : null, wait ? `${wait} pending` : null] diff --git a/apps/ade-cli/src/tuiClient/types.ts b/apps/ade-cli/src/tuiClient/types.ts index 9af28a957..2d78357ee 100644 --- a/apps/ade-cli/src/tuiClient/types.ts +++ b/apps/ade-cli/src/tuiClient/types.ts @@ -25,6 +25,7 @@ import type { ExternalSessionSummary, } from "../../../desktop/src/shared/types/externalSessions"; import type { LaneSummary } from "../../../desktop/src/shared/types/lanes"; +import type { PrChecksStatus } from "../../../desktop/src/shared/types/prs"; import type { UsageProviderSource, UsageProviderState } from "../../../desktop/src/shared/types/usage"; import type { BufferedEvent } from "../eventBuffer"; import type { HelpGroup } from "./helpIndex"; @@ -170,6 +171,8 @@ export type ChatInfoPrSummary = { state: "open" | "merged" | "closed"; checksPassed: number; checksTotal: number; + /** ADE-135 canonical rollup; `passed === total` is not proof of a pass. */ + checksStatus?: PrChecksStatus; }; export type ChatInfoSnapshot = { @@ -376,6 +379,8 @@ export type RightPaneContent = checksTotal: number; checksPending: number; checksFailed: number; + /** ADE-135 canonical rollup; `passed === total` is not proof of a pass. */ + checksStatus?: PrChecksStatus; } | null; chats: { active: number; diff --git a/apps/desktop/src/main/services/prs/prChatCards.test.ts b/apps/desktop/src/main/services/prs/prChatCards.test.ts index 20058b6c2..c5abcc05c 100644 --- a/apps/desktop/src/main/services/prs/prChatCards.test.ts +++ b/apps/desktop/src/main/services/prs/prChatCards.test.ts @@ -83,9 +83,10 @@ function check(name: string, overrides: Partial = {}): PrCheck { detailsUrl: null, startedAt: null, completedAt: null, - // Unattributed by default: that is what a preview/review bot looks like to - // the card, and it must never land in the CI group. - appSlug: null, + // Default to a real CI producer. An ABSENT slug is deliberately treated as + // CI-eligible (legacy rows carry no slug at all), so a fixture that means + // "preview/review bot" has to say which bot — exactly as GitHub does. + appSlug: "github-actions", ...overrides, }; } @@ -122,7 +123,7 @@ describe("PR chat cards", () => { }); it("builds one live CI episode keyed by head and attempt", () => { - const card = buildPrCiCard({ pr: pr(), runs: [run()], checks: [check("external")] }); + const card = buildPrCiCard({ pr: pr(), runs: [run()], checks: [check("external", { appSlug: "vercel" })] }); expect(card).toMatchObject({ cardId: "pr-ci:pr-7:abc123:2", variant: "pr_ci", @@ -142,7 +143,7 @@ describe("PR chat cards", () => { const card = buildPrCiCard({ pr: pr({ checksStatus: "not_run" }), runs: [], - checks: [check("Vercel"), check("CodeRabbit"), check("coverage-bot")], + checks: [check("Vercel", { appSlug: "vercel" }), check("CodeRabbit", { appSlug: "coderabbitai" }), check("coverage-bot", { appSlug: "mintlify" })], }); expect(card.state).toBe("terminal"); expect(card.title).toBe("CI has not run"); @@ -160,7 +161,7 @@ describe("PR chat cards", () => { const card = buildPrCiCard({ pr: pr({ checksStatus: "passing" }), runs: [], - checks: [check("buildkite/ci", { id: null, appSlug: "commit_status" }), check("Vercel")], + checks: [check("buildkite/ci", { id: null, appSlug: "commit_status" }), check("Vercel", { appSlug: "vercel" })], }); expect(card.progress).toEqual({ passed: 1, failed: 0, running: 0, queued: 0 }); expect(card.metrics).toContainEqual({ label: "other checks", value: "1", tone: "neutral" }); @@ -224,7 +225,7 @@ describe("PR chat cards", () => { checksReason: "3 checks reported, none from a CI provider. CI has not run on this commit.", }), runs: [], - checks: [check("Vercel"), check("CodeRabbit")], + checks: [check("Vercel", { appSlug: "vercel" }), check("CodeRabbit", { appSlug: "coderabbitai" })], }); expect(notRun.title).toBe("CI has not run"); expect(notRun.subtitle).toContain("none from a CI provider"); diff --git a/apps/desktop/src/main/services/prs/prChatCards.ts b/apps/desktop/src/main/services/prs/prChatCards.ts index 40f2f1b1a..85b83b2b8 100644 --- a/apps/desktop/src/main/services/prs/prChatCards.ts +++ b/apps/desktop/src/main/services/prs/prChatCards.ts @@ -10,6 +10,7 @@ import type { PrActionJob, PrActionRun, PrCheck, + PrChecksStatus, PrReview, PrReviewThread, PrSummary, @@ -149,10 +150,12 @@ function countBuckets(entries: RankedItem[]): { progress: AdeCardProgress; other * Actions jobs are CI by construction — they only exist because a workflow run * produced them. Check rows are classified by `isCiProducerCheck`, the same * predicate the rollup uses, so the headline and the rows can never go back to - * disagreeing about what counts as CI. A check with no `appSlug` (older row, or - * a producer GitHub did not name) lands in "Other": an unattributable success - * is not evidence that CI ran. Any check whose name a job already covers is - * dropped so the same work is not counted twice once both endpoints answered. + * disagreeing about what counts as CI. A check GitHub names as a preview or + * review bot lands in "Other" and cannot carry the headline; a slug-less row is + * CI-eligible, because `appSlug` only started being populated in this change + * and failing legacy rows closed would report "CI has not run" for every one of + * them. Any check whose name a job already covers is dropped so the same work + * is not counted twice once both endpoints answered. */ function groupCheckItems(runs: PrActionRun[], checks: PrCheck[]): { ci: RankedItem[]; @@ -183,6 +186,28 @@ export function selectPrCardSession( ))[0] ?? null; } +/** + * Presentation per rollup state, as one exhaustive map rather than three + * parallel ternary ladders over the same discriminant. + * + * `not_run` and `none` are deliberately neutral: the card reports an absence, + * and an absence is not an alarm — only a real red job earns the warning tone. + * `none` used to fall through to "CI is running", which was its own small lie. + * + * Being a `Record` makes the next state a compile error + * here instead of three silent `else` arms. + */ +const CI_CARD_PRESENTATION: Record< + PrChecksStatus, + { title: string; tone: "success" | "warning" | "accent" | "neutral"; state: "live" | "terminal" } +> = { + passing: { title: "CI passed", tone: "success", state: "terminal" }, + failing: { title: "CI failed", tone: "warning", state: "terminal" }, + pending: { title: "CI is running", tone: "accent", state: "live" }, + not_run: { title: "CI has not run", tone: "neutral", state: "terminal" }, + none: { title: "No checks reported", tone: "neutral", state: "terminal" }, +}; + export function buildPrCiCard(args: { pr: PrSummary; runs: PrActionRun[]; @@ -214,27 +239,7 @@ export function buildPrCiCard(args: { const { progress, other } = countBuckets(groups.ci); const ciTotal = groups.ci.length; const otherTotal = groups.other.length; - const state = pr.checksStatus === "pending" ? "live" : "terminal"; - const title = pr.checksStatus === "passing" - ? "CI passed" - : pr.checksStatus === "failing" - ? "CI failed" - : pr.checksStatus === "pending" - ? "CI is running" - : pr.checksStatus === "not_run" - // Checks exist or are expected, but nothing verified this commit. The - // old fallback said "CI is running" here, which was its own lie. - ? "CI has not run" - : "No checks reported"; - // `not_run`/`none` are neutral on purpose: the card reports an absence, and - // an absence is not an alarm. Only a real red job earns the amber treatment. - const tone = pr.checksStatus === "failing" - ? "warning" - : pr.checksStatus === "passing" - ? "success" - : pr.checksStatus === "pending" - ? "accent" - : "neutral"; + const { title, tone, state } = CI_CARD_PRESENTATION[pr.checksStatus]; const reason = pr.checksReason?.trim() || null; const missingRequired = (pr.checksMissingRequired ?? []).map((c) => c.trim()).filter(Boolean); // A partial response is still degraded: the surviving endpoint's rows remain diff --git a/apps/desktop/src/main/services/prs/prService.test.ts b/apps/desktop/src/main/services/prs/prService.test.ts index e6971e99f..24bc44924 100644 --- a/apps/desktop/src/main/services/prs/prService.test.ts +++ b/apps/desktop/src/main/services/prs/prService.test.ts @@ -843,6 +843,30 @@ describe("prService.getForLane", () => { }); }); + it("does not report a lane PR as fully passed when only bots reported", async () => { + // ADE-135: `checksPassed` counted any `success` row with no producer + // awareness, so a lane whose PR had only CodeRabbit/Vercel checks reported + // N/N — and every consumer inferring a pass from `passed === total` + // (the ade code drawer, the lane rail) painted it green. + const lane = makeFakeLane({ id: "lane-bots", branchRef: "refs/heads/bots-feature" }); + const service = buildGetForLaneService(lane, [ + makePrRow({ + lane_id: lane.id, + state: "open", + github_pr_number: 988, + head_branch: "bots-feature", + checks_status: "not_run", + }), + ]); + + const rows = await service.listPrsByLane(); + const row = rows.find((entry) => entry.laneId === lane.id); + + expect(row, "lane PR summary must exist").toBeTruthy(); + expect(row!.checksStatus).toBe("not_run"); + expect(row!.checksPassed).toBe(0); + }); + it("keeps listPrsByLane in parity with the branch-first per-lane resolver", async () => { const mappedLane = makeFakeLane({ id: "lane-mapped", branchRef: "refs/heads/mapped-feature" }); const projectedLane = makeFakeLane({ id: "lane-projected", branchRef: "refs/heads/projected-feature" }); @@ -872,6 +896,10 @@ describe("prService.getForLane", () => { state: pr.state === "draft" ? "open" : pr.state, checksPassed: 0, checksTotal: 0, + // ADE-135: the rollup rides along so consumers never infer a pass from + // `checksPassed === checksTotal`. With no checks fetched it mirrors the + // row's own status. + checksStatus: pr.checksStatus, stack: pr.stack ?? null, })); @@ -2867,6 +2895,77 @@ describe("prService.getStatus", () => { vi.clearAllMocks(); }); + it("keeps the previous checks rollup when the check-runs fetch fails", async () => { + // ADE-135: `bestEffort` turns a 403/rate-limit on /check-runs into `[]`, + // which is byte-identical to "this commit has no checks". Recomputing from + // that flipped a green PR to `not_run` and persisted it — and the row + // replicates to iOS, so a transient GitHub blip became a durable lie. + const row = makePrRow({ + id: "pr-fetchfail", + github_pr_number: 91, + checks_status: "passing", + checks_reason: null, + }); + const db = makeMockDb(); + installPullRequestRowStore(db, [row]); + let checkRunCalls = 0; + const githubService = makeGithubService({ + apiRequest: vi.fn(async (args: { method?: string; path: string }) => { + if (args.path === "/repos/test-owner/test-repo/pulls/91") { + return { + data: makeGitHubPull({ + number: 91, + html_url: row.github_url, + title: row.title, + mergeable: true, + mergeable_state: "clean", + head: { ref: "my-feature", sha: "head-sha" }, + base: { ref: "main", sha: "base-sha" }, + }), + }; + } + if (args.path === "/repos/test-owner/test-repo/commits/head-sha/status") { + return { data: { state: "", statuses: [] } }; + } + if (args.path === "/repos/test-owner/test-repo/commits/head-sha/check-runs") { + checkRunCalls += 1; + throw new Error("403 rate limit exceeded"); + } + if (args.path === "/repos/test-owner/test-repo/pulls/91/reviews") return { data: [] }; + if (args.path.includes("/compare/")) return { data: { behind_by: 0 } }; + if (args.path.includes("/rules/branches/")) return { data: [] }; + if (args.path.includes("/protection")) return { data: {} }; + if (args.method === "POST" && args.path === "/graphql") { + return { + data: { + data: { + repository: { + viewerPermission: "WRITE", + pullRequest: { + mergeable: "MERGEABLE", + mergeStateStatus: "CLEAN", + reviewDecision: null, + headRefOid: "head-sha", + baseRef: { branchProtectionRule: null }, + latestOpinionatedReviews: { nodes: [] }, + }, + }, + }, + }, + }; + } + return { data: {} }; + }), + }); + const { service } = buildService({ db, githubService }); + + const status = await service.getStatus("pr-fetchfail"); + + expect(checkRunCalls, "check-runs must actually have been attempted").toBe(1); + expect(status.checksStatus).toBe("passing"); + expect(status.checksStatus).not.toBe("not_run"); + }); + it("returns promptly without polling when GitHub mergeability is unknown", async () => { // getStatus must stay cheap so the renderer can re-poll: it does NOT block on // the long mergeability wait. When mergeability is still unknown it flags diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 1bc0756f3..b2b726be0 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -155,8 +155,8 @@ import type { IssueTracker } from "../cto/issueTracker"; import type { LinearLiveStatusService } from "../cto/linearLiveStatusService"; import { publishLinearPrCard } from "../cto/linearLaneCardService"; import { parseSyntheticGithubPrId, syntheticGithubPrId } from "../../../shared/types/prs"; -import { rollupChecks } from "../../../shared/prChecksRollup"; -import type { ChecksRollupCheckRun, ChecksRollupCommitStatus } from "../../../shared/prChecksRollup"; +import { COMMIT_STATUS_APP_SLUG, rollupChecks, rollupPrChecks } from "../../../shared/prChecksRollup"; +import type { ChecksRollup, ChecksRollupCheckRun, ChecksRollupCommitStatus } from "../../../shared/prChecksRollup"; import { createRequiredChecksResolver } from "./requiredChecks"; import { spawn } from "node:child_process"; import { runGit, runGitMergeTree, runGitOrThrow } from "../git/git"; @@ -1023,6 +1023,40 @@ function toRollupCommitStatuses( })); } +/** + * Earliest moment anything reported on this commit. + * + * ADE-135: the grace window originally keyed off the PR's `updated_at`, but + * GitHub bumps that on every comment, label and review — including the comments + * posted by the very bots (CodeRabbit, Vercel) whose presence is the finding. + * Each bot comment reset the age below the grace window and demoted a + * months-old unverified commit back to "pending / has not run *yet*". A check's + * own start time cannot be moved by a comment, so it is the honest clock. + */ +function earliestReportAt( + checkRuns: any[], + combinedStatus: { statuses?: Array> } | undefined, +): string | null { + let earliest: number | null = null; + let earliestRaw: string | null = null; + const consider = (raw: string | null | undefined) => { + if (!raw) return; + const parsed = Date.parse(raw); + if (!Number.isFinite(parsed)) return; + if (earliest == null || parsed < earliest) { + earliest = parsed; + earliestRaw = raw; + } + }; + if (Array.isArray(checkRuns)) { + for (const run of checkRuns) consider(asString(run?.started_at) || null); + } + if (Array.isArray(combinedStatus?.statuses)) { + for (const status of combinedStatus!.statuses!) consider(asString(status?.created_at) || null); + } + return earliestRaw; +} + function commitAgeMs(timestamp: string | null | undefined, nowMs: number): number | null { if (!timestamp) return null; const parsed = Date.parse(timestamp); @@ -1210,6 +1244,11 @@ function hasMaterialSummaryChange(row: PullRequestRow, summary: PrSummary): bool // in *why* CI is not green is a material change even when the state holds. || (Object.prototype.hasOwnProperty.call(summary, "checksReason") && (row.checks_reason ?? null) !== (summary.checksReason ?? null)) + // A required context appearing or finally reporting changes the ghost rows + // the PR detail renders, even when the status and reason string both hold. + || (Object.prototype.hasOwnProperty.call(summary, "checksMissingRequired") + && parseMissingRequired(row.checks_missing_required).join("\u0000") + !== (summary.checksMissingRequired ?? []).join("\u0000")) || row.review_status !== summary.reviewStatus || (row.title ?? "") !== summary.title || row.base_branch !== summary.baseBranch @@ -1895,6 +1934,8 @@ export function createPrService({ baseBranch: row.base_branch ?? "", headBranch: row.head_branch ?? "", checksStatus: "none", + checksReason: null, + checksMissingRequired: [], reviewStatus: "none", additions: 0, deletions: 0, @@ -2065,12 +2106,19 @@ export function createPrService({ const summaryToLanePrSummary = (pr: PrSummary, checks: PrCheck[] = []): PrLaneSummary => { const state: PrLaneSummary["state"] = pr.state === "merged" || pr.state === "closed" ? pr.state : "open"; + // ADE-135: `checksPassed` used to count any `success` row, so a lane whose + // PR had only preview/review bots reported N/N and every consumer of this + // shape rendered it green. The counts are producer-aware now, and the + // canonical rollup rides along so a consumer never has to infer a verdict + // from `passed === total`. + const rollup = rollupPrChecks(checks); return { laneId: pr.laneId, number: Number(pr.githubPrNumber), state, - checksPassed: checks.filter((check) => check.status === "completed" && check.conclusion === "success").length, - checksTotal: checks.length, + checksPassed: rollup.counts.passing, + checksTotal: rollup.counts.total, + checksStatus: checks.length > 0 ? rollup.status : pr.checksStatus, stack: pr.stack ?? null, }; }; @@ -3057,6 +3105,8 @@ export function createPrService({ baseBranch: asString(rawPr?.base?.ref) || branchNameFromRef(lane.baseRef), headBranch, checksStatus: "none", + checksReason: null, + checksMissingRequired: [], reviewStatus: "none", additions: Number(rawPr?.additions ?? 0), deletions: Number(rawPr?.deletions ?? 0), @@ -4693,6 +4743,16 @@ export function createPrService({ return Array.isArray(data?.check_runs) ? data.check_runs : []; }; + /** Last-known rollup for a PR row, so a failed checks fetch can hold it. */ + const previousChecksRollup = (prId: string): ChecksRollup => { + const row = getRowById(prId); + return { + status: (row?.checks_status as PrChecksStatus | null) ?? "none", + reason: row?.checks_reason ?? null, + missingRequiredContexts: parseMissingRequired(row?.checks_missing_required), + }; + }; + const requiredChecksResolver = createRequiredChecksResolver({ apiRequest: (options) => githubService.apiRequest(options), logger, @@ -4701,8 +4761,9 @@ export function createPrService({ /** * ADE-135: the single checks derivation. Both the poller and the webhook path * land here — `ingestGithubWebhook` uses its payload only to resolve which PR - * changed and then re-derives through `computeStatus`, so there is exactly one - * place where "is this commit verified?" gets answered. + * changed and hands those ids to the poller (`onPrStateIngested` → + * `reconcilePrs`), which re-derives through `computeStatus`. So there is + * exactly one place where "is this commit verified?" gets answered. */ const deriveChecksRollup = async (args: { repo: GitHubRepoRef; @@ -4711,13 +4772,22 @@ export function createPrService({ combinedStatus: { state: string; statuses: Array<{ context: string; state: string }> }; mergeStateBlocked: boolean; /** - * PR `updated_at`. An approximation of head-commit age: GitHub bumps it on - * every `synchronize`, which is precisely the event that resets CI. Only - * used to choose between "has not run yet" and "has not run", so being a - * few seconds off changes wording, never state. + * Last-resort clock, used only when nothing has reported on the commit at + * all. PR `updated_at` is unreliable on its own — GitHub bumps it on every + * comment, not just on `synchronize` — so `earliestReportAt` is preferred + * whenever any check or status carries a timestamp. */ headActivityAt: string | null; + /** + * `bestEffort` turns a 403/rate-limit on /check-runs into `[]`, which is + * byte-identical to "this commit has no checks". Recomputing from that + * would flip a green PR to `not_run` and persist it to every surface, + * so a failed fetch keeps whatever we last knew. + */ + checksFetchFailed?: boolean; + previous: ChecksRollup; }) => { + if (args.checksFetchFailed) return args.previous; const required = await requiredChecksResolver .resolve(args.repo, args.baseBranch, args.mergeStateBlocked) .catch(() => ({ contexts: null, source: "unavailable" as const })); @@ -4725,9 +4795,8 @@ export function createPrService({ checkRuns: toRollupCheckRuns(args.checkRuns), commitStatuses: toRollupCommitStatuses(args.combinedStatus?.statuses), requiredContexts: required.contexts, - requiredSource: required.source, mergeStateBlocked: args.mergeStateBlocked, - headCommitAgeMs: commitAgeMs(args.headActivityAt, Date.now()), + headCommitAgeMs: commitAgeMs(earliestReportAt(args.checkRuns, args.combinedStatus) ?? args.headActivityAt, Date.now()), }); }; @@ -4774,10 +4843,11 @@ export function createPrService({ const requestedReviewers = Array.isArray(pr?.requested_reviewers) ? pr.requested_reviewers.map((u: any) => asString(u?.login)).filter(Boolean) : []; const requestedTeams = Array.isArray(pr?.requested_teams) ? pr.requested_teams.map((team: any) => asString(team?.slug)).filter(Boolean) : []; + let checkRunsFetchFailed = false; const [combinedStatus, checkRuns, reviews, compare] = shouldFetchLiveStatus ? await Promise.all([ headSha ? fetchCombinedStatus(repo, headSha) : Promise.resolve({ state: "", statuses: [] }), - headSha ? bestEffort("refreshOne.fetchCheckRuns", fetchCheckRuns(repo, headSha), [] as any[]) : Promise.resolve([]), + headSha ? bestEffort("refreshOne.fetchCheckRuns", fetchCheckRuns(repo, headSha), [] as any[], () => { checkRunsFetchFailed = true; }) : Promise.resolve([]), bestEffort("refreshOne.fetchReviews", fetchReviews(repo, Number(row.github_pr_number)), []), baseSha && headSha ? bestEffort("refreshOne.fetchCompare", fetchCompare(repo, baseSha, headSha), { behindBy: null as number | null }) : Promise.resolve({ behindBy: null as number | null }) ]) @@ -4804,6 +4874,12 @@ export function createPrService({ // "no corroboration", never as "not blocked". mergeStateBlocked: false, headActivityAt: asString(pr?.updated_at) || row.updated_at || null, + checksFetchFailed: checkRunsFetchFailed, + previous: { + status: (row.checks_status as PrChecksStatus | null) ?? "none", + reason: row.checks_reason ?? null, + missingRequiredContexts: parseMissingRequired(row.checks_missing_required), + }, }) : { status: (row.checks_status as PrChecksStatus | null) ?? "none", @@ -5095,9 +5171,10 @@ export function createPrService({ const baseSha = asString(pr?.base?.sha); const mergeConflicts = mergeConflictsFromPull(pr); + let checkRunsFetchFailed = false; const [combinedStatus, checkRuns, reviews, compare, mergeState] = await Promise.all([ restHeadSha ? fetchCombinedStatus(repo, restHeadSha) : Promise.resolve({ state: "", statuses: [] }), - restHeadSha ? bestEffort("computeStatus.fetchCheckRuns", fetchCheckRuns(repo, restHeadSha), [] as any[]) : Promise.resolve([]), + restHeadSha ? bestEffort("computeStatus.fetchCheckRuns", fetchCheckRuns(repo, restHeadSha), [] as any[], () => { checkRunsFetchFailed = true; }) : Promise.resolve([]), bestEffort("computeStatus.fetchReviews", fetchReviews(repo, prNumber), []), baseSha && restHeadSha ? bestEffort("computeStatus.fetchCompare", fetchCompare(repo, baseSha, restHeadSha), { behindBy: null as number | null }) : Promise.resolve({ behindBy: null as number | null }), fetchMergeStateViaGraphql(repo, prNumber), @@ -5123,6 +5200,8 @@ export function createPrService({ combinedStatus, mergeStateBlocked: (mergeState?.mergeStateStatus ?? "").toLowerCase() === "blocked", headActivityAt: asString(pr?.updated_at) || null, + checksFetchFailed: checkRunsFetchFailed, + previous: previousChecksRollup(prId), }); const checksStatus = checksRollup.status; const reviewStatus = computeReviewStatus({ requestedReviewers, requestedTeams, reviewStatesByUser }); @@ -5259,7 +5338,7 @@ export function createPrService({ completedAt: s.updated_at ?? null, // Legacy commit statuses are how Jenkins/Buildkite/CircleCI report, so // they count as CI even though they carry no app slug. - appSlug: "commit_status" + appSlug: COMMIT_STATUS_APP_SLUG }); } @@ -6452,6 +6531,8 @@ export function createPrService({ baseBranch, headBranch, checksStatus: "none", + checksReason: null, + checksMissingRequired: [], reviewStatus: "none", additions: Number(pr?.additions ?? 0), deletions: Number(pr?.deletions ?? 0), @@ -6586,6 +6667,8 @@ export function createPrService({ baseBranch, headBranch, checksStatus: "none", + checksReason: null, + checksMissingRequired: [], reviewStatus: "none", additions: Number(pr?.additions ?? 0), deletions: Number(pr?.deletions ?? 0), diff --git a/apps/desktop/src/renderer/components/app/prToastPresentation.ts b/apps/desktop/src/renderer/components/app/prToastPresentation.ts index a71199c4f..8b4e654e4 100644 --- a/apps/desktop/src/renderer/components/app/prToastPresentation.ts +++ b/apps/desktop/src/renderer/components/app/prToastPresentation.ts @@ -12,7 +12,13 @@ function compactLabel(value: string | null | undefined): string | null { /** * ADE-135: `not_run` means nothing verified the head commit. The only * checks-derived success tone is `merge_ready`, so that is the one that must - * never go green on an unverified commit — a green toast is exactly the signal + * never go green on an unverified commit. + * + * Defence in depth, deliberately: `prPollingService.isMergeReady` already + * requires `checksStatus === "passing"`, so today this branch cannot fire. + * It exists so that relaxing that upstream predicate cannot silently + * reintroduce a green "ready to merge" toast on an unverified commit — do not + * read its tests as coverage of a live path — a green toast is exactly the signal * a human (or a `/ship` loop) reads as "the suite is fine". It drops to `info` * rather than `danger`: absence is a finding, not a failure. The lifecycle * kinds (opened / reopened / merged) say nothing about CI and keep their tone. diff --git a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx index 66474f1ad..5868949c1 100644 --- a/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatGitToolbar.tsx @@ -21,7 +21,8 @@ import { formatPrBadgeLabel } from "../prs/shared/prFormatters"; import { buildPrsRouteSearch } from "../prs/prsRouteState"; import { useAppStore } from "../../state/appStore"; import { refreshLinkedPrCoalesced } from "../../lib/prReadCache"; -import { pipelineStateOf } from "../../../shared/prPipelineState"; +import { rollupPrChecks } from "../../../shared/prChecksRollup"; +import type { PrChecksStatus } from "../../../shared/types/prs"; import { GitHubStackBadge } from "../prs/shared/GitHubStackBadge"; // --------------------------------------------------------------------------- @@ -98,34 +99,24 @@ function formatRelativeTime(iso: string | null | undefined): string | null { return new Date(ts).toLocaleDateString(undefined, { month: "short", day: "numeric" }); } +/** + * ADE-135: this carried its own tally and was producer-blind, so a PR whose + * only checks were CodeRabbit and Vercel rendered a green "3" here — the + * ticket's bug, beside a pill that had already been fixed to say "not run". + * The shared row rollup decides; this only reshapes the counts for the JSX. + */ function summarizeChecks( checks: PrCheck[], -): { passed: number; failed: number; running: number; skipped: number; total: number } { - let passed = 0; - let failed = 0; - let running = 0; - // ADE-135: `skipped` used to be counted as `passed`, so a PR whose whole - // suite was skipped rendered a green "3" here. A skipped job verified - // nothing; it gets its own bucket and never colours the pill green. - let skipped = 0; - for (const c of checks) { - switch (pipelineStateOf(c)) { - case "running": - case "queued": - running += 1; - break; - case "passed": - passed += 1; - break; - case "skipped": - skipped += 1; - break; - case "failed": - failed += 1; - break; - } - } - return { passed, failed, running, skipped, total: checks.length }; +): { passed: number; failed: number; running: number; skipped: number; total: number; status: PrChecksStatus } { + const { status, counts } = rollupPrChecks(checks); + return { + passed: counts.passing, + failed: counts.failing, + running: counts.pending, + skipped: counts.skipped, + total: counts.total, + status, + }; } // --------------------------------------------------------------------------- @@ -474,11 +465,21 @@ export const ChatGitToolbar = React.memo(function ChatGitToolbar({ {summary.running} ) : null} - {summary.passed === 0 && summary.failed === 0 && summary.running === 0 ? ( - // Every row settled without verifying anything (all skipped or - // neutral). Name that instead of a bare count that reads neutral. + {summary.skipped > 0 && summary.status !== "not_run" ? ( + // Previously folded into `passed`, so a 3-pass/2-skip PR read + // "5". Shown in its own muted bucket rather than silently + // dropped, which would under-report the suite instead. + + + {summary.skipped} + + ) : null} + {summary.status === "not_run" ? ( + // Nothing verified this commit — either every row was skipped, + // or the only reporters were preview/review bots. Say so rather + // than showing a bare count that reads as merely neutral. - {summary.skipped === summary.total ? "not run" : `${summary.total} check${summary.total === 1 ? "" : "s"}`} + {`not run · ${summary.total} check${summary.total === 1 ? "" : "s"}`} ) : null} diff --git a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx index 8a14bffdc..545b76fa2 100644 --- a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx @@ -26,6 +26,7 @@ import { refreshLinkedPrCoalesced } from "../../lib/prReadCache"; import { useAppStore } from "../../state/appStore"; import { pipelineStateOf } from "../../../shared/prPipelineState"; import { GitHubStackBadge } from "../prs/shared/GitHubStackBadge"; +import { NO_CI_REASON } from "../../../shared/prChecksRollup"; /** * Left floating info-pane for an ADE chat's pull request. Mirrors the right @@ -175,7 +176,7 @@ function notRunView(reason: string | null | undefined): ChecksView { icon: , text: "CI not run", tone: NOT_RUN_TONE, - title: reason ?? "No CI has run on this commit.", + title: reason ?? NO_CI_REASON, }; } diff --git a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx index 451b8de52..e3eec013a 100644 --- a/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx +++ b/apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx @@ -122,6 +122,7 @@ import { PrDetailPane } from "../prs/detail/PrDetailPane"; import { PrsProvider } from "../prs/state/PrsContext"; import { buildGraphPrOverlay } from "./graphPrData"; import { getPrChecksBadge, getPrReviewsBadge, InlinePrBadge } from "../prs/shared/prVisuals"; +import { NO_CI_REASON } from "../../../shared/prChecksRollup"; const nodeTypes = { lane: GraphLaneNode, proposal: GraphProposalNode }; const edgeTypes = { custom: RiskEdge }; @@ -3297,7 +3298,7 @@ function GraphInner({ active = true }: { active?: boolean }) { // Only shown when the rollup has something to explain, which // in practice means a not-run or a held-back pending. pr.checksStatus === "not_run" - ? pr.checksReason ?? "No CI has run on this commit." + ? pr.checksReason ?? NO_CI_REASON : null, `${pr.reviewCount} reviews · ${pr.commentCount} comments${pr.behindBaseBy != null ? ` · behind ${pr.behindBaseBy}` : ""}`, pr.title ? pr.title : null, diff --git a/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx b/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx index 67c280c70..7aa818e07 100644 --- a/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx +++ b/apps/desktop/src/renderer/components/lanes/LanePrBadgePopover.tsx @@ -14,6 +14,7 @@ import { } from "../prs/shared/prVisuals"; import { formatPrBadgeLabel } from "../prs/shared/prFormatters"; import { GitHubStackBadge } from "../prs/shared/GitHubStackBadge"; +import { NO_CI_REASON } from "../../../shared/prChecksRollup"; /** Caption beneath the state badge: "PR opened / merged / draft / closed". */ function prStateCaption(state: LaneTabPrTag["state"]): string { @@ -180,11 +181,7 @@ export function LanePrBadgePopover({ // The rollup's own sentence when it has one ("2 required // checks have not reported: …"), so the muted caption is // explainable without opening the PRs tab. - title={ - pr.checksStatus === "not_run" - ? pr.checksReason ?? "No CI has run on this commit." - : pr.checksReason ?? undefined - } + title={pr.checksReason ?? (pr.checksStatus === "not_run" ? NO_CI_REASON : undefined)} > {checksCaption(pr.checksStatus!)} diff --git a/apps/desktop/src/renderer/components/prs/detail/PrChecksTab.tsx b/apps/desktop/src/renderer/components/prs/detail/PrChecksTab.tsx index 2e52d3530..6f7d02fc1 100644 --- a/apps/desktop/src/renderer/components/prs/detail/PrChecksTab.tsx +++ b/apps/desktop/src/renderer/components/prs/detail/PrChecksTab.tsx @@ -20,6 +20,7 @@ import { import type { PrActionRun, PrCheck, + PrChecksStatus, PrCheckLogExcerpt, PrPipelineState, PrRerunChecksTarget, @@ -468,6 +469,12 @@ export type PrChecksTabProps = { checks: PrCheck[]; actionRuns: PrActionRun[]; actionBusy: boolean; + /** + * The canonical rollup. `buckets` folds `graph.externalChecks` into its + * counts, so with no Actions run three preview-bot successes rendered + * "Passed 3/3" in green — contradicting the header pill on the same screen. + */ + checksStatus?: PrChecksStatus | null; /** * True for GitHub-tab PRs with no `pull_requests` row. The workflow-graph and * log-excerpt endpoints are row-based and reject for these, so we never call @@ -488,6 +495,7 @@ export function PrChecksTab({ checks, actionRuns, actionBusy, + checksStatus, unmapped = false, onRerunChecks, focusedCheckId, @@ -795,9 +803,13 @@ export function PrChecksTab({
{buckets.passed}/{buckets.total}} + label={checksStatus === "not_run" ? "Verified" : "Passed"} + color={checksStatus === "not_run" ? COLORS.textMuted : COLORS.checkPass} + value={ + checksStatus === "not_run" + ? <>0/{buckets.total} + : <>{buckets.passed}/{buckets.total} + } /> 0 ? COLORS.danger : COLORS.textMuted} /> 0 ? COLORS.warning : COLORS.textMuted} /> diff --git a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx index 576759919..d6de752fd 100644 --- a/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx +++ b/apps/desktop/src/renderer/components/prs/detail/PrDetailPane.tsx @@ -1,6 +1,6 @@ import React from "react"; import { - GithubLogo, CheckCircle, XCircle, Circle, + GithubLogo, CheckCircle, XCircle, Circle, CircleDashed, CircleNotch, ArrowRight, Eye, Code, PencilSimple, X, Check, ArrowsClockwise, Play, CaretDown, CaretRight, @@ -571,6 +571,9 @@ export function PrDetailPane({ // One unified check set for the tab header, the CI rollup, and the adaptive // refresh cadence below. const headerChecks = React.useMemo(() => buildUnifiedChecks(checks, actionRuns), [checks, actionRuns]); + // Prefer the live status when we have it; fall back to the row. Both carry + // the canonical rollup, which the header's own bucket counts cannot see. + const checksStatusForHeader = status?.checksStatus ?? pr.checksStatus; const checksTerminal = React.useMemo( // An empty result is not terminal: a workflow may not have created its // first check run yet. Keep the checks-tab poll alive until at least one @@ -1371,7 +1374,14 @@ export function PrDetailPane({ const showDetailLoadingPill = (detailLoading || detailBusy) && !hasVisibleDetailData; const headerCi = React.useMemo(() => { - if (headerChecks.length === 0) return null; + // A `not_run` rollup with zero rows is the pure layer-3 case: required + // contexts are known and none reported. Returning null there left the + // header silent in exactly the situation the rule exists to surface. + if (headerChecks.length === 0) { + return checksStatusForHeader === "not_run" + ? { color: COLORS.textMuted, label: "No CI has run", icon: } + : null; + } // Same rollup as the CI tab and the overview card — one implementation. const buckets = summarizePipelineStates(headerChecks); const passing = buckets.passed; @@ -1383,8 +1393,18 @@ export function PrDetailPane({ if (pending > 0) { return { color: COLORS.info, label: `${pending} running`, icon: }; } + // ADE-135: this pill is the surface in the ticket title. The bucket counts + // above are producer-blind, so three third-party successes rendered a green + // "3/3 passed" here. The canonical rollup decides whether green is earned. + if (checksStatusForHeader === "not_run") { + return { + color: COLORS.textMuted, + label: "No CI has run", + icon: , + }; + } return { color: COLORS.checkPass, label: `${passing}/${headerChecks.length} passed`, icon: }; - }, [headerChecks]); + }, [headerChecks, checksStatusForHeader]); const DETAIL_TABS: Array<{ id: DetailTab; label: string; icon: React.ElementType; count?: number }> = [ { id: "overview", label: "Overview", icon: Eye }, @@ -1674,6 +1694,7 @@ export function PrDetailPane({ checks={checks} actionRuns={actionRuns} actionBusy={actionBusy} + checksStatus={checksStatusForHeader} unmapped={isUnmapped} onRerunChecks={pr.laneId ? handleRerunChecks : undefined} focusedCheckId={focusedCheckId} diff --git a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx index 21f329efe..cd5479469 100644 --- a/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/GitHubTabPrRow.tsx @@ -11,6 +11,7 @@ import { PrCiRunningIndicator } from "./prVisuals"; import { GitHubStackBadge } from "./GitHubStackBadge"; import { formatPrListGroupDiff, type PrListGroupHeader as PrListGroupHeaderModel } from "./prListGrouping"; import { branchNameFromRef } from "../tabs/githubPrBranch"; +import { NO_CI_REASON } from "../../../../shared/prChecksRollup"; /** * Presentation for one row of the GitHub PR list, and the period header that groups @@ -50,9 +51,6 @@ function stateBadgeStyle(item: GitHubPrListItem): React.CSSProperties { }; } -/** Copy used when the producer had no more specific reason to offer. */ -const NO_CI_REASON = "No CI has run on this commit."; - function PrRowCiStatus({ status, reason, diff --git a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx index 142ac910c..d8cd230e0 100644 --- a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.test.tsx @@ -173,8 +173,23 @@ describe("PrChecksCard summary + bucketing", () => { expect(rowNames()).toEqual(["e2e"]); }); - it("renders ghost rows even when nothing at all reported", () => { - render(); - expect(screen.getAllByTestId("pr-checks-card-ghost-row")).toHaveLength(1); + it("renders ghost rows, and no green header, when nothing at all reported", () => { + // The pure layer-3 case: branch protection names a required context and + // nothing ran. The list must still render, and the header must not claim a + // pass off an empty row set. + render( + , + ); + + const ghosts = screen.getAllByTestId("pr-checks-card-ghost-row"); + expect(ghosts).toHaveLength(1); + expect(ghosts[0]!.textContent).toContain("CI / build"); + expect(screen.queryByText(/passed/i)).toBeNull(); + expect(screen.getByText(/No CI has run/i)).toBeTruthy(); }); }); diff --git a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx index e0294ce4e..5c1434142 100644 --- a/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/PrChecksCard.tsx @@ -9,7 +9,7 @@ import { XCircle, } from "@phosphor-icons/react"; -import type { PrActionRun, PrCheck, PrRerunChecksTarget } from "../../../../shared/types/prs"; +import type { PrActionRun, PrCheck, PrChecksStatus, PrRerunChecksTarget } from "../../../../shared/types/prs"; import { COLORS, SANS_FONT, floatingPane } from "../../lanes/laneDesignTokens"; import { buildUnifiedChecks, @@ -42,6 +42,13 @@ export type PrChecksCardProps = { * rather than as nothing at all. */ missingRequired?: readonly string[] | null; + /** + * The canonical rollup. The header used to derive its verdict from row counts + * alone, which is producer-blind: on the ADE-135 payload it rendered a green + * "3/3 passed" directly above the `required · not reported` ghost rows this + * card had just been taught to show. + */ + checksStatus?: PrChecksStatus | null; }; type Bucket = "pass" | "fail" | "pending" | "skip"; @@ -100,6 +107,7 @@ export const PrChecksCard = memo(function PrChecksCard({ actionBusy = false, fill = false, missingRequired, + checksStatus, }: PrChecksCardProps) { // Order is meaningful (GitHub's declaration order), so this is never sorted. const ghosts = missingRequired ?? []; @@ -120,8 +128,11 @@ export const PrChecksCard = memo(function PrChecksCard({ [items, fill], ); + // `not_run` outranks the row tally: rows can all be green and still have + // verified nothing. + const notRun = checksStatus === "not_run"; const summaryColor = - total === 0 + total === 0 || notRun ? COLORS.textMuted : failing > 0 ? COLORS.danger @@ -129,8 +140,15 @@ export const PrChecksCard = memo(function PrChecksCard({ ? COLORS.info : COLORS.checkPass; - const summaryText = total === 0 ? "No checks yet" : `${passing}/${total} passed`; - const headerBucket: Bucket = total === 0 ? "skip" : failing > 0 ? "fail" : pending > 0 ? "pending" : "pass"; + const summaryText = notRun + ? total === 0 + ? "No CI has run" + : `No CI has run · ${total} check${total === 1 ? "" : "s"}` + : total === 0 + ? "No checks yet" + : `${passing}/${total} passed`; + const headerBucket: Bucket = + total === 0 || notRun ? "skip" : failing > 0 ? "fail" : pending > 0 ? "pending" : "pass"; return (
{ expect(result.title).toBe(""); expect(result.body).toBe(""); }); + + it("does not claim all checks passed when nothing verified the commit", () => { + // ADE-135: summarizeChecks is producer-blind, so three third-party + // successes reported passing: 3 and this row said "All 3 checks passed" — + // on the surface a reader trusts most. + const items = buildMergeChecklist({ + pr: makePr({ checksStatus: "not_run" }), + status: null, + checks: [ + { name: "CodeRabbit", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "coderabbitai" }, + { name: "Vercel", status: "completed", conclusion: "success", detailsUrl: null, startedAt: null, completedAt: null, appSlug: "vercel" }, + ], + reviews: [], + }); + const checksRow = items.find((item) => item.id === "checks"); + expect(checksRow?.label).toBe("No CI has run on this commit"); + expect(checksRow?.state).not.toBe("pass"); + }); }); diff --git a/apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts b/apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts index f063ec8ad..82cb3f0d4 100644 --- a/apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts +++ b/apps/desktop/src/renderer/components/prs/shared/prMergeRailUtils.ts @@ -83,7 +83,17 @@ export function deriveMergeBlockers(args: { }); } + // NOTE: this function has no production caller — only its own test — but it + // duplicates `buildMergeChecklist`'s checks logic, so it is kept in step with + // it. Letting the two diverge is how the ADE-135 bug survived in one of them. const checkSummary = summarizeChecks(checks); + const blockersRollup = status?.checksStatus ?? pr.checksStatus; + if (blockersRollup === "not_run") { + blockers.push({ + id: "no-ci", + label: "No CI has run on this commit.", + }); + } if (checkSummary.failing > 0) { blockers.push({ id: "failing-checks", @@ -214,7 +224,13 @@ export function buildMergeChecklist(args: { } // --- Checks --------------------------------------------------------------- + // ADE-135: `summarizeChecks` counts rows and is producer-blind, so on a PR + // whose only checks are third-party apps (CodeRabbit, Vercel, a comment bot) + // it reports `passing: 3` and this row said "All 3 checks passed" — the + // ticket's exact lie, on the surface a reader trusts most. The canonical + // rollup decides the verdict; the counts are still summarizeChecks' job. const summary = summarizeChecks(checks); + const checksRollup = status?.checksStatus ?? pr.checksStatus; if (summary.failing > 0) { items.push({ id: "checks", @@ -227,6 +243,12 @@ export function buildMergeChecklist(args: { label: `${summary.pending} pending check${summary.pending === 1 ? "" : "s"}`, state: "neutral", }); + } else if (checksRollup === "not_run") { + items.push({ + id: "checks", + label: "No CI has run on this commit", + state: "neutral", + }); } else if (summary.passing > 0) { items.push({ id: "checks", diff --git a/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx b/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx index a7e6fdced..970f77793 100644 --- a/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx +++ b/apps/desktop/src/renderer/components/prs/shared/prVisuals.tsx @@ -27,10 +27,10 @@ export function getPrChecksBadge(status: PrChecksStatus): PrBadgeSpec { if (status === "passing") return { label: "CI", ...colorBadge(COLORS.success) }; if (status === "failing") return { label: "CI", ...colorBadge(COLORS.danger) }; if (status === "pending") return { label: "CI", ...colorBadge(COLORS.warning) }; - // ADE-135: `not_run` is absence, not failure — nothing verified the commit, - // so it reads muted like the quiet default rather than borrowing the danger - // colour. It is separated from `none` only so the branch is explicit here. - if (status === "not_run") return { label: "CI", ...colorBadge(COLORS.textMuted) }; + // ADE-135: `not_run` and `none` both land here. Absence is not failure — + // nothing verified the commit, so it reads muted rather than borrowing the + // danger colour. Anything that is not explicitly green above must fall here; + // a new state reaching the success branch by accident is this ticket's bug. return { label: "CI", ...colorBadge(COLORS.textMuted) }; } @@ -66,8 +66,9 @@ export function getPrCiDotColor(args: { }): string { if (args.ciRunning || args.checksStatus === "pending") return COLORS.info; if (args.checksStatus === "failing") return COLORS.danger; - if (args.checksStatus === "not_run") return COLORS.textMuted; if (args.checksStatus === "passing") return COLORS.success; + // `not_run`/`none`: nothing verified the commit, so it stays muted. Only an + // explicit `passing` above earns green. return COLORS.textMuted; } diff --git a/apps/desktop/src/shared/prChecksRollup.test.ts b/apps/desktop/src/shared/prChecksRollup.test.ts index 20faa5b39..2d4af3346 100644 --- a/apps/desktop/src/shared/prChecksRollup.test.ts +++ b/apps/desktop/src/shared/prChecksRollup.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { rollupChecks, isCiProducerAppSlug, CI_PENDING_GRACE_MS } from "./prChecksRollup"; +import { rollupChecks, rollupPrChecks, isCiProducerAppSlug, CI_PENDING_GRACE_MS } from "./prChecksRollup"; import type { ChecksRollupCheckRun, ChecksRollupInput } from "./prChecksRollup"; import { ADE_MAIN_REQUIRED_CONTEXTS, PR988_CHECK_RUNS } from "./__fixtures__/pr988CheckRuns"; @@ -8,7 +8,6 @@ function input(overrides: Partial = {}): ChecksRollupInput { checkRuns: [], commitStatuses: [], requiredContexts: null, - requiredSource: "unavailable", mergeStateBlocked: false, headCommitAgeMs: null, ...overrides, @@ -38,7 +37,6 @@ describe("rollupChecks — ADE-135 regression", () => { input({ checkRuns: [...PR988_CHECK_RUNS], requiredContexts: [...ADE_MAIN_REQUIRED_CONTEXTS], - requiredSource: "rulesets", headCommitAgeMs: 6 * 60 * 60 * 1000, }), ); @@ -115,7 +113,6 @@ describe("rollupChecks — required contexts", () => { input({ checkRuns: [actionsRun("install", "success")], requiredContexts: ["install", "ci-pass"], - requiredSource: "rulesets", }), ); @@ -128,7 +125,6 @@ describe("rollupChecks — required contexts", () => { input({ checkRuns: [actionsRun("install", "success"), actionsRun("ci-pass", "success")], requiredContexts: ["install", "ci-pass"], - requiredSource: "rulesets", }), ); @@ -139,7 +135,6 @@ describe("rollupChecks — required contexts", () => { const result = rollupChecks( input({ requiredContexts: ["test-desktop (2)", "install", "test-desktop (1)"], - requiredSource: "rulesets", }), ); @@ -156,7 +151,6 @@ describe("rollupChecks — required contexts", () => { input({ checkRuns: [actionsRun("test", "failure")], requiredContexts: ["ci-pass"], - requiredSource: "rulesets", }), ); @@ -169,7 +163,6 @@ describe("rollupChecks — required contexts", () => { input({ checkRuns: [actionsRun("test", "success")], requiredContexts: null, - requiredSource: "unavailable", }), ); @@ -235,3 +228,163 @@ describe("rollupChecks — in-flight", () => { expect(result.status).toBe("pending"); }); }); + +describe("rollupChecks — non-Actions CI providers (ADE-135 regression)", () => { + it("does not mark a CircleCI repo permanently not-run", () => { + // CircleCI, Buildkite, Azure Pipelines and Semaphore report through the + // Checks API with their own app slugs, not the commit-status API. An + // Actions-only allowlist marked every one of those repos unverified + // forever — the original bug, pointing the other way. + const result = rollupChecks( + input({ + checkRuns: [ + { name: "ci/circleci: build", status: "completed", conclusion: "success", appSlug: "circleci-checks" }, + ], + }), + ); + + expect(result.status).toBe("passing"); + }); + + it("still refuses a green from a known preview/review bot", () => { + const result = rollupChecks( + input({ + checkRuns: [ + { name: "CodeRabbit", status: "completed", conclusion: "success", appSlug: "coderabbitai" }, + { name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }, + ], + }), + ); + + expect(result.status).toBe("not_run"); + }); + + it("accepts branch protection as proof regardless of producer", () => { + // If every required context reported and passed, the commit was verified — + // whichever app ran it. This is the safety net for CI we do not recognise. + const result = rollupChecks( + input({ + checkRuns: [ + { name: "custom-ci", status: "completed", conclusion: "success", appSlug: "some-unknown-enterprise-ci" }, + ], + requiredContexts: ["custom-ci"], + }), + ); + + expect(result.status).toBe("passing"); + }); +}); + +describe("rollupChecks — in-flight statuses and fresh pushes", () => { + it("treats waiting/requested/pending as in flight, not as terminal-unknown", () => { + // A deployment-approval gate reports `waiting` with a null conclusion. + // Coercing that to `completed` made it "unknown" and the PR read not-run. + for (const status of ["waiting", "requested", "pending"]) { + const result = rollupChecks( + input({ checkRuns: [{ name: "deploy", status, conclusion: null, appSlug: "github-actions" }] }), + ); + expect(result.status, status).toBe("pending"); + } + }); + + it("holds at pending inside the grace window after a fresh push", () => { + // GitHub takes seconds to register a suite; calling that "CI has not run" + // flashed a spurious card on every push. + const result = rollupChecks( + input({ + checkRuns: [{ name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }], + headCommitAgeMs: 10_000, + }), + ); + + expect(result.status).toBe("pending"); + }); + + it("treats unknown commit age as stale so a finding is never hidden", () => { + const result = rollupChecks( + input({ + checkRuns: [{ name: "Vercel", status: "completed", conclusion: "success", appSlug: "vercel" }], + headCommitAgeMs: null, + }), + ); + + expect(result.status).toBe("not_run"); + }); +}); + +describe("rollupChecks — ordering of the branch-protection override", () => { + it("does not claim CI passed while another job is still running", () => { + // A satisfied branch-protection gate outranks the producer guess, but not + // in-flight work — GitHub itself says "some checks haven't completed yet". + const result = rollupChecks( + input({ + checkRuns: [ + { name: "ci-pass", status: "completed", conclusion: "success", appSlug: "github-actions" }, + { name: "e2e", status: "in_progress", conclusion: null, appSlug: "github-actions" }, + ], + requiredContexts: ["ci-pass"], + }), + ); + + expect(result.status).toBe("pending"); + }); + + it("does not stick at pending when a conclusion arrives before the status flips", () => { + // GitHub can report `status: "queued"` with `conclusion: "skipped"`. + // Treating that as in-flight left the PR pending forever. + const result = rollupChecks( + input({ + checkRuns: [{ name: "build", status: "queued", conclusion: "skipped", appSlug: "github-actions" }], + headCommitAgeMs: 10 * 60 * 1000, + }), + ); + + expect(result.status).toBe("not_run"); + }); +}); + +describe("rollupPrChecks — row-level rollup shared by CLI/TUI/toolbars", () => { + it("refuses a green built only from third-party rows", () => { + const result = rollupPrChecks([ + { status: "completed", conclusion: "success", appSlug: "coderabbitai" }, + { status: "completed", conclusion: "success", appSlug: "vercel" }, + ]); + + expect(result.status).toBe("not_run"); + expect(result.counts.passing).toBe(0); + expect(result.counts.total).toBe(2); + }); + + it("reports none for zero checks rather than defaulting to passing", () => { + // adeRpcServer initialised its verdict to "passing", so an empty list read + // green on the surface agents consume. + expect(rollupPrChecks([]).status).toBe("none"); + }); + + it("does not count skipped as passing", () => { + const result = rollupPrChecks([ + { status: "completed", conclusion: "skipped", appSlug: "github-actions" }, + ]); + + expect(result.status).toBe("not_run"); + expect(result.counts.skipped).toBe(1); + }); + + it("treats a slug-less legacy row as CI rather than reporting not-run", () => { + // `appSlug` only started being populated in this change. Persisted rows, + // older hosts and the TUI's action payloads all carry checks with no slug; + // failing those closed would report "CI has not run" for every legacy + // payload whose CI genuinely passed. + const result = rollupPrChecks([{ status: "completed", conclusion: "success", appSlug: null }]); + + expect(result.status).toBe("passing"); + }); + + it("counts a legacy commit status as CI", () => { + const result = rollupPrChecks([ + { status: "completed", conclusion: "success", appSlug: "commit_status" }, + ]); + + expect(result.status).toBe("passing"); + }); +}); diff --git a/apps/desktop/src/shared/prChecksRollup.ts b/apps/desktop/src/shared/prChecksRollup.ts index 133362688..7478578c9 100644 --- a/apps/desktop/src/shared/prChecksRollup.ts +++ b/apps/desktop/src/shared/prChecksRollup.ts @@ -17,9 +17,11 @@ import type { PrPipelineState } from "./types/prs"; * 1. State mapping is delegated to `prPipelineState`, so `skipped`/`neutral` * can no longer masquerade as success and the rollup can no longer * disagree with the per-job rows rendered beneath it. - * 2. Green requires a *CI producer* — GitHub Actions or a legacy commit - * status. Preview/review/comment apps still render, but cannot carry a - * green on their own. + * 2. Green requires either a producer that is not a known non-CI app, or a + * satisfied branch-protection gate. Preview/review/comment apps render but + * cannot carry a green on their own. See the tradeoff note on + * `NON_CI_PRODUCER_APP_SLUGS` for why this is a denylist and not an + * allowlist — the allowlist version broke every non-Actions CI provider. * 3. Required contexts that never reported hold the rollup back, so a missing * job is visible rather than silently absent. * @@ -27,8 +29,47 @@ import type { PrPipelineState } from "./types/prs"; * about our own knowledge rather than about the code. */ -/** GitHub App slugs whose successes constitute actual verification. */ -const CI_PRODUCER_APP_SLUGS = new Set(["github-actions"]); +/** + * Producers that are definitively NOT CI: preview deploys, review bots, comment + * bots, docs builders. These are the apps that made PR #988 read green while + * nothing verified the code, so they can never carry a pass. + * + * This is a denylist rather than an allowlist because the allowlist version was + * wrong in the dangerous direction. CircleCI (`circleci-checks`), Buildkite, + * Azure Pipelines, Semaphore and Travis all report through the *Checks API* + * with their own slugs — not the legacy commit-status API — so an + * Actions-only allowlist marked every one of those repos permanently + * "CI has not run", silently and forever. Breaking real CI users to catch a + * bot is a worse trade than the residual it leaves: an unrecognised bot can + * still carry a green, which the required-context rule below then catches + * wherever branch protection is readable. + */ +const NON_CI_PRODUCER_APP_SLUGS = new Set([ + "coderabbitai", + "vercel", + "netlify", + "cloudflare-workers-and-pages", + "changeset-bot", + "copilot-pull-request-reviewer", + "greptile-apps", + "railway-app", + "graphite-app", + "mintlify", + "cursor", + "sonarcloud", + "snyk-io", + "renovate", + "dependabot", +]); + +/** + * Fallback sentence for a `not_run` rollup whose `reason` is absent — an older + * host, or a row written before the reason column existed. Every `not_run` + * return below sets a reason, so this is the pre-migration path only. Shared + * because it had already been pasted into eleven files and one copy had lost + * its full stop. + */ +export const NO_CI_REASON = "No CI has run on this commit."; /** * Below this age a missing CI run is more likely "hasn't started" than @@ -66,14 +107,18 @@ export type ChecksRollupInput = { * we hold could read them. Null means "unknown", never "none required". */ requiredContexts: readonly string[] | null; - requiredSource: RequiredContextSource; /** * GraphQL `mergeStateStatus === "blocked"`. Corroborating only: it conflates * missing checks with review-required and out-of-date branches, so it may * strengthen a not-run finding but must never downgrade a genuine pass. */ mergeStateBlocked: boolean; - /** Age of the head commit, used only to choose between "yet" and "never". */ + /** + * Age of the head commit. Distinguishes "CI has not started yet" from "CI + * never ran", and inside the grace window holds the rollup at `pending`. + * Null means unknown, which is treated as *stale* — a row whose age we + * cannot establish must not have a finding hidden behind a grace window. + */ headCommitAgeMs: number | null; }; @@ -97,9 +142,9 @@ export type ChecksRollup = { */ export function isCiProducerAppSlug(slug: string | null | undefined): boolean { const value = (slug ?? "").trim().toLowerCase(); - // A missing slug is treated as non-CI: GitHub always populates it for - // Actions, so an absent one means we cannot vouch for the producer. - return CI_PRODUCER_APP_SLUGS.has(value); + // A missing slug cannot be vouched for, so it does not count as CI. + if (!value) return false; + return !NON_CI_PRODUCER_APP_SLUGS.has(value); } /** @@ -119,13 +164,53 @@ export const COMMIT_STATUS_APP_SLUG = "commit_status"; */ export function isCiProducerCheck(appSlug: string | null | undefined): boolean { const value = (appSlug ?? "").trim().toLowerCase(); - return value === COMMIT_STATUS_APP_SLUG || isCiProducerAppSlug(value); + if (value === COMMIT_STATUS_APP_SLUG) return true; + // Unlike the payload-level predicate, an ABSENT slug counts as CI here. + // These rows come from `PrCheck` lists, and `appSlug` only started being + // populated in this change — every persisted row, every older host, and the + // TUI's own action payloads carry checks with no slug at all. Failing those + // closed would report "CI has not run" for every legacy payload whose CI + // genuinely passed, which is the original bug pointing the other way. + // A named bot is still caught, because the denylist matches on the slug it + // does send. + if (!value) return true; + return isCiProducerAppSlug(value); } function isCiProducer(run: ChecksRollupCheckRun): boolean { return isCiProducerAppSlug(run.appSlug); } +/** + * GitHub's check-run `status` enum is wider than the three values + * `pipelineStateOf` accepts: `waiting`, `requested` and `pending` also occur, + * and they arrive with a null conclusion. Folding those into `completed` would + * make them terminal-but-unknown, and a PR sitting on a deployment-approval + * gate would report "CI has not run" while its job is very much alive. They are + * in-flight, so they map to `queued`. + */ +function toPipelineStatus( + raw: string, + conclusion: string | null, +): "queued" | "in_progress" | "completed" { + switch (raw) { + case "in_progress": + return "in_progress"; + case "completed": + return "completed"; + case "queued": + case "waiting": + case "requested": + case "pending": + default: + // A check can carry a terminal conclusion (e.g. `skipped`) before its + // status flips to `completed`; the code this replaced said so explicitly. + // Ignoring that here left `hasInFlight` true forever, so the PR could + // never leave `pending` and the chat card stayed `live` indefinitely. + return conclusion ? "completed" : "queued"; + } +} + function commitStatusState(state: string): PrPipelineState { const value = state.trim().toLowerCase(); if (value === "success") return "passed"; @@ -155,10 +240,7 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup { const ciStates: PrPipelineState[] = [ ...ciRuns.map((run) => pipelineStateOf({ - status: - run.status === "queued" || run.status === "in_progress" || run.status === "completed" - ? run.status - : "completed", + status: toPipelineStatus(run.status, run.conclusion), conclusion: run.conclusion, }), ), @@ -173,6 +255,25 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup { (context) => !observedContexts.has(context.trim()), ); + // Branch protection is the one authority that outranks producer guessing: if + // every required context reported and passed, this commit was verified, no + // matter which app ran the job. Without this, a repo whose CI is an app we do + // not recognise would sit at "not run" while its own merge gate was satisfied. + const requiredKnown = (input.requiredContexts?.length ?? 0) > 0; + const passedContexts = new Set([ + ...input.checkRuns + .filter((run) => pipelineStateOf({ + status: toPipelineStatus(run.status, run.conclusion), + conclusion: run.conclusion, + }) === "passed") + .map((run) => run.name.trim()), + ...input.commitStatuses + .filter((status) => commitStatusState(status.state) === "passed") + .map((status) => status.context.trim()), + ]); + const allRequiredPassed = + requiredKnown && (input.requiredContexts ?? []).every((context) => passedContexts.has(context.trim())); + const ciProducerCount = ciStates.length; const hasFailure = ciStates.some((state) => state === "failed"); const hasInFlight = ciStates.some((state) => state === "running" || state === "queued"); @@ -199,14 +300,26 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup { // Required checks are known and some never reported. Something is expected // that has not arrived, so the rollup stays open rather than going green. if (missingRequiredContexts.length > 0) { - const stale = (input.headCommitAgeMs ?? 0) >= CI_PENDING_GRACE_MS; + const stale = (input.headCommitAgeMs ?? Number.POSITIVE_INFINITY) >= CI_PENDING_GRACE_MS; return { - status: hasPass || ciProducerCount > 0 ? "pending" : "not_run", + // Inside the grace window a required check that has not reported is + // simply one GitHub has not registered yet. Calling that "not run" made + // every single push flash a spurious "CI has not run" card before the + // suite appeared. + status: ciProducerCount > 0 || !stale ? "pending" : "not_run", reason: `${pluralize(missingRequiredContexts.length, "required check has", "required checks have")} not reported${stale ? "" : " yet"}: ${formatContexts(missingRequiredContexts)}.`, missingRequiredContexts, }; } + // Ordered AFTER the in-flight check on purpose: a satisfied branch-protection + // gate outranks the producer guess and the missing-context rule, but it must + // not claim "CI passed" while another job is still running — that is the same + // rollup-disagrees-with-the-rows failure this module exists to prevent. + if (allRequiredPassed) { + return { status: "passing", reason: null, missingRequiredContexts }; + } + if (hasPass) { return { status: "passing", reason: null, missingRequiredContexts }; } @@ -226,10 +339,13 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup { // No CI producer at all. If other apps reported, or GitHub says the merge is // blocked, then something was expected here and its absence is the finding. - const stale = (input.headCommitAgeMs ?? 0) >= CI_PENDING_GRACE_MS; + const stale = (input.headCommitAgeMs ?? Number.POSITIVE_INFINITY) >= CI_PENDING_GRACE_MS; if (otherRuns.length > 0) { return { - status: "not_run", + // A commit pushed seconds ago whose CI suite has not registered yet is + // pending, not unverified. Only once the grace window closes is absence + // a finding. + status: stale ? "not_run" : "pending", reason: `${pluralize(otherRuns.length, "check", "checks")} reported, none from a CI provider. CI has ${stale ? "not run on this commit" : "not run yet"}.`, missingRequiredContexts, }; @@ -246,3 +362,74 @@ export function rollupChecks(input: ChecksRollupInput): ChecksRollup { // quiet rather than inventing a warning for repos that simply have no CI. return { status: "none", reason: null, missingRequiredContexts }; } + +/** + * A flattened check row as the UI surfaces see it — check runs and legacy + * commit statuses already merged into one list, which is what `PrCheck` is. + */ +export type PrCheckRow = { + status: string; + conclusion: string | null; + appSlug?: string | null; +}; + +export type PrChecksRowRollup = { + status: PrChecksStatus; + counts: { passing: number; failing: number; pending: number; skipped: number; total: number }; +}; + +/** + * Row-level rollup for surfaces that only ever have the merged check list and + * no required-context knowledge — the `ade` RPC server, the TUI right pane, the + * chat toolbars. + * + * ADE-135: each of those had grown its own pass/fail rule, and each got it + * wrong differently — one initialised its verdict to "passing" so zero checks + * read green, another counted `skipped` into the passed bucket, a third derived + * green from row counts alone. They share this instead. It applies the same two + * rules as `rollupChecks`: state comes from `pipelineStateOf`, and only a CI + * producer's success can carry a green. + */ +export function rollupPrChecks(checks: readonly PrCheckRow[]): PrChecksRowRollup { + let passing = 0; + let failing = 0; + let pending = 0; + let skipped = 0; + + for (const check of checks) { + const state = pipelineStateOf({ + status: toPipelineStatus(check.status, check.conclusion), + conclusion: check.conclusion, + }); + // Only a CI producer's verdict counts toward pass/fail. Preview, review and + // comment apps are still tallied in `total` so the UI can say how many + // checks reported, but they cannot move the rollup. + if (!isCiProducerCheck(check.appSlug)) continue; + switch (state) { + case "passed": + passing += 1; + break; + case "failed": + failing += 1; + break; + case "running": + case "queued": + pending += 1; + break; + case "skipped": + skipped += 1; + break; + default: + break; + } + } + + const counts = { passing, failing, pending, skipped, total: checks.length }; + if (failing > 0) return { status: "failing", counts }; + if (pending > 0) return { status: "pending", counts }; + if (passing > 0) return { status: "passing", counts }; + // No CI producer succeeded. If nothing at all reported we cannot even say + // something was expected, so stay quiet; otherwise this is the ADE-135 case. + if (checks.length === 0) return { status: "none", counts }; + return { status: "not_run", counts }; +} diff --git a/apps/desktop/src/shared/types/prs.ts b/apps/desktop/src/shared/types/prs.ts index c342f8441..0d5dda705 100644 --- a/apps/desktop/src/shared/types/prs.ts +++ b/apps/desktop/src/shared/types/prs.ts @@ -78,6 +78,10 @@ export type PrSummary = { * One sentence explaining a non-obvious rollup, e.g. "3 checks reported, none * from a CI provider." Null when the state speaks for itself. Every surface * reads this instead of re-deriving the explanation. + * + * Optional AND nullable, and the difference is load-bearing at the upsert: + * absent means "leave whatever is stored alone" (partial summaries flow + * through `upsertRow` constantly), null means "clear it". */ checksReason?: string | null; /** Required contexts that never reported, in the order GitHub declared them. */ @@ -132,6 +136,12 @@ export type PrLaneSummary = { state: "open" | "merged" | "closed"; checksPassed: number; checksTotal: number; + /** + * ADE-135 canonical rollup. Consumers must read this rather than inferring a + * pass from `checksPassed === checksTotal` — producer-blind counts are how + * three preview-bot successes rendered as a green N/N. + */ + checksStatus?: PrChecksStatus; stack?: GitHubPrStackMembership | null; }; diff --git a/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift b/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift index 0d5e33dcc..4dae73c62 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailChecksTab.swift @@ -4,38 +4,51 @@ struct PrChecksSummaryStats: Equatable { let fail: Int let pending: Int let pass: Int + /// ADE-135: neutral/skipped used to be folded into `pass`, so an all-skipped + /// suite rendered a green "3 pass" bar directly beneath the banner saying no + /// CI had run. They get their own muted bucket; `total` still sums the four. + let skipped: Int let total: Int } func prChecksSummaryStats(checks: [PrCheck], overallChecksStatus: String?) -> PrChecksSummaryStats { - var fail = 0, pending = 0, pass = 0 + var fail = 0, pending = 0, pass = 0, skipped = 0 for check in checks { switch prCheckConclusionKind(check) { case .success: pass += 1 case .failure: fail += 1 case .pending: pending += 1 - // Neutral/skipped checks are non-failing outcomes; bucket them with pass so the - // stat strip's total always equals the sum of pass + fail + pending. - case .neutral: pass += 1 + // Non-failing, but nothing was verified either — counting these as passes + // is the same mistake the rollup used to make. + case .neutral: skipped += 1 } } if !checks.isEmpty { - return .init(fail: fail, pending: pending, pass: pass, total: checks.count) + // ADE-135: on PR #988 three third-party rows carried `success`, so a naive + // tally renders "3 pass" directly beneath a banner saying no CI ran. iOS + // cannot tell a test job from a preview bot on its own — the host already + // decided that and said `not_run`, so trust it and report those rows as + // unverified rather than reimplementing the producer rule here and letting + // the two drift. + if overallChecksStatus?.lowercased() == "not_run" { + return .init(fail: fail, pending: pending, pass: 0, skipped: pass + skipped, total: checks.count) + } + return .init(fail: fail, pending: pending, pass: pass, skipped: skipped, total: checks.count) } switch overallChecksStatus?.lowercased() { case "failing", "failure", "failed": - return .init(fail: 1, pending: 0, pass: 0, total: 1) + return .init(fail: 1, pending: 0, pass: 0, skipped: 0, total: 1) case "pending", "running", "in_progress": - return .init(fail: 0, pending: 1, pass: 0, total: 1) + return .init(fail: 0, pending: 1, pass: 0, skipped: 0, total: 1) case "passing", "success", "passed": - return .init(fail: 0, pending: 0, pass: 1, total: 1) + return .init(fail: 0, pending: 0, pass: 1, skipped: 0, total: 1) // ADE-135: nothing verified the commit, so there is no synthetic row to invent // in any bucket — least of all pass. case "not_run": - return .init(fail: 0, pending: 0, pass: 0, total: 0) + return .init(fail: 0, pending: 0, pass: 0, skipped: 0, total: 0) default: - return .init(fail: 0, pending: 0, pass: 0, total: 0) + return .init(fail: 0, pending: 0, pass: 0, skipped: 0, total: 0) } } @@ -54,7 +67,7 @@ func prChecksEmptyStateCopy( case "not_run": return ( "No CI ran on this commit", - checksReason ?? "No CI has run on this commit." + checksReason ?? noCIReasonText ) case "failing", "failure", "failed": return ( @@ -131,10 +144,15 @@ struct PrChecksTab: View { prChecksEmptyStateCopy(overallChecksStatus: overallChecksStatus, checksReason: checksReason) } + /// The canonical rollup said nothing verified this commit. + private var isNotRun: Bool { + overallChecksStatus?.lowercased() == "not_run" + } + /// True when the rollup itself is the finding: nothing verified the commit, so /// the reason banner leads even if unrelated check rows did sync. private var showsNotRunBanner: Bool { - overallChecksStatus?.lowercased() == "not_run" && !checks.isEmpty + isNotRun && !checks.isEmpty } var body: some View { @@ -145,7 +163,7 @@ struct PrChecksTab: View { PrChecksStatStrip(stats: stats) if showsNotRunBanner { - PrChecksNotRunBanner(reason: checksReason ?? "No CI has run on this commit.") + PrChecksNotRunBanner(reason: checksReason ?? noCIReasonText) } if checks.isEmpty { @@ -156,7 +174,7 @@ struct PrChecksTab: View { ) } else { ForEach(groups, id: \.kind) { group in - PrChecksGroupCard(group: group) + PrChecksGroupCard(group: group, notRun: isNotRun) } } @@ -216,6 +234,10 @@ private struct PrChecksProgressBar: View { if total == 0 { return "no checks" } if stats.fail > 0 { return "\(stats.fail) failing" } if stats.pending > 0 { return "\(stats.pending) pending · \(stats.pass) passing" } + // ADE-135: "all passing" with zero passes is the lie in miniature. A suite + // that only skipped verified nothing, and says so. + if stats.pass == 0 && stats.skipped > 0 { return "\(stats.skipped) skipped · none passing" } + if stats.skipped > 0 { return "\(stats.pass) passing · \(stats.skipped) skipped" } return "all passing" } @@ -229,7 +251,13 @@ private struct PrChecksProgressBar: View { Spacer(minLength: 6) Text(passSummary) .font(.system(size: 10.5, weight: .semibold)) - .foregroundStyle(stats.fail > 0 ? ADEColor.danger : (stats.pending > 0 ? ADEColor.warning : ADEColor.success)) + .foregroundStyle( + stats.fail > 0 + ? ADEColor.danger + : (stats.pending > 0 + ? ADEColor.warning + : (stats.pass > 0 ? ADEColor.success : ADEColor.textMuted)) + ) } .padding(.horizontal, 4) @@ -238,10 +266,12 @@ private struct PrChecksProgressBar: View { let passW = geo.size.width * CGFloat(stats.pass) / CGFloat(total) let failW = geo.size.width * CGFloat(stats.fail) / CGFloat(total) let pendW = geo.size.width * CGFloat(stats.pending) / CGFloat(total) + let skipW = geo.size.width * CGFloat(stats.skipped) / CGFloat(total) HStack(spacing: 0) { if stats.fail > 0 { Rectangle().fill(ADEColor.danger).frame(width: failW) } if stats.pending > 0 { Rectangle().fill(ADEColor.warning).frame(width: pendW) } if stats.pass > 0 { Rectangle().fill(ADEColor.success).frame(width: passW) } + if stats.skipped > 0 { Rectangle().fill(ADEColor.textMuted.opacity(0.5)).frame(width: skipW) } if stats.total == 0 { Rectangle().fill(ADEColor.textMuted.opacity(0.3)) } } .clipShape(Capsule()) @@ -263,6 +293,9 @@ private struct PrChecksStatStrip: View { PrChecksStatTile(count: stats.fail, label: "Fail", tint: ADEColor.danger) PrChecksStatTile(count: stats.pending, label: "Pending", tint: ADEColor.warning) PrChecksStatTile(count: stats.pass, label: "Pass", tint: ADEColor.success) + if stats.skipped > 0 { + PrChecksStatTile(count: stats.skipped, label: "Skipped", tint: ADEColor.textMuted) + } PrChecksStatTile(count: stats.total, label: "Total", tint: PrGlassPalette.purpleBright) } } @@ -478,42 +511,68 @@ private struct PrCheckGroup { } } +/// One coloured fragment of a group card's right-hand summary. Split out from the +/// view so ADE-135's "never green when nothing verified the commit" rule can be +/// asserted without rendering. +struct PrChecksGroupSummaryPart: Equatable { + enum Tone: Equatable { case fail, pending, pass, muted } + let text: String + let tone: Tone +} + +/// ADE-135. The per-group tally is producer-blind in exactly the way the top-level +/// rollup used to be: three third-party apps reporting `success` rendered a green +/// "3 pass" in the CI group, directly under the banner saying no CI ran. When the +/// host says `not_run`, those rows are reported as arrived-but-unverifying instead +/// of as passes, so no green survives anywhere on the screen. +func prChecksGroupSummaryParts(checks: [PrCheck], notRun: Bool) -> [PrChecksGroupSummaryPart] { + var pass = 0 + var fail = 0 + var pending = 0 + for check in checks { + switch prCheckConclusionKind(check) { + case .success: pass += 1 + case .failure: fail += 1 + case .pending: pending += 1 + case .neutral: break + } + } + var parts: [PrChecksGroupSummaryPart] = [] + if fail > 0 { + parts.append(.init(text: "\(fail) fail", tone: .fail)) + } + if pending > 0 { + parts.append(.init(text: "\(pending) pending", tone: .pending)) + } + if pass > 0 { + parts.append( + notRun + ? .init(text: "\(pass) reported", tone: .muted) + : .init(text: "\(pass) pass", tone: .pass) + ) + } + if parts.isEmpty { + return [.init(text: "\(checks.count) total", tone: .muted)] + } + return parts +} + private struct PrChecksGroupCard: View { let group: PrCheckGroup + /// The canonical rollup says nothing verified this commit. + let notRun: Bool - private var summary: AttributedString { - var pass = 0 - var fail = 0 - var pending = 0 - for check in group.checks { - switch prCheckConclusionKind(check) { - case .success: pass += 1 - case .failure: fail += 1 - case .pending: pending += 1 - case .neutral: break - } - } - var parts: [AttributedString] = [] - if fail > 0 { - var f = AttributedString("\(fail) fail") - f.foregroundColor = ADEColor.danger - parts.append(f) - } - if pending > 0 { - var p = AttributedString("\(pending) pending") - p.foregroundColor = ADEColor.warning - parts.append(p) - } - if pass > 0 { - var s = AttributedString("\(pass) pass") - s.foregroundColor = ADEColor.success - parts.append(s) - } - if parts.isEmpty { - var s = AttributedString("\(group.checks.count) total") - s.foregroundColor = ADEColor.textSecondary - return s + private func color(for tone: PrChecksGroupSummaryPart.Tone) -> Color { + switch tone { + case .fail: return ADEColor.danger + case .pending: return ADEColor.warning + case .pass: return ADEColor.success + case .muted: return ADEColor.textSecondary } + } + + private var summary: AttributedString { + let parts = prChecksGroupSummaryParts(checks: group.checks, notRun: notRun) var result = AttributedString("") for (index, part) in parts.enumerated() { if index > 0 { @@ -521,7 +580,9 @@ private struct PrChecksGroupCard: View { sep.foregroundColor = ADEColor.textMuted result.append(sep) } - result.append(part) + var fragment = AttributedString(part.text) + fragment.foregroundColor = color(for: part.tone) + result.append(fragment) } return result } diff --git a/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift b/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift index 8222501aa..723874deb 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailHeaderComponents.swift @@ -90,7 +90,7 @@ struct PrDetailSummarySection: View { /// header says. private var subline: String { if checksStatus == "not_run" { - return checksReason ?? "No CI has run on this commit." + return checksReason ?? noCIReasonText } return mergeGate.subline } diff --git a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift index 5eaf74671..524d38236 100644 --- a/apps/ios/ADE/Views/PRs/PrDetailScreen.swift +++ b/apps/ios/ADE/Views/PRs/PrDetailScreen.swift @@ -419,7 +419,8 @@ struct PrDetailView: View { summaryReviewStatus: snapshot?.status?.reviewStatus ?? currentPr.reviewStatus, status: snapshot?.status, checks: snapshot?.checks ?? [], - reviews: snapshot?.reviews ?? [] + reviews: snapshot?.reviews ?? [], + summaryChecksStatus: currentPr.checksStatus ) } diff --git a/apps/ios/ADE/Views/PRs/PrHelpers.swift b/apps/ios/ADE/Views/PRs/PrHelpers.swift index 97c5923ca..f6b792e53 100644 --- a/apps/ios/ADE/Views/PRs/PrHelpers.swift +++ b/apps/ios/ADE/Views/PRs/PrHelpers.swift @@ -1,6 +1,12 @@ import Foundation import SwiftUI +/// ADE-135: fallback when the host sent no `checksReason` (older host, or a row +/// written before the column existed). Shared because it had been pasted into +/// six files and one copy had already lost its full stop. +let noCIReasonText = "No CI has run on this commit." + + private let prIsoFormatter: ISO8601DateFormatter = { let formatter = ISO8601DateFormatter() formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] diff --git a/apps/ios/ADE/Views/PRs/PrMergeChecklist.swift b/apps/ios/ADE/Views/PRs/PrMergeChecklist.swift index 92463cb52..6875ae640 100644 --- a/apps/ios/ADE/Views/PRs/PrMergeChecklist.swift +++ b/apps/ios/ADE/Views/PRs/PrMergeChecklist.swift @@ -88,7 +88,8 @@ enum PrMergeChecklist { summaryReviewStatus: String, status: PrStatus?, checks: [PrCheck], - reviews: [PrReview] + reviews: [PrReview], + summaryChecksStatus: String? = nil ) -> [PrMergeChecklistItem] { var items: [PrMergeChecklistItem] = [] let mergeState = status?.mergeStateStatus @@ -145,7 +146,12 @@ enum PrMergeChecklist { } // --- Checks ------------------------------------------------------------- + // ADE-135: `summarizeChecks` counts rows and is producer-blind, so a PR + // whose only checks are third-party apps reports passing == 3 and this row + // claimed "All 3 checks passed" — the ticket's lie, on the surface a reader + // trusts most. The canonical rollup decides the verdict; the counts stay. let summary = summarizeChecks(checks) + let checksRollup = status?.checksStatus ?? summaryChecksStatus if summary.failing > 0 { items.append( PrMergeChecklistItem( @@ -162,6 +168,14 @@ enum PrMergeChecklist { state: .neutral ) ) + } else if checksRollup == "not_run" { + items.append( + PrMergeChecklistItem( + id: "checks", + label: "No CI has run on this commit", + state: .neutral + ) + ) } else if summary.passing > 0 { items.append( PrMergeChecklistItem( diff --git a/apps/ios/ADE/Views/PRs/PrRowCard.swift b/apps/ios/ADE/Views/PRs/PrRowCard.swift index 614c27209..5066523cf 100644 --- a/apps/ios/ADE/Views/PRs/PrRowCard.swift +++ b/apps/ios/ADE/Views/PRs/PrRowCard.swift @@ -445,7 +445,7 @@ extension PrRowCard { return CIIndicator( glyph: .hollowRing, color: PrsGlass.textMuted, - title: checksReason ?? "No CI has run on this commit." + title: checksReason ?? noCIReasonText ) default: return nil diff --git a/apps/ios/ADE/Views/Work/WorkChatPrViews.swift b/apps/ios/ADE/Views/Work/WorkChatPrViews.swift index 8e0e2b6f5..b9ac526c0 100644 --- a/apps/ios/ADE/Views/Work/WorkChatPrViews.swift +++ b/apps/ios/ADE/Views/Work/WorkChatPrViews.swift @@ -59,7 +59,7 @@ struct WorkChatPrActivePopup: View { private var accessibilityText: String { var parts = [badge.label, lanePrStateLabel(badge.state)] if badge.checksStatus == "not_run" { - parts.append(badge.checksReason ?? "No CI has run on this commit.") + parts.append(badge.checksReason ?? noCIReasonText) } else if let checksStatus = badge.checksStatus, !checksStatus.isEmpty { parts.append("checks \(checksStatus)") } @@ -129,13 +129,20 @@ struct WorkChatPrDetailsSheet: View { (tag?.githubUrl ?? pr?.githubUrl ?? summary?.githubUrl ?? "").trimmingCharacters(in: .whitespacesAndNewlines) } - private var checksStatus: String? { - snapshot?.status?.checksStatus ?? pr?.checksStatus ?? summary?.checksStatus + /// ADE-135: status and reason must come from the SAME source. Resolving them + /// as two independent `??` chains let a status from `pr` be captioned with a + /// stale sentence from `summary` — the reason no longer explaining the state + /// it sits next to. Pick the source once, then read both off it. + private var checks: (status: String?, reason: String?) { + if let status = snapshot?.status { return (status.checksStatus, status.checksReason) } + if let pr { return (pr.checksStatus, pr.checksReason) } + if let summary { return (summary.checksStatus, summary.checksReason) } + return (nil, nil) } - private var checksReason: String? { - snapshot?.status?.checksReason ?? pr?.checksReason ?? summary?.checksReason - } + private var checksStatus: String? { checks.status } + + private var checksReason: String? { checks.reason } private var additions: Int { pr?.additions ?? summary?.additions ?? 0 @@ -426,7 +433,7 @@ private struct WorkChatPrChecksMetricCard: View { /// carries the host's one-line explanation with it. private var detail: String? { guard normalized == "not_run" else { return nil } - return reason ?? "No CI has run on this commit." + return reason ?? noCIReasonText } var body: some View { diff --git a/apps/ios/ADETests/ADETests.swift b/apps/ios/ADETests/ADETests.swift index fc3366852..79f407145 100644 --- a/apps/ios/ADETests/ADETests.swift +++ b/apps/ios/ADETests/ADETests.swift @@ -11613,7 +11613,7 @@ final class ADETests: XCTestCase { func testPrChecksSummaryFallsBackToOverallFailingStatus() { let stats = prChecksSummaryStats(checks: [], overallChecksStatus: "failing") - XCTAssertEqual(stats, PrChecksSummaryStats(fail: 1, pending: 0, pass: 0, total: 1)) + XCTAssertEqual(stats, PrChecksSummaryStats(fail: 1, pending: 0, pass: 0, skipped: 0, total: 1)) XCTAssertTrue(prChecksHasFailedSignal(checks: [], overallChecksStatus: "failing")) XCTAssertEqual(prChecksEmptyStateCopy(overallChecksStatus: "failing").title, "Checks failing") } @@ -11631,10 +11631,197 @@ final class ADETests: XCTestCase { ] let stats = prChecksSummaryStats(checks: checks, overallChecksStatus: "failing") - XCTAssertEqual(stats, PrChecksSummaryStats(fail: 0, pending: 0, pass: 1, total: 1)) + XCTAssertEqual(stats, PrChecksSummaryStats(fail: 0, pending: 0, pass: 1, skipped: 0, total: 1)) XCTAssertFalse(prChecksHasFailedSignal(checks: checks, overallChecksStatus: "failing")) } + // MARK: - ADE-135: nothing verified the commit + + /// PR #988's shape: three third-party apps reported `success`, GitHub Actions + /// never registered a suite. Every surface below used to read this as a pass. + private func ade135ThirdPartyChecks() -> [PrCheck] { + ["CodeRabbit", "Vercel", "Greptile"].map { name in + PrCheck( + name: name, + status: "completed", + conclusion: "success", + detailsUrl: nil, + startedAt: nil, + completedAt: nil + ) + } + } + + func testPrChecksSummaryReportsNoPassesWhenRollupSaysNotRun() { + let stats = prChecksSummaryStats(checks: ade135ThirdPartyChecks(), overallChecksStatus: "not_run") + + XCTAssertEqual(stats, PrChecksSummaryStats(fail: 0, pending: 0, pass: 0, skipped: 3, total: 3)) + XCTAssertFalse(prChecksHasFailedSignal(checks: ade135ThirdPartyChecks(), overallChecksStatus: "not_run")) + } + + func testPrChecksSummaryInventsNoRowForNotRunWithoutChecks() { + XCTAssertEqual( + prChecksSummaryStats(checks: [], overallChecksStatus: "not_run"), + PrChecksSummaryStats(fail: 0, pending: 0, pass: 0, skipped: 0, total: 0) + ) + } + + func testPrChecksEmptyStateCarriesHostReasonForNotRun() { + let copy = prChecksEmptyStateCopy( + overallChecksStatus: "not_run", + checksReason: "3 checks reported, none from a CI provider." + ) + XCTAssertEqual(copy.title, "No CI ran on this commit") + XCTAssertEqual(copy.message, "3 checks reported, none from a CI provider.") + + // Older hosts send no reason; the copy must still be a sentence. + XCTAssertEqual( + prChecksEmptyStateCopy(overallChecksStatus: "not_run").message, + noCIReasonText + ) + } + + func testPrChecksGroupSummaryNeverShowsPassWhenNothingVerifiedTheCommit() { + let checks = ade135ThirdPartyChecks() + + XCTAssertEqual( + prChecksGroupSummaryParts(checks: checks, notRun: false), + [PrChecksGroupSummaryPart(text: "3 pass", tone: .pass)] + ) + XCTAssertEqual( + prChecksGroupSummaryParts(checks: checks, notRun: true), + [PrChecksGroupSummaryPart(text: "3 reported", tone: .muted)] + ) + } + + func testPrChecksLabelAndTintTreatNotRunAsAbsenceNotFailure() { + XCTAssertEqual(prChecksLabel("not_run"), "Not run") + XCTAssertEqual(prChecksTint("not_run"), ADEColor.textSecondary) + XCTAssertNotEqual(prChecksTint("not_run"), ADEColor.danger) + // An unknown state from a newer host must degrade, never render green. + XCTAssertEqual(prChecksTint("some_future_state"), ADEColor.textSecondary) + } + + func testPrRowCiIndicatorDrawsHollowRingForNotRun() { + var item = ade135ListItem(checksStatus: "not_run") + item.checksReason = "3 checks reported, none from a CI provider." + let data = PrRowCard.Data(pr: item) + + XCTAssertEqual(data.ciIndicator?.glyph, .hollowRing) + XCTAssertEqual(data.ciIndicator?.title, "3 checks reported, none from a CI provider.") + // `not_run` is a finding, not a warning banner, and never a failure label. + XCTAssertNil(data.warnMessage) + + let passing = PrRowCard.Data(pr: ade135ListItem(checksStatus: "passing")) + XCTAssertEqual(passing.ciIndicator?.glyph, .symbol("checkmark.circle.fill")) + + // "none" stays silent: nothing observed and nothing expected. + XCTAssertNil(PrRowCard.Data(pr: ade135ListItem(checksStatus: "none")).ciIndicator) + } + + func testPrMergeChecklistReportsNoCiInsteadOfCountingThirdPartyRows() { + let items = PrMergeChecklist.build( + prState: "open", + summaryReviewStatus: "approved", + status: nil, + checks: ade135ThirdPartyChecks(), + reviews: [], + summaryChecksStatus: "not_run" + ) + + let checksRow = items.first { $0.id == "checks" } + XCTAssertEqual(checksRow?.label, "No CI has run on this commit") + XCTAssertEqual(checksRow?.state, .neutral) + } + + func testPrMergeGateSublineDropsAllChecksGreenWhenNothingRan() { + let status = PrStatus( + prId: "pr-988", + state: "open", + checksStatus: "not_run", + reviewStatus: "approved", + isMergeable: true, + mergeConflicts: false, + behindBaseBy: 0 + ) + let gate = prComputeMergeGate( + status: status, + checks: ade135ThirdPartyChecks(), + summaryChecksStatus: "not_run", + reviewThreadsUnresolved: 0, + reviewsNeeded: 1, + reviewsHave: 1, + capabilities: nil + ) + + XCTAssertFalse(gate.subline.contains("all checks green")) + XCTAssertTrue(gate.subline.contains("no CI has run on this commit")) + // Tone stays green on purpose: it feeds merge enablement and this fix is not + // allowed to gate a merge. Only the sentence was false. + XCTAssertEqual(gate.tone, .green) + } + + /// Older brains send neither `checks_reason` nor `checks_missing_required`, and + /// never send `not_run`. Decoding must not fail and must not invent a verdict. + func testPrStatusDecodesWithoutAde135FieldsFromOlderHosts() throws { + let json = """ + {"prId":"pr-1","state":"open","checksStatus":"passing","reviewStatus":"approved", + "isMergeable":true,"mergeConflicts":false,"behindBaseBy":0} + """ + let status = try JSONDecoder().decode(PrStatus.self, from: Data(json.utf8)) + + XCTAssertEqual(status.checksStatus, "passing") + XCTAssertNil(status.checksReason) + XCTAssertNil(status.checksMissingRequired) + } + + func testPrStatusDecodesAde135FieldsWhenPresent() throws { + let json = """ + {"prId":"pr-988","state":"open","checksStatus":"not_run", + "checksReason":"3 checks reported, none from a CI provider.", + "checksMissingRequired":["CI / build","CI / test"], + "reviewStatus":"approved","isMergeable":true,"mergeConflicts":false,"behindBaseBy":0} + """ + let status = try JSONDecoder().decode(PrStatus.self, from: Data(json.utf8)) + + XCTAssertEqual(status.checksStatus, "not_run") + XCTAssertEqual(status.checksReason, "3 checks reported, none from a CI provider.") + // Declaration order is the ruleset's order and is never sorted. + XCTAssertEqual(status.checksMissingRequired, ["CI / build", "CI / test"]) + } + + private func ade135ListItem(checksStatus: String) -> PullRequestListItem { + PullRequestListItem( + id: "pr-988", + laneId: "lane-988", + laneName: "rate-limit", + projectId: "project-1", + repoOwner: "arul28", + repoName: "ADE", + githubPrNumber: 988, + githubUrl: "https://github.com/arul28/ADE/pull/988", + title: "GitHub Rate Limit Fallback", + state: "open", + baseBranch: "main", + headBranch: "lane/rate-limit", + checksStatus: checksStatus, + reviewStatus: "approved", + additions: 10, + deletions: 1, + lastSyncedAt: nil, + createdAt: "2026-08-01T00:00:00.000Z", + updatedAt: "2026-08-01T00:00:00.000Z", + adeKind: "single", + linkedGroupId: nil, + linkedGroupType: nil, + linkedGroupName: nil, + linkedGroupPosition: nil, + linkedGroupCount: 0, + workflowDisplayState: nil, + cleanupState: nil + ) + } + func testPrMergeGateDoesNotShowGreenWhenStatusIsMissing() { let gate = prComputeMergeGate( status: nil, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b9bbab200..0c69c8be2 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -223,7 +223,7 @@ The desktop app is a **client of the runtime**. It owns a trusted main process, | `apps/desktop/src/main/` | Node process with full OS access. Hosts windows, registers IPC handlers, routes runtime-backed APIs through local/remote runtime pools, spawns the local ADE runtime when needed, and owns Electron-only services that cannot run inside the runtime. Entry: `main.ts`. | | `apps/desktop/src/preload/` | Typed bridge. Entry: `preload.ts`. Uses `contextBridge.exposeInMainWorld("ade", { ... })`. Runtime-backed APIs route through `LocalRuntimeConnectionPool` (local) or `RemoteConnectionPool` (paired/SSH-bound window); file APIs are strict once a local/remote runtime is bound, while usage/budget reads only route to runtime for remote-bound windows. Usage push delivery follows the active binding too: unbound windows accept main-process usage events, while bound windows accept only the runtime event stream, so a dormant local tracker cannot overwrite the active project's snapshot. During project switches, mutating runtime/sync calls that target the ambiguous active binding are blocked, read-only calls avoid refreshing stale bindings, active remote opens can be awaited before retrying reads, and remote lane preview URLs are localized through desktop-owned TCP forwards. Chat history reads are the exception to the local-IPC fallback: `isRemoteProjectRuntimeContext()` gives a synchronous, transition-safe answer to "is this window's runtime remote?" (live binding → in-flight remote open → the kind snapshotted by `detachProjectBindingForTransition()`), and a remote context returns `unavailable: true` rather than letting the local chat service answer a remote session id with a false `sessionFound: false` that would wipe the transcript. History runtime actions use one object envelope (`sessionId` plus caps/cursor) across preload and ADE Code; the action registry still normalizes the legacy positional form for packaged-client compatibility. If a packaged local window is temporarily bound to an isolated runtime whose sync service is disabled, only the exact machine-level sync-unavailable/register-project failures retry through main-process sync IPC; remote-bound failures never fall back locally. Explicitly targeted work can pass an `OpenProjectBinding` pin through `callPinnedRuntimeAction` to route to the captured project during a switch, used by detached draft launches and rollback. The same pin is the per-session and detached-draft runtime routing mechanism: chat/session calls, the PTY and terminal surface (`pty.create` / `resumeSession` / `sendToSession` / `write` / `resize` / `dispose` / `setDataSubscriptions` / `onData` / `onExit`, `terminal.preview`), plus machine-owned supporting APIs (AI discovery, slash commands, file search, attachments, lane management, parallel launch state, session deltas, and computer-use snapshots) accept an optional `OpenProjectBinding` so foreign work — a CLI or shell session as much as a chat — stays on its owning machine without rebinding the window's tab. Pinned event subscriptions poll the selected runtime when Electron's bound event stream cannot represent a foreign machine, and preload releases a main-side subscription explicitly once its last pump stops reading. Required foreign ownership fails closed; `callPinnedOrBoundRuntimeActionOr` retains the unchanged bound path only when no pin is required. | | `apps/desktop/src/renderer/` | React 18 SPA. No Node access, no filesystem access, no direct process/network. Everything goes through `window.ade`. Entry: `main.tsx`. | -| `apps/desktop/src/shared/` | Types, IPC channel constants (`ipc.ts`), model registry (`modelRegistry.ts`), keybindings, and cross-client derivations such as `chatScheduledWork.ts` and `externalSessionAffordances.ts` (the desktop/ADE Code Continue/Copy policy for provider-native imports). The project/machine model lives here too: `projectIdentity.ts` is the single definition of a binding key (`local:` / `remote::`) that every per-project cache and the repo tab join are keyed by, `machineIdentity.ts` is the single definition of "the machine ADE is running on" (`THIS_MACHINE_ID` / `THIS_MACHINE_NAME` / `isThisMachineId` / `machineDisplayName`, with machines named absolutely and "remote" never used as a machine name), and `laneDivergence.ts` is the pure push-time guard against stranding another machine's unpushed commits. Imported by desktop, `apps/ade-cli`, and mobile contract generation paths. New runtime-facing types live in `shared/types/remoteRuntime.ts` and `shared/types/core.ts`. | +| `apps/desktop/src/shared/` | Types, IPC channel constants (`ipc.ts`), model registry (`modelRegistry.ts`), keybindings, and cross-client derivations such as `chatScheduledWork.ts`, `externalSessionAffordances.ts` (the desktop/ADE Code Continue/Copy policy for provider-native imports), and `prChecksRollup.ts` (the one definition of whether a commit's CI actually verified it, imported by the desktop service, the renderer, and the `ade code` TUI so no surface counts check rows on its own). The project/machine model lives here too: `projectIdentity.ts` is the single definition of a binding key (`local:` / `remote::`) that every per-project cache and the repo tab join are keyed by, `machineIdentity.ts` is the single definition of "the machine ADE is running on" (`THIS_MACHINE_ID` / `THIS_MACHINE_NAME` / `isThisMachineId` / `machineDisplayName`, with machines named absolutely and "remote" never used as a machine name), and `laneDivergence.ts` is the pure push-time guard against stranding another machine's unpushed commits. Imported by desktop, `apps/ade-cli`, and mobile contract generation paths. New runtime-facing types live in `shared/types/remoteRuntime.ts` and `shared/types/core.ts`. | | `apps/desktop/src/generated/` | Build-time generated code (e.g., bootstrap SQL snapshots). | | `apps/desktop/src/test/` | Shared vitest setup and fixtures. | | `apps/desktop/src/types/` | Ambient type declarations. | @@ -436,7 +436,7 @@ Schema bootstrap in `kvDb.ts` creates ~104 tables. Anchor tables for agents read | `operations` | Audit log of every significant mutation (git, pack updates). Pre/post HEAD SHAs enable undo. | | `usage_events` | Low-volume local ledger of successful user mutations, attributed to `desktop`, `mobile`, `tui`, `web`, or `api`. It is excluded from CRR replication; controllers read its aggregates through `usage.getAdeStats` instead of syncing raw events. | | `test_suites` / `test_runs` | Declared test suites and their execution history. | -| `pull_requests` / `pr_review_threads` / `pr_checks` | GitHub PR projections with queue and stack metadata. | +| `pull_requests` / `pr_review_threads` / `pr_checks` | GitHub PR projections with queue and stack metadata. `pull_requests` also stores the checks rollup's explanation (`checks_reason`) and the required contexts that never reported (`checks_missing_required`, JSON), so every device renders the same verdict without re-deriving it. Both columns were added through `crrAwareDb`'s `crsql_begin_alter` / `crsql_commit_alter` wrapper and replicate like the rest of the table. | | `integration_proposals` | PR merge-plan simulations. Stores source lanes, pairwise results, sequential resolution state, optional adopted merge target (`preferred_integration_lane_id`), and merge-target drift snapshot (`merge_into_head_sha`). | | `computer_use_artifacts` + `computer_use_artifact_links` | Canonical proof-artifact records and cross-domain ownership. | | `prompt_stashes` | Project-scoped desktop composer stashes. The runtime owns create/list/delete, caps the shared set at 20, and CRR replication makes entries available to connected desktops without storing machine-bound attachments. | @@ -843,7 +843,7 @@ Most services described here live under `apps/desktop/src/main/services/ | `opencode/` | `openCodeRuntime.ts`, `openCodeServerManager.ts`, `openCodeBinaryManager.ts`, `openCodeInventory.ts`, `openCodeModelCatalog.ts` | OpenCode server spawn, binary resolution, model discovery. | | `orchestration/` | `orchestrationService.ts`, `applyPatches.ts`, `patchPolicy.ts`, `manifestNormalization.ts`, `runtimeProfile.ts` | Work-tab orchestration for multi-phase plans. `orchestrationService` manages run lifecycle, manifest persistence, the `leadState.planning` state machine, `plan.md`, validation strategy/findings, asset bundles, the `lineage` delegation ledger (lead→worker/validator spawn + result edges), and two service-owned durability records: a `receipts` idempotency ledger and a transactional `outbox` chat-delivery queue. Receipts key on a per-request idempotency key so a retried `spawnAgent`/`messageAgent` replays its original result instead of double-spawning; the outbox holds `brief`/`ping`/`lead_status`/`cancel_interrupt`/`completion` deliveries that are written atomically with the state transition that produced them (a worker/validator reaching a terminal state enqueues a `completion` entry in the same transaction) and drained event-driven with bounded backoff, so the lead can never miss a completion. Worker/validator completion is event-driven (no transcript polling), heartbeats coalesce and a stall sweep flips `agent.stalled` for silent-but-`running` workers with a single plain-language lead notification, and cancellation reaches native worker processes. Runs also carry a per-run `finishing` decision (`worktree` vs. push-PR-and-update-Linear), a `goalSource`, `scheduledFollowups`, a declared `capabilities` policy, and evidence asset kinds (`proof_artifact`/`computer_use`/`video`/`pr_link`/`linear_issue`/`deeplink`) with `externalRef` + `registeredBySessionId`. `patchPolicy` keeps privileged fields (`leadState.planning`, `planSpec`, the `/lineage` ledger, and the `/receipts` + `/outbox` records) behind service methods so the lead cannot forge intake, planning rounds, model routing, approval readiness, delegation edges, or delivery/idempotency state with a raw patch. `runtimeProfile` resolves the active orchestration profile per session and gates model selection / plan approval on planning readiness. The renderer surfaces live in `renderer/components/orchestration/` (see §7.3). The former `orchestrator/` and `missions/` directories were consolidated into this service. | | `projects/` | `adeProjectService.ts`, `configReloadService.ts`, `projectService.ts`, `logIntegrityService.ts`, `recentProjectSummary.ts`, `projectBrowserService.ts`, `projectDetailService.ts` | Project detection + `.ade` repair/bootstrap, reload on config change, recent-project metadata. `recentProjectSummary.ts` emits local and remote recent summaries without disk-inspecting remote paths, and attaches each local checkout's `origin` URL — read straight from git config rather than by spawning `git` per recent, undecorated the way git parses config values, resolved through a linked worktree's metadata dir back to the main repo's config, and cached per root on the config file's mtime. That URL is the key the renderer's top bar joins local and remote checkouts of one repository on. `projectBrowserService` is the in-app directory autocomplete used by the Command Palette project browser (typed-path completion, `.git` detection, home expansion, system-picker fallback); `projectDetailService` returns repo metadata (branch, dirty count plus staged/unstaged/untracked breakdown, ahead/behind, last commit, README excerpt inputs, language mix, lane count, last-opened) for the palette's preview pane. | -| `prs/` | `prService.ts`, `prPollingService.ts`, `prSummaryService.ts`, `githubPrStackService.ts`, `prIssueResolver.ts`, `prRebaseResolver.ts`, `integrationPlanning.ts`, `integrationValidation.ts` | PR CRUD, polling (with per-PR `last_polled_at` cursor), AI summary cache keyed by `(prId, head_sha)`, native GitHub stack reconciliation, AI-assisted issue resolution, rebase resolution, integration planning, and merge-into-existing-lane proposal adoption. | +| `prs/` | `prService.ts`, `prPollingService.ts`, `prSummaryService.ts`, `requiredChecks.ts`, `githubPrStackService.ts`, `prIssueResolver.ts`, `prRebaseResolver.ts`, `integrationPlanning.ts`, `integrationValidation.ts` | PR CRUD, polling (with per-PR `last_polled_at` cursor), AI summary cache keyed by `(prId, head_sha)`, required-status-check resolution (rulesets → classic protection → `mergeStateStatus` corroboration, cached per repo+base branch), native GitHub stack reconciliation, AI-assisted issue resolution, rebase resolution, integration planning, and merge-into-existing-lane proposal adoption. The checks verdict itself is derived by the shared `prChecksRollup.ts`, not here. See [features/pull-requests/README.md](./features/pull-requests/README.md#checks-rollup-what-counts-as-a-pass). | | `pty/` | `ptyService.ts` | `node-pty` spawn, PTY I/O bridging, transcript writing. | | `remoteRuntime/` | `remoteTargetRegistry.ts`, `sshTransport.ts`, `remoteBootstrap.ts`, `remoteConnectionPool.ts`, `remoteConnectionService.ts`, `runtimeRpcClient.ts`, `runtimeDiscovery.ts` | Saved SSH machines (manual host + alternate `routes[]` with `lastSucceededAt` and manual-disconnect state), ssh-agent/key transport with bounded connect/exec timeouts and multi-route fallback, first-connect runtime upload/version/SHA verification with channel-home fallback (`.ade` / `.ade-alpha` / `.ade-beta`) and capability/version skew demoted from fatal errors to `RemoteRuntimeConnectResult.compatibilityWarnings`, remote project catalog, action dispatch (with a `projects.*` capability gate against `RemoteRuntimeCapabilities.machineProjects`), handoff storage/Git preflight, route-pinned sensitive calls, local TCP forwards for remote preview ports, reconnect/eviction with pool eviction listeners and implicit reconnect backoff, `powerMonitor` resume probe, and LAN + Tailscale discovery that returns diagnostics alongside machines. The JSON-RPC client formats remote errors with the original method name plus the JSON-RPC `code` / `message` / `data` for clearer diagnostics. See [Cross-machine session handoff](./features/sync-and-multi-device/cross-machine-session-handoff.md). | | `runtime/` | `tempCleanupService.ts`, `processRegistryService.ts`, `machineStateMigration.ts`, `packagedNodePath.ts`, `lastFailureStore.ts`, `projectRecoveryService.ts` | Runtime temp cleanup. `processRegistryService` is the per-process heartbeat registrar against machine-local `runtime_processes` (see §3.4); reconcile/dispose paths in `sessionService` and `ptyService` consult live and known owner sets before sweeping `terminal_sessions` rows so sibling processes and synced remote-machine owners are preserved. `machineStateMigration` carries one-shot migrations of the per-machine state files under `~/.ade/`. `packagedNodePath.ts` centralizes the `Resources/app*.asar(.unpacked)/node_modules` search path used by packaged runtime children. `lastFailureStore` records bounded typed project/machine failure reports and crash-loop backoff; `projectRecoveryService` runs the brain-independent diagnose/repair sequence behind `ade.recovery.*` (see [features/storage-and-recovery/README.md](./features/storage-and-recovery/README.md)). | diff --git a/docs/features/ade-code/README.md b/docs/features/ade-code/README.md index c5707210d..6eab9c9f1 100644 --- a/docs/features/ade-code/README.md +++ b/docs/features/ade-code/README.md @@ -43,7 +43,7 @@ Point Cursor’s browser inspector at the served page for layout debugging. The | `apps/ade-cli/src/tuiClient/providerMetadata.ts` | Provider labels, family labels, token normalization, and provider lookup helpers shared by setup rows and the model picker. Keeps Anthropic/OpenAI/Factory aliases mapped onto the TUI's provider ids and decides which providers support runtime catalog refresh. | | `apps/ade-cli/src/tuiClient/modelState.ts` | Pure model/setup state for draft chats and `/model`: GPT-5.6 Sol default plus Sol/Terra/Luna ordering, Chat vs CLI interface mode, Cursor chat-vs-CLI availability reconciliation, Codex preset/approval/sandbox mapping, provider-specific permission summaries, host-aware reasoning defaults/visible tiers, Fast Mode support, and the `SetupPaneRow` list rendered in setup panes. GPT-5.6 labels `low` as Light and `xhigh` as Extra High, exposes Max on all three models, and adds Ultra after Max on Sol/Terra. | | `apps/ade-cli/src/tuiClient/modelPickerController.ts` | Small adapter between right-pane model-picker state and `modelPickerLayout.ts`: supplies active model/reasoning/interface, favorites/recents, AI status, footer focus, lane label, and provider refresh routing. | -| `apps/ade-cli/src/tuiClient/rightPaneFormatters.ts` | Pure formatters for right-pane result panes (PR summary / review / checks / comments, Linear status, system details). Keeps `app.tsx` free of ad-hoc rendering helpers. | +| `apps/ade-cli/src/tuiClient/rightPaneFormatters.ts` | Pure formatters for right-pane result panes (PR summary / review / checks / comments, Linear status, system details). Keeps `app.tsx` free of ad-hoc rendering helpers. The PR checks formatter does not tally rows itself — it runs them through the shared `rollupPrChecks` so the TUI applies the same CI-producer rule as desktop and iOS, and a payload `checksStatus` of `not_run` still wins, because only the host knows about required contexts because only the host knows about required contexts. A `not_run` rollup renders as `CI: not run — ` instead of a passing count. See [pull-requests](../pull-requests/README.md#checks-rollup-what-counts-as-a-pass). | | `apps/ade-cli/src/tuiClient/format.ts` | Transcript rendering helpers for the TUI. `webSearchResultPreviewLines` / `webSearchResultDomain` turn Codex structured web-search `results` into compact `title — domain` lines with a `+N more` tail, shared by `renderChatLines` (subagent pane) and `ChatView` (work log). | | `apps/ade-cli/src/tuiClient/displayWidth.ts` | Grapheme-aware terminal-cell helpers using `string-width`: code-unit ↔ display-cell mapping, display-cell slicing, truncation, wrapping, and selection splitting for Unicode-safe chat/model/right-pane rendering. | | `apps/ade-cli/src/tuiClient/aggregate.ts` | Pure derivations on top of the chat event stream. Produces `AggregatedBlock`s (assistant text, connector-aware tool calls, files changed, web/image/plan/compaction groups, runtime-activity rows for subagent and activity envelopes, queued steers) and `derivePendingSteers`, consumed by `ChatView` and the right-pane steer view. MCP app/server identity replaces generic tool labels and image lifecycle updates collapse by item id. | @@ -173,7 +173,7 @@ For the embedded runtime there is no `projects.add` step — the in-process runt - **Drawer** (toggled with the configured shortcut) — two modes, **lanes** (default) and **chats**, switched with `Tab` while the drawer is focused. Lane cards show name + status (no branch ref — that lives in lane details). Every lane shows its chats: the selected lane expands the full chat block (the same tight single-row chats every lane shows, distinguished only by a violet border plus a trailing `+ new chat` row — there is no `CHATS` header), while every other lane renders a compact always-visible preview (the lane's chats as single rows, plus a `+N more` tail only when the row budget can't fit them all) whose rows are clickable and select lane + chat in one step. The TUI enriches both chat and tracked-CLI rows from `session.list`: explicit asks render the blocking question in amber, status notes render inline (`done: …` when settled), and a sanitized last-output preview is the first fallback before summary or goal. Settled rows dim into the quiet glyph tier, and last-turn failures render as failures. Snooze is a second, independent quiet tier: a snoozed row carries a text-only `z` marker plus its wake label ("wakes in 3h" / "wakes tomorrow" / "wakes when asked" / "wakes now"), and a row that woke early carries a `*` marker naming the reason ("needs approval" / "errored" / "turn finished") until it is visited, at which point the marker is cleared. Because snooze is a visibility overlay rather than a phase, a snoozed row that is blocking on the user stays in its normal place — `isSessionFiledAsSnoozed` yields to a `needs_you` phase — while `isSessionSnoozed` remains the raw column read used for row chrome. Ended tracked CLI sessions are hidden behind a `closed (N)` row in the expanded lane; expanding it shows dim one-line rows with provider glyph, title, and relative end time, and `↵` resumes a resumable closed CLI session through the same terminal resume path as desktop. Continuation forwards the stored model, reasoning, Fast Mode, permission mode, and exact Codex approval/sandbox/config controls through the shared launch-field mapper. Row layout and mouse hit-testing share one pure model (`drawerLayout.ts: computeDrawerLayout` / `drawerMouseHitForLayout`) so open chats, closed toggles, closed sessions, and `+ new chat` cannot drift. In **lanes** mode, `↑`/`↓` move lane cards; `↓` on an available lane enters **chats** mode for that lane; `↵` opens lane details or resumes the lane's last chat. In **chats** mode, `↑`/`↓` move within the lane's chat rows, closed group, and `+ new chat`; highlighting a chat previews it in the centre pane via `resolveTuiChatRefreshTarget` before `↵` commits the session. `Esc` returns from the chat list to **lanes**. Lane and chat selection drive the right pane's context. - **ChatView** — the main transcript. Renders user, assistant, file-change, and system events from `chat/event` notifications while normalized tool telemetry stays behind the active activity/status row or the completed turn's `Ran for` row. Codex and most providers label the live row `model working`; Claude keeps its existing provider-specific live presentation and adds only a compact actions disclosure when tools are available. Expanding either status reveals one line per tool (`✓ read apps/x.ts`) with the desktop slug + target-arg derivation (pure helpers imported from `apps/desktop/.../chatTranscriptRows` and `toolPresentation`); MCP events prefer the app/plugin/server name plus action instead of a generic `mcp` label, and expanded `web_search` actions include the first provider action query/title/URL when available plus up to three Codex structured `title — domain` previews with a `+N more` tail. Generated/viewed-image lifecycle updates still collapse to one concise notice per item, and provider-specific narration, reasoning, subagent/activity cards, and notices remain in their existing positions. File-change groups remain chronological, collapse to one summary row, and expand to typed file rows whose `diff` action opens the turn diff in the right pane. Every row truncates to the pane width (rows never wrap — the scroll math assumes 1 row = 1 line). Codex app-server runtime events (`codex_safety_buffering`, `codex_moderation_metadata`, `codex_sleep`, `codex_thread_deleted`, `codex_turn_stalled`) render as concise transcript notices instead of disappearing into generic activity. A valid ` ```mosaic ` fence (the interactive card the desktop transcript renders — see [chat composer-and-ui.md](../chat/composer-and-ui.md)) collapses to a single dim summary line here via `summarizeMosaicCard` from `apps/desktop/src/shared/chatMosaic.ts`, since the TUI cannot render the interactive form; a fence that fails to parse falls back to the plain code block. The most recent expandable failure id is tracked so `Enter` can drill into it. Mouse selection is ADE-owned so it can follow virtual transcript rows: drag selects, edge-drag scrolls, wheel scrolling preserves the highlighted range, Shift-click extends the current anchor, and `Ctrl+C` / delivered `Cmd+C` copy selected chat text. Near the top, both scroll and underfilled viewports silently request more history; the stable first row reads `↑ older messages`, changes in place to `↑ loading earlier…`, and exposes `Ctrl+R` only after all automatic retries fail. Paging continues beyond the 60,000-event resident ceiling by sliding the window toward the transcript head; live events are buffered while detached, and `End` restores the latest bounded tail. - **Composer** — multi-line input with mention completion (`@…`) sourced from `MentionPalette` and slash command completion from `SlashPalette`. Both triggers are detected cursor-relatively through the shared `apps/desktop/src/shared/composerTriggers.ts` module (`detectComposerTrigger`), so a `/command` or `@file` token is recognized anywhere in the draft — not just at position 0 (`fix @src/foo.ts then run /test`). Both palettes stay visible with a no-match row while the user is actively typing. Selecting a suggestion splices exactly the trigger span (`replaceComposerTriggerSpan`) rather than replacing the whole prompt; a lone leading `/command` keeps the legacy fill-the-prompt behavior. `Tab` completes the highlighted slash command, and for a **mid-sentence** slash trigger `Enter` completes into the draft (instead of submitting/running), mirroring the desktop command menu — a leading-only command still runs on `Enter`. Confirmed tokens render as colored chips in the prompt rows via `findConfirmedComposerTokens` + `segmentPromptLineText`: inserted `@file` mentions and `/command` names matching the built-in or runtime catalog paint cyan (files) or violet (commands) and bold, while unmatched `@`/`/` text stays plain. URLs detected by shared `smartLinks.ts` also paint violet and add a compact `links [provider label]` row above the raw prompt; GitHub, Linear, ADE, and generic web labels are deterministic and do not require metadata fetching in the terminal. Character Backspace/Delete removes the whole intersected URL, while the canonical URL remains the submitted prompt text. Mention completion publishes local lane/chat hits immediately, then debounces remote file/git/PR RPCs; file results are cached per lane+query and git/PR results are cached per lane for the open TUI session. Pending tool approvals surface as `ApprovalPrompt`. AskUserQuestion-style requests render every question inline with its options, decision impact, visible default assumption, and an `N of M answered` header. The seeded recommendation is only a cursor, never a preselected answer. While the composer is empty, `↑`/`↓` move the cursor, `←`/`→` switch questions, and `1`-`9` mark an option without submitting; clicking does the same. `Enter` banks the active answer and advances or sends. Typed text accumulates after marked selections instead of replacing them. A question that forbids freeform never advertises a note; without options it either offers Enter for its visible default assumption or directs the user to decline. If printable text follows a provisional digit pick, that digit becomes freeform text and the earlier selection is restored. The deny chip declines the whole request. Selection lives in `pendingInput.ts`'s `PendingQuestionSelectionState`, while payload and label semantics come from `apps/desktop/src/shared/pendingInputAnswers.ts`. -- **RightPane** — context-sensitive drawer for slash command output. The "right" placement commands (see below) render their results here as forms, lists, diffs, help text, or rendered objects. `/secrets` opens a masked project-secret list and copies the selected secret value to the local system clipboard with `Enter` or `c`; it never reveals values inline and only uses the read actions behind the existing project-secret RPC path. When a chat is active the default content is the **Chat Info** view (`kind: "chat-info"`): provider/model header, lane label, streaming/idle indicator with context-percent + token summary, plan steps for the current turn (plus the provider's plan explanation / streaming text when present), Codex `/goal` block when present, a roster of subagents (running first, then teammates and background), and — below the roster, like the Droid Missions block — **TASKS** (latest `todo_update` snapshot, desktop ChatTasksPanel parity), **SCHEDULE** (Claude wakeups/cron/`/loop` from `scheduled_work_update` via `deriveScheduleItems`, desktop Chat Info parity, plus `⏰ next wake ` from the active session summary), **BACKGROUND** (`background_task` work from `scheduled_work_update` via `deriveBackgroundItems`, each rendered as a `$