Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions gateway/platforms/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -1169,24 +1169,40 @@ def _log_safe_path(path: str) -> str:
# Audio (delivered as voice/audio where supported)
".mp3", ".wav", ".ogg", ".opus", ".m4a", ".flac",
# Documents (uploaded as file attachments)
".pdf", ".docx", ".doc", ".odt", ".rtf", ".txt", ".md", ".epub",
".pdf", ".docx", ".doc", ".odt", ".rtf", ".txt", ".md", ".markdown", ".epub",
# Spreadsheets / data
".xlsx", ".xls", ".ods", ".csv", ".tsv", ".json", ".xml", ".yaml", ".yml",
".xlsx", ".xls", ".ods", ".csv", ".tsv", ".json", ".xml", ".yaml", ".yml", ".toml",
# Presentations
".pptx", ".ppt", ".odp", ".key",
# Archives
".zip", ".tar", ".gz", ".tgz", ".bz2", ".xz", ".7z", ".rar", ".apk", ".ipa",
# Web / rendered output
".html", ".htm",
)
# NOTE: source-file extensions (.py / .js / .sh) are intentionally NOT in this
# tuple. The reporter of #37318 asked for them, but the bare-path detector in
# ``extract_local_files`` shares this same tuple, and auto-shipping arbitrary
# source files based on a stray ``/tmp/script.py`` mention in agent output
# would be a surprise (see ``test_no_media_extensions`` for the contract).
# Users who want source files delivered can still use the broader file_tool /
# send_message MEDIA: flow with a glob, but the auto-detector stays narrow.

