Skip to content

fix(windows): unmangle MSYS Hermes paths - #39910

Open
yinkev wants to merge 3 commits into
NousResearch:mainfrom
yinkev:fix-windows-msys-profile-paths
Open

fix(windows): unmangle MSYS Hermes paths#39910
yinkev wants to merge 3 commits into
NousResearch:mainfrom
yinkev:fix-windows-msys-profile-paths

Conversation

@yinkev

@yinkev yinkev commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a shared native-Windows normalizer (normalize_windows_msys_path) that translates the unambiguous MSYS/git-bash drive forms (/c/..., /cygdrive/c/..., /mnt/c/...) to native C:\..., and a canonicalize_hermes_path helper that produces a stable path without Windows Path.resolve()/os.path.abspath().
  • A legitimate native C:\c\work is preserved byte-for-byte — it cannot be structurally distinguished from a mangled path, so it is never rewritten. The "already-mangled C:\c\..." rotating-log case is handled by preventing the mangle (restoring the caller's requested path after abspath), not by guessing at an un-mangle.
  • Route HERMES_HOME, %LOCALAPPDATA% fallback, profile-root comparison, the cron jobs/suggestions/executions stores, gateway PID/lock/status identity, and rotating-log path identity through the safe handling.
  • POSIX behavior (where CI runs) is unchanged: the normalizer is a no-op passthrough off-Windows.

Fixes #39834.

Verification

  • .venv/bin/python -m py_compile hermes_constants.py hermes_logging.py cron/jobs.py cron/suggestions.py cron/executions.py gateway/status.py
  • .venv/bin/python -m pytest tests/test_hermes_constants.py tests/test_hermes_logging.py tests/cron/test_jobs.py tests/cron/test_suggestions.py tests/cron/test_execution_ledger.py tests/gateway/test_status.py -q -o addopts= → 531 passed
  • ruff check on all touched files → clean
  • python scripts/check-windows-footguns.py --all → no footguns (809 files)
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Jun 5, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing a real Windows/MSYS failure. The premise is still present on current main: hermes_constants.py:75-77 returns raw HERMES_HOME, cron/jobs.py:66 resolves it, and hermes_logging.py:460-465 resolves logging path identities.

Problems

  • In PR commit 517eb5973439, the new normalize_windows_msys_path() strips a first tail component matching the drive letter. That also rewrites a legitimate native path such as C:\\c\\work to C:\\work; the normalizer needs a discriminator that does not corrupt valid paths.
  • The patch predates current path owners and leaves sibling routes unhandled: gateway/status.py:60-63 reads HERMES_HOME directly for PID/lock/status identity, while cron/jobs.py:129 and cron/suggestions.py:48 still resolve Hermes-home-derived paths.

Suggested changes

  • Add a valid C:\\c\\... regression case and narrow the correction accordingly.
  • Audit the current gateway-identity and dynamic cron-store call paths and apply the shared safe handling there as needed.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@yinkev
yinkev force-pushed the fix-windows-msys-profile-paths branch 2 times, most recently from f27c863 to 5a19ce6 Compare July 25, 2026 15:31
@yinkev

yinkev commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both problems from the hermes-sweeper review, plus one additional sibling route it pointed at. Rebased onto current main and force-pushed; head is now 5a19ce64a.

P1 — normalizer no longer corrupts valid paths

The old normalize_windows_msys_path() stripped a first tail component equal to the drive letter, which rewrote a legitimate C:\c\workC:\work. That heuristic is gone. The normalizer now translates only the unambiguous MSYS POSIX drive forms (/c/…, /cygdrive/c/…, /mnt/c/…) and preserves native C:\c\… byte-for-byte — there is no structural way to tell a mangled C:\c\… from a real directory named c, so it is never rewritten.

The "already-mangled C:\c\…" case (rotating logs) is now handled by prevention, not guessing: _ManagedRotatingFileHandler rebuilds baseFilename from the caller's originally requested path after os.path.abspath() runs, instead of trying to un-mangle after the fact.

Regression added on every surface asserting C:\c\work… round-trips unchanged (e.g. test_preserves_legit_duplicate_drive_segment_on_windows, test_windows_hermes_home_preserves_legit_duplicate_drive_segment).

P2 — sibling identity / store routes

Added a shared canonicalize_hermes_path() helper (byte-identical to the prior resolve() behavior on POSIX; on Windows it avoids Path.resolve()/abspath, which re-introduce the C:\c\… mangle) and routed the flagged surfaces through the safe handling:

  • gateway/status.py_get_process_hermes_home() normalizes the raw env; _canonical_hermes_home() (used for PID/lock/runtime-status identity and takeover markers) delegates to the helper.
  • cron/jobs.pyuse_cron_store() (the dynamic per-profile store).
  • cron/suggestions.pyCRON_DIR.
  • cron/executions.pyEXECUTIONS_FILE (the durable execution ledger). Same get_hermes_home().resolve() / "cron" / … pattern in the same package; left untouched it would split the ledger away from the co-located jobs.json/suggestions.json under exactly the MSYS environments this PR targets.

Also made get_default_hermes_root() profile detection use ntpath string ops so a native C:\…\profiles\name is parsed correctly (PurePosixPath.parent silently misses the segment in a backslash path).

Out of scope for this PR (different semantics — noted for a follow-up): agent/context_references.py and hermes_cli/gateway.py still call .resolve(), but those are symlink-aware security-containment checks and CLI diagnostics, not Hermes-home identity keying.

Verification

.venv/bin/python -m py_compile hermes_constants.py hermes_logging.py cron/jobs.py \
  cron/suggestions.py cron/executions.py gateway/status.py

.venv/bin/python -m pytest tests/test_hermes_constants.py tests/test_hermes_logging.py \
  tests/cron/test_jobs.py tests/cron/test_suggestions.py tests/cron/test_execution_ledger.py \
  tests/gateway/test_status.py -q -o addopts=
# -> 531 passed

ruff check <touched files>                       # All checks passed!
python scripts/check-windows-footguns.py --all   # No footguns (809 files) — CI blocking gate
ty check <touched source>                         # no new diagnostics on changed lines (advisory)

Behavior on POSIX (where CI runs) is unchanged: normalize_windows_msys_path is a no-op passthrough off-Windows, so every helper reduces to its prior Path(...).resolve() expression.

@yinkev
yinkev force-pushed the fix-windows-msys-profile-paths branch from 5a19ce6 to 86e529d Compare July 25, 2026 16:19
@yinkev

yinkev commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up: fixed the sole failing check from run 30163779963 and force-pushed head 86e529d62.

Failure

tests/hermes_cli/test_web_server.py::TestNewEndpoints::test_profile_open_terminal_uses_windows_cmd switched sys.platform to win32 while retaining the Linux runner's absolute POSIX HERMES_HOME. get_default_hermes_root() then fed that foreign POSIX path through ntpath, changing /tmp/.../.hermes into a relative \\tmp\\...\\.hermes path. The profile lookup returned 404.

This was a real cross-platform simulation regression introduced by the Windows branch in the root resolver, not a flaky test.

Fix

When the active Path flavour reports an absolute, drive-less path, the resolver preserves that path and performs profile-parent detection with the host Path semantics. A real Windows WindowsPath('/tmp') is drive-relative rather than absolute, so this branch is naturally limited to foreign/simulated POSIX paths. Native Windows paths still use ntpath; /c/... MSYS drive mounts still normalize to C:\\...; legitimate C:\\c\\... paths remain untouched.

Regression and verification

  • Added test_windows_platform_simulation_preserves_posix_profile_root (red before the fix, green after).
  • Exact CI failure + new unit regression: 2 passed.
  • Canonical per-file runner over constants, logging, cron jobs/suggestions/execution ledger, gateway status, and the complete web-server file: 1,048 passed.
  • Ruff clean; py_compile clean; git diff --check clean.
  • Windows-footgun scan: 809 files, no findings.

New CI is running on 86e529d62.

…\c\...

Native CPython launched from git-bash/MSYS can receive Hermes paths as
/c/..., /cygdrive/c/..., or /mnt/c/..., which Path.resolve()/os.path.abspath()
then bind to the current drive (C:\c\...), creating a second Hermes tree for
HERMES_HOME, the cron stores, gateway identity files, and rotating logs.

- normalize_windows_msys_path(): translate ONLY the unambiguous MSYS POSIX
  drive forms; preserve a legitimate native C:\c\work byte-for-byte. The
  earlier heuristic that stripped a drive-letter tail component corrupted
  valid paths (maintainer review P1).
- canonicalize_hermes_path(): MSYS-safe stable-path helper that avoids
  Windows Path.resolve()/abspath; byte-identical to the prior resolve()
  behavior on POSIX.
- Route HERMES_HOME, %LOCALAPPDATA% fallback, profile-root comparison, the
  cron jobs/suggestions/executions stores, gateway PID/lock/status identity,
  and rotating-log path identity through the safe handling (review P2). The
  rotating-log handler restores the caller's requested path after abspath
  rather than guessing at an un-mangle of the ambiguous C:\c\... form.
- get_default_hermes_root() profile detection uses ntpath string ops so a
  native C:\...\profiles\name path is parsed correctly.

Adds regressions across every surface asserting C:\c\... is preserved and raw
MSYS forms are translated.

Fixes NousResearch#39834.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management 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-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] MSYS2 path mangling creates phantom C:\c directory — breaks logging, cron, and sessions

3 participants