master
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 happy5318
|
6455aa9b2b |
fix(media): two-pass MEDIA: scan (pre-pass for wrapped, bare class without backtick exclusion) (#7708 round 2)
#7708 re-gate (9/22 23:47), both findings: **Finding 1 (SILENT)**: A bare path that contains a backtick (MEDIA:/tmp/report`final.png) is truncated at the first backtick, so the server stops authorizing it. The round-1 fix traded one bug for another: adding the backtick to the bare-token exclusion set dropped the inline-code closing backtick (good) but truncated any bare path with a backtick in the filename (bad — POSIX and Windows both allow backticks). **Finding 2 (SILENT)**: TTS now reads filename fragments aloud (MEDIA:/tmp/report(1).png -> 'a file).png') because the parenthesis-exclusion was added to a site that should not need it. Inline code is already stripped at the top of the TTS flattener. **Fix shape** (maintainer-suggested two-pass scan): 1. Pre-pass: `MEDIA:([^`\s]+)` -> MEDIA:$1 — strips the inline-code backticks off the wrapped form so the bare class below sees a plain MEDIA:path. 2. Bare class: MEDIA:([^\s\)\]]) (original, backtick removed from the exclusion set) — captures the full filename even when the path itself contains a backtick. 3. TTS: restore the original MEDIA:[^\s]+ (no paren/bracket exclusion either). Inline code is stripped at the top of the flattener so the closing backtick of an inline-code wrapper has already been eaten by the time the TTS regex scans the text. Applied to all 7 capture sites: - api/routes.py:20453 (_MEDIA_TOKEN_RE, findall site 20513) - api/media_snapshots.py:507 (media_re, findall site 533) - static/ui.js:7695 (linkifier, replace site) - static/ui.js:9142 (TTS flattener — separate shape) - static/messages.js:4848 (chunk finalize, exec) - static/messages.js:4896 (combined walk, exec loop) - static/messages.js:4922 (partial tail, exec) **Tests** (24 total, all green): - 11 source-shape guards: each JS/PY site must carry the bare-token class literal AND the wrapped-form pre-pass literal - 4 behaviour tests against the live source: wrapped form strips closing backtick, bare form captures plain path, bare form with backtick in filename captures the full path (the new regression), trailing paren is still excluded - 2 round-1 regression guards documenting the trade-off that drove the two-pass compose - 1 composed two-pass test (pre-pass + bare class on 4 inputs) - 2 node --check syntax guards - 1 site-count guard: 5 JS + 2 PY = 7 capture sites, all guarded **Verification**: - pytest tests/test_issue7359_media_backtick_strip.py -v -> 24/24 pass - python3 scripts/ruff_lint.py --diff origin/master -> 'no new violations on added/modified lines. OK.' - node --check on the two modified JS files -> both OK |
||
|
happy5318
|
2330faa0e4 |
fix(media): MEDIA: token regex strips markdown backtick on 7 capture sites (#7359)
## Thinking Path When an assistant emits `` `MEDIA:/path` `` wrapped in inline-code markdown, the shared character class ``[^\s\)\]]+`` across all seven MEDIA capture sites greedy-matched the closing backtick into the path. The resulting ``/api/media?path=...%60`` URL 404'd on the legitimate file (issue #7359). The same drift between frontend linkification and backend allowlist means even if the URL is constructed with the trailing backtick, the session allowlist rejects the backtick-polluted path — the user gets a broken download and a 404 body saved as ``.json``. Verified the issue's repro: 6 sites share ``[^\s\)\]]+`` (the preview-flattener at ui.js:9142 is ``[^\s]+`` and is a 7th site, also covered). One additional site, ``messages.js:4896`` (combined walk), was missing from the original 6-site list in the issue body — the suggested fix shape applies to all seven. The fix is a one-character-set widening: add `` ` `` to the excluded class on every site so the capture stops at the markdown delimiter instead of including it. ## What Changed All seven sites: ``[^\s\)\]]+`` → ``[^\s\)\]\`]+`` (JS) and the Python equivalent ``MEDIA:([^\s\)\]\`]+)``: 1. ``static/ui.js:7695`` — linkifier regex (the front-door capture). 2. ``static/ui.js:9142`` — preview-text flattener (also drops ``)`` and ``]`` for symmetric handling; was ``[^\s]+`` which would still pull the backtick into the preview). 3. ``static/messages.js:4848`` — streaming chunk finalize. 4. ``static/messages.js:4896`` — streaming combined walk (the issue-missing 7th site). 5. ``static/messages.js:4922`` — streaming partial tail. 6. ``api/routes.py:20453`` — ``_MEDIA_TOKEN_RE`` (the session allowlist). 7. ``api/media_snapshots.py:507`` — ``media_re`` (settle-time snapshot capture). ## Why It Matters A user clicking an inline-code-wrapped MEDIA token gets a 404 body saved as ``.json`` instead of the file they asked for. On Android Chrome (and desktop Chrome), the 404 silently downloads as ``skills.zip.json`` with no UI signal that the original was missing. The backend allowlist and the frontend linkifier must agree, so the fix widens the character class on both sides. ## Verification - ``tests/test_issue7359_media_backtick_strip.py``: 17 passed (new). - 7 site-shape assertions (parametrized over the seven sites). - ``_MEDIA_TOKEN_RE`` behaviour: strips trailing backtick from inline-code payload, preserves bare-token capture, preserves trailing-``)`` exclusion. - ``media_re`` behaviour: same three. - pre-fix class round-trip: ``\`MEDIA:/a.zip\``` → ``["/a.zip\`"]`` (the reported bug); post-fix → ``["/a.zip"]``. - ``node --check`` on the two modified JS files. - ``pytest tests/ -k 'media and not 7359'``: 291 passed, 1 skipped, 0 failed — no existing media test regresses. - ``ruff check tests/test_issue7359_media_backtick_strip.py``: clean. - ``scripts/ruff_lint.py --diff origin/master`` curated gate: 0 new findings on added/modified lines. ## Contract Change No API contract change. UI rendering may now display ``somefile.zip`` (a file) where it previously displayed ``somefile.zip\``` (literal backtick in the preview); both already displayed correctly in the bare-token case, and the inline-code path was the broken one. ## Risks - Sites that depended on the backtick being included in the path would regress, but no consumer in this repo treats a trailing `` ` `` as part of a MEDIA path — the original six-site fix shape was maintainer-recommended and the 7th site matches the same pattern. - The preview flattener at ``ui.js:9142`` previously used a wider class (``[^\s]+`` only, no ``)``/``]``) so it would have matched a path containing a literal ``)``. The fix tightens that to match the other six sites; if anyone relied on the looser class the rendering would now use ``a file`` for the surrounding paragraph. Verified no test in the suite exercises a MEDIA token containing ``)``. ## Model Used Hermes Agent (step-5-preview) with pytest, ruff, and node. Tests are executed against the live compiled regexes (for the two Python sites) and against the literal source text (for the five JS sites), so a revert on any of the seven sites surfaces as a source-shape failure. ## Related - #7359 — original issue (KimSpecht, 2026-08-29) with the repro and six-site list. - #7334 — pre-existing PR (angelusbr, 2026-08-27) that covers three sites; this PR supersedes it by covering all seven and following the maintainer-recommended shape from the 9/20 comment. - Maintainer's 9/20 comment on #7359 named #7334 as "the correct existing implementation lane"; if #7334 lands first, this PR is closeable as duplicate and the maintainer's ``sprint-candidate`` label can move. Credit goes to angelusbr for the early character-class idea regardless of which PR merges. |