fix(update): avoid cryptography self-lock on Windows - #77517
Conversation
SummaryAll 40 PRs address or reference the Windows update-lock complex, spanning shim quarantine, launcher ancestry, venv and gateway holders, installer recreation, Desktop handoff, false-positive filtering, ZIP preservation, and updater-native-module self-locks. For #73381, #77517 uniquely prevents the updater itself from loading Bitwarden's cryptography Related pull requests
Duplicates#23353/#23408, #29358/#31712/#31806/#31808, #46726/#47569/#47610/#47621/#52044, #57852/#58343, and #74419/#74618/#74707/#75380/#75881/#76057 are overlapping families; the canonical landed implementations are #26677, #35257, #52044, and #75881 respectively. #68821 and #77517 are complementary rather than duplicates: the former hard-stops a still-locked shim, while the latter prevents the updater from mapping cryptography Suggested consolidationKeep #77517 open with the concrete salvage path already present—update-only external-secret suppression, lazy Bitwarden cryptography imports, and full-dispatch regression coverage—because no other diff addresses the updater's own Complex graphflowchart 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
I73381(["issue #73381 (open)"])
P77517["PR #77517 (open)"]
P77517 -->|best fix| I73381
class I73381 open
class P77517 open
class P77517 best
class P77517 target
click I73381 "https://github.com/NousResearch/hermes-agent/issues/73381"
click P77517 "https://github.com/NousResearch/hermes-agent/pull/77517"
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 40 pull requests and 34 issues in this complex. Each diff was read against this issue; Assessment working set: 555 kB of PR diffs, 250 kB of issue/PR text, 146 kB of discussion (144 comments), 212 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
tneemo
left a comment
There was a problem hiding this comment.
Independent verification — reproduced and confirmed on Windows 11
Verified the diff on a real Windows 11 host (the exact platform where the bug bites). Applied the head files + ran the new regression test.
Result: 5/5 tests pass in tests/hermes_cli/test_update_secret_import_lock.py:
test_update_startup_does_not_import_bitwarden_or_cryptography✅test_complete_update_dispatch_does_not_import_cryptography✅test_normal_startup_still_loads_enabled_external_secret_source✅test_dotenv_loading_is_preserved_when_external_secrets_are_skipped[True]✅test_dotenv_loading_is_preserved_when_external_secrets_are_skipped[False]✅
Before/after check on the actual mechanism:
| State | cryptography in sys.modules after startup |
|---|---|
current main |
✅ loaded → maps _rust.pyd into the process (the self-lock) |
| with this diff | ❌ not loaded for the hermes update path; still loaded for normal startup |
Confirmed the root-cause mechanism exactly as described: on current main, importing agent.secret_sources.bitwarden pulls cryptography.hazmat.bindings._rust into the interpreter; with the lazy imports + load_external_secrets=False for the update subcommand, the updater process no longer holds the native image that uv needs to replace.
Design observations:
- The
sys.argv[1:2] != ["update"]gate inmain.pyis correctly placed after profile-flag stripping, sohermes -p profile updatestill routes correctly. - Moving
AESGCM/HKDF/hashesimports inside the cache read/write/derive functions keeps the lazy-load semantic tight — only encrypted-cache ops pay the import. - The new flag default (
load_external_secrets=True) preserves normal startup behavior — no regression for users who rely on Bitwarden env injection.
This is a clean, surgical fix for a nasty Windows-only self-lock. MERGEABLE, CI-ready tests included. Approving the diff as-is.
d6a592d to
80dd64d
Compare
80dd64d to
a5b5928
Compare
|
Rebased this PR onto current Windows validation through the repository-mandated
I also attempted The behavior boundary remains the same after rebase: normal startup still loads configured external secrets, while only the |
|
Hitting this exact bug on a clean system -- no gateway running, no other Hermes processes, just the updater locking its own _rust.pyd. The self-repair loop on every launch is painful. tneemo already verified and approved -- would love a second review to get this merged. Thanks! |
… base interpreter is uv-managed Two gaps left every Windows git-checkout install unable to recover from the exact failure state #83569 reports: 1. Self-lock detection. _detect_venv_python_processes() always excludes the calling process by design — a CLI hermes update IS the venv python. An updater that had already imported a native venv extension (the canonical one being cryptography.hazmat.bindings._rust, mapped while hermes_cli.main resolved external secret sources) passed every preflight and then died mid-sync with os error 5 when uv tried to rewrite the mapped .pyd, stranding the venv half-updated. A new preflight now refuses the sync before touching the checkout, writes the update-incomplete marker so the next fresh launch completes the install, and exits 2. Verified on a live Windows 11 host: after importing hermes_cli.main, tasklist /m _rust.pyd shows the .pyd mapped in the caller, and a peer process cannot open it read-write (Permission denied) — while a rename succeeds, matching how uv/pip actually fail (truncate+write, not rename). 2. Early-recovery install path. _early_recovery._run_repair_install used sys.executable -m pip unconditionally. Windows git checkouts install on a uv-managed base interpreter (python-build-standalone), whose EXTERNALLY-MANAGED marker makes plain pip abort with externally-managed-environment — the repair no-oped and the venv stayed broken. The repair now detects the PEP 668 marker, prefers uv pip install with VIRTUAL_ENV pointed at the project venv, and falls back to pip --break-system-packages when no uv binary exists. Both fixes ship with subprocess/unit regressions (sabotage-verified): the new tests fail on pre-fix code and pass with it. Complements #77517, which keeps the updater from importing cryptography in the first place; this PR is the defence-in-depth when any future path loads it anyway. Fixes #83569
What does this PR do?
Prevents
hermes updatefrom loading Bitwarden's nativecryptographyextension into the updater process before itsuvchild replaces dependencies.On Windows, importing
cryptographymapscryptography/hazmat/bindings/_rust.pyd. The parent updater then holds the exact native image thatuv pip install -e .needs to rename or delete during a cryptography version change, so the update deterministically fails withos error 5and can leave the venv partially uninstalled.The update path still loads the selected profile's
.env, managed environment, and terminal config. It skips only external secret-source fetching, which is not needed for Git/dependency maintenance. Because the full argparse construction also importssecrets_cli, Bitwarden's cryptographic cache primitives are now imported only when an encrypted cache is actually read or written.Related Issue
Fixes #73381
Type of Change
Changes Made
load_external_secretsswitch toload_hermes_dotenv(), defaulting to the existing behavior.updatesubcommand after profile arguments have been resolved.cryptographyprimitives in Bitwarden's encrypted-cache operations so parser construction cannot map_rust.pyd.main()dispatch, plus controls for normal external-source and dotenv behavior.How to Test
Results on Windows 11 / Python 3.12.13:
The full
tests/test_bitwarden_secrets.pyfile has two existing Windows-specific failures unrelated to this patch: its install fixture archivesbwswhile the Windows implementation correctly searches forbws.exe, and its POSIX0o600mode assertion observes0o666on NTFS. Those are not reported as passing here.Checklist
Code
pytest tests/ -qsuite and all tests passDocumentation & Housekeeping
cli-config.yaml.example: N/A; no config key addedCONTRIBUTING.md/AGENTS.md: N/A; no workflow change