Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

update task_v2 block workflow run id missing log to warning #1807

Merged
merged 1 commit into from
Feb 23, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 21, 2025

Important

Change log level from error to warning for missing block_workflow_run_id in task_v2 blocks in agent_protocol.py.

  • Logging:
    • Change log level from error to warning for missing block_workflow_run_id in task_v2 blocks in _flatten_workflow_run_timeline() in agent_protocol.py.

This description was created by Ellipsis for 0d6a46b. 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 0d6a46b in 1 minute and 5 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:1396
  • Draft comment:
    Changing the log level from error to warning is acceptable if a missing block_workflow_run_id is not critical. Ensure that this behavior aligns with the overall error handling strategy.
  • 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 ensure that the behavior aligns with the overall error handling strategy, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written.
2. skyvern/forge/sdk/routes/agent_protocol.py:1395
  • Draft comment:
    Downgrading the log level from error to warning here seems appropriate if a missing task_v2 block workflow run id is expected. Consider adding a brief comment explaining this choice for future maintainers.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While documenting log level changes can be helpful, in this case:
  1. The comment is asking for explanation of something that should be self-evident from the code
  2. The log level change from error to warning is a minor change that doesn't warrant special documentation
  3. The comment is not pointing out a clear issue that needs fixing
  4. The comment is asking for documentation without strong evidence that it's actually needed
    I could be wrong about whether this log level change is significant enough to warrant documentation. Perhaps there's important context about why this state occurs that would be valuable to document.
    While documentation can be valuable, in this case the log level change appears to be a straightforward adjustment to match the severity level with the actual impact. The code and message are clear enough without additional comments.
    This comment should be deleted as it requests documentation for a minor log level change without clear evidence that such documentation would be valuable.

Workflow ID: wflow_jjZRhhkuokYmOI3X


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

@wintonzheng wintonzheng merged commit b0fbb5e into main Feb 23, 2025
7 checks passed
@wintonzheng wintonzheng deleted the shu/update_task_v2_block_warning_log branch February 23, 2025 09:08
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant