From 1f411e00e82a2bbe187d16eba31f8644c9ae7df0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:43:07 +0200 Subject: [PATCH 1/2] Move CodeRabbit to opportunistic; disambiguate the CLAUDE.md rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/pr-review-bots.md — CodeRabbit leaves Wait for. Five consecutive heads went unreviewed (#12, #13, #15, #16, #17 — the last with zero review records on the PR at all), then #18 came back genuinely reviewed with three findings. That is a real findings source with unpredictable delivery and no completion signal you can block on, which is this file's own definition of the opportunistic category. Wait for is now empty; Greptile set the precedent. Two quirks added to its row: the status check goes green whether or not a review happened, and `@coderabbitai review` is a no-op while automatic reviews are active — CodeRabbit's own message on #17, which retroactively explains why #14's re-trigger produced nothing. Plan corrected to Free per Daniel; "Pro Plus" was observed on PR #1 only. The per-head count rule is unchanged. Its scope is now stated: it is a merge-time check, and the blocking half of the completion-signal distinction binds no bot, because nothing sits under Wait for. Sentences the move falsified are corrected in place (the "check you block on" framing, the "CodeRabbit-gated pass" anchor, the category rationale). process-pr-review needs no change for an empty Wait for list: Step 2 reads "block only on the bots under Wait for", vacuous when empty, then proceeds to the opportunistic reads, whose timing anchor is the start of the pass that Step 1 already establishes. Verified, not assumed — no plugin path touched, so no version bump. MANIFEST.md — the bare `CLAUDE.md` row named a path under source-files/ but read alone resolves to the repo root, which is how CodeRabbit reached a false Major on #18 calling the root file a reusable template. Row qualified, and the three distinct files distinguished: the frozen seed, the inline scaffold in workflow-init.md (invariant 8, §1–§5), and the root file as this repo's own instance. Records that root §6 (context canary) sits outside the template range by design and must never be synced into it — the durable guard the #18 review asked for, in prose rather than in product files. Gate B: N/A — every staged path is explanatory documentation (docs/pr-review-bots.md, MANIFEST.md) per §5's prose rule. Battery green: shellcheck (6 files), hook tests, invariant tests + check (123 assertions), version-bump tests + check (36 assertions), `claude plugin validate . --strict`. --- MANIFEST.md | 24 ++++++++++++- docs/pr-review-bots.md | 77 +++++++++++++++++++++++++++++++----------- 2 files changed, 80 insertions(+), 21 deletions(-) diff --git a/MANIFEST.md b/MANIFEST.md index 8a45929..f9d1aa9 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -11,7 +11,7 @@ archive is the complete extraction base. | commands/process-pr-review.md | PR bot processor | plugin (make bot names configurable) | | hooks/codex-gate.sh + .test.sh | gate counter/reminder | plugin (loaded by convention from `hooks/hooks.json` — never declared in the manifest) | | claude-settings.json | shows HOW the hook is wired up in a *project* (settings.json), which the plugin replaces with convention loading | reference only | -| CLAUDE.md | discipline rules §1–5 | /workflow-init template (project file) | +| `source-files/CLAUDE.md` | discipline rules §1–5 | seed for the /workflow-init template — **not** the root `CLAUDE.md`, see below | | prompt-standards.md | 11-criteria checklist | template + the plugin repo's own standard | | hardening-log.md | FORMAT reference ONLY (sanitized: real findings removed) | template: empty ledger with header/convention | | coding-workflow.md | neutral overall documentation | basis for the plugin README | @@ -22,6 +22,28 @@ archive is the complete extraction base. | codex-config.toml, .mcp.json | reviewer pin + MCP wiring | templates | | eslint-rules/ | custom rules | example ONLY (Convex-specific) — document, don't generalize | +**Three different `CLAUDE.md` files, and the bare name above once cost a review.** Every +row in this table names a path under `source-files/`, but read alone the unqualified +`CLAUDE.md` resolves to the repo root — a PR bot did exactly that on #18 and raised a +Major saying the root file was "a reusable template" that must not hardcode a user's +name. It is not. The three: + +- **`source-files/CLAUDE.md`** — the frozen extraction seed this row describes. Never + edited (see the header above). +- **The inline copy inside `plugins/dev-workflow/commands/workflow-init.md`** — the + *operative* scaffold, the only one a user's project ever receives. Invariant 8 keeps it + inline in the command body, so nothing reads a template off disk. It covers **§1–§5**, + and `/workflow-init` §2.1 scaffolds it by that name. +- **The repo-root `CLAUDE.md`** — this project's own instance of the rules, governing + work in this repo. It is not a template and is not read by `/workflow-init`. + +**Root `CLAUDE.md` additionally carries a repo-local §6 (context canary) that sits +deliberately outside the §1–§5 template range and must never be synced into the +scaffolded template.** The canary names one user; propagating it would address every +initialized project's user by that name. The section number is the guard — a sync that +copies §1–§5 leaves it behind by construction — and this sentence is the durable record +of why, since nothing mechanical enforces the range. + ## Not included (deliberately) - AGENTS.md (the source project's invariants — written fresh per project; /workflow-init walks you through it) - Ledger CONTENTS, baselines, todo contents (project state) diff --git a/docs/pr-review-bots.md b/docs/pr-review-bots.md index d6439ca..86c1e10 100644 --- a/docs/pr-review-bots.md +++ b/docs/pr-review-bots.md @@ -10,29 +10,44 @@ uses hangs the loop, and treating a channel as context silently drops real findi | Bot | Enabled | Where findings appear | Notes (plan/tier limits, completion signal, quirks) | |---|---|---|---| -| CodeRabbit | yes | **inline** | Observed on PR #1 (plan: Pro Plus, profile CHILL): posts real inline review comments on the diff, each with severity and a committable suggestion, plus a walkthrough summary comment. Read the inline comments — the walkthrough is not a findings source. **Its status check passes while the comment says "Review rate limited" — observed on four PRs (#12, #13, #15, #16), so treat it as this bot's normal behaviour rather than an edge case. A green check does not prove the final head was reviewed; the review count is the arbiter. See the completion-signal note.** | +| CodeRabbit | yes | **inline** | Posts real inline review comments on the diff, each with severity and a committable suggestion, plus a walkthrough summary comment. Read the inline comments — the walkthrough is not a findings source. Observed on #1 and #18 (profile CHILL). **Plan: Free** (per Daniel); the "Pro Plus" recorded here earlier was observed on PR #1 only and no longer describes the account — the review-limit behaviour below is what a Free plan produces. **Routed opportunistically since #18** — five consecutive unreviewed heads (#12, #13, #15, #16, #17), then a genuine review. Two quirks that look like signals and are not: **its status check goes green whether or not a review happened**, so a green check never proves the final head was reviewed (the per-head count is the arbiter); and **`@coderabbitai review` is a no-op while automatic reviews are active** — the bot says so itself ("This command is applicable only when automatic reviews are paused", #17), which is why #14's re-trigger produced nothing. | | Greptile | yes | **summary always; inline usually** | Four PRs observed (#1, #2, #4, #5): a PR-level **summary comment every time**, with findings sometimes only inside it under "Comments Outside Diff". **Inline** comments on #2 (1), #4 (2), #5 (1) but **none on #1** — so inline is usual, not guaranteed. Read both channels; the summary is the one that has never been missing. **Completion signal: none you can block on.** `gh pr checks` displayed a "Greptile Review" entry for #4 and #5, but the check-runs and statuses APIs return no Greptile entry for any of those heads — the two tools disagree, so neither proves it has finished. Posts within ~4–11 min. | | Cursor Bugbot | no | n/a | Comments only to say it is disabled for this account. Ignore. | **Routing — these lists are authoritative.** -- **Wait for (block on it):** CodeRabbit. It has a status check, so `gh pr checks` going - non-pending is proof the *check* finished — which is what you block on. That is **not** - the same as proof the final head was reviewed; verify that separately before merging - (below). -- **Process opportunistically (never block):** Greptile. Read whatever it has posted - when the CodeRabbit-gated pass begins, in both channels. If it posts later, handle it +- **Wait for (block on it):** *none.* CodeRabbit sat here until #18 and no longer does — + grounds below. +- **Process opportunistically (never block):** CodeRabbit and Greptile. Read whatever + each has posted when the pass begins, in both channels. If one posts later, handle it as a follow-up. - **Ignore:** Cursor Bugbot — disabled for this account, and it says so itself. -**Completion signal, per bot.** CodeRabbit posts a status, so `gh pr checks` shows it and -you can block on it. **Two different things, and conflating them merges unreviewed heads.** - -- *The check stopped pending* — the blocking signal. Block on this. +**Why CodeRabbit moved.** It has a status check, so blocking on it always *terminated* — +what it stopped doing was delivering. Five consecutive heads went unreviewed (#12, #13, +#15, #16, #17), the last with **zero review records on the PR at all**; then #18 came +back genuinely reviewed with three findings. That is the opportunistic category exactly +as this file defines it: a real findings source whose delivery is unpredictable and whose +completion signal proves nothing about whether a review happened. Greptile set the +precedent for a different reason — no signal at all — and CodeRabbit arrives at the same +place by a signal that exists and does not mean what it appears to mean. + +**What the routing change does *not* touch: the per-head count.** The verification below +is unchanged and stays exactly as #17 left it. Note only what its scope now is: it is a +**merge-time** check, and the *blocking* half of the completion-signal distinction below +now binds no bot, because nothing sits under **Wait for**. The count is still how you +learn whether a given head was reviewed; it is no longer paired with a bot you wait on. + +**Completion signal, per bot.** CodeRabbit posts a status — visible in `gh pr checks`, +and it finishes whether or not a review happened. **Two different things, and conflating +them merges unreviewed heads.** + +- *The check stopped pending* — a blocking signal, and the reason CodeRabbit looked + waitable. Nothing is blocked on it now. - *The final head was reviewed* — a separate verification, and the one that decides whether you may merge. **This is settled behaviour, not a hazard that might occur: - never merge on the check alone — the review count is the arbiter.** Four occurrences, - the last two caught by running the count rather than by luck: + never merge on the check alone — the review count is the arbiter.** Five occurrences, + the last three caught by running the count rather than by luck: - **#12 and #13** — the check passed while the issue comment read "Review rate limited"; on #13 the only CodeRabbit **review record** carried `commit_id` `eed589c` while the merged head was `92de0d2`. The head that merged was never reviewed. @@ -43,6 +58,16 @@ you can block on it. **Two different things, and conflating them merges unreview head `c6c1850` the check was green, the comment read "Review rate limited", and the count was `0` — unreviewed. Two heads later the comment read "Review rate limited" again while the count was `1` — reviewed. + - **#17** — the fifth, and the one that moved CodeRabbit out of **Wait for**: green + check, count `0` before and after a re-trigger, and **zero CodeRabbit review records + on the PR for any head**. Its first comment was not "Review rate limited" but *"Review + limit reached — you've reached your PR review limit, so we couldn't start this + review"*; the re-trigger then answered *"Review finished… does not re-review already + reviewed commits"* and produced nothing, so a review that never started was booked as + done. Merged as a recorded human exception (#14 precedent). + - **#18** — reviewed, with three findings on the live head and a count of `1`. Five + unreviewed heads then a genuine review is the whole argument for the move: the bot + delivers real findings, on no schedule you can predict or wait on. **The message is noise. The count is signal. In both directions.** "Review rate limited" appears on heads that were never reviewed and on heads that were, so it tells @@ -89,15 +114,20 @@ the check said `pass` while the comment said "Review rate limited", and this que returned `0` for head `787dd9a` **before** the merge: the re-trigger produced nothing, and the PR merged on an explicit human decision with the exception recorded, the unreviewed delta being a one-word prose correction the reviewer had itself requested. That is the -intended shape — the check is a signal you block on, this query is what tells you whether -a review actually happened, and when they disagree a human decides. +intended shape — this query is what tells you whether a review actually happened, and +when it disagrees with the check a human decides. (Written while CodeRabbit was still +under **Wait for**, so it read "the check is a signal you block on"; nothing is blocked +on now, and the rest of the shape is unchanged.) **#17 explains the "re-trigger produced +nothing" here:** `@coderabbitai review` is a no-op while automatic reviews are active. `--paginate` matters: without it only page one is read, so a qualifying review can sit on page two and be read as absent. `jq -s` is what slurps the pages — `gh api --slurp` cannot do it here, being rejected outright when combined with `--jq`. `DISMISSED` is excluded — a dismissed review is not -a review of that head. If no qualifying record exists, re-trigger once; if it is still -absent, **merge only on an explicit human decision**, recording that the head went -unreviewed. +a review of that head. If no qualifying record exists, re-trigger once (expect nothing — +`@coderabbitai review` is a no-op while automatic reviews are active, so the attempt +costs a wait and is kept only because it is cheap and has not been observed to hurt); if +it is still absent, **merge only on an explicit human decision**, recording that the head +went unreviewed. What was actually measured, stated exactly: the rate-limit warning appeared in the **issue comment**, while the review record was an earlier completed review of an earlier commit. @@ -109,8 +139,8 @@ body filter is bounded defensive filtering, not a check against something seen. "Greptile Review" entry for #4 and #5, while the check-runs and statuses APIs return no Greptile entry for any observed head. Two tools, two answers, so neither is proof. -So: **process Greptile opportunistically, never block on it.** Do the CodeRabbit-gated -pass, and read whatever Greptile has posted at that moment via +So: **process Greptile opportunistically, never block on it.** Begin the pass — nothing +gates it now — and read whatever Greptile has posted at that moment via `gh pr view --json comments,reviews` plus `gh api .../pulls/N/comments`. If it posts later, process it as a follow-up. Waiting on it risks hanging forever; ignoring it drops real findings, since every observed PR carried some. @@ -128,6 +158,13 @@ findings source with no signal that says it has finished. Blocking on such a bot the loop; dropping it loses findings. Reading what is there and revisiting later is the only option that does neither. +CodeRabbit widened the category rather than fitting the original shape. Greptile has *no* +completion signal; CodeRabbit has one that always fires and carries no information about +whether a review happened. The category turns out to be about **whether a signal predicts +delivery**, not about whether a signal exists — a distinction only visible once a bot +supplied the second case. Both belong here for the same practical reason: what they post +is worth reading, and when they post is not something you can wait on. + A bot belongs under **Wait for** only once it has been *seen* producing findings here. Listing an unconfirmed bot there is the failure this file exists to prevent — From 2634bf219e196ba96e5a2e659a2d83538579505e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20S=C3=A4nger?= <20968534+dsnger@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:00:51 +0200 Subject: [PATCH 2/2] Scope the merge-exception clause to Wait-for bots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The count rule carried two facts fused into one sentence. They separate: 1. The per-head count is the arbiter of whether a head was reviewed — unchanged, and the diagnostic for any bot. Routing a bot opportunistically does not make its review record less readable or the answer less true. 2. "Merge only on an explicit recorded human decision" binds only bots under Wait for. An absent review from an opportunistic bot blocks nothing and needs no exception: every head reaching a PR has already passed Gate B, the workflow's actual cross-model review, and the PR bots are a supplementary third instance. Requiring a recorded exception whenever a supplementary reviewer stayed quiet would reimpose as ceremony the blocking that opportunistic routing removed. The clause is dormant, not retired — Wait for is empty, so nothing triggers it; promote a bot back and it reactivates unchanged for that bot. The #12–#18 history and the #14 precedent stay as the record of when it was live: a real requirement under the routing of its time. Four sentences elsewhere attached a merge consequence to a `0` and are corrected to state the routing dependency: the scope paragraph above the completion-signal list, the "decides whether you may merge" bullet, "`0` means do not merge", and the "run it on every merge" lead-in to the query. Gate B: N/A — the only staged path is explanatory documentation (docs/pr-review-bots.md) per §5's prose rule. Battery green: shellcheck (6 files), hook tests, invariant tests + check (123 assertions), version-bump tests + check (36 assertions), `claude plugin validate . --strict`. --- docs/pr-review-bots.md | 59 +++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 18 deletions(-) diff --git a/docs/pr-review-bots.md b/docs/pr-review-bots.md index 86c1e10..38c6c89 100644 --- a/docs/pr-review-bots.md +++ b/docs/pr-review-bots.md @@ -32,11 +32,12 @@ completion signal proves nothing about whether a review happened. Greptile set t precedent for a different reason — no signal at all — and CodeRabbit arrives at the same place by a signal that exists and does not mean what it appears to mean. -**What the routing change does *not* touch: the per-head count.** The verification below -is unchanged and stays exactly as #17 left it. Note only what its scope now is: it is a -**merge-time** check, and the *blocking* half of the completion-signal distinction below -now binds no bot, because nothing sits under **Wait for**. The count is still how you -learn whether a given head was reviewed; it is no longer paired with a bot you wait on. +**What the routing change does *not* touch: the count itself.** The query below and what +it measures are exactly as #17 left them, and the count remains the arbiter of whether a +head was reviewed — for **any** bot, opportunistic or not. What the change does scope is +the *consequence* the file used to attach to a `0`: the merge-exception clause now binds +only bots under **Wait for**, which is currently none. Both facts are stated where the +query lives, below. **Completion signal, per bot.** CodeRabbit posts a status — visible in `gh pr checks`, and it finishes whether or not a review happened. **Two different things, and conflating @@ -44,10 +45,12 @@ them merges unreviewed heads.** - *The check stopped pending* — a blocking signal, and the reason CodeRabbit looked waitable. Nothing is blocked on it now. -- *The final head was reviewed* — a separate verification, and the one that decides - whether you may merge. **This is settled behaviour, not a hazard that might occur: - never merge on the check alone — the review count is the arbiter.** Five occurrences, - the last three caught by running the count rather than by luck: +- *The final head was reviewed* — a separate verification, and the one that told you + whether you may merge while a bot was under **Wait for**. **This is settled behaviour, + not a hazard that might occur: a green check never establishes that a head was reviewed + — the review count is the arbiter.** (What follows from a `0` depends on routing; see + the two facts beside the query below.) Five occurrences, the last three caught by + running the count rather than by luck: - **#12 and #13** — the check passed while the issue comment read "Review rate limited"; on #13 the only CodeRabbit **review record** carried `commit_id` `eed589c` while the merged head was `92de0d2`. The head that merged was never reviewed. @@ -72,9 +75,11 @@ them merges unreviewed heads.** **The message is noise. The count is signal. In both directions.** "Review rate limited" appears on heads that were never reviewed and on heads that were, so it tells you nothing either way; there is no interpretation left to do, and nothing to weigh — - one integer per head decides it. `0` means do not merge. Anything else means the head - was reviewed. What the green tick establishes is that CodeRabbit's *check* finished, - which is a different fact about a different thing. + one integer per head decides it. `0` means the head was not reviewed; anything else + means it was. Whether a `0` may be merged past is a routing question, answered below — + under **Wait for** it may not be, without a recorded human decision. What the green + tick establishes is that CodeRabbit's *check* finished, which is a different fact about + a different thing. **Verification is per head, not per PR.** Every push moves the head and the previous answer expires with it; a PR that takes three pushes takes three verifications. #16 @@ -82,7 +87,8 @@ them merges unreviewed heads.** because the new head went unreviewed past the point of waiting. Verify the second before merging — a deterministic boolean, so it can gate rather than be -eyeballed. Run it on every merge, including the ones where the check looks unambiguous: +eyeballed. Run it on every merge, including the ones where the check looks unambiguous; it +gates directly only for a bot under **Wait for**, and is diagnostic for the rest: ```sh head=$(gh pr view --json headRefOid --jq .headRefOid) # the LIVE head, not local HEAD @@ -116,8 +122,8 @@ the PR merged on an explicit human decision with the exception recorded, the unr delta being a one-word prose correction the reviewer had itself requested. That is the intended shape — this query is what tells you whether a review actually happened, and when it disagrees with the check a human decides. (Written while CodeRabbit was still -under **Wait for**, so it read "the check is a signal you block on"; nothing is blocked -on now, and the rest of the shape is unchanged.) **#17 explains the "re-trigger produced +under **Wait for**, when that recorded decision was required; the query still answers the +same question, but an absent review from an opportunistic bot no longer needs one.) **#17 explains the "re-trigger produced nothing" here:** `@coderabbitai review` is a no-op while automatic reviews are active. `--paginate` matters: without it only page one is read, so a qualifying review can sit on @@ -125,9 +131,26 @@ page two and be read as absent. `jq -s` is what slurps the pages — `gh api --s do it here, being rejected outright when combined with `--jq`. `DISMISSED` is excluded — a dismissed review is not a review of that head. If no qualifying record exists, re-trigger once (expect nothing — `@coderabbitai review` is a no-op while automatic reviews are active, so the attempt -costs a wait and is kept only because it is cheap and has not been observed to hurt); if -it is still absent, **merge only on an explicit human decision**, recording that the head -went unreviewed. +costs a wait and is kept only because it is cheap and has not been observed to hurt). + +**Then the two facts separate, and only one of them still binds.** + +1. **The count is the arbiter of whether a head was reviewed.** Unchanged, and it applies + to **any** bot — it is the diagnostic, and being routed opportunistically does not + make a bot's review record less readable or the answer less true. +2. **"Merge only on an explicit recorded human decision" applies only to bots under + **Wait for**.** An absent review from an *opportunistic* bot blocks nothing and needs + no exception. Every head reaching a PR has already passed **Gate B**, the workflow's + actual cross-model review; the PR bots are a supplementary third instance, and + requiring a recorded exception whenever a supplementary reviewer stayed quiet would + reimpose as ceremony exactly the blocking that routing the bot opportunistically + removed. + +**The clause is dormant.** **Wait for** is empty, so nothing currently triggers it. It is +kept, not retired: promote a bot back and it reactivates unchanged, for that bot. The +#12–#18 history above and the #14 precedent are the record of the period when it was +live — they document a real requirement under the routing of their time, not a +requirement suspended in the abstract. What was actually measured, stated exactly: the rate-limit warning appeared in the **issue comment**, while the review record was an earlier completed review of an earlier commit.