diff --git a/kubernetes/apps/review-bot/deployment.yaml b/kubernetes/apps/review-bot/deployment.yaml index 6d8c4d12..83241de9 100644 --- a/kubernetes/apps/review-bot/deployment.yaml +++ b/kubernetes/apps/review-bot/deployment.yaml @@ -34,8 +34,6 @@ spec: - name: review-bot image: gitea.renner.uno/gitea_admin/review-bot:2026-10-27-0757 # {"$imagepolicy": "flux-system:review-bot"} env: - - name: REPOS - value: konairius/homelab,konairius/homeassistant,konairius/authentik-samba-bridge,konairius/shelly-manager - name: GITEA_URL value: https://gitea.renner.uno - name: AGENT_NAME diff --git a/review-bot/config.go b/review-bot/config.go index e8b4b6f2..7302fd0d 100644 --- a/review-bot/config.go +++ b/review-bot/config.go @@ -48,12 +48,14 @@ type Config struct { TypeSafeKey string TypeSafeModel string - Repos []string - Workers int - QueueSize int - Sweep time.Duration - ReviewTimeout time.Duration - ShutdownWait time.Duration + Workers int + QueueSize int + // Sweep is how often the bot looks for requested reviews in every repository it can read. + Sweep time.Duration + // FailureCooldown keeps the sweep from rerunning a failed review of the same head. + FailureCooldown time.Duration + ReviewTimeout time.Duration + ShutdownWait time.Duration // A review that runs into a rate limit or a gateway failure is asked again after a wait. RateLimitRetries int RateLimitWait time.Duration @@ -157,11 +159,12 @@ func loadConfig(getenv func(string) string, readFile func(string) ([]byte, error TypeSafeKey: optional("TYPESAFE_API_KEY"), TypeSafeModel: text("TYPESAFE_MODEL", "jev-latest"), - Workers: number("WORKERS", 1), - QueueSize: number("QUEUE_SIZE", 32), - Sweep: duration("SWEEP_INTERVAL", 30*time.Minute), - ReviewTimeout: duration("REVIEW_TIMEOUT", 25*time.Minute), - ShutdownWait: duration("SHUTDOWN_WAIT", 9*time.Minute), + Workers: number("WORKERS", 1), + QueueSize: number("QUEUE_SIZE", 32), + Sweep: duration("SWEEP_INTERVAL", 5*time.Minute), + FailureCooldown: duration("FAILURE_COOLDOWN", time.Hour), + ReviewTimeout: duration("REVIEW_TIMEOUT", 25*time.Minute), + ShutdownWait: duration("SHUTDOWN_WAIT", 9*time.Minute), RateLimitRetries: number("RATE_LIMIT_RETRIES", 3), RateLimitWait: duration("RATE_LIMIT_WAIT", time.Minute), @@ -204,15 +207,5 @@ func loadConfig(getenv func(string) string, readFile func(string) ([]byte, error errs = append(errs, fmt.Errorf("SELECTOR must be empty or typesafe, got %q", c.Selector)) } - for _, r := range list("REPOS", nil) { - if strings.Count(r, "/") != 1 { - errs = append(errs, fmt.Errorf("REPOS entry %q is not owner/repo", r)) - continue - } - c.Repos = append(c.Repos, r) - } - if len(c.Repos) == 0 { - errs = append(errs, errors.New("REPOS is empty")) - } return c, errors.Join(errs...) } diff --git a/review-bot/config_test.go b/review-bot/config_test.go index edaf4528..3b4ff67b 100644 --- a/review-bot/config_test.go +++ b/review-bot/config_test.go @@ -14,7 +14,7 @@ func noFiles(string) ([]byte, error) { return nil, errors.New("no such file") } func baseEnv() map[string]string { return map[string]string{ "REVIEW_TOKEN": "t", "WEBHOOK_SECRET": "s", "MISTRAL_API_KEY": "k", - "CONNECTOR_ID": "c1", "REPOS": "konairius/homelab, konairius/shelly-manager", + "CONNECTOR_ID": "c1", } } @@ -24,16 +24,14 @@ func TestLoadConfigDefaults(t *testing.T) { t.Fatal(err) } if c.Listen != ":8080" || c.GiteaURL != "https://gitea.renner.uno" || c.BotLogin != "review-bot" || - c.Workers != 1 || c.Sweep != 30*time.Minute || c.RateLimitRetries != 3 || c.RateLimitWait != time.Minute || + c.Workers != 1 || c.Sweep != 5*time.Minute || c.FailureCooldown != time.Hour || + c.RateLimitRetries != 3 || c.RateLimitWait != time.Minute || c.Selector != "" || c.MCPURL != "https://gitea-mcp.renner.uno/mcp" || c.AnthropicEffort != "medium" || c.SelectorMinConfidence != 0.6 { t.Errorf("unexpected defaults: %+v", c) } if len(c.Models) != 1 || c.Models[0].ID() != "mistral:mistral-large-4-0" { t.Errorf("models: %+v", c.Models) } - if len(c.Repos) != 2 || c.Repos[1] != "konairius/shelly-manager" { - t.Errorf("repos: %v", c.Repos) - } } func TestLoadConfigSecretFromFile(t *testing.T) { @@ -47,13 +45,14 @@ func TestLoadConfigSecretFromFile(t *testing.T) { } func TestLoadConfigErrors(t *testing.T) { - _, err := loadConfig(env(map[string]string{"WORKERS": "0", "REPOS": "nonsense", "SWEEP_INTERVAL": "soon", - "SELECTOR": "dice"}), noFiles) + _, err := loadConfig(env(map[string]string{"WORKERS": "0", "SWEEP_INTERVAL": "soon", "FAILURE_COOLDOWN": "never", + "SELECTOR": "dice", "SELECTOR_MIN_CONFIDENCE": "2"}), noFiles) if err == nil { t.Fatal("expected errors") } for _, want := range []string{"REVIEW_TOKEN is not set", "WEBHOOK_SECRET is not set", "no model of MODELS is usable", - "MISTRAL_API_KEY is not set", "WORKERS must be", "SWEEP_INTERVAL must be", `REPOS entry "nonsense"`, "SELECTOR must be"} { + "MISTRAL_API_KEY is not set", "WORKERS must be", "SWEEP_INTERVAL must be", "FAILURE_COOLDOWN must be", + "SELECTOR must be", "SELECTOR_MIN_CONFIDENCE must be"} { if !strings.Contains(err.Error(), want) { t.Errorf("missing %q in: %v", want, err) } diff --git a/review-bot/gitea.go b/review-bot/gitea.go index 2a0bcb93..10beb050 100644 --- a/review-bot/gitea.go +++ b/review-bot/gitea.go @@ -96,10 +96,28 @@ func (g *Gitea) PR(ctx context.Context, slug string, n int) (PR, error) { return pr, err } -func (g *Gitea) OpenPRs(ctx context.Context, slug string) ([]PR, error) { - var prs []PR - err := g.do(ctx, http.MethodGet, fmt.Sprintf("/repos/%s/pulls?state=open&limit=50", slug), nil, &prs) - return prs, err +// Requested lists every open pull request, in any repository, that asks the token's user for a +// review, as (repository, number) pairs. +func (g *Gitea) Requested(ctx context.Context) ([]job, error) { + var out []job + for page := 1; ; page++ { + var issues []struct { + Number int `json:"number"` + Repository struct { + FullName string `json:"full_name"` + } `json:"repository"` + } + path := fmt.Sprintf("/repos/issues/search?type=pulls&state=open&review_requested=true&limit=50&page=%d", page) + if err := g.do(ctx, http.MethodGet, path, nil, &issues); err != nil { + return out, err + } + for _, is := range issues { + out = append(out, job{slug: is.Repository.FullName, num: is.Number}) + } + if len(issues) < 50 { + return out, nil + } + } } func (g *Gitea) Reviews(ctx context.Context, slug string, n int) ([]Review, error) { diff --git a/review-bot/main.go b/review-bot/main.go index 2554e551..983b926e 100644 --- a/review-bot/main.go +++ b/review-bot/main.go @@ -75,7 +75,7 @@ func run(ctx context.Context, cfg Config, log *slog.Logger) error { } failed := make(chan error, 1) go func() { failed <- srv.ListenAndServe() }() - log.Info("listening", "addr", cfg.Listen, "repos", cfg.Repos) + log.Info("listening", "addr", cfg.Listen, "sweep", cfg.Sweep.String()) select { case <-ctx.Done(): diff --git a/review-bot/review.go b/review-bot/review.go index 8f9bb8be..c23e6fbd 100644 --- a/review-bot/review.go +++ b/review-bot/review.go @@ -354,25 +354,33 @@ func (q *Queue) Stop(wait time.Duration) { } } -// Sweep queues every open PR of the configured repositories that has no review at its head -// commit. It is the safety net for webhook deliveries that were lost. +// Sweep queues every open PR, in any repository, that requests a review from the bot and needs +// one. It reviews repositories without a webhook and catches lost deliveries. A head whose review +// failed is left alone for FailureCooldown, so a failing review is not rerun on every sweep. func (r *Reviewer) Sweep(ctx context.Context, q *Queue) { - for _, slug := range r.cfg.Repos { - prs, err := r.gitea.OpenPRs(ctx, slug) + defer r.metrics.Inc("sweeps_total") + jobs, err := r.gitea.Requested(ctx) + if err != nil { + r.log.Error("sweep: cannot list requested reviews", "err", err) + } + for _, j := range jobs { + pr, err := r.gitea.PR(ctx, j.slug, j.num) if err != nil { - r.log.Error("sweep: cannot list pull requests", "repo", slug, "err", err) + r.log.Error("sweep: cannot read the pull request", "pr", key(j.slug, j.num), "err", err) continue } - for _, pr := range prs { - reviews, err := r.gitea.Reviews(ctx, slug, pr.Number) - if err != nil { - r.log.Error("sweep: cannot read reviews", "pr", key(slug, pr.Number), "err", err) - continue - } - if eligible(pr, reviews, r.cfg.BotLogin) == "ok" { - r.log.Info("sweep: queued", "pr", key(slug, pr.Number), "result", q.Enqueue(slug, pr.Number, pr.Head.SHA)) - } + reviews, err := r.gitea.Reviews(ctx, j.slug, j.num) + if err != nil { + r.log.Error("sweep: cannot read reviews", "pr", key(j.slug, j.num), "err", err) + continue } + if eligible(pr, reviews, r.cfg.BotLogin) != "ok" { + continue + } + if st, ok := r.status.Get(j.slug, j.num); ok && st.Phase == PhaseFailed && st.Head == pr.Head.SHA && + time.Since(st.Since) < r.cfg.FailureCooldown { + continue + } + r.log.Info("sweep: queued", "pr", key(j.slug, j.num), "result", q.Enqueue(j.slug, j.num, pr.Head.SHA)) } - r.metrics.Inc("sweeps_total") } diff --git a/review-bot/review_test.go b/review-bot/review_test.go index d5528663..69625b1f 100644 --- a/review-bot/review_test.go +++ b/review-bot/review_test.go @@ -118,8 +118,17 @@ func newFixture(t *testing.T) *fixture { _, _ = io.WriteString(w, prJSON) case r.Method == "GET" && r.URL.Path == "/api/v1/repos/konairius/homelab/pulls/7/reviews": _, _ = io.WriteString(w, f.reviews) - case r.Method == "GET" && r.URL.Path == "/api/v1/repos/konairius/homelab/pulls": - _, _ = io.WriteString(w, "["+prJSON+"]") + case r.Method == "GET" && r.URL.Path == "/api/v1/repos/issues/search": + q := r.URL.Query() + if q.Get("type") != "pulls" || q.Get("state") != "open" || q.Get("review_requested") != "true" { + http.Error(w, "unexpected search", http.StatusBadRequest) + return + } + if !f.requested || q.Get("page") != "1" { + _, _ = io.WriteString(w, "[]") + return + } + _, _ = io.WriteString(w, `[{"number":7,"repository":{"full_name":"konairius/homelab"}}]`) case r.Method == "POST" && r.URL.Path == "/api/v1/repos/konairius/homelab/pulls/7/reviews": var in map[string]string _ = json.NewDecoder(r.Body).Decode(&in) @@ -203,7 +212,7 @@ func newFixture(t *testing.T) *fixture { GiteaURL: f.gitea.URL, BotLogin: "review-bot", ReviewToken: "review-token", WebhookSecret: "hook-secret", MistralURL: f.mistral.URL, MistralKey: "mistral-key", AgentName: "pr-review", Models: []Model{{Backend: "mistral", Name: "mistral-large-4-0"}}, - ConnectorID: "c1", Repos: []string{"konairius/homelab"}, Workers: 1, QueueSize: 4, + ConnectorID: "c1", Workers: 1, QueueSize: 4, FailureCooldown: time.Hour, ReviewTimeout: 5 * time.Second, RateLimitRetries: 2, RateLimitWait: time.Millisecond, } f.metrics = newMetrics() @@ -473,7 +482,7 @@ func TestHook(t *testing.T) { {"opened alone starts nothing", "pull_request", openedSig, openedBody, 200}, {"a push alone starts nothing", "pull_request", syncSig, syncBody, 200}, {"review requested from someone else", "pull_request", otherSig, otherBody, 200}, - {"repository that is not configured", "pull_request", repoSig, repoBody, 200}, + {"any repository", "pull_request", repoSig, repoBody, 202}, {"payload without a number", "pull_request", sign("hook-secret", []byte(noNum)), noNum, 400}, {"payload that is not json", "pull_request", sign("hook-secret", []byte("nope")), "nope", 400}, } @@ -523,6 +532,26 @@ func TestSweepQueuesOnlyWhatNeedsAReview(t *testing.T) { } } +// A head whose review just failed is not rerun by the sweep; a new head is. +func TestSweepLeavesARecentFailureAlone(t *testing.T) { + f := newFixture(t) + f.rev.status.Set("konairius/homelab", 7, "abc123", PhaseFailed, "boom", "") + q := newQueue(f.rev, 4, f.rev.log, f.metrics) + f.rev.Sweep(context.Background(), q) + if len(q.jobs) != 0 { + t.Fatalf("queued %d right after a failure, want 0", len(q.jobs)) + } + + f.mu.Lock() + f.head = "def456" + f.mu.Unlock() + q2 := newQueue(f.rev, 4, f.rev.log, f.metrics) + f.rev.Sweep(context.Background(), q2) + if len(q2.jobs) != 1 { + t.Errorf("queued %d for a new head after a failure, want 1", len(q2.jobs)) + } +} + func TestHealthEndpoints(t *testing.T) { f := newFixture(t) s := &Server{cfg: f.cfg, queue: newQueue(f.rev, 1, f.rev.log, f.metrics), reviewer: f.rev, metrics: f.metrics, log: f.rev.log} diff --git a/review-bot/server.go b/review-bot/server.go index 6460595f..279e8a65 100644 --- a/review-bot/server.go +++ b/review-bot/server.go @@ -8,7 +8,6 @@ import ( "io" "log/slog" "net/http" - "slices" "strconv" ) @@ -136,10 +135,6 @@ func (s *Server) hook(w http.ResponseWriter, r *http.Request) { http.Error(w, "no repository or pull request number", http.StatusBadRequest) return } - if !slices.Contains(s.cfg.Repos, slug) { - ignore("repository " + slug + " is not configured") - return - } switch res := s.queue.Enqueue(slug, n, p.PullRequest.Head.SHA); res { case "full": s.metrics.Inc("webhooks_total", "result", "dropped")