Repository navigation
fix(sdk): fast-fail provider errors to fallback orchestration - #1251
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis change centralizes provider retry control, preserves retry metadata, refines fallback handling for aborts and 429 errors, expands modern Claude vision matching, disables SDK retries, and adds continuous latency and capability tests. ChangesProvider resilience and capability handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/utils/providerRetry.ts (1)
53-68: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuck-typed status/retryAfter extraction is duplicated across
providerRetry.tsandbaseProvider.ts. Both files independently extractstatusCode(from.statusCodethen.status) andretryAfterMs(from aretryAfterMsfield, thenheaders/responseHeadersviaparseRetryAfterMs) from an arbitrary error-shaped object. The shared root cause is that neither helper is exported for reuse.
src/lib/utils/providerRetry.ts#L53-L68: exportduckTypedStatusCode(or add a new exportedextractRetryMetadata(error)helper) so other modules can reuse it instead of reimplementing.src/lib/utils/providerRetry.ts#L110-L135: extract theheaders/responseHeadersduck-typed fallback loop into an exported helper (e.g.extractRetryAfterMsFromError) shared withbaseProvider.ts.src/lib/core/baseProvider.ts#L2117-L2173: replace the re-implementedstatusCode/retryAfterMsextraction with calls to the shared helpers exported fromproviderRetry.ts.♻️ Suggested extraction
+// src/lib/utils/providerRetry.ts +export function duckTypedStatusCode(error: unknown): number | undefined { + // existing implementation, now exported +} + +export function extractRetryAfterMsFromError(error: unknown): number | undefined { + if (!error || typeof error !== "object") { + return undefined; + } + const err = error as { + retryAfterMs?: unknown; + headers?: unknown; + responseHeaders?: unknown; + }; + if (typeof err.retryAfterMs === "number") { + return err.retryAfterMs; + } + for (const candidate of [err.headers, err.responseHeaders]) { + if (candidate && typeof candidate === "object") { + const parsed = parseRetryAfterMs( + candidate as Pick<Headers, "get"> | Readonly<Record<string, string>>, + ); + if (parsed !== undefined) { + return parsed; + } + } + } + return undefined; +}-// src/lib/core/baseProvider.ts — replace the re-implemented statusCode/retryAfterMs -// extraction inside handleProviderError() with calls to the exported helpers above. +import { duckTypedStatusCode, extractRetryAfterMsFromError } from "../utils/providerRetry.js"; +// ... then use duckTypedStatusCode(src) and extractRetryAfterMsFromError(src)🤖 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/lib/utils/providerRetry.ts` around lines 53 - 68, Export the shared status and retry-after extraction helpers from src/lib/utils/providerRetry.ts, including the duck-typed headers/responseHeaders fallback currently handled near lines 110-135. Update src/lib/core/baseProvider.ts lines 2117-2173 to call these exported helpers instead of reimplementing statusCode and retryAfterMs extraction, preserving the existing extraction order and parseRetryAfterMs behavior.src/lib/providers/openaiChatCompletionsClient.ts (1)
756-762: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider allowlisting response headers instead of capturing all of them.
err.responseHeaderscurrently stores every header fromres.headers. Only rate-limit related headers (retry-after,x-ratelimit-*) are needed downstream. If this error object is later logged in full, unrelated headers (for example a proxy-setset-cookieor internal routing header) would be exposed along with it.Filter to the headers actually consumed by the retry logic before attaching them to the error.
🔒 Proposed fix to allowlist headers
- err.responseHeaders = Object.fromEntries(res.headers.entries()); + const RETRY_RELEVANT_HEADER_PREFIXES = ["retry-after", "x-ratelimit-"]; + err.responseHeaders = Object.fromEntries( + [...res.headers.entries()].filter(([key]) => + RETRY_RELEVANT_HEADER_PREFIXES.some((p) => key.toLowerCase().startsWith(p)), + ), + );Based on learnings, prior reviews in this codebase repeatedly flagged that raw values that may carry sensitive data must not be attached to errors/logs without redaction (for example,
Do NOT assume any redaction will occur via transformParamsForLogging()andnever log or surface full URL strings in client-facing errors or logs).🤖 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/lib/providers/openaiChatCompletionsClient.ts` around lines 756 - 762, Update the response-header handling near err.responseHeaders to allowlist only retry-related headers: retry-after and headers whose names start with x-ratelimit-, using case-insensitive matching. Attach only those filtered entries to the error, excluding unrelated or potentially sensitive headers such as set-cookie and internal routing headers.Source: Learnings
🤖 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 `@test/continuous-test-suite-provider-fallback-latency.ts`:
- Around line 292-317: Update the fetch URL routing around the input-to-url
conversion to parse the URL and compare its hostname exactly against
api.anthropic.com and api.openai.com, replacing both substring includes checks.
Preserve the existing abort-hanging behavior, OpenAI response, and
unexpected-URL error for all other hosts.
---
Nitpick comments:
In `@src/lib/providers/openaiChatCompletionsClient.ts`:
- Around line 756-762: Update the response-header handling near
err.responseHeaders to allowlist only retry-related headers: retry-after and
headers whose names start with x-ratelimit-, using case-insensitive matching.
Attach only those filtered entries to the error, excluding unrelated or
potentially sensitive headers such as set-cookie and internal routing headers.
In `@src/lib/utils/providerRetry.ts`:
- Around line 53-68: Export the shared status and retry-after extraction helpers
from src/lib/utils/providerRetry.ts, including the duck-typed
headers/responseHeaders fallback currently handled near lines 110-135. Update
src/lib/core/baseProvider.ts lines 2117-2173 to call these exported helpers
instead of reimplementing statusCode and retryAfterMs extraction, preserving the
existing extraction order and parseRetryAfterMs behavior.
🪄 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: a17bb8ac-f113-469b-9f7e-ad768be28777
📒 Files selected for processing (15)
package.jsonsrc/lib/adapters/providerImageAdapter.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/googleVertex.tssrc/lib/providers/openaiChatCompletionsClient.tssrc/lib/types/config.tssrc/lib/types/generate.tssrc/lib/types/providers.tssrc/lib/types/stream.tssrc/lib/utils/providerRetry.tstest/continuous-test-suite-provider-fallback-latency.tstest/continuous-test-suite-provider-fallback.tstest/retryAfter.test.ts
5cd08bb to
f2779fd
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
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/lib/utils/providerRetry.ts`:
- Around line 91-93: Update the retry-hint handling around the retryAfterMs
check to reject NaN as well as non-number values before returning a delay,
allowing the existing no-hint fallback to apply. Add a regression test covering
an error with retryAfterMs: NaN and verify it produces the fallback delay.
🪄 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: 6a440fff-d2fc-4bbe-acf5-b3f7a6bf24eb
📒 Files selected for processing (15)
package.jsonsrc/lib/adapters/providerImageAdapter.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/googleVertex.tssrc/lib/providers/openaiChatCompletionsClient.tssrc/lib/types/config.tssrc/lib/types/generate.tssrc/lib/types/providers.tssrc/lib/types/stream.tssrc/lib/utils/providerRetry.tstest/continuous-test-suite-provider-fallback-latency.tstest/continuous-test-suite-provider-fallback.tstest/retryAfter.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- package.json
- src/lib/providers/googleVertex.ts
- src/lib/providers/anthropic.ts
- src/lib/types/providers.ts
- src/lib/types/stream.ts
- src/lib/types/generate.ts
- src/lib/providers/openaiChatCompletionsClient.ts
- src/lib/types/config.ts
- test/retryAfter.test.ts
- test/continuous-test-suite-provider-fallback.ts
- src/lib/core/baseProvider.ts
- src/lib/neurolink.ts
- test/continuous-test-suite-provider-fallback-latency.ts
- src/lib/adapters/providerImageAdapter.ts
| * modelRegistry.ts) because gateway ids carry arbitrary prefixes/suffixes | ||
| * (`vertex_ai/claude-sonnet-5@20260203`) that exact entries can't cover. | ||
| * | ||
| * Every name-first Claude id (claude-{opus,sonnet,haiku}-N) with major ≥ 4 is |
There was a problem hiding this comment.
💡 MINOR: Add a JSDoc comment above CLAUDE_MODERN_VISION_FAMILIES explaining that these regex patterns match name-first IDs (claude-4-0, claude-sonnet-5) and Fable/Mythos models, and are checked only when an exact allowlist match fails. Explicitly note that legacy number-first 3.x IDs like claude-3-5-haiku never had vision and deliberately don't match.
Review SummaryDecision: CHANGES_REQUESTED Findings (2 total)
Impact on Existing Code
Review ScopeReviewed all changed files file-by-file:
Next Steps
Review performed by Yama following NeuroLink project standards and bootstrapped reviewer patterns. |
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · Reviewed PR #1251 across all changed files. Found 1 gate-verified issue requiring fixes before merge: Findings (this run): - Findings behind this verdict
|
f2779fd to
d6c07f6
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 10.8.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Problem
When a provider returns non-2xx, the error must reach
runWithFallbackOrchestrationpromptly so a configuredproviderFallback/modelChaincan route to another provider. Today, retry layers below the orchestrator can swallow failures for hours: a prod 429 withretry-after: 8549made@anthropic-ai/sdk(maxRetries 2, uncapped un-abortable Retry-After sleep) sleep ~2.4h per retry inside onegenerate()— the error never surfaced, fallback was never consulted, and turns died via the stall watchdog.Fixes (provider-generic)
1. Never honor unbounded Retry-After
maxRetries: 0on both Anthropic SDK clients (api_key + OAuth paths) and onAnthropicVertex— transport retries are the orchestrator's job; transient network blips are still retried byfetchWithRetryinsidecreateProxyFetch().withProviderRetry: Retry-After cap lowered to 60s, and a hint above the cap now surfaces immediately (no clamp-and-sleep) with the error stampedisRetryable: false+retryAfterMsso upper layers stand down. Hints ≤ 60s keep the honor-the-hint behavior..status(official SDK errors) alongside.statusCode, and extracts hints from.headers/.responseHeaders.handleProviderErrornow preservesstatusCode/isRetryable/retryAfterMsacrossformatProviderError(formatters previously destroyed all retry metadata).responseHeadersto its errors so Retry-After hints work for that whole family.@google/genai(no auto-retry configured).2. Watchdog aborts no longer masquerade as caller cancels at the fallback gate
Error shape can't distinguish provenance — SDKs normalize NeuroLink's internal watchdog/timeout aborts into the same AbortError shapes a user cancel produces. The gate now uses ground truth: an abort-shaped error skips fallback only when the caller-supplied
abortSignalhas actually fired. Internal aborts (turn/stall watchdog, per-step timeout) are fallback-eligible; genuine caller cancels stay excluded. JSDoc contracts updated intypes/{config,generate,stream}.ts.3. Vision capability registry: Claude 5 / Opus 4.x+ on anthropic
SAMPLING_PARAM_REJECTING_FAMILIESprecedent): every name-firstclaude-{opus,sonnet,haiku}-Nid with major ≥ 4 (plus fable/mythos) is vision-capable on anthropic and vertex — coversclaude-sonnet-5,claude-opus-5,claude-opus-4-7/4-8,@-suffixed and gateway-prefixed ids. Legacy number-first 3.x ids (e.g. non-visionclaude-3-5-haiku) stay fail-closed.ANTHROPIC_BASE_URLset) no longer capability-gates at all — the upstream decides (mirrors thevalidateModelAccessproxy bypass).Tests
New
test/continuous-test-suite-provider-fallback-latency.ts(pnpm run test:provider-fallback-latency, no API keys, all HTTP mocked):retry-after: 8549→generate()rejects in ~200ms (pre-fix: hours); withproviderFallback→ callback invoked once, retry served by the returned provider.buildMultimodalMessagesArrayonanthropic/claude-sonnet-5.Updated:
test/continuous-test-suite-provider-fallback.ts(abort contract),test/retryAfter.test.ts(60s cap, over-cap surfacing,.status/.headersduck-typing).Verified:
tsc --strictclean, lint clean, full build + publint clean, suites green (9/9 new, 13/13 fallback, 17/17 retry-after, 30/30 providers-mocked, 39/39 anthropic-cap).Summary by CodeRabbit
New Features
Bug Fixes
Tests