From 7dc48df80aab0f2d22ffc67ed0b48e7362d7a3e2 Mon Sep 17 00:00:00 2001 From: StressTestor <212606152+StressTestor@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:47:20 -0600 Subject: [PATCH 1/5] feat(rank): treat incident-closed PRs as open, not rejected A repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. selectCanonical() ranks lifecycle state before score, so those items ranked as rejections and sank below genuinely-closed siblings -- exactly inverting the triage order for the set a maintainer most needs to review. prism.config.yaml now accepts an `incidents:` list of {start, end, reason} windows. closedAt is captured from both the GraphQL and REST paths, stored in metadata_json (no schema migration), and store.ts stamps `incidentClosed` onto each item at hydration. statePriority() ranks an incident-closed PR as open. Only the raw closedAt is persisted; incidentClosed is derived at read time, so correcting a mis-set window is a config edit rather than a rescan of the whole backlog. The starmap payload carries `incidentClosed: true` (omitted when false, keeping the consumer contract additive) so a consumer can bucket these for re-triage instead of treating them as rejected. --- ARCHITECTURE.md | 6 ++- src/__tests__/canonical.test.ts | 17 +++++++++ src/__tests__/github.test.ts | 16 +++++++- src/__tests__/incident.test.ts | 65 +++++++++++++++++++++++++++++++++ src/__tests__/starmap.test.ts | 15 ++++++++ src/__tests__/store.test.ts | 57 +++++++++++++++++++++++++++++ src/canonical.ts | 13 +++++-- src/config.ts | 11 ++++++ src/github.ts | 17 +++++++++ src/incident.ts | 34 +++++++++++++++++ src/pipeline.ts | 5 ++- src/starmap.ts | 7 ++++ src/store.ts | 14 ++++++- src/types.ts | 6 +++ 14 files changed, 274 insertions(+), 9 deletions(-) create mode 100644 src/__tests__/incident.test.ts create mode 100644 src/incident.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4a2ab0d..3b588af 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -73,6 +73,8 @@ server/ # webhook server (GitHub App) - **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 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. confirmed (identity) clusters override to the earliest-created rule: byte-identical dupes resolve by which-was-first, never score (a copy can outscore its original) +- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag - so correcting a mis-set window is a config edit, not a rescan of the whole backlog. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected + - **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 @@ -81,7 +83,7 @@ single SQLite database per project, managed by `store.ts`. | table | purpose | |-------|---------| -| items | PRs and issues with metadata | +| items | PRs and issues with metadata (incl. `closedAt` in `metadata_json`; no migration needed) | | embeddings | vector embeddings (sqlite-vec) | | prism_meta | config versioning, model mismatch detection | @@ -105,4 +107,4 @@ npm run server # start webhook server - the npm package excludes compiled tests (`tsconfig.build.json`); `npm run build` cleans `dist/` first so stale artifacts can't leak into a publish (the 2.0.1 orphan-file gotcha) - brew tap bump is manual: update `Formula/prism-triage.rb` (tarball url + sha256) in StressTestor/homebrew-tap after the GitHub Release exists -last updated: 2026-07-13 +last updated: 2026-07-28 diff --git a/src/__tests__/canonical.test.ts b/src/__tests__/canonical.test.ts index d275ee8..f35d3c9 100644 --- a/src/__tests__/canonical.test.ts +++ b/src/__tests__/canonical.test.ts @@ -359,3 +359,20 @@ describe("selectTracker (tracker substrate: original bug + role-tagged candidate expect(items.map((i) => i.number)).toEqual(before); }); }); + +describe("incident-closed lifecycle ranking", () => { + it("ranks an incident-closed PR as open, so a bulk auto-close cannot bury it under a deliberately closed sibling", () => { + const items = [ + c({ number: 5559, state: "closed", incidentClosed: true, score: 0.1 }), + c({ number: 5560, state: "closed", score: 0.99 }), + ]; + + expect(selectCanonical(items).number).toBe(5559); + }); + + it("leaves a deliberately closed PR ranked below an open one", () => { + const items = [c({ number: 30, state: "closed", score: 0.99 }), c({ number: 31, state: "open", score: 0.1 })]; + + expect(selectCanonical(items).number).toBe(31); + }); +}); diff --git a/src/__tests__/github.test.ts b/src/__tests__/github.test.ts index 92591c3..247e0da 100644 --- a/src/__tests__/github.test.ts +++ b/src/__tests__/github.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { accountIsBot, GitHubClient, restPRState } from "../github.js"; +import { accountIsBot, GitHubClient, itemClosedAt, restPRState } from "../github.js"; import { createWriteGate } from "../write-gate.js"; function fakeOctokit() { @@ -219,3 +219,17 @@ describe("accountIsBot", () => { expect(accountIsBot({})).toBeUndefined(); }); }); + +describe("itemClosedAt", () => { + it("reads the REST closed_at field", () => { + expect(itemClosedAt({ closed_at: "2026-07-23T10:18:00Z" })).toBe("2026-07-23T10:18:00Z"); + }); + + it("reads the GraphQL closedAt field", () => { + expect(itemClosedAt({ closedAt: "2026-07-23T10:18:00Z" })).toBe("2026-07-23T10:18:00Z"); + }); + + it("returns undefined for an item that is still open", () => { + expect(itemClosedAt({ closed_at: null })).toBeUndefined(); + }); +}); diff --git a/src/__tests__/incident.test.ts b/src/__tests__/incident.test.ts new file mode 100644 index 0000000..6188682 --- /dev/null +++ b/src/__tests__/incident.test.ts @@ -0,0 +1,65 @@ +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { loadConfig } from "../config.js"; +import { isIncidentClosed } from "../incident.js"; +import { itemMetadata } from "../pipeline.js"; + +const INCIDENT = { + start: "2026-07-23T00:00:00Z", + end: "2026-07-24T00:00:00Z", + reason: "repository visibility flip auto-closed all open PRs", +}; + +describe("isIncidentClosed", () => { + it("classifies a PR closed inside an incident window as incident-closed", () => { + const item = { state: "closed", closedAt: "2026-07-23T10:18:00Z" }; + + expect(isIncidentClosed(item, [INCIDENT])).toBe(true); + }); +}); + +describe("incident windows in config", () => { + it("parses an incidents block into typed windows", () => { + const dir = mkdtempSync(join(tmpdir(), "prism-incident-")); + const path = join(dir, "prism.config.yaml"); + writeFileSync( + path, + [ + "repo: odysseus-dev/odysseus", + "incidents:", + ' - start: "2026-07-23T00:00:00Z"', + ' end: "2026-07-24T00:00:00Z"', + ' reason: "repository visibility flip auto-closed all open PRs"', + ].join("\n"), + ); + + try { + const config = loadConfig(path); + expect(config.incidents).toEqual([INCIDENT]); + } finally { + rmSync(dir, { recursive: true }); + } + }); +}); + +describe("itemMetadata", () => { + it("persists closedAt so incident windows can be re-evaluated without a rescan", () => { + const item = { + number: 5559, + type: "pr" as const, + repo: "odysseus-dev/odysseus", + title: "fix(rag): skip hidden dirs", + body: "", + state: "closed", + author: "stresstestor", + createdAt: "2026-07-20T00:00:00Z", + updatedAt: "2026-07-23T10:18:00Z", + closedAt: "2026-07-23T10:18:00Z", + labels: [], + }; + + expect(itemMetadata(item).closedAt).toBe("2026-07-23T10:18:00Z"); + }); +}); diff --git a/src/__tests__/starmap.test.ts b/src/__tests__/starmap.test.ts index d4afe12..726fcb1 100644 --- a/src/__tests__/starmap.test.ts +++ b/src/__tests__/starmap.test.ts @@ -302,3 +302,18 @@ describe("confirmed clusters in the starmap payload", () => { expect(buildStarmapPayload(clusters, META).clusters[0].canonical.number).toBe(10); }); }); + +describe("incident-closed in the starmap payload", () => { + it("flags an incident-closed item so a consumer can bucket it apart from a rejection", () => { + const items = [ + item(5559, "pr", 0.9, { state: "closed", incidentClosed: true }), + item(5560, "pr", 0.8, { state: "closed" }), + ]; + + const payload = buildStarmapPayload([cluster(1, items, 0.93, 0.91)], META); + const refs = payload.clusters[0].items; + + expect(refs.find((r) => r.number === 5559)?.incidentClosed).toBe(true); + expect(refs.find((r) => r.number === 5560)?.incidentClosed).toBeUndefined(); + }); +}); diff --git a/src/__tests__/store.test.ts b/src/__tests__/store.test.ts index 0ec8304..31691ea 100644 --- a/src/__tests__/store.test.ts +++ b/src/__tests__/store.test.ts @@ -313,6 +313,63 @@ describe("closesIssues metadata round-trip", () => { expect(store.getAllItems("owner/repo")).toHaveLength(1); store.close(); }); + + it("hydrates closedAt so incident windows can be applied without a rescan", () => { + const path = tmpDb(); + dbs.push(path); + const store = new VectorStore(path, 4); + store.upsert({ + id: "odysseus-dev/odysseus#5559", + type: "pr", + number: 5559, + repo: "odysseus-dev/odysseus", + title: "fix(rag): skip hidden dirs", + bodySnippet: "", + embedding: new Float32Array(4), + metadata: { state: "closed", closedAt: "2026-07-23T10:18:00Z" }, + createdAt: "2026-07-20T00:00:00Z", + updatedAt: "2026-07-23T10:18:00Z", + }); + + const [item] = store.getAllItems("odysseus-dev/odysseus") as any[]; + + expect(item.closedAt).toBe("2026-07-23T10:18:00Z"); + }); + + it("marks items closed inside a configured incident window", () => { + const path = tmpDb(); + dbs.push(path); + const store = new VectorStore(path, 4, undefined, [ + { start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }, + ]); + const base = { + type: "pr" as const, + repo: "odysseus-dev/odysseus", + title: "t", + bodySnippet: "", + embedding: new Float32Array(4), + createdAt: "2026-07-20T00:00:00Z", + updatedAt: "2026-07-23T10:18:00Z", + }; + store.upsert({ + ...base, + id: "a", + number: 1, + metadata: { state: "closed", closedAt: "2026-07-23T10:18:00Z" }, + }); + store.upsert({ + ...base, + id: "b", + number: 2, + metadata: { state: "closed", closedAt: "2026-07-01T00:00:00Z" }, + }); + + const items = store.getAllItems("odysseus-dev/odysseus") as any[]; + const byNumber = new Map(items.map((i) => [i.number, i.incidentClosed])); + + expect(byNumber.get(1)).toBe(true); + expect(byNumber.get(2)).toBe(false); + }); }); describe("bot authorship round-trip", () => { diff --git a/src/canonical.ts b/src/canonical.ts index c60b0a1..ad05b3a 100644 --- a/src/canonical.ts +++ b/src/canonical.ts @@ -26,6 +26,10 @@ export interface CanonicalCandidate { number: number; /** "open" | "closed" | "merged"; absent for callers with no state (triage). */ state?: string; + /** True when this item was closed by a repository-wide incident rather than + * by a maintainer decision. Ranked as open: a bulk auto-close says nothing + * about the item's quality. See src/incident.ts. */ + incidentClosed?: boolean; /** CI rollup; absent for callers with no CI signal (triage). Only "failure" demotes. */ ciStatus?: "success" | "failure" | "pending" | "unknown"; } @@ -55,7 +59,10 @@ export interface CanonicalDecision { * (a triage DupeMatch has none) is 0, below every real state, so a stateless set * all ties here and falls through to the score/date rules unchanged. */ -function statePriority(state: string | undefined): number { +function statePriority(candidate: { state?: string; incidentClosed?: boolean }): number { + // An incident-closed item never reached a maintainer verdict, so it is ranked + // where it sat before the incident rather than as a rejection. + const state = candidate.state === "closed" && candidate.incidentClosed ? "open" : candidate.state; switch (state) { case "merged": return 3; @@ -84,7 +91,7 @@ function isNearTie(mode: "issue" | "pr", canonical 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; + if (statePriority(canonical) !== statePriority(runnerUp)) 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; @@ -126,7 +133,7 @@ export function decideCanonical( } else { // Prefer a merged PR (the real source of truth) over open over closed, // BEFORE score - a merged fix often does not have the top computed score. - const sp = statePriority(b.state) - statePriority(a.state); + const sp = statePriority(b) - statePriority(a); 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. diff --git a/src/config.ts b/src/config.ts index 9de8f74..cf5a252 100644 --- a/src/config.ts +++ b/src/config.ts @@ -51,10 +51,21 @@ const ClusterSchema = z.object({ bot_authors: z.array(z.string().trim().min(1)).default([]), }); +/** A repository-wide event that closed items for reasons unrelated to their + * quality (visibility flip, bulk close, migration). See src/incident.ts. */ +const IncidentWindowSchema = z.object({ + /** ISO-8601, inclusive. */ + start: z.string(), + /** ISO-8601, exclusive. */ + end: z.string(), + reason: z.string(), +}); + const ConfigSchema = z.object({ version: z.number().optional().default(1), repo: z.string().optional(), repos: z.array(z.string()).optional(), + incidents: z.array(IncidentWindowSchema).optional(), vision_doc: z.string().optional(), vision_docs: z.record(z.string(), z.string()).optional(), thresholds: ThresholdsSchema.optional().transform((v) => ThresholdsSchema.parse(v ?? {})), diff --git a/src/github.ts b/src/github.ts index 273917c..7223f75 100644 --- a/src/github.ts +++ b/src/github.ts @@ -25,6 +25,16 @@ export function restPRState(pr: { state: string; merged_at?: string | null }): s return pr.merged_at ? "merged" : pr.state; } +/** + * Normalize a close timestamp across both API shapes. REST returns snake_case + * `closed_at`, GraphQL returns `closedAt`; an item still open has neither. + * Needed so incident windows can tell a bulk auto-close apart from a + * maintainer's deliberate close (see src/incident.ts). + */ +export function itemClosedAt(raw: { closed_at?: string | null; closedAt?: string | null }): string | undefined { + return raw.closed_at ?? raw.closedAt ?? undefined; +} + /** GraphQL closingIssuesReferences -> same-repo issue numbers. Cross-repo closing * refs are dropped: keeping only the bare number would alias another repo's issue * onto this repo's numbering and fabricate closing edges. A fetched PR with no @@ -195,6 +205,7 @@ export class GitHubClient { author { login __typename } createdAt updatedAt + closedAt additions deletions changedFiles @@ -261,6 +272,7 @@ export class GitHubClient { author: pr.author?.login || "unknown", authorIsBot: accountIsBot(pr.author), createdAt: pr.createdAt, + closedAt: itemClosedAt(pr), nodeId: pr.id, headRefOid: pr.headRefOid, updatedAt: pr.updatedAt, @@ -316,6 +328,7 @@ export class GitHubClient { author { login __typename } createdAt updatedAt + closedAt labels(first: 20) { nodes { name } } } } @@ -354,6 +367,7 @@ export class GitHubClient { author: issue.author?.login || "unknown", authorIsBot: accountIsBot(issue.author), createdAt: issue.createdAt, + closedAt: itemClosedAt(issue), nodeId: issue.id, updatedAt: issue.updatedAt, labels: (issue.labels?.nodes || []).map((l: any) => l.name), @@ -430,6 +444,7 @@ export class GitHubClient { author: pr.user?.login || "unknown", authorIsBot: accountIsBot(pr.user), createdAt: pr.created_at, + closedAt: itemClosedAt(pr), nodeId: pr.node_id, headRefOid: pr.head?.sha, updatedAt: pr.updated_at, @@ -488,6 +503,7 @@ export class GitHubClient { author: issue.user?.login || "unknown", authorIsBot: accountIsBot(issue.user), createdAt: issue.created_at, + closedAt: itemClosedAt(issue), nodeId: issue.node_id, updatedAt: issue.updated_at, labels: issue.labels.map((l) => (typeof l === "string" ? l : l.name || "")), @@ -523,6 +539,7 @@ export class GitHubClient { author: pr.user?.login || "unknown", authorIsBot: accountIsBot(pr.user), createdAt: pr.created_at, + closedAt: itemClosedAt(pr), updatedAt: pr.updated_at, labels: pr.labels.map((l) => (typeof l === "string" ? l : l.name || "")), diffUrl: pr.diff_url, diff --git a/src/incident.ts b/src/incident.ts new file mode 100644 index 0000000..f136a0c --- /dev/null +++ b/src/incident.ts @@ -0,0 +1,34 @@ +/** + * Incident awareness. + * + * A repository-wide event (a visibility flip, a bulk close, a migration) can + * close large numbers of pull requests for reasons unrelated to their quality. + * Lifecycle state is a ranking signal here, so without this the affected items + * are indistinguishable from ones a maintainer closed deliberately, and they + * rank as though they had been rejected. + */ + +export interface IncidentWindow { + /** ISO-8601 timestamp; inclusive. */ + start: string; + /** ISO-8601 timestamp; exclusive. */ + end: string; + reason: string; +} + +export interface IncidentClosable { + state: string; + closedAt?: string | null; +} + +export function isIncidentClosed(item: IncidentClosable, windows: readonly IncidentWindow[]): boolean { + if (item.state !== "closed" || !item.closedAt) return false; + const closed = Date.parse(item.closedAt); + if (Number.isNaN(closed)) return false; + return windows.some((w) => { + const start = Date.parse(w.start); + const end = Date.parse(w.end); + if (Number.isNaN(start) || Number.isNaN(end)) return false; + return closed >= start && closed < end; + }); +} diff --git a/src/pipeline.ts b/src/pipeline.ts index a95113c..21bb6e1 100644 --- a/src/pipeline.ts +++ b/src/pipeline.ts @@ -65,7 +65,7 @@ export async function createPipelineContext(repoOverride?: string): Promise { +export function itemMetadata(item: PRItem): Record { return { author: item.author, authorIsBot: item.authorIsBot, state: item.state, + closedAt: item.closedAt, labels: item.labels, additions: item.additions, deletions: item.deletions, diff --git a/src/starmap.ts b/src/starmap.ts index 315b76e..966edca 100644 --- a/src/starmap.ts +++ b/src/starmap.ts @@ -25,6 +25,10 @@ export interface StarmapItemRef { nodeId?: string; /** "open" | "closed" | "merged"; lets a consumer see an open dup superseded by a merged PR. */ state?: "open" | "closed" | "merged"; + /** Present only when true: this item was closed by a repository-wide incident + * (a visibility flip, a bulk close), not by a maintainer verdict. Consumers + * should bucket these for re-triage rather than treating them as rejected. */ + incidentClosed?: true; } export interface StarmapItem extends StarmapItemRef { @@ -121,6 +125,7 @@ function ref(item: { type: "pr" | "issue"; nodeId?: string; state?: string; + incidentClosed?: boolean; }): StarmapItemRef { const r: StarmapItemRef = { repo: item.repo, @@ -132,6 +137,8 @@ function ref(item: { if (item.state === "open" || item.state === "closed" || item.state === "merged") { r.state = item.state; } + // Omitted when false so the payload stays additive for existing consumers. + if (item.incidentClosed) r.incidentClosed = true; return r; } diff --git a/src/store.ts b/src/store.ts index 7d6224d..912a55b 100644 --- a/src/store.ts +++ b/src/store.ts @@ -2,6 +2,7 @@ import { mkdirSync } from "node:fs"; import { resolve } from "node:path"; import Database from "better-sqlite3"; import * as sqliteVec from "sqlite-vec"; +import { type IncidentWindow, isIncidentClosed } from "./incident.js"; import type { StoreItem } from "./types.js"; export class VectorStore { @@ -9,7 +10,14 @@ export class VectorStore { private dimensions: number; private embeddingModel?: string; - constructor(dbPath?: string, dimensions?: number, embeddingModel?: string) { + constructor( + dbPath?: string, + dimensions?: number, + embeddingModel?: string, + /** Repository-wide events that closed items for non-quality reasons. + * Applied at read time so a corrected window needs no rescan. */ + private incidentWindows: readonly IncidentWindow[] = [], + ) { const p = dbPath || resolve(process.cwd(), "data", "prism.db"); mkdirSync(resolve(p, ".."), { recursive: true }); this.db = new Database(p); @@ -265,6 +273,8 @@ export class VectorStore { author: metadata.author, authorIsBot: metadata.authorIsBot, state: metadata.state, + closedAt: metadata.closedAt, + incidentClosed: isIncidentClosed({ state: metadata.state, closedAt: metadata.closedAt }, this.incidentWindows), labels: metadata.labels, additions: metadata.additions, deletions: metadata.deletions, @@ -330,6 +340,8 @@ export class VectorStore { author: metadata.author, authorIsBot: metadata.authorIsBot, state: metadata.state, + closedAt: metadata.closedAt, + incidentClosed: isIncidentClosed({ state: metadata.state, closedAt: metadata.closedAt }, this.incidentWindows), labels: metadata.labels, additions: metadata.additions, deletions: metadata.deletions, diff --git a/src/types.ts b/src/types.ts index dadf934..1f4f281 100644 --- a/src/types.ts +++ b/src/types.ts @@ -13,6 +13,12 @@ export interface PRItem { authorIsBot?: boolean; createdAt: string; updatedAt: string; + /** ISO-8601 close timestamp; absent while open. Retained so incident + * windows can distinguish a bulk auto-close from a deliberate one. */ + closedAt?: string; + /** Derived at read time from the configured incident windows, never stored: + * correcting a window is then a config edit rather than a rescan. */ + incidentClosed?: boolean; labels: string[]; // PR-specific diffUrl?: string; From 6a98bd457b1ada926a6bf711b196956cd531932f Mon Sep 17 00:00:00 2001 From: StressTestor <212606152+StressTestor@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:15:17 -0600 Subject: [PATCH 2/5] fix(incident): require absolute window bounds, fail loudly on bad config Review found two real bugs in the first cut. Timezone: window bounds were bare strings fed to Date.parse, so an offset-less ISO literal resolved in the HOST timezone. The same config selected a different set of PRs under TZ=America/Denver than under UTC in CI, and "2026-07-23" parsed as UTC while "2026-07-23T00:00:00" parsed local -- two spellings of one day disagreeing. Bounds now require an explicit offset and are rejected at load. Verified across UTC, America/Denver and Asia/Kolkata: every accepted form yields a byte-identical instant. Silent misconfiguration: unparseable ("last tuesday"), inverted, and blank windows were accepted and then matched nothing, forever, with no diagnostic. A single typo made the whole feature a no-op. Now fails at load. Scope: incident-aware ranking is CLI-only. An earlier version of this commit claimed to wire the server/GitHub-App path; it did not. openRepoDB gained a parameter no caller could supply -- ServerConfig has no `incidents` field -- and the scheduler fetches open items only, so closedAt is never populated there. The whole server change was removable with the suite still green. That parameter is gone and the limitation is stated in server/db.ts, ARCHITECTURE.md and CHANGELOG. Wiring the App path needs a config field, a scheduler that fetches closed items, and server/triage.ts's hand-rolled metadata literal replaced; that is its own change. Kept from that attempt: server/scheduler.ts now builds metadata via itemMetadata() instead of a hand-rolled literal that had drifted from it, silently dropping bodyLength, nodeId, headRefOid and closesIssues on every scheduled sync. Unrelated to incidents, but a real bug and a one-line fix. Also: the "config edit, not a rescan" claim holds only for rows scanned since the feature landed; earlier rows need a one-time `prism scan --state all`, now documented. Extracted the duplicated hydration expression so the two sites cannot drift, documented `incidents:` in the config template so init surfaces it, and fixed a stale doc comment. --- ARCHITECTURE.md | 2 +- CHANGELOG.md | 2 ++ prism.config.yaml | 15 +++++++++++++++ server/db.ts | 6 ++++++ server/scheduler.ts | 16 +++++----------- src/__tests__/incident.test.ts | 27 ++++++++++++++++++++++++++ src/canonical.ts | 3 +++ src/config.ts | 35 +++++++++++++++++++++++++++------- src/incident.ts | 2 ++ src/store.ts | 13 +++++++++++-- 10 files changed, 100 insertions(+), 21 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 3b588af..87218f6 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -73,7 +73,7 @@ server/ # webhook server (GitHub App) - **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 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. confirmed (identity) clusters override to the earliest-created rule: byte-identical dupes resolve by which-was-first, never score (a copy can outscore its original) -- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag - so correcting a mis-set window is a config edit, not a rescan of the whole backlog. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected +- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag - so correcting a mis-set window is a config edit, not a rescan — for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path ranks exactly as it did before. wiring it needs a config field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled metadata literal replaced — tracked separately. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected - **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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bf381e..ccb0495 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ all notable changes to pr-prism are documented here. - `prism benchmark --out ` writes a run's results to a chosen file. every run previously wrote `data/benchmark-results.json`, so a second run silently destroyed the first one's numbers - an hour of embedding lost to starting the next comparison. an empty or directory-shaped value is rejected rather than falling back to the default - benchmark results now record `clusterMembership` per model per threshold. cluster counts cannot tell you whether a model finding more clusters is catching real duplicates or chaining unrelated items, and re-deriving membership means re-embedding the whole corpus - starmap items now carry `createdAt` alongside `updatedAt` (additive), so consumers can render and reason about which-was-first without re-fetching from github. star-map's importer rejects unknown fields; the coordinated patch is star-map PR #11, which must land before it consumes a dataset carrying this field +- incident-aware ranking: `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows. a repository-wide event (visibility flip, bulk close, migration) closes items for reasons unrelated to their quality, and because `selectCanonical()` ranks lifecycle state before score those items sank below genuinely-closed siblings, inverting triage order for exactly the backlog a maintainer needs. PRs closed inside a window now rank as open. `closedAt` is captured from both API paths and stored in `metadata_json` (no schema migration); `incidentClosed` is derived at read time, so correcting a window is a config edit rather than a rescan. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted. an offset-less timestamp parses in the host timezone and would select different PRs on a laptop than in CI. starmap items carry `incidentClosed: true` (omitted when false). **CLI only**: the server/GitHub-App path is unchanged and still ranks without incident awareness, because `ServerConfig` has no `incidents` field and the scheduler fetches open items only. NOTE for star-map: same coordinated importer patch as `createdAt` below, since its importer rejects unknown fields +- NOTE: rows scanned before this release carry no `closedAt`, and a default scan only fetches open items. run `prism scan --state all` once to backfill before configuring a window, or previously-closed PRs stay classified as deliberate closes ## [3.1.0] — 2026-07-20 diff --git a/prism.config.yaml b/prism.config.yaml index e480717..c6b87ac 100644 --- a/prism.config.yaml +++ b/prism.config.yaml @@ -16,6 +16,21 @@ vision_doc: ./VISION.md # owner/repo1: ./VISION_1.md # owner/repo2: ./VISION_2.md +# repository-wide events that closed items for reasons unrelated to their +# quality (a visibility flip, a bulk close, a migration). PRs closed inside a +# window rank as open instead of rejected, so a mass auto-close does not bury +# the exact backlog you need to triage. +# +# start/end MUST carry an explicit UTC offset - an offset-less timestamp is +# parsed in the host timezone and would select a different set of PRs on your +# laptop than in CI. Pin the window tightly: a day-wide window sweeps up items +# that were closed deliberately on the same day. +# +# incidents: +# - start: "2026-07-23T09:00:00Z" +# end: "2026-07-23T11:00:00Z" +# reason: "repo visibility flip auto-closed all open PRs" + thresholds: duplicate_similarity: 0.85 aligned: 0.65 diff --git a/server/db.ts b/server/db.ts index 107b77a..6913375 100644 --- a/server/db.ts +++ b/server/db.ts @@ -38,6 +38,12 @@ export function openRepoDB( model?: string, ): VectorStore { const dbPath = getRepoDBPath(dataDir, owner, repo); + // NOTE: no incident windows. Incident-aware ranking is CLI-only for now: + // ServerConfig has no `incidents` field and the scheduler fetches open items + // only, so `closedAt` is never populated on this path. Wiring the App path + // needs a config field, a scheduler that fetches closed items, and the + // triage.ts metadata literal fixed. Tracked separately; until then this + // store deliberately ranks the way it always has. return new VectorStore(dbPath, dimensions, model); } diff --git a/server/scheduler.ts b/server/scheduler.ts index 5608081..62eac40 100644 --- a/server/scheduler.ts +++ b/server/scheduler.ts @@ -1,3 +1,4 @@ +import { itemMetadata } from "../src/pipeline.js"; import cron from "node-cron"; import { findDuplicateClusters } from "../src/cluster.js"; import { createEmbeddingProvider, prepareEmbeddingText } from "../src/embeddings.js"; @@ -165,17 +166,10 @@ export async function runBacklogScan( title: item.title, bodySnippet: (item.body || "").slice(0, 2000), embedding, - metadata: { - author: item.author, - state: item.state, - labels: item.labels, - additions: item.additions, - deletions: item.deletions, - changedFiles: item.changedFiles, - ciStatus: item.ciStatus, - reviewCount: item.reviewCount, - hasTests: item.hasTests, - }, + // Single source for stored metadata. The hand-rolled literal that + // was here had drifted from it, silently dropping bodyLength, + // nodeId, headRefOid and closesIssues on every scheduled sync. + metadata: itemMetadata(item), createdAt: item.createdAt, updatedAt: item.updatedAt, }; diff --git a/src/__tests__/incident.test.ts b/src/__tests__/incident.test.ts index 6188682..a1c4a3c 100644 --- a/src/__tests__/incident.test.ts +++ b/src/__tests__/incident.test.ts @@ -63,3 +63,30 @@ describe("itemMetadata", () => { expect(itemMetadata(item).closedAt).toBe("2026-07-23T10:18:00Z"); }); }); + +describe("incident window validation", () => { + function writeConfig(body: string): string { + const dir = mkdtempSync(join(tmpdir(), "prism-incident-")); + const path = join(dir, "prism.config.yaml"); + writeFileSync(path, `repo: odysseus-dev/odysseus\nincidents:\n${body}`); + return path; + } + + it("rejects a window without an explicit UTC offset, which would parse in the host timezone", () => { + const path = writeConfig(' - start: "2026-07-23T00:00:00"\n end: "2026-07-24T00:00:00Z"\n reason: "x"\n'); + + expect(() => loadConfig(path)).toThrow(/offset/i); + }); + + it("rejects an unparseable timestamp instead of silently matching nothing", () => { + const path = writeConfig(' - start: "last tuesday"\n end: "2026-07-24T00:00:00Z"\n reason: "x"\n'); + + expect(() => loadConfig(path)).toThrow(); + }); + + it("rejects a window whose end precedes its start", () => { + const path = writeConfig(' - start: "2026-07-24T00:00:00Z"\n end: "2026-07-23T00:00:00Z"\n reason: "x"\n'); + + expect(() => loadConfig(path)).toThrow(/end/i); + }); +}); diff --git a/src/canonical.ts b/src/canonical.ts index ad05b3a..4d48bc8 100644 --- a/src/canonical.ts +++ b/src/canonical.ts @@ -58,6 +58,9 @@ export interface CanonicalDecision { * of truth, so it outranks open, which outranks a closed-unmerged PR. Absent state * (a triage DupeMatch has none) is 0, below every real state, so a stateless set * all ties here and falls through to the score/date rules unchanged. + * + * Takes the whole candidate rather than the bare state so an incident-closed + * item can be ranked where it sat before the incident. */ function statePriority(candidate: { state?: string; incidentClosed?: boolean }): number { // An incident-closed item never reached a maintainer verdict, so it is ranked diff --git a/src/config.ts b/src/config.ts index cf5a252..b131c40 100644 --- a/src/config.ts +++ b/src/config.ts @@ -51,15 +51,36 @@ const ClusterSchema = z.object({ bot_authors: z.array(z.string().trim().min(1)).default([]), }); +/** + * An absolute ISO-8601 instant. The explicit offset is required, not cosmetic: + * `Date.parse("2026-07-23T00:00:00")` resolves in the HOST timezone, so the + * same config would select a different set of items on a laptop in Denver than + * in CI running UTC — and `"2026-07-23"` parses as UTC while + * `"2026-07-23T00:00:00"` parses local, so two spellings of one day disagree. + * Rejecting the ambiguous forms is the only way the window means one thing. + */ +const AbsoluteInstant = z + .string() + .refine((s) => /(?:Z|[+-]\d{2}:?\d{2})$/.test(s.trim()), { + message: "must end in an explicit UTC offset (e.g. 2026-07-23T00:00:00Z or +02:00)", + }) + .refine((s) => !Number.isNaN(Date.parse(s)), { message: "is not a parseable ISO-8601 timestamp" }); + /** A repository-wide event that closed items for reasons unrelated to their * quality (visibility flip, bulk close, migration). See src/incident.ts. */ -const IncidentWindowSchema = z.object({ - /** ISO-8601, inclusive. */ - start: z.string(), - /** ISO-8601, exclusive. */ - end: z.string(), - reason: z.string(), -}); +const IncidentWindowSchema = z + .object({ + /** ISO-8601 with explicit offset, inclusive. */ + start: AbsoluteInstant, + /** ISO-8601 with explicit offset, exclusive. */ + end: AbsoluteInstant, + reason: z.string().trim().min(1), + }) + // An inverted window matches nothing and reports nothing. Fail at load + // instead: a silent no-op here is indistinguishable from "no incident". + .refine((w) => Date.parse(w.end) > Date.parse(w.start), { + message: "incident window end must be after start", + }); const ConfigSchema = z.object({ version: z.number().optional().default(1), diff --git a/src/incident.ts b/src/incident.ts index f136a0c..97ed1f7 100644 --- a/src/incident.ts +++ b/src/incident.ts @@ -16,6 +16,8 @@ export interface IncidentWindow { reason: string; } +/** Accepts null as well as undefined: a SQLite row round-trips a missing + * close timestamp as null, while PRItem uses undefined. */ export interface IncidentClosable { state: string; closedAt?: string | null; diff --git a/src/store.ts b/src/store.ts index 912a55b..1bcf59b 100644 --- a/src/store.ts +++ b/src/store.ts @@ -138,6 +138,15 @@ export class VectorStore { } } + /** One place that decides the incident flag, so the two hydration sites + * cannot drift apart. */ + private incidentFlag(metadata: { state?: unknown; closedAt?: unknown }): boolean { + return isIncidentClosed( + { state: String(metadata.state ?? ""), closedAt: (metadata.closedAt as string) ?? null }, + this.incidentWindows, + ); + } + getMeta(key: string): string | undefined { const row = this.db.prepare("SELECT value FROM prism_meta WHERE key = ?").get(key) as any; return row?.value; @@ -274,7 +283,7 @@ export class VectorStore { authorIsBot: metadata.authorIsBot, state: metadata.state, closedAt: metadata.closedAt, - incidentClosed: isIncidentClosed({ state: metadata.state, closedAt: metadata.closedAt }, this.incidentWindows), + incidentClosed: this.incidentFlag(metadata), labels: metadata.labels, additions: metadata.additions, deletions: metadata.deletions, @@ -341,7 +350,7 @@ export class VectorStore { authorIsBot: metadata.authorIsBot, state: metadata.state, closedAt: metadata.closedAt, - incidentClosed: isIncidentClosed({ state: metadata.state, closedAt: metadata.closedAt }, this.incidentWindows), + incidentClosed: this.incidentFlag(metadata), labels: metadata.labels, additions: metadata.additions, deletions: metadata.deletions, From 2b87c44e2f0b657371e22091b36e3ea11512754a Mon Sep 17 00:00:00 2001 From: StressTestor Date: Wed, 29 Jul 2026 13:01:19 -0600 Subject: [PATCH 3/5] docs(incident): close out the gate follow-ups on the CLI-only claim Three loose ends from the incident-window review. ARCHITECTURE said the server path "ranks exactly as it did before", which is now wrong twice over: it overstates a narrow gap, and bot filtering does reach that path, since scheduler.ts calls findDuplicateClusters and picks up the default. Scoped to what is actually missing there, which is incident windows. "Tracked separately" tracked nothing. Opened #27 for wiring the server path, and pointed both the doc and the db.ts comment at it. The issue also carries the triage.ts hand-rolled metadata literal, which is worth fixing on its own: it has already drifted behind itemMetadata() by three fields and nothing catches it. scheduler.ts moving to the shared helper is now in the changelog, which it was not. Adds config tests for the incident window rules. Checked they discriminate: relaxing `reason` to a bare `z.string()` fails the blank-reason case rather than passing silently. --- ARCHITECTURE.md | 2 +- CHANGELOG.md | 3 ++- server/db.ts | 4 +-- src/__tests__/config.test.ts | 48 ++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 4 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 87218f6..b282cbd 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -73,7 +73,7 @@ server/ # webhook server (GitHub App) - **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 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. confirmed (identity) clusters override to the earliest-created rule: byte-identical dupes resolve by which-was-first, never score (a copy can outscore its original) -- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag - so correcting a mis-set window is a config edit, not a rescan — for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path ranks exactly as it did before. wiring it needs a config field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled metadata literal replaced — tracked separately. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected +- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag, so correcting a mis-set window is a config edit rather than a rescan, for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path is not otherwise frozen: bot filtering (`src/bots.ts`) does apply to it, since `scheduler.ts` calls `findDuplicateClusters` and gets the default. wiring incident awareness there needs a `ServerConfig.incidents` field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled `metadata: { author, state }` literal replaced with `itemMetadata()` the way `scheduler.ts` now is: tracked in #27. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected - **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 diff --git a/CHANGELOG.md b/CHANGELOG.md index ccb0495..d5ecbba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ all notable changes to pr-prism are documented here. - `prism benchmark --out ` writes a run's results to a chosen file. every run previously wrote `data/benchmark-results.json`, so a second run silently destroyed the first one's numbers - an hour of embedding lost to starting the next comparison. an empty or directory-shaped value is rejected rather than falling back to the default - benchmark results now record `clusterMembership` per model per threshold. cluster counts cannot tell you whether a model finding more clusters is catching real duplicates or chaining unrelated items, and re-deriving membership means re-embedding the whole corpus - starmap items now carry `createdAt` alongside `updatedAt` (additive), so consumers can render and reason about which-was-first without re-fetching from github. star-map's importer rejects unknown fields; the coordinated patch is star-map PR #11, which must land before it consumes a dataset carrying this field -- incident-aware ranking: `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows. a repository-wide event (visibility flip, bulk close, migration) closes items for reasons unrelated to their quality, and because `selectCanonical()` ranks lifecycle state before score those items sank below genuinely-closed siblings, inverting triage order for exactly the backlog a maintainer needs. PRs closed inside a window now rank as open. `closedAt` is captured from both API paths and stored in `metadata_json` (no schema migration); `incidentClosed` is derived at read time, so correcting a window is a config edit rather than a rescan. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted. an offset-less timestamp parses in the host timezone and would select different PRs on a laptop than in CI. starmap items carry `incidentClosed: true` (omitted when false). **CLI only**: the server/GitHub-App path is unchanged and still ranks without incident awareness, because `ServerConfig` has no `incidents` field and the scheduler fetches open items only. NOTE for star-map: same coordinated importer patch as `createdAt` below, since its importer rejects unknown fields +- incident-aware ranking: `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows. a repository-wide event (visibility flip, bulk close, migration) closes items for reasons unrelated to their quality, and because `selectCanonical()` ranks lifecycle state before score those items sank below genuinely-closed siblings, inverting triage order for exactly the backlog a maintainer needs. PRs closed inside a window now rank as open. `closedAt` is captured from both API paths and stored in `metadata_json` (no schema migration); `incidentClosed` is derived at read time, so correcting a window is a config edit rather than a rescan. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted. an offset-less timestamp parses in the host timezone and would select different PRs on a laptop than in CI. starmap items carry `incidentClosed: true` (omitted when false). **CLI only**: the server/GitHub-App path applies no incident windows, because `ServerConfig` has no `incidents` field and the scheduler fetches open items only (#27). NOTE for star-map: same coordinated importer patch as `createdAt` below, since its importer rejects unknown fields +- `server/scheduler.ts` now builds stored metadata with the shared `itemMetadata()` instead of its own literal. the hand-rolled copy had already drifted behind the real one, so items scanned by the App path were missing fields the CLI path stored. `server/triage.ts` still hand-rolls its own and is tracked in #27 - NOTE: rows scanned before this release carry no `closedAt`, and a default scan only fetches open items. run `prism scan --state all` once to backfill before configuring a window, or previously-closed PRs stay classified as deliberate closes ## [3.1.0] — 2026-07-20 diff --git a/server/db.ts b/server/db.ts index 6913375..0c87850 100644 --- a/server/db.ts +++ b/server/db.ts @@ -42,8 +42,8 @@ export function openRepoDB( // ServerConfig has no `incidents` field and the scheduler fetches open items // only, so `closedAt` is never populated on this path. Wiring the App path // needs a config field, a scheduler that fetches closed items, and the - // triage.ts metadata literal fixed. Tracked separately; until then this - // store deliberately ranks the way it always has. + // triage.ts metadata literal fixed. Tracked in #27. Incident windows are the + // only thing missing here; other clustering behaviour is shared. return new VectorStore(dbPath, dimensions, model); } diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 4743894..56696e7 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -130,6 +130,54 @@ describe("loadEnvConfig", () => { }); }); +describe("incident window config", () => { + function load(yaml: string) { + const dir = mkdtempSync(join(tmpdir(), "prism-incident-")); + const path = join(dir, "prism.config.yaml"); + writeFileSync(path, yaml); + try { + return loadConfig(path); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + + const window = (extra: string) => + `repo: owner/name\nincidents:\n - start: 2026-07-23T00:00:00Z\n end: 2026-07-24T00:00:00Z\n${extra}`; + + it("accepts a well-formed window", () => { + const cfg = load(window(" reason: bulk close\n")); + expect(cfg.incidents?.[0].reason).toBe("bulk close"); + }); + + it("rejects a blank reason", () => { + // A window is a manual override of lifecycle state. Without a reason the + // next person cannot tell a deliberate correction from a stray edit. + expect(() => load(window(" reason: ''\n"))).toThrow(); + expect(() => load(window(" reason: ' '\n"))).toThrow(); + }); + + it("rejects a missing reason", () => { + expect(() => load(window(""))).toThrow(); + }); + + it("rejects bounds with no UTC offset", () => { + expect(() => + load( + "repo: owner/name\nincidents:\n - start: 2026-07-23T00:00:00\n end: 2026-07-24T00:00:00Z\n reason: x\n", + ), + ).toThrow(/offset/); + }); + + it("rejects an inverted window", () => { + expect(() => + load( + "repo: owner/name\nincidents:\n - start: 2026-07-24T00:00:00Z\n end: 2026-07-23T00:00:00Z\n reason: x\n", + ), + ).toThrow(/after start/); + }); +}); + describe("bot author clustering config", () => { function load(yaml: string) { const dir = mkdtempSync(join(tmpdir(), "prism-bots-")); From d2994b5e6cf2847c1d3fc2346bb12820d7096556 Mon Sep 17 00:00:00 2001 From: StressTestor Date: Wed, 29 Jul 2026 13:11:35 -0600 Subject: [PATCH 4/5] fix(incident): correct the server-path claims, tighten window validation Adversarial review follow-ups. The comment I added to server/db.ts said incident windows were the only thing missing on the App path. They are not: ServerConfig has no `cluster` field either, so `cluster.bot_authors` and `cluster.include_bot_authors` are honoured by the CLI and ignored by the App. Only the built-in bot list reaches that path. ARCHITECTURE had the same overstatement. A maintainer whose repo-specific bot login is filtered locally and not in the digest would have gone hunting for a clustering bug. The changelog called `prism scan --state all` a one-time backfill. It is per-incident: a default scan fetches open items only, so after a bulk close the affected PRs stop being returned and their rows keep `state: open` with no `closedAt`. Configure a window without re-scanning and it matches nothing. `incidentClosed` is emitted on every reference to an item, not just on items, so a consumer contract has to accept it in six positions. The changelog said "items", which understates what a coordinated importer patch needs. Validation: - the offset rule accepted `2026-07-23 00:00:00+00:00`, which has an offset but a space where the T belongs, putting Date.parse into its implementation-defined fallback. Now requires the full ISO-8601 shape. - an unparseable bound also reported "end must be after start", because NaN > NaN is false. That points the reader at the wrong field, so the ordering check now defers when either bound failed to parse. Tests: - `closedAt` was verified only through itemClosedAt in isolation. Both ways it can silently die (a query stops requesting the field, a mapper stops calling it) now fail a test; checked by breaking each in turn. - the incident hydration test covered getAllItems only. getAllItemsMulti duplicates that literal and is what cross-repo dupes uses. - the "unparseable timestamp" test fed input that tripped the offset rule first, so it passed with the parseability rule deleted. Split into cases that isolate each rule. --- ARCHITECTURE.md | 2 +- CHANGELOG.md | 4 ++-- server/db.ts | 5 +++-- src/__tests__/config.test.ts | 32 ++++++++++++++++++++++++++++++ src/__tests__/github.test.ts | 22 +++++++++++++++++++++ src/__tests__/store.test.ts | 38 ++++++++++++++++++++++++++++++++++++ src/config.ts | 23 +++++++++++++++++----- 7 files changed, 116 insertions(+), 10 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index b282cbd..7c48743 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -73,7 +73,7 @@ server/ # webhook server (GitHub App) - **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 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. confirmed (identity) clusters override to the earliest-created rule: byte-identical dupes resolve by which-was-first, never score (a copy can outscore its original) -- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag, so correcting a mis-set window is a config edit rather than a rescan, for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path is not otherwise frozen: bot filtering (`src/bots.ts`) does apply to it, since `scheduler.ts` calls `findDuplicateClusters` and gets the default. wiring incident awareness there needs a `ServerConfig.incidents` field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled `metadata: { author, state }` literal replaced with `itemMetadata()` the way `scheduler.ts` now is: tracked in #27. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected +- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag, so correcting a mis-set window is a config edit rather than a rescan, for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path is not otherwise frozen: `scheduler.ts` calls `findDuplicateClusters`, so the *built-in* bot list applies there. the `cluster.*` config does not, because `ServerConfig` has no `cluster` field, so a repo-specific `cluster.bot_authors` is honoured by the CLI and ignored by the App. wiring incident awareness there needs a `ServerConfig.incidents` field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled `metadata: { author, state }` literal replaced with `itemMetadata()` the way `scheduler.ts` now is: tracked in #27. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected - **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 diff --git a/CHANGELOG.md b/CHANGELOG.md index d5ecbba..c918104 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,9 +14,9 @@ all notable changes to pr-prism are documented here. - `prism benchmark --out ` writes a run's results to a chosen file. every run previously wrote `data/benchmark-results.json`, so a second run silently destroyed the first one's numbers - an hour of embedding lost to starting the next comparison. an empty or directory-shaped value is rejected rather than falling back to the default - benchmark results now record `clusterMembership` per model per threshold. cluster counts cannot tell you whether a model finding more clusters is catching real duplicates or chaining unrelated items, and re-deriving membership means re-embedding the whole corpus - starmap items now carry `createdAt` alongside `updatedAt` (additive), so consumers can render and reason about which-was-first without re-fetching from github. star-map's importer rejects unknown fields; the coordinated patch is star-map PR #11, which must land before it consumes a dataset carrying this field -- incident-aware ranking: `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows. a repository-wide event (visibility flip, bulk close, migration) closes items for reasons unrelated to their quality, and because `selectCanonical()` ranks lifecycle state before score those items sank below genuinely-closed siblings, inverting triage order for exactly the backlog a maintainer needs. PRs closed inside a window now rank as open. `closedAt` is captured from both API paths and stored in `metadata_json` (no schema migration); `incidentClosed` is derived at read time, so correcting a window is a config edit rather than a rescan. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted. an offset-less timestamp parses in the host timezone and would select different PRs on a laptop than in CI. starmap items carry `incidentClosed: true` (omitted when false). **CLI only**: the server/GitHub-App path applies no incident windows, because `ServerConfig` has no `incidents` field and the scheduler fetches open items only (#27). NOTE for star-map: same coordinated importer patch as `createdAt` below, since its importer rejects unknown fields +- incident-aware ranking: `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows. a repository-wide event (visibility flip, bulk close, migration) closes items for reasons unrelated to their quality, and because `selectCanonical()` ranks lifecycle state before score those items sank below genuinely-closed siblings, inverting triage order for exactly the backlog a maintainer needs. PRs closed inside a window now rank as open. `closedAt` is captured from both API paths and stored in `metadata_json` (no schema migration); `incidentClosed` is derived at read time, so correcting a window is a config edit rather than a rescan. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted. an offset-less timestamp parses in the host timezone and would select different PRs on a laptop than in CI. starmap carries `incidentClosed: true` on items *and* on every reference to them (canonical, runnerUp, partition, tracker ref and candidates), omitted when false. a consumer contract has to accept it in all of those positions, not just on items. **CLI only**: the server/GitHub-App path applies no incident windows, because `ServerConfig` has no `incidents` field and the scheduler fetches open items only (#27). NOTE for star-map: same coordinated importer patch as `createdAt` below, since its importer rejects unknown fields - `server/scheduler.ts` now builds stored metadata with the shared `itemMetadata()` instead of its own literal. the hand-rolled copy had already drifted behind the real one, so items scanned by the App path were missing fields the CLI path stored. `server/triage.ts` still hand-rolls its own and is tracked in #27 -- NOTE: rows scanned before this release carry no `closedAt`, and a default scan only fetches open items. run `prism scan --state all` once to backfill before configuring a window, or previously-closed PRs stay classified as deliberate closes +- NOTE: rows scanned before this release carry no `closedAt`, and a default scan only fetches open items. run `prism scan --state all` to pick them up. this is per-incident, not a one-time backfill: a default scan fetches open items only, so after a bulk close the affected PRs are no longer returned at all and their stored rows keep `state: open` with no `closedAt`. re-scan with `--state all` after each incident, or the window matches nothing ## [3.1.0] — 2026-07-20 diff --git a/server/db.ts b/server/db.ts index 0c87850..0f1dfaf 100644 --- a/server/db.ts +++ b/server/db.ts @@ -42,8 +42,9 @@ export function openRepoDB( // ServerConfig has no `incidents` field and the scheduler fetches open items // only, so `closedAt` is never populated on this path. Wiring the App path // needs a config field, a scheduler that fetches closed items, and the - // triage.ts metadata literal fixed. Tracked in #27. Incident windows are the - // only thing missing here; other clustering behaviour is shared. + // triage.ts metadata literal fixed. Tracked in #27. ServerConfig also has no + // `cluster` field, so `cluster.bot_authors` and `cluster.include_bot_authors` + // do not reach this path either; only the built-in bot list in bots.ts applies. return new VectorStore(dbPath, dimensions, model); } diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 56696e7..b6bde59 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -169,6 +169,38 @@ describe("incident window config", () => { ).toThrow(/offset/); }); + it("rejects a bound that has an offset but is not a real instant", () => { + // Isolates the parseability rule: the offset rule passes, Date.parse does + // not. "last tuesday" trips the offset rule first and proves nothing. + expect(() => + load( + "repo: owner/name\nincidents:\n - start: 2026-13-45T00:00:00Z\n end: 2026-07-24T00:00:00Z\n reason: x\n", + ), + ).toThrow(/parseable/); + }); + + it("rejects the space-separated form, which parses per-engine", () => { + // `2026-07-23 00:00:00+00:00` is not ISO-8601; Date.parse falls back to + // implementation-defined behaviour, the ambiguity this schema exists to reject. + expect(() => + load( + "repo: owner/name\nincidents:\n - start: '2026-07-23 00:00:00+00:00'\n end: 2026-07-24T00:00:00Z\n reason: x\n", + ), + ).toThrow(); + }); + + it("does not blame ordering when a bound is unparseable", () => { + // Reporting "end must be after start" for a bound that never parsed sends + // the reader to the wrong field. + let message = ""; + try { + load("repo: owner/name\nincidents:\n - start: last tuesday\n end: 2026-07-24T00:00:00Z\n reason: x\n"); + } catch (e) { + message = (e as Error).message; + } + expect(message).not.toMatch(/after start/); + }); + it("rejects an inverted window", () => { expect(() => load( diff --git a/src/__tests__/github.test.ts b/src/__tests__/github.test.ts index 247e0da..149753a 100644 --- a/src/__tests__/github.test.ts +++ b/src/__tests__/github.test.ts @@ -1,3 +1,4 @@ +import { readFileSync } from "node:fs"; import { describe, expect, it, vi } from "vitest"; import { accountIsBot, GitHubClient, itemClosedAt, restPRState } from "../github.js"; import { createWriteGate } from "../write-gate.js"; @@ -233,3 +234,24 @@ describe("itemClosedAt", () => { expect(itemClosedAt({ closed_at: null })).toBeUndefined(); }); }); + +describe("closedAt wiring", () => { + // itemClosedAt being correct is worth nothing if a query stops asking for the + // field or a mapper stops calling it. Both failures are silent: every item + // hydrates with closedAt undefined, isIncidentClosed returns false for + // everything, and the whole incident feature dies with all tests green. + const source = readFileSync(new URL("../github.ts", import.meta.url), "utf-8"); + + it("both GraphQL queries still request closedAt", () => { + const queryBlocks = source.split("author { login __typename }").length - 1; + expect(queryBlocks).toBe(2); + expect(source.match(/^\s+closedAt$/gm)?.length).toBe(2); + }); + + it("every PRItem construction site populates closedAt", () => { + // Five constructors: 2 GraphQL (pr, issue), 3 REST (pr list, issue list, single pr). + const constructions = source.match(/author: \w+\??\.\w+\?\.login \|\| "unknown"/g) ?? []; + const closedAtCalls = source.match(/closedAt: itemClosedAt\(/g) ?? []; + expect(closedAtCalls.length).toBe(constructions.length); + }); +}); diff --git a/src/__tests__/store.test.ts b/src/__tests__/store.test.ts index 31691ea..fe330ae 100644 --- a/src/__tests__/store.test.ts +++ b/src/__tests__/store.test.ts @@ -370,6 +370,44 @@ describe("closesIssues metadata round-trip", () => { expect(byNumber.get(1)).toBe(true); expect(byNumber.get(2)).toBe(false); }); + + it("marks them on the multi-repo read path too", () => { + // getAllItemsMulti duplicates the hydration literal for the >1 repo branch, + // and that copy is what cross-repo `dupes` uses. An omission there is + // invisible from the single-repo tests above. + const path = tmpDb(); + dbs.push(path); + const store = new VectorStore(path, 4, undefined, [ + { start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }, + ]); + const base = { + type: "pr" as const, + title: "t", + bodySnippet: "", + embedding: new Float32Array(4), + createdAt: "2026-07-20T00:00:00Z", + updatedAt: "2026-07-23T10:18:00Z", + }; + store.upsert({ + ...base, + id: "a", + number: 1, + repo: "owner/one", + metadata: { state: "closed", closedAt: "2026-07-23T10:18:00Z" }, + }); + store.upsert({ + ...base, + id: "b", + number: 2, + repo: "owner/two", + metadata: { state: "closed", closedAt: "2026-07-01T00:00:00Z" }, + }); + + const items = store.getAllItemsMulti(["owner/one", "owner/two"]) as any[]; + const byNumber = new Map(items.map((i) => [i.number, i.incidentClosed])); + expect(byNumber.get(1)).toBe(true); + expect(byNumber.get(2)).toBe(false); + }); }); describe("bot authorship round-trip", () => { diff --git a/src/config.ts b/src/config.ts index b131c40..a479763 100644 --- a/src/config.ts +++ b/src/config.ts @@ -59,10 +59,15 @@ const ClusterSchema = z.object({ * `"2026-07-23T00:00:00"` parses local, so two spellings of one day disagree. * Rejecting the ambiguous forms is the only way the window means one thing. */ +const ISO_8601_ABSOLUTE = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?(?:Z|[+-]\d{2}:?\d{2})$/; + const AbsoluteInstant = z .string() - .refine((s) => /(?:Z|[+-]\d{2}:?\d{2})$/.test(s.trim()), { - message: "must end in an explicit UTC offset (e.g. 2026-07-23T00:00:00Z or +02:00)", + // The full ISO-8601 shape, not just a trailing offset. "2026-07-23 00:00:00+00:00" + // has an offset but a space where the T belongs, which puts Date.parse into its + // implementation-defined fallback: the ambiguity this rule exists to reject. + .refine((s) => ISO_8601_ABSOLUTE.test(s.trim()), { + message: "must be an ISO-8601 instant with an explicit UTC offset (e.g. 2026-07-23T00:00:00Z or +02:00)", }) .refine((s) => !Number.isNaN(Date.parse(s)), { message: "is not a parseable ISO-8601 timestamp" }); @@ -78,9 +83,17 @@ const IncidentWindowSchema = z }) // An inverted window matches nothing and reports nothing. Fail at load // instead: a silent no-op here is indistinguishable from "no incident". - .refine((w) => Date.parse(w.end) > Date.parse(w.start), { - message: "incident window end must be after start", - }); + .refine( + (w) => { + const start = Date.parse(w.start); + const end = Date.parse(w.end); + // An unparseable bound is already reported by the field rules. Claiming + // the ordering is wrong too would point the reader at the wrong field. + if (Number.isNaN(start) || Number.isNaN(end)) return true; + return end > start; + }, + { message: "incident window end must be after start" }, + ); const ConfigSchema = z.object({ version: z.number().optional().default(1), From 51d91e715c8a0abdadebecebd5b5d9c240b1b7a0 Mon Sep 17 00:00:00 2001 From: StressTestor Date: Wed, 29 Jul 2026 13:15:43 -0600 Subject: [PATCH 5/5] refactor(incident): compile windows once and fail loudly on a bad one Review follow-ups. isIncidentClosed skipped a window whose bounds would not parse and returned false, so a malformed window matched nothing and said nothing. That is the same silent no-op the config schema rejects at load, and windows reach a VectorStore from callers that never went through that schema. Bounds are now resolved once by compileIncidentWindows, which throws and names the offending window by its reason so a long list is searchable. Compiling once also stops re-parsing two constants for every item in the backlog, which was work with no result. The incident windows were a fourth positional constructor parameter. They are now a named options object, so the tail stays one argument as more knobs arrive. The first three positionals are pre-existing and left alone. itemMetadata moves out of pipeline.ts into src/metadata.ts. server/scheduler.ts needs it, and importing it from the CLI pipeline dragged a spinner, a GitHub client and an embedder factory into the server for one object literal. The flag has two spellings on purpose, now stated where they are declared: `boolean` internally, `true`-only on the starmap wire format, because the payload omits the field when false and a consumer contract should reject anything else. Also corrects ARCHITECTURE's "one-time backfill" the same way the changelog was already corrected: `--state all` is needed after each incident. --- ARCHITECTURE.md | 4 ++- server/scheduler.ts | 2 +- src/__tests__/incident.test.ts | 40 ++++++++++++++++++++++++++--- src/__tests__/store.test.ts | 12 ++++----- src/canonical.ts | 3 ++- src/incident.ts | 47 +++++++++++++++++++++++++++++----- src/metadata.ts | 30 ++++++++++++++++++++++ src/pipeline.ts | 27 +++---------------- src/starmap.ts | 6 ++++- src/store.ts | 19 ++++++++++++-- 10 files changed, 145 insertions(+), 45 deletions(-) create mode 100644 src/metadata.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7c48743..83303e9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -46,6 +46,8 @@ src/ # CLI tool (published to npm as prism-triage) write-gate.ts # one dry-run-by-default gate every GitHub mutation funnels through (read-only ethos) benchmark.ts # embedding provider benchmark tool (--out per-run results + cluster membership) bots.ts # bot-author detection; excluded from clustering by default + incident.ts # incident windows: compile once, match closedAt at read time + metadata.ts # the stored-metadata shape, shared by the CLI and App paths config.ts # Zod-validated YAML + env config errors.ts # typed error classes types.ts # shared interfaces @@ -73,7 +75,7 @@ server/ # webhook server (GitHub App) - **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 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. confirmed (identity) clusters override to the earliest-created rule: byte-identical dupes resolve by which-was-first, never score (a copy can outscore its original) -- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag, so correcting a mis-set window is a config edit rather than a rescan, for rows scanned since the feature landed. rows stored earlier carry no `closedAt` and a default scan only fetches open items, so a one-time `prism scan --state all` is needed to backfill them. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path is not otherwise frozen: `scheduler.ts` calls `findDuplicateClusters`, so the *built-in* bot list applies there. the `cluster.*` config does not, because `ServerConfig` has no `cluster` field, so a repo-specific `cluster.bot_authors` is honoured by the CLI and ignored by the App. wiring incident awareness there needs a `ServerConfig.incidents` field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled `metadata: { author, state }` literal replaced with `itemMetadata()` the way `scheduler.ts` now is: tracked in #27. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected +- **incident awareness**: a repository-wide event (visibility flip, bulk close, migration) can close hundreds of PRs for reasons unrelated to their quality. because `selectCanonical()` ranks lifecycle state before score, those items would otherwise rank as rejections and sink below genuinely-closed siblings. `prism.config.yaml` accepts an `incidents:` list of `{start, end, reason}` windows; `store.ts` stamps `incidentClosed` onto each item at hydration via `isIncidentClosed()` (src/incident.ts), and `statePriority()` ranks an incident-closed PR as open. the raw `closedAt` is what gets persisted, never the derived flag, so correcting a mis-set window is a config edit rather than a rescan, for rows scanned since the feature landed. rows stored earlier carry no `closedAt`, and a default scan fetches open items only, so after a bulk close the affected PRs are no longer returned at all and their stored rows keep `state: open`. `prism scan --state all` is needed after each incident, not once. window bounds require an explicit UTC offset and are rejected at load if unparseable or inverted: an offset-less timestamp resolves in the host timezone, so the same config would select different items on a laptop than in CI. **CLI-only.** the server / GitHub-App path (`server/db.ts`, `server/scheduler.ts`) does not apply windows: `ServerConfig` has no `incidents` field and the scheduler fetches open items only, so `closedAt` is never populated there. that path is not otherwise frozen: `scheduler.ts` calls `findDuplicateClusters`, so the *built-in* bot list applies there. the `cluster.*` config does not, because `ServerConfig` has no `cluster` field, so a repo-specific `cluster.bot_authors` is honoured by the CLI and ignored by the App. wiring incident awareness there needs a `ServerConfig.incidents` field, a scheduler that fetches closed items, and `server/triage.ts`'s hand-rolled `metadata: { author, state }` literal replaced with `itemMetadata()` the way `scheduler.ts` now is: tracked in #27. the starmap payload carries `incidentClosed: true` (omitted when false, keeping the contract additive) so a consumer can bucket them for re-triage rather than treating them as rejected - **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 diff --git a/server/scheduler.ts b/server/scheduler.ts index 62eac40..5559a64 100644 --- a/server/scheduler.ts +++ b/server/scheduler.ts @@ -1,8 +1,8 @@ -import { itemMetadata } from "../src/pipeline.js"; import cron from "node-cron"; import { findDuplicateClusters } from "../src/cluster.js"; import { createEmbeddingProvider, prepareEmbeddingText } from "../src/embeddings.js"; import { GitHubClient } from "../src/github.js"; +import { itemMetadata } from "../src/metadata.js"; import { escapeTableCell } from "../src/sanitize.js"; import type { Cluster, PRItem, StoreItem } from "../src/types.js"; import { diff --git a/src/__tests__/incident.test.ts b/src/__tests__/incident.test.ts index a1c4a3c..c0fe345 100644 --- a/src/__tests__/incident.test.ts +++ b/src/__tests__/incident.test.ts @@ -3,8 +3,8 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { loadConfig } from "../config.js"; -import { isIncidentClosed } from "../incident.js"; -import { itemMetadata } from "../pipeline.js"; +import { compileIncidentWindows, isIncidentClosed } from "../incident.js"; +import { itemMetadata } from "../metadata.js"; const INCIDENT = { start: "2026-07-23T00:00:00Z", @@ -16,7 +16,7 @@ describe("isIncidentClosed", () => { it("classifies a PR closed inside an incident window as incident-closed", () => { const item = { state: "closed", closedAt: "2026-07-23T10:18:00Z" }; - expect(isIncidentClosed(item, [INCIDENT])).toBe(true); + expect(isIncidentClosed(item, compileIncidentWindows([INCIDENT]))).toBe(true); }); }); @@ -90,3 +90,37 @@ describe("incident window validation", () => { expect(() => loadConfig(path)).toThrow(/end/i); }); }); + +describe("compileIncidentWindows", () => { + it("rejects an unparseable bound instead of skipping the window", () => { + // Config validation catches this on the CLI path, but VectorStore accepts + // windows from any caller. Silently ignoring a malformed window is the + // same no-op the config layer exists to prevent, just later and quieter. + expect(() => compileIncidentWindows([{ start: "nonsense", end: "2026-07-24T00:00:00Z", reason: "x" }])).toThrow( + /nonsense/, + ); + }); + + it("rejects an inverted window", () => { + expect(() => + compileIncidentWindows([{ start: "2026-07-24T00:00:00Z", end: "2026-07-23T00:00:00Z", reason: "x" }]), + ).toThrow(/after its start/); + }); + + it("names the offending window so a long list is searchable", () => { + expect(() => + compileIncidentWindows([ + { start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "fine" }, + { start: "broken", end: "2026-07-24T00:00:00Z", reason: "the bad one" }, + ]), + ).toThrow(/the bad one/); + }); + + it("parses bounds once, so matching does not re-parse per item", () => { + const [w] = compileIncidentWindows([ + { start: "2026-07-23T09:00:00Z", end: "2026-07-23T11:00:00Z", reason: "flip" }, + ]); + expect(w.start).toBe(Date.parse("2026-07-23T09:00:00Z")); + expect(w.end).toBe(Date.parse("2026-07-23T11:00:00Z")); + }); +}); diff --git a/src/__tests__/store.test.ts b/src/__tests__/store.test.ts index fe330ae..3e7b85a 100644 --- a/src/__tests__/store.test.ts +++ b/src/__tests__/store.test.ts @@ -339,9 +339,9 @@ describe("closesIssues metadata round-trip", () => { it("marks items closed inside a configured incident window", () => { const path = tmpDb(); dbs.push(path); - const store = new VectorStore(path, 4, undefined, [ - { start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }, - ]); + const store = new VectorStore(path, 4, undefined, { + incidentWindows: [{ start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }], + }); const base = { type: "pr" as const, repo: "odysseus-dev/odysseus", @@ -377,9 +377,9 @@ describe("closesIssues metadata round-trip", () => { // invisible from the single-repo tests above. const path = tmpDb(); dbs.push(path); - const store = new VectorStore(path, 4, undefined, [ - { start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }, - ]); + const store = new VectorStore(path, 4, undefined, { + incidentWindows: [{ start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "visibility flip" }], + }); const base = { type: "pr" as const, title: "t", diff --git a/src/canonical.ts b/src/canonical.ts index 4d48bc8..6cdeef8 100644 --- a/src/canonical.ts +++ b/src/canonical.ts @@ -28,7 +28,8 @@ export interface CanonicalCandidate { state?: string; /** True when this item was closed by a repository-wide incident rather than * by a maintainer decision. Ranked as open: a bulk auto-close says nothing - * about the item's quality. See src/incident.ts. */ + * about the item's quality. Same `boolean` as `PRItem.incidentClosed`, which + * is what gets passed in; see src/incident.ts. */ incidentClosed?: boolean; /** CI rollup; absent for callers with no CI signal (triage). Only "failure" demotes. */ ciStatus?: "success" | "failure" | "pending" | "unknown"; diff --git a/src/incident.ts b/src/incident.ts index 97ed1f7..344b2d1 100644 --- a/src/incident.ts +++ b/src/incident.ts @@ -8,6 +8,7 @@ * rank as though they had been rejected. */ +/** A window as written in `prism.config.yaml`, with bounds still as text. */ export interface IncidentWindow { /** ISO-8601 timestamp; inclusive. */ start: string; @@ -16,6 +17,13 @@ export interface IncidentWindow { reason: string; } +/** A window with its bounds resolved to epoch milliseconds. */ +export interface CompiledIncidentWindow { + start: number; + end: number; + reason: string; +} + /** Accepts null as well as undefined: a SQLite row round-trips a missing * close timestamp as null, while PRItem uses undefined. */ export interface IncidentClosable { @@ -23,14 +31,39 @@ export interface IncidentClosable { closedAt?: string | null; } -export function isIncidentClosed(item: IncidentClosable, windows: readonly IncidentWindow[]): boolean { - if (item.state !== "closed" || !item.closedAt) return false; - const closed = Date.parse(item.closedAt); - if (Number.isNaN(closed)) return false; - return windows.some((w) => { +/** + * Resolve window bounds once, up front. + * + * Two reasons this is not done per item. A malformed window would otherwise + * match nothing and say nothing, which is the silent no-op the config schema + * rejects at load; windows reaching a VectorStore from anywhere other than that + * validated path deserve the same treatment. And bounds are constants, so + * re-parsing them for every item in a backlog is work with no result. + */ +export function compileIncidentWindows(windows: readonly IncidentWindow[]): CompiledIncidentWindow[] { + return windows.map((w) => { const start = Date.parse(w.start); const end = Date.parse(w.end); - if (Number.isNaN(start) || Number.isNaN(end)) return false; - return closed >= start && closed < end; + // The reason is quoted so the offending entry is findable in a long list. + const label = `incident window ${JSON.stringify(w.reason)}`; + if (Number.isNaN(start)) { + throw new Error(`${label}: start ${JSON.stringify(w.start)} is not a parseable ISO-8601 timestamp`); + } + if (Number.isNaN(end)) { + throw new Error(`${label}: end ${JSON.stringify(w.end)} is not a parseable ISO-8601 timestamp`); + } + if (end <= start) { + throw new Error(`${label}: end must be after its start`); + } + return { start, end, reason: w.reason }; }); } + +/** Half-open `[start, end)`: an item closed exactly at `end` belongs to + * whatever happened next, not to this incident. */ +export function isIncidentClosed(item: IncidentClosable, windows: readonly CompiledIncidentWindow[]): boolean { + if (item.state !== "closed" || !item.closedAt) return false; + const closed = Date.parse(item.closedAt); + if (Number.isNaN(closed)) return false; + return windows.some((w) => closed >= w.start && closed < w.end); +} diff --git a/src/metadata.ts b/src/metadata.ts new file mode 100644 index 0000000..57beb44 --- /dev/null +++ b/src/metadata.ts @@ -0,0 +1,30 @@ +import type { PRItem } from "./types.js"; + +/** + * Single source for an item's stored metadata: used for new-item upserts, the + * unchanged-item refresh, and the App path's backlog scan, so drifting fields + * cannot diverge between them. + * + * Its own module rather than living beside the CLI pipeline, because the + * server imports it and should not pull a spinner, a GitHub client and an + * embedder factory along with it. + */ +export function itemMetadata(item: PRItem): Record { + return { + author: item.author, + authorIsBot: item.authorIsBot, + state: item.state, + closedAt: item.closedAt, + labels: item.labels, + additions: item.additions, + deletions: item.deletions, + changedFiles: item.changedFiles, + ciStatus: item.ciStatus, + reviewCount: item.reviewCount, + hasTests: item.hasTests, + bodyLength: item.body.length, + nodeId: item.nodeId, + headRefOid: item.headRefOid, + closesIssues: item.closesIssues, + }; +} diff --git a/src/pipeline.ts b/src/pipeline.ts index 21bb6e1..a68f7fa 100644 --- a/src/pipeline.ts +++ b/src/pipeline.ts @@ -13,6 +13,7 @@ import { } from "./embeddings.js"; import { GitHubClient } from "./github.js"; import { applyLabelActions, ensureLabelsExist, type LabelAction } from "./labels.js"; +import { itemMetadata } from "./metadata.js"; import { classifyClusterRelation } from "./relations.js"; import { escapeTableCell, sanitizeTitle } from "./sanitize.js"; import { buildScorerContext, rankPRs } from "./scorer.js"; @@ -65,7 +66,9 @@ export async function createPipelineContext(repoOverride?: string): Promise { - return { - author: item.author, - authorIsBot: item.authorIsBot, - state: item.state, - closedAt: item.closedAt, - labels: item.labels, - additions: item.additions, - deletions: item.deletions, - changedFiles: item.changedFiles, - ciStatus: item.ciStatus, - reviewCount: item.reviewCount, - hasTests: item.hasTests, - bodyLength: item.body.length, - nodeId: item.nodeId, - headRefOid: item.headRefOid, - closesIssues: item.closesIssues, - }; -} - export async function runScan( ctx: PipelineContext, opts: { since?: string; state?: string; useRest?: boolean; json?: boolean }, diff --git a/src/starmap.ts b/src/starmap.ts index 966edca..8715cb1 100644 --- a/src/starmap.ts +++ b/src/starmap.ts @@ -27,7 +27,11 @@ export interface StarmapItemRef { state?: "open" | "closed" | "merged"; /** Present only when true: this item was closed by a repository-wide incident * (a visibility flip, a bulk close), not by a maintainer verdict. Consumers - * should bucket these for re-triage rather than treating them as rejected. */ + * should bucket these for re-triage rather than treating them as rejected. + * + * Narrower than the internal `boolean` on PRItem deliberately: the payload + * omits the field entirely when false, so `true` is the only value that can + * appear on the wire and a consumer contract can reject anything else. */ incidentClosed?: true; } diff --git a/src/store.ts b/src/store.ts index 1bcf59b..e6aec49 100644 --- a/src/store.ts +++ b/src/store.ts @@ -2,11 +2,23 @@ import { mkdirSync } from "node:fs"; import { resolve } from "node:path"; import Database from "better-sqlite3"; import * as sqliteVec from "sqlite-vec"; -import { type IncidentWindow, isIncidentClosed } from "./incident.js"; +import { + type CompiledIncidentWindow, + compileIncidentWindows, + type IncidentWindow, + isIncidentClosed, +} from "./incident.js"; import type { StoreItem } from "./types.js"; +/** Named so the constructor tail stays one argument as more knobs arrive, + * rather than growing another positional parameter each time. */ +export interface VectorStoreOptions { + incidentWindows?: readonly IncidentWindow[]; +} + export class VectorStore { private db: Database.Database; + private incidentWindows: readonly CompiledIncidentWindow[]; private dimensions: number; private embeddingModel?: string; @@ -16,8 +28,11 @@ export class VectorStore { embeddingModel?: string, /** Repository-wide events that closed items for non-quality reasons. * Applied at read time so a corrected window needs no rescan. */ - private incidentWindows: readonly IncidentWindow[] = [], + options: VectorStoreOptions = {}, ) { + // Compiled here rather than per item: a malformed window throws now instead + // of quietly matching nothing on every read. + this.incidentWindows = compileIncidentWindows(options.incidentWindows ?? []); const p = dbPath || resolve(process.cwd(), "data", "prism.db"); mkdirSync(resolve(p, ".."), { recursive: true }); this.db = new Database(p);