Merged
Conversation
Co-authored-by: jason <jason@jxnl.co>
|
Cursor Agent can help with this pull request. Just |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
instructor | 8ad4b0b | Commit Preview URL Branch Preview URL |
Jan 16 2026, 02:03 PM |
Co-authored-by: jason <jason@jxnl.co>
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8ad4b0b in 1 minute and 24 seconds. Click for details.
- Reviewed
146lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft 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. instructor/providers/openai/utils.py:31
- Draft comment:
Consider using a list comprehension in _filter_responses_tool_calls for conciseness and adding a comment that explicitly skips items (e.g. reasoning items) that don’t have an 'arguments' attribute. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
2. instructor/providers/openai/utils.py:44
- Draft comment:
The helper _format_responses_tool_call_details cleanly extracts details using getattr. Consider adding type annotations for the 'tool_call' parameter to improve clarity. - Reason this comment was not posted:
Confidence changes required:20%<= threshold85%None
3. instructor/providers/openai/utils.py:134
- Draft comment:
Ensure that _filter_responses_tool_calls reliably filters out items without the 'arguments' attribute so that accessing tool_call.arguments won’t cause runtime errors. The current design appears correct, but a brief inline note could clarify this intent. - Reason this comment was not posted:
Confidence changes required:20%<= threshold85%None
4. tests/test_streaming_reask_bug.py:138
- Draft comment:
The test 'test_reask_responses_tools_skips_reasoning_items_and_includes_details' effectively validates that reasoning items are skipped and that tool call details are appended. Consider adding an additional case with multiple valid tool calls to further strengthen coverage. - Reason this comment was not posted:
Confidence changes required:20%<= threshold85%None
Workflow ID: wflow_vKnvg29gv7mJkRg0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: Prevent
reaskresponsestoolsfrom crashing with reasoning itemsDescribe your changes
Adds a check for
hasattr(tool_call, "arguments")in thereask_responses_toolsfunction. This prevents the function from attempting to access the.argumentsattribute onResponseReasoningItemobjects, which do not have it, thus resolving a bug where validation errors were not properly sent back to the LLM during retry attempts when using reasoning models.Issue ticket number and link
567-251: #1957
Checklist before requesting a review
Linear Issue: 567-251
Important
Fixes crash in
reask_responses_toolsby checking forargumentsattribute, ensuring proper handling of reasoning items.hasattr(tool_call, "arguments")check inreask_responses_toolsto prevent crashes withResponseReasoningItemobjects._filter_responses_tool_calls()to filter tool calls inutils.py._format_responses_tool_call_details()to format tool call details inutils.py.test_reask_responses_tools_skips_reasoning_items_and_includes_detailsintest_streaming_reask_bug.pyto verify reasoning items are skipped and tool details are included.This description was created by
for 8ad4b0b. You can customize this summary. It will automatically update as commits are pushed.