From 0aec52e63e951abe3eb147f30a2cbf9f79061f38 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:53:35 -0400 Subject: [PATCH 1/4] fix(pr-merge): take a cost decision's decided head from the review lane's check-run annotation The cost-decision readers took the decided head from the newest github-actions[bot] `claude-lane-hold ... criteria=cost` comment before the decision. Any workflow on GITHUB_TOKEN posts as github-actions[bot], so the head rested on a comment author login. All three merge-side readers (ci-status's cost-hold step, merge-holds.mjs costDecision, babysit_merge_policy.cost_decision) now read it from the review lanes' status-job notices instead: the newest cost notice recorded before the decision, only where its sha= is its own run's head, from runs vetted not to change either review-lane workflow. Two heads at the newest time decide nothing. The comment read, and the edit and deletion checks that guarded it, are removed. Refs: #9635 Co-Authored-By: Claude Opus 5.5 --- ...t-decided-head-from-annotation-9635a1c4.md | 7 + .../scripts/pr-merge/enable-auto-merge.mjs | 13 +- .../pr-merge/enable-auto-merge.test.mjs | 26 +- .github/scripts/pr-merge/merge-holds.mjs | 176 ++++++---- .github/scripts/pr-merge/merge-holds.test.mjs | 278 +++++++++------- .github/workflows/pr-require-checks.yml | 81 +++-- .../scripts/babysit_merge_policy.py | 224 ++++++++----- .../scripts/tests/test_babysit_merge_cost.py | 306 ++++++++++-------- .../scripts/tests/test_merge_holds_sync.py | 194 ++++++----- scripts/ci-hold-cost-decision.test.sh | 214 +++++++----- 10 files changed, 893 insertions(+), 626 deletions(-) create mode 100644 .changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md diff --git a/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md b/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md new file mode 100644 index 0000000000..99addd76d1 --- /dev/null +++ b/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `cost_decision` takes a cost decision's decided head from the review lanes' check-run annotations, never from a `github-actions[bot]` hold comment, which any workflow on `GITHUB_TOKEN` can post. The decided head is the `sha=` of the newest `claude-lane-hold ... criteria=` notice with `cost` that a review lane's status job (`pr-review-hosted.yml`, `pr-review-security-hosted.yml`) recorded before the decision, counted only where it names its own run's head. Every run on the head branch started before the decision must be on a head that changes neither review-lane workflow and lists fewer than 300 files, or nothing carries; two heads noticed at the newest time carry nothing. The runs, jobs, annotations and those compares are read with `BABYSIT_VERIFY_GH_TOKEN` when it is set. The edit and deletion checks on the hold comment are gone with the comment read. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index e6d2dd9772..64d277ff0d 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -699,7 +699,7 @@ async function listPulls(github, owner, name) { throw new Error(`more than ${MAX_PAGES * 100} open pull requests into main`); } -export async function main({ env = process.env, github, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, now = () => Date.now(), latch } = {}) { +export async function main({ env = process.env, github, actions, verify, room, sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)), random, now = () => Date.now(), latch } = {}) { try { const repo = REPO.exec(env.GITHUB_REPOSITORY ?? ""); if (!repo) { @@ -721,7 +721,16 @@ export async function main({ env = process.env, github, verify, room, sleep = (m const onResponse = (response) => { answer = { limited: response.headers.get("x-ratelimit-remaining") === "0" || response.headers.has("retry-after"), text: response.clone().text().catch(() => "") }; }; + const injected = github !== undefined; github = withRetry(github ?? createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined, onResponse, latch }), { sleep, random }); + // The lane runs, jobs and annotations a cost decision's decided head is + // read from, on the job token (actions: read, checks: read), as the + // verifier reads take it; the lanes App token holds neither. + actions = + actions ?? + (!injected && env.BABYSIT_VERIFY_GH_TOKEN + ? withRetry(createGitHub({ token: env.BABYSIT_VERIFY_GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined, latch }), { sleep, random }) + : github); verify ??= pythonVerify(env); const reads = READER_EVENTS.includes(env.EVENT_NAME); const pulls = await listPulls(github, repo[1], repo[2]); @@ -833,7 +842,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m detail = (await github("POST", "/graphql", { query: DETAIL_QUERY, variables }))?.data?.repository?.pullRequest ?? null; if (detail !== null) { const files = await changedFiles(github, `${repo[1]}/${repo[2]}`, pull.number); - const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid) : null; + const decision = costHold(files, null) ? await costDecision(github, repo[0], pull.number, pull.body, trustedHumans(env), pull.headRefOid, actions) : 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 c2072bc2fb..f62e7b7e3c 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -40,7 +40,7 @@ import { } from "./enable-auto-merge.mjs"; import { GitHubError } from "../../actions/check-trusted-trigger/check-trusted-trigger.mjs"; import { createRateLimitLatch } from "../../actions/log-api-budget/rate-guard.mjs"; -import { COMMENT_DELETED_QUERY, COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, VERIFY_PATH, verdictHold } from "./merge-holds.mjs"; +import { 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"; @@ -128,9 +128,20 @@ 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: `` }]; + // Its decided head: the code review lane's status job noticed a cost + // hold at the head before it; the security lane has no run. + const runs = /^\/repos\/melodic-software\/claude-code-plugins\/actions\/workflows\/([a-z-]+)\.yml\/runs\?event=pull_request&branch=fix%2Fcost&per_page=100&page=1$/.exec(path); + if (method === "GET" && runs) { + return { workflow_runs: runs[1] === "pr-review-hosted" ? [{ id: 9, head_sha: HEAD, created_at: "2026-10-22T10:50:00Z" }] : [] }; + } + if (method === "GET" && path === `/repos/melodic-software/claude-code-plugins/compare/main...${HEAD}`) { + return { files: [{ filename: "infra/main.tf" }] }; + } + if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/actions/runs/9/jobs?filter=all&per_page=100&page=1") { + return { jobs: [{ id: 90, name: "pr-review / claude-review-status", completed_at: "2026-10-22T11:00:00Z" }] }; + } + if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/check-runs/90/annotations?per_page=100&page=1") { + return [{ annotation_level: "notice", message: `claude-lane-hold lane=claude-review sha=${HEAD} criteria=cost label=needs-human` }]; } // The read-back pin, posted before an arm. if (method === "POST" && String(body?.body ?? "").startsWith("(?:\n|$)/; +// The review lanes whose status job records a `claude-lane-hold` notice, as +// [workflow file, status job name] (ci-status's cost-hold step reads the same +// two). The notice's head and criteria, and the base branch name a compare +// takes, as the hold step reads it. +export const REVIEW_LANES = [ + ["pr-review-hosted.yml", "pr-review / claude-review-status"], + ["pr-review-security-hosted.yml", "pr-review-security / security-review"], +]; +const HOLD_NOTICE = /^claude-lane-hold lane=[a-z-]+ sha=([0-9a-f]{40}) criteria=([a-z,-]+) label=/; 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) { +// Why a compare answer (base...head) shows a head whose lane runs cannot be +// trusted, else null: it changes a review-lane workflow, which a +// pull_request run takes from the head and so could write the notice, or it +// lists 300 files or more (the compare API's cap), or no file list. +export function laneChange(compare) { + const files = compare?.files; + if (!Array.isArray(files)) return "no file list"; + if (files.length >= 300) return "too many files"; + const lanes = REVIEW_LANES.map(([file]) => `.github/workflows/${file}`); + return files.some((f) => lanes.includes(f?.filename)) ? "a review-lane workflow" : null; +} + +// The cost hold notices among one status job's `annotations`, as { sha, at } +// with `at` the job's completion: only those naming `head`, the head its run +// ran on, so a notice can speak for no other commit. +export function costNotices(annotations, head, at) { + return (Array.isArray(annotations) ? annotations : []).flatMap((a) => { + const notice = a?.annotation_level === "notice" ? HOLD_NOTICE.exec(String(a.message ?? "")) : null; + return notice && notice[1] === head && notice[2].split(",").includes("cost") ? [{ sha: notice[1], at }] : []; + }); +} + +// The decided head: the sha of the newest of `notices` before `at`, else +// null; null too when two heads share that time +// (babysit_merge_policy.decided_head, and ci-status's cost-hold step). +export function decidedHead(notices, 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; + const before = notices.filter((n) => Date.parse(n.at) < cutoff); + const newest = Math.max(...before.map((n) => Date.parse(n.at))); + const heads = new Set(before.filter((n) => Date.parse(n.at) === newest).map((n) => n.sha)); + return heads.size === 1 ? [...heads][0] : null; +} + +// Every item of the REST list at `path` whose answer holds it under `key`. +async function listed(read, path, key) { + const rows = []; + for (let page = 1; ; page += 1) { + const answer = await read("GET", `${path}${path.includes("?") ? "&" : "?"}per_page=100&page=${page}`); + const items = answer?.[key]; + if (!Array.isArray(items)) { + throw new Error(`${path} has no ${key} list`); + } + rows.push(...items); + if (items.length < 100) { + return rows; } } - 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; - }); +// The head a decision made at `at` decided, from the review lanes' check-run +// annotations, never a comment (any GITHUB_TOKEN workflow posts as +// github-actions[bot]): every pull_request run of a REVIEW_LANES workflow on +// the head `branch` started before `at`, each run's head vetted against +// `base` (laneChange), its status job's cost notices (costNotices) from jobs +// done before `at`, then decidedHead. A run on a head that fails the vet, or +// a read that fails, decides nothing. `read` holds actions: read and +// checks: read. +async function noticedHead(read, repo, branch, base, at) { + const cutoff = Date.parse(at); + if (!Number.isFinite(cutoff) || typeof branch !== "string" || !BRANCH.test(branch)) { + return null; + } + const notices = []; + const vetted = new Set(); + for (const [file, check] of REVIEW_LANES) { + const runs = await listed(read, `/repos/${repo}/actions/workflows/${file}/runs?event=pull_request&branch=${encodeURIComponent(branch)}`, "workflow_runs"); + for (const run of runs) { + if (!(Date.parse(run?.created_at) < cutoff)) { + continue; + } + const head = String(run.head_sha ?? ""); + if (!/^[0-9a-f]{40}$/.test(head)) { + return null; + } + if (!vetted.has(head)) { + if (laneChange(await read("GET", `/repos/${repo}/compare/${base}...${head}`)) !== null) { + return null; + } + vetted.add(head); + } + for (const job of await listed(read, `/repos/${repo}/actions/runs/${run.id}/jobs?filter=all`, "jobs")) { + if (job?.name === check && Date.parse(job.completed_at) < cutoff) { + notices.push(...costNotices(await pages(read, `/repos/${repo}/check-runs/${job.id}/annotations`), head, job.completed_at)); + } + } + } + } + return decidedHead(notices, at); } // Whether a COMMENT_EDITED_QUERY answer's node was never edited. @@ -528,37 +578,25 @@ async function pages(github, path) { } } -// Whether the decision `comment` covers pull request `number` at `head` -// (owner decisions Q17 c and Q18 d on #9436): its decided head (costMarker, -// never edited, so no one with write access moved its sha) is `head`, with +// Whether the decision `comment` covers the pull request at `head` (owner +// decisions Q17 c and Q18 d on #9436): its decided head (noticedHead, read +// with `actions` from the head branch `branch`'s lane runs) is `head`, with // no compare read, or every change since is a merge of the base branch // `base`, so the net change against it is the same at both (sameNetChange). -// No 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) { +// No decided head, an unusable base name or a read that fails carries +// nothing. +async function carries(github, actions, repo, comment, head, base, branch) { try { - 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)) { + if (typeof base !== "string" || !BRANCH.test(base)) { 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)) { + const decided = await noticedHead(actions, repo, branch, base, comment.created_at); + if (decided === null) { 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); @@ -580,9 +618,11 @@ async function carries(github, repo, number, comment, head, base) { // 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) { +// body)` reads; `actions`, the same shape, reads the lane runs, their jobs, +// annotations and compares (it needs actions: read and checks: read). 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, actions = github) { const links = [...String(body ?? "").replaceAll(HTML_COMMENT, "").matchAll(DECISION_LINK)].filter( (link) => `${link[1]}/${link[2]}`.toLowerCase() === String(repo).toLowerCase(), ); @@ -651,7 +691,7 @@ export async function costDecision(github, repo, number, body, humans, head) { // 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; + return decision && (await carries(github, actions, repo, decision.comment, head, pull?.baseRefName, pull?.headRefName)) ? decision.link : null; } // A GitHub REST and GraphQL reader on fetch, for costDecision. diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 8b196211a7..c82a980d5d 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -12,10 +12,10 @@ // 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). +// newest cost hold notice a review lane's status job recorded before it on a +// run vetted not to change a review-lane workflow (#9635), 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). import assert from "node:assert/strict"; import { Buffer } from "node:buffer"; import { mkdtempSync, writeFileSync } from "node:fs"; @@ -23,7 +23,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { COMMENT_DELETED_QUERY, COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, costMarker, deletedSince, linkHold, main, sameNetChange } from "./merge-holds.mjs"; +import { COMMENT_EDITED_QUERY, PULL_OPENED_QUERY, changeOf, costDecision, costHold, costNotices, decidedHead, laneChange, linkHold, main, sameNetChange } from "./merge-holds.mjs"; const REPO = "owner/repo"; const LINK = "https://github.com/owner/repo/issues/40#issuecomment-7"; @@ -32,14 +32,17 @@ 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`, +const CHECKS = { "pr-review-hosted.yml": "pr-review / claude-review-status", "pr-review-security-hosted.yml": "pr-review-security / security-review" }; +// A review lane's pull_request run on `sha`, started at `created`, whose +// status job (or `job`) finished at `at` and recorded a hold notice naming +// `notice`. +const laneRun = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", lane = "pr-review-hosted.yml", job = CHECKS[lane], notice = sha, created = "2026-10-22T10:30:00Z" } = {}) => ({ + lane, + sha, + at, + job, + created, + notes: [{ annotation_level: "notice", message: `claude-lane-hold lane=claude-review sha=${notice} criteria=${criteria} label=needs-human` }], }); // 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" }; @@ -56,10 +59,11 @@ const comment = (changes = {}) => ({ }); // A GitHub that answers the decision reads: the pull request's creation time -// 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. +// and closing issues, the linked issue (`target`), comment 7, and its edit +// time; then the head reads: the review lanes' `runs` on the head branch (a +// cost notice at HEAD by default), their jobs and annotations, and a +// `compares` answer per head against `base` (a run's head answers net(TF) +// unless given). `fail` names a read that throws. function world({ row = comment(), closing = [{ number: 40, repository: { nameWithOwner: REPO } }], @@ -67,61 +71,62 @@ function world({ created = PR_CREATED, target = { number: 40 }, fail, - comments = [marker(HEAD)], + runs = [laneRun(HEAD)], compares = {}, base = "main", - markerNode, - deleted = [], } = {}) { const reads = []; + const answers = { ...Object.fromEntries(runs.map((run) => [run.sha, net(TF)])), ...compares }; const github = async (method, path, body) => { const compare = /^\/repos\/owner\/repo\/compare\/(.+)\.\.\.([0-9a-f]{40})$/.exec(path); + const listing = /^\/repos\/owner\/repo\/actions\/workflows\/([a-z-]+\.yml)\/runs\?event=pull_request&branch=fix%2Fcost&per_page=100&page=1$/.exec(path); + const jobs = /^\/repos\/owner\/repo\/actions\/runs\/([0-9]+)\/jobs\?filter=all&per_page=100&page=1$/.exec(path); + const notes = /^\/repos\/owner\/repo\/check-runs\/([0-9]+)\/annotations\?per_page=100&page=1$/.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}`; + ? "edited" + : listing + ? `runs ${listing[1]}` + : jobs + ? `jobs ${jobs[1]}` + : notes + ? `notes ${notes[1]}` + : compare + ? `compare ${compare[2]}` + : `${method} ${path}`; reads.push(read); if (read === fail) { throw new Error(`${read} failed`); } - if (read === "comments") { - return comments; + if (listing) { + return { workflow_runs: runs.flatMap((run, i) => (run.lane === listing[1] ? [{ id: i + 1, head_sha: run.sha, created_at: run.created }] : [])) }; + } + if (jobs) { + const run = runs[Number(jobs[1]) - 1]; + return { jobs: [{ id: Number(jobs[1]), name: run.job, completed_at: run.at }] }; + } + if (notes) { + return runs[Number(notes[1]) - 1].notes; } if (compare) { assert.equal(compare[1], base); - if (!(compare[2] in compares)) { + if (!(compare[2] in answers)) { throw new Error("no such commit"); } - return compares[compare[2]]; + return answers[compare[2]]; } if (read === "pull") { assert.deepEqual(body.variables, { owner: "owner", name: "repo", number: 99 }); return { data: { repository: { - pullRequest: { createdAt: created, baseRefName: base, closingIssuesReferences: { totalCount: closing.length, nodes: closing } }, + pullRequest: { createdAt: created, baseRefName: base, headRefName: "fix/cost", 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 } }; @@ -143,13 +148,22 @@ const decide = (body = `Closes #40\n\nCost decision: ${LINK}\n`, options = {}) = test("an owner decision on the closing issue, linked from the body, clears the cost hold", async () => { const { link, reads } = await decide(); assert.equal(link, LINK); - assert.deepEqual(reads, ["pull", "GET /repos/owner/repo/issues/comments/7", "edited", "comments", "marker edited", "deleted"]); + assert.deepEqual(reads, [ + "pull", + "GET /repos/owner/repo/issues/comments/7", + "edited", + "runs pr-review-hosted.yml", + `compare ${HEAD}`, + "jobs 1", + "notes 1", + "runs pr-review-security-hosted.yml", + ]); }); 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: "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); + assert.equal((await decide(own, { row, closing: [], runs: [laneRun(HEAD, "2026-10-21T08:00:00Z", { created: "2026-10-21T07:50: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); }); @@ -377,7 +391,7 @@ test("ci-status lifts a cost hold for the head the decision decided, and re-hold 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" }) } } }); + const later = await ci(["infra/main.tf"], { body, options: { row, runs: [laneRun(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. @@ -387,13 +401,13 @@ test("ci-status lifts a cost hold for the head the decision decided, and re-hold }); // 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 }); +const later = (compares, options = {}) => decide(undefined, { runs: [laneRun(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}`]); + assert.deepEqual(commit.reads.slice(-3), ["runs pr-review-security-hosted.yml", `compare ${OLD}`, `compare ${HEAD}`]); }); test("a rebase onto a newer base with the same net change carries, whatever order the files list in", async () => { @@ -421,7 +435,9 @@ test("a compare of 300 files or more, a file with no patch, a filename listed tw 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); + for (const read of ["runs pr-review-hosted.yml", "runs pr-review-security-hosted.yml", "jobs 1", "notes 1", `compare ${OLD}`]) { + assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { fail: read })).link, null, read); + } 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" }; @@ -434,64 +450,46 @@ test("a compare of 300 files or more, a file with no patch, a filename listed tw assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) }, { base: "release/2.x" })).link, LINK); }); -test("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: "" }); +test("at the decided head the decision carries with no net-change compare", async () => { + const { link, reads } = await decide(); assert.equal(link, LINK); - assert.ok(!reads.some((read) => read.startsWith("compare"))); + assert.deepEqual( + reads.filter((read) => read.startsWith("compare")), + [`compare ${HEAD}`], + ); + // With no usable base name no run can be vetted, so nothing carries. + assert.equal((await decide(undefined, { base: "" })).link, null); }); -test("with no cost hold marker before the decision nothing carries, even at the same head", async () => { +test("with no cost hold notice 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: [] }); + const { link, reads } = await decide(options.row ? own : undefined, { ...options, runs: [] }); assert.equal(link, null); - assert.deepEqual(reads.slice(-1), ["comments"]); + assert.deepEqual(reads.slice(-2), ["runs pr-review-hosted.yml", "runs pr-review-security-hosted.yml"]); } }); +test("the decided head is read from the lanes' check-run notices, never from a comment", async () => { + const { reads } = await decide(); + assert.ok(!reads.some((read) => /\/issues\/99\/comments/.test(read)), reads.join(", ")); + // The lane-run reads go to the reader given for them, the rest to github. + const { github, reads: all } = world(); + const seen = []; + const actions = async (method, path, body) => { + seen.push(path); + return github(method, path, body); + }; + const plain = async (method, path, body) => { + assert.ok(!/\/actions\/|\/check-runs\//.test(path), path); + return github(method, path, body); + }; + assert.equal(await costDecision(plain, REPO, 99, `Closes #40\n\n${LINK}`, HUMANS, HEAD, actions), LINK); + assert.ok(seen.some((path) => path.includes("/actions/workflows/")) && seen.some((path) => path.includes("/check-runs/")), seen.join(", ")); + assert.ok(all.includes("edited")); +}); + 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 = { @@ -499,8 +497,8 @@ test("the decision is the newest on the pull request, else the newest on its lin 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 pick = async (body, runs) => { + const { github: base } = world({ runs, 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); @@ -509,32 +507,76 @@ test("the decision is the newest on the pull request, else the newest on its lin }; 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)); + assert.equal(await pick(all, [laneRun(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); + assert.equal(await pick(all, [laneRun(HEAD, "2026-10-22T10:00:00Z", { created: "2026-10-22T09:50:00Z" }), laneRun(OLD, "2026-10-22T11:15:00Z")]), null); + assert.equal(await pick(`Closes #40\n${LINK}`, [laneRun(HEAD)]), LINK); }); -test("the decided head is the latest bot cost hold marker made before the decision", async () => { +test("the decided head is the newest cost notice a lane's status job recorded 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. + const runs = (...extra) => decide(undefined, { runs: [laneRun(OLD), ...extra], compares: same }); + // An older notice at the current head does not outweigh the latest one before the decision. + assert.equal((await decide(undefined, { runs: [laneRun(HEAD, "2026-10-22T10:40:00Z"), laneRun(OLD, "2026-10-22T11:30:00Z")], compares: same })).link, null); + assert.equal((await runs(laneRun(HEAD, "2026-10-22T11:30:00Z"))).link, LINK); + assert.equal((await runs(laneRun(HEAD, "2026-10-22T11:30:00Z", { lane: "pr-review-security-hosted.yml" }))).link, LINK); + assert.equal((await runs(laneRun(HEAD, "2026-10-22T11:30:00Z", { criteria: "cost,trust-gate" }))).link, LINK); 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" }), + "a job done after the decision": laneRun(HEAD, "2026-10-22T12:30:00Z"), + "a run started after the decision": laneRun(HEAD, "2026-10-22T11:30:00Z", { created: "2026-10-22T12:10:00Z" }), + "a job other than the status job": laneRun(HEAD, "2026-10-22T11:30:00Z", { job: "pr-review / review-key" }), + "the other lane's status job name": laneRun(HEAD, "2026-10-22T11:30:00Z", { job: CHECKS["pr-review-security-hosted.yml"] }), + "a notice naming another head than its run's": laneRun(OLD, "2026-10-22T11:30:00Z", { notice: HEAD }), + "a notice without cost": laneRun(HEAD, "2026-10-22T11:30:00Z", { criteria: "trust-gate" }), + })) { + assert.equal((await runs(extra)).link, null, name); + } + // Two heads noticed at the newest time decide nothing. + assert.equal((await runs(laneRun(HEAD, "2026-10-22T11:00:00Z", { lane: "pr-review-security-hosted.yml" }))).link, null); + // A run that started after the decision is not read at all. + const late = await decide(undefined, { runs: [laneRun(HEAD), laneRun(OLD, "2026-10-22T12:30:00Z", { created: "2026-10-22T12:10:00Z" })] }); + assert.ok(!late.reads.includes("jobs 2"), late.reads.join(", ")); +}); + +test("a lane run on a head that changes a review-lane workflow, or whose compare cannot vet it, decides nothing", async () => { + for (const [name, answer] of Object.entries({ + "the code lane's workflow": net(TF, { filename: ".github/workflows/pr-review-hosted.yml" }), + "the security lane's workflow": net({ filename: ".github/workflows/pr-review-security-hosted.yml" }), + "300 files": net(...Array.from({ length: 300 }, (_, i) => ({ filename: `f${i}` }))), + "no file list": { status: "diverged" }, + })) { + assert.equal((await decide(undefined, { compares: { [HEAD]: answer } })).link, null, name); + // Even a vetted later notice does not stand in for it. + const mixed = await decide(undefined, { runs: [laneRun(OLD, "2026-10-22T10:40:00Z"), laneRun(HEAD)], compares: { [OLD]: answer } }); + assert.equal(mixed.link, null, name); + } +}); + +test("laneChange, costNotices and decidedHead", () => { + assert.equal(laneChange(net(TF)), null); + assert.equal(laneChange(net({ filename: ".github/workflows/pr-review-hosted.yml" })), "a review-lane workflow"); + assert.equal(laneChange(net(...Array.from({ length: 300 }, (_, i) => ({ filename: `f${i}` })))), "too many files"); + assert.equal(laneChange(null), "no file list"); + const note = (message, level = "notice") => ({ annotation_level: level, message }); + const at = "2026-10-22T11:00:00Z"; + assert.deepEqual(costNotices([note(`claude-lane-hold lane=claude-review sha=${HEAD} criteria=critical-security,cost label=needs-human`)], HEAD, at), [{ sha: HEAD, at }]); + for (const [name, row] of Object.entries({ + "another head": note(`claude-lane-hold lane=claude-review sha=${OLD} criteria=cost label=needs-human`), + "a warning": note(`claude-lane-hold lane=claude-review sha=${HEAD} criteria=cost label=needs-human`, "warning"), + "no label": note(`claude-lane-hold lane=claude-review sha=${HEAD} criteria=cost`), + "not cost": note(`claude-lane-hold lane=claude-review sha=${HEAD} criteria=critical-security label=needs-human`), + "not first": note(`x claude-lane-hold lane=claude-review sha=${HEAD} criteria=cost label=needs-human`), })) { - assert.equal((await decide(undefined, { comments: [marker(OLD), extra], compares: same })).link, null, name); + assert.deepEqual(costNotices([row], HEAD, at), [], 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); + assert.deepEqual(costNotices(null, HEAD, at), []); + const n = (sha, when) => ({ sha, at: when }); + assert.equal(decidedHead([n(OLD, "2026-10-22T10:00:00Z"), n(HEAD, "2026-10-22T11:00:00Z")], "2026-10-22T12:00:00Z"), HEAD); + assert.equal(decidedHead([n(OLD, "2026-10-22T10:00:00Z"), n(HEAD, "2026-10-22T12:00:00Z")], "2026-10-22T12:00:00Z"), OLD); + assert.equal(decidedHead([n(OLD, at), n(HEAD, at)], "2026-10-22T12:00:00Z"), null); + assert.equal(decidedHead([n(HEAD, at), n(HEAD, at)], "2026-10-22T12:00:00Z"), HEAD); + assert.equal(decidedHead([], "2026-10-22T12:00:00Z"), null); + assert.equal(decidedHead([n(HEAD, at)], null), null); }); test("sameNetChange compares filename, status, previous_filename and patch", () => { diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 3cdf99878a..f828b730ec 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3749,9 +3749,9 @@ jobs: # lane's status job records (any attempt, on this branch), never from # the hold comment, which anyone with write access can edit or delete. # A pull_request run takes its workflow from the pull request, so a - # run whose head changes either review-lane workflow from the base (or - # whose comparison lists 300 files, the API's cap) could forge that - # notice, and keeps the hold. + # run in it or started before the decision whose head changes either + # review-lane workflow from the base (or whose comparison lists 300 + # files, the API's cap) could forge that notice, and keeps the hold. # - No lanes App escalation comment (a `|\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: @@ -2178,66 +2179,136 @@ 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)" +# The review lanes whose status job records a `claude-lane-hold` notice, as +# (workflow file, status job name) (merge-holds.mjs `REVIEW_LANES`, and +# ci-status's cost-hold step). The notice's head and criteria, and the base +# branch name a compare takes, as the hold step reads it. +REVIEW_LANES = ( + ("pr-review-hosted.yml", "pr-review / claude-review-status"), + ("pr-review-security-hosted.yml", "pr-review-security / security-review"), +) +HOLD_NOTICE = re.compile( + r"claude-lane-hold lane=[a-z-]+ sha=([0-9a-f]{40}) criteria=([a-z,-]+) label=" ) BASE_BRANCH = re.compile(r"(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}") +# The token the lane-run, job, annotation and vetting reads take when set: +# pr-merge.yml passes the job's GITHUB_TOKEN (actions: read, checks: read), +# while the lanes App token it runs gh with holds neither +# (babysit_verify_evidence.py `TOKEN_ENV`). +ACTIONS_TOKEN_ENV = "BABYSIT_VERIFY_GH_TOKEN" _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): +def lane_change(compare: Any) -> str | None: + """Why a compare answer (base...head) shows a head whose lane runs cannot + be trusted, else None: it changes a review-lane workflow, which a + pull_request run takes from the head and so could write the notice, or it + lists 300 files or more (the compare API's cap), or no file list + (merge-holds.mjs `laneChange`).""" + files = json_object(compare).get("files") + if not is_json_array(files): + return "no file list" + if len(files) >= 300: + return "too many files" + lanes = {f".github/workflows/{file}" for file, _ in REVIEW_LANES} + if any(json_object(f).get("filename") in lanes for f in files): + return "a review-lane workflow" + return None + + +def cost_notices(annotations: Any, head: str, at: Any) -> list[tuple[str, Any]]: + """The cost hold notices among one status job's `annotations`, as (sha, + at) with `at` the job's completion: only those naming `head`, the head its + run ran on, so a notice can speak for no other commit (merge-holds.mjs + `costNotices`).""" + found: list[tuple[str, Any]] = [] + for row in json_array(annotations): 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]" + notice = ( + HOLD_NOTICE.match(str(row.get("message") or "")) + if row.get("annotation_level") == "notice" 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 + if notice and notice[1] == head and "cost" in notice[2].split(","): + found.append((notice[1], at)) + return found -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.""" +def decided_head(notices: Collection[tuple[str, Any]], at: Any) -> str | None: + """The sha of the newest of `notices` before `at`, else None; None too + when two heads share that time (merge-holds.mjs `decidedHead`).""" when = parse_timestamp(at) - items = json_object( - json_object(json_object(json_object(answer).get("repository")).get("pullRequest")).get( - "timelineItems" + if when is None: + return None + before = [ + (made, sha) + for sha, made in ((sha, parse_timestamp(t)) for sha, t in notices) + if made is not None and made < when + ] + if not before: + return None + newest = max(made for made, _ in before) + heads = {sha for made, sha in before if made == newest} + return heads.pop() if len(heads) == 1 else None + + +def _noticed_head(owner: str, name: str, branch: Any, base: str, at: Any) -> str | None: + """The head a decision made at `at` decided, from the review lanes' + check-run annotations, never a comment (any GITHUB_TOKEN workflow posts as + github-actions[bot]): every pull_request run of a REVIEW_LANES workflow on + the head `branch` started before `at`, each run's head vetted against + `base` (`lane_change`), its status job's cost notices (`cost_notices`) + from jobs done before `at`, then `decided_head`. A run on a head that fails + the vet decides nothing, and a read that fails raises.""" + cutoff = parse_timestamp(at) + if cutoff is None or not isinstance(branch, str) or not BASE_BRANCH.fullmatch(branch): + return None + token = os.environ.get(ACTIONS_TOKEN_ENV) + env = {"GH_TOKEN": token} if token else None + notices: list[tuple[str, Any]] = [] + vetted: set[str] = set() + for file, check in REVIEW_LANES: + runs = _paginated( + f"repos/{owner}/{name}/actions/workflows/{file}/runs" + f"?event=pull_request&branch={quote(branch, safe='')}&per_page=100", + f"{file} runs", + items_key="workflow_runs", + env=env, ) - ) - 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 + for run in runs: + started = parse_timestamp(run.get("created_at")) + if started is None or started >= cutoff: + continue + head = str(run.get("head_sha") or "") + if not re.fullmatch(r"[0-9a-f]{40}", head): + return None + if head not in vetted: + status, compare = _get(f"repos/{owner}/{name}/compare/{base}...{head}", env) + if status != 200: + raise PolicyReadError(f"the changes of {head} are unreadable") + if lane_change(compare) is not None: + return None + vetted.add(head) + jobs = _paginated( + f"repos/{owner}/{name}/actions/runs/{run.get('id')}/jobs?filter=all&per_page=100", + f"run {run.get('id')} jobs", + items_key="jobs", + env=env, + ) + for job in jobs: + done = parse_timestamp(job.get("completed_at")) + if job.get("name") != check or done is None or done >= cutoff: + continue + notices += cost_notices( + _paginated( + f"repos/{owner}/{name}/check-runs/{job.get('id')}/annotations?per_page=100", + f"job {job.get('id')} annotations", + env=env, + ), + head, + job.get("completed_at"), + ) + return decided_head(notices, at) def same_net_change(decided: Any, current: Any) -> bool: @@ -2267,42 +2338,25 @@ def net(compare: Any) -> dict[str, tuple[Any, Any, str]] | None: def _carries(repo: str, pr: Mapping[str, Any], comment: Mapping[str, Any]) -> bool: """Whether the decision `comment` covers the pull request at its head (owner decisions Q17 c and Q18 d on #9436): its decided head - (`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.""" + (`_noticed_head`, from the head branch's review-lane runs) is the head, + with no compare read, or every change since is a merge of the base + branch, so the net change against it is the same at both + (`same_net_change`). No decided head, an unusable base name or a read + that fails carries nothing.""" sha = str(json_object(pr.get("head")).get("sha") or "") + branch = json_object(pr.get("head")).get("ref") base = json_object(pr.get("base")).get("ref") + if not isinstance(base, str) or not BASE_BRANCH.fullmatch(base): + return False 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") - ) + decided = _noticed_head(owner, name, branch, base, comment.get("created_at")) except PolicyReadError: return False - if deleted_since(deleted, marker[2]): + if decided is None: 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) ] 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 bc457fd664..1be14447de 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 @@ -23,7 +23,7 @@ HEAD = "0123456789abcdef0123456789abcdef01234567" BASE = "main" OLD = "fedcba9876543210fedcba9876543210fedcba98" -LANE = {"login": "github-actions[bot]", "type": "Bot"} +CHECKS = dict(policy.REVIEW_LANES) PULL = { "number": 99, "created_at": PR_CREATED, @@ -33,29 +33,85 @@ TF = {"filename": "infra/main.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} -def _marker( +def _run( sha: str, at: str = "2026-10-22T11:00:00Z", + *, criteria: str = "cost", - user: dict[str, Any] = LANE, + lane: str = "pr-review-hosted.yml", + job: str | None = None, + notice: str | None = None, + created: str = "2026-10-22T10:30:00Z", ) -> dict[str, Any]: - body = f"\nHeld." - return {"user": user, "node_id": "IC_M", "created_at": at, "body": body} + """A review lane's pull_request run on `sha`, started at `created`, whose + status job (or `job`) finished at `at` and recorded a hold notice naming + `notice` (default `sha`).""" + message = ( + f"claude-lane-hold lane=claude-review sha={notice or sha} " + f"criteria={criteria} label=needs-human" + ) + return { + "lane": lane, + "sha": sha, + "at": at, + "job": job or CHECKS[lane], + "created": created, + "notes": [{"annotation_level": "notice", "message": message}], + } 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}}} +class Lanes: + """The head reads of `_noticed_head` over `runs`, and the compares: a + run's head answers _net(TF) unless `compares` names it. A read whose label + (or `compare `) is `unreadable` fails. `reads` logs every endpoint and + `envs` the environment each lane read took.""" + + def __init__(self, runs: list[dict[str, Any]], compares: dict[str, Any], unreadable: str = "") -> None: + self.runs = runs + self.compares = {r["sha"]: _net(TF) for r in runs} | compares + self.unreadable = unreadable + self.reads: list[str] = [] + self.envs: list[Any] = [] + + def get(self, endpoint: str, env: Any = None) -> tuple[int | None, Any]: + self.reads.append(endpoint) + sha = endpoint.rsplit("...", 1)[1] + if sha in self.compares and self.unreadable != f"compare {sha}": + return 200, self.compares[sha] + return 404, None + + def paginated( + self, endpoint: str, label: str, items_key: str | None = None, env: Any = None + ) -> list[dict[str, Any]]: + self.reads.append(endpoint) + self.envs.append(env) + if label == self.unreadable: + raise policy.PolicyReadError(label) + prefix = "repos/owner/repo/" + if "/actions/workflows/" in endpoint: + lane = endpoint.split("/actions/workflows/", 1)[1].split("/", 1)[0] + assert endpoint == ( + f"{prefix}actions/workflows/{lane}/runs?event=pull_request" + "&branch=fix%2Fcost&per_page=100" + ), endpoint + assert items_key == "workflow_runs" + return [ + {"id": i + 1, "head_sha": r["sha"], "created_at": r["created"]} + for i, r in enumerate(self.runs) + if r["lane"] == lane + ] + if "/jobs?" in endpoint: + assert items_key == "jobs" + n = int(endpoint.split("/actions/runs/", 1)[1].split("/", 1)[0]) + run = self.runs[n - 1] + return [{"id": n, "name": run["job"], "completed_at": run["at"]}] + n = int(endpoint.split("/check-runs/", 1)[1].split("/", 1)[0]) + assert endpoint == f"{prefix}check-runs/{n}/annotations?per_page=100", endpoint + return self.runs[n - 1]["notes"] def _comment(**changes: Any) -> dict[str, Any]: @@ -81,61 +137,40 @@ def decide( issue: int | None = 40, node: Any = ..., created: Any = None, - markers: list[dict[str, Any]] | None = None, + runs: 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 {} + self.lanes = lanes = Lanes( + [_run(HEAD)] if runs is None else runs, compares or {}, unreadable + ) + self.reads = lanes.reads - def get(endpoint: str) -> tuple[int | None, Any]: - self.reads.append(endpoint) + def get(endpoint: str, env: Any = None) -> tuple[int | None, Any]: 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]]: + return lanes.get(endpoint, env) 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 + return status, comment if comment is not None else _comment() answer = {"lastEditedAt": edited} if node is ... else node - held = {"lastEditedAt": None} if marker_node is ... else marker_node def graphql(query: str, what: str, **variables: Any) -> dict[str, Any]: - 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} + self.assertEqual(query, policy.COMMENT_EDITED) + return {"node": answer} with ( mock.patch.object(policy, "_get", side_effect=get), - mock.patch.object(policy, "_paginated", side_effect=paginated), + mock.patch.object(policy, "_paginated", side_effect=lanes.paginated), mock.patch.object(policy, "_graphql", side_effect=graphql), ): return policy.cost_decision( REPO, (pull or PULL) | {"body": body, "created_at": created} - | ({} if head == HEAD else {"head": {"sha": head}}), + | ({} if head == HEAD else {"head": {"sha": head, "ref": "fix/cost"}}), issue, permission=lambda login: permission, agent_identity=IDENTITY, @@ -148,9 +183,13 @@ def test_an_owner_decision_on_the_closing_issue_clears(self) -> None: self.reads, [ "repos/owner/repo/issues/comments/7", - "repos/owner/repo/issues/99/comments?per_page=100", - "marker edited", - "deleted", + "repos/owner/repo/actions/workflows/pr-review-hosted.yml/runs" + "?event=pull_request&branch=fix%2Fcost&per_page=100", + f"repos/owner/repo/compare/{BASE}...{HEAD}", + "repos/owner/repo/actions/runs/1/jobs?filter=all&per_page=100", + "repos/owner/repo/check-runs/1/annotations?per_page=100", + "repos/owner/repo/actions/workflows/pr-review-security-hosted.yml/runs" + "?event=pull_request&branch=fix%2Fcost&per_page=100", ], ) @@ -193,8 +232,8 @@ def test_a_decision_on_the_pull_request_needs_no_time_check(self) -> None: issue_url="https://api.github.com/repos/owner/repo/issues/99", created_at="2026-10-21T09:00:00Z", ) - markers = [_marker(HEAD, "2026-10-21T08:00:00Z")] - self.assertEqual(self.decide(body=link, comment=comment, markers=markers), link) + runs = [_run(HEAD, "2026-10-21T08:00:00Z", created="2026-10-21T07:50:00Z")] + self.assertEqual(self.decide(body=link, comment=comment, runs=runs), 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)) @@ -272,7 +311,7 @@ def test_each_failing_condition_holds(self) -> None: # 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) + return self.decide(runs=[_run(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) @@ -325,88 +364,93 @@ def many(n: int) -> dict[str, Any]: 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")) + for label in ( + "pr-review-hosted.yml runs", + "pr-review-security-hosted.yml runs", + "run 1 jobs", + "job 1 annotations", + f"compare {OLD}", + ): + with self.subTest(label): + self.assertIsNone(self.later(same, unreadable=label)) self.assertIsNone(self.later({OLD: _net(TF), HEAD: {"status": "diverged"}})) - def test_the_decided_head_is_the_latest_bot_marker_before_the_decision( + def test_the_decided_head_is_the_newest_cost_notice_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) + older_head = [_run(HEAD, "2026-10-22T10:40:00Z"), _run(OLD, "2026-10-22T11:30:00Z")] + self.assertIsNone(self.decide(runs=older_head, compares=changed)) + late = "2026-10-22T11:30:00Z" for name, extra in { - "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"} + "a later notice at the head": _run(HEAD, late), + "from the security lane": _run(HEAD, late, lane="pr-review-security-hosted.yml"), + "with cost among its criteria": _run(HEAD, late, criteria="cost,trust-gate"), + }.items(): + with self.subTest(name): + self.assertEqual(self.decide(runs=[_run(OLD), extra], compares=changed), LINK) + for name, extra in { + "a job done after it": _run(HEAD, "2026-10-22T12:30:00Z"), + "a run started after it": _run(HEAD, late, created="2026-10-22T12:10:00Z"), + "a job other than the status job": _run(HEAD, late, job="pr-review / review-key"), + "the other lane's status job name": _run( + HEAD, late, job=CHECKS["pr-review-security-hosted.yml"] + ), + "a notice naming another head than its run's": _run(OLD, late, notice=HEAD), + "a notice without cost": _run(HEAD, late, criteria="trust-gate"), + "two heads at the newest time": _run( + HEAD, lane="pr-review-security-hosted.yml" ), - "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: + self.assertIsNone(self.decide(runs=[_run(OLD), extra], compares=changed)) + # A run that started after the decision is not read at all. + self.decide(runs=[_run(HEAD), _run(OLD, "2026-10-22T12:30:00Z", created="2026-10-22T12:10:00Z")]) + self.assertNotIn("repos/owner/repo/actions/runs/2/jobs?filter=all&per_page=100", self.reads) + + def test_a_lane_run_on_a_head_it_cannot_vet_decides_nothing(self) -> None: + for name, answer in { + "the code lane's workflow": _net(TF, {"filename": ".github/workflows/pr-review-hosted.yml"}), + "the security lane's workflow": _net( + {"filename": ".github/workflows/pr-review-security-hosted.yml"} + ), + "300 files": _net(*({"filename": f"f{i}"} for i in range(300))), + "no file list": {"status": "diverged"}, + }.items(): + with self.subTest(name): + self.assertIsNone(self.decide(compares={HEAD: answer})) + runs = [_run(OLD, "2026-10-22T10:40:00Z"), _run(HEAD)] + self.assertIsNone(self.decide(runs=runs, compares={OLD: answer})) + + def test_the_lane_reads_take_the_actions_token_when_set(self) -> None: + with mock.patch.dict(policy.os.environ, {policy.ACTIONS_TOKEN_ENV: "job"}): + self.assertEqual(self.decide(), LINK) + self.assertEqual(self.lanes.envs, [{"GH_TOKEN": "job"}] * 4) + with mock.patch.dict(policy.os.environ, {policy.ACTIONS_TOKEN_ENV: ""}): + self.assertEqual(self.decide(), LINK) + self.assertEqual(self.lanes.envs, [None] * 4) + + def test_the_decided_head_is_never_read_from_a_comment(self) -> None: + self.decide() + self.assertFalse(any("/issues/99/comments" in r for r in self.reads)) + + def test_with_no_notice_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.assertIsNone(self.decide(body=body, comment=comment, runs=[])) 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"], + def test_at_the_decided_head_it_carries_with_no_net_change_compare(self) -> None: + self.assertEqual(self.decide(), LINK) + self.assertEqual( + [r for r in self.reads if "/compare/" in r], + [f"repos/owner/repo/compare/{BASE}...{HEAD}"], ) - 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)) + # With no usable base no run can be vetted, so nothing carries. + self.assertIsNone(self.decide(pull=PULL | {"base": {"ref": "-unusable"}})) def test_a_duplicate_filename_a_pure_rename_or_an_unusable_base_does_not_carry( self, @@ -443,22 +487,19 @@ def on_pull(n: int) -> str: } 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]: + def pick(body: str, runs: list[dict[str, Any]]) -> str | None: + lanes = Lanes(runs, changed) + + def get(endpoint: str, env: Any = None) -> tuple[int | None, Any]: if "/compare/" in endpoint: - sha = endpoint.rsplit("...", 1)[1] - return 200, changed[sha] + return lanes.get(endpoint, env) 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, "_paginated", side_effect=lanes.paginated), mock.patch.object( - policy, - "_graphql", - side_effect=lambda query, *_, **__: _deletions([]) - if query == policy.COMMENT_DELETED - else {"node": {"lastEditedAt": None}}, + policy, "_graphql", return_value={"node": {"lastEditedAt": None}} ), ): return policy.cost_decision( @@ -470,10 +511,13 @@ def get(endpoint: str) -> tuple[int | None, Any]: ) 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.assertEqual(pick(every, [_run(HEAD, "2026-10-22T11:15:00Z")]), on_pull(9)) + older_head = [ + _run(HEAD, "2026-10-22T10:00:00Z", created="2026-10-22T09:50:00Z"), + _run(OLD, "2026-10-22T11:15:00Z"), + ] self.assertIsNone(pick(every, older_head)) - self.assertEqual(pick(f"Closes #40\n{LINK}", [_marker(HEAD)]), LINK) + self.assertEqual(pick(f"Closes #40\n{LINK}", [_run(HEAD)]), LINK) def test_a_failed_permission_read_clears_nothing(self) -> None: def refuse(login: str) -> str: diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_merge_holds_sync.py index ec78821edb..6dde0f2098 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 @@ -22,8 +22,9 @@ `babysit_merge_policy.cost_decision`, with the owner a trusted person in place of an admin, a linked-issue comment counting only when it names the pull request as `#` (`namesPull`, `names_pull`), 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`). + to the same heads: the decided head, read from the review lanes' check-run + notices (`noticedHead`, `_noticed_head`), or a later one with the same net + change against the base branch (`sameNetChange`). """ from __future__ import annotations @@ -393,119 +394,135 @@ 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": []} +CHECKS = dict(policy.REVIEW_LANES) + + +def _run( + sha: str, + at: str = "2026-01-02T12:00:00Z", + *, + criteria: str = "cost", + lane: str = "pr-review-hosted.yml", + job: str | None = None, + notice: str | None = None, + created: str = "2026-01-02T11:00:00Z", +) -> dict[str, Any]: + """A review lane's run on `sha`, started at `created`, whose status job + (or `job`) finished at `at` with a hold notice naming `notice`.""" + message = f"claude-lane-hold lane=claude-review sha={notice or sha} criteria={criteria} label=needs-human" + return { + "lane": lane, + "sha": sha, + "at": at, + "job": job or CHECKS[lane], + "created": created, + "notes": [{"annotation_level": "notice", "message": message}], + } + + +# The head reads: the review lanes' runs on the head branch (a cost notice at +# the head before the decision by default), their jobs and annotations, and a +# compare per head; a run's head answers {"files": [TF]} unless `compares` +# names it. +DEFAULT_HEADS = {"runs": [_run(HEAD)], "compares": {}} TF = {"filename": "a.tf", "status": "modified", "patch": "@@ -1 +1 @@\n-a\n+b"} MORE = {"filename": "b.sh", "status": "added", "patch": "+c"} +LATE = "2026-01-02T18:00:00Z" # Each case: the heads world for a decision on the pull request (_link(7)). CARRIES = { - "a marker at the head": DEFAULT_HEADS, + "a notice at the head": DEFAULT_HEADS, "a base merge since the decided head": { - "markers": [_marker(OLD)], + "runs": [_run(OLD)], "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF]}}, }, "a commit of its own since": { - "markers": [_marker(OLD)], + "runs": [_run(OLD)], "compares": {OLD: {"files": [TF]}, HEAD: {"files": [TF, MORE]}}, }, "a rename since": { - "markers": [_marker(OLD)], + "runs": [_run(OLD)], "compares": { OLD: {"files": [TF]}, HEAD: {"files": [TF | {"status": "renamed", "previous_filename": "z.tf"}]}, }, }, "300 files": { - "markers": [_marker(OLD)], + "runs": [_run(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)], + "runs": [_run(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)], + "an unreadable compare": {"runs": [_run(OLD)], "compares": {OLD: {"files": [TF]}}}, + "the latest notice before it, not an older one at the head": { + "runs": [_run(HEAD, "2026-01-02T06:00:00Z", created="2026-01-02T05:00:00Z"), _run(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 notice from the security lane": { + "runs": [_run(HEAD, lane="pr-review-security-hosted.yml")] }, - "a filename listed twice": { - "markers": [_marker(OLD)], - "compares": {OLD: {"files": [TF, TF]}, HEAD: {"files": [TF, TF]}}, + "a job done after it": {"runs": [_run(HEAD, "2026-01-04T00:00:00Z")]}, + "a run started after it": {"runs": [_run(HEAD, LATE, created="2026-01-03T01:00:00Z")]}, + "a job other than the status job": {"runs": [_run(HEAD, job="pr-review / review-key")]}, + "a notice naming another head than its run's": {"runs": [_run(OLD, notice=HEAD)]}, + "a notice without cost": {"runs": [_run(HEAD, criteria="trust-gate")]}, + "two heads at the newest time": { + "runs": [_run(HEAD), _run(OLD, lane="pr-review-security-hosted.yml")] }, - "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"], + "no notice": {"runs": []}, + "a run on a head that changes a review-lane workflow": { + "compares": {HEAD: {"files": [{"filename": ".github/workflows/pr-review-hosted.yml"}]}} }, - "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 run on a head whose compare has no file list": { + "compares": {HEAD: {"status": "diverged"}} }, - "a deleted-comment count that is not a number": { - "deleted": {"filteredCount": True, "nodes": [{"createdAt": "2026-01-01T00:00:00Z"}]} + "a filename listed twice": { + "runs": [_run(OLD)], + "compares": {OLD: {"files": [TF, TF]}, HEAD: {"files": [TF, TF]}}, }, "a patchless pure rename": { - "markers": [_marker(OLD)], + "runs": [_run(OLD)], "compares": { sha: {"files": [{"filename": "b", "status": "renamed", "previous_filename": "a"}]} for sha in (OLD, HEAD) }, }, } + + +def _answers(heads: dict[str, Any]) -> dict[str, Any]: + return {r["sha"]: {"files": [TF]} for r in heads["runs"]} | heads["compares"] + + NODE_DECISION = """ import { costDecision } from %s; -const [body, comment, issue, heads] = JSON.parse(process.argv[1]); +const [body, comment, issue, heads, answers] = 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 (!(sha in answers)) throw new Error("no such commit"); + return answers[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 } } } }; + const runs = /\\/actions\\/workflows\\/([a-z-]+\\.yml)\\/runs\\?event=pull_request&branch=b&per_page=100&page=1$/.exec(path); + if (method === "GET" && runs) { + return { workflow_runs: heads.runs.flatMap((r, i) => (r.lane === runs[1] ? [{ id: i + 1, head_sha: r.sha, created_at: r.created }] : [])) }; + } + const jobs = /\\/actions\\/runs\\/([0-9]+)\\/jobs\\?filter=all&per_page=100&page=1$/.exec(path); + if (method === "GET" && jobs) { + const r = heads.runs[Number(jobs[1]) - 1]; + return { jobs: [{ id: Number(jobs[1]), name: r.job, completed_at: r.at }] }; } + const notes = /\\/check-runs\\/([0-9]+)\\/annotations\\?per_page=100&page=1$/.exec(path); + if (method === "GET" && notes) return heads.runs[Number(notes[1]) - 1].notes; + if (method === "GET") return comment; if (data.variables.number !== undefined) { - return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", baseRefName: "main", + return { data: { repository: { pullRequest: { createdAt: "2026-01-02T00:00:00Z", baseRefName: "main", headRefName: "b", closingIssuesReferences: { totalCount: 1, nodes: [{ number: issue, repository: { nameWithOwner: "o/r" } }] } } } } }; } - if (data.variables.id === "M") { - if (heads.marker_edited === "fail") throw new Error("unreadable"); - return { data: { node: { lastEditedAt: heads.marker_edited } } }; - } return { data: { node: { lastEditedAt: comment.edited } } }; }; console.log(JSON.stringify(await costDecision(github, "o/r", 7, body, new Set([1]), %s))); @@ -522,7 +539,7 @@ def js_decision( "--input-type=module", "-e", script, - json.dumps([body, comment, ISSUE, DEFAULT_HEADS | heads]), + json.dumps([body, comment, ISSUE, DEFAULT_HEADS | heads, _answers(DEFAULT_HEADS | heads)]), ], capture_output=True, text=True, @@ -535,33 +552,34 @@ def py_decision( body: str, comment: dict[str, Any], heads: dict[str, Any] = DEFAULT_HEADS ) -> str | None: heads = DEFAULT_HEADS | heads + answers = _answers(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]: + def get(endpoint: str, env: Any = None) -> 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, answers[sha]) if sha in answers 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"] + def paginated( + endpoint: str, _label: str, items_key: str | None = None, env: Any = None + ) -> list[dict[str, Any]]: + if "/actions/workflows/" in endpoint: + lane = endpoint.split("/actions/workflows/", 1)[1].split("/", 1)[0] + assert endpoint.endswith("/runs?event=pull_request&branch=b&per_page=100"), endpoint + return [ + {"id": i + 1, "head_sha": r["sha"], "created_at": r["created"]} + for i, r in enumerate(heads["runs"]) + if r["lane"] == lane + ] + if "/jobs?" in endpoint: + n = int(endpoint.split("/actions/runs/", 1)[1].split("/", 1)[0]) + r = heads["runs"][n - 1] + return [{"id": n, "name": r["job"], "completed_at": r["at"]}] + n = int(endpoint.split("/check-runs/", 1)[1].split("/", 1)[0]) + return heads["runs"][n - 1]["notes"] with ( mock.patch.object(policy, "_get", side_effect=get), @@ -601,9 +619,9 @@ def test_both_carry_a_decision_to_the_same_heads(self) -> None: 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 notice at the head", "a base merge since the decided head", - "a comment deleted before the marker", + "a notice from the security lane", } for case in carried: self.assertEqual(answers[case], _link(7), case) diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index 4bf008f69f..e4d1cf4316 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -11,11 +11,12 @@ # 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. +# covers the head it decided (the sha of the newest cost hold notice a review +# lane's status job recorded before it, naming its own run's head; 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). The holds +# and the decided head are read from the lanes' check-run notices, never from +# a hold comment. # 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. @@ -114,7 +115,6 @@ case "$*" in */pulls/6419*) failing pull; printf '%s\n' "$STUB_PR" ;; *graphql*COMMENT_DELETED_EVENT*) failing deleted; printf '%s\n' "$STUB_DELETED" ;; *graphql*closingIssuesReferences*) failing closing; printf '%s\n' "$STUB_CLOSING" ;; -*graphql*lastEditedAt*'id=IC_M'*) failing marker-edited; printf '%s\n' "$STUB_MARKER_EDITED" ;; *graphql*lastEditedAt*) failing edited; printf '%s\n' "$STUB_EDITED" ;; *) exit 1 ;; esac @@ -143,27 +143,47 @@ 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 [at]: a review lane's hold comment on the pull request, which +# decides no head. marker() { - jq -c '.node_id = "IC_M"' <<<"$(comment "$LANE" "${2:-2026-10-23T13:10:00Z}" " -Held for a person.")" + 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. +# lanes : one run per lane, its status job done +# at 13:20:30 and 13:21:00, and the decided run (decided "$HEAD"). lanes() { - STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","updated_at":"2026-10-23T13:20:40Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","updated_at":"2026-10-23T13:21:10Z"}]}}')" + STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z"}]}}')" STUB_JOBS="$(jq -cn --argjson a "$(job 11 "$CODE_CHECK" 2026-10-23T13:20:30Z)" --argjson b "$(job 21 "$SEC_CHECK" 2026-10-23T13:21:00Z)" \ '{"1": {jobs: [{id: 10, name: "kill-switch", completed_at: "2026-10-23T13:19:00Z"}, $a]}, "2": {jobs: [$b]}}')" STUB_NOTES="$(jq -cn --argjson a "$1" --argjson b "$2" '{"11": $a, "21": $b, "10": []}')" STUB_COMPARE="$(jq -cn --arg h "$HEAD" --arg o "$OLD" '{($h): {files: [{filename: "README.md"}]}, ($o): {files: []}}')" export STUB_RUNS STUB_JOBS STUB_NOTES STUB_COMPARE + nodecided + decided "$HEAD" +} +# decided [completed_at] [criteria] [workflow] [job] [notice-sha]: a run +# on , started at 13:00, whose job (the code lane's status job unless +# named) recorded a hold notice naming [notice-sha] (default ). +decided() { + local id=$((DECIDED += 1)) at="${2:-2026-10-23T13:10:00Z}" + STUB_RUNS="$(jq -c --arg f "${4:-pr-review-hosted.yml}" --argjson id "$id" --arg sha "$1" --arg at "$at" \ + '.[$f].workflow_runs += [{id: $id, head_sha: $sha, created_at: "2026-10-23T13:00:00Z", updated_at: $at}]' <<<"$STUB_RUNS")" + STUB_JOBS="$(jq -c --arg id "$id" --argjson j "$(job "$id" "${5:-$CODE_CHECK}" "$at")" '. + {($id): {jobs: [$j]}}' <<<"$STUB_JOBS")" + STUB_NOTES="$(jq -c --arg id "$id" --argjson n "[$(note "claude-lane-hold lane=claude-review sha=${6:-$1} criteria=${3:-cost} label=needs-human")]" \ + '. + {($id): $n}' <<<"$STUB_NOTES")" + export STUB_RUNS STUB_JOBS STUB_NOTES +} +# nodecided: drops every run decided added. +nodecided() { + DECIDED=100 + STUB_RUNS="$(jq -c 'map_values(.workflow_runs |= map(select(.id <= 100)))' <<<"$STUB_RUNS")" + export STUB_RUNS } reset() { @@ -174,11 +194,10 @@ reset() { STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" export STUB_DECISIONS export STUB_EDITED='{"data":{"node":{"lastEditedAt":null}}}' - export STUB_MARKER_EDITED='{"data":{"node":{"lastEditedAt":null}}}' export STUB_ISSUES='{"9306":{"number":9306}}' export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":0,"nodes":[]}}}}}' STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" - STUB_COMMENTS="[$MARKER,$(comment "$OWNER" 2026-10-23T05:40:00Z 'A comment.')]" + STUB_COMMENTS="[$(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 @@ -186,7 +205,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="[$MARKER,$(comment "$APP" 2026-10-23T05:39:35Z '')]" + STUB_COMMENTS="[$(comment "$APP" 2026-10-23T05:39:35Z '')]" export STUB_EVENTS STUB_COMMENTS } @@ -244,10 +263,10 @@ else fi reset -STUB_COMMENTS="[$MARKER,$(comment "$LANE" 2026-10-23T13:21:00Z ' +STUB_COMMENTS="[$(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="[$MARKER,$(comment "$STRANGER" 2026-10-23T13:21:00Z '')]" +STUB_COMMENTS="[$(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='' @@ -275,10 +294,12 @@ STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$OWNER" 2026-10-23T04:00: expect "an issue decision older than the pull request clears nothing" no no no STUB_DECISIONS="$(jq -cn --argjson c "$(decision 6419 "$OWNER" 2026-10-23T04:00:00Z '**Decision recorded.** Cost: accepted.')" '{"111": $c}')" STUB_PR='{"number":6419,"created_at":"2026-10-23T05:00:00Z","head":{"ref":"fix/cost","sha":"0123456789abcdef0123456789abcdef01234567"},"base":{"ref":"main"},"body":"https://github.com/o/r/pull/6419#issuecomment-111"}' -STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T03:00:00Z)]" +nodecided +decided "$HEAD" 2026-10-23T03:00:00Z WANT_LINK=https://github.com/o/r/pull/6419#issuecomment-111 expect "a decision on the pull request itself clears the hold" yes yes yes -STUB_COMMENTS="[$MARKER]" -expect "with no cost hold marker before the decision nothing carries, even at the same head" no no no "covering this head" +nodecided +decided "$HEAD" +expect "with no cost hold notice before the decision nothing carries, even at the same head" no no no "covering this head" reset STUB_DECISIONS="$(jq -cn --argjson c "$(decision 9306 "$STRANGER" 2026-10-23T13:14:57Z '**Decision recorded.** Cost: accepted for #6419.')" '{"111": $c}')" expect "a decision by someone off the trusted-actor list clears nothing" no no no @@ -354,7 +375,8 @@ done # The carry rule (#9436 Q18 d): the decided head is OLD, the current HEAD. reset -STUB_COMMENTS="[$(marker "$OLD")]" +nodecided +decided "$OLD" nets "[$TF]" "[$TF]" expect "a base merge after the decided head carries the decision" yes yes yes if grep -q "compare/main...$OLD" "$TMP_ROOT/log"; then ok "the decided head's net change is read against the base branch"; else fail "no compare at the decided head"; fi @@ -376,41 +398,76 @@ nets '[{"filename":"a.png","status":"modified"}]' '[{"filename":"a.png","status" 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" +expect "a failed compare read carries nothing" no no no "are unreadable" export STUB_FAIL='' -STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T13:00:00Z),$(marker "$OLD" 2026-10-23T13:12:00Z)]" +nodecided +decided "$HEAD" 2026-10-23T13:00:00Z +decided "$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" +expect "the latest notice before the decision decides its head, not an older one at the head" no no no "covering this head" +nodecided +decided "$OLD" 2026-10-23T13:00:00Z +decided "$HEAD" 2026-10-23T13:12:00Z cost,trust-gate +lifted +expect "a later notice before the decision whose criteria include cost decides its head" yes yes yes +nodecided +decided "$OLD" +decided "$HEAD" 2026-10-23T13:16:00Z +expect "a notice after the decision decides nothing" no no no "covering this head" +nodecided +decided "$OLD" +decided "$HEAD" 2026-10-23T13:12:00Z trust-gate +expect "a later notice without cost decides nothing, so the older cost notice does" no no no "covering this head" +nodecided +decided "$OLD" 2026-10-23T13:12:00Z cost pr-review-hosted.yml "$CODE_CHECK" "$HEAD" +expect "a notice naming a head its run did not run on decides nothing" no no no "covering this head" +nodecided +decided "$HEAD" 2026-10-23T13:12:00Z cost pr-review-hosted.yml 'pr-review / review-key' +expect "a notice from a job other than the lane's status job decides nothing" no no no "covering this head" +nodecided +decided "$HEAD" 2026-10-23T13:12:00Z cost pr-review-hosted.yml "$SEC_CHECK" +expect "a notice under the other lane's status job name in this lane's workflow decides nothing" no no no "covering this head" +nodecided +decided "$HEAD" 2026-10-23T13:12:00Z cost pr-review-security-hosted.yml "$SEC_CHECK" +expect "a cost notice from the security lane's status job decides its head" yes yes yes +nodecided +decided "$HEAD" 2026-10-23T13:12:00Z +decided "$OLD" 2026-10-23T13:12:00Z cost pr-review-security-hosted.yml "$SEC_CHECK" +expect "two heads noticed at the newest time decide nothing" no no no "covering this head" +nodecided +STUB_COMMENTS="[$(marker "$HEAD" 2026-10-23T13:12:00Z)]" +expect "a github-actions[bot] hold comment with no notice decides no head" no no no "covering this head" +reset +STUB_COMMENTS="[$(marker "$OLD" 2026-10-23T13:12:00Z)]" +expect "a newer hold comment naming another head does not move the decided head" yes yes yes +reset +nodecided +decided "$OLD" +STUB_COMPARE="$(jq -cn --arg o "$OLD" --arg h "$HEAD" '{($o): {files: [{filename: ".github/workflows/pr-review-hosted.yml"}]}, ($h): {files: []}}')" +expect "a run before the decision on a head that changes a review-lane workflow keeps the hold" no no no "changes a review-lane workflow" +reset +nodecided +STUB_RUNS="$(jq -c --arg o "$OLD" '.["pr-review-hosted.yml"].workflow_runs += [{id: 5, head_sha: $o, created_at: "2026-10-23T13:16:00Z", updated_at: "2026-10-23T13:17:00Z"}]' <<<"$STUB_RUNS")" +STUB_JOBS="$(jq -c --argjson j "$(job 5 "$CODE_CHECK" 2026-10-23T13:12:00Z)" '. + {"5": {jobs: [$j]}}' <<<"$STUB_JOBS")" +STUB_NOTES="$(jq -c --argjson n "[$(hold_note cost)]" '. + {"5": $n}' <<<"$STUB_NOTES")" +STUB_EVENTS="[$(event unlabeled "$OWNER" 2026-10-23T13:18:00Z),$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" +export STUB_RUNS STUB_JOBS STUB_NOTES STUB_EVENTS +expect "with only a run started after the decision and outside the window, nothing carries" no no no "covering this head" +if grep -q 'actions/runs/5/jobs' "$TMP_ROOT/log"; then fail "a run started after the decision was read for the decided head"; else ok "a run started after the decision is not read for the decided head"; fi +nodecided STUB_COMMENTS='[]' -expect "an issue decision with no marker before it clears nothing" no no no "covering this head" +expect "an issue decision with no notice before it clears nothing" no no no "covering this head" reset +nodecided +decided "$OLD" +nets "[$TF]" "[$TF]" 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")]" +nodecided +decided "$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"}]' @@ -423,10 +480,6 @@ for base in '-main' 'a..b' 'a b' ''; do 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 @@ -438,9 +491,12 @@ STUB_DECISIONS="$(jq -cn --argjson a "$(decision 9306 "$OWNER" 2026-10-23T13:14: --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)]" +nodecided +decided "$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)]" +nodecided +decided "$HEAD" 2026-10-23T12:00:00Z +decided "$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" @@ -449,7 +505,7 @@ 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="[$MARKER,$(comment "$LANE" 2026-10-23T13:21:01Z " +STUB_COMMENTS="[$(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 @@ -460,13 +516,17 @@ STUB_JOBS="$(jq -cn --argjson a "$(job 11 "$CODE_CHECK" 2026-10-23T13:20:30Z)" - --argjson b "$(job 21 "$SEC_CHECK" 2026-10-23T13:21:00Z)" '{"1": {jobs: [$a, $c]}, "2": {jobs: [$b]}}')" STUB_NOTES="$(jq -cn --argjson a "[$(hold_note cost)]" --argjson c "[$(hold_note trust-gate)]" '{"11": $a, "12": $c, "21": []}')" export STUB_JOBS STUB_NOTES +nodecided +decided "$HEAD" expect "a re-run attempt that held for trust-gate clears nothing" no no no "is not cost-only" reset -STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","updated_at":"2026-10-23T13:20:40Z"},{"id":3,"head_sha":"fedcba9876543210fedcba9876543210fedcba98","updated_at":"2026-10-23T13:10:10Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","updated_at":"2026-10-23T13:21:10Z"}]}}')" +STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z"},{"id":3,"head_sha":"fedcba9876543210fedcba9876543210fedcba98","created_at":"2026-10-23T13:09:00Z","updated_at":"2026-10-23T13:10:10Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z"}]}}')" STUB_JOBS="$(jq -c --argjson c "$(job 31 "$CODE_CHECK" 2026-10-23T13:10:00Z)" '. + {"3": {jobs: [$c]}}' <<<"$STUB_JOBS")" STUB_NOTES="$(jq -c --argjson c "[$(hold_note trust-gate)]" '. + {"31": $c}' <<<"$STUB_NOTES")" export STUB_RUNS STUB_JOBS STUB_NOTES +nodecided +decided "$HEAD" 2026-10-23T13:05:00Z expect "an earlier trust-gate hold nobody trusted lifted keeps the label" no no no "is not cost-only" STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:09:58Z),$(event unlabeled "$STRANGER" 2026-10-23T13:15:00Z),$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" expect "an untrusted removal of a trust-gate hold does not open the window" no no no "is not cost-only" @@ -474,46 +534,31 @@ 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="[$MARKER,$(comment "$APP" 2026-10-23T05:39:35Z '')]" +STUB_COMMENTS="[$(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 escalated" lifted expect "a lanes App escalation before the window does not keep the label" yes yes yes -STUB_COMMENTS="[$MARKER,$(comment "$APP" 2026-10-23T13:22:00Z '')]" +STUB_COMMENTS="[$(comment "$APP" 2026-10-23T13:22:00Z '')]" expect "a lanes App escalation in the window keeps the label" no no no "lanes App escalated" -STUB_COMMENTS="[$MARKER,$(jq -c '.updated_at = "2026-10-23T13:22:00Z"' <<<"$(comment "$APP" 2026-10-23T05:39:35Z '')")]" +STUB_COMMENTS="[$(jq -c '.updated_at = "2026-10-23T13:22:00Z"' <<<"$(comment "$APP" 2026-10-23T05:39:35Z '')")]" expect "a lanes App escalation edited in the window keeps the label" no no no "lanes App escalated" -STUB_COMMENTS="[$MARKER,$(comment "$APP" 2026-10-23T13:22:00Z 'Replied to the review thread.')]" +STUB_COMMENTS="[$(comment "$APP" 2026-10-23T13:22:00Z 'Replied to the review thread.')]" expect "a lanes App comment in the window that is no escalation does not keep the label" yes yes yes 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 "covering this head" +expect "a comment deleted in the window keeps the label" no no no "was deleted" 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 "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" +expect "more deletions than one page shows keeps the label" no no no "was deleted" +# Deleting a hold comment moves no decided head: the notices decide it. 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" +STUB_COMMENTS="[$(marker "$OLD" 2026-10-23T13:12:00Z)]" +export STUB_COMMENTS +export STUB_DELETED='{"data":{"repository":{"pullRequest":{"timelineItems":{"filteredCount":1,"nodes":[{"createdAt":"2026-10-23T13:11:00Z"}]}}}}}' +expect "a comment deleted before the window, after the decided notice, does not stop the carry" yes yes yes # 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")" @@ -524,7 +569,7 @@ STUB_EVENTS="[$(event labeled "$OWNER" 2026-10-23T13:20:20Z)]" expect "a label a person added clears nothing" no no no "someone other than a review lane" STUB_EVENTS="[$(event labeled "$APP" 2026-10-23T13:20:20Z)]" expect "a label the lanes App added clears nothing" no no no "someone other than a review lane" -STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T13:00:00Z)]" +STUB_EVENTS="[$(event labeled "$LANE" 2026-10-23T12:59:00Z)]" expect "a lane label with no held review within ten minutes clears nothing" no no no "no held review after it" STUB_EVENTS='[]' expect "a hold with no label event clears nothing" no no no "no label was added" @@ -549,12 +594,13 @@ STUB_COMPARE="$(jq -cn --arg h "$HEAD" '{($h): {status: "diverged"}}')" expect "a comparison with no file list clears nothing" no no no "do not parse" reset lanes '[]' '[]' +lifted expect "a label with no recorded hold clears nothing" no no no "no review lane recorded a hold" 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 module; do +for read in tracker actors events pull runs compare jobs notes comments deleted closing decision edited; do reset export STUB_FAIL="$read" expect "an unreadable $read read clears nothing" no no no From 96c2d113335c57efe0312700f365a2b5096a1b71 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 26 Oct 2026 00:33:22 -0400 Subject: [PATCH 2/4] ci(pr-review): grant the review lanes checks: read for the cost-decision reads The ci-workflows partner change has pr-review-cli.yml's status job read the lane's earlier status-job notices, which needs actions: read and checks: read. A called workflow can only narrow its caller's grant, so both callers now grant checks: read beside actions: read. The scope is read-only, and the job that uses it runs no model. callers.test.mjs pins the review job's four scopes in both callers. Refs: #9635 Co-Authored-By: Claude Opus 5.5 --- .github/actions/resolve-config/callers.test.mjs | 9 +++++++++ .github/workflows/pr-review-hosted.yml | 5 ++++- .github/workflows/pr-review-security-hosted.yml | 5 ++++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/actions/resolve-config/callers.test.mjs b/.github/actions/resolve-config/callers.test.mjs index a18a1ab829..035b4a23e9 100644 --- a/.github/actions/resolve-config/callers.test.mjs +++ b/.github/actions/resolve-config/callers.test.mjs @@ -135,6 +135,15 @@ for (const [file, job, route, laneSwitch] of [ assert.equal(evaluate(review.with["model-args"], failed), ""); }); + test(`${file}: the review grants the reusable's status job its read scopes and one write`, () => { + assert.deepEqual(jobs[job].permissions, { + actions: "read", + checks: "read", + contents: "read", + "pull-requests": "write", + }); + }); + test(`${file}: the review runs only when the kill-switch job says proceed`, () => { const review = jobs[job]; const gate = jobs["kill-switch"]; diff --git a/.github/workflows/pr-review-hosted.yml b/.github/workflows/pr-review-hosted.yml index b43151aa7f..09e86558d1 100644 --- a/.github/workflows/pr-review-hosted.yml +++ b/.github/workflows/pr-review-hosted.yml @@ -165,7 +165,10 @@ jobs: # only the reusable's status job (the claude-review-status check) writes. permissions: actions: read # the reusable's check-key-credit job reads this - # workflow's runs for the key's recent credit stop + # workflow's runs for the key's recent credit stop, + # and its status job reads them for cost decisions + checks: read # the reusable's status job reads its earlier + # runs' cost notices contents: read pull-requests: write # the review, its inline comments and the lane's # reviewed-head marker comment diff --git a/.github/workflows/pr-review-security-hosted.yml b/.github/workflows/pr-review-security-hosted.yml index 969864c263..2b171c866f 100644 --- a/.github/workflows/pr-review-security-hosted.yml +++ b/.github/workflows/pr-review-security-hosted.yml @@ -170,7 +170,10 @@ jobs: # only the reusable's status job (the security-review check) writes. permissions: actions: read # the reusable's check-key-credit job reads this - # workflow's runs for the key's recent credit stop + # workflow's runs for the key's recent credit stop, + # and its status job reads them for cost decisions + checks: read # the reusable's status job reads its earlier + # runs' cost notices contents: read pull-requests: write # the review, its inline comments and the lane's # reviewed-head marker comment From 19feefe48017f72d0386ce1dfae03591a1e59787 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 26 Oct 2026 18:03:44 -0400 Subject: [PATCH 3/4] fix(pr-merge): read the decided head only from this repository's lane runs The workflow-runs branch= filter matches head_branch alone, so a fork pull request on a same-named branch could post the cost notice that decides another pull request's head. merge-holds.mjs noticedHead, babysit _noticed_head and the ci-status cost-hold step now skip any lane run whose head_repository.full_name is not this repository, before the vet, so a foreign run neither decides the head nor holds the step. run.pull_requests is not used: GitHub documents it as open pull requests matching the run's head branch or sha, not the one that triggered the run, and it is empty for closed pull requests and fork runs. Co-Authored-By: Claude Opus 5.5 --- ...t-decided-head-from-annotation-9635a1c4.md | 2 +- .github/scripts/pr-merge/merge-holds.mjs | 6 ++++-- .github/scripts/pr-merge/merge-holds.test.mjs | 9 ++++++--- .github/workflows/pr-require-checks.yml | 10 ++++++---- .../scripts/babysit_merge_policy.py | 8 +++++--- .../scripts/tests/test_babysit_merge_cost.py | 17 +++++++++++++---- .../scripts/tests/test_merge_holds_sync.py | 18 ++++++++++++++---- scripts/ci-hold-cost-decision.test.sh | 19 +++++++++++++++---- 8 files changed, 64 insertions(+), 25 deletions(-) diff --git a/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md b/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md index 99addd76d1..bffab13efb 100644 --- a/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md +++ b/.changes/source-control/fix-cost-decided-head-from-annotation-9635a1c4.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `cost_decision` takes a cost decision's decided head from the review lanes' check-run annotations, never from a `github-actions[bot]` hold comment, which any workflow on `GITHUB_TOKEN` can post. The decided head is the `sha=` of the newest `claude-lane-hold ... criteria=` notice with `cost` that a review lane's status job (`pr-review-hosted.yml`, `pr-review-security-hosted.yml`) recorded before the decision, counted only where it names its own run's head. Every run on the head branch started before the decision must be on a head that changes neither review-lane workflow and lists fewer than 300 files, or nothing carries; two heads noticed at the newest time carry nothing. The runs, jobs, annotations and those compares are read with `BABYSIT_VERIFY_GH_TOKEN` when it is set. The edit and deletion checks on the hold comment are gone with the comment read. +- `cost_decision` takes a cost decision's decided head from the review lanes' check-run annotations, never from a `github-actions[bot]` hold comment, which any workflow on `GITHUB_TOKEN` can post. The decided head is the `sha=` of the newest `claude-lane-hold ... criteria=` notice with `cost` that a review lane's status job (`pr-review-hosted.yml`, `pr-review-security-hosted.yml`) recorded before the decision, counted only where it names its own run's head and that head is in this repository, so a fork pull request on a same-named branch decides nothing. Every such run on the head branch started before the decision must be on a head that changes neither review-lane workflow and lists fewer than 300 files, or nothing carries; two heads noticed at the newest time carry nothing. The runs, jobs, annotations and those compares are read with `BABYSIT_VERIFY_GH_TOKEN` when it is set. The edit and deletion checks on the hold comment are gone with the comment read. diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 139885fc14..1f0c90e578 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -566,7 +566,9 @@ async function listed(read, path, key) { // The head a decision made at `at` decided, from the review lanes' check-run // annotations, never a comment (any GITHUB_TOKEN workflow posts as // github-actions[bot]): every pull_request run of a REVIEW_LANES workflow on -// the head `branch` started before `at`, each run's head vetted against +// the head `branch` of `repo` started before `at` (a run from another +// repository's same-named branch speaks for another pull request), each +// run's head vetted against // `base` (laneChange), its status job's cost notices (costNotices) from jobs // done before `at`, then decidedHead. A run on a head that fails the vet, or // a read that fails, decides nothing. `read` holds actions: read and @@ -581,7 +583,7 @@ async function noticedHead(read, repo, branch, base, at) { for (const [file, check] of REVIEW_LANES) { const runs = await listed(read, `/repos/${repo}/actions/workflows/${file}/runs?event=pull_request&branch=${encodeURIComponent(branch)}`, "workflow_runs"); for (const run of runs) { - if (!(Date.parse(run?.created_at) < cutoff)) { + if (run?.head_repository?.full_name !== repo || !(Date.parse(run.created_at) < cutoff)) { continue; } const head = String(run.head_sha ?? ""); diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index b4c86efb43..dfca94208f 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -30,9 +30,10 @@ const OLD = "fedcba9876543210fedcba9876543210fedcba98"; const CHECKS = { "pr-review-hosted.yml": "pr-review / claude-review-status", "pr-review-security-hosted.yml": "pr-review-security / security-review" }; // A review lane's pull_request run on `sha`, started at `created`, whose // status job (or `job`) finished at `at` and recorded a hold notice naming -// `notice`. -const laneRun = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", lane = "pr-review-hosted.yml", job = CHECKS[lane], notice = sha, created = "2026-10-22T10:30:00Z" } = {}) => ({ +// `notice`, with its head in `repo`. +const laneRun = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", lane = "pr-review-hosted.yml", job = CHECKS[lane], notice = sha, created = "2026-10-22T10:30:00Z", repo = REPO } = {}) => ({ lane, + repo, sha, at, job, @@ -96,7 +97,7 @@ function world({ throw new Error(`${read} failed`); } if (listing) { - return { workflow_runs: runs.flatMap((run, i) => (run.lane === listing[1] ? [{ id: i + 1, head_sha: run.sha, created_at: run.created }] : [])) }; + return { workflow_runs: runs.flatMap((run, i) => (run.lane === listing[1] ? [{ id: i + 1, head_sha: run.sha, created_at: run.created, head_repository: run.repo && { full_name: run.repo } }] : [])) }; } if (jobs) { const run = runs[Number(jobs[1]) - 1]; @@ -586,6 +587,8 @@ test("the decided head is the newest cost notice a lane's status job recorded be "the other lane's status job name": laneRun(HEAD, "2026-10-22T11:30:00Z", { job: CHECKS["pr-review-security-hosted.yml"] }), "a notice naming another head than its run's": laneRun(OLD, "2026-10-22T11:30:00Z", { notice: HEAD }), "a notice without cost": laneRun(HEAD, "2026-10-22T11:30:00Z", { criteria: "trust-gate" }), + "a run whose head is in another repository": laneRun(HEAD, "2026-10-22T11:30:00Z", { repo: "fork/repo" }), + "a run with no head repository": laneRun(HEAD, "2026-10-22T11:30:00Z", { repo: null }), })) { assert.equal((await runs(extra)).link, null, name); } diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index 880e8f4991..ab6518d0c4 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -3992,9 +3992,10 @@ jobs: [ -n "$chosen" ] || keep "the body cites no recorded owner cost decision covering this head" decided_at=$(jq -r '.at' <<<"$chosen") || keep "the decisions do not parse" # Every hold notice a review lane's status job recorded in a run - # updated in the window or started before the decision, as {at, - # criteria, sha, run}: `sha` as the notice names it, `run` the head - # its run ran on. + # whose head is in this repository (a fork's same-named branch speaks + # for another pull request), updated in the window or started before + # the decision, as {at, criteria, sha, run}: `sha` as the notice + # names it, `run` the head its run ran on. notices='[]' vetted='' while read -r file check; do runs=$(read_api "repos/$repo/actions/workflows/$file/runs?event=pull_request&branch=$ref&per_page=100" --paginate) || keep "the $file runs are unreadable" @@ -4020,7 +4021,8 @@ jobs: | {at: ($at | fromdateiso8601), criteria: $m.c, sha: $m.sha, run: $run}]' \ <<<"$notes") || keep "the annotations of job ${job%%/*} do not parse" done - done < <(jq -nr --argjson since "$since" --argjson decided "$decided_at" '[inputs.workflow_runs[] + done < <(jq -nr --arg repo "$repo" --argjson since "$since" --argjson decided "$decided_at" '[inputs.workflow_runs[] + | select(.head_repository.full_name == $repo) | select((.updated_at | fromdateiso8601) > $since or (.created_at | fromdateiso8601) < $decided) | "\(.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' 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 03666c90fb..372562b101 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 @@ -2260,8 +2260,9 @@ def _noticed_head(owner: str, name: str, branch: Any, base: str, at: Any) -> str """The head a decision made at `at` decided, from the review lanes' check-run annotations, never a comment (any GITHUB_TOKEN workflow posts as github-actions[bot]): every pull_request run of a REVIEW_LANES workflow on - the head `branch` started before `at`, each run's head vetted against - `base` (`lane_change`), its status job's cost notices (`cost_notices`) + the head `branch` of `owner`/`name` started before `at` (a run from + another repository's same-named branch speaks for another pull request), + each run's head vetted against `base` (`lane_change`), its status job's cost notices (`cost_notices`) from jobs done before `at`, then `decided_head`. A run on a head that fails the vet decides nothing, and a read that fails raises.""" cutoff = parse_timestamp(at) @@ -2281,7 +2282,8 @@ def _noticed_head(owner: str, name: str, branch: Any, base: str, at: Any) -> str ) for run in runs: started = parse_timestamp(run.get("created_at")) - if started is None or started >= cutoff: + head_repo = json_object(run.get("head_repository")).get("full_name") + if head_repo != f"{owner}/{name}" or started is None or started >= cutoff: continue head = str(run.get("head_sha") or "") if not re.fullmatch(r"[0-9a-f]{40}", head): 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 1be14447de..0a2f558723 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 @@ -42,10 +42,11 @@ def _run( job: str | None = None, notice: str | None = None, created: str = "2026-10-22T10:30:00Z", + repo: str | None = REPO, ) -> dict[str, Any]: - """A review lane's pull_request run on `sha`, started at `created`, whose - status job (or `job`) finished at `at` and recorded a hold notice naming - `notice` (default `sha`).""" + """A review lane's pull_request run on `sha` with its head in `repo`, + started at `created`, whose status job (or `job`) finished at `at` and + recorded a hold notice naming `notice` (default `sha`).""" message = ( f"claude-lane-hold lane=claude-review sha={notice or sha} " f"criteria={criteria} label=needs-human" @@ -56,6 +57,7 @@ def _run( "at": at, "job": job or CHECKS[lane], "created": created, + "repo": repo, "notes": [{"annotation_level": "notice", "message": message}], } @@ -100,7 +102,12 @@ def paginated( ), endpoint assert items_key == "workflow_runs" return [ - {"id": i + 1, "head_sha": r["sha"], "created_at": r["created"]} + { + "id": i + 1, + "head_sha": r["sha"], + "created_at": r["created"], + "head_repository": r["repo"] and {"full_name": r["repo"]}, + } for i, r in enumerate(self.runs) if r["lane"] == lane ] @@ -399,6 +406,8 @@ def test_the_decided_head_is_the_newest_cost_notice_before_the_decision( ), "a notice naming another head than its run's": _run(OLD, late, notice=HEAD), "a notice without cost": _run(HEAD, late, criteria="trust-gate"), + "a run whose head is in another repository": _run(HEAD, late, repo="fork/repo"), + "a run with no head repository": _run(HEAD, late, repo=None), "two heads at the newest time": _run( HEAD, lane="pr-review-security-hosted.yml" ), 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 d88401dd20..c7371e0b99 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 @@ -457,9 +457,11 @@ def _run( job: str | None = None, notice: str | None = None, created: str = "2026-01-02T11:00:00Z", + repo: str | None = "o/r", ) -> dict[str, Any]: - """A review lane's run on `sha`, started at `created`, whose status job - (or `job`) finished at `at` with a hold notice naming `notice`.""" + """A review lane's run on `sha` with its head in `repo`, started at + `created`, whose status job (or `job`) finished at `at` with a hold notice + naming `notice`.""" message = f"claude-lane-hold lane=claude-review sha={notice or sha} criteria={criteria} label=needs-human" return { "lane": lane, @@ -467,6 +469,7 @@ def _run( "at": at, "job": job or CHECKS[lane], "created": created, + "repo": repo, "notes": [{"annotation_level": "notice", "message": message}], } @@ -531,6 +534,8 @@ def _run( }, "a notice naming another head than its run's": {"runs": [_run(OLD, notice=HEAD)]}, "a notice without cost": {"runs": [_run(HEAD, criteria="trust-gate")]}, + "a run whose head is in another repository": {"runs": [_run(HEAD, repo="f/r")]}, + "a run with no head repository": {"runs": [_run(HEAD, repo=None)]}, "two heads at the newest time": { "runs": [_run(HEAD), _run(OLD, lane="pr-review-security-hosted.yml")] }, @@ -576,7 +581,7 @@ def _answers(heads: dict[str, Any]) -> dict[str, Any]: } const runs = /\\/actions\\/workflows\\/([a-z-]+\\.yml)\\/runs\\?event=pull_request&branch=b&per_page=100&page=1$/.exec(path); if (method === "GET" && runs) { - return { workflow_runs: heads.runs.flatMap((r, i) => (r.lane === runs[1] ? [{ id: i + 1, head_sha: r.sha, created_at: r.created }] : [])) }; + return { workflow_runs: heads.runs.flatMap((r, i) => (r.lane === runs[1] ? [{ id: i + 1, head_sha: r.sha, created_at: r.created, head_repository: r.repo && { full_name: r.repo } }] : [])) }; } const jobs = /\\/actions\\/runs\\/([0-9]+)\\/jobs\\?filter=all&per_page=100&page=1$/.exec(path); if (method === "GET" && jobs) { @@ -647,7 +652,12 @@ def paginated( "/runs?event=pull_request&branch=b&per_page=100" ), endpoint return [ - {"id": i + 1, "head_sha": r["sha"], "created_at": r["created"]} + { + "id": i + 1, + "head_sha": r["sha"], + "created_at": r["created"], + "head_repository": r["repo"] and {"full_name": r["repo"]}, + } for i, r in enumerate(heads["runs"]) if r["lane"] == lane ] diff --git a/scripts/ci-hold-cost-decision.test.sh b/scripts/ci-hold-cost-decision.test.sh index b574c274cb..ad69744d16 100755 --- a/scripts/ci-hold-cost-decision.test.sh +++ b/scripts/ci-hold-cost-decision.test.sh @@ -158,7 +158,7 @@ job() { jq -cn --argjson id "$1" --arg name "$2" --arg at "$3" '{id: $id, name: # lanes : one run per lane, its status job done # at 13:20:30 and 13:21:00, and the decided run (decided "$HEAD"). lanes() { - STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z"}]}}')" + STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z","head_repository":{"full_name":"o/r"}}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z","head_repository":{"full_name":"o/r"}}]}}')" STUB_JOBS="$(jq -cn --argjson a "$(job 11 "$CODE_CHECK" 2026-10-23T13:20:30Z)" --argjson b "$(job 21 "$SEC_CHECK" 2026-10-23T13:21:00Z)" \ '{"1": {jobs: [{id: 10, name: "kill-switch", completed_at: "2026-10-23T13:19:00Z"}, $a]}, "2": {jobs: [$b]}}')" STUB_NOTES="$(jq -cn --argjson a "$1" --argjson b "$2" '{"11": $a, "21": $b, "10": []}')" @@ -173,7 +173,7 @@ lanes() { decided() { local id=$((DECIDED += 1)) at="${2:-2026-10-23T13:10:00Z}" STUB_RUNS="$(jq -c --arg f "${4:-pr-review-hosted.yml}" --argjson id "$id" --arg sha "$1" --arg at "$at" \ - '.[$f].workflow_runs += [{id: $id, head_sha: $sha, created_at: "2026-10-23T13:00:00Z", updated_at: $at}]' <<<"$STUB_RUNS")" + '.[$f].workflow_runs += [{id: $id, head_sha: $sha, created_at: "2026-10-23T13:00:00Z", updated_at: $at, head_repository: {full_name: "o/r"}}]' <<<"$STUB_RUNS")" STUB_JOBS="$(jq -c --arg id "$id" --argjson j "$(job "$id" "${5:-$CODE_CHECK}" "$at")" '. + {($id): {jobs: [$j]}}' <<<"$STUB_JOBS")" STUB_NOTES="$(jq -c --arg id "$id" --argjson n "[$(note "claude-lane-hold lane=claude-review sha=${6:-$1} criteria=${3:-cost} label=needs-human")]" \ '. + {($id): $n}' <<<"$STUB_NOTES")" @@ -421,6 +421,17 @@ expect "a later notice without cost decides nothing, so the older cost notice do nodecided decided "$OLD" 2026-10-23T13:12:00Z cost pr-review-hosted.yml "$CODE_CHECK" "$HEAD" expect "a notice naming a head its run did not run on decides nothing" no no no "covering this head" +# A run on a same-named branch of another repository (a fork pull request) +# speaks for that pull request, not this one. +for head_repo in '{"full_name":"f/r"}' null; do + nodecided + decided "$OLD" + decided "$HEAD" 2026-10-23T13:12:00Z + nets "[$TF]" "[$TF,$MORE]" + STUB_RUNS="$(jq -c --argjson r "$head_repo" '.["pr-review-hosted.yml"].workflow_runs |= map(if .id == 102 then .head_repository = $r else . end)' <<<"$STUB_RUNS")" + export STUB_RUNS + expect "a notice from a run whose head repository is $head_repo decides nothing" no no no "covering this head" +done nodecided decided "$HEAD" 2026-10-23T13:12:00Z cost pr-review-hosted.yml 'pr-review / review-key' expect "a notice from a job other than the lane's status job decides nothing" no no no "covering this head" @@ -447,7 +458,7 @@ STUB_COMPARE="$(jq -cn --arg o "$OLD" --arg h "$HEAD" '{($o): {files: [{filename expect "a run before the decision on a head that changes a review-lane workflow keeps the hold" no no no "changes a review-lane workflow" reset nodecided -STUB_RUNS="$(jq -c --arg o "$OLD" '.["pr-review-hosted.yml"].workflow_runs += [{id: 5, head_sha: $o, created_at: "2026-10-23T13:16:00Z", updated_at: "2026-10-23T13:17:00Z"}]' <<<"$STUB_RUNS")" +STUB_RUNS="$(jq -c --arg o "$OLD" '.["pr-review-hosted.yml"].workflow_runs += [{id: 5, head_sha: $o, created_at: "2026-10-23T13:16:00Z", updated_at: "2026-10-23T13:17:00Z", head_repository: {full_name: "o/r"}}]' <<<"$STUB_RUNS")" STUB_JOBS="$(jq -c --argjson j "$(job 5 "$CODE_CHECK" 2026-10-23T13:12:00Z)" '. + {"5": {jobs: [$j]}}' <<<"$STUB_JOBS")" STUB_NOTES="$(jq -c --argjson n "[$(hold_note cost)]" '. + {"5": $n}' <<<"$STUB_NOTES")" STUB_EVENTS="[$(event unlabeled "$OWNER" 2026-10-23T13:18:00Z),$(event labeled "$LANE" 2026-10-23T13:20:20Z)]" @@ -521,7 +532,7 @@ decided "$HEAD" expect "a re-run attempt that held for trust-gate clears nothing" no no no "is not cost-only" reset -STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z"},{"id":3,"head_sha":"fedcba9876543210fedcba9876543210fedcba98","created_at":"2026-10-23T13:09:00Z","updated_at":"2026-10-23T13:10:10Z"}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z"}]}}')" +STUB_RUNS="$(jq -c . <<<'{"pr-review-hosted.yml":{"workflow_runs":[{"id":1,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:00Z","updated_at":"2026-10-23T13:20:40Z","head_repository":{"full_name":"o/r"}},{"id":3,"head_sha":"fedcba9876543210fedcba9876543210fedcba98","created_at":"2026-10-23T13:09:00Z","updated_at":"2026-10-23T13:10:10Z","head_repository":{"full_name":"o/r"}}]},"pr-review-security-hosted.yml":{"workflow_runs":[{"id":2,"head_sha":"0123456789abcdef0123456789abcdef01234567","created_at":"2026-10-23T13:19:30Z","updated_at":"2026-10-23T13:21:10Z","head_repository":{"full_name":"o/r"}}]}}')" STUB_JOBS="$(jq -c --argjson c "$(job 31 "$CODE_CHECK" 2026-10-23T13:10:00Z)" '. + {"3": {jobs: [$c]}}' <<<"$STUB_JOBS")" STUB_NOTES="$(jq -c --argjson c "[$(hold_note trust-gate)]" '. + {"31": $c}' <<<"$STUB_NOTES")" export STUB_RUNS STUB_JOBS STUB_NOTES From 81ca23d9be3de1e90243ff3c63dedb9f6874dd8e Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:18:48 -0400 Subject: [PATCH 4/4] test(pr-merge): name the repository on the cost decision fixture's lane run noticedHead skips a lane run whose head_repository is another repository, so the enable-auto-merge fixture's run carries this repository, as the merge-holds fixtures do. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/enable-auto-merge.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index ce8db79576..b072481eae 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -144,7 +144,7 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note // hold at the head before it; the security lane has no run. const runs = /^\/repos\/melodic-software\/claude-code-plugins\/actions\/workflows\/([a-z-]+)\.yml\/runs\?event=pull_request&branch=fix%2Fcost&per_page=100&page=1$/.exec(path); if (method === "GET" && runs) { - return { workflow_runs: runs[1] === "pr-review-hosted" ? [{ id: 9, head_sha: HEAD, created_at: "2026-10-22T10:50:00Z" }] : [] }; + return { workflow_runs: runs[1] === "pr-review-hosted" ? [{ id: 9, head_sha: HEAD, head_repository: { full_name: REPO }, created_at: "2026-10-22T10:50:00Z" }] : [] }; } if (method === "GET" && path === `/repos/melodic-software/claude-code-plugins/compare/main...${HEAD}`) { return { files: [{ filename: "infra/main.tf" }] };