Skip to content

fix: support markdown media attachments#33206

Closed
1TomAwesome wants to merge 1 commit into
NousResearch:mainfrom
1TomAwesome:fix/markdown-media-attachments
Closed

fix: support markdown media attachments#33206
1TomAwesome wants to merge 1 commit into
NousResearch:mainfrom
1TomAwesome:fix/markdown-media-attachments

Conversation

@1TomAwesome

Copy link
Copy Markdown

Summary

  • Allow Telegram/gateway MEDIA: tags to reference Markdown and common document/code/archive files.
  • Add regression coverage for Markdown document extraction.
  • Add streaming Telegram delivery coverage so Markdown MEDIA: tags route to document upload instead of being left as plain text.

Why

MEDIA:/path/to/file.md was ignored because the gateway parser allowlist did not include .md. This caused Markdown plans to appear as plain text paths instead of Telegram document attachments.

Test Plan

  • python -m pytest tests/gateway/test_tts_media_routing.py::test_extract_media_accepts_markdown_document_tags tests/gateway/test_tts_media_routing.py::test_streaming_delivery_routes_markdown_media_tag_to_document_sender -q -o 'addopts='
  • python -m pytest tests/gateway/test_tts_media_routing.py tests/gateway/test_extract_local_files.py tests/tools/test_send_message_telegram_proxy.py -q -o 'addopts='
  • git diff --check -- gateway/platforms/base.py tests/gateway/test_tts_media_routing.py

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels May 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competes with multiple open PRs for the same MEDIA .md allowlist fix: #29374, #30192, #30193, #32535, #33089. Preferred approach is #29609 (dynamic derivation from SUPPORTED_DOCUMENT_TYPES). Related issue: #31560.

GodsBoy added a commit to GodsBoy/hermes-agent that referenced this pull request May 29, 2026
…sResearch#31733)

Builds on the direction in NousResearch#31764 with a broader cut at the same bug
surface that surfaces in operator reports as "MEDIA tag did not attach":

* MEDIA_DELIVERY_SAFE_ROOTS now lists the canonical
  ~/.hermes/cache/{images,audio,videos,documents,screenshots} explicitly
  alongside the legacy ~/.hermes/{type}_cache/ entries. get_hermes_dir
  returns the legacy path when it exists, so IMAGE_CACHE_DIR alone drops
  the canonical layout on co-existing installs (root cause of NousResearch#31733).

* validate_media_delivery_path now delegates to a private
  _validate_media_delivery_path_with_reason helper that returns
  (resolved_or_None, kebab_reason). filter_media_delivery_paths and
  filter_local_delivery_paths log the reason plus a redacted path
  (_redact_path_for_log), so operators chasing rejected MEDIA tags can
  tell allowlist-miss from stale-mtime from denied-prefix from
  does-not-resolve without rebuilding the bot with debug logging.

* _redact_path_for_log neutralises control characters (newline, NUL,
  carriage return) so a model-emitted path containing an embedded
  newline cannot forge a second log line.

* Hoisted _FINAL_RESPONSE_IMAGE_EXTS / _FINAL_RESPONSE_VIDEO_EXTS to
  module scope so the regression test imports the same partition the
  final-response dispatch actually uses, instead of snapshotting a
  local copy that could drift.

* Regression coverage added in tests/gateway/test_platform_base.py:
  - default safe-roots tuple contains both legacy and canonical layouts
  - legacy and canonical image caches co-exist (the strengthened
    failure-shape test NousResearch#31764 review asked for)
  - PDF deliverable from canonical ~/.hermes/cache/documents and
    legacy ~/.hermes/document_cache
  - filter_*_delivery_paths logs the redacted path and the kebab
    reason tag (using the imported _MEDIA_REASON_* constants, not raw
    strings) for stale-mtime, no-recency, does-not-resolve,
    denied-prefix, and newline-injection cases
  - validate_media_delivery_path delegation invariant: public API
    matches the resolved path returned by the reason-bearing helper
  - send_message tool and final-response paths share extract_media +
    filter_media_delivery_paths (asserted via source introspection so
    a future split surfaces here)
  - PDF MEDIA tag routes to send_document via the production
    extension partition (no image / video misroute)

Scope: gateway media delivery only. Does not duplicate NousResearch#32472's
extension-derivation refactor or NousResearch#31561 / NousResearch#33206's markdown attachment
recognition work; if any of those land first, the diagnosable-logging
contract here still applies and the canonical-root entries become a
clean merge against NousResearch#31764.

Deferred follow-up (out of scope for this PR):
- Surfacing the rejection reason to the calling agent (send_message
  tool currently returns {success: true} regardless of dropped
  attachments)
- Unifying the three dispatch-site extension sets across
  _process_message_background, _deliver_media_from_response, and the
  send_message tool path
- Pre-existing residual: symlink-swap of a cache root before
  image_generate writes; TOCTOU between resolve() and upload;
  /var/tmp / /var/cache not in denylist
@teknium1

Copy link
Copy Markdown
Contributor

Superseded by #34844, which consolidates this cluster.

This PR widens the extract_media extension allowlist, which is the right direction — but on its own it leaves the unconditional MEDIA:\s*\S+ strip in place, so a MEDIA: tag with any extension still outside the (now wider) list keeps getting deleted from the body before extract_local_files can pick up the bare path. #34844 fixes both halves: it unifies the two extractors onto a single shared extension set (MEDIA_DELIVERY_EXTS) AND replaces the loose strip with an extension-anchored one, so an unknown-extension path survives in the text instead of vanishing.

Closing as superseded — thanks for surfacing and helping pin down this bug; it was part of getting the full fix right. See #34844.

@teknium1 teknium1 closed this May 29, 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 P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants