diff --git a/.env.example b/.env.example index f6597f069..af646d734 100644 --- a/.env.example +++ b/.env.example @@ -46,8 +46,6 @@ HUB_DATA_DIR=./tmp/hub-data # Milliseconds between passes that re-queue open pull requests whose head is not triaged. Default 300000. # TRIAGE_RECONCILE_INTERVAL_MS= -# Runs the pr-triage deployment's log may hold before the hub deploys a fresh copy of it. Default 15. -# TRIAGE_ROTATE_AFTER_RUNS= # Pull request heads the reconciler lets a tenant have queued or running at once, each in a sidecar of its own. Default 5. # TRIAGE_MAX_IN_FLIGHT= # Milliseconds after a head is capped by runs that never started before it gets a fresh set of attempts. Default 21600000. diff --git a/apps/hub/src/code-review-feed.test.ts b/apps/hub/src/code-review-feed.test.ts index 52dac90af..abf5e3eb5 100644 --- a/apps/hub/src/code-review-feed.test.ts +++ b/apps/hub/src/code-review-feed.test.ts @@ -41,16 +41,12 @@ test("each new non-draft head is sent to its host's code review once; Review aga source: "event", payload: { kind: "code-review", number: 7, title: "Fix", description: "Body", diff: PULL.diff, diffHash: diffHash(PULL.diff), checkout: { source: "github", repo: "acme/widgets", sha: "sha1" } }, }]); - expect(await feed.again("t", RECORD, 7)).toBe("sent"); - expect(sent.map((job) => job.source)).toEqual(["event", "retry"]); }); -test("a head that takes the newest review over is not sent, and Review again sends it anyway", async () => { +test("a head that takes the newest review over is not sent", async () => { const { feed, sent } = harness(["github"], PULL, true); await feed.offer("t", RECORD, mail("synchronize")); expect(sent).toEqual([]); - expect(await feed.again("t", RECORD, 7)).toBe("sent"); - expect(sent.map((job) => job.source)).toEqual(["retry"]); }); test("a diff's hash changes with any blob id, and a diff with none, as GitHub rebuilds from the file list, has none and is sent unhashed", async () => { @@ -64,9 +60,11 @@ test("a diff's hash changes with any blob id, and a diff with none, as GitHub re }); test("nothing is sent while code review is off, the key is missing or the pull request turned draft", async () => { - expect(await harness([]).feed.again("t", RECORD, 7)).toBe("not_reviewed"); - expect(await harness().feed.again("t", { ...RECORD, codeReview: false }, 7)).toBe("not_reviewed"); - expect(await harness(["github"], { ...PULL, draft: true }).feed.again("t", RECORD, 7)).toBe("draft"); + for (const [reviewed, record, pull] of [[[], RECORD, PULL], [["github"], { ...RECORD, codeReview: false }, PULL], [["github"], RECORD, { ...PULL, draft: true }]] as const) { + const { feed, sent } = harness([...reviewed], pull); + await feed.offer("t", record, mail("synchronize")); + expect(sent).toEqual([]); + } }); test("a diff over the limit is cut at a line boundary and names the files it cut part-way or left out", () => { @@ -95,7 +93,7 @@ test("a cut names files as git writes them with or without prefixes and quotes, test("a host answer that is not a unified diff is refused, not reviewed", async () => { const { feed, sent } = harness(["github"], { ...PULL, diff: "{\"url\":\"…\"}" }); - await expect(feed.again("t", RECORD, 7)).rejects.toThrow("the host returned no unified diff"); + await feed.offer("t", RECORD, mail("synchronize")); expect(sent).toEqual([]); }); diff --git a/apps/hub/src/code-review-feed.ts b/apps/hub/src/code-review-feed.ts index acca9d444..d63242104 100644 --- a/apps/hub/src/code-review-feed.ts +++ b/apps/hub/src/code-review-feed.ts @@ -42,7 +42,7 @@ export type CodeReviewFeedDeps = { log: (entry: Record) => void; }; -export type Offered = "sent" | "not_reviewed" | "draft" | "not_open"; +type Unreviewed = "not_reviewed" | "draft" | "not_open"; /** * The diff's hash, its `index` lines' blob ids included, so a base change to a file the diff touches changes it; undefined @@ -172,7 +172,7 @@ export function createCodeReviewFeed(deps: CodeReviewFeedDeps) { } /** The pull request to review and its request, or why it is not reviewed. */ - async function toReview(tenantId: string, record: RepoRecord, number: number): Promise<{ pull: PullRequestToReview; request: ReviewRequest } | Exclude> { + async function toReview(tenantId: string, record: RepoRecord, number: number): Promise<{ pull: PullRequestToReview; request: ReviewRequest } | Unreviewed> { if (record.codeReview !== true || !(await deps.reviewed(tenantId, record)).includes(sourceOf(record))) return "not_reviewed"; const pull = await deps.read(tenantId, record, number); if (!pull.open) return "not_open"; @@ -181,15 +181,15 @@ export function createCodeReviewFeed(deps: CodeReviewFeedDeps) { } /** Sends the reserved head, which is let go when the send fails. */ - async function send(tenantId: string, record: RepoRecord, number: number, request: ReviewRequest, again: boolean): Promise { + async function send(tenantId: string, record: RepoRecord, number: number, request: ReviewRequest): Promise { const headSha = request.checkout.sha; try { - await deps.send({ tenantId, workflow: codeReviewWorkflowFor(sourceOf(record)).id, repo: record.name, number, headSha, source: again ? "retry" : "event", payload: request }); + await deps.send({ tenantId, workflow: codeReviewWorkflowFor(sourceOf(record)).id, repo: record.name, number, headSha, source: "event", payload: request }); } catch (err) { sent.delete(keyOf(tenantId, record, number, headSha)); throw err; } - deps.log({ level: "info", msg: "code_review_sent", tenantId, repo: record.name, pr: number, headSha, again, ...(request.cut && { cut: request.cut }) }); + deps.log({ level: "info", msg: "code_review_sent", tenantId, repo: record.name, pr: number, headSha, ...(request.cut && { cut: request.cut }) }); } /** A new head is sent once, unless it takes the newest review over. */ @@ -202,7 +202,7 @@ export function createCodeReviewFeed(deps: CodeReviewFeedDeps) { deps.log({ level: "info", msg: "code_review_carried", tenantId, repo: record.name, pr: number, headSha: pull.headSha }); return; } - await send(tenantId, record, number, request, false); + await send(tenantId, record, number, request); } return { @@ -217,14 +217,6 @@ export function createCodeReviewFeed(deps: CodeReviewFeedDeps) { deps.log({ level: "error", msg: "code_review_send_failed", tenantId, repo: record.name, pr: mail.prNumber, error: String(err) }); } }, - /** "Review again": the pull request's current head, even when it was reviewed already or took a review over. */ - async again(tenantId: string, record: RepoRecord, number: number): Promise { - const found = await toReview(tenantId, record, number); - if (typeof found === "string") return found; - reserve(keyOf(tenantId, record, number, found.pull.headSha)); - await send(tenantId, record, number, found.request, true); - return "sent"; - }, /** A queued review whose worker could not run, so a later event for its head sends it again. */ forget(job: TriageJob): void { const host = CODE_REVIEW_WORKFLOWS.find((workflow) => workflow.id === job.workflow)?.source; diff --git a/apps/hub/src/code-review.postgres.test.ts b/apps/hub/src/code-review.postgres.test.ts index 5967cce81..7bc00fde7 100644 --- a/apps/hub/src/code-review.postgres.test.ts +++ b/apps/hub/src/code-review.postgres.test.ts @@ -6,9 +6,9 @@ import { createDB, runMigrations, schema } from "@intx/db"; import { createSidecarPluginRegistry, type SidecarProvisioner } from "@intx/hub-sessions"; import type { SidecarCapabilityDeclaration } from "@intx/types"; import { DOCKER_PLACE, DOCKER_REMOTE_PLACE } from "@corbits/provisioner-docker"; -import type { CodeReviewPlace } from "@corbits/triage-contracts"; +import { emptyPack, type CodeReviewPlace } from "@corbits/triage-contracts"; import { createCodeReviewPlace, createCodeReviewPlacement, createCodeReviewRun, createCodeReviewSwitch } from "./code-review.js"; -import type { Offered } from "./code-review-feed.js"; +import { NoLiveDeploymentError } from "./github/deployment.js"; import { codeReviewSources } from "./github/source-workflow.js"; import { repoRecords, triageNs } from "./github/tenant-config.js"; @@ -102,32 +102,38 @@ test("PostgreSQL: code review turns on only with an isolated place to run, the h expect(repoRecords(triageNs(tenant?.config)).map((record) => [record.name, record.codeReview])).toEqual([["acme/widgets", false], ["TRI/widgets", undefined], ["acme/gone", undefined]]); expect(changed).toEqual([TENANT, TENANT]); - const outcomes: Array = ["sent", "draft", new Error("github GET /repos/acme/widgets/pulls/7 -> 404")]; - const runCodeReview = createCodeReviewRun({ + await db.update(schema.tenant).set({ config: { corbitsTriage: { repos: repos.map((record) => ({ ...record, enabled: true })) } } }).where(eq(schema.tenant.id, TENANT)); + const heads: Array<{ open: boolean; draft: boolean } | Error> = [{ open: true, draft: false }, { open: true, draft: true }, { open: false, draft: false }, new Error("github GET /repos/acme/widgets/pulls/7 -> 404")]; + const sent: unknown[] = []; + const runDeps = { db, authorize: async () => true, - feed: { - async again() { - const next = outcomes.shift()!; - if (next instanceof Error) throw next; - return next; - }, - }, place, - inPrWorkflow: async () => false, - }); - const runNowhere = createCodeReviewRun({ db, authorize: async () => true, feed: { again: async () => "sent" }, place: nowhere, inPrWorkflow: async () => false }); - const runDecided = createCodeReviewRun({ db, authorize: async () => true, feed: { again: async () => "sent" }, place, inPrWorkflow: async () => true }); - expect(((await (await runDecided({ repo: "acme/widgets", number: 7 }, MEMBER)).json()) as { error: { code: string } }).error.code).toBe("policy_file_decides"); + readCheckPack: async () => ({ status: "ok" as const, pack: { ...emptyPack("acme/widgets"), checks: { "no-merge-conflicts": { enabled: true } } }, automatic: [], source: { repo: { id: "art_pack", version: 1 } } }), + async readHead() { + const next = heads.shift()!; + if (next instanceof Error) throw next; + return { title: "Fix", description: "", headSha: "sha1", ...next }; + }, + async send(_tenantId: string, payload: unknown) { + sent.push(payload); + }, + }; + const runCodeReview = createCodeReviewRun(runDeps); + const runNowhere = createCodeReviewRun({ ...runDeps, place: nowhere }); + const runUnlistened = createCodeReviewRun({ ...runDeps, readHead: async () => ({ title: "Fix", description: "", headSha: "sha1", open: true, draft: false }), send: async () => Promise.reject(new NoLiveDeploymentError("triage-listener-github")) }); expect((await runNowhere({ repo: "acme/widgets", number: 7 }, MEMBER)).status).toBe(409); - async function run(repo: string) { - const response = await runCodeReview({ repo, number: 7 }, MEMBER); + async function run(repo: string, runner = runCodeReview) { + const response = await runner({ repo, number: 7 }, MEMBER); return [response.status, ((await response.json()) as { status?: string; error?: { code: string } }).error?.code ?? "queued"]; } expect(await run("acme/missing")).toEqual([404, "repo_not_found"]); expect(await run("acme/widgets")).toEqual([202, "queued"]); + expect(sent).toEqual([expect.objectContaining({ kind: "pr", prNumber: 7, headSha: "sha1", noWait: true, codeReview: { on: true, for: ["everyone"], triage: ["pass", "fail", "unsure"], rules: "any", again: expect.any(String) } })]); expect(await run("acme/widgets")).toEqual([409, "draft"]); + expect(await run("acme/widgets")).toEqual([409, "not_open"]); expect(await run("acme/widgets")).toEqual([502, "code_review_unavailable"]); + expect(await run("acme/widgets", runUnlistened)).toEqual([503, "service_not_running"]); } finally { await handle?.close(); await admin.db.execute(sql.raw(`drop database if exists ${database} with (force)`)); diff --git a/apps/hub/src/code-review.ts b/apps/hub/src/code-review.ts index edea47c07..64d21f544 100644 --- a/apps/hub/src/code-review.ts +++ b/apps/hub/src/code-review.ts @@ -1,14 +1,19 @@ +import { randomUUID } from "node:crypto"; import { type } from "arktype"; import { resolveTenantSidecarCapabilityPolicies, type DB } from "@intx/db"; import type { SidecarPluginRegistry } from "@intx/hub-sessions"; import { CODE_REVIEW_PLACEMENT } from "@corbits/pr-code-review/pr-code-review"; -import { REPO_SOURCES, type CodeReviewPlace, type CodeReviewPlaceOption, type PlaceGuide, type RepoSource } from "@corbits/triage-contracts"; +import { errorText } from "@corbits/common"; +import { EVERYONE, REPO_SOURCES, triageStopped, VERDICTS, type CodeReviewPlace, type CodeReviewPlaceOption, type PlaceGuide, type RepoSource } from "@corbits/triage-contracts"; import { githubLimited } from "./github/github-refusal.js"; import { failure } from "./github/portal-credential.js"; import { connectedSources, hasCodeReviewKey } from "./github/source-workflow.js"; -import { tenantConfigOf } from "./github/bridge.js"; +import { mailPayload, tenantConfigOf } from "./github/bridge.js"; import { patchCorbitsTriage, repoRecords, sourceOf, triageNs, type RepoRecord } from "./github/tenant-config.js"; -import type { CodeReviewFeed, Offered } from "./code-review-feed.js"; +import type { PullRequestHead } from "./code-review-feed.js"; +import type { PolicyReader } from "./github/check-pack-store.js"; +import { NoLiveDeploymentError } from "./github/deployment.js"; +import { triageInputs } from "./github/pr-triage.js"; import type { Member } from "./integrations/app.js"; export const CODE_REVIEW_PATH = "/api/integrations/code-review"; @@ -56,12 +61,6 @@ export function createCodeReviewPlace(deps: CodeReviewPlaceDeps) { }; } -const REFUSED: Record, { code: string; message: string }> = { - not_reviewed: { code: "code_review_off", message: "Code review is off for this repository, or the Anthropic key is missing." }, - draft: { code: "draft", message: "The pull request is a draft; code review starts once it is ready." }, - not_open: { code: "not_open", message: "The pull request is not open." }, -}; - type CodeReviewDeps = { db: DB["db"]; authorize: (principalId: string, tenantId: string, resource: string, action: string) => Promise; @@ -116,26 +115,42 @@ export function createCodeReviewSwitch(deps: CodeReviewDeps) { }; } -type CodeReviewRunDeps = Pick & { feed: Pick }; +type CodeReviewRunDeps = Pick & { + readCheckPack: PolicyReader; + /** The pull request as its host shows it now, without its diff. */ + readHead: (tenantId: string, record: RepoRecord, number: number) => Promise; + /** Hands the mail to the host's listener, whose manager deploys the review. */ + send: (tenantId: string, payload: unknown) => Promise; +}; + +/** Code review as "Review again" asks for it: on, whatever the policy's own scope says, and named as a review of its own. */ +function reviewNow() { + return { on: true, for: [EVERYONE], triage: [...VERDICTS], rules: "any", again: randomUUID() }; +} -/** "Review again": reviews the pull request's current head once more. */ +/** "Review again": triages the pull request's current head with code review on, so its manager reviews it once more. */ export function createCodeReviewRun(deps: CodeReviewRunDeps) { return async function runCodeReview(body: typeof CodeReviewRunBody.infer, { tenantId, principalId }: Member): Promise { if (!(await deps.authorize(principalId, tenantId, `tenant:${tenantId}`, "manage"))) { return failure(403, "forbidden", "You do not have permission to manage this workspace."); } if ((await deps.place(tenantId)) === null) return noPlace(); + if (!(await hasCodeReviewKey(deps.db, tenantId))) return failure(409, "anthropic_key_missing", "Save an Anthropic key in Settings before reviewing again."); const source = body.source ?? "github"; const record = repoRecords(triageNs(await tenantConfigOf(deps, tenantId))).find((row) => row.name === body.repo && sourceOf(row) === source && row.connected); if (!record) return failure(404, "repo_not_found", "The workspace has no such connected repository."); - if (await deps.inPrWorkflow(tenantId, record.name, source)) return fileDecides("Press Triage again to review the latest commit."); - let offered: Offered; + const inputs = await triageInputs(deps, tenantId, record); + if (inputs instanceof Response) return inputs; try { - offered = await deps.feed.again(tenantId, record, body.number); + const pull = await deps.readHead(tenantId, record, body.number); + if (!pull.open) return failure(409, "not_open", "The pull request is not open."); + if (pull.draft) return failure(409, "draft", "The pull request is a draft; code review starts once it is ready."); + const payload = mailPayload(record, inputs.policy, inputs.pack, { prNumber: body.number, headSha: pull.headSha, noWait: true }); + await deps.send(tenantId, { ...payload, codeReview: reviewNow() }); } catch (err) { - return githubLimited(err) ?? failure(502, "code_review_unavailable", `Could not send the pull request to code review: ${err instanceof Error ? err.message : String(err)}`); + if (err instanceof NoLiveDeploymentError) return failure(503, "service_not_running", triageStopped(source)); + return githubLimited(err) ?? failure(502, "code_review_unavailable", `Could not send the pull request to code review: ${errorText(err)}`); } - if (offered !== "sent") return failure(409, REFUSED[offered].code, REFUSED[offered].message); return Response.json({ status: "queued" }, { status: 202 }); }; } diff --git a/apps/hub/src/env.ts b/apps/hub/src/env.ts index aaafe05c2..8d6fbd69a 100644 --- a/apps/hub/src/env.ts +++ b/apps/hub/src/env.ts @@ -70,7 +70,6 @@ const HubEnvSchema = type({ "RAILWAY_GIT_REPO_NAME?": "string", "RAILWAY_ENVIRONMENT_ID?": "string", "TRIAGE_RECONCILE_INTERVAL_MS?": positiveInteger, - "TRIAGE_ROTATE_AFTER_RUNS?": positiveInteger, "TRIAGE_MAX_IN_FLIGHT?": positiveInteger, "TRIAGE_CAPPED_RETRY_MS?": positiveInteger, "TRIAGE_COALESCE_QUIET_MS?": positiveInteger, @@ -255,15 +254,6 @@ export function triageReconcileIntervalMs(env: HubEnv): number { return env.TRIAGE_RECONCILE_INTERVAL_MS === undefined ? 5 * 60_000 : Number(env.TRIAGE_RECONCILE_INTERVAL_MS); } -/** - * The stock hub checks every commit pushed to a deployment's run log against - * the whole log, so the hub replaces the pr-triage deployment with a fresh copy - * once its log holds more runs than this. - */ -export function triageRotateAfterRuns(env: HubEnv): number { - return numberOr(env.TRIAGE_ROTATE_AFTER_RUNS, 15); -} - /** Heads the reconciler lets a tenant have queued or running at once; each runs in a worker sidecar of its own. */ export function triageReconcilePolicy(env: Pick): ReconcilePolicy { return { diff --git a/apps/hub/src/github/backlog-triage.test.ts b/apps/hub/src/github/backlog-triage.test.ts index 83f41db80..434fa8ec8 100644 --- a/apps/hub/src/github/backlog-triage.test.ts +++ b/apps/hub/src/github/backlog-triage.test.ts @@ -3,7 +3,7 @@ import { checkPackName, emptyPack } from "@corbits/triage-contracts"; /** One rule on; with every rule off the hub starts no run. */ const PACK = { ...emptyPack("octocat/hello"), checks: { "no-merge-conflicts": { enabled: true } } }; -import { createGithubBacklog } from "./backlog-triage.js"; +import { createBacklogStart, createGithubBacklog } from "./backlog-triage.js"; import { NoLiveDeploymentError } from "./deployment.js"; /** No deployment holds the credential in these tests. */ @@ -36,7 +36,7 @@ function backlog(repo: Record, sent: unknown[], sendMail = asyn }); } -test("a backlog request mails pr-triage-historical the bridge's payload as a backlog, and only for an enabled repository with a live deployment", async () => { +test("a backlog request mails the listener the bridge's payload as a backlog, and only for an enabled repository with a live listener", async () => { const sent: unknown[] = []; const started = await backlog({ name: REPO, connected: true, enabled: true }, sent)({ repo: REPO }, MEMBER); expect(started.status).toBe(202); @@ -45,8 +45,24 @@ test("a backlog request mails pr-triage-historical the bridge's payload as a bac const disabled = await backlog({ name: REPO, connected: true, enabled: false }, sent)({ repo: REPO }, MEMBER); expect(await disabled.json()).toMatchObject({ error: { code: "repo_not_enabled" } }); const notRunning = await backlog({ name: REPO, connected: true, enabled: true }, sent, async function noDeployment() { - throw new NoLiveDeploymentError("pr-triage-historical"); + throw new NoLiveDeploymentError("triage-listener-github"); })({ repo: REPO }, MEMBER); expect(notRunning.status).toBe(503); expect(sent).toHaveLength(1); }); + +test("a Bitbucket Data Center repository's backlog goes to its listener like a GitHub one's", async () => { + const sent: unknown[] = []; + const start = createBacklogStart({ + async readCheckPack() { + return { status: "ok", pack: PACK, automatic: ["labels"], source: SOURCE }; + }, + async sendMail(_tenant, payload) { + sent.push(payload); + }, + log() {}, + }); + const started = await start(MEMBER.tenantId, { name: "TRI/widgets", source: "bitbucket-datacenter", connected: true, enabled: true }); + expect(started.status).toBe(202); + expect(sent).toEqual([expect.objectContaining({ kind: "backlog", repo: "TRI/widgets", repoSource: "bitbucket-datacenter" })]); +}); diff --git a/apps/hub/src/github/backlog-triage.ts b/apps/hub/src/github/backlog-triage.ts index d662c8ad2..625bee8fd 100644 --- a/apps/hub/src/github/backlog-triage.ts +++ b/apps/hub/src/github/backlog-triage.ts @@ -1,7 +1,9 @@ // Triages every open pull request of a repository again, at a maintainer's request or when a push leaves a valid -// policy file: the hub mails pr-triage-historical the same payload it mails pr-triage, as the tenant's triage sender. +// policy file: the hub hands the listener a backlog mail, which stands for each open pull request that is not a draft, +// as the tenant's triage sender. import { type } from "arktype"; import { errorText } from "@corbits/common"; +import { triageStopped } from "@corbits/triage-contracts"; import { isRunTriggerUnroutable } from "@corbits/webhooks"; import { mailPayload } from "./bridge.js"; import type { PolicyReader } from "./check-pack-store.js"; @@ -26,15 +28,13 @@ export type BacklogStart = (tenantId: string, record: RepoRecord) => Promise { - const route = createPrWorkflowRouter({ - credentials: async () => [], - live: async () => true, - readHead: async () => Promise.reject(new Error("502 Bad Gateway")), - readPull: async () => Promise.reject(new Error("502 Bad Gateway")), - log() {}, - }); + const route = createPauseGate({ readHead: async () => Promise.reject(new Error("502 Bad Gateway")), log() {} }); const { clock, state, mails, coalescer } = harness([], { async sendMail(job) { - if (await route(job)) mails.push(job.payload as Record); + if (await route(job.tenantId, job.record!, job.number, job.payload as Record)) mails.push(job.payload as Record); }, }); await coalescer.accept({ ...delivery("opened", "sha1"), pauseLabel: "hold" }); diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index d6780e329..19312573e 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -29,7 +29,7 @@ import type { PrMail } from "./normalize.js"; import { sourceOf, triageStateKey, type RepoRecord } from "./tenant-config.js"; import type { HeadChecksReader, OpenHeadReader } from "./tenant-open-heads.js"; import { inFlight, waitedOut, waitsForCi, type ReconcilePolicy } from "./reconcile-plan.js"; -import { failHead, queueHead, restoreHead, waitHead, type QueuedHead, type Remembered } from "./triage-queue.js"; +import { deliveryFailure, failHead, queueHead, restoreHead, waitHead, type QueuedHead, type Remembered } from "./triage-queue.js"; import { TriageStateConflictError, type TriageStateStore } from "./triage-state-store.js"; export type CoalesceWindow = { quietMs: number; maxWaitMs: number; burstMs: number }; @@ -266,7 +266,7 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { } catch (err) { if (err instanceof GithubRateLimited) return drop({ ...open, remembered }, entry, "github_rate_limited"); deps.log({ level: "error", msg: "forward_failed", ...entry, error: String(err) }); - await failHead(deps.store, head, `delivery failed: ${String(err)}`, deps.now()); + await failHead(deps.store, head, deliveryFailure(err), deps.now()); return; } deps.log({ level: "info", msg: "forwarded", ...entry, events, workflow: latest.workflow }); diff --git a/apps/hub/src/github/deployment-heal.postgres.test.ts b/apps/hub/src/github/deployment-heal.postgres.test.ts deleted file mode 100644 index c1854987c..000000000 --- a/apps/hub/src/github/deployment-heal.postgres.test.ts +++ /dev/null @@ -1,308 +0,0 @@ -// Requires PostgreSQL at TEST_DB. A pr-triage deployment Interchange failed, -// redeployed by the reconciler from its stored launch spec; and a worker's one -// run, read after the coordinator released the worker. -import { expect, test } from "bun:test"; -import { randomBytes } from "node:crypto"; -import { eq, sql } from "drizzle-orm"; -import { createDB, createWorkflowRunLaunchSpecStore, runMigrations, schema } from "@intx/db"; -import { WorkflowProvisioningError, type PrepareProvisionedWorkflowDeploymentArgs, type WorkflowAllocationService, type WorkflowLifecycleService } from "@intx/hub-sessions"; -import { emptyPack, type PrTriageRow } from "@corbits/triage-contracts"; -import { CAPACITY_UNAVAILABLE } from "../sidecar/host-capacity.js"; -import { createDeploymentRotation, latestDeployments, recentWorkers, resolveLiveDeployments, WORKER_MARKER } from "./deployment.js"; -import { DEFAULT_RECONCILE_POLICY, type ObservedRun } from "./reconcile-plan.js"; -import { createRunRecorder } from "./run-recorder.js"; -import { createTenantObserver, createTenantVersions } from "./tenant-observer.js"; -import { createTriageReconciler } from "./triage-reconciler.js"; -import type { ObservedRuns } from "./triage-runs.js"; -import type { RecordedVerdict } from "./verdict-record-store.js"; - -const TEST_DB = { host: "localhost", port: 5432, user: "postgres", password: "postgres", database: "interchange" }; -const TENANT = "tnt_heal"; -const DOMAIN = "heal.test"; -const WORKFLOW = "pr-triage"; -const REPO = "acme/widgets"; -const NOW = new Date("2026-10-12T12:00:00Z"); -const MINUTE = 60_000; -const CONFIG = { corbitsTriage: { repos: [{ name: REPO, connected: true, enabled: true, installationId: 1 }] } }; -const PACK = { ...emptyPack(REPO), checks: { "no-merge-conflicts": { enabled: true } } }; -const BUNDLE = { - source: { kind: "asset", assetId: "ast_triage", package: { format: "source", commitSha: "c0ffee", packageName: "@corbits/pr-triage-workflow" } }, - entry: "./pr-triage.mjs", - projection: { id: WORKFLOW, stepOrder: ["a"], steps: { a: { kind: "step", id: "a" } }, triggers: [] }, - closure: { schemaVersion: "1", topLevel: [], entries: [] }, - approvedWireHash: "deadbeef", - approvedGrants: [], -}; - -type Anchor = { status: "deployed" | "failed" | "cancelled"; createdAt: Date; endedAt?: Date; cancellationRequestedAt?: Date }; -const HEAD = { number: 8, headSha: "abc123" }; -const NO_RUNS: ObservedRuns = { byRepo: new Map(), runCount: 0, verdicts: [], approved: [] }; -const QUEUED: PrTriageRow = { ...HEAD, status: "queued", attempts: 1, firstSeenAt: ago(60 * MINUTE).toISOString(), queuedAt: ago(6 * MINUTE).toISOString(), updatedAt: ago(6 * MINUTE).toISOString() }; - -/** A worker's log: one run for the head, with its verdict when it completed. */ -function workerRuns(run: ObservedRun): ObservedRuns { - const verdict: RecordedVerdict = { - runId: run.runId, repo: REPO, ...HEAD, startedAt: new Date(run.startedAt), verdictAt: new Date(run.startedAt), settled: true, - state: "awaiting-review", actor: "maintainer", humanGated: false, degraded: null, checks: [], dos: [], commented: false, - }; - return { byRepo: new Map([[REPO, new Map([[`${HEAD.number}@${HEAD.headSha}`, [run]]])]]), runCount: 1, verdicts: run.status === "completed" ? [verdict] : [], approved: [] }; -} - -function ago(ms: number): Date { - return new Date(NOW.getTime() - ms); -} - -async function withHub(body: (hub: ReturnType) => Promise): Promise { - const database = `corbits_heal_${randomBytes(6).toString("hex")}`; - const admin = createDB(TEST_DB); - await admin.db.execute(sql.raw(`create database ${database}`)); - const config = { ...TEST_DB, database }; - let handle: ReturnType | undefined; - try { - await runMigrations(config, { schema: "public" }); - handle = createDB(config); - await handle.db.insert(schema.tenant).values({ id: TENANT, name: "Heal", slug: "heal", domain: DOMAIN, config: CONFIG }); - await handle.db.insert(schema.principal).values({ id: "prn_deployer", tenantId: TENANT, kind: "user", refId: "deployer", status: "active" }); - await handle.db.insert(schema.workflowDefinition).values({ id: "wfd_triage", tenantId: TENANT, name: WORKFLOW }); - await body(harness(handle.db)); - } finally { - await handle?.close(); - await admin.db.execute(sql.raw(`drop database if exists ${database} with (force)`)); - await admin.close(); - } -} - -function harness(db: ReturnType["db"]) { - const launchSpecs = createWorkflowRunLaunchSpecStore(db); - const prepared: PrepareProvisionedWorkflowDeploymentArgs[] = []; - const delivered: { payload: unknown }[] = []; - const logged: Record[] = []; - const rows = new Map(); - const runs = new Map(); - const reads: string[] = []; - const verdicts: RecordedVerdict[] = []; - - async function anchor(runId: string, at: Anchor, worker = false): Promise { - await db.insert(schema.workflowRun).values({ id: runId, anchorRunId: runId, definitionId: "wfd_triage", tenantId: TENANT, ...at }); - await launchSpecs.create({ - anchorRunId: runId, - sessionId: `ses_${runId}`, - deploymentDomain: DOMAIN, - sourceAuthorityPrincipalId: "prn_deployer", - frozenApprovalBundle: BUNDLE, - sourceOfferingIds: ["off_a", "off_b"], - defaultSourceOfferingId: "off_a", - deployContent: { systemPrompt: "Triage pull requests.", ...(worker && { [WORKER_MARKER]: true }) }, - }); - } - - // Runs once the reconciler has read the failed deployment and before it looks for a live one, like a portal deploy landing mid-pass. - const hooks: { beforeRedeploy?: () => Promise; full?: boolean } = {}; - - // Stands in for Interchange's provisioning: the new deployment is an anchor run born `deployed` with its own launch spec. - const allocation = { - async prepareProvisionedDeployment(args: PrepareProvisionedWorkflowDeploymentArgs) { - if (hooks.full) throw new WorkflowProvisioningError(CAPACITY_UNAVAILABLE, "100 MiB free, 512 MiB needed"); - prepared.push(args); - await anchor(args.anchorRunId, { status: "deployed", createdAt: NOW }); - return { anchorRunId: args.anchorRunId, deploymentAddress: `${args.anchorRunId}@${DOMAIN}`, allocationId: "alc", status: "pending" as const }; - }, - } as unknown as WorkflowAllocationService; - - function live() { - return resolveLiveDeployments(db, TENANT, WORKFLOW); - } - - async function liveDeployments() { - await hooks.beforeRedeploy?.(); - hooks.beforeRedeploy = undefined; - return live(); - } - - const observer = createTenantObserver({ - liveDeployments: live, - workers: { deployments: (tenantId, endedSince) => recentWorkers(db, tenantId, WORKFLOW, endedSince), endedWithinMs: 5 * MINUTE }, - async observeRuns(anchorRunId) { - reads.push(anchorRunId); - return runs.get(anchorRunId) ?? NO_RUNS; - }, - recordRuns: createRunRecorder({ - verdicts: { - async record(_tenantId, recorded) { - verdicts.push(...recorded); - }, - async recordApproved() {}, - }, - log: () => {}, - }), - versions: createTenantVersions(), - minIntervalMs: 0, - now: () => NOW, - }); - - const reconcile = createTriageReconciler({ - source: "github", - rotationKey: "rotation", - tenants: async () => [{ id: TENANT, domain: DOMAIN, config: CONFIG }], - liveDeployments, - listener: async () => undefined, - latestDeployments: (tenantId, limit) => latestDeployments(db, tenantId, WORKFLOW, limit), - rotation: { - afterRuns: 1_000, - ...createDeploymentRotation({ db, launchSpecs, allocation, lifecycle: {} as WorkflowLifecycleService, mayDeploy: async () => true }), - }, - openHeadsFor: async () => async () => [{ ...HEAD, updatedAt: ago(60 * MINUTE).toISOString(), draft: false }], - activity: { stale: () => true }, - observeTenant: observer.observe, - managerRuns: async () => NO_RUNS, - headChecks: async () => [], - verdicts: { latest: async () => [] }, - store: { - async load(_tenantId, repo) { - return { rows: rows.get(repo) ?? [], version: null }; - }, - async save(_tenantId, repo, next) { - rows.set(repo, next); - return null; - }, - }, - readCheckPack: async () => ({ status: "ok", pack: PACK, automatic: [], source: { repo: { id: "art_pack", version: 1 } } }), - send: async function send({ payload }) { - delivered.push({ payload }); - }, - policy: DEFAULT_RECONCILE_POLICY, - now: () => NOW, - log: (entry) => logged.push(entry), - }); - - async function update(runId: string, at: Partial): Promise { - await db.update(schema.workflowRun).set(at).where(eq(schema.workflowRun.id, runId)); - } - - return { db, launchSpecs, anchor, update, reconcile, live, prepared, delivered, logged, hooks, rows, runs, reads, verdicts }; -} - -function messages(logged: readonly Record[]): unknown[] { - return logged.map((entry) => entry["msg"]).filter((msg) => String(msg).startsWith("triage_heal") || msg === "triage_deployment_healed"); -} - -test("PostgreSQL: a deployment Interchange failed is deployed again from its launch spec, and the next pass sends its heads", async () => { - await withHub(async (hub) => { - await hub.anchor("run_failed", { status: "failed", createdAt: ago(60 * MINUTE), endedAt: ago(3 * MINUTE) }); - expect(await hub.reconcile()).toEqual({ retry: true }); - expect(hub.prepared).toHaveLength(1); - const copy = hub.prepared[0]!.anchorRunId; - expect(copy).not.toBe("run_failed"); - expect((await hub.live()).map((live) => live.runId)).toEqual([copy]); - expect(hub.logged).toContainEqual(expect.objectContaining({ msg: "triage_deployment_healed", deployment: "run_failed", replacedBy: copy })); - - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.delivered).toHaveLength(1); - expect(hub.prepared).toHaveLength(1); - }); -}, 120_000); - -test("PostgreSQL: a copy the host has no room for yet asks for a pass soon, and is made once there is room", async () => { - await withHub(async (hub) => { - await hub.anchor("run_failed", { status: "failed", createdAt: ago(60 * MINUTE), endedAt: ago(3 * MINUTE) }); - hub.hooks.full = true; - expect(await hub.reconcile()).toEqual({ retry: true }); - expect(hub.prepared).toEqual([]); - expect(messages(hub.logged)).toEqual(["triage_heal_deferred"]); - hub.hooks.full = false; - expect(await hub.reconcile()).toEqual({ retry: true }); - expect(hub.prepared).toHaveLength(1); - }); -}, 120_000); - -test("PostgreSQL: a deployment the portal made during the pass is kept, and no copy is made", async () => { - await withHub(async (hub) => { - await hub.anchor("run_failed", { status: "failed", createdAt: ago(60 * MINUTE), endedAt: ago(3 * MINUTE) }); - hub.hooks.beforeRedeploy = () => hub.anchor("run_portal", { status: "deployed", createdAt: NOW }); - expect(await hub.reconcile()).toEqual({ retry: true }); - expect(hub.prepared).toEqual([]); - expect((await hub.live()).map((live) => live.runId)).toEqual(["run_portal"]); - expect(messages(hub.logged)).toEqual([]); - }); -}, 120_000); - -test("PostgreSQL: a failure under two minutes old is waited on; a cancelled or released deployment is never redeployed", async () => { - await withHub(async (hub) => { - await hub.anchor("run_a", { status: "failed", createdAt: ago(60 * MINUTE), endedAt: ago(30_000) }); - expect(await hub.reconcile()).toEqual({ retry: true }); - await hub.update("run_a", { status: "cancelled", endedAt: ago(10 * MINUTE) }); - expect(await hub.reconcile()).toEqual({ retry: false }); - await hub.update("run_a", { status: "failed", cancellationRequestedAt: ago(11 * MINUTE) }); - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.prepared).toEqual([]); - expect(messages(hub.logged)).toEqual([]); - }); -}, 120_000); - -test("PostgreSQL: three failed deployments of one source within an hour stop the redeploys until the oldest failure is an hour old", async () => { - await withHub(async (hub) => { - await hub.anchor("run_1", { status: "failed", createdAt: ago(50 * MINUTE), endedAt: ago(40 * MINUTE) }); - await hub.anchor("run_2", { status: "failed", createdAt: ago(30 * MINUTE), endedAt: ago(20 * MINUTE) }); - await hub.anchor("run_3", { status: "failed", createdAt: ago(10 * MINUTE), endedAt: ago(5 * MINUTE) }); - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.prepared).toEqual([]); - expect(messages(hub.logged)).toEqual(["triage_heal_capped"]); - - await hub.update("run_1", { endedAt: ago(61 * MINUTE) }); - expect(await hub.reconcile()).toEqual({ retry: true }); - expect(hub.prepared).toHaveLength(1); - expect(messages(hub.logged)).toEqual(["triage_heal_capped", "triage_deployment_healed"]); - }); -}, 120_000); - -test("PostgreSQL: a released worker's completed run records its verdict and settles its head and is read once; a worker released before the window is not read", async () => { - await withHub(async (hub) => { - await hub.anchor("run_listener", { status: "deployed", createdAt: ago(60 * MINUTE) }); - await hub.anchor("run_worker_old", { status: "cancelled", createdAt: ago(12 * MINUTE), endedAt: ago(10 * MINUTE), cancellationRequestedAt: ago(10 * MINUTE) }, true); - hub.runs.set("run_worker_old", workerRuns({ runId: "run_worker_old__events__0", source: "github", status: "completed", startedAt: ago(11 * MINUTE).toISOString() })); - await hub.anchor("run_worker", { status: "cancelled", createdAt: ago(2 * MINUTE), endedAt: ago(MINUTE), cancellationRequestedAt: ago(MINUTE) }, true); - hub.runs.set("run_worker", workerRuns({ runId: "run_worker__events__0", source: "github", status: "completed", startedAt: ago(90_000).toISOString() })); - hub.rows.set(REPO, [QUEUED]); - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.verdicts.map((verdict) => verdict.runId)).toEqual(["run_worker__events__0"]); - expect(hub.rows.get(REPO)![0]).toMatchObject({ status: "triaged", runId: "run_worker__events__0" }); - expect(hub.delivered).toEqual([]); - expect(hub.prepared).toEqual([]); - - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.reads).toEqual(["run_listener", "run_worker", "run_listener"]); - expect(hub.rows.get(REPO)![0]).toMatchObject({ status: "triaged", runId: "run_worker__events__0" }); - }); -}, 120_000); - -test("PostgreSQL: a released worker's failed run fails its head, which is sent again", async () => { - await withHub(async (hub) => { - await hub.anchor("run_listener", { status: "deployed", createdAt: ago(60 * MINUTE) }); - await hub.anchor("run_worker", { status: "cancelled", createdAt: ago(2 * MINUTE), endedAt: ago(MINUTE), cancellationRequestedAt: ago(MINUTE) }, true); - hub.runs.set("run_worker", workerRuns({ runId: "run_worker__events__0", source: "github", status: "failed", startedAt: ago(80_000).toISOString() })); - hub.rows.set(REPO, [QUEUED]); - expect(await hub.reconcile()).toEqual({ retry: false }); - expect(hub.verdicts).toEqual([]); - expect(hub.rows.get(REPO)![0]).toMatchObject({ status: "queued", attempts: 2 }); - expect(hub.delivered).toHaveLength(1); - }); -}, 120_000); - -test("PostgreSQL: a listener published per person, whose definition is named for its asset, is found by its workflow's id", async () => { - await withHub(async (hub) => { - const listener = "triage-listener-github"; - await hub.db.insert(schema.workflowDefinition).values({ id: "wfd_listener", tenantId: TENANT, name: `${listener}-prn-deployer-0123abcd` }); - await hub.db.insert(schema.workflowRun).values({ id: "run_published", anchorRunId: "run_published", definitionId: "wfd_listener", tenantId: TENANT, status: "deployed", createdAt: NOW }); - await hub.launchSpecs.create({ - anchorRunId: "run_published", - sessionId: "ses_run_published", - deploymentDomain: DOMAIN, - sourceAuthorityPrincipalId: "prn_deployer", - frozenApprovalBundle: { ...BUNDLE, projection: { ...BUNDLE.projection, id: listener } }, - sourceOfferingIds: ["off_a"], - defaultSourceOfferingId: "off_a", - deployContent: { systemPrompt: "Route pull requests." }, - }); - expect((await resolveLiveDeployments(hub.db, TENANT, listener)).map((live) => [live.runId, live.workflow])).toEqual([["run_published", listener]]); - }); -}, 120_000); diff --git a/apps/hub/src/github/deployment.ts b/apps/hub/src/github/deployment.ts index 5be30c22b..0e59f898f 100644 --- a/apps/hub/src/github/deployment.ts +++ b/apps/hub/src/github/deployment.ts @@ -5,6 +5,7 @@ import { MAIL_ADMISSION_DENIED_CODE, type WorkflowAllocationService, type Workfl import type { RunTriggerMaterialize } from "@corbits/webhooks"; import type { ToolPackageManifest } from "@intx/types/tool-packages"; import type { FrozenApprovalBundle } from "@intx/types/sidecar"; +import { TRIAGE_LISTENERS, triageStopped, type RepoSource } from "@corbits/triage-contracts"; import { claimRotation, clearRotation, type RotationRecord } from "./tenant-config.js"; export class NoLiveDeploymentError extends Error { @@ -13,6 +14,14 @@ export class NoLiveDeploymentError extends Error { } } +/** A workspace runs no listener for the host, so its pull requests wait for an upgrade; the message says what to press. */ +export class TriageStoppedError extends NoLiveDeploymentError { + constructor(source: RepoSource) { + super(TRIAGE_LISTENERS[source]); + this.message = triageStopped(source); + } +} + export class DeploymentNotReadyError extends Error { readonly code = "deployment_not_ready"; constructor() { @@ -130,21 +139,6 @@ export async function liveWorkers(db: DB["db"]): Promise<{ tenantId: string; run .where(and(eq(workflowRun.id, workflowRun.anchorRunId), inArray(workflowRun.status, [...liveWorkflowRunStatuses]), markedAs("worker"))); } -/** A deployment in any status: `cancelling` tells one released on purpose from one Interchange failed. */ -export type DeploymentRecord = { runId: string; status: (typeof schema.workflowRun.$inferSelect)["status"]; endedAt: Date | null; cancelling: boolean }; - -export async function latestDeployments(db: DB["db"], tenantId: string, definitionName: string, limit: number): Promise { - const { workflowRun, workflowDefinition } = schema; - const rows = await db - .select({ runId: workflowRun.id, status: workflowRun.status, endedAt: workflowRun.endedAt, cancellationRequestedAt: workflowRun.cancellationRequestedAt }) - .from(workflowRun) - .innerJoin(workflowDefinition, eq(workflowRun.definitionId, workflowDefinition.id)) - .where(and(eq(workflowRun.tenantId, tenantId), eq(workflowDefinition.name, definitionName), eq(workflowRun.id, workflowRun.anchorRunId))) - .orderBy(desc(workflowRun.createdAt), desc(workflowRun.id)) - .limit(limit); - return rows.map(({ cancellationRequestedAt, ...row }) => ({ ...row, cancelling: cancellationRequestedAt !== null })); -} - export type ActiveTenant = { id: string; domain: string }; /** 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 18cda0716..8bca1b62e 100644 --- a/apps/hub/src/github/github-read-access.postgres.test.ts +++ b/apps/hub/src/github/github-read-access.postgres.test.ts @@ -6,7 +6,7 @@ import { HUB_CONDITION_REGISTRY } from "@intx/hub-api"; import { createEnvKeyCredentialCipher } from "@intx/crypto"; import { createDB, createGrantStore, dropSchema, runMigrations, schema } from "@intx/db"; import { credentialAad } from "@intx/types"; -import { TRIAGE_ROLE, type ActionKind } from "@corbits/triage-contracts"; +import { TRIAGE_ROLE, triageStopped, type ActionKind } from "@corbits/triage-contracts"; import { doEffectId } from "@corbits/triage-workflows/logic/actions"; import { guardedApp } from "../integrations/test-app.js"; import { createAuditLog } from "../audit-log.js"; @@ -320,7 +320,7 @@ test("PostgreSQL: every verdict route needs workflow-run read, which holding the } }); -test("PostgreSQL: approving a held Do needs its kind's grant and mails pr-triage who approved it; it stands only on the newest judged verdict; a written or unrunnable Do is refused", async () => { +test("PostgreSQL: approving a held Do needs its kind's grant and hands the listener who approved it; it stands only on the newest judged verdict; a written or unrunnable Do is refused, and none goes out without a listener", async () => { await handle.db.insert(schema.workflowDefinition).values({ id: "wfd_triage", tenantId: TENANT, name: "pr-triage", status: "stopped" }); await handle.db.insert(schema.workflowRun).values([ { id: "wfr_anchor", definitionId: "wfd_triage", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "deployed" }, @@ -336,15 +336,17 @@ test("PostgreSQL: approving a held Do needs its kind's grant and mails pr-triage const derived = held("3", "labels", { unresolved: "derive", from: "type" }); const verdict = { repo: REPO, number: 8, headSha: HEAD, request: { repo: REPO, number: 8, labels: [], owned: [], comment: "" }, actions: [ship, tag, written, derived] }; const events = [{ seq: 1, type: "StepCompleted", body: { type: "StepCompleted", seq: 1, stepId: "evaluate", output: { ref: `inline:${JSON.stringify(verdict)}` } } }]; - const mailed: Array<{ address: string; payload: unknown }> = []; + const mailed: unknown[] = []; + let listening = true; let recorded: Array> = []; const heldDos = createGithubHeldDos({ ...deps, runReader: { async readRunEvents() { return events; } }, verdicts: { async forPull() { return recorded as VerdictRecord[]; } }, - async liveDeployments() { return [{ runId: "wfr_anchor", address: "wfr_anchor@read-access.test", createdAt: new Date(), cancelling: false }]; }, - async deliver(_tenantId, address, payload) { mailed.push({ address, payload }); }, - async toListener() { return false; }, + async toListener(_tenantId, payload) { + if (listening) mailed.push(payload); + return listening; + }, async passRequest() { throw new Error("pr-triage's verdict is in its run's log"); }, log() {}, }); @@ -360,11 +362,8 @@ test("PostgreSQL: approving a held Do needs its kind's grant and mails pr-triage expect(await approve("usr_user", tag)).toMatchObject({ status: 403, body: { error: { code: "forbidden" } } }); expect(await approve("usr_approver", tag)).toEqual({ status: 202, body: { status: "sent" } }); expect(mailed).toEqual([{ - address: "wfr_anchor@read-access.test", - payload: { - kind: "approve-do", runId: "wfr_verdict", principalId: "prn_approver", list: "pass", index: 1, - request: verdict.request, step: { effectId: tag.effectId, headSha: HEAD, kind: "labels", target: { labels: ["api"] } }, - }, + kind: "approve-do", runId: "wfr_verdict", principalId: "prn_approver", list: "pass", index: 1, + request: verdict.request, step: { effectId: tag.effectId, headSha: HEAD, kind: "labels", target: { labels: ["api"] } }, }]); recorded = [{ runId: "wfr_verdict", degraded: null, runStatus: "completed" }, { runId: "wfr_degraded", degraded: "error", runStatus: "completed" }, { runId: "wfr_failed", degraded: null, runStatus: "failed" }]; @@ -372,6 +371,9 @@ test("PostgreSQL: approving a held Do needs its kind's grant and mails pr-triage recorded.push({ runId: "wfr_newer", degraded: null, runStatus: "completed" }); expect(await approve("usr_approver", ship)).toMatchObject({ status: 409, body: { error: { code: "verdict_superseded" } } }); expect(mailed).toHaveLength(2); + recorded.pop(); + listening = false; + expect(await approve("usr_approver", ship)).toMatchObject({ status: 503, body: { error: { code: "service_not_running", message: triageStopped("github") } } }); }); test("PostgreSQL: a held Do of a verdict a manager reported is approved from the recorded verdict and the mirror request its pass kept, and goes to the listener; once the run's newer verdict replaced it, it is superseded", async () => { @@ -387,8 +389,6 @@ test("PostgreSQL: a held Do of a verdict a manager reported is approved from the ...deps, runReader: { async readRunEvents() { return [{ seq: 0, type: "RunStarted", body: { type: "RunStarted", seq: 0 } }]; } }, verdicts: { async forPull() { return recorded as VerdictRecord[]; } }, - async liveDeployments() { return []; }, - async deliver() { throw new Error("a manager's approval goes to the listener"); }, async toListener(_tenantId, payload) { listened.push(payload); return true; diff --git a/apps/hub/src/github/held-dos.ts b/apps/hub/src/github/held-dos.ts index e4e7b8f1b..32743a279 100644 --- a/apps/hub/src/github/held-dos.ts +++ b/apps/hub/src/github/held-dos.ts @@ -1,11 +1,11 @@ // Approves a Do a triage run held, or one it failed or could not run -// unattended. The hub checks the clicker holds the Do kind's grant and mails the -// deployment of the workflow that held it, pr-triage or a PR workflow variant, the Do as the run's verdict recorded it, with who -// approved it; the run writes it at most once under its effect id. +// unattended. The hub checks the clicker holds the Do kind's grant and hands the +// host's listener the Do as the run's verdict recorded it, with who approved it; +// the pull request's manager writes it at most once under its effect id. import { and, eq } from "drizzle-orm"; import { type } from "arktype"; import { isRunTriggerUnroutable } from "@corbits/webhooks"; -import { ACTION_KINDS, AGENT_DO, APPROVE_DO_KIND, DoList, repoPolicy, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts"; +import { ACTION_KINDS, AGENT_DO, APPROVE_DO_KIND, DoList, repoPolicy, triageStopped, type ActionKind, type ResolvedTarget } from "@corbits/triage-contracts"; import { schema } from "@intx/db"; import { formatRunAddress } from "@intx/types"; import { WORKFLOW_RUN_REF, workflowRunRepoIdForAddress, type WorkflowRunReader } from "@intx/hub-sessions"; @@ -16,9 +16,7 @@ import { doRefused, failure, type PortalCredentialDeps } from "./portal-credenti import type { Member } from "../integrations/app.js"; import type { PullRequestRef } from "../manager-state.js"; import type { VerdictRecordStore } from "./verdict-record-store.js"; -import { DeploymentNotReadyError, newestLive, type LiveDeployment } from "./deployment.js"; -import { prWorkflowVariantOf } from "./pr-workflow.js"; -import { triageWorkflowFor } from "./source-workflow.js"; +import { DeploymentNotReadyError } from "./deployment.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./tenant-config.js"; import { verdictFor } from "./triage-runs.js"; @@ -43,8 +41,6 @@ const MirrorRequest = type({ repo: "string", number: "number", labels: "string[] export type GithubHeldDosDeps = PortalCredentialDeps & { runReader: Pick; verdicts: Pick; - liveDeployments: (tenantId: string, workflow: string) => Promise; - deliver: (tenantId: string, address: string, payload: unknown) => Promise; /** Hands the approval to the workspace's listener, whose manager runs it; false when the workspace has none. */ toListener: (tenantId: string, payload: unknown) => Promise; /** The mirror request a manager's pass kept with the verdict it posted on the head. */ @@ -83,9 +79,6 @@ function heldDo(verdict: Record, body: Body, principalId: strin return { kind: APPROVE_DO_KIND, runId: body.runId, principalId, list, index, request, step: { effectId, headSha, kind, target } }; } -/** The run that held the Do has no workflow definition, which every run has: the store is broken, so trying again cannot help. */ -class RunWithoutDefinitionError extends Error {} - export function createGithubHeldDos(deps: GithubHeldDosDeps) { async function runEvents(tenantId: string, domain: string, runId: string) { const run = await deps.db.query.workflowRun.findFirst({ @@ -97,22 +90,6 @@ export function createGithubHeldDos(deps: GithubHeldDosDeps) { return { anchorRunId: run.anchorRunId, events: await deps.runReader.readRunEvents(repoId, WORKFLOW_RUN_REF, runId) }; } - /** - * The workflow whose deployment held the Do: a worker is its listener's copy, so it goes by the listener's name. The - * anchor run was just read from the run that held it, and every run has a definition, so a missing one is a broken store. - */ - async function heldBy(tenantId: string, anchorRunId: string): Promise { - const { workflowRun, workflowDefinition } = schema; - const [row] = await deps.db - .select({ name: workflowDefinition.name }) - .from(workflowRun) - .innerJoin(workflowDefinition, eq(workflowRun.definitionId, workflowDefinition.id)) - .where(and(eq(workflowRun.id, anchorRunId), eq(workflowRun.tenantId, tenantId))) - .limit(1); - if (!row) throw new RunWithoutDefinitionError(`run ${anchorRunId} has no workflow definition`); - return row.name; - } - /** An approval stands only on the newest verdict that judged something; a run not recorded yet has none newer that is known. */ async function superseded(tenantId: string, repo: string, number: number, runId: string): Promise { const verdicts = await deps.verdicts.forPull(tenantId, repo, number); @@ -160,21 +137,10 @@ export function createGithubHeldDos(deps: GithubHeldDosDeps) { // A held merge on a Fail or Unsure list may still be approved, and the person approving it is told so. const warning = approval.step.kind === "merge" && approval.list !== "pass" ? `The verdict on this head was ${approval.list}, not pass.` : undefined; try { - if (!(await deps.toListener(tenantId, mailed))) { - const variant = prWorkflowVariantOf(await heldBy(tenantId, run.anchorRunId)); - const heldByVariant = variant?.source === repoSource; - const workflow = heldByVariant ? variant.id : triageWorkflowFor(repoSource).id; - const deployment = newestLive(await deps.liveDeployments(tenantId, workflow)); - if (!deployment && heldByVariant) { - return failure(503, "service_not_running", "Code review held this action, and it is not running now, so the action can't be approved. It runs again once the repository's policy turns a check on, the Anthropic key is saved and code review has a place to run."); - } - if (!deployment) return failure(503, "service_not_running", `The ${workflow} service is not running.`); - await deps.deliver(tenantId, deployment.address, mailed); - } + if (!(await deps.toListener(tenantId, mailed))) return failure(503, "service_not_running", triageStopped(repoSource)); } catch (err) { deps.log({ level: "error", msg: "held_do_approval_failed", ...logged, error: String(err) }); if (err instanceof DeploymentNotReadyError) return failure(409, err.code, err.message); - if (err instanceof RunWithoutDefinitionError) return failure(500, "run_without_definition", `Could not send the approval: ${err.message}.`); const unroutable = isRunTriggerUnroutable(err); return failure(unroutable ? 503 : 502, unroutable ? "service_not_running" : "hub_unavailable", `Could not send the approval: ${errorText(err)}. Try again.`); } diff --git a/apps/hub/src/github/interleave.postgres.test.ts b/apps/hub/src/github/interleave.postgres.test.ts index 2374c7d4d..4ec7f102a 100644 --- a/apps/hub/src/github/interleave.postgres.test.ts +++ b/apps/hub/src/github/interleave.postgres.test.ts @@ -50,9 +50,7 @@ function route(store: TriageStateStore, deliver: Deliver) { authorize: async () => true, pullHead: async () => "abc123", readCheckPack: async () => ({ status: "ok", pack: PACK, automatic: [], source: { repo: { id: "art_pack", version: 1 } } }), - liveDeployments: async () => [DEPLOYMENT], - listener: async () => undefined, - observeRuns: noRuns, + listener: async () => DEPLOYMENT, managerRuns: noRuns, store, send: async ({ payload }) => deliver(payload), @@ -65,12 +63,8 @@ function route(store: TriageStateStore, deliver: Deliver) { function reconciler(store: TriageStateStore, heads: number[], deliver: Deliver, pack: CheckPack = PACK, checks: CheckRun[] = []) { return createTriageReconciler({ source: "github", - rotationKey: "rotation", tenants: async () => [{ id: TENANT, domain: "tenant.example", config: CONFIG }], - liveDeployments: async () => [DEPLOYMENT], - listener: async () => undefined, - latestDeployments: async () => [], - rotation: { afterRuns: 15, redeploy: async () => ({ status: "skipped", reason: "unused" }), sameSource: async () => false, release: async () => {}, claim: async () => false, clear: async () => {} }, + listener: async () => DEPLOYMENT, openHeadsFor: async () => async () => heads.map((number) => ({ number, headSha: number === 8 ? "abc123" : `sha${number}`, updatedAt: LONG_AGO, draft: false })), activity: { stale: () => true }, observeTenant: async () => ({ deployments: [DEPLOYMENT], observed: [await noRuns()], workers: [] }), diff --git a/apps/hub/src/github/pr-triage.test.ts b/apps/hub/src/github/pr-triage.test.ts index 01e28a312..e9fb5e147 100644 --- a/apps/hub/src/github/pr-triage.test.ts +++ b/apps/hub/src/github/pr-triage.test.ts @@ -15,7 +15,7 @@ const CREDENTIAL_ID = "crd_github"; const NOW = new Date("2026-10-08T12:00:00Z"); const PACK = { ...emptyPack(REPO), checks: { "no-merge-conflicts": { enabled: true } } }; const ENABLED = { name: REPO, connected: true, enabled: true }; -const ANCHOR = { runId: "anchor-1", address: "anchor-1@tenant.example", createdAt: NOW, cancelling: false }; +const LISTENER = { runId: "listener-1", address: "listener-1@tenant.example", createdAt: NOW, cancelling: false }; type Delivered = { payload: unknown }; type Stub = { delivered: Delivered[]; saved: PrTriageRow[][]; authorized: unknown[][] }; @@ -29,8 +29,8 @@ function stubDb(repo: Record) { } as never; } -function observed(runs: ObservedRun[]): GithubPrTriageDeps["observeRuns"] & GithubPrTriageDeps["managerRuns"] { - return async function observeRuns() { +function observed(runs: ObservedRun[]): GithubPrTriageDeps["managerRuns"] { + return async function managerRuns() { return { byRepo: new Map([[REPO, new Map([["8@abc123", runs]])]]), runCount: runs.length, verdicts: [], approved: [] }; }; } @@ -72,10 +72,8 @@ function handler(repo: Record, s: Stub, overrides: Partial (number === 8 ? "abc123" : undefined), readCheckPack: async () => ({ status: "ok", pack: PACK, automatic: [], source: { repo: { id: "art_pack", version: 1 } } }), - liveDeployments: async () => [ANCHOR], - listener: async () => undefined, - observeRuns: observed([]), - managerRuns: async () => ({ byRepo: new Map(), runCount: 0, verdicts: [], approved: [] }), + listener: async () => LISTENER, + managerRuns: observed([]), store: stored([], s.saved), send: async ({ payload }) => { s.delivered.push({ payload }); @@ -161,29 +159,21 @@ describe("createGithubPrTriage", () => { const live: ObservedRun = { runId: "run-live", source: "github", status: "running", startedAt: new Date(NOW.getTime() - 60_000).toISOString() }; const stuck: ObservedRun = { runId: "run-stuck", source: "github", status: "running", startedAt: new Date(NOW.getTime() - 2 * DEFAULT_RECONCILE_POLICY.stuckAfterMs).toISOString() }; const s = stub(); - await refused(await handler(ENABLED, s, { observeRuns: observed([live]) })(request(), MEMBER), 409, "already_running"); + await refused(await handler(ENABLED, s, { managerRuns: observed([live]) })(request(), MEMBER), 409, "already_running"); expect(s.delivered).toEqual([]); - expect((await handler(ENABLED, s, { observeRuns: observed([stuck]) })(request(), MEMBER)).status).toBe(202); + expect((await handler(ENABLED, s, { managerRuns: observed([stuck]) })(request(), MEMBER)).status).toBe(202); expect(s.delivered).toHaveLength(1); }); - test("with the host's listener live, mails with no pr-triage deployment and refuses a pull request whose manager is up", async () => { + test("without the host's listener nothing is queued or mailed", async () => { const s = stub(); - const listening: Partial = { listener: async () => ({ ...ANCHOR, runId: "listener-1", address: "listener-1@tenant.example" }), liveDeployments: async () => [] }; - expect((await handler(ENABLED, s, listening)(request(), MEMBER)).status).toBe(202); - expect(s.delivered).toHaveLength(1); - const managed = stub(); - const running: ObservedRun = { runId: "run-manager", source: "github", status: "running", startedAt: NOW.toISOString() }; - await refused(await handler(ENABLED, managed, { ...listening, managerRuns: observed([running]) })(request(), MEMBER), 409, "already_running"); - expect(managed.delivered).toEqual([]); + await refused(await handler(ENABLED, s, { listener: async () => undefined })(request(), MEMBER), 503, "service_not_running"); + expect(s.delivered).toEqual([]); + expect(s.saved).toEqual([]); }); - test("refuses a head still running on a replaced deployment or stored as running, but not one stored running past the stuck time", async () => { - const live: ObservedRun = { runId: "run-live", source: "github", status: "running", startedAt: new Date(NOW.getTime() - 60_000).toISOString() }; - const replaced = { ...ANCHOR, runId: "anchor-0", address: "anchor-0@tenant.example" }; - const onReplaced: GithubPrTriageDeps["observeRuns"] = async (anchorRunId, domain) => (anchorRunId === replaced.runId ? observed([live]) : observed([]))(anchorRunId, domain); + test("refuses a head stored as running, but not one stored running past the stuck time", async () => { const s = stub(); - await refused(await handler(ENABLED, s, { liveDeployments: async () => [ANCHOR, replaced], observeRuns: onReplaced })(request(), MEMBER), 409, "already_running"); const running = (agoMs: number): PrTriageRow => ({ ...queuedRow(agoMs), status: "running", runId: "run-live" }); await refused(await handler(ENABLED, s, { store: stored([running(60_000)], s.saved) })(request(), MEMBER), 409, "already_running"); expect(s.delivered).toEqual([]); @@ -203,7 +193,7 @@ describe("createGithubPrTriage", () => { test("a head a live manager holds is running, and one whose manager reported done or went away since it was queued is queued again at once", async () => { const queued = queuedRow(60_000); - const manager = (run: ObservedRun): Partial => ({ store: stored([queued], s.saved), managerRuns: async () => observed([run])("", "") }); + const manager = (run: ObservedRun): Partial => ({ store: stored([queued], s.saved), managerRuns: observed([run]) }); const s = stub(); await refused(await handler(ENABLED, s, manager({ runId: "run_manager", source: "github", status: "running", startedAt: NOW.toISOString() }))(request(), MEMBER), 409, "already_running"); await refused(await handler(ENABLED, s, manager({ runId: "run_manager", source: "github", status: "completed", startedAt: new Date(NOW.getTime() - 120_000 - DEFAULT_RECONCILE_POLICY.clockSkewMs).toISOString() }))(request(), MEMBER), 409, "already_queued"); diff --git a/apps/hub/src/github/pr-triage.ts b/apps/hub/src/github/pr-triage.ts index 24aa365fa..c21a8edb1 100644 --- a/apps/hub/src/github/pr-triage.ts +++ b/apps/hub/src/github/pr-triage.ts @@ -1,12 +1,11 @@ // Queues one open pull request's head at a maintainer's request, for the host's -// listener while one is live and the live pr-triage deployment otherwise. The -// row is marked queued before the mail goes out so repeated clicks and reconcile +// listener. The row is marked queued before the mail goes out so repeated clicks and reconcile // passes see it; a failed mail puts it back. // Attempts are left alone: a head the hub gave up on is only ever retried here. import { eq } from "drizzle-orm"; import { type } from "arktype"; import { schema } from "@intx/db"; -import { hasEnabledRules, repoPolicy, type PrTriageRow, type RepoSource } from "@corbits/triage-contracts"; +import { hasEnabledRules, repoPolicy, triageStopped, type PrTriageRow, type RepoSource } from "@corbits/triage-contracts"; import { errorText } from "@corbits/common"; import { mailPayload } from "./bridge.js"; import type { TriageJob } from "./coordinator.js"; @@ -18,8 +17,8 @@ import type { ManagerRuns } from "./manager-runs.js"; import { isStuck, runKey, type ObservedRun, type ReconcilePolicy } from "./reconcile-plan.js"; import { triageWorkflowFor } from "./source-workflow.js"; import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; -import { newestLive, type LiveDeployment } from "./deployment.js"; -import { mergeObservedRuns, observeDeployments, type ObservedRuns, type ObserveRuns } from "./triage-runs.js"; +import type { LiveDeployment } from "./deployment.js"; +import type { ObservedRuns } from "./triage-runs.js"; import { isQueuedSince, isRunningSince, queueHead, restoreHead } from "./triage-queue.js"; import type { TriageStateStore } from "./triage-state-store.js"; @@ -31,11 +30,8 @@ export type GithubPrTriageDeps = PortalCredentialDeps & { /** The open pull request's head on the record's host, or undefined when it is not open. */ pullHead: (tenantId: string, record: RepoRecord, number: number) => Promise; readCheckPack: PolicyReader; - /** The named workflow's, newest first; mail goes to the first, and a head may still be running on a replaced one. */ - liveDeployments: (tenantId: string, workflow: string) => Promise; - /** The workspace's live listener for the host, if it has one; its managers then run the pull requests. */ + /** The workspace's live listener for the host, if it has one; its managers run the pull requests. */ listener: (tenantId: string, source: RepoSource) => Promise; - observeRuns: ObserveRuns; managerRuns: ManagerRuns; store: TriageStateStore; send: (job: TriageJob) => Promise; @@ -95,11 +91,9 @@ export function createGithubPrTriage(deps: GithubPrTriageDeps) { const { tenantId, principalId } = member; const ready = await readyToTriage(deps, member, repo); if (ready instanceof Response) return ready; - const { tenant, record, policy, pack } = ready; + const { record, policy, pack } = ready; const triage = triageWorkflowFor(sourceOf(record)); - const listening = (await deps.listener(tenantId, sourceOf(record))) !== undefined; - const deployments = listening ? [] : await deps.liveDeployments(tenantId, triage.id); - if (!listening && !newestLive(deployments)) return failure(503, "service_not_running", `The ${triage.id} service is not running.`); + if (!(await deps.listener(tenantId, sourceOf(record)))) return failure(503, "service_not_running", triageStopped(sourceOf(record))); let openHead: string | undefined; try { @@ -114,8 +108,7 @@ export function createGithubPrTriage(deps: GithubPrTriageDeps) { const now = deps.now(); const key = runKey(number, headSha); const managed = headRuns(await deps.managerRuns(tenantId, [{ source: sourceOf(record), repo, number }], undefined), repo, key); - const runs = [...headRuns(mergeObservedRuns(await observeDeployments(deps.observeRuns, deployments, tenant.domain)), repo, key), ...managed]; - if (runs.some((run) => run.status === "running" && !isStuck(run, now, deps.policy))) { + if (managed.some((run) => run.status === "running" && !isStuck(run, now, deps.policy))) { return failure(409, "already_running", `${repo}#${number} is already being triaged.`); } const head = { tenantId, repo: triageStateKey(record), number, headSha }; diff --git a/apps/hub/src/github/pr-workflow.test.ts b/apps/hub/src/github/pr-workflow.test.ts index 536d19efd..18dc39cbe 100644 --- a/apps/hub/src/github/pr-workflow.test.ts +++ b/apps/hub/src/github/pr-workflow.test.ts @@ -1,43 +1,36 @@ import { expect, test } from "bun:test"; import type { WorkflowRunEvent } from "@intx/hub-sessions"; import { decideOf, defaultActions, WAITING_KIND, type PolicyWorkflow } from "@corbits/triage-contracts"; -import type { TriageJob } from "./coordinator.js"; -import { reviewRequestOf, type PullRequestToReview } from "../code-review-feed.js"; -import { createPrWorkflowRouter, prWorkflowKeys, workflowPart } from "./pr-workflow.js"; +import type { PullRequestToReview } from "../code-review-feed.js"; +import { createPauseGate, prWorkflowKeys, workflowPart } from "./pr-workflow.js"; import type { RepoRecord } from "./tenant-config.js"; import { verdictFor } from "./triage-runs.js"; const DECIDE = [{ if: { triage: "fail" as const }, do: "request-changes" as const }]; const RECORD = { name: "acme/widgets", connected: true } as RepoRecord; const PULL: PullRequestToReview = { title: "t", description: "d", headSha: "abc", draft: false, open: true, diff: "", labels: ["bug"] }; -const REVIEW = reviewRequestOf(RECORD, 8, PULL); type Read = () => Promise; -function router(keys: string[], { live = true, read = async () => PULL, readDiff = read }: { live?: boolean; read?: Read; readDiff?: Read } = {}) { +function gate(read: Read = async () => PULL) { const logged: Array> = []; - const diffsRead: number[] = []; - const route = createPrWorkflowRouter({ - credentials: async () => keys, - live: async () => live, + const pass = createPauseGate({ async readHead() { const { diff: _diff, ...head } = await read(); return head; }, - readPull(_tenantId, _record, number) { - diffsRead.push(number); - return readDiff(); - }, log(entry) { logged.push(entry); }, }); - return { route, logged, diffsRead }; + function route(request: Record) { + return pass("tnt", RECORD, 8, request); + } + return { route, logged }; } -function job(policyWorkflow: PolicyWorkflow | undefined, kind = "pr", pauseLabel?: string): TriageJob { - const payload = { kind, repo: "acme/widgets", automatic: ["comment", "approve", "merge"], ...workflowPart(policyWorkflow), ...(pauseLabel && { pauseLabel }) }; - return { tenantId: "tnt", workflow: "pr-triage-github", repo: "acme/widgets", number: 8, headSha: "abc", source: "event", record: RECORD, payload }; +function job(policyWorkflow: PolicyWorkflow | undefined, kind = "pr", pauseLabel?: string): Record { + return { kind, repo: "acme/widgets", headSha: "abc", automatic: ["comment", "approve", "merge"], ...workflowPart(policyWorkflow), ...(pauseLabel && { pauseLabel }) }; } test("only a block on, or a decide other than the actions', needs the PR workflow, whose trigger carries the blocks, decide and code review's block", () => { @@ -53,75 +46,22 @@ test("only a block on, or a decide other than the actions', needs the PR workflo expect(workflowPart({ decide: DECIDE })).toEqual({ codeReview: { on: false, for: ["everyone"], triage: ["pass", "fail", "unsure"], rules: "pass" }, blocks: [], decide: DECIDE }); }); -test("code review on with the provider's key runs the host's variant, carrying the review of the head; a decide alone reads no diff", async () => { - const routed = await router(["anthropic"]).route(job({ "code-review": { on: true }, decide: DECIDE })); - expect(routed?.workflow).toBe("pr-workflow-anthropic-github"); - expect(routed?.payload).toMatchObject({ blocks: ["code-review"], decide: DECIDE, codeReviewRequest: REVIEW, automatic: ["comment", "approve", "merge"] }); - const decided = await router(["anthropic"]).route(job({ decide: DECIDE })); - expect(decided?.workflow).toBe("pr-workflow-anthropic-github"); - expect(decided?.payload).not.toHaveProperty("codeReviewRequest"); -}); - -test("a diff that cannot be read is sent without its review, which the run records incomplete", async () => { - const { route, logged } = router(["anthropic"], { read: async () => Promise.reject(new Error("host down")) }); - const routed = await route(job({ "code-review": { on: true } })); - expect(routed?.workflow).toBe("pr-workflow-anthropic-github"); - expect(routed?.payload).not.toHaveProperty("codeReviewRequest"); - expect(logged).toMatchObject([{ msg: "pull_request_unread", error: "host down" }]); -}); - -test("without the provider's key, its variant running, or a variant that runs every block on, pr-triage runs with approve and merge left to a person", async () => { - const cases = [ - { keys: [], live: true, workflow: { "code-review": { on: true } }, reason: "no_provider_key" }, - { keys: ["anthropic"], live: true, workflow: { "code-review": { on: true, provider: "openai" as const } }, reason: "no_provider_key" }, - { keys: ["anthropic"], live: true, workflow: { security: { on: true } }, reason: "block_not_runnable" }, - { keys: ["anthropic"], live: false, workflow: { decide: DECIDE }, reason: "variant_not_running" }, - ]; - for (const { keys, live, workflow, reason } of cases) { - const { route, logged } = router(keys, { live }); - const routed = await route(job(workflow)); - expect(routed?.workflow).toBe("pr-triage-github"); - expect(routed?.payload).toMatchObject({ automatic: ["comment"] }); - expect(routed?.payload).not.toHaveProperty("codeReviewRequest"); - expect(logged.map((entry) => [entry.msg, entry.reason])).toEqual([["pr_workflow_unavailable", reason]]); - } -}); - -test("a pull request carrying the pause label starts no run, nor does one whose labels can't be read", async () => { - const paused = router(["anthropic"]); - expect(await paused.route(job(undefined, "pr", "bug"))).toBeUndefined(); - expect(await paused.route(job({ "code-review": { on: true } }, "pr", "bug"))).toBeUndefined(); - expect(paused.logged.map((entry) => entry.msg)).toEqual(["pr_paused", "pr_paused"]); - const unread = router(["anthropic"], { read: async () => Promise.reject(new Error("502 Bad Gateway")) }); +test("a pull request carrying the pause label on GitHub starts no run, and one whose labels can't be read throws to be retried", async () => { + const { route, logged } = gate(); + expect(await route(job(undefined, "pr", "bug"))).toBeUndefined(); + expect(logged.map((entry) => entry.msg)).toEqual(["pr_paused"]); + expect(await route(job(undefined, "pr", "wip"))).toEqual(job(undefined, "pr", "wip")); + const unread = gate(async () => Promise.reject(new Error("502 Bad Gateway"))); await expect(unread.route(job(undefined, "pr", "bug"))).rejects.toThrow("502 Bad Gateway"); - await expect(unread.route(job({ "code-review": { on: true } }, "pr", "bug"))).rejects.toThrow("502 Bad Gateway"); const bitbucket = job(undefined, "pr", "bug"); - const unlabelled = { ...bitbucket, payload: { ...(bitbucket.payload as object), repoSource: "bitbucket-datacenter" } }; + const unlabelled = { ...bitbucket, repoSource: "bitbucket-datacenter" }; expect(await unread.route(unlabelled)).toEqual(unlabelled); - expect(await router(["anthropic"]).route(job(undefined, "pr", "wip"))).toEqual(job(undefined, "pr", "wip")); -}); - -test("with a pause label, a diff that cannot be read still pauses on the head's labels, or is sent without its review", async () => { - const { route, logged } = router(["anthropic"], { readDiff: async () => Promise.reject(new Error("502 Bad Gateway")) }); - expect(await route(job({ "code-review": { on: true } }, "pr", "bug"))).toBeUndefined(); - const routed = await route(job({ "code-review": { on: true } }, "pr", "wip")); - expect(routed?.workflow).toBe("pr-workflow-anthropic-github"); - expect(routed?.payload).not.toHaveProperty("codeReviewRequest"); - expect(logged.map((entry) => entry.msg)).toEqual(["pull_request_unread", "pr_paused", "pull_request_unread"]); -}); - -test("only code review reads the diff; a pause label alone reads the pull request without it", async () => { - const paused = router(["anthropic"]); - expect(await paused.route(job(undefined, "pr", "bug"))).toBeUndefined(); - expect(await paused.route(job({ decide: DECIDE }, "pr", "wip"))).toMatchObject({ workflow: "pr-workflow-anthropic-github" }); - expect(paused.diffsRead).toEqual([]); - await paused.route(job({ "code-review": { on: true } }, "pr", "wip")); - expect(paused.diffsRead).toEqual([8]); }); -test("a policy pr-triage runs, and a waiting mail, leave the job as it is", async () => { - const { route } = router(["anthropic"]); - for (const sent of [job(undefined), job({ "code-review": { on: true } }, WAITING_KIND)]) { +test("a policy writing its own decide leaves approve and merge to a person; code review alone, or a waiting mail, leaves the job as it is", async () => { + const { route } = gate(); + expect(await route(job({ decide: DECIDE }))).toMatchObject({ automatic: ["comment"] }); + for (const sent of [job(undefined), job({ "code-review": { on: true } }), job({ decide: DECIDE }, WAITING_KIND)]) { expect(await route(sent)).toEqual(sent); } }); diff --git a/apps/hub/src/github/pr-workflow.ts b/apps/hub/src/github/pr-workflow.ts index 1cc84e406..ccc758e67 100644 --- a/apps/hub/src/github/pr-workflow.ts +++ b/apps/hub/src/github/pr-workflow.ts @@ -1,12 +1,8 @@ -// Picks the workflow a pull request's mail runs: its host's pr-triage, or, while the policy file turns a block on or -// writes its own `decide`, the PR workflow variant of the provider whose key the workspace stores, which triages, runs -// the blocks and decides once. Only key names are read, never secrets. In doubt nothing approves or merges unattended. +// The policy file's v4 keys as a pull request's mail carries them, and the gate every pull request mail passes before +// the listener: a paused pull request starts no run, and in doubt nothing approves or merges unattended. import { isDeepStrictEqual } from "node:util"; -import { asRecord, errorText } from "@corbits/common"; -import { BLOCK_KINDS, CODE_REVIEW_PROVIDERS, codeReviewProviderOf, decideOf, EVERYONE, repoSourceOf, VERDICTS, type BlockKind, type CheckPack, type CodeReviewProvider, type PolicyWorkflow, type RepoSource } from "@corbits/triage-contracts"; -import { reviewRequestOf, type PullRequestHead, type PullRequestToReview } from "../code-review-feed.js"; -import type { TriageJob } from "./coordinator.js"; -import { GithubRateLimited } from "./github-usage.js"; +import { BLOCK_KINDS, decideOf, EVERYONE, repoSourceOf, VERDICTS, type BlockKind, type CheckPack, type CodeReviewProvider, type PolicyWorkflow, type RepoSource } from "@corbits/triage-contracts"; +import type { PullRequestHead } from "../code-review-feed.js"; import type { RepoRecord } from "./tenant-config.js"; /** `blocks` are those the variant runs; a policy turning any other on runs nowhere yet. */ @@ -48,90 +44,33 @@ export function workflowPart(workflow: PolicyWorkflow | undefined) { return { codeReview, blocks, ...(workflow.decide && { decide: workflow.decide }) }; } -/** The host's variant for the policy's provider, else the first in the default order whose key is stored; undefined without that key. */ -export function variantFor(source: RepoSource, provider: unknown, credentials: readonly string[]): PrWorkflowVariant | undefined { - const chosen = provider ?? codeReviewProviderOf(credentials); - return PR_WORKFLOW_VARIANTS.find((variant) => variant.source === source && variant.provider === chosen && credentials.includes(CODE_REVIEW_PROVIDERS[variant.provider].credential)); -} - -/** Only a person approves or merges a pull request whose policy no running workflow can carry out. */ -const HELD_WITHOUT_VARIANT = new Set(["approve", "merge"]); +/** Only a person approves or merges a pull request whose policy writes its own `decide`, which no running workflow carries out. */ +const HELD_UNDER_DECIDE = new Set(["approve", "merge"]); -export type PrWorkflowRouterDeps = { - /** The names of the workspace's active code review provider credentials. */ - credentials: (tenantId: string) => Promise; - live: (tenantId: string, workflow: string) => Promise; +export type PauseGateDeps = { /** The pull request as its host shows it now, without its diff. */ readHead: (tenantId: string, record: RepoRecord, number: number) => Promise; - /** The same with its diff, read only for code review. */ - readPull: (tenantId: string, record: RepoRecord, number: number) => Promise; log: (entry: Record) => void; }; -function hasDiff(pull: PullRequestHead): pull is PullRequestToReview { - return "diff" in pull; -} - -export function createPrWorkflowRouter(deps: PrWorkflowRouterDeps) { - /** - * Undefined when it cannot be read: the run goes ahead, and records a review it needed incomplete. While a pause can fire, - * a diff that cannot be read falls back to the head, whose labels decide the pause, and a head that cannot be read throws. - */ - async function pullOf(job: TriageJob, record: RepoRecord, reviewed: boolean, pausable: boolean): Promise { - try { - if (reviewed) return await deps.readPull(job.tenantId, record, job.number); - return await deps.readHead(job.tenantId, record, job.number); - } catch (err) { - // An unreadable head cannot be told from a paused one: the mail waits on a rate limit, and otherwise fails to be retried. - if (err instanceof GithubRateLimited || (pausable && !reviewed)) throw err; - deps.log({ level: "warn", msg: "pull_request_unread", tenantId: job.tenantId, repo: job.repo, pr: job.number, headSha: job.headSha, error: errorText(err) }); - return pausable ? await pullOf(job, record, false, true) : undefined; - } - } - - /** The review of the job's head, or undefined when the pull request has moved on or its diff is unusable. */ - function reviewOf(job: TriageJob, record: RepoRecord, pull: PullRequestToReview): object | undefined { - if (pull.headSha !== job.headSha) return undefined; - try { - return reviewRequestOf(record, job.number, pull); - } catch (err) { - deps.log({ level: "warn", msg: "code_review_unread", tenantId: job.tenantId, repo: job.repo, pr: job.number, headSha: job.headSha, error: errorText(err) }); - return undefined; - } - } - - /** The variant that runs the mail, or why none can. */ - async function variantOf(job: TriageJob, request: Record, blocks: readonly unknown[]): Promise { - const variant = variantFor(repoSourceOf(request["repoSource"]), asRecord(request["codeReview"])?.["provider"], await deps.credentials(job.tenantId)); - if (!variant) return "no_provider_key"; - if (!blocks.every((kind) => variant.blocks.some((runs) => runs === kind))) return "block_not_runnable"; - if (!(await deps.live(job.tenantId, variant.id))) return "variant_not_running"; - return variant; - } - - /** Undefined when the pull request carries the policy's pause label: no run starts. */ - return async function route(job: TriageJob): Promise { - const request = asRecord(job.payload); - if (!request || request["kind"] !== "pr") return job; - const blocks = request["blocks"]; +/** + * One pull request's mail as the listener gets it, or undefined when the pull request carries the policy's pause label: + * no run starts. A head whose labels can't be read throws, to be retried. Only `pr` mails are gated. + */ +export function createPauseGate(deps: PauseGateDeps) { + return async function gate(tenantId: string, record: RepoRecord, number: number, request: Record): Promise | undefined> { + if (request["kind"] !== "pr") return request; const pauseLabel = request["pauseLabel"]; // Only GitHub heads carry labels, so only there can a pause fire. - const pausable = typeof pauseLabel === "string" && repoSourceOf(request["repoSource"]) === "github"; - const reviewed = Array.isArray(blocks) && asRecord(request["codeReview"])?.["on"] === true; - if (reviewed && !job.record) deps.log({ level: "warn", msg: "code_review_unread", tenantId: job.tenantId, repo: job.repo, pr: job.number, headSha: job.headSha, reason: "no_record" }); - const pull = job.record && (typeof pauseLabel === "string" || reviewed) ? await pullOf(job, job.record, reviewed, pausable) : undefined; - if (typeof pauseLabel === "string" && pull?.labels?.includes(pauseLabel)) { - deps.log({ level: "info", msg: "pr_paused", tenantId: job.tenantId, repo: job.repo, pr: job.number, headSha: job.headSha, label: pauseLabel }); - return undefined; - } - if (!Array.isArray(blocks)) return job; - const variant = await variantOf(job, request, blocks); - if (typeof variant === "string") { - deps.log({ level: "warn", msg: "pr_workflow_unavailable", tenantId: job.tenantId, repo: job.repo, pr: job.number, headSha: job.headSha, reason: variant }); - const automatic = Array.isArray(request["automatic"]) ? request["automatic"].filter((kind) => !HELD_WITHOUT_VARIANT.has(kind)) : []; - return { ...job, payload: { ...request, automatic } }; + if (typeof pauseLabel === "string" && repoSourceOf(request["repoSource"]) === "github") { + const pull = await deps.readHead(tenantId, record, number); + if (pull.labels?.includes(pauseLabel)) { + deps.log({ level: "info", msg: "pr_paused", tenantId, repo: record.name, pr: number, headSha: request["headSha"], label: pauseLabel }); + return undefined; + } } - const codeReviewRequest = job.record && pull && hasDiff(pull) ? reviewOf(job, job.record, pull) : undefined; - return { ...job, workflow: variant.id, payload: { ...request, ...(codeReviewRequest && { codeReviewRequest }) } }; + if (request["decide"] === undefined) return request; + const automatic = Array.isArray(request["automatic"]) ? request["automatic"].filter((kind) => !HELD_UNDER_DECIDE.has(kind)) : []; + return { ...request, automatic }; }; } diff --git a/apps/hub/src/github/source-workflow.ts b/apps/hub/src/github/source-workflow.ts index e5c47ad5b..f2954460b 100644 --- a/apps/hub/src/github/source-workflow.ts +++ b/apps/hub/src/github/source-workflow.ts @@ -73,7 +73,7 @@ export function entrySource(entry: string): RepoSource | undefined { } /** The names of the code review provider credentials the workspace holds active; names only, never secrets. */ -export async function codeReviewKeys(db: DB["db"], tenantId: string): Promise { +async function codeReviewKeys(db: DB["db"], tenantId: string): Promise { const credentials = Object.values(CODE_REVIEW_PROVIDERS).map((provider) => provider.credential); const rows = await db.query.credential.findMany({ where: and(eq(schema.credential.tenantId, tenantId), eq(schema.credential.status, "active"), inArray(schema.credential.name, credentials)), diff --git a/apps/hub/src/github/triage-queue.ts b/apps/hub/src/github/triage-queue.ts index d739ac9d9..8cbdc12f0 100644 --- a/apps/hub/src/github/triage-queue.ts +++ b/apps/hub/src/github/triage-queue.ts @@ -4,9 +4,15 @@ // coalescer. import { setTimeout as sleep } from "node:timers/promises"; import type { PrTriageRow, TriageEvent } from "@corbits/triage-contracts"; +import { TriageStoppedError } from "./deployment.js"; import { BASE_MOVED, type ReconcilePolicy } from "./reconcile-plan.js"; import { TriageStateConflictError, type TriageStateStore, type TriageStateVersion } from "./triage-state-store.js"; +/** Why a head's mail did not go out: the next step a person takes when Triage is stopped, else the error. */ +export function deliveryFailure(err: unknown): string { + return err instanceof TriageStoppedError ? err.message : `delivery failed: ${String(err)}`; +} + export type QueuedHead = { tenantId: string; repo: string; number: number; headSha: string }; export type Remembered = { prior: PrTriageRow | undefined; rows: PrTriageRow[]; version: TriageStateVersion }; diff --git a/apps/hub/src/github/triage-reconciler.test.ts b/apps/hub/src/github/triage-reconciler.test.ts index dd5c8485d..8031551d9 100644 --- a/apps/hub/src/github/triage-reconciler.test.ts +++ b/apps/hub/src/github/triage-reconciler.test.ts @@ -10,7 +10,6 @@ import { triageReconcilePolicy } from "../env.js"; import { TriageStateConflictError, type TriageStateStore } from "./triage-state-store.js"; import { createRunTriggerDeliverer } from "@corbits/webhooks"; import type { LiveDeployment } from "./deployment.js"; -import type { RotationRecord } from "./tenant-config.js"; import { createRunRecorder } from "./run-recorder.js"; import { createTenantObserver, createTenantVersions } from "./tenant-observer.js"; import type { ApprovedDo, HostedVerdict, RecordedVerdict } from "./verdict-record-store.js"; @@ -187,15 +186,12 @@ type HarnessOptions = { maxInFlight?: string; refuseSaves?: number; onRefusedSave?: (repo: string) => void; - rotateAfterRuns?: number; now?: () => Date; deployments?: LiveDeployment[]; - listener?: LiveDeployment; + /** The host's listener; a live one when not given. */ + listener?: LiveDeployment | undefined; /** The managers' runs by head, read on each pass. */ managed?: () => Record; - whileRedeploying?: (deployments: LiveDeployment[]) => void; - rotation?: RotationRecord; - sameSource?: boolean; pack?: CheckPack; automatic?: ActionKind[]; noPack?: boolean; @@ -216,9 +212,8 @@ type HarnessOptions = { function harness(logs: Logs, prs: OpenPr[] | Record, options: HarnessOptions = {}) { const repos = Array.isArray(prs) ? { [REPO]: prs } : prs; - const records = { rotation: options.rotation }; function tenant() { - const ns = { repos: Object.keys(repos).map((name) => ({ name, connected: true, enabled: true })), ...(records.rotation && { rotation: records.rotation }) }; + const ns = { repos: Object.keys(repos).map((name) => ({ name, connected: true, enabled: true })) }; return { id: TENANT_ID, domain: DOMAIN, config: { corbitsTriage: ns } }; } const { reader, reads, latestOf } = fakeReader(logs); @@ -229,7 +224,6 @@ function harness(logs: Logs, prs: OpenPr[] | Record, options: const listings: string[] = []; const now = options.now ?? (() => NOW); const deployments = (options.deployments ?? [LIVE]).map((live) => ({ ...live })); - const released: string[] = []; const verdicts: RecordedVerdict[] = []; const approved: ApprovedDo[] = []; async function liveDeployments() { @@ -259,38 +253,10 @@ function harness(logs: Logs, prs: OpenPr[] | Record, options: }); const reconcile = createTriageReconciler({ source: "github", - rotationKey: "rotation", tenants: async function tenants() { return [tenant()]; }, - liveDeployments, - listener: async () => options.listener, - latestDeployments: async () => [], - rotation: { - afterRuns: options.rotateAfterRuns ?? 1_000, - async redeploy(_tenantId, anchorRunId) { - const runId = `${anchorRunId}_fresh`; - options.whileRedeploying?.(deployments); - deployments.unshift(deployment(runId, now())); - return { status: "deployed", runId }; - }, - async sameSource() { - return options.sameSource ?? false; - }, - // Like the stock cancellation request, the deployment stays live until the lifecycle sweep stops it. - async release(_tenantId, anchorRunId) { - released.push(anchorRunId); - deployments.find((live) => live.runId === anchorRunId)!.cancelling = true; - }, - async claim(_tenantId, record) { - if (records.rotation) return false; - records.rotation = record; - return true; - }, - async clear(_tenantId, record) { - if (records.rotation?.from === record.from && records.rotation.to === record.to) records.rotation = undefined; - }, - }, + listener: async () => ("listener" in options ? options.listener : LISTENER), openHeadsFor: async function openHeadsFor() { if (options.github === false) return undefined; return async function openHeads(record) { @@ -335,11 +301,7 @@ function harness(logs: Logs, prs: OpenPr[] | Record, options: }, }; } - function sweep(): void { - const stopped = deployments.filter((live) => live.cancelling); - for (const live of stopped) deployments.splice(deployments.indexOf(live), 1); - } - return { reconcile, listings, delivered, saved, logged, reads, latestOf, deployments, released, sweep, records, verdicts, approved, store: store() }; + return { reconcile, listings, delivered, saved, logged, reads, latestOf, deployments, verdicts, approved, store: store() }; } function settled(from: number, count: number): Record { @@ -348,11 +310,14 @@ function settled(from: number, count: number): Record>): unknown[] { - return logged.map((entry) => entry["msg"]).filter((msg) => String(msg).startsWith("triage_deployment_") || String(msg).startsWith("triage_rotation_")); -} - describe("triage reconciler", () => { + test("a host without a live listener is left alone", async () => { + const run = harness({}, [head(1)], { listener: undefined }); + await run.reconcile(); + expect(run.delivered).toEqual([]); + expect(run.logged).toContainEqual(expect.objectContaining({ msg: "triage_reconcile_skipped", reason: "no_listener" })); + }); + test("a host whose listener is live is caught up with no pr-triage deployment", async () => { const run = harness({}, [head(1)], { deployments: [], listener: LISTENER }); await run.reconcile(); @@ -716,10 +681,10 @@ describe("triage reconciler", () => { const pack: CheckPack = { ...emptyPack(REPO), checks: { "no-merge-conflicts": { enabled: true }, focused: { enabled: true } } }; const policy = { enabled: true, checkPack: { name: REPO } }; - async function remailed(related: number, deployments?: LiveDeployment[]) { + async function remailed(related: number, listener = LIVE) { const first = await triageMail(stockTriggerMail({ kind: "pr", repo: REPO, items: [1, 3].map((n) => ({ prNumber: n, headSha: `sha${n}` })), policy, checkPack: pack }), { mergeable: true }, related); const runs = { run_first: [startedBatch([1, 3]), evaluated({ items: first.verdicts }), completed] }; - const { reconcile, delivered, store } = harness({ [repoKey(LIVE.runId)]: runs }, [head(1), head(3)], { pack, ...(deployments && { deployments }) }); + const { reconcile, delivered, store } = harness({ [repoKey(LIVE.runId)]: runs }, [head(1), head(3)], { pack, listener }); await reconcile(); await markBaseMoved(store, TENANT_ID, REPO, 2, NOW, DEFAULT_RECONCILE_POLICY); await reconcile(); @@ -777,8 +742,8 @@ describe("triage reconciler", () => { expect((payload["items"] as Array<{ judged?: object }>).every((item) => item.judged === undefined)).toBe(true); }); - test("answers are not carried to a deployment on another decision-model offering", async () => { - const { payload } = await remailed(0.8, [{ ...deployment("run_new", NOW), offeringId: "off_other" }, LIVE]); + test("answers are not carried to a listener on another decision-model offering", async () => { + const { payload } = await remailed(0.8, { ...deployment("run_new", NOW), offeringId: "off_other" }); expect((payload["items"] as Array<{ judged?: object }>).every((item) => item.judged === undefined)).toBe(true); }); }); @@ -906,139 +871,6 @@ describe("triage reconciler", () => { expect(mailedNumbers(delivered)).toEqual([1, 2, 3, 4, 5]); }); - test("a deployment past its run threshold is replaced once before the pass mails, and released once after its running head settles", async () => { - const live = { ...settled(1, 19), run_going: [started(20, "2026-10-07T11:55:00.000Z")] }; - const logs: Logs = { [repoKey(LIVE.runId)]: live }; - let clock = NOW; - const { reconcile, delivered, saved, deployments, released, logged, sweep, records } = harness(logs, [head(20), head(21)], { rotateAfterRuns: 19, now: () => clock }); - await reconcile(); - expect(deployments.map((live) => live.runId)).toEqual(["run_live_fresh", LIVE.runId]); - expect(records.rotation).toEqual({ from: LIVE.runId, to: "run_live_fresh", at: NOW.toISOString() }); - const fresh = deployments[0]!; - expect(mailedNumbers(delivered)).toEqual([21]); - - await reconcile(); - expect(released).toEqual([]); - - live["run_going"] = [started(20, "2026-10-07T11:55:00.000Z"), rendered(current(20)), completed]; - clock = new Date(NOW.getTime() + DEFAULT_RECONCILE_POLICY.unstartedAfterMs); - await reconcile(); - await reconcile(); - expect(released).toEqual([LIVE.runId]); - expect(saved.get(REPO)!.find((row) => row.number === 20)).toMatchObject({ status: "triaged", runId: "run_going" }); - expect(delivered).toHaveLength(1); - expect(rotationLogs(logged)).toEqual(["triage_deployment_rotated", "triage_deployment_released"]); - - sweep(); - await reconcile(); - expect(records.rotation).toBeUndefined(); - await reconcile(); - expect(deployments.map((live) => live.runId)).toEqual([fresh.runId]); - expect(rotationLogs(logged)).toHaveLength(2); - }); - - test("a busy deployment is not replaced while another is live, and one no rotation names is never released", async () => { - const fresh = deployment("run_fresh", new Date(NOW.getTime() - 2 * DEFAULT_RECONCILE_POLICY.stuckAfterMs)); - const logs: Logs = { [repoKey(fresh.runId)]: settled(1, 20), [repoKey(LIVE.runId)]: settled(30, 1) }; - const { reconcile, deployments, logged, released } = harness(logs, [], { rotateAfterRuns: 15, deployments: [fresh, LIVE] }); - await reconcile(); - expect(deployments.map((live) => live.runId)).toEqual([fresh.runId, LIVE.runId]); - expect(released).toEqual([]); - expect(rotationLogs(logged)).toEqual([]); - }); - - test("a replaced deployment is kept while a head still runs there or its copy is younger than the unstarted grace, and released once a head there is stuck", async () => { - const record = (to: LiveDeployment): RotationRecord => ({ from: LIVE.runId, to: to.runId, at: LONG_AGO }); - const running = { [repoKey(LIVE.runId)]: { run_going: [started(30, "2026-10-07T11:55:00.000Z")] } }; - const settledAgo = deployment("run_fresh", new Date(NOW.getTime() - 2 * DEFAULT_RECONCILE_POLICY.unstartedAfterMs)); - const stillRunning = harness(running, [], { deployments: [settledAgo, LIVE], rotation: record(settledAgo) }); - await stillRunning.reconcile(); - expect(stillRunning.released).toEqual([]); - - const young = deployment("run_fresh", new Date(NOW.getTime() - MINUTE)); - const tooYoung = harness({ [repoKey(LIVE.runId)]: settled(1, 3) }, [], { deployments: [young, LIVE], rotation: record(young) }); - await tooYoung.reconcile(); - expect(tooYoung.released).toEqual([]); - - const stuckLogs = { [repoKey(LIVE.runId)]: { run_stuck: [started(30)] } }; - const longAgo = deployment("run_fresh", new Date(NOW.getTime() - DEFAULT_RECONCILE_POLICY.stuckAfterMs - MINUTE)); - const stuck = harness(stuckLogs, [], { deployments: [longAgo, LIVE], rotation: record(longAgo) }); - await stuck.reconcile(); - await stuck.reconcile(); - expect(stuck.released).toEqual([LIVE.runId]); - expect(rotationLogs(stuck.logged)).toEqual(["triage_deployment_released"]); - }); - - test("two live deployments of the same source with no rotation recorded retire the older one; of different sources, neither", async () => { - const newer = deployment("run_newer", new Date(NOW.getTime() - 2 * DEFAULT_RECONCILE_POLICY.unstartedAfterMs)); - const logs: Logs = { [repoKey(LIVE.runId)]: settled(1, 3) }; - const same = harness(logs, [head(20)], { deployments: [newer, LIVE], sameSource: true }); - await same.reconcile(); - expect(same.records.rotation).toEqual({ from: LIVE.runId, to: newer.runId, at: NOW.toISOString() }); - expect(same.released).toEqual([LIVE.runId]); - - const other = harness(logs, [head(20)], { deployments: [newer, LIVE] }); - await other.reconcile(); - expect(other.records.rotation).toBeUndefined(); - expect(other.released).toEqual([]); - }); - - test("a deployment made while a pass runs is not overtaken by a copy of the one it replaced", async () => { - const logs: Logs = { [repoKey(LIVE.runId)]: settled(1, 19) }; - const portal = deployment("run_portal_v2", new Date(NOW.getTime() + 1)); - let deployed = false; - let state: ReturnType | undefined; - function now(): Date { - // The portal deploys a new workflow version and cancels the old one while this pass runs. - if (state && !deployed) { - deployed = true; - state.deployments.unshift(portal); - state.deployments.splice(state.deployments.findIndex((live) => live.runId === LIVE.runId), 1); - } - return NOW; - } - state = harness(logs, [head(20)], { rotateAfterRuns: 15, now }); - await state.reconcile(); - expect(state.deployments.map((live) => live.runId)).toEqual([portal.runId]); - }); - - test("a copy that finished deploying after another deployment landed is released", async () => { - const logs: Logs = { [repoKey(LIVE.runId)]: settled(1, 19) }; - const portal = deployment("run_portal_v2", NOW); - const { reconcile, deployments, released, records } = harness(logs, [head(20)], { - rotateAfterRuns: 15, - whileRedeploying: (live) => live.unshift({ ...portal }), - }); - await reconcile(); - expect(released).toEqual(["run_live_fresh"]); - expect(records.rotation).toBeUndefined(); - expect(deployments.map((live) => [live.runId, live.cancelling])).toEqual([["run_live_fresh", true], [portal.runId, false], [LIVE.runId, false]]); - }); - - test("a copy that outlived a failed post-deploy check never overtakes the user's newer deployment", async () => { - const logs: Logs = { [repoKey(LIVE.runId)]: settled(1, 19) }; - const portal = deployment("run_portal_v2", new Date(NOW.getTime() - 1)); - let clock = NOW; - const state = harness(logs, [head(20)], { - rotateAfterRuns: 15, - now: () => clock, - whileRedeploying(live) { - // The user's new version lands while the copy probes; the copy's row is inserted later, so it is newer. - live.unshift({ ...portal }); - failNextList = true; - }, - }); - await state.reconcile(); - expect(state.logged.some((entry) => entry["msg"] === "triage_rotation_failed")).toBe(true); - expect(state.deployments.map((live) => [live.runId, live.cancelling])).toEqual([["run_live_fresh", false], [portal.runId, false], [LIVE.runId, false]]); - clock = new Date(NOW.getTime() + DEFAULT_RECONCILE_POLICY.unstartedAfterMs); - await state.reconcile(); - expect(state.released).toEqual(["run_live_fresh"]); - expect(state.records.rotation).toBeUndefined(); - await state.reconcile(); - expect(state.released).toEqual(["run_live_fresh"]); - }); - test("a tenant without a GitHub credential is skipped with a reason", async () => { const { reconcile, delivered, logged } = harness({}, [head(1)], { github: false }); await reconcile(); diff --git a/apps/hub/src/github/triage-reconciler.ts b/apps/hub/src/github/triage-reconciler.ts index 2d6ac692f..b283038cd 100644 --- a/apps/hub/src/github/triage-reconciler.ts +++ b/apps/hub/src/github/triage-reconciler.ts @@ -1,57 +1,37 @@ // Brings every open pull request on an enabled repository to a triaged head // without relying on any webhook, sidecar or portal visit having survived: -// reads GitHub's open pull requests, the live deployments' run logs and the -// stored state, then queues what the plan says, each head to a worker of its own, -// or to the host's listener while one is live. +// reads GitHub's open pull requests, the managers' runs and the stored state, +// then mails what the plan says to the host's listener. A workspace without a +// live listener is left alone. // A head waiting for CI is released here once its CI finished or its wait ran // out, on hosts and pull requests whose CI results send no webhook. import { dependsOnCi, hasEnabledRules, marksWaiting, repoPolicy, type PrTriageRow, type RepoPolicy, type RepoSource, type VerdictDetail } from "@corbits/triage-contracts"; -import { WorkflowProvisioningError } from "@intx/hub-sessions"; -import { CAPACITY_UNAVAILABLE } from "../sidecar/host-capacity.js"; import { mailPayload, unreadablePayload, waitingPayload, type PolicyUnreadable } from "./bridge.js"; import type { TriageJob } from "./coordinator.js"; import { GithubRateLimited } from "./github-usage.js"; import { triageWorkflowFor } from "./source-workflow.js"; -import { failHead } from "./triage-queue.js"; +import { deliveryFailure, failHead } from "./triage-queue.js"; import type { PolicyReader, StoredPack } from "./check-pack-store.js"; import { inFlight, planTenant, requeue, runKey, type OpenPr, type QueuedPr, type ReconcilePolicy, type RepoPlan } from "./reconcile-plan.js"; import { summarizeChecks, type CiState } from "@corbits/github-reads"; -import { repoRecords, rotationRecord, sourceOf, triageNs, triageStateKey, type RepoRecord, type RotationRecord } from "./tenant-config.js"; +import { repoRecords, sourceOf, triageNs, triageStateKey, type RepoRecord } from "./tenant-config.js"; import type { HeadChecksReader, OpenHeadsReader } from "./tenant-open-heads.js"; import type { RepoActivity } from "./repo-activity.js"; -import { newestLive, type DeploymentRecord, type LiveDeployment, type Redeployed } from "./deployment.js"; +import type { LiveDeployment } from "./deployment.js"; import type { ManagerRuns } from "./manager-runs.js"; -import { isRunning, mergeObservedRuns, type ObservedRuns } from "./triage-runs.js"; +import { mergeObservedRuns, type ObservedRuns } from "./triage-runs.js"; import type { TenantObserver } from "./tenant-observer.js"; import type { VerdictRecord, VerdictRecordStore } from "./verdict-record-store.js"; import { TriageStateConflictError, type LoadedTriageState, type TriageStateStore, type TriageStateVersion } from "./triage-state-store.js"; export type ReconcileTenant = { id: string; domain: string; config: unknown }; -type DeploymentRotation = { - afterRuns: number; - redeploy: (tenantId: string, anchorRunId: string) => Promise; - sameSource: (anchorRunId: string, otherAnchorRunId: string) => Promise; - /** Requests cancellation; the deployment stays listed, `cancelling`, until the lifecycle sweep stops it. */ - release: (tenantId: string, anchorRunId: string) => Promise; - claim: (tenantId: string, record: RotationRecord) => Promise; - clear: (tenantId: string, record: RotationRecord) => Promise; -}; - export type TriageReconcilerDeps = { - /** The host whose repositories this reconciler brings to a triaged head; the deployment deps below are its workflow's. */ + /** The host whose repositories this reconciler brings to a triaged head. */ source: RepoSource; - rotationKey: string; tenants: () => Promise; - liveDeployments: (tenantId: string) => Promise; - /** - * The host's live listener, whose managers run its pull requests: the pass then needs no pr-triage deployment, and - * neither rotates nor heals one, so a rotation under way is abandoned. A listener names no workflow version, so no - * head starts over for one. - */ + /** The host's live listener, whose managers run its pull requests; a listener names no workflow version, so no head starts over for one. */ listener: (tenantId: string, source: RepoSource) => Promise; - latestDeployments: (tenantId: string, limit: number) => Promise; - rotation: DeploymentRotation; /** Undefined when the tenant holds no credential for any pull request host. */ openHeadsFor: (tenantId: string) => Promise; /** A repository's open pull requests are listed again only once its last listing may be stale. */ @@ -81,21 +61,11 @@ type TenantPass = { tenantId: string; deployment: LiveDeployment; openHeads: Ope type LoadedRepo = EnabledRepo & (StoredPack | PolicyUnreadable) & { prs: OpenPr[]; stored: LoadedTriageState; ci: Map }; /** - * `retry` asks for another pass soon: a repository's or a tenant's pass failed, or a failed deployment is being redeployed. + * `retry` asks for another pass soon: a repository's or a tenant's pass failed. * `retryAt` is when a GitHub rate limit the pass ran into lifts; no sooner pass could read anything. */ export type ReconcileOutcome = { retry: boolean; retryAt?: number }; -/** A failed deployment is left this long first, so a portal visit can bring one back. */ -const HEAL_AFTER_MS = 2 * 60_000; -/** Failed deployments of one source in a row, within `HEAL_CAP_WINDOW_MS`, after which no more copies are made. */ -const HEAL_CAP = 3; -const HEAL_CAP_WINDOW_MS = 60 * 60_000; - -function failedByInterchange(deployment: DeploymentRecord): deployment is DeploymentRecord & { endedAt: Date } { - return deployment.status === "failed" && !deployment.cancelling && deployment.endedAt !== null; -} - function enabledRepos(config: unknown, source: RepoSource): EnabledRepo[] { return repoRecords(triageNs(config)).flatMap(function enabled(record) { const policy = repoPolicy(record); @@ -320,156 +290,24 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { } catch (err) { if (err instanceof GithubRateLimited) throw err; deps.log({ level: "error", msg: "forward_failed", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, error: String(err) }); - await failHead(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, `delivery failed: ${String(err)}`, deps.now()); + await failHead(deps.store, { tenantId: pass.tenantId, repo: key, number: queued.number, headSha }, deliveryFailure(err), deps.now()); continue; } deps.log({ level: "info", msg: "triage_requeued", tenantId: pass.tenantId, repo: repo.record.name, pr: queued.number, headSha, reason: queued.reason }); } } - /** - * Keeps a recorded rotation, or drops it, and returns the deployment to - * mail: the copy while the rotation stands. The replaced deployment gets no - * new mail; it is released once its heads settled and mail it was sent has - * had time to start, or once a head still running there counts as stuck and - * is queued again on the copy. A deployment newer than the replaced one - * that the hub did not make, such as the portal's of a new version, - * supersedes the copy, which is released. The record is dropped once the - * replaced deployment or the copy is gone, the copy is being cancelled, or - * the copy is superseded. - */ - async function settle(tenantId: string, record: RotationRecord, deployments: readonly LiveDeployment[], runningOn: ReadonlySet, now: Date): Promise { - const from = deployments.find((live) => live.runId === record.from); - const copy = deployments.find((live) => live.runId === record.to); - const superseded = from !== undefined && deployments.some(function newer(live) { - return live.runId !== record.from && live.runId !== record.to && !live.cancelling && live.createdAt > from.createdAt; - }); - if (copy !== undefined && !copy.cancelling && superseded) { - await deps.rotation.release(tenantId, copy.runId); - deps.log({ level: "info", msg: "triage_rotation_superseded", tenantId, deployment: record.from, copy: copy.runId }); - } - if (from === undefined || copy === undefined || copy.cancelling || superseded) { - await deps.rotation.clear(tenantId, record); - return newestLive(superseded ? deployments.filter((live) => live.runId !== record.to) : deployments); - } - const since = now.getTime() - copy.createdAt.getTime(); - const quiet = !runningOn.has(from.runId) && since >= deps.policy.unstartedAfterMs; - if (!from.cancelling && (quiet || since > deps.policy.stuckAfterMs)) { - await deps.rotation.release(tenantId, from.runId); - deps.log({ level: "info", msg: "triage_deployment_released", tenantId, deployment: from.runId, replacedBy: copy.runId }); - } - return copy; - } - - /** - * Two live deployments of the same source with no rotation recorded are a - * rotation whose record was never written, or a portal redeploy whose cancel - * of the older one failed; the newer is recorded as the older one's copy so - * the older one is retired like any replaced deployment. - */ - async function adopt(tenantId: string, deployments: readonly LiveDeployment[], now: Date): Promise { - if (deployments.length !== 2 || deployments.some((live) => live.cancelling)) return undefined; - const [newer, older] = deployments as [LiveDeployment, LiveDeployment]; - if (!await deps.rotation.sameSource(older.runId, newer.runId)) return undefined; - const record = { from: older.runId, to: newer.runId, at: now.toISOString() }; - if (!await deps.rotation.claim(tenantId, record)) return undefined; - deps.log({ level: "info", msg: "triage_rotation_adopted", tenantId, deployment: older.runId, copy: newer.runId }); - return record; - } - - /** - * Returns the deployment this pass mails. The only live deployment is - * replaced by a copy once its log holds more than `afterRuns` runs; the - * rotation is recorded before the copy is mailed, so a later pass, even - * after a restart, releases only what the record names. - */ - async function route(tenant: ReconcileTenant, deployments: readonly LiveDeployment[], observed: readonly ObservedRuns[]): Promise { - const now = deps.now(); - const runningOn = new Set(deployments.filter((_live, index) => isRunning(observed[index]!)).map((live) => live.runId)); - const recorded = rotationRecord(triageNs(tenant.config), deps.rotationKey); - if (recorded) return settle(tenant.id, recorded, deployments, runningOn, now); - const adopted = await adopt(tenant.id, deployments, now); - if (adopted) return settle(tenant.id, adopted, deployments, runningOn, now); - const newest = newestLive(deployments); - const runCount = observed[0]!.runCount; - if (deployments.length !== 1 || newest === undefined || runCount <= deps.rotation.afterRuns) return newest; - // A deployment made while this pass ran, such as the portal's of a newer version, must not be replaced by a copy of the one it superseded. - const current = await deps.liveDeployments(tenant.id); - if (current.length !== 1 || current[0]!.runId !== newest.runId || current[0]!.cancelling) { - deps.log({ level: "info", msg: "triage_rotation_skipped", tenantId: tenant.id, deployment: newest.runId, reason: "deployments_changed" }); - return newestLive(current); - } - const redeployed = await deps.rotation.redeploy(tenant.id, newest.runId); - if (redeployed.status === "skipped") { - deps.log({ level: "warn", msg: "triage_rotation_skipped", tenantId: tenant.id, deployment: newest.runId, reason: redeployed.reason }); - return newest; - } - const record = { from: newest.runId, to: redeployed.runId, at: now.toISOString() }; - if (!await deps.rotation.claim(tenant.id, record)) { - await deps.rotation.release(tenant.id, redeployed.runId); - deps.log({ level: "warn", msg: "triage_rotation_skipped", tenantId: tenant.id, deployment: newest.runId, reason: "rotation_recorded" }); - return newest; - } - deps.log({ level: "info", msg: "triage_deployment_rotated", tenantId: tenant.id, deployment: newest.runId, replacedBy: redeployed.runId, runs: runCount }); - const after = await deps.liveDeployments(tenant.id); - return settle(tenant.id, record, after, runningOn, now); - } - - /** The newest `HEAL_CAP` failed within the window, from the same source. */ - async function healCapped(latest: readonly DeploymentRecord[], now: number): Promise { - if (latest.length < HEAL_CAP) return false; - if (!latest.every((deployment) => failedByInterchange(deployment) && now - deployment.endedAt.getTime() < HEAL_CAP_WINDOW_MS)) return false; - for (const older of latest.slice(1)) { - if (!await deps.rotation.sameSource(latest[0]!.runId, older.runId)) return false; - } - return true; - } - - /** - * With no live deployment, the newest one is deployed again from its launch - * spec when Interchange failed it, rather than someone cancelling it. True - * when a pass soon is worth it: the failure is still recent, a copy was just - * made and its sidecar is starting, or the host had no room for it yet. - */ - async function heal(tenantId: string): Promise { - const latest = await deps.latestDeployments(tenantId, HEAL_CAP); - const failed = latest[0]; - if (failed === undefined || !failedByInterchange(failed)) return false; - const now = deps.now().getTime(); - if (now - failed.endedAt.getTime() < HEAL_AFTER_MS) return true; - if (await healCapped(latest, now)) { - deps.log({ level: "warn", msg: "triage_heal_capped", tenantId, deployment: failed.runId }); - return false; - } - // The portal may have deployed while this pass read; a copy of the old source would then outrank it. - if ((await deps.liveDeployments(tenantId)).length > 0) return true; - let redeployed: Redeployed; - try { - redeployed = await deps.rotation.redeploy(tenantId, failed.runId); - } catch (error) { - const full = error instanceof WorkflowProvisioningError && error.code === CAPACITY_UNAVAILABLE; - deps.log({ level: "warn", msg: full ? "triage_heal_deferred" : "triage_heal_skipped", tenantId, deployment: failed.runId, reason: String(error) }); - return full; - } - if (redeployed.status === "skipped") { - deps.log({ level: "warn", msg: "triage_heal_skipped", tenantId, deployment: failed.runId, reason: redeployed.reason }); - return false; - } - deps.log({ level: "info", msg: "triage_deployment_healed", tenantId, deployment: failed.runId, replacedBy: redeployed.runId }); - return true; - } - - /** True when the pass is worth repeating soon: a repository's pass failed, or a failed deployment is being redeployed. */ + /** True when the pass is worth repeating soon: a repository's pass failed. */ async function reconcileTenant(tenant: ReconcileTenant): Promise { const repos = enabledRepos(tenant.config, deps.source); if (repos.length === 0) return false; const listener = await deps.listener(tenant.id, deps.source); - // Only live deployments' and recent workers' logs are read; heads triaged under a released one stay settled in the stored state until the live deployment runs another workflow version. - const { deployments, observed, workers } = await deps.observeTenant(tenant.id, tenant.domain); - if (!listener && deployments.length === 0) { - deps.log({ level: "warn", msg: "triage_reconcile_skipped", tenantId: tenant.id, reason: "no_live_deployment" }); - return heal(tenant.id); + if (!listener) { + deps.log({ level: "warn", msg: "triage_reconcile_skipped", tenantId: tenant.id, reason: "no_listener" }); + return false; } + // Only live deployments' and recent workers' logs are read; heads triaged under a released one stay settled in the stored state. + const { observed, workers } = await deps.observeTenant(tenant.id, tenant.domain); let openHeads: OpenHeadsReader | undefined; try { openHeads = await deps.openHeadsFor(tenant.id); @@ -483,19 +321,7 @@ export function createTriageReconciler(deps: TriageReconcilerDeps) { return false; } const runs = mergeObservedRuns([...observed, ...workers]); - let deployment = listener ?? newestLive(deployments); - if (!listener) { - try { - deployment = await route(tenant, deployments, observed); - } catch (err) { - deps.log({ level: "error", msg: "triage_rotation_failed", tenantId: tenant.id, error: String(err) }); - } - } - if (!deployment) { - deps.log({ level: "warn", msg: "triage_reconcile_skipped", tenantId: tenant.id, reason: "no_live_deployment" }); - return false; - } - const pass: TenantPass = { tenantId: tenant.id, deployment, openHeads, runs }; + const pass: TenantPass = { tenantId: tenant.id, deployment: listener, openHeads, runs }; const loaded: LoadedRepo[] = []; let failed = false; for (const repo of repos) { diff --git a/apps/hub/src/integrations/app.ts b/apps/hub/src/integrations/app.ts index ea1854785..db9c9f85b 100644 --- a/apps/hub/src/integrations/app.ts +++ b/apps/hub/src/integrations/app.ts @@ -367,7 +367,7 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ `${GITHUB_HELD_DOS_PATH}/:tenantId`, describeRoute({ summary: "Approve a Do a triage run held, failed or could not run", - description: "Needs `use` on the GitHub App credential, and the approve grant for an approval. Mails the repository's pr-triage deployment the Do and who approved it; pr-triage writes the Do at most once under its effect id, and a close, comment or review not once the head has moved.", + description: "Needs `use` on the GitHub App credential, and the approve grant for an approval. Hands the host's listener the Do and who approved it; the pull request's manager writes the Do at most once under its effect id, and a close, comment or review not once the head has moved.", tags: TAGS, responses: { 202: json("Sent to pr-triage", HeldDoSent), @@ -376,7 +376,7 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ 404: json("Run, verdict or Do not found in this workspace", Failure), 409: json("GitHub not connected, repository not enabled, Do already written or not awaiting approval, verdict outdated or superseded", Failure), 502: json("The run trigger failed", Failure), - 503: json("The pr-triage service is not running", Failure), + 503: json("`service_not_running`: Triage is stopped on the host until it is upgraded", Failure), }, }), member, @@ -397,7 +397,7 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ ...invalid, ...failures, 502: json("`source_failed`: the pull request could not be read; `hub_unavailable`: it could not be queued", Failure), - 503: json("`service_not_running`: neither the listener nor the pr-triage service is running; `github_rate_limited`: GitHub's rate limit is reached, `Retry-After` and `resetAt` say when it lifts", Failure), + 503: json("`service_not_running`: Triage is stopped on the host until it is upgraded; `github_rate_limited`: GitHub's rate limit is reached, `Retry-After` and `resetAt` say when it lifts", Failure), }, }), member, @@ -482,14 +482,14 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ `${GITHUB_PR_TRIAGE_PATH}/:tenantId/backlog`, describeRoute({ summary: "Triage a repository's open pull requests again", - description: "Needs `use` on the workspace's GitHub App credential, a connected and enabled repository and a readable policy. Mails pr-triage-historical the payload pr-triage gets.", + description: "Needs `use` on the workspace's GitHub App credential, a connected and enabled repository and a readable policy. Hands the host's listener a backlog mail, which stands for each open pull request that is not a draft.", tags: TAGS, responses: { 202: json("Started", BacklogStarted), ...invalid, ...failures, 502: json("The run trigger failed", Failure), - 503: json("The pr-triage-historical service is not running", Failure), + 503: json("`service_not_running`: Triage is stopped on the host until it is upgraded", Failure), }, }), member, @@ -732,7 +732,7 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ `${CODE_REVIEW_PATH}/:tenantId/run`, describeRoute({ summary: "Review a pull request again", - description: "Needs `manage` on the workspace. Sends the pull request's current head to its host's code review even when that head was reviewed already. Needs a provisioner code review can run on (`no_isolated_place` otherwise). The repository must have code review on and the workspace its Anthropic key. Refuses a repository whose policy file decides code review (`policy_file_decides`). Refuses a draft (`draft`) or a closed pull request (`not_open`).", + description: "Needs `manage` on the workspace. Hands the host's listener a pull request mail for the current head with code review on, so its manager reviews that head once more even when it was reviewed already. Needs a provisioner code review can run on (`no_isolated_place` otherwise) and the workspace's Anthropic key (`anthropic_key_missing`). Refuses a draft (`draft`) or a closed pull request (`not_open`).", tags: TAGS, responses: { 202: json("Queued", CodeReviewQueued), @@ -740,9 +740,9 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ 401: failures[401], 403: failures[403], 404: json("No such connected repository", Failure), - 409: json("`no_isolated_place`, `policy_file_decides`, `code_review_off`, `draft` or `not_open`", Failure), + 409: json("`no_isolated_place`, `anthropic_key_missing`, `draft` or `not_open`", Failure), 502: json("The host or the run trigger failed", Failure), - ...githubLimited, + 503: json("`service_not_running`: Triage is stopped on the host until it is upgraded; `github_rate_limited`: GitHub's rate limit is reached", Failure), }, }), member, diff --git a/apps/hub/src/manager-intake.postgres.test.ts b/apps/hub/src/manager-intake.postgres.test.ts index 8edee8be7..9b3236d07 100644 --- a/apps/hub/src/manager-intake.postgres.test.ts +++ b/apps/hub/src/manager-intake.postgres.test.ts @@ -6,6 +6,7 @@ import { createDB, runMigrations, schema } from "@intx/db"; import { APPROVE_DO_KIND, emptyPack, repoPolicy } from "@corbits/triage-contracts"; import type { PullRequestToReview } from "./code-review-feed.js"; import { mailPayload, waitingPayload } from "./github/bridge.js"; +import { createPauseGate } from "./github/pr-workflow.js"; import { createManagerIntake } from "./manager-intake.js"; import { migrateTriageTables } from "./triage-db.js"; @@ -38,7 +39,13 @@ async function withDatabase(run: (db: Db) => Promise): Promise { } } -function stack(db: Db, listener: string | undefined, pull = PULL) { +type Gate = Parameters[0]["gate"]; + +async function passed(_tenantId: string, _record: unknown, _number: number, request: Record) { + return request; +} + +function stack(db: Db, listener: string | undefined, pull = PULL, gate: Gate = passed) { const mailed: { address: string; payload: unknown }[] = []; const logged: Record[] = []; const toListener = createManagerIntake({ @@ -50,6 +57,7 @@ function stack(db: Db, listener: string | undefined, pull = PULL) { async deliver(_tenantId, address, payload) { mailed.push({ address, payload }); }, + gate, log: (entry) => logged.push(entry), }); return { toListener, mailed, logged }; @@ -153,3 +161,19 @@ test("PostgreSQL: with code review on, a head that moved before intake is record expect(logged).toEqual([expect.objectContaining({ msg: "code_review_head_moved", pr: 7, headSha: "h1", hostHeadSha: "h2" })]); }); }, 60_000); + +test("PostgreSQL: every pull request a mail names passes the gate, whatever sent it: a paused one is not recorded or mailed, and the rest are recorded as the gate leaves them", async () => { + await withDatabase(async function gated(db) { + const gate = createPauseGate({ readHead: async (_tenantId, _record, number) => ({ ...PULL, labels: number === 9 ? ["hold"] : [] }), log() {} }); + const { toListener, mailed } = stack(db, LISTENER, PULL, gate); + const decided = { ...mailPayload(RECORD, POLICY, STORED, {}), pauseLabel: "hold", automatic: ["comment", "approve", "merge"], decide: [{ if: { triage: "fail" }, do: "request-changes" }] }; + // The reconciler's catch-up names its heads as items; Review again and Triage again name one; a backlog names every open one. + expect(await toListener(TENANT, { ...decided, items: [{ prNumber: 7, headSha: "h1" }, { prNumber: 9, headSha: "h9" }] })).toBe(true); + expect(await toListener(TENANT, { ...decided, prNumber: 9, headSha: "h9", codeReview: { on: true, rules: "any" } })).toBe(true); + expect(await toListener(TENANT, { ...decided, kind: "backlog" })).toBe(true); + const recorded = await events(db); + expect(recorded.map((row) => row.state.head)).toEqual(["h1", "h1"]); + expect(recorded.every((row) => JSON.stringify(row.state.payload["automatic"]) === JSON.stringify(["comment"]))).toBe(true); + expect(mailed.flatMap((mail) => (mail.payload as { items: { prNumber: number }[] }).items.map((item) => item.prNumber))).toEqual([7, 7]); + }); +}, 120_000); diff --git a/apps/hub/src/manager-intake.ts b/apps/hub/src/manager-intake.ts index 9dd35a44e..f9d1aa6d1 100644 --- a/apps/hub/src/manager-intake.ts +++ b/apps/hub/src/manager-intake.ts @@ -22,6 +22,8 @@ export type ManagerIntakeDeps = { /** The mail as a GitHub workspace's runs read it, naming the App's bot account. */ withAppLogin: (tenantId: string, payload: Record) => Promise; deliver: (tenantId: string, address: string, payload: unknown) => Promise; + /** The pull request's mail as its manager takes it, or undefined when none is to be taken, as for a paused pull request. */ + gate: (tenantId: string, record: RepoRecord, number: number, request: Record) => Promise | undefined>; log: (entry: Record) => void; }; @@ -107,7 +109,10 @@ export function createManagerIntake(deps: ManagerIntakeDeps) { if (pulls.length === 0) return false; const domain = domainOf(address); const start: Named[] = []; - for (const pull of pulls) { + for (const asked of pulls) { + const request = await deps.gate(tenantId, record, asked.number, asked.request); + if (!request) continue; + const pull = { ...asked, request }; const recorded = await deps.withAppLogin(tenantId, await withReviewRequest(tenantId, record, pull)); await recordEvent(deps.db, tenantId, { source, repo, number: pull.number }, pull.id, { type: "webhook", head: pull.head, payload: recorded as Record }); const runId = await liveManager(deps.db, tenantId, { source, repo, number: pull.number }); diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index d86ee65ea..f72a8a899 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -30,7 +30,8 @@ import { } from "@corbits/cron"; import { E2B_PLACE } from "@corbits/provisioner-e2b"; import { RAILWAY_PLACE } from "@corbits/provisioner-railway"; -import { TRIAGE_MAIL_SENDER, type RepoSource } from "@corbits/triage-contracts"; +import { repoSourceOf, TRIAGE_MAIL_SENDER, type RepoSource } from "@corbits/triage-contracts"; +import { asRecord } from "@corbits/common"; import { createInterchangeHub } from "./interchange-hub.js"; import { createManagerIntake } from "./manager-intake.js"; import { MANAGER_RUN_TOKEN_ROUTES } from "./manager-routes.js"; @@ -52,8 +53,8 @@ import { createIntegrationsApp, INTEGRATIONS_PREFIX, logRefusal } from "./integr import { createBitbucketCodeReviewSend } from "./bitbucket-datacenter/code-review-send.js"; import { createBitbucketIntegration } from "./bitbucket-datacenter/integration.js"; import { checkedBitbucketSpawner, createTenantBitbucket } from "./bitbucket-datacenter/tenant-bitbucket.js"; -import { createPrWorkflowRouter, PR_WORKFLOW_VARIANTS, prWorkflowVariantOf } from "./github/pr-workflow.js"; -import { codeReviewKeys, codeReviewSources, codeReviewWorkflowFor, connectedSources, createSourceDeployments, jobWorkflowsFor, TRIAGE_WORKFLOWS, triageWorkflowFor, triageWorkflowOf, type TriageWorkflow } from "./github/source-workflow.js"; +import { createPauseGate, PR_WORKFLOW_VARIANTS, prWorkflowVariantOf } from "./github/pr-workflow.js"; +import { codeReviewSources, codeReviewWorkflowFor, connectedSources, createSourceDeployments, jobWorkflowsFor, TRIAGE_WORKFLOWS, triageWorkflowFor, triageWorkflowOf, type TriageWorkflow } from "./github/source-workflow.js"; import { createInvitations } from "./invitations/invitations.js"; import { createSmtpEmail } from "./invitations/invite-email.js"; import { createWorkflowDeploy } from "./workflow-deploy.js"; @@ -61,7 +62,7 @@ import { createCodeReviewPlace, createCodeReviewPlacement, createCodeReviewRun, import { createCodeReviewFeed, createPullRequestReader } from "./code-review-feed.js"; import { createCodeReviewRuns } from "./code-review-runs.js"; import { createWorkflowVersions } from "./workflow-versions.js"; -import { bitbucketEgressAllowlist, childProcessEnv, clientIpHeaders, databaseConfig, dockerSettings, e2bSettings, emailSettings, interchangeSettings, githubApiOrigin, loadHubEnv, inviteLinkOrigin, railwaySandboxSettings, signInSettings, triageCoalesceWindow, triageReconcileIntervalMs, triageReconcilePolicy, triageRotateAfterRuns } from "./env.js"; +import { bitbucketEgressAllowlist, childProcessEnv, clientIpHeaders, databaseConfig, dockerSettings, e2bSettings, emailSettings, interchangeSettings, githubApiOrigin, loadHubEnv, inviteLinkOrigin, railwaySandboxSettings, signInSettings, triageCoalesceWindow, triageReconcileIntervalMs, triageReconcilePolicy } from "./env.js"; import { HOOK_MOUNT_PATH } from "./hooks.js"; import { assertMigrated } from "./migrations.js"; import { createBridgeHandler, logJson, withAppLogin } from "./github/bridge.js"; @@ -75,7 +76,7 @@ import { createCoalescer, type Coalescer } from "./github/coalescer.js"; import { createCoordinator, type Coordinator, type TriageJob } from "./github/coordinator.js"; import { createWorkerSpawner, releaseOrphanedWorkers } from "./github/worker-spawn.js"; import { DeliveryCache } from "./github/dedupe.js"; -import { activeTenants, createDeploymentRotation, latestDeployments, NoLiveDeploymentError, readyMaterializer, recentWorkers, systemSenderMaterializer, resolveLiveDeployment, resolveLiveDeployments, withWorkflowVersions, type LiveDeployment } from "./github/deployment.js"; +import { activeTenants, createDeploymentRotation, readyMaterializer, recentWorkers, systemSenderMaterializer, resolveLiveDeployments, TriageStoppedError, withWorkflowVersions, type LiveDeployment } from "./github/deployment.js"; import { createGithubOpenPulls, createMemberOpenPulls, createOpenPullsCache, keptOpenPrs } from "./github/open-pulls.js"; import { createRepoReads } from "./github/repo-reads.js"; import { createGithubPrActions } from "./github/pr-actions.js"; @@ -109,7 +110,7 @@ import { createSettledStatusReader, createTriageRuns } from "./github/triage-run import { createTriageStateStore } from "./github/triage-state-store.js"; import { failHead, markBaseMoved } from "./github/triage-queue.js"; import { createGithubManifestIntegration } from "./github/manifest.js"; -import { prepareCorbitsTriagePatch, repoRecords, sourceOf, tenantManager, triageNs, type RepoRecord } from "./github/tenant-config.js"; +import { prepareCorbitsTriagePatch, repoRecords, sourceOf, tenantManager, triageNs } from "./github/tenant-config.js"; import { workflow as prTriageGithub } from "@corbits/triage-workflows/pr-triage-github"; import { PR_TRIAGE_HISTORICAL_ADDRESS, workflow as prTriageHistoricalWorkflow } from "@corbits/triage-workflows/pr-triage-historical"; import { githubRead } from "@corbits/github-tool"; @@ -333,6 +334,9 @@ const toListener = createManagerIntake({ if (!read) throw new Error("no active source credential"); return read(record); }, + gate: function gateOf(tenantId, record, number, request) { + return pauseGate(tenantId, record, number, request); + }, deliver: deliverToDeployment, log: logJson, }); @@ -466,27 +470,20 @@ async function readyWorkerCredentials(job: TriageJob): Promise { logJson({ level: "error", msg: "github_token_renew_failed", tenantId: job.tenantId, error: String(err) }); } } -/** A hook credential written before hosts names `pr-triage`; its mail goes to the GitHub workflow that replaced it. */ -async function sendBridgeMail(tenantId: string, workflow: string, payload: unknown): Promise { - const id = triageWorkflowOf(workflow)?.id ?? workflow; - const live = await resolveLiveDeployment(composition.db, tenantId, id); - if (!live) throw new NoLiveDeploymentError(id); - await deliverToDeployment(tenantId, live.address, payload); -} -/** The workflow a job's worker is seeded from: a host's triage workflow, by either name it goes by. */ -function workflowIdFor(name: string): string { - const triage = triageWorkflowOf(name); - if (!triage) throw new Error(`Not a triage workflow: ${name}`); - return triage.id; -} /** Every triage and code review mail, whatever started it, runs in a worker of its own. */ async function sendToWorker(job: TriageJob): Promise { coordinator!.enqueue(job); } -/** A paused pull request's job is dropped; the reconciler leaves it alone until its label comes off. A workspace with a listener runs its pull requests in managers. */ async function sendTriageMail(job: TriageJob): Promise { - const routed = await routePrWorkflow({ ...job, workflow: workflowIdFor(job.workflow) }); - if (routed && !(await toListener(routed.tenantId, routed.payload))) await sendToWorker(routed); + await sendToListener(job.tenantId, job.payload); +} +/** + * A workspace's pull requests run in managers its listener deploys; one without a listener runs none. Each pull request + * passes the pause gate at intake, whatever sent it: a paused one is dropped, and the reconciler leaves it alone until its + * label comes off. + */ +async function sendToListener(tenantId: string, payload: unknown): Promise { + if (!(await toListener(tenantId, payload))) throw new TriageStoppedError(repoSourceOf(asRecord(payload)?.["repoSource"])); } const placeCodeReview = createCodeReviewPlacement(composition.db, composition.sidecarPlugins); async function codeReviewPlaced(tenantId: string): Promise { @@ -497,10 +494,9 @@ async function inPrWorkflow(tenantId: string, repo: string, source: RepoSource): const read = await readCheckPack(tenantId, repo, source); return read.status === "ok" && read.policyWorkflow !== undefined; } -// Repositories left switched on send nothing while code review has nowhere to run, nor one the PR workflow or a manager reviews. -async function reviewedSources(tenantId: string, record: RepoRecord) { - if (!(await codeReviewPlaced(tenantId)) || (await inPrWorkflow(tenantId, record.name, sourceOf(record))) || (await listenerAddress(tenantId, sourceOf(record)))) return []; - return codeReviewSources(composition.db, tenantId); +// Code review runs in the managers a listener deploys, and nothing runs without one, so the feed sends nothing. +async function reviewedSources(): Promise { + return []; } /** The hosts whose PR workflow deploys: a connected repository's policy file needs it, and code review has a place to run. */ async function prWorkflowSources(tenantId: string): Promise { @@ -519,17 +515,7 @@ const codeReviewFeed = createCodeReviewFeed({ send: sendToWorker, log: logJson, }); -const routePrWorkflow = createPrWorkflowRouter({ - credentials: function workspaceCodeReviewKeys(tenantId) { - return codeReviewKeys(composition.db, tenantId); - }, - live: async function variantLive(tenantId, workflow) { - return (await resolveLiveDeployment(composition.db, tenantId, workflow)) !== null; - }, - readHead: pullRequests.head, - readPull: pullRequests.withDiff, - log: logJson, -}); +const pauseGate = createPauseGate({ readHead: pullRequests.head, log: logJson }); const tenantSources = { ...credentialDeps, githubApiOrigin: githubOrigin, tenantBitbucket }; const tenantOpenPrs = keptOpenPrs(createTenantOpenPrs(tenantSources), repoReads); const openPullsCache = createOpenPullsCache(now); @@ -641,7 +627,7 @@ function recordingStack(workflowId: string, workerIds: readonly string[] = [work /** One host's observer, run recorder, rotation and reconciler over its own workflow's deployments. */ function triageStack(triage: TriageWorkflow) { const variants = PR_WORKFLOW_VARIANTS.filter((variant) => variant.source === triage.source).map((variant) => variant.id); - const { observer, recordPass, liveDeployments } = recordingStack(triage.id, [triage.id, ...variants]); + const { observer, recordPass } = recordingStack(triage.id, [triage.id, ...variants]); const rotation = createDeploymentRotation({ db: composition.db, launchSpecs, @@ -654,16 +640,10 @@ function triageStack(triage: TriageWorkflow) { }); const reconcile = createTriageReconciler({ source: triage.source, - rotationKey: triage.rotationKey, tenants: reconcileTenants, - liveDeployments, listener: liveListenerOf, headChecks: tenantHeadChecks, verdicts: verdictRecordStore, - latestDeployments: function latestTriageDeployments(tenantId, limit) { - return latestDeployments(composition.db, tenantId, triage.id, limit); - }, - rotation: { afterRuns: triageRotateAfterRuns(env), ...rotation }, openHeadsFor: tenantOpenHeads, activity: repoActivity, observeTenant: observer.observe, @@ -721,9 +701,7 @@ const githubPrTriage = createGithubPrTriage({ return (await openHead(record, number))?.headSha; }, readCheckPack, - liveDeployments: liveTriageDeployments, listener: liveListenerOf, - observeRuns: observeTriageRuns, managerRuns, store: triageStateStore, send: sendTriageMail, @@ -736,8 +714,6 @@ const githubHeldDos = createGithubHeldDos({ authorize: authorizePortal, runReader: composition.runReader, verdicts: verdictRecordStore, - liveDeployments: liveTriageDeployments, - deliver: deliverToDeployment, toListener, passRequest: function managerPassRequest(tenantId, pr, runId, head) { return passRequest(composition.db, tenantId, pr, runId, head); @@ -827,7 +803,7 @@ async function recheckOpenPulls(tenantId: string, repo: string, except: number | return due; } async function sendBacklogMail(tenantId: string, payload: unknown) { - if (!(await toListener(tenantId, payload))) await sendBridgeMail(tenantId, prTriageHistoricalWorkflow.id, payload); + await sendToListener(tenantId, payload); } const deliveryCache = new DeliveryCache({ filePath: `${env.HUB_DATA_DIR}/delivery-cache.json` }); const bridge = createBridgeHandler({ @@ -967,7 +943,7 @@ const integrations = createIntegrationsApp({ db: composition.db, getSession: com }), codeReview: createCodeReviewSwitch({ db: composition.db, authorize: authorizePortal, onChanged: onSourcesChanged, place: placeCodeReview, inPrWorkflow }), codeReviewPlace: createCodeReviewPlace({ authorize: authorizePortal, place: placeCodeReview, prWorkflowSources, probes: [...dockerPlaces(docker), { guide: E2B_PLACE, found: e2b }, { guide: RAILWAY_PLACE, found: railway }] }), - codeReviewRun: createCodeReviewRun({ db: composition.db, authorize: authorizePortal, feed: codeReviewFeed, place: placeCodeReview, inPrWorkflow }), + codeReviewRun: createCodeReviewRun({ db: composition.db, authorize: authorizePortal, place: placeCodeReview, readCheckPack, readHead: pullRequests.head, send: sendToListener }), codeReviewLatest: codeReviewRuns.latestCodeReview, invitations: createInvitations({ ...credentialDeps, diff --git a/apps/web/src/components/settings/TriageUpgrade.tsx b/apps/web/src/components/settings/TriageUpgrade.tsx index cb90db556..bd9c46d82 100644 --- a/apps/web/src/components/settings/TriageUpgrade.tsx +++ b/apps/web/src/components/settings/TriageUpgrade.tsx @@ -20,7 +20,7 @@ function publisherName(principalId: string | null, people: readonly HubPrincipal /** One sentence per connected host; the host is named only when there are several. */ function hostText(release: TriageRelease, host: HostRelease, people: readonly HubPrincipal[], named: boolean): string { const on = named ? ` on ${hostFullName(host.source)}` : ""; - if (!host.running) return `Triage${on} runs on its old workflow, which is being retired. Upgrade to keep it working.`; + if (!host.running) return `Triage is stopped${on} until it is upgraded.`; const what = host.running.version === undefined ? "the new workflow" : `version ${host.running.version}`; const ran = `Triage${on} runs ${what}, published by ${publisherName(host.running.publisherPrincipalId, people)}.`; if (host.live > 1) return `${ran} Another copy is still running too; upgrade to stop it.`; diff --git a/apps/web/src/lib/pull-link.ts b/apps/web/src/lib/pull-link.ts index 7b714419b..2d70f8a0c 100644 --- a/apps/web/src/lib/pull-link.ts +++ b/apps/web/src/lib/pull-link.ts @@ -1,9 +1,7 @@ -import type { RepoSource } from "@corbits/triage-contracts"; +import { HOST_FULL_NAMES, type RepoSource } from "@corbits/triage-contracts"; const HOST_NAMES: Record = { github: "GitHub", "bitbucket-datacenter": "Bitbucket" }; -const HOST_FULL_NAMES: Record = { github: "GitHub", "bitbucket-datacenter": "Bitbucket Data Center" }; - export function hostName(source: RepoSource): string { return HOST_NAMES[source]; } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0a59f55a1..ec59b26d4 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -59,9 +59,10 @@ GitHub ──webhook──► Hub ──spawn──► Sidecar ──► pr-tria and export routes take the repository's host as `source`; without it, a name connected on both hosts is refused. - **Coalescer**: one pull request's burst of webhook events goes out as one - mail to the tenant's live pr-triage deployment for the repository's host, - as the tenant system sender, the same path cron uses, naming the host as - `repoSource`. When the policy depends on CI, a head whose CI is still + mail to the tenant's live listener for the repository's host, as the + tenant system sender, the same path cron uses, naming the host as + `repoSource`. A workspace with no live listener runs nothing: the head + fails with what to press, **Upgrade to new version** in Settings. When the policy depends on CI, a head whose CI is still running is not mailed: its triage-state row turns `waiting` and keeps the window's events (`triage_waiting_ci`). The window that sees CI finish, or its first check fail, mails the head once with those events; a head with @@ -70,8 +71,9 @@ GitHub ──webhook──► Hub ──spawn──► Sidecar ──► pr-tria When a head starts waiting and the role holds Labels (GitHub) or Comment (Bitbucket), the hub also mails it once with `kind: "waiting"`, which only mirrors the waiting marker. - - **Reconciler**: one per host. On an interval it compares each enabled - repository's open pull requests with the run logs and stored state, and + - **Reconciler**: one per host, for workspaces whose listener for the host + is live. On an interval it compares each enabled repository's open pull + requests with the managers' runs, the run logs and stored state, and re-queues every head without a verdict, and every stale head at most once per 15 minutes, under a per-tenant in-flight cap. A repository's open pull requests are listed again only once a webhook named it, until a minute @@ -83,21 +85,12 @@ GitHub ──webhook──► Hub ──spawn──► Sidecar ──► pr-tria releases Bitbucket heads, whose builds send no webhook, and any head whose `check_run` never arrived. Moving a head from `waiting` to `queued` is a compare-and-swap write, so a check run and the reconciler releasing it - together start one run. A catch-up mail carries the decision-model - answers from the head's last verdict when the deployment runs the same - workflow version and decision-model offering. It also replaces a pr-triage - deployment once its run log grows past `TRIAGE_ROTATE_AFTER_RUNS`, and - redeploys one Interchange failed, after two minutes and at most three - times an hour. While the host's listener is live, a pass needs no - pr-triage deployment, neither replaces nor redeploys one (a replacement - under way is abandoned, its copy left running), carries no answers, and - mails its heads through the manager intake. A listener counts as live as - a manager does: running, on its sidecar, or deployed within the last two - minutes; one deployed without a sidecar past that is released, so the next - Upgrade deploys it again. The workflow-versions route reports each - deployment's `live` the same way, which Settings reads. - - **Coordinator**: every triage mail (webhook, retry, reconciler) and every - code review mail is a job in one in-memory queue keyed per lane (triage or + together start one run. It mails its heads to the listener. A listener + counts as live as a manager does: running, on its sidecar, or deployed + within the last two minutes; one deployed without a sidecar past that is + released, so the next Upgrade deploys it again. The workflow-versions route + reports each deployment's `live` the same way, which Settings reads. + - **Coordinator**: every standalone code review mail is a job in one in-memory queue keyed per lane (triage or code review), host and pull request. Each job runs in a worker deployment of its own, copied from the host's live deployment, which only seeds workers, and released once its run ends. How many run at once is the @@ -216,10 +209,11 @@ any other tenant or an unknown sender. connected host. A deployment of the earlier `pr-triage` workflow is released once `pr-triage-github` is live. - `pr-triage-historical`: one run that triages every open pull request of a - GitHub repository, skipping drafts. The hub mails it through its backlog - route when triage is turned on, when a push to the default branch changes the - policy file of an enabled repository and leaves it valid, or when someone clicks **Triage open pull requests - again**, and records its verdicts like pr-triage's. + GitHub repository, skipping drafts. The hub no longer mails it: its backlog + route, run when triage is turned on, when a push to the default branch + changes the policy file of an enabled repository and leaves it valid, or + when someone clicks **Triage open pull requests again**, mails the listener + instead. - `repo-scan`: one run that reads a repository's most recently updated closed pull requests (200 by default) and measures what maintainers merged and closed. Not deployed by the hub yet. @@ -518,7 +512,11 @@ is built from the files' patches. The files a cut leaves unread, read from the diff's headers, ride as `cut.unread` and never reach a prompt; the review sent to GitHub lists them. The title and description are read as the facts step reads them (`shownBody` and `visibleText` in `@corbits/triage-contracts`), so -hidden text never reaches the review. `…/run` reviews the current head again. +hidden text never reaches the review. `…/run` (Review again) records a pull +request mail for the current head with code review on for everyone, on any +verdict and whatever the rules say, and names it with an `again` id. The +manager deploys one more review of a head for each `again` it has not +reviewed, even where an earlier review of the head settled. Code review only reads: a checkout action downloads the head through the host credential to `/checkouts////`, and its tools are the host read bundle and repository reads confined to that @@ -644,39 +642,12 @@ alone holds nothing back: triage's execute is one step after the mirror and scope. The definition binds both pipelines' credentials and, like code review, requires `isolation:workload`. -The hub picks the workflow per mail, wherever the mail came from -(`github/pr-workflow.ts`). A policy file needs the PR workflow when it turns a -block on or its `decide` is not the one the parser derives from its `actions`; -only then does the stored policy keep its v4 keys, and its pull request mails -carry `codeReview`, `blocks` and `decide`. Such a `pr` mail goes to the host's -variant for code review's `provider`, else the first provider in the default -order whose key is stored; only credential names are read. With code review -on, the hub reads the diff, through the repository record the job carries, -into `codeReviewRequest`, and leaves it out when the head moved or the read -failed, so the review is incomplete. The mail runs in a worker copied from the -variant's listener, like pr-triage's. Without that key, with a block on that -the variant does not run (`security`, `deslop`), or with no live variant -listener, pr-triage runs it with approve and merge taken out of `automatic`, -so they wait in Needs you, and the hub logs `pr_workflow_unavailable`. Saved -rules carry no v4 keys, so they always run pr-triage. - -The coordinator keeps one triage job per pull request whichever workflow it -names, so a newer head's job replaces a queued one and a deferred job a newer -head replaced is dropped. In a repository the PR workflow reviews, the file -alone decides code review: the standalone feed skips it, and the code review -switch and "Review again" refuse with `policy_file_decides`. A host's -pr-triage observer also reads its variants' workers, so their runs settle its -heads, and reads a run's Dos from `decide`: the Dos it made, then evaluate's it -did not make that wait, those `decide` left held with its reason and those -evaluate held itself, such as for a missing grant. A held Do's approval goes to -the listener of the workflow whose run held it; with that listener released, -the approval is refused. The portal deploys a variant while a connected -repository's policy file needs it (`prWorkflow` on the code review place -route) and the provider's key is stored, redeploys it with the key or the -decision model, and releases it once no repository needs it or either is -removed; the hub releases it once its host is disconnected or code review has -no place to run. The separate `pr-code-review` deployments still run beside -it. +No deployment of the PR workflow is reached any more: every pull request mail +goes to the host's listener, whose manager runs triage and code review as +stages of their own. A policy file that turns a block on keeps its v4 keys in +the stored policy, and its pull request mails carry `codeReview`, `blocks` +and `decide`; the manager does not run `decide`, so the hub takes approve and +merge out of such a mail's `automatic` and they wait in Needs you. ## Tenancy and authority @@ -897,12 +868,14 @@ read it, and passes the rest of the request to stock. kind (`code-review` or `judge`), the head it reviewed and its result, a code review's output or a judge's whole verdict, or `failed` with why it has none. -- In a workspace with a host's listener live, pull request, waiting, - held-Do approval and backlog mail goes to it instead of a pr-triage worker, - pr-triage-historical or the deployment that held the Do - (`apps/hub/src/manager-intake.ts`). The hub records one webhook event per - pull request the mail names (a reconciler's `items`, a backlog's open - pull requests), with the head's code review request when the policy turns +- Pull request, waiting, held-Do approval and backlog mail goes to the + host's listener (`apps/hub/src/manager-intake.ts`), on both hosts. Each + pull request the mail names (a reconciler's `items`, a backlog's open pull + requests) passes one gate first, whatever sent the mail: on GitHub, a pull + request carrying the policy's pause label is dropped, and a policy file + that writes its own `decide` leaves approve and merge to a person, since no + workflow carries it out. The hub records one webhook event per pull request + that passed, with the head's code review request when the policy turns code review on and the App's login on GitHub, then mails the listener the heads. When the host's head moved past the mail's, the event goes without a review request and the hub logs `code_review_head_moved`; the new head's @@ -910,8 +883,11 @@ read it, and passes the rest of the request to stock. the coalescer's mail carries, so a redelivery records nothing new; any other mail is a new event, so a head mailed again, as after its base moved or its CI settled, - is triaged again. The code review feed sends nothing - for that host. A workspace with no listener routes as before. + is triaged again. The code review feed sends nothing. A workspace with no + live listener for the host mails nothing: Triage again, Review again, a + held Do's approval and backlog triage answer 503 `service_not_running`, and + a webhook's head fails, each saying Triage is stopped on that host until + someone presses **Upgrade to new version** in Settings. - Webhook replay protection uses the `@corbits/webhooks` table and a delivery-id cache in `HUB_DATA_DIR`. diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index a8173357f..1a00300b9 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -352,7 +352,7 @@ On Bitbucket Data Center labels have no effect; see [Bitbucket Data Center repos ### Code review -Code review reads each new commit of a pull request for bugs. It runs whether or not triage is on for the repository, and skips drafts. Switch on **Review the code in every pull request for bugs** to start it, and off to stop. In a repository whose [policy file](#writing-the-file) turns a check on or writes its own `decide`, the file decides instead: changing the switch, or pressing **Review again**, says **This repository's policy file decides code review, which runs with triage.** +Code review reads each new commit of a pull request for bugs. It runs whether or not triage is on for the repository, and skips drafts. Switch on **Review the code in every pull request for bugs** to start it, and off to stop. In a repository whose [policy file](#writing-the-file) turns a check on or writes its own `decide`, the file decides instead: changing the switch says **This repository's policy file decides code review, which runs with triage.** **Review again** still reviews the latest commit once more. ![The popup Code review needs an Anthropic key, with the Anthropic key field, You won't see it again, Cancel and Save and turn on](images/27.png) @@ -465,8 +465,6 @@ The bar at the bottom of the pane always offers the next step first. For a passi Anything you do here is also written to the audit log. -An action that code review held can be approved only while code review runs. If it has stopped, because the policy file no longer turns a check on, the Anthropic key was removed or code review has no place to run, approving it says **Code review held this action, and it is not running now, so the action can't be approved.** Set code review up again, or press **Triage again**. - ### What the author sees Authors do not use the portal. Depending on the repository's actions, they see a comment from Triage on their pull request with the verdict and reasons, review requests, and on GitHub labels such as **Triage: Ready**, **Triage: Not Ready** or **Triage: Needs Human Review** and assignments. On Bitbucket they see the verdict as the **Corbits Triage** build status instead of a label. **Triage: Ready** comes off as soon as Triage sees a new commit and returns once that commit passes, so it never stands on a commit Triage has not checked. While Triage waits for CI, they see **Triage: Waiting for CI** in its place, or no Triage label when the commit has no checks; **Triage: Not Ready** and **Triage: Needs Human Review** stay until the new verdict. When Triage could not read the pull request, or the repository's policy, it shows **Triage: Failed to Run** in place of any verdict label, or on Bitbucket a failed **Corbits Triage** build status that says **Triage couldn't run**, and posts nothing else. It does this only where Triage adds labels on its own, or on Bitbucket comments on its own. It clears on the next run that can read the pull request, which shows the verdict again. Triage keeps one comment and edits it in place as the pull request changes, so an author who pushes a fix sees the fresh verdict in the same comment. Each change Triage asks to split out names what it is not part of: the pull request's title. When new commits change the verdict, the comment also says what the verdict was, how many commits arrived since and which pull requests were merged into the branch. Only when someone changes the repository's policy and that changes the verdict on the same commit does Triage post a new comment, which says it supersedes the one above. @@ -493,7 +491,7 @@ The first line says how many repositories Triage is running on, with a **Choose In the new version, each pull request gets one manager that posts a single update. Once a source and a decision model are set up, a line below the status says which version runs, one line per connected host (named when there are two): -- **Triage runs on its old workflow, which is being retired. Upgrade to keep it working.** That host has not switched yet. +- **Triage is stopped until it is upgraded.** That host has not switched yet, so nothing is triaged there until someone presses **Upgrade to new version**. - **Triage runs version 0.1.0-sha-66ee60f0, published by Ana.** That host runs the new version, published by that person. When this portal has a newer release, it adds **A new version is ready.** If an earlier copy is somehow still running beside it, it adds **Another copy is still running too; upgrade to stop it.** Owners and Deployers (see [People](#people)) see **Upgrade to new version** whenever an upgrade is due. Pressing it publishes the new version under your name, starts it on each host, and then stops the versions started before it, whoever published them. If two people upgrade at once, the newer start stays, and the other person sees **Someone else's upgrade finished first; Triage runs their version.** The button says what it is doing. If the new version does not start within five minutes, it is stopped, the one already running keeps running, and the error names the host. Everyone else sees **Ask a workspace owner or a Deployer to upgrade.** **Advanced info** lists this portal's release, who published the running one and when, and the version of each workflow in the release. diff --git a/docs/SELF_HOST.md b/docs/SELF_HOST.md index 00c6697dd..9bbfcb0b1 100644 --- a/docs/SELF_HOST.md +++ b/docs/SELF_HOST.md @@ -279,7 +279,6 @@ All optional; the defaults suit most installs. | `TRIAGE_RECONCILE_INTERVAL_MS` | 300000 | How often the hub re-queues open pull requests whose latest commit has no verdict. | | `TRIAGE_MAX_IN_FLIGHT` | 5 | Pull request heads a workspace may have queued or running at once. | | `TRIAGE_CAPPED_RETRY_MS` | 21600000 | How long a head that ran out of attempts waits before a fresh set. | -| `TRIAGE_ROTATE_AFTER_RUNS` | 15 | Runs a pr-triage deployment's log may hold before the hub deploys a fresh copy. | | `TRIAGE_COALESCE_QUIET_MS` | 30000 | Quiet time before a pull request's webhook events go out as one run, at most four times this after the first. A new commit, an opened, reopened or ready pull request, an approval or a change request waits only 3 seconds. Must stay below 150000. | ### Hub and sidecars diff --git a/packages/triage-contracts/src/repo-source.ts b/packages/triage-contracts/src/repo-source.ts index 218521ce4..c95973255 100644 --- a/packages/triage-contracts/src/repo-source.ts +++ b/packages/triage-contracts/src/repo-source.ts @@ -5,3 +5,11 @@ export type RepoSource = (typeof REPO_SOURCES)[number]; export function repoSourceOf(value: unknown): RepoSource { return value === "bitbucket-datacenter" ? value : "github"; } + +/** Each host as the product names it in full, such as in Settings. */ +export const HOST_FULL_NAMES: Record = { github: "GitHub", "bitbucket-datacenter": "Bitbucket Data Center" }; + +/** What a person reads while a host runs no Triage, naming the button that starts it. */ +export function triageStopped(source: RepoSource): string { + return `Triage is stopped on ${HOST_FULL_NAMES[source]} until it is upgraded. Press Upgrade to new version in Settings.`; +} diff --git a/packages/triage-manager/src/actions/manager.ts b/packages/triage-manager/src/actions/manager.ts index 62b726bc9..9e1de79ec 100644 --- a/packages/triage-manager/src/actions/manager.ts +++ b/packages/triage-manager/src/actions/manager.ts @@ -4,7 +4,8 @@ import { APPROVE_DO_KIND, CodeReviewOutput, JudgeVerdict, StageResult, VerdictFa import { decided } from "@corbits/triage-workflows/logic/settled"; import { answered, callHub, endRun, managerMailOf, tenantPath, type ManagerMail } from "./hub.js"; -export const Reviewer = type({ kind: "string > 0", head: "string > 0", runId: "string > 0", deployedBy: "string > 0", deadline: "string", "result?": "unknown" }); +/** `again` names the Review again a code review was deployed for. */ +export const Reviewer = type({ kind: "string > 0", head: "string > 0", runId: "string > 0", deployedBy: "string > 0", deadline: "string", "again?": "string > 0", "result?": "unknown" }); export type Reviewer = typeof Reviewer.infer; const TakenEvent = type({ key: "string", event: { type: "string", head: "string", payload: "Record" } }); diff --git a/packages/triage-manager/src/actions/review.ts b/packages/triage-manager/src/actions/review.ts index d62837dfb..d0cccc3c8 100644 --- a/packages/triage-manager/src/actions/review.ts +++ b/packages/triage-manager/src/actions/review.ts @@ -12,7 +12,7 @@ import { Reviewer, TakeOutput } from "./manager.js"; /** How long the manager waits for a code review it deployed, from its deploy to its report. */ export const REVIEW_DEADLINE_MS = 60 * 60_000; -const CodeReviewPart = type({ on: "boolean", rules: type.enumerated(...RULE_GATES) }); +const CodeReviewPart = type({ on: "boolean", rules: type.enumerated(...RULE_GATES), "again?": "string > 0" }); const ScopeInput = type({ payload: "unknown", take: { output: TakeOutput }, evaluateRules: { output: "unknown" } }); /** @@ -21,18 +21,22 @@ const ScopeInput = type({ payload: "unknown", take: { output: TakeOutput }, eval * a code review for the host. The pass that deploys * it, and every pass before its review is in, posts nothing, so the verdict and the review go up together; one that * reported a failure or ran past its deadline posts the verdict alone. A later pass on a head whose review settled posts - * again and deploys no other. + * again and deploys no other. A Review again names itself as `again`, so its head is reviewed once more for it, whatever + * earlier reviews of the head said. */ export async function reviewScope(input: unknown): Promise<{ deploy: boolean; post: boolean; reviewed: boolean }> { const parsed = ScopeInput(input); if (parsed instanceof type.errors) throw new Error(`reviewScope: ${parsed.summary}`); const { taken, head, review, reviewFailed, reviewers } = parsed.take.output; const part = CodeReviewPart(taken?.["codeReview"]); + const again = part instanceof type.errors ? undefined : part.again; const reviewed = !(part instanceof type.errors) && part.on && rulesGate(part.rules, asRecord(parsed.evaluateRules.output)?.["checks"]).open; const published = publishedFor(repoSourceOf(managerMailOf(parsed.payload).pullRequest.source), "codeReview") !== undefined; - const own = reviewers.filter((reviewer) => reviewer.kind === "code-review" && reviewer.head === head); + const own = reviewers.filter((reviewer) => reviewer.kind === "code-review" && reviewer.head === head && (again === undefined || reviewer.again === again)); const settled = own.some((reviewer) => reviewer.result !== undefined || Date.parse(reviewer.deadline) <= Date.now()); - if (!reviewed || !published || review || reviewFailed || settled || taken?.["codeReviewRequest"] === undefined) return { deploy: false, post: true, reviewed }; + // A report on this pass is from an earlier review until the one deployed for this Review again is out. + const reported = (review !== undefined || reviewFailed !== undefined) && (again === undefined || own.length > 0); + if (!reviewed || !published || reported || settled || taken?.["codeReviewRequest"] === undefined) return { deploy: false, post: true, reviewed }; return { deploy: own.length === 0, post: false, reviewed }; } @@ -50,10 +54,11 @@ export async function deployReview(input: unknown, ctx: EffectContext, _signal: if (head === undefined || !taken) throw new Error("deployReview: the pass names no head"); const release = publishedFor(repoSourceOf(pullRequest.source), "codeReview"); if (!release) throw new Error("deployReview: release.json publishes no code review for the host"); + const again = asRecord(taken["codeReview"])?.["again"]; const reviewRun = await deploy(ctx, "deploy", tenantId, release); const deadline = new Date(Date.now() + REVIEW_DEADLINE_MS).toISOString(); const content = JSON.stringify({ ...asRecord(taken["codeReviewRequest"]), tenantId, reply: { to: runId, content: manageMail(tenantId, pullRequest) } }); - return { reviewer: { kind: "code-review", head, runId: reviewRun, deployedBy: runId, deadline }, mail: { tenantId, to: reviewRun, content, readyBy: deadline } }; + return { reviewer: { kind: "code-review", head, runId: reviewRun, deployedBy: runId, deadline, ...(typeof again === "string" && { again }) }, mail: { tenantId, to: reviewRun, content, readyBy: deadline } }; } const MailedInput = type({ deployReview: { output: { reviewer: Reviewer } }, mailReview: { output: { reply: "string" } } }); diff --git a/packages/triage-manager/src/triage-manager.test.ts b/packages/triage-manager/src/triage-manager.test.ts index 0e00f1825..eb9468b07 100644 --- a/packages/triage-manager/src/triage-manager.test.ts +++ b/packages/triage-manager/src/triage-manager.test.ts @@ -402,6 +402,24 @@ describe("triage-manager with code review", () => { expect(hub.calls).toEqual([TAKE, { ...DONE, body: expect.objectContaining({ ...PULL, reviewers: [{ ...reviewer(), result: REVIEW }] }) }, END]); }); + test("Review again on a head whose review settled deploys one more review for it, and that review's report is posted", async () => { + const { writes, host } = githubHost(); + const mailed: unknown[] = []; + const again = hubRequest(RULES, { codeReview: { ...CODE_REVIEW.codeReview, again: "again_1" }, ...REVIEW_REQUEST }); + const settled = { ...reviewer(), runId: "run_earlier", result: REVIEW }; + const first = fakeHub({ takes: [{ ...PASS, request: again, events: [mailEvent(HEAD, again, "evt_2")], reviewers: [settled] }], dones: [ENDED] }); + expect((await runMail(reviewed, wake(), host, first.hub, mailed)).ran).toContain("deployReview"); + expect(writes).toEqual([]); + expect(mailed).toHaveLength(1); + expect(first.calls.at(-2)).toMatchObject({ ...DONE, body: { reviewers: [settled, { kind: "code-review", head: HEAD, runId: DEPLOYED, again: "again_1" }] } }); + + const second = fakeHub({ takes: [{ ...reported({ result: REVIEW }, again), reviewers: [settled, { ...reviewer(), again: "again_1" }] }], dones: [ENDED] }); + const { ran } = await runMail(reviewed, wake(), host, second.hub, mailed); + expect(ran).not.toContain("deployReview"); + expect(writes).toContainEqual(POSTED_REVIEW); + expect(mailed).toHaveLength(1); + }); + test("a stored review is not posted once this pass's policy no longer asks for code review", async () => { const { writes, host } = githubHost(); const hub = fakeHub({ takes: [{ ...PASS, reviewers: [{ ...reviewer(), result: REVIEW }] }], dones: [ENDED] });