diff --git a/.github/actions/apply-review-hold/hold.cjs b/.github/actions/apply-review-hold/hold.cjs index 14c1071c..ebde7964 100644 --- a/.github/actions/apply-review-hold/hold.cjs +++ b/.github/actions/apply-review-hold/hold.cjs @@ -78,6 +78,11 @@ const PULL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { }`; const EDITED_QUERY = "query($id: ID!) { node(id: $id) { ... on IssueComment { lastEditedAt } } }"; +const DELETED_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { timelineItems(itemTypes: [COMMENT_DELETED_EVENT], last: 100) { filteredCount nodes { ... on CommentDeletedEvent { createdAt } } } } + } +}`; // Whether comment `c` was ever edited: "unedited" when GraphQL answers // lastEditedAt null, "edited" when it answers a time, else "unknown" (a @@ -93,6 +98,34 @@ async function editState(github, c) { } } +// Whether a comment on pull request `number` was deleted at or after `at` +// (merge-holds.mjs deletedSince at claude-code-plugins#9569's head +// 149f3214): "deleted", "none", or "unknown" when the read fails, lists +// fewer deletions than its filteredCount, has no list or integer count, or +// holds a time that does not parse. A deleted newer marker would otherwise +// make an older one the decided head. +async function deletedSince(github, owner, repo, number, at) { + let items; + try { + items = (await github.graphql(DELETED_QUERY, { owner, name: repo, number })) + ?.repository?.pullRequest?.timelineItems; + } catch { + return "unknown"; + } + const when = Date.parse(at); + const nodes = items?.nodes; + if ( + !Number.isFinite(when) || + !Array.isArray(nodes) || + !Number.isInteger(items.filteredCount) || + items.filteredCount > nodes.length + ) + return "unknown"; + const times = nodes.map((node) => Date.parse(node?.createdAt)); + if (!times.every(Number.isFinite)) return "unknown"; + return times.some((t) => t >= when) ? "deleted" : "none"; +} + // The issue one plain `Refs: #` line names, or null: claude-code-plugins@eda837d4c8 // plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py:1851-1895 // refs_issue. A line counts from its first column, outside an HTML comment @@ -225,14 +258,17 @@ const BRANCH = /^(?!-)(?!.*\.\.)[A-Za-z0-9._/-]{1,255}$/u; // the sha of this lane's newest cost hold marker posted before the decision, // and only while that marker is unedited: an edited marker, or one whose // edit state cannot be read, carries nothing, and no older marker is used. -// The decision carries when the head is that sha, or when the pull request's +// It carries nothing either when any comment on the pull request was +// deleted at or after that marker was posted, or the deletion list cannot +// show none was (deletedSince), read before the head check so deleting a +// newer marker cannot promote an older head. The decision carries when the head is that sha, or when the pull request's // net diff against its base branch is the same at both: compare // base...decided and base...head (three-dot, so each is the diff from its // merge base) list the same files, each with the same status, // previous_filename and patch text. So a base-only merge or a rebase that // leaves every patch as it was carries, and any other change does not. One -// edit read and at most two compare reads; any failure, a missing patch, or -// a list of 300 or more files carries nothing. +// edit read, one deletion read and at most two compare reads; any failure, +// a missing patch, or a list of 300 or more files carries nothing. async function carryDecision(github, owner, repo, env, prComments, decision) { const cutoff = Date.parse(decision.created_at); if (!Number.isFinite(cutoff)) return { carry: false, why: "the decision has no time" }; @@ -260,6 +296,14 @@ async function carryDecision(github, owner, repo, env, prComments, decision) { return { carry: false, why: "the decided-head marker was edited" }; if (state !== "unedited") return { carry: false, why: "could not confirm the marker is unedited" }; + const deleted = await deletedSince(github, owner, repo, Number(env.PR_NUMBER), held.created_at); + if (deleted === "deleted") + return { carry: false, why: "a comment was deleted since the decided-head marker was posted" }; + if (deleted !== "none") + return { + carry: false, + why: "could not confirm no comment was deleted since the decided-head marker was posted", + }; if (decided === env.HEAD_SHA) return { carry: true, why: `the head is the decided head ${decided}` }; const base = String(env.BASE_REF ?? ""); diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index 89d94f13..01638ceb 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -47,7 +47,9 @@ const TRUST_PATHS = [ // edit read fails; `pullNumbers` the numbers // that are pull requests, not issues; `misnumbered` the numbers whose read // answers another number; `compares` maps a basehead to its compare -// response data. +// response data; `deletions` the created_at of each COMMENT_DELETED_EVENT +// on the pull request, `deletedItems` a raw timelineItems answer in their +// place, and `failDeleted` fails that read. function fakeGithub({ files = [], changedFiles, @@ -64,6 +66,9 @@ function fakeGithub({ pullNumbers = new Set([7]), misnumbered = new Set(), compares = {}, + deletions = [], + deletedItems, + failDeleted = false, }) { const calls = []; const refuse = (name) => () => { @@ -75,6 +80,22 @@ function fakeGithub({ calls.push(["graphql", variables]); if (/lastEditedAt/u.test(query) && failEdited.has(variables.id)) throw new Error("HTTP 502"); + if (/COMMENT_DELETED_EVENT/u.test(query)) { + if (failDeleted) throw new Error("HTTP 502"); + return { + repository: { + pullRequest: { + timelineItems: + deletedItems !== undefined + ? deletedItems + : { + filteredCount: deletions.length, + nodes: deletions.map((createdAt) => ({ createdAt })), + }, + }, + }, + }; + } if (/lastEditedAt/u.test(query)) return { node: { @@ -476,14 +497,17 @@ test("a cost decision on the pull request keeps the cost hold from coming back", assert.equal(core.outputs.hold, "false"); assert.deepEqual(writes, []); // The lookup's reads, the pull request's comments and the decision's edit - // check, then the marker's edit check, then hold-paths' own file list: the - // head is the decided head, so no compare. + // check, then the marker's edit check and the deletion read, then + // hold-paths' own file list: the head is the decided head, so no compare. assert.deepEqual( github.calls.map(([name]) => name), - ["listComments", "graphql", "graphql", "listFiles", "get"], + ["listComments", "graphql", "graphql", "graphql", "listFiles", "get"], ); + assert.deepEqual(github.calls[3][1], { owner: "o", name: "r", number: 7 }); assert.deepEqual( - github.calls.filter(([name]) => name === "graphql").map(([, v]) => v.id), + github.calls + .filter(([name, v]) => name === "graphql" && "id" in v) + .map(([, v]) => v.id), ["IC_1", "IC_100"], ); }); @@ -509,6 +533,7 @@ test("a cost decision on the Refs issue that names the pull request keeps the co "listComments", "graphql", "graphql", + "graphql", "listFiles", "get", ], @@ -895,15 +920,16 @@ test("an unusable base branch name reads no compare and keeps the cost hold", as assert.equal(compares(github).length, 0); }); -test("the carry check reads at most two compares and nothing else", async () => { +test("the carry check reads one deletion list, at most two compares and nothing else", async () => { const { github } = await carryCase(atDecided); const names = github.calls.map(([name]) => name); // The pull request's comments, the decision's and the marker's edit - // checks, two compares, then hold-paths' own file list. + // checks, the deletion read, two compares, then hold-paths' own file list. assert.deepEqual(names, [ "listComments", "graphql", "graphql", + "graphql", "compare", "compare", "listFiles", @@ -964,3 +990,98 @@ test("a failed marker edit check keeps the cost hold", async () => { /no recorded cost decision carries \(could not confirm the marker is unedited\)/u, ); }); + +// A deleted comment can remove the newest marker, which would make an older +// one the decided head (as merge-holds.mjs deletedSince reads it): any +// comment deleted at or after the decided-head marker was posted carries +// nothing. The pull request timeline's COMMENT_DELETED_EVENT +// items give the deletion times. +test("a comment deleted after the decided-head marker keeps the cost hold, with no older marker standing in", async () => { + // The newer marker, at 11:00, was deleted; the one left, at 10:30, holds + // the head's sha and would carry with no compare. + const { core, writes, github } = await carryCase(atDecided, { + comments: [ + marker({ id: 101, sha: LATER, createdAt: "2026-10-23T10:30:00Z" }), + decision(), + ], + deletions: ["2026-10-23T11:05:00Z"], + }); + assert.equal(core.outputs.hold, "true"); + assert.equal(core.outputs.criteria, "cost"); + // The label comes back; the remaining marker already names this head. + assert.deepEqual( + writes.map(([name]) => name), + ["addLabels"], + ); + assert.equal(compares(github).length, 0); + assert.match( + core.infos.join("\n"), + /no recorded cost decision carries \(a comment was deleted since the decided-head marker was posted\)/u, + ); +}); + +test("a deletion at the marker's own time keeps the cost hold, and one before it does not", async () => { + const at = await apply(costEnv(), { + comments: [marker(), decision()], + deletions: ["2026-10-23T10:00:00Z", "2026-10-23T11:00:00Z"], + }); + assert.equal(at.core.outputs.hold, "true"); + const before = await apply(costEnv(), { + comments: [marker(), decision()], + deletions: ["2026-10-23T10:59:59Z"], + }); + assert.equal(before.core.outputs.hold, "false"); + assert.match( + before.core.infos.join("\n"), + /the recorded cost decision carries \(the head is the decided head/u, + ); +}); + +test("a deletion list that cannot show no comment was deleted keeps the cost hold", async () => { + const listed = (filteredCount, nodes) => ({ filteredCount, nodes }); + for (const [name, options] of [ + ["a failed read", { failDeleted: true }], + ["no timeline", { deletedItems: null }], + ["no node list", { deletedItems: { filteredCount: 0 } }], + ["no count", { deletedItems: { nodes: [] } }], + ["a count that is not a number", { deletedItems: listed("0", []) }], + [ + "more deletions than listed", + { + deletedItems: listed( + 101, + Array.from({ length: 100 }, () => ({ + createdAt: "2026-10-23T09:00:00Z", + })), + ), + }, + ], + [ + "a time that does not parse", + { deletedItems: listed(1, [{ createdAt: "yesterday" }]) }, + ], + ["an event with no time", { deletedItems: listed(1, [{}]) }], + ]) { + const { core, error, github } = await apply(costEnv(), { + comments: [marker(), decision()], + ...options, + }); + assert.equal(error, undefined, name); + assert.equal(core.outputs.hold, "true", name); + assert.equal(compares(github).length, 0, name); + assert.match( + core.infos.join("\n"), + /no recorded cost decision carries \(could not confirm no comment was deleted since the decided-head marker was posted\)/u, + name, + ); + } +}); + +test("an edited marker reads no deletion list", async () => { + const { github } = await apply(costEnv(), { + comments: [marker(), decision()], + edited: new Set(["IC_100"]), + failDeleted: true, + }); + assert.equal(github.calls.filter(([name]) => name === "graphql").length, 2); +}); diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 91686452..1a6ff6d6 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -112,7 +112,10 @@ name: pr-review-cli # before the decision; with none, nothing carries. That marker must be # unedited (GraphQL lastEditedAt null): an edited marker, or one whose # edit state cannot be read, carries nothing, and no older marker is -# used. It covers the head when the head is that sha, or when the pull +# used. Nor does it carry when the pull request timeline shows a +# comment deleted at or after that marker was posted +# (COMMENT_DELETED_EVENT), or cannot show none was: a failed read, more +# deletions than it lists, or a time that does not parse. It covers the head when the head is that sha, or when the pull # request's net diff is the same at both: the three-dot compares # base...decided and base...head list the same files with the same # status, previous_filename and patch text. A base-only merge or a rebase that leaves every patch alone