From 333bdd86d498150ee09b84227b8fe178c64c2adc Mon Sep 17 00:00:00 2001 From: bompus Date: Sat, 17 Oct 2026 15:06:50 -0600 Subject: [PATCH 1/2] fix(hr-pr-followup): refuse to merge while a check on the head commit is unfinished or failed check-pr-review.ts looked only at CodeRabbit and cubic, so merge-pr.sh could land a pull request whose CI check was red or still running. It now also refuses any other check run that is not COMPLETED or concluded something other than SUCCESS, NEUTRAL or SKIPPED, and any commit status that is not SUCCESS. A head commit with no checks passes, and --wait polls while another check runs. --- CHANGELOG.md | 4 + skills/hr-pr-followup/SKILL.md | 9 ++- .../references/review-helpers.md | 11 ++- .../hr-pr-followup/scripts/check-pr-review.ts | 22 +++++- skills/hr-pr-followup/scripts/merge-pr.sh | 3 +- .../test/check-pr-review.test.js | 74 +++++++++++++++++-- 6 files changed, 108 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 00636376..93f3fc6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ Notable changes to house-rules. Versions follow [semantic versioning](https://se - Repository only (not in the npm package): the end-of-reply eval grader no longer drops a reply's opening prose when a scenario allows reads. A reply with a status line, a read-only call and an acting call, and no prose after them, is now graded whole. Its required patterns can match the status line. +- `check-pr-review.ts`, and so `merge-pr.sh`, now refuses while any check on the head commit is + unfinished, failed, cancelled or timed out. It used to look only at CodeRabbit and cubic, so a + pull request with a red CI check could merge. Neutral and skipped check runs still pass, a head + commit with no checks passes, and `--wait` polls while another check runs. ## 0.15.2 - 2026-10-17 diff --git a/skills/hr-pr-followup/SKILL.md b/skills/hr-pr-followup/SKILL.md index 86408183..ce0aed8d 100644 --- a/skills/hr-pr-followup/SKILL.md +++ b/skills/hr-pr-followup/SKILL.md @@ -56,10 +56,11 @@ Clean CI alone is not evidence that every review finding was handled. When the repository or user selects these GitHub helper policies, use [the CodeRabbit and cubic review gate and one-thread resolver](references/review-helpers.md). -Its review-status checks cover CodeRabbit and cubic only; its thread check covers any -reviewer and its review-body check any bot. It does not replace other required -checks or the caller's judgment. The same reference describes the landing helper scripts (`merge-pr.sh`, -`verify-landed.sh`, `check-landing.py`). +Its review-status checks cover CodeRabbit and cubic; its thread check covers any +reviewer and its review-body check any bot. It also refuses while any other check on the head +commit is unfinished or did not pass. It does not replace the caller's judgment. The same +reference describes the landing helper scripts (`merge-pr.sh`, `verify-landed.sh`, +`check-landing.py`). Immediately before landing, re-read the head, base, checks, review bodies and unresolved threads. If the head or base changed, discard readiness and repeat diff --git a/skills/hr-pr-followup/references/review-helpers.md b/skills/hr-pr-followup/references/review-helpers.md index 1862f2ed..8f3501b5 100644 --- a/skills/hr-pr-followup/references/review-helpers.md +++ b/skills/hr-pr-followup/references/review-helpers.md @@ -20,8 +20,15 @@ outside-diff, nitpick or duplicate findings. A completed CodeRabbit CheckRun must conclude SUCCESS; failed, cancelled, skipped or missing conclusions block the gate. A CodeRabbit status also requires SUCCESS and a description starting with "Review completed"; that is how the gate tests the general squash rule that an -existing review check must report completion. It does not check every CI result or -authorize landing. +existing review check must report completion. + +Every other check on the head commit must have finished and passed. A check run that is +not COMPLETED, or that concluded anything but SUCCESS, NEUTRAL or SKIPPED, blocks the gate, +and so does a commit status that is not SUCCESS. A head commit with no checks is not blocked, +so a repository without CI passes on this point. The gate does not tell required checks from +optional ones. A check that fails for a known unrelated reason blocks the merge until it passes +or the owner decides to land another way. `--wait` also polls while such a check runs. The gate +does not authorize landing. The check named exactly "cubic ยท AI code reviewer" on the head commit is held to the same rule: while it runs, the gate refuses, and a completed one must conclude diff --git a/skills/hr-pr-followup/scripts/check-pr-review.ts b/skills/hr-pr-followup/scripts/check-pr-review.ts index 9647f7d5..7d341a0b 100644 --- a/skills/hr-pr-followup/scripts/check-pr-review.ts +++ b/skills/hr-pr-followup/scripts/check-pr-review.ts @@ -14,9 +14,12 @@ // not blocked, since cubic's `cubic.yaml` is optional and its absence does not show that cubic // is expected, and --wait does not wait for one to appear. Missing or incomplete reviews // require inspection of the review integration and the repository's selected review policy. +// Every other check on the head commit must have finished and passed too: a check run that is not +// COMPLETED, or that concluded anything but SUCCESS, NEUTRAL or SKIPPED, and a commit status that +// is not SUCCESS, refuse. A head commit with no checks is not blocked. // bun scripts/check-pr-review.ts [--repo /] [--wait [--timeout ]] // Exit 1 while feedback is open, 2 when gh fails. Without --repo, gh resolves the repository from -// the current checkout. --wait polls every 30 s while CodeRabbit's or cubic's review is still running, and for +// the current checkout. --wait polls every 30 s while CodeRabbit's or cubic's review or any other check is still running, and for // up to 3 minutes while the head commit has no CodeRabbit check yet (CodeRabbit posts it a little // after a push), then reports as usual; after --timeout minutes (default 30) it reports what is // still open. @@ -44,6 +47,9 @@ const BODY_FINDINGS = /Outside diff range comments|Nitpick comments|Duplicate co const REVIEWED = /^Review completed/; const FOLLOW_UP = "check the review integration and repository review policy"; const RUNNING = "review still running: "; +const CHECK_RUNNING = "check still running: "; +// Check-run conclusions that do not block a merge; every other conclusion does. +const PASSING_CONCLUSIONS = ["SUCCESS", "NEUTRAL", "SKIPPED"]; const NO_CHECK = "no CodeRabbit check on the head commit"; // A neutral cubic result means it did not review the head; a person has to ask again. const CUBIC_RETRY = "comment `@cubic review` or wait for its limit to clear"; @@ -382,6 +388,17 @@ export function openFeedback(pr: ReviewState): string[] { const running = ctx.state === "PENDING" || (ctx.status !== undefined && ctx.status !== "COMPLETED"); if (!isReviewBot(name)) { + if (ctx.status !== undefined) { + if (ctx.status !== "COMPLETED") { + open.push(`${CHECK_RUNNING}${name}`); + } else if (!PASSING_CONCLUSIONS.includes(ctx.conclusion ?? "")) { + open.push(`check ${name} concluded ${ctx.conclusion ?? "(missing conclusion)"}`); + } + } else if (ctx.state === "PENDING" || ctx.state === "EXPECTED") { + open.push(`${CHECK_RUNNING}${name}`); + } else if (ctx.state !== "SUCCESS") { + open.push(`check ${name} reported ${ctx.state ?? "(missing state)"}`); + } continue; } if (running) { @@ -441,7 +458,8 @@ export function openFeedback(pr: ReviewState): string[] { return open; } -const reviewRunning = (open: string[]) => open.some((line) => line.startsWith(RUNNING)); +const reviewRunning = (open: string[]) => + open.some((line) => line.startsWith(RUNNING) || line.startsWith(CHECK_RUNNING)); /** * Re-reads the pull request while a review is still running, or briefly while its check has not diff --git a/skills/hr-pr-followup/scripts/merge-pr.sh b/skills/hr-pr-followup/scripts/merge-pr.sh index 5162be3d..a07fcd82 100644 --- a/skills/hr-pr-followup/scripts/merge-pr.sh +++ b/skills/hr-pr-followup/scripts/merge-pr.sh @@ -6,7 +6,8 @@ # worktree or local branch: it lists the worktrees still on the head so their owners clean up. # # Refuses a PR that is not open, one with open review feedback (check-pr-review.ts: an unresolved -# thread, a review bot still running, unanswered findings in a bot's review body), or one another +# thread, a review bot still running, unanswered findings in a bot's review body, any other check on +# the head commit that is unfinished or did not pass), or one another # open PR targets (deleting the head would close that PR; retarget it first with # `gh pr edit --base `). The merge passes # --match-head-commit, so a push after the head was read fails the merge instead of landing it. diff --git a/skills/hr-pr-followup/test/check-pr-review.test.js b/skills/hr-pr-followup/test/check-pr-review.test.js index d6942047..2a29cda4 100644 --- a/skills/hr-pr-followup/test/check-pr-review.test.js +++ b/skills/hr-pr-followup/test/check-pr-review.test.js @@ -34,7 +34,7 @@ test("passes once threads are resolved, the bot is done and body findings were a comments: [ack()], contexts: [ { context: "CodeRabbit", state: "SUCCESS", description: "Review completed" }, - { name: "check", status: "COMPLETED" }, + { name: "check", status: "COMPLETED", conclusion: "SUCCESS" }, ], }); expect(openFeedback(state)).toEqual([]); @@ -70,7 +70,7 @@ test("duplicate body findings still need acknowledgement after the earlier threa expect(openFeedback({ ...state, comments: { nodes: [ack()] } })).toEqual([]); }); -test("blocks unresolved threads and a review bot still running, but not other pending checks", () => { +test("blocks unresolved threads, a review bot still running and another running check", () => { const state = pr({ threads: [{ isResolved: false, comments: { nodes: [{ url: "https://example.test/t1" }] } }], contexts: [ @@ -81,6 +81,7 @@ test("blocks unresolved threads and a review bot still running, but not other pe expect(openFeedback(state)).toEqual([ "unresolved thread: https://example.test/t1", "review still running: CodeRabbit", + "check still running: build", ]); }); @@ -88,7 +89,7 @@ test("blocks unresolved threads and a review bot still running, but not other pe test("blocks a repository with .coderabbit.yaml whose head commit has no CodeRabbit check", () => { const state = pr({ files: [".coderabbit.yaml", "README.md"], - contexts: [{ name: "check", status: "COMPLETED" }], + contexts: [{ name: "check", status: "COMPLETED", conclusion: "SUCCESS" }], }); expect(openFeedback(state)).toEqual([ "no CodeRabbit check on the head commit: check the review integration and repository review policy", @@ -150,10 +151,14 @@ test("blocks a cubic check that concluded without reviewing the head", () => { test("passes a successful cubic check and a repository with no cubic check", () => { const done = { name: CUBIC, status: "COMPLETED", conclusion: "SUCCESS" }; expect(openFeedback(pr({ contexts: [done] }))).toEqual([]); - expect(openFeedback(pr({ contexts: [{ name: "check", status: "COMPLETED" }] }))).toEqual([]); + expect( + openFeedback(pr({ contexts: [{ name: "check", status: "COMPLETED", conclusion: "SUCCESS" }] })), + ).toEqual([]); + // Names that merely contain "cubic" are ordinary checks: no cubic retry advice, same pass rules. for (const other of ["bicubic-resize tests", "cubic-tests", "cubic-lint"]) { const similar = { name: other, status: "COMPLETED", conclusion: "FAILURE" }; - expect(openFeedback(pr({ contexts: [similar] }))).toEqual([]); + expect(openFeedback(pr({ contexts: [similar] }))).toEqual([`check ${other} concluded FAILURE`]); + expect(openFeedback(pr({ contexts: [{ ...similar, conclusion: "SUCCESS" }] }))).toEqual([]); } }); @@ -209,7 +214,10 @@ test("waitForReview polls while the review runs and stops when it finishes or ti // Right after a push CodeRabbit has not posted its check yet; --wait used to give up at once. test("waitForReview waits a few minutes for a CodeRabbit check that has not appeared yet", async () => { const files = [".coderabbit.yaml"]; - const missing = pr({ files, contexts: [{ name: "check", status: "COMPLETED" }] }); + const missing = pr({ + files, + contexts: [{ name: "check", status: "COMPLETED", conclusion: "SUCCESS" }], + }); const running = pr({ files, contexts: [{ context: "CodeRabbit", state: "PENDING" }] }); const done = pr({ files, @@ -229,6 +237,60 @@ test("waitForReview waits a few minutes for a CodeRabbit check that has not appe expect(clock).toBe(180_000); }); +test("blocks any other check that failed, was cancelled or is unfinished, and passes finished ones", () => { + const run = (conclusion, status = "COMPLETED") => ({ name: "build", status, conclusion }); + for (const conclusion of [ + "FAILURE", + "CANCELLED", + "TIMED_OUT", + "ACTION_REQUIRED", + "STARTUP_FAILURE", + "STALE", + ]) { + expect(openFeedback(pr({ contexts: [run(conclusion)] }))).toEqual([ + `check build concluded ${conclusion}`, + ]); + } + expect(openFeedback(pr({ contexts: [run(null)] }))).toEqual([ + "check build concluded (missing conclusion)", + ]); + for (const conclusion of ["SUCCESS", "NEUTRAL", "SKIPPED"]) { + expect(openFeedback(pr({ contexts: [run(conclusion)] }))).toEqual([]); + } + for (const status of ["QUEUED", "IN_PROGRESS", "WAITING", "PENDING", "REQUESTED"]) { + expect(openFeedback(pr({ contexts: [run(null, status)] }))).toEqual([ + "check still running: build", + ]); + } +}); + +test("blocks a commit status that is not SUCCESS", () => { + const status = (state) => ({ context: "ci/legacy", state, description: null }); + expect(openFeedback(pr({ contexts: [status("SUCCESS")] }))).toEqual([]); + for (const state of ["PENDING", "EXPECTED"]) { + expect(openFeedback(pr({ contexts: [status(state)] }))).toEqual([ + "check still running: ci/legacy", + ]); + } + for (const state of ["FAILURE", "ERROR"]) { + expect(openFeedback(pr({ contexts: [status(state)] }))).toEqual([ + `check ci/legacy reported ${state}`, + ]); + } +}); + +test("waitForReview polls while another check runs and returns its failure", async () => { + const running = pr({ contexts: [{ name: "build", status: "IN_PROGRESS", conclusion: null }] }); + const failed = pr({ contexts: [{ name: "build", status: "COMPLETED", conclusion: "FAILURE" }] }); + let clock = 0; + const opts = { pollMs: 30_000, sleep: async (ms) => (clock += ms), now: () => clock }; + const reads = [running, running, failed]; + expect(await waitForReview(() => reads.shift(), { ...opts, timeoutMs: 600_000 })).toEqual([ + "check build concluded FAILURE", + ]); + expect(clock).toBe(60_000); +}); + function ack(overrides = {}) { return { author: user, From 71ecf95f5bcc73def8b2ea27c7ce4c7b00d61842 Mon Sep 17 00:00:00 2001 From: bompus Date: Sat, 17 Oct 2026 15:18:14 -0600 Subject: [PATCH 2/2] docs(hr-pr-followup): --wait also polls while any other check runs --- skills/hr-pr-followup/references/review-helpers.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/hr-pr-followup/references/review-helpers.md b/skills/hr-pr-followup/references/review-helpers.md index 8f3501b5..eb5bbc68 100644 --- a/skills/hr-pr-followup/references/review-helpers.md +++ b/skills/hr-pr-followup/references/review-helpers.md @@ -62,8 +62,8 @@ judges whether the explanation answers the findings. Connections are paginated. Partial responses, malformed fields, repeated cursors and head changes fail closed. A final head read does not make review -feedback an atomic snapshot. `--wait --timeout 30` polls only while CodeRabbit -or cubic runs, or briefly while CodeRabbit's expected check has not appeared. +feedback an atomic snapshot. `--wait --timeout 30` polls while CodeRabbit, +cubic or any other check runs, or briefly while CodeRabbit's expected check has not appeared. ## Resolve one review thread