fix(agents-api): properly exit a subworkflow with return step #1186
Conversation
This file contains 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
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
WalkthroughThis PR enhances the task execution workflow by introducing a Changes
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
Comment on lines
285
to
287
|
|
||
| return result | ||
|
|
||
| return WorkflowResult(state=PartialTransition(output=result)) |
There was a problem hiding this comment.
The execute_map_reduce_step function has an empty line before the return statement that could lead to unreachable code and incorrect results being returned.
📝 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.
Suggested change
| return result | |
| return WorkflowResult(state=PartialTransition(output=result)) | |
| return WorkflowResult(state=PartialTransition(output=result)) |
![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 b4f5b24 in 2 minutes and 17 seconds
More details
- Looked at
567lines of code in5files - Skipped
0files when reviewing. - Skipped posting
17drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:139
- Draft comment:
Introduces WorkflowResult with a concise docstring. Consider adding more detailed type hints or examples if future extensions are expected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The existing type hints are already quite specific - state is a PartialTransition, returned is a bool with a clear purpose, and metadata is a flexible dict[str, Any]. The comment doesn't point to any specific field that needs better typing. It's a speculative suggestion about "future extensions" without identifying a concrete issue.
Perhaps there could be specific use cases where more detailed typing of the metadata dictionary would be valuable. Maybe the comment author saw patterns in how metadata is used elsewhere in the codebase.
Without specific evidence of how the metadata is used or what future extensions are anticipated, this is just speculation. The current typing is appropriate for the current implementation.
The comment should be deleted as it's speculative and doesn't identify a concrete issue with the current implementation. The existing type hints are appropriate and clear.
2. agents-api/agents_api/routers/tasks/create_task_execution.py:77
- Draft comment:
Adding 'scope_id=uuid7()' in TransitionTarget improves uniqueness. Verify that uuid7 meets the deterministic needs (if any) in workflow transitions. - 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. agents-api/agents_api/workflows/task_execution/__init__.py:239
- Draft comment:
In _handle_ReturnStep the workflow now returns a WorkflowResult with returned=True. Confirm that calling transition before returning is safe here; the new implementation changes the control flow. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm the safety of a change in control flow, which violates the rule against asking for confirmation of intentions or safety. It does not provide a specific suggestion or ask for a test to be written. Therefore, this comment should be removed.
4. agents-api/agents_api/workflows/task_execution/__init__.py:658
- Draft comment:
The handle_step method now explicitly returns a WorkflowResult. This clearer typing is a best practice; ensure all calling methods properly unwrap the 'state' field as needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that all calling methods properly unwrap a field. This is a general request for verification and does not provide a specific suggestion or point out a specific issue in the code. It violates the rule against asking the author to ensure behavior is intended or tested.
5. agents-api/agents_api/workflows/task_execution/__init__.py:790
- Draft comment:
After checking if workflow_result.returned is true, a transition is executed before returning. Confirm that this transition is intended to be executed synchronously and that its side effects are expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm their intention regarding the execution of a transition. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific code suggestion or point out a clear issue.
6. agents-api/agents_api/workflows/task_execution/helpers.py:227
- Draft comment:
The execute_foreach_step function now checks for 'result.returned' in the loop. Ensure that returning early from a foreach iteration correctly propagates the WorkflowResult upstream. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a specific behavior is correct, which violates the rule against asking the author to ensure behavior is intended. It doesn't provide a specific suggestion or point out a clear issue.
7. agents-api/tests/test_task_execution_workflow.py:100
- Draft comment:
Test assertions now expect a WorkflowResult instead of a PartialTransition. This is good for clear contract testing; verify that all tests consistently check both state and returned flag. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify that all tests are consistent with the new expectations. It doesn't provide a specific suggestion or point out a specific issue, which violates the rule against asking the author to ensure things are tested or verified.
8. agents-api/tests/test_task_execution_workflow.py:667
- Draft comment:
Test for yield step expressions checks transition_to tuple with generated scope_id. Consider documenting the expected behavior for yield steps, since the scope_id is generated dynamically. - 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. agents-api/tests/test_task_execution_workflow.py:1417
- Draft comment:
The tool call evaluation for function tool now includes a generated call ID ('XXXX') in the output. Ensure this mechanism is robust and that the test accounts for any future changes in call ID generation. - 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. agents-api/tests/test_task_execution_workflow.py:100
- Draft comment:
Instead of asserting full equality with a WorkflowResult, consider comparing only its essential fields (like output and type). This makes the test less brittle if additional metadata is added later. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
This is a reasonable suggestion about test brittleness - comparing only essential fields can make tests more maintainable. However, in this case, WorkflowResult seems to be a simple wrapper with just a state field, and the test is explicitly verifying the exact structure that should be returned. Making the assertion more granular could actually make the test less clear about the expected return type.
The comment has a valid point about test brittleness. If WorkflowResult gains more fields in the future, tests would need updates even if the core behavior hasn't changed.
While the brittleness concern is valid, the current assertion clearly shows the exact expected return type and structure, which is valuable for test readability and documentation. The suggested change could obscure the fact that a WorkflowResult is expected.
The comment should be deleted. While it makes a reasonable suggestion, the current assertion style has benefits in terms of clarity and documentation that outweigh the brittleness concerns.
11. agents-api/tests/test_task_execution_workflow.py:217
- Draft comment:
Use substring or regex matching for the exception message instead of checking full equality. This avoids brittleness if the formatting or additional context is added in the error message. - 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. agents-api/tests/test_task_execution_workflow.py:672
- Draft comment:
When using multiple side effects with workflow.execute_activity, ensure the order and expected calls are documented. A change in call order may break the test unexpectedly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that the order and expected calls are documented, which is similar to asking them to double-check or ensure something. This violates the rule against asking the author to ensure behavior is intended or tested. The comment does not provide a specific code suggestion or ask for a specific test to be written, which would have been acceptable.
13. agents-api/tests/test_task_execution_workflow.py:1267
- Draft comment:
The log step test checks the output of a log expression. Consider adding a scenario with multiple log messages or edge cases to better verify the aggregation behavior if it's expected to evolve. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
14. agents-api/tests/test_task_execution_workflow.py:1425
- Draft comment:
The test for evaluating tool call expressions depends on generate_call_id returning a fixed value ('XXXX'). Consider abstracting or injecting the call ID generator to avoid tight coupling between tests and implementation. - 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.
15. agents-api/tests/test_task_execution_workflow.py:1500
- Draft comment:
In the yield expressions test, the assertion extracts the dynamic scope_id from the transition target. Consider isolating dynamic components in the assertion so that changes in runtime-generated values don't break the test unnecessarily. - 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.
16. agents-api/tests/test_task_execution_workflow.py:1514
- Draft comment:
Overall, the test suite provides comprehensive coverage of the workflow step handling. As the workflow complexity grows (e.g., with nested foreach or map-reduce steps), consider adding integration tests to ensure long-term maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
17. agents-api/agents_api/workflows/task_execution/helpers.py:123
- Draft comment:
Typo: The variable 'seprated_workflow_name' should be spelled 'separated_workflow_name' for consistency and clarity. - 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.
Workflow ID: wflow_I5YId3Oq3nJNKWmE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| batch_result for batch_result in batch_results if batch_result.returned | ||
| ) | ||
|
|
||
| batch_results = [batch_result.state.output for batch_result in batch_results] |
There was a problem hiding this comment.
can this be refactored somehow to not traverse batch_results twice?
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.
PR Type
Bug fix, Enhancement, Tests
Description
Introduced a new
WorkflowResultmodel to encapsulate workflow execution results.Replaced
PartialTransitionwithWorkflowResultacross workflow handling methods.Enhanced subworkflow handling to properly exit with return steps.
Updated and added tests to validate
WorkflowResultintegration.Changes walkthrough 📝
tasks.py
Introduced `WorkflowResult` model for workflow resultsagents-api/agents_api/common/protocol/tasks.py
WorkflowResultmodel to represent workflow execution results.WorkflowResult.create_task_execution.py
Added `scope_id` to transition initializationagents-api/agents_api/routers/tasks/create_task_execution.py
scope_idtoTransitionTargetinitialization.__init__.py
Refactored workflow execution to use `WorkflowResult`agents-api/agents_api/workflows/task_execution/init.py
PartialTransitionwithWorkflowResultin step handlers.WorkflowResult.runmethod to handleWorkflowResulttransitions.helpers.py
Updated helpers to integrate `WorkflowResult`agents-api/agents_api/workflows/task_execution/helpers.py
WorkflowResult.steps.
test_task_execution_workflow.py
Updated tests for `WorkflowResult` integrationagents-api/tests/test_task_execution_workflow.py
WorkflowResultusage.PartialTransitionassertions withWorkflowResult.Important
Introduces
WorkflowResultto encapsulate workflow execution results, updating step handlers and tests to use this new class.WorkflowResultclass intasks.pyto encapsulate workflow execution results, including metadata and areturnedflag._handle_LogStep,_handle_ReturnStep,_handle_SwitchStep,_handle_IfElseWorkflowStep,_handle_ForeachStep,_handle_MapReduceStep,_handle_SleepStep,_handle_EvaluateStep,_handle_YieldStep,_handle_WaitForInputStep,_handle_PromptStep,_handle_SetStep,_handle_GetStep,_handle_ToolCallStepintask_execution/__init__.pyto returnWorkflowResult.run()intask_execution/__init__.pyto handleWorkflowResultand transition onreturnedflag.execute_foreach_step,execute_map_reduce_step,execute_map_reduce_step_parallelinhelpers.pyto returnWorkflowResult.test_task_execution_workflow.pyto assertWorkflowResultinstead ofPartialTransition.This description was created by
for b4f5b24. It will automatically update as commits are pushed.
EntelligenceAI PR Summary
Purpose:
Changes:
WorkflowResultclass to encapsulate workflow execution results, replacingPartialTransitionreturns.scope_idtoTransitionTargetfor improved tracking of workflow execution context.WorkflowResult.Impact: