Repository navigation
refactor(ai-gateway): split the LLM proxy handler into phases - #9717
chrarnoldus wants to merge 11 commits into
Conversation
…drop stale comments
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe phase split is behaviorally faithful to the previous handler: I traced every extracted statement (parse/validate, model resolution, provider and policy, respond) against the base revision and found no dropped checks, reordered side effects that change responses, or new unhandled promises. Assumptions: Files Reviewed (8 files)
Reviewed by deepseek-flash-latest · Input: 134.5K · Output: 31.2K · Cached: 2.9M Review guidance: REVIEW.md from base branch |
Summary
handleLlmProxyRequestwas a single ~1,090-line function and the gateway's top churn hotspot. This splits it into phases. Each extracted phase returns{ ok: true, ... } | { ok: false, response }, except model resolution, which returns the result type described below. No generic pipeline abstraction is introduced.The auth, balance, group-policy, guardrail and bouncer promises still start at the same points and overlap as before. Auth still starts before the body is parsed, so it overlaps
JSON.parseof large bodies. Several invalid requests now return before anything awaits auth, so a no-opcatchkeeps an auth failure on those paths from becoming an unhandled rejection. Malformed JSON already had this exposure.Phases
llm-proxy.tshandlers/llm-proxy-request.ts→parseLlmProxyRequestproviderOptions, model, messages/input, provider privacy, missing client IP, Responsesstore/previous_response_id. AlsoreadLlmProxyHeaders, which reads each header once.handlers/llm-proxy-model-resolution.ts→resolveModelx-kilo-auto-decision, catalog-mode mapping. AlsoautoRoutingFailureResponseandbillLegacyClassifierCost.handlers/llm-proxy-provider-policy.ts→resolveProviderAndPolicygetProvider, Organization Auto target checks, API kind, unavailable model, balance, org access policy, data-collection check, custom LLM provider intersectionsendUpstreamAttempthandlers/llm-proxy-respond.ts→respondToUpstreammakeErrorReadable,rewriteModelResponseThe orchestrator is about 430 lines, more than the ~150 originally estimated. Ingress, context, admission and upstream stay inline because extracting them would only move code around. Most of the remaining length is the auth outcome, balance and usage-context code. A parallel PR that adds shared admission helpers (
resolveGatewayCaller) replaces that code, so this PR leaves it as is to keep the two PRs from conflicting.Auto-routing result type
The five mutable auto-routing variables (
autoModel,routingTarget,legacyClassifierCostUsd,autoRoutingUnavailable,isAutoRoutedRequest) and the closures that wrote them are gone.resolveModelreturns one discriminated result:AutoRoutingFailurenames each failure:invalid_token,auth_required,decision_secret_missing,invalid_auto_decision,no_free_models_available,organization_auto_configuration,custom_routerorunavailable. The signedx-kilo-auto-decisionbranch returns the same type:routedwithpoolRouted: falseand no classifier cost, orrejected.unavailableis answered later than the other failures, as before. It yields to the caller's auth failure, and the legacy classifier cost it carries is billed first. The other failures are answered right after resolution. Legacy positive-cost classifier billing still works and reads its cost from the result. Deleting it is a separate change that needs the deployed worker versions confirmed first. The provider and policy phase readsroutingTargetandpoolRoutedfrom the result.applyResolvedAutoModelis unchanged. Its signature already fits the phase, and keeping it avoids rewriting the many handler tests that mock it.Other changes (one commit each)
autoRoutingDenialsalong with itscandidateIdsparameter. It could never run: group policy is null only when the caller has no organization, and thenplanis undefined. Removed the test that reached it only through impossible mocks.MAX_TOKENS_LIMIT = 99999999999). It logged the full prompt throughconsole.warn, which Sentry Logs collects. It also returned a retryable 503 for a client error, and with that limit it protected nothing.getMaxTokensis now called once.taskId ?? sessionHeader) and the editor-name header are each read once.effectiveProviderContextalias and the single-usecreateAccessCheckResolverfactory, along with stale comments.validatePathaccepts only/api/v1/....apps/web/ai-gateway-rewrites.mjsalways rewrites/api/gatewayand/api/openrouterto/api/v1, andapps/ai-gatewayhas only/api/v1routes.invalidPathResponsenow lists all three paths in itsmessage; the status anderror_typeare unchanged. The tests' default prefix is/api/v1, a new test covers each rejected legacy prefix, andapps/ai-gateway/AGENTS.mdis updated.Error precedence changes
These matter only when several errors apply to one request:
store/previous_response_idnow returns 400 before any auth, auto-routing or rate-limit error. Before, it ran after auto-routing, so a rejected request could still call the paid classifier. The free-model rate limit 429, invalid-token and paid-model-auth 401s, auto-routing errors and the missing-IP 400 also came before it. A new test asserts that the classifier is not called.providerprivacy fields already returned 400 before any other error could occur, so the only change is that the check no longer waits for the balance read. A failed balance read no longer comes first.console.debugandsetTag. The response is unchanged.isUnavailableModelstays where it was, after the free-model rate limit, because it does I/O. The test that expects the Gemma free-model 429 before the unavailable-model error still passes unchanged.Owner decision, deferred
body.provideris still replaced wholesale by the organization's effective provider config when organization policy applies (request.body.provider = effectiveProviderConfiginresolveProviderAndPolicy). Tests lock this behavior in, and whether to merge the caller's provider object instead is an owner decision, so this PR does not change it.Not touched
providers/get-provider.ts, which a parallel PR is refactoring.Verification
pnpm --filter @kilocode/web-shared typecheck: passesllm-proxy.test.ts: 219 tests pass. Changes are the removed dead-branch test, the/api/v1default prefix and legacy-prefix test, and new precedence tests.packages/web-shared/src/lib/ai-gateway/**, includingauto-model/*,auto-routing-*, and theapps/ai-gatewayroute tests): 164 suites pass. One DB-backed suite (auto-free-config.test.ts) timed out during the parallel run, which shares a local database with other sessions, and passed when rerun on its own.pnpm exec oxfmt --checkandoxlint --config .oxlintrc.jsonon changed files: clean.git diff --check: clean.Models
Written with
kilo/anthropic/claude-opus-5.5. A review subagent ran on the same model with no override.