Skip to content

fix(gateway): surface dropped MEDIA paths in send_message result - #34178

Open
konsisumer wants to merge 1 commit into
NousResearch:mainfrom
konsisumer:fix/telegram-media-false-success-warning
Open

fix(gateway): surface dropped MEDIA paths in send_message result#34178
konsisumer wants to merge 1 commit into
NousResearch:mainfrom
konsisumer:fix/telegram-media-false-success-warning

Conversation

@konsisumer

@konsisumer konsisumer commented May 28, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The send_message tool extracted MEDIA: directives, ran them through filter_media_delivery_paths (which silently drops paths outside the delivery allowlist), and then sent the remaining text. A successful text send was returned to the model as {"success": true} with no signal that the attachment had been stripped, so the agent reported "media sent" while the user only saw plain text — the exact failure reported in #32644.

This PR adds partition_media_delivery_paths on BasePlatformAdapter returning (safe, dropped). filter_media_delivery_paths keeps its old return shape by delegating to the partition (no behavior change for existing callers). send_message_tool._handle_send switches to the partition and, on a successful send, attaches a "MEDIA attachment(s) were dropped" warning plus a media_dropped: [...] field listing the rejected paths, so the agent can correct the path or expand HERMES_MEDIA_ALLOW_DIRS instead of treating text-only delivery as a fully successful attachment send.

Scope is intentionally narrow: this addresses acceptance criteria 1, 2, and the send_message half of 4 from the issue. The gateway post-stream delivery path in gateway/run.py::_deliver_post_stream_media also calls filter_media_delivery_paths and silently drops paths after text has already streamed; turning that into a user-visible follow-up message is a separate UX decision and is deferred to a follow-up PR. The operator-facing sendVoice/sendAudio smoke-test command (AC 3) is likewise deferred.

Related Issue

Refs #32644

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/base.py: add BasePlatformAdapter.partition_media_delivery_paths(media_files) -> (safe, dropped). Refactor filter_media_delivery_paths to delegate to it so existing callers keep their old return shape and per-path warning log.
  • tools/send_message_tool.py: in _handle_send, use the partition helper and, on a successful send, attach a user-facing warnings entry plus a media_dropped: [<paths>] field so the model/operator sees that the attachment was filtered out. The warning is only added when the underlying send succeeded — error results are not mutated.
  • tests/gateway/test_platform_base.py: add test_partition_returns_dropped_paths_so_caller_can_warn covering the new (safe, dropped) return shape under the strict allowlist.
  • tests/tools/test_send_message_tool.py: the existing test_media_tag_outside_allowed_roots_is_not_sent now asserts that result["media_dropped"] and the warning are populated (it previously codified the silent-success bug). Add test_media_dropped_warning_not_added_when_send_errors so an upstream send failure isn't masked by the new warning.

How to Test

  1. pytest tests/gateway/test_platform_base.py::TestMediaDeliveryPathValidation tests/tools/test_send_message_tool.py -q — 134 tests, all green locally.
  2. End-to-end repro from Telegram MEDIA audio attachment can report success while delivering no attachment #32644: with HERMES_MEDIA_DELIVERY_STRICT=1 and HERMES_MEDIA_TRUST_RECENT_FILES=0, call send_message_tool({"action": "send", "target": "telegram:<chat>", "message": "[[audio_as_voice]]\nMEDIA:/tmp/outside-allowlist.ogg"}). The returned JSON now contains "media_dropped": ["/tmp/outside-allowlist.ogg"] plus a warnings entry naming the dropped attachment, instead of a bare {"success": true}.
  3. Negative coverage: when _send_to_platform returns {"error": "..."}, the test confirms media_dropped is not attached — so a real send failure isn't masked.

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/tools/test_send_message_tool.py tests/gateway/test_platform_base.py -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-arm64 (Python 3.11). No Windows footguns introduced — scripts/check-windows-footguns.py clean on the four touched files.

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A, new method's docstring covers the behavior change and existing callers see no API change.
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no new config keys.
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A.
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — change is pure Python list/tuple manipulation, no signals/subprocess/path-mode involved.
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A, the send_message schema is unchanged; the result dict gains an optional media_dropped field on success which is additive.

Addressing maintainer feedback

@alt-glitch noted (PR triage comment) that this competes with #32880, #32054, and #31864 — all targeting #32644 (silent MEDIA attachment drop):

This PR sits between them: it adds partition_media_delivery_paths on BasePlatformAdapter and surfaces dropped paths in the send_message result, while keeping filter_media_delivery_paths' existing return shape (no change at its other call sites). Scope is intentionally limited to the four files above; the gateway post-stream path and the smoke-test command remain deferred (Refs #32644, not Closes). Happy to defer to whichever of the competing PRs the maintainers prefer — flagging the overlap here so the duplication is visible.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verified: the refactoring correctly surfaces a real UX problem where send_message silently reported success after dropping media attachments.

