fix(gateway): report skipped media attachments instead of silently dropping them - #32054
fix(gateway): report skipped media attachments instead of silently dropping them#32054jrockwar wants to merge 1 commit into
Conversation
…opping them Media attachments outside allowed roots were silently stripped with only a generic WARNING naming no specific path. Users had no way of knowing why their images/files never arrived. Changes: - gateway/platforms/base.py: Add partition_media_delivery_paths() and partition_local_delivery_paths() returning (safe, skipped) tuples. Keep filter_* as backward-compatible wrappers that already log each skipped path by name (no change needed at other call sites). - tools/send_message_tool.py: Use partition method, surface skipped paths in JSON result under 'skipped_attachments' + 'warnings'. - tests/gateway/test_platform_base.py: Regression tests. This is the minimal change: all other call sites (gateway/run.py, cron/scheduler.py, yuanbao_tools.py, weixin.py) already get correct per-path logging via the existing filter_* wrappers.
23a670c to
3124353
Compare
|
Nice refactor — splitting Scope clarification worth noting in the PR (since the title says 'report skipped … instead of silently dropping them' broadly): this fixes the |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the real text-only success case. Current main still filters rejected MEDIA paths before send_message returns the platform result (tools/send_message_tool.py:438-440), while the filter only logs rejection (gateway/platforms/base.py:3497-3507).
Problems
gateway/platforms/base.py:2250in this diff moves skipped-path collection intopartition_*, but logging remains only in thefilter_*wrappers. Sincesend_message_tool.pynow callspartition_*directly, that path loses diagnostic logging. Preserve current main's sanitized_log_safe_path()logging in the partition helper.- The new tests do not assert the advertised
skipped_attachments/warningsresult. Add coverage to the existing strict rejection scenario intests/tools/test_send_message_tool.py:534.
Suggested changes
- Rebase/salvage against current media validation and retain the sanitizer introduced by
e28a668b4. - Assert the success result includes skipped-media feedback, and define whether error results should remain unmodified.
Automated hermes-sweeper review.
| for media_path, is_voice in media_files or []: | ||
| safe_path = validate_media_delivery_path(str(media_path)) | ||
| if safe_path: | ||
| safe_media.append((safe_path, bool(is_voice))) |
There was a problem hiding this comment.
send_message_tool now calls this partition helper directly, so keeping logging only in filter_* means rejected paths on this newly changed path are no longer logged. Log the sanitized rejected path here, then let the backward-compatible filter wrapper delegate without logging again.
| @@ -314,6 +314,15 @@ async def _open_slack_dm(token, user_id): | |||
| if used_home_channel and isinstance(result, dict) and result.get("success"): | |||
There was a problem hiding this comment.
Please add an end-to-end tool test for this result mutation. The existing strict rejection test proves the attachment is filtered but currently asserts only success; it should assert this warning and skipped_attachments field on a text-only successful send.
Bug Report
Discord (and all platform) media attachments are silently dropped when outside
MEDIA_DELIVERY_SAFE_ROOTS.Symptom
When the model emits
MEDIA:<path>inline directives, image/file attachments never appear in Discord (or other platforms). Text-only messages arrive fine. No error is surfaced to the user.Root Cause
BasePlatformAdapter.filter_media_delivery_paths()validates every path against allowed roots. When a file lives outside the whitelist, the gateway logs a genericWARNINGwith no path name, and silently drops the attachment with zero user-visible feedback.Current Allowed Roots
~/.hermes/cache/{images,audio,video,documents,screenshots}~/.hermes/{image_cache,audio_cache,video_cache,document_cache,browser_screenshots}HERMES_MEDIA_ALLOW_DIRSenvironment variableImpact
Any media stored outside these directories is silently stripped before reaching any platform.
Scope of this PR
This PR targets the send_message tool path specifically (the path where an LLM explicitly calls
send_messagewith aMEDIA:tag).The gateway auto-delivery path (
gateway/platforms/base.py:3508/3522) is left unchanged — it still callsfilter_*and only gets log-line visibility of skipped paths. Fixing that path properly would require injecting skipped-attachment warnings into the response text itself (so the user sees them), which is a distinct design decision from surfacing skips in a tool JSON result.If desired, a follow-up PR can extend the same
partition_*+ user-visible warning pattern to the gateway auto-delivery path.Changes
gateway/platforms/base.py: Addpartition_media_delivery_paths()andpartition_local_delivery_paths()returning(safe, skipped)tuples. Keepfilter_*as backward-compatible wrappers (they already log each skipped path by name, so no changes needed at other call sites).tools/send_message_tool.py: Usepartition_media_delivery_paths(), surface skipped paths in JSON result underskipped_attachmentsandwarnings, so the LLM can see why files did not arrive.tests/gateway/test_platform_base.py: Regression tests for partition methods plus backward-compatibility check onfilter_media_delivery_paths().Why so few files?
All other call sites (
gateway/run.py,cron/scheduler.py,tools/yuanbao_tools.py,gateway/platforms/weixin.py) already get correct per-path logging via the existingfilter_*wrappers. The only place where skipped attachments were truly invisible wassend_message_tool.py, where the JSON result went back to the LLM without any mention of what was dropped.Testing
New regression tests:
test_partition_media_returns_safe_and_skippedtest_partition_local_returns_safe_and_skippedtest_filter_backward_compatibleFixes the silent-drop behavior on the send_message tool path.