Skip to content

fix: surface Codex findings without gating convergence on it#14

Merged
kristofferR merged 2 commits into
mainfrom
fix/codex-feedback-extraction
Jul 3, 2026
Merged

fix: surface Codex findings without gating convergence on it#14
kristofferR merged 2 commits into
mainfrom
fix/codex-feedback-extraction

Conversation

@kristofferR

@kristofferR kristofferR commented Jul 3, 2026

Copy link
Copy Markdown
Owner

The bug

crq feedback filtered every finding source (reviews, review threads, review comments, issue comments) through RequiredBots, which defaults to coderabbitai[bot] only. So when Codex (chatgpt-codex-connector[bot]) reviewed a PR, its inline findings were silently dropped — crq feedback returned zero findings and crq loop could report converged while Codex had open issues on the PR.

The README even documented CRQ_REQUIRED_BOTS as defaulting to coderabbitai[bot],chatgpt-codex-connector[bot], but the code default was CodeRabbit-only — so Codex was never actually consulted.

Why not just add Codex to RequiredBots

RequiredBots gates convergence: crq loop waits for every required bot to review the head before it can converge. Adding Codex there would make crq hang forever on the many repos where Codex isn't installed and never posts a review.

The fix

Split extraction from the convergence gate:

  • RequiredBots still seeds ReviewedBy and gates convergence — unchanged, CodeRabbit-only by default, so no new hang risk.
  • FeedbackBots (new) is the extraction set: a superset that also includes Codex by default, overridable via CRQ_FEEDBACK_BOTS. Extraction always unions in RequiredBots, so a required bot's findings can never be dropped even in a hand-built config.

An open Codex finding keeps findings > 0, so crq loop correctly stays unconverged until it's resolved — but convergence, once findings hit zero, only waits on the required bots.

Verification

  • Against a live PR with Codex review: crq feedback went from 0 → 7 findings with the fixed binary.
  • New regression tests: Feedback surfaces a Codex finding while keeping Codex out of ReviewedBy; FeedbackBots default includes Codex and honors the CRQ_FEEDBACK_BOTS override; unionBots dedupe/order. Full suite + go vet green, gofmt clean.
  • README env table corrected (CRQ_REQUIRED_BOTS real default + new CRQ_FEEDBACK_BOTS row).

Summary by CodeRabbit

  • New Features
    • Added separate configuration for “required” bots (used for convergence) and “feedback” bots (used for surfacing findings), allowing additional bots’ results without affecting review convergence.
    • Updated the README configuration to document the new bot settings.
  • Bug Fixes
    • Fixed feedback reporting so findings (including non-required bots) are still included correctly.
    • Improved prompt-suppression behavior to consistently prevent duplicate prompts across the expanded bot set.

crq feedback filtered every finding source (reviews, review threads,
review comments, issue comments) through RequiredBots, which defaults to
coderabbitai[bot] only. So when Codex (chatgpt-codex-connector[bot])
reviewed a PR, its findings were silently dropped and crq could report
"converged" while Codex flagged open issues — the README even documented
Codex as a default required bot, which the code never actually did.

Making Codex a *required* bot would fix extraction but hang crq waiting
for Codex to review on repos where it isn't installed. So split the two
concerns:

- RequiredBots still gates convergence (seeds ReviewedBy) — unchanged,
  CodeRabbit-only by default, so no new hang risk.
- New FeedbackBots is the extraction set: a superset that also includes
  Codex by default (override via CRQ_FEEDBACK_BOTS). Extraction always
  unions in RequiredBots so a required bot's findings can never be
  dropped.

Verified against a live PR: crq feedback went from 0 to 7 Codex findings
with the fix. Adds regression coverage (Feedback surfaces Codex while
keeping it out of the convergence gate; FeedbackBots defaults + override;
unionBots dedupe) and corrects the README env table.
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 99427173-4e94-4fb9-a03a-7d3bcdfca70e

📥 Commits

Reviewing files that changed from the base of the PR and between e30a4cb and 15a79b1.

📒 Files selected for processing (3)
  • internal/crq/config.go
  • internal/crq/config_test.go
  • internal/crq/feedback_test.go
📜 Recent review details
🔇 Additional comments (3)
internal/crq/config.go (1)

24-25: LGTM!

Also applies to: 73-110, 210-241

internal/crq/config_test.go (1)

48-74: LGTM!

internal/crq/feedback_test.go (1)

72-123: LGTM!


📝 Walkthrough

Walkthrough

Splits bot configuration into required and feedback sets, updates feedback collection to use the broader extraction set, and adds tests plus README coverage for the new behavior.

Changes

Feedback bot config and extraction

Layer / File(s) Summary
Config schema and unionBots helper
internal/crq/config.go, internal/crq/config_test.go
Adds FeedbackBots []string, computes requiredBots from CRQ_REQUIRED_BOTS, derives default feedback bots with unionBots, and covers defaulting plus override behavior in tests.
Feedback extraction using extractBots
internal/crq/feedback.go, internal/crq/feedback_test.go
Service.Feedback uses requiredBots for ReviewedBy and extractBots for review, thread, comment, and suppression filtering; the new regression test verifies Codex findings surface without changing convergence gating.
Fake GitHub test helper support
internal/crq/service_test.go
Adds in-memory review comment storage and a real ListReviewComments implementation for the new feedback-path tests.
README configuration documentation
README.md
Updates the configuration table to describe the split between required bots and feedback bots.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Service as Service.Feedback
  participant Config as Config
  participant GitHub as GitHub API

  Caller->>Service: Feedback(pr)
  Service->>Config: read RequiredBots, FeedbackBots
  Service->>Service: extractBots = union(FeedbackBots, RequiredBots)
  Service->>GitHub: list reviews/threads/comments
  GitHub-->>Service: results
  Service->>Service: filter findings using extractBots
  Service->>Service: init ReviewedBy using requiredBots only
  Service-->>Caller: report(findings, ReviewedBy)
Loading

Possibly related PRs

  • kristofferR/coderabbit-queue#10: Both PRs modify internal/crq/feedback.go’s feedback aggregation flow to decide which bot signals are surfaced and which affect convergence.

Poem

A rabbit split the bots in two,
Required ones and feedback crew.
Codex whispers, “I can still show,”
While ReviewedBy keeps its own row. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the core change: surfacing Codex findings while keeping convergence gating separate.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/codex-feedback-extraction

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e30a4cb009

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread internal/crq/config.go Outdated
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

The extra-feedback default listed coderabbitai[bot] alongside Codex, so a
crq configured for a different reviewer (custom CRQ_BOT/CRQ_REQUIRED_BOTS)
would still surface CodeRabbit findings and keep crq loop returning
feedback for a bot it neither fires nor waits for. CodeRabbit already
enters FeedbackBots via RequiredBots when it's the configured reviewer, so
the extra default is now just Codex. Adds a regression test.
@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant