From b1c57adf00faf021558069298a97a5d02e28bbde Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:19:51 +0200 Subject: [PATCH 01/12] Delete the trigger comments nothing needs any more MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A PR driven through a dozen rounds collects a dozen "@coderabbitai review" comments and a dozen acknowledgements, which buries the conversation a human came to read. crq now removes its own half of that as rounds progress, and `crq tidy` does it on demand. Deleting a comment crq still reads is how this becomes expensive, so a comment has to clear three guards. It belongs to a round that has PROGRESSED — a live round keeps its command, because that is the comment crq adopts instead of posting another. The bot acted after it, so it was read rather than merely old. And it predates the current head, because adoption only ever considers commands newer than the head commit; delete one of those and the next pump posts a duplicate and buys a second review. Only comments crq posted. Candidates are the command IDs recorded on its own rounds, never anything matching the text — a person's "@coderabbitai review" is their decision to ask, not crq's to erase, and crq posts under the same account so authorship alone cannot tell them apart. Never the bots' own comments. An auto-generated reply can be a rate-limit or skipped-review notice, which crq classifies as evidence and surfaces as a finding, so deleting those would quietly destroy feedback nobody had read yet. A recorded command that is already gone counts as tidied rather than failing: the bot removes some of its own command comments, and a person may have tidied by hand. --- cmd/crq/main.go | 48 ++++++++ internal/crq/auto.go | 7 +- internal/crq/config.go | 32 +++-- internal/crq/tidy.go | 205 +++++++++++++++++++++++++++++++ internal/crq/tidy_test.go | 165 +++++++++++++++++++++++++ internal/engine/tidy.go | 59 +++++++++ internal/engine/tidy_test.go | 63 ++++++++++ llms.txt | 10 ++ skills/coderabbit-queue/SKILL.md | 14 +++ 9 files changed, 589 insertions(+), 14 deletions(-) create mode 100644 internal/crq/tidy.go create mode 100644 internal/crq/tidy_test.go create mode 100644 internal/engine/tidy.go create mode 100644 internal/engine/tidy_test.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index b1cd9db..3e6b46d 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -224,6 +224,28 @@ func run(ctx context.Context, args []string) int { } printJSON(result) return 0 + case "tidy": + dryRun := hasFlag(args[1:], "--dry-run") + repo, pr, ok := repoPR(positional(args[1:])) + if !ok { + fatal(errors.New("usage: crq tidy [--dry-run]")) + return 1 + } + if bad, found := unknownFlag(args[1:], "--dry-run"); found { + fatal(fmt.Errorf("unknown flag %s (usage: crq tidy [--dry-run])", bad)) + return 1 + } + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + result, terr := service.Tidy(ctx, repo, pr, dryRun) + if terr != nil { + fatal(terr) + return 1 + } + printJSON(result) + return 0 case "autoreview", "auto": fs := flag.NewFlagSet("autoreview", flag.ContinueOnError) fs.SetOutput(os.Stderr) @@ -356,6 +378,7 @@ 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 tidy [--dry-run] remove crq's own spent review-trigger comments crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -509,6 +532,31 @@ 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 "tidy": + fmt.Print(`crq tidy [--dry-run] + +Delete the review-trigger comments crq posted that nothing needs any more. A PR +driven through a dozen rounds collects a dozen "@coderabbitai review" comments, +which buries the conversation a human came to read. + +A comment is removed only when all three hold: + + * crq posted it — the candidates are the command IDs crq recorded on its own + rounds, not anything matching the text. A person's "@coderabbitai review" is + their decision to ask and not crq's to erase. + * the round that asked has PROGRESSED. A live round keeps its command, because + that is the comment crq adopts instead of posting another one. + * the bot answered it, and it predates the current head. Adoption only ever + considers commands newer than the head commit, so deleting one of those + would make the next pump post a duplicate and buy a second review. + +It never deletes the bots' own comments. An auto-generated reply can be a +rate-limit or skipped-review notice, which crq reads as evidence and surfaces as +a finding — deleting those would destroy feedback nobody had read yet. + +Runs by itself as rounds progress under crq autoreview; CRQ_TIDY=0 turns that +off. --dry-run reports what it would remove. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] diff --git a/internal/crq/auto.go b/internal/crq/auto.go index cd69e12..091ac92 100644 --- a/internal/crq/auto.go +++ b/internal/crq/auto.go @@ -68,7 +68,12 @@ func (s *Service) AutoReview(ctx context.Context, opts AutoOptions) error { s.log.Printf("warning: autoreview pass failed: %v", passErr) } passFailure = passErr - if _, err := s.Pump(ctx); err != nil { + pumped, err := s.Pump(ctx) + // A round that just progressed is the moment its trigger comments + // stop being needed, so tidying here costs one observation on a PR + // crq was already looking at rather than a sweep of the fleet. + s.tidyAfterPump(ctx, pumped) + if err != nil { if _, ok := ghapi.ThrottleWait(err); ok { if cont, serr := s.sleepThrottle(ctx, opts, "pump", err); serr != nil || !cont { if opts.Once { diff --git a/internal/crq/config.go b/internal/crq/config.go index 98d6cdf..a21fdfe 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -55,19 +55,24 @@ type Config struct { // CompletionMarker identifies the bot's reply to a processed review command // (CodeRabbit: "Review finished."). Feedback uses it to count a command // round that produced no review object toward convergence. - CompletionMarker string - Host string - Timezone string - MinInterval time.Duration - InflightTimeout time.Duration - PollInterval time.Duration - WaitTimeout time.Duration - CalibrationTTL time.Duration - RateLimitFallback time.Duration - AutoReviewPoll time.Duration - AutoReviewMaxScan int - LeaderTTL time.Duration - FiredMax int + CompletionMarker string + Host string + Timezone string + MinInterval time.Duration + InflightTimeout time.Duration + PollInterval time.Duration + WaitTimeout time.Duration + CalibrationTTL time.Duration + RateLimitFallback time.Duration + AutoReviewPoll time.Duration + AutoReviewMaxScan int + LeaderTTL time.Duration + FiredMax int + // Tidy removes crq's own spent review-trigger comments as rounds progress. + // On by default: they are comments crq posted and the bot has answered, and + // a PR driven through a dozen rounds is otherwise unreadable. CRQ_TIDY=0 + // turns it off. + Tidy bool NoOpen bool DryRun bool FeedbackWaitTimeout time.Duration @@ -172,6 +177,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), + Tidy: stringEnv(env, "CRQ_TIDY", "1") != "0", 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/tidy.go b/internal/crq/tidy.go new file mode 100644 index 0000000..1d9a900 --- /dev/null +++ b/internal/crq/tidy.go @@ -0,0 +1,205 @@ +package crq + +import ( + "context" + "errors" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// TidyResult reports what a tidy pass removed, or would have. +type TidyResult struct { + Repo string `json:"repo"` + PR int `json:"pr"` + // Deleted are the comment IDs removed; Kept explains, per remaining + // candidate, why it stayed — so a pass that deletes nothing says why rather + // than looking broken. + Deleted []int64 `json:"deleted"` + Kept []string `json:"kept,omitempty"` + DryRun bool `json:"dry_run,omitempty"` +} + +// Tidy removes the review-trigger comments crq posted that nothing needs any +// more: the bot answered them, and the round that asked has progressed. +// +// A PR driven through a dozen rounds accumulates a dozen "@coderabbitai review" +// comments and a dozen acknowledgements, which buries the conversation a human +// came to read. This deletes crq's own half of that. +// +// It is deliberately narrow in two ways. +// +// Only comments CRQ POSTED. A human's "@coderabbitai review" is someone's +// decision to ask, and not crq's to erase; the candidate list is built from the +// command IDs crq recorded on its own rounds, not by matching text. +// +// Only crq's, not the bot's. An auto-generated reply can be a rate-limit notice +// or a skipped-review notice, which crq classifies as evidence and surfaces as +// a finding — so deleting bot comments is how this feature would quietly destroy +// feedback nobody had read yet. +func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (TidyResult, error) { + repo = NormalizeRepo(repo) + result := TidyResult{Repo: repo, PR: pr, Deleted: []int64{}, DryRun: dryRun} + + st, _, err := s.store.Load(ctx) + if err != nil { + return result, err + } + cfg := s.cfg + + // Candidates come from rounds that have PROGRESSED — archived (superseded, + // closed, cancelled) or completed. A round still working keeps its command, + // because that is the comment crq would adopt instead of posting again. + live := map[int64]bool{} + var commands []engine.CommandComment + collect := func(r Round, progressed bool) { + for _, entry := range roundCommands(r, cfg) { + if progressed { + commands = append(commands, entry) + } else { + live[entry.ID] = true + } + } + } + if round := st.Round(repo, pr); round != nil { + collect(*round, !round.Active()) + } + for _, archived := range st.Archive { + if NormalizeRepo(archived.Repo) == repo && archived.PR == pr { + collect(archived, true) + } + } + if len(commands) == 0 { + result.Kept = append(result.Kept, "no progressed round has a trigger comment to remove") + return result, nil + } + + obs, err := s.observe(ctx, repo, pr, nil, s.clock()) + if err != nil { + return result, err + } + if !obs.eng.Open { + // A closed PR is nobody's reading material, and deleting from it spends + // writes for no one's benefit. + result.Kept = append(result.Kept, "pr is closed") + return result, nil + } + + in := engine.TidyInput{ + Commands: commands, + Live: live, + HeadAt: obs.eng.HeadAt, + AnsweredAt: answeredAt(obs, cfg), + } + stale := engine.StaleCommands(in) + if len(stale) == 0 { + result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or newer than the head") + return result, nil + } + if dryRun || s.cfg.DryRun { + result.DryRun = true + result.Deleted = stale + return result, nil + } + for _, id := range stale { + err := s.gh.DeleteIssueComment(ctx, repo, id) + switch { + case err == nil, errors.Is(err, ghapi.ErrNotFound): + // Already gone is the outcome we wanted. A recorded command can + // vanish without crq: the bot removes some of its own command + // comments, and a person may tidy by hand. + result.Deleted = append(result.Deleted, id) + default: + // One write failing must not abandon the rest. + if s.log != nil { + s.log.Printf("tidy: %s#%d comment %d: %v", repo, pr, id, err) + } + } + } + if s.log != nil && len(result.Deleted) > 0 { + s.log.Printf("tidy: %s#%d removed %d spent trigger comment(s)", repo, pr, len(result.Deleted)) + } + return result, nil +} + +// roundCommands lists the trigger comments crq recorded on one round: the +// primary's, and each co-reviewer's. +func roundCommands(r Round, cfg Config) []engine.CommandComment { + var out []engine.CommandComment + at := time.Time{} + if r.FiredAt != nil { + at = r.FiredAt.UTC() + } + if r.CommandID != 0 { + out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at}) + } + for login, co := range r.CoBots { + if co.CommandID == 0 { + continue + } + when := at + if co.CommandedAt != nil { + when = co.CommandedAt.UTC() + } + out = append(out, engine.CommandComment{ID: co.CommandID, Bot: dialect.NormalizeBotName(login), CreatedAt: when}) + } + return out +} + +// answeredAt is the newest moment each reviewer demonstrably acted on this PR. +// A command with nothing after it was never read, and only what has been read is +// removed. +func answeredAt(obs observation, cfg Config) map[string]time.Time { + out := map[string]time.Time{} + note := func(bot string, at time.Time) { + key := dialect.NormalizeBotName(bot) + if key == "" || at.IsZero() { + return + } + if at.After(out[key]) { + out[key] = at + } + } + for _, review := range obs.reviews { + note(review.User.Login, review.SubmittedAt) + } + for _, event := range obs.eng.Events { + note(event.Bot, eventAt(event)) + } + for _, check := range obs.eng.Checks { + note(check.Bot, check.CompletedAt) + } + return out +} + +// eventAt is when a classified bot event happened. +func eventAt(e dialect.BotEvent) time.Time { + if !e.UpdatedAt.IsZero() { + return e.UpdatedAt + } + return e.CreatedAt +} + +// tidyAfterPump removes spent trigger comments for a PR whose round just +// progressed, which is what makes tidying happen regularly without a sweep of +// its own: the daemon is already looking at this PR. +// +// It is best-effort by design. Deleting a comment is housekeeping, and a +// housekeeping failure must never break the pass that did the real work. +func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) { + if !s.cfg.Tidy || res.Repo == "" || res.PR == 0 { + return + } + switch res.Action { + case "deduped", "waiting", "requeued", "skipped": + // The round reached a state where its earlier commands are answered and + // spent. "fired" is not here: that round is live and owns its command. + default: + return + } + if _, err := s.Tidy(ctx, res.Repo, res.PR, false); err != nil && s.log != nil { + s.log.Printf("tidy: %s#%d: %v", res.Repo, res.PR, err) + } +} diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go new file mode 100644 index 0000000..70bb1df --- /dev/null +++ b/internal/crq/tidy_test.go @@ -0,0 +1,165 @@ +package crq + +import ( + "context" + "testing" + "time" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// Tidy deletes, so what it must NOT touch matters more than what it does. A PR +// with a spent command from a superseded round, the live round's own command, +// and a person's request to review. +func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Tidy = true + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 12 + head := "bbbbbbbb2" + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = head + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits[head] = commitAt(now.Add(-10 * time.Minute)) + + add := func(id int64, login, body string, at time.Time) { + c := ghapi.IssueComment{ID: id, Body: body, CreatedAt: at, UpdatedAt: at} + c.User.Login = login + gh.comments[fakeKey(repo, pr)] = append(gh.comments[fakeKey(repo, pr)], c) + } + add(100, "kristofferR", cfg.ReviewCommand, now.Add(-2*time.Hour)) // spent: old round + add(200, "kristofferR", cfg.ReviewCommand, now.Add(-time.Minute)) // the live round's + add(300, "someone-else", cfg.ReviewCommand, now.Add(-3*time.Hour)) + // The bot's own answer. It must survive: an auto-reply can be a rate-limit or + // skip notice, which crq reads as evidence and surfaces as a finding. + add(400, cfg.Bot, "\nReview in progress", now.Add(-2*time.Hour)) + + // The bot reviewed after the old command, which is the evidence that it read + // it — and reviewed at the CURRENT head, so the round is answered. + review := ghapi.Review{ID: 900, CommitID: head, State: "COMMENTED", SubmittedAt: now.Add(-90 * time.Minute), Body: "looks fine"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + + // One archived round holding the spent command, one live round holding 200. + if _, err := store.Update(ctx, func(st *State) error { + old, err := st.NewRound(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)) + if err != nil { + return err + } + if err := old.Reserve("t", "h", now.Add(-3*time.Hour)); err != nil { + return err + } + if err := old.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + st.PutRound(*old) + if _, err := st.Supersede(repo, pr, head, now.Add(-time.Hour)); err != nil { + return err + } + live := st.Round(repo, pr) + if err := live.Reserve("t2", "h", now.Add(-2*time.Minute)); err != nil { + return err + } + if err := live.Fire(200, now.Add(-time.Minute)); err != nil { + return err + } + st.PutRound(*live) + return nil + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 1 || result.Deleted[0] != 100 { + t.Fatalf("deleted = %v, want only the spent command from the superseded round", result.Deleted) + } + + left := map[int64]bool{} + for _, c := range gh.comments[fakeKey(repo, pr)] { + left[c.ID] = true + } + if !left[200] { + t.Error("the live round's command was deleted; the next pump will post a duplicate") + } + if !left[300] { + t.Error("a person's review request was deleted; that was not crq's to erase") + } + if !left[400] { + t.Error("a bot comment was deleted; those carry rate-limit and skip notices crq reads as findings") + } +} + +// Dry run must report the same decision and change nothing. +func TestTidyDryRunWritesNothing(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 13 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: cfg.ReviewCommand, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", SubmittedAt: now.Add(-90 * time.Minute), State: "COMMENTED"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + r, err := st.NewRound(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)) + if err != nil { + return err + } + if err := r.Reserve("t", "h", now.Add(-3*time.Hour)); err != nil { + return err + } + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + if err := r.Complete(); err != nil { + return err + } + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, true) + if err != nil { + t.Fatal(err) + } + if !result.DryRun || len(result.Deleted) != 1 { + t.Fatalf("result = %+v, want the same decision, reported not applied", result) + } + if len(gh.comments[fakeKey(repo, pr)]) != 1 { + t.Error("a dry run deleted a comment") + } +} + +// commitAt is a commit object with a committer date, which is the cutoff +// adoption uses and therefore the cutoff tidying respects. +func commitAt(at time.Time) ghapi.Commit { + var c ghapi.Commit + c.Committer.Date = at + return c +} diff --git a/internal/engine/tidy.go b/internal/engine/tidy.go new file mode 100644 index 0000000..60d64b6 --- /dev/null +++ b/internal/engine/tidy.go @@ -0,0 +1,59 @@ +package engine + +import "time" + +// CommandComment is one review-trigger comment crq posted. +type CommandComment struct { + ID int64 + Bot string + CreatedAt time.Time +} + +// TidyInput is everything the decision needs about one PR's trigger comments. +type TidyInput struct { + // Commands are the trigger comments crq itself posted, oldest first. + Commands []CommandComment + // AnsweredAt is the newest moment each bot demonstrably acted — a review, a + // completion reply, a classified event. A command with nothing after it was + // never read, and the instruction is to remove only what has been. + AnsweredAt map[string]time.Time + // Live are the command IDs a round that has NOT progressed still depends on: + // the open round's own command and its co-reviewer triggers. + Live map[int64]bool + // HeadAt is when the current head was committed. A command at or after it is + // still adoptable, so removing it would make crq post a duplicate. + HeadAt time.Time +} + +// StaleCommands returns the trigger comments that can be deleted: crq asked, the +// bot answered, and the round that asked has moved on. +// +// Deleting a comment crq still reads is the way this becomes expensive. Three +// guards, and a command has to clear all of them: +// +// - it belongs to no live round — the user's rule, and the one that matters: +// only rounds that have already progressed; +// - the bot acted after it, so it was actually read rather than merely old; +// - it predates the current head, because adoption only ever considers +// commands newer than the head commit. Delete one of those and the next +// pump sees no command, posts another, and buys a second review. +// +// Anything crq did not post is not here at all: the caller only collects its own +// comments, so a human's "@coderabbitai review" is never a candidate. +func StaleCommands(in TidyInput) []int64 { + var stale []int64 + for _, cmd := range in.Commands { + if in.Live[cmd.ID] { + continue + } + answered, ok := in.AnsweredAt[cmd.Bot] + if !ok || answered.Before(cmd.CreatedAt) { + continue + } + if !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) { + continue + } + stale = append(stale, cmd.ID) + } + return stale +} diff --git a/internal/engine/tidy_test.go b/internal/engine/tidy_test.go new file mode 100644 index 0000000..1e76af7 --- /dev/null +++ b/internal/engine/tidy_test.go @@ -0,0 +1,63 @@ +package engine + +import ( + "testing" + "time" +) + +// Every guard exists because removing the wrong comment costs a review. The +// expensive mistake is deleting one crq would otherwise have adopted: the next +// pump sees no command, posts another, and buys a second review of the same code. +func TestStaleCommandsKeepsWhatCrqStillReads(t *testing.T) { + base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + head := base.Add(30 * time.Minute) + answered := map[string]time.Time{"coderabbitai": base.Add(40 * time.Minute)} + + in := TidyInput{ + HeadAt: head, + AnsweredAt: answered, + Live: map[int64]bool{5: true}, + Commands: []CommandComment{ + {ID: 1, Bot: "coderabbitai", CreatedAt: base}, // stale: answered, old, not live + {ID: 5, Bot: "coderabbitai", CreatedAt: base.Add(time.Minute)}, // live round depends on it + {ID: 6, Bot: "codex", CreatedAt: base.Add(2 * time.Minute)}, // no evidence this bot ever acted + {ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}, // newer than the head: still adoptable + {ID: 8, Bot: "coderabbitai", CreatedAt: base.Add(50 * time.Minute)}, // after the answer AND after the head + }, + } + + got := StaleCommands(in) + if len(got) != 1 || got[0] != 1 { + t.Fatalf("stale = %v, want only the answered, superseded, non-live command", got) + } +} + +// A round that has not progressed keeps its command, whatever else is true. +func TestStaleCommandsNeverTouchesALiveRound(t *testing.T) { + base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + in := TidyInput{ + HeadAt: base.Add(time.Hour), + AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(2 * time.Hour)}, + Live: map[int64]bool{1: true, 2: true}, + Commands: []CommandComment{ + {ID: 1, Bot: "coderabbitai", CreatedAt: base}, + {ID: 2, Bot: "codex", CreatedAt: base}, + }, + } + if got := StaleCommands(in); len(got) != 0 { + t.Errorf("stale = %v, want nothing while the round is live", got) + } +} + +// With no head timestamp, the adoption guard cannot be evaluated, so it must not +// silently pass: an unreadable head is not permission to delete. +func TestStaleCommandsWithoutAHeadStillRequiresEvidence(t *testing.T) { + base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + in := TidyInput{ + AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(time.Hour)}, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + } + if got := StaleCommands(in); len(got) != 1 { + t.Errorf("stale = %v, want the answered command", got) + } +} diff --git a/llms.txt b/llms.txt index ad5de80..0c4203c 100644 --- a/llms.txt +++ b/llms.txt @@ -202,6 +202,16 @@ 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. +crq removes its own spent `@coderabbitai review` / `@codex review` comments as rounds progress +(`CRQ_TIDY=0` disables). On demand: + +```bash +crq tidy REPO PR [--dry-run] +``` + +Only comments crq posted, only from rounds that have progressed, only after the bot answered, and +never the bots' own comments. + Current feedback without triggering a review: ```bash diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index ef9dae2..ddb8b3b 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -171,6 +171,20 @@ 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. +## Spent Trigger Comments + +crq deletes its own `@coderabbitai review` / `@codex review` comments once the round that posted +them has progressed and the bot has answered, so a PR driven through a dozen rounds stays readable. +This happens by itself under `crq autoreview` (`CRQ_TIDY=0` turns it off), or on demand: + +```bash +crq tidy "$REPO" "$PR" [--dry-run] +``` + +It only ever removes comments **crq posted** — candidates come from the command IDs recorded on its +own rounds, never from matching text — and never the bots' own comments, because an auto-generated +reply can be a rate-limit or skipped-review notice that crq reads as evidence. + ## Fleet Auto-Review To keep all open PRs in scope reviewed while CodeRabbit native auto-review is off: From 75bffe71862854c41922ba258d3a10efc4fd8031 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 00:13:09 +0200 Subject: [PATCH 02/12] Remember the review request each retry replaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rate-limited round retries, and every retry posts a NEW "@coderabbitai review" — the bot answered the previous one with the rate-limit notice, so it can never be adopted again. crq overwrote CommandID and forgot the old comment, which is why a throttled PR collects a column of identical requests: nothing knew they were there, and tidying could only ever find commands from rounds that had finished. A round now records the commands it supersedes, bounded so a PR that retries all day cannot grow its round without limit, and tidy treats them as spent whichever phase the round is in. They are exempt from the predates-the-head check on purpose: crq's own record that it has posted a successor is stronger evidence than any timestamp. Observed on PR #50 — five heads, nine review requests, three of them for the first head alone. Codex is not affected: it gets one command per head because crq skips the trigger when the bot has already reviewed it. --- internal/crq/tidy.go | 12 ++++++++ internal/engine/tidy.go | 9 ++++-- internal/engine/tidy_test.go | 10 +++++++ internal/state/spent_test.go | 54 ++++++++++++++++++++++++++++++++++++ internal/state/state.go | 35 +++++++++++++++++++++++ 5 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 internal/state/spent_test.go diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 1d9a900..1a3b9dd 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -53,6 +53,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // closed, cancelled) or completed. A round still working keeps its command, // because that is the comment crq would adopt instead of posting again. live := map[int64]bool{} + superseded := map[int64]bool{} var commands []engine.CommandComment collect := func(r Round, progressed bool) { for _, entry := range roundCommands(r, cfg) { @@ -62,6 +63,16 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T live[entry.ID] = true } } + // Superseded commands are spent whether or not the round has finished. + // A retry answered by a rate-limit notice can never be adopted again, so + // leaving it on the PR is how a throttled review collects a column of + // identical requests. + for _, id := range r.SpentCommands { + superseded[id] = true + commands = append(commands, engine.CommandComment{ + ID: id, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: r.EnqueuedAt, + }) + } } if round := st.Round(repo, pr); round != nil { collect(*round, !round.Active()) @@ -90,6 +101,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T in := engine.TidyInput{ Commands: commands, Live: live, + Superseded: superseded, HeadAt: obs.eng.HeadAt, AnsweredAt: answeredAt(obs, cfg), } diff --git a/internal/engine/tidy.go b/internal/engine/tidy.go index 60d64b6..a80c6ef 100644 --- a/internal/engine/tidy.go +++ b/internal/engine/tidy.go @@ -21,8 +21,13 @@ type TidyInput struct { // the open round's own command and its co-reviewer triggers. Live map[int64]bool // HeadAt is when the current head was committed. A command at or after it is - // still adoptable, so removing it would make crq post a duplicate. + // still adoptable, so removing it would make crq post a duplicate — unless + // the round itself has already replaced it (Superseded). HeadAt time.Time + // Superseded are commands the round explicitly moved past by posting a newer + // one. They are exempt from the head check: crq's own record that it has + // replaced a command is stronger evidence than any timestamp. + Superseded map[int64]bool } // StaleCommands returns the trigger comments that can be deleted: crq asked, the @@ -50,7 +55,7 @@ func StaleCommands(in TidyInput) []int64 { if !ok || answered.Before(cmd.CreatedAt) { continue } - if !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) { + if !in.Superseded[cmd.ID] && !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) { continue } stale = append(stale, cmd.ID) diff --git a/internal/engine/tidy_test.go b/internal/engine/tidy_test.go index 1e76af7..249a70d 100644 --- a/internal/engine/tidy_test.go +++ b/internal/engine/tidy_test.go @@ -30,6 +30,16 @@ func TestStaleCommandsKeepsWhatCrqStillReads(t *testing.T) { if len(got) != 1 || got[0] != 1 { t.Fatalf("stale = %v, want only the answered, superseded, non-live command", got) } + + // A retry the round explicitly replaced is spent even though it is newer + // than the head: crq's own record that it posted a successor is stronger + // evidence than the timestamp. This is the case that leaves a rate-limited + // PR with a column of identical review requests. + in.Superseded = map[int64]bool{8: true} + got = StaleCommands(in) + if len(got) != 2 || got[1] != 8 { + t.Fatalf("stale = %v, want the superseded retry included", got) + } } // A round that has not progressed keeps its command, whatever else is true. diff --git a/internal/state/spent_test.go b/internal/state/spent_test.go new file mode 100644 index 0000000..492d795 --- /dev/null +++ b/internal/state/spent_test.go @@ -0,0 +1,54 @@ +package state + +import ( + "testing" + "time" +) + +// A rate-limited round retries, and each retry posts a NEW review request. crq +// overwrote CommandID and forgot the old one, so nothing knew those comments +// existed — which is why a throttled PR collects a column of identical requests +// that tidying could never find. +func TestFireRemembersTheCommandItSuperseded(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + r := &Round{Repo: "o/r", PR: 1, Head: "aaaaaaaa1", Phase: PhaseQueued} + + fire := func(id int64) { + t.Helper() + if r.Phase != PhaseQueued { + if err := r.AwaitRetry(now, "rate limited", now); err != nil { + t.Fatal(err) + } + r.Phase = PhaseQueued + } + if err := r.Reserve("tok", "host", now); err != nil { + t.Fatal(err) + } + if err := r.Fire(id, now); err != nil { + t.Fatal(err) + } + } + fire(100) + fire(200) + fire(300) + + if len(r.SpentCommands) != 2 || r.SpentCommands[0] != 100 || r.SpentCommands[1] != 200 { + t.Fatalf("spent = %v, want the two superseded commands", r.SpentCommands) + } + if r.CommandID != 300 { + t.Errorf("CommandID = %d, want the newest", r.CommandID) + } + // Re-firing the same id is not a supersede. + fire(300) + if len(r.SpentCommands) != 2 { + t.Errorf("spent = %v, want no change when the command is unchanged", r.SpentCommands) + } + + // Bounded: a PR that retries all day must not grow its round without limit. + for i := int64(1000); i < 1100; i++ { + fire(i) + } + if len(r.SpentCommands) > 50 { + t.Errorf("spent grew to %d entries", len(r.SpentCommands)) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index ede4e85..9bfdcd8 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -79,6 +79,13 @@ type Round struct { // CodeRabbit requests must skip these. CoOnly bool `json:"co_only,omitempty"` + // SpentCommands are trigger comments this round posted and then moved past. + // A retry supersedes its predecessor — the bot answered the old one, usually + // with a rate-limit notice, so it can never be adopted again. Without this + // record nothing knows those comments exist, which is why a throttled PR + // collects a column of identical review requests. + SpentCommands []int64 `json:"spent_commands,omitempty"` + // RetryAt is the earliest time this head may fire again (awaiting_retry). RetryAt *time.Time `json:"retry_at,omitempty"` @@ -170,6 +177,11 @@ func (r *Round) setCo(login string, c CoBotRound) { // round and releases its claim. func (r *Round) SetCoCommand(login string, commandID int64, at time.Time) { c := r.Co(login) + // Same as the primary: a replaced co-reviewer trigger is spent, and only + // this round remembers the comment it left on the PR. + if c.CommandID != 0 && c.CommandID != commandID { + r.SpentCommands = appendSpent(r.SpentCommands, c.CommandID) + } c.CommandID = commandID t := at.UTC() c.CommandedAt = &t @@ -347,6 +359,13 @@ func (r *Round) Fire(commandID int64, at time.Time) error { return r.illegal(PhaseFired) } r.Phase = PhaseFired + // A retry posts a NEW command: the bot answered the previous one (with a + // rate-limit notice, typically) so it can never be adopted again. Remember + // it, or the comment it left behind is one nothing knows about — which is + // why a rate-limited PR collects a column of identical review requests. + if r.CommandID != 0 && r.CommandID != commandID { + r.SpentCommands = appendSpent(r.SpentCommands, r.CommandID) + } r.CommandID = commandID t := at.UTC() r.FiredAt = &t @@ -605,6 +624,22 @@ func (s *State) QueuedRounds(now time.Time) []Round { return out } +// appendSpent records a superseded command, bounded so a PR that retries all day +// cannot grow its round without limit. +func appendSpent(spent []int64, id int64) []int64 { + const maxSpent = 50 + for _, have := range spent { + if have == id { + return spent + } + } + spent = append(spent, id) + if len(spent) > maxSpent { + spent = spent[len(spent)-maxSpent:] + } + return spent +} + // 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 180cb8ae8e0679d76369699b56f1c12961148afd Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 03:38:07 +0200 Subject: [PATCH 03/12] Only delete the review requests crq wrote itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adoption was the hole. A round records the command it adopted in exactly the same CommandID as one it posted, so "the command IDs crq recorded on its own rounds" included a person's "@coderabbitai review" — and once the head moved on and the bot had spoken, tidy would delete it. With a maintainer token nothing even objects. A round now records the comments it WROTE, with the reviewer each was addressed to and when it landed, and that list is the only candidate source. Adopting a comment is not writing one. It also replaces the spent-command list: a command missing from the round's current anchors is one the round replaced, which is the same fact without a second field. Four things fall out of carrying the reviewer with the comment: * a co-only round's anchor is the co-reviewer's trigger, not the primary's — unrelated CodeRabbit activity no longer passes for the answer that trigger is still waiting on, and the id stops appearing on the deletion list twice; * a superseded co-reviewer trigger is attributed to the bot that was asked rather than to CodeRabbit; * candidates are filtered against the comments actually on the PR, so a comment tidy already deleted is not DELETEd again on every later pass with the 404 read back as a fresh removal; * state is re-read after the observation and before the deletes, so a round another fleet member started in that window keeps its command. An unreadable head commit now keeps every non-superseded command instead of clearing the guard: the command may well be adoptable again once the read recovers, and deleting it buys a second review. And a round that completes while holding the slot ("cleared") or inside the reviewing sweep is tidied — between them that is most successful rounds, so the automatic cleanup was barely running. --- cmd/crq/main.go | 10 +- internal/crq/codex_replay_test.go | 35 +++++++ internal/crq/service.go | 15 +++ internal/crq/tidy.go | 168 ++++++++++++++++++------------ internal/crq/tidy_test.go | 79 +++++++++++++- internal/engine/tidy.go | 13 ++- internal/engine/tidy_test.go | 137 ++++++++++++++---------- internal/state/posted_test.go | 73 +++++++++++++ internal/state/spent_test.go | 54 ---------- internal/state/state.go | 69 ++++++------ llms.txt | 6 +- skills/coderabbit-queue/SKILL.md | 9 +- 12 files changed, 451 insertions(+), 217 deletions(-) create mode 100644 internal/state/posted_test.go delete mode 100644 internal/state/spent_test.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 3e6b46d..93624a5 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -542,14 +542,16 @@ which buries the conversation a human came to read. A comment is removed only when all three hold: - * crq posted it — the candidates are the command IDs crq recorded on its own - rounds, not anything matching the text. A person's "@coderabbitai review" is - their decision to ask and not crq's to erase. + * crq WROTE it — the candidates are the comments each round recorded posting, + not anything matching the text and not one the round adopted. A person's + "@coderabbitai review" is their decision to ask and not crq's to erase. * the round that asked has PROGRESSED. A live round keeps its command, because that is the comment crq adopts instead of posting another one. * the bot answered it, and it predates the current head. Adoption only ever considers commands newer than the head commit, so deleting one of those - would make the next pump post a duplicate and buy a second review. + would make the next pump post a duplicate and buy a second review; a head + crq cannot read leaves the check unevaluable, and everything stays. Only a + request crq's own retry replaced is spent whatever its timestamp says. It never deletes the bots' own comments. An auto-generated reply can be a rate-limit or skipped-review notice, which crq reads as evidence and surfaces as diff --git a/internal/crq/codex_replay_test.go b/internal/crq/codex_replay_test.go index fc84fe7..a382e30 100644 --- a/internal/crq/codex_replay_test.go +++ b/internal/crq/codex_replay_test.go @@ -446,6 +446,41 @@ func TestFireCoOnlyPostFailureParks(t *testing.T) { } } +// A co-only round's CommandID is the co-reviewer's trigger comment — one +// comment under two names. Recording it as the primary's own would let +// unrelated CodeRabbit activity pass for the answer that trigger is still +// waiting on, and would put the same id on the cleanup list twice. +func TestFireCoOnlyRecordsItsAnchorAsTheCoReviewersCommand(t *testing.T) { + base := time.Date(2026, 7, 17, 12, 0, 0, 0, time.UTC) + f := newCodexReplayFixture(t, base, func(cfg *Config) { + cfg.RequiredBots = []string{cfg.Bot, codexLogin} + }) + repo, pr, head := "o/r", 13, "aaaabbbbccccdddd" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Hour)) + // CodeRabbit already reviewed the head → DecideFire returns FireCoOnly. + f.botReview(repo, pr, 500, head, base.Add(-time.Minute)) + + f.enqueue(repo, pr) + if res := f.pump(); res.Action != "fired" { + t.Fatalf("expected a co-only fire, got %+v", res) + } + r := f.round(repo, pr) + if r == nil || !r.CoOnly { + t.Fatalf("expected a co-only round, got %+v", r) + } + if len(r.PostedCommands) != 1 { + t.Fatalf("posted = %v, want the one comment crq wrote", r.PostedCommands) + } + posted := r.PostedCommands[0] + if posted.ID != r.CommandID { + t.Errorf("posted id = %d, want the round's anchor %d", posted.ID, r.CommandID) + } + if dialect.NormalizeBotName(posted.Bot) != dialect.NormalizeBotName(codexLogin) { + t.Errorf("posted bot = %q, want the co-reviewer it was addressed to", posted.Bot) + } +} + // TestSelfHealCodexClaimPreventsDoublePost pins claim-before-post: two sweepers // observing the same round with CodexCommandID==0 must produce exactly one // `@codex review` — the second claim fails under CAS. diff --git a/internal/crq/service.go b/internal/crq/service.go index fb33c75..8f4402f 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -652,6 +652,10 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( return st, err } s.sync(ctx, updated) + // A round completed here is invisible to the caller — Pump goes on to report + // whatever it fires next, or idle — so this is the only moment that knows the + // PR's trigger comments are spent. + s.tidyProgressed(ctx, target.Repo, target.PR) return updated, nil } @@ -1071,6 +1075,11 @@ func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, st.Warn = "" } for _, p := range posts { + // Recorded against the co-reviewer it was addressed to, including the + // one anchoring the round: a co-only round's CommandID is that same + // comment, and calling it the primary's would let unrelated CodeRabbit + // activity pass for the answer this trigger is still waiting on. + r.RecordPosted(p.login, p.id, p.at) if r.Co(p.login).CommandID == 0 { r.SetCoCommand(p.login, p.id, p.at) } @@ -1219,7 +1228,12 @@ func (s *Service) recordFire(ctx context.Context, round Round, token string, com if err := r.Fire(commandID, firedAt); err != nil { return err } + // Recorded as crq's own whatever the round then does with it: the + // comment is on the PR either way, and the record is the only proof + // crq (rather than a person) wrote it. + r.RecordPosted(s.cfg.Bot, commandID, firedAt) for _, p := range coPosts { + r.RecordPosted(p.login, p.id, p.at) if r.Co(p.login).CommandID == 0 { r.SetCoCommand(p.login, p.id, p.at) } @@ -1298,6 +1312,7 @@ func (s *Service) fireCoTrigger(ctx context.Context, round Round, login string) if !sameRound(r, round) { return ErrNoChange } + r.RecordPosted(login, id, at) if r.Co(login).CommandID == 0 { r.SetCoCommand(login, id, at) } else { diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 1a3b9dd..8c93e47 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -33,7 +33,9 @@ type TidyResult struct { // // Only comments CRQ POSTED. A human's "@coderabbitai review" is someone's // decision to ask, and not crq's to erase; the candidate list is built from the -// command IDs crq recorded on its own rounds, not by matching text. +// comments each round recorded WRITING (Round.PostedCommands), not by matching +// text and not from the round's CommandID — a round records an adopted command +// there too, and adoption is exactly how a person's request gets into it. // // Only crq's, not the bot's. An auto-generated reply can be a rate-limit notice // or a skipped-review notice, which crq classifies as evidence and surfaces as @@ -47,43 +49,8 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T if err != nil { return result, err } - cfg := s.cfg - - // Candidates come from rounds that have PROGRESSED — archived (superseded, - // closed, cancelled) or completed. A round still working keeps its command, - // because that is the comment crq would adopt instead of posting again. - live := map[int64]bool{} - superseded := map[int64]bool{} - var commands []engine.CommandComment - collect := func(r Round, progressed bool) { - for _, entry := range roundCommands(r, cfg) { - if progressed { - commands = append(commands, entry) - } else { - live[entry.ID] = true - } - } - // Superseded commands are spent whether or not the round has finished. - // A retry answered by a rate-limit notice can never be adopted again, so - // leaving it on the PR is how a throttled review collects a column of - // identical requests. - for _, id := range r.SpentCommands { - superseded[id] = true - commands = append(commands, engine.CommandComment{ - ID: id, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: r.EnqueuedAt, - }) - } - } - if round := st.Round(repo, pr); round != nil { - collect(*round, !round.Active()) - } - for _, archived := range st.Archive { - if NormalizeRepo(archived.Repo) == repo && archived.PR == pr { - collect(archived, true) - } - } - if len(commands) == 0 { - result.Kept = append(result.Kept, "no progressed round has a trigger comment to remove") + if len(collectPosted(st, repo, pr).commands) == 0 { + result.Kept = append(result.Kept, "no round on this pr posted a trigger comment") return result, nil } @@ -98,16 +65,43 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, nil } + // Re-read state AFTER the observation. What follows is destructive, and the + // snapshot above is old enough for another fleet member to have started a + // round that adopted one of these comments — which would make this delete the + // command that round is now waiting on. + st, _, err = s.store.Load(ctx) + if err != nil { + return result, err + } + posted := collectPosted(st, repo, pr) + + // A deleted comment stays on its round for ever, so without this every later + // pass would DELETE it again and read the 404 as a fresh removal. + present := map[int64]bool{} + for _, comment := range obs.comments { + present[comment.ID] = true + } + var commands []engine.CommandComment + for _, cmd := range posted.commands { + if present[cmd.ID] { + commands = append(commands, cmd) + } + } + if len(commands) == 0 { + result.Kept = append(result.Kept, "every trigger comment crq posted is already gone") + return result, nil + } + in := engine.TidyInput{ Commands: commands, - Live: live, - Superseded: superseded, + Live: posted.live, + Superseded: posted.superseded, HeadAt: obs.eng.HeadAt, - AnsweredAt: answeredAt(obs, cfg), + AnsweredAt: answeredAt(obs, s.cfg), } stale := engine.StaleCommands(in) if len(stale) == 0 { - result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or newer than the head") + result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or not yet past the current head") return result, nil } if dryRun || s.cfg.DryRun { @@ -136,26 +130,57 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, nil } -// roundCommands lists the trigger comments crq recorded on one round: the -// primary's, and each co-reviewer's. -func roundCommands(r Round, cfg Config) []engine.CommandComment { - var out []engine.CommandComment - at := time.Time{} - if r.FiredAt != nil { - at = r.FiredAt.UTC() - } - if r.CommandID != 0 { - out = append(out, engine.CommandComment{ID: r.CommandID, Bot: dialect.NormalizeBotName(cfg.Bot), CreatedAt: at}) - } - for login, co := range r.CoBots { - if co.CommandID == 0 { - continue +// postedCommands is one PR's trigger comments as tidying sees them. +type postedCommands struct { + // commands are the comments crq POSTED, each with the reviewer it was + // addressed to. A round's CommandID is not in here unless crq wrote it: an + // adopted command is someone else's comment, and deleting a person's + // "@coderabbitai review" is not crq's to do. + commands []engine.CommandComment + // live are the commands a round that has NOT progressed still depends on — + // posted or adopted, because either is what crq would read instead of + // posting again. + live map[int64]bool + // superseded are commands their own round replaced with a newer one, so + // they can never be adopted again whatever phase that round is in. + superseded map[int64]bool +} + +// collectPosted gathers the trigger comments crq posted for repo#pr, from the +// open round and from every archived round of the same PR. +func collectPosted(st State, repo string, pr int) postedCommands { + out := postedCommands{live: map[int64]bool{}, superseded: map[int64]bool{}} + collect := func(r Round, progressed bool) { + current := map[int64]bool{} + if r.CommandID != 0 { + current[r.CommandID] = true } - when := at - if co.CommandedAt != nil { - when = co.CommandedAt.UTC() + for _, co := range r.CoBots { + if co.CommandID != 0 { + current[co.CommandID] = true + } + } + if !progressed { + for id := range current { + out.live[id] = true + } + } + for _, p := range r.PostedCommands { + if !current[p.ID] { + out.superseded[p.ID] = true + } + out.commands = append(out.commands, engine.CommandComment{ + ID: p.ID, Bot: dialect.NormalizeBotName(p.Bot), CreatedAt: p.At.UTC(), + }) + } + } + if round := st.Round(repo, pr); round != nil { + collect(*round, !round.Active()) + } + for _, archived := range st.Archive { + if NormalizeRepo(archived.Repo) == repo && archived.PR == pr { + collect(archived, true) } - out = append(out, engine.CommandComment{ID: co.CommandID, Bot: dialect.NormalizeBotName(login), CreatedAt: when}) } return out } @@ -201,17 +226,30 @@ func eventAt(e dialect.BotEvent) time.Time { // It is best-effort by design. Deleting a comment is housekeeping, and a // housekeeping failure must never break the pass that did the real work. func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) { - if !s.cfg.Tidy || res.Repo == "" || res.PR == 0 { + if res.Repo == "" || res.PR == 0 { return } switch res.Action { - case "deduped", "waiting", "requeued", "skipped": + case "cleared", "deduped", "waiting", "requeued", "skipped": // The round reached a state where its earlier commands are answered and - // spent. "fired" is not here: that round is live and owns its command. + // spent. "cleared" is the ordinary one — the round holding the slot + // completed or was acknowledged — and leaving it out meant the common + // successful round was never tidied at all. "fired" is not here: that + // round is live and owns its command. default: return } - if _, err := s.Tidy(ctx, res.Repo, res.PR, false); err != nil && s.log != nil { - s.log.Printf("tidy: %s#%d: %v", res.Repo, res.PR, err) + s.tidyProgressed(ctx, res.Repo, res.PR) +} + +// tidyProgressed runs a tidy pass for one PR whose round just moved, and +// swallows the outcome: deleting a comment is housekeeping, and a housekeeping +// failure must never break the pass that did the real work. +func (s *Service) tidyProgressed(ctx context.Context, repo string, pr int) { + if !s.cfg.Tidy { + return + } + if _, err := s.Tidy(ctx, repo, pr, false); err != nil && s.log != nil { + s.log.Printf("tidy: %s#%d: %v", repo, pr, err) } } diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 70bb1df..8687d40 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -34,7 +34,10 @@ func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { } add(100, "kristofferR", cfg.ReviewCommand, now.Add(-2*time.Hour)) // spent: old round add(200, "kristofferR", cfg.ReviewCommand, now.Add(-time.Minute)) // the live round's - add(300, "someone-else", cfg.ReviewCommand, now.Add(-3*time.Hour)) + // A person's request that a round ADOPTED as its own fire. The round records + // it in exactly the same CommandID as one crq posted, so only "did crq write + // it" keeps this from being deleted out from under whoever asked. + add(300, "someone-else", cfg.ReviewCommand, now.Add(-4*time.Hour)) // The bot's own answer. It must survive: an auto-reply can be a rate-limit or // skip notice, which crq reads as evidence and surfaces as a finding. add(400, cfg.Bot, "\nReview in progress", now.Add(-2*time.Hour)) @@ -48,18 +51,28 @@ func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { store := NewMemoryStore(cfg) svc := NewService(cfg, gh, store, nil) - // One archived round holding the spent command, one live round holding 200. + // Three rounds: one that adopted the person's command, one holding the spent + // command crq posted, and the live round holding 200. if _, err := store.Update(ctx, func(st *State) error { - old, err := st.NewRound(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)) + adopting, err := st.NewRound(repo, pr, "aaaaaaaa0", now.Add(-5*time.Hour)) if err != nil { return err } + if err := adopting.Fire(300, now.Add(-4*time.Hour)); err != nil { + return err + } + st.PutRound(*adopting) + if _, err := st.Supersede(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)); err != nil { + return err + } + old := st.Round(repo, pr) if err := old.Reserve("t", "h", now.Add(-3*time.Hour)); err != nil { return err } if err := old.Fire(100, now.Add(-2*time.Hour)); err != nil { return err } + old.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) st.PutRound(*old) if _, err := st.Supersede(repo, pr, head, now.Add(-time.Hour)); err != nil { return err @@ -71,6 +84,7 @@ func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { if err := live.Fire(200, now.Add(-time.Minute)); err != nil { return err } + live.RecordPosted(cfg.Bot, 200, now.Add(-time.Minute)) st.PutRound(*live) return nil }); err != nil { @@ -93,7 +107,7 @@ func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { t.Error("the live round's command was deleted; the next pump will post a duplicate") } if !left[300] { - t.Error("a person's review request was deleted; that was not crq's to erase") + t.Error("a person's review request was deleted; adopting one is not writing it") } if !left[400] { t.Error("a bot comment was deleted; those carry rate-limit and skip notices crq reads as findings") @@ -135,6 +149,7 @@ func TestTidyDryRunWritesNothing(t *testing.T) { if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { return err } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) if err := r.Complete(); err != nil { return err } @@ -156,6 +171,62 @@ func TestTidyDryRunWritesNothing(t *testing.T) { } } +// A command crq deleted stays recorded on its round for ever, so a pass that +// did not check what is still on the PR would DELETE it again every time and +// read the 404 back as a fresh removal. +func TestTidySkipsCommandsAlreadyGone(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 14 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", SubmittedAt: now.Add(-90 * time.Minute), State: "COMMENTED"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + // The round remembers posting command 100; the PR no longer has it. + if _, err := store.Update(ctx, func(st *State) error { + r, err := st.NewRound(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)) + if err != nil { + return err + } + if err := r.Reserve("t", "h", now.Add(-3*time.Hour)); err != nil { + return err + } + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + if err := r.Complete(); err != nil { + return err + } + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 0 { + t.Errorf("deleted = %v, want nothing: the comment is already gone", result.Deleted) + } + if len(gh.deleted) != 0 { + t.Errorf("issued %d delete(s) for a comment that is not on the pr", len(gh.deleted)) + } +} + // commitAt is a commit object with a committer date, which is the cutoff // adoption uses and therefore the cutoff tidying respects. func commitAt(at time.Time) ghapi.Commit { diff --git a/internal/engine/tidy.go b/internal/engine/tidy.go index a80c6ef..3827cee 100644 --- a/internal/engine/tidy.go +++ b/internal/engine/tidy.go @@ -22,7 +22,9 @@ type TidyInput struct { Live map[int64]bool // HeadAt is when the current head was committed. A command at or after it is // still adoptable, so removing it would make crq post a duplicate — unless - // the round itself has already replaced it (Superseded). + // the round itself has already replaced it (Superseded). Zero when the head + // commit could not be read, which is not permission to delete: the guard is + // simply unevaluable, and an unevaluable guard keeps the comment. HeadAt time.Time // Superseded are commands the round explicitly moved past by posting a newer // one. They are exempt from the head check: crq's own record that it has @@ -41,7 +43,12 @@ type TidyInput struct { // - the bot acted after it, so it was actually read rather than merely old; // - it predates the current head, because adoption only ever considers // commands newer than the head commit. Delete one of those and the next -// pump sees no command, posts another, and buys a second review. +// pump sees no command, posts another, and buys a second review. An +// unreadable head means the guard cannot be evaluated, and the command +// stays: it may well be adoptable again once the read recovers. +// +// Only a command the round itself replaced (Superseded) skips the head check — +// crq's own record that it posted a successor outranks any timestamp. // // Anything crq did not post is not here at all: the caller only collects its own // comments, so a human's "@coderabbitai review" is never a candidate. @@ -55,7 +62,7 @@ func StaleCommands(in TidyInput) []int64 { if !ok || answered.Before(cmd.CreatedAt) { continue } - if !in.Superseded[cmd.ID] && !in.HeadAt.IsZero() && !cmd.CreatedAt.Before(in.HeadAt) { + if !in.Superseded[cmd.ID] && (in.HeadAt.IsZero() || !cmd.CreatedAt.Before(in.HeadAt)) { continue } stale = append(stale, cmd.ID) diff --git a/internal/engine/tidy_test.go b/internal/engine/tidy_test.go index 249a70d..7085a82 100644 --- a/internal/engine/tidy_test.go +++ b/internal/engine/tidy_test.go @@ -8,66 +8,97 @@ import ( // Every guard exists because removing the wrong comment costs a review. The // expensive mistake is deleting one crq would otherwise have adopted: the next // pump sees no command, posts another, and buys a second review of the same code. -func TestStaleCommandsKeepsWhatCrqStillReads(t *testing.T) { +func TestStaleCommands(t *testing.T) { base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) head := base.Add(30 * time.Minute) answered := map[string]time.Time{"coderabbitai": base.Add(40 * time.Minute)} - in := TidyInput{ - HeadAt: head, - AnsweredAt: answered, - Live: map[int64]bool{5: true}, - Commands: []CommandComment{ - {ID: 1, Bot: "coderabbitai", CreatedAt: base}, // stale: answered, old, not live - {ID: 5, Bot: "coderabbitai", CreatedAt: base.Add(time.Minute)}, // live round depends on it - {ID: 6, Bot: "codex", CreatedAt: base.Add(2 * time.Minute)}, // no evidence this bot ever acted - {ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}, // newer than the head: still adoptable - {ID: 8, Bot: "coderabbitai", CreatedAt: base.Add(50 * time.Minute)}, // after the answer AND after the head + cases := []struct { + name string + in TidyInput + want []int64 + }{ + { + name: "answered, old, and no round depends on it", + in: TidyInput{ + HeadAt: head, + AnsweredAt: answered, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + }, + want: []int64{1}, }, - } - - got := StaleCommands(in) - if len(got) != 1 || got[0] != 1 { - t.Fatalf("stale = %v, want only the answered, superseded, non-live command", got) - } - - // A retry the round explicitly replaced is spent even though it is newer - // than the head: crq's own record that it posted a successor is stronger - // evidence than the timestamp. This is the case that leaves a rate-limited - // PR with a column of identical review requests. - in.Superseded = map[int64]bool{8: true} - got = StaleCommands(in) - if len(got) != 2 || got[1] != 8 { - t.Fatalf("stale = %v, want the superseded retry included", got) - } -} - -// A round that has not progressed keeps its command, whatever else is true. -func TestStaleCommandsNeverTouchesALiveRound(t *testing.T) { - base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) - in := TidyInput{ - HeadAt: base.Add(time.Hour), - AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(2 * time.Hour)}, - Live: map[int64]bool{1: true, 2: true}, - Commands: []CommandComment{ - {ID: 1, Bot: "coderabbitai", CreatedAt: base}, - {ID: 2, Bot: "codex", CreatedAt: base}, + { + name: "a round that has not progressed keeps its command", + in: TidyInput{ + HeadAt: head, + AnsweredAt: answered, + Live: map[int64]bool{1: true, 2: true}, + Commands: []CommandComment{ + {ID: 1, Bot: "coderabbitai", CreatedAt: base}, + {ID: 2, Bot: "codex", CreatedAt: base}, + }, + }, + }, + { + name: "no evidence this bot ever acted", + in: TidyInput{ + HeadAt: head, + AnsweredAt: answered, + Commands: []CommandComment{{ID: 6, Bot: "codex", CreatedAt: base}}, + }, + }, + { + name: "newer than the head: still adoptable", + in: TidyInput{ + HeadAt: head, + AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, + Commands: []CommandComment{{ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, + }, + }, + { + // This is the case that leaves a rate-limited PR with a column of + // identical review requests: the retry that replaced this command is + // newer than the head too, so the adoption guard alone never clears it. + name: "a retry the round replaced is spent despite the head", + in: TidyInput{ + HeadAt: head, + AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, + Superseded: map[int64]bool{8: true}, + Commands: []CommandComment{{ID: 8, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, + }, + want: []int64{8}, + }, + { + // An unreadable head commit is not permission to delete: the command + // may still be adoptable once the read recovers. + name: "no head timestamp keeps what the guard cannot clear", + in: TidyInput{ + AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(time.Hour)}, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + }, + }, + { + name: "no head timestamp still releases a replaced command", + in: TidyInput{ + AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(time.Hour)}, + Superseded: map[int64]bool{1: true}, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + }, + want: []int64{1}, }, } - if got := StaleCommands(in); len(got) != 0 { - t.Errorf("stale = %v, want nothing while the round is live", got) - } -} -// With no head timestamp, the adoption guard cannot be evaluated, so it must not -// silently pass: an unreadable head is not permission to delete. -func TestStaleCommandsWithoutAHeadStillRequiresEvidence(t *testing.T) { - base := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) - in := TidyInput{ - AnsweredAt: map[string]time.Time{"coderabbitai": base.Add(time.Hour)}, - Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, - } - if got := StaleCommands(in); len(got) != 1 { - t.Errorf("stale = %v, want the answered command", got) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := StaleCommands(tc.in) + if len(got) != len(tc.want) { + t.Fatalf("stale = %v, want %v", got, tc.want) + } + for i, id := range tc.want { + if got[i] != id { + t.Fatalf("stale = %v, want %v", got, tc.want) + } + } + }) } } diff --git a/internal/state/posted_test.go b/internal/state/posted_test.go new file mode 100644 index 0000000..fe3f658 --- /dev/null +++ b/internal/state/posted_test.go @@ -0,0 +1,73 @@ +package state + +import ( + "testing" + "time" +) + +// A rate-limited round retries, and each retry posts a NEW review request. crq +// overwrote CommandID and forgot the old one, so nothing knew those comments +// existed — which is why a throttled PR collects a column of identical requests +// that tidying could never find. +func TestRecordPostedRemembersEveryCommandCrqWrote(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + r := &Round{Repo: "o/r", PR: 1, Head: "aaaaaaaa1", Phase: PhaseQueued} + + fire := func(id int64) { + t.Helper() + if r.Phase != PhaseQueued { + if err := r.AwaitRetry(now, "rate limited", now); err != nil { + t.Fatal(err) + } + r.Phase = PhaseQueued + } + if err := r.Reserve("tok", "host", now); err != nil { + t.Fatal(err) + } + if err := r.Fire(id, now); err != nil { + t.Fatal(err) + } + r.RecordPosted("coderabbitai", id, now) + } + fire(100) + fire(200) + fire(300) + + if len(r.PostedCommands) != 3 || r.PostedCommands[0].ID != 100 || r.PostedCommands[2].ID != 300 { + t.Fatalf("posted = %v, want every request this round wrote", r.PostedCommands) + } + if r.PostedCommands[0].Bot != "coderabbitai" { + t.Errorf("bot = %q, want the reviewer the request was addressed to", r.PostedCommands[0].Bot) + } + if r.CommandID != 300 { + t.Errorf("CommandID = %d, want the newest", r.CommandID) + } + // Re-recording the same id is not a second comment. + fire(300) + if len(r.PostedCommands) != 3 { + t.Errorf("posted = %v, want no change when the command is unchanged", r.PostedCommands) + } + + // Bounded: a PR that retries all day must not grow its round without limit. + for i := int64(1000); i < 1100; i++ { + fire(i) + } + if len(r.PostedCommands) > 50 { + t.Errorf("posted grew to %d entries", len(r.PostedCommands)) + } +} + +// A round records an ADOPTED command in exactly the same CommandID, so only the +// posted list can tell the two apart — and getting that wrong means deleting a +// person's request to review. +func TestFireAloneClaimsNoAuthorship(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + r := &Round{Repo: "o/r", PR: 1, Head: "aaaaaaaa1", Phase: PhaseQueued} + if err := r.Fire(77, now); err != nil { + t.Fatal(err) + } + r.SetCoCommand("chatgpt-codex-connector", 78, now) + if len(r.PostedCommands) != 0 { + t.Fatalf("posted = %v, want nothing: crq wrote neither comment", r.PostedCommands) + } +} diff --git a/internal/state/spent_test.go b/internal/state/spent_test.go deleted file mode 100644 index 492d795..0000000 --- a/internal/state/spent_test.go +++ /dev/null @@ -1,54 +0,0 @@ -package state - -import ( - "testing" - "time" -) - -// A rate-limited round retries, and each retry posts a NEW review request. crq -// overwrote CommandID and forgot the old one, so nothing knew those comments -// existed — which is why a throttled PR collects a column of identical requests -// that tidying could never find. -func TestFireRemembersTheCommandItSuperseded(t *testing.T) { - now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) - r := &Round{Repo: "o/r", PR: 1, Head: "aaaaaaaa1", Phase: PhaseQueued} - - fire := func(id int64) { - t.Helper() - if r.Phase != PhaseQueued { - if err := r.AwaitRetry(now, "rate limited", now); err != nil { - t.Fatal(err) - } - r.Phase = PhaseQueued - } - if err := r.Reserve("tok", "host", now); err != nil { - t.Fatal(err) - } - if err := r.Fire(id, now); err != nil { - t.Fatal(err) - } - } - fire(100) - fire(200) - fire(300) - - if len(r.SpentCommands) != 2 || r.SpentCommands[0] != 100 || r.SpentCommands[1] != 200 { - t.Fatalf("spent = %v, want the two superseded commands", r.SpentCommands) - } - if r.CommandID != 300 { - t.Errorf("CommandID = %d, want the newest", r.CommandID) - } - // Re-firing the same id is not a supersede. - fire(300) - if len(r.SpentCommands) != 2 { - t.Errorf("spent = %v, want no change when the command is unchanged", r.SpentCommands) - } - - // Bounded: a PR that retries all day must not grow its round without limit. - for i := int64(1000); i < 1100; i++ { - fire(i) - } - if len(r.SpentCommands) > 50 { - t.Errorf("spent grew to %d entries", len(r.SpentCommands)) - } -} diff --git a/internal/state/state.go b/internal/state/state.go index 9bfdcd8..94e196e 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -79,12 +79,16 @@ type Round struct { // CodeRabbit requests must skip these. CoOnly bool `json:"co_only,omitempty"` - // SpentCommands are trigger comments this round posted and then moved past. - // A retry supersedes its predecessor — the bot answered the old one, usually - // with a rate-limit notice, so it can never be adopted again. Without this - // record nothing knows those comments exist, which is why a throttled PR - // collects a column of identical review requests. - SpentCommands []int64 `json:"spent_commands,omitempty"` + // PostedCommands are the trigger comments crq itself WROTE for this round, + // including the ones a retry replaced. Two things need it. A retry posts a + // new command — the bot answered the previous one, usually with a rate-limit + // notice, so it can never be adopted again — and without this record nothing + // knows the old comment exists, which is why a throttled PR collects a + // column of identical review requests. And CommandID alone cannot say who + // wrote a comment: a round records an ADOPTED command there just the same, + // so treating it as crq's own is how tidying would erase a person's request + // to review. + PostedCommands []PostedCommand `json:"posted_commands,omitempty"` // RetryAt is the earliest time this head may fire again (awaiting_retry). RetryAt *time.Time `json:"retry_at,omitempty"` @@ -120,6 +124,15 @@ type Round struct { unknown unknownFields } +// PostedCommand is one trigger comment crq posted, with the reviewer it was +// addressed to and when it landed — the two facts a later cleanup needs to +// decide whether that reviewer has read it yet. +type PostedCommand struct { + ID int64 `json:"id"` + Bot string `json:"bot,omitempty"` + At time.Time `json:"at,omitempty"` +} + // CoBotRound is one co-reviewer's bookkeeping inside a Round: the trigger // comment crq posted/adopted for it, and the CAS claim that serializes the // pending post (the per-round concurrency guard — co-reviewer fires never @@ -177,11 +190,6 @@ func (r *Round) setCo(login string, c CoBotRound) { // round and releases its claim. func (r *Round) SetCoCommand(login string, commandID int64, at time.Time) { c := r.Co(login) - // Same as the primary: a replaced co-reviewer trigger is spent, and only - // this round remembers the comment it left on the PR. - if c.CommandID != 0 && c.CommandID != commandID { - r.SpentCommands = appendSpent(r.SpentCommands, c.CommandID) - } c.CommandID = commandID t := at.UTC() c.CommandedAt = &t @@ -359,13 +367,6 @@ func (r *Round) Fire(commandID int64, at time.Time) error { return r.illegal(PhaseFired) } r.Phase = PhaseFired - // A retry posts a NEW command: the bot answered the previous one (with a - // rate-limit notice, typically) so it can never be adopted again. Remember - // it, or the comment it left behind is one nothing knows about — which is - // why a rate-limited PR collects a column of identical review requests. - if r.CommandID != 0 && r.CommandID != commandID { - r.SpentCommands = appendSpent(r.SpentCommands, r.CommandID) - } r.CommandID = commandID t := at.UTC() r.FiredAt = &t @@ -624,20 +625,30 @@ func (s *State) QueuedRounds(now time.Time) []Round { return out } -// appendSpent records a superseded command, bounded so a PR that retries all day -// cannot grow its round without limit. -func appendSpent(spent []int64, id int64) []int64 { - const maxSpent = 50 - for _, have := range spent { - if have == id { - return spent +// RecordPosted remembers a trigger comment crq WROTE for this round, addressed +// to bot. Call it only where crq actually posted: an adopted command is not +// crq's to record, and later cleanup trusts this list as proof of authorship. +// +// Bounded, so a PR that retries all day cannot grow its round without limit. +func (r *Round) RecordPosted(bot string, id int64, at time.Time) { + const maxPosted = 50 + if id == 0 { + return + } + for _, have := range r.PostedCommands { + if have.ID == id { + return } } - spent = append(spent, id) - if len(spent) > maxSpent { - spent = spent[len(spent)-maxSpent:] + // Copy-on-write, like setCo: Rounds are passed around by value, so appending + // in place could write this entry into a sibling copy's backing array. + posted := make([]PostedCommand, len(r.PostedCommands), len(r.PostedCommands)+1) + copy(posted, r.PostedCommands) + posted = append(posted, PostedCommand{ID: id, Bot: bot, At: at.UTC()}) + if len(posted) > maxPosted { + posted = posted[len(posted)-maxPosted:] } - return spent + r.PostedCommands = posted } // Queue-entry wait reasons. A waiting round is held by exactly one of these diff --git a/llms.txt b/llms.txt index 0c4203c..8f85c98 100644 --- a/llms.txt +++ b/llms.txt @@ -209,8 +209,10 @@ crq removes its own spent `@coderabbitai review` / `@codex review` comments as r crq tidy REPO PR [--dry-run] ``` -Only comments crq posted, only from rounds that have progressed, only after the bot answered, and -never the bots' own comments. +Only comments crq posted (never one it adopted — that comment is someone else's), only from rounds +that have progressed, only after the bot answered, only once the comment predates the current head +(a command crq's own retry replaced is spent regardless, and an unreadable head keeps everything), +and never the bots' own comments. Current feedback without triggering a review: diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index ddb8b3b..2eb7b19 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -181,9 +181,12 @@ This happens by itself under `crq autoreview` (`CRQ_TIDY=0` turns it off), or on crq tidy "$REPO" "$PR" [--dry-run] ``` -It only ever removes comments **crq posted** — candidates come from the command IDs recorded on its -own rounds, never from matching text — and never the bots' own comments, because an auto-generated -reply can be a rate-limit or skipped-review notice that crq reads as evidence. +It only ever removes comments **crq posted** — candidates come from the comments each round recorded +writing, never from matching text, and never one the round merely adopted (a person's request to +review is not crq's to erase). A candidate also has to predate the current head, because a newer +command is one crq would adopt instead of posting again; a request crq's own retry replaced is spent +either way, and an unreadable head keeps everything. Never the bots' own comments, because an +auto-generated reply can be a rate-limit or skipped-review notice that crq reads as evidence. ## Fleet Auto-Review From ac9901447b133f862cd20278ba2da664767e287d Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:13:43 +0200 Subject: [PATCH 04/12] Keep tidy off comments that stopped being trigger comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four things review found in the tidy pass. A recorded comment ID proves crq wrote that comment, not that it is still the one-line command crq wrote. crq posts under the operator's own account, so anyone can edit it into an explanatory note — and the pass would then delete someone's words as a spent request. The body is now checked against the reviewer's trigger command; anything else, including a command whose config changed since, is kept. A refused delete was only logged, so a caller reading an empty `deleted` could not tell "nothing was spent" from "this token may not delete". The pass still tries every ID and now reports the failures in `.failed[]`. Codex can answer a trigger with nothing but its thumbs-up, and that reaction alone completes the round — leaving no review, event or check for answeredAt to find, so the `@codex review` comment was kept for ever. Tidy observes with no round, so observe() never fetches reactions; they are read here instead, once per candidate nothing else has answered. And the README documented neither `crq tidy` nor `CRQ_TIDY`, so an upgrade started deleting comments with no entry in the documented CLI contract. --- README.md | 10 ++ cmd/crq/main.go | 8 +- internal/crq/service_test.go | 5 + internal/crq/tidy.go | 127 +++++++++++++++++++++-- internal/crq/tidy_test.go | 173 +++++++++++++++++++++++++++++++ llms.txt | 5 +- skills/coderabbit-queue/SKILL.md | 4 +- 7 files changed, 315 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index f9d091a..3990d29 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,14 @@ reviewed. The two flags mirror CodeRabbit's own toggles: default = *Automatic + commit it requested a review for, so the same commit is never reviewed twice. One process is the leader at a time (a lease in the shared state), so running the daemon on several machines is safe. +As each round progresses the daemon also **deletes crq's own spent trigger comments** — the +`@coderabbitai review` / `@codex review` one-liners it posted, which otherwise bury the conversation a +human came to read. It removes a comment only when crq wrote it (never one it adopted from a person, +never a bot's own comment, and never one edited into something else), only from a round that has moved +on, only after the bot answered it, and only once it predates the current head. Set `CRQ_TIDY=0` to +keep every comment, or run a pass by hand with `crq tidy ` (`--dry-run` reports what it +would remove). +
Run it persistently (macOS launchd / Linux systemd) @@ -385,6 +393,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 tidy # delete crq's own spent review-trigger comments (--dry-run previews) 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 @@ -460,6 +469,7 @@ Set these in `~/.config/crq/env` (sourced automatically) or as environment varia | `CRQ_EXCLUDE` | _(none)_ | `autoreview` denylist — never these `owner/name` repos (comma-separated) | | `CRQ_AUTOREVIEW_SKIP_AUTHORS` | `dependabot[bot]` | PR authors `autoreview` never enqueues (comma-separated; case and `[bot]` suffix don't matter) — set to empty to auto-review bot PRs too; manual `crq review` is unaffected | | `CRQ_AUTOREVIEW_SKIP_MARKER` | `` | exact PR-body marker that suppresses fleet auto-review; set empty to disable; manual `crq loop` is unaffected | +| `CRQ_TIDY` | `1` | delete crq's own spent review-trigger comments as rounds progress; `0` leaves every comment on the PR (`crq tidy` by hand is unaffected) | | `CRQ_REQUIRED_BOTS` | `coderabbitai[bot]` | bots that must review the head for convergence (crq waits for all of them) | | `CRQ_COBOTS` | `codex,bugbot,macroscope` | co-reviewers crq surfaces and (optionally) triggers; set empty to disable all | | `CRQ_COBOT__REQUIRED` | `0` | make that co-reviewer gate convergence (folds it into `CRQ_REQUIRED_BOTS`); `` ∈ `CODEX`, `BUGBOT`, `MACROSCOPE` | diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 93624a5..9cc7ba4 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -542,9 +542,11 @@ which buries the conversation a human came to read. A comment is removed only when all three hold: - * crq WROTE it — the candidates are the comments each round recorded posting, - not anything matching the text and not one the round adopted. A person's - "@coderabbitai review" is their decision to ask and not crq's to erase. + * crq WROTE it, and it still READS as that one-line command. The candidates + are the comments each round recorded posting, not anything matching the text + and not one the round adopted: a person's "@coderabbitai review" is their + decision to ask and not crq's to erase. crq posts under your own account, so + a recorded comment someone has since edited is their words, and it stays. * the round that asked has PROGRESSED. A live round keeps its command, because that is the comment crq adopts instead of posting another one. * the bot answered it, and it predates the current head. Adoption only ever diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 59aba22..49cba7b 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -30,6 +30,7 @@ type fakeGitHub struct { checkRuns map[string][]ghapi.CheckRun // key: ref (short or full sha) checkRunErrs map[string]error postBodyErrs map[string]error // body → error (selective trigger-post failures) + deleteErrs map[int64]error // comment id → error (GitHub refuses the delete) posted []string deleted []int64 commentID int64 @@ -66,6 +67,7 @@ func newFakeGitHub() *fakeGitHub { reviewComments: map[string][]ghapi.ReviewComment{}, issueReactions: map[string][]ghapi.Reaction{}, reactions: map[int64][]ghapi.Reaction{}, + deleteErrs: map[int64]error{}, } } @@ -216,6 +218,9 @@ func (f *fakeGitHub) ListIssueCommentsPage(_ context.Context, repo string, pr, p func (f *fakeGitHub) DeleteIssueComment(_ context.Context, repo string, id int64) error { f.mu.Lock() defer f.mu.Unlock() + if err := f.deleteErrs[id]; err != nil { + return err + } for key, list := range f.comments { for i, c := range list { if c.ID == id { diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 8c93e47..627c017 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -3,6 +3,7 @@ package crq import ( "context" "errors" + "strings" "time" "github.com/kristofferR/coderabbit-queue/internal/dialect" @@ -17,9 +18,19 @@ type TidyResult struct { // Deleted are the comment IDs removed; Kept explains, per remaining // candidate, why it stayed — so a pass that deletes nothing says why rather // than looking broken. - Deleted []int64 `json:"deleted"` - Kept []string `json:"kept,omitempty"` - DryRun bool `json:"dry_run,omitempty"` + Deleted []int64 `json:"deleted"` + // Failed are the deletions GitHub refused. Without them an empty Deleted + // reads as "nothing was spent" when it may mean "the token may not delete", + // and the caller has no way to tell the two apart. + Failed []TidyFailure `json:"failed,omitempty"` + Kept []string `json:"kept,omitempty"` + DryRun bool `json:"dry_run,omitempty"` +} + +// TidyFailure is one comment a pass decided to remove and could not. +type TidyFailure struct { + ID int64 `json:"id"` + Error string `json:"error"` } // Tidy removes the review-trigger comments crq posted that nothing needs any @@ -29,7 +40,7 @@ type TidyResult struct { // comments and a dozen acknowledgements, which buries the conversation a human // came to read. This deletes crq's own half of that. // -// It is deliberately narrow in two ways. +// It is deliberately narrow in three ways. // // Only comments CRQ POSTED. A human's "@coderabbitai review" is someone's // decision to ask, and not crq's to erase; the candidate list is built from the @@ -37,6 +48,11 @@ type TidyResult struct { // text and not from the round's CommandID — a round records an adopted command // there too, and adoption is exactly how a person's request gets into it. // +// Only comments that STILL READ as a trigger. A recorded ID says crq wrote that +// comment, not that it is still the one-line command crq wrote: anyone with +// write access can edit it into an explanatory note, and deleting that destroys +// someone's words rather than a spent request. +// // Only crq's, not the bot's. An auto-generated reply can be a rate-limit notice // or a skipped-review notice, which crq classifies as evidence and surfaces as // a finding — so deleting bot comments is how this feature would quietly destroy @@ -77,15 +93,26 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // A deleted comment stays on its round for ever, so without this every later // pass would DELETE it again and read the 404 as a fresh removal. - present := map[int64]bool{} + present := map[int64]string{} for _, comment := range obs.comments { - present[comment.ID] = true + present[comment.ID] = comment.Body } + triggers := s.triggerBodies() var commands []engine.CommandComment + edited := 0 for _, cmd := range posted.commands { - if present[cmd.ID] { - commands = append(commands, cmd) + body, onPR := present[cmd.ID] + if !onPR { + continue + } + if !isTriggerBody(triggers, cmd.Bot, body) { + edited++ + continue } + commands = append(commands, cmd) + } + if edited > 0 { + result.Kept = append(result.Kept, "a trigger comment crq posted no longer reads as one; someone edited it") } if len(commands) == 0 { result.Kept = append(result.Kept, "every trigger comment crq posted is already gone") @@ -97,7 +124,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T Live: posted.live, Superseded: posted.superseded, HeadAt: obs.eng.HeadAt, - AnsweredAt: answeredAt(obs, s.cfg), + AnsweredAt: s.answered(ctx, repo, obs, commands, posted.live), } stale := engine.StaleCommands(in) if len(stale) == 0 { @@ -118,7 +145,10 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // comments, and a person may tidy by hand. result.Deleted = append(result.Deleted, id) default: - // One write failing must not abandon the rest. + // One write failing must not abandon the rest — but it is reported, + // not just logged: a caller reading the result is the only one who can + // act on "the token cannot delete these". + result.Failed = append(result.Failed, TidyFailure{ID: id, Error: err.Error()}) if s.log != nil { s.log.Printf("tidy: %s#%d comment %d: %v", repo, pr, id, err) } @@ -185,10 +215,85 @@ func collectPosted(st State, repo string, pr int) postedCommands { return out } +// triggerBodies maps each reviewer (normalized login) to the comment bodies +// that count as its trigger: the configured review command for the primary, the +// command plus registry aliases for each enabled co-reviewer. +func (s *Service) triggerBodies() map[string][]string { + out := s.coCommandBodies() + if command := strings.TrimSpace(s.cfg.ReviewCommand); command != "" { + key := dialect.NormalizeBotName(s.cfg.Bot) + out[key] = append(out[key], command) + } + return out +} + +// isTriggerBody reports whether body is still one of bot's trigger commands. +// +// The author is no help here — crq posts as the operator's own account, so +// crq's comment and that person's are written by the same login. The body is +// the only thing that distinguishes a spent one-line command from a comment +// someone has since edited into something they meant to keep. +// +// Unrecognised keeps the comment, which is also what happens when the trigger +// was reconfigured or its co-reviewer disabled after the comment went out: an +// unevaluable guard is not permission to delete. +func isTriggerBody(triggers map[string][]string, bot, body string) bool { + body = strings.TrimSpace(body) + for _, want := range triggers[dialect.NormalizeBotName(bot)] { + if body == want { + return true + } + } + return false +} + +// answered is answeredAt plus the evidence only a reaction carries. +// +// Codex can answer a trigger with nothing but a thumbs-up on the comment +// itself, and that reaction alone satisfies its gate and completes the round — +// so a round that ended that way leaves no review, event or check for +// answeredAt to find, and its "@codex review" comment would be kept for ever. +// observe() fetches reactions only for a round that has fired, and tidying +// observes with no round at all, so they are read here, once per candidate that +// nothing else has answered. +func (s *Service) answered(ctx context.Context, repo string, obs observation, commands []engine.CommandComment, live map[int64]bool) map[string]time.Time { + out := answeredAt(obs) + for _, cmd := range commands { + if live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) { + continue + } + if at, ok := out[cmd.Bot]; ok && !at.Before(cmd.CreatedAt) { + continue + } + reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) + if err != nil { + // Housekeeping: an unreadable reaction keeps the comment, and the + // next pass tries again. + if s.log != nil { + s.log.Printf("tidy: %s reactions on comment %d: %v", repo, cmd.ID, err) + } + continue + } + for _, reaction := range reactions { + if !isCurrentCodexThumbsUp(reaction, cmd.CreatedAt) { + continue + } + at := reaction.CreatedAt + if at.IsZero() { + at = cmd.CreatedAt + } + if at.After(out[cmd.Bot]) { + out[cmd.Bot] = at + } + } + } + return out +} + // answeredAt is the newest moment each reviewer demonstrably acted on this PR. // A command with nothing after it was never read, and only what has been read is // removed. -func answeredAt(obs observation, cfg Config) map[string]time.Time { +func answeredAt(obs observation) map[string]time.Time { out := map[string]time.Time{} note := func(bot string, at time.Time) { key := dialect.NormalizeBotName(bot) diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 8687d40..b9a4225 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -2,9 +2,11 @@ package crq import ( "context" + "errors" "testing" "time" + "github.com/kristofferR/coderabbit-queue/internal/dialect" ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" ) @@ -227,6 +229,177 @@ func TestTidySkipsCommandsAlreadyGone(t *testing.T) { } } +// A recorded ID proves crq wrote the comment, not that it is still the one-line +// command crq wrote. crq posts as the operator's own account, so anyone who can +// edit that comment can turn it into something they meant to keep — and the body +// is the only thing that still says which it is. +func TestTidyKeepsATriggerCommentEditedIntoSomethingElse(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 15 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: "Asked for a re-review here because the auth change needs a second pair of eyes.", CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", SubmittedAt: now.Add(-90 * time.Minute), State: "COMMENTED"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 0 || len(gh.deleted) != 0 { + t.Fatalf("deleted %v: an edited comment is someone's words, not a spent request", result.Deleted) + } +} + +// A delete GitHub refuses must reach the caller. An empty Deleted otherwise +// reads as "nothing was spent" when it means "the token may not delete". +func TestTidyReportsDeletionFailures(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + gh.deleteErrs[100] = errors.New("resource not accessible by integration") + now := time.Now().UTC() + repo, pr := "o/r", 16 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: cfg.ReviewCommand, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", SubmittedAt: now.Add(-90 * time.Minute), State: "COMMENTED"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 0 { + t.Fatalf("deleted = %v, want nothing: the delete was refused", result.Deleted) + } + if len(result.Failed) != 1 || result.Failed[0].ID != 100 || result.Failed[0].Error == "" { + t.Fatalf("failed = %+v, want the refused comment and why", result.Failed) + } +} + +// Codex can answer a trigger with nothing but its thumbs-up, and that reaction +// alone completes the round — so it is the only evidence that trigger was ever +// read, and without it the comment would be kept for ever. +func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = []CoBotConfig{{Login: dialect.CodexBotLogin, Name: "codex", Command: "@codex review"}} + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 17 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: "@codex review", CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + // The whole answer: a +1 on the trigger, no review, comment or check run. + thumb := ghapi.Reaction{ID: 1, Content: "+1", CreatedAt: now.Add(-100 * time.Minute)} + thumb.User.Login = dialect.CodexBotLogin + gh.reactions[100] = []ghapi.Reaction{thumb} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + // A co-only round, which is how a Codex trigger becomes a round's own + // command: the comment anchors the round and is recorded against Codex. + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.CoOnly = true + r.SetCoCommand(dialect.CodexBotLogin, 100, now.Add(-2*time.Hour)) + r.RecordPosted(dialect.CodexBotLogin, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 1 || result.Deleted[0] != 100 { + t.Fatalf("deleted = %v (kept: %v), want the thumbed-up trigger removed", result.Deleted, result.Kept) + } +} + +// completedRound builds a round that fired (via fire) and then completed, which +// is the "has progressed" precondition every tidy candidate needs. +func completedRound(st *State, repo string, pr int, now time.Time, fire func(*Round) error) error { + r, err := st.NewRound(repo, pr, "aaaaaaaa1", now.Add(-3*time.Hour)) + if err != nil { + return err + } + if err := r.Reserve("t", "h", now.Add(-3*time.Hour)); err != nil { + return err + } + if err := fire(r); err != nil { + return err + } + if err := r.Complete(); err != nil { + return err + } + st.PutRound(*r) + return nil +} + // commitAt is a commit object with a committer date, which is the cutoff // adoption uses and therefore the cutoff tidying respects. func commitAt(at time.Time) ghapi.Commit { diff --git a/llms.txt b/llms.txt index 8f85c98..80b2b5c 100644 --- a/llms.txt +++ b/llms.txt @@ -209,10 +209,11 @@ crq removes its own spent `@coderabbitai review` / `@codex review` comments as r crq tidy REPO PR [--dry-run] ``` -Only comments crq posted (never one it adopted — that comment is someone else's), only from rounds +Only comments crq posted (never one it adopted — that comment is someone else's), only while the +comment still reads as that one-line command (an edited one is someone's words), only from rounds that have progressed, only after the bot answered, only once the comment predates the current head (a command crq's own retry replaced is spent regardless, and an unreadable head keeps everything), -and never the bots' own comments. +and never the bots' own comments. Deletions GitHub refuses are reported in `.failed[]`. Current feedback without triggering a review: diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 2eb7b19..c95a2ff 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -183,7 +183,9 @@ crq tidy "$REPO" "$PR" [--dry-run] It only ever removes comments **crq posted** — candidates come from the comments each round recorded writing, never from matching text, and never one the round merely adopted (a person's request to -review is not crq's to erase). A candidate also has to predate the current head, because a newer +review is not crq's to erase). A candidate must also still read as that one-line command: crq posts +under your own account, so a recorded comment someone has since edited into a note is their words and +it stays. A candidate also has to predate the current head, because a newer command is one crq would adopt instead of posting again; a request crq's own retry replaced is spent either way, and an unreadable head keeps everything. Never the bots' own comments, because an auto-generated reply can be a rate-limit or skipped-review notice that crq reads as evidence. From 3415785a25276460aa860f7ab282bc0dcf1ac6d1 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:44:17 +0200 Subject: [PATCH 05/12] Tidy the triggers a force-push and a PR reaction retired MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the tidy pass read the PR differently from the code it mirrors. Adoption's cutoff is the head commit date raised to the last force-push, because a force-push can point the PR at a commit object older than the commands made for an earlier head. Tidy compared against the commit date alone, so a rebase onto such a commit kept a trigger no round can ever adopt again — for ever, once the PR merged and the closed-PR guard stopped looking. It now asks for the same cutoff, and TidyInput.HeadAt is renamed AdoptableFrom to say which one it is. Codex answers a command in the PR description by reacting to the PR, and observe() accepts that as the round's completion. Tidy only read reactions on each recorded trigger, so a round that ended that way left no evidence it could see and kept the spent "@codex review" comment. Both of observe's sources count now; the PR's reactions are read at most once per pass, and only when a candidate is still unanswered. And a pump that changed nothing reports "waiting", which tidy treated as progress: every poll of a long-running review bought a second full observation of the PR the pump had just observed. Housekeeping does not get to spend the REST quota the queue runs on. The one "waiting" that does move a round parks it with its commands still live, so it has nothing of its own to remove. --- internal/crq/tidy.go | 121 +++++++++++++++++++++------- internal/crq/tidy_test.go | 151 +++++++++++++++++++++++++++++++++++ internal/engine/tidy.go | 21 ++--- internal/engine/tidy_test.go | 32 ++++---- 4 files changed, 270 insertions(+), 55 deletions(-) diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 627c017..891ef6f 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -120,15 +120,15 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T } in := engine.TidyInput{ - Commands: commands, - Live: posted.live, - Superseded: posted.superseded, - HeadAt: obs.eng.HeadAt, - AnsweredAt: s.answered(ctx, repo, obs, commands, posted.live), + Commands: commands, + Live: posted.live, + Superseded: posted.superseded, + AdoptableFrom: s.adoptableFrom(ctx, repo, pr, obs.eng.HeadAt), + AnsweredAt: s.answered(ctx, repo, pr, obs, commands, posted.live), } stale := engine.StaleCommands(in) if len(stale) == 0 { - result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or not yet past the current head") + result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or still adoptable") return result, nil } if dryRun || s.cfg.DryRun { @@ -247,22 +247,51 @@ func isTriggerBody(triggers map[string][]string, bot, body string) bool { return false } +// adoptableFrom is the moment a command stops being adoptable, which is what +// tidying may delete below: the head commit date raised to the last force-push. +// +// Adoption uses the later of the two on purpose — a force-push can point the PR +// at a commit object whose date predates commands made for an earlier head — so +// a tidy pass reading the commit date alone keeps an already-answered trigger +// that no round can ever adopt again, for ever once the PR merges. A zero headAt +// is the unevaluable guard that keeps every comment, and needs no lookup; a +// failed lookup falls back to the commit date, which only ever keeps more. +func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt time.Time) time.Time { + if headAt.IsZero() { + return headAt + } + fp, err := s.headForcePushCutoff(ctx, repo, pr) + if err != nil { + if s.log != nil { + s.log.Printf("tidy: %s#%d force-push lookup: %v", repo, pr, err) + } + return headAt + } + if fp.After(headAt) { + return fp + } + return headAt +} + // answered is answeredAt plus the evidence only a reaction carries. // -// Codex can answer a trigger with nothing but a thumbs-up on the comment -// itself, and that reaction alone satisfies its gate and completes the round — -// so a round that ended that way leaves no review, event or check for -// answeredAt to find, and its "@codex review" comment would be kept for ever. -// observe() fetches reactions only for a round that has fired, and tidying -// observes with no round at all, so they are read here, once per candidate that -// nothing else has answered. -func (s *Service) answered(ctx context.Context, repo string, obs observation, commands []engine.CommandComment, live map[int64]bool) map[string]time.Time { +// Codex can answer a trigger with nothing but a thumbs-up, and that reaction +// alone satisfies its gate and completes the round — so a round that ended that +// way leaves no review, event or check for answeredAt to find, and its +// "@codex review" comment would be kept for ever. observe() fetches reactions +// only for a round that has fired, and tidying observes with no round at all, so +// they are read here. +// +// Both of observe's sources count, because either completes the round: the +// trigger comment itself, read once per candidate nothing else has answered, and +// the PR — Codex answers a command in the PR description by reacting there — +// read at most once per pass, and only when a candidate is still unanswered. +func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, live map[int64]bool) map[string]time.Time { out := answeredAt(obs) + var onPR []ghapi.Reaction + readPR := false for _, cmd := range commands { - if live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) { - continue - } - if at, ok := out[cmd.Bot]; ok && !at.Before(cmd.CreatedAt) { + if live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) || answeredSince(out, cmd) { continue } reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) @@ -274,22 +303,47 @@ func (s *Service) answered(ctx context.Context, repo string, obs observation, co } continue } - for _, reaction := range reactions { - if !isCurrentCodexThumbsUp(reaction, cmd.CreatedAt) { - continue - } - at := reaction.CreatedAt - if at.IsZero() { - at = cmd.CreatedAt - } - if at.After(out[cmd.Bot]) { - out[cmd.Bot] = at + noteThumbsUp(out, cmd, reactions) + if answeredSince(out, cmd) { + continue + } + if !readPR { + readPR = true + if onPR, err = s.gh.ListIssueReactions(ctx, repo, pr); err != nil { + if s.log != nil { + s.log.Printf("tidy: %s#%d reactions: %v", repo, pr, err) + } + onPR = nil } } + noteThumbsUp(out, cmd, onPR) } return out } +// answeredSince reports whether cmd's bot has demonstrably acted since cmd was +// posted, which is the evidence tidying needs before removing it. +func answeredSince(answered map[string]time.Time, cmd engine.CommandComment) bool { + at, ok := answered[cmd.Bot] + return ok && !at.Before(cmd.CreatedAt) +} + +// noteThumbsUp records a Codex +1 among reactions as cmd's bot having acted. +func noteThumbsUp(answered map[string]time.Time, cmd engine.CommandComment, reactions []ghapi.Reaction) { + for _, reaction := range reactions { + if !isCurrentCodexThumbsUp(reaction, cmd.CreatedAt) { + continue + } + at := reaction.CreatedAt + if at.IsZero() { + at = cmd.CreatedAt + } + if at.After(answered[cmd.Bot]) { + answered[cmd.Bot] = at + } + } +} + // answeredAt is the newest moment each reviewer demonstrably acted on this PR. // A command with nothing after it was never read, and only what has been read is // removed. @@ -335,12 +389,21 @@ func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) { return } switch res.Action { - case "cleared", "deduped", "waiting", "requeued", "skipped": + case "cleared", "deduped", "requeued", "skipped": // The round reached a state where its earlier commands are answered and // spent. "cleared" is the ordinary one — the round holding the slot // completed or was acknowledged — and leaving it out meant the common // successful round was never tidied at all. "fired" is not here: that // round is live and owns its command. + // + // Neither is "waiting", which is what a pump that changed NOTHING reports: + // every poll of a long-running review would then re-observe the PR the + // pump just observed — pull, commit, paginated reviews/comments, check + // runs — so housekeeping would spend REST quota the queue needs, over and + // over, for a PR whose comments are exactly as spent as they were last + // poll. The one "waiting" that does move a round (a co-review wait) parks + // it with its commands still live, so it has nothing of its own to remove; + // anything older waits for the pass that clears it. default: return } diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index b9a4225..e8626f1 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -2,6 +2,7 @@ package crq import ( "context" + "encoding/json" "errors" "testing" "time" @@ -380,6 +381,156 @@ func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { } } +// A force-push can point the PR at a commit object OLDER than the command it +// answered, and adoption knows it: its cutoff is the force-push, not the commit +// date. Tidying reads the same rule backwards, so it must use the same cutoff — +// or a command no round can adopt again is kept for ever. +func TestTidyUsesTheForcePushCutoffNotJustTheCommitDate(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + now := time.Now().UTC() + repo, pr := "o/r", 18 + forcePushAt := now.Add(-time.Hour) + gh.graphQL = func(_ string, _ map[string]any, out any) error { + payload := `{"repository":{"pullRequest":{"timelineItems":{"nodes":[{"createdAt":"` + forcePushAt.Format(time.RFC3339) + `"}]}}}}` + return json.Unmarshal([]byte(payload), out) + } + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + // The head was force-pushed to a commit made before the command. + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-3 * time.Hour)) + + c := ghapi.IssueComment{ID: 100, Body: cfg.ReviewCommand, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", State: "COMMENTED", Body: "looks fine", SubmittedAt: now.Add(-90 * time.Minute)} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 1 || result.Deleted[0] != 100 { + t.Fatalf("deleted = %v (kept: %v), want the command the force-push put out of adoption's reach", result.Deleted, result.Kept) + } +} + +// Codex answers a command in the PR description by reacting to the PR itself, +// and observe() accepts that as the round's completion — so tidying has to read +// the same source, or the trigger it completed is kept for ever. +func TestTidyCountsACodexThumbsUpOnThePR(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = []CoBotConfig{{Login: dialect.CodexBotLogin, Name: "codex", Command: "@codex review"}} + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 19 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: "@codex review", CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + // The whole answer, and it is nowhere near the trigger comment. + thumb := ghapi.Reaction{ID: 1, Content: "+1", CreatedAt: now.Add(-100 * time.Minute)} + thumb.User.Login = dialect.CodexBotLogin + gh.issueReactions[fakeKey(repo, pr)] = []ghapi.Reaction{thumb} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.CoOnly = true + r.SetCoCommand(dialect.CodexBotLogin, 100, now.Add(-2*time.Hour)) + r.RecordPosted(dialect.CodexBotLogin, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 1 || result.Deleted[0] != 100 { + t.Fatalf("deleted = %v (kept: %v), want the trigger the PR reaction answered", result.Deleted, result.Kept) + } +} + +// A pump that changed nothing must not buy a second observation of the PR it +// just observed: a long review is polled for hours, and housekeeping that +// re-reads it every poll spends the REST quota the queue runs on. +func TestTidyAfterPumpSkipsAPumpThatChangedNothing(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Tidy = true + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 20 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + c := ghapi.IssueComment{ID: 100, Body: cfg.ReviewCommand, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + svc.tidyAfterPump(ctx, PumpResult{Action: "waiting", Repo: repo, PR: pr, Reason: "review in progress"}) + if reads := gh.reviewPolls(); reads != 0 { + t.Fatalf("a no-progress pump observed the pr %d time(s); the pump had just read it", reads) + } + // The same PR, once something actually moved. + svc.tidyAfterPump(ctx, PumpResult{Action: "cleared", Repo: repo, PR: pr}) + if gh.reviewPolls() == 0 { + t.Fatal("a round that progressed was never tidied") + } +} + // completedRound builds a round that fired (via fire) and then completed, which // is the "has progressed" precondition every tidy candidate needs. func completedRound(st *State, repo string, pr int, now time.Time, fire func(*Round) error) error { diff --git a/internal/engine/tidy.go b/internal/engine/tidy.go index 3827cee..9445c8e 100644 --- a/internal/engine/tidy.go +++ b/internal/engine/tidy.go @@ -20,12 +20,13 @@ type TidyInput struct { // Live are the command IDs a round that has NOT progressed still depends on: // the open round's own command and its co-reviewer triggers. Live map[int64]bool - // HeadAt is when the current head was committed. A command at or after it is - // still adoptable, so removing it would make crq post a duplicate — unless - // the round itself has already replaced it (Superseded). Zero when the head + // AdoptableFrom is the cutoff adoption itself uses: the head commit date, + // raised to the last force-push. A command at or after it is still + // adoptable, so removing it would make crq post a duplicate — unless the + // round itself has already replaced it (Superseded). Zero when the head // commit could not be read, which is not permission to delete: the guard is // simply unevaluable, and an unevaluable guard keeps the comment. - HeadAt time.Time + AdoptableFrom time.Time // Superseded are commands the round explicitly moved past by posting a newer // one. They are exempt from the head check: crq's own record that it has // replaced a command is stronger evidence than any timestamp. @@ -41,11 +42,11 @@ type TidyInput struct { // - it belongs to no live round — the user's rule, and the one that matters: // only rounds that have already progressed; // - the bot acted after it, so it was actually read rather than merely old; -// - it predates the current head, because adoption only ever considers -// commands newer than the head commit. Delete one of those and the next -// pump sees no command, posts another, and buys a second review. An -// unreadable head means the guard cannot be evaluated, and the command -// stays: it may well be adoptable again once the read recovers. +// - it predates the adoption cutoff, because adoption only ever considers +// commands newer than that. Delete one of those and the next pump sees no +// command, posts another, and buys a second review. An unreadable head +// means the guard cannot be evaluated, and the command stays: it may well +// be adoptable again once the read recovers. // // Only a command the round itself replaced (Superseded) skips the head check — // crq's own record that it posted a successor outranks any timestamp. @@ -62,7 +63,7 @@ func StaleCommands(in TidyInput) []int64 { if !ok || answered.Before(cmd.CreatedAt) { continue } - if !in.Superseded[cmd.ID] && (in.HeadAt.IsZero() || !cmd.CreatedAt.Before(in.HeadAt)) { + if !in.Superseded[cmd.ID] && (in.AdoptableFrom.IsZero() || !cmd.CreatedAt.Before(in.AdoptableFrom)) { continue } stale = append(stale, cmd.ID) diff --git a/internal/engine/tidy_test.go b/internal/engine/tidy_test.go index 7085a82..1686605 100644 --- a/internal/engine/tidy_test.go +++ b/internal/engine/tidy_test.go @@ -21,18 +21,18 @@ func TestStaleCommands(t *testing.T) { { name: "answered, old, and no round depends on it", in: TidyInput{ - HeadAt: head, - AnsweredAt: answered, - Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + AdoptableFrom: head, + AnsweredAt: answered, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, }, want: []int64{1}, }, { name: "a round that has not progressed keeps its command", in: TidyInput{ - HeadAt: head, - AnsweredAt: answered, - Live: map[int64]bool{1: true, 2: true}, + AdoptableFrom: head, + AnsweredAt: answered, + Live: map[int64]bool{1: true, 2: true}, Commands: []CommandComment{ {ID: 1, Bot: "coderabbitai", CreatedAt: base}, {ID: 2, Bot: "codex", CreatedAt: base}, @@ -42,17 +42,17 @@ func TestStaleCommands(t *testing.T) { { name: "no evidence this bot ever acted", in: TidyInput{ - HeadAt: head, - AnsweredAt: answered, - Commands: []CommandComment{{ID: 6, Bot: "codex", CreatedAt: base}}, + AdoptableFrom: head, + AnsweredAt: answered, + Commands: []CommandComment{{ID: 6, Bot: "codex", CreatedAt: base}}, }, }, { name: "newer than the head: still adoptable", in: TidyInput{ - HeadAt: head, - AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, - Commands: []CommandComment{{ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, + AdoptableFrom: head, + AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, + Commands: []CommandComment{{ID: 7, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, }, }, { @@ -61,10 +61,10 @@ func TestStaleCommands(t *testing.T) { // newer than the head too, so the adoption guard alone never clears it. name: "a retry the round replaced is spent despite the head", in: TidyInput{ - HeadAt: head, - AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, - Superseded: map[int64]bool{8: true}, - Commands: []CommandComment{{ID: 8, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, + AdoptableFrom: head, + AnsweredAt: map[string]time.Time{"coderabbitai": head.Add(2 * time.Minute)}, + Superseded: map[int64]bool{8: true}, + Commands: []CommandComment{{ID: 8, Bot: "coderabbitai", CreatedAt: head.Add(time.Minute)}}, }, want: []int64{8}, }, From e9a427556d22474c6d1484c30cce0df7458eeb06 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 04:53:50 +0200 Subject: [PATCH 06/12] Read the third place a Codex thumbs-up can be MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Codex-gated round completes on a +1 left on the trigger, on the PR, or on the command the round fired on — observe() reads all three, and for a round whose primary is CodeRabbit that last one is a different comment from the "@codex review" crq posted beside it. Tidying now reads it too, at most once per candidate nothing cheaper has answered, so a round that ended there stops looking like a trigger nobody read. The README and llms.txt said a comment is removed once it predates the current head; it is the cutoff adoption uses, the head commit raised to the last force-push. --- README.md | 3 ++- internal/crq/tidy.go | 41 +++++++++++++++++++++++------ internal/crq/tidy_test.go | 55 +++++++++++++++++++++++++++++++++++++++ llms.txt | 5 ++-- 4 files changed, 93 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 3990d29..a0061ed 100644 --- a/README.md +++ b/README.md @@ -217,7 +217,8 @@ As each round progresses the daemon also **deletes crq's own spent trigger comme `@coderabbitai review` / `@codex review` one-liners it posted, which otherwise bury the conversation a human came to read. It removes a comment only when crq wrote it (never one it adopted from a person, never a bot's own comment, and never one edited into something else), only from a round that has moved -on, only after the bot answered it, and only once it predates the current head. Set `CRQ_TIDY=0` to +on, only after the bot answered it, and only once it is too old to adopt again (older than the head +commit, or than a later force-push). Set `CRQ_TIDY=0` to keep every comment, or run a pass by hand with `crq tidy ` (`--dry-run` reports what it would remove). diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 891ef6f..fcf4515 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -124,7 +124,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T Live: posted.live, Superseded: posted.superseded, AdoptableFrom: s.adoptableFrom(ctx, repo, pr, obs.eng.HeadAt), - AnsweredAt: s.answered(ctx, repo, pr, obs, commands, posted.live), + AnsweredAt: s.answered(ctx, repo, pr, obs, commands, posted), } stale := engine.StaleCommands(in) if len(stale) == 0 { @@ -174,12 +174,16 @@ type postedCommands struct { // superseded are commands their own round replaced with a newer one, so // they can never be adopted again whatever phase that round is in. superseded map[int64]bool + // firedOn maps a posted trigger to the primary command its round fired on, + // when that is another comment. observe() reads a Codex thumbs-up from there + // too, so it is a reaction source tidying needs — never a delete candidate. + firedOn map[int64]int64 } // collectPosted gathers the trigger comments crq posted for repo#pr, from the // open round and from every archived round of the same PR. func collectPosted(st State, repo string, pr int) postedCommands { - out := postedCommands{live: map[int64]bool{}, superseded: map[int64]bool{}} + out := postedCommands{live: map[int64]bool{}, superseded: map[int64]bool{}, firedOn: map[int64]int64{}} collect := func(r Round, progressed bool) { current := map[int64]bool{} if r.CommandID != 0 { @@ -199,6 +203,9 @@ func collectPosted(st State, repo string, pr int) postedCommands { if !current[p.ID] { out.superseded[p.ID] = true } + if r.CommandID != 0 && r.CommandID != p.ID { + out.firedOn[p.ID] = r.CommandID + } out.commands = append(out.commands, engine.CommandComment{ ID: p.ID, Bot: dialect.NormalizeBotName(p.Bot), CreatedAt: p.At.UTC(), }) @@ -282,16 +289,18 @@ func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt // only for a round that has fired, and tidying observes with no round at all, so // they are read here. // -// Both of observe's sources count, because either completes the round: the -// trigger comment itself, read once per candidate nothing else has answered, and -// the PR — Codex answers a command in the PR description by reacting there — -// read at most once per pass, and only when a candidate is still unanswered. -func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, live map[int64]bool) map[string]time.Time { +// All of observe's sources count, because each completes the round: the trigger +// comment itself, read once per candidate nothing else has answered; the PR — +// Codex answers a command in the PR description by reacting there — read at most +// once per pass; and the primary command the candidate's own round fired on, +// which is where observe() looks first. Each is read only while a candidate is +// still unanswered, so the ordinary pass pays for none of them. +func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands) map[string]time.Time { out := answeredAt(obs) var onPR []ghapi.Reaction readPR := false for _, cmd := range commands { - if live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) || answeredSince(out, cmd) { + if posted.live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) || answeredSince(out, cmd) { continue } reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) @@ -317,6 +326,22 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa } } noteThumbsUp(out, cmd, onPR) + if answeredSince(out, cmd) { + continue + } + // Last, the command this trigger's own round fired on. A round gated on + // Codex completes on a thumbs-up left there too, and a round that ended + // that way leaves its trigger looking like nobody ever read it. + if fired := posted.firedOn[cmd.ID]; fired != 0 { + reactions, err := s.gh.ListCommentReactions(ctx, repo, fired) + if err != nil { + if s.log != nil { + s.log.Printf("tidy: %s reactions on comment %d: %v", repo, fired, err) + } + continue + } + noteThumbsUp(out, cmd, reactions) + } } return out } diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index e8626f1..ad8109f 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -485,6 +485,61 @@ func TestTidyCountsACodexThumbsUpOnThePR(t *testing.T) { } } +// The third place observe() reads a Codex thumbs-up from is the command the +// round fired on, which for a Codex-gated round is CodeRabbit's — not the +// "@codex review" crq posted beside it. A round that completed from one of those +// leaves its own trigger looking like nobody ever read it. +func TestTidyCountsACodexThumbsUpOnTheFiredCommand(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = []CoBotConfig{{Login: dialect.CodexBotLogin, Name: "codex", Command: "@codex review"}} + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 21 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + add := func(id int64, body string) { + c := ghapi.IssueComment{ID: id, Body: body, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = append(gh.comments[fakeKey(repo, pr)], c) + } + add(100, cfg.ReviewCommand) // the round's own fire, adopted from a person + add(101, "@codex review") // and the co-reviewer trigger crq posted beside it + // Codex's whole answer, left on the command that fired the round. + thumb := ghapi.Reaction{ID: 1, Content: "+1", CreatedAt: now.Add(-100 * time.Minute)} + thumb.User.Login = dialect.CodexBotLogin + gh.reactions[100] = []ghapi.Reaction{thumb} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.SetCoCommand(dialect.CodexBotLogin, 101, now.Add(-2*time.Hour)) + r.RecordPosted(dialect.CodexBotLogin, 101, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 1 || result.Deleted[0] != 101 { + t.Fatalf("deleted = %v (kept: %v), want the trigger the reaction on the fired command answered", result.Deleted, result.Kept) + } +} + // A pump that changed nothing must not buy a second observation of the PR it // just observed: a long review is polled for hours, and housekeeping that // re-reads it every poll spends the REST quota the queue runs on. diff --git a/llms.txt b/llms.txt index 80b2b5c..9ee4fe9 100644 --- a/llms.txt +++ b/llms.txt @@ -211,8 +211,9 @@ crq tidy REPO PR [--dry-run] Only comments crq posted (never one it adopted — that comment is someone else's), only while the comment still reads as that one-line command (an edited one is someone's words), only from rounds -that have progressed, only after the bot answered, only once the comment predates the current head -(a command crq's own retry replaced is spent regardless, and an unreadable head keeps everything), +that have progressed, only after the bot answered, only once the comment is too old for crq to adopt +again — older than the head commit, or than a later force-push (a command crq's own retry replaced is +spent regardless, and an unreadable head keeps everything), and never the bots' own comments. Deletions GitHub refuses are reported in `.failed[]`. Current feedback without triggering a review: From cc15556832fdc715a63cbc77c53418e5e49a31a3 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 06:04:20 +0200 Subject: [PATCH 07/12] Take the reviewer configuration as a value, like the rest of crq MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This branch was cut before #51, so it still read the reviewer configuration off the Service: observe with no config argument, and coCommandBodies as a Service method. Both are Config methods now, and the branch stopped compiling the moment main moved under it — the tests here passed only because CI builds the branch, not the merge. Tidy takes a Config through the whole path now: which comments count as a trigger depends on who reviews, so that has to be a value the caller supplies. It reads the fleet configuration today, and per-repo reviewers (#57) can substitute one there in a single line without threading anything new through. --- internal/crq/tidy.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index fcf4515..1d70581 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -65,12 +65,17 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T if err != nil { return result, err } + // Which comments count as a trigger depends on who reviews, so the whole + // path takes a configuration value rather than reading the Service's. That + // is what lets per-repo reviewers substitute one here later without + // threading anything new through. + cfg := s.cfg if len(collectPosted(st, repo, pr).commands) == 0 { result.Kept = append(result.Kept, "no round on this pr posted a trigger comment") return result, nil } - obs, err := s.observe(ctx, repo, pr, nil, s.clock()) + obs, err := s.observe(ctx, cfg, repo, pr, nil, s.clock()) if err != nil { return result, err } @@ -97,7 +102,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T for _, comment := range obs.comments { present[comment.ID] = comment.Body } - triggers := s.triggerBodies() + triggers := cfg.triggerBodies() var commands []engine.CommandComment edited := 0 for _, cmd := range posted.commands { @@ -225,10 +230,10 @@ func collectPosted(st State, repo string, pr int) postedCommands { // triggerBodies maps each reviewer (normalized login) to the comment bodies // that count as its trigger: the configured review command for the primary, the // command plus registry aliases for each enabled co-reviewer. -func (s *Service) triggerBodies() map[string][]string { - out := s.coCommandBodies() - if command := strings.TrimSpace(s.cfg.ReviewCommand); command != "" { - key := dialect.NormalizeBotName(s.cfg.Bot) +func (c Config) triggerBodies() map[string][]string { + out := c.coCommandBodies() + if command := strings.TrimSpace(c.ReviewCommand); command != "" { + key := dialect.NormalizeBotName(c.Bot) out[key] = append(out[key], command) } return out From b14786357d30c128f1469370f023865fc02504fc Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:06:03 +0200 Subject: [PATCH 08/12] Preserve review evidence when tidying commands --- internal/crq/codex_replay_test.go | 4 +- internal/crq/feedback.go | 2 +- internal/crq/feedback_test.go | 86 ++++++++++++++++++- internal/crq/observe.go | 33 +++++++- internal/crq/service.go | 13 +-- internal/crq/service_test.go | 8 ++ internal/crq/tidy.go | 2 +- internal/crq/tidy_test.go | 136 ++++++++++++++++++++++++++++++ 8 files changed, 271 insertions(+), 13 deletions(-) diff --git a/internal/crq/codex_replay_test.go b/internal/crq/codex_replay_test.go index df8e131..8a92501 100644 --- a/internal/crq/codex_replay_test.go +++ b/internal/crq/codex_replay_test.go @@ -382,7 +382,7 @@ func TestObserveScopesShellFilterToCodeRabbit(t *testing.T) { f.gh.reviews[key] = []ghapi.Review{crShell, codexReview} f.gh.mu.Unlock() - obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, nil, f.clk.now()) + obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, nil, nil, f.clk.now()) if err != nil { t.Fatal(err) } @@ -530,7 +530,7 @@ func TestSelfHealCodexClaimPreventsDoublePost(t *testing.T) { } st, _, _ := f.store.Load(f.ctx) round := st.Round(repo, pr) - obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, round, f.clk.now()) + obs, err := f.svc.observe(f.ctx, f.svc.cfg, repo, pr, round, nil, f.clk.now()) if err != nil { t.Fatal(err) } diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index c2722bf..58baed3 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -88,7 +88,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe // findings from the raw reviews/comments and derives convergence from // engine.Completion over the same snapshot — no second fetch path, and the // "is head reviewed?" rules live only in the engine. - obs, err := s.observe(ctx, s.cfg, repo, pr, round, now) + obs, err := s.observe(ctx, s.cfg, repo, pr, round, collectPosted(st, repo, pr).commands, now) if err != nil { return FeedbackReport{}, err } diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index b28682c..f9d0357 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -348,6 +348,83 @@ func TestFeedbackRejectsCompletionReplyFromEarlierRound(t *testing.T) { } } +func TestFeedbackPairsRepliesWithTidiedCommandHistory(t *testing.T) { + cfg := firingConfig() + sha := "abcdef1234567890" + head := sha[:9] + firedAt := time.Now().UTC().Add(-10 * time.Minute) + oldCommandAt := firedAt.Add(-5 * time.Minute) + completion := "\n✅ Action performed\n\nReview finished." + mk := func(id int64, login, body string, at time.Time) ghapi.IssueComment { + ic := ghapi.IssueComment{ID: id, Body: body, CreatedAt: at, UpdatedAt: at} + ic.User.Login = login + return ic + } + + gh := newFakeGitHub() + pull := ghapi.Pull{State: "open"} + pull.Head.SHA = sha + gh.pulls[fakeKey("o/repo", 3)] = pull + // Tidy removed command 1. Its delayed completion lands after command 2, + // and must still pair with the remembered command 1. + gh.comments[fakeKey("o/repo", 3)] = []ghapi.IssueComment{ + mk(2, "kristofferR", cfg.ReviewCommand, firedAt), + mk(3, cfg.Bot, completion, firedAt.Add(30*time.Second)), + } + prior := ghapi.Review{ + ID: 9, + CommitID: "0123456fedcba", + State: "COMMENTED", + SubmittedAt: oldCommandAt.Add(-time.Hour), + Body: "**Actionable comments posted: 2**", + } + prior.User.Login = cfg.Bot + gh.reviews[fakeKey("o/repo", 3)] = []ghapi.Review{prior} + + store := NewMemoryStore(cfg) + if _, err := store.Update(context.Background(), func(st *State) error { + old, err := st.NewRound("o/repo", 3, "0123456fe", oldCommandAt) + if err != nil { + return err + } + if err := old.Reserve("old", "host", oldCommandAt); err != nil { + return err + } + if err := old.Fire(1, oldCommandAt); err != nil { + return err + } + old.RecordPosted(cfg.Bot, 1, oldCommandAt) + st.PutRound(*old) + + current, err := st.Supersede("o/repo", 3, head, firedAt) + if err != nil { + return err + } + if err := current.Reserve("current", "host", firedAt); err != nil { + return err + } + if err := current.Fire(2, firedAt); err != nil { + return err + } + current.RecordPosted(cfg.Bot, 2, firedAt) + if err := current.Acknowledge(); err != nil { + return err + } + st.PutRound(*current) + return nil + }); err != nil { + t.Fatal(err) + } + + report, err := NewService(cfg, gh, store, nil).Feedback(context.Background(), "o/repo", 3) + if err != nil { + t.Fatal(err) + } + if report.ReviewedBy[cfg.Bot] || report.Converged { + t.Fatalf("the old command's delayed completion converged the current round: %#v", report) + } +} + func TestFeedbackSkipsReviewAnsweredCommandsWhenPairingCompletionReplies(t *testing.T) { cfg := Config{ Bot: "coderabbitai[bot]", @@ -1022,6 +1099,13 @@ func TestFeedbackUsesNoActionCompletionAfterCodexThumbsUp(t *testing.T) { pull.State = "open" pull.Head.SHA = "abcdef1234567890" gh.pulls[fakeKey("o/repo", 1)] = pull + command := ghapi.IssueComment{ + ID: 99, + Body: "@coderabbitai review", + CreatedAt: started, + UpdatedAt: started, + } + command.User.Login = "kristofferR" comment := ghapi.IssueComment{ ID: 1, Body: "No actionable comments were generated in the recent review. 🎉", @@ -1029,7 +1113,7 @@ func TestFeedbackUsesNoActionCompletionAfterCodexThumbsUp(t *testing.T) { UpdatedAt: started.Add(time.Minute), } comment.User.Login = "coderabbitai[bot]" - gh.comments[fakeKey("o/repo", 1)] = []ghapi.IssueComment{comment} + gh.comments[fakeKey("o/repo", 1)] = []ghapi.IssueComment{command, comment} thumb := ghapi.Reaction{Content: "+1", CreatedAt: started.Add(2 * time.Minute)} thumb.User.Login = "chatgpt-codex-connector[bot]" gh.reactions[99] = []ghapi.Reaction{thumb} diff --git a/internal/crq/observe.go b/internal/crq/observe.go index 409cda3..866f27d 100644 --- a/internal/crq/observe.go +++ b/internal/crq/observe.go @@ -30,8 +30,9 @@ type observation struct { // // round anchors the round-relative facts: reactions target its fired command, // the adoption cutoff is its LastAttemptAt, and reactions/thumbs-up are fetched -// only for a round that has fired. -func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, round *Round, now time.Time) (observation, error) { +// only for a round that has fired. posted restores the chronological position +// of crq-authored trigger comments that Tidy has removed from GitHub. +func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, round *Round, posted []engine.CommandComment, now time.Time) (observation, error) { pull, err := s.gh.GetPull(ctx, repo, pr) if err != nil { return observation{}, err @@ -90,9 +91,31 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, ReviewCommand: cfg.ReviewCommand, CoReviewers: cfg.classifierCoReviewers(), } + presentComments := make(map[int64]bool, len(comments)) for _, c := range comments { + presentComments[c.ID] = true o.eng.Events = append(o.eng.Events, classifier.Classify(c.User.Login, c.Body, c.ID, c.CreatedAt, c.UpdatedAt)) } + // Tidy removes spent trigger comments, but command/reply pairing still needs + // their place in the chronological FIFO. PostedCommands is the persisted + // proof of the comments crq wrote, so restore only commands no longer on the + // PR; a live comment remains classified from its actual body above. + for _, cmd := range posted { + if presentComments[cmd.ID] { + continue + } + ev := dialect.BotEvent{ + Kind: dialect.EvCoCommand, + CommentID: cmd.ID, + CreatedAt: cmd.CreatedAt, + For: dialect.NormalizeBotName(cmd.Bot), + } + if dialect.NormalizeBotName(cmd.Bot) == dialect.NormalizeBotName(cfg.Bot) { + ev.Kind = dialect.EvCommand + ev.For = "" + } + o.eng.Events = append(o.eng.Events, ev) + } // Check runs are the only place a silent-clean Bugbot round exists, and they // also suppress selfheal triggers and inform pre-fire decisions — so they are @@ -125,7 +148,11 @@ func (s *Service) observe(ctx context.Context, cfg Config, repo string, pr int, // Reactions and Codex thumbs-up only matter for a round that has fired. if round != nil && round.FiredAt != nil { cutoff := round.FiredAt.UTC() - if round.CommandID != 0 { + // A completed round remains as the reviewed-head marker after Tidy + // deletes its trigger, and an adopted command can be removed by its + // author too. Reactions on a missing issue comment return 404, and no + // reaction can appear after the comment has gone, so skip that read. + if round.CommandID != 0 && presentComments[round.CommandID] { reactions, err := s.gh.ListCommentReactions(ctx, repo, round.CommandID) if err != nil { return observation{}, err diff --git a/internal/crq/service.go b/internal/crq/service.go index 59ddc35..c0f1b86 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -244,6 +244,9 @@ func (s *Service) Pump(ctx context.Context) (PumpResult, error) { if free, handled, err := s.sweepQuotaFree(ctx, st, s.clock(), slot.Repo, slot.PR); err != nil { return PumpResult{}, err } else if handled { + // The quota-free result is the one Pump exposes to its caller, so + // preserve the cleanup hook for the slot result it replaces. + s.tidyAfterPump(ctx, res) return free, nil } return res, nil @@ -298,7 +301,7 @@ func (s *Service) Pump(ctx context.Context) (PumpResult, error) { if next == nil { return PumpResult{Action: "idle"}, nil } - obs, err := s.observe(ctx, s.cfg, next.Repo, next.PR, next, now) + obs, err := s.observe(ctx, s.cfg, next.Repo, next.PR, next, collectPosted(st, next.Repo, next.PR).commands, now) if err != nil { return PumpResult{}, err } @@ -363,7 +366,7 @@ func (s *Service) sweepQuotaFree(ctx context.Context, st State, now time.Time, s // left them behind the account block for hours. The budget above bounds // the cost instead. scanned++ - obs, err := s.observe(ctx, s.cfg, round.Repo, round.PR, &round, now) + obs, err := s.observe(ctx, s.cfg, round.Repo, round.PR, &round, collectPosted(st, round.Repo, round.PR).commands, now) if err != nil { continue } @@ -399,7 +402,7 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu if round == nil || !round.FireEligible(now) { return PumpResult{}, false, nil } - obs, err := s.observe(ctx, s.cfg, repo, pr, round, now) + obs, err := s.observe(ctx, s.cfg, repo, pr, round, collectPosted(st, repo, pr).commands, now) if err != nil { return PumpResult{}, false, err } @@ -487,7 +490,7 @@ func (s *Service) progressSlotRound(ctx context.Context, slot Round) (PumpResult if err != nil { return PumpResult{}, err } - obs, err := s.observe(ctx, s.cfg, slot.Repo, slot.PR, &slot, now) + obs, err := s.observe(ctx, s.cfg, slot.Repo, slot.PR, &slot, collectPosted(st, slot.Repo, slot.PR).commands, now) if err != nil { return PumpResult{}, err } @@ -626,7 +629,7 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( if target == nil { return st, nil } - obs, err := s.observe(ctx, s.cfg, target.Repo, target.PR, target, now) + obs, err := s.observe(ctx, s.cfg, target.Repo, target.PR, target, collectPosted(st, target.Repo, target.PR).commands, now) if err != nil { if s.log != nil { s.log.Printf("warning: reviewing-round sweep for %s#%d failed: %v", target.Repo, target.PR, err) diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 8d4b09f..eea4b53 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -27,6 +27,7 @@ type fakeGitHub struct { reviewComments map[string][]ghapi.ReviewComment issueReactions map[string][]ghapi.Reaction reactions map[int64][]ghapi.Reaction + reactionErrs map[int64]error checkRuns map[string][]ghapi.CheckRun // key: ref (short or full sha) checkRunErrs map[string]error postBodyErrs map[string]error // body → error (selective trigger-post failures) @@ -67,6 +68,7 @@ func newFakeGitHub() *fakeGitHub { reviewComments: map[string][]ghapi.ReviewComment{}, issueReactions: map[string][]ghapi.Reaction{}, reactions: map[int64][]ghapi.Reaction{}, + reactionErrs: map[int64]error{}, deleteErrs: map[int64]error{}, } } @@ -169,6 +171,9 @@ func (f *fakeGitHub) ListIssueReactions(_ context.Context, repo string, pr int) func (f *fakeGitHub) ListCommentReactions(_ context.Context, _ string, id int64) ([]ghapi.Reaction, error) { f.mu.Lock() defer f.mu.Unlock() + if err := f.reactionErrs[id]; err != nil { + return nil, err + } return append([]ghapi.Reaction(nil), f.reactions[id]...), nil } @@ -1265,6 +1270,9 @@ func TestPumpKeepsRoundReviewingWhenBotOnlyReacted(t *testing.T) { pull.State = "open" pull.Head.SHA = "abcdef1234567890" gh.pulls[fakeKey("owner/repo", 12)] = pull + command := ghapi.IssueComment{ID: 5, Body: cfg.ReviewCommand, CreatedAt: firedAt, UpdatedAt: firedAt} + command.User.Login = "kristofferR" + gh.comments[fakeKey("owner/repo", 12)] = []ghapi.IssueComment{command} reaction := ghapi.Reaction{} reaction.User.Login = cfg.Bot gh.reactions[5] = []ghapi.Reaction{reaction} diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 1d70581..d417081 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -75,7 +75,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, nil } - obs, err := s.observe(ctx, cfg, repo, pr, nil, s.clock()) + obs, err := s.observe(ctx, cfg, repo, pr, nil, collectPosted(st, repo, pr).commands, s.clock()) if err != nil { return result, err } diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index ad8109f..820986d 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -586,6 +586,142 @@ func TestTidyAfterPumpSkipsAPumpThatChangedNothing(t *testing.T) { } } +// Pump can progress the slot holder and then return a different PR's +// quota-free result. The caller can only tidy the returned result, so Pump must +// tidy the progressed slot before replacing it. +func TestPumpTidiesProgressedSlotBeforeReturningQuotaFreeResult(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Tidy = true + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + + holderRepo, holderPR := "o/holder", 21 + holderSHA := "1111111111111111" + holderPull := ghapi.Pull{State: "open"} + holderPull.Head.SHA = holderSHA + gh.pulls[fakeKey(holderRepo, holderPR)] = holderPull + gh.commits[holderSHA] = commitAt(now.Add(-10 * time.Minute)) + holderCommand := ghapi.IssueComment{ + ID: 100, + Body: cfg.ReviewCommand, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now.Add(-2 * time.Hour), + } + holderCommand.User.Login = "kristofferR" + gh.comments[fakeKey(holderRepo, holderPR)] = []ghapi.IssueComment{holderCommand} + holderReview := ghapi.Review{ + ID: 101, + CommitID: holderSHA, + State: "COMMENTED", + SubmittedAt: now.Add(-time.Minute), + Body: "review complete", + } + holderReview.User.Login = cfg.Bot + gh.reviews[fakeKey(holderRepo, holderPR)] = []ghapi.Review{holderReview} + + backRepo, backPR := "o/back", 22 + backSHA := "2222222222222222" + backPull := ghapi.Pull{State: "open"} + backPull.Head.SHA = backSHA + gh.pulls[fakeKey(backRepo, backPR)] = backPull + backReview := ghapi.Review{ + ID: 201, + CommitID: backSHA, + State: "COMMENTED", + SubmittedAt: now.Add(-time.Minute), + Body: "already reviewed", + } + backReview.User.Login = cfg.Bot + gh.reviews[fakeKey(backRepo, backPR)] = []ghapi.Review{backReview} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + svc.now = func() time.Time { return now } + seedRound(t, store, cfg, holderRepo, holderPR, holderSHA[:9], PhaseFired, now.Add(-2*time.Hour), holderCommand.ID) + if _, err := store.Update(ctx, func(st *State) error { + r := st.Round(holderRepo, holderPR) + r.RecordPosted(cfg.Bot, holderCommand.ID, holderCommand.CreatedAt) + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + if _, err := svc.Enqueue(ctx, backRepo, backPR); err != nil { + t.Fatal(err) + } + + result, err := svc.Pump(ctx) + if err != nil { + t.Fatal(err) + } + if result.Repo != backRepo || result.PR != backPR { + t.Fatalf("the quota-free result should replace the progressed slot result, got %#v", result) + } + if len(gh.deleted) != 1 || gh.deleted[0] != holderCommand.ID { + t.Fatalf("the replaced slot result was not tidied, deleted=%v", gh.deleted) + } +} + +// A completed round remains the dedupe marker after its trigger is tidied. +// Feedback must not ask GitHub for reactions on that now-deleted comment. +func TestFeedbackSkipsReactionsForTidiedCommand(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 23 + head := "bbbbbbbb22222222" + + pull := ghapi.Pull{State: "open"} + pull.Head.SHA = head + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits[head] = commitAt(now.Add(-10 * time.Minute)) + command := ghapi.IssueComment{ + ID: 100, + Body: cfg.ReviewCommand, + CreatedAt: now.Add(-2 * time.Hour), + UpdatedAt: now.Add(-2 * time.Hour), + } + command.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{command} + review := ghapi.Review{ + ID: 101, + CommitID: head, + State: "COMMENTED", + SubmittedAt: now.Add(-time.Minute), + Body: "review complete", + } + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(command.ID, command.CreatedAt); err != nil { + return err + } + r.RecordPosted(cfg.Bot, command.ID, command.CreatedAt) + return nil + }) + }); err != nil { + t.Fatal(err) + } + if result, err := svc.Tidy(ctx, repo, pr, false); err != nil { + t.Fatal(err) + } else if len(result.Deleted) != 1 { + t.Fatalf("tidy result = %#v, want the spent trigger deleted", result) + } + gh.reactionErrs[command.ID] = ghapi.ErrNotFound + + if _, err := svc.Feedback(ctx, repo, pr); err != nil { + t.Fatalf("feedback read reactions from the deleted command: %v", err) + } +} + // completedRound builds a round that fired (via fire) and then completed, which // is the "has progressed" precondition every tidy candidate needs. func completedRound(st *State, repo string, pr int, now time.Time, fire func(*Round) error) error { From fbac5bf6fab3ac76e8f8968700ac9003bd8ea99a Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:37:26 +0200 Subject: [PATCH 09/12] Harden tidy command cleanup --- AGENTS.md | 3 +- internal/crq/feedback_test.go | 10 +++ internal/crq/service.go | 1 + internal/crq/service_test.go | 19 ++++ internal/crq/state.go | 20 +++-- internal/crq/tidy.go | 159 +++++++++++++++++++++++++++++++--- internal/crq/tidy_test.go | 141 ++++++++++++++++++++++++++++++ internal/gh/github.go | 6 ++ internal/state/posted_test.go | 22 +++++ internal/state/state.go | 61 +++++++++++++ 10 files changed, 421 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 399e946..83f0423 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -28,7 +28,8 @@ Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state them to a bot is dialect's `ClassifyCheckRun`, never gh's. - `internal/state/` — persisted schema v3: one `Round` per PR, one global `FireSlot`, the CodeRabbit `AccountQuota`, an `Archive` ring. Round transition - methods, the CAS store, and dashboard rendering. `Round.CoBots` holds per- + methods, durable tombstones for tidied trigger comments, the CAS store, and + dashboard rendering. `Round.CoBots` holds per- co-reviewer trigger bookkeeping; Codex's entry is **dual-written** to the legacy `Codex*` round fields because the fleet shares one state ref across binary versions (`Normalize` folds them back on load). `Round` and `State` also diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index f9d0357..fe17e6d 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -411,6 +411,16 @@ func TestFeedbackPairsRepliesWithTidiedCommandHistory(t *testing.T) { return err } st.PutRound(*current) + // Tidy persisted the old command outside Archive before removing it. + // Enough unrelated rounds then finish to evict this PR's old round from + // the fleet-wide ring before its delayed completion arrives. + st.RecordTidied("o/repo", 3, PostedCommand{ID: 1, Bot: cfg.Bot, At: oldCommandAt}) + for i := 0; i < ArchiveMax; i++ { + st.Archive = append(st.Archive, Round{ + Repo: "o/other", PR: 100 + i, Head: "999999999", + Phase: PhaseCompleted, EnqueuedAt: oldCommandAt, + }) + } return nil }); err != nil { t.Fatal(err) diff --git a/internal/crq/service.go b/internal/crq/service.go index c0f1b86..244d0f8 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -27,6 +27,7 @@ type GitHubAPI interface { GetCommit(context.Context, string, string) (ghapi.Commit, error) ListReviews(context.Context, string, int) ([]ghapi.Review, error) ListIssueComments(context.Context, string, int) ([]ghapi.IssueComment, error) + GetIssueComment(context.Context, string, int64) (ghapi.IssueComment, error) ListIssueCommentsPage(context.Context, string, int, int, int) ([]ghapi.IssueComment, error) ListReviewComments(context.Context, string, int) ([]ghapi.ReviewComment, error) ListIssueReactions(context.Context, string, int) ([]ghapi.Reaction, error) diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index eea4b53..b566ef4 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -28,6 +28,7 @@ type fakeGitHub struct { issueReactions map[string][]ghapi.Reaction reactions map[int64][]ghapi.Reaction reactionErrs map[int64]error + reactionReads []int64 checkRuns map[string][]ghapi.CheckRun // key: ref (short or full sha) checkRunErrs map[string]error postBodyErrs map[string]error // body → error (selective trigger-post failures) @@ -45,6 +46,7 @@ type fakeGitHub struct { refReads int reviewReads int searchPRs []ghapi.SearchPR + getComment func(repo string, id int64) (ghapi.IssueComment, error) // now, when set, timestamps posted comments off the same injected clock the // service uses, so a fire's recorded FiredAt tracks the fake wall clock the // replay suite advances. nil falls back to real time (all existing tests). @@ -156,6 +158,22 @@ func (f *fakeGitHub) ListIssueComments(_ context.Context, repo string, pr int) ( return append([]ghapi.IssueComment(nil), f.comments[fakeKey(repo, pr)]...), nil } +func (f *fakeGitHub) GetIssueComment(_ context.Context, repo string, id int64) (ghapi.IssueComment, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.getComment != nil { + return f.getComment(repo, id) + } + for _, comments := range f.comments { + for _, comment := range comments { + if comment.ID == id { + return comment, nil + } + } + } + return ghapi.IssueComment{}, ghapi.ErrNotFound +} + func (f *fakeGitHub) ListReviewComments(_ context.Context, repo string, pr int) ([]ghapi.ReviewComment, error) { f.mu.Lock() defer f.mu.Unlock() @@ -171,6 +189,7 @@ func (f *fakeGitHub) ListIssueReactions(_ context.Context, repo string, pr int) func (f *fakeGitHub) ListCommentReactions(_ context.Context, _ string, id int64) ([]ghapi.Reaction, error) { f.mu.Lock() defer f.mu.Unlock() + f.reactionReads = append(f.reactionReads, id) if err := f.reactionErrs[id]; err != nil { return nil, err } diff --git a/internal/crq/state.go b/internal/crq/state.go index a9da1d2..482a75b 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -14,18 +14,20 @@ import ( // the package qualifier, and without colliding with the many `state`/`st` // variable names in this package. type ( - State = crqstate.State - Round = crqstate.Round - Phase = crqstate.Phase - FireSlot = crqstate.FireSlot - AccountQuota = crqstate.AccountQuota - LeaderLease = crqstate.LeaderLease - Revision = crqstate.Revision - StateStore = crqstate.StateStore - StoreConfig = crqstate.StoreConfig + State = crqstate.State + Round = crqstate.Round + Phase = crqstate.Phase + FireSlot = crqstate.FireSlot + AccountQuota = crqstate.AccountQuota + LeaderLease = crqstate.LeaderLease + PostedCommand = crqstate.PostedCommand + Revision = crqstate.Revision + StateStore = crqstate.StateStore + StoreConfig = crqstate.StoreConfig ) const ( + ArchiveMax = crqstate.ArchiveMax PhaseQueued = crqstate.PhaseQueued PhaseReserved = crqstate.PhaseReserved PhaseFired = crqstate.PhaseFired diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index d417081..5a7c05d 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -3,6 +3,7 @@ package crq import ( "context" "errors" + "sort" "strings" "time" @@ -98,19 +99,19 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // A deleted comment stays on its round for ever, so without this every later // pass would DELETE it again and read the 404 as a fresh removal. - present := map[int64]string{} + present := map[int64]ghapi.IssueComment{} for _, comment := range obs.comments { - present[comment.ID] = comment.Body + present[comment.ID] = comment } triggers := cfg.triggerBodies() var commands []engine.CommandComment edited := 0 for _, cmd := range posted.commands { - body, onPR := present[cmd.ID] + comment, onPR := present[cmd.ID] if !onPR { continue } - if !isTriggerBody(triggers, cmd.Bot, body) { + if !isTriggerBody(triggers, cmd.Bot, comment.Body) { edited++ continue } @@ -129,10 +130,16 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T Live: posted.live, Superseded: posted.superseded, AdoptableFrom: s.adoptableFrom(ctx, repo, pr, obs.eng.HeadAt), - AnsweredAt: s.answered(ctx, repo, pr, obs, commands, posted), } + cursor := st.TidyReactionCursors[QueueKey(repo, pr)] + in.AnsweredAt, cursor = s.answered(ctx, repo, pr, obs, commands, posted, cursor) stale := engine.StaleCommands(in) if len(stale) == 0 { + if !dryRun && !s.cfg.DryRun { + if err := s.recordTidyState(ctx, repo, pr, cursor, nil); err != nil { + return result, err + } + } result.Kept = append(result.Kept, "every trigger comment is still live, unanswered, or still adoptable") return result, nil } @@ -141,8 +148,47 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T result.Deleted = stale return result, nil } + + byID := make(map[int64]PostedCommand, len(commands)) + for _, command := range commands { + byID[command.ID] = PostedCommand{ID: command.ID, Bot: command.Bot, At: command.CreatedAt} + } + tombstones := make([]PostedCommand, 0, len(stale)) + for _, id := range stale { + tombstones = append(tombstones, byID[id]) + } + // Persist the deleted command's FIFO position before the destructive write. + // If this process dies after the state write, the still-present GitHub + // comment suppresses the synthetic event; if it dies after the delete, the + // tombstone already exists. + if err := s.recordTidyState(ctx, repo, pr, cursor, tombstones); err != nil { + return result, err + } + + var forget []int64 for _, id := range stale { - err := s.gh.DeleteIssueComment(ctx, repo, id) + snapshot := present[id] + latest, err := s.gh.GetIssueComment(ctx, repo, id) + switch { + case errors.Is(err, ghapi.ErrNotFound): + // It disappeared after the observation. Keep its tombstone: a + // delayed reply still needs the command's FIFO position. + result.Deleted = append(result.Deleted, id) + continue + case err != nil: + forget = append(forget, id) + result.Failed = append(result.Failed, TidyFailure{ID: id, Error: err.Error()}) + continue + case latest.Body != snapshot.Body || !latest.UpdatedAt.Equal(snapshot.UpdatedAt): + // Revalidate immediately before DELETE. An operator may have turned + // the one-line trigger into a note while the observation, force-push + // lookup, and reaction reads were in flight. + forget = append(forget, id) + result.Kept = append(result.Kept, "a trigger comment changed while tidy was running; its edited words were preserved") + continue + } + + err = s.gh.DeleteIssueComment(ctx, repo, id) switch { case err == nil, errors.Is(err, ghapi.ErrNotFound): // Already gone is the outcome we wanted. A recorded command can @@ -150,6 +196,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // comments, and a person may tidy by hand. result.Deleted = append(result.Deleted, id) default: + forget = append(forget, id) // One write failing must not abandon the rest — but it is reported, // not just logged: a caller reading the result is the only one who can // act on "the token cannot delete these". @@ -159,6 +206,14 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T } } } + if len(forget) > 0 { + if _, err := s.store.Update(ctx, func(st *State) error { + st.ForgetTidied(repo, pr, forget...) + return nil + }); err != nil { + return result, err + } + } if s.log != nil && len(result.Deleted) > 0 { s.log.Printf("tidy: %s#%d removed %d spent trigger comment(s)", repo, pr, len(result.Deleted)) } @@ -189,6 +244,14 @@ type postedCommands struct { // open round and from every archived round of the same PR. func collectPosted(st State, repo string, pr int) postedCommands { out := postedCommands{live: map[int64]bool{}, superseded: map[int64]bool{}, firedOn: map[int64]int64{}} + seen := map[int64]bool{} + appendCommand := func(command engine.CommandComment) { + if command.ID == 0 || seen[command.ID] { + return + } + seen[command.ID] = true + out.commands = append(out.commands, command) + } collect := func(r Round, progressed bool) { current := map[int64]bool{} if r.CommandID != 0 { @@ -211,7 +274,7 @@ func collectPosted(st State, repo string, pr int) postedCommands { if r.CommandID != 0 && r.CommandID != p.ID { out.firedOn[p.ID] = r.CommandID } - out.commands = append(out.commands, engine.CommandComment{ + appendCommand(engine.CommandComment{ ID: p.ID, Bot: dialect.NormalizeBotName(p.Bot), CreatedAt: p.At.UTC(), }) } @@ -224,6 +287,17 @@ func collectPosted(st State, repo string, pr int) postedCommands { collect(archived, true) } } + for _, command := range st.TidiedCommands[QueueKey(repo, pr)] { + appendCommand(engine.CommandComment{ + ID: command.ID, Bot: dialect.NormalizeBotName(command.Bot), CreatedAt: command.At.UTC(), + }) + } + sort.Slice(out.commands, func(i, j int) bool { + if !out.commands[i].CreatedAt.Equal(out.commands[j].CreatedAt) { + return out.commands[i].CreatedAt.Before(out.commands[j].CreatedAt) + } + return out.commands[i].ID < out.commands[j].ID + }) return out } @@ -300,14 +374,51 @@ func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt // once per pass; and the primary command the candidate's own round fired on, // which is where observe() looks first. Each is read only while a candidate is // still unanswered, so the ordinary pass pays for none of them. -func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands) map[string]time.Time { +const maxTidyReactionCandidates = 4 + +func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands, cursor int64) (map[string]time.Time, int64) { out := answeredAt(obs) - var onPR []ghapi.Reaction - readPR := false + var candidates []engine.CommandComment for _, cmd := range commands { if posted.live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) || answeredSince(out, cmd) { continue } + candidates = append(candidates, cmd) + } + if len(candidates) == 0 { + return out, cursor + } + start := 0 + foundCursor := false + for i, cmd := range candidates { + if cmd.ID == cursor { + start = (i + 1) % len(candidates) + foundCursor = true + break + } + } + // A successful prior pass may already have deleted the cursor's comment. + // GitHub comment IDs increase chronologically, so resume at the first later + // candidate instead of falling back to the oldest and rescanning it. + if cursor != 0 && !foundCursor { + for i, cmd := range candidates { + if cmd.ID > cursor { + start = i + break + } + } + } + + var onPR []ghapi.Reaction + readPR := false + scanned := 0 + for offset := 0; offset < len(candidates) && scanned < maxTidyReactionCandidates; offset++ { + cmd := candidates[(start+offset)%len(candidates)] + if answeredSince(out, cmd) { + continue + } + scanned++ + cursor = cmd.ID reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) if err != nil { // Housekeeping: an unreadable reaction keeps the comment, and the @@ -348,7 +459,33 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa noteThumbsUp(out, cmd, reactions) } } - return out + return out, cursor +} + +// recordTidyState advances the bounded reaction scan and writes command +// tombstones in one CAS update. It avoids a state commit when neither changed. +func (s *Service) recordTidyState(ctx context.Context, repo string, pr int, cursor int64, commands []PostedCommand) error { + key := QueueKey(repo, pr) + _, err := s.store.Update(ctx, func(st *State) error { + changed := false + if cursor != 0 && st.TidyReactionCursors[key] != cursor { + if st.TidyReactionCursors == nil { + st.TidyReactionCursors = map[string]int64{} + } + st.TidyReactionCursors[key] = cursor + changed = true + } + before := len(st.TidiedCommands[key]) + st.RecordTidied(repo, pr, commands...) + if len(st.TidiedCommands[key]) != before { + changed = true + } + if !changed { + return ErrNoChange + } + return nil + }) + return err } // answeredSince reports whether cmd's bot has demonstrably acted since cmd was diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 820986d..fc79c69 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -101,6 +101,13 @@ func TestTidyRemovesOnlySpentCommandsCrqPosted(t *testing.T) { if len(result.Deleted) != 1 || result.Deleted[0] != 100 { t.Fatalf("deleted = %v, want only the spent command from the superseded round", result.Deleted) } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 1 || got[0].ID != 100 { + t.Fatalf("tidied commands = %v, want a durable tombstone for comment 100", got) + } left := map[int64]bool{} for _, c := range gh.comments[fakeKey(repo, pr)] { @@ -278,6 +285,71 @@ func TestTidyKeepsATriggerCommentEditedIntoSomethingElse(t *testing.T) { } } +// The initial issue-comment page is only a snapshot. Recheck the exact comment +// after the slower force-push and reaction reads, immediately before DELETE. +func TestTidyKeepsATriggerEditedDuringThePass(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 24 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + original := ghapi.IssueComment{ + ID: 100, Body: cfg.ReviewCommand, + CreatedAt: now.Add(-2 * time.Hour), UpdatedAt: now.Add(-2 * time.Hour), + } + original.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{original} + edited := original + edited.Body = cfg.ReviewCommand + "\n\nKeep this context." + edited.UpdatedAt = now.Add(-time.Minute) + gh.getComment = func(_ string, _ int64) (ghapi.IssueComment, error) { + return edited, nil + } + review := ghapi.Review{ + ID: 900, CommitID: "bbbbbbbb2", State: "COMMENTED", Body: "looks fine", + SubmittedAt: now.Add(-90 * time.Minute), + } + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(original.ID, original.CreatedAt); err != nil { + return err + } + r.RecordPosted(cfg.Bot, original.ID, original.CreatedAt) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Deleted) != 0 || len(gh.deleted) != 0 { + t.Fatalf("edited comment was deleted: result=%#v deleted=%v", result, gh.deleted) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 0 { + t.Fatalf("kept comment retained a deletion tombstone: %v", got) + } +} + // A delete GitHub refuses must reach the caller. An empty Deleted otherwise // reads as "nothing was spent" when it means "the token may not delete". func TestTidyReportsDeletionFailures(t *testing.T) { @@ -328,6 +400,75 @@ func TestTidyReportsDeletionFailures(t *testing.T) { } } +func TestTidyBoundsAndRotatesUnansweredCodexReactionReads(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = []CoBotConfig{{Login: dialect.CodexBotLogin, Name: "codex", Command: "@codex review"}} + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 25 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + round := Round{ + Repo: repo, PR: pr, Head: "aaaaaaaa1", Phase: PhaseCompleted, + EnqueuedAt: now.Add(-3 * time.Hour), + } + for i := int64(1); i <= 10; i++ { + at := now.Add(time.Duration(-130+i) * time.Minute) + comment := ghapi.IssueComment{ID: i, Body: "@codex review", CreatedAt: at, UpdatedAt: at} + comment.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = append(gh.comments[fakeKey(repo, pr)], comment) + round.RecordPosted(dialect.CodexBotLogin, i, at) + } + + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.Archive = append(st.Archive, round) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + + if _, err := svc.Tidy(ctx, repo, pr, false); err != nil { + t.Fatal(err) + } + first := append([]int64(nil), gh.reactionReads...) + if len(first) != maxTidyReactionCandidates { + t.Fatalf("first pass read reactions for %v, want %d candidates", first, maxTidyReactionCandidates) + } + for i, id := range first { + if want := int64(i + 1); id != want { + t.Fatalf("first pass = %v, want commands 1 through %d", first, maxTidyReactionCandidates) + } + } + if _, err := svc.Tidy(ctx, repo, pr, false); err != nil { + t.Fatal(err) + } + second := gh.reactionReads[len(first):] + if len(second) != maxTidyReactionCandidates { + t.Fatalf("second pass read reactions for %v, want %d candidates", second, maxTidyReactionCandidates) + } + for i, id := range second { + if want := int64(maxTidyReactionCandidates + i + 1); id != want { + t.Fatalf("persisted cursor did not rotate the scan: first=%v second=%v", first, second) + } + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidyReactionCursors[QueueKey(repo, pr)]; got != second[len(second)-1] { + t.Fatalf("cursor = %d, want last inspected command %d", got, second[len(second)-1]) + } +} + // Codex can answer a trigger with nothing but its thumbs-up, and that reaction // alone completes the round — so it is the only evidence that trigger was ever // read, and without it the comment would be kept for ever. diff --git a/internal/gh/github.go b/internal/gh/github.go index 0b81662..774e0ed 100644 --- a/internal/gh/github.go +++ b/internal/gh/github.go @@ -908,6 +908,12 @@ func (g *GitHub) ListIssueComments(ctx context.Context, repo string, issue int) return out, err } +func (g *GitHub) GetIssueComment(ctx context.Context, repo string, commentID int64) (IssueComment, error) { + var out IssueComment + err := g.request(ctx, http.MethodGet, fmt.Sprintf("/repos/%s/issues/comments/%d", repoPath(repo), commentID), nil, &out) + return out, err +} + // ListIssueCommentsPage fetches a single page (GitHub returns oldest-first), so // callers that only need the oldest comments (e.g. calibration pruning) don't // page through thousands of them. diff --git a/internal/state/posted_test.go b/internal/state/posted_test.go index fe3f658..d952d26 100644 --- a/internal/state/posted_test.go +++ b/internal/state/posted_test.go @@ -71,3 +71,25 @@ func TestFireAloneClaimsNoAuthorship(t *testing.T) { t.Fatalf("posted = %v, want nothing: crq wrote neither comment", r.PostedCommands) } } + +func TestTidiedCommandsSurviveArchiveEviction(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + st := New() + command := PostedCommand{ID: 77, Bot: "coderabbitai", At: now} + st.RecordTidied("O/R", 1, command) + st.RecordTidied("o/r", 1, command) + + for i := 0; i < ArchiveMax+1; i++ { + st.Archive = append(st.Archive, Round{Repo: "o/other", PR: i + 1, Phase: PhaseCompleted}) + } + st.Normalize(now) + + key := Key("o/r", 1) + if got := st.TidiedCommands[key]; len(got) != 1 || got[0].ID != command.ID { + t.Fatalf("tidied commands = %v, want the durable tombstone", got) + } + st.ForgetTidied("o/r", 1, command.ID) + if _, ok := st.TidiedCommands[key]; ok { + t.Fatalf("forgotten tombstone still present: %v", st.TidiedCommands[key]) + } +} diff --git a/internal/state/state.go b/internal/state/state.go index 94e196e..f8b4701 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -309,6 +309,16 @@ type State struct { // for the dashboard and debugging. Bounded by ArchiveMax. Archive []Round `json:"archive,omitempty"` + // TidiedCommands are durable tombstones for trigger comments Tidy removed. + // Unlike Archive, they are not bounded: GitHub can deliver a delayed reply + // for as long as the PR conversation exists, and command/reply FIFO pairing + // still needs the deleted command's chronological position. + TidiedCommands map[string][]PostedCommand `json:"tidied_commands,omitempty"` + // TidyReactionCursors rotate the bounded scan of unanswered Codex commands + // so old candidates are not reread on every housekeeping pass and newer + // candidates are not starved. + TidyReactionCursors map[string]int64 `json:"tidy_reaction_cursors,omitempty"` + Warn string `json:"warn,omitempty"` UpdatedAt *time.Time `json:"wrote_at,omitempty"` DashboardSHA string `json:"dashboard_sha,omitempty"` @@ -651,6 +661,57 @@ func (r *Round) RecordPosted(bot string, id int64, at time.Time) { r.PostedCommands = posted } +// RecordTidied remembers a trigger before Tidy removes it from GitHub. It is +// idempotent because a CAS retry may apply the same mutation more than once. +func (s *State) RecordTidied(repo string, pr int, commands ...PostedCommand) { + if len(commands) == 0 { + return + } + if s.TidiedCommands == nil { + s.TidiedCommands = map[string][]PostedCommand{} + } + key := Key(repo, pr) + have := make(map[int64]bool, len(s.TidiedCommands[key])+len(commands)) + for _, command := range s.TidiedCommands[key] { + have[command.ID] = true + } + for _, command := range commands { + if command.ID == 0 || have[command.ID] { + continue + } + command.At = command.At.UTC() + s.TidiedCommands[key] = append(s.TidiedCommands[key], command) + have[command.ID] = true + } +} + +// ForgetTidied removes tombstones for comments Tidy ultimately kept or failed +// to delete. A present GitHub comment remains the source of truth for those. +func (s *State) ForgetTidied(repo string, pr int, ids ...int64) { + key := Key(repo, pr) + if len(ids) == 0 || len(s.TidiedCommands[key]) == 0 { + return + } + remove := make(map[int64]bool, len(ids)) + for _, id := range ids { + remove[id] = true + } + kept := s.TidiedCommands[key][:0] + for _, command := range s.TidiedCommands[key] { + if !remove[command.ID] { + kept = append(kept, command) + } + } + if len(kept) == 0 { + delete(s.TidiedCommands, key) + if len(s.TidiedCommands) == 0 { + s.TidiedCommands = nil + } + return + } + s.TidiedCommands[key] = kept +} + // 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 71bb2ce84486ff6c0e538361ac0efc3f33c8761f Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 07:59:21 +0200 Subject: [PATCH 10/12] Preserve tidy history and throttle backoff --- README.md | 11 +-- cmd/crq/main.go | 4 +- internal/crq/auto.go | 4 +- internal/crq/config.go | 8 +-- internal/crq/config_test.go | 27 ++++++++ internal/crq/service.go | 8 ++- internal/crq/tidy.go | 113 ++++++++++++++++++++++++------- internal/crq/tidy_test.go | 39 ++++++++++- llms.txt | 4 +- skills/coderabbit-queue/SKILL.md | 2 +- 10 files changed, 174 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index a0061ed..eef155a 100644 --- a/README.md +++ b/README.md @@ -213,14 +213,15 @@ reviewed. The two flags mirror CodeRabbit's own toggles: default = *Automatic + commit it requested a review for, so the same commit is never reviewed twice. One process is the leader at a time (a lease in the shared state), so running the daemon on several machines is safe. -As each round progresses the daemon also **deletes crq's own spent trigger comments** — the +With `CRQ_TIDY=1`, the daemon also **deletes crq's own spent trigger comments** as each round +progresses — the `@coderabbitai review` / `@codex review` one-liners it posted, which otherwise bury the conversation a human came to read. It removes a comment only when crq wrote it (never one it adopted from a person, never a bot's own comment, and never one edited into something else), only from a round that has moved on, only after the bot answered it, and only once it is too old to adopt again (older than the head -commit, or than a later force-push). Set `CRQ_TIDY=0` to -keep every comment, or run a pass by hand with `crq tidy ` (`--dry-run` reports what it -would remove). +commit, or than a later force-push). Automatic tidying is opt-in so older binaries sharing the state +ref never mis-pair a delayed reply after a newer daemon deletes its command. You can also run a pass +by hand with `crq tidy ` (`--dry-run` reports what it would remove).
Run it persistently (macOS launchd / Linux systemd) @@ -470,7 +471,7 @@ Set these in `~/.config/crq/env` (sourced automatically) or as environment varia | `CRQ_EXCLUDE` | _(none)_ | `autoreview` denylist — never these `owner/name` repos (comma-separated) | | `CRQ_AUTOREVIEW_SKIP_AUTHORS` | `dependabot[bot]` | PR authors `autoreview` never enqueues (comma-separated; case and `[bot]` suffix don't matter) — set to empty to auto-review bot PRs too; manual `crq review` is unaffected | | `CRQ_AUTOREVIEW_SKIP_MARKER` | `` | exact PR-body marker that suppresses fleet auto-review; set empty to disable; manual `crq loop` is unaffected | -| `CRQ_TIDY` | `1` | delete crq's own spent review-trigger comments as rounds progress; `0` leaves every comment on the PR (`crq tidy` by hand is unaffected) | +| `CRQ_TIDY` | `0` | set to `1` to delete crq's own spent review-trigger comments as rounds progress (`crq tidy` by hand is unaffected) | | `CRQ_REQUIRED_BOTS` | `coderabbitai[bot]` | bots that must review the head for convergence (crq waits for all of them) | | `CRQ_COBOTS` | `codex,bugbot,macroscope` | co-reviewers crq surfaces and (optionally) triggers; set empty to disable all | | `CRQ_COBOT__REQUIRED` | `0` | make that co-reviewer gate convergence (folds it into `CRQ_REQUIRED_BOTS`); `` ∈ `CODEX`, `BUGBOT`, `MACROSCOPE` | diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 9cc7ba4..1ec703f 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -559,8 +559,8 @@ It never deletes the bots' own comments. An auto-generated reply can be a rate-limit or skipped-review notice, which crq reads as evidence and surfaces as a finding — deleting those would destroy feedback nobody had read yet. -Runs by itself as rounds progress under crq autoreview; CRQ_TIDY=0 turns that -off. --dry-run reports what it would remove. +Set CRQ_TIDY=1 to run it automatically as rounds progress under crq autoreview. +--dry-run reports what it would remove. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] diff --git a/internal/crq/auto.go b/internal/crq/auto.go index 091ac92..14f7110 100644 --- a/internal/crq/auto.go +++ b/internal/crq/auto.go @@ -72,7 +72,9 @@ func (s *Service) AutoReview(ctx context.Context, opts AutoOptions) error { // A round that just progressed is the moment its trigger comments // stop being needed, so tidying here costs one observation on a PR // crq was already looking at rather than a sweep of the fleet. - s.tidyAfterPump(ctx, pumped) + if tidyErr := s.tidyAfterPump(ctx, pumped); err == nil { + err = tidyErr + } if err != nil { if _, ok := ghapi.ThrottleWait(err); ok { if cont, serr := s.sleepThrottle(ctx, opts, "pump", err); serr != nil || !cont { diff --git a/internal/crq/config.go b/internal/crq/config.go index a21fdfe..9063720 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -69,9 +69,9 @@ type Config struct { LeaderTTL time.Duration FiredMax int // Tidy removes crq's own spent review-trigger comments as rounds progress. - // On by default: they are comments crq posted and the bot has answered, and - // a PR driven through a dozen rounds is otherwise unreadable. CRQ_TIDY=0 - // turns it off. + // It is opt-in while older fleet binaries share the state ref: those + // binaries preserve tombstones as unknown fields but cannot use them when + // pairing a delayed reply. CRQ_TIDY=1 turns it on. Tidy bool NoOpen bool DryRun bool @@ -177,7 +177,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), - Tidy: stringEnv(env, "CRQ_TIDY", "1") != "0", + Tidy: stringEnv(env, "CRQ_TIDY", "0") == "1", 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/config_test.go b/internal/crq/config_test.go index 1016086..47fb903 100644 --- a/internal/crq/config_test.go +++ b/internal/crq/config_test.go @@ -216,6 +216,33 @@ func TestLoadConfigEmptyAutoReviewSkipMarkerDisablesOptOut(t *testing.T) { } } +func TestLoadConfigTidyIsOptIn(t *testing.T) { + t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) + t.Setenv("CRQ_TIDY", "restore-after-test") + os.Unsetenv("CRQ_TIDY") + + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + if cfg.Tidy { + t.Fatal("automatic tidying must stay off until every fleet binary understands tombstones") + } +} + +func TestLoadConfigTidyCanBeEnabled(t *testing.T) { + t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) + t.Setenv("CRQ_TIDY", "1") + + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + if !cfg.Tidy { + t.Fatal("CRQ_TIDY=1 did not enable automatic tidying") + } +} + func TestAuthorSetNormalizesCaseAndBotSuffix(t *testing.T) { set := authorSet("Dependabot[bot], renovate ,") if len(set) != 2 || !set["dependabot"] || !set["renovate"] { diff --git a/internal/crq/service.go b/internal/crq/service.go index 244d0f8..4779ad8 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -247,7 +247,9 @@ func (s *Service) Pump(ctx context.Context) (PumpResult, error) { } else if handled { // The quota-free result is the one Pump exposes to its caller, so // preserve the cleanup hook for the slot result it replaces. - s.tidyAfterPump(ctx, res) + if err := s.tidyAfterPump(ctx, res); err != nil { + return res, err + } return free, nil } return res, nil @@ -659,7 +661,9 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( // A round completed here is invisible to the caller — Pump goes on to report // whatever it fires next, or idle — so this is the only moment that knows the // PR's trigger comments are spent. - s.tidyProgressed(ctx, target.Repo, target.PR) + if err := s.tidyProgressed(ctx, target.Repo, target.PR); err != nil { + return updated, err + } return updated, nil } diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 5a7c05d..625ea50 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -71,12 +71,13 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T // is what lets per-repo reviewers substitute one here later without // threading anything new through. cfg := s.cfg - if len(collectPosted(st, repo, pr).commands) == 0 { + observedPosted := collectPosted(st, repo, pr) + if len(observedPosted.commands) == 0 { result.Kept = append(result.Kept, "no round on this pr posted a trigger comment") return result, nil } - obs, err := s.observe(ctx, cfg, repo, pr, nil, collectPosted(st, repo, pr).commands, s.clock()) + obs, err := s.observe(ctx, cfg, repo, pr, nil, observedPosted.commands, s.clock()) if err != nil { return result, err } @@ -104,11 +105,22 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T present[comment.ID] = comment } triggers := cfg.triggerBodies() + observedIDs := make(map[int64]bool, len(observedPosted.commands)) + for _, cmd := range observedPosted.commands { + observedIDs[cmd.ID] = true + } var commands []engine.CommandComment + var missing []PostedCommand edited := 0 for _, cmd := range posted.commands { comment, onPR := present[cmd.ID] if !onPR { + // The state reload may include a command posted after observation. + // Its absence from this older snapshot says nothing; the next pass + // can classify it from a fresh comment list. + if observedIDs[cmd.ID] { + missing = append(missing, PostedCommand{ID: cmd.ID, Bot: cmd.Bot, At: cmd.CreatedAt}) + } continue } if !isTriggerBody(triggers, cmd.Bot, comment.Body) { @@ -121,22 +133,34 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T result.Kept = append(result.Kept, "a trigger comment crq posted no longer reads as one; someone edited it") } if len(commands) == 0 { + if !dryRun && !s.cfg.DryRun { + if err := s.recordTidyState(ctx, repo, pr, 0, missing); err != nil { + return result, err + } + } result.Kept = append(result.Kept, "every trigger comment crq posted is already gone") return result, nil } + adoptableFrom, err := s.adoptableFrom(ctx, repo, pr, obs.eng.HeadAt) + if err != nil { + return result, err + } in := engine.TidyInput{ Commands: commands, Live: posted.live, Superseded: posted.superseded, - AdoptableFrom: s.adoptableFrom(ctx, repo, pr, obs.eng.HeadAt), + AdoptableFrom: adoptableFrom, } cursor := st.TidyReactionCursors[QueueKey(repo, pr)] - in.AnsweredAt, cursor = s.answered(ctx, repo, pr, obs, commands, posted, cursor) + in.AnsweredAt, cursor, err = s.answered(ctx, repo, pr, obs, commands, posted, cursor) + if err != nil { + return result, err + } stale := engine.StaleCommands(in) if len(stale) == 0 { if !dryRun && !s.cfg.DryRun { - if err := s.recordTidyState(ctx, repo, pr, cursor, nil); err != nil { + if err := s.recordTidyState(ctx, repo, pr, cursor, missing); err != nil { return result, err } } @@ -153,7 +177,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T for _, command := range commands { byID[command.ID] = PostedCommand{ID: command.ID, Bot: command.Bot, At: command.CreatedAt} } - tombstones := make([]PostedCommand, 0, len(stale)) + tombstones := append(make([]PostedCommand, 0, len(missing)+len(stale)), missing...) for _, id := range stale { tombstones = append(tombstones, byID[id]) } @@ -165,8 +189,12 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, err } - var forget []int64 - for _, id := range stale { + var ( + forget []int64 + throttleErr error + ) +deleteLoop: + for i, id := range stale { snapshot := present[id] latest, err := s.gh.GetIssueComment(ctx, repo, id) switch { @@ -177,6 +205,11 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T continue case err != nil: forget = append(forget, id) + if ghapi.IsThrottled(err) { + throttleErr = err + forget = append(forget, stale[i+1:]...) + break deleteLoop + } result.Failed = append(result.Failed, TidyFailure{ID: id, Error: err.Error()}) continue case latest.Body != snapshot.Body || !latest.UpdatedAt.Equal(snapshot.UpdatedAt): @@ -197,6 +230,11 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T result.Deleted = append(result.Deleted, id) default: forget = append(forget, id) + if ghapi.IsThrottled(err) { + throttleErr = err + forget = append(forget, stale[i+1:]...) + break deleteLoop + } // One write failing must not abandon the rest — but it is reported, // not just logged: a caller reading the result is the only one who can // act on "the token cannot delete these". @@ -214,6 +252,9 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, err } } + if throttleErr != nil { + return result, throttleErr + } if s.log != nil && len(result.Deleted) > 0 { s.log.Printf("tidy: %s#%d removed %d spent trigger comment(s)", repo, pr, len(result.Deleted)) } @@ -342,21 +383,24 @@ func isTriggerBody(triggers map[string][]string, bot, body string) bool { // that no round can ever adopt again, for ever once the PR merges. A zero headAt // is the unevaluable guard that keeps every comment, and needs no lookup; a // failed lookup falls back to the commit date, which only ever keeps more. -func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt time.Time) time.Time { +func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt time.Time) (time.Time, error) { if headAt.IsZero() { - return headAt + return headAt, nil } fp, err := s.headForcePushCutoff(ctx, repo, pr) if err != nil { + if ghapi.IsThrottled(err) { + return headAt, err + } if s.log != nil { s.log.Printf("tidy: %s#%d force-push lookup: %v", repo, pr, err) } - return headAt + return headAt, nil } if fp.After(headAt) { - return fp + return fp, nil } - return headAt + return headAt, nil } // answered is answeredAt plus the evidence only a reaction carries. @@ -376,7 +420,7 @@ func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt // still unanswered, so the ordinary pass pays for none of them. const maxTidyReactionCandidates = 4 -func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands, cursor int64) (map[string]time.Time, int64) { +func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands, cursor int64) (map[string]time.Time, int64, error) { out := answeredAt(obs) var candidates []engine.CommandComment for _, cmd := range commands { @@ -386,7 +430,7 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa candidates = append(candidates, cmd) } if len(candidates) == 0 { - return out, cursor + return out, cursor, nil } start := 0 foundCursor := false @@ -421,6 +465,9 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa cursor = cmd.ID reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) if err != nil { + if ghapi.IsThrottled(err) { + return out, cursor, err + } // Housekeeping: an unreadable reaction keeps the comment, and the // next pass tries again. if s.log != nil { @@ -435,6 +482,9 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa if !readPR { readPR = true if onPR, err = s.gh.ListIssueReactions(ctx, repo, pr); err != nil { + if ghapi.IsThrottled(err) { + return out, cursor, err + } if s.log != nil { s.log.Printf("tidy: %s#%d reactions: %v", repo, pr, err) } @@ -451,6 +501,9 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa if fired := posted.firedOn[cmd.ID]; fired != 0 { reactions, err := s.gh.ListCommentReactions(ctx, repo, fired) if err != nil { + if ghapi.IsThrottled(err) { + return out, cursor, err + } if s.log != nil { s.log.Printf("tidy: %s reactions on comment %d: %v", repo, fired, err) } @@ -459,7 +512,7 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa noteThumbsUp(out, cmd, reactions) } } - return out, cursor + return out, cursor, nil } // recordTidyState advances the bounded reaction scan and writes command @@ -551,9 +604,9 @@ func eventAt(e dialect.BotEvent) time.Time { // // It is best-effort by design. Deleting a comment is housekeeping, and a // housekeeping failure must never break the pass that did the real work. -func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) { +func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) error { if res.Repo == "" || res.PR == 0 { - return + return nil } switch res.Action { case "cleared", "deduped", "requeued", "skipped": @@ -572,19 +625,27 @@ func (s *Service) tidyAfterPump(ctx context.Context, res PumpResult) { // it with its commands still live, so it has nothing of its own to remove; // anything older waits for the pass that clears it. default: - return + return nil } - s.tidyProgressed(ctx, res.Repo, res.PR) + return s.tidyProgressed(ctx, res.Repo, res.PR) } // tidyProgressed runs a tidy pass for one PR whose round just moved, and -// swallows the outcome: deleting a comment is housekeeping, and a housekeeping -// failure must never break the pass that did the real work. -func (s *Service) tidyProgressed(ctx context.Context, repo string, pr int) { +// swallows ordinary failures: deleting a comment is housekeeping, and a +// housekeeping failure must never break the pass that did the real work. +// GitHub throttles are returned so autoreview can sleep through the reset +// window instead of continuing to spend requests that are expected to fail. +func (s *Service) tidyProgressed(ctx context.Context, repo string, pr int) error { if !s.cfg.Tidy { - return + return nil } - if _, err := s.Tidy(ctx, repo, pr, false); err != nil && s.log != nil { - s.log.Printf("tidy: %s#%d: %v", repo, pr, err) + if _, err := s.Tidy(ctx, repo, pr, false); err != nil { + if s.log != nil { + s.log.Printf("tidy: %s#%d: %v", repo, pr, err) + } + if ghapi.IsThrottled(err) { + return err + } } + return nil } diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index fc79c69..020332a 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -181,9 +181,8 @@ func TestTidyDryRunWritesNothing(t *testing.T) { } } -// A command crq deleted stays recorded on its round for ever, so a pass that -// did not check what is still on the PR would DELETE it again every time and -// read the 404 back as a fresh removal. +// A command may disappear before crq sees it. It must not be deleted again, but +// its FIFO position must survive after the round falls out of the archive. func TestTidySkipsCommandsAlreadyGone(t *testing.T) { ctx := context.Background() cfg := firingConfig() @@ -235,6 +234,13 @@ func TestTidySkipsCommandsAlreadyGone(t *testing.T) { if len(gh.deleted) != 0 { t.Errorf("issued %d delete(s) for a comment that is not on the pr", len(gh.deleted)) } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 1 || got[0].ID != 100 { + t.Fatalf("tidied commands = %v, want a durable tombstone for absent comment 100", got) + } } // A recorded ID proves crq wrote the comment, not that it is still the one-line @@ -355,6 +361,7 @@ func TestTidyKeepsATriggerEditedDuringThePass(t *testing.T) { func TestTidyReportsDeletionFailures(t *testing.T) { ctx := context.Background() cfg := firingConfig() + cfg.Tidy = true gh := newFakeGitHub() gh.graphQL = noForcePush gh.deleteErrs[100] = errors.New("resource not accessible by integration") @@ -398,6 +405,32 @@ func TestTidyReportsDeletionFailures(t *testing.T) { if len(result.Failed) != 1 || result.Failed[0].ID != 100 || result.Failed[0].Error == "" { t.Fatalf("failed = %+v, want the refused comment and why", result.Failed) } + + // Ordinary cleanup failures stay best-effort, but a GitHub throttle must + // reach autoreview so it sleeps through the reset window. + throttle := &ghapi.RateLimitError{Kind: "primary"} + delete(gh.deleteErrs, 100) + gh.getComment = func(string, int64) (ghapi.IssueComment, error) { + return ghapi.IssueComment{}, throttle + } + if err := svc.tidyProgressed(ctx, repo, pr); !ghapi.IsThrottled(err) { + t.Fatalf("tidyProgressed error = %v, want the pre-delete read throttle", err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 0 { + t.Fatalf("failed pre-delete read retained tombstones for present comments: %v", got) + } + + // The force-push cutoff is another housekeeping lookup; its throttle must + // take the same backoff path. + gh.getComment = nil + gh.graphQL = func(string, map[string]any, any) error { return throttle } + if _, err := svc.Tidy(ctx, repo, pr, false); !ghapi.IsThrottled(err) { + t.Fatalf("Tidy error = %v, want the force-push lookup throttle", err) + } } func TestTidyBoundsAndRotatesUnansweredCodexReactionReads(t *testing.T) { diff --git a/llms.txt b/llms.txt index 9ee4fe9..4215c31 100644 --- a/llms.txt +++ b/llms.txt @@ -202,8 +202,8 @@ 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. -crq removes its own spent `@coderabbitai review` / `@codex review` comments as rounds progress -(`CRQ_TIDY=0` disables). On demand: +With `CRQ_TIDY=1`, crq removes its own spent `@coderabbitai review` / `@codex review` comments as +rounds progress. Automatic tidying is opt-in while older binaries may share the state ref. On demand: ```bash crq tidy REPO PR [--dry-run] diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index c95a2ff..db5351f 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -175,7 +175,7 @@ finding. Pass `--keep-open` to leave it unresolved deliberately. crq deletes its own `@coderabbitai review` / `@codex review` comments once the round that posted them has progressed and the bot has answered, so a PR driven through a dozen rounds stays readable. -This happens by itself under `crq autoreview` (`CRQ_TIDY=0` turns it off), or on demand: +Set `CRQ_TIDY=1` to do this automatically under `crq autoreview`, or run it on demand: ```bash crq tidy "$REPO" "$PR" [--dry-run] From e7ba62de679a4180483609d90593d9d47f8f06f4 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 08:46:29 +0200 Subject: [PATCH 11/12] Retain Codex reaction evidence during tidy --- internal/crq/tidy.go | 56 ++++++++++++++++++++++++++++-------- internal/crq/tidy_test.go | 40 +++++++++++++++++++------- internal/engine/tidy.go | 5 +++- internal/engine/tidy_test.go | 9 ++++++ 4 files changed, 86 insertions(+), 24 deletions(-) diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 625ea50..3f81692 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -153,7 +153,7 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T AdoptableFrom: adoptableFrom, } cursor := st.TidyReactionCursors[QueueKey(repo, pr)] - in.AnsweredAt, cursor, err = s.answered(ctx, repo, pr, obs, commands, posted, cursor) + in.AnsweredAt, in.ReactionTargets, cursor, err = s.answered(ctx, repo, pr, obs, commands, posted, cursor) if err != nil { return result, err } @@ -277,7 +277,7 @@ type postedCommands struct { superseded map[int64]bool // firedOn maps a posted trigger to the primary command its round fired on, // when that is another comment. observe() reads a Codex thumbs-up from there - // too, so it is a reaction source tidying needs — never a delete candidate. + // too, so answered() retains it while its reaction is completion evidence. firedOn map[int64]int64 } @@ -420,8 +420,9 @@ func (s *Service) adoptableFrom(ctx context.Context, repo string, pr int, headAt // still unanswered, so the ordinary pass pays for none of them. const maxTidyReactionCandidates = 4 -func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands, cursor int64) (map[string]time.Time, int64, error) { +func (s *Service) answered(ctx context.Context, repo string, pr int, obs observation, commands []engine.CommandComment, posted postedCommands, cursor int64) (map[string]time.Time, map[int64]bool, int64, error) { out := answeredAt(obs) + reactionTargets := map[int64]bool{} var candidates []engine.CommandComment for _, cmd := range commands { if posted.live[cmd.ID] || !dialect.IsCodexBot(cmd.Bot) || answeredSince(out, cmd) { @@ -430,7 +431,28 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa candidates = append(candidates, cmd) } if len(candidates) == 0 { - return out, cursor, nil + return out, reactionTargets, cursor, nil + } + // A candidate may have been triggered beside another command and Codex may + // have approved by reacting to that primary command. Until this bounded scan + // identifies where the approval lives, conservatively retain every possible + // target; otherwise an unscanned candidate could lose its evidence. + possibleTargets := map[int64]int{} + for _, cmd := range candidates { + if fired := posted.firedOn[cmd.ID]; fired != 0 { + possibleTargets[fired]++ + reactionTargets[fired] = true + } + } + clearPossibleTarget := func(cmd engine.CommandComment) { + fired := posted.firedOn[cmd.ID] + if fired == 0 { + return + } + possibleTargets[fired]-- + if possibleTargets[fired] == 0 { + delete(reactionTargets, fired) + } } start := 0 foundCursor := false @@ -466,7 +488,7 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa reactions, err := s.gh.ListCommentReactions(ctx, repo, cmd.ID) if err != nil { if ghapi.IsThrottled(err) { - return out, cursor, err + return out, reactionTargets, cursor, err } // Housekeeping: an unreadable reaction keeps the comment, and the // next pass tries again. @@ -475,15 +497,18 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa } continue } - noteThumbsUp(out, cmd, reactions) + if noteThumbsUp(out, cmd, reactions) { + reactionTargets[cmd.ID] = true + } if answeredSince(out, cmd) { + clearPossibleTarget(cmd) continue } if !readPR { readPR = true if onPR, err = s.gh.ListIssueReactions(ctx, repo, pr); err != nil { if ghapi.IsThrottled(err) { - return out, cursor, err + return out, reactionTargets, cursor, err } if s.log != nil { s.log.Printf("tidy: %s#%d reactions: %v", repo, pr, err) @@ -493,6 +518,7 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa } noteThumbsUp(out, cmd, onPR) if answeredSince(out, cmd) { + clearPossibleTarget(cmd) continue } // Last, the command this trigger's own round fired on. A round gated on @@ -502,17 +528,19 @@ func (s *Service) answered(ctx context.Context, repo string, pr int, obs observa reactions, err := s.gh.ListCommentReactions(ctx, repo, fired) if err != nil { if ghapi.IsThrottled(err) { - return out, cursor, err + return out, reactionTargets, cursor, err } if s.log != nil { s.log.Printf("tidy: %s reactions on comment %d: %v", repo, fired, err) } continue } - noteThumbsUp(out, cmd, reactions) + if !noteThumbsUp(out, cmd, reactions) { + clearPossibleTarget(cmd) + } } } - return out, cursor, nil + return out, reactionTargets, cursor, nil } // recordTidyState advances the bounded reaction scan and writes command @@ -548,12 +576,15 @@ func answeredSince(answered map[string]time.Time, cmd engine.CommandComment) boo return ok && !at.Before(cmd.CreatedAt) } -// noteThumbsUp records a Codex +1 among reactions as cmd's bot having acted. -func noteThumbsUp(answered map[string]time.Time, cmd engine.CommandComment, reactions []ghapi.Reaction) { +// noteThumbsUp records a Codex +1 among reactions as cmd's bot having acted and +// reports whether the comment carrying those reactions must be retained. +func noteThumbsUp(answered map[string]time.Time, cmd engine.CommandComment, reactions []ghapi.Reaction) bool { + found := false for _, reaction := range reactions { if !isCurrentCodexThumbsUp(reaction, cmd.CreatedAt) { continue } + found = true at := reaction.CreatedAt if at.IsZero() { at = cmd.CreatedAt @@ -562,6 +593,7 @@ func noteThumbsUp(answered map[string]time.Time, cmd engine.CommandComment, reac answered[cmd.Bot] = at } } + return found } // answeredAt is the newest moment each reviewer demonstrably acted on this PR. diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 020332a..0e81aad 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -502,10 +502,10 @@ func TestTidyBoundsAndRotatesUnansweredCodexReactionReads(t *testing.T) { } } -// Codex can answer a trigger with nothing but its thumbs-up, and that reaction -// alone completes the round — so it is the only evidence that trigger was ever -// read, and without it the comment would be kept for ever. -func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { +// Codex can answer a trigger with nothing but its thumbs-up, and deleting the +// target deletes that approval too. Keep the target so subsequent observations +// still see the completed gate. +func TestTidyRetainsCodexThumbsUpReactionTarget(t *testing.T) { ctx := context.Background() cfg := firingConfig() cfg.CoBots = []CoBotConfig{{Login: dialect.CodexBotLogin, Name: "codex", Command: "@codex review"}} @@ -516,9 +516,9 @@ func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { var pull ghapi.Pull pull.State = "open" - pull.Head.SHA = "bbbbbbbb2" + pull.Head.SHA = "aaaaaaaa1" gh.pulls[fakeKey(repo, pr)] = pull - gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + gh.commits["aaaaaaaa1"] = commitAt(now.Add(-10 * time.Minute)) c := ghapi.IssueComment{ID: 100, Body: "@codex review", CreatedAt: now.Add(-2 * time.Hour)} c.User.Login = "kristofferR" @@ -527,6 +527,12 @@ func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { thumb := ghapi.Reaction{ID: 1, Content: "+1", CreatedAt: now.Add(-100 * time.Minute)} thumb.User.Login = dialect.CodexBotLogin gh.reactions[100] = []ghapi.Reaction{thumb} + review := ghapi.Review{ + ID: 101, CommitID: pull.Head.SHA, State: "COMMENTED", + SubmittedAt: now.Add(-90 * time.Minute), Body: "review complete", + } + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} store := NewMemoryStore(cfg) svc := NewService(cfg, gh, store, nil) @@ -550,8 +556,13 @@ func TestTidyCountsCodexThumbsUpAsTheAnswer(t *testing.T) { if err != nil { t.Fatal(err) } - if len(result.Deleted) != 1 || result.Deleted[0] != 100 { - t.Fatalf("deleted = %v (kept: %v), want the thumbed-up trigger removed", result.Deleted, result.Kept) + if len(result.Deleted) != 0 { + t.Fatalf("deleted = %v, want the reaction target retained", result.Deleted) + } + if report, err := svc.Feedback(ctx, repo, pr); err != nil { + t.Fatal(err) + } else if !report.Converged { + t.Fatalf("feedback lost the Codex approval after tidy: %#v", report) } } @@ -683,12 +694,18 @@ func TestTidyCountsACodexThumbsUpOnTheFiredCommand(t *testing.T) { c.User.Login = "kristofferR" gh.comments[fakeKey(repo, pr)] = append(gh.comments[fakeKey(repo, pr)], c) } - add(100, cfg.ReviewCommand) // the round's own fire, adopted from a person - add(101, "@codex review") // and the co-reviewer trigger crq posted beside it + add(100, cfg.ReviewCommand) // the round's own fire, posted by crq + add(101, "@codex review") // and its co-reviewer trigger // Codex's whole answer, left on the command that fired the round. thumb := ghapi.Reaction{ID: 1, Content: "+1", CreatedAt: now.Add(-100 * time.Minute)} thumb.User.Login = dialect.CodexBotLogin gh.reactions[100] = []ghapi.Reaction{thumb} + review := ghapi.Review{ + ID: 900, CommitID: pull.Head.SHA, State: "COMMENTED", + SubmittedAt: now.Add(-90 * time.Minute), Body: "review complete", + } + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} store := NewMemoryStore(cfg) svc := NewService(cfg, gh, store, nil) @@ -698,6 +715,7 @@ func TestTidyCountsACodexThumbsUpOnTheFiredCommand(t *testing.T) { return err } r.SetCoCommand(dialect.CodexBotLogin, 101, now.Add(-2*time.Hour)) + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) r.RecordPosted(dialect.CodexBotLogin, 101, now.Add(-2*time.Hour)) return nil }) @@ -710,7 +728,7 @@ func TestTidyCountsACodexThumbsUpOnTheFiredCommand(t *testing.T) { t.Fatal(err) } if len(result.Deleted) != 1 || result.Deleted[0] != 101 { - t.Fatalf("deleted = %v (kept: %v), want the trigger the reaction on the fired command answered", result.Deleted, result.Kept) + t.Fatalf("deleted = %v (kept: %v), want the co trigger removed and its reaction target retained", result.Deleted, result.Kept) } } diff --git a/internal/engine/tidy.go b/internal/engine/tidy.go index 9445c8e..a23480c 100644 --- a/internal/engine/tidy.go +++ b/internal/engine/tidy.go @@ -31,6 +31,9 @@ type TidyInput struct { // one. They are exempt from the head check: crq's own record that it has // replaced a command is stronger evidence than any timestamp. Superseded map[int64]bool + // ReactionTargets are comments whose reaction is still completion evidence. + // Deleting the comment would delete that evidence with it. + ReactionTargets map[int64]bool } // StaleCommands returns the trigger comments that can be deleted: crq asked, the @@ -56,7 +59,7 @@ type TidyInput struct { func StaleCommands(in TidyInput) []int64 { var stale []int64 for _, cmd := range in.Commands { - if in.Live[cmd.ID] { + if in.Live[cmd.ID] || in.ReactionTargets[cmd.ID] { continue } answered, ok := in.AnsweredAt[cmd.Bot] diff --git a/internal/engine/tidy_test.go b/internal/engine/tidy_test.go index 1686605..7771c5a 100644 --- a/internal/engine/tidy_test.go +++ b/internal/engine/tidy_test.go @@ -39,6 +39,15 @@ func TestStaleCommands(t *testing.T) { }, }, }, + { + name: "a reaction target keeps its completion evidence", + in: TidyInput{ + AdoptableFrom: head, + AnsweredAt: answered, + ReactionTargets: map[int64]bool{1: true}, + Commands: []CommandComment{{ID: 1, Bot: "coderabbitai", CreatedAt: base}}, + }, + }, { name: "no evidence this bot ever acted", in: TidyInput{ From be549798d6e0628217518dad2a250c6360b4de28 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Mon, 27 Jul 2026 09:11:32 +0200 Subject: [PATCH 12/12] Preserve tombstones after ambiguous deletes --- internal/crq/service_test.go | 24 +++++++------ internal/crq/tidy.go | 9 ++++- internal/crq/tidy_test.go | 65 ++++++++++++++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 15 deletions(-) diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index b566ef4..b795738 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -33,6 +33,7 @@ type fakeGitHub struct { checkRunErrs map[string]error postBodyErrs map[string]error // body → error (selective trigger-post failures) deleteErrs map[int64]error // comment id → error (GitHub refuses the delete) + deleteAfterErrs map[int64]error // comment id → error after GitHub applies the delete posted []string deleted []int64 commentID int64 @@ -62,16 +63,17 @@ func (f *fakeGitHub) clock() time.Time { func newFakeGitHub() *fakeGitHub { return &fakeGitHub{ - pulls: map[string]ghapi.Pull{}, - commits: map[string]ghapi.Commit{}, - commitErrs: map[string]error{}, - reviews: map[string][]ghapi.Review{}, - comments: map[string][]ghapi.IssueComment{}, - reviewComments: map[string][]ghapi.ReviewComment{}, - issueReactions: map[string][]ghapi.Reaction{}, - reactions: map[int64][]ghapi.Reaction{}, - reactionErrs: map[int64]error{}, - deleteErrs: map[int64]error{}, + pulls: map[string]ghapi.Pull{}, + commits: map[string]ghapi.Commit{}, + commitErrs: map[string]error{}, + reviews: map[string][]ghapi.Review{}, + comments: map[string][]ghapi.IssueComment{}, + reviewComments: map[string][]ghapi.ReviewComment{}, + issueReactions: map[string][]ghapi.Reaction{}, + reactions: map[int64][]ghapi.Reaction{}, + reactionErrs: map[int64]error{}, + deleteErrs: map[int64]error{}, + deleteAfterErrs: map[int64]error{}, } } @@ -250,7 +252,7 @@ func (f *fakeGitHub) DeleteIssueComment(_ context.Context, repo string, id int64 if c.ID == id { f.comments[key] = append(list[:i], list[i+1:]...) f.deleted = append(f.deleted, id) - return nil + return f.deleteAfterErrs[id] } } } diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index 3f81692..bb04f1f 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -229,12 +229,19 @@ deleteLoop: // comments, and a person may tidy by hand. result.Deleted = append(result.Deleted, id) default: - forget = append(forget, id) if ghapi.IsThrottled(err) { + forget = append(forget, id) throttleErr = err forget = append(forget, stale[i+1:]...) break deleteLoop } + // A transport failure may arrive after GitHub applied the DELETE. + // Keep the prewritten tombstone unless an API response proves the + // comment remained on GitHub. + var apiErr *ghapi.APIError + if errors.As(err, &apiErr) { + forget = append(forget, id) + } // One write failing must not abandon the rest — but it is reported, // not just logged: a caller reading the result is the only one who can // act on "the token cannot delete these". diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 0e81aad..149da86 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -3,7 +3,6 @@ package crq import ( "context" "encoding/json" - "errors" "testing" "time" @@ -364,7 +363,7 @@ func TestTidyReportsDeletionFailures(t *testing.T) { cfg.Tidy = true gh := newFakeGitHub() gh.graphQL = noForcePush - gh.deleteErrs[100] = errors.New("resource not accessible by integration") + gh.deleteErrs[100] = &ghapi.APIError{Method: "DELETE", Status: 403, Body: "resource not accessible by integration"} now := time.Now().UTC() repo, pr := "o/r", 16 @@ -405,6 +404,13 @@ func TestTidyReportsDeletionFailures(t *testing.T) { if len(result.Failed) != 1 || result.Failed[0].ID != 100 || result.Failed[0].Error == "" { t.Fatalf("failed = %+v, want the refused comment and why", result.Failed) } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 0 { + t.Fatalf("definitively refused delete retained tombstones: %v", got) + } // Ordinary cleanup failures stay best-effort, but a GitHub throttle must // reach autoreview so it sleeps through the reset window. @@ -416,7 +422,7 @@ func TestTidyReportsDeletionFailures(t *testing.T) { if err := svc.tidyProgressed(ctx, repo, pr); !ghapi.IsThrottled(err) { t.Fatalf("tidyProgressed error = %v, want the pre-delete read throttle", err) } - st, _, err := store.Load(ctx) + st, _, err = store.Load(ctx) if err != nil { t.Fatal(err) } @@ -433,6 +439,59 @@ func TestTidyReportsDeletionFailures(t *testing.T) { } } +func TestTidyRetainsTombstoneWhenDeleteOutcomeIsAmbiguous(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Tidy = true + gh := newFakeGitHub() + gh.graphQL = noForcePush + gh.deleteAfterErrs[100] = context.DeadlineExceeded + now := time.Now().UTC() + repo, pr := "o/r", 29 + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: cfg.ReviewCommand, CreatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + review := ghapi.Review{ID: 900, CommitID: "bbbbbbbb2", SubmittedAt: now.Add(-90 * time.Minute), State: "COMMENTED"} + review.User.Login = cfg.Bot + gh.reviews[fakeKey(repo, pr)] = []ghapi.Review{review} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(100, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted(cfg.Bot, 100, now.Add(-2*time.Hour)) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, false) + if err != nil { + t.Fatal(err) + } + if len(result.Failed) != 1 || result.Failed[0].ID != 100 { + t.Fatalf("failed = %+v, want the ambiguous delete failure", result.Failed) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := st.TidiedCommands[QueueKey(repo, pr)]; len(got) != 1 || got[0].ID != 100 { + t.Fatalf("tidied commands = %v, want tombstone for ambiguously deleted comment 100", got) + } +} + func TestTidyBoundsAndRotatesUnansweredCodexReactionReads(t *testing.T) { ctx := context.Background() cfg := firingConfig()