-
Notifications
You must be signed in to change notification settings - Fork 904
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement ToolCallStep & Fix transition after PromptStep #513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 5ab9e3a in 27 seconds
More details
- Looked at
176
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/tool_call_step.py:48
- Draft comment:
Replaceprint
statements with proper logging for better production code practices. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment addresses a change made in the diff, specifically the addition of print statements. Using logging instead of print statements is a widely accepted practice for better control over output, especially in production environments. This comment is actionable and clear, suggesting a code quality improvement.
The comment might be considered obvious to experienced developers, but it still provides a valid suggestion for improving code quality. It doesn't address a critical issue, but it is relevant to the changes made.
Even if the suggestion is somewhat obvious, it is still a valid and actionable improvement to the code quality, which aligns with the rules for keeping comments.
Keep the comment as it provides a valid suggestion for improving code quality by replacing print statements with logging.
2. agents-api/agents_api/activities/task_steps/tool_call_step.py:27
- Draft comment:
Consider replacing assertions with proper error handling to provide informative error messages and avoid abrupt termination in production. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change made in the diff, specifically the use of assertions. Assertions can be useful during development but may not be suitable for production code where proper error handling is preferred. The comment is actionable and suggests a code quality improvement.
The comment does not specify what kind of error handling should be used, which might make it less actionable. However, the suggestion to replace assertions is a common practice for production code.
While the comment could be more specific, it still points out a valid concern about using assertions in production code, which is a recognized best practice.
The comment is valid and suggests a code quality improvement by replacing assertions with error handling. It should be kept.
3. agents-api/agents_api/activities/task_steps/tool_call_step.py:34
- Draft comment:
Consider replacing assertions with proper error handling to provide informative error messages and avoid abrupt termination in production. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/activities/task_steps/tool_call_step.py:14
- Draft comment:
Thegenerate_call_id
function is well-implemented for generating secure and unique identifiers usingsecrets
andbase64
. - Reason this comment was not posted:
Confidence changes required:0%
Thegenerate_call_id
function usessecrets
andbase64
to generate a unique ID. This is a good practice for generating secure and unique identifiers.
5. agents-api/agents_api/common/protocol/tasks.py:75
- Draft comment:
Verify that the new transition fromwait
tostep
invalid_transitions
aligns with the intended workflow logic. - Reason this comment was not posted:
Confidence changes required:50%
Thevalid_transitions
dictionary intasks.py
was updated to include a new transition fromwait
tostep
. This change should be verified to ensure it aligns with the intended workflow logic.
6. agents-api/agents_api/workflows/task_execution/__init__.py:456
- Draft comment:
Verify that handlingToolCallStep
by entering a wait-for-input step aligns with the intended workflow logic. - Reason this comment was not posted:
Confidence changes required:50%
Thetool_call_step
function intask_execution/__init__.py
now handlesToolCallStep
by entering a wait-for-input step. This change should be verified to ensure it aligns with the intended workflow logic.
Workflow ID: wflow_3KjONhYlH04aXtea
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 0552f80 in 24 seconds
More details
- Looked at
165
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. agents-api/agents_api/activities/task_steps/tool_call_step.py:49
- Draft comment:
Consider setting thenext
attribute inStepOutcome
if a transition to the next step is expected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative as it suggests an action based on an assumption that a transition might be expected. The current code does not indicate any need for anext
attribute, and the comment does not point out a definite issue. Therefore, it seems unnecessary to keep this comment.
I might be missing some context about howStepOutcome
is used elsewhere in the codebase, which could make the comment relevant. However, based on the provided code, there is no evidence that thenext
attribute is needed.
Given the rules, I should focus on the evidence within the provided code. Since there's no indication that thenext
attribute is required, the comment should be removed.
Remove the comment as it is speculative and not based on any evidence from the provided code that anext
attribute is needed.
2. agents-api/agents_api/activities/task_steps/prompt_step.py:22
- Draft comment:
Remove unnecessary blank line to improve code readability. - Reason this comment was not posted:
Confidence changes required:10%
Theprompt_step.py
file has an unnecessary blank line at line 22. Removing it can improve code readability.
Workflow ID: wflow_QV6whnstH40tGT0i
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.
|
||
tools = context.execution_input.tools | ||
|
||
assert tool_name in [tool.name for tool in tools], f"Tool {tool_name} not found" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using assert
for runtime checks is not recommended as it can be disabled in production. Consider raising an exception instead for better error handling.
async def tool_call_step(context: StepContext) -> StepOutcome: | ||
assert isinstance(context.current_step, ToolCallStep) | ||
|
||
tool_type, tool_name = context.current_step.tool.split(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure context.current_step.tool
contains a .
to avoid ValueError
during unpacking. Consider adding error handling for this case.
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Implement `ToolCallStep` and fix transition logic after `PromptStep` in workflow execution. > > - **ToolCallStep Implementation**: > - Implements `tool_call_step()` in `tool_call_step.py` to handle tool calls, including generating a call ID and validating tool names. > - Updates `STEP_TO_ACTIVITY` in `task_execution/__init__.py` to map `ToolCallStep` to `tool_call_step()`. > - **PromptStep Transition Fix**: > - Updates transition logic in `task_execution/__init__.py` to handle tool calls after a `PromptStep`. > - Removes unused code related to tool calls in `PromptStep` handling. > - **State Machine Update**: > - Updates `valid_transitions` in `tasks.py` to allow 'wait' to 'step' transition. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=julep-ai%2Fjulep&utm_source=github&utm_medium=referral)<sup> for 5ab9e3a. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
Important
Implements
ToolCallStep
and fixes transition logic afterPromptStep
in workflow execution.tool_call_step()
intool_call_step.py
to handle tool calls, generating call ID and validating tool names.STEP_TO_ACTIVITY
intask_execution/__init__.py
to mapToolCallStep
totool_call_step()
.task_execution/__init__.py
to handle tool calls after aPromptStep
.PromptStep
handling.valid_transitions
intasks.py
to allow 'wait' to 'step' transition.This description was created by
for 0552f80. It will automatically update as commits are pushed.