From 4a2d9880fa1020a8d1ba0f64f3c766bf72550564 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:06:19 +0200 Subject: [PATCH 01/74] 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 f9d091a9..87109526 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 f83f08ae..af8566a2 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 a7930cb6..3690e293 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 00000000..d39025cf --- /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 d72155a7..cb903afe 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 3599d7af..3ab29d7c 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 ede4e853..5d474546 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 f16ddff6..1589da22 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 cab6ef8b..1de200c5 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/74] 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 d39025cf..9ae9e0c9 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 85d81b1b..24ff2a8d 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 cb903afe..700ac347 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 3ab29d7c..184362e3 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 1589da22..6d43f4a3 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 1de200c5..c08a257d 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/74] 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 9ae9e0c9..3ca7b869 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 00000000..387160c4 --- /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 184362e3..582f84f2 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 2f385945..b10e2ac5 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 8361991202b7963610ea080c4711b3b2e4475cd7 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:16:24 +0200 Subject: [PATCH 04/74] Give crq its own place on disk for a repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything crq does with git assumes it was RUN inside the checkout it cares about: the local-work probe shells out with no directory, target inference reads the current branch. That is right for a command an agent types from its working copy, and useless for the daemon, which has no checkout of any repository it reviews — the reason dispatch has nothing to stand on. Workspace separates the two: a bare mirror per repository, fetched rather than re-cloned, and a throwaway detached worktree per head. Detached on purpose — a place to inspect and build, not a branch to commit to by accident. A worktree left behind by a killed process is replaced rather than reused, since the head it holds is stale anyway. Credentials stay out of it. The remote is the ordinary https URL, so the host's existing git credential helper supplies the token and crq never holds a secret it would then have to avoid logging. One runner now executes every git command, taking the directory to run in, and it folds stderr into the error — "exit status 128" alone has never told anybody what went wrong. localWork takes that directory too, through Config.WorkDir, so a caller working in a worktree it made can say so without the Service being copied. The tests build a real repository and clone it, because the thing under test is whether the git invocations are right. --- internal/crq/config.go | 7 +- internal/crq/next.go | 11 +- internal/crq/target.go | 9 +- internal/crq/workspace.go | 209 +++++++++++++++++++++++++++++++++ internal/crq/workspace_test.go | 121 +++++++++++++++++++ 5 files changed, 345 insertions(+), 12 deletions(-) create mode 100644 internal/crq/workspace.go create mode 100644 internal/crq/workspace_test.go diff --git a/internal/crq/config.go b/internal/crq/config.go index 98d6cdf8..9d24f267 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -47,7 +47,12 @@ type Config struct { // Reviewers is the single description of who reviews and what they cost. // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. - Reviewers []Reviewer + Reviewers []Reviewer + // WorkDir is the checkout the local-work probe inspects. Empty means the + // process's own directory, which is what an agent running crq from its + // working copy means. Set programmatically by a caller working in a + // worktree it made, since that caller is not standing in one. + WorkDir string RateLimitCommand string RateLimitMarker string CalibrationMarker string diff --git a/internal/crq/next.go b/internal/crq/next.go index d72155a7..1ee0d81b 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -2,7 +2,6 @@ package crq import ( "context" - "os/exec" "strings" "time" @@ -293,7 +292,7 @@ func (s *Service) checkLocalWork(ctx context.Context, repos []string, head, head if s.localWorkFn != nil { return s.localWorkFn(ctx, head) } - return localWork(ctx, repos, head, headRef) + return localWork(ctx, s.cfg.WorkDir, repos, head, headRef) } // localWork reports whether the working copy holds changes the PR head does not @@ -313,13 +312,15 @@ func (s *Service) checkLocalWork(ctx context.Context, repos []string, head, head // `done` rather than `push`. That is the safe direction: `push` is only ever // emitted once the head is already released, so a missed one costs one extra // call, while a spurious `hold` would stall the loop. -func localWork(ctx context.Context, repos []string, head, headRef string) (bool, string) { +// dir is the checkout to inspect; "" means the process's own directory, +// which is what an agent running crq from its working copy means. +func localWork(ctx context.Context, dir string, repos []string, head, headRef string) (bool, string) { git := func(args ...string) (string, bool) { - out, err := exec.CommandContext(ctx, "git", args...).Output() + out, err := gitDir(ctx, dir, args...) if err != nil { return "", false } - return strings.TrimSpace(string(out)), true + return out, true } if _, ok := git("rev-parse", "--is-inside-work-tree"); !ok { return false, "not run inside a git checkout" diff --git a/internal/crq/target.go b/internal/crq/target.go index 36ed7d3c..d577191e 100644 --- a/internal/crq/target.go +++ b/internal/crq/target.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "net/url" - "os/exec" "strings" ) @@ -117,10 +116,8 @@ func remoteSlugs(remotes string) (repos, owners []string) { return repos, owners } +// gitIn runs git in the process's own working directory, which is what target +// inference is about: the checkout the caller is standing in. func gitIn(ctx context.Context, args ...string) (string, error) { - out, err := exec.CommandContext(ctx, "git", args...).Output() - if err != nil { - return "", err - } - return strings.TrimSpace(string(out)), nil + return gitDir(ctx, "", args...) } diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go new file mode 100644 index 00000000..455cb073 --- /dev/null +++ b/internal/crq/workspace.go @@ -0,0 +1,209 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// Workspace is crq's own place on disk for a repository. +// +// Everything crq does with git today assumes it was RUN inside the checkout it +// cares about: localWork shells out with no directory, target inference reads +// the current branch. That is fine for a command an agent types from its own +// working copy, and useless for the daemon, which has no checkout of any of the +// repositories it reviews. +// +// A Workspace separates the two: a bare mirror per repository, fetched rather +// than re-cloned, and a throwaway worktree per head. Nothing here reads or +// writes the process's working directory. +// +// Credentials are deliberately not crq's business. The remote is the ordinary +// https URL, so whatever git credential helper the host already uses — `gh auth +// setup-git` writes one — supplies the token. crq never holds a secret it would +// then have to avoid logging. +type Workspace struct { + // Root holds the mirrors and worktrees. Empty means the default cache + // location (see DefaultWorkspaceRoot). + Root string +} + +// DefaultWorkspaceRoot is $XDG_CACHE_HOME/crq (or ~/.cache/crq). +func DefaultWorkspaceRoot() (string, error) { + if dir := strings.TrimSpace(os.Getenv("CRQ_WORKSPACE")); dir != "" { + return dir, nil + } + cache, err := os.UserCacheDir() + if err != nil { + return "", err + } + return filepath.Join(cache, "crq"), nil +} + +func (w Workspace) root() (string, error) { + if strings.TrimSpace(w.Root) != "" { + return w.Root, nil + } + return DefaultWorkspaceRoot() +} + +// mirrorPath is where repo's bare mirror lives. The owner and name are path +// segments, so a repo string that is not exactly "owner/name" is refused rather +// than allowed to escape the root. +func (w Workspace) mirrorPath(repo string) (string, error) { + root, err := w.root() + if err != nil { + return "", err + } + owner, name, ok := splitRepo(repo) + if !ok { + return "", fmt.Errorf("repo must be owner/name, got %q", repo) + } + return filepath.Join(root, "mirrors", owner, name+".git"), nil +} + +// splitRepo splits owner/name, rejecting anything that could traverse a path. +func splitRepo(repo string) (owner, name string, ok bool) { + owner, name, ok = strings.Cut(NormalizeRepo(repo), "/") + if !ok || owner == "" || name == "" { + return "", "", false + } + for _, part := range []string{owner, name} { + if part == "." || part == ".." || strings.ContainsAny(part, `/\`) { + return "", "", false + } + } + return owner, name, true +} + +// Mirror makes sure repo's bare mirror exists and is up to date, and returns its +// path. It clones on first use and fetches afterwards — a mirror is reused +// across PRs and across rounds, so the expensive clone happens once per +// repository rather than once per dispatch. +func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { + path, err := w.mirrorPath(repo) + if err != nil { + return "", err + } + if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { + if _, err := gitDir(ctx, path, "fetch", "--prune", "origin"); err != nil { + return "", fmt.Errorf("fetching %s: %w", repo, err) + } + return path, nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", err + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return "", err + } + if _, err := gitDir(ctx, "", "clone", "--mirror", w.remoteURL(repo), path); err != nil { + return "", fmt.Errorf("cloning %s: %w", repo, err) + } + return path, nil +} + +// remoteURL is the URL crq clones from. CRQ_REMOTE_BASE exists so a test can +// point at a local path; in production it is github.com over https, which is +// what the host's credential helper is configured for. +func (w Workspace) remoteURL(repo string) string { + base := strings.TrimSpace(os.Getenv("CRQ_REMOTE_BASE")) + if base == "" { + return "https://github.com/" + NormalizeRepo(repo) + ".git" + } + return strings.TrimRight(base, "/") + "/" + NormalizeRepo(repo) +} + +// Checkout is a worktree at one commit, and the directory git commands for that +// PR run in. +type Checkout struct { + Dir string + Repo string + PR int + mirror string +} + +// Checkout creates a worktree of repo at sha, on a detached HEAD. +// +// Detached on purpose: this is a place to inspect and build, not a branch to +// commit to by accident. A caller that means to write creates its own branch. +func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string) (Checkout, error) { + if strings.TrimSpace(sha) == "" { + return Checkout{}, errors.New("checkout needs a commit") + } + mirror, err := w.Mirror(ctx, repo) + if err != nil { + return Checkout{}, err + } + root, err := w.root() + if err != nil { + return Checkout{}, err + } + owner, name, ok := splitRepo(repo) + if !ok { + return Checkout{}, fmt.Errorf("repo must be owner/name, got %q", repo) + } + dir := filepath.Join(root, "work", fmt.Sprintf("%s-%s-%d", owner, name, pr)) + // A worktree left behind by a killed process would make this fail forever, + // so replace rather than reuse: the head it holds is probably stale anyway. + if err := w.removeWorktree(ctx, mirror, dir); err != nil { + return Checkout{}, err + } + if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { + return Checkout{}, err + } + if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { + return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) + } + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror}, nil +} + +// Remove deletes the worktree. Safe to call on an already-removed one. +func (c Checkout) Remove(ctx context.Context) error { + if c.mirror == "" || c.Dir == "" { + return nil + } + return Workspace{}.removeWorktree(ctx, c.mirror, c.Dir) +} + +func (w Workspace) removeWorktree(ctx context.Context, mirror, dir string) error { + if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) { + // Still prune: git may hold a record of a directory somebody deleted. + _, _ = gitDir(ctx, mirror, "worktree", "prune") + return nil + } + if _, err := gitDir(ctx, mirror, "worktree", "remove", "--force", dir); err != nil { + // Fall back to deleting it ourselves; a stale registration is pruned. + if rmErr := os.RemoveAll(dir); rmErr != nil { + return rmErr + } + } + _, _ = gitDir(ctx, mirror, "worktree", "prune") + return nil +} + +// Git runs a git command inside this checkout. +func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { + return gitDir(ctx, c.Dir, args...) +} + +// gitDir runs git in dir ("" means the process's own directory) and returns its +// trimmed stdout. Stderr is folded into the error, because "exit status 128" on +// its own has never told anybody what went wrong. +func gitDir(ctx context.Context, dir string, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Dir = dir + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + if detail := strings.TrimSpace(stderr.String()); detail != "" { + return "", fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), err, detail) + } + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return strings.TrimSpace(string(out)), nil +} diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go new file mode 100644 index 00000000..a07d4a42 --- /dev/null +++ b/internal/crq/workspace_test.go @@ -0,0 +1,121 @@ +package crq + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// originRepo builds a real repository on disk to clone from, so these tests +// exercise git itself rather than a mock of it — the whole point of this code is +// that the git invocations are right. +func originRepo(t *testing.T, dir string) (sha string) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + run := func(args ...string) string { + t.Helper() + out, err := gitDir(context.Background(), dir, args...) + if err != nil { + t.Fatalf("git %s: %v", strings.Join(args, " "), err) + } + return out + } + run("init", "--initial-branch=main") + run("config", "user.email", "test@example.invalid") + run("config", "user.name", "crq test") + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("one\n"), 0o644); err != nil { + t.Fatal(err) + } + run("add", "README.md") + run("commit", "-m", "first") + return run("rev-parse", "HEAD") +} + +// crq's daemon has no checkout of any repository it reviews, so it needs its own +// place on disk. This covers the whole shape: clone once, fetch after, worktree +// at a commit, and clean up. +func TestWorkspaceChecksOutAHeadWithoutACheckout(t *testing.T) { + // The remote base plus "owner/name" has to resolve to a real repository, so + // lay the origin out the way GitHub does. + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(mirror, "HEAD")); err != nil { + t.Fatalf("mirror is not a git directory: %v", err) + } + + // Second call fetches into the same mirror rather than cloning again: a + // mirror is reused across PRs and rounds, so the clone happens once. + again, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + if again != mirror { + t.Errorf("second Mirror = %q, want the same path %q", again, mirror) + } + + co, err := ws.Checkout(ctx, repo, 7, sha) + if err != nil { + t.Fatal(err) + } + if body, err := os.ReadFile(filepath.Join(co.Dir, "README.md")); err != nil || string(body) != "one\n" { + t.Fatalf("worktree content = %q err=%v, want the committed file", body, err) + } + // Detached on purpose: this is a place to inspect, not a branch to commit to + // by accident. + if branch, err := co.Git(ctx, "rev-parse", "--abbrev-ref", "HEAD"); err != nil || branch != "HEAD" { + t.Errorf("HEAD = %q err=%v, want a detached checkout", branch, err) + } + if head, err := co.Git(ctx, "rev-parse", "HEAD"); err != nil || head != sha { + t.Errorf("checked out %q, want %q", head, sha) + } + + // A worktree left by a killed process must not wedge the next attempt. + if _, err := ws.Checkout(ctx, repo, 7, sha); err != nil { + t.Fatalf("re-checkout over an existing worktree: %v", err) + } + if err := co.Remove(ctx); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(co.Dir); !os.IsNotExist(err) { + t.Errorf("worktree still present after Remove: %v", err) + } + if err := co.Remove(ctx); err != nil { + t.Errorf("removing an already-removed worktree must be harmless, got %v", err) + } +} + +// A repo name is turned into path segments, so anything that could climb out of +// the workspace root has to be refused rather than joined. +func TestWorkspaceRefusesRepoNamesThatEscape(t *testing.T) { + ws := Workspace{Root: t.TempDir()} + for _, repo := range []string{"", "noslash", "../etc/passwd", "owner/../../etc", "owner/", "/name"} { + if _, err := ws.mirrorPath(repo); err == nil { + t.Errorf("mirrorPath(%q) was accepted; it must be refused", repo) + } + } +} + +// The error has to say what git said. "exit status 128" alone has never told +// anybody what went wrong. +func TestGitErrorsCarryStderr(t *testing.T) { + _, err := gitDir(context.Background(), t.TempDir(), "rev-parse", "HEAD") + if err == nil { + t.Fatal("expected an error outside a repository") + } + if !strings.Contains(err.Error(), "rev-parse") || !strings.Contains(strings.ToLower(err.Error()), "git") { + t.Errorf("error %q does not name the command that failed", err) + } +} From 9dfcf4f3ff821cb5c081561e1f582bbe24f23768 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:22:42 +0200 Subject: [PATCH 05/74] Act on a transition instead of logging it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loop had a hole at the end: crq could say a PR needed fixing, and nothing turned that into work. A supervisor script polled, wrote the answer to a file, and logged the change — and then depended on somebody reading the log. Tonight that somebody did not, and eight findings sat for an hour. crq watch drives every open PR through the same next oracle an agent uses and emits one JSON line per PR per pass. With --dispatch, a PR whose action is fix gets a session started for it, in a worktree crq checked out at the head the findings are about, with the findings on disk and the target in the environment. This keeps the queue's non-goal intact. crq still does not decide which findings are real: it starts the session and says which PR to look at, and the session judges. That is why dispatch is a separate command over the same oracle rather than something the pump does, and why it is off unless asked for — watching is an observation, dispatching writes code. Two bounds make it safe to leave running. Every dispatch is claimed under compare-and-swap with a heartbeat, so two watchers cannot both work one PR and a session that dies frees its round after the TTL rather than holding it forever. And attempts are counted per head, so a fix that keeps not working stops instead of spending a review round each time; a new head clears the count, because reaching one means the attempt achieved something. The command runs directly, not through a shell, so nothing expands that the operator did not write. --- cmd/crq/main.go | 67 +++++++++ internal/crq/config.go | 23 +++ internal/crq/state.go | 3 + internal/crq/watch.go | 251 ++++++++++++++++++++++++++++++++ internal/crq/watch_test.go | 107 ++++++++++++++ internal/state/dispatch_test.go | 73 ++++++++++ internal/state/state.go | 71 +++++++++ 7 files changed, 595 insertions(+) create mode 100644 internal/crq/watch.go create mode 100644 internal/crq/watch_test.go create mode 100644 internal/state/dispatch_test.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index b1cd9dbd..7f0a6be2 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -224,6 +224,44 @@ func run(ctx context.Context, args []string) int { } printJSON(result) return 0 + case "watch": + fs := flag.NewFlagSet("watch", flag.ContinueOnError) + fs.SetOutput(os.Stderr) + dispatch := fs.Bool("dispatch", false, "start a fix session for a PR that needs one") + once := fs.Bool("once", false, "run one pass and exit") + interval := fs.Duration("interval", 0, "time between passes") + attempts := fs.Int("max-attempts", 0, "dispatches allowed per head") + if err := fs.Parse(args[1:]); err != nil { + return 1 + } + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + opts := crq.WatchOptions{ + Dispatch: *dispatch, Once: *once, + Interval: *interval, MaxAttempts: *attempts, + } + // Everything after the flags is either repositories or, past a --, the + // fix session's argv. + rest := fs.Args() + for i, arg := range rest { + if arg == "--" { + opts.Command = rest[i+1:] + rest = rest[:i] + break + } + } + opts.Repos = rest + enc := json.NewEncoder(os.Stdout) + werr := service.Watch(ctx, opts, func(e crq.WatchEvent) { + _ = enc.Encode(e) + }) + if werr != nil && !errors.Is(werr, context.Canceled) { + fatal(werr) + return 1 + } + return 0 case "autoreview", "auto": fs := flag.NewFlagSet("autoreview", flag.ContinueOnError) fs.SetOutput(os.Stderr) @@ -356,6 +394,9 @@ 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 watch [--dispatch] [--once] [...] [-- ] + drive open PRs through crq next; --dispatch starts a + session to fix the ones that need it crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -509,6 +550,32 @@ 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 "watch": + fmt.Print(`crq watch [--dispatch] [--once] [--interval ] [--max-attempts ] [...] [-- ...] + +Drive every open PR through the same crq next oracle an agent uses, and emit one +JSON line per PR per pass. Without --dispatch that is all it does: watching is an +observation. + +With --dispatch, a PR whose action is "fix" gets a session started for it, in a +worktree crq checked out at that head, with: + + CRQ_DISPATCH_REPO owner/name + CRQ_DISPATCH_PR the number + CRQ_DISPATCH_HEAD the commit the findings are about + CRQ_DISPATCH_FINDINGS path to the findings as JSON + +The command comes from CRQ_DISPATCH_CMD or from everything after --. It is run +directly, not through a shell, so nothing expands that you did not write. + +crq still does not decide which findings are real — it starts the session and +says which PR to look at; the session judges. Every dispatch is claimed under +compare-and-swap, so two watchers cannot both work one PR, and bounded per head +(CRQ_DISPATCH_MAX_ATTEMPTS, default 3) so a fix that keeps not working stops +instead of spending a review round each time. + +Repositories default to CRQ_REPOS. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] diff --git a/internal/crq/config.go b/internal/crq/config.go index 9d24f267..3bb49815 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -48,6 +48,14 @@ type Config struct { // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. Reviewers []Reviewer + // WatchInterval paces `crq watch`; DispatchCommand is the fix session it + // runs with --dispatch, argv-style; DispatchMaxAttempts bounds dispatches per + // head so a fix that keeps not working stops. WorkspaceRoot holds crq's own + // mirrors and worktrees. + WatchInterval time.Duration + DispatchCommand []string + DispatchMaxAttempts int + WorkspaceRoot string // WorkDir is the checkout the local-work probe inspects. Empty means the // process's own directory, which is what an agent running crq from its // working copy means. Set programmatically by a caller working in a @@ -177,6 +185,10 @@ func LoadConfig() (Config, error) { AutoReviewMaxScan: intEnv(env, "CRQ_AUTOREVIEW_MAX_SCAN", 400), LeaderTTL: durationEnv(env, "CRQ_LEADER_TTL", 3*time.Minute), FiredMax: intEnv(env, "CRQ_FIRED_MAX", 500), + WatchInterval: durationEnv(env, "CRQ_WATCH_INTERVAL", 2*time.Minute), + DispatchCommand: splitArgv(env["CRQ_DISPATCH_CMD"]), + DispatchMaxAttempts: intEnv(env, "CRQ_DISPATCH_MAX_ATTEMPTS", 3), + WorkspaceRoot: env["CRQ_WORKSPACE"], NoOpen: env["CRQ_NO_OPEN"] != "", DryRun: env["CRQ_DRY_RUN"] == "1", FeedbackWaitTimeout: durationEnv(env, "CRQ_FEEDBACK_WAIT_TIMEOUT", 20*time.Minute), @@ -485,3 +497,14 @@ func ownerOf(repo string) string { owner, _, _ := strings.Cut(repo, "/") return owner } + +// splitArgv splits a configured command into argv on whitespace. It is +// deliberately not a shell: a dispatch command runs directly, so nothing here +// expands a variable, a glob, or a pipe that the operator did not write. +func splitArgv(value string) []string { + fields := strings.Fields(value) + if len(fields) == 0 { + return nil + } + return fields +} diff --git a/internal/crq/state.go b/internal/crq/state.go index d6cbfc35..4cab05ea 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -33,6 +33,9 @@ const ( PhaseAwaitingRetry = crqstate.PhaseAwaitingRetry PhaseCompleted = crqstate.PhaseCompleted PhaseAbandoned = crqstate.PhaseAbandoned + + // DispatchTTL is how long a watcher's fix claim survives without a heartbeat. + DispatchTTL = crqstate.DispatchTTL ) var ( diff --git a/internal/crq/watch.go b/internal/crq/watch.go new file mode 100644 index 00000000..2c7fc068 --- /dev/null +++ b/internal/crq/watch.go @@ -0,0 +1,251 @@ +package crq + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/url" + "os" + "os/exec" + "path/filepath" + "sort" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// WatchOptions configures `crq watch`. +type WatchOptions struct { + // Repos to watch. Empty means every repository in CRQ_REPOS. + Repos []string + // Interval between passes. + Interval time.Duration + // Once runs a single pass and returns. + Once bool + // Dispatch turns "a PR needs fixing" into a session that fixes it. Off by + // default: watching is an observation, dispatching writes code. + Dispatch bool + // Command is the fix session to run, argv-style. Empty means CRQ_DISPATCH_CMD. + Command []string + // MaxAttempts bounds dispatches per head. 0 means the configured default. + MaxAttempts int +} + +// WatchEvent is one PR's state at a pass, and what the watcher did about it. +type WatchEvent struct { + Repo string `json:"repo"` + PR int `json:"pr"` + Action string `json:"action"` + Reason string `json:"reason,omitempty"` + Findings int `json:"findings"` + // Dispatched says a fix session was started for this event; Skipped says why + // one was not, when dispatch was enabled and the action asked for it. + Dispatched bool `json:"dispatched,omitempty"` + Skipped string `json:"skipped,omitempty"` + At time.Time `json:"at"` +} + +// Watch drives every open PR in scope through the same `next` oracle an agent +// uses, and — with Dispatch — starts a session to fix the ones that need it. +// +// The queue's stated non-goal is that crq does not write code or decide which +// findings are real. Dispatch does not change that: crq starts a session and +// tells it which PR to look at; the session does the judging. That is why this +// is a separate command over the same oracle rather than something the pump +// does, and why it is off unless asked for. +// +// Every dispatch is claimed under CAS, so two watchers cannot both spawn a +// session for one PR, and bounded per head, so a fix that keeps not working +// stops instead of spending a review round each time. +func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchEvent)) error { + if opts.Interval <= 0 { + opts.Interval = s.cfg.WatchInterval + } + if opts.MaxAttempts <= 0 { + opts.MaxAttempts = s.cfg.DispatchMaxAttempts + } + if opts.Dispatch && len(opts.Command) == 0 { + opts.Command = s.cfg.DispatchCommand + } + if opts.Dispatch && len(opts.Command) == 0 { + return errors.New("dispatch needs a command: set CRQ_DISPATCH_CMD, or pass one after --") + } + for { + if err := s.watchPass(ctx, opts, emit); err != nil { + if _, ok := ghapi.ThrottleWait(err); !ok { + return err + } + if s.log != nil { + s.log.Printf("watch: %v; waiting it out", err) + } + } + if opts.Once { + return nil + } + if err := ghapi.SleepCtx(ctx, opts.Interval); err != nil { + return err + } + } +} + +func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(WatchEvent)) error { + repos := opts.Repos + if len(repos) == 0 { + for repo := range s.cfg.AllowRepos { + repos = append(repos, repo) + } + sort.Strings(repos) // stable order: a pass must not depend on map iteration + } + if len(repos) == 0 { + return errors.New("nothing to watch: pass a repository, or set CRQ_REPOS") + } + for _, repo := range repos { + pulls, err := s.gh.ListPulls(ctx, repo, openPullQuery()) + if err != nil { + return err + } + for _, pull := range pulls { + if err := ctx.Err(); err != nil { + return err + } + report, err := s.Next(ctx, repo, pull.Number) + if err != nil { + if _, ok := ghapi.ThrottleWait(err); ok { + return err + } + if s.log != nil { + s.log.Printf("watch: %s#%d: %v", repo, pull.Number, err) + } + continue + } + event := WatchEvent{ + Repo: repo, PR: pull.Number, + Action: report.Action, Reason: report.Reason, + Findings: len(report.Findings), At: s.clock().UTC(), + } + if opts.Dispatch && report.Action == string(engine.ActionFix) { + event.Dispatched, event.Skipped = s.dispatch(ctx, opts, report) + } + if emit != nil { + emit(event) + } + } + } + return nil +} + +// dispatch claims the round, checks the head out, and runs the fix session. +func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextReport) (bool, string) { + token := randomToken() + claimed, why := s.claimDispatch(ctx, report, token, opts.MaxAttempts) + if !claimed { + return false, why + } + defer s.releaseDispatch(context.WithoutCancel(ctx), report, token) + + co, err := s.workspace().Checkout(ctx, report.Repo, report.PR, report.Head) + if err != nil { + return false, "checkout failed: " + err.Error() + } + defer func() { _ = co.Remove(context.WithoutCancel(ctx)) }() + + // The session gets the findings as a file rather than an argument: they are + // long, and a shell would be the wrong place to carry them. + findingsPath := filepath.Join(co.Dir, ".crq-findings.json") + body, err := json.MarshalIndent(report.Findings, "", " ") + if err != nil { + return false, err.Error() + } + if err := os.WriteFile(findingsPath, body, 0o600); err != nil { + return false, err.Error() + } + + stop := s.beatDispatch(ctx, report, token) + defer stop() + + cmd := exec.CommandContext(ctx, opts.Command[0], opts.Command[1:]...) + cmd.Dir = co.Dir + cmd.Env = append(os.Environ(), + "CRQ_DISPATCH_REPO="+report.Repo, + fmt.Sprintf("CRQ_DISPATCH_PR=%d", report.PR), + "CRQ_DISPATCH_HEAD="+report.Head, + "CRQ_DISPATCH_FINDINGS="+findingsPath, + ) + if s.log != nil { + s.log.Printf("watch: dispatching %s for %s#%d@%s (%d findings)", + opts.Command[0], report.Repo, report.PR, report.Head, len(report.Findings)) + } + if err := cmd.Run(); err != nil { + return false, "fix session failed: " + err.Error() + } + return true, "" +} + +func (s *Service) claimDispatch(ctx context.Context, report NextReport, token string, maxAttempts int) (bool, string) { + reason := "" + _, err := s.store.Update(ctx, func(st *State) error { + round := st.Round(report.Repo, report.PR) + if round == nil || round.Head != report.Head { + reason = "no round for this head" + return ErrNoChange + } + ok, why := round.ClaimDispatch(s.cfg.Host, token, s.clock(), maxAttempts) + if !ok { + reason = why + return ErrNoChange + } + st.PutRound(*round) + return nil + }) + if err != nil { + return false, err.Error() + } + return reason == "", reason +} + +func (s *Service) releaseDispatch(ctx context.Context, report NextReport, token string) { + _, _ = s.store.Update(ctx, func(st *State) error { + round := st.Round(report.Repo, report.PR) + if round == nil || !round.ReleaseDispatch(token) { + return ErrNoChange + } + st.PutRound(*round) + return nil + }) +} + +// beatDispatch refreshes the claim while the session runs, so a session that +// outlives the TTL keeps its round and a crashed watcher's does not. +func (s *Service) beatDispatch(ctx context.Context, report NextReport, token string) func() { + beatCtx, cancel := context.WithCancel(ctx) + go func() { + ticker := time.NewTicker(DispatchTTL / 3) + defer ticker.Stop() + for { + select { + case <-beatCtx.Done(): + return + case <-ticker.C: + _, _ = s.store.Update(beatCtx, func(st *State) error { + round := st.Round(report.Repo, report.PR) + if round == nil || !round.HeartbeatDispatch(token, s.clock()) { + return ErrNoChange + } + st.PutRound(*round) + return nil + }) + } + } + }() + return cancel +} + +func (s *Service) workspace() Workspace { return Workspace{Root: s.cfg.WorkspaceRoot} } + +func openPullQuery() url.Values { + q := url.Values{} + q.Set("state", "open") + return q +} diff --git a/internal/crq/watch_test.go b/internal/crq/watch_test.go new file mode 100644 index 00000000..786f845e --- /dev/null +++ b/internal/crq/watch_test.go @@ -0,0 +1,107 @@ +package crq + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// Dispatch is the one place crq starts something that writes code, so what it +// hands the session has to be right: the PR's own worktree at the head the +// findings are about, and the findings themselves. +func TestWatchDispatchesAFixSessionWithItsContext(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + + cfg := firingConfig() + cfg.WorkspaceRoot = t.TempDir() + cfg.AllowRepos = map[string]bool{repo: true} + gh := newFakeGitHub() + gh.graphQL = noForcePush + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + + var pull ghapi.Pull + pull.State = "open" + pull.Number = 4 + pull.Head.SHA = sha + gh.pulls[fakeKey(repo, 4)] = pull + + // A record of what the session was given, written by the "session" itself. + record := filepath.Join(t.TempDir(), "record.json") + script := filepath.Join(t.TempDir(), "session.sh") + body := "#!/bin/sh\n" + + "printf '{\"repo\":\"%s\",\"pr\":\"%s\",\"head\":\"%s\",\"cwd\":\"%s\",\"findings\":\"%s\"}' " + + "\"$CRQ_DISPATCH_REPO\" \"$CRQ_DISPATCH_PR\" \"$CRQ_DISPATCH_HEAD\" \"$(pwd)\" \"$CRQ_DISPATCH_FINDINGS\" > " + record + "\n" + if err := os.WriteFile(script, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + + report := NextReport{Repo: repo, PR: 4, Head: sha, Action: "fix"} + seedRound(t, store, cfg, repo, 4, sha, PhaseQueued, time.Now().UTC(), 0) + + ok, why := svc.dispatch(context.Background(), WatchOptions{ + Dispatch: true, Command: []string{script}, MaxAttempts: 3, + }, report) + if !ok { + t.Fatalf("dispatch did not run: %s", why) + } + + var got struct{ Repo, PR, Head, Cwd, Findings string } + data, err := os.ReadFile(record) + if err != nil { + t.Fatalf("the session did not run: %v", err) + } + if err := json.Unmarshal(data, &got); err != nil { + t.Fatal(err) + } + if got.Repo != repo || got.PR != "4" || got.Head != sha { + t.Errorf("session context = %+v, want %s#4@%s", got, repo, sha) + } + // It ran in a checkout of the PR, not in whatever directory crq was started + // from — the entire reason the workspace exists. + if !strings.HasPrefix(got.Cwd, cfg.WorkspaceRoot) { + t.Errorf("session ran in %q, want a worktree under %q", got.Cwd, cfg.WorkspaceRoot) + } + if got.Findings == "" { + t.Error("the session was given no findings file") + } + + // The claim is released afterwards, so the next round is not blocked by a + // session that already finished. + st, _, err := store.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + round := st.Round(repo, 4) + if round == nil || round.DispatchHeld(time.Now().UTC()) { + t.Errorf("claim still held after the session finished: %#v", round.Dispatch) + } + if round.Dispatch == nil || round.Dispatch.Attempts != 1 { + t.Errorf("attempts = %#v, want 1 recorded", round.Dispatch) + } + // And the worktree is cleaned up rather than left to accumulate. + if entries, err := os.ReadDir(filepath.Join(cfg.WorkspaceRoot, "work")); err == nil && len(entries) != 0 { + t.Errorf("worktrees left behind: %v", entries) + } +} + +// Watching is an observation; dispatching writes code. Asking for the second +// without saying what to run must fail loudly rather than silently watch. +func TestWatchRefusesDispatchWithNoCommand(t *testing.T) { + cfg := firingConfig() + cfg.DispatchCommand = nil + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + err := svc.Watch(context.Background(), WatchOptions{Dispatch: true, Once: true}, nil) + if err == nil || !strings.Contains(err.Error(), "command") { + t.Errorf("err = %v, want a refusal naming the missing command", err) + } +} diff --git a/internal/state/dispatch_test.go b/internal/state/dispatch_test.go new file mode 100644 index 00000000..62bb9860 --- /dev/null +++ b/internal/state/dispatch_test.go @@ -0,0 +1,73 @@ +package state + +import ( + "testing" + "time" +) + +// The claim is what keeps dispatch from being dangerous: two watchers must not +// both spawn a session for one PR, a dead session must not hold a round for +// ever, and a fix that keeps not working must stop rather than loop. +func TestDispatchClaim(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + round := &Round{Repo: "o/r", PR: 1, Head: "aaaaaaaa1"} + + if ok, why := round.ClaimDispatch("host-a", "tok-a", now, 3); !ok { + t.Fatalf("first claim refused: %s", why) + } + if ok, why := round.ClaimDispatch("host-b", "tok-b", now, 3); ok { + t.Error("a second watcher took a live claim") + } else if why == "" { + t.Error("a refusal must say why") + } + + // A heartbeat keeps a long session's claim alive. + if !round.HeartbeatDispatch("tok-a", now.Add(9*time.Minute)) { + t.Error("the owner must be able to heartbeat") + } + if round.HeartbeatDispatch("tok-b", now.Add(9*time.Minute)) { + t.Error("a heartbeat from another token must be refused") + } + if ok, _ := round.ClaimDispatch("host-b", "tok-b", now.Add(10*time.Minute), 3); ok { + t.Error("a heartbeated claim must not be stolen") + } + + // A dead session's claim is taken over once its heartbeat goes stale, and the + // attempt it already spent is not forgotten. + stale := now.Add(9*time.Minute + DispatchTTL) + ok, why := round.ClaimDispatch("host-b", "tok-b", stale, 3) + if !ok { + t.Fatalf("a stale claim must be takeable: %s", why) + } + if round.Dispatch.Attempts != 2 { + t.Errorf("attempts = %d, want 2 — the dead session's attempt still happened", round.Dispatch.Attempts) + } + + // Releasing frees the round but keeps the count. + if !round.ReleaseDispatch("tok-b") { + t.Fatal("the owner must be able to release") + } + if round.DispatchHeld(stale) { + t.Error("a released claim must not read as held") + } + if ok, _ := round.ClaimDispatch("host-c", "tok-c", stale, 3); !ok { + t.Fatal("a released round must be claimable") + } + if round.Dispatch.Attempts != 3 { + t.Errorf("attempts = %d, want 3", round.Dispatch.Attempts) + } + + // And the bound holds: a fix that keeps not working stops. + round.ReleaseDispatch("tok-c") + if ok, why := round.ClaimDispatch("host-c", "tok-d", stale, 3); ok { + t.Error("the attempt bound must stop a fourth dispatch") + } else if why == "" { + t.Error("the bound's refusal must say why") + } + + // A new head is a fresh start: the previous attempt achieved something. + fresh := &Round{Repo: "o/r", PR: 1, Head: "bbbbbbbb2"} + if ok, _ := fresh.ClaimDispatch("host-c", "tok-e", stale, 3); !ok { + t.Error("a new head must be dispatchable again") + } +} diff --git a/internal/state/state.go b/internal/state/state.go index ede4e853..e23286ae 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -79,6 +79,12 @@ type Round struct { // CodeRabbit requests must skip these. CoOnly bool `json:"co_only,omitempty"` + // Dispatch is the claim held by a watcher running a fix for this round. It + // exists so two watchers cannot both spawn a session for the same PR, and so + // a session that dies does not hold the round forever — the claim is + // heartbeated, and one older than DispatchTTL is free to take over. + Dispatch *DispatchClaim `json:"dispatch,omitempty"` + // RetryAt is the earliest time this head may fire again (awaiting_retry). RetryAt *time.Time `json:"retry_at,omitempty"` @@ -605,6 +611,71 @@ func (s *State) QueuedRounds(now time.Time) []Round { return out } +// DispatchTTL is how long a dispatch claim survives without a heartbeat. A fix +// session outlives any single poll, so the claim is refreshed while one runs; +// this bounds how long a crashed watcher blocks the next attempt. +const DispatchTTL = 10 * time.Minute + +// DispatchClaim records who is running a fix for a round, and how many attempts +// this head has had. +type DispatchClaim struct { + Host string `json:"host"` + // Token distinguishes two claims from the same host, so a restarted watcher + // cannot heartbeat or release the claim of the process it replaced. + Token string `json:"token"` + At time.Time `json:"at"` + Heartbeat time.Time `json:"heartbeat"` + // Attempts counts dispatches for THIS head, so a fix that keeps failing + // stops instead of looping. It survives release; only a new head clears it, + // because a new head means the previous attempt achieved something. + Attempts int `json:"attempts,omitempty"` +} + +// DispatchHeld reports whether a live claim exists. +func (r *Round) DispatchHeld(now time.Time) bool { + return r.Dispatch != nil && !r.Dispatch.Heartbeat.IsZero() && now.UTC().Sub(r.Dispatch.Heartbeat) < DispatchTTL +} + +// ClaimDispatch takes this round's dispatch claim, or reports why it cannot. A +// claim past its TTL is taken over, keeping the attempt count: that session died, +// but its attempt still happened. +func (r *Round) ClaimDispatch(host, token string, now time.Time, maxAttempts int) (bool, string) { + now = now.UTC() + attempts := 0 + if r.Dispatch != nil { + if r.DispatchHeld(now) { + return false, "another watcher is already fixing this round" + } + attempts = r.Dispatch.Attempts + } + if maxAttempts > 0 && attempts >= maxAttempts { + return false, fmt.Sprintf("%d dispatch attempts already made for this head", attempts) + } + r.Dispatch = &DispatchClaim{Host: host, Token: token, At: now, Heartbeat: now, Attempts: attempts + 1} + return true, "" +} + +// HeartbeatDispatch refreshes a claim this token owns. False means the claim is +// gone or belongs to somebody else, which is how a watcher learns it was taken +// over and should stop. +func (r *Round) HeartbeatDispatch(token string, now time.Time) bool { + if r.Dispatch == nil || r.Dispatch.Token != token { + return false + } + r.Dispatch.Heartbeat = now.UTC() + return true +} + +// ReleaseDispatch drops a claim this token owns, keeping the attempt count. +func (r *Round) ReleaseDispatch(token string) bool { + if r.Dispatch == nil || r.Dispatch.Token != token { + return false + } + r.Dispatch.Heartbeat = time.Time{} + r.Dispatch.Token = "" + return true +} + // 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 ( From b0506d379b6a2cbc8e951b7512ac527c34e30486 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:31:48 +0200 Subject: [PATCH 06/74] Keep one checkout from destroying another, and private source private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review findings on the workspace, two of them P1. The mirror tree was created with the umask's 0755, so on a shared host git wrote a private repository's objects and source world-readable under ~/.cache. The root is 0700 now, enforced rather than assumed. Clones were unauthenticated in the documented token-only setup: git does not read GITHUB_TOKEN or GH_TOKEN by itself, so a daemon with no credential helper would have failed at its first private checkout — dispatch's first real act. A credential helper is injected when crq has a token, and the token travels in the environment, never in argv, so a process listing, a log line and this package's own error strings carry the helper snippet and not the secret. Three ways a checkout could destroy another. Two workers first cloning one repository both passed the missing-HEAD check and cloned into the same directory; the clone now lands in a staging directory and is moved into place, and losing that race is fine because the winner's mirror is just as good. "a-b/c" and "a/b-c" joined with a dash are the same path, so one repository's cleanup deleted the other's live worktree; owner and name stay separate components. And a deferred Remove on a stale handle deleted the checkout that replaced it — each checkout now owns a generation directory and removes only its own. CRQ_WORKSPACE is read through Config, so a value in ~/.config/crq/env is actually used instead of being silently ignored by the daemon. --- internal/crq/config.go | 5 ++ internal/crq/workspace.go | 138 +++++++++++++++++++++++++++++---- internal/crq/workspace_test.go | 93 ++++++++++++++++++++++ 3 files changed, 221 insertions(+), 15 deletions(-) diff --git a/internal/crq/config.go b/internal/crq/config.go index 9d24f267..719d5a76 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -48,6 +48,10 @@ type Config struct { // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. Reviewers []Reviewer + // WorkspaceRoot holds crq's own mirrors and worktrees. Read here rather than + // from the process environment, so a value in ~/.config/crq/env — the + // documented place for crq settings — is actually used. + WorkspaceRoot string // WorkDir is the checkout the local-work probe inspects. Empty means the // process's own directory, which is what an agent running crq from its // working copy means. Set programmatically by a caller working in a @@ -177,6 +181,7 @@ func LoadConfig() (Config, error) { AutoReviewMaxScan: intEnv(env, "CRQ_AUTOREVIEW_MAX_SCAN", 400), LeaderTTL: durationEnv(env, "CRQ_LEADER_TTL", 3*time.Minute), FiredMax: intEnv(env, "CRQ_FIRED_MAX", 500), + WorkspaceRoot: env["CRQ_WORKSPACE"], NoOpen: env["CRQ_NO_OPEN"] != "", DryRun: env["CRQ_DRY_RUN"] == "1", FeedbackWaitTimeout: durationEnv(env, "CRQ_FEEDBACK_WAIT_TIMEOUT", 20*time.Minute), diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 455cb073..12fcc227 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" ) @@ -30,6 +31,11 @@ type Workspace struct { // Root holds the mirrors and worktrees. Empty means the default cache // location (see DefaultWorkspaceRoot). Root string + // Token authenticates clones and fetches. Empty falls back to whatever git + // credential helper the host already has, which is the right answer on a + // developer machine; a daemon configured with GITHUB_TOKEN alone has no + // helper, and git does not read that variable by itself. + Token string } // DefaultWorkspaceRoot is $XDG_CACHE_HOME/crq (or ~/.cache/crq). @@ -44,6 +50,21 @@ func DefaultWorkspaceRoot() (string, error) { return filepath.Join(cache, "crq"), nil } +// git runs a git command for this workspace, injecting credentials when a token +// is configured. +// +// The token travels in the ENVIRONMENT, never in argv: the helper below is a +// shell snippet that reads it, so a process listing, a log line and this +// package's own error strings all carry the snippet and never the secret. +func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, error) { + if strings.TrimSpace(w.Token) == "" { + return gitDir(ctx, dir, args...) + } + const helper = `!f() { test "$1" = get && printf 'username=x-access-token\npassword=%s\n' "$CRQ_GIT_TOKEN"; }; f` + full := append([]string{"-c", "credential.helper=", "-c", "credential.helper=" + helper}, args...) + return gitEnv(ctx, dir, []string{"CRQ_GIT_TOKEN=" + w.Token}, full...) +} + func (w Workspace) root() (string, error) { if strings.TrimSpace(w.Root) != "" { return w.Root, nil @@ -89,20 +110,45 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if err != nil { return "", err } + // 0700, not the umask's 0755: a mirror of a private repository is private + // source, and on a shared host the default would let any local user read it. + if root, rerr := w.root(); rerr == nil { + if err := os.MkdirAll(root, 0o700); err != nil { + return "", err + } + if err := os.Chmod(root, 0o700); err != nil { + return "", err + } + } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - if _, err := gitDir(ctx, path, "fetch", "--prune", "origin"); err != nil { + if _, err := w.git(ctx, path, "fetch", "--prune", "origin"); err != nil { return "", fmt.Errorf("fetching %s: %w", repo, err) } return path, nil } else if !errors.Is(err, os.ErrNotExist) { return "", err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return "", err + } + // Clone somewhere else and move it into place, so two workers starting on the + // same repository at once cannot clone into one directory — the second would + // fail with "destination path already exists" and take the caller with it. + staging, err := os.MkdirTemp(filepath.Dir(path), ".clone-") + if err != nil { return "", err } - if _, err := gitDir(ctx, "", "clone", "--mirror", w.remoteURL(repo), path); err != nil { + defer os.RemoveAll(staging) + pending := filepath.Join(staging, "mirror.git") + if _, err := w.git(ctx, "", "clone", "--mirror", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } + if err := os.Rename(pending, path); err != nil { + if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { + return path, nil // somebody else won the race; their mirror is as good + } + return "", err + } return path, nil } @@ -124,6 +170,10 @@ type Checkout struct { Repo string PR int mirror string + // token makes this handle's directory its own. Two checkouts of one PR + // otherwise share a path, and a deferred Remove on the older handle deletes + // the newer one's worktree out from under it. + token string } // Checkout creates a worktree of repo at sha, on a detached HEAD. @@ -138,32 +188,64 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err != nil { return Checkout{}, err } - root, err := w.root() + prDir, err := w.workPath(repo, pr) if err != nil { return Checkout{}, err } - owner, name, ok := splitRepo(repo) - if !ok { - return Checkout{}, fmt.Errorf("repo must be owner/name, got %q", repo) - } - dir := filepath.Join(root, "work", fmt.Sprintf("%s-%s-%d", owner, name, pr)) - // A worktree left behind by a killed process would make this fail forever, - // so replace rather than reuse: the head it holds is probably stale anyway. - if err := w.removeWorktree(ctx, mirror, dir); err != nil { + // Anything already here belongs to an earlier checkout of this PR, which is + // finished or dead either way. Clearing it keeps the generation directories + // from accumulating without letting a live handle delete a newer sibling. + if err := w.clearWork(ctx, mirror, prDir); err != nil { return Checkout{}, err } - if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { + token := randomToken() + dir := filepath.Join(prDir, token) + if err := os.MkdirAll(prDir, 0o700); err != nil { return Checkout{}, err } if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } - return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror}, nil + return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, token: token}, nil +} + +// workPath is the directory holding this PR's checkouts. Owner and name stay +// separate path components: joined with a dash, "a-b/c" and "a/b-c" would +// collide, and one repository's cleanup would delete the other's live worktree. +func (w Workspace) workPath(repo string, pr int) (string, error) { + root, err := w.root() + if err != nil { + return "", err + } + owner, name, ok := splitRepo(repo) + if !ok { + return "", fmt.Errorf("repo must be owner/name, got %q", repo) + } + return filepath.Join(root, "work", owner, name, strconv.Itoa(pr)), nil +} + +// clearWork removes every checkout under prDir and deregisters it. +func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { + entries, err := os.ReadDir(prDir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + for _, entry := range entries { + if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { + return err + } + } + return nil } // Remove deletes the worktree. Safe to call on an already-removed one. func (c Checkout) Remove(ctx context.Context) error { - if c.mirror == "" || c.Dir == "" { + if c.mirror == "" || c.Dir == "" || filepath.Base(c.Dir) != c.token { + // Not this handle's directory: a later checkout replaced it, and removing + // it would delete a worktree somebody else is using. return nil } return Workspace{}.removeWorktree(ctx, c.mirror, c.Dir) @@ -194,8 +276,16 @@ func (c Checkout) Git(ctx context.Context, args ...string) (string, error) { // trimmed stdout. Stderr is folded into the error, because "exit status 128" on // its own has never told anybody what went wrong. func gitDir(ctx context.Context, dir string, args ...string) (string, error) { + return gitEnv(ctx, dir, nil, args...) +} + +// gitEnv is gitDir with extra environment entries. +func gitEnv(ctx context.Context, dir string, env []string, args ...string) (string, error) { cmd := exec.CommandContext(ctx, "git", args...) cmd.Dir = dir + if len(env) > 0 { + cmd.Env = append(os.Environ(), env...) + } var stderr strings.Builder cmd.Stderr = &stderr out, err := cmd.Output() @@ -207,3 +297,21 @@ func gitDir(ctx context.Context, dir string, args ...string) (string, error) { } return strings.TrimSpace(string(out)), nil } + +// workspace is crq's workspace as this configuration describes it: the root from +// the config file (not just the process environment) and the token crq already +// resolved for the API, so a daemon with GITHUB_TOKEN alone can still clone. +func (s *Service) workspace() Workspace { + return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken()} +} + +// gitToken is the token to authenticate git with, or "" to leave git to the +// host's own credential helper. +func gitToken() string { + for _, name := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { + if value := strings.TrimSpace(os.Getenv(name)); value != "" { + return value + } + } + return "" +} diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index a07d4a42..6a14e589 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -119,3 +119,96 @@ func TestGitErrorsCarryStderr(t *testing.T) { t.Errorf("error %q does not name the command that failed", err) } } + +// Four ways a workspace could destroy or expose something it should not. +func TestWorkspaceIsolatesCheckouts(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + root := t.TempDir() + ws := Workspace{Root: root} + ctx := context.Background() + + // A stale handle must not delete the checkout that replaced it. + first, err := ws.Checkout(ctx, "owner/thing", 9, sha) + if err != nil { + t.Fatal(err) + } + second, err := ws.Checkout(ctx, "owner/thing", 9, sha) + if err != nil { + t.Fatal(err) + } + if first.Dir == second.Dir { + t.Fatal("two checkouts of one PR share a directory, so either can delete the other") + } + if err := first.Remove(ctx); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(second.Dir); err != nil { + t.Errorf("the live checkout was removed by a stale handle: %v", err) + } + + // Repo names that differ only in where the slash falls must not collide: + // "a-b/c" and "a/b-c" joined with a dash are the same string. + sha2 := originRepo(t, filepath.Join(base, "a-b/c")) + sha3 := originRepo(t, filepath.Join(base, "a/b-c")) + one, err := ws.Checkout(ctx, "a-b/c", 1, sha2) + if err != nil { + t.Fatal(err) + } + two, err := ws.Checkout(ctx, "a/b-c", 1, sha3) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(one.Dir); err != nil { + t.Errorf("checking out a/b-c destroyed a-b/c's worktree: %v", err) + } + if strings.HasPrefix(two.Dir, one.Dir) || strings.HasPrefix(one.Dir, two.Dir) { + t.Errorf("checkout paths overlap: %q and %q", one.Dir, two.Dir) + } + + // A mirror of a private repository is private source: on a shared host the + // umask default would let any local user read it. + info, err := os.Stat(root) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o700 { + t.Errorf("workspace root mode = %o, want 0700", perm) + } +} + +// Two workers starting on the same repository at once must not clone into one +// directory — the second used to fail with "destination path already exists". +func TestMirrorSurvivesAConcurrentFirstClone(t *testing.T) { + base := t.TempDir() + originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + + errs := make(chan error, 4) + for i := 0; i < 4; i++ { + go func() { + _, err := ws.Mirror(context.Background(), "owner/thing") + errs <- err + }() + } + for i := 0; i < 4; i++ { + if err := <-errs; err != nil { + t.Errorf("concurrent first clone: %v", err) + } + } +} + +// The token must never reach argv: a process listing, a log line and this +// package's own error strings would all carry it. +func TestGitTokenTravelsInTheEnvironment(t *testing.T) { + ws := Workspace{Root: t.TempDir(), Token: "ghp_secret_value"} + _, err := ws.git(context.Background(), t.TempDir(), "rev-parse", "HEAD") + if err == nil { + t.Fatal("expected an error outside a repository") + } + if strings.Contains(err.Error(), "ghp_secret_value") { + t.Errorf("the token leaked into an error message: %v", err) + } +} From c1f63aa5581c3d767334531782718509e160f099 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:43:25 +0200 Subject: [PATCH 07/74] 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 3ca7b869..90e98b3f 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 24ff2a8d..31d70fe8 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 582f84f2..195f43a1 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 b10e2ac5..29b48509 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 9bff9ecdf24bffc98a2834bb791094855e748bd2 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:44:40 +0200 Subject: [PATCH 08/74] Authenticate the way the API client already does Four findings, one P1. Reading only GITHUB_TOKEN and GH_TOKEN meant the documented `gh auth login` setup produced unauthenticated clones: API calls worked, and every private checkout failed at dispatch's first real act. git now gets the same token the API client resolves, `gh auth token` included. A relative workspace root put the worktree somewhere other than where the returned path said. `git worktree add` runs inside the mirror, so the relative directory landed under the mirror while Checkout.Dir pointed at a path that did not exist. Roots are absolute now. Clearing a PR's directory before making a new generation force-removed a checkout another worker might be building in. Old generations are pruned by age instead, which collects what a killed process left without touching a live session. And two workers fetching one mirror race on git's ref locks, so the loser reported "cannot lock ref" although the winner had just made the mirror current. That retries briefly and then accepts the mirror as it stands, rather than failing a dispatch over a fetch somebody else finished. --- internal/crq/workspace.go | 81 +++++++++++++++++++++++++--------- internal/crq/workspace_test.go | 52 ++++++++++++++++++++++ internal/gh/github.go | 5 +++ 3 files changed, 118 insertions(+), 20 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 12fcc227..00f453c0 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -9,6 +9,9 @@ import ( "path/filepath" "strconv" "strings" + "time" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" ) // Workspace is crq's own place on disk for a repository. @@ -66,10 +69,17 @@ func (w Workspace) git(ctx context.Context, dir string, args ...string) (string, } func (w Workspace) root() (string, error) { - if strings.TrimSpace(w.Root) != "" { - return w.Root, nil + root := strings.TrimSpace(w.Root) + if root == "" { + var err error + if root, err = DefaultWorkspaceRoot(); err != nil { + return "", err + } } - return DefaultWorkspaceRoot() + // Absolute, always: `git worktree add` runs inside the mirror, so a relative + // path would create the worktree under the mirror while the path handed back + // points at a directory that does not exist. + return filepath.Abs(root) } // mirrorPath is where repo's bare mirror lives. The owner and name are path @@ -121,10 +131,20 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { - if _, err := w.git(ctx, path, "fetch", "--prune", "origin"); err != nil { - return "", fmt.Errorf("fetching %s: %w", repo, err) + // Two workers fetching one mirror race on git's ref locks, and the loser + // reports "cannot lock ref" even though the winner has just made the + // mirror current. Retry briefly, then accept the mirror as it stands + // rather than failing a dispatch over a fetch somebody else completed. + var ferr error + for attempt := 0; attempt < 3; attempt++ { + if _, ferr = w.git(ctx, path, "fetch", "--prune", "origin"); ferr == nil { + return path, nil + } + if err := sleepCtx(ctx, time.Duration(attempt+1)*200*time.Millisecond); err != nil { + return "", err + } } - return path, nil + return path, fmt.Errorf("fetching %s: %w", repo, ferr) } else if !errors.Is(err, os.ErrNotExist) { return "", err } @@ -192,10 +212,11 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err != nil { return Checkout{}, err } - // Anything already here belongs to an earlier checkout of this PR, which is - // finished or dead either way. Clearing it keeps the generation directories - // from accumulating without letting a live handle delete a newer sibling. - if err := w.clearWork(ctx, mirror, prDir); err != nil { + // Old generations are pruned by AGE, not by being there: another worker may + // be building in one right now, and force-removing it would pull the ground + // out from under a live session. Each handle removes its own on the way out; + // this only collects what a killed process left behind. + if err := w.pruneStaleWork(ctx, mirror, prDir); err != nil { return Checkout{}, err } token := randomToken() @@ -224,8 +245,14 @@ func (w Workspace) workPath(repo string, pr int) (string, error) { return filepath.Join(root, "work", owner, name, strconv.Itoa(pr)), nil } -// clearWork removes every checkout under prDir and deregisters it. -func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { +// staleWorkAge is how long an abandoned checkout survives. Longer than any fix +// session should take, short enough that a killed watcher does not leave the +// disk filling up. +const staleWorkAge = 12 * time.Hour + +// pruneStaleWork removes checkouts under prDir that nothing has touched for +// staleWorkAge, leaving live ones alone. +func (w Workspace) pruneStaleWork(ctx context.Context, mirror, prDir string) error { entries, err := os.ReadDir(prDir) if errors.Is(err, os.ErrNotExist) { return nil @@ -234,6 +261,10 @@ func (w Workspace) clearWork(ctx context.Context, mirror, prDir string) error { return err } for _, entry := range entries { + info, err := entry.Info() + if err != nil || time.Since(info.ModTime()) < staleWorkAge { + continue + } if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { return err } @@ -301,17 +332,27 @@ func gitEnv(ctx context.Context, dir string, env []string, args ...string) (stri // workspace is crq's workspace as this configuration describes it: the root from // the config file (not just the process environment) and the token crq already // resolved for the API, so a daemon with GITHUB_TOKEN alone can still clone. -func (s *Service) workspace() Workspace { - return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken()} +func (s *Service) workspace(ctx context.Context) Workspace { + return Workspace{Root: s.cfg.WorkspaceRoot, Token: gitToken(ctx)} } // gitToken is the token to authenticate git with, or "" to leave git to the // host's own credential helper. -func gitToken() string { - for _, name := range []string{"GITHUB_TOKEN", "GH_TOKEN"} { - if value := strings.TrimSpace(os.Getenv(name)); value != "" { - return value - } +// +// It uses the SAME resolution the API client does, including `gh auth token`. +// Reading only the environment meant the documented `gh auth login` setup — API +// calls working fine — still produced unauthenticated clones, so every private +// checkout failed at dispatch's first real act. +func gitToken(ctx context.Context) string { return ghapi.LookupToken(ctx) } + +// sleepCtx waits, or returns early when the context ends. +func sleepCtx(ctx context.Context, d time.Duration) error { + timer := time.NewTimer(d) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil } - return "" } diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 6a14e589..7626487c 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -212,3 +212,55 @@ func TestGitTokenTravelsInTheEnvironment(t *testing.T) { t.Errorf("the token leaked into an error message: %v", err) } } + +// A relative root would make `git worktree add` — which runs inside the mirror — +// create the worktree under the mirror, while the path handed back points at a +// directory that does not exist. +func TestWorkspaceResolvesARelativeRoot(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + + // Run from a scratch directory so a relative root has somewhere to land. + scratch := t.TempDir() + prev, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(scratch); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chdir(prev) }) + + co, err := Workspace{Root: "relative-cache"}.Checkout(context.Background(), "owner/thing", 5, sha) + if err != nil { + t.Fatal(err) + } + if !filepath.IsAbs(co.Dir) { + t.Errorf("checkout dir %q is relative", co.Dir) + } + if _, err := os.Stat(filepath.Join(co.Dir, "README.md")); err != nil { + t.Errorf("the checkout is not where it says it is: %v", err) + } +} + +// Another worker may be building in an earlier generation right now. Clearing +// the directory eagerly pulled the ground out from under a live session. +func TestCheckoutLeavesALiveSiblingAlone(t *testing.T) { + base := t.TempDir() + sha := originRepo(t, filepath.Join(base, "owner/thing")) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + first, err := ws.Checkout(ctx, "owner/thing", 6, sha) + if err != nil { + t.Fatal(err) + } + if _, err := ws.Checkout(ctx, "owner/thing", 6, sha); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(first.Dir, "README.md")); err != nil { + t.Errorf("a live checkout was removed by the next one: %v", err) + } +} diff --git a/internal/gh/github.go b/internal/gh/github.go index 0b816620..d4e82614 100644 --- a/internal/gh/github.go +++ b/internal/gh/github.go @@ -167,6 +167,11 @@ func (g *GitHub) cacheGET(url string, resp *http.Response) (*http.Response, erro return resp, nil } +// LookupToken resolves a GitHub token from the environment or the gh CLI, for +// callers outside this package that need the same credential — git, which does +// not read GITHUB_TOKEN or gh's store by itself. +func LookupToken(ctx context.Context) string { return lookupToken(ctx) } + // lookupToken resolves a GitHub token from the environment or the gh CLI. gh can // hand back a freshly-rotated OAuth token, which is why send re-runs this on a 401. func lookupToken(ctx context.Context) string { From c5c2e86c24814280a52600d7f8c6a217a450ade0 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:27:05 +0200 Subject: [PATCH 09/74] 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 90e98b3f..eeb0c2ff 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 195f43a1..90da48b0 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 29b48509..571e6222 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 6d43f4a3..2601c930 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 c08a257d..ec77d87f 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 d054450030e89ce7fb9af9f6ae71c9c0984771a9 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:30:06 +0200 Subject: [PATCH 10/74] Stop the dispatcher losing work, quota, and its own arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten findings, six P1. `crq watch --dispatch -- ` never worked: FlagSet.Parse consumes the terminator, so looking for it afterwards found nothing and the fix command was read as a list of repositories. argv is split before parsing. Five ways dispatch could do harm. It ignored DryRun, so the mode that promises crq writes nothing claimed shared state and ran a code-writing command. It wrote the findings at the worktree root, where a session following the documented `git add -A` push would commit crq's review payload into the PR — they go outside the checkout now. It deleted the worktree after a successful session, discarding fixes that were made but not pushed; a worktree with uncommitted work is kept. It ignored the fleet skip marker before calling Next, which enqueues and can fire — the marker exists to protect the shared quota, so it is honoured first. And a lost heartbeat left the session running while another watcher started a second one on the same worktree; losing the claim now stops the session. Attempts were counted at the claim, so a failed clone ate the per-head budget and permanently skipped the PR after three tries. The attempt is given back when nothing ran. Two smaller: a throttle now sleeps for the reset the API named instead of the ordinary interval, which was hammering an exhausted quota, and a one-shot pass reports the PRs it could not read instead of exiting 0 — cron reporting a clean scan of a broken one is worse than a failure. An emit failure stops the watcher, because a closed pipe means nothing is observing something that fires reviews and starts sessions. --- cmd/crq/main.go | 30 ++++---- internal/crq/watch.go | 153 ++++++++++++++++++++++++++++++------- internal/crq/watch_test.go | 78 ++++++++++++++++++- 3 files changed, 218 insertions(+), 43 deletions(-) diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 7f0a6be2..90edc871 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -227,11 +227,22 @@ func run(ctx context.Context, args []string) int { case "watch": fs := flag.NewFlagSet("watch", flag.ContinueOnError) fs.SetOutput(os.Stderr) + // Split on "--" BEFORE parsing: FlagSet.Parse consumes the terminator, so + // looking for it in fs.Args() afterwards never finds it and the fix + // command is silently read as a list of repositories. + flagArgs, command := args[1:], []string(nil) + for i, arg := range flagArgs { + if arg == "--" { + command = flagArgs[i+1:] + flagArgs = flagArgs[:i] + break + } + } dispatch := fs.Bool("dispatch", false, "start a fix session for a PR that needs one") once := fs.Bool("once", false, "run one pass and exit") interval := fs.Duration("interval", 0, "time between passes") attempts := fs.Int("max-attempts", 0, "dispatches allowed per head") - if err := fs.Parse(args[1:]); err != nil { + if err := fs.Parse(flagArgs); err != nil { return 1 } if err := cfg.RequireState(); err != nil { @@ -241,21 +252,12 @@ func run(ctx context.Context, args []string) int { opts := crq.WatchOptions{ Dispatch: *dispatch, Once: *once, Interval: *interval, MaxAttempts: *attempts, + Command: command, } - // Everything after the flags is either repositories or, past a --, the - // fix session's argv. - rest := fs.Args() - for i, arg := range rest { - if arg == "--" { - opts.Command = rest[i+1:] - rest = rest[:i] - break - } - } - opts.Repos = rest + opts.Repos = fs.Args() enc := json.NewEncoder(os.Stdout) - werr := service.Watch(ctx, opts, func(e crq.WatchEvent) { - _ = enc.Encode(e) + werr := service.Watch(ctx, opts, func(e crq.WatchEvent) error { + return enc.Encode(e) }) if werr != nil && !errors.Is(werr, context.Canceled) { fatal(werr) diff --git a/internal/crq/watch.go b/internal/crq/watch.go index ebcd718f..6c3fa4b7 100644 --- a/internal/crq/watch.go +++ b/internal/crq/watch.go @@ -8,8 +8,9 @@ import ( "net/url" "os" "os/exec" - "path/filepath" "sort" + "strings" + "sync/atomic" "time" "github.com/kristofferR/coderabbit-queue/internal/engine" @@ -59,7 +60,7 @@ type WatchEvent struct { // Every dispatch is claimed under CAS, so two watchers cannot both spawn a // session for one PR, and bounded per head, so a fix that keeps not working // stops instead of spending a review round each time. -func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchEvent)) error { +func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchEvent) error) error { if opts.Interval <= 0 { opts.Interval = s.cfg.WatchInterval } @@ -73,24 +74,33 @@ func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchE return errors.New("dispatch needs a command: set CRQ_DISPATCH_CMD, or pass one after --") } for { + wait := opts.Interval if err := s.watchPass(ctx, opts, emit); err != nil { - if _, ok := ghapi.ThrottleWait(err); !ok { + reset, throttled := ghapi.ThrottleWait(err) + if !throttled { return err } + // Retrying on the ordinary interval hammers an exhausted quota and + // pushes the reset further out, which is the opposite of waiting it + // out. Sleep for what the API said. + if reset > wait { + wait = reset + } if s.log != nil { - s.log.Printf("watch: %v; waiting it out", err) + s.log.Printf("watch: %v; waiting %s for the reset", err, wait.Round(time.Second)) } } if opts.Once { return nil } - if err := ghapi.SleepCtx(ctx, opts.Interval); err != nil { + if err := ghapi.SleepCtx(ctx, wait); err != nil { return err } } } -func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(WatchEvent)) error { +func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(WatchEvent) error) error { + var failures []string repos := opts.Repos if len(repos) == 0 { for repo := range s.cfg.AllowRepos { @@ -110,6 +120,13 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa if err := ctx.Err(); err != nil { return err } + // The fleet's skip marker suppresses review deliberately, to protect + // the shared quota. Next is a MUTATING oracle — it enqueues and can + // fire — so the marker has to be honoured before calling it, not + // after. + if marker := strings.TrimSpace(s.cfg.SkipMarker); marker != "" && strings.Contains(pull.Body, marker) { + continue + } report, err := s.Next(ctx, repo, pull.Number) if err != nil { if _, ok := ghapi.ThrottleWait(err); ok { @@ -118,6 +135,12 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa if s.log != nil { s.log.Printf("watch: %s#%d: %v", repo, pull.Number, err) } + // A one-shot run is somebody's cron or CI job: reporting success + // after skipping a PR it could not read makes a broken scan look + // like a clean one. + if opts.Once { + failures = append(failures, fmt.Sprintf("%s#%d: %v", repo, pull.Number, err)) + } continue } event := WatchEvent{ @@ -129,43 +152,62 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa event.Dispatched, event.Skipped = s.dispatch(ctx, opts, report) } if emit != nil { - emit(event) + // A consumer that has gone away (a closed pipe, a full + // destination) means nothing is observing a watcher that is still + // firing reviews and starting sessions. Stop instead. + if err := emit(event); err != nil { + return fmt.Errorf("emitting %s#%d: %w", repo, pull.Number, err) + } } } } + if len(failures) > 0 { + return fmt.Errorf("%d pull request(s) could not be checked: %s", len(failures), strings.Join(failures, "; ")) + } return nil } // dispatch claims the round, checks the head out, and runs the fix session. func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextReport) (bool, string) { + // DryRun means crq writes nothing and posts nothing. Claiming shared state + // and running a code-writing command is the largest possible violation of + // that, so it is checked before anything else. + if s.cfg.DryRun { + return false, "dry run: would dispatch a fix session" + } token := randomToken() claimed, why := s.claimDispatch(ctx, report, token, opts.MaxAttempts) if !claimed { return false, why } - defer s.releaseDispatch(context.WithoutCancel(ctx), report, token) co, err := s.workspace().Checkout(ctx, report.Repo, report.PR, report.Head) if err != nil { + // Nothing ran, so the attempt did not happen: a transient clone failure + // must not eat the per-head budget and permanently skip the PR. + s.releaseDispatch(context.WithoutCancel(ctx), report, token, false) return false, "checkout failed: " + err.Error() } - defer func() { _ = co.Remove(context.WithoutCancel(ctx)) }() - // The session gets the findings as a file rather than an argument: they are - // long, and a shell would be the wrong place to carry them. - findingsPath := filepath.Join(co.Dir, ".crq-findings.json") - body, err := json.MarshalIndent(report.Findings, "", " ") + // The findings go OUTSIDE the worktree. At the repository root they are an + // untracked file, and a session following the documented `git add -A` push + // would commit crq's review payload into the PR. + findingsPath, err := s.writeFindings(report) if err != nil { + s.releaseDispatch(context.WithoutCancel(ctx), report, token, false) + _ = co.Remove(context.WithoutCancel(ctx)) return false, err.Error() } - if err := os.WriteFile(findingsPath, body, 0o600); err != nil { - return false, err.Error() - } + defer os.Remove(findingsPath) - stop := s.beatDispatch(ctx, report, token) - defer stop() + // Losing the claim means another watcher has taken this round and may be + // running its own session. Two sessions writing one worktree is worse than + // no session, so the heartbeat cancels this one. + runCtx, cancel := context.WithCancel(ctx) + defer cancel() + lost := s.beatDispatch(runCtx, report, token, cancel) - cmd := exec.CommandContext(ctx, opts.Command[0], opts.Command[1:]...) + cmd := exec.CommandContext(runCtx, opts.Command[0], opts.Command[1:]...) cmd.Dir = co.Dir cmd.Env = append(os.Environ(), "CRQ_DISPATCH_REPO="+report.Repo, @@ -177,12 +219,47 @@ func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextRe s.log.Printf("watch: dispatching %s for %s#%d@%s (%d findings)", opts.Command[0], report.Repo, report.PR, report.Head, len(report.Findings)) } - if err := cmd.Run(); err != nil { - return false, "fix session failed: " + err.Error() + runErr := cmd.Run() + s.releaseDispatch(context.WithoutCancel(ctx), report, token, true) + if lost() { + return false, "another watcher took this round; the session was stopped" } + if runErr != nil { + _ = co.Remove(context.WithoutCancel(ctx)) + return false, "fix session failed: " + runErr.Error() + } + + // Keep a worktree the session left work in. Removing it discards fixes that + // were made but not pushed, which is the one outcome a fix session must + // never suffer. + if dirty, err := co.Git(context.WithoutCancel(ctx), "status", "--porcelain"); err == nil && strings.TrimSpace(dirty) != "" { + if s.log != nil { + s.log.Printf("watch: keeping %s — the session left uncommitted work", co.Dir) + } + return true, "" + } + _ = co.Remove(context.WithoutCancel(ctx)) return true, "" } +// writeFindings puts the findings somewhere the fix session can read and the +// repository cannot accidentally commit. +func (s *Service) writeFindings(report NextReport) (string, error) { + body, err := json.MarshalIndent(report.Findings, "", " ") + if err != nil { + return "", err + } + file, err := os.CreateTemp("", fmt.Sprintf("crq-findings-%d-*.json", report.PR)) + if err != nil { + return "", err + } + defer file.Close() + if _, err := file.Write(body); err != nil { + return "", err + } + return file.Name(), nil +} + func (s *Service) claimDispatch(ctx context.Context, report NextReport, token string, maxAttempts int) (bool, string) { reason := "" _, err := s.store.Update(ctx, func(st *State) error { @@ -205,12 +282,18 @@ func (s *Service) claimDispatch(ctx context.Context, report NextReport, token st return reason == "", reason } -func (s *Service) releaseDispatch(ctx context.Context, report NextReport, token string) { +// releaseDispatch frees the round. attempted=false also gives the attempt back, +// for a claim that never reached a running session — a clone that failed or a +// command that could not start did not use up the per-head budget. +func (s *Service) releaseDispatch(ctx context.Context, report NextReport, token string, attempted bool) { _, _ = s.store.Update(ctx, func(st *State) error { round := st.Round(report.Repo, report.PR) if round == nil || !round.ReleaseDispatch(token) { return ErrNoChange } + if !attempted && round.Dispatch != nil && round.Dispatch.Attempts > 0 { + round.Dispatch.Attempts-- + } st.PutRound(*round) return nil }) @@ -218,28 +301,42 @@ func (s *Service) releaseDispatch(ctx context.Context, report NextReport, token // beatDispatch refreshes the claim while the session runs, so a session that // outlives the TTL keeps its round and a crashed watcher's does not. -func (s *Service) beatDispatch(ctx context.Context, report NextReport, token string) func() { - beatCtx, cancel := context.WithCancel(ctx) +// beatDispatch refreshes the claim while the session runs and reports whether it +// was lost. Losing it means another watcher took the round over, so stop() is +// called to end this session rather than let two write one worktree. +func (s *Service) beatDispatch(ctx context.Context, report NextReport, token string, stop func()) func() bool { + var lost atomic.Bool go func() { ticker := time.NewTicker(DispatchTTL / 3) defer ticker.Stop() for { select { - case <-beatCtx.Done(): + case <-ctx.Done(): return case <-ticker.C: - _, _ = s.store.Update(beatCtx, func(st *State) error { + held := false + if _, err := s.store.Update(ctx, func(st *State) error { round := st.Round(report.Repo, report.PR) if round == nil || !round.HeartbeatDispatch(token, s.clock()) { return ErrNoChange } + held = true st.PutRound(*round) return nil - }) + }); err != nil { + // A failed write is not proof the claim is gone; the next + // tick decides. Only an explicit "not yours" ends the session. + continue + } + if !held { + lost.Store(true) + stop() + return + } } } }() - return cancel + return lost.Load } func openPullQuery() url.Values { diff --git a/internal/crq/watch_test.go b/internal/crq/watch_test.go index ea04fdc2..6269cdb2 100644 --- a/internal/crq/watch_test.go +++ b/internal/crq/watch_test.go @@ -72,7 +72,13 @@ func TestWatchDispatchesAFixSessionWithItsContext(t *testing.T) { t.Errorf("session ran in %q, want a worktree under %q", got.Cwd, cfg.WorkspaceRoot) } if got.Findings == "" { - t.Error("the session was given no findings file") + t.Fatal("the session was given no findings file") + } + // OUTSIDE the worktree: at the repository root it is an untracked file, and + // a session following the documented `git add -A` push would commit crq's + // review payload into the PR. + if strings.HasPrefix(got.Findings, got.Cwd) { + t.Errorf("findings at %q are inside the worktree %q", got.Findings, got.Cwd) } // The claim is released afterwards, so the next round is not blocked by a @@ -109,3 +115,73 @@ func TestWatchRefusesDispatchWithNoCommand(t *testing.T) { t.Errorf("err = %v, want a refusal naming the missing command", err) } } + +// DryRun means crq writes nothing and posts nothing. Claiming shared state and +// running a code-writing command is the largest possible violation of that. +func TestDispatchHonoursDryRun(t *testing.T) { + cfg := firingConfig() + cfg.DryRun = true + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + ran := filepath.Join(t.TempDir(), "ran") + script := filepath.Join(t.TempDir(), "s.sh") + if err := os.WriteFile(script, []byte("#!/bin/sh\ntouch "+ran+"\n"), 0o755); err != nil { + t.Fatal(err) + } + ok, why := svc.dispatch(context.Background(), WatchOptions{Dispatch: true, Command: []string{script}}, + NextReport{Repo: "o/r", PR: 1, Head: "aaaaaaaa1", Action: "fix"}) + if ok { + t.Error("a dry run dispatched a session") + } + if !strings.Contains(why, "dry run") { + t.Errorf("reason = %q, want it to say why", why) + } + if _, err := os.Stat(ran); !os.IsNotExist(err) { + t.Error("the fix command ran under a dry run") + } +} + +// A session that fixes files without pushing must not have that work deleted: +// removing the worktree discards fixes that were made but not landed. +func TestDispatchKeepsAWorktreeWithUnpushedWork(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + + cfg := firingConfig() + cfg.WorkspaceRoot = t.TempDir() + gh := newFakeGitHub() + gh.graphQL = noForcePush + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = sha + gh.pulls[fakeKey(repo, 8)] = pull + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + seedRound(t, store, cfg, repo, 8, sha, PhaseQueued, time.Now().UTC(), 0) + + // A session that edits a file and stops there, which is the ordinary shape. + script := filepath.Join(t.TempDir(), "fix.sh") + if err := os.WriteFile(script, []byte("#!/bin/sh\necho fixed >> README.md\n"), 0o755); err != nil { + t.Fatal(err) + } + ok, why := svc.dispatch(context.Background(), WatchOptions{Dispatch: true, Command: []string{script}, MaxAttempts: 3}, + NextReport{Repo: repo, PR: 8, Head: sha, Action: "fix"}) + if !ok { + t.Fatalf("dispatch failed: %s", why) + } + found := false + _ = filepath.WalkDir(filepath.Join(cfg.WorkspaceRoot, "work"), func(path string, d os.DirEntry, err error) error { + if err == nil && d.Name() == "README.md" { + if body, rerr := os.ReadFile(path); rerr == nil && strings.Contains(string(body), "fixed") { + found = true + } + } + return nil + }) + if !found { + t.Error("the session's uncommitted fix was deleted with the worktree") + } +} From 16c60043c0b5a4aa9cb18e9f87bf443d35109bee Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:34:45 +0200 Subject: [PATCH 11/74] Stop the mirror deleting the branches sessions create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, one P1 that would have destroyed a fix session's work. The mirror was a --mirror clone, whose refspec is +refs/*:refs/*, so the next `fetch --prune` reached into refs/heads and deleted any branch a session had created in its worktree — the documented way for a session to make changes. It is a bare clone now, fetching into refs/remotes/origin/*, which leaves refs/heads to the sessions. Pruning read the checkout directory's own timestamp, which editing files inside does not update: a session busy for twelve hours read as abandoned and had its worktree force-removed. It measures the newest file under the checkout instead. Persistent ref-lock contention on a shared mirror returned an error even though the mirror was current, failing a dispatch over another worker's success — the exact case concurrent dispatch has to survive. And the worktree add now goes through the credential-carrying runner, so a checkout filter that fetches from a private repository is authenticated like every other git call. --- internal/crq/workspace.go | 41 ++++++++++++++++++++++---- internal/crq/workspace_test.go | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 00f453c0..685250ba 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -144,6 +144,12 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { return "", err } } + // Still contended. The mirror exists and another worker is updating it, + // so returning an error here fails a dispatch over somebody else's + // success — the case concurrent dispatch is supposed to survive. + if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { + return path, nil + } return path, fmt.Errorf("fetching %s: %w", repo, ferr) } else if !errors.Is(err, os.ErrNotExist) { return "", err @@ -160,9 +166,16 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } defer os.RemoveAll(staging) pending := filepath.Join(staging, "mirror.git") - if _, err := w.git(ctx, "", "clone", "--mirror", w.remoteURL(repo), pending); err != nil { + // --bare, not --mirror. A mirror's refspec is +refs/*:refs/*, so `fetch + // --prune` reaches into refs/heads and deletes any branch a fix session + // created in a worktree — the documented way to make changes. Remote refs + // live under refs/remotes/origin/*, leaving refs/heads to the sessions. + if _, err := w.git(ctx, "", "clone", "--bare", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } + if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); err != nil { + return "", fmt.Errorf("configuring %s: %w", repo, err) + } if err := os.Rename(pending, path); err != nil { if _, statErr := os.Stat(filepath.Join(path, "HEAD")); statErr == nil { return path, nil // somebody else won the race; their mirror is as good @@ -224,7 +237,7 @@ func (w Workspace) Checkout(ctx context.Context, repo string, pr int, sha string if err := os.MkdirAll(prDir, 0o700); err != nil { return Checkout{}, err } - if _, err := gitDir(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { + if _, err := w.git(ctx, mirror, "worktree", "add", "--detach", dir, sha); err != nil { return Checkout{}, fmt.Errorf("checking out %s@%s: %w", repo, shortSHA(sha), err) } return Checkout{Dir: dir, Repo: NormalizeRepo(repo), PR: pr, mirror: mirror, token: token}, nil @@ -261,17 +274,35 @@ func (w Workspace) pruneStaleWork(ctx context.Context, mirror, prDir string) err return err } for _, entry := range entries { - info, err := entry.Info() - if err != nil || time.Since(info.ModTime()) < staleWorkAge { + dir := filepath.Join(prDir, entry.Name()) + // The NEWEST file in the checkout, not the directory's own timestamp: + // editing a file or running a build inside leaves the root's mtime + // untouched, so a busy session would read as abandoned. + if touched := newestModTime(dir); time.Since(touched) < staleWorkAge { continue } - if err := w.removeWorktree(ctx, mirror, filepath.Join(prDir, entry.Name())); err != nil { + if err := w.removeWorktree(ctx, mirror, dir); err != nil { return err } } return nil } +// newestModTime is the most recent modification anywhere under dir. +func newestModTime(dir string) time.Time { + newest := time.Time{} + _ = filepath.WalkDir(dir, func(_ string, d os.DirEntry, err error) error { + if err != nil { + return nil + } + if info, ierr := d.Info(); ierr == nil && info.ModTime().After(newest) { + newest = info.ModTime() + } + return nil + }) + return newest +} + // Remove deletes the worktree. Safe to call on an already-removed one. func (c Checkout) Remove(ctx context.Context) error { if c.mirror == "" || c.Dir == "" || filepath.Base(c.Dir) != c.token { diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 7626487c..2d985986 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) // originRepo builds a real repository on disk to clone from, so these tests @@ -264,3 +265,56 @@ func TestCheckoutLeavesALiveSiblingAlone(t *testing.T) { t.Errorf("a live checkout was removed by the next one: %v", err) } } + +// A fix session's documented way to make changes is a branch in its checkout. +// A --mirror clone's refspec is +refs/*:refs/*, so the next `fetch --prune` +// reached into refs/heads and deleted that branch — destroying the session's work +// between one dispatch and the next. +func TestFetchDoesNotDeleteABranchAWorktreeCreated(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + co, err := ws.Checkout(ctx, repo, 3, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "crq/fix-3"); err != nil { + t.Fatal(err) + } + + // Another dispatch fetches the shared mirror. + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatal(err) + } + if branch, err := co.Git(ctx, "rev-parse", "--abbrev-ref", "HEAD"); err != nil || branch != "crq/fix-3" { + t.Fatalf("branch = %q err=%v, want the session's branch to survive a fetch", branch, err) + } +} + +// Pruning read the checkout directory's own mtime, which editing files inside +// does not update — so a busy session read as abandoned and was deleted. +func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { + dir := t.TempDir() + old := time.Now().Add(-48 * time.Hour) + nested := filepath.Join(dir, "sub") + if err := os.MkdirAll(nested, 0o755); err != nil { + t.Fatal(err) + } + file := filepath.Join(nested, "edited.go") + if err := os.WriteFile(file, []byte("package main"), 0o644); err != nil { + t.Fatal(err) + } + // The directories look ancient; the file inside was just written. + for _, d := range []string{dir, nested} { + if err := os.Chtimes(d, old, old); err != nil { + t.Fatal(err) + } + } + if since := time.Since(newestModTime(dir)); since > time.Minute { + t.Errorf("newest modification read as %s ago; a live session would be pruned", since) + } +} From a3da74ebde6ade082c201c0ced1ac2f8c2b0d937 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:41:28 +0200 Subject: [PATCH 12/74] 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 87109526..fee847b7 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 eeb0c2ff..3ab19f88 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 571e6222..3dfb120a 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 13/74] 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 3ab19f88..53ba22f1 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 170892387b912ace5e6ad4c75312ebe8814f9251 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 01:27:12 +0200 Subject: [PATCH 14/74] Migrate a mirror's refspec instead of only setting it at clone time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applying the refspec only when cloning left every mirror made by an earlier crq still fetching +refs/*:refs/*. A fix session that created a branch in its worktree then wedged the WHOLE repository: git refuses to fetch into a branch checked out somewhere, so every later checkout of every PR failed with "refusing to fetch into branch ... checked out at". Observed live — one session's branch stopped the drain from dispatching anything for hours, while PRs sat with findings nobody was looking at. The refspec is now enforced on every Mirror call, which migrates the mirrors that already exist. The test reproduces the original failure: an old refspec, a branch created in a worktree, and a fetch for a different PR that has to keep working. --- internal/crq/workspace.go | 14 ++++++++++++- internal/crq/workspace_test.go | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/internal/crq/workspace.go b/internal/crq/workspace.go index 685250ba..70a0b2f3 100644 --- a/internal/crq/workspace.go +++ b/internal/crq/workspace.go @@ -131,6 +131,13 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { } } if _, err := os.Stat(filepath.Join(path, "HEAD")); err == nil { + // Enforce the refspec on EVERY call, not only at clone time. A mirror + // created before this rule still fetches +refs/*:refs/*, and one branch + // created in a worktree then wedges every future fetch for the whole + // repository with "refusing to fetch into branch ... checked out at". + if _, cerr := w.git(ctx, path, "config", "remote.origin.fetch", originRefspec); cerr != nil { + return "", fmt.Errorf("configuring %s: %w", repo, cerr) + } // Two workers fetching one mirror race on git's ref locks, and the loser // reports "cannot lock ref" even though the winner has just made the // mirror current. Retry briefly, then accept the mirror as it stands @@ -173,7 +180,7 @@ func (w Workspace) Mirror(ctx context.Context, repo string) (string, error) { if _, err := w.git(ctx, "", "clone", "--bare", w.remoteURL(repo), pending); err != nil { return "", fmt.Errorf("cloning %s: %w", repo, err) } - if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); err != nil { + if _, err := w.git(ctx, pending, "config", "remote.origin.fetch", originRefspec); err != nil { return "", fmt.Errorf("configuring %s: %w", repo, err) } if err := os.Rename(pending, path); err != nil { @@ -196,6 +203,11 @@ func (w Workspace) remoteURL(repo string) string { return strings.TrimRight(base, "/") + "/" + NormalizeRepo(repo) } +// originRefspec keeps fetched refs out of refs/heads, which belongs to the +// worktrees: a session's branch there must survive a fetch, and a branch checked +// out in a worktree makes any fetch that would update it fail outright. +const originRefspec = "+refs/heads/*:refs/remotes/origin/*" + // Checkout is a worktree at one commit, and the directory git commands for that // PR run in. type Checkout struct { diff --git a/internal/crq/workspace_test.go b/internal/crq/workspace_test.go index 2d985986..f25b91f3 100644 --- a/internal/crq/workspace_test.go +++ b/internal/crq/workspace_test.go @@ -318,3 +318,41 @@ func TestPruningMeasuresTheNewestFileNotTheDirectory(t *testing.T) { t.Errorf("newest modification read as %s ago; a live session would be pruned", since) } } + +// A mirror created before the refspec rule still fetches +refs/*:refs/*. One +// branch created in a worktree then wedges every future fetch for the whole +// repository — "refusing to fetch into branch ... checked out at" — which is how +// a single fix session stopped every dispatch for hours. +func TestMirrorMigratesAnOldRefspec(t *testing.T) { + base := t.TempDir() + repo := "owner/thing" + sha := originRepo(t, filepath.Join(base, repo)) + t.Setenv("CRQ_REMOTE_BASE", base) + ws := Workspace{Root: t.TempDir()} + ctx := context.Background() + + mirror, err := ws.Mirror(ctx, repo) + if err != nil { + t.Fatal(err) + } + // Put it back the way an older crq left it. + if _, err := gitDir(ctx, mirror, "config", "remote.origin.fetch", "+refs/*:refs/*"); err != nil { + t.Fatal(err) + } + co, err := ws.Checkout(ctx, repo, 4, sha) + if err != nil { + t.Fatal(err) + } + if _, err := co.Git(ctx, "checkout", "-b", "session-branch"); err != nil { + t.Fatal(err) + } + + // The next dispatch of ANY pr in this repository fetches the same mirror. + if _, err := ws.Mirror(ctx, repo); err != nil { + t.Fatalf("a branch in one worktree wedged the whole repository: %v", err) + } + got, err := gitDir(ctx, mirror, "config", "--get", "remote.origin.fetch") + if err != nil || got != originRefspec { + t.Errorf("refspec = %q err=%v, want it migrated to %q", got, err, originRefspec) + } +} From f692045008c683d607b92ae4e0aa243f93ec1bed Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 01:27:54 +0200 Subject: [PATCH 15/74] Merge the mirror refspec migration --- internal/crq/watch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/crq/watch.go b/internal/crq/watch.go index 6c3fa4b7..d57e8757 100644 --- a/internal/crq/watch.go +++ b/internal/crq/watch.go @@ -181,7 +181,7 @@ func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextRe return false, why } - co, err := s.workspace().Checkout(ctx, report.Repo, report.PR, report.Head) + co, err := s.workspace(ctx).Checkout(ctx, report.Repo, report.PR, report.Head) if err != nil { // Nothing ran, so the attempt did not happen: a transient clone failure // must not eat the per-head budget and permanently skip the PR. From c839f2722682ae9ef553f8b36cc416aa8f6a425c Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 01:32:41 +0200 Subject: [PATCH 16/74] Say it out loud when no fix session can start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dispatcher failed for hours and looked like success from outside: the watcher ran, the queue moved, PRs reported findings — and every session died on a wedged git mirror, in a log line nobody was reading. The only thing that noticed was a person eventually asking why a PR was untouched. A pass that attempts a dispatch now records whether one actually STARTED. Three failed passes in a row is a dispatcher that is not working, and crq says so where it is seen: a 🚨 line on the dashboard naming the host and the error, and the status line saying "dispatch failing" above every other state — a queue that looks busy while nothing can start is worse news than anything the queue itself reports. It is deliberately not Warn. Warn is cleared by the next successful fire, so unrelated progress would wipe it, which is how this stayed invisible. A claim another watcher holds does not count as a failure, and one started session clears the alarm. --- internal/crq/state.go | 2 ++ internal/crq/watch.go | 35 ++++++++++++++++++++++ internal/state/dashboard.go | 4 +++ internal/state/drain_test.go | 57 ++++++++++++++++++++++++++++++++++++ internal/state/state.go | 50 +++++++++++++++++++++++++++++++ internal/state/statusline.go | 4 +++ 6 files changed, 152 insertions(+) create mode 100644 internal/state/drain_test.go diff --git a/internal/crq/state.go b/internal/crq/state.go index 4cab05ea..a93e10e6 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -36,6 +36,8 @@ const ( // DispatchTTL is how long a watcher's fix claim survives without a heartbeat. DispatchTTL = crqstate.DispatchTTL + // DrainUnhealthyAfter is how many passes may fail to dispatch before crq says so. + DrainUnhealthyAfter = crqstate.DrainUnhealthyAfter ) var ( diff --git a/internal/crq/watch.go b/internal/crq/watch.go index d57e8757..3aeed7b0 100644 --- a/internal/crq/watch.go +++ b/internal/crq/watch.go @@ -101,6 +101,10 @@ func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchE func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(WatchEvent) error) error { var failures []string + // Whether any fix session actually STARTED this pass. A dispatcher that + // cannot start one — a wedged mirror, a missing command — otherwise fails + // silently while the queue looks busy. + attempted, started, lastFailure := false, false, "" repos := opts.Repos if len(repos) == 0 { for repo := range s.cfg.AllowRepos { @@ -150,6 +154,13 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa } if opts.Dispatch && report.Action == string(engine.ActionFix) { event.Dispatched, event.Skipped = s.dispatch(ctx, opts, report) + // A claim somebody else holds is not a failure of this pass. + if event.Dispatched { + attempted, started = true, true + } else if event.Skipped != "" && !strings.Contains(event.Skipped, "already fixing") { + attempted = true + lastFailure = event.Skipped + } } if emit != nil { // A consumer that has gone away (a closed pipe, a full @@ -161,12 +172,36 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa } } } + if opts.Dispatch && attempted { + s.noteDispatchHealth(ctx, started, lastFailure) + } if len(failures) > 0 { return fmt.Errorf("%d pull request(s) could not be checked: %s", len(failures), strings.Join(failures, "; ")) } return nil } +// noteDispatchHealth records whether fix sessions are starting, and says so +// loudly the first time it is clear they are not. +// +// The failure this exists for looked exactly like success from the outside: the +// watcher ran, the queue moved, PRs reported findings — and every dispatch died +// on a wedged git mirror, in a log line nobody was reading. +func (s *Service) noteDispatchHealth(ctx context.Context, started bool, reason string) { + var unhealthy bool + if _, err := s.store.Update(ctx, func(st *State) error { + was := st.Drain.Unhealthy() + st.NoteDispatch(s.cfg.Host, started, reason, s.clock()) + unhealthy = st.Drain.Unhealthy() && !was + return nil + }); err != nil { + return + } + if unhealthy && s.log != nil { + s.log.Printf("ALERT: no fix session has started in %d passes — %s", DrainUnhealthyAfter, reason) + } +} + // dispatch claims the round, checks the head out, and runs the fix session. func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextReport) (bool, string) { // DryRun means crq writes nothing and posts nothing. Claiming shared state diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 6509b1d3..c0d39c1a 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -254,6 +254,10 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "| **Co-reviewers** | %s |\n", cfg.CoReviewers) } fmt.Fprintf(&b, "| **Last review fired** | %s |\n", fmtStamp(st.LastFired, loc)) + if st.Drain.Unhealthy() { + fmt.Fprintf(&b, "\n> 🚨 fix sessions are not starting on %s — %d passes in a row: %s\n", + dash(st.Drain.Host), st.Drain.ConsecutiveFailures, st.Drain.LastError) + } if st.Warn != "" { fmt.Fprintf(&b, "\n> ⚠️ %s\n", st.Warn) } diff --git a/internal/state/drain_test.go b/internal/state/drain_test.go new file mode 100644 index 00000000..36430499 --- /dev/null +++ b/internal/state/drain_test.go @@ -0,0 +1,57 @@ +package state + +import ( + "strings" + "testing" + "time" +) + +// The failure this exists for looked like success from outside: the watcher ran, +// the queue moved, PRs reported findings — and every dispatch died on a wedged +// git mirror, in a log line nobody read. It has to reach a surface someone looks +// at, and survive the unrelated progress that clears Warn. +func TestDrainHealthSurfacesAFailingDispatcher(t *testing.T) { + now := time.Date(2026, 7, 26, 23, 0, 0, 0, time.UTC) + st := New() + st.Account.Scope = "owner" + + if st.Drain.Unhealthy() { + t.Fatal("a fleet that has never dispatched is not unhealthy") + } + // One failure is a transient. + st.NoteDispatch("cachyos", false, "checkout failed", now) + if st.Drain.Unhealthy() { + t.Error("one failure must not raise the alarm") + } + for i := 0; i < DrainUnhealthyAfter; i++ { + st.NoteDispatch("cachyos", false, "refusing to fetch into branch", now) + } + if !st.Drain.Unhealthy() { + t.Fatalf("after %d failures the dispatcher is not working", DrainUnhealthyAfter) + } + + // The two surfaces a person actually looks at. + if line := StatusLine(st, StoreConfig{}); !strings.Contains(line, "dispatch failing") { + t.Errorf("status line = %q, want the failing dispatcher named", line) + } + dash := RenderDashboard(st, StoreConfig{Scope: []string{"owner"}}) + if !strings.Contains(dash, "fix sessions are not starting") || !strings.Contains(dash, "cachyos") { + t.Errorf("dashboard does not report the failure:\n%s", dash) + } + + // Unrelated progress clears Warn; it must NOT clear this, or the failure + // disappears again the moment something else succeeds. + st.Warn = "" + if !st.Drain.Unhealthy() { + t.Error("clearing Warn cleared the dispatch alarm") + } + + // One success is recovery. + st.NoteDispatch("cachyos", true, "", now.Add(time.Minute)) + if st.Drain.Unhealthy() { + t.Error("a started session must clear the alarm") + } + if st.Drain.LastSuccessAt == nil { + t.Error("recovery must be recorded") + } +} diff --git a/internal/state/state.go b/internal/state/state.go index e23286ae..d53b43e1 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -295,6 +295,12 @@ type State struct { // for the dashboard and debugging. Bounded by ArchiveMax. Archive []Round `json:"archive,omitempty"` + // Drain records the watcher's dispatch health. It is separate from Warn + // because Warn is cleared by the next successful fire — a dispatcher that + // has been failing for hours would be wiped by unrelated progress, which is + // exactly how the failure stayed invisible. + Drain *DrainHealth `json:"drain,omitempty"` + Warn string `json:"warn,omitempty"` UpdatedAt *time.Time `json:"wrote_at,omitempty"` DashboardSHA string `json:"dashboard_sha,omitempty"` @@ -676,6 +682,50 @@ func (r *Round) ReleaseDispatch(token string) bool { return true } +// DrainUnhealthyAfter is how many consecutive passes may fail to dispatch +// anything before crq says so out loud. One failure is a transient; three in a +// row is a dispatcher that is not working. +const DrainUnhealthyAfter = 3 + +// DrainHealth is the watcher's dispatch record: whether fix sessions are +// actually starting. +// +// A dispatch failure used to be a line in a log nobody read, so a wedged git +// mirror stopped every session for hours while the queue looked busy. Counting +// it here puts it on the dashboard and in the status line instead. +type DrainHealth struct { + Host string `json:"host,omitempty"` + ConsecutiveFailures int `json:"consecutive_failures,omitempty"` + LastError string `json:"last_error,omitempty"` + LastFailureAt *time.Time `json:"last_failure_at,omitempty"` + LastSuccessAt *time.Time `json:"last_success_at,omitempty"` +} + +// Unhealthy reports whether dispatch has failed enough times in a row to be +// worth someone's attention. +func (d *DrainHealth) Unhealthy() bool { + return d != nil && d.ConsecutiveFailures >= DrainUnhealthyAfter +} + +// NoteDispatch records one pass's outcome: whether any session started, and the +// reason if none did. +func (s *State) NoteDispatch(host string, started bool, reason string, now time.Time) { + if s.Drain == nil { + s.Drain = &DrainHealth{} + } + at := now.UTC() + s.Drain.Host = host + if started { + s.Drain.ConsecutiveFailures = 0 + s.Drain.LastError = "" + s.Drain.LastSuccessAt = &at + return + } + s.Drain.ConsecutiveFailures++ + s.Drain.LastError = reason + s.Drain.LastFailureAt = &at +} + // 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/internal/state/statusline.go b/internal/state/statusline.go index 3952d766..cbfcdb28 100644 --- a/internal/state/statusline.go +++ b/internal/state/statusline.go @@ -29,6 +29,10 @@ func StatusLine(st State, cfg StoreConfig) string { var parts []string stranded := firstStranded(st, inFlight) switch { + case st.Drain.Unhealthy(): + // Above everything else: a queue that looks busy while no session can + // start is the state that hid a wedged dispatcher for hours. + parts = append(parts, fmt.Sprintf("🚨 dispatch failing (%d)", st.Drain.ConsecutiveFailures)) case stranded != nil: parts = append(parts, fmt.Sprintf("⚠ #%d stranded", stranded.PR)) case !ready && st.Account.BlockedUntil != nil && st.Account.BlockedUntil.After(now): From b38778be2be24e5f1b67871bb9bfdbc9d181746f Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 01:49:45 +0200 Subject: [PATCH 17/74] Run fix sessions beside each other, not inside the loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A session ran inline in the pass, so one twenty-minute fix meant twenty minutes in which no other PR was even looked at. The drain went quiet for six minutes at a time with six PRs waiting and one session working. Sessions now run in a bounded pool off the pass, up to CRQ_DISPATCH_CONCURRENCY (default 3). When every slot is busy the PR is left for the next pass rather than queued behind a session — waiting here would recreate the problem the pool exists to solve. The DECISIONS stay serial, deliberately. `Next` is what enqueues and fires, so deciding one PR at a time is what keeps the account-metered review in a single queue; only the sessions overlap, and they spend no CodeRabbit quota. Concurrent pushes still land in that one queue and the fire slot serializes them as before. Dispatch health moves into the session, since the pass no longer knows the outcome by the time it ends, and a --once run waits for its sessions so it cannot return while they are still writing. --- cmd/crq/main.go | 11 ++++- internal/crq/config.go | 3 ++ internal/crq/pool_test.go | 61 +++++++++++++++++++++++++ internal/crq/watch.go | 94 ++++++++++++++++++++++++++++++++------- 4 files changed, 151 insertions(+), 18 deletions(-) create mode 100644 internal/crq/pool_test.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 90edc871..560d2af6 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -242,6 +242,7 @@ func run(ctx context.Context, args []string) int { once := fs.Bool("once", false, "run one pass and exit") interval := fs.Duration("interval", 0, "time between passes") attempts := fs.Int("max-attempts", 0, "dispatches allowed per head") + concurrency := fs.Int("concurrency", 0, "fix sessions allowed to run at once") if err := fs.Parse(flagArgs); err != nil { return 1 } @@ -252,7 +253,8 @@ func run(ctx context.Context, args []string) int { opts := crq.WatchOptions{ Dispatch: *dispatch, Once: *once, Interval: *interval, MaxAttempts: *attempts, - Command: command, + Concurrency: *concurrency, + Command: command, } opts.Repos = fs.Args() enc := json.NewEncoder(os.Stdout) @@ -571,6 +573,13 @@ worktree crq checked out at that head, with: The command comes from CRQ_DISPATCH_CMD or from everything after --. It is run directly, not through a shell, so nothing expands that you did not write. +Sessions run concurrently, up to CRQ_DISPATCH_CONCURRENCY (default 3), and OFF +the decision loop: a long session no longer blocks every other PR behind it. The +decisions themselves stay serial, because that is what keeps the account-metered +review in one queue — only the sessions, which spend no CodeRabbit quota, +overlap. When every slot is busy a PR waits for the next pass rather than +stalling the loop. + crq still does not decide which findings are real — it starts the session and says which PR to look at; the session judges. Every dispatch is claimed under compare-and-swap, so two watchers cannot both work one PR, and bounded per head diff --git a/internal/crq/config.go b/internal/crq/config.go index d55fcee2..48881629 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -54,6 +54,8 @@ type Config struct { WatchInterval time.Duration DispatchCommand []string DispatchMaxAttempts int + // DispatchConcurrency is how many fix sessions may run at once. + DispatchConcurrency int // WorkspaceRoot holds crq's own mirrors and worktrees. Read here rather than // from the process environment, so a value in ~/.config/crq/env — the // documented place for crq settings — is actually used. @@ -190,6 +192,7 @@ func LoadConfig() (Config, error) { WatchInterval: durationEnv(env, "CRQ_WATCH_INTERVAL", 2*time.Minute), DispatchCommand: splitArgv(env["CRQ_DISPATCH_CMD"]), DispatchMaxAttempts: intEnv(env, "CRQ_DISPATCH_MAX_ATTEMPTS", 3), + DispatchConcurrency: intEnv(env, "CRQ_DISPATCH_CONCURRENCY", 3), WorkspaceRoot: env["CRQ_WORKSPACE"], NoOpen: env["CRQ_NO_OPEN"] != "", DryRun: env["CRQ_DRY_RUN"] == "1", diff --git a/internal/crq/pool_test.go b/internal/crq/pool_test.go new file mode 100644 index 00000000..bc09965b --- /dev/null +++ b/internal/crq/pool_test.go @@ -0,0 +1,61 @@ +package crq + +import ( + "sync" + "testing" + "time" +) + +// A session used to run inline, so one twenty-minute fix blocked every other PR +// for twenty minutes. The pool exists to keep the decision loop moving. +func TestDispatchPoolDoesNotBlockTheCaller(t *testing.T) { + pool := newDispatchPool(2) + release := make(chan struct{}) + + start := time.Now() + for i := 0; i < 2; i++ { + if ok, why := pool.start(func() { <-release }); !ok { + t.Fatalf("session %d refused: %s", i, why) + } + } + // Both slots are busy: the caller is told so immediately rather than waiting. + ok, why := pool.start(func() {}) + if ok { + t.Error("a third session ran with only two slots") + } + if why == "" { + t.Error("a refusal must say why, or the PR looks handled") + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("starting sessions blocked for %s; the pass must not wait on them", elapsed) + } + + // A finished session frees its slot. + close(release) + pool.wait() + if ok, why := pool.start(func() {}); !ok { + t.Errorf("slot not released after the session finished: %s", why) + } + pool.wait() +} + +// --once must not return while its sessions are still writing. +func TestDispatchPoolWaitsForRunningSessions(t *testing.T) { + pool := newDispatchPool(3) + var mu sync.Mutex + done := 0 + for i := 0; i < 3; i++ { + pool.start(func() { + time.Sleep(20 * time.Millisecond) + mu.Lock() + done++ + mu.Unlock() + }) + } + pool.wait() + mu.Lock() + defer mu.Unlock() + if done != 3 { + t.Errorf("wait returned with %d/3 sessions finished", done) + } +} diff --git a/internal/crq/watch.go b/internal/crq/watch.go index 3aeed7b0..fd360a48 100644 --- a/internal/crq/watch.go +++ b/internal/crq/watch.go @@ -10,6 +10,7 @@ import ( "os/exec" "sort" "strings" + "sync" "sync/atomic" "time" @@ -32,6 +33,9 @@ type WatchOptions struct { Command []string // MaxAttempts bounds dispatches per head. 0 means the configured default. MaxAttempts int + // Concurrency is how many fix sessions may run at once. 0 means the + // configured default. + Concurrency int } // WatchEvent is one PR's state at a pass, and what the watcher did about it. @@ -61,12 +65,24 @@ type WatchEvent struct { // session for one PR, and bounded per head, so a fix that keeps not working // stops instead of spending a review round each time. func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchEvent) error) error { + // Fix sessions run OUTSIDE the pass. Running one inline blocked every other + // PR for as long as it took — one twenty-minute session meant twenty minutes + // in which nothing else was even looked at. + // + // The decisions stay serial on purpose. `Next` is what enqueues and fires, + // so deciding one PR at a time is what keeps the account-metered review in + // one queue; only the sessions, which spend no CodeRabbit quota, overlap. + pool := newDispatchPool(opts.Concurrency) + defer pool.wait() if opts.Interval <= 0 { opts.Interval = s.cfg.WatchInterval } if opts.MaxAttempts <= 0 { opts.MaxAttempts = s.cfg.DispatchMaxAttempts } + if opts.Concurrency <= 0 { + opts.Concurrency = s.cfg.DispatchConcurrency + } if opts.Dispatch && len(opts.Command) == 0 { opts.Command = s.cfg.DispatchCommand } @@ -75,7 +91,7 @@ func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchE } for { wait := opts.Interval - if err := s.watchPass(ctx, opts, emit); err != nil { + if err := s.watchPass(ctx, opts, pool, emit); err != nil { reset, throttled := ghapi.ThrottleWait(err) if !throttled { return err @@ -99,12 +115,8 @@ func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchE } } -func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(WatchEvent) error) error { +func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispatchPool, emit func(WatchEvent) error) error { var failures []string - // Whether any fix session actually STARTED this pass. A dispatcher that - // cannot start one — a wedged mirror, a missing command — otherwise fails - // silently while the queue looks busy. - attempted, started, lastFailure := false, false, "" repos := opts.Repos if len(repos) == 0 { for repo := range s.cfg.AllowRepos { @@ -153,14 +165,11 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa Findings: len(report.Findings), At: s.clock().UTC(), } if opts.Dispatch && report.Action == string(engine.ActionFix) { - event.Dispatched, event.Skipped = s.dispatch(ctx, opts, report) - // A claim somebody else holds is not a failure of this pass. - if event.Dispatched { - attempted, started = true, true - } else if event.Skipped != "" && !strings.Contains(event.Skipped, "already fixing") { - attempted = true - lastFailure = event.Skipped - } + // Started, not finished: the pass moves on to the next PR while + // this session runs. + event.Dispatched, event.Skipped = pool.start(func() { + s.runDispatch(ctx, opts, report) + }) } if emit != nil { // A consumer that has gone away (a closed pipe, a full @@ -172,9 +181,6 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, emit func(Wa } } } - if opts.Dispatch && attempted { - s.noteDispatchHealth(ctx, started, lastFailure) - } if len(failures) > 0 { return fmt.Errorf("%d pull request(s) could not be checked: %s", len(failures), strings.Join(failures, "; ")) } @@ -202,6 +208,20 @@ func (s *Service) noteDispatchHealth(ctx context.Context, started bool, reason s } } +// runDispatch performs one dispatch and records whether a session started. It +// runs in the pool, off the pass, so a long session delays nothing else. +func (s *Service) runDispatch(ctx context.Context, opts WatchOptions, report NextReport) { + ok, why := s.dispatch(ctx, opts, report) + // A round another watcher already holds is not this dispatcher failing. + if !ok && strings.Contains(why, "already fixing") { + return + } + s.noteDispatchHealth(ctx, ok, why) + if !ok && s.log != nil { + s.log.Printf("watch: %s#%d not fixed: %s", report.Repo, report.PR, why) + } +} + // dispatch claims the round, checks the head out, and runs the fix session. func (s *Service) dispatch(ctx context.Context, opts WatchOptions, report NextReport) (bool, string) { // DryRun means crq writes nothing and posts nothing. Claiming shared state @@ -379,3 +399,43 @@ func openPullQuery() url.Values { q.Set("state", "open") return q } + +// dispatchPool bounds how many fix sessions run at once. +// +// Non-blocking on purpose: when every slot is busy the PR is left for the next +// pass rather than stalling the decision loop behind a session. Queuing here +// would recreate the problem it exists to solve. +type dispatchPool struct { + slots chan struct{} + wg sync.WaitGroup +} + +func newDispatchPool(size int) *dispatchPool { + if size <= 0 { + size = 1 + } + return &dispatchPool{slots: make(chan struct{}, size)} +} + +// start runs fn in the pool. It reports whether a session was started, and why +// not when it was not. +func (p *dispatchPool) start(fn func()) (bool, string) { + select { + case p.slots <- struct{}{}: + default: + return false, "at dispatch capacity; will retry next pass" + } + p.wg.Add(1) + go func() { + defer func() { + <-p.slots + p.wg.Done() + }() + fn() + }() + return true, "" +} + +// wait blocks until every running session has finished, so a --once run does not +// return while its sessions are still writing. +func (p *dispatchPool) wait() { p.wg.Wait() } From bfb47142f87332b35a264ad8e22391c44e0d6b62 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 02:41:00 +0200 Subject: [PATCH 18/74] Give a fix session a log, and stop killing it when it succeeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects that together explain why sessions committed code and never resolved anything. A session's push moves the head, so crq supersedes the round and the fresh one carries no dispatch claim. The heartbeat read that as "another watcher took this round" and killed the session — between the push and the resolve, every single time it worked. Losing a claim and having it STOLEN are now different answers: only a claim somebody else actively holds stops a session, and a superseded round just stops the heartbeat. And the session's output went to nowhere at all, so a failed one left nothing to read but the absence of a result. Every session now writes to $CRQ_WORKSPACE/logs///--