fix(slack): preserve progress edits on network failures - #64267
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
This PR preserves progress edits on network failures in Slack.
Looks Good
- Network resilience improvement.
- Well-scoped: 4 files.
No Issues Found
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the transient Slack failure to the edit-result contract. The normal progress consumer already preserves an editable message when result.retryable is true (gateway/run.py:18191-18207), and the proposed Slack classification fits that contract.
Problems
- The overflow sibling path still unconditionally disables editing after any failed edit:
gateway/run.py:18095-18099. Once_split_progress_groups()produces multiple groups, a retryable Slackchat.updatefailure will still setcan_edit = False, recreating the replacement-bubble behavior this PR addresses.
Suggested changes
- Teach
_roll_progress_overflow_if_needed()to retaincan_editforresult.retryable, and add an overflow regression covering that branch. - Salvage the adapter hunk against current
main:SlackAdapter.edit_messagenow routes via metadata-aware_get_clientandstop_typingcalls (plugins/platforms/slack/adapter.py:1545-1553).
Automated hermes-sweeper review.
| return SendResult( | ||
| success=False, | ||
| error=str(e), | ||
| retryable=True, |
There was a problem hiding this comment.
This retryable result is honored by the normal progress loop, but not by the overflow branch: current gateway/run.py:18095-18099 sets can_edit = False for every failed _edit_progress_message. Please extend that branch to preserve the message ID when result.retryable is true and add an overflow regression.
913015d to
36ab5ac
Compare
- rt.cms012@gmail.com -> trac3r00 (#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (#64267) - hello@jeromeiveson.com -> Trantor-develops (#57196) - boumagent@gmail.com -> patp (#18859) - dorukardahan@hotmail.com -> dorukardahan (#17184) was already mapped.
- rt.cms012@gmail.com -> trac3r00 (#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (#64267) - hello@jeromeiveson.com -> Trantor-develops (#57196) - boumagent@gmail.com -> patp (#18859) - dorukardahan@hotmail.com -> dorukardahan (#17184) was already mapped.
… tests CI shards run without the slack extras; the two #64267 tests import the real SDK (SlackApiError, the lazy-rebind path) and errored with ModuleNotFoundError. Skip on bare environments — classification coverage for stdlib exception types (OSError/TimeoutError/cert errors) still runs everywhere.
- rt.cms012@gmail.com -> trac3r00 (#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (#64267) - hello@jeromeiveson.com -> Trantor-develops (#57196) - boumagent@gmail.com -> patp (#18859) - dorukardahan@hotmail.com -> dorukardahan (#17184) was already mapped.
… tests CI shards run without the slack extras; the two #64267 tests import the real SDK (SlackApiError, the lazy-rebind path) and errored with ModuleNotFoundError. Skip on bare environments — classification coverage for stdlib exception types (OSError/TimeoutError/cert errors) still runs everywhere.
|
Merged via #70189 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your transient-transport classifier was cherry-picked (we added importorskip for CI environments without the slack extras). Thanks for the contribution! |
- rt.cms012@gmail.com -> trac3r00 (NousResearch#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (NousResearch#64267) - hello@jeromeiveson.com -> Trantor-develops (NousResearch#57196) - boumagent@gmail.com -> patp (NousResearch#18859) - dorukardahan@hotmail.com -> dorukardahan (NousResearch#17184) was already mapped.
… tests CI shards run without the slack extras; the two NousResearch#64267 tests import the real SDK (SlackApiError, the lazy-rebind path) and errored with ModuleNotFoundError. Skip on bare environments — classification coverage for stdlib exception types (OSError/TimeoutError/cert errors) still runs everywhere.
- rt.cms012@gmail.com -> trac3r00 (NousResearch#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (NousResearch#64267) - hello@jeromeiveson.com -> Trantor-develops (NousResearch#57196) - boumagent@gmail.com -> patp (NousResearch#18859) - dorukardahan@hotmail.com -> dorukardahan (NousResearch#17184) was already mapped.
… tests CI shards run without the slack extras; the two NousResearch#64267 tests import the real SDK (SlackApiError, the lazy-rebind path) and errored with ModuleNotFoundError. Skip on bare environments — classification coverage for stdlib exception types (OSError/TimeoutError/cert errors) still runs everywhere.
- rt.cms012@gmail.com -> trac3r00 (NousResearch#68378; commit authored as 'Minseo-Choi' — trac3r00's display name, same account) - 15167896+2001Y@users.noreply.github.com -> 2001Y (NousResearch#64267) - hello@jeromeiveson.com -> Trantor-develops (NousResearch#57196) - boumagent@gmail.com -> patp (NousResearch#18859) - dorukardahan@hotmail.com -> dorukardahan (NousResearch#17184) was already mapped.
… tests CI shards run without the slack extras; the two NousResearch#64267 tests import the real SDK (SlackApiError, the lazy-rebind path) and errored with ModuleNotFoundError. Skip on bare environments — classification coverage for stdlib exception types (OSError/TimeoutError/cert errors) still runs everywhere.
What does this PR do?
Slack tool-progress updates use
chat.updateto keep one editable progress message. A transientaiohttpDNS/connection failure was returned as a permanent edit failure, so the gateway disabled editing for the rest of the turn and posted later tool updates as new messages.This change classifies
TimeoutErrorandaiohttp.ClientConnectionErroras retryable for the idempotentchat.updatepath. It deliberately keeps Slack API errors and TLS/certificate/fingerprint validation failures non-retryable. The retryable result lets the existing gateway consumer retain the same progress message ID and catch up on the next edit.The
aiohttpexception lookup uses the module rebound by Slack's lazy dependency loader, so both initial-import and post-install states behave the same way.Related Issue
N/A — reproduced from a live Slack gateway
ClientConnectorDNSErrorwhile updating a tool-progress message.Type of Change
Changes Made
plugins/platforms/slack/adapter.pyaiohttp.ClientConnectionErrorfailures fromchat.updateas retryable transient failures;aiohttpexception classes after lazy dependency rebinding.tests/gateway/test_slack_block_kit_adapter.pyTimeoutError, the observedClientConnectorDNSError, TLS/certificate/fingerprint errors, plainOSError, Slack API errors, and lazy dependency rebinding.tests/gateway/test_run_progress_topics.pytests/gateway/test_slack.pyHow to Test
scripts/run_tests.sh tests/gateway/test_slack_block_kit_adapter.py tests/gateway/test_run_progress_topics.py tests/gateway/test_slack.py -- -q -o addopts=.scripts/run_tests.sh tests/gateway/test_slack*.py tests/gateway/test_run_progress_topics.py -- -q -o addopts=.scripts/run_tests.sh.python scripts/check-windows-footguns.py plugins/platforms/slack/adapter.py tests/gateway/test_slack.py tests/gateway/test_slack_block_kit_adapter.py tests/gateway/test_run_progress_topics.pyandruff checkfor the same paths.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Observed failure class (sanitized):
Verification:
The full-suite checkbox remains unchecked because the canonical suite was not completely green. The baseline comparison used the same
scripts/run_tests.shrunner and environment; its failing-file set covered every file that failed on this branch.