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
17 changes: 17 additions & 0 deletions .changes/pr-pipeline/human-merge-hold-label-9437a4d1.md
Original file line number Diff line number Diff line change
@@ -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.
9 changes: 9 additions & 0 deletions .changes/source-control/human-merge-hold-label-9437a4d1.md
Original file line number Diff line number Diff line change
@@ -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)).
2 changes: 1 addition & 1 deletion .github/scripts/pr-merge/enable-auto-merge.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 19 additions & 0 deletions .github/scripts/pr-merge/enable-auto-merge.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
UNLABELED_QUERY,
decide,
describe,
holdReason,
unblockHold,
liftedByPerson,
READER_EVENTS,
Expand Down Expand Up @@ -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 }]);
});

// report-outcome labels a held PR human-merge-hold, so a lanes App edit of
// the body (the hold line deleted, or the Refs line rewritten to a closing
// line) 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 }),
// The Refs line rewritten to a closing line, 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
Expand Down
9 changes: 8 additions & 1 deletion .github/scripts/pr-merge/merge-holds.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -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);
}

Expand Down
1 change: 1 addition & 0 deletions .github/scripts/pr-merge/merge-holds.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
33 changes: 28 additions & 5 deletions .github/scripts/work-resolve-issue/report-outcome.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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-<digit>`, so the squash commit title
// cannot name an issue to close.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1062,6 +1066,7 @@ export function decide({
title: prTitle(outcome.title, issueTitle, issueNumber),
body,
branch,
hold,
};
}

Expand Down Expand Up @@ -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,
Expand Down
79 changes: 70 additions & 9 deletions .github/scripts/work-resolve-issue/report-outcome.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
RESUMABLE,
reusePlan,
HOLD_LINE,
HUMAN_MERGE_HOLD,
trustedLabeler,
unmarkedAnswer,
} from "./report-outcome.mjs";
Expand Down Expand Up @@ -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) };
Expand Down Expand Up @@ -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) => {
Expand All @@ -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/")));
},
);
Expand Down Expand Up @@ -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"));
},
);
Expand Down Expand Up @@ -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"] }]);
},
);
});
Expand Down Expand Up @@ -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"] }]);
});
}
});
Expand Down Expand Up @@ -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),
Expand All @@ -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 };
},
);
}
Expand All @@ -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")));
});

// 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("<!-- work-resolve-issue outcome=retry"), comment);
assert.match(comment, /`hold-label-failed`/);
});

test("main ignores merge: human from a labeler who is not a listed human or the lanes App", async () => {
const { body } = await runHumanMerge({ labels: MERGE_HUMAN, events: [labeled("merge: human", "stranger", 42)] });
assert.ok(!holds(body), body);
Expand Down
1 change: 1 addition & 0 deletions .github/scripts/work-resolve-issue/stop-routes.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"resumed-unchecked": { "route": "retry", "why": "the runner's checks did not pass over the resumed branch, so the next run continues it" },
"check-failed": { "route": "retry", "why": "a check the runner ran on the branch failed or could not be read, so the next run continues the branch" },
"required-undone": { "route": "retry", "why": "the run left a required check undone, so the next run continues the branch" },
"hold-label-failed": { "route": "retry", "why": "the held PR could not be labeled human-merge-hold, a failed write, not a finding about the issue" },
"pr-head-moved": { "route": "retry", "why": "the PR opened at a head other than the tip the runner checked, a race, not a finding about the issue" },
"closed-by-open-pr": { "route": "retry", "why": "an open PR the lanes App or a listed human opened already closes the issue, a race with the earlier-work check, not a finding about the issue; while it stays open the next run stops before any model turn" },
"turn-cap-spent": { "route": "fail", "why": "the run reached its turn cap with no restart left" },
Expand Down
Loading
Loading