From c872aa082cb63fcfafae81b2438e18b2e72e013a Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:37:59 -0400 Subject: [PATCH 1/2] feat(pr-pipeline): hold lanes App human-merge pull requests with a label only a listed person lifts report-outcome labels a pull request it holds for a person human-merge-hold right after it opens or reuses it, and closes and retries one it cannot label (hold-label-failed). A new ci-status step fails while the label is on and, once it is off, unless the first removal at or after its newest add was a trusted-actor human acting directly. enable-auto-merge, the merge queue's labelHolds and the babysit merge lane hold the label too, so deleting the body's hold line or rewriting Refs to Closes no longer lets a held pull request merge. Closes #9437 Co-Authored-By: Claude Opus 5.5 --- .../human-merge-hold-label-9437a4d1.md | 17 ++ .../human-merge-hold-label-9437a4d1.md | 9 + .../scripts/pr-merge/enable-auto-merge.mjs | 2 +- .../pr-merge/enable-auto-merge.test.mjs | 19 ++ .github/scripts/pr-merge/merge-holds.mjs | 9 +- .github/scripts/pr-merge/merge-holds.test.mjs | 1 + .../work-resolve-issue/report-outcome.mjs | 33 +++- .../report-outcome.test.mjs | 79 ++++++++- .../work-resolve-issue/stop-routes.json | 1 + .github/workflows/pr-require-checks.yml | 81 +++++++++ docs/conventions/pr-pipeline/README.md | 6 + .../pr-pipeline/pr-run-activity.md | 5 +- .../scripts/babysit_merge_policy.py | 3 +- scripts/ci-hold-human-merge.test.sh | 165 ++++++++++++++++++ 14 files changed, 412 insertions(+), 18 deletions(-) create mode 100644 .changes/pr-pipeline/human-merge-hold-label-9437a4d1.md create mode 100644 .changes/source-control/human-merge-hold-label-9437a4d1.md create mode 100755 scripts/ci-hold-human-merge.test.sh diff --git a/.changes/pr-pipeline/human-merge-hold-label-9437a4d1.md b/.changes/pr-pipeline/human-merge-hold-label-9437a4d1.md new file mode 100644 index 00000000000..1a925191851 --- /dev/null +++ b/.changes/pr-pipeline/human-merge-hold-label-9437a4d1.md @@ -0,0 +1,17 @@ +--- +bump: minor +--- + +### Added + +- A lanes App pull request that holds for a person carries the `human-merge-hold` label as well as + the `Hold: a human merges this` body line, which the App can edit + ([#9437](https://github.com/melodic-software/claude-code-plugins/issues/9437)). report-outcome + adds the label right after it opens the pull request, or when it reuses its open one; a pull + request the label cannot reach is closed and the issue retried (`hold-label-failed`). ci-status's + new `Fail a human-merge hold a listed person has not lifted` step fails while the label is on, + and once it is off unless the first removal at or after its newest add was by a `human` of the + base's trusted-actor list acting directly, not through an App. `enable-auto-merge.mjs` + (`HOLD_LABELS`) never arms a pull request carrying it, and merge-holds.mjs `labelHolds` holds it + in the merge queue, so deleting the hold line or rewriting `Refs: #N` to `Closes #N` no longer + lets a held pull request merge. diff --git a/.changes/source-control/human-merge-hold-label-9437a4d1.md b/.changes/source-control/human-merge-hold-label-9437a4d1.md new file mode 100644 index 00000000000..6bcccd6b072 --- /dev/null +++ b/.changes/source-control/human-merge-hold-label-9437a4d1.md @@ -0,0 +1,9 @@ +--- +bump: patch +--- + +### Changed + +- The babysit merge lane holds a pull request carrying `human-merge-hold` + (`babysit_merge_policy.MERGE_HOLD_LABELS`), the label the lanes App puts on a pull request a + person merges ([#9437](https://github.com/melodic-software/claude-code-plugins/issues/9437)). diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index b74602371d9..c09d4f3519e 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -219,7 +219,7 @@ const REPO = /^([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)$/; const SHA = /^[0-9a-f]{40}$/; const BOT_LOGIN = /^([a-z0-9][a-z0-9-]*)\[bot\]$/; // babysit_merge_policy.MERGE_HOLD_LABELS, plus the human-gated label. -export const HOLD_LABELS = Object.freeze(["do-not-merge", "pr-verify-failed"]); +export const HOLD_LABELS = Object.freeze(["do-not-merge", "pr-verify-failed", "human-merge-hold"]); // 100 pull requests a page; more than this many open is a listing the run // stops on rather than one it reads only part of. const MAX_PAGES = 10; diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 36465d31065..2788c4e9831 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -27,6 +27,7 @@ import { UNLABELED_QUERY, decide, describe, + holdReason, unblockHold, liftedByPerson, READER_EVENTS, @@ -1716,6 +1717,24 @@ test("the lane's own hold line holds a lanes App body", async () => { assert.deepEqual(writes, [{ mutation: "arm", id: "PR_3", head: HEAD }]); }); +// #9437: report-outcome labels a held PR human-merge-hold, so a lanes App +// edit of the body (the hold line deleted, or `Refs: #N` rewritten to +// `Closes #N`) leaves it unarmed while the label is on. +test("a human-merge-hold label holds a lanes App PR whose body lost its hold line or its Refs line", async () => { + const appOnly = { totalCount: 0, nodes: [] }; + const held = labels("human-merge-hold"); + const { writes } = await run([ + // The hold line deleted from a Refs PR's body. + pull(1, { body: "Refs: #5\n\n## Summary", labels: held, userContentEdits: appOnly }), + // `Refs: #5` rewritten to `Closes #5`, the hold line gone with it. + pull(2, { body: "Closes #5\n\n## Summary", labels: held, userContentEdits: appOnly }), + pull(3, { body: "Closes #5\n\n## Summary", userContentEdits: appOnly }), + ]); + assert.deepEqual(writes, [{ mutation: "arm", id: "PR_3", head: HEAD }]); + assert.equal(holdReason({ labels: held, body: "Closes #5" }, "needs-human"), "it carries human-merge-hold"); + assert.equal(holdReason({ labels: labels("Human-Merge-Hold"), body: "Closes #5" }, "needs-human"), "it carries human-merge-hold"); +}); + // Owner decision Q-HOLDLINE: report-outcome's own hold line is a cause a // person must clear, so a reader run comments once and adds the human-gated // label, as for a configured denied path; it puts the label back unless a diff --git a/.github/scripts/pr-merge/merge-holds.mjs b/.github/scripts/pr-merge/merge-holds.mjs index 75fdfaac6c9..43c5a1165ac 100755 --- a/.github/scripts/pr-merge/merge-holds.mjs +++ b/.github/scripts/pr-merge/merge-holds.mjs @@ -1438,8 +1438,13 @@ export function humanGatedLabel(tracker) { return typeof label === "string" && label.trim() ? label.trim() : "needs-human"; } +// The label report-outcome puts on a lanes App pull request that holds for a +// person (report-outcome.mjs HUMAN_MERGE_HOLD). Only a listed person's own +// removal lifts it (ci-status's `Fail a human-merge hold` step). +export const HUMAN_MERGE_HOLD = "human-merge-hold"; + // The hold labels on pull request `number`: `do-not-merge`, the tracker's -// human-gated label and `pr-verify-failed`, one reason each. `labels` are +// human-gated label, `pr-verify-failed` and HUMAN_MERGE_HOLD, one reason each. `labels` are // names or label objects; a list that is not an array throws. export function labelHolds(labels, tracker, number) { if (!Array.isArray(labels)) { @@ -1452,6 +1457,8 @@ export function labelHolds(labels, tracker, number) { names.has(gated) && `#${number} carries '${gated}'; the merge queue removes it. Removing the label approves this pull request; re-queue it to merge.`, names.has("pr-verify-failed") && `#${number} carries 'pr-verify-failed': pr-verify failed its head, and a pass at a new commit removes the label. The merge queue removes it; re-queue it once the label is off.`, + names.has(HUMAN_MERGE_HOLD) && + `#${number} carries '${HUMAN_MERGE_HOLD}': a person merges it. The merge queue removes it; a person on the trusted-actor list removing the label approves it, then re-queue it to merge.`, ].filter(Boolean); } diff --git a/.github/scripts/pr-merge/merge-holds.test.mjs b/.github/scripts/pr-merge/merge-holds.test.mjs index 7a7c0519b75..bee95945995 100644 --- a/.github/scripts/pr-merge/merge-holds.test.mjs +++ b/.github/scripts/pr-merge/merge-holds.test.mjs @@ -1471,6 +1471,7 @@ test("queueHolds: each hold label holds with ci-status's text", () => { ["do-not-merge", "#7 carries the 'do-not-merge' label; the merge queue removes it. Remove the label and re-queue to merge."], ["needs-human", "#7 carries 'needs-human'; the merge queue removes it. Removing the label approves this pull request; re-queue it to merge."], ["pr-verify-failed", "#7 carries 'pr-verify-failed': pr-verify failed its head, and a pass at a new commit removes the label. The merge queue removes it; re-queue it once the label is off."], + ["human-merge-hold", "#7 carries 'human-merge-hold': a person merges it. The merge queue removes it; a person on the trusted-actor list removing the label approves it, then re-queue it to merge."], ]) { assert.deepEqual(holdsModule.queueHolds({ pull: queuedPull({ labels: [{ name: "bug" }, { name: label }] }), statuses: [] }), [want], label); } diff --git a/.github/scripts/work-resolve-issue/report-outcome.mjs b/.github/scripts/work-resolve-issue/report-outcome.mjs index 6fa6b222ae8..f3163eef9ac 100755 --- a/.github/scripts/work-resolve-issue/report-outcome.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.mjs @@ -128,6 +128,10 @@ import { networkError, retryFetch } from "./retry-fetch.mjs"; import { ATTENDED_LABEL, HOST_RUN_LABEL, runCode, stopRoute, stopWhy } from "./stop-routes.mjs"; export const HOLD_LINE = "Hold: a human merges this"; +// The label a held PR carries from its opening. The hold line is body text the +// lanes App can edit; ci-status fails while this label is on and unless a +// listed person's own removal took it off (merge-holds.mjs HUMAN_MERGE_HOLD). +export const HUMAN_MERGE_HOLD = "human-merge-hold"; const EXITS = new Set(["done", "pushback", "stopped"]); // The subject allows no `#`, `/` or `GH-`, so the squash commit title // cannot name an issue to close. @@ -834,7 +838,7 @@ export function reusePlan({ "", `#${issueNumber}`, ].join("\n"); - return { action: "open-pr", issueNumber, title: prTitle(summary?.title ?? "", issueTitle, issueNumber), body, branch }; + return { action: "open-pr", issueNumber, title: prTitle(summary?.title ?? "", issueTitle, issueNumber), body, branch, hold }; } // What to do on the issue, from the outcome and what the run left. `limit` is @@ -1062,6 +1066,7 @@ export function decide({ title: prTitle(outcome.title, issueTitle, issueNumber), body, branch, + hold, }; } @@ -1833,18 +1838,36 @@ export async function main({ env = process.env, fetch = globalThis.fetch, now = pull = (await findOpen()) ?? (await github("POST", `${repo}/pulls`, fields)); } } + // A held PR carries the hold label from its opening; one the label + // cannot reach is closed rather than left held by body text alone. + const labeled = + !plan.hold || + (await github("POST", `${repo}/issues/${pull.number}/labels`, { labels: [HUMAN_MERGE_HOLD] }, { safeToResend: true }).then( + () => true, + (error) => { + console.log(`::warning title=report-outcome::could not label PR #${pull.number} ${HUMAN_MERGE_HOLD}: ${String(error?.message ?? error)}`); + return false; + }, + )); // A plan with a tip opens only at it: a PR whose head moved off the // checked tip is closed, and the issue goes to a person. Closing a // closed PR changes nothing, so the close is resent. - if (!plan.tip || pull.head?.sha === plan.tip) { + const moved = Boolean(plan.tip) && pull.head?.sha !== plan.tip; + if (labeled && !moved) { console.log(`report-outcome: opened PR #${pull.number} for issue #${issueNumber}`); return 0; } await github("PATCH", `${repo}/pulls/${pull.number}`, { state: "closed" }, { safeToResend: true }); - console.log(`report-outcome: closed PR #${pull.number}: its head is not ${plan.tip}, the tip the runner checked`); - plan = routeStop("pr-head-moved", { + console.log( + moved + ? `report-outcome: closed PR #${pull.number}: its head is not ${plan.tip}, the tip the runner checked` + : `report-outcome: closed PR #${pull.number}: it holds for a person and could not be labeled ${HUMAN_MERGE_HOLD}`, + ); + plan = routeStop(moved ? "pr-head-moved" : "hold-label-failed", { outcome: stopped( - `The PR from \`${plan.branch}\` opened at a head other than ${plan.tip}, the tip the runner checked, so it was closed.`, + moved + ? `The PR from \`${plan.branch}\` opened at a head other than ${plan.tip}, the tip the runner checked, so it was closed.` + : `The PR from \`${plan.branch}\` holds for a person, and the \`${HUMAN_MERGE_HOLD}\` label could not be added, so it was closed.`, ), issueNumber, branch: plan.branch, diff --git a/.github/scripts/work-resolve-issue/report-outcome.test.mjs b/.github/scripts/work-resolve-issue/report-outcome.test.mjs index cdc96f9cb7e..5d48fd88a7e 100644 --- a/.github/scripts/work-resolve-issue/report-outcome.test.mjs +++ b/.github/scripts/work-resolve-issue/report-outcome.test.mjs @@ -33,6 +33,7 @@ import { RESUMABLE, reusePlan, HOLD_LINE, + HUMAN_MERGE_HOLD, trustedLabeler, unmarkedAnswer, } from "./report-outcome.mjs"; @@ -762,7 +763,13 @@ function fakeFetch(routes) { const path = new URL(url).pathname; calls.push({ method, path, body: init.body ? JSON.parse(init.body) : undefined }); // An issue no open PR closes, unless a route says otherwise. - const none = path === "/graphql" ? [method, /./, 200, { data: { repository: { issue: { closedByPullRequestsReferences: { nodes: [], pageInfo: { hasNextPage: false } } } } } }] : null; + // A held PR's human-merge-hold label write succeeds, unless a route says otherwise. + const none = + path === "/graphql" + ? [method, /./, 200, { data: { repository: { issue: { closedByPullRequestsReferences: { nodes: [], pageInfo: { hasNextPage: false } } } } } }] + : method === "POST" && /\/issues\/90\/labels$/.test(path) + ? [method, /./, 200, []] + : null; const route = routes.find(([m, p]) => m === method && p.test(path)) ?? none; const [, , status, body] = route ?? [method, /./, 404, { message: "Not Found" }]; return { ok: status < 300, status, json: async () => body, text: async () => JSON.stringify(body) }; @@ -1000,7 +1007,7 @@ test("main refuses an escalation reason it does not know and writes nothing", as }); }); -test("main on done opens a ready PR from the branch and adds no label to it", async () => { +test("main on done opens a ready PR from the branch, labels it human-merge-hold when it holds, and writes nothing to the issue", async () => { await withDir( { "work-outcome.json": JSON.stringify(done), "lane.json": JSON.stringify({ branch: BRANCH }), "runner-checks": checksFile() }, async (dir) => { @@ -1019,7 +1026,8 @@ test("main on done opens a ready PR from the branch and adds no label to it", as assert.equal(pull.head, BRANCH); assert.equal(pull.base, "main"); assert.ok(pull.body.startsWith("Closes #77\n")); - assert.ok(!calls.some((c) => c.path.endsWith("/issues/90/labels"))); + assert.ok(pull.body.split("\n").includes(HOLD_LINE), pull.body); + assert.deepEqual(calls.filter((c) => c.path.endsWith("/issues/90/labels")).map((c) => c.body), [{ labels: ["human-merge-hold"] }]); assert.ok(!calls.some((c) => c.method === "POST" && c.path.startsWith("/repos/o/r/issues/77/"))); }, ); @@ -1117,7 +1125,8 @@ test("main on done with Refs opens the PR and writes nothing to the issue", asyn ]); assert.equal(await main({ env: env(dir), fetch }), 0); const writes = calls.filter((c) => c.method !== "GET" && c.path !== "/graphql").map((c) => `${c.method} ${c.path}`); - assert.deepEqual(writes, ["POST /repos/o/r/pulls"]); + // No denied-path list reads here, so the PR holds and carries the hold label. + assert.deepEqual(writes, ["POST /repos/o/r/pulls", "POST /repos/o/r/issues/90/labels"]); assert.ok(calls.find((c) => c.path.endsWith("/pulls") && c.method === "POST").body.body.startsWith("Refs: #77\n")); }, ); @@ -1184,7 +1193,7 @@ test("main on a rerun reuses the open PR from the branch", async () => { ]); assert.equal(await main({ env: env(dir), fetch }), 0); assert.ok(!calls.some((c) => c.method === "POST" && c.path.endsWith("/pulls"))); - assert.ok(!calls.some((c) => c.path.endsWith("/issues/90/labels"))); + assert.deepEqual(calls.filter((c) => c.path.endsWith("/issues/90/labels")).map((c) => c.body), [{ labels: ["human-merge-hold"] }]); }, ); }); @@ -2462,7 +2471,8 @@ test("a reused branch with another run's or no outcome opens a held PR with none assert.ok(pull.body.split("\n").includes("Hold: a human merges this"), pull.body); assert.ok(!pull.body.includes("MODEL-SUMMARY-TEXT") && !pull.body.includes("model title"), pull.body); assert.ok(pull.body.includes("https://example/o/r/actions/runs/400"), pull.body); - assert.ok(!calls.some((c) => c.path.endsWith("/comments") || c.path.endsWith("/labels")), Object.keys(files).join()); + assert.ok(!calls.some((c) => c.path.endsWith("/comments") || c.path.endsWith("/issues/77/labels")), Object.keys(files).join()); + assert.deepEqual(calls.filter((c) => c.path.endsWith("/issues/90/labels")).map((c) => c.body), [{ labels: ["human-merge-hold"] }]); }); } }); @@ -2626,7 +2636,7 @@ test("a branch's own outcome wins over a summary, so the summary never stands in // main on a done run of an issue with these labels and `labeled` events, under // a resolved config that denies no path, so only the label can hold the PR. -async function runHumanMerge({ labels, events = [], eventsStatus = 200, trusted = TRUSTED_LIST }) { +async function runHumanMerge({ labels, events = [], eventsStatus = 200, trusted = TRUSTED_LIST, open = [], labelStatus = 200 }) { return withAsyncDir( { "work-outcome.json": JSON.stringify(done), @@ -2642,14 +2652,21 @@ async function runHumanMerge({ labels, events = [], eventsStatus = 200, trusted branchTip(), ["GET", /\/repos\/o\/r$/, 200, { default_branch: "main" }], ["GET", /\/compare\//, 200, { files: [{ filename: "README.md" }] }], - ["GET", /\/pulls$/, 200, []], + ["GET", /\/pulls$/, 200, open], ["POST", /\/pulls$/, 201, { number: 90 }], + ["POST", /\/issues\/90\/labels$/, labelStatus, []], + ["PATCH", /\/pulls\/90$/, 200, {}], + ...HISTORY, + ["POST", /\/issues\/77\/labels$/, 200, []], + ["DELETE", /\/issues\/77\/labels\//, 200, []], + ["POST", /\/issues\/77\/comments$/, 201, {}], ]); const code = await main({ env: { ...env(dir), RESOLVED_CONFIG: join(dir, "resolved.json"), TRUSTED_ACTORS: join(dir, "trusted.json") }, fetch, + sleep: async () => {}, }); - return { code, calls, body: calls.find((c) => c.method === "POST" && c.path.endsWith("/pulls")).body.body }; + return { code, calls, body: calls.find((c) => c.method === "POST" && c.path.endsWith("/pulls"))?.body.body }; }, ); } @@ -2674,6 +2691,50 @@ test("main without merge: human holds no PR and reads no label events", async () assert.ok(!calls.some((c) => c.path.endsWith("/events"))); }); +// #9437: the hold line is body text the lanes App can edit, so a held PR also +// carries a label only a listed person's removal lifts (ci-status's +// human-merge hold step). +const holdLabelPosts = (calls) => calls.filter((c) => c.method === "POST" && c.path.endsWith("/issues/90/labels")); + +test("main labels a held PR human-merge-hold right after it opens", async () => { + assert.equal(HUMAN_MERGE_HOLD, "human-merge-hold"); + const { code, calls } = await runHumanMerge({ labels: MERGE_HUMAN, events: [labeled("merge: human", "kyle-sexton", OWNER_ID)] }); + assert.equal(code, 0); + assert.deepEqual(holdLabelPosts(calls).map((c) => c.body), [{ labels: ["human-merge-hold"] }]); + assert.ok(!calls.some((c) => c.method === "PATCH")); +}); + +test("main puts no human-merge-hold label on a PR that does not hold", async () => { + const { code, calls } = await runHumanMerge({ labels: ["agent-ready", "work-class: scoped"] }); + assert.equal(code, 0); + assert.deepEqual(holdLabelPosts(calls), []); +}); + +test("main labels a held PR it reuses from an earlier attempt", async () => { + const { code, calls } = await runHumanMerge({ + labels: MERGE_HUMAN, + events: [labeled("merge: human", "kyle-sexton", OWNER_ID)], + open: [{ number: 90, user: { login: BOT } }], + }); + assert.equal(code, 0); + assert.ok(!calls.some((c) => c.method === "POST" && c.path.endsWith("/pulls"))); + assert.deepEqual(holdLabelPosts(calls).map((c) => c.body), [{ labels: ["human-merge-hold"] }]); +}); + +test("main closes a held PR it cannot label and retries the issue", async () => { + const { code, calls } = await runHumanMerge({ + labels: MERGE_HUMAN, + events: [labeled("merge: human", "kyle-sexton", OWNER_ID)], + labelStatus: 500, + }); + assert.equal(code, 0); + assert.ok(holdLabelPosts(calls).length >= 2, "the label write is resent"); + assert.deepEqual(calls.find((c) => c.method === "PATCH" && c.path.endsWith("/pulls/90"))?.body, { state: "closed" }); + const comment = calls.find((c) => c.method === "POST" && c.path.endsWith("/issues/77/comments")).body.body; + assert.ok(comment.startsWith("