fix: surface Codex findings without gating convergence on it

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 |

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, ",") {

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")

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) {

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,

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) {