mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-23 16:36:23 +00:00
fix(gateway): make post-stream media delivery explicit-only
The post-stream helper (_deliver_media_from_response) rescanned the already-streamed response and promoted bare local filesystem paths into real uploads via extract_local_files. Since the visible reply was already streamed verbatim, any bare path there is either text the user has seen or stale inspected/tool content — not an attachment request. On Slack this uploaded images from stale inspected content after otherwise clean replies. Post-stream delivery now honors only explicit MEDIA: directives. The non-streaming path in gateway/platforms/base.py keeps its bare-path auto-detect, because that path controls the visible text and strips the path from the reply when it attaches — auto-attach is intentional there. Regression tests: bare image/document paths in a streamed reply produce no upload; explicit MEDIA: tags still deliver. Fixes #20834
This commit is contained in:
parent
e1883df75e
commit
1cdb46238c
2 changed files with 163 additions and 37 deletions
|
|
@ -14867,11 +14867,21 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
event: MessageEvent,
|
||||
adapter,
|
||||
) -> None:
|
||||
"""Extract MEDIA: tags and local file paths from a response and deliver them.
|
||||
"""Extract explicit MEDIA: tags from a response and deliver them.
|
||||
|
||||
Called after streaming has already sent the text to the user, so the
|
||||
text itself is already delivered — this only handles file attachments
|
||||
that the normal _process_message_background path would have caught.
|
||||
|
||||
Unlike the non-streaming path in ``gateway/platforms/base.py`` (which
|
||||
also auto-detects bare local paths via ``extract_local_files``), this
|
||||
post-stream rescan is EXPLICIT-ONLY. The visible reply has already
|
||||
been streamed verbatim, so a bare path string here was either (a)
|
||||
already shown to the user as text, or (b) stale tool/inspected
|
||||
content that was never part of the intended visible reply. Promoting
|
||||
such paths into uploads after the fact sent files the model never
|
||||
asked to deliver (#20834). Only ``MEDIA:`` directives — the explicit
|
||||
attachment contract — trigger post-stream uploads.
|
||||
"""
|
||||
from pathlib import Path
|
||||
from urllib.parse import quote as _quote
|
||||
|
|
@ -14887,16 +14897,12 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
|
||||
media_files, cleaned = adapter.extract_media(response)
|
||||
media_files = BasePlatformAdapter.filter_media_delivery_paths(media_files)
|
||||
# Chain the cleaned text through each extractor (extract_media →
|
||||
# extract_images → extract_local_files) so MEDIA: tags and image URLs
|
||||
# are removed before the bare-path auto-detect runs. Previously the
|
||||
# cleaned text from extract_media was dropped (``_``) and
|
||||
# extract_local_files scanned text that still contained MEDIA: tags,
|
||||
# producing false-positive bare-path matches with the MEDIA: prefix
|
||||
# glued on. This matches the chain order in gateway/platforms/base.py.
|
||||
_, cleaned = adapter.extract_images(cleaned)
|
||||
local_files, _ = adapter.extract_local_files(cleaned)
|
||||
local_files = BasePlatformAdapter.filter_local_delivery_paths(local_files)
|
||||
# Strip image URLs from the cleaned text for parity with the
|
||||
# non-streaming chain, but do NOT run extract_local_files here:
|
||||
# post-stream delivery is explicit-only (#20834). Bare local paths
|
||||
# in an already-streamed reply are text the user has seen (or
|
||||
# stale inspected content), not an attachment request.
|
||||
adapter.extract_images(cleaned)
|
||||
|
||||
_thread_meta = self._thread_metadata_for_source(event.source, self._reply_anchor_for_event(event))
|
||||
|
||||
|
|
@ -14918,14 +14924,6 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
else:
|
||||
non_image_media.append((media_path, is_voice))
|
||||
|
||||
non_image_local: list = []
|
||||
for file_path in local_files:
|
||||
if (Path(file_path).suffix.lower() in _IMAGE_EXTS
|
||||
and not force_document_attachments):
|
||||
image_paths.append(file_path)
|
||||
else:
|
||||
non_image_local.append(file_path)
|
||||
|
||||
if image_paths:
|
||||
try:
|
||||
images = [(f"file://{_quote(p)}", "") for p in image_paths]
|
||||
|
|
@ -14961,24 +14959,6 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
except Exception as e:
|
||||
logger.warning("[%s] Post-stream media delivery failed: %s", adapter.name, e)
|
||||
|
||||
for file_path in non_image_local:
|
||||
try:
|
||||
ext = Path(file_path).suffix.lower()
|
||||
if ext in _VIDEO_EXTS:
|
||||
await adapter.send_video(
|
||||
chat_id=event.source.chat_id,
|
||||
video_path=file_path,
|
||||
metadata=_thread_meta,
|
||||
)
|
||||
else:
|
||||
await adapter.send_document(
|
||||
chat_id=event.source.chat_id,
|
||||
file_path=file_path,
|
||||
metadata=_thread_meta,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning("[%s] Post-stream file delivery failed: %s", adapter.name, e)
|
||||
|
||||
except Exception as e:
|
||||
logger.warning("Post-stream media extraction failed: %s", e)
|
||||
|
||||
|
|
|
|||
146
tests/gateway/test_post_stream_media_delivery.py
Normal file
146
tests/gateway/test_post_stream_media_delivery.py
Normal file
|
|
@ -0,0 +1,146 @@
|
|||
"""Post-stream media delivery is explicit-only (#20834).
|
||||
|
||||
``GatewayRunner._deliver_media_from_response`` runs AFTER streaming has sent
|
||||
the visible reply. At that point a bare local filesystem path in the response
|
||||
text is either text the user already saw, or stale inspected/tool content —
|
||||
it is NOT an attachment request. Only explicit ``MEDIA:`` directives may
|
||||
trigger post-stream uploads.
|
||||
|
||||
The non-streaming path (``gateway/platforms/base.py``) keeps its bare-path
|
||||
auto-detect (``extract_local_files``) — that path controls what text is sent
|
||||
and can strip the path from the visible reply, so auto-attach is intentional
|
||||
there. This file pins the asymmetry.
|
||||
"""
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.config import Platform
|
||||
from gateway.platforms.base import BasePlatformAdapter, MessageEvent, MessageType, SendResult
|
||||
from gateway.run import GatewayRunner
|
||||
from gateway.session import SessionSource
|
||||
|
||||
|
||||
def _event():
|
||||
source = SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
chat_id="C123CHAN",
|
||||
chat_type="group",
|
||||
thread_id=None,
|
||||
)
|
||||
return MessageEvent(
|
||||
text="hi",
|
||||
message_type=MessageType.TEXT,
|
||||
source=source,
|
||||
message_id="171.000001",
|
||||
)
|
||||
|
||||
|
||||
def _fake_runner(thread_meta):
|
||||
return SimpleNamespace(
|
||||
_thread_metadata_for_source=lambda source, anchor=None: thread_meta,
|
||||
_reply_anchor_for_event=lambda event: None,
|
||||
)
|
||||
|
||||
|
||||
def _adapter():
|
||||
return SimpleNamespace(
|
||||
name="test",
|
||||
extract_media=BasePlatformAdapter.extract_media,
|
||||
extract_images=BasePlatformAdapter.extract_images,
|
||||
extract_local_files=BasePlatformAdapter.extract_local_files,
|
||||
send_voice=AsyncMock(return_value=SendResult(success=True, message_id="voice")),
|
||||
send_document=AsyncMock(return_value=SendResult(success=True, message_id="doc")),
|
||||
send_image_file=AsyncMock(return_value=SendResult(success=True, message_id="image")),
|
||||
send_video=AsyncMock(return_value=SendResult(success=True, message_id="video")),
|
||||
send_multiple_images=AsyncMock(return_value=SendResult(success=True, message_id="imgs")),
|
||||
)
|
||||
|
||||
|
||||
def _allowed_media_path(tmp_path, monkeypatch, name):
|
||||
root = tmp_path / "media-cache"
|
||||
media_file = root / name
|
||||
media_file.parent.mkdir(parents=True, exist_ok=True)
|
||||
media_file.write_bytes(b"media")
|
||||
monkeypatch.setattr(
|
||||
"gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS",
|
||||
(root,),
|
||||
)
|
||||
return media_file.resolve()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bare_local_path_in_streamed_reply_is_not_uploaded(tmp_path, monkeypatch):
|
||||
"""The #20834 shape: visible reply contains a bare path (from inspected
|
||||
content), no MEDIA: directive — nothing may be uploaded post-stream."""
|
||||
media_file = _allowed_media_path(tmp_path, monkeypatch, "mockup.png")
|
||||
adapter = _adapter()
|
||||
|
||||
await GatewayRunner._deliver_media_from_response(
|
||||
_fake_runner({}),
|
||||
f"The design lives at {media_file} if you want to look later.",
|
||||
_event(),
|
||||
adapter,
|
||||
)
|
||||
|
||||
adapter.send_multiple_images.assert_not_awaited()
|
||||
adapter.send_image_file.assert_not_awaited()
|
||||
adapter.send_document.assert_not_awaited()
|
||||
adapter.send_video.assert_not_awaited()
|
||||
adapter.send_voice.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_bare_document_path_in_streamed_reply_is_not_uploaded(tmp_path, monkeypatch):
|
||||
media_file = _allowed_media_path(tmp_path, monkeypatch, "report.pdf")
|
||||
adapter = _adapter()
|
||||
|
||||
await GatewayRunner._deliver_media_from_response(
|
||||
_fake_runner({}),
|
||||
f"I saved it to {media_file}.",
|
||||
_event(),
|
||||
adapter,
|
||||
)
|
||||
|
||||
adapter.send_document.assert_not_awaited()
|
||||
adapter.send_video.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_explicit_media_tag_still_delivers_post_stream(tmp_path, monkeypatch):
|
||||
"""Explicit MEDIA: directives keep working after the #20834 fix."""
|
||||
media_file = _allowed_media_path(tmp_path, monkeypatch, "chart.png")
|
||||
adapter = _adapter()
|
||||
|
||||
await GatewayRunner._deliver_media_from_response(
|
||||
_fake_runner({}),
|
||||
f"Here is the chart.\nMEDIA:{media_file}",
|
||||
_event(),
|
||||
adapter,
|
||||
)
|
||||
|
||||
adapter.send_multiple_images.assert_awaited_once()
|
||||
images_kwargs = adapter.send_multiple_images.await_args.kwargs
|
||||
assert images_kwargs["chat_id"] == "C123CHAN"
|
||||
assert str(media_file) in images_kwargs["images"][0][0]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_explicit_media_document_still_delivers_post_stream(tmp_path, monkeypatch):
|
||||
media_file = _allowed_media_path(tmp_path, monkeypatch, "report.pdf")
|
||||
adapter = _adapter()
|
||||
|
||||
await GatewayRunner._deliver_media_from_response(
|
||||
_fake_runner({}),
|
||||
f"Report attached.\nMEDIA:{media_file}",
|
||||
_event(),
|
||||
adapter,
|
||||
)
|
||||
|
||||
adapter.send_document.assert_awaited_once_with(
|
||||
chat_id="C123CHAN",
|
||||
file_path=str(media_file),
|
||||
metadata={},
|
||||
)
|
||||
Loading…
Add table
Add a link
Reference in a new issue