Skip to content

fix(slack): avoid null table column settings - #57128

Closed
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-slack-table-column-settings
Closed

fix(slack): avoid null table column settings#57128
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-slack-table-column-settings

Conversation

@tianma-if

@tianma-if tianma-if commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace invalid null Slack table column_settings entries with schema-valid objects and omit trailing default columns.
  • If Slack rejects a rendered chat.postMessage payload with invalid_blocks, retry once as plain text.
  • Apply the same fallback to finalized streaming chat.update calls, using blocks=[] so an existing block layout is cleared.
  • Preserve workspace routing by resolving the metadata-selected client once and reusing it for both attempts.

Tests

  • Table settings contain only objects and omit trailing defaults.
  • Post fallback retries without blocks on the same workspace client.
  • Final edit fallback clears blocks on the same workspace client.
  • .venv/bin/python -m pytest tests/gateway/test_slack_block_kit.py tests/gateway/test_slack_block_kit_adapter.py tests/gateway/test_slack.py -q (277 passed)
  • .venv/bin/python -m ruff check plugins/platforms/slack/adapter.py plugins/platforms/slack/block_kit.py tests/gateway/test_slack_block_kit.py tests/gateway/test_slack_block_kit_adapter.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competes with open PR #56618, which also fixes #56615 (null column_settingsinvalid_blocks) by hardening the merged #56102 Block Kit path. Same goal, independent implementations — flagging for a maintainer to pick one; not marking either as a duplicate. Related: #56615 (issue), #56102 (the merged rich-blocks feature this hardens).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting the live rich-table failure; current main still emits None placeholders in plugins/platforms/slack/block_kit.py:288-300.

Problems

  • The new client = self._get_client(chat_id) in this PR's plugins/platforms/slack/adapter.py:1407 drops current workspace routing. Main's live send path uses _get_client(chat_id, team_id=self._metadata_team_id(metadata)) at plugins/platforms/slack/adapter.py:1447-1449; preserve that on both retry attempts and test it with metadata.
  • The no-block recovery is only added to send(). Finalized streaming edits also render blocks and call chat_update in plugins/platforms/slack/adapter.py:1541-1547, so an invalid_blocks response there still fails. Add a matching fallback that sends blocks=[] to clear a previous layout.

Suggested changes

  • Salvage the retry into the current workspace-aware send call and assert both client selections.
  • Add the finalized-edit fallback and regression coverage.

This is an automated hermes-sweeper review.

@@ -1407,7 +1407,19 @@ async def send(
if broadcast and i == 0:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current main routes outbound sends through _get_client(chat_id, team_id=self._metadata_team_id(metadata)) (plugins/platforms/slack/adapter.py:1447-1449). Retain that metadata on the initial post and retry; this unqualified lookup can route a multi-workspace response through the primary client.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified at the current head (aff6d4835): the metadata-selected client is resolved with team_id=self._metadata_team_id(metadata) and reused for both the initial request and fallback. The adapter regression test asserts _get_client.assert_called_once_with("C1", team_id="T_OTHER"). The focused Slack suite passes 277/277. This concern is addressed; leaving thread resolution to a maintainer.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026

tishan-jayasekera commented Jul 21, 2026

Copy link
Copy Markdown

Independent validation evidence for this PR:

  • reproduced invalid_blocks / must provide an object when a mixed-alignment table emitted a null column_settings placeholder
  • confirmed an object-only placeholder payload posts successfully in a private DM canary
  • checked out this PR's exact head (aff6d4835) and ran scripts/run_tests.sh tests/gateway/test_slack_block_kit.py tests/gateway/test_slack_block_kit_adapter.py tests/gateway/test_slack.py -q: 277 passed, 0 failed
  • git diff --check origin/main...HEAD is clean
  • the earlier sweeper concerns are addressed at the current head: workspace-specific client routing is preserved and finalized chat.update calls have the same fallback

This renderer correction plus the plain-text retry is more complete than my narrower duplicate #68434, which I closed in favor of this canonical PR.

@teknium1, could you please take the maintainer merge look when convenient? The PR is currently mergeable with all required checks green.

@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #69317 (merged): same column_settings fix as #56618, submitted later; #56618 was credited as earliest.

Thanks for digging into this — the consolidated fix stands on the cluster's collective analysis, and your work is credited in #69317's summary.

@teknium1 teknium1 closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants