feat: multi-model consensus review with parallel execution and finding dedup - #7
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughConfiguration now accepts multiple models and a consensus threshold. The model layer adds a concurrent multi-provider that merges reviewer outputs by thresholded agreement, and tests cover matching, aggregation, orchestration, and JSON parsing behavior. ChangesMulti-model consensus review
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/model/multi.go (1)
15-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a
Providerinterface to makeMultiProvider(and its concurrency) unit-testable.
MultiProviderholds[]*Provider(a concrete struct backed by the genai client). This meansMultiProvider.Review's actual concurrent path can't be exercised with fakes in tests —multi_test.goonly tests the pure helper functions (findingsMatch,mergeResults), leaving the concurrent execution and error-aggregation behavior ofReviewuntested. Extracting a smallReviewer-style interface (Review(ctx, systemPrompt, userPrompt) (*ReviewResult, error)) would let tests inject fakes/mocks forMultiProvider.As per path instructions for
internal/**: "Focus on error handling, context propagation, and interface design."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/multi.go` around lines 15 - 51, MultiProvider is hard to unit-test because it stores concrete *Provider values, which prevents injecting fakes into its concurrent Review path. Introduce a small Reviewer-style interface with the Review method, update MultiProvider to hold a slice of that interface instead of []*Provider, and have NewMultiProvider build the concrete providers behind that abstraction. Keep the existing concurrency and error-handling behavior in MultiProvider.Review, but make it depend on the interface so tests can cover concurrent execution and aggregation with mock reviewers.Source: Path instructions
internal/config/config.go (1)
294-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate comma-split/trim/filter logic between
loadEnvandloadFlags.The exact same "split on
,, trim, drop empties" loop is repeated verbatim forREVIEW_MODELSand--models. Extract a small helper to avoid drift if the parsing logic ever needs to change (e.g. dedup, case-normalization).♻️ Proposed refactor
+// splitAndTrim splits s on sep, trims whitespace from each part, and drops empty entries. +func splitAndTrim(s, sep string) []string { + var result []string + for _, part := range strings.Split(s, sep) { + part = strings.TrimSpace(part) + if part != "" { + result = append(result, part) + } + } + return result +}if v := os.Getenv("REVIEW_MODELS"); v != "" { - var models []string - for _, m := range strings.Split(v, ",") { - m = strings.TrimSpace(m) - if m != "" { - models = append(models, m) - } - } - c.Models = models + c.Models = splitAndTrim(v, ",") }if *models != "" { - var parsed []string - for _, m := range strings.Split(*models, ",") { - m = strings.TrimSpace(m) - if m != "" { - parsed = append(parsed, m) - } - } - c.Models = parsed + c.Models = splitAndTrim(*models, ",") }Also applies to: 376-388
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 294 - 303, The comma-split/trim/filter parsing for REVIEW_MODELS is duplicated in loadEnv and loadFlags, so extract it into a shared helper and use that from both paths. Factor the repeated loop in config.go around loadEnv and loadFlags into a single small parser function (for example, one that handles strings.Split, strings.TrimSpace, and dropping empties) and assign c.Models from that helper so future parsing changes stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/model/multi.go`:
- Around line 91-164: The grouping logic in mergeResults is using g.canonical as
both the match anchor and the display choice, which lets the canonical finding
shift and incorrectly merge distant findings through a chain. Update
mergeResults so each findingGroup keeps a fixed anchor for findingsMatch
comparisons, and use the longest-body Finding only as the canonical/display
result. Ensure findingsMatch continues to compare against the stable anchor
rather than the mutable canonical field.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 294-303: The comma-split/trim/filter parsing for REVIEW_MODELS is
duplicated in loadEnv and loadFlags, so extract it into a shared helper and use
that from both paths. Factor the repeated loop in config.go around loadEnv and
loadFlags into a single small parser function (for example, one that handles
strings.Split, strings.TrimSpace, and dropping empties) and assign c.Models from
that helper so future parsing changes stay consistent.
In `@internal/model/multi.go`:
- Around line 15-51: MultiProvider is hard to unit-test because it stores
concrete *Provider values, which prevents injecting fakes into its concurrent
Review path. Introduce a small Reviewer-style interface with the Review method,
update MultiProvider to hold a slice of that interface instead of []*Provider,
and have NewMultiProvider build the concrete providers behind that abstraction.
Keep the existing concurrency and error-handling behavior in
MultiProvider.Review, but make it depend on the interface so tests can cover
concurrent execution and aggregation with mock reviewers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38498832-851c-404e-a965-28bfc96790cc
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modgo.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (3)
internal/config/config.gointernal/model/multi.gointernal/model/multi_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/model/multi_test.go (1)
205-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertion doesn't verify expected deterministic behavior.
m1.callCount.Load() == 0 && m2.callCount.Load() == 0only fails if neither provider is ever called, which givenMultiProvider.Review's use oferrgroup.Go(each goroutine callsReviewunconditionally before any context check) should never happen — both should deterministically be called exactly once regardless ofm2's error. This makes the check nearly tautological and doesn't guard against a future correctness regression.♻️ Tighten the assertion
- if m1.callCount.Load() == 0 && m2.callCount.Load() == 0 { - t.Error("expected at least one model to be called") - } + if m1.callCount.Load() != 1 { + t.Errorf("m1 called %d times, want 1", m1.callCount.Load()) + } + if m2.callCount.Load() != 1 { + t.Errorf("m2 called %d times, want 1", m2.callCount.Load()) + }As per path instructions,
**/*_test.gofiles should be verified for proper race condition safety and meaningful assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/model/multi_test.go` around lines 205 - 217, The assertion in TestMultiProviderReview_PartialError is too weak and does not verify the deterministic behavior of MultiProvider.Review. Update the test to assert that both mockReviewer instances are invoked exactly once, since ReviewProvider calls are launched unconditionally via errgroup.Go and m2’s error should not prevent m1 from running. Use the existing mockReviewer.callCount checks in the test to make the expectation explicit and meaningful.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/model/multi_test.go`:
- Around line 205-217: The assertion in TestMultiProviderReview_PartialError is
too weak and does not verify the deterministic behavior of MultiProvider.Review.
Update the test to assert that both mockReviewer instances are invoked exactly
once, since ReviewProvider calls are launched unconditionally via errgroup.Go
and m2’s error should not prevent m1 from running. Use the existing
mockReviewer.callCount checks in the test to make the expectation explicit and
meaningful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dd3f164-4be2-4d0d-98bc-97801c05159d
📒 Files selected for processing (4)
internal/config/config.gointernal/model/multi.gointernal/model/multi_test.gointernal/model/parse_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/config/config.go
- internal/model/multi.go
…g dedup Multi-model consensus mode runs 2+ AI models in parallel and only keeps findings that multiple models agree on, reducing false positives. New files: - internal/model/multi.go: MultiProvider with errgroup-based parallel execution, finding dedup (file+line±3+category), consensus threshold Config: - --models flag: comma-separated model list (e.g. gemini-2.5-flash,claude-sonnet-4) - --consensus-threshold flag: min models that must agree (default: 2) - REVIEW_MODELS env var support Wiring: - cmd/code-reviewer/main.go: auto-creates MultiProvider when len(models) > 1 Tests (8 new): - findingsMatch: boundary cases for line proximity and category matching - mergeResults: threshold filtering, all-agree, single model, nil results, summary dedup, high threshold, multi-finding dedup 112 tests total, all passing with -race.
…rehensive tests Bug Fixes: - CRITICAL: Fixed transitive chain in mergeResults — canonical finding's line shift could merge unrelated findings 5+ lines apart. Added stable 'anchor' field that never changes after group creation. - Extracted ReviewProvider interface from []*Provider for testability and correct resource cleanup via Close() New Tests (15 added, 127 total): - Transitive drift regression: L10→L12→L15 chain correctly rejected - MultiProvider.Review concurrent execution with mock providers - Partial model error propagation - MultiProvider.Close verification - parseReviewJSON: markdown fences (json/JSON/bare), surrounding text, nested braces, malformed input, empty string, whitespace wrapping Refactoring: - splitAndTrim helper in config.go (DRYs loadEnv/loadFlags) - NewMultiProviderFromReviewers constructor for test injection Coverage: - internal/model: 62.7% → 73.8% - config: 75.1% → 76.7%
brucearctor
force-pushed
the
feat/multi-model-consensus
branch
from
July 9, 2026 04:22
97cf14f to
bcac196
Compare
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Run 2+ AI models in parallel and only keep findings that multiple models agree on, drastically reducing false positives.
Usage
How it works
errgroupFiles changed
internal/model/multi.gointernal/model/multi_test.gointernal/config/config.gocmd/code-reviewer/main.goTests
112 total, all passing with
-race. 8 new tests for multi-model consensus.Summary by CodeRabbit