improve need_verification_code prompt #788
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to a26da3d in 13 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/prompts/skyvern/extract-action.j2:38
- Draft comment:
JSON does not support comments. Remove the comment from the JSON template to ensure valid JSON output. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is technically correct that JSON does not support comments. However, this is a Jinja2 template, and the comment might not be present in the final JSON output. The comment is about a line that was changed, so it is relevant to the diff.
I might be overthinking the Jinja2 context. The comment could be valid if the template is rendered with the comment intact, leading to invalid JSON.
The comment is relevant if the template rendering process does not remove the comment, leading to invalid JSON. It's important to ensure the final output is valid JSON.
Keep the comment as it addresses a potential issue with JSON validity in the context of the template rendering.
Workflow ID: wflow_KAuKBBllvXCvIDWy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
wintonzheng
force-pushed
the
shu/increase_totp_code_lifespan
branch
from
a26da3d to
02598fa
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 02598fa in 17 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_hzwAbIOYreNIIftS
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.
| @@ -35,7 +35,7 @@ Reply in JSON format with the following keys: | |||
| }] | |||
| {% endif %} | |||
| }],{% if verification_code_check %} | |||
| "need_verification_code": bool, // Whether a verification code is needed to proceed.{% endif %} | |||
| "need_verification_code": bool, // Whether a verification code is needed to proceed. True only after the code is sent to the email or phone or 2FA/MFA device. If the code is not sent, return false {% endif %} | |||
There was a problem hiding this comment.
JSON does not support comments. Remove the comment from the JSON template to avoid parsing errors.
wintonzheng
force-pushed
the
shu/increase_totp_code_lifespan
branch
from
02598fa to
b9a11ce
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b9a11ce in 17 seconds
More details
- Looked at
27lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_0LZr19SxzhZPhAyD
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.
| @@ -35,7 +35,8 @@ Reply in JSON format with the following keys: | |||
| }] | |||
| {% endif %} | |||
| }],{% if verification_code_check %} | |||
| "need_verification_code": bool, // Whether a verification code is needed to proceed.{% endif %} | |||
| "need_verification_code_reasoning": str, // What makes you think if the verification is needed or not needed? Any where in the website stating you need the veficiation? Is the verification code sent already? | |||
There was a problem hiding this comment.
Remove comments from JSON as they are not valid JSON syntax. This applies to other comments in the JSON template as well.
|
🙅 do not merge: failed a couple of test cases. might need to adjust the prompt. |
|
This pull request is stale because it has been open for 14 days with no activity. |
wintonzheng
force-pushed
the
shu/increase_totp_code_lifespan
branch
from
b9a11ce to
cae6527
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on cae6527 in 24 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/prompts/skyvern/extract-action.j2:40
- Draft comment:
Remove the comment syntax (//) to ensure valid JSON output. This applies to lines 40, 41, and 42. - Reason this comment was not posted:
Based on historical feedback, this comment is too similar to comments previously marked by users as bad.
Workflow ID: wflow_nOn9Q8YlZ2Pn1Apb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
wintonzheng
force-pushed
the
shu/increase_totp_code_lifespan
branch
from
cae6527 to
c100ec5
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c100ec5 in 22 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/prompts/skyvern/extract-action.j2:40
- Draft comment:
Remove the comment syntax (//) to ensure valid JSON output. Comments are not allowed in JSON. This applies to other similar instances in the file as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The file is a Jinja2 template, which means the comments are not part of the final JSON output. The comment syntax is used for clarity within the template and does not affect the JSON output. Therefore, the comment about removing comment syntax is not applicable in this context.
I might be missing the possibility that the template is directly outputting JSON with comments, but typically, Jinja2 templates are processed to remove such comments.
Jinja2 templates are designed to be processed, and comments are not included in the final output. The comment syntax is for template clarity and does not affect the JSON output.
The comment about removing comment syntax is not applicable because the file is a Jinja2 template, and the comments do not affect the final JSON output.
Workflow ID: wflow_EQg4mda9dLNLoyLb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 69631b1 in 25 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_Y58YhUlsK23n9OGD
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.
| @@ -37,7 +37,8 @@ Reply in JSON format with the following keys: | |||
| }] | |||
| {% endif %} | |||
| }],{% if verification_code_check %} | |||
| "need_verification_code": bool, // Whether a verification code is needed to proceed.{% endif %} | |||
| "verification_code_reasoning": str, // Let's think step by step. Describe what you see and think if a verification code is needed for login or any verification step. Explain why you believe a verification code is needed or not. Has the code been sent and is code available somewhere (email, phone or 2FA device)? | |||
There was a problem hiding this comment.
Remove the comment syntax (//) to ensure valid JSON output. Comments are not allowed in JSON. This applies to all similar instances in the file.
| "verification_code_reasoning": str, // Let's think step by step. Describe what you see and think if a verification code is needed for login or any verification step. Explain why you believe a verification code is needed or not. Has the code been sent and is code available somewhere (email, phone or 2FA device)? | |
| "verification_code_reasoning": str |
Important
Clarifies
need_verification_codeprompt and adds timeout warning log inpoll_verification_code.need_verification_codeprompt conditions inextract-action.j2.need_verification_code_reasoningandverification_code_reasoningfor explanation.poll_verification_codeinhandler.py.poll_verification_codefunction inhandler.py.This description was created by
for 69631b1. It will automatically update as commits are pushed.