diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 83303e9..8db7bd0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -75,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 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 +- **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. the server / GitHub-App path applies windows too: they are declared per repo in `{dataDir}/{owner}-{repo}/config.json` under `incidents`, validated on load (a readable config with a malformed window throws rather than silently becoming "no incident"), and passed through `openRepoDB()` into the same read-time flag. a scan fetches closed items only when that repo declares a window, since a window matches on `closedAt` and closed items otherwise cost API calls for nothing. the `cluster` block is declared in the same per-repo file (camelCase there: `botAuthors`, `includeBotAuthors`, matching that file's convention rather than the CLI yaml's snake_case) and reaches both server clustering sites plus the webhook triage matcher. the weekly digest loads that repo's config rather than a global one: it previously clustered every repo at `DEFAULT_REPO_CONFIG.similarityThreshold`, so it reported different clusters than the backlog scan for the same repo. 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 c918104..e83941f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ all notable changes to pr-prism are documented here. ## [unreleased] ### changed +- the `cluster` block (bot filtering) now reaches the server/GitHub-App path, declared per repo in that repo's `config.json` alongside `incidents`. previously a repo-specific bot login was honoured by the CLI and silently ignored by the App. note the key names differ by file format: the CLI's yaml uses `cluster.bot_authors` / `cluster.include_bot_authors`, the App's json uses `cluster.botAuthors` / `cluster.includeBotAuthors`, matching each file's existing convention, so the same repo produced different clusters depending on which path you looked at. closes #27 +- the weekly digest reads per-repo settings instead of a global copy. it was passed `DEFAULT_REPO_CONFIG.similarityThreshold` and clustered *every* repo at the default, ignoring whatever that repo had configured, so its cluster counts disagreed with the backlog scan's for the same repo. `WeeklyDigestConfig` loses `similarityThreshold` and `autoClose`: the first now comes from the repo, the second was already dead +- incident windows now work on the server/GitHub-App path, not just the CLI. declare them per repo under `incidents` in `{dataDir}/{owner}-{repo}/config.json`. a readable config carrying a malformed window throws on load rather than quietly loading as "no incident", which would rank the affected backlog as rejected. a backlog scan fetches closed items only for repos that declare a window: a window matches on `closedAt`, so without one the extra API calls buy nothing. part of #27 +- the webhook path no longer overwrites what a scan learned. `server/triage.ts` wrote `metadata: { author, state }` wholesale, and because `upsert` replaces `metadata_json` outright, a `pull_request.opened` webhook draining after a backlog scan dropped every other field the scan had stored (labels, diff size, ci status, closing refs, `authorIsBot`). it now writes only the fields the event can actually observe and leaves the rest of the row alone. field names come from the shared `itemMetadata()`, and a name that is not part of it throws rather than silently creating a key nothing reads. part of #27 - bot-authored items are excluded from clustering by default. automation reuses titles for unrelated content - dependabot files "chore(deps): bump the npm group with 2 updates" week after week - so consecutive bot PRs embed as near-identical and surfaced as duplicates nobody could act on. measured on odysseus-dev/odysseus: every false positive among the extra clusters a high-recall model found was a recurring bot PR, and filtering removed exactly those two clusters (39 -> 37) while leaving all 11 genuine duplicates. set `cluster.include_bot_authors: true` to restore the old behaviour, or list repo-specific automation under `cluster.bot_authors` when a self-hosted bot is not in the built-in list. applies to confirmed (identity) clusters too, not just fuzzy ones - confirmed (identity) duplicate clusters now pick canonical by earliest-created instead of quality score: byte-identical duplicates are a which-was-first question, and a copied PR can outscore the original it was lifted from. fuzzy clusters keep the state/CI/score rule. starmap `canonical`/`contested` for confirmed clusters shift accordingly (value change, schema stays v1) @@ -14,7 +18,7 @@ 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 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 +- 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. the server/GitHub-App path honours windows too, declared per repo in that repo's `config.json` (#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` 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 diff --git a/server/__tests__/config.test.ts b/server/__tests__/config.test.ts index 243d2de..72b3543 100644 --- a/server/__tests__/config.test.ts +++ b/server/__tests__/config.test.ts @@ -1,14 +1,15 @@ -import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { RepoConfig } from "../config.js"; import { DEFAULT_REPO_CONFIG, loadRepoConfig, + loadRepoConfigIsolated, loadServerConfig, saveRepoConfig, } from "../config.js"; -import type { RepoConfig } from "../config.js"; describe("loadServerConfig", () => { const originalEnv = { ...process.env }; @@ -105,6 +106,8 @@ describe("loadRepoConfig", () => { it("reads config from disk", () => { const custom: RepoConfig = { + incidents: [], + cluster: { includeBotAuthors: false, botAuthors: [] }, autoClose: true, autoCloseThreshold: 0.9, similarityThreshold: 0.75, @@ -123,11 +126,7 @@ describe("loadRepoConfig", () => { const dir = join(dataDir, "octocat-my-repo"); const { mkdirSync, writeFileSync } = require("node:fs"); mkdirSync(dir, { recursive: true }); - writeFileSync( - join(dir, "config.json"), - JSON.stringify({ autoClose: true }), - "utf-8", - ); + writeFileSync(join(dir, "config.json"), JSON.stringify({ autoClose: true }), "utf-8"); const loaded = loadRepoConfig(dataDir, "octocat", "my-repo"); @@ -199,6 +198,8 @@ describe("saveRepoConfig", () => { it("roundtrips correctly", () => { const custom: RepoConfig = { + incidents: [], + cluster: { includeBotAuthors: false, botAuthors: [] }, autoClose: true, autoCloseThreshold: 0.92, similarityThreshold: 0.7, @@ -222,3 +223,194 @@ describe("DEFAULT_REPO_CONFIG", () => { expect(DEFAULT_REPO_CONFIG.smartRouting).toBe(true); }); }); + +describe("repo incident windows", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-repo-incidents-")); + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + function write(config: unknown) { + const dir = join(dataDir, "octocat-my-repo"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "config.json"), JSON.stringify(config)); + } + + it("defaults to no windows", () => { + expect(loadRepoConfig(dataDir, "octocat", "my-repo").incidents).toEqual([]); + }); + + it("loads configured windows", () => { + write({ + incidents: [{ start: "2026-07-23T09:00:00Z", end: "2026-07-23T11:00:00Z", reason: "visibility flip" }], + }); + const cfg = loadRepoConfig(dataDir, "octocat", "my-repo"); + expect(cfg.incidents).toHaveLength(1); + expect(cfg.incidents?.[0].reason).toBe("visibility flip"); + }); + + it("throws on a malformed window instead of falling back to none", () => { + // The catch-all fallback is fine for a corrupt file, but a readable config + // with a bad window would silently rank an incident backlog as rejected, + // which is the bug this feature exists to fix. + write({ incidents: [{ start: "nonsense", end: "2026-07-23T11:00:00Z", reason: "typo" }] }); + expect(() => loadRepoConfig(dataDir, "octocat", "my-repo")).toThrow(/typo/); + }); + + it("throws on an inverted window", () => { + write({ + incidents: [{ start: "2026-07-23T11:00:00Z", end: "2026-07-23T09:00:00Z", reason: "backwards" }], + }); + expect(() => loadRepoConfig(dataDir, "octocat", "my-repo")).toThrow(/after its start/); + }); + + it("still falls back to defaults for an unreadable file", () => { + const dir = join(dataDir, "octocat-my-repo"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "config.json"), "{ not json"); + expect(loadRepoConfig(dataDir, "octocat", "my-repo")).toEqual(DEFAULT_REPO_CONFIG); + }); +}); + +describe("repo cluster config", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-repo-cluster-")); + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + function write(config: unknown) { + const dir = join(dataDir, "octocat-my-repo"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "config.json"), JSON.stringify(config)); + } + + it("defaults to excluding bots with no extra logins", () => { + const cfg = loadRepoConfig(dataDir, "octocat", "my-repo"); + expect(cfg.cluster.includeBotAuthors).toBe(false); + expect(cfg.cluster.botAuthors).toEqual([]); + }); + + it("loads repo-specific bot logins", () => { + write({ cluster: { botAuthors: ["acme-ci"] } }); + const cfg = loadRepoConfig(dataDir, "octocat", "my-repo"); + expect(cfg.cluster.botAuthors).toEqual(["acme-ci"]); + // merged with the default, not replacing the whole block + expect(cfg.cluster.includeBotAuthors).toBe(false); + }); + + it("can opt bots back in", () => { + write({ cluster: { includeBotAuthors: true } }); + expect(loadRepoConfig(dataDir, "octocat", "my-repo").cluster.includeBotAuthors).toBe(true); + }); +}); + +describe("repo config hostile input", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-repo-hostile-")); + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + function write(config: unknown) { + const dir = join(dataDir, "octocat-my-repo"); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "config.json"), JSON.stringify(config)); + } + + it("rejects a botAuthors that is not a list of strings", () => { + // config.json is hand-editable. Without a check this reaches + // `new Set(42)` inside the scheduler, far from the file that caused it. + write({ cluster: { botAuthors: 42 } }); + expect(() => loadRepoConfig(dataDir, "octocat", "my-repo")).toThrow(/botAuthors/); + }); + + it("rejects a non-boolean includeBotAuthors", () => { + write({ cluster: { includeBotAuthors: "yes" } }); + expect(() => loadRepoConfig(dataDir, "octocat", "my-repo")).toThrow(/includeBotAuthors/); + }); + + it("rejects incidents that is not a list", () => { + write({ incidents: "yes" }); + expect(() => loadRepoConfig(dataDir, "octocat", "my-repo")).toThrow(/incidents/); + }); +}); + +describe("loadRepoConfigIsolated", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-isolated-")); + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + function write(owner: string, repo: string, config: unknown) { + const dir = join(dataDir, `${owner}-${repo}`); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "config.json"), JSON.stringify(config)); + } + + it("returns the config for a good repo", () => { + write("octocat", "good", { similarityThreshold: 0.7 }); + expect(loadRepoConfigIsolated(dataDir, "octocat", "good")?.similarityThreshold).toBe(0.7); + }); + + it("returns null and names the repo instead of throwing", () => { + // One repo's hand-edited config must not be able to abort a loop over every + // other repo in the installation. The throw is still loud, just contained. + write("octocat", "bad", { incidents: [{ start: "nonsense", end: "2026-07-24T00:00:00Z", reason: "typo" }] }); + const err = vi.spyOn(console, "error").mockImplementation(() => {}); + const result = loadRepoConfigIsolated(dataDir, "octocat", "bad"); + const logged = err.mock.calls.map((c) => c.join(" ")).join("\n"); + err.mockRestore(); + expect(result).toBeNull(); + expect(logged).toMatch(/octocat\/bad/); + expect(logged).toMatch(/typo/); + }); + + it("does not fall back to defaults, which would hide the misconfiguration", () => { + write("octocat", "bad", { incidents: "yes" }); + const err = vi.spyOn(console, "error").mockImplementation(() => {}); + const result = loadRepoConfigIsolated(dataDir, "octocat", "bad"); + err.mockRestore(); + expect(result).toBeNull(); + }); +}); + +describe("repo cluster must be an object", () => { + it("rejects a string, which would otherwise spread its character indices", () => { + const dir = mkdtempSync(join(tmpdir(), "prism-cluster-str-")); + const d = join(dir, "octocat-my-repo"); + mkdirSync(d, { recursive: true }); + writeFileSync(join(d, "config.json"), JSON.stringify({ cluster: "acme" })); + try { + expect(() => loadRepoConfig(dir, "octocat", "my-repo")).toThrow(/cluster must be an object/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/server/__tests__/scheduler.test.ts b/server/__tests__/scheduler.test.ts index a5a8a8a..b1d2f83 100644 --- a/server/__tests__/scheduler.test.ts +++ b/server/__tests__/scheduler.test.ts @@ -1,11 +1,11 @@ -import { mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; -import { formatWeeklyDigest, getLastMonday } from "../scheduler.js"; -import type { RepoDigestData } from "../scheduler.js"; -import { getItemCountSince, listInstalledRepos, openRepoDB } from "../db.js"; import type { Cluster, ScoredPR } from "../../src/types.js"; +import { getItemCountSince, listInstalledRepos, openRepoDB } from "../db.js"; +import type { RepoDigestData } from "../scheduler.js"; +import { backlogFetchSince, backlogFetchesClosed, formatWeeklyDigest, getLastMonday } from "../scheduler.js"; function makeScoredPR(number: number, title: string): ScoredPR { return { @@ -328,3 +328,35 @@ describe("getItemCountSince", () => { expect(count3).toBe(0); }); }); + +describe("backlogFetchesClosed", () => { + it("is false when no incident is declared", () => { + expect(backlogFetchesClosed([])).toBe(false); + expect(backlogFetchesClosed(undefined)).toBe(false); + }); + + it("is true once a window exists", () => { + // A window matches on closedAt, which a scan can only record for items it + // actually fetched. Fetching open-only would make every window match zero. + expect(backlogFetchesClosed([{ start: "a", end: "b", reason: "c" }])).toBe(true); + }); +}); + +describe("backlogFetchSince", () => { + it("is undefined when no incident is declared", () => { + expect(backlogFetchSince([])).toBeUndefined(); + expect(backlogFetchSince(undefined)).toBeUndefined(); + }); + + it("is the earliest window start, so closed items outside every window are not fetched", () => { + // Without this, one window flips the scan to every closed item the repo has + // ever had. An item closed by an incident was updated at or after that + // window opened, so the earliest start is a safe lower bound. + expect( + backlogFetchSince([ + { start: "2026-07-23T09:00:00Z", end: "2026-07-23T11:00:00Z", reason: "b" }, + { start: "2026-05-01T00:00:00Z", end: "2026-05-02T00:00:00Z", reason: "a" }, + ]), + ).toBe("2026-05-01T00:00:00Z"); + }); +}); diff --git a/server/__tests__/triage.test.ts b/server/__tests__/triage.test.ts index 851fe9b..267db5a 100644 --- a/server/__tests__/triage.test.ts +++ b/server/__tests__/triage.test.ts @@ -2,11 +2,10 @@ import { mkdtempSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import type { WebhookEvent } from "../webhook.js"; -import { openRepoDB } from "../db.js"; -import { setRepoScanning, isRepoScanning, drainWebhookQueue } from "../db.js"; -import { formatTriageComment, formatAutoCloseComment } from "../format.js"; +import { drainWebhookQueue, isRepoScanning, openRepoDB, setRepoScanning } from "../db.js"; import type { DupeMatch } from "../format.js"; +import { formatAutoCloseComment, formatTriageComment } from "../format.js"; +import type { WebhookEvent } from "../webhook.js"; // --- mock createEmbeddingProvider so we never hit a real API --- @@ -33,12 +32,10 @@ vi.mock("../../src/embeddings.js", () => ({ embedBatch: vi.fn(async (texts: string[]) => texts.map(() => mockEmbedResult)), dimensions: DIMS, })), - prepareEmbeddingText: vi.fn( - (item: { title: string; body: string; type: string }) => { - const prefix = item.type === "pr" ? "Pull Request" : "Issue"; - return `${prefix}: ${item.title}\n\n${item.body}`; - }, - ), + prepareEmbeddingText: vi.fn((item: { title: string; body: string; type: string }) => { + const prefix = item.type === "pr" ? "Pull Request" : "Issue"; + return `${prefix}: ${item.title}\n\n${item.body}`; + }), })); // --- helpers --- @@ -326,3 +323,255 @@ describe("formatAutoCloseComment", () => { expect(result).toContain("auto-closed by"); }); }); + +describe("triageNewItem stored metadata", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-triage-meta-")); + setRepoScanning("octocat", "my-repo", false); + mockEmbedResult = VEC_UNRELATED; + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + const event = (over: Partial = {}): WebhookEvent => ({ + action: "opened", + eventType: "pull_request", + number: 77, + title: "a new pr", + body: "body", + repo: { owner: "octocat", name: "my-repo", fullName: "octocat/my-repo" }, + sender: "contributor", + ...over, + }); + + const cfg = () => ({ + dataDir, + jinaApiKey: "fake-key", + similarityThreshold: 0.8, + autoClose: false, + autoCloseThreshold: 0.95, + }); + + it("names its fields the way the scan path does", async () => { + const { triageNewItem } = await import("../triage.js"); + const { itemMetadata } = await import("../../src/metadata.js"); + await triageNewItem(event(), cfg() as never); + + const store = openRepoDB(dataDir, "octocat", "my-repo", DIMS, "jina-embeddings-v3"); + const stored = (store.getByNumber("octocat/my-repo", 77)?.metadata ?? {}) as Record; + store.close(); + + const scanKeys = new Set( + Object.keys( + itemMetadata({ + number: 77, + type: "pr", + repo: "octocat/my-repo", + title: "a new pr", + body: "body", + state: "open", + author: "contributor", + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + labels: [], + }), + ), + ); + // Not every key: the webhook cannot observe labels or CI. But no key it + // does write may be one the scan path has never heard of. + for (const key of Object.keys(stored)) { + expect(scanKeys.has(key), `"${key}" is not a field itemMetadata writes`).toBe(true); + } + expect(stored.author).toBe("contributor"); + expect(stored.state).toBe("open"); + expect(stored.bodyLength).toBe("body".length); + }); + + it("does not erase fields a previous scan stored", async () => { + // A webhook arriving after a backlog scan used to overwrite metadata_json + // wholesale, dropping everything the scan had learned about the item. + const store = openRepoDB(dataDir, "octocat", "my-repo", DIMS, "jina-embeddings-v3"); + store.upsert({ + id: "octocat/my-repo:pr:77", + type: "pr", + number: 77, + repo: "octocat/my-repo", + title: "a new pr", + bodySnippet: "", + embedding: new Float32Array(VEC_UNRELATED), + metadata: { + author: "contributor", + authorIsBot: false, + state: "open", + labels: ["bug"], + additions: 120, + ciStatus: "success", + closesIssues: [12], + }, + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + }); + store.close(); + + const { triageNewItem } = await import("../triage.js"); + await triageNewItem(event(), cfg() as never); + + const after = openRepoDB(dataDir, "octocat", "my-repo", DIMS, "jina-embeddings-v3"); + const stored = (after.getByNumber("octocat/my-repo", 77)?.metadata ?? {}) as Record; + after.close(); + + expect(stored.additions).toBe(120); + expect(stored.ciStatus).toBe("success"); + expect(stored.closesIssues).toEqual([12]); + expect(stored.labels).toEqual(["bug"]); + }); +}); + +describe("triageNewItem bot filtering", () => { + let dataDir: string; + + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-triage-bots-")); + setRepoScanning("octocat", "my-repo", false); + mockEmbedResult = VEC_A; + }); + + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + const event = (over: Partial = {}): WebhookEvent => ({ + action: "opened", + eventType: "pull_request", + number: 90, + title: "chore(deps): bump the npm group with 2 updates", + body: "bumps things", + repo: { owner: "octocat", name: "my-repo", fullName: "octocat/my-repo" }, + sender: "human-dev", + ...over, + }); + + const cfg = (over: Record = {}) => ({ + dataDir, + jinaApiKey: "fake-key", + similarityThreshold: 0.8, + autoClose: false, + autoCloseThreshold: 0.95, + cluster: { includeBotAuthors: false, botAuthors: [] }, + ...over, + }); + + function seedBot(number: number) { + const store = openRepoDB(dataDir, "octocat", "my-repo", DIMS, "jina-embeddings-v3"); + store.upsert({ + id: `octocat/my-repo:pr:${number}`, + type: "pr", + number, + repo: "octocat/my-repo", + title: "chore(deps): bump the npm group with 2 updates", + bodySnippet: "", + embedding: new Float32Array(VEC_B), + metadata: { author: "dependabot[bot]", authorIsBot: true, state: "open" }, + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + }); + store.close(); + } + + it("does not comment when the incoming item is bot-authored", async () => { + // dependabot reuses titles, so consecutive bot PRs read as near-identical. + // #26 removed that noise from clustering; posting it as a webhook comment + // on someone's repository is the same noise, just louder. + seedBot(80); + const { triageNewItem } = await import("../triage.js"); + const result = await triageNewItem(event({ sender: "dependabot[bot]" }), cfg() as never, async () => {}); + expect(result.commented).toBe(false); + expect(result.matches).toHaveLength(0); + }); + + it("does not offer a bot-authored item as a duplicate of a human PR", async () => { + seedBot(80); + const { triageNewItem } = await import("../triage.js"); + const result = await triageNewItem(event(), cfg() as never, async () => {}); + expect(result.matches.map((m) => m.number)).not.toContain(80); + }); + + it("still triages bots when the repo opts in", async () => { + seedBot(80); + const { triageNewItem } = await import("../triage.js"); + const result = await triageNewItem( + event({ sender: "dependabot[bot]" }), + cfg({ cluster: { includeBotAuthors: true, botAuthors: [] } }) as never, + async () => {}, + ); + expect(result.matches.map((m) => m.number)).toContain(80); + }); + + it("honours repo-specific bot logins", async () => { + seedBot(80); + const { triageNewItem } = await import("../triage.js"); + const result = await triageNewItem( + event({ sender: "acme-ci" }), + cfg({ cluster: { includeBotAuthors: false, botAuthors: ["acme-ci"] } }) as never, + async () => {}, + ); + expect(result.commented).toBe(false); + }); +}); + +describe("triage bot items are still indexed", () => { + let dataDir: string; + beforeEach(() => { + dataDir = mkdtempSync(join(tmpdir(), "prism-triage-index-")); + setRepoScanning("octocat", "my-repo", false); + mockEmbedResult = VEC_A; + }); + afterEach(() => { + try { + rmSync(dataDir, { recursive: true, force: true }); + } catch {} + }); + + it("stores a bot-authored item even though it does not comment on it", async () => { + // The backlog scan stores every item and filters at cluster time. If the + // webhook path dropped bot items instead, the database would depend on + // which path saw the item, and flipping includeBotAuthors on would need a + // full rescan to become true. + const { triageNewItem } = await import("../triage.js"); + const result = await triageNewItem( + { + action: "opened", + eventType: "pull_request", + number: 91, + title: "chore(deps): bump", + body: "b", + repo: { owner: "octocat", name: "my-repo", fullName: "octocat/my-repo" }, + sender: "dependabot[bot]", + } as WebhookEvent, + { + dataDir, + jinaApiKey: "fake-key", + similarityThreshold: 0.8, + autoClose: false, + autoCloseThreshold: 0.95, + cluster: { includeBotAuthors: false, botAuthors: [] }, + } as never, + async () => {}, + ); + expect(result.commented).toBe(false); + + const store = openRepoDB(dataDir, "octocat", "my-repo", DIMS, "jina-embeddings-v3"); + const stored = store.getByNumber("octocat/my-repo", 91); + store.close(); + expect(stored).toBeDefined(); + expect(stored?.metadata?.author).toBe("dependabot[bot]"); + }); +}); diff --git a/server/config.ts b/server/config.ts index 655fa51..dcac23d 100644 --- a/server/config.ts +++ b/server/config.ts @@ -1,5 +1,6 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import { compileIncidentWindows, type IncidentWindow } from "../src/incident.js"; export interface ServerConfig { port: number; @@ -10,7 +11,19 @@ export interface ServerConfig { dataDir: string; } +/** Mirrors the CLI's `cluster:` block, in this file's camelCase. */ +export interface RepoClusterConfig { + /** Cluster bot-authored items too. Off by default; see src/bots.ts. */ + includeBotAuthors: boolean; + /** Extra bot logins for this repo, added to the built-in list. */ + botAuthors: string[]; +} + export interface RepoConfig { + /** Repository-wide events that closed items for reasons unrelated to their + * quality. PRs closed inside a window rank as open. See src/incident.ts. */ + incidents: IncidentWindow[]; + cluster: RepoClusterConfig; autoClose: boolean; autoCloseThreshold: number; similarityThreshold: number; @@ -19,6 +32,8 @@ export interface RepoConfig { } export const DEFAULT_REPO_CONFIG: RepoConfig = { + incidents: [], + cluster: { includeBotAuthors: false, botAuthors: [] }, autoClose: false, autoCloseThreshold: 0.95, similarityThreshold: 0.85, @@ -46,9 +61,7 @@ export function loadServerConfig(): ServerConfig { if (!jinaApiKey) missing.push("JINA_API_KEY"); if (missing.length > 0) { - throw new Error( - `missing required environment variables: ${missing.join(", ")}`, - ); + throw new Error(`missing required environment variables: ${missing.join(", ")}`); } return { @@ -65,42 +78,91 @@ export function loadServerConfig(): ServerConfig { * Load per-repo configuration from {dataDir}/{owner}-{repo}/config.json. * Returns DEFAULT_REPO_CONFIG if the file doesn't exist. */ -export function loadRepoConfig( - dataDir: string, - owner: string, - repo: string, -): RepoConfig { +export function loadRepoConfig(dataDir: string, owner: string, repo: string): RepoConfig { const configPath = join(dataDir, `${owner}-${repo}`, "config.json"); if (!existsSync(configPath)) { return { ...DEFAULT_REPO_CONFIG }; } + let parsed: Partial; try { - const raw = readFileSync(configPath, "utf-8"); - const parsed = JSON.parse(raw) as Partial; - - // merge with defaults so any missing keys get sane values - return { - ...DEFAULT_REPO_CONFIG, - ...parsed, - }; + parsed = JSON.parse(readFileSync(configPath, "utf-8")) as Partial; } catch { - // corrupted or unreadable config — fall back to defaults + // corrupted or unreadable config - fall back to defaults return { ...DEFAULT_REPO_CONFIG }; } + + // merge with defaults so any missing keys get sane values. `cluster` is + // merged a level deeper: a config setting only `botAuthors` should keep the + // default `includeBotAuthors`, not lose it to a wholesale block replacement. + const merged = { + ...DEFAULT_REPO_CONFIG, + ...parsed, + cluster: { ...DEFAULT_REPO_CONFIG.cluster, ...(parsed.cluster ?? {}) }, + }; + + // Everything below is deliberately outside the catch above. A file we could + // not read at all is one thing; a readable one declaring settings we cannot + // honour is another. Silently defaulting those would rank an incident backlog + // as rejected, or ignore a repo's bot logins, which are the exact failures + // these settings exist to prevent. + // + // The cast to Partial above is a claim about a hand-editable file, + // not a fact, so the shapes that reach other modules are checked here rather + // than failing later as `new Set(42)` somewhere in the scheduler. + if (parsed.cluster !== undefined && (typeof parsed.cluster !== "object" || parsed.cluster === null || Array.isArray(parsed.cluster))) { + throw new Error(`${configPath}: cluster must be an object`); + } + if (!Array.isArray(merged.incidents)) { + throw new Error(`${configPath}: incidents must be a list of {start, end, reason} objects`); + } + if (typeof merged.cluster.includeBotAuthors !== "boolean") { + throw new Error(`${configPath}: cluster.includeBotAuthors must be true or false`); + } + if ( + !Array.isArray(merged.cluster.botAuthors) || + merged.cluster.botAuthors.some((login) => typeof login !== "string" || login.trim() === "") + ) { + throw new Error(`${configPath}: cluster.botAuthors must be a list of non-empty login strings`); + } + // Compiled here for its validation, and the result discarded on purpose: the + // store compiles its own copy from the same raw windows. Bounds are cheap to + // parse and the alternative is threading a second type through openRepoDB. + compileIncidentWindows(merged.incidents); + return merged; +} + +/** + * loadRepoConfig, contained to one repository. + * + * loadRepoConfig throws on a config it cannot honour, which is right: silently + * defaulting would rank an incident backlog as rejected or ignore a repo's bot + * logins. But the App iterates every repo in an installation, and one repo's + * hand-edited file must not abort that loop and silently skip the repos after + * it - a failure that repeats identically on every webhook redelivery. + * + * Returns null rather than defaults. A caller that skips the repo loudly is + * recoverable; one that proceeds on defaults has the misconfiguration hidden + * from it, which is what throwing was meant to prevent. + */ +export function loadRepoConfigIsolated(dataDir: string, owner: string, repo: string): RepoConfig | null { + try { + return loadRepoConfig(dataDir, owner, repo); + } catch (err) { + console.error( + `[config] ${owner}/${repo}: unusable config.json, this repo is skipped:`, + err instanceof Error ? err.message : err, + ); + return null; + } } /** * Save per-repo configuration to {dataDir}/{owner}-{repo}/config.json. * Creates the directory if it doesn't exist. */ -export function saveRepoConfig( - dataDir: string, - owner: string, - repo: string, - config: RepoConfig, -): void { +export function saveRepoConfig(dataDir: string, owner: string, repo: string, config: RepoConfig): void { const dir = join(dataDir, `${owner}-${repo}`); mkdirSync(dir, { recursive: true }); diff --git a/server/db.ts b/server/db.ts index 0f1dfaf..5dd295d 100644 --- a/server/db.ts +++ b/server/db.ts @@ -1,6 +1,7 @@ import { existsSync, readdirSync } from "node:fs"; import { join } from "node:path"; import Database from "better-sqlite3"; +import type { IncidentWindow } from "../src/incident.js"; import { VectorStore } from "../src/store.js"; export interface RepoStatus { @@ -36,16 +37,15 @@ export function openRepoDB( repo: string, dimensions?: number, model?: string, + /** Repository-wide close events from this repo's config.json. Omitted by + * callers that do not rank, since the flag only affects ranking. */ + incidentWindows: readonly IncidentWindow[] = [], ): 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 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); + // Incident windows are the store's business because the flag is derived at + // hydration. The `cluster` block is not: it reaches findDuplicateClusters and + // the triage matcher directly from each repo's config.json, not through here. + return new VectorStore(dbPath, dimensions, model, { incidentWindows }); } /** Returns item count, embedding count, and last sync time for a repo DB. */ @@ -103,7 +103,8 @@ export function getItemCountSince(dataDir: string, owner: string, repo: string, const db = new Database(dbPath, { readonly: true }); try { - const row = db.prepare("SELECT COUNT(*) as c FROM items WHERE repo = ? AND created_at >= ?") + const row = db + .prepare("SELECT COUNT(*) as c FROM items WHERE repo = ? AND created_at >= ?") .get(`${owner}/${repo}`, since) as { c: number } | undefined; return row?.c ?? 0; } finally { diff --git a/server/index.ts b/server/index.ts index 9e83899..5cbd950 100644 --- a/server/index.ts +++ b/server/index.ts @@ -1,7 +1,7 @@ import { readFileSync } from "node:fs"; import { serve } from "@hono/node-server"; import { Hono } from "hono"; -import { loadServerConfig, loadRepoConfig, DEFAULT_REPO_CONFIG } from "./config.js"; +import { loadRepoConfig, loadRepoConfigIsolated, loadServerConfig } from "./config.js"; import { parseWebhookEvent, verifyWebhookSignature } from "./webhook.js"; import { triageNewItem } from "./triage.js"; import type { TriageConfig } from "./triage.js"; @@ -29,14 +29,16 @@ const app = new Hono(); * Build a TriageConfig for a specific repo by merging server-level * settings with per-repo settings from config.json. */ -function triageConfigFor(owner: string, repo: string): TriageConfig { - const repoConfig = loadRepoConfig(serverConfig.dataDir, owner, repo); +function triageConfigFor(owner: string, repo: string): TriageConfig | null { + const repoConfig = loadRepoConfigIsolated(serverConfig.dataDir, owner, repo); + if (!repoConfig) return null; return { dataDir: serverConfig.dataDir, jinaApiKey: serverConfig.jinaApiKey, similarityThreshold: repoConfig.similarityThreshold, autoClose: repoConfig.autoClose, autoCloseThreshold: repoConfig.autoCloseThreshold, + cluster: repoConfig.cluster, }; } @@ -209,12 +211,17 @@ app.post("/webhook", async (c) => { // kick off backlog scans in the background (don't block webhook response) for (const { owner, repo, fullName } of installRepos) { - const repoConfig = loadRepoConfig(serverConfig.dataDir, owner, repo); + // Contained per repo: one unusable config.json must not abort the loop + // and silently skip every repo listed after it, on every redelivery. + const repoConfig = loadRepoConfigIsolated(serverConfig.dataDir, owner, repo); + if (!repoConfig) continue; const backlogConfig: BacklogScanConfig = { dataDir: serverConfig.dataDir, jinaApiKey: serverConfig.jinaApiKey, githubToken: installToken, similarityThreshold: repoConfig.similarityThreshold, + incidents: repoConfig.incidents, + cluster: repoConfig.cluster, }; runBacklogScan(owner, repo, backlogConfig, callbacks.postIssue, { @@ -256,7 +263,10 @@ app.post("/webhook", async (c) => { // triage in the background so the webhook responds quickly const triageConfig = triageConfigFor(event.repo.owner, event.repo.name); - if (triageConfig.jinaApiKey) { + // A null config means this repo's config.json is unusable; loadRepoConfigIsolated + // has already logged which repo and why. Acknowledge the delivery rather than + // 500ing, since GitHub would redeliver into the same deterministic failure. + if (triageConfig && triageConfig.jinaApiKey) { triageNewItem(event, triageConfig, callbacks.postComment, callbacks.closeIssue, callbacks.fetchFileContent) .then((result) => { if (result.commented) { @@ -283,11 +293,7 @@ app.post("/webhook", async (c) => { // weekly digest doesn't have an installation context at cron time, // so we log a warning instead of posting if no installation is available startWeeklyDigest( - { - dataDir: serverConfig.dataDir, - similarityThreshold: DEFAULT_REPO_CONFIG.similarityThreshold, - autoClose: DEFAULT_REPO_CONFIG.autoClose, - }, + { dataDir: serverConfig.dataDir }, async (fullName: string, title: string, body: string): Promise => { // the weekly digest fires on a cron, not from a webhook, so we don't have // an installation ID in context. we need to look it up from cached tokens diff --git a/server/scheduler.ts b/server/scheduler.ts index 5559a64..fabaa4c 100644 --- a/server/scheduler.ts +++ b/server/scheduler.ts @@ -2,6 +2,8 @@ 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 type { IncidentWindow } from "../src/incident.js"; +import { loadRepoConfig, type RepoClusterConfig } from "./config.js"; import { itemMetadata } from "../src/metadata.js"; import { escapeTableCell } from "../src/sanitize.js"; import type { Cluster, PRItem, StoreItem } from "../src/types.js"; @@ -22,6 +24,41 @@ export interface BacklogScanConfig { jinaApiKey: string; githubToken: string; similarityThreshold: number; + /** Repository-wide close events from this repo's config.json. When any are + * declared the scan also fetches closed items, since a window can only match + * against a `closedAt` the scan actually captured. */ + incidents?: IncidentWindow[]; + /** This repo's `cluster` block. Without it the App would honour only the + * built-in bot list while the CLI honoured the repo's own additions. */ + cluster?: RepoClusterConfig; +} + +/** + * Whether a backlog scan fetches closed items in addition to open ones. + * + * Closed items cost API calls and only earn them when a window could match + * one: an incident window tests `closedAt`, which is absent unless the scan + * captured it. A repo with no incidents declared fetches open only. + */ +export function backlogFetchesClosed(incidents: readonly unknown[] | undefined): boolean { + return Boolean(incidents && incidents.length > 0); +} + +/** + * The lower bound for a backlog scan's fetch. + * + * backlogFetchState only decides open-vs-all. Without a bound as well, a single + * declared window flips the scan to every closed item the repository has ever + * had, which for a large repo is most of its history. An item closed by an + * incident was necessarily updated at or after that window opened, so the + * earliest window start is a safe lower bound: GitHub filters on updated_at, + * which for these items is the close itself or later. + */ +export function backlogFetchSince( + incidents: readonly { start: string }[] | undefined, +): string | undefined { + if (!incidents || incidents.length === 0) return undefined; + return incidents.reduce((earliest, w) => (Date.parse(w.start) < Date.parse(earliest) ? w.start : earliest), incidents[0].start); } /** @@ -33,7 +70,7 @@ function formatTriageReport(owner: string, repo: string, totalItems: number, clu const totalDupes = clusters.reduce((s, c) => s + c.items.length, 0); let body = `## summary\n\n`; - body += `- **${totalItems}** open issues and PRs scanned\n`; + body += `- **${totalItems}** issues and PRs scanned\n`; body += `- **${clusters.length}** duplicate clusters found\n`; body += `- **${totalDupes}** items involved in duplicates\n\n`; @@ -106,24 +143,38 @@ export async function runBacklogScan( }); const dims = embedder.dimensions; - const store = openRepoDB(config.dataDir, owner, repo, dims, "jina-embeddings-v3"); + const incidents = config.incidents ?? []; + const store = openRepoDB(config.dataDir, owner, repo, dims, "jina-embeddings-v3", incidents); try { - // 2. fetch all open PRs + issues via REST (avoids 502s on large repos) + // 2. fetch PRs + issues via REST (avoids 502s on large repos) const github = new GitHubClient(config.githubToken, owner, repo); + // Closed items are only worth their API cost when a window could match + // one. A repo with no incidents declared keeps fetching open items only. + // Open and closed are fetched separately on purpose. `since` aborts a + // fetch at the first item updated before it, and the lists are sorted by + // updated desc, so a single state:"all" call bounded by `since` would + // also drop open items nobody has touched lately - exactly the stale + // duplicates a backlog scan exists to find. Open is therefore unbounded, + // and only the closed pass carries the window bound. console.log(`[backlog] ${fullName}: fetching open PRs...`); const prs = await github.fetchPRs({ state: "open", maxItems: 5000, batchSize: 100 }); - console.log(`[backlog] ${fullName}: fetched ${prs.length} open PRs`); - console.log(`[backlog] ${fullName}: fetching open issues...`); const issues = await github.fetchIssues({ state: "open", maxItems: 5000, batchSize: 100 }); - console.log(`[backlog] ${fullName}: fetched ${issues.length} open issues`); + + if (backlogFetchesClosed(incidents)) { + const since = backlogFetchSince(incidents); + console.log(`[backlog] ${fullName}: fetching closed items updated since ${since}...`); + prs.push(...(await github.fetchPRs({ state: "closed", since, maxItems: 5000, batchSize: 100 }))); + issues.push(...(await github.fetchIssues({ state: "closed", since, maxItems: 5000, batchSize: 100 }))); + } + console.log(`[backlog] ${fullName}: fetched ${prs.length} PRs, ${issues.length} issues`); const allItems: PRItem[] = [...prs, ...issues]; if (allItems.length === 0) { - console.log(`[backlog] ${fullName}: no open items found, skipping`); + console.log(`[backlog] ${fullName}: no items found, skipping`); store.setMeta("last_sync", new Date().toISOString()); return; } @@ -186,6 +237,8 @@ export async function runBacklogScan( const clusters = findDuplicateClusters(store, items, { threshold: config.similarityThreshold, repo: fullName, + includeBotAuthors: config.cluster?.includeBotAuthors, + botAuthors: new Set(config.cluster?.botAuthors ?? []), }); console.log(`[backlog] ${fullName}: found ${clusters.length} duplicate clusters`); @@ -217,6 +270,10 @@ export async function runBacklogScan( similarityThreshold: config.similarityThreshold, autoClose: false, autoCloseThreshold: 0.95, + // Drained events are triaged on the same terms as live ones. Omitting + // this would filter bots on the webhook that arrived a second later and + // not on the one that arrived during the scan. + cluster: config.cluster, }; const postComment = @@ -244,10 +301,11 @@ export async function runBacklogScan( // --- weekly digest --- +/** The digest reads per-repo settings itself, so it needs only the location of + * the data directory. A global threshold here would be a lie: it applied to + * every repo regardless of that repo's own configuration. */ export interface WeeklyDigestConfig { dataDir: string; - similarityThreshold: number; - autoClose: boolean; } export interface RepoDigestData { @@ -354,16 +412,23 @@ export function startWeeklyDigest( const status = getRepoStatus(config.dataDir, owner, repo); const newItems = getItemCountSince(config.dataDir, owner, repo, lastMonday); - // run clustering to get current cluster state + // run clustering to get current cluster state. per-repo config is + // loaded here rather than taken from the digest's global config: + // otherwise the digest reports different clusters than the backlog + // scan for the same repo, having applied neither its incident + // windows nor its bot logins. + const repoConfig = loadRepoConfig(config.dataDir, owner, repo); let clusters: Cluster[] = []; try { - const store = openRepoDB(config.dataDir, owner, repo); + const store = openRepoDB(config.dataDir, owner, repo, undefined, undefined, repoConfig.incidents); try { const items = store.getAllItems(fullName) as unknown as PRItem[]; if (items.length > 0) { clusters = findDuplicateClusters(store, items, { - threshold: config.similarityThreshold, + threshold: repoConfig.similarityThreshold, repo: fullName, + includeBotAuthors: repoConfig.cluster.includeBotAuthors, + botAuthors: new Set(repoConfig.cluster.botAuthors), }); } } finally { diff --git a/server/triage.ts b/server/triage.ts index 6966dea..2867a4e 100644 --- a/server/triage.ts +++ b/server/triage.ts @@ -1,12 +1,14 @@ +import { isBotAuthor } from "../src/bots.js"; import { selectCanonical } from "../src/canonical.js"; import { createEmbeddingProvider, prepareEmbeddingText } from "../src/embeddings.js"; +import type { ItemMetadata } from "../src/metadata.js"; import { cosineSimilarity, isZeroVector } from "../src/similarity.js"; import type { StoreItem } from "../src/types.js"; import { isRepoScanning, openRepoDB, queueWebhook } from "./db.js"; -import { formatAutoCloseComment, formatTriageComment } from "./format.js"; import type { DupeMatch } from "./format.js"; -import { suggestOwners } from "./routing.js"; +import { formatAutoCloseComment, formatTriageComment } from "./format.js"; import type { OwnerSuggestion } from "./routing.js"; +import { suggestOwners } from "./routing.js"; import type { WebhookEvent } from "./webhook.js"; export type { DupeMatch } from "./format.js"; @@ -27,6 +29,8 @@ export interface TriageConfig { similarityThreshold: number; autoClose: boolean; autoCloseThreshold: number; + /** This repo's `cluster` block. Absent means the built-in bot list only. */ + cluster?: { includeBotAuthors: boolean; botAuthors: string[] }; } export async function triageNewItem( @@ -39,6 +43,9 @@ export async function triageNewItem( const start = performance.now(); const { owner, name: repoName, fullName: repo } = event.repo; + const botLogins = new Set(config.cluster?.botAuthors ?? []); + const includeBots = config.cluster?.includeBotAuthors ?? false; + const empty: TriageResult = { repo, number: event.number, @@ -56,6 +63,7 @@ export async function triageNewItem( return empty; } + // set up embedding provider (Jina) const embedder = await createEmbeddingProvider({ provider: "jina", @@ -85,20 +93,55 @@ export async function triageNewItem( // upsert the new item into the store const now = new Date().toISOString(); + const id = `${repo}:${itemType}:${event.number}`; + + // A webhook knows only what the event carries. Everything else an item has + // (labels, diff size, CI, closing refs) comes from a scan, so only the + // event's own fields are written and the rest of the stored row is left + // alone: upsert replaces metadata_json wholesale, and a webhook arriving + // after a backlog scan would otherwise drop everything that scan learned. + const existing = (store.getItem(id)?.metadata ?? {}) as Record; + // Only the fields the event can observe. Typed as a subset of what + // itemMetadata produces, so a rename there breaks the build here rather + // than silently writing a key nothing reads. A field the webhook cannot + // see is left as stored: `labels: []` would claim the item has none rather + // than that the webhook did not look. + const observed: Partial = { + author: event.sender, + state: "open", + bodyLength: (event.body || "").length, + }; + const metadata: Record = { ...existing, ...observed }; + const storeItem: StoreItem = { - id: `${repo}:${itemType}:${event.number}`, + id, type: itemType, number: event.number, repo, title: event.title, bodySnippet: (event.body || "").slice(0, 2000), embedding, - metadata: { author: event.sender, state: "open" }, + metadata, createdAt: now, updatedAt: now, }; store.upsert(storeItem); + // Automation reuses titles for unrelated content, so consecutive bot items + // read as near-identical. Clustering already excludes them; commenting + // "this looks like a duplicate" on a bot's PR is that same noise, posted to + // someone's repository. + // + // Deliberately after the upsert, not before it. The backlog scan stores + // every item and filters at cluster time, so bailing earlier would make the + // database depend on which path saw the item, and flipping + // includeBotAuthors on would need a full rescan to become true. The cost is + // one embedding per bot item, which the scan path pays anyway. + if (!includeBots && isBotAuthor({ author: event.sender }, botLogins)) { + empty.elapsedMs = performance.now() - start; + return empty; + } + // get all existing embeddings and items for this repo const allEmbeddings = store.getAllEmbeddings(repo); const allItems = store.getAllItems(repo); @@ -120,6 +163,20 @@ export async function triageNewItem( if (sim >= config.similarityThreshold) { const item = itemMap.get(id); if (!item) continue; + // Same rule on the other side: a bot's PR is not a useful "you + // duplicated this" answer for a human contributor. + if ( + !includeBots && + isBotAuthor( + { + author: (item.metadata?.author as string) ?? "", + authorIsBot: item.metadata?.authorIsBot as boolean | undefined, + }, + botLogins, + ) + ) { + continue; + } matches.push({ number: item.number, @@ -174,11 +231,7 @@ export async function triageNewItem( let closed = false; // auto-close if enabled and top match exceeds threshold - if ( - config.autoClose && - closeIssue && - matches[0].similarity >= config.autoCloseThreshold - ) { + if (config.autoClose && closeIssue && matches[0].similarity >= config.autoCloseThreshold) { const closeComment = formatAutoCloseComment(repo, source, matches[0].similarity); await postComment(repo, event.number, closeComment); await closeIssue(repo, event.number); diff --git a/src/__tests__/incident.test.ts b/src/__tests__/incident.test.ts index c0fe345..7ab372c 100644 --- a/src/__tests__/incident.test.ts +++ b/src/__tests__/incident.test.ts @@ -124,3 +124,41 @@ describe("compileIncidentWindows", () => { expect(w.end).toBe(Date.parse("2026-07-23T11:00:00Z")); }); }); + +describe("compileIncidentWindows offset rule", () => { + // The CLI rejects offset-less bounds through zod before they ever reach + // compileIncidentWindows. The server has no zod layer, so the same string + // reaching the same feature was accepted there and parsed in the host + // timezone - the exact ambiguity the CLI rule exists to prevent. + it("rejects a bound with no UTC offset", () => { + expect(() => + compileIncidentWindows([{ start: "2026-07-23T00:00:00", end: "2026-07-24T00:00:00Z", reason: "x" }]), + ).toThrow(/offset/); + }); + + it("rejects a date-only bound", () => { + expect(() => compileIncidentWindows([{ start: "2026-07-23", end: "2026-07-24T00:00:00Z", reason: "x" }])).toThrow( + /offset/, + ); + }); + + it("rejects the space-separated form", () => { + expect(() => + compileIncidentWindows([{ start: "2026-07-23 00:00:00+00:00", end: "2026-07-24T00:00:00Z", reason: "x" }]), + ).toThrow(/offset/); + }); + + it("accepts Z and numeric offsets", () => { + expect( + compileIncidentWindows([{ start: "2026-07-23T00:00:00Z", end: "2026-07-24T00:00:00Z", reason: "x" }]), + ).toHaveLength(1); + expect( + compileIncidentWindows([{ start: "2026-07-23T00:00:00+02:00", end: "2026-07-24T00:00:00+02:00", reason: "x" }]), + ).toHaveLength(1); + }); + + it("rejects a non-array and a non-object entry with a usable message", () => { + expect(() => compileIncidentWindows("yes" as never)).toThrow(/list/); + expect(() => compileIncidentWindows([null] as never)).toThrow(/object/); + }); +}); diff --git a/src/incident.ts b/src/incident.ts index 344b2d1..b6515af 100644 --- a/src/incident.ts +++ b/src/incident.ts @@ -40,18 +40,44 @@ export interface IncidentClosable { * 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. */ +const ISO_8601_ABSOLUTE = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(?::\d{2}(?:\.\d+)?)?(?:Z|[+-]\d{2}:?\d{2})$/; + +/** + * A bound must be a full ISO-8601 instant carrying an explicit offset. This + * lives here rather than only in the CLI's zod schema because the App path + * reaches this function directly from a hand-edited config.json: the same + * string that the CLI rejects was being accepted there and parsed in the host + * timezone, so one window meant different instants on a laptop and on a server. + */ +function requireAbsoluteInstant(value: unknown, label: string, field: string): number { + if (typeof value !== "string") { + throw new Error(`${label}: ${field} must be a string, got ${typeof value}`); + } + if (!ISO_8601_ABSOLUTE.test(value.trim())) { + throw new Error( + `${label}: ${field} ${JSON.stringify(value)} must be an ISO-8601 instant with an explicit UTC offset (e.g. 2026-07-23T00:00:00Z or +02:00)`, + ); + } + const parsed = Date.parse(value); + if (Number.isNaN(parsed)) { + throw new Error(`${label}: ${field} ${JSON.stringify(value)} is not a parseable ISO-8601 timestamp`); + } + return parsed; +} + export function compileIncidentWindows(windows: readonly IncidentWindow[]): CompiledIncidentWindow[] { - return windows.map((w) => { - const start = Date.parse(w.start); - const end = Date.parse(w.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 (!Array.isArray(windows)) { + throw new Error("incidents must be a list of {start, end, reason} objects"); + } + return windows.map((w, index) => { + if (typeof w !== "object" || w === null) { + throw new Error(`incidents[${index}] must be an object with start, end and reason`); } + // The reason is quoted so the offending entry is findable in a long list; + // the index covers an entry whose reason is itself missing. + const label = w.reason ? `incident window ${JSON.stringify(w.reason)}` : `incidents[${index}]`; + const start = requireAbsoluteInstant(w.start, label, "start"); + const end = requireAbsoluteInstant(w.end, label, "end"); if (end <= start) { throw new Error(`${label}: end must be after its start`); } diff --git a/src/metadata.ts b/src/metadata.ts index 57beb44..58ba230 100644 --- a/src/metadata.ts +++ b/src/metadata.ts @@ -9,7 +9,30 @@ import type { PRItem } from "./types.js"; * 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 { +/** + * The stored-metadata shape. Named so a partial writer (the webhook path, which + * can only observe some fields) is checked against it by the compiler instead + * of asserting the key names at runtime. + */ +export type ItemMetadata = { + author: string; + authorIsBot: boolean | undefined; + state: string; + closedAt: string | undefined; + labels: string[]; + additions: number | undefined; + deletions: number | undefined; + changedFiles: number | undefined; + ciStatus: PRItem["ciStatus"]; + reviewCount: number | undefined; + hasTests: boolean | undefined; + bodyLength: number; + nodeId: string | undefined; + headRefOid: string | undefined; + closesIssues: number[] | undefined; +}; + +export function itemMetadata(item: PRItem): ItemMetadata { return { author: item.author, authorIsBot: item.authorIsBot,