feat: add additional checks for openai stream requires action #936
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 38317cc in 11 seconds
More details
- Looked at
59lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/frameworks/openai.ts:171
- Draft comment:
The check for run status before submitting tool outputs is redundant here since it's already checked in the while loop condition. Consider removing it to avoid unnecessary checks. - Reason this comment was not posted:
Confidence changes required:50%
The check for run status before submitting tool outputs is repeated unnecessarily.
2. js/src/frameworks/openai.ts:145
- Draft comment:
The check for run status before submitting tool outputs is redundant here since it's already checked in the while loop condition. Consider removing it to avoid unnecessary checks. - Reason this comment was not posted:
Confidence changes required:50%
The check for run status before submitting tool outputs is repeated unnecessarily.
Workflow ID: wflow_DwerCMGd59ZR2D0E
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 56b8836 in 26 seconds
More details
- Looked at
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_t2P1uCKokdBErPoT
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Code Review SummaryOverall Assessment: ✅ ApprovedThe changes improve the robustness of the OpenAI integration by adding important validation checks for stream states. The code quality is good with proper error handling and state validation. Strengths:
Suggestions for Enhancement:
The code is well-structured and the changes make the system more reliable. Good to merge after considering the minor suggestions. |
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cc7cf36 in 16 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/frameworks/openai.ts:145
- Draft comment:
The check for valid run statuses should include 'queued' and 'in_progress' as mentioned in the PR description. Consider updating the condition to:
if (["queued", "in_progress", "requires_action"].includes(currentRun.status)) {
- Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. js/src/frameworks/openai.ts:173
- Draft comment:
The check for valid run statuses should include 'queued' and 'in_progress' as mentioned in the PR description. Consider updating the condition to:
if (["queued", "in_progress", "requires_action"].includes(runInfo.status)) {
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_GK2WavtpYfneHj4l
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 1a8c915 in 26 seconds
More details
- Looked at
55lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/models/actions.ts:250
- Draft comment:
Ensure error messages are consistent. Consider using the same message as in line 192 for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The error message in line 250 is slightly different from the one in line 192. Consistency in error messages is important for clarity and maintainability.
Workflow ID: wflow_kMAfxihYDSmY9yC1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| }); | ||
| // Check if the run is still in a state that accepts tool outputs | ||
| const currentRun = await client.beta.threads.runs.retrieve(thread.id, runId); | ||
| if (currentRun.status === "requires_action") { |
There was a problem hiding this comment.
Chore: (Not important)Can we use constants for things like this? Easy to maintain and scope of error is also reduced
Important
Add checks for valid run statuses in
openai.tsand introducefilterByAvailableAppsproperty with validation inactions.ts.waitAndHandleAssistantStreamToolCalls()andwaitAndHandleAssistantToolCalls()inopenai.tsto submit tool outputs only if run status isqueued,in_progress, orrequires_action.ifconditions forrunIdchecks inwaitAndHandleAssistantStreamToolCalls()andwaitAndHandleAssistantToolCalls().filterByAvailableAppsboolean property toAdvancedUseCaseSearchQueryDTOinschemas.gen.tsandtypes.gen.ts.findActionEnumsByUseCase()inactions.tsto ensurefilterByAvailableAppsandappsare not both provided.This description was created by
for 1a8c915. It will automatically update as commits are pushed.