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

use common logic when building task response #1311

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Dec 3, 2024

Important

Refactor task response construction by centralizing logic in build_task_response in agent.py, ensuring consistent handling of browser logs and failure reasons.

  • Refactoring:
    • Introduced build_task_response in agent.py to centralize task response construction.
    • Replaced direct task response construction with build_task_response in execute_task_webhook, get_task, and retry_webhook.
  • Behavior:
    • Task responses now consistently include browser logs and failure reasons when applicable.
    • Improved error logging for missing action screenshots in agent.py.
  • Imports:
    • Added TaskResponse import in agent.py.
    • Removed unused ArtifactType import in agent_protocol.py.

This description was created by Ellipsis for 7f1155b. 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! Reviewed everything up to 7f1155b in 1 minute and 25 seconds

More details
  • Looked at 289 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/agent.py:1572
  • Draft comment:
    Consider instantiating httpx.AsyncClient() outside the try block and using it within a context manager to ensure proper resource management.
async with httpx.AsyncClient() as client:
    resp = await client.post(
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new method build_task_response to centralize the logic for building task responses. This method is now used in multiple places, replacing the previous inline logic. However, there is a potential issue with the httpx.AsyncClient() usage. The client should be instantiated outside the try block to ensure proper resource management and should be closed after use.
2. skyvern/forge/agent.py:1591
  • Draft comment:
    Consider closing the httpx.AsyncClient() after use to ensure proper resource management. This can be done by using a context manager as suggested in the previous comment.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new method build_task_response to centralize the logic for building task responses. This method is now used in multiple places, replacing the previous inline logic. However, there is a potential issue with the httpx.AsyncClient() usage. The client should be instantiated outside the try block to ensure proper resource management and should be closed after use.

Workflow ID: wflow_Yuoy04Cm1Rgw4a0Y


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

@LawyZheng LawyZheng merged commit a206f51 into main Dec 3, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/use-common-logic-when-build-task-response branch December 3, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant