Move CodeRabbit to opportunistic; disambiguate the CLAUDE.md rows - #19
Conversation
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`.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe PR clarifies which ChangesTemplate manifest clarification
PR review-bot routing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryUpdates review-bot documentation to route CodeRabbit opportunistically and clarifies the distinct roles and synchronization boundaries of the three CLAUDE.md artifacts. Confidence Score: 5/5The documentation-only PR appears safe to merge. The revised routing remains compatible with the command’s empty Wait-for behavior, preserves the separate merge-time per-head verification, and accurately distinguishes the CLAUDE.md artifacts. Important Files Changed
Reviews (1): Last reviewed commit: "Move CodeRabbit to opportunistic; disamb..." | Re-trigger Greptile |
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`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pr-review-bots.md`:
- Around line 129-134: Remove the `@coderabbitai review` re-trigger instruction
from the deterministic merge-verification procedure, including its associated
wait and expectation text. Keep the pagination, slurping, and `DISMISSED`
filtering guidance intact; only retain the retry as explicitly optional
historical diagnostics if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 553a0bc2-8586-4699-874a-29e1ddaad111
📒 Files selected for processing (2)
MANIFEST.mddocs/pr-review-bots.md
| `--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). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the ineffective re-trigger from merge verification.
The document says @coderabbitai review is a no-op while automatic reviews are active, but still instructs operators to retry it. That action cannot change the count and only adds delay; remove it from the deterministic verification path or label it as an optional historical diagnostic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/pr-review-bots.md` around lines 129 - 134, Remove the `@coderabbitai
review` re-trigger instruction from the deterministic merge-verification
procedure, including its associated wait and expectation text. Keep the
pagination, slurping, and `DISMISSED` filtering guidance intact; only retain the
retry as explicitly optional historical diagnostics if needed.
|
Per-head count, run as a diagnostic before merge — per the rule this PR writes. Reviewed. One CI: The one finding — 🟡 Minor, "Remove the ineffective re-trigger from merge verification." CodeRabbit's point: the file says Valid, and independently arrived at — this is the same seam flagged to Daniel when the qualification was written, where the choice was between removing the step and qualifying it in place. The current text does qualify it heavily ("expect nothing… kept only because it is cheap and has not been observed to hurt"), but the imperative "re-trigger once" still sits inside the procedure, which is what CodeRabbit is objecting to. It is a wording call, not a defect in what the procedure computes. Disposition: collected, not actioned. §5's mechanics are explicit — Blocker/Major must resolve, "Minor · Nit → collect, never iterate" — so it does not gate this merge. Worth a follow-up that decides the step's status outright rather than describing it twice. |
|
Tip For best results, initiate chat on the files or code changes.
The re-trigger wording is a valid Minor consistency improvement, but its collected/not-actioned disposition is consistent with the documented §5 policy. A follow-up can decide definitively whether to remove that step or make it explicitly optional outside the deterministic path. ✏️ Learnings added
|
Resolves the Minor collected on #19. The file established that `@coderabbitai review` is a no-op while automatic reviews are active, then instructed operators to run it anyway inside the deterministic merge verification — described twice, once imperatively and once undercut. The imperative is out; the procedure ends at the DISMISSED filter and the step survives once, as an explicitly optional note beside the human-exception clause it precedes, which also scopes it to Wait-for bots. Facts kept: the no-op behaviour with its #17 source, that #14 and #17 both produced nothing, that nothing claims a re-trigger cannot work (only that this account has never seen one work), and that `full review` must not be claimed effective without a run observed here. Pre-merge diagnostic: per-head count 0 on head 55c3d74 — which blocks nothing, Wait for being empty. Recorded on the PR: this 0 is a false negative. CodeRabbit reported "No actionable comments were generated in the recent review" over 286c55f..55c3d74, so the head was reviewed and simply had nothing to say; a zero-finding review posts no review record. The count conflates "not reviewed" with "reviewed, clean". Follow-up, not fixed here. Gate B: N/A — single explanatory documentation path per §5's prose rule. Battery green before commit.
1 participant
Docs-only. Two files, no plugin path touched, so no version bump.
1. CodeRabbit leaves Wait for
The grounds are the observed history, kept in the file:
1after ~3 min1Five consecutive unreviewed heads and then a genuine review is the whole argument. Blocking on CodeRabbit always terminated — its check goes green regardless — so the failure was never a hang; it was that the signal predicted nothing about delivery. A real findings source with unpredictable delivery and no completion signal you can block on is this file's own definition of the opportunistic category. Wait for is now empty; Greptile set the precedent.
Also added to its row, both first observed on #17:
@coderabbitai reviewis a no-op while automatic reviews are active — CodeRabbit's own words, "This command is applicable only when automatic reviews are paused". This retroactively explains Ship the §5 companion files, the deferred template sync, and 0.6.0 #14's "the re-trigger produced nothing", which the file had recorded as an unexplained observation.Plan note corrected: Free per Daniel. "Pro Plus" was observed on PR #1 only and no longer describes the account — which matters, because the review-limit behaviour is what a Free plan produces.
The per-head count rule is untouched. What changed is a statement of its scope: it is a merge-time check, and the blocking half of the completion-signal distinction now binds no bot. It still tells you whether a head was reviewed; it is no longer paired with a bot you wait on.
Sentences the move falsified, corrected in place — the §5 standing lens applied to a docs change:
One judgement call worth naming: the instruction "if no qualifying record exists, re-trigger once" sits inside the count rule I was told to leave alone, but I had just documented that the re-trigger is a no-op. Leaving it bare would have made the file contradict itself. I qualified it in place rather than removing it — the attempt is cheap and has not been observed to hurt — and flag it here as the one edit adjacent to the preserved rule.
Empty Wait for — checked, no plugin change needed
process-pr-review.mdStep 2 reads "Block only on the bots under Wait for", which is vacuous when the list is empty, then proceeds to "Then, without blocking, read whatever each Process opportunistically bot has posted at that moment." The timing anchor degrades from "when the gate clears" to "when the pass begins", which Step 1 (the CI check) already establishes. Nothing in the command requires the list to be non-empty. Verified by reading Steps 0–2 and grepping the plugin for other Wait-for assumptions — so this stays docs-only.2 & 3.
MANIFEST.md— three files, one bare nameThe row read
| CLAUDE.md | discipline rules §1–5 | /workflow-init template (project file) |. Every row in that table names a path undersource-files/, but read alone the bare name resolves to the repo root — which is exactly how CodeRabbit reached a false Major on #18, calling the root file "a reusable template" that must not hardcode a user's name.Row qualified to
source-files/CLAUDE.md, plus prose distinguishing all three: the frozen seed, the inline scaffold inworkflow-init.md(invariant 8 — the only copy a user's project ever receives, scoped §1–§5), and the root file as this repo's own instance.And the durable guard the #18 review asked for: root §6 (context canary) sits outside the §1–§5 template range by design and must never be synced into the scaffolded template. The section number is the mechanism — a sync copying §1–§5 leaves it behind by construction — and this sentence is the record of why, since nothing enforces the range. Placed in prose rather than in product files, per the request.
Gate B
N/A — every staged path is explanatory documentation (
docs/pr-review-bots.md,MANIFEST.md) per §5's prose rule. Battery green before commit: shellcheck (6 files), hook tests, invariant tests + check (123 assertions), version-bump tests + check (36 assertions),claude plugin validate . --strict.Summary by CodeRabbit
CLAUDE.mdfiles and protect local context from being included in generated templates.