Let an agent account for a finding GitHub cannot close#48
Conversation
crq resolve and crq decline both act on a review thread. A review-body finding, a review-skipped notice and an outside-diff remark have none, so neither command can touch them — and drain-first then blocks every future round on a finding that can never drain. The observed end state was a PR reporting that no review was ever requested for its current head, four rounds running: the round could not start because the finding was undrained, and the finding could not drain because nothing could act on it. crq dismiss <repo> <pr> <finding-id>... --reason "<why>" records that the agent judged it. Dismissed findings are withheld from the action, and crq next reports dismissed: N so nothing looks silently dropped. Three choices worth stating. The reason is stored, not just demanded — a dismissal that discards its justification is not auditable. Finding IDs are content-derived rather than GitHub node IDs, so the repo and PR are required to identify one. And the record is scoped to the round for the current head: the same content yields the same ID, so a dismissal that outlived its head would silently swallow the finding when the next reviewer reports it again. Pushing supersedes the round and clears it, which is the rule body findings already follow. Dismissing enqueues the PR when crq is not yet tracking the current head, because that is the deadlock's own signature — no round for the head at all — and a dismissal with nowhere to live would change nothing.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds ChangesThreadless Finding Dismissal
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant crq CLI
participant Service.Dismiss
participant Round State
participant crq next
Agent->>crq CLI: crq dismiss repo PR finding-id --reason
crq CLI->>Service.Dismiss: parsed dismissal request
Service.Dismiss->>Round State: persist current-head dismissal
Service.Dismiss-->>crq CLI: dismissal result JSON
Agent->>crq next: request next action
crq next->>Round State: read current-head dismissals
Round State-->>crq next: dismissed finding IDs
crq next-->>Agent: findings and dismissed count
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a2d9880fa
ℹ️ 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".
Three review findings, all correct. Filtering happened in nextFromState, so a dismissal did nothing for crq feedback or crq loop: convergence and the exit code are computed from the full list, leaving the finding permanently actionable outside crq next. The filter now lives in Feedback, before Converged, so there is one of it. Nothing stopped dismissing a finding that HAS a thread. That would let a round converge with the thread still open, skipping the resolve/decline flow that puts the decision on the PR where the bot can answer it. A threaded finding is now refused by name, and so is an ID that is not a finding at this head at all — a stale copy-paste would otherwise record a dismissal that silences whatever later matches it. And the write is one CAS update again. Enqueueing first meant that a failure between the enqueue and the record left a fire-eligible round behind with nothing dismissed on it, which the autoreview daemon could claim and spend a review on. Dismiss now reads the findings, validates them, then creates or supersedes the round and records the decision in a single write.
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d60a0ec2
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dismiss.go`:
- Around line 75-111: The Dismiss flow performs its CAS write directly in
dismiss.go instead of through service.go. Extract the s.store.Update logic into
a service.go effects-executor method such as recordDismissal, have Dismiss
invoke that method, and preserve the existing round, validation, and output
behavior while keeping DryRun write-free.
- Around line 58-108: The store.Update mutation currently reuses stale
feedback.Head and current data across CAS retries. Move feedback retrieval and
finding-map construction into the Update callback using the loaded round’s head,
refresh clean IDs against that feedback, and use the refreshed head for
NewRound/Supersede and validation so retries cannot regress state or reject
against stale findings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 82b94e8d-e252-4104-98ce-f98ba0c342d5
📒 Files selected for processing (10)
README.mdcmd/crq/main.gocmd/crq/main_test.gointernal/crq/dismiss.gointernal/crq/feedback.gointernal/crq/next.gointernal/crq/next_test.gointernal/state/state.gollms.txtskills/coderabbit-queue/SKILL.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Keep
internal/crqlimited to orchestration and effects wiring;service.gois the only effects executor for CAS writes andPostIssueComment, andDryRunmust write nothing.
Files:
internal/crq/next_test.gointernal/crq/next.gointernal/crq/dismiss.gointernal/crq/feedback.go
internal/state/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/state/**/*.go: Persist schema v3 with oneRoundper PR, one globalFireSlot,AccountQuota, and anArchive; never delete rounds, and preserve unknown JSON members during round trips.
Model round lifecycle transitions asRoundmethods, reject illegal edges, and never delete a round; completed rounds remain deduplication markers, while rate-limited retries useawaiting_retrywithout deleting fired history.
Files:
internal/state/state.go
🔇 Additional comments (9)
internal/state/state.go (1)
82-95: LGTM!Also applies to: 622-640
internal/crq/next_test.go (1)
5-5: LGTM!Also applies to: 306-424
internal/crq/feedback.go (1)
28-32: LGTM!Also applies to: 435-448
internal/crq/next.go (1)
34-37: LGTM!Also applies to: 222-225
cmd/crq/main.go (1)
214-241: LGTM!Also applies to: 350-350, 373-374, 528-546, 788-811
cmd/crq/main_test.go (1)
115-152: LGTM!README.md (1)
287-287: LGTM!Also applies to: 388-388, 602-605
llms.txt (1)
51-51: LGTM!Also applies to: 203-219
skills/coderabbit-queue/SKILL.md (1)
28-28: LGTM!Also applies to: 172-193
Six review findings, all correct, and three of them P1. A round tracking a different head is now refused rather than superseded. If the head moved after Dismiss read the findings, superseding archives the live round and points the queue back at a commit nobody is looking at — and CAS retries cannot protect against a mutation that deliberately overwrites newer state. A round may only be CREATED when this call drains the head. Dismissing one of several open findings used to queue a fire-eligible round that DecideFire cannot hold back, because it never sees findings — so a pump could spend the primary's quota on code the caller is still expected to fix. A finding carried from an older commit is refused. IDs hash the text, not the commit, so recording one against the current head would silently filter the identical finding when the current reviewer reports it. And "no thread ID" turns out not to be the question. When the GraphQL thread query fails, Feedback falls back to REST, which does not return thread IDs at all, so an inline comment with an open thread arrives looking threadless. Only sources that intrinsically cannot have one — review bodies, prompt blocks, skip notices, issue comments — are dismissible now. The CAS write moves to service.go as recordDismissal, which is where AGENTS.md says every write belongs, and it honours DryRun.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c401ac3ded
ℹ️ 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".
Three more findings, one P1. The eligibility guard asked whether a round object existed, when what matters is whether a FIRE-ELIGIBLE one would. A queued round is exactly as dangerous as a newly created one: Pump can hand it to DecideFire, which sees no findings and cannot enforce drain-first, so dismissing one of several open findings could still buy a review of code the caller is still fixing. Repeating a dismissal failed on its own earlier success: Feedback filters the dismissed ID out, so validating against the current findings alone rejected it as unknown. The command is documented as idempotent and an interrupted agent repeating itself is the ordinary case, so an ID this round already dismissed is accepted and reported as such. And the filter now checks the SOURCE, not just the ID. Finding IDs hash the text, not where it came from, so a dismissed body finding later delivered as an inline comment through the REST fallback hashes the same — and an ID-only filter would hide a review thread that is open.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1f63aa558
ℹ️ 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.
|
Three findings, one P1 in the fix itself. After a push the stored round is still on the PREVIOUS head, because `crq next` returns on a current-head finding before it enqueues. Treating that as a concurrent move rejected every dismissal and left the new head in the exact drain-first deadlock this command exists to end. The two cases are told apart by when the round was enqueued: after the findings were read means somebody moved the PR forward and the decision is stale; otherwise it is the ordinary post-push round, and it is superseded. A finding another agent dismissed concurrently now counts as handled when deciding whether other work is still open, instead of making this call refuse over a finding that is already accounted for. And the docs no longer send an agent to a command that will refuse it: a `review_comment` finding lost its thread ID to the REST fallback, so it is neither resolvable nor dismissible until crq can read review threads again.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5c2e86c24
ℹ️ 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".
Three findings, one P1 in the guard I added last round. FireEligible answers "right now", which is the wrong question when deciding whether a partial dismissal would leave a round able to buy a review. A round cooling in awaiting_retry is not eligible this second and becomes eligible the moment RetryAt passes, so letting the dismissal through on that technicality just deferred the hazard to the cooldown expiring. The guard asks whether the round will ever fire again. State alone could not tell "no round for this head" from "the head moved and nothing has enqueued it yet", so a push landing between Feedback and the write could record the dismissal against the wrong commit. The head is re-read once before recording. And the README now carries the same caveat as the skill: a review_comment finding lost its thread ID to the REST fallback and still has an open thread, so it is neither resolvable nor dismissible until crq can read threads again.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3da74ebde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Roadmap item D — the #4 annoyance in the transcript sweep, and the one with the most damning
observed end state: "No review was ever requested for
7a16eeb." Four rounds, one PR, no review.The deadlock
crq resolveandcrq declineboth act on a review thread. These findings have none:review_skippednotice,Drain-first says a round may not start while findings for the current head are undrained. So the
round cannot start because the finding is undrained, and the finding cannot drain because nothing
can act on it.
crq nextreturnsfixforever and no review is ever requested for the head.crq declinewas already tightened to resolve by default for exactly this reason (issue #8). Thatfixed the threaded case and left this one untouched.
The lever
Dismissed findings are withheld from the action, so the round moves on.
crq nextreportsdismissed: N— a set-aside finding must not look like one that was never reported.Three choices worth arguing about:
The reason is stored, not just demanded. A dismissal that discards its justification is not
auditable, so
Round.Dismissedmaps ID → reason rather than being a list of IDs.Finding IDs, not node IDs.
.findings[].idis a hash of the finding's own content, so unlike athread ID it is not globally unique — the repo and PR genuinely identify something here and are
required.
Scoped to the current head. The same content yields the same ID, so a dismissal that outlived
its head would silently swallow the finding when the next reviewer reports it again. Superseding the
round drops it, which is the rule body findings already follow: the current reviewer must report it
again.
And dismissing enqueues the PR when crq is not yet tracking the current head. That is deliberate:
no-round-for-the-head is the deadlock's own signature, so a dismissal with nowhere to live would
change nothing.
Verification
TestDismissEndsTheUnresolvableFindingDeadlockwalks the whole shape: a body finding lands with nothread,
fixrepeats across two calls, dismissing ends it, the finding is withheld,dismissedis1, nothing was posted by the dismissal itself, and a push clears the record.
TestParseDismissArgscovers both--reasonforms, a missing value, an empty reason, several IDs,and an unknown flag — a typo like
--resaonmust fail rather than silently become a finding ID.gofmt -l .clean,go vet ./...clean,go test ./... -count=1green.Local preflight returned 7 findings (all minor/trivial); all 7 are applied — target validation now
reuses
repoPRso a non-positive PR fails as it does everywhere else, the post-dismiss assertion isexact rather than decrease-only, and the docs distinguish narrowing a PR (which fixes a skipped
review's cause) from dismissing (which only records the decision).
Ref #42
Summary by CodeRabbit
New Features
crq dismissfor dismissing findings without GitHub review threads, with a required reason.Documentation