Skip to content

fix(send_message): route plugin platform media via adapters - #18686

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/plugin-platform-media-18422
Open

fix(send_message): route plugin platform media via adapters#18686
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:fix/plugin-platform-media-18422

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

Summary

  • route plugin platform MEDIA attachments through the live gateway adapter instead of dropping them in the non-media fallback
  • dispatch images, audio/voice, and documents using the adapter media methods
  • preserve media-only sends for plugin platforms and report adapter errors clearly

Fixes #18422

Verification

  • scripts/run_tests.sh tests/tools/test_send_message_tool.py -k 'PluginMedia or Whatsapp'\n- scripts/run_tests.sh tests/tools/test_signal_media.py::TestSendSignalMediaRestrictions::test_non_media_platforms_reject_text_only_media (skipped locally: httpx/telegram compatibility guard)\n- scripts/run_tests.sh tests/tools/test_send_message_tool.py\n- git diff --check origin/main...HEAD

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets labels May 2, 2026
@fcfangcc

Copy link
Copy Markdown

@LeonSGP43 Our custom plugin depend on this feature

Can you grant me permission to edit this branch?
I will help fix this issue

@shashb27

Copy link
Copy Markdown

Verified this root cause independently on Windows 11 / Telegram DM delivery.

I reproduced the exact behavior this PR is addressing:

  • live-adapter path delivered text only
  • no native image attachment arrived
  • direct Telegram Bot API sendPhoto with the same JPG worked, so the asset/platform were fine

The key local observation matched this PR direction:
_send_via_adapter() only called adapter.send(...) and ignored media_files in the live-adapter branch.

I validated the dispatch-style fix locally in a clean worktree with a focused regression test:

  • added test_live_adapter_path_sends_image_attachment
  • ran:
    pytest -q -o addopts='' tests/tools/test_send_message_tool.py -k 'live_adapter_path_sends_image_attachment or sends_text_then_photo_for_media_tag'
  • result: 2 passed

I also checked the broader file on clean upstream main; it currently has an unrelated pre-existing failure in test_media_tag_outside_allowed_roots_is_not_sent, so the focused targeted pass is the honest verification signal for this bug.

Based on that repro, this PR is addressing a real user-facing regression, including Telegram (not just plugin-only edge cases).

@chbo297

chbo297 commented May 28, 2026

Copy link
Copy Markdown

I compared this PR with a separate local split of plugin send_message media routing work.

This PR looks like the better primary fix for #18422 because it covers the live-adapter path more broadly than my local variant: image, audio/voice, documents, and media-only sends.

A few follow-up hardening points from that comparison, in case useful:

  • Check that a plugin adapter actually overrides the native media method being called, rather than falling through to BasePlatformAdapter behavior. That avoids accidentally degrading into local-path-as-text leakage.
  • Preserve thread metadata for text + media sends when adapter media methods support metadata.
  • For plugin platforms without a live gateway adapter, consider an explicit PlatformEntry.send_message_media or equivalent opt-in before passing media_files to a standalone sender. That keeps standalone plugins from receiving local paths unless they declared support.
  • For unsupported media on text + media sends, a text-only delivery with a warning may be preferable to silently dropping the text or leaking paths.

I will not open a duplicate PR for the overlapping media-routing change; these are just possible follow-ups/gaps to consider here.

@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #43569 (competing OPEN fix for #18422). Both route plugin-platform media in tools/send_message_tool.py, but via different mechanisms: this PR dispatches through the live gateway adapter's media methods (broader live-adapter coverage), while #43569 routes through the standalone sender (/send-attachment) on the fast path. Cross-linking so a maintainer can pick the canonical approach.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the live-adapter media gap. The premise remains valid on current main: tools/send_message_tool.py:713-734 returns after generic adapter.send(...), while the later plugin standalone_sender_fn path is the one that receives media_files (tools/send_message_tool.py:743-752).

Problems

  • The proposed helper drops thread_id: current main builds adapter metadata at tools/send_message_tool.py:720-727, but the added helper neither accepts nor forwards it. Media sent to threaded plugin targets would lose its thread association.
  • Calling send_image_file / send_voice / send_document is not itself proof of native support. BasePlatformAdapter provides success-returning warning fallbacks for those methods (gateway/platforms/base.py:3329-3466), so a plugin without overrides could report success without delivering an attachment.
  • The helper lacks a send_video branch, unlike established gateway routing (gateway/run.py:13176-13193).

Suggested changes

  • Rework this against current _send_via_adapter and preserve its thread_id, media_files, and force_document contract. The competing fix(send-message): route plugin media through standalone senders #43569 demonstrates a narrower placement: prioritize a media-capable registered standalone_sender_fn before the generic live adapter.send path.
  • Add coverage for threaded media, video media, and inherited base fallback methods.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Jul 12, 2026
@teknium1 teknium1 added 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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists P3 Low — cosmetic, nice to have and removed P3 Low — cosmetic, nice to have 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 P2 Medium — degraded but workaround exists labels Jul 12, 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 comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have 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.

[Feature]: Support MEDIA delivery for plugin platforms via adapter

6 participants