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 2 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>
📝 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
| async ensureModelFetched(): Promise<void> { | ||
| if (Object.keys(this.models).length === 0) { | ||
| await this.fetchModel() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make model fetching single-flight and reusable.
Concurrent callers can all observe an empty models map before fetchModel() completes, causing duplicate remote requests. Also, ZooGatewayHandler.createMessage() and completePrompt() still call fetchModel() unconditionally; for auth-scoped providers, getModels() skips caching, so the same request can fetch metadata twice.
Use an in-flight promise and update request paths to call ensureModelFetched() followed by getModel().
Suggested fix
+private modelFetchPromise?: Promise<void>
+
async ensureModelFetched(): Promise<void> {
if (Object.keys(this.models).length === 0) {
- await this.fetchModel()
+ const fetchPromise = (this.modelFetchPromise ??= this.fetchModel().then(() => undefined))
+ try {
+ await fetchPromise
+ } finally {
+ if (this.modelFetchPromise === fetchPromise) {
+ this.modelFetchPromise = undefined
+ }
+ }
}
}Then replace direct request-time fetchModel() calls with:
-await this.fetchModel()
+await this.ensureModelFetched()
+const { id, info } = this.getModel()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async ensureModelFetched(): Promise<void> { | |
| if (Object.keys(this.models).length === 0) { | |
| await this.fetchModel() | |
| } | |
| } | |
| private modelFetchPromise?: Promise<void> | |
| async ensureModelFetched(): Promise<void> { | |
| if (Object.keys(this.models).length === 0) { | |
| const fetchPromise = (this.modelFetchPromise ??= this.fetchModel().then(() => undefined)) | |
| try { | |
| await fetchPromise | |
| } finally { | |
| if (this.modelFetchPromise === fetchPromise) { | |
| this.modelFetchPromise = undefined | |
| } | |
| } | |
| } | |
| } |
🤖 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/api/providers/router-provider.ts` around lines 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.
…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
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
New Features
Bug Fixes