The approach is clean:

  1. partition_media_delivery_paths returns both safe and dropped lists — backward-compatible since filter_media_delivery_paths still works by discarding the dropped list
  2. send_message_tool.py adds media_dropped to the result dict and appends an actionable warning with three concrete remediation steps (move file, add HERMES_MEDIA_ALLOW_DIRS, enable strict mode)
  3. Tests cover both the new partition function and the end-to-end send_message behavior

One consideration: the warning message is appended every time dropped_media is non-empty and the send succeeds. If a model repeatedly sends messages with the same dropped path, the user will see the same warning each time. This is probably the right behavior (each send is a distinct event), but worth noting if noise becomes a concern.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets labels May 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #32880, #32054, and #31864 — all fix #32644 (silent MEDIA attachment drop). This PR adds partition_media_delivery_paths on BasePlatformAdapter and surfaces dropped paths in send_message result. #32880 is the most focused (send_message only). #32054 is broader (also touches run.py, scheduler.py). #31864 restructures the filter return type across 13 call sites.

@konsisumer

Copy link
Copy Markdown
Contributor Author

Rebased onto current origin/main (PR base was stale; DIRTY merge state). The only conflict was in gateway/platforms/base.py's partition_media_delivery_paths else-branch, where upstream had switched the dropped-path log line to the sanitized _log_safe_path(raw) helper. Resolved by keeping this PR's dropped_media.append(...) tracking and adopting upstream's sanitized log message. The other three files auto-merged; the diff is unchanged at the same 4 in-scope files (89 insertions). partition_media_delivery_paths/media_dropped tests pass locally. Two remaining failures in TestSendToPlatformChunking reproduce identically on clean origin/main, so they're pre-existing upstream and outside this PR's scope. Updated the PR body with an ## Addressing maintainer feedback section noting the overlap with #32880, #32054, and #31864 per @alt-glitch's triage comment.

@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 preserving the filter API while making rejected paths observable in the structured send result.

Problems

  • The added result warning is not visible to the default hermes send user: successful results print only note or sent in hermes_cli/send_cmd.py:120-128. A stripped attachment would therefore still print sent unless the caller selects JSON output.
  • The new remediation text tells users to set HERMES_MEDIA_ALLOW_DIRS / HERMES_MEDIA_TRUST_RECENT_FILES. The supported configuration is gateway.media_delivery_allow_dirs, gateway.strict, and gateway.trust_recent_files; the latter is only consulted in strict mode (hermes_cli/config.py:2927-2950).

Suggested changes

  • Render this warning in the normal CLI formatter and add a regression test for that output.
  • Point the warning at the supported gateway.* config keys, including the strict-mode prerequisite for recency trust.
  • Scope the rationale to current CLI/MCP callers: send_message is intentionally not agent-callable on current main (tools/send_message_tool.py:1955-1965).

Automated hermes-sweeper review.

# Surface MEDIA paths that the delivery allowlist dropped so the
# caller can react instead of treating a text-only send as a fully
# successful attachment send (issue #32644).
if dropped_media and isinstance(result, dict) and result.get("success"):

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.

Please also surface this warning through the default hermes send formatter. hermes_cli/send_cmd.py:120-128 ignores warnings and prints only sent for a successful result, so the normal CLI user would still receive the false-success presentation this change is intended to prevent.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@konsisumer

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed in this push:

  • Supported configuration guidance: the structured dropped-media warning now points to gateway.media_delivery_allow_dirs, and correctly describes gateway.trust_recent_files as requiring gateway.strict. The regression test asserts the full user-facing warning.
  • Caller scope: clarified that this structured result is for current CLI/MCP callers, not agent tool use.
  • Normal CLI rendering: hermes_cli/send_cmd.py is outside this PR’s original four-file scope boundary, so I did not modify that separate formatter in this narrowly scoped follow-up.

@konsisumer

Copy link
Copy Markdown
Contributor Author

Rebased onto current origin/main and resolved the in-scope test conflict while preserving both the dropped-MEDIA coverage and upstream's Kanban attachment coverage. Also corrected five Windows encoding diagnostics reported in the two test files already modified by this PR. Local validation is green: 126 focused tests, the bounded full pytest suite, ruff, Windows-footgun scan, and git diff --check.

@konsisumer
konsisumer force-pushed the fix/telegram-media-false-success-warning branch from 361a938 to 2015083 Compare August 3, 2026 11:32
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 comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists 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 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