From 243d48d59d59c1849d862a5cec4ac252a66e0b15 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 18:28:42 +0200 Subject: [PATCH 1/5] Choose which bots review which project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewers were a property of the process: one CRQ_COBOTS for every repository the fleet touches. A repo that only wants Codex, or one where Bugbot should gate, had no way to say so — and the answer to "which bots do you want here" was to edit a global env file and restart a daemon. crq reviewers now reports who runs there and what each one costs; crq reviewers set chooses them; crq reviewers clear goes back to the fleet default. Each reviewer reports its budget, which is the only property the queue cares about: account is serialized against the shared CodeRabbit allowance, none runs immediately and waits for nobody. The configuration lives in the shared state ref rather than a .crq.yaml the repository carries. The daemon has no checkout of the repositories it reviews, so an in-repo file would be invisible to it or cost a REST fetch per PR — and a daemon and an agent reading different configurations while writing one state ref is a class of bug worth not having. The override reaches the derived views, not just the stored list: after applying it, Reviewers, RequiredBots and FeedbackBots are rebuilt exactly as LoadConfig builds them, so crq cannot gate on or surface findings from a bot the repository excluded. "Chosen to be none" is kept distinct from "not chosen". The primary stays fleet-wide. Its markers and command are compiled into the dialect classifiers when the Service is built, so a per-repo primary means per-repo classifiers. --- README.md | 3 + cmd/crq/main.go | 117 ++++++++++++++++++++ internal/crq/codex_replay_test.go | 4 +- internal/crq/feedback.go | 13 +-- internal/crq/repoconfig.go | 173 ++++++++++++++++++++++++++++++ internal/crq/repoconfig_test.go | 65 +++++++++++ internal/crq/reviewers.go | 51 +++++++++ internal/crq/reviewers_test.go | 51 +++++++++ internal/crq/service.go | 82 +++++++------- internal/crq/service_test.go | 6 +- internal/crq/state.go | 2 + internal/state/state.go | 56 ++++++++++ llms.txt | 12 +++ skills/coderabbit-queue/SKILL.md | 14 +++ 14 files changed, 599 insertions(+), 50 deletions(-) create mode 100644 internal/crq/repoconfig.go create mode 100644 internal/crq/repoconfig_test.go diff --git a/README.md b/README.md index f9d091a..5f3c96b 100644 --- a/README.md +++ b/README.md @@ -385,6 +385,9 @@ 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 reviewers # which bots review this project, and what each costs +crq reviewers set --bots [--required ] # choose them +crq reviewers clear # back to the fleet default 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 diff --git a/cmd/crq/main.go b/cmd/crq/main.go index b1cd9db..6f80808 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -211,6 +211,12 @@ func run(ctx context.Context, args []string) int { } printJSON(result) return 0 + case "reviewers": + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + return runReviewers(ctx, service, args[1:]) case "decline": threads, reason, resolve, ok := parseDeclineArgs(args[1:]) if !ok || len(threads) == 0 || strings.TrimSpace(reason) == "" { @@ -356,6 +362,10 @@ 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 reviewers which bots review this project (and what each costs) + crq reviewers set --bots [--required ] + choose this project's reviewers + crq reviewers clear go back to the fleet default crq autoreview [--once] [--no-incremental] keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] @@ -509,6 +519,32 @@ replies contesting the decline, crq re-surfaces that reply as its own finding. Pass --keep-open to leave it unresolved anyway (an on-the-record disagreement you intend to keep working). Thread IDs come from .findings[].thread_id. +`) + case "reviewers": + fmt.Print(`crq reviewers +crq reviewers set --bots [--required ] +crq reviewers clear + +Which bots review one project, and what each of them costs. + +Without a subcommand it reports the reviewers that will actually run there — the +fleet default, or this repository's own choice if it has one. Each entry carries +its budget: "account" is serialized against the shared CodeRabbit allowance, +"none" runs immediately and waits for nobody. + + set --bots chooses the co-reviewers; --required chooses which reviewers + gate convergence. An empty value means none here, which is a different + answer from not setting it at all. + clear drops the override so the repository follows the fleet again. + +The configuration lives in the shared state ref, not in a file the repository +carries: the daemon has no checkout of the repos it reviews, and a daemon and an +agent reading different configurations while writing one state ref is a class of +bug worth not having. + +The primary reviewer is fleet-wide. Its markers and command are compiled into the +classifiers when crq starts, so a per-repo primary would mean per-repo +classifiers — a much larger change than choosing who else runs. `) case "autoreview", "auto": fmt.Print(`crq autoreview [--once] [--no-incremental] @@ -696,6 +732,87 @@ func codeRabbitOrg(ctx context.Context, binary string) string { return checkCodeRabbitAuth(ctx, tools).CurrentOrg } +// runReviewers handles `crq reviewers [set|clear] [flags]`. +func runReviewers(ctx context.Context, service *crq.Service, args []string) int { + action, rest := "show", args + if len(args) > 0 && (args[0] == "set" || args[0] == "clear") { + action, rest = args[0], args[1:] + } + var repo string + var bots, required *string + for i := 0; i < len(rest); i++ { + arg := rest[i] + switch { + case arg == "--bots", arg == "--required": + if i+1 >= len(rest) { + fatal(fmt.Errorf("%s needs a value", arg)) + return 1 + } + value := rest[i+1] + i++ + if arg == "--bots" { + bots = &value + } else { + required = &value + } + case strings.HasPrefix(arg, "--bots="): + value := strings.TrimPrefix(arg, "--bots=") + bots = &value + case strings.HasPrefix(arg, "--required="): + value := strings.TrimPrefix(arg, "--required=") + required = &value + case strings.HasPrefix(arg, "-"): + fatal(fmt.Errorf("unknown flag %s (usage: crq reviewers set --bots )", arg)) + return 1 + case repo == "": + repo = arg + default: + fatal(fmt.Errorf("unexpected argument %q", arg)) + return 1 + } + } + if repo == "" { + fatal(errors.New("usage: crq reviewers [set|clear] [--bots ] [--required ]")) + return 1 + } + + var view crq.ReviewerView + var err error + switch action { + case "clear": + view, err = service.ClearReviewers(ctx, repo) + case "set": + if bots == nil && required == nil { + fatal(errors.New("set needs --bots or --required (crq reviewers clear drops the override)")) + return 1 + } + view, err = service.SetReviewers(ctx, repo, splitList(bots), splitList(required)) + default: + view, err = service.Reviewers(ctx, repo) + } + if err != nil { + fatal(err) + return 1 + } + printJSON(view) + return 0 +} + +// splitList turns an unset flag into nil and an empty one into an empty +// non-nil slice: "not chosen" and "chosen to be none" are different answers. +func splitList(value *string) []string { + if value == nil { + return nil + } + out := []string{} + for _, part := range strings.Split(*value, ",") { + if part = strings.TrimSpace(part); part != "" { + out = append(out, part) + } + } + return out +} + func repoPR(args []string) (string, int, bool) { if len(args) != 2 { return "", 0, false diff --git a/internal/crq/codex_replay_test.go b/internal/crq/codex_replay_test.go index 5373d08..041965e 100644 --- a/internal/crq/codex_replay_test.go +++ b/internal/crq/codex_replay_test.go @@ -513,9 +513,9 @@ func TestSelfHealCodexClaimPreventsDoublePost(t *testing.T) { } obs.eng.Events = events before := f.codexPosted(repo, pr) - f.svc.selfHealCoReviewers(f.ctx, *round, obs.eng, f.clk.now()) + f.svc.selfHealCoReviewers(f.ctx, f.svc.cfg, *round, obs.eng, f.clk.now()) // Second sweeper with the SAME stale observation (still CodexCommandID==0). - f.svc.selfHealCoReviewers(f.ctx, *round, obs.eng, f.clk.now()) + f.svc.selfHealCoReviewers(f.ctx, f.svc.cfg, *round, obs.eng, f.clk.now()) if got := f.codexPosted(repo, pr) - before; got != 1 { t.Fatalf("concurrent self-heals must post exactly once, got %d extra posts", got) } diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 12d007a..b10509b 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -88,7 +88,8 @@ 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) + cfg := s.cfgFor(st, repo) + obs, err := s.observe(ctx, cfg, repo, pr, round, now) if err != nil { return FeedbackReport{}, err } @@ -123,13 +124,13 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe if anchorOK { anchorCutoff = completionRound.FiredAt.UTC() } - completion := engine.Completion(completionRound, obs.eng, s.cfg.policy()) + completion := engine.Completion(completionRound, obs.eng, cfg.policy()) report.ReviewedBy = completion.ReviewedBy // Completion does not always arrive as a review: a clean-summary comment, a // paired completion reply and a co-reviewer check run all satisfy it. Anchor // the settle window on the newest of ANY of them, or a round completed by a // comment would settle against a stale timestamp and converge instantly. - evidenceBots := s.cfg.evidenceBots() + evidenceBots := cfg.evidenceBots() noteEvidence := func(at time.Time) { if at.After(report.LastEvidenceAt) { report.LastEvidenceAt = at @@ -155,8 +156,8 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe // head commit instead of reporting stale state as current. verdictCutoff = obs.eng.HeadAt } - report.CoReviewers = coReviewerStatuses(s.cfg, obs.eng, verdictCutoff) - if why := engine.PrimaryUnavailableReason(obs.eng, s.cfg.policy(), head); why != "" { + report.CoReviewers = coReviewerStatuses(cfg, obs.eng, verdictCutoff) + if why := engine.PrimaryUnavailableReason(obs.eng, cfg.policy(), head); why != "" { report.PrimaryUnavailable = true report.PrimaryUnavailableReason = s.cfg.Bot + " " + why } @@ -166,7 +167,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe // hang convergence if it were) still has its findings reported instead of // dropped. It always includes the required bots: a bot crq waits for whose // findings it didn't surface would hang the loop forever. - extractBots := s.cfg.evidenceBots() + extractBots := cfg.evidenceBots() // Review-body findings — CodeRabbit's detailed and "Prompt for AI agents" // blocks — carry no per-finding resolution state, only the review's commit. diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go new file mode 100644 index 0000000..22bae4b --- /dev/null +++ b/internal/crq/repoconfig.go @@ -0,0 +1,173 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" +) + +// ReviewerView is how one repository's reviewers read after its override is +// applied — the answer to "which bots run on this project". +type ReviewerView struct { + Repo string `json:"repo"` + // Overridden says whether this repository has its own configuration or is + // simply following the fleet default. + Overridden bool `json:"overridden"` + UpdatedAt string `json:"updated_at,omitempty"` + By string `json:"by,omitempty"` + Reviewers []ReviewerDetail `json:"reviewers"` +} + +// ReviewerDetail is one reviewer as it will actually be used. +type ReviewerDetail struct { + Login string `json:"login"` + // Budget is the only property the queue cares about: "account" is serialized + // against the shared allowance, "none" runs immediately. + Budget string `json:"budget"` + Required bool `json:"required"` + Trigger string `json:"trigger,omitempty"` +} + +// Reviewers reports the reviewers that will run on repo. +func (s *Service) Reviewers(ctx context.Context, repo string) (ReviewerView, error) { + st, _, err := s.store.Load(ctx) + if err != nil { + return ReviewerView{}, err + } + repo = NormalizeRepo(repo) + cfg := s.cfgFor(st, repo) + view := ReviewerView{Repo: repo, Reviewers: []ReviewerDetail{}} + if ov, ok := st.RepoOverride(repo); ok { + view.Overridden = true + view.By = ov.By + if ov.UpdatedAt != nil { + view.UpdatedAt = ov.UpdatedAt.UTC().Format("2006-01-02T15:04:05Z") + } + } + for _, r := range cfg.Reviewers { + view.Reviewers = append(view.Reviewers, ReviewerDetail{ + Login: r.Login, + Budget: string(r.Budget), + Required: r.Required, + Trigger: string(r.Trigger), + }) + } + return view, nil +} + +// SetReviewers records which co-reviewers run on repo and which of them gate +// convergence. A nil list means "leave that half alone"; an empty non-nil list +// means "none here", which is a different thing and has to survive as one. +// +// The primary is not settable. Its markers and command are injected into the +// dialect classifiers when the Service is built, so a per-repo primary would +// mean per-repo classifiers. +func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, required []string) (ReviewerView, error) { + repo = NormalizeRepo(repo) + if repo == "" || !strings.Contains(repo, "/") { + return ReviewerView{}, fmt.Errorf("repo must be owner/name, got %q", repo) + } + // Accept either spelling: the login (chatgpt-codex-connector[bot]) or the + // short config name (codex), which is what CRQ_COBOTS already takes. + known := map[string]string{} + for _, cb := range s.cfg.CoBots { + known[dialect.NormalizeBotName(cb.Login)] = cb.Login + if cb.Name != "" { + known[strings.ToLower(strings.TrimSpace(cb.Name))] = cb.Login + } + } + // Refuse a bot crq has no registry entry for: it could never be triggered or + // classified, so accepting it would record a configuration that silently + // does nothing. + resolve := func(known map[string]string, list []string, what string) ([]string, error) { + out := make([]string, 0, len(list)) + for _, name := range list { + name = strings.TrimSpace(name) + if name == "" { + continue + } + login, ok := known[dialect.NormalizeBotName(name)] + if !ok { + login, ok = known[strings.ToLower(strings.TrimSpace(name))] + } + if !ok { + return nil, fmt.Errorf("%s: unknown reviewer %q (known: %s)", what, name, strings.Join(knownLogins(known), ", ")) + } + out = append(out, login) + } + return out, nil + } + + ov := RepoReviewers{} + if coBots != nil { + resolved, err := resolve(known, coBots, "--bots") + if err != nil { + return ReviewerView{}, err + } + ov.CoBots, ov.SetCoBots = resolved, true + } + if required != nil { + // The primary may gate here even though it cannot be replaced here. + allowed := map[string]string{dialect.NormalizeBotName(s.cfg.Bot): s.cfg.Bot} + for k, v := range known { + allowed[k] = v + } + resolved, err := resolve(allowed, required, "--required") + if err != nil { + return ReviewerView{}, err + } + ov.Required, ov.SetRequired = resolved, true + } + now := s.clock().UTC() + ov.UpdatedAt = &now + ov.By = s.cfg.Host + + state, err := s.store.Update(ctx, func(st *State) error { + st.SetRepoOverride(repo, ov) + return nil + }) + if err != nil { + return ReviewerView{}, err + } + s.sync(ctx, state) + return s.Reviewers(ctx, repo) +} + +// ClearReviewers returns repo to the fleet default. +func (s *Service) ClearReviewers(ctx context.Context, repo string) (ReviewerView, error) { + repo = NormalizeRepo(repo) + state, err := s.store.Update(ctx, func(st *State) error { + if !st.ClearRepoOverride(repo) { + return ErrNoChange + } + return nil + }) + if err != nil && !errors.Is(err, ErrNoChange) { + return ReviewerView{}, err + } + if err == nil { + s.sync(ctx, state) + } + return s.Reviewers(ctx, repo) +} + +// knownLogins is the deduplicated set of accepted reviewers, for an error a +// caller can act on. The map holds each bot twice (login and short name), so it +// is the values that must be deduplicated, not the keys. +func knownLogins(known map[string]string) []string { + seen := map[string]bool{} + out := make([]string, 0, len(known)) + for _, login := range known { + if seen[login] { + continue + } + seen[login] = true + out = append(out, login) + } + sort.Strings(out) + return out +} diff --git a/internal/crq/repoconfig_test.go b/internal/crq/repoconfig_test.go new file mode 100644 index 0000000..ac36cd1 --- /dev/null +++ b/internal/crq/repoconfig_test.go @@ -0,0 +1,65 @@ +package crq + +import ( + "context" + "testing" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" +) + +// The override has to reach a real decision, not just the config value: a +// per-repo setting that every caller reads and no decision honours is worse +// than none, because it reads as configured. +func TestRepoOverrideReachesTheDecision(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.RequiredBots = []string{"coderabbitai[bot]", dialect.CodexBotLogin} + cfg.CoBots = codexCoBots(cfg.RequiredBots) + cfg.FeedbackBots = cfg.RequiredBots + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + // Fleet default: Codex gates. + if got := svc.cfgFor(st, "o/plain"); !containsBot(got.RequiredBots, dialect.CodexBotLogin) { + t.Fatalf("fleet RequiredBots = %v, want codex gating", got.RequiredBots) + } + + now := time.Now().UTC() + if _, err := store.Update(ctx, func(st *State) error { + st.SetRepoOverride("o/quiet", RepoReviewers{ + CoBots: []string{}, SetCoBots: true, + Required: []string{"coderabbitai[bot]"}, SetRequired: true, + UpdatedAt: &now, + }) + return nil + }); err != nil { + t.Fatal(err) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + + quiet := svc.cfgFor(st, "o/quiet") + if containsBot(quiet.RequiredBots, dialect.CodexBotLogin) { + t.Errorf("RequiredBots = %v, want codex not gating this repo", quiet.RequiredBots) + } + // The policy the engine actually decides from must carry it. + for _, cp := range quiet.policy().CoReviewerPolicies() { + if dialect.IsCodexBot(cp.Login) { + t.Errorf("policy still drives codex on o/quiet: %+v", cp) + } + } + if !containsBot(quiet.policy().RequiredBots, "coderabbitai[bot]") { + t.Errorf("policy RequiredBots = %v, want the primary still gating", quiet.policy().RequiredBots) + } + // And the other repository is untouched by its neighbour's choice. + if !containsBot(svc.cfgFor(st, "o/plain").RequiredBots, dialect.CodexBotLogin) { + t.Error("one repository's override must not change another's") + } +} diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index bcfe7df..886bb99 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -175,3 +175,54 @@ func silenceTrigger(coBots []CoBotConfig, login string) []CoBotConfig { func (c Config) evidenceBots() map[string]struct{} { return dialect.BotSet(unionBots(c.FeedbackBots, c.RequiredBots)) } + +// ForRepo applies a repository's reviewer override to the fleet configuration. +// +// The primary is deliberately NOT overridable. Its markers and command are +// injected into the dialect classifiers when the Service is constructed, so a +// per-repo primary would mean per-repo classifiers — a much larger change than +// "which co-reviewers run here", which is what the request actually is. +func (c Config) ForRepo(ov RepoReviewers) Config { + if !ov.SetCoBots && !ov.SetRequired { + return c + } + out := c + if ov.SetCoBots { + keep := make([]CoBotConfig, 0, len(c.CoBots)) + for _, cb := range c.CoBots { + if containsBot(ov.CoBots, cb.Login) { + keep = append(keep, cb) + } + } + out.CoBots = keep + } + if ov.SetRequired { + out.RequiredBots = append([]string(nil), ov.Required...) + } + // Rebuild the derived views from the overridden lists, exactly as + // LoadConfig does, so no view can answer differently from another. + out.Reviewers = buildReviewers(out.Bot, out.ReviewCommand, out.RequiredBots, out.CoBots) + out.RequiredBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required }) + out.FeedbackBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required || !r.Metered() }) + return out +} + +func containsBot(logins []string, login string) bool { + key := dialect.NormalizeBotName(login) + for _, candidate := range logins { + if dialect.NormalizeBotName(candidate) == key { + return true + } + } + return false +} + +// cfgFor is the configuration crq should use for one repository: the fleet +// default with that repository's override applied. +func (s *Service) cfgFor(st State, repo string) Config { + ov, ok := st.RepoOverride(repo) + if !ok { + return s.cfg + } + return s.cfg.ForRepo(ov) +} diff --git a/internal/crq/reviewers_test.go b/internal/crq/reviewers_test.go index 2f3e451..74b4173 100644 --- a/internal/crq/reviewers_test.go +++ b/internal/crq/reviewers_test.go @@ -225,3 +225,54 @@ func TestDerivationLosesNothing(t *testing.T) { } }) } + +// Per-repo reviewers answer the request crq exists to make easy: which bots run +// on which project. The override has to reach the DERIVED views too, or crq +// would still gate on, and surface findings from, a bot this repository excluded. +func TestForRepoOverridesEveryDerivedView(t *testing.T) { + // isolatedConfig blanks CRQ_COBOTS, so ask for the shipped default explicitly. + fleet := isolatedConfig(t, map[string]string{"CRQ_COBOTS": "codex,bugbot,macroscope"}) + if len(fleet.CoBots) < 2 { + t.Fatalf("this test needs several co-reviewers by default, got %d", len(fleet.CoBots)) + } + + t.Run("no override changes nothing", func(t *testing.T) { + if got := fleet.ForRepo(RepoReviewers{}); len(got.Reviewers) != len(fleet.Reviewers) { + t.Errorf("reviewers = %d, want the fleet's %d", len(got.Reviewers), len(fleet.Reviewers)) + } + }) + + t.Run("only the chosen co-reviewer runs", func(t *testing.T) { + only := fleet.ForRepo(RepoReviewers{ + CoBots: []string{dialect.CodexBotLogin}, SetCoBots: true, + Required: []string{dialect.CodexBotLogin}, SetRequired: true, + }) + if len(only.CoBots) != 1 || dialect.NormalizeBotName(only.CoBots[0].Login) != dialect.NormalizeBotName(dialect.CodexBotLogin) { + t.Fatalf("CoBots = %+v, want only codex", only.CoBots) + } + // The derived views must agree, or crq gates on a bot this repo excluded. + if len(only.RequiredBots) != 1 || dialect.NormalizeBotName(only.RequiredBots[0]) != dialect.NormalizeBotName(dialect.CodexBotLogin) { + t.Errorf("RequiredBots = %v, want only codex", only.RequiredBots) + } + for _, login := range only.FeedbackBots { + if dialect.NormalizeBotName(login) == "cursor" { + t.Errorf("FeedbackBots = %v still surfaces an excluded bot", only.FeedbackBots) + } + } + // The primary is fleet-wide and still configured, just not gating here. + primary, ok := only.Primary() + if !ok || primary.Login != fleet.Bot { + t.Errorf("primary = %+v ok=%v, want it kept", primary, ok) + } + }) + + t.Run("chosen to be none is not the same as unset", func(t *testing.T) { + none := fleet.ForRepo(RepoReviewers{CoBots: []string{}, SetCoBots: true}) + if len(none.CoBots) != 0 { + t.Errorf("CoBots = %+v, want none — an empty choice must survive", none.CoBots) + } + if _, ok := none.Primary(); !ok { + t.Error("excluding every co-reviewer must not remove the primary") + } + }) +} diff --git a/internal/crq/service.go b/internal/crq/service.go index 98886e2..e85309a 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -298,13 +298,14 @@ 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) + cfg := s.cfgFor(st, next.Repo) + obs, err := s.observe(ctx, cfg, next.Repo, next.PR, next, now) if err != nil { return PumpResult{}, err } global := s.global(st, now) - decision := engine.DecideFire(global, *next, obs.eng, now, s.cfg.policy()) - result, err := s.applyFire(ctx, *next, obs.eng, decision, now) + decision := engine.DecideFire(global, *next, obs.eng, now, cfg.policy()) + result, err := s.applyFire(ctx, cfg, *next, obs.eng, decision, now) if err != nil { return result, err } @@ -344,7 +345,6 @@ func (s *Service) sweepQuotaFree(ctx context.Context, st State, now time.Time, s if len(queued) == 0 { return PumpResult{}, false, nil } - policy := s.cfg.policy() global := s.global(st, now) scanned := 0 defer func() { s.scanOffset = (s.scanOffset + scanned + 1) % len(queued) }() @@ -363,15 +363,16 @@ 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) + cfg := s.cfgFor(st, round.Repo) + obs, err := s.observe(ctx, cfg, round.Repo, round.PR, &round, now) if err != nil { continue } - d := engine.DecideFire(global, round, obs.eng, now, policy) + d := engine.DecideFire(global, round, obs.eng, now, cfg.policy()) if !quotaFreeVerdict(d.Verdict) { continue } - res, err := s.applyFire(ctx, round, obs.eng, d, now) + res, err := s.applyFire(ctx, cfg, round, obs.eng, d, now) if err != nil { return PumpResult{}, false, err } @@ -399,15 +400,16 @@ 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) + cfg := s.cfgFor(st, repo) + obs, err := s.observe(ctx, cfg, repo, pr, round, now) if err != nil { return PumpResult{}, false, err } - d := engine.DecideFire(s.global(st, now), *round, obs.eng, now, s.cfg.policy()) + d := engine.DecideFire(s.global(st, now), *round, obs.eng, now, cfg.policy()) if !quotaFreeVerdict(d.Verdict) { return PumpResult{}, false, nil } - res, err := s.applyFire(ctx, *round, obs.eng, d, now) + res, err := s.applyFire(ctx, cfg, *round, obs.eng, d, now) if err != nil { return PumpResult{}, false, err } @@ -487,12 +489,13 @@ 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) + cfg := s.cfgFor(st, slot.Repo) + obs, err := s.observe(ctx, cfg, slot.Repo, slot.PR, &slot, now) if err != nil { return PumpResult{}, err } - s.selfHealCoReviewers(ctx, slot, obs.eng, now) - tr := engine.Progress(slot, st.Account, obs.eng, now, s.cfg.policy()) + s.selfHealCoReviewers(ctx, cfg, slot, obs.eng, now) + tr := engine.Progress(slot, st.Account, obs.eng, now, cfg.policy()) if tr.Outcome == engine.KeepWaiting { return PumpResult{Action: "waiting", Repo: slot.Repo, PR: slot.PR, Reason: tr.Reason}, nil } @@ -626,15 +629,16 @@ 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) + cfg := s.cfgFor(st, target.Repo) + obs, err := s.observe(ctx, cfg, target.Repo, target.PR, target, 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) } return st, nil } - s.selfHealCoReviewers(ctx, *target, obs.eng, now) - tr := engine.Progress(*target, st.Account, obs.eng, now, s.cfg.policy()) + s.selfHealCoReviewers(ctx, cfg, *target, obs.eng, now) + tr := engine.Progress(*target, st.Account, obs.eng, now, cfg.policy()) if tr.Outcome == engine.KeepWaiting { return st, nil } @@ -663,24 +667,24 @@ func firedOrEnqueuedAt(r Round) time.Time { } // applyFire executes a DecideFire verdict. -func (s *Service) applyFire(ctx context.Context, round Round, obs engine.Observation, d engine.FireDecision, now time.Time) (PumpResult, error) { +func (s *Service) applyFire(ctx context.Context, cfg Config, round Round, obs engine.Observation, d engine.FireDecision, now time.Time) (PumpResult, error) { switch d.Verdict { case engine.FireDrop: return s.abandonRound(ctx, round, "pr closed", "skipped") case engine.FireDedupe: return s.dedupeRound(ctx, round, now, d.Reason) case engine.FireCoOnly: - return s.fireCoOnly(ctx, round, d.PostCo, d.Reason, now) + return s.fireCoOnly(ctx, cfg, round, d.PostCo, d.Reason, now) case engine.FireCoDeferred: - return s.fireCoDeferred(ctx, round, d, now) + return s.fireCoDeferred(ctx, cfg, round, d, now) case engine.FireCoReviewWait: - return s.fireCoReviewWait(ctx, round, obs, d.Reason, now) + return s.fireCoReviewWait(ctx, cfg, round, obs, d.Reason, now) case engine.FireSupersede: return s.supersedeRound(ctx, round, obs.Head, now) case engine.FireAdopt: - return s.fireRound(ctx, round, obs, false, d.AdoptCommandID, d.AdoptAt, d.Reason, d.PostCo, now) + return s.fireRound(ctx, cfg, round, obs, false, d.AdoptCommandID, d.AdoptAt, d.Reason, d.PostCo, now) case engine.FirePost: - return s.fireRound(ctx, round, obs, true, 0, time.Time{}, "", d.PostCo, now) + return s.fireRound(ctx, cfg, round, obs, true, 0, time.Time{}, "", d.PostCo, now) default: // FireNo return PumpResult{Action: mapFireNo(d.Reason), Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: d.Reason}, nil } @@ -786,7 +790,7 @@ func (s *Service) supersedeRound(ctx context.Context, round Round, head string, // round, reserving the global slot under compare-and-swap. postCo lists the // co-reviewer logins whose trigger commands are posted alongside (non-fatal // on failure — the self-heal path retries). -func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observation, post bool, adoptID int64, adoptAt time.Time, reason string, postCo []string, now time.Time) (PumpResult, error) { +func (s *Service) fireRound(ctx context.Context, cfg Config, round Round, obs engine.Observation, post bool, adoptID int64, adoptAt time.Time, reason string, postCo []string, now time.Time) (PumpResult, error) { key := QueueKey(round.Repo, round.PR) if s.cfg.DryRun { return PumpResult{Action: "dry_run", Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: reason}, nil @@ -834,7 +838,7 @@ func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observa // posting a duplicate; recording it here keeps the round "asked". Its // timestamp anchors that bot's cutoff too, or a SHA-less answer that // landed before this adopted fire would never bind to the round. - for _, cp := range s.cfg.policy().CoReviewerPolicies() { + for _, cp := range cfg.policy().CoReviewerPolicies() { if hasLogin(postCo, cp.Login) || r.Co(cp.Login).CommandID != 0 { continue } @@ -858,7 +862,7 @@ func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observa s.log.Printf("fire %s@%s (adopted existing review command)", key, round.Head) } for _, login := range postCo { - s.fireCoTrigger(ctx, round, login) + s.fireCoTrigger(ctx, cfg, round, login) } return PumpResult{Action: "fired", Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: reason}, nil } @@ -919,7 +923,7 @@ func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observa // retries. var coPosts []coPost for _, login := range postCo { - if id, at := s.postCoTrigger(ctx, round, login); id != 0 { + if id, at := s.postCoTrigger(ctx, cfg, round, login); id != 0 { coPosts = append(coPosts, coPost{login: login, id: id, at: at}) } } @@ -971,7 +975,7 @@ func (c Config) coCommandFor(login string) string { // no CodeRabbit quota is spent, and therefore NO FireSlot is taken: the // per-round trigger claims (CoBots[login].ClaimedAt, CAS-set before the // network post) are the concurrency guard. -func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, reason string, now time.Time) (PumpResult, error) { +func (s *Service) fireCoOnly(ctx context.Context, cfg Config, round Round, logins []string, reason string, now time.Time) (PumpResult, error) { key := QueueKey(round.Repo, round.PR) if s.cfg.DryRun { return PumpResult{Action: "dry_run", Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: reason}, nil @@ -1010,7 +1014,7 @@ func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, var posts []coPost for _, login := range claimed { - if id, at := s.postCoTrigger(ctx, round, login); id != 0 { + if id, at := s.postCoTrigger(ctx, cfg, round, login); id != 0 { posts = append(posts, coPost{login: login, id: id, at: at}) } } @@ -1123,7 +1127,7 @@ func commandCreatedAt(commands []engine.CommandSeen, id int64, fallback time.Tim // review` command on the PR is adopted as the round's CodexCommandID so the // self-heal path (which anchors on the round's fire time, later than a pre-existing // command) does not re-post it. -func (s *Service) fireCoReviewWait(ctx context.Context, round Round, obs engine.Observation, reason string, now time.Time) (PumpResult, error) { +func (s *Service) fireCoReviewWait(ctx context.Context, cfg Config, round Round, obs engine.Observation, reason string, now time.Time) (PumpResult, error) { result := PumpResult{Action: "waiting", Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: reason} if s.cfg.DryRun { return result, nil @@ -1151,7 +1155,7 @@ func (s *Service) fireCoReviewWait(ctx context.Context, round Round, obs engine. at time.Time } var adopts []adoptCmd - for _, cp := range s.cfg.policy().CoReviewerPolicies() { + for _, cp := range cfg.policy().CoReviewerPolicies() { cmds := obs.CoSeenFor(cp.Login).Commands id := newestCommandID(cmds) if id == 0 { @@ -1254,8 +1258,8 @@ func (s *Service) recordFire(ctx context.Context, round Round, token string, com // 0 on failure. A failed post is non-fatal: it logs and leaves the round's // command unset so a later pump's self-heal retries. The fresh-fire path // folds the returned id into recordFire's write. -func (s *Service) postCoTrigger(ctx context.Context, round Round, login string) (int64, time.Time) { - command := strings.TrimSpace(s.cfg.coCommandFor(login)) +func (s *Service) postCoTrigger(ctx context.Context, cfg Config, round Round, login string) (int64, time.Time) { + command := strings.TrimSpace(cfg.coCommandFor(login)) if command == "" { return 0, time.Time{} } @@ -1284,8 +1288,8 @@ func (s *Service) postCoTrigger(ctx context.Context, round Round, login string) // self-heal retry (the fresh-post path records the id inside recordFire // instead). The CAS guard (same head, command still unset) makes a concurrent // post benign. -func (s *Service) fireCoTrigger(ctx context.Context, round Round, login string) { - id, at := s.postCoTrigger(ctx, round, login) +func (s *Service) fireCoTrigger(ctx context.Context, cfg Config, round Round, login string) { + id, at := s.postCoTrigger(ctx, cfg, round, login) if id == 0 { // Failed post: KEEP the claim — its TTL is the retry backoff. Clearing it // here would let the very next pump repost, bypassing triggerClaimTTL. @@ -1322,7 +1326,7 @@ func (s *Service) fireCoTrigger(ctx context.Context, round Round, login string) // normally the moment the window opens, at which point the recorded command // ids keep the triggers from re-posting. The claim-then-post shape mirrors // selfHealCoReviewers — this path is not serialized by the fire slot either. -func (s *Service) fireCoDeferred(ctx context.Context, round Round, d engine.FireDecision, now time.Time) (PumpResult, error) { +func (s *Service) fireCoDeferred(ctx context.Context, cfg Config, round Round, d engine.FireDecision, now time.Time) (PumpResult, error) { action := func(base string) string { // Preserve the historical action names for the Codex-only case. if len(d.PostCo) <= 1 && len(d.AdoptCo) <= 1 { @@ -1403,7 +1407,7 @@ func (s *Service) fireCoDeferred(ctx context.Context, round Round, d engine.Fire } s.sync(ctx, updated) for _, login := range claimed { - s.fireCoTrigger(ctx, round, login) + s.fireCoTrigger(ctx, cfg, round, login) } if len(claimed) == 0 { result.Action = action("adopted") @@ -1418,12 +1422,12 @@ func (s *Service) fireCoDeferred(ctx context.Context, round Round, d engine.Fire // idempotence comes from the observation — the bot's evidence, a live trigger // command, or an account that reviews on its own all suppress it (see // DecideCoPost) — not a retry counter. -func (s *Service) selfHealCoReviewers(ctx context.Context, round Round, obs engine.Observation, now time.Time) { +func (s *Service) selfHealCoReviewers(ctx context.Context, cfg Config, round Round, obs engine.Observation, now time.Time) { if s.cfg.DryRun || round.FiredAt == nil || obs.Head != round.Head { return } firedAt := round.FiredAt.UTC() - for _, cp := range s.cfg.policy().CoReviewerPolicies() { + for _, cp := range cfg.policy().CoReviewerPolicies() { if round.Co(cp.Login).CommandID != 0 || (dialect.IsCodexBot(cp.Login) && round.CodexCommandID != 0) { continue } @@ -1455,7 +1459,7 @@ func (s *Service) selfHealCoReviewers(ctx context.Context, round Round, obs engi continue } s.sync(ctx, updated) - s.fireCoTrigger(ctx, round, login) + s.fireCoTrigger(ctx, cfg, round, login) } } diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 59aba22..22474c4 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -2361,7 +2361,7 @@ func TestFireCoDeferredAdoptionHonorsDryRunAndActiveClaim(t *testing.T) { round := *st.Round("o/carrier", 95) svc.cfg.DryRun = true - res, err := svc.fireCoDeferred(ctx, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "dry run adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, now) + res, err := svc.fireCoDeferred(ctx, svc.cfg, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "dry run adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, now) if err != nil { t.Fatal(err) } @@ -2382,7 +2382,7 @@ func TestFireCoDeferredAdoptionHonorsDryRunAndActiveClaim(t *testing.T) { }); err != nil { t.Fatal(err) } - res, err = svc.fireCoDeferred(ctx, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "claimed adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, now) + res, err = svc.fireCoDeferred(ctx, svc.cfg, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "claimed adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, now) if err != nil { t.Fatal(err) } @@ -2395,7 +2395,7 @@ func TestFireCoDeferredAdoptionHonorsDryRunAndActiveClaim(t *testing.T) { } staleNow := now.Add(triggerClaimTTL + time.Second) - res, err = svc.fireCoDeferred(ctx, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "stale claim adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, staleNow) + res, err = svc.fireCoDeferred(ctx, svc.cfg, round, engine.FireDecision{Verdict: engine.FireCoDeferred, Reason: "stale claim adopt", AdoptCo: map[string]engine.CommandSeen{dialect.CodexBotLogin: {ID: 703, CreatedAt: now.Add(-time.Minute)}}}, staleNow) if err != nil { t.Fatal(err) } diff --git a/internal/crq/state.go b/internal/crq/state.go index a9da1d2..bd82577 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -23,6 +23,8 @@ type ( Revision = crqstate.Revision StateStore = crqstate.StateStore StoreConfig = crqstate.StoreConfig + // RepoReviewers is the per-repository reviewer override (see Config.ForRepo). + RepoReviewers = crqstate.RepoReviewers ) const ( diff --git a/internal/state/state.go b/internal/state/state.go index ede4e85..56b8a8c 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -285,6 +285,17 @@ type State struct { // one. Persisted in the shared state so the whole fleet uses the new issue. CalibrationIssue int `json:"calibration_issue,omitempty"` + // Repos holds per-repository reviewer overrides, keyed by normalized + // "owner/name". + // + // It lives HERE, in the shared state ref, rather than in a .crq.yaml the + // repository carries. The daemon has no checkout of any repository it + // reviews, so an in-repo file would be invisible to it or cost a REST fetch + // per PR — and a daemon and an agent reading different configurations while + // writing one shared state ref is a new class of divergence. Both already + // read this ref, so both cannot disagree about it. + Repos map[string]RepoReviewers `json:"repos,omitempty"` + // Archive keeps recently finished rounds (superseded, closed, cancelled) // for the dashboard and debugging. Bounded by ArchiveMax. Archive []Round `json:"archive,omitempty"` @@ -298,6 +309,51 @@ type State struct { unknown unknownFields } +// RepoReviewers overrides which reviewers run on one repository. A nil slice +// means "no override, use the fleet default"; an empty non-nil slice means +// "none here" — the difference is why these are pointers to slices in JSON +// terms and why SetRepoReviewers takes explicit values. +type RepoReviewers struct { + // CoBots are the co-reviewer logins enabled here, replacing the fleet list. + CoBots []string `json:"cobots,omitempty"` + // Required are the logins that gate convergence here. + Required []string `json:"required,omitempty"` + // SetCoBots/SetRequired record whether each list was set at all, so + // "explicitly none" survives a JSON round trip that drops empty slices. + SetCoBots bool `json:"set_cobots,omitempty"` + SetRequired bool `json:"set_required,omitempty"` + UpdatedAt *time.Time `json:"updated_at,omitempty"` + By string `json:"by,omitempty"` +} + +// RepoOverride returns the override for repo, and whether one exists. +func (s *State) RepoOverride(repo string) (RepoReviewers, bool) { + ov, ok := s.Repos[normalizeRepoKey(repo)] + return ov, ok +} + +// SetRepoOverride records repo's reviewer override, replacing any earlier one. +func (s *State) SetRepoOverride(repo string, ov RepoReviewers) { + if s.Repos == nil { + s.Repos = map[string]RepoReviewers{} + } + s.Repos[normalizeRepoKey(repo)] = ov +} + +// ClearRepoOverride drops repo's override, returning it to the fleet default. +func (s *State) ClearRepoOverride(repo string) bool { + key := normalizeRepoKey(repo) + if _, ok := s.Repos[key]; !ok { + return false + } + delete(s.Repos, key) + return true +} + +func normalizeRepoKey(repo string) string { + return strings.ToLower(strings.TrimSuffix(strings.TrimSpace(repo), ".git")) +} + const SchemaVersion = 3 // ArchiveMax bounds the finished-rounds ring. Active rounds are never diff --git a/llms.txt b/llms.txt index ad5de80..0264233 100644 --- a/llms.txt +++ b/llms.txt @@ -202,6 +202,18 @@ 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. +Choose which bots review one project: + +```bash +crq reviewers REPO # effective reviewers + budgets +crq reviewers set REPO --bots codex --required codex # only Codex here +crq reviewers clear REPO # fleet default again +``` + +`budget: account` means the reviewer is serialized against the shared CodeRabbit allowance; +`budget: none` means it runs immediately. Stored in the shared state ref, so the daemon and agents +cannot disagree. The primary reviewer is fleet-wide. + Current feedback without triggering a review: ```bash diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index ef9dae2..b4609e1 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. +## Which Bots Review Which Project + +```bash +crq reviewers "$REPO" # who runs here, and what each costs +crq reviewers set "$REPO" --bots codex --required codex # only Codex here +crq reviewers clear "$REPO" # back to the fleet default +``` + +Each reviewer reports its `budget`: `account` is serialized against the shared CodeRabbit allowance, +`none` runs immediately and waits for nobody. That is the only property the queue cares about. + +The setting lives in the shared state ref, so the daemon and every agent read the same one. The +primary reviewer is fleet-wide and cannot be set per repository. + ## Fleet Auto-Review To keep all open PRs in scope reviewed while CodeRabbit native auto-review is off: From a81050a470f31f7160cfda04eefbea1061ab3a1b Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:08:39 +0200 Subject: [PATCH 2/5] Let a project add a reviewer, not only remove one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight review findings, two of them P1. Choices resolved against the fleet's enabled list, so the feature could only ever subtract: with CRQ_COBOTS=codex, asking a project for bugbot was rejected as unknown though it is a registered reviewer. They resolve against the registry now, and a bot the fleet does not enable is built from its registry entry. Naming a bot in --required enables it too, the rule fleet parsing already follows — one that gates but is never triggered waits forever. A primary that is itself a registry bot keeps its silenced entry through an override. That entry carries its wording and check-run hooks, so dropping it costs the PRIMARY its evidence and the round waits out a clean result crq can no longer read. Same defect as the fleet path had, one layer up. A second call naming one half no longer wipes the other: the existing override is loaded and merged, which is what the nil-versus-empty contract promised. An empty --required is refused, because gating on nobody makes Feedback converge before any chosen reviewer runs, and clear takes no mutation flags — ignoring them turned a malformed call into a silent wipe. The Codex-only degrade is gated on Codex being a reviewer here, or an unsolicited auto-review would release the head during an account block on behalf of a bot the project excluded. And sharing a ref only stops an old binary ERASING the field; it does not make it act on one. State now records which hosts write it and what they understand, and `crq reviewers` names any host driving the queue that predates overrides, instead of implying agreement. --- cmd/crq/main.go | 6 +++ internal/crq/config.go | 44 ++++++++++++++------- internal/crq/feedback.go | 8 +++- internal/crq/repoconfig.go | 53 +++++++++++++++++-------- internal/crq/repoconfig_test.go | 62 +++++++++++++++++++++++++++++ internal/crq/reviewers.go | 43 +++++++++++++++++++- internal/crq/state.go | 4 ++ internal/state/state.go | 67 ++++++++++++++++++++++++++++++++ internal/state/store.go | 4 ++ internal/state/writers_test.go | 44 +++++++++++++++++++++ skills/coderabbit-queue/SKILL.md | 6 ++- 11 files changed, 307 insertions(+), 34 deletions(-) create mode 100644 internal/state/writers_test.go diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 6f80808..6fe5c69 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -780,6 +780,12 @@ func runReviewers(ctx context.Context, service *crq.Service, args []string) int var err error switch action { case "clear": + // Ignoring a mutation flag here would turn a malformed automation call + // like `reviewers clear repo --bots codex` into a silent wipe. + if bots != nil || required != nil { + fatal(errors.New("clear takes no --bots/--required (it drops the whole override)")) + return 1 + } view, err = service.ClearReviewers(ctx, repo) case "set": if bots == nil && required == nil { diff --git a/internal/crq/config.go b/internal/crq/config.go index 98d6cdf..e505d70 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -359,20 +359,18 @@ func parseCoBots(env map[string]string, requiredBots []string) ([]CoBotConfig, e if !enabled[co.Name] && !required { continue } + base := defaultCoBot(co, required) // Uniform across bots: the per-bot key wins, then the registry's legacy // alias if it declares one, then its default command. No bot is named // here — the policy travels as registry metadata. - command := co.Command + command := base.Command if v, ok := env["CRQ_COBOT_"+key+"_CMD"]; ok { command = v } else if co.LegacyCommandEnv != "" { command = stringEnvAllowEmpty(env, co.LegacyCommandEnv, command) } command = strings.TrimSpace(command) - trigger := triggerMode(co.DefaultTrigger, engine.TriggerSelfHeal) - if required && co.RequiredTrigger != "" { - trigger = triggerMode(co.RequiredTrigger, trigger) - } + trigger := base.Trigger switch v := engine.TriggerMode(strings.ToLower(strings.TrimSpace(env["CRQ_COBOT_"+key+"_TRIGGER"]))); v { case engine.TriggerNever, engine.TriggerSelfHeal, engine.TriggerAlways: trigger = v @@ -381,18 +379,38 @@ func parseCoBots(env map[string]string, requiredBots []string) ([]CoBotConfig, e // No trigger command means crq can never post one, whatever the mode. trigger = engine.TriggerNever } - out = append(out, CoBotConfig{ - Login: co.Login, - Name: co.Name, - Command: command, - Trigger: trigger, - Required: required, - SelfHealGrace: durationEnv(env, "CRQ_COBOT_"+key+"_GRACE", 10*time.Minute), - }) + base.Command, base.Trigger = command, trigger + base.SelfHealGrace = durationEnv(env, "CRQ_COBOT_"+key+"_GRACE", defaultSelfHealGrace) + out = append(out, base) } return out, nil } +const defaultSelfHealGrace = 10 * time.Minute + +// defaultCoBot is a co-reviewer's configuration from the registry alone, with no +// environment applied. parseCoBots layers env on top of it, and a per-repo +// override uses it directly for a bot the fleet default does not enable — which +// is the whole point of choosing reviewers per project. +func defaultCoBot(co dialect.CoReviewer, required bool) CoBotConfig { + trigger := triggerMode(co.DefaultTrigger, engine.TriggerSelfHeal) + if required && co.RequiredTrigger != "" { + trigger = triggerMode(co.RequiredTrigger, trigger) + } + command := strings.TrimSpace(co.Command) + if command == "" { + trigger = engine.TriggerNever + } + return CoBotConfig{ + Login: co.Login, + Name: co.Name, + Command: command, + Trigger: trigger, + Required: required, + SelfHealGrace: defaultSelfHealGrace, + } +} + // triggerMode converts a registry trigger string to the engine mode, falling // back when a bot declares none. func triggerMode(name string, fallback engine.TriggerMode) engine.TriggerMode { diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index b10509b..c795892 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -446,7 +446,11 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe // quota block qualifies — a round's own awaiting_retry cooldown also // covers non-quota retries (post failures, timeouts) that must keep their // normal retry handling. - if s.cfg.RateLimitCoDegrade && !report.Converged && st.Account.BlockedUntil != nil { + // Gated on Codex being a reviewer HERE. The degrade releases the head on + // Codex's word while the primary's window is shut; on a repository that + // excluded Codex, an unsolicited auto-review would otherwise stand in for a + // bot nobody asked for. + if cfg.RateLimitCoDegrade && cfg.coBotEnabled(dialect.CodexBotLogin) && !report.Converged && st.Account.BlockedUntil != nil { until := st.Account.BlockedUntil.UTC() if until.After(now) && engine.CodexOnlyEligible(completionRound, obs.eng, &until, now) { report.CodeRabbitDeferred = true @@ -457,7 +461,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe case report.Converged: report.Status = "converged" case report.CodeRabbitDeferred && len(report.Findings) == 0 && - engine.DoneExceptWithEvidence(report.ReviewedBy, s.cfg.Bot, dialect.CodexBotLogin): + engine.DoneExceptWithEvidence(report.ReviewedBy, cfg.Bot, dialect.CodexBotLogin): report.Status = "deferred" report.Reason = "codex reviewed clean; coderabbit review deferred until " + report.DeferredUntil.UTC().Format(time.RFC3339) + " (account rate-limited)" diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go index 22bae4b..fb17c7e 100644 --- a/internal/crq/repoconfig.go +++ b/internal/crq/repoconfig.go @@ -16,10 +16,15 @@ type ReviewerView struct { Repo string `json:"repo"` // Overridden says whether this repository has its own configuration or is // simply following the fleet default. - Overridden bool `json:"overridden"` - UpdatedAt string `json:"updated_at,omitempty"` - By string `json:"by,omitempty"` - Reviewers []ReviewerDetail `json:"reviewers"` + Overridden bool `json:"overridden"` + // Lagging names hosts that are driving this queue without understanding + // per-repo overrides — an older binary loads the field, writes it back + // untouched, and keeps deciding from its own fleet-wide configuration. The + // override is real; those hosts will not honour it until they are upgraded. + Lagging []string `json:"lagging_hosts,omitempty"` + UpdatedAt string `json:"updated_at,omitempty"` + By string `json:"by,omitempty"` + Reviewers []ReviewerDetail `json:"reviewers"` } // ReviewerDetail is one reviewer as it will actually be used. @@ -41,6 +46,7 @@ func (s *Service) Reviewers(ctx context.Context, repo string) (ReviewerView, err repo = NormalizeRepo(repo) cfg := s.cfgFor(st, repo) view := ReviewerView{Repo: repo, Reviewers: []ReviewerDetail{}} + view.Lagging = st.LaggingWriters(CapsRepoOverrides, s.clock().UTC()) if ov, ok := st.RepoOverride(repo); ok { view.Overridden = true view.By = ov.By @@ -73,36 +79,45 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require } // Accept either spelling: the login (chatgpt-codex-connector[bot]) or the // short config name (codex), which is what CRQ_COBOTS already takes. + // Resolve against the REGISTRY, not the fleet's enabled list. Restricting a + // project to bots the fleet already runs would make this feature only ever + // subtract, when the point is choosing different reviewers per project. + // Either spelling works: the login, or the short name CRQ_COBOTS takes. known := map[string]string{} - for _, cb := range s.cfg.CoBots { - known[dialect.NormalizeBotName(cb.Login)] = cb.Login - if cb.Name != "" { - known[strings.ToLower(strings.TrimSpace(cb.Name))] = cb.Login - } + for _, co := range dialect.KnownCoReviewers() { + known[dialect.NormalizeBotName(co.Login)] = co.Login + known[strings.ToLower(strings.TrimSpace(co.Name))] = co.Login } - // Refuse a bot crq has no registry entry for: it could never be triggered or - // classified, so accepting it would record a configuration that silently - // does nothing. - resolve := func(known map[string]string, list []string, what string) ([]string, error) { + resolve := func(allowed map[string]string, list []string, what string) ([]string, error) { out := make([]string, 0, len(list)) for _, name := range list { name = strings.TrimSpace(name) if name == "" { continue } - login, ok := known[dialect.NormalizeBotName(name)] + login, ok := allowed[dialect.NormalizeBotName(name)] if !ok { - login, ok = known[strings.ToLower(strings.TrimSpace(name))] + login, ok = allowed[strings.ToLower(name)] } if !ok { - return nil, fmt.Errorf("%s: unknown reviewer %q (known: %s)", what, name, strings.Join(knownLogins(known), ", ")) + return nil, fmt.Errorf("%s: unknown reviewer %q (known: %s)", what, name, strings.Join(knownLogins(allowed), ", ")) } out = append(out, login) } return out, nil } + // Start from the existing override so a call that names one half leaves the + // other alone — the contract the nil/empty distinction promises. ov := RepoReviewers{} + if st, _, err := s.store.Load(ctx); err == nil { + if existing, ok := st.RepoOverride(repo); ok { + ov = existing + } + } else { + return ReviewerView{}, err + } + if coBots != nil { resolved, err := resolve(known, coBots, "--bots") if err != nil { @@ -111,6 +126,12 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require ov.CoBots, ov.SetCoBots = resolved, true } if required != nil { + if len(required) == 0 { + // Gating on nobody means Feedback reports converged before anything + // runs, so `crq next` says done and the reviewers chosen by --bots + // never review at all. + return ReviewerView{}, errors.New("--required cannot be empty: a round that gates on nobody converges before any reviewer runs (crq reviewers clear to drop the override)") + } // The primary may gate here even though it cannot be replaced here. allowed := map[string]string{dialect.NormalizeBotName(s.cfg.Bot): s.cfg.Bot} for k, v := range known { diff --git a/internal/crq/repoconfig_test.go b/internal/crq/repoconfig_test.go index ac36cd1..580164f 100644 --- a/internal/crq/repoconfig_test.go +++ b/internal/crq/repoconfig_test.go @@ -63,3 +63,65 @@ func TestRepoOverrideReachesTheDecision(t *testing.T) { t.Error("one repository's override must not change another's") } } + +// The override must be able to ADD a reviewer, not only subtract one. Resolving +// choices against the fleet's enabled list made "which bots for which project" +// a one-way filter: with CRQ_COBOTS=codex, asking for bugbot was rejected as +// unknown even though it is a registered reviewer. +func TestOverrideCanEnableABotTheFleetDoesNot(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{"CRQ_COBOTS": "codex"}) + if len(fleet.CoBots) != 1 { + t.Fatalf("fleet CoBots = %+v, want only codex", fleet.CoBots) + } + + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"bugbot"}, SetCoBots: true, + Required: []string{"bugbot"}, SetRequired: true, + }) + found := false + for _, cb := range got.CoBots { + if cb.Name == "bugbot" { + found = true + if cb.Command == "" { + t.Error("an enabled bot with no trigger command can never be asked to review") + } + } + } + if !found { + t.Fatalf("CoBots = %+v, want bugbot enabled from the registry", got.CoBots) + } + // Required implies enabled, as the fleet parse already does — a bot that + // gates but is never triggered waits forever. + only := fleet.ForRepo(RepoReviewers{ + CoBots: []string{}, SetCoBots: true, + Required: []string{"macroscope"}, SetRequired: true, + }) + if len(only.CoBots) != 1 || only.CoBots[0].Name != "macroscope" { + t.Errorf("CoBots = %+v, want the required bot enabled too", only.CoBots) + } +} + +// A primary that is itself a registry bot keeps its silenced entry through an +// override: that entry is where its wording and check-run hooks come from, so +// dropping it costs the PRIMARY its evidence and the round waits for a bot +// whose clean result crq can no longer read. +func TestOverrideKeepsTheSilencedPrimaryEntry(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{ + // Bugbot's login is cursor[bot]; naming that as the primary is what makes + // the primary a registry bot. + "CRQ_BOT": "cursor[bot]", + "CRQ_COBOTS": "codex,bugbot", + "CRQ_REVIEW_CMD": "bugbot run", + }) + primaryPresent := func(cfg Config, when string) { + t.Helper() + for _, cb := range cfg.CoBots { + if sameBot(cb.Login, cfg.Bot) { + return + } + } + t.Errorf("%s: CoBots = %+v lost the primary's registry entry", when, cfg.CoBots) + } + primaryPresent(fleet, "fleet default") + primaryPresent(fleet.ForRepo(RepoReviewers{CoBots: []string{"codex"}, SetCoBots: true}), "override naming only codex") +} diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index 886bb99..0fe3c9d 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -188,10 +188,44 @@ func (c Config) ForRepo(ov RepoReviewers) Config { } out := c if ov.SetCoBots { - keep := make([]CoBotConfig, 0, len(c.CoBots)) + // Required implies enabled, the same rule the fleet parse follows: a bot + // that gates but is never triggered waits forever. + wanted := append([]string(nil), ov.CoBots...) + if ov.SetRequired { + for _, login := range ov.Required { + if !containsBot(wanted, login) && !sameBot(login, c.Bot) { + wanted = append(wanted, login) + } + } + } + keep := make([]CoBotConfig, 0, len(wanted)+1) + have := map[string]bool{} for _, cb := range c.CoBots { - if containsBot(ov.CoBots, cb.Login) { + // A primary that is itself a registry bot keeps its silenced entry + // whatever the override says: that entry is where its wording and + // check-run hooks come from, and dropping it costs the primary its + // evidence (see LoadConfig). + if sameBot(cb.Login, c.Bot) { keep = append(keep, cb) + have[dialect.NormalizeBotName(cb.Login)] = true + continue + } + if containsBot(wanted, cb.Login) { + keep = append(keep, cb) + have[dialect.NormalizeBotName(cb.Login)] = true + } + } + // A repository may choose a bot the fleet default does not enable — + // otherwise "which bots for which project" only ever subtracts. Its + // configuration comes from the registry, since there is no per-bot + // environment for a repo. + for _, login := range wanted { + if have[dialect.NormalizeBotName(login)] { + continue + } + if co, ok := dialect.CoReviewerByName(login); ok { + keep = append(keep, defaultCoBot(co, containsBot(ov.Required, login))) + have[dialect.NormalizeBotName(login)] = true } } out.CoBots = keep @@ -207,6 +241,11 @@ func (c Config) ForRepo(ov RepoReviewers) Config { return out } +// sameBot reports whether two spellings name the same reviewer. +func sameBot(a, b string) bool { + return dialect.NormalizeBotName(a) == dialect.NormalizeBotName(b) +} + func containsBot(logins []string, login string) bool { key := dialect.NormalizeBotName(login) for _, candidate := range logins { diff --git a/internal/crq/state.go b/internal/crq/state.go index bd82577..8a19aac 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -35,6 +35,9 @@ const ( PhaseAwaitingRetry = crqstate.PhaseAwaitingRetry PhaseCompleted = crqstate.PhaseCompleted PhaseAbandoned = crqstate.PhaseAbandoned + + // CapsRepoOverrides is the binary capability per-repo reviewer overrides need. + CapsRepoOverrides = crqstate.CapsRepoOverrides ) var ( @@ -51,6 +54,7 @@ func (c Config) storeConfig() StoreConfig { Timezone: c.Timezone, Scope: c.Scope, CoReviewers: c.coReviewerSummary(), + Host: c.Host, MinInterval: c.MinInterval, } } diff --git a/internal/state/state.go b/internal/state/state.go index 56b8a8c..f188549 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -285,6 +285,12 @@ type State struct { // one. Persisted in the shared state so the whole fleet uses the new issue. CalibrationIssue int `json:"calibration_issue,omitempty"` + // Writers records which hosts have written this state and what they can do, + // so a feature that only SOME binaries understand can say so instead of + // pretending agreement. Sharing a ref stops an old binary erasing a new + // field; it does not make that binary act on it. + Writers map[string]WriterSeen `json:"writers,omitempty"` + // Repos holds per-repository reviewer overrides, keyed by normalized // "owner/name". // @@ -309,6 +315,67 @@ type State struct { unknown unknownFields } +// WriterCaps is what THIS binary understands. Bump it when a state field starts +// changing decisions, so a fleet running two versions can tell. +const WriterCaps = 1 + +// CapsRepoOverrides is the capability that makes per-repository reviewer +// overrides safe to act on. +const CapsRepoOverrides = 1 + +// writerTTL is how long a host counts as still active for capability purposes. +const writerTTL = 30 * time.Minute + +// WriterSeen is one host's last write. +type WriterSeen struct { + Caps int `json:"caps"` + At time.Time `json:"at"` +} + +// NoteWriter records that host wrote this state with the given capabilities. +func (s *State) NoteWriter(host string, caps int, now time.Time) { + if host == "" { + return + } + if s.Writers == nil { + s.Writers = map[string]WriterSeen{} + } + s.Writers[host] = WriterSeen{Caps: caps, At: now.UTC()} + // Bounded: a host that has not written in a day is not part of the fleet's + // current behaviour, and the state ref is not an audit log. + for name, seen := range s.Writers { + if now.Sub(seen.At) > 24*time.Hour { + delete(s.Writers, name) + } + } +} + +// LaggingWriters names the hosts that are DRIVING this queue — holding the +// leader lease or the fire slot — without having announced the capability +// needed for caps. +// +// It answers the question a shared config field cannot: "will everyone who acts +// on this actually honour it?" An old binary loads an unknown field, writes it +// back untouched, and keeps deciding from its own fleet-wide configuration. +func (s *State) LaggingWriters(caps int, now time.Time) []string { + acting := map[string]bool{} + if s.Leader != nil && s.Leader.Owner != "" && s.Leader.ExpiresAt.After(now) { + acting[s.Leader.Owner] = true + } + if slot := s.SlotRound(); slot != nil && slot.ByHost != "" { + acting[slot.ByHost] = true + } + var out []string + for host := range acting { + if seen, ok := s.Writers[host]; ok && seen.Caps >= caps && now.Sub(seen.At) <= writerTTL { + continue + } + out = append(out, host) + } + sort.Strings(out) + return out +} + // RepoReviewers overrides which reviewers run on one repository. A nil slice // means "no override, use the fleet default"; an empty non-nil slice means // "none here" — the difference is why these are pointers to slices in JSON diff --git a/internal/state/store.go b/internal/state/store.go index 6b6a83a..bbdbf71 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -43,6 +43,9 @@ type StoreConfig struct { // bots ("" hides the dashboard row, keeping co-bot-less dashboards // byte-identical). CoReviewers string + // Host names this process in the state it writes, so the fleet can tell which + // binaries are driving and what they understand (see State.NoteWriter). + Host string // MinInterval is DecideFire's pacing gate. The dashboard needs it so a queue // entry is not advertised as ready before firing would actually accept it. MinInterval time.Duration @@ -209,6 +212,7 @@ func (s *GitStateStore) Update(ctx context.Context, mutate func(*State) error) ( now := time.Now().UTC() st.Rev++ st.UpdatedAt = &now + st.NoteWriter(s.cfg.Host, WriterCaps, now) st.Normalize(now) if err := s.compareAndSwap(ctx, &st, rev); err != nil { if errors.Is(err, ErrCASConflict) { diff --git a/internal/state/writers_test.go b/internal/state/writers_test.go new file mode 100644 index 0000000..0e02555 --- /dev/null +++ b/internal/state/writers_test.go @@ -0,0 +1,44 @@ +package state + +import ( + "testing" + "time" +) + +// Sharing a state ref stops an older binary ERASING a new field. It does not +// make that binary act on it: it loads the field as unknown JSON, writes it back +// untouched, and keeps deciding from its own fleet-wide configuration. So the +// fleet has to be able to say who is driving and what they understand. +func TestLaggingWritersNamesWhoWillIgnoreTheOverride(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + st := New() + st.Leader = &LeaderLease{Owner: "old-mac", Token: "t", ExpiresAt: now.Add(time.Minute)} + + // A leader that has never announced a capability is exactly the old binary. + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 1 || got[0] != "old-mac" { + t.Fatalf("lagging = %v, want the leader named", got) + } + + // Once it writes with the capability, it is no longer lagging. + st.NoteWriter("old-mac", CapsRepoOverrides, now) + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 0 { + t.Errorf("lagging = %v, want none once the leader understands", got) + } + + // A stale stamp does not count: the host may have been downgraded since. + if got := st.LaggingWriters(CapsRepoOverrides, now.Add(2*time.Hour)); len(got) != 0 { + // The lease has expired by then, so nobody is acting. + t.Errorf("lagging = %v, want none when no lease is live", got) + } + st.Leader = &LeaderLease{Owner: "old-mac", Token: "t", ExpiresAt: now.Add(2*time.Hour + time.Minute)} + if got := st.LaggingWriters(CapsRepoOverrides, now.Add(2*time.Hour)); len(got) != 1 { + t.Errorf("lagging = %v, want the leader named again once its stamp is stale", got) + } + + // Writers are bounded: a host silent for a day is not part of the fleet. + st.NoteWriter("other", CapsRepoOverrides, now) + st.NoteWriter("fresh", CapsRepoOverrides, now.Add(25*time.Hour)) + if _, ok := st.Writers["other"]; ok { + t.Errorf("writers = %v, want the day-old host pruned", st.Writers) + } +} diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index b4609e1..321daca 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -183,7 +183,11 @@ Each reviewer reports its `budget`: `account` is serialized against the shared C `none` runs immediately and waits for nobody. That is the only property the queue cares about. The setting lives in the shared state ref, so the daemon and every agent read the same one. The -primary reviewer is fleet-wide and cannot be set per repository. +primary reviewer is fleet-wide and cannot be set per repository. `--required` cannot be empty (a +round gating on nobody converges before anything runs); use `clear` to drop the override. + +If the output lists `lagging_hosts`, those hosts are driving the queue with a binary that predates +per-repo overrides — they will keep using the fleet default until upgraded. ## Fleet Auto-Review From e8c201710a196590ed649f431f909e867f7447a6 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:27:22 +0200 Subject: [PATCH 3/5] Never gate a repository on a reviewer crq will not ask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine review findings, four of them P1, and they converge on one mistake: the override changed who has to answer without changing who gets asked. `--required bugbot` with no `--bots` set the required half only, so nothing enabled Bugbot and it became an unknown reviewer with no command. Required now implies enabled whichever half was named, the rule fleet parsing already follows. A retained co-reviewer that becomes required is promoted out of a never trigger for the same reason — Codex's fleet entry is never while it is optional, so keeping that entry and flipping only the Required bit left the engine waiting for evidence no command was posted for. Only a never trigger is promoted, so a deliberate selfheal survives. Autoreview decided from the primary alone, so a repo requiring Codex whose head CodeRabbit had already reviewed was never enqueued and Codex was never asked. It evaluates the repository's own required set now. And changing the requirements on a head with a completed round stranded the PR outright: convergence reported the new reviewer pending while enqueue kept skipping the head, because a completed round IS the dedup marker. Those rounds are reopened when the required set actually changes. Three more. The two halves are merged inside the CAS closure, or two hosts setting different halves both derive from one snapshot and the later write drops the other's. An explicit CRQ_FEEDBACK_BOTS survives an override, since it is the one list an operator may widen beyond who reviews. And the lagging-host check now matches the leader's "host=x pid=n" form against the host key capabilities are recorded under — otherwise every current-version daemon read as needing an upgrade. The docs no longer call the Codex example Codex-only: an override chooses the co-reviewers, so the primary is still triggered and still spends quota. Leaving it out of --required means the round does not wait for it. --- internal/crq/auto.go | 25 ++++++- internal/crq/config.go | 21 ++++-- internal/crq/repoconfig.go | 94 +++++++++++++++++++----- internal/crq/repoconfig_test.go | 114 +++++++++++++++++++++++++++++ internal/crq/reviewers.go | 118 ++++++++++++++++++++----------- internal/state/state.go | 44 +++++++++++- internal/state/writers_test.go | 14 ++++ llms.txt | 6 +- skills/coderabbit-queue/SKILL.md | 9 ++- 9 files changed, 372 insertions(+), 73 deletions(-) diff --git a/internal/crq/auto.go b/internal/crq/auto.go index cd69e12..c9d5611 100644 --- a/internal/crq/auto.go +++ b/internal/crq/auto.go @@ -305,17 +305,36 @@ func (s *Service) needsReview(ctx context.Context, state State, repo string, pr if err != nil { return false, "", err } - bot := dialect.NormalizeBotName(s.cfg.Bot) + cfg := s.cfgFor(state, repo) + bot := dialect.NormalizeBotName(cfg.Bot) lastBotReview := "" + // Every reviewer this repository gates on, not just the primary. A repo that + // requires Codex or Bugbot and already has a CodeRabbit review at the head + // would otherwise never be enqueued, so the reviewer it chose is never asked. + reviewedHere := map[string]string{} for _, review := range reviews { - if dialect.NormalizeBotName(review.User.Login) == bot && review.CommitID != "" { + login := dialect.NormalizeBotName(review.User.Login) + if review.CommitID == "" { + continue + } + if login == bot { lastBotReview = dialect.ShortOID(review.CommitID) } + reviewedHere[login] = dialect.ShortOID(review.CommitID) } if incremental { need := lastBotReview != head + missing := cfg.Bot + if !need { + for _, login := range cfg.RequiredBots { + if reviewedHere[dialect.NormalizeBotName(login)] != head { + need, missing = true, login + break + } + } + } if need { - s.logEnqueue(repo, pr, head, "no bot review at head") + s.logEnqueue(repo, pr, head, "no "+dialect.NormalizeBotName(missing)+" review at head") } return need, head, nil } diff --git a/internal/crq/config.go b/internal/crq/config.go index e505d70..a96d1f7 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -47,11 +47,15 @@ type Config struct { // Reviewers is the single description of who reviews and what they cost. // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. - Reviewers []Reviewer - RateLimitCommand string - RateLimitMarker string - CalibrationMarker string - ReviewDoneMarker string + Reviewers []Reviewer + // FeedbackBotsExplicit records that CRQ_FEEDBACK_BOTS was set. It is the one + // list an operator may widen beyond who reviews, so neither LoadConfig's + // derivation nor a per-repo override may quietly replace it. + FeedbackBotsExplicit bool + RateLimitCommand string + RateLimitMarker string + CalibrationMarker string + ReviewDoneMarker string // 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. @@ -190,7 +194,12 @@ func LoadConfig() (Config, error) { // CRQ_FEEDBACK_BOTS still wins: it is the one list an operator may widen // beyond who reviews (to surface a bot's findings without waiting for it). cfg.RequiredBots = cfg.reviewerLogins(func(r Reviewer) bool { return r.Required }) - if _, explicit := env["CRQ_FEEDBACK_BOTS"]; !explicit { + // Present-but-empty is not a choice: an operator who exports the variable + // blank has named nobody, and treating that as explicit would freeze the + // derived list and, through ForRepo, ignore every per-repo override. + explicitFeedback := strings.TrimSpace(env["CRQ_FEEDBACK_BOTS"]) != "" + cfg.FeedbackBotsExplicit = explicitFeedback + if !explicitFeedback { // Everyone except a primary the operator deliberately left out of // CRQ_REQUIRED_BOTS. That omission is how you say "do not wait for // CodeRabbit here", and surfacing its findings anyway would put the round diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go index fb17c7e..c30f214 100644 --- a/internal/crq/repoconfig.go +++ b/internal/crq/repoconfig.go @@ -6,6 +6,7 @@ import ( "fmt" "sort" "strings" + "time" "github.com/kristofferR/coderabbit-queue/internal/dialect" ) @@ -107,23 +108,17 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require return out, nil } - // Start from the existing override so a call that names one half leaves the - // other alone — the contract the nil/empty distinction promises. - ov := RepoReviewers{} - if st, _, err := s.store.Load(ctx); err == nil { - if existing, ok := st.RepoOverride(repo); ok { - ov = existing - } - } else { - return ReviewerView{}, err - } - + // Both halves are resolved here; the MERGE happens inside the CAS closure, + // because two hosts setting different halves would otherwise derive from the + // same snapshot and the later write would drop the earlier one's half — and + // a retry that reuses an already-merged value cannot fix that. + var setCoBots, setRequired []string if coBots != nil { resolved, err := resolve(known, coBots, "--bots") if err != nil { return ReviewerView{}, err } - ov.CoBots, ov.SetCoBots = resolved, true + setCoBots = resolved } if required != nil { if len(required) == 0 { @@ -141,14 +136,22 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require if err != nil { return ReviewerView{}, err } - ov.Required, ov.SetRequired = resolved, true + setRequired = resolved } - now := s.clock().UTC() - ov.UpdatedAt = &now - ov.By = s.cfg.Host + now := s.clock().UTC() state, err := s.store.Update(ctx, func(st *State) error { + ov, _ := st.RepoOverride(repo) + if coBots != nil { + ov.CoBots, ov.SetCoBots = setCoBots, true + } + if required != nil { + ov.Required, ov.SetRequired = setRequired, true + } + ov.UpdatedAt, ov.By = &now, s.cfg.Host + before := s.cfg.ForRepo(mustOverride(st, repo)) st.SetRepoOverride(repo, ov) + s.reopenForChangedReviewers(st, repo, before, s.cfg.ForRepo(ov), now) return nil }) if err != nil { @@ -161,10 +164,13 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require // ClearReviewers returns repo to the fleet default. func (s *Service) ClearReviewers(ctx context.Context, repo string) (ReviewerView, error) { repo = NormalizeRepo(repo) + now := s.clock().UTC() state, err := s.store.Update(ctx, func(st *State) error { + before := s.cfg.ForRepo(mustOverride(st, repo)) if !st.ClearRepoOverride(repo) { return ErrNoChange } + s.reopenForChangedReviewers(st, repo, before, s.cfg, now) return nil }) if err != nil && !errors.Is(err, ErrNoChange) { @@ -192,3 +198,59 @@ func knownLogins(known map[string]string) []string { sort.Strings(out) return out } + +// mustOverride is the repo's override, or the zero value meaning "fleet +// default" — the same thing ForRepo treats as no override. +func mustOverride(st *State, repo string) RepoReviewers { + ov, _ := st.RepoOverride(repo) + return ov +} + +// reopenForChangedReviewers requeues this repository's completed rounds when the +// set of reviewers that gates them changed. +// +// A completed round is the "this head was reviewed" dedup marker, so adding a +// required reviewer would otherwise strand the PR: Feedback reports the new bot +// pending, while Enqueue keeps skipping the head because the completed round is +// still there. No eligible round exists to trigger it, and `crq next` waits for +// a push that has no reason to come. +// +// Only rounds whose required set actually changed are touched, and only +// completed ones — an in-flight round is already going to answer. +func (s *Service) reopenForChangedReviewers(st *State, repo string, before, after Config, now time.Time) { + if sameLogins(before.RequiredBots, after.RequiredBots) { + return + } + for _, round := range st.Rounds { + if NormalizeRepo(round.Repo) != NormalizeRepo(repo) || round.Phase != PhaseCompleted { + continue + } + reopened := round + if err := reopened.Reopen(now); err != nil { + continue + } + st.PutRound(reopened) + if s.log != nil { + s.log.Printf("reviewers: requeued %s#%d@%s — the required set changed", round.Repo, round.PR, round.Head) + } + } +} + +// sameLogins compares two reviewer lists as sets, since order is presentation. +func sameLogins(a, b []string) bool { + if len(a) != len(b) { + return false + } + seen := map[string]int{} + for _, login := range a { + seen[dialect.NormalizeBotName(login)]++ + } + for _, login := range b { + key := dialect.NormalizeBotName(login) + seen[key]-- + if seen[key] < 0 { + return false + } + } + return true +} diff --git a/internal/crq/repoconfig_test.go b/internal/crq/repoconfig_test.go index 580164f..a354dd4 100644 --- a/internal/crq/repoconfig_test.go +++ b/internal/crq/repoconfig_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" ) // The override has to reach a real decision, not just the config value: a @@ -125,3 +126,116 @@ func TestOverrideKeepsTheSilencedPrimaryEntry(t *testing.T) { primaryPresent(fleet, "fleet default") primaryPresent(fleet.ForRepo(RepoReviewers{CoBots: []string{"codex"}, SetCoBots: true}), "override naming only codex") } + +// Four defects that all end the same way: a round waiting on a reviewer crq +// never asks, or never waiting at all. +func TestOverrideNeverGatesOnAReviewerItCannotTrigger(t *testing.T) { + t.Run("required alone enables the bot", func(t *testing.T) { + // `reviewers set repo --required bugbot` with no --bots: SetRequired is + // true and SetCoBots is false, so nothing enabled Bugbot and it became an + // unknown required reviewer with no command. + fleet := isolatedConfig(t, map[string]string{"CRQ_COBOTS": "codex"}) + got := fleet.ForRepo(RepoReviewers{Required: []string{"bugbot"}, SetRequired: true}) + for _, r := range got.Reviewers { + if dialect.NormalizeBotName(r.Login) != "cursor" { + continue + } + if !r.Required { + t.Error("bugbot must gate here") + } + if r.Command == "" || r.Trigger == engine.TriggerNever { + t.Errorf("bugbot = %+v: required but never triggered, so the round waits forever", r) + } + return + } + t.Errorf("reviewers = %+v, want bugbot enabled by requiring it", got.Reviewers) + }) + + t.Run("a promoted co-reviewer gets a trigger", func(t *testing.T) { + // Codex's fleet entry is trigger=never while it is optional. Retaining + // that entry and only flipping Required leaves the engine waiting for + // evidence no command was ever posted for. + fleet := isolatedConfig(t, map[string]string{"CRQ_COBOTS": "codex,bugbot"}) + for _, cb := range fleet.CoBots { + if cb.Name == "codex" && cb.Trigger != engine.TriggerNever { + t.Skip("codex is already triggered by default here; nothing to promote") + } + } + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"codex"}, SetCoBots: true, + Required: []string{"codex"}, SetRequired: true, + }) + for _, cb := range got.CoBots { + if cb.Name == "codex" && cb.Trigger == engine.TriggerNever { + t.Errorf("codex = %+v: required with no trigger", cb) + } + } + }) + + t.Run("an explicit feedback set survives", func(t *testing.T) { + // CRQ_FEEDBACK_BOTS is the one list an operator may widen beyond who + // reviews. Deriving over it would silently stop surfacing those findings + // the moment the repository got any override at all. + fleet := isolatedConfig(t, map[string]string{"CRQ_FEEDBACK_BOTS": "sonar[bot]"}) + got := fleet.ForRepo(RepoReviewers{CoBots: []string{"codex"}, SetCoBots: true}) + if len(got.FeedbackBots) != 1 || got.FeedbackBots[0] != "sonar[bot]" { + t.Errorf("FeedbackBots = %v, want the explicit setting kept", got.FeedbackBots) + } + }) +} + +// Changing who has to review a head must not strand the PR. A completed round +// is the "this head was reviewed" dedup marker, so adding a required reviewer +// leaves convergence reporting it pending while enqueue keeps skipping the head: +// no eligible round exists to trigger it, and next waits for a push that has no +// reason to come. +func TestChangingRequirementsReopensACompletedRound(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + repo, pr, head := "o/r", 3, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseCompleted, time.Now().UTC(), 11) + + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, []string{"codex"}); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %#v, want it requeued so the new reviewer can be asked", round) + } + if round.Head != head { + t.Errorf("head = %q, want the same head %q — what changed is who must answer", round.Head, head) + } + + // An override that does not change the required set leaves rounds alone: a + // finished round must not be reopened for nothing. + if _, err := store.Update(ctx, func(st *State) error { + r := st.Round(repo, pr) + if err := r.Reserve("t", "h", time.Now().UTC()); err != nil { + return err + } + if err := r.Fire(12, time.Now().UTC()); err != nil { + return err + } + if err := r.Complete(); err != nil { + return err + } + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, []string{"codex"}); err != nil { + t.Fatal(err) + } + st, _, _ = store.Load(ctx) + if round := st.Round(repo, pr); round == nil || round.Phase != PhaseCompleted { + t.Errorf("round = %#v, want it left completed when nothing changed", round) + } +} diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index 0fe3c9d..318df20 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -187,60 +187,94 @@ func (c Config) ForRepo(ov RepoReviewers) Config { return c } out := c - if ov.SetCoBots { - // Required implies enabled, the same rule the fleet parse follows: a bot - // that gates but is never triggered waits forever. - wanted := append([]string(nil), ov.CoBots...) - if ov.SetRequired { - for _, login := range ov.Required { - if !containsBot(wanted, login) && !sameBot(login, c.Bot) { - wanted = append(wanted, login) - } - } - } - keep := make([]CoBotConfig, 0, len(wanted)+1) - have := map[string]bool{} + + // The effective required set, whichever half the override named. + required := c.RequiredBots + if ov.SetRequired { + required = ov.Required + } + + // The effective co-reviewer set. Required implies enabled — the rule the + // fleet parse already follows, and it has to hold when only the required + // half is overridden too: a bot that gates but is never triggered makes the + // round wait for evidence crq never asks for. + enabled := ov.CoBots + if !ov.SetCoBots { + enabled = nil for _, cb := range c.CoBots { - // A primary that is itself a registry bot keeps its silenced entry - // whatever the override says: that entry is where its wording and - // check-run hooks come from, and dropping it costs the primary its - // evidence (see LoadConfig). - if sameBot(cb.Login, c.Bot) { - keep = append(keep, cb) - have[dialect.NormalizeBotName(cb.Login)] = true - continue - } - if containsBot(wanted, cb.Login) { - keep = append(keep, cb) - have[dialect.NormalizeBotName(cb.Login)] = true - } + enabled = append(enabled, cb.Login) } - // A repository may choose a bot the fleet default does not enable — - // otherwise "which bots for which project" only ever subtracts. Its - // configuration comes from the registry, since there is no per-bot - // environment for a repo. - for _, login := range wanted { - if have[dialect.NormalizeBotName(login)] { - continue - } - if co, ok := dialect.CoReviewerByName(login); ok { - keep = append(keep, defaultCoBot(co, containsBot(ov.Required, login))) - have[dialect.NormalizeBotName(login)] = true - } + } + for _, login := range required { + if sameBot(login, c.Bot) || containsBot(enabled, login) { + continue + } + if _, ok := dialect.CoReviewerByName(login); ok { + enabled = append(enabled, login) } - out.CoBots = keep } - if ov.SetRequired { - out.RequiredBots = append([]string(nil), ov.Required...) + + keep := make([]CoBotConfig, 0, len(enabled)+1) + have := map[string]bool{} + for _, cb := range c.CoBots { + switch { + case sameBot(cb.Login, c.Bot): + // A primary that is itself a registry bot keeps its silenced entry + // whatever the override says: that entry carries its wording and + // check-run hooks, and dropping it costs the PRIMARY its evidence. + case containsBot(enabled, cb.Login): + cb.Required = containsBot(required, cb.Login) + cb = promoteTrigger(cb) + default: + continue + } + keep = append(keep, cb) + have[dialect.NormalizeBotName(cb.Login)] = true } + // A repository may choose a bot the fleet does not enable — otherwise + // "which bots for which project" only ever subtracts. Its configuration + // comes from the registry, since there is no per-bot environment for a repo. + for _, login := range enabled { + if have[dialect.NormalizeBotName(login)] { + continue + } + if co, ok := dialect.CoReviewerByName(login); ok { + keep = append(keep, defaultCoBot(co, containsBot(required, login))) + have[dialect.NormalizeBotName(login)] = true + } + } + out.CoBots = keep + out.RequiredBots = append([]string(nil), required...) + // Rebuild the derived views from the overridden lists, exactly as // LoadConfig does, so no view can answer differently from another. out.Reviewers = buildReviewers(out.Bot, out.ReviewCommand, out.RequiredBots, out.CoBots) out.RequiredBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required }) - out.FeedbackBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required || !r.Metered() }) + if !c.FeedbackBotsExplicit { + out.FeedbackBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required || !r.Metered() }) + } return out } +// promoteTrigger stops a required co-reviewer from being one crq never asks. +// +// A fleet entry carries the trigger its OWN required-ness produced: Codex +// defaults to never and only becomes always when required. Retaining that entry +// while making the bot required leaves the engine waiting for evidence no +// command was ever posted for. Only a never trigger is promoted, so an operator +// who deliberately configured selfheal keeps it. +func promoteTrigger(cb CoBotConfig) CoBotConfig { + if !cb.Required || cb.Trigger != engine.TriggerNever || cb.Command == "" { + return cb + } + if co, ok := dialect.CoReviewerByName(cb.Name); ok && co.RequiredTrigger != "" { + cb.Trigger = triggerMode(co.RequiredTrigger, engine.TriggerAlways) + return cb + } + cb.Trigger = engine.TriggerAlways + return cb +} + // sameBot reports whether two spellings name the same reviewer. func sameBot(a, b string) bool { return dialect.NormalizeBotName(a) == dialect.NormalizeBotName(b) diff --git a/internal/state/state.go b/internal/state/state.go index f188549..ac5a8ce 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -359,8 +359,13 @@ func (s *State) NoteWriter(host string, caps int, now time.Time) { // back untouched, and keeps deciding from its own fleet-wide configuration. func (s *State) LaggingWriters(caps int, now time.Time) []string { acting := map[string]bool{} - if s.Leader != nil && s.Leader.Owner != "" && s.Leader.ExpiresAt.After(now) { - acting[s.Leader.Owner] = true + // The leader records itself as "host= pid=", while capabilities are + // keyed by host alone. Comparing the two directly would report every + // current-version daemon as lagging. + if s.Leader != nil && s.Leader.ExpiresAt.After(now) { + if host := ownerHost(s.Leader.Owner); host != "" { + acting[host] = true + } } if slot := s.SlotRound(); slot != nil && slot.ByHost != "" { acting[slot.ByHost] = true @@ -376,6 +381,17 @@ func (s *State) LaggingWriters(caps int, now time.Time) []string { return out } +// ownerHost extracts the host from a leader owner string ("host=x pid=1"), +// falling back to the whole value for an owner that is just a host. +func ownerHost(owner string) string { + for _, field := range strings.Fields(owner) { + if rest, ok := strings.CutPrefix(field, "host="); ok { + return rest + } + } + return strings.TrimSpace(owner) +} + // RepoReviewers overrides which reviewers run on one repository. A nil slice // means "no override, use the fleet default"; an empty non-nil slice means // "none here" — the difference is why these are pointers to slices in JSON @@ -495,6 +511,30 @@ func (r *Round) ReleaseToQueue(reason string, now time.Time) error { return nil } +// Reopen puts a completed round back in the queue because the set of reviewers +// that has to answer for its head changed. +// +// A completed round is the "this head was reviewed" dedup marker, so a newly +// required reviewer would otherwise strand the PR: convergence reports it +// pending while enqueue keeps skipping the head, and no eligible round exists to +// trigger it. This is the one transition that reopens a finished round, and it +// keeps the head, the attempts and the co-reviewer bookkeeping — what changed is +// who still has to answer, not what happened. +func (r *Round) Reopen(now time.Time) error { + if r.Phase != PhaseCompleted { + return r.illegal(PhaseQueued) + } + r.Phase = PhaseQueued + r.Token = "" + r.ReservedAt = nil + r.WaitDeadline = nil + r.RetryAt = nil + t := now.UTC() + r.LastAttemptAt = &t + r.Note = "reviewer requirements changed" + return nil +} + // Acknowledge records that the bot has seen the fired command (reaction, // in-progress summary, or other non-terminal reply): fired → reviewing. The // fire slot may be released; the round itself stays open until Complete. diff --git a/internal/state/writers_test.go b/internal/state/writers_test.go index 0e02555..f805f1d 100644 --- a/internal/state/writers_test.go +++ b/internal/state/writers_test.go @@ -42,3 +42,17 @@ func TestLaggingWritersNamesWhoWillIgnoreTheOverride(t *testing.T) { t.Errorf("writers = %v, want the day-old host pruned", st.Writers) } } + +// The leader records itself as "host= pid=" while capabilities are +// keyed by host alone. Comparing those directly reported every current-version +// daemon as needing an upgrade — the warning would have been pure noise. +func TestLaggingWritersMatchesTheLeadersHostForm(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + st := New() + st.Leader = &LeaderLease{Owner: "host=cachyos pid=1234", ExpiresAt: now.Add(time.Minute)} + st.NoteWriter("cachyos", CapsRepoOverrides, now) + + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 0 { + t.Errorf("lagging = %v, want none — the leader IS the capable writer", got) + } +} diff --git a/llms.txt b/llms.txt index 0264233..1a62f1a 100644 --- a/llms.txt +++ b/llms.txt @@ -206,10 +206,14 @@ Choose which bots review one project: ```bash crq reviewers REPO # effective reviewers + budgets -crq reviewers set REPO --bots codex --required codex # only Codex here +crq reviewers set REPO --bots codex --required codex # Codex is the only required co-reviewer crq reviewers clear REPO # fleet default again ``` +The primary is fleet-wide: an override chooses the CO-reviewers, so CodeRabbit is still triggered +and still spends account quota even when it is left out of `--required`. Leaving it out means the +round does not WAIT for it, not that it is never asked. + `budget: account` means the reviewer is serialized against the shared CodeRabbit allowance; `budget: none` means it runs immediately. Stored in the shared state ref, so the daemon and agents cannot disagree. The primary reviewer is fleet-wide. diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 321daca..129e245 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -175,15 +175,18 @@ finding. Pass `--keep-open` to leave it unresolved deliberately. ```bash crq reviewers "$REPO" # who runs here, and what each costs -crq reviewers set "$REPO" --bots codex --required codex # only Codex here +crq reviewers set "$REPO" --bots codex --required codex # Codex the only required co-reviewer crq reviewers clear "$REPO" # back to the fleet default ``` Each reviewer reports its `budget`: `account` is serialized against the shared CodeRabbit allowance, `none` runs immediately and waits for nobody. That is the only property the queue cares about. -The setting lives in the shared state ref, so the daemon and every agent read the same one. The -primary reviewer is fleet-wide and cannot be set per repository. `--required` cannot be empty (a +The setting lives in the shared state ref, so the daemon and every agent read the same one. + +The primary reviewer is fleet-wide: an override chooses the **co-reviewers**. Leaving the primary out +of `--required` means the round does not wait for it — not that it is never triggered, so it still +spends account quota. `--required` cannot be empty (a round gating on nobody converges before anything runs); use `clear` to drop the override. If the output lists `lagging_hosts`, those hosts are driving the queue with a binary that predates From 6da6ae0e596baa4801c0f80bef7164d2f023e1c6 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 19:46:02 +0200 Subject: [PATCH 4/5] Refuse a reviewers command that looks like it worked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of four findings; the fourth is answered rather than fixed. `crq reviewers owner/repo --bots codex` is a set command missing its verb. It showed the configuration and exited 0, which tells automation the mutation worked when nothing changed. It is refused now, naming the verb. `crq reviewers clear owner-repo` cleared a key nothing uses and exited 0, so a typo left the real override in force while the caller believed it had restored the fleet default. Clear validates the shape set already validates. On the fourth: reopening a completed round can re-ask a primary that finished with a completion reply and no Review object, because DecideFire's already-reviewed gate reads submitted reviews. Marking the round co-only does not help — that flag records what a fire DID, it does not gate one — and the alternatives all need an observation the CAS closure cannot make. So the trade is stated in the code: one duplicate review in an uncommon case, against a PR stranded silently and permanently. The loud failure is the better one, and the gate is where a real fix belongs. --- cmd/crq/main.go | 7 +++++++ internal/crq/repoconfig.go | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 6fe5c69..1bc64fd 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -794,6 +794,13 @@ func runReviewers(ctx context.Context, service *crq.Service, args []string) int } view, err = service.SetReviewers(ctx, repo, splitList(bots), splitList(required)) default: + // `crq reviewers owner/repo --bots codex` is a set command missing its + // verb. Showing the configuration and exiting 0 tells automation the + // mutation worked when nothing changed. + if bots != nil || required != nil { + fatal(errors.New("did you mean `crq reviewers set`? --bots/--required only apply to set")) + return 1 + } view, err = service.Reviewers(ctx, repo) } if err != nil { diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go index c30f214..5fb9af8 100644 --- a/internal/crq/repoconfig.go +++ b/internal/crq/repoconfig.go @@ -164,6 +164,12 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require // ClearReviewers returns repo to the fleet default. func (s *Service) ClearReviewers(ctx context.Context, repo string) (ReviewerView, error) { repo = NormalizeRepo(repo) + // The same shape check set does. A typo like "owner-repo" would otherwise + // clear a key nothing uses and exit 0, so automation believes it restored the + // fleet default while the real override is still in force. + if !strings.Contains(repo, "/") { + return ReviewerView{}, fmt.Errorf("repo must be owner/name, got %q", repo) + } now := s.clock().UTC() state, err := s.store.Update(ctx, func(st *State) error { before := s.cfg.ForRepo(mustOverride(st, repo)) @@ -217,6 +223,12 @@ func mustOverride(st *State, repo string) RepoReviewers { // // Only rounds whose required set actually changed are touched, and only // completed ones — an in-flight round is already going to answer. +// +// Known cost: DecideFire's already-reviewed gate reads submitted reviews, so a +// round the primary finished with a completion REPLY and no Review object can be +// asked once more. That is one duplicate review in an uncommon case, against a +// PR stranded silently and permanently — the loud failure is the better one, and +// the gate is where a real fix belongs. func (s *Service) reopenForChangedReviewers(st *State, repo string, before, after Config, now time.Time) { if sameLogins(before.RequiredBots, after.RequiredBots) { return From 44f9cedceb6af174886111e1ddbd1f926f203d17 Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 21:37:36 +0200 Subject: [PATCH 5/5] Recognise a completion reply as a reviewed head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four P1s, and the first retires a trade-off I had argued for instead of fixing. Reopening a completed round could re-ask a primary that answered with a completion reply and no Review object, because the already-reviewed gate read only obs.Reviews. The reviewer's point was that the gate is where the fix belongs, and they were right: it now also counts a completion reply paired to that round's own command. Reopening no longer risks a duplicate review, so the round can be requeued when the required set changes without the cost I had written into a comment. A repository enabling a bot the fleet disabled was built from registry defaults, silently discarding the operator's CRQ_COBOT__CMD, TRIGGER and GRACE — a different bot than the one configured. Every registry co-reviewer is now resolved with the environment applied, enabled or not, and an override picks from that. Capabilities are keyed per PROCESS rather than per host. A new CLI and an old daemon on one machine is the ordinary upgrade, and a per-host key let the CLI's write vouch for the daemon that had not been upgraded — hiding exactly the case the signal exists to catch. And applyFire revalidates the override before acting. Deciding and writing are two steps, so an operator removing a co-reviewer between them would otherwise have crq post that bot's trigger on the authority of a configuration that no longer exists. --- internal/crq/config.go | 87 +++++++++++++++++++++++----------- internal/crq/repoconfig.go | 9 ++-- internal/crq/reviewers.go | 45 ++++++++++++++++-- internal/crq/service.go | 7 +++ internal/crq/state.go | 2 +- internal/engine/fire.go | 9 ++++ internal/state/state.go | 27 ++++------- internal/state/writers_test.go | 20 +++++--- 8 files changed, 145 insertions(+), 61 deletions(-) diff --git a/internal/crq/config.go b/internal/crq/config.go index a96d1f7..9be51c1 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -44,6 +44,11 @@ type Config struct { // CRQ_COBOT__* keys). An entry exists for every wanted or required // co-reviewer; required ones are already folded into RequiredBots. CoBots []CoBotConfig + // KnownCoBots is every registry co-reviewer with the operator's per-bot + // environment applied, enabled here or not. A repository override picks from + // this, so enabling a bot the fleet disabled still honours CRQ_COBOT__* + // instead of silently falling back to registry defaults. + KnownCoBots []CoBotConfig // Reviewers is the single description of who reviews and what they cost. // Bot / RequiredBots / FeedbackBots / CoBots above are DERIVED from it and // kept only so existing consumers keep compiling; new code should read this. @@ -52,10 +57,13 @@ type Config struct { // list an operator may widen beyond who reviews, so neither LoadConfig's // derivation nor a per-repo override may quietly replace it. FeedbackBotsExplicit bool - RateLimitCommand string - RateLimitMarker string - CalibrationMarker string - ReviewDoneMarker string + // OverrideAt is when the per-repo reviewer override this configuration was + // built from was last written, so a fire can tell whether it still holds. + OverrideAt *time.Time + RateLimitCommand string + RateLimitMarker string + CalibrationMarker string + ReviewDoneMarker string // 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. @@ -188,6 +196,7 @@ func LoadConfig() (Config, error) { } // Built here, after the command is resolved, because the primary's trigger is // part of describing it. + cfg.KnownCoBots = parseAllCoBots(env) cfg.Reviewers = buildReviewers(cfg.Bot, cfg.ReviewCommand, requiredBots, coBots) // The legacy lists are now VIEWS of cfg.Reviewers rather than parallel // parses, so they cannot answer differently from it. An explicit @@ -335,6 +344,44 @@ type CoBotConfig struct { // legacy alias of CRQ_COBOT_CODEX_CMD. Bugbot/Macroscope default to // `selfheal` — they auto-review pushes, so crq only nudges one that went // silent on a head it should have covered. +// resolveCoBot applies the operator's per-bot environment to a registry entry. +// Uniform across bots: the per-bot key wins, then the registry's legacy alias if +// it declares one, then its default command. No bot is named here — the policy +// travels as registry metadata. +func resolveCoBot(env map[string]string, co dialect.CoReviewer, required bool) CoBotConfig { + key := strings.ToUpper(co.Name) + base := defaultCoBot(co, required) + command := base.Command + if v, ok := env["CRQ_COBOT_"+key+"_CMD"]; ok { + command = v + } else if co.LegacyCommandEnv != "" { + command = stringEnvAllowEmpty(env, co.LegacyCommandEnv, command) + } + command = strings.TrimSpace(command) + trigger := base.Trigger + switch v := engine.TriggerMode(strings.ToLower(strings.TrimSpace(env["CRQ_COBOT_"+key+"_TRIGGER"]))); v { + case engine.TriggerNever, engine.TriggerSelfHeal, engine.TriggerAlways: + trigger = v + } + if command == "" { + // No trigger command means crq can never post one, whatever the mode. + trigger = engine.TriggerNever + } + base.Command, base.Trigger = command, trigger + base.SelfHealGrace = durationEnv(env, "CRQ_COBOT_"+key+"_GRACE", defaultSelfHealGrace) + return base +} + +// parseAllCoBots resolves every registry co-reviewer with the environment +// applied, regardless of whether CRQ_COBOTS enables it. +func parseAllCoBots(env map[string]string) []CoBotConfig { + out := make([]CoBotConfig, 0, 4) + for _, co := range dialect.KnownCoReviewers() { + out = append(out, resolveCoBot(env, co, false)) + } + return out +} + func parseCoBots(env map[string]string, requiredBots []string) ([]CoBotConfig, error) { enabled := map[string]bool{} var unknown []string @@ -368,29 +415,7 @@ func parseCoBots(env map[string]string, requiredBots []string) ([]CoBotConfig, e if !enabled[co.Name] && !required { continue } - base := defaultCoBot(co, required) - // Uniform across bots: the per-bot key wins, then the registry's legacy - // alias if it declares one, then its default command. No bot is named - // here — the policy travels as registry metadata. - command := base.Command - if v, ok := env["CRQ_COBOT_"+key+"_CMD"]; ok { - command = v - } else if co.LegacyCommandEnv != "" { - command = stringEnvAllowEmpty(env, co.LegacyCommandEnv, command) - } - command = strings.TrimSpace(command) - trigger := base.Trigger - switch v := engine.TriggerMode(strings.ToLower(strings.TrimSpace(env["CRQ_COBOT_"+key+"_TRIGGER"]))); v { - case engine.TriggerNever, engine.TriggerSelfHeal, engine.TriggerAlways: - trigger = v - } - if command == "" { - // No trigger command means crq can never post one, whatever the mode. - trigger = engine.TriggerNever - } - base.Command, base.Trigger = command, trigger - base.SelfHealGrace = durationEnv(env, "CRQ_COBOT_"+key+"_GRACE", defaultSelfHealGrace) - out = append(out, base) + out = append(out, resolveCoBot(env, co, required)) } return out, nil } @@ -507,3 +532,11 @@ func ownerOf(repo string) string { owner, _, _ := strings.Cut(repo, "/") return owner } + +// WriterID identifies this PROCESS in the shared state, in the same form the +// autoreview leader records. A new CLI and an old daemon commonly run on one +// machine, so a per-host key would let the upgraded CLI's write vouch for the +// daemon that has not been upgraded. +func (c Config) WriterID() string { + return fmt.Sprintf("host=%s pid=%d", c.Host, os.Getpid()) +} diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go index 5fb9af8..49c9ac4 100644 --- a/internal/crq/repoconfig.go +++ b/internal/crq/repoconfig.go @@ -224,11 +224,10 @@ func mustOverride(st *State, repo string) RepoReviewers { // Only rounds whose required set actually changed are touched, and only // completed ones — an in-flight round is already going to answer. // -// Known cost: DecideFire's already-reviewed gate reads submitted reviews, so a -// round the primary finished with a completion REPLY and no Review object can be -// asked once more. That is one duplicate review in an uncommon case, against a -// PR stranded silently and permanently — the loud failure is the better one, and -// the gate is where a real fix belongs. +// The primary is not re-asked: DecideFire's already-reviewed gate now counts a +// completion reply paired to the round's command, not only a submitted Review +// object, so a reopened round that the primary already answered dedupes instead +// of buying a second review. func (s *Service) reopenForChangedReviewers(st *State, repo string, before, after Config, now time.Time) { if sameLogins(before.RequiredBots, after.RequiredBots) { return diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index 318df20..195ca48 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -238,8 +238,13 @@ func (c Config) ForRepo(ov RepoReviewers) Config { if have[dialect.NormalizeBotName(login)] { continue } - if co, ok := dialect.CoReviewerByName(login); ok { - keep = append(keep, defaultCoBot(co, containsBot(required, login))) + // From the operator's resolved settings, not registry defaults: a bot the + // fleet disabled still has CRQ_COBOT__CMD/TRIGGER/GRACE, and + // ignoring them silently gives this repository a different bot than the + // one that was configured. + if cb, ok := c.knownCoBot(login); ok { + cb.Required = containsBot(required, login) + keep = append(keep, promoteTrigger(cb)) have[dialect.NormalizeBotName(login)] = true } } @@ -297,5 +302,39 @@ func (s *Service) cfgFor(st State, repo string) Config { if !ok { return s.cfg } - return s.cfg.ForRepo(ov) + out := s.cfg.ForRepo(ov) + out.OverrideAt = ov.UpdatedAt + return out +} + +// overrideChanged reports whether repo's reviewer override differs from the one +// cfg was built from. +// +// Deciding and writing are two steps. An operator removing a co-reviewer between +// them would otherwise have crq claim and post that bot's trigger anyway, on the +// authority of a configuration that no longer exists. +func overrideChanged(st *State, repo string, cfg Config) bool { + ov, _ := st.RepoOverride(repo) + switch { + case ov.UpdatedAt == nil && cfg.OverrideAt == nil: + return false + case ov.UpdatedAt == nil || cfg.OverrideAt == nil: + return true + default: + return !ov.UpdatedAt.Equal(*cfg.OverrideAt) + } +} + +// knownCoBot is a registry co-reviewer with the operator's environment applied, +// enabled fleet-wide or not. +func (c Config) knownCoBot(login string) (CoBotConfig, bool) { + for _, cb := range c.KnownCoBots { + if sameBot(cb.Login, login) || strings.EqualFold(cb.Name, strings.TrimSpace(login)) { + return cb, true + } + } + if co, ok := dialect.CoReviewerByName(login); ok { + return defaultCoBot(co, false), true + } + return CoBotConfig{}, false } diff --git a/internal/crq/service.go b/internal/crq/service.go index a8e4028..a6f1f5b 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -668,6 +668,13 @@ func firedOrEnqueuedAt(r Round) time.Time { // applyFire executes a DecideFire verdict. func (s *Service) applyFire(ctx context.Context, cfg Config, round Round, obs engine.Observation, d engine.FireDecision, now time.Time) (PumpResult, error) { + // The decision was made from a configuration that may have been replaced + // since. Acting on it would post a trigger for a co-reviewer an operator has + // just removed, or skip one they have just required. + if st, _, err := s.store.Load(ctx); err == nil && overrideChanged(&st, round.Repo, cfg) { + return PumpResult{Action: "deduped", Repo: round.Repo, PR: round.PR, Head: round.Head, + Reason: "reviewer configuration changed while deciding"}, nil + } switch d.Verdict { case engine.FireDrop: return s.abandonRound(ctx, round, "pr closed", "skipped") diff --git a/internal/crq/state.go b/internal/crq/state.go index 8a19aac..cd422b1 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -54,7 +54,7 @@ func (c Config) storeConfig() StoreConfig { Timezone: c.Timezone, Scope: c.Scope, CoReviewers: c.coReviewerSummary(), - Host: c.Host, + Host: c.WriterID(), MinInterval: c.MinInterval, } } diff --git a/internal/engine/fire.go b/internal/engine/fire.go index 7b4c9f5..638961e 100644 --- a/internal/engine/fire.go +++ b/internal/engine/fire.go @@ -157,6 +157,15 @@ func DecideFire(g Global, r state.Round, obs Observation, now time.Time, p Polic break } } + // A submitted Review is not the only way the primary finishes. It also + // answers a command with a completion reply and no Review object, and + // reading only obs.Reviews there means crq posts the command again and buys + // a second review of code it has already been told about. The reply is + // paired to THIS round's command, so it only counts while the round still + // tracks the head. + if !reviewedHead && r.Head == obs.Head && r.CommandID != 0 { + reviewedHead = CommandHasCompletionReply(obs, p, r.CommandID) + } // Belt-and-braces live check: even with a fresh round, never fire at a // head the bot has already reviewed (e.g. state was reinitialized). But a // CodeRabbit review does not finish a round that a gating co-reviewer still diff --git a/internal/state/state.go b/internal/state/state.go index ac5a8ce..5d0ff33 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -332,7 +332,10 @@ type WriterSeen struct { At time.Time `json:"at"` } -// NoteWriter records that host wrote this state with the given capabilities. +// NoteWriter records that a process wrote this state with the given +// capabilities. The key identifies the PROCESS, not the machine: a new CLI and +// an old daemon on one host is the ordinary upgrade, and keying by hostname +// would let the CLI's write vouch for the daemon that has not been upgraded. func (s *State) NoteWriter(host string, caps int, now time.Time) { if host == "" { return @@ -359,13 +362,10 @@ func (s *State) NoteWriter(host string, caps int, now time.Time) { // back untouched, and keeps deciding from its own fleet-wide configuration. func (s *State) LaggingWriters(caps int, now time.Time) []string { acting := map[string]bool{} - // The leader records itself as "host= pid=", while capabilities are - // keyed by host alone. Comparing the two directly would report every - // current-version daemon as lagging. - if s.Leader != nil && s.Leader.ExpiresAt.After(now) { - if host := ownerHost(s.Leader.Owner); host != "" { - acting[host] = true - } + // The leader identifies itself as "host= pid=", which is exactly the + // process identity capabilities are recorded under. + if s.Leader != nil && s.Leader.ExpiresAt.After(now) && strings.TrimSpace(s.Leader.Owner) != "" { + acting[s.Leader.Owner] = true } if slot := s.SlotRound(); slot != nil && slot.ByHost != "" { acting[slot.ByHost] = true @@ -381,17 +381,6 @@ func (s *State) LaggingWriters(caps int, now time.Time) []string { return out } -// ownerHost extracts the host from a leader owner string ("host=x pid=1"), -// falling back to the whole value for an owner that is just a host. -func ownerHost(owner string) string { - for _, field := range strings.Fields(owner) { - if rest, ok := strings.CutPrefix(field, "host="); ok { - return rest - } - } - return strings.TrimSpace(owner) -} - // RepoReviewers overrides which reviewers run on one repository. A nil slice // means "no override, use the fleet default"; an empty non-nil slice means // "none here" — the difference is why these are pointers to slices in JSON diff --git a/internal/state/writers_test.go b/internal/state/writers_test.go index f805f1d..bc93c18 100644 --- a/internal/state/writers_test.go +++ b/internal/state/writers_test.go @@ -12,15 +12,15 @@ import ( func TestLaggingWritersNamesWhoWillIgnoreTheOverride(t *testing.T) { now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) st := New() - st.Leader = &LeaderLease{Owner: "old-mac", Token: "t", ExpiresAt: now.Add(time.Minute)} + st.Leader = &LeaderLease{Owner: "host=old-mac pid=7", Token: "t", ExpiresAt: now.Add(time.Minute)} // A leader that has never announced a capability is exactly the old binary. - if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 1 || got[0] != "old-mac" { + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 1 || got[0] != "host=old-mac pid=7" { t.Fatalf("lagging = %v, want the leader named", got) } // Once it writes with the capability, it is no longer lagging. - st.NoteWriter("old-mac", CapsRepoOverrides, now) + st.NoteWriter("host=old-mac pid=7", CapsRepoOverrides, now) if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 0 { t.Errorf("lagging = %v, want none once the leader understands", got) } @@ -30,7 +30,7 @@ func TestLaggingWritersNamesWhoWillIgnoreTheOverride(t *testing.T) { // The lease has expired by then, so nobody is acting. t.Errorf("lagging = %v, want none when no lease is live", got) } - st.Leader = &LeaderLease{Owner: "old-mac", Token: "t", ExpiresAt: now.Add(2*time.Hour + time.Minute)} + st.Leader = &LeaderLease{Owner: "host=old-mac pid=7", Token: "t", ExpiresAt: now.Add(2*time.Hour + time.Minute)} if got := st.LaggingWriters(CapsRepoOverrides, now.Add(2*time.Hour)); len(got) != 1 { t.Errorf("lagging = %v, want the leader named again once its stamp is stale", got) } @@ -46,13 +46,21 @@ func TestLaggingWritersNamesWhoWillIgnoreTheOverride(t *testing.T) { // The leader records itself as "host= pid=" while capabilities are // keyed by host alone. Comparing those directly reported every current-version // daemon as needing an upgrade — the warning would have been pure noise. -func TestLaggingWritersMatchesTheLeadersHostForm(t *testing.T) { +func TestLaggingWritersMatchesTheLeadersProcessIdentity(t *testing.T) { now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) st := New() st.Leader = &LeaderLease{Owner: "host=cachyos pid=1234", ExpiresAt: now.Add(time.Minute)} - st.NoteWriter("cachyos", CapsRepoOverrides, now) + st.NoteWriter("host=cachyos pid=1234", CapsRepoOverrides, now) if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 0 { t.Errorf("lagging = %v, want none — the leader IS the capable writer", got) } + + // A new CLI on the same machine must not vouch for an old daemon: that is + // the ordinary upgrade, and per-host keying would hide exactly it. + st.NoteWriter("host=cachyos pid=9999", CapsRepoOverrides, now) + st.Leader = &LeaderLease{Owner: "host=cachyos pid=1", ExpiresAt: now.Add(time.Minute)} + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 1 { + t.Errorf("lagging = %v, want the un-upgraded daemon named", got) + } }