From a2779519433aaf74a18134eb9eb9c5a1f8cfbb21 Mon Sep 17 00:00:00 2001 From: bompus Date: Fri, 16 Oct 2026 10:00:49 -0600 Subject: [PATCH 1/2] feat(hr-pr-followup): block the review gate on a running or non-successful cubic check A running cubic check refuses and --wait polls it. A concluded check must be SUCCESS; NEUTRAL means cubic did not review the head and stays blocked until someone comments @cubic review or the limit clears. A head commit with no cubic check is not blocked. --- CHANGELOG.md | 6 +++ README.md | 2 +- skills/hr-pr-followup/SKILL.md | 4 +- .../references/review-helpers.md | 24 +++++++--- .../hr-pr-followup/scripts/check-pr-review.ts | 21 ++++++--- .../test/check-pr-review.test.js | 46 +++++++++++++++++++ 6 files changed, 87 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89b8f161..dc26fc30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Notable changes to house-rules. Versions follow [semantic versioning](https://se ## Unreleased +- The `hr-pr-followup` review gate (`check-pr-review.ts`) now treats a cubic check on the head + commit like CodeRabbit's. A running cubic check refuses, and `--wait` polls it. A completed one + must conclude SUCCESS, so a NEUTRAL or skipped result, which means cubic did not review the head, + also refuses until someone comments `@cubic review` or its limit clears. A head commit with no + cubic check is not blocked. + - The release procedure (`release-batching` reference) now says to check every package for commits past its latest release tag before any decision to publish. When there are any, it recommends a new release first. This covers public and private packages and republishing an diff --git a/README.md b/README.md index b0ddee28..9e4098b3 100644 --- a/README.md +++ b/README.md @@ -291,7 +291,7 @@ changing the configuration. | `hr-ordering-tests` | Enumerate event orderings through the real code to find race bugs. | | `hr-plain-prose` | Make text people read plain and specific. | | `hr-progress-report` | Report current task activity, milestone completion and remaining wall-clock time. | -| `hr-pr-followup` | Address conflicts, feedback and failing checks for a PR; honor landing authority; CodeRabbit review gate. | +| `hr-pr-followup` | Address conflicts, feedback and failing checks for a PR; landing authority; CodeRabbit and cubic gate. | | `hr-read-reddit` | Read Reddit feed output while supported; RSS support ends November 13, 2026. Partial web-search fallback. | | `hr-read-x-links` | Read the full content of X posts. | | `hr-research-shared-links` | Research a shared link or repository in depth, compare it with your setup and rate each idea. | diff --git a/skills/hr-pr-followup/SKILL.md b/skills/hr-pr-followup/SKILL.md index dbf78577..86408183 100644 --- a/skills/hr-pr-followup/SKILL.md +++ b/skills/hr-pr-followup/SKILL.md @@ -55,8 +55,8 @@ Name the exact revision, failed or unfinished gates and unresolved decisions. 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 review gate and one-thread resolver](references/review-helpers.md). -Its review-status checks cover CodeRabbit only; its thread check covers any +[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`). diff --git a/skills/hr-pr-followup/references/review-helpers.md b/skills/hr-pr-followup/references/review-helpers.md index 9f600d6a..574844eb 100644 --- a/skills/hr-pr-followup/references/review-helpers.md +++ b/skills/hr-pr-followup/references/review-helpers.md @@ -14,12 +14,22 @@ bun scripts/check-pr-review.ts 123 --repo OWNER/REPO The gate exits 0 when its selected feedback checks pass, 1 for open feedback, and 2 when coverage cannot be verified. It checks unresolved threads, pending -CodeRabbit review, skipped review statuses and a missing check when the head -contains `.coderabbit.yaml`. It also checks bot review bodies with 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". It does not -check every CI result or authorize landing. +CodeRabbit or cubic review, skipped review statuses and a missing check when +the head contains `.coderabbit.yaml`. It also checks bot review bodies with +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". It does not check every CI result or authorize landing. + +A check named "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 +SUCCESS. A NEUTRAL or skipped conclusion means cubic did not review the head +("Incremental review could not start", "Automatic AI review not started after +branch rewrite", "AI review line limit reached"), so the gate refuses it too. +That pull request stays blocked until someone comments `@cubic review` or the +limit clears, and the refusal says so. A head commit with no cubic check is not blocked, +because cubic has no config file to require. Right after a push, the gate cannot tell a +missing check from one that has not posted yet. If a review is missing or incomplete, inspect the integration and the repository's review policy. The helper does not determine automatic-review enablement or @@ -42,7 +52,7 @@ 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 -runs, or briefly while its expected check has not appeared. +or cubic 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 33c2232a..b6dcb8f5 100644 --- a/skills/hr-pr-followup/scripts/check-pr-review.ts +++ b/skills/hr-pr-followup/scripts/check-pr-review.ts @@ -1,16 +1,21 @@ #!/usr/bin/env bun // Refuses to land a pull request while review feedback is open: an unresolved review thread, a -// review bot (CodeRabbit) whose check is still running, or a bot review whose body carries findings +// review bot (CodeRabbit or cubic) whose check is still running, or a bot review whose body carries findings // GitHub cannot show as threads ("Outside diff range comments", "Nitpick comments", "Duplicate comments") with no human // explicit acknowledgement for that review and the current head. Run only when the repository // selects this gate. It also refuses when CodeRabbit's status on the // head commit says anything but "Review completed": CodeRabbit marks a skipped review (for example // on a non-default base) as success with "Review skipped: …". When the head commit has -// `.coderabbit.yaml` but no CodeRabbit check, it refuses too. Missing or incomplete reviews +// `.coderabbit.yaml` but no CodeRabbit check, it refuses too. A cubic check ("cubic · AI code +// reviewer") on the head commit must also complete with SUCCESS: a running one refuses, and so does +// any other conclusion, NEUTRAL and skipped included, because NEUTRAL means cubic did not review the +// head ("Incremental review could not start", "AI review line limit reached"). That PR stays blocked +// until someone comments `@cubic review` or the limit clears. A head commit with no cubic check is +// not blocked, since cubic has no config file to require. Missing or incomplete reviews // require inspection of the review integration and the repository's selected review policy. // 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 review is still running, and for +// the current checkout. --wait polls every 30 s while CodeRabbit's or cubic's review 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. @@ -30,13 +35,16 @@ import { spawnSync } from "node:child_process"; import { parseArgs } from "node:util"; -const REVIEW_BOTS = /coderabbit/i; +const REVIEW_BOTS = /coderabbit|^cubic\b/i; +const CUBIC = /^cubic\b/i; const REVIEW_BOT_CONFIG = ".coderabbit.yaml"; const BODY_FINDINGS = /Outside diff range comments|Nitpick comments|Duplicate comments/; const REVIEWED = /^Review completed/; const FOLLOW_UP = "check the review integration and repository review policy"; const RUNNING = "review still running: "; 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"; const POLL_MS = 30_000; const NO_CHECK_GRACE_MS = 180_000; @@ -377,12 +385,13 @@ export function openFeedback(pr: ReviewState): string[] { if (running) { open.push(`${RUNNING}${name}`); } else if (ctx.name !== undefined && ctx.conclusion !== "SUCCESS") { + const retry = CUBIC.test(name) ? `; ${CUBIC_RETRY}` : ""; open.push( - `${name} review concluded ${ctx.conclusion ?? "(missing conclusion)"}: ${FOLLOW_UP}`, + `${name} review concluded ${ctx.conclusion ?? "(missing conclusion)"}: ${FOLLOW_UP}${retry}`, ); } else if ( ctx.context !== undefined && - (ctx.state !== "SUCCESS" || !REVIEWED.test(ctx.description ?? "")) + (ctx.state !== "SUCCESS" || (!CUBIC.test(name) && !REVIEWED.test(ctx.description ?? ""))) ) { open.push( `${name} did not review the head commit ("${ctx.description ?? ""}"): ${FOLLOW_UP}`, 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 18d1ff21..f3710919 100644 --- a/skills/hr-pr-followup/test/check-pr-review.test.js +++ b/skills/hr-pr-followup/test/check-pr-review.test.js @@ -126,6 +126,52 @@ test("failed review statuses cannot pass with a completion description", () => { expect(openFeedback(pr({ contexts: [ctx] }))).toEqual([]); }); +const CUBIC = "cubic · AI code reviewer"; +const FOLLOW_UP = "check the review integration and repository review policy"; +const CUBIC_RETRY = "comment `@cubic review` or wait for its limit to clear"; + +// cubic posts a CheckRun and has no config file, so only a check that exists can block. +test("blocks a cubic check that is still running", () => { + for (const status of ["QUEUED", "IN_PROGRESS", "PENDING"]) { + const ctx = { name: CUBIC, status, conclusion: null }; + expect(openFeedback(pr({ contexts: [ctx] }))).toEqual([`review still running: ${CUBIC}`]); + } +}); + +test("blocks a cubic check that concluded without reviewing the head", () => { + for (const conclusion of ["NEUTRAL", "SKIPPED", "FAILURE", "CANCELLED", "TIMED_OUT", null]) { + const ctx = { name: CUBIC, status: "COMPLETED", conclusion }; + expect(openFeedback(pr({ contexts: [ctx] }))).toEqual([ + `${CUBIC} review concluded ${conclusion ?? "(missing conclusion)"}: ${FOLLOW_UP}; ${CUBIC_RETRY}`, + ]); + } +}); + +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([]); + const similar = { name: "bicubic-resize tests", status: "COMPLETED", conclusion: "FAILURE" }; + expect(openFeedback(pr({ contexts: [similar] }))).toEqual([]); +}); + +test("a cubic commit status needs success but no CodeRabbit description", () => { + expect(openFeedback(pr({ contexts: [{ context: CUBIC, state: "SUCCESS" }] }))).toEqual([]); + expect(openFeedback(pr({ contexts: [{ context: CUBIC, state: "FAILURE" }] }))).toHaveLength(1); +}); + +test("waitForReview polls while cubic runs and stops on its result", async () => { + const running = pr({ contexts: [{ name: CUBIC, status: "IN_PROGRESS", conclusion: null }] }); + const neutral = pr({ contexts: [{ name: CUBIC, status: "COMPLETED", conclusion: "NEUTRAL" }] }); + let clock = 0; + const opts = { pollMs: 30_000, sleep: async (ms) => (clock += ms), now: () => clock }; + const reads = [running, running, neutral]; + const open = await waitForReview(() => reads.shift(), { ...opts, timeoutMs: 600_000 }); + expect(clock).toBe(60_000); + expect(open).toHaveLength(1); + expect(open[0]).toContain("NEUTRAL"); +}); + // Hand-written wait loops read a field that stays blank and never stopped; --wait polls this way. test("waitForReview polls while the review runs and stops when it finishes or time runs out", async () => { const running = pr({ contexts: [{ context: "CodeRabbit", state: "PENDING" }] }); From 235fb9bffff3db95510f41214ba1023963d74b13 Mon Sep 17 00:00:00 2001 From: bompus Date: Fri, 16 Oct 2026 10:13:05 -0600 Subject: [PATCH 2/2] fix(hr-pr-followup): match only cubic's own check and keep the CodeRabbit presence check separate --- skills/hr-pr-followup/references/review-helpers.md | 10 +++++----- skills/hr-pr-followup/scripts/check-pr-review.ts | 14 ++++++++------ skills/hr-pr-followup/test/check-pr-review.test.js | 14 ++++++++++++-- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/skills/hr-pr-followup/references/review-helpers.md b/skills/hr-pr-followup/references/review-helpers.md index 574844eb..49735590 100644 --- a/skills/hr-pr-followup/references/review-helpers.md +++ b/skills/hr-pr-followup/references/review-helpers.md @@ -21,15 +21,15 @@ 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". It does not check every CI result or authorize landing. -A check named "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 +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 SUCCESS. A NEUTRAL or skipped conclusion means cubic did not review the head ("Incremental review could not start", "Automatic AI review not started after branch rewrite", "AI review line limit reached"), so the gate refuses it too. That pull request stays blocked until someone comments `@cubic review` or the -limit clears, and the refusal says so. A head commit with no cubic check is not blocked, -because cubic has no config file to require. Right after a push, the gate cannot tell a -missing check from one that has not posted yet. +limit clears, and the refusal says so. A head commit with no cubic check is not blocked, because cubic has no config file +to require. `--wait` does not wait for a cubic check that has not posted yet, so right +after a push, confirm that cubic's check has appeared before landing. If a review is missing or incomplete, inspect the integration and the repository's review policy. The helper does not determine automatic-review enablement or diff --git a/skills/hr-pr-followup/scripts/check-pr-review.ts b/skills/hr-pr-followup/scripts/check-pr-review.ts index b6dcb8f5..4b80afb7 100644 --- a/skills/hr-pr-followup/scripts/check-pr-review.ts +++ b/skills/hr-pr-followup/scripts/check-pr-review.ts @@ -11,7 +11,8 @@ // any other conclusion, NEUTRAL and skipped included, because NEUTRAL means cubic did not review the // head ("Incremental review could not start", "AI review line limit reached"). That PR stays blocked // until someone comments `@cubic review` or the limit clears. A head commit with no cubic check is -// not blocked, since cubic has no config file to require. Missing or incomplete reviews +// not blocked, since cubic has no config file to require, 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. // 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 @@ -35,8 +36,9 @@ import { spawnSync } from "node:child_process"; import { parseArgs } from "node:util"; -const REVIEW_BOTS = /coderabbit|^cubic\b/i; -const CUBIC = /^cubic\b/i; +const CODERABBIT = /coderabbit/i; +const CUBIC = /^cubic · AI code reviewer$/i; +const isReviewBot = (name: string) => CODERABBIT.test(name) || CUBIC.test(name); const REVIEW_BOT_CONFIG = ".coderabbit.yaml"; const BODY_FINDINGS = /Outside diff range comments|Nitpick comments|Duplicate comments/; const REVIEWED = /^Review completed/; @@ -372,14 +374,14 @@ export function openFeedback(pr: ReviewState): string[] { const head = pr.commits.nodes[0]?.commit; const contexts = head?.statusCheckRollup?.contexts.nodes ?? []; const hasConfig = head?.tree.entries.some((e) => e.name === REVIEW_BOT_CONFIG) ?? false; - if (hasConfig && !contexts.some((ctx) => REVIEW_BOTS.test(ctx.context ?? ctx.name ?? ""))) { + if (hasConfig && !contexts.some((ctx) => CODERABBIT.test(ctx.context ?? ctx.name ?? ""))) { open.push(`${NO_CHECK}: ${FOLLOW_UP}`); } for (const ctx of contexts) { const name = ctx.context ?? ctx.name ?? ""; const running = ctx.state === "PENDING" || (ctx.status !== undefined && ctx.status !== "COMPLETED"); - if (!REVIEW_BOTS.test(name)) { + if (!isReviewBot(name)) { continue; } if (running) { @@ -391,7 +393,7 @@ export function openFeedback(pr: ReviewState): string[] { ); } else if ( ctx.context !== undefined && - (ctx.state !== "SUCCESS" || (!CUBIC.test(name) && !REVIEWED.test(ctx.description ?? ""))) + (ctx.state !== "SUCCESS" || (CODERABBIT.test(name) && !REVIEWED.test(ctx.description ?? ""))) ) { open.push( `${name} did not review the head commit ("${ctx.description ?? ""}"): ${FOLLOW_UP}`, 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 f3710919..71944782 100644 --- a/skills/hr-pr-followup/test/check-pr-review.test.js +++ b/skills/hr-pr-followup/test/check-pr-review.test.js @@ -151,8 +151,18 @@ 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([]); - const similar = { name: "bicubic-resize tests", status: "COMPLETED", conclusion: "FAILURE" }; - expect(openFeedback(pr({ contexts: [similar] }))).toEqual([]); + 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([]); + } +}); + +// cubic's check must not stand in for CodeRabbit's when the repository configures CodeRabbit. +test("a successful cubic check does not satisfy the .coderabbit.yaml presence check", () => { + const done = { name: CUBIC, status: "COMPLETED", conclusion: "SUCCESS" }; + expect(openFeedback(pr({ files: [".coderabbit.yaml"], contexts: [done] }))).toEqual([ + `no CodeRabbit check on the head commit: ${FOLLOW_UP}`, + ]); }); test("a cubic commit status needs success but no CodeRabbit description", () => {