diff --git a/AGENTS.md b/AGENTS.md index 399e9461..3d184f87 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,10 @@ Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state added survives being read and rewritten by an older one — which is what makes adding one safe without another dual-write, and without a schema bump (an unknown version auto-reinitialises and would erase the fleet's rounds). + `FireSlot.HoldUntil` is the compatibility exception for the binary immediately + before nested slot tolerance: its deadline is mirrored at the tolerant top + level and in the legacy pacing anchor so that writer both preserves and + honours it during a rolling deployment. - `internal/engine/` — PURE decision logic, `now` passed in, no ctx/gh: `DecideFire` (the single fire owner), `Progress` (fired/reviewing round transitions), `Completion` (the one "is the round done?"), `BlockingFindings` @@ -72,9 +76,25 @@ r.Head == head → skip`. A completed round stays as the "this head was reviewed dedup marker. A rate-limited requeue parks the round in `awaiting_retry` (keeping its head/attempts/history), it does not delete a fired marker. +The one exception to that skip is `Round.ReviewersChanged`: a reviewer change +requeues the repository's completed rounds, but only for PRs that are open — +marking the closed ones instead of handing Pump dead work. A marked round is +reopened by whichever enqueue path next sees the PR alive, so reopening a PR +picks up the requirements it missed while it was shut. + The global `FireSlot` allows ≤1 concurrent fire fleet-wide (CAS). A bot ack releases the slot while the review keeps running (the round moves to -`reviewing`); the round itself stays open until `Completion` is done. +`reviewing`); the round itself stays open until `Completion` is done. The +converse does not hold: convergence alone never releases the slot. A repository +whose required set omits the primary converges as soon as its co-reviewers +answer, and completing there would hand the slot to the next PR while the +metered command is still unanswered — so a round that spent the quota stays +`fired` until the primary acknowledges or its in-flight timeout expires. Staying +`fired` holds the slot only while the round exists, and converging is what tells +the agent to push, so the loop also stamps `FireSlot.HoldUntil`: the hold belongs +to the command, not to the head it was posted at, and it outlives the supersede +that the success it reported invites. Every fire gate asks `SlotHeld`, not +"is there a round holding it". ## observe → decide → apply diff --git a/README.md b/README.md index 5bf6c377..c20cd553 100644 --- a/README.md +++ b/README.md @@ -386,6 +386,10 @@ crq feedback # current normalized findings as JSON, WITHOUT trigger crq threads # every unresolved thread, outdated included 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 (either flag alone) +crq reviewers clear # back to the fleet default + crq dismiss [...] --reason "" # account for a finding with no thread crq autoreview # ⭐ review ALL open PRs automatically, rate-coordinated # (--no-incremental = first review only; --once = single pass for cron) @@ -514,7 +518,9 @@ the same fire step as the CodeRabbit one. Bugbot and Macroscope default to `self already auto-review every push: crq stays silent unless a bot it has seen working misses the current head for longer than `CRQ_COBOT__GRACE`. In every mode crq suppresses the trigger when the bot auto-reviews, has already reviewed the head, has a check run in flight, or has a live command on the -PR — so no bot is ever double-asked. +PR — so no bot is ever double-asked. A mode you set explicitly wins over requiredness, per-repo +requiredness included: `crq reviewers set` gives a required bot the trigger its registry default +would have had, but never overrides a `never` you configured yourself. A co-reviewer that joins a round on its own (an actionable comment, a review, a check run) gates that round dynamically: convergence waits for it even though it isn't required. An exhaustion notice — for diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 4f4c9278..fb7961c6 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 "threads": repo, pr, ok := repoPR(args[1:]) if !ok { @@ -402,6 +408,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 (either flag alone) + crq reviewers clear go back to the fleet default crq threads list every unresolved review thread, outdated ones included crq dismiss [...] --reason "" account for a finding GitHub gives you no thread to close @@ -558,6 +568,35 @@ 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, outside that queue. Budget is not requiredness: whether +a round WAITS for a reviewer is --required, whatever the reviewer costs. + + set --bots chooses the co-reviewers; --required chooses which reviewers + gate convergence. Either flag alone updates only its own half. An + empty --bots means none here, which is a different answer from not + setting it at all; an empty --required is refused, because a round + that gates on nobody converges before any reviewer runs. + 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 "threads": fmt.Print(`crq threads @@ -778,6 +817,100 @@ 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": + // 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 { + 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: + // `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 { + 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/auto.go b/internal/crq/auto.go index cd69e125..2a3b7e68 100644 --- a/internal/crq/auto.go +++ b/internal/crq/auto.go @@ -3,8 +3,6 @@ package crq import ( "context" "errors" - "fmt" - "os" "strings" "time" @@ -21,7 +19,10 @@ type AutoOptions struct { var errLostLeadership = errors.New("lost autoreview leadership mid-pass") func (s *Service) AutoReview(ctx context.Context, opts AutoOptions) error { - owner := fmt.Sprintf("host=%s pid=%d", s.cfg.Host, os.Getpid()) + // The same identity capabilities are recorded under, so LaggingWriters can + // ask whether the process holding the lease understands what it is deciding + // from. Spelling it out here again would let the two drift apart. + owner := s.cfg.WriterID() token := randomToken() for { held, err := s.acquireLeader(ctx, owner, token) @@ -299,23 +300,51 @@ func (s *Service) needsReview(ctx context.Context, state State, repo string, pr return false, "", err } if r := state.Round(repo, pr); r != nil && r.Head == head { - return false, head, nil + // Unless the round is a marker for reviewers that changed while this PR + // was closed: it answered for a set that no longer gates the head, and + // enqueueBatch reopens it. Deciding here rather than falling through to + // the live checks is also the only correct answer — those read Review + // objects, which a co-reviewer answering by comment never submits. + if !r.ReviewersChanged { + return false, head, nil + } + s.logEnqueue(repo, pr, head, "reviewer configuration changed while the pr was closed") + return true, head, nil } reviews, err := s.gh.ListReviews(ctx, repo, 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/codex_replay_test.go b/internal/crq/codex_replay_test.go index 5373d08d..041965ef 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/config.go b/internal/crq/config.go index 98d6cdf8..9cb6d5dc 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -44,10 +44,22 @@ 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. - Reviewers []Reviewer + 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 + // 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 @@ -184,13 +196,19 @@ 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 // 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 @@ -310,12 +328,18 @@ func listEnv(env map[string]string, key, fallback string) []string { // convergence via RequiredBots membership. Trigger and SelfHealGrace shape // when crq may post Command (see engine.DecideCoPost). type CoBotConfig struct { - Login string - Name string - Command string - Trigger engine.TriggerMode - Required bool - SelfHealGrace time.Duration + Login string + Name string + Command string + Trigger engine.TriggerMode + // TriggerExplicit records that CRQ_COBOT__TRIGGER named this mode, + // rather than it falling out of the registry defaults. The fleet parse lets + // that value win over the registry's required trigger, so a per-repo override + // promoting the bot to required must not quietly overrule it either — an + // operator who disabled a bot's command asked for that on every repository. + TriggerExplicit bool + Required bool + SelfHealGrace time.Duration } // parseCoBots resolves the enabled co-reviewers from CRQ_COBOTS (default all @@ -326,6 +350,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, explicit := base.Trigger, false + switch v := engine.TriggerMode(strings.ToLower(strings.TrimSpace(env["CRQ_COBOT_"+key+"_TRIGGER"]))); v { + case engine.TriggerNever, engine.TriggerSelfHeal, engine.TriggerAlways: + trigger, explicit = v, true + } + if command == "" { + // No trigger command means crq can never post one, whatever the mode. + trigger = engine.TriggerNever + } + base.Command, base.Trigger, base.TriggerExplicit = command, trigger, explicit + 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 @@ -359,40 +421,36 @@ func parseCoBots(env map[string]string, requiredBots []string) ([]CoBotConfig, e if !enabled[co.Name] && !required { continue } - // 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 - 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) - } - 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 - } - 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), - }) + out = append(out, resolveCoBot(env, co, required)) } 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 { @@ -480,3 +538,20 @@ func ownerOf(repo string) string { owner, _, _ := strings.Cut(repo, "/") return owner } + +// processRun distinguishes this RUN of crq from an earlier one that happened to +// get the same pid. Host and pid do not identify a process over time — a +// containerized daemon restarts as pid 1, and ordinary pids are reused — so +// without it a replacement process inherits its predecessor's recorded +// capabilities: an old binary restarted into a capable one's pid would vouch for +// itself, and LaggingWriters would report no incompatible writer while that +// daemon ignores every repository override. +var processRun = randomToken()[:8] + +// WriterID identifies this PROCESS in the shared state, and is what the +// autoreview leader records as its owner. 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 run=%s", c.Host, os.Getpid(), processRun) +} diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index c6fea22d..d6a79023 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -62,6 +62,16 @@ type FeedbackReport struct { // window entirely — it cannot apply to a round that never spends quota. PrimaryUnavailable bool `json:"primary_review_unavailable,omitempty"` PrimaryUnavailableReason string `json:"primary_review_unavailable_reason,omitempty"` + // PrimaryAckPending reports that the round still holds the fire slot for a + // review command the primary has not acknowledged. A required set that omits + // the primary converges without it, so the loop must not read convergence as + // permission to release the slot. Not serialized: the feedback JSON contract + // is frozen. + PrimaryAckPending bool `json:"-"` + // config is the reviewer configuration identity that produced this report. + // A completion write revalidates it under CAS so an override committed after + // observation cannot turn stale convergence into a permanent dedup marker. + config Config } // CoReviewerStatus is one co-reviewer's observed state for the current head. @@ -92,7 +102,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 } @@ -110,6 +121,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe HeadRef: pull.Head.Ref, HeadRepo: NormalizeRepo(pull.Head.Repo.FullName), ReviewedBy: map[string]bool{}, + config: cfg, Findings: []dialect.Finding{}, CheckedAt: now, } @@ -127,13 +139,14 @@ 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 + report.PrimaryAckPending = engine.PrimaryAckPending(completionRound, obs.eng, cfg.policy()) // 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 @@ -159,8 +172,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 } @@ -170,7 +183,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. @@ -472,7 +485,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 @@ -483,7 +500,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)" @@ -562,7 +579,7 @@ func (s *Service) Loop(ctx context.Context, repo string, pr int) (FeedbackReport // timeout so the caller retries later instead. A skipped wait result is // terminal, not retryable, so preserve it as a skipped report. if waitResult.Action == "skipped" { - s.completeWaitRound(ctx, repo, pr, "") + s.completeWaitRound(ctx, repo, pr, "", false, nil) } return FeedbackReport{Status: status, Repo: NormalizeRepo(repo), PR: pr, Head: waitResult.Head, Reason: waitResult.Reason, ReviewedBy: map[string]bool{}, Findings: []dialect.Finding{}}, code, nil } @@ -615,7 +632,7 @@ func (s *Service) Loop(ctx context.Context, repo string, pr int) (FeedbackReport report.Status = "feedback" if allReviewed(report.ReviewedBy) { report.Reason = "all required reviewers finished; address findings, push once, and resolve threads" - s.completeWaitRound(ctx, repo, pr, head) + s.completeWaitRound(ctx, repo, pr, head, report.PrimaryAckPending, &report.config) } else if report.CodeRabbitDeferred && engine.DoneExceptWithEvidence(report.ReviewedBy, s.cfg.Bot, dialect.CodexBotLogin) { // Degraded round: every required bot except the rate-limited // CodeRabbit has finished. These findings are this round's work — @@ -653,7 +670,7 @@ func (s *Service) Loop(ctx context.Context, repo string, pr int) (FeedbackReport } if s.cfg.SettleWindow <= 0 || s.clock().Sub(settledAt) >= s.cfg.SettleWindow { if report.Converged { - s.completeWaitRound(ctx, repo, pr, head) + s.completeWaitRound(ctx, repo, pr, head, report.PrimaryAckPending, &report.config) } return report, 0, nil } @@ -708,7 +725,7 @@ func (s *Service) Loop(ctx context.Context, repo string, pr int) (FeedbackReport // A degraded round must not be completed on timeout: marking the head // reviewed would silently cancel the still-owed CodeRabbit review. if !report.CodeRabbitDeferred { - s.completeWaitRound(ctx, repo, pr, head) + s.completeWaitRound(ctx, repo, pr, head, false, &report.config) } if len(report.Findings) > 0 { report.Status = "feedback" @@ -816,7 +833,17 @@ func (s *Service) pushWaitDeadline(ctx context.Context, repo string, pr int, hea // completeWaitRound ends the wait by completing the fired/reviewing round. The // completed round remains as the "this head was reviewed" dedup marker, so a // subsequent enqueue/needsReview at the same head is deduped rather than re-fired. -func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, head string) { +// +// holdUnacked leaves alone a round still holding the fire slot for a review +// command the primary has not acknowledged: with the primary outside the +// required set the round converges without it, and completing here would release +// the slot to the next pull request while this one's metered command is +// unanswered — the same release Progress now withholds. The round stays fired +// and any Pump finishes it, on the acknowledgement or on the in-flight timeout, +// and the hold is stamped on the slot so the push this success invites cannot +// drop it along with the superseded round. Only the convergence callers pass it; +// a timed-out or never fired wait must still end. +func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, head string, holdUnacked bool, cfg *Config) error { repo = NormalizeRepo(repo) changed := false state, err := s.store.Update(ctx, func(st *State) error { @@ -828,6 +855,26 @@ func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, he if head != "" && r.Head != head { return ErrNoChange } + if cfg != nil && overrideChanged(st, repo, *cfg) { + return ErrNoChange + } + if holdUnacked && st.FireSlot != nil && st.FireSlot.Key == QueueKey(repo, pr) { + // Leaving the round fired keeps the slot only while the round exists, + // and converging is precisely the loop's signal to push: the head + // advance that follows archives this round, and the slot would be + // released with the command it was taken for still unanswered. So + // record the hold on the slot itself, where it survives the supersede. + // Bounded by the in-flight window, the deadline Progress gives up at. + if r.FiredAt != nil { + until := r.FiredAt.UTC().Add(s.cfg.InflightTimeout) + if until.After(s.clock()) && (st.FireSlot.HoldUntil == nil || st.FireSlot.HoldUntil.Before(until)) { + st.HoldSlotUntil(until) + changed = true + return nil + } + } + return ErrNoChange + } if err := r.Complete(); err != nil { return err } @@ -840,11 +887,12 @@ func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, he if s.log != nil { s.log.Printf("warning: failed to clear feedback wait for %s#%d: %v", repo, pr, err) } - return + return err } if changed { s.sync(ctx, state) } + return nil } // waitToFire runs Wait (enqueue + coordinated fire), riding out GitHub REST rate diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index b28682ca..eba01edc 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -2201,3 +2201,79 @@ func TestExcludeSkipNoticeIsTargeted(t *testing.T) { // Which occurrences of a stable id count as settled is decided by the head each // one names, not by this filter — see // TestCoReplayBugbotSiblingSettlementUsesHead, which drives the real threads. + +// The loop completes the round it waited on, which releases the fire slot. With +// the primary outside the required set the round converges without it, so that +// completion would hand the next pull request a second concurrent metered +// command while this one's is still unanswered. Holding leaves the round fired +// for a Pump to finish — on the acknowledgement, or on the in-flight timeout. +func TestCompleteWaitRoundHoldsAnUnacknowledgedFireSlot(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + now := time.Now().UTC() + repo, pr, head := "o/r", 4, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 12) + + svc.completeWaitRound(ctx, repo, pr, head, true, &cfg) + if got := roundPhase(t, store, repo, pr); got != PhaseFired { + t.Fatalf("phase = %s, want the round left fired while the primary is silent", got) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.FireSlot == nil { + t.Fatal("the fire slot must stay held until the primary acknowledges") + } + + svc.completeWaitRound(ctx, repo, pr, head, false, &cfg) + if got := roundPhase(t, store, repo, pr); got != PhaseCompleted { + t.Fatalf("phase = %s, want an acknowledged round completed as before", got) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.FireSlot != nil { + t.Fatalf("completing the round must release the slot, got %+v", st.FireSlot) + } +} + +// Converging is the loop's signal to push, and the push supersedes the round the +// held slot points at. Leaving the round fired is therefore not enough on its +// own: the replacement round archives its predecessor, Normalize drops a slot no +// round holds, and the next Pump spends the account allowance on a second review +// command while the first is still unanswered. +func TestAHeldFireSlotSurvivesTheHeadAdvanceItInvites(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.InflightTimeout = time.Hour + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + now := time.Now().UTC() + repo, pr, head := "o/r", 5, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 12) + svc.completeWaitRound(ctx, repo, pr, head, true, &cfg) + + // The push: the agent acted on the success the loop just reported. + if _, err := store.Update(ctx, func(st *State) error { + _, err := st.Supersede(repo, pr, "bbbbbbbb2", now) + return err + }); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if !st.SlotHeld(now) { + t.Fatalf("fire slot = %+v, want it still held for the unanswered command", st.FireSlot) + } + // Bounded by the in-flight window, so an unanswered command cannot wedge the + // queue for longer than crq would have waited for it anyway. + if st.SlotHeld(now.Add(cfg.InflightTimeout + time.Minute)) { + t.Error("the hold must expire with the in-flight window it was taken for") + } +} diff --git a/internal/crq/next.go b/internal/crq/next.go index 572bb79e..e778971f 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -71,6 +71,16 @@ func (s *Service) Next(ctx context.Context, repo string, pr int) (NextReport, er if err != nil { return report, err } + // Convergence can release the head before the optional primary acknowledges + // the metered command. Preserve that slot before telling the caller to push: + // the push supersedes this round, so leaving the hold attached only to the + // live round would let Normalize release it while the review is still in + // flight. + if action.Kind == engine.ActionPush && feedback.PrimaryAckPending { + if err := s.completeWaitRound(ctx, repo, pr, report.Head, true, &feedback.config); err != nil { + return report, err + } + } // Undrained feedback for THIS head: publish nothing. Another review of the // same head would spend account quota to be told what the caller is already diff --git a/internal/crq/next_test.go b/internal/crq/next_test.go index 6424c8c8..a566bdf3 100644 --- a/internal/crq/next_test.go +++ b/internal/crq/next_test.go @@ -123,6 +123,40 @@ func TestNextDrivesAReviewRound(t *testing.T) { } } +func TestNextPreservesUnacknowledgedSlotBeforeReturningPush(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newCodexReplayFixture(t, base, func(cfg *Config) { + cfg.RequiredBots = []string{dialect.CodexBotLogin} + cfg.FeedbackBots = cfg.RequiredBots + }) + repo, pr, head := "owner/repo", 506, "aaaaaaaa1" + f.openPull(repo, pr, head) + f.setCommitDate(head, base.Add(-time.Minute)) + f.setLocalWork(false, "") + f.next(repo, pr) + + f.clk.advance(time.Minute) + f.gh.mu.Lock() + review := ghapi.Review{ + ID: 900, CommitID: head, State: "COMMENTED", + SubmittedAt: f.clk.now(), Body: "[review body]", + } + review.User.Login = dialect.CodexBotLogin + f.gh.reviews[fakeKey(repo, pr)] = append(f.gh.reviews[fakeKey(repo, pr)], review) + f.gh.mu.Unlock() + f.setLocalWork(true, "uncommitted changes in the working tree") + + report := f.next(repo, pr) + f.wantAction(report, engine.ActionPush) + st, _, err := f.store.Load(f.ctx) + if err != nil { + t.Fatal(err) + } + if st.FireSlot == nil || st.FireSlot.HoldUntil == nil || !st.SlotHeld(f.clk.now()) { + t.Fatalf("push returned without preserving the unanswered primary slot: %+v", st.FireSlot) + } +} + // `next` advances the queue as a side effect, and that step owns the review // fire — so the two halves of drain-first are both properties of ONE decision: // undrained feedback for the current head must not buy another review of that diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go new file mode 100644 index 00000000..b79886cf --- /dev/null +++ b/internal/crq/repoconfig.go @@ -0,0 +1,411 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// 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"` + // 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. +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) { + repo = NormalizeRepo(repo) + // The same shape check set and clear do. A malformed target reads no + // override, so reporting it would answer with the fleet default and exit 0 — + // telling the caller its typo is a repository crq is following. + if err := checkRepoShape(repo); err != nil { + return ReviewerView{}, err + } + st, _, err := s.store.Load(ctx) + if err != nil { + return ReviewerView{}, err + } + 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 + 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 err := checkRepoShape(repo); err != nil { + return ReviewerView{}, err + } + if coBots == nil && required == nil { + // Neither half was named, so there is nothing to set. Writing anyway would + // stamp a fresh UpdatedAt, and that timestamp IS the override's identity: + // applyFire revalidates against it, so a no-op call would discard every + // in-flight fire decision for the repository. + return s.Reviewers(ctx, repo) + } + // 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 _, co := range dialect.KnownCoReviewers() { + known[dialect.NormalizeBotName(co.Login)] = co.Login + known[strings.ToLower(strings.TrimSpace(co.Name))] = co.Login + } + 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 := allowed[dialect.NormalizeBotName(name)] + if !ok { + login, ok = allowed[strings.ToLower(name)] + } + if !ok { + return nil, fmt.Errorf("%s: unknown reviewer %q (known: %s)", what, name, strings.Join(knownLogins(allowed), ", ")) + } + out = append(out, login) + } + return out, nil + } + + // 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 + } + setCoBots = resolved + } + 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 { + allowed[k] = v + } + resolved, err := resolve(allowed, required, "--required") + if err != nil { + return ReviewerView{}, err + } + setRequired = resolved + } + + // Read before the write: the requeue below may only touch live pull requests, + // and the CAS closure cannot ask GitHub. A failed lookup fails the whole + // command, which is retryable — writing the override and skipping the requeue + // would strand exactly the PRs the requeue exists for. + open, err := s.openPRs(ctx, repo) + if err != nil { + return ReviewerView{}, err + } + now := s.clock().UTC() + state, err := s.store.Update(ctx, func(st *State) error { + ov, _ := st.RepoOverride(repo) + beforeOverride := ov + if coBots != nil { + ov.CoBots, ov.SetCoBots = setCoBots, true + } + if required != nil { + ov.Required, ov.SetRequired = setRequired, true + } + if ov.SetCoBots == beforeOverride.SetCoBots && + ov.SetRequired == beforeOverride.SetRequired && + sameLogins(ov.CoBots, beforeOverride.CoBots) && + sameLogins(ov.Required, beforeOverride.Required) { + return ErrNoChange + } + 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), open) + 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) + // 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 err := checkRepoShape(repo); err != nil { + return ReviewerView{}, err + } + open, err := s.openPRs(ctx, repo) + if err != nil { + return ReviewerView{}, err + } + 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, open) + return nil + }) + if err != nil && !errors.Is(err, ErrNoChange) { + return ReviewerView{}, err + } + if err == nil { + s.sync(ctx, state) + } + return s.Reviewers(ctx, repo) +} + +// checkRepoShape is the one repository-shape check every reviewers path applies, +// so reading a target can never succeed where setting it would fail. +// +// Exactly two nonempty components: "owner/", "/name" and "owner/name/extra" name +// no repository, and the read path never contacts GitHub — so a typo would +// otherwise print the fleet default and exit 0, reading as a report about a +// project crq follows. +func checkRepoShape(repo string) error { + owner, name, ok := strings.Cut(repo, "/") + if !ok || owner == "" || name == "" || strings.Contains(name, "/") { + return fmt.Errorf("repo must be owner/name, got %q", repo) + } + return nil +} + +// 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 +} + +// 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 updates this repository's live rounds when the +// effective reviewer set 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. +// +// Optional co-reviewers count too: once one has participation evidence, +// Completion waits for it, and its trigger/self-heal needs an active round to +// run. Existing active rounds receive the same one-shot force as reopened ones: +// an in-flight self-heal reviewer with no activity cannot otherwise know it was +// just required. Completed rounds are reopened only when their pull request is +// still open. Rounds are never deleted, so a repository's merged and closed PRs +// stay behind as completed dedup markers: requeueing those would hand Pump +// hundreds of dead rounds to observe and drop one per tick, ahead of every real +// one, and a stranded PR is by definition an open one. +// +// A closed PR's round is marked instead of requeued, because closed is not +// final: reopened at the same head, its completed round would be the dedup +// marker that hides the requirement the operator added while it was shut. The +// mark costs nothing until an enqueue finds the PR alive again. +// +// 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, open map[int]bool) { + beforeCo := before.reviewerLogins(func(r Reviewer) bool { return !r.Metered() }) + afterCo := after.reviewerLogins(func(r Reviewer) bool { return !r.Metered() }) + if sameLogins(before.RequiredBots, after.RequiredBots) && sameLogins(beforeCo, afterCo) { + return + } + for _, round := range st.Rounds { + if NormalizeRepo(round.Repo) != NormalizeRepo(repo) { + continue + } + forced := forcedCoReviewers(round.ForceCoReviewers, before, after) + switch round.Phase { + case PhaseQueued, PhaseReserved, PhaseFired, PhaseReviewing, PhaseAwaitingRetry: + if !sameLogins(round.ForceCoReviewers, forced) { + updated := round + updated.ForceCoReviewers = forced + st.PutRound(updated) + } + continue + case PhaseCompleted: + default: + continue + } + if !open[round.PR] { + if !round.ReviewersChanged || !sameLogins(round.ForceCoReviewers, forced) { + marked := round + marked.ReviewersChanged = true + marked.ForceCoReviewers = forced + st.PutRound(marked) + } + continue + } + reopened := round + if err := reopened.Reopen(); err != nil { + continue + } + reopened.ForceCoReviewers = forced + st.PutRound(reopened) + if s.log != nil { + s.log.Printf("reviewers: requeued %s#%d@%s — the reviewer set changed", round.Repo, round.PR, round.Head) + } + } +} + +// forcedCoReviewers carries the one exceptional trigger an existing round +// needs. A newly enabled or required self-heal bot has no activity on that head, +// so its normal mode cannot decide it missed anything; force it once without +// changing the repository's steady-state trigger policy. +func forcedCoReviewers(existing []string, before, after Config) []string { + var out []string + for _, cb := range after.CoBots { + if cb.Trigger != engine.TriggerSelfHeal || cb.Command == "" { + continue + } + newlyEnabled := !containsCoBot(before.CoBots, cb.Login) + newlyRequired := cb.Required && !containsBot(before.RequiredBots, cb.Login) + if containsBot(existing, cb.Login) || newlyEnabled || newlyRequired { + out = append(out, cb.Login) + } + } + return out +} + +func containsCoBot(bots []CoBotConfig, login string) bool { + for _, cb := range bots { + if sameBot(cb.Login, login) { + return true + } + } + return false +} + +// requeueIfReviewersChanged reopens a completed round that a reviewer change +// marked while its pull request was closed, and reports whether it did. This is +// the other half of reopenForChangedReviewers: the enqueue paths call it when +// the PR turns out to be alive after all, which is the moment the round stops +// being a harmless dead marker and starts being the thing that strands the PR. +func requeueIfReviewersChanged(st *State, r *Round) bool { + if r == nil || r.Phase != PhaseCompleted || !r.ReviewersChanged { + return false + } + if err := r.Reopen(); err != nil { + return false + } + st.PutRound(*r) + return true +} + +// openPRs is the set of repo's currently open pull request numbers — the only +// ones a reviewer change can strand. +func (s *Service) openPRs(ctx context.Context, repo string) (map[int]bool, error) { + open := map[int]bool{} + err := s.gh.EachOpenPR(ctx, repo, true, func(pr ghapi.SearchPR) (bool, error) { + if NormalizeRepo(pr.Repo) == repo { + open[pr.Number] = true + } + return false, nil + }) + if err != nil { + return nil, err + } + return open, nil +} + +// 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 new file mode 100644 index 00000000..e53cd8e2 --- /dev/null +++ b/internal/crq/repoconfig_test.go @@ -0,0 +1,915 @@ +package crq + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// 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") + } +} + +// 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) + } +} + +func TestOverrideRecomputesImplicitTriggerAfterRemovingRequiredness(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot],codex", + }) + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"codex"}, + SetCoBots: true, + Required: []string{"coderabbitai[bot]"}, + SetRequired: true, + }) + if len(got.CoBots) != 1 { + t.Fatalf("CoBots = %+v, want Codex still enabled", got.CoBots) + } + if got.CoBots[0].Required || got.CoBots[0].Trigger != engine.TriggerNever { + t.Fatalf("Codex = %+v, want optional with its implicit never trigger", got.CoBots[0]) + } + + explicit := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot],codex", + "CRQ_COBOT_CODEX_TRIGGER": "always", + }) + got = explicit.ForRepo(RepoReviewers{ + CoBots: []string{"codex"}, + SetCoBots: true, + Required: []string{"coderabbitai[bot]"}, + SetRequired: true, + }) + if got.CoBots[0].Trigger != engine.TriggerAlways { + t.Fatalf("explicit Codex trigger = %q, want always preserved", got.CoBots[0].Trigger) + } +} + +// 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") + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"codex"}, SetCoBots: true, + Required: []string{dialect.CodexBotLogin}, SetRequired: true, + }) + primaryPresent(got, "override naming only codex") + for _, cp := range got.policy().CoReviewerPolicies() { + if sameBot(cp.Login, got.Bot) { + t.Errorf("primary %q leaked into the dynamic co-review policies", got.Bot) + } + } +} + +// 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("a newly required self-heal bot gets an immediate trigger", func(t *testing.T) { + // Bugbot normally self-heals only after activity proves it missed a head. + // On a completed head reopened by this override it has no activity yet, so + // preserving selfheal would complete the round without ever asking it. + fleet := isolatedConfig(t, map[string]string{"CRQ_COBOTS": "bugbot"}) + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"bugbot"}, SetCoBots: true, + Required: []string{"bugbot"}, SetRequired: true, + }) + for _, cb := range got.CoBots { + if cb.Name != "bugbot" { + continue + } + if cb.Trigger != engine.TriggerSelfHeal { + t.Errorf("bugbot = %+v: the repository's steady-state policy must stay selfheal", cb) + } + forced := forcedCoReviewers(nil, fleet, got) + if len(forced) != 1 || !sameBot(forced[0], cb.Login) { + t.Fatalf("forced reviewers = %v, want newly required bugbot", forced) + } + round := Round{ + Repo: "o/r", PR: 3, Head: "aaaaaaaa1", Phase: PhaseQueued, + ForceCoReviewers: forced, + } + obs := engine.Observation{ + Head: "aaaaaaaa1", Open: true, + Reviews: []engine.ReviewSeen{{ + Bot: got.Bot, Commit: "aaaaaaaa1", + }}, + } + decision := engine.DecideFire(engine.Global{SlotFree: true}, round, obs, time.Now().UTC(), got.policy()) + if decision.Verdict != engine.FireCoOnly || len(decision.PostCo) != 1 || + !sameBot(decision.PostCo[0], cb.Login) { + t.Errorf("decision = %+v, want one immediate bugbot trigger", decision) + } + return + } + t.Fatalf("CoBots = %+v, want bugbot enabled", got.CoBots) + }) + + t.Run("a newly enabled optional self-heal bot gets an immediate trigger", func(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "", + "CRQ_COBOT_CODEX_REQUIRED": "false", + "CRQ_COBOT_BUGBOT_REQUIRED": "false", + "CRQ_COBOT_MACROSCOPE_REQUIRED": "false", + }) + got := fleet.ForRepo(RepoReviewers{CoBots: []string{"bugbot"}, SetCoBots: true}) + for _, cb := range got.CoBots { + if cb.Name != "bugbot" { + continue + } + if cb.Required { + t.Fatalf("bugbot = %+v, want an optional reviewer", cb) + } + forced := forcedCoReviewers(nil, fleet, got) + if len(forced) != 1 || !sameBot(forced[0], cb.Login) { + t.Fatalf("forced reviewers = %v, want newly enabled bugbot", forced) + } + round := Round{ + Repo: "o/r", PR: 4, Head: "aaaaaaaa1", Phase: PhaseQueued, + ForceCoReviewers: forced, + } + obs := engine.Observation{ + Head: "aaaaaaaa1", Open: true, + Reviews: []engine.ReviewSeen{{ + Bot: got.Bot, Commit: "aaaaaaaa1", + }}, + } + decision := engine.DecideFire(engine.Global{SlotFree: true}, round, obs, time.Now().UTC(), got.policy()) + if decision.Verdict != engine.FireCoOnly || len(decision.PostCo) != 1 || + !sameBot(decision.PostCo[0], cb.Login) { + t.Errorf("decision = %+v, want one immediate optional bugbot trigger", decision) + } + return + } + t.Fatalf("CoBots = %+v, want bugbot enabled", got.CoBots) + }) + + 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) + gh := newFakeGitHub() + repo, pr, head := "o/r", 3, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, store, nil) + 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) + } +} + +func TestSettingIdenticalReviewersPreservesOverrideIdentity(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + first := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + svc.now = func() time.Time { return first } + + if _, err := svc.SetReviewers(ctx, "o/r", []string{"codex"}, []string{"codex"}); err != nil { + t.Fatal(err) + } + before, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + original, ok := before.RepoOverride("o/r") + if !ok || original.UpdatedAt == nil { + t.Fatalf("override = %#v, want a timestamped override", original) + } + + svc.now = func() time.Time { return first.Add(time.Hour) } + if _, err := svc.SetReviewers(ctx, "o/r", []string{"codex"}, []string{"codex"}); err != nil { + t.Fatal(err) + } + after, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + unchanged, ok := after.RepoOverride("o/r") + if !ok || unchanged.UpdatedAt == nil || !unchanged.UpdatedAt.Equal(*original.UpdatedAt) { + t.Fatalf("UpdatedAt = %v, want the original identity %v", unchanged.UpdatedAt, original.UpdatedAt) + } + if after.Rev != before.Rev { + t.Fatalf("state revision = %d, want unchanged %d", after.Rev, before.Rev) + } +} + +// Optional co-reviewers can become convergence gates after participation +// evidence appears. Enabling one therefore needs the same active round as +// changing the statically required set, so its trigger and bounded wait run. +func TestChangingEnabledCoReviewersReopensACompletedRound(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{"CRQ_COBOTS": ""}) + store := NewMemoryStore(cfg) + gh := newFakeGitHub() + repo, pr, head := "o/r", 4, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, store, nil) + seedRound(t, store, cfg, repo, pr, head, PhaseCompleted, time.Now().UTC(), 11) + + if _, err := svc.SetReviewers(ctx, repo, []string{"bugbot"}, nil); 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 newly enabled co-reviewer can run", round) + } +} + +func TestChangingReviewersForcesExistingActiveRounds(t *testing.T) { + for _, phase := range []Phase{PhaseQueued, PhaseFired, PhaseReviewing, PhaseAwaitingRetry} { + t.Run(string(phase), func(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{"CRQ_COBOTS": ""}) + store := NewMemoryStore(cfg) + gh := newFakeGitHub() + repo, pr, head := "o/r", 4, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, store, nil) + seedPhase := phase + if phase == PhaseAwaitingRetry { + seedPhase = PhaseReviewing + } + seedRound(t, store, cfg, repo, pr, head, seedPhase, time.Now().UTC(), 11) + if phase == PhaseAwaitingRetry { + if _, err := store.Update(ctx, func(st *State) error { + r := st.Round(repo, pr) + retryAt := time.Now().UTC().Add(time.Hour) + if err := r.AwaitRetry(retryAt, "test", time.Now().UTC()); err != nil { + return err + } + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + } + + if _, err := svc.SetReviewers(ctx, repo, []string{"bugbot"}, []string{"bugbot"}); 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.ForceCoReviewer(dialect.BugbotLogin) { + t.Fatalf("round = %#v, want newly required bugbot forced on the active round", round) + } + if round.Phase != phase { + t.Fatalf("phase = %s, want active round to stay %s", round.Phase, phase) + } + }) + } +} + +// Dedupe writes the "every required reviewer answered this head" marker, so a +// required reviewer added between the decision and the write must void it. The +// round is still queued when the operator's write lands — nothing to requeue — +// and dedupe posts nothing, so only this revalidation catches the race. +func TestDedupeIsRevalidatedAgainstAReviewerChange(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + now := time.Now().UTC() + repo, settled, raced, head := "o/r", 9, 10, "aaaaaaaa1" + seedRound(t, store, cfg, repo, settled, head, PhaseQueued, now, 0) + seedRound(t, store, cfg, repo, raced, head, PhaseQueued, now, 0) + dedupe := engine.FireDecision{Verdict: engine.FireDedupe, Reason: "bot already reviewed head"} + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + round := *st.Round(repo, settled) + if _, err := svc.applyFire(ctx, svc.cfgFor(st, repo), round, engine.Observation{}, dedupe, now); err != nil { + t.Fatal(err) + } + if got := roundPhase(t, store, repo, settled); got != PhaseCompleted { + t.Fatalf("phase = %s, want an unraced dedupe to still complete the round", got) + } + + // Now the override lands after the decision was made from svc.cfg. + 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, raced) + result, err := svc.applyFire(ctx, svc.cfg, round, engine.Observation{}, dedupe, now) + if err != nil { + t.Fatal(err) + } + if result.Action != "lost_race" { + t.Errorf("action = %q, want the stale dedupe reported as a lost race", result.Action) + } + if got := roundPhase(t, store, repo, raced); got != PhaseQueued { + t.Fatalf("phase = %s, want the round left queued so the new reviewer is asked", got) + } +} + +// hookedStore runs a hook once, immediately before the next Update's mutation +// reaches the store — the window a fire effect's own CAS has to close, and the +// one a check made before the write cannot see into. +type hookedStore struct { + StateStore + hook func() +} + +func (h *hookedStore) Update(ctx context.Context, mutate func(*State) error) (State, error) { + if h.hook != nil { + hook := h.hook + h.hook = nil + hook() + } + return h.StateStore.Update(ctx, mutate) +} + +// The revalidation has to happen in the write itself: an override that lands +// after any pre-flight re-read but before the dedupe commits would otherwise +// still complete the round under the reviewer set it replaced. +func TestDedupeIsRevalidatedInsideItsWrite(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + hooked := &hookedStore{StateStore: store} + svc := NewService(cfg, newFakeGitHub(), hooked, nil) + now := time.Now().UTC() + repo, pr, head := "o/r", 11, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseQueued, now, 0) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + round := *st.Round(repo, pr) + hooked.hook = func() { + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, []string{"codex"}); err != nil { + t.Error(err) + } + } + dedupe := engine.FireDecision{Verdict: engine.FireDedupe, Reason: "bot already reviewed head"} + result, err := svc.applyFire(ctx, svc.cfgFor(st, repo), round, engine.Observation{}, dedupe, now) + if err != nil { + t.Fatal(err) + } + if result.Action != "lost_race" { + t.Errorf("action = %q, want the dedupe voided by the override that beat its write", result.Action) + } + if got := roundPhase(t, store, repo, pr); got != PhaseQueued { + t.Fatalf("phase = %s, want the round left queued so the new reviewer is asked", got) + } +} + +// Every reviewers path rejects a target that is not exactly owner/name. The read +// path never contacts GitHub, so a typo would otherwise report the fleet default +// and exit 0 — a plausible answer about a project that does not exist. +func TestReviewersRejectAnIncompleteRepository(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + for _, repo := range []string{"", "owner", "owner/", "/name", "owner/name/extra"} { + if _, err := svc.Reviewers(ctx, repo); err == nil { + t.Errorf("Reviewers(%q) = nil error, want the malformed target refused", repo) + } + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, nil); err == nil { + t.Errorf("SetReviewers(%q) = nil error, want the malformed target refused", repo) + } + if _, err := svc.ClearReviewers(ctx, repo); err == nil { + t.Errorf("ClearReviewers(%q) = nil error, want the malformed target refused", repo) + } + } + if _, err := svc.Reviewers(ctx, "owner/name"); err != nil { + t.Errorf("Reviewers(owner/name) = %v, want a well-formed target accepted", err) + } +} + +// A closed PR is not a dead PR. Requirements that change while it is shut must +// still reach it if it is reopened at the same head, or its completed round is +// the dedup marker that hides the reviewer the operator added. +func TestAReopenedPRPicksUpRequirementsChangedWhileItWasClosed(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + gh := newFakeGitHub() + repo, manual, auto, head := "o/r", 7, 8, "aaaaaaaa1" + svc := NewService(cfg, gh, store, nil) + for _, pr := range []int{manual, auto} { + seedRound(t, store, cfg, repo, pr, head, PhaseCompleted, time.Now().UTC(), 11) + } + + // Both PRs are closed while the required set changes: no round is requeued. + 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) + } + for _, pr := range []int{manual, auto} { + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseCompleted { + t.Fatalf("round = %#v, want a closed PR left alone", round) + } + if !round.ReviewersChanged { + t.Fatalf("round = %#v, want the change recorded for a reopen", round) + } + } + + // Both come back at the same head — the manual path first. + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = head + "bcdef1234" + gh.pulls[fakeKey(repo, manual)] = pull + gh.pulls[fakeKey(repo, auto)] = pull + result, err := svc.Enqueue(ctx, repo, manual) + if err != nil { + t.Fatal(err) + } + if !result.Queued || result.Deduped { + t.Fatalf("enqueue = %#v, want the reopened round queued rather than deduped", result) + } + + // And the autoreview path, which never calls Enqueue. + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + need, gotHead, err := svc.needsReview(ctx, st, repo, auto, true) + if err != nil { + t.Fatal(err) + } + if !need || gotHead != head { + t.Fatalf("needsReview = %v %q, want the reopened PR enqueued at %q", need, gotHead, head) + } + if err := svc.enqueueBatch(ctx, []queueCandidate{{Repo: repo, PR: auto, Head: gotHead}}); err != nil { + t.Fatal(err) + } + + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + for _, pr := range []int{manual, auto} { + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseQueued || round.Head != head { + t.Fatalf("round = %#v, want it queued at the same head so the new reviewer is asked", round) + } + if round.ReviewersChanged { + t.Errorf("round = %#v, want the mark cleared — it answers under the current requirements now", round) + } + } +} + +// Rounds are never deleted, so a repository's merged and closed PRs stay behind +// as completed dedup markers. Requeueing those on a reviewer change would put +// every dead round ahead of real work — Pump observes and drops them one per +// tick — and no closed PR can be stranded by a reviewer it will never get. +func TestChangingRequirementsLeavesClosedPRsAlone(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + gh := newFakeGitHub() + repo, open, merged := "o/r", 4, 5 + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: open}} + svc := NewService(cfg, gh, store, nil) + seedRound(t, store, cfg, repo, open, "aaaaaaaa1", PhaseCompleted, time.Now().UTC(), 11) + seedRound(t, store, cfg, repo, merged, "bbbbbbbb2", PhaseCompleted, time.Now().UTC(), 12) + + 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) + } + if round := st.Round(repo, open); round == nil || round.Phase != PhaseQueued { + t.Errorf("open round = %#v, want it requeued so the new reviewer can be asked", round) + } + if round := st.Round(repo, merged); round == nil || round.Phase != PhaseCompleted { + t.Errorf("merged round = %#v, want it left completed — a closed PR cannot be stranded", round) + } +} + +// The other half of TestOverrideKeepsTheSilencedPrimaryEntry: when the fleet +// configuration never carried an entry for a registry-bot primary (not enabled, +// not fleet-required), a repository that gates on it has to gain one. Skipping +// it as "not a co-reviewer here" leaves observation without that bot's wording +// and check-run hooks, so a check-only clean result is never fetched and the +// primary this repository waits for stays pending until the round times out. +func TestOverrideAddsTheSilencedPrimaryEntryTheFleetLacked(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{ + // Bugbot's login is cursor[bot]: naming it primary makes the primary a + // registry bot, and neither CRQ_COBOTS nor CRQ_REQUIRED_BOTS mentions it. + "CRQ_BOT": "cursor[bot]", + "CRQ_REVIEW_CMD": "bugbot run", + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": dialect.CodexBotLogin, + "CRQ_COBOT_CODEX_CMD": "@codex review", + }) + for _, cb := range fleet.CoBots { + if sameBot(cb.Login, fleet.Bot) { + t.Fatalf("fleet CoBots = %+v already carry the primary; this test needs the gap", fleet.CoBots) + } + } + + got := fleet.ForRepo(RepoReviewers{ + Required: []string{"cursor[bot]", dialect.CodexBotLogin}, SetRequired: true, + }) + var primary *CoBotConfig + for i, cb := range got.CoBots { + if sameBot(cb.Login, got.Bot) { + primary = &got.CoBots[i] + } + } + if primary == nil { + t.Fatalf("CoBots = %+v, want the primary's registry entry so its evidence can be read", got.CoBots) + } + // Silenced: it is asked as the primary, and asking the same bot twice is the + // bug the fleet parse silences it for. + if primary.Trigger != engine.TriggerNever { + t.Errorf("primary entry = %+v, want trigger never — it is triggered as the primary", *primary) + } + if !got.coChecksRelevant() { + t.Error("the primary's check runs must be fetched; a check-only clean result is its whole answer") + } + // And it is still exactly one reviewer, account-metered, not a second free one. + if p, ok := got.Primary(); !ok || !sameBot(p.Login, got.Bot) { + t.Errorf("Primary() = %+v/%v, want the configured primary", p, ok) + } + metered := 0 + for _, r := range got.Reviewers { + if r.Metered() { + metered++ + } + } + if metered != 1 { + t.Errorf("reviewers = %+v, want exactly one metered entry", got.Reviewers) + } +} + +// An operator who sets CRQ_COBOT__TRIGGER=never has disabled that bot's +// command everywhere: the fleet parse already lets that value win over the +// registry's required trigger. A repository requiring the bot must not be able +// to turn it back on — that would post the command the operator switched off. +func TestOverrideKeepsAnExplicitNeverTrigger(t *testing.T) { + fleet := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_COBOT_CODEX_TRIGGER": "never", + }) + got := fleet.ForRepo(RepoReviewers{ + CoBots: []string{"codex"}, SetCoBots: true, + Required: []string{"codex"}, SetRequired: true, + }) + for _, cb := range got.CoBots { + if cb.Name != "codex" { + continue + } + if cb.Trigger != engine.TriggerNever { + t.Errorf("codex = %+v, want the explicit never kept — the fleet parse honours it too", cb) + } + return + } + t.Fatalf("CoBots = %+v, want codex required here", got.CoBots) +} + +// Completing a round writes the "this head was reviewed" dedup marker, and +// reopenForChangedReviewers deliberately leaves an in-flight round alone — it is +// already going to answer. A required reviewer added between Progress deciding +// and that write is therefore caught by neither: the marker dedupes the head +// under the set that no longer gates it, and nothing re-fires it. +func TestCompletionIsRevalidatedInsideItsWrite(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + hooked := &hookedStore{StateStore: store} + gh := newFakeGitHub() + repo, pr, head := "o/r", 12, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, hooked, nil) + now := time.Now().UTC() + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 21) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + stale := svc.cfgFor(st, repo) + hooked.hook = func() { + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, []string{"codex"}); err != nil { + t.Error(err) + } + } + done := engine.Transition{Outcome: engine.OutComplete, Reason: "feedback complete"} + if _, err := hooked.Update(ctx, func(st *State) error { + return svc.applyTransition(st, st.Round(repo, pr), done, now, stale) + }); err != nil { + t.Fatal(err) + } + if got := roundPhase(t, store, repo, pr); got != PhaseFired { + t.Fatalf("phase = %s, want the stale completion dropped so the new reviewer is still asked", got) + } +} + +// Feedback and its completion write have the same decision/write window as +// Progress. A reviewer override landing in that window must leave the in-flight +// round open so the next observation can ask the newly required reviewer. +func TestFeedbackCompletionIsRevalidatedInsideItsWrite(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CoBots = codexCoBots(nil) + store := NewMemoryStore(cfg) + hooked := &hookedStore{StateStore: store} + gh := newFakeGitHub() + repo, pr, head := "o/r", 13, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, hooked, nil) + now := time.Now().UTC() + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 21) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + stale := svc.cfgFor(st, repo) + hooked.hook = func() { + if _, err := svc.SetReviewers(ctx, repo, []string{"codex"}, []string{"codex"}); err != nil { + t.Error(err) + } + } + svc.completeWaitRound(ctx, repo, pr, head, false, &stale) + if got := roundPhase(t, store, repo, pr); got != PhaseFired { + t.Fatalf("phase = %s, want stale feedback completion dropped", got) + } +} + +// The self-heal sweep claims a co-reviewer's trigger post under CAS and then +// posts it. The claim is what authorizes the post, so — as in fireCoOnly — the +// reviewer set that chose the bot has to still be the configured one when the +// claim commits, or `crq reviewers set` reports a bot disabled while crq is +// asking it for a review. +func TestSelfHealTriggerIsRevalidatedInsideItsClaim(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.RequiredBots = []string{cfg.Bot, dialect.CodexBotLogin} + cfg.CoBots = codexCoBots(cfg.RequiredBots) + store := NewMemoryStore(cfg) + hooked := &hookedStore{StateStore: store} + gh := newFakeGitHub() + repo, pr, head := "o/r", 13, "aaaaaaaa1" + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + svc := NewService(cfg, gh, hooked, nil) + now := time.Now().UTC() + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 22) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + round := *st.Round(repo, pr) + stale := svc.cfgFor(st, repo) + // Nothing observed for the head yet, so an always-mode trigger wants posting. + obs := engine.Observation{Head: head, Open: true} + hooked.hook = func() { + // The operator drops the co-reviewer, keeping only the primary. + if _, err := svc.SetReviewers(ctx, repo, []string{}, []string{cfg.Bot}); err != nil { + t.Error(err) + } + } + svc.selfHealCoReviewers(ctx, stale, round, obs, now) + + for _, body := range gh.posted { + if strings.Contains(body, "@codex") { + t.Errorf("posted %q for a co-reviewer the operator had just removed", body) + } + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if c := st.Round(repo, pr).Co(dialect.CodexBotLogin); c.ClaimedAt != nil || c.CommandID != 0 { + t.Errorf("codex bookkeeping = %+v, want no claim for a removed reviewer", c) + } +} diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index bcfe7dfe..c851f3c0 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -175,3 +175,195 @@ 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 + + // 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 { + enabled = append(enabled, cb.Login) + } + } + for _, login := range required { + if sameBot(login, c.Bot) || containsBot(enabled, login) { + continue + } + if _, ok := dialect.CoReviewerByName(login); ok { + enabled = append(enabled, login) + } + } + + 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 = reconcileTrigger(cb) + default: + continue + } + keep = append(keep, cb) + have[dialect.NormalizeBotName(cb.Login)] = true + } + // A primary that is itself a registry bot but that the fleet neither enabled + // nor required has no entry to preserve above — and this repository naming it + // is exactly when its evidence has to be read. Add the silenced entry the + // fleet parse would have carried: without it observation loses that bot's + // wording and check-run hooks, so a check-only clean result is never fetched + // and the primary stays pending until the round times out. Recording it here + // also keeps the loop below from adding it as an ordinary co-reviewer, which + // would ask the same bot twice. + if primary := c.Bot; primary != "" && !have[dialect.NormalizeBotName(primary)] && + (containsBot(required, primary) || containsBot(enabled, primary)) { + if cb, ok := c.knownCoBot(primary); ok { + cb.Required = containsBot(required, primary) + cb.Trigger = engine.TriggerNever // triggered as the primary; asking twice is the bug + keep = append(keep, cb) + have[dialect.NormalizeBotName(primary)] = 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 + } + // 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, reconcileTrigger(cb)) + 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 }) + if !c.FeedbackBotsExplicit { + out.FeedbackBots = out.reviewerLogins(func(r Reviewer) bool { return r.Required || !r.Metered() }) + } + return out +} + +// reconcileTrigger recomputes a co-reviewer's implicit mode after a repository +// override changes its requiredness. +// +// 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 changing requiredness must therefore handle both directions: promote a +// newly required Codex, and demote one a repository made optional. Explicit +// operator settings still win. Reviewer-change reopens carry their one-shot +// force on the Round instead, so the override does not permanently change how +// that bot runs on later heads. +func reconcileTrigger(cb CoBotConfig) CoBotConfig { + if cb.TriggerExplicit { + return cb + } + co, ok := dialect.CoReviewerByName(cb.Name) + if !ok { + return cb + } + cb.Trigger = triggerMode(co.DefaultTrigger, engine.TriggerSelfHeal) + if cb.Required && co.RequiredTrigger != "" { + cb.Trigger = triggerMode(co.RequiredTrigger, cb.Trigger) + } + if cb.Command == "" { + cb.Trigger = engine.TriggerNever + } + return cb +} + +// 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 { + 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 + } + 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. It is therefore called +// from INSIDE the CAS mutation that commits the decision, where the state it +// reads is the state the write lands on; a separate read beforehand would leave +// the same window one step earlier. +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/reviewers_test.go b/internal/crq/reviewers_test.go index 2f3e4517..74b4173c 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 2fdf87bd..06cfd9eb 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -9,7 +9,6 @@ import ( "io" "net/url" "sort" - "strconv" "strings" "time" @@ -128,6 +127,14 @@ func (s *Service) Enqueue(ctx context.Context, repo string, pr int) (EnqueueResu now := s.clock() r := st.Round(repo, pr) if r != nil && r.Head == head { + // A PR reopened after its reviewers changed: the completed round is a + // marker for requirements that no longer hold, so it goes back in the + // queue instead of deduping the enqueue that would have asked. + if requeueIfReviewersChanged(st, r) { + result.Queued = true + result.Seq = r.Seq + return nil + } switch r.Phase { case PhaseFired, PhaseReviewing, PhaseCompleted: result.Deduped = true @@ -181,6 +188,9 @@ func (s *Service) enqueueBatch(ctx context.Context, items []queueCandidate) erro repo := NormalizeRepo(it.Repo) if r := st.Round(repo, it.PR); r != nil { if r.Head == it.Head { + if requeueIfReviewersChanged(st, r) { + added++ + } continue } if _, err := st.Supersede(repo, it.PR, it.Head, now); err != nil { @@ -298,21 +308,22 @@ 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 } - // A blocked front of the queue must not starve later PRs of resolutions that - // spend NO CodeRabbit quota — a co-reviewer defer, or a summary-only round - // whose review is never coming from CodeRabbit at all. + // A blocked or orphaned-slot-held front of the queue must not starve later + // PRs of resolutions that spend NO CodeRabbit quota — a co-reviewer defer, + // or a summary-only round whose review is never coming from CodeRabbit at all. accountBlocked := global.BlockedUntil != nil && global.BlockedUntil.After(now) - if decision.Verdict == engine.FireNo && accountBlocked { + if decision.Verdict == engine.FireNo && (accountBlocked || st.SlotHeld(now)) { if free, handled, err := s.sweepQuotaFree(ctx, st, now, next.Repo, next.PR); err != nil { return PumpResult{}, err } else if handled { @@ -344,7 +355,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 +373,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 +410,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 } @@ -560,7 +572,7 @@ func quotaFreeVerdict(v engine.FireVerdict) bool { func (s *Service) global(st State, now time.Time) engine.Global { return engine.Global{ - SlotFree: st.SlotRound() == nil, + SlotFree: !st.SlotHeld(now), BlockedUntil: st.Account.BlockedUntil, LastFired: st.LastFired, } @@ -573,12 +585,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 } @@ -590,7 +603,7 @@ func (s *Service) progressSlotRound(ctx context.Context, slot Round) (PumpResult if r == nil || st.FireSlot == nil || st.FireSlot.Token != slot.Token { return ErrNoChange } - return s.applyTransition(st, r, tr, now) + return s.applyTransition(st, r, tr, now, cfg) }) if err != nil { return PumpResult{}, err @@ -608,10 +621,23 @@ func (s *Service) progressSlotRound(ctx context.Context, slot Round) (PumpResult // applyTransition applies a fired/reviewing round's engine Transition to state: // the round transition plus any fire-slot release and account-quota block. -func (s *Service) applyTransition(st *State, r *Round, tr engine.Transition, now time.Time) error { +// +// cfg is the configuration Progress decided from, revalidated here for the same +// reason applyFire revalidates its verdicts — this runs inside the CAS mutation, +// where the state it reads is the state the write lands on. +func (s *Service) applyTransition(st *State, r *Round, tr engine.Transition, now time.Time, cfg Config) error { key := QueueKey(r.Repo, r.PR) switch tr.Outcome { case engine.OutComplete: + // The completed round is the "this head was reviewed" dedup marker, and + // reopenForChangedReviewers deliberately leaves an in-flight round alone — + // it is already going to answer. A reviewer change that commits between + // the decision and this write would therefore be answered by neither: the + // marker dedupes the head under the set that no longer gates it. Drop the + // stale transition; the next pump decides again under the new set. + if overrideChanged(st, r.Repo, cfg) { + return ErrNoChange + } if err := r.Complete(); err != nil { return err } @@ -646,6 +672,7 @@ func (s *Service) applyTransition(st *State, r *Round, tr engine.Transition, now func releaseSlot(st *State, key string) { if st.FireSlot != nil && st.FireSlot.Key == key { st.FireSlot = nil + st.ClearSlotHold() } } @@ -712,15 +739,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 } @@ -732,7 +760,7 @@ func (s *Service) sweepReviewing(ctx context.Context, st State, now time.Time) ( if r == nil || !sameRound(r, *target) || (r.Phase != PhaseFired && r.Phase != PhaseReviewing) { return ErrNoChange } - return s.applyTransition(st, r, tr, now) + return s.applyTransition(st, r, tr, now, cfg) }) if err != nil { return st, err @@ -749,24 +777,34 @@ 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) { +// +// 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, skip one they have just required, or record that a head is reviewed +// by a set that no longer gates it — so the verdicts the reviewer configuration +// decides revalidate it (overrideChanged) inside their own commit point, the CAS +// mutation that claims the trigger, reserves the slot or writes the dedupe +// marker. Checking it in a separate read here would leave exactly the window it +// is meant to close: SetReviewers commits in between, and the mutation goes on +// to apply the decision the new configuration would not have made. +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) + return s.dedupeRound(ctx, cfg, 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 } @@ -817,7 +855,7 @@ func (s *Service) abandonRound(ctx context.Context, round Round, reason, action // dedupeRound completes a not-yet-fired round because the bot already reviewed // its head, leaving the completed round as the dedupe marker (v2's Fired[key]). -func (s *Service) dedupeRound(ctx context.Context, round Round, now time.Time, reason string) (PumpResult, error) { +func (s *Service) dedupeRound(ctx context.Context, cfg Config, round Round, now time.Time, reason string) (PumpResult, error) { result := PumpResult{Action: "deduped", Repo: round.Repo, PR: round.PR, Head: round.Head, Reason: reason} if s.cfg.DryRun { return result, nil @@ -826,7 +864,13 @@ func (s *Service) dedupeRound(ctx context.Context, round Round, now time.Time, r updated, err := s.store.Update(ctx, func(st *State) error { deduped = false r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || !r.FireEligible(now) { + // The marker this writes asserts that everyone the repository gates on has + // already answered the head, so a reviewer change committed since the + // decision voids it — and voids it permanently: dedupe posts nothing, so + // SetReviewers cannot see it coming, the round is still queued when the + // override lands (requeuing a queued round is a no-op), yet the marker is + // exactly what stops the newly required reviewer from ever being asked. + if !sameRound(r, round) || !r.FireEligible(now) || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } if err := r.Dedupe(now); err != nil { @@ -872,7 +916,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 @@ -889,14 +933,16 @@ func (s *Service) fireRound(ctx context.Context, round Round, obs engine.Observa recorded := false updated, err := s.store.Update(ctx, func(st *State) error { recorded = false - if st.FireSlot != nil { + if st.SlotHeld(now) { return ErrNoChange } r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || !r.FireEligible(now) { + // postCo below was chosen by cfg's reviewers; a change since means the + // claims written here are for a set the operator has replaced. + if !sameRound(r, round) || !r.FireEligible(now) || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } - if err := r.Reserve(token, s.cfg.Host, now); err != nil { + if err := r.Reserve(token, s.cfg.WriterID(), now); err != nil { return err } if err := r.Fire(adoptID, firedAt); err != nil { @@ -920,7 +966,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 } @@ -944,21 +990,23 @@ 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 } - // Reserve the slot, then post the command. + // Reserve the slot, then post the command. The reservation is this fire's + // commit point — nothing is posted before it — so it is where the reviewer + // configuration the decision used is revalidated. reserved, err := s.store.Update(ctx, func(st *State) error { - if st.FireSlot != nil { + if st.SlotHeld(now) { return ErrNoChange } r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || !r.FireEligible(now) { + if !sameRound(r, round) || !r.FireEligible(now) || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } - if err := r.Reserve(token, s.cfg.Host, now); err != nil { + if err := r.Reserve(token, s.cfg.WriterID(), now); err != nil { return err } st.FireSlot = &FireSlot{Key: key, Token: token, Since: now} @@ -1005,7 +1053,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}) } } @@ -1057,7 +1105,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 @@ -1066,7 +1114,9 @@ func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, updated, err := s.store.Update(ctx, func(st *State) error { claimed = claimed[:0] r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || !r.FireEligible(now) { + // The claim is what authorizes the posts below, so the reviewer set that + // chose them must still be the configured one when it commits. + if !sameRound(r, round) || !r.FireEligible(now) || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } for _, login := range logins { @@ -1096,7 +1146,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}) } } @@ -1113,7 +1163,7 @@ func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, // still-queued round through Reserve (a pure phase transition — no // global FireSlot is registered) so the park is a legal edge. if r.FireEligible(now) { - if rerr := r.Reserve(randomToken(), s.cfg.Host, now); rerr != nil { + if rerr := r.Reserve(randomToken(), s.cfg.WriterID(), now); rerr != nil { return rerr } } @@ -1144,7 +1194,7 @@ func (s *Service) fireCoOnly(ctx context.Context, round Round, logins []string, return ErrNoChange } if r.FireEligible(now) { - if err := r.Reserve(randomToken(), s.cfg.Host, now); err != nil { + if err := r.Reserve(randomToken(), s.cfg.WriterID(), now); err != nil { return err } if err := r.Fire(posts[0].id, firedAt); err != nil { @@ -1209,7 +1259,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 @@ -1237,7 +1287,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 { @@ -1340,8 +1390,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{} } @@ -1370,8 +1420,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. @@ -1408,7 +1458,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 { @@ -1434,7 +1484,10 @@ func (s *Service) fireCoDeferred(ctx context.Context, round Round, d engine.Fire updated, err := s.store.Update(ctx, func(st *State) error { adopted, claimed = 0, claimed[:0] r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || (r.Phase != PhaseQueued && r.Phase != PhaseAwaitingRetry) { + // As in fireCoOnly: the claims and adoptions written here name the + // co-reviewers cfg chose, so a reviewer change since voids them. + if !sameRound(r, round) || (r.Phase != PhaseQueued && r.Phase != PhaseAwaitingRetry) || + overrideChanged(st, round.Repo, cfg) { return ErrNoChange } changed := false @@ -1489,7 +1542,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") @@ -1504,12 +1557,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 } @@ -1521,12 +1574,15 @@ func (s *Service) selfHealCoReviewers(ctx context.Context, round Round, obs engi // not serialized by the fire slot, so two concurrent pumps observing an // unset command would otherwise both post. A claim older than // triggerClaimTTL is stale (the poster died mid-flight) and may be - // re-claimed. + // re-claimed. As in fireCoOnly, the claim is what authorizes the post, so + // the reviewer set that chose this bot must still be the configured one + // when it commits — otherwise a bot `crq reviewers set` has just removed + // is asked for a review anyway. login := cp.Login claimed := false updated, err := s.store.Update(ctx, func(st *State) error { r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || r.Co(login).CommandID != 0 { + if !sameRound(r, round) || r.Co(login).CommandID != 0 || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } if c := r.Co(login); c.ClaimedAt != nil && now.Sub(c.ClaimedAt.UTC()) < triggerClaimTTL { @@ -1541,7 +1597,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) } } @@ -1878,11 +1934,15 @@ func (s *Service) sync(ctx context.Context, state State) { func randomToken() string { var buf [16]byte if _, err := io.ReadFull(rand.Reader, buf[:]); err != nil { - return strconv.FormatInt(time.Now().UnixNano(), 16) + return fallbackToken(time.Now()) } return hex.EncodeToString(buf[:]) } +func fallbackToken(now time.Time) string { + return fmt.Sprintf("%016x", uint64(now.UnixNano())) +} + // isCommentCapError reports whether err is GitHub's hard cap of 2500 comments per // issue ("Commenting is disabled on issues with more than 2500 comments"). func isCommentCapError(err error) bool { diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 715abb3d..4cfec43c 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -373,7 +373,16 @@ func (s *adoptionRaceStore) Load(context.Context) (State, Revision, error) { func (s *adoptionRaceStore) Update(_ context.Context, mutate func(*State) error) (State, error) { state := cloneState(s.loadState) - state.FireSlot = &FireSlot{Key: "owner/repo#99", Token: "other", Since: time.Now().UTC()} + now := time.Now().UTC() + other, err := state.NewRound("owner/repo", 99, "999999999", now) + if err != nil { + return State{}, err + } + if err := other.Reserve("other", "other-host", now); err != nil { + return State{}, err + } + state.PutRound(*other) + state.FireSlot = &FireSlot{Key: "owner/repo#99", Token: "other", Since: now} if err := mutate(&state); err != nil { if errors.Is(err, ErrNoChange) { return state, nil @@ -686,6 +695,14 @@ func firingConfig() Config { } } +func TestFallbackTokenIsSafeToShorten(t *testing.T) { + for _, now := range []time.Time{time.Unix(0, 0), time.Unix(0, 1)} { + if got := fallbackToken(now); len(got) < 8 { + t.Fatalf("fallbackToken(%v) = %q, want at least 8 characters", now, got) + } + } +} + func TestEnqueueIsIdempotentAndPumpFiresOnce(t *testing.T) { ctx := context.Background() cfg := firingConfig() @@ -761,6 +778,12 @@ func TestPumpPersistsPostedReviewAfterTransientStateFailure(t *testing.T) { if r == nil || r.Phase != PhaseFired || r.CommandID == 0 { t.Fatalf("posted review metadata was not persisted after retry: %#v", r) } + // The firing PROCESS, in the form capabilities are recorded under: a bare + // hostname here can never match a writer entry, so LaggingWriters would name + // this very process as needing an upgrade for as long as the fire lasts. + if r.ByHost != cfg.WriterID() { + t.Errorf("ByHost = %q, want the writer id %q", r.ByHost, cfg.WriterID()) + } if state.FiredMarker("owner/repo", 12) != "abcdef123" { t.Fatalf("fired marker was not persisted after retry") } @@ -1415,6 +1438,46 @@ func TestPumpTreatsExistingReviewAdoptionRaceAsLostRace(t *testing.T) { } } +func TestFireRoundRechecksOrphanedSlotHold(t *testing.T) { + for _, post := range []bool{false, true} { + name := "adopt" + if post { + name = "post" + } + t.Run(name, func(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + store := NewMemoryStore(cfg) + now := time.Now().UTC() + seedRound(t, store, cfg, "owner/repo", 12, "abcdef123", PhaseQueued, now, 0) + if _, err := store.Update(ctx, func(st *State) error { + until := now.Add(cfg.InflightTimeout) + st.FireSlotHoldUntil = &until + return nil + }); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + round := *st.Round("owner/repo", 12) + svc := NewService(cfg, gh, store, nil) + got, err := svc.fireRound(ctx, cfg, round, engine.Observation{}, post, 77, now.Add(-time.Minute), "test", nil, now) + if err != nil { + t.Fatal(err) + } + if got.Action != "lost_race" { + t.Fatalf("action = %q, want lost_race while an orphaned hold is active", got.Action) + } + if len(gh.posted) != 0 { + t.Fatalf("posted %d review commands while an orphaned hold is active", len(gh.posted)) + } + }) + } +} + func TestRecordFireResetsRecordedAcrossRetry(t *testing.T) { cfg := firingConfig() svc := NewService(cfg, newFakeGitHub(), retryNoChangeStore{cfg: cfg}, nil) @@ -2361,7 +2424,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 +2445,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 +2458,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) } @@ -2572,6 +2635,70 @@ func TestPumpSweepsQuotaFreeRoundWhileTheSlotIsHeld(t *testing.T) { } } +func TestPumpSweepsQuotaFreeRoundWhileAnOrphanedHoldIsActive(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.RequiredBots = []string{"coderabbitai[bot]", dialect.CodexBotLogin} + cfg.CoBots = codexCoBots(cfg.RequiredBots) + cfg.FeedbackBots = cfg.RequiredBots + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + + frontSHA := "111111111" + front := ghapi.Pull{State: "open"} + front.Head.SHA = frontSHA + "abcdef0" + gh.pulls[fakeKey("o/front", 10)] = front + + backSHA := "2222222222222222" + back := ghapi.Pull{State: "open"} + back.Head.SHA = backSHA + gh.pulls[fakeKey("o/back", 20)] = back + walkthrough := ghapi.IssueComment{ + ID: 900, + Body: corpusMessage(t, "coderabbit/summary-only-free-plan.md"), + CreatedAt: now.Add(-time.Minute), + UpdatedAt: now.Add(-time.Minute), + } + walkthrough.User.Login = "coderabbitai[bot]" + gh.comments[fakeKey("o/back", 20)] = []ghapi.IssueComment{walkthrough} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + svc.now = func() time.Time { return now } + seedRound(t, store, cfg, "o/front", 10, frontSHA, PhaseQueued, now.Add(-time.Minute), 0) + if _, err := svc.Enqueue(ctx, "o/back", 20); err != nil { + t.Fatal(err) + } + if _, err := store.Update(ctx, func(st *State) error { + holdUntil := now.Add(cfg.InflightTimeout) + st.FireSlotHoldUntil = &holdUntil + frontRound := st.Round("o/front", 10) + frontRound.SetCoCommand(dialect.CodexBotLogin, 701, now.Add(-time.Minute)) + st.PutRound(*frontRound) + return nil + }); err != nil { + t.Fatal(err) + } + + res, err := svc.Pump(ctx) + if err != nil { + t.Fatal(err) + } + if res.Repo != "o/back" || res.PR != 20 { + t.Fatalf("the quota-free round behind an orphaned hold must be rescued, got %#v", res) + } + st, _, _ := store.Load(ctx) + if round := st.Round("o/back", 20); round == nil || round.Phase == PhaseQueued { + t.Fatalf("the quota-free round must leave the queue, got %#v", round) + } + for _, posted := range gh.posted { + if strings.Contains(posted, cfg.ReviewCommand) { + t.Fatalf("the rescue must not spend primary review quota, posted=%v", gh.posted) + } + } +} + // TestWaitResolvesSummaryOnlyWithoutTheQueue pins the architectural rule the // dogfood exposed: a summary-only round is NOT a queue citizen. The Seq FIFO and // the FireSlot exist solely to serialize CodeRabbit's account-wide review limit, diff --git a/internal/crq/state.go b/internal/crq/state.go index a9da1d28..9a70bc3e 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 ( @@ -33,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 ( @@ -49,6 +54,7 @@ func (c Config) storeConfig() StoreConfig { Timezone: c.Timezone, Scope: c.Scope, CoReviewers: c.coReviewerSummary(), + Host: c.WriterID(), MinInterval: c.MinInterval, } } @@ -121,6 +127,12 @@ func (c Config) policy() engine.Policy { RateLimitCoDegrade: c.RateLimitCoDegrade, } for _, cb := range c.CoBots { + // A registry-backed primary keeps a silenced CoBots entry so observation + // can use that registry's wording and check hooks. It is still the + // primary, not a dynamic co-reviewer convergence gate. + if sameBot(cb.Login, c.Bot) { + continue + } p.CoReviewers = append(p.CoReviewers, engine.CoReviewerPolicy{ Login: cb.Login, Command: cb.Command, diff --git a/internal/engine/completion.go b/internal/engine/completion.go index 7334efe2..f337fcf9 100644 --- a/internal/engine/completion.go +++ b/internal/engine/completion.go @@ -247,6 +247,44 @@ func CommandHasCompletionReply(obs Observation, p Policy, commandID int64) bool return false } +// PrimaryCompletedRound reports whether the primary completed THIS round with +// evidence that counts as its review of the head: either the clean-summary +// verdict Completion accepts, or a completion reply to the round's own command. +// +// It is the gate that lets a reopened round skip re-asking a primary that +// already answered, so it holds to Completion's evidence rules rather than to +// the weaker adoption question: CommandHasCompletionReply deliberately omits +// the prior-review requirement and the failed-summary guard, and a reply failing +// either is not a review of this head. Deduping on it writes a completed marker +// that every later same-head check skips, so a wrong yes here is one nothing +// recovers from. +func PrimaryCompletedRound(r state.Round, obs Observation, p Policy) bool { + if r.FiredAt == nil { + return false + } + cutoff := r.FiredAt.UTC() + for _, ev := range obs.Events { + if ev.Kind == dialect.EvNoAction && sameBot(ev.Bot, p.Bot) && + notBefore(ev.ObservedTime(), cutoff) { + return true + } + } + if r.CommandID == 0 { + return false + } + if !botHasAnyReview(obs.Reviews, p.Bot) { + return false + } + for _, reply := range commandReplies(obs, p) { + if reply.commandID == r.CommandID && reply.completion && + notBefore(reply.commandAt, cutoff) && + !stateSince(obs, p, reply.commandAt, dialect.EvInProgress, dialect.EvRateLimited, dialect.EvPaused, dialect.EvFailed) { + return true + } + } + return false +} + func botHasAnyReview(reviews []ReviewSeen, bot string) bool { for _, review := range reviews { if sameBot(review.Bot, bot) { diff --git a/internal/engine/coreview.go b/internal/engine/coreview.go index ca7f63c3..44ff5d1e 100644 --- a/internal/engine/coreview.go +++ b/internal/engine/coreview.go @@ -419,6 +419,9 @@ func DecideCoPost(r state.Round, obs Observation, cp CoReviewerPolicy, commandPr case TriggerAlways: return !obs.co(cp.Login).AutoActive case TriggerSelfHeal: + if r.ForceCoReviewer(cp.Login) { + return true + } co := obs.co(cp.Login) if !co.AutoActive && !co.ActiveThisRound { return false diff --git a/internal/engine/coreview_test.go b/internal/engine/coreview_test.go index 2d154bbd..5fa0d028 100644 --- a/internal/engine/coreview_test.go +++ b/internal/engine/coreview_test.go @@ -99,6 +99,18 @@ func TestDecideCoPostTriggerMatrix(t *testing.T) { } } +func TestDecideCoPostHonorsReviewerChangeForce(t *testing.T) { + cp := CoReviewerPolicy{ + Login: "cursor[bot]", + Command: "@cursor review", + Trigger: TriggerSelfHeal, + } + round := state.Round{ForceCoReviewers: []string{"cursor[bot]"}} + if !DecideCoPost(round, Observation{}, cp, false, time.Now().Add(-time.Minute), time.Now()) { + t.Fatal("a newly required self-heal reviewer must be triggered without prior activity") + } +} + // TestCompletionCheckEvidence pins step 2b and the generic dynamic gate: a // required check-bearing bot converges on its completed check run alone (the // silent-clean Bugbot round), an in-progress check does not, and a diff --git a/internal/engine/engine_test.go b/internal/engine/engine_test.go index 98fc08ba..7d867072 100644 --- a/internal/engine/engine_test.go +++ b/internal/engine/engine_test.go @@ -150,6 +150,30 @@ func TestFailedSummaryParksTheRound(t *testing.T) { } } +func TestFailedOptionalPrimaryCompletesConvergedRound(t *testing.T) { + r := firedRound(t, "abcdef123") + now := t0.Add(time.Minute) + p := withCodex(policy, "@codex review") + p.RequiredBots = []string{dialect.CodexBotLogin} + obs := Observation{ + Head: "abcdef123", Open: true, + Reviews: []ReviewSeen{{ + Bot: dialect.CodexBotLogin, ReviewID: 4, + Commit: "abcdef1234567890", SubmittedAt: t0.Add(30 * time.Second), + }}, + Events: []dialect.BotEvent{{ + Kind: dialect.EvFailed, Bot: "coderabbitai[bot]", CommentID: 900, + CreatedAt: t0.Add(-time.Hour), UpdatedAt: t0.Add(9 * time.Second), + }}, + } + if got := Completion(r, obs, p); !got.Done { + t.Fatalf("the only required reviewer answered; want convergence, got %+v", got) + } + if tr := Progress(r, state.AccountQuota{}, obs, now, p); tr.Outcome != OutComplete { + t.Fatalf("an optional primary failure must finish a converged round, got %+v", tr) + } +} + func TestReviewAtHeadCompletesRound(t *testing.T) { r := firedRound(t, "abcdef123") obs := Observation{Head: "abcdef123", Open: true, @@ -166,6 +190,56 @@ func TestReviewAtHeadCompletesRound(t *testing.T) { } } +// TestFireSlotHeldUntilPrimaryAcknowledges separates convergence from slot +// release. A repository may leave the primary out of its required set (required +// Codex only), and then the round converges the moment Codex answers — while the +// account-metered command it posted is still unacknowledged. Completing there +// would hand the slot to the next PR mid-review, which is the serialization the +// whole queue exists for. +func TestFireSlotHeldUntilPrimaryAcknowledges(t *testing.T) { + p := withCodex(policy, "@codex review") + p.RequiredBots = []string{dialect.CodexBotLogin} + obs := Observation{Head: "abcdef123", Open: true, + Reviews: []ReviewSeen{{Bot: dialect.CodexBotLogin, ReviewID: 4, Commit: "abcdef1234567890", SubmittedAt: t0.Add(time.Minute)}}} + + r := firedRound(t, "abcdef123") + if got := Completion(r, obs, p); !got.Done { + t.Fatalf("the only required reviewer answered; want a converged round, got %+v", got) + } + if tr := Progress(r, state.AccountQuota{}, obs, t0.Add(2*time.Minute), p); tr.Outcome != KeepWaiting { + t.Fatalf("converged is not acknowledged — the slot must stay held, got %+v", tr) + } + // The primary reacts to the command: the slot has done its job, so the + // converged round completes. + acked := obs + acked.Reacted = true + if tr := Progress(r, state.AccountQuota{}, acked, t0.Add(2*time.Minute), p); tr.Outcome != OutComplete { + t.Fatalf("an acknowledged converged round must complete, got %+v", tr) + } + // It never reacts: the in-flight window ends the wait rather than buying a + // second metered review no configured reviewer asked for. + if tr := Progress(r, state.AccountQuota{}, obs, t0.Add(16*time.Minute), p); tr.Outcome != OutComplete { + t.Fatalf("the in-flight timeout must end a converged round, not re-fire it, got %+v", tr) + } + // A co-only round spent no quota and holds no slot, so it completes at once. + co := firedRound(t, "abcdef123") + co.CoOnly = true + if tr := Progress(co, state.AccountQuota{}, obs, t0.Add(2*time.Minute), p); tr.Outcome != OutComplete { + t.Fatalf("a co-only round has no primary command to wait on, got %+v", tr) + } +} + +func TestSubmittedPrimaryReviewAcknowledgesTheRound(t *testing.T) { + firedAt := t0.Add(-time.Minute) + r := state.Round{Phase: state.PhaseFired, Head: "abcdef123", FiredAt: &firedAt} + obs := Observation{Reviews: []ReviewSeen{{ + Bot: "coderabbitai[bot]", Commit: "abcdef1234567890", SubmittedAt: t0, + }}} + if PrimaryAckPending(r, obs, policy) { + t.Fatal("a submitted primary review must acknowledge the command that produced it") + } +} + func TestInflightTimeoutCarriesCooldown(t *testing.T) { r := firedRound(t, "abcdef123") now := t0.Add(16 * time.Minute) @@ -535,6 +609,42 @@ func TestDecideFireCodexDedupe(t *testing.T) { } } +func TestDecideFireForcedCoReviewerGate(t *testing.T) { + now := t0.Add(10 * time.Minute) + head := "abcdef123" + round := state.Round{Repo: "owner/repo", PR: 448, Head: head, Phase: state.PhaseQueued, Seq: 1} + obs := Observation{Head: head, Open: true, Reviews: []ReviewSeen{{ + Bot: policy.Bot, Commit: "abcdef1234567890", SubmittedAt: now, + }}} + selfHeal := policy + selfHeal.CoReviewers = []CoReviewerPolicy{{ + Login: dialect.CodexBotLogin, Command: "@codex review", Trigger: TriggerSelfHeal, + }} + + for _, tc := range []struct { + name string + forced bool + want FireVerdict + }{ + {name: "optional self-heal reviewer is not a gate", want: FireDedupe}, + {name: "forced optional self-heal reviewer gates", forced: true, want: FireCoOnly}, + } { + t.Run(tc.name, func(t *testing.T) { + candidate := round + if tc.forced { + candidate.ForceCoReviewers = []string{dialect.CodexBotLogin} + } + got := DecideFire(Global{SlotFree: true}, candidate, obs, now, selfHeal) + if got.Verdict != tc.want { + t.Fatalf("verdict = %v, want %v (%+v)", got.Verdict, tc.want, got) + } + if tc.forced && (len(got.PostCo) != 1 || !dialect.IsCodexBot(got.PostCo[0])) { + t.Fatalf("PostCo = %v, want the forced reviewer", got.PostCo) + } + }) + } +} + // TestDynamicCodexGate covers the dynamic completion gate: an observed-active // Codex gates a round it isn't configured-required for, a usage-limit notice // disengages that dynamic gate, and a configured-required Codex is left gating @@ -944,3 +1054,98 @@ func TestAcceptAccountBlock(t *testing.T) { }) } } + +// A reopened round asks whether the primary already answered THIS round's +// command, and answering yes writes a completed marker every later same-head +// check skips — so the gate must hold to Completion's evidence, not to the +// weaker adoption question CommandHasCompletionReply answers. +func TestReopenedRoundDedupesOnlyOnCompletionEvidence(t *testing.T) { + free := Global{SlotFree: true} + now := t0.Add(10 * time.Minute) + head := "abcdef123" + + reopened := func(t *testing.T) state.Round { + t.Helper() + r := firedRound(t, head) + if err := r.Complete(); err != nil { + t.Fatal(err) + } + if err := r.Reopen(); err != nil { + t.Fatal(err) + } + return r + } + command := dialect.BotEvent{Kind: dialect.EvCommand, Bot: "kristofferR", CommentID: 1001, + CreatedAt: t0.Add(2 * time.Second), UpdatedAt: t0.Add(2 * time.Second)} + completion := dialect.BotEvent{Kind: dialect.EvCompletion, Bot: "coderabbitai[bot]", CommentID: 1002, + AutoReply: true, CreatedAt: t0.Add(time.Minute), UpdatedAt: t0.Add(time.Minute)} + priorReview := ReviewSeen{Bot: "coderabbitai[bot]", Commit: "0000000099", SubmittedAt: t0.Add(-time.Hour)} + + cases := []struct { + name string + obs Observation + want FireVerdict + }{ + { + // The reply stands in for a re-review that found nothing new. + name: "completion reply with a prior review", + obs: Observation{Head: head, Open: true, Reviews: []ReviewSeen{priorReview}, + Events: []dialect.BotEvent{command, completion}}, + want: FireDedupe, + }, + { + // A clean-summary verdict is Completion's other no-Review evidence. + // Reopening the marker for a reviewer change must not buy the same + // primary review again. + name: "clean summary for this round", + obs: Observation{Head: head, Open: true, Events: []dialect.BotEvent{{ + Kind: dialect.EvNoAction, Bot: "coderabbitai[bot]", CommentID: 1003, + CreatedAt: t0.Add(time.Minute), UpdatedAt: t0.Add(time.Minute), + }}}, + want: FireDedupe, + }, + { + // Nothing to stand in for: the bot has never submitted a review, so + // the head has not been reviewed and the round must still fire. + name: "completion reply with no review anywhere", + obs: Observation{Head: head, Open: true, + Events: []dialect.BotEvent{command, completion}}, + want: FirePost, + }, + { + // The review failed after answering, so the reply describes a review + // that did not land. + name: "failed summary after the completion reply", + obs: Observation{Head: head, Open: true, Reviews: []ReviewSeen{priorReview}, + Events: []dialect.BotEvent{command, completion, + {Kind: dialect.EvFailed, Bot: "coderabbitai[bot]", CommentID: 900, + CreatedAt: t0, UpdatedAt: t0.Add(2 * time.Minute)}}}, + want: FirePost, + }, + { + // Each half used to find its own reply: the round's failed command + // satisfied the command-id check, while a later successful command + // satisfied the completion-evidence check. + name: "later command cannot repair this round's failed reply", + obs: Observation{Head: head, Open: true, Reviews: []ReviewSeen{priorReview}, + Events: []dialect.BotEvent{ + command, + completion, + {Kind: dialect.EvFailed, Bot: "coderabbitai[bot]", CommentID: 900, + CreatedAt: t0, UpdatedAt: t0.Add(2 * time.Minute)}, + {Kind: dialect.EvCommand, Bot: "kristofferR", CommentID: 2001, + CreatedAt: t0.Add(3 * time.Minute), UpdatedAt: t0.Add(3 * time.Minute)}, + {Kind: dialect.EvCompletion, Bot: "coderabbitai[bot]", CommentID: 2002, + AutoReply: true, CreatedAt: t0.Add(4 * time.Minute), UpdatedAt: t0.Add(4 * time.Minute)}, + }}, + want: FirePost, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if d := DecideFire(free, reopened(t), tc.obs, now, policy); d.Verdict != tc.want { + t.Fatalf("verdict = %v, want %v (%+v)", d.Verdict, tc.want, d) + } + }) + } +} diff --git a/internal/engine/fire.go b/internal/engine/fire.go index 7b4c9f59..51cd4616 100644 --- a/internal/engine/fire.go +++ b/internal/engine/fire.go @@ -157,6 +157,16 @@ 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, and it must carry the evidence Completion asks for — a + // reply nothing backs would mark an unreviewed head reviewed for good. + if !reviewedHead && r.Head == obs.Head { + reviewedHead = PrimaryCompletedRound(r, obs, p) + } // 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 @@ -301,7 +311,7 @@ func coAwareDedupe(r state.Round, obs Observation, p Policy, now time.Time, prim anchor := selfHealAnchor(r, obs, primaryUnavailable) for _, cp := range p.coReviewers() { co := obs.co(cp.Login) - gates := requiredBot(p, cp.Login) || co.AutoActive || primaryUnavailable + gates := requiredBot(p, cp.Login) || co.AutoActive || primaryUnavailable || r.ForceCoReviewer(cp.Login) if !gates || coReviewedHead(obs, cp.Login) { continue } diff --git a/internal/engine/progress.go b/internal/engine/progress.go index 2b82c0c9..df68c40d 100644 --- a/internal/engine/progress.go +++ b/internal/engine/progress.go @@ -122,34 +122,41 @@ func Progress(r state.Round, q state.AccountQuota, obs Observation, now time.Tim // treated this as a normal bot comment and released the slot with the wait // still pending; parking with a bounded cooldown retries it instead. if r.Phase == state.PhaseFired && stateSince(obs, p, firedAt, dialect.EvFailed) { + if completion.Done { + return Transition{Outcome: OutComplete, Reason: "feedback complete; optional primary review failed"} + } return Transition{Outcome: OutRetry, Reason: "review failed", RetryAt: now.Add(p.retryBackoff())} } - if completion.Done { + // Convergence and slot release answer different questions. A repository may + // leave the primary out of its required set (required Codex only), and then + // completion is done the moment that co-reviewer answers — while the + // account-metered command this round posted is still unacknowledged. + // Completing here would release the fire slot, letting the next PR spend the + // shared allowance alongside a review that is still running, and would leave a + // late account-block reply landing on a completed round nobody progresses. So + // a round that spent the quota stays fired until the primary speaks (below) or + // its in-flight timeout gives up on it; a co-only round posted no command of + // its own and has nothing to wait for. + if completion.Done && (r.Phase != state.PhaseFired || r.CoOnly) { return Transition{Outcome: OutComplete, Reason: "feedback complete"} } if r.Phase == state.PhaseFired { - // A bare reaction acknowledges the command; the review is still running. - if obs.Reacted { - return Transition{Outcome: OutReviewing, Reason: "bot reacted"} - } - // Any other bot comment in the round window acknowledges it too — but an - // account-block/paused/already-reviewed notice is not an ack (v2), and - // neither is the in-progress summary of a PREVIOUS round edit... which - // it cannot be: UpdatedAt gates the window. An in-progress summary IS - // an ack that reviewing started. - for _, ev := range obs.Events { - if !sameBot(ev.Bot, p.Bot) || ev.CommentID == r.CommandID || ev.UpdatedAt.Before(firedAt) { - continue + if reason, acked := primaryAck(r, obs, p, firedAt); acked { + if completion.Done { + return Transition{Outcome: OutComplete, Reason: "feedback complete"} } - switch ev.Kind { - case dialect.EvRateLimited, dialect.EvPaused, dialect.EvAlreadyReviewed: - continue - } - return Transition{Outcome: OutReviewing, Reason: "bot responded"} + return Transition{Outcome: OutReviewing, Reason: reason} } if now.Sub(firedAt) > p.InflightTimeout { + // Nothing more is owed: everything this repository gates on has + // answered and the slot has been held for the whole in-flight window, + // so the serialization the wait exists for is satisfied. Re-firing + // would buy another metered review no configured reviewer asked for. + if completion.Done { + return Transition{Outcome: OutComplete, Reason: "feedback complete; primary never acknowledged"} + } return Transition{Outcome: OutRetry, Reason: "in-flight timeout", RetryAt: now.Add(p.retryBackoff())} } return Transition{Outcome: KeepWaiting, Reason: "review in flight"} @@ -162,6 +169,51 @@ func Progress(r state.Round, q state.AccountQuota, obs Observation, now time.Tim return Transition{Outcome: KeepWaiting, Reason: "reviewing"} } +// primaryAck reports whether the primary has acknowledged this round's command, +// and how — the condition that releases the fire slot. +// +// A bare reaction acknowledges it; so does any other comment of the bot's in the +// round window — but an account-block/paused/already-reviewed notice is not an +// ack (v2), and neither is the in-progress summary of a PREVIOUS round edit... +// which it cannot be: UpdatedAt gates the window. An in-progress summary IS an +// ack that reviewing started. +func primaryAck(r state.Round, obs Observation, p Policy, firedAt time.Time) (string, bool) { + if obs.Reacted { + return "bot reacted", true + } + for _, review := range obs.Reviews { + if sameBot(review.Bot, p.Bot) && reviewMatchesRound(review, r.Head, firedAt) { + return "review submitted", true + } + } + for _, ev := range obs.Events { + if !sameBot(ev.Bot, p.Bot) || ev.CommentID == r.CommandID || ev.UpdatedAt.Before(firedAt) { + continue + } + switch ev.Kind { + case dialect.EvRateLimited, dialect.EvPaused, dialect.EvAlreadyReviewed: + continue + } + return "bot responded", true + } + return "", false +} + +// PrimaryAckPending reports whether a round is still holding the fire slot for a +// command the primary has not acknowledged — the condition Progress waits on +// above, exported because the LOOP releases that same slot when it completes the +// round it waited on, and convergence is no more permission to do so there. +// +// Only a fired round can be holding it: a co-only one posted no command of its +// own, and a reviewing one released the slot when the ack it is named for landed. +func PrimaryAckPending(r state.Round, obs Observation, p Policy) bool { + if r.Phase != state.PhaseFired || r.FiredAt == nil || r.CoOnly { + return false + } + _, acked := primaryAck(r, obs, p, r.FiredAt.UTC()) + return !acked +} + // resolveBlockWindow ports v2's requeueInflight window logic: reuse the // standing block when the SAME edited account-quota comment is re-observed // (CodeRabbit edits one comment in place — a re-observation must not extend diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 6509b1d3..0ace6c27 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -191,6 +191,19 @@ func dash(s string) string { return s } +// hostName renders a round's writer id ("host=blue pid=4711 run=1a2b3c4d") as +// the machine name the host column has always shown. The round stores the writer +// id because that is what capabilities are keyed by; the pid and run id are +// bookkeeping for LaggingWriters, not something a reader of the table needs. +func hostName(writer string) string { + rest, ok := strings.CutPrefix(writer, "host=") + if !ok { + return writer + } + name, _, _ := strings.Cut(rest, " ") + return name +} + // RenderDashboard renders the human-facing dashboard for v3 state: rounds by // phase instead of v2's queue/fired/awaiting maps. func RenderDashboard(st State, cfg StoreConfig) string { @@ -266,7 +279,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { for _, r := range inFlight { fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %s | %s | `%s` |\n", r.Repo, r.PR, r.Repo, r.PR, r.Head, r.Phase, - fmtStamp(firedTimeOf(r), loc), fmtStamp(r.WaitDeadline, loc), dash(coBotMarks(r)), r.ByHost) + fmtStamp(firedTimeOf(r), loc), fmtStamp(r.WaitDeadline, loc), dash(coBotMarks(r)), hostName(r.ByHost)) } } @@ -303,7 +316,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { } fmt.Fprintf(&b, "| %s | [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | %s | %d | %s | `%s` |\n", position, e.Repo, e.PR, e.Repo, e.PR, e.Head, ready, dash(e.Why), - e.Attempts, fmtStamp(&e.EnqueuedAt, loc), e.ByHost) + e.Attempts, fmtStamp(&e.EnqueuedAt, loc), hostName(e.ByHost)) } } @@ -315,7 +328,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { fmt.Fprintf(&b, "| PR | commit | requested | host |\n|---|---|---|---|\n") for _, r := range requested { fmt.Fprintf(&b, "| [%s#%d](https://github.com/%s/pull/%d) | `%s` | %s | `%s` |\n", - r.Repo, r.PR, r.Repo, r.PR, r.Head, fmtStamp(r.FiredAt, loc), r.ByHost) + r.Repo, r.PR, r.Repo, r.PR, r.Head, fmtStamp(r.FiredAt, loc), hostName(r.ByHost)) } } diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index bd65af68..84f021f7 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -849,6 +849,36 @@ func TestHeldSlotStopsFreeRunningRoundsToo(t *testing.T) { } } +func TestOrphanedSlotHoldLeavesFreeRunningRoundsReady(t *testing.T) { + now := time.Now().UTC() + st := stateWith( + queuedRound("kristofferr/metered", 1, 1, now), + queuedRound("kristofferr/free", 2, 2, now), + ) + st.Rounds["kristofferr/free#2"] = func() Round { + r := st.Rounds["kristofferr/free#2"] + r.CoOnly = true + return r + }() + until := now.Add(time.Hour) + st.FireSlot = &FireSlot{ + Key: "kristofferr/gone#9", Token: "old", Since: now.Add(-time.Minute), + HoldUntil: &until, + } + st.FireSlotHoldUntil = &until + + q := st.Queue(now, 0) + if len(q) != 2 { + t.Fatalf("Queue = %d entries, want 2", len(q)) + } + if q[0].PR != 2 || q[0].Why != "" || !q[0].ReadyAt.IsZero() { + t.Fatalf("quota-free round = %+v, want it ready ahead of the orphaned hold", q[0]) + } + if q[1].PR != 1 || q[1].Why != WaitSlotBusy { + t.Fatalf("metered round = %+v, want it blocked by the orphaned hold", q[1]) + } +} + // The status line answers "is it still going?" continuously, so a session never // has to spend a tool call and a paragraph on it. Each state must read at a // glance, and it must never claim a next PR the queue itself will not name. diff --git a/internal/state/state.go b/internal/state/state.go index 5d474546..9d5fa389 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -117,7 +117,23 @@ type Round struct { // the round is retried or surfaced as timed out. WaitDeadline *time.Time `json:"wait_deadline,omitempty"` - Token string `json:"token,omitempty"` // reservation token (CAS race detection) + // ReviewersChanged marks a completed round whose effective reviewer set + // changed while its pull request was closed. Requeueing a closed PR's round + // would hand Pump dead work ahead of every live round, so the change is + // recorded on the marker instead: if the PR is ever reopened, enqueue reopens + // the round rather than treating it as "this head was reviewed". Reopen + // clears it — the reopened round answers under the current requirements. + ReviewersChanged bool `json:"reviewers_changed,omitempty"` + // ForceCoReviewers names newly enabled or required self-heal reviewers that + // need one immediate trigger on an existing round. Once their command is + // recorded, normal per-bot dedupe makes the force harmless. + ForceCoReviewers []string `json:"force_co_reviewers,omitempty"` + + Token string `json:"token,omitempty"` // reservation token (CAS race detection) + // ByHost identifies the PROCESS that reserved this round, in the writer form + // "host= pid= run=" — the key NoteWriter records capabilities under, so + // LaggingWriters can ask whether the process driving a fire understands the + // configuration it is firing from. The dashboard shows the machine name. ByHost string `json:"by_host,omitempty"` Note string `json:"note,omitempty"` // human-readable reason for the last transition @@ -251,6 +267,16 @@ type FireSlot struct { Key string `json:"key"` // repo#pr holding the slot Token string `json:"token"` Since time.Time `json:"since"` + // HoldUntil keeps the slot taken after the round holding it went away with + // its metered command still unacknowledged — a head advance archives the + // round, and the command it posted does not stop being in flight because of + // that. Set to the end of that command's in-flight window, so the hold is + // bounded by the same deadline Progress would have applied. + HoldUntil *time.Time `json:"hold_until,omitempty"` + + // unknown carries JSON members this binary has no field for, so a newer + // binary's additions survive being read and rewritten here. See tolerant.go. + unknown unknownFields } // AccountQuota is the CodeRabbit account-wide review quota (NOT the GitHub @@ -288,17 +314,44 @@ type State struct { Rev int64 `json:"rev"` NextSeq int64 `json:"next_seq"` - Rounds map[string]Round `json:"rounds"` - FireSlot *FireSlot `json:"fire_slot,omitempty"` - LastFired *time.Time `json:"last_fired,omitempty"` - Account AccountQuota `json:"account"` - Leader *LeaderLease `json:"leader,omitempty"` + Rounds map[string]Round `json:"rounds"` + FireSlot *FireSlot `json:"fire_slot,omitempty"` + // FireSlotHoldUntil is the top-level compatibility mirror of + // FireSlot.HoldUntil. Binaries predating nested FireSlot tolerance discard + // hold_until and clear an orphaned slot during Normalize, but their State + // tolerance carries this unknown top-level member. New binaries can therefore + // still recover the hold after an older writer rewrites the shared state. + FireSlotHoldUntil *time.Time `json:"fire_slot_hold_until,omitempty"` + // FireSlotHoldLastFired is the real pacing anchor replaced while the hold + // is dual-written into LastFired for binaries that do not understand the + // top-level mirror. Current binaries restore it when the hold ends. + FireSlotHoldLastFired *time.Time `json:"fire_slot_hold_last_fired,omitempty"` + LastFired *time.Time `json:"last_fired,omitempty"` + Account AccountQuota `json:"account"` + Leader *LeaderLease `json:"leader,omitempty"` // CalibrationIssue overrides the configured calibration PR/issue when the // original hit GitHub's hard 2500-comment cap and crq rotated to a fresh // 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". + // + // 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"` @@ -312,6 +365,118 @@ 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 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 + } + 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{} + // The leader identifies itself as "host= pid= run=", which is + // exactly the process identity capabilities are recorded under — the run + // component is what keeps a restart into a reused pid from inheriting them. + 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 + } + 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 +// 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 @@ -340,14 +505,15 @@ func (e *TransitionError) Error() string { func (r *Round) illegal(to Phase) error { return &TransitionError{From: r.Phase, To: to} } // Reserve takes the fire slot for this round: queued (or retry-eligible -// awaiting_retry) → reserved. -func (r *Round) Reserve(token, host string, now time.Time) error { +// awaiting_retry) → reserved. writer is the reserving process's writer id (see +// ByHost), not a bare hostname. +func (r *Round) Reserve(token, writer string, now time.Time) error { if r.Phase != PhaseQueued && !r.retryEligible(now) { return r.illegal(PhaseReserved) } r.Phase = PhaseReserved r.Token = token - r.ByHost = host + r.ByHost = writer t := now.UTC() r.ReservedAt = &t r.Note = "" @@ -386,6 +552,47 @@ func (r *Round) ReleaseToQueue(reason string, now time.Time) error { return nil } +// Reopen puts a completed round back in the queue because its effective +// reviewer set 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. An optional reviewer also needs an active round for its trigger, +// self-heal and bounded participation wait. 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 runs, not what happened. +// +// LastAttemptAt is deliberately left alone: it is the adoption floor for a +// FAILED attempt, and moving it would discard a newly required co-reviewer's own +// unanswered trigger comment as too old to adopt — so crq would post that bot a +// second request for the very round the reopen exists to let it answer. +func (r *Round) Reopen() error { + if r.Phase != PhaseCompleted { + return r.illegal(PhaseQueued) + } + r.Phase = PhaseQueued + r.Token = "" + r.ReservedAt = nil + r.WaitDeadline = nil + r.RetryAt = nil + r.ReviewersChanged = false + r.Note = "reviewer configuration changed" + return nil +} + +// ForceCoReviewer reports whether a reviewer-change reopen granted login its +// one immediate trigger. Bot names are normalized like CoBots keys. +func (r *Round) ForceCoReviewer(login string) bool { + key := coBotKey(login) + for _, candidate := range r.ForceCoReviewers { + if coBotKey(candidate) == key { + return true + } + } + return false +} + // 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. @@ -460,6 +667,7 @@ func (r *Round) Complete() error { return r.illegal(PhaseCompleted) } r.Phase = PhaseCompleted + r.ForceCoReviewers = nil r.Note = "" return nil } @@ -476,6 +684,7 @@ func (r *Round) Dedupe(now time.Time) error { r.Phase = PhaseCompleted r.Token = "" r.ReservedAt = nil + r.ForceCoReviewers = nil r.Note = "bot already reviewed head" return nil } @@ -591,6 +800,59 @@ func (s *State) SlotRound() *Round { return &r } +// SlotHeld reports whether the fire slot is taken — the question every fire gate +// actually asks. A live round holds it; so does an orphaned hold, left behind +// when the round that posted a still-unacknowledged metered command was +// superseded by a new head. Without the second case the expected push after a +// round that converged without its primary would free the slot for a second +// concurrent metered command. +func (s *State) SlotHeld(now time.Time) bool { + if s.FireSlot != nil && s.SlotRound() != nil { + return true + } + if s.FireSlotHoldUntil != nil && s.FireSlotHoldUntil.After(now) { + return true + } + return s.FireSlot != nil && s.FireSlot.HoldUntil != nil && s.FireSlot.HoldUntil.After(now) +} + +// HoldSlotUntil keeps the current fire slot held past the round that owns it. +// The caller sets the deadline, since only it knows the in-flight window the +// command this slot was taken for is bounded by. +func (s *State) HoldSlotUntil(until time.Time) { + if s.FireSlot == nil { + return + } + u := until.UTC() + before := s.LastFired + if s.FireSlotHoldUntil != nil && s.LastFired != nil && + s.LastFired.Equal(*s.FireSlotHoldUntil) { + before = s.FireSlotHoldLastFired + } + s.FireSlot.HoldUntil = &u + s.FireSlotHoldUntil = &u + s.FireSlotHoldLastFired = before + // The previous binary preserves the top-level mirror but does not read it. + // It does read LastFired, so a future pacing anchor keeps its Pump from + // posting another metered review. Its MinInterval may conservatively extend + // the wait; a current binary restores the real anchor below. + if s.LastFired == nil || s.LastFired.Before(u) { + s.LastFired = &u + } +} + +// ClearSlotHold removes the compatibility hold and restores the pacing anchor +// it temporarily replaced. If another writer fired after the deadline, +// LastFired no longer equals the synthetic value and is left untouched. +func (s *State) ClearSlotHold() { + if s.FireSlotHoldUntil != nil && s.LastFired != nil && + s.LastFired.Equal(*s.FireSlotHoldUntil) { + s.LastFired = s.FireSlotHoldLastFired + } + s.FireSlotHoldUntil = nil + s.FireSlotHoldLastFired = nil +} + // NextEligible returns the fire-eligible round with the lowest Seq, or nil. func (s *State) NextEligible(now time.Time) *Round { var best *Round @@ -694,7 +956,8 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { if s.Account.BlockedUntil != nil && s.Account.BlockedUntil.After(now) { blocked = s.Account.BlockedUntil.UTC() } - slotBusy := s.SlotRound() != nil + liveSlotBusy := s.SlotRound() != nil + orphanSlotBusy := !liveSlotBusy && s.SlotHeld(now) // The pacing gate applies to whichever round fires next, so it bounds every // entry's earliest possible start. var paced time.Time @@ -741,7 +1004,7 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { queued = append(queued, e) } - // A held slot stops EVERYTHING, free-running rounds included. + // A live slot stops EVERYTHING, free-running rounds included. // // Not because they need the slot — they do not — but because Pump returns as // soon as it sees a slot holder, so the quota-free path that would advance @@ -750,13 +1013,20 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { // ready here would promise action the daemon cannot take until the holder is // acknowledged. (An agent's own `crq next` can still resolve such a round // directly, which is why this describes the queue rather than forbidding it.) - if slotBusy { + if liveSlotBusy { for i := range queued { queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitSlotBusy } for i := range freeRunning { freeRunning[i].ReadyAt, freeRunning[i].Why = time.Time{}, WaitSlotBusy } + } else if orphanSlotBusy { + // An orphaned bounded hold still blocks another metered fire, but Pump + // deliberately scans past it for quota-free work. Reflect that split: + // ordinary rounds wait on the slot while co-only rounds remain ready. + for i := range queued { + queued[i].ReadyAt, queued[i].Why = time.Time{}, WaitSlotBusy + } } // One list, ordered by readiness then Seq — which is NextEligible's own rule @@ -819,7 +1089,7 @@ func (s *State) Queue(now time.Time, minInterval time.Duration) []QueueEntry { // Normalize repairs invariants after load: map init, expired retry windows // (awaiting_retry with a passed RetryAt is simply fire-eligible; nothing to -// do), and a FireSlot pointing at a round that no longer holds it. +// do), and a FireSlot no round holds and no orphaned hold keeps alive. func (s *State) Normalize(now time.Time) { if s.Rounds == nil { s.Rounds = map[string]Round{} @@ -827,8 +1097,22 @@ func (s *State) Normalize(now time.Time) { if s.Version == 0 { s.Version = SchemaVersion } - if s.FireSlot != nil && s.SlotRound() == nil { + // Fold both hold representations before repairing the slot. The top-level + // mirror may be the only copy left after a pre-FireSlot-tolerance binary + // normalized and rewrote an orphaned hold. + if s.FireSlot != nil && s.FireSlot.HoldUntil != nil && + (s.FireSlotHoldUntil == nil || s.FireSlotHoldUntil.Before(*s.FireSlot.HoldUntil)) { + u := s.FireSlot.HoldUntil.UTC() + s.FireSlotHoldUntil = &u + } + if s.FireSlotHoldUntil != nil && s.FireSlot != nil && + (s.FireSlot.HoldUntil == nil || s.FireSlot.HoldUntil.Before(*s.FireSlotHoldUntil)) { + u := s.FireSlotHoldUntil.UTC() + s.FireSlot.HoldUntil = &u + } + if !s.SlotHeld(now) { s.FireSlot = nil + s.ClearSlotHold() } for key, r := range s.Rounds { r.foldLegacyCodex() diff --git a/internal/state/store.go b/internal/state/store.go index 6b6a83a6..bbdbf71e 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/tolerant.go b/internal/state/tolerant.go index 957d9a8d..db4fefc9 100644 --- a/internal/state/tolerant.go +++ b/internal/state/tolerant.go @@ -28,10 +28,33 @@ import ( type unknownFields map[string]json.RawMessage var ( - roundFields = jsonFieldNames(reflect.TypeOf(Round{})) - stateFields = jsonFieldNames(reflect.TypeOf(State{})) + fireSlotFields = jsonFieldNames(reflect.TypeOf(FireSlot{})) + roundFields = jsonFieldNames(reflect.TypeOf(Round{})) + stateFields = jsonFieldNames(reflect.TypeOf(State{})) ) +// UnmarshalJSON decodes a fire slot and remembers anything it did not recognise. +func (s *FireSlot) UnmarshalJSON(raw []byte) error { + type plain FireSlot + var decoded plain + if err := json.Unmarshal(raw, &decoded); err != nil { + return err + } + unknown, err := captureUnknown(raw, fireSlotFields) + if err != nil { + return err + } + *s = FireSlot(decoded) + s.unknown = unknown + return nil +} + +// MarshalJSON writes a fire slot back with the members it did not recognise intact. +func (s FireSlot) MarshalJSON() ([]byte, error) { + type plain FireSlot + return mergeUnknown(plain(s), s.unknown) +} + // UnmarshalJSON decodes a round and remembers anything it did not recognise. func (r *Round) UnmarshalJSON(raw []byte) error { // A distinct type with the same layout: without it, json would call this diff --git a/internal/state/tolerant_test.go b/internal/state/tolerant_test.go index b80b3c85..1aa628a8 100644 --- a/internal/state/tolerant_test.go +++ b/internal/state/tolerant_test.go @@ -68,6 +68,100 @@ func TestUnknownRoundFieldsSurviveARewrite(t *testing.T) { } } +// FireSlot is nested beneath a known State field, so top-level tolerance cannot +// carry additions made to the slot itself. +func TestUnknownFireSlotFieldsSurviveARewrite(t *testing.T) { + foreign := `{ + "v": 3, "rev": 7, "next_seq": 9, + "rounds": { + "owner/repo#1": { + "repo": "owner/repo", "pr": 1, "head": "abcdef123", "seq": 1, + "phase": "reserved", "enqueued_at": "2026-07-26T12:00:00Z", + "token": "slot-token" + } + }, + "fire_slot": { + "key": "owner/repo#1", "token": "slot-token", + "since": "2026-07-26T12:01:00Z", + "future_hold": {"until": "2026-07-26T12:05:00Z"} + }, + "account": {"scope": "owner"} + }` + + var st State + if err := json.Unmarshal([]byte(foreign), &st); err != nil { + t.Fatal(err) + } + out, err := json.Marshal(st) + if err != nil { + t.Fatal(err) + } + var back map[string]any + if err := json.Unmarshal(out, &back); err != nil { + t.Fatal(err) + } + slot, _ := back["fire_slot"].(map[string]any) + if slot == nil { + t.Fatalf("the fire slot vanished:\n%s", out) + } + hold, _ := slot["future_hold"].(map[string]any) + if hold == nil || hold["until"] != "2026-07-26T12:05:00Z" { + t.Errorf("carried fire-slot member lost its content: %#v", slot["future_hold"]) + } +} + +// A binary from before FireSlot had its own tolerant marshal drops nested +// hold_until and then clears the orphaned slot in Normalize. The top-level +// mirror is unknown to that binary, so State's existing tolerance carries it +// and a current reader must still honor the in-flight command window. +func TestOrphanedHoldSurvivesALegacyRewrite(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + until := now.Add(15 * time.Minute) + st := State{ + Version: SchemaVersion, + Rounds: map[string]Round{}, + FireSlot: &FireSlot{ + Key: "owner/repo#1", Token: "slot-token", Since: now, + }, + } + st.HoldSlotUntil(until) + raw, err := json.Marshal(st) + if err != nil { + t.Fatal(err) + } + + // Model the legacy rewrite: its Normalize removes fire_slot because no live + // round owns it, while its top-level unknown-field carrier leaves the mirror. + var legacy map[string]any + if err := json.Unmarshal(raw, &legacy); err != nil { + t.Fatal(err) + } + if legacy["last_fired"] != until.Format(time.RFC3339) { + t.Fatalf("legacy pacing anchor = %v, want hold deadline %s", legacy["last_fired"], until) + } + delete(legacy, "fire_slot") + rewritten, err := json.Marshal(legacy) + if err != nil { + t.Fatal(err) + } + + var back State + if err := json.Unmarshal(rewritten, &back); err != nil { + t.Fatal(err) + } + back.Normalize(now) + if !back.SlotHeld(now) { + t.Fatalf("legacy rewrite lost the orphaned hold: %+v", back) + } + back.Normalize(until.Add(time.Second)) + if back.SlotHeld(until.Add(time.Second)) || back.FireSlotHoldUntil != nil { + t.Fatalf("the recovered compatibility hold did not expire: %+v", back) + } + if back.LastFired != nil { + t.Fatalf("current writer did not restore the pre-hold pacing anchor: %s", back.LastFired) + } +} + // A carried member must never win over a field this binary owns: what this build // computes now is the current truth, and a stale copy from a foreign write would // silently override it. diff --git a/internal/state/writers_test.go b/internal/state/writers_test.go new file mode 100644 index 00000000..cc816eea --- /dev/null +++ b/internal/state/writers_test.go @@ -0,0 +1,118 @@ +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: "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] != "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("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) + } + + // 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: "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) + } + + // 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) + } +} + +// 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 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("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) + } +} + +// The other acting process is whoever holds the fire slot, and a round records +// that process in ByHost. Recording a bare hostname there could never match a +// writer entry, so every `crq reviewers` call during a fire named the current +// process as lagging — telling operators to upgrade a binary that already +// understands overrides. +func TestLaggingWritersMatchesTheFireSlotOwner(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + writer := "host=cachyos pid=1234" + st := New() + r, err := st.NewRound("owner/repo", 7, "abcdef123", now) + if err != nil { + t.Fatal(err) + } + if err := r.Reserve("tok", writer, now); err != nil { + t.Fatal(err) + } + st.PutRound(*r) + st.FireSlot = &FireSlot{Key: Key("owner/repo", 7), Token: "tok", Since: now} + + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 1 || got[0] != writer { + t.Fatalf("lagging = %v, want the un-announced slot owner named", got) + } + st.NoteWriter(writer, CapsRepoOverrides, now) + if got := st.LaggingWriters(CapsRepoOverrides, now); len(got) != 0 { + t.Errorf("lagging = %v, want none — the process firing IS the capable writer", got) + } +} + +// Reopening a round is not a failed attempt. Moving LastAttemptAt would raise +// the adoption floor past a newly required co-reviewer's own unanswered trigger, +// so crq would post that bot a second request for the round it is reopening to +// let it answer. +func TestReopenKeepsTheAdoptionFloor(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + r := Round{Repo: "owner/repo", PR: 7, Head: "abcdef123", Phase: PhaseFired, + FiredAt: &now, CommandID: 11} + floor := now.Add(-time.Hour) + r.LastAttemptAt = &floor + if err := r.Complete(); err != nil { + t.Fatal(err) + } + if err := r.Reopen(); err != nil { + t.Fatal(err) + } + if r.Phase != PhaseQueued { + t.Fatalf("phase = %s, want the round requeued", r.Phase) + } + if r.LastAttemptAt == nil || !r.LastAttemptAt.Equal(floor) { + t.Errorf("LastAttemptAt = %v, want it untouched by a reopen", r.LastAttemptAt) + } +} diff --git a/llms.txt b/llms.txt index a47d3cd3..6e62cb29 100644 --- a/llms.txt +++ b/llms.txt @@ -209,6 +209,22 @@ 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 # 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. + A finding with no `thread_id` — a review body finding, a review-skipped notice, an outside-diff remark, an issue-comment finding — has nothing for `resolve` or `decline` to act on, and would otherwise block every future round on that PR. Judge it, then record the decision: diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 2efe0e86..dd415950 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -182,6 +182,29 @@ 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 # 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, outside that queue. That is the only property the queue cares about — it says +what a reviewer costs, never whether a round waits for it. `--required` alone decides that, and either +flag may be given without the other (`--bots` and `--required` update separate halves of the override). + +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 +per-repo overrides — they will keep using the fleet default until upgraded. + ## Findings With No Thread Review-body findings, review-skipped notices, outside-diff remarks and issue-comment findings have