fix(router-provider): fetch model metadata before context management decisions - #1053
fix(router-provider): fetch model metadata before context management decisions#1053JamesRobert20 wants to merge 4 commits into
Conversation
…decisions Router providers (zoo-gateway, kimi-code) that are auth-scoped skip the model cache entirely. On a fresh handler instance getModel() falls back to hardcoded defaults (e.g. 200k context window) because the real model list has not been fetched yet. Context management runs before createMessage() which is where fetchModel() normally happens, so condensing/truncation decisions use the wrong context window. Add ensureModelFetched() to RouterProvider that fetches once when the instance model map is empty. Call it in Task before context management so getModel() returns accurate metadata from the API. Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesModel Metadata Fetching
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant ApiHandler
participant RouterProvider
Task->>ApiHandler: ensureModelFetched()
ApiHandler->>RouterProvider: ensureModelFetched()
RouterProvider->>RouterProvider: fetchModel() when models are empty
RouterProvider-->>Task: model metadata available
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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 `@src/api/providers/router-provider.ts`:
- Around line 64-68: Make ensureModelFetched() single-flight by storing and
reusing an in-flight fetchModel() promise, clearing it after completion while
preserving the empty-model check. Update ZooGatewayHandler.createMessage() and
completePrompt() to call ensureModelFetched() followed by getModel(), replacing
their unconditional fetchModel() calls so auth-scoped providers avoid duplicate
metadata requests.
In `@src/core/task/Task.ts`:
- Around line 4046-4047: Move the optional ensureModelFetched call before the
cachedStreamingModel snapshot and before getSystemPrompt-dependent request
setup. Reuse the initialized model returned by this.api.getModel() for both
cachedStreamingModel and modelInfo, ensuring fresh router providers have
metadata available even when contextTokens is falsy.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc932c48-2b90-434b-b92a-9aef4da2165e
📒 Files selected for processing (4)
src/api/index.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/router-provider.tssrc/core/task/Task.ts
…ll site Make ensureModelFetched single-flight so concurrent callers share a single in-flight fetch instead of firing duplicates. Move the call site before the cachedStreamingModel snapshot so the model info is accurate from the start of the streaming session, not just for context management. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/task/Task.ts (1)
4048-4049: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove model fetching before
getSystemPrompt()and remove thecontextTokensguard.This is the same still-valid issue from the previous review. Direct
attemptApiRequest()callers computegetSystemPrompt()before fetching metadata, and requests with falsycontextTokensskip fetching entirely, leaving router-provider defaults in model-dependent logic. Fetch at the start ofattemptApiRequest()beforegetSystemPrompt().🤖 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/core/task/Task.ts` around lines 4048 - 4049, Update attemptApiRequest to call ensureModelFetched at its start, before getSystemPrompt is computed, and remove the contextTokens guard that currently allows fetching to be skipped. Preserve the existing model metadata usage through getModel().info after the unconditional fetch.
🤖 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.
Duplicate comments:
In `@src/core/task/Task.ts`:
- Around line 4048-4049: Update attemptApiRequest to call ensureModelFetched at
its start, before getSystemPrompt is computed, and remove the contextTokens
guard that currently allows fetching to be skipped. Preserve the existing model
metadata usage through getModel().info after the unconditional fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d25401b-9da0-459f-9f39-79608468aa3a
📒 Files selected for processing (3)
src/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/router-provider.tssrc/core/task/Task.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/providers/tests/zoo-gateway.spec.ts
- src/api/providers/router-provider.ts
github-actions
Bot
added
the
awaiting-review
|
|
||
| await this.diffViewProvider.reset() | ||
|
|
||
| await this.api.ensureModelFetched?.() |
There was a problem hiding this comment.
The litellm/deepseek/moonshot fetchers re-throw, so a failure here would reject into the catch at L3743, which return trues and ends the task silently — the comment there assumes only attemptApiRequest can throw. Pre-change, the same failure surfaced via the createMessage retry path. Would catching and logging here be safer, letting getModel() fall back to defaults?
|
|
||
| private modelFetchPromise?: Promise<void> | ||
|
|
||
| async ensureModelFetched(): Promise<void> { |
There was a problem hiding this comment.
For auth-scoped providers, does the first request now fetch the model list twice — once here and again unconditionally in createMessage (zoo-gateway.ts L184)? getModels bypasses both caches for auth-scoped providers (modelCache.ts L265), so the second call is another network round trip. Worth making fetchModel() single-flight (or short-circuiting when this.models is populated) so both callers share one fetch?
| }) | ||
| }) | ||
|
|
||
| describe("ensureModelFetched", () => { |
There was a problem hiding this comment.
Is the rejection path covered anywhere? If getModels rejects once and the finally cleanup in router-provider.ts (L71-74) ever regresses, the stored rejected promise would poison every subsequent call. A reject-then-resolve test here would lock that in.
github-actions
Bot
added
awaiting-author
…ouble-fetch Make fetchModel single-flight and short-circuit once models are loaded so auth-scoped providers do not hit the models endpoint twice per request. Catch ensureModelFetched failures in Task via safeEnsureModelFetched so a metadata fetch error falls back to defaults instead of ending the task. Add reject-then-recover coverage and Task tests for the new call sites. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
removed
the
awaiting-author
2 participants
Problem
Router providers that are auth-scoped (zoo-gateway, kimi-code) skip the model cache entirely. When a fresh handler is constructed (on provider switch or profile change),
getModel()falls back to hardcoded default model info because the real model list has not been fetched from the API yet.Context management in
Task.tscallsgetModel()beforecreateMessage()(wherefetchModel()normally runs). This means condensing and truncation decisions use the default context window (200k for zoo-gateway) instead of the actual model's context window.If the user selects a model with a larger context window through the gateway (e.g. Opus 4.8 at 1M tokens), the system incorrectly calculates context usage against 200k, triggering premature condensation.
Fix
ensureModelFetched()toRouterProviderthat callsfetchModel()once when the instance model map is empty. Subsequent calls are no-ops.ensureModelFetchedas an optional method on theApiHandlerinterface soTask.tscan call it.await this.api.ensureModelFetched?.()inTask.tsbefore both context management paths (attemptApiRequestandhandleContextWindowExceededError).Static providers (Anthropic, Gemini, Bedrock, etc.) are unaffected since they don't implement the method. Non-auth-scoped router providers (OpenRouter, Vercel AI Gateway) already have cache hits via
getModelsFromCache()so the fetch is skipped after the first call.Summary by CodeRabbit