fix(gateway): accept MSYS drive paths in media delivery on Windows - #76489
fix(gateway): accept MSYS drive paths in media delivery on Windows#76489Shamzilla wants to merge 1 commit into
Conversation
Git Bash rewrites Windows paths into MSYS form (`/c/Users/me/out.png`), so any file the agent produces via a Git Bash command is reported that way. `validate_media_delivery_path` fed that string straight to `Path()`. On Windows a rooted-but-driveless path has no drive, so `is_absolute()` is False and the function returned None. The file existed and was perfectly safe, but delivery was refused with "Skipping unsafe MEDIA directive path" and the attachment silently never arrived. Translate MSYS drive paths to native form before the absolute check. Gated on `sys.platform == "win32"`: on POSIX hosts `/c/...` is a legitimate absolute path and must be left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Related: #31472 also repairs the /c/... validator path and extends normalization to Cygwin, file-URL, and Weixin read seams. This focused patch has the same validator goal but a narrower mechanism/scope, so it is a competing subset rather than a duplicate. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the native-Windows MEDIA validation failure. The premise is confirmed on current main: gateway/platforms/base.py:1479-1484 constructs Path from the unnormalized candidate and rejects a rooted-but-driveless path before resolution.
Problems
gateway/platforms/base.py:1454creates a second, narrower normalizer.tools/environments/local.py:25-50already defines the Windows normalization contract for/c/...,/cygdrive/c/..., and/mnt/c/...; the new helper does not handle the latter two whilewebsite/docs/user-guide/windows-native.md:104-116documents Cygwin as a supported fallback.- Bare-path delivery remains unnormalized:
BasePlatformAdapter.extract_local_fileschecksos.path.isfile(expanded)atgateway/platforms/base.py:4625-4626before delivery filtering, so a bare/c/...path is still missed.
Suggested changes
- Consolidate on one low-dependency normalizer and apply it at both the validator and bare-path existence seams.
- Add behavioral coverage for bare
/c/...extraction in addition to validator coverage.
Automated hermes-sweeper review.
| _MSYS_DRIVE_RE = re.compile(r"^/([A-Za-z])(?:/(?P<rest>.*))?$") | ||
|
|
||
|
|
||
| def _msys_to_windows_path(path: str) -> str: |
There was a problem hiding this comment.
Please consolidate this with the existing Windows path-normalization contract in tools/environments/local.py:25-50 (or extract a shared low-dependency utility). That helper also accepts documented Cygwin /cygdrive/c/... and /mnt/c/... forms; this narrower parser leaves those paths rejected by the validator.
Native Win11 verification (monerostar)Host: native Windows 11 ( Bug baseline (no patch needed to see the Path trap)So Live conversion + delivery probe (PR head)TestsReview notes
LGTM from native Win11. Happy to re-check if the diff moves. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Four PRs address the Windows media-path failure: #47821 and #57696 normalize /c/... before validation, #73043 instead normalizes the distinct /C:/... and /C:\... tag forms, and #76489 adds a local /c/... converter plus validator tests. None addresses the issue’s separate stale-token/CDN-500 report.
Related pull requests
- #47821 [closed]
best fix— (+5/-0) — historical focused fix: converts/c/...to a native drive path insidevalidate_media_delivery_path(), directly targeting the reported absolute-path rejection, but without regression coverage; although closed, it remains relevant as the earlier canonical implementation referenced by #57696. - #57696 [closed]
best fix— (+28/-0) — historical tested duplicate of #47821: reusestools.environments.local._msys_to_windows_path()at the validator boundary and adds a normalization test. Although closed, it remains relevant because it demonstrates the shared-helper approach now requested for #76489. - #73043
related— (+7/-1) — keep open with a salvage path: handles the distinct slash-colon forms/C:/...and/C:\...in_normalize_media_tag_path(), not the issue’s primary/c/...validator form. This agrees with the maintainer-bot keep_open verdict; add the requested behavioral extraction tests for both spellings. - #76489
partial— (+81/-0) — author action: consolidate with the existing normalizer and cover the remaining extraction seam. Despite the visible keep_open review, its diff duplicates only the/c/...subset in a new narrower helper; the contributor review identifies missing/cygdrive/c/...and/mnt/c/...support and shows that bare-path extraction still checks existence before normalization, while the supplied tests cover conversion and validator behavior rather than that bare-path path.
Duplicates
#47821 and #57696 are substantially the same /c/... validator fix, with #57696 adding shared-helper reuse and a regression test. #76489 overlaps that same validator goal but is a competing, narrower implementation; #73043 is complementary rather than duplicate because it handles /C:/... and /C:\... during tag normalization.
Suggested consolidation
Keep #73043 open with a salvage path: preserve its distinct slash-colon normalization and add the extraction tests requested by the maintainer-bot verdict. For #76489, author action: rebase onto main and replace the new local parser with the existing Windows normalization contract, or split out a shared low-dependency utility applied at both validator and bare-path existence seams, with coverage for /c/..., /cygdrive/c/..., /mnt/c/..., and bare-path extraction. #47821 and #57696 are already closed historical duplicates and need no reopening; their useful shared-helper/test approach should inform the revision of #76489.
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
I47767(["issue #47767 (open)"])
P76489["PR #76489 (open)"]
P76489 -.->|partial| I47767
class I47767 open
class P76489 open
class P76489 target
click I47767 "https://github.com/NousResearch/hermes-agent/issues/47767"
click P76489 "https://github.com/NousResearch/hermes-agent/pull/76489"
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 (state tag in the node label).
Cross-PR triage: Reviewed 4 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 8 kB of PR diffs, 12 kB of issue/PR text, 5 kB of discussion (8 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
What does this PR do?
Git Bash rewrites Windows paths into MSYS form, so a command run through it reports files as
/c/Users/me/out.pngrather thanC:\Users\me\out.png. Any file the agent produces that way is then handed tovalidate_media_delivery_pathin MSYS form.Path("/c/Users/me/out.png")on Windows is rooted but driveless, sois_absolute()isFalseand the function returnsNoneat the absolute check. The file exists and is perfectly safe, but delivery is refused. The user sees nothing arrive, and the only trace is a warning:The fix translates MSYS drive paths to native Windows form before the absolute check. It is gated on
sys.platform == "win32", because on POSIX hosts/c/...is a legitimate absolute path and must be left untouched.This is purely a path-normalisation fix ahead of the existing checks. The denylist, allowlist-root, strict-mode and symlink-resolution logic all run afterwards exactly as before, so the security posture is unchanged: a normalised path still has to clear every existing gate.
Related Issue
No existing issue. Searched open and merged PRs and issues for
msys,git bash path,validate_media_delivery_path, andMEDIA path windows drive letterbefore filing. The other open Windows/MSYS PRs (#63012 prompt guidance, #39910 cron, #73782 terminal quoting, #75455 rg/grep) are in different components and do not touch media egress.Type of Change
Changes Made
gateway/platforms/base.py:_MSYS_DRIVE_REand_msys_to_windows_path(), converting/c/Users/...toC:\Users\...(also handles a bare/cand/c/). No-op on non-Windows and for any path that is not an MSYS drive path.validate_media_delivery_path()immediately beforePath(os.path.expanduser(...)).tests/gateway/test_platform_base.py:TestMsysMediaDeliveryPathswith 11 cases: conversion, bare-drive edge cases, already-native paths, POSIX paths left alone, no-op on non-Windows, and a Windows-only end-to-end check thatvalidate_media_delivery_pathnow accepts an MSYS path to a real file.How to Test
Repro on Windows before the fix:
/c/Users/<you>/Downloads/track.mp3.MEDIA:/c/Users/....logs/gateway.logshowsSkipping unsafe MEDIA directive path.After the fix: the same path resolves to
C:\Users\..., clears the existing safety checks, and the file is delivered.Automated:
pytest tests/gateway/test_platform_base.py::TestMsysMediaDeliveryPaths -q # 11 passedChecklist
Code
Note on the suite:
tests/gateway/has 11 pre-existing failures on Windows on an unmodifiedmain(symlink cases needingSeCreateSymbolicLinkPrivilege, plus media-resend-dedup). The identical 11 fail before and after this change; the only delta is the 11 new passing tests (489 to 500 passed on themedia or document or path or deliverysubset).Documentation & Housekeeping