From 4a2d9880fa1020a8d1ba0f64f3c766bf72550564 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:06:19 +0200 Subject: [PATCH 01/12] Let an agent account for a finding GitHub cannot close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ... --reason "" 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. --- README.md | 7 ++- cmd/crq/main.go | 74 +++++++++++++++++++++++++ cmd/crq/main_test.go | 38 +++++++++++++ internal/crq/dismiss.go | 93 ++++++++++++++++++++++++++++++++ internal/crq/next.go | 22 +++++++- internal/crq/next_test.go | 84 +++++++++++++++++++++++++++++ internal/state/state.go | 33 ++++++++++++ llms.txt | 17 +++++- skills/coderabbit-queue/SKILL.md | 20 ++++++- 9 files changed, 384 insertions(+), 4 deletions(-) create mode 100644 internal/crq/dismiss.go diff --git a/README.md b/README.md index f9d091a..8710952 100644 --- a/README.md +++ b/README.md @@ -284,7 +284,7 @@ Read `.action`, do exactly that, call it again. | `.action` | what to do | |---|---| -| `fix` | fix `.findings[]`, validate, then `crq resolve` (or `crq decline`) each thread | +| `fix` | fix `.findings[]`, validate, then `crq resolve` (or `crq decline`) each thread; `crq dismiss` one with no thread | | `hold` | **do not commit or push** — a required reviewer hasn't answered for this head; call again at `.recheck_after` | | `push` | the head is released — commit and push your fixes once | | `wait` | nothing to do until `.recheck_after` | @@ -385,6 +385,7 @@ crq loop # blocking one-shot round: fire + wait + emit JSON fin crq feedback # current normalized findings as JSON, WITHOUT triggering a review crq resolve [...] # resolve addressed review threads crq decline [...] --reason "" [--resolve] # record why a finding is declined +crq dismiss [...] --reason "" # account for a finding with no thread crq autoreview # ⭐ review ALL open PRs automatically, rate-coordinated # (--no-incremental = first review only; --once = single pass for cron) crq status # show the dashboard: queue, in-flight, quota, next slot @@ -598,6 +599,10 @@ If you're an autonomous agent running a PR-review loop, here's everything you ne - **Resolve / decline:** after fixing a finding, `crq resolve ...` (pass them all at once). If you're declining one, `crq decline --reason "…"` — that resolves it too, because a thread left open keeps its finding actionable; `--keep-open` overrides. +- **Dismiss what has no thread:** a finding with no `thread_id` cannot be resolved or declined, and + blocks every future round until it is accounted for: `crq dismiss + --reason "…"`. It covers the current head only. For a skipped review, narrowing the PR fixes the + cause; dismissing only records that you chose to proceed. - **Don't narrate the wait.** Report real state changes — findings, a push, convergence, a block — not elapsed time. - **Setup check:** run `crq doctor`; if config is missing, do the Quick Start (install + `crq init`). diff --git a/cmd/crq/main.go b/cmd/crq/main.go index f83f08a..af8566a 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -211,6 +211,34 @@ func run(ctx context.Context, args []string) int { } printJSON(result) return 0 + case "dismiss": + rest, reason, ok := parseDismissArgs(args[1:]) + if !ok || strings.TrimSpace(reason) == "" { + fatal(errors.New(`usage: crq dismiss [...] --reason ""`)) + return 1 + } + if len(rest) < 3 { + fatal(errors.New(`usage: crq dismiss [...] --reason ""`)) + return 1 + } + // Same target validation as every other command, so a non-numeric or + // non-positive PR fails the same way here as there. + repo, pr, ok := repoPR(rest[:2]) + if !ok { + fatal(fmt.Errorf("bad target %q %q (usage: crq dismiss ... --reason \"\")", rest[0], rest[1])) + return 1 + } + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + result, derr := service.Dismiss(ctx, repo, pr, rest[2:], reason) + if derr != nil { + fatal(derr) + return 1 + } + printJSON(result) + return 0 case "autoreview", "auto": fs := flag.NewFlagSet("autoreview", flag.ContinueOnError) fs.SetOutput(os.Stderr) @@ -319,6 +347,7 @@ DRIVING A PR REVIEW Call crq next, do exactly what .action says, call it again. That is the whole loop. fix fix .findings[], validate, then crq resolve (or crq decline) each thread + (no .thread_id? crq dismiss it once judged — at this head nothing else can) hold do NOT push: a required reviewer is pending; call again at .recheck_after push the head is released — commit and push your fixes once wait nothing to do; call again at .recheck_after @@ -341,6 +370,8 @@ USAGE crq decline [...] --reason "" [--keep-open] reply on a thread to record why a finding is declined (resolves it; --keep-open leaves it open) + crq dismiss [...] --reason "" + account for a finding GitHub gives you no thread to close crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -494,6 +525,25 @@ replies contesting the decline, crq re-surfaces that reply as its own finding. Pass --keep-open to leave it unresolved anyway (an on-the-record disagreement you intend to keep working). Thread IDs come from .findings[].thread_id. +`) + case "dismiss": + fmt.Print(`crq dismiss [...] --reason "" + +Record that you have accounted for a finding that has no review thread, so it +stops blocking the round. + +crq resolve and crq decline both act on a thread. A review-body finding, a +review-skipped notice or an outside-diff remark has none, so neither command can +touch it — and a finding that can never drain blocks every future round, leaving +a PR whose current head no review was ever requested for. + +Finding IDs come from .findings[].id. They are content-derived, not GitHub node +IDs, so the repo and PR are required. A dismissal covers the current head only: +push, and the next reviewer has to report it again. + +Use it for a finding you have judged and set aside. Fix what is real instead — and +for a review crq was told was SKIPPED, narrowing the PR fixes the cause, while +dismissing only records that you decided to live with it at this head. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] @@ -735,6 +785,30 @@ func parseResolveArgs(args []string) ([]string, bool) { return threads, ok } +// parseDismissArgs splits `crq dismiss ...` from its --reason. +// Unlike a thread ID, a finding ID is not globally unique — it is a hash of the +// finding's own text — so the repo and PR genuinely identify something here and +// are required. +func parseDismissArgs(args []string) (rest []string, reason string, ok bool) { + for i := 0; i < len(args); i++ { + switch arg := args[i]; { + case arg == "--reason": + if i+1 >= len(args) { + return nil, "", false + } + reason = args[i+1] + i++ + case strings.HasPrefix(arg, "--reason="): + reason = strings.TrimPrefix(arg, "--reason=") + case strings.HasPrefix(arg, "-"): + return nil, "", false // a typo must fail, not become a finding ID + default: + rest = append(rest, arg) + } + } + return rest, reason, true +} + func parseDeclineArgs(args []string) (threads []string, reason string, resolve, ok bool) { return parseThreadCommand(args, true) } diff --git a/cmd/crq/main_test.go b/cmd/crq/main_test.go index a7930cb..3690e29 100644 --- a/cmd/crq/main_test.go +++ b/cmd/crq/main_test.go @@ -112,3 +112,41 @@ func TestUnknownFlag(t *testing.T) { t.Errorf("unknownFlag = (%q, %v), want (--wiat, true)", bad, found) } } + +// parseDismissArgs decides what counts as a finding ID, so a typo must not +// quietly become one. +func TestParseDismissArgs(t *testing.T) { + for _, tc := range []struct { + name string + args []string + rest []string + reason string + ok bool + }{ + {"separate reason", []string{"o/r", "7", "abc", "--reason", "why"}, []string{"o/r", "7", "abc"}, "why", true}, + {"equals reason", []string{"o/r", "7", "abc", "--reason=why"}, []string{"o/r", "7", "abc"}, "why", true}, + {"several ids", []string{"o/r", "7", "a", "b", "--reason", "why"}, []string{"o/r", "7", "a", "b"}, "why", true}, + {"reason first", []string{"--reason", "why", "o/r", "7", "a"}, []string{"o/r", "7", "a"}, "why", true}, + {"missing value", []string{"o/r", "7", "a", "--reason"}, nil, "", false}, + {"unknown flag", []string{"o/r", "7", "a", "--resaon", "why"}, nil, "", false}, + {"empty reason", []string{"o/r", "7", "a", "--reason", ""}, []string{"o/r", "7", "a"}, "", true}, + } { + t.Run(tc.name, func(t *testing.T) { + rest, reason, ok := parseDismissArgs(tc.args) + if ok != tc.ok || reason != tc.reason { + t.Fatalf("got rest=%v reason=%q ok=%v, want reason=%q ok=%v", rest, reason, ok, tc.reason, tc.ok) + } + if !tc.ok { + return + } + if len(rest) != len(tc.rest) { + t.Fatalf("rest = %v, want %v", rest, tc.rest) + } + for i := range rest { + if rest[i] != tc.rest[i] { + t.Fatalf("rest = %v, want %v", rest, tc.rest) + } + } + }) + } +} diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go new file mode 100644 index 0000000..d39025c --- /dev/null +++ b/internal/crq/dismiss.go @@ -0,0 +1,93 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "strings" +) + +// DismissResult reports what a dismissal did, per finding ID. +type DismissResult struct { + Repo string `json:"repo"` + PR int `json:"pr"` + Head string `json:"head"` + // Dismissed lists the IDs recorded by this call; Already lists the ones this + // round had already accounted for. Repeating a dismissal is not an error — + // an agent re-reading its own findings should not have to remember. + Dismissed []string `json:"dismissed"` + Already []string `json:"already_dismissed,omitempty"` + Reason string `json:"reason"` +} + +// Dismiss records that an agent has accounted for findings GitHub gives it no +// way to close. +// +// `crq resolve` and `crq decline` both act on a review thread. A review-body +// finding, a review-skipped notice or an outside-diff remark has none, so +// neither command can touch it — and drain-first then blocks every future round +// on a finding that can never drain. The observed end state was a PR where "no +// review was ever requested" for the current head, four rounds running. +// +// The dismissal is recorded against the round for the CURRENT head, enqueueing +// the PR if crq was not already tracking that head. That is deliberate: the +// deadlock's signature is that no round exists for the head at all, so a +// dismissal with nowhere to live would change nothing. +func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string, reason string) (DismissResult, error) { + reason = strings.TrimSpace(reason) + if reason == "" { + return DismissResult{}, errors.New("a dismissal needs a reason") + } + clean := make([]string, 0, len(ids)) + for _, id := range ids { + if id = strings.TrimSpace(id); id != "" { + clean = append(clean, id) + } + } + if len(clean) == 0 { + return DismissResult{}, errors.New("no finding id given") + } + + // Ensure a round exists for the head this dismissal is about. + if _, err := s.Enqueue(ctx, repo, pr); err != nil { + return DismissResult{}, err + } + head, open, err := s.pullHead(ctx, repo, pr) + if err != nil { + return DismissResult{}, err + } + if !open { + return DismissResult{}, fmt.Errorf("%s#%d is closed", repo, pr) + } + + out := DismissResult{Repo: repo, PR: pr, Head: head, Reason: reason, Dismissed: []string{}} + if _, err := s.store.Update(ctx, func(st *State) error { + round := st.Round(repo, pr) + if round == nil { + return fmt.Errorf("%s#%d is not tracked", repo, pr) + } + // Guard the race the enqueue above cannot close: a push between the + // enqueue and here means this dismissal is about findings from a head + // nobody is looking at any more. + if !strings.HasPrefix(head, round.Head) { + return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, head[:min(9, len(head))]) + } + out.Dismissed = out.Dismissed[:0] + out.Already = nil + for _, id := range clean { + if round.Dismiss(id, reason) { + out.Dismissed = append(out.Dismissed, id) + } else { + out.Already = append(out.Already, id) + } + } + st.PutRound(*round) + return nil + }); err != nil { + return DismissResult{}, err + } + if s.log != nil && len(out.Dismissed) > 0 { + s.log.Printf("%s#%d dismissed %d finding(s) at %s: %s", repo, pr, len(out.Dismissed), out.Head, reason) + } + return out, nil +} diff --git a/internal/crq/next.go b/internal/crq/next.go index d72155a..cb903af 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -31,6 +31,10 @@ type NextReport struct { // Pending lists the required reviewers with no evidence for this head. Pending []string `json:"pending,omitempty"` Findings []dialect.Finding `json:"findings"` + // Dismissed counts the findings this head's round has accounted for through + // `crq dismiss`. They are withheld from Findings, so the count is how a + // caller sees that something was set aside rather than never reported. + Dismissed int `json:"dismissed,omitempty"` ReviewedBy map[string]bool `json:"reviewed_by,omitempty"` // LocalWork records whether crq saw changes the PR head does not have. It @@ -215,10 +219,26 @@ func (s *Service) nextFromState(ctx context.Context, repo string, pr int) (NextR report.LocalWork, report.LocalWorkReason = s.checkLocalWork(ctx, []string{repo, feedback.HeadRepo}, report.Head, feedback.HeadRef) + // A dismissed finding is one the agent accounted for and GitHub offers no way + // to close. Withholding it here is what lets the round move on; leaving it in + // would repeat `fix` forever, which is the deadlock dismissal exists to end. + findings := feedback.Findings + if round != nil && round.Head == feedback.Head && len(round.Dismissed) > 0 { + kept := make([]dialect.Finding, 0, len(findings)) + for _, f := range findings { + if round.IsDismissed(f.ID) { + report.Dismissed++ + continue + } + kept = append(kept, f) + } + findings = kept + } + in := engine.NextInput{ Obs: engine.Observation{Head: feedback.Head, Open: feedback.Open}, Completion: engine.CompletionStatus{ReviewedBy: feedback.ReviewedBy, Done: allReviewed(feedback.ReviewedBy)}, - Findings: feedback.Findings, + Findings: findings, Global: s.global(st, now), Primary: s.cfg.Bot, LocalWork: report.LocalWork, diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 3599d7a..3ab29d7 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -302,3 +302,87 @@ func TestNextResolvesSummaryOnlyWithoutTheQueue(t *testing.T) { t.Fatalf("the front PR must keep the fire slot, got %#v", st.FireSlot) } } + +// A finding with no review thread cannot be resolved or declined — GitHub offers +// nothing to act on — so drain-first blocks every future round on it. The +// observed end state was a PR reporting "no review was ever requested" for its +// current head, four rounds running. `crq dismiss` is the only way out, and this +// pins both halves: that the deadlock is real, and that dismissing ends it. +func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + repo, pr := "owner/repo", 505 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + + // The review lands as a BODY finding: no inline comment, so no thread. + f.clk.advance(2 * time.Minute) + f.gh.mu.Lock() + r := ghapi.Review{ID: 900, CommitID: head, State: "COMMENTED", SubmittedAt: f.clk.now(), + Body: corpusMessage(t, "coderabbit/findings-outside-diff.md")} + r.User.Login = f.bot + key := fakeKey(repo, pr) + f.gh.reviews[key] = append(f.gh.reviews[key], r) + f.gh.mu.Unlock() + + report := f.next(repo, pr) + f.wantAction(report, engine.ActionFix) + if len(report.Findings) == 0 { + t.Fatal("the body finding must be reported before it can be dismissed") + } + var id string + for _, finding := range report.Findings { + if finding.ThreadID == "" { + id = finding.ID + break + } + } + if id == "" { + t.Fatal("this test is only meaningful for a finding with no thread") + } + + // It repeats forever: nothing the caller can do clears it. + f.clk.advance(2 * time.Minute) + f.wantAction(f.next(repo, pr), engine.ActionFix) + posted := f.reviewsPosted(repo, pr) + + if _, err := f.svc.Dismiss(f.ctx, repo, pr, []string{id}, "already handled in an earlier commit"); err != nil { + t.Fatal(err) + } + // Dismissing is a record, not a review: it must post nothing of its own, + // even though it enqueues so the decision has a round to live on. + if got := f.reviewsPosted(repo, pr); got != posted { + t.Fatalf("dismissing posted a review: %d -> %d", posted, got) + } + + f.clk.advance(2 * time.Minute) + after := f.next(repo, pr) + if after.Action == string(engine.ActionFix) { + t.Fatalf("a dismissed finding must stop blocking the round, got %+v", after) + } + if after.Dismissed != 1 { + t.Errorf("dismissed = %d, want 1 — the count is how a caller sees it was set aside, not lost", after.Dismissed) + } + for _, finding := range after.Findings { + if finding.ID == id { + t.Error("a dismissed finding must be withheld from findings") + } + } + // It is scoped to this head. A push supersedes the round, and the next + // reviewer reporting the same thing must be heard again. + f.clk.advance(time.Minute) + f.setHead(repo, pr, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", f.clk.now()) + f.next(repo, pr) + st, _, err := f.store.Load(f.ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round(repo, pr); round == nil || round.IsDismissed(id) { + t.Errorf("the dismissal must not outlive the head it was made for, got %#v", round) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index ede4e85..5d47454 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -79,6 +79,20 @@ type Round struct { // CodeRabbit requests must skip these. CoOnly bool `json:"co_only,omitempty"` + // Dismissed maps the ID of a finding an agent explicitly accounted for to the + // reason given. GitHub offers no way to close these: a review-body finding, a + // review-skipped notice and an outside-diff remark all lack a review thread, + // so `crq resolve` and `crq decline` have nothing to act on and drain-first + // can never be satisfied — the round repeats `fix` forever and no new review is ever + // requested for the head. + // + // Scoped to this round on purpose. Finding IDs are content-derived, so a + // dismissal that outlived the head would silently swallow the same finding + // when the next reviewer reports it again. Superseding the round drops these + // with it, which is the same rule body findings already follow: the current + // reviewer must report it again. + Dismissed map[string]string `json:"dismissed,omitempty"` + // RetryAt is the earliest time this head may fire again (awaiting_retry). RetryAt *time.Time `json:"retry_at,omitempty"` @@ -605,6 +619,25 @@ func (s *State) QueuedRounds(now time.Time) []Round { return out } +// Dismiss records a finding ID as accounted for. Returns false when it was +// already dismissed, so a caller can tell a repeat from a new decision. +func (r *Round) Dismiss(id, reason string) bool { + if id == "" || r.IsDismissed(id) { + return false + } + if r.Dismissed == nil { + r.Dismissed = map[string]string{} + } + r.Dismissed[id] = reason + return true +} + +// IsDismissed reports whether this round already accounted for the finding. +func (r *Round) IsDismissed(id string) bool { + _, ok := r.Dismissed[id] + return ok +} + // Queue-entry wait reasons. A waiting round is held by exactly one of these // (or by nothing, in which case it is next up). const ( diff --git a/llms.txt b/llms.txt index f16ddff..1589da2 100644 --- a/llms.txt +++ b/llms.txt @@ -48,7 +48,7 @@ crq next OWNER/REPO PR_NUMBER | `.action` | what to do | |---|---| -| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). Call again. | +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` clears with `crq dismiss` once judged. Call again. | | `hold` | Do NOT commit or push: a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | | `push` | The head is released. Commit and push the accumulated fixes once. Call again. | | `wait` | Nothing to do until `.recheck_after`. | @@ -200,6 +200,21 @@ convergence never hides a rebuttal you have not answered. Address it or decline reason; the loop clears it once the bot withdraws (or you stop declining and fix it). Ambiguous bot replies surface too — crq errs toward showing a possible rebuttal rather than burying it. +A finding with no `thread_id` — a review body finding, a review-skipped notice, an outside-diff +remark — has nothing for `resolve` or `decline` to act on, and would otherwise block every future +round on that PR. Judge it, then record the decision: + +```bash +crq dismiss REPO PR FINDING_ID [FINDING_ID...] --reason "why this is set aside" +``` + +`FINDING_ID` is `.findings[].id` — content-derived, not a GitHub node ID, which is why the repo and +PR are required. The dismissal covers the current head only; after a push the next reviewer must +report it again. `crq next` reports `dismissed: N` so a set-aside finding never looks lost. + +For a SKIPPED review specifically, the two remedies differ: narrowing the PR addresses why the +review was skipped, while `crq dismiss` only records that you chose to proceed at this head. + Current feedback without triggering a review: ```bash diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index cab6ef8..1de200c 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -25,7 +25,7 @@ crq next "$REPO" "$PR" | `.action` | what to do | |---|---| -| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). Call again. | +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` is cleared with `crq dismiss` once judged — at this head nothing else can. Call again. | | `hold` | Do NOT commit or push — a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | | `push` | The head is released. Commit and push the accumulated fixes once. Call again. | | `wait` | Nothing to do until `.recheck_after`. | @@ -169,6 +169,24 @@ thread left open keeps its finding actionable and `crq next` would repeat `fix` disagreement is not lost: if the bot contests the decline, crq re-surfaces that reply as its own finding. Pass `--keep-open` to leave it unresolved deliberately. +## Findings With No Thread + +Review-body findings, review-skipped notices and outside-diff remarks have no +`thread_id`. `crq resolve` and `crq decline` both act on a thread, so neither can touch them — and a +finding that can never drain blocks every future round on that PR. + +```bash +crq dismiss "$REPO" "$PR" "$FINDING_ID" --reason "why this is being set aside" +``` + +Finding IDs come from `.findings[].id`; they are content-derived, not GitHub node IDs, which is why +the repo and PR are required. A dismissal covers the current head only — push, and the next reviewer +has to report it again. `crq next` then reports `dismissed: N` so nothing looks silently dropped. + +Judge the finding first. Dismiss is for one you have decided about, not for clearing the list. When +the notice is a SKIPPED review, narrowing the PR addresses the cause; dismissing only records that +you chose to proceed at this head. + ## Fleet Auto-Review To keep all open PRs in scope reviewed while CodeRabbit native auto-review is off: From b0d60a0ec21eb175d074c08f023a403435d2c4a0 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:18:20 +0200 Subject: [PATCH 02/12] Dismiss only what has no thread, and mean it everywhere MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/dismiss.go | 73 ++++++++++++++++++++++---------- internal/crq/feedback.go | 20 ++++++++- internal/crq/next.go | 20 ++------- internal/crq/next_test.go | 36 ++++++++++++++++ llms.txt | 4 +- skills/coderabbit-queue/SKILL.md | 6 ++- 6 files changed, 117 insertions(+), 42 deletions(-) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index d39025c..9ae9e0c 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "strings" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" ) // DismissResult reports what a dismissal did, per finding ID. @@ -29,10 +31,15 @@ type DismissResult struct { // on a finding that can never drain. The observed end state was a PR where "no // review was ever requested" for the current head, four rounds running. // -// The dismissal is recorded against the round for the CURRENT head, enqueueing -// the PR if crq was not already tracking that head. That is deliberate: the -// deadlock's signature is that no round exists for the head at all, so a -// dismissal with nowhere to live would change nothing. +// A finding must be present at the current head and have no thread. A threaded +// one has resolve and decline, and both put the decision on the PR where the bot +// can answer it; dismissing it instead would converge the round with the thread +// still open. +// +// The record goes on the round for the CURRENT head, creating or superseding +// that round in the same write. That is deliberate: the deadlock's signature is +// that no round exists for the head at all, so a dismissal with nowhere to live +// would change nothing. func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string, reason string) (DismissResult, error) { reason = strings.TrimSpace(reason) if reason == "" { @@ -48,44 +55,64 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string return DismissResult{}, errors.New("no finding id given") } - // Ensure a round exists for the head this dismissal is about. - if _, err := s.Enqueue(ctx, repo, pr); err != nil { - return DismissResult{}, err - } - head, open, err := s.pullHead(ctx, repo, pr) + // Read the findings BEFORE writing anything. Two reasons: a dismissal is only + // meaningful against a finding that is actually there, and enqueueing first + // would leave a fire-eligible round behind whenever validation then failed — + // which the autoreview daemon could claim and spend a review on. + feedback, err := s.Feedback(ctx, repo, pr) if err != nil { return DismissResult{}, err } - if !open { + if !feedback.Open { return DismissResult{}, fmt.Errorf("%s#%d is closed", repo, pr) } + current := make(map[string]dialect.Finding, len(feedback.Findings)) + for _, finding := range feedback.Findings { + current[finding.ID] = finding + } - out := DismissResult{Repo: repo, PR: pr, Head: head, Reason: reason, Dismissed: []string{}} - if _, err := s.store.Update(ctx, func(st *State) error { + out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} + state, err := s.store.Update(ctx, func(st *State) error { + now := s.clock() round := st.Round(repo, pr) - if round == nil { - return fmt.Errorf("%s#%d is not tracked", repo, pr) + var err error + switch { + case round == nil: + round, err = st.NewRound(repo, pr, feedback.Head, now) + case round.Head != feedback.Head: + round, err = st.Supersede(repo, pr, feedback.Head, now) } - // Guard the race the enqueue above cannot close: a push between the - // enqueue and here means this dismissal is about findings from a head - // nobody is looking at any more. - if !strings.HasPrefix(head, round.Head) { - return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, head[:min(9, len(head))]) + if err != nil { + return err } out.Dismissed = out.Dismissed[:0] out.Already = nil for _, id := range clean { - if round.Dismiss(id, reason) { - out.Dismissed = append(out.Dismissed, id) - } else { + if round.IsDismissed(id) { out.Already = append(out.Already, id) + continue + } + finding, ok := current[id] + if !ok { + return fmt.Errorf("%s is not a finding on %s#%d at %s; re-read them with crq next", id, repo, pr, feedback.Head) } + // A threaded finding has resolve and decline, and both put the + // decision on the PR where the bot can answer it. Dismissing one + // would converge the round with the thread still open, silently + // skipping the rebuttal flow that exists for exactly this. + if finding.ThreadID != "" { + return fmt.Errorf("%s has review thread %s: resolve it or decline it, so the decision is on the PR", id, finding.ThreadID) + } + round.Dismiss(id, reason) + out.Dismissed = append(out.Dismissed, id) } st.PutRound(*round) return nil - }); err != nil { + }) + if err != nil { return DismissResult{}, err } + s.sync(ctx, state) if s.log != nil && len(out.Dismissed) > 0 { s.log.Printf("%s#%d dismissed %d finding(s) at %s: %s", repo, pr, len(out.Dismissed), out.Head, reason) } diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 85d81b1..24ff2a8 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -25,7 +25,11 @@ type FeedbackReport struct { Converged bool `json:"converged"` ReviewedBy map[string]bool `json:"reviewed_by"` Findings []dialect.Finding `json:"findings"` - CheckedAt time.Time `json:"checked_at"` + // Dismissed counts the findings this head's round accounted for through + // `crq dismiss`. They are withheld from Findings, so the count is how a + // reader sees something was set aside rather than never reported. + Dismissed int `json:"dismissed,omitempty"` + CheckedAt time.Time `json:"checked_at"` // Open is whether the PR was open in the same observation Head and // ReviewedBy came from. It is deliberately not serialized — the feedback // JSON contract is frozen — and exists so a caller deciding an action reads @@ -428,6 +432,20 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe } report.Findings = dedupeFindings(report.Findings, suppressPromptAt, settledStableIDs) + // Dismissals apply HERE, not in the caller: convergence and `crq loop`'s exit + // code are computed from this list, so filtering further out would leave a + // dismissed finding permanently actionable everywhere except `crq next`. + if round != nil && round.Head == head && len(round.Dismissed) > 0 { + kept := make([]dialect.Finding, 0, len(report.Findings)) + for _, finding := range report.Findings { + if round.IsDismissed(finding.ID) { + report.Dismissed++ + continue + } + kept = append(kept, finding) + } + report.Findings = kept + } sort.Slice(report.Findings, func(i, j int) bool { if dialect.RankSeverity(report.Findings[i].Severity) != dialect.RankSeverity(report.Findings[j].Severity) { return dialect.RankSeverity(report.Findings[i].Severity) > dialect.RankSeverity(report.Findings[j].Severity) diff --git a/internal/crq/next.go b/internal/crq/next.go index cb903af..700ac34 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -219,26 +219,14 @@ func (s *Service) nextFromState(ctx context.Context, repo string, pr int) (NextR report.LocalWork, report.LocalWorkReason = s.checkLocalWork(ctx, []string{repo, feedback.HeadRepo}, report.Head, feedback.HeadRef) - // A dismissed finding is one the agent accounted for and GitHub offers no way - // to close. Withholding it here is what lets the round move on; leaving it in - // would repeat `fix` forever, which is the deadlock dismissal exists to end. - findings := feedback.Findings - if round != nil && round.Head == feedback.Head && len(round.Dismissed) > 0 { - kept := make([]dialect.Finding, 0, len(findings)) - for _, f := range findings { - if round.IsDismissed(f.ID) { - report.Dismissed++ - continue - } - kept = append(kept, f) - } - findings = kept - } + // Feedback has already withheld what this round dismissed — including from + // its own convergence verdict — so there is one filter, not one per caller. + report.Dismissed = feedback.Dismissed in := engine.NextInput{ Obs: engine.Observation{Head: feedback.Head, Open: feedback.Open}, Completion: engine.CompletionStatus{ReviewedBy: feedback.ReviewedBy, Done: allReviewed(feedback.ReviewedBy)}, - Findings: findings, + Findings: feedback.Findings, Global: s.global(st, now), Primary: s.cfg.Bot, LocalWork: report.LocalWork, diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 3ab29d7..184362e 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -2,6 +2,7 @@ package crq import ( "context" + "encoding/json" "fmt" "strings" "testing" @@ -372,6 +373,41 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { t.Error("a dismissed finding must be withheld from findings") } } + // A threaded finding is refused: resolve and decline both put the decision on + // the PR where the bot can answer it, and dismissing one would converge the + // round with its thread still open. + f.gh.graphQL = func(query string, _ map[string]any, out any) error { + if !strings.Contains(query, "reviewThreads") { + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"timelineItems":{"nodes":[]}}}}`), out) + } + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":""}, + "nodes":[{"id":"PRRT_open","isResolved":false,"isOutdated":false,"path":"internal/state/state.go","line":42, + "comments":{"totalCount":1,"nodes":[{"databaseId":902,"body":"_⚠️ Potential issue_\n\nThis dereferences a nil round.", + "path":"internal/state/state.go","line":42,"author":{"login":"coderabbitai[bot]"},"commit":{"oid":"aaaaaaaa1"}}]}}] + }}}}`), out) + } + threaded := f.next(repo, pr) + var threadedID string + for _, finding := range threaded.Findings { + if finding.ThreadID != "" { + threadedID = finding.ID + break + } + } + if threadedID == "" { + t.Fatal("expected a threaded finding to try to dismiss") + } + if _, err := f.svc.Dismiss(f.ctx, repo, pr, []string{threadedID}, "not doing this one"); err == nil { + t.Error("dismissing a finding that HAS a thread must be refused") + } + + // So is an ID that is not a finding here at all — a stale copy-paste would + // otherwise record a dismissal that silences whatever later matches it. + if _, err := f.svc.Dismiss(f.ctx, repo, pr, []string{"deadbeef"}, "typo"); err == nil { + t.Error("dismissing an unknown finding id must be refused") + } + // It is scoped to this head. A push supersedes the round, and the next // reviewer reporting the same thing must be heard again. f.clk.advance(time.Minute) diff --git a/llms.txt b/llms.txt index 1589da2..6d43f4a 100644 --- a/llms.txt +++ b/llms.txt @@ -210,7 +210,9 @@ crq dismiss REPO PR FINDING_ID [FINDING_ID...] --reason "why this is set aside" `FINDING_ID` is `.findings[].id` — content-derived, not a GitHub node ID, which is why the repo and PR are required. The dismissal covers the current head only; after a push the next reviewer must -report it again. `crq next` reports `dismissed: N` so a set-aside finding never looks lost. +report it again. `crq next` and `crq feedback` report `dismissed: N` so a set-aside finding never +looks lost, and `crq loop` converges on the same filtered list. A finding that HAS a thread is +refused: resolve or decline it instead, so the decision lands where the bot can answer it. For a SKIPPED review specifically, the two remedies differ: narrowing the PR addresses why the review was skipped, while `crq dismiss` only records that you chose to proceed at this head. diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 1de200c..c08a257 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -181,7 +181,11 @@ crq dismiss "$REPO" "$PR" "$FINDING_ID" --reason "why this is being set aside" Finding IDs come from `.findings[].id`; they are content-derived, not GitHub node IDs, which is why the repo and PR are required. A dismissal covers the current head only — push, and the next reviewer -has to report it again. `crq next` then reports `dismissed: N` so nothing looks silently dropped. +has to report it again. `crq next` and `crq feedback` both report `dismissed: N` so nothing looks +silently dropped, and `crq loop` converges on the same filtered list. + +Only a finding with no thread can be dismissed. One that has a `thread_id` is refused — resolve or +decline it, so the decision lands on the PR where the bot can answer it. Judge the finding first. Dismiss is for one you have decided about, not for clearing the list. When the notice is a SKIPPED review, narrowing the PR addresses the cause; dismissing only records that From c401ac3ded896e47fedb90bc08d9f726d5005577 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:01:20 +0200 Subject: [PATCH 03/12] Refuse the dismissals that would cost quota or bury a thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/dismiss.go | 102 +++++++++++++++++++++-------------- internal/crq/dismiss_test.go | 38 +++++++++++++ internal/crq/next_test.go | 7 +++ internal/crq/service.go | 50 +++++++++++++++++ 4 files changed, 157 insertions(+), 40 deletions(-) create mode 100644 internal/crq/dismiss_test.go diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index 9ae9e0c..3ca7b86 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" ) // DismissResult reports what a dismissal did, per finding ID. @@ -55,10 +56,9 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string return DismissResult{}, errors.New("no finding id given") } - // Read the findings BEFORE writing anything. Two reasons: a dismissal is only - // meaningful against a finding that is actually there, and enqueueing first - // would leave a fire-eligible round behind whenever validation then failed — - // which the autoreview daemon could claim and spend a review on. + // Read the findings BEFORE writing anything. A dismissal is only meaningful + // against a finding that is actually there, and writing first would leave a + // round behind whenever validation then failed. feedback, err := s.Feedback(ctx, repo, pr) if err != nil { return DismissResult{}, err @@ -71,50 +71,72 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string current[finding.ID] = finding } - out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} - state, err := s.store.Update(ctx, func(st *State) error { - now := s.clock() - round := st.Round(repo, pr) - var err error - switch { - case round == nil: - round, err = st.NewRound(repo, pr, feedback.Head, now) - case round.Head != feedback.Head: - round, err = st.Supersede(repo, pr, feedback.Head, now) + wanted := map[string]bool{} + for _, id := range clean { + finding, ok := current[id] + if !ok { + return DismissResult{}, fmt.Errorf("%s is not a finding on %s#%d at %s; re-read them with crq next", id, repo, pr, feedback.Head) + } + if err := dismissible(finding); err != nil { + return DismissResult{}, err } - if err != nil { - return err + // A finding ID is a hash of its text, not of its commit. Dismissing one + // carried from an older commit would record it against the current head, + // and the identical finding re-reported here would then be filtered + // though it was never judged for this head. + if finding.Commit != "" && feedback.Head != "" && !dialect.SHAPrefixMatch(finding.Commit, feedback.Head) { + return DismissResult{}, fmt.Errorf("%s belongs to commit %s, not the current head %s; re-read the findings", + id, shortSHA(finding.Commit), feedback.Head) } - out.Dismissed = out.Dismissed[:0] - out.Already = nil - for _, id := range clean { - if round.IsDismissed(id) { - out.Already = append(out.Already, id) - continue - } - finding, ok := current[id] - if !ok { - return fmt.Errorf("%s is not a finding on %s#%d at %s; re-read them with crq next", id, repo, pr, feedback.Head) - } - // A threaded finding has resolve and decline, and both put the - // decision on the PR where the bot can answer it. Dismissing one - // would converge the round with the thread still open, silently - // skipping the rebuttal flow that exists for exactly this. - if finding.ThreadID != "" { - return fmt.Errorf("%s has review thread %s: resolve it or decline it, so the decision is on the PR", id, finding.ThreadID) - } - round.Dismiss(id, reason) - out.Dismissed = append(out.Dismissed, id) + wanted[id] = true + } + + // Whether this call drains the head decides if a round may be CREATED here. + // Creating one while other findings are still open would put a fire-eligible + // round in the queue that DecideFire cannot hold back — it sees no findings — + // so a pump could spend the primary's quota on code the caller is still + // expected to fix. + remaining := 0 + for _, finding := range engine.BlockingFindings(feedback.Findings, feedback.Head) { + if !wanted[finding.ID] { + remaining++ } - st.PutRound(*round) - return nil - }) + } + + out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} + out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0) if err != nil { return DismissResult{}, err } - s.sync(ctx, state) if s.log != nil && len(out.Dismissed) > 0 { s.log.Printf("%s#%d dismissed %d finding(s) at %s: %s", repo, pr, len(out.Dismissed), out.Head, reason) } return out, nil } + +// dismissibleSources are the finding kinds that intrinsically have no review +// thread. Everything else either has one, or only LOOKS threadless: the REST +// fallback Feedback uses when the GraphQL thread query fails emits inline +// comments as "review_comment" with no thread ID, because REST does not return +// one — dismissing those would converge a round over threads that are open. +var dismissibleSources = map[string]bool{ + "review_body": true, + "review_prompt": true, + "review_skipped": true, + "issue_comment": true, +} + +// dismissible reports why a finding may not be dismissed, or nil. +func dismissible(finding dialect.Finding) error { + // A threaded finding has resolve and decline, and both put the decision on + // the PR where the bot can answer it. Dismissing one would converge the + // round with the thread still open, skipping the rebuttal flow. + if finding.ThreadID != "" { + return fmt.Errorf("%s has review thread %s: resolve it or decline it, so the decision is on the PR", finding.ID, finding.ThreadID) + } + if !dismissibleSources[finding.Source] { + return fmt.Errorf("%s came from %s, which can carry a review thread crq could not read; resolve or decline it instead", + finding.ID, finding.Source) + } + return nil +} diff --git a/internal/crq/dismiss_test.go b/internal/crq/dismiss_test.go new file mode 100644 index 0000000..387160c --- /dev/null +++ b/internal/crq/dismiss_test.go @@ -0,0 +1,38 @@ +package crq + +import ( + "testing" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" +) + +// Only a finding that intrinsically cannot have a thread may be dismissed. +// "No thread ID" is not the same 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. +func TestOnlyIntrinsicallyThreadlessFindingsAreDismissible(t *testing.T) { + for _, tc := range []struct { + source string + want bool + }{ + {"review_body", true}, + {"review_prompt", true}, + {"review_skipped", true}, + {"issue_comment", true}, + {"review_comment", false}, // REST fallback: the thread exists, unread + {"review_thread", false}, + {"review_reply", false}, + } { + t.Run(tc.source, func(t *testing.T) { + err := dismissible(dialect.Finding{ID: "x", Source: tc.source}) + if (err == nil) != tc.want { + t.Errorf("dismissible(%s) err = %v, want dismissible=%v", tc.source, err, tc.want) + } + }) + } + + // A thread ID settles it whatever the source says. + if err := dismissible(dialect.Finding{ID: "x", Source: "review_body", ThreadID: "PRRT_1"}); err == nil { + t.Error("a finding with a thread must never be dismissible") + } +} diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 184362e..582f84f 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -408,6 +408,13 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { t.Error("dismissing an unknown finding id must be refused") } + // And a round that has moved on is refused rather than superseded back: the + // head advanced after the findings were read, so this decision is about a + // commit nobody is looking at, and superseding would archive the live round. + if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, "999999999", []string{id}, "stale", true); err == nil { + t.Error("recording a dismissal against a stale head must be refused, not superseded") + } + // It is scoped to this head. A push supersedes the round, and the next // reviewer reporting the same thing must be heard again. f.clk.advance(time.Minute) diff --git a/internal/crq/service.go b/internal/crq/service.go index 2f38594..b10e2ac 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -375,6 +375,56 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu return res, true, nil } +// recordDismissal is the effects executor for `crq dismiss`: the CAS write that +// records which findings a round has accounted for. Dismiss decides WHETHER a +// dismissal is legitimate; this performs it, so the write surface stays in one +// file with every other one. +// +// allowCreate says whether a round may be created for this head. It is false +// when other blocking findings remain, because a fresh round is fire-eligible +// and DecideFire — which never sees findings — could not hold it back. +// +// A round tracking a DIFFERENT head is refused rather than superseded. The head +// moved after Dismiss read the findings, so this decision is about a commit +// nobody is looking at any more; superseding would archive the newer round and +// point the queue back at the stale head. +func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head string, ids []string, reason string, allowCreate bool) (dismissed, already []string, err error) { + if s.cfg.DryRun { + return []string{}, nil, nil + } + state, err := s.store.Update(ctx, func(st *State) error { + round := st.Round(repo, pr) + switch { + case round == nil && !allowCreate: + return fmt.Errorf("%s#%d has other unaddressed findings at %s: dismiss or resolve them in the same pass, so no round is queued while work is open", repo, pr, head) + case round == nil: + var err error + if round, err = st.NewRound(repo, pr, head, s.clock()); err != nil { + return err + } + case round.Head != head: + return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, round.Head) + } + // Reset on every CAS attempt: a retry replays this closure, and appending + // to the outer slices would report each dismissal once per attempt. + dismissed, already = []string{}, nil + for _, id := range ids { + if round.Dismiss(id, reason) { + dismissed = append(dismissed, id) + } else { + already = append(already, id) + } + } + st.PutRound(*round) + return nil + }) + if err != nil { + return nil, nil, err + } + s.sync(ctx, state) + return dismissed, already, nil +} + // applyAccountBlock records an observed account-quota block, whatever observed // it. It is the single writer for that transition: the decision of whether the // block counts is engine.AcceptAccountBlock's, and executing it belongs here with From c1f63aa5581c3d767334531782718509e160f099 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:43:25 +0200 Subject: [PATCH 04/12] Make a repeated dismissal succeed, and a partial one refuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/dismiss.go | 20 ++++++++++++++++++++ internal/crq/feedback.go | 6 +++++- internal/crq/next_test.go | 9 +++++++++ internal/crq/service.go | 6 +++++- 4 files changed, 39 insertions(+), 2 deletions(-) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index 3ca7b86..90e98b3 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -71,8 +71,28 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string current[finding.ID] = finding } + // Already-dismissed IDs are no longer in Findings — Feedback filters them — + // so a retried call would fail validation on its own earlier success. The + // command is documented as idempotent, and an interrupted agent repeating + // itself is the ordinary case. + alreadyDone := map[string]bool{} + if st, _, err := s.store.Load(ctx); err == nil { + if round := st.Round(repo, pr); round != nil && round.Head == feedback.Head { + for _, id := range clean { + if round.IsDismissed(id) { + alreadyDone[id] = true + } + } + } + } else { + return DismissResult{}, err + } + wanted := map[string]bool{} for _, id := range clean { + if alreadyDone[id] { + continue + } finding, ok := current[id] if !ok { return DismissResult{}, fmt.Errorf("%s is not a finding on %s#%d at %s; re-read them with crq next", id, repo, pr, feedback.Head) diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 24ff2a8..31d70fe 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -438,7 +438,11 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe if round != nil && round.Head == head && len(round.Dismissed) > 0 { kept := make([]dialect.Finding, 0, len(report.Findings)) for _, finding := range report.Findings { - if round.IsDismissed(finding.ID) { + // Only where the source itself cannot carry a thread. IDs hash the + // text, not the source, so a body finding later delivered as an inline + // comment through the REST fallback hashes the same — and filtering on + // the ID alone would hide a review thread that is open. + if dismissibleSources[finding.Source] && finding.ThreadID == "" && round.IsDismissed(finding.ID) { report.Dismissed++ continue } diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 582f84f..195f43a 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -373,6 +373,15 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { t.Error("a dismissed finding must be withheld from findings") } } + // Repeating a successful dismissal must succeed: Feedback has already + // filtered that ID out, so validating against the current findings alone + // would fail an interrupted agent on its own earlier success. + if res, err := f.svc.Dismiss(f.ctx, repo, pr, []string{id}, "already handled in an earlier commit"); err != nil { + t.Errorf("repeating a dismissal must be idempotent, got %v", err) + } else if len(res.Already) != 1 { + t.Errorf("result = %+v, want the id reported as already dismissed", res) + } + // A threaded finding is refused: resolve and decline both put the decision on // the PR where the bot can answer it, and dismissing one would converge the // round with its thread still open. diff --git a/internal/crq/service.go b/internal/crq/service.go index b10e2ac..29b4850 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -394,8 +394,12 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head } state, err := s.store.Update(ctx, func(st *State) error { round := st.Round(repo, pr) + // The guard is about whether a FIRE-ELIGIBLE round would exist with other + // findings still open, not about whether a round object exists. A queued + // round is just as dangerous as a new one: Pump can hand it to DecideFire, + // which sees no findings and cannot enforce drain-first. switch { - case round == nil && !allowCreate: + case !allowCreate && (round == nil || round.FireEligible(s.clock())): return fmt.Errorf("%s#%d has other unaddressed findings at %s: dismiss or resolve them in the same pass, so no round is queued while work is open", repo, pr, head) case round == nil: var err error From c5c2e86c24814280a52600d7f8c6a217a450ade0 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:27:05 +0200 Subject: [PATCH 05/12] Supersede the stale round instead of refusing the dismissal 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. --- internal/crq/dismiss.go | 7 +++++-- internal/crq/next_test.go | 9 +++++++-- internal/crq/service.go | 20 ++++++++++++++++++-- llms.txt | 2 +- skills/coderabbit-queue/SKILL.md | 6 +++++- 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index 90e98b3..eeb0c2f 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -59,6 +59,7 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string // Read the findings BEFORE writing anything. A dismissal is only meaningful // against a finding that is actually there, and writing first would leave a // round behind whenever validation then failed. + readAt := s.clock().UTC() feedback, err := s.Feedback(ctx, repo, pr) if err != nil { return DismissResult{}, err @@ -118,13 +119,15 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string // expected to fix. remaining := 0 for _, finding := range engine.BlockingFindings(feedback.Findings, feedback.Head) { - if !wanted[finding.ID] { + // alreadyDone counts as handled: a concurrent agent dismissing the same + // finding must not make this call think work is still open and refuse. + if !wanted[finding.ID] && !alreadyDone[finding.ID] { remaining++ } } out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} - out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0) + out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0, readAt) if err != nil { return DismissResult{}, err } diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 195f43a..90da48b 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -420,8 +420,13 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { // And a round that has moved on is refused rather than superseded back: the // head advanced after the findings were read, so this decision is about a // commit nobody is looking at, and superseding would archive the live round. - if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, "999999999", []string{id}, "stale", true); err == nil { - t.Error("recording a dismissal against a stale head must be refused, not superseded") + // A round enqueued AFTER the findings were read means another worker moved + // the PR forward, so this decision is about a commit nobody is looking at. + // (A round left on the previous head is the ordinary post-push state and is + // superseded instead — that is what the deadlock fix depends on.) + if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, "999999999", []string{id}, "stale", true, + f.clk.now().Add(-time.Hour)); err == nil { + t.Error("a round enqueued after the read must be refused, not superseded") } // It is scoped to this head. A push supersedes the round, and the next diff --git a/internal/crq/service.go b/internal/crq/service.go index 29b4850..571e622 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -380,6 +380,10 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu // dismissal is legitimate; this performs it, so the write surface stays in one // file with every other one. // +// readAt is when the caller read the findings, which is what separates a round +// left on the PREVIOUS head — the ordinary state after a push — from one another +// worker moved forward while this call was deciding. +// // allowCreate says whether a round may be created for this head. It is false // when other blocking findings remain, because a fresh round is fire-eligible // and DecideFire — which never sees findings — could not hold it back. @@ -388,7 +392,7 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu // moved after Dismiss read the findings, so this decision is about a commit // nobody is looking at any more; superseding would archive the newer round and // point the queue back at the stale head. -func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head string, ids []string, reason string, allowCreate bool) (dismissed, already []string, err error) { +func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head string, ids []string, reason string, allowCreate bool, readAt time.Time) (dismissed, already []string, err error) { if s.cfg.DryRun { return []string{}, nil, nil } @@ -406,8 +410,20 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head if round, err = st.NewRound(repo, pr, head, s.clock()); err != nil { return err } - case round.Head != head: + case round.Head != head && round.EnqueuedAt.After(readAt): + // Enqueued after the findings were read: something moved the PR + // forward underneath this call, so the decision is about a commit + // nobody is looking at any more. return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, round.Head) + case round.Head != head: + // The ordinary state after a push: the stored round is still on the + // PREVIOUS head, because `crq next` returns on a current-head finding + // before it enqueues. Refusing here would leave the new head in the + // exact drain-first deadlock this command exists to end. + var err error + if round, err = st.Supersede(repo, pr, head, s.clock()); err != nil { + return err + } } // Reset on every CAS attempt: a retry replays this closure, and appending // to the outer slices would report each dismissal once per attempt. diff --git a/llms.txt b/llms.txt index 6d43f4a..2601c93 100644 --- a/llms.txt +++ b/llms.txt @@ -48,7 +48,7 @@ crq next OWNER/REPO PR_NUMBER | `.action` | what to do | |---|---| -| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` clears with `crq dismiss` once judged. Call again. | +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` clears with `crq dismiss` once judged — except `source: "review_comment"`, which lost its thread ID to a GraphQL fallback and has to wait for crq to read threads again. Call again. | | `hold` | Do NOT commit or push: a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | | `push` | The head is released. Commit and push the accumulated fixes once. Call again. | | `wait` | Nothing to do until `.recheck_after`. | diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index c08a257..ec77d87 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -187,7 +187,11 @@ silently dropped, and `crq loop` converges on the same filtered list. Only a finding with no thread can be dismissed. One that has a `thread_id` is refused — resolve or decline it, so the decision lands on the PR where the bot can answer it. -Judge the finding first. Dismiss is for one you have decided about, not for clearing the list. When +Judge the finding first. Dismiss is for one you have decided about, not for clearing the list. + +If `crq dismiss` refuses a finding whose source is `review_comment`, crq could not read GitHub's +review threads (a GraphQL failure) and fell back to REST, which returns no thread IDs. The finding +does have a thread; retry once crq can read threads again rather than working around it. When the notice is a SKIPPED review, narrowing the PR addresses the cause; dismissing only records that you chose to proceed at this head. From a3da74ebde6ade082c201c0ced1ac2f8c2b0d937 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:41:28 +0200 Subject: [PATCH 06/12] Judge a cooling round by whether it will fire, not whether it can now 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. --- README.md | 4 +++- internal/crq/dismiss.go | 9 +++++++++ internal/crq/service.go | 16 +++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8710952..fee847b 100644 --- a/README.md +++ b/README.md @@ -601,7 +601,9 @@ If you're an autonomous agent running a PR-review loop, here's everything you ne because a thread left open keeps its finding actionable; `--keep-open` overrides. - **Dismiss what has no thread:** a finding with no `thread_id` cannot be resolved or declined, and blocks every future round until it is accounted for: `crq dismiss - --reason "…"`. It covers the current head only. For a skipped review, narrowing the PR fixes the + --reason "…"`. It covers the current head only. A `source: "review_comment"` + finding is refused: it lost its thread ID to crq's REST fallback and still has + an open thread, so it needs `resolve`/`decline` once crq can read threads again. For a skipped review, narrowing the PR fixes the cause; dismissing only records that you chose to proceed. - **Don't narrate the wait.** Report real state changes — findings, a push, convergence, a block — not elapsed time. diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index eeb0c2f..3ab19f8 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -126,6 +126,15 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string } } + // The stored round can be absent or still on the old head after a push that + // nothing has enqueued yet, so state alone cannot tell "no round for this + // head" from "the head moved". Ask GitHub once more. + if head, _, err := s.pullHead(ctx, repo, pr); err != nil { + return DismissResult{}, err + } else if head != feedback.Head { + return DismissResult{}, fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, head) + } + out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0, readAt) if err != nil { diff --git a/internal/crq/service.go b/internal/crq/service.go index 571e622..3dfb120 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -403,7 +403,7 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head // round is just as dangerous as a new one: Pump can hand it to DecideFire, // which sees no findings and cannot enforce drain-first. switch { - case !allowCreate && (round == nil || round.FireEligible(s.clock())): + case !allowCreate && (round == nil || canStillFire(*round)): return fmt.Errorf("%s#%d has other unaddressed findings at %s: dismiss or resolve them in the same pass, so no round is queued while work is open", repo, pr, head) case round == nil: var err error @@ -591,6 +591,20 @@ func releaseSlot(st *State, key string) { } } +// canStillFire reports whether a round will ask for a review at some point. +// +// FireEligible answers "right now", which is the wrong question here: a round +// cooling in awaiting_retry is not eligible this second and becomes eligible the +// moment its RetryAt passes. Letting a partial dismissal through on that +// technicality just defers the problem to the cooldown expiring. +func canStillFire(r Round) bool { + switch r.Phase { + case PhaseQueued, PhaseReserved, PhaseAwaitingRetry: + return true + } + return false +} + // sameRound reports whether the stored round r is still the one that was // observed — same Seq and Head. Every CAS mutation guards on it so a concurrent // supersede (which archives the old round and creates a fresh one with a new Seq) From 3fd635e600fc5d13d08773b5d106204f5e1cf744 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 22:51:43 +0200 Subject: [PATCH 07/12] Dismiss the same repository it validated --- internal/crq/dismiss.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index 3ab19f8..53ba22f 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -42,6 +42,11 @@ type DismissResult struct { // that no round exists for the head at all, so a dismissal with nowhere to live // would change nothing. func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string, reason string) (DismissResult, error) { + // Normalize once, up front: Feedback normalizes internally but pullHead and + // the round key do not, so a spelling the CLI accepts (`Owner/Repo.git`) + // would otherwise validate against one repository and write the round + // against another. + repo = NormalizeRepo(repo) reason = strings.TrimSpace(reason) if reason == "" { return DismissResult{}, errors.New("a dismissal needs a reason") From c1174458b584ab17496ba6ec039717429eb29c40 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:23:22 +0200 Subject: [PATCH 08/12] Refuse the partial dismissal that would queue the new head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A round left on the previous head can be past firing, so canStillFire said no and the guard let a partial dismissal through — and the supersede below then replaced it with a fresh queued round for the new head, which is exactly the fire-eligible round the guard exists to prevent while findings are open. Tell a stale round from a live one by Seq rather than by comparing its EnqueuedAt against this host's clock: the fleet's hosts do not agree about the time, and a worker whose clock lagged could supersede a newer round back to a commit nobody is looking at. A replay of a dismissal that already succeeded writes nothing, so it now returns before the guard instead of being refused whenever some other finding is still open — the command is documented as idempotent. The dry run runs the same mutation against a throwaway copy of the state, so it reports the ids it would record and the refusals it would hit instead of a blanket success it cannot drift from. Also clear the dismissal count when `crq next` finds the head moved (it is head-scoped), and name issue-comment findings in both threadless-finding lists, which dismissibleSources has covered all along. --- internal/crq/dismiss.go | 31 +++++++++-- internal/crq/next.go | 4 ++ internal/crq/next_test.go | 96 +++++++++++++++++++++++++++++--- internal/crq/service.go | 62 +++++++++++++-------- llms.txt | 2 +- skills/coderabbit-queue/SKILL.md | 4 +- 6 files changed, 160 insertions(+), 39 deletions(-) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index 53ba22f..d73e8c6 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -64,7 +64,6 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string // Read the findings BEFORE writing anything. A dismissal is only meaningful // against a finding that is actually there, and writing first would leave a // round behind whenever validation then failed. - readAt := s.clock().UTC() feedback, err := s.Feedback(ctx, repo, pr) if err != nil { return DismissResult{}, err @@ -81,12 +80,22 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string // so a retried call would fail validation on its own earlier success. The // command is documented as idempotent, and an interrupted agent repeating // itself is the ordinary case. + // + // seenSeq identifies the round as this call read it. recordDismissal + // compares it to tell the ordinary round left on the PREVIOUS head from one + // another worker moved forward while this call was deciding — Seq is the + // state's own counter, so that holds across a fleet whose clocks do not + // agree. alreadyDone := map[string]bool{} + var seenSeq int64 if st, _, err := s.store.Load(ctx); err == nil { - if round := st.Round(repo, pr); round != nil && round.Head == feedback.Head { - for _, id := range clean { - if round.IsDismissed(id) { - alreadyDone[id] = true + if round := st.Round(repo, pr); round != nil { + seenSeq = round.Seq + if round.Head == feedback.Head { + for _, id := range clean { + if round.IsDismissed(id) { + alreadyDone[id] = true + } } } } @@ -117,6 +126,16 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string wanted[id] = true } + // Every ID is already recorded at this head: a replay of a call that + // succeeded, writing nothing. It must not be judged by whether a round may + // be created, or an interrupted agent repeating itself would be refused the + // moment some OTHER finding is still open — the exact case the command + // promises is idempotent. + if len(wanted) == 0 { + return DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, + Dismissed: []string{}, Already: clean}, nil + } + // Whether this call drains the head decides if a round may be CREATED here. // Creating one while other findings are still open would put a fire-eligible // round in the queue that DecideFire cannot hold back — it sees no findings — @@ -141,7 +160,7 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string } out := DismissResult{Repo: repo, PR: pr, Head: feedback.Head, Reason: reason, Dismissed: []string{}} - out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0, readAt) + out.Dismissed, out.Already, err = s.recordDismissal(ctx, repo, pr, feedback.Head, clean, reason, remaining == 0, seenSeq) if err != nil { return DismissResult{}, err } diff --git a/internal/crq/next.go b/internal/crq/next.go index 700ac34..572bb79 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -116,6 +116,10 @@ func (s *Service) Next(ctx context.Context, repo string, pr int) (NextReport, er report.Action = string(engine.ActionWait) report.Reason = "the head moved while deciding; re-reading it" report.Findings = []dialect.Finding{} + // Dismissals are head-scoped, so the count read for the old head says + // nothing about this one — reporting it beside the new head would credit + // the new head with decisions never made about it. + report.Dismissed = 0 at := s.clock().Add(s.waitTick()).UTC() report.RecheckAfter = &at return report, nil diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 90da48b..e34b399 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -420,13 +420,13 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { // And a round that has moved on is refused rather than superseded back: the // head advanced after the findings were read, so this decision is about a // commit nobody is looking at, and superseding would archive the live round. - // A round enqueued AFTER the findings were read means another worker moved - // the PR forward, so this decision is about a commit nobody is looking at. - // (A round left on the previous head is the ordinary post-push state and is - // superseded instead — that is what the deadlock fix depends on.) - if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, "999999999", []string{id}, "stale", true, - f.clk.now().Add(-time.Hour)); err == nil { - t.Error("a round enqueued after the read must be refused, not superseded") + // A round whose Seq is not the one the findings were read against means + // another worker moved the PR forward — identity, not a host clock, because + // the fleet's clocks cannot be compared. (A round left on the previous head + // is the ordinary post-push state and is superseded instead — that is what + // the deadlock fix depends on.) + if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, "999999999", []string{id}, "stale", true, 0); err == nil { + t.Error("a round created after the read must be refused, not superseded") } // It is scoped to this head. A push supersedes the round, and the next @@ -443,3 +443,85 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { t.Errorf("the dismissal must not outlive the head it was made for, got %#v", round) } } + +// A partial dismissal must not queue the new head. The round left behind by a +// push can be past firing — completed, say — which makes it harmless where it +// stands but not harmless superseded: superseding replaces it with a FRESH +// queued round for the new head, and DecideFire never sees findings, so nothing +// holds that round back while the caller still has work to do. +func TestPartialDismissalWillNotSupersedeIntoAQueuedRound(t *testing.T) { + f := newReplayFixture(t, time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC)) + repo, pr := "owner/repo", 77 + old, head := "aaaaaaaa1", "bbbbbbbb2" + + var seq int64 + if _, err := f.store.Update(f.ctx, func(st *State) error { + r, err := st.NewRound(repo, pr, old, f.clk.now()) + if err != nil { + return err + } + r.Phase = PhaseCompleted + seq = r.Seq + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + + if _, _, err := f.svc.recordDismissal(f.ctx, repo, pr, head, []string{"f1"}, "one of two", false, seq); err == nil { + t.Error("dismissing one finding while others are open must be refused, not superseded into a queued round") + } + st, _, err := f.store.Load(f.ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round(repo, pr); round == nil || round.Head != old { + t.Fatalf("the stale round must be left as it was, got %#v", round) + } +} + +// A dry run reports what a real one would do and writes nothing — including the +// refusals, which is the half a caller cannot see from a blanket success. +func TestDryRunDismissalReportsWithoutWriting(t *testing.T) { + f := newReplayFixture(t, time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC)) + repo, pr, head := "owner/repo", 78, "aaaaaaaa1" + cfg := f.cfg + cfg.DryRun = true + dry := NewService(cfg, f.gh, f.store, nil) + dry.now = f.clk.now + + var seq int64 + if _, err := f.store.Update(f.ctx, func(st *State) error { + r, err := st.NewRound(repo, pr, head, f.clk.now()) + if err != nil { + return err + } + r.Phase = PhaseCompleted + seq = r.Seq + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + + dismissed, _, err := dry.recordDismissal(f.ctx, repo, pr, head, []string{"f1"}, "set aside", true, seq) + if err != nil { + t.Fatal(err) + } + if len(dismissed) != 1 { + t.Errorf("a dry run must report the ids it would record, got %v", dismissed) + } + st, _, err := f.store.Load(f.ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round(repo, pr); round == nil || round.IsDismissed("f1") { + t.Errorf("a dry run must write nothing, got %#v", round) + } + + // The head moved under the call: a real run refuses it, so the dry run has + // to say so rather than report a dismissal that could never happen. + if _, _, err := dry.recordDismissal(f.ctx, repo, pr, "bbbbbbbb2", []string{"f1"}, "set aside", true, seq+1); err == nil { + t.Error("a dry run must surface the refusal a real run would hit") + } +} diff --git a/internal/crq/service.go b/internal/crq/service.go index 3dfb120..5c16780 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -380,41 +380,43 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu // dismissal is legitimate; this performs it, so the write surface stays in one // file with every other one. // -// readAt is when the caller read the findings, which is what separates a round -// left on the PREVIOUS head — the ordinary state after a push — from one another -// worker moved forward while this call was deciding. +// seenSeq is the Seq of the round Dismiss read the findings against (0 for +// none), which is what separates a round left on the PREVIOUS head — the +// ordinary state after a push — from one another worker moved forward while +// this call was deciding. Seq is the state's own counter rather than a +// timestamp, so the two cannot be confused by a fleet whose hosts disagree +// about the time. // // allowCreate says whether a round may be created for this head. It is false // when other blocking findings remain, because a fresh round is fire-eligible // and DecideFire — which never sees findings — could not hold it back. // -// A round tracking a DIFFERENT head is refused rather than superseded. The head -// moved after Dismiss read the findings, so this decision is about a commit -// nobody is looking at any more; superseding would archive the newer round and -// point the queue back at the stale head. -func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head string, ids []string, reason string, allowCreate bool, readAt time.Time) (dismissed, already []string, err error) { - if s.cfg.DryRun { - return []string{}, nil, nil - } - state, err := s.store.Update(ctx, func(st *State) error { +// A round tracking a DIFFERENT head is refused when it is not the one Dismiss +// read: the head moved after the findings were read, so this decision is about +// a commit nobody is looking at any more, and superseding would archive the +// newer round and point the queue back at the stale head. +func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head string, ids []string, reason string, allowCreate bool, seenSeq int64) (dismissed, already []string, err error) { + mutate := func(st *State) error { round := st.Round(repo, pr) - // The guard is about whether a FIRE-ELIGIBLE round would exist with other - // findings still open, not about whether a round object exists. A queued - // round is just as dangerous as a new one: Pump can hand it to DecideFire, - // which sees no findings and cannot enforce drain-first. switch { - case !allowCreate && (round == nil || canStillFire(*round)): + case round != nil && round.Head != head && round.Seq != seenSeq: + // Not the round Dismiss read: something moved the PR forward + // underneath this call, so the decision is about a commit nobody is + // looking at any more. + return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, round.Head) + case !allowCreate && (round == nil || round.Head != head || canStillFire(*round)): + // The guard is about whether a FIRE-ELIGIBLE round would exist with + // other findings still open, not about whether a round object + // exists. A queued round is just as dangerous as a new one: Pump can + // hand it to DecideFire, which sees no findings and cannot enforce + // drain-first. So is a round on the previous head, because the + // supersede below would replace it with a fresh queued one. return fmt.Errorf("%s#%d has other unaddressed findings at %s: dismiss or resolve them in the same pass, so no round is queued while work is open", repo, pr, head) case round == nil: var err error if round, err = st.NewRound(repo, pr, head, s.clock()); err != nil { return err } - case round.Head != head && round.EnqueuedAt.After(readAt): - // Enqueued after the findings were read: something moved the PR - // forward underneath this call, so the decision is about a commit - // nobody is looking at any more. - return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, round.Head) case round.Head != head: // The ordinary state after a push: the stored round is still on the // PREVIOUS head, because `crq next` returns on a current-head finding @@ -437,7 +439,21 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head } st.PutRound(*round) return nil - }) + } + // A dry run must report what a real one would do, refusals included. The one + // way it cannot drift from the write is to BE the write, run against a + // throwaway copy of the state that nothing stores. + if s.cfg.DryRun { + st, _, err := s.store.Load(ctx) + if err != nil { + return nil, nil, err + } + if err := mutate(&st); err != nil { + return nil, nil, err + } + return dismissed, already, nil + } + state, err := s.store.Update(ctx, mutate) if err != nil { return nil, nil, err } diff --git a/llms.txt b/llms.txt index 2601c93..7eb32bb 100644 --- a/llms.txt +++ b/llms.txt @@ -201,7 +201,7 @@ reason; the loop clears it once the bot withdraws (or you stop declining and fix replies surface too — crq errs toward showing a possible rebuttal rather than burying it. A finding with no `thread_id` — a review body finding, a review-skipped notice, an outside-diff -remark — has nothing for `resolve` or `decline` to act on, and would otherwise block every future +remark, an issue-comment finding — has nothing for `resolve` or `decline` to act on, and would otherwise block every future round on that PR. Judge it, then record the decision: ```bash diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index ec77d87..309e4ac 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -171,8 +171,8 @@ finding. Pass `--keep-open` to leave it unresolved deliberately. ## Findings With No Thread -Review-body findings, review-skipped notices and outside-diff remarks have no -`thread_id`. `crq resolve` and `crq decline` both act on a thread, so neither can touch them — and a +Review-body findings, review-skipped notices, outside-diff remarks and issue-comment findings have +no `thread_id`. `crq resolve` and `crq decline` both act on a thread, so neither can touch them — and a finding that can never drain blocks every future round on that PR. ```bash From ec3270b48fca09bc415c482c305a57c06da3a474 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:33:21 +0200 Subject: [PATCH 09/12] Pin the replayed dismissal the guard used to refuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The idempotence the short-circuit restores had no test that could fail: the existing replay runs against a completed round, which the guard lets through anyway. This one drives the case that broke — a dismissal that ended the deadlock leaves a QUEUED round for the new head, a later review reports something else, and the agent repeats the dismissal it does not remember finishing — and asserts the replay is both accepted and a no-op. --- internal/crq/next_test.go | 61 +++++++++++++++++++++++++++++++++++++ internal/crq/replay_test.go | 13 ++++++++ 2 files changed, 74 insertions(+) diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index e34b399..8feafff 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -444,6 +444,67 @@ func TestDismissEndsTheUnresolvableFindingDeadlock(t *testing.T) { } } +// Replaying a dismissal that already succeeded writes nothing, so the guard +// against queueing a round while work is open has nothing to judge. Judging the +// call instead of the write refused an interrupted agent on its own earlier +// success the moment any OTHER finding was still open — with a fire-eligible +// round for the head, which a dismissal that ended the deadlock is exactly what +// leaves behind. +func TestReplayedDismissalIsNotRefusedByOtherOpenFindings(t *testing.T) { + f := newReplayFixture(t, time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC)) + repo, pr := "owner/repo", 506 + head := "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, f.clk.now().Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + + // The head moves and threadless findings for the NEW head land before + // anything enqueues a round for it — the deadlock — so the dismissal that + // ends it supersedes the stale round, leaving a queued one behind. + f.clk.advance(2 * time.Minute) + f.setHead(repo, pr, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", f.clk.now()) + f.corpusReview(t, repo, pr, 900, "bbbbbbbb2", "coderabbit/findings-prompt-block.md") + + report := f.next(repo, pr) + f.wantAction(report, engine.ActionFix) + ids := []string{} + for _, finding := range report.Findings { + if finding.ThreadID == "" { + ids = append(ids, finding.ID) + } + } + if len(ids) < 2 { + t.Fatalf("this test needs two threadless findings to dismiss, got %d", len(ids)) + } + if _, err := f.svc.Dismiss(f.ctx, repo, pr, ids, "carried from an earlier commit"); err != nil { + t.Fatal(err) + } + before := f.round(repo, pr) + if before == nil || before.Head != "bbbbbbbb2" || !canStillFire(*before) { + t.Fatalf("the dismissal must leave a fire-eligible round for the new head, got %+v", before) + } + + // A later review reports something new at that same head, so work IS open + // when the agent — which does not remember finishing — repeats itself. + f.clk.advance(time.Minute) + f.corpusReview(t, repo, pr, 901, "bbbbbbbb2", "coderabbit/findings-outside-diff.md") + res, err := f.svc.Dismiss(f.ctx, repo, pr, ids[:1], "carried from an earlier commit") + if err != nil { + t.Fatalf("replaying a completed dismissal must not be refused over another finding: %v", err) + } + if len(res.Already) != 1 || len(res.Dismissed) != 0 { + t.Errorf("result = %+v, want the id reported as already dismissed and nothing recorded", res) + } + // And it stays a no-op: the new finding is still the caller's to fix. + if after := f.round(repo, pr); after == nil || after.Seq != before.Seq || after.Phase != before.Phase { + t.Errorf("a replayed dismissal must not touch the round: %+v -> %+v", before, after) + } + f.wantAction(f.next(repo, pr), engine.ActionFix) +} + // A partial dismissal must not queue the new head. The round left behind by a // push can be past firing — completed, say — which makes it harmless where it // stands but not harmless superseded: superseding replaces it with a FRESH diff --git a/internal/crq/replay_test.go b/internal/crq/replay_test.go index ecbd9a3..71a82f6 100644 --- a/internal/crq/replay_test.go +++ b/internal/crq/replay_test.go @@ -193,6 +193,19 @@ func (f *replayFixture) botReview(repo string, pr int, id int64, commitSHA strin f.gh.reviews[key] = append(f.gh.reviews[key], r) } +// corpusReview posts a bot review whose body is a captured bot message, for the +// reviews that carry their findings in the body itself. +func (f *replayFixture) corpusReview(t *testing.T, repo string, pr int, id int64, commitSHA, corpus string) { + t.Helper() + body := corpusMessage(t, corpus) + f.gh.mu.Lock() + defer f.gh.mu.Unlock() + r := ghapi.Review{ID: id, CommitID: commitSHA, State: "COMMENTED", SubmittedAt: f.clk.now().UTC(), Body: body} + r.User.Login = f.bot + key := fakeKey(repo, pr) + f.gh.reviews[key] = append(f.gh.reviews[key], r) +} + func (f *replayFixture) botReviewComment(repo string, pr int, id int64, commitSHA, path string, line int, body string) { f.gh.mu.Lock() defer f.gh.mu.Unlock() From 384e3e09a252c2d20ac4dfce7694a64eb7210ee3 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:54:01 +0200 Subject: [PATCH 10/12] Let the fixes land instead of holding them for a queued round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A round is no longer created only by an enqueue: `crq dismiss` creates one so the decision has somewhere to live. `crq next` read any round at all as "a review is running for this head", so the documented fix flow — fix locally, dismiss the threadless finding, call again — was answered `hold`, telling the caller to sit on the very fixes it had just made until a review of the code they replace finished. That call then enqueued and pumped, so it could buy that review itself. A queued round has asked for nothing: no command posted, no quota spent, and a push supersedes it for free. Only a round that actually got as far as reserving the slot is a review worth holding a head for. Also count the dismissals the round recorded rather than the current findings that still match one. A finding ID hashes the text, so a bot editing or deleting the comment a dismissal was made against left nothing to match and `dismissed` fell back to zero — making a finding deliberately set aside read exactly like one that was never reported. --- internal/crq/feedback.go | 7 ++- internal/crq/next_test.go | 98 ++++++++++++++++++++++++++++++++++++ internal/engine/next.go | 24 +++++++-- internal/engine/next_test.go | 11 ++++ 4 files changed, 134 insertions(+), 6 deletions(-) diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 31d70fe..9d61046 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -443,12 +443,17 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe // comment through the REST fallback hashes the same — and filtering on // the ID alone would hide a review thread that is open. if dismissibleSources[finding.Source] && finding.ThreadID == "" && round.IsDismissed(finding.ID) { - report.Dismissed++ continue } kept = append(kept, finding) } report.Findings = kept + // The count is of the DECISIONS this round recorded, not of the findings + // that happened to still match one. A bot that edits or deletes the + // comment a dismissal was made against leaves nothing to match, and + // reporting zero there would make a set-aside finding indistinguishable + // from one that was never reported at all. + report.Dismissed = len(round.Dismissed) } sort.Slice(report.Findings, func(i, j int) bool { if dialect.RankSeverity(report.Findings[i].Severity) != dialect.RankSeverity(report.Findings[j].Severity) { diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 8feafff..04cb5f4 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -541,6 +541,104 @@ func TestPartialDismissalWillNotSupersedeIntoAQueuedRound(t *testing.T) { } } +// The documented fix flow dismisses a threadless finding with the fixes for it +// still in the working tree, and the round that dismissal leaves behind has +// asked for nothing yet. Reading it as a review in progress made `crq next` +// answer `hold` — telling the caller to sit on its own fixes until a review of +// the code they replace finished. +func TestDismissalDoesNotHoldTheFixesItWasMadeFor(t *testing.T) { + // A required co-reviewer that never answers is what keeps the head gated + // once the primary's findings have landed — the case the guard is about. + f := newCoReplayFixture(t, time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC), requireBugbot) + repo, pr := "owner/repo", 507 + f.openPull(repo, pr, "aaaaaaaa1") + f.setCommitDate("aaaaaaaa1", f.clk.now().Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + + // The deadlock's shape: the head moved and threadless findings for the NEW + // head landed before anything enqueued a round for it. + f.clk.advance(2 * time.Minute) + f.setHead(repo, pr, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", f.clk.now()) + f.corpusReview(t, repo, pr, 900, "bbbbbbbb2", "coderabbit/findings-prompt-block.md") + + report := f.next(repo, pr) + f.wantAction(report, engine.ActionFix) + ids := []string{} + for _, finding := range report.Findings { + if finding.ThreadID == "" { + ids = append(ids, finding.ID) + } + } + if len(ids) == 0 { + t.Fatal("this test needs a threadless finding to dismiss") + } + + // The caller fixes them locally, then dismisses — the order llms.txt asks + // for. The dismissal creates the queued round for this head. + f.setLocalWork(true, "uncommitted changes in the working tree") + if _, err := f.svc.Dismiss(f.ctx, repo, pr, ids, "fixed locally, not yet pushed"); err != nil { + t.Fatal(err) + } + if round := f.round(repo, pr); round == nil || round.Phase != PhaseQueued { + t.Fatalf("the dismissal must leave a queued round for this head, got %+v", round) + } + + f.clk.advance(time.Minute) + f.wantAction(f.next(repo, pr), engine.ActionPush) +} + +// The dismissed count is of the decisions the round recorded, not of the +// findings that still match one. A finding ID hashes the text, so a bot editing +// the review it came from leaves nothing to match — and counting matches instead +// reported zero, making a finding that was deliberately set aside look like one +// that was never reported at all. +func TestDismissedCountSurvivesTheBotEditingItsOwnReview(t *testing.T) { + f := newReplayFixture(t, time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC)) + repo, pr, head := "owner/repo", 508, "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, f.clk.now().Add(-time.Minute)) + f.setLocalWork(false, "") + + f.next(repo, pr) + f.clk.advance(2 * time.Minute) + f.corpusReview(t, repo, pr, 900, head, "coderabbit/findings-outside-diff.md") + + report := f.next(repo, pr) + f.wantAction(report, engine.ActionFix) + id := "" + for _, finding := range report.Findings { + if finding.ThreadID == "" { + id = finding.ID + break + } + } + if id == "" { + t.Fatal("this test needs a threadless finding to dismiss") + } + if _, err := f.svc.Dismiss(f.ctx, repo, pr, []string{id}, "handled in an earlier commit"); err != nil { + t.Fatal(err) + } + + // The bot rewrites the review the finding came from. Its text — and so its + // ID — is gone, but the decision about it stands. + f.gh.mu.Lock() + key := fakeKey(repo, pr) + for i := range f.gh.reviews[key] { + if f.gh.reviews[key][i].ID == 900 { + f.gh.reviews[key][i].Body = "Reviewed the changes." + } + } + f.gh.mu.Unlock() + + f.clk.advance(time.Minute) + if after := f.next(repo, pr); after.Dismissed != 1 { + t.Errorf("dismissed = %d, want 1 — the record outlives the text it was made against", after.Dismissed) + } +} + // A dry run reports what a real one would do and writes nothing — including the // refusals, which is the half a caller cannot see from a blanket success. func TestDryRunDismissalReportsWithoutWriting(t *testing.T) { diff --git a/internal/engine/next.go b/internal/engine/next.go index 328a236..e8a5e20 100644 --- a/internal/engine/next.go +++ b/internal/engine/next.go @@ -197,11 +197,12 @@ func NextAction(in NextInput, now time.Time) Action { } kind, reason := ActionWait, "awaiting review" if in.LocalWork { - // Holding protects a review that is actually happening. With no round - // at all, nothing has been requested for this head, so holding would - // stall the caller AND spend a review window on code it is about to - // replace. Land the work first; the round then covers the real head. - if in.Round.Phase == "" { + // Holding protects a review that is actually happening. Nothing has + // been requested for this head while the round is only queued — or + // absent entirely — so holding would stall the caller AND spend a + // review window on code it is about to replace. Land the work first; + // the round then covers the real head. + if !reviewRequested(in.Round) { return Action{ Kind: ActionPush, Reason: "no review has been requested for this head yet; land your work before one is", @@ -237,6 +238,19 @@ func (in NextInput) settling(now time.Time) bool { return in.SettleUntil != nil && in.SettleUntil.After(now) } +// reviewRequested reports whether a review has actually been asked for at this +// round's head. +// +// A round with no phase does not exist; a queued one is only a place in the +// fleet's FIFO — no command posted, no quota spent — and a push supersedes it +// for free. The distinction matters because a round is no longer created only by +// an enqueue: `crq dismiss` creates one to record the decision, and reading that +// as "a review is running" made the documented fix flow hold its own fixes until +// a review of the pre-fix head finished. +func reviewRequested(r state.Round) bool { + return r.Phase != "" && r.Phase != state.PhaseQueued +} + // waitExpired reports whether this round's own wait deadline has passed while it // is still under review. // diff --git a/internal/engine/next_test.go b/internal/engine/next_test.go index e8c561e..0d7b638 100644 --- a/internal/engine/next_test.go +++ b/internal/engine/next_test.go @@ -158,6 +158,17 @@ func TestNextAction(t *testing.T) { }, want: ActionPush, }, + { + // A queued round has asked for nothing either — and `crq dismiss` + // creates one to record its decision, so the documented fix flow + // reaches here holding the very fixes the review would be spent on. + name: "a queued round is not a review to hold for", + in: NextInput{ + Round: state.Round{Phase: state.PhaseQueued}, + Obs: openObs(), Completion: both(false, false), LocalWork: true, + }, + want: ActionPush, + }, { name: "all answered with staged work means push", in: NextInput{ From 589fd102d4331f6dbbea30652210b4e880aae176 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:59:49 +0200 Subject: [PATCH 11/12] Pin the other side of "a review worth holding for" reviewRequested draws a line between queued and reserved, and only the queued side is asserted. Pin the reserved one too, so a later widening of the predicate releases a head with the command already about to be posted and fails here instead of on a PR. --- internal/engine/next_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/engine/next_test.go b/internal/engine/next_test.go index 0d7b638..6e908fc 100644 --- a/internal/engine/next_test.go +++ b/internal/engine/next_test.go @@ -169,6 +169,17 @@ func TestNextAction(t *testing.T) { }, want: ActionPush, }, + { + // The other side of that boundary: reserving the slot IS the request, + // so the command is imminent and the head must stop moving. + name: "a reserved round is a review to hold for", + in: NextInput{ + Round: state.Round{Phase: state.PhaseReserved}, + Obs: openObs(), Completion: both(false, false), LocalWork: true, + MinDelay: time.Minute, + }, + want: ActionHold, wantAt: t0.Add(time.Minute), + }, { name: "all answered with staged work means push", in: NextInput{ From bb5a7ea90aada527d26e6fe62e4a17f4b26c8b8b Mon Sep 17 00:00:00 2001 From: kristofferR <481270+kristofferR@users.noreply.github.com> Date: Mon, 27 Jul 2026 07:16:39 +0200 Subject: [PATCH 12/12] Address review feedback on dismissal flow --- internal/crq/dismiss.go | 6 +++++- internal/crq/next_test.go | 32 ++++++++++++++++++++++++++++++-- internal/crq/service.go | 16 +--------------- internal/engine/next.go | 13 +++++++++++++ internal/engine/next_test.go | 28 ++++++++++++++++++++++++++++ skills/coderabbit-queue/SKILL.md | 2 +- 6 files changed, 78 insertions(+), 19 deletions(-) diff --git a/internal/crq/dismiss.go b/internal/crq/dismiss.go index d73e8c6..5dc37ce 100644 --- a/internal/crq/dismiss.go +++ b/internal/crq/dismiss.go @@ -165,7 +165,11 @@ func (s *Service) Dismiss(ctx context.Context, repo string, pr int, ids []string return DismissResult{}, err } if s.log != nil && len(out.Dismissed) > 0 { - s.log.Printf("%s#%d dismissed %d finding(s) at %s: %s", repo, pr, len(out.Dismissed), out.Head, reason) + verb := "dismissed" + if s.cfg.DryRun { + verb = "would dismiss" + } + s.log.Printf("%s#%d %s %d finding(s) at %s: %s", repo, pr, verb, len(out.Dismissed), out.Head, reason) } return out, nil } diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 04cb5f4..6424c8c 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -1,9 +1,11 @@ package crq import ( + "bytes" "context" "encoding/json" "fmt" + "log" "strings" "testing" "time" @@ -483,7 +485,7 @@ func TestReplayedDismissalIsNotRefusedByOtherOpenFindings(t *testing.T) { t.Fatal(err) } before := f.round(repo, pr) - if before == nil || before.Head != "bbbbbbbb2" || !canStillFire(*before) { + if before == nil || before.Head != "bbbbbbbb2" || !engine.CanStillFire(*before) { t.Fatalf("the dismissal must leave a fire-eligible round for the new head, got %+v", before) } @@ -646,7 +648,8 @@ func TestDryRunDismissalReportsWithoutWriting(t *testing.T) { repo, pr, head := "owner/repo", 78, "aaaaaaaa1" cfg := f.cfg cfg.DryRun = true - dry := NewService(cfg, f.gh, f.store, nil) + var logs bytes.Buffer + dry := NewService(cfg, f.gh, f.store, log.New(&logs, "", 0)) dry.now = f.clk.now var seq int64 @@ -683,4 +686,29 @@ func TestDryRunDismissalReportsWithoutWriting(t *testing.T) { if _, _, err := dry.recordDismissal(f.ctx, repo, pr, "bbbbbbbb2", []string{"f1"}, "set aside", true, seq+1); err == nil { t.Error("a dry run must surface the refusal a real run would hit") } + + // The public operation logs the simulated result as hypothetical, rather + // than claiming the throwaway mutation was persisted. + f.openPull(repo, pr, head) + f.setCommitDate(head, f.clk.now()) + f.corpusReview(t, repo, pr, 900, head, "coderabbit/findings-outside-diff.md") + feedback, err := dry.Feedback(f.ctx, repo, pr) + if err != nil { + t.Fatal(err) + } + ids := []string{} + for _, finding := range feedback.Findings { + if finding.ThreadID == "" { + ids = append(ids, finding.ID) + } + } + if len(ids) == 0 { + t.Fatal("this test needs a threadless finding to dismiss") + } + if _, err := dry.Dismiss(f.ctx, repo, pr, ids, "set aside"); err != nil { + t.Fatal(err) + } + if got := logs.String(); !strings.Contains(got, "would dismiss") || strings.Contains(got, "#78 dismissed") { + t.Errorf("dry-run log must describe a hypothetical dismissal, got %q", got) + } } diff --git a/internal/crq/service.go b/internal/crq/service.go index 5c16780..16629bb 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -404,7 +404,7 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head // underneath this call, so the decision is about a commit nobody is // looking at any more. return fmt.Errorf("%s#%d moved to %s while dismissing; re-read the findings", repo, pr, round.Head) - case !allowCreate && (round == nil || round.Head != head || canStillFire(*round)): + case !allowCreate && (round == nil || round.Head != head || engine.CanStillFire(*round)): // The guard is about whether a FIRE-ELIGIBLE round would exist with // other findings still open, not about whether a round object // exists. A queued round is just as dangerous as a new one: Pump can @@ -607,20 +607,6 @@ func releaseSlot(st *State, key string) { } } -// canStillFire reports whether a round will ask for a review at some point. -// -// FireEligible answers "right now", which is the wrong question here: a round -// cooling in awaiting_retry is not eligible this second and becomes eligible the -// moment its RetryAt passes. Letting a partial dismissal through on that -// technicality just defers the problem to the cooldown expiring. -func canStillFire(r Round) bool { - switch r.Phase { - case PhaseQueued, PhaseReserved, PhaseAwaitingRetry: - return true - } - return false -} - // sameRound reports whether the stored round r is still the one that was // observed — same Seq and Head. Every CAS mutation guards on it so a concurrent // supersede (which archives the old round and creates a fresh one with a new Seq) diff --git a/internal/engine/next.go b/internal/engine/next.go index e8a5e20..ac580e1 100644 --- a/internal/engine/next.go +++ b/internal/engine/next.go @@ -251,6 +251,19 @@ func reviewRequested(r state.Round) bool { return r.Phase != "" && r.Phase != state.PhaseQueued } +// CanStillFire reports whether a round will ask for a review at some point. +// +// FireEligible answers "right now", which is the wrong question for callers +// guarding future work: a round cooling in awaiting_retry becomes eligible the +// moment its RetryAt passes. +func CanStillFire(r state.Round) bool { + switch r.Phase { + case state.PhaseQueued, state.PhaseReserved, state.PhaseAwaitingRetry: + return true + } + return false +} + // waitExpired reports whether this round's own wait deadline has passed while it // is still under review. // diff --git a/internal/engine/next_test.go b/internal/engine/next_test.go index 6e908fc..7477853 100644 --- a/internal/engine/next_test.go +++ b/internal/engine/next_test.go @@ -30,6 +30,34 @@ func finding(commit string) dialect.Finding { return dialect.Finding{Bot: "coderabbitai[bot]", Title: "nil deref", Commit: commit, ThreadID: "T1"} } +func TestRoundPhaseRules(t *testing.T) { + for _, tc := range []struct { + name string + phase state.Phase + reviewRequested bool + canStillFire bool + }{ + {"missing", "", false, false}, + {"queued", state.PhaseQueued, false, true}, + {"reserved", state.PhaseReserved, true, true}, + {"fired", state.PhaseFired, true, false}, + {"reviewing", state.PhaseReviewing, true, false}, + {"awaiting retry", state.PhaseAwaitingRetry, true, true}, + {"completed", state.PhaseCompleted, true, false}, + {"abandoned", state.PhaseAbandoned, true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + round := state.Round{Phase: tc.phase} + if got := reviewRequested(round); got != tc.reviewRequested { + t.Errorf("reviewRequested(%q) = %t, want %t", tc.phase, got, tc.reviewRequested) + } + if got := CanStillFire(round); got != tc.canStillFire { + t.Errorf("CanStillFire(%q) = %t, want %t", tc.phase, got, tc.canStillFire) + } + }) + } +} + // The action table. Each row is a claim about what a caller must do next, and // together they are the whole contract `crq next` exposes. func TestNextAction(t *testing.T) { diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 309e4ac..517a93e 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -25,7 +25,7 @@ crq next "$REPO" "$PR" | `.action` | what to do | |---|---| -| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` is cleared with `crq dismiss` once judged — at this head nothing else can. Call again. | +| `fix` | Fix `.findings[]`, validate locally, then `crq resolve` each addressed `.thread_id` (or `crq decline` with a reason). A finding with no `.thread_id` is cleared with `crq dismiss` once judged, except `source: "review_comment"`: retry it because thread lookup failed. Call again. | | `hold` | Do NOT commit or push — a required reviewer has not answered for this head, and moving the head restarts its review (resolving threads does not). Call again at `.recheck_after`. | | `push` | The head is released. Commit and push the accumulated fixes once. Call again. | | `wait` | Nothing to do until `.recheck_after`. |