diff --git a/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md b/.changes/source-control/agent-a5e09147a92521857-c3fc6a4d.md new file mode 100644 index 0000000000..b376f0573b --- /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 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.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 0f6af5a30e..06af02207b 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -712,7 +712,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)) : 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 ca416398d8..045d782dc0 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -34,7 +34,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"; @@ -87,7 +87,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", }; @@ -120,6 +120,10 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/issues/comments/7") { return COST_COMMENT; } + // 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: { 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("(?:\n|$)/; +const BRANCH = /^(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}$/; + +// The newest cost hold marker github-actions[bot] posted among the pull +// request's `comments` before a decision made at `at`, as { sha, node, at } +// (the head it held, its comment's node id and created_at), 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 and while no comment was deleted since it was posted, which the +// caller reads. +export function costMarker(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?.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 = { sha: marker[1], node: String(row.node_id ?? ""), at: row.created_at }; + latest = made; + } + } + return found; +} + +// Whether a COMMENT_DELETED_QUERY 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 +// (babysit_merge_policy.deleted_since). A deleted newer marker would +// otherwise make an older one the decided head. +export function deletedSince(answer, at) { + const when = Date.parse(at); + const items = answer?.repository?.pullRequest?.timelineItems; + const nodes = items?.nodes; + if (!Number.isFinite(when) || !Array.isArray(nodes) || !Number.isInteger(items.filteredCount) || items.filteredCount > nodes.length) { + return true; + } + return nodes.some((node) => { + const made = Date.parse(node?.createdAt); + return !Number.isFinite(made) || made >= when; + }); +} + +// 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 +// 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; + 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 && now !== null && then.size === now.size && [...then].every(([name, value]) => now.get(name) === value); +} + +// 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 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), +// a comment deleted since that marker was posted (deletedSince, 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. +async function carries(github, repo, number, comment, head, base) { + try { + 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 [owner, name] = repo.split("/"); + const deleted = await github("POST", "/graphql", { query: COMMENT_DELETED_QUERY, variables: { owner, name, number } }); + if (deletedSince(deleted?.data, marker.at)) { + return false; + } + const decided = marker.sha; + if (decided === head) { + return true; + } + if (typeof base !== "string" || !BRANCH.test(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 -// 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 -// 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) { +// 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 +// 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:`. 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(), ); @@ -451,15 +597,32 @@ 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() + 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; + : null); + let checked = refs === null; + const decisions = []; 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 ( @@ -468,19 +631,26 @@ 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; } 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) { - return link[0]; + if (unedited(node)) { + 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. @@ -543,7 +713,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 for the head it decided and a later head that only merges the base`, ); } if (BODY_HOLD.test(String(body ?? ""))) { @@ -685,7 +855,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) : 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 fd3d612338..8b196211a7 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -9,7 +9,13 @@ // 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: 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 +// 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"; @@ -17,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, linkHold, main } from "./merge-holds.mjs"; +import { COMMENT_DELETED_QUERY, COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, costMarker, deletedSince, linkHold, main, sameNetChange } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -25,36 +31,104 @@ 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`. +// 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`, +}); +// 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", 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, }); // 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, the linked issue (`target`), comment 7, and its edit time; then +// the head reads: the pull request's `comments` (a cost hold marker at HEAD +// by default) and a `compares` answer per head against `base`. +// `fail` names a read that throws. +function world({ + row = comment(), + closing = [{ number: 40, repository: { nameWithOwner: REPO } }], + node, + created = PR_CREATED, + target = { number: 40 }, + fail, + comments = [marker(HEAD)], + compares = {}, + base = "main", + markerNode, + deleted = [], +} = {}) { 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})$/.exec(path); + const read = + body?.query === PULL_OPENED_QUERY + ? "pull" + : body?.query === COMMENT_EDITED_QUERY + ? body.variables.id === "IC_M" + ? "marker edited" + : "edited" + : body?.query === COMMENT_DELETED_QUERY + ? "deleted" + : path === "/repos/owner/repo/issues/99/comments?per_page=100&page=1" + ? "comments" + : compare + ? `compare ${compare[2]}` + : `${method} ${path}`; reads.push(read); if (read === fail) { throw new Error(`${read} failed`); } + if (read === "comments") { + return comments; + } + 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, baseRefName: base, closingIssuesReferences: { totalCount: closing.length, nodes: closing } }, + }, + }, + }; + } + if (read === "deleted") { + assert.deepEqual(body.variables, { owner: "owner", name: "repo", number: 99 }); + const items = Array.isArray(deleted) ? { filteredCount: deleted.length, nodes: deleted.map((createdAt) => ({ createdAt })) } : deleted; + return { data: { repository: { pullRequest: { timelineItems: items } } } }; + } + 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 } }; } + if (read === "GET /repos/owner/repo/issues/40") { + return target; + } assert.equal(read, "GET /repos/owner/repo/issues/comments/7"); return row; }; @@ -63,19 +137,57 @@ 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).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", "marker edited", "deleted"]); }); -test("one on the pull request itself clears with no time check", 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 }); - 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 () => { + 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 c", "for ##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 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 () => { @@ -121,7 +233,7 @@ test("each failing condition of the gate's rule clears nothing", async () => { }); test("a decision line after leading blank lines still counts", async () => { - assert.equal((await decide(undefined, { row: comment({ body: "\n **Decision recorded.**\tCost: yes" }) })).link, LINK); + assert.equal((await decide(undefined, { row: comment({ body: "\n **Decision recorded.**\tCost: yes for #99" }) })).link, LINK); }); test("costHold holds a cost path, case-folded, until a decision", () => { @@ -137,14 +249,14 @@ test("costHold holds a cost path, case-folded, until a decision", () => { // workflow step runs it, with a GitHub for the decision reads. // A `files` entry is a path, or a files API row; `texts` maps // `@` to a manifest's text, which the contents API answers. -function ci(files, { reviews = [], body = "Closes #40", options, texts = {} } = {}) { +function ci(files, { reviews = [], body = "Closes #40", options, texts = {}, head = HEAD } = {}) { const dir = mkdtempSync(join(tmpdir(), "merge-holds-")); writeFileSync(join(dir, "body"), body); writeFileSync(join(dir, "discussion"), reviews.map((row) => JSON.stringify(row)).join("\n")); writeFileSync(join(dir, "actors"), JSON.stringify({ actors: [{ kind: "human", login: "owner-person", id: 5 }] })); writeFileSync(join(dir, "files"), files.map((row) => JSON.stringify(typeof row === "string" ? [row] : row)).join("\n")); writeFileSync(join(dir, "denied"), "[]"); - const argv = ["99", ...["body", "discussion", "files", "denied", "actors"].map((name) => join(dir, name)), HEAD]; + const argv = ["99", ...["body", "discussion", "files", "denied", "actors"].map((name) => join(dir, name)), head]; const { github: decisions } = world(options); const github = async (method, path, data) => { const read = /^\/repos\/owner\/repo\/contents\/(.+)\?ref=([0-9a-f]{40})$/.exec(path); @@ -260,6 +372,178 @@ 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 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." }); + 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(-4), ["marker edited", "deleted", `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, 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); + 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); + 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), null]) { + const odd = await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base }); + assert.equal(odd.link, null, String(base)); + assert.ok(!odd.reads.some((read) => read.startsWith("compare")), String(base)); + } + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base: "release/2.x" })).link, LINK); +}); + +test("a comment deleted since the newest marker carries nothing, even at its own head", async () => { + const same = { [OLD]: net(TF), [HEAD]: net(TF) }; + // Deleting the newer marker would make an older head the decided head. + const promoted = await decide(undefined, { comments: [marker(OLD, "2026-10-22T10:30:00Z")], head: OLD, deleted: ["2026-10-22T11:45:00Z"] }); + assert.equal(promoted.link, null); + assert.ok(!promoted.reads.some((read) => read.startsWith("compare"))); + assert.equal((await decide(undefined, { deleted: ["2026-10-22T11:00:00Z"] })).link, null); + assert.equal((await later(same, { deleted: ["2026-10-22T11:30:00Z"] })).link, null); + assert.equal((await decide(undefined, { fail: "deleted" })).link, null); + assert.equal((await decide(undefined, { deleted: { filteredCount: 101, nodes: [{ createdAt: "2026-10-22T09:00:00Z" }] } })).link, null); + // A deletion before the marker was posted does not stop it. + assert.equal((await decide(undefined, { deleted: ["2026-10-22T10:59:59Z"] })).link, LINK); + assert.equal((await later(same, { deleted: ["2026-10-22T09:00:00Z"] })).link, LINK); + const items = (nodes, filteredCount = nodes.length) => ({ repository: { pullRequest: { timelineItems: { filteredCount, nodes } } } }); + assert.equal(deletedSince(items([]), "2026-10-22T11:00:00Z"), false); + for (const [name, answer, at] of [ + ["no answer", undefined, "2026-10-22T11:00:00Z"], + ["no time", items([]), null], + ["an unparsed deletion time", items([{ createdAt: "soon" }]), "2026-10-22T11:00:00Z"], + ["no count", { repository: { pullRequest: { timelineItems: { nodes: [] } } } }, "2026-10-22T11:00:00Z"], + ]) { + assert.equal(deletedSince(answer, at), true, name); + } +}); + +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); + 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) => `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 () => { + 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" } }), + "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.deepEqual(costMarker([marker(OLD), marker(HEAD, "2026-10-22T11:30:00Z")], "2026-10-22T11:30:00Z"), { sha: OLD, node: "IC_M", at: "2026-10-22T11:00:00Z" }); + // Of two at one time, the first listed wins, as in the hold step. + 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", () => { + 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 fb6162e915..e6bd1ac1b1 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3127,9 +3127,9 @@ jobs: # queue up to four verdict lookups and their run reads more), about four # minutes, plus the cost-hold step's reads: one for most pull requests, # which stop at the label events; for a lane-labeled one whose body cites a - # decision, about eight, and two more per review run since the window - # opened, plus the base-config checkout's fetch (seconds; its retries - # replace the contents reads' 15 s each). + # decision, about eight (nine on a Refs issue), and two more per review + # run since the window opened, plus the base-config checkout's fetch + # (seconds; its retries replace the contents reads' 15 s each). timeout-minutes: 11 env: # gh_read's wait budget: a rate-limit wait must end by @@ -3491,16 +3491,34 @@ jobs: msg="draft: lanes not run; the ready_for_review run lints and tests this SHA" echo "::notice::$msg" echo "$msg" >>"$GITHUB_STEP_SUMMARY" + # The cost-hold and lanes-hold steps below run node; pin it to main's + # .node-version, as every other Node job is, rather than the runner + # image's default. + - name: Check out main's Node pin + if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.repository.default_branch }} + path: .base + persist-credentials: false + sparse-checkout: .node-version + sparse-checkout-cone-mode: false + - name: Set up Node + if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} + uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version-file: .base/.node-version # THE HOLD STEPS READ THEIR CONFIG FROM THE BASE COMMIT, ONCE. The tracker # config names the human-gated label and the trusted-actor list names who # may lift a hold; both are read at the base so the pull request cannot - # rewrite its own hold. A sparse checkout of the two files at the base SHA + # rewrite its own hold, as is merge-holds.mjs, whose refsIssue the cost + # step runs. A sparse checkout of the three files at the base SHA # is a git fetch, not a REST request: as of actions/checkout v7.0.1 its # API calls (default branch, object format, the stale-merge tracepoint, the # no-git archive download) all return early for a 40-hex `ref` on a runner # with git; recheck on a checkout bump. It checks out no pull request - # code: the ref is the base commit, nothing here is executed, and no - # credential is persisted. + # code: the ref is the base commit, the only code run from it is that + # base module, and no credential is persisted. # The steps below read BASE_CONFIG: empty when this step did not succeed, # which each of them treats as an unreadable config; a file missing from # a checkout that succeeded is a file the base commit does not have. @@ -3516,6 +3534,7 @@ jobs: sparse-checkout: | /.work-item-tracker.json /.github/standards/trusted-actors/trusted-actors.json + /.github/scripts/pr-merge/merge-holds.mjs sparse-checkout-cone-mode: false # A COST HOLD THE OWNER ALREADY DECIDED COMES OFF. A review lane re-holds # every new head, so its `cost` hold went back on after the owner had @@ -3539,11 +3558,24 @@ 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:`, on the pull request itself or (written - # after the pull request) on the one issue in this repository that - # GitHub records it as closing. A body `Refs:` line or a second closing - # issue counts for nothing, so an owner decision on an unrelated issue - # cannot clear this hold. + # `**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). 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, while + # never edited and no comment was deleted since it was posted; with + # none, an edited one or a deletion since, 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 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 + # 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 + # counts for nothing, and an owner decision on the issue for another + # change cannot clear this hold. + # The pull request's live head must still be the event's head. # It then outputs cleared=true and the decision link, and, with the label # on, posts one comment citing the decision and removes the label. A # GITHUB_TOKEN removal starts no run, so the steps below read the label @@ -3608,6 +3640,8 @@ jobs: else "lane" end' <<<"$events") || keep "the events do not parse" [ "$added" = lane ] || keep "$added" pr=$(read_api "repos/$repo/pulls/$PR_NUMBER") || keep "the pull request is unreadable" + # A late or re-run event for an older head must not lift the label off a newer one. + [ "$(jq -r '.head.sha // ""' <<<"$pr")" = "$HEAD_SHA" ] || keep "the head moved since this event" ref=$(jq -r '.head.ref | @uri' <<<"$pr") || keep "the pull request has no head branch" links=$(jq -r '.body // "" | gsub("|\\z)"; "") | match("https://github\\.com/([A-Za-z0-9-]+)/([A-Za-z0-9._-]+)/(?:issues|pull)/([0-9]+)#issuecomment-([0-9]+)"; "g") @@ -3616,32 +3650,108 @@ jobs: # shellcheck disable=SC2016 # GraphQL variables, not shell closing=$(read_api graphql -f query='query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){pullRequest(number:$number){closingIssuesReferences(first:2){totalCount nodes{number repository{nameWithOwner}}}}}}' \ -F owner="${repo%%/*}" -F name="${repo#*/}" -F number="$PR_NUMBER") || keep "the closing issues are unreadable" - # The one issue in this repository the pull request closes, as babysit's - # cost_decision reads it: none when it closes more than one. + # The linked issue, as babysit's _linked_issue reads it: the one issue + # in this repository the pull request closes, none when it closes more + # than one, and its one plain `Refs:` issue when it closes none. issues=" $(jq -r --arg repo "$repo" '.data.repository.pullRequest.closingIssuesReferences | select(.totalCount == 1) | .nodes[] | select((.repository.nameWithOwner | ascii_downcase) == ($repo | ascii_downcase)) | .number | tostring' <<<"$closing" 2>/dev/null) " + refs= + if [ "$(jq -r '.data.repository.pullRequest.closingIssuesReferences.totalCount' <<<"$closing" 2>/dev/null)" = 0 ]; then + jq -r '.body // ""' <<<"$pr" >"$RUNNER_TEMP/cost-hold-body.txt" || keep "the pull request body is unreadable" + # shellcheck disable=SC2016 # JavaScript, not shell + refs=$(node --input-type=module -e 'import { readFileSync } from "node:fs"; + import { pathToFileURL } from "node:url"; + const { refsIssue } = await import(pathToFileURL(process.argv[2]).href); + console.log(refsIssue(readFileSync(process.argv[1], "utf8")) ?? "");' \ + "$RUNNER_TEMP/cost-hold-body.txt" "$BASE_CONFIG/.github/scripts/pr-merge/merge-holds.mjs") || keep "the Refs line could not be read" + issues=" $refs " + fi opened=$(jq -r '.created_at | fromdateiso8601' <<<"$pr") || keep "the pull request has no creation time" - decision='' decision_id='' + 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. + 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]; + 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, from the newest cost hold marker before it and + # 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, 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 + 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 + 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 + 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 ] + } + # 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 [ "$number" = "$PR_NUMBER" ] || [[ $issues == *" $number "* ]] || continue + # A Refs number may name a pull request; only an issue is consulted. + if [ "$number" != "$PR_NUMBER" ] && [ -n "$refs" ] && [ -z "$refs_read" ]; then + target=$(read_api "repos/$repo/issues/$refs") || keep "the Refs issue is unreadable" + refs_read=1 + jq -e --argjson n "$refs" '.number == $n and .pull_request == null' <<<"$target" >/dev/null 2>&1 || issues=' ' + [[ $issues == *" $number "* ]] || continue + fi comment=$(read_api "repos/$repo/issues/comments/$id") || continue jq -e --arg number "$number" --arg pr "$PR_NUMBER" --argjson humans "$humans" --argjson opened "$opened" ' (.issue_url // "" | endswith("/issues/\($number)")) and .user.type == "User" and .performed_via_github_app == null and (.user.id as $id | $humans | index($id) != null) and (.body // "" | sub("\\A\\s+"; "") | test("\\A\\*\\*Decision recorded\\.\\*\\*[ \\t]+Cost:")) - and ($number == $pr or (.created_at | fromdateiso8601) > $opened)' <<<"$comment" >/dev/null 2>&1 || continue + and ($number == $pr or ((.created_at | fromdateiso8601) > $opened + and (.body // "" | test("(?:^|[^A-Za-z0-9_#&/.-])#\($pr)(?:[^0-9]|$)"))))' <<<"$comment" >/dev/null 2>&1 || continue node=$(jq -r .node_id <<<"$comment") # 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 - 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" - [ -n "$decision" ] || keep "the body cites no recorded owner cost decision covering it" + # 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='' while read -r file check; do @@ -3670,10 +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' - 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" 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") ' @@ -4067,22 +4173,6 @@ jobs: fi [ "$held" -eq 0 ] || exit 1 echo "#$pr carries neither 'do-not-merge' nor '$human_gated'." - # The lanes-hold step below runs node; pin it to main's .node-version, - # as every other Node job is, rather than the runner image's default. - - name: Check out main's Node pin - if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - with: - ref: ${{ github.event.repository.default_branch }} - path: .base - persist-credentials: false - sparse-checkout: .node-version - sparse-checkout-cone-mode: false - - name: Set up Node - if: ${{ !cancelled() && (github.event_name == 'pull_request' || github.event_name == 'merge_group') }} - uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 - with: - node-version-file: .base/.node-version # A LANES APP PULL REQUEST IS HELD BY ITS TEXT TOO. pr-merge.yml's # enable-auto-merge job arms native auto-merge on lanes App pull # requests, which then merge without pr-merge's gate, so this step fails diff --git a/.github/workflows/work-unblock.yml b/.github/workflows/work-unblock.yml index 8b3f0aef1b..33fff1987a 100644 --- a/.github/workflows/work-unblock.yml +++ b/.github/workflows/work-unblock.yml @@ -37,7 +37,8 @@ name: work-unblock # When that run opened a lanes-App PR instead, and the PR merged with `Refs:` # or was closed unmerged, the App adds needs-triage and takes agent-ready off, # after a comment; a person adding agent-ready again re-runs it. A Refs PR -# merging also lifts the needs-human report-outcome put on the issue for it. +# merging also lifts a needs-human refs-hold already on the issue for it. +# report-outcome no longer sets that hold, so only an existing one is lifted. # It also relabels a red-main fix issue post-merge-demotion-alert.yml reopened # on a repeat failure: that reopen keeps agent-ready but, made with the # workflow's GITHUB_TOKEN, starts no run. Matched by the issue's 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 44c157f242..0247d59b9a 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,27 @@ 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: + """Whether `text` names pull request `number` as `#`: not part of + a longer number, and not after a letter, digit, `_`, `#`, `&`, `/`, `.` + or `-`, so neither `owner/repo#` (another repository's) nor an + `&#` entity counts. ASCII classes only, as merge-holds.mjs + `namesPull` and ci-status read it.""" + if not isinstance(number, int) or isinstance(number, bool): + return False + return ( + re.search( + rf"(?:^|[^A-Za-z0-9_#&/.-])#{number}(?:[^0-9]|$)", str(text or "") + ) + is not None + ) def _after(value: Any, opened: datetime | None) -> bool: @@ -1982,6 +2003,139 @@ 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 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|\Z)" +) +BASE_BRANCH = re.compile(r"(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}") +_NEVER = datetime.min.replace(tzinfo=UTC) + + +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, 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): + row = json_object(row) + made = parse_timestamp(row.get("created_at")) + marker = ( + COST_MARKER.match(str(row.get("body") or "")) + if json_object(row.get("user")).get("login") == "github-actions[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], 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 isinstance(count, bool) 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: + """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 + 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) -> 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 + by_name: dict[str, tuple[Any, Any, str]] = {} + for f in files: + 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 + by_name[name] = (f.get("status"), f.get("previous_filename"), patch) + return by_name + + 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]) -> bool: + """Whether the decision `comment` covers the pull request at its head + (owner decisions Q17 c and Q18 d on #9436): its decided head + (`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), 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) + try: + 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") + 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 deleted_since(deleted, marker[2]): + return False + decided = marker[0] + 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}...{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], @@ -1991,21 +2145,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 its closing issue or on the - pull request itself 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 closing issue - counts only when created after the pull request was, so an older decision - on the issue cannot cover new work (an author can forge commit dates, not - these); one on the pull request postdates it already. 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 @@ -2025,7 +2186,10 @@ def cost_decision( or not DECISION_LINE.match(str(comment.get("body") or "").lstrip()) or ( int(link[3]) != pr.get("number") - and not _after(comment.get("created_at"), opened) + and not ( + _after(comment.get("created_at"), opened) + and names_pull(comment.get("body"), pr.get("number")) + ) ) ): continue @@ -2042,7 +2206,15 @@ def cost_decision( and "lastEditedAt" in node and node["lastEditedAt"] is None ): - 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 758bf4aada..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 @@ -1,5 +1,8 @@ """`cost_decision`: only an owner `**Decision recorded.**` comment on the -closing issue or the pull request, linked from the body, clears a cost hold.""" +pull request, or on its linked issue naming it as `#`, linked from +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 @@ -17,6 +20,42 @@ LINK = "https://github.com/owner/repo/issues/40#issuecomment-7" IDENTITY = "agent-lanes[bot]" PR_CREATED = "2026-10-22T10:00:00Z" +HEAD = "0123456789abcdef0123456789abcdef01234567" +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": "89abcdef0123456789abcdef0123456789abcdef", "ref": 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, "node_id": "IC_M", "created_at": at, "body": body} + + +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]: @@ -24,7 +63,7 @@ def _comment(**changes: Any) -> dict[str, Any]: "issue_url": "https://api.github.com/repos/owner/repo/issues/40", "user": {"login": "owner-person", "type": "User"}, "performed_via_github_app": None, - "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 @@ -42,23 +81,61 @@ def decide( issue: int | None = 40, node: Any = ..., created: Any = None, + markers: list[dict[str, Any]] | None = None, + compares: dict[str, Any] | None = None, + 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] = [] + 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) + 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}) + held = {"lastEditedAt": None} if marker_node is ... else marker_node + + 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") + if unreadable == "marker edited": + raise policy.PolicyReadError(what) + return {"node": held} + with ( mock.patch.object(policy, "_get", side_effect=get), - mock.patch.object(policy, "_graphql", graphql), + mock.patch.object(policy, "_paginated", side_effect=paginated), + mock.patch.object(policy, "_graphql", side_effect=graphql), ): return policy.cost_decision( REPO, - {"number": 99, "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, @@ -67,22 +144,60 @@ 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", + "marker edited", + "deleted", + ], + ) def test_a_decision_on_the_pull_request_itself_clears(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" + issue_url="https://api.github.com/repos/owner/repo/issues/99", + body="**Decision recorded.** Cost: approve it.", ) self.assertEqual(self.decide(body=link, comment=comment), link) + def test_an_issue_decision_counts_only_when_it_names_the_pull_request( + self, + ) -> None: + for text in ("for #99.", "(#99)", "#99", "see #99, then"): + with self.subTest(text): + comment = _comment(body=f"**Decision recorded.** Cost: ok {text}") + self.assertEqual(self.decide(comment=comment), LINK) + for text in ( + "for another change.", + "for #98.", + "for #990.", + "for owner/repo#99.", + "for x#99.", + "for pull 99.", + "for c", + "for ##99.", + ): + with self.subTest(text): + comment = _comment(body=f"**Decision recorded.** Cost: ok {text}") + self.assertIsNone(self.decide(comment=comment)) + + def test_a_refs_issue_decision_naming_the_pull_request_clears(self) -> None: + # The caller passes `_linked_issue`'s number: here the Refs issue. + self.assertEqual(self.decide(body=f"Refs: #40\n\n{LINK}\n"), LINK) + 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" @@ -108,6 +223,13 @@ def test_each_failing_condition_holds(self) -> None: "posted through an App": dict( comment=_comment(performed_via_github_app={"slug": "cursor"}) ), + "a non-admin naming the pull request": dict( + permission="write", + comment=_comment( + user={"login": "someone", "type": "User"}, + body="**Decision recorded.** Cost: approve #99.", + ), + ), "a bot": dict( comment=_comment(user={"login": "agent-lanes[bot]", "type": "Bot"}) ), @@ -148,6 +270,211 @@ 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"}), + "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): + 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_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_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_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) + 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: + 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", + side_effect=lambda query, *_, **__: _deletions([]) + if query == policy.COMMENT_DELETED + else {"node": {"lastEditedAt": None}}, + ), + ): + return policy.cost_decision( + REPO, + PULL | {"body": body}, + 40, + permission=lambda login: "admin", + agent_identity=IDENTITY, + ) + + 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: raise policy.PolicyReadError("permission unreadable") @@ -158,7 +485,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 8d664f9a63..ec78821edb 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 @@ -20,7 +20,10 @@ holds and passes what it passes. - 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. + 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 (`costMarker`, `sameNetChange`). """ from __future__ import annotations @@ -308,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, "ref": "main"}, +} ISSUE = 5 OWNER = {"login": "owner", "id": 1, "type": "User"} OTHER = {"login": "other", "id": 2, "type": "User"} @@ -334,13 +345,41 @@ def _link(target: int, kind: str = "pull", repo: str = REPO) -> str: # Each case: the body, then the comment its link names. DECISIONS = { "on the pull request": (_link(7), _comment(7)), - "on the closing issue after the pull request": ( + "on the closing issue after the pull request, naming it": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for #7."), + ), + "on the closing issue, naming it in parentheses": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} (#7)"), + ), + "on the closing issue, naming no pull request": ( _link(ISSUE, "issues"), _comment(ISSUE), ), + "on the closing issue, naming another pull request": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for #70."), + ), + "on the closing issue, naming another repository's": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for x/r#7."), + ), + "on the closing issue, naming it only as an entity": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for "), + ), + "on the closing issue, naming it only after a #": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for ##7."), + ), + "on the closing issue, naming it, by someone else": ( + _link(ISSUE, "issues"), + _comment(ISSUE, body=f"{DECISION} for #7.", user=OTHER), + ), "on the closing issue before the pull request": ( _link(ISSUE, "issues"), - _comment(ISSUE, created_at="2026-01-01T00:00:00Z"), + _comment(ISSUE, body=f"{DECISION} for #7.", created_at="2026-01-01T00:00:00Z"), ), "on another issue": (_link(9, "issues"), _comment(9)), "in another repository": (_link(7, repo="x/r"), _comment(7)), @@ -354,30 +393,136 @@ 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." + user = {"login": "github-actions[bot]", "type": "Bot"} + 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. +# 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)). +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"})]}, + "another bot's marker": { + "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]}}, + }, + "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 deleted-comment count that is not a number": { + "deleted": {"filteredCount": True, "nodes": [{"createdAt": "2026-01-01T00:00:00Z"}]} + }, + "a patchless pure rename": { + "markers": [_marker(OLD)], + "compares": { + sha: {"files": [{"filename": "b", "status": "renamed", "previous_filename": "a"}]} + for sha in (OLD, HEAD) + }, + }, +} 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") 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", + 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])))); +console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %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)) 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, @@ -386,12 +531,41 @@ 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 graphql(_query: str, _what: str, **_vars: Any) -> dict[str, Any]: +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, **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) + return {"node": {"lastEditedAt": heads["marker_edited"]}} 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]]: + 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( @@ -412,8 +586,30 @@ def test_both_accept_the_same_comments(self) -> None: answers[case] = py_decision(body, comment) self.assertEqual(js_decision(body, comment), answers[case]) self.assertIsNotNone(answers["on the pull request"]) + self.assertIsNotNone( + answers["on the closing issue after the pull request, naming it"] + ) + self.assertIsNone(answers["on the closing issue, naming no pull request"]) + self.assertIsNone(answers["on the closing issue, naming it only as an entity"]) + 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]) + 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) - carried: + 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 87a2882f2f..d4f98b9b67 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -6,9 +6,16 @@ # A review lane's `cost` hold is cleared when every lane hold since a trusted # person last removed the human-gated label is cost-only, and the pull request # body cites the owner's unedited `**Decision recorded.** Cost:` comment on the -# pull request itself, or (newer than the pull request) on the one issue GitHub -# records it as closing; a `Refs:` line or a second closing issue counts for -# nothing. The holds are read from the lanes' check-run notices. +# pull request itself, or, newer than the pull request and naming it as +# `#`, 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, 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. @@ -60,6 +67,12 @@ if [[ -n "${lift_line:-}" && "$job_line" -lt "$cost_line" && "$cost_line" -lt "$ else fail "step is not inside ci-status before the human-gated step (lines: $order)" fi +node_line="$(awk -v job="$job_line" 'NR > job && $0 == " - name: Set up Node" { print NR; exit }' "$WORKFLOW")" +if [[ -n "$node_line" && "$job_line" -lt "$node_line" && "$node_line" -lt "$cost_line" ]] && grep -qxF ' /.github/scripts/pr-merge/merge-holds.mjs' "$WORKFLOW"; then + ok "ci-status sets up the pinned Node and checks out the base merge-holds.mjs before the step runs refsIssue" +else + fail "the step runs node with no pinned Node before it, or the base checkout lacks merge-holds.mjs (Set up Node line ${node_line:-none})" +fi lift="$(awk '$0 == " - name: Fail a review-lane hold an untrusted account lifted" { on = 1; next } on && /^ if: / { print; exit }' "$WORKFLOW")" if [[ "$lift" == *"steps.cost_hold.outputs.cleared != 'true'"* ]]; then ok "the hold-lift step does not run once a recorded decision clears the hold" @@ -88,17 +101,20 @@ case "$*" in *'-X POST'*/issues/6419/comments*) failing post ;; */issues/6419/comments*) failing comments; printf '%s\n' "$STUB_COMMENTS" ;; */issues/comments/*) failing decision; from "$STUB_DECISIONS" "$(key comments "$@")" ;; +*/issues/[0-9]*) failing issue; from "$STUB_ISSUES" "$(key issues "$@")" ;; */actions/workflows/*/runs*) failing runs [[ "$*" == *'branch=fix%2Fcost&'* ]] || exit 1 from "$STUB_RUNS" "$(key workflows "$@")" ;; */compare/"$BASE_SHA..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; +*/compare/"${STUB_BASE_REF:-main}..."*) failing compare; args="$*"; from "$STUB_COMPARE" "${args##*...}" ;; */actions/runs/*/jobs*) failing jobs; from "$STUB_JOBS" "$(key runs "$@")" ;; */check-runs/*/annotations*) failing notes; from "$STUB_NOTES" "$(key check-runs "$@")" ;; */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 @@ -127,6 +143,17 @@ 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, 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"}' +# 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. @@ -142,14 +169,16 @@ 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\",\"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.')" '{"111": $c}')" + 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)]" - 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")]" '[]' unset STUB_TRACKER @@ -157,7 +186,7 @@ reset() { # 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 } @@ -170,6 +199,8 @@ expect() { mkdir -p "$config/.github/standards/trusted-actors" [[ -z "${STUB_TRACKER+set}" ]] || printf '%s\n' "$STUB_TRACKER" >"$config/.work-item-tracker.json" [[ " $STUB_FAIL " == *" actors "* ]] || printf '%s\n' "$STUB_ACTORS" >"$config/.github/standards/trusted-actors/trusted-actors.json" + mkdir -p "$config/.github/scripts/pr-merge" + [[ " $STUB_FAIL " == *" module "* ]] || cp "$ROOT/.github/scripts/pr-merge/merge-holds.mjs" "$config/.github/scripts/pr-merge/" [[ " $STUB_FAIL " != *" tracker "* ]] || config= out="$(PATH="$TMP_ROOT/bin:$PATH" STUB_LOG="$TMP_ROOT/log" PR_NUMBER=6419 GITHUB_REPOSITORY=o/r \ RUNNER_TEMP="$TMP_ROOT" GITHUB_OUTPUT="$TMP_ROOT/output" BASE_SHA="$BASE" HEAD_SHA="$HEAD" \ @@ -213,44 +244,47 @@ 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='' 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\":\"' '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 + +# 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/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]" +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 +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")]" +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)]" '[]' 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 @@ -299,22 +474,48 @@ 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 export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:21:30Z"}]}}}}}' -expect "a comment deleted in the window keeps the label" no no no "was deleted" +expect "a comment deleted in the window keeps the label" no no no "covering this head" export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T10:00:00Z"}]}}}}}' expect "a comment deleted before the window does not keep the label" yes yes yes export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":101,"nodes":[]}}}}}' -expect "more deletions than one page shows keeps the label" no no no "was deleted" +expect "more deletions than one page shows keeps the label" no no no "covering this head" +# A deleted newer marker must not make an older head the decided head. +reset +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T12:00:00Z)]" +export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:10:00Z"}]}}}}}' +expect "a comment deleted since the newest marker carries nothing at its head" no no no "covering this head" +if grep -q 'compare/main\.\.\.' "$TMP_ROOT/log"; then fail "a deletion since the marker still read a compare"; else ok "a deletion since the marker stops before any compare"; fi +STUB_COMMENTS="[$(marker "$OLD")]" +nets "[$TF]" "[$TF]" +export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:12:00Z"}]}}}}}' +expect "a comment deleted since the newest marker carries nothing after a base merge" no no no "covering this head" +reset +lifted +export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:09:00Z"}]}}}}}' +expect "a comment deleted before the newest marker does not stop the carry" yes yes yes +# A deletion in the window but before the marker passes the carry and is still caught by the window. +reset +lifted +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T13:17:00Z)]" +STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T13:18:00Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" +export STUB_COMMENTS STUB_DECISIONS +export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:16:00Z"}]}}}}}' +expect "a comment deleted in the window before the newest marker keeps the label" no no no "was deleted" +# A late event for an older head lifts nothing off the live one. +reset +STUB_PR="$(jq -c --arg o "$OLD" '.head.sha = $o' <<<"$STUB_PR")" +expect "a pull request whose head moved since the event clears nothing" no no no "the head moved since this event" reset STUB_EVENTS="[$(event labeled "$OWNER" 2026-10-23T13:20:20Z)]" @@ -351,7 +552,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