diff --git a/apps/hub/src/bitbucket-datacenter/policy-pull.test.ts b/apps/hub/src/bitbucket-datacenter/policy-pull.test.ts index 78a887b48..7f05bdf7b 100644 --- a/apps/hub/src/bitbucket-datacenter/policy-pull.test.ts +++ b/apps/hub/src/bitbucket-datacenter/policy-pull.test.ts @@ -1,5 +1,6 @@ import { expect, test } from "bun:test"; -import { bitbucketFetch } from "./client.js"; +import { BitbucketFailed, bitbucketFetch } from "./client.js"; +import { bodyWith, DescriptionNotUpdated, PolicyFileMoved } from "../github/policy-pull.js"; import { openPolicyPull, proposePolicy } from "./policy-pull.js"; const REPO = "ACME/widgets"; @@ -12,12 +13,21 @@ const pulls: Array<{ id: number; version: number; title: string; description: st const edits: Array> = []; /** Set, the next commit is followed by another caller opening the pull request first. */ let raced: (typeof pulls)[number] | undefined; +/** Each commit made through the browse API, newest last. */ +const log: Array<{ branch: string; id: string; message: string }> = []; +/** How many description edits fail next. */ +let pullPutFailures = 0; +/** Set, the description is edited on Bitbucket just before the next edit, so that edit's version is stale. */ +let editedBeforePut = false; +/** Set, the policy file appears on the branch just before the next commit. */ +let appearsBeforePut: string | undefined; async function bitbucket(req: Request): Promise { const url = new URL(req.url); if (req.method === "DELETE" && url.pathname === "/rest/branch-utils/1.0/projects/ACME/repos/widgets/branches") { const { name } = await req.json() as { name: string }; branches.delete(name.slice("refs/heads/".length)); + log.splice(0, log.length, ...log.filter((row) => row.branch !== name.slice("refs/heads/".length))); return new Response(null, { status: 204 }); } const path = url.pathname.slice(BASE.length); @@ -34,9 +44,12 @@ async function bitbucket(req: Request): Promise { expect(req.headers.get("content-type")).toStartWith("multipart/form-data"); const form = Object.fromEntries(await Promise.all([...(await req.formData()).entries()].map(async ([key, value]) => [key, typeof value === "string" ? value : await value.text()]))); edits.push(form); + if (appearsBeforePut !== undefined) branches.set(form.branch!, { text: appearsBeforePut, commit: "c-appeared" }); + appearsBeforePut = undefined; const from = branches.get(form.sourceBranch ?? form.branch!)!; if (form.sourceCommitId !== (from.text === undefined ? undefined : from.commit)) return new Response(null, { status: 409 }); branches.set(form.branch!, { text: form.content, commit: `c${edits.length}` }); + log.push({ branch: form.branch!, id: `c${edits.length}`, message: form.message! }); if (raced) pulls.push(raced); raced = undefined; return Response.json({ id: `c${edits.length}` }); @@ -49,8 +62,23 @@ async function bitbucket(req: Request): Promise { pulls.push({ id: pulls.length + 1, version: 0, open: true, ...(await req.json() as Omit<(typeof pulls)[number], "id" | "version" | "open">) }); return Response.json(pullJson(pulls.at(-1)!), { status: 201 }); } + if (req.method === "GET" && /^\/pull-requests\/\d+\/commits$/.test(path)) { + const pull = pulls.find((row) => row.id === Number(path.split("/")[2]))!; + const all = log.filter((row) => `refs/heads/${row.branch}` === pull.fromRef.id).toReversed(); + const start = Number(url.searchParams.get("start") ?? "0"); + const end = start + Number(url.searchParams.get("limit") ?? "25"); + const values = all.slice(start, end).map((row) => ({ id: row.id, message: row.message, author: { name: "triage" }, committerTimestamp: 0 })); + return Response.json({ values, isLastPage: end >= all.length, ...(end < all.length && { nextPageStart: end }) }); + } + if (req.method === "GET" && path.startsWith("/pull-requests/")) return Response.json(pullJson(pulls.find((row) => row.id === Number(path.slice("/pull-requests/".length)))!)); if (req.method === "PUT" && path.startsWith("/pull-requests/")) { + if (pullPutFailures > 0) { + pullPutFailures -= 1; + return new Response(null, { status: 502 }); + } const pull = pulls.find((row) => row.id === Number(path.slice("/pull-requests/".length)))!; + if (editedBeforePut) Object.assign(pull, { description: `${pull.description}\n\nedited on Bitbucket`, version: pull.version + 1 }); + editedBeforePut = false; const body = await req.json() as { version: number; title: string; description: string }; if (body.version !== pull.version) return new Response(null, { status: 409 }); Object.assign(pull, { title: body.title, description: body.description, version: pull.version + 1 }); @@ -60,7 +88,7 @@ async function bitbucket(req: Request): Promise { } function pullJson(pull: (typeof pulls)[number]) { - return { id: pull.id, version: pull.version, links: { self: [{ href: `https://bitbucket.test/projects/ACME/repos/widgets/pull-requests/${pull.id}` }] } }; + return { id: pull.id, version: pull.version, title: pull.title, description: pull.description, links: { self: [{ href: `https://bitbucket.test/projects/ACME/repos/widgets/pull-requests/${pull.id}` }] } }; } test("the Bitbucket pull request is made once from a new branch, then brought up to date in place", async () => { @@ -80,11 +108,63 @@ test("the Bitbucket pull request is made once from a new branch, then brought up expect(pulls[0]!.description).toBe("second"); expect(await openPolicyPull(bb, REPO)).toEqual(opened); + expect(await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Go }\n", title: "chore: another title", body: "appended", append: true })).toEqual(opened); + expect(pulls[0]!.title).toBe(change.title); + expect(pulls[0]!.description).toStartWith("second\n\n"); + expect(pulls[0]!.description).toEndWith("\nappended"); + const appended = pulls[0]!.description; + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Go }\n", body: "unchanged", append: true }); + expect(pulls[0]!.description).toBe(appended); + await expect(proposePolicy(bb, REPO, "main", { ...change, text: "rules: [] # stale\n", body: "stale", append: true, readSha: "c1" })).rejects.toBeInstanceOf(PolicyFileMoved); + expect(pulls[0]!.description).toBe(appended); + + // A description edit that keeps failing names the commit that landed, and the next batch adds its section. + pullPutFailures = 4; + const lost = proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Lost }\n", body: "lost", append: true }); + await expect(lost).rejects.toBeInstanceOf(DescriptionNotUpdated); + await expect(lost).rejects.toMatchObject({ commit: log.at(-1)!.id }); + expect(pulls[0]!.description).not.toContain("lost"); + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Found }\n", body: "found", append: true }); + expect(pulls[0]!.description).toStartWith(appended); + expect(pulls[0]!.description).toMatch(/\nlost\n[\s\S]*\nfound$/); + + // A description edited on Bitbucket between the read and the edit is read again and keeps both. + editedBeforePut = true; + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Raced }\n", body: "raced", append: true }); + expect(pulls[0]!.description).toContain("\nedited on Bitbucket\n"); + expect(pulls[0]!.description).toEndWith("\nraced"); + + // An export's refused edit is Bitbucket's refusal, sent once and not retried. + pullPutFailures = 1; + await expect(proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Export }\n", body: "export" })).rejects.toBeInstanceOf(BitbucketFailed); + expect(pullPutFailures).toBe(0); + + // A file that appears on a branch read without one is a moved file. + branches.set("corbits-triage/policy", { commit: branches.get("corbits-triage/policy")!.commit }); + appearsBeforePut = "rules: []\n"; + await expect(proposePolicy(bb, REPO, "main", { ...change, text: "rules: [] # new\n", body: "new", append: true, readSha: null })).rejects.toBeInstanceOf(PolicyFileMoved); + + // An export over a description holding batch sections keeps them and adds its own, so the next batch adds none back. + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Kept }\n", body: "kept", append: true }); + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Exported }\n", body: "exported" }); + expect(pulls[0]!.description).toContain("\nkept\n"); + expect(pulls[0]!.description).toEndWith("\nexported"); + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: After }\n", body: "after", append: true }); + expect(pulls[0]!.description.split("\nkept\n")).toHaveLength(2); + expect(pulls[0]!.description.split("\nlost\n")).toHaveLength(2); + + // A lost section older than the newest hundred commits is found on a later page. + const older = Array.from({ length: 101 }, (_, at) => ({ branch: "corbits-triage/policy", id: `x${at + 1}`, message: `${change.title}\n\nolder ${at + 1}` })); + log.unshift(...older); + pulls[0]!.description = bodyWith(pulls[0]!.description, older.slice(1).map((row) => ({ commit: row.id, text: row.message.slice(change.title.length + 2) }))); + await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\nlabels: { pass: Paged }\n", body: "paged", append: true }); + expect(pulls[0]!.description).toContain("(corbits-triage added x1)\n\nolder 1\n"); + // Merged and left behind, the branch starts over from main for the next pull request. pulls[0]!.open = false; branches.set("main", { text: "rules: []\nlabels: { pass: Ready }\n", commit: "c9" }); expect(await proposePolicy(bb, REPO, "main", { ...change, text: "rules: []\n", body: "third" })).toMatchObject({ number: 2 }); - expect(edits[2]).toEqual({ content: "rules: []\n", message: change.title, branch: "corbits-triage/policy", sourceBranch: "main", sourceCommitId: "c9" }); + expect(edits.at(-1)).toEqual({ content: "rules: []\n", message: change.title, branch: "corbits-triage/policy", sourceBranch: "main", sourceCommitId: "c9" }); // Another caller opening it between the check and the create is answered with theirs. pulls[1]!.open = false; diff --git a/apps/hub/src/bitbucket-datacenter/policy-pull.ts b/apps/hub/src/bitbucket-datacenter/policy-pull.ts index 9221eb993..76891f014 100644 --- a/apps/hub/src/bitbucket-datacenter/policy-pull.ts +++ b/apps/hub/src/bitbucket-datacenter/policy-pull.ts @@ -1,13 +1,14 @@ // The Bitbucket Data Center side of policy-pull.ts: one pull request from `corbits-triage/policy`, opened once and // brought up to date when asked again. import { type } from "arktype"; -import { filePath, readRepoFile, repoParts, repoPath, type BitbucketFetch } from "@corbits/bitbucket-datacenter-reads"; -import { POLICY_BRANCH, type PolicyChange, type PolicyPull } from "../github/policy-pull.js"; +import { filePath, listPrCommits, readRepoFile, repoParts, repoPath, type BitbucketFetch } from "@corbits/bitbucket-datacenter-reads"; +import { bodyWith, commitMessage, missingSections, openingBody, POLICY_BRANCH, PolicyFileMoved, replacedBody, retriedEdit, withCommitted, type PolicyChange, type PolicyPull } from "../github/policy-pull.js"; import { BitbucketFailed } from "./client.js"; -const Pull = type({ id: "number", version: "number", links: { self: type({ href: "string" }).array().atLeastLength(1) } }); +const Pull = type({ id: "number", version: "number", title: "string", "description?": "string", links: { self: type({ href: "string" }).array().atLeastLength(1) } }); const Pulls = type({ values: Pull.array() }); const Branches = type({ values: type({ displayId: "string" }).array() }); +const Written = type({ id: "string" }); const BRANCH_REF = `refs/heads/${POLICY_BRANCH}`; @@ -42,37 +43,63 @@ async function deleteBranch(bb: BitbucketFetch, repo: string): Promise { await send(bb, `/rest/branch-utils/1.0/projects/${project}/repos/${slug}/branches`, { method: "DELETE", body: JSON.stringify({ name: BRANCH_REF, dryRun: false }) }); } -/** Commits the file to the branch, making the branch from `base` when it is not there; the commit that last changed the file is how Data Center refuses a stale edit. */ -async function commitFile(bb: BitbucketFetch, repo: string, base: string, onBranch: boolean, { path, text, title }: PolicyChange): Promise { +/** + * Commits the file to the branch, making the branch from `base` when it is not there; the commit that last changed the + * file is how Data Center refuses a stale edit. The commit made, undefined when the branch already held the text. + */ +async function commitFile(bb: BitbucketFetch, repo: string, base: string, onBranch: boolean, change: PolicyChange): Promise { + const { path, text, readSha } = change; const current = await readRepoFile(bb, repo, path, onBranch ? POLICY_BRANCH : base); - if (onBranch && current?.kind === "file" && current.text === text) return; + if (onBranch && current?.kind === "file" && current.text === text) return undefined; + if (readSha !== undefined && (current?.kind === "file" ? current.sha : null) !== readSha) throw new PolicyFileMoved(path); const form = new FormData(); // A file part keeps the text's line endings; a plain field would send them as CRLF. form.set("content", new Blob([text], { type: "text/plain" }), path); - form.set("message", title); + form.set("message", commitMessage(change)); form.set("branch", POLICY_BRANCH); if (!onBranch) form.set("sourceBranch", base); if (current?.kind === "file") form.set("sourceCommitId", current.sha); - await send(bb, `${repoPath(repo)}/browse/${filePath(path)}`, { method: "PUT", body: form }); + const written = `${repoPath(repo)}/browse/${filePath(path)}`; + // Data Center refuses the edit with 409 when a commit has changed the file since `sourceCommitId`, or when a file + // appeared where none was read. + const res = await bb(written, { method: "PUT", body: form }); + if (res.status === 409 && readSha !== undefined) throw new PolicyFileMoved(path); + if (!res.ok) throw new BitbucketFailed(res.status, written); + return Written.assert(await res.json()).id; +} + +/** Adds a section for each batch commit the description lacks, the new one included; with none lacking, no edit is made. */ +async function appendSections(bb: BitbucketFetch, repo: string, id: number, change: PolicyChange, committed: string | undefined): Promise { + const path = `${repoPath(repo)}/pull-requests/${id}`; + await retriedEdit(committed, async function appendMissing() { + const latest = Pull.assert(await send(bb, path)); + // Data Center lists the newest commit first. + const listed = (await listPrCommits(bb, repo, id)).toReversed(); + const commits = withCommitted(listed.map((row) => ({ commit: row.sha, message: row.message })), committed, change); + const added = missingSections(latest.description, commits, change.title); + if (added.length === 0) return; + await send(bb, path, { method: "PUT", body: JSON.stringify({ version: latest.version, title: latest.title, description: bodyWith(latest.description, added) }) }); + }); } export async function proposePolicy(bb: BitbucketFetch, repo: string, base: string, change: PolicyChange): Promise { const open = await openPull(bb, repo); const left = !open && (await branchExists(bb, repo)); if (left) await deleteBranch(bb, repo); - await commitFile(bb, repo, base, Boolean(open), change); - const pulls = `${repoPath(repo)}/pull-requests`; - if (open) { - await send(bb, `${pulls}/${open.id}`, { method: "PUT", body: JSON.stringify({ version: open.version, title: change.title, description: change.body }) }); + const committed = await commitFile(bb, repo, base, Boolean(open), change); + if (!open) return createPull(bb, repo, base, change, committed); + if (change.append) { + await appendSections(bb, repo, open.id, change, committed); return pullOf(open); } - return createPull(bb, repo, base, change); + await send(bb, `${repoPath(repo)}/pull-requests/${open.id}`, { method: "PUT", body: JSON.stringify({ version: open.version, title: change.title, description: replacedBody(open.description, change, committed) }) }); + return pullOf(open); } /** A pull request opened by a concurrent call in between is answered instead of refused. */ -async function createPull(bb: BitbucketFetch, repo: string, base: string, { title, body }: PolicyChange): Promise { +async function createPull(bb: BitbucketFetch, repo: string, base: string, change: PolicyChange, committed: string | undefined): Promise { const path = `${repoPath(repo)}/pull-requests`; - const res = await bb(path, { method: "POST", body: JSON.stringify({ title, description: body, fromRef: { id: BRANCH_REF }, toRef: { id: `refs/heads/${base}` } }) }); + const res = await bb(path, { method: "POST", body: JSON.stringify({ title: change.title, description: openingBody(change, committed), fromRef: { id: BRANCH_REF }, toRef: { id: `refs/heads/${base}` } }) }); if (res.ok) return pullOf(Pull.assert(await res.json())); const raced = res.status === 409 ? await openPull(bb, repo) : undefined; if (raced) return pullOf(raced); diff --git a/apps/hub/src/github/coalescer.ts b/apps/hub/src/github/coalescer.ts index 9c746c96e..d54adf370 100644 --- a/apps/hub/src/github/coalescer.ts +++ b/apps/hub/src/github/coalescer.ts @@ -31,6 +31,7 @@ import type { HeadChecksReader, OpenHeadReader } from "./tenant-open-heads.js"; import { inFlight, waitedOut, waitsForCi, type ReconcilePolicy } from "./reconcile-plan.js"; import { deliveryFailure, failHead, queueHead, restoreHead, waitHead, type QueuedHead, type Remembered } from "./triage-queue.js"; import { TriageStateConflictError, type TriageStateStore } from "./triage-state-store.js"; +import { createKeyedSerial } from "../keyed-serial.js"; export type CoalesceWindow = { quietMs: number; maxWaitMs: number; burstMs: number }; @@ -130,18 +131,10 @@ function eventsOf(input: CoalescedEvent, rows: readonly PrTriageRow[]): TriageEv return untriaged ? ["ready", "opened"] : [input.event]; } -async function settled(work: Promise): Promise { - try { - await work; - } catch { - // The caller of `work` handles its failure; the chain only waits for it. - } -} - export function createCoalescer(deps: CoalescerDeps): Coalescer { const windows = new Map(); - /** The last accept or flush of each pull request, so they never interleave. */ - const tails = new Map>(); + /** Each pull request's accepts and flushes, so they never interleave. */ + const serialized = createKeyedSerial(); /** The head each pull request's latest new commit or opening named, kept past its window so a late event cannot move it back. */ const newest = new Map(); @@ -153,23 +146,6 @@ export function createCoalescer(deps: CoalescerDeps): Coalescer { return { tenantId, repo: triageStateKey(record), number, headSha }; } - async function serialized(key: string, work: () => Promise): Promise { - const previous = tails.get(key); - const current = runAfter(previous, work); - const tail = settled(current); - tails.set(key, tail); - try { - return await current; - } finally { - if (tails.get(key) === tail) tails.delete(key); - } - } - - async function runAfter(previous: Promise | undefined, work: () => Promise): Promise { - await previous; - return work(); - } - function timer(key: string, ms: number): () => void { return deps.setTimer(function onWindowEnd() { return flushLogged(key); diff --git a/apps/hub/src/github/policy-file.postgres.test.ts b/apps/hub/src/github/policy-file.postgres.test.ts index 470c8bf10..e021fa60c 100644 --- a/apps/hub/src/github/policy-file.postgres.test.ts +++ b/apps/hub/src/github/policy-file.postgres.test.ts @@ -6,10 +6,10 @@ import { createEnvKeyCredentialCipher } from "@intx/crypto"; import { createDB, runMigrations, schema } from "@intx/db"; import { credentialAad } from "@intx/types"; import { createArtifact, findArtifactByTitle, runArtifactMigrations } from "@corbits/artifacts"; -import { checkPackName, emptyPack, exportPolicyFile, policyDifferences, readPolicyFile, starterPack, type CheckPack, type RepoSource } from "@corbits/triage-contracts"; +import { checkPackName, checkPolicyFile, emptyPack, exportPolicyFile, policyDifferences, readPolicyFile, starterPack, type CheckPack, type RepoSource } from "@corbits/triage-contracts"; import { PROVIDER, TOKEN_CREDENTIAL } from "../bitbucket-datacenter/names.js"; import { fileRefusal, settlePolicy } from "./check-pack-store.js"; -import { createPolicyFile, listedPolicyPull } from "./policy-file.js"; +import { createPolicyFile, listedPolicyPull, PolicyPatchBody } from "./policy-file.js"; import { POLICY_BRANCH } from "./policy-pull.js"; import { createMemberOpenPulls, createOpenPullsCache } from "./open-pulls.js"; import { createRepoActivity } from "./repo-activity.js"; @@ -30,6 +30,7 @@ const EXTENDS = "acme/extends"; const CHOSEN = "acme/chosen"; const CHOSEN_SAVED = "acme/chosen-saved"; const CHOSEN_RACED = "acme/chosen-raced"; +const PATCHED = "acme/patched"; const BOTH = "acme/both"; const UNHEARD = "acme/unheard"; const BITBUCKET_UNHEARD = "PROJ/unheard"; @@ -55,6 +56,12 @@ let commits = 0; const requests: string[] = []; /** Set, the next commit is followed by another caller opening this pull request first. */ let raced: (typeof pulls)[number] | undefined; +/** Each commit written through the contents API, with its message, as `repo:branch`. */ +const branchCommits: Array<{ at: string; sha: string; message: string }> = []; +/** How many pull request edits fail next. */ +let pullEditFailures = 0; +/** Set to a `repo@branch/path` key, the next read of it is followed by another commit to that file. */ +let movedAfterRead: string | undefined; function onMain(repo: string, path: string, text: string | null, sha = `blob${++commits}`): void { files.set(`${repo}@main/${path}`, { sha, text }); @@ -71,18 +78,22 @@ async function github(req: Request): Promise { if (req.method === "GET" && path.startsWith("contents/")) { if (readFails) return new Response(null, { status: 502 }); const at = path.slice("contents/".length); - const file = files.get(`${repo}@${url.searchParams.get("ref") ?? "main"}/${at}`); + const key = `${repo}@${url.searchParams.get("ref") ?? "main"}/${at}`; + const file = files.get(key); if (!file) return new Response(null, { status: 404 }); + if (movedAfterRead === key) files.set(key, { sha: `blob${++commits}`, text: file.text }); + if (movedAfterRead === key) movedAfterRead = undefined; if (file.sha === DIRECTORY) return Response.json([{ type: "file", name: "inner.yaml" }]); const content = file.text === null ? { content: "", encoding: "none" } : { content: Buffer.from(file.text).toString("base64"), encoding: "base64" }; return Response.json({ type: "file", sha: file.sha, ...content, html_url: `https://github.com/${repo}/blob/main/${at}` }); } if (req.method === "PUT" && path.startsWith("contents/")) { const at = path.slice("contents/".length); - const { branch, content, sha } = await req.json() as { branch: string; content: string; sha?: string }; + const { branch, content, sha, message } = await req.json() as { branch: string; content: string; sha?: string; message: string }; if (!refs.has(`${repo}:${branch}`)) return Response.json({ message: "Branch not found" }, { status: 404 }); if (files.get(`${repo}@${branch}/${at}`)?.sha !== sha) return Response.json({ message: "sha does not match" }, { status: 409 }); files.set(`${repo}@${branch}/${at}`, { sha: `blob${++commits}`, text: Buffer.from(content, "base64").toString("utf8") }); + branchCommits.push({ at: `${repo}:${branch}`, sha: `commit${commits}`, message }); if (raced) pulls.push(raced); raced = undefined; return Response.json({ content: { sha: `blob${commits}` }, commit: { sha: `commit${commits}` } }, { status: 201 }); @@ -100,6 +111,7 @@ async function github(req: Request): Promise { if (req.method === "PATCH" && path.startsWith("git/refs/heads/")) { const branch = path.slice("git/refs/heads/".length); for (const key of [...files.keys()]) if (key.startsWith(`${repo}@${branch}/`)) files.delete(key); + branchCommits.splice(0, branchCommits.length, ...branchCommits.filter((row) => row.at !== `${repo}:${branch}`)); for (const [key, file] of files) if (key.startsWith(`${repo}@main/`)) files.set(key.replace("@main/", `@${branch}/`), file); return Response.json({ ref: `refs/heads/${branch}` }); } @@ -109,7 +121,7 @@ async function github(req: Request): Promise { } if (req.method === "GET" && path === "pulls") { const head = url.searchParams.get("head"); - return Response.json(pulls.filter((pull) => pull.repo === repo && pull.open && `${owner}:${pull.head}` === head).map((pull) => ({ number: pull.number, html_url: `https://github.com/${repo}/pull/${pull.number}`, head: { sha: "head-sha" } }))); + return Response.json(pulls.filter((pull) => pull.repo === repo && pull.open && `${owner}:${pull.head}` === head).map((pull) => ({ number: pull.number, title: pull.title, body: pull.body, html_url: `https://github.com/${repo}/pull/${pull.number}`, head: { sha: "head-sha" } }))); } if (req.method === "POST" && path === "pulls") { const body = await req.json() as { head: string; base: string; title: string; body: string }; @@ -117,7 +129,19 @@ async function github(req: Request): Promise { pulls.push({ repo, number: pulls.length + 1, open: true, ...body }); return Response.json({ number: pulls.length, html_url: `https://github.com/${repo}/pull/${pulls.length}`, head: { sha: "head-sha" } }, { status: 201 }); } + if (req.method === "GET" && /^pulls\/\d+\/commits$/.test(path)) { + const pull = pulls.find((row) => row.repo === repo && row.number === Number(path.split("/")[1]))!; + return Response.json(branchCommits.filter((row) => row.at === `${repo}:${pull.head}`).map((row) => ({ sha: row.sha, commit: { message: row.message } }))); + } + if (req.method === "GET" && path.startsWith("pulls/")) { + const pull = pulls.find((row) => row.repo === repo && row.number === Number(path.slice("pulls/".length)))!; + return Response.json({ number: pull.number, title: pull.title, body: pull.body, html_url: `https://github.com/${repo}/pull/${pull.number}`, head: { sha: "head-sha" } }); + } if (req.method === "PATCH" && path.startsWith("pulls/")) { + if (pullEditFailures > 0) { + pullEditFailures -= 1; + return new Response(null, { status: 502 }); + } const pull = pulls.find((row) => row.repo === repo && row.number === Number(path.slice("pulls/".length)))!; Object.assign(pull, await req.json()); return Response.json({ number: pull.number, html_url: `https://github.com/${repo}/pull/${pull.number}`, head: { sha: "patched-head" } }); @@ -155,7 +179,7 @@ beforeAll(async () => { handle = createDB(config); const db = handle.db; const cipher = createEnvKeyCredentialCipher(new Uint8Array(32).fill(7)); - const repos = [...[FROM_FILE, SAVED, EXPORTED, STARTER].map((name) => ({ name, connected: true, enabled: true, installationId: 1 })), { name: OFF, connected: true, enabled: false, installationId: 1 }, { name: SHARED, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN_SAVED, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN_RACED, connected: true, enabled: false, installationId: 1 }, { name: EXTENDS, connected: true, enabled: true, installationId: 1 }, { name: BOTH, connected: true, enabled: true, installationId: 1 }, { name: BOTH, source: "bitbucket-datacenter", connected: true, enabled: true }, { name: UNHEARD, connected: true, enabled: true, installationId: 1 }, { name: BITBUCKET_UNHEARD, source: "bitbucket-datacenter", connected: true, enabled: true }]; + const repos = [...[FROM_FILE, SAVED, EXPORTED, STARTER, PATCHED].map((name) => ({ name, connected: true, enabled: true, installationId: 1 })), { name: OFF, connected: true, enabled: false, installationId: 1 }, { name: SHARED, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN_SAVED, connected: true, enabled: false, installationId: 1 }, { name: CHOSEN_RACED, connected: true, enabled: false, installationId: 1 }, { name: EXTENDS, connected: true, enabled: true, installationId: 1 }, { name: BOTH, connected: true, enabled: true, installationId: 1 }, { name: BOTH, source: "bitbucket-datacenter", connected: true, enabled: true }, { name: UNHEARD, connected: true, enabled: true, installationId: 1 }, { name: BITBUCKET_UNHEARD, source: "bitbucket-datacenter", connected: true, enabled: true }]; await db.insert(schema.tenant).values({ id: TENANT, name: "Policy file", slug: "policy-file", domain: "policy-file.test", config: { corbitsTriage: { repos } } }); await db.insert(schema.principal).values([ { id: "prn_user", tenantId: TENANT, kind: "user", refId: "usr_user", status: "active" }, @@ -223,6 +247,11 @@ async function exportOf(repo: string, text?: string): Promise) }; } +async function patchOf(body: typeof PolicyPatchBody.infer): Promise> { + const res = await policyFile.patchRoute(body, MEMBER); + return { http: res.status, ...(await res.json() as Record) }; +} + async function fileOf(repo: string): Promise { const res = await policyFile.fileRoute({ repo }, MEMBER); return ((await res.json()) as { file: unknown }).file; @@ -322,7 +351,9 @@ test("PostgreSQL: writing the saved rules to the file opens one pull request, up const number = pulls.length + 1; expect(await exportOf(EXPORTED)).toEqual({ http: 200, status: "opened", url: `https://github.com/${EXPORTED}/pull/${number}`, number, headSha: "head-sha" }); const [pull] = pulls.filter((row) => row.repo === EXPORTED); - expect(pull).toMatchObject({ head: POLICY_BRANCH, base: "main", body: expect.stringContaining("The saved rules keep applying until this pull request is merged.") }); + // Bun 1.4.2's toMatchObject writes an asymmetric matcher's result into the object it checks, so the body is checked apart. + expect(pull).toMatchObject({ head: POLICY_BRANCH, base: "main" }); + expect(pull!.body).toContain("The saved rules keep applying until this pull request is merged."); const written = files.get(`${EXPORTED}@${POLICY_BRANCH}/${YML}`)!.text!; expect(written).toContain("author:\n - maintainers"); expect(written).not.toMatch(/https?:\/\//); @@ -336,6 +367,10 @@ test("PostgreSQL: writing the saved rules to the file opens one pull request, up expect(files.get(`${EXPORTED}@${POLICY_BRANCH}/${YML}`)!.text).toContain("check: focused"); // Nothing new to commit: the head is GitHub's answer to the update. expect(await exportOf(EXPORTED)).toMatchObject({ http: 200, number: pull!.number, headSha: "patched-head" }); + // An export's refused edit is GitHub's refusal, sent once and not retried. + pullEditFailures = 1; + expect(await exportOf(EXPORTED)).toMatchObject({ http: 502, error: { code: "github_failed" } }); + expect(pullEditFailures).toBe(0); // Merged: the file lands on the default branch and runs the same, so it is the policy and the saved rules retire. onMain(EXPORTED, YML, files.get(`${EXPORTED}@${POLICY_BRANCH}/${YML}`)!.text); @@ -488,3 +523,132 @@ test("a Bitbucket policy pull request carries no head, so it is merged on Bitbuc expect(listedPolicyPull(listing("github"), "PROJ/widgets", "github")).toEqual({ url: pr.url, number: 4, headSha: "bb-head" }); expect(listedPolicyPull(listing("github"), "PROJ/widgets", "bitbucket-datacenter")).toBeUndefined(); }); + +const PATCH_FILE = `# Corbits Triage policy. +rules: + # leave ci-green alone + - check: ci-green + text: CI passes. + reason: Required by branch protection. + evidence: [maintainer-stated] + - check: focused + text: One focused change. + reason: Mixed changes were split. + evidence: [maintainer-stated] + - check: max-lines-changed + text: Flag pull requests over 400 lines changed. + reason: Review load. + evidence: [maintainer-stated] + max: 400 +pauseLabel: hold +code-review: + on: false + provider: anthropic +`; + +const PATCH_CHANGES = [ + { kind: "rule" as const, id: "focused", enabled: false }, + { kind: "rule-options" as const, id: "max-lines-changed", options: { max: 200 } }, + { kind: "rule" as const, id: "docs", enabled: true }, + { kind: "rule-options" as const, id: "docs", options: { reason: "Docs drift from the code." } }, + { kind: "code-review" as const, on: true }, +]; + +test("PostgreSQL: a batch of policy edits opens one pull request, a preview writes nothing, and the same batch again makes no new commit", async () => { + const absent = await patchOf({ repo: PATCHED, changes: PATCH_CHANGES }); + expect(absent).toMatchObject({ http: 409, error: { code: "policy_file_absent" } }); + expect(pulls.filter((row) => row.repo === PATCHED)).toEqual([]); + + onMain(PATCHED, YML, PATCH_FILE, "patch-main"); + const refused = await patchOf({ repo: PATCHED, changes: [{ kind: "rule", id: "made-up", enabled: true }] }); + expect(refused).toMatchObject({ http: 422, error: { code: "policy_file_invalid", message: "Unknown rule made-up." } }); + expect(files.has(`${PATCHED}@${POLICY_BRANCH}/${YML}`)).toBe(false); + expect(pulls.filter((row) => row.repo === PATCHED)).toEqual([]); + + const preview = await patchOf({ repo: PATCHED, changes: PATCH_CHANGES, preview: true }); + expect(preview.http).toBe(200); + expect(preview.before).toBe(PATCH_FILE); + expect(preview.after).toEqual(expect.any(String)); + expect(String(preview.after)).toContain("# leave ci-green alone"); + expect(String(preview.after)).toContain("text: CI passes."); + expect(String(preview.after)).not.toBe(PATCH_FILE); + expect(files.get(`${PATCHED}@main/${YML}`)?.text).toBe(PATCH_FILE); + expect(files.has(`${PATCHED}@${POLICY_BRANCH}/${YML}`)).toBe(false); + expect(pulls.filter((row) => row.repo === PATCHED)).toEqual([]); + + const noop = await patchOf({ repo: PATCHED, changes: [{ kind: "rule-options", id: "max-lines-changed", options: { max: 400 } }] }); + expect(noop).toEqual({ http: 200, before: PATCH_FILE, after: PATCH_FILE }); + expect(files.has(`${PATCHED}@${POLICY_BRANCH}/${YML}`)).toBe(false); + expect(pulls.filter((row) => row.repo === PATCHED)).toEqual([]); + + const number = pulls.length + 1; + const opened = await patchOf({ repo: PATCHED, changes: PATCH_CHANGES }); + expect(opened).toEqual({ http: 200, status: "opened", url: `https://github.com/${PATCHED}/pull/${number}`, number, headSha: "head-sha" }); + const title = `chore: update the Corbits Triage rules in ${YML}`; + expect(pulls.filter((row) => row.repo === PATCHED)).toMatchObject([{ number, head: POLICY_BRANCH, base: "main", title }]); + const written = files.get(`${PATCHED}@${POLICY_BRANCH}/${YML}`)!; + expect(written.text).toContain("# leave ci-green alone"); + expect(written.text).toContain("text: CI passes."); + expect(written.text).toContain("pauseLabel: hold"); + expect(written.text).toContain("provider: anthropic"); + expect(written.text).toContain("max: 200"); + expect(written.text).toContain("Check docs are updated when behavior changes."); + expect(checkPolicyFile(written.text!, PATCHED)).toMatchObject({ ok: true }); + + const sha = written.sha; + const again = await patchOf({ repo: PATCHED, changes: PATCH_CHANGES }); + expect(again).toEqual({ http: 200, before: written.text, after: written.text }); + expect(pulls.filter((row) => row.repo === PATCHED)).toHaveLength(1); + expect(files.get(`${PATCHED}@${POLICY_BRANCH}/${YML}`)?.sha).toBe(sha); + + // An edit made on the open pull request's branch is what the next patch reads, so it is not overwritten from the default branch. + const branchKey = `${PATCHED}@${POLICY_BRANCH}/${YML}`; + const edited = files.get(branchKey)!.text!.replace("# leave ci-green alone", "# leave ci-green alone\n # edited on the pull request"); + files.set(branchKey, { sha, text: edited }); + const follow = await patchOf({ repo: PATCHED, changes: [{ kind: "rule", id: "ci-green", enabled: false }] }); + expect(follow).toMatchObject({ http: 200, number }); + expect(files.get(branchKey)?.text).toContain("# edited on the pull request"); + expect(files.get(branchKey)?.text).toContain("# leave ci-green alone"); + const [pull] = pulls.filter((row) => row.repo === PATCHED); + expect(pull).toMatchObject({ title }); + expect(pull!.body.split("Turns off `focused`.")).toHaveLength(2); + expect(pull!.body.endsWith("- Turns off `ci-green`.")).toBe(true); + + // A commit to the branch between the route's read and its write is not overwritten. + const before = files.get(branchKey)!; + movedAfterRead = branchKey; + const stale = await patchOf({ repo: PATCHED, changes: [{ kind: "code-review", on: false }] }); + expect(stale).toMatchObject({ http: 409, error: { code: "policy_file_changed" } }); + expect(files.get(branchKey)?.text).toBe(before.text); + expect(files.get(branchKey)?.sha).not.toBe(before.sha); + + // An open pull request whose branch has no policy file gets one, built from the default branch's. + files.delete(branchKey); + const created = await patchOf({ repo: PATCHED, changes: [{ kind: "rule", id: "focused", enabled: false }] }); + expect(created).toMatchObject({ http: 200, number }); + expect(files.get(branchKey)?.text).toContain("pauseLabel: hold"); + expect(files.get(branchKey)?.text).toContain("enabled: false"); + expect(files.get(`${PATCHED}@main/${YML}`)?.text).toBe(PATCH_FILE); + + // Two batches sent together both commit and both reach the body. + const together = await Promise.all([ + patchOf({ repo: PATCHED, changes: [{ kind: "rule", id: "focused", enabled: true }] }), + patchOf({ repo: PATCHED, changes: [{ kind: "code-review", on: true }] }), + ]); + expect(together).toMatchObject([{ http: 200, number }, { http: 200, number }]); + const both = pulls.find((row) => row.repo === PATCHED)!.body; + expect(both).toContain("Turns on `focused`."); + expect(both).toContain("Turns on code review."); + + // A commit whose description edit kept failing is named in the refusal, and the next batch adds its section. + pullEditFailures = 4; + const lost = await patchOf({ repo: PATCHED, changes: [{ kind: "code-review", on: false }] }); + const lostCommit = branchCommits.at(-1)!.sha; + expect(lost).toMatchObject({ http: 502, error: { code: "description_not_updated", message: expect.stringContaining(lostCommit) } }); + expect(pulls.find((row) => row.repo === PATCHED)!.body).not.toContain("Turns off code review."); + const repaired = await patchOf({ repo: PATCHED, changes: [{ kind: "rule", id: "focused", enabled: false }] }); + expect(repaired).toMatchObject({ http: 200, number }); + const body = pulls.find((row) => row.repo === PATCHED)!.body; + expect(body).toContain("Turns off code review."); + expect(body.indexOf("Turns off code review.")).toBeLessThan(body.lastIndexOf("Turns off `focused`.")); +}); diff --git a/apps/hub/src/github/policy-file.ts b/apps/hub/src/github/policy-file.ts index 4f19d895a..b70697a6d 100644 --- a/apps/hub/src/github/policy-file.ts +++ b/apps/hub/src/github/policy-file.ts @@ -14,7 +14,7 @@ import { defaultBranch, readRepo } from "../bitbucket-datacenter/client.js"; import * as bitbucketPull from "../bitbucket-datacenter/policy-pull.js"; import { splitName } from "../bitbucket-datacenter/records.js"; import type { TenantBitbucket } from "../bitbucket-datacenter/tenant-bitbucket.js"; -import { checkPolicyFile, exportPolicyFile, POLICY_FILE_PATH, notAFileReason, POLICY_FILE_PATHS, policyFileReason, REPO_SOURCES, starterPack, type PolicyFileStatus, type PolicyUpgrades, type PolicyWorkflow, type RepoSource } from "@corbits/triage-contracts"; +import { checkPolicyFile, exportPolicyFile, fileRuleId, patchPolicyFile, POLICY_FILE_PATH, PolicyPatchChanges, notAFileReason, POLICY_FILE_PATHS, policyFileReason, REPO_SOURCES, starterPack, type PolicyFileStatus, type PolicyPatchChange, type PolicyUpgrades, type PolicyWorkflow, type RepoSource } from "@corbits/triage-contracts"; import type { PolicySettled } from "./check-pack-store.js"; import { forInstallation, type MemberOpenPulls, type OpenPullsRead } from "./open-pulls.js"; import * as github from "./policy-pull.js"; @@ -23,10 +23,12 @@ import type { RepoActivity } from "./repo-activity.js"; import type { RepoReads } from "./repo-reads.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./tenant-config.js"; import { bitbucketFailed, githubFailed } from "./github-refusal.js"; +import { createKeyedSerial } from "../keyed-serial.js"; import type { Member } from "../integrations/app.js"; export const PolicyFileBody = type({ repo: /^[\w.-]+\/[\w.-]+$/, "source?": type.enumerated(...REPO_SOURCES) }); export const PolicyExportBody = PolicyFileBody.and({ "text?": "string" }); +export const PolicyPatchBody = PolicyFileBody.and({ changes: PolicyPatchChanges, "preview?": "boolean" }); const HOST_NAMES: Record = { github: "GitHub", "bitbucket-datacenter": "Bitbucket" }; @@ -41,6 +43,30 @@ function pullBody(state: "upgrade" | "differs", path: string, unexpressed: reado return [...lead, "", "Once merged, triage reads its rules from this file, and you change them by editing it.", ...left].join("\n"); } +function optionValue(value: unknown): string { + return typeof value === "string" ? value : JSON.stringify(value); +} + +function changeSentence(change: PolicyPatchChange): string { + switch (change.kind) { + case "rule": + return `${change.enabled ? "Turns on" : "Turns off"} \`${change.id}\`.`; + case "rule-options": + return `Sets ${Object.entries(change.options).map(([key, value]) => `\`${change.id}\` ${key} to ${optionValue(value)}`).join(", ")}.`; + case "add-rule": + return `Adds the rule \`${fileRuleId(change.rule)}\`.`; + case "remove-rule": + return `Removes the rule \`${change.id}\`.`; + case "code-review": + return `${change.on ? "Turns on" : "Turns off"} code review.`; + } +} + +/** What one batch changes, in the words a maintainer reviewing it reads; an open pull request gets it added to its body. */ +function patchPullBody(path: string, changes: readonly PolicyPatchChange[]): string { + return [`Updates the rules in \`${path}\`:`, "", ...changes.map((change) => `- ${changeSentence(change)}`)].join("\n"); +} + const ADD_TITLE = `chore: add a Corbits Triage policy in ${POLICY_FILE_PATH}`; function addBody(lead: string): string { @@ -101,6 +127,12 @@ const UNHEARD_MAX_AGE_MS = 60_000; export function createPolicyFile(deps: PolicyFileDeps) { const cache = new Map; at: number }>(); + /** One repository's policy pull request writes, so each one's read, commit and body edit finish before the next reads. */ + const serialized = createKeyedSerial(); + + function repoKey(tenantId: string, record: RepoRecord): string { + return `${tenantId}\n${sourceOf(record)}\n${record.name}`; + } /** * A read is kept until a push re-reads it while the repository's webhook is heard; where it is not (a hub the host @@ -142,15 +174,15 @@ export function createPolicyFile(deps: PolicyFileDeps) { return bb; } - async function readAt(tenantId: string, record: RepoRecord, path: string): Promise { - if (sourceOf(record) !== "bitbucket-datacenter") return readRepoFile(await repoGithub(tenantId, record), record.name, path); - return bitbucket.readRepoFile(await repoBitbucket(tenantId), record.name, path, record.defaultBranch); + async function readAt(tenantId: string, record: RepoRecord, path: string, branch?: string): Promise { + if (sourceOf(record) !== "bitbucket-datacenter") return readRepoFile(await repoGithub(tenantId, record), record.name, path, branch); + return bitbucket.readRepoFile(await repoBitbucket(tenantId), record.name, path, branch ?? record.defaultBranch); } - /** The first of the policy file's names found on the record's default branch. */ - async function findFile(tenantId: string, record: RepoRecord): Promise { + /** The first of the policy file's names found on `branch`, else on the record's default branch. */ + async function findFile(tenantId: string, record: RepoRecord, branch?: string): Promise { for (const path of POLICY_FILE_PATHS) { - const read = await readAt(tenantId, record, path); + const read = await readAt(tenantId, record, path, branch); if (read) return { path, read }; } return undefined; @@ -165,7 +197,7 @@ export function createPolicyFile(deps: PolicyFileDeps) { /** A failed read is not kept, so the next trigger reads again rather than run on a stale answer. */ async function cached(tenantId: string, record: RepoRecord, maxAgeMs: number): Promise { - const key = `${tenantId}\n${sourceOf(record)}\n${record.name}`; + const key = repoKey(tenantId, record); const kept = cache.get(key); if (kept && Date.now() - kept.at < maxAgeMs) return kept.read; const entry = { read: load(tenantId, record), at: Date.now() }; @@ -233,9 +265,9 @@ export function createPolicyFile(deps: PolicyFileDeps) { return bitbucketPull.proposePolicy(bb, record.name, await bitbucketBase(bb, record), change); } - async function opened(tenantId: string, record: RepoRecord, change: github.PolicyChange): Promise { + async function opened(tenantId: string, record: RepoRecord, change: github.PolicyChange, msg = "policy_file_exported"): Promise { const pull = await propose(tenantId, record, change); - deps.log({ level: "info", msg: "policy_file_exported", tenantId, repo: record.name, number: pull.number }); + deps.log({ level: "info", msg, tenantId, repo: record.name, number: pull.number }); return Response.json({ status: "opened", ...pull }); } @@ -276,9 +308,15 @@ export function createPolicyFile(deps: PolicyFileDeps) { * Opens the pull request writing the saved rules to the policy file, or brings the open one up to date; an enabled * repository with neither gets a starter file instead, or `text` when sent (see `chosenFile`). Refused when nothing differs. */ - async function exportRoute({ repo, source, text: chosen }: typeof PolicyExportBody.infer, member: Member): Promise { - const record = await grantedRecord(member, repo, source, "use"); + async function exportRoute(body: typeof PolicyExportBody.infer, member: Member): Promise { + const record = await grantedRecord(member, body.repo, body.source, "use"); if (record instanceof Response) return record; + return serialized(repoKey(member.tenantId, record), function exportOne() { + return exportRecord(body, member, record); + }); + } + + async function exportRecord({ repo, text: chosen }: typeof PolicyExportBody.infer, member: Member, record: RepoRecord): Promise { try { if (chosen !== undefined) return await chosenFile(member, record, chosen); const { upgrade, saved } = await deps.settle(member.tenantId, repo, sourceOf(record)); @@ -295,6 +333,70 @@ export function createPolicyFile(deps: PolicyFileDeps) { } } + function textOfFound(found: FoundFile): { text: string; sha: string } | Response { + if (found.read.kind === "not-a-file") return failure(422, "policy_file_invalid", notAFileReason(found.path)); + if (found.read.text === null) return failure(422, "policy_file_invalid", `${found.path} is too large or uses aliases.`); + return { text: found.read.text, sha: found.read.sha }; + } + + /** + * The file to patch and the sha to commit against. Without a policy pull request the branch starts from the default + * branch, so its sha holds. With one open, its branch's file wins so edits there survive; a branch without one takes + * the default branch's text and is committed as a new file (sha null). + */ + async function policySource(tenantId: string, record: RepoRecord): Promise<{ path: string; text: string; sha: string | null } | Response> { + const pull = await openPull(tenantId, record); + const onBranch = pull ? await findFile(tenantId, record, github.POLICY_BRANCH) : undefined; + const found = onBranch ?? await findFile(tenantId, record); + if (!found) return failure(409, "policy_file_absent", `${record.name} has no policy file. Open one with the export route before changing it.`); + const read = textOfFound(found); + if (read instanceof Response) return read; + return { path: found.path, text: read.text, sha: pull && !onBranch ? null : read.sha }; + } + + /** + * Patches the existing policy file in place and opens the one policy pull request, or adds the batch to the open one, + * whose title and earlier batches stay. `preview` returns the file before and after and writes nothing. Refused when + * there is no file, or the batch or the file is refused. + */ + async function patchRoute(body: typeof PolicyPatchBody.infer, member: Member): Promise { + const record = await grantedRecord(member, body.repo, body.source, "use"); + if (record instanceof Response) return record; + return serialized(repoKey(member.tenantId, record), function patchOne() { + return patchRecord(body, member, record); + }); + } + + async function patchRecord({ repo, changes, preview }: typeof PolicyPatchBody.infer, member: Member, record: RepoRecord): Promise { + try { + const found = await policySource(member.tenantId, record); + if (found instanceof Response) return found; + const patched = patchPolicyFile(found.text, record.name, changes); + if (!patched.ok) return failure(422, "policy_file_invalid", patched.message); + // A batch that changes nothing writes nothing; a pull request for it alone would have no commits to show. + if (!patched.changed) return Response.json({ before: found.text, after: found.text }); + if (preview === true) return Response.json({ before: found.text, after: patched.text }); + return await opened(member.tenantId, record, { + path: found.path, + text: patched.text, + title: `chore: update the Corbits Triage rules in ${found.path}`, + body: patchPullBody(found.path, changes), + append: true, + readSha: found.sha, + }, "policy_file_patched"); + } catch (err) { + if (err instanceof github.PolicyFileMoved) return failure(409, "policy_file_changed", `${err.path} changed since it was read; send the changes again.`); + if (err instanceof github.DescriptionNotUpdated) { + deps.log({ level: "warn", msg: "policy_pull_description_failed", tenantId: member.tenantId, repo, commit: err.commit, error: errorText(err.reason) }); + const landed = err.commit === undefined ? "The change is on the policy pull request" : `Commit ${err.commit} landed on the policy pull request`; + return failure(502, "description_not_updated", `${landed}, but its description could not be updated; the next change to this policy adds it.`); + } + deps.log({ level: "warn", msg: "policy_file_patch_failed", tenantId: member.tenantId, repo, error: errorText(err) }); + const what = `open the pull request updating ${repo}'s policy file`; + return sourceOf(record) === "github" ? githubFailed(what, err) : bitbucketFailed(what, err); + } + } + async function upgradeOf(member: Member, record: RepoRecord, listed: () => Promise): Promise { if (await sourceCredentialGrant(deps, member.principalId, member.tenantId, record, "read")) return undefined; const row = { repo: record.name, source: sourceOf(record) }; @@ -326,5 +428,5 @@ export function createPolicyFile(deps: PolicyFileDeps) { return Response.json({ repos: rows.filter((row) => row !== undefined) }); } - return { current, onPush, fileRoute, exportRoute, upgradesRoute }; + return { current, onPush, fileRoute, exportRoute, patchRoute, upgradesRoute }; } diff --git a/apps/hub/src/github/policy-pull.test.ts b/apps/hub/src/github/policy-pull.test.ts new file mode 100644 index 000000000..cf78653a3 --- /dev/null +++ b/apps/hub/src/github/policy-pull.test.ts @@ -0,0 +1,135 @@ +import { expect, test } from "bun:test"; +import { bodyWith, DescriptionNotUpdated, missingSections, PolicyFileMoved, proposePolicy, PULL_BODY_MAX } from "./policy-pull.js"; + +const TITLE = "chore: update the Corbits Triage rules"; + +test("a body near the limit drops its oldest sections and keeps the one it was opened with", () => { + const opened = "Updates the rules in `.corbits-triage.yml`:\n\n- Turns on `docs`."; + let body: string = opened; + for (let batch = 0; batch < 40; batch += 1) body = bodyWith(body, [{ commit: `c${batch}`, text: `batch ${batch}\n${"x".repeat(2_000)}` }]); + expect(body.length).toBeLessThanOrEqual(PULL_BODY_MAX); + expect(body).toStartWith(`${opened}\n\n`); + expect(body).toEndWith(`batch 39\n${"x".repeat(2_000)}`); + expect(body).not.toContain("batch 0\n"); + expect(body).toContain("batch 20\n"); + // A section cut for room keeps its marker, so its commit is not added back by the next batch. + expect(body).toContain("(corbits-triage added c0)"); + expect(missingSections(body, [{ commit: "c0", message: `${TITLE}\n\nbatch 0` }], TITLE)).toEqual([]); +}); + +test("the cap holds when the body has CRLF endings, and a section too long for it ends truncated", () => { + const opened = "Opened with this."; + const crlf = bodyWith(opened, [{ commit: "c0", text: "batch 0" }]).replace(/\n/g, "\r\n"); + const capped = bodyWith(`${crlf}\r\n`, [{ commit: "c1", text: `batch 1\n${"x".repeat(PULL_BODY_MAX)}` }]); + expect(capped.length).toBeLessThanOrEqual(PULL_BODY_MAX); + expect(capped).toStartWith(`${opened}\n\n`); + expect(capped).not.toContain("batch 0"); + expect(capped).toContain("batch 1\n"); + expect(capped).toEndWith("\n(truncated)"); + const full = bodyWith("x".repeat(PULL_BODY_MAX), [{ commit: "c2", text: "batch 2" }]); + expect(full.length).toBeLessThanOrEqual(PULL_BODY_MAX); + expect(full).toContain("(corbits-triage added c2)"); + expect(full).not.toContain("batch 2"); +}); + +test("markers past the cap give way to one line, and the commits it leaves out are not added back", () => { + function sha(at: number): string { + return at.toString(16).padStart(40, "0"); + } + const body = ["Opened.", ...Array.from({ length: 900 }, (_, at) => `[//]: # (corbits-triage added ${sha(at)})`)].join("\n\n"); + const capped = bodyWith(body, [{ commit: sha(900), text: "newest" }]); + expect(capped.length).toBeLessThanOrEqual(PULL_BODY_MAX); + expect(capped).toStartWith("Opened.\n\n"); + expect(capped.split("Earlier batches are left out to fit the description.")).toHaveLength(2); + expect(capped).not.toContain(`added ${sha(0)})`); + expect(capped).toEndWith("newest"); + const commits = Array.from({ length: 902 }, (_, at) => ({ commit: sha(at), message: `${TITLE}\n\nbatch ${at}` })); + expect(missingSections(capped, commits, TITLE)).toEqual([{ commit: sha(901), text: "batch 901" }]); + expect(bodyWith(capped, [{ commit: sha(901), text: "next" }]).length).toBeLessThanOrEqual(PULL_BODY_MAX); +}); + +test("only the batch commits the description does not name yet get a section", () => { + const body = bodyWith("Opened.", [{ commit: "c1", text: "first" }]); + const commits = [ + { commit: "c1", message: `${TITLE}\n\nfirst` }, + { commit: "c2", message: "chore: add a Corbits Triage policy" }, + { commit: "c3", message: `${TITLE}\n\nthird` }, + ]; + expect(missingSections(body, commits, TITLE)).toEqual([{ commit: "c3", text: "third" }]); +}); + +const CHANGE = { path: ".corbits-triage.yml", text: "rules: []\n", title: TITLE, body: "- Turns on `docs`.", append: true }; + +/** An open policy pull request on `acme/w`; `put` answers the commit, `pages` the commit listing, and `edits` holds each description written. */ +function fakeGithub(put: () => Response, pages: Array>, edits: string[], opened: string | null = null) { + return async function gh(path: string, init?: RequestInit): Promise { + const method = init?.method ?? "GET"; + const pull = { number: 1, html_url: "https://github.com/acme/w/pull/1", head: { sha: "head" }, body: edits.at(-1) ?? opened }; + if (method === "GET" && path === "/repos/acme/w") return Response.json({ default_branch: "main" }); + if (method === "GET" && path.startsWith("/repos/acme/w/pulls?")) return Response.json([pull]); + if (method === "GET" && path.startsWith("/repos/acme/w/contents/")) return new Response(null, { status: 404 }); + if (method === "PUT") return put(); + if (method === "GET" && path.startsWith("/repos/acme/w/pulls/1/commits")) { + const page = Number(new URL(path, "https://github.invalid").searchParams.get("page") ?? "1"); + const next = page < pages.length ? { link: `; rel="next"` } : undefined; + return Response.json(pages[page - 1], { headers: next }); + } + if (method === "GET" && path === "/repos/acme/w/pulls/1") return Response.json(pull); + if (method === "PATCH" && path === "/repos/acme/w/pulls/1") { + const { body } = JSON.parse(String(init!.body)) as { body: string }; + edits.push(body); + return Response.json({ number: 1, html_url: "https://github.com/acme/w/pull/1", head: { sha: "head" }, body }); + } + return new Response(null, { status: 404 }); + }; +} + +test("a file that appears on the branch between the read and a write naming no sha is a moved file; another 422 is GitHub's refusal", async () => { + const appeared = fakeGithub(() => Response.json({ message: "Invalid request.\n\n\"sha\" wasn't supplied." }, { status: 422 }), [[]], []); + await expect(proposePolicy(appeared, "acme/w", { ...CHANGE, readSha: null })).rejects.toBeInstanceOf(PolicyFileMoved); + const protectedBranch = fakeGithub(() => Response.json({ message: "Repository rule violations found" }, { status: 422 }), [[]], []); + const refused = proposePolicy(protectedBranch, "acme/w", { ...CHANGE, readSha: null }); + await expect(refused).rejects.not.toBeInstanceOf(PolicyFileMoved); + await expect(refused).rejects.toThrow("-> 422 Repository rule violations found"); +}); + +test("a lost section past the first hundred commits is found and added", async () => { + const commits = Array.from({ length: 101 }, (_, at) => ({ sha: `c${at + 1}`, commit: { message: `${TITLE}\n\nbatch ${at + 1}` } })); + const edits: string[] = []; + const gh = fakeGithub(() => Response.json({ commit: { sha: "c102" } }, { status: 201 }), [commits.slice(0, 100), commits.slice(100)], edits); + await proposePolicy(gh, "acme/w", { ...CHANGE, body: "batch 102" }); + expect(edits).toHaveLength(1); + expect(edits[0]).toContain("(corbits-triage added c101)\n\nbatch 101"); + expect(edits[0]!.endsWith("(corbits-triage added c102)\n\nbatch 102")).toBe(true); +}); + +test("an export over a description holding batch sections keeps them and adds its own, so the next batch adds none back", async () => { + const pages = [[{ sha: "c1", commit: { message: `${TITLE}\n\nbatch 1` } }]]; + const edits: string[] = []; + let written = 1; + function put(): Response { + written += 1; + return Response.json({ commit: { sha: `c${written}` } }, { status: 201 }); + } + const gh = fakeGithub(put, pages, edits, bodyWith("", [{ commit: "c1", text: "batch 1" }])); + await proposePolicy(gh, "acme/w", { ...CHANGE, title: "chore: keep the rules in the file", body: "exported", append: false }); + expect(edits.at(-1)).toContain("(corbits-triage added c1)\n\nbatch 1"); + expect(edits.at(-1)!.endsWith("(corbits-triage added c2)\n\nexported")).toBe(true); + await proposePolicy(gh, "acme/w", { ...CHANGE, body: "batch 3" }); + expect(edits.at(-1)!.split("(corbits-triage added c1)")).toHaveLength(2); + expect(edits.at(-1)!.endsWith("(corbits-triage added c3)\n\nbatch 3")).toBe(true); +}); + +test("a description edit GitHub refuses for good is its refusal at once, not retried", async () => { + let patches = 0; + const gh = fakeGithub(() => Response.json({ commit: { sha: "c1" } }, { status: 201 }), [[]], []); + async function refusing(path: string, init?: RequestInit): Promise { + if (init?.method !== "PATCH") return gh(path, init); + patches += 1; + return Response.json({ message: "Resource not accessible by integration" }, { status: 403 }); + } + const refused = proposePolicy(refusing, "acme/w", CHANGE); + await expect(refused).rejects.not.toBeInstanceOf(DescriptionNotUpdated); + await expect(refused).rejects.toThrow("-> 403 Resource not accessible by integration"); + expect(patches).toBe(1); +}); diff --git a/apps/hub/src/github/policy-pull.ts b/apps/hub/src/github/policy-pull.ts index 661e78b5b..19128923b 100644 --- a/apps/hub/src/github/policy-pull.ts +++ b/apps/hub/src/github/policy-pull.ts @@ -1,13 +1,73 @@ // The pull request that writes a repository's saved rules to its policy file, on one branch so it is opened once: // asking again updates the open one, and with none open the branch starts over from the default branch. -import { json, readRepoFileResponse, type GithubFetch } from "@corbits/github-reads"; +import { errorText } from "@corbits/common"; +import { json, listPrCommits, readRepoFileResponse, requestError, type GithubFetch } from "@corbits/github-reads"; +import { BitbucketFailed } from "../bitbucket-datacenter/client.js"; +import { GithubRateLimited } from "./github-usage.js"; export const POLICY_BRANCH = "corbits-triage/policy"; -export type PolicyChange = { path: string; text: string; title: string; body: string }; +/** + * With `append`, an open pull request keeps its title and gets `body` added after its own as a section named by its + * commit, and the commit message carries `body` so a section lost to a failed edit is added by the next batch; without, + * it takes both. `readSha` is the sha of the file on the policy branch that `text` was built from, null when the branch + * had none; set, a file that has changed since is refused with `PolicyFileMoved`. + */ +export type PolicyChange = { path: string; text: string; title: string; body: string; append?: boolean; readSha?: string | null }; + +export class PolicyFileMoved extends Error { + constructor(readonly path: string) { + super(`${path} changed since it was read`); + } +} + +/** The commit landed but the pull request's description could not be brought up to it. */ +export class DescriptionNotUpdated extends Error { + constructor(readonly commit: string | undefined, readonly reason: unknown) { + super(`the policy pull request's description was not updated${commit === undefined ? "" : ` for commit ${commit}`}`); + } +} + +export function commitMessage({ title, body, append }: PolicyChange): string { + return append ? `${title}\n\n${body}` : title; +} + +const EDIT_DELAYS_MS = [100, 200, 400]; + +/** + * A host answer of 429 or 5xx, or none at all, may pass, as may Bitbucket's 409 for a description edited since it was + * read, which the next try reads again; the hub's own GitHub limit and any other refusal are final. + */ +function isTransient(err: unknown): boolean { + if (err instanceof GithubRateLimited) return false; + if (err instanceof BitbucketFailed) return err.status === 409 || err.status === 429 || err.status >= 500; + const status = /-> (\d{3})\b/.exec(errorText(err))?.[1]; + return status === undefined || status === "429" || status.startsWith("5"); +} + +/** + * A batch's description edit, tried again after each of `EDIT_DELAYS_MS` while it fails in a way that may pass; still + * failing so, it is `DescriptionNotUpdated`. Any other failure is thrown as the host gave it. + */ +export async function retriedEdit(commit: string | undefined, edit: () => Promise): Promise { + for (const delay of EDIT_DELAYS_MS) { + try { + return await edit(); + } catch (err) { + if (!isTransient(err)) throw err; + } + await Bun.sleep(delay); + } + try { + return await edit(); + } catch (err) { + throw isTransient(err) ? new DescriptionNotUpdated(commit, err) : err; + } +} + export type PolicyPull = { url: string; number: number; headSha?: string }; -type Pull = { number: number; html_url: string; head: { sha: string } }; +type Pull = { number: number; html_url: string; head: { sha: string }; body: string | null }; function pullOf(pull: Pull): PolicyPull { return { url: pull.html_url, number: pull.number, headSha: pull.head.sha }; @@ -36,13 +96,132 @@ async function branchFromBase(gh: GithubFetch, repo: string, base: string): Prom } /** The commit made, undefined when the branch already held the text. */ -async function commitFile(gh: GithubFetch, repo: string, { path, text, title }: PolicyChange): Promise { +async function commitFile(gh: GithubFetch, repo: string, change: PolicyChange): Promise { + const { path, text, readSha } = change; const url = `/repos/${repo}/contents/${path}`; const current = await readRepoFileResponse(await gh(`${url}?ref=${encodeURIComponent(POLICY_BRANCH)}`, { headers: { accept: "application/vnd.github+json" } }), url); if (current?.kind === "file" && current.text === text) return undefined; - const sha = current?.kind === "file" ? { sha: current.sha } : {}; - const written = await json(gh, url, { method: "PUT", body: JSON.stringify({ message: title, content: Buffer.from(text, "utf8").toString("base64"), branch: POLICY_BRANCH, ...sha }) }); - return written.commit.sha; + const currentSha = current?.kind === "file" ? current.sha : undefined; + if (readSha !== undefined && (currentSha ?? null) !== readSha) throw new PolicyFileMoved(path); + const sha = currentSha === undefined ? {} : { sha: currentSha }; + const res = await gh(url, { method: "PUT", body: JSON.stringify({ message: commitMessage(change), content: Buffer.from(text, "utf8").toString("base64"), branch: POLICY_BRANCH, ...sha }) }); + // GitHub refuses the write with 409 when the file moved off `sha` after the read above. + if (res.status === 409 && readSha !== undefined) throw new PolicyFileMoved(path); + if (res.ok) return (await res.json() as { commit: { sha: string } }).commit.sha; + const refused = await requestError(res, "PUT", url); + // A file that appeared where none was read is refused with 422 for the sha the write did not name. + if (res.status === 422 && readSha === null && /\bsha\b/.test(refused.message)) throw new PolicyFileMoved(path); + throw refused; +} + +/** One batch's part of the description, named by the commit that made it. */ +type Section = { commit: string; text: string }; +type BranchCommit = { commit: string; message: string }; +/** The text before the first section, the newest commit whose section was dropped for length, and the sections kept. */ +type Body = { opened: string; omitted?: string; sections: Section[] }; + +/** Starts each section, naming its commit; a Markdown comment, so it shows nothing. */ +function marker(commit: string): string { + return `[//]: # (corbits-triage added ${commit})`; +} +const MARKER = /^\[\/\/\]: # \(corbits-triage added ?([0-9A-Za-z]*)\)(?:\n\n([\s\S]*))?$/; +const OMITTED = /^\[\/\/\]: # \(corbits-triage omitted through ([0-9A-Za-z]+)\)(?:\n\n[\s\S]*)?$/; +const BEFORE_MARKER = /\n\n(?=\[\/\/\]: # \(corbits-triage (?:added|omitted through) )/; + +function omission(commit: string): string { + return `[//]: # (corbits-triage omitted through ${commit})\n\nEarlier batches are left out to fit the description.`; +} + +/** Under GitHub's 65,536-character body limit, with room. */ +export const PULL_BODY_MAX = 60_000; + +const TRUNCATED = "\n(truncated)"; + +/** CRLF endings from an edit on the host are read as LF. */ +function sectionsOf(body: string | null | undefined): Body { + const chunks = (body ?? "").replace(/\r\n/g, "\n").trimEnd().split(BEFORE_MARKER); + const opened = MARKER.test(chunks[0]!) || OMITTED.test(chunks[0]!) ? "" : chunks.shift()!; + const omitted = OMITTED.exec(chunks[0] ?? "")?.[1]; + if (omitted !== undefined) chunks.shift(); + const sections = chunks.map(function sectionOf(chunk) { + const [, commit = "", text = ""] = MARKER.exec(chunk) ?? []; + return { commit, text }; + }); + return { opened, omitted, sections }; +} + +function joined({ opened, omitted, sections }: Body): string { + const written = sections.map((section) => (section.text === "" ? marker(section.commit) : `${marker(section.commit)}\n\n${section.text}`)); + return [opened, omitted === undefined ? "" : omission(omitted), ...written].filter((part) => part !== "").join("\n\n"); +} + +/** `text` shortened by `over` characters and marked as cut; as it is when nothing is over. */ +function cut(text: string, over: number): string { + if (over <= 0) return text; + return `${text.slice(0, Math.max(0, text.length - over - TRUNCATED.length))}${TRUNCATED}`; +} + +/** + * The description with `added` as its newest sections, never past `PULL_BODY_MAX`. The oldest sections lose their + * text first but keep their marker, so their commits stay named and are not added again. When the markers alone are + * too long, the oldest are dropped and one line says so, naming the newest commit dropped so the commits before it + * are not added again either. Still too long, the newest section and then the opening lose their tail. + */ +export function bodyWith(body: string | null | undefined, added: readonly Section[]): string { + const read = sectionsOf(body); + const fitted: Body = { ...read, sections: [...read.sections, ...added] }; + const { sections } = fitted; + for (const [at, section] of sections.slice(0, -1).entries()) { + if (section.text === "") continue; + if (joined(fitted).length <= PULL_BODY_MAX) break; + sections[at] = { ...section, text: "" }; + } + while (sections.length > 1 && joined(fitted).length > PULL_BODY_MAX) fitted.omitted = sections.shift()!.commit; + const newest = sections.at(-1); + if (newest !== undefined) sections[sections.length - 1] = { ...newest, text: cut(newest.text, joined(fitted).length - PULL_BODY_MAX) }; + fitted.opened = cut(fitted.opened, joined(fitted).length - PULL_BODY_MAX); + return joined(fitted); +} + +/** + * The description a change that is not appended leaves: its own `body`, or, on a description holding batch sections, + * those sections with the change added as one more, so later batches find every commit named. + */ +export function replacedBody(current: string | null | undefined, change: PolicyChange, committed: string | undefined): string { + const read = sectionsOf(current); + if (read.sections.length === 0 && read.omitted === undefined) return change.body; + return bodyWith(current, committed === undefined ? [] : [{ commit: committed, text: change.body }]); +} + +/** A section for each of the branch's batch commits, oldest first, that the description does not name or leave out yet. */ +export function missingSections(body: string | null | undefined, commits: readonly BranchCommit[], title: string): Section[] { + const { omitted, sections } = sectionsOf(body); + const named = new Set(sections.map((section) => section.commit)); + const after = omitted === undefined ? 0 : commits.findIndex((row) => row.commit === omitted) + 1; + // A message sent as a form field, as Bitbucket's is, comes back with CRLF endings. + const batches = commits.slice(after).map((row) => ({ ...row, message: row.message.replace(/\r\n/g, "\n") })).filter((row) => !named.has(row.commit) && row.message.startsWith(`${title}\n\n`)); + return batches.map((row) => ({ commit: row.commit, text: row.message.slice(title.length + 2).trim() })); +} + +/** The branch's commits with `committed`, which a listing read just after the write may not hold yet. */ +export function withCommitted(commits: readonly BranchCommit[], committed: string | undefined, change: PolicyChange): BranchCommit[] { + if (committed === undefined || commits.some((row) => row.commit === committed)) return [...commits]; + return [...commits, { commit: committed, message: commitMessage(change) }]; +} + +/** Adds a section for each batch commit the description lacks, the new one included; with none lacking, no edit is made. */ +async function appendSections(gh: GithubFetch, repo: string, number: number, change: PolicyChange, committed: string | undefined): Promise { + const path = `/repos/${repo}/pulls/${number}`; + return retriedEdit(committed, async function appendMissing() { + // The description is read just before it is written; GitHub takes no version on the edit, and the hub runs one + // batch per repository at a time. + const latest: Pull = await json(gh, path); + const listed = await listPrCommits(gh, repo, number); + const commits = withCommitted(listed.map((row) => ({ commit: row.sha, message: row.message })), committed, change); + const added = missingSections(latest.body, commits, change.title); + if (added.length === 0) return latest; + return json(gh, path, { method: "PATCH", body: JSON.stringify({ body: bodyWith(latest.body, added) }) }); + }); } /** Opens the pull request, or brings the open one up to `change`. */ @@ -51,18 +230,21 @@ export async function proposePolicy(gh: GithubFetch, repo: string, change: Polic const open = await openPull(gh, repo, base); if (!open) await branchFromBase(gh, repo, base); const committed = await commitFile(gh, repo, change); - if (open) { - const updated = pullOf(await json(gh, `/repos/${repo}/pulls/${open.number}`, { method: "PATCH", body: JSON.stringify({ title: change.title, body: change.body }) })); - // GitHub may answer the update before it has moved the pull request's head to the new commit. - return committed === undefined ? updated : { ...updated, headSha: committed }; - } - return createPull(gh, repo, base, change); + if (!open) return createPull(gh, repo, base, change, committed); + const updated = pullOf(change.append ? await appendSections(gh, repo, open.number, change, committed) : await json(gh, `/repos/${repo}/pulls/${open.number}`, { method: "PATCH", body: JSON.stringify({ title: change.title, body: replacedBody(open.body, change, committed) }) })); + // GitHub may answer the update before it has moved the pull request's head to the new commit. + return committed === undefined ? updated : { ...updated, headSha: committed }; +} + +/** An appended pull request opens with its first batch as a section, so later batches see it named. */ +export function openingBody(change: PolicyChange, committed: string | undefined): string { + return change.append && committed !== undefined ? bodyWith("", [{ commit: committed, text: change.body }]) : change.body; } /** A pull request opened by a concurrent call in between is answered instead of refused. */ -async function createPull(gh: GithubFetch, repo: string, base: string, { title, body }: PolicyChange): Promise { +async function createPull(gh: GithubFetch, repo: string, base: string, change: PolicyChange, committed: string | undefined): Promise { const path = `/repos/${repo}/pulls`; - const res = await gh(path, { method: "POST", headers: { accept: "application/vnd.github+json" }, body: JSON.stringify({ title, head: POLICY_BRANCH, base, body }) }); + const res = await gh(path, { method: "POST", headers: { accept: "application/vnd.github+json" }, body: JSON.stringify({ title: change.title, head: POLICY_BRANCH, base, body: openingBody(change, committed) }) }); if (res.ok) return pullOf(await res.json() as Pull); const raced = res.status === 422 ? await openPull(gh, repo, base) : undefined; if (raced) return pullOf(raced); diff --git a/apps/hub/src/integrations/app.ts b/apps/hub/src/integrations/app.ts index 42bece8f7..6ca978bf8 100644 --- a/apps/hub/src/integrations/app.ts +++ b/apps/hub/src/integrations/app.ts @@ -9,7 +9,7 @@ import type { DB } from "@intx/db"; import { getLogger } from "@intx/log"; import { honoLogger, type HonoContext } from "@intx/log/hono"; import { createHubContextMiddleware, createResolveTenant, requireAuth, type GetSession, type RequireGrant, type SessionInfo, type SessionUser, type TenantEnv } from "@intx/hub-api"; -import { AuditLog, AUTHOR_KINDS, BacklogStarted, REPO_SOURCES, CodeReviewPlace, checkPackSchema, checkPolicyFile, PolicyExported, PolicyFileRead, PolicyRead, PolicyUpgrades, PolicyValidated, POLICY_FILE_MAX_BYTES, DeployedVersions, InviteRoles, InviteSent, MAX_MERGE_STATES, policyFileSchema, policyOverrideSchema, repoPolicySchema, SessionList, TriageStats, TriageVerdicts } from "@corbits/triage-contracts"; +import { AuditLog, AUTHOR_KINDS, BacklogStarted, REPO_SOURCES, CodeReviewPlace, checkPackSchema, checkPolicyFile, PolicyExported, PolicyFileRead, PolicyPatchResponse, PolicyRead, PolicyUpgrades, PolicyValidated, POLICY_FILE_MAX_BYTES, DeployedVersions, InviteRoles, InviteSent, MAX_MERGE_STATES, policyFileSchema, policyOverrideSchema, repoPolicySchema, SessionList, TriageStats, TriageVerdicts } from "@corbits/triage-contracts"; import { AUDIT_PATH } from "../audit-log.js"; import { AUTH_METHODS_PATH } from "../auth.js"; import { CODE_REVIEW_PATH, CodeReviewQueued, CodeReviewRunBody } from "../code-review.js"; @@ -25,7 +25,7 @@ import { failure } from "../github/portal-credential.js"; import { ActionBody, GITHUB_PR_ACTIONS_PATH } from "../github/pr-actions.js"; import { GITHUB_PR_DETAILS_PATH } from "../github/pr-details.js"; import { GITHUB_PR_TRIAGE_PATH, TriageBody } from "../github/pr-triage.js"; -import { PolicyExportBody, PolicyFileBody } from "../github/policy-file.js"; +import { PolicyExportBody, PolicyFileBody, PolicyPatchBody } from "../github/policy-file.js"; import { PolicyQuery } from "../github/policy-read.js"; import { BacklogBody } from "../github/backlog-triage.js"; import { TRIAGE_STATS_PATH } from "../github/triage-stats.js"; @@ -74,6 +74,7 @@ export type IntegrationHandlers = { policyRead: BodyHandler; policyFileRead: BodyHandler; policyFileExport: BodyHandler; + policyFilePatch: BodyHandler; policyUpgrades: (member: Member) => Promise; backlog: BodyHandler; openPulls: (member: Member) => Promise; @@ -473,6 +474,31 @@ export function createIntegrationsApp(guards: IntegrationGuards, handlers: Integ }, ); + app.post( + `${GITHUB_PR_TRIAGE_PATH}/:tenantId/policy/patch`, + describeRoute({ + summary: "Patch a repository's policy file and open one pull request", + description: "Needs `use` on the host credential (GitHub App or Bitbucket Data Center) and a connected repository that already has a policy file. Reads `.corbits-triage.yml` or `.yaml` from the open `corbits-triage/policy` pull request when one exists, otherwise from the default branch, and changes only the pairs named in `changes`: turn a rule on or off, set a rule's options other than `check` and `id`, add or remove a custom rule, or turn the `code-review` block on or off. A built-in rule turned on needs its `reason`, and a size or path rule its limit, set in the same batch. A built-in rule turned on without one, or given a new limit, gets its sentence from its limit; one added with its own `text`, given one in the batch, or turned back on keeps it. Comments and untouched pairs stay. One pull request is opened for the batch; when it is already open, it keeps its title and the batch is added to its body. A batch that leaves the file's content as it was writes nothing and returns the file as both before and after. With `preview: true`, returns the file before and after the patch and writes nothing. Refused when the repository has no policy file (the export route creates one), or when the file or the batch is refused, including a file with no `rules` list, which is the older shape and needs converting first. `source` is the repository's host; without it, a name connected on both hosts is refused.", + tags: TAGS, + responses: { + 200: json("The patched file, before and after, or the pull request opened or brought up to date", PolicyPatchResponse), + ...invalid, + ...failures, + 409: json("GitHub, the workspace or the repository is not ready. `policy_file_absent`: the repository has no policy file. `policy_file_changed`: the file changed between its read and the commit; send the changes again. `repo_not_connected`: it is not connected, or the host was not named and both hosts hold the name", Failure), + 413: json("The body is too large", Failure), + 422: json("`policy_file_invalid`: the file is unreadable or refused, or the batch cannot be applied; `message` names the problem", Failure), + 502: json("`github_failed` or `bitbucket_failed`: the host refused. `description_not_updated`: the commit landed but the pull request's description could not be updated; the next change adds it", Failure), + ...githubLimited, + }, + }), + member, + policyBodyLimit, + body(PolicyPatchBody), + function policyFilePatch(c) { + return handlers.policyFilePatch(c.req.valid("json"), memberOf(c)); + }, + ); + app.post( `${GITHUB_PR_TRIAGE_PATH}/:tenantId/backlog`, describeRoute({ diff --git a/apps/hub/src/keyed-serial.ts b/apps/hub/src/keyed-serial.ts new file mode 100644 index 000000000..852ec2513 --- /dev/null +++ b/apps/hub/src/keyed-serial.ts @@ -0,0 +1,27 @@ +async function settled(work: Promise): Promise { + try { + await work; + } catch { + // The caller of `work` handles its failure; the chain only waits for it. + } +} + +async function runAfter(previous: Promise | undefined, work: () => Promise): Promise { + await previous; + return work(); +} + +/** Runs each piece of work after the earlier work for its key has settled, so work for one key never interleaves. */ +export function createKeyedSerial() { + const tails = new Map>(); + return async function serialized(key: string, work: () => Promise): Promise { + const current = runAfter(tails.get(key), work); + const tail = settled(current); + tails.set(key, tail); + try { + return await current; + } finally { + if (tails.get(key) === tail) tails.delete(key); + } + }; +} diff --git a/apps/hub/src/server.test.ts b/apps/hub/src/server.test.ts index ed01886eb..a2d280ee7 100644 --- a/apps/hub/src/server.test.ts +++ b/apps/hub/src/server.test.ts @@ -303,6 +303,7 @@ test("composed hub documents the integration routes and still answers an invalid "/api/integrations/github-triage/{tenantId}/policy": ["get"], "/api/integrations/github-triage/{tenantId}/policy/file": ["get"], "/api/integrations/github-triage/{tenantId}/policy/export": ["post"], + "/api/integrations/github-triage/{tenantId}/policy/patch": ["post"], "/api/integrations/github-triage/{tenantId}/policy/upgrades": ["get"], "/api/integrations/github-triage/{tenantId}/backlog": ["post"], "/api/integrations/github-open-pulls/{tenantId}": ["get"], diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index 94980d27c..4a13dcd50 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -779,6 +779,7 @@ const integrations = createIntegrationsApp({ db: composition.db, getSession: com policyRead: createGithubPolicyRead({ readCheckPack }), policyFileRead: policyFile.fileRoute, policyFileExport: policyFile.exportRoute, + policyFilePatch: policyFile.patchRoute, policyUpgrades: policyFile.upgradesRoute, backlog: createGithubBacklog({ ...credentialDeps, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2bbe65d6b..5585e77a1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -55,8 +55,12 @@ GitHub ──webhook──► Hub ──spawn──► Sidecar ──► listene writing the stored policy, or the starter policy for `start`, as a schema 3 file. For a repository with no file, no stored policy and no policy pull request open it can instead write a chosen `text`, refused - when triage would refuse it once merged, `extends` resolved. The file - and export routes take the repository's host as `source`; without it, + when triage would refuse it once merged, `extends` resolved. + `POST …/policy/patch` changes only the requested parts of the existing + file, so comments stay, and opens that same pull request, or adds the + batch to the open one's body under its title. + A preview returns the file before and after and writes nothing. The file, + export and patch 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 listener for the repository's host, as the diff --git a/packages/github-reads/src/index.ts b/packages/github-reads/src/index.ts index 02b37eb02..c38653f66 100644 --- a/packages/github-reads/src/index.ts +++ b/packages/github-reads/src/index.ts @@ -44,10 +44,15 @@ async function request(gh: GithubFetch, path: string, init?: RequestInit): Promi ...init, headers: { accept: "application/vnd.github+json", ...init?.headers }, }); - if (!res.ok) throw new Error(`github ${init?.method ?? "GET"} ${path} -> ${res.status} ${await errorMessage(res)}`.trimEnd()); + if (!res.ok) throw await requestError(res, init?.method ?? "GET", path); return res; } +/** The error a refused GitHub request throws, carrying its status and GitHub's reason; for a caller that sent the request itself. */ +export async function requestError(res: Response, method: string, path: string): Promise { + return new Error(`github ${method} ${path} -> ${res.status} ${await errorMessage(res)}`.trimEnd()); +} + export async function json(gh: GithubFetch, path: string, init?: RequestInit): Promise { const res = await request(gh, path, init); return res.status === 204 ? null : res.json(); @@ -921,10 +926,11 @@ function base64Text(content: string): string { return new TextDecoder().decode(Uint8Array.from(atob(content.replace(/\s/g, "")), (char) => char.charCodeAt(0))); } -/** A file on the default branch, or undefined when nothing is there. */ -export async function readRepoFile(gh: GithubFetch, repo: string, path: string): Promise { +/** A file on `ref`, the default branch when none is named, or undefined when nothing is there. */ +export async function readRepoFile(gh: GithubFetch, repo: string, path: string, ref?: string): Promise { const url = `/repos/${repo}/contents/${path}`; - return readRepoFileResponse(await gh(url, { headers: { accept: "application/vnd.github+json" } }), url); + const at = ref === undefined ? url : `${url}?ref=${encodeURIComponent(ref)}`; + return readRepoFileResponse(await gh(at, { headers: { accept: "application/vnd.github+json" } }), url); } /** A contents API response for one path, read as `readRepoFile` reads it; for a caller that sent the request itself. */ diff --git a/packages/triage-contracts/src/hub-responses.ts b/packages/triage-contracts/src/hub-responses.ts index a3a5e0380..47d95e4c7 100644 --- a/packages/triage-contracts/src/hub-responses.ts +++ b/packages/triage-contracts/src/hub-responses.ts @@ -272,6 +272,13 @@ const PolicyPull = type({ url: "string", number: "number", "headSha?": "string" export const PolicyExported = type({ status: "'opened'" }).and(PolicyPull); export type PolicyExported = typeof PolicyExported.infer; +/** No diff library is a dependency, so a preview is the file before the patch and the patched file. */ +export const PolicyPatchPreview = type({ before: "string", after: "string" }); +export type PolicyPatchPreview = typeof PolicyPatchPreview.infer; + +export const PolicyPatchResponse = PolicyPatchPreview.or(PolicyExported); +export type PolicyPatchResponse = typeof PolicyPatchResponse.infer; + /** * One way the file's policy runs differently from the saved one: the rule or step list, its values in each (empty is * off or nothing), and in plain words what changes if the file is used instead. diff --git a/packages/triage-contracts/src/index.ts b/packages/triage-contracts/src/index.ts index 30d05637c..c896e4b84 100644 --- a/packages/triage-contracts/src/index.ts +++ b/packages/triage-contracts/src/index.ts @@ -27,6 +27,7 @@ export * from "./stage-result.js"; export * from "./listener.js"; export * from "./rules-gate.js"; export * from "./policy-export.js"; +export * from "./policy-patch.js"; export * from "./code-patterns.js"; export * from "./visible-text.js"; export * from "./workflow-release.js"; diff --git a/packages/triage-contracts/src/policy-patch.test.ts b/packages/triage-contracts/src/policy-patch.test.ts new file mode 100644 index 000000000..020a9c8d3 --- /dev/null +++ b/packages/triage-contracts/src/policy-patch.test.ts @@ -0,0 +1,237 @@ +import { expect, test } from "bun:test"; +import { checkPolicyFile, policyFileReason } from "./policy-file.ts"; +import { patchPolicyFile, type PolicyPatchChange } from "./policy-patch.ts"; + +const REPO = "acme/widgets"; + +const FILE = `# Corbits Triage policy. +rules: + # leave ci-green alone + - check: ci-green + text: CI passes. + reason: Required by branch protection. + evidence: [maintainer-stated] + - check: focused + text: One focused change. + reason: Mixed changes were split. + evidence: [maintainer-stated] + - check: max-lines-changed + text: Flag pull requests over 400 lines changed. + reason: Review load. + evidence: [maintainer-stated] + max: 400 +pauseLabel: hold +`; + +const CHANGES: PolicyPatchChange[] = [ + { kind: "rule", id: "focused", enabled: false }, + { kind: "rule-options", id: "max-lines-changed", options: { max: 200 } }, + { kind: "rule", id: "docs", enabled: true }, + { kind: "rule-options", id: "docs", options: { reason: "Docs drift from the code." } }, + { kind: "code-review", on: true }, +]; + +test("a comment above an untouched rule survives a change to another rule", () => { + const patched = patchPolicyFile(FILE, REPO, CHANGES); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text.startsWith(FILE.slice(0, FILE.indexOf(" - check: focused")))).toBe(true); + const commentAt = patched.text.indexOf("# leave ci-green alone"); + const ciAt = patched.text.indexOf("check: ci-green"); + expect(commentAt).toBeGreaterThanOrEqual(0); + expect(commentAt).toBeLessThan(ciAt); + expect(patched.text).toContain("text: CI passes."); + expect(patched.text).toContain("pauseLabel: hold"); + expect(patched.text).toContain("text: Flag pull requests over 200 lines changed."); + expect(patched.text).not.toContain("over 400"); + expect(patched.text).toContain("max: 200"); + expect(patched.text).toContain("Check docs are updated when behavior changes."); + expect(patched.text).toContain("enabled: false"); + const focusedAt = patched.text.indexOf("check: focused"); + const nextAt = patched.text.indexOf("check:", focusedAt + 1); + expect(patched.text.slice(focusedAt, nextAt)).toContain("enabled: false"); + expect(patched.text.slice(ciAt, focusedAt)).not.toContain("enabled:"); + expect(checkPolicyFile(patched.text, REPO)).toMatchObject({ ok: true }); +}); + +test("a batch that cannot be applied names the problem and writes no file", () => { + const older = "checks:\n ci-green: { enabled: true }\n"; + expect(patchPolicyFile(older, REPO, [{ kind: "rule", id: "ci-green", enabled: false }])).toEqual({ + ok: false, + message: "This file has no rules list. It is the older policy shape and needs converting before it can be changed.", + }); + expect(patchPolicyFile(FILE, REPO, [{ kind: "rule", id: "made-up", enabled: true }])).toEqual({ + ok: false, + message: "Unknown rule made-up.", + }); + expect(patchPolicyFile(FILE, REPO, [{ kind: "remove-rule", id: "docs" }])).toEqual({ + ok: false, + message: "Rule docs is not in the file.", + }); + for (const options of [{ check: "focused" }, { id: "focused" }]) { + expect(patchPolicyFile(FILE, REPO, [{ kind: "rule-options", id: "ci-green", options }])).toEqual({ + ok: false, + message: "Rule ci-green's check and id cannot be changed.", + }); + } + const rule = { id: "errors-are-handled", check: "model" as const, text: "Errors are handled.", reason: "Outages.", evidence: ["maintainer-stated"] }; + const added = patchPolicyFile(FILE, REPO, [{ kind: "add-rule", rule }]); + expect(added.ok).toBe(true); + if (!added.ok) return; + expect(patchPolicyFile(added.text, REPO, [{ kind: "add-rule", rule }])).toEqual({ + ok: false, + message: "Rule errors-are-handled is already in the file.", + }); +}); + +const BASE = `rules: + - check: ci-green + text: CI passes. + reason: Required by branch protection. + evidence: [maintainer-stated] +`; + +test("options later in the batch apply to a built-in rule turned on earlier", () => { + const patched = patchPolicyFile(BASE, REPO, [ + { kind: "rule", id: "docs", enabled: true }, + { kind: "rule-options", id: "docs", options: { onFail: "label", label: "needs-docs", reason: "Docs drift from the code." } }, + ]); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text).toContain("check: docs"); + expect(patched.text).toContain("onFail: label"); + expect(patched.text).toContain("label: needs-docs"); + expect(checkPolicyFile(patched.text, REPO)).toMatchObject({ ok: true }); +}); + +test("a later change sees a custom rule added earlier in the batch", () => { + const patched = patchPolicyFile(BASE, REPO, [ + { kind: "add-rule", rule: { id: "errors-are-handled", check: "model", text: "Errors are handled.", reason: "Outages.", evidence: ["maintainer-stated"] } }, + { kind: "rule", id: "errors-are-handled", enabled: false }, + ]); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text).toContain("id: errors-are-handled"); + expect(patched.text).toContain("enabled: false"); + expect(checkPolicyFile(patched.text, REPO)).toMatchObject({ ok: true }); +}); + +test("a limit set later in the batch lands on a size rule turned on earlier", () => { + const patched = patchPolicyFile(BASE, REPO, [ + { kind: "rule", id: "max-lines-changed", enabled: true }, + { kind: "rule-options", id: "max-lines-changed", options: { max: 200, reason: "Review load." } }, + ]); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text).toContain("check: max-lines-changed"); + expect(patched.text).toContain("text: Flag pull requests over 200 lines changed."); + expect(patched.text).toContain("max: 200"); + expect(checkPolicyFile(patched.text, REPO)).toMatchObject({ ok: true }); +}); + +test("turning on a catalog rule without a required field or a reason names the rule and the field", () => { + expect(patchPolicyFile(BASE, REPO, [{ kind: "rule", id: "max-lines-changed", enabled: true }])).toEqual({ + ok: false, + message: "Rule max-lines-changed is missing max.", + }); + expect(patchPolicyFile(BASE, REPO, [{ kind: "rule", id: "no-forbidden-paths", enabled: true }])).toEqual({ + ok: false, + message: "Rule no-forbidden-paths is missing paths.", + }); + expect(patchPolicyFile(BASE, REPO, [{ kind: "rule", id: "docs", enabled: true }])).toEqual({ + ok: false, + message: "Rule docs is missing reason.", + }); +}); + +test("an untouched rule is not held to what the batch must set; the file's own check judges it", () => { + const incomplete = `${BASE} - check: focused + text: One focused change. + evidence: [maintainer-stated] +`; + const fileCheck = checkPolicyFile(incomplete, REPO); + expect(fileCheck.ok).toBe(false); + if (fileCheck.ok) return; + expect(patchPolicyFile(incomplete, REPO, [{ kind: "code-review", on: true }])).toEqual({ ok: false, message: policyFileReason(fileCheck.problems) }); +}); + +test("turning a disabled rule back on keeps its own text", () => { + const disabled = `${BASE} - check: max-lines-changed + text: Keep it small. + reason: Review load. + evidence: [maintainer-stated] + enabled: false + max: 200 +`; + const patched = patchPolicyFile(disabled, REPO, [{ kind: "rule", id: "max-lines-changed", enabled: true }]); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text).toContain("text: Keep it small."); + expect(patched.text).toContain("enabled: true"); +}); + +test("turning a rule on through its options does what turning it on does", () => { + const stale = `${BASE} - check: max-lines-changed + text: Flag pull requests over 400 lines changed. + reason: Review load. + evidence: [maintainer-stated] + enabled: false + max: 200 +`; + const viaOptions = patchPolicyFile(stale, REPO, [{ kind: "rule-options", id: "max-lines-changed", options: { enabled: true } }]); + expect(viaOptions).toEqual(patchPolicyFile(stale, REPO, [{ kind: "rule", id: "max-lines-changed", enabled: true }])); + expect(viaOptions).toMatchObject({ ok: true, changed: true }); + const added = patchPolicyFile(BASE, REPO, [{ kind: "rule-options", id: "docs", options: { enabled: true, reason: "Docs drift from the code." } }]); + expect(added).toEqual(patchPolicyFile(BASE, REPO, [ + { kind: "rule", id: "docs", enabled: true }, + { kind: "rule-options", id: "docs", options: { reason: "Docs drift from the code." } }, + ])); + expect(added).toMatchObject({ ok: true, text: expect.stringContaining("check: docs") }); +}); + +test("turning on a rule that is already on changes nothing", () => { + expect(patchPolicyFile(BASE, REPO, [{ kind: "rule", id: "ci-green", enabled: true }])).toEqual({ ok: true, text: BASE, changed: false }); +}); + +test("a rule's own text set in the batch outlasts a later limit or turn-on for it", () => { + const disabled = `${BASE} - check: max-lines-changed + text: Keep it small. + reason: Review load. + evidence: [maintainer-stated] + enabled: false + max: 400 +`; + const patched = patchPolicyFile(disabled, REPO, [ + { kind: "rule-options", id: "max-lines-changed", options: { text: "Keep pull requests short." } }, + { kind: "rule-options", id: "max-lines-changed", options: { max: 200 } }, + { kind: "rule", id: "max-lines-changed", enabled: true }, + ]); + expect(patched.ok).toBe(true); + if (!patched.ok) return; + expect(patched.text).toContain("text: Keep pull requests short."); + expect(patched.text).toContain("max: 200"); +}); + +test("a batch that changes nothing reads as unchanged even when the YAML library re-spaces the file", () => { + const unindented = `rules: +- check: max-lines-changed + text: "Flag pull requests over 400 lines changed." + reason: Review load. + evidence: [ maintainer-stated ] + max: 400 +`; + const same = patchPolicyFile(unindented, REPO, [{ kind: "rule-options", id: "max-lines-changed", options: { max: 400 } }]); + expect(same).toMatchObject({ ok: true, changed: false }); + if (!same.ok) return; + expect(same.text).not.toBe(unindented); + expect(patchPolicyFile(unindented, REPO, [{ kind: "rule-options", id: "max-lines-changed", options: { max: 300 } }])).toMatchObject({ ok: true, changed: true }); +}); + +test("a built-in rule added whole keeps its own text", () => { + const rule = { check: "max-lines-changed" as const, text: "Keep pull requests short.", reason: "Review load.", evidence: ["maintainer-stated"], max: 200 }; + const patched = patchPolicyFile(BASE, REPO, [{ kind: "add-rule", rule }]); + expect(patched).toMatchObject({ ok: true, changed: true }); + if (!patched.ok) return; + expect(patched.text).toContain("text: Keep pull requests short."); + expect(patched.text).toContain("max: 200"); +}); diff --git a/packages/triage-contracts/src/policy-patch.ts b/packages/triage-contracts/src/policy-patch.ts new file mode 100644 index 000000000..9442631cd --- /dev/null +++ b/packages/triage-contracts/src/policy-patch.ts @@ -0,0 +1,263 @@ +// A batch of edits to a schema 3 policy file, applied on the parsed document so comments and untouched pairs stay. +// `exportPolicyFile` rebuilds the file with `stringify`, which drops them; this path must not. +import { type } from "arktype"; +import { isMap, isScalar, isSeq, parseDocument, type Document, type YAMLSeq } from "yaml"; +import { asRecord } from "@corbits/common"; +import { CATALOG_IDS, emptyPack, isSizeRule, readCheckPack, type CatalogId } from "./check-pack.js"; +import { checkPolicyFile, policyFileReason } from "./policy-file.js"; +import { policyRuleSchema } from "./policy-rules.js"; +import { catalogSentence } from "./rule-prose.js"; + +const RuleId = "string > 0"; + +export const PolicyPatchChange = type({ kind: "'rule'", id: RuleId, enabled: "boolean" }) + .or({ kind: "'rule-options'", id: RuleId, options: "Record" }) + .or({ kind: "'add-rule'", rule: policyRuleSchema }) + .or({ kind: "'remove-rule'", id: RuleId }) + .or({ kind: "'code-review'", on: "boolean" }); +export type PolicyPatchChange = typeof PolicyPatchChange.infer; + +export const PolicyPatchChanges = PolicyPatchChange.array().atLeastLength(1).atMostLength(40); + +type PolicyRule = typeof policyRuleSchema.infer; + +/** `changed` is false when the patched file reads the same as before, however the YAML library re-spaced it. */ +type PolicyPatchOutcome = { ok: true; text: string; changed: boolean } | { ok: false; message: string }; + +const MIGRATION = "This file has no rules list. It is the older policy shape and needs converting before it can be changed."; +const WRITTEN = { lineWidth: 0, flowCollectionPadding: false } as const; +/** A rule's keys that are not what it checks, so changing them leaves its sentence as it is. */ +const NOTE_KEYS = ["text", "reason", "evidence", "enabled", "onFail", "label"]; + +/** A built-in rule's id is its check; a custom rule's is its id. */ +export function fileRuleId(rule: PolicyRule): string { + if (rule.check === "model" || rule.check === "text" || rule.check === "code-patterns") return rule.id; + return rule.check; +} + +function refused(message: string): PolicyPatchOutcome { + return { ok: false, message }; +} + +function isCatalogId(id: string): id is CatalogId { + return (CATALOG_IDS as readonly string[]).includes(id); +} + +function unknown(id: string): string { + return `Unknown rule ${id}.`; +} + +function missing(id: string): string { + return `Rule ${id} is not in the file.`; +} + +function present(id: string): string { + return `Rule ${id} is already in the file.`; +} + +function scalarText(node: unknown): string | undefined { + if (!isScalar(node) || typeof node.value !== "string") return undefined; + return node.value; +} + +function ruleIdAt(item: unknown): string | undefined { + if (!isMap(item)) return undefined; + const check = scalarText(item.get("check", true)); + if (check !== undefined && isCatalogId(check)) return check; + return scalarText(item.get("id", true)); +} + +/** `parsedPolicy` refuses a file without a rules list, and no change removes it. */ +function rulesOf(doc: Document): YAMLSeq { + const rules = doc.get("rules", true); + if (!isSeq(rules)) throw new Error("the policy file lost its rules list"); + return rules; +} + +function ruleIndex(rules: YAMLSeq, id: string): number { + return rules.items.findIndex((item) => ruleIdAt(item) === id); +} + +/** A plain object pushed onto `rules` is invisible to `ruleIdAt`, so a later change in the batch would miss it. */ +function appendRule(doc: Document, rule: unknown): void { + rulesOf(doc).add(doc.createNode(rule)); +} + +/** Skip a write that would replace a pair with the value it already holds, so asking again does not rewrite the line. */ +function setPair(doc: Document, path: readonly (string | number)[], value: unknown): void { + const current = doc.getIn(path); + if (current === value || JSON.stringify(current) === JSON.stringify(value)) return; + doc.setIn(path, value); +} + +/** Its sentence stands until `writeSentences` writes it from the limits the batch sets; its reason comes from the batch. */ +function addedCatalogRule(id: CatalogId, repo: string): Record { + return { check: id, text: `${catalogSentence(id, emptyPack(repo))}.`, evidence: ["maintainer-stated"], enabled: true }; +} + +/** + * The built-in rules a batch turns on or gives new limits: only these must hold their required fields, and `resentence` + * lists those whose sentence is rewritten from their limits, which are the rules it turns on without one and gives new + * limits, unless their text is set in the batch (`ownText`). + */ +type Touched = { rules: Set; resentence: Set; ownText: Set }; + +/** A rule given its own `text` anywhere in the batch keeps it. */ +function touch(touched: Touched, id: CatalogId): void { + touched.rules.add(id); + if (!touched.ownText.has(id)) touched.resentence.add(id); +} + +function applyEnabled(doc: Document, repo: string, touched: Touched, id: string, enabled: boolean): string | undefined { + const rules = rulesOf(doc); + const index = ruleIndex(rules, id); + if (index >= 0) { + // A rule without `enabled` is on, so turning it on changes nothing; turned back on, it keeps its own text. + if ((doc.getIn(["rules", index, "enabled"]) !== false) === enabled) return undefined; + if (enabled && isCatalogId(id)) touched.rules.add(id); + setPair(doc, ["rules", index, "enabled"], enabled); + return undefined; + } + if (!isCatalogId(id)) return unknown(id); + if (!enabled) return missing(id); + appendRule(doc, addedCatalogRule(id, repo)); + touch(touched, id); + return undefined; +} + +/** + * `enabled` among the options goes through `applyEnabled`, so a rule turned on here is held to the same checks, and a + * built-in rule not in the file is added the same way. + */ +function applyOptions(doc: Document, repo: string, touched: Touched, id: string, options: Record): string | undefined { + if ("check" in options || "id" in options) return `Rule ${id}'s check and id cannot be changed.`; + const rules = rulesOf(doc); + const { enabled, ...rest } = options; + const problem = typeof enabled === "boolean" ? applyEnabled(doc, repo, touched, id, enabled) : undefined; + if (problem !== undefined) return problem; + if (typeof enabled !== "boolean" && enabled !== undefined) rest.enabled = enabled; + const index = ruleIndex(rules, id); + if (index < 0) return isCatalogId(id) ? missing(id) : unknown(id); + if (isCatalogId(id)) { + const limits = Object.entries(options).filter(([key]) => !NOTE_KEYS.includes(key)); + if (limits.some(([key, value]) => JSON.stringify(doc.getIn(["rules", index, key])) !== JSON.stringify(value))) touch(touched, id); + if ("text" in options) { + touched.ownText.add(id); + touched.resentence.delete(id); + } + } + for (const [key, value] of Object.entries(rest)) setPair(doc, ["rules", index, key], value); + return undefined; +} + +/** A built-in rule added here is held to what turning it on holds it to, and keeps the text it is added with. */ +function applyAdd(doc: Document, touched: Touched, rule: PolicyRule): string | undefined { + const rules = rulesOf(doc); + const id = fileRuleId(rule); + if (ruleIndex(rules, id) >= 0) return present(id); + appendRule(doc, rule); + if (isCatalogId(id)) { + touched.ownText.add(id); + touched.rules.add(id); + } + return undefined; +} + +function applyRemove(doc: Document, id: string): string | undefined { + const rules = rulesOf(doc); + const index = ruleIndex(rules, id); + if (index < 0) return isCatalogId(id) ? missing(id) : unknown(id); + rules.delete(index); + return undefined; +} + +function applyCodeReview(doc: Document, on: boolean): string | undefined { + const block = doc.get("code-review", true); + if (block !== undefined && !isMap(block)) return "`code-review` is not a block, so its on key cannot be set."; + setPair(doc, ["code-review", "on"], on); + return undefined; +} + +function applyChange(doc: Document, repo: string, touched: Touched, change: PolicyPatchChange): string | undefined { + switch (change.kind) { + case "rule": + return applyEnabled(doc, repo, touched, change.id, change.enabled); + case "rule-options": + return applyOptions(doc, repo, touched, change.id, change.options); + case "add-rule": + return applyAdd(doc, touched, change.rule); + case "remove-rule": + return applyRemove(doc, change.id); + case "code-review": + return applyCodeReview(doc, change.on); + } +} + +function parsedPolicy(text: string): Document | string { + const doc = parseDocument(text, { schema: "core", prettyErrors: false }); + const error = doc.errors[0] ?? doc.warnings[0]; + if (error) return `The policy file is not YAML: ${error.message.replace(/\.$/, "")}.`; + if (!isMap(doc.contents)) return "The policy file must be a mapping."; + if (!isSeq(doc.get("rules", true))) return MIGRATION; + return doc; +} + +function requiredField(id: CatalogId): string | undefined { + if (isSizeRule(id)) return "max"; + if (id === "no-forbidden-paths") return "paths"; + return undefined; +} + +/** A size or path rule is not given a made-up limit, nor a turned-on rule a made-up reason; the batch has to set them, or the refusal names both. */ +function missingRequired(doc: Document, touched: ReadonlySet): string | undefined { + const rules = rulesOf(doc); + for (const id of touched) { + const item = rules.items[ruleIndex(rules, id)]; + if (!isMap(item)) continue; + const field = requiredField(id); + if (field !== undefined && !item.has(field)) return `Rule ${id} is missing ${field}.`; + if (!item.has("reason")) return `Rule ${id} is missing reason.`; + } + return undefined; +} + +/** Each rule in `resentence` gets the sentence its limits make, read from `policy`, the checked file's. */ +function writeSentences(doc: Document, repo: string, resentence: ReadonlySet, policy: Record): void { + const rules = rulesOf(doc); + const checks = asRecord(policy.checks); + if (!checks) return; + for (const id of resentence) { + const index = ruleIndex(rules, id); + if (index < 0) continue; + const pack = readCheckPack({ ...emptyPack(repo), checks: { [id]: checks[id] } }, repo); + doc.setIn(["rules", index, "text"], `${catalogSentence(id, pack)}.`); + } +} + +function checkedText(doc: Document, repo: string): { text: string; policy: Record } | string { + const text = doc.toString(WRITTEN); + const checked = checkPolicyFile(text, repo); + if (!checked.ok) return policyFileReason(checked.problems); + return { text, policy: checked.policy }; +} + +/** The file after `changes`, or why the batch cannot be applied. The result passes `checkPolicyFile`. */ +export function patchPolicyFile(text: string, repo: string, changes: readonly PolicyPatchChange[]): PolicyPatchOutcome { + const parsed = parsedPolicy(text); + if (typeof parsed === "string") return refused(parsed); + const original = JSON.stringify(parsed.toJS()); + const touched: Touched = { rules: new Set(), resentence: new Set(), ownText: new Set() }; + for (const change of changes) { + const problem = applyChange(parsed, repo, touched, change); + if (problem !== undefined) return refused(problem); + } + const required = missingRequired(parsed, touched.rules); + if (required !== undefined) return refused(required); + const patched = checkedText(parsed, repo); + if (typeof patched === "string") return refused(patched); + if (touched.resentence.size === 0) return { ok: true, text: patched.text, changed: JSON.stringify(parsed.toJS()) !== original }; + writeSentences(parsed, repo, touched.resentence, patched.policy); + const written = checkedText(parsed, repo); + if (typeof written === "string") return refused(written); + return { ok: true, text: written.text, changed: JSON.stringify(parsed.toJS()) !== original }; +} diff --git a/packages/triage-contracts/src/policy-rules.ts b/packages/triage-contracts/src/policy-rules.ts index 899592851..4359f1adc 100644 --- a/packages/triage-contracts/src/policy-rules.ts +++ b/packages/triage-contracts/src/policy-rules.ts @@ -25,7 +25,7 @@ const SIZE_RULE_IDS = Object.keys(SIZE_RULES) as SizeRuleId[]; const SIMPLE_CHECKS = ["focused", "docs", "no-merge-conflicts", "ci-green", "hidden-content", "no-duplicate", "duplicates-open-pull-request"] as const satisfies readonly CatalogId[]; -const RuleSchema = type({ ...RULE_BASE, check: type.enumerated(...SIZE_RULE_IDS), max: Limit }) +export const policyRuleSchema = type({ ...RULE_BASE, check: type.enumerated(...SIZE_RULE_IDS), max: Limit }) .or({ ...RULE_BASE, check: "'linked-issue'", "tracker?": type.enumerated(...ISSUE_TRACKERS.filter((tracker) => tracker !== "off")) }) .or({ ...RULE_BASE, check: "'no-forbidden-paths'", paths: Lines }) .or({ ...RULE_BASE, check: "'regression-risk'", "hotFileFixes?": Limit, "migrations?": Lines }) @@ -37,14 +37,14 @@ const RuleSchema = type({ ...RULE_BASE, check: type.enumerated(...SIZE_RULE_IDS) .or({ ...OWN_RULE, check: "'code-patterns'", "presets?": type.enumerated(...Object.keys(PATTERN_PRESETS)).array().atLeastLength(1), "patterns?": PatternRuleSchema.array().atLeastLength(1) }) .or({ ...OWN_RULE, check: "'text'", fields: type.enumerated(...TEXT_FIELDS).array().atLeastLength(1), failWhen: type.enumerated(...TEXT_MATCHES), phrases: Lines }); -type PolicyRule = typeof RuleSchema.infer; +type PolicyRule = typeof policyRuleSchema.infer; type OnFailWord = typeof OnFailWord.infer; export const policyRulesSchema = type({ "+": "reject", "extends?": RepoName, "onFail?": OnFailWord, - rules: RuleSchema.array(), + rules: policyRuleSchema.array(), "actions?": { "+": "reject", "on?": type.enumerated(...POLICY_EVENTS).array().atLeastLength(1), "pass?": stepListSchema, "fail?": stepListSchema, "unsure?": stepListSchema }, "labels?": { "+": "reject", "pass?": Label, "fail?": Label, "unsure?": Label }, ...WORKFLOW_PROPS,