Nate/bedrock-oai-adapter #5936
Nate/bedrock-oai-adapter #5936
Conversation
|
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev canceled.
|
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
| type: "tool_use" as const, | ||
| id: toolCall.id, | ||
| name: toolCall.function?.name, | ||
| input: JSON.parse(toolCall.function?.arguments || "{}"), |
There was a problem hiding this comment.
Unsafe JSON parsing: The code directly parses potentially malformed JSON without error handling. If toolCall.function.arguments contains invalid JSON, this will throw an unhandled runtime error. Should wrap in try-catch and handle parsing failures gracefully.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
| env: z.object({ | ||
| region: z.string().optional(), | ||
| profile: z.string().optional(), | ||
| modelArn: z.string().optional(), |
There was a problem hiding this comment.
The modelArn field is marked as optional but it is required for AWS Bedrock to know which foundation model to use. Making this optional will lead to runtime errors when the field is not provided. The schema should make this field required by removing .optional().
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
|
😱 Found 2 issues. Time to roll up your sleeves! 😱 |
Description
Bedrock OpenAI adapter for use in proxy. Purposefully re-implemented existing Bedrock.ts as that should eventually just use the openai-adapters implementation
Checklist
Tests
Some test were added outside the usual oai-adapters tests, but this was also tested locally with the usual test suite in main.test.ts. Won't be running in CI though