Converged Phase 2 finding (two reviewers independently): _discard_staged
only ran when phase-1 staging failed. A phase-2 (commit) failure rolled the
live tree back correctly but orphaned staging copies for every not-yet-
swapped entry — up to most of a full tree. The retry's up-front free-space
check runs BEFORE the lazy per-entry leftover cleanup, so the litter makes
the retry fail 'not enough free disk space' on exactly the space-constrained
machines the 1.2x threshold was chosen for: the same 'retry fails harder'
failure mode _discard_staged's docstring says it exists to prevent.
Two tests: a behavioral one pinning rollback+discard leaves the old tree
intact with zero litter, and an AST wiring contract on _update_via_zip so a
refactor can't silently drop the cleanup. Mutation-verified: removing the
try/except around _commit_staged_replacements fails the wiring test.
Phase 2 review HIGH (empirically reproduced): a hard kill between
os.rename(dst, backup) and os.rename(staging, dst) leaves dst missing and
the backup as the ONLY copy of that entry. On retry, _stage_replacement
deleted that backup as a 'leftover' BEFORE staging the fresh copy — so a
staging failure (disk exhaustion is likeliest exactly after writing a full
staging copy) left a hole in the install with nothing to roll back to.
Restore the backup to dst first when dst is missing; it's a same-filesystem
rename. Mutation-verified: removing the restore makes the new test fail.
CI slice 8/8 red:
test_verify_core_dependencies.py::test_uses_virtual_env_from_environment
AssertionError: assert None == PosixPath('.../newvenv/Scripts/python.exe')
The Phase 2 reviewer flagged this exact risk (W4) and I under-weighted it as
"latent, not broken". It was neither — it was already failing.
The suite exercises Windows-only paths on Linux CI by patching predicates
(`hermes_cli.main._is_windows`, `is_windows`, `platform.system`). Routing
those call sites through a helper that reads `sys.platform` unconditionally
meant the patches no longer reached the path derivation: the test built
`Scripts/python.exe` while the code looked for `bin/python`.
venv_bin_dir/venv_python_path now take an optional `windows=` verdict,
defaulting to the host. Every converted site passes its own predicate, so
the patched-predicate coverage is restored — the dedup keeps the layout in
one place without hijacking the platform decision.
Verified by causation: dropping `windows=` reproduces the CI failure exactly;
restoring it goes green. Added two regression tests, including one asserting
a patched `_is_windows` still reaches the derivation.
Phase 2 review findings on the first commit.
C1 (critical) — the two-phase replace covered directories only, so the 20
first-party modules at the repo root (run_agent.py, cli.py,
hermes_constants.py, model_tools.py, toolsets.py, ...) were still copied
one-at-a-time with shutil.copy2 straight onto live paths. A failure in that
loop left all directories new and the root modules stale: precisely the
ImportError shape this PR exists to prevent. Worse, copy2 truncates in place,
so a crash mid-copy could leave a half-written cli.py — strictly worse than
stale on the flaky-AV path this code runs on.
Stage files the same way as directories and swap them in the same commit
phase. The docstring's "wholly new or wholly old" is now actually true.
C2 (critical) — a phase-1 failure (disk exhaustion being the likely one)
orphaned one staging copy per entry already processed, up to a second copy
of the tree. The user then follows our "re-run hermes update" advice with
LESS free space and the retry fails harder. Added _discard_staged() on the
staging path. Verified: staging failure now leaves zero litter.
W1 — _stage_replacement duplicated _atomic_replace_dir's first half verbatim.
_atomic_replace_dir is now a 1-line shim over the two-phase helpers; its
#49145 regression test still passes.
W2 — the failure message still said "some directories were replaced and
others were not", which the fix makes false. Now says the install was left
in place.
W3 — the free-space gate demanded 2x the tree when only the staging copy is
new (the live tree already occupies its space; swaps are renames). Relaxed
to need * 1.2, so we stop blocking updates that would have succeeded on the
space-constrained machines most likely to hit this.
W5/W6 — the lint-style guard used `"if" in line`, which matches "modify" and
"verify" and still missed os.path.join(venv, "Scripts"). Rewritten as an AST
check; it immediately found the real offender the substring version missed
(stdio.py, now explicitly exempted — it lists literal Windows-only PATH
candidates, not a cross-platform derivation). Softened venv_bin_dir's
"single source of truth" claim, since sites outside hermes_cli/ remain.
S1 — the rollback loop now logs instead of silently swallowing OSError.
Both C1 and C2 fixes are mutation-verified: reverting either makes the new
tests fail.
Closes#76104, closes#76105.
#76104 — `_atomic_replace_dir` (#49145) made each individual directory swap
safe, but `_update_via_zip` replaced ~70 top-level entries in a loop with no
atomicity across iterations. `agent/` lands at os.listdir index 13 and
`tools/` at 66, so an interruption between them left the new
`agent/context_compressor.py` (module-level `from tools.todo_tool import
TODO_INJECTION_HEADER`) beside a stale `tools/todo_tool.py` — every file
valid Python, the tree unbootable. That is the mechanism behind the
ImportError fixed in #76091, and the "partial update" field report in #63717.
Split into stage-all-then-swap-all:
- `_stage_replacement` copies each dir to a sibling staging path, touching
nothing live, so a failure during the long copy phase is a no-op.
- `_commit_staged_replacements` performs the renames and, if any fails,
restores every entry already swapped — the tree lands wholly new or
wholly old, never mixed.
This shrinks the failure window from a full tree copy to N renames and makes
what remains recoverable. Added an up-front free-space check, since staging
needs a second copy of the tree; a clear error beats running out mid-swap.
#76105 — venv interpreter resolution was open-coded in 7 places across 4
files using 3 different Windows predicates. #76091 added the seventh because
the correct behaviour lived 2400 lines away. Hoisted `venv_bin_dir()` /
`venv_python_path()` into hermes_constants (import-safe, no new imports) and
routed every site through them; `managed_uv._venv_python` now delegates so
its 6 callers are untouched.
`_atomic_replace_dir` is retained — it is re-exported from main.py and has
its own #49145 regression test; removing it is out of scope here.
Tests: 10 new (rollback-on-mid-swap-failure is mutation-verified — it fails
when the rollback loop is removed), plus a guard that fails if a new call
site hand-rolls Scripts/bin again. E2E-verified against the real staging +
commit helpers with a live tree.
kshitij