fix(install): never strand hermes.exe when a Windows update fails

[hsearcy]

What does this PR do?

On Windows the updater renames the live hermes*.exe shims aside (hermes.exe.old.<unix-ms>) so uv can write replacements. When that quarantine succeeds but the install then fails, two gaps in the recovery path can leave the install with no hermes on PATH at all — which is unrecoverable in place, because the command that would repair it is hermes update.

I hit this on a real install: all three shims were renamed aside at 22:34:33, the install died, nothing put them back, and from that point every hermes update and every press of the desktop app's Update button failed instantly — both shell out to a binary that no longer existed. Recovery required manually renaming the .old files back.

Two defects, both in the recovery path:

1. _restore_quarantined_exes — the safety-critical direction — got a single attempt whose OSError was swallowed in silence, while the outbound quarantine rename gets four attempts with backoff. That is backwards: a failed quarantine merely aborts an update; a failed restore removes hermes from PATH. The same handle that blocks the outbound rename blocks the inbound one.

2. _cleanup_quarantined_exes unconditionally unlink()ed every *.exe.old.* on each hermes invocation. When the original shim was already missing, that .old file was the only surviving copy — deleting it converts a one-rename recovery into a full reinstall. It also races a concurrent in-flight update (the desktop Update button and a shell hermes update hit this), destroying the quarantine that update's own restore was about to rename back.

Scope note (please read before triaging as a duplicate)

This deliberately does not touch _quarantine_running_hermes_exe — that function is byte-identical to main in this branch.

Why the rename fails in the first place (the launcher holding its own image without FILE_SHARE_DELETE) is being handled by #88121, which refuses before mutation and prints the python -m hermes_cli.main update escape hatch. I verified that escape hatch works on my machine — it is what finally completed my update.

This PR is the net underneath that: it covers the other branch of the fork, where quarantine succeeds and the install dies afterwards for an unrelated reason (network, ENOTEMPTY, a kill). #88121 does not touch either function I change here, so the two are disjoint and can merge in either order.

Related Issue

Related to #75584 — specifically the "hermes.exe is gone from venv/Scripts" half of that report. The npm ENOTEMPTY and Playwright failures described there are separate problems and are not addressed here, so I have not marked it Fixes.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• hermes_cli/main.py — _restore_quarantined_exes retries on the same backoff ladder as the quarantine rename and, on persistent failure, prints the literal move command to recover. _cleanup_quarantined_exes rescues orphaned shims instead of deleting them, and honours a 15-minute grace window for quarantines that may belong to a concurrent update.
• hermes_cli/_install_repair.py — same hardening for the early-recovery copy of _restore_quarantined_exes. Its warning goes to stderr, because that module runs in the early path and hermes acp speaks JSON-RPC on stdout.
• tests/hermes_cli/test_quarantine_orphan_rescue.py — 8 new regression tests.

One implementation detail worth flagging

The grace window keys off the .old.<unix-ms> stamp in the filename, not st_mtime. rename preserves the original shim's mtime, which records when uv wrote the shim — days earlier, in general — not when it was quarantined. On my install the stranded files read Aug 17 17:57 for a quarantine that happened at 22:34. An mtime-based check would sweep a live quarantine immediately. test_cleanup_age_comes_from_filename_not_mtime pins this.

How to Test

Reproduce the strand (Windows):

1. Strand a shim the way a failed update does:

   cd $env:LOCALAPPDATA\hermes\hermes-agent\venv\Scripts
   Move-Item hermes-acp.exe "hermes-acp.exe.old.$([DateTimeOffset]::Now.ToUnixTimeMilliseconds())"

2. Run any hermes command, e.g. hermes doctor.
   • Before: the sweep deletes the .old file — the shim is gone for good, reinstall required.
   • After: the shim is renamed back automatically and hermes-acp.exe is present again.
3. Verify the loud-failure path: make the restore rename fail persistently and confirm the update prints ✖ FAILED to restore hermes.exe ... plus the exact move command, instead of exiting silently with the shim renamed aside.

Automated:

pytest tests/hermes_cli/test_quarantine_orphan_rescue.py \
       tests/hermes_cli/test_quarantine_noop_restore.py \
       tests/hermes_cli/test_quarantine_forensic_logging.py \
       tests/hermes_cli/test_update_concurrent_quarantine.py -q

27 passed (19 existing + 8 new). scripts/check-windows-footguns.py is clean on all three files.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate — I fetched all 16 open updater/Windows PR heads and diffed each against its merge-base; none modify _restore_quarantined_exes or _cleanup_quarantined_exes (fix(update): hard-stop when Windows hermes.exe shim stays locked #68821 only calls the former; the function itself is unchanged there)
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run the relevant test suites and all pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Windows 11 (10.0.26200), Python 3.11.15

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstrings on both changed functions explain the failure mode; no user-facing docs affected
• I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact — both changed paths are already _is_windows()-gated; non-Windows behavior is unchanged
• I've updated tool descriptions/schemas if I changed tool behavior — N/A, no model tool behavior changed

Screenshots / Logs

The strand as it happened, from venv/Scripts after a failed update — no hermes.exe, only the quarantined copies:

hermes-acp.exe.old.1787020473886
hermes-agent.exe.old.1787020473888
hermes.exe.old.1787020473885

With this change, the next hermes invocation renames those back instead of deleting them.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

1. hermes_cli/main.py:_cleanup_quarantined_exes (orphan-rescue rename) — the new rescue path executes stale.rename(original) exactly once and swallows OSError. The PR's own diagnosis is that transient AV-scanner locks are what defeat these renames, and the restore direction was just given a five-attempt backoff ladder for precisely that reason — but the rescue, which fires when the shim is already gone from PATH, got neither the ladder nor a diagnostic. Why it matters: an AV handle held for >0s during the one early-startup sweep that runs before the next update silently leaves the user stranded anyway; the title's promise ("never strand hermes.exe") still has an unguarded window. Suggestion: route the rescue through the same backoff_ms ladder and print the same literal move recovery command (to stderr) when it exhausts.

2. hermes_cli/main.py:_cleanup_quarantined_exes ("newest first" ordering) — sorted(glob("*.exe.old.*"), reverse=True) is lexicographic over whole names, so it only picks the true newest while all stamps share digit width. A stray hand-named file (hermes.exe.old.999) or any future stamp-format change sorts above a 13-digit epoch-ms stamp, and the wrong copy gets rescued into place. Why it matters: the rescue writes attacker-/accident-controlled bytes onto the live shim name. Suggestion: sort by parsed stamp (_quarantine_age_seconds's parse, negated) with unparseable names sorted last.

3. hermes_cli/main.py:_quarantine_age_seconds (unparseable → ancient) — a file whose suffix doesn't parse as int-ms is treated as older than the grace window and therefore deleted whenever the original exists. The grace mechanism exists to avoid destroying things this process can't understand; sweeping unrecognized names buys nothing (they aren't produced by the quarantiner) and deletes exactly the files whose provenance is uncertain. Suggestion: return a "not ours" sentinel and continue instead of unlinking.

4. tests/hermes_cli/test_quarantine_orphan_rescue.py — strong coverage of the happy rescue, grace window, filename-vs-mtime, and both restore failure modes. Missing: (a) a transient-lock test for the rescue rename (pins item 1 once implemented), (b) a two-process race test (two sweeps seeing the same orphan; loser must no-op cleanly), and (c) test_cleanup_rescues_newest_orphan_when_several_exist uses two equal-width stamps, which masks item 2 — add a mixed-width case. Why it matters: these are the exact environments the fix targets.

Nit: _restore_quarantined_exes now exists as near-identical copies in main.py and _install_repair.py differing only in stream choice and rename call — they already diverged once (stdout vs stderr). Extract a shared helper taking a rename_fn/stream, so the ladder and recovery-message wording stay in lockstep.

— Reviewed by Hermes AI reviewer (reviewer-f2)

[hsearcy]

Thanks — all four points were right, and I've taken all of them plus the nit. Force-pushed: the branch is rebased onto current main (it was ~1000 commits behind, and _cleanup_quarantined_exes had gained its _cleanup_pending_shim_renames call in the meantime, so the old base no longer applied).

1. Rescue rename had no ladder and no diagnostic — fixed.

You're right that this was the worst instance of the exact bug the PR is about: the rescue fires when the shim is already gone from PATH, so giving up there strands the user just as surely as deleting the file. It now routes through the same helper as the update-time restore, so it gets the ladder and the same recovery message. Covered by test_cleanup_rescue_survives_a_transient_lock and test_cleanup_rescue_reports_when_it_cannot_recover.

2. Lexicographic "newest first" — fixed.

Confirmed: sorted(..., reverse=True) only tracks recency while every stamp shares a digit width. Now sorted by the parsed integer stamp.

One note on the framing: I don't think this is an escalation boundary, since anything able to write hermes.exe.old.999 into venv/Scripts can already write hermes.exe directly. The correctness problem is real regardless — the wrong copy landing on the live shim name is bad on accident alone — so it's fixed either way. Pinned by test_cleanup_rescues_newest_by_parsed_stamp_not_lexicographic, which uses mixed-width stamps (your 4c).

3. Unparseable → ancient → deleted — fixed, and I went further than suggested.

_quarantine_age_seconds is replaced by _quarantine_stamp_ms, which returns None for a name the quarantiner didn't produce. Those files are now skipped entirely — neither deleted nor rescued. Your suggestion was to sort them last for rescue purposes, but rescuing renames a file of unknown provenance onto the live shim name, which is a bigger commitment than deleting it. Treating "not ours" as "don't touch" seemed like the consistent reading of your own point. Two tests pin both halves (test_cleanup_ignores_names_it_did_not_create, test_cleanup_does_not_rescue_from_a_foreign_name).

If you'd rather have a last-resort rescue from a foreign name when the shim is missing and there's no parseable candidate, say so and I'll add it — it's a one-line policy change.

4. Tests — added all three.

• (a) transient-lock on the rescue rename — test_cleanup_rescue_survives_a_transient_lock
• (b) two-process race — test_cleanup_rescue_is_quiet_when_another_process_wins. The loser must be silent, not merely non-crashing: a benign lost race must not print a scary "FAILED to restore". The helper treats "original reappeared" and "quarantine file vanished" as success for exactly this reason.
• (c) mixed-width stamps — as above.

Plus helper-contract tests (returns the failed pairs; never clobbers a fresh shim). 16 tests in the file now, 35 across the four quarantine suites.

Nit: shared helper — done.

It went into hermes_cli/_early_recovery.py rather than a new module or main.py, because _install_repair is deliberately stdlib-only and cannot import main — that constraint is the reason the copies existed. _early_recovery is already imported by both and already documented as the place canonical tables live "so the early and full recovery layers can never drift apart", so it's the existing home for exactly this. All three sites now call _early_recovery.restore_quarantined_shims().

While doing that I settled the stdout/stderr divergence you flagged: everything reports on stderr now. The sweep runs on every hermes invocation, and hermes acp speaks JSON-RPC on stdout, so stdout was never safe for this. test_repair_restore_reports_on_stderr asserts stdout stays empty.

Verification

_quarantine_running_hermes_exe is still byte-identical to main (sha256 of the function body), so this stays disjoint from #88121.

Regression check, patched vs. pristine origin/main over the same ~40-file updater/install/recovery set:

pristine   12 failed, 588 passed, 60 skipped
patched    12 failed, 604 passed, 60 skipped     (+16 = the new tests)

Same twelve failing IDs in both runs, so nothing here regresses — they fail on an unmodified checkout (test_install_cua_driver, test_plugin_install_ref, test_tui_npm_install, plus four that only fail when the whole set shares one process, including test_early_recovery::test_core_marker_triggers_install_before_any_native_import; that one passes in isolation with this patch applied). scripts/check-windows-footguns.py clean on all four files.

[teknium1]

Salvaged and merged in PR #92810 (merge 503d863) — your commit landed with authorship intact, unchanged. This was the strongest PR in the batch: the shared stdlib-only restore ladder, orphan rescue over deletion, the filename-stamp-not-mtime insight, and the .old.999 lexicographic trap were all exactly right, and your 347 lines of tests made the salvage trivial. We added an A/B repro on a real windows-latest runner as final proof: merge-base code STRANDED the shim (sweep deleted the only copy, hermes gone from PATH), your code RESCUED it in the identical scenario on the same runner — plus 30/30 across the quarantine suites on Windows and a Linux real-file E2E. Thanks for an exemplary contribution.