Skip to content

fix(media): route attachments via a fail-closed per-adapter MEDIA_KINDS capability - #36817

Open
banditburai wants to merge 12 commits into
NousResearch:mainfrom
banditburai:fix/media-attachment-routing
Open

fix(media): route attachments via a fail-closed per-adapter MEDIA_KINDS capability#36817
banditburai wants to merge 12 commits into
NousResearch:mainfrom
banditburai:fix/media-attachment-routing

Conversation

@banditburai

Copy link
Copy Markdown
Contributor

Adds a fail-closed per-adapter MEDIA_KINDS capability descriptor on BasePlatformAdapter, consulted at the three in-scope send-pipeline dispatch sites (the send_message tool path, the agent reply path, and the kanban notifier), so a platform that can't natively deliver a given media kind warns-and-skips instead of leaking a local file path as chat text.
12 commits · 33 files · +798/−30 · 25 adapters declare capability (24 × +2 lines: import + declaration; email also adds a substantive override); logic concentrated in base.py / run.py / send_message_tool.py / email.py

Problem

Base send_* methods are text-emitters, not deliverers — any media kind that reaches a stub is posted as 📎 File: / 🖼️ Image: / 🎬 Video: / 🔊 Audio: + the local filesystem path. The send pipeline had no capability negotiation, so:

  • The send_message tool path covered media for only 7 hand-tuned platform branches; every other platform fell through to the text-only path → path leaked or media silently dropped (tools/send_message_tool.py::_send_to_platform).
  • _send_via_adapter forwarded chunk but dropped media_files entirely — qqbot attachments broken ([Bug]: _send_via_adapter ignores media_files parameter — file attachments broken for qqbot #23760).
  • The agent reply path (gateway/run.py::_deliver_media_from_response) and kanban notifier (_deliver_kanban_artifacts) dispatched send_image_file/send_voice/send_video/send_document unconditionally; any adapter lacking that override inherited the leaking base stub.

Three distinct dispatch sites, one defect class: the dispatcher never asks the adapter what it can actually deliver. (Two further sibling sites — the background-task completion path and the auto-TTS voice reply — share this defect class but are out of scope for this PR; see Out of scope.)

Approach / Design

  • DescriptorMediaKind enum (IMAGE/VIDEO/VOICE/DOCUMENT) + MEDIA_KINDS: frozenset[MediaKind] on BasePlatformAdapter, fail-closed default frozenset() (an adapter advertises nothing until it declares). Hand-declared per adapter; pinned and backing-asserted by tests/gateway/test_media_kinds.py.
  • Classifierclassify_media_kind mirrors the reply-path routing exactly: force_document/[[as_document]] wins → DOCUMENT, image/video by extension, audio via should_send_media_as_audio, unknown → DOCUMENT. One classifier, identical decisions at all three sites.
  • Three consult sites — tool path adds a unified capability branch (_resolve_live_adapter → per-file classify_media_kind_dispatch_media_one); reply path and kanban notifier each gate every kind with if kind not in adapter.MEDIA_KINDS: warn + skip.
  • Ordering — the unified branch runs after the 7 hand-tuned branches and before the text-only fallback, and only when media_files + a live adapter + non-empty MEDIA_KINDS are all present. Hand-tuned quirks are preserved, generic coverage is added, and the text fallback stays reachable for non-media sends.

Design decisions

  • Hand-declared, not auto-derived from which send_* are overridden — structural ≠ semantic: dingtalk/simplex override media methods but deliberately refuse/emit-text, so they declare EMPTY. Auto-derivation would mis-mark them capable and re-open the leak.
  • Fail-closed empty default over permissive — a wrong-positive declaration re-opens the path-as-text leak; safest default is "delivers nothing."
  • Skip-and-warn over silent-drop and over reroute-to-another-kind — the user gets honest partial delivery plus a warning, never a leaked path and never a surprise kind substitution.
  • Additive after the 7 branches over replacing them — the 7 encode real per-platform quirks (e.g. Telegram voice/audio ext split); the generic path is purely additive.
  • email send_image_file delegates to _send_email_with_attachment (the same helper send_document uses) so IMAGE and DOCUMENT stay symmetric and honor the SendResult contract.
  • Test-pinned + backing-asserted map over docs-only — the declared set is a correctness contract: each declared kind must override the real dispatch method, enforced executably in CI.

Adapter capability matrix

Fail-closed default = none; the declared set below is pinned by tests/gateway/test_media_kinds.py.

Adapters IMG VID VOICE DOC
telegram, slack, signal, matrix, whatsapp, wecom, bluebubbles, feishu, qqbot, weixin, discord, google_chat, mattermost
email, yuanbao · ·
line ·
teams · · ·
dingtalk, sms, homeassistant, webhook, msgraph_webhook, simplex, irc, ntfy · · · ·

EMPTY tier = structurally text-only or deliberately refuses (dingtalk/simplex override but emit-text). The 7 hand-tuned branches (telegram/weixin/discord/matrix/signal/yuanbao/feishu) and the out-of-process standalone path are left as-is; the matrix governs the unified tool branch + reply + kanban gates.

Linkage

Ref Keyword Notes
#18422 Closes Adds the capability negotiation this issue asks for — any platform with a live adapter now delivers MEDIA natively.
#23760 Closes The unified dispatch branch forwards media_files to qqbot's adapter, so attachments deliver instead of being dropped.
PR #18686 Supersedes Same goal — route plugin media through adapters — generalized to fail-closed capability gating across all three dispatch sites, where #18686 patched only the tool path.
#18831 Refs Handles the silently-dropped-MEDIA half of this report; the native voice-bubble half has a separate root cause in tts_tool.py that this PR doesn't touch.
#10136, #17418, #18161, #24906 Refs Already fixed on main by cdde0c841; referenced here for cluster context.

#10136 / #17418 / #18161 / #24906 were already fixed on main by cdde0c841; they're listed only because they belong to the same issue cluster, and can be closed as already-resolved.

Out of scope

This PR is the routing/capability layer only. The items below share the same defect class or issue but are not addressed here; all are pre-existing, not regressions:

Item Why out of scope
Background-task completion path — gateway/run.py::_run_background_task (ungated send_document) runs in an executor path outside the routing layer the capability gate covers
Auto-TTS voice reply — gateway/run.py::_send_voice_reply (hasattr-gated only) uses a different gating mechanism, not the MEDIA_KINDS dispatch path
Feishu native voice-bubble (#18831) separate root cause in tools/tts_tool.py (want_opus excludes feishu); that file is untouched here

Testing

uv run --extra dev --extra messaging --extra feishu pytest \
  tests/gateway/test_media_kinds.py \
  tests/gateway/test_reply_path_media_capability.py \
  tests/tools/test_send_message_tool.py \
  tests/gateway/test_tts_media_routing.py \
  tests/hermes_cli/test_kanban_notify.py
→ 229 passed

Coverage:

  • Pinned capability map (25 adapters): each adapter's MEDIA_KINDS matches its pin; MediaKind is exactly the four members; base default is empty; PlatformEntry has no media_kinds field (the descriptor is adapter-only, leaving the standalone path uncoupled); classify_media_kind routes png→IMAGE / mp4→VIDEO / pdf→DOCUMENT / mp3→VOICE / ogg(is_voice)→VOICE / force_document→DOCUMENT.
  • Executable backing-invariant: every declared kind must override the method its dispatch site calls (not the leaking base stub); a declared-but-unbacked kind fails CI rather than leaking at runtime.
  • Reply-path + kanban gates: undeclared kind ⇒ skipped with a warning containing the path, zero leak; declared kind ⇒ delivered; partial capability ⇒ only the declared kind delivered.
  • Unified-branch dispatch (10 cases): qqbot live-adapter dispatch ([Bug]: _send_via_adapter ignores media_files parameter — file attachments broken for qqbot #23760) · unsupported-kind warn+skip (no leak) · security gate drops unsafe path · force_documentsend_document · thread_id forwarded · out-of-process (no live adapter)→text fallback + warning · adapter exception→error-dict · media-only all-dropped→explicit non-empty error · media attaches only to the last chunk · partial delivery→success + skip-warning merge.
  • 7-branch regression lock: each dedicated per-platform sender still handles its own media and _resolve_live_adapter is never called for those branches.

Regression & blast radius

  • 7 hand-tuned per-platform branches: behavior-unchanged (locked by the regression test).
  • Standalone / out-of-process path: behavior-unchanged (descriptor intentionally absent from PlatformEntry; pinned).
  • Reply-path + kanban gates: purely subtractive — skip-and-warn only; never reroute, never add a delivery channel.
  • _send_via_adapter refactor (_resolve_live_adapter extraction): behavior-identical.
  • Fail-closed default is safe for the 8 EMPTY-MEDIA_KINDS adapters — they now skip-and-warn instead of leaking.
  • Scope: of the 25 adapter files, 24 are +2-line declarations (import + MEDIA_KINDS, no logic); email is the exception (+35 — the new send_image_file override). Behavioral change is concentrated in base.py (+35/−0), email.py (+35/−0), run.py (+41/−7), send_message_tool.py (+82/−22).

…tch-method backing

The unified media branch dispatches MediaKind.IMAGE via send_image_file
(_dispatch_media_one), but EmailAdapter declared {IMAGE, DOCUMENT} while
overriding only send_image/send_multiple_images — so a single image to
email inherited the base stub and leaked the local path as body text
(🖼️ Image: /path), the exact leak this epic closes. The reply/kanban
paths were unaffected because they batch via send_multiple_images, which
email does override.

- Add EmailAdapter.send_image_file delegating to _send_email_with_attachment
  (the same helper send_document uses), so IMAGE and DOCUMENT deliver
  symmetrically and honor the SendResult contract.
- Harden the MEDIA_KINDS pin: assert every declared kind overrides the
  method its dispatch site actually calls (IMAGE→send_image_file, etc.),
  making the descriptor's documented "must be backed by a real override"
  invariant executable so the next adapter can't silently re-open the leak.

Closes NousResearch#18422, NousResearch#23760
…very warning merge

Two edge cases in the unified capability branch that lacked explicit coverage:
- media attaches only to the final text chunk (never glued to an earlier one)
- a partially-deliverable set returns success with the delivered message_id
  while still surfacing the skip warning for the undeliverable kind

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Automated PR Review

Security Scan

  • ✓ No hardcoded secrets, injection sinks, unsafe deserialization, or dependency red flags found by this automated scan.

Code Quality

  • ✓ No blocking code-quality issues found by this automated scan.

Summary

Status: APPROVE — security findings: 0, quality suggestions: 0.

Automated review; raw diff content intentionally omitted.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists labels Jun 1, 2026

@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 thorough routing analysis and capability/backing tests. The native cross-channel delivery gap is still present: current tools/send_message_tool.py:1043-1104 warns that attachments were omitted, and #17261 independently reports that behavior.

Problems

  • The kanban target moved after this PR: _deliver_kanban_artifacts is now gateway/kanban_watchers.py:635 following 1c68f6f81f, so the gateway/run.py hunk and its test need porting.
  • tools/send_message_tool.py:540 constructs metadata with only thread_id. Current main preserves NTFY publish_topic metadata in the analogous live-adapter path at tools/send_message_tool.py:720-725; retain that behavior when salvaging.
  • The path-leak fallback itself is already fixed by cb9d18c759 in gateway/platforms/base.py:3364-3489; current work should concentrate on native attachment routing.

Suggested changes

  • Port the capability gate to the current kanban mixin and current plugin adapter layout.
  • Share or preserve the NTFY metadata construction and cover it with a regression test.

Automated hermes-sweeper review.

if result.success:
return {"success": True, "message_id": result.message_id}
return {"error": f"Adapter send failed: {result.error}"}
metadata = {"thread_id": thread_id} if thread_id else None

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.

When salvaging onto current main, preserve NTFY's publish_topic=chat_id metadata too. The current live-adapter path builds it at tools/send_message_tool.py:720-725; keeping only thread_id here would make NTFY publish to its default topic rather than the requested target.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 13, 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 sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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 sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants