diff --git a/AGENTS.md b/AGENTS.md index 3f8507e6..82045259 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state credential-safe Git execution, stale-worktree pruning, and mirror migration. Owns persistent filesystem and process I/O for checkouts; `crq` supplies only configured roots and a current-token resolver. -- `internal/state/` — persisted schema v4: one `Round` per PR, one global +- `internal/state/` — persisted schema v5: one `Round` per PR, one global `FireSlot`, the CodeRabbit `AccountQuota`, an `Archive` ring. Round transition methods, durable tombstones for tidied trigger comments, the CAS store, and dashboard rendering. `Round.CoBots` holds per- @@ -39,9 +39,9 @@ Dependency rule (Go-enforced, no cycles): `dialect ← engine ← crq`, `state binary versions (`Normalize` folds them back on load). `Round` and `State` also **round-trip unknown JSON members** (`tolerant.go`), so a field a newer binary added survives being read and rewritten by an older one — which is what makes - ordinary additions safe without another dual-write or schema bump. Schema v4 - is the deliberate exception: older v3 clients refuse it, fencing pumping - clients that cannot enforce administrative holds. + ordinary additions safe without another dual-write or schema bump. Schema v5 + is the deliberate exception: older v4 clients refuse it, fencing pumping + clients that cannot enforce state-backed fleet policy. - `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` diff --git a/README.md b/README.md index d2fed2ef..52b6b2ca 100644 --- a/README.md +++ b/README.md @@ -397,12 +397,16 @@ 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 autofix install # ⭐ unattended: watch every PR and fix what needs fixing +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 autofix install # ⭐ unattended: watch every PR and fix what needs fixing crq watch # what autofix runs: drive open PRs through crq next, one JSON # line each. Fixing is ON by default (--no-dispatch observes only) crq autofix # which repositories crq may fix crq autofix off --reason "" # stop fixing there; watching and reviewing continue -crq autofix on | crq autofix default +crq autofix on # enable an explicit override +crq autofix default # return to the fleet default crq hold --reason "" # persistently stop reviews for a PR crq unhold # resume reviews for a held PR crq hold # list held PRs @@ -485,13 +489,44 @@ clears it. Ambiguous replies surface too — crq never buries a possible rebutta Set these in `~/.config/crq/env` (sourced automatically) or as environment variables: +### One configuration for the fleet, not one per machine + +These settings belong to the whole fleet and live in the state ref, not in the table below: +`scope`, `repos`, `exclude`, `required-bots`, `cobots`, `feedback-bots`, `rate-limit-co-degrade`, +`min-interval`, `inflight-timeout`, `rate-limit-fallback`, `calibrate-ttl`, `settle`, +`skip-marker`, `skip-authors`, and one family per co-reviewer — +`cobot--trigger`, `cobot--cmd`, `cobot--grace`. + +```bash +crq config # what is in force, and where it came from +crq config set exclude owner/paused-repo +crq config unset exclude +crq config seed # adopt this host's current answers as the fleet's +``` + +Per-host files diverge the moment somebody edits one — a repository excluded on the laptop and +reviewed by the server — and nothing says so, because each host is behaving correctly according to +what it can see. A recorded setting wins over the matching environment variable, so you can adopt +this one machine at a time, and `crq doctor` names any variable still set locally that the fleet +overrides. + +What stays per host: where the state lives (`CRQ_REPO`, `CRQ_ISSUE`, `CRQ_STATE_REF`), credentials, +and what the machine can physically do (`CRQ_DISPATCH_CMD`, `CRQ_WORKSPACE`, +`CRQ_DISPATCH_CONCURRENCY`). + +The primary reviewer stays per host too, for a different reason: who it is (`CRQ_BOT`) and the +wording crq reads it by (`CRQ_REVIEW_CMD` and the completion/rate-limit/review-done/calibration +markers) are one unit, and they are compiled into the dialect classifiers before any state ref is +read. Keep them the same on every host — `crq doctor` cannot report a disagreement about them. + + | Variable | Default | What it does | |----------|---------|--------------| | `CRQ_REPO` | *(required)* | the gate repo (`owner/name`) holding the state ref, dashboard, calibration PR | | `CRQ_ISSUE` | from `init` | dashboard issue number | | `CRQ_CAL_PR` | from `init` | calibration PR number | | `CRQ_SCOPE` | owner of `CRQ_REPO` | which owners/orgs share this quota (comma-separated) | -| `CRQ_STATE_REF` | `crq-state-v3` | git ref that stores the typed CAS state. The name is fixed; the schema inside it is v4, and a binary that predates a schema **refuses** the payload rather than erasing it — so upgrade every host together | +| `CRQ_STATE_REF` | `crq-state-v3` | git ref that stores the typed CAS state. The name is fixed; the schema inside it is v5, and a binary that predates a schema **refuses** the payload rather than erasing it — so upgrade every host together | | `CRQ_REPOS` | _(all in scope)_ | `autoreview` allowlist — only these `owner/name` repos (comma-separated) | | `CRQ_EXCLUDE` | _(none)_ | denylist — crq never reviews, watches or fixes these `owner/name` repos (comma-separated) | | `CRQ_AUTOREVIEW_SKIP_AUTHORS` | `dependabot[bot]` | PR authors `autoreview` never enqueues (comma-separated; case and `[bot]` suffix don't matter) — set to empty to auto-review bot PRs too; manual `crq review` is unaffected | diff --git a/cmd/crq/main.go b/cmd/crq/main.go index 2c4a4b25..6c15418d 100644 --- a/cmd/crq/main.go +++ b/cmd/crq/main.go @@ -57,6 +57,14 @@ func run(ctx context.Context, args []string) int { return 0 case "doctor": report := doctor(ctx) + // The fleet's own view needs the state ref, which needs credentials, so + // it is asked for only when the rest of the report says that will work. + // A doctor that cannot answer the basics has more urgent things to say. + if report.Ready { + if diverged := fleetDivergence(ctx); len(diverged) > 0 { + report.Recommendations = append(report.Recommendations, diverged...) + } + } printJSON(report) if report.Ready { return 0 @@ -64,27 +72,6 @@ func run(ctx context.Context, args []string) int { return 1 case "preflight": return preflight(ctx, args[1:]) - case "autofix": - // A dry run is documented as a PREVIEW: it writes nothing and reads no - // GitHub state. Deciding it here, before the authenticated client is - // built, is what lets somebody inspect the setup before finishing it — - // otherwise the one command for looking at the plan was itself another - // thing to set up first. A real install still goes through the - // authenticated path below. - if opts, perr := parseAutofixArgs(args[1:]); perr == nil && opts.dryRun { - cfg, cerr := crq.LoadConfig() - if cerr != nil { - fatal(cerr) - return 1 - } - plan, ierr := crq.AutofixPlan(cfg, opts.agent, crq.SplitArgv(opts.agentArgs), opts.repos, true) - if ierr != nil { - fatal(ierr) - return 1 - } - printJSON(plan) - return 0 - } } cfg, err := crq.LoadConfig() @@ -94,6 +81,24 @@ func run(ctx context.Context, args []string) int { } gh, err := ghapi.NewGitHub(ctx) if err != nil { + // A dry run is documented as a PREVIEW, so it has to work for somebody + // who has not authenticated yet — otherwise the one command for looking + // at the plan is itself another thing to set up first. An authenticated + // one takes the normal path below, where InstallAutofix builds it from the + // same fleet state as the real install (and falls back to this host's + // own plan if that state cannot be read). Every other command needs the + // token this just failed to find. + if args[0] == "autofix" { + if opts, perr := parseAutofixArgs(args[1:]); perr == nil && opts.dryRun { + plan, ierr := hostOnlyAutofixPlan(cfg, opts) + if ierr != nil { + fatal(ierr) + return 1 + } + printJSON(plan) + return 0 + } + } fatal(err) return 1 } @@ -118,7 +123,7 @@ func run(ctx context.Context, args []string) int { if result.CalibrationPR > 0 { fmt.Printf("export CRQ_CAL_PR=%q\n", strconv.Itoa(result.CalibrationPR)) } - fmt.Printf("export CRQ_SCOPE=%q\n", strings.Join(cfg.Scope, ",")) + fmt.Printf("export CRQ_SCOPE=%q\n", strings.Join(result.Scope, ",")) fmt.Printf("export CRQ_STATE_REF=%q\n", result.StateRef) return 0 case "status": @@ -326,8 +331,19 @@ func run(ctx context.Context, args []string) int { return 1 } if err := cfg.RequireState(); err != nil { - fatal(err) - return 1 + // Without CRQ_REPO there is no state ref to read, so a preview here + // is the host-only one InstallAutofix would otherwise produce. + if !opts.dryRun { + fatal(err) + return 1 + } + plan, ierr := hostOnlyAutofixPlan(cfg, opts) + if ierr != nil { + fatal(ierr) + return 1 + } + printJSON(plan) + return 0 } plan, ierr := service.InstallAutofix(ctx, opts.agent, crq.SplitArgv(opts.agentArgs), opts.repos, opts.dryRun) if ierr != nil { @@ -518,6 +534,68 @@ func run(ctx context.Context, args []string) int { } printJSON(map[string]any{"status": "cancelled", "repo": crq.NormalizeRepo(repo), "pr": pr}) return 0 + case "config": + if err := cfg.RequireState(); err != nil { + fatal(err) + return 1 + } + rest := args[1:] + switch { + case len(rest) == 0 || rest[0] == "show": + // Showing takes no operand either: a trailing token is a misspelled + // setting or subcommand, and printing the whole configuration would + // report success for an invocation that asked for something else. + if len(rest) > 1 { + fatal(errors.New("usage: crq config show")) + return 1 + } + settings, cerr := service.FleetConfig(ctx) + if cerr != nil { + fatal(cerr) + return 1 + } + printJSON(settings) + return 0 + case rest[0] == "set": + if len(rest) != 3 { + fatal(errors.New("usage: crq config set ")) + return 1 + } + if err := service.SetFleetConfig(ctx, rest[1], rest[2]); err != nil { + fatal(err) + return 1 + } + printJSON(map[string]any{"setting": rest[1], "value": rest[2], "scope": "fleet"}) + return 0 + case rest[0] == "unset": + if len(rest) != 2 { + fatal(errors.New("usage: crq config unset ")) + return 1 + } + dropped, uerr := service.UnsetFleetConfig(ctx, rest[1]) + if uerr != nil { + fatal(uerr) + return 1 + } + printJSON(map[string]any{"setting": rest[1], "cleared": dropped}) + return 0 + case rest[0] == "seed": + // Seeding takes no operand and records every unset fleet key, so a + // stray token is a typo for something narrower, not a selector. + if len(rest) != 1 { + fatal(errors.New("usage: crq config seed")) + return 1 + } + seeded, serr := service.SeedFleetConfig(ctx) + if serr != nil { + fatal(serr) + return 1 + } + printJSON(map[string]any{"seeded": seeded}) + return 0 + } + fatal(fmt.Errorf("unknown config subcommand %q (try: crq config, crq config set|unset [value], crq config seed)", rest[0])) + return 1 case "debug": return debug(ctx, service, store, cfg, args[1:]) default: @@ -626,7 +704,7 @@ USAGE crq decline [...] --reason "" [--keep-open] reply on a thread to record why a finding is declined (resolves it; --keep-open leaves it open) - crq autofix install [--agent ] [--dry-run] [...] + crq autofix install [--agent ] [--agent-args ] [--dry-run] [...] install and start unattended autofix crq autofix [on|off|default ] which repositories crq may fix (on by default) @@ -648,6 +726,8 @@ USAGE keep open PRs reviewed, rate-coordinated crq preflight [--type all|committed|uncommitted] [--base ] local CodeRabbit CLI pre-push review as JSON + crq config [set|unset [value]] | seed + fleet-wide policy, shared by every host crq doctor emit JSON readiness report for agents and humans crq status [--line] print the dashboard, or one line for a status bar crq cancel [ ] remove queued/in-flight state for a PR @@ -797,9 +877,38 @@ 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 "autofix": - fmt.Print(`crq autofix install [--agent ] [--dry-run] [...] + fmt.Print(`crq autofix install [--agent ] [--agent-args ] [--dry-run] [...] crq autofix (which repositories crq may fix) crq autofix off [--reason ""] crq autofix on @@ -821,7 +930,9 @@ platform needs to survive a logout, and starts it. --dry-run prints the paths an commands without touching anything. The agent defaults to "claude" on PATH; --agent takes another path to it, or a -wrapper around it. The installed service runs the agent with Claude Code's own +wrapper around it. --agent-args passes a shell-style argument string to that +agent, for example a model or reasoning-effort override. The installed service +runs the agent with Claude Code's own flags (-p, --permission-mode, --output-format), so an agent with a different CLI needs its own wrapper script and "crq watch -- ...", which runs whatever argv you give it. --agent must name something runnable; a name that @@ -918,35 +1029,6 @@ a finding — deleting those would destroy feedback nobody had read yet. Set CRQ_TIDY=1 to run it automatically as rounds progress under crq autoreview. --dry-run reports what it would remove. -`) - 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 @@ -968,10 +1050,10 @@ Read this, decide, then crq resolve (or crq decline) the ones you have answered. Record that you have accounted for a finding that has no review thread, so it stops blocking the round. -crq resolve and crq decline both act on a thread. A review-body finding, a -review-skipped notice or an outside-diff remark has none, so neither command can -touch it — and a finding that can never be cleared blocks every future round, leaving -a PR whose current head no review was ever requested for. +crq resolve and crq decline both act on a thread. Review-body findings, +review-skipped notices, outside-diff remarks and issue-comment findings have no +thread, so neither command can act on them. Dismiss one after judging it so it +does not block the current head's round. Finding IDs come from .findings[].id. They are content-derived, not GitHub node IDs, so the repo and PR are required. A dismissal covers the current head only: @@ -1035,6 +1117,44 @@ Typical setup: crq init Save the printed exports to ~/.config/crq/env on every machine or agent host. +`) + case "config": + fmt.Print(`crq config +crq config set +crq config unset +crq config seed + +The policy every host shares, kept in the state ref rather than in each +machine's environment. + +Per-host configuration files diverge the moment somebody edits one — a +repository excluded on the laptop and reviewed by the server, a rate-limit +window one host respects and another does not — and nothing says so, because +each host is behaving correctly according to what it can see. These settings +have one answer for the fleet: + + scope, repos, exclude, required-bots, cobots, feedback-bots, + rate-limit-co-degrade, min-interval, inflight-timeout, rate-limit-fallback, + calibrate-ttl, settle, skip-marker, skip-authors, and per co-reviewer: + cobot--trigger, cobot--cmd, cobot--grace + +Three kinds of setting deliberately stay local: where the state lives +(CRQ_REPO, CRQ_ISSUE, CRQ_STATE_REF — a host cannot read fleet policy until it +knows where to look), credentials, and what a machine can physically do (which +fix agent is installed, where its disk is, how many sessions it can take). + +A recorded setting wins over the environment variable it replaces, so adopting +this is one machine at a time: until something is recorded, every host keeps +using its own. "crq config seed" writes this host's current answers for +everything the fleet has not decided yet — run it once, on the machine whose +configuration is the one you mean. + +A value this crq cannot read is refused at "set", and one that arrives anyway +from a newer binary leaves the host on its own value rather than acting on half +a policy. "crq config unset" drops such a setting from any host — except while a +newer crq is driving the queue, since that binary is the one that can reconcile +its removal. "crq doctor" reports both that and any variable still set here that +the fleet overrides. `) case "doctor": fmt.Print(`crq doctor @@ -1777,6 +1897,27 @@ func configPath() string { return home + "/.config/crq/env" } +// fleetDivergence asks the state ref what the fleet's policy is and reports +// where this host disagrees. Errors are swallowed: doctor is a report, and one +// that fails because the network was down would be worse than one missing a +// section. +func fleetDivergence(ctx context.Context) []string { + cfg, err := crq.LoadConfig() + if err != nil || cfg.RequireState() != nil { + return nil + } + gh, err := ghapi.NewGitHub(ctx) + if err != nil { + return nil + } + store := crq.NewGitStateStore(cfg, gh, nil) + diverged, err := crq.NewService(cfg, gh, store, nil).FleetDivergence(ctx) + if err != nil { + return nil + } + return diverged +} + // autofixArgs is `crq autofix install`'s parsed command line. type autofixArgs struct { agent string @@ -1785,6 +1926,19 @@ type autofixArgs struct { repos []string } +// hostOnlyAutofixPlan is the preview for a host that cannot reach fleet state at +// all: this machine's own answers, labelled so the plan cannot be mistaken for +// the fleet-backed one an authenticated install would write. +func hostOnlyAutofixPlan(cfg crq.Config, opts autofixArgs) (crq.AutofixInstall, error) { + plan, err := crq.AutofixPlan(cfg, opts.agent, crq.SplitArgv(opts.agentArgs), opts.repos, true) + if err != nil { + return crq.AutofixInstall{}, err + } + plan.PolicySource = "host" + plan.Warning = crq.HostOnlyAutofixWarning + return plan, nil +} + // parseAutofixArgs is shared by the pre-authentication dry-run path and the // install itself, so the two cannot disagree about what was asked for. func parseAutofixArgs(args []string) (autofixArgs, error) { @@ -1801,7 +1955,7 @@ func parseAutofixArgs(args []string) (autofixArgs, error) { return autofixArgs{}, err } if sub != "install" { - return autofixArgs{}, errors.New("usage: crq autofix install [--agent ] [--dry-run] [...]") + return autofixArgs{}, errors.New("usage: crq autofix install [--agent ] [--agent-args ] [--dry-run] [...]") } return autofixArgs{agent: *agent, agentArgs: *agentArgs, dryRun: *dryRun, repos: fs.Args()}, nil } diff --git a/internal/crq/auto.go b/internal/crq/auto.go index 4b5021e2..9f59b64a 100644 --- a/internal/crq/auto.go +++ b/internal/crq/auto.go @@ -3,6 +3,7 @@ package crq import ( "context" "errors" + "sort" "strings" "time" @@ -221,24 +222,35 @@ func (s *Service) renewLeader(ctx context.Context, owner, token string) (State, } func (s *Service) autoReviewPass(ctx context.Context, opts AutoOptions, owner, token string) error { - targets := s.cfg.Scope - byRepo := false - if len(s.cfg.AllowRepos) > 0 { - targets = make([]string, 0, len(s.cfg.AllowRepos)) - for repo := range s.cfg.AllowRepos { - targets = append(targets, repo) - } - byRepo = true - } // Load the queue snapshot once per pass and reuse it across candidates: a // git-backed Load is GetRef+GetCommit+GetTree+GetBlob, so reloading it per PR // would burn the shared REST quota on a large scan. The heartbeat refreshes it, // and enqueueBatch re-checks Contains under CAS, so a slightly stale snapshot // during collection is safe. + // + // It is loaded BEFORE the targets are chosen, because the state ref is where + // the fleet records which repositories those are. Reading this host's own + // list first is what let one machine scan a repository another had excluded. state, _, err := s.store.Load(ctx) if err != nil { return err } + cfg := s.fleetCfg(state) + // Copied, never sorted in place. Without a fleet `scope` the configuration is + // this host's own, so the slice shares its backing array with s.cfg.Scope and + // the sort below would silently reorder the operator's configured scan order + // — and race any concurrent reader of it. The allow-repository branch already + // builds its own. + targets := append([]string(nil), cfg.Scope...) + byRepo := false + if len(cfg.AllowRepos) > 0 { + targets = make([]string, 0, len(cfg.AllowRepos)) + for repo := range cfg.AllowRepos { + targets = append(targets, repo) + } + byRepo = true + } + sort.Strings(targets) // stable: a pass must not depend on map iteration var candidates []queueCandidate lastBeat := time.Now() for _, target := range targets { @@ -253,16 +265,16 @@ func (s *Service) autoReviewPass(ctx context.Context, opts AutoOptions, owner, t return true, nil } repo := NormalizeRepo(pr.Repo) - if repo == NormalizeRepo(s.cfg.GateRepo) || s.cfg.ExcludeRepos[repo] { + if repo == NormalizeRepo(s.cfg.GateRepo) || cfg.ExcludeRepos[repo] { return false, nil } - if len(s.cfg.AllowRepos) > 0 && !s.cfg.AllowRepos[repo] { + if len(cfg.AllowRepos) > 0 && !cfg.AllowRepos[repo] { return false, nil } - if s.cfg.SkipAuthors[dialect.NormalizeBotName(strings.ToLower(pr.Author))] { + if cfg.SkipAuthors[dialect.NormalizeBotName(strings.ToLower(pr.Author))] { return false, nil } - if s.cfg.SkipsReview(pr.Body) { + if cfg.SkipsReview(pr.Body) { return false, nil } scanned++ @@ -302,7 +314,7 @@ func (s *Service) autoReviewPass(ctx context.Context, opts AutoOptions, owner, t } } // One batched write for the whole pass instead of N (#2). - return s.enqueueBatch(ctx, candidates) + return s.enqueueBatch(ctx, candidates, cfg.FleetRevision) } // needsReview reports whether an open PR should be enqueued for review, and its diff --git a/internal/crq/autofix.go b/internal/crq/autofix.go index 7243c532..493b55b7 100644 --- a/internal/crq/autofix.go +++ b/internal/crq/autofix.go @@ -22,16 +22,23 @@ import ( //go:embed dispatch/fix-prompt.txt var fixPrompt string +// HostOnlyAutofixWarning labels a preview built from this host's own +// configuration because fleet state could not be read. Shared so every path +// that can only offer that answer says the same thing about it. +const HostOnlyAutofixWarning = "GitHub fleet state is unavailable; this host-only preview may differ from an authenticated install" + // AutofixInstall describes what an install would do, so --dry-run can print it and // the result can be reported. type AutofixInstall struct { - Platform string `json:"platform"` - Prompt string `json:"prompt"` - Wrapper string `json:"wrapper"` - Unit string `json:"unit"` - LogDir string `json:"log_dir"` - Workspace string `json:"workspace,omitempty"` - Agent string `json:"agent"` + Platform string `json:"platform"` + Prompt string `json:"prompt"` + Wrapper string `json:"wrapper"` + Unit string `json:"unit"` + LogDir string `json:"log_dir"` + Workspace string `json:"workspace,omitempty"` + Agent string `json:"agent"` + PolicySource string `json:"policy_source"` + Warning string `json:"warning,omitempty"` // Invocation is the exact command the wrapper runs, so --dry-run shows what // the fix session will actually be — including the model, which is the // agent's business and not crq's to hardcode. @@ -55,11 +62,47 @@ type AutofixInstall struct { // failure this whole feature is about. func (s *Service) InstallAutofix(ctx context.Context, agent string, agentArgs []string, repos []string, dryRun bool) (AutofixInstall, error) { effectiveDryRun := dryRun || s.cfg.DryRun - plan, err := AutofixPlan(s.cfg, agent, agentArgs, repos, effectiveDryRun) + st, _, err := s.store.Load(ctx) + if err != nil { + // A dry run is documented as a preview, so a state ref this host cannot + // read downgrades it to the host's own plan rather than failing — the + // warning is what keeps it from being mistaken for the fleet's. A real + // install has no such fallback: writing a unit from policy the fleet may + // disagree with is the divergence this whole mechanism exists to end. + if !effectiveDryRun { + return AutofixInstall{}, err + } + plan, perr := AutofixPlan(s.cfg, agent, agentArgs, repos, true) + if perr != nil { + return AutofixInstall{}, perr + } + plan.PolicySource = "host" + plan.Warning = HostOnlyAutofixWarning + return plan, nil + } + effective := s.fleetCfg(st) + plan, err := AutofixPlan(effective, agent, agentArgs, repos, effectiveDryRun) + plan.PolicySource = "fleet" if err != nil || effectiveDryRun { return plan, err } - return s.applyAutofix(ctx, plan) + return s.applyAutofix(ctx, plan, s.autofixFallbackConfig(repos), repos) +} + +// autofixFallbackConfig is what the unit should carry for settings the fleet may +// later unset. Fleet policy is overlaid from state at runtime; baking today's +// effective values into the service would make them survive after that policy +// was removed. Explicit install repositories remain this host's chosen fallback. +func (s *Service) autofixFallbackConfig(repos []string) Config { + cfg := s.cfg + if len(repos) == 0 { + return cfg + } + cfg.AllowRepos = make(map[string]bool, len(repos)) + for _, repo := range repos { + cfg.AllowRepos[repo] = true + } + return cfg } // AutofixPlan computes what an install WOULD write and run, from configuration @@ -165,7 +208,7 @@ func AutofixPlan(cfg Config, agent string, agentArgs []string, repos []string, d } // applyAutofix writes the plan to disk and starts the service. -func (s *Service) applyAutofix(ctx context.Context, plan AutofixInstall) (AutofixInstall, error) { +func (s *Service) applyAutofix(ctx context.Context, plan AutofixInstall, cfg Config, repos []string) (AutofixInstall, error) { invocation, logDir := plan.Invocation, plan.LogDir self, err := os.Executable() if err != nil { @@ -180,12 +223,7 @@ func (s *Service) applyAutofix(ctx context.Context, plan AutofixInstall) (Autofi // Known agents receive their non-interactive flags. Any other executable is // treated as a self-contained prompt-taking wrapper. - wrapper := fmt.Sprintf(`#!/usr/bin/env bash -# Installed by "crq autofix install". Runs autofix: crq decides, and a -# fix session is started for each PR that needs one. -set -uo pipefail -exec %s watch -- %s -`, shellQuote(self), invocation) + wrapper := autofixWrapper(self, invocation, repos) for _, f := range []struct { path string @@ -194,7 +232,7 @@ exec %s watch -- %s }{ {plan.Prompt, fixPrompt, 0o644}, {plan.Wrapper, wrapper, 0o755}, - {plan.Unit, s.autofixUnit(plan), 0o644}, + {plan.Unit, autofixUnitFor(cfg, plan), 0o644}, } { if err := os.MkdirAll(filepath.Dir(f.path), 0o755); err != nil { return plan, err @@ -243,6 +281,23 @@ exec %s watch -- %s return plan, nil } +func autofixWrapper(self, invocation string, repos []string) string { + watchArgs := make([]string, 0, len(repos)) + for _, repo := range repos { + watchArgs = append(watchArgs, shellQuote(repo)) + } + watch := strings.Join(watchArgs, " ") + if watch != "" { + watch += " " + } + return fmt.Sprintf(`#!/usr/bin/env bash +# Installed by "crq autofix install". Runs autofix: crq decides, and a +# fix session is started for each PR that needs one. +set -uo pipefail +exec %s watch %s-- %s +`, shellQuote(self), watch, invocation) +} + func writeAutofixFile(path, body string, mode os.FileMode) error { if err := os.WriteFile(path, []byte(body), mode); err != nil { return err @@ -381,55 +436,106 @@ func autofixPath(plan AutofixInstall) string { // wrong still reports Started, and the watcher then loads a different queue, or // none. func (s *Service) autofixEnv(plan AutofixInstall) map[string]string { + return autofixEnvFor(s.cfg, plan) +} + +func autofixEnvFor(cfg Config, plan AutofixInstall) map[string]string { + // Only an operator's OWN list travels. When it was derived from who reviews, + // writing the currently derived logins out makes the service read them back + // as an explicit choice, frozen: a later fleet `cobots` change that enables + // an optional reviewer would then never reach the surfaced set, and a round + // could converge without ever showing that bot's findings. Empty is the + // environment's "unset" — the same reasoning, and the same encoding, as the + // implicit co-reviewer trigger below. + feedbackBots := "" + if cfg.FeedbackBotsExplicit { + feedbackBots = strings.Join(cfg.FeedbackBots, ",") + } env := map[string]string{ - "CRQ_REPOS": strings.Join(plan.Repos, ","), + "CRQ_REPOS": strings.Join(sortedRepoList(cfg.AllowRepos), ","), // The denylist travels with the allowlist. Carrying one and not the other // installs a service that watches a repository the operator excluded. - "CRQ_EXCLUDE": strings.Join(sortedRepoList(s.cfg.ExcludeRepos), ","), - "CRQ_SCOPE": strings.Join(s.cfg.Scope, ","), - "CRQ_WATCH_INTERVAL": s.cfg.WatchInterval.String(), - "CRQ_DISPATCH_MAX_ATTEMPTS": fmt.Sprint(s.cfg.DispatchMaxAttempts), - "CRQ_DISPATCH_CONCURRENCY": fmt.Sprint(s.cfg.DispatchConcurrency), - "CRQ_DISPATCH_FORKS": strconv.FormatBool(s.cfg.DispatchForks), - "CRQ_AUTOREVIEW_SKIP_MARKER": s.cfg.SkipMarker, - "CRQ_BOT": s.cfg.Bot, - "CRQ_REQUIRED_BOTS": strings.Join(s.cfg.RequiredBots, ","), - "CRQ_FEEDBACK_BOTS": strings.Join(s.cfg.FeedbackBots, ","), - "CRQ_REVIEW_CMD": s.cfg.ReviewCommand, - "CRQ_RATELIMIT_CMD": s.cfg.RateLimitCommand, - "CRQ_RL_MARKER": s.cfg.RateLimitMarker, - "CRQ_CAL_REPLY_MARKER": s.cfg.CalibrationMarker, - "CRQ_REVIEW_DONE_MARKER": s.cfg.ReviewDoneMarker, - "CRQ_COMPLETION_MARKER": s.cfg.CompletionMarker, - "CRQ_MIN_INTERVAL": s.cfg.MinInterval.String(), - "CRQ_INFLIGHT_TIMEOUT": s.cfg.InflightTimeout.String(), - "CRQ_POLL": s.cfg.PollInterval.String(), - "CRQ_FEEDBACK_WAIT_TIMEOUT": s.cfg.FeedbackWaitTimeout.String(), - "CRQ_SETTLE": s.cfg.SettleWindow.String(), + "CRQ_EXCLUDE": strings.Join(sortedRepoList(cfg.ExcludeRepos), ","), + "CRQ_SCOPE": strings.Join(cfg.Scope, ","), + "CRQ_WATCH_INTERVAL": cfg.WatchInterval.String(), + "CRQ_DISPATCH_MAX_ATTEMPTS": fmt.Sprint(cfg.DispatchMaxAttempts), + "CRQ_DISPATCH_CONCURRENCY": fmt.Sprint(cfg.DispatchConcurrency), + "CRQ_DISPATCH_FORKS": strconv.FormatBool(cfg.DispatchForks), + "CRQ_AUTOREVIEW_SKIP_MARKER": cfg.SkipMarker, + "CRQ_BOT": cfg.Bot, + "CRQ_REQUIRED_BOTS": strings.Join(cfg.RequiredBots, ","), + "CRQ_FEEDBACK_BOTS": feedbackBots, + "CRQ_REVIEW_CMD": cfg.ReviewCommand, + "CRQ_RATELIMIT_CMD": cfg.RateLimitCommand, + "CRQ_RL_MARKER": cfg.RateLimitMarker, + "CRQ_CAL_REPLY_MARKER": cfg.CalibrationMarker, + "CRQ_REVIEW_DONE_MARKER": cfg.ReviewDoneMarker, + "CRQ_COMPLETION_MARKER": cfg.CompletionMarker, + "CRQ_MIN_INTERVAL": cfg.MinInterval.String(), + "CRQ_INFLIGHT_TIMEOUT": cfg.InflightTimeout.String(), + "CRQ_POLL": cfg.PollInterval.String(), + "CRQ_FEEDBACK_WAIT_TIMEOUT": cfg.FeedbackWaitTimeout.String(), + "CRQ_SETTLE": cfg.SettleWindow.String(), // The quota timings belong here for the same reason as every other // setting: the service does not inherit the shell that installed it. A // deliberately longer fallback set only in that shell was folded into // this config, written into no unit, and then silently replaced by the // default — so the watcher retried a review command earlier than // configured for every account block it could not parse a window from. - "CRQ_CALIBRATE_TTL": s.cfg.CalibrationTTL.String(), - "CRQ_RL_FALLBACK": s.cfg.RateLimitFallback.String(), + "CRQ_CALIBRATE_TTL": cfg.CalibrationTTL.String(), + "CRQ_RL_FALLBACK": cfg.RateLimitFallback.String(), "PATH": autofixPath(plan), } - if s.cfg.RateLimitCoDegrade { + if cfg.RateLimitCoDegrade { env["CRQ_RL_CO_DEGRADE"] = "1" } else { env["CRQ_RL_CO_DEGRADE"] = "0" } - coNames := make([]string, 0, len(s.cfg.CoBots)) - for _, co := range s.cfg.CoBots { + coNames := make([]string, 0, len(cfg.CoBots)) + for _, co := range cfg.CoBots { coNames = append(coNames, co.Name) - prefix := "CRQ_COBOT_" + strings.ToUpper(co.Name) + } + written := map[string]bool{} + writeCoBot := func(co CoBotConfig) { + key := strings.ToUpper(co.Name) + if written[key] { + return + } + written[key] = true + prefix := "CRQ_COBOT_" + key env[prefix+"_CMD"] = co.Command - env[prefix+"_TRIGGER"] = string(co.Trigger) + // The explicitness bit has to survive the install, not just the mode. An + // implicit trigger is the registry default for how the bot is REQUIRED, + // recomputed whenever that changes; writing it out as a value makes the + // service read it back as an operator's explicit choice, and a later + // fleet `required-bots` change can then no longer promote the bot to its + // required trigger — leaving a required reviewer that is never commanded + // and a round that waits for it forever. Empty is the environment's + // "unset", and it is written rather than omitted so an inherited variable + // cannot supply an explicitness the installing host did not have. + trigger := "" + if co.TriggerExplicit { + trigger = string(co.Trigger) + } + env[prefix+"_TRIGGER"] = trigger env[prefix+"_REQUIRED"] = strconv.FormatBool(co.Required) env[prefix+"_GRACE"] = co.SelfHealGrace.String() } + // The enabled entries first: they carry the requiredness this host resolved, + // and with it the trigger mode that requiredness implies. + for _, co := range cfg.CoBots { + writeCoBot(co) + } + // Then the registry-wide fallbacks for the bots this host has switched off. + // A command, trigger or grace set for one of those is still the value this + // host would drive it with the moment something enables it — a per-repo + // reviewer override, or a fleet `cobots` change that names the bot without + // naming its per-bot keys. Carrying only the enabled set left the service + // running that bot on registry defaults while the installing shell's preview + // showed the host's own. + for _, co := range cfg.KnownCoBots { + writeCoBot(co) + } // Explicitly carry an empty set: omitting this key would re-enable every // default co-reviewer when the service starts. env["CRQ_COBOTS"] = strings.Join(coNames, ",") @@ -438,7 +544,7 @@ func (s *Service) autofixEnv(plan AutofixInstall) map[string]string { // or dispatch silently falls back to another filesystem. workspace := plan.Workspace if workspace == "" { - workspace = s.cfg.WorkspaceRoot + workspace = cfg.WorkspaceRoot } if workspace != "" { env["CRQ_WORKSPACE"] = workspace @@ -450,24 +556,28 @@ func (s *Service) autofixEnv(plan AutofixInstall) map[string]string { // the dashboard, and the PR the account quota is probed on. Without the first // two, the watcher cannot even load state — or loads a queue nobody else is // using, which looks exactly like an idle fleet. - if s.cfg.GateRepo != "" { - env["CRQ_REPO"] = s.cfg.GateRepo + if cfg.GateRepo != "" { + env["CRQ_REPO"] = cfg.GateRepo } - if s.cfg.StateRef != "" { - env["CRQ_STATE_REF"] = s.cfg.StateRef + if cfg.StateRef != "" { + env["CRQ_STATE_REF"] = cfg.StateRef } - if s.cfg.DashboardIssue > 0 { - env["CRQ_ISSUE"] = fmt.Sprint(s.cfg.DashboardIssue) + if cfg.DashboardIssue > 0 { + env["CRQ_ISSUE"] = fmt.Sprint(cfg.DashboardIssue) } - if s.cfg.CalibrationPR > 0 { - env["CRQ_CAL_PR"] = fmt.Sprint(s.cfg.CalibrationPR) + if cfg.CalibrationPR > 0 { + env["CRQ_CAL_PR"] = fmt.Sprint(cfg.CalibrationPR) } return env } // autofixUnit renders the platform's service definition. func (s *Service) autofixUnit(plan AutofixInstall) string { - env := s.autofixEnv(plan) + return autofixUnitFor(s.cfg, plan) +} + +func autofixUnitFor(cfg Config, plan AutofixInstall) string { + env := autofixEnvFor(cfg, plan) // Sorted: the unit is a file on disk that a re-install rewrites, and map order // would make every rewrite a different file for the same configuration. keys := make([]string, 0, len(env)) diff --git a/internal/crq/autofix_test.go b/internal/crq/autofix_test.go index 30767980..618395fc 100644 --- a/internal/crq/autofix_test.go +++ b/internal/crq/autofix_test.go @@ -3,6 +3,7 @@ package crq import ( "context" "encoding/xml" + "errors" "html" "os" "os/exec" @@ -13,6 +14,7 @@ import ( "testing" "time" + "github.com/kristofferR/coderabbit-queue/internal/dialect" "github.com/kristofferR/coderabbit-queue/internal/engine" ) @@ -143,6 +145,114 @@ func TestInstallAutofixHonoursConfiguredDryRun(t *testing.T) { } } +func TestInstallAutofixUsesFleetRepositories(t *testing.T) { + cfg := firingConfig() + cfg.AllowRepos = nil + store := NewMemoryStore(cfg) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("repos", "owner/fleet-repo") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + + plan, err := svc.InstallAutofix(context.Background(), fakeAgent(t, "claude"), nil, nil, true) + if err != nil { + t.Fatal(err) + } + if len(plan.Repos) != 1 || plan.Repos[0] != "owner/fleet-repo" { + t.Fatalf("autofix repos = %v, want the fleet repository", plan.Repos) + } + if plan.PolicySource != "fleet" { + t.Fatalf("policy source = %q, want fleet", plan.PolicySource) + } +} + +func TestAutofixUnitKeepsHostFallbacksUnderFleetPolicy(t *testing.T) { + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/host-repo": true} + cfg.ExcludeRepos = map[string]bool{"owner/host-excluded": true} + store := NewMemoryStore(cfg) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("repos", "owner/fleet-repo") + st.SetFleetValue("exclude", "owner/fleet-excluded") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + + plan, err := svc.InstallAutofix(context.Background(), fakeAgent(t, "claude"), nil, nil, true) + if err != nil { + t.Fatal(err) + } + if len(plan.Repos) != 1 || plan.Repos[0] != "owner/fleet-repo" { + t.Fatalf("preview repos = %v, want current fleet policy", plan.Repos) + } + env := autofixEnvFor(svc.autofixFallbackConfig(nil), plan) + if got := env["CRQ_REPOS"]; got != "owner/host-repo" { + t.Fatalf("unit CRQ_REPOS = %q, want host fallback", got) + } + if got := env["CRQ_EXCLUDE"]; got != "owner/host-excluded" { + t.Fatalf("unit CRQ_EXCLUDE = %q, want host fallback", got) + } +} + +func TestAutofixUnitKeepsExplicitRepositoriesAsHostFallback(t *testing.T) { + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/configured": true} + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + plan := AutofixInstall{Repos: []string{"owner/explicit"}} + + env := autofixEnvFor(svc.autofixFallbackConfig(plan.Repos), plan) + if got := env["CRQ_REPOS"]; got != "owner/explicit" { + t.Fatalf("unit CRQ_REPOS = %q, want explicit install repository", got) + } +} + +func TestAutofixWrapperKeepsExplicitRepositoriesAuthoritative(t *testing.T) { + got := autofixWrapper("/usr/bin/crq", "'agent' 'prompt'", []string{"owner/explicit", "owner/with space"}) + if !strings.Contains(got, "watch 'owner/explicit' 'owner/with space' --") { + t.Fatalf("wrapper lost explicit repositories:\n%s", got) + } + if fallback := autofixWrapper("/usr/bin/crq", "'agent' 'prompt'", nil); !strings.Contains(fallback, "watch --") { + t.Fatalf("wrapper without explicit repositories does not use runtime fleet policy:\n%s", fallback) + } +} + +// A state ref this host cannot read must not turn the documented preview into an +// error — --dry-run is what somebody runs to inspect the setup before finishing +// it. The plan says which policy it could see, so it cannot be mistaken for the +// fleet's; a real install has no such fallback. +func TestInstallAutofixPreviewsFromTheHostWhenFleetStateIsUnreadable(t *testing.T) { + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/host-repo": true} + svc := NewService(cfg, newFakeGitHub(), unreadableStore{NewMemoryStore(cfg)}, nil) + + plan, err := svc.InstallAutofix(context.Background(), fakeAgent(t, "claude"), nil, nil, true) + if err != nil { + t.Fatal(err) + } + if plan.PolicySource != "host" || plan.Warning == "" { + t.Fatalf("plan = %+v, want a labelled host-only preview", plan) + } + if len(plan.Repos) != 1 || plan.Repos[0] != "owner/host-repo" { + t.Fatalf("preview repos = %v, want this host's own", plan.Repos) + } + if _, err := svc.InstallAutofix(context.Background(), fakeAgent(t, "claude"), nil, nil, false); err == nil { + t.Fatal("an install wrote a unit from policy it could not check against the fleet's") + } +} + +// unreadableStore is a host that cannot reach the state ref: no access to the +// gate repository, or no network to reach it over. +type unreadableStore struct{ StateStore } + +func (unreadableStore) Load(context.Context) (State, Revision, error) { + return State{}, Revision{}, errors.New("state ref unreadable") +} + // A missing agent must fail loudly at install time. Discovering it at the first // dispatch means an autofix watcher that looks installed and fixes nothing. func TestInstallAutofixRefusesWithoutAnAgent(t *testing.T) { @@ -254,7 +364,10 @@ func TestAutofixUnitCarriesEffectiveReviewerConfiguration(t *testing.T) { cfg.SkipMarker = "" cfg.Bot = "custom-reviewer[bot]" cfg.RequiredBots = []string{"custom-reviewer[bot]", "cursor[bot]"} + // Named by the operator: it reaches past who reviews, which only an explicit + // CRQ_FEEDBACK_BOTS can do — and only an explicit one travels. cfg.FeedbackBots = []string{"custom-reviewer[bot]", "cursor[bot]", "observer[bot]"} + cfg.FeedbackBotsExplicit = true cfg.ReviewCommand = "@custom review this" cfg.RateLimitCoDegrade = false cfg.MinInterval = time.Hour @@ -264,7 +377,8 @@ func TestAutofixUnitCarriesEffectiveReviewerConfiguration(t *testing.T) { cfg.SettleWindow = 17 * time.Second cfg.CoBots = []CoBotConfig{{ Name: "bugbot", Login: "cursor[bot]", Command: "bugbot run now", - Trigger: engine.TriggerAlways, Required: true, SelfHealGrace: 4 * time.Minute, + Trigger: engine.TriggerAlways, TriggerExplicit: true, + Required: true, SelfHealGrace: 4 * time.Minute, }} svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) plan := AutofixInstall{Platform: "linux", Wrapper: "/tmp/crq autofix", LogDir: "/tmp/crq logs"} @@ -295,6 +409,89 @@ func TestAutofixUnitCarriesEffectiveReviewerConfiguration(t *testing.T) { } } +// An implicit trigger must install as unset. Freezing the resolved mode into the +// unit makes the service read it as an operator's choice, and the registry +// default for a bot the fleet later REQUIRES can then never be recomputed — so a +// required Codex would be waited for and never commanded. +func TestAutofixEnvInstallsAnImplicitTriggerAsUnset(t *testing.T) { + cfg := firingConfig() + cfg.CoBots = []CoBotConfig{{ + Name: "codex", Login: dialect.CodexBotLogin, Command: "@codex review", + Trigger: engine.TriggerNever, SelfHealGrace: 10 * time.Minute, + }} + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + + env := svc.autofixEnv(AutofixInstall{}) + trigger, ok := env["CRQ_COBOT_CODEX_TRIGGER"] + if !ok || trigger != "" { + t.Fatalf("CRQ_COBOT_CODEX_TRIGGER = %q, present=%t; want an explicit empty value", trigger, ok) + } + // The installed service reloads from this environment: the mode has to come + // back implicit, so requiring the bot still promotes it to always. + reloaded := resolveCoBot(env, mustCoReviewer(t, "codex"), true) + if reloaded.TriggerExplicit { + t.Fatal("reloaded trigger is explicit; the installing host had no such setting") + } + if reloaded.Trigger != engine.TriggerAlways { + t.Fatalf("required codex reloaded with trigger %q, want always", reloaded.Trigger) + } +} + +// A derived surfaced set must install as unset, for the same reason as the +// implicit trigger: written out as a value, the service reads it back as the +// operator's own list and stops recomputing it, so a co-reviewer the fleet +// enables later never has its findings surfaced. +func TestAutofixEnvInstallsDerivedFeedbackBotsAsUnset(t *testing.T) { + cfg := firingConfig() + cfg.FeedbackBots = []string{cfg.Bot, dialect.CodexBotLogin} + cfg.FeedbackBotsExplicit = false + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + + env := svc.autofixEnv(AutofixInstall{}) + bots, ok := env["CRQ_FEEDBACK_BOTS"] + if !ok || bots != "" { + t.Fatalf("CRQ_FEEDBACK_BOTS = %q, present=%t; want an explicit empty value", bots, ok) + } +} + +// The per-bot settings of a co-reviewer this host has switched OFF still travel. +// They are what a repository override — or a fleet `cobots` change that names +// the bot without naming its keys — picks the bot up with, and the installing +// shell's own preview uses them. +func TestAutofixEnvCarriesDisabledCoBotFallbacks(t *testing.T) { + cfg := firingConfig() + cfg.CoBots = nil + cfg.KnownCoBots = []CoBotConfig{{ + Name: "bugbot", Login: "cursor[bot]", Command: "bugbot run please", + Trigger: engine.TriggerAlways, TriggerExplicit: true, + SelfHealGrace: 42 * time.Minute, + }} + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + + env := svc.autofixEnv(AutofixInstall{}) + if got := env["CRQ_COBOTS"]; got != "" { + t.Fatalf("CRQ_COBOTS = %q, want the enabled set to stay empty", got) + } + for key, want := range map[string]string{ + "CRQ_COBOT_BUGBOT_CMD": "bugbot run please", + "CRQ_COBOT_BUGBOT_TRIGGER": "always", + "CRQ_COBOT_BUGBOT_GRACE": "42m0s", + } { + if got := env[key]; got != want { + t.Errorf("%s = %q, want %q", key, got, want) + } + } +} + +func mustCoReviewer(t *testing.T, name string) dialect.CoReviewer { + t.Helper() + co, ok := dialect.CoReviewerByName(name) + if !ok { + t.Fatalf("unknown co-reviewer %q", name) + } + return co +} + func TestAutofixEnvCarriesAnIntentionallyEmptySkipMarker(t *testing.T) { cfg := firingConfig() cfg.SkipMarker = "" diff --git a/internal/crq/autofixswitch.go b/internal/crq/autofixswitch.go index 3a4a9166..88d9572d 100644 --- a/internal/crq/autofixswitch.go +++ b/internal/crq/autofixswitch.go @@ -81,8 +81,9 @@ func (s *Service) AutofixSettings(ctx context.Context) ([]AutofixSetting, error) } out = append(out, setting) } - watched := make([]string, 0, len(s.cfg.AllowRepos)) - for repo := range s.cfg.AllowRepos { + effective := s.fleetCfg(st) + watched := make([]string, 0, len(effective.AllowRepos)) + for repo := range effective.AllowRepos { watched = append(watched, repo) } sort.Strings(watched) @@ -102,13 +103,55 @@ func (s *Service) AutofixSettings(ctx context.Context) ([]AutofixSetting, error) // the hazard is a setting recorded under a name nothing will ever match. func validRepoSlug(repo string) bool { owner, name, ok := strings.Cut(repo, "/") - if !ok || owner == "" || name == "" { + return ok && validOwnerLogin(owner) && validNameSegment(name) +} + +// maxOwnerLogin is GitHub's hard length limit for a user or organisation login. +const maxOwnerLogin = 39 + +// validOwnerLogin reports whether login is a GitHub user or organisation login. +// +// Stricter than validNameSegment on purpose: a repository NAME may hold +// underscores and dots and may be long, a login may not, and a login may +// neither begin nor end with a hyphen nor hold two in a row. Everything this +// validates is looked up as /users/, so a value outside those rules is +// one no account can ever have — and recorded for the fleet, a single such +// entry fails the whole scan on every host at once, which is precisely the +// mistake worth catching at `crq config set` instead. +func validOwnerLogin(login string) bool { + if login == "" || len(login) > maxOwnerLogin || + strings.HasPrefix(login, "-") || strings.HasSuffix(login, "-") || + strings.Contains(login, "--") { + return false + } + for _, r := range login { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-': + default: + return false + } + } + return true +} + +// validNameSegment reports whether part is one segment a GitHub owner or +// repository name could be. +// +// Same segment rules the workspace package applies to a path: "." and ".." are +// not names. The character class is GitHub's: an owner or repository name is +// letters, digits, hyphen, underscore and dot, so a segment holding anything +// else — a space, a slash, a control character — is one no scan result can ever +// normalize to. Recorded, it reads as a rule covering something while covering +// nothing at all. +func validNameSegment(part string) bool { + if part == "" || part == "." || part == ".." { return false } - // Same segment rules the workspace package applies to a path: "." and ".." - // are not repository names, and a second slash means this is not one either. - for _, part := range []string{owner, name} { - if part == "." || part == ".." || strings.ContainsAny(part, `/\`) { + for _, r := range part { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '_', r == '.': + default: return false } } diff --git a/internal/crq/autofixswitch_test.go b/internal/crq/autofixswitch_test.go index 569d51d4..ade4a9e4 100644 --- a/internal/crq/autofixswitch_test.go +++ b/internal/crq/autofixswitch_test.go @@ -55,13 +55,38 @@ func TestAutofixIsOnUnlessARepositorySaysOtherwise(t *testing.T) { } } +func TestAutofixSettingsListsTheFleetRepositoryAllowlist(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/host-only": true} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("repos", "owner/fleet-only") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + + settings, err := svc.AutofixSettings(ctx) + if err != nil { + t.Fatal(err) + } + if len(settings) != 1 || settings[0].Repo != "owner/fleet-only" { + t.Fatalf("settings = %+v, want the effective fleet repository", settings) + } +} + func TestAutofixSwitchRejectsMalformedRepositoryNames(t *testing.T) { ctx := context.Background() cfg := firingConfig() store := NewMemoryStore(cfg) svc := NewService(cfg, newFakeGitHub(), store, nil) - for _, repo := range []string{"owner", "owner/repo/", "/repo", "owner/repo/extra", "../repo"} { + for _, repo := range []string{ + "owner", "owner/repo/", "/repo", "owner/repo/extra", "../repo", + "owner_name/repo", ".owner/repo", "owner--name/repo", + } { if _, err := svc.SetAutofixEnabled(ctx, repo, false, "operator stop"); err == nil { t.Errorf("SetAutofixEnabled(%q) succeeded", repo) } diff --git a/internal/crq/cliquota.go b/internal/crq/cliquota.go index 55744026..755de9da 100644 --- a/internal/crq/cliquota.go +++ b/internal/crq/cliquota.go @@ -2,6 +2,7 @@ package crq import ( "context" + "errors" "strings" "time" @@ -61,10 +62,15 @@ func (s *Service) RecordCLIQuota(ctx context.Context, report PreflightReport, cl if err := s.cfg.RequireState(); err != nil { return CLIQuotaResult{Reason: "no crq state configured, so there is no shared quota to update"}, nil } - if !s.cliOrgMatches(cliOrg) { + st, _, err := s.store.Load(ctx) + if err != nil { + return CLIQuotaResult{}, err + } + cfg := s.fleetCfg(st) + if !cliOrgMatches(cfg, cliOrg) { return CLIQuotaResult{ Reason: "the coderabbit cli is authenticated to " + orDash(cliOrg) + - ", which is not the account crq queues for (" + strings.Join(s.cfg.Scope, ",") + ")", + ", which is not the account crq queues for (" + strings.Join(cfg.Scope, ",") + ")", }, nil } @@ -75,7 +81,7 @@ func (s *Service) RecordCLIQuota(ctx context.Context, report PreflightReport, cl // conservative fallback is right: treating an unreadable window as "not // blocked" is what let the daemon re-fire every couple of minutes against // a limit measured in tens of minutes. - fallback := now.Add(cliQuotaFallback(s.cfg.RateLimitFallback)) + fallback := now.Add(cliQuotaFallback(cfg.RateLimitFallback)) until = &fallback } @@ -85,7 +91,11 @@ func (s *Service) RecordCLIQuota(ctx context.Context, report PreflightReport, cl return result, nil } - applied, standing, err := s.applyAccountBlock(ctx, *until, "coderabbit-cli") + applied, standing, err := s.applyAccountBlock(ctx, *until, "coderabbit-cli", cfg, cliOrg) + if errors.Is(err, errFleetQuotaChanged) { + result.Reason = "fleet policy changed while recording the block; run preflight again" + return result, nil + } if err != nil { return result, err } @@ -97,10 +107,12 @@ func (s *Service) RecordCLIQuota(ctx context.Context, report PreflightReport, cl return result, nil } +var errFleetQuotaChanged = errors.New("fleet policy changed while recording cli quota") + // cliOrgMatches reports whether the CLI's current organisation is the account // crq queues for. An empty org fails closed: without knowing whose limit this is, // applying it fleet-wide is the more expensive mistake. -func (s *Service) cliOrgMatches(cliOrg string) bool { +func cliOrgMatches(cfg Config, cliOrg string) bool { org := strings.ToLower(strings.TrimSpace(cliOrg)) if org == "" { return false @@ -109,7 +121,7 @@ func (s *Service) cliOrgMatches(cliOrg string) bool { // let a personal CodeRabbit org stall an unrelated scope: with // CRQ_REPO=alice/crq-state and CRQ_SCOPE=acme, Alice's local limit would have // blocked every review for acme. - for _, scope := range s.cfg.Scope { + for _, scope := range cfg.Scope { if strings.EqualFold(strings.TrimSpace(scope), org) { return true } diff --git a/internal/crq/cliquota_test.go b/internal/crq/cliquota_test.go index eb6a28b7..4a00e033 100644 --- a/internal/crq/cliquota_test.go +++ b/internal/crq/cliquota_test.go @@ -3,6 +3,7 @@ package crq import ( "context" "encoding/json" + "errors" "os" "path/filepath" "testing" @@ -98,6 +99,84 @@ func TestRecordCLIQuotaRefusesAnotherAccount(t *testing.T) { } } +func TestRecordCLIQuotaUsesFleetScopeAndFallback(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + svc, store := cliQuotaService(t, now) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("scope", "fleet-org") + st.SetFleetValue("rate-limit-fallback", "47m") + return nil + }); err != nil { + t.Fatal(err) + } + got, err := svc.RecordCLIQuota(context.Background(), blockedReport(t, "soon"), "fleet-org") + if err != nil { + t.Fatal(err) + } + want := now.Add(47 * time.Minute) + if !got.Applied || got.Until == nil || !got.Until.Equal(want) { + t.Fatalf("fleet quota result = %+v, want applied until %s", got, want) + } +} + +func TestRecordCLIQuotaRefusesAChangedFleetAccountAtCommit(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + svc, store := cliQuotaService(t, now) + st, _, err := store.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + snapshot := svc.fleetCfg(st) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("scope", "other-org") + return nil + }); err != nil { + t.Fatal(err) + } + + _, _, err = svc.applyAccountBlock( + context.Background(), now.Add(time.Hour), "coderabbit-cli", snapshot, "kristofferR", + ) + if !errors.Is(err, errFleetQuotaChanged) { + t.Fatalf("stale quota write = %v, want fleet-account refusal", err) + } + st, _, _ = store.Load(context.Background()) + if st.Account.BlockedUntil != nil { + t.Fatalf("stale CLI account block was recorded: %+v", st.Account) + } +} + +// Only the account the evidence belongs to can invalidate it. Comparing the +// whole fleet revision meant any unrelated setting moving between the read and +// the write refused an explicit, organisation-attributed block: the operator was +// told to run preflight again while the shared quota stayed open, and the daemon +// could post a metered review inside the window the CLI had just reported. +func TestRecordCLIQuotaSurvivesAnUnrelatedFleetChange(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + svc, store := cliQuotaService(t, now) + st, _, err := store.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + snapshot := svc.fleetCfg(st) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("settle", "9m") + return nil + }); err != nil { + t.Fatal(err) + } + + applied, standing, err := svc.applyAccountBlock( + context.Background(), now.Add(time.Hour), "coderabbit-cli", snapshot, "kristofferR", + ) + if err != nil { + t.Fatal(err) + } + if !applied || standing == nil || !standing.Equal(now.Add(time.Hour)) { + t.Fatalf("applied=%v standing=%v, want the block recorded across the unrelated change", applied, standing) + } +} + // A window read from a PR comment is authoritative about the whole account; a // local reading may be a narrower limit. Extending is safe, shortening is not. func TestRecordCLIQuotaNeverShortensAStandingBlock(t *testing.T) { diff --git a/internal/crq/config.go b/internal/crq/config.go index 50e878e4..7175c257 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -83,7 +83,17 @@ type Config struct { 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 + OverrideAt *time.Time + // FleetRevision identifies the fleet policy snapshot this configuration was + // built from. Like OverrideAt, it is revalidated inside the CAS mutation + // that commits a decision. + FleetRevision string + // ExplicitFleetEnv records fleet-owned variables supplied by either the + // config file or the process environment, under the name the host actually + // used — a legacy alias or per-bot key counts, since the fleet overrides it + // just the same. LoadConfig does not export normal config-file values, so + // os.Getenv alone cannot detect that divergence. + ExplicitFleetEnv map[string]bool RateLimitCommand string RateLimitMarker string CalibrationMarker string @@ -261,6 +271,14 @@ func LoadConfig() (Config, error) { RateLimitCoDegrade: stringEnv(env, "CRQ_RL_CO_DEGRADE", stringEnv(env, "CRQ_RL_CODEX_DEGRADE", "1")) != "0", } + cfg.ExplicitFleetEnv = map[string]bool{} + for _, setting := range fleetSettings() { + for _, name := range setting.envNames() { + if _, ok := env[name]; ok { + cfg.ExplicitFleetEnv[name] = true + } + } + } if len(cfg.Scope) == 0 && cfg.GateRepo != "" { cfg.Scope = []string{ownerOf(cfg.GateRepo)} } diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index c41233e7..335dd658 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -702,7 +702,7 @@ func (s *Service) Loop(ctx context.Context, repo string, pr int) (FeedbackReport if settledAt.IsZero() { settledAt = s.clock() } - if s.cfg.SettleWindow <= 0 || s.clock().Sub(settledAt) >= s.cfg.SettleWindow { + if report.config.SettleWindow <= 0 || s.clock().Sub(settledAt) >= report.config.SettleWindow { if report.Converged { s.completeWaitRound(ctx, repo, pr, head, report.PrimaryAckPending, &report.config) } @@ -864,6 +864,15 @@ func (s *Service) pushWaitDeadline(ctx context.Context, repo string, pr int, hea } } +// inflightTimeout is the in-flight window in force: the fleet's when the caller +// decided from a fleet configuration, this host's when it had none. +func (s *Service) inflightTimeout(cfg *Config) time.Duration { + if cfg != nil { + return cfg.InflightTimeout + } + return s.cfg.InflightTimeout +} + // 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. @@ -898,9 +907,12 @@ func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, he // 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. + // Bounded by the in-flight window, the deadline Progress gives up at — + // the FLEET's, revalidated just above, not this host's. A host whose + // local value is shorter would otherwise drop the hold while Progress + // is still waiting on the command, letting another metered review fire. if r.FiredAt != nil { - until := r.FiredAt.UTC().Add(s.cfg.InflightTimeout) + until := r.FiredAt.UTC().Add(s.inflightTimeout(cfg)) if until.After(s.clock()) && (st.FireSlot.HoldUntil == nil || st.FireSlot.HoldUntil.Before(until)) { st.HoldSlotUntil(until) changed = true @@ -912,7 +924,7 @@ func (s *Service) completeWaitRound(ctx context.Context, repo string, pr int, he if err := r.Complete(); err != nil { return err } - releaseSlot(st, QueueKey(repo, pr)) + releaseSlot(st, QueueKey(repo, pr), r.Token) st.PutRound(*r) changed = true return nil diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index 8947b540..a94f1a71 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -100,6 +100,45 @@ func TestFeedbackReturnsObservedAccountBlockPersistenceFailure(t *testing.T) { } } +func TestFeedbackUsesTheFleetFallbackForAWindowlessAccountBlock(t *testing.T) { + cfg := firingConfig() + cfg.RateLimitFallback = 5 * time.Minute + now := time.Now().UTC() + gh := newFakeGitHub() + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "abcdef1234567890" + gh.pulls[fakeKey("o/repo", 3)] = pull + notice := ghapi.IssueComment{ + ID: 17, Body: "You are rate limited by coderabbit.ai. Please wait before requesting another review.", + CreatedAt: now, UpdatedAt: now, + } + notice.User.Login = cfg.Bot + gh.comments[fakeKey("o/repo", 3)] = []ghapi.IssueComment{notice} + + store := NewMemoryStore(cfg) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("rate-limit-fallback", "45m") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + svc.now = func() time.Time { return now } + + if _, err := svc.Feedback(context.Background(), "o/repo", 3); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + want := now.Add(45 * time.Minute) + if st.Account.BlockedUntil == nil || !st.Account.BlockedUntil.Equal(want) { + t.Fatalf("blocked until = %v, want fleet fallback %s", st.Account.BlockedUntil, want) + } +} + func TestFeedbackCountsCompletionReplyForFiredHead(t *testing.T) { // A re-review with nothing new to say produces no review object: CodeRabbit // only replies "Review finished" to the command. That reply must satisfy @@ -2362,6 +2401,45 @@ func TestCompleteWaitRoundHoldsAnUnacknowledgedFireSlot(t *testing.T) { } } +// The hold is bounded by the in-flight window Progress gives up at, and that +// window is fleet policy. A host whose own value is shorter would otherwise +// stamp a hold that lapses while Progress is still waiting on the command, +// letting the next pull request spend the allowance alongside it. +func TestHeldFireSlotUsesTheFleetsInflightTimeout(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.InflightTimeout = time.Minute // this host's own, shorter, answer + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + now := time.Now().UTC() + repo, pr, head := "o/r", 6, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 12) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("inflight-timeout", "2h") + return nil + }); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + fleet := svc.cfgFor(st, repo) + + svc.completeWaitRound(ctx, repo, pr, head, true, &fleet) + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.FireSlot == nil || st.FireSlot.HoldUntil == nil { + t.Fatalf("the slot must be held for the unanswered command, got %+v", st.FireSlot) + } + want := now.Add(2 * time.Hour) + if !st.FireSlot.HoldUntil.Equal(want) { + t.Fatalf("hold until = %s, want the fleet's window %s", st.FireSlot.HoldUntil, want) + } +} + // 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 diff --git a/internal/crq/fleetcmd.go b/internal/crq/fleetcmd.go new file mode 100644 index 00000000..cad6120a --- /dev/null +++ b/internal/crq/fleetcmd.go @@ -0,0 +1,565 @@ +package crq + +import ( + "context" + "errors" + "fmt" + "net/http" + "sort" + "strings" + + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +// FleetSetting is one policy as `crq config` reports it: what the fleet +// records, what this host would use on its own, and which of the two is in +// effect. +type FleetSetting struct { + Key string `json:"key"` + Doc string `json:"doc"` + Env string `json:"env"` + Value string `json:"value"` + Source string `json:"source"` // "fleet" or "host" + // HostValue is what this machine's own environment says, reported only when + // it differs from what is in force. It is the divergence an operator needs + // to see: a setting they changed on this box that the fleet is overriding. + HostValue *string `json:"host_value,omitempty"` + // Error is why a recorded value was not applied, when this binary could not + // read it. + Error string `json:"error,omitempty"` + // Unknown marks a setting the fleet records that this binary has no notion + // of — written by a newer crq. Nothing is in force for it here, so Value and + // Env are empty and only the key is worth reporting. + Unknown bool `json:"unknown,omitempty"` + // Lagging names active queue drivers that cannot enforce fleet policy. + Lagging []string `json:"lagging_hosts,omitempty"` +} + +// FleetConfig reports every fleet setting, in force and as this host has it — +// including the ones only a newer crq knows. Reporting just the keys this binary +// understands would hide exactly the case that matters: a recorded setting this +// process silently ignores, while `crq config` and `crq doctor` call the host +// fully in step with the fleet. +func (s *Service) FleetConfig(ctx context.Context) ([]FleetSetting, error) { + st, _, err := s.store.Load(ctx) + if err != nil { + return nil, err + } + settings := fleetSettings() + effective := s.fleetCfg(st) + lagging := st.LaggingWriters(CapsFleetPolicy, s.clock()) + out := make([]FleetSetting, 0, len(settings)) + for _, key := range FleetKeys() { + setting := settings[key] + item := FleetSetting{ + Key: key, Doc: setting.Doc, Env: setting.Env, + Value: setting.Show(effective), Source: "host", Lagging: lagging, + } + if recorded, ok := st.FleetValue(key); ok { + item.Source = "fleet" + if err := ValidateFleetSetting(key, recorded); err != nil { + item.Source, item.Error = "host", err.Error() + } + } + if host := setting.Show(s.cfg); host != item.Value { + item.HostValue = &host + } + out = append(out, item) + } + for _, key := range st.FleetKeys() { + if _, known := settings[key]; known { + continue + } + out = append(out, FleetSetting{ + Key: key, + Unknown: true, + Source: "host", + Error: "not a setting this crq understands; it is being ignored on this host", + Lagging: lagging, + }) + } + return out, nil +} + +// SetFleetConfig records one setting for every host, refusing a value this +// binary cannot read: the ref is shared, so a value only some hosts can parse +// breaks the fleet from the inside. +func (s *Service) SetFleetConfig(ctx context.Context, key, value string) error { + if err := ValidateFleetSetting(key, value); err != nil { + return err + } + open, err := s.prepareFleetReviewerChange(ctx, key, func(st State) bool { + current, recorded := st.FleetValue(key) + return !recorded || current != value + }) + if err != nil { + return err + } + _, err = s.updateFleet(ctx, func(st *State) error { + if err := s.requireFleetCapableDrivers(st); err != nil { + return err + } + before := s.fleetCfg(*st) + current, recorded := st.FleetValue(key) + if recorded && current == value { + return ErrNoChange + } + st.SetFleetValue(key, value) + change := fleetChange{before: before, after: s.fleetCfg(*st)} + if !recorded { + change.adopted = []string{key} + } + return s.reconcileFleetChange(st, change, open) + }) + return err +} + +// UnsetFleetConfig returns one setting to whatever each host says, reporting +// whether the fleet had an opinion to drop. +// +// A key this binary does not know is refused only when the fleet has not +// recorded it either. Dropping one it HAS recorded is the remedy `crq doctor` +// names for a setting a newer crq wrote and this host ignores, and refusing it +// would leave that key unremovable from every host but the newest — but only +// while no newer binary is driving the queue, see requireNoAdvancedDrivers. +func (s *Service) UnsetFleetConfig(ctx context.Context, key string) (bool, error) { + known := false + if _, ok := fleetSettings()[key]; ok { + known = true + } + open, err := s.prepareFleetReviewerChange(ctx, key, func(st State) bool { + _, recorded := st.FleetValue(key) + return recorded + }) + if err != nil { + return false, err + } + dropped := false + _, err = s.updateFleet(ctx, func(st *State) error { + if _, recorded := st.FleetValue(key); !known && !recorded { + return fmt.Errorf("unknown setting %q", key) + } + if err := s.requireFleetCapableDrivers(st); err != nil { + return err + } + if !known { + if err := s.requireNoAdvancedDrivers(st, key); err != nil { + return err + } + } + before := s.fleetCfg(*st) + dropped = st.UnsetFleetValue(key) + if !dropped { + return ErrNoChange + } + return s.reconcileFleetChange(st, fleetChange{before: before, after: s.fleetCfg(*st)}, open) + }) + return dropped, err +} + +// SeedFleetConfig records this host's settings as the fleet's, for the ones the +// fleet has no answer for yet. +// +// It is how an existing setup adopts this without retyping it: run it once on +// the machine whose configuration is the one you mean. Settings the fleet has +// already recorded are left alone — seeding twice from two machines would +// otherwise make the last one to run the winner, which is the divergence this +// whole mechanism exists to end. +func (s *Service) SeedFleetConfig(ctx context.Context) ([]string, error) { + settings := fleetSettings() + rendered := make(map[string]string, len(settings)) + for _, key := range FleetKeys() { + rendered[key] = settings[key].Show(s.cfg) + } + var seeded []string + initial, _, err := s.store.Load(ctx) + if err != nil { + return nil, err + } + var open map[string]map[int]bool + if seedsReviewers(initial) { + open, err = s.openFleetPRs(ctx, initial) + if err != nil { + return nil, err + } + } + _, err = s.updateFleet(ctx, func(st *State) error { + if err := s.requireFleetCapableDrivers(st); err != nil { + return err + } + before := s.fleetCfg(*st) + seeded = seeded[:0] + for _, key := range FleetKeys() { + if _, ok := st.FleetValue(key); ok { + continue + } + if err := ValidateFleetSetting(key, rendered[key]); err != nil { + return fmt.Errorf("cannot seed %s from this host: %w", key, err) + } + seeded = append(seeded, key) + } + for _, key := range seeded { + st.SetFleetValue(key, rendered[key]) + } + if len(seeded) == 0 { + return ErrNoChange + } + return s.reconcileFleetChange(st, fleetChange{before: before, after: s.fleetCfg(*st), adopted: seeded}, open) + }) + return seeded, err +} + +// updateFleet runs the exact same mutation for a preview and a real update. +// Dry-run differs only at the persistence boundary, so validation, capability +// fences and reconciliation cannot drift from the command it previews. +func (s *Service) updateFleet(ctx context.Context, mutate func(*State) error) (State, error) { + changed := false + apply := func(st *State) error { + err := mutate(st) + if err == nil { + changed = true + } + return err + } + if !s.cfg.DryRun { + st, err := s.store.Update(ctx, apply) + if err == nil && changed { + s.sync(ctx, st) + } + return st, err + } + st, _, err := s.store.Load(ctx) + if err != nil { + return State{}, err + } + err = apply(&st) + if errors.Is(err, ErrNoChange) { + err = nil + } + return st, err +} + +// FleetDivergence lists the settings this host still answers for itself while +// the fleet records them, for `crq doctor`. +// +// A host that quietly disagrees is the failure this is about: everything looks +// healthy on both machines, and a repository is excluded on one and reviewed by +// the other. Naming the variable is the point — the remedy is to delete it from +// this host's environment. +func (s *Service) FleetDivergence(ctx context.Context) ([]string, error) { + items, err := s.FleetConfig(ctx) + if err != nil { + return nil, err + } + var out []string + settings := fleetSettings() + for _, item := range items { + // Every name the host is still feeding this policy from, not just the + // canonical one: a legacy alias left behind is what the host falls back to + // if the fleet key is ever unset. + hostEnv := explicitEnv(s.cfg, settings[item.Key]) + switch { + case item.Unknown: + out = append(out, fmt.Sprintf("%s is recorded for the fleet but is not a setting this crq understands; upgrade crq on this host or run crq config unset %s", + item.Key, item.Key)) + case item.Error != "": + out = append(out, fmt.Sprintf("%s: the fleet's value is one this crq cannot read (%s); using this host's %q", + item.Key, item.Error, item.Value)) + case item.Source == "fleet" && item.HostValue != nil && len(hostEnv) > 0: + out = append(out, fmt.Sprintf("%s is %q for the fleet, but %s is set to %q on this host; remove it or run crq config set %s", + item.Key, item.Value, strings.Join(hostEnv, ", "), *item.HostValue, item.Key)) + case item.Source == "fleet" && len(hostEnv) > 0: + // A host copy that currently AGREES is still worth naming. Nothing + // misbehaves while the fleet records the key — but `crq config unset` + // hands the setting back to whatever each host says, and this host + // then falls back to a value nobody remembers setting, diverging from + // every host without it. That is the same failure, one command later, + // and by then there is no recorded value left to compare against and + // nothing here would report it. + out = append(out, fmt.Sprintf("%s is %q for the fleet and %s is still set to that value on this host; remove it, or unsetting %s silently returns this host to its own copy", + item.Key, item.Value, strings.Join(hostEnv, ", "), item.Key)) + } + } + return out, nil +} + +func (s *Service) requireFleetCapableDrivers(st *State) error { + if lagging := st.LaggingWriters(CapsFleetPolicy, s.clock()); len(lagging) > 0 { + return fmt.Errorf("cannot activate fleet policy while queue drivers lack fleet-policy support: %v", lagging) + } + return nil +} + +// requireNoAdvancedDrivers refuses to drop a setting this binary cannot +// interpret while a newer one is driving the queue. +// +// Removing a recorded key is the remedy `crq doctor` names for a setting a newer +// crq wrote, and it has to keep working — an unremovable key would be worse. But +// the capability fence above only asks for THIS binary's fixed CapsFleetPolicy, +// which a newer driver passes by definition, so nothing else stops an old CLI +// deleting a policy it can neither validate nor reconcile. If that key drives +// reviewers, exclusions or quota, the newer driver sees it vanish without the +// cleanup its removal calls for. The setting's own binary is the one that may +// drop it; here the remedy is to run the unset from an upgraded host. +func (s *Service) requireNoAdvancedDrivers(st *State, key string) error { + if ahead := st.AdvancedWriters(WriterCaps, s.clock()); len(ahead) > 0 { + return fmt.Errorf("cannot unset %q from this crq: it is not a setting this binary understands, and newer queue drivers are running: %v; upgrade crq on this host and unset it there", + key, ahead) + } + return nil +} + +// prepareFleetReviewerChange fetches the open PRs reconciliation needs, for the +// keys that can actually move a round. +// +// Only MEMBERSHIP asks that question. reopenForChangedReviewers compares the +// required and co-reviewer login sets and returns before touching this map when +// they match, and a co-reviewer's command, trigger mode or self-heal grace never +// moves either — so asking for the open PRs of every repository ever recorded in +// Rounds bought nothing, spent a REST lookup per historical repository, and made +// a purely local timing update fail whenever one of them was throttled. +// +// A membership key that is not actually moving asks it just as pointlessly, and +// `changes` is what settles that against the recorded value before anything is +// spent: re-recording the value the fleet already holds, or unsetting a key it +// never held, reconciles nothing, and without this an idempotent command still +// cost a lookup per historical repository and still failed under throttling. The +// recorded value is read again inside the CAS, so a concurrent write landing +// between the two leaves `open` nil rather than wrong — the same degraded case an +// inaccessible repository already produces, where a completed round is marked +// rather than reopened and the next enqueue that finds the PR alive reopens it. +func (s *Service) prepareFleetReviewerChange(ctx context.Context, key string, changes func(State) bool) (map[string]map[int]bool, error) { + if !isReviewerMembershipFleetKey(key) { + return nil, nil + } + st, _, err := s.store.Load(ctx) + if err != nil { + return nil, err + } + if !changes(st) { + return nil, nil + } + return s.openFleetPRs(ctx, st) +} + +// seedsReviewers reports whether seeding would record a reviewer setting the +// fleet has no answer for yet, which is the case where the effective reviewers +// can move and existing rounds have to be reconciled against them. Same +// membership-only question as prepareFleetReviewerChange, for the same reason. +func seedsReviewers(st State) bool { + for _, key := range FleetKeys() { + if !isReviewerMembershipFleetKey(key) { + continue + } + if _, ok := st.FleetValue(key); !ok { + return true + } + } + return false +} + +// fleetChange is one policy mutation as reconciliation sees it: the effective +// configuration before and after it, plus the keys the fleet had no answer for +// until now. +// +// Those adopted keys are why `before` cannot be taken at face value. fleetCfg +// fills an unrecorded setting from THIS host, so the first `crq config set` or +// `crq config seed` of a value this machine was already using renders before and +// after identical — while every other host may have been acting under a +// different one all along. For an adopted key the fleet-wide baseline is +// unknown, and reconciliation has to assume it differed rather than assume it +// matched. +type fleetChange struct { + before, after Config + adopted []string +} + +func (c fleetChange) adopts(key string) bool { + for _, adopted := range c.adopted { + if adopted == key { + return true + } + } + return false +} + +// baseline is the pre-change configuration with each adopted key's host fallback +// removed, so a policy the fleet is only now recording is reconciled as the +// change it is for everyone else. +// +// A reviewer MEMBERSHIP key drops the whole reviewer set: another host may have +// completed a head without a bot the adoption makes fleet-required, and that +// host's completed round is the dedup marker that would keep the newly required +// bot from ever being triggered. It is conservative, not expensive — a reopened +// round the primary already answered dedupes at DecideFire's already-reviewed +// gate instead of buying a second review, and a repository that pins its own +// reviewers still decides its effective set, so its rounds compare equal and are +// left alone. Adopting `exclude` likewise drops the baseline exclusions, so a +// repository this host already skipped still has to pass the claimed-trigger +// refusal that the rest of the fleet never applied to it. +// +// The per-bot keys are deliberately not in that set. Adopting a co-reviewer's +// command, trigger mode or self-heal grace moves nobody in or out of the +// reviewer set, and reconciliation compares membership — so erasing the baseline +// for one would reopen every completed round in the fleet and force a self-heal +// trigger post on every open PR, for a timing value. Ordinary updates to the +// same key reopen nothing, and adoption has no more to reconcile than they do. +func (c fleetChange) baseline() Config { + before := c.before + for _, key := range c.adopted { + if isReviewerMembershipFleetKey(key) { + before.RequiredBots, before.CoBots, before.Reviewers = nil, nil, nil + } + if key == "exclude" { + before.ExcludeRepos = nil + } + } + return before +} + +func (s *Service) openFleetPRs(ctx context.Context, st State) (map[string]map[int]bool, error) { + repos := map[string]bool{} + for _, round := range st.Rounds { + repos[NormalizeRepo(round.Repo)] = true + } + open := make(map[string]map[int]bool, len(repos)) + for repo := range repos { + pulls, err := s.openPRs(ctx, repo) + if err != nil { + if !inaccessibleRepoLookup(err) { + return nil, err + } + // Completed rounds are permanent dedup markers, so repositories that + // were deleted or became inaccessible remain in state indefinitely. + // Treat their PRs as closed for this reconciliation: the rounds are + // marked and will reopen if a later enqueue proves the PR is live. + if s.log != nil { + s.log.Printf("warning: reviewer change could not inspect historical repository %s: %v", repo, err) + } + continue + } + open[repo] = pulls + } + return open, nil +} + +func inaccessibleRepoLookup(err error) bool { + if ghapi.IsThrottled(err) { + return false + } + if errors.Is(err, ghapi.ErrNotFound) { + return true + } + var apiErr *ghapi.APIError + return errors.As(err, &apiErr) && apiErr.Status == http.StatusForbidden +} + +// reconcileFleetChange applies what a policy change invalidates — and refuses +// the change outright when there is a network effect it cannot take back. +func (s *Service) reconcileFleetChange(st *State, change fleetChange, open map[string]map[int]bool) error { + before, after := change.baseline(), change.after + if err := checkExcludedTriggerPosts(st, before, after); err != nil { + return err + } + if fleetScopeMoved(st, change) { + st.Account = AccountQuota{ + Scope: strings.Join(after.Scope, ","), + Source: "fleet scope changed", + } + } + for _, round := range st.Rounds { + if round.Phase != PhaseQueued && round.Phase != PhaseAwaitingRetry { + continue + } + if !after.ExcludeRepos[NormalizeRepo(round.Repo)] { + continue + } + // A queued round holds no slot, so this releases one only when the round + // it retires is the one that took it — which it never is here. An orphaned + // hold at the same key stands for a metered command a previous round + // posted and is left alone: excluding a repository stops the next review, + // it does not answer the one already in flight. + st.EndRound(round.Repo, round.PR, "repository excluded by fleet policy") + releaseSlot(st, QueueKey(round.Repo, round.PR), round.Token) + } + s.reopenForFleetReviewerChange(st, before, after, open) + return nil +} + +// checkExcludedTriggerPosts refuses an exclusion that would race a trigger post +// already claimed for the repository being excluded. +// +// The reservation is a fire's commit point, but the command goes to GitHub +// after it: a round left reserved here is one whose post is already authorized, +// so retiring it would not stop the excluded repository receiving a review +// command — it would only destroy the record of quota the account is about to +// be charged for. As with a hold, the claim and this write are both CAS, which +// makes either ordering safe: an existing claim rejects the exclusion, and an +// exclusion recorded first stops the next fire. +func checkExcludedTriggerPosts(st *State, before, after Config) error { + var blocked []string + for _, round := range st.Rounds { + repo := NormalizeRepo(round.Repo) + if !after.ExcludeRepos[repo] || before.ExcludeRepos[repo] { + continue + } + if triggerPostClaimed(&round) { + blocked = append(blocked, QueueKey(round.Repo, round.PR)) + } + } + if len(blocked) == 0 { + return nil + } + sort.Strings(blocked) + return fmt.Errorf("a review trigger is already being posted for %s; wait for it to finish before excluding it", + strings.Join(blocked, ", ")) +} + +// fleetScopeMoved reports whether the recorded account quota belongs to an +// account other than the one the fleet now scans — which is what makes it +// meaningless rather than merely stale. +// +// Ordinarily that is just "did the scope change". Adopting the scope needs the +// other question: fleetCfg fills an unrecorded setting from THIS host, so the +// first `crq config set scope` compares equal to itself even when the fleet was +// scanning something else entirely. The quota's own recorded scope is the one +// piece of that account's identity that survived, so ask it instead. +func fleetScopeMoved(st *State, change fleetChange) bool { + if change.adopts("scope") { + return !sameFoldedSet(splitList(st.Account.Scope), change.after.Scope) + } + return !sameFoldedSet(change.before.Scope, change.after.Scope) +} + +func sameFoldedSet(a, b []string) bool { + if len(a) != len(b) { + return false + } + seen := make(map[string]int, len(a)) + for _, value := range a { + seen[strings.ToLower(strings.TrimSpace(value))]++ + } + for _, value := range b { + key := strings.ToLower(strings.TrimSpace(value)) + seen[key]-- + if seen[key] < 0 { + return false + } + } + return true +} + +// reopenForFleetReviewerChange requeues what a fleet reviewer change invalidates. +// Which rounds those are is decided per repository, against that repository's +// override — the fleet's required set is only half of the effective one, and a +// co-reviewer the fleet enables or disables moves it just as much. +func (s *Service) reopenForFleetReviewerChange(st *State, before, after Config, open map[string]map[int]bool) { + repos := map[string]bool{} + for _, round := range st.Rounds { + repos[NormalizeRepo(round.Repo)] = true + } + for repo := range repos { + ov, _ := st.RepoOverride(repo) + s.reopenForChangedReviewers(st, repo, before.ForRepo(ov), after.ForRepo(ov), open[repo]) + } +} diff --git a/internal/crq/fleetconfig.go b/internal/crq/fleetconfig.go new file mode 100644 index 00000000..04a9153c --- /dev/null +++ b/internal/crq/fleetconfig.go @@ -0,0 +1,663 @@ +package crq + +import ( + "fmt" + "sort" + "strconv" + "strings" + "time" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + "github.com/kristofferR/coderabbit-queue/internal/engine" +) + +// fleetSetting is one policy the whole fleet shares: how to read it from the +// state ref onto a Config, and how to render what a host is currently using. +// +// The registry below is the definition of "fleet policy". A setting listed here +// stops being something each machine answers for itself; one that is not listed +// stays local, and the split is deliberate — see FleetKeys. +type fleetSetting struct { + // Doc is what the setting means, for `crq config`. + Doc string + // Env names the variable it used to come from, so an operator can find it. + Env string + // AltEnv names the other variables a host may still feed this policy from: + // legacy aliases and per-bot keys. A host exporting one of these diverges + // from the fleet exactly as the canonical name does, and `crq doctor` has to + // say so — otherwise the stale setting stays, invisible, and the host falls + // back to it the moment the fleet key is unset. + AltEnv []string + // Apply writes value onto cfg, rejecting a value it cannot parse. A bad + // value must fail where it is SET, but it is validated here too: the state + // ref is shared, and a host reading a value it cannot use has to say so + // rather than silently keep its own. + Apply func(cfg *Config, value string) error + // Show renders what cfg is currently using, for comparison. + Show func(cfg Config) string +} + +// fleetSettings is every setting that belongs to the fleet rather than to a +// machine. +// +// What is NOT here matters as much. Three kinds stay local: where the state +// lives (CRQ_REPO, CRQ_ISSUE, CRQ_STATE_REF — a host cannot read fleet config +// until it knows where to look), credentials, and what this machine can +// physically do (which fix agent is installed, where its disk is, how many +// sessions it can take). Everything else is policy, and policy the hosts +// disagree about is how a repository ends up excluded on one and reviewed by +// another with nothing to say so. +// +// The primary reviewer is the one policy deliberately left out, and it is a +// FOURTH kind rather than an oversight: who the primary is (CRQ_BOT) is +// inseparable from the wording crq reads it by (CRQ_REVIEW_CMD and the +// completion, rate-limit, review-done and calibration markers), and those are +// compiled into the dialect classifiers when the Service is constructed — +// before any state ref has been read. Recording the login and its command while +// the markers stayed per-host would make a host post the fleet's command for a +// bot it classifies with its own wording, which is the half-applied policy +// applyFleet refuses everywhere else. Centralizing it means moving the whole +// family and building the classifiers from the fleet-applied configuration, not +// adding two entries here. +func fleetSettings() map[string]fleetSetting { + settings := map[string]fleetSetting{ + "scope": { + Doc: "owners crq scans when no repository list is set", + Env: "CRQ_SCOPE", + Apply: func(cfg *Config, v string) error { + owners, err := fleetOwners(v) + cfg.Scope = owners + return err + }, + Show: func(cfg Config) string { return strings.Join(cfg.Scope, ",") }, + }, + "repos": { + Doc: "the repositories crq reviews and watches", + Env: "CRQ_REPOS", + Apply: func(cfg *Config, v string) error { + repos, err := fleetRepoSet(v) + cfg.AllowRepos = repos + return err + }, + Show: func(cfg Config) string { return strings.Join(sortedSetKeys(cfg.AllowRepos), ",") }, + }, + "exclude": { + Doc: "repositories crq never reviews, watches or fixes", + Env: "CRQ_EXCLUDE", + Apply: func(cfg *Config, v string) error { + repos, err := fleetRepoSet(v) + cfg.ExcludeRepos = repos + return err + }, + Show: func(cfg Config) string { return strings.Join(sortedSetKeys(cfg.ExcludeRepos), ",") }, + }, + requiredBotsKey: { + Doc: "logins that must review a head before a round converges", + Env: "CRQ_REQUIRED_BOTS", + AltEnv: coBotRequiredEnvs(), + Apply: func(cfg *Config, v string) error { + cfg.RequiredBots = splitList(v) + if len(cfg.RequiredBots) == 0 { + return fmt.Errorf("at least one required bot is needed") + } + return nil + }, + Show: func(cfg Config) string { return strings.Join(cfg.RequiredBots, ",") }, + }, + feedbackBotsKey: { + Doc: "logins whose findings crq surfaces beyond the ones it waits for (empty follows required-bots and cobots)", + Env: "CRQ_FEEDBACK_BOTS", + Apply: func(cfg *Config, v string) error { + logins := splitList(v) + // Present-but-empty is not a choice here either: it is the + // environment's "unset", so the surfaced set goes back to being + // derived from who reviews, recomputed rather than frozen. + cfg.FeedbackBots, cfg.FeedbackBotsExplicit = logins, len(logins) > 0 + if !cfg.FeedbackBotsExplicit { + cfg.FeedbackBots = cfg.reviewerLogins(func(r Reviewer) bool { return r.Required || !r.Metered() }) + } + return nil + }, + Show: func(cfg Config) string { + if !cfg.FeedbackBotsExplicit { + return "" + } + return strings.Join(cfg.FeedbackBots, ",") + }, + }, + "min-interval": { + Doc: "floor between two metered reviews", + Env: "CRQ_MIN_INTERVAL", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + cfg.MinInterval = d + return err + }, + Show: func(cfg Config) string { return cfg.MinInterval.String() }, + }, + "inflight-timeout": { + Doc: "how long a metered review command may remain unanswered", + Env: "CRQ_INFLIGHT_TIMEOUT", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + cfg.InflightTimeout = d + return err + }, + Show: func(cfg Config) string { return cfg.InflightTimeout.String() }, + }, + "rate-limit-fallback": { + Doc: "how long to wait when a rate-limit notice states no window", + Env: "CRQ_RL_FALLBACK", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + cfg.RateLimitFallback = d + return err + }, + Show: func(cfg Config) string { return cfg.RateLimitFallback.String() }, + }, + "calibrate-ttl": { + Doc: "how long a calibration answer counts as current", + Env: "CRQ_CALIBRATE_TTL", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + cfg.CalibrationTTL = d + return err + }, + Show: func(cfg Config) string { return cfg.CalibrationTTL.String() }, + }, + "settle": { + Doc: "quiet window before a PR counts as converged", + Env: "CRQ_SETTLE", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + cfg.SettleWindow = d + return err + }, + Show: func(cfg Config) string { return cfg.SettleWindow.String() }, + }, + "skip-marker": { + Doc: "text in a PR body that opts it out of review (empty disables the opt-out)", + Env: "CRQ_AUTOREVIEW_SKIP_MARKER", + Apply: func(cfg *Config, v string) error { + cfg.SkipMarker = v + return nil + }, + Show: func(cfg Config) string { return cfg.SkipMarker }, + }, + "skip-authors": { + Doc: "PR authors autoreview never enqueues (empty reviews every author)", + Env: "CRQ_AUTOREVIEW_SKIP_AUTHORS", + Apply: func(cfg *Config, v string) error { + cfg.SkipAuthors = authorSet(v) + return nil + }, + Show: func(cfg Config) string { return strings.Join(sortedSetKeys(cfg.SkipAuthors), ",") }, + }, + coBotsKey: { + Doc: "co-reviewers crq surfaces and triggers (empty disables all)", + Env: "CRQ_COBOTS", + // A required bot is enabled whatever CRQ_COBOTS lists, so the per-bot + // required keys shape this set too. + AltEnv: coBotRequiredEnvs(), + Apply: func(cfg *Config, v string) error { + names, err := fleetCoBotNames(v) + if err != nil { + return err + } + *cfg = cfg.ForRepo(RepoReviewers{CoBots: names, SetCoBots: true}) + return nil + }, + Show: func(cfg Config) string { + names := make([]string, 0, len(cfg.CoBots)) + for _, cb := range cfg.CoBots { + names = append(names, cb.Name) + } + return strings.Join(names, ",") + }, + }, + "rate-limit-co-degrade": { + Doc: "run co-reviewer-only rounds while the CodeRabbit account is blocked", + Env: "CRQ_RL_CO_DEGRADE", + AltEnv: []string{"CRQ_RL_CODEX_DEGRADE"}, + Apply: func(cfg *Config, v string) error { + on, err := parseFleetBool(v) + if err != nil { + return err + } + cfg.RateLimitCoDegrade = on + return nil + }, + Show: func(cfg Config) string { return fleetBool(cfg.RateLimitCoDegrade) }, + }, + } + // Every co-reviewer's own policy, one family per bot. Nothing here names a + // bot: the registry is what says which exist, so a new co-reviewer arrives + // with its fleet settings already in place. + for _, co := range dialect.KnownCoReviewers() { + for key, setting := range coBotFleetSettings(co) { + settings[key] = setting + } + } + return settings +} + +// coBotFleetSettings is how the fleet drives one co-reviewer: whether crq posts +// its command, what it posts, and how long a self-heal nudge waits first. +// +// Required-ness is deliberately NOT here — required-bots owns that list, and two +// settings answering the same question is how they end up disagreeing. +func coBotFleetSettings(co dialect.CoReviewer) map[string]fleetSetting { + name := co.Name + env := "CRQ_COBOT_" + strings.ToUpper(name) + var legacyCommand []string + if co.LegacyCommandEnv != "" { + legacyCommand = []string{co.LegacyCommandEnv} + } + return map[string]fleetSetting{ + "cobot-" + name + "-trigger": { + Doc: "when crq posts " + name + "'s command: never, selfheal or always (empty leaves it to how the bot is required)", + Env: env + "_TRIGGER", + Apply: func(cfg *Config, v string) error { + value := strings.ToLower(strings.TrimSpace(v)) + if value == "" { + // The environment's "unset" — the registry default for how the + // bot is required, recomputed rather than frozen. + updateCoBot(cfg, name, func(cb *CoBotConfig) { cb.TriggerExplicit = false }) + return nil + } + mode := engine.TriggerMode(value) + switch mode { + case engine.TriggerNever, engine.TriggerSelfHeal, engine.TriggerAlways: + default: + return fmt.Errorf("trigger must be never, selfheal or always, got %q", v) + } + updateCoBot(cfg, name, func(cb *CoBotConfig) { + cb.Trigger, cb.TriggerExplicit = mode, true + }) + return nil + }, + Show: func(cfg Config) string { + if cb := coBotOf(cfg, name); cb.TriggerExplicit { + return string(cb.Trigger) + } + return "" + }, + }, + "cobot-" + name + "-cmd": { + Doc: "the comment that triggers " + name + " (empty means crq never posts one)", + Env: env + "_CMD", + AltEnv: legacyCommand, + Apply: func(cfg *Config, v string) error { + command := strings.TrimSpace(v) + updateCoBot(cfg, name, func(cb *CoBotConfig) { cb.Command = command }) + return nil + }, + Show: func(cfg Config) string { return coBotOf(cfg, name).Command }, + }, + "cobot-" + name + "-grace": { + Doc: "how long a selfheal trigger waits for " + name + " to show up on its own", + Env: env + "_GRACE", + Apply: func(cfg *Config, v string) error { + d, err := parseFleetDuration(v) + if err != nil { + return err + } + updateCoBot(cfg, name, func(cb *CoBotConfig) { cb.SelfHealGrace = d }) + return nil + }, + Show: func(cfg Config) string { return coBotOf(cfg, name).SelfHealGrace.String() }, + }, + } +} + +// coBotRequiredEnvs is every per-bot required key. Required-ness is not a fleet +// setting of its own (required-bots owns the list), but the environment still +// lets a host answer it per bot — so those variables belong to the settings that +// carry the answer, not to none of them. +func coBotRequiredEnvs() []string { + out := make([]string, 0, 3) + for _, co := range dialect.KnownCoReviewers() { + out = append(out, "CRQ_COBOT_"+strings.ToUpper(co.Name)+"_REQUIRED") + } + return out +} + +// envNames is every variable this host may be feeding the setting from, canonical +// name first. +func (s fleetSetting) envNames() []string { + return append([]string{s.Env}, s.AltEnv...) +} + +// explicitEnv names the variables this host actually set for one fleet setting, +// canonical name first. Empty for a setting the host leaves to the fleet — and +// for the zero fleetSetting an unknown key resolves to, whose empty Env no +// environment can hold. +func explicitEnv(cfg Config, setting fleetSetting) []string { + var out []string + for _, name := range setting.envNames() { + if name == "" { + continue + } + if cfg.ExplicitFleetEnv[name] { + out = append(out, name) + } + } + return out +} + +// coBotOf is how this host currently drives a co-reviewer: its enabled entry +// when it has one, otherwise what its environment resolved for a bot the fleet +// leaves disabled — which is still the configuration a repository override +// would pick it up with. +func coBotOf(cfg Config, name string) CoBotConfig { + for _, cb := range cfg.CoBots { + if strings.EqualFold(cb.Name, name) { + return cb + } + } + cb, _ := cfg.knownCoBot(name) + return cb +} + +// updateCoBot changes one co-reviewer wherever its configuration is held: the +// enabled set crq drives, and the registry-wide set a per-repo override draws +// from for a bot the fleet does not enable. Both are copied first — the caller's +// Config shares these slices, and a setting that fails to parse must leave it +// untouched. +func updateCoBot(cfg *Config, name string, mutate func(*CoBotConfig)) { + cfg.CoBots = updateCoBotIn(cfg.CoBots, name, mutate) + // The registry-wide set is what a bot this host never resolved is enabled + // from — by the fleet's own cobots setting, or by a repository override — so + // it has to hold the policy even when there is no entry to change yet. + // Without this the fleet's answer would be silently dropped, which is the + // failure the shared registry exists to prevent. + if !hasCoBot(cfg.KnownCoBots, name) { + if co, ok := dialect.CoReviewerByName(name); ok { + cfg.KnownCoBots = append(append([]CoBotConfig(nil), cfg.KnownCoBots...), defaultCoBot(co, false)) + } + } + cfg.KnownCoBots = updateCoBotIn(cfg.KnownCoBots, name, mutate) +} + +func hasCoBot(bots []CoBotConfig, name string) bool { + for _, cb := range bots { + if strings.EqualFold(cb.Name, name) { + return true + } + } + return false +} + +func updateCoBotIn(bots []CoBotConfig, name string, mutate func(*CoBotConfig)) []CoBotConfig { + out := append([]CoBotConfig(nil), bots...) + for i, cb := range out { + if !strings.EqualFold(cb.Name, name) { + continue + } + mutate(&cb) + // Re-derive what the change implies, exactly as the environment parse + // does: an implicit trigger follows the registry default for how the bot + // is required, and no command means crq can never post one. + cb = reconcileTrigger(cb) + if cb.Command == "" { + cb.Trigger = engine.TriggerNever + } + out[i] = cb + } + return out +} + +// fleetCoBotNames resolves a recorded co-reviewer list to registry names, +// refusing one this binary does not know rather than skipping it: a silently +// dropped bot looks exactly like one that is simply never triggered. +func fleetCoBotNames(value string) ([]string, error) { + var names, unknown []string + for _, item := range splitList(value) { + co, ok := dialect.CoReviewerByName(item) + if !ok { + unknown = append(unknown, item) + continue + } + names = append(names, co.Name) + } + if len(unknown) > 0 { + known := make([]string, 0, 3) + for _, co := range dialect.KnownCoReviewers() { + known = append(known, co.Name) + } + return nil, fmt.Errorf("unknown co-reviewer %s (known: %s)", + strings.Join(unknown, ", "), strings.Join(known, ", ")) + } + return names, nil +} + +// feedbackBotsKey is the one reviewer-view input that does not reshape who +// reviews: it widens whose findings are read, which decides whether a head +// comes back as `fix` or as clean. Two hosts answering it differently is the +// same divergence as one excluding a repository the other reviews — one queue +// driver reports findings the next considers absent — so it is fleet policy, +// but it never requeues a round. +const feedbackBotsKey = "feedback-bots" + +// requiredBotsKey and coBotsKey are the two membership settings, named because +// their order relative to each other decides what they resolve to. See +// fleetApplyOrder. +const ( + requiredBotsKey = "required-bots" + coBotsKey = "cobots" +) + +// isReviewerFleetKey reports whether a setting reshapes who reviews, or how a +// co-reviewer is driven. Those are the ones whose derived views have to be +// rebuilt, and whose changes existing rounds may have to be reconciled against. +func isReviewerFleetKey(key string) bool { + return isReviewerMembershipFleetKey(key) || strings.HasPrefix(key, "cobot-") +} + +// isReviewerMembershipFleetKey reports whether a setting decides WHO reviews: +// the reviewer set itself, or which of them a round waits for. The per-bot keys +// isReviewerFleetKey also covers only say how crq asks a co-reviewer that is +// already configured — its command, its trigger mode, its self-heal grace — and +// none of them adds or removes a reviewer. +// +// The distinction matters when the fleet adopts a key, see fleetChange.baseline. +func isReviewerMembershipFleetKey(key string) bool { + return key == requiredBotsKey || key == coBotsKey +} + +// FleetKeys lists every setting the fleet owns, in a stable order. +func FleetKeys() []string { + settings := fleetSettings() + keys := make([]string, 0, len(settings)) + for key := range settings { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +// parseFleetDuration rejects what a Go duration cannot express, and refuses a +// negative one: every setting here is a window, and a negative window is a +// setting that would make crq act immediately, for ever. +func parseFleetDuration(v string) (time.Duration, error) { + d, err := time.ParseDuration(strings.TrimSpace(v)) + if err != nil { + return 0, fmt.Errorf("not a duration (try 90s, 15m, 2h): %w", err) + } + if d < 0 { + return 0, fmt.Errorf("duration must not be negative, got %s", d) + } + return d, nil +} + +// parseFleetBool reads the spellings the environment already accepts, so a +// seeded value and a hand-typed one mean the same thing. +func parseFleetBool(v string) (bool, error) { + switch value := strings.ToLower(strings.TrimSpace(v)); value { + case "on": + return true, nil + case "off": + return false, nil + default: + b, err := strconv.ParseBool(value) + if err != nil { + return false, fmt.Errorf("not a switch (try 1 or 0): %w", err) + } + return b, nil + } +} + +func fleetBool(on bool) string { + if on { + return "1" + } + return "0" +} + +// ValidateFleetSetting reports whether key is a fleet setting and value is one +// it can hold. Set fails on a bad value rather than writing it: the state ref +// is shared, so a value only one host can parse is a value that breaks the +// fleet from the inside. +func ValidateFleetSetting(key, value string) error { + setting, ok := fleetSettings()[key] + if !ok { + return fmt.Errorf("unknown setting %q (try one of: %s)", key, strings.Join(FleetKeys(), ", ")) + } + probe := Config{} + return setting.Apply(&probe, value) +} + +// fleetOwners refuses a scope entry that is not an owner login — a repository +// slug typed here being the mistake to catch. +// +// autoReviewPass hands every scope target to EachOpenPR as a user or +// organisation name, which resolves it against /users/; one that is +// neither fails the whole pass. Recorded for the fleet, a single typo therefore +// stops scanning on every host at once, which is exactly the reach that makes +// validating it here worth more than validating a per-host variable. +func fleetOwners(value string) ([]string, error) { + owners := splitList(value) + for _, owner := range owners { + if !validOwnerLogin(owner) { + return nil, fmt.Errorf("scope must be owner or organisation logins, got %q", owner) + } + } + return owners, nil +} + +func fleetRepoSet(value string) (map[string]bool, error) { + repos := repoSet(value) + for repo := range repos { + if !validRepoSlug(repo) { + return nil, fmt.Errorf("repo must be owner/name, got %q", repo) + } + } + return repos, nil +} + +// applyFleet overlays the fleet's recorded policy onto a host's configuration. +// +// Recorded wins. The environment is what a host uses until the fleet has an +// opinion — which is what makes adopting this safe on a machine at a time, and +// what `crq config seed` writes from. +// +// A value this binary cannot parse is LEFT to the host rather than applied +// half-way: a newer crq may have widened what it accepts, and acting on a +// partially-read policy is worse than acting on the one already in hand. The +// disagreement surfaces in `crq doctor`, not silently here. +func applyFleet(cfg Config, fleet map[string]string, warn func(string)) Config { + settings := fleetSettings() + for _, key := range fleetApplyOrder(fleet) { + setting, ok := settings[key] + if !ok { + if warn != nil { + warn(fmt.Sprintf("fleet setting %q is not one this crq understands; ignoring it", key)) + } + continue + } + candidate := cfg + if err := setting.Apply(&candidate, fleet[key]); err != nil { + if warn != nil { + warn(fmt.Sprintf("fleet setting %q: %v; keeping this host's value", key, err)) + } + continue + } + cfg = candidate + } + // The reviewer settings are the inputs to the derived reviewer views. + // Applying one as a scalar without rebuilding those views can leave a + // required co-reviewer disabled, carrying its optional trigger mode, or + // still driven by the command this host was started with. + if touchesReviewers(fleet) { + // A primary that is also a registry bot is triggered as the primary; + // asking it twice is the bug, and a fleet trigger for it must not undo + // the silencing LoadConfig applied. + cfg.CoBots = silenceTrigger(cfg.CoBots, cfg.Bot) + enabled := make([]string, 0, len(cfg.CoBots)) + for _, cb := range cfg.CoBots { + enabled = append(enabled, cb.Login) + } + cfg = cfg.ForRepo(RepoReviewers{ + CoBots: enabled, + SetCoBots: true, + Required: cfg.RequiredBots, + SetRequired: true, + }) + } + return cfg +} + +// fleetApplyOrder is the order recorded settings are applied in: alphabetical, +// except that required-bots comes first. +// +// The two membership settings are interdependent — `cobots` resolves through +// ForRepo, where a required bot is enabled whatever the list says. Alphabetical +// order applies it BEFORE required-bots, so it reads THIS HOST's required set: +// a fleet that records `required-bots=coderabbitai[bot]` with `cobots=""` has +// the host's locally required co-reviewer put straight back, and the rebuild +// below preserves the contaminated set — leaving hosts surfacing and triggering +// a bot the fleet disabled, from local environment the fleet is meant to +// override. Resolving the required list first gives both keys one baseline. +func fleetApplyOrder(fleet map[string]string) []string { + keys := make([]string, 0, len(fleet)) + if _, ok := fleet[requiredBotsKey]; ok { + keys = append(keys, requiredBotsKey) + } + for _, key := range sortedKeys(fleet) { + if key != requiredBotsKey { + keys = append(keys, key) + } + } + return keys +} + +// touchesReviewers reports whether the recorded policy holds anything the +// derived reviewer views are built from. feedback-bots is one of those inputs +// without reshaping who reviews: dropping it has to return the surfaced set to +// the derived one, which only the rebuild can compute. +func touchesReviewers(fleet map[string]string) bool { + for key := range fleet { + if isReviewerFleetKey(key) || key == feedbackBotsKey { + return true + } + } + return false +} + +func sortedKeys(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + +func sortedSetKeys(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} diff --git a/internal/crq/fleetconfig_test.go b/internal/crq/fleetconfig_test.go new file mode 100644 index 00000000..0cb186ec --- /dev/null +++ b/internal/crq/fleetconfig_test.go @@ -0,0 +1,1261 @@ +package crq + +import ( + "context" + "net/http" + "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" +) + +// One setting, one place. A host that carries its own answer for something the +// fleet has decided is how a repository ends up excluded on one machine and +// reviewed by another, with nothing to say so. +func TestFleetPolicyOverridesTheHostsOwn(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/local": true} + cfg.MinInterval = time.Minute + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, "repos", "owner/fleet-a,owner/fleet-b"); err != nil { + t.Fatal(err) + } + if err := svc.SetFleetConfig(ctx, "min-interval", "5m"); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + got := svc.fleetCfg(st) + if !got.AllowRepos["owner/fleet-a"] || !got.AllowRepos["owner/fleet-b"] { + t.Errorf("repos = %v, want the fleet's list", got.AllowRepos) + } + if got.AllowRepos["owner/local"] { + t.Error("the host's own repository list survived the fleet's") + } + if got.MinInterval != 5*time.Minute { + t.Errorf("min-interval = %s, want the fleet's 5m", got.MinInterval) + } + + // A setting the fleet has no opinion on stays this host's answer. + if got.SettleWindow != cfg.SettleWindow { + t.Errorf("settle = %s, want the host's %s when the fleet is silent", got.SettleWindow, cfg.SettleWindow) + } + + // And unsetting hands it back. + if dropped, err := svc.UnsetFleetConfig(ctx, "min-interval"); err != nil || !dropped { + t.Fatalf("unset = %v %v", dropped, err) + } + st, _, _ = store.Load(ctx) + if svc.fleetCfg(st).MinInterval != time.Minute { + t.Error("unsetting a fleet setting did not return the host's own value") + } +} + +// A value only some binaries can read would break the fleet from the inside, +// so it is refused where it is set — and if one arrives anyway, from a newer +// crq, the host keeps what it has rather than acting on half a policy. +func TestFleetRefusesWhatItCannotRead(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.MinInterval = 90 * time.Second + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, "min-interval", "later"); err == nil { + t.Error("a duration that does not parse was accepted") + } + if err := svc.SetFleetConfig(ctx, "min-interval", "-5m"); err == nil { + t.Error("a negative window was accepted") + } + if err := svc.SetFleetConfig(ctx, "nonsense", "1"); err == nil { + t.Error("an unknown setting was accepted") + } + for _, key := range []string{"repos", "exclude"} { + // A slug GitHub can never return is a rule covering nothing, whether it + // is missing the slash or holds a character no repository name can. + for _, slug := range []string{"owner-repo", "owner/re po", "owner/re\tpo", "owner/repo?"} { + if err := svc.SetFleetConfig(ctx, key, slug); err == nil { + t.Errorf("%s accepted the malformed repository slug %q", key, slug) + } + } + } + // A scope entry is an owner login, not a slug: autoreview hands each one to + // EachOpenPR as a user or organisation name, so a repository typed here — or + // anything else GitHub cannot resolve — fails every pass, on every host at + // once, because the value is fleet-wide. + // A login may not begin or end with a hyphen, hold two in a row, run past + // GitHub's 39-character limit, or hold underscores or dots — those are + // repository-name characters, and accepting any of them here records a scope + // no /users/ lookup can ever resolve. + for _, owner := range []string{ + "owner/repo", "own er", "owner?", "..", "-team", "team-", "a_team", "a.team", + "a--team", strings.Repeat("a", maxOwnerLogin+1), + } { + if err := svc.SetFleetConfig(ctx, "scope", owner); err == nil { + t.Errorf("scope accepted the malformed owner %q", owner) + } + } + if err := svc.SetFleetConfig(ctx, "scope", "acme-2,Some-Org"); err != nil { + t.Errorf("a pair of ordinary owner logins was rejected: %v", err) + } + if err := svc.SetFleetConfig(ctx, "required-bots", ""); err == nil { + t.Error("an empty required reviewer set was accepted") + } + + // Planted directly, as a newer binary would leave it. + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("min-interval", "a fortnight") + st.SetFleetValue("from-the-future", "whatever") + return nil + }); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if got := svc.fleetCfg(st).MinInterval; got != 90*time.Second { + t.Errorf("min-interval = %s, want this host's 90s kept when the fleet's is unreadable", got) + } +} + +// Seeding is how an existing setup adopts this without retyping it, and it must +// not let a second machine overwrite the first one's answer. +func TestSeedingDoesNotOverwriteWhatTheFleetAlreadySays(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/from-this-host": true} + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, "repos", "owner/decided-already"); err != nil { + t.Fatal(err) + } + seeded, err := svc.SeedFleetConfig(ctx) + if err != nil { + t.Fatal(err) + } + for _, key := range seeded { + if key == "repos" { + t.Error("seeding overwrote a setting the fleet had already recorded") + } + } + st, _, _ := store.Load(ctx) + if v, _ := st.FleetValue("repos"); v != "owner/decided-already" { + t.Errorf("repos = %q, want the fleet's existing answer", v) + } + // Everything else it had no answer for is now recorded. + if v, ok := st.FleetValue("settle"); !ok || v == "" { + t.Errorf("settle = %q %v, want this host's value seeded", v, ok) + } +} + +func TestSeedingValidatesEveryMissingValueBeforeWriting(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"not-a-repository": true} + store := NewMemoryStore(cfg) + + seeded, err := NewService(cfg, newFakeGitHub(), store, nil).SeedFleetConfig(ctx) + if err == nil || !strings.Contains(err.Error(), "cannot seed repos") { + t.Fatalf("seed = %v, %v; want the malformed host repository rejected", seeded, err) + } + st, _, loadErr := store.Load(ctx) + if loadErr != nil { + t.Fatal(loadErr) + } + if len(st.FleetConfig) != 0 { + t.Fatalf("failed seed partially wrote fleet policy: %v", st.FleetConfig) + } +} + +func TestFleetRequiredBotsRebuildsDerivedReviewers(t *testing.T) { + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + st := DefaultState(cfg) + st.SetFleetValue("required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin) + + got := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil).fleetCfg(st) + if !containsBot(got.RequiredBots, dialect.CodexBotLogin) { + t.Fatalf("RequiredBots = %v, want fleet-required codex", got.RequiredBots) + } + if len(got.CoBots) != 1 || !got.CoBots[0].Required || got.CoBots[0].Trigger != "always" { + t.Fatalf("CoBots = %+v, want required codex with its required trigger", got.CoBots) + } +} + +// Which co-reviewers run, how crq drives them, and whether an account-blocked +// round degrades to them are decisions, not machine facts. Two hosts answering +// them differently is the same divergence as one excluding a repository the +// other reviews: both behave correctly, and the PRs disagree. +func TestFleetOwnsTheCoReviewerPolicy(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + "CRQ_COBOT_BUGBOT_CMD": "bugbot run", + "CRQ_COBOT_BUGBOT_GRACE": "10m", + "CRQ_COBOT_CODEX_TRIGGER": "always", + "CRQ_RL_CO_DEGRADE": "0", + }) + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + for _, set := range []struct{ key, value string }{ + {"cobots", "bugbot"}, + {"cobot-bugbot-trigger", "always"}, + {"cobot-bugbot-cmd", "bugbot run now"}, + {"cobot-bugbot-grace", "30m"}, + {"rate-limit-co-degrade", "1"}, + } { + if err := svc.SetFleetConfig(ctx, set.key, set.value); err != nil { + t.Fatalf("set %s: %v", set.key, err) + } + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + got := svc.fleetCfg(st) + + if len(got.CoBots) != 1 || got.CoBots[0].Name != "bugbot" { + t.Fatalf("CoBots = %+v, want only the fleet's bugbot", got.CoBots) + } + bugbot := got.CoBots[0] + if bugbot.Trigger != engine.TriggerAlways || bugbot.Command != "bugbot run now" || bugbot.SelfHealGrace != 30*time.Minute { + t.Errorf("bugbot = %+v, want the fleet's trigger, command and grace", bugbot) + } + if !got.RateLimitCoDegrade { + t.Error("the fleet turned co-reviewer degradation on and this host stayed off") + } + // The derived views have to follow, or which co-reviewer runs depends on + // which view crq happens to ask. + var seen bool + for _, r := range got.Reviewers { + if !sameBot(r.Login, bugbot.Login) { + continue + } + seen = true + if r.Trigger != engine.TriggerAlways || r.Command != "bugbot run now" { + t.Errorf("reviewer %+v disagrees with the fleet's co-reviewer policy", r) + } + } + if !seen { + t.Errorf("Reviewers = %+v, want the fleet's co-reviewer among them", got.Reviewers) + } +} + +// The two membership settings are read together or not at all. Applied in +// alphabetical order, `cobots` resolved against the host's required list — which +// re-enabled a co-reviewer the fleet had just disabled, and the rebuild kept it, +// leaving the policy depending on local environment again. +func TestFleetCoBotsResolveAgainstTheFleetsRequiredBots(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_COBOT_CODEX_REQUIRED": "1", + }) + if len(cfg.CoBots) != 1 || cfg.CoBots[0].Name != "codex" { + t.Fatalf("this host must start with codex required: %+v", cfg.CoBots) + } + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, "cobots", ""); err != nil { + t.Fatal(err) + } + if err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot]"); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + got := svc.fleetCfg(st) + if len(got.CoBots) != 0 { + t.Errorf("CoBots = %+v, want none — the fleet disabled every co-reviewer", got.CoBots) + } + if containsBot(got.RequiredBots, dialect.CodexBotLogin) { + t.Errorf("RequiredBots = %v, want only the fleet's", got.RequiredBots) + } +} + +// A value only some hosts can read breaks the fleet from the inside, so the +// co-reviewer settings are refused where they are set like every other one. +func TestFleetRefusesCoReviewerPolicyItCannotRead(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + + for _, bad := range []struct{ key, value string }{ + {"cobots", "codex,nosuchbot"}, + {"cobot-codex-trigger", "sometimes"}, + {"cobot-codex-grace", "-5m"}, + {"rate-limit-co-degrade", "maybe"}, + } { + if err := svc.SetFleetConfig(ctx, bad.key, bad.value); err == nil { + t.Errorf("%s accepted %q", bad.key, bad.value) + } + } + // Explicitly none is a real answer, not a malformed one. + if err := svc.SetFleetConfig(ctx, "cobots", ""); err != nil { + t.Errorf("cobots refused an empty set: %v", err) + } +} + +// Enabling a co-reviewer fleet-wide is a reviewer change like requiring one: +// the heads already reviewed were reviewed without it. +func TestFleetCoBotChangeReopensCompletedRounds(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + repo, pr := "owner/repo", 7 + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + if err := svc.SetFleetConfig(ctx, "cobots", "codex,bugbot"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %+v, want the completed round reopened for the new co-reviewer", round) + } + if !containsBot(round.ForceCoReviewers, dialect.BugbotLogin) { + t.Errorf("ForceCoReviewers = %v, want the newly enabled self-heal bot nudged once", round.ForceCoReviewers) + } +} + +func TestFleetMutationsAreInertInDryRun(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("min-interval", "5m") + return nil + }); err != nil { + t.Fatal(err) + } + cfg.DryRun = true + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, "min-interval", "10m"); err != nil { + t.Fatal(err) + } + if dropped, err := svc.UnsetFleetConfig(ctx, "min-interval"); err != nil || !dropped { + t.Fatalf("dry-run unset = %v, %v; want the report without a write", dropped, err) + } + if seeded, err := svc.SeedFleetConfig(ctx); err != nil || len(seeded) == 0 { + t.Fatalf("dry-run seed = %v, %v; want missing keys reported", seeded, err) + } + st, _, _ := store.Load(ctx) + if value, _ := st.FleetValue("min-interval"); value != "5m" || len(st.FleetConfig) != 1 { + t.Fatalf("dry run changed fleet state: %v", st.FleetConfig) + } +} + +func TestFleetDryRunStillChecksDriverCompatibility(t *testing.T) { + ctx := context.Background() + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.Leader = &LeaderLease{Owner: "old-daemon", ExpiresAt: now.Add(time.Minute)} + st.NoteWriter("old-daemon", CapsFleetPolicy-1, now) + return nil + }); err != nil { + t.Fatal(err) + } + cfg.DryRun = true + svc := NewService(cfg, newFakeGitHub(), store, nil) + svc.now = func() time.Time { return now } + + err := svc.SetFleetConfig(ctx, "min-interval", "5m") + if err == nil || !strings.Contains(err.Error(), "lack fleet-policy support") { + t.Fatalf("dry-run set with a lagging driver = %v, want a capability refusal", err) + } +} + +func TestFleetScopeChangeInvalidatesAccountQuota(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + blocked := time.Date(2026, 7, 27, 13, 0, 0, 0, time.UTC) + remaining := 3 + if _, err := store.Update(ctx, func(st *State) error { + st.Account.BlockedUntil = &blocked + st.Account.Remaining = &remaining + st.Account.CheckedAt = &blocked + st.Account.CalibAskedAt = &blocked + st.Account.RLCommentID = 42 + st.Account.RLCommentUpdated = &blocked + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "scope", "other-org"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if st.Account.Scope != "other-org" { + t.Fatalf("account scope = %q, want other-org", st.Account.Scope) + } + if st.Account.BlockedUntil != nil || st.Account.Remaining != nil || st.Account.CheckedAt != nil || + st.Account.CalibAskedAt != nil || st.Account.RLCommentID != 0 || st.Account.RLCommentUpdated != nil { + t.Fatalf("old account quota survived the scope change: %+v", st.Account) + } +} + +func TestEquivalentFleetScopeKeepsAccountQuota(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Scope = []string{"Acme", "Foo"} + store := NewMemoryStore(cfg) + blocked := time.Date(2026, 7, 27, 13, 0, 0, 0, time.UTC) + if _, err := store.Update(ctx, func(st *State) error { + st.Account.BlockedUntil = &blocked + st.Account.Scope = "Acme,Foo" + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "scope", "foo,acme"); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.Account.BlockedUntil == nil || !st.Account.BlockedUntil.Equal(blocked) { + t.Fatalf("equivalent scope cleared the account block: %+v", st.Account) + } +} + +func TestFleetInflightTimeoutOverridesTheHost(t *testing.T) { + cfg := firingConfig() + cfg.InflightTimeout = 15 * time.Minute + st := DefaultState(cfg) + st.SetFleetValue("inflight-timeout", "45m") + + got := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil).fleetCfg(st) + if got.InflightTimeout != 45*time.Minute { + t.Fatalf("inflight timeout = %s, want fleet 45m", got.InflightTimeout) + } +} + +func TestFleetExcludeRetiresQueuedRounds(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{ + Repo: "owner/repo", PR: 7, Head: "abcdef123", + Phase: PhaseQueued, EnqueuedAt: time.Now().UTC(), + }) + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "exclude", "owner/repo"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if round := st.Round("owner/repo", 7); round != nil { + t.Fatalf("excluded round remains fire-eligible: %+v", round) + } + if len(st.Archive) != 1 || st.Archive[0].Phase != PhaseAbandoned { + t.Fatalf("excluded round was not archived as abandoned: %+v", st.Archive) + } +} + +// The slot outlives the round that took it: a hold left behind for a metered +// command whose round was superseded by the push it invited belongs to that +// command. Excluding the repository retires the QUEUED replacement at the same +// key, and dropping the hold with it would let a second metered review fire +// while the first command is still unanswered. +func TestFleetExcludeLeavesAnOrphanedFireSlotHold(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + now := time.Now().UTC() + until := now.Add(20 * time.Minute) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{ + Repo: "owner/repo", PR: 7, Head: "beef456", + Phase: PhaseQueued, EnqueuedAt: now, + }) + st.FireSlot = &FireSlot{Key: QueueKey("owner/repo", 7), Token: "gone", Since: now} + st.HoldSlotUntil(until) + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "exclude", "owner/repo"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if round := st.Round("owner/repo", 7); round != nil { + t.Fatalf("excluded round remains fire-eligible: %+v", round) + } + if !st.SlotHeld(now) { + t.Fatalf("exclusion released the unanswered command's hold: %+v", st.FireSlot) + } +} + +// A reservation is a fire's commit point, but the review command reaches GitHub +// after it. Excluding the repository in that window cannot take the post back: +// retiring the round would only destroy the record of the quota the account is +// about to be charged for, on a repository crq is no longer meant to touch. +func TestFleetExcludeRefusesToRaceAClaimedTriggerPost(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/repo", PR: 7, Head: "abcdef123", Phase: PhaseReserved, Token: "tok"}) + return nil + }); err != nil { + t.Fatal(err) + } + + svc := NewService(cfg, newFakeGitHub(), store, nil) + err := svc.SetFleetConfig(ctx, "exclude", "owner/repo") + if err == nil || !strings.Contains(err.Error(), "already being posted") { + t.Fatalf("exclude during a claimed trigger post = %v, want a refusal", err) + } + st, _, _ := store.Load(ctx) + if value, ok := st.FleetValue("exclude"); ok { + t.Errorf("exclude = %q, want the refused change not written", value) + } + if round := st.Round("owner/repo", 7); round == nil || round.Phase != PhaseReserved { + t.Fatalf("round = %+v, want the reserved round left alone", round) + } + + // A co-reviewer claim is the same promise, and both clear once the post is + // recorded — then the exclusion goes through. + if _, err := store.Update(ctx, func(st *State) error { + r := st.Round("owner/repo", 7) + r.Phase = PhaseQueued + st.PutRound(*r) + return nil + }); err != nil { + t.Fatal(err) + } + if err := svc.SetFleetConfig(ctx, "exclude", "owner/repo"); err != nil { + t.Fatalf("exclude after the post finished = %v, want it recorded", err) + } +} + +// Whose findings crq reads decides whether a head comes back as `fix` or as +// clean, which makes it fleet policy: one queue driver reporting findings the +// next considers absent is the same divergence as one host excluding a +// repository the other reviews. +func TestFleetOwnsTheFeedbackBots(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + "CRQ_FEEDBACK_BOTS": "coderabbitai[bot]", + }) + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.SetFleetConfig(ctx, feedbackBotsKey, "coderabbitai[bot],"+dialect.CodexBotLogin); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + got := svc.fleetCfg(st) + if !containsBot(got.FeedbackBots, dialect.CodexBotLogin) { + t.Errorf("FeedbackBots = %v, want the fleet's list to win over this host's", got.FeedbackBots) + } + // And `crq doctor` names the variable still set locally, or the host looks + // healthy while it reads a different set. + diverged, err := svc.FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(strings.Join(diverged, "\n"), "CRQ_FEEDBACK_BOTS") { + t.Errorf("divergence = %v, want the overridden host variable named", diverged) + } + + // Recording the empty value is the fleet saying "derive it", which has to + // recompute the surfaced set rather than leave this host's explicit one. + if err := svc.SetFleetConfig(ctx, feedbackBotsKey, ""); err != nil { + t.Fatal(err) + } + st, _, _ = store.Load(ctx) + got = svc.fleetCfg(st) + if got.FeedbackBotsExplicit || !containsBot(got.FeedbackBots, dialect.CodexBotLogin) { + t.Fatalf("FeedbackBots = %v (explicit %v), want the derived set including the enabled co-reviewer", + got.FeedbackBots, got.FeedbackBotsExplicit) + } +} + +// Seeding writes THIS host's answers, so comparing the reviewer policy it +// records against the policy this host was already using can only ever say +// "nothing changed" — while the host that completed the head may have been +// running an entirely different set. That divergence is what seeding exists to +// end, so a seeded reviewer key reconciles as if the fleet had no reviewers. +func TestSeedingReviewerPolicyReopensCompletedRounds(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_REPOS": "", + "CRQ_EXCLUDE": "", + "CRQ_COBOTS": "codex,bugbot", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + "CRQ_COBOT_BUGBOT_CMD": "bugbot run", + "CRQ_COBOT_BUGBOT_TRIGGER": "selfheal", + }) + repo, pr := "owner/repo", 7 + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + + if _, err := NewService(cfg, gh, store, nil).SeedFleetConfig(ctx); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %+v, want the completed round reopened by the seed", round) + } + if !containsBot(round.ForceCoReviewers, dialect.BugbotLogin) { + t.Errorf("ForceCoReviewers = %v, want the seeded self-heal bot nudged once", round.ForceCoReviewers) + } +} + +// The first `crq config set` of a key is the same adoption a seed performs, and +// the pre-change baseline it is reconciled against comes from THIS host — so +// recording the value this machine was already using looks like no change at +// all, while the host that completed the head may have been running another set +// entirely. +func TestFirstFleetReviewerValueReopensCompletedRounds(t *testing.T) { + ctx := context.Background() + required := "coderabbitai[bot]," + dialect.CodexBotLogin + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": required, + }) + repo, pr := "owner/repo", 7 + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + + // The same value this host already had: only its absence from the fleet + // makes it a change, and that is the change that has to be reconciled. + if err := NewService(cfg, gh, store, nil).SetFleetConfig(ctx, "required-bots", required); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if round := st.Round(repo, pr); round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %+v, want the completed round reopened by the adoption", round) + } +} + +// Adoption erases the reviewer baseline because the fleet may never have agreed +// on WHO reviews. A co-reviewer's self-heal grace is not that question: it is +// how long an already-configured bot is given, every host was driving the same +// set before and after, and reconciliation compares membership. Erasing the +// baseline for it reopened every completed round in the fleet and forced a +// self-heal trigger post on every open PR, for a timing value. +func TestFirstFleetCoBotTimingValueLeavesCompletedRoundsAlone(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex,bugbot", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + "CRQ_COBOT_BUGBOT_CMD": "bugbot run", + "CRQ_COBOT_BUGBOT_TRIGGER": "selfheal", + }) + repo, pr := "owner/repo", 7 + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + // Who reviews is already the fleet's answer; only the timing is not. + st.SetFleetValue("cobots", "codex,bugbot") + st.SetFleetValue("required-bots", "coderabbitai[bot]") + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, gh, store, nil).SetFleetConfig(ctx, "cobot-bugbot-grace", "10m"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + round := st.Round(repo, pr) + if round == nil || round.Phase != PhaseCompleted { + t.Fatalf("round = %+v, want the completed round left alone by a timing-only adoption", round) + } + if len(round.ForceCoReviewers) != 0 { + t.Errorf("ForceCoReviewers = %v, want no trigger forced by a timing-only adoption", round.ForceCoReviewers) + } +} + +// Adopting an exclusion this host already applied is a change for every host +// that did not, so it still has to pass the claimed-trigger refusal. +func TestFirstFleetExcludeStillRefusesAClaimedTriggerPost(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.ExcludeRepos = map[string]bool{"owner/repo": true} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/repo", PR: 7, Head: "abcdef123", Phase: PhaseReserved, Token: "tok"}) + return nil + }); err != nil { + t.Fatal(err) + } + + err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "exclude", "owner/repo") + if err == nil || !strings.Contains(err.Error(), "already being posted") { + t.Fatalf("adopting an exclusion during a claimed trigger post = %v, want a refusal", err) + } +} + +// The recorded quota belongs to whichever account the fleet was scanning, which +// this host's own scope says nothing about — so adopting the scope is judged +// against the quota's own recorded one. +func TestFirstFleetScopeDropsQuotaFromAnotherAccount(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.Scope = []string{"acme"} + store := NewMemoryStore(cfg) + blocked := time.Date(2026, 7, 27, 13, 0, 0, 0, time.UTC) + if _, err := store.Update(ctx, func(st *State) error { + st.Account.BlockedUntil = &blocked + st.Account.Scope = "other-org" + return nil + }); err != nil { + t.Fatal(err) + } + + if err := NewService(cfg, newFakeGitHub(), store, nil).SetFleetConfig(ctx, "scope", "acme"); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if st.Account.BlockedUntil != nil || st.Account.Scope != "acme" { + t.Fatalf("account = %+v, want another account's block dropped", st.Account) + } +} + +// Dropping a recorded key this binary cannot read is the remedy doctor names, +// but it is not this binary's to make while the crq that wrote it is driving the +// queue: the removal may need cleanup only that version knows how to do. +func TestUnsettingAnUnknownKeyWaitsForTheNewerDriver(t *testing.T) { + ctx := context.Background() + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("some-future-knob", "7") + st.Leader = &LeaderLease{Owner: "new-daemon", ExpiresAt: now.Add(time.Minute)} + st.NoteWriter("new-daemon", WriterCaps+1, now) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + svc.now = func() time.Time { return now } + + if _, err := svc.UnsetFleetConfig(ctx, "some-future-knob"); err == nil || !strings.Contains(err.Error(), "newer queue drivers") { + t.Fatalf("unset of an uninterpretable key = %v, want a refusal naming the newer driver", err) + } + // A setting this binary does understand is still its own to drop: it can + // reconcile that removal itself. + if err := svc.SetFleetConfig(ctx, "min-interval", "5m"); err != nil { + t.Fatal(err) + } + if dropped, err := svc.UnsetFleetConfig(ctx, "min-interval"); err != nil || !dropped { + t.Fatalf("unset of a known key = %v %v, want it dropped", dropped, err) + } +} + +func TestFleetDivergenceIncludesConfigFileValues(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.MinInterval = time.Minute + cfg.ExplicitFleetEnv = map[string]bool{"CRQ_MIN_INTERVAL": true} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("min-interval", "5m") + return nil + }); err != nil { + t.Fatal(err) + } + got, err := NewService(cfg, newFakeGitHub(), store, nil).FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || !strings.Contains(got[0], "CRQ_MIN_INTERVAL") { + t.Fatalf("divergence = %v, want the explicitly loaded config-file value", got) + } +} + +func TestFleetDivergenceIncludesAnExplicitlyEmptyHostValue(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.ExcludeRepos = map[string]bool{} + cfg.ExplicitFleetEnv = map[string]bool{"CRQ_EXCLUDE": true} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("exclude", "owner/repo") + return nil + }); err != nil { + t.Fatal(err) + } + got, err := NewService(cfg, newFakeGitHub(), store, nil).FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || !strings.Contains(got[0], `CRQ_EXCLUDE is set to ""`) { + t.Fatalf("divergence = %v, want the explicitly empty host value", got) + } +} + +// A host copy that agrees with the fleet is the same variable waiting to be +// fallen back on, and `crq config unset` is what makes it one: the setting goes +// back to whatever each host says, and by then there is no recorded value left +// for this report to compare against. A setting the fleet does not record is not +// that — it is simply this host's to answer, and saying so would name every +// variable on a machine that has yet to adopt anything. +func TestFleetDivergenceNamesAHostCopyThatAgrees(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.MinInterval = 5 * time.Minute + cfg.ExplicitFleetEnv = map[string]bool{"CRQ_MIN_INTERVAL": true} + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + got, err := svc.FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(got) != 0 { + t.Fatalf("divergence = %v, want nothing reported for a setting the fleet leaves to this host", got) + } + + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("min-interval", "5m") + return nil + }); err != nil { + t.Fatal(err) + } + got, err = svc.FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || !strings.Contains(got[0], "CRQ_MIN_INTERVAL") { + t.Fatalf("divergence = %v, want the matching host copy named", got) + } +} + +// A policy a host feeds from a legacy alias or a per-bot key diverges exactly as +// the canonical variable does — and the remedy names the variable that is +// actually there, since that is the one still waiting to be fallen back on. +func TestFleetDivergenceNamesTheVariableTheHostSet(t *testing.T) { + for _, tc := range []struct { + name string + key string + value string + env string + host func(*Config) + }{ + { + name: "legacy alias", + key: "rate-limit-co-degrade", + value: "1", + env: "CRQ_RL_CODEX_DEGRADE", + host: func(cfg *Config) { cfg.RateLimitCoDegrade = false }, + }, + { + name: "per-bot required key", + key: "required-bots", + value: "coderabbitai[bot]", + env: "CRQ_COBOT_BUGBOT_REQUIRED", + host: func(cfg *Config) { + cfg.RequiredBots = []string{"coderabbitai[bot]", dialect.BugbotLogin} + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + tc.host(&cfg) + cfg.ExplicitFleetEnv = map[string]bool{tc.env: true} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue(tc.key, tc.value) + return nil + }); err != nil { + t.Fatal(err) + } + got, err := NewService(cfg, newFakeGitHub(), store, nil).FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || !strings.Contains(got[0], tc.env) { + t.Fatalf("divergence = %v, want it to name %s", got, tc.env) + } + }) + } +} + +func TestFleetRevisionInvalidatesAStaleDecision(t *testing.T) { + cfg := firingConfig() + st := DefaultState(cfg) + svc := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil) + snapshot := svc.cfgFor(st, "owner/repo") + st.SetFleetValue("min-interval", "5m") + if !overrideChanged(&st, "owner/repo", snapshot) { + t.Error("a fleet policy change did not invalidate the earlier decision") + } +} + +type dashboardConfigStore struct { + StateStore + render StoreConfig + syncs int +} + +func (s *dashboardConfigStore) SyncDashboard(_ context.Context, _ State, cfg StoreConfig) error { + s.render = cfg + s.syncs++ + return nil +} + +func TestFleetMutationsSyncTheDashboard(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.DashboardIssue = 1 + store := &dashboardConfigStore{StateStore: NewMemoryStore(cfg)} + svc := NewService(cfg, newFakeGitHub(), store, &recordingLogger{}) + + if err := svc.SetFleetConfig(ctx, "min-interval", "5m"); err != nil { + t.Fatal(err) + } + if store.syncs != 1 { + t.Fatalf("dashboard syncs after set = %d, want 1", store.syncs) + } + if dropped, err := svc.UnsetFleetConfig(ctx, "min-interval"); err != nil || !dropped { + t.Fatalf("unset = %v, %v", dropped, err) + } + if store.syncs != 2 { + t.Fatalf("dashboard syncs after unset = %d, want 2", store.syncs) + } + if seeded, err := svc.SeedFleetConfig(ctx); err != nil || len(seeded) == 0 { + t.Fatalf("seed = %v, %v", seeded, err) + } + if store.syncs != 3 { + t.Fatalf("dashboard syncs after seed = %d, want 3", store.syncs) + } +} + +func TestDashboardSyncReceivesTheEffectiveFleetReviewers(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + cfg.DashboardIssue = 1 + inner := NewMemoryStore(cfg) + store := &dashboardConfigStore{StateStore: inner} + if _, err := inner.Update(ctx, func(st *State) error { + st.SetFleetValue("required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin) + return nil + }); err != nil { + t.Fatal(err) + } + st, _, err := inner.Load(ctx) + if err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, &recordingLogger{}) + svc.sync(ctx, st) + if !strings.Contains(store.render.CoReviewers, "codex (required") { + t.Fatalf("dashboard co-reviewers = %q, want fleet-required codex", store.render.CoReviewers) + } +} + +func TestFleetSettleWindowShapesNext(t *testing.T) { + cfg := firingConfig() + cfg.SettleWindow = 0 + st := DefaultState(cfg) + st.SetFleetValue("settle", "5m") + effective := NewService(cfg, newFakeGitHub(), NewMemoryStore(cfg), nil).fleetCfg(st) + evidence := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + got := settleUntil(FeedbackReport{LastEvidenceAt: evidence}, effective.SettleWindow) + want := evidence.Add(5 * time.Minute) + if got == nil || !got.Equal(want) { + t.Fatalf("settle until = %v, want %s", got, want) + } +} + +func TestFleetPolicyRefusesALaggingQueueDriver(t *testing.T) { + ctx := context.Background() + now := time.Date(2026, 7, 27, 12, 0, 0, 0, time.UTC) + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.Leader = &LeaderLease{Owner: "old-daemon", ExpiresAt: now.Add(time.Minute)} + st.NoteWriter("old-daemon", CapsFleetPolicy-1, now) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + svc.now = func() time.Time { return now } + if err := svc.SetFleetConfig(ctx, "min-interval", "5m"); err == nil || !strings.Contains(err.Error(), "lack fleet-policy support") { + t.Fatalf("set with a lagging driver = %v, want a capability refusal", err) + } +} + +func TestFleetReviewerChangeReopensCompletedRounds(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + repo, pr := "owner/repo", 7 + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + if err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if round := st.Round(repo, pr); round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %+v, want completed round reopened", round) + } +} + +func TestFleetReviewerChangeDoesNotFailOnAnInaccessibleHistoricalRepository(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: "owner/live", Number: 7}} + gh.listPullErrs["owner/gone"] = &ghapi.APIError{Status: http.StatusForbidden, Body: "resource not accessible"} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/gone", PR: 6, Head: "aaaaaaa11", Phase: PhaseCompleted}) + st.PutRound(Round{Repo: "owner/live", PR: 7, Head: "bbbbbbb22", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + if err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("owner/live", 7); round == nil || round.Phase != PhaseQueued { + t.Fatalf("live round = %+v, want it reopened", round) + } + if round := st.Round("owner/gone", 6); round == nil || !round.ReviewersChanged || round.Phase != PhaseCompleted { + t.Fatalf("inaccessible historical round = %+v, want a marked completed round", round) + } +} + +// A co-reviewer's command, trigger mode or self-heal grace moves no membership, +// so reconciliation compares the same reviewer sets before and after and never +// reads the open-PR map. Building one anyway spent a REST lookup on every +// repository ever recorded in Rounds — and made a purely local timing update +// fail whenever one of those historical lookups was throttled. +func TestFleetCoBotTimingChangeScansNoPullRequests(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex,bugbot", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + gh := newFakeGitHub() + gh.listPullErrs["owner/repo"] = &ghapi.RateLimitError{Kind: "secondary"} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/repo", PR: 7, Head: "bbbbbbb22", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + for _, key := range []string{"cobot-bugbot-grace", "cobot-bugbot-cmd", "cobot-bugbot-trigger"} { + value := map[string]string{ + "cobot-bugbot-grace": "12m", + "cobot-bugbot-cmd": "bugbot run", + "cobot-bugbot-trigger": "selfheal", + }[key] + if err := svc.SetFleetConfig(ctx, key, value); err != nil { + t.Fatalf("set %s: %v", key, err) + } + if _, err := svc.UnsetFleetConfig(ctx, key); err != nil { + t.Fatalf("unset %s: %v", key, err) + } + } +} + +// An idempotent membership command scans nothing either. Re-recording the value +// the fleet already holds, or unsetting a key it never held, reconciles nothing +// — and a fleet whose round history names many repositories would otherwise +// spend an open-PR lookup on each of them and fail outright when one was +// throttled, for a command with nothing to change. +func TestIdempotentFleetReviewerCommandScansNoPullRequests(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + gh := newFakeGitHub() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/repo", PR: 7, Head: "bbbbbbb22", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + if err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot]"); err != nil { + t.Fatalf("recording required-bots: %v", err) + } + // Only now does the lookup fail, so reaching it at all is the failure. + gh.listPullErrs["owner/repo"] = &ghapi.RateLimitError{Kind: "secondary"} + if err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot]"); err != nil { + t.Fatalf("repeating the recorded value: %v", err) + } + if dropped, err := svc.UnsetFleetConfig(ctx, "cobots"); err != nil || dropped { + t.Fatalf("unsetting a key the fleet never recorded = (%v, %v), want (false, nil)", dropped, err) + } +} + +func TestFleetReviewerChangePropagatesGitHubThrottling(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + gh := newFakeGitHub() + gh.listPullErrs["owner/repo"] = &ghapi.RateLimitError{Kind: "secondary"} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.PutRound(Round{Repo: "owner/repo", PR: 7, Head: "bbbbbbb22", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + err := svc.SetFleetConfig(ctx, "required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin) + if !ghapi.IsThrottled(err) { + t.Fatalf("reviewer change error = %v, want GitHub throttling propagated", err) + } +} + +func TestInaccessibleRepoLookupClassification(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "not found", err: ghapi.ErrNotFound, want: true}, + {name: "forbidden", err: &ghapi.APIError{Status: http.StatusForbidden}, want: true}, + {name: "unprocessable", err: &ghapi.APIError{Status: http.StatusUnprocessableEntity}, want: false}, + {name: "primary throttle", err: &ghapi.RateLimitError{Kind: "primary"}, want: false}, + {name: "secondary throttle", err: &ghapi.RateLimitError{Kind: "secondary"}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := inaccessibleRepoLookup(tt.err); got != tt.want { + t.Fatalf("inaccessibleRepoLookup(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +// A key only a newer crq knows is the one divergence this host cannot act on: +// applyFleet ignores it, and reporting only the settings this binary understands +// would have `crq config` and `crq doctor` both call the host fully in step +// while it silently drops part of the shared policy. +func TestFleetConfigReportsSettingsOnlyANewerCrqKnows(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.ExplicitFleetEnv = map[string]bool{} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("some-future-knob", "7") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, newFakeGitHub(), store, nil) + + items, err := svc.FleetConfig(ctx) + if err != nil { + t.Fatal(err) + } + var reported *FleetSetting + for i, item := range items { + if item.Key == "some-future-knob" { + reported = &items[i] + } + } + if reported == nil || !reported.Unknown || reported.Error == "" { + t.Fatalf("crq config must report the recorded key it ignores, got %+v", items) + } + diverged, err := svc.FleetDivergence(ctx) + if err != nil { + t.Fatal(err) + } + if len(diverged) != 1 || !strings.Contains(diverged[0], "some-future-knob") { + t.Fatalf("doctor divergence = %v, want the ignored setting named", diverged) + } + + // And the remedy doctor names has to work: refusing to unset a key this + // binary does not know would leave it unremovable from every older host. + if dropped, err := svc.UnsetFleetConfig(ctx, "some-future-knob"); err != nil || !dropped { + t.Fatalf("unset = %v %v, want the recorded key dropped", dropped, err) + } + if _, err := svc.UnsetFleetConfig(ctx, "never-recorded"); err == nil { + t.Fatal("a key that is neither known nor recorded is still a typo") + } +} diff --git a/internal/crq/init.go b/internal/crq/init.go index ecc7a565..a0a1dab6 100644 --- a/internal/crq/init.go +++ b/internal/crq/init.go @@ -14,6 +14,12 @@ type InitResult struct { DashboardIssue int `json:"dashboard_issue"` CalibrationPR int `json:"calibration_pr,omitempty"` StateRef string `json:"state_ref"` + // Scope is the owner list in force after the fleet's policy is applied, not + // the one this host started with. Joining an existing queue is exactly when + // the two differ, and the setup lines this result prints are copied into a + // config file verbatim — printing the host's own would install a fallback + // that scans the wrong account the moment the fleet key is unset. + Scope []string `json:"scope,omitempty"` } func Init(ctx context.Context, cfg Config, gh *ghapi.GitHub, store StateStore) (InitResult, error) { @@ -38,6 +44,7 @@ func Init(ctx context.Context, cfg Config, gh *ghapi.GitHub, store StateStore) ( if err != nil { return InitResult{}, err } + cfg = applyFleet(cfg, state.FleetConfig, nil) if cfg.DashboardIssue <= 0 { body, err := issueBody(state, cfg) if err != nil { @@ -48,7 +55,7 @@ func Init(ctx context.Context, cfg Config, gh *ghapi.GitHub, store StateStore) ( return InitResult{}, err } cfg.DashboardIssue = issue.Number - } else if err := store.SyncDashboard(ctx, state); err != nil { + } else if err := store.SyncDashboard(ctx, state, cfg.storeConfig()); err != nil { return InitResult{}, err } return InitResult{ @@ -56,6 +63,7 @@ func Init(ctx context.Context, cfg Config, gh *ghapi.GitHub, store StateStore) ( DashboardIssue: cfg.DashboardIssue, CalibrationPR: cfg.CalibrationPR, StateRef: cfg.StateRef, + Scope: cfg.Scope, }, nil } diff --git a/internal/crq/init_test.go b/internal/crq/init_test.go new file mode 100644 index 00000000..ade05ece --- /dev/null +++ b/internal/crq/init_test.go @@ -0,0 +1,77 @@ +package crq + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/kristofferR/coderabbit-queue/internal/dialect" + ghapi "github.com/kristofferR/coderabbit-queue/internal/gh" +) + +func TestInitSyncsDashboardWithFleetReviewers(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_REPO": "owner/gate", + "CRQ_ISSUE": "7", + "CRQ_CAL_PR": "8", + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + inner := NewMemoryStore(cfg) + if _, err := inner.Update(ctx, func(st *State) error { + st.SetFleetValue("required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin) + return nil + }); err != nil { + t.Fatal(err) + } + store := &dashboardConfigStore{StateStore: inner} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + if _, err := Init(ctx, cfg, ghapi.NewTestClient(server.URL, server.Client()), store); err != nil { + t.Fatal(err) + } + if !strings.Contains(store.render.CoReviewers, "codex (required") { + t.Fatalf("dashboard co-reviewers = %q, want fleet-required codex", store.render.CoReviewers) + } +} + +// The setup lines init prints are copied into a config file verbatim. Joining a +// queue whose fleet scans another account and printing this host's own scope +// installs a fallback that targets the wrong one the moment the fleet key is +// unset — and reports a divergence immediately. +func TestInitReportsTheFleetScope(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_REPO": "owner/gate", + "CRQ_ISSUE": "7", + "CRQ_CAL_PR": "8", + "CRQ_SCOPE": "this-host", + }) + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("scope", "the-fleet") + return nil + }); err != nil { + t.Fatal(err) + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + result, err := Init(ctx, cfg, ghapi.NewTestClient(server.URL, server.Client()), store) + if err != nil { + t.Fatal(err) + } + if strings.Join(result.Scope, ",") != "the-fleet" { + t.Fatalf("scope = %v, want the fleet's", result.Scope) + } +} diff --git a/internal/crq/next.go b/internal/crq/next.go index 9170b48d..30ec0efd 100644 --- a/internal/crq/next.go +++ b/internal/crq/next.go @@ -174,11 +174,11 @@ func (s *Service) Next(ctx context.Context, repo string, pr int) (NextReport, er // settleUntil is when a convergence verdict may be trusted: the newest review // plus the configured quiet period. Nil when settling is disabled or nothing has // been observed to settle from. -func (s *Service) settleUntil(feedback FeedbackReport) *time.Time { - if s.cfg.SettleWindow <= 0 || feedback.LastEvidenceAt.IsZero() { +func settleUntil(feedback FeedbackReport, window time.Duration) *time.Time { + if window <= 0 || feedback.LastEvidenceAt.IsZero() { return nil } - at := feedback.LastEvidenceAt.Add(s.cfg.SettleWindow).UTC() + at := feedback.LastEvidenceAt.Add(window).UTC() return &at } @@ -260,7 +260,7 @@ func (s *Service) nextFromState(ctx context.Context, repo string, pr int) (NextR Deferred: feedback.CodeRabbitDeferred, DeferredUntil: feedback.DeferredUntil, MinDelay: s.cfg.PollInterval, - SettleUntil: s.settleUntil(feedback), + SettleUntil: settleUntil(feedback, feedback.config.SettleWindow), } // Only a round that still tracks THIS head may shape the verdict. A stale // fired/reviewing round carries its own phase and deadline, and an elapsed diff --git a/internal/crq/replay_test.go b/internal/crq/replay_test.go index 2a7e9b0c..98a6331c 100644 --- a/internal/crq/replay_test.go +++ b/internal/crq/replay_test.go @@ -241,6 +241,7 @@ func (f *replayFixture) autoReviewEnqueue(repo string, pr int) bool { if err != nil { f.t.Fatalf("load: %v", err) } + fleetRevision := f.svc.fleetCfg(st).FleetRevision need, head, err := f.svc.needsReview(f.ctx, st, repo, pr, true) if err != nil { f.t.Fatalf("needsReview %s#%d: %v", repo, pr, err) @@ -248,7 +249,7 @@ func (f *replayFixture) autoReviewEnqueue(repo string, pr int) bool { if !need { return false } - if err := f.svc.enqueueBatch(f.ctx, []queueCandidate{{Repo: repo, PR: pr, Head: head}}); err != nil { + if err := f.svc.enqueueBatch(f.ctx, []queueCandidate{{Repo: repo, PR: pr, Head: head}}, fleetRevision); err != nil { f.t.Fatalf("enqueueBatch %s#%d: %v", repo, pr, err) } return true diff --git a/internal/crq/repoconfig.go b/internal/crq/repoconfig.go index b79886cf..9ae02f5e 100644 --- a/internal/crq/repoconfig.go +++ b/internal/crq/repoconfig.go @@ -163,6 +163,7 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require } now := s.clock().UTC() state, err := s.store.Update(ctx, func(st *State) error { + base := s.fleetCfg(*st) ov, _ := st.RepoOverride(repo) beforeOverride := ov if coBots != nil { @@ -178,9 +179,9 @@ func (s *Service) SetReviewers(ctx context.Context, repo string, coBots, require return ErrNoChange } ov.UpdatedAt, ov.By = &now, s.cfg.Host - before := s.cfg.ForRepo(mustOverride(st, repo)) + before := base.ForRepo(mustOverride(st, repo)) st.SetRepoOverride(repo, ov) - s.reopenForChangedReviewers(st, repo, before, s.cfg.ForRepo(ov), open) + s.reopenForChangedReviewers(st, repo, before, base.ForRepo(ov), open) return nil }) if err != nil { @@ -204,11 +205,12 @@ func (s *Service) ClearReviewers(ctx context.Context, repo string) (ReviewerView return ReviewerView{}, err } state, err := s.store.Update(ctx, func(st *State) error { - before := s.cfg.ForRepo(mustOverride(st, repo)) + base := s.fleetCfg(*st) + before := base.ForRepo(mustOverride(st, repo)) if !st.ClearRepoOverride(repo) { return ErrNoChange } - s.reopenForChangedReviewers(st, repo, before, s.cfg, open) + s.reopenForChangedReviewers(st, repo, before, base, open) return nil }) if err != nil && !errors.Is(err, ErrNoChange) { diff --git a/internal/crq/repoconfig_test.go b/internal/crq/repoconfig_test.go index e53cd8e2..7d0e590f 100644 --- a/internal/crq/repoconfig_test.go +++ b/internal/crq/repoconfig_test.go @@ -67,6 +67,38 @@ func TestRepoOverrideReachesTheDecision(t *testing.T) { } } +func TestClearReviewersComparesAgainstFleetPolicy(t *testing.T) { + ctx := context.Background() + cfg := isolatedConfig(t, map[string]string{ + "CRQ_COBOTS": "codex", + "CRQ_REQUIRED_BOTS": "coderabbitai[bot]", + }) + repo, pr := "owner/repo", 19 + now := time.Now().UTC() + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{{Repo: repo, Number: pr}} + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("required-bots", "coderabbitai[bot],"+dialect.CodexBotLogin) + st.SetRepoOverride(repo, RepoReviewers{ + Required: []string{"coderabbitai[bot]"}, + SetRequired: true, + UpdatedAt: &now, + }) + st.PutRound(Round{Repo: repo, PR: pr, Head: "abcdef123", Phase: PhaseCompleted}) + return nil + }); err != nil { + t.Fatal(err) + } + if _, err := NewService(cfg, gh, store, nil).ClearReviewers(ctx, repo); err != nil { + t.Fatal(err) + } + st, _, _ := store.Load(ctx) + if round := st.Round(repo, pr); round == nil || round.Phase != PhaseQueued { + t.Fatalf("round = %+v, want clearing to the fleet reviewer set to reopen it", round) + } +} + // 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 @@ -662,7 +694,7 @@ func TestAReopenedPRPicksUpRequirementsChangedWhileItWasClosed(t *testing.T) { 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 { + if err := svc.enqueueBatch(ctx, []queueCandidate{{Repo: repo, PR: auto, Head: gotHead}}, svc.fleetCfg(st).FleetRevision); err != nil { t.Fatal(err) } @@ -833,6 +865,57 @@ func TestCompletionIsRevalidatedInsideItsWrite(t *testing.T) { } } +// A retry is policy-dependent too: its RetryAt and account block are computed +// from the fleet's rate-limit fallback, so a widened window landing in the same +// decision/write gap would be persisted at the old, earlier deadline — and the +// account unblocked in time for another metered review the fleet meant to defer. +func TestRetryIsRevalidatedInsideItsWrite(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + hooked := &hookedStore{StateStore: store} + svc := NewService(cfg, newFakeGitHub(), hooked, nil) + now := time.Now().UTC() + repo, pr, head := "o/r", 14, "aaaaaaaa1" + seedRound(t, store, cfg, repo, pr, head, PhaseFired, now, 22) + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + stale := svc.cfgFor(st, repo) + hooked.hook = func() { + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("rate-limit-fallback", "2h") + return nil + }); err != nil { + t.Error(err) + } + } + until := now.Add(15 * time.Minute) + retry := engine.Transition{ + Outcome: engine.OutRetry, + Reason: dialect.ReasonRateLimited, + RetryAt: until, + Blocked: &engine.AccountBlock{Until: until, CommentID: 99, CommentUpdated: now}, + } + if _, err := hooked.Update(ctx, func(st *State) error { + return svc.applyTransition(st, st.Round(repo, pr), retry, now, stale) + }); err != nil { + t.Fatal(err) + } + if got := roundPhase(t, store, repo, pr); got != PhaseFired { + t.Fatalf("phase = %s, want the stale retry dropped so it is recomputed under the fleet's window", got) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.Account.BlockedUntil != nil { + t.Fatalf("the block computed from the old window must not be recorded, got %v", st.Account.BlockedUntil) + } +} + // 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. @@ -913,3 +996,74 @@ func TestSelfHealTriggerIsRevalidatedInsideItsClaim(t *testing.T) { t.Errorf("codex bookkeeping = %+v, want no claim for a removed reviewer", c) } } + +func TestSelfHealDoesNotTriggerForFleetExcludedRepository(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.RequiredBots = []string{cfg.Bot, dialect.CodexBotLogin} + cfg.CoBots = codexCoBots(cfg.RequiredBots) + cfg.ExcludeRepos = map[string]bool{"o/r": true} + store := NewMemoryStore(cfg) + gh := newFakeGitHub() + svc := NewService(cfg, gh, store, nil) + now := time.Now().UTC() + seedRound(t, store, cfg, "o/r", 13, "aaaaaaaa1", PhaseFired, now, 22) + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + + svc.selfHealCoReviewers(ctx, cfg, *st.Round("o/r", 13), engine.Observation{ + Head: "aaaaaaaa1", Open: true, + }, now) + + if len(gh.posted) != 0 { + t.Fatalf("excluded repository received a co-review trigger: %v", gh.posted) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if c := st.Round("o/r", 13).Co(dialect.CodexBotLogin); c.ClaimedAt != nil || c.CommandID != 0 { + t.Fatalf("excluded repository retained a co-review claim: %+v", c) + } +} + +// The co-review wait is the one fire verdict with no later reconciliation to +// undo it: the round is still queued when a reviewer change lands, requeuing a +// queued round is a no-op, and the stale decision then parks it in reviewing +// with commands adopted for the reviewer set that no longer gates it. +func TestCoReviewWaitIsRevalidatedInsideItsWrite(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", 12, "aaaaaaaa2" + 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) + } + } + wait := engine.FireDecision{Verdict: engine.FireCoReviewWait, Reason: "awaiting co-review"} + obs := engine.Observation{Open: true, Head: head, HeadAt: now.Add(-time.Minute)} + result, err := svc.applyFire(ctx, svc.cfgFor(st, repo), round, obs, wait, now) + if err != nil { + t.Fatal(err) + } + if result.Action != "lost_race" { + t.Errorf("action = %q, want the wait 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 set decides it", got) + } +} diff --git a/internal/crq/reviewers.go b/internal/crq/reviewers.go index c851f3c0..1a998acd 100644 --- a/internal/crq/reviewers.go +++ b/internal/crq/reviewers.go @@ -321,18 +321,62 @@ func containsBot(logins []string, login string) bool { return false } -// cfgFor is the configuration crq should use for one repository: the fleet -// default with that repository's override applied. +// cfgFor is the configuration crq should use for one repository: this host's +// own settings, then the fleet's recorded policy, then that repository's +// override. Each layer is narrower than the one before it. func (s *Service) cfgFor(st State, repo string) Config { + base := s.fleetCfg(st) ov, ok := st.RepoOverride(repo) if !ok { - return s.cfg + return base } - out := s.cfg.ForRepo(ov) + out := base.ForRepo(ov) out.OverrideAt = ov.UpdatedAt return out } +// fleetCfg is this host's configuration with the fleet's policy applied. It is +// the base every other decision starts from, and the reason a setting only has +// to be changed in one place for every machine to follow it. +func (s *Service) fleetCfg(st State) Config { + cfg := s.cfg + if len(st.FleetConfig) != 0 { + cfg = applyFleet(cfg, st.FleetConfig, s.warnOnce) + } + cfg.FleetRevision = fleetRevision(st) + return cfg +} + +func fleetRevision(st State) string { + var b strings.Builder + for _, key := range st.FleetKeys() { + b.WriteString(key) + b.WriteByte(0) + b.WriteString(st.FleetConfig[key]) + b.WriteByte(0) + } + return b.String() +} + +// warnOnce logs a fleet-configuration complaint at most once per process. The +// same unreadable setting is met on every pass, and a daemon repeating it every +// two minutes for a week is how a log stops being read. +func (s *Service) warnOnce(msg string) { + if s.log == nil { + return + } + s.fleetWarned.Lock() + defer s.fleetWarned.Unlock() + if s.fleetWarnedOf == nil { + s.fleetWarnedOf = map[string]bool{} + } + if s.fleetWarnedOf[msg] { + return + } + s.fleetWarnedOf[msg] = true + s.log.Printf("config: %s", msg) +} + // overrideChanged reports whether repo's reviewer override differs from the one // cfg was built from. // @@ -343,6 +387,9 @@ func (s *Service) cfgFor(st State, repo string) Config { // 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 { + if fleetRevision(*st) != cfg.FleetRevision { + return true + } ov, _ := st.RepoOverride(repo) switch { case ov.UpdatedAt == nil && cfg.OverrideAt == nil: diff --git a/internal/crq/service.go b/internal/crq/service.go index 89bda48b..38760d2f 100644 --- a/internal/crq/service.go +++ b/internal/crq/service.go @@ -10,6 +10,7 @@ import ( "net/url" "sort" "strings" + "sync" "time" "github.com/kristofferR/coderabbit-queue/internal/dialect" @@ -63,6 +64,11 @@ type Service struct { // scanOffset rotates the bounded quota-free rescue scan's window so a round // past the first few is not starved forever; in-memory only, same writer. scanOffset int + // fleetWarned guards fleetWarnedOf, which remembers the fleet-configuration + // complaints already logged. Passes read the fleet policy concurrently with + // dispatched sessions, so this one is genuinely shared. + fleetWarned sync.Mutex + fleetWarnedOf map[string]bool // now overrides the wall clock for the scheduling DECISIONS in the // pump/enqueue/sweep/wait paths (see clock). nil in production; the replay // suite injects a controllable fake so an incident can be re-enacted @@ -232,6 +238,11 @@ func (s *Service) Enqueue(ctx context.Context, repo string, pr int) (EnqueueResu result.Head = head state, err := s.store.Update(ctx, func(st *State) error { now := s.clock() + if s.fleetCfg(*st).ExcludeRepos[repo] { + result.Held = true + result.Reason = "repository excluded by fleet policy" + return ErrNoChange + } if h, held := st.HeldPR(repo, pr); held { // Enqueueing a held PR would queue a round nothing may fire, which // reads on the dashboard as work waiting its turn. @@ -291,15 +302,22 @@ type queueCandidate struct { // dashboard sync, so a large autoreview pass doesn't produce N separate state // writes / issue edits. A PR already tracked at the same head is skipped; a // stale head is superseded. The DecideFire dedup still backstops at pump time. -func (s *Service) enqueueBatch(ctx context.Context, items []queueCandidate) error { - if len(items) == 0 { +func (s *Service) enqueueBatch(ctx context.Context, items []queueCandidate, expectedFleetRevision string) error { + if len(items) == 0 || s.cfg.DryRun { return nil } state, err := s.store.Update(ctx, func(st *State) error { now := s.clock() + cfg := s.fleetCfg(*st) + if cfg.FleetRevision != expectedFleetRevision { + return ErrNoChange + } added := 0 for _, it := range items { repo := NormalizeRepo(it.Repo) + if cfg.ExcludeRepos[repo] { + continue + } if _, held := st.HeldPR(repo, it.PR); held { continue } @@ -573,15 +591,35 @@ func (s *Service) advanceQuotaFree(ctx context.Context, repo string, pr int) (Pu // counts here. AcceptAccountBlock still decides whether it replaces the standing // window, and never shortens it. func (s *Service) recordObservedBlock(ctx context.Context, obs observation, st State, now time.Time) (*State, error) { - blk := engine.ObservedAccountBlock(obs.eng, s.cfg.policy(), st.Account, now) + cfg := s.fleetCfg(st) + blk := engine.ObservedAccountBlock(obs.eng, cfg.policy(), st.Account, now) if blk == nil || s.cfg.DryRun || !observedAccountBlockChanges(st.Account, blk) { return nil, nil } + // Update swallows ErrNoChange, so whether the block was recorded has to be + // carried out of the mutation rather than read from its error. + var recorded *engine.AccountBlock updated, err := s.store.Update(ctx, func(w *State) error { - if !observedAccountBlockChanges(w.Account, blk) { + recorded = nil + current := s.fleetCfg(*w) + // Only the SCOPE decides whose account this notice is evidence about. + // Any other fleet setting moving between the observation and this write + // leaves the notice just as valid, and comparing the whole revision + // discarded it — silently, since a returned ErrNoChange reads as a + // successful update. The account then stayed open for the rest of a + // window the bot had already stated, and the next fire went out inside + // it. So the window is recomputed here instead, under the policy that is + // current at the moment of the write, against the quota this write lands + // on. + if !sameFoldedSet(current.Scope, cfg.Scope) { + return ErrNoChange + } + blk := engine.ObservedAccountBlock(obs.eng, current.policy(), w.Account, now) + if blk == nil || !observedAccountBlockChanges(w.Account, blk) { return ErrNoChange } applyAccountBlock(w, blk, now) + recorded = blk return nil }) if err != nil { @@ -590,14 +628,19 @@ func (s *Service) recordObservedBlock(ctx context.Context, obs observation, st S } return nil, err } - // Like every other writer of this window: the dashboard is where an operator - // reads whether the account is available, and the decision that follows this - // is usually FireNo — which writes nothing further, leaving the issue claiming - // a free account for the whole block while state and `crq status` say - // otherwise. - s.sync(ctx, updated) - if s.log != nil { - s.log.Printf("account blocked until %s (observed, not tied to a round)", blk.Until.UTC().Format(time.RFC3339)) + // The state is handed back either way — it is newer than the caller's, and a + // window another host recorded in the meantime is one this decision has to + // respect. Only the report of a WRITE is conditional. + if recorded != nil { + // Like every other writer of this window: the dashboard is where an operator + // reads whether the account is available, and the decision that follows this + // is usually FireNo — which writes nothing further, leaving the issue claiming + // a free account for the whole block while state and `crq status` say + // otherwise. + s.sync(ctx, updated) + if s.log != nil { + s.log.Printf("account blocked until %s (observed, not tied to a round)", recorded.Until.UTC().Format(time.RFC3339)) + } } return &updated, nil } @@ -718,13 +761,31 @@ func (s *Service) recordDismissal(ctx context.Context, repo string, pr int, head // the rest of the CAS writes rather than beside each evidence source. // // It returns whether anything was written, and the block that stands either way. -func (s *Service) applyAccountBlock(ctx context.Context, until time.Time, source string) (bool, *time.Time, error) { +func (s *Service) applyAccountBlock( + ctx context.Context, + until time.Time, + source string, + expected Config, + cliOrg string, +) (bool, *time.Time, error) { now := s.clock() // Update swallows ErrNoChange, so whether anything was written has to be // recorded by the mutation itself. It is assigned on every attempt because a // CAS conflict runs the closure again. applied := false state, err := s.store.Update(ctx, func(st *State) error { + current := s.fleetCfg(*st) + // Only the SCOPE decides which account this block is evidence about, and + // cliOrgMatches asks the sharper form of that question against the policy + // the write lands on. Comparing the whole revision instead refused an + // explicit, organisation-attributed block because an unrelated setting — + // settle, repos, a co-reviewer's command — moved between the read and this + // write: the caller was told to run preflight again while the shared quota + // stayed open, and the daemon could post a metered review inside the window + // the CLI had just reported. + if !sameFoldedSet(current.Scope, expected.Scope) || !cliOrgMatches(current, cliOrg) { + return errFleetQuotaChanged + } if !engine.AcceptAccountBlock(st.Account.BlockedUntil, until) { applied = false return ErrNoChange @@ -734,7 +795,7 @@ func (s *Service) applyAccountBlock(ctx context.Context, until time.Time, source st.Account.BlockedUntil = &at st.Account.CheckedAt = &now st.Account.Source = source - st.Account.Scope = strings.Join(s.cfg.Scope, ",") + st.Account.Scope = strings.Join(current.Scope, ",") // A reading from outside the PR says nothing about how many reviews are // left, and a stale count beside a fresh block would read as authoritative. st.Account.Remaining = nil @@ -824,17 +885,29 @@ func (s *Service) progressSlotRound(ctx context.Context, slot Round) (PumpResult // 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) + // Captured before the transitions below clear it: this round's claim on the + // fire slot is what lets it release the slot at the end. + token := r.Token + // Completion and retry are the policy-dependent outcomes, so both are dropped + // when the policy moved under them. 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 — so a reviewer change + // committing between the decision and this write would be answered by neither: + // the marker dedupes the head under the set that no longer gates it. A retry + // carries a RetryAt and an account block computed from the fleet's fallback + // and backoff windows, so a widened window would be persisted at the old, + // earlier deadline and let another metered review fire inside it. Dropping is + // safe either way: the round keeps the slot and the next pump decides again + // under the current policy. The other outcomes record facts — an ack, a closed + // PR, a reservation that never posted — that no policy change revises. 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. + case engine.OutComplete, engine.OutRetry: if overrideChanged(st, r.Repo, cfg) { return ErrNoChange } + } + switch tr.Outcome { + case engine.OutComplete: if err := r.Complete(); err != nil { return err } @@ -855,19 +928,29 @@ func (s *Service) applyTransition(st *State, r *Round, tr engine.Transition, now } case engine.OutAbandon: st.EndRound(r.Repo, r.PR, tr.Reason) - releaseSlot(st, key) + releaseSlot(st, key, token) return nil default: return nil } st.PutRound(*r) - releaseSlot(st, key) + releaseSlot(st, key, token) return nil } -// releaseSlot clears the fire slot when it points at key. -func releaseSlot(st *State, key string) { - if st.FireSlot != nil && st.FireSlot.Key == key { +// releaseSlot clears the fire slot when it points at key AND token — the round +// that took it is the only one that may give it back. +// +// The key alone is not enough because the slot outlives the round: a hold left +// behind for a metered command whose round was superseded belongs to that +// command, not to the pull request it was posted on. Its replacement round at +// the same key ending — excluded by fleet policy, cancelled, or abandoned with +// the PR — would otherwise drop the hold and let a second metered review fire +// while the first command is still unanswered. Its token is the archived one's, +// so it releases nothing here. Nobody needs to: the hold is bounded by the +// in-flight window, and Normalize reaps the slot once it expires. +func releaseSlot(st *State, key, token string) { + if st.FireSlot != nil && st.FireSlot.Key == key && st.FireSlot.Token == token { st.FireSlot = nil st.ClearSlotHold() } @@ -1006,6 +1089,9 @@ func firedOrEnqueuedAt(r Round) time.Time { // 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) { + if cfg.ExcludeRepos[NormalizeRepo(round.Repo)] { + return s.abandonRound(ctx, round, "repository excluded by fleet policy", "skipped") + } switch d.Verdict { case engine.FireDrop: return s.abandonRound(ctx, round, "pr closed", "skipped") @@ -1057,7 +1143,7 @@ func (s *Service) abandonRound(ctx context.Context, round Round, reason, action return ErrNoChange } st.EndRound(round.Repo, round.PR, reason) - releaseSlot(st, QueueKey(round.Repo, round.PR)) + releaseSlot(st, QueueKey(round.Repo, round.PR), round.Token) ended = true return nil }) @@ -1249,7 +1335,7 @@ func (s *Service) fireRound(ctx context.Context, cfg Config, round Round, obs en if rerr := r.AwaitRetry(now.Add(postFailureBackoff), "failed to post review command: "+err.Error(), now); rerr != nil { return rerr } - releaseSlot(st, key) + releaseSlot(st, key, token) st.Warn = "failed to post review command: " + err.Error() st.PutRound(*r) return nil @@ -1532,7 +1618,12 @@ func (s *Service) fireCoReviewWait(ctx context.Context, cfg Config, round Round, updated, err := s.store.Update(ctx, func(st *State) error { changed = false r := st.Round(round.Repo, round.PR) - if !sameRound(r, round) || !firable(st, r, now) { + // This is the wait's commit point, so the reviewer set that chose it must + // still be the configured one. A change committed since the decision has + // already reconciled the round while it was queued, and nothing later + // undoes this transition: moving it to reviewing with adopted commands + // would leave the round waiting on — and crediting — the former set. + if !sameRound(r, round) || !firable(st, r, now) || overrideChanged(st, round.Repo, cfg) { return ErrNoChange } deadline := now.Add(s.cfg.FeedbackWaitTimeout) @@ -1790,7 +1881,7 @@ func (s *Service) fireCoDeferred(ctx context.Context, cfg Config, round Round, d // 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, cfg Config, round Round, obs engine.Observation, now time.Time) { - if s.cfg.DryRun || round.FiredAt == nil || obs.Head != round.Head { + if s.cfg.DryRun || cfg.ExcludeRepos[NormalizeRepo(round.Repo)] || round.FiredAt == nil || obs.Head != round.Head { return } firedAt := round.FiredAt.UTC() @@ -1843,11 +1934,13 @@ const triggerClaimTTL = 2 * time.Minute func (s *Service) Cancel(ctx context.Context, repo string, pr int) error { repo = NormalizeRepo(repo) state, err := s.store.Update(ctx, func(st *State) error { - if st.Round(repo, pr) == nil { + r := st.Round(repo, pr) + if r == nil { return ErrNoChange } + token := r.Token st.EndRound(repo, pr, "cancelled") - releaseSlot(st, QueueKey(repo, pr)) + releaseSlot(st, QueueKey(repo, pr), token) return nil }) if err != nil { @@ -1862,7 +1955,7 @@ func (s *Service) Status(ctx context.Context) (State, string, error) { if err != nil { return State{}, "", err } - return state, renderDashboard(state, s.cfg), nil + return state, renderDashboard(state, s.fleetCfg(state)), nil } func (s *Service) RefreshQuota(ctx context.Context) (State, error) { @@ -1870,22 +1963,51 @@ func (s *Service) RefreshQuota(ctx context.Context) (State, error) { if err != nil { return State{}, err } - if s.cfg.CalibrationPR <= 0 { + if s.cfg.DryRun { + return state, nil + } + cfg := s.fleetCfg(state) + if cfg.CalibrationPR <= 0 { return state, nil } now := s.clock() // Honor the freshness shortcut only when the last reading was conclusive. If a // probe is still pending (CalibAskedAt set, no reply yet), keep re-checking so a // late "account blocked" reply isn't ignored for the full TTL. - if state.Account.CalibAskedAt == nil && state.Account.CheckedAt != nil && now.Sub(*state.Account.CheckedAt) < s.cfg.CalibrationTTL { + if state.Account.CalibAskedAt == nil && state.Account.CheckedAt != nil && now.Sub(*state.Account.CheckedAt) < cfg.CalibrationTTL { return state, nil } - quota, err := s.readQuota(ctx, s.calibrationIssue(state), now, state.Account.CalibAskedAt) + quota, readAt, err := s.readQuota(ctx, s.calibrationIssue(state), now, state.Account.CalibAskedAt, cfg) if err != nil { return state, err } updated, err := s.store.Update(ctx, func(st *State) error { - if st.Account.CalibAskedAt == nil && st.Account.CheckedAt != nil && now.Sub(*st.Account.CheckedAt) < s.cfg.CalibrationTTL { + current := s.fleetCfg(*st) + // Only the scope invalidates the reading: it says WHICH account the probe + // answered for, and a scope that moved under the read already reset the + // quota to be recalibrated, so writing the old account's answer over that + // reset would state a window for an account crq no longer queues. + // + // Every other fleet setting leaves the reading true, and refusing it over + // one — as a whole-revision comparison did — discarded a still-standing + // account block, since Update reports a refused write as success: this + // same pump then went on to post a metered review inside the blocked + // window it had just been told about. + if !sameFoldedSet(current.Scope, cfg.Scope) { + return ErrNoChange + } + currentTTL := current.CalibrationTTL + if st.Account.CalibAskedAt == nil && st.Account.CheckedAt != nil && now.Sub(*st.Account.CheckedAt) < currentTTL { + return ErrNoChange + } + // The reading is an existing reply adopted under the TTL the read STARTED + // with. A fleet that shortened calibrate-ttl in the meantime has already + // declared that reply too old to decide from, and committing it stamps + // CheckedAt=now — presenting it as fresh for a whole new window, and + // letting a metered review fire on an availability result the shorter + // window asks crq to re-establish first. Drop it; the next pass reads + // under the TTL that is now current, and probes when nothing qualifies. + if !readAt.IsZero() && now.Sub(readAt) >= currentTTL { return ErrNoChange } // A fresh reading replaces the whole quota; carry the account-quota comment @@ -1981,25 +2103,32 @@ func (s *Service) rotateCalibration(ctx context.Context, oldIssue int) (int, err return issue.Number, nil } -func (s *Service) readQuota(ctx context.Context, issue int, now time.Time, pendingAsked *time.Time) (AccountQuota, error) { - quota := AccountQuota{Scope: strings.Join(s.cfg.Scope, ","), Source: "calibrate", CheckedAt: &now} - cutoff := now.Add(-s.cfg.CalibrationTTL) - keepAfter := now.Add(-2 * s.cfg.CalibrationTTL) +// readQuota reads the account quota off the calibration thread, adopting a reply +// that is still inside the TTL before spending a probe on a fresh one. +// +// It also returns when that adopted reply was written, and the zero time when +// the reading is the answer to a probe this call posted. Only the caller's write +// knows the TTL in force when the reading is committed, and an adopted reply +// accepted under a longer one is exactly what a shortened TTL invalidates. +func (s *Service) readQuota(ctx context.Context, issue int, now time.Time, pendingAsked *time.Time, cfg Config) (AccountQuota, time.Time, error) { + quota := AccountQuota{Scope: strings.Join(cfg.Scope, ","), Source: "calibrate", CheckedAt: &now} + cutoff := now.Add(-cfg.CalibrationTTL) + keepAfter := now.Add(-2 * cfg.CalibrationTTL) if reply, ok, err := s.latestCalibrationReply(ctx, issue, cutoff); err != nil { - return quota, err + return quota, time.Time{}, err } else if ok { remaining, reset := dialect.ParseQuota(reply.Body, reply.UpdatedAt) quota.Remaining = remaining quota.BlockedUntil = reset s.pruneCalibration(ctx, issue, keepAfter, 80) - return quota, nil + return quota, reply.UpdatedAt, nil } // A probe from a previous call is still pending and not yet stale, and no // reply to it was found: keep waiting for its (possibly late) reply instead of // posting another probe every cycle. if pendingAsked != nil && pendingAsked.After(cutoff) { quota.CalibAskedAt = pendingAsked - return quota, nil + return quota, time.Time{}, nil } asked, err := s.gh.PostIssueComment(ctx, s.cfg.GateRepo, issue, s.cfg.RateLimitCommand) if err != nil { @@ -2023,19 +2152,19 @@ func (s *Service) readQuota(ctx context.Context, issue int, now time.Time, pendi if s.log != nil { s.log.Printf("calibration probe on #%d failed: %v", issue, err) } - return quota, err + return quota, time.Time{}, err } } quota.CalibAskedAt = &asked.CreatedAt for i := 0; i < 6; i++ { select { case <-ctx.Done(): - return quota, ctx.Err() + return quota, time.Time{}, ctx.Err() case <-time.After(2 * time.Second): } reply, ok, err := s.latestCalibrationReply(ctx, issue, asked.CreatedAt.Add(-time.Second)) if err != nil { - return quota, err + return quota, time.Time{}, err } if ok { remaining, reset := dialect.ParseQuota(reply.Body, reply.UpdatedAt) @@ -2043,10 +2172,10 @@ func (s *Service) readQuota(ctx context.Context, issue int, now time.Time, pendi quota.BlockedUntil = reset quota.CalibAskedAt = nil s.pruneCalibration(ctx, issue, keepAfter, 80) - return quota, nil + return quota, time.Time{}, nil } } - return quota, nil + return quota, time.Time{}, nil } // pruneCalibration deletes crq's old calibration probe comments and CodeRabbit's @@ -2185,7 +2314,7 @@ func (s *Service) sync(ctx context.Context, state State) { if s.log == nil || s.cfg.DashboardIssue <= 0 { return } - if err := s.store.SyncDashboard(ctx, state); err != nil { + if err := s.store.SyncDashboard(ctx, state, s.fleetCfg(state).storeConfig()); err != nil { s.log.Printf("warning: dashboard sync failed: %v", err) } } diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index f8ebad98..e52578f1 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -73,6 +73,7 @@ func newFakeGitHub() *fakeGitHub { issueReactions: map[string][]ghapi.Reaction{}, reactions: map[int64][]ghapi.Reaction{}, reactionErrs: map[int64]error{}, + listPullErrs: map[string]error{}, deleteErrs: map[int64]error{}, deleteAfterErrs: map[int64]error{}, } @@ -264,8 +265,12 @@ func (f *fakeGitHub) SearchOpenPRs(context.Context, string, bool, int) ([]ghapi. return nil, nil } -func (f *fakeGitHub) EachOpenPR(_ context.Context, _ string, _ bool, fn func(ghapi.SearchPR) (bool, error)) error { +func (f *fakeGitHub) EachOpenPR(_ context.Context, target string, _ bool, fn func(ghapi.SearchPR) (bool, error)) error { f.mu.Lock() + if err := f.listPullErrs[strings.ToLower(target)]; err != nil { + f.mu.Unlock() + return err + } prs := append([]ghapi.SearchPR(nil), f.searchPRs...) f.mu.Unlock() for _, pr := range prs { @@ -417,7 +422,7 @@ func (s retryNoChangeStore) Update(_ context.Context, mutate func(*State) error) return second, nil } -func (retryNoChangeStore) SyncDashboard(context.Context, State) error { return nil } +func (retryNoChangeStore) SyncDashboard(context.Context, State, StoreConfig) error { return nil } // adoptionRaceStore loads a queued round with an adoptable command, but every // Update simulates another worker already holding the fire slot. @@ -453,7 +458,7 @@ func (s *adoptionRaceStore) Update(_ context.Context, mutate func(*State) error) return state, nil } -func (s *adoptionRaceStore) SyncDashboard(context.Context, State) error { return nil } +func (s *adoptionRaceStore) SyncDashboard(context.Context, State, StoreConfig) error { return nil } // --- test helpers --- @@ -622,6 +627,87 @@ func TestAutoReviewScanSkipsConfiguredAuthors(t *testing.T) { } } +// Who autoreview skips is fleet policy for the same reason the body marker is: +// leadership moves between hosts, and a Dependabot PR skipped on the machine +// that held the lease yesterday must not spend the shared allowance on the one +// that holds it today. +func TestAutoReviewScanSkipsTheFleetsAuthors(t *testing.T) { + ctx := context.Background() + cfg := Config{ + GateRepo: "o/gate", + Scope: []string{"o"}, + Host: "h", + Bot: "coderabbitai[bot]", + ReviewCommand: "@coderabbitai review", + LeaderTTL: time.Minute, + AutoReviewMaxScan: 10, + SkipAuthors: authorSet("renovate[bot]"), + } + gh := newFakeGitHub() + gh.searchPRs = []ghapi.SearchPR{ + {Repo: "o/app", Number: 1, Author: "Dependabot"}, + {Repo: "o/app", Number: 2, Author: "renovate[bot]"}, + {Repo: "o/app", Number: 3, Author: "alice"}, + } + for pr := 1; pr <= 3; pr++ { + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "abcdef1234567890" + gh.pulls[fakeKey("o/app", pr)] = pull + } + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("skip-authors", "dependabot[bot]") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + + if err := svc.AutoReview(ctx, AutoOptions{Once: true, Incremental: true}); err != nil { + t.Fatal(err) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if st.Round("o/app", 1) != nil { + t.Fatalf("the fleet's skipped author must never be queued, got %#v", st.Round("o/app", 1)) + } + if st.Round("o/app", 2) == nil { + t.Fatal("the fleet's list replaces this host's, so its author is reviewed again") + } + if st.Round("o/app", 3) == nil { + t.Fatalf("a human-authored PR must still be enqueued, got rounds=%#v", st.Rounds) + } +} + +// Without a fleet `scope` the pass's configuration is this host's own, so its +// target list shares the backing array of CRQ_SCOPE. Sorting it in place +// rewrote the operator's configured scan order under every later reader of it, +// including a concurrent one. +func TestAutoReviewScanLeavesTheConfiguredScopeAlone(t *testing.T) { + ctx := context.Background() + cfg := Config{ + GateRepo: "o/gate", + Scope: []string{"zeta-org", "alpha-org"}, + Host: "h", + Bot: "coderabbitai[bot]", + ReviewCommand: "@coderabbitai review", + LeaderTTL: time.Minute, + AutoReviewMaxScan: 10, + } + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + + if err := svc.AutoReview(ctx, AutoOptions{Once: true, Incremental: true}); err != nil { + t.Fatal(err) + } + if got := strings.Join(svc.cfg.Scope, ","); got != "zeta-org,alpha-org" { + t.Fatalf("Scope = %q, want the configured order untouched", got) + } +} + func TestAutoReviewScanSkipsMarkedPRs(t *testing.T) { ctx := context.Background() cfg := Config{ @@ -632,11 +718,11 @@ func TestAutoReviewScanSkipsMarkedPRs(t *testing.T) { ReviewCommand: "@coderabbitai review", LeaderTTL: time.Minute, AutoReviewMaxScan: 10, - SkipMarker: "", + SkipMarker: "", } gh := newFakeGitHub() gh.searchPRs = []ghapi.SearchPR{ - {Repo: "o/app", Number: 1, Author: "alice", Body: "Tiny maintenance change.\n\n"}, + {Repo: "o/app", Number: 1, Author: "alice", Body: "Tiny maintenance change.\n\n"}, {Repo: "o/app", Number: 2, Author: "alice", Body: "Review this change."}, } for pr := 1; pr <= 2; pr++ { @@ -646,6 +732,12 @@ func TestAutoReviewScanSkipsMarkedPRs(t *testing.T) { gh.pulls[fakeKey("o/app", pr)] = pull } store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("skip-marker", "") + return nil + }); err != nil { + t.Fatal(err) + } svc := NewService(cfg, gh, store, nil) if err := svc.AutoReview(ctx, AutoOptions{Once: true, Incremental: true}); err != nil { @@ -672,7 +764,7 @@ func TestEnqueueBatchAppendsOncePerPR(t *testing.T) { {Repo: "o/b", PR: 2, Head: "bbbbbbbb2"}, {Repo: "o/a", PR: 1, Head: "aaaaaaaa1"}, } - if err := svc.enqueueBatch(ctx, items); err != nil { + if err := svc.enqueueBatch(ctx, items, svc.fleetCfg(DefaultState(cfg)).FleetRevision); err != nil { t.Fatal(err) } st, _, _ := svc.store.Load(ctx) @@ -683,7 +775,7 @@ func TestEnqueueBatchAppendsOncePerPR(t *testing.T) { if queued[0].Seq == queued[1].Seq || queued[0].Seq == 0 { t.Fatalf("expected distinct non-zero seqs, got %d and %d", queued[0].Seq, queued[1].Seq) } - if err := svc.enqueueBatch(ctx, items); err != nil { + if err := svc.enqueueBatch(ctx, items, svc.fleetCfg(DefaultState(cfg)).FleetRevision); err != nil { t.Fatal(err) } st2, _, _ := svc.store.Load(ctx) @@ -709,7 +801,7 @@ func TestEnqueueBatchSkipsHeldPRsUnderCAS(t *testing.T) { {Repo: "o/held", PR: 1, Head: "aaaaaaaa1"}, {Repo: "o/ready", PR: 2, Head: "bbbbbbbb2"}, } - if err := svc.enqueueBatch(ctx, items); err != nil { + if err := svc.enqueueBatch(ctx, items, svc.fleetCfg(DefaultState(cfg)).FleetRevision); err != nil { t.Fatal(err) } st, _, err := store.Load(ctx) @@ -724,6 +816,60 @@ func TestEnqueueBatchSkipsHeldPRsUnderCAS(t *testing.T) { } } +func TestEnqueueBatchRejectsCandidatesSelectedUnderStaleFleetPolicy(t *testing.T) { + cfg := Config{GateRepo: "o/gate", Scope: []string{"o"}, Host: "h"} + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + ctx := context.Background() + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + selectedRevision := svc.fleetCfg(st).FleetRevision + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("repos", "o/elsewhere") + return nil + }); err != nil { + t.Fatal(err) + } + + if err := svc.enqueueBatch(ctx, []queueCandidate{ + {Repo: "o/obsolete", PR: 1, Head: "aaaaaaaa1"}, + }, selectedRevision); err != nil { + t.Fatal(err) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("o/obsolete", 1); round != nil { + t.Fatalf("stale fleet discovery enqueued a round: %+v", round) + } +} + +func TestEnqueueBatchDoesNotWriteInDryRun(t *testing.T) { + cfg := Config{GateRepo: "o/gate", Scope: []string{"o"}, Host: "h", DryRun: true} + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + ctx := context.Background() + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if err := svc.enqueueBatch(ctx, []queueCandidate{ + {Repo: "o/repo", PR: 1, Head: "aaaaaaaa1"}, + }, svc.fleetCfg(st).FleetRevision); err != nil { + t.Fatal(err) + } + st, _, err = store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("o/repo", 1); round != nil { + t.Fatalf("dry-run batch wrote a round: %+v", round) + } +} + func TestLatestCalibrationReplyToleratesBotSuffix(t *testing.T) { cfg := Config{Bot: "coderabbitai", GateRepo: "o/gate", CalibrationPR: 1, CalibrationMarker: "auto-generated reply by CodeRabbit"} gh := newFakeGitHub() @@ -2355,6 +2501,144 @@ func TestRefreshQuotaPreservesBlockOnInconclusiveProbe(t *testing.T) { } } +func TestRefreshQuotaRejectsAReadingFromAnObsoleteFleetScope(t *testing.T) { + ctx := context.Background() + cfg := Config{ + GateRepo: "owner/gate", StateRef: "crq-state-v3", CalibrationPR: 1, + CalibrationMarker: "auto-generated reply by CodeRabbit", + RateLimitCommand: "@coderabbitai rate limit", + CalibrationTTL: 2 * time.Minute, Scope: []string{"old-owner"}, + } + gh := newFakeGitHub() + inner := NewMemoryStore(cfg) + store := &hookedStore{StateStore: inner} + svc := NewService(cfg, gh, store, nil) + store.hook = func() { + if _, err := inner.Update(ctx, func(st *State) error { + st.SetFleetValue("scope", "new-owner") + st.Account = AccountQuota{Scope: "new-owner", Source: "fleet scope changed"} + return nil + }); err != nil { + t.Fatal(err) + } + } + + updated, err := svc.RefreshQuota(ctx) + if err != nil { + t.Fatal(err) + } + if updated.Account.Scope != "new-owner" || updated.Account.CalibAskedAt != nil || updated.Account.CheckedAt != nil { + t.Fatalf("obsolete calibration reading overwrote the new scope reset: %+v", updated.Account) + } +} + +// Only the scope says which account a probe answered for. Refusing the write +// because some unrelated fleet setting moved under the read dropped the block +// the probe had just found — and Update reports a refused write as success, so +// the same pump went on to post a metered review inside that window. +func TestRefreshQuotaRecordsABlockAcrossAnUnrelatedFleetChange(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CalibrationPR = 77 + cfg.GateRepo = "o/state" + cfg.CalibrationTTL = time.Minute + gh := newFakeGitHub() + inner := NewMemoryStore(cfg) + store := &hookedStore{StateStore: inner} + svc := NewService(cfg, gh, store, nil) + now := time.Now().UTC() + svc.now = func() time.Time { return now } + + reply := ghapi.IssueComment{ + ID: 5, + Body: "auto-generated reply by CodeRabbit\n> **Next review available in:** **45 minutes**", + CreatedAt: now.Add(-10 * time.Second), UpdatedAt: now.Add(-10 * time.Second), + } + reply.User.Login = cfg.Bot + gh.comments[fakeKey(cfg.GateRepo, 77)] = []ghapi.IssueComment{reply} + store.hook = func() { + if _, err := inner.Update(ctx, func(st *State) error { + st.SetFleetValue("min-interval", "45m") + return nil + }); err != nil { + t.Error(err) + } + } + + updated, err := svc.RefreshQuota(ctx) + if err != nil { + t.Fatal(err) + } + if updated.Account.BlockedUntil == nil || !updated.Account.BlockedUntil.After(now.Add(40*time.Minute)) { + t.Fatalf("account = %+v, want the probe's block recorded despite the unrelated policy change", updated.Account) + } +} + +// The other half of that rule: a reading adopted under the TTL the read started +// with is exactly what a SHORTENED calibrate-ttl invalidates. Committing it +// would stamp CheckedAt=now over a reply the new window already calls too old, +// making it fresh for another full TTL and letting a metered review fire on a +// calibration the fleet has just asked to be redone. +func TestRefreshQuotaRejectsAReadingAShortenedTTLMakesStale(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.CalibrationPR = 77 + cfg.GateRepo = "o/state" + cfg.CalibrationTTL = 10 * time.Minute + gh := newFakeGitHub() + inner := NewMemoryStore(cfg) + store := &hookedStore{StateStore: inner} + svc := NewService(cfg, gh, store, nil) + now := time.Now().UTC() + svc.now = func() time.Time { return now } + + reply := ghapi.IssueComment{ + ID: 5, + Body: "auto-generated reply by CodeRabbit\n> **6 reviews** remaining", + CreatedAt: now.Add(-5 * time.Minute), UpdatedAt: now.Add(-5 * time.Minute), + } + reply.User.Login = cfg.Bot + gh.comments[fakeKey(cfg.GateRepo, 77)] = []ghapi.IssueComment{reply} + store.hook = func() { + if _, err := inner.Update(ctx, func(st *State) error { + st.SetFleetValue("calibrate-ttl", "1m") + return nil + }); err != nil { + t.Error(err) + } + } + + updated, err := svc.RefreshQuota(ctx) + if err != nil { + t.Fatal(err) + } + if updated.Account.CheckedAt != nil || updated.Account.Remaining != nil { + t.Fatalf("account = %+v, want the stale reading refused rather than stamped fresh", updated.Account) + } +} + +func TestRefreshQuotaDoesNotProbeOrWriteInDryRun(t *testing.T) { + ctx := context.Background() + cfg := Config{ + GateRepo: "owner/gate", CalibrationPR: 1, DryRun: true, + CalibrationTTL: 2 * time.Minute, Scope: []string{"owner"}, + } + gh := newFakeGitHub() + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + + updated, err := svc.RefreshQuota(ctx) + if err != nil { + t.Fatal(err) + } + if len(gh.posted) != 0 { + t.Fatalf("dry-run calibration posted comments: %v", gh.posted) + } + if updated.Account.CalibAskedAt != nil || updated.Account.CheckedAt != nil { + t.Fatalf("dry-run calibration changed quota state: %+v", updated.Account) + } +} + // TestLoopDegradesToCodexOnlyOnRateLimit: a rate-limited round with Codex // activity must return Codex findings promptly — marked deferred — instead of // waiting out the CodeRabbit window. @@ -3105,6 +3389,58 @@ func TestFeedbackRecordsARateLimitNoticeItObserves(t *testing.T) { } } +// A fleet setting that has nothing to do with the account must not throw away +// an observed block. Update reports ErrNoChange as success, so refusing the +// write on a whole-revision mismatch left the caller believing the window had +// been recorded while the account stayed open — and the next fire went out +// inside it. +func TestObservedAccountBlockSurvivesAnUnrelatedFleetChange(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + pull := ghapi.Pull{State: "open"} + pull.Head.SHA = "a0646f010abcdef0" + gh.pulls[fakeKey("o/carrier", 82)] = pull + + notice := time.Now().UTC().Add(-time.Minute) + rl := ghapi.IssueComment{ID: 501, + Body: "\n> ## Review limit reached\n> **Next review available in:** **40 minutes**", + CreatedAt: notice, UpdatedAt: notice} + rl.User.Login = "coderabbitai[bot]" + gh.comments[fakeKey("o/carrier", 82)] = []ghapi.IssueComment{rl} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + now := time.Now().UTC() + + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + obs, err := svc.observe(ctx, svc.fleetCfg(st), "o/carrier", 82, nil, nil, now) + if err != nil { + t.Fatal(err) + } + // Somebody changes an unrelated policy while this notice is being read. + if _, err := store.Update(ctx, func(w *State) error { + w.SetFleetValue("settle", "30s") + return nil + }); err != nil { + t.Fatal(err) + } + + if _, err := svc.recordObservedBlock(ctx, obs, st, now); err != nil { + t.Fatal(err) + } + after, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if after.Account.BlockedUntil == nil { + t.Fatal("the observed block was discarded because an unrelated fleet setting moved") + } +} + func TestObservedAccountBlockPersistsANewerNoticeAtTheStandingWindow(t *testing.T) { now := time.Date(2026, 7, 27, 8, 0, 0, 0, time.UTC) standing := now.Add(time.Hour) diff --git a/internal/crq/state.go b/internal/crq/state.go index b9bb483c..210344c9 100644 --- a/internal/crq/state.go +++ b/internal/crq/state.go @@ -30,6 +30,8 @@ type ( // RepoAutofixSwitch is one repository's answer to whether crq may fix it. RepoAutofixSwitch = crqstate.RepoAutofixSwitch + + // RepoReviewers is the per-repository reviewer override (see Config.ForRepo). ) const ( @@ -48,6 +50,10 @@ const ( AutofixUnhealthyAfter = crqstate.AutofixUnhealthyAfter // CapsRepoOverrides is the binary capability per-repo reviewer overrides need. CapsRepoOverrides = crqstate.CapsRepoOverrides + // CapsFleetPolicy is the binary capability state-backed fleet policy needs. + CapsFleetPolicy = crqstate.CapsFleetPolicy + // WriterCaps is what THIS binary understands of the shared state. + WriterCaps = crqstate.WriterCaps ) var ( @@ -93,8 +99,18 @@ func (c Config) coReviewerSummary() string { // NewGitStateStore builds the git-ref-backed store. The logger surfaces the // loud auto-reinit line when a stale-schema payload is loaded. +// +// The store renders dashboard.md on every state commit, and it renders it with +// the fleet's policy rather than this host's startup settings — otherwise the +// file in the state ref would carry whichever co-reviewer set the writing +// machine happened to be configured with, while the gate issue shows the +// fleet's. func NewGitStateStore(cfg Config, gh *ghapi.GitHub, log Logger) *crqstate.GitStateStore { - return crqstate.NewGitStateStore(cfg.storeConfig(), gh, log) + store := crqstate.NewGitStateStore(cfg.storeConfig(), gh, log) + store.SetRenderConfig(func(st State) StoreConfig { + return applyFleet(cfg, st.FleetConfig, nil).storeConfig() + }) + return store } func NewMemoryStore(cfg Config) *crqstate.MemoryStore { diff --git a/internal/crq/threads.go b/internal/crq/threads.go index 86520c71..e2d8581c 100644 --- a/internal/crq/threads.go +++ b/internal/crq/threads.go @@ -36,6 +36,17 @@ type OpenThread struct { // // This is the read that closes that loop: list, decide, `crq resolve`. func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenThread, error) { + repo = NormalizeRepo(repo) + // The same reviewer set `crq feedback` reads findings with. A login the + // fleet or this repository added is a bot there, and reading threads from + // this host's own configuration instead reported its unresolved threads as + // human comments — the one output an agent uses to decide whether a thread + // needs a person. + st, _, err := s.store.Load(ctx) + if err != nil { + return nil, err + } + cfg := s.cfgFor(st, repo) threads, err := s.reviewThreads(ctx, repo, pr) if err != nil { return nil, err @@ -51,7 +62,7 @@ func (s *Service) OpenThreads(ctx context.Context, repo string, pr int) ([]OpenT // Only when the author is actually a reviewer bot. The command // promises every unresolved thread, humans included, and labelling // "alice" as a bot makes the output plainly wrong. - isBot := isReviewerBot(s.cfg, first.Author.Login) + isBot := isReviewerBot(cfg, first.Author.Login) if isBot { open.Bot = dialect.NormalizeBotName(first.Author.Login) } else { diff --git a/internal/crq/threads_test.go b/internal/crq/threads_test.go index 93bb90a6..caacf87b 100644 --- a/internal/crq/threads_test.go +++ b/internal/crq/threads_test.go @@ -89,6 +89,45 @@ func TestOpenThreadsNamesAConfiguredFeedbackBot(t *testing.T) { } } +// The same, for a reviewer only the FLEET names. `crq feedback` already reads +// its findings through the effective configuration, so listing its thread as a +// human's would have the two commands disagree about the same thread. +func TestOpenThreadsNamesAFleetFeedbackBot(t *testing.T) { + gh := newFakeGitHub() + gh.graphQL = func(query string, _ map[string]any, out any) error { + if !strings.Contains(query, "reviewThreads") { + return nil + } + return json.Unmarshal([]byte(`{"repository":{"pullRequest":{"reviewThreads":{ + "pageInfo":{"hasNextPage":false,"endCursor":""}, + "nodes":[{"id":"T_fleet","isResolved":false,"isOutdated":false,"path":"a.go","line":1, + "comments":{"totalCount":1,"nodes":[{"body":"### Nil deref\n\n**High Severity**\n\nDetail.", + "author":{"login":"reviewdog[bot]"}}]}}] + }}}}`), out) + } + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("feedback-bots", "coderabbitai[bot],reviewdog[bot]") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) + + threads, err := svc.OpenThreads(ctx, "o/r", 1) + if err != nil { + t.Fatal(err) + } + if len(threads) != 1 { + t.Fatalf("got %d threads, want the fleet reviewer's", len(threads)) + } + if threads[0].Bot != "reviewdog" || threads[0].Author != "" { + t.Errorf("thread = %+v, want the fleet's feedback bot named as a bot", threads[0]) + } +} + // The command promises every unresolved thread, so it also gets humans'. Copying // the author into `bot` made the output say `"bot":"alice"`. func TestOpenThreadsDoesNotCallAPersonABot(t *testing.T) { diff --git a/internal/crq/tidy.go b/internal/crq/tidy.go index bb04f1f6..96cecd02 100644 --- a/internal/crq/tidy.go +++ b/internal/crq/tidy.go @@ -67,10 +67,12 @@ func (s *Service) Tidy(ctx context.Context, repo string, pr int, dryRun bool) (T return result, err } // Which comments count as a trigger depends on who reviews, so the whole - // path takes a configuration value rather than reading the Service's. That - // is what lets per-repo reviewers substitute one here later without - // threading anything new through. - cfg := s.cfg + // path takes a configuration value rather than reading the Service's — the + // effective one, fleet policy and this repository's override included. + // Reading this host's own would compare the comments against a command crq + // never posted, read every spent trigger as edited by hand, and keep them on + // the PR for ever. + cfg := s.cfgFor(st, repo) observedPosted := collectPosted(st, repo, pr) if len(observedPosted.commands) == 0 { result.Kept = append(result.Kept, "no round on this pr posted a trigger comment") diff --git a/internal/crq/tidy_test.go b/internal/crq/tidy_test.go index 47c379d3..9c958b3d 100644 --- a/internal/crq/tidy_test.go +++ b/internal/crq/tidy_test.go @@ -3,6 +3,7 @@ package crq import ( "context" "encoding/json" + "strings" "testing" "time" @@ -365,6 +366,59 @@ func TestTidyKeepsATriggerEditedDuringThePass(t *testing.T) { } } +// The command crq POSTED is the fleet's, so that is the one a trigger comment +// has to be compared against. Reading this host's own startup configuration +// instead found no such command, read every spent co-reviewer trigger as edited +// by hand, and kept them on the PR for ever. +func TestTidyRecognisesATriggerTheFleetConfigured(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + gh := newFakeGitHub() + gh.graphQL = noForcePush + now := time.Now().UTC() + repo, pr := "o/r", 27 + const command = "bugbot run please" + + var pull ghapi.Pull + pull.State = "open" + pull.Head.SHA = "bbbbbbbb2" + gh.pulls[fakeKey(repo, pr)] = pull + gh.commits["bbbbbbbb2"] = commitAt(now.Add(-10 * time.Minute)) + + c := ghapi.IssueComment{ID: 100, Body: command, + CreatedAt: now.Add(-2 * time.Hour), UpdatedAt: now.Add(-2 * time.Hour)} + c.User.Login = "kristofferR" + gh.comments[fakeKey(repo, pr)] = []ghapi.IssueComment{c} + + store := NewMemoryStore(cfg) + svc := NewService(cfg, gh, store, nil) + if _, err := store.Update(ctx, func(st *State) error { + // The fleet enables a co-reviewer this host does not run, with a command + // only the fleet knows. + st.SetFleetValue("cobots", "bugbot") + st.SetFleetValue("cobot-bugbot-cmd", command) + return completedRound(st, repo, pr, now, func(r *Round) error { + if err := r.Fire(0, now.Add(-2*time.Hour)); err != nil { + return err + } + r.RecordPosted("cursor[bot]", c.ID, c.CreatedAt) + return nil + }) + }); err != nil { + t.Fatal(err) + } + + result, err := svc.Tidy(ctx, repo, pr, true) + if err != nil { + t.Fatal(err) + } + for _, kept := range result.Kept { + if strings.Contains(kept, "someone edited it") { + t.Fatalf("the fleet's own trigger read as an edited comment: %v", result.Kept) + } + } +} + // A delete GitHub refuses must reach the caller. An empty Deleted otherwise // reads as "nothing was spent" when it means "the token may not delete". func TestTidyReportsDeletionFailures(t *testing.T) { diff --git a/internal/crq/watch.go b/internal/crq/watch.go index 1c4a2b13..150dfdb1 100644 --- a/internal/crq/watch.go +++ b/internal/crq/watch.go @@ -177,8 +177,36 @@ func (s *Service) Watch(ctx context.Context, opts WatchOptions, emit func(WatchE } } +// watchesRepo reports whether the policy in hand still covers this repository, +// the same two questions the pass asked when it gathered its candidates: +// `exclude` means "crq does not go here" everywhere, and the fleet's repository +// list decides the set whenever the pass had to build one. Repositories named on +// the command line are the operator's own request and outrank the list — but not +// the exclusion, exactly as at the top of the pass. +// +// An emptied list is not "watch everything" here. Unlike autoReviewPass, which +// falls back to scanning by scope, a pass with no repositories of its own has +// nothing to watch at all and refuses to run — so a candidate gathered under the +// old list is one the live policy no longer covers, and treating the empty list +// as permissive would dispatch a fix for a repository the fleet just dropped. +func watchesRepo(cfg Config, explicit []string, repo string) bool { + key := NormalizeRepo(repo) + if cfg.ExcludeRepos[key] { + return false + } + if len(explicit) > 0 { + return true + } + return cfg.AllowRepos[key] +} + func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispatchPool, emit func(WatchEvent) error) error { var failures []string + state, _, err := s.store.Load(ctx) + if err != nil { + return err + } + cfg := s.fleetCfg(state) type pendingEvent struct { event WatchEvent result <-chan dispatchResult @@ -190,10 +218,10 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispat // repository names straight over the fix command, and every dispatch in the // fleet died with "fork/exec kristofferr/coderabbit-queue: no such file or // directory". A caller's slice is the caller's. - repos := make([]string, 0, len(opts.Repos)+len(s.cfg.AllowRepos)) + repos := make([]string, 0, len(opts.Repos)+len(cfg.AllowRepos)) repos = append(repos, opts.Repos...) if len(repos) == 0 { - for repo := range s.cfg.AllowRepos { + for repo := range cfg.AllowRepos { repos = append(repos, repo) } sort.Strings(repos) // stable order: a pass must not depend on map iteration @@ -213,16 +241,14 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispat pull ghapi.Pull } var candidates []candidate - gate := NormalizeRepo(s.cfg.GateRepo) + gate := NormalizeRepo(cfg.GateRepo) // Which repositories may be FIXED is per repository, read once for the pass. // Watching is unaffected: a repository with autofix off is still observed // and still reviewed, so its feedback arrives for a person to act on. autofixOff := map[string]bool{} - if st, _, err := s.store.Load(ctx); err == nil { - for _, repo := range repos { - if !st.AutofixEnabled(repo) { - autofixOff[NormalizeRepo(repo)] = true - } + for _, repo := range repos { + if !state.AutofixEnabled(repo) { + autofixOff[NormalizeRepo(repo)] = true } } for _, repo := range repos { @@ -238,7 +264,7 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispat // it; this one did not, so the single setting that reads like a fleet-wide // opt-out silently covered half of what crq does — reviews stopped and the // watcher carried on, which is not a setting anyone can reason about. - if s.cfg.ExcludeRepos[NormalizeRepo(repo)] { + if cfg.ExcludeRepos[NormalizeRepo(repo)] { continue } pulls, err := s.gh.ListPulls(ctx, repo, openPullQuery()) @@ -286,10 +312,40 @@ func (s *Service) watchPass(ctx context.Context, opts WatchOptions, pool *dispat return err } // The fleet's skip marker suppresses review deliberately, to protect - // the shared quota. Next is a MUTATING oracle — it enqueues and can - // fire — so the marker has to be honoured before calling it, not - // after. - if s.cfg.SkipsReview(pull.Body) { + // the shared quota, and its repository lists say where crq goes at + // all. Next is a MUTATING oracle — it enqueues and can fire — and it + // enforces neither, because it is the manual path: its target is a + // request a person made. So both have to be honoured before calling + // it, not after. + // + // The policy is re-read here rather than reused from the top of the + // pass. A fleet-wide scan is long, so a marker the fleet adopted — or + // a repository it dropped — mid-pass would otherwise spend a metered + // review on every candidate still to come in it, gathered under the + // policy the pass started with. Rereading costs a conditional GET on + // the ETag'd state ref, against a call that already re-observes the + // PR. + live, _, lerr := s.store.Load(ctx) + if lerr != nil { + if _, throttled := ghapi.ThrottleWait(lerr); throttled { + return lerr + } + // Next reads the same ref and would fail the same way; treat it + // like any other unreadable candidate rather than firing under a + // policy crq could not confirm. + if s.log != nil { + s.log.Printf("watch: %s#%d: %v", repo, pull.Number, lerr) + } + if opts.Once { + failures = append(failures, fmt.Sprintf("%s#%d: %v", repo, pull.Number, lerr)) + } + continue + } + liveCfg := s.fleetCfg(live) + if !watchesRepo(liveCfg, opts.Repos, repo) { + continue + } + if liveCfg.SkipsReview(pull.Body) { event := WatchEvent{ Repo: repo, PR: pull.Number, Action: "skipped", Reason: "fleet auto-review skip marker present", @@ -786,6 +842,10 @@ func (s *Service) claimDispatch(ctx context.Context, report NextReport, token st reason, byDesign = "fix sessions are disabled for this repository", true return ErrNoChange } + if s.fleetCfg(*st).ExcludeRepos[NormalizeRepo(report.Repo)] { + reason, byDesign = "repository is excluded by fleet policy", true + return ErrNoChange + } round := st.Round(report.Repo, report.PR) // What this attempt actually read, recorded before anything acts on it. // Three sessions once ran on one PR at one head while the round showed no diff --git a/internal/crq/watch_test.go b/internal/crq/watch_test.go index 03b87615..14613731 100644 --- a/internal/crq/watch_test.go +++ b/internal/crq/watch_test.go @@ -689,7 +689,7 @@ type dashboardCountingStore struct { syncs []State } -func (s *dashboardCountingStore) SyncDashboard(_ context.Context, state State) error { +func (s *dashboardCountingStore) SyncDashboard(_ context.Context, state State, _ StoreConfig) error { s.syncs = append(s.syncs, cloneState(state)) return nil } @@ -808,6 +808,35 @@ func TestClaimDispatchRechecksRepositoryAutofixSwitch(t *testing.T) { } } +func TestClaimDispatchRechecksFleetExclusion(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + store := NewMemoryStore(cfg) + svc := NewService(cfg, newFakeGitHub(), store, nil) + report := NextReport{ + Repo: "owner/thing", PR: 12, Head: "aaaaaaaa1", Action: "fix", + Findings: []dialect.Finding{{ID: "f1", Commit: "aaaaaaaa1"}}, + } + if _, err := store.Update(ctx, func(st *State) error { + st.SetFleetValue("exclude", report.Repo) + return nil + }); err != nil { + t.Fatal(err) + } + + ok, why, byDesign := svc.claimDispatch(ctx, report, "tok", 3) + if ok || !byDesign || !strings.Contains(why, "excluded") { + t.Fatalf("claim = %v, %q, %v; want a fleet-exclusion refusal", ok, why, byDesign) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round(report.Repo, report.PR); round != nil && round.Dispatch != nil { + t.Errorf("refused claim mutated the round: %+v", round) + } +} + // A head that moved on while its previous round still stood: `Next` reports fix // without enqueueing, so nothing else supersedes the stale round. Refusing the // claim over the mismatch left the new head's findings undispatchable on every @@ -1066,14 +1095,20 @@ func dispatchOn() *bool { func TestWatchHonoursTheExcludedRepositories(t *testing.T) { cfg := firingConfig() cfg.AllowRepos = map[string]bool{"owner/kept": true, "owner/gone": true} - cfg.ExcludeRepos = map[string]bool{"owner/gone": true} gh := newFakeGitHub() for _, repo := range []string{"owner/kept", "owner/gone"} { var pull ghapi.Pull pull.State, pull.Number, pull.Head.SHA = "open", 1, "aaaaaaaa1" gh.pulls[fakeKey(repo, 1)] = pull } - svc := NewService(cfg, gh, NewMemoryStore(cfg), nil) + store := NewMemoryStore(cfg) + if _, err := store.Update(context.Background(), func(st *State) error { + st.SetFleetValue("exclude", "owner/gone") + return nil + }); err != nil { + t.Fatal(err) + } + svc := NewService(cfg, gh, store, nil) var seen []string if err := svc.watchPass(context.Background(), WatchOptions{}, newDispatchPool(0), @@ -1089,3 +1124,152 @@ func TestWatchHonoursTheExcludedRepositories(t *testing.T) { t.Error("excluding one repository stopped the rest being watched") } } + +// loadHookedStore runs a hook once, immediately AFTER the first Load returns — +// the window between a pass reading the fleet policy and acting on the +// candidates it collected under it. +type loadHookedStore struct { + StateStore + hook func() +} + +func (l *loadHookedStore) Load(ctx context.Context) (State, Revision, error) { + st, rev, err := l.StateStore.Load(ctx) + if l.hook != nil { + hook := l.hook + l.hook = nil + hook() + } + return st, rev, err +} + +// A fleet-wide scan is long, and `Next` is a mutating oracle that does not +// enforce the body marker itself. Reusing the policy read at the top of the +// pass therefore spent a metered review on every candidate still to come after +// the fleet opted them out. +func TestWatchRereadsTheSkipMarkerBeforeEachCandidate(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/repo": true} + gh := newFakeGitHub() + var pull ghapi.Pull + pull.State, pull.Number, pull.Head.SHA = "open", 1, "aaaaaaaa1" + pull.Body = "Tiny change.\n\n" + gh.pulls[fakeKey("owner/repo", 1)] = pull + + store := NewMemoryStore(cfg) + hooked := &loadHookedStore{StateStore: store} + svc := NewService(cfg, gh, hooked, nil) + hooked.hook = func() { + if err := svc.SetFleetConfig(ctx, "skip-marker", ""); err != nil { + t.Error(err) + } + } + + var seen []WatchEvent + if err := svc.watchPass(ctx, WatchOptions{}, newDispatchPool(0), + func(e WatchEvent) error { seen = append(seen, e); return nil }); err != nil { + t.Fatal(err) + } + if len(seen) != 1 || seen[0].Action != "skipped" { + t.Fatalf("events = %+v, want the marker adopted mid-pass to skip the candidate", seen) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("owner/repo", 1); round != nil { + t.Fatalf("round = %+v, want an opted-out PR never queued", round) + } +} + +// The candidates are gathered under the policy the pass started with, and `Next` +// treats its target as a manual request — so a repository the fleet dropped +// mid-pass still had its PRs enqueued and reviewed by the pass that was already +// running, in a repository crq no longer watches. +func TestWatchRereadsTheRepositoryListBeforeEachCandidate(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/repo": true} + gh := newFakeGitHub() + var pull ghapi.Pull + pull.State, pull.Number, pull.Head.SHA = "open", 1, "aaaaaaaa1" + gh.pulls[fakeKey("owner/repo", 1)] = pull + + store := NewMemoryStore(cfg) + hooked := &loadHookedStore{StateStore: store} + svc := NewService(cfg, gh, hooked, nil) + hooked.hook = func() { + if err := svc.SetFleetConfig(ctx, "repos", "owner/other"); err != nil { + t.Error(err) + } + } + + var seen []WatchEvent + if err := svc.watchPass(ctx, WatchOptions{}, newDispatchPool(0), + func(e WatchEvent) error { seen = append(seen, e); return nil }); err != nil { + t.Fatal(err) + } + if len(seen) != 0 { + t.Fatalf("events = %+v, want no work in a repository the fleet dropped mid-pass", seen) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("owner/repo", 1); round != nil { + t.Fatalf("round = %+v, want an unwatched repository never queued", round) + } +} + +// Emptying the list is the same removal. `crq watch` has no scope fallback — a +// pass with no repositories of its own refuses to run at all — so an emptied +// list left the candidates of the pass already running as the only repositories +// crq would ever act on, each one no longer covered by the live policy. +func TestWatchDropsCandidatesWhenTheRepositoryListIsEmptied(t *testing.T) { + ctx := context.Background() + cfg := firingConfig() + cfg.AllowRepos = map[string]bool{"owner/repo": true} + gh := newFakeGitHub() + var pull ghapi.Pull + pull.State, pull.Number, pull.Head.SHA = "open", 1, "aaaaaaaa1" + gh.pulls[fakeKey("owner/repo", 1)] = pull + + store := NewMemoryStore(cfg) + hooked := &loadHookedStore{StateStore: store} + svc := NewService(cfg, gh, hooked, nil) + hooked.hook = func() { + if err := svc.SetFleetConfig(ctx, "repos", ""); err != nil { + t.Error(err) + } + } + + var seen []WatchEvent + if err := svc.watchPass(ctx, WatchOptions{}, newDispatchPool(0), + func(e WatchEvent) error { seen = append(seen, e); return nil }); err != nil { + t.Fatal(err) + } + if len(seen) != 0 { + t.Fatalf("events = %+v, want no work once the fleet's list is emptied mid-pass", seen) + } + st, _, err := store.Load(ctx) + if err != nil { + t.Fatal(err) + } + if round := st.Round("owner/repo", 1); round != nil { + t.Fatalf("round = %+v, want an unwatched repository never queued", round) + } +} + +// A repository named on the command line is the operator's own request and +// outranks the fleet's list, emptied or not — only the exclusion overrides it. +func TestWatchesRepoHonoursExplicitTargetsAndExclusion(t *testing.T) { + empty := Config{} + if !watchesRepo(empty, []string{"owner/repo"}, "owner/repo") { + t.Error("an explicitly requested repository must stay watched with no fleet list") + } + excluded := Config{ExcludeRepos: map[string]bool{"owner/repo": true}} + if watchesRepo(excluded, []string{"owner/repo"}, "owner/repo") { + t.Error("exclusion outranks an explicit target") + } +} diff --git a/internal/state/autofix_test.go b/internal/state/autofix_test.go index eb42a910..bb868b1f 100644 --- a/internal/state/autofix_test.go +++ b/internal/state/autofix_test.go @@ -23,7 +23,7 @@ func TestAutofixHealthSurfacesAFailingDispatcher(t *testing.T) { if st.Autofix.Unhealthy() { t.Error("one failure must not raise the alarm") } - for i := 0; i < AutofixUnhealthyAfter; i++ { + for i := 1; i < AutofixUnhealthyAfter; i++ { st.NoteDispatch("cachyos", false, "refusing to fetch into branch", now) } if !st.Autofix.Unhealthy() { diff --git a/internal/state/dashboard.go b/internal/state/dashboard.go index 1b6150c1..74d3f0e7 100644 --- a/internal/state/dashboard.go +++ b/internal/state/dashboard.go @@ -272,6 +272,7 @@ func hostCell(writer string) string { // RenderDashboard renders the human-facing dashboard for the current state: // rounds by phase instead of v2's queue/fired/awaiting maps. func RenderDashboard(st State, cfg StoreConfig) string { + cfg = cfg.withFleet(st) loc := dashboardLoc(cfg) now := time.Now().UTC() queue := st.Queue(now, cfg.MinInterval) @@ -331,7 +332,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { } else { fmt.Fprintf(&b, "| **CodeRabbit quota** | ✅ not currently blocked |\n") } - if cfg.CoReviewers != "" { + if cfg.CoReviewers != "" || hasCoReviewerOverrides(st) { fmt.Fprintf(&b, "| **Co-reviewers** | %s |\n", coReviewerCell(st, cfg.CoReviewers)) } fmt.Fprintf(&b, "| **Last review fired** | %s |\n", fmtStamp(st.LastFired, loc)) @@ -426,6 +427,7 @@ func RenderDashboard(st State, cfg StoreConfig) string { // count is the WHOLE queue, cooling-down rounds included: a state whose only // work is not yet fire-eligible is queued, never idle. func RenderTitle(st State, cfg StoreConfig) string { + cfg = cfg.withFleet(st) now := time.Now().UTC() queue := len(st.Queue(now, cfg.MinInterval)) held := len(heldRounds(st)) @@ -483,11 +485,14 @@ func cell(v string) string { // labelled, and the repositories that differ are named beside it. func coReviewerCell(st State, fleet string) string { overrides := make([]string, 0, len(st.Repos)) - for repo := range st.Repos { + for repo, settings := range st.Repos { + if !settings.SetCoBots { + continue + } overrides = append(overrides, repo) } if len(overrides) == 0 { - return fleet + " _(fleet default)_" + return dash(fleet) + " _(fleet default)_" } sort.Strings(overrides) // Named, not just counted: "3 repositories differ" sends the reader to the @@ -498,13 +503,22 @@ func coReviewerCell(st State, fleet string) string { if len(listed) > show { listed, suffix = listed[:show], fmt.Sprintf(" +%d more", len(overrides)-show) } - return fmt.Sprintf("%s _(fleet default; %s override%s)_", fleet, + return fmt.Sprintf("%s _(fleet default; %s override%s)_", dash(fleet), strings.Join(listed, ", ")+suffix, plural(len(overrides))) } +func hasCoReviewerOverrides(st State) bool { + for _, settings := range st.Repos { + if settings.SetCoBots { + return true + } + } + return false +} + func plural(n int) string { if n == 1 { - return "s" + return "" } - return "" + return "s" } diff --git a/internal/state/dashboard_sync_test.go b/internal/state/dashboard_sync_test.go index 831e5963..d96f4fda 100644 --- a/internal/state/dashboard_sync_test.go +++ b/internal/state/dashboard_sync_test.go @@ -98,7 +98,7 @@ func TestSyncDashboardWritesOnlyOnChange(t *testing.T) { st := syncTestState(t) for i := 0; i < 4; i++ { - if err := store.SyncDashboard(ctx, st); err != nil { + if err := store.SyncDashboard(ctx, st, cfg); err != nil { t.Fatalf("sync %d: %v", i, err) } } @@ -120,7 +120,7 @@ func TestSyncDashboardWritesOnlyOnChange(t *testing.T) { t.Fatal(err) } st.Normalize(t0) - if err := store.SyncDashboard(ctx, st); err != nil { + if err := store.SyncDashboard(ctx, st, cfg); err != nil { t.Fatal(err) } srv.mu.Lock() @@ -142,7 +142,7 @@ func TestSyncDashboardSkipsWriteWhenTheIssueAlreadyMatches(t *testing.T) { st := syncTestState(t) // One process (think: the daemon) publishes the dashboard. - if err := NewGitStateStore(cfg, client, nil).SyncDashboard(ctx, st); err != nil { + if err := NewGitStateStore(cfg, client, nil).SyncDashboard(ctx, st, cfg); err != nil { t.Fatal(err) } srv.mu.Lock() @@ -155,7 +155,7 @@ func TestSyncDashboardSkipsWriteWhenTheIssueAlreadyMatches(t *testing.T) { // A genuinely cold second process against the same issue: its own client, so // it shares no ETag cache with the first. cold := NewGitStateStore(cfg, gh.NewTestClient(httpSrv.URL, httpSrv.Client()), nil) - if err := cold.SyncDashboard(ctx, st); err != nil { + if err := cold.SyncDashboard(ctx, st, cfg); err != nil { t.Fatal(err) } srv.mu.Lock() @@ -167,3 +167,82 @@ func TestSyncDashboardSkipsWriteWhenTheIssueAlreadyMatches(t *testing.T) { t.Fatal("the cross-process check must actually read the issue") } } + +func TestSyncDashboardUsesTheSuppliedEffectiveRenderingConfig(t *testing.T) { + srv := &issueServer{} + _, client := srv.start(t) + startup := StoreConfig{ + GateRepo: "owner/state", StateRef: "crq-state-v3", DashboardIssue: 1, + Scope: []string{"owner"}, CoReviewers: "codex (selfheal)", + } + effective := startup + effective.CoReviewers = "codex (required, always)" + st := syncTestState(t) + + if err := NewGitStateStore(startup, client, nil).SyncDashboard(context.Background(), st, effective); err != nil { + t.Fatal(err) + } + srv.mu.Lock() + defer srv.mu.Unlock() + if !strings.Contains(srv.body, effective.CoReviewers) || strings.Contains(srv.body, startup.CoReviewers) { + t.Fatalf("dashboard body did not use the effective reviewer summary:\n%s", srv.body) + } +} + +// dashboard.md is rewritten on EVERY state commit, so it has to be rendered +// with the same effective policy the gate issue is. Rendering it from the +// writing host's startup configuration would put that machine's co-reviewer set +// in the shared file — disagreeing with the issue, and flapping between hosts +// that are each configured differently. +func TestStateCommitsRenderTheDashboardFileWithTheEffectiveConfig(t *testing.T) { + var blobs []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + enc := json.NewEncoder(w) + switch { + case strings.Contains(r.URL.Path, "/git/ref/"): + http.NotFound(w, r) // no state ref yet: the first write creates it + case strings.HasSuffix(r.URL.Path, "/git/blobs"): + var payload struct { + Content string `json:"content"` + } + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + blobs = append(blobs, payload.Content) + enc.Encode(map[string]string{"sha": "blobsha"}) + case strings.HasSuffix(r.URL.Path, "/git/trees"): + enc.Encode(map[string]string{"sha": "treesha"}) + case strings.HasSuffix(r.URL.Path, "/git/commits"): + enc.Encode(map[string]string{"sha": "commitsha"}) + case strings.HasSuffix(r.URL.Path, "/git/refs"): + enc.Encode(map[string]any{"object": map[string]string{"sha": "commitsha"}}) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + + startup := StoreConfig{ + GateRepo: "owner/state", StateRef: "crq-state-v3", DashboardIssue: 1, + Scope: []string{"owner"}, CoReviewers: "codex (selfheal)", + } + store := NewGitStateStore(startup, gh.NewTestClient(srv.URL, srv.Client()), nil) + store.SetRenderConfig(func(State) StoreConfig { + effective := startup + effective.CoReviewers = "codex (required, always)" + return effective + }) + if _, err := store.Update(context.Background(), func(st *State) error { + _, err := st.NewRound("owner/repo", 7, "abcdef123", t0) + return err + }); err != nil { + t.Fatal(err) + } + + written := strings.Join(blobs, "\n") + if !strings.Contains(written, "codex (required, always)") || strings.Contains(written, "codex (selfheal)") { + t.Fatalf("the committed dashboard did not use the effective reviewer summary:\n%s", written) + } +} diff --git a/internal/state/dashboard_test.go b/internal/state/dashboard_test.go index 107b8761..17e50815 100644 --- a/internal/state/dashboard_test.go +++ b/internal/state/dashboard_test.go @@ -933,6 +933,19 @@ func TestStatusLine(t *testing.T) { if got := StatusLine(ready, StoreConfig{}); !strings.Contains(got, "next #7") { t.Errorf("a ready queue should name what is next, got %q", got) } + fleetPaced := stateWith(queuedRound("kristofferr/a", 7, 1, now)) + fleetPaced.LastFired = &now + fleetPaced.SetFleetValue("min-interval", "1h") + if got := StatusLine(fleetPaced, StoreConfig{MinInterval: 0}); strings.Contains(got, "next #7") { + t.Errorf("fleet pacing must keep status from claiming the round is ready: %q", got) + } + // The recorded value is whatever an operator typed, and the parser that + // accepted it at `crq config set` trims. One that queue decisions honour must + // not leave this renderer quietly falling back to the host's interval. + fleetPaced.SetFleetValue("min-interval", " 1h ") + if got := StatusLine(fleetPaced, StoreConfig{MinInterval: 0}); strings.Contains(got, "next #7") { + t.Errorf("a padded fleet interval must pace status like an unpadded one: %q", got) + } // Blocked: the countdown is the useful part. blockedState := stateWith(queuedRound("kristofferr/a", 7, 1, now)) diff --git a/internal/state/fleet.go b/internal/state/fleet.go new file mode 100644 index 00000000..1e15789f --- /dev/null +++ b/internal/state/fleet.go @@ -0,0 +1,76 @@ +package state + +import ( + "sort" + "strings" + "time" +) + +// Fleet is the policy every host in the fleet shares, keyed by setting name. +// +// It lives HERE for the reason the per-repository overrides do, one level up: +// a daemon on one machine and an agent on another reading different +// configurations while writing one shared state ref is a class of divergence +// worth not having. Per-host environment files diverge the moment somebody +// edits one — a repository excluded on the laptop and reviewed by the server, +// a rate-limit window one host respects and another does not — and nothing +// says so, because each host is behaving correctly according to what it can +// see. +// +// A flat map rather than a struct, on purpose. One JSON member means an older +// binary round-trips the whole thing rather than dropping settings it does not +// know. Newly interpreted decision-changing keys still require a writer +// capability bump so an active older driver cannot ignore them. +type Fleet map[string]string + +// FleetValue returns a fleet setting and whether it is recorded. Absent means +// "the fleet has no opinion", which is what lets a host fall back to its own +// environment and to crq's defaults. +func (s *State) FleetValue(key string) (string, bool) { + value, ok := s.FleetConfig[key] + return value, ok +} + +// SetFleetValue records a fleet setting, replacing any earlier value. +func (s *State) SetFleetValue(key, value string) { + if s.FleetConfig == nil { + s.FleetConfig = Fleet{} + } + s.FleetConfig[key] = value +} + +// UnsetFleetValue drops a setting, returning the fleet to per-host defaults for +// it, and reports whether one was there. +func (s *State) UnsetFleetValue(key string) bool { + if _, ok := s.FleetConfig[key]; !ok { + return false + } + delete(s.FleetConfig, key) + return true +} + +// FleetKeys lists the recorded settings in a stable order. +func (s *State) FleetKeys() []string { + keys := make([]string, 0, len(s.FleetConfig)) + for key := range s.FleetConfig { + keys = append(keys, key) + } + sort.Strings(keys) + return keys +} + +// withFleet applies the state-backed settings the state package itself renders. +// The full policy is interpreted by crq; MinInterval also shapes queue +// readiness in dashboards/status, including GitStateStore's background sync. +// +// Trimmed the way crq's own fleet parser trims, because the recorded value is +// whatever an operator typed: a stored " 5m " that queue decisions honour must +// not leave the dashboard and status line advertising this host's interval. +func (c StoreConfig) withFleet(s State) StoreConfig { + if value, ok := s.FleetValue("min-interval"); ok { + if interval, err := time.ParseDuration(strings.TrimSpace(value)); err == nil && interval >= 0 { + c.MinInterval = interval + } + } + return c +} diff --git a/internal/state/holdview_test.go b/internal/state/holdview_test.go index ed57506b..2aa88ba6 100644 --- a/internal/state/holdview_test.go +++ b/internal/state/holdview_test.go @@ -101,6 +101,13 @@ func TestCoReviewerRowSaysItIsTheDefaultAndNamesExceptions(t *testing.T) { t.Error("the row does not say the list is a default") } + // A required-reviewer override does not change the co-reviewer list. + st.SetRepoOverride("owner/required-only", RepoReviewers{SetRequired: true, UpdatedAt: &now}) + body = RenderDashboard(st, cfg) + if strings.Contains(body, "owner/required-only") { + t.Errorf("the co-reviewer row names an unrelated override:\n%s", body) + } + // Once a repository overrides it, the row names which. st.SetRepoOverride("owner/special", RepoReviewers{SetCoBots: true, UpdatedAt: &now}) body = RenderDashboard(st, cfg) @@ -111,6 +118,13 @@ func TestCoReviewerRowSaysItIsTheDefaultAndNamesExceptions(t *testing.T) { t.Error("the row does not say the named repository overrides the default") } + // An explicit repository answer still needs a row when the fleet default is empty. + withoutDefault := RenderDashboard(st, StoreConfig{}) + if !strings.Contains(withoutDefault, "**Co-reviewers**") || + !strings.Contains(withoutDefault, "owner/special") { + t.Errorf("an override with an empty fleet default is hidden:\n%s", withoutDefault) + } + // Many overrides are truncated rather than allowed to wrap the table. for _, repo := range []string{"owner/a", "owner/b", "owner/c", "owner/d"} { st.SetRepoOverride(repo, RepoReviewers{SetCoBots: true, UpdatedAt: &now}) @@ -119,4 +133,7 @@ func TestCoReviewerRowSaysItIsTheDefaultAndNamesExceptions(t *testing.T) { if !strings.Contains(body, "+2 more") { t.Errorf("five overrides were not summarised:\n%s", body) } + if !strings.Contains(body, "overrides") { + t.Errorf("multiple repositories were labelled as one override:\n%s", body) + } } diff --git a/internal/state/state.go b/internal/state/state.go index e936989b..50b6105e 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -1,4 +1,4 @@ -// Package state defines crq's persisted schema v4: one Round per tracked PR, +// Package state defines crq's persisted schema v5: one Round per tracked PR, // a single global fire slot, and the CodeRabbit account quota. A Round is // never deleted, only transitioned (or archived when superseded by a new // head) — the invariant that makes "forgot we already requested a review at @@ -367,10 +367,10 @@ func (l LeaderCapabilityLease) HasCapability(want string) bool { return false } -// State is schema v4. It persists as state.json in the existing git state ref; -// v3 is migrated in place so its live rounds survive the compatibility fence. +// State is schema v5. It persists as state.json in the existing git state ref; +// v4 is migrated in place so its live rounds survive the compatibility fence. type State struct { - Version int `json:"v"` // 4 + Version int `json:"v"` // 5 Rev int64 `json:"rev"` NextSeq int64 `json:"next_seq"` @@ -399,13 +399,6 @@ type State struct { // one. Persisted in the shared state so the whole fleet uses the new issue. CalibrationIssue int `json:"calibration_issue,omitempty"` - // Holds are the PRs crq must not fire a review for, keyed by "owner/name#pr". - // - // Holding used to take two commands that could not be one: the skip marker - // stops fleet auto-review from enqueueing, `crq cancel` stops the pump, and - // between the two a daemon fired anyway. A hold is one fact, in the state - // every firing path already reads. - Holds map[string]Hold `json:"holds,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 @@ -423,6 +416,18 @@ type State struct { // read this ref, so both cannot disagree about it. Repos map[string]RepoReviewers `json:"repos,omitempty"` + // FleetConfig is the policy every host shares — which repositories are in + // scope, who reviews, and the timings the queue paces itself by. Same + // argument as Repos above, one level up: hosts that each carry their own + // answer diverge silently. See fleet.go. + FleetConfig Fleet `json:"fleet,omitempty"` + // Holds are the PRs crq must not fire a review for, keyed by "owner/name#pr". + // + // Holding used to take two commands that could not be one: the skip marker + // stops fleet auto-review from enqueueing, `crq cancel` stops the pump, and + // between the two a daemon fired anyway. A hold is one fact, in the state + // every firing path already reads. + Holds map[string]Hold `json:"holds,omitempty"` // Archive keeps recently finished rounds (superseded, closed, cancelled) // for the dashboard and debugging. Bounded by ArchiveMax. Archive []Round `json:"archive,omitempty"` @@ -476,16 +481,20 @@ func (s State) LeaderHasCapability(want string) bool { s.LeaderCapabilities.HasCapability(want) } -const SchemaVersion = 4 +const SchemaVersion = 5 // 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 +const WriterCaps = 3 // CapsRepoOverrides is the capability that makes per-repository reviewer // overrides safe to act on. const CapsRepoOverrides = 1 +// CapsFleetPolicy is the capability that makes state-backed fleet policy safe +// to activate while a process is driving the queue. +const CapsFleetPolicy = 3 + // writerTTL is how long a host counts as still active for capability purposes. const writerTTL = 30 * time.Minute @@ -524,6 +533,39 @@ func (s *State) NoteWriter(host string, caps int, now time.Time) { // 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 { + var out []string + for host := range s.actingWriters(now) { + 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 +} + +// AdvancedWriters names the hosts driving this queue with a capability HIGHER +// than caps — the fleet running a newer binary than the caller's. +// +// It is the mirror of LaggingWriters, and answers the question an old CLI has to +// ask before it deletes a recorded setting it cannot interpret: is anyone acting +// on it? WriterCaps is bumped exactly when a state field starts changing +// decisions, so a driver announcing more than this binary knows is the one that +// wrote that setting and owns whatever cleanup dropping it needs. +func (s *State) AdvancedWriters(caps int, now time.Time) []string { + var out []string + for host := range s.actingWriters(now) { + if seen, ok := s.Writers[host]; ok && seen.Caps > caps && now.Sub(seen.At) <= writerTTL { + out = append(out, host) + } + } + sort.Strings(out) + return out +} + +// actingWriters names the processes DRIVING this queue: holding the leader lease +// or the fire slot. +func (s *State) actingWriters(now time.Time) map[string]bool { acting := map[string]bool{} // The leader identifies itself as "host= pid= run=", which is // exactly the process identity capabilities are recorded under — the run @@ -534,15 +576,7 @@ func (s *State) LaggingWriters(caps int, now time.Time) []string { 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 + return acting } // RepoReviewers overrides which reviewers run on one repository. A nil slice diff --git a/internal/state/statusline.go b/internal/state/statusline.go index 8058b5f9..211d37b2 100644 --- a/internal/state/statusline.go +++ b/internal/state/statusline.go @@ -16,6 +16,7 @@ import ( // Everything here is already computed for the dashboard; this is a second // rendering of the same reduced state, not new logic. func StatusLine(st State, cfg StoreConfig) string { + cfg = cfg.withFleet(st) now := time.Now().UTC() queue := st.Queue(now, cfg.MinInterval) inFlight := inFlightRounds(st) diff --git a/internal/state/store.go b/internal/state/store.go index 2237303b..31fb9d55 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -81,13 +81,13 @@ type Revision struct { type StateStore interface { Load(context.Context) (State, Revision, error) Update(context.Context, func(*State) error) (State, error) - SyncDashboard(context.Context, State) error + SyncDashboard(context.Context, State, StoreConfig) error } -// GitStateStore persists v4 state as state.json in a git ref, with the same -// compare-and-swap mechanism as v3 (12 retries on UpdateRef 409/422). +// GitStateStore persists v5 state as state.json in a git ref, with the same +// compare-and-swap mechanism as v4 (12 retries on UpdateRef 409/422). // -// V3 is migrated in place; still older payloads are discarded because crq is +// V4 is migrated in place; still older payloads are discarded because crq is // pre-release and they describe a world this binary cannot act on. A NEWER one // is refused. The fleet runs mixed binary versions during a rolling deploy, so // reinitializing there would mean the first old binary to wake up erases every @@ -98,6 +98,15 @@ type GitStateStore struct { gh *gh.GitHub log Logger + // renderCfg resolves the configuration a dashboard render should use for a + // given state, defaulting to this process's own. crq replaces it with one + // that applies fleet policy: dashboard.md is rewritten on EVERY state + // commit, so rendering it from startup configuration would have each host + // write its own reviewer set into the shared file — flapping between + // machines, and disagreeing with the gate issue, which SyncDashboard already + // renders from the effective policy. + renderCfg func(State) StoreConfig + // syncMu serializes the read-then-write in SyncDashboard, so concurrent // syncs cannot both see a stale gate issue and both write it. syncMu sync.Mutex @@ -107,6 +116,21 @@ func NewGitStateStore(cfg StoreConfig, client *gh.GitHub, log Logger) *GitStateS return &GitStateStore{cfg: cfg, gh: client, log: log} } +// SetRenderConfig installs the resolver compareAndSwap renders dashboard.md +// with. It is set once, before the store is used: the state package holds no bot +// registry, so only crq can turn recorded fleet policy into a rendering +// configuration. +func (s *GitStateStore) SetRenderConfig(resolve func(State) StoreConfig) { + s.renderCfg = resolve +} + +func (s *GitStateStore) renderConfig(st State) StoreConfig { + if s.renderCfg == nil { + return s.cfg + } + return s.renderCfg(st) +} + func (s *GitStateStore) logf(format string, args ...any) { if s.log != nil { s.log.Printf(format, args...) @@ -149,9 +173,9 @@ func (s *GitStateStore) Load(ctx context.Context) (State, Revision, error) { if err != nil { return State{}, Revision{}, err } - // Peek at the schema version before a full decode. V3 is the one supported - // migration: v4 intentionally fences v3 pumping clients from administrative - // holds, while preserving every live v3 round during the rollout. + // Peek at the schema version before a full decode. V4 is the one supported + // migration: v5 intentionally fences v4 pumping clients from fleet policy, + // while preserving every live v4 round during the rollout. var probe struct { Version int `json:"v"` } @@ -228,7 +252,7 @@ func (s *GitStateStore) Update(ctx context.Context, mutate func(*State) error) ( } func (s *GitStateStore) compareAndSwap(ctx context.Context, st *State, rev Revision) error { - dashboard := RenderDashboard(*st, s.cfg) + dashboard := RenderDashboard(*st, s.renderConfig(*st)) st.DashboardSHA = hashString(dashboard) stateJSON, err := json.MarshalIndent(*st, "", " ") if err != nil { @@ -290,15 +314,15 @@ func (s *GitStateStore) compareAndSwap(ctx context.Context, st *State, rev Revis // // A read failure is never fatal: fall through and PATCH, which is the old // behavior. -func (s *GitStateStore) SyncDashboard(ctx context.Context, st State) error { +func (s *GitStateStore) SyncDashboard(ctx context.Context, st State, renderCfg StoreConfig) error { if err := s.cfg.requireDashboard(); err != nil { return err } - body, err := IssueBody(st, s.cfg) + body, err := IssueBody(st, renderCfg) if err != nil { return err } - title := RenderTitle(st, s.cfg) + title := RenderTitle(st, renderCfg) // Held across read-then-write so two concurrent syncs cannot both observe a // stale issue and both write it. @@ -356,7 +380,7 @@ func (m *MemoryStore) Update(_ context.Context, mutate func(*State) error) (Stat return st, nil } -func (m *MemoryStore) SyncDashboard(context.Context, State) error { return nil } +func (m *MemoryStore) SyncDashboard(context.Context, State, StoreConfig) error { return nil } // Clone deep-copies a State via its JSON representation, so a mutate closure // can never scribble on the store's retained copy. diff --git a/internal/state/store_version_test.go b/internal/state/store_version_test.go index edabb503..a6274cbe 100644 --- a/internal/state/store_version_test.go +++ b/internal/state/store_version_test.go @@ -76,9 +76,9 @@ func TestLoadRefusesUndecodableCurrentState(t *testing.T) { } } -func TestLoadMigratesV3WithoutLosingLiveRounds(t *testing.T) { +func TestLoadMigratesV4WithoutLosingLiveRounds(t *testing.T) { payload := `{ - "v":3, + "v":4, "rev":7, "next_seq":2, "rounds":{ @@ -102,21 +102,21 @@ func TestLoadMigratesV3WithoutLosingLiveRounds(t *testing.T) { t.Errorf("version = %d, want migrated v%d", st.Version, SchemaVersion) } if round := st.Round("owner/repo", 7); round == nil || round.Head != "abcdef123" { - t.Fatalf("live v3 round was lost during migration: %+v", round) + t.Fatalf("live v4 round was lost during migration: %+v", round) } encoded, err := json.Marshal(st) if err != nil { t.Fatal(err) } if !strings.Contains(string(encoded), "future_top_level") { - t.Fatalf("unknown v3 state was lost during migration: %s", encoded) + t.Fatalf("unknown v4 state was lost during migration: %s", encoded) } } // An OLDER payload is genuinely obsolete: crq is pre-release, there is no -// migration, and a v2 state describes a world this binary cannot act on. +// migration, and a v3 state describes a world this binary cannot act on. func TestLoadReinitializesOlderState(t *testing.T) { - st, _, err := versionStore(t, `{"v":2,"queue":[{"repo":"owner/repo","pr":7}]}`).Load(context.Background()) + st, _, err := versionStore(t, `{"v":3,"queue":[{"repo":"owner/repo","pr":7}]}`).Load(context.Background()) if err != nil { t.Fatalf("an older schema must reinitialize, got %v", err) } diff --git a/internal/state/tolerant.go b/internal/state/tolerant.go index 8e4309d5..eb04aa04 100644 --- a/internal/state/tolerant.go +++ b/internal/state/tolerant.go @@ -18,8 +18,8 @@ import ( // Unknown members are therefore carried by default. A load keeps whatever it // did not recognise, a save puts it back, and a field this binary has never // heard of survives a foreign write untouched. A schema bump is reserved for a -// compatibility fence: newer state is refused by older binaries, as v4 requires -// so v3 pumping clients cannot ignore administrative holds. +// compatibility fence: newer state is refused by older binaries, as v5 requires +// so v4 pumping clients cannot ignore state-backed fleet policy. // unknownFields holds JSON members this binary has no field for, verbatim. type unknownFields map[string]json.RawMessage diff --git a/internal/state/writers_test.go b/internal/state/writers_test.go index cc816eea..6759cb38 100644 --- a/internal/state/writers_test.go +++ b/internal/state/writers_test.go @@ -5,6 +5,12 @@ import ( "time" ) +func TestFleetPolicyCapabilityAdvancesWithNewDecisionKeys(t *testing.T) { + if WriterCaps != 3 || CapsFleetPolicy != WriterCaps { + t.Fatalf("writer caps = %d, fleet caps = %d; want fleet policy fenced at writer capability 3", WriterCaps, CapsFleetPolicy) + } +} + // 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 @@ -93,6 +99,31 @@ func TestLaggingWritersMatchesTheFireSlotOwner(t *testing.T) { } } +// The mirror question: an old binary about to drop a setting it cannot read has +// to know whether the crq that wrote it is still driving the queue. +func TestAdvancedWritersNamesTheNewerDriver(t *testing.T) { + now := time.Date(2026, 7, 26, 12, 0, 0, 0, time.UTC) + st := New() + st.Leader = &LeaderLease{Owner: "host=new-mac pid=7", ExpiresAt: now.Add(time.Minute)} + + // A leader running this version is not ahead of it. + st.NoteWriter("host=new-mac pid=7", WriterCaps, now) + if got := st.AdvancedWriters(WriterCaps, now); len(got) != 0 { + t.Errorf("advanced = %v, want none for a leader on this version", got) + } + + st.NoteWriter("host=new-mac pid=7", WriterCaps+1, now) + if got := st.AdvancedWriters(WriterCaps, now); len(got) != 1 || got[0] != "host=new-mac pid=7" { + t.Fatalf("advanced = %v, want the newer leader named", got) + } + + // And only while it is driving: a stale stamp says nothing about now. + st.Leader.ExpiresAt = now + if got := st.AdvancedWriters(WriterCaps, now); len(got) != 0 { + t.Errorf("advanced = %v, want none once the lease has lapsed", 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 diff --git a/llms.txt b/llms.txt index 2fd5a680..3ba1a143 100644 --- a/llms.txt +++ b/llms.txt @@ -211,6 +211,21 @@ convergence never hides a rebuttal you have not answered. Address it or decline reason; the loop clears it once the bot withdraws (or you stop declining and fix it). Ambiguous bot replies surface too — crq errs toward showing a possible rebuttal rather than burying it. +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. Run the whole thing unattended — crq watches every open PR and starts a fix session for the ones that need one: @@ -233,7 +248,7 @@ authenticate. crq watch [--no-dispatch] [--once] [--interval 2m] [--concurrency 0] [...] [-- ...] crq autofix # which repositories crq may fix crq autofix off REPO --reason "why" # stop fixing there; watching continues -crq autofix on REPO | crq autofix default REPO +crq autofix on REPO # or: crq autofix default REPO ``` It drives every open PR in `CRQ_REPOS` through the same `crq next` oracle and emits one JSON line diff --git a/skills/coderabbit-queue/SKILL.md b/skills/coderabbit-queue/SKILL.md index 4beaed6b..e3717e19 100644 --- a/skills/coderabbit-queue/SKILL.md +++ b/skills/coderabbit-queue/SKILL.md @@ -182,6 +182,28 @@ 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. ## Unattended Autofix `crq watch` starts a fix session for every PR whose action is `fix` — that is the default — in a worktree crq @@ -266,8 +288,8 @@ 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 -no `thread_id`. `crq resolve` and `crq decline` both act on a thread, so neither can touch them — and a -finding that can never be cleared blocks every future round on that PR. +no `thread_id`, so `crq resolve` and `crq decline` cannot act on them. Once judged, dismiss one for +the current head so it no longer blocks the round: ```bash crq dismiss "$REPO" "$PR" "$FINDING_ID" --reason "why this is being set aside"