fix(gateway): surface rejected media paths instead of dropping silently - #31864
Open
Masterlincs wants to merge 1 commit into
Open
Masterlincs wants to merge 1 commit into
Masterlincs wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes media attachment filtering non-silent by returning rejected file basenames from allowlist filtering and surfacing a user-visible rejection notice across multiple send paths.
Changes:
- Update
BasePlatformAdapter.filter_*_delivery_pathsto return(accepted, rejected_basenames)and addformat_media_rejection_notice(). - Append (or send) rejection notices in multiple message delivery flows (tools, gateway, cron, platform adapter).
- Add/extend tests to validate acceptance/rejection behavior and notice composition.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/yuanbao_tools.py | Appends rejection notice to DM text when unsafe MEDIA paths are dropped. |
| tools/send_message_tool.py | Appends rejection notice to outgoing text when unsafe MEDIA paths are dropped. |
| gateway/platforms/base.py | Changes filter APIs to return rejected basenames; adds formatting helper; appends notice in streaming flow. |
| gateway/run.py | Sends post-stream / notifier rejection notices; appends notice to background task messages. |
| gateway/platforms/weixin.py | Appends rejection notice to outgoing content when media/local paths are dropped. |
| cron/scheduler.py | Defensively surfaces rejection notice when media filtering rejects paths. |
| tests/gateway/test_platform_base.py | Expands tests for rejected basenames and notice formatting/composition. |
| tests/tools/test_send_message_tool.py | Updates test to assert user-visible rejection notice is appended. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5154
to
+5160
| candidates, rejected = BasePlatformAdapter.filter_local_delivery_paths(candidates) | ||
| if rejected: | ||
| notice = BasePlatformAdapter.format_media_rejection_notice(rejected) | ||
| try: | ||
| await adapter.send(chat_id=chat_id, content=notice.lstrip(), metadata=metadata) | ||
| except Exception as exc: | ||
| logger.warning("kanban notifier: failed to send rejection notice: %s", exc) |
Comment on lines
+2238
to
+2240
| def filter_media_delivery_paths( | ||
| media_files, | ||
| ) -> Tuple[List[Tuple[str, bool]], List[str]]: |
|
|
||
| @staticmethod | ||
| def filter_local_delivery_paths(file_paths) -> List[str]: | ||
| """Drop unsafe bare local file paths and normalize accepted paths.""" | ||
| def filter_local_delivery_paths(file_paths) -> Tuple[List[str], List[str]]: |
| if safe_path: | ||
| safe_media.append((safe_path, bool(is_voice))) | ||
| else: | ||
| logger.warning("Skipping unsafe MEDIA directive path outside allowed roots") | ||
| return safe_media | ||
| rejected.append(os.path.basename(raw.strip().strip("`\"'")) or raw) |
| if safe_path: | ||
| safe_paths.append(safe_path) | ||
| else: | ||
| logger.warning("Skipping unsafe local file path outside allowed roots") | ||
| return safe_paths | ||
| rejected.append(os.path.basename(raw.strip().strip("`\"'")) or raw) |
Comment on lines
+2289
to
+2290
| names = ", ".join(rejected_basenames) | ||
| return f"\n\n⚠️ Couldn't attach: {names} (path outside allowed roots)" |
alt-glitch
added
type/bug
18 tasks
When a model emits MEDIA:/path or a bare local path that falls outside the allowed roots (image_cache, audio_cache, document_cache, etc., or operator-configured HERMES_MEDIA_ALLOW_DIRS), filter_media_delivery_paths and filter_local_delivery_paths previously logged a WARNING and returned only the surviving paths — the rejection vanished. The user saw text without the promised attachment and nothing in the response indicated why; the only trace was a gateway log line. This change: - Changes both filter functions to return (accepted, rejected_basenames) so callers can react. Basenames (not full paths) are surfaced to avoid leaking filesystem layout into chat output. - Adds BasePlatformAdapter.format_media_rejection_notice() so the user- visible string lives in one place. - Updates every call site (13 total across base.py, weixin.py, run.py, scheduler.py, send_message_tool.py, yuanbao_tools.py) to unpack the new tuple and either append the notice to outgoing text or send it as a follow-up message when text has already streamed. Breaking change to the filter_media_delivery_paths / filter_local_delivery_paths signatures. Both are part of BasePlatformAdapter's static-method surface; third-party platform plugins that call them will need a one-line unpack update. Happy to split into deprecated wrappers + new variants if maintainers prefer. Tests: - Updates the existing filter test to assert the new tuple shape. - Adds tests for rejected basename extraction (incl. quoted paths), empty input handling, the notice helper, and an end-to-end composition test (extract -> filter -> notice).
Masterlincs
force-pushed
the
fix/media-surface-rejections
branch
from
May 28, 2026 18:26
9c7e336 to
780ff9e
Compare
19 tasks
teknium1
reviewed
Jul 13, 2026
teknium1
left a comment
teknium1
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for addressing a real user-facing failure mode. The silent-drop premise still exists on current main: gateway/platforms/base.py:4926-4928 filters rejected paths, then :5030-5034 sends the remaining text without a notice.
Problems
gateway/run.py:5267in this PR addsawait adapter.send(...)inside synchronous_add(...); that is invalid Python syntax.- The tuple return change is stale against current main.
gateway/kanban_watchers.py:702is an unmodified current caller that still requires a list, andtests/gateway/test_tool_response_drop_recovery.py:209stubs the existing list-returning API. gateway/platforms/base.py:2373derives chat-visible names from raw model output without bounded control-character sanitization. Current main sanitizes rejected paths for logs atgateway/platforms/base.py:3506.
Suggested changes
- Preserve
filter_*list-return compatibility and add explicit partition helpers for callers that need rejection details. - Rebase the audit on current call sites, including
gateway/kanban_watchers.py:702, and repair the synchronous Kanban send path. - Sanitize, deduplicate, and cap user-visible rejected filenames before sending notices.
Automated hermes-sweeper review.
| try: | ||
| await adapter.send(chat_id=chat_id, content=notice.lstrip(), metadata=metadata) | ||
| except Exception as exc: | ||
| logger.warning("kanban notifier: failed to send rejection notice: %s", exc) |
Collaborator
There was a problem hiding this comment.
_add(...) is declared with def, so this await makes the module fail to parse. Schedule the coroutine or move this send into an async caller; the current-main Kanban artifact flow has also moved to gateway/kanban_watchers.py.
| """Drop unsafe MEDIA paths and normalize accepted paths.""" | ||
| def filter_media_delivery_paths( | ||
| media_files, | ||
| ) -> Tuple[List[Tuple[str, bool]], List[str]]: |
Collaborator
There was a problem hiding this comment.
Changing this established list-returning helper to a tuple breaks current callers that are not updated by this PR, including the current-main Kanban watcher and tests that stub the helper. Please retain this API and add a separate partition/helper method for accepted plus rejected paths.
| else: | ||
| logger.warning("Skipping unsafe MEDIA directive path outside allowed roots") | ||
| return safe_media | ||
| rejected.append(os.path.basename(raw.strip().strip("`\"'")) or raw) |
Collaborator
There was a problem hiding this comment.
This basename is derived from model output and will be sent to users. Normalize control characters, deduplicate, and cap per-name and total notice length before composing chat-visible text; current main already sanitizes the analogous log value.
teknium1
added
sweeper:risk-message-delivery
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When a model emits
MEDIA:/pathor a bare local path that falls outside the allowed roots (image_cache,audio_cache,document_cache, etc., or operator-configuredHERMES_MEDIA_ALLOW_DIRS),filter_media_delivery_pathsandfilter_local_delivery_pathspreviously logged aWARNINGand returned only the surviving paths — the rejection vanished. The user saw text without the promised attachment and nothing in the response indicated why; the only trace was a gateway log line.This change:
(accepted, rejected_basenames)so callers can react. Basenames (not full paths) are surfaced to avoid leaking filesystem layout into chat output.BasePlatformAdapter.format_media_rejection_notice()so the user-visible string lives in one place.base.py,weixin.py,run.py,scheduler.py,send_message_tool.py,yuanbao_tools.py) to unpack the new tuple and either append the notice to outgoing text or send it as a follow-up message when text has already streamed.Breaking change to the
filter_media_delivery_paths/filter_local_delivery_pathssignatures. Both are part ofBasePlatformAdapter's static-method surface; third-party platform plugins that call them will need a one-line unpack update. Happy to split into deprecated wrappers + new variants if maintainers prefer.Related Issue
No related issue — surfaced while testing media delivery on a personal Discord deployment.
Fixes #
Type of Change
Changes Made
gateway/platforms/base.pyfilter_media_delivery_pathsreturns(accepted, rejected_basenames)filter_local_delivery_pathsreturns(accepted, rejected_basenames)format_media_rejection_notice(rejected_basenames) -> strhelper_process_message_backgroundunpacks new tuples and appends notice totext_contentgateway/platforms/weixin.py— same pattern; appends notice tofinal_contentgateway/run.py:5154) — sends notice as standalone message (no surrounding text)_deliver_media_from_response(:11355/11358) — sends notice as follow-up (text already streamed):11669) — appends notice to header+text payloadcron/scheduler.py_send_media_via_adapter— defensive notice send (normally a no-op;_deliver_resultalready filters)_deliver_result— appends notice tocleaned_delivery_contenttools/send_message_tool.py— appends notice tocleaned_messagetools/yuanbao_tools.py— appends notice tomessagetests/gateway/test_platform_base.pytest_filter_keeps_safe_media_and_drops_unsafeto assert new tuple shapetest_filter_media_rejected_basename_strips_quotestest_filter_local_returns_rejected_basenamestest_filter_empty_input_returns_empty_pairtest_rejection_notice_empty_when_nothing_rejectedtest_rejection_notice_lists_basenamestest_extract_filter_notice_composition(end-to-end)tests/tools/test_send_message_tool.pytest_media_tag_outside_allowed_roots_is_not_sentto assert the new visible-notice behavior (was asserting the silent-failure behavior this PR fixes)How to Test
scripts/run_tests.sh tests/gateway/test_platform_base.py tests/tools/test_send_message_tool.py— all relevant tests pass (350/350 across the changed paths and their adjacentsuites).
file to
/tmp/test.txtand upload it here." You'll see the bot's reply text but no attachment, andgrep "Skipping unsafe" ~/.hermes/logs/gateway.logis the only trace.⚠️ Couldn't attach: test.txt (path outside allowed roots)in the reply.~/.hermes/document_cache/instead and ask the bot to deliver it. Should still arrive as an attachment, no notice (existingbehavior).
Checklist
Code
fix(gateway): …)pytest tests/ -qand all tests pass — all tests related to the changed code paths pass (350/350 acrosstests/gateway/,tests/tools/test_send_message_tool.py,tests/hermes_cli/test_kanban_notify.py,tests/test_yuanbao_pipeline.py). The fullscripts/run_tests.sh tests/run surfaces ~45 pre-existing failures in unrelated areas (process management, MCPstability, voice/TTS, plugin scanner) that reproduce on clean
upstream/mainwithout these changes applied; none touch media filtering.Documentation & Housekeeping
docs/, docstrings) — docstrings on both filter functions updated to describe the new return shape and the rationalecli-config.yaml.exampleif I added/changed config keys — N/A (no config keys added)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Ascripts/check-windows-footguns.py --diff upstream/mainreturned clean; change uses onlyos.path.basename+ string ops, no platform-specific syscallssend_messageandyuanbao_toolsinternal handlers updated but visible tool schemas unchanged)Screenshots / Logs
Before the fix (gateway log shows the silent rejection):
2026-05-23 22:41:54,978 WARNING gateway.platforms.base: Skipping unsafe local file path outside allowed roots
2026-05-23 22:41:54,979 INFO gateway.platforms.base: [Discord] Sending response (737 chars) to
…and the user's chat shows the bot's text reply with no attachment and no explanation.
After the fix (verified live on Discord):
…appended to the same reply, telling the user exactly what failed and which file.