fix(gateway): build _TOOL_MEDIA_RE from shared MEDIA_EXT_ALTERNATION (#37318) - #37347
Closed
temalo wants to merge 1 commit into
Closed
fix(gateway): build _TOOL_MEDIA_RE from shared MEDIA_EXT_ALTERNATION (#37318)#37347temalo wants to merge 1 commit into
temalo wants to merge 1 commit into
Conversation
The auto-append and history-dedup regexes in gateway/run.py
hand-rolled their own extension whitelist that drifted behind
the source-of-truth list MEDIA_DELIVERY_EXTS in
gateway/platforms/base.py. Common deliverable extensions —
.md, .markdown, .json, .yaml, .yml, .toml, .html, .htm, .svg,
.bmp, .tiff, .tsv, .xml, .odt, .ods, .odp, .key, .tar, .bz2,
.xz — were silently dropped from the auto-append path, and
the dispatch regex in base.py would happily strip the MEDIA:
tag before delivery returned a misleading success.
User-visible symptom (from the issue): asking the agent to
send a .md file via Feishu/Telegram/etc returned
{success: True} but the file never reached the user.
Fix:
- Export MEDIA_EXT_ALTERNATION (was _MEDIA_EXT_ALTERNATION,
kept as alias) from gateway/platforms/base.py so other
dispatch sites can reuse the same alternation.
- Rebuild gateway/run.py's module-level _TOOL_MEDIA_RE from
the shared alternation.
- Replace the duplicate locally-redeclared _TOOL_MEDIA_RE
inside the history scan with a reference to the module
pattern, so the two patterns can never drift again.
- Add .markdown and .toml to MEDIA_DELIVERY_EXTS (called out
by the reporter, lossless additions). Deliberately keep
.py/.js/.sh out — the existing test_no_media_extensions
contract makes clear those should not be auto-shipped from
bare-path mentions.
Test coverage (tests/gateway/test_run_tool_media_ext_parity.py):
- Every extension in MEDIA_DELIVERY_EXTS is matched by
_TOOL_MEDIA_RE (parity guard against future drift).
- Each extension named in the issue now matches.
- Existing Unix/home-relative/Windows path matching unchanged.
- Invalid paths (relative, no anchor, unknown ext) still rejected.
- End-to-end: BasePlatformAdapter.extract_media now strips a
MEDIA:/tmp/report.md tag cleanly.
Regression check: full tests/gateway/ suite — baseline 39 failures
on upstream/main (pre-existing missing-deps / py3.14 / flake), my
branch 39 failures, new failures: zero.
Fixes NousResearch#37318
Collaborator
Contributor
Author
|
Closing as confirmed duplicate of #37342 per @alt-glitch's triage. Both PRs took the static-whitelist approach to fixing #37318; #29609 has the canonical dynamic fix and is the better path forward. Thanks for the catch — should've grepped open PRs against the same issue before opening. |
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.
Why
The agent emits
MEDIA:/tmp/report.md(or any of a dozen common deliverable extensions). The dispatch-site regex ingateway/platforms/base.pystrips the tag and returns{success: true}— but the file is never actually shipped to the user. The reporter found this on Feishu after extensive debugging that ruled out Feishu, streaming, and the platform adapter; the root cause was a regex whitelist mismatch one layer down.gateway/run.py's auto-append + history-dedup path uses its own_TOOL_MEDIA_REthat hand-rolled the extension list. The list was a strict subset of the canonicalMEDIA_DELIVERY_EXTStuple thatgateway/platforms/base.pyuses, so the two patterns drifted:MEDIA_DELIVERY_EXTS_TOOL_MEDIA_RE(run.py).md.json,.yaml,.yml,.xml.html,.htm,.svg.bmp,.tiff.tsv,.odt,.ods,.odp,.key.tar,.bz2,.xzResult:
_collect_auto_append_media_tagssilently dropped them on the way out, and the in-line copy of the same regex inside the history scan silently dropped them again on the way in.What
MEDIA_EXT_ALTERNATION(was_MEDIA_EXT_ALTERNATION, kept as a backwards-compat alias) fromgateway/platforms/base.pyso other dispatch sites can build their regex off the same source-of-truth alternation that drivesMEDIA_TAG_CLEANUP_REandextract_media.gateway/run.py's module-level_TOOL_MEDIA_REfrom that shared alternation._TOOL_MEDIA_REinside the history-scan loop with a reference to the module-level pattern, so a future drift can only happen in one place instead of three..markdownand.tomltoMEDIA_DELIVERY_EXTS(called out by the reporter, lossless additions).base.pynoting that.py/.js/.share deliberately not in the tuple — the existingtest_no_media_extensionscontract makes clear that bare-path mentions of arbitrary source files should not auto-ship. The reporter asked for them; the constitution says no, and I preserved the constitution.Behaviour change footprint
extract_media(base.py) gains.markdownand.tomlas recognised extensions. All other behaviour identical._TOOL_MEDIA_REingateway/run.pynow matches the fullMEDIA_DELIVERY_EXTSset instead of its old subset. This was the bug — fixing the regex strictly broadens what the auto-append path will deliver, which is exactly what the dispatch path already does.gateway/run.py(the_collect_auto_append_media_tagsallowlist and the per-tool MEDIA: detection) still gate which tools are eligible to emit MEDIA: tags in the first place, so the broadened regex doesn't open new attack surface — it only stops dropping deliverable tags that the dispatch layer was already approving.Test coverage
New file:
tests/gateway/test_run_tool_media_ext_parity.pytest_every_delivery_ext_matched_by_run_patternMEDIA_DELIVERY_EXTS. Locks in the parity contract so a future contributor who adds an extension to the shared tuple gets a failing test if they forget to keep the patterns in sync.test_issue_37318_named_extensions_match.md,.markdown,.json,.yaml,.yml,.toml, plus the silently-dropped pre-existing ones like.html/.svg/.bmp/...) now matches.test_run_pattern_uses_shared_alternationtest_existing_paths_still_matchtest_invalid_inputs_still_rejectedMEDIA:tokens are still rejected — the fix didn't loosen anchoring or accept arbitrary extensions.test_md_tag_extractedBasePlatformAdapter.extract_mediastrips aMEDIA:/tmp/report.mdtag cleanly.Pre-existing tests (
test_run_tool_media_re.py,test_media_extraction.py,test_extract_local_files.py) continue to pass, includingtest_no_media_extensionswhich guards the deliberate.py/.logexclusion.Full
tests/gateway/directory: baseline onupstream/mainhas 39 failures (pre-existing missing-deps / Python 3.14 / flake — unrelated). My branch has 39 failures, the same set; new failures introduced: 0.Out of scope
.py/.js/.shtoMEDIA_DELIVERY_EXTSeven though the issue asked for them, because the existingtest_no_media_extensionscontract intentionally excludes them from the bare-path auto-detector. Adding them would surprise existing users whose agents mention/tmp/script.pyin prose. Users who want source-file delivery can still use the explicitsend_messageMEDIA: flow with a glob, but the auto-detector should stay narrow. The comment inbase.pynearMEDIA_DELIVERY_EXTSdocuments this.gateway/run.py(line ~17121)" — that line is the history-dedup_TOOL_MEDIA_RE, and it's the second of the two patterns I fixed.Fixes #37318