Replace the patchwork core with a round state machine, bot dialects, and a pure decision engine#30
Conversation
All CodeRabbit/Codex message heuristics — completion phrases, rate-limit notices, in-progress/failure summaries, findings markup parsers, SHA conventions — move to a dependency-free internal/dialect package. Config markers are injected as CodeRabbit struct fields instead of read from cfg inside classifiers. A golden corpus (internal/dialect/testdata) now pins one file per known message format; existing crq call sites and tests are kept stable through thin aliases in dialect_aliases.go that die with the engine extraction.
github.go and its tests move verbatim to a gh package. The GitHub REST quota vocabulary is renamed to Throttle (IsThrottled/ThrottleWait) so it can no longer be confused with CodeRabbit's account-quota rate limits, which stay a dialect concern. The 2500-comment cap check — calibration policy, not transport — hoists into crq, and the User-Agent version is injected at init since gh cannot import crq. crq keeps compiling unchanged through type/func aliases in gh_aliases.go, deleted once the engine extraction rewrites the callers.
internal/state is schema v3: one Round per PR with owned transitions (queued/reserved/fired/reviewing/awaiting_retry/completed/abandoned), a global FireSlot, and AccountQuota for CodeRabbit's quota. Rounds are never deleted — only transitioned or archived on supersede — so the #448 failure (a requeue deleting the fired marker, re-firing the same head every pass) is structurally unrepresentable, and illegal transitions are errors. internal/engine is the pure decision core both the daemon and crq loop will drive: DecideFire (the single fire owner), Progress (round transitions; ports inflightStatus plus requeueInflight's same-comment rate-limit window logic), and Completion (the ONE round-done answer; ports reviewedByForRound, the completion-reply pairing fold with its prior-review/nonterminal/failed gates, and codexInactiveOrThumbed). Deliberate fixes over v2: the in-progress/failed summary states now gate the daemon path too, and every retry carries a RetryAt cooldown. dialect gains Classifier/BotEvent: issue comments become typed events at the observation boundary, so the engine never touches message text. Old v2 code is untouched and still runs; cutover comes next.
Stage 4a of the refactor: persistence and the daemon/pump paths now run on the schema-v3 round state machine instead of the five parallel v2 maps. - Move the git-ref CAS store and dashboard rendering into internal/state (store.go, dashboard.go), operating on state.State. A payload whose schema version isn't 3 (or won't parse) is logged loudly and auto-reinitialized — no migration, crq is pre-release. The default ref is now crq-state-v3, so a stale binary writing the old ref is harmless. - Rewrite Pump as observe -> engine -> apply: observe.go is the one place that fetches a PR's GitHub facts (head/open, reviews, classified comment events, reactions, adoptable commands with the v2 adoption cutoffs) and builds an engine.Observation. Pump progresses the slot round via engine.Progress, sweeps one reviewing round toward completion, then fires the next eligible round via engine.DecideFire. The mirrored dry-run branch is gone: apply() simply writes and posts nothing under DryRun. PumpResult action strings are preserved. - Rewrite Enqueue/enqueueBatch/Cancel/Status and autoreview needsReview on rounds (NewRound/Supersede/EndRound; needsReview skips only when a round already tracks the current head, any phase). - Port the calibration/quota code to AccountQuota (renamed from Blocked). - Keep feedback.go/Loop/Wait compiling on v3 with minimal shims (waitView, roundAnchor, firedMarker, accountBlockedUntil, wait-lifecycle helpers over round transitions); stage 4b rewrites them. - Add a Round.Dedupe transition (bot already reviewed the head) and drop the reviewing-round wall-clock deadline retry from engine.Progress (the loop owns its own wait timeout; the daemon waits for real bot evidence). - Migrate service_test.go and state_test.go to v3; delete tests that only exercised deleted v2 internals (requeueInflight, inflightStatus, the wait sweep/prune, FiredMax trimming, cooldowns) and add the missing edge cases as engine/state tests.
Completes the flag-day cutover: Loop, Wait, and Feedback now run on the
v3 round state machine and the pure engine, and the transitional v2 shim
and alias layers are gone.
- Feedback keeps its findings-extraction pipeline (review bodies, threads,
review comments, issue comments) but derives convergence from
engine.Completion driven off observe(). observe() now carries the raw
reviews/comments Feedback parses, so one fetch serves both the daemon
(DecideFire/Progress) and the loop (Completion + findings) with no second
path. ReviewedBy comes from CompletionStatus.
- Loop/waitToFire and Wait run on rounds directly: the wait IS the round
(fired/reviewing with a WaitDeadline), drain-first uses
engine.BlockingFindings, Wait uses engine.FindingsOnHead. Exit codes
0/10/2 and the internal 3 dedupe are unchanged; throttle riding, the
>=60s pump cadence, and account-block deadline extension are preserved.
- Deleted the completion-fallback chain and its Service classifier
forwarders (feedbackCompletionContext, applyCompletionReplyFallback,
completionReplyForFiredCommand, reviewCommandReplies, codexInactiveOrThumbed,
codexThumbedUp, hasNonterminal/hasFailedReviewState, is{RateLimited,
ReviewsPaused,ReviewAlreadyDone,CompletionReply,AutoReply}) plus the
FeedbackWait shim struct and waitView/roundAnchor/ensureFeedbackWait/
extendFeedbackWaitDeadline/clearFeedbackWait. The adoption completion-reply
guard moved to engine.CommandHasCompletionReply.
- Deleted the dialect and gh alias layers; call sites use dialect.X/ghapi.X
directly. The crq-local Finding/FeedbackReport JSON output is unchanged.
- Vocabulary: "rate limit" as literal text now lives only in gh (Throttle)
and dialect; engine/state/crq use Throttle (GitHub) or AccountQuota/
"account blocked" (CodeRabbit). The requeue reason="rate limited" log
string is preserved via dialect.ReasonRateLimited.
- Migrated the Loop/Feedback/Wait tests onto the native round API; ported
the markReviewed suffix and completion-reply-adoption edges to
engine_test.go. Added an AGENTS.md architecture map (CLAUDE.md symlink).
Re-enact the 2026-07-16 ha-adjustable-bed#448 incident (crq posted `@coderabbitai review` ~19x in one day, 10+ on a single head) end to end against the fakeGitHub + MemoryStore harness, proving the v3 round-state architecture makes that spam class structurally impossible. Inject a controllable clock into Service (s.clock) for the scheduling decisions in the pump/enqueue/sweep/wait paths, and let the fake GitHub timestamp posted comments off the same clock, so a whole day replays deterministically with no sleeps and no wall-clock dependence. Production leaves the clock nil and reads time.Now; logging/jitter uses are untouched. Five scenarios, each asserting at most one command per (head, retry window): - a rate-limit bounce (40m window, comment edited in place every few passes) fires once, parks at a fixed RetryAt for the whole window, then fires one retry — never more; - an instant "Review finished" ack on a first-ever command keeps the round reviewing (not converged, not completed, no double-fire) and the real review's findings surface via Loop (exit 10); - an in-progress top summary releases the fire slot (a second PR fires) while the round stays open and never re-fires; - a force-push mid-round supersedes the round and fires a fresh command for the new head without adopting the stale one; - the full incident sequence (fire, 15m-fallback rate limit, retry, parseable 40m rate limit, head move mid-window, block-gated refire, real review) posts exactly three commands and completes. No production bug surfaced: the architecture already holds the invariant, and the suite pins it against regression.
📝 WalkthroughWalkthroughThe PR introduces CRQ v3: dialect-based bot parsing, persistent round state, pure fire/completion decisions, coordinated orchestration, Codex gating, observation-backed feedback, and deterministic replay coverage. ChangesCRQ v3 queue architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Service
participant Observation
participant Engine
participant StateStore
GitHub->>Observation: fetch pull, reviews, comments, and reactions
Observation->>Engine: build normalized observation
Engine->>Service: return fire or progress decision
Service->>StateStore: apply round transition with CAS
Service->>GitHub: post or adopt review commands
GitHub->>Observation: expose bot events and findings
Observation->>Engine: evaluate completion
Engine->>Service: return convergence transition
Service->>StateStore: complete round and release slot
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/engine/engine_test.go (1)
46-369: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftTable-test every engine rule and add the missing finding cases.
The current one-off scenarios do not lock down guard precedence, and
BlockingFindings,FindingsOnHead, andConvergedhave no direct coverage. Add table rows for fire ordering, progress/completion variants, SHA lengths, and finding convergence.As per coding guidelines, every engine rule—including fire, progress, completion, finding, and convergence behavior—must be table-tested.
🤖 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/engine_test.go` around lines 46 - 369, The engine tests currently use one-off cases and omit direct coverage for finding and convergence helpers. Convert the rule coverage around DecideFire, Progress, Completion, BlockingFindings, FindingsOnHead, and Converged into table-driven tests, adding rows for fire-guard precedence, progress/completion variants, valid and invalid SHA lengths, and finding-based convergence outcomes while preserving each existing expected behavior.Source: Coding guidelines
internal/crq/feedback.go (3)
353-365: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the round reviewing when findings arrive before required reviewers finish.
Line 360 completes the round unconditionally, even when
allReviewedis false. This prematurely creates a completed dedupe marker and stops daemon progress for the pending reviewer.- s.completeWaitRound(ctx, repo, pr, head) + if allReviewed(report.ReviewedBy) { + s.completeWaitRound(ctx, repo, pr, head) + } return report, 10, nilAs per coding guidelines, completion must be centralized in
Completion, and rounds stay in progress until completion is established.🤖 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/crq/feedback.go` around lines 353 - 365, The findings path in the feedback round currently calls completeWaitRound before all required reviewers have finished. Update the surrounding report handling to leave the round in progress when allReviewed(report.ReviewedBy) is false, and only complete it once completion is established through the centralized Completion flow; preserve the existing status, reason, and return behavior.Source: Coding guidelines
882-912: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove finding deduplication and suppression out of orchestration.
dedupeFindingsdecides which findings remain actionable and therefore directly affects convergence. Put normalization/identity indialectand convergence-oriented suppression/deduplication inengine;crqshould only wire the result.As per coding guidelines,
internal/crqis orchestration-only and finding convergence logic belongs ininternal/engine.🤖 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/crq/feedback.go` around lines 882 - 912, Move dedupeFindings out of internal/crq: put finding normalization and identity generation in the dialect layer, and move actionable filtering plus structured-location and prompt suppression/deduplication into the engine layer. Update the CRQ orchestration to call the engine convergence helper and only wire its result, preserving the current suppression behavior and generated finding IDs.Source: Coding guidelines
124-145: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMove all PR-state reads into
observe.go.
Feedbackcombines the observation with laterreviewThreads, review-comment, and commit reads. Completion and findings can therefore be derived from different GitHub snapshots, causing inconsistent convergence decisions.Extend the raw observation with threads, fallback review comments, and the head commit timestamp, then parse only that snapshot here.
As per coding guidelines,
observe.gomust be the single place querying GitHub for PR state and must build one observation per decision.Also applies to: 179-185
🤖 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/crq/feedback.go` around lines 124 - 145, Move all GitHub PR-state reads out of Feedback and into observe.go, including reviewThreads, fallback review comments, and the head commit timestamp query. Extend the raw observation to carry these values, build one observation per decision, and have Feedback parse only that snapshot for findings and completion. Preserve throttle/error propagation while ensuring all derived state comes from the same GitHub snapshot.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 `@AGENTS.md`:
- Line 38: Update the state-diagram code fence in AGENTS.md to specify the text
language by using the text fence marker, preserving the diagram content
unchanged.
In `@internal/crq/observe.go`:
- Around line 34-43: Update Pump’s decision flow to remove the pullHead
preflight and call observe as the single PR-state read for each decision. Use
the resulting observation’s o.eng.Open and o.eng.Head values when evaluating
fire decisions, ensuring each decision builds exactly one observation and avoids
duplicate pull-state queries.
- Around line 187-188: Update adoptableCommands and the headForcePushCutoff
lookup so a GraphQL failure is not converted to a zero cutoff. Propagate the
lookup error or make adoptableCommands return no adoption candidates when the
force-push guard is unavailable; retain normal cutoff comparison only when the
lookup succeeds.
In `@internal/crq/replay_test.go`:
- Around line 67-84: Replace the duplicated bot-message literals in the replay
harness constants replayCompletionReply, replayInProgress, and
replayUnparseableRateLimit, plus the template used by replayFairUsage, with
content loaded from the corresponding files in the CodeRabbit dialect corpus.
Reuse the existing corpus-loading helpers or add the minimal loader needed so
replay and golden-classification tests share those files, while preserving the
current message variants and rate-limit parameterization.
In `@internal/crq/service_test.go`:
- Line 218: Remove the duplicate retryNoChangeStore type declaration in
internal/crq/service_test.go, keeping only one definition with its existing cfg
Config field so the redeclaration compile error is resolved.
In `@internal/crq/service.go`:
- Around line 461-475: Require each CAS mutation in abandonRound,
supersedeRound, and both reservation branches of fireRound to verify that the
currently stored round matches the observed round’s identity, including Seq and
Head, before mutating or reserving it. Return ErrNoChange when the round is
missing or either identity field differs, preventing stale decisions from
affecting a replacement round.
- Around line 733-745: Use the injectable s.clock() consistently for all
scheduling decisions in the calibration flow: replace wall-clock calls used by
quota freshness checks, wait-timeout calculations, feedback cadence, and
lastFeedbackCheck with the service clock, preserving UTC/elapsed-time semantics
as applicable. Update both freshness checks around the quota read and the
related logic near the wait and feedback handling paths.
- Around line 226-230: Update the quota refresh flow in Pump to avoid calling
RefreshQuota when DryRun is enabled; retain the already-loaded quota snapshot
for dry-run execution, while preserving the existing RefreshQuota behavior for
normal runs. Ensure dry-run processing remains entirely write-free.
- Around line 687-689: Update the detached retry context in the retry flow
around record to derive it with context.WithoutCancel(ctx) instead of
context.Background(), preserving parent context values while ignoring
cancellation before applying the 30-second timeout.
In `@internal/crq/state.go`:
- Around line 103-160: Move waitingHead, containsActive, and firedMarker from
internal/crq into methods on the shared state.State or state.Round types,
updating callers to use those methods. Move accountBlockedUntil into
internal/engine as the time-dependent blocking decision, preserving its
account-wide and awaiting_retry checks. Remove the corresponding internal/crq
helpers so orchestration remains limited to coordinating shared decisions.
In `@internal/dialect/coderabbit.go`:
- Around line 214-225: Move ParseReviewBodyFindings from coderabbit.go to
common.go, preserving its existing body normalization and aggregation behavior
across ParseDetailedReviewFindings, ParsePromptReviewFindings, and
ParseCodexReviewFindings. Keep CodeRabbit-specific recognition and parsing in
coderabbit.go, Codex-specific parsing in codex.go, and ensure bot-message
recognition remains in the parser appropriate to each dialect.
In `@internal/engine/completion.go`:
- Around line 154-183: Update commandReply and commandReplies to retain the
completion reply timestamp as replyAt, then use reply.replyAt—not
reply.commandAt—when calling stateSince in completionReplyForRound and
CommandHasCompletionReply. Ensure contradictory states are evaluated only from
the reply time onward, while preserving the existing before-reply and
after-reply behavior.
In `@internal/engine/findings.go`:
- Around line 14-20: Replace direct SHA prefix comparisons with
dialect.SHAPrefixMatch across BlockingFindings and the current-head matching
logic in internal/engine/findings.go (lines 14-20 and 27-34),
already-reviewed-head detection in internal/engine/fire.go (lines 69-74),
reviewMatchesRound in internal/engine/progress.go (lines 153-160), and
submitted-review and Codex review evidence in internal/engine/completion.go
(lines 49-55 and 109-120). Preserve each surrounding condition while routing all
SHA comparison semantics through the dialect helper; do not introduce SHA
vocabulary in engine packages.
In `@internal/state/dashboard.go`:
- Around line 121-128: Update the remaining-review display in the dashboard
rendering flow around the remaining variable to use the recorded
st.Account.Remaining value when it is populated, while preserving the
blocked-account message and existing via suffix behavior.
- Around line 4-5: Update hashString to use the SHA-256 implementation instead
of crypto/sha1, replacing the corresponding hash constructor while preserving
the existing hex encoding and State.DashboardSHA behavior. Remove the unused
SHA-1 import and add the SHA-256 import.
In `@internal/state/state.go`:
- Around line 349-369: Update EndRound so abandoned rounds remain in s.Rounds
and are not appended to s.Archive or deleted; retain the PhaseAbandoned
transition for ordinary close, cancel, and repair paths. Move the
archive-and-remove behavior into Supersede before creating the fresh queued
round, ensuring only a new head supersedes and archives the existing round.
---
Outside diff comments:
In `@internal/crq/feedback.go`:
- Around line 353-365: The findings path in the feedback round currently calls
completeWaitRound before all required reviewers have finished. Update the
surrounding report handling to leave the round in progress when
allReviewed(report.ReviewedBy) is false, and only complete it once completion is
established through the centralized Completion flow; preserve the existing
status, reason, and return behavior.
- Around line 882-912: Move dedupeFindings out of internal/crq: put finding
normalization and identity generation in the dialect layer, and move actionable
filtering plus structured-location and prompt suppression/deduplication into the
engine layer. Update the CRQ orchestration to call the engine convergence helper
and only wire its result, preserving the current suppression behavior and
generated finding IDs.
- Around line 124-145: Move all GitHub PR-state reads out of Feedback and into
observe.go, including reviewThreads, fallback review comments, and the head
commit timestamp query. Extend the raw observation to carry these values, build
one observation per decision, and have Feedback parse only that snapshot for
findings and completion. Preserve throttle/error propagation while ensuring all
derived state comes from the same GitHub snapshot.
In `@internal/engine/engine_test.go`:
- Around line 46-369: The engine tests currently use one-off cases and omit
direct coverage for finding and convergence helpers. Convert the rule coverage
around DecideFire, Progress, Completion, BlockingFindings, FindingsOnHead, and
Converged into table-driven tests, adding rows for fire-guard precedence,
progress/completion variants, valid and invalid SHA lengths, and finding-based
convergence outcomes while preserving each existing expected 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
Run ID: 13544132-df76-4dcf-912f-c15a70f4c9bf
📒 Files selected for processing (51)
AGENTS.mdCLAUDE.mdcmd/crq/main.gointernal/crq/auto.gointernal/crq/config.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/init.gointernal/crq/observe.gointernal/crq/preflight.gointernal/crq/replay_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/crq/state_test.gointernal/crq/store.gointernal/dialect/coderabbit.gointernal/dialect/codex.gointernal/dialect/common.gointernal/dialect/event.gointernal/dialect/finding.gointernal/dialect/golden_test.gointernal/dialect/testdata/coderabbit/already-reviewed.mdinternal/dialect/testdata/coderabbit/completion-reply.mdinternal/dialect/testdata/coderabbit/findings-failed-to-post.mdinternal/dialect/testdata/coderabbit/findings-nested-quotes.mdinternal/dialect/testdata/coderabbit/findings-outside-diff.mdinternal/dialect/testdata/coderabbit/findings-prompt-block.mdinternal/dialect/testdata/coderabbit/no-actionable-comments.mdinternal/dialect/testdata/coderabbit/rate-limit-bold-window.mdinternal/dialect/testdata/coderabbit/rate-limit-fair-usage.mdinternal/dialect/testdata/coderabbit/rate-limit-legacy.mdinternal/dialect/testdata/coderabbit/review-failed.mdinternal/dialect/testdata/coderabbit/review-in-progress.mdinternal/dialect/testdata/coderabbit/reviews-paused.mdinternal/dialect/testdata/codex/clean-summary-legacy.mdinternal/dialect/testdata/codex/clean-summary-tada.mdinternal/dialect/testdata/codex/findings-outside-diff.mdinternal/dialect/testdata/codex/usage-limit.mdinternal/engine/completion.gointernal/engine/engine.gointernal/engine/engine_test.gointernal/engine/findings.gointernal/engine/fire.gointernal/engine/progress.gointernal/gh/github.gointernal/gh/github_test.gointernal/state/dashboard.gointernal/state/state.gointernal/state/state_test.gointernal/state/store.go
💤 Files with no reviewable changes (1)
- internal/crq/store.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
internal/dialect/testdata/{coderabbit,codex}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
For each new bot-message format, add one corpus file containing the real message under the appropriate dialect testdata directory.
Files:
internal/dialect/testdata/coderabbit/completion-reply.mdinternal/dialect/testdata/codex/clean-summary-legacy.mdinternal/dialect/testdata/coderabbit/findings-outside-diff.mdinternal/dialect/testdata/coderabbit/rate-limit-legacy.mdinternal/dialect/testdata/codex/usage-limit.mdinternal/dialect/testdata/coderabbit/reviews-paused.mdinternal/dialect/testdata/coderabbit/review-failed.mdinternal/dialect/testdata/coderabbit/no-actionable-comments.mdinternal/dialect/testdata/codex/clean-summary-tada.mdinternal/dialect/testdata/codex/findings-outside-diff.mdinternal/dialect/testdata/coderabbit/already-reviewed.mdinternal/dialect/testdata/coderabbit/rate-limit-fair-usage.mdinternal/dialect/testdata/coderabbit/review-in-progress.mdinternal/dialect/testdata/coderabbit/findings-failed-to-post.mdinternal/dialect/testdata/coderabbit/rate-limit-bold-window.mdinternal/dialect/testdata/coderabbit/findings-nested-quotes.mdinternal/dialect/testdata/coderabbit/findings-prompt-block.md
internal/{dialect,engine,crq,state,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the Go dependency direction:
dialect ← engine ← crq,state ← crq, andgh ← {state, crq}; do not introduce dependency cycles.
Files:
internal/engine/findings.gointernal/dialect/finding.gointernal/engine/progress.gointernal/gh/github_test.gointernal/dialect/golden_test.gointernal/engine/engine.gointernal/engine/fire.gointernal/crq/preflight.gointernal/state/dashboard.gointernal/dialect/event.gointernal/crq/observe.gointernal/crq/init.gointernal/dialect/codex.gointernal/engine/completion.gointernal/state/state_test.gointernal/crq/config.gointernal/state/state.gointernal/dialect/common.gointernal/state/store.gointernal/crq/replay_test.gointernal/crq/state_test.gointernal/dialect/coderabbit.gointernal/engine/engine_test.gointernal/crq/auto.gointernal/gh/github.gointernal/crq/state.gointernal/crq/service_test.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/service.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/engine/**/*.go: Keepinternal/enginepure: passnowexplicitly and do not use context or GitHub I/O. Centralize fire decisions inDecideFire, progress inProgress, completion inCompletion, and finding convergence logic in the engine.
DecideFiremust be the sole owner of review firing and consolidate guards in order: open, readable/current head, eligible phase, free slot, account quota, minimum interval, not already reviewed, then adopt/post.
Files:
internal/engine/findings.gointernal/engine/progress.gointernal/engine/engine.gointernal/engine/fire.gointernal/engine/completion.gointernal/engine/engine_test.go
internal/dialect/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep all bot-text knowledge, literal bot wording, classifiers, finding parsers, SHA/severity vocabulary,
Finding,BotEvent, andClassifierdefinitions ininternal/dialect; this package must have zero dependencies.
Files:
internal/dialect/finding.gointernal/dialect/golden_test.gointernal/dialect/event.gointernal/dialect/codex.gointernal/dialect/common.gointernal/dialect/coderabbit.go
internal/gh/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Represent GitHub REST quota using the
Throttleconcept and itsThrottleWait/IsThrottledAPIs; onlyghanddialectmay use the literal textrate limit.
Files:
internal/gh/github_test.gointernal/gh/github.go
internal/dialect/golden_test.go
📄 CodeRabbit inference engine (AGENTS.md)
For each new bot-message format, add one row to
TestGoldenClassification; that row defines the classification specification.
Files:
internal/dialect/golden_test.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only: coordinate service operations and wire packages, but do not place pure decision logic or bot wording there.
Use the shared observe → decide → apply flow for daemon and loop behavior; no code outsideDecideFiremay post the review command.
Files:
internal/crq/preflight.gointernal/crq/observe.gointernal/crq/init.gointernal/crq/config.gointernal/crq/replay_test.gointernal/crq/state_test.gointernal/crq/auto.gointernal/crq/state.gointernal/crq/service_test.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/service.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Maintain persisted schema v3 with oneRoundper PR, one globalFireSlot, the CodeRabbitAccountQuota, and anArchivering.
Implement round transitions as methods onRound, reject illegal edges, never delete rounds, and archive them only when a new head supersedes them.
Use a global CAS-protectedFireSlotto allow at most one concurrent fire fleet-wide; release the slot on bot acknowledgement while keeping the round in progress.
Files:
internal/state/dashboard.gointernal/state/state_test.gointernal/state/state.gointernal/state/store.go
internal/crq/observe.go
📄 CodeRabbit inference engine (AGENTS.md)
Make
observe.gothe single place that queries GitHub for PR state and reduces it to anengine.Observation; build the observation once per decision and retain raw reviews/comments for feedback parsing.
Files:
internal/crq/observe.go
internal/dialect/{coderabbit,codex,common}.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding bot-message recognition, modify only the relevant classifier/parser in
coderabbit.go,codex.go, orcommon.go; keep bot wording out of engine, state, and crq.
Files:
internal/dialect/codex.gointernal/dialect/common.gointernal/dialect/coderabbit.go
internal/engine/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Table-test every engine rule, including fire decisions, progress, completion, and finding/convergence behavior.
Files:
internal/engine/engine_test.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
Make
service.gothe only effects executor for CAS state writes andPostIssueComment;DryRunmust report without writing.
Files:
internal/crq/service.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-17T16:24:08.013Z
Learning: Preserve frozen loop exit codes: `0` for converged/skipped, `10` for findings, and `2` for timeout.
🪛 ast-grep (0.44.1)
internal/state/dashboard.go
[warning] 206-206: SHA-1 is a cryptographically broken hash function vulnerable to collision attacks and is unsuitable for security purposes such as signatures, integrity checks, or password hashing. Use a SHA-2 family function (e.g. sha256.New() / sha256.Sum256()) or SHA-3 instead.
Context: sha1.Sum
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-sha1-go)
🪛 golangci-lint (2.12.2)
internal/state/dashboard.go
[medium] 4-4: G505: Blocklisted import crypto/sha1: weak cryptographic primitive
(gosec)
[medium] 207-207: G401: Use of weak cryptographic primitive
(gosec)
internal/state/store.go
[error] 296-296: Error return value of encoding/json.Marshal is not checked: unsafe type time.Time found
(errchkjson)
internal/dialect/coderabbit.go
[error] 187-187: QF1001: could apply De Morgan's law
(staticcheck)
[error] 211-211: S1007: should use raw string (...) with regexp.MustCompile to avoid having to escape twice
(staticcheck)
internal/crq/service.go
[error] 689-689: Non-inherited new context, use function like context.WithXXX instead
(contextcheck)
🪛 markdownlint-cli2 (0.23.0)
internal/dialect/testdata/coderabbit/completion-reply.md
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/codex/clean-summary-legacy.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/findings-outside-diff.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/rate-limit-legacy.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/codex/usage-limit.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/reviews-paused.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/review-failed.md
[warning] 3-3: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/no-actionable-comments.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/codex/clean-summary-tada.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/codex/findings-outside-diff.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/already-reviewed.md
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/rate-limit-fair-usage.md
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/review-in-progress.md
[warning] 3-3: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/findings-failed-to-post.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/rate-limit-bold-window.md
[warning] 3-3: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/findings-nested-quotes.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/findings-prompt-block.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
[warning] 4-4: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 15-15: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
AGENTS.md
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.25.0)
internal/state/dashboard.go
[WARNING] 207-207: MD5 and SHA1 are cryptographically broken and should not be used for security purposes. Use SHA-256 or stronger.
(coderabbit.crypto.go-weak-hash)
🔇 Additional comments (50)
internal/dialect/coderabbit.go (1)
1-213: LGTM!Also applies to: 229-336
internal/dialect/event.go (1)
1-114: LGTM!internal/dialect/testdata/coderabbit/rate-limit-bold-window.md (1)
1-8: LGTM!internal/dialect/testdata/coderabbit/rate-limit-fair-usage.md (1)
1-2: LGTM!internal/dialect/testdata/coderabbit/rate-limit-legacy.md (1)
1-1: LGTM!internal/dialect/testdata/coderabbit/review-failed.md (1)
1-5: LGTM!internal/dialect/testdata/coderabbit/review-in-progress.md (1)
1-8: LGTM!internal/dialect/testdata/coderabbit/reviews-paused.md (1)
1-4: LGTM!internal/dialect/testdata/codex/clean-summary-legacy.md (1)
1-1: LGTM!internal/crq/preflight.go (1)
18-19: LGTM!Also applies to: 242-246
internal/dialect/codex.go (1)
1-142: LGTM!internal/dialect/common.go (1)
1-209: LGTM!internal/dialect/finding.go (1)
1-51: LGTM!internal/dialect/golden_test.go (1)
1-181: LGTM!internal/dialect/testdata/coderabbit/already-reviewed.md (1)
1-2: LGTM!internal/dialect/testdata/coderabbit/completion-reply.md (1)
1-2: LGTM!internal/dialect/testdata/coderabbit/findings-failed-to-post.md (1)
1-15: LGTM!internal/dialect/testdata/coderabbit/findings-nested-quotes.md (1)
1-27: LGTM!internal/dialect/testdata/coderabbit/findings-outside-diff.md (1)
1-19: LGTM!internal/dialect/testdata/coderabbit/findings-prompt-block.md (1)
1-16: LGTM!internal/dialect/testdata/coderabbit/no-actionable-comments.md (1)
1-3: LGTM!internal/dialect/testdata/codex/clean-summary-tada.md (1)
1-3: LGTM!internal/dialect/testdata/codex/findings-outside-diff.md (1)
1-10: LGTM!internal/dialect/testdata/codex/usage-limit.md (1)
1-1: LGTM!internal/gh/github.go (1)
1-1: LGTM!Also applies to: 36-39, 348-348, 362-370, 402-423, 524-524, 548-548, 569-569, 591-591, 630-630, 741-747, 1065-1082, 1127-1151
internal/gh/github_test.go (1)
1-1: LGTM!internal/crq/init.go (1)
8-9: LGTM!Also applies to: 19-19, 62-62
internal/crq/observe.go (1)
45-186: LGTM!Also applies to: 190-225, 258-262
cmd/crq/main.go (1)
16-16: LGTM!Also applies to: 66-72, 245-245
internal/crq/config.go (1)
11-20: LGTM!Also applies to: 103-109, 261-261, 289-289
CLAUDE.md (1)
1-1: LGTM!internal/state/state.go (1)
1-348: LGTM!Also applies to: 370-430
internal/state/dashboard.go (1)
1-3: LGTM!Also applies to: 6-120, 129-205
internal/state/store.go (1)
1-300: LGTM!internal/state/state_test.go (1)
1-224: LGTM!internal/crq/state.go (1)
8-90: LGTM!internal/crq/state_test.go (1)
4-26: LGTM!internal/engine/engine.go (1)
1-100: LGTM!internal/engine/fire.go (1)
1-68: LGTM!Also applies to: 75-94
internal/engine/progress.go (1)
1-152: LGTM!Also applies to: 161-161
internal/engine/completion.go (1)
1-48: LGTM!Also applies to: 56-108, 121-148, 184-214, 285-307
internal/engine/findings.go (1)
1-13: LGTM!Also applies to: 21-26, 35-41
internal/engine/engine_test.go (1)
1-45: LGTM!internal/crq/service.go (1)
13-178: LGTM!Also applies to: 189-225, 231-460, 476-524, 532-556, 579-686, 690-732, 746-1006, 1018-1025, 1034-1074, 1081-1140
internal/crq/auto.go (1)
10-12: LGTM!Also applies to: 29-30, 55-59, 72-73, 90-90, 112-115, 129-139, 223-223, 232-232, 243-243, 263-268, 277-277, 289-342
internal/crq/service_test.go (1)
12-217: LGTM!Also applies to: 219-1801
internal/crq/feedback.go (1)
14-123: LGTM!Also applies to: 146-178, 186-352, 366-881, 913-940
internal/crq/feedback_test.go (1)
8-1876: LGTM!internal/crq/replay_test.go (1)
1-66: LGTM!Also applies to: 85-645
AGENTS.md (1)
1-37: LGTM!Also applies to: 39-97
crq now drives Codex instead of only listening for it. When Codex is in CRQ_REQUIRED_BOTS and does not auto-review the PR, the fire step posts CRQ_CODEX_CMD (default "@codex review") alongside the CodeRabbit command and records it on the round (CodexCommandID); a failed Codex post self-heals on a later pump. Auto-review is detected from evidence — any Codex review or clean summary that no @codex command preceded — and suppresses posting entirely. A Codex that joins a round uninvited (actionable comment, review, or thumbs-up in the round window, or auto-review on the PR) now gates that round's completion dynamically, so both bots converge on the same head even where Codex isn't configured as required. Its usage-limit notice releases the dynamic gate so an exhausted Codex cannot stall rounds it volunteered for; explicitly-required Codex stays bounded by the normal feedback deadline. The @codex command and the usage-limit notice are classified in dialect (EvCodexCommand/EvCodexUsageLimit, corpus-pinned); the decisions live in engine/codex.go (DecideCodexPost + the dynamic gate, table-tested); the end-to-end scenarios in codex_replay_test.go drive all three modes through the real pump: required-no-auto posts exactly once and never on retry, auto-active never posts, and the dynamic gate holds then releases on usage exhaustion.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Dogfooding crq v3 on its own PR (crq loop → exit 10, drain-first) surfaced twelve findings; nine were valid and are fixed here: - A dry-run Pump no longer calls RefreshQuota — the calibration probe it can post is a side effect dry-run promises not to have. - The detached recordFire retry derives from context.WithoutCancel(ctx) instead of context.Background(), keeping parent values. - ParseReviewBodyFindings moves to dialect/common.go — it dispatches across both dialects and never belonged to coderabbit.go. - engine/findings.go compares commits via dialect.SHAPrefixMatch, so a 10-char Codex commit matches a 9-char head instead of silently failing the one-directional prefix test. - The dashboard renders the recorded remaining-review count, hashes with SHA-256, and AGENTS.md's diagram fence declares a language. - The round-view helpers (WaitingHead, RoundWaitDeadline, ContainsActive, FiredMarker, AccountBlockedUntil) move onto state.State where they belong; crq keeps only genuinely crq-shaped adapters. - The replay suite loads bot messages from the dialect corpus instead of duplicating the wording (new corpus file: rate-limit-no-window.md). Declined with reasons on the PR threads: the duplicate-type claim (the suite compiles; phantom finding), merging the pullHead preflight into observe (the cheap head check deliberately gates the expensive fetch behind quota/pacing), and keeping abandoned rounds in Rounds (archiving on terminal PR events is the designed semantics; the live already-reviewed check covers re-fire safety).
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
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)
72-81: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not let CodeRabbit deduplication skip a required Codex review.
Line 76 returns
FireDedupebeforePostCodexis computed. If CodeRabbit already reviewed the head but required Codex has not, the round is deduped without ever requesting Codex. Add a Codex-only posting outcome or equivalent apply-layer signal before final deduplication.🤖 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 72 - 81, Update the decision flow around the review deduplication loop and PostCodex computation so a CodeRabbit match cannot return FireDedupe before a required Codex request is scheduled. Compute or preserve the Codex-posting outcome first, then return an equivalent Codex-only/apply-layer signal when Codex is still required, while retaining FireDedupe when no Codex action is needed.
🤖 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/codex_replay_test.go`:
- Around line 15-29: Move the Codex login, clean-summary, usage-limit, and
review-command literals out of the replay test and use the canonical
fixtures/constants exposed by internal/dialect. Update newCodexReplayFixture and
the related test expectations to reference those dialect-owned symbols,
including the SHA-parameterized clean-summary fixture, so bot wording exists
only in the dialect package.
In `@internal/crq/service.go`:
- Around line 740-776: Update fireCodexReview and the Round state flow to
reserve Codex posting before calling postCodexReviewComment, using a persisted
in-progress/token marker and bounded retry cooldown so concurrent pumps cannot
post duplicates. After the external call, finalize the reservation with the
command ID on success, or release it while retaining the cooldown on
failure/ambiguous results. Update selfHealCodex to honor the reservation and
cooldown when deciding whether to retry.
In `@internal/engine/codex.go`:
- Around line 78-94: Update CodexAutoActive and its command-correlation logic so
automatic reviews or clean summaries are evaluated only against commands
associated with the same head or current observation window, rather than
firstCodexCommand across the entire PR history. Ensure old commands cannot
suppress later-head automatic detection, and early automatic reviews cannot
suppress commands for later windows; preserve the existing Codex bot and
clean-summary checks within the relevant window.
- Around line 11-14: The canonical Codex bot identity is defined in the engine
instead of the dialect layer. Add and export the identity/key from
internal/dialect, remove the local codexBot literal in internal/engine/codex.go,
and consume the dialect-owned symbol there. Update
internal/engine/engine_test.go lines 336-359 to use that dialect symbol for test
policies and observations; ensure the bot literal appears only in
internal/dialect.
---
Outside diff comments:
In `@internal/engine/fire.go`:
- Around line 72-81: Update the decision flow around the review deduplication
loop and PostCodex computation so a CodeRabbit match cannot return FireDedupe
before a required Codex request is scheduled. Compute or preserve the
Codex-posting outcome first, then return an equivalent Codex-only/apply-layer
signal when Codex is still required, while retaining FireDedupe when no Codex
action is 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: ASSERTIVE
Plan: Pro
Run ID: 28be516b-b552-44bc-bc48-40dd4d4f9b47
📒 Files selected for processing (19)
README.mdinternal/crq/codex_replay_test.gointernal/crq/config.gointernal/crq/observe.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/dialect/codex.gointernal/dialect/common.gointernal/dialect/event.gointernal/dialect/golden_test.gointernal/dialect/testdata/codex/review-command.mdinternal/engine/codex.gointernal/engine/completion.gointernal/engine/engine.gointernal/engine/engine_test.gointernal/engine/fire.gointernal/state/state.gollms.txt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
internal/dialect/testdata/{coderabbit,codex}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Each new bot-message format must include one corpus file containing the real message.
Files:
internal/dialect/testdata/codex/review-command.md
internal/{dialect,engine,crq,state,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Enforce the dependency direction
dialect � engine � crq,state � crq, andgh � {state, crq}; prevent dependency cycles.
Files:
internal/engine/fire.gointernal/crq/codex_replay_test.gointernal/engine/codex.gointernal/dialect/golden_test.gointernal/engine/engine.gointernal/crq/observe.gointernal/engine/completion.gointernal/crq/config.gointernal/dialect/event.gointernal/crq/state.gointernal/state/state.gointernal/dialect/common.gointernal/dialect/codex.gointernal/engine/engine_test.gointernal/crq/service_test.gointernal/crq/service.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/engine/**/*.go: Keep the engine pure: decision logic must not perform I/O, usecontext, or depend ongh; passnowexplicitly.
DecideFireis the single owner of review firing and must consolidate guards in order: open, readable/current head, eligible phase, free slot, account quota, minimum interval, not already reviewed, then adopt/post.
Files:
internal/engine/fire.gointernal/engine/codex.gointernal/engine/engine.gointernal/engine/completion.gointernal/engine/engine_test.go
internal/{engine,state,crq}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use
AccountQuotaor “account blocked” for the CodeRabbit account quota; do not call it GitHub RESTrate limitor mix the two concepts.
Files:
internal/engine/fire.gointernal/crq/codex_replay_test.gointernal/engine/codex.gointernal/engine/engine.gointernal/crq/observe.gointernal/engine/completion.gointernal/crq/config.gointernal/crq/state.gointernal/state/state.gointernal/engine/engine_test.gointernal/crq/service_test.gointernal/crq/service.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep orchestration in
internal/crq; it should wire packages through service, observation, automation, feedback, configuration, calibration, preflight, and initialization rather than implement bot classification or pure decision rules.
Files:
internal/crq/codex_replay_test.gointernal/crq/observe.gointernal/crq/config.gointernal/crq/state.gointernal/crq/service_test.gointernal/crq/service.go
internal/dialect/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep all bot-text knowledge in
internal/dialect, including CodeRabbit/Codex classifiers, finding parsers, SHA/severity vocabulary, andFinding,BotEvent, andClassifiertypes. Bot literal wording must appear only there.
Files:
internal/dialect/golden_test.gointernal/dialect/event.gointernal/dialect/common.gointernal/dialect/codex.go
internal/dialect/golden_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Each new bot-message corpus file must have one corresponding row in
TestGoldenClassification; that row defines the classification specification.
Files:
internal/dialect/golden_test.go
internal/crq/observe.go
📄 CodeRabbit inference engine (AGENTS.md)
observe.gois the single place that queries GitHub for PR state and constructsengine.Observation; it must carry raw reviews/comments soFeedbackparses findings from the same fetch.
Files:
internal/crq/observe.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Persist schema v3 with oneRoundper PR, one globalFireSlot, the CodeRabbitAccountQuota, and anArchivering.
ARoundmust never be deleted. Illegal state transitions must return errors; rounds may only transition or be archived when superseded by a new head.
Files:
internal/state/state.go
internal/dialect/{coderabbit,codex,common}.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding recognition for a new bot phrasing, modify only the relevant classifier/parser in
internal/dialect; keep bot wording out of engine, state, and crq.
Files:
internal/dialect/common.gointernal/dialect/codex.go
internal/engine/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Table-test every engine rule, including
DecideFire, progress, completion, and convergence behavior.
Files:
internal/engine/engine_test.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
service.gois the only effects executor: apply CAS state writes andPostIssueComment;DryRunmust report without writing.
Files:
internal/crq/service.go
🪛 markdownlint-cli2 (0.23.0)
internal/dialect/testdata/codex/review-command.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🪛 OpenGrep (1.25.0)
internal/crq/codex_replay_test.go
[ERROR] 146-146: 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 (18)
internal/state/state.go (1)
57-60: LGTM!internal/crq/state.go (1)
87-87: LGTM!internal/engine/engine.go (1)
19-19: LGTM!Also applies to: 67-83
internal/engine/codex.go (1)
16-76: LGTM!Also applies to: 97-161
internal/engine/completion.go (1)
49-58: LGTM!Also applies to: 92-99, 126-139
internal/engine/engine_test.go (1)
332-335: LGTM!Also applies to: 360-404
internal/crq/service_test.go (1)
715-715: LGTM!Also applies to: 1291-1291
internal/crq/codex_replay_test.go (1)
1-14: LGTM!Also applies to: 30-225
llms.txt (1)
64-67: LGTM!README.md (1)
423-423: LGTM!Also applies to: 446-453
internal/dialect/codex.go (1)
46-52: LGTM!Also applies to: 99-150
internal/dialect/common.go (1)
123-149: LGTM!internal/dialect/event.go (1)
18-18: LGTM!Also applies to: 27-28, 66-72, 86-103
internal/dialect/golden_test.go (1)
29-108: LGTM!internal/dialect/testdata/codex/review-command.md (1)
1-1: LGTM!internal/crq/service.go (1)
269-269: LGTM!Also applies to: 402-402, 438-440, 543-546, 596-598, 653-659, 674-690
internal/crq/config.go (1)
35-43: LGTM!Also applies to: 111-113
internal/crq/observe.go (1)
67-69: LGTM!Also applies to: 103-118, 152-199, 202-264
Dogfooding on PR #30 surfaced both, one per review round: 1. CodeRabbit appends an "Also applies to: <lines>" trailer to real finding bodies. The non-actionable denylist matched the phrase as a substring, so all four findings carrying the trailer in round one were silently dropped — never surfaced, never resolved. The phrase now only marks a comment that IS the trailer (prefix match); corpus files pin both directions. 2. CodeRabbit submits empty-bodied COMMENTED review objects as carriers for its inline-comment batches, minutes before the real review lands. Counting a shell as review evidence converged round two with zero findings at 17:26 while the real review was still posting until 17:32. observe() now skips body-less COMMENTED reviews; the replay suite pins the shell-then-real-review sequence, and review fixtures carry real bodies as actual bot reviews do.
… review - CAS identity guards: every round-mutating CAS closure (abandon, dedupe, supersede, both fireRound reservations, recordFire, fireCodexOnly) now verifies the stored round still matches the observed Seq+Head via a shared sameRound helper, so a concurrent supersede reads as a benign lost race instead of mutating a replacement round. - Canonical Codex login: export dialect.CodexBotLogin and consume it from IsCodexBot, engine's codexBot, config's extraFeedbackBots, and the tests; the replay suite now loads the clean-summary (SHA-templated) and usage-limit texts from the dialect corpus instead of duplicating bot wording. - Adoption force-push guard: headForcePushCutoff returns (time, error); on any lookup failure adoptableCommands skips adoption this pass rather than adopting an old-head command blind (worst case is re-posting an existing command). - Codex auto-review detection scoped to the latest evidence: only the most recent Codex review/clean-summary decides auto-active, so an old unprompted review no longer suppresses posting once a later commanded review lands. - Dedupe no longer skips a required Codex: a CodeRabbit review at the head that a gating Codex has not yet reviewed yields FireCodexOnly (post only the Codex command, wait on Codex alone) when crq may post, FireNo when Codex will still produce its own evidence, and plain FireDedupe only when Codex is truly unobtainable so the round never wedges. - Calibration scheduling uses the injectable s.clock() so replay tests can drive quota freshness deterministically.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/crq/service.go (1)
219-234: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep every dry-run path state-write-free. The guard only skips quota refresh: Line 224 can persist an abandonment before it, while
Waitstill persists an enqueue and can repair a completed round.
internal/crq/service.go#L219-L234: return the closed-PR dry-run result without callingabandonRound.internal/crq/service.go#L1118-L1155: simulate enqueue/dedupe decisions in dry-run; do not invokeEnqueueor the poison-marker repair update.As per coding guidelines, “DryRun must report without writing.”
🤖 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/crq/service.go` around lines 219 - 234, Keep all dry-run paths state-write-free: in internal/crq/service.go lines 219-234, return the closed-PR dry-run result without calling abandonRound; in lines 1118-1155, update Wait to simulate enqueue and dedupe decisions without invoking Enqueue or performing the poison-marker repair update. Preserve normal write behavior outside DryRun.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.
Outside diff comments:
In `@internal/crq/service.go`:
- Around line 219-234: Keep all dry-run paths state-write-free: in
internal/crq/service.go lines 219-234, return the closed-PR dry-run result
without calling abandonRound; in lines 1118-1155, update Wait to simulate
enqueue and dedupe decisions without invoking Enqueue or performing the
poison-marker repair update. Preserve normal write behavior outside DryRun.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07cfc6c4-2d7d-43ac-9e1d-5b5c3634752e
📒 Files selected for processing (15)
AGENTS.mdinternal/crq/codex_replay_test.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/replay_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/dialect/coderabbit.gointernal/dialect/common.gointernal/dialect/golden_test.gointernal/dialect/testdata/coderabbit/rate-limit-no-window.mdinternal/engine/findings.gointernal/state/dashboard.gointernal/state/state.go
💤 Files with no reviewable changes (2)
- internal/dialect/coderabbit.go
- internal/crq/state.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
internal/dialect/testdata/{coderabbit,codex}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
For each new bot-message format, add one corpus file containing the real message.
Files:
internal/dialect/testdata/coderabbit/rate-limit-no-window.md
internal/{dialect,engine,crq,state,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the dependency direction:
dialect ← engine ← crq,state ← crq, andgh ← {state, crq}; introduce no dependency cycles.
Files:
internal/engine/findings.gointernal/dialect/golden_test.gointernal/crq/codex_replay_test.gointernal/dialect/common.gointernal/state/dashboard.gointernal/state/state.gointernal/crq/replay_test.gointernal/crq/feedback.gointernal/crq/service_test.gointernal/crq/service.gointernal/crq/feedback_test.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep the engine pure: pass
nowexplicitly and do not use context or GitHub I/O.DecideFireis the sole fire owner, whileProgresshandles round transitions andCompletiondetermines whether a round is done.
Files:
internal/engine/findings.go
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the instructions defined in
AGENTS.mdfor the applicable project or directory scope.
Files:
AGENTS.md
internal/dialect/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep all bot-text knowledge in
internal/dialect, including CodeRabbit/Codex classifiers, finding parsers, SHA/severity vocabulary, and bot event types; bot literal wording must appear only there.
Files:
internal/dialect/golden_test.gointernal/dialect/common.go
internal/dialect/golden_test.go
📄 CodeRabbit inference engine (AGENTS.md)
For each new bot-message format, add one
TestGoldenClassificationrow; that row defines the classification specification.
Files:
internal/dialect/golden_test.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only: it coordinates service operations and wires packages but must not contain decision logic or bot wording.
Use the shared observe → decide → apply flow for both daemon and loop operation; no code outsideengine.DecideFiremay post the review command.
Files:
internal/crq/codex_replay_test.gointernal/crq/replay_test.gointernal/crq/feedback.gointernal/crq/service_test.gointernal/crq/service.gointernal/crq/feedback_test.go
internal/dialect/{coderabbit,codex,common}.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding support for a new bot-message phrasing, modify only the relevant classifier/parser in
internal/dialect; keep the wording out of engine, state, and crq.
Files:
internal/dialect/common.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Maintain persisted schema v3 with oneRoundper PR, one globalFireSlot, the CodeRabbitAccountQuota, and anArchivering.
Rounds must never be deleted; illegalRoundtransitions must return errors, and superseded rounds may only be archived.
UseFireSlotwith CAS to allow at most one concurrent fire fleet-wide; releasing the slot on bot acknowledgement must not close the round.
Files:
internal/state/dashboard.gointernal/state/state.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
Make
service.gothe only effects executor for CAS state writes andPostIssueComment;DryRunmust report without writing.
Files:
internal/crq/service.go
🪛 markdownlint-cli2 (0.23.0)
internal/dialect/testdata/coderabbit/rate-limit-no-window.md
[warning] 2-2: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (13)
AGENTS.md (1)
38-38: LGTM!internal/crq/feedback_test.go (1)
8-27: LGTM!Also applies to: 48-60, 97-116, 200-219, 232-232, 254-273, 306-341, 363-382, 406-420, 430-430, 447-594, 644-649, 669-669, 699-714, 739-763, 787-794, 804-807, 819-819, 870-891, 911-924, 950-963, 983-996, 1019-1035, 1057-1071, 1097-1113, 1139-1155, 1187-1200, 1213-1213, 1226-1226, 1269-1287, 1299-1299, 1336-1379, 1433-1433, 1462-1470, 1500-1512, 1529-1533, 1552-1570, 1607-1625, 1666-1686, 1721-1734, 1765-1782, 1811-1817, 1838-1839, 1856-1876
internal/dialect/common.go (1)
201-214: LGTM!internal/dialect/golden_test.go (1)
55-56: LGTM!internal/dialect/testdata/coderabbit/rate-limit-no-window.md (1)
1-3: LGTM!internal/engine/findings.go (1)
15-15: LGTM!Also applies to: 28-28
internal/crq/codex_replay_test.go (1)
79-135: LGTM!internal/crq/feedback.go (1)
243-422: LGTM!Also applies to: 430-467, 576-617
internal/crq/replay_test.go (1)
69-90: LGTM!Also applies to: 287-354, 363-419, 427-484, 564-650
internal/crq/service.go (1)
681-780: LGTM!Also applies to: 959-1033
internal/crq/service_test.go (1)
431-435: LGTM!Also applies to: 472-476, 635-642, 685-692, 1116-1118, 1627-1634, 1701-1708
internal/state/dashboard.go (1)
4-4: LGTM!Also applies to: 93-182, 201-212
internal/state/state.go (1)
436-495: LGTM!
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/observe.go`:
- Around line 54-61: Restrict the empty COMMENTED review suppression in the
review-processing loop to reviews authored by the configured CodeRabbit
identity. Update the predicate near review.Body and review.State to also
validate that reviewer, preserving the existing behavior for CodeRabbit shells
while retaining legitimate empty reviews from Codex or other bots.
In `@internal/crq/replay_test.go`:
- Around line 183-187: Move bot-message literals out of tests: in
internal/crq/replay_test.go lines 183-187, load the CodeRabbit review body from
dialect corpus data or use a neutral sentinel; in
internal/crq/codex_replay_test.go lines 64-68, replace the Codex prose with
dialect-owned corpus data or a neutral sentinel; and in
internal/engine/engine_test.go lines 336-343, use an opaque non-bot command
value while preserving the existing blank/nonblank behavior tests.
In `@internal/crq/service.go`:
- Around line 723-741: The failed-post handling in the `PostIssueComment` error
path currently requeues the round immediately; after `ReleaseToQueue` in the
`s.store.Update` callback, set the round’s next eligible time to `now +
RetryBackoff` while still releasing the slot. Preserve the existing token
validation, warning, state persistence, sync, and `PumpResult` behavior.
In `@internal/engine/codex.go`:
- Around line 78-127: Update CodexAutoActive and codexCommandAtOrBefore so
historical commands only suppress the latest Codex evidence when they belong to
that same head/evidence window, rather than any command at or before its
timestamp. Track the preceding Codex review or clean-summary boundary when
evaluating the latest evidence, preserving the commanded classification when a
command precedes that evidence and marking a later unprompted evidence event as
automatic.
In `@internal/engine/engine_test.go`:
- Around line 416-456: Convert TestDecideFireCodexDedupe into a table-driven
test with named cases covering each existing Codex fire-decision scenario and
expected verdict. Define shared fixtures outside the case table where
appropriate, then iterate through the rows and report failures with the case
name. Preserve all current observations, request configurations, and verdict
expectations so future Codex guard-matrix cases can be added systematically.
In `@internal/engine/fire.go`:
- Around line 125-126: The Codex waiting branch in the fire decision logic must
persist a timed waiting state instead of returning an untracked FireNo decision.
Update the path guarded by obs.CodexAutoActive, obs.CodexCommands, or
r.CodexCommandID to adopt the existing command-phase mechanism and set an
appropriate WaitDeadline, or use an existing wait verdict that records the
fired/reviewing phase; ensure subsequent pumps can time out when Codex never
responds.
🪄 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
Run ID: 12982932-fe7a-4ee1-96d7-e7a837d68c43
📒 Files selected for processing (15)
internal/crq/codex_replay_test.gointernal/crq/config.gointernal/crq/feedback_test.gointernal/crq/observe.gointernal/crq/replay_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/dialect/codex.gointernal/dialect/common.gointernal/dialect/golden_test.gointernal/dialect/testdata/coderabbit/finding-with-also-applies-trailer.mdinternal/dialect/testdata/coderabbit/thread-ack-also-applies.mdinternal/engine/codex.gointernal/engine/engine_test.gointernal/engine/fire.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
internal/dialect/**/*.{go,md}
📄 CodeRabbit inference engine (AGENTS.md)
When adding a bot-message format, change only its classifier/parser in
internal/dialect, one real-message corpus file underinternal/dialect/testdata/{coderabbit,codex}/, and one correspondingTestGoldenClassificationrow.
Files:
internal/dialect/testdata/coderabbit/finding-with-also-applies-trailer.mdinternal/dialect/testdata/coderabbit/thread-ack-also-applies.mdinternal/dialect/golden_test.gointernal/dialect/common.gointernal/dialect/codex.go
internal/dialect/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep all bot-text knowledge, literal bot wording, completion/rate-limit/paused/in-progress/failed/clean-review classifiers, finding parsers, SHA/severity vocabulary,
Finding,BotEvent, andClassifierdefinitions ininternal/dialect; this package must have zero dependencies.
Files:
internal/dialect/golden_test.gointernal/dialect/common.gointernal/dialect/codex.go
internal/{dialect,engine,state,crq}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Do not mix GitHub REST
Throttlewith CodeRabbitAccountQuotaor “account blocked”; use the distinct vocabulary consistently, and keep the literal “rate limit” restricted tointernal/ghandinternal/dialect.
Files:
internal/dialect/golden_test.gointernal/engine/codex.gointernal/crq/config.gointernal/engine/fire.gointernal/crq/observe.gointernal/crq/replay_test.gointernal/crq/codex_replay_test.gointernal/dialect/common.gointernal/dialect/codex.gointernal/engine/engine_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/feedback_test.go
internal/dialect/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use
TestGoldenClassificationrows as the specification for how each bot corpus message classifies.
Files:
internal/dialect/golden_test.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/engine/**/*.go: Keepinternal/enginepure: decision logic must receivenowexplicitly and must not use context or GitHub I/O.DecideFireis the single fire owner;Progress,Completion, findings helpers, andPolicybelong here.
DecideFiremust consolidate every fire guard in order: open, readable head, current head, eligible phase, free slot, account quota, minimum interval, not already reviewed, and adopt/post. No other code may post the review command.
Files:
internal/engine/codex.gointernal/engine/fire.gointernal/engine/engine_test.go
internal/{engine, state, crq}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep bot wording out of
internal/engine,internal/state, andinternal/crq; those packages should consume dialect classifications rather than matching literal bot text.
Files:
internal/engine/codex.gointernal/engine/fire.gointernal/engine/engine_test.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only: service operations, observation, automation, feedback assembly, configuration, calibration, preflight, initialization, and package wiring belong here; it must not contain bot wording or duplicate engine decision logic.
Use the shared observe → decide → apply flow for daemon and loop behavior: observation is built once, engine decisions remain pure, and orchestration applies the decision through the service.
Files:
internal/crq/config.gointernal/crq/observe.gointernal/crq/replay_test.gointernal/crq/codex_replay_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/feedback_test.go
internal/crq/observe.go
📄 CodeRabbit inference engine (AGENTS.md)
Make
crq/observe.gothe single place that queries GitHub for a PR and builds anengine.Observation, including the head, open state, reviews, classified bot events, adoptable commands, reactions, and raw reviews/comments for feedback parsing.
Files:
internal/crq/observe.go
internal/engine/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Table-test every engine rule, including fire decisions, progress, completion, and findings/convergence behavior.
Files:
internal/engine/engine_test.go
internal/crq/service.go
📄 CodeRabbit inference engine (AGENTS.md)
Make
crq/service.gothe only effects executor for CAS state writes andPostIssueComment;DryRunmust report without writing state or posting effects.
Files:
internal/crq/service.go
🪛 golangci-lint (2.12.2)
internal/crq/observe.go
[error] 204-204: error is not nil (line 202) but it returns nil
(nilerr)
🪛 LanguageTool
internal/dialect/testdata/coderabbit/finding-with-also-applies-trailer.md
[grammar] ~1-~1: Use a hyphen to join words.
Context: Guard the cutoff against a zero fired time. A round that never fired ...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.23.0)
internal/dialect/testdata/coderabbit/finding-with-also-applies-trailer.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/thread-ack-also-applies.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (15)
internal/dialect/testdata/coderabbit/finding-with-also-applies-trailer.md (1)
1-5: LGTM!internal/dialect/testdata/coderabbit/thread-ack-also-applies.md (1)
1-1: LGTM!internal/dialect/codex.go (1)
1-17: LGTM!Also applies to: 19-35, 37-49, 51-58, 60-68, 70-82, 84-88, 90-102, 104-156
internal/dialect/common.go (1)
1-7: LGTM!Also applies to: 8-13, 15-29, 31-44, 46-58, 60-65, 67-81, 83-107, 109-121, 123-156, 158-163, 164-167, 168-173, 175-184, 186-201, 203-205, 207-220, 222-229
internal/dialect/golden_test.go (1)
1-26: LGTM!Also applies to: 27-62, 63-66, 67-117, 118-200
internal/engine/fire.go (1)
7-7: LGTM!Also applies to: 21-21, 75-80, 106-124, 127-129
internal/engine/codex.go (1)
11-14: LGTM!internal/engine/engine_test.go (1)
462-488: LGTM!Also applies to: 495-525
internal/crq/config.go (1)
251-251: LGTM!internal/crq/observe.go (1)
197-205: LGTM!Also applies to: 282-318
internal/crq/service.go (1)
345-352: LGTM!Also applies to: 448-449, 477-499, 515-515, 544-544, 582-582, 623-623, 688-722, 743-760, 773-773, 902-902, 914-914, 1195-1204, 1294-1294
internal/crq/service_test.go (1)
198-204: LGTM!Also applies to: 669-669, 948-948, 1075-1075
internal/crq/feedback_test.go (1)
110-110: LGTM!Also applies to: 215-215, 269-269, 320-320
internal/crq/replay_test.go (1)
653-705: LGTM!internal/crq/codex_replay_test.go (1)
4-8: LGTM!Also applies to: 17-35, 185-185, 232-239, 244-289
…tion Round-three self-hosted-loop findings on the new head: - The empty-COMMENTED review filter (CodeRabbit's inline-comment carriers) is scoped to the configured reviewer, so it can never drop another bot's empty review that a Codex-gated round waits on. - Both fire paths (CodeRabbit and Codex-only) park in awaiting_retry with a bounded cooldown when a command post fails, instead of re-posting on the next pump — the same no-cooldown-requeue shape that fed the #448 spam. - CodexAutoActive keys on a command in the window since the previous Codex evidence, so a stale @codex review from an earlier round no longer masks a later unprompted review as commanded and suppresses posting forever. Declined on the PR: converting TestDecideFireCodexDedupe to a table test (the cases are already readable; churn), and giving the codex-wait branch a timed phase (it only fires when Codex evidence is imminent — auto-active, commanded, or already asked — and the loop's own deadline bounds the agent-facing wait).
![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: 58f6f86ecf
ℹ️ 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.
|
When an agent declines a finding with 'crq decline --resolve', the bot
replies either conceding ('I'm withdrawing this finding') or contesting
('I'm retaining the finding: ...'). crq dropped resolved threads wholesale,
so a valid rebuttal vanished and the loop converged over an unaddressed
disagreement — exactly how the real #30 review caught a bug I'd wrongly
declined.
Now the feedback pass reads that reply: dialect gains the verdict
classifiers IsReviewFindingWithdrawn/IsReviewFindingRetained (golden-pinned
with CodeRabbit's actual concede/contest messages from PR #30), and
feedback.go re-surfaces a resolved thread whose latest comment is a bot
reply following the agent's own comment and not a clear withdrawal, as a
'review_reply' finding. Ambiguous replies surface too — never bury a
possible rebuttal on a false concession. The loop then holds until the
agent re-addresses it (fix, or decline again convincingly enough that the
bot withdraws).
Codex auto-reviewed PR #30 and caught three real edges: - readQuota/RefreshQuota wiped a still-active account block when a calibration probe was inconclusive (no fresh reply): after CalibrationTTL the block vanished and Pump could fire queued reviews inside the original blocked window. The refresh now carries a live block forward until a conclusive reply lands. - The loop completed a fired/reviewing round on findings-return even when a required (or dynamically-gating) bot was still pending — so a Codex finding posted before CodeRabbit's review released the slot and stopped observing CodeRabbit's review/rate-limit/timeout, losing evidence. It now leaves the round active while any reviewer is pending, matching hold-the-head; the co-review wait deadline bounds it. - sweepReviewing's CAS lacked the sameRound(Seq+Head) identity guard the other mutations use, so a supersede between observe and write could apply an old head's Progress to a replacement round. Guarded. TestLoopResumesAwaitingFeedbackWithoutRefiring updated: a round with a pending dynamically-gating Codex now correctly stays active (not completed), still without re-firing.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f673edee02
ℹ️ 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".
…t anchors The rebuttal reader's first live run surfaced Codex's platform boilerplate as a false rebuttal; its 'create an environment' notice is now classified non-actionable in dialect (corpus-pinned) and threadRebuttal skips non-actionable replies generally. Codex's round-four findings, all real: - Codex-only resolutions (dedupe, FireCodexOnly, co-review wait) spend no CodeRabbit quota, so DecideFire resolves them BEFORE the account-block and pacing gates — a block from another PR no longer delays them. - The self-heal Codex post claims the round under CAS (CodexClaimedAt, 2-minute TTL) before the network call, so two unserialized sweepers can no longer double-post; the claim is released on record or failure. Codex flagged this race three times across rounds — it was right that the observation-guard alone left a real window. - A reviewing round that would re-acknowledge now returns KeepWaiting, so a silent co-bot wait no longer writes identical state and re-syncs the dashboard on every pump. - AwaitCoReview anchors at the adopted @codex command's time, not the observation time, so a SHA-less legacy clean summary posted before the pump still counts toward completion. - A PR closed while parked in awaiting_retry is abandoned on the next pump (sweepParkedClosed) instead of surviving its cooldown window. - threadRebuttal only trusts complete threads: the reviewThreads query carries totalCount and a partially-fetched thread is skipped rather than judged on a stale mid-thread reply.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/codex_replay_test.go`:
- Around line 535-537: Strengthen the assertion after f.round(repo, pr) to
verify the closed round is archived rather than deleted: confirm State.Archive
contains this round’s head with PhaseAbandoned. Retain the existing nil-result
check, and use the round’s existing head and archive symbols to validate the
one-round persistence invariant.
In `@internal/crq/feedback.go`:
- Around line 885-890: Move the unknown-to-major severity flooring from the
orchestration code around SeverityOf into internal/dialect. Add or reuse
dialect-owned severity constants or a FloorSeverity helper, then update the
caller to use that API instead of comparing or assigning literal severity
strings; preserve the existing behavior for all other severities.
- Around line 868-875: Update the agentReplied detection loop in the feedback
evaluation flow to require a bot → non-bot → bot sequence, rather than marking
any non-bot comment as an agent reply. Track the preceding and following comment
authors using dialect.InBots, and only set agentReplied when the non-bot comment
is immediately preceded and followed by bot comments before the existing
!agentReplied handling.
In `@internal/crq/service.go`:
- Around line 1437-1468: Update sweepParkedClosed to avoid repeatedly inspecting
only the oldest open awaiting_retry round; rotate the candidate across pumps or
continue scanning parked rounds until a closed PR is found. Preserve the
one-pull-per-pump constraint and abandon the closed round via abandonRound with
the existing skipped outcome.
- Around line 904-916: Update fireCodexReview and its associated finalization
logic to validate the round with sameRound rather than only comparing Head,
preventing stale results from updating a replacement round with a different Seq.
Clear CodexClaimedAt only when codexID indicates a successful post; retain the
claim after failure so codexClaimTTL governs retries.
In `@internal/engine/codex.go`:
- Around line 85-110: Ensure latestCodexEvidence keeps prev strictly earlier
than latest by ignoring evidence timestamps equal to the current latest or prev
when updating the candidates. Add a regression test covering review and
clean-summary evidence with the same timestamp and a command at that timestamp,
verifying CodexAutoActive classifies the review as commanded.
🪄 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
Run ID: 2a4a080f-5fb1-4008-9980-664e1647fd79
📒 Files selected for processing (23)
AGENTS.mdREADME.mdinternal/crq/codex_replay_test.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/observe.gointernal/crq/replay_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/dialect/codex.gointernal/dialect/common.gointernal/dialect/golden_test.gointernal/dialect/reply.gointernal/dialect/testdata/coderabbit/reply-retained.mdinternal/dialect/testdata/coderabbit/reply-withdrawn.mdinternal/dialect/testdata/codex/environment-notice.mdinternal/engine/codex.gointernal/engine/engine_test.gointernal/engine/fire.gointernal/engine/progress.gointernal/state/state.gointernal/state/state_test.gollms.txt
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
internal/dialect/testdata/{coderabbit,codex}/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When adding support for a new bot-message format, add one corpus file containing the real message under the appropriate dialect testdata directory.
Files:
internal/dialect/testdata/codex/environment-notice.mdinternal/dialect/testdata/coderabbit/reply-withdrawn.mdinternal/dialect/testdata/coderabbit/reply-retained.md
internal/dialect/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep all bot-text knowledge, literal bot wording, classifiers, parsers, SHA/severity vocabulary, and bot-event definitions in
internal/dialect; do not place bot wording in engine, state, or crq.
Files:
internal/dialect/reply.gointernal/dialect/golden_test.gointernal/dialect/common.gointernal/dialect/codex.go
internal/{dialect,engine,state,crq,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Preserve the dependency direction
dialect � engine � crq,state � crq, andgh � {state, crq}; do not introduce dependency cycles. The engine must perform no I/O.
Files:
internal/dialect/reply.gointernal/dialect/golden_test.gointernal/engine/fire.gointernal/engine/codex.gointernal/dialect/common.gointernal/engine/progress.gointernal/crq/codex_replay_test.gointernal/dialect/codex.gointernal/crq/replay_test.gointernal/crq/observe.gointernal/state/state_test.gointernal/state/state.gointernal/engine/engine_test.gointernal/crq/service_test.gointernal/crq/feedback.gointernal/crq/service.gointernal/crq/feedback_test.go
internal/dialect/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
For each new bot-message format, add one row to
TestGoldenClassificationingolden_test.go; that row is the classification specification.
Files:
internal/dialect/golden_test.go
internal/engine/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep decision logic pure: pass
nowexplicitly, avoid context and GitHub dependencies, and centralize fire eligibility inDecideFire; useProgressfor round transitions andCompletionfor determining whether a round is done.
Files:
internal/engine/fire.gointernal/engine/codex.gointernal/engine/progress.gointernal/engine/engine_test.go
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep orchestration in
internal/crq: observation belongs inobserve.go, effects belong inservice.go, and no code outside the apply path may post the review command.DryRunmust report without writing.
Files:
internal/crq/codex_replay_test.gointernal/crq/replay_test.gointernal/crq/observe.gointernal/crq/service_test.gointernal/crq/feedback.gointernal/crq/service.gointernal/crq/feedback_test.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Maintain oneRoundper PR, never delete rounds, and represent lifecycle changes only through validatedRoundtransition methods; archive superseded rounds rather than deleting them.
Preserve the globalFireSlotinvariant of at most one concurrent fire fleet-wide, and release the slot on bot acknowledgement while keeping the round open inreviewing.
Files:
internal/state/state_test.gointernal/state/state.go
internal/engine/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Table-test every convergence and fire rule in the engine test suite, including rules consuming dialect classifications.
Files:
internal/engine/engine_test.go
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: kristofferR/coderabbit-queue
Timestamp: 2026-07-17T22:31:52.909Z
Learning: Keep the observe � decide � apply flow intact: observe builds one `engine.Observation`, engine performs pure decisions, and crq applies CAS state writes and GitHub comments.
🪛 markdownlint-cli2 (0.23.0)
internal/dialect/testdata/codex/environment-notice.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/reply-withdrawn.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
internal/dialect/testdata/coderabbit/reply-retained.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (23)
AGENTS.md (1)
1-37: LGTM!Also applies to: 38-60, 61-83, 84-98
README.md (1)
405-410: LGTM!Also applies to: 429-429, 452-460
llms.txt (1)
64-67: LGTM!Also applies to: 95-100
internal/dialect/codex.go (1)
51-56: LGTM!internal/dialect/common.go (1)
123-156: LGTM!internal/dialect/golden_test.go (1)
70-72: LGTM!Also applies to: 205-227
internal/dialect/reply.go (1)
1-39: LGTM!internal/dialect/testdata/coderabbit/reply-retained.md (1)
1-1: LGTM!internal/dialect/testdata/coderabbit/reply-withdrawn.md (1)
1-1: LGTM!internal/dialect/testdata/codex/environment-notice.md (1)
1-1: LGTM!internal/state/state.go (1)
65-69: LGTM!Also applies to: 258-286
internal/state/state_test.go (1)
147-190: LGTM!internal/crq/replay_test.go (1)
186-189: LGTM!internal/crq/codex_replay_test.go (1)
4-4: LGTM!Also applies to: 292-505, 540-569
internal/engine/codex.go (1)
126-144: LGTM!internal/engine/fire.go (1)
17-24: LGTM!Also applies to: 72-80, 116-133
internal/engine/progress.go (1)
64-74: LGTM!Also applies to: 85-89, 170-180
internal/engine/engine_test.go (1)
178-218: LGTM!Also applies to: 445-451, 483-493, 588-617
internal/crq/observe.go (1)
58-62: LGTM!internal/crq/service.go (1)
214-221: LGTM!Also applies to: 431-434, 462-463, 662-662, 744-744, 777-835, 1024-1040, 1307-1310, 1390-1394
internal/crq/feedback.go (1)
135-140: LGTM!Also applies to: 363-369, 741-742, 793-793, 858-863, 891-903
internal/crq/service_test.go (1)
1813-1849: LGTM!internal/crq/feedback_test.go (1)
1529-1538: LGTM!Also applies to: 1886-1967
… rotation All seven findings from the fifth self-hosted round were valid: - fireCodexReview finalizes under the sameRound guard (a same-head replacement round with a new Seq no longer inherits an old post's result), and a FAILED post keeps its claim — the claim TTL is the retry backoff, so clearing it early bypassed codexClaimTTL. - threadRebuttal requires the strict bot-finding → agent-reply → bot last-word shape: a human-started thread a bot merely answered no longer fabricates a contested finding. - The severity floor moved into dialect (FloorSeverity) — no severity literals in orchestration code. - sweepParkedClosed rotates its inspected candidate across pumps so one long-cooldown open PR cannot starve the closed-PR check for every parked round behind it. - latestCodexEvidence keeps prev strictly older than latest, so a review and its clean summary in the same second don't collapse the command window and misclassify a commanded review as automatic. - The parked-closed replay asserts the abandoned round lands in the Archive (never-delete invariant), not merely leaves Rounds. - Codex's second boilerplate variant (create account / connect) joins the dialect notice corpus.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c0c15077
ℹ️ 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".
- Pump no longer returns blocked/min_interval before observe+DecideFire: the engine resolves quota-free verdicts (dedupe, FireCodexOnly, FireCoReviewWait) ahead of those gates, and mapFireNo still reports the same actions for real fires. The early return was re-adding the gate the engine had deliberately moved. - The co-review wait anchor falls back to the primary bot's head-review time when no @codex command exists, so a pre-pump legacy clean summary from auto-review counts instead of hiding until the deadline. - The adopt path takes the Codex self-heal claim in the same CAS that records the fire — the fired state is never visible unclaimed. - The poisoned-marker repair keeps a completed round whose primary review is real but whose co-bot is pending (a deliberate dedupe), instead of deleting it into ack-and-dedupe churn.
Bots deliver in waves: Codex auto-reviews a pushed head minutes after the push, and CodeRabbit's real review can trail its comment shells. The loop exited 0 on the first converged observation, so a wave landing moments later was found by a human re-checking the PR instead of by crq — exactly the failure this tool exists to prevent. The loop now records when it first observes convergence and keeps polling for CRQ_SETTLE (default 90s): anything new inside the window flips the verdict back to the normal findings/pending flow, and only a quiet window exits 0. The wait deadline is suppressed while settling (the settle is its own bound). Replay scenarios pin both directions; scenario configs disable the window where they assert immediate verdicts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f1105bb59
ℹ️ 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".
- Update the README and llms.txt setup snippets to the v3 default state ref crq-state-v3, so following the docs no longer bypasses the protective default and risks a v3 client overwriting a v2 payload on the old crq-state ref. - Ignore a `@codex review` command Codex already answered with a review when gathering adoptable Codex commands: on a regular push whose commit date predates that consumed command, the command survives the cutoff, and treating it as live suppressed the Codex command the new head still needed. - Record the adopted Codex command id in the adopt fire's CAS write when crq is not posting Codex, so the self-heal scan (anchored on FiredAt) never re-posts a Codex command that was posted before the adopted CodeRabbit command.
1 participant
Why
On 2026-07-16 crq posted
@coderabbitai review~19 times in one day on kristofferR/ha-adjustable-bed#448 (10+ on a single head), and the same pattern hit another repo. The daemon log showed the mechanism: a rate-limited requeue deleted the "already fired at this head" marker, so every 60s pass re-concluded "no bot review at head" and fired again. That was one instance of a general disease after ~40 consecutive one-off fixes: fire-safety emerged from six scattered guards, "is the round done?" had two divergent implementations, state was five parallel maps, and every bot-message heuristic was an inline string match.What
A phased rewrite of the core around two ideas (see AGENTS.md for the full map):
internal/state, schema v3): phases queued → reserved → fired → reviewing → awaiting_retry/completed/abandoned, with transitions as methods that error on illegal edges. Forgetting "already requested at this head" now requires destroying a record no transition destroys; the rate-limit cooldown (RetryAt) lives on the round and survives every requeue.internal/dialectturns bot text into typed events (all CodeRabbit/Codex wording lives there, pinned by a golden corpus in testdata/);internal/engineis the pure decision core —DecideFire(single fire owner),Progress,Completion(the one round-done answer) — shared by the daemon andcrq loop;internal/crqorchestrates with a singleobserve()fetch point and one effects boundary (dry-run = decide without apply, the mirrored dry-run branch is gone).internal/ghis the transport with GitHub REST quota renamed Throttle so it can never again be conflated with CodeRabbit's account quota.internal/crq/replay_test.gore-enacts the incident end-to-end with an injected clock: rate-limit bounce over a full 40-minute window fires exactly once; the instant "Review finished" ack cannot converge a first review; the in-progress summary releases the slot without closing the round; force-push supersedes without stale command adoption; the full day-sequence produces exactly 3 commands where v2 produced ~19.Notes
crq-state-v3with loud auto-reinit on unknown payloads — no migration, crq is pre-release.launchctl bootout+bootstrapthe daemon, watch one autoreview pass, run one realcrq loopto convergence.Summary by CodeRabbit
CRQ_CODEX_CMD) and richer dialect parsing for review findings.debug refreshnow reports the account value instead of blocked state.Added in review: Codex firing + auto-detection + dynamic gating
crq now drives Codex too (
2c4aefb): with Codex inCRQ_REQUIRED_BOTSit postsCRQ_CODEX_CMD(default@codex review) in the same fire step — never when it detects Codex auto-review on the PR (any Codex review no command preceded). A Codex that joins a round uninvited gates that round's convergence dynamically; its usage-limit notice releases the dynamic gate. End-to-end scenarios incodex_replay_test.gocover all three modes.