diff --git a/README.md b/README.md index bcba3ec..5e824fd 100644 --- a/README.md +++ b/README.md @@ -384,7 +384,8 @@ Set these in `~/.config/crq/env` (sourced automatically) or as environment varia | `CRQ_STATE_REF` | `crq-state` | git ref that stores the typed CAS state | | `CRQ_REPOS` | _(all in scope)_ | `autoreview` allowlist — only these `owner/name` repos (comma-separated) | | `CRQ_EXCLUDE` | _(none)_ | `autoreview` denylist — never these `owner/name` repos (comma-separated) | -| `CRQ_REQUIRED_BOTS` | `coderabbitai[bot],chatgpt-codex-connector[bot]` | bots that must review the head for convergence | +| `CRQ_REQUIRED_BOTS` | `coderabbitai[bot]` | bots that must review the head for convergence (crq waits for all of them) | +| `CRQ_FEEDBACK_BOTS` | required bots + `chatgpt-codex-connector[bot]` | bots whose findings are surfaced — a superset of required bots, so Codex reviews show up without gating convergence on repos where Codex isn't installed | | `CRQ_TZ` | `UTC` | dashboard display timezone (IANA name, e.g. `Europe/Oslo`) | | `CRQ_MIN_INTERVAL` | `90s` | minimum time between fired reviews | | `CRQ_POLL` | `15s` | how often `crq loop` checks its place in line | diff --git a/internal/crq/config.go b/internal/crq/config.go index 666031a..cb7ca36 100644 --- a/internal/crq/config.go +++ b/internal/crq/config.go @@ -22,6 +22,7 @@ type Config struct { StateRef string Bot string RequiredBots []string + FeedbackBots []string ReviewCommand string RateLimitCommand string RateLimitMarker string @@ -70,6 +71,7 @@ func LoadConfig() (Config, error) { host, _ := os.Hostname() bot := stringEnv(env, "CRQ_BOT", "coderabbitai[bot]") + requiredBots := listEnv(env, "CRQ_REQUIRED_BOTS", bot) cfg := Config{ GateRepo: env["CRQ_REPO"], DashboardIssue: intEnv(env, "CRQ_ISSUE", 0), @@ -79,7 +81,8 @@ func LoadConfig() (Config, error) { ExcludeRepos: repoSet(env["CRQ_EXCLUDE"]), StateRef: stringEnv(env, "CRQ_STATE_REF", "crq-state"), Bot: bot, - RequiredBots: listEnv(env, "CRQ_REQUIRED_BOTS", bot), + RequiredBots: requiredBots, + FeedbackBots: listEnv(env, "CRQ_FEEDBACK_BOTS", strings.Join(unionBots(requiredBots, extraFeedbackBots), ",")), ReviewCommand: stringEnv(env, "CRQ_REVIEW_CMD", "@coderabbitai review"), RateLimitCommand: stringEnv(env, "CRQ_RATELIMIT_CMD", "@coderabbitai rate limit"), RateLimitMarker: stringEnv(env, "CRQ_RL_MARKER", "rate limited by coderabbit.ai"), @@ -204,6 +207,38 @@ func listEnv(env map[string]string, key, fallback string) []string { return out } +// extraFeedbackBots are review bots whose findings crq surfaces on top of the +// required bots, without gating convergence on them. Codex is the motivating +// case: it reviews but isn't "required" (crq neither fires nor waits for it), so +// its findings would otherwise be silently dropped. This is deliberately just +// Codex — CodeRabbit (or any configured reviewer) already enters the feedback +// set via RequiredBots, so listing it here too would wrongly surface CodeRabbit +// findings even when crq is configured for a different reviewer. +var extraFeedbackBots = []string{"chatgpt-codex-connector[bot]"} + +// unionBots concatenates bot lists, dropping blanks and case-insensitively +// de-duplicating on the normalized login (so "coderabbitai" and +// "coderabbitai[bot]" collapse to one), preserving first-seen order. +func unionBots(lists ...[]string) []string { + seen := map[string]bool{} + var out []string + for _, list := range lists { + for _, item := range list { + item = strings.TrimSpace(item) + if item == "" { + continue + } + key := normalizeBotName(item) + if seen[key] { + continue + } + seen[key] = true + out = append(out, item) + } + } + return out +} + func repoSet(value string) map[string]bool { set := map[string]bool{} for _, item := range strings.Split(value, ",") { diff --git a/internal/crq/config_test.go b/internal/crq/config_test.go index 782f1d6..6e46e2c 100644 --- a/internal/crq/config_test.go +++ b/internal/crq/config_test.go @@ -18,6 +18,87 @@ func TestLoadConfigDefaultsToCodeRabbitRequiredBot(t *testing.T) { } } +func TestLoadConfigFeedbackBotsIncludeCodexByDefault(t *testing.T) { + t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) + t.Setenv("CRQ_REQUIRED_BOTS", "") + t.Setenv("CRQ_FEEDBACK_BOTS", "") + + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + // RequiredBots (convergence gate) stays CodeRabbit-only, but FeedbackBots + // (finding extraction) must also include Codex so its reviews aren't dropped. + has := func(list []string, want string) bool { + for _, b := range list { + if b == want { + return true + } + } + return false + } + if has(cfg.RequiredBots, "chatgpt-codex-connector[bot]") { + t.Fatalf("Codex must not be a required (convergence-gating) bot, got %#v", cfg.RequiredBots) + } + if !has(cfg.FeedbackBots, "coderabbitai[bot]") || !has(cfg.FeedbackBots, "chatgpt-codex-connector[bot]") { + t.Fatalf("feedback bots should include CodeRabbit and Codex by default, got %#v", cfg.FeedbackBots) + } +} + +func TestLoadConfigFeedbackBotsExcludesCodeRabbitForCustomReviewer(t *testing.T) { + // A crq configured for a different reviewer must not surface CodeRabbit + // findings — crq neither fires nor waits for CodeRabbit in that setup. + t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) + t.Setenv("CRQ_BOT", "custom-review-bot") + t.Setenv("CRQ_REQUIRED_BOTS", "") + t.Setenv("CRQ_FEEDBACK_BOTS", "") + + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + for _, b := range cfg.FeedbackBots { + if b == "coderabbitai[bot]" { + t.Fatalf("custom-reviewer feedback bots must not include CodeRabbit, got %#v", cfg.FeedbackBots) + } + } + has := false + for _, b := range cfg.FeedbackBots { + if b == "custom-review-bot" { + has = true + } + } + if !has { + t.Fatalf("feedback bots should include the configured reviewer, got %#v", cfg.FeedbackBots) + } +} + +func TestLoadConfigFeedbackBotsOverride(t *testing.T) { + t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) + t.Setenv("CRQ_FEEDBACK_BOTS", "only-this[bot]") + + cfg, err := LoadConfig() + if err != nil { + t.Fatal(err) + } + if len(cfg.FeedbackBots) != 1 || cfg.FeedbackBots[0] != "only-this[bot]" { + t.Fatalf("CRQ_FEEDBACK_BOTS should override the default, got %#v", cfg.FeedbackBots) + } +} + +func TestUnionBotsDedupesAndPreservesOrder(t *testing.T) { + got := unionBots([]string{"coderabbitai[bot]", ""}, []string{"coderabbitai", "chatgpt-codex-connector[bot]"}) + want := []string{"coderabbitai[bot]", "chatgpt-codex-connector[bot]"} + if len(got) != len(want) { + t.Fatalf("unionBots length mismatch: got %#v want %#v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("unionBots[%d] = %q, want %q (full %#v)", i, got[i], want[i], got) + } + } +} + func TestLoadConfigDefaultRequiredBotFollowsCustomBot(t *testing.T) { t.Setenv("CRQ_CONFIG", filepath.Join(t.TempDir(), "missing-env")) t.Setenv("CRQ_BOT", "custom-review-bot") diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index d7d5980..807af46 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -61,8 +61,16 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe Findings: []Finding{}, CheckedAt: time.Now().UTC(), } - bots := botSet(s.cfg.RequiredBots) - for bot := range bots { + // Two bot sets with different jobs. requiredBots gates convergence: crq isn't + // "done" until every one has reviewed the head, so only these seed ReviewedBy. + // extractBots is the broader set whose findings we surface — a superset that + // includes Codex — so a bot that reviews without being required (and would hang + // convergence if it were) still has its findings reported instead of dropped. + requiredBots := botSet(s.cfg.RequiredBots) + // Always extract from the required bots too: a bot crq waits for whose findings + // it didn't surface would hang the loop forever. FeedbackBots only widens this. + extractBots := botSet(unionBots(s.cfg.FeedbackBots, s.cfg.RequiredBots)) + for bot := range requiredBots { report.ReviewedBy[bot] = false } @@ -71,10 +79,12 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe return report, err } for _, review := range reviews { - if !inBots(bots, review.User.Login) { + if !inBots(extractBots, review.User.Login) { continue } if head != "" && strings.HasPrefix(review.CommitID, head) { + // markReviewed only flips existing ReviewedBy keys (required bots), so a + // non-required extract bot reviewing here is a harmless no-op. markReviewed(report.ReviewedBy, review.User.Login) } if head != "" && review.CommitID != "" && !strings.HasPrefix(review.CommitID, head) { @@ -86,13 +96,13 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe suppressPromptAt := map[string]bool{} if threads, err := s.reviewThreads(ctx, repo, pr); err == nil { for _, thread := range threads { - report.Findings = append(report.Findings, threadFindings(thread, bots)...) + report.Findings = append(report.Findings, threadFindings(thread, extractBots)...) // A resolved/outdated inline thread emits no finding, but CodeRabbit's // "Prompt for AI agents" block still lists the same location. Record it so // the prompt duplicate is suppressed too — otherwise an addressed finding // reappears as a thread-less prompt finding and the loop never converges. if thread.IsResolved || thread.IsOutdated { - for _, key := range promptSuppressKeys(thread, bots) { + for _, key := range promptSuppressKeys(thread, extractBots) { suppressPromptAt[key] = true } } @@ -109,7 +119,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe return report, cerr } for _, comment := range comments { - if !inBots(bots, comment.User.Login) { + if !inBots(extractBots, comment.User.Login) { continue } commit := shortOID(firstNonEmpty(comment.CommitID, comment.OriginalCommitID)) @@ -158,7 +168,7 @@ func (s *Service) Feedback(ctx context.Context, repo string, pr int) (FeedbackRe return headCutoff } for _, comment := range issueComments { - if !inBots(bots, comment.User.Login) { + if !inBots(extractBots, comment.User.Login) { continue } if s.isRateLimited(comment.Body) { diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index b7494c8..64db0af 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -69,6 +69,58 @@ func TestFeedbackBoundsIssueCommentsToHead(t *testing.T) { } } +func TestFeedbackSurfacesCodexEvenWhenNotRequired(t *testing.T) { + // Regression: Codex (chatgpt-codex-connector) reviews a PR and posts inline + // findings, but it isn't in RequiredBots (which is CodeRabbit-only by default). + // crq must still surface Codex's findings — and must NOT falsely converge just + // because CodeRabbit reviewed clean — while not waiting on Codex to converge. + cfg := Config{ + Bot: "coderabbitai[bot]", + RequiredBots: []string{"coderabbitai[bot]"}, + FeedbackBots: unionBots([]string{"coderabbitai[bot]"}, extraFeedbackBots), + } + gh := newFakeGitHub() + sha := "abcdef1234567890" + var pull Pull + pull.State = "open" + pull.Head.SHA = sha + gh.pulls[fakeKey("o/repo", 7)] = pull + + // CodeRabbit reviewed this head and found nothing (empty body → no findings). + crReview := Review{ID: 1, Body: "", CommitID: sha, SubmittedAt: time.Now().UTC()} + crReview.User.Login = "coderabbitai[bot]" + gh.reviews[fakeKey("o/repo", 7)] = []Review{crReview} + + // Codex left an inline finding on the same head (REST review-comment path, since + // the fake GraphQL is unavailable and Feedback falls back to it). + cx := ReviewComment{ID: 22, Body: "**Fix the off-by-one.** This clips the last row.", Path: "app/x.go", Line: 10, CommitID: sha} + cx.User.Login = "chatgpt-codex-connector[bot]" + gh.reviewComments[fakeKey("o/repo", 7)] = []ReviewComment{cx} + + svc := NewService(cfg, gh, NewMemoryStore(cfg), nil) + rep, err := svc.Feedback(context.Background(), "o/repo", 7) + if err != nil { + t.Fatal(err) + } + if len(rep.Findings) != 1 || !strings.Contains(rep.Findings[0].Body, "off-by-one") { + t.Fatalf("expected the Codex finding to be surfaced, got %#v", rep.Findings) + } + if normalizeBotName(rep.Findings[0].Bot) != "chatgpt-codex-connector" { + t.Fatalf("expected the finding attributed to Codex, got %q", rep.Findings[0].Bot) + } + if rep.Converged { + t.Fatal("must not converge while a Codex finding is open") + } + // Convergence gate stays CodeRabbit-only: Codex must not be tracked in ReviewedBy + // (otherwise crq would hang on repos where Codex never reviews). + if _, tracked := rep.ReviewedBy["chatgpt-codex-connector[bot]"]; tracked { + t.Fatalf("Codex must not gate convergence, ReviewedBy=%#v", rep.ReviewedBy) + } + if reviewed, ok := rep.ReviewedBy["coderabbitai[bot]"]; !ok || !reviewed { + t.Fatalf("CodeRabbit should be marked reviewed, ReviewedBy=%#v", rep.ReviewedBy) + } +} + func TestParseReviewBodyFindingsExtractsOutsideDiffItems(t *testing.T) { review := Review{ ID: 99, diff --git a/internal/crq/service_test.go b/internal/crq/service_test.go index 98c9786..d51e378 100644 --- a/internal/crq/service_test.go +++ b/internal/crq/service_test.go @@ -10,15 +10,16 @@ import ( ) type fakeGitHub struct { - mu sync.Mutex - pulls map[string]Pull - commits map[string]gitCommit - reviews map[string][]Review - comments map[string][]IssueComment - reactions map[int64][]Reaction - posted []string - deleted []int64 - commentID int64 + mu sync.Mutex + pulls map[string]Pull + commits map[string]gitCommit + reviews map[string][]Review + comments map[string][]IssueComment + reviewComments map[string][]ReviewComment + reactions map[int64][]Reaction + posted []string + deleted []int64 + commentID int64 } type failNthUpdateStore struct { @@ -62,11 +63,12 @@ func (retryNoChangeStore) SyncDashboard(context.Context, State) error { return n func newFakeGitHub() *fakeGitHub { return &fakeGitHub{ - pulls: map[string]Pull{}, - commits: map[string]gitCommit{}, - reviews: map[string][]Review{}, - comments: map[string][]IssueComment{}, - reactions: map[int64][]Reaction{}, + pulls: map[string]Pull{}, + commits: map[string]gitCommit{}, + reviews: map[string][]Review{}, + comments: map[string][]IssueComment{}, + reviewComments: map[string][]ReviewComment{}, + reactions: map[int64][]Reaction{}, } } @@ -100,8 +102,10 @@ func (f *fakeGitHub) ListIssueComments(_ context.Context, repo string, pr int) ( return append([]IssueComment(nil), f.comments[fakeKey(repo, pr)]...), nil } -func (f *fakeGitHub) ListReviewComments(context.Context, string, int) ([]ReviewComment, error) { - return nil, nil +func (f *fakeGitHub) ListReviewComments(_ context.Context, repo string, pr int) ([]ReviewComment, error) { + f.mu.Lock() + defer f.mu.Unlock() + return append([]ReviewComment(nil), f.reviewComments[fakeKey(repo, pr)]...), nil } func (f *fakeGitHub) ListCommentReactions(_ context.Context, _ string, id int64) ([]Reaction, error) {