Skip to content

fix(gateway): normalize git-bash MEDIA: paths to native Windows paths - #41643

Closed
deacon-botdoctor wants to merge 1 commit into
NousResearch:mainfrom
Bot-Doctor-LLC:botdoctor/media-path-normalize-upstream
Closed

fix(gateway): normalize git-bash MEDIA: paths to native Windows paths#41643
deacon-botdoctor wants to merge 1 commit into
NousResearch:mainfrom
Bot-Doctor-LLC:botdoctor/media-path-normalize-upstream

Conversation

@deacon-botdoctor

Copy link
Copy Markdown
Contributor

Summary

On Windows, models running under git-bash / MSYS often emit POSIX-looking paths (/c/Users/..., /tmp/...) in MEDIA: tags. Native Windows file APIs can't open those, so the attachment is silently dropped.

  • add _normalize_media_path and apply it at the single extract_media append site
  • /<drive>/rest<DRIVE>:/rest, /tmp/rest%TEMP%/rest, ~/rest → expanded home
  • strictly a no-op on non-Windows hosts and for paths that match no rule, so Linux/macOS behavior is unchanged

Validation

  • python3 -m py_compile gateway/platforms/base.py tests/gateway/test_platform_base.py
  • .venv/bin/python -m pytest tests/gateway/test_platform_base.py (152 passed, 2 skipped)
  • git diff --check

Scope check

  • rebuilt from fresh NousResearch/hermes-agent main
  • scanned staged diff for obvious secrets/private identifiers and downstream overlay markers

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels Jun 8, 2026

@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 targeting a real Windows gateway delivery gap. Current main still returns an MSYS-form path unchanged from BasePlatformAdapter.extract_media() (gateway/platforms/base.py:3658), so the issue remains relevant.

Problems

  • The new call at gateway/platforms/base.py:2974 covers only extension-bearing MEDIA: tags. Current main handles extensionless tags in a separate branch that calls validate_media_delivery_path(path) directly (gateway/platforms/base.py:3669), so MEDIA:/c/.../Caddyfile would still bypass normalization.
  • The added tests exercise the helper only; they do not cover the extraction-plus-validation path or the extensionless branch.

Suggested changes

  • Normalize at the beginning of validate_media_delivery_path() before constructing Path. That is the shared native-delivery safety boundary used by regular media filtering (gateway/platforms/base.py:3502) and extensionless tags.
  • Add simulated-Windows regression coverage for both extension-bearing and extensionless MSYS paths. The existing helper in tools/environments/local.py:23-48 also supports /cygdrive/<drive>/... and /mnt/<drive>/...; align the accepted forms if those paths are in scope.

Automated hermes-sweeper review.

Comment thread gateway/platforms/base.py
@@ -2946,7 +2974,7 @@ def extract_media(content: str) -> Tuple[List[Tuple[str, bool]], str]:
path = path.lstrip("`\"'").rstrip("`\"',.;:)}]")

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 normalizes only the extension-bearing extract_media() branch. Current main's extensionless MEDIA flow calls validate_media_delivery_path(path) directly, so /c/.../Caddyfile still bypasses the conversion. Please normalize at the start of validate_media_delivery_path() instead, before Path(...), so every native attachment path shares the same conversion and safety checks.

@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 14, 2026
@Thouser-wu

Copy link
Copy Markdown

Also affected on Windows 10 + Git Bash (Hermes v33). Feishu bot cannot send files, videos, or images because the agent emits MEDIA: paths in MSYS format (/C:/Users/Wujunze/...) and validate_media_delivery_path() rejects them with Skipping unsafe MEDIA directive path.

The failure chain:

  1. Agent produces MEDIA:/C:/Users/Wujunze/AppData/Local/hermes/audio_cache/pig_diving.mp4
  2. Path("/C:/Users/...").is_absolute() returns False on native Windows Python
  3. resolve(strict=True) throws WinError 123 (invalid path syntax)
  4. validate_media_delivery_path() returns None, tag is stripped but treated as plain text

