Skip to content

fix(desktop): surface external venv update blockers - #63318

Closed
AtakanGs wants to merge 1 commit into
NousResearch:mainfrom
AtakanGs:fix-desktop-external-backend-update-blocker
Closed

AtakanGs wants to merge 1 commit into
NousResearch:mainfrom
AtakanGs:fix-desktop-external-backend-update-blocker

Conversation

@AtakanGs

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents the Windows Desktop updater from silently handing off to a detached updater when another process is still using the Hermes installation venv.

The existing Desktop update flow releases only the backend processes owned by the current Desktop instance. An externally started backend, gateway, CLI session, or another venv Python process can remain alive. The Python updater detects these holders and refuses to continue, but because the staged updater is launched with detached stdio: ignore, its diagnostic output is not visible in the Desktop UI.

This PR adds a Windows-only preflight after Desktop-owned backend teardown and before updater handoff:

  • runs a small structured Python venv-holder scan
  • validates the JSON result strictly
  • aborts the handoff when remaining holders are found
  • reports PID, process name, and sanitized command line
  • warns that stopping a network-bound backend may disconnect remote clients
  • fails closed when the probe cannot verify that the venv is free
  • restores or reconnects the Desktop backend after an aborted update

External processes are not automatically terminated. The user remains responsible for stopping the owning terminal, app, or service and retrying the update.

Sensitive command-line values, including API credentials and internal session keys, are redacted before being shown.

Related Issue

Fixes #63206

Testing

  • .\venv\Scripts\python.exe -m pytest tests/hermes_cli/test_scan_venv_blockers.py -q
    • 13 passed
  • .\venv\Scripts\python.exe -m pytest tests/hermes_cli/test_update_venv_health.py -q -k "detect_venv_python or format_venv_holders or venv_holder_guard"
    • 10 passed, 8 deselected
  • .\venv\Scripts\python.exe -m pytest tests/hermes_cli/test_update_concurrent_quarantine.py -q
    • 25 passed
  • npx --no-install tsx --test electron/venv-blocker-scan.test.ts
    • 20 passed
  • npm run typecheck
  • ESLint passed for the two new Electron files
  • Ruff passed for the two new Python files
  • git diff --cached --check
  • Manual Windows probe confirmed live venv holders are reported and --session-key values are redacted

Scope

This PR intentionally does not:

  • automatically kill external processes
  • restart an externally managed backend
  • manage the full remote-client stop/update/restart lifecycle
  • change non-Windows update behavior

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows P2 Medium — degraded but workaround exists labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for addressing the updater handoff gap. The premise remains valid on current main: Desktop launches the updater with detached ignored stdio at apps/desktop/electron/main.ts:2516-2527, while the updater rejects venv-Python holders at hermes_cli/main.py:9594-9609. The proposed preflight directly surfaces that otherwise invisible refusal before Desktop exits, while leaving externally managed processes under user control.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 16, 2026
@AtakanGs
AtakanGs force-pushed the fix-desktop-external-backend-update-blocker branch from 88f0cee to a3c8e44 Compare July 18, 2026 00:40
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Salvaged and merged via #73569 (commit 03af12c) — your commit cherry-picked with authorship preserved, plus three follow-ups on top from review/CI:

  1. Converted the preflight's execFileSync to async execFile — the sync psutil scan (up to 15s) blocked the Electron main-process event loop.
  2. Moved the test suite from node:test to vitest so CI's electron project actually collects it.
  3. Import-group spacing lint fix.

The preflight design was right where it counts: reusing the CLI's canonical _detect_venv_python_processes detector, strict JSON contract, and fail-closed on every probe-failure mode. Thanks for the thorough test coverage on both sides of the boundary.

Closing this original in favor of the salvage PR.

@teknium1 teknium1 closed this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage 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-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] Network-bound hermes serve used by remote Desktop blocks in-app update without actionable recovery

3 participants