Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion MANIFEST.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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)
Expand Down
112 changes: 86 additions & 26 deletions docs/pr-review-bots.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,47 @@ 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.
- *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:
**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 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
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 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.
Expand All @@ -43,21 +61,34 @@ 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
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
took exactly that, and its final push had to be dropped and re-landed separately
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 <n> --json headRefOid --jq .headRefOid) # the LIVE head, not local HEAD
Expand Down Expand Up @@ -89,15 +120,37 @@ 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**, 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
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).
Comment on lines 129 to +134

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


**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.
Expand All @@ -109,8 +162,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.
Expand All @@ -128,6 +181,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 —
Expand Down