-
Notifications
You must be signed in to change notification settings - Fork 902
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 workflow not found bug #1792
Conversation
<!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Fixes workflow retrieval bug by using `get_workflow_by_permanent_id` in `restart_workflow.py` and adjusts `organization_id` handling for templates in `agent_protocol.py`. > > - **Behavior**: > - Fixes workflow retrieval bug by using `get_workflow_by_permanent_id` instead of `get_workflow` in `rerun_workflow()` in `restart_workflow.py`. > - Adjusts `execute_workflow()` in `agent_protocol.py` to set `organization_id` to `None` for template workflows. > - **Misc**: > - No other changes or refactors were made. > > <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=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for f60d8f17ca941fd389be9acc581acf42665c0ee5. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
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 dffa61b in 1 minute and 11 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:704
- Draft comment:
Ensure get_workflow_by_permanent_id correctly handles organization_id=None for template workflows. Consider adding a comment or documentation on why organization_id is omitted when template is True. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This comment violates several rules:
- It starts with "Ensure that..." which is asking the author to verify something
- It's asking for documentation without strong evidence that it's needed
- It's speculative - it assumes there might be an issue with handling None
- There's no clear evidence of a problem that needs fixing
Could there be a real issue with None handling that needs to be documented? Maybe the function has complex None handling logic that should be explained?
Without seeing the implementation of get_workflow_by_permanent_id, we can't know if there's any complex None handling. The code looks straightforward - it just passes None when it's a template. We should assume the function works correctly unless we see evidence otherwise.
Delete this comment. It's asking for verification and documentation without clear evidence that either is needed.
2. skyvern/forge/sdk/routes/agent_protocol.py:704
- Draft comment:
Good change: now passing organization_id=None for template workflows. Ensure that get_workflow_by_permanent_id handles a None value correctly and add corresponding tests/documentation. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_AVDK66Ov7YvPTqKa
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! Reviewed everything up to dffa61b in 1 minute and 32 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:704
- Draft comment:
Ensure get_workflow_by_permanent_id accepts organization_id=None for template workflows. Consider adding a comment explaining the rationale. - Reason this comment was not posted:
Marked as duplicate.
2. skyvern/forge/sdk/routes/agent_protocol.py:704
- Draft comment:
Good fix. Passing organization_id as None when 'template' is True ensures global workflows are queried. Verify that get_workflow_by_permanent_id correctly handles None and that this change doesn't affect non-template lookups. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment appears to be explaining what the code does and then asking for verification. It's not identifying any specific issue or suggesting any changes. The comment starts with an explanation of what the code does, but then asks to "Verify that..." which is a red flag based on the rules. The rules specifically say to NOT ask the PR author to verify, confirm, or double-check things.
The comment does accurately explain what the code is doing. Maybe there is a real issue with how None is handled that should be flagged?
While the comment's explanation is accurate, it doesn't identify any actual problems. If there were real concerns about None handling, the comment should state them directly rather than asking for verification. The rules are clear that we should not ask authors to verify things.
This comment should be deleted because it merely asks for verification without identifying any specific issues. It violates the rule against asking authors to verify or confirm things.
Workflow ID: wflow_KzneTJQQchmc1O8R
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Fixes bug in
execute_workflow
to correctly handle template workflows by settingorganization_id
toNone
.execute_workflow
inagent_protocol.py
whereorganization_id
is set toNone
iftemplate
is true, ensuring correct retrieval of template workflows.This description was created by
for dffa61b. It will automatically update as commits are pushed.