fix(slack): bridge rich_blocks opt-in from a top-level slack: block to extra - #58691
kamonspecial wants to merge 1 commit into
Conversation
|
The failing |
b81ac0f to
3c3ace4
Compare
|
Thanks for the focused Slack configuration bridge. The premise is confirmed on current main: The canonical This is an automated hermes-sweeper review. |
54ebaec to
08e18ef
Compare
|
Rebased onto current Two notes on scope:
|
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs are listed, but they address distinct Slack defects: #58691 bridges the top-level rich_blocks opt-in into PlatformConfig.extra, while #64267 classifies transient chat.update transport failures as retryable and preserves progress-message identity across normal and overflow edits.
Related pull requests
- #58691
related— (+132/-9) — merge: The diff directly fixes the reported configuration-path mismatch by returning rich_blocks from Slack’s YAML hook for merging into PlatformConfig.extra, with coverage for flat and nested top-level forms, precedence, false values, malformed extra data, and the adapter reader. - #64267 [closed]
related— (+389/-30) — already integrated, not a duplicate: This closed PR addresses an unrelated Slack progress-edit failure mode by making transient transport failures retryable. Despite the earlier keep_open review on #64267, the shown diff explicitly fixes its blocking overflow-path concern by retaining can_edit on retryable failures and adds an overflow regression; the contributor confirms that the implementation was merged via #70189.
Suggested consolidation
Merge #58691 as the focused fix for the inert top-level Slack rich_blocks configuration path. #64267 should remain closed because its separate network-retry fix was merged via #70189; no listed PR should be closed as a duplicate of #58691.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 31 kB of PR diffs, 11 kB of issue/PR text, 4 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
…o extra SlackAdapter._rich_blocks_enabled() reads the opt-in from self.config.extra["rich_blocks"], but under a top-level `slack:` block that key never reached extra: it is not a shared key, is not env-bridged, and _merge_platform_map only deep-merges the nested `extra` sub-dict for `platforms:`-nested blocks. So `slack.rich_blocks` / `slack.extra.rich_blocks` under the top-level `slack:` block — where every sibling key (require_mention, allow_bots, …) lives — was silently inert, and Block Kit rendering (headers, dividers, rich_text lists, native `table` blocks) stayed off there. (`platforms.slack.extra.rich_blocks` already worked via _merge_platform_map.) Have the slack _apply_yaml_config hook return the rich_blocks value so the gateway merges it into PlatformConfig.extra — the same returned-dict dispatch telegram's hook uses (`extras or None` -> extra.update()). The flat `slack.rich_blocks` wins over the nested `slack.extra.rich_blocks`, matching the discord/telegram hooks' flat-over-nested precedence. Coercion stays the reader's job; env-only keys and the return-None-when-absent contract are unchanged. Adds unit tests for the hook (flat/nested/string/false/precedence/non-dict) and end-to-end load_gateway_config dispatch tests for both top-level forms.
08e18ef to
2a0cc70
Compare
|
Rebased onto current main (fa83af3) — the conflict is cleared and the branch is mergeable again. |
What does this PR do?
SlackAdapter._rich_blocks_enabled()reads the Block Kit opt-in fromself.config.extra["rich_blocks"]. But under a top-levelslack:block —the surface where every sibling key lives (
require_mention,allow_bots,free_response_channels, …) — that key never reachedextra:require_mentionet al., which_apply_yaml_configtranslates intoSLACK_*env vars), and_merge_platform_maponly deep-merges the nestedextrasub-dict for blocksunder
platforms:/gateway.platforms:, not for a top-levelslack:block.So
slack.rich_blocks/slack.extra.rich_blocksunder a top-levelslack:block was silently inert:
_rich_blocks_enabled()stayedFalse,_maybe_blocks()returnedNone, and outbound agent messages went out as flatmrkdwn
textregardless of config. The Block Kit rendering that already existsin the adapter (headers, dividers,
rich_textlists, and nativetableblocks) was unreachable from the top-level block — most visibly with markdown
tables, since Slack has no table syntax and
| ... |renders as raw pipes.Fix: have the slack
_apply_yaml_confighook return therich_blocksvalue so the gateway merges it into
PlatformConfig.extravia the returned-dictdispatch the hook contract already provides (
extra.update(seeded)). Thismirrors telegram's hook, which returns
extras or Nonefor the same reason.Keeping the bridge in the slack plugin — rather than adding a
gateway/config.pywhitelist entry — is the point of the plugin
apply_yaml_config_fnmigration(#24849).
slack.rich_blocksand the nestedslack.extra.rich_blocksforms are accepted; the flat form wins when both are set, matching the
discord/telegram hooks' flat-over-nested precedence.
_rich_blocks_enabled()alreadyhandles
true/1/on/…).Nonewhen norich_blocksis present — no behaviour change for existing configs.(
platforms.slack.extra.rich_blocksalready worked, because_merge_platform_mapdeep-merges the nested
extrasub-dict forplatforms:-nested blocks. Thischange makes the top-level
slack:forms behave the same as that documentedpath.)
Related Issue
No existing issue — self-contained bug fix; root cause and repro are described
above. (Searched open issues/PRs for the config-bridge gap; none found.)
Complementary to the open rendering-side PR #56618 (harden rich table block
fallback), which hardens
block_kit.py/ the fallback path againstinvalid_blocks. This PR is orthogonal: it fixes the config→extrawiring sothe opt-in is reachable in the first place, and does not touch
_apply_yaml_configin #56618 (no overlap there).Type of Change
Changes Made
plugins/platforms/slack/adapter.py—_apply_yaml_confignow returns{"rich_blocks": <value>}(wasreturn None) so the gateway merges it intoPlatformConfig.extra; accepts flatslack.rich_blocksand nestedslack.extra.rich_blocks, flat wins. Updated the_rich_blocks_enabledand_apply_yaml_configdocstrings and theregister()comment to reflect thebridged surface.
tests/gateway/test_slack_block_kit_adapter.py— newTestApplyYamlConfigRichBlocksBridge(flat, nested, string,false,flat-wins-over-nested precedence, non-dict
extra, absent, hook→readerroundtrip).
tests/gateway/test_config.py— end-to-endload_gateway_config()dispatchtests for both top-level forms reaching
config.platforms[Platform.SLACK].extra["rich_blocks"].How to Test
Repro (before this change): in
config.yaml, under a top-levelslack:block, set
extra: { rich_blocks: true }; ask the agent to reply with amarkdown table. It posts as raw
| ... |pipes (notableblock), becauseextra["rich_blocks"]is never populated.With this change: the same config posts a native Block Kit
tableblock.pytest tests/gateway/test_slack_block_kit_adapter.py tests/gateway/test_config.py -qreturn None(slack isn't evenseeded into
config.platforms, so the e2e assertions raiseKeyError),confirming they're real regression guards.
slack.extra.rich_blocks: trueunder a top-level
slack:block, a 5-column markdown table posts as a nativetableblock, noinvalid_blockserrors.Checklist
Code
fix(slack): …)test_slack_block_kit_adapter.py,test_config.py,test_platform_registry.py: 146/146 pass. (The fulltests/gateway/run has ~10 unrelated timing/environment-flaky failures thatalso fail on the base commit; none are in files this PR touches.)
Documentation & Housekeeping
_rich_blocks_enabled/_apply_yaml_configdocstrings and theregister()commentcli-config.yaml.exampleif I added/changed config keys —N/A (
rich_blocksis a pre-existing documented key; no new key introduced)CONTRIBUTING.mdorAGENTS.mdif I changed architecture — N/Afile/process/terminal I/O)