fix: surface http errors correctly #2383
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 84bcc06 in 1 minute and 32 seconds. Click for details.
- Reviewed
474lines of code in19files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. integ-tests/typescript/tests/providers/aws.test.ts:20
- Draft comment:
The tests show mixed expectations for errors (e.g. expecting 'DispatchFailure' in one test and 'BamlClientHttpError' in another). Verify that error codes/names remain consistent across AWS error scenarios. Consider standardizing the error output so tests don’t become brittle when internal error strings change. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. integ-tests/typescript/tests/providers/aws.test.ts:25
- Draft comment:
The test for invalid AWS profile expects a rejection with code 'GenericFailure'. Confirm that this error code is the intended output and that the error structure is stable, as other tests rely on 'BamlClientHttpError'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. integ-tests/typescript/tests/providers/aws.test.ts:141
- Draft comment:
The tests for dynamic client configuration and region resolution (using environment variables like AWS_REGION and AWS_DEFAULT_REGION) are good, but be sure to reset or isolate environment variable changes to avoid test interference. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. tools/build:62
- Draft comment:
The build script uses 'cd "$1"' and then eval on constructed commands. Ensure that the input argument is properly validated to avoid any risk from unsanitized inputs. If this script is only used in a controlled CI environment this may be acceptable, but it’s worth commenting on the risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. integ-tests/typescript/tests/abort-handlers.test.ts:12
- Draft comment:
The abort tests using AbortController look solid. Consider adding additional verification that no further messages are received after abort (e.g. by checking the stream stops receiving data) to further strengthen cancellation coverage. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. integ-tests/typescript/tests/prompt_renderer.test.ts:40
- Draft comment:
The test for maintaining field order in JSON is precise but may be brittle if output formatting changes. Consider comparing object keys using deep equality rather than string matching if possible. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
7. integ-tests/baml_src/test-files/abort-handlers/abort-handlers.baml:52
- Draft comment:
The abort-handlers BAML test file is clear. Ensure that the prompts used for failure cases (e.g. in FnFailRetryConstantDelay and FnFailRetryExponentialDelay) fully exercise cancellation paths in the client. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
8. integ-tests/typescript/tests/providers/aws.test.ts:310
- Draft comment:
In the test 'should throw error when using temporary credentials without session token', verify that the client fails gracefully and does not leak sensitive information in error messages. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. integ-tests/typescript/tests/providers/aws.test.ts:371
- Draft comment:
Tests using dynamic client registry cover a wide range of scenarios. It’s good to see tests for invalid region, non-existent model, and profile issues. Double-check that the error matching in rejects.toMatchObject() remains robust as internal error messages may change. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. engine/baml-runtime/src/types/response.rs:148
- Draft comment:
Typo: The TODO comment contains "dont" which should be "don't" for proper grammar. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment is pointing out a grammatical error in a TODO comment. While this is a minor issue, correcting typos can improve code readability and maintainability. However, it doesn't directly impact the functionality of the code.
11. integ-tests/typescript-esm/baml_client/inlinedbaml.ts:8
- Draft comment:
Typo: In the comment "tests whether k1 doesnt incorrectly get matched with k11", consider changing "doesnt" to "doesn't". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. integ-tests/typescript/tests/prompt_renderer.test.ts:40
- Draft comment:
Consider revising the test description text "maintain field order" for improved clarity. Perhaps change it to "maintains field order" to align with standard grammar. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_HPVHHQyIgMpqyeqS
You can customize 
|
🔒 Entelligence AI Vulnerability Scanner ✅ No security vulnerabilities found! Your code passed our comprehensive security analysis. |
Review Summary🏷️ Draft Comments (3)
🔍 Comments beyond diff scope (2)
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| SdkError::TimeoutError(_) => ErrorCode::Other(2), | ||
| SdkError::DispatchFailure(_) => ErrorCode::Other(2), | ||
| SdkError::DispatchFailure(_) => ErrorCode::FailedToConnect, |
There was a problem hiding this comment.
correctness: SdkError::TimeoutError(_) and SdkError::Other(2) are mapped to ErrorCode::Other(2) instead of a more specific error, which may cause connection errors to be misclassified and not surfaced as HTTP errors.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-runtime/src/internal/llm_client/primitive/aws/aws_client.rs, lines 1249-1250, update the error mapping so that both `SdkError::TimeoutError(_)` and `SdkError::DispatchFailure(_)` are mapped to `ErrorCode::FailedToConnect` instead of `ErrorCode::Other(2)`. This ensures connection errors are surfaced as HTTP errors. Change:
SdkError::TimeoutError(_) => ErrorCode::Other(2),
SdkError::DispatchFailure(_) => ErrorCode::FailedToConnect,
to:
SdkError::TimeoutError(_) => ErrorCode::FailedToConnect,
SdkError::DispatchFailure(_) => ErrorCode::FailedToConnect,
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| SdkError::TimeoutError(_) => ErrorCode::Other(2), | |
| SdkError::DispatchFailure(_) => ErrorCode::Other(2), | |
| SdkError::DispatchFailure(_) => ErrorCode::FailedToConnect, | |
| SdkError::TimeoutError(_) => ErrorCode::FailedToConnect, | |
| SdkError::DispatchFailure(_) => ErrorCode::FailedToConnect, |
| match &media.content { | ||
| BamlMediaContent::Url(url_content) => { | ||
| // For URLs, we need to resolve them to base64 first | ||
| anyhow::bail!( | ||
| "BAML internal error (openai): Pdf URL are not supported by OpenAI use base64." | ||
| content.insert( | ||
| payload_key.into(), | ||
| json!({ | ||
| "type": "input_file", | ||
| "file_url": url_content.url | ||
| }), | ||
| ); |
There was a problem hiding this comment.
correctness: to_media_message for BamlMediaType::Pdf with BamlMediaContent::Url inserts a nested map with both file and file_url, which does not match OpenAI's expected schema and may cause request failures.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-runtime/src/internal/llm_client/primitive/openai/openai_client.rs, lines 724-733, the `to_media_message` function for `BamlMediaType::Pdf` with `BamlMediaContent::Url` inserts a map with both `type` and `file_url` under the `file` key, which does not match OpenAI's expected schema and may cause request failures. Please update this block so that only `{ "file_url": url_content.url }` is inserted under the `file` key, not a nested map with `type`.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| match &media.content { | |
| BamlMediaContent::Url(url_content) => { | |
| // For URLs, we need to resolve them to base64 first | |
| anyhow::bail!( | |
| "BAML internal error (openai): Pdf URL are not supported by OpenAI use base64." | |
| content.insert( | |
| payload_key.into(), | |
| json!({ | |
| "type": "input_file", | |
| "file_url": url_content.url | |
| }), | |
| ); | |
| match &media.content { | |
| BamlMediaContent::Url(url_content) => { | |
| content.insert( | |
| payload_key.into(), | |
| json!({ | |
| "file_url": url_content.url | |
| }), | |
| ); | |
| } |
integ-tests fail on
canaryright now in two places (0.206.0 is going out despite these, because these failures have been present since pre-0.205.0):I can't tell when this started happening; looking at the git history, it's hard to figure out what happened here.
#2383 is an attemptat fixing this: it boils down to that we have a lot of tech debt around error handling and it needs a lot of cleanup because I don't think any of us actually understand our error propagation logic. The central pivot point is that
ExposedErroris an attempt at creating a fan-in point for different types of errors - the runtime raisesanyhowerrors and then we try to translate that intoExposedErrorbefore handing it to the FFI layers - but either it isn't sufficiently comprehensive or there's another error type I'm missing.