Skip to content
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 action history #1076

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Fix action history #1076

merged 1 commit into from
Oct 28, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Oct 28, 2024

Important

Modify _get_action_results in agent.py to include specific fields in action and result model dumps.

  • Behavior:
    • Modify _get_action_results in agent.py to include only action_type, element_id, status, reasoning, and option in action model dumps.
    • Include only success, exception_type, and exception_message in result model dumps.
  • Misc:
    • No other files or functions are affected by this change.

This description was created by Ellipsis for 7e2874f. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 486f8ef in 30 seconds

More details
  • Looked at 124 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/agent.py:1219
  • Draft comment:
    The logic to exclude successful actions from history is missing. Ensure that only unsuccessful actions are included in the action history as per the PR description.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_u1s1UFHdTveUHUzH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 486f8ef in 39 seconds

More details
  • Looked at 124 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/agent.py:1222
  • Draft comment:
    The use of include here does not align with the PR description, which specifies excluding certain fields. Consider using exclude to remove workflow_run_id, intention, and response from the action model dumps.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests a change that is not directly related to the diff, as the diff shows a switch to 'include' rather than 'exclude'. The fields mentioned in the comment ('workflow_run_id', 'intention', 'response') are not part of the 'include' list, so they are already excluded. The comment seems to be based on the PR description, which is not available, making it speculative.
    I might be missing the context of the PR description, which could provide more insight into the intention behind the changes. However, without this context, the comment appears speculative.
    Without the PR description, it's difficult to verify the comment's claim. The diff itself does not show any issue with the current 'include' usage.
    Delete the comment as it seems speculative and not directly related to the changes in the diff.

Workflow ID: wflow_re3Nq2aktHOMZo6d


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Modify `_get_action_results` in `agent.py` to exclude certain fields from action model dumps.
>
>   - **Behavior**:
>     - Modify `_get_action_results` in `agent.py` to exclude `workflow_run_id`, `intention`, and `response` from action model dumps.
>   - **Misc**:
>     - No other files or functions are affected by this change.
>
> <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 1211ca9149e97a8c690b20a47832142cb45ed613. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
@wintonzheng wintonzheng force-pushed the suchintan.fix-action-history branch from 486f8ef to 7e2874f Compare October 28, 2024 19:30
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 7e2874f in 22 seconds

More details
  • Looked at 38 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/agent.py:1219
  • Draft comment:
    The logic to exclude successful actions from history is not implemented. Consider adding a filter to exclude actions where all results are successful.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_NCGxyD9znmekopBZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wintonzheng wintonzheng merged commit 2b176df into main Oct 28, 2024
2 checks passed
@wintonzheng wintonzheng deleted the suchintan.fix-action-history branch October 28, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant