Skip to content

fix(mcp): support quoted and spaced MEDIA paths in attachment extraction - #6742

Open
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/mcp-media-path-integrity
Open

fix(mcp): support quoted and spaced MEDIA paths in attachment extraction#6742
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/mcp-media-path-integrity

Conversation

@Dusk1e

@Dusk1e Dusk1e commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Problem

_extract_attachments() in mcp_serve.py uses a single-token regex
(MEDIA:\s*(\S+)) to extract attachment paths. This truncates or
malforms any path that contains spaces or is wrapped in quotes/backticks:

MEDIA: "C:\Users\\Desktop\my image.png"   → only captures `"C:\Users\\Desktop\my`
MEDIA: '/tmp/my image.png'                      → only captures `'/tmp/my`

This breaks attachments_fetch for valid Hermes outputs, especially on
Windows where file paths commonly include spaces.

This is distinct from the earlier fix in gateway/platforms/base.py:
the gateway path extraction was already hardened there, but the MCP
bridge still had the older, narrower parser.

What changed

Updated mcp_serve.py so _extract_attachments() no longer relies on
the single-token MEDIA:\s*(\S+) pattern. It now accepts:

  • Quoted paths ("..." and '...')
  • Backticked paths (`...`)
  • Unix absolute and tilde paths with spaces
  • Windows drive-letter paths with spaces

Extracted paths are normalized by trimming wrapping quotes/backticks and
trailing punctuation before being returned.

Why it matters

  • Real behavior bug, not a refactor.
  • Small and isolated to the MCP bridge.
  • Improves Windows compatibility.
  • Closes a consistency gap between gateway delivery parsing and MCP
    attachment parsing.

Tests

Added regression coverage in tests/test_mcp_serve.py:

  • Quoted Unix path with spaces: MEDIA: "/tmp/my image.png"
  • Quoted Windows path with spaces: MEDIA: "C:\Users\\Desktop\my image.png"
  • Backticked path: MEDIA: `/tmp/out file.mp3`
  • Trailing punctuation boundary: MEDIA: "/tmp/out.png", done
  • Existing simple-path behavior unchanged

Verification:

python -m pytest tests/test_mcp_serve.py -q
# Result: 44 passed, 39 skipped

Manual verification

  1. Call _extract_attachments() on text containing quoted MEDIA: paths
    with spaces and verify the full path is returned.
  2. Confirm existing plain MEDIA: /tmp/out.png behavior still works.
  3. Optionally run hermes mcp serve and verify attachments_fetch
    returns the full path for a message containing a quoted MEDIA: tag.

Files changed

  • mcp_serve.py — updated _extract_attachments() parser
  • tests/test_mcp_serve.py — added regression tests

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing a real MCP extraction gap: current main still truncates space-containing MEDIA: paths in mcp_serve.py:276, and attachments_fetch exposes that result at mcp_serve.py:753.

Problems

  • The added unquoted-path regex at mcp_serve.py:161 only matches a lower-case, limited extension set. MEDIA: /tmp/My Image.PNG falls through to \S+ and still returns /tmp/My. The current gateway equivalent is case-insensitive and derives from the broader canonical extension set in gateway/platforms/base.py:1432-1449 and gateway/platforms/base.py:1468-1473.

Suggested changes

  • Make the unquoted spaced-path branch case-insensitive, align it with the gateway extension policy, and cover uppercase plus non-image deliverable extensions in tests/test_mcp_serve.py.

Automated hermes-sweeper review.

Comment thread mcp_serve.py
if text:
media_pattern = re.compile(r'MEDIA:\s*(\S+)')
media_pattern = re.compile(
r'''[`"']?MEDIA:\s*(?P<path>`[^`\n]+`|"[^"\n]+"|'[^'\n]+'|(?:[A-Za-z]:[\\/]|~/|/)[^\n]*?\.(?:png|jpe?g|gif|webp|mp4|mov|avi|mkv|webm|ogg|opus|mp3|wav|m4a|pdf|txt|csv)(?=[\s`"',;:)\]}]|$)|\S+)[`"']?'''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unquoted-path branch only accepts a lower-case subset of extensions. MEDIA: /tmp/My Image.PNG misses this branch and falls back to \S+, returning /tmp/My. Please make this case-insensitive and align the recognized extensions with the gateway's canonical media-extension policy.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 12, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Two PRs address the MCP _extract_attachments() regex that truncates quoted or space-containing MEDIA: paths. #6742 covers quoted, backticked, Unix, and Windows paths but has an extension-policy gap, while #24176 implements the narrower quoted/backticked-path fix.

Related pull requests

  • #6742 related — (+33/-3) — preferred after revision: Replaces the single-token parser with handling for quoted, backticked, and unquoted spaced paths and adds Unix/Windows regression coverage. Consistent with the keep_open review on #6742, its unquoted-path branch must first become case-insensitive, use the gateway's broader canonical extension policy, and gain uppercase and non-image extension tests.
  • #24176 [closed] duplicate — (+19/-4) — closed duplicate: Fixes the same quoted/backticked-path truncation while deliberately preserving token-only parsing for unquoted paths, with one quoted-space regression test. It remains relevant as a narrower implementation reference but does not cover #6742's intended unquoted spaced-path behavior and was closed in favor of #6742.

Duplicates

#6742 and #24176 substantially duplicate the quoted/backticked MEDIA: path parsing change; #6742 additionally attempts unquoted spaced-path support.

Suggested consolidation

Update and merge #6742 after addressing its contributor review by aligning case sensitivity and supported extensions with the gateway parser and adding the requested regressions; keep #24176 closed as the narrower duplicate.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup6742 ["PRs duplicating each other"]
        P6742["PR #6742 (open)"]
        P24176["PR #24176 (closed)"]
    end
    class P6742 open
    class P24176 closed
    class P6742 target
    click P6742 "https://github.com/NousResearch/hermes-agent/pull/6742"
    click P24176 "https://github.com/NousResearch/hermes-agent/pull/24176"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 3 kB of issue/PR text, 1 kB of discussion (3 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants