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..eb5bbc68 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 @@ -55,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 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,