Skip to content

fix(slack): preserve legacy attachment URLs - #69620

Open
jcarugati wants to merge 1 commit into
NousResearch:mainfrom
jcarugati:fix/slack-attachment-urls
Open

fix(slack): preserve legacy attachment URLs#69620
jcarugati wants to merge 1 commit into
NousResearch:mainfrom
jcarugati:fix/slack-attachment-urls

Conversation

@jcarugati

Copy link
Copy Markdown

What does this PR do?

Preserves HTTP(S) URLs carried by legacy Slack attachments when rendering fetched thread context and thread-parent reply context.

Current main already merges visible legacy attachment text and top-level Block Kit URLs. However, its URL walker only receives top-level blocks, so attachment fields such as title_link, author_link, and from_url—plus URLs in attachment-nested Block Kit—are omitted even though the attachment title/text remains visible.

This follows up on #69316 and keeps the same renderer, URL filtering, and discovery-order deduplication.

Related Issue

Follow-up to #69316. Searches for the affected legacy attachment URL fields found no open issue or equivalent PR.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • Generalize the existing Slack URL walker to cover both top-level Block Kit and legacy attachment URL fields.
  • Feed the renderer's blocks and attachments through that single walker.
  • Add a regression test for title_link, author_link, from_url, and attachment-nested Block Kit URLs.

How to Test

  1. Render a Slack message whose visible title/text is in a legacy attachment and whose links are only in attachment URL fields or nested Block Kit.
  2. Confirm all attachment URLs appear in _render_message_text output alongside existing top-level Block Kit URLs.
  3. Run the focused Slack suite below.

Validation

  • scripts/run_tests.sh tests/gateway/test_slack*.py -q — 544 passed
  • .venv/bin/ruff check . — passed
  • .venv/bin/python scripts/check-windows-footguns.py --all — passed
  • .venv/bin/python -m compileall -q plugins/platforms/slack/adapter.py tests/gateway/test_slack_approval_buttons.py — passed
  • git diff upstream/main...HEAD --check — passed

Compatibility

  • No configuration, dependency, session, or outbound Slack payload changes.
  • URL collection remains limited to HTTP(S) and preserves first-seen order with duplicate removal.
  • The change is platform-neutral dictionary traversal.

Checklist

  • I've read the contribution guide and repository instructions.
  • My commit uses the repository's Conventional Commit format.
  • I searched open and closed issues/PRs for an equivalent fix.
  • The PR contains only this Slack thread-rendering fix and its regression test.
  • Documentation, configuration, architecture, and tool-schema updates are N/A.
  • Cross-platform impact was considered and the repository's Windows footgun check passes.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter labels Jul 22, 2026
@jcarugati
jcarugati marked this pull request as ready for review July 23, 2026 00:49
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Slack rendering fix. The premise remains valid on current main: plugins/platforms/slack/adapter.py:641 recognizes only Block Kit URL keys, and _render_message_text passes only top-level blocks to that walker at plugins/platforms/slack/adapter.py:7158. The proposed generic traversal preserves the existing HTTP(S), first-seen-order, and deduplication behavior while adding the attachment fields and nested blocks that current rendering omits.

No additional correctness or design-fit changes identified. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 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-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.

3 participants