-
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
fix(agent-api): fix the list of steps accessable by a subworkflow #1131
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Feedback 🧐(Feedback updated until commit c88ae74)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
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 8904e92 in 1 minute and 48 seconds
More details
- Looked at
62
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/agents_api/common/interceptors.py:244
- Draft comment:
Unwrapping ActivityError in a while loop may risk an infinite loop if a cyclical cause chain occurs. Consider adding a max iteration count. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While circular reference in exception cause chains is theoretically possible, Python's exception handling system typically doesn't create circular references in cause. The code also has two safety checks. The risk seems very theoretical rather than practical. The code is following a common pattern for exception unwrapping.
I could be underestimating the risk - if someone deliberately constructs a circular reference in cause, this would hang. Also, even non-circular but very long chains could be problematic.
While technically possible, circular exception causes would be highly artificial and likely only possible in test scenarios. In production code, exception cause chains are naturally acyclic.
This comment raises a very theoretical edge case that is unlikely to occur in practice. The current implementation follows standard exception unwrapping patterns.
2. agents-api/agents_api/workflows/task_execution/transition.py:64
- Draft comment:
Same pattern of unwrapping ActivityError is used here. Consider consolidating unwrapping logic to avoid repetition. - Reason this comment was not posted:
Marked as duplicate.
3. agents-api/agents_api/common/interceptors.py:244
- Draft comment:
Unwrapping nested ActivityError causes is useful, but consider guarding against potential cyclic cause chains (or a very deep chain). - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. agents-api/agents_api/common/protocol/tasks.py:230
- Draft comment:
The new filtering on transitions uses a backtick prefix (f"{workflow}
"). Ensure that this matches the intended subworkflow naming convention and is well documented. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. agents-api/agents_api/workflows/task_execution/transition.py:63
- Draft comment:
Similar error unwrapping logic appears here. Consider refactoring this logic into a shared helper to avoid duplication and ensure consistency. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_SVsfooXNijJtlsRo
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.
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. Incremental review on 7ff5bc7 in 43 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:228
- Draft comment:
Consider replacing the assert with explicit error handling. Assert can be disabled in production, leading to unexpected behavior. E.g., raise an exception if 'transitions' is empty. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_UGEPxg86hUuVy91p
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.
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. Incremental review on 7f5f352 in 54 seconds
More details
- Looked at
48
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:232
- Draft comment:
Using the last transition's workflow as reference assumes all transitions in the list belong to the same workflow. Confirm this assumption is valid. - 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 an assumption about the code's logic. It is not making a specific code suggestion or pointing out a specific issue. This violates the rule against asking the author to confirm their intention or assumptions.
2. agents-api/agents_api/common/protocol/tasks.py:236
- Draft comment:
Awaiting get_workflow_name in the list comprehension could impact performance for large transition lists; consider precomputing workflow names if possible. - 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%
The comment makes a valid technical point - awaiting in a list comprehension means each iteration happens sequentially rather than in parallel. The suggestion to precompute workflow names would improve performance. However, we don't know the typical size of the transitions list or whether this is actually a bottleneck. The comment is speculative without evidence of actual performance impact.
I may be too quick to dismiss performance concerns. Even if not a current bottleneck, this could become an issue as the system scales.
While the concern is valid, we should follow the principle of not optimizing prematurely. The comment is speculative without evidence of actual performance impact.
Delete the comment since it's speculative about performance impact without clear evidence that this is causing actual problems.
3. agents-api/agents_api/common/protocol/tasks.py:232
- Draft comment:
The usage of 'await get_workflow_name(...)' suggests an asynchronous function, but its current implementation has no async operations. Consider making it synchronous if no async behavior is needed. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/common/utils/workflows.py:4
- Draft comment:
The 'get_workflow_name' function is defined as async but doesn't perform any asynchronous operation. Consider refactoring it to a synchronous function unless future async operations are planned. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Afdhmr76c27Y5Q4a
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.
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. Incremental review on 852a475 in 59 seconds
More details
- Looked at
53
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/tests/test_prepare_for_step.py:8
- Draft comment:
Importing Transition here is correct since it's being used in the new test. Ensure that the correct Transition type is used consistently across tests. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. agents-api/tests/test_prepare_for_step.py:92
- Draft comment:
The 'get_workflow_name' test covers various formatting cases including backtick-enclosed workflow names. Consider adding negative tests or edge cases for malformed workflow strings. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. agents-api/tests/test_prepare_for_step.py:8
- Draft comment:
Added 'Transition' import is appropriate; ensure it remains consistent with other type imports. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. agents-api/tests/test_prepare_for_step.py:17
- Draft comment:
Importing 'get_workflow_name' from the utilities is clear and needed for the new tests. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. agents-api/tests/test_prepare_for_step.py:109
- Draft comment:
Good test coverage for 'get_workflow_name' with formatted workflow strings. For better isolation and robustness, consider splitting scenarios into separate tests and adding edge cases (e.g. empty or malformed workflow names). - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_Rr4pgmZkR7mlPjvW
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.
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! Incremental review on 1aad183 in 54 seconds
More details
- Looked at
53
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/agents_api/common/protocol/tasks.py:233
- Draft comment:
Removed await from get_workflow_name calls since it's now synchronous. Ensure that all contexts updated accordingly. - Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/common/utils/workflows.py:4
- Draft comment:
Changed get_workflow_name from async to sync. Confirm that no asynchronous operations are needed here. - 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 function is clearly doing only synchronous string operations. There are no I/O operations, no database calls, no network requests, and no other potentially async operations. The function is simple enough that we can see its entire implementation and verify this. The comment is asking for unnecessary verification of something that is obvious from the code.
Could there be async operations happening inside the Transition class methods that we can't see? Could transition.current.workflow be an async property?
Even if transition.current.workflow were an async property, Python would require using await to access it, which isn't present here. The code as written can only work with synchronous properties.
Delete this comment. It's asking for verification of something that is clearly true from the code, and it violates the rule about not asking authors to verify or confirm things.
3. agents-api/tests/test_prepare_for_step.py:106
- Draft comment:
Tests now use synchronous get_workflow_name calls. Confirm that the extraction logic (e.g. backticks, PAR:`) works correctly. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/common/protocol/tasks.py:233
- Draft comment:
Good change—refactored get_workflow_name calls to be synchronous. Ensure that filtering logic using the workflow remains valid for all cases. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. agents-api/agents_api/common/utils/workflows.py:4
- Draft comment:
Changed get_workflow_name to a synchronous function since it only performs string parsing. This simplifies its use elsewhere. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_dnabfSxSIEkqiNqO
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. Incremental review on 42f0a87 in 53 seconds
More details
- Looked at
71
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. agents-api/agents_api/common/utils/workflows.py:12
- Draft comment:
Avoid using assert for input validation. Instead, raise a proper exception like ValueError to ensure robustness in production. - Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/common/utils/workflows.py:8
- Draft comment:
Using 'assert' for input validation may be disabled in production with optimizations. Consider raising a specific exception (e.g. ValueError) instead of assert to ensure robust error handling. - Reason this comment was not posted:
Marked as duplicate.
3. agents-api/tests/test_prepare_for_step.py:131
- Draft comment:
New tests for 'get_workflow_name' cover both valid and error cases thoroughly. Great work! - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_0ds58ygsAUG0IZIR
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.
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. Incremental review on 9c34ed3 in 1 minute and 15 seconds
More details
- Looked at
102
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. agents-api/agents_api/common/utils/workflows.py:9
- Draft comment:
Consider updating the comment to reflect the use of PAR_PREFIX and SEPARATOR constants rather than hard-coded strings. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. agents-api/agents_api/workflows/task_execution/helpers.py:104
- Draft comment:
Typo: 'seprated_workflow_name' should be 'separated_workflow_name'. - Reason this comment was not posted:
Marked as duplicate.
3. agents-api/agents_api/workflows/task_execution/helpers.py:145
- Draft comment:
Typo: 'seprated_workflow_name' should be 'separated_workflow_name'. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/workflows/task_execution/helpers.py:182
- Draft comment:
Typo: 'seprated_workflow_name' should be 'separated_workflow_name'. - Reason this comment was not posted:
Marked as duplicate.
5. agents-api/agents_api/workflows/task_execution/helpers.py:223
- Draft comment:
Typo: 'seprated_workflow_name' should be 'separated_workflow_name'. - Reason this comment was not posted:
Marked as duplicate.
6. agents-api/agents_api/workflows/task_execution/helpers.py:293
- Draft comment:
Typo: 'seprated_workflow_name' should be 'separated_workflow_name'. - Reason this comment was not posted:
Marked as duplicate.
7. agents-api/agents_api/common/utils/workflows.py:9
- Draft comment:
Consider updating the inline comment to reference the SEPARATOR constant instead of a hardcoded backtick (```). This will keep the comment consistent if the separator ever changes. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_sUZsfCWPzqx8BYpU
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.
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! Incremental review on f38e75b in 29 seconds
More details
- Looked at
52
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. agents-api/agents_api/workflows/task_execution/helpers.py:104
- Draft comment:
Fixed typo: variable name 'seprated_workflow_name' updated to 'separated_workflow_name'. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. agents-api/agents_api/workflows/task_execution/helpers.py:182
- Draft comment:
Fixed typo: variable name 'seprated_workflow_name' updated to 'separated_workflow_name' in foreach step. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. agents-api/agents_api/workflows/task_execution/helpers.py:223
- Draft comment:
Fixed typo: variable name 'seprated_workflow_name' updated to 'separated_workflow_name' in map_reduce step. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. agents-api/agents_api/workflows/task_execution/helpers.py:293
- Draft comment:
Fixed typo: variable name 'seprated_workflow_name' updated to 'separated_workflow_name' in parallel map_reduce step. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. agents-api/agents_api/workflows/task_execution/helpers.py:141
- Draft comment:
Nice fix renaming the typo from 'seprated_workflow_name' to 'separated_workflow_name'. Since this pattern is repeated in multiple functions, consider extracting it into a helper to reduce duplication. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_8M57tzohQjwdVZxW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
PR Type
Bug fix
Description
Fixed handling of
ActivityError
by unwrapping its cause.Filtered transitions to match workflow in
get_inputs
.Enhanced error handling in
transition
function.Changes walkthrough 📝
interceptors.py
Handle `ActivityError` by unwrapping its cause
agents-api/agents_api/common/interceptors.py
ActivityError
to its cause.tasks.py
Filter transitions to match workflow in `get_inputs`
agents-api/agents_api/common/protocol/tasks.py
transition.py
Enhance error handling in `transition` function
agents-api/agents_api/workflows/task_execution/transition.py
ActivityError
to its cause.transition
.Important
Fixes subworkflow step access by unwrapping
ActivityError
, filtering transitions, and addingget_workflow_name
utility.ActivityError
to its cause ininterceptors.py
andtransition.py
.interceptors.py
.get_inputs
intasks.py
.tasks.py
.get_workflow_name
inworkflows.py
to extract workflow names from transitions.get_workflow_name
intest_prepare_for_step.py
.This description was created by for f38e75b. It will automatically update as commits are pushed.