fix(gateway): salvage MEDIA: false-positive guards (#16721 + #35699) - #36275
Conversation
|
CI note: the two red shards (
This PR only changes |
tonydwb
left a comment
There was a problem hiding this comment.
Review: APPROVE ✅
This PR salvages two related fixes for MEDIA: false-positive guards in the gateway, cherry-picked from #16721 and #35699 with current-main reconciliation.
Correctness
_mask_protected_spans(): correctly masks fenced code blocks, inline code, and blockquotes before MEDIA tag extraction, while preserving backtick-quoted paths inMEDIA:tags. Character count is preserved so regex offsets stay valid.extract_media(): masked content is used for both matching and cleaning, preventing bogus extractions while keeping real tags working._collect_auto_append_media_tags(): extracts inline scan logic into a named function with proper tool-name-to-call-ID mapping, producer-tool allowlist (text_to_speech/text_to_speech_tool), current-turn isolation viahistory_offset, and a compression-safe fallback path. All three concerns from the original code are preserved or improved.
Security
No new security concerns. The existing filter_local_delivery_paths handles path safety.
Code Quality
- Clean refactoring — the inline scan block in
gateway/run.pyis extracted into a well-documented helper function. _TOOL_MEDIA_REnow compiled once at module level (previously recompiled on every scan loop iteration).- Edge case handling for backtick-quoted
MEDIA:paths inside inline code is careful and correct.
Testing
- 7 new tests across
test_platform_base.pyandtest_media_extraction.pycovering:- MEDIA in fenced code blocks ignored
- MEDIA in inline code ignored
- MEDIA in blockquotes ignored
- Real MEDIA outside protected regions still extracted
- Mixed code+prose extraction
- Skill docs not auto-appended
- Real TTS output still auto-appended
- All existing tests pass (139 passed, 2 skipped).
Performance
No concerns. _mask_protected_spans() does a few linear passes over reply text, acceptable for typical message lengths.
Documentation
- PR body is thorough with per-fix breakdown, reconciliation notes, and verification results.
- Code docstrings reference the relevant PRs.
AUTHOR_MAPupdated to credit the original contributor.
Verdict
Clean fix, well-tested, well-documented. Approving.
Self-review (hermes-pr-review)Ran the regex-defense review workflow (empirical false-positive + bypass sweeps against the live functions, plus an adversarial correctness pass). One real bug found and fixed in this PR; remaining items dispositioned below. 🔴 Fixed in this PR (commit "keep code blocks verbatim in cleaned text when media present")
The existing tests missed it because all the code-block cases had empty Fix: mask only a length-equal copy of 🟡 Known limitation (not a regression — pre-existing main behavior, preserved)Auto-append compression-safe fallback dedups by exact path string. When mid-run context compression shrinks the message list below 🟢 Verified clean (empirical sweeps)
Minor / acknowledged
Tests: |
|
CI after the review-fix push: same two pre-existing unrelated red shards as before — |
extract_media() scanned the full response text without distinguishing live delivery tags from example paths in fenced code blocks, inline code spans, and blockquotes. This caused false positives where the agent's explanation of MEDIA: syntax (or tool output containing example paths) was stripped from user-visible text and the path was added to the media delivery list. Added _mask_protected_spans() helper that replaces protected regions with equal-length whitespace before regex matching, preserving match offsets. The helper skips backtick-quoted paths in MEDIA: tags to maintain existing path extraction behavior. Fixes NousResearch#35695
…esent
Self-review of the code-block masking fix: the cleanup path ran
media_pattern.sub('') over the _mask_protected_spans() copy of the text and
assigned that back to 'cleaned', so whenever a real MEDIA: tag was delivered
(if media: branch), every fenced code block / inline code / blockquote in the
reply was blanked to whitespace in the user-visible text.
Now mask only a length-equal copy of 'cleaned' to locate the real tag spans,
then delete those spans from the unmasked 'cleaned' — masking is a locator,
not a text rewrite. Protected spans survive verbatim. Strengthens the existing
mixed-code test (it only asserted 'Done.' survived, not the code block) and
adds an inline-code-survives regression test. Both fail on the old sub-based
code and pass now.
9abf01f to
f810149
Compare
Summary
User-requested batch salvage of two tiny, related
MEDIA:false-positive fixes for the gateway. Both target the same problem class —MEDIA:example strings in ordinary text getting mis-delivered as real attachments — on two different code paths.Original PRs salvaged (commits cherry-picked, original authorship preserved):
gateway/run.py)MEDIA:tags inside fenced code blocks, inline code, and blockquotes (gateway/platforms/base.py)What each fix does
#16721 — producer-tool allowlist for auto-append (
gateway/run.py)When the model omits a
MEDIA:tag from its final reply, the gateway scans the turn's tool results and auto-appends any media it finds. Previously it scanned all tool output, so a literalMEDIA:/path/...example string inskill_viewdocs, logs, or search results could be delivered as a bogus attachment. This adds_collect_auto_append_media_tags()keyed by a producer-tool allowlist (text_to_speech), so only tools that actually emit deliverables are eligible.#35699 — mask protected spans in
extract_media(gateway/platforms/base.py)extract_mediaranMEDIA_TAG_CLEANUP_REover the raw reply text, so an exampleMEDIA:/path.pnginside a```fenced block,`inline code`, or a>blockquote was extracted as a real attachment. This adds_mask_protected_spans()which blanks those regions (preserving offsets) before matching, while keeping backtick-quotedMEDIA:paths working.Current-main reconciliation (in #16721's cherry-pick)
#16721 was opened against an older
gateway/run.py. Current main has since gained the #34608 current-turn slice isolation and a compression-safe fallback (#160). The cherry-pick was reconciled so the salvaged helper keeps both:history_offset([Bug]: MEDIA tag from stale tool output in session history leaks into later text-only responses #34608),_history_media_pathsdedup) when mid-run compression shrinks the list below the history length (Telegram: All voice messages are attached to replies and send multiple times #160),_TOOL_MEDIA_REalready on main (instead of the looserMEDIA:(\S+)in the original PR),and layers on #16721's producer-tool allowlist as the primary guard. Net result is strictly tighter than current main with no regression to the compression or slicing behavior.
Follow-up commit on top
chore(release): add@VinciZhu(haaasined@gmail.com) toscripts/release.pyAUTHOR_MAP so the contribution is credited in release notes. (@liuhao1024 was already mapped.)Verification
tests/gateway/test_platform_base.py+tests/gateway/test_media_extraction.py— 139 passed, 2 skipped (both PRs' new tests included).MEDIA:not extracted, real prose tag still extracted; non-producer tool output not auto-appended, real TTS output still auto-appended and preserved under the compression-safe fallback path.Scope
Two concerns, both narrow and in the same subsystem; touched files limited to the two production files, their two test files, and the AUTHOR_MAP. A third related PR (#34388, serialized-JSON
MEDIA:boundary) is intentionally not in this batch — it will be handled separately because its line-start-anchor approach needs reworking against current main.