Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Fixed

- `cost_decision` keeps a cost hold when a lane run of a newer head was deleted. Deleting a run left the older run's notice as the newest, so an older head became the decided head. The decided head now stands only while the head branch's push history (the repository activity API, one page, which deleting a run leaves) shows its push before the decision and a run of every review lane, started before the decision, on each head pushed after that push and before the decision (`unran_head`). A full page or an unreadable history carries nothing.
4 changes: 4 additions & 0 deletions .github/scripts/pr-merge/enable-auto-merge.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ function world(pulls, { failArm = false, armErrors = {}, filesError = null, note
if (method === "GET" && runs) {
return { workflow_runs: runs[1] === "pr-review-hosted" ? [{ id: 9, head_sha: HEAD, head_repository: { full_name: REPO }, created_at: "2026-10-22T10:50:00Z" }] : [] };
}
// The head branch's push history: the head, pushed before its run.
if (method === "GET" && path === "/repos/melodic-software/claude-code-plugins/activity?ref=refs%2Fheads%2Ffix%2Fcost&direction=desc&per_page=100") {
return [{ after: HEAD, timestamp: "2026-10-22T10:45:00Z", activity_type: "push", ref: "refs/heads/fix/cost" }];
}
if (method === "GET" && path === `/repos/melodic-software/claude-code-plugins/compare/main...${HEAD}`) {
return { files: [{ filename: "infra/main.tf" }] };
}
Expand Down
58 changes: 54 additions & 4 deletions .github/scripts/pr-merge/merge-holds.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,46 @@ export function decidedHead(notices, at) {
return heads.size === 1 ? [...heads][0] : null;
}

// Why the head branch's push history shows a lane run missing before a
// decision made at `at`, else null (babysit_merge_policy.unran_head, and
// ci-status's cost-hold step). `activity` is one page of the repository
// activity API for the branch, newest first, which deleting a run leaves;
// `ran` lists, per REVIEW_LANES entry, the heads of its runs started before
// `at`. Every head the branch moved to after its last push of `decided`
// before `at` needs a run of every lane: with one deleted, an older notice
// would become the decided head (#10437). No push of `decided` before `at`
// or a full page, which may not reach it, keeps the hold too.
export function unranHead(activity, ran, decided, at) {
if (!Array.isArray(activity)) {
return "the branch's push history is unreadable";
}
if (activity.length >= 100) {
return "the branch's push history fills a page";
}
if (!Array.isArray(ran) || ran.length !== REVIEW_LANES.length || !ran.every(Array.isArray)) {
return "the lane runs do not cover every review lane";
}
const moves = [];
for (const move of activity) {
const after = String(move?.after ?? "");
if (/^[0-9a-f]{40}$/.test(after) && !/^0{40}$/.test(after)) {
const made = Date.parse(move.timestamp);
if (!Number.isFinite(made)) {
return "the branch's push history is unreadable";
}
moves.push({ after, made });
}
}
const cutoff = Date.parse(at);
const before = moves.filter((m) => m.made < cutoff);
const last = Math.max(...before.filter((m) => m.after === decided).map((m) => m.made));
if (!Number.isFinite(last)) {
return `the branch shows no push of ${decided} before the decision`;
}
const gap = before.find((m) => m.after !== decided && m.made >= last && !ran.every((heads) => heads.includes(m.after)));
return gap ? `${gap.after} was pushed after the decided head with no run of every review lane before the decision` : null;
}

// Every item of the REST list at `path` whose answer holds it under `key`.
async function listed(read, path, key) {
const rows = [];
Expand All @@ -593,18 +633,22 @@ async function listed(read, path, key) {
// repository's same-named branch speaks for another pull request), each
// run's head vetted against
// `base` (laneChange), its status job's cost notices (costNotices) from jobs
// done before `at`, then decidedHead. A run on a head that fails the vet, or
// a read that fails, decides nothing. `read` holds actions: read and
// checks: read.
// done before `at`, then decidedHead, kept only while the branch's push
// history shows no lane run missing since the decided head (unranHead). A
// run on a head that fails the vet, or a read that fails, decides nothing.
// `read` holds contents: read, actions: read and checks: read.
async function noticedHead(read, repo, branch, base, at) {
const cutoff = Date.parse(at);
if (!Number.isFinite(cutoff) || typeof branch !== "string" || !BRANCH.test(branch)) {
return null;
}
const notices = [];
const vetted = new Set();
const ran = [];
for (const [file, check] of REVIEW_LANES) {
const runs = await listed(read, `/repos/${repo}/actions/workflows/${file}/runs?event=pull_request&branch=${encodeURIComponent(branch)}`, "workflow_runs");
const heads = [];
ran.push(heads);
for (const run of runs) {
if (run?.head_repository?.full_name !== repo || !(Date.parse(run.created_at) < cutoff)) {
continue;
Expand All @@ -613,6 +657,7 @@ async function noticedHead(read, repo, branch, base, at) {
if (!/^[0-9a-f]{40}$/.test(head)) {
return null;
}
heads.push(head);
if (!vetted.has(head)) {
if (laneChange(await read("GET", `/repos/${repo}/compare/${base}...${head}`)) !== null) {
return null;
Expand All @@ -626,7 +671,12 @@ async function noticedHead(read, repo, branch, base, at) {
}
}
}
return decidedHead(notices, at);
const decided = decidedHead(notices, at);
if (decided === null) {
return null;
}
const activity = await read("GET", `/repos/${repo}/activity?ref=${encodeURIComponent(`refs/heads/${branch}`)}&direction=desc&per_page=100`);
return unranHead(activity, ran, decided, at) === null ? decided : null;
}

// Whether a COMMENT_EDITED_QUERY answer's node was never edited.
Expand Down
86 changes: 83 additions & 3 deletions .github/scripts/pr-merge/merge-holds.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
// request counts as it is, one on its linked issue (closing, else `Refs:`)
// only when it names `#<number>`. It covers the head it decided, the sha of the
// newest cost hold notice a review lane's status job recorded before it on a
// run vetted not to change a review-lane workflow (#9635), and a later head
// run vetted not to change a review-lane workflow (#9635), while the head
// branch's push history shows a run of every lane on each head pushed since
// it before the decision (#10437), and a later head
// only when the net change against the base branch is the same at both (owner
// decisions Q17 c and Q18 d on #9436).
import assert from "node:assert/strict";
Expand All @@ -17,7 +19,7 @@ import { writeFileSync } from "node:fs";
import { join } from "node:path";
import { test } from "node:test";

import { BODY_WRITERS_QUERY, COMMENT_EDITED_QUERY, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, changeOf, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange } from "./merge-holds.mjs";
import { BODY_WRITERS_QUERY, COMMENT_EDITED_QUERY, HOLD_PATH_CLASSES, HOLD_PATH_FLOOR, PULL_OPENED_QUERY, REVIEW_FLOOR, changeOf, costDecision, costHold, costNotices, decidedHead, evidenceHold, laneChange, linkHold, main, pathHold, sameNetChange, unranHead } from "./merge-holds.mjs";
import { tempDir } from "../../../lib/test-tmp.mjs";

const REPO = "owner/repo";
Expand All @@ -40,6 +42,18 @@ const laneRun = (sha, at = "2026-10-22T11:00:00Z", { criteria = "cost", lane = "
created,
notes: [{ annotation_level: "notice", message: `claude-lane-hold lane=claude-review sha=${notice} criteria=${criteria} label=needs-human` }],
});
// The head branch's history as the repository activity API lists it, newest
// first: a push of `after` at `timestamp`.
const push = (after, timestamp) => ({ after, timestamp, activity_type: "push", ref: "refs/heads/fix/cost" });
const newestFirst = (moves) => [...moves].sort((a, b) => Date.parse(b.timestamp) - Date.parse(a.timestamp));
// By default each run's head was pushed five minutes before its first status
// job finished.
const pushesOf = (runs) =>
newestFirst(
Object.entries(
runs.reduce((first, run) => ({ ...first, [run.sha]: Math.min(first[run.sha] ?? Infinity, Date.parse(run.at)) }), {}),
).map(([sha, at]) => push(sha, new Date(at - 300_000).toISOString())),
);
// A compare answer: the pull request's files against the base.
const TF = { filename: "infra/main.tf", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" };
const net = (...files) => ({ files });
Expand Down Expand Up @@ -70,10 +84,18 @@ function world({
runs = [laneRun(HEAD)],
compares = {},
base = "main",
activity = pushesOf(runs),
} = {}) {
const reads = [];
const answers = { ...Object.fromEntries(runs.map((run) => [run.sha, net(TF)])), ...compares };
const github = async (method, path, body) => {
if (path === "/repos/owner/repo/activity?ref=refs%2Fheads%2Ffix%2Fcost&direction=desc&per_page=100") {
reads.push("activity");
if (fail === "activity") {
throw new Error("activity failed");
}
return activity;
}
const compare = /^\/repos\/owner\/repo\/compare\/(.+)\.\.\.([0-9a-f]{40})$/.exec(path);
const listing = /^\/repos\/owner\/repo\/actions\/workflows\/([a-z-]+\.yml)\/runs\?event=pull_request&branch=fix%2Fcost&per_page=100&page=1$/.exec(path);
const jobs = /^\/repos\/owner\/repo\/actions\/runs\/([0-9]+)\/jobs\?filter=all&per_page=100&page=1$/.exec(path);
Expand Down Expand Up @@ -153,6 +175,7 @@ test("an owner decision on the closing issue, linked from the body, clears the c
"jobs 1",
"notes 1",
"runs pr-review-security-hosted.yml",
"activity",
]);
});

Expand Down Expand Up @@ -500,7 +523,7 @@ test("a base merge after the decision carries it, and a pull request commit afte
assert.equal((await later({ [OLD]: net(TF), [HEAD]: net(TF) })).link, LINK);
const commit = await later({ [OLD]: net(TF), [HEAD]: net(TF, { filename: "scripts/run.sh", status: "modified", patch: "@@ -1 +1 @@\n-x\n+y" }) });
assert.equal(commit.link, null);
assert.deepEqual(commit.reads.slice(-3), ["runs pr-review-security-hosted.yml", `compare ${OLD}`, `compare ${HEAD}`]);
assert.deepEqual(commit.reads.slice(-3), ["activity", `compare ${OLD}`, `compare ${HEAD}`]);
});

test("a rebase onto a newer base with the same net change carries, whatever order the files list in", async () => {
Expand Down Expand Up @@ -647,6 +670,63 @@ test("a lane run on a head that changes a review-lane workflow, or whose compare
}
});

// #10437: a deleted lane run of a head pushed after the decided head, before
// the decision, would make an older notice the decided head. The branch's
// push history, which deleting a run leaves, shows that head, and it must
// have a run of every review lane started before the decision.
test("a head pushed between the decided head and the decision with no run of every lane keeps the hold", async () => {
const NEW = "1111111111111111111111111111111111111111";
const same = { [OLD]: net(TF), [HEAD]: net(TF) };
const history = (...moves) => newestFirst([push(OLD, "2026-10-22T10:25:00Z"), ...moves]);
const plain = (sha, lane) => laneRun(sha, "2026-10-22T11:20:00Z", { criteria: "trust-gate", lane, created: "2026-10-22T11:12:00Z" });
const carried = (runs, activity, options = {}) => decide(undefined, { runs: [laneRun(OLD), ...runs], compares: same, activity, ...options });
// No deletion: both lanes ran on the newer head, which noticed no cost.
const both = [plain(NEW, "pr-review-hosted.yml"), plain(NEW, "pr-review-security-hosted.yml")];
assert.equal((await carried(both, history(push(NEW, "2026-10-22T11:10:00Z")))).link, LINK);
// A head pushed after the decision needs no run here.
assert.equal((await carried([], history(push(NEW, "2026-10-22T12:10:00Z")))).link, LINK);
for (const [name, runs] of Object.entries({
"both lanes' runs deleted": [],
"the code lane's run deleted": [plain(NEW, "pr-review-security-hosted.yml")],
"the security lane's run deleted": [plain(NEW, "pr-review-hosted.yml")],
"a run started only after the decision": both.map((run) => ({ ...run, created: "2026-10-22T12:05:00Z" })),
"a run whose head is in another repository": both.map((run) => ({ ...run, repo: "fork/repo" })),
})) {
const kept = await carried(runs, history(push(NEW, "2026-10-22T11:10:00Z")));
assert.equal(kept.link, null, name);
assert.ok(kept.reads.includes("activity"), name);
}
// Pushing the decided head again after the decision makes it the head: the
// gap before the decision still keeps the hold.
const back = history(push(NEW, "2026-10-22T11:10:00Z"), push(OLD, "2026-10-22T12:30:00Z"));
assert.equal((await carried([], back, { head: OLD })).link, null);
// A history that shows no push of the decided head before the decision, a
// full page that may not reach it, or a read that fails keeps the hold.
assert.equal((await carried(both, [push(NEW, "2026-10-22T11:10:00Z")])).link, null);
assert.equal((await carried(both, [push(OLD, "2026-10-22T12:30:00Z")])).link, null);
const full = newestFirst([...Array.from({ length: 99 }, (_, i) => push(NEW, new Date(Date.parse("2026-10-22T12:10:00Z") + i * 1000).toISOString())), push(OLD, "2026-10-22T10:25:00Z")]);
assert.equal((await carried(both, full)).link, null);
assert.equal((await carried(both, full.slice(1))).link, LINK);
assert.equal((await carried(both, history(), { fail: "activity" })).link, null);
assert.equal((await carried(both, { message: "Not Found" })).link, null);
});

test("unranHead names the first head pushed after the decided head with no run of every lane", () => {
const NEW = "1111111111111111111111111111111111111111";
const at = "2026-10-22T12:00:00Z";
const moves = [push(NEW, "2026-10-22T11:10:00Z"), { ...push("0".repeat(40), "2026-10-22T11:05:00Z"), activity_type: "branch_deletion" }, push(OLD, "2026-10-22T10:25:00Z")];
assert.equal(unranHead(moves, [[OLD, NEW], [NEW]], OLD, at), null);
assert.match(unranHead(moves, [[OLD], [NEW]], OLD, at), new RegExp(NEW));
assert.match(unranHead(moves, [[OLD, NEW]], OLD, at), /every review lane/);
assert.match(unranHead(moves, null, OLD, at), /every review lane/);
assert.match(unranHead(moves, [[NEW], [NEW]], HEAD, at), /no push of/);
assert.match(unranHead(moves, [[NEW], [NEW]], OLD, "not a time"), /no push of/);
assert.match(unranHead([{ after: NEW, timestamp: "soon" }, ...moves], [[NEW], [NEW]], OLD, at), /unreadable/);
assert.match(unranHead("nope", [[], []], OLD, at), /unreadable/);
// Only the moves after the decided head's last push before the decision count.
assert.equal(unranHead([push(OLD, "2026-10-22T11:30:00Z"), ...moves], [[OLD], [OLD]], OLD, at), null);
});

test("laneChange, costNotices and decidedHead", () => {
assert.equal(laneChange(net(TF)), null);
assert.equal(laneChange(net({ filename: ".github/workflows/pr-review-hosted.yml" })), "a review-lane workflow");
Expand Down
29 changes: 27 additions & 2 deletions .github/workflows/pr-require-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3984,7 +3984,14 @@ jobs:
# way and only where it names its own run's head; never a hold
# comment, which any GITHUB_TOKEN workflow can post as
# github-actions[bot]. With none, or two heads at that time, it covers
# no head. The linked issue is the one issue in this repository that
# no head. Nor does it while the head branch's push history (the
# repository activity API, which deleting a run leaves) shows a head
# pushed after the decided head's last push and before the decision
# with no run of every review lane started before the decision, since
# a deleted run of a newer head would make an older notice the decided
# head (the base's merge-holds.mjs unranHead, #10437); a history with
# no push of the decided head before it, or a full page, covers none
# too. The linked issue is the one issue in this repository that
# GitHub records it as closing, else, when it closes none, the one issue
# its plain `Refs:` lines name (the base's merge-holds.mjs refsIssue),
# read to be an issue and not a pull request. A second closing issue
Expand Down Expand Up @@ -4159,9 +4166,14 @@ jobs:
# for another pull request), updated in the window or started before
# the decision, as {at, criteria, sha, run}: `sha` as the notice
# names it, `run` the head its run ran on.
notices='[]' vetted=''
# `ran` lists, per lane, the heads of its runs from this repository
# started before the decision.
notices='[]' vetted='' ran='[]'
while read -r file check; do
runs=$(read_api "repos/$repo/actions/workflows/$file/runs?event=pull_request&branch=$ref&per_page=100" --paginate) || keep "the $file runs are unreadable"
ran=$(jq -nc --argjson ran "$ran" --arg repo "$repo" --argjson decided "$decided_at" '$ran + [[inputs.workflow_runs[]
| select(.head_repository.full_name == $repo and (.created_at | fromdateiso8601) < $decided) | .head_sha] | unique]' <<<"$runs") ||
keep "the $file runs do not parse"
while read -r id sha; do
[ -n "$id" ] || continue
[[ $sha =~ ^[0-9a-f]{40}$ ]] || keep "run $id names no head commit"
Expand Down Expand Up @@ -4195,6 +4207,19 @@ jobs:
decided=$(jq -r --argjson at "$decided_at" '[.[] | select(.at < $at and .sha == .run and any(.criteria | split(",")[]; . == "cost"))]
| (map(.at) | max) as $newest | [.[] | select(.at == $newest) | .sha] | unique
| if length == 1 then .[0] else "" end' <<<"$notices") || keep "the hold notices do not parse"
# The decided head stands only while the head branch's push history
# shows a run of every lane on each head pushed since it before the
# decision (the base's unranHead; one page, as merge-holds.mjs reads it).
if [ -n "$decided" ]; then
read_api "repos/$repo/activity?ref=refs%2Fheads%2F$ref&direction=desc&per_page=100" >"$RUNNER_TEMP/cost-hold-activity.json" ||
keep "the head branch's push history is unreadable"
printf '%s\n' "$ran" >"$RUNNER_TEMP/cost-hold-ran.json"
jq -n --arg sha "$decided" '$sha' >"$RUNNER_TEMP/cost-hold-decided.json"
jq '.comment.created_at' <<<"$chosen" >"$RUNNER_TEMP/cost-hold-decided-at.json" || keep "the decisions do not parse"
gap=$(holds_js unranHead "$RUNNER_TEMP/cost-hold-activity.json" "$RUNNER_TEMP/cost-hold-ran.json" \
"$RUNNER_TEMP/cost-hold-decided.json" "$RUNNER_TEMP/cost-hold-decided-at.json") || keep "the head branch's push history could not be checked"
[ -z "$gap" ] || keep "$gap"
fi
carries "$decided" || keep "the body cites no recorded owner cost decision covering this head"
decision=$(jq -r .link <<<"$chosen") decision_id=$(jq -r .id <<<"$chosen")
holds=$(jq -c --argjson since "$since" '[.[] | select(.at > $since)]' <<<"$notices") || keep "the hold notices do not parse"
Expand Down
Loading
Loading