Choose which bots review which project - #57
Conversation
The reviewers were a property of the process: one CRQ_COBOTS for every repository the fleet touches. A repo that only wants Codex, or one where Bugbot should gate, had no way to say so — and the answer to "which bots do you want here" was to edit a global env file and restart a daemon. crq reviewers <repo> now reports who runs there and what each one costs; crq reviewers set chooses them; crq reviewers clear goes back to the fleet default. Each reviewer reports its budget, which is the only property the queue cares about: account is serialized against the shared CodeRabbit allowance, none runs immediately and waits for nobody. The configuration lives in the shared state ref rather than a .crq.yaml the repository carries. The daemon has no checkout of the repositories it reviews, so an in-repo file would be invisible to it or cost a REST fetch per PR — and a daemon and an agent reading different configurations while writing one state ref is a class of bug worth not having. The override reaches the derived views, not just the stored list: after applying it, Reviewers, RequiredBots and FeedbackBots are rebuilt exactly as LoadConfig builds them, so crq cannot gate on or surface findings from a bot the repository excluded. "Chosen to be none" is kept distinct from "not chosen". The primary stays fleet-wide. Its markers and command are compiled into the dialect classifiers when the Service is built, so a per-repo primary means per-repo classifiers.
Eight review findings, two of them P1. Choices resolved against the fleet's enabled list, so the feature could only ever subtract: with CRQ_COBOTS=codex, asking a project for bugbot was rejected as unknown though it is a registered reviewer. They resolve against the registry now, and a bot the fleet does not enable is built from its registry entry. Naming a bot in --required enables it too, the rule fleet parsing already follows — one that gates but is never triggered waits forever. A primary that is itself a registry bot keeps its silenced entry through an override. That entry carries its wording and check-run hooks, so dropping it costs the PRIMARY its evidence and the round waits out a clean result crq can no longer read. Same defect as the fleet path had, one layer up. A second call naming one half no longer wipes the other: the existing override is loaded and merged, which is what the nil-versus-empty contract promised. An empty --required is refused, because gating on nobody makes Feedback converge before any chosen reviewer runs, and clear takes no mutation flags — ignoring them turned a malformed call into a silent wipe. The Codex-only degrade is gated on Codex being a reviewer here, or an unsolicited auto-review would release the head during an account block on behalf of a bot the project excluded. And sharing a ref only stops an old binary ERASING the field; it does not make it act on one. State now records which hosts write it and what they understand, and `crq reviewers` names any host driving the queue that predates overrides, instead of implying agreement.
Nine review findings, four of them P1, and they converge on one mistake: the override changed who has to answer without changing who gets asked. `--required bugbot` with no `--bots` set the required half only, so nothing enabled Bugbot and it became an unknown reviewer with no command. Required now implies enabled whichever half was named, the rule fleet parsing already follows. A retained co-reviewer that becomes required is promoted out of a never trigger for the same reason — Codex's fleet entry is never while it is optional, so keeping that entry and flipping only the Required bit left the engine waiting for evidence no command was posted for. Only a never trigger is promoted, so a deliberate selfheal survives. Autoreview decided from the primary alone, so a repo requiring Codex whose head CodeRabbit had already reviewed was never enqueued and Codex was never asked. It evaluates the repository's own required set now. And changing the requirements on a head with a completed round stranded the PR outright: convergence reported the new reviewer pending while enqueue kept skipping the head, because a completed round IS the dedup marker. Those rounds are reopened when the required set actually changes. Three more. The two halves are merged inside the CAS closure, or two hosts setting different halves both derive from one snapshot and the later write drops the other's. An explicit CRQ_FEEDBACK_BOTS survives an override, since it is the one list an operator may widen beyond who reviews. And the lagging-host check now matches the leader's "host=x pid=n" form against the host key capabilities are recorded under — otherwise every current-version daemon read as needing an upgrade. The docs no longer call the Codex example Codex-only: an override chooses the co-reviewers, so the primary is still triggered and still spends quota. Leaving it out of --required means the round does not wait for it.
Three of four findings; the fourth is answered rather than fixed. `crq reviewers owner/repo --bots codex` is a set command missing its verb. It showed the configuration and exited 0, which tells automation the mutation worked when nothing changed. It is refused now, naming the verb. `crq reviewers clear owner-repo` cleared a key nothing uses and exited 0, so a typo left the real override in force while the caller believed it had restored the fleet default. Clear validates the shape set already validates. On the fourth: reopening a completed round can re-ask a primary that finished with a completion reply and no Review object, because DecideFire's already-reviewed gate reads submitted reviews. Marking the round co-only does not help — that flag records what a fire DID, it does not gate one — and the alternatives all need an observation the CAS closure cannot make. So the trade is stated in the code: one duplicate review in an uncommon case, against a PR stranded silently and permanently. The loud failure is the better one, and the gate is where a real fix belongs.
Four P1s, and the first retires a trade-off I had argued for instead of fixing. Reopening a completed round could re-ask a primary that answered with a completion reply and no Review object, because the already-reviewed gate read only obs.Reviews. The reviewer's point was that the gate is where the fix belongs, and they were right: it now also counts a completion reply paired to that round's own command. Reopening no longer risks a duplicate review, so the round can be requeued when the required set changes without the cost I had written into a comment. A repository enabling a bot the fleet disabled was built from registry defaults, silently discarding the operator's CRQ_COBOT_<NAME>_CMD, TRIGGER and GRACE — a different bot than the one configured. Every registry co-reviewer is now resolved with the environment applied, enabled or not, and an override picks from that. Capabilities are keyed per PROCESS rather than per host. A new CLI and an old daemon on one machine is the ordinary upgrade, and a per-host key let the CLI's write vouch for the daemon that had not been upgraded — hiding exactly the case the signal exists to catch. And applyFire revalidates the override before acting. Deciding and writing are two steps, so an operator removing a co-reviewer between them would otherwise have crq post that bot's trigger on the authority of a configuration that no longer exists.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds repository-scoped reviewer overrides, persisted writer capability tracking, per-repository queue configuration, new ChangesRepository reviewer configuration
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant Service
participant State
participant Queue
Operator->>CLI: crq reviewers set repo --bots codex --required codex
CLI->>Service: SetReviewers(repo, bots, required)
Service->>State: Store repository override
Service->>Queue: Reopen affected rounds
Queue->>Service: Process repository round
Service->>State: Load effective repository configuration
Service-->>Queue: Apply repository-specific reviewer policy
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37d0bc2dcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@cmd/crq/main.go`:
- Around line 530-533: Update the reviewer budget description in cmd/crq/main.go
at lines 530-533 to remove the claim that budget "none" waits for nobody, while
retaining its meaning as no account serialization. Update
skills/coderabbit-queue/SKILL.md at lines 182-183 to state that --required
independently controls whether a reviewer gates convergence.
- Around line 535-537: Update the CLI help text describing --bots and --required
so the empty-value behavior applies only to --bots, while documenting that
--required rejects an empty reviewer list. Preserve the distinction between
selecting co-reviewers and choosing convergence-gating reviewers.
- Around line 365-368: The reviewer-setting documentation is inconsistent with
the handler’s partial-update contract because it requires --bots even when only
--required is supplied. Update cmd/crq/main.go lines 365-368, cmd/crq/main.go
lines 524-526, and README.md lines 388-390 to show --bots and --required as
independently optional while preserving the existing reviewers set behavior.
- Around line 774-777: Update the reviewers show-path validation around the repo
check to require the same valid owner/name repository shape enforced by
SetReviewers and ClearReviewers, rejecting malformed targets such as “show”
before calling Service.Reviewers. Preserve the existing usage error and return
behavior for invalid repositories.
In `@internal/crq/feedback.go`:
- Around line 91-92: Update the configured-bot checks in the feedback handling
flow at the remaining call sites around lines 192, 366, and 410 to invoke
isConfiguredBot on the repo-derived cfg rather than the fleet-level Config.
Preserve the existing login values and surrounding behavior while ensuring repo
override co-reviewers are recognized.
In `@internal/crq/repoconfig.go`:
- Around line 142-162: Move the CAS write closures used by SetReviewers and
ClearReviewers out of repoconfig.go into service.go, exposing Service methods
for repoconfig.go to call after validation. Ensure those apply-phase methods
check cfg.DryRun and perform no store.Update or other writes in dry-run mode,
while preserving the existing reviewer updates, timestamps, reopening behavior,
sync, and return results.
- Around line 143-156: Update SetReviewers around the store.Update closure to
detect when both coBots and required are nil and return ErrNoChange before
mutating the override or invoking st.SetRepoOverride, reopenForChangedReviewers,
or related syncs. Reuse the existing no-op guard pattern and preserve normal
updates when either reviewer setting is provided.
In `@internal/crq/service.go`:
- Around line 670-694: Scope the staleness guard in applyFire to only verdicts
that can post configuration-dependent actions: FireCoOnly, FireCoDeferred,
FireAdopt when PostCo is set, and FirePost when PostCo is set. Avoid calling
s.store.Load for FireNo and the other verdicts, while preserving the existing
overrideChanged check and handler behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ef4c99ab-c722-4f32-b218-0fd636bd2fbf
📒 Files selected for processing (19)
README.mdcmd/crq/main.gointernal/crq/auto.gointernal/crq/codex_replay_test.gointernal/crq/config.gointernal/crq/feedback.gointernal/crq/repoconfig.gointernal/crq/repoconfig_test.gointernal/crq/reviewers.gointernal/crq/reviewers_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/engine/fire.gointernal/state/state.gointernal/state/store.gointernal/state/writers_test.gollms.txtskills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep
internal/crqlimited to orchestration and effects wiring;service.gois the only effects executor for CAS writes andPostIssueComment, andDryRunmust write nothing.
Files:
internal/crq/state.gointernal/crq/codex_replay_test.gointernal/crq/reviewers_test.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/crq/reviewers.gointernal/crq/repoconfig.gointernal/crq/config.gointernal/crq/service.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Persist schema v3 with oneRoundper PR, one globalFireSlot,AccountQuota, and anArchive; never delete rounds, and preserve unknown JSON members during round trips.
Model round lifecycle transitions asRoundmethods, reject illegal edges, and never delete a round; completed rounds remain deduplication markers, while rate-limited retries useawaiting_retrywithout deleting fired history.
Files:
internal/state/writers_test.gointernal/state/store.gointernal/state/state.go
internal/crq/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add a default entry in
parseCoBotswhen a new co-reviewer should be enabled by default.
Files:
internal/crq/config.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/engine/**/*.go: Keepinternal/enginepure: decision logic must receivenow, perform no I/O, context, or GitHub access, and table-test every rule.
DecideFiremust be the single fire owner and evaluate guards in order: open, readable/current head, eligible phase, primary review unavailable, already reviewed, free slot, account quota, minimum interval, then adopt/post.
Keep convergence, fire, and co-reviewer rules ininternal/engine; keep orchestration ininternal/crq, and table-test engine rules.
Files:
internal/engine/fire.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
The apply phase in
service.gois the only place allowed to execute effects: CAS state writes andPostIssueComment;DryRunmust report without writing.
Files:
internal/crq/service.go
🪛 LanguageTool
skills/coderabbit-queue/SKILL.md
[style] ~187-~187: To strengthen your wording, consider replacing the phrasal verb “leave out”.
Context: ... override chooses the co-reviewers. Leaving the primary out of --required means the round does not w...
(OMIT_EXCLUDE)
🔇 Additional comments (17)
cmd/crq/main.go (2)
214-219: LGTM!
814-827: LGTM!llms.txt (1)
205-220: LGTM!internal/state/state.go (1)
272-316: LGTM!Also applies to: 318-428, 503-526
internal/state/store.go (1)
46-48: LGTM!Also applies to: 215-215
internal/crq/config.go (1)
47-62: LGTM!Also applies to: 199-211, 347-384, 418-447, 535-542
internal/crq/state.go (1)
26-27: LGTM!Also applies to: 38-40, 57-57
internal/state/writers_test.go (1)
1-67: LGTM!internal/crq/reviewers.go (1)
178-262: LGTM!Also applies to: 264-281, 283-296, 298-308, 310-326, 328-340
internal/crq/service_test.go (1)
2364-2364: LGTM!Also applies to: 2385-2385, 2398-2398
internal/crq/repoconfig_test.go (1)
1-241: LGTM!internal/crq/reviewers_test.go (1)
228-278: LGTM!internal/crq/auto.go (1)
296-362: LGTM!internal/crq/feedback.go (1)
127-133: LGTM!Also applies to: 159-160, 170-170, 449-453, 464-464
internal/crq/service.go (1)
301-308: LGTM!Also applies to: 348-375, 403-412, 492-498, 632-641, 800-934, 985-1024, 1137-1165, 1268-1272, 1298-1302, 1336-1417, 1432-1470
internal/engine/fire.go (1)
160-168: 🎯 Functional CorrectnessCorrectly extends "already reviewed" detection to completion replies.
Sound addition and it preserves the required guard order (primary-unavailable → already-reviewed → free-slot → account-quota → min-interval).
Please confirm
fire_test.go(not included in this batch) has a table-test case for this new completion-reply branch, per the engine guideline requiring every rule to be table-tested.As per coding guidelines, "table-test every rule."
Source: Coding guidelines
internal/crq/codex_replay_test.go (1)
516-518: LGTM!
Ten findings across both reviewers; two are answered rather than fixed.
Changing a repository's required set requeued every completed round it
had. Rounds are never deleted, so a repository's merged and closed PRs
stay behind as completed dedup markers: on a busy repo that hands Pump
hundreds of dead rounds to observe and drop, one per tick, ahead of every
real one. A stranded PR is by definition an open one, so the requeue asks
GitHub which PRs are open first and touches only those.
Reopening also stamped LastAttemptAt, which is the adoption floor for a
FAILED attempt. Raising it discarded a newly required co-reviewer's own
unanswered trigger as too old to adopt, so crq posted that bot a second
request for the very round the reopen exists to let it answer.
The reopened round's already-reviewed gate read the ADOPTION helper,
which deliberately omits Completion's prior-review requirement and its
failed-summary guard — it answers "was this command spoken for", a
weaker question. A completion reply nothing backs would have written a
completed marker that every later same-head check skips, forever.
Capabilities are keyed per process, but a round recorded its bare
hostname, which no writer entry can match. Every `crq reviewers` call
during a fire therefore named the current process in lagging_hosts and
told the operator to upgrade a binary that already understands
overrides. The round now records the writer id; the dashboard still
prints the machine name.
applyFire re-read the state on EVERY verdict to revalidate the override,
including the FireNo no-op that is most of what a pump does — an extra
four API calls per tick against the git-backed store. Only the verdicts
that post a configuration-chosen command pay for it now.
Setting reviewers with neither half named wrote a fresh UpdatedAt
anyway, and that timestamp IS the override's identity, so a no-op call
discarded every in-flight fire decision for the repository. Reading a
repository now applies the shape check set and clear apply, so
`crq reviewers show` fails instead of reporting the fleet default for a
repository nobody has.
The help conflated budget with requiredness ("none ... waits for
nobody"), required --bots for an update the handler accepts with
--required alone, and offered an empty value for a flag that refuses
one.
Not fixed: feedback.go reading isConfiguredBot from the fleet config
rather than the repo's is identical by construction — the primary is not
overridable, so ForRepo never touches Config.Bot. And SetReviewers'
store.Update is an operator's explicit mutation, not the apply phase of
a review decision: Enqueue and Cancel write outside it too, and DryRun
means "request no reviews", not "silently ignore a configuration
command".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e09bfae29f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A reviewer change requeues a repository's completed rounds, but three paths let one slip through and leave the PR waiting on a bot crq never asks. A closed PR's round was skipped, and closed is not final: reopened at the same head, its completed round is the dedup marker that hides the requirement added while it was shut. Mark those rounds instead of requeueing them — requeueing dead work would put it ahead of every live round — and let whichever enqueue path next finds the PR alive reopen it. FireDedupe was exempt from the apply-time revalidation because it posts nothing. But its completed round asserts that everyone gating the head has answered, and an override landing after the decision finds the round still queued, so nothing requeues it. Revalidate dedupe too, and report the refusal as the lost race it is rather than as a dedupe. `crq reviewers <repo>` accepted "owner/", "/name" and "owner/name/extra". The read path never contacts GitHub, so a typo printed the fleet default and exited 0, reading as a report about a project crq follows.
|
@codex review |
|
@coderabbitai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 770df01b1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 330cf83e3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/engine/fire.go (1)
308-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
coAwareDedupe/DecideFiretable case for the forced co-reviewer gate.Line 314's new
|| r.ForceCoReviewer(cp.Login)gate has no dedicated fire-rule test (onlyDecideCoPost's own unit test covers the force branch in isolation). A regression here — e.g. a forced co-reviewer being dropped fromgatesand silently deduped away when the primary already reviewed the head — wouldn't be caught by any existing table case.As per coding guidelines,
internal/engine/*_test.go: Table-test convergence and fire rules in engine_test.go.— a case withForceCoReviewersset on a round whose primary already reviewed the head (assertingFireCoOnly/FireCoReviewWaitrather thanFireDedupe) would close this gap.🤖 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 `@internal/engine/fire.go` around lines 308 - 338, The fire-rule table tests need coverage for the forced co-reviewer gate in coAwareDedupe and DecideFire. Add a case in the engine fire-rule table with ForceCoReviewers set for a round where the primary has already reviewed the head, and assert the forced co-reviewer produces FireCoOnly or FireCoReviewWait rather than FireDedupe.Source: Coding guidelines
🤖 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 `@internal/crq/config.go`:
- Around line 541-557: Ensure the processRun initialization safely enforces an
8-character token before slicing randomToken’s result. Update randomToken or the
processRun assignment so the time-based fallback cannot produce a string shorter
than 8 characters, while preserving WriterID’s existing run identifier format.
In `@internal/crq/repoconfig.go`:
- Around line 88-94: Update the SetReviewers CAS closure to compare the merged
reviewer halves and their Set* flags with the stored override before writing;
when they are identical, return ErrNoChange so UpdatedAt and the override
identity remain unchanged. Preserve the existing no-op path for both unspecified
halves that returns s.Reviewers(ctx, repo), and only persist changes when the
reviewer configuration differs.
In `@internal/state/writers_test.go`:
- Around line 100-116: Strengthen TestReopenKeepsTheAdoptionFloor by
initializing r.LastAttemptAt to a non-nil timestamp before calling Complete and
Reopen. Keep the existing assertion, so the test verifies Reopen preserves the
seeded adoption floor rather than merely confirming it remains unset.
---
Outside diff comments:
In `@internal/engine/fire.go`:
- Around line 308-338: The fire-rule table tests need coverage for the forced
co-reviewer gate in coAwareDedupe and DecideFire. Add a case in the engine
fire-rule table with ForceCoReviewers set for a round where the primary has
already reviewed the head, and assert the forced co-reviewer produces FireCoOnly
or FireCoReviewWait rather than FireDedupe.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 8bfe68e3-1a70-4246-9da7-ce97ae54ccaa
📒 Files selected for processing (25)
AGENTS.mdREADME.mdcmd/crq/main.gointernal/crq/auto.gointernal/crq/config.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/repoconfig.gointernal/crq/repoconfig_test.gointernal/crq/reviewers.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/engine/completion.gointernal/engine/coreview.gointernal/engine/coreview_test.gointernal/engine/engine_test.gointernal/engine/fire.gointernal/engine/progress.gointernal/state/dashboard.gointernal/state/state.gointernal/state/tolerant.gointernal/state/tolerant_test.gointernal/state/writers_test.goskills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/engine/**/*.go: Keepinternal/enginepure: decision logic must receivenow, perform no I/O, and use generic co-reviewer evidence keyed by login; all rules must be table-tested.
DecideFireis the sole fire owner and must evaluate guards in order: open, readable/current head, eligible phase, primary availability, already reviewed, slot, account quota, interval, then adopt/post; primary-unavailable and already-reviewed checks must precede slot and quota gates.
Co-reviewers must not consume the globalFireSlot; serialize their trigger posts using per-roundCoBots[login].ClaimedAtCAS claims.
Files:
internal/engine/coreview_test.gointernal/engine/fire.gointernal/engine/completion.gointernal/engine/coreview.gointernal/engine/engine_test.gointernal/engine/progress.go
internal/{dialect,gh,state,engine,crq}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Respect the dependency direction
dialect ← engine ← crq,state ← crq, andgh ← {state, crq}; do not introduce dependency cycles.
Files:
internal/engine/coreview_test.gointernal/engine/fire.gointernal/engine/completion.gointernal/state/dashboard.gointernal/engine/coreview.gointernal/state/writers_test.gointernal/crq/feedback_test.gointernal/state/tolerant.gointernal/crq/state.gointernal/state/tolerant_test.gointernal/engine/engine_test.gointernal/engine/progress.gointernal/crq/config.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/repoconfig.gointernal/crq/reviewers.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/state/state.gointernal/crq/service.go
internal/{state,engine,crq,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the literal phrase “rate limit” outside
internal/ghandinternal/dialect; distinguish GitHubThrottlefrom CodeRabbitAccountQuotaor “account blocked.”
Files:
internal/engine/coreview_test.gointernal/engine/fire.gointernal/engine/completion.gointernal/state/dashboard.gointernal/engine/coreview.gointernal/state/writers_test.gointernal/crq/feedback_test.gointernal/state/tolerant.gointernal/crq/state.gointernal/state/tolerant_test.gointernal/engine/engine_test.gointernal/engine/progress.gointernal/crq/config.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/repoconfig.gointernal/crq/reviewers.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/state/state.gointernal/crq/service.go
internal/engine/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Table-test convergence and fire rules in
engine_test.go.
Files:
internal/engine/coreview_test.gointernal/engine/engine_test.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Maintain persisted schema v3 with oneRoundper PR, one globalFireSlot,AccountQuota, and anArchive; never delete rounds, preserve unknown JSON members, and dual-write Codex fields for compatibility.
Implement legalRoundtransitions as methods that return errors for illegal edges; never delete a round, and keep completed rounds as deduplication markers.
Files:
internal/state/dashboard.gointernal/state/writers_test.gointernal/state/tolerant.gointernal/state/tolerant_test.gointernal/state/state.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqlimited to orchestration and package wiring; it must not contain bot wording or duplicate decision logic.
observeis the single place that queries GitHub and constructsengine.Observation;service.gois the only effects executor for CAS writes and comment posting, andDryRunmust write nothing.
Keep bot identity as data through login-keyed registry and observation structures; do not create per-bot event kinds or verdicts.
Files:
internal/crq/feedback_test.gointernal/crq/state.gointernal/crq/config.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/repoconfig.gointernal/crq/reviewers.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/crq/service.go
internal/crq/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add a default in
parseCoBotswhen a new co-reviewer should be enabled by default.
Files:
internal/crq/config.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-27T12:09:25.123Z
Learning: Use the observe → decide → apply architecture: observe external state once, decide purely in the engine, and apply effects only through the service layer.
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-27T12:09:25.123Z
Learning: Preserve frozen `crq loop` exit codes: 0 for converged/skipped, 10 for findings, and 2 for timeout.
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.
Applied to files:
internal/crq/feedback_test.gointernal/crq/state.gointernal/crq/config.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/repoconfig.gointernal/crq/reviewers.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/crq/service.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.
Applied to files:
internal/crq/feedback_test.gointernal/crq/state.gointernal/crq/config.gointernal/crq/auto.gointernal/crq/service_test.gointernal/crq/repoconfig.gointernal/crq/reviewers.gointernal/crq/feedback.gointernal/crq/repoconfig_test.gointernal/crq/service.go
🪛 OpenGrep (1.25.0)
internal/crq/service_test.go
[ERROR] 2645-2645: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🔇 Additional comments (28)
internal/state/dashboard.go (1)
194-205: LGTM!Also applies to: 282-282, 319-319, 331-331
internal/crq/feedback_test.go (1)
2205-2242: LGTM!Also applies to: 2244-2279
cmd/crq/main.go (1)
214-219: LGTM!Also applies to: 365-368, 522-550, 738-815, 817-830
README.md (1)
388-390: LGTM!Also applies to: 518-520
skills/coderabbit-queue/SKILL.md (1)
174-196: LGTM!internal/crq/state.go (1)
129-142: LGTM!internal/state/state.go (1)
106-123: LGTM!Also applies to: 256-266, 303-317, 399-403, 494-507, 541-580, 651-676, 789-841, 921-926, 1051-1075
internal/state/tolerant.go (1)
31-56: LGTM!internal/state/writers_test.go (1)
68-94: LGTM!internal/state/tolerant_test.go (1)
71-163: LGTM!AGENTS.md (1)
39-42: LGTM!Also applies to: 79-97
internal/crq/reviewers.go (1)
234-250: LGTM!Also applies to: 286-300
internal/crq/repoconfig.go (1)
156-184: LGTM!Also applies to: 187-214, 216-229, 255-325, 327-385
internal/crq/repoconfig_test.go (1)
129-138: LGTM!Also applies to: 186-263, 287-290, 334-406, 408-509, 511-533, 535-642, 644-724, 726-844
internal/crq/config.go (1)
331-343: LGTM!Also applies to: 353-379
internal/crq/auto.go (1)
22-25: LGTM!Also applies to: 297-372
internal/crq/feedback.go (4)
19-71: LGTM!Also applies to: 87-175
486-728: LGTM!
1409-1479: LGTM!
809-849: 🗄️ Data Integrity & IntegrationNo change needed for the slot-hold path.
synconly writes dashboard state and does not participate in the CAS round-commit flow, so skipping it here does not leave the persistence mutation unstamped.> Likely an incorrect or invalid review comment.internal/crq/service.go (1)
116-167: LGTM! This resolves the earlier scoping concern about the staleness guard by validatingoverrideChangedinside each verdict's own CAS commit rather than via a single upfront read, and correctly keeps co-reviewer trigger posting off the globalFireSlot.Also applies to: 181-218, 232-335, 488-584, 587-592, 639-685, 695-987, 1006-1144, 1376-1517
internal/crq/service_test.go (1)
363-393: LGTM!Also applies to: 743-788, 1433-1472, 2419-2453, 2630-2693
internal/engine/completion.go (1)
250-287: LGTM!internal/engine/coreview.go (1)
396-436: LGTM!internal/engine/coreview_test.go (1)
102-113: LGTM!internal/engine/engine_test.go (1)
153-176: LGTM!Also applies to: 193-231, 1010-1104
internal/engine/fire.go (1)
127-221: LGTM!internal/engine/progress.go (1)
48-170: LGTM!Also applies to: 172-211
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a454a83f63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # README.md # cmd/crq/main.go # llms.txt # skills/coderabbit-queue/SKILL.md
* Delete the trigger comments nothing needs any more A PR driven through a dozen rounds collects a dozen "@coderabbitai review" comments and a dozen acknowledgements, which buries the conversation a human came to read. crq now removes its own half of that as rounds progress, and `crq tidy` does it on demand. Deleting a comment crq still reads is how this becomes expensive, so a comment has to clear three guards. It belongs to a round that has PROGRESSED — a live round keeps its command, because that is the comment crq adopts instead of posting another. The bot acted after it, so it was read rather than merely old. And it predates the current head, because adoption only ever considers commands newer than the head commit; delete one of those and the next pump posts a duplicate and buys a second review. Only comments crq posted. Candidates are the command IDs recorded on its own rounds, never anything matching the text — a person's "@coderabbitai review" is their decision to ask, not crq's to erase, and crq posts under the same account so authorship alone cannot tell them apart. Never the bots' own comments. An auto-generated reply can be a rate-limit or skipped-review notice, which crq classifies as evidence and surfaces as a finding, so deleting those would quietly destroy feedback nobody had read yet. A recorded command that is already gone counts as tidied rather than failing: the bot removes some of its own command comments, and a person may have tidied by hand. * Remember the review request each retry replaces A rate-limited round retries, and every retry posts a NEW "@coderabbitai review" — the bot answered the previous one with the rate-limit notice, so it can never be adopted again. crq overwrote CommandID and forgot the old comment, which is why a throttled PR collects a column of identical requests: nothing knew they were there, and tidying could only ever find commands from rounds that had finished. A round now records the commands it supersedes, bounded so a PR that retries all day cannot grow its round without limit, and tidy treats them as spent whichever phase the round is in. They are exempt from the predates-the-head check on purpose: crq's own record that it has posted a successor is stronger evidence than any timestamp. Observed on PR #50 — five heads, nine review requests, three of them for the first head alone. Codex is not affected: it gets one command per head because crq skips the trigger when the bot has already reviewed it. * Only delete the review requests crq wrote itself Adoption was the hole. A round records the command it adopted in exactly the same CommandID as one it posted, so "the command IDs crq recorded on its own rounds" included a person's "@coderabbitai review" — and once the head moved on and the bot had spoken, tidy would delete it. With a maintainer token nothing even objects. A round now records the comments it WROTE, with the reviewer each was addressed to and when it landed, and that list is the only candidate source. Adopting a comment is not writing one. It also replaces the spent-command list: a command missing from the round's current anchors is one the round replaced, which is the same fact without a second field. Four things fall out of carrying the reviewer with the comment: * a co-only round's anchor is the co-reviewer's trigger, not the primary's — unrelated CodeRabbit activity no longer passes for the answer that trigger is still waiting on, and the id stops appearing on the deletion list twice; * a superseded co-reviewer trigger is attributed to the bot that was asked rather than to CodeRabbit; * candidates are filtered against the comments actually on the PR, so a comment tidy already deleted is not DELETEd again on every later pass with the 404 read back as a fresh removal; * state is re-read after the observation and before the deletes, so a round another fleet member started in that window keeps its command. An unreadable head commit now keeps every non-superseded command instead of clearing the guard: the command may well be adoptable again once the read recovers, and deleting it buys a second review. And a round that completes while holding the slot ("cleared") or inside the reviewing sweep is tidied — between them that is most successful rounds, so the automatic cleanup was barely running. * Keep tidy off comments that stopped being trigger comments Four things review found in the tidy pass. A recorded comment ID proves crq wrote that comment, not that it is still the one-line command crq wrote. crq posts under the operator's own account, so anyone can edit it into an explanatory note — and the pass would then delete someone's words as a spent request. The body is now checked against the reviewer's trigger command; anything else, including a command whose config changed since, is kept. A refused delete was only logged, so a caller reading an empty `deleted` could not tell "nothing was spent" from "this token may not delete". The pass still tries every ID and now reports the failures in `.failed[]`. Codex can answer a trigger with nothing but its thumbs-up, and that reaction alone completes the round — leaving no review, event or check for answeredAt to find, so the `@codex review` comment was kept for ever. Tidy observes with no round, so observe() never fetches reactions; they are read here instead, once per candidate nothing else has answered. And the README documented neither `crq tidy` nor `CRQ_TIDY`, so an upgrade started deleting comments with no entry in the documented CLI contract. * Tidy the triggers a force-push and a PR reaction retired Three ways the tidy pass read the PR differently from the code it mirrors. Adoption's cutoff is the head commit date raised to the last force-push, because a force-push can point the PR at a commit object older than the commands made for an earlier head. Tidy compared against the commit date alone, so a rebase onto such a commit kept a trigger no round can ever adopt again — for ever, once the PR merged and the closed-PR guard stopped looking. It now asks for the same cutoff, and TidyInput.HeadAt is renamed AdoptableFrom to say which one it is. Codex answers a command in the PR description by reacting to the PR, and observe() accepts that as the round's completion. Tidy only read reactions on each recorded trigger, so a round that ended that way left no evidence it could see and kept the spent "@codex review" comment. Both of observe's sources count now; the PR's reactions are read at most once per pass, and only when a candidate is still unanswered. And a pump that changed nothing reports "waiting", which tidy treated as progress: every poll of a long-running review bought a second full observation of the PR the pump had just observed. Housekeeping does not get to spend the REST quota the queue runs on. The one "waiting" that does move a round parks it with its commands still live, so it has nothing of its own to remove. * Read the third place a Codex thumbs-up can be A Codex-gated round completes on a +1 left on the trigger, on the PR, or on the command the round fired on — observe() reads all three, and for a round whose primary is CodeRabbit that last one is a different comment from the "@codex review" crq posted beside it. Tidying now reads it too, at most once per candidate nothing cheaper has answered, so a round that ended there stops looking like a trigger nobody read. The README and llms.txt said a comment is removed once it predates the current head; it is the cutoff adoption uses, the head commit raised to the last force-push. * Take the reviewer configuration as a value, like the rest of crq This branch was cut before #51, so it still read the reviewer configuration off the Service: observe with no config argument, and coCommandBodies as a Service method. Both are Config methods now, and the branch stopped compiling the moment main moved under it — the tests here passed only because CI builds the branch, not the merge. Tidy takes a Config through the whole path now: which comments count as a trigger depends on who reviews, so that has to be a value the caller supplies. It reads the fleet configuration today, and per-repo reviewers (#57) can substitute one there in a single line without threading anything new through. * Preserve review evidence when tidying commands * Harden tidy command cleanup * Preserve tidy history and throttle backoff * Retain Codex reaction evidence during tidy * Preserve tombstones after ambiguous deletes * Address tidy review findings --------- Co-authored-by: kristofferR <kristofferR@users.noreply.github.com>
1 participant
Roadmap item H, and the request that started this refactor: "which bots you want for which project
should be much easier to set up."
Reopened against
main— the original PR (#52) was closed automatically when its base branch wasdeleted on merging #51. Same branch, rebased onto main, all review threads on it already resolved.
Each reviewer reports its
budget:accountis serialized against the shared CodeRabbit allowance,noneruns immediately and waits for nobody. That is the only property the queue cares about.It lives in the shared state ref, not a committed
.crq.yaml: the daemon has no checkout of therepositories it reviews, and a daemon and an agent reading different configurations while writing one
state ref is a new class of divergence.
The primary stays fleet-wide. Its markers are compiled into the dialect classifiers when the
Service is built, so a per-repo primary would mean per-repo classifiers.
What review changed
Thirteen findings on the original PR, six of them P1, all resolved:
CRQ_COBOTS=codex, asking a project for Bugbot wasrejected as unknown. Choices resolve against the registry now, and required implies enabled.
nevertrigger, so the round waited forevidence no command was ever posted for.
wording and check-run hooks.
already reviewed was never enqueued.
and safely, because
DecideFire's already-reviewed gate counts a completion reply paired to theround's command, not only a submitted
Reviewobject.operator's
CRQ_COBOT_<NAME>_*settings.machine — and
crq reviewersnames any host driving the queue that predates overrides.applyFirerevalidates the override before acting, so a reviewer removed mid-decision is nottriggered anyway.
gofmt -l .clean,go vet ./...clean,go test ./... -count=1green.Ref #42
Summary by CodeRabbit
crq reviewerscommands to view, set, or clear overrides.