From 0f171b3d338c5cc53317f175c319990d322a8b91 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 10:54:58 -0400 Subject: [PATCH 1/2] fix(apply-review-hold): carry no cost decision past a deleted comment The decided head is the newest cost hold marker before the decision. If that marker was deleted, an older one became the decided head, so a decision could carry to a diff the owner never saw. carryDecision now reads the pull request timeline's COMMENT_DELETED_EVENT items after the marker's edit check and carries nothing when any comment was deleted at or after the marker was posted, or when the list cannot show none was (failed read, filteredCount above the listed nodes, no list or integer count, or a time that does not parse). This is the rule merge-holds.mjs deletedSince applies in melodic-software/claude-code-plugins#9569. Refs: melodic-software/claude-code-plugins#9436 Co-Authored-By: Claude Opus 5.5 --- .github/actions/apply-review-hold/hold.cjs | 50 +++++++- .github/scripts/apply-review-hold.test.cjs | 136 +++++++++++++++++++-- .github/workflows/pr-review-cli.yml | 5 +- 3 files changed, 180 insertions(+), 11 deletions(-) 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..c5d98cac 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,99 @@ 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 (owner decision Q18 d on +// melodic-software/claude-code-plugins#9436, 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 From 1a570ecf75dff314dcddd642089e5144eaaf64cd Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 11:02:57 -0400 Subject: [PATCH 2/2] test(apply-review-hold): drop the tracker reference from a test comment comment-hygiene flags an owner/repo#number reference in a code comment. Co-Authored-By: Claude Opus 5.5 --- .github/scripts/apply-review-hold.test.cjs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/scripts/apply-review-hold.test.cjs b/.github/scripts/apply-review-hold.test.cjs index c5d98cac..01638ceb 100644 --- a/.github/scripts/apply-review-hold.test.cjs +++ b/.github/scripts/apply-review-hold.test.cjs @@ -992,10 +992,9 @@ test("a failed marker edit check keeps the cost hold", async () => { }); // A deleted comment can remove the newest marker, which would make an older -// one the decided head (owner decision Q18 d on -// melodic-software/claude-code-plugins#9436, 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 +// 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