From 63de4483fd72250b43ad88eccc8b0f6a91a595f6 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 00:21:08 -0400 Subject: [PATCH 1/9] fix(pr-merge): bind a cost decision to the pull request it names A `**Decision recorded.** Cost:` comment on a pull request covers every head of it. One on its linked issue counts only when it names the pull request as `#`, so a decision recorded on an issue for another change no longer lifts the cost hold of a pull request that links the issue later through a body edit. The author, App, edit and creation-time checks are unchanged. merge-holds.mjs costDecision, babysit cost_decision and ci-status's cost-hold lift apply the same rule, pinned by the parity test. With no closing issue, the JS reader and ci-status now read the one plain `Refs:` issue, as babysit's _linked_issue does; ci-status runs the base commit's merge-holds.mjs refsIssue for it, so its Node setup moves ahead of the cost-hold step. Co-Authored-By: Claude Opus 5.5 --- .../agent-a5e09147a92521857-c3fc6a4d.md | 7 ++ .../pr-merge/enable-auto-merge.test.mjs | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 28 +++++-- .github/scripts/pr-merge/merge-holds.test.mjs | 51 +++++++++++-- .github/workflows/pr-require-checks.yml | 73 ++++++++++++------- .../scripts/babysit_merge_policy.py | 40 +++++++--- .../scripts/tests/test_babysit_merge_cost.py | 39 +++++++++- .../scripts/tests/test_merge_holds_sync.py | 31 +++++++- scripts/ci-hold-cost-decision.test.sh | 70 ++++++++++++++++-- 9 files changed, 274 insertions(+), 67 deletions(-) create mode 100644 .changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md new file mode 100644 index 0000000000..7f7e097568 --- /dev/null +++ b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `cost_decision` binds an owner cost decision to one pull request: a `**Decision recorded.** Cost:` comment on the pull request covers every head of it, and one on its linked issue (closing, else the one `Refs:` issue) counts only when it names the pull request as `#`, so a decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 527e9725fd..ca646f292d 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -85,7 +85,7 @@ const COST_COMMENT = { issue_url: `https://api.github.com/repos/${REPO}/issues/1040`, user: { login: "kyle-sexton", id: 5, type: "User" }, performed_via_github_app: null, - body: "**Decision recorded.** Cost: approve it.", + body: "**Decision recorded.** Cost: approve it for #40.", node_id: "IC_7", created_at: "2026-10-22T12:00:00Z", }; diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 6a725d8d28..db6083867b 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -22,7 +22,8 @@ // key by key. The same approval lifts it. // - A cost path (COST_PATHS): a changed file the gate holds for the owner's // cost decision. An approval does not lift it; only the decision the gate -// reads does (costDecision), linked from the body. +// reads does (costDecision), linked from the body: on the pull request, or +// on its linked issue naming it. // - No verifier pass, when a verdicts file is given (ci-status gives one in // the merge queue only): the head merges on a PASS or PASS-NOTES that // pr-verify.yml uploaded from a completed, successful run on main @@ -427,12 +428,21 @@ export const PULL_OPENED_QUERY = `query($owner: String!, $name: String!, $number }`; export const COMMENT_EDITED_QUERY = "query($id: ID!) { node(id: $id) { ... on IssueComment { lastEditedAt } } }"; +// Whether `text` names pull request `number` as `#`: not part of a +// longer number, and not after a letter, digit, `_`, `/`, `.` or `-`, so +// `owner/repo#` does not count (babysit_merge_policy.names_pull). +export function namesPull(text, number) { + return Number.isSafeInteger(number) && new RegExp(`(?:^|[^A-Za-z0-9_/.-])#${number}(?:[^0-9]|$)`).test(String(text ?? "")); +} + // The first comment link in `body`, outside HTML comments, that records the // owner's cost decision for pull request `number` of `repo`, else null: the // rule of babysit_merge_policy.cost_decision, with the owner a person whose // id is in `humans` (trusted-actors.json), as ci-status's cost-only review -// hold step names one. The comment is on the pull request, or on its one -// closing issue in `repo` and newer than the pull request; by a User, not +// hold step names one. A decision binds to one pull request and covers every +// head of it: the comment is on the pull request, or on its linked issue in +// `repo` (the one closing issue, else with none the one refsIssue names), +// newer than the pull request and naming it (namesPull); by a User, not // through an App; never edited; its first line `**Decision recorded.** Cost:`. // `github(method, path, body)` reads; a read that fails clears nothing. export async function costDecision(github, repo, number, body, humans) { @@ -452,9 +462,11 @@ export async function costDecision(github, repo, number, body, humans) { const opened = Date.parse(pull?.createdAt); const closing = pull?.closingIssuesReferences; const issue = - closing?.totalCount === 1 && closing.nodes?.length === 1 && String(closing.nodes[0]?.repository?.nameWithOwner).toLowerCase() === repo.toLowerCase() - ? closing.nodes[0].number - : null; + closing?.totalCount === 0 + ? refsIssue(body) + : closing?.totalCount === 1 && closing.nodes?.length === 1 && String(closing.nodes[0]?.repository?.nameWithOwner).toLowerCase() === repo.toLowerCase() + ? closing.nodes[0].number + : null; for (const link of links) { const target = Number(link[3]); if (target !== number && target !== issue) { @@ -468,7 +480,7 @@ export async function costDecision(github, repo, number, body, humans) { comment.performed_via_github_app != null || !humans.has(comment.user.id) || !DECISION_LINE.test(String(comment.body ?? "").trimStart()) || - (target !== number && !(Date.parse(comment.created_at) > opened)) + (target !== number && !(Date.parse(comment.created_at) > opened && namesPull(comment.body, number))) ) { continue; } @@ -543,7 +555,7 @@ export function holds(body, items, files, holdPaths, lifters = [], decision = nu const cost = costHold(files, decision); if (cost) { reasons.push( - `${cost}; the owner's unedited comment opening '**Decision recorded.** Cost:' on this pull request or its closing issue, linked from the body, lifts it`, + `${cost}; the owner's unedited comment opening '**Decision recorded.** Cost:' on this pull request, or on its linked issue naming this pull request, linked from the body, lifts it`, ); } if (BODY_HOLD.test(String(body ?? ""))) { diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index fd3d612338..3ebcaddf37 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -9,7 +9,10 @@ // babysit_merge_policy.cost_decision reads and test_babysit_merge_cost.py // pins case by case. ci-status names its owner by the trusted-actor list, as // its cost-only review hold step does, so a review approval lifts a hold path -// but never a cost hold. +// but never a cost hold. A decision binds to one pull request and covers every +// head of it (owner decision Q17, option b): one on the pull request counts as +// it is, one on its linked issue (closing, else `Refs:`) only when it names +// `#`. import assert from "node:assert/strict"; import { Buffer } from "node:buffer"; import { mkdtempSync, writeFileSync } from "node:fs"; @@ -30,7 +33,7 @@ const comment = (changes = {}) => ({ issue_url: "https://api.github.com/repos/owner/repo/issues/40", user: { login: "owner-person", id: 5, type: "User" }, performed_via_github_app: null, - body: "**Decision recorded.** Cost: approve the larger runner.", + body: "**Decision recorded.** Cost: approve the larger runner for #99.", node_id: "IC_7", created_at: "2026-10-22T12:00:00Z", ...changes, @@ -72,12 +75,33 @@ test("an owner decision on the closing issue, linked from the body, clears the c assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited"]); }); -test("one on the pull request itself clears with no time check", async () => { +test("one on the pull request itself clears with no time check and no #number", async () => { const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; - const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: null }); + const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: null, body: "**Decision recorded.** Cost: approve it." }); assert.equal((await decide(own, { row, closing: [] })).link, own); }); +test("one on the linked issue clears only when its text names the pull request", async () => { + for (const text of ["for #99.", "(#99)", "#99", "see #99, then"]) { + assert.equal((await decide(undefined, { row: comment({ body: `**Decision recorded.** Cost: approve ${text}` }) })).link, LINK, text); + } + for (const text of ["for another change.", "for #98.", "for #990.", "for owner/repo#99.", "for x#99.", "for pull 99."]) { + assert.equal((await decide(undefined, { row: comment({ body: `**Decision recorded.** Cost: approve ${text}` }) })).link, null, text); + } +}); + +test("with no closing issue, one on the issue the Refs line names counts the same way", async () => { + const refs = `Refs: #40\n\nCost decision: ${LINK}\n`; + assert.equal((await decide(refs, { closing: [] })).link, LINK); + const unnamed = comment({ body: "**Decision recorded.** Cost: approve the larger runner." }); + assert.equal((await decide(refs, { closing: [], row: unnamed })).link, null); + for (const body of [`Refs: #41\n\n${LINK}`, `Refs: #40\nRefs: #41\n\n${LINK}`, `Refs: #40, #41\n\n${LINK}`, `\`\`\`\nRefs: #40\n\`\`\`\n\n${LINK}`, `\n\n${LINK}`]) { + assert.equal((await decide(body, { closing: [] })).link, null, body); + } + // A closing issue wins over the Refs line, as babysit's _linked_issue reads it. + assert.equal((await decide(refs, { closing: [{ number: 12, repository: { nameWithOwner: REPO } }] })).link, null); +}); + test("a body with no covering link reads nothing", async () => { for (const body of ["Closes #40", LINK.replace("owner/repo", "evil/repo"), `Closes #40\n\n`, `Closes #40\n' 'refs #9306'; do + with_body "$body + +- Cost decision: [Q50]($LINK)" + expect "a body whose Refs lines read '${body//$'\n'/ }' names no linked issue and clears nothing" no no no +done + reset lanes "[$(hold_note cost,trust-gate)]" '[]' expect "a hold with cost and trust-gate clears nothing" no no no "is not cost-only" From 6c3b6ae9aa2c97048e90bdd3c31e47dd2766e822 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:00:12 -0400 Subject: [PATCH 2/9] fix(pr-merge): read cost decisions as the review lanes' hold step does Align costDecision, cost_decision and ci-status's cost-hold lift with ci-workflows' apply-review-hold reader, taking the stricter reading where they differ: - #N rule: a linked-issue decision names the pull request only when `#N` follows none of a letter, digit, `_`, `#`, `&`, `/`, `.` or `-` (the union of both readers' exclusions), so `&#N` and `##N` no longer count. - The issue a plain `Refs:` line names is read to be an issue, not a pull request, in the JS and bash readers too, as Python and the hold step already read it. The read happens only when a body link targets it; a failed read decides nothing. Co-Authored-By: Claude Opus 5.5 --- .../agent-a5e09147a92521857-c3fc6a4d.md | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 44 +++++++++++++------ .github/scripts/pr-merge/merge-holds.test.mjs | 26 +++++++++-- .github/workflows/pr-require-checks.yml | 25 +++++++---- .../scripts/babysit_merge_policy.py | 11 +++-- .../scripts/tests/test_babysit_merge_cost.py | 2 + .../scripts/tests/test_merge_holds_sync.py | 10 +++++ scripts/ci-hold-cost-decision.test.sh | 9 +++- 8 files changed, 98 insertions(+), 31 deletions(-) diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md index 7f7e097568..e6c7583127 100644 --- a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md +++ b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `cost_decision` binds an owner cost decision to one pull request: a `**Decision recorded.** Cost:` comment on the pull request covers every head of it, and one on its linked issue (closing, else the one `Refs:` issue) counts only when it names the pull request as `#`, so a decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later. +- `cost_decision` binds an owner cost decision to one pull request: a `**Decision recorded.** Cost:` comment on the pull request covers every head of it, and one on its linked issue (closing, else the one `Refs:` issue) counts only when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`), so a decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index db6083867b..df844ea9ce 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -429,10 +429,12 @@ export const PULL_OPENED_QUERY = `query($owner: String!, $name: String!, $number export const COMMENT_EDITED_QUERY = "query($id: ID!) { node(id: $id) { ... on IssueComment { lastEditedAt } } }"; // Whether `text` names pull request `number` as `#`: not part of a -// longer number, and not after a letter, digit, `_`, `/`, `.` or `-`, so -// `owner/repo#` does not count (babysit_merge_policy.names_pull). +// longer number, and not after a letter, digit, `_`, `#`, `&`, `/`, `.` or +// `-`, so neither `owner/repo#` nor `&#` counts +// (babysit_merge_policy.names_pull; the review lanes' hold step excludes the +// same characters but `.` and `-`). export function namesPull(text, number) { - return Number.isSafeInteger(number) && new RegExp(`(?:^|[^A-Za-z0-9_/.-])#${number}(?:[^0-9]|$)`).test(String(text ?? "")); + return Number.isSafeInteger(number) && new RegExp(`(?:^|[^A-Za-z0-9_#&/.-])#${number}(?:[^0-9]|$)`).test(String(text ?? "")); } // The first comment link in `body`, outside HTML comments, that records the @@ -441,10 +443,12 @@ export function namesPull(text, number) { // id is in `humans` (trusted-actors.json), as ci-status's cost-only review // hold step names one. A decision binds to one pull request and covers every // head of it: the comment is on the pull request, or on its linked issue in -// `repo` (the one closing issue, else with none the one refsIssue names), -// newer than the pull request and naming it (namesPull); by a User, not -// through an App; never edited; its first line `**Decision recorded.** Cost:`. -// `github(method, path, body)` reads; a read that fails clears nothing. +// `repo` (the one closing issue, else with none the one refsIssue names, read +// to be an issue and not a pull request), newer than the pull request and +// naming it (namesPull); by a User, not through an App; never edited; its +// first line `**Decision recorded.** Cost:`. `github(method, path, body)` +// reads; a failed read of the pull request or the Refs target decides +// nothing, and one of a comment or its edit state skips that comment. export async function costDecision(github, repo, number, body, humans) { const links = [...String(body ?? "").replaceAll(HTML_COMMENT, "").matchAll(DECISION_LINK)].filter( (link) => `${link[1]}/${link[2]}`.toLowerCase() === String(repo).toLowerCase(), @@ -461,17 +465,31 @@ export async function costDecision(github, repo, number, body, humans) { } const opened = Date.parse(pull?.createdAt); const closing = pull?.closingIssuesReferences; - const issue = - closing?.totalCount === 0 - ? refsIssue(body) - : closing?.totalCount === 1 && closing.nodes?.length === 1 && String(closing.nodes[0]?.repository?.nameWithOwner).toLowerCase() === repo.toLowerCase() - ? closing.nodes[0].number - : null; + const refs = closing?.totalCount === 0 ? refsIssue(body) : null; + let issue = + refs ?? + (closing?.totalCount === 1 && closing.nodes?.length === 1 && String(closing.nodes[0]?.repository?.nameWithOwner).toLowerCase() === repo.toLowerCase() + ? closing.nodes[0].number + : null); + let checked = refs === null; for (const link of links) { const target = Number(link[3]); if (target !== number && target !== issue) { continue; } + if (target !== number && !checked) { + // A Refs number may name a pull request; only an issue is consulted. + try { + const read = await github("GET", `/repos/${repo}/issues/${refs}`); + issue = read?.number === refs && read.pull_request == null ? refs : null; + } catch { + return null; + } + checked = true; + if (issue === null) { + continue; + } + } try { const comment = await github("GET", `/repos/${repo}/issues/comments/${link[4]}`); if ( diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 3ebcaddf37..9c9613e65c 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -40,9 +40,9 @@ const comment = (changes = {}) => ({ }); // A GitHub that answers the decision reads: the pull request's creation time -// and closing issues, comment 7, and its edit time. `fail` names a read that -// throws. -function world({ row = comment(), closing = [{ number: 40, repository: { nameWithOwner: REPO } }], node, created = PR_CREATED, fail } = {}) { +// and closing issues, issue 40 (`target`), comment 7, and its edit time. +// `fail` names a read that throws. +function world({ row = comment(), closing = [{ number: 40, repository: { nameWithOwner: REPO } }], node, created = PR_CREATED, target = { number: 40 }, fail } = {}) { const reads = []; const github = async (method, path, body) => { const read = body?.query === PULL_OPENED_QUERY ? "pull" : body?.query === COMMENT_EDITED_QUERY ? "edited" : `${method} ${path}`; @@ -58,6 +58,9 @@ function world({ row = comment(), closing = [{ number: 40, repository: { nameWit assert.deepEqual(body.variables, { id: "IC_7" }); return { data: { node: node === undefined ? { lastEditedAt: null } : node } }; } + if (read === "GET /repos/owner/repo/issues/40") { + return target; + } assert.equal(read, "GET /repos/owner/repo/issues/comments/7"); return row; }; @@ -85,7 +88,7 @@ test("one on the linked issue clears only when its text names the pull request", for (const text of ["for #99.", "(#99)", "#99", "see #99, then"]) { assert.equal((await decide(undefined, { row: comment({ body: `**Decision recorded.** Cost: approve ${text}` }) })).link, LINK, text); } - for (const text of ["for another change.", "for #98.", "for #990.", "for owner/repo#99.", "for x#99.", "for pull 99."]) { + for (const text of ["for another change.", "for #98.", "for #990.", "for owner/repo#99.", "for x#99.", "for pull 99.", "for c", "for ##99."]) { assert.equal((await decide(undefined, { row: comment({ body: `**Decision recorded.** Cost: approve ${text}` }) })).link, null, text); } }); @@ -102,6 +105,21 @@ test("with no closing issue, one on the issue the Refs line names counts the sam assert.equal((await decide(refs, { closing: [{ number: 12, repository: { nameWithOwner: REPO } }] })).link, null); }); +test("a Refs number that names a pull request, or cannot be read, consults no issue", async () => { + const refs = `Refs: #40\n\nCost decision: ${LINK}\n`; + const { link, reads } = await decide(refs, { closing: [], target: { number: 40, pull_request: { url: "x" } } }); + assert.equal(link, null); + assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/40"]); + assert.equal((await decide(refs, { closing: [], target: { number: 41 } })).link, null); + assert.equal((await decide(refs, { closing: [], fail: "GET /repos/owner/repo/issues/40" })).link, null); + // The pull request's own decision still counts, with no read of the Refs target. + const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; + const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", body: "**Decision recorded.** Cost: approve it." }); + const mine = await decide(`Refs: #40\n\n${own}\n`, { closing: [], row, target: { number: 40, pull_request: {} } }); + assert.equal(mine.link, own); + assert.ok(!mine.reads.includes("GET /repos/owner/repo/issues/40")); +}); + test("a body with no covering link reads nothing", async () => { for (const body of ["Closes #40", LINK.replace("owner/repo", "evil/repo"), `Closes #40\n\n`, `Closes #40\n` }]; + } // The read-back pin, posted before an arm. if (method === "POST" && String(body?.body ?? "").startsWith("(?![^\r\n])/; +const SHA = /^[0-9a-f]{40}$/; + +// The head a cost decision made at `at` decided, from the pull request's +// `comments`: the sha of the latest cost hold marker a Bot posted before it, +// else null (babysit_merge_policy.decided_head). +export function decidedHead(comments, at) { + let found = null; + for (const row of Array.isArray(comments) ? comments : []) { + const marker = row?.user?.type === "Bot" ? COST_MARKER.exec(String(row.body ?? "")) : null; + const made = Date.parse(row?.created_at); + if (marker && marker[2].split(",").includes("cost") && made < Date.parse(at) && (found === null || made >= found.made)) { + found = { made, sha: marker[1] }; + } + } + return found?.sha ?? null; +} + +// The head of the branch when a decision on the pull request was made at +// `at`, with no cost hold marker before it: the commit of the latest push +// the repository activity `pushes` timestamp before it, else null +// (babysit_merge_policy.pushed_head). A commit's own dates are its +// author's text, so they place nothing. +export function pushedHead(pushes, at) { + let found = null; + for (const row of Array.isArray(pushes) ? pushes : []) { + const made = Date.parse(row?.timestamp); + if (SHA.test(String(row?.after)) && !/^0+$/.test(row.after) && made < Date.parse(at) && (found === null || made >= found.made)) { + found = { made, sha: row.after }; + } + } + return found?.sha ?? null; +} + +// Whether two compare answers (base...head) show the same net change of the +// pull request: the same files by filename, each with the same status, +// previous_filename and patch. A list of 300 files or more (the compare +// API's cap), a file with no patch, or an answer with no file list is not +// the same (babysit_merge_policy.same_net_change). +export function sameNetChange(decided, current) { + const net = (compare) => { + const files = compare?.files; + if (!Array.isArray(files) || files.length >= 300) return null; + if (!files.every((f) => typeof f?.filename === "string" && typeof f.status === "string" && typeof f.patch === "string")) return null; + return files + .map((f) => JSON.stringify([f.filename, f.status, f.previous_filename ?? null, f.patch])) + .sort() + .join("\n"); + }; + const [then, now] = [net(decided), net(current)]; + return then !== null && then === now; +} + +// Every page of the REST list at `path`. +async function pages(github, path) { + const rows = []; + for (let page = 1; ; page += 1) { + const read = await github("GET", `${path}${path.includes("?") ? "&" : "?"}per_page=100&page=${page}`); + if (!Array.isArray(read)) { + throw new Error(`${path} is not a list`); + } + rows.push(...read); + if (read.length < 100) { + return rows; + } + } +} + +// Whether the decision `comment` covers pull request `number` at `head` +// (owner decisions Q17 c and Q18 d): its decided head (decidedHead; for a +// decision on the pull request with no marker before it, pushedHead on the +// branch `pull.headRefName` of `repo`) is `head`, or every change since it +// is a merge of the base, so the net change against `base` is the same at +// both (sameNetChange). A read that fails carries nothing. +async function carries(github, repo, number, comment, onPull, pull, head, base) { + if (!SHA.test(String(head)) || !SHA.test(String(base))) { + return false; + } + try { + let decided = decidedHead(await pages(github, `/repos/${repo}/issues/${number}/comments`), comment.created_at); + if (decided === null && onPull && pull?.isCrossRepository === false && typeof pull.headRefName === "string" && pull.headRefName) { + decided = pushedHead(await pages(github, `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${pull.headRefName}`)}`), comment.created_at); + } + if (decided === null) { + return false; + } + if (decided === head) { + return true; + } + const [then, now] = await Promise.all([decided, head].map((sha) => github("GET", `/repos/${repo}/compare/${base}...${sha}`))); + return sameNetChange(then, now); + } catch { + return false; + } +} + // The first comment link in `body`, outside HTML comments, that records the -// owner's cost decision for pull request `number` of `repo`, else null: the -// rule of babysit_merge_policy.cost_decision, with the owner a person whose -// id is in `humans` (trusted-actors.json), as ci-status's cost-only review -// hold step names one. A decision binds to one pull request and covers every -// head of it: the comment is on the pull request, or on its linked issue in -// `repo` (the one closing issue, else with none the one refsIssue names, read -// to be an issue and not a pull request), newer than the pull request and -// naming it (namesPull); by a User, not through an App; never edited; its -// first line `**Decision recorded.** Cost:`. `github(method, path, body)` +// owner's cost decision for pull request `number` of `repo` at `head`, else +// null: the rule of babysit_merge_policy.cost_decision, with the owner a +// person whose id is in `humans` (trusted-actors.json), as ci-status's +// cost-only review hold step names one. The comment is on the pull request, +// or on its linked issue in `repo` (the one closing issue, else with none the +// one refsIssue names, read to be an issue and not a pull request), newer +// than the pull request and naming it (namesPull); by a User, not through an +// App; never edited; its first line `**Decision recorded.** Cost:`. It +// covers the head it decided and a later one only when every change since is +// a base merge (carries, against `base`). `github(method, path, body)` // reads; a failed read of the pull request or the Refs target decides -// nothing, and one of a comment or its edit state skips that comment. -export async function costDecision(github, repo, number, body, humans) { +// nothing, and one of a comment, its edit state or its head skips that +// comment. +export async function costDecision(github, repo, number, body, humans, head, base) { const links = [...String(body ?? "").replaceAll(HTML_COMMENT, "").matchAll(DECISION_LINK)].filter( (link) => `${link[1]}/${link[2]}`.toLowerCase() === String(repo).toLowerCase(), ); @@ -503,7 +604,7 @@ export async function costDecision(github, repo, number, body, humans) { continue; } const node = (await github("POST", "/graphql", { query: COMMENT_EDITED_QUERY, variables: { id: String(comment.node_id) } }))?.data?.node; - if (node && "lastEditedAt" in node && node.lastEditedAt === null) { + if (node && "lastEditedAt" in node && node.lastEditedAt === null && (await carries(github, repo, number, comment, target === number, pull, head, base))) { return link[0]; } } catch { @@ -573,7 +674,7 @@ export function holds(body, items, files, holdPaths, lifters = [], decision = nu const cost = costHold(files, decision); if (cost) { reasons.push( - `${cost}; the owner's unedited comment opening '**Decision recorded.** Cost:' on this pull request, or on its linked issue naming this pull request, linked from the body, lifts it`, + `${cost}; the owner's unedited comment opening '**Decision recorded.** Cost:' on this pull request, or on its linked issue naming this pull request, linked from the body, lifts it for the head it decided and a later head that only merges the base`, ); } if (BODY_HOLD.test(String(body ?? ""))) { @@ -715,7 +816,7 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi const body = readFileSync(bodyFile, "utf8"); const repo = String(env.GITHUB_REPOSITORY); const decision = - costHold(files, null) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo) ? await costDecision(github, repo, Number(number), body, humans) : null; + costHold(files, null) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo) ? await costDecision(github, repo, Number(number), body, humans, head, env.BASE_SHA) : null; if (decision) { console.log(`#${number}: ${decision} records the owner's cost decision, which lifts its cost hold.`); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 9c9613e65c..3484acdbc6 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -9,10 +9,12 @@ // babysit_merge_policy.cost_decision reads and test_babysit_merge_cost.py // pins case by case. ci-status names its owner by the trusted-actor list, as // its cost-only review hold step does, so a review approval lifts a hold path -// but never a cost hold. A decision binds to one pull request and covers every -// head of it (owner decision Q17, option b): one on the pull request counts as -// it is, one on its linked issue (closing, else `Refs:`) only when it names -// `#`. +// but never a cost hold. A decision binds to one pull request: one on the pull +// request counts as it is, one on its linked issue (closing, else `Refs:`) only when it names +// `#`. It covers the head it decided, the sha of the latest cost hold +// marker before it (else, for one on the pull request, the last push before +// it), and a later head only when the net change against the base is the same +// at both (owner decisions Q17 c and Q18 d on #9436). import assert from "node:assert/strict"; import { Buffer } from "node:buffer"; import { mkdtempSync, writeFileSync } from "node:fs"; @@ -20,7 +22,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, linkHold, main } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, decidedHead, linkHold, main, pushedHead, sameNetChange } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -28,6 +30,17 @@ const PR_CREATED = "2026-10-22T10:00:00Z"; const HUMANS = new Set([5]); const HEAD = "0123456789abcdef0123456789abcdef01234567"; const BASE = "89abcdef0123456789abcdef0123456789abcdef"; +const OLD = "fedcba9876543210fedcba9876543210fedcba98"; +const LANE = { login: "github-actions[bot]", id: 41898282, type: "Bot" }; +// A review lane's cost hold comment on the pull request at `sha`. +const marker = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", user = LANE } = {}) => ({ + user, + created_at: at, + body: `\nHeld for a person.\n\nclaude-lane: claude-review`, +}); +// A compare answer: the pull request's files against the base. +const TF = { filename: "infra/main.tf", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }; +const net = (...files) => ({ files }); const comment = (changes = {}) => ({ issue_url: "https://api.github.com/repos/owner/repo/issues/40", @@ -40,19 +53,63 @@ const comment = (changes = {}) => ({ }); // A GitHub that answers the decision reads: the pull request's creation time -// and closing issues, issue 40 (`target`), comment 7, and its edit time. +// and closing issues, issue 40 (`target`), comment 7, and its edit time; then +// the head reads: the pull request's `comments` (a cost hold marker at HEAD +// by default), its branch's `pushes` and a `compares` answer per head. // `fail` names a read that throws. -function world({ row = comment(), closing = [{ number: 40, repository: { nameWithOwner: REPO } }], node, created = PR_CREATED, target = { number: 40 }, fail } = {}) { +function world({ + row = comment(), + closing = [{ number: 40, repository: { nameWithOwner: REPO } }], + node, + created = PR_CREATED, + target = { number: 40 }, + fail, + comments = [marker(HEAD)], + pushes = [], + compares = {}, + cross = false, +} = {}) { const reads = []; const github = async (method, path, body) => { - const read = body?.query === PULL_OPENED_QUERY ? "pull" : body?.query === COMMENT_EDITED_QUERY ? "edited" : `${method} ${path}`; + const compare = /^\/repos\/owner\/repo\/compare\/([0-9a-f]{40})\.\.\.([0-9a-f]{40})$/.exec(path); + const read = + body?.query === PULL_OPENED_QUERY + ? "pull" + : body?.query === COMMENT_EDITED_QUERY + ? "edited" + : path === "/repos/owner/repo/issues/99/comments?per_page=100&page=1" + ? "comments" + : path === "/repos/owner/repo/activity?ref=refs%2Fheads%2Ffix%2Fcost&per_page=100&page=1" + ? "pushes" + : compare + ? `compare ${compare[2]}` + : `${method} ${path}`; reads.push(read); if (read === fail) { throw new Error(`${read} failed`); } + if (read === "comments") { + return comments; + } + if (read === "pushes") { + return pushes; + } + if (compare) { + assert.equal(compare[1], BASE); + if (!(compare[2] in compares)) { + throw new Error("no such commit"); + } + return compares[compare[2]]; + } if (read === "pull") { assert.deepEqual(body.variables, { owner: "owner", name: "repo", number: 99 }); - return { data: { repository: { pullRequest: { createdAt: created, closingIssuesReferences: { totalCount: closing.length, nodes: closing } } } } }; + return { + data: { + repository: { + pullRequest: { createdAt: created, headRefName: "fix/cost", isCrossRepository: cross, closingIssuesReferences: { totalCount: closing.length, nodes: closing } }, + }, + }, + }; } if (read === "edited") { assert.deepEqual(body.variables, { id: "IC_7" }); @@ -69,19 +126,21 @@ function world({ row = comment(), closing = [{ number: 40, repository: { nameWit const decide = (body = `Closes #40\n\nCost decision: ${LINK}\n`, options = {}) => { const { github, reads } = world(options); - return costDecision(github, REPO, 99, body, HUMANS).then((link) => ({ link, reads })); + return costDecision(github, REPO, 99, body, HUMANS, options.head ?? HEAD, BASE).then((link) => ({ link, reads })); }; test("an owner decision on the closing issue, linked from the body, clears the cost hold", async () => { const { link, reads } = await decide(); assert.equal(link, LINK); - assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited"]); + assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited", "comments"]); }); -test("one on the pull request itself clears with no time check and no #number", async () => { +test("one on the pull request itself clears with no check against the pull request's time and no #number", async () => { const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; - const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: null, body: "**Decision recorded.** Cost: approve it." }); - assert.equal((await decide(own, { row, closing: [] })).link, own); + const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: "2026-10-21T09:00:00Z", body: "**Decision recorded.** Cost: approve it." }); + assert.equal((await decide(own, { row, closing: [], comments: [marker(HEAD, "2026-10-21T08:00:00Z")] })).link, own); + // With no time it decides no head, so it carries to none. + assert.equal((await decide(own, { row: { ...row, created_at: null }, closing: [] })).link, null); }); test("one on the linked issue clears only when its text names the pull request", async () => { @@ -302,19 +361,103 @@ test("ci-status holds a cost path that an approval does not lift, and the owner' assert.equal((await ci(["infra/main.tf"], { body: `Closes #40\n\n${LINK}`, options: { node: { lastEditedAt: "2026-10-22T13:00:00Z" } } })).code, 1); }); -test("ci-status lets the owner's decision on the pull request cover every head pushed after it", async () => { +test("ci-status lifts a cost hold for the head the decision decided, and re-holds a later head with a change of its own", async () => { const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", body: "**Decision recorded.** Cost: approve it." }); - for (const head of [HEAD, BASE.replace(/^8/, "7")]) { - const decided = await ci(["infra/main.tf"], { body: `Closes #40\n\n${own}`, head, options: { row } }); - assert.equal(decided.code, 0, `${head}: ${decided.out}`); - } + const body = `Closes #40\n\n${own}`; + assert.equal((await ci(["infra/main.tf"], { body, options: { row } })).code, 0); + const later = await ci(["infra/main.tf"], { body, options: { row, comments: [marker(OLD)], compares: { [OLD]: net(TF), [HEAD]: net(TF, { filename: "infra/b.tf", status: "added", patch: "@@ -0,0 +1 @@\n+c" }) } } }); + assert.equal(later.code, 1, later.out); + assert.match(later.out, /a cost path/); // The same decision on the closing issue, which names no pull request, holds. const unnamed = await ci(["infra/main.tf"], { body: `Closes #40\n\n${LINK}`, options: { row: comment({ body: "**Decision recorded.** Cost: approve it." }) } }); assert.equal(unnamed.code, 1); assert.match(unnamed.out, /on its linked issue naming this pull request/); }); +// The carry rule (#9436 Q18 d): the decided head is OLD, the current HEAD. +const later = (compares, options = {}) => decide(undefined, { comments: [marker(OLD)], compares, ...options }); + +test("a base merge after the decision carries it, and a pull request commit after it re-holds", async () => { + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) })).link, LINK); + const commit = await later({ [OLD]: net(TF), [HEAD]: net(TF, { filename: "scripts/run.sh", status: "modified", patch: "@@ -1 +1 @@\n-x\n+y" }) }); + assert.equal(commit.link, null); + assert.deepEqual(commit.reads.slice(-3), ["comments", `compare ${OLD}`, `compare ${HEAD}`]); +}); + +test("a rebase onto a newer base with the same net change carries, whatever order the files list in", async () => { + const doc = { filename: "docs/a.md", status: "added", patch: "@@ -0,0 +1 @@\n+a" }; + assert.equal((await later({ [OLD]: net(TF, doc), [HEAD]: net(doc, TF) })).link, LINK); + // A rebase that drops one of its files is a change of its own. + assert.equal((await later({ [OLD]: net(TF, doc), [HEAD]: net(TF) })).link, null); +}); + +test("a conflict resolution, a rename or a status change that alters the net change re-holds", async () => { + for (const [name, changed] of Object.entries({ + "an edited patch": { ...TF, patch: "@@ -1 +1 @@\n-a\n+c" }, + "a rename": { ...TF, status: "renamed", previous_filename: "infra/old.tf" }, + "a status": { ...TF, status: "added" }, + })) { + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(changed) })).link, null, name); + } +}); + +test("a compare of 300 files or more, a file with no patch, or a failed compare read carries nothing", async () => { + const many = (n) => net(...Array.from({ length: n }, (_, i) => ({ filename: `f${i}`, status: "added", patch: "+x" }))); + assert.equal((await later({ [OLD]: many(299), [HEAD]: many(299) })).link, LINK); + assert.equal((await later({ [OLD]: many(300), [HEAD]: many(300) })).link, null); + const binary = { filename: "a.png", status: "modified" }; + assert.equal((await later({ [OLD]: net(binary), [HEAD]: net(binary) })).link, null); + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { fail: `compare ${HEAD}` })).link, null); + assert.equal((await later({ [OLD]: net(TF), [HEAD]: { status: "diverged" } })).link, null); + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { fail: "comments" })).link, null); +}); + +test("the decided head is the latest bot cost hold marker made before the decision", async () => { + const same = { [OLD]: net(TF), [HEAD]: net(TF, { filename: "b.tf", status: "added", patch: "+b" }) }; + // An older marker at the current head does not outweigh the latest one before the decision. + assert.equal((await decide(undefined, { comments: [marker(HEAD, "2026-10-22T10:30:00Z"), marker(OLD, "2026-10-22T11:30:00Z")], compares: same })).link, null); + assert.equal((await decide(undefined, { comments: [marker(OLD, "2026-10-22T10:30:00Z"), marker(HEAD, "2026-10-22T11:30:00Z")], compares: same })).link, LINK); + // A marker after the decision, by a person, or without cost decides nothing. + for (const [name, extra] of Object.entries({ + after: marker(HEAD, "2026-10-22T12:30:00Z"), + "a person": marker(HEAD, "2026-10-22T11:30:00Z", { user: { login: "owner-person", id: 5, type: "User" } }), + "not cost": marker(HEAD, "2026-10-22T11:30:00Z", { criteria: "trust-gate" }), + })) { + assert.equal((await decide(undefined, { comments: [marker(OLD), extra], compares: same })).link, null, name); + } + assert.equal((await decide(undefined, { comments: [marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z", { criteria: "cost,trust-gate" })], compares: same })).link, LINK); + assert.equal(decidedHead([marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z")], "2026-10-22T11:30:00Z"), OLD); + assert.equal(decidedHead([], "2026-10-22T12:00:00Z"), null); +}); + +test("with no marker before it, a decision on the pull request covers the head of the last push before it", async () => { + const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; + const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", body: "**Decision recorded.** Cost: approve it." }); + const push = (after, timestamp) => ({ activity_type: "push", after, timestamp }); + const changed = { [OLD]: net(TF), [HEAD]: net(TF, { filename: "b.tf", status: "added", patch: "+b" }) }; + const pushed = (pushes, options = {}) => decide(own, { row, closing: [], comments: [], pushes, compares: changed, ...options }); + assert.equal((await pushed([push(HEAD, "2026-10-22T11:59:00Z"), push(OLD, "2026-10-22T11:00:00Z")])).link, own); + assert.equal((await pushed([push(HEAD, "2026-10-22T12:01:00Z"), push(OLD, "2026-10-22T11:00:00Z")])).link, null); + assert.equal((await pushed([push(HEAD, "2026-10-22T12:01:00Z")])).link, null); + assert.equal((await pushed([push(HEAD, "2026-10-22T11:59:00Z")], { fail: "pushes" })).link, null); + const fork = await pushed([push(HEAD, "2026-10-22T11:59:00Z")], { cross: true }); + assert.equal(fork.link, null); + assert.ok(!fork.reads.includes("pushes")); + // An issue decision with no marker carries nothing and reads no pushes. + const issue = await decide(undefined, { comments: [], pushes: [push(HEAD, "2026-10-22T11:59:00Z")] }); + assert.equal(issue.link, null); + assert.ok(!issue.reads.includes("pushes")); + assert.equal(pushedHead([push("0".repeat(40), "2026-10-22T11:59:00Z"), push(OLD, "2026-10-22T11:00:00Z")], "2026-10-22T12:00:00Z"), OLD); +}); + +test("sameNetChange compares filename, status, previous_filename and patch", () => { + assert.equal(sameNetChange(net(TF), net({ ...TF })), true); + assert.equal(sameNetChange(net(), net()), true); + assert.equal(sameNetChange(net(TF), net({ ...TF, previous_filename: "x.tf" })), false); + assert.equal(sameNetChange(null, net(TF)), false); +}); + // The live link a pass binds, by the gate's rule (README "Eligibility"): one // closing issue in this repository, or, with none, one plain `Refs: #` // issue; a pass binds only the issue it judged, linked the same way diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 872d736839..a9b4cd6070 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3558,11 +3558,16 @@ jobs: # - The pull request body, outside HTML comments, links a decision comment # as babysit_merge_policy.py's cost_decision reads one: by a User on the # trusted-actor list, not through an App, never edited, first line - # `**Decision recorded.** Cost:`. A decision binds to one pull request - # and covers every head of it: one on the pull request itself counts as - # it is; one on its linked issue counts only when written after the + # `**Decision recorded.** Cost:`. One on the pull request itself counts + # as it is; one on its linked issue counts only when written after the # pull request and naming it as `#` (merge-holds.mjs - # namesPull). The linked issue is the one issue in this repository that + # namesPull). It covers the head it decided (the latest review-lane + # cost hold marker before it, else, for one on the pull request, the + # last push before it) and a later head only when the pull request's + # net change against the base is the same at both, so every change + # since is a base merge (the base's merge-holds.mjs decidedHead, + # pushedHead and sameNetChange; owner decisions Q17 c and Q18 d on + # #9436). The linked issue is the one issue in this repository that # GitHub records it as closing, else, when it closes none, the one issue # its plain `Refs:` lines name (the base's merge-holds.mjs refsIssue), # read to be an issue and not a pull request. A second closing issue @@ -3658,6 +3663,38 @@ jobs: issues=" $refs " fi opened=$(jq -r '.created_at | fromdateiso8601' <<<"$pr") || keep "the pull request has no creation time" + comments=$(read_api "repos/$repo/issues/$PR_NUMBER/comments?per_page=100" --paginate) || keep "the comments are unreadable" + jq -sc 'add // []' <<<"$comments" >"$RUNNER_TEMP/cost-hold-comments.json" || keep "the comments do not parse" + # holds_js ...: one function of the base's + # merge-holds.mjs over the files. The module goes last: as the first + # argument it would run as the command line. + holds_js() { + # shellcheck disable=SC2016 # JavaScript, not shell + node --input-type=module -e 'import { readFileSync } from "node:fs"; + import { pathToFileURL } from "node:url"; + const [name, ...files] = process.argv.slice(1, -1); + const run = (await import(pathToFileURL(process.argv.at(-1)).href))[name]; + console.log(run(...files.map((file) => JSON.parse(readFileSync(file, "utf8")))) ?? "");' \ + "$@" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs" + } + # carries : whether the decision comment on + # covers HEAD_SHA. Any read that fails carries nothing. + carries() { + local at="$RUNNER_TEMP/cost-hold-at.json" decided pushes + jq '.created_at' <<<"$1" >"$at" || return 1 + decided=$(holds_js decidedHead "$RUNNER_TEMP/cost-hold-comments.json" "$at") || return 1 + if [ -z "$decided" ] && [ "$2" = "$PR_NUMBER" ] && + jq -e --arg repo "$repo" '(.head.repo.full_name // "" | ascii_downcase) == ($repo | ascii_downcase)' <<<"$pr" >/dev/null; then + pushes=$(read_api "repos/$repo/activity?ref=refs%2Fheads%2F$ref&per_page=100" --paginate) || return 1 + jq -sc 'add // []' <<<"$pushes" >"$RUNNER_TEMP/cost-hold-pushes.json" || return 1 + decided=$(holds_js pushedHead "$RUNNER_TEMP/cost-hold-pushes.json" "$at") || return 1 + fi + [[ $decided =~ ^[0-9a-f]{40}$ ]] || return 1 + [ "$decided" != "$HEAD_SHA" ] || return 0 + read_api "repos/$repo/compare/$BASE_SHA...$decided" >"$RUNNER_TEMP/cost-hold-then.json" || return 1 + read_api "repos/$repo/compare/$BASE_SHA...$HEAD_SHA" >"$RUNNER_TEMP/cost-hold-now.json" || return 1 + [ "$(holds_js sameNetChange "$RUNNER_TEMP/cost-hold-then.json" "$RUNNER_TEMP/cost-hold-now.json")" = true ] + } decision='' decision_id='' refs_read= while IFS=$'\t' read -r link owner name number id; do [ -n "$link" ] || continue @@ -3682,10 +3719,11 @@ jobs: # shellcheck disable=SC2016 # GraphQL variables, not shell edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$node") || continue jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$edited" >/dev/null 2>&1 || continue + carries "$comment" "$number" || continue decision=$link decision_id=$id break done <<<"$links" - [ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering it" + [ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering this head" # Every hold a review lane recorded in the window, as {at, criteria}. holds='[]' vetted='' while read -r file check; do @@ -3714,7 +3752,6 @@ jobs: done < <(jq -nr --argjson since "$since" '[inputs.workflow_runs[] | select((.updated_at | fromdateiso8601) > $since) | "\(.id) \(.head_sha)"] | unique[]' <<<"$runs") done <<<'pr-review-hosted.yml pr-review / claude-review-status pr-review-security-hosted.yml pr-review-security / security-review' - comments=$(read_api "repos/$repo/issues/$PR_NUMBER/comments?per_page=100" --paginate) || keep "the comments are unreadable" # shellcheck disable=SC2016 # GraphQL variables, not shell deleted=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){timelineItems(itemTypes:[COMMENT_DELETED_EVENT],last:100){filteredCount nodes{... on CommentDeletedEvent{createdAt}}}}}}' \ -F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the deleted comments are unreadable" 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 41378f8d27..8691729fcf 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 @@ -1998,6 +1998,145 @@ def _after(value: Any, opened: datetime | None) -> bool: return made is not None and opened is not None and made > opened +# A review lane's cost hold marker, as ci-workflows apply-review-hold writes +# it: the head it held and its criteria, cost among them. +COST_MARKER = re.compile( + r"(?:^|\n)(?![^\r\n])" +) +COMMIT_SHA = re.compile(r"[0-9a-f]{40}") + + +def decided_head(comments: Any, at: Any) -> str | None: + """The head a cost decision made at `at` decided, from the pull + request's `comments`: the sha of the latest cost hold marker a Bot posted + before it, else None (merge-holds.mjs `decidedHead`).""" + when = parse_timestamp(at) + found: tuple[datetime, str] | None = None + for row in json_array(comments): + row = json_object(row) + made = parse_timestamp(row.get("created_at")) + marker = ( + COST_MARKER.search(str(row.get("body") or "")) + if json_object(row.get("user")).get("type") == "Bot" + else None + ) + if ( + marker + and "cost" in marker[2].split(",") + and made is not None + and when is not None + and made < when + and (found is None or made >= found[0]) + ): + found = (made, marker[1]) + return found[1] if found else None + + +def pushed_head(pushes: Any, at: Any) -> str | None: + """The head of the branch when a decision on the pull request was made at + `at`, with no cost hold marker before it: the commit of the latest push + the repository activity `pushes` timestamp before it, else None + (merge-holds.mjs `pushedHead`). A commit's own dates are its author's + text, so they place nothing.""" + when = parse_timestamp(at) + found: tuple[datetime, str] | None = None + for row in json_array(pushes): + row = json_object(row) + after = str(row.get("after") or "") + made = parse_timestamp(row.get("timestamp")) + if ( + COMMIT_SHA.fullmatch(after) + and after.strip("0") + and made is not None + and when is not None + and made < when + and (found is None or made >= found[0]) + ): + found = (made, after) + return found[1] if found else None + + +def same_net_change(decided: Any, current: Any) -> bool: + """Whether two compare answers (base...head) show the same net change of + the pull request: the same files by filename, each with the same status, + previous_filename and patch. A list of 300 files or more (the compare + API's cap), a file with no patch, or an answer with no file list is not + the same (merge-holds.mjs `sameNetChange`).""" + + def net(compare: Any) -> list[str] | None: + files = json_object(compare).get("files") + if not is_json_array(files) or len(files) >= 300: + return None + rows = [] + for f in files: + if not is_json_object(f) or not all( + isinstance(f.get(k), str) for k in ("filename", "status", "patch") + ): + return None + rows.append( + json.dumps( + [f["filename"], f["status"], f.get("previous_filename"), f["patch"]] + ) + ) + return sorted(rows) + + then = net(decided) + return then is not None and then == net(current) + + +def _carries( + repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any], on_pull: bool +) -> bool: + """Whether the decision `comment` covers the pull request at its head + (owner decisions Q17 c and Q18 d): its decided head (`decided_head`; for + a decision on the pull request with no marker before it, `pushed_head` on + its branch) is the head, or every change since it is a merge of the base, + so the net change against the base is the same at both + (`same_net_change`). A read that fails carries nothing.""" + head, base = json_object(pr.get("head")), json_object(pr.get("base")) + sha, base_sha = str(head.get("sha") or ""), str(base.get("sha") or "") + if not COMMIT_SHA.fullmatch(sha) or not COMMIT_SHA.fullmatch(base_sha): + return False + owner, name = _split_repo(repo) + at = comment.get("created_at") + try: + decided = decided_head( + _paginated( + f"repos/{owner}/{name}/issues/{pr.get('number')}/comments?per_page=100", + "cost hold markers", + ), + at, + ) + branch = head.get("ref") + same_repo = ( + str(json_object(head.get("repo")).get("full_name")).casefold() + == repo.casefold() + ) + if decided is None and on_pull and same_repo and isinstance(branch, str) and branch: + ref = quote(f"refs/heads/{branch}", safe="") + decided = pushed_head( + _paginated( + f"repos/{owner}/{name}/activity?ref={ref}&per_page=100", + "head pushes", + ), + at, + ) + except PolicyReadError: + return False + if decided is None: + return False + if decided == sha: + return True + reads = [ + _get(f"repos/{owner}/{name}/compare/{base_sha}...{each}") + for each in (decided, sha) + ] + if any(status != 200 for status, _ in reads): + return False + return same_net_change(reads[0][1], reads[1][1]) + + def cost_decision( repo: str, pr: Mapping[str, Any], @@ -2012,8 +2151,9 @@ def cost_decision( `issue` (its linked issue, `_linked_issue`) in `repo`, by a User with admin permission who is not the agent identity, not posted through an App, never edited, and whose first line starts `**Decision recorded.** - Cost:`. A decision binds to one pull request and covers every head of it - (owner decision Q17, option b): one on the pull request counts as it is; + Cost:`. It covers the head it decided and a later head only when every + change since is a base merge (`_carries`, owner decisions Q17 c and Q18 d + on #9436). One on the pull request counts as it is; one on the issue counts only when its text names this pull request (`names_pull`) and it was created after the pull request was, so a decision recorded on the issue for another change does not cover a pull @@ -2064,6 +2204,7 @@ def cost_decision( is_json_object(node) and "lastEditedAt" in node and node["lastEditedAt"] is None + and _carries(repo, pr, comment, int(link[3]) == pr.get("number")) ): return link[0] return None diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py index 6efe4875f3..24221db556 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py @@ -1,7 +1,8 @@ """`cost_decision`: only an owner `**Decision recorded.**` comment on the pull request, or on its linked issue naming it as `#`, linked from -the body, clears a cost hold (owner decision Q17, option b: a decision binds -to one pull request and covers every head of it).""" +the body, clears a cost hold, for the head it decided and a later head whose +every change since is a base merge (owner decisions Q17 c and Q18 d on +#9436).""" from __future__ import annotations @@ -19,6 +20,31 @@ LINK = "https://github.com/owner/repo/issues/40#issuecomment-7" IDENTITY = "agent-lanes[bot]" PR_CREATED = "2026-10-22T10:00:00Z" +HEAD = "0123456789abcdef0123456789abcdef01234567" +BASE = "89abcdef0123456789abcdef0123456789abcdef" +OLD = "fedcba9876543210fedcba9876543210fedcba98" +LANE = {"login": "github-actions[bot]", "type": "Bot"} +PULL = { + "number": 99, + "created_at": PR_CREATED, + "head": {"sha": HEAD, "ref": "fix/cost", "repo": {"full_name": REPO}}, + "base": {"sha": BASE}, +} +TF = {"filename": "infra/main.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} + + +def _marker( + sha: str, + at: str = "2026-10-22T11:00:00Z", + criteria: str = "cost", + user: dict[str, Any] = LANE, +) -> dict[str, Any]: + body = f"\nHeld." + return {"user": user, "created_at": at, "body": body} + + +def _net(*files: dict[str, Any]) -> dict[str, Any]: + return {"files": list(files)} def _comment(**changes: Any) -> dict[str, Any]: @@ -44,23 +70,45 @@ def decide( issue: int | None = 40, node: Any = ..., created: Any = None, + markers: list[dict[str, Any]] | None = None, + pushes: list[dict[str, Any]] | None = None, + compares: dict[str, Any] | None = None, + unreadable: str = "", + pull: dict[str, Any] | None = None, ) -> str | None: created = PR_CREATED if created is None else created self.reads: list[str] = [] + compares = compares or {} - def get(endpoint: str) -> tuple[int, Any]: + def get(endpoint: str) -> tuple[int | None, Any]: self.reads.append(endpoint) + if "/compare/" in endpoint: + sha = endpoint.rsplit("...", 1)[1] + if sha in compares and unreadable != f"compare {sha}": + return 200, compares[sha] + return 404, None return status, comment if comment is not None else _comment() + def paginated(endpoint: str, label: str) -> list[dict[str, Any]]: + self.reads.append(endpoint) + if label == unreadable: + raise policy.PolicyReadError(label) + if "/activity?" in endpoint: + self.assertIn("ref=refs%2Fheads%2Ffix%2Fcost&", endpoint) + return pushes or [] + self.assertEqual(endpoint, "repos/owner/repo/issues/99/comments?per_page=100") + return [_marker(HEAD)] if markers is None else markers + answer = {"lastEditedAt": edited} if node is ... else node graphql = mock.Mock(return_value={"node": answer}) with ( mock.patch.object(policy, "_get", side_effect=get), + mock.patch.object(policy, "_paginated", side_effect=paginated), mock.patch.object(policy, "_graphql", graphql), ): return policy.cost_decision( REPO, - {"number": 99, "body": body, "created_at": created}, + (pull or PULL) | {"body": body, "created_at": created}, issue, permission=lambda login: permission, agent_identity=IDENTITY, @@ -69,7 +117,13 @@ def get(endpoint: str) -> tuple[int, Any]: def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: self.assertEqual(self.decide(), LINK) - self.assertEqual(self.reads, ["repos/owner/repo/issues/comments/7"]) + self.assertEqual( + self.reads, + [ + "repos/owner/repo/issues/comments/7", + "repos/owner/repo/issues/99/comments?per_page=100", + ], + ) def test_a_decision_on_the_pull_request_itself_clears(self) -> None: link = "https://github.com/owner/repo/pull/99#issuecomment-7" @@ -108,9 +162,13 @@ def test_a_decision_on_the_pull_request_needs_no_time_check(self) -> None: link = "https://github.com/owner/repo/pull/99#issuecomment-7" comment = _comment( issue_url="https://api.github.com/repos/owner/repo/issues/99", - created_at=None, + created_at="2026-10-21T09:00:00Z", ) - self.assertEqual(self.decide(body=link, comment=comment), link) + markers = [_marker(HEAD, "2026-10-21T08:00:00Z")] + self.assertEqual(self.decide(body=link, comment=comment, markers=markers), link) + # With no time it decides no head, so it carries to none. + timeless = comment | {"created_at": None} + self.assertIsNone(self.decide(body=link, comment=timeless)) def test_a_decision_on_another_issue_does_not_cover_it(self) -> None: other = "https://github.com/owner/repo/issues/12#issuecomment-7" @@ -183,6 +241,114 @@ def test_each_failing_condition_holds(self) -> None: with self.subTest(name): self.assertIsNone(self.decide(**change)) + # The carry rule (#9436 Q18 d): the decided head is OLD, the head HEAD. + def later(self, compares: dict[str, Any], **change: Any) -> str | None: + return self.decide(markers=[_marker(OLD)], compares=compares, **change) + + def test_a_base_merge_after_the_decision_carries(self) -> None: + self.assertEqual(self.later({OLD: _net(TF), HEAD: _net(TF)}), LINK) + # The same decision at a head whose net change moved does not. + self.assertIsNone(self.later({OLD: _net(), HEAD: _net(TF)})) + + def test_a_pull_request_commit_after_the_decision_re_holds(self) -> None: + script = {"filename": "scripts/run.sh", "status": "modified", "patch": "+y"} + self.assertIsNone(self.later({OLD: _net(TF), HEAD: _net(TF, script)})) + self.assertEqual( + self.reads[-2:], + [ + f"repos/owner/repo/compare/{BASE}...{OLD}", + f"repos/owner/repo/compare/{BASE}...{HEAD}", + ], + ) + + def test_a_rebase_with_the_same_net_change_carries_in_any_file_order( + self, + ) -> None: + doc = {"filename": "docs/a.md", "status": "added", "patch": "+a"} + self.assertEqual(self.later({OLD: _net(TF, doc), HEAD: _net(doc, TF)}), LINK) + # A rebase that drops one of its files is a change of its own. + self.assertIsNone(self.later({OLD: _net(TF, doc), HEAD: _net(TF)})) + + def test_a_conflict_resolution_that_changes_the_net_change_re_holds( + self, + ) -> None: + for name, changed in { + "an edited patch": TF | {"patch": "@@ -1 +1 @@\n-a\n+c"}, + "a rename": TF | {"status": "renamed", "previous_filename": "old.tf"}, + "a status": TF | {"status": "added"}, + }.items(): + with self.subTest(name): + self.assertIsNone(self.later({OLD: _net(TF), HEAD: _net(changed)})) + + def test_a_compare_of_300_files_or_a_file_with_no_patch_does_not_carry( + self, + ) -> None: + def many(n: int) -> dict[str, Any]: + return _net( + *({"filename": f"f{i}", "status": "added", "patch": "+x"} for i in range(n)) + ) + + self.assertEqual(self.later({OLD: many(299), HEAD: many(299)}), LINK) + self.assertIsNone(self.later({OLD: many(300), HEAD: many(300)})) + binary = {"filename": "a.png", "status": "modified"} + self.assertIsNone(self.later({OLD: _net(binary), HEAD: _net(binary)})) + + def test_a_failed_read_does_not_carry(self) -> None: + same = {OLD: _net(TF), HEAD: _net(TF)} + self.assertIsNone(self.later(same, unreadable=f"compare {HEAD}")) + self.assertIsNone(self.later(same, unreadable="cost hold markers")) + self.assertIsNone(self.later({OLD: _net(TF), HEAD: {"status": "diverged"}})) + + def test_the_decided_head_is_the_latest_bot_marker_before_the_decision( + self, + ) -> None: + added = {"filename": "b.tf", "status": "added", "patch": "+b"} + changed = {OLD: _net(TF), HEAD: _net(TF, added)} + older_head = [_marker(HEAD, "2026-10-22T10:30:00Z"), _marker(OLD, "2026-10-22T11:30:00Z")] + self.assertIsNone(self.decide(markers=older_head, compares=changed)) + newer_head = [_marker(OLD, "2026-10-22T10:30:00Z"), _marker(HEAD, "2026-10-22T11:30:00Z")] + self.assertEqual(self.decide(markers=newer_head, compares=changed), LINK) + for name, extra in { + "after": _marker(HEAD, "2026-10-22T12:30:00Z"), + "a person": _marker(HEAD, "2026-10-22T11:30:00Z", user={"type": "User"}), + "not cost": _marker(HEAD, "2026-10-22T11:30:00Z", criteria="trust-gate"), + }.items(): + with self.subTest(name): + self.assertIsNone( + self.decide(markers=[_marker(OLD), extra], compares=changed) + ) + both = _marker(HEAD, "2026-10-22T11:30:00Z", criteria="cost,trust-gate") + self.assertEqual(self.decide(markers=[_marker(OLD), both], compares=changed), LINK) + + def test_with_no_marker_a_pull_request_decision_covers_the_last_push_before_it( + self, + ) -> None: + link = "https://github.com/owner/repo/pull/99#issuecomment-7" + comment = _comment(issue_url="https://api.github.com/repos/owner/repo/issues/99") + added = {"filename": "b.tf", "status": "added", "patch": "+b"} + changed = {OLD: _net(TF), HEAD: _net(TF, added)} + + def push(after: str, at: str) -> dict[str, Any]: + return {"activity_type": "push", "after": after, "timestamp": at} + + def pushed(pushes: list[dict[str, Any]], **change: Any) -> str | None: + return self.decide( + body=link, comment=comment, markers=[], pushes=pushes, compares=changed, **change + ) + + before = push(HEAD, "2026-10-22T11:59:00Z") + self.assertEqual(pushed([before, push(OLD, "2026-10-22T11:00:00Z")]), link) + after = push(HEAD, "2026-10-22T12:01:00Z") + self.assertIsNone(pushed([after, push(OLD, "2026-10-22T11:00:00Z")])) + self.assertIsNone(pushed([after])) + self.assertIsNone(pushed([before], unreadable="head pushes")) + fork = PULL | {"head": PULL["head"] | {"repo": {"full_name": "fork/repo"}}} + self.assertIsNone(pushed([before], pull=fork)) + self.assertFalse(any("/activity?" in r for r in self.reads)) + # An issue decision with no marker carries nothing and reads no pushes. + self.assertIsNone(self.decide(markers=[], pushes=[before])) + self.assertFalse(any("/activity?" in r for r in self.reads)) + def test_a_failed_permission_read_clears_nothing(self) -> None: def refuse(login: str) -> str: raise policy.PolicyReadError("permission unreadable") @@ -193,7 +359,7 @@ def refuse(login: str) -> str: ): decision = policy.cost_decision( REPO, - {"number": 99, "body": LINK, "created_at": PR_CREATED}, + PULL | {"body": LINK}, 40, permission=refuse, agent_identity=IDENTITY, 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 5c197a414b..fc57e04e6a 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 @@ -21,7 +21,9 @@ - The cost decision: `costDecision` accepts the same comments as `babysit_merge_policy.cost_decision`, with the owner a trusted person in place of an admin, a linked-issue comment counting only when it names the - pull request as `#` (`namesPull`, `names_pull`). + pull request as `#` (`namesPull`, `names_pull`), and both carry it + to the same heads: the decided head, or a later one with the same net + change against the base (`decidedHead`, `pushedHead`, `sameNetChange`). """ from __future__ import annotations @@ -309,7 +311,15 @@ def py_manifest_changes(change: dict[str, Any]) -> list[str]: REPO = "o/r" -PR = {"number": 7, "created_at": "2026-01-02T00:00:00Z"} +HEAD = "0123456789abcdef0123456789abcdef01234567" +BASE = "89abcdef0123456789abcdef0123456789abcdef" +OLD = "fedcba9876543210fedcba9876543210fedcba98" +PR = { + "number": 7, + "created_at": "2026-01-02T00:00:00Z", + "head": {"sha": HEAD, "ref": "b", "repo": {"full_name": REPO}}, + "base": {"sha": BASE}, +} ISSUE = 5 OWNER = {"login": "owner", "id": 1, "type": "User"} OTHER = {"login": "other", "id": 2, "type": "User"} @@ -383,30 +393,98 @@ def _link(target: int, kind: str = "pull", repo: str = REPO) -> str: "another decision": (_link(7), _comment(7, body="**Decision recorded.** Ship")), "indented": (_link(7), _comment(7, body=f" {DECISION}")), } +def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[str, Any]: + body = f"\nHeld." + return {"user": {"type": "Bot"}, "created_at": at, "body": body} | fields + + +# The head reads: the pull request's comments (a cost hold marker at the head +# before the decision by default), its branch's pushes, a compare per head. +DEFAULT_HEADS = {"markers": [_marker(HEAD)], "pushes": [], "compares": {}} +TF = {"filename": "a.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} +MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} +# Each case: the heads world for a decision on the pull request (_link(7)). +CARRIES = { + "a marker at the head": DEFAULT_HEADS, + "a base merge since the decided head": { + "markers": [_marker(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF]}}, + }, + "a commit of its own since": { + "markers": [_marker(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, + }, + "a rename since": { + "markers": [_marker(OLD)], + "compares": { + OLD: {"files": [TF]}, + HEAD: {"files": [TF | {"status": "renamed", "previous_filename": "z.tf"}]}, + }, + }, + "300 files": { + "markers": [_marker(OLD)], + "compares": { + sha: {"files": [TF | {"filename": f"f{i}"} for i in range(300)]} + for sha in (OLD, HEAD) + }, + }, + "a file with no patch": { + "markers": [_marker(OLD)], + "compares": {sha: {"files": [{"filename": "a.png", "status": "added"}]} for sha in (OLD, HEAD)}, + }, + "an unreadable compare": {"markers": [_marker(OLD)], "compares": {OLD: {"files": [TF]}}}, + "the latest marker before it, not an older one at the head": { + "markers": [_marker(HEAD, "2026-01-02T06:00:00Z"), _marker(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, + }, + "a marker after it": {"markers": [_marker(HEAD, "2026-01-04T00:00:00Z")]}, + "a person's marker": {"markers": [_marker(HEAD, user={"type": "User"})]}, + "no marker, pushed at the head before it": { + "markers": [], + "pushes": [{"after": HEAD, "timestamp": "2026-01-02T20:00:00Z"}], + }, + "no marker, pushed at the head after it": { + "markers": [], + "pushes": [ + {"after": HEAD, "timestamp": "2026-01-03T01:00:00Z"}, + {"after": OLD, "timestamp": "2026-01-02T20:00:00Z"}, + ], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, + }, +} NODE_DECISION = """ import { costDecision } from %s; -const [body, comment, issue] = JSON.parse(process.argv[1]); +const [body, comment, issue, heads] = JSON.parse(process.argv[1]); const github = async (method, path, data) => { + if (method === "GET" && path.includes("/compare/")) { + const sha = path.split("...")[1]; + if (!(sha in heads.compares)) throw new Error("no such commit"); + return heads.compares[sha]; + } + if (method === "GET" && path === "/repos/o/r/issues/7/comments?per_page=100&page=1") return heads.markers; + if (method === "GET" && path === "/repos/o/r/activity?ref=refs%%2Fheads%%2Fb&per_page=100&page=1") return heads.pushes; if (method === "GET") return comment; if (data.variables.number !== undefined) { - return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", + return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", headRefName: "b", isCrossRepository: false, closingIssuesReferences: { totalCount: 1, nodes: [{ number: issue, repository: { nameWithOwner: "o/r" } }] } } } } }; } return { data: { node: { lastEditedAt: comment.edited } } }; }; -console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1])))); +console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %s, %s))); """ -def js_decision(body: str, comment: dict[str, Any]) -> str | None: - script = NODE_DECISION % json.dumps(MODULE.as_uri()) +def js_decision( + body: str, comment: dict[str, Any], heads: dict[str, Any] = DEFAULT_HEADS +) -> str | None: + script = NODE_DECISION % (json.dumps(MODULE.as_uri()), json.dumps(HEAD), json.dumps(BASE)) proc = subprocess.run( [ "node", "--input-type=module", "-e", script, - json.dumps([body, comment, ISSUE]), + json.dumps([body, comment, ISSUE, DEFAULT_HEADS | heads]), ], capture_output=True, text=True, @@ -415,12 +493,29 @@ def js_decision(body: str, comment: dict[str, Any]) -> str | None: return json.loads(proc.stdout) -def py_decision(body: str, comment: dict[str, Any]) -> str | None: +def py_decision( + body: str, comment: dict[str, Any], heads: dict[str, Any] = DEFAULT_HEADS +) -> str | None: + heads = DEFAULT_HEADS | heads + def graphql(_query: str, _what: str, **_vars: Any) -> dict[str, Any]: return {"node": {"lastEditedAt": comment["edited"]}} + def get(endpoint: str) -> tuple[int | None, Any]: + if "/compare/" in endpoint: + sha = endpoint.rsplit("...", 1)[1] + return (200, heads["compares"][sha]) if sha in heads["compares"] else (404, None) + return 200, comment + + def paginated(endpoint: str, _label: str) -> list[dict[str, Any]]: + if endpoint == "repos/o/r/activity?ref=refs%2Fheads%2Fb&per_page=100": + return heads["pushes"] + assert endpoint == "repos/o/r/issues/7/comments?per_page=100", endpoint + return heads["markers"] + with ( - mock.patch.object(policy, "_get", return_value=(200, comment)), + mock.patch.object(policy, "_get", side_effect=get), + mock.patch.object(policy, "_paginated", side_effect=paginated), mock.patch.object(policy, "_graphql", graphql), ): return policy.cost_decision( @@ -449,6 +544,25 @@ def test_both_accept_the_same_comments(self) -> None: self.assertIsNone(answers["on the closing issue, naming it only after a #"]) self.assertIsNone(answers["edited"]) + def test_both_carry_a_decision_to_the_same_heads(self) -> None: + answers = {} + for case, heads in CARRIES.items(): + with self.subTest(case): + answers[case] = py_decision(_link(7), _comment(7), heads) + self.assertEqual(js_decision(_link(7), _comment(7), heads), answers[case]) + for case in ( + "a marker at the head", + "a base merge since the decided head", + "no marker, pushed at the head before it", + ): + self.assertEqual(answers[case], _link(7), case) + for case in set(CARRIES) - { + "a marker at the head", + "a base merge since the decided head", + "no marker, pushed at the head before it", + }: + self.assertIsNone(answers[case], case) + if __name__ == "__main__": unittest.main() diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index c61f1a4c1d..2ec0914f9c 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -10,7 +10,11 @@ # `#`, on its linked issue: the one issue GitHub records it as closing, # else, when it closes none, the one plain `Refs:` issue (the base's # merge-holds.mjs refsIssue). A second closing issue counts for nothing. The -# holds are read from the lanes' check-run notices. +# decision covers the head it decided (the latest review-lane cost hold marker +# before it, else, for one on the pull request, the last push before it) and a +# later head only when the net change against the base is the same at both +# (owner decisions Q17 c and Q18 d on #9436). The holds are read from the +# lanes' check-run notices. # The step then outputs cleared=true and the decision, and takes the label off # with one comment citing it. Anything else, an unreadable read included, # clears nothing and leaves the hold to the steps after it. @@ -103,6 +107,7 @@ case "$*" in from "$STUB_RUNS" "$(key workflows "$@")" ;; */compare/"$BASE_SHA..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; +*/activity\?ref=refs%2Fheads%2Ffix%2Fcost\&per_page=100*) failing activity; printf '%s\n' "$STUB_ACTIVITY" ;; */actions/runs/*/jobs*) failing jobs; from "$STUB_JOBS" "$(key runs "$@")" ;; */check-runs/*/annotations*) failing notes; from "$STUB_NOTES" "$(key check-runs "$@")" ;; */pulls/6419*) failing pull; printf '%s\n' "$STUB_PR" ;; @@ -136,6 +141,14 @@ decision() { } note() { jq -cn --arg m "$1" '{annotation_level: "notice", message: $m}'; } hold_note() { note "claude-lane-hold lane=${2:-claude-review} sha=$HEAD criteria=$1 label=needs-human"; } +# marker [at] [criteria]: a review lane's hold comment on the pull request. +marker() { comment "$LANE" "${2:-2026-10-23T13:10:00Z}" " +Held for a person."; } +MARKER="$(marker "$HEAD")" +TF='{"filename":"infra/main.tf","status":"modified","patch":"@@ -1 +1 @@\n-a\n+b"}' +MORE='{"filename":"scripts/run.sh","status":"modified","patch":"+y"}' +# nets : the compare answers at OLD and HEAD. +nets() { STUB_COMPARE="$(jq -cn --arg h "$HEAD" --arg o "$OLD" --argjson a "$1" --argjson b "$2" '{($o): {files: $a}, ($h): {files: $b}}')"; export STUB_COMPARE; } # job job() { jq -cn --argjson id "$1" --arg name "$2" --arg at "$3" '{id: $id, name: $name, completed_at: $at}'; } # lanes : one run per lane, its status job done at 13:20:30 and 13:21:00. @@ -159,15 +172,16 @@ reset() { export STUB_ISSUES='{"9306":{"number":9306}}' export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":0,"nodes":[]}}}}}' STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" - STUB_COMMENTS="[$(comment "$OWNER" 2026-10-23T05:40:00Z 'A comment.')]" + STUB_COMMENTS="[$MARKER,$(comment "$OWNER" 2026-10-23T05:40:00Z 'A comment.')]" export STUB_EVENTS STUB_COMMENTS lanes "[$(hold_note cost),$(note "claude-lane-review-complete lane=claude-review sha=$HEAD findings=0")]" '[]' + export STUB_ACTIVITY='[]' unset STUB_TRACKER } # The owner lifted an earlier hold at 13:15, so the 05:39 escalation is outside the window. lifted() { STUB_EVENTS="[$(event labeled "$APP" 2026-10-23T05:39:30Z),$(event unlabeled "$OWNER" 2026-10-23T13:15:00Z),$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" - STUB_COMMENTS="[$(comment "$APP" 2026-10-23T05:39:35Z '')]" + STUB_COMMENTS="[$MARKER,$(comment "$APP" 2026-10-23T05:39:35Z '')]" export STUB_EVENTS STUB_COMMENTS } @@ -225,10 +239,10 @@ else fi reset -STUB_COMMENTS="[$(comment "$LANE" 2026-10-23T13:21:00Z ' +STUB_COMMENTS="[$MARKER,$(comment "$LANE" 2026-10-23T13:21:00Z ' ci-status took it off.')]" expect "a decision already cited in a removal comment is not cited again" yes yes no -STUB_COMMENTS="[$(comment "$STRANGER" 2026-10-23T13:21:00Z '')]" +STUB_COMMENTS="[$MARKER,$(comment "$STRANGER" 2026-10-23T13:21:00Z '')]" expect "a removal comment by a person does not count as posted" yes yes yes reset export STUB_LABELS='' @@ -255,8 +269,19 @@ reset STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T04:00:00Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" expect "an issue decision older than the pull request clears nothing" no no no STUB_DECISIONS="$(jq -cn --argjson c "$(decision 6419 "$OWNER" 2026-10-23T04:00:00Z '**Decision recorded.** Cost: accepted.')" '{"111": $c}')" -STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost"},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' -WANT_LINK=https://github.com/o/r/pull/6419#issuecomment-111 expect "a decision on the pull request itself clears the hold" yes yes yes +STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","repo":{"full_name":"o/r"}},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' +export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" +WANT_LINK=https://github.com/o/r/pull/6419#issuecomment-111 expect "a decision on the pull request itself, with no marker before it, clears the hold for the head pushed before it" yes yes yes +export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T04:30:00Z\"},{\"activity_type\":\"push\",\"after\":\"$OLD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" +nets "[$TF]" "[$TF,$MORE]" +expect "a decision on the pull request with no marker, pushed over with a change of its own, clears nothing" no no no "covering this head" +export STUB_FAIL=activity +expect "a decision on the pull request with no marker and unreadable pushes clears nothing" no no no "covering this head" +export STUB_FAIL='' +STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","repo":{"full_name":"fork/r"}},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' +export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" +expect "a decision on a pull request from a fork with no marker clears nothing" no no no "covering this head" +if grep -q '/activity' "$TMP_ROOT/log"; then fail "a fork's pushes were read from the base repository"; else ok "a fork's pushes are not read"; fi reset STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$STRANGER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" expect "a decision by someone off the trusted-actor list clears nothing" no no no @@ -330,12 +355,53 @@ Refs: #9306 expect "a body whose Refs lines read '${body//$'\n'/ }' names no linked issue and clears nothing" no no no done +# The carry rule (#9436 Q18 d): the decided head is OLD, the current HEAD. +reset +STUB_COMMENTS="[$(marker "$OLD")]" +nets "[$TF]" "[$TF]" +expect "a base merge after the decided head carries the decision" yes yes yes +if grep -q "compare/$BASE...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base"; else fail "no compare at the decided head"; fi +nets "[$TF]" "[$TF,$MORE]" +expect "a pull request commit after the decided head clears nothing" no no no "covering this head" +nets "[$TF,$MORE]" "[$MORE,$TF]" +expect "a rebase with the same net change, in another file order, carries the decision" yes yes yes +nets "[$TF,$MORE]" "[$TF]" +expect "a rebase that drops one of its files clears nothing" no no no "covering this head" +nets "[$TF]" "[$(jq -c '.patch = "@@ -1 +1 @@\n-a\n+c"' <<<"$TF")]" +expect "a conflict resolution that changes the net change clears nothing" no no no "covering this head" +nets "[$TF]" "[$(jq -c '.status = "renamed" | .previous_filename = "infra/old.tf"' <<<"$TF")]" +expect "a rename since the decided head clears nothing" no no no "covering this head" +nets "$(jq -cn '[range(299) | {filename: "f\(.)", status: "added", patch: "+x"}]')" "$(jq -cn '[range(299) | {filename: "f\(.)", status: "added", patch: "+x"}]')" +expect "a net change of 299 files carries the decision" yes yes yes +nets "$(jq -cn '[range(300) | {filename: "f\(.)", status: "added", patch: "+x"}]')" "$(jq -cn '[range(300) | {filename: "f\(.)", status: "added", patch: "+x"}]')" +expect "a compare of 300 files carries nothing" no no no +nets '[{"filename":"a.png","status":"modified"}]' '[{"filename":"a.png","status":"modified"}]' +expect "a file with no patch carries nothing" no no no "covering this head" +nets "[$TF]" "[$TF]" +export STUB_FAIL=compare +expect "a failed compare read carries nothing" no no no "covering this head" +export STUB_FAIL='' +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T13:00:00Z),$(marker "$OLD" 2026-10-23T13:12:00Z)]" +nets "[$TF]" "[$TF,$MORE]" +expect "the latest marker before the decision decides its head, not an older one at the head" no no no "covering this head" +STUB_COMMENTS="[$(marker "$OLD" 2026-10-23T13:00:00Z),$(marker "$HEAD" 2026-10-23T13:12:00Z cost,trust-gate)]" +expect "a later marker before the decision whose criteria include cost decides its head" yes yes yes +STUB_COMMENTS="[$(marker "$OLD"),$(marker "$HEAD" 2026-10-23T13:16:00Z)]" +expect "a marker after the decision decides nothing" no no no "covering this head" +STUB_COMMENTS="[$(marker "$OLD"),$(comment "$OWNER" 2026-10-23T13:12:00Z "")]" +expect "a marker a person wrote decides nothing" no no no "covering this head" +STUB_COMMENTS='[]' +expect "an issue decision with no marker before it clears nothing" no no no "covering this head" +reset +export STUB_FAIL=module +expect "an unreadable base merge-holds.mjs carries no closing-issue decision" no no no "covering this head" + reset lanes "[$(hold_note cost,trust-gate)]" '[]' expect "a hold with cost and trust-gate clears nothing" no no no "is not cost-only" lanes "[$(hold_note cost)]" "[$(hold_note critical-security claude-security-review)]" expect "a critical-security hold from the other lane clears nothing" no no no "is not cost-only" -STUB_COMMENTS="[$(comment "$LANE" 2026-10-23T13:21:01Z " +STUB_COMMENTS="[$MARKER,$(comment "$LANE" 2026-10-23T13:21:01Z " claude-lane: claude-security-review")]" expect "a hold comment edited to read cost does not outweigh the lane's notice" no no no "is not cost-only" reset @@ -360,13 +426,13 @@ STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:09:58Z),$(event unlabeled "$ expect "a trusted person's removal of the trust-gate hold opens a new window" yes yes yes reset -STUB_COMMENTS="[$(comment "$APP" 2026-10-23T05:39:35Z '')]" +STUB_COMMENTS="[$MARKER,$(comment "$APP" 2026-10-23T05:39:35Z '')]" expect "a lanes App escalation with no trusted removal since keeps the label" no no no "lanes App commented" lifted expect "a lanes App escalation before the window does not keep the label" yes yes yes -STUB_COMMENTS="[$(comment "$APP" 2026-10-23T13:22:00Z '')]" +STUB_COMMENTS="[$MARKER,$(comment "$APP" 2026-10-23T13:22:00Z '')]" expect "a lanes App comment in the window keeps the label" no no no "lanes App commented" -STUB_COMMENTS="[$(jq -c '.updated_at = "2026-10-23T13:22:00Z"' <<<"$(comment "$APP" 2026-10-23T05:39:35Z 'Edited.')")]" +STUB_COMMENTS="[$MARKER,$(jq -c '.updated_at = "2026-10-23T13:22:00Z"' <<<"$(comment "$APP" 2026-10-23T05:39:35Z 'Edited.')")]" expect "a lanes App comment edited in the window keeps the label" no no no "lanes App commented" reset lifted @@ -412,7 +478,7 @@ reset export LANES_APP_ID='' expect "an unset lanes App id clears nothing" no no no -for read in tracker actors events pull runs compare jobs notes comments deleted closing decision edited; do +for read in tracker actors events pull runs compare jobs notes comments deleted closing decision edited module; do reset export STUB_FAIL="$read" expect "an unreadable $read read clears nothing" no no no From b3556945372ad43e1c3696a80bb5c254739068c2 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 02:47:47 -0400 Subject: [PATCH 4/9] fix(pr-merge): read the decided head as the review lanes' hold step does Align the three cost-decision readers with ci-workflows v0.65.4's apply-review-hold carry test (ci-workflows#877, 31b44f3f): - The decided head is the newest marker github-actions[bot] posted as the first line of a comment, criteria including cost, before the decision. With none, nothing carries, even at the same head; the push-time fallback is gone. - At the decided head it carries with no compare read. Otherwise compare ...decided and ...head; a duplicate filename, a file with no patch (so a pure rename), 300 or more files, an unusable base branch name or any failed read carries nothing. - The decision judged is the newest on the pull request, else the newest on its linked issue; only it may carry. Co-Authored-By: Claude Opus 5.5 --- .../agent-a5e09147a92521857-c3fc6a4d.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 2 +- .../pr-merge/enable-auto-merge.test.mjs | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 119 +++++++------ .github/scripts/pr-merge/merge-holds.test.mjs | 112 +++++++----- .github/workflows/pr-require-checks.yml | 52 +++--- .../scripts/babysit_merge_policy.py | 159 +++++++----------- .../scripts/tests/test_babysit_merge_cost.py | 110 ++++++++---- .../scripts/tests/test_merge_holds_sync.py | 45 +++-- scripts/ci-hold-cost-decision.test.sh | 72 +++++--- 10 files changed, 375 insertions(+), 300 deletions(-) diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md index a9e6160333..069c2f5e83 100644 --- a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md +++ b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `cost_decision` binds an owner cost decision to one pull request and the head it decided: a `**Decision recorded.** Cost:` comment on the pull request, or one on its linked issue (closing, else the one `Refs:` issue) that names the pull request as `#` (not `owner/repo#`, `&#` or `##`), covers the head of the latest review-lane cost hold marker before it (for one on the pull request with no marker, the last push before it), and a later head only when every change since is a merge of the base: the pull request's net change against the base, file by file, is the same at both. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. +- `cost_decision` binds an owner cost decision to one pull request and the head it decided. A `**Decision recorded.** Cost:` comment counts on the pull request, or on its linked issue (closing, else the one `Refs:` issue) when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`); the newest on the pull request, else the newest on the issue, is the decision. It covers the head of the newest github-actions[bot] cost hold marker before it, and a later head only when every change since is a merge of the base: the pull request's net change against its base branch, file by file, is the same at both. With no marker before it, it covers no head. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index e6fb097ed5..47184c20cd 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -696,7 +696,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m detail = (await github("POST", "/graphql", { query: DETAIL_QUERY, variables }))?.data?.repository?.pullRequest ?? null; if (detail !== null) { const files = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); - const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid, pull.baseRefOid) : null; + const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid) : null; detail = { ...detail, files, costDecision: decision }; } } catch (error) { diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 0081a82383..7d30106cd4 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -120,7 +120,7 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note } // Its decided head: a review lane's cost hold marker at the head, before it. if (method === "GET" && /^\/repos\/melodic-software\/claude-code-plugins\/issues\/[0-9]+\/comments\?per_page=100&page=1$/.test(path)) { - return [{ user: { type: "Bot" }, created_at: "2026-10-22T11:00:00Z", body: `` }]; + return [{ user: { login: "github-actions[bot]", type: "Bot" }, created_at: "2026-10-22T11:00:00Z", body: `` }]; } // The read-back pin, posted before an arm. if (method === "POST" && String(body?.body ?? "").startsWith("(?![^\r\n])/; -const SHA = /^[0-9a-f]{40}$/; +// it on the first line of its hold comment: the head it held and its +// criteria. The base branch name a compare takes, as the hold step reads it. +const COST_MARKER = /^(?:\n|$)/; +const BRANCH = /^(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}$/; // The head a cost decision made at `at` decided, from the pull request's -// `comments`: the sha of the latest cost hold marker a Bot posted before it, -// else null (babysit_merge_policy.decided_head). +// `comments`: the sha of the newest cost hold marker github-actions[bot] +// posted before it, else null (babysit_merge_policy.decided_head; the first +// of two at one time wins, as in the review lanes' hold step). export function decidedHead(comments, at) { + const cutoff = Date.parse(at); let found = null; + let latest = -Infinity; for (const row of Array.isArray(comments) ? comments : []) { - const marker = row?.user?.type === "Bot" ? COST_MARKER.exec(String(row.body ?? "")) : null; + const marker = row?.user?.login === "github-actions[bot]" ? COST_MARKER.exec(String(row.body ?? "")) : null; const made = Date.parse(row?.created_at); - if (marker && marker[2].split(",").includes("cost") && made < Date.parse(at) && (found === null || made >= found.made)) { - found = { made, sha: marker[1] }; + if (marker && marker[2].split(",").includes("cost") && made < cutoff && made > latest) { + found = marker[1]; + latest = made; } } - return found?.sha ?? null; -} - -// The head of the branch when a decision on the pull request was made at -// `at`, with no cost hold marker before it: the commit of the latest push -// the repository activity `pushes` timestamp before it, else null -// (babysit_merge_policy.pushed_head). A commit's own dates are its -// author's text, so they place nothing. -export function pushedHead(pushes, at) { - let found = null; - for (const row of Array.isArray(pushes) ? pushes : []) { - const made = Date.parse(row?.timestamp); - if (SHA.test(String(row?.after)) && !/^0+$/.test(row.after) && made < Date.parse(at) && (found === null || made >= found.made)) { - found = { made, sha: row.after }; - } - } - return found?.sha ?? null; + return found; } // Whether two compare answers (base...head) show the same net change of the -// pull request: the same files by filename, each with the same status, +// pull request: the same filenames, each with the same status, // previous_filename and patch. A list of 300 files or more (the compare -// API's cap), a file with no patch, or an answer with no file list is not -// the same (babysit_merge_policy.same_net_change). +// API's cap), a file with no patch, a filename listed twice, or an answer +// with no file list is not the same (babysit_merge_policy.same_net_change). export function sameNetChange(decided, current) { const net = (compare) => { const files = compare?.files; if (!Array.isArray(files) || files.length >= 300) return null; - if (!files.every((f) => typeof f?.filename === "string" && typeof f.status === "string" && typeof f.patch === "string")) return null; - return files - .map((f) => JSON.stringify([f.filename, f.status, f.previous_filename ?? null, f.patch])) - .sort() - .join("\n"); + const byName = new Map(); + for (const f of files) { + if (typeof f?.filename !== "string" || typeof f.patch !== "string" || byName.has(f.filename)) return null; + byName.set(f.filename, JSON.stringify([f.status ?? null, f.previous_filename ?? null, f.patch])); + } + return byName; }; const [then, now] = [net(decided), net(current)]; - return then !== null && then === now; + return then !== null && now !== null && then.size === now.size && [...then].every(([name, value]) => now.get(name) === value); } // Every page of the REST list at `path`. @@ -509,34 +499,32 @@ async function pages(github, path) { } // Whether the decision `comment` covers pull request `number` at `head` -// (owner decisions Q17 c and Q18 d): its decided head (decidedHead; for a -// decision on the pull request with no marker before it, pushedHead on the -// branch `pull.headRefName` of `repo`) is `head`, or every change since it -// is a merge of the base, so the net change against `base` is the same at -// both (sameNetChange). A read that fails carries nothing. -async function carries(github, repo, number, comment, onPull, pull, head, base) { - if (!SHA.test(String(head)) || !SHA.test(String(base))) { - return false; - } +// (owner decisions Q17 c and Q18 d on #9436): its decided head (decidedHead) +// is `head`, with no compare read, or every change since is a merge of the +// base branch `base`, so the net change against it is the same at both +// (sameNetChange). No marker, an unusable base name or a read that fails +// carries nothing. +async function carries(github, repo, number, comment, head, base) { try { - let decided = decidedHead(await pages(github, `/repos/${repo}/issues/${number}/comments`), comment.created_at); - if (decided === null && onPull && pull?.isCrossRepository === false && typeof pull.headRefName === "string" && pull.headRefName) { - decided = pushedHead(await pages(github, `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${pull.headRefName}`)}`), comment.created_at); - } + const decided = decidedHead(await pages(github, `/repos/${repo}/issues/${number}/comments`), comment.created_at); if (decided === null) { return false; } if (decided === head) { return true; } - const [then, now] = await Promise.all([decided, head].map((sha) => github("GET", `/repos/${repo}/compare/${base}...${sha}`))); + if (!BRANCH.test(String(base))) { + return false; + } + const then = await github("GET", `/repos/${repo}/compare/${base}...${decided}`); + const now = await github("GET", `/repos/${repo}/compare/${base}...${head}`); return sameNetChange(then, now); } catch { return false; } } -// The first comment link in `body`, outside HTML comments, that records the +// The comment link in `body`, outside HTML comments, that records the // owner's cost decision for pull request `number` of `repo` at `head`, else // null: the rule of babysit_merge_policy.cost_decision, with the owner a // person whose id is in `humans` (trusted-actors.json), as ci-status's @@ -544,13 +532,14 @@ async function carries(github, repo, number, comment, onPull, pull, head, base) // or on its linked issue in `repo` (the one closing issue, else with none the // one refsIssue names, read to be an issue and not a pull request), newer // than the pull request and naming it (namesPull); by a User, not through an -// App; never edited; its first line `**Decision recorded.** Cost:`. It -// covers the head it decided and a later one only when every change since is -// a base merge (carries, against `base`). `github(method, path, body)` -// reads; a failed read of the pull request or the Refs target decides -// nothing, and one of a comment, its edit state or its head skips that -// comment. -export async function costDecision(github, repo, number, body, humans, head, base) { +// App; never edited; its first line `**Decision recorded.** Cost:`. Of +// those, the decision is the newest on the pull request, else the newest on +// the issue, as the review lanes' hold step picks it; it covers the head it +// decided and a later one only when every change since is a base merge +// (carries, against the pull request's base branch). `github(method, path, +// body)` reads; a failed read of the pull request or the Refs target decides +// nothing, and one of a comment or its edit state skips that comment. +export async function costDecision(github, repo, number, body, humans, head) { const links = [...String(body ?? "").replaceAll(HTML_COMMENT, "").matchAll(DECISION_LINK)].filter( (link) => `${link[1]}/${link[2]}`.toLowerCase() === String(repo).toLowerCase(), ); @@ -573,6 +562,7 @@ export async function costDecision(github, repo, number, body, humans, head, bas ? closing.nodes[0].number : null); let checked = refs === null; + const decisions = []; for (const link of links) { const target = Number(link[3]); if (target !== number && target !== issue) { @@ -604,14 +594,21 @@ export async function costDecision(github, repo, number, body, humans, head, bas continue; } const node = (await github("POST", "/graphql", { query: COMMENT_EDITED_QUERY, variables: { id: String(comment.node_id) } }))?.data?.node; - if (node && "lastEditedAt" in node && node.lastEditedAt === null && (await carries(github, repo, number, comment, target === number, pull, head, base))) { - return link[0]; + if (node && "lastEditedAt" in node && node.lastEditedAt === null) { + decisions.push({ link: link[0], comment, own: target === number }); } } catch { // An unreadable comment clears nothing. } } - return null; + const at = (d) => { + const made = Date.parse(d.comment.created_at); + return Number.isNaN(made) ? -Infinity : made; + }; + // The newest; of two at one time, the first linked. + const newest = (own) => decisions.filter((d) => d.own === own).reduce((best, d) => (best === undefined || at(d) > at(best) ? d : best), undefined); + const decision = newest(true) ?? newest(false); + return decision && (await carries(github, repo, number, decision.comment, head, pull?.baseRefName)) ? decision.link : null; } // A GitHub REST and GraphQL reader on fetch, for costDecision. @@ -816,7 +813,7 @@ export async function main(argv = process.argv.slice(2), { env = process.env, gi const body = readFileSync(bodyFile, "utf8"); const repo = String(env.GITHUB_REPOSITORY); const decision = - costHold(files, null) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo) ? await costDecision(github, repo, Number(number), body, humans, head, env.BASE_SHA) : null; + costHold(files, null) && /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo) ? await costDecision(github, repo, Number(number), body, humans, head) : null; if (decision) { console.log(`#${number}: ${decision} records the owner's cost decision, which lifts its cost hold.`); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 3484acdbc6..22e369b8f3 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -10,11 +10,12 @@ // pins case by case. ci-status names its owner by the trusted-actor list, as // its cost-only review hold step does, so a review approval lifts a hold path // but never a cost hold. A decision binds to one pull request: one on the pull -// request counts as it is, one on its linked issue (closing, else `Refs:`) only when it names -// `#`. It covers the head it decided, the sha of the latest cost hold -// marker before it (else, for one on the pull request, the last push before -// it), and a later head only when the net change against the base is the same -// at both (owner decisions Q17 c and Q18 d on #9436). +// request counts as it is, one on its linked issue (closing, else `Refs:`) +// only when it names `#`. It covers the head it decided, the sha of the +// newest cost hold marker github-actions[bot] posted before it, and a later +// head only when the net change against the base branch is the same at both +// (owner decisions Q17 c and Q18 d on #9436, as ci-workflows v0.65.4's hold +// step reads them). import assert from "node:assert/strict"; import { Buffer } from "node:buffer"; import { mkdtempSync, writeFileSync } from "node:fs"; @@ -22,7 +23,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, decidedHead, linkHold, main, pushedHead, sameNetChange } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, decidedHead, linkHold, main, sameNetChange } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -55,7 +56,7 @@ const comment = (changes = {}) => ({ // A GitHub that answers the decision reads: the pull request's creation time // and closing issues, issue 40 (`target`), comment 7, and its edit time; then // the head reads: the pull request's `comments` (a cost hold marker at HEAD -// by default), its branch's `pushes` and a `compares` answer per head. +// by default) and a `compares` answer per head against `base`. // `fail` names a read that throws. function world({ row = comment(), @@ -65,13 +66,12 @@ function world({ target = { number: 40 }, fail, comments = [marker(HEAD)], - pushes = [], compares = {}, - cross = false, + base = "main", } = {}) { const reads = []; const github = async (method, path, body) => { - const compare = /^\/repos\/owner\/repo\/compare\/([0-9a-f]{40})\.\.\.([0-9a-f]{40})$/.exec(path); + const compare = /^\/repos\/owner\/repo\/compare\/(.+)\.\.\.([0-9a-f]{40})$/.exec(path); const read = body?.query === PULL_OPENED_QUERY ? "pull" @@ -79,11 +79,9 @@ function world({ ? "edited" : path === "/repos/owner/repo/issues/99/comments?per_page=100&page=1" ? "comments" - : path === "/repos/owner/repo/activity?ref=refs%2Fheads%2Ffix%2Fcost&per_page=100&page=1" - ? "pushes" - : compare - ? `compare ${compare[2]}` - : `${method} ${path}`; + : compare + ? `compare ${compare[2]}` + : `${method} ${path}`; reads.push(read); if (read === fail) { throw new Error(`${read} failed`); @@ -91,11 +89,8 @@ function world({ if (read === "comments") { return comments; } - if (read === "pushes") { - return pushes; - } if (compare) { - assert.equal(compare[1], BASE); + assert.equal(compare[1], base); if (!(compare[2] in compares)) { throw new Error("no such commit"); } @@ -106,7 +101,7 @@ function world({ return { data: { repository: { - pullRequest: { createdAt: created, headRefName: "fix/cost", isCrossRepository: cross, closingIssuesReferences: { totalCount: closing.length, nodes: closing } }, + pullRequest: { createdAt: created, baseRefName: base, closingIssuesReferences: { totalCount: closing.length, nodes: closing } }, }, }, }; @@ -126,7 +121,7 @@ function world({ const decide = (body = `Closes #40\n\nCost decision: ${LINK}\n`, options = {}) => { const { github, reads } = world(options); - return costDecision(github, REPO, 99, body, HUMANS, options.head ?? HEAD, BASE).then((link) => ({ link, reads })); + return costDecision(github, REPO, 99, body, HUMANS, options.head ?? HEAD).then((link) => ({ link, reads })); }; test("an owner decision on the closing issue, linked from the body, clears the cost hold", async () => { @@ -402,7 +397,7 @@ test("a conflict resolution, a rename or a status change that alters the net cha } }); -test("a compare of 300 files or more, a file with no patch, or a failed compare read carries nothing", async () => { +test("a compare of 300 files or more, a file with no patch, a filename listed twice, an unusable base name or a failed read carries nothing", async () => { const many = (n) => net(...Array.from({ length: n }, (_, i) => ({ filename: `f${i}`, status: "added", patch: "+x" }))); assert.equal((await later({ [OLD]: many(299), [HEAD]: many(299) })).link, LINK); assert.equal((await later({ [OLD]: many(300), [HEAD]: many(300) })).link, null); @@ -411,6 +406,55 @@ test("a compare of 300 files or more, a file with no patch, or a failed compare assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { fail: `compare ${HEAD}` })).link, null); assert.equal((await later({ [OLD]: net(TF), [HEAD]: { status: "diverged" } })).link, null); assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { fail: "comments" })).link, null); + assert.equal((await later({ [OLD]: net(TF, TF), [HEAD]: net(TF, TF) })).link, null); + // A patchless pure rename never carries. + const moved = { filename: "b.tf", status: "renamed", previous_filename: "a.tf" }; + assert.equal((await later({ [OLD]: net(moved), [HEAD]: net(moved) })).link, null); + for (const base of ["-main", "a..b", "a b", "", "x".repeat(256)]) { + const odd = await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base }); + assert.equal(odd.link, null, base); + assert.ok(!odd.reads.some((read) => read.startsWith("compare")), base); + } + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base: "release/2.x" })).link, LINK); +}); + +test("at the decided head the decision carries with no compare read", async () => { + const { link, reads } = await decide(undefined, { base: "" }); + assert.equal(link, LINK); + assert.ok(!reads.some((read) => read.startsWith("compare"))); +}); + +test("with no cost hold marker before the decision nothing carries, even at the same head", async () => { + const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; + const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", body: "**Decision recorded.** Cost: approve it." }); + for (const options of [{ row, closing: [] }, {}]) { + const { link, reads } = await decide(options.row ? own : undefined, { ...options, comments: [] }); + assert.equal(link, null); + assert.deepEqual(reads.slice(-1), ["comments"]); + } +}); + +test("the decision is the newest on the pull request, else the newest on its linked issue", async () => { + const onPull = (id, at) => `https://github.com/owner/repo/pull/99#issuecomment-${id}`; + const rows = { + 7: comment({ created_at: "2026-10-22T12:00:00Z" }), + 8: comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: "2026-10-22T11:00:00Z", node_id: "IC_7" }), + 9: comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", created_at: "2026-10-22T11:30:00Z", node_id: "IC_7" }), + }; + const pick = async (body, comments) => { + const { github: base } = world({ comments, compares: { [OLD]: net(TF), [HEAD]: net(TF, { filename: "b.tf", status: "added", patch: "+b" }) } }); + const github = async (method, path, data) => { + const id = /\/issues\/comments\/([0-9]+)$/.exec(path)?.[1]; + return id ? rows[id] : base(method, path, data); + }; + return costDecision(github, REPO, 99, body, HUMANS, HEAD); + }; + const all = `Closes #40\n${LINK}\n${onPull(8)}\n${onPull(9)}`; + // The pull request's newest (9, at 11:30) is the one judged, over the issue's newer one. + assert.equal(await pick(all, [marker(HEAD, "2026-10-22T11:15:00Z")]), onPull(9)); + // Its decided head is not this head, and the older one at this head is not consulted. + assert.equal(await pick(all, [marker(HEAD, "2026-10-22T10:00:00Z"), marker(OLD, "2026-10-22T11:15:00Z")]), null); + assert.equal(await pick(`Closes #40\n${LINK}`, [marker(HEAD)]), LINK); }); test("the decided head is the latest bot cost hold marker made before the decision", async () => { @@ -422,35 +466,19 @@ test("the decided head is the latest bot cost hold marker made before the decisi for (const [name, extra] of Object.entries({ after: marker(HEAD, "2026-10-22T12:30:00Z"), "a person": marker(HEAD, "2026-10-22T11:30:00Z", { user: { login: "owner-person", id: 5, type: "User" } }), + "another bot": marker(HEAD, "2026-10-22T11:30:00Z", { user: { login: "claude[bot]", id: 9, type: "Bot" } }), + "not on the first line": { ...marker(HEAD, "2026-10-22T11:30:00Z"), body: `Held.\n` }, "not cost": marker(HEAD, "2026-10-22T11:30:00Z", { criteria: "trust-gate" }), })) { assert.equal((await decide(undefined, { comments: [marker(OLD), extra], compares: same })).link, null, name); } assert.equal((await decide(undefined, { comments: [marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z", { criteria: "cost,trust-gate" })], compares: same })).link, LINK); assert.equal(decidedHead([marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z")], "2026-10-22T11:30:00Z"), OLD); + // Of two at one time, the first listed wins, as in the hold step. + assert.equal(decidedHead([marker(OLD), marker(HEAD)], "2026-10-22T12:00:00Z"), OLD); assert.equal(decidedHead([], "2026-10-22T12:00:00Z"), null); }); -test("with no marker before it, a decision on the pull request covers the head of the last push before it", async () => { - const own = "https://github.com/owner/repo/pull/99#issuecomment-7"; - const row = comment({ issue_url: "https://api.github.com/repos/owner/repo/issues/99", body: "**Decision recorded.** Cost: approve it." }); - const push = (after, timestamp) => ({ activity_type: "push", after, timestamp }); - const changed = { [OLD]: net(TF), [HEAD]: net(TF, { filename: "b.tf", status: "added", patch: "+b" }) }; - const pushed = (pushes, options = {}) => decide(own, { row, closing: [], comments: [], pushes, compares: changed, ...options }); - assert.equal((await pushed([push(HEAD, "2026-10-22T11:59:00Z"), push(OLD, "2026-10-22T11:00:00Z")])).link, own); - assert.equal((await pushed([push(HEAD, "2026-10-22T12:01:00Z"), push(OLD, "2026-10-22T11:00:00Z")])).link, null); - assert.equal((await pushed([push(HEAD, "2026-10-22T12:01:00Z")])).link, null); - assert.equal((await pushed([push(HEAD, "2026-10-22T11:59:00Z")], { fail: "pushes" })).link, null); - const fork = await pushed([push(HEAD, "2026-10-22T11:59:00Z")], { cross: true }); - assert.equal(fork.link, null); - assert.ok(!fork.reads.includes("pushes")); - // An issue decision with no marker carries nothing and reads no pushes. - const issue = await decide(undefined, { comments: [], pushes: [push(HEAD, "2026-10-22T11:59:00Z")] }); - assert.equal(issue.link, null); - assert.ok(!issue.reads.includes("pushes")); - assert.equal(pushedHead([push("0".repeat(40), "2026-10-22T11:59:00Z"), push(OLD, "2026-10-22T11:00:00Z")], "2026-10-22T12:00:00Z"), OLD); -}); - test("sameNetChange compares filename, status, previous_filename and patch", () => { assert.equal(sameNetChange(net(TF), net({ ...TF })), true); assert.equal(sameNetChange(net(), net()), true); diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index a9b4cd6070..8a67c59ea5 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3561,13 +3561,14 @@ jobs: # `**Decision recorded.** Cost:`. One on the pull request itself counts # as it is; one on its linked issue counts only when written after the # pull request and naming it as `#` (merge-holds.mjs - # namesPull). It covers the head it decided (the latest review-lane - # cost hold marker before it, else, for one on the pull request, the - # last push before it) and a later head only when the pull request's - # net change against the base is the same at both, so every change - # since is a base merge (the base's merge-holds.mjs decidedHead, - # pushedHead and sameNetChange; owner decisions Q17 c and Q18 d on - # #9436). The linked issue is the one issue in this repository that + # namesPull). Of those, the newest on the pull request, else the + # newest on the issue, is the decision. It covers the head it decided + # (the newest github-actions[bot] cost hold marker before it; with + # none, no head) and a later head only when the pull request's net + # change against its base branch is the same at both, so every change + # since is a base merge (the base's merge-holds.mjs decidedHead and + # sameNetChange; owner decisions Q17 c and Q18 d on #9436, as + # ci-workflows v0.65.4's hold step reads them). The linked issue is the one issue in this repository that # GitHub records it as closing, else, when it closes none, the one issue # its plain `Refs:` lines name (the base's merge-holds.mjs refsIssue), # read to be an issue and not a pull request. A second closing issue @@ -3677,25 +3678,24 @@ jobs: console.log(run(...files.map((file) => JSON.parse(readFileSync(file, "utf8")))) ?? "");' \ "$@" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs" } - # carries : whether the decision comment on - # covers HEAD_SHA. Any read that fails carries nothing. + # carries : whether the decision comment covers HEAD_SHA: + # its decided head is HEAD_SHA, or the net change against the base + # branch is the same at both. No marker, an unusable base branch + # name or any read that fails carries nothing. carries() { - local at="$RUNNER_TEMP/cost-hold-at.json" decided pushes + local at="$RUNNER_TEMP/cost-hold-at.json" decided base_ref jq '.created_at' <<<"$1" >"$at" || return 1 decided=$(holds_js decidedHead "$RUNNER_TEMP/cost-hold-comments.json" "$at") || return 1 - if [ -z "$decided" ] && [ "$2" = "$PR_NUMBER" ] && - jq -e --arg repo "$repo" '(.head.repo.full_name // "" | ascii_downcase) == ($repo | ascii_downcase)' <<<"$pr" >/dev/null; then - pushes=$(read_api "repos/$repo/activity?ref=refs%2Fheads%2F$ref&per_page=100" --paginate) || return 1 - jq -sc 'add // []' <<<"$pushes" >"$RUNNER_TEMP/cost-hold-pushes.json" || return 1 - decided=$(holds_js pushedHead "$RUNNER_TEMP/cost-hold-pushes.json" "$at") || return 1 - fi [[ $decided =~ ^[0-9a-f]{40}$ ]] || return 1 [ "$decided" != "$HEAD_SHA" ] || return 0 - read_api "repos/$repo/compare/$BASE_SHA...$decided" >"$RUNNER_TEMP/cost-hold-then.json" || return 1 - read_api "repos/$repo/compare/$BASE_SHA...$HEAD_SHA" >"$RUNNER_TEMP/cost-hold-now.json" || return 1 + base_ref=$(jq -r '.base.ref // ""' <<<"$pr") || return 1 + jq -en --arg b "$base_ref" '$b | test("^(?!-)(?!.*\\.\\.)[A-Za-z0-9._/-]{1,255}$")' >/dev/null || return 1 + read_api "repos/$repo/compare/$base_ref...$decided" >"$RUNNER_TEMP/cost-hold-then.json" || return 1 + read_api "repos/$repo/compare/$base_ref...$HEAD_SHA" >"$RUNNER_TEMP/cost-hold-now.json" || return 1 [ "$(holds_js sameNetChange "$RUNNER_TEMP/cost-hold-then.json" "$RUNNER_TEMP/cost-hold-now.json")" = true ] } - decision='' decision_id='' refs_read= + # Every decision that counts, as {own, at, link, id, comment}. + decisions='[]' refs_read= while IFS=$'\t' read -r link owner name number id; do [ -n "$link" ] || continue [ "$(tr '[:upper:]' '[:lower:]' <<<"$owner/$name")" = "$(tr '[:upper:]' '[:lower:]' <<<"$repo")" ] || continue @@ -3719,10 +3719,18 @@ jobs: # shellcheck disable=SC2016 # GraphQL variables, not shell edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$node") || continue jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$edited" >/dev/null 2>&1 || continue - carries "$comment" "$number" || continue - decision=$link decision_id=$id - break + decisions=$(jq -c --argjson c "$comment" --arg link "$link" --arg id "$id" --arg own "$([ "$number" = "$PR_NUMBER" ] && echo 1)" \ + '. + [{own: ($own == "1"), at: ($c.created_at | fromdateiso8601? // -1), link: $link, id: $id, comment: $c}]' <<<"$decisions") || continue done <<<"$links" + # The decision is the newest on the pull request, else the newest on + # its linked issue (the first linked of two at one time), as the + # review lanes' hold step picks it; only it may carry. + chosen=$(jq -c '([.[] | select(.own)] | if length > 0 then . else null end) // [.[] | select(.own | not)] + | to_entries | sort_by([-.value.at, .key]) | .[0].value // empty' <<<"$decisions") || keep "the decisions do not parse" + decision='' decision_id='' + if [ -n "$chosen" ] && carries "$(jq -c .comment <<<"$chosen")"; then + decision=$(jq -r .link <<<"$chosen") decision_id=$(jq -r .id <<<"$chosen") + fi [ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering this head" # Every hold a review lane recorded in the window, as {at, criteria}. holds='[]' vetted='' 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 8691729fcf..0e9326d018 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 @@ -1999,26 +1999,30 @@ def _after(value: Any, opened: datetime | None) -> bool: # A review lane's cost hold marker, as ci-workflows apply-review-hold writes -# it: the head it held and its criteria, cost among them. +# it on the first line of its hold comment: the head it held and its +# criteria. The base branch name a compare takes, as the hold step reads it. COST_MARKER = re.compile( - r"(?:^|\n)(?![^\r\n])" + r"(?:\n|\Z)" ) -COMMIT_SHA = re.compile(r"[0-9a-f]{40}") +BASE_BRANCH = re.compile(r"(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}") +_NEVER = datetime.min.replace(tzinfo=UTC) def decided_head(comments: Any, at: Any) -> str | None: """The head a cost decision made at `at` decided, from the pull - request's `comments`: the sha of the latest cost hold marker a Bot posted - before it, else None (merge-holds.mjs `decidedHead`).""" + request's `comments`: the sha of the newest cost hold marker + github-actions[bot] posted before it, else None (merge-holds.mjs + `decidedHead`; the first of two at one time wins, as in the review lanes' + hold step).""" when = parse_timestamp(at) found: tuple[datetime, str] | None = None for row in json_array(comments): row = json_object(row) made = parse_timestamp(row.get("created_at")) marker = ( - COST_MARKER.search(str(row.get("body") or "")) - if json_object(row.get("user")).get("type") == "Bot" + COST_MARKER.match(str(row.get("body") or "")) + if json_object(row.get("user")).get("login") == "github-actions[bot]" else None ) if ( @@ -2027,110 +2031,64 @@ def decided_head(comments: Any, at: Any) -> str | None: and made is not None and when is not None and made < when - and (found is None or made >= found[0]) + and (found is None or made > found[0]) ): found = (made, marker[1]) return found[1] if found else None -def pushed_head(pushes: Any, at: Any) -> str | None: - """The head of the branch when a decision on the pull request was made at - `at`, with no cost hold marker before it: the commit of the latest push - the repository activity `pushes` timestamp before it, else None - (merge-holds.mjs `pushedHead`). A commit's own dates are its author's - text, so they place nothing.""" - when = parse_timestamp(at) - found: tuple[datetime, str] | None = None - for row in json_array(pushes): - row = json_object(row) - after = str(row.get("after") or "") - made = parse_timestamp(row.get("timestamp")) - if ( - COMMIT_SHA.fullmatch(after) - and after.strip("0") - and made is not None - and when is not None - and made < when - and (found is None or made >= found[0]) - ): - found = (made, after) - return found[1] if found else None - - def same_net_change(decided: Any, current: Any) -> bool: """Whether two compare answers (base...head) show the same net change of - the pull request: the same files by filename, each with the same status, + the pull request: the same filenames, each with the same status, previous_filename and patch. A list of 300 files or more (the compare - API's cap), a file with no patch, or an answer with no file list is not - the same (merge-holds.mjs `sameNetChange`).""" + API's cap), a file with no patch, a filename listed twice, or an answer + with no file list is not the same (merge-holds.mjs `sameNetChange`).""" - def net(compare: Any) -> list[str] | None: + def net(compare: Any) -> dict[str, tuple[Any, Any, str]] | None: files = json_object(compare).get("files") if not is_json_array(files) or len(files) >= 300: return None - rows = [] + by_name: dict[str, tuple[Any, Any, str]] = {} for f in files: - if not is_json_object(f) or not all( - isinstance(f.get(k), str) for k in ("filename", "status", "patch") - ): + f = json_object(f) + name, patch = f.get("filename"), f.get("patch") + if not isinstance(name, str) or not isinstance(patch, str) or name in by_name: return None - rows.append( - json.dumps( - [f["filename"], f["status"], f.get("previous_filename"), f["patch"]] - ) - ) - return sorted(rows) + by_name[name] = (f.get("status"), f.get("previous_filename"), patch) + return by_name - then = net(decided) - return then is not None and then == net(current) + then, now = net(decided), net(current) + return then is not None and now is not None and then == now -def _carries( - repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any], on_pull: bool -) -> bool: +def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bool: """Whether the decision `comment` covers the pull request at its head - (owner decisions Q17 c and Q18 d): its decided head (`decided_head`; for - a decision on the pull request with no marker before it, `pushed_head` on - its branch) is the head, or every change since it is a merge of the base, - so the net change against the base is the same at both - (`same_net_change`). A read that fails carries nothing.""" - head, base = json_object(pr.get("head")), json_object(pr.get("base")) - sha, base_sha = str(head.get("sha") or ""), str(base.get("sha") or "") - if not COMMIT_SHA.fullmatch(sha) or not COMMIT_SHA.fullmatch(base_sha): - return False + (owner decisions Q17 c and Q18 d on #9436): its decided head + (`decided_head`) is the head, with no compare read, or every change since + is a merge of the base branch, so the net change against it is the same at + both (`same_net_change`). No marker, an unusable base name or a read that + fails carries nothing.""" + sha = str(json_object(pr.get("head")).get("sha") or "") + base = json_object(pr.get("base")).get("ref") owner, name = _split_repo(repo) - at = comment.get("created_at") try: decided = decided_head( _paginated( f"repos/{owner}/{name}/issues/{pr.get('number')}/comments?per_page=100", "cost hold markers", ), - at, + comment.get("created_at"), ) - branch = head.get("ref") - same_repo = ( - str(json_object(head.get("repo")).get("full_name")).casefold() - == repo.casefold() - ) - if decided is None and on_pull and same_repo and isinstance(branch, str) and branch: - ref = quote(f"refs/heads/{branch}", safe="") - decided = pushed_head( - _paginated( - f"repos/{owner}/{name}/activity?ref={ref}&per_page=100", - "head pushes", - ), - at, - ) except PolicyReadError: return False if decided is None: return False if decided == sha: return True + if not isinstance(base, str) or not BASE_BRANCH.fullmatch(base): + return False reads = [ - _get(f"repos/{owner}/{name}/compare/{base_sha}...{each}") - for each in (decided, sha) + _get(f"repos/{owner}/{name}/compare/{base}...{each}") for each in (decided, sha) ] if any(status != 200 for status, _ in reads): return False @@ -2146,26 +2104,28 @@ def cost_decision( agent_identity: str | None, self_logins: Collection[str] = frozenset(), ) -> str | None: - """The first comment link in the body that is an owner decision covering - this pull request, else None: a comment on the pull request itself or on - `issue` (its linked issue, `_linked_issue`) in `repo`, by a User with - admin permission who is not the agent identity, not posted through an - App, never edited, and whose first line starts `**Decision recorded.** - Cost:`. It covers the head it decided and a later head only when every - change since is a base merge (`_carries`, owner decisions Q17 c and Q18 d - on #9436). One on the pull request counts as it is; - one on the issue counts only when its text names this pull request - (`names_pull`) and it was created after the pull request was, so a - decision recorded on the issue for another change does not cover a pull - request that links the issue later (an author can forge commit dates, - not these). A link inside an HTML comment, and a comment by a - `self_logins` account, count for nothing. A read that fails clears - nothing.""" + """The comment link in the body that is an owner decision covering this + pull request at its head, else None: a comment on the pull request itself + or on `issue` (its linked issue, `_linked_issue`) in `repo`, by a User + with admin permission who is not the agent identity, not posted through + an App, never edited, and whose first line starts `**Decision recorded.** + Cost:`. One on the pull request counts as it is; one on the issue counts + only when its text names this pull request (`names_pull`) and it was + created after the pull request was, so a decision recorded on the issue + for another change does not cover a pull request that links the issue + later (an author can forge commit dates, not these). Of those, the + decision is the newest on the pull request, else the newest on the issue, + as the review lanes' hold step picks it; it covers the head it decided and + a later head only when every change since is a base merge (`_carries`, + owner decisions Q17 c and Q18 d on #9436). A link inside an HTML comment, + and a comment by a `self_logins` account, count for nothing. A read that + fails clears nothing.""" owner, name = _split_repo(repo) covered = {pr.get("number"), issue} - {None} opened = parse_timestamp(pr.get("created_at")) excluded = {_login_key(agent_identity)} | {_login_key(x) for x in self_logins} body = HTML_COMMENT.sub("", str(pr.get("body") or "")) + decisions: list[tuple[bool, str, dict[str, Any]]] = [] for link in COST_DECISION_LINK.finditer(body): if f"{link[1]}/{link[2]}".casefold() != repo.casefold(): continue @@ -2204,9 +2164,16 @@ def cost_decision( is_json_object(node) and "lastEditedAt" in node and node["lastEditedAt"] is None - and _carries(repo, pr, comment, int(link[3]) == pr.get("number")) ): - return link[0] + decisions.append((int(link[3]) == pr.get("number"), link[0], comment)) + for own in (True, False): + mine = [d for d in decisions if d[0] is own] + if mine: + _, link, comment = max( + mine, + key=lambda d: parse_timestamp(d[2].get("created_at")) or _NEVER, + ) + return link if _carries(repo, pr, comment) else None return None diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py index 24221db556..c4d3ed45c7 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py @@ -21,14 +21,14 @@ IDENTITY = "agent-lanes[bot]" PR_CREATED = "2026-10-22T10:00:00Z" HEAD = "0123456789abcdef0123456789abcdef01234567" -BASE = "89abcdef0123456789abcdef0123456789abcdef" +BASE = "main" OLD = "fedcba9876543210fedcba9876543210fedcba98" LANE = {"login": "github-actions[bot]", "type": "Bot"} PULL = { "number": 99, "created_at": PR_CREATED, "head": {"sha": HEAD, "ref": "fix/cost", "repo": {"full_name": REPO}}, - "base": {"sha": BASE}, + "base": {"sha": "89abcdef0123456789abcdef0123456789abcdef", "ref": BASE}, } TF = {"filename": "infra/main.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} @@ -71,7 +71,6 @@ def decide( node: Any = ..., created: Any = None, markers: list[dict[str, Any]] | None = None, - pushes: list[dict[str, Any]] | None = None, compares: dict[str, Any] | None = None, unreadable: str = "", pull: dict[str, Any] | None = None, @@ -93,9 +92,6 @@ def paginated(endpoint: str, label: str) -> list[dict[str, Any]]: self.reads.append(endpoint) if label == unreadable: raise policy.PolicyReadError(label) - if "/activity?" in endpoint: - self.assertIn("ref=refs%2Fheads%2Ffix%2Fcost&", endpoint) - return pushes or [] self.assertEqual(endpoint, "repos/owner/repo/issues/99/comments?per_page=100") return [_marker(HEAD)] if markers is None else markers @@ -311,6 +307,11 @@ def test_the_decided_head_is_the_latest_bot_marker_before_the_decision( for name, extra in { "after": _marker(HEAD, "2026-10-22T12:30:00Z"), "a person": _marker(HEAD, "2026-10-22T11:30:00Z", user={"type": "User"}), + "another bot": _marker( + HEAD, "2026-10-22T11:30:00Z", user={"login": "claude[bot]", "type": "Bot"} + ), + "not on the first line": _marker(HEAD, "2026-10-22T11:30:00Z") + | {"body": f"Held.\n"}, "not cost": _marker(HEAD, "2026-10-22T11:30:00Z", criteria="trust-gate"), }.items(): with self.subTest(name): @@ -320,34 +321,81 @@ def test_the_decided_head_is_the_latest_bot_marker_before_the_decision( both = _marker(HEAD, "2026-10-22T11:30:00Z", criteria="cost,trust-gate") self.assertEqual(self.decide(markers=[_marker(OLD), both], compares=changed), LINK) - def test_with_no_marker_a_pull_request_decision_covers_the_last_push_before_it( + def test_with_no_marker_before_the_decision_nothing_carries(self) -> None: + link = "https://github.com/owner/repo/pull/99#issuecomment-7" + own = _comment(issue_url="https://api.github.com/repos/owner/repo/issues/99") + for body, comment in ((link, own), (LINK, None)): + with self.subTest(body): + self.assertIsNone(self.decide(body=body, comment=comment, markers=[])) + self.assertFalse(any("/compare/" in r for r in self.reads)) + + def test_at_the_decided_head_it_carries_with_no_compare_read(self) -> None: + pull = PULL | {"base": {"ref": "-unusable"}} + self.assertEqual(self.decide(pull=pull), LINK) + self.assertFalse(any("/compare/" in r for r in self.reads)) + + def test_a_duplicate_filename_a_pure_rename_or_an_unusable_base_does_not_carry( self, ) -> None: - link = "https://github.com/owner/repo/pull/99#issuecomment-7" - comment = _comment(issue_url="https://api.github.com/repos/owner/repo/issues/99") - added = {"filename": "b.tf", "status": "added", "patch": "+b"} - changed = {OLD: _net(TF), HEAD: _net(TF, added)} - - def push(after: str, at: str) -> dict[str, Any]: - return {"activity_type": "push", "after": after, "timestamp": at} - - def pushed(pushes: list[dict[str, Any]], **change: Any) -> str | None: - return self.decide( - body=link, comment=comment, markers=[], pushes=pushes, compares=changed, **change - ) + self.assertIsNone(self.later({OLD: _net(TF, TF), HEAD: _net(TF, TF)})) + moved = {"filename": "b.tf", "status": "renamed", "previous_filename": "a.tf"} + self.assertIsNone(self.later({OLD: _net(moved), HEAD: _net(moved)})) + same = {OLD: _net(TF), HEAD: _net(TF)} + for base in ("-main", "a..b", "a b", "", "x" * 256, None): + with self.subTest(base): + pull = PULL | {"base": {"ref": base}} + self.assertIsNone(self.later(same, pull=pull)) + self.assertFalse(any("/compare/" in r for r in self.reads)) + pull = PULL | {"base": {"ref": "release/2.x"}} + self.assertEqual(self.later(same, pull=pull), LINK) + self.assertIn(f"repos/owner/repo/compare/release/2.x...{OLD}", self.reads) + + def test_the_decision_is_the_newest_on_the_pull_request_else_the_issue( + self, + ) -> None: + def on_pull(n: int) -> str: + return f"https://github.com/owner/repo/pull/99#issuecomment-{n}" + + rows = { + "7": _comment(created_at="2026-10-22T12:00:00Z"), + "8": _comment( + issue_url="https://api.github.com/repos/owner/repo/issues/99", + created_at="2026-10-22T11:00:00Z", + ), + "9": _comment( + issue_url="https://api.github.com/repos/owner/repo/issues/99", + created_at="2026-10-22T11:30:00Z", + ), + } + changed = {OLD: _net(TF), HEAD: _net(TF, {"filename": "b.tf", "status": "added", "patch": "+b"})} + + def pick(body: str, markers: list[dict[str, Any]]) -> str | None: + def get(endpoint: str) -> tuple[int | None, Any]: + if "/compare/" in endpoint: + sha = endpoint.rsplit("...", 1)[1] + return 200, changed[sha] + return 200, rows[endpoint.rsplit("/", 1)[1]] + + with ( + mock.patch.object(policy, "_get", side_effect=get), + mock.patch.object(policy, "_paginated", return_value=markers), + mock.patch.object( + policy, "_graphql", return_value={"node": {"lastEditedAt": None}} + ), + ): + return policy.cost_decision( + REPO, + PULL | {"body": body}, + 40, + permission=lambda login: "admin", + agent_identity=IDENTITY, + ) - before = push(HEAD, "2026-10-22T11:59:00Z") - self.assertEqual(pushed([before, push(OLD, "2026-10-22T11:00:00Z")]), link) - after = push(HEAD, "2026-10-22T12:01:00Z") - self.assertIsNone(pushed([after, push(OLD, "2026-10-22T11:00:00Z")])) - self.assertIsNone(pushed([after])) - self.assertIsNone(pushed([before], unreadable="head pushes")) - fork = PULL | {"head": PULL["head"] | {"repo": {"full_name": "fork/repo"}}} - self.assertIsNone(pushed([before], pull=fork)) - self.assertFalse(any("/activity?" in r for r in self.reads)) - # An issue decision with no marker carries nothing and reads no pushes. - self.assertIsNone(self.decide(markers=[], pushes=[before])) - self.assertFalse(any("/activity?" in r for r in self.reads)) + every = f"Closes #40\n{LINK}\n{on_pull(8)}\n{on_pull(9)}" + self.assertEqual(pick(every, [_marker(HEAD, "2026-10-22T11:15:00Z")]), on_pull(9)) + older_head = [_marker(HEAD, "2026-10-22T10:00:00Z"), _marker(OLD, "2026-10-22T11:15:00Z")] + self.assertIsNone(pick(every, older_head)) + self.assertEqual(pick(f"Closes #40\n{LINK}", [_marker(HEAD)]), LINK) def test_a_failed_permission_read_clears_nothing(self) -> None: def refuse(login: str) -> str: 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 fc57e04e6a..8bb3eb4fb7 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 @@ -23,7 +23,7 @@ place of an admin, a linked-issue comment counting only when it names the pull request as `#` (`namesPull`, `names_pull`), and both carry it to the same heads: the decided head, or a later one with the same net - change against the base (`decidedHead`, `pushedHead`, `sameNetChange`). + change against the base branch (`decidedHead`, `sameNetChange`). """ from __future__ import annotations @@ -318,7 +318,7 @@ def py_manifest_changes(change: dict[str, Any]) -> list[str]: "number": 7, "created_at": "2026-01-02T00:00:00Z", "head": {"sha": HEAD, "ref": "b", "repo": {"full_name": REPO}}, - "base": {"sha": BASE}, + "base": {"sha": BASE, "ref": "main"}, } ISSUE = 5 OWNER = {"login": "owner", "id": 1, "type": "User"} @@ -395,12 +395,13 @@ def _link(target: int, kind: str = "pull", repo: str = REPO) -> str: } def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[str, Any]: body = f"\nHeld." - return {"user": {"type": "Bot"}, "created_at": at, "body": body} | fields + user = {"login": "github-actions[bot]", "type": "Bot"} + return {"user": user, "created_at": at, "body": body} | fields # The head reads: the pull request's comments (a cost hold marker at the head -# before the decision by default), its branch's pushes, a compare per head. -DEFAULT_HEADS = {"markers": [_marker(HEAD)], "pushes": [], "compares": {}} +# before the decision by default) and a compare per head. +DEFAULT_HEADS = {"markers": [_marker(HEAD)], "compares": {}} TF = {"filename": "a.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} # Each case: the heads world for a decision on the pull request (_link(7)). @@ -439,17 +440,20 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s }, "a marker after it": {"markers": [_marker(HEAD, "2026-01-04T00:00:00Z")]}, "a person's marker": {"markers": [_marker(HEAD, user={"type": "User"})]}, - "no marker, pushed at the head before it": { - "markers": [], - "pushes": [{"after": HEAD, "timestamp": "2026-01-02T20:00:00Z"}], + "another bot's marker": { + "markers": [_marker(HEAD, user={"login": "claude[bot]", "type": "Bot"})] }, - "no marker, pushed at the head after it": { - "markers": [], - "pushes": [ - {"after": HEAD, "timestamp": "2026-01-03T01:00:00Z"}, - {"after": OLD, "timestamp": "2026-01-02T20:00:00Z"}, - ], - "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, + "no marker": {"markers": []}, + "a filename listed twice": { + "markers": [_marker(OLD)], + "compares": {OLD: {"files": [TF, TF]}, HEAD: {"files": [TF, TF]}}, + }, + "a patchless pure rename": { + "markers": [_marker(OLD)], + "compares": { + sha: {"files": [{"filename": "b", "status": "renamed", "previous_filename": "a"}]} + for sha in (OLD, HEAD) + }, }, } NODE_DECISION = """ @@ -462,22 +466,21 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s return heads.compares[sha]; } if (method === "GET" && path === "/repos/o/r/issues/7/comments?per_page=100&page=1") return heads.markers; - if (method === "GET" && path === "/repos/o/r/activity?ref=refs%%2Fheads%%2Fb&per_page=100&page=1") return heads.pushes; if (method === "GET") return comment; if (data.variables.number !== undefined) { - return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", headRefName: "b", isCrossRepository: false, + return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", baseRefName: "main", closingIssuesReferences: { totalCount: 1, nodes: [{ number: issue, repository: { nameWithOwner: "o/r" } }] } } } } }; } return { data: { node: { lastEditedAt: comment.edited } } }; }; -console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %s, %s))); +console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %s))); """ def js_decision( body: str, comment: dict[str, Any], heads: dict[str, Any] = DEFAULT_HEADS ) -> str | None: - script = NODE_DECISION % (json.dumps(MODULE.as_uri()), json.dumps(HEAD), json.dumps(BASE)) + script = NODE_DECISION % (json.dumps(MODULE.as_uri()), json.dumps(HEAD)) proc = subprocess.run( [ "node", @@ -508,8 +511,6 @@ def get(endpoint: str) -> tuple[int | None, Any]: return 200, comment def paginated(endpoint: str, _label: str) -> list[dict[str, Any]]: - if endpoint == "repos/o/r/activity?ref=refs%2Fheads%2Fb&per_page=100": - return heads["pushes"] assert endpoint == "repos/o/r/issues/7/comments?per_page=100", endpoint return heads["markers"] @@ -553,13 +554,11 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: for case in ( "a marker at the head", "a base merge since the decided head", - "no marker, pushed at the head before it", ): self.assertEqual(answers[case], _link(7), case) for case in set(CARRIES) - { "a marker at the head", "a base merge since the decided head", - "no marker, pushed at the head before it", }: self.assertIsNone(answers[case], case) diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index 2ec0914f9c..4d8ad43d0d 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -10,11 +10,12 @@ # `#`, on its linked issue: the one issue GitHub records it as closing, # else, when it closes none, the one plain `Refs:` issue (the base's # merge-holds.mjs refsIssue). A second closing issue counts for nothing. The -# decision covers the head it decided (the latest review-lane cost hold marker -# before it, else, for one on the pull request, the last push before it) and a -# later head only when the net change against the base is the same at both -# (owner decisions Q17 c and Q18 d on #9436). The holds are read from the -# lanes' check-run notices. +# decision, the newest on the pull request else the newest on the issue, +# covers the head it decided (the newest github-actions[bot] cost hold marker +# before it; with none, no head) and a later head only when the net change +# against the base branch is the same at both (owner decisions Q17 c and Q18 d +# on #9436, as ci-workflows v0.65.4's hold step reads them). The holds are +# read from the lanes' check-run notices. # The step then outputs cleared=true and the decision, and takes the label off # with one comment citing it. Anything else, an unreadable read included, # clears nothing and leaves the hold to the steps after it. @@ -107,7 +108,7 @@ case "$*" in from "$STUB_RUNS" "$(key workflows "$@")" ;; */compare/"$BASE_SHA..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; -*/activity\?ref=refs%2Fheads%2Ffix%2Fcost\&per_page=100*) failing activity; printf '%s\n' "$STUB_ACTIVITY" ;; +*/compare/"${STUB_BASE_REF:-main}..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; */actions/runs/*/jobs*) failing jobs; from "$STUB_JOBS" "$(key runs "$@")" ;; */check-runs/*/annotations*) failing notes; from "$STUB_NOTES" "$(key check-runs "$@")" ;; */pulls/6419*) failing pull; printf '%s\n' "$STUB_PR" ;; @@ -164,7 +165,7 @@ lanes() { reset() { export STUB_FAIL='' STUB_LABELS='needs-human' LANES_APP_ID="$APP_ID" export STUB_ACTORS="{\"actors\":[{\"kind\":\"human\",\"id\":$OWNER_ID},{\"kind\":\"bot\",\"id\":5}]}" - export STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\"},\"body\":\"Closes #9306\\n\\n- Cost decision: [Q50]($LINK)\"}" + export STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\"},\"base\":{\"ref\":\"main\"},\"body\":\"Closes #9306\\n\\n- Cost decision: [Q50]($LINK)\"}" export STUB_CLOSING='{"data":{"repository":{"pullRequest":{"closingIssuesReferences":{"totalCount":1,"nodes":[{"number":9306,"repository":{"nameWithOwner":"o/r"}}]}}}}}' STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" export STUB_DECISIONS @@ -175,7 +176,6 @@ reset() { STUB_COMMENTS="[$MARKER,$(comment "$OWNER" 2026-10-23T05:40:00Z 'A comment.')]" export STUB_EVENTS STUB_COMMENTS lanes "[$(hold_note cost),$(note "claude-lane-review-complete lane=claude-review sha=$HEAD findings=0")]" '[]' - export STUB_ACTIVITY='[]' unset STUB_TRACKER } # The owner lifted an earlier hold at 13:15, so the 05:39 escalation is outside the window. @@ -269,19 +269,11 @@ reset STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T04:00:00Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" expect "an issue decision older than the pull request clears nothing" no no no STUB_DECISIONS="$(jq -cn --argjson c "$(decision 6419 "$OWNER" 2026-10-23T04:00:00Z '**Decision recorded.** Cost: accepted.')" '{"111": $c}')" -STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","repo":{"full_name":"o/r"}},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' -export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" -WANT_LINK=https://github.com/o/r/pull/6419#issuecomment-111 expect "a decision on the pull request itself, with no marker before it, clears the hold for the head pushed before it" yes yes yes -export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T04:30:00Z\"},{\"activity_type\":\"push\",\"after\":\"$OLD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" -nets "[$TF]" "[$TF,$MORE]" -expect "a decision on the pull request with no marker, pushed over with a change of its own, clears nothing" no no no "covering this head" -export STUB_FAIL=activity -expect "a decision on the pull request with no marker and unreadable pushes clears nothing" no no no "covering this head" -export STUB_FAIL='' -STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","repo":{"full_name":"fork/r"}},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' -export STUB_ACTIVITY="[{\"activity_type\":\"push\",\"after\":\"$HEAD\",\"timestamp\":\"2026-10-23T03:00:00Z\"}]" -expect "a decision on a pull request from a fork with no marker clears nothing" no no no "covering this head" -if grep -q '/activity' "$TMP_ROOT/log"; then fail "a fork's pushes were read from the base repository"; else ok "a fork's pushes are not read"; fi +STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost"},"base":{"ref":"main"},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T03:00:00Z)]" +WANT_LINK=https://github.com/o/r/pull/6419#issuecomment-111 expect "a decision on the pull request itself clears the hold" yes yes yes +STUB_COMMENTS="[$MARKER]" +expect "with no cost hold marker before the decision nothing carries, even at the same head" no no no "covering this head" reset STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$STRANGER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" expect "a decision by someone off the trusted-actor list clears nothing" no no no @@ -360,7 +352,7 @@ reset STUB_COMMENTS="[$(marker "$OLD")]" nets "[$TF]" "[$TF]" expect "a base merge after the decided head carries the decision" yes yes yes -if grep -q "compare/$BASE...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base"; else fail "no compare at the decided head"; fi +if grep -q "compare/main...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base branch"; else fail "no compare at the decided head"; fi nets "[$TF]" "[$TF,$MORE]" expect "a pull request commit after the decided head clears nothing" no no no "covering this head" nets "[$TF,$MORE]" "[$MORE,$TF]" @@ -395,6 +387,42 @@ expect "an issue decision with no marker before it clears nothing" no no no "cov reset export STUB_FAIL=module expect "an unreadable base merge-holds.mjs carries no closing-issue decision" no no no "covering this head" +reset +expect "at the decided head the decision carries" yes yes yes +if grep -q 'compare/main\.\.\.' "$TMP_ROOT/log"; then fail "a decision at its own head read a compare"; else ok "a decision at its own head reads no compare"; fi +STUB_COMMENTS="[$(marker "$OLD")]" +nets "[$TF,$TF]" "[$TF,$TF]" +expect "a filename listed twice carries nothing" no no no "covering this head" +nets '[{"filename":"b.tf","status":"renamed","previous_filename":"a.tf"}]' '[{"filename":"b.tf","status":"renamed","previous_filename":"a.tf"}]' +expect "a patchless pure rename carries nothing" no no no "covering this head" +nets "[$TF]" "[$TF]" +for base in '-main' 'a..b' 'a b' ''; do + STUB_PR="$(jq -c --arg b "$base" '.base.ref = $b' <<<"$STUB_PR")" + expect "a base branch named '$base' carries nothing" no no no "covering this head" + if grep -q '/compare/[^0-9]' "$TMP_ROOT/log"; then fail "an unusable base name '$base' was compared"; else ok "an unusable base name '$base' is never compared"; fi +done +STUB_PR="$(jq -c '.base.ref = "release/2.x"' <<<"$STUB_PR")" +STUB_BASE_REF=release/2.x expect "a base merge on a base branch with a slash carries" yes yes yes +reset +STUB_COMMENTS="[$(marker "$OLD"),$(comment '{"login":"claude[bot]","id":9,"type":"Bot"}' 2026-10-23T13:12:00Z "")]" +nets "[$TF]" "[$TF,$MORE]" +expect "a marker another bot wrote decides nothing" no no no "covering this head" +# The decision is the newest on the pull request, else the newest on the issue. +reset +PULL_LINK=https://github.com/o/r/pull/6419#issuecomment +STUB_PR="$(jq -c --arg body "Closes #9306 +$LINK +$PULL_LINK-112 +$PULL_LINK-113" '.body = $body' <<<"$STUB_PR")" +STUB_DECISIONS="$(jq -cn --argjson a "$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" \ + --argjson b "$(decision 6419 "$OWNER" 2026-10-23T13:00:00Z '**Decision recorded.** Cost: accepted.')" \ + --argjson c "$(decision 6419 "$OWNER" 2026-10-23T13:12:00Z '**Decision recorded.** Cost: accepted.')" '{"111": $a, "112": $b, "113": $c}')" +export STUB_DECISIONS +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T13:11:00Z)]" +WANT_LINK=$PULL_LINK-113 expect "the newest decision on the pull request is judged over a newer one on the issue" yes yes yes +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T12:00:00Z),$(marker "$OLD" 2026-10-23T13:11:00Z)]" +nets "[$TF]" "[$TF,$MORE]" +expect "only the chosen decision may carry, so an older one at this head does not" no no no "covering this head" reset lanes "[$(hold_note cost,trust-gate)]" '[]' From 1ef578728cfb616a876ed6aba2ddae5a0098eedc Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 02:56:06 -0400 Subject: [PATCH 5/9] fix(pr-merge): count a cost hold marker only while it was never edited A write collaborator can edit a github-actions[bot] hold comment and move its sha= to the current head, which would turn the carry into a cost bypass. In all three readers the newest cost hold marker before the decision now decides a head only when its GraphQL lastEditedAt is null, read as the decision's edit check reads it. An edited marker, or one whose edit state is unreadable, decides no head, and no older marker stands in, so nothing carries. merge-holds.mjs exports costMarker ({sha, node}) in place of decidedHead; babysit_merge_policy has cost_marker in place of decided_head. Co-Authored-By: Claude Opus 5.5 --- .../agent-a5e09147a92521857-c3fc6a4d.md | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 39 ++++++++++++------- .github/scripts/pr-merge/merge-holds.test.mjs | 39 +++++++++++++++---- .github/workflows/pr-require-checks.yml | 26 ++++++++----- .../scripts/babysit_merge_policy.py | 36 ++++++++++------- .../scripts/tests/test_babysit_merge_cost.py | 33 ++++++++++++++-- .../scripts/tests/test_merge_holds_sync.py | 22 +++++++++-- scripts/ci-hold-cost-decision.test.sh | 26 +++++++++++-- 8 files changed, 168 insertions(+), 55 deletions(-) diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md index 069c2f5e83..acede92f54 100644 --- a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md +++ b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `cost_decision` binds an owner cost decision to one pull request and the head it decided. A `**Decision recorded.** Cost:` comment counts on the pull request, or on its linked issue (closing, else the one `Refs:` issue) when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`); the newest on the pull request, else the newest on the issue, is the decision. It covers the head of the newest github-actions[bot] cost hold marker before it, and a later head only when every change since is a merge of the base: the pull request's net change against its base branch, file by file, is the same at both. With no marker before it, it covers no head. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. +- `cost_decision` binds an owner cost decision to one pull request and the head it decided. A `**Decision recorded.** Cost:` comment counts on the pull request, or on its linked issue (closing, else the one `Refs:` issue) when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`); the newest on the pull request, else the newest on the issue, is the decision. It covers the head of the newest github-actions[bot] cost hold marker before it while that marker was never edited (an edited or unreadable one covers no head, and no older marker stands in), and a later head only when every change since is a merge of the base: the pull request's net change against its base branch, file by file, is the same at both. With no marker before it, it covers no head. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 52b49fc7e6..61a53942a1 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -444,11 +444,13 @@ export function namesPull(text, number) { const COST_MARKER = /^(?:\n|$)/; const BRANCH = /^(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}$/; -// The head a cost decision made at `at` decided, from the pull request's -// `comments`: the sha of the newest cost hold marker github-actions[bot] -// posted before it, else null (babysit_merge_policy.decided_head; the first -// of two at one time wins, as in the review lanes' hold step). -export function decidedHead(comments, at) { +// The newest cost hold marker github-actions[bot] posted among the pull +// request's `comments` before a decision made at `at`, as { sha, node } +// (the head it held and its comment's node id), else null +// (babysit_merge_policy.cost_marker; the first of two at one time wins, as +// in the review lanes' hold step). It decides a head only while never +// edited, which the caller reads. +export function costMarker(comments, at) { const cutoff = Date.parse(at); let found = null; let latest = -Infinity; @@ -456,13 +458,16 @@ export function decidedHead(comments, at) { const marker = row?.user?.login === "github-actions[bot]" ? COST_MARKER.exec(String(row.body ?? "")) : null; const made = Date.parse(row?.created_at); if (marker && marker[2].split(",").includes("cost") && made < cutoff && made > latest) { - found = marker[1]; + found = { sha: marker[1], node: String(row.node_id ?? "") }; latest = made; } } return found; } +// Whether a COMMENT_EDITED_QUERY answer's node was never edited. +const unedited = (node) => Boolean(node) && "lastEditedAt" in node && node.lastEditedAt === null; + // Whether two compare answers (base...head) show the same net change of the // pull request: the same filenames, each with the same status, // previous_filename and patch. A list of 300 files or more (the compare @@ -499,17 +504,23 @@ async function pages(github, path) { } // Whether the decision `comment` covers pull request `number` at `head` -// (owner decisions Q17 c and Q18 d on #9436): its decided head (decidedHead) -// is `head`, with no compare read, or every change since is a merge of the -// base branch `base`, so the net change against it is the same at both -// (sameNetChange). No marker, an unusable base name or a read that fails -// carries nothing. +// (owner decisions Q17 c and Q18 d on #9436): its decided head (costMarker, +// never edited, so no one with write access moved its sha) is `head`, with +// no compare read, or every change since is a merge of the base branch +// `base`, so the net change against it is the same at both (sameNetChange). +// No marker, an edited or unreadable newest marker (no older one stands in), +// an unusable base name or a read that fails carries nothing. async function carries(github, repo, number, comment, head, base) { try { - const decided = decidedHead(await pages(github, `/repos/${repo}/issues/${number}/comments`), comment.created_at); - if (decided === null) { + const marker = costMarker(await pages(github, `/repos/${repo}/issues/${number}/comments`), comment.created_at); + if (marker === null) { + return false; + } + const node = (await github("POST", "/graphql", { query: COMMENT_EDITED_QUERY, variables: { id: marker.node } }))?.data?.node; + if (!unedited(node)) { return false; } + const decided = marker.sha; if (decided === head) { return true; } @@ -594,7 +605,7 @@ export async function costDecision(github, repo, number, body, humans, head) { continue; } const node = (await github("POST", "/graphql", { query: COMMENT_EDITED_QUERY, variables: { id: String(comment.node_id) } }))?.data?.node; - if (node && "lastEditedAt" in node && node.lastEditedAt === null) { + if (unedited(node)) { decisions.push({ link: link[0], comment, own: target === number }); } } catch { diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 22e369b8f3..c9932cb40b 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -23,7 +23,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, decidedHead, linkHold, main, sameNetChange } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, costMarker, linkHold, main, sameNetChange } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -34,8 +34,10 @@ const BASE = "89abcdef0123456789abcdef0123456789abcdef"; const OLD = "fedcba9876543210fedcba9876543210fedcba98"; const LANE = { login: "github-actions[bot]", id: 41898282, type: "Bot" }; // A review lane's cost hold comment on the pull request at `sha`. +// Its node id IC_M answers the marker's edit read. const marker = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", user = LANE } = {}) => ({ user, + node_id: "IC_M", created_at: at, body: `\nHeld for a person.\n\nclaude-lane: claude-review`, }); @@ -68,6 +70,7 @@ function world({ comments = [marker(HEAD)], compares = {}, base = "main", + markerNode, } = {}) { const reads = []; const github = async (method, path, body) => { @@ -76,7 +79,9 @@ function world({ body?.query === PULL_OPENED_QUERY ? "pull" : body?.query === COMMENT_EDITED_QUERY - ? "edited" + ? body.variables.id === "IC_M" + ? "marker edited" + : "edited" : path === "/repos/owner/repo/issues/99/comments?per_page=100&page=1" ? "comments" : compare @@ -106,6 +111,9 @@ function world({ }, }; } + if (read === "marker edited") { + return { data: { node: markerNode === undefined ? { lastEditedAt: null } : markerNode } }; + } if (read === "edited") { assert.deepEqual(body.variables, { id: "IC_7" }); return { data: { node: node === undefined ? { lastEditedAt: null } : node } }; @@ -127,7 +135,7 @@ const decide = (body = `Closes #40\n\nCost decision: ${LINK}\n`, options = {}) = test("an owner decision on the closing issue, linked from the body, clears the cost hold", async () => { const { link, reads } = await decide(); assert.equal(link, LINK); - assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited", "comments"]); + assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited", "comments", "marker edited"]); }); test("one on the pull request itself clears with no check against the pull request's time and no #number", async () => { @@ -377,7 +385,7 @@ test("a base merge after the decision carries it, and a pull request commit afte assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) })).link, LINK); const commit = await later({ [OLD]: net(TF), [HEAD]: net(TF, { filename: "scripts/run.sh", status: "modified", patch: "@@ -1 +1 @@\n-x\n+y" }) }); assert.equal(commit.link, null); - assert.deepEqual(commit.reads.slice(-3), ["comments", `compare ${OLD}`, `compare ${HEAD}`]); + assert.deepEqual(commit.reads.slice(-4), ["comments", "marker edited", `compare ${OLD}`, `compare ${HEAD}`]); }); test("a rebase onto a newer base with the same net change carries, whatever order the files list in", async () => { @@ -418,6 +426,23 @@ test("a compare of 300 files or more, a file with no patch, a filename listed tw assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base: "release/2.x" })).link, LINK); }); +test("an edited or unreadable newest marker carries nothing, and no older marker stands in", async () => { + const same = { [OLD]: net(TF), [HEAD]: net(TF) }; + for (const [name, options] of Object.entries({ + edited: { markerNode: { lastEditedAt: "2026-10-22T11:05:00Z" } }, + "a null node": { markerNode: null }, + "no lastEditedAt key": { markerNode: {} }, + unreadable: { fail: "marker edited" }, + })) { + // At the head it held, and after a base merge. + assert.equal((await decide(undefined, options)).link, null, name); + assert.equal((await later(same, options)).link, null, name); + } + // An older unedited marker at this head does not stand in for an edited newest one. + const older = { comments: [marker(HEAD, "2026-10-22T10:00:00Z"), marker(OLD)], compares: same, markerNode: { lastEditedAt: "2026-10-22T11:05:00Z" } }; + assert.equal((await decide(undefined, older)).link, null); +}); + test("at the decided head the decision carries with no compare read", async () => { const { link, reads } = await decide(undefined, { base: "" }); assert.equal(link, LINK); @@ -473,10 +498,10 @@ test("the decided head is the latest bot cost hold marker made before the decisi assert.equal((await decide(undefined, { comments: [marker(OLD), extra], compares: same })).link, null, name); } assert.equal((await decide(undefined, { comments: [marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z", { criteria: "cost,trust-gate" })], compares: same })).link, LINK); - assert.equal(decidedHead([marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z")], "2026-10-22T11:30:00Z"), OLD); + assert.deepEqual(costMarker([marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z")], "2026-10-22T11:30:00Z"), { sha: OLD, node: "IC_M" }); // Of two at one time, the first listed wins, as in the hold step. - assert.equal(decidedHead([marker(OLD), marker(HEAD)], "2026-10-22T12:00:00Z"), OLD); - assert.equal(decidedHead([], "2026-10-22T12:00:00Z"), null); + assert.equal(costMarker([marker(OLD), marker(HEAD)], "2026-10-22T12:00:00Z").sha, OLD); + assert.equal(costMarker([], "2026-10-22T12:00:00Z"), null); }); test("sameNetChange compares filename, status, previous_filename and patch", () => { diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 8a67c59ea5..539cd0b0b0 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3563,10 +3563,10 @@ jobs: # pull request and naming it as `#` (merge-holds.mjs # namesPull). Of those, the newest on the pull request, else the # newest on the issue, is the decision. It covers the head it decided - # (the newest github-actions[bot] cost hold marker before it; with - # none, no head) and a later head only when the pull request's net + # (the newest github-actions[bot] cost hold marker before it, while + # never edited; with none, or an edited one, no head) and a later head only when the pull request's net # change against its base branch is the same at both, so every change - # since is a base merge (the base's merge-holds.mjs decidedHead and + # since is a base merge (the base's merge-holds.mjs costMarker and # sameNetChange; owner decisions Q17 c and Q18 d on #9436, as # ci-workflows v0.65.4's hold step reads them). The linked issue is the one issue in this repository that # GitHub records it as closing, else, when it closes none, the one issue @@ -3675,18 +3675,26 @@ jobs: import { pathToFileURL } from "node:url"; const [name, ...files] = process.argv.slice(1, -1); const run = (await import(pathToFileURL(process.argv.at(-1)).href))[name]; - console.log(run(...files.map((file) => JSON.parse(readFileSync(file, "utf8")))) ?? "");' \ + const out = run(...files.map((file) => JSON.parse(readFileSync(file, "utf8")))); + console.log(out !== null && typeof out === "object" ? JSON.stringify(out) : String(out ?? ""));' \ "$@" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs" } # carries : whether the decision comment covers HEAD_SHA: - # its decided head is HEAD_SHA, or the net change against the base - # branch is the same at both. No marker, an unusable base branch - # name or any read that fails carries nothing. + # its decided head, from the newest cost hold marker before it and + # only while that marker was never edited, is HEAD_SHA, or the net + # change against the base branch is the same at both. No marker, an + # edited or unreadable newest marker, an unusable base branch name or + # any read that fails carries nothing. carries() { - local at="$RUNNER_TEMP/cost-hold-at.json" decided base_ref + local at="$RUNNER_TEMP/cost-hold-at.json" marker decided marker_edited base_ref jq '.created_at' <<<"$1" >"$at" || return 1 - decided=$(holds_js decidedHead "$RUNNER_TEMP/cost-hold-comments.json" "$at") || return 1 + marker=$(holds_js costMarker "$RUNNER_TEMP/cost-hold-comments.json" "$at") || return 1 + [ -n "$marker" ] || return 1 + decided=$(jq -r '.sha' <<<"$marker") || return 1 [[ $decided =~ ^[0-9a-f]{40}$ ]] || return 1 + # shellcheck disable=SC2016 # GraphQL variables, not shell + marker_edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$(jq -r '.node' <<<"$marker")") || return 1 + jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$marker_edited" >/dev/null 2>&1 || return 1 [ "$decided" != "$HEAD_SHA" ] || return 0 base_ref=$(jq -r '.base.ref // ""' <<<"$pr") || return 1 jq -en --arg b "$base_ref" '$b | test("^(?!-)(?!.*\\.\\.)[A-Za-z0-9._/-]{1,255}$")' >/dev/null || return 1 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 a1926fd792..b863cf76bd 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 @@ -2009,14 +2009,14 @@ def _after(value: Any, opened: datetime | None) -> bool: _NEVER = datetime.min.replace(tzinfo=UTC) -def decided_head(comments: Any, at: Any) -> str | None: - """The head a cost decision made at `at` decided, from the pull - request's `comments`: the sha of the newest cost hold marker - github-actions[bot] posted before it, else None (merge-holds.mjs - `decidedHead`; the first of two at one time wins, as in the review lanes' - hold step).""" +def cost_marker(comments: Any, at: Any) -> tuple[str, str] | None: + """The newest cost hold marker github-actions[bot] posted among the pull + request's `comments` before a decision made at `at`, as (the head it + held, its comment's node id), else None (merge-holds.mjs `costMarker`; + the first of two at one time wins, as in the review lanes' hold step). + It decides a head only while never edited, which the caller reads.""" when = parse_timestamp(at) - found: tuple[datetime, str] | None = None + found: tuple[datetime, str, str] | None = None for row in json_array(comments): row = json_object(row) made = parse_timestamp(row.get("created_at")) @@ -2033,8 +2033,8 @@ def decided_head(comments: Any, at: Any) -> str | None: and made < when and (found is None or made > found[0]) ): - found = (made, marker[1]) - return found[1] if found else None + found = (made, marker[1], str(row.get("node_id") or "")) + return (found[1], found[2]) if found else None def same_net_change(decided: Any, current: Any) -> bool: @@ -2064,25 +2064,31 @@ def net(compare: Any) -> dict[str, tuple[Any, Any, str]] | None: def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bool: """Whether the decision `comment` covers the pull request at its head (owner decisions Q17 c and Q18 d on #9436): its decided head - (`decided_head`) is the head, with no compare read, or every change since - is a merge of the base branch, so the net change against it is the same at - both (`same_net_change`). No marker, an unusable base name or a read that - fails carries nothing.""" + (`cost_marker`, never edited, so no one with write access moved its sha) + is the head, with no compare read, or every change since is a merge of the + base branch, so the net change against it is the same at both + (`same_net_change`). No marker, an edited or unreadable newest marker (no + older one stands in), an unusable base name or a read that fails carries + nothing.""" sha = str(json_object(pr.get("head")).get("sha") or "") base = json_object(pr.get("base")).get("ref") owner, name = _split_repo(repo) try: - decided = decided_head( + marker = cost_marker( _paginated( f"repos/{owner}/{name}/issues/{pr.get('number')}/comments?per_page=100", "cost hold markers", ), comment.get("created_at"), ) + if marker is None: + return False + node = _graphql(COMMENT_EDITED, "cost hold marker", id=marker[1]).get("node") except PolicyReadError: return False - if decided is None: + if not (is_json_object(node) and "lastEditedAt" in node and node["lastEditedAt"] is None): return False + decided = marker[0] if decided == sha: return True if not isinstance(base, str) or not BASE_BRANCH.fullmatch(base): diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py index c4d3ed45c7..5baf46b459 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py @@ -40,7 +40,7 @@ def _marker( user: dict[str, Any] = LANE, ) -> dict[str, Any]: body = f"\nHeld." - return {"user": user, "created_at": at, "body": body} + return {"user": user, "node_id": "IC_M", "created_at": at, "body": body} def _net(*files: dict[str, Any]) -> dict[str, Any]: @@ -74,6 +74,7 @@ def decide( compares: dict[str, Any] | None = None, unreadable: str = "", pull: dict[str, Any] | None = None, + marker_node: Any = ..., ) -> str | None: created = PR_CREATED if created is None else created self.reads: list[str] = [] @@ -96,11 +97,20 @@ def paginated(endpoint: str, label: str) -> list[dict[str, Any]]: return [_marker(HEAD)] if markers is None else markers answer = {"lastEditedAt": edited} if node is ... else node - graphql = mock.Mock(return_value={"node": answer}) + held = {"lastEditedAt": None} if marker_node is ... else marker_node + + def graphql(_query: str, what: str, **variables: Any) -> dict[str, Any]: + if variables.get("id") != "IC_M": + return {"node": answer} + self.reads.append("marker edited") + if unreadable == "marker edited": + raise policy.PolicyReadError(what) + return {"node": held} + with ( mock.patch.object(policy, "_get", side_effect=get), mock.patch.object(policy, "_paginated", side_effect=paginated), - mock.patch.object(policy, "_graphql", graphql), + mock.patch.object(policy, "_graphql", side_effect=graphql), ): return policy.cost_decision( REPO, @@ -118,6 +128,7 @@ def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: [ "repos/owner/repo/issues/comments/7", "repos/owner/repo/issues/99/comments?per_page=100", + "marker edited", ], ) @@ -329,6 +340,22 @@ def test_with_no_marker_before_the_decision_nothing_carries(self) -> None: self.assertIsNone(self.decide(body=body, comment=comment, markers=[])) self.assertFalse(any("/compare/" in r for r in self.reads)) + def test_an_edited_or_unreadable_newest_marker_does_not_carry(self) -> None: + same = {OLD: _net(TF), HEAD: _net(TF)} + for name, change in { + "edited": dict(marker_node={"lastEditedAt": "2026-10-22T11:05:00Z"}), + "a null node": dict(marker_node=None), + "no lastEditedAt key": dict(marker_node={}), + "unreadable": dict(unreadable="marker edited"), + }.items(): + with self.subTest(name): + self.assertIsNone(self.decide(**change)) + self.assertIsNone(self.later(same, **change)) + # An older unedited marker at this head does not stand in. + older = [_marker(HEAD, "2026-10-22T10:00:00Z"), _marker(OLD)] + edited = {"lastEditedAt": "2026-10-22T11:05:00Z"} + self.assertIsNone(self.decide(markers=older, compares=same, marker_node=edited)) + def test_at_the_decided_head_it_carries_with_no_compare_read(self) -> None: pull = PULL | {"base": {"ref": "-unusable"}} self.assertEqual(self.decide(pull=pull), LINK) 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 8bb3eb4fb7..e9b2163ce2 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 @@ -396,12 +396,13 @@ def _link(target: int, kind: str = "pull", repo: str = REPO) -> str: def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[str, Any]: body = f"\nHeld." user = {"login": "github-actions[bot]", "type": "Bot"} - return {"user": user, "created_at": at, "body": body} | fields + return {"user": user, "node_id": "M", "created_at": at, "body": body} | fields # The head reads: the pull request's comments (a cost hold marker at the head # before the decision by default) and a compare per head. -DEFAULT_HEADS = {"markers": [_marker(HEAD)], "compares": {}} +# The marker's edit read answers `marker_edited`, or fails when it is "fail". +DEFAULT_HEADS = {"markers": [_marker(HEAD)], "compares": {}, "marker_edited": None} TF = {"filename": "a.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} # Each case: the heads world for a decision on the pull request (_link(7)). @@ -444,6 +445,13 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s "markers": [_marker(HEAD, user={"login": "claude[bot]", "type": "Bot"})] }, "no marker": {"markers": []}, + "an edited marker": {"marker_edited": "2026-01-02T13:00:00Z"}, + "an unreadable marker edit state": {"marker_edited": "fail"}, + "an edited marker after a base merge": { + "markers": [_marker(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF]}}, + "marker_edited": "2026-01-02T13:00:00Z", + }, "a filename listed twice": { "markers": [_marker(OLD)], "compares": {OLD: {"files": [TF, TF]}, HEAD: {"files": [TF, TF]}}, @@ -471,6 +479,10 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", baseRefName: "main", closingIssuesReferences: { totalCount: 1, nodes: [{ number: issue, repository: { nameWithOwner: "o/r" } }] } } } } }; } + if (data.variables.id === "M") { + if (heads.marker_edited === "fail") throw new Error("unreadable"); + return { data: { node: { lastEditedAt: heads.marker_edited } } }; + } return { data: { node: { lastEditedAt: comment.edited } } }; }; console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %s))); @@ -501,7 +513,11 @@ def py_decision( ) -> str | None: heads = DEFAULT_HEADS | heads - def graphql(_query: str, _what: str, **_vars: Any) -> dict[str, Any]: + def graphql(_query: str, what: str, **variables: Any) -> dict[str, Any]: + if variables.get("id") == "M": + if heads["marker_edited"] == "fail": + raise policy.PolicyReadError(what) + return {"node": {"lastEditedAt": heads["marker_edited"]}} return {"node": {"lastEditedAt": comment["edited"]}} def get(endpoint: str) -> tuple[int | None, Any]: diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index 4d8ad43d0d..686c0c64e7 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -114,6 +114,7 @@ case "$*" in */pulls/6419*) failing pull; printf '%s\n' "$STUB_PR" ;; *graphql*COMMENT_DELETED_EVENT*) failing deleted; printf '%s\n' "$STUB_DELETED" ;; *graphql*closingIssuesReferences*) failing closing; printf '%s\n' "$STUB_CLOSING" ;; +*graphql*lastEditedAt*'id=IC_M'*) failing marker-edited; printf '%s\n' "$STUB_MARKER_EDITED" ;; *graphql*lastEditedAt*) failing edited; printf '%s\n' "$STUB_EDITED" ;; *) exit 1 ;; esac @@ -142,9 +143,12 @@ decision() { } note() { jq -cn --arg m "$1" '{annotation_level: "notice", message: $m}'; } hold_note() { note "claude-lane-hold lane=${2:-claude-review} sha=$HEAD criteria=$1 label=needs-human"; } -# marker [at] [criteria]: a review lane's hold comment on the pull request. -marker() { comment "$LANE" "${2:-2026-10-23T13:10:00Z}" " -Held for a person."; } +# marker [at] [criteria]: a review lane's hold comment on the pull +# request, node IC_M, whose edit read answers STUB_MARKER_EDITED. +marker() { + jq -c '.node_id = "IC_M"' <<<"$(comment "$LANE" "${2:-2026-10-23T13:10:00Z}" " +Held for a person.")" +} MARKER="$(marker "$HEAD")" TF='{"filename":"infra/main.tf","status":"modified","patch":"@@ -1 +1 @@\n-a\n+b"}' MORE='{"filename":"scripts/run.sh","status":"modified","patch":"+y"}' @@ -170,6 +174,7 @@ reset() { STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" export STUB_DECISIONS export STUB_EDITED='{"data":{"node":{"lastEditedAt":null}}}' + export STUB_MARKER_EDITED='{"data":{"node":{"lastEditedAt":null}}}' export STUB_ISSUES='{"9306":{"number":9306}}' export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":0,"nodes":[]}}}}}' STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" @@ -388,6 +393,21 @@ reset export STUB_FAIL=module expect "an unreadable base merge-holds.mjs carries no closing-issue decision" no no no "covering this head" reset +export STUB_MARKER_EDITED='{"data":{"node":{"lastEditedAt":"2026-10-23T13:11:00Z"}}}' +expect "an edited newest cost hold marker carries nothing at its own head" no no no "covering this head" +STUB_COMMENTS="[$(marker "$OLD")]" +nets "[$TF]" "[$TF]" +expect "an edited newest cost hold marker carries nothing after a base merge" no no no "covering this head" +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T12:00:00Z),$(marker "$OLD")]" +expect "an older unedited marker does not stand in for an edited newest one" no no no "covering this head" +for answer in '{"data":{"node":null}}' '{"data":{"node":{}}}'; do + export STUB_MARKER_EDITED="$answer" + STUB_COMMENTS="[$MARKER]" + expect "a marker edit state reading '$answer' carries nothing" no no no "covering this head" +done +export STUB_MARKER_EDITED='{"data":{"node":{"lastEditedAt":null}}}' STUB_FAIL=marker-edited +expect "an unreadable marker edit state carries nothing" no no no "covering this head" +reset expect "at the decided head the decision carries" yes yes yes if grep -q 'compare/main\.\.\.' "$TMP_ROOT/log"; then fail "a decision at its own head read a compare"; else ok "a decision at its own head reads no compare"; fi STUB_COMMENTS="[$(marker "$OLD")]" From 7010efcb691c38833187328a538b6712935e910e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:26:23 -0400 Subject: [PATCH 6/9] fix(pr-merge): carry no cost decision past a deleted comment A deleted newer cost hold marker made an older one the decided head, so a head matching that older marker inherited a decision the owner made at a later head. All three readers now refuse to carry when any comment on the pull request was deleted at or after the chosen marker (deletedSince / deleted_since), checked before the same-head shortcut, and fail closed when the deletion list is unreadable or overflows. ci-status's cost-hold step also keeps the hold when the pull request's live head is no longer the event's head, so a late run cannot lift the label off a newer head. costDecision rejects a non-string base name before the compare. Co-Authored-By: Claude Opus 5.5 --- .../agent-a5e09147a92521857-c3fc6a4d.md | 2 +- .../pr-merge/enable-auto-merge.test.mjs | 7 ++- .github/scripts/pr-merge/merge-holds.mjs | 41 +++++++++++-- .github/scripts/pr-merge/merge-holds.test.mjs | 51 ++++++++++++++--- .github/workflows/pr-require-checks.yml | 23 +++++--- .../scripts/babysit_merge_policy.py | 53 ++++++++++++++--- .../scripts/tests/test_babysit_merge_cost.py | 57 ++++++++++++++++++- .../scripts/tests/test_merge_holds_sync.py | 49 ++++++++++++---- scripts/ci-hold-cost-decision.test.sh | 42 ++++++++++---- 9 files changed, 267 insertions(+), 58 deletions(-) diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md index acede92f54..b376f0573b 100644 --- a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md +++ b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `cost_decision` binds an owner cost decision to one pull request and the head it decided. A `**Decision recorded.** Cost:` comment counts on the pull request, or on its linked issue (closing, else the one `Refs:` issue) when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`); the newest on the pull request, else the newest on the issue, is the decision. It covers the head of the newest github-actions[bot] cost hold marker before it while that marker was never edited (an edited or unreadable one covers no head, and no older marker stands in), and a later head only when every change since is a merge of the base: the pull request's net change against its base branch, file by file, is the same at both. With no marker before it, it covers no head. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. +- `cost_decision` binds an owner cost decision to one pull request and the head it decided. A `**Decision recorded.** Cost:` comment counts on the pull request, or on its linked issue (closing, else the one `Refs:` issue) when it names the pull request as `#` (not `owner/repo#`, `&#` or `##`); the newest on the pull request, else the newest on the issue, is the decision. It covers the head of the newest github-actions[bot] cost hold marker before it while that marker was never edited and no comment on the pull request was deleted since it was posted (an edited or unreadable one, or a deletion since, covers no head, and no older marker stands in), and a later head only when every change since is a merge of the base: the pull request's net change against its base branch, file by file, is the same at both. With no marker before it, it covers no head. A decision recorded on that issue for another change no longer clears the cost hold of a pull request that links the issue later, and a commit after the decision needs a new one. diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 7d30106cd4..3415e29707 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -32,7 +32,7 @@ import { verifyHold, } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; -import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; +import { COMMENT_DELETED_QUERY, COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; import { VERIFY_PATH as RESOLVER_VERIFY_PATH } from "../pr-verify/resolve-pr.mjs"; import { decide as decideOutcome } from "../work-resolve-issue/report-outcome.mjs"; @@ -147,6 +147,9 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note if (body.query === COMMENT_EDITED_QUERY) { return { data: { node: { lastEditedAt: null } } }; } + if (body.query === COMMENT_DELETED_QUERY) { + return { data: { repository: { pullRequest: { timelineItems: { filteredCount: 0, nodes: [] } } } } }; + } if (body.query === PULLS_QUERY) { assert.deepEqual(body.variables, { owner: "melodic-software", name: "claude-code-plugins", after: null }); return { data: { repository: { pullRequests: { pageInfo: { hasNextPage: false, endCursor: null }, nodes: pulls } } } }; @@ -183,7 +186,7 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note }; const writes = () => calls - .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, UNLABELED_QUERY].includes(body.query)) + .filter(({ method, path, body }) => method === "POST" && path === "/graphql" && ![PULLS_QUERY, DETAIL_QUERY, PULL_OPENED_QUERY, COMMENT_EDITED_QUERY, COMMENT_DELETED_QUERY, UNLABELED_QUERY].includes(body.query)) .map(({ body }) => ({ mutation: body.query === ARM_MUTATION ? "arm" : body.query === DISARM_MUTATION ? "disarm" : "?", ...body.variables })); return { github, writes, verify, verified, calls, notes, pins }; } diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 61a53942a1..0609e7956f 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -428,6 +428,11 @@ export const PULL_OPENED_QUERY = `query($owner: String!, $name: String!, $number } }`; export const COMMENT_EDITED_QUERY = "query($id: ID!) { node(id: $id) { ... on IssueComment { lastEditedAt } } }"; +export const COMMENT_DELETED_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { timelineItems(itemTypes: [COMMENT_DELETED_EVENT], last: 100) { filteredCount nodes { ... on CommentDeletedEvent { createdAt } } } } + } +}`; // Whether `text` names pull request `number` as `#`: not part of a // longer number, and not after a letter, digit, `_`, `#`, `&`, `/`, `.` or @@ -445,11 +450,12 @@ const COST_MARKER = /^|\\z)"; "") | match("https://github\\.com/([A-Za-z0-9-]+)/([A-Za-z0-9._-]+)/(?:issues|pull)/([0-9]+)#issuecomment-([0-9]+)"; "g") @@ -3666,6 +3670,10 @@ jobs: opened=$(jq -r '.created_at | fromdateiso8601' <<<"$pr") || keep "the pull request has no creation time" comments=$(read_api "repos/$repo/issues/$PR_NUMBER/comments?per_page=100" --paginate) || keep "the comments are unreadable" jq -sc 'add // []' <<<"$comments" >"$RUNNER_TEMP/cost-hold-comments.json" || keep "the comments do not parse" + # shellcheck disable=SC2016 # GraphQL variables, not shell + deleted=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){timelineItems(itemTypes:[COMMENT_DELETED_EVENT],last:100){filteredCount nodes{... on CommentDeletedEvent{createdAt}}}}}}' \ + -F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the deleted comments are unreadable" + jq '.data' <<<"$deleted" >"$RUNNER_TEMP/cost-hold-deleted.json" || keep "the deleted comments do not parse" # holds_js ...: one function of the base's # merge-holds.mjs over the files. The module goes last: as the first # argument it would run as the command line. @@ -3681,10 +3689,12 @@ jobs: } # carries : whether the decision comment covers HEAD_SHA: # its decided head, from the newest cost hold marker before it and - # only while that marker was never edited, is HEAD_SHA, or the net + # only while that marker was never edited and no comment was deleted + # since it was posted (the base's deletedSince, so deleting a newer + # marker cannot promote an older head), is HEAD_SHA, or the net # change against the base branch is the same at both. No marker, an - # edited or unreadable newest marker, an unusable base branch name or - # any read that fails carries nothing. + # edited or unreadable newest marker, a deletion since it, an + # unusable base branch name or any read that fails carries nothing. carries() { local at="$RUNNER_TEMP/cost-hold-at.json" marker decided marker_edited base_ref jq '.created_at' <<<"$1" >"$at" || return 1 @@ -3695,6 +3705,8 @@ jobs: # shellcheck disable=SC2016 # GraphQL variables, not shell marker_edited=$(read_api graphql -f query='query($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}' -F id="$(jq -r '.node' <<<"$marker")") || return 1 jq -e '.data.node | objects | has("lastEditedAt") and .lastEditedAt == null' <<<"$marker_edited" >/dev/null 2>&1 || return 1 + jq '.at' <<<"$marker" >"$RUNNER_TEMP/cost-hold-marker-at.json" || return 1 + [ "$(holds_js deletedSince "$RUNNER_TEMP/cost-hold-deleted.json" "$RUNNER_TEMP/cost-hold-marker-at.json")" = false ] || return 1 [ "$decided" != "$HEAD_SHA" ] || return 0 base_ref=$(jq -r '.base.ref // ""' <<<"$pr") || return 1 jq -en --arg b "$base_ref" '$b | test("^(?!-)(?!.*\\.\\.)[A-Za-z0-9._/-]{1,255}$")' >/dev/null || return 1 @@ -3768,9 +3780,6 @@ jobs: done < <(jq -nr --argjson since "$since" '[inputs.workflow_runs[] | select((.updated_at | fromdateiso8601) > $since) | "\(.id) \(.head_sha)"] | unique[]' <<<"$runs") done <<<'pr-review-hosted.yml pr-review / claude-review-status pr-review-security-hosted.yml pr-review-security / security-review' - # shellcheck disable=SC2016 # GraphQL variables, not shell - deleted=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){timelineItems(itemTypes:[COMMENT_DELETED_EVENT],last:100){filteredCount nodes{... on CommentDeletedEvent{createdAt}}}}}}' \ - -F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the deleted comments are unreadable" verdict=$(jq -nr --arg label "$label" --argjson since "$since" --argjson holds "$holds" --argjson app "$LANES_APP_ID" \ --slurpfile events <(printf '%s\n' "$events") --slurpfile comments <(printf '%s\n' "$comments") \ --slurpfile deleted <(printf '%s\n' "$deleted") ' 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 b863cf76bd..0fd6934ae8 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 @@ -1975,6 +1975,11 @@ def _closing_issue(pr: dict[str, Any], repo: str) -> tuple[dict[str, Any], str | # one is hidden from a reader, so it cites nothing. HTML_COMMENT = re.compile(r"|\Z)", re.DOTALL) COMMENT_EDITED = "query PolicyCommentEdited($id:ID!){node(id:$id){... on IssueComment{lastEditedAt}}}" +COMMENT_DELETED = ( + "query PolicyCommentDeleted($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name)" + "{pullRequest(number:$number){timelineItems(itemTypes:[COMMENT_DELETED_EVENT],last:100)" + "{filteredCount nodes{... on CommentDeletedEvent{createdAt}}}}}}" +) def names_pull(text: Any, number: Any) -> bool: @@ -2009,12 +2014,13 @@ def _after(value: Any, opened: datetime | None) -> bool: _NEVER = datetime.min.replace(tzinfo=UTC) -def cost_marker(comments: Any, at: Any) -> tuple[str, str] | None: +def cost_marker(comments: Any, at: Any) -> tuple[str, str, Any] | None: """The newest cost hold marker github-actions[bot] posted among the pull request's `comments` before a decision made at `at`, as (the head it - held, its comment's node id), else None (merge-holds.mjs `costMarker`; - the first of two at one time wins, as in the review lanes' hold step). - It decides a head only while never edited, which the caller reads.""" + held, its comment's node id, its created_at), else None (merge-holds.mjs + `costMarker`; the first of two at one time wins, as in the review lanes' + hold step). It decides a head only while never edited and while no + comment was deleted since it was posted, which the caller reads.""" when = parse_timestamp(at) found: tuple[datetime, str, str] | None = None for row in json_array(comments): @@ -2033,8 +2039,30 @@ def cost_marker(comments: Any, at: Any) -> tuple[str, str] | None: and made < when and (found is None or made > found[0]) ): - found = (made, marker[1], str(row.get("node_id") or "")) - return (found[1], found[2]) if found else None + found = (made, marker[1], str(row.get("node_id") or ""), row.get("created_at")) + return (found[1], found[2], found[3]) if found else None + + +def deleted_since(answer: Any, at: Any) -> bool: + """Whether a `COMMENT_DELETED` answer (its `data`) shows a comment on the + pull request deleted at or after `at`, or cannot show that none was: no + readable list, more deletions than it lists, or a time that does not + parse (merge-holds.mjs `deletedSince`). A deleted newer marker would + otherwise make an older one the decided head.""" + when = parse_timestamp(at) + items = json_object( + json_object(json_object(json_object(answer).get("repository")).get("pullRequest")).get( + "timelineItems" + ) + ) + nodes, count = items.get("nodes"), items.get("filteredCount") + if when is None or not is_json_array(nodes) or not isinstance(count, int) or count > len(nodes): + return True + for node in nodes: + made = parse_timestamp(json_object(node).get("createdAt")) + if made is None or made >= when: + return True + return False def same_net_change(decided: Any, current: Any) -> bool: @@ -2068,8 +2096,10 @@ def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bo is the head, with no compare read, or every change since is a merge of the base branch, so the net change against it is the same at both (`same_net_change`). No marker, an edited or unreadable newest marker (no - older one stands in), an unusable base name or a read that fails carries - nothing.""" + older one stands in), a comment deleted since that marker was posted + (`deleted_since`, read before the head check, so deleting a newer marker + cannot promote an older head), an unusable base name or a read that fails + carries nothing.""" sha = str(json_object(pr.get("head")).get("sha") or "") base = json_object(pr.get("base")).get("ref") owner, name = _split_repo(repo) @@ -2084,9 +2114,14 @@ def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bo if marker is None: return False node = _graphql(COMMENT_EDITED, "cost hold marker", id=marker[1]).get("node") + if not (is_json_object(node) and "lastEditedAt" in node and node["lastEditedAt"] is None): + return False + deleted = _graphql( + COMMENT_DELETED, "deleted comments", owner=owner, name=name, number=pr.get("number") + ) except PolicyReadError: return False - if not (is_json_object(node) and "lastEditedAt" in node and node["lastEditedAt"] is None): + if deleted_since(deleted, marker[2]): return False decided = marker[0] if decided == sha: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py index 5baf46b459..bc457fd664 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_merge_cost.py @@ -47,6 +47,17 @@ def _net(*files: dict[str, Any]) -> dict[str, Any]: return {"files": list(files)} +def _deletions(deleted: Any) -> dict[str, Any]: + """A COMMENT_DELETED answer: a list of deletion times, or the timeline + items as given.""" + items = ( + {"filteredCount": len(deleted), "nodes": [{"createdAt": t} for t in deleted]} + if isinstance(deleted, (list, tuple)) + else deleted + ) + return {"repository": {"pullRequest": {"timelineItems": items}}} + + def _comment(**changes: Any) -> dict[str, Any]: return { "issue_url": "https://api.github.com/repos/owner/repo/issues/40", @@ -75,6 +86,8 @@ def decide( unreadable: str = "", pull: dict[str, Any] | None = None, marker_node: Any = ..., + deleted: Any = (), + head: str = HEAD, ) -> str | None: created = PR_CREATED if created is None else created self.reads: list[str] = [] @@ -99,7 +112,13 @@ def paginated(endpoint: str, label: str) -> list[dict[str, Any]]: answer = {"lastEditedAt": edited} if node is ... else node held = {"lastEditedAt": None} if marker_node is ... else marker_node - def graphql(_query: str, what: str, **variables: Any) -> dict[str, Any]: + def graphql(query: str, what: str, **variables: Any) -> dict[str, Any]: + if query == policy.COMMENT_DELETED: + self.reads.append("deleted") + self.assertEqual(variables, {"owner": "owner", "name": "repo", "number": 99}) + if unreadable == "deleted": + raise policy.PolicyReadError(what) + return _deletions(deleted) if variables.get("id") != "IC_M": return {"node": answer} self.reads.append("marker edited") @@ -114,7 +133,9 @@ def graphql(_query: str, what: str, **variables: Any) -> dict[str, Any]: ): return policy.cost_decision( REPO, - (pull or PULL) | {"body": body, "created_at": created}, + (pull or PULL) + | {"body": body, "created_at": created} + | ({} if head == HEAD else {"head": {"sha": head}}), issue, permission=lambda login: permission, agent_identity=IDENTITY, @@ -129,6 +150,7 @@ def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: "repos/owner/repo/issues/comments/7", "repos/owner/repo/issues/99/comments?per_page=100", "marker edited", + "deleted", ], ) @@ -356,6 +378,31 @@ def test_an_edited_or_unreadable_newest_marker_does_not_carry(self) -> None: edited = {"lastEditedAt": "2026-10-22T11:05:00Z"} self.assertIsNone(self.decide(markers=older, compares=same, marker_node=edited)) + def test_a_comment_deleted_since_the_newest_marker_does_not_carry(self) -> None: + same = {OLD: _net(TF), HEAD: _net(TF)} + # Deleting the newer marker would make an older head the decided head. + promoted = self.decide( + markers=[_marker(OLD, "2026-10-22T10:30:00Z")], + head=OLD, + deleted=["2026-10-22T11:45:00Z"], + ) + self.assertIsNone(promoted) + self.assertFalse(any("/compare/" in r for r in self.reads)) + for name, change in { + "at the marker's time": dict(deleted=["2026-10-22T11:00:00Z"]), + "unreadable": dict(unreadable="deleted"), + "more than listed": dict( + deleted={"filteredCount": 101, "nodes": [{"createdAt": "2026-10-22T09:00:00Z"}]} + ), + "an unparsed time": dict(deleted=["soon"]), + }.items(): + with self.subTest(name): + self.assertIsNone(self.decide(**change)) + self.assertIsNone(self.later(same, deleted=["2026-10-22T11:30:00Z"])) + # A deletion before the marker was posted does not stop it. + self.assertEqual(self.decide(deleted=["2026-10-22T10:59:59Z"]), LINK) + self.assertEqual(self.later(same, deleted=["2026-10-22T09:00:00Z"]), LINK) + def test_at_the_decided_head_it_carries_with_no_compare_read(self) -> None: pull = PULL | {"base": {"ref": "-unusable"}} self.assertEqual(self.decide(pull=pull), LINK) @@ -407,7 +454,11 @@ def get(endpoint: str) -> tuple[int | None, Any]: mock.patch.object(policy, "_get", side_effect=get), mock.patch.object(policy, "_paginated", return_value=markers), mock.patch.object( - policy, "_graphql", return_value={"node": {"lastEditedAt": None}} + policy, + "_graphql", + side_effect=lambda query, *_, **__: _deletions([]) + if query == policy.COMMENT_DELETED + else {"node": {"lastEditedAt": None}}, ), ): return policy.cost_decision( 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 e9b2163ce2..1dbc06a99d 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 @@ -23,7 +23,7 @@ place of an admin, a linked-issue comment counting only when it names the pull request as `#` (`namesPull`, `names_pull`), and both carry it to the same heads: the decided head, or a later one with the same net - change against the base branch (`decidedHead`, `sameNetChange`). + change against the base branch (`costMarker`, `sameNetChange`). """ from __future__ import annotations @@ -401,8 +401,10 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s # The head reads: the pull request's comments (a cost hold marker at the head # before the decision by default) and a compare per head. -# The marker's edit read answers `marker_edited`, or fails when it is "fail". -DEFAULT_HEADS = {"markers": [_marker(HEAD)], "compares": {}, "marker_edited": None} +# The marker's edit read answers `marker_edited`, or fails when it is "fail"; +# the deleted-comment read lists the `deleted` times (or answers the timeline +# items given), or fails when it is "fail". +DEFAULT_HEADS = {"markers": [_marker(HEAD)], "compares": {}, "marker_edited": None, "deleted": []} TF = {"filename": "a.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} # Each case: the heads world for a decision on the pull request (_link(7)). @@ -456,6 +458,17 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s "markers": [_marker(OLD)], "compares": {OLD: {"files": [TF, TF]}, HEAD: {"files": [TF, TF]}}, }, + "a comment deleted since the marker": {"deleted": ["2026-01-02T13:00:00Z"]}, + "a comment deleted since the marker, after a base merge": { + "markers": [_marker(OLD)], + "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF]}}, + "deleted": ["2026-01-02T13:00:00Z"], + }, + "a comment deleted before the marker": {"deleted": ["2026-01-02T11:00:00Z"]}, + "an unreadable deleted-comment list": {"deleted": "fail"}, + "more deleted comments than listed": { + "deleted": {"filteredCount": 2, "nodes": [{"createdAt": "2026-01-01T00:00:00Z"}]} + }, "a patchless pure rename": { "markers": [_marker(OLD)], "compares": { @@ -475,6 +488,13 @@ def _marker(sha: str, at: str = "2026-01-02T12:00:00Z", **fields: Any) -> dict[s } if (method === "GET" && path === "/repos/o/r/issues/7/comments?per_page=100&page=1") return heads.markers; if (method === "GET") return comment; + if (data.query.includes("COMMENT_DELETED_EVENT")) { + if (heads.deleted === "fail") throw new Error("unreadable"); + const items = Array.isArray(heads.deleted) + ? { filteredCount: heads.deleted.length, nodes: heads.deleted.map((createdAt) => ({ createdAt })) } + : heads.deleted; + return { data: { repository: { pullRequest: { timelineItems: items } } } }; + } if (data.variables.number !== undefined) { return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", baseRefName: "main", closingIssuesReferences: { totalCount: 1, nodes: [{ number: issue, repository: { nameWithOwner: "o/r" } }] } } } } }; @@ -513,7 +533,17 @@ def py_decision( ) -> str | None: heads = DEFAULT_HEADS | heads - def graphql(_query: str, what: str, **variables: Any) -> dict[str, Any]: + def graphql(query: str, what: str, **variables: Any) -> dict[str, Any]: + if "COMMENT_DELETED_EVENT" in query: + if heads["deleted"] == "fail": + raise policy.PolicyReadError(what) + deleted = heads["deleted"] + items = ( + {"filteredCount": len(deleted), "nodes": [{"createdAt": t} for t in deleted]} + if isinstance(deleted, list) + else deleted + ) + return {"repository": {"pullRequest": {"timelineItems": items}}} if variables.get("id") == "M": if heads["marker_edited"] == "fail": raise policy.PolicyReadError(what) @@ -567,15 +597,14 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: with self.subTest(case): answers[case] = py_decision(_link(7), _comment(7), heads) self.assertEqual(js_decision(_link(7), _comment(7), heads), answers[case]) - for case in ( + carried = { "a marker at the head", "a base merge since the decided head", - ): + "a comment deleted before the marker", + } + for case in carried: self.assertEqual(answers[case], _link(7), case) - for case in set(CARRIES) - { - "a marker at the head", - "a base merge since the decided head", - }: + for case in set(CARRIES) - carried: self.assertIsNone(answers[case], case) diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index 686c0c64e7..7e96d6bc84 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -169,7 +169,7 @@ lanes() { reset() { export STUB_FAIL='' STUB_LABELS='needs-human' LANES_APP_ID="$APP_ID" export STUB_ACTORS="{\"actors\":[{\"kind\":\"human\",\"id\":$OWNER_ID},{\"kind\":\"bot\",\"id\":5}]}" - export STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\"},\"base\":{\"ref\":\"main\"},\"body\":\"Closes #9306\\n\\n- Cost decision: [Q50]($LINK)\"}" + export STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\",\"sha\":\"0123456789abcdef0123456789abcdef01234567\"},\"base\":{\"ref\":\"main\"},\"body\":\"Closes #9306\\n\\n- Cost decision: [Q50]($LINK)\"}" export STUB_CLOSING='{"data":{"repository":{"pullRequest":{"closingIssuesReferences":{"totalCount":1,"nodes":[{"number":9306,"repository":{"nameWithOwner":"o/r"}}]}}}}}' STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" export STUB_DECISIONS @@ -254,27 +254,27 @@ export STUB_LABELS='' expect "with the label already off the hold reads cleared and nothing is written" yes no no reset -STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost"},"body":"Closes #9306"}' +STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","sha":"0123456789abcdef0123456789abcdef01234567"},"body":"Closes #9306"}' expect "no decision cited in the body clears nothing" no no no if grep -qE 'actions/workflows|check-runs|graphql' "$TMP_ROOT/log"; then fail "with no link in the body the step read runs or GraphQL" else ok "with no link in the body the step stops before the run and GraphQL reads" fi -STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\"},\"body\":\"\"}" +STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\",\"sha\":\"0123456789abcdef0123456789abcdef01234567\"},\"body\":\"\"}" expect "a decision cited only inside an HTML comment clears nothing" no no no -STUB_PR="{\"number\":6419,\"created_at\":\"2026-10-23T05:00:00Z\",\"head\":{\"ref\":\"fix/cost\"},\"body\":\"