-
Notifications
You must be signed in to change notification settings - Fork 777
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 missing video recording - recording is stored in the first step #1350
Conversation
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 457cf51 in 36 seconds
More details
- Looked at
69
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/agent.py:1640
- Draft comment:
Thestep_id
used for fetching the recording artifact should befirst_step.step_id
instead oflast_step.step_id
to align with the intent of using the first step for recording. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_HbTwbqFQYoEI3YDD
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 b041410 in 13 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/agent.py:1639
- Draft comment:
The change fromlast_step.step_id
tofirst_step.step_id
is correct for retrieving the recording artifact from the first step, as per the PR description. - Reason this comment was not posted:
Confidence changes required:0%
The change in the PR is correct as it updates the step_id from last_step to first_step for retrieving the recording artifact. This aligns with the intent to fix video recording storage by using the first step for artifact retrieval.
Workflow ID: wflow_2YfevTKFGe9g15rf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
b041410
to
c263b98
Compare
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 c263b98 in 18 seconds
More details
- Looked at
27
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:340
- Draft comment:
The log message should indicate that the first step was not found, not the latest step. - Reason this comment was not posted:
Confidence changes required:50%
The log message in theget_first_step
method is misleading. It should indicate that the first step was not found, not the latest step.
Workflow ID: wflow_zZXTCgjzOyho2Zes
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Fixes video recording storage by retrieving the recording artifact from the first step in
build_task_response()
and addsget_first_step()
toclient.py
.build_task_response()
inagent.py
.get_first_step()
toclient.py
to fetch the first step of a task for recording retrieval.client.py
for error handling.This description was created by for c263b98. It will automatically update as commits are pushed.