Skip to content

Conversation

@wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Aug 15, 2025


📝 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

  • API Documentation: Updated the summary field in the OpenAPI specification from "Retry run webhook" to "Retry webhook"
  • Endpoint Affected: The /runs/{run_id}/retry_webhook/ POST endpoint in the agent protocol routes
  • Scope: Pure documentation change with no functional code modifications

Technical 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]
Loading

Impact

  • Documentation Clarity: Removes redundant "run" terminology since the endpoint context already implies it's for runs
  • API Consistency: Improves the consistency of endpoint naming conventions in the API documentation
  • Zero Functional Impact: No changes to actual functionality, behavior, or performance - purely cosmetic documentation improvement

Created with Palmier


Important

Adds API endpoint to generate webhook payloads without sending and updates frontend to display them, along with API documentation improvements.

  • API Endpoints:
    • Adds get_webhook_payload endpoint in agent_protocol.py to generate webhook payloads without sending.
    • Updates retry_run_webhook endpoint summary in agent_protocol.py from "Retry run webhook" to "Retry webhook".
  • Frontend:
    • Adds useWebhookPayloadQuery in useWebhookPayloadQuery.ts to fetch webhook payloads.
    • Displays webhook payload in DebuggerRunOutput.tsx and WorkflowRunOutput.tsx.
  • Backend Services:
    • Implements build_run_webhook_payload in run_service.py to construct webhook payloads for different run types.

This description was created by Ellipsis for 623fa87. You can customize this summary. 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.

Important

Looks good to me! 👍

Reviewed everything up to aeb79f0 in 33 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@wintonzheng wintonzheng changed the title documentation update in the retry webhook endpoint: retry run webhook -> retry webhook API to get webhook response endpoint Aug 15, 2025
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.

Important

Looks good to me! 👍

Reviewed e40743a in 1 minute and 4 seconds. Click for details.
  • Reviewed 170 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% None

Workflow ID: wflow_jLH8mbzWpgm1z3nW

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@wintonzheng wintonzheng force-pushed the shu/retry_webhook_renaming branch from e40743a to b03e8d6 Compare August 15, 2025 22:43
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.

Caution

Changes requested ❌

Reviewed b03e8d6 in 1 minute and 56 seconds. Click for details.
  • Reviewed 170 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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% <= threshold 50% 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 Ellipsis 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"})
Copy link
Contributor

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.

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.

Important

Looks good to me! 👍

Reviewed 623fa87 in 39 seconds. Click for details.
  • Reviewed 134 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft 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% <= threshold 50% 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% <= threshold 50% 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% <= threshold 50% None

Workflow ID: wflow_bV6bPe0Avu9QBBBP

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

2 participants