# Regex alternation fragment of bare extensions (no leading dot), e.g.
# ``png|jpe?g|...``. ``jpe?g`` collapses jpg/jpeg into one branch. Sorted
# longest-first so the alternation never matches a shorter ext as a prefix of
# a longer one (e.g. ``.tar`` before ``.tar.gz`` components).
_MEDIA_EXT_ALTERNATION = "|".join(
#
# Exported (without the leading underscore) so other dispatch sites — notably
# ``gateway/run.py``'s ``_TOOL_MEDIA_RE`` for auto-append + history dedup —
# can build their regex off the same source-of-truth alternation. Keeping the
# alias means historical callers that imported ``_MEDIA_EXT_ALTERNATION``
# continue to work. Issue #37318 was a direct consequence of run.py hand-
# rolling its own extension whitelist that quietly drifted behind this one
# (``.md``, ``.json``, ``.yaml``, ``.html``, ``.svg`` etc. silently dropped).
MEDIA_EXT_ALTERNATION = "|".join(
sorted((e.lstrip(".") for e in MEDIA_DELIVERY_EXTS), key=len, reverse=True)
)
_MEDIA_EXT_ALTERNATION = MEDIA_EXT_ALTERNATION # backwards-compat alias

# Anchored ``MEDIA:<path>`` cleanup pattern. Unlike the old loose
# ``MEDIA:\\s*\\S+``, this only strips a tag whose path ends in a known
Expand Down
29 changes: 18 additions & 11 deletions gateway/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -680,11 +680,21 @@ def _last_transcript_timestamp(history: Optional[List[Dict[str, Any]]]) -> Any:
# pattern so a bare ``MEDIA:`` token in prose (no deliverable extension) is never
# auto-appended. Kept local to the auto-append path; the producer-tool allowlist
# below is the primary guard, this is the secondary precision guard.
#
# The extension alternation is sourced from ``MEDIA_EXT_ALTERNATION`` in
# ``gateway/platforms/base.py`` — the same source of truth that drives
# ``MEDIA_TAG_CLEANUP_RE`` and ``extract_media``. Issue #37318 was a
# silent-drop bug caused by this regex hand-rolling its own narrower
# whitelist (``.md`` / ``.json`` / ``.yaml`` / ``.html`` / ``.svg`` were
# missing); building from the shared alternation prevents that class of
# drift entirely. Imported here (rather than via the consolidated import
# block ~450 lines below) because ``_TOOL_MEDIA_RE`` is built at module
# import time and needs the alternation before any from-block runs.
from gateway.platforms.base import (
MEDIA_EXT_ALTERNATION as _MEDIA_EXT_ALTERNATION,
)
_TOOL_MEDIA_RE = re.compile(
r'MEDIA:((?:[A-Za-z]:[/\\]|/|~\/)\S+\.(?:png|jpe?g|gif|webp|'
r'mp4|mov|avi|mkv|webm|ogg|opus|mp3|wav|m4a|'
r'flac|epub|pdf|zip|rar|7z|docx?|xlsx?|pptx?|'
r'txt|csv|apk|ipa))',
r'MEDIA:((?:[A-Za-z]:[/\\]|/|~\/)\S+\.(?:' + _MEDIA_EXT_ALTERNATION + r'))',
re.IGNORECASE,
)

Expand Down Expand Up @@ -17763,13 +17773,10 @@ def _clarify_callback_sync(question: str, choices) -> str:
if _hm.get("role") in {"tool", "function"}:
_hc = _hm.get("content", "")
if "MEDIA:" in _hc:
_TOOL_MEDIA_RE = re.compile(
r'MEDIA:((?:[A-Za-z]:[/\\]|/|~\/)\S+\.(?:png|jpe?g|gif|webp|'
r'mp4|mov|avi|mkv|webm|ogg|opus|mp3|wav|m4a|'
r'flac|epub|pdf|zip|rar|7z|docx?|xlsx?|pptx?|'
r'txt|csv|apk|ipa))',
re.IGNORECASE
)
# Reuse the module-level pattern — see #37318 for why
# this used to be a local copy and why it must not be
# one again (the local re-declaration silently drifted
# behind the shared extension list).
for _match in _TOOL_MEDIA_RE.finditer(_hc):
_p = _match.group(1).strip().rstrip('",}')
if _p:
Expand Down
142 changes: 142 additions & 0 deletions tests/gateway/test_run_tool_media_ext_parity.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
"""Tests for issue #37318: _TOOL_MEDIA_RE in gateway/run.py must accept the
same set of file extensions as MEDIA_DELIVERY_EXTS / MEDIA_TAG_CLEANUP_RE in
gateway/platforms/base.py.

The bug: the agent emits ``MEDIA:/tmp/report.md`` (or any of the extensions
in MEDIA_DELIVERY_EXTS that weren't in the hardcoded run.py whitelist —
``.md``, ``.json``, ``.yaml``, ``.html``, ``.svg``, etc.). The dispatch-site
regex in ``base.py`` correctly recognises these because it is built from the
shared ``MEDIA_DELIVERY_EXTS`` tuple. The narrower regex baked into ``run.py``
silently drops them, so:

* ``_collect_auto_append_media_tags`` (run.py L692) skips them — they are
never appended to outgoing messages.
* The history-dedup scan (run.py L17765) doesn't see them — stale tags can
leak through.

Fix: build the run.py regex from the same ``MEDIA_DELIVERY_EXTS`` alternation
exposed by ``gateway.platforms.base``, so the two patterns stay in lock-step
by construction.
"""

import re

import pytest

from gateway.platforms.base import (
MEDIA_DELIVERY_EXTS,
MEDIA_EXT_ALTERNATION,
MEDIA_TAG_CLEANUP_RE,
)
from gateway.run import _TOOL_MEDIA_RE


# Extensions that issue #37318 specifically calls out as silently dropped.
# Source-file extensions (.py / .js / .sh) from the issue are deliberately
# NOT added to MEDIA_DELIVERY_EXTS — see the NOTE in gateway/platforms/base.py.
# Auto-shipping source files based on a bare-path mention would surprise users.
MISSING_EXTS_FROM_ISSUE = [
"md", "markdown", "json", "yaml", "yml", "toml",
# Plus extensions that ALREADY lived in MEDIA_DELIVERY_EXTS but were
# missing from the run.py whitelist (silent drift, the real bug):
"html", "htm", "svg", "bmp", "tiff", "tsv", "xml", "odt", "ods", "odp",
"key", "tar", "bz2", "xz",
]


class TestRunPyMediaExtParity:
"""run.py's _TOOL_MEDIA_RE must accept every ext in MEDIA_DELIVERY_EXTS."""

@pytest.mark.parametrize("ext", [e.lstrip(".") for e in MEDIA_DELIVERY_EXTS])
def test_every_delivery_ext_matched_by_run_pattern(self, ext):
"""Every extension MEDIA_DELIVERY_EXTS advertises must match.

Without this, ``base.py.extract_media`` happily strips and ships the
tag while ``run.py._collect_auto_append_media_tags`` silently
discards it from the auto-append path — the exact contract drift
that produced #37318.
"""
tag = f"MEDIA:/tmp/example.{ext}"
assert _TOOL_MEDIA_RE.search(tag) is not None, (
f"run.py _TOOL_MEDIA_RE rejected MEDIA:/tmp/example.{ext} "
f"even though .{ext} is in MEDIA_DELIVERY_EXTS"
)

@pytest.mark.parametrize("ext", MISSING_EXTS_FROM_ISSUE)
def test_issue_37318_named_extensions_match(self, ext):
"""The specific extensions called out in issue #37318 now match."""
tag = f"MEDIA:/path/to/file.{ext}"
match = _TOOL_MEDIA_RE.search(tag)
assert match is not None, f"Issue #37318 ext .{ext} should match: {tag}"
assert match.group(1) == f"/path/to/file.{ext}"

def test_run_pattern_uses_shared_alternation(self):
"""run.py must source its alternation from base.py to prevent drift.

We don't dictate HOW the run.py regex is constructed — only that it
accepts every extension in MEDIA_EXT_ALTERNATION. This guard ensures
a future contributor who adds a new extension to MEDIA_DELIVERY_EXTS
doesn't have to remember to also hand-edit the run.py whitelist.
"""
# Pick an extension out of the shared alternation and try it.
for raw in MEDIA_EXT_ALTERNATION.split("|"):
# Strip optional groups like (?:...) — bare ext only
ext = raw.replace("\\.", ".")
if "?" in ext or "(" in ext or ")" in ext:
# e.g. 'jpe?g', 'docx?'. Pick a concrete form.
if ext == "jpe?g":
ext = "jpg"
elif ext == "docx?":
ext = "doc"
elif ext == "xlsx?":
ext = "xls"
elif ext == "pptx?":
ext = "ppt"
else:
continue # skip exotic patterns
tag = f"MEDIA:/tmp/x.{ext}"
assert _TOOL_MEDIA_RE.search(tag), (
f"run.py _TOOL_MEDIA_RE missed .{ext} from shared "
f"MEDIA_EXT_ALTERNATION — patterns have drifted"
)

# ── Regression: pre-existing behaviour preserved ───────────────

@pytest.mark.parametrize("tag,expected", [
("MEDIA:/tmp/output.png", "/tmp/output.png"),
("MEDIA:/var/log/r.pdf", "/var/log/r.pdf"),
("MEDIA:~/Downloads/a.jpg", "~/Downloads/a.jpg"),
("MEDIA:C:\\Users\\t\\image.png", "C:\\Users\\t\\image.png"),
("MEDIA:D:/data/report.pdf", "D:/data/report.pdf"),
])
def test_existing_paths_still_match(self, tag, expected):
"""Unix, home-relative, and Windows paths from existing tests still match."""
m = _TOOL_MEDIA_RE.search(tag)
assert m is not None, f"regression: {tag} no longer matches"
assert m.group(1) == expected

@pytest.mark.parametrize("text", [
"No MEDIA tag here",
"MEDIA:relative/path/file.png", # no anchor
"MEDIA:file.md", # no directory
"MEDIA:/path/to/file.unknown", # unsupported ext
"MEDIA:/path/to/file", # no extension
])
def test_invalid_inputs_still_rejected(self, text):
"""The fix must not loosen anchoring or accept arbitrary extensions."""
assert _TOOL_MEDIA_RE.search(text) is None, (
f"should still reject: {text}"
)


class TestExtractMediaIssue37318:
"""End-to-end: base.py extract_media accepts the same ``.md`` tag that
triggered the user-visible bug in #37318."""

def test_md_tag_extracted(self):
from gateway.platforms.base import BasePlatformAdapter
content = "Here is the doc MEDIA:/tmp/report.md"
media, cleaned = BasePlatformAdapter.extract_media(content)
assert len(media) == 1
assert media[0][0] == "/tmp/report.md"
assert "MEDIA:" not in cleaned