Skip to content

fix(gateway): render Slack attachment thread context - #32022

Closed
gijss wants to merge 1 commit into
NousResearch:mainfrom
gijss:fix/slack-thread-attachment-context
Closed

fix(gateway): render Slack attachment thread context#32022
gijss wants to merge 1 commit into
NousResearch:mainfrom
gijss:fix/slack-thread-attachment-context

Conversation

@gijss

@gijss gijss commented May 25, 2026

Copy link
Copy Markdown

What does this PR do?

Slack app messages can put the useful parent content in legacy attachments while leaving top-level text empty. Thread backfill only read text, so a reply in one of those threads could reach the agent without the ticket or alert details it was replying to.

This reuses the Slack message renderer for live messages, thread history, and reply_to_text. Historical thread context keeps readable block text, but does not include the full Block Kit JSON payload that current messages still expose.

Related Issue

No linked issue.

Overlap: #30765 fixes the same Slack legacy-attachment thread-context failure. This PR folds that behavior into the shared renderer and adds coverage for dedupe, truncation, parent-text lookup, and avoiding Block Kit JSON in historical context.

Prior related work: #16213 and #16281.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/platforms/slack.py: render Slack text, blocks, and legacy attachments through one helper.
  • gateway/platforms/slack.py: use that renderer for thread backfill and parent reply_to_text, with bounded attachment output.
  • tests/gateway/test_slack.py, tests/gateway/test_slack_approval_buttons.py: add regressions for attachment-only parents, fallback handling, truncation, and Block Kit history behavior.

How to Test

  1. Reply in a Slack thread whose parent has empty text and useful legacy attachments.
  2. Confirm the agent sees the parent attachment text, fields, footer, and fallback.
  3. Confirm historical thread context includes readable Block Kit text but not [Slack Block Kit payload ...] JSON.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 / Darwin via targeted Slack gateway tests

Full local scripts/run_tests.sh completed on macOS/Python 3.12 after installing .[all,dev]: 25,806 passed, 26 failed. The failures are outside this PR's Slack path and are local platform/runtime issues (Python 3.12 coroutine-frame assertions, Linux/systemd assumptions on macOS, temporary-HOME path behavior, and local browser/tooling assumptions). The changed Slack files passed in that full run.

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

uv run --extra dev ruff check .
uv run python scripts/check-windows-footguns.py --all
scripts/run_tests.sh tests/gateway/test_slack.py tests/gateway/test_slack_approval_buttons.py
scripts/run_tests.sh tests/gateway/test_slack_channel_skills.py tests/gateway/test_slack_mention.py
HOME=$(mktemp -d) scripts/run_tests.sh  # 25,806 passed, 26 unrelated local failures
git diff --check

Copilot AI review requested due to automatic review settings May 25, 2026 11:16

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds richer Slack message rendering so agents can see meaningful content when integrations (e.g., Zendesk) send empty top-level text and place payload into legacy attachments, including in thread parent backfills.

Changes:

  • Centralized Slack message rendering into _render_slack_message_text() (text + blocks + attachments).
  • Added attachment field/footer extraction so “attachment-only” integration messages are visible.
  • Added tests covering attachment-only thread parents and attachment field rendering in message handling.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
gateway/platforms/slack.py Introduces shared rendering helpers and uses them in message handling + thread context/parent fetches.
tests/gateway/test_slack_approval_buttons.py Adds coverage for attachment-only thread parent rendering in context + parent-text fetch.
tests/gateway/test_slack.py Adds coverage ensuring attachment fields/footer content becomes visible for integration messages.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gateway/platforms/slack.py Outdated
Comment thread gateway/platforms/slack.py Outdated
Comment thread gateway/platforms/slack.py Outdated
@gijss
gijss force-pushed the fix/slack-thread-attachment-context branch from e11cffa to c1cacb0 Compare May 25, 2026 11:32
@daimon-nous daimon-nous Bot added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter labels May 25, 2026
@daimon-nous

daimon-nous Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Overlaps with #30765 — both add Slack legacy attachment rendering for thread context. This PR is more comprehensive (unified renderer, dedup, truncation, Zendesk-style coverage). See also merged #16213 and #16281 for prior work.

@gijss gijss changed the title Fix Slack thread context for attachment-only messages fix(gateway): render Slack attachment thread context May 25, 2026
@gijss
gijss force-pushed the fix/slack-thread-attachment-context branch from c1cacb0 to 0216c45 Compare May 25, 2026 11:39

@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 the comprehensive shared-renderer approach. The underlying defect is still present on current main: plugins/platforms/slack/adapter.py:3788-3790 skips attachment-only thread messages after reading only text, and :3891 does the same for cold reply_to_text fetches.

Problems

  • The submitted production change targets gateway/platforms/slack.py, but 5600105478ffde29d7566b45421b100eaa29c4ef moved the active adapter to plugins/platforms/slack/adapter.py. The submitted file is no longer the runtime implementation.
  • Current Slack tests import the plugin adapter (tests/gateway/test_slack.py:67-71; tests/gateway/test_slack_approval_buttons.py:45), so the regression coverage must move with the implementation.

Suggested changes

  • Port the shared renderer and its live-message, thread-backfill, and parent-text call sites to plugins/platforms/slack/adapter.py.
  • Preserve the proposed include_block_payload=True behavior only for live events; historical thread context should remain readable text without the serialized Block Kit payload.

Automated hermes-sweeper review.

"Slack: appended %d link unfurl(s) to message text",
len(att_parts),
)
text = _render_slack_message_text(

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 no longer loads this file: 5600105478ffde29d7566b45421b100eaa29c4ef moved SlackAdapter to plugins/platforms/slack/adapter.py. Port this renderer call and its helpers there, otherwise the active Slack path remains unchanged.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #69316 (merged): targeted the pre-plugin-migration file; covered by the consolidated helper.

Thanks for digging into this — the consolidated fix stands on the cluster's collective analysis, and your work is credited in #69316'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/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants