Skip to content

fix(gateway): preserve spaced MEDIA paths - #24217

Open
felix-windsor wants to merge 2 commits into
NousResearch:mainfrom
felix-windsor:codex/fix-media-spaced-paths
Open

fix(gateway): preserve spaced MEDIA paths#24217
felix-windsor wants to merge 2 commits into
NousResearch:mainfrom
felix-windsor:codex/fix-media-spaced-paths

Conversation

@felix-windsor

Copy link
Copy Markdown
Contributor

Summary

  • preserve full MEDIA paths that contain spaces, including Windows drive paths and UNC shares
  • add GIS / structured-data extensions such as kmz, kml, geojson, gpx, json, xml, and html
  • reuse the base platform MEDIA parser in gateway result collection and update MCP/TUI handling

Fixes #24032

Tests

  • scripts/run_tests.sh tests/gateway/test_platform_base.py::TestExtractMedia tests/gateway/test_stream_consumer.py::TestCleanForDisplay tests/gateway/test_send_image_file.py tests/test_mcp_serve.py::TestAttachmentExtraction tests/cron/test_scheduler.py::TestResolveDeliveryTarget
  • npm test -- --run src/__tests__/markdown.test.ts
  • ruff check gateway/platforms/base.py gateway/stream_consumer.py gateway/run.py mcp_serve.py tests/gateway/test_platform_base.py tests/gateway/test_stream_consumer.py tests/test_mcp_serve.py

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) tool/mcp MCP client and OAuth labels May 12, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: This PR is a superset of #24132 and #24049 (both fix #24032). Also overlaps with #6742 (MCP spaced paths). This PR unifies the fix across gateway, MCP, stream consumer, and TUI.

@liuhao1024 liuhao1024 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.

The extension list _MEDIA_EXTENSIONS_RE is duplicated verbatim in 4 places: gateway/platforms/base.py, gateway/stream_consumer.py, mcp_serve.py, and (partially) ui-tui/src/components/markdown.tsx. If someone adds a new extension, they'd need to find and update all four copies — an easy miss.

Since base.py already defines it as a class constant on BasePlatformAdapter, consider importing it from there in the other Python files:

# stream_consumer.py / mcp_serve.py
from gateway.platforms.base import BasePlatformAdapter
_MEDIA_EXTENSIONS_RE = BasePlatformAdapter._MEDIA_EXTENSIONS_RE

For the TypeScript side, a shared MEDIA_EXTENSIONS array or a comment cross-referencing the Python source would help keep them in sync.

@Bartok9

Bartok9 commented May 12, 2026

Copy link
Copy Markdown
Contributor

I rechecked this against current origin/main (99ad2d137).

tests/gateway/test_platform_base.py::TestExtractMedia and tests/gateway/test_stream_consumer.py::TestCleanForDisplay both pass cleanly on current main (24 tests). The spaced-path parsing gap is still present in main. This PR's fix for space-containing paths and the added GIS/structured-data extensions look correct and the CI baseline should be clean for merge.

@felix-windsor

Copy link
Copy Markdown
Contributor Author

The extension list _MEDIA_EXTENSIONS_RE is duplicated verbatim in 4 places: gateway/platforms/base.py, gateway/stream_consumer.py, mcp_serve.py, and (partially) ui-tui/src/components/markdown.tsx. If someone adds a new extension, they'd need to find and update all four copies — an easy miss.

Since base.py already defines it as a class constant on BasePlatformAdapter, consider importing it from there in the other Python files:

# stream_consumer.py / mcp_serve.py
from gateway.platforms.base import BasePlatformAdapter
_MEDIA_EXTENSIONS_RE = BasePlatformAdapter._MEDIA_EXTENSIONS_RE

For the TypeScript side, a shared MEDIA_EXTENSIONS array or a comment cross-referencing the Python source would help keep them in sync.

Addressed in 663120ff5.

  • Reused BasePlatformAdapter._MEDIA_TAG_RE for the stream consumer.
  • Reused BasePlatformAdapter.extract_media() in mcp_serve.py instead of keeping a separate extension regex.
  • Added a shared MEDIA_EXTENSIONS array in the TUI with a comment pointing back to the Python source of truth.

Verified with:

  • scripts/run_tests.sh tests/gateway/test_platform_base.py::TestExtractMedia tests/gateway/test_stream_consumer.py::TestCleanForDisplay tests/gateway/test_send_image_file.py tests/test_mcp_serve.py::TestAttachmentExtraction tests/cron/test_scheduler.py::TestResolveDeliveryTarget
  • npm run build --prefix packages/hermes-ink && npm test -- --run src/__tests__/markdown.test.ts
  • ruff check gateway/stream_consumer.py mcp_serve.py

@felix-windsor

Copy link
Copy Markdown
Contributor Author

Follow-up commit pushed for the extension-list deduplication feedback. This is ready for another look when convenient.

@Bartok9

Bartok9 commented May 14, 2026

Copy link
Copy Markdown
Contributor

Verified 663120ff5. Clean dedup work:

  • gateway/stream_consumer.py now uses BasePlatformAdapter._MEDIA_TAG_RE (single source of truth)
  • mcp_serve.py calls BasePlatformAdapter.extract_media() directly — eliminates the separate extension regex entirely
  • ✅ TUI MEDIA_EXTENSIONS array with cross-reference comment back to Python — best you can do across language boundaries
  • ✅ Test coverage confirmed across Python (gateway + mcp_serve + cron) and TUI (Vitest)
  • ruff check clean

The cross-language comment pattern is the right tradeoff. Future maintainers updating the Python regex will see the TUI comment when they grep for the extension list. Approving on the follow-up. 🎻

@felix-windsor

Copy link
Copy Markdown
Contributor Author

Thanks for rechecking and validating the follow-up. I’ll leave the PR unchanged unless maintainers spot anything else.

@felix-windsor

Copy link
Copy Markdown
Contributor Author

Rebased on latest main (as of 2026-05-18) and force-pushed.

Local verification:

  • ./scripts/run_tests.sh tests/gateway/test_platform_base.py::TestExtractMedia tests/gateway/test_stream_consumer.py::TestCleanForDisplay tests/test_mcp_serve.py::TestAttachmentExtraction tests/cron/test_scheduler.py::TestResolveDeliveryTarget

(Still seeing no CI statuses reported on the PR head; may need maintainer Actions approval for fork PRs.)

@felix-windsor
felix-windsor force-pushed the codex/fix-media-spaced-paths branch from 9d12f80 to 34c4512 Compare May 18, 2026 12:37
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this PR is a superset unifying the spaced/Windows/GIS MEDIA-path fix across gateway, MCP, stream consumer, and TUI. It supersedes the narrower attempts:

Marked related_to these rather than duplicating them, so a maintainer can pick the unified fix over the piecemeal ones. Priority held at P2. _MEDIA_EXTENSIONS_RE is duplicated across 4 files (per @felix-windsor) — worth consolidating into a shared constant before merge.

@Bartok9

Bartok9 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

One correction on the triage note: the _MEDIA_EXTENSIONS_RE duplication flagged here was already resolved in 663120ff5 (2026-05-13). stream_consumer.py now reuses BasePlatformAdapter._MEDIA_TAG_RE, mcp_serve.py calls BasePlatformAdapter.extract_media() directly (no separate regex), and the TUI carries a shared MEDIA_EXTENSIONS array with a cross-reference comment back to the Python source. So the consolidation ask is done — the only remaining blocker is the CONFLICTING merge state (needs a rebase on current main). Superset framing over #24132/#24049/#6742/#31035 is accurate. 🎻

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the unified MEDIA-path work. Current main already incorporated a newer gateway-side media parsing architecture in 781604ce4: MEDIA_DELIVERY_EXTS and MEDIA_TAG_CLEANUP_RE now own backend parsing at gateway/platforms/base.py:1432-1474, and streaming delegates to strip_media_directives_for_display() at gateway/stream_consumer.py:889-902.

Problems

  • The PR's added class-local extension regex would duplicate and narrow that current source of truth, bypassing the protected-span and JSON masking used by extract_media() at gateway/platforms/base.py:3629-3630.
  • The remaining live gaps are real: MCP still uses MEDIA:\\s*(\\S+) at mcp_serve.py:276, TUI uses \\S+? at ui-tui/src/components/markdown.tsx:89, and gateway auto-append/history collection uses \\S+ at gateway/run.py:1061-1066.
  • The requested GIS extensions are absent from current MEDIA_DELIVERY_EXTS at gateway/platforms/base.py:1432-1449; MCP reuse alone would not add support for them.

Suggested changes

  • Salvage the MCP and TUI portions against the current shared parser, add approved GIS extensions to MEDIA_DELIVERY_EXTS, and cover the current auto-append/history matcher while retaining its producer-tool and dedup safeguards.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 13, 2026
@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@Bartok9

Bartok9 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Confirming the sweeper's read against current origin/main. Main did adopt the newer parser in 781604ce4: MEDIA_DELIVERY_EXTS/MEDIA_TAG_CLEANUP_RE own backend parsing (gateway/platforms/base.py:1432-1474) and streaming delegates via strip_media_directives_for_display() (gateway/stream_consumer.py:889-902). That supersedes this PR's class-local extension regex, and re-introducing it would bypass the protected-span + JSON masking in extract_media().

The live gaps the sweeper flags are real and still worth closing:

  • MCP MEDIA:\\s*(\\S+) at mcp_serve.py:276
  • TUI \\S+? at ui-tui/src/components/markdown.tsx:89
  • gateway auto-append/history \\S+ at gateway/run.py:1061-1066
  • GIS extensions (kmz/kml/geojson/gpx) absent from MEDIA_DELIVERY_EXTS

Right move is to salvage the MCP + TUI + auto-append portions against the current shared parser rather than rebase the old regex, and add the approved GIS extensions to MEDIA_DELIVERY_EXTS directly — preserving the producer-tool and dedup safeguards. 🎻

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

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

[BUG] extract_media regex truncates Windows spaced paths and rejects GIS extensions (.kmz/.kml/.geojson/.gpx)

5 participants