diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 47c6306..c7e7767 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -70,7 +70,7 @@ server/ # webhook server (GitHub App) - **incremental processing**: only re-embeds new/changed items. crash-recoverable via sqlite - **read-only default**: every GitHub mutation (labels, comments, closes, issue creation) funnels through one `write-gate.ts` gate that defaults to dry-run. CLI writes only under `--apply-labels`; the webhook server writes only when `PRISM_APPLY=1`. `--dry-run` always wins. (Fixes the prior leak where `ensureLabelsExist` created labels even under `--dry-run`, and the server writing unconditionally.) - **cross-repo**: config accepts multiple repos, dupe detection works across repo boundaries -- **canonical selection**: one `selectCanonical()` (src/canonical.ts) picks each cluster's source of truth for the report, the starmap payload, and the live triage bot alike. issue-majority clusters resolve to the earliest report (the original bug); PR-majority to the highest-quality item. fully deterministic - every tie bottoms out at item number - so re-runs name the same canonical +- **canonical selection**: one `selectCanonical()` (src/canonical.ts) picks each cluster's source of truth for the report, the starmap payload, and the live triage bot alike. issue-majority clusters resolve to the earliest report (the original bug); PR-majority ranks by lifecycle state (merged > open > closed), then a CI veto (a known-red build never outranks a same-state green sibling, before score), then quality score. the veto stops a high-scored PR with failing checks from becoming bestPick over the green fix that actually landed; only `ciStatus === "failure"` demotes, so a not-yet-reported PR is never penalized. fully deterministic - every tie bottoms out at item number - so re-runs name the same canonical - **cluster confidence**: clustering is single-linkage (BFS over pairs >= threshold) with a centroid-refinement pass to break chained mega-clusters. because single-linkage can still chain in loosely-related members, each cluster reports both `avgSimilarity` and `minSimilarity` (lowest pairwise). the report/dupes output surfaces min as a confidence tier (high >= 90%, solid >= 80%, loose < 80%) so a low-min "loose" cluster gets eyeballed before anything is closed. avg and min are computed exactly over all pairs (no sampling), so the tier a maintainer sees is reproducible run to run ## database diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b87134..e777a80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ all notable changes to pr-prism are documented here. +## [unreleased] + +### changed +- canonical/bestPick selection now vetoes a red build: a PR with failing CI never outranks a same-state sibling with a non-failing build, regardless of quality score. stops a high-scored PR (e.g. one that added a test file) from being named bestPick over the green fix that actually landed, when the added test fails. only `ciStatus === "failure"` demotes, so a PR whose checks have not reported yet is never penalized; state (merged > open > closed) still dominates. starmap `canonical`/`contested` for such clusters shift accordingly (schema unchanged, still v1) + ## [3.0.1] — 2026-07-13 ### fixed diff --git a/src/__tests__/canonical.test.ts b/src/__tests__/canonical.test.ts index a071897..d275ee8 100644 --- a/src/__tests__/canonical.test.ts +++ b/src/__tests__/canonical.test.ts @@ -220,6 +220,82 @@ describe("selectCanonical state preference (merged PRs are the source of truth)" }); }); +describe("selectCanonical CI veto (a red build never becomes bestPick over a green sibling)", () => { + it("the odysseus #5207 case: a failing higher-scored PR loses to a passing lower-scored sibling", () => { + // Both open at scan time. #5358 scored higher (a test file it added inflated + // its quality score) but its CI is red; #5208 is the minimal green fix that + // was actually merged. Without the veto, score picks #5358 (the wrong one). + const items = [ + c({ number: 5358, type: "pr", state: "open", score: 0.752, ciStatus: "failure" }), + c({ number: 5208, type: "pr", state: "open", score: 0.532, ciStatus: "success" }), + ]; + expect(selectCanonical(items).number).toBe(5208); + }); + + it("a red build loses to a green sibling no matter how large its score lead", () => { + const items = [ + c({ number: 1, type: "pr", state: "open", score: 0.99, ciStatus: "failure" }), + c({ number: 2, type: "pr", state: "open", score: 0.01, ciStatus: "success" }), + ]; + expect(selectCanonical(items).number).toBe(2); + }); + + it("only failure demotes: pending/unknown/absent CI are never penalized, score still decides", () => { + const pending = [ + c({ number: 1, type: "pr", state: "open", score: 0.9, ciStatus: "pending" }), + c({ number: 2, type: "pr", state: "open", score: 0.5, ciStatus: "success" }), + ]; + expect(selectCanonical(pending).number).toBe(1); // pending not demoted -> higher score wins + const unknownVsAbsent = [ + c({ number: 3, type: "pr", state: "open", score: 0.9, ciStatus: "unknown" }), + c({ number: 4, type: "pr", state: "open", score: 0.5 }), + ]; + expect(selectCanonical(unknownVsAbsent).number).toBe(3); // neither demoted -> higher score wins + }); + + it("when every sibling is failing, the CI gate ties and score decides (no bottomless demotion)", () => { + const items = [ + c({ number: 1, type: "pr", state: "open", score: 0.4, ciStatus: "failure" }), + c({ number: 2, type: "pr", state: "open", score: 0.9, ciStatus: "failure" }), + ]; + expect(selectCanonical(items).number).toBe(2); // both red -> highest score + }); + + it("state still dominates CI: a merged PR wins even if its build is red", () => { + const items = [ + c({ number: 1, type: "pr", state: "open", score: 0.9, ciStatus: "success" }), + c({ number: 2, type: "pr", state: "merged", score: 0.4, ciStatus: "failure" }), + ]; + expect(selectCanonical(items).number).toBe(2); // merged is in main = source of truth + }); + + it("the CI gate applies within a state: a green closed PR beats a red closed PR", () => { + const items = [ + c({ number: 1, type: "pr", state: "closed", score: 0.9, ciStatus: "failure" }), + c({ number: 2, type: "pr", state: "closed", score: 0.4, ciStatus: "success" }), + ]; + expect(selectCanonical(items).number).toBe(2); + }); + + it("a pick decided by the CI gate is NOT contested, even when scores are within the tie margin", () => { + const items = [ + c({ number: 1, type: "pr", state: "open", score: 0.52, ciStatus: "failure" }), + c({ number: 2, type: "pr", state: "open", score: 0.5, ciStatus: "success" }), + ]; + const d = decideCanonical(items); + expect(d.canonical.number).toBe(2); // green wins despite lower score + expect(d.contested).toBe(false); // CI decided it, not a coin flip + }); + + it("issue-majority clusters ignore CI (earliest report stays canonical)", () => { + const items = [ + c({ number: 1, type: "issue", createdAt: "2026-01-01T00:00:00Z", score: 0.1, ciStatus: "failure" }), + c({ number: 2, type: "issue", createdAt: "2026-02-01T00:00:00Z", score: 0.9, ciStatus: "success" }), + ]; + expect(selectCanonical(items).number).toBe(1); // earliest report, CI is a PR-mode concept + }); +}); + describe("selectTracker (tracker substrate: original bug + role-tagged candidates)", () => { it("mixed cluster: tracker = earliest issue, PRs are fixes, later issue is a duplicate", () => { const items = [ diff --git a/src/canonical.ts b/src/canonical.ts index f96253f..c60b0a1 100644 --- a/src/canonical.ts +++ b/src/canonical.ts @@ -7,8 +7,11 @@ // Semantics (the cli.ts rule, now shared): // - issue-majority cluster -> the earliest report is canonical (the original // bug), quality score then item number as tiebreaks. -// - PR-majority cluster -> the highest-quality item is canonical, then -// earliest, then most-reviewed, then item number. +// - PR-majority cluster -> ranked in order: lifecycle state (merged > +// open > closed), then a CI veto (a known-red build never outranks a +// same-state non-failing sibling), then quality score, then earliest, then +// most-reviewed, then item number. state and the CI veto both sit BEFORE +// score so neither a stale open PR nor a failing one wins on score alone. // `mode` lets a caller pin the rule instead of deriving it from the set — the // triage bot passes the incoming item's type so its behavior is unchanged. @@ -23,6 +26,8 @@ export interface CanonicalCandidate { number: number; /** "open" | "closed" | "merged"; absent for callers with no state (triage). */ state?: string; + /** CI rollup; absent for callers with no CI signal (triage). Only "failure" demotes. */ + ciStatus?: "success" | "failure" | "pending" | "unknown"; } function createdMs(item: CanonicalCandidate): number { @@ -63,11 +68,25 @@ function statePriority(state: string | undefined): number { } } +/** + * Rank a PR by CI outcome: only a KNOWN-red build demotes. A failing PR must not + * become bestPick over a same-state green sibling however high its quality score + * (the score rewards e.g. having a test file, blind to whether that test passes). + * success / pending / unknown / absent all rank equal (1) so a PR whose checks + * simply have not reported yet is never penalized, and an all-failing set ties + * here and falls through to score unchanged. + */ +function ciRank(status: string | undefined): number { + return status === "failure" ? 0 : 1; +} + function isNearTie(mode: "issue" | "pr", canonical: T, runnerUp: T): boolean { if (mode === "pr") { // A different lifecycle state decided the pick (e.g. merged over open) -> a // clear winner, not a coin flip. Only a same-state pair is a score near-tie. if (statePriority(canonical.state) !== statePriority(runnerUp.state)) return false; + // A green build chosen over a red sibling is a decisive reason, not a coin flip. + if (ciRank(canonical.ciStatus) !== ciRank(runnerUp.ciStatus)) return false; return canonical.score - runnerUp.score < TIE_MARGIN; } // The pick is on createdAt, so nearness is a time window (a huge score gap @@ -109,6 +128,10 @@ export function decideCanonical( // BEFORE score - a merged fix often does not have the top computed score. const sp = statePriority(b.state) - statePriority(a.state); if (sp !== 0) return sp; + // Then veto a known-red build: a same-state green sibling always outranks it, + // BEFORE score, so a high quality score cannot promote a failing PR. + const cr = ciRank(b.ciStatus) - ciRank(a.ciStatus); + if (cr !== 0) return cr; if (b.score !== a.score) return b.score - a.score; // then highest score first const d = createdMs(a) - createdMs(b); if (d !== 0) return d;