fix(media): add missing extensions to MEDIA delivery whitelist (#37318) - #37342
fix(media): add missing extensions to MEDIA delivery whitelist (#37318)#37342luoxiao6645 wants to merge 1 commit into
Conversation
…esearch#37318) Extensions like .md, .py, .json, .yaml were silently dropped because extract_media() and _TOOL_MEDIA_RE used stale hardcoded extension lists that didn't include common document/code types. Changes: - base.py: add .markdown, .toml, .py, .js, .ts, .sh, .bash, .zsh to MEDIA_DELIVERY_EXTS (now 60 extensions, was 52) - run.py: replace two hardcoded extension regexes with _MEDIA_EXT_ALTERNATION imported from base.py, eliminating the duplicate maintenance that caused the mismatch Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Scope
Adds 8 missing extensions (.markdown, .toml, .py, .js, .ts, .sh, .bash, .zsh) to the MEDIA delivery whitelist and consolidates three separate regex lists into one shared source of truth.
Strengths
- DRY refactor: Replaced two hardcoded inline regex copies in
run.pywith_MEDIA_EXT_ALTERNATIONfrombase.py— future extensions only need one change location. - Good root cause analysis: Identified that only
base.pyhad been updated while two regexes inrun.pywere independently stale. - Well-scoped: 7 additions, 10 deletions. Clean diff, single concern.
- Correct by inspection: The approach (importing the shared alternation fragment) is syntactically and logically sound.
Minor Notes
- No tests added for the new extensions — acceptable for a whitelist change this simple; regression test would be testing that a constant contains a string.
Reviewed by Hermes Agent
|
Thanks for narrowing this to the remaining MEDIA extension drift. Problems
Suggested changes
Current Automated hermes-sweeper review. |
|
Closing as resolved on main: PR #65510 replaced the extension-allowlist approach with universal validated egress — ANY MEDIA: path (.py/.js/.ts/.sh/.toml and everything else) now delivers after passing validate_media_delivery_path. Verified live on main for your target extensions. Your idea of routing run.py's _TOOL_MEDIA_RE through the shared alternation is a reasonable residual improvement for the tool-result auto-append path — that narrow list is now only a secondary precision guard, but if you want to unify it, a focused PR for just that would be welcome. Thanks! |
Problem
MEDIA:/path/to/file.md tags silently fail for common file types (.md,
.json, .yaml, .py, etc.) — the file is never delivered, send_message
returns misleading {success: true}.
Root Cause
Two of the three MEDIA regexes in the codebase used a stale
hardcoded extension list that never grew beyond the original
image/video/archive set. Only base.py's MEDIA_DELIVERY_EXTS had been
updated; run.py's _TOOL_MEDIA_RE and its inline clone at L17764
carried independent, narrower lists.
Changes
gateway/platforms/base.py.markdown.toml.py.js.ts.sh.bash.zshgateway/run.py_MEDIA_EXT_ALTERNATIONfrom base.pyAll three MEDIA regexes now share one source of truth (60 extensions),
so new extensions never need syncing across files again.
Platforms affected
Feishu, Telegram, and any platform using send_document via MEDIA: tags.
Fixes #37318