fix(gateway): allow [[as_document]] glued directly to MEDIA path - #63644
fix(gateway): allow [[as_document]] glued directly to MEDIA path#63644liuhao1024 wants to merge 1 commit into
Conversation
…sResearch#63632) MEDIA_TAG_CLEANUP_RE failed to match when a directive like [[as_document]] was concatenated directly to the file extension without whitespace (e.g., MEDIA:/home/user/report.xlsx[[as_document]]). This caused the file to be silently not delivered while the gateway reported success. Root cause: the lookahead character class [\s`",;:)\}\]|$) did not include [, so [[as_document]] immediately after the extension broke the lookahead assertion. Fix: add \[ to the lookahead class so directives can follow the path without whitespace. This is safe because [ is already stripped elsewhere in the same file via .replace("[[as_document]]", ""). Added regression tests covering: - Directive glued to extension (issue case) - Directive with whitespace (baseline) - Tag at end of string ($ anchor)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real media-extraction edge case. The premise still holds on current main: extract_media() scans the original response at gateway/platforms/base.py:3684-3687, while MEDIA_TAG_CLEANUP_RE at gateway/platforms/base.py:1503 does not currently accept [ after the extension.
Problems
- The proposed
[boundary atgateway/platforms/base.py:1495is broader than the supported directive grammar. It also accepts arbitrary bracket-bearing filenames, allowing the regex to match a valid-extension prefix rather than leaving an unrecognized path intact. tests/gateway/test_media_tag_cleanup.pytests onlyMEDIA_TAG_CLEANUP_RE. The delivery path isBasePlatformAdapter.extract_media()(gateway/platforms/base.py:3637-3731), and current code supports[[as_document]], not[[as_image]](gateway/platforms/base.py:1542,3667-3671).
Suggested changes
- Match the literal
[[as_document]]boundary rather than a generic[, and add a production-path regression test intests/gateway/test_platform_base.pyasserting both extraction and cleaned output.
Automated hermes-sweeper review.
| r'''(?P<path>`[^`\n]+`|"[^"\n]+"|'[^'\n]+'|''' | ||
| r'''(?:~/|/|[A-Za-z]:[/\\])\S+(?:[^\S\n]+\S+)*?\.(?:''' + _MEDIA_EXT_ALTERNATION + r'''))''' | ||
| r'''(?=[\s`"',;:)\]}]|$)[`"']?''', | ||
| r'''(?=[\s`"',;:)\]}\[]|$)[`"']?''', |
There was a problem hiding this comment.
Please restrict this new boundary to the literal supported [[as_document]] directive rather than any [. A generic bracket boundary can make MEDIA:/safe/report.xlsx[revision] match and deliver the .xlsx prefix, whereas the current anchored parser leaves that unrecognized path intact.
| assert "MEDIA:" not in stripped | ||
| assert "/home/hermes/report.xlsx" not in stripped | ||
|
|
||
| # Other directives ([[as_image]]) should also work |
There was a problem hiding this comment.
[[as_image]] is not a supported directive on current main: cleanup only strips [[audio_as_voice]] and [[as_document]]. Please remove this case or implement that directive separately; add an extract_media() regression for the supported glued [[as_document]] form instead.
|
Merged via PR #72170 (rebase-merge) — your commit was cherry-picked onto current main with your authorship preserved in git history. Thanks for the fix! PR #72170 consolidated six open MEDIA-delivery fixes (formatting-variant regex misses + the Discord upload race) into one salvage wave so the overlapping regex changes could be resolved together. |
What does this PR do?
Fixes a regex bug in
MEDIA_TAG_CLEANUP_REthat caused MEDIA tags to be silently ignored when a directive like[[as_document]]was concatenated directly to the file extension without whitespace.When the regex failed to match, the MEDIA tag was not stripped and the file path was not extracted, resulting in the file not being delivered while the gateway reported success (no error surfaced).
Root cause
The lookahead character class
[\s"',;:)}]|$)inMEDIA_TAG_CLEANUP_REdid not include[. When[[as_document]]immediately followed the file extension (e.g.,MEDIA:/home/user/report.xlsx[[as_document]]), the lookahead assertion failed because[` was not a permitted boundary character.Fix
Add
\[to the lookahead character class so it becomes[\s"',;:)}[]|$)`. This allows directives to follow the path without whitespace.This change is safe because
[characters (specifically[[as_document]]and[[as_image]]) are already stripped elsewhere in the same file via a literal.replace("[[as_document]]", "")call.Related Issue
Fixes #63632
Type of Change
Changes Made
gateway/platforms/base.py: Added\[to the lookahead character class inMEDIA_TAG_CLEANUP_REtests/gateway/test_media_tag_cleanup.py: New test file with 3 regression testsHow to Test
Run the new regression tests:
Observed result: All 3 tests pass, confirming that:
MEDIA:/home/hermes/report.xlsx[[as_document]]now matches (fixes the issue)MEDIA:/home/hermes/report.xlsx [[as_document]]still matches (baseline unchanged)MEDIA:/tmp/file.docxat end of string still matches (baseline unchanged)You can also verify the fix with a quick Python REPL session:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/gateway/test_media_tag_cleanup.py -vand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A