diff --git a/.changes/pr-pipeline/feat-clear-stale-head-holds-5b1e9c07.md b/.changes/pr-pipeline/feat-clear-stale-head-holds-5b1e9c07.md new file mode 100644 index 0000000000..774cae23d3 --- /dev/null +++ b/.changes/pr-pipeline/feat-clear-stale-head-holds-5b1e9c07.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Added + +- pr-merge's scheduled `clear-escalations` removes a human-gated label a review lane added when every hold comment since the label was last removed names an earlier head, as `stale-review-hold`. At the current head it needs a trusted move (every push since the one that landed the newest held head is by a `human` of the base trusted-actor list, never the lanes App), a whole review from both lanes that passed (each caller's newest run's status job succeeded with a review-complete notice and no hold notice for the head, on a head that changes neither review caller), no HIGH or CRITICAL finding: `critical-findings: 0` from the security lane, every finding inline for the security lane and any lane a hold names, and no unresolved lane thread at `CRITICAL` or `IMPORTANT` (for those strict lanes, none with no readable severity either), one of the pull request's own commits since each held head (not reachable from `main`) that is not a merge, and for a `critical-security` hold at least one `CRITICAL` inline finding of its lane at the held head, each in a file one of those commits changed. A label it keeps on the re-read gets no clear comment. It reads the head and the hold comments again just before the removal. A lane that still finds the cause holds the new head again. +- `ci-status`'s review-lane lift check counts the lanes App's removal only after its own newest `stale-review-hold` comment since the hold names a head no hold comment since the label's last removal names, while none names the event's head, and only when the base commit's `clear-escalations.mjs` `recheckStaleHold`, the same predicate, passes again with the job's own token at the head that comment names, as it was when the App removed the label, including the check-signed-commits hold, other lanes App markers and a person speaking up to the removal. A later head is judged by its own review lanes, so a clean push after the clear stays green before they run. The App's comment only points at the hold; ci-status does not take its judgment on trust. diff --git a/.github/scripts/pr-merge/clear-escalations-recheck.test.mjs b/.github/scripts/pr-merge/clear-escalations-recheck.test.mjs new file mode 100644 index 0000000000..23062998b5 --- /dev/null +++ b/.github/scripts/pr-merge/clear-escalations-recheck.test.mjs @@ -0,0 +1,218 @@ +// Expected values come from the owner's needs-human brief, Part A2, and the +// coordinator's direction that ci-status re-runs the App's predicate instead +// of trusting its marker: after the lanes App removed a stale review-lane hold, +// the base commit's clear-escalations.mjs recheckStaleHold, run by ci-status with its own token, +// judges the head the App's marker names (CLEARED_SHA), which must be the +// head when the App removed the label, so a later clean push stays green +// before its lanes run (thread PRRT_kwDOTCGFQM6xmIfF). A CRITICAL thread opened +// after the clear, or a lane with no run at the cleared head, makes it fail. +// Shapes are the review lanes' (#10743). +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; + +import { recheckStaleHold as main } from "./clear-escalations.mjs"; + +const REPO = "melodic-software/claude-code-plugins"; +const ACTIONS_BOT = { login: "github-actions[bot]", type: "Bot" }; +const APP = { login: "melodic-automation-lanes[bot]", type: "Bot" }; +const PERSON = { login: "kyle-sexton", id: 153232337, type: "User" }; +const APP_ACTOR = { login: APP.login, id: 337595936, type: "Bot" }; +const MAIN = "a".repeat(40); +const HEAD = "b".repeat(40); +const OLD = "c".repeat(40); +const LATER = "d".repeat(40); + +const DIR = mkdtempSync(join(tmpdir(), "stale-review-hold-")); +after(() => rmSync(DIR, { recursive: true, force: true })); +const TRUSTED = join(DIR, "trusted-actors.json"); +writeFileSync( + TRUSTED, + JSON.stringify({ + version: 1, + actors: [ + { id: PERSON.id, login: PERSON.login, kind: "human" }, + { id: APP_ACTOR.id, login: APP.login, kind: "bot" }, + ], + }), +); +const ENV = { GITHUB_REPOSITORY: REPO, PR_NUMBER: "7", HEAD_SHA: HEAD, CLEARED_SHA: HEAD, HUMAN_GATED: "needs-human", TRUSTED_ACTORS_PATH: TRUSTED }; + +const commented = (user, body, at) => ({ event: "commented", user, actor: user, body, created_at: at, updated_at: at }); +// The timeline ci-status sees on the unlabeled event: the lane's label and +// hold at OLD, the security summary at HEAD, the App's marker and removal. +const timeline = [ + { event: "labeled", actor: ACTIONS_BOT, label: { name: "needs-human" }, created_at: "2026-10-18T10:00:00Z" }, + commented(ACTIONS_BOT, `\nHeld.\n\nclaude-lane: claude-security-review`, "2026-10-18T10:00:02Z"), + commented(ACTIONS_BOT, `\nReviewed.\n\ncritical-findings: 0\n\nclaude-lane: claude-security-review`, "2026-10-18T11:20:00Z"), + commented(APP, `\nCleared.`, "2026-10-18T12:00:00Z"), + { event: "unlabeled", actor: APP, label: { name: "needs-human" }, created_at: "2026-10-18T12:00:01Z" }, +]; +const thread = (severity, lane = "claude-security-review", { isResolved = false, path = "plugins/x/z.py", commit = HEAD } = {}) => ({ + isResolved, + path, + comments: { nodes: [{ author: { __typename: "Bot", login: "github-actions" }, body: `${severity}\n\nA finding.\n\nclaude-lane: ${lane}`, originalCommit: { oid: commit }, originalLine: 12, originalStartLine: null }] }, +}); +// The held CRITICAL finding, inline at OLD on line 12 of the file the fix changed there, resolved. +const heldFinding = () => thread("severity: CRITICAL", "claude-security-review", { isResolved: true, path: "plugins/x/y.py", commit: OLD }); +const notice = (lane) => ({ annotation_level: "notice", message: `claude-lane-review-complete lane=${lane} sha=${HEAD} findings=0 inline=0 scope=whole` }); +const LANES = { + "pr-review-hosted.yml": { check: "pr-review / claude-review-status", lane: "claude-review" }, + "pr-review-security-hosted.yml": { check: "pr-review-security / security-review", lane: "claude-security-review" }, +}; +const pushes = [ + { activity_type: "push", actor: PERSON, after: HEAD, timestamp: "2026-10-18T11:00:00Z" }, + { activity_type: "push", actor: PERSON, after: OLD, timestamp: "2026-10-18T09:50:00Z" }, +]; + +// One token answers every read, as ci-status's job token does. +function world({ threads = [thread("severity: SUGGESTION"), heldFinding()], notRun = null, head = HEAD, activity = pushes, extra = [], threadComments = [] } = {}) { + const calls = []; + const github = async (method, path) => { + calls.push(`${method} ${path}`); + const route = path.replace(/[?&]per_page=100(?:&page=\d+)?$/, ""); + if (method === "GET" && route === `/repos/${REPO}/pulls/7`) { + return { number: 7, state: "open", base: { ref: "main", sha: MAIN }, head: { sha: head, ref: "feat/x", repo: { full_name: REPO } } }; + } + if (method === "GET" && route === `/repos/${REPO}/issues/7/timeline`) { + return path.endsWith("page=1") ? [...timeline, ...extra] : []; + } + if (method === "GET" && route === `/repos/${REPO}/compare/${MAIN}...${HEAD}`) { + return { files: [{ filename: "plugins/x/y.py" }] }; + } + if (method === "GET" && route === `/repos/${REPO}/activity?ref=${encodeURIComponent("refs/heads/feat/x")}`) { + return activity; + } + if (method === "GET" && route === `/repos/${REPO}/compare/${OLD}...${HEAD}`) { + return { total_commits: 1, commits: [{ sha: HEAD, parents: [{ sha: OLD }] }], files: [{ filename: "plugins/x/y.py", patch: "@@ -12,1 +12,1 @@\n- run(user_input)\n+ run(quote(user_input))" }] }; + } + if (method === "GET" && route === `/repos/${REPO}/compare/main...${HEAD}`) { + return { total_commits: 2, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }] }] }; + } + if (method === "GET" && route === `/repos/${REPO}/commits/${HEAD}`) { + return { sha: HEAD, files: [{ filename: "plugins/x/y.py" }] }; + } + if (method === "GET" && /^\/repos\/[^/]+\/[^/]+\/pulls\/7\/comments\?since=[^&]+$/.test(route)) { + return path.endsWith("page=1") ? threadComments : []; + } + if (method === "GET" && route === `/repos/${REPO}/pulls/7/reviews`) { + return []; + } + if (method === "POST" && path === "/graphql") { + return { data: { repository: { pullRequest: { reviewThreads: { nodes: threads, pageInfo: { hasNextPage: false } } } } } }; + } + for (const [file, { check, lane }] of Object.entries(LANES)) { + const id = file.length; + if (route === `/repos/${REPO}/actions/workflows/${file}/runs?event=pull_request&head_sha=${HEAD}`) { + return { workflow_runs: file === notRun ? [] : [{ id, head_sha: HEAD, head_repository: { full_name: REPO } }] }; + } + if (route === `/repos/${REPO}/actions/runs/${id}/jobs?filter=latest`) { + return { jobs: [{ id: id * 10, name: check, status: "completed", conclusion: "success" }] }; + } + if (route === `/repos/${REPO}/check-runs/${id * 10}/annotations`) { + return [notice(lane)]; + } + } + throw new Error(`unexpected ${method} ${path}`); + }; + return { github, calls }; +} + +const run = async (w, env = ENV) => { + const logs = []; + const original = console.log; + console.log = (line) => logs.push(line); + try { + return { code: await main({ env, github: w.github }), log: logs.join("\n") }; + } finally { + console.log = original; + } +}; + +test("ci-status's recheck accepts the App's removal while the hold still no longer stands at the head", async () => { + const w = world(); + const { code, log } = await run(w); + assert.equal(code, 0, log); + assert.match(log, new RegExp(`the hold at ${OLD} no longer stands at ${HEAD}: kyle-sexton moved the head`)); + assert.ok(w.calls.every((call) => call.startsWith("GET ") || call === "POST /graphql"), "the recheck writes nothing"); +}); + +test("ci-status's recheck fails a valid marker once a CRITICAL thread opened after the clear", async () => { + const { code, log } = await run(world({ threads: [thread("severity: SUGGESTION"), thread("severity: CRITICAL")] })); + assert.equal(code, 1); + assert.match(log, /the hold stands at .*: 1 unresolved claude-security-review finding\(s\) at CRITICAL or IMPORTANT/); +}); + +test("ci-status's recheck fails a valid marker when a lane has no run at the head", async () => { + const { code, log } = await run(world({ notRun: "pr-review-security-hosted.yml" })); + assert.equal(code, 1); + assert.match(log, new RegExp(`claude-security-review has not run on ${HEAD}`)); +}); + +test("ci-status's recheck fails when the pull request's head is not the event's", async () => { + const { code, log } = await run(world({ head: "f".repeat(40) })); + assert.equal(code, 1); + assert.match(log, new RegExp(`head is no longer ${HEAD}`)); +}); + +test("ci-status's recheck fails closed on missing input", async () => { + for (const key of ["HEAD_SHA", "CLEARED_SHA", "PR_NUMBER", "HUMAN_GATED"]) { + const { code } = await run(world(), { ...ENV, [key]: "" }); + assert.equal(code, 1, key); + } +}); + +test("ci-status's recheck at a later head judges the head the App cleared, before the lanes run there and after a lanes App push", async () => { + const later = [{ activity_type: "push", actor: APP_ACTOR, after: LATER, timestamp: "2026-10-18T12:30:00Z" }, ...pushes]; + const { code, log } = await run(world({ head: LATER, activity: later }), { ...ENV, HEAD_SHA: LATER }); + assert.equal(code, 0, log); + assert.match(log, new RegExp(`the hold at ${OLD} no longer stands at ${HEAD}`)); +}); + +test("ci-status's recheck fails when a lanes App push landed the cleared head", async () => { + const appLanded = [{ activity_type: "push", actor: APP_ACTOR, after: HEAD, timestamp: "2026-10-18T11:00:00Z" }, pushes[1]]; + const { code, log } = await run(world({ activity: appLanded })); + assert.equal(code, 1); + assert.match(log, /no trusted push alone moved the head off the held head/); +}); + +test("ci-status's recheck fails when the cleared head was not the head as the App removed the label", async () => { + const moved = [{ activity_type: "push", actor: PERSON, after: LATER, timestamp: "2026-10-18T11:59:00Z" }, ...pushes]; + const { code, log } = await run(world({ head: LATER, activity: moved }), { ...ENV, HEAD_SHA: LATER }); + assert.equal(code, 1); + assert.match(log, /no trusted push alone moved the head off the held head/); +}); + +// The rest of the App's predicate, at the removal (thread PRRT_kwDOTCGFQM6xqn0Y): +// a marker comment the App wrote outside clear-escalations proves nothing. +test("ci-status's recheck fails on a signed-commits hold, another App marker or a person speaking before the removal", async () => { + for (const [extra, why] of [ + [[commented(ACTIONS_BOT, "\nA lane commit is not verified.", "2026-10-18T11:30:00Z")], "check-signed-commits holds it"], + [[commented(APP, "\n\nHeld.", "2026-10-18T11:30:00Z")], "a lanes App comment since needs-human went on carries a marker"], + [[commented(PERSON, "Do not merge this yet.", "2026-10-18T11:30:00Z")], "kyle-sexton commented or reviewed after needs-human went on"], + ]) { + const { code, log } = await run(world({ extra })); + assert.equal(code, 1, why); + assert.match(log, new RegExp(why)); + } + const reply = { user: PERSON, created_at: "2026-10-18T11:30:00Z", updated_at: "2026-10-18T11:30:00Z", body: "Keep this held." }; + const { code, log } = await run(world({ threadComments: [reply] })); + assert.equal(code, 1); + assert.match(log, /kyle-sexton commented or reviewed after needs-human went on/); +}); + +// A reply written before the removal and edited after it still counts +// (thread PRRT_kwDOTCGFQM6xqzoY). +test("ci-status's recheck fails on a person's thread reply written before the removal and edited after it", async () => { + const reply = { user: PERSON, created_at: "2026-10-18T11:30:00Z", updated_at: "2026-10-18T12:30:00Z", body: "Keep this held." }; + const { code, log } = await run(world({ threadComments: [reply] })); + assert.equal(code, 1, log); + assert.match(log, /kyle-sexton commented or reviewed after needs-human went on/); +}); + +test("ci-status's recheck ignores a person who spoke only after the removal", async () => { + const { code, log } = await run(world({ extra: [commented(PERSON, "Thanks.", "2026-10-18T12:30:00Z")] })); + assert.equal(code, 0, log); +}); diff --git a/.github/scripts/pr-merge/clear-escalations-stale-hold.test.mjs b/.github/scripts/pr-merge/clear-escalations-stale-hold.test.mjs new file mode 100644 index 0000000000..a96f5628f1 --- /dev/null +++ b/.github/scripts/pr-merge/clear-escalations-stale-hold.test.mjs @@ -0,0 +1,323 @@ +// Expected values come from the owner's needs-human brief, Part A2 +// (2026-10-29): a review-lane hold recorded against an older head clears once +// a new head's review lanes pass with no HIGH or CRITICAL finding, stays when +// the new head still carries the finding or the lanes have not passed there, +// and a lanes App push alone never clears it (only a trusted move does). +// Security-review IMPORTANT counts as HIGH (R47). Notice and comment shapes are +// the ones the review lanes post (#10743's security lane comments). +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { after, test } from "node:test"; + +import { main } from "./clear-escalations.mjs"; + +const REPO = "melodic-software/claude-code-plugins"; +const ACTIONS_BOT = { login: "github-actions[bot]", type: "Bot" }; +const APP = { login: "melodic-automation-lanes[bot]", type: "Bot" }; +const OWNER = { login: "kyle-sexton", type: "User" }; +const PERSON = { login: "kyle-sexton", id: 153232337, type: "User" }; +const APP_ACTOR = { login: APP.login, id: 337595936, type: "Bot" }; +const MAIN = "a".repeat(40); +const HEAD = "b".repeat(40); +const OLD = "c".repeat(40); +const LABELED_AT = "2026-10-18T10:00:00Z"; + +const TRUSTED_DIR = mkdtempSync(join(tmpdir(), "clear-escalations-stale-")); +after(() => rmSync(TRUSTED_DIR, { recursive: true, force: true })); +const TRUSTED = join(TRUSTED_DIR, "trusted-actors.json"); +writeFileSync( + TRUSTED, + JSON.stringify({ + version: 1, + actors: [ + { id: PERSON.id, login: PERSON.login, kind: "human" }, + { id: APP_ACTOR.id, login: APP.login, kind: "bot" }, + ], + }), +); +const ENV = { + GITHUB_REPOSITORY: REPO, + KILL_SWITCH: "proceed", + TRACKER_CONFIG: "/nonexistent/.work-item-tracker.json", + TRUSTED_ACTORS_PATH: TRUSTED, + BABYSIT_VERIFY_GH_TOKEN: "job-token", +}; + +const commented = (user, body, at) => ({ event: "commented", user, actor: user, body, created_at: at, updated_at: at }); +const hold = (sha, at = "2026-10-18T10:00:02Z") => + commented( + ACTIONS_BOT, + `\n**claude-security-review** holds this pull request for a person.\n\nclaude-lane: claude-security-review`, + at, + ); +const summary = (critical = "critical-findings: 0") => + commented( + ACTIONS_BOT, + `\n**claude-security-review** reviewed \`${HEAD}\`: 1 finding(s), 1 as inline comments.\n\n${critical}\n\nclaude-lane: claude-security-review`, + "2026-10-18T11:20:00Z", + ); +const baseTimeline = () => [{ event: "labeled", actor: ACTIONS_BOT, label: { name: "needs-human" }, created_at: LABELED_AT }, hold(OLD), summary()]; + +// Review threads, root comment first. The fixture's one open security finding +// is a SUGGESTION, so a clear shows the bar is HIGH or CRITICAL, not any finding. +// A thread carries its file and the commit its root comment was made on. +// A thread carries its file, the commit its root comment was made on and the line it names there. +const thread = (isResolved, author, body, path = "plugins/x/z.py", commit = HEAD, line = 12) => ({ + isResolved, + path, + comments: { nodes: [{ author, body, originalCommit: { oid: commit }, originalLine: line, originalStartLine: null }] }, +}); +const securityThread = (severity, isResolved = false, path = undefined, commit = undefined, line = undefined) => + thread(isResolved, { __typename: "Bot", login: "github-actions" }, `${severity}\n\nA finding.\n\nclaude-lane: claude-security-review`, path, commit, line); +const codeThread = thread(false, { __typename: "Bot", login: "github-actions" }, "A code finding.\n\nclaude-lane: claude-review"); +// The held CRITICAL finding: inline at OLD on the file the person's fix changed, now resolved. +const heldFinding = () => securityThread("severity: CRITICAL", true, "plugins/x/y.py", OLD); +const baseThreads = () => [securityThread("severity: SUGGESTION"), heldFinding(), codeThread]; +// The person's fix rewrites line 12 of plugins/x/y.py, the held finding's line. +const FIX_PATCH = "@@ -10,5 +10,5 @@ def f():\n a = 1\n b = 2\n- run(user_input)\n+ run(quote(user_input))\n c = 3\n d = 4"; +// The compare from the held head to the head: the person's one fix commit; +// from main to the head: the pull request's own commits, the held head and the fix. +const baseHeldCompare = () => ({ total_commits: 1, commits: [{ sha: HEAD, parents: [{ sha: OLD }] }], files: [{ filename: "plugins/x/y.py", patch: FIX_PATCH }] }); +const baseOwnCompare = () => ({ total_commits: 2, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, { sha: HEAD, parents: [{ sha: OLD }] }] }); + +const completeNotice = (lane, scope = "scope=whole") => ({ + annotation_level: "notice", + title: "claude-lane-review-complete", + message: `claude-lane-review-complete lane=${lane} sha=${HEAD} findings=1 inline=1 ${scope} account=fallback`, +}); +// Each review caller's newest run on HEAD and its status job. +const baseLanes = () => ({ + "pr-review-hosted.yml": { check: "pr-review / claude-review-status", conclusion: "success", notes: [completeNotice("claude-review")] }, + "pr-review-security-hosted.yml": { check: "pr-review-security / security-review", conclusion: "success", notes: [completeNotice("claude-security-review")] }, +}); + +// The branch's pushes, newest first: the person's fix after the held head. +const push = (actor, sha, at) => ({ activity_type: "push", actor, after: sha, timestamp: at }); +const basePushes = () => [push(PERSON, HEAD, "2026-10-18T11:00:00Z"), push(PERSON, OLD, "2026-10-18T09:50:00Z"), { activity_type: "branch_creation", actor: PERSON, after: "e".repeat(40), timestamp: "2026-10-18T08:00:00Z" }]; + +function world({ later = {}, timeline = baseTimeline(), threads = baseThreads(), lanes = baseLanes(), pushes = basePushes(), headFiles = [{ filename: "plugins/x/y.py" }], heldCompare = baseHeldCompare(), ownCompare = baseOwnCompare(), fixFiles = [{ filename: "plugins/x/y.py" }] } = {}) { + const calls = []; + // `later` answers the reads after the first judgment: { head, timeline }. + const seen = { pull: 0, timeline: 0 }; + const github = async (method, path, body) => { + calls.push({ method, path, body }); + const route = path.replace(/[?&]per_page=100&page=\d+$/, ""); + const first = path.endsWith("page=1"); + if (method === "GET" && route === `/repos/${REPO}/git/ref/heads/main`) { + return { ref: "refs/heads/main", object: { sha: MAIN } }; + } + if (method === "GET" && route === `/repos/${REPO}/issues?state=open&labels=needs-human`) { + return first ? [{ number: 7, pull_request: {} }] : []; + } + if (method === "GET" && route === `/repos/${REPO}/issues/7/timeline`) { + if (first) { + seen.timeline += 1; + } + return first ? (seen.timeline > 1 ? (later.timeline ?? timeline) : timeline) : []; + } + if (method === "GET" && route === `/repos/${REPO}/pulls/7`) { + seen.pull += 1; + return { number: 7, state: "open", user: OWNER, base: { ref: "main", sha: MAIN }, head: { sha: seen.pull > 1 ? (later.head ?? HEAD) : HEAD, ref: "feat/x", repo: { full_name: REPO } }, labels: [{ name: "needs-human" }] }; + } + if (method === "GET" && route === `/repos/${REPO}/compare/${MAIN}...${HEAD}`) { + return { files: headFiles }; + } + if (method === "GET" && route === `/repos/${REPO}/compare/${OLD}...${HEAD}`) { + return heldCompare; + } + if (method === "GET" && route === `/repos/${REPO}/compare/main...${HEAD}`) { + return ownCompare; + } + if (method === "GET" && route === `/repos/${REPO}/commits/${HEAD}`) { + return { sha: HEAD, files: fixFiles }; + } + if (method === "POST" && path === "/graphql") { + return { data: { repository: { pullRequest: { reviewThreads: { nodes: threads, pageInfo: { hasNextPage: false } } } } } }; + } + if (method === "GET" && /^\/repos\/[^/]+\/[^/]+\/pulls\/7\/(comments\?since=[^&]+|reviews)$/.test(route)) { + return []; + } + if (method === "POST" || method === "DELETE") { + return null; + } + throw new Error(`unexpected ${method} ${path}`); + }; + const checks = async (method, path) => { + calls.push({ method, path, token: "checks" }); + for (const [file, lane] of Object.entries(lanes)) { + const id = file.length; + if (path === `/repos/${REPO}/actions/workflows/${file}/runs?event=pull_request&head_sha=${HEAD}&per_page=100`) { + return { workflow_runs: lane === null ? [] : [{ id, head_sha: HEAD, head_repository: { full_name: REPO } }] }; + } + if (path === `/repos/${REPO}/actions/runs/${id}/jobs?filter=latest&per_page=100`) { + return { jobs: [{ id: id * 10, name: lane.check, status: "completed", conclusion: lane.conclusion }] }; + } + if (path === `/repos/${REPO}/check-runs/${id * 10}/annotations?per_page=100`) { + return lane.notes; + } + } + throw new Error(`unexpected checks ${method} ${path}`); + }; + const reader = async (method, path) => { + if (method === "GET" && path === `/repos/${REPO}/activity?ref=${encodeURIComponent("refs/heads/feat/x")}&per_page=100`) { + return pushes; + } + throw new Error(`unexpected reader ${method} ${path}`); + }; + return { github, checks, reader, writes: () => calls.filter((call) => call.method !== "GET" && call.path !== "/graphql") }; +} + +const run = async (w) => { + const logs = []; + const original = console.log; + console.log = (line) => logs.push(line); + try { + const code = await main({ env: ENV, github: w.github, checks: w.checks, reader: w.reader, verify: async () => null }); + return { code, log: logs.join("\n") }; + } finally { + console.log = original; + } +}; + +const assertKept = async (w, why) => { + const { code, log } = await run(w); + assert.equal(code, 0); + assert.deepEqual(w.writes(), []); + assert.match(log, new RegExp(`#7 kept: ${why}`)); +}; + +test("a hold at an older head comes off once a person's push passes both review lanes with no HIGH or CRITICAL finding", async () => { + const w = world(); + const { code } = await run(w); + assert.equal(code, 0); + const [comment, removal] = w.writes(); + assert.equal(w.writes().length, 2); + assert.equal(comment.path, `/repos/${REPO}/issues/7/comments`); + assert.equal(comment.body.body.split("\n")[0], ``); + assert.match(comment.body.body, new RegExp(`it held \`${OLD}\``)); + assert.match(comment.body.body, /kyle-sexton moved the head/); + assert.deepEqual(removal, { method: "DELETE", path: `/repos/${REPO}/issues/7/labels/needs-human`, body: undefined }); +}); + +test("the hold stays when the new head still carries the finding", async () => { + await assertKept(world({ timeline: [...baseTimeline(), hold(HEAD, "2026-10-18T11:30:00Z")] }), `a review lane holds the current head ${HEAD}`); + const notes = [completeNotice("claude-security-review"), { annotation_level: "notice", message: `claude-lane-hold lane=claude-security-review sha=${HEAD} criteria=critical-security label=needs-human` }]; + await assertKept(world({ lanes: { ...baseLanes(), "pr-review-security-hosted.yml": { ...baseLanes()["pr-review-security-hosted.yml"], notes } } }), `claude-security-review holds ${HEAD}`); + for (const severity of ["severity: IMPORTANT", "severity: CRITICAL", "no severity line"]) { + await assertKept(world({ threads: [...baseThreads(), securityThread(severity)] }), "1 unresolved claude-security-review finding\\(s\\) at CRITICAL or IMPORTANT"); + } + await assertKept(world({ timeline: [...baseTimeline().slice(0, 2), summary("critical-findings: 1")] }), `the security lane's summary of ${HEAD} does not read critical-findings: 0`); +}); + +test("the hold stays while the review lanes have not passed on the new head", async () => { + const lanes = baseLanes(); + await assertKept(world({ lanes: { ...lanes, "pr-review-security-hosted.yml": { ...lanes["pr-review-security-hosted.yml"], conclusion: "failure" } } }), `claude-security-review has not passed on ${HEAD}`); + await assertKept(world({ lanes: { ...lanes, "pr-review-hosted.yml": null } }), `claude-review has not run on ${HEAD}`); + await assertKept(world({ lanes: { ...lanes, "pr-review-hosted.yml": { ...lanes["pr-review-hosted.yml"], notes: [] } } }), `claude-review recorded no one review of ${HEAD}`); + const incremental = [completeNotice("claude-security-review", `scope=incremental since=${OLD}`)]; + await assertKept(world({ lanes: { ...lanes, "pr-review-security-hosted.yml": { ...lanes["pr-review-security-hosted.yml"], notes: incremental } } }), `claude-security-review reviewed only part of ${HEAD}`); +}); + +test("a lanes App push alone never clears the hold, even with both lanes passing", async () => { + for (const pushes of [ + [push(APP_ACTOR, HEAD, "2026-10-18T11:00:00Z"), ...basePushes().slice(1)], + [push(APP_ACTOR, HEAD, "2026-10-18T11:10:00Z"), ...basePushes()], + ]) { + await assertKept(world({ pushes }), "no trusted push alone moved the head off the held head"); + } +}); + +test("the hold stays when the head changes a review caller, a person spoke, or the App marked the PR since the label", async () => { + await assertKept(world({ headFiles: [{ filename: ".github/workflows/pr-review-security-hosted.yml" }] }), "the head's lane runs prove nothing: it changes a review-lane workflow"); + await assertKept(world({ timeline: [...baseTimeline(), commented(OWNER, "Wait on this.", "2026-10-18T11:40:00Z")] }), "kyle-sexton commented or reviewed after needs-human went on"); + const escalation = commented(APP, "\n\nHeld.", "2026-10-18T11:40:00Z"); + await assertKept(world({ timeline: [...baseTimeline(), escalation] }), "a lanes App comment since needs-human went on carries a marker"); +}); + +// Fixes from the security review of 990c1941. +test("an incremental review by either lane keeps the hold: a finding at the held head may not have been looked at again", async () => { + const lanes = baseLanes(); + const incremental = [completeNotice("claude-review", `scope=incremental since=${OLD}`)]; + await assertKept(world({ lanes: { ...lanes, "pr-review-hosted.yml": { ...lanes["pr-review-hosted.yml"], notes: incremental } } }), `claude-review reviewed only part of ${HEAD}`); +}); + +test("an open code-lane thread at CRITICAL or IMPORTANT keeps the hold, and so does an unrated one when the hold names the code lane", async () => { + const codeAt = (severity) => thread(false, { __typename: "Bot", login: "github-actions" }, `${severity}\n\nA code finding.\n\nclaude-lane: claude-review`); + for (const severity of ["severity: IMPORTANT", "severity: CRITICAL"]) { + await assertKept(world({ threads: [...baseThreads(), codeAt(severity)] }), "1 unresolved claude-review finding\\(s\\) at CRITICAL or IMPORTANT"); + } + const codeHold = commented(ACTIONS_BOT, `\nHeld.\n\nclaude-lane: claude-review`, "2026-10-18T10:00:02Z"); + await assertKept(world({ timeline: [baseTimeline()[0], codeHold, summary()] }), "1 unresolved claude-review finding\\(s\\) at CRITICAL or IMPORTANT"); +}); + +// A kept label gets no comment saying it came off (threads PRRT_kwDOTCGFQM6xqa-M, PRRT_kwDOTCGFQM6xqmzn). +test("a hold written, or a head pushed, after the judgment keeps the label with no clear comment", async () => { + const after = (later) => world({ later }); + for (const [w, why] of [ + [after({ timeline: [...baseTimeline(), hold(HEAD, "2026-10-18T11:50:00Z")] }), "a review lane wrote a hold since it was judged"], + [after({ head: "f".repeat(40) }), `its head moved to ${"f".repeat(40)} since it was judged`], + ]) { + const { code, log } = await run(w); + assert.equal(code, 0); + assert.deepEqual(w.writes(), []); + assert.match(log, new RegExp(`#7 kept: ${why}`)); + } +}); + +// Fixes from the security review of e18df308 (thread PRRT_kwDOTCGFQM6xmJXW): a +// lane's later verdict alone never lifts a critical-security hold, and a push +// that only merges the base is no fix. +test("a critical-security hold stays for a person when its lane left no CRITICAL inline finding at the held head", async () => { + await assertKept(world({ threads: [securityThread("severity: SUGGESTION"), codeThread] }), `the critical-security hold at ${OLD} has no CRITICAL inline claude-security-review finding there`); + await assertKept( + world({ threads: [securityThread("severity: SUGGESTION"), securityThread("severity: CRITICAL", true, "plugins/x/y.py", "f".repeat(40)), codeThread] }), + `the critical-security hold at ${OLD} has no CRITICAL inline claude-security-review finding there`, + ); +}); + +test("a critical-security hold stays when no push since the held head changed the held finding's file", async () => { + await assertKept(world({ fixFiles: [{ filename: "plugins/x/other.py" }] }), `plugins/x/y.py, which holds a CRITICAL finding at ${OLD}, is unchanged since then`); +}); + +// Thread PRRT_kwDOTCGFQM6xwRR5: an edit elsewhere in the held file (a comment, +// whitespace) is no fix of the flagged code. +test("a critical-security hold stays when no push since the held head changed the held finding's lines", async () => { + const elsewhere = "@@ -40,3 +40,3 @@\n x = 1\n-# old note\n+# new note\n y = 2"; + const files = [{ filename: "plugins/x/y.py", patch: elsewhere }]; + const why = `the lines of plugins/x/y.py that hold a CRITICAL finding at ${OLD} are unchanged since then`; + await assertKept(world({ heldCompare: { ...baseHeldCompare(), files } }), why); + await assertKept(world({ heldCompare: { ...baseHeldCompare(), files: [{ filename: "plugins/x/y.py" }] } }), why); + const fileLevel = [securityThread("severity: SUGGESTION"), securityThread("severity: CRITICAL", true, "plugins/x/y.py", OLD, null), codeThread]; + await assertKept(world({ threads: fileLevel }), why); +}); + +test("the hold stays when only merges moved the head off the held head, or the compare does not read whole", async () => { + const merge = { sha: HEAD, parents: [{ sha: OLD }, { sha: MAIN }] }; + await assertKept(world({ heldCompare: { ...baseHeldCompare(), commits: [merge] } }), `no commit of the pull request's own but merges moved the head off ${OLD}`); + await assertKept(world({ heldCompare: { ...baseHeldCompare(), total_commits: 2 } }), `the commits from ${OLD} or main to ${HEAD} did not read whole`); +}); + +// An Update branch once main has moved: the compare from the held head lists +// main's own single-parent commits beside the merge (threads +// PRRT_kwDOTCGFQM6xqaIb, PRRT_kwDOTCGFQM6xqa-F, PRRT_kwDOTCGFQM6xqn0R). +test("an Update branch that brings in main's commits, even ones changing the held file, lifts no hold", async () => { + const mainCommit = { sha: "9".repeat(40), parents: [{ sha: MAIN }] }; + const merge = { sha: HEAD, parents: [{ sha: OLD }, { sha: mainCommit.sha }] }; + const heldCompare = { total_commits: 2, commits: [mainCommit, merge] }; + const ownCompare = { total_commits: 2, commits: [{ sha: OLD, parents: [{ sha: MAIN }] }, merge] }; + await assertKept(world({ heldCompare, ownCompare }), `no commit of the pull request's own but merges moved the head off ${OLD}`); + const gate = commented(ACTIONS_BOT, `\nHeld.\n\nclaude-lane: claude-security-review`, "2026-10-18T10:00:02Z"); + await assertKept(world({ heldCompare, ownCompare, timeline: [baseTimeline()[0], gate, summary()] }), `no commit of the pull request's own but merges moved the head off ${OLD}`); +}); + +test("a hold other than critical-security needs no inline finding at the held head", async () => { + const gate = commented(ACTIONS_BOT, `\nHeld.\n\nclaude-lane: claude-security-review`, "2026-10-18T10:00:02Z"); + const w = world({ timeline: [baseTimeline()[0], gate, summary()], threads: [securityThread("severity: SUGGESTION"), codeThread] }); + await run(w); + assert.deepEqual( + w.writes().map(({ method }) => method), + ["POST", "DELETE"], + ); +}); diff --git a/.github/scripts/pr-merge/clear-escalations.mjs b/.github/scripts/pr-merge/clear-escalations.mjs index e4478ef001..03a718e28b 100755 --- a/.github/scripts/pr-merge/clear-escalations.mjs +++ b/.github/scripts/pr-merge/clear-escalations.mjs @@ -66,8 +66,9 @@ // activity API, read with the job token) is by an id trusted-actors.json // lists as a `human`, never the lanes App, the newest landing the head. // Comment text never counts. Removing the label approves nothing: ci-status -// fails a review-lane hold an App lifted, and a hold path needs a trusted -// person's approving review. +// fails a review-lane hold an App lifted, unless the App lifted it as +// stale-review-hold (below), and a hold path needs a trusted person's +// approving review. // A listing that may be cut short, or a mergeable state not yet computed, // counts as the reason still holding. // - No person wrote or edited a review comment, or submitted a review, since @@ -97,6 +98,18 @@ // must clear. A marker whose head does not parse as 40-hex keeps the label, // and so does any other marker, as above. // +// When a review lane (github-actions[bot] or claude[bot]) added the label +// last, it clears as `stale-review-hold` when staleHoldEvidence (below) finds every +// hold since the label was last removed no longer standing at the current head +// (a trusted move off the held head, both lanes' whole reviews passing there +// with no HIGH or CRITICAL finding), check-signed-commits does not hold it, no +// lanes App marker but an address-feedback round's was written since the +// label, and no person spoke since. Just before the delete, the head and the +// hold comments are read again: a moved head or a new hold keeps the label. +// ci-status runs the base commit's copy of this module's recheckStaleHold +// before it counts the App's removal. A lane that still finds the cause holds the new +// head again with its own comment and label. +// // Before the label comes off, the App posts one comment naming the cleared // reason and the head; a sweep that finds that comment for this escalation // and head only retries the removal. At most CLEARS_PER_RUN a run. @@ -158,6 +171,7 @@ import { escalationRoute, failingChecks, foreignMarker, + HOLD_LINE, isApp, labelHistory, LANE_STOPPED, @@ -169,13 +183,14 @@ import { personSince, RESUME_KEY, reviewEntries, + ROUND_MARKER, ROUTES, same, sinceParam, spoken, } from "../pr-unblock/gate-source.mjs"; import { isProtectedPath } from "../pr-unblock/thread-scope.mjs"; -import { LANES_APP_LOGIN as DEFAULT_BOT } from "./merge-holds.mjs"; +import { LANES_APP_LOGIN as DEFAULT_BOT, laneChange, REVIEW_LANES } from "./merge-holds.mjs"; import { pythonVerify, READER_HOLDS, REFUSAL_MARKER, refusalBody, rotated, verifyHold } from "./enable-auto-merge.mjs"; const REASON = /^- Reason: `([a-z-]+)`$/m; @@ -448,40 +463,11 @@ async function ciPasses(context) { return `${moved}, and no check other than ${AGGREGATE} fails, runs or was cancelled at it`; } -// Whether trusted pushes alone moved `pull`'s head off every resume key since -// the label went on: the head is this repository's (the activity API shows no -// fork push), its newest push since `since` landed the head, and every push -// or other activity on the branch since then is by an id `trusted` holds (a -// `human` of trusted-actors.json; never the lanes App, whose own push would -// lift its own stop). The page must -// reach back past `since`, or the move stays unread. A description, or null. -async function trustedMove({ read, repo, pull, keys, since, trusted }) { - const head = pull.head.sha; - const ref = String(pull.head?.ref ?? ""); - if (trusted === null || keys.includes(head) || pull.head?.repo?.full_name !== repo || ref === "") { - return null; - } - const activity = await read((reader) => reader("GET", `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${ref}`)}&per_page=100`)); - if (!Array.isArray(activity)) { - return null; - } - const at = (item) => Date.parse(String(item?.timestamp)); - const recent = activity.filter((item) => !(at(item) <= since)).sort((a, b) => at(b) - at(a)); - if (recent.length === 0 || recent.length === activity.length || recent[0].after !== head) { - return null; - } - if (recent.some((item) => !trusted.has(item?.actor?.id))) { - return null; - } - const by = [...new Set(recent.map((item) => item.actor.login))].join(", "); - return `${by} moved the head from the resume key to ${head}`; -} - // The review lanes reviewLanesClean reads, as pr-require-checks.yml's // `Require both review lanes on this head` step names them: the workflow, the // lane its completion record names, its status job, and whether only a whole // review counts. -const REVIEW_LANES = Object.freeze([ +const LANE_RECORDS = Object.freeze([ { file: "pr-review-hosted.yml", lane: "claude-review", job: "pr-review / claude-review-status", whole: false }, { file: "pr-review-security-hosted.yml", lane: "claude-security-review", job: "pr-review-security / security-review", whole: true }, ]); @@ -497,7 +483,6 @@ const REVIEW_RECORD = const SECURITY_REVIEWED_HEAD = /^$/; const MARKER_AUTHOR = "github-actions[bot]"; const CRITICAL_FINDINGS = /^\s*`?critical-findings: (\d{1,6})`?\s*$/gm; -const SEVERITY = /^\s*`?severity: (CRITICAL|IMPORTANT|SUGGESTION)`?\s*$/gm; const newestById = (items) => items.reduce((newest, item) => (newest === undefined || item.id > newest.id ? item : newest), undefined); // Whether both review lanes read `pull`'s head clean: a description, or null @@ -514,7 +499,7 @@ const newestById = (items) => items.reduce((newest, item) => (newest === undefin async function reviewLanesClean(context) { const { read, repo, pull, timeline } = context; const head = pull.head.sha; - for (const { file, lane, job, whole } of REVIEW_LANES) { + for (const { file, lane, job, whole } of LANE_RECORDS) { const scope = await read(async (reader) => { const runs = (await reader("GET", `/repos/${repo}/actions/workflows/${file}/runs?head_sha=${head}&event=pull_request&per_page=100`))?.workflow_runs; if (!Array.isArray(runs) || runs.length >= 100) { @@ -654,7 +639,9 @@ async function unresolvedThreads({ github, repo, pull }) { const keepNote = (label) => `To keep a person on it, add \`${label}\` back: a label a person adds is never removed.`; export const clearedComment = (reason, head, why, label) => - reason === REFUSAL_CLEARED + reason === STALE_HOLD + ? `${clearedMarker(reason, head)}\nA review lane held this pull request at an earlier head, and that hold no longer stands at \`${head}\` (\`${STALE_HOLD}\`): ${why}. pr-merge removed \`${label}\`; a review lane that still finds the cause holds the head again. ${keepNote(label)}` + : reason === REFUSAL_CLEARED ? `${clearedMarker(reason, head)}\nThe pr-merge lane left this pull request for a person, and at \`${head}\` that no longer holds (\`${REFUSAL_CLEARED}\`): ${why}. pr-merge removed \`${label}\`, so the next scheduled run judges it again. ${keepNote(label)}` : reason === HEAD_MOVED ? `${clearedMarker(reason, head)}\nThe pr-merge lane escalated this pull request, and its head has moved (\`${HEAD_MOVED}\`): ${why}. pr-merge removed \`${label}\` so it judges \`${head}\` afresh, and escalates again if its refusal still holds. ${keepNote(label)}` @@ -731,10 +718,537 @@ function pairedWithPrMerge(appComments, since) { return ours && (since - newest <= PAIRED_MS || escalated); } +// A STALE HOLD (staleHoldEvidence, which clear-escalations runs before the +// App removes the label and ci-status runs again through recheckStaleHold). +// The holds since the label was last removed no longer stand only when every +// hold comment names a head (`sha=`), none names the current one, and at the +// judged head (the current one; for ci-status, the head the App cleared): +// - a trusted move took the branch off the newest held head: every push since +// the one that landed it is by a `human` of trusted-actors.json, never the +// lanes App; +// - a push since each held head is not a merge, so an `Update branch` alone +// lifts nothing, and a critical-security hold's lane left at least one +// CRITICAL inline finding at its held head, each in a file a pushed commit +// changed and on lines changed since: a finding only the summary carried, +// or one whose lines no push touched, stays for a person; +// - the head changes neither review caller (merge-holds laneChange), which a +// pull_request run takes from the head and so could forge its notices; +// - both review lanes passed: each caller's newest pull_request run on the +// head, from this repository, has its status job succeeded, recording one +// review-complete notice for the lane and the head and no hold notice; +// - every lane's review was whole, so a finding at an older head was looked +// at again, and none leaves a HIGH or CRITICAL finding: the security lane's +// summary reads `critical-findings: 0`; a strict lane (the security lane, or +// one a hold names) counted every finding inline; and no unresolved thread a +// lane opened is at CRITICAL or IMPORTANT (IMPORTANT counts as HIGH), or, +// for a strict lane, has no one readable `severity:` line. +// A read that fails, or a listing that may be cut short, keeps the hold. + +export const STALE_HOLD = "stale-review-hold"; +// Each review caller's lane id, as its notices and comments name it. +const LANE_OF = Object.freeze({ "pr-review-hosted.yml": "claude-review", "pr-review-security-hosted.yml": "claude-security-review" }); +const SECURITY_LANE = "claude-security-review"; +// The security lane's per-finding and summary severity lines +// (/review:security-review "Reporting"; babysit_review_evidence's SEVERITY_RE +// and SUMMARY_CRITICAL_RE). +const SEVERITY = /^\s*`?severity: (CRITICAL|IMPORTANT|SUGGESTION)`?\s*$/gm; +const CRITICAL_COUNT = /^\s*`?critical-findings: (\d{1,6})`?\s*$/gm; +const HOLD_LANE = /^$/; +// The most files GitHub lists for a commit; a listing that long may be cut short. +const COMPARE_FILES = 300; +// The most fix commits whose files a critical-security clear reads (judgment). +const FIX_COMMITS = 20; +const LANE_TAG = /^\s*`?claude-lane: ([\w-]+)`?\s*$/m; +const REVIEW_THREAD_PAGES = 10; +const REVIEW_THREADS_QUERY = `query($owner: String!, $name: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $after) { + nodes { isResolved path comments(first: 1) { nodes { author { __typename login } body originalCommit { oid } originalLine originalStartLine } } } + pageInfo { hasNextPage endCursor } + } + } + } +}`; + +// The review lane a review comment (GraphQL author) is from: a Claude App +// comment's tag, ANY_LANE when it has none, or a github-actions comment's last +// non-blank line as its footer (babysit_review_evidence._lane_comment); null +// for any other comment. +const ANY_LANE = "*"; +function commentLane(author, body) { + if (author?.__typename !== "Bot") { + return null; + } + const text = String(body ?? ""); + if (["claude", "claude[bot]"].includes(author.login)) { + return LANE_TAG.exec(text)?.[1] ?? ANY_LANE; + } + const last = lines(text).filter((line) => line.trim() !== "").at(-1) ?? ""; + return ["github-actions", "github-actions[bot]"].includes(author.login) ? (LANE_TAG.exec(last)?.[1] ?? null) : null; +} + +// Each review thread a review lane opened, as { lane, severity, resolved, +// path, commit, lines } (its root comment's `severity:` values, the commit it +// was made on and the [first, last] lines it names there, or null); null when the listing does not read or may be cut short. +async function laneThreads({ github, repo, number }) { + const [owner, name] = repo.split("/"); + const open = []; + let after = null; + for (let page = 0; page < REVIEW_THREAD_PAGES; page += 1) { + const threads = (await github("POST", "/graphql", { query: REVIEW_THREADS_QUERY, variables: { owner, name, number, after } }))?.data?.repository?.pullRequest + ?.reviewThreads; + if (!Array.isArray(threads?.nodes)) { + return null; + } + for (const thread of threads.nodes) { + const root = thread?.comments?.nodes?.[0]; + if (thread?.isResolved === true && root === undefined) { + continue; + } + if (typeof thread?.isResolved !== "boolean" || root === undefined) { + return null; + } + const lane = commentLane(root.author, root.body); + if (lane !== null) { + open.push({ + lane, + severity: [...String(root.body ?? "").matchAll(SEVERITY)].map((match) => match[1]), + resolved: thread.isResolved, + path: thread.path ?? null, + commit: root.originalCommit?.oid ?? null, + lines: Number.isInteger(root.originalLine) ? [root.originalStartLine ?? root.originalLine, root.originalLine] : null, + }); + } + } + if (!threads.pageInfo?.hasNextPage) { + return open; + } + after = threads.pageInfo.endCursor; + } + return null; +} + +// One review caller's verdict at `head`, read with the job token: { findings, +// inline, whole } from its review-complete notice, or { held } saying why it +// has not passed there. +async function laneReview({ checks, repo, head }, [file, check]) { + const lane = LANE_OF[file]; + try { + const runs = (await checks("GET", `/repos/${repo}/actions/workflows/${file}/runs?event=pull_request&head_sha=${head}&per_page=100`))?.workflow_runs; + if (!Array.isArray(runs) || runs.length >= 100) { + return { held: `${lane}'s runs on ${head} did not read whole` }; + } + const run = runs.filter((item) => item?.head_sha === head && item.head_repository?.full_name === repo).sort((a, b) => b.id - a.id)[0]; + if (run === undefined) { + return { held: `${lane} has not run on ${head}` }; + } + const jobs = (await checks("GET", `/repos/${repo}/actions/runs/${run.id}/jobs?filter=latest&per_page=100`))?.jobs; + const job = (Array.isArray(jobs) ? jobs : []).filter((item) => item?.name === check).sort((a, b) => b.id - a.id)[0]; + if (job?.status !== "completed" || job.conclusion !== "success") { + return { held: `${lane} has not passed on ${head}` }; + } + const notes = await checks("GET", `/repos/${repo}/check-runs/${job.id}/annotations?per_page=100`); + if (!Array.isArray(notes) || notes.length >= 100) { + return { held: `${lane}'s notices on ${head} did not read whole` }; + } + const messages = notes.filter((note) => note?.annotation_level === "notice").map((note) => String(note.message ?? "")); + if (messages.some((message) => message.startsWith("claude-lane-hold ") && message.includes(` sha=${head} `))) { + return { held: `${lane} holds ${head}` }; + } + const done = new RegExp( + `^claude-lane-review-complete lane=${lane} sha=${head} findings=(\\d{1,6}) inline=(\\d{1,6}) scope=(whole|incremental since=[0-9a-f]{40})(?: account=fallback)?$`, + ); + const reviewed = messages.map((message) => done.exec(message)).filter((match) => match !== null); + if (reviewed.length !== 1) { + return { held: `${lane} recorded no one review of ${head}` }; + } + return { lane, findings: Number(reviewed[0][1]), inline: Number(reviewed[0][2]), whole: reviewed[0][3] === "whole" }; + } catch (error) { + return { held: `${lane}'s run on ${head} did not read: ${String(error?.message ?? error).replaceAll(/[\r\n%]/g, " ")}` }; + } +} + +// Why a lane's review of `head` may leave a HIGH or CRITICAL finding, or null +// when none does. Every lane's review must be whole, so a finding at an older +// head was looked at again. A lane is strict when it is the security lane or a +// hold names it: every finding it counted must be inline, and an open thread +// of it with no one readable `severity:` line counts as CRITICAL. Any lane's +// open thread at CRITICAL or IMPORTANT counts (IMPORTANT as HIGH). +function findingsHeld({ head, timeline, threads }, reviews, heldLanes) { + for (const review of reviews) { + if (!review.whole) { + return `${review.lane} reviewed only part of ${head}`; + } + if ((review.lane === SECURITY_LANE || heldLanes.has(review.lane)) && review.findings > review.inline) { + return `${review.lane} reported ${review.findings - review.inline} finding(s) at ${head} with no inline severity`; + } + } + const marker = ``; + const summary = timeline.filter((item) => item.event === "commented" && isApp(item.user, "github-actions[bot]") && lines(item.body)[0] === marker); + const counts = summary.length === 1 ? [...String(summary[0].body).matchAll(CRITICAL_COUNT)].map((match) => match[1]) : []; + if (counts.length !== 1 || counts[0] !== "0") { + return `the security lane's summary of ${head} does not read critical-findings: 0`; + } + const open = threads.filter((thread) => !thread.resolved); + for (const { lane } of reviews) { + const strict = lane === SECURITY_LANE || heldLanes.has(lane); + const blocking = open.filter( + (thread) => + (thread.lane === lane || thread.lane === ANY_LANE) && + (thread.severity.length === 1 ? thread.severity[0] !== "SUGGESTION" : strict || thread.severity.some((value) => value !== "SUGGESTION")), + ).length; + if (blocking > 0) { + return `${blocking} unresolved ${lane} finding(s) at CRITICAL or IMPORTANT`; + } + } + return null; +} + +// Whether trusted pushes alone moved `pull`'s head off every resume key since +// the label went on: the head is this repository's (the activity API shows no +// fork push), its newest push since `since` landed the head, and every push +// or other activity on the branch since then is by an id `trusted` holds (a +// `human` of trusted-actors.json; never the lanes App, whose own push would +// lift its own stop). The page must +// reach back past `since`, or the move stays unread. A description, or null. +export async function trustedMove({ read, repo, pull, keys, since, trusted }) { + const head = pull.head.sha; + const ref = String(pull.head?.ref ?? ""); + if (trusted === null || keys.includes(head) || pull.head?.repo?.full_name !== repo || ref === "") { + return null; + } + const activity = await read((reader) => reader("GET", `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${ref}`)}&per_page=100`)); + if (!Array.isArray(activity)) { + return null; + } + const at = (item) => Date.parse(String(item?.timestamp)); + const recent = activity.filter((item) => !(at(item) <= since)).sort((a, b) => at(b) - at(a)); + if (recent.length === 0 || recent.length === activity.length || recent[0].after !== head) { + return null; + } + if (recent.some((item) => !trusted.has(item?.actor?.id))) { + return null; + } + const by = [...new Set(recent.map((item) => item.actor.login))].join(", "); + return `${by} moved the head from the resume key to ${head}`; +} + +// The review-lane hold comments written or edited since `lifted`, the label's +// last removal before it went on: { shas, lanes, critical }, a sha undefined +// when its marker names no head, and `critical` the { sha, lane } of each +// hold whose criteria include critical-security. +export function holdsSince(timeline, lifted) { + const holds = timeline + .filter((item) => laneHold(item) !== undefined && !(Math.max(Date.parse(item.created_at), Date.parse(item.updated_at ?? item.created_at)) < lifted)) + .map((item) => { + const marker = laneHold(item); + return { sha: HOLD_LINE.exec(marker)?.[1], lane: HOLD_LANE.exec(marker)?.[1], criteria: (HOLD_CRITERIA.exec(marker)?.[1] ?? "").split(",") }; + }); + return { + shas: holds.map((hold) => hold.sha), + lanes: new Set(holds.map((hold) => hold.lane)), + critical: holds.filter((hold) => hold.criteria.includes("critical-security")).map(({ sha, lane }) => ({ sha, lane })), + }; +} + +// A compare as { commits, files }, or null when its commits do not read whole. +async function compareCommits(github, repo, from, to) { + const compare = await github("GET", `/repos/${repo}/compare/${from}...${to}`); + const commits = compare?.commits; + return Array.isArray(commits) && commits.length === compare.total_commits && commits.every((commit) => SHA.test(String(commit?.sha))) + ? { commits, files: Array.isArray(compare.files) ? compare.files : [] } + : null; +} + +// Whether a unified-diff `patch` removes, or inserts beside, an old-side line +// in [first, last]. +export function patchTouches(patch, [first, last]) { + let old = 0; + for (const line of String(patch).split("\n")) { + const hunk = /^@@ -(\d+)/.exec(line); + if (hunk !== null) { + old = Number(hunk[1]); + } else if (line.startsWith("-")) { + if (old >= first && old <= last) { + return true; + } + old += 1; + } else if (line.startsWith("+")) { + if (old >= first && old - 1 <= last) { + return true; + } + } else if (!line.startsWith("\\")) { + old += 1; + } + } + return false; +} + +// Why the pushes from held head `held` to `head` fix nothing a hold could +// name, or null. Only the pull request's own commits since `held` count (not +// reachable from the base branch, so an `Update branch` brings in none), and +// one of them must not be a merge. A critical-security hold's lane must have +// left a CRITICAL inline finding at `held`, each in a file one of those +// commits changed, on lines the diff from `held` changed. A listing that does not read whole keeps the hold. +async function heldCauseUnchanged({ github, repo, base, head, threads }, held, critical) { + const since = await compareCommits(github, repo, held, head); + const own = await compareCommits(github, repo, base, head); + if (since === null || own === null) { + return `the commits from ${held} or ${base} to ${head} did not read whole`; + } + const mine = new Set(own.commits.map((commit) => commit.sha)); + const fixes = since.commits.filter((commit) => mine.has(commit.sha) && Array.isArray(commit.parents) && commit.parents.length === 1); + if (fixes.length === 0) { + return `no commit of the pull request's own but merges moved the head off ${held}`; + } + const holds = critical.filter((hold) => hold.sha === held); + if (holds.length === 0) { + return null; + } + if (fixes.length > FIX_COMMITS) { + return `more than ${FIX_COMMITS} commits moved the head off ${held}`; + } + const changed = new Set(); + for (const { sha } of fixes) { + const files = (await github("GET", `/repos/${repo}/commits/${sha}`))?.files; + if (!Array.isArray(files) || files.length >= COMPARE_FILES) { + return `the files of ${sha} did not read whole`; + } + for (const file of files) { + changed.add(file?.filename); + } + } + for (const { lane } of holds) { + const findings = threads.filter((thread) => thread.lane === lane && thread.commit === held && thread.severity.includes("CRITICAL")); + if (findings.length === 0) { + return `the critical-security hold at ${held} has no CRITICAL inline ${lane} finding there`; + } + const unchanged = findings.find((thread) => !changed.has(thread.path)); + if (unchanged !== undefined) { + return `${unchanged.path}, which holds a CRITICAL finding at ${held}, is unchanged since then`; + } + const untouched = findings.find((thread) => { + const patch = since.files.find((file) => file?.filename === thread.path)?.patch; + return thread.lines === null || typeof patch !== "string" || !patchTouches(patch, thread.lines); + }); + if (untouched !== undefined) { + return `the lines of ${untouched.path} that hold a CRITICAL finding at ${held} are unchanged since then`; + } + } + return null; +} + +// Whether the holds since `lifted` no longer stand at `head` (`pull`'s head +// unless named): { held, holds, moved } (the held heads, how many hold +// comments, who moved the head), or { reason } saying why they stand. Only +// pushes up to `until` count, and the newest of them must land `head`, so +// ci-status judges the head the App cleared as it was at the removal. +// `reader` reads the branch activity and `checks` the lanes' runs; null +// `reader` or `trusted` keeps them. +export async function staleHoldEvidence({ github, checks, reader, repo, number, pull, timeline, lifted, trusted, head = pull.head.sha, until = Infinity }) { + const { shas, lanes, critical } = holdsSince(timeline, lifted); + if (shas.length === 0) { + return { reason: "no review-lane hold comment stands since the label went on" }; + } + if (shas.includes(undefined)) { + return { reason: "a review-lane hold names no head this can read" }; + } + for (const sha of new Set([head, pull.head.sha])) { + if (shas.includes(sha)) { + return { reason: `a review lane holds the current head ${sha}` }; + } + } + const changed = laneChange(await github("GET", `/repos/${repo}/compare/${pull.base.sha}...${head}`)); + if (changed !== null) { + return { reason: `the head's lane runs prove nothing: it changes ${changed}` }; + } + // The push that landed the newest held head anchors the trusted move. + const ref = String(pull.head?.ref ?? ""); + let activity = null; + if (reader !== null && ref !== "") { + try { + activity = await reader("GET", `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${ref}`)}&per_page=100`); + } catch { + activity = null; + } + } + activity = Array.isArray(activity) ? activity.filter((item) => !(Date.parse(String(item?.timestamp)) > until)) : null; + const landed = (activity ?? []).filter((item) => shas.includes(item?.after)).map((item) => Date.parse(String(item.timestamp))); + if (landed.length === 0 || !landed.every(Number.isFinite)) { + return { reason: "the push that landed the held head is not on the branch's activity page" }; + } + const judged = { ...pull, head: { ...pull.head, sha: head } }; + const moved = await trustedMove({ read: (fn) => fn(async () => activity), repo, pull: judged, keys: shas, since: Math.max(...landed), trusted }); + if (moved === null) { + return { reason: "no trusted push alone moved the head off the held head" }; + } + const reviews = []; + for (const lane of REVIEW_LANES) { + const review = await laneReview({ checks, repo, head }, lane); + if (review.held !== undefined) { + return { reason: review.held }; + } + reviews.push(review); + } + const threads = await laneThreads({ github, repo, number }); + if (threads === null) { + return { reason: "the review thread listing may be cut short" }; + } + const findings = findingsHeld({ head, timeline, threads }, reviews, lanes); + if (findings !== null) { + return { reason: findings }; + } + for (const held of new Set(shas)) { + const unchanged = await heldCauseUnchanged({ github, repo, base: pull.base.ref, head, threads }, held, critical); + if (unchanged !== null) { + return { reason: unchanged }; + } + } + return { held: [...new Set(shas)], holds: shas.length, moved }; +} + +// ci-status's recheck (pr-require-checks.yml, the hold-lift step, running the +// base commit's copy of this module with its own token): after the lanes App +// removed a review-lane hold as STALE_HOLD, 0 when the hold no longer stands +// at CLEARED_SHA, the head the App's marker names, judged as it was when the +// App removed the label (its newest push then landed that head), 1 when it +// stands or anything did not read. A later head is judged by its own lanes, +// as after a person's lift, so a clean push stays green before they run. +// Environment: GH_TOKEN, GITHUB_REPOSITORY, PR_NUMBER, HEAD_SHA (the event's +// head, still the pull request's), CLEARED_SHA, HUMAN_GATED (the label), +// TRUSTED_ACTORS_PATH, GITHUB_API_URL. +export async function recheckStaleHold({ env = process.env, github } = {}) { + try { + const repo = env.GITHUB_REPOSITORY ?? ""; + const number = Number(env.PR_NUMBER); + const label = env.HUMAN_GATED ?? ""; + const cleared = env.CLEARED_SHA ?? ""; + if (!/^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/.test(repo) || !Number.isSafeInteger(number) || number < 1 || !SHA.test(env.HEAD_SHA ?? "") || !SHA.test(cleared) || label === "") { + throw new Error("GITHUB_REPOSITORY, PR_NUMBER, HEAD_SHA, CLEARED_SHA or HUMAN_GATED is not set"); + } + github ??= createGitHub({ token: env.GH_TOKEN, apiUrl: env.GITHUB_API_URL || undefined }); + const trusted = new Set(loadTrustedList(env.TRUSTED_ACTORS_PATH).humans); + const pull = await github("GET", `/repos/${repo}/pulls/${number}`); + if (pull?.head?.sha !== env.HEAD_SHA || pull.head?.repo?.full_name !== repo) { + console.log(`stale-review-hold: #${number}'s head is no longer ${env.HEAD_SHA}`); + return 1; + } + const timeline = await paginate(github, `/repos/${repo}/issues/${number}/timeline`); + const { since, lifted } = labelHistory(timeline, label); + const removed = timeline + .filter((item) => item.event === "unlabeled" && same(item.label?.name, label)) + .map((item) => Date.parse(item.created_at)) + .filter((time) => time >= since); + if (removed.length === 0) { + console.log(`stale-review-hold: no removal of ${label} since it went on is recorded`); + return 1; + } + // Judged as the App saw it: the timeline, review comments and reviews + // written up to the removal (an edit after it still counts), whose actor + // ci-status checked is the lanes App. + const until = Math.min(...removed); + const removal = timeline.find((item) => item.event === "unlabeled" && same(item.label?.name, label) && Date.parse(item.created_at) === until); + const written = (item) => !(Date.parse(item?.created_at ?? item?.submitted_at ?? "") > until); + const seen = timeline.filter(written); + const stop = + standingStop({ timeline: seen, lifted, since, bot: removal?.actor?.login ?? "", label }) ?? + ((speaker) => (speaker === undefined ? null : spokeReason(speaker, label)))( + personSince( + reviewEntries({ + comments: (await paginate(github, `/repos/${repo}/pulls/${number}/comments?since=${sinceParam(since - 1000)}`)).filter(written), + reviews: (await paginate(github, `/repos/${repo}/pulls/${number}/reviews`)).filter(written), + }), + since, + ), + ); + if (stop !== null) { + console.log(`stale-review-hold: the hold stands at ${cleared}: ${stop}`); + return 1; + } + const evidence = await staleHoldEvidence({ github, checks: github, reader: github, repo, number, pull, timeline: seen, lifted, trusted, head: cleared, until }); + if (evidence.reason !== undefined) { + console.log(`stale-review-hold: the hold stands at ${cleared}: ${evidence.reason}`); + return 1; + } + console.log(`stale-review-hold: the hold at ${evidence.held.join(", ")} no longer stands at ${cleared}: ${evidence.moved}`); + return 0; + } catch (error) { + console.log(`stale-review-hold: did not read: ${String(error?.message ?? error).replaceAll(/[\r\n%]/g, " ")}`); + return 1; + } +} + +// The accounts a review lane's hold step adds the label as. +const LANE_LABELERS = Object.freeze(["github-actions[bot]", "claude[bot]"]); +// A label a review lane added: what to clear as STALE_HOLD, or { reason } +// saying why nothing is. +// Why something besides the review-lane holds keeps the label, from the +// timeline, or null: check-signed-commits' hold, a lanes App (`bot`) marker +// other than an address-feedback round's or a clear's since the label went +// on, or a person who spoke since. clear-escalations and ci-status's recheck +// both run it. +function standingStop({ timeline, lifted, since, bot, label }) { + if (openSignedCommitsHold(timeline, lifted) !== undefined) { + return "check-signed-commits holds it: a lane commit could not be verified"; + } + const appComments = timeline.filter((item) => item.event === "commented" && isApp(item.user, bot)); + const foreign = foreignMarker(appComments, since, (line) => line === ROUND_MARKER); + if (foreign !== undefined) { + return `a lanes App comment since ${label} went on carries a marker: ${foreign.slice(0, 120)}`; + } + const speaker = personSince(timeline.flatMap(spoken), since); + return speaker === undefined ? null : spokeReason(speaker, label); +} + +async function staleReviewHold({ github, checks, repo, number, label, bot, timeline, since, lifted, reader, trusted }) { + const stop = standingStop({ timeline, lifted, since, bot, label }); + if (stop !== null) { + return { reason: stop }; + } + const pull = await github("GET", `/repos/${repo}/pulls/${number}`); + if (pull?.state !== "open" || pull.base?.ref !== "main" || !SHA.test(String(pull.head?.sha))) { + return { reason: "not an open pull request into main" }; + } + if (!(pull.labels ?? []).some((item) => same(item.name, label))) { + return { reason: `${label} is no longer on it` }; + } + const evidence = await staleHoldEvidence({ github, checks, reader, repo, number, pull, timeline, lifted, trusted }); + if (evidence.reason !== undefined) { + return evidence; + } + const head = pull.head.sha; + return { + ...cleared(timeline, bot, since, [STALE_HOLD], head, [ + `it held \`${evidence.held.join("`, `")}\``, + evidence.moved, + "both review lanes passed on it with a whole review and no open HIGH or CRITICAL finding", + ]), + holds: evidence.holds, + }; +} + +// Why a stale-review-hold clear no longer stands at a fresh read of the head +// and the timeline, or null: the head moved, or the hold comments since the +// label was last removed are not the ones judged. +async function staleHoldMoved({ github, repo, number, label, result }) { + const pull = await github("GET", `/repos/${repo}/pulls/${number}`); + if (pull?.head?.sha !== result.head) { + return `its head moved to ${pull?.head?.sha ?? "an unread head"} since it was judged`; + } + const timeline = await paginate(github, `/repos/${repo}/issues/${number}/timeline`); + const holds = holdsSince(timeline, labelHistory(timeline, label).lifted).shas; + return holds.length !== result.holds || holds.some((sha) => sha === undefined || sha === result.head) ? "a review lane wrote a hold since it was judged" : null; +} + // What to clear on `number`, or { reason } saying why nothing is. async function verdict({ github, checks, repo, number, label, bot, mainSha, lanePrs, verify, reads, reader, trusted }) { const timeline = await paginate(github, `/repos/${repo}/issues/${number}/timeline`); const { labeled, since, lifted } = labelHistory(timeline, label); + if (LANE_LABELERS.some((login) => isApp(labeled?.actor, login))) { + return staleReviewHold({ github, checks, repo, number, label, bot, timeline, since, lifted, reader, trusted }); + } if (!isApp(labeled?.actor, bot)) { return { reason: `${label} was last added by ${labeled?.actor?.login ?? "no one on the timeline"}, not the lanes App` }; } @@ -982,11 +1496,22 @@ export async function main({ env = process.env, github, checks, verify, reader } console.log(`clear-escalations: #${number} kept: ${unsettled}`); continue; } + // A stale hold is judged again before the comment and again before + // the delete: a hold a lane wrote since (the label already on, so no + // labeled event) or a moved head keeps it, with no comment saying + // the label came off. + const recheck = async () => (result.recorded === STALE_HOLD ? await staleHoldMoved({ github, repo, number, label, result }) : null); + let moved = await recheck(); // The comment first: a failed removal is retried by the next sweep, // which finds the comment and posts no second one. const base = `/repos/${repo}/issues/${number}`; - if (!result.explained) { + if (moved === null && !result.explained) { await github("POST", `${base}/comments`, { body: clearedComment(result.recorded, result.head, result.why, label) }); + moved = await recheck(); + } + if (moved !== null) { + console.log(`clear-escalations: #${number} kept: ${moved}`); + continue; } await github("DELETE", `${base}/labels/${encodeURIComponent(label)}`); cleared += 1; diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 9791450c19..53abf33fbc 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1809,7 +1809,10 @@ jobs: # Under merge.lane-prs native, a label enable-auto-merge set at the # current head is judged again with the gate's reader, at most three # reads a run of up to four minutes each, sharing the gate step's run - # cache, listing and head-ref map. + # cache, listing and head-ref map. A review lane's hold at an older head + # comes off as stale-review-hold once trusted human pushes moved the + # head and both review lanes passed there with no HIGH or CRITICAL + # security finding. # Rules: .github/scripts/pr-merge/clear-escalations.mjs. - name: Clear escalations whose reason no longer holds if: >- diff --git a/.github/workflows/pr-require-checks.yml b/.github/workflows/pr-require-checks.yml index e2d1dd14a5..b9fd4caa5c 100644 --- a/.github/workflows/pr-require-checks.yml +++ b/.github/workflows/pr-require-checks.yml @@ -4146,13 +4146,15 @@ jobs: # merge.trust-path-hold-apps the agent Apps whose trust-gate changes the # review-lanes step holds; all are read at the base # so the pull request cannot rewrite its own hold, as is merge-holds.mjs, - # whose refsIssue the cost step runs. A sparse checkout of the four files at the base SHA + # whose refsIssue the cost step runs, and clear-escalations.mjs with every + # module and data file it loads, whose recheckStaleHold the hold-lift + # step runs. A sparse checkout of these files at the base SHA # is a git fetch, not a REST request: as of actions/checkout v7.0.1 its # API calls (default branch, object format, the stale-merge tracepoint, the # no-git archive download) all return early for a 40-hex `ref` on a runner # with git; recheck on a checkout bump. It checks out no pull request - # code: the ref is the base commit, the only code run from it is that - # base module, and no credential is persisted. + # code: the ref is the base commit, the only code run from it is those + # base modules, and no credential is persisted. # The steps below read BASE_CONFIG: empty when this step did not succeed, # which each of them treats as an unreadable config; a file missing from # a checkout that succeeded is a file the base commit does not have. @@ -4170,6 +4172,23 @@ jobs: /.github/standards/trusted-actors/trusted-actors.json /.github/scripts/pr-merge/merge-holds.mjs /docs/conventions/pr-pipeline.yaml + /.github/scripts/pr-merge/clear-escalations.mjs + /.github/scripts/pr-merge/enable-auto-merge.mjs + /.github/scripts/pr-unblock/address-feedback.mjs + /.github/scripts/pr-unblock/gate-source.mjs + /.github/scripts/pr-unblock/escalation-routes.mjs + /.github/scripts/pr-unblock/escalation-routes.json + /.github/scripts/pr-unblock/retry-fetch.mjs + /.github/scripts/pr-unblock/thread-scope.mjs + /.github/scripts/work-resolve-issue/report-outcome.mjs + /.github/scripts/work-resolve-issue/limits.mjs + /.github/scripts/work-resolve-issue/retry-fetch.mjs + /.github/scripts/work-resolve-issue/stop-routes.mjs + /.github/scripts/work-resolve-issue/stop-routes.json + /.github/actions/check-trusted-trigger/check-trusted-trigger.mjs + /.github/actions/check-kill-switch/check-kill-switch.mjs + /.github/actions/log-api-budget/rate-guard.mjs + /.github/actions/log-api-budget/rate-limits.json sparse-checkout-cone-mode: false # A COST HOLD THE OWNER ALREADY DECIDED COMES OFF. A review lane re-holds # every new head, so its `cost` hold went back on after the owner had @@ -4625,7 +4644,13 @@ jobs: # lifts. A bot or another account lifting the hold, or no recorded # removal, fails; adding the label back and having a trusted person # remove it approves: a trusted person's `labeled` event after the hold - # starts a new hold that only that list's removal lifts. + # starts a new hold that only that list's removal lifts. The one removal + # by another account that lifts it is the lanes App's (LANES_APP_ID, on + # the base list) own, after its stale-review-hold comment since the hold + # names a head no hold comment names, while none names this head, and + # only when the base commit's clear-escalations.mjs recheckStaleHold, + # the predicate the App ran, passes again here at this head with this + # job's token. # It runs only when the step above read the live labels and found the # label off. A hold is the newest hold comment or the newest `labeled` # event of the label by github-actions[bot] or claude[bot], whichever is @@ -4641,6 +4666,7 @@ jobs: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} HUMAN_GATED: ${{ steps.human_gated.outputs.label }} BASE_CONFIG: ${{ steps.base_config.outcome == 'success' && format('{0}/base-config', github.workspace) || '' }} run: | @@ -4704,6 +4730,57 @@ jobs: echo "::error::a review lane held #$PR_NUMBER at $held_at, and no removal of '$HUMAN_GATED' since then is recorded. Add the label back; a person on the trusted-actor list removing it approves this pull request." exit 1 fi + # A stale hold the lanes App cleared (clear-escalations.mjs + # stale-review-hold): the App removed the label itself, after its own + # comment since the hold opening with that clear's marker; the + # newest such marker names a head no review-lane hold comment names, + # every hold comment names its head, and none names this head. Hold + # comments count from the label's last removal before the label the + # App removed went on, as clear-escalations.mjs holdsSince counts + # them, so a hold a person already lifted counts for nothing. The + # base commit's clear-escalations.mjs recheckStaleHold must then find + # the hold no longer standing at the marker's head as it was when the + # App removed the label. A later head is judged by its own review + # lanes, as after a person's lift, so a clean push after the clear + # stays green before they run. + if [[ ${LANES_APP_ID:-} =~ ^[1-9][0-9]*$ && $actor_id == "$LANES_APP_ID" && -z $via_app && ${HEAD_SHA:-} =~ ^[0-9a-f]{40}$ ]] && + jq -e --argjson id "$LANES_APP_ID" 'any(.actors[]; .id == $id)' <<<"$actors" >/dev/null 2>&1; then + # shellcheck disable=SC2016 # $e and $on are jq variables + since=$(jq -rs --arg label "$HUMAN_GATED" --arg lifted_at "$lifted_at" '[add[] | select(.label.name == $label)] as $e + | ([$e[] | select(.event == "labeled" and .created_at < $lifted_at) | .created_at] | max) as $on + | [$e[] | select(.event == "unlabeled" and $on != null and .created_at <= $on) | .created_at] | max // ""' <<<"$events") || since= + # shellcheck disable=SC2016 # $c, $app and the rest are jq variables + stale=$(jq -rs --arg held_at "$held_at" --arg lifted_at "$lifted_at" --arg since "$since" --arg head "$HEAD_SHA" --argjson app "$LANES_APP_ID" '(add // []) as $c + | [$c[] | select(.user.type == "Bot" and (.body | test("(^|\n)$").sha] | first // null] as $held + | [$c[] | select(.user.id == $app and .created_at > $held_at and .created_at <= $lifted_at)] | sort_by(.created_at) + | [last // empty | .body | split("\n")[0] | sub("\r$"; "") + | capture("^$").head] as $cleared + | if any($held[]; . == null or . == $head) then "" + else [$cleared[] | select(. as $h | $held | index($h) == null)] | first // "" end' <<<"$raw") || stale= + # The marker only points here: the base commit's + # clear-escalations.mjs recheckStaleHold, the predicate the App + # ran, judges the hold again at the marker's head with this job's + # token. The module is not the first argument, which would run + # its main. + module="$BASE_CONFIG/.github/scripts/pr-merge/clear-escalations.mjs" + if [ -n "$stale" ] && [ -f "$module" ]; then + # shellcheck disable=SC2016 # JavaScript, not shell + if PR_NUMBER="$PR_NUMBER" HEAD_SHA="$HEAD_SHA" CLEARED_SHA="$stale" HUMAN_GATED="$HUMAN_GATED" \ + TRUSTED_ACTORS_PATH="$BASE_CONFIG/.github/standards/trusted-actors/trusted-actors.json" \ + node --input-type=module -e 'import { pathToFileURL } from "node:url"; + const { recheckStaleHold } = await import(pathToFileURL(process.argv[2]).href); + process.exitCode = await recheckStaleHold();' recheck "$module"; then + echo "$actor_login cleared the review-lane hold on #$PR_NUMBER as stale: its marker names $stale, which no hold names, and the hold no longer stands at $HEAD_SHA." + exit 0 + fi + fi + fi if [[ " $humans " != *" $actor_id "* ]]; then echo "::error::$actor_login removed '$HUMAN_GATED' from #$PR_NUMBER after a review lane held it, and is not a person on the trusted-actor list. Add the label back; a person on that list removing it approves this pull request." exit 1 diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 7051bd1891..2c113afc32 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -692,8 +692,10 @@ head: | a reason `escalation-routes.json` routes `fail`, on a label set before it did | a `ci-fix-*` reason once a lane check of a `pr-require-checks.yml` run on the current head reads failed (`ci-status` itself fails on the label, so it is not read, and an advisory lane such as `pr-test-windows` does not count; green, missing or unreadable keeps the label). Any other fail-routed reason but `feedback-not-landed` stays for a person: a lane posts a `pr-unblock/*` status on a head only when it stops there or clears its own failure, so a new head carries nothing that shows the cause gone or fails that head in the label's place | | `head-moved` (pr-merge's `babysit-merge-verify-escalation`, `babysit-merge-risk` and `babysit-merge-shared-copy-escalation`, with no pr-unblock escalation since the label, and the marked App comment written last before the label is pr-merge's and within a minute of it) | the newest App comment carrying each marker names, as ``at head `` ``, a 40-hex head other than the current one; pr-merge then judges the new head and escalates again if its refusal still holds | | `refusal-cleared` (under `merge.lane-prs: native`, `enable-auto-merge`'s own unedited person refusal comment on a lanes App pull request, naming the current head, whose recorded verifier or risk cause is one the lanes clear: every code on `LANE_CAUSES`) | the gate's reader at that head finds nothing refusing, or only causes the lanes clear, read at least as far as the recorded one. A recorded cause a person clears (an issue link, a security finding, a trust-gate widening, a cost hold, a high path, an untrusted spec, any code off that list) is never read or lifted at its head (owner decision Q-CLEAR B): a person removes the label, or the `head-moved` clear does once the head moves. At most three reader runs a run | +| `stale-review-hold` (a label a review lane added, every hold comment since it was last removed naming an earlier head) | a trusted move took the branch off the newest held head, and one of the pull request's own commits since each held head (not reachable from `main`, so an `Update branch` brings in none) is not a merge; both review lanes passed on the current head with a whole review each (status job succeeded, review-complete notice and no hold notice for it, and the head changes neither review caller); no lane leaves a HIGH or CRITICAL finding (`critical-findings: 0` from the security lane, every finding inline for the security lane and any lane a hold names, no unresolved lane thread at `CRITICAL` or `IMPORTANT`, nor one with no readable severity from those strict lanes); and a `critical-security` hold's lane left at least one `CRITICAL` inline finding at the held head, each in a file one of those commits changed (a finding only the summary carried stays for a person). The head and the hold comments are read again just before the removal. A lane that still finds the cause holds the new head again. `ci-status` counts the lanes App's removal only after its own newest `stale-review-hold` comment names a head no hold comment names, while none names the event's head (hold comments counted from the label's last removal before it went on), and only when the base commit's `recheckStaleHold` (the same predicate) passes again with the job's own token at the head the comment names, as it was when the App removed the label, including the check-signed-commits hold, other lanes App markers and a person speaking up to the removal; a later head is judged by its own review lanes, as after a person's lift | -It removes only a label whose newest `labeled` event has the lanes App as its actor, and keeps it +It removes only a label whose newest `labeled` event has the lanes App, or for `stale-review-hold` a +review lane, as its actor, and keeps it when a person commented, reviewed or left a review comment since that event (a bot account, such as an AI review or another lane, does not count), or when an App comment written or edited since then carries any marker other than the escalation being judged (another diff --git a/scripts/ci-hold-lifted-by.test.sh b/scripts/ci-hold-lifted-by.test.sh index a762083ddc..b45b0daede 100755 --- a/scripts/ci-hold-lifted-by.test.sh +++ b/scripts/ci-hold-lifted-by.test.sh @@ -49,7 +49,7 @@ else ok "step fails the job on its exit code" fi # shellcheck disable=SC2016 # the ${{ }} text is workflow source, not shell -for env in 'BASE_SHA: ${{ github.event.pull_request.base.sha }}' 'HUMAN_GATED: ${{ steps.human_gated.outputs.label }}' \ +for env in 'BASE_SHA: ${{ github.event.pull_request.base.sha }}' 'HEAD_SHA: ${{ github.event.pull_request.head.sha }}' 'HUMAN_GATED: ${{ steps.human_gated.outputs.label }}' \ "BASE_CONFIG: \${{ steps.base_config.outcome == 'success' && format('{0}/base-config', github.workspace) || '' }}"; do if [[ "$step" == *"$env"* ]]; then ok "step env carries $env"; else fail "step env lacks $env"; fi done @@ -108,6 +108,27 @@ expect() { rm -rf "$config" "$TMP_ROOT/pr-reads" mkdir -p "$config/.github/standards/trusted-actors" printf '%s\n' "${STUB_ACTORS-$ACTORS}" >"$config/.github/standards/trusted-actors/trusted-actors.json" + # The base commit's clear-escalations.mjs, stubbed: its recheckStaleHold + # records what it was given and answers STUB_PREDICATE (pass, else the hold + # stands); a run of the module as a script is recorded too. Absent when + # STUB_NO_PREDICATE is set. clear-escalations-recheck.test.mjs tests the + # real one. + if [[ -z "${STUB_NO_PREDICATE:-}" ]]; then + mkdir -p "$config/.github/scripts/pr-merge" + cat >"$config/.github/scripts/pr-merge/clear-escalations.mjs" <<'JS' +import { appendFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; +const { PR_NUMBER, HEAD_SHA, CLEARED_SHA, HUMAN_GATED, TRUSTED_ACTORS_PATH, STUB_LOG, STUB_PREDICATE } = process.env; +if (import.meta.url === pathToFileURL(process.argv[1] ?? "").href) { + appendFileSync(STUB_LOG, "clear-escalations main ran\n"); +} +export async function recheckStaleHold() { + appendFileSync(STUB_LOG, `predicate pr=${PR_NUMBER} head=${HEAD_SHA} cleared=${CLEARED_SHA} label=${HUMAN_GATED} trusted=${TRUSTED_ACTORS_PATH}\n`); + console.log(`stale-review-hold: stub ${STUB_PREDICATE ?? "unset"}`); + return STUB_PREDICATE === "pass" ? 0 : 1; +} +JS + fi [[ -z "${STUB_CHECKOUT_FAILED:-}" ]] || config= out="$(PATH="$TMP_ROOT/bin:$PATH" STUB_LOG="$TMP_ROOT/log" PR_NUMBER=6419 \ GITHUB_REPOSITORY=o/r HUMAN_GATED="${HUMAN_GATED-needs-human}" BASE_SHA="${BASE_SHA-$BASE}" \ @@ -188,6 +209,96 @@ expect "a bot re-adding the label does not move the hold" 1 "melodic-automation- STUB_EVENTS="[$BOT_LIFT,$TRUSTED_RELABEL,$(unlabeled needs-human 2026-10-23T12:05:00Z 4242 octocat)]" expect "an untrusted removal after a trusted re-add fails" 1 "octocat removed 'needs-human'" +# A stale hold the lanes App cleared (clear-escalations.mjs stale-review-hold, +# owner brief Part A2): the App's own removal lifts it only after its own +# marked comment names a head no hold comment names. +OLD_SHA=$(printf 'c%.0s' {1..40}) +NEW_SHA=$(printf 'd%.0s' {1..40}) +APP_USER='{"login":"melodic-automation-lanes[bot]","type":"Bot","id":337595936}' +held_at() { comment "$LANE" ""$'\nHeld.\n\nclaude-lane: claude-security-review' "$2"; } +cleared_for() { comment "${2:-$APP_USER}" ""$'\nA review lane held this pull request at an earlier head.' "${3:-2026-10-23T11:29:00Z}"; } +STALE_HOLD="$(held_at "$OLD_SHA" 2026-10-23T10:00:00Z)" +export LANES_APP_ID=337595936 +export HEAD_SHA="$NEW_SHA" +export STUB_PREDICATE=pass +STUB_EVENTS="[$BOT_LIFT]" +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +expect "the lanes App's removal after its stale-review-hold comment for an unheld head passes" 0 \ + "melodic-automation-lanes[bot] cleared the review-lane hold on #6419 as stale: its marker names $NEW_SHA" +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$OLD_SHA")]" +expect "a stale-review-hold comment naming the held head does not lift it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$(held_at "$NEW_SHA" 2026-10-23T09:30:00Z),$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +expect "a stale-review-hold comment naming a head an earlier hold names does not lift it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$STALE_HOLD]" +expect "the lanes App's removal with no stale-review-hold comment still fails" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA" "" 2026-10-23T09:00:00Z)]" +expect "a stale-review-hold comment from before the hold does not lift it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA" '{"login":"someone","type":"User","id":4242}')]" +expect "the marker in another account's comment does not lift it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$HOLD,$(cleared_for "$NEW_SHA")]" +expect "a hold comment whose head does not parse keeps it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +STUB_EVENTS="[$(jq -c '.performed_via_github_app = {slug: "some-app"}' <<<"$BOT_LIFT")]" +expect "the lanes App's removal through another App does not lift it" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_EVENTS="[$(unlabeled needs-human 2026-10-23T11:30:00Z 4242 octocat)]" +expect "another account's removal after the App's comment does not lift it" 1 "octocat removed 'needs-human'" +STUB_EVENTS="[$BOT_LIFT]" +LANES_APP_ID='' expect "with no lanes App id the App's removal fails closed" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_ACTORS='{"version":1,"actors":[{"id":153232337,"login":"kyle-sexton","kind":"human"}]}' \ + expect "a lanes App id the base list does not name fails closed" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +# The race the security review of 990c1941 found: a lane held the current +# head after the App judged it, under the label already on, before its delete. +STUB_COMMENTS="[$STALE_HOLD,$(held_at "$NEW_SHA" 2026-10-23T11:20:00Z),$(cleared_for "$OLD_SHA")]" +expect "a hold naming this head, written before the App's marker, keeps it whatever head the marker names" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +OTHER_SHA=$(printf 'e%.0s' {1..40}) +STUB_COMMENTS="[$STALE_HOLD,$(held_at "$NEW_SHA" 2026-10-23T11:20:00Z),$(cleared_for "$OTHER_SHA")]" +expect "a hold naming this head keeps it beside a marker for an unheld head" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +# A later clean push: the marker names the head the App judged, this head is +# a newer one no hold names, so its own reviews decide (not strict equality). +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +HEAD_SHA="$OTHER_SHA" expect "a later head no hold names passes after the App's clear of an earlier one" 0 \ + "melodic-automation-lanes[bot] cleared the review-lane hold on #6419 as stale" +if grep -qxF "predicate pr=6419 head=$OTHER_SHA cleared=$NEW_SHA label=needs-human trusted=$TMP_ROOT/base-config/.github/standards/trusted-actors/trusted-actors.json" "$TMP_ROOT/log"; then + ok "at a later head the predicate judges the head the App's marker names" +else + fail "at a later head the predicate was not given the marker's head: '$(cat "$TMP_ROOT/log")'" +fi +# Only the newest marker before the removal names the cleared head. +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$OTHER_SHA" "" 2026-10-23T11:10:00Z),$(cleared_for "$NEW_SHA")]" +expect "the newest marker before the removal is the one judged" 0 "its marker names $NEW_SHA," +# A hold a person already lifted counts for nothing (thread PRRT_kwDOTCGFQM6xmJXy): +# an old unparsable hold, its trusted lift, then the lane's new label and hold. +STUB_COMMENTS="[$(comment "$LANE" $'\nHeld.\n\nclaude-lane: claude-review' 2026-10-22T10:00:00Z),$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +STUB_EVENTS="[$(unlabeled needs-human 2026-10-22T12:00:00Z 153232337 kyle-sexton),$(labeled needs-human 2026-10-23T09:59:00Z 'github-actions[bot]'),$BOT_LIFT]" +expect "an unparsable hold a person lifted before the label went on again does not block the App's clear" 0 \ + "melodic-automation-lanes[bot] cleared the review-lane hold on #6419 as stale" +STUB_COMMENTS="[$(held_at "$NEW_SHA" 2026-10-22T10:00:00Z),$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +expect "a lifted hold naming the head the App cleared does not block it" 0 \ + "melodic-automation-lanes[bot] cleared the review-lane hold on #6419 as stale" +STUB_EVENTS="[$(labeled needs-human 2026-10-23T09:59:00Z 'github-actions[bot]'),$BOT_LIFT]" +expect "with no removal before the label, every hold comment counts" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_EVENTS="[$BOT_LIFT]" +HEAD_SHA='' expect "an empty head fails the App's removal closed" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +HEAD_SHA=main expect "a head that is not a commit SHA fails the App's removal closed" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +# The marker only points ci-status at the hold: the base commit's +# clear-escalations.mjs recheckStaleHold judges it again at this head (coordinator direction: +# never take the App's judgment on trust). +STUB_COMMENTS="[$STALE_HOLD,$(cleared_for "$NEW_SHA")]" +STUB_PREDICATE=stands expect "a valid marker whose predicate now finds the hold standing fails" 1 \ + "melodic-automation-lanes[bot] removed 'needs-human'" +if grep -qxF "predicate pr=6419 head=$NEW_SHA cleared=$NEW_SHA label=needs-human trusted=$TMP_ROOT/base-config/.github/standards/trusted-actors/trusted-actors.json" "$TMP_ROOT/log"; then + ok "the predicate runs at this head with the base commit's trusted-actor list and the resolved label" + if grep -q 'clear-escalations main ran' "$TMP_ROOT/log"; then fail "importing the base module ran its sweep"; else ok "importing the base module runs no sweep"; fi +else + fail "the predicate's inputs are not this head, label and base list: '$(cat "$TMP_ROOT/log")'" +fi +STUB_NO_PREDICATE=1 expect "a base commit with no clear-escalations.mjs fails the App's removal closed" 1 \ + "melodic-automation-lanes[bot] removed 'needs-human'" +STUB_COMMENTS="[$STALE_HOLD]" +expect "with no marker the predicate is not run" 1 "melodic-automation-lanes[bot] removed 'needs-human'" +if grep -q '^predicate ' "$TMP_ROOT/log"; then fail "the predicate ran with no marker"; else ok "the predicate runs only behind a marker"; fi +unset LANES_APP_ID HEAD_SHA STUB_PREDICATE + export STUB_EVENTS="[$OWNER_LIFT]" export STUB_ACTORS='{"version":1,"actors":[{"id":153232337,"login":"kyle-sexton","kind":"bot"}]}' expect "an id listed only as a bot is not a trusted person" 1 "the trusted-actor list at $BASE names no person"