feat(routing): fail over between policy candidates on retryable failures - #1281
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds Responses policy fallback handling. It ranks eligible policy candidates, retries retryable pre-success failures, preserves the original policy context, and avoids fallback after terminal errors, cancellation, or stream start. Tests cover these outcomes. ChangesPolicy fallback behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleResponses
participant CoreHandler
participant PolicyFallback
Client->>handleResponses: Send replayable policy request
handleResponses->>CoreHandler: Execute original request
CoreHandler-->>PolicyFallback: Return retryable pre-success failure
PolicyFallback->>PolicyFallback: Rank eligible untried candidates
PolicyFallback->>CoreHandler: Retry with next candidate model
CoreHandler-->>PolicyFallback: Return response
PolicyFallback-->>Client: Return response with original policy context
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/routing-policy-fallback.test.ts`:
- Around line 125-154: Expand the fallback regression coverage around
handleResponsesWithPolicyFallback with separate tests for client cancellation
and streaming failures after a non-empty output delta. In each case, simulate
the terminal condition through runCore and assert the response behavior plus
exactly one runCore invocation, preserving the no-candidate-switch behavior for
both guards.
- Around line 119-123: Extend the test around the existing response, seenModels,
and routeDecision assertions to validate logCtx.attempts. Assert two attempts in
order: provider-a/model-a with the defined candidate identity and a 429 failure,
followed by provider-b/model-b with the defined candidate identity and a
successful outcome, using the existing attempt-field names and preserving the
current fallback assertions.
🪄 Autofix
❌ Autofix failed (check again to retry)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 969e683c-3ac2-45ca-9d9d-c7703767e709
📒 Files selected for processing (1)
tests/routing-policy-fallback.test.ts
|
The branch was updated while autofix was in progress. Please try again. |
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/server/responses/policy-fallback.ts`:
- Around line 16-18: Update candidateKey and the tried initialization in the
fallback flow to include accountRef, so candidates sharing provider/model but
using different accounts remain distinct. In the retry logic around the fallback
execution path, preserve and explicitly pass the ranked candidate’s accountRef
(and corresponding codexAccountId when required) instead of re-routing only by
provider/model; verify core selection cannot replace the selected account.
🪄 Autofix
❌ Autofix failed (check again to retry)
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: ASSERTIVE
Plan: Pro Plus
Run ID: 85dda22f-b5fb-4054-a5c3-809e537e3fbf
📒 Files selected for processing (2)
src/server/responses.tssrc/server/responses/policy-fallback.ts
|
The branch was updated while autofix was in progress. Please try again. |
|
Review follow-up: the |
1 participant
Summary
This PR adds policy-level fallback after the initially selected routing-profile candidate fails before useful output escapes.
Design constraints:
attempts[]historyTDD state
The first commit intentionally adds failing tests for the missing behavior. The implementation commit will follow after CI confirms the red state.
Scope
Base inspected:
devat3ad5bb6bd3f76f6879d84b78ea39edd3e01ec296.Summary by CodeRabbit
New Features
Tests