diff --git a/app/src-tauri/src/commands/mod.rs b/app/src-tauri/src/commands/mod.rs index 3ef5d80..6ea36e4 100644 --- a/app/src-tauri/src/commands/mod.rs +++ b/app/src-tauri/src/commands/mod.rs @@ -192,6 +192,12 @@ pub async fn get_evidence( /// `base_sha` when set, else the base branch name; the head is `head_sha` when /// set, else `HEAD` for a local read or the head branch name for a GitHub read. /// +/// Known limitation for imported PRs: their `base_sha` is empty (it is the +/// restack fork point, computed locally at kickoff — an import has none), so the +/// base side resolves by branch name and may drift past the PR's actual fork +/// point once the base branch advances. The New side is pinned to `head_sha` and +/// is authoritative. +/// /// Content is read locally with /// [`git::file_at_rev`](cockpit_core::adapters::git::file_at_rev) (off the async /// runtime, since `git2` is blocking) when a usable local repo dir exists — a @@ -2398,10 +2404,13 @@ pub async fn fetch_review_requests( /// Shared implementation for fetching PRs by filter. /// -/// When a review already exists in the store (matched by PR URL), only the -/// diff, branch, and base are refreshed — comments, gate state, agent run, -/// and stale flag are preserved. This prevents re-fetching from GitHub from -/// blowing away in-progress review work. +/// When a review already exists in the store (matched by PR URL), the diff, +/// branch, base, CI summary, and pinned base/head SHAs are refreshed from +/// GitHub — comments, gate state, agent run, and stale flag are preserved so a +/// re-fetch never blows away in-progress review work. The head SHA is held back +/// while a rework is in flight (an attached agent or `Dispatched` state): the +/// local worktree HEAD leads GitHub's last-reported OID then, so adopting the +/// fetched head would point the diff/full-file view at a stale revision. async fn fetch_prs_by_filter( state: State<'_, Arc>, filter: github::PrFilter, @@ -2437,10 +2446,40 @@ async fn fetch_prs_by_filter( if state.reviews.get(&pr_ref).is_some() { let branch = pr.head_ref_name.clone(); let base = pr.base_ref_name.clone(); + let head_sha = pr.head_ref_oid.clone(); + let ci_summary = github::rollup_to_summary(&pr.status_check_rollup); state.reviews.update(&pr_ref, |r| { r.diff = cockpit_core::model::DiffData { raw: diff }; r.branch = branch; r.base = base; + + // Refresh CI + the pinned head SHA from GitHub. A failed per-PR + // enrichment falls back to an empty rollup / empty OID; treat + // those as "no fresh data" and keep what we already have rather + // than degrading the diff resolution to a branch-name lookup. + // + // `base_sha` is deliberately NOT refreshed here: it is the restack + // fork point (see `Review::base_sha`), not the base branch tip, so + // the kickoff-computed value must never be clobbered by a GitHub + // read (that would break restack once the base advances). + if let Some(ci) = ci_summary { + r.ci_summary = Some(ci); + } + // The head SHA is authoritative locally while a rework is in + // flight: a review with an attached agent, in `Dispatched`, or in + // `Reworked` has a worktree HEAD that leads what GitHub last + // reported, so adopting the fetched head here would point the + // diff/full-file view at a stale revision. `Reworked` in + // particular: after `apply_agent_completion` the local worktree + // HEAD leads GitHub until the agent's push is visible, and + // reverting it would make the interdiff read empty. Only take + // GitHub's head OID when no rework owns the branch. + let rework_in_flight = r.agent.is_some() + || r.gate_state == GateState::Dispatched + || r.gate_state == GateState::Reworked; + if !rework_in_flight && !head_sha.is_empty() { + r.head_sha = head_sha; + } }); if let Some(updated) = state.reviews.get(&pr_ref) { reviews.push(updated); diff --git a/app/src/App.tsx b/app/src/App.tsx index 2148c42..5ed0c5e 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -11,6 +11,7 @@ import type { ShortcutId } from "./lib/shortcuts"; import { Sidebar } from "./components/Sidebar"; import { ReviewCard } from "./components/ReviewCard"; import { StackContainer } from "./components/StackContainer"; +import { FastLaneShelf } from "./components/FastLaneShelf"; import { ProjectCard } from "./components/ProjectCard"; import { ReviewWorkspace } from "./components/ReviewWorkspace"; import { PlanView } from "./components/PlanView"; @@ -38,6 +39,7 @@ import { } from "lucide-react"; import { cn } from "@/lib/utils"; import { buildBoardItems } from "./lib/stack-tree"; +import { isFastLane } from "./lib/attention"; import type { CardDensity } from "./components/ReviewCard"; import type { GateState } from "./bindings/GateState"; import type { Review } from "./bindings/Review"; @@ -514,13 +516,18 @@ function App() { function renderProjectGroupedList(items: readonly Review[]) { const filtered = filterReviews(items); - const groups = groupReviewsByProject(filtered, projects); - - if (groups.length === 0) { + // Fast lane filters AFTER search/state filters, then its members are + // excluded from the project groups below so nothing renders twice. + const fastLane = filtered.filter(isFastLane); + const fastLaneIds = new Set(fastLane.map((r) => r.id)); + const rest = filtered.filter((r) => !fastLaneIds.has(r.id)); + const groups = groupReviewsByProject(rest, projects); + + if (fastLane.length === 0 && groups.length === 0) { return null; } - return groups.map((group) => ( + const projectSections = groups.map((group) => (

{group.title}{" "} @@ -552,6 +559,20 @@ function App() {

)); + + return ( + <> + {fastLane.length > 0 && ( + + )} + {projectSections} + + ); } function renderPrsContent(items: readonly Review[], emptyIcon: LucideIcon, emptyTitle: string, emptyDescription: string) { diff --git a/app/src/components/FastLaneShelf.tsx b/app/src/components/FastLaneShelf.tsx new file mode 100644 index 0000000..391b442 --- /dev/null +++ b/app/src/components/FastLaneShelf.tsx @@ -0,0 +1,72 @@ +/** + * Fast lane shelf (C2): a visually distinct, teal-framed group of + * "small + green + low-risk" reviews surfaced above the project groups. + * + * The shelf compresses the *decision* — these are the reviews a human can + * approve in ~2 minutes — but never the *authority*: its cards use the exact + * same {@link ReviewCard} action path as the rest of the board, so there is + * deliberately no batch-approve or auto-approve here (roadmap C2 / CLAUDE.md + * §9). Membership is decided by {@link isFastLane}; App excludes members from + * the project groups below so nothing renders twice. + */ + +import { Zap } from "lucide-react"; +import { ReviewCard } from "./ReviewCard"; +import type { CardDensity } from "./ReviewCard"; +import type { Review } from "../bindings/Review"; +import { attentionRank } from "../lib/attention"; + +interface FastLaneShelfProps { + /** The fast-lane members to render; the shelf is not rendered when empty. */ + readonly reviews: readonly Review[]; + /** Presentation density, mirroring the rest of the board. */ + readonly density: CardDensity; + /** Same handler the board uses — no new terminal action is introduced here. */ + readonly onAction: (pr: string) => void; + /** Restack a stale member; wired through for parity with the board cards. */ + readonly onRestack: (pr: string) => void; +} + +/** Order the shelf attention-first, using the board's rank + id tiebreak. */ +function orderByAttention(reviews: readonly Review[]): readonly Review[] { + return [...reviews].sort((a, b) => { + const byRank = attentionRank(a) - attentionRank(b); + if (byRank !== 0) return byRank; + return a.id < b.id ? -1 : a.id > b.id ? 1 : 0; + }); +} + +/** The teal-framed fast-lane shelf. */ +export function FastLaneShelf({ + reviews, + density, + onAction, + onRestack, +}: FastLaneShelfProps) { + const ordered = orderByAttention(reviews); + const count = ordered.length; + + return ( +
+

+

+
+ {ordered.map((review) => ( + + ))} +
+
+ ); +} diff --git a/app/src/components/ReviewCard.test.tsx b/app/src/components/ReviewCard.test.tsx index d9cac86..1ae2f56 100644 --- a/app/src/components/ReviewCard.test.tsx +++ b/app/src/components/ReviewCard.test.tsx @@ -4,6 +4,15 @@ import userEvent from "@testing-library/user-event"; import { ReviewCard } from "./ReviewCard"; import { makeReview, makeAgentRun, ALL_GATE_STATES } from "../test/fixtures"; +/** A diff adding `n` lines to `path`, for exercising the risk chips. */ +function addLines(path: string, n: number): string { + let s = `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -0,0 +1,${String(n)} @@\n`; + for (let i = 0; i < n; i++) { + s += `+row ${String(i)}\n`; + } + return s; +} + describe("ReviewCard", () => { it("shows the Restack button only when the review is stale", () => { const fresh = makeReview({ stale: false }); @@ -119,4 +128,71 @@ describe("ReviewCard", () => { screen.getByRole("button", { name: "Review" }), ).toBeInTheDocument(); }); + + it("renders a CI x/y chip from the review's ci_summary", () => { + render( + , + ); + expect(screen.getByText("2/3")).toBeInTheDocument(); + }); + + it("adds the F6 splitting nudge to the size chip past 400 changed lines", () => { + render( + , + ); + expect( + screen.getByTitle("Consider splitting (>400 changed lines)"), + ).toBeInTheDocument(); + }); + + it("surfaces a sensitive-path chip for a risky file", () => { + render( + , + ); + expect(screen.getByText("migrations")).toBeInTheDocument(); + }); + + it("surfaces a test-touch chip when the diff touches tests", () => { + render( + , + ); + expect(screen.getByText("tests")).toBeInTheDocument(); + }); + + it("layers a risk note under the gate reason", () => { + render( + , + ); + expect(screen.getByText(/needs your review/i)).toBeInTheDocument(); + expect(screen.getByText(/CI failing/)).toBeInTheDocument(); + }); }); diff --git a/app/src/components/ReviewCard.tsx b/app/src/components/ReviewCard.tsx index da828b8..3390657 100644 --- a/app/src/components/ReviewCard.tsx +++ b/app/src/components/ReviewCard.tsx @@ -1,11 +1,22 @@ +import { useMemo } from "react"; import { Button } from "@/components/ui/button"; -import { Layers } from "lucide-react"; +import { Layers, ShieldAlert } from "lucide-react"; import { cn } from "@/lib/utils"; import type { Review } from "../bindings/Review"; import type { GateState } from "../bindings/GateState"; +import type { SizeClass } from "../bindings/SizeClass"; +import type { RiskFlag } from "../bindings/RiskFlag"; +import type { CiSummary } from "../bindings/CiSummary"; import { cardSignal } from "../lib/card-signal"; import type { SignalTone } from "../lib/card-signal"; import { diffStats } from "../diff-parser"; +import { ciState } from "../lib/ci"; +import { + diffTotals, + sizeClass, + sensitiveFlags, + touchesTests, +} from "../lib/diff-signals"; /** Presentation density for the board. */ export type CardDensity = "cards" | "compact"; @@ -189,6 +200,173 @@ function TelemetryChips({ review }: { readonly review: Review }) { ); } +/** Semantic tone for a risk chip's status dot. */ +type ChipTone = "pass" | "fail" | "pending" | "warning" | "neutral"; + +/** Dot color for a risk chip, using the semantic `--color-*` tokens. */ +function chipDotClass(tone: ChipTone): string { + switch (tone) { + case "pass": + return "bg-success"; + case "fail": + return "bg-danger"; + case "pending": + return "bg-warning"; + case "warning": + return "bg-warning"; + case "neutral": + return "bg-muted-foreground"; + default: + return assertNever(tone); + } +} + +/** Overall CI tone for the x/y chip, from the rolled-up summary. */ +function ciSummaryTone(ci: CiSummary): ChipTone { + const state = ciState(ci); + switch (state) { + case "pass": + return "pass"; + case "fail": + return "fail"; + case "pending": + return "pending"; + case "none": + return "neutral"; + default: + return assertNever(state); + } +} + +/** Short label for a diff size bucket (`Xl` renders as `XL`). */ +function sizeLabel(size: SizeClass): string { + switch (size) { + case "S": + return "S"; + case "M": + return "M"; + case "L": + return "L"; + case "Xl": + return "XL"; + default: + return assertNever(size); + } +} + +/** Short label for a sensitive-path risk flag. */ +function riskLabel(flag: RiskFlag): string { + switch (flag) { + case "Migration": + return "migrations"; + case "Lockfile": + return "lockfile"; + case "CiConfig": + return "CI config"; + case "Auth": + return "auth"; + case "GithubDir": + return ".github"; + case "Dependency": + return "deps"; + default: + return assertNever(flag); + } +} + +/** A small, calm status chip: a semantic dot plus a mono label. */ +function RiskChip({ + tone, + title, + children, +}: { + readonly tone: ChipTone; + readonly title?: string | undefined; + readonly children: React.ReactNode; +}) { + return ( + + + ); +} + +/** + * Deterministic diff-derived risk chips shared by both densities: CI x/y, diff + * size class, a sensitive-path flag, and a test-touch marker. The four signals + * the research says carry most of the routing value (C3). All are parsed from + * `review.diff.raw` once per render (memoized on the raw diff). The size chip + * carries the F6 ">400 changed lines" splitting nudge as a warning tone. + */ +function RiskChips({ review }: { readonly review: Review }) { + const signals = useMemo(() => { + const totals = diffTotals(review.diff.raw); + const flags = sensitiveFlags(review.diff.raw); + return { + total: totals.additions + totals.deletions, + size: sizeClass(totals.additions, totals.deletions), + firstFlag: flags[0] ?? null, + hasTests: touchesTests(review.diff.raw), + }; + }, [review.diff.raw]); + + const ci = review.ci_summary; + // F6: nudge splitting once a diff crosses 400 changed lines. + const oversized = signals.total > 400; + + return ( + <> + {ci !== undefined && ci.total > 0 && ( + + CI + + {String(ci.passed)}/{String(ci.total)} + + + )} + + {signals.total > 0 && ( + 400 changed lines)" : undefined + } + > + + {sizeLabel(signals.size)} + + + )} + + {signals.firstFlag !== null && ( + + + )} + + {signals.hasTests && ( + + tests + + )} + + ); +} + /** Restack button; only meaningful for stale reviews. */ function RestackButton({ review, @@ -251,8 +429,14 @@ export function ReviewCard({ {signal.reason} + {signal.note !== undefined && ( + + · {signal.note} + + )} + {repo !== "" && ( @@ -289,14 +473,15 @@ export function ReviewCard({
- {/* Reason line — the card's headline signal. */} -
+ {signal.reason} + {signal.note !== undefined && ( + + · {signal.note} + )} - > - {signal.reason}
{/* Title = the branch/PR subject. */} @@ -320,9 +505,10 @@ export function ReviewCard({ )}
- {/* Telemetry chips. */} + {/* Telemetry + risk chips. */}
+
diff --git a/app/src/lib/attention.test.ts b/app/src/lib/attention.test.ts index 42d2f17..3819ca8 100644 --- a/app/src/lib/attention.test.ts +++ b/app/src/lib/attention.test.ts @@ -1,13 +1,48 @@ import { describe, it, expect } from "vitest"; -import { sortByAttention, attentionRank } from "./attention"; +import { + sortByAttention, + attentionRank, + attentionReasons, + isFastLane, +} from "./attention"; import { makeReview, ALL_GATE_STATES } from "../test/fixtures"; import type { Review } from "../bindings/Review"; +import type { CiSummary } from "../bindings/CiSummary"; /** Read the gate-state order out of a sorted list, for readable assertions. */ function states(reviews: readonly Review[]): readonly string[] { return reviews.map((r) => r.gate_state); } +/** A diff adding `n` lines to `path`. Size = n changed lines. */ +function addLines(path: string, n: number): string { + let s = `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -0,0 +1,${String(n)} @@\n`; + for (let i = 0; i < n; i++) { + s += `+row ${String(i)}\n`; + } + return s; +} + +const SMALL = addLines("data.txt", 10); // S, non-sensitive +const LARGE = addLines("data.txt", 500); // L +const XL = addLines("data.txt", 700); // Xl +const SENSITIVE = addLines("migrations/001_init.sql", 5); // S, Migration flag +const XL_SENSITIVE = addLines("migrations/big.sql", 700); // Xl + Migration + +const CI_GREEN: CiSummary = { passed: 3, total: 3, failed: 0, pending: 0 }; +const CI_FAIL: CiSummary = { passed: 1, total: 2, failed: 1, pending: 0 }; +const CI_PENDING: CiSummary = { passed: 1, total: 2, failed: 0, pending: 1 }; + +/** Gate states in attention (rank) order, most-urgent first. */ +const RANK_ORDER = [ + "Reworked", + "InReview", + "Pending", + "Dispatched", + "Approved", + "Merged", +] as const; + describe("sortByAttention", () => { it("orders the reviewer's active queue ahead of settled work", () => { const reviews = ALL_GATE_STATES.map((state, i) => @@ -79,3 +114,148 @@ describe("sortByAttention", () => { expect(attentionRank(stale)).toBeGreaterThan(attentionRank(fresh)); }); }); + +describe("attentionRank — within-bucket adjustments never cross buckets", () => { + it("keeps every higher bucket ahead of the next even at worst-case signals", () => { + // A maximally-sinking review (CI failing + XL + sensitive) in the higher + // bucket must still outrank a maximally-rising review (CI green + small) in + // the adjacent lower bucket, for every adjacent pair in rank order. + RANK_ORDER.forEach((state, i) => { + const next = RANK_ORDER[i + 1]; + if (next === undefined) return; + const higher = makeReview({ + gate_state: state, + ci_summary: CI_FAIL, + diff: { raw: XL_SENSITIVE }, + }); + const lower = makeReview({ + gate_state: next, + ci_summary: CI_GREEN, + diff: { raw: SMALL }, + }); + expect(attentionRank(higher)).toBeLessThan(attentionRank(lower)); + }); + }); + + it("within a bucket, CI-green sorts ahead of CI-failing", () => { + const green = makeReview({ id: "g", gate_state: "Pending", ci_summary: CI_GREEN, diff: { raw: SMALL } }); + const red = makeReview({ id: "r", gate_state: "Pending", ci_summary: CI_FAIL, diff: { raw: SMALL } }); + expect(attentionRank(green)).toBeLessThan(attentionRank(red)); + }); + + it("within a bucket, a small diff sorts ahead of an XL diff", () => { + const small = makeReview({ id: "s", gate_state: "InReview", diff: { raw: SMALL } }); + const xl = makeReview({ id: "x", gate_state: "InReview", diff: { raw: XL } }); + expect(attentionRank(small)).toBeLessThan(attentionRank(xl)); + }); + + it("within a bucket, a sensitive path sinks a review", () => { + const plain = makeReview({ id: "p", gate_state: "InReview", diff: { raw: SMALL } }); + const sensitive = makeReview({ id: "z", gate_state: "InReview", diff: { raw: SENSITIVE } }); + expect(attentionRank(sensitive)).toBeGreaterThan(attentionRank(plain)); + }); +}); + +describe("attentionReasons", () => { + it("is empty for a clean, small, green review", () => { + const clean = makeReview({ ci_summary: CI_GREEN, diff: { raw: SMALL } }); + expect(attentionReasons(clean)).toEqual([]); + }); + + it("reports CI failing", () => { + const red = makeReview({ ci_summary: CI_FAIL, diff: { raw: SMALL } }); + expect(attentionReasons(red)).toContain("CI failing"); + }); + + it("does not report CI pending (a mild rank signal, not a headline)", () => { + const pending = makeReview({ ci_summary: CI_PENDING, diff: { raw: SMALL } }); + expect(attentionReasons(pending)).toEqual([]); + }); + + it("distinguishes large from very large diffs", () => { + expect(attentionReasons(makeReview({ diff: { raw: LARGE } }))).toContain("Large diff"); + expect(attentionReasons(makeReview({ diff: { raw: XL } }))).toContain("Very large diff"); + }); + + it("names the sensitive path touched", () => { + const sensitive = makeReview({ diff: { raw: SENSITIVE } }); + expect(attentionReasons(sensitive)).toContain("Touches migrations"); + }); + + it("orders reasons: CI failing, then size, then sensitive paths", () => { + const review = makeReview({ ci_summary: CI_FAIL, diff: { raw: XL_SENSITIVE } }); + expect(attentionReasons(review)).toEqual([ + "CI failing", + "Very large diff", + "Touches migrations", + ]); + }); +}); + +describe("isFastLane", () => { + it("includes an actionable, small, green, low-risk review", () => { + const review = makeReview({ gate_state: "Pending", ci_summary: CI_GREEN, diff: { raw: SMALL } }); + expect(isFastLane(review)).toBe(true); + }); + + it("accepts every actionable gate state", () => { + for (const state of ["Pending", "InReview", "Reworked"] as const) { + const review = makeReview({ gate_state: state, ci_summary: CI_GREEN, diff: { raw: SMALL } }); + expect(isFastLane(review)).toBe(true); + } + }); + + it("excludes settled and agent-owned states", () => { + for (const state of ["Dispatched", "Approved", "Merged"] as const) { + const review = makeReview({ gate_state: state, ci_summary: CI_GREEN, diff: { raw: SMALL } }); + expect(isFastLane(review)).toBe(false); + } + }); + + it("excludes a stale review", () => { + const review = makeReview({ gate_state: "InReview", stale: true, ci_summary: CI_GREEN, diff: { raw: SMALL } }); + expect(isFastLane(review)).toBe(false); + }); + + it("excludes a large diff (>= 200 changed lines)", () => { + const review = makeReview({ gate_state: "Pending", ci_summary: CI_GREEN, diff: { raw: LARGE } }); + expect(isFastLane(review)).toBe(false); + }); + + it("excludes a failing or pending CI", () => { + const red = makeReview({ gate_state: "Pending", ci_summary: CI_FAIL, diff: { raw: SMALL } }); + const pending = makeReview({ gate_state: "Pending", ci_summary: CI_PENDING, diff: { raw: SMALL } }); + expect(isFastLane(red)).toBe(false); + expect(isFastLane(pending)).toBe(false); + }); + + it("excludes a review with no loaded CI (needs positive green evidence)", () => { + const review = makeReview({ gate_state: "Pending", diff: { raw: SMALL } }); + expect(isFastLane(review)).toBe(false); + }); + + it("excludes a review touching a sensitive path", () => { + const review = makeReview({ gate_state: "Pending", ci_summary: CI_GREEN, diff: { raw: SENSITIVE } }); + expect(isFastLane(review)).toBe(false); + }); + + it("excludes a parented (non-root) review to keep stacks intact", () => { + // A child that is otherwise fast-lane-eligible must stay in its stack group + // so it never surfaces ahead of its unreviewed parent (frontier-root-first). + const child = makeReview({ + gate_state: "Pending", + ci_summary: CI_GREEN, + diff: { raw: SMALL }, + parents: ["rev-parent"], + }); + expect(isFastLane(child)).toBe(false); + // The same review with no parents (a stack root) does qualify. + const root = makeReview({ + gate_state: "Pending", + ci_summary: CI_GREEN, + diff: { raw: SMALL }, + parents: [], + }); + expect(isFastLane(root)).toBe(true); + }); +}); diff --git a/app/src/lib/attention.ts b/app/src/lib/attention.ts index ea582cf..7e98488 100644 --- a/app/src/lib/attention.ts +++ b/app/src/lib/attention.ts @@ -1,15 +1,24 @@ /** - * Attention-first ordering for the PRs board. + * Attention-first ordering and triage signals for the PRs board (C1/C2). * - * The board leads with what needs the reviewer. `sortByAttention` produces a - * stable ordering where reviews awaiting a human decision rise and settled or - * blocked-by-ancestor work sinks. CI status is intentionally *not* an input: - * the list-item `Review` does not carry loaded checks, so the rank is derived - * only from fields present on every review (gate state, staleness, comments). + * The board leads with what needs the reviewer. `attentionRank` produces a + * numeric key where reviews awaiting a human decision rise and settled or + * blocked-by-ancestor work sinks. The rank is *layered*: the gate state is the + * primary bucket, and small documented within-bucket adjustments (CI status, + * diff size, path sensitivity) reorder reviews *inside* a bucket without ever + * crossing a bucket boundary — a Reworked review always outranks an Approved one + * regardless of CI or size. `attentionReasons` exposes the human-readable WHY + * behind those adjustments for the cards, and `isFastLane` picks out the + * "small + green + low-risk" reviews for the fast-lane shelf. */ import type { Review } from "../bindings/Review"; import type { GateState } from "../bindings/GateState"; +import type { SizeClass } from "../bindings/SizeClass"; +import type { RiskFlag } from "../bindings/RiskFlag"; +import type { CiState } from "./ci"; +import { ciState } from "./ci"; +import { diffTotals, sizeClass, sensitiveFlags } from "./diff-signals"; function assertNever(x: never): never { throw new Error(`unreachable: ${String(x)}`); @@ -40,25 +49,272 @@ function gateStateRank(state: GateState): number { } } +/** + * Within-bucket attention adjustments (C1). + * + * These are *secondary* terms added on top of the integer gate-state bucket + * rank. Their combined span is deliberately kept below 1.0 — the spacing + * between two adjacent gate-state buckets — so they can reorder reviews *within* + * a bucket but can NEVER move one across a bucket boundary. The most a review + * can rise is `CI_PASS + SIZE_SMALL = -0.20`; the most it can sink is + * `CI_FAIL + SIZE_XL + SENSITIVE = +0.50`; the total span (0.70) is < 1.0, so + * the needs-human ordering is invariant. + * + * Sign convention: negative = rises (more attention), positive = sinks. The + * roadmap's routing rule (C1/C2): small + green surfaces for the fast lane; + * red / XL / risky sinks toward deep review. A CI-failing PR still *needs* + * attention (for a fix-CI decision), but it is a deep-review decision, not a + * 2-minute one, so it sinks within its bucket rather than leading it. + */ +const CI_PASS = -0.1; // all checks green — a decision you can make in 2 minutes. +const CI_PENDING = 0.05; // not yet decidable — mildly deprioritized. +const CI_FAIL = 0.15; // needs a deeper fix-CI decision — sinks. +const SIZE_SMALL = -0.1; // an S diff reviews fast — rises. +const SIZE_LARGE = 0.1; // an L diff needs a careful read — sinks. +const SIZE_XL = 0.2; // an XL diff sinks hardest. +const SENSITIVE = 0.15; // auth/migrations/config/etc. warrant scrutiny — sinks. + +/** The rolled-up CI state for a review, or `"none"` when no checks are loaded. */ +function reviewCiState(review: Review): CiState { + const ci = review.ci_summary; + return ci === undefined ? "none" : ciState(ci); +} + +/** Within-bucket adjustment from CI status. */ +function ciAdjustment(state: CiState): number { + switch (state) { + case "pass": + return CI_PASS; + case "pending": + return CI_PENDING; + case "fail": + return CI_FAIL; + case "none": + return 0; + default: + return assertNever(state); + } +} + +/** Within-bucket adjustment from diff size class. */ +function sizeAdjustment(size: SizeClass): number { + switch (size) { + case "S": + return SIZE_SMALL; + case "M": + return 0; + case "L": + return SIZE_LARGE; + case "Xl": + return SIZE_XL; + default: + return assertNever(size); + } +} + +/** + * The memoized triage signals derived for a single review: its rank plus the + * diff-scan results the cards and the fast lane read. Bundled so a review's + * (potentially large) diff is parsed at most once, no matter how many of + * `attentionRank` / `attentionReasons` / `isFastLane` / the stack-tree sort keys + * touch it within a render pass. + */ +interface ReviewSignals { + /** Full attention rank (lower sorts first). */ + readonly rank: number; + /** Rolled-up CI state (`"none"` when no checks are loaded). */ + readonly ci: CiState; + /** Diff size class, from the add/del totals. */ + readonly size: SizeClass; + /** Total changed lines (additions + deletions), for the fast-lane cutoff. */ + readonly totalLines: number; + /** Sensitive-path risk flags, at most one per touched file, in diff order. */ + readonly sensitiveFlags: readonly RiskFlag[]; + /** Whether the diff touches any sensitive path. */ + readonly hasSensitive: boolean; +} + +/** + * Per-review signal cache, keyed on object identity. + * + * A `WeakMap` is safe against staleness precisely because it is keyed + * on identity: every fetch/refresh replaces a review with a *fresh* object, so a + * mutated review can never hit a stale entry (the old object is simply gone and + * gets garbage-collected with its cache slot). The cache lives only to spare + * repeated diff scans of the *same* object within a single render pass. + */ +const signalsCache = new WeakMap(); + +/** Compute (or return the cached) triage signals for a review. */ +function signalsFor(review: Review): ReviewSignals { + const cached = signalsCache.get(review); + if (cached !== undefined) { + return cached; + } + + const { additions, deletions } = diffTotals(review.diff.raw); + const size = sizeClass(additions, deletions); + const flags = sensitiveFlags(review.diff.raw); + const hasSensitive = flags.length > 0; + const ci = reviewCiState(review); + + const stalePenalty = review.stale ? 100 : 0; + const within = + ciAdjustment(ci) + sizeAdjustment(size) + (hasSensitive ? SENSITIVE : 0); + const rank = stalePenalty + gateStateRank(review.gate_state) + within; + + const signals: ReviewSignals = { + rank, + ci, + size, + totalLines: additions + deletions, + sensitiveFlags: flags, + hasSensitive, + }; + signalsCache.set(review, signals); + return signals; +} + /** * Full attention rank for a review. Lower sorts first. A stale review is * deprioritized past every non-stale one (it is blocked on an ancestor's - * rework), while preserving relative order among stale reviews by gate state. + * rework) via a large `+100` penalty, while the gate-state bucket and the small + * within-bucket signal adjustments order the rest. */ export function attentionRank(review: Review): number { - const stalePenalty = review.stale ? 100 : 0; - return stalePenalty + gateStateRank(review.gate_state); + return signalsFor(review).rank; +} + +/** Human-readable phrase for a sensitive-path risk flag. */ +function riskReason(flag: RiskFlag): string { + switch (flag) { + case "Migration": + return "Touches migrations"; + case "Lockfile": + return "Touches lockfile"; + case "CiConfig": + return "Touches CI config"; + case "Auth": + return "Touches auth"; + case "GithubDir": + return "Touches .github"; + case "Dependency": + return "Touches dependencies"; + default: + return assertNever(flag); + } +} + +/** + * The human-readable reasons a review carries extra risk, in priority order: + * CI failing first, then a large-diff note, then one entry per distinct + * sensitive path touched. These mirror the within-bucket rank adjustments and + * are what the cards surface as the "why". Returns `[]` for a clean, small, + * green review (its gate reason stands alone). + */ +export function attentionReasons(review: Review): string[] { + const { ci, size, sensitiveFlags: flags } = signalsFor(review); + const reasons: string[] = []; + + if (ci === "fail") { + reasons.push("CI failing"); + } + + if (size === "Xl") { + reasons.push("Very large diff"); + } else if (size === "L") { + reasons.push("Large diff"); + } + + // One reason per distinct flag, in first-seen order (dedupe repeats). + const seen = new Set(); + for (const flag of flags) { + if (!seen.has(flag)) { + seen.add(flag); + reasons.push(riskReason(flag)); + } + } + + return reasons; +} + +/** Whether a review is awaiting a human decision (not blocked, not settled). */ +function isActionable(review: Review): boolean { + if (review.stale) { + return false; + } + return ( + review.gate_state === "Pending" || + review.gate_state === "InReview" || + review.gate_state === "Reworked" + ); +} + +/** + * The upper bound (exclusive) on changed lines for the fast lane. `< 200` covers + * the whole `S` bucket (< 50) *and* the whole `M` bucket (`M` is [50, 200)), + * matching the roadmap's "small" definition; `L`/`Xl` (>= 200) never qualify. + */ +const FAST_LANE_MAX_LINES = 200; + +/** + * Whether a review belongs in the fast-lane shelf (C2): an actionable, + * "small + green + low-risk" review whose decision should take ~2 minutes. + * + * A review qualifies when it is actionable (Pending/InReview/Reworked and not + * stale), it is a stack ROOT (no parents), its diff is small (< + * {@link FAST_LANE_MAX_LINES} changed lines), CI is present and fully green, and + * it touches no sensitive paths. The test-weakening signal is deliberately *not* + * consulted: it is computed server-side and is not part of the card-subset TS + * mirror, so the fast lane is defined on size + CI + paths only (a weakening flag + * still surfaces later at the diff gate). + * + * The stack-root requirement keeps stacks intact: surfacing a child in the fast + * lane ahead of its unreviewed parent violates frontier-root-first ordering and + * leaves a visual hole in the stack below it, so a parented review stays in its + * stack group even when it is small + green + low-risk. + * + * The fast lane compresses the *decision*, never the *authority*: it is not + * auto-approve and grants no new terminal action (see roadmap C2 / CLAUDE.md + * §9). + */ +export function isFastLane(review: Review): boolean { + if (!isActionable(review)) { + return false; + } + // Only a stack root (no parents) may enter the fast lane — see the note above. + if (review.parents.length > 0) { + return false; + } + const { totalLines, ci, hasSensitive } = signalsFor(review); + if (totalLines >= FAST_LANE_MAX_LINES) { + return false; + } + // CI must be loaded and fully green; absent CI ("none") does not qualify. + if (ci !== "pass") { + return false; + } + if (hasSensitive) { + return false; + } + return true; } /** * Return a new array of reviews sorted attention-first. Stable within equal - * ranks (uses `id` as the tiebreaker) so ordering is deterministic. Does not - * mutate the input. + * ranks (uses `id` as the tiebreaker) so ordering is deterministic. Ranks are + * computed once per review (not per comparison) so a large diff is parsed only + * once. Does not mutate the input. */ export function sortByAttention(reviews: readonly Review[]): readonly Review[] { - return [...reviews].sort((a, b) => { - const byRank = attentionRank(a) - attentionRank(b); + const ranked = reviews.map((review) => ({ + review, + rank: attentionRank(review), + })); + ranked.sort((a, b) => { + const byRank = a.rank - b.rank; if (byRank !== 0) return byRank; - return a.id < b.id ? -1 : a.id > b.id ? 1 : 0; + return a.review.id < b.review.id ? -1 : a.review.id > b.review.id ? 1 : 0; }); + return ranked.map((r) => r.review); } diff --git a/app/src/lib/card-signal.test.ts b/app/src/lib/card-signal.test.ts new file mode 100644 index 0000000..f7abb57 --- /dev/null +++ b/app/src/lib/card-signal.test.ts @@ -0,0 +1,58 @@ +import { describe, it, expect } from "vitest"; +import { cardSignal } from "./card-signal"; +import { makeReview } from "../test/fixtures"; +import type { CiSummary } from "../bindings/CiSummary"; + +/** A diff adding `n` lines to `path`. */ +function addLines(path: string, n: number): string { + let s = `diff --git a/${path} b/${path}\n--- a/${path}\n+++ b/${path}\n@@ -0,0 +1,${String(n)} @@\n`; + for (let i = 0; i < n; i++) { + s += `+row ${String(i)}\n`; + } + return s; +} + +const CI_FAIL: CiSummary = { passed: 1, total: 2, failed: 1, pending: 0 }; +const LARGE = addLines("data.txt", 500); + +describe("cardSignal — risk note layering", () => { + it("keeps the gate reason as the headline and adds the risk note", () => { + const signal = cardSignal( + makeReview({ gate_state: "Pending", ci_summary: CI_FAIL }), + ); + expect(signal.reason).toBe("Needs your review"); + expect(signal.note).toBe("CI failing"); + }); + + it("notes the sharpest reason for a reworked review", () => { + const signal = cardSignal( + makeReview({ gate_state: "Reworked", diff: { raw: LARGE } }), + ); + expect(signal.reason).toBe("Agent reworked — re-review"); + expect(signal.note).toBe("Large diff"); + }); + + it("omits the note for a clean actionable review", () => { + const signal = cardSignal(makeReview({ gate_state: "InReview" })); + expect(signal.note).toBeUndefined(); + }); + + it("lets the stale reason take precedence over any risk note", () => { + const signal = cardSignal( + makeReview({ stale: true, gate_state: "InReview", ci_summary: CI_FAIL }), + ); + expect(signal.reason).toBe("Restack needed"); + expect(signal.note).toBeUndefined(); + }); + + it("does not note risk on non-actionable states", () => { + const dispatched = cardSignal( + makeReview({ gate_state: "Dispatched", ci_summary: CI_FAIL }), + ); + const approved = cardSignal( + makeReview({ gate_state: "Approved", ci_summary: CI_FAIL }), + ); + expect(dispatched.note).toBeUndefined(); + expect(approved.note).toBeUndefined(); + }); +}); diff --git a/app/src/lib/card-signal.ts b/app/src/lib/card-signal.ts index 89143e3..7f80ed9 100644 --- a/app/src/lib/card-signal.ts +++ b/app/src/lib/card-signal.ts @@ -11,6 +11,7 @@ import type { Review } from "../bindings/Review"; import type { GateState } from "../bindings/GateState"; import { elapsedSince } from "./relative-time"; +import { attentionReasons } from "./attention"; function assertNever(x: never): never { throw new Error(`unreachable: ${String(x)}`); @@ -30,6 +31,23 @@ export interface CardSignal { readonly reason: string; /** Semantic tone driving the reason line color. */ readonly tone: SignalTone; + /** + * Optional secondary risk note (e.g. `CI failing`, `Large diff`) layered + * *under* the gate reason on the same line. Present only for actionable + * reviews that carry a risk signal; the gate/stale reason always leads. + */ + readonly note?: string; +} + +/** + * Attach the sharpest risk note to an actionable review's signal, keeping the + * gate reason as the loud headline. Precedence follows {@link attentionReasons} + * (CI failing, then size, then sensitive paths); `undefined` when the review is + * clean, so `note` is omitted rather than set empty. + */ +function withRiskNote(signal: CardSignal, review: Review): CardSignal { + const note = attentionReasons(review)[0]; + return note === undefined ? signal : { ...signal, note }; } /** @@ -46,11 +64,20 @@ export function cardSignal(review: Review, now: number = Date.now()): CardSignal const state: GateState = review.gate_state; switch (state) { case "Pending": - return { reason: "Needs your review", tone: "attention" }; + return withRiskNote( + { reason: "Needs your review", tone: "attention" }, + review, + ); case "InReview": - return { reason: "Needs your review", tone: "attention" }; + return withRiskNote( + { reason: "Needs your review", tone: "attention" }, + review, + ); case "Reworked": - return { reason: "Agent reworked — re-review", tone: "attention" }; + return withRiskNote( + { reason: "Agent reworked — re-review", tone: "attention" }, + review, + ); case "Dispatched": { const elapsed = review.agent !== null diff --git a/app/src/lib/ci.test.ts b/app/src/lib/ci.test.ts index 8f9606f..6898834 100644 --- a/app/src/lib/ci.test.ts +++ b/app/src/lib/ci.test.ts @@ -47,6 +47,13 @@ describe("checkOutcome", () => { ); }); + it("classifies commit-status 'error' as fail (mirrors Rust classify_check_signal)", () => { + // The legacy commit-status `ERROR` state is a failure — it must not fall + // through to the pending default. Mirrors the Rust `error` -> fail arm. + expect(checkOutcome(check({ bucket: "error" }))).toBe("fail"); + expect(checkOutcome(check({ bucket: "", state: "ERROR" }))).toBe("fail"); + }); + it("is case-insensitive on the signal", () => { expect(checkOutcome(check({ bucket: "PASS" }))).toBe("pass"); expect(checkOutcome(check({ bucket: "", state: "Failure" }))).toBe("fail"); diff --git a/app/src/lib/ci.ts b/app/src/lib/ci.ts index f095dec..3c4cba7 100644 --- a/app/src/lib/ci.ts +++ b/app/src/lib/ci.ts @@ -19,6 +19,11 @@ export type CiState = "pass" | "fail" | "pending" | "none"; /** * Classify a single check outcome. Mirrors the Rust `summarize` rules: * neutral/skipped/cancelled count as pass; unknown states count as pending. + * + * CROSS-LANGUAGE MIRROR: these arms reproduce `classify_check_signal` in + * `cockpit-core`'s github adapter one-for-one (incl. the commit-status `error` + * -> fail). Any signal added or moved on the Rust side must be mirrored here, + * and vice versa, or the board badge and the server rollup will disagree. */ export function checkOutcome(check: CiCheck): CheckOutcome { const signal = (check.bucket !== "" ? check.bucket : check.state).toLowerCase(); @@ -38,6 +43,7 @@ export function checkOutcome(check: CiCheck): CheckOutcome { case "action_required": case "startup_failure": case "stale": + case "error": return "fail"; default: return "pending"; diff --git a/crates/cockpit-core/src/adapters/github.rs b/crates/cockpit-core/src/adapters/github.rs index b0c8e7a..e3eb999 100644 --- a/crates/cockpit-core/src/adapters/github.rs +++ b/crates/cockpit-core/src/adapters/github.rs @@ -73,6 +73,16 @@ pub struct PrData { /// Repository slug (e.g. "Nexcade/garage"). Present for cross-repo searches. #[serde(default)] pub repo_slug: String, + /// The PR's `statusCheckRollup` entries. Empty for legacy field sets (which + /// did not request the rollup), so defaulted; rolled up via + /// [`rollup_to_summary`]. + #[serde(default)] + pub status_check_rollup: Vec, + /// Head commit SHA (`headRefOid`), pinned at fetch so the diff/full-file + /// fallback resolves the exact revision instead of a drifting branch lookup. + /// Empty for legacy field sets, so defaulted. + #[serde(default)] + pub head_ref_oid: String, } /// CI check status from `gh pr checks`. @@ -110,7 +120,8 @@ pub struct CiCheck { /// Classification of a single check's outcome, derived from its bucket/state. /// -/// Kept private: the public surface is [`CiSummary`] via [`summarize`]. +/// Kept private: the public surface is [`CiSummary`] via [`summarize`] and +/// [`rollup_to_summary`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum CheckOutcome { /// Passed, or a non-blocking outcome (neutral, skipped, cancelled). @@ -121,35 +132,54 @@ enum CheckOutcome { Pending, } +/// Classify a single raw check signal into a [`CheckOutcome`]. +/// +/// The signal is a `gh` bucket (`pass`/`fail`/`pending`/…), a raw GitHub check +/// state/conclusion (`SUCCESS`/`FAILURE`/…), or a legacy commit-status state +/// (`ERROR`/`EXPECTED`/…) — the match is case-insensitive and covers all three. +/// Both the `gh pr checks` path ([`CiCheck::outcome`]) and the +/// `statusCheckRollup` path ([`rollup_to_summary`]) route through here so their +/// pass/fail/pending semantics can never drift apart. +/// +/// Neutral, skipped, and cancelled map to [`CheckOutcome::Pass`] (they are not +/// failures); an unknown signal maps conservatively to [`CheckOutcome::Pending`] +/// so it is neither a false pass nor a false failure. +/// +/// CROSS-LANGUAGE MIRROR: `checkOutcome` in `app/src/lib/ci.ts` reproduces these +/// exact arms for the board's client-side CI badge. Any signal added or moved +/// here (e.g. `error` -> fail) must be mirrored there, and vice versa. +fn classify_check_signal(signal: &str) -> CheckOutcome { + match signal.to_ascii_lowercase().as_str() { + // gh buckets. + "pass" | "skipping" | "cancel" => CheckOutcome::Pass, + "fail" => CheckOutcome::Fail, + "pending" => CheckOutcome::Pending, + // Raw GitHub check states / conclusions and commit-status states. + "success" | "neutral" | "skipped" | "cancelled" | "canceled" => CheckOutcome::Pass, + "failure" | "timed_out" | "action_required" | "startup_failure" | "stale" | "error" => { + CheckOutcome::Fail + } + "queued" | "in_progress" | "waiting" | "requested" | "expected" => CheckOutcome::Pending, + // Unknown signal: treat conservatively as pending so it is neither a + // false pass nor a false failure. + _ => CheckOutcome::Pending, + } +} + impl CiCheck { /// Classify this check's outcome from its `bucket` (falling back to `state`). /// /// `gh`'s `bucket` field is the normalized signal; when a fixture or older - /// `gh` omits it, the raw `state` is used. Neutral/skipped/cancelled all map - /// to [`CheckOutcome::Pass`] — they do not represent a failure. + /// `gh` omits it, the raw `state` is used. Delegates to + /// [`classify_check_signal`] so `gh pr checks` and the rollup share one + /// classification. fn outcome(&self) -> CheckOutcome { let signal = if self.bucket.is_empty() { self.state.as_str() } else { self.bucket.as_str() }; - match signal.to_ascii_lowercase().as_str() { - // gh buckets. - "pass" | "skipping" | "cancel" => CheckOutcome::Pass, - "fail" => CheckOutcome::Fail, - "pending" => CheckOutcome::Pending, - // Raw GitHub states (used when bucket is absent). - "success" | "neutral" | "skipped" | "cancelled" | "canceled" => CheckOutcome::Pass, - "failure" | "timed_out" | "action_required" | "startup_failure" | "stale" => { - CheckOutcome::Fail - } - "queued" | "in_progress" | "waiting" | "requested" | "expected" => { - CheckOutcome::Pending - } - // Unknown signal: treat conservatively as pending so it is neither a - // false pass nor a false failure. - _ => CheckOutcome::Pending, - } + classify_check_signal(signal) } } @@ -174,6 +204,101 @@ pub fn summarize(checks: &[CiCheck]) -> CiSummary { summary } +/// Deserialize a possibly-null JSON string as the empty string when null. +/// +/// `gh` emits `"conclusion": null` for an in-flight CheckRun, and plain +/// `#[serde(default)]` only covers an *absent* field, not an explicit `null`. +/// Applied to every string field of [`StatusCheckNode`] so a null anywhere in +/// the heterogeneous rollup coalesces to empty rather than failing the parse. +fn null_as_empty_string<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) +} + +/// A single entry in a PR's `statusCheckRollup` (from `gh pr view --json`). +/// +/// The rollup is a heterogeneous array: GitHub Actions/checks appear as +/// `CheckRun` nodes (carrying `status`/`conclusion`/`name`) while legacy commit +/// statuses appear as `StatusContext` nodes (carrying `state`/`context`). Every +/// field is optional and defaulted because each node only populates the subset +/// belonging to its own type, and a present-but-null value coalesces to empty. +/// Not TS-exported: this is an adapter-internal shape that is rolled up into the +/// domain [`CiSummary`] via [`rollup_to_summary`]. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct StatusCheckNode { + /// GraphQL type discriminator: `"CheckRun"` or `"StatusContext"`. + #[serde( + default, + rename = "__typename", + deserialize_with = "null_as_empty_string" + )] + pub typename: String, + /// CheckRun: the check's name (e.g. `"build"`). Empty for a StatusContext. + #[serde(default, deserialize_with = "null_as_empty_string")] + pub name: String, + /// CheckRun: run status (e.g. `"COMPLETED"`, `"IN_PROGRESS"`). Empty for a StatusContext. + #[serde(default, deserialize_with = "null_as_empty_string")] + pub status: String, + /// CheckRun: conclusion once completed (e.g. `"SUCCESS"`, `"FAILURE"`). + /// Empty while a run is in flight (`gh` reports `null`), and for a StatusContext. + #[serde(default, deserialize_with = "null_as_empty_string")] + pub conclusion: String, + /// StatusContext: the status name (e.g. `"ci/circleci"`). Empty for a CheckRun. + #[serde(default, deserialize_with = "null_as_empty_string")] + pub context: String, + /// StatusContext: raw state (e.g. `"SUCCESS"`, `"FAILURE"`, `"ERROR"`, `"PENDING"`). + /// Empty for a CheckRun. + #[serde(default, deserialize_with = "null_as_empty_string")] + pub state: String, +} + +impl StatusCheckNode { + /// The raw signal fed to [`classify_check_signal`] for this node. + /// + /// A StatusContext carries its outcome in `state`; a CheckRun carries it in + /// `conclusion` once completed, or in `status` while still in flight. `state` + /// is only non-empty for a StatusContext, so it is checked first. + fn signal(&self) -> &str { + if !self.state.is_empty() { + &self.state + } else if !self.conclusion.is_empty() { + &self.conclusion + } else { + &self.status + } + } +} + +/// Roll up a PR's `statusCheckRollup` nodes into a [`CiSummary`]. +/// +/// Returns `None` for an empty slice: a PR with **no** checks is not the same as +/// a PR whose checks are all green, and the CI badge must be able to tell them +/// apart. Classification routes through [`classify_check_signal`] — the same +/// function [`summarize`] uses — so the rollup and `gh pr checks` paths agree on +/// every equivalent state. `passed + failed + pending == total`. +pub fn rollup_to_summary(nodes: &[StatusCheckNode]) -> Option { + if nodes.is_empty() { + return None; + } + let mut summary = CiSummary { + passed: 0, + total: nodes.len() as u32, + failed: 0, + pending: 0, + }; + for node in nodes { + match classify_check_signal(node.signal()) { + CheckOutcome::Pass => summary.passed += 1, + CheckOutcome::Fail => summary.failed += 1, + CheckOutcome::Pending => summary.pending += 1, + } + } + Some(summary) +} + // --------------------------------------------------------------------------- // Core functions // --------------------------------------------------------------------------- @@ -607,6 +732,8 @@ async fn enrich_task(index: usize, sr: SearchPrResult) -> (usize, PrData) { state: sr.state, url: sr.url, repo_slug: slug, + status_check_rollup: Vec::new(), + head_ref_oid: String::new(), }, ), } @@ -704,7 +831,7 @@ async fn enrich_pr(repo_slug: &str, pr_number: u64) -> Result { "--repo", repo_slug, "--json", - "number,headRefName,baseRefName,title,body,state,url", + "number,headRefName,baseRefName,title,body,state,url,statusCheckRollup,headRefOid", ]) .output() .await?; @@ -789,12 +916,20 @@ pub fn build_review_from_pr( body: pr.body.clone(), branch: pr.head_ref_name.clone(), base: pr.base_ref_name.clone(), + // `base_sha` is the restack fork point (see [`Review::base_sha`]), + // computed locally at kickoff by walking git history — it is NOT the base + // branch tip. Pinning it to `baseRefOid` (the base branch's current tip) + // would break restack once the base advances, so imported PRs (which have + // no locally-known fork point) leave it empty; `get_file_pair` then + // resolves the base by branch name (see its doc for the drift limitation). base_sha: String::new(), source, worktree: repo_path.to_path_buf(), gate_state: GateState::Pending, diff: DiffData { raw: diff }, - head_sha: String::new(), + // Pin `head_sha` from `headRefOid` so the diff / full-file view resolve + // the exact PR head revision rather than a drifting branch lookup. + head_sha: pr.head_ref_oid.clone(), comments: vec![], parents: vec![], children: vec![], @@ -803,7 +938,7 @@ pub fn build_review_from_pr( repo_slug: slug, project: None, dispatch_snapshot: None, - ci_summary: None, + ci_summary: rollup_to_summary(&pr.status_check_rollup), review_findings: vec![], conversation: vec![], last_reviewed_sha: None, @@ -1631,6 +1766,50 @@ mod tests { assert_eq!(prs[0].base_ref_name, "main"); assert_eq!(prs[1].number, 43); assert_eq!(prs[1].state, "OPEN"); + + // Legacy JSON omits the rollup + OID fields; they default cleanly. + assert!(prs[0].status_check_rollup.is_empty()); + assert_eq!(prs[0].head_ref_oid, ""); + } + + #[test] + fn deserialize_pr_data_with_rollup_and_oids() { + // The enriched field set: the rollup array plus the pinned head OID. A + // `baseRefOid` in the payload is tolerated (unknown fields are ignored) + // but deliberately not deserialized — base_sha is the local fork point, + // never the base branch tip. + let json = r#"[ + { + "number": 7, + "headRefName": "alejandro/NEX-7-thing", + "baseRefName": "main", + "title": "Thing", + "state": "OPEN", + "url": "https://github.com/owner/repo/pull/7", + "headRefOid": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "baseRefOid": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "statusCheckRollup": [ + {"__typename":"CheckRun","name":"build","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"lint","status":"IN_PROGRESS","conclusion":null}, + {"__typename":"StatusContext","context":"ci/circleci","state":"FAILURE"} + ] + } + ]"#; + + let prs: Vec = serde_json::from_str(json).expect("enriched JSON parses"); + + assert_eq!(prs.len(), 1); + assert_eq!( + prs[0].head_ref_oid, + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + ); + assert_eq!(prs[0].status_check_rollup.len(), 3); + // A CheckRun's completed conclusion, an in-flight null conclusion, and a + // StatusContext state all parse (null coalesces to empty). + assert_eq!(prs[0].status_check_rollup[0].conclusion, "SUCCESS"); + assert_eq!(prs[0].status_check_rollup[1].conclusion, ""); + assert_eq!(prs[0].status_check_rollup[1].status, "IN_PROGRESS"); + assert_eq!(prs[0].status_check_rollup[2].state, "FAILURE"); } // -- link_prs_to_issues -- @@ -1647,6 +1826,8 @@ mod tests { state: "OPEN".into(), url: "https://github.com/o/r/pull/1".into(), repo_slug: String::new(), + status_check_rollup: Vec::new(), + head_ref_oid: String::new(), }, PrData { number: 2, @@ -1657,6 +1838,8 @@ mod tests { state: "OPEN".into(), url: "https://github.com/o/r/pull/2".into(), repo_slug: String::new(), + status_check_rollup: Vec::new(), + head_ref_oid: String::new(), }, PrData { number: 3, @@ -1667,6 +1850,8 @@ mod tests { state: "MERGED".into(), url: "https://github.com/o/r/pull/3".into(), repo_slug: String::new(), + status_check_rollup: Vec::new(), + head_ref_oid: String::new(), }, ]; @@ -1895,6 +2080,122 @@ mod tests { assert_eq!(s.pending, 0); } + // -- rollup_to_summary -- + + /// A mixed rollup: CheckRun (with an in-flight null conclusion) plus legacy + /// StatusContext nodes, covering pass/fail/pending on both node kinds. + const ROLLUP_MIXED: &str = r#"[ + {"__typename":"CheckRun","name":"build","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"FAILURE"}, + {"__typename":"CheckRun","name":"deploy","status":"COMPLETED","conclusion":"CANCELLED"}, + {"__typename":"CheckRun","name":"lint","status":"IN_PROGRESS","conclusion":null}, + {"__typename":"StatusContext","context":"ci/circleci","state":"SUCCESS"}, + {"__typename":"StatusContext","context":"legacy/errored","state":"ERROR"}, + {"__typename":"StatusContext","context":"legacy/queued","state":"PENDING"} + ]"#; + + #[test] + fn rollup_mixed_checkrun_and_statuscontext() { + let nodes: Vec = + serde_json::from_str(ROLLUP_MIXED).expect("mixed rollup parses"); + let s = rollup_to_summary(&nodes).expect("non-empty rollup yields Some"); + assert_eq!(s.total, 7); + // pass: SUCCESS + CANCELLED (CheckRun) + SUCCESS (StatusContext) = 3 + assert_eq!(s.passed, 3, "success + cancelled + statuscontext success"); + // fail: FAILURE (CheckRun) + ERROR (StatusContext) = 2 + assert_eq!(s.failed, 2, "checkrun failure + statuscontext error"); + // pending: in-flight IN_PROGRESS + StatusContext PENDING = 2 + assert_eq!(s.pending, 2, "in_progress + statuscontext pending"); + assert_eq!(s.passed + s.failed + s.pending, s.total); + } + + #[test] + fn rollup_all_success() { + let json = r#"[ + {"__typename":"CheckRun","name":"a","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"b","status":"COMPLETED","conclusion":"SUCCESS"} + ]"#; + let nodes: Vec = serde_json::from_str(json).expect("parses"); + let s = rollup_to_summary(&nodes).expect("Some"); + assert_eq!((s.passed, s.failed, s.pending, s.total), (2, 0, 0, 2)); + } + + #[test] + fn rollup_with_pending() { + let json = r#"[ + {"__typename":"CheckRun","name":"a","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"b","status":"QUEUED","conclusion":null} + ]"#; + let nodes: Vec = serde_json::from_str(json).expect("parses"); + let s = rollup_to_summary(&nodes).expect("Some"); + assert_eq!((s.passed, s.failed, s.pending, s.total), (1, 0, 1, 2)); + } + + #[test] + fn rollup_with_failure() { + let json = r#"[ + {"__typename":"CheckRun","name":"a","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"b","status":"COMPLETED","conclusion":"FAILURE"} + ]"#; + let nodes: Vec = serde_json::from_str(json).expect("parses"); + let s = rollup_to_summary(&nodes).expect("Some"); + assert_eq!((s.passed, s.failed, s.pending, s.total), (1, 1, 0, 2)); + } + + #[test] + fn rollup_empty_is_none() { + // No checks is NOT the same as all-green: the badge must distinguish them. + assert_eq!(rollup_to_summary(&[]), None); + } + + #[test] + fn rollup_classification_matches_summarize() { + // Every state a CiCheck (`gh pr checks`) and a StatusCheckNode (rollup) + // can both carry must classify identically. Both paths route through + // `classify_check_signal`; this test fails loudly if they ever diverge. + let states = [ + "SUCCESS", + "NEUTRAL", + "SKIPPED", + "CANCELLED", + "FAILURE", + "TIMED_OUT", + "ACTION_REQUIRED", + "ERROR", + "STALE", + "PENDING", + "IN_PROGRESS", + "QUEUED", + "EXPECTED", + "WAITING", + ]; + for state in states { + let check = CiCheck { + name: "c".into(), + state: state.into(), + bucket: String::new(), + link: String::new(), + workflow: String::new(), + }; + let node = StatusCheckNode { + state: state.into(), + ..StatusCheckNode::default() + }; + let via_summarize = summarize(std::slice::from_ref(&check)); + let via_rollup = + rollup_to_summary(std::slice::from_ref(&node)).expect("non-empty yields Some"); + assert_eq!( + ( + via_summarize.passed, + via_summarize.failed, + via_summarize.pending + ), + (via_rollup.passed, via_rollup.failed, via_rollup.pending), + "classification drift for state {state}" + ); + } + } + #[test] fn run_id_extraction() { assert_eq!( @@ -2192,6 +2493,77 @@ index 1111111..2222222 100644 assert!(reviews[1].children.is_empty()); } + // -- build_review_from_pr -- + + #[test] + fn build_review_pins_head_sha_and_ci_summary() { + let pr = PrData { + number: 7, + head_ref_name: "alejandro/NEX-7-thing".into(), + base_ref_name: "main".into(), + title: "Thing".into(), + body: "desc".into(), + state: "OPEN".into(), + url: "https://github.com/o/r/pull/7".into(), + repo_slug: "o/r".into(), + status_check_rollup: serde_json::from_str( + r#"[ + {"__typename":"CheckRun","name":"build","status":"COMPLETED","conclusion":"SUCCESS"}, + {"__typename":"CheckRun","name":"test","status":"COMPLETED","conclusion":"FAILURE"} + ]"#, + ) + .expect("rollup parses"), + head_ref_oid: "aaa111".into(), + }; + + let review = build_review_from_pr( + &pr, + "diff".to_string(), + std::path::Path::new("/tmp/wt"), + ReviewSource::Authored, + ); + + // head_sha is pinned from headRefOid so the diff resolves the exact PR + // head. base_sha is the restack fork point (computed locally at kickoff), + // NOT the base branch tip, so an import leaves it empty — pinning it to + // baseRefOid would break restack once the base advances. This assertion + // guards that invariant against a re-pinning regression. + assert_eq!(review.head_sha, "aaa111"); + assert_eq!(review.base_sha, ""); + // The rollup is summarized into ci_summary (1 pass, 1 fail). + let ci = review.ci_summary.expect("rollup yields a summary"); + assert_eq!((ci.passed, ci.failed, ci.pending, ci.total), (1, 1, 0, 2)); + assert_eq!(review.issue, IssueRef::new("NEX-7")); + } + + #[test] + fn build_review_without_rollup_has_no_ci_summary() { + let pr = PrData { + number: 8, + head_ref_name: "feature/no-id".into(), + base_ref_name: "main".into(), + title: "No checks".into(), + body: String::new(), + state: "OPEN".into(), + url: "https://github.com/o/r/pull/8".into(), + repo_slug: String::new(), + status_check_rollup: Vec::new(), + head_ref_oid: String::new(), + }; + + let review = build_review_from_pr( + &pr, + String::new(), + std::path::Path::new("/tmp/wt"), + ReviewSource::Authored, + ); + + // No checks -> no summary (distinct from an all-green summary). + assert_eq!(review.ci_summary, None); + assert_eq!(review.head_sha, ""); + assert_eq!(review.base_sha, ""); + } + // -- decode_base64_content -- #[test]