From c2a7895d7365a518d9fea48e9fa737c965c2dc0d Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 00:09:59 -0700 Subject: [PATCH 1/4] fix(hub): show untriaged notes only for the current head A failure note shows only when its head sha is the pull request's current head. The list compares it with the open-pulls listing; a repository whose listing failed keeps its notes, but a note alone never lists a pull request there, so closed pull requests no longer come back. The single pull request read asks the host for that one pull request's head instead of listing every repository. --- .../github-read-access.postgres.test.ts | 2 +- .../github/triage-verdicts.postgres.test.ts | 60 +++++++++++++--- apps/hub/src/github/triage-verdicts.ts | 69 +++++++++++++------ apps/hub/src/server.ts | 1 + 4 files changed, 98 insertions(+), 34 deletions(-) diff --git a/apps/hub/src/github/github-read-access.postgres.test.ts b/apps/hub/src/github/github-read-access.postgres.test.ts index 7b0bf46d9..97492873a 100644 --- a/apps/hub/src/github/github-read-access.postgres.test.ts +++ b/apps/hub/src/github/github-read-access.postgres.test.ts @@ -308,7 +308,7 @@ test("PostgreSQL: a code review is sent once per head as one COMMENT review, inl test("PostgreSQL: every verdict route needs workflow-run read, which holding the GitHub credential does not give", async () => { const unused = {} as never; const app = guardedApp(handle.db, sessionOf, grants, { - triageVerdicts: createTriageVerdicts({ ...deps, list, verdicts: unused, managerRuns: unused, openPulls: unused, triageState: unused, now: unused }), + triageVerdicts: createTriageVerdicts({ ...deps, list, verdicts: unused, managerRuns: unused, openPulls: unused, openHeadFor: unused, triageState: unused, now: unused }), triageTimeline: createTriageTimeline({ verdicts: unused }), triageStats: createTriageStats({ verdicts: unused, outcomes: unused, now: unused, log: unused }), audit: createAuditLog({ db: unused, artifacts: unused, readCheckPack: unused, runReader: unused, authorize: unused, workflows: ["pr-triage"] }), diff --git a/apps/hub/src/github/triage-verdicts.postgres.test.ts b/apps/hub/src/github/triage-verdicts.postgres.test.ts index 36b709d58..ecdfb0524 100644 --- a/apps/hub/src/github/triage-verdicts.postgres.test.ts +++ b/apps/hub/src/github/triage-verdicts.postgres.test.ts @@ -19,6 +19,11 @@ import { createVerdictRecordStore, type RecordedVerdict, replaceVerdict } from " /** No deployment holds the credential in these tests. */ async function noKeyPush(): Promise {} +/** No host credential reads a single pull request's head. */ +async function noHeads(): Promise { + return undefined; +} + const TEST_DB = { host: "localhost", port: 5432, user: "postgres", password: "postgres", database: "interchange" }; const TENANT = "tnt_verdicts"; const OTHER_TENANT = "tnt_verdicts_other"; @@ -58,25 +63,40 @@ function observed(byHead: Array<[string, ObservedRun[]]>): ObservedRuns { const NO_RUNS: ObservedRuns = { byRepo: new Map() }; -/** The managers' runs: #8's newest run failed after its verdict, #9 and #10 are still running and #6's run failed early; #7 has no run, but waits for CI. */ -const MANAGED = observed([ - ["8@sha_run_3", [live("run_3", "failed", minutes(9))]], - ["9@sha9", [live("run_9", "running", minutes(-5), "pull_request:synchronize")]], - ["10@sha10", [live("run_10", "running", minutes(-120))]], - ["6@sha6", [live("run_6", "failed", minutes(-3))]], -]); +/** + * The managers' runs: #8's newest run failed after its verdict, #9 and #10 are still running and #6's run failed early; #7 has no run, but waits for CI. + * #4 of the repository GitHub could not list has a failed run. + */ +const MANAGED: ObservedRuns = { + byRepo: new Map([ + ...observed([ + ["8@sha_run_3", [live("run_3", "failed", minutes(9))]], + ["9@sha9", [live("run_9", "running", minutes(-5), "pull_request:synchronize")]], + ["10@sha10", [live("run_10", "running", minutes(-120))]], + ["6@sha6", [live("run_6", "failed", minutes(-3))]], + ]).byRepo, + [UNREAD, new Map([["4@sha_u4", [live("run_u4", "failed", minutes(-2))]]])], + ]), +}; const OPEN: RepoPulls[] = [ { repo: REPO, source: "github", prs: [6, 7, 8, 9, 10, 11, 12].map((number) => ({ number, title: `PR ${number}`, author: "ada", draft: false, sha: `sha${number}`, branch: null, updatedAt: minutes(0).toISOString(), labels: [], url: null })) }, { repo: UNREAD, source: "github", prs: [], error: "rate limited" }, ]; -/** #7 waits for CI; #8's degraded run kept its verdict, #11's run never started and #12's mail failed to send. */ +/** #7 waits for CI; #8's degraded run kept its verdict, #11's run never started, #12's mail failed to send and #6's run failed on a head since pushed over. */ const waitingRows: PrTriageRow[] = [ { number: 7, headSha: "sha_waiting", status: "waiting", attempts: 0, firstSeenAt: minutes(-3).toISOString(), updatedAt: minutes(-3).toISOString() }, { number: 8, headSha: "sha_run_3", status: "failed", attempts: 1, error: "degraded: the policy could not be read", firstSeenAt: minutes(9).toISOString(), updatedAt: minutes(10).toISOString() }, { number: 11, headSha: "sha11", status: "failed", attempts: 3, error: "run never started", firstSeenAt: minutes(-4).toISOString(), updatedAt: minutes(-4).toISOString() }, { number: 12, headSha: "sha12", status: "failed", attempts: 1, error: "delivery failed: connection refused", firstSeenAt: minutes(-4).toISOString(), updatedAt: minutes(-4).toISOString() }, + { number: 6, headSha: "sha6_old", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-6).toISOString(), updatedAt: minutes(-6).toISOString() }, +]; + +/** In the repository GitHub could not list, #3 has since closed and #4 is open with its manager's run; the run of each failed. */ +const unreadRows: PrTriageRow[] = [ + { number: 3, headSha: "sha_u3", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-6).toISOString(), updatedAt: minutes(-6).toISOString() }, + { number: 4, headSha: "sha_u4", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-2).toISOString(), updatedAt: minutes(-2).toISOString() }, ]; type DB = ReturnType["db"]; @@ -135,6 +155,7 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in // The portal's github-open-pulls listing; the workspace has no GitHub credential, so the route never lists on its own. const openPulls = createOpenPullsCache(() => minutes(0)); await openPulls.read(TENANT, async () => OPEN); + const headReads: string[] = []; const triageVerdicts = createTriageVerdicts({ db, cipher: createEnvKeyCredentialCipher(new Uint8Array(32).fill(7)), @@ -144,7 +165,12 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in managerRuns: async () => managed, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, - triageState: { load: async (_tenant, key) => ({ rows: key === REPO ? waitingRows : [], version: null }) }, + openHeadFor: async () => async function openHead(record, number) { + headReads.push(`${record.name}#${number}`); + const sha = OPEN[0]!.prs.find((pr) => pr.number === number)?.sha; + return sha === undefined ? undefined : { headSha: sha, draft: false }; + }, + triageState: { load: async (_tenant, key) => ({ rows: ({ [REPO]: waitingRows, [UNREAD]: unreadRows })[key] ?? [], version: null }) }, now: () => now, }); const app = guardedApp(db, async () => session as never, createInMemoryGrantStore(grants), { triageVerdicts }); @@ -158,8 +184,8 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in expect(res.status).toBe(200); const body = TriageVerdicts.assert(await res.json()); const byPull = new Map(body.pulls.map((pull) => [`${pull.repo}#${pull.number}`, pull])); - // #5 is closed, #4 is in a repository no longer connected, #12 has nothing a person can read; where GitHub could not answer, only a recent verdict is listed. - expect([...byPull.keys()]).toEqual([`${UNREAD}#2`, `${REPO}#6`, `${REPO}#7`, `${REPO}#8`, `${REPO}#9`, `${REPO}#10`, `${REPO}#11`]); + // #5 is closed, #4 is in a repository no longer connected, #12 has nothing a person can read; where GitHub could not answer, only a recent verdict or a run is listed, never a note alone. + expect([...byPull.keys()]).toEqual([`${UNREAD}#2`, `${UNREAD}#4`, `${REPO}#6`, `${REPO}#7`, `${REPO}#8`, `${REPO}#9`, `${REPO}#10`, `${REPO}#11`]); expect(body.triaged).toEqual([{ repo: REPO, source: "github" }, { repo: UNREAD, source: "github" }]); expect(body.active).toEqual([{ repo: REPO, source: "github" }, { repo: UNREAD, source: "github" }]); expect(body.scored).toBe(false); @@ -184,6 +210,15 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in }); expect(byPull.get(`${REPO}#10`)?.run?.status).toBe("stuck"); expect(byPull.get(`${REPO}#6`)?.run?.status).toBe("failed"); + expect(byPull.get(`${REPO}#6`)?.untriaged).toBeUndefined(); + expect(byPull.get(`${UNREAD}#4`)?.untriaged).toBe("Couldn't triage: the run failed"); + // A single read asks the host for that pull request's head alone. + async function untriagedOf(number: number): Promise> { + return TriageVerdicts.assert(await (await get(`?repo=${REPO}&number=${number}`)).json()).pulls.map((pull) => pull.untriaged); + } + expect(await untriagedOf(11)).toEqual(["Couldn't triage: the run never started"]); + expect(await untriagedOf(6)).toEqual([undefined]); + expect(headReads).toEqual([`${REPO}#11`, `${REPO}#6`]); // Polls while nothing changed are answered from memory; a recorded change is read once. const etag = res.headers.get("etag")!; @@ -235,6 +270,7 @@ test("PostgreSQL: pull requests of a repository with triage off are listed only managerRuns: async () => NO_RUNS, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, + openHeadFor: noHeads, triageState: { load: async () => ({ rows: [], version: null }) }, now: () => minutes(0), }); @@ -284,6 +320,7 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi ...deps, verdicts, managerRuns: async () => NO_RUNS, + openHeadFor: noHeads, triageState: { load: async () => ({ rows: [], version: null }) }, now: () => now, }); @@ -373,6 +410,7 @@ test("PostgreSQL: the same repository and number on both hosts come back separat managerRuns: async () => managed, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, + openHeadFor: noHeads, triageState: { load: async () => ({ rows: [], version: null }) }, now: () => minutes(0), }); diff --git a/apps/hub/src/github/triage-verdicts.ts b/apps/hub/src/github/triage-verdicts.ts index 23006084e..727155b8d 100644 --- a/apps/hub/src/github/triage-verdicts.ts +++ b/apps/hub/src/github/triage-verdicts.ts @@ -13,6 +13,7 @@ import { PR_TRIAGE_STUCK_RUN_MS, REPO_SOURCES, repoPolicy, triageStopped, Triage import { bitbucketWrite } from "@corbits/bitbucket-datacenter-tool"; import { listConnectedPulls, listingRefusal, refusedSources, type OpenPrsLister, type OpenPullsCache, type OpenPullsRead, type RepoPulls } from "./open-pulls.js"; import { failure, type PortalCredentialDeps } from "./portal-credential.js"; +import type { OpenHeadReader } from "./tenant-open-heads.js"; import type { Member } from "../integrations/app.js"; import { runKeyNumber, type ObservedRun } from "./reconcile-plan.js"; import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; @@ -59,6 +60,7 @@ export type TriageVerdictsDeps = PortalCredentialDeps & { managerRuns: ManagerRuns; openPulls: OpenPullsCache; list: OpenPrsLister; + openHeadFor: (tenantId: string) => Promise; triageState: Pick; now: () => Date; }; @@ -68,11 +70,14 @@ type LivePull = HostedPull & { runs: ObservedRun[] }; /** A pull request whose newest head waits for CI, first seen `since`. */ type WaitingPull = HostedPull & { since: string; headSha: string }; -/** Why the newest head has no verdict, in words for the portal, with the pull request it belongs to. */ +/** Why the pull request's current head has no verdict, in words for the portal, with the pull request it belongs to. */ type Untriaged = HostedPull & { untriaged: string }; type PullNotes = { waiting: Map; untriaged: Map }; +/** Each listed repository on its host, with the head sha of each open pull request. */ +type OpenHeads = Map }>; + /** What a person sees for a head's stored note. The stopped-triage note is already written for them; any other note stays internal. */ const UNTRIAGED_TEXT: ReadonlyMap = new Map([ ["run never started", "Couldn't triage: the run never started"], @@ -152,19 +157,19 @@ function connectedRecord(connected: readonly RepoRecord[], repo: string, source: } /** - * Open pull request numbers per listed repository on its host among `records`, plus the `held` ones elsewhere: an approval + * Open pull requests per listed repository on its host among `records`, plus the `held` ones elsewhere: an approval * still needs a person after triage is turned off. A repository its host could not list is left out. */ -function openNumbers(listed: OpenPullsRead | undefined, records: readonly RepoRecord[], held: readonly HostedPull[]): Map }> { +function openHeads(listed: OpenPullsRead | undefined, records: readonly RepoRecord[], held: readonly HostedPull[]): OpenHeads { if (!listed) return new Map(); - function numbersOf({ repo, source, prs, error }: RepoPulls): Array }]> { + function headsOf({ repo, source, prs, error }: RepoPulls): Array }]> { if (error !== undefined) return []; const key = hostKey({ repo, source }); const wanted = records.some((record) => triageStateKey(record) === key); - const numbers = prs.map((pr) => pr.number).filter((number) => wanted || held.some((pull) => pullKey(pull) === pullKey({ repo, source, number }))); - return wanted || numbers.length > 0 ? [[key, { repo, source, numbers: new Set(numbers) }]] : []; + const heads = prs.filter((pr) => wanted || held.some((pull) => pullKey(pull) === pullKey({ repo, source, number: pr.number }))).map((pr) => [pr.number, pr.sha] as const); + return wanted || heads.length > 0 ? [[key, { repo, source, heads: new Map(heads) }]] : []; } - return new Map(listed.repos.flatMap(numbersOf)); + return new Map(listed.repos.flatMap(headsOf)); } const ApprovalPull = type({ repo: "string", number: "number.integer" }); @@ -220,8 +225,8 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { return connected.map((record) => ({ repo: record.name, source: sourceOf(record) })).filter((repo) => seen.has(hostKey(repo))); } - /** The newest stored head of each pull request: one waiting for CI, and why triage produced no verdict when a person can read it. */ - async function notesOf(tenantId: string, records: readonly RepoRecord[]): Promise { + /** The newest stored head of each pull request: one waiting for CI, and why triage produced no verdict when a person can read it and the head is `current`. */ + async function notesOf(tenantId: string, records: readonly RepoRecord[], current: (pull: HostedPull, headSha: string) => boolean): Promise { const states = await Promise.all(records.map((record) => deps.triageState.load(tenantId, triageStateKey(record)))); const waiting = new Map(); const untriaged = new Map(); @@ -232,17 +237,18 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const pull = { repo: records[index]!.name, source: sourceOf(records[index]!), number: row.number }; if (row.status === "waiting") waiting.set(pullKey(pull), { ...pull, since: row.firstSeenAt, headSha: row.headSha }); const text = row.error === undefined ? undefined : untriagedText(row.error, pull.source); - if (text !== undefined) untriaged.set(pullKey(pull), { ...pull, untriaged: text }); + if (text !== undefined && current(pull, row.headSha)) untriaged.set(pullKey(pull), { ...pull, untriaged: text }); } } return { waiting, untriaged }; } - async function build(tenantId: string, latest: readonly HostedVerdict[], live: Map, notes: PullNotes, wanted: (pull: HostedPull) => boolean, connected: readonly RepoRecord[]): Promise { + /** `noted`: the pull requests a note alone lists, being known open. */ + async function build(tenantId: string, latest: readonly HostedVerdict[], live: Map, notes: PullNotes, noted: readonly HostedPull[], wanted: (pull: HostedPull) => boolean, connected: readonly RepoRecord[]): Promise { const now = deps.now(); const byPull = new Map(latest.map((verdict) => [pullKey(verdict), verdict])); const refs = new Map(); - for (const ref of [...latest, ...live.values(), ...notes.waiting.values(), ...notes.untriaged.values()]) { + for (const ref of [...latest, ...live.values(), ...notes.waiting.values(), ...noted]) { if (wanted(ref)) refs.set(pullKey(ref), { repo: ref.repo, number: ref.number, source: ref.source }); } const pulls = [...refs.values()].sort((a, b) => a.repo.localeCompare(b.repo) || a.number - b.number || a.source.localeCompare(b.source)); @@ -283,17 +289,34 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { } /** Lists the pull requests of `enabled` repositories and the `held` ones; `connected`, every connected one, still decides which count as triaged. */ - async function openList(tenantId: string, enabled: readonly RepoRecord[], connected: readonly RepoRecord[], open: Map }>, live: Map): Promise { + async function openList(tenantId: string, enabled: readonly RepoRecord[], connected: readonly RepoRecord[], open: OpenHeads, live: Map): Promise { const unknown = enabled.filter((record) => !open.has(triageStateKey(record))); const unlisted = new Set(unknown.map(triageStateKey)); - const pairs = [...open.values()].flatMap(({ repo, numbers }) => [...numbers].map((number) => ({ repo, number }))); + const pairs = [...open.values()].flatMap(({ repo, heads }) => [...heads.keys()].map((number) => ({ repo, number }))); const since = new Date(deps.now().getTime() - RECENT_MS); - const [known, recent, notes] = await Promise.all([deps.verdicts.latest(tenantId, pairs), deps.verdicts.latestSince(tenantId, unknown.map((record) => record.name), since), notesOf(tenantId, enabled)]); + // A repository its host could not list keeps its notes, shown only on a pull request listed for another reason. + function current(pull: HostedPull, headSha: string): boolean { + const listing = open.get(hostKey(pull)); + return listing === undefined || listing.heads.get(pull.number) === headSha; + } + const [known, recent, notes] = await Promise.all([deps.verdicts.latest(tenantId, pairs), deps.verdicts.latestSince(tenantId, unknown.map((record) => record.name), since), notesOf(tenantId, enabled, current)]); function wanted(pull: HostedPull): boolean { const listing = open.get(hostKey(pull)); - return listing === undefined ? unlisted.has(hostKey(pull)) : listing.numbers.has(pull.number); + return listing === undefined ? unlisted.has(hostKey(pull)) : listing.heads.has(pull.number); + } + const noted = [...notes.untriaged.values()].filter((pull) => open.has(hostKey(pull))); + return build(tenantId, [...known.filter((verdict) => open.has(hostKey(verdict))), ...recent], live, notes, noted, wanted, connected); + } + + /** The pull request's open head on its host: null once it is closed, undefined when the host could not say. */ + async function headOn(tenantId: string, record: RepoRecord, number: number): Promise { + try { + const read = await deps.openHeadFor(tenantId); + if (!read) return undefined; + return (await read(record, number))?.headSha ?? null; + } catch { + return undefined; } - return build(tenantId, [...known.filter((verdict) => open.has(hostKey(verdict))), ...recent], live, notes, wanted, connected); } return async function handle(req: Request, member: Member): Promise { @@ -309,19 +332,21 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const record = connectedRecord(connected, query.repo, query.source); if (!record) return failure(404, "not_found", "That repository is not connected."); const pull = { repo: query.repo, number: query.number, source: sourceOf(record) }; - const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record]), deps.managerRuns(tenantId, [pull])]); + const [latest, head, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), headOn(tenantId, record, pull.number), deps.managerRuns(tenantId, [pull])]); + const notes = await notesOf(tenantId, [record], (_pull, headSha) => head === undefined || head === headSha); const key = pullKey(pull); + const untriaged = new Map([...notes.untriaged].filter(([id]) => id === key)); const one = await build(tenantId, latest ? [latest] : [], new Map([...livePulls(runs)].filter(([id]) => id === key)), { waiting: new Map([...notes.waiting].filter(([id]) => id === key)), - untriaged: new Map([...notes.untriaged].filter(([id]) => id === key)), - }, () => true, connected); + untriaged, + }, [...untriaged.values()], () => true, connected); return respond(req, bodyOf(one.pulls.length > 0 ? one : { ...one, pulls: [{ ...pull, activityAt: null, verdict: null, run: null }] })); } const enabled = connected.filter((record) => repoPolicy(record).enabled); const [listed, held] = await Promise.all([listingOf(member, connected), heldPulls(tenantId)]); - const open = openNumbers(listed, enabled, held); - const pulls = [...open.values()].flatMap(({ repo, source, numbers }) => [...numbers].map((number) => ({ repo, source, number }))); + const open = openHeads(listed, enabled, held); + const pulls = [...open.values()].flatMap(({ repo, source, heads }) => [...heads.keys()].map((number) => ({ repo, source, number }))); const live = livePulls(await deps.managerRuns(tenantId, pulls)); const runs = [...live.values()].map((pull) => `${pullKey(pull)}:${pull.runs.map((run) => `${run.runId}:${run.status}`).join(",")}`).sort(); const key = [listed?.at, Math.floor(deps.now().getTime() / REBUILD_MS), ...connected.map(triageStateKey), "--", ...enabled.map(triageStateKey), "--", ...held.map(pullKey).sort(), "--", ...runs].join("\n"); diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index 94980d27c..6c0cfd665 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -814,6 +814,7 @@ const integrations = createIntegrationsApp({ db: composition.db, getSession: com managerRuns, openPulls: openPullsCache, list: tenantOpenPrs, + openHeadFor: tenantOpenHead, triageState: triageStateStore, now, }), From 4060768e7d9c0b2fa413855fd71cff2186bb1bf8 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 09:56:27 -0700 Subject: [PATCH 2/4] fix(hub): read the head only for an untriaged note, and keep it when the host gives no sha --- apps/hub/src/github/coalescer.ts | 2 +- apps/hub/src/github/tenant-open-heads.ts | 6 ++++-- .../src/github/triage-verdicts.postgres.test.ts | 16 +++++++++++++--- apps/hub/src/github/triage-verdicts.ts | 16 +++++++++------- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 9c746c96e..3d89a45ef 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -193,7 +193,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { const read = await deps.openHeadFor(head.tenantId); if (!read) throw new Error("no active github credential"); const pr = await read(latest.record, head.number); - if (pr === undefined) return "not_open"; + if (!pr) return "not_open"; if (pr.headSha !== head.headSha) return "superseded"; return pr.draft ? "draft" : undefined; } diff --git a/apps/hub/src/github/tenant-open-heads.ts b/apps/hub/src/github/tenant-open-heads.ts index ff033f5a5..98842d2e9 100644 --- a/apps/hub/src/github/tenant-open-heads.ts +++ b/apps/hub/src/github/tenant-open-heads.ts @@ -14,7 +14,8 @@ import type { OpenPr } from "./reconcile-plan.js"; import { sourceOf, type RepoRecord } from "./tenant-config.js"; export type OpenHeadsReader = (record: RepoRecord) => Promise; -export type OpenHeadReader = (record: RepoRecord, number: number) => Promise | undefined>; +/** Null once the pull request is closed; undefined when it is open but the host gave no head. */ +export type OpenHeadReader = (record: RepoRecord, number: number) => Promise | null | undefined>; export type TenantGithubDeps = AppKeyDeps & { githubApiOrigin: string }; @@ -52,7 +53,8 @@ function openHeadsOf(prs: Array<{ number: number; sha?: string; updatedAt: strin } function openHeadOf(pr: { state: string; sha?: string; draft?: boolean }) { - return pr.state === "open" && typeof pr.sha === "string" && pr.sha !== "" ? { headSha: pr.sha, draft: pr.draft === true } : undefined; + if (pr.state !== "open") return null; + return typeof pr.sha === "string" && pr.sha !== "" ? { headSha: pr.sha, draft: pr.draft === true } : undefined; } function createTenantSources(deps: TenantSourcesDeps) { diff --git a/apps/hub/src/github/triage-verdicts.postgres.test.ts b/apps/hub/src/github/triage-verdicts.postgres.test.ts index ecdfb0524..671bd02b3 100644 --- a/apps/hub/src/github/triage-verdicts.postgres.test.ts +++ b/apps/hub/src/github/triage-verdicts.postgres.test.ts @@ -84,13 +84,18 @@ const OPEN: RepoPulls[] = [ { repo: UNREAD, source: "github", prs: [], error: "rate limited" }, ]; -/** #7 waits for CI; #8's degraded run kept its verdict, #11's run never started, #12's mail failed to send and #6's run failed on a head since pushed over. */ +/** + * #7 waits for CI; #8's degraded run kept its verdict, #11's run never started, #12's mail failed to send and #6's run failed on a head since pushed over. + * The runs of #13, since closed, and #14, open while its host gives no head, failed. + */ const waitingRows: PrTriageRow[] = [ { number: 7, headSha: "sha_waiting", status: "waiting", attempts: 0, firstSeenAt: minutes(-3).toISOString(), updatedAt: minutes(-3).toISOString() }, { number: 8, headSha: "sha_run_3", status: "failed", attempts: 1, error: "degraded: the policy could not be read", firstSeenAt: minutes(9).toISOString(), updatedAt: minutes(10).toISOString() }, { number: 11, headSha: "sha11", status: "failed", attempts: 3, error: "run never started", firstSeenAt: minutes(-4).toISOString(), updatedAt: minutes(-4).toISOString() }, { number: 12, headSha: "sha12", status: "failed", attempts: 1, error: "delivery failed: connection refused", firstSeenAt: minutes(-4).toISOString(), updatedAt: minutes(-4).toISOString() }, { number: 6, headSha: "sha6_old", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-6).toISOString(), updatedAt: minutes(-6).toISOString() }, + { number: 13, headSha: "sha13", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-6).toISOString(), updatedAt: minutes(-6).toISOString() }, + { number: 14, headSha: "sha14", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-6).toISOString(), updatedAt: minutes(-6).toISOString() }, ]; /** In the repository GitHub could not list, #3 has since closed and #4 is open with its manager's run; the run of each failed. */ @@ -167,8 +172,9 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in list: () => async () => { throw new Error("the workspace has no host credential"); }, openHeadFor: async () => async function openHead(record, number) { headReads.push(`${record.name}#${number}`); + if (number === 14) return undefined; const sha = OPEN[0]!.prs.find((pr) => pr.number === number)?.sha; - return sha === undefined ? undefined : { headSha: sha, draft: false }; + return sha === undefined ? null : { headSha: sha, draft: false }; }, triageState: { load: async (_tenant, key) => ({ rows: ({ [REPO]: waitingRows, [UNREAD]: unreadRows })[key] ?? [], version: null }) }, now: () => now, @@ -218,7 +224,9 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in } expect(await untriagedOf(11)).toEqual(["Couldn't triage: the run never started"]); expect(await untriagedOf(6)).toEqual([undefined]); - expect(headReads).toEqual([`${REPO}#11`, `${REPO}#6`]); + expect(await untriagedOf(13)).toEqual([undefined]); + expect(await untriagedOf(14)).toEqual(["Couldn't triage: the run failed"]); + expect(headReads).toEqual([`${REPO}#11`, `${REPO}#6`, `${REPO}#13`, `${REPO}#14`]); // Polls while nothing changed are answered from memory; a recorded change is read once. const etag = res.headers.get("etag")!; @@ -231,6 +239,8 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in const closed = TriageVerdicts.assert(await (await get(`?repo=${REPO}&number=5`)).json()); expect(closed.pulls.map((pull) => [pull.number, pull.verdict?.runId])).toEqual([[5, "run_5"]]); + // With a verdict and no note, there is no head to ask the host for. + expect(headReads).toHaveLength(4); expect((await get(`?repo=${GONE}&number=4`)).status).toBe(404); expect((await get(`?repo=${REPO}`)).status).toBe(400); diff --git a/apps/hub/src/github/triage-verdicts.ts b/apps/hub/src/github/triage-verdicts.ts index 727155b8d..07cc08ad0 100644 --- a/apps/hub/src/github/triage-verdicts.ts +++ b/apps/hub/src/github/triage-verdicts.ts @@ -70,8 +70,8 @@ type LivePull = HostedPull & { runs: ObservedRun[] }; /** A pull request whose newest head waits for CI, first seen `since`. */ type WaitingPull = HostedPull & { since: string; headSha: string }; -/** Why the pull request's current head has no verdict, in words for the portal, with the pull request it belongs to. */ -type Untriaged = HostedPull & { untriaged: string }; +/** Why the pull request's current head has no verdict, in words for the portal, with the pull request and head it belongs to. */ +type Untriaged = HostedPull & { untriaged: string; headSha: string }; type PullNotes = { waiting: Map; untriaged: Map }; @@ -237,7 +237,7 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const pull = { repo: records[index]!.name, source: sourceOf(records[index]!), number: row.number }; if (row.status === "waiting") waiting.set(pullKey(pull), { ...pull, since: row.firstSeenAt, headSha: row.headSha }); const text = row.error === undefined ? undefined : untriagedText(row.error, pull.source); - if (text !== undefined && current(pull, row.headSha)) untriaged.set(pullKey(pull), { ...pull, untriaged: text }); + if (text !== undefined && current(pull, row.headSha)) untriaged.set(pullKey(pull), { ...pull, untriaged: text, headSha: row.headSha }); } } return { waiting, untriaged }; @@ -313,7 +313,8 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { try { const read = await deps.openHeadFor(tenantId); if (!read) return undefined; - return (await read(record, number))?.headSha ?? null; + const head = await read(record, number); + return head === null ? null : head?.headSha; } catch { return undefined; } @@ -332,10 +333,11 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const record = connectedRecord(connected, query.repo, query.source); if (!record) return failure(404, "not_found", "That repository is not connected."); const pull = { repo: query.repo, number: query.number, source: sourceOf(record) }; - const [latest, head, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), headOn(tenantId, record, pull.number), deps.managerRuns(tenantId, [pull])]); - const notes = await notesOf(tenantId, [record], (_pull, headSha) => head === undefined || head === headSha); + const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record], () => true), deps.managerRuns(tenantId, [pull])]); const key = pullKey(pull); - const untriaged = new Map([...notes.untriaged].filter(([id]) => id === key)); + const note = notes.untriaged.get(key); + const head = note === undefined ? undefined : await headOn(tenantId, record, pull.number); + const untriaged = new Map(note !== undefined && (head === undefined || head === note.headSha) ? [[key, note]] : []); const one = await build(tenantId, latest ? [latest] : [], new Map([...livePulls(runs)].filter(([id]) => id === key)), { waiting: new Map([...notes.waiting].filter(([id]) => id === key)), untriaged, From ad5ddf2c824e30ab0931e3e1e68fa94acab54249 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 10:14:28 -0700 Subject: [PATCH 3/4] fix(hub): judge a note current by one rule, read from the cached open listing --- apps/hub/src/github/coalescer.ts | 2 +- .../github-read-access.postgres.test.ts | 2 +- apps/hub/src/github/tenant-open-heads.ts | 6 +-- .../github/triage-verdicts.postgres.test.ts | 46 ++++++++----------- apps/hub/src/github/triage-verdicts.ts | 40 ++++++++-------- apps/hub/src/server.ts | 1 - 6 files changed, 42 insertions(+), 55 deletions(-) diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 3d89a45ef..9c746c96e 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -193,7 +193,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { const read = await deps.openHeadFor(head.tenantId); if (!read) throw new Error("no active github credential"); const pr = await read(latest.record, head.number); - if (!pr) return "not_open"; + if (pr === undefined) return "not_open"; if (pr.headSha !== head.headSha) return "superseded"; return pr.draft ? "draft" : undefined; } diff --git a/apps/hub/src/github/github-read-access.postgres.test.ts b/apps/hub/src/github/github-read-access.postgres.test.ts index 97492873a..7b0bf46d9 100644 --- a/apps/hub/src/github/github-read-access.postgres.test.ts +++ b/apps/hub/src/github/github-read-access.postgres.test.ts @@ -308,7 +308,7 @@ test("PostgreSQL: a code review is sent once per head as one COMMENT review, inl test("PostgreSQL: every verdict route needs workflow-run read, which holding the GitHub credential does not give", async () => { const unused = {} as never; const app = guardedApp(handle.db, sessionOf, grants, { - triageVerdicts: createTriageVerdicts({ ...deps, list, verdicts: unused, managerRuns: unused, openPulls: unused, openHeadFor: unused, triageState: unused, now: unused }), + triageVerdicts: createTriageVerdicts({ ...deps, list, verdicts: unused, managerRuns: unused, openPulls: unused, triageState: unused, now: unused }), triageTimeline: createTriageTimeline({ verdicts: unused }), triageStats: createTriageStats({ verdicts: unused, outcomes: unused, now: unused, log: unused }), audit: createAuditLog({ db: unused, artifacts: unused, readCheckPack: unused, runReader: unused, authorize: unused, workflows: ["pr-triage"] }), diff --git a/apps/hub/src/github/tenant-open-heads.ts b/apps/hub/src/github/tenant-open-heads.ts index 98842d2e9..ff033f5a5 100644 --- a/apps/hub/src/github/tenant-open-heads.ts +++ b/apps/hub/src/github/tenant-open-heads.ts @@ -14,8 +14,7 @@ import type { OpenPr } from "./reconcile-plan.js"; import { sourceOf, type RepoRecord } from "./tenant-config.js"; export type OpenHeadsReader = (record: RepoRecord) => Promise; -/** Null once the pull request is closed; undefined when it is open but the host gave no head. */ -export type OpenHeadReader = (record: RepoRecord, number: number) => Promise | null | undefined>; +export type OpenHeadReader = (record: RepoRecord, number: number) => Promise | undefined>; export type TenantGithubDeps = AppKeyDeps & { githubApiOrigin: string }; @@ -53,8 +52,7 @@ function openHeadsOf(prs: Array<{ number: number; sha?: string; updatedAt: strin } function openHeadOf(pr: { state: string; sha?: string; draft?: boolean }) { - if (pr.state !== "open") return null; - return typeof pr.sha === "string" && pr.sha !== "" ? { headSha: pr.sha, draft: pr.draft === true } : undefined; + return pr.state === "open" && typeof pr.sha === "string" && pr.sha !== "" ? { headSha: pr.sha, draft: pr.draft === true } : undefined; } function createTenantSources(deps: TenantSourcesDeps) { diff --git a/apps/hub/src/github/triage-verdicts.postgres.test.ts b/apps/hub/src/github/triage-verdicts.postgres.test.ts index 671bd02b3..0f8743dd0 100644 --- a/apps/hub/src/github/triage-verdicts.postgres.test.ts +++ b/apps/hub/src/github/triage-verdicts.postgres.test.ts @@ -19,11 +19,6 @@ import { createVerdictRecordStore, type RecordedVerdict, replaceVerdict } from " /** No deployment holds the credential in these tests. */ async function noKeyPush(): Promise {} -/** No host credential reads a single pull request's head. */ -async function noHeads(): Promise { - return undefined; -} - const TEST_DB = { host: "localhost", port: 5432, user: "postgres", password: "postgres", database: "interchange" }; const TENANT = "tnt_verdicts"; const OTHER_TENANT = "tnt_verdicts_other"; @@ -79,8 +74,9 @@ const MANAGED: ObservedRuns = { ]), }; +/** #14 is open with no head GitHub could name. */ const OPEN: RepoPulls[] = [ - { repo: REPO, source: "github", prs: [6, 7, 8, 9, 10, 11, 12].map((number) => ({ number, title: `PR ${number}`, author: "ada", draft: false, sha: `sha${number}`, branch: null, updatedAt: minutes(0).toISOString(), labels: [], url: null })) }, + { repo: REPO, source: "github", prs: [6, 7, 8, 9, 10, 11, 12, 14].map((number) => ({ number, title: `PR ${number}`, author: "ada", draft: false, sha: number === 14 ? "" : `sha${number}`, branch: null, updatedAt: minutes(0).toISOString(), labels: [], url: null })) }, { repo: UNREAD, source: "github", prs: [], error: "rate limited" }, ]; @@ -160,7 +156,6 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in // The portal's github-open-pulls listing; the workspace has no GitHub credential, so the route never lists on its own. const openPulls = createOpenPullsCache(() => minutes(0)); await openPulls.read(TENANT, async () => OPEN); - const headReads: string[] = []; const triageVerdicts = createTriageVerdicts({ db, cipher: createEnvKeyCredentialCipher(new Uint8Array(32).fill(7)), @@ -170,12 +165,6 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in managerRuns: async () => managed, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, - openHeadFor: async () => async function openHead(record, number) { - headReads.push(`${record.name}#${number}`); - if (number === 14) return undefined; - const sha = OPEN[0]!.prs.find((pr) => pr.number === number)?.sha; - return sha === undefined ? null : { headSha: sha, draft: false }; - }, triageState: { load: async (_tenant, key) => ({ rows: ({ [REPO]: waitingRows, [UNREAD]: unreadRows })[key] ?? [], version: null }) }, now: () => now, }); @@ -191,7 +180,7 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in const body = TriageVerdicts.assert(await res.json()); const byPull = new Map(body.pulls.map((pull) => [`${pull.repo}#${pull.number}`, pull])); // #5 is closed, #4 is in a repository no longer connected, #12 has nothing a person can read; where GitHub could not answer, only a recent verdict or a run is listed, never a note alone. - expect([...byPull.keys()]).toEqual([`${UNREAD}#2`, `${UNREAD}#4`, `${REPO}#6`, `${REPO}#7`, `${REPO}#8`, `${REPO}#9`, `${REPO}#10`, `${REPO}#11`]); + expect([...byPull.keys()]).toEqual([`${UNREAD}#2`, `${UNREAD}#4`, `${REPO}#6`, `${REPO}#7`, `${REPO}#8`, `${REPO}#9`, `${REPO}#10`, `${REPO}#11`, `${REPO}#14`]); expect(body.triaged).toEqual([{ repo: REPO, source: "github" }, { repo: UNREAD, source: "github" }]); expect(body.active).toEqual([{ repo: REPO, source: "github" }, { repo: UNREAD, source: "github" }]); expect(body.scored).toBe(false); @@ -218,15 +207,12 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in expect(byPull.get(`${REPO}#6`)?.run?.status).toBe("failed"); expect(byPull.get(`${REPO}#6`)?.untriaged).toBeUndefined(); expect(byPull.get(`${UNREAD}#4`)?.untriaged).toBe("Couldn't triage: the run failed"); - // A single read asks the host for that pull request's head alone. + expect(byPull.get(`${REPO}#14`)?.untriaged).toBe("Couldn't triage: the run failed"); + // A single read keeps and drops the same notes the list does. async function untriagedOf(number: number): Promise> { return TriageVerdicts.assert(await (await get(`?repo=${REPO}&number=${number}`)).json()).pulls.map((pull) => pull.untriaged); } - expect(await untriagedOf(11)).toEqual(["Couldn't triage: the run never started"]); - expect(await untriagedOf(6)).toEqual([undefined]); - expect(await untriagedOf(13)).toEqual([undefined]); - expect(await untriagedOf(14)).toEqual(["Couldn't triage: the run failed"]); - expect(headReads).toEqual([`${REPO}#11`, `${REPO}#6`, `${REPO}#13`, `${REPO}#14`]); + for (const number of [6, 11, 13, 14]) expect(await untriagedOf(number)).toEqual([byPull.get(`${REPO}#${number}`)?.untriaged]); // Polls while nothing changed are answered from memory; a recorded change is read once. const etag = res.headers.get("etag")!; @@ -239,8 +225,6 @@ test("PostgreSQL: the latest verdict of each open pull request, with the run in const closed = TriageVerdicts.assert(await (await get(`?repo=${REPO}&number=5`)).json()); expect(closed.pulls.map((pull) => [pull.number, pull.verdict?.runId])).toEqual([[5, "run_5"]]); - // With a verdict and no note, there is no head to ask the host for. - expect(headReads).toHaveLength(4); expect((await get(`?repo=${GONE}&number=4`)).status).toBe(404); expect((await get(`?repo=${REPO}`)).status).toBe(400); @@ -280,7 +264,6 @@ test("PostgreSQL: pull requests of a repository with triage off are listed only managerRuns: async () => NO_RUNS, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, - openHeadFor: noHeads, triageState: { load: async () => ({ rows: [], version: null }) }, now: () => minutes(0), }); @@ -309,10 +292,12 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi id: "crd_github", tenantId: TENANT, principalId: null, providerId: "prv_github", oauthClientId: null, name: "github", type: "api_key", secret: "unused", status: "active", }); const verdicts = createVerdictRecordStore(db); - await seed(db, TENANT, [verdict("run_1", 1, 0)]); + await seed(db, TENANT, [verdict("run_1", 1, 0), verdict("run_3", 3, 0)]); let now = minutes(0); const openPulls = createOpenPullsCache(() => now); const listed: string[] = []; + /** #1's run failed on its open head. */ + const noted: PrTriageRow[] = [{ number: 1, headSha: "sha1", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-1).toISOString(), updatedAt: minutes(-1).toISOString() }]; const deps = { db, cipher: createEnvKeyCredentialCipher(new Uint8Array(32).fill(7)), @@ -330,8 +315,7 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi ...deps, verdicts, managerRuns: async () => NO_RUNS, - openHeadFor: noHeads, - triageState: { load: async () => ({ rows: [], version: null }) }, + triageState: { load: async (_tenant, key) => ({ rows: key === REPO ? noted : [], version: null }) }, now: () => now, }); const githubOpenPulls = createGithubOpenPulls({ ...deps, cache: openPulls }); @@ -346,6 +330,15 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi return TriageVerdicts.assert(await res.json()).pulls.map((pull) => `${pull.repo}#${pull.number}`); } + // A single read lists open pull requests only for a note, and within the listing's 30 seconds only once. + async function single(number: number): Promise { + expect((await app.request(`/api/integrations/triage-verdicts/${TENANT}?repo=${REPO}&number=${number}`, { headers: { "sec-fetch-site": "same-origin" } })).status).toBe(200); + } + await single(3); + expect(listed).toEqual([]); + await single(1); + now = minutes(0.1); + await single(1); expect(await pullsListed()).toEqual([`${REPO}#1`]); expect(listed).toEqual([REPO]); const res = await get(GITHUB_OPEN_PULLS_PATH); @@ -420,7 +413,6 @@ test("PostgreSQL: the same repository and number on both hosts come back separat managerRuns: async () => managed, openPulls, list: () => async () => { throw new Error("the workspace has no host credential"); }, - openHeadFor: noHeads, triageState: { load: async () => ({ rows: [], version: null }) }, now: () => minutes(0), }); diff --git a/apps/hub/src/github/triage-verdicts.ts b/apps/hub/src/github/triage-verdicts.ts index 07cc08ad0..d431b41bf 100644 --- a/apps/hub/src/github/triage-verdicts.ts +++ b/apps/hub/src/github/triage-verdicts.ts @@ -13,7 +13,6 @@ import { PR_TRIAGE_STUCK_RUN_MS, REPO_SOURCES, repoPolicy, triageStopped, Triage import { bitbucketWrite } from "@corbits/bitbucket-datacenter-tool"; import { listConnectedPulls, listingRefusal, refusedSources, type OpenPrsLister, type OpenPullsCache, type OpenPullsRead, type RepoPulls } from "./open-pulls.js"; import { failure, type PortalCredentialDeps } from "./portal-credential.js"; -import type { OpenHeadReader } from "./tenant-open-heads.js"; import type { Member } from "../integrations/app.js"; import { runKeyNumber, type ObservedRun } from "./reconcile-plan.js"; import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; @@ -60,7 +59,6 @@ export type TriageVerdictsDeps = PortalCredentialDeps & { managerRuns: ManagerRuns; openPulls: OpenPullsCache; list: OpenPrsLister; - openHeadFor: (tenantId: string) => Promise; triageState: Pick; now: () => Date; }; @@ -172,6 +170,18 @@ function openHeads(listed: OpenPullsRead | undefined, records: readonly RepoReco return new Map(listed.repos.flatMap(headsOf)); } +/** + * A note explains the head it was recorded on while that head is still open, or while the host can't say: it could not list the + * repository, or listed the pull request open with no head. + */ +function noteCurrent(open: OpenHeads, pull: HostedPull, headSha: string): boolean { + const listing = open.get(hostKey(pull)); + if (listing === undefined) return true; + if (!listing.heads.has(pull.number)) return false; + const head = listing.heads.get(pull.number); + return !head || head === headSha; +} + const ApprovalPull = type({ repo: "string", number: "number.integer" }); /** A held approval's host is its tool's: Bitbucket Data Center's write tools, else GitHub's. */ @@ -295,11 +305,11 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const pairs = [...open.values()].flatMap(({ repo, heads }) => [...heads.keys()].map((number) => ({ repo, number }))); const since = new Date(deps.now().getTime() - RECENT_MS); // A repository its host could not list keeps its notes, shown only on a pull request listed for another reason. - function current(pull: HostedPull, headSha: string): boolean { - const listing = open.get(hostKey(pull)); - return listing === undefined || listing.heads.get(pull.number) === headSha; - } - const [known, recent, notes] = await Promise.all([deps.verdicts.latest(tenantId, pairs), deps.verdicts.latestSince(tenantId, unknown.map((record) => record.name), since), notesOf(tenantId, enabled, current)]); + const [known, recent, notes] = await Promise.all([ + deps.verdicts.latest(tenantId, pairs), + deps.verdicts.latestSince(tenantId, unknown.map((record) => record.name), since), + notesOf(tenantId, enabled, (pull, headSha) => noteCurrent(open, pull, headSha)), + ]); function wanted(pull: HostedPull): boolean { const listing = open.get(hostKey(pull)); return listing === undefined ? unlisted.has(hostKey(pull)) : listing.heads.has(pull.number); @@ -308,18 +318,6 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { return build(tenantId, [...known.filter((verdict) => open.has(hostKey(verdict))), ...recent], live, notes, noted, wanted, connected); } - /** The pull request's open head on its host: null once it is closed, undefined when the host could not say. */ - async function headOn(tenantId: string, record: RepoRecord, number: number): Promise { - try { - const read = await deps.openHeadFor(tenantId); - if (!read) return undefined; - const head = await read(record, number); - return head === null ? null : head?.headSha; - } catch { - return undefined; - } - } - return async function handle(req: Request, member: Member): Promise { const { tenantId } = member; const query = PullQuery(Object.fromEntries(new URL(req.url).searchParams)); @@ -336,8 +334,8 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record], () => true), deps.managerRuns(tenantId, [pull])]); const key = pullKey(pull); const note = notes.untriaged.get(key); - const head = note === undefined ? undefined : await headOn(tenantId, record, pull.number); - const untriaged = new Map(note !== undefined && (head === undefined || head === note.headSha) ? [[key, note]] : []); + const current = note !== undefined && noteCurrent(openHeads(await listingOf(member, connected), [record], []), note, note.headSha); + const untriaged = new Map(current ? [[key, note]] : []); const one = await build(tenantId, latest ? [latest] : [], new Map([...livePulls(runs)].filter(([id]) => id === key)), { waiting: new Map([...notes.waiting].filter(([id]) => id === key)), untriaged, diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index 6c0cfd665..94980d27c 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -814,7 +814,6 @@ const integrations = createIntegrationsApp({ db: composition.db, getSession: com managerRuns, openPulls: openPullsCache, list: tenantOpenPrs, - openHeadFor: tenantOpenHead, triageState: triageStateStore, now, }), From 676aa288cf73ee653aff66caa35b7d32d8a829d8 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 10:46:03 -0700 Subject: [PATCH 4/4] fix(hub): list only a single read's repository, and only when its note can show --- apps/hub/src/github/open-pulls.ts | 25 +++++++++++-------- .../github/triage-verdicts.postgres.test.ts | 19 ++++++++------ apps/hub/src/github/triage-verdicts.ts | 24 +++++++++--------- 3 files changed, 38 insertions(+), 30 deletions(-) diff --git a/apps/hub/src/github/open-pulls.ts b/apps/hub/src/github/open-pulls.ts index 0faa78f86..37a368eca 100644 --- a/apps/hub/src/github/open-pulls.ts +++ b/apps/hub/src/github/open-pulls.ts @@ -101,35 +101,38 @@ export async function listConnectedPulls(list: OpenPrsLister, tenantId: string, export type OpenPullsRead = { at: number; repos: RepoPulls[] }; -/** GitHub's last listing of each tenant's open pull requests, shared by the portal's listing and the verdict route. */ +/** + * GitHub's last listing of open pull requests under `key`: each tenant's, shared by the portal's listing and the verdict route, + * or one repository's, read for a single pull request. + */ export type OpenPullsCache = { - kept(tenantId: string, maxAgeMs: number): OpenPullsRead | undefined; + kept(key: string, maxAgeMs: number): OpenPullsRead | undefined; /** Lists now and keeps the listing; callers while it is read share it. */ - read(tenantId: string, list: () => Promise): Promise; + read(key: string, list: () => Promise): Promise; }; export function createOpenPullsCache(now: () => Date): OpenPullsCache { const kept = new Map(); const reading = new Map>(); - async function readOnce(tenantId: string, list: () => Promise): Promise { + async function readOnce(key: string, list: () => Promise): Promise { try { const listed = { at: now().getTime(), repos: await list() }; - kept.set(tenantId, listed); + kept.set(key, listed); return listed; } finally { - reading.delete(tenantId); + reading.delete(key); } } return { - kept(tenantId, maxAgeMs) { - const listed = kept.get(tenantId); + kept(key, maxAgeMs) { + const listed = kept.get(key); return listed && now().getTime() - listed.at < maxAgeMs ? listed : undefined; }, - read(tenantId, list) { - const pending = reading.get(tenantId) ?? readOnce(tenantId, list); - reading.set(tenantId, pending); + read(key, list) { + const pending = reading.get(key) ?? readOnce(key, list); + reading.set(key, pending); return pending; }, }; diff --git a/apps/hub/src/github/triage-verdicts.postgres.test.ts b/apps/hub/src/github/triage-verdicts.postgres.test.ts index 0f8743dd0..7749e992d 100644 --- a/apps/hub/src/github/triage-verdicts.postgres.test.ts +++ b/apps/hub/src/github/triage-verdicts.postgres.test.ts @@ -284,7 +284,7 @@ test("PostgreSQL: pull requests of a repository with triage off are listed only test("PostgreSQL: a host with no credential costs only its own repositories, while a member barred from a host's credential is refused and leaves the shared listing as it was", async () => { await withDatabase(async function hostWithoutCredential(db) { const bitbucket = "TRI/widgets"; - const config = { corbitsTriage: { repos: [{ name: REPO, connected: true, enabled: true }, { name: bitbucket, connected: true, source: "bitbucket-datacenter", enabled: true }] } }; + const config = { corbitsTriage: { repos: [{ name: REPO, connected: true, enabled: true }, { name: UNREAD, connected: true, enabled: true }, { name: bitbucket, connected: true, source: "bitbucket-datacenter", enabled: true }] } }; await db.insert(schema.tenant).values({ id: TENANT, name: "Verdicts", slug: "verdicts", domain: "verdicts.test", config }); await db.insert(schema.principal).values({ id: "prn_verdicts", tenantId: TENANT, kind: "user", refId: "usr_member", status: "active" }); await db.insert(schema.provider).values({ id: "prv_github", tenantId: TENANT, name: "github", plugin: "http", apiBaseUrl: "https://api.github.test" }); @@ -292,12 +292,15 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi id: "crd_github", tenantId: TENANT, principalId: null, providerId: "prv_github", oauthClientId: null, name: "github", type: "api_key", secret: "unused", status: "active", }); const verdicts = createVerdictRecordStore(db); - await seed(db, TENANT, [verdict("run_1", 1, 0), verdict("run_3", 3, 0)]); + await seed(db, TENANT, [verdict("run_3", 3, 0)]); let now = minutes(0); const openPulls = createOpenPullsCache(() => now); const listed: string[] = []; - /** #1's run failed on its open head. */ - const noted: PrTriageRow[] = [{ number: 1, headSha: "sha1", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-1).toISOString(), updatedAt: minutes(-1).toISOString() }]; + /** #1's run failed on its open head; #3's failed too, but #3 has a verdict. */ + const noted: PrTriageRow[] = [ + { number: 1, headSha: "sha1", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-1).toISOString(), updatedAt: minutes(-1).toISOString() }, + { number: 3, headSha: "sha3", status: "failed", attempts: 1, error: "run failed", firstSeenAt: minutes(-1).toISOString(), updatedAt: minutes(-1).toISOString() }, + ]; const deps = { db, cipher: createEnvKeyCredentialCipher(new Uint8Array(32).fill(7)), @@ -330,7 +333,7 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi return TriageVerdicts.assert(await res.json()).pulls.map((pull) => `${pull.repo}#${pull.number}`); } - // A single read lists open pull requests only for a note, and within the listing's 30 seconds only once. + // A single read lists only its own repository, only for a note a verdict does not replace, and within the listing's 30 seconds only once. async function single(number: number): Promise { expect((await app.request(`/api/integrations/triage-verdicts/${TENANT}?repo=${REPO}&number=${number}`, { headers: { "sec-fetch-site": "same-origin" } })).status).toBe(200); } @@ -339,13 +342,15 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi await single(1); now = minutes(0.1); await single(1); - expect(await pullsListed()).toEqual([`${REPO}#1`]); expect(listed).toEqual([REPO]); + expect(await pullsListed()).toEqual([`${REPO}#1`]); + expect(listed).toEqual([REPO, REPO, UNREAD]); const res = await get(GITHUB_OPEN_PULLS_PATH); expect(res.status).toBe(200); const { repos } = await res.json() as { repos: RepoPulls[] }; expect(repos.map((row) => [row.repo, row.source, row.prs.length, row.error])).toEqual([ [REPO, "github", 1, undefined], + [UNREAD, "github", 1, undefined], [bitbucket, "bitbucket-datacenter", 0, "Connect Bitbucket Data Center first."], ]); @@ -358,7 +363,7 @@ test("PostgreSQL: a host with no credential costs only its own repositories, whi expect(refused.status).toBe(403); expect(await refused.json()).toEqual({ error: { code: "forbidden", message: "You do not have access to the Bitbucket Data Center credential." } }); expect(await pullsListed()).toEqual([`${REPO}#1`]); - expect(listed).toEqual([REPO]); + expect(listed).toEqual([REPO, REPO, UNREAD]); expect(openPulls.kept(TENANT, Infinity)).toBe(kept); }); }, 120_000); diff --git a/apps/hub/src/github/triage-verdicts.ts b/apps/hub/src/github/triage-verdicts.ts index d431b41bf..fe8523edc 100644 --- a/apps/hub/src/github/triage-verdicts.ts +++ b/apps/hub/src/github/triage-verdicts.ts @@ -177,9 +177,8 @@ function openHeads(listed: OpenPullsRead | undefined, records: readonly RepoReco function noteCurrent(open: OpenHeads, pull: HostedPull, headSha: string): boolean { const listing = open.get(hostKey(pull)); if (listing === undefined) return true; - if (!listing.heads.has(pull.number)) return false; const head = listing.heads.get(pull.number); - return !head || head === headSha; + return head !== undefined && (head === "" || head === headSha); } const ApprovalPull = type({ repo: "string", number: "number.integer" }); @@ -204,17 +203,18 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const kept = new Map(); /** - * The listing kept from the last 30 seconds, else one read now on each repository's host, like github-open-pulls, when - * the caller holds `read` on every host's credential, a host with none listed as errors. When github-open-pulls would refuse, - * the route asks nothing and uses the last listing of any age, or none, rather than refusing: the verdicts themselves need only the run grant. + * The open pull requests of `records`, kept under `key`: a listing from the last 30 seconds under `key`, or the workspace's + * shared one, else one read now on each repository's host, like github-open-pulls, when the caller holds `read` on every host's + * credential, a host with none listed as errors. When github-open-pulls would refuse, the route asks nothing and uses the last + * listing of any age, or none, rather than refusing: the verdicts themselves need only the run grant. */ - async function listingOf(member: Member, connected: readonly RepoRecord[]): Promise { + async function listingOf(member: Member, key: string, records: readonly RepoRecord[]): Promise { const { tenantId } = member; - const fresh = deps.openPulls.kept(tenantId, LISTING_MS); + const fresh = deps.openPulls.kept(key, LISTING_MS) ?? deps.openPulls.kept(tenantId, LISTING_MS); if (fresh) return fresh; - const refused = await refusedSources(deps, member, connected); - if (listingRefusal(connected, refused)) return deps.openPulls.kept(tenantId, Infinity); - return deps.openPulls.read(tenantId, () => listConnectedPulls(deps.list, tenantId, connected, refused)); + const refused = await refusedSources(deps, member, records); + if (listingRefusal(records, refused)) return deps.openPulls.kept(key, Infinity) ?? deps.openPulls.kept(tenantId, Infinity); + return deps.openPulls.read(key, () => listConnectedPulls(deps.list, tenantId, records, refused)); } async function activityOf(tenantId: string, pulls: readonly HostedPull[], live: Map): Promise> { @@ -334,7 +334,7 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { const [latest, notes, runs] = await Promise.all([deps.verdicts.latestFor(tenantId, pull), notesOf(tenantId, [record], () => true), deps.managerRuns(tenantId, [pull])]); const key = pullKey(pull); const note = notes.untriaged.get(key); - const current = note !== undefined && noteCurrent(openHeads(await listingOf(member, connected), [record], []), note, note.headSha); + const current = note !== undefined && !latest && noteCurrent(openHeads(await listingOf(member, `${tenantId}\n${triageStateKey(record)}`, [record]), [record], []), note, note.headSha); const untriaged = new Map(current ? [[key, note]] : []); const one = await build(tenantId, latest ? [latest] : [], new Map([...livePulls(runs)].filter(([id]) => id === key)), { waiting: new Map([...notes.waiting].filter(([id]) => id === key)), @@ -344,7 +344,7 @@ export function createTriageVerdicts(deps: TriageVerdictsDeps) { } const enabled = connected.filter((record) => repoPolicy(record).enabled); - const [listed, held] = await Promise.all([listingOf(member, connected), heldPulls(tenantId)]); + const [listed, held] = await Promise.all([listingOf(member, tenantId, connected), heldPulls(tenantId)]); const open = openHeads(listed, enabled, held); const pulls = [...open.values()].flatMap(({ repo, source, heads }) => [...heads.keys()].map((number) => ({ repo, source, number }))); const live = livePulls(await deps.managerRuns(tenantId, pulls));