Related open issues: #47767, #31457. Related open PRs: #31472, #32735. All of these stem from the same root cause — Windows MSYS/Git Bash drive-letter paths (/C:/...) are not normalized to native Windows paths (C:/...) before validation.

Would really appreciate getting this merged so Feishu (and other platform) users on Windows can actually receive file attachments.

@Thouser-wu

Copy link
Copy Markdown

Follow-up: I dug deeper after testing locally and found a second variant of this bug that my original comment missed.

The agent actually emits two MSYS path formats on Windows, not just one:

  1. /C:/Users/... (forward slashes — what I originally reported)
  2. /C:\Users\... (backslashes — what QQ Bot's adapter actually produces)

Both get rejected for the same reason: Path().is_absolute() returns False and resolve() throws WinError 123. My local fix normalizes both variants:

if os.name == "nt" and re.match(r"^/[A-Za-z]:[/\\]", path):
    path = path[1:]

This handles both /C:/ and /C:\ prefixes. Sharing in case the PR fix needs to cover both cases.

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

This was generated by AI during triage.

Summary

Two PRs address the Windows/MSYS media-path failure: #41596 normalizes /mnt/<drive>/ and /<drive>/ paths across extraction and validation, while #41643 normalizes /<drive>/ and /tmp/ only at the extension-bearing MEDIA: extraction site. Neither diff covers the additionally reported /C:/... and /C:\... forms, and #41643 also leaves extensionless tags and the shared validation boundary unchanged.

Related pull requests

  • #41596 [closed] duplicate — (+92/-15) — broader reference implementation, currently closed: it centralizes normalization for explicit tags, bare-file extraction, and validate_media_delivery_path(), with extraction tests for /mnt/c/... and /c/...; however, it needs rebasing onto the current parser plus filter/validation tests and support for /C:/... and /C:\.... Despite the keep_open review on #41596, retaining it as a closed reference rather than reopening it is appropriate because its diff is stale and its own contributor review requires a rebase and missing delivery-boundary coverage.
  • #41643 related — (+58/-1) — preferred consolidation target, but not merge-ready: it adds /c/..., /tmp/..., and home expansion at the extension-bearing extraction append site, while the contributor keep_open review correctly identifies that extensionless tags and validate_media_delivery_path() bypass the helper. Move normalization to the shared validation boundary and add end-to-end Windows-mocked coverage, including /c/..., /C:/..., /C:\..., and extensionless paths, before merging.

Duplicates

#41596 and #41643 substantially duplicate the core /<drive>/... Windows normalization, although #41596 has broader extraction/validation scope and #41643 uniquely adds /tmp/ mapping.

Suggested consolidation

Merge #41643 after replacing its extraction-only normalization with shared validation-boundary normalization, incorporating the useful broader coverage from closed #41596, and adding regression tests for extension-bearing and extensionless delivery paths plus all reported MSYS drive forms. Keep #41596 closed as the superseded reference implementation; no additional duplicate PR needs closing.

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 Dup41596 ["PRs duplicating each other"]
        P41596["PR #41596 (closed)"]
        P41643["PR #41643 (open)"]
    end
    class P41596 closed
    class P41643 open
    class P41643 target
    click P41596 "https://github.com/NousResearch/hermes-agent/pull/41596"
    click P41643 "https://github.com/NousResearch/hermes-agent/pull/41643"
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: 13 kB of PR diffs, 2 kB of issue/PR text, 5 kB of discussion (5 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@deacon-botdoctor

Copy link
Copy Markdown
Contributor Author

Closing as part of open-PR prune.

Why: CONFLICTING Windows MEDIA path normalize; cold; if revived, do it at validate_media_delivery_path() covering extensionless paths too (with #41609).

If the need resurfaces on current main, open a fresh tip-based PR with current contracts covered — do not revive this stale conflicting head.

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 P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants