fix: stabilize consensus and make it provider-agnostic#64
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMulti-provider consensus BYOK
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConsensusRoute
participant KeyResolver
participant Provider
Client->>ConsensusRoute: Submit preferred providers and user API keys
ConsensusRoute->>KeyResolver: Resolve ordered consensus candidates
KeyResolver-->>ConsensusRoute: Return candidate providers and keys
ConsensusRoute->>Provider: Generate and validate verdict
Provider-->>ConsensusRoute: Return verdict or generation error
ConsensusRoute-->>Client: Return verdict or localized failure
Possibly related PRs
Comment |
|
Fixed in ebc8807 - prettier on consensus route, debate engine, consensus-resolve. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useDebateEngine.ts (2)
668-696: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing session guard in the
!res.okbranch lets a stale response resurrect a cleared message.The
res.status === 402branch (Line 680) checkssessionIdRef.current !== thisSessionbefore dispatching, and the success branch (Line 697) checkssessionIdRef.current === thisSession. The!res.okbranch (Lines 689-696) has no such check. If a new debate session starts while this fetch is still in flight, the stale-session cleanup at Lines 592-600 will already have clearedanalyzingMsg's content to""; this branch can then resurrect that same message withfailMsg, showing a stray error bubble unrelated to the current session.🐛 Proposed fix
} else if (!res.ok) { + if (sessionIdRef.current !== thisSession) return const failMsg = await consensusFailureMessage(res, locale) logDebate("verdict:failed", { status: res.status, message: failMsg }) dispatch({ type: "UPDATE_MESSAGE", id: analyzingMsg.id, content: failMsg, }) }🤖 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 `@src/hooks/useDebateEngine.ts` around lines 668 - 696, Add a session guard to the !res.ok failure branch before computing or dispatching failMsg, matching the existing 402 and success-branch checks: return when sessionIdRef.current !== thisSession. Keep logging and UPDATE_MESSAGE strictly limited to the active debate session within the verdict-fetch flow.
800-829: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame missing session guard in
handleStop's!res.okbranch.Mirrors the issue in the final-verdict flow above. Line 809's
402branch checkssessionIdRef.current !== stoppedSessionand the second.then(Line 833) also checks it, but the!res.okbranch (Lines 820-828) dispatchesUPDATE_MESSAGEunconditionally. A superseded stop-time consensus fetch can revive an already-cleared "analyzing" message with a stale error after a new debate has started.🐛 Proposed fix
if (!res.ok) { + if (sessionIdRef.current !== stoppedSession) return null const failMsg = await consensusFailureMessage(res, locale) dispatch({ type: "UPDATE_MESSAGE", id: analyzingMsg.id, content: failMsg, }) return null }🤖 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 `@src/hooks/useDebateEngine.ts` around lines 800 - 829, Guard the non-OK response handling in handleStop’s fetchConsensus chain with the same session check used by the 402 branch and subsequent .then: before dispatching UPDATE_MESSAGE with consensusFailureMessage, return without updating when sessionIdRef.current differs from stoppedSession. This prevents stale stop-time requests from modifying a newer debate.
🤖 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 `@src/app/api/consensus/route.ts`:
- Around line 370-374: Replace the hardcoded 30_000 timeout in the generateRaw
malformed-JSON retry with the same tier-aware timeout used by the primary
provider call, ensuring Pro retries receive the 120_000 allowance while other
tiers retain their configured limits.
- Around line 155-162: Update withTimeout to create and return an
AbortController signal, clear the timeout when the provider promise settles, and
abort the controller on timeout. Thread the signal through generateRaw and pass
it to generateClaudeVerdict, generateGptVerdict, and generatePerplexityVerdict
so timed-out requests are cancelled before trying the next candidate.
In `@src/lib/consensus-resolve.ts`:
- Line 1: Run Prettier in write mode on src/lib/consensus-resolve.ts and verify
the resulting formatting passes the repository’s Prettier check.
- Around line 111-119: In the rate-limit detection logic, replace the broad
lower.includes("rate") check with a more precise match for rate-limit
terminology, such as the standalone token or phrase “rate limit,” while
preserving the existing 429, quota, and resource-exhausted checks. Update the
condition in the error-classification function containing this block.
---
Outside diff comments:
In `@src/hooks/useDebateEngine.ts`:
- Around line 668-696: Add a session guard to the !res.ok failure branch before
computing or dispatching failMsg, matching the existing 402 and success-branch
checks: return when sessionIdRef.current !== thisSession. Keep logging and
UPDATE_MESSAGE strictly limited to the active debate session within the
verdict-fetch flow.
- Around line 800-829: Guard the non-OK response handling in handleStop’s
fetchConsensus chain with the same session check used by the 402 branch and
subsequent .then: before dispatching UPDATE_MESSAGE with
consensusFailureMessage, return without updating when sessionIdRef.current
differs from stoppedSession. This prevents stale stop-time requests from
modifying a newer debate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 287db974-0c19-4961-984f-fa3b3894415d
📒 Files selected for processing (10)
src/__tests__/byok-route-guards.test.tssrc/__tests__/consensus-resolve.test.tssrc/app/api/consensus/route.tssrc/components/SettingsModal.tsxsrc/hooks/useDebateEngine.tssrc/lib/consensus-resolve.tssrc/lib/providers/claude.tssrc/lib/providers/gpt.tssrc/lib/providers/perplexity.tssrc/lib/server-provider-keys.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI / ci: fix: stabilize consensus and make it provider-agnostic
Conclusion: failure
##[group]Run npx prettier --check .
�[36;1mnpx prettier --check .�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Checking formatting...
[�[33mwarn�[39m] src/app/api/consensus/route.ts
[�[33mwarn�[39m] src/hooks/useDebateEngine.ts
[�[33mwarn�[39m] src/lib/consensus-resolve.ts
[�[33mwarn�[39m] Code style issues found in 3 files. Run Prettier with --write to fix.
##[error]Process completed with exit code 1.
GitHub Actions: CI / 0_ci.txt: fix: stabilize consensus and make it provider-agnostic
Conclusion: failure
##[group]Run npx prettier --check .
�[36;1mnpx prettier --check .�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Checking formatting...
[�[33mwarn�[39m] src/app/api/consensus/route.ts
[�[33mwarn�[39m] src/hooks/useDebateEngine.ts
[�[33mwarn�[39m] src/lib/consensus-resolve.ts
[�[33mwarn�[39m] Code style issues found in 3 files. Run Prettier with --write to fix.
##[error]Process completed with exit code 1.
🧰 Additional context used
🪛 ESLint
src/__tests__/consensus-resolve.test.ts
[error] 31-31: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 35-35: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 39-39: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
src/__tests__/byok-route-guards.test.ts
[error] 187-187: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
🪛 GitHub Actions: CI / 0_ci.txt
src/app/api/consensus/route.ts
[warning] 1-1: Prettier --check reported a formatting/style warning in this file.
src/lib/consensus-resolve.ts
[warning] 1-1: Prettier --check reported a formatting/style warning in this file.
src/hooks/useDebateEngine.ts
[warning] 1-1: Prettier --check reported a formatting/style warning in this file.
🪛 GitHub Actions: CI / ci
src/app/api/consensus/route.ts
[warning] 1-1: Prettier --check reported code style issues in this file.
src/lib/consensus-resolve.ts
[warning] 1-1: Prettier --check reported code style issues in this file.
src/hooks/useDebateEngine.ts
[warning] 1-1: Prettier --check reported code style issues in this file.
🔇 Additional comments (11)
src/lib/server-provider-keys.ts (1)
18-79: LGTM!src/__tests__/consensus-resolve.test.ts (1)
9-41: LGTM!src/app/api/consensus/route.ts (1)
254-332: LGTM!src/lib/providers/gpt.ts (1)
58-88: LGTM!src/lib/providers/perplexity.ts (1)
92-129: LGTM!src/lib/providers/claude.ts (1)
91-99: 🎯 Functional CorrectnessNo issue:
claude-sonnet-4-6is a valid Anthropic model ID and this pin is fine.> Likely an incorrect or invalid review comment.src/hooks/useDebateEngine.ts (3)
5-5: LGTM!Also applies to: 48-66, 114-142, 144-156
502-505: LGTM!Correctly implements the PR objective — the last round no longer fires the intermediate confidence consensus call, avoiding a double consensus request against the final-stage call.
Also applies to: 524-555, 628-630
872-872: LGTM!src/components/SettingsModal.tsx (1)
295-296: LGTM!Also applies to: 323-324
src/__tests__/byok-route-guards.test.ts (1)
21-36: LGTM!Also applies to: 117-119, 175-175, 185-192, 242-244, 261-305, 387-424
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebc8807925
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the /api/consensus “verdict” step by making it provider-agnostic (Gemini → Claude → GPT → Perplexity) and improving key resolution + user-facing error messaging during consensus.
Changes:
- Added multi-provider consensus candidate resolution (including anonymous multi-key BYOK) and provider-order selection.
- Implemented non-streaming “verdict” calls for Claude/GPT/Perplexity and updated the consensus route to fall back across providers.
- Improved client UX for consensus failures (plain-language timeout/rate-limit/key messaging) and avoided double-firing consensus in the last round.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/server-provider-keys.ts | Exposes access-code validation and adds “soft” key resolution for multi-provider fallback without emitting 402s directly. |
| src/lib/providers/perplexity.ts | Adds non-streaming verdict generation for consensus fallback. |
| src/lib/providers/gpt.ts | Adds non-streaming verdict generation for consensus. |
| src/lib/providers/claude.ts | Adds non-streaming verdict generation for consensus. |
| src/lib/consensus-resolve.ts | New consensus provider ordering + candidate resolution + human-friendly error mapping. |
| src/app/api/consensus/route.ts | Reworks consensus route to try multiple providers, adds JSON extraction/repair, and plain-language error responses. |
| src/hooks/useDebateEngine.ts | Sends all anonymous browser keys, avoids last-round double consensus, and surfaces server-provided error messages. |
| src/components/SettingsModal.tsx | Updates settings copy to reflect consensus provider fallback behavior. |
| src/tests/consensus-resolve.test.ts | Adds unit tests for provider ordering and human error mapping. |
| src/tests/byok-route-guards.test.ts | Extends BYOK route-guard tests for multi-provider verdict fallback and request-body key usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/__tests__/consensus-resolve.test.ts`:
- Around line 38-44: Hoist the regular expressions used by the “does not treat
generate as a rate limit” and “localizes Korean timeouts” tests in
consensus-resolve.test.ts to module scope, then update the expect matchers to
reference those shared regex constants instead of inline literals, avoiding
e18e/prefer-static-regex lint errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f6445e0-90e2-45e1-ba91-b7f86a5f5025
📒 Files selected for processing (4)
src/__tests__/consensus-resolve.test.tssrc/app/api/consensus/route.tssrc/hooks/useDebateEngine.tssrc/lib/consensus-resolve.ts
📜 Review details
🧰 Additional context used
🪛 ESLint
src/__tests__/consensus-resolve.test.ts
[error] 39-39: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
[error] 43-43: Move this regular expression to module scope to avoid re-compilation on every call.
(e18e/prefer-static-regex)
🔇 Additional comments (6)
src/lib/consensus-resolve.ts (1)
99-156: LGTM!src/__tests__/consensus-resolve.test.ts (1)
9-27: LGTM!src/app/api/consensus/route.ts (2)
155-172: LGTM!
264-351: LGTM!Also applies to: 389-393, 434-449
src/hooks/useDebateEngine.ts (2)
114-147: LGTM!Also applies to: 503-559, 624-648
666-736: LGTM!Also applies to: 799-871
Review fixes applied