Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the large model discovery GET handler in route.ts by extracting provider-specific logic into modular handler files, custom normalizers, configurations, and types. However, the refactoring introduces several critical issues: the extracted handlers are called sequentially without provider guards (causing short-circuiting), the handler mappings are completely mixed up (e.g., handleDatarobotModels contains OpenAI logic), and there are multiple compilation and runtime errors due to missing imports and undefined variables (such as HttpProxyConfig, toLocalCatalogModels, url, and handleCloudflare_aiModels).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| connection: any; | ||
| apiKey: string; | ||
| accessToken: string; | ||
| proxy: HttpProxyConfig | undefined; |
There was a problem hiding this comment.
HttpProxyConfig is referenced here but it is never imported or defined in this file, which will cause a TypeScript compilation error.
| proxyConfig: proxy, | ||
| ...(init as Record<string, unknown>), | ||
| }), | ||
| fallbackModels: toLocalCatalogModels(), |
There was a problem hiding this comment.
toLocalCatalogModels is called here but it is neither imported nor defined in this file. It was a local helper function inside the GET handler in route.ts. This will throw a ReferenceError at runtime.
| { status: 400 } | ||
| ); | ||
| } | ||
| url = url.replace("{accountId}", accountId); |
There was a problem hiding this comment.
url is referenced and assigned here, but it is not defined or declared anywhere in this file or function. This will throw a ReferenceError at runtime.
| } | ||
| url = url.replace("{accountId}", accountId); | ||
| } | ||
| const cloudflare_aiRes = await handleCloudflare_aiModels(ctx); if (cloudflare_aiRes) return cloudflare_aiRes; |
There was a problem hiding this comment.
handleCloudflare_aiModels is called here but it is never imported in this file. This will throw a ReferenceError at runtime.
| const rekaRes = await handleRekaModels(ctx); | ||
| if (rekaRes) return rekaRes; | ||
| const bedrockRes = await handleBedrockModels(ctx); | ||
| if (bedrockRes) return bedrockRes; | ||
| const datarobotRes = await handleDatarobotModels(ctx); | ||
| if (datarobotRes) return datarobotRes; | ||
| const azure_aiRes = await handleAzureAiModels(ctx); | ||
| if (azure_aiRes) return azure_aiRes; | ||
| const azure_openaiRes = await handleAzureOpenaiModels(ctx); | ||
| if (azure_openaiRes) return azure_openaiRes; | ||
| const watsonxRes = await handleWatsonxModels(ctx); | ||
| if (watsonxRes) return watsonxRes; | ||
| const ociRes = await handleOciModels(ctx); | ||
| if (ociRes) return ociRes; | ||
| const sapRes = await handleSapModels(ctx); | ||
| if (sapRes) return sapRes; | ||
| const claudeRes = await handleClaudeModels(ctx); | ||
| if (claudeRes) return claudeRes; | ||
| const cursorRes = await handleCursorModels(ctx); | ||
| if (cursorRes) return cursorRes; | ||
| const inner_aiRes = await handleInnerAiModels(ctx); | ||
| if (inner_aiRes) return inner_aiRes; | ||
| const glmRes = await handleGlmModels(ctx); | ||
| if (glmRes) return glmRes; | ||
| const gemini_cliRes = await handleGeminiCliModels(ctx); | ||
| if (gemini_cliRes) return gemini_cliRes; | ||
| const antigravityRes = await handleAntigravityModels(ctx); | ||
| if (antigravityRes) return antigravityRes; | ||
| const githubRes = await handleGithubModels(ctx); | ||
| if (githubRes) return githubRes; | ||
| const anthropicCompatibleRes = await handleAnthropicCompatibleModels(ctx); | ||
| if (anthropicCompatibleRes) return anthropicCompatibleRes; |
There was a problem hiding this comment.
Critical Bug: Missing Provider Guards and Mixed-up Handler Mappings
There are two major issues with how the handlers are invoked here:
- Missing Provider Guards: The handlers are called sequentially without checking if the current
providermatches the handler's target provider (e.g.,if (provider === "reka")). SincehandleRekaModelsis called first and immediately returnsbuildLocalCatalogResponse()(which returns a response for any provider that has local models), almost all requests will short-circuit and return the local catalog, completely bypassing remote API discovery. - Mixed-up Handler Mappings: The handlers and files have been completely mixed up (shifted down) during extraction:
handleDatarobotModelscontains OpenAI-compatible logic.handleAzureAiModelscontains DataRobot logic.handleAzureOpenaiModelscontains Azure AI logic.handleWatsonxModelscontains Azure OpenAI logic.handleOciModelscontains Watsonx logic.handleSapModelscontains OCI logic.handleClaudeModelscontains SAP logic.handleCursorModelscontains Claude logic.handleInnerAiModelscontains Cursor logic.handleGlmModelscontains Inner.ai logic.handleGeminiCliModelscontains GLM logic.handleAntigravityModelscontains Gemini CLI logic.handleGithubModelscontains Antigravity logic.handleAnthropicCompatibleModelscontains GitHub Copilot logic.- The original
isAnthropicCompatibleProviderlogic is completely missing.
You must restore the proper mapping and wrap each handler call with a check for the corresponding provider.
Code Review SummaryStatus: 19 Active Issues | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Previously Reported Issues (Resolved in this commit):
Files Reviewed (19 files)
Reply with Reviewed by laguna-m.1-20260312:free · 2,716,381 tokens |
…aper-inference#3691) Integrated into release/v3.8.23
…ce#3690) Integrated into release/v3.8.23
…heaper-inference#3689) Integrated into release/v3.8.23
…ution (cheaper-inference#3692) Integrated into release/v3.8.23
…e#3698) Integrated into release/v3.8.23
Integrated into release/v3.8.23
…per-inference#3685) (cheaper-inference#3702) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…per-inference#3696) (cheaper-inference#3703) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…er-inference#3660) Integrated into release/v3.8.23
…API (cheaper-inference#3712) Integrated into release/v3.8.23. Vertex dynamic model discovery — surfaces image models (imagen-*, gemini-*-image), embeddings and audio from the live Generative Language catalog, with cached→static fallback and the shared parseGeminiModelsList helper. Validated: parser test 5/5, typecheck:core clean.
Integrated into release/v3.8.23. Makes the cheaper-inference#3588 reasoning token buffer safe and configurable: only inflates max_tokens when the model has a known, non-default output cap and the buffered value fits inside it; otherwise preserves/clamps the client limit. Adds the reasoningTokenBufferEnabled kill switch (default ON). Validated: combo-routing-engine 81/81, combo-config 25/25, combo-quality-validator-reasoning 12/12, phase1f 10/10, typecheck:core clean.
…4062→3408 LOC (-654) (cheaper-inference#3717) Phase 1g-1j of cheaper-inference#3501: client 4062→3408 LOC. Pure extraction (ProviderPlaygroundPanel, useCommandCodeAuth, useExternalLinkFlow+ExternalLinkModal, useAuthFileHandlers) + loadConnProxies ReferenceError fix + phase1f test path fix. Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com>
…3408→2553 LOC (-855) (cheaper-inference#3721) Phase 1k-1m of cheaper-inference#3501: client 3408→2553 LOC. Pure extraction (useModelImportHandlers+ImportProgressModal, useModelVisibilityHandlers, ProviderModelsSection). Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com>
….8.20 release branch The fix itself reached main pre-tag via cherry-pick cheaper-inference#3591, but its changelog bullet (commit e33fdd4) only ever existed on release/v3.8.20 after the squash-merge. Restored under [3.8.20] per the 2026-06-12 release-branch leftover audit (_tasks/release-audit/release-leftovers-audit-2026-06-12.md).
…rofileArn (cheaper-inference#3722) Integrated into release/v3.8.23
…2553→1376 LOC (-1177) (cheaper-inference#3725) Phase 1n-1s of cheaper-inference#3501: client 2553→1376 LOC. Pure extraction (ConnectionsListPanel, ConnectionsHeaderToolbar, ZedImportCard, BatchTestResultsModal, AdaptaTutorialModal, useApiKeySave + helpers). Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com>
…cation, and success-decay recovery (cheaper-inference#3629) Integrated into release/v3.8.23
…6→781 LOC (≤800 TARGET REACHED ✅) (cheaper-inference#3727) Phase 1t of cheaper-inference#3501: client 1376→781 LOC (≤800 reached). Original god-component 12,882→781 (−94%). Co-authored-by: oyi77 <14921983+oyi77@users.noreply.github.com>
…opencode' CLI command (cheaper-inference#3726) Integrated into release/v3.8.23
…erence#3724) Integrated into release/v3.8.23
…eaper-inference#3288) (cheaper-inference#3723) Integrated into release/v3.8.23
… every chatHelpers import cheaper-inference#3692 added a lazy 'await import(proxyEgress)' for egress-IP visibility inside safeLogEvents, which is a sync function — an ES syntax error. It went unnoticed because typecheck:core does not cover src/sse and no test in the merge gates loaded chatHelpers via tsx; any consumer that did (chat-context-relay and chat-route-coverage suites, integration harnesses) failed at module load with 'await can only be used inside an async function'. safeLogEvents is fire-and-forget logging with an outer try/catch, so making it async (and 'void'-ing the single chat.ts call site) preserves behavior exactly. Validation: tests/unit/chat-context-relay.test.ts + chat-route-coverage.test.ts went from failing-at-load to green (+14 tests destravados).
… + combo/proxy audit fixes (cheaper-inference#3699) Integrated into release/v3.8.23
…ops 403ing (cheaper-inference#3728) Integrated into release/v3.8.23
… auto-hide (cheaper-inference#3729) Integrated into release/v3.8.23
…ce#3594) - extracted all generic helpers to utils, config, types, antigravityHelpers, customNormalizers - extracted 17 provider-specific model discovery handlers to handlers/ directory - reduced route.ts from 2,286 lines to ~200 lines
…er instead of using undefined toLocalCatalogModels
99c547f to
31e867a
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const url = `https://api.cloudflare.com/client/v4/accounts/${accountId}/ai/models/search`; | ||
|
|
||
| try { | ||
| const res = await fetch(url, { |
There was a problem hiding this comment.
CRITICAL: Cloudflare model discovery bypasses safe outbound fetch
This handler still uses raw fetch() for an outbound provider URL, which bypasses the repository's outbound URL guard and proxy configuration. Use safeOutboundFetch() with the models discovery preset, guard: getProviderOutboundGuard(), and proxyConfig: proxy to match the surrounding handlers.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| await resolveAntigravityVersion(); | ||
| await ensureAntigravityProjectAssigned( | ||
| accessToken, | ||
| fetch, |
There was a problem hiding this comment.
CRITICAL: Antigravity bootstrap bypasses safe outbound fetch
ensureAntigravityProjectAssigned receives global fetch, so the bootstrap loadCodeAssist request can bypass safeOutboundFetch URL guard and proxy handling. Pass a guarded fetch wrapper or change the helper call site to use safeOutboundFetch with the models discovery preset, guard, and proxy config.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| maybeReturnCachedDiscovery: () => NextResponse | null; | ||
| maybeReturnAutoFetchDisabled: () => NextResponse | null; | ||
| buildDiscoveryFallbackResponse: (args: { cacheWarning?: string; localWarning?: string; error?: any }) => NextResponse | null; | ||
| buildDiscoveryErrorFallbackResponse: (error: any, message: string) => NextResponse | null; |
There was a problem hiding this comment.
CRITICAL: Context callback type does not match implementation
ModelsRequestContext.buildDiscoveryErrorFallbackResponse is typed as (error: any, message: string), but route.ts implements (error: unknown, warnings?: { cacheWarning?: string; localWarning?: string }). This hides mismatches and allows handlers to pass the wrong second argument without type feedback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| console.warn(`[models] All endpoints failed for ${provider}, using local catalog`); | ||
| models = toLocalCatalogModels(); |
There was a problem hiding this comment.
CRITICAL: Undefined local helper reference
toLocalCatalogModels is referenced here but is neither imported nor defined in this handler. It used to be a local helper inside route.ts, so invoking this handler would throw a ReferenceError.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return buildResponse({ | ||
| provider, | ||
| connectionId, | ||
| models: toLocalCatalogModels(), |
There was a problem hiding this comment.
CRITICAL: Undefined local helper reference
toLocalCatalogModels is referenced here but is neither imported nor defined in this handler. It used to be a local helper inside route.ts, so invoking this handler would throw a ReferenceError.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| let response: Response; | ||
| try { | ||
| response = await safeOutboundFetch(buildSapModelsUrl(baseUrl), { |
There was a problem hiding this comment.
CRITICAL: Missing import for buildSapModelsUrl
This handler calls buildSapModelsUrl() but the import only brings in getProviderBaseUrl and buildOptionalBearerHeaders. Add the missing import or this handler will fail when invoked.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…tpProxyConfig and any usage
| localWarning?: string; | ||
| error?: unknown; | ||
| }) => NextResponse | null; | ||
| buildDiscoveryErrorFallbackResponse: (error: unknown, message: string) => NextResponse | null; |
There was a problem hiding this comment.
CRITICAL: Type signature mismatch — buildDiscoveryErrorFallbackResponse is typed with message: string as second parameter, but all call sites (route.ts:lines 1278, 1500, 1629, etc.) and the actual implementation call it with warnings?: { cacheWarning?: string; localWarning?: string }. This will cause TypeScript errors and potential runtime failures.
| return buildResponse({ | ||
| provider, | ||
| connectionId, | ||
| models: toLocalCatalogModels(), |
There was a problem hiding this comment.
CRITICAL: Undefined function reference — toLocalCatalogModels() is called on this line but is neither imported nor defined in this handler. This function is defined locally within route.ts and is not available in the handler scope. This will cause a ReferenceError at runtime.
|
|
||
| export async function handleClaudeModels(ctx: ModelsRequestContext): Promise<any> { | ||
| const { provider, connectionId, connection, apiKey, accessToken, proxy, id, maybeReturnCachedDiscovery, maybeReturnAutoFetchDisabled, buildDiscoveryFallbackResponse, buildDiscoveryErrorFallbackResponse, buildApiDiscoveryResponse, buildResponse, buildLocalCatalogResponse } = ctx; | ||
| return buildResponse({ |
There was a problem hiding this comment.
CRITICAL: Handler returns static models instead of discovering from the actual Anthropic API. The handleClaudeModels function should call https://api.anthropic.com/v1/models with the API key to fetch the live model catalog, not return hardcoded static models. This defeats the purpose of model discovery for Claude providers.
|
Thanks @oyi77 for the modularization work on
What would let this land: re-cut it as a single, independent PR branched from the current |
This PR addresses another "Medium Priority" file from Issue #3594 by modularizing the massive `src/app/api/providers/[id]/models/route.ts` file (2,286 lines).
The file has been heavily refactored to support the upcoming Plugin System.
Changes:
Fixes #3594