-
Notifications
You must be signed in to change notification settings - Fork 1.7k
API to get webhook response endpoint #3206
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
base: main
Are you sure you want to change the base?
Conversation
… -> retry webhook
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.
Important
Looks good to me! 👍
Reviewed everything up to aeb79f0 in 33 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/forge/sdk/routes/agent_protocol.py:834
- Draft comment:
The summary text has been updated from 'Retry run webhook' to 'Retry webhook'. This improves clarity. Ensure consistency with related endpoints if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative, noting a change in text for clarity. It also suggests ensuring consistency, which is a vague request for confirmation. This violates the rule against asking the PR author to ensure behavior is intended or to double-check things.
Workflow ID: wflow_78jGrnjQRtGTaGyj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed e40743a in 1 minute and 4 seconds. Click for details.
- Reviewed
170lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/services/run_service.py:175
- Draft comment:
Consider logging a warning when no latest_step is found before executing the task webhook in retry_run_webhook; ensure that silent omission is intended. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. skyvern/services/run_service.py:99
- Draft comment:
In cancel_task_v1, latest_step is fetched and passed to execute_task_webhook without checking if it exists. Consider handling missing latest_step consistently with retry_run_webhook. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. skyvern/services/run_service.py:192
- Draft comment:
The error message in retry_run_webhook now uses 'retry webhook' which aligns with the updated documentation—this looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. skyvern/services/run_service.py:220
- Draft comment:
Consider refactoring the merging logic in build_run_webhook_payload for improved readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_jLH8mbzWpgm1z3nW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
e40743a to
b03e8d6
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.
Caution
Changes requested ❌
Reviewed b03e8d6 in 1 minute and 56 seconds. Click for details.
- Reviewed
170lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/forge/sdk/routes/agent_protocol.py:848
- Draft comment:
Consider adding an explicit response model to the 'get_webhook_payload' endpoint to enhance API documentation and type safety. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While having explicit response models is generally good practice for API documentation and type safety, we don't have enough context to know what the payload structure is. The payload comes from a service method we can't see. Without knowing the payload structure, suggesting a response model is speculative. The comment is asking the author to investigate and define something without clear evidence it's needed. The suggestion could improve API documentation and type safety. Not having a response model means clients don't know what to expect from the endpoint. However, we don't have enough context to know if this is actually an issue. We can't see the payload structure and don't know if it's consistent enough to model. Making speculative suggestions without full context violates our review rules. Delete this comment. While explicit response models are good practice, we don't have enough context about the payload structure to make this suggestion confidently. The comment is speculative and could send the author on an unnecessary investigation.
2. skyvern/services/run_service.py:232
- Draft comment:
In the Task V2 branch, the code uses 'run.run_id' to fetch the task_v2. Verify that this is consistent with the input 'run_id' used in other branches. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify consistency between branches, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
3. skyvern/services/run_service.py:285
- Draft comment:
When merging payload dictionaries in the workflow run branch, consider handling potential key conflicts between the legacy status response and the new workflow response. - 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 raises a valid point about dictionary merging behavior. However, this appears to be intentional - the code is deliberately overwriting the legacy response fields with the new response fields. The .update() behavior is the desired behavior here. The comment seems to be raising a hypothetical issue rather than pointing out an actual problem. The code could potentially lose important data if the legacy response contains fields that should be preserved but are accidentally overwritten. Maybe there's a reason both responses are needed. The code structure and variable names strongly suggest this is an intentional migration pattern where we want the new response format to take precedence over legacy fields. If this was unintended, it would likely be structured differently. This comment should be deleted as it raises a speculative concern about an intentional code pattern. The dictionary merging behavior appears to be working as designed.
Workflow ID: wflow_V326jLrQtPlIxUqJ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| task_response = await app.agent.build_task_response(task=task, last_step=latest_step) | ||
|
|
||
| # Build the new RunResponse for backward compatibility and merge | ||
| payload_json = task_response.model_dump_json(exclude={"request"}) |
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.
To avoid unnecessary serialization overhead, consider using model_dump() to obtain dicts directly instead of dump->json->load.
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.
Important
Looks good to me! 👍
Reviewed 623fa87 in 39 seconds. Click for details.
- Reviewed
134lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern-frontend/src/hooks/useWebhookPayloadQuery.ts:14
- Draft comment:
The hook is well implemented using react‐query. Consider wrapping the getClient call in try/catch for more explicit error logging in case of failures. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. skyvern-frontend/src/routes/workflows/debugger/DebuggerRunOutput.tsx:178
- Draft comment:
Webhook payload is integrated cleanly. Note the header text size ('text-sm') here differs from WorkflowRunOutput ('text-lg'); consider unifying styles or clarifying if intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. skyvern-frontend/src/routes/workflows/workflowRun/WorkflowRunOutput.tsx:178
- Draft comment:
Webhook payload section duplicates similar UI from DebuggerRunOutput. Consider refactoring the common logic into a shared component. Also, verify if the larger header ('text-lg') is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_bV6bPe0Avu9QBBBP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
📝 This PR updates the API documentation for the retry webhook endpoint by changing the summary text from "Retry run webhook" to "Retry webhook" for better clarity and consistency.
🔍 Detailed Analysis
Key Changes
summaryfield in the OpenAPI specification from "Retry run webhook" to "Retry webhook"/runs/{run_id}/retry_webhook/POST endpoint in the agent protocol routesTechnical Implementation
flowchart TD A[API Endpoint Documentation] --> B[OpenAPI Summary Field] B --> C["Old: 'Retry run webhook'"] B --> D["New: 'Retry webhook'"] C --> E[Less Clear/Redundant] D --> F[More Concise/Clear]Impact
Created with Palmier
Important
Adds API endpoint to generate webhook payloads without sending and updates frontend to display them, along with API documentation improvements.
get_webhook_payloadendpoint inagent_protocol.pyto generate webhook payloads without sending.retry_run_webhookendpoint summary inagent_protocol.pyfrom "Retry run webhook" to "Retry webhook".useWebhookPayloadQueryinuseWebhookPayloadQuery.tsto fetch webhook payloads.DebuggerRunOutput.tsxandWorkflowRunOutput.tsx.build_run_webhook_payloadinrun_service.pyto construct webhook payloads for different run types.This description was created by
for 623fa87. You can customize this summary. It will automatically update as commits are pushed.