fix(ai-gateway): apply provider policy before Vercel routing - #5401
Conversation
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
|
@kilocode-bot please review this PR. The automated review check failed with |
There was a problem hiding this comment.
Review: ordering fix looks right, a few things to tighten
I traced the ordering claim and it holds: getProvider early-returns for direct BYOK, Vercel BYOK, experiments and kilo-internal/... custom LLMs before the Vercel branch, so getRoutingProviderConfig is structurally unreachable on those routes — the exemption doesn't depend on the caller remembering anything. And gating on effectiveProviderConfig is consistent with what actually ends up on the wire, because the access-check block later does requestBodyParsed.body.provider = effectiveProviderConfig (a full replacement, so the caller's ignore/order/zdr were already being dropped when an org policy applies) and convertProviderOptions reads that same replaced value. Also confirmed eligibleProviderRoutes and provider_allow_list are both OpenRouter provider slugs, so the intersection and the later openRouterToVercelInferenceProviderId mapping line up.
No blocking correctness bug found. Main concerns, in order:
- The policy lookup is eager, not lazy —
await getRoutingProviderConfig?.()sits in the argument list, so it resolves beforeshouldRouteToVercelruns any of its cheap gates. See the inline comment onget-provider.ts; this is the one I'd actually change. - Silent fallback via default parameter in
shouldRouteToVercel— an explicitly-passedundefinedmeans "use the unpoliced request body". Fine today, fragile as a contract. - Two undocumented behavior changes in the rewritten denial guard (empty org
provider_allow_list, andefficientPoolBlockedResponsenow covering the empty-intersection case). - Failure-mode shift: because provider selection now awaits both
balanceAndSettingsPromiseandorganizationGroupPolicyPromise, a transient failure in either now rejectsgetProvider(→ 500) on paths that previously returned a specific response before ever touching the policy — unavailable model, rules-engine block, unsupported api kind. Failing closed on a policy read is defensible; just worth being deliberate about it, and note thevoid organizationGroupPolicyPromise.catch(() => {})guard no longer describes all consumers.
Test coverage gaps
The one added route test asserts only that the callback exists on the first getProvider call and resolves to { only: [...] }. Not covered:
shouldRouteToVercel's new parameter has no unit test at all —vercel/index.test.tsonly exercises the helpers. The highest-value test for this PR is: policyonlycontains a provider with no compatible Vercel inference provider ⇒false, even thoughrequest.body.provideris empty. That is precisely the bug being fixed, and right now nothing pins it.- The intersection path (org
provider_allow_list∩ groupeligibleProviderRoutes) reaching the callback — the route test'sgetEffectiveModelDecisionmock returns{ allowed: true }with noeligibleProviderRoutes, so the interesting branch inresolveAccessCheckis untested. - Anonymous requests ⇒
getRoutingProviderConfigisundefined. - The quarantine-3 call site resolving the policy for the overridden model id — that's the only reason the cache is keyed by model, and it's the case most likely to regress.
- The stated caching guarantee itself (e.g.
getEffectiveModelDecisioncalled once per distinct model, twice across a quarantine downgrade).
Performance-wise the memoization is sound: at most two entries per request, no cross-request state, no staleness window, and the map holds the promise rather than the resolved value so concurrent callers share one lookup.
|
The follow-up review completed successfully this time — no need to retry. Summary of what happened: the earlier automated review run hit an "Agent wrapper failed" error and posted nothing, but this run succeeded. Posted: review with summary + 6 inline comments Key takeaways:
No further action needed from me unless you want me to kick off a follow-up Cloud Agent session to address any of these. |
|
Pulling the one item I'd actually block on out of the review thread, since it's easy to lose among the nits. Make the policy lookup lazy instead of eager
(await shouldRouteToVercel(
requestedModel,
request,
taskId || user.id,
await getRoutingProviderConfig?.() // <- resolved before shouldRouteToVercel runs at all
))So the laziness the type suggests doesn't exist. For every non-anonymous request that gets past the BYOK / custom-LLM / experiment early returns, this now resolves
The provider config is only consulted after all three pass, and even then only inside It also removes a property the previous code called out explicitly. The comment this PR deleted:
That overlap was the reason Suggested shape — keep the thunk intact through export async function shouldRouteToVercel(
requestedModel: string,
request: GatewayRequest,
randomSeed: string,
getRoutingProviderConfig?: () => Promise<OpenRouterProviderConfig | undefined>
) {
// ... opt-out, percentage, and vercelModels.has checks unchanged ...
const provider = (await getRoutingProviderConfig?.()) ?? request.body.provider;
if (provider && (provider.only || provider.ignore?.length)) {
// ... unchanged ...
}
}and at the call site simply Two side benefits: the JSDoc becomes accurate, and the |
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 files)
Previous Review Summaries (4 snapshots, latest commit 04a5301)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 04a5301)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 files)
Fix these issues in Kilo Cloud Previous review (commit e9383ba)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 5fa207f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 1b79795)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Reviewed by grok-4.6 · Input: 161.7K · Output: 4.7K · Cached: 122.2K Review guidance: REVIEW.md from base branch |
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
Summary
Testing