Fix desktop updater runtime cleanup

[grimmjoww]

Summary

Fix the Windows desktop updater preflight so it actually finds and stops Hermes-owned runtime processes before handing off to the staged updater.

The desktop update path already tries to release backend/runtime locks before launching hermes-setup.exe --update, but the Windows process scan built an invalid PowerShell command ($ErrorActionPreference = 'Stop' Get-CimInstance ...). That parse error was swallowed and returned an empty process list, so stray venv/gateway processes could keep .pyd/venv files locked and make uv pip install fail with Access is denied.

This PR:

• fixes the Win32 process-list command so process cleanup can actually see candidates
• keeps the cleanup matcher constrained to Hermes-owned runtimes under the target install/root
• adds a Windows regression test that requires listWindowsProcesses() to include the current Node process
• adds an explicit Windows smoke command, npm run smoke:desktop:update-handoff, that uses a disposable HERMES_HOME, a fake locked venv runtime, a staged fake updater, and the packaged desktop app to prove handoff/relaunch behavior end to end

Validation

• node --test electron/update-processes.test.cjs -> 8 pass
• npm run test:desktop:platforms -> 87 pass
• npx eslint electron/main.cjs electron/update-processes.cjs electron/update-processes.test.cjs electron/update-handoff-smoke.cjs --max-warnings=0
• git diff --check origin/main..HEAD; git diff --check
• .\venv\Scripts\python.exe -m hermes_cli.main desktop --build-only
• npm run smoke:desktop:update-handoff

Smoke evidence from the latest run:

• desktop log recorded stopped 1 Hermes runtime process(es) before update
• desktop log recorded venv shim unlocked; safe to hand off the update
• fake updater received --update --branch main
• fake updater relaunched Hermes from the packaged app
• no disposable smoke-root Hermes processes remained afterward

Notes

The terminal-command fallback remains intentional for source/CLI installs that do not have HERMES_HOME/hermes-setup.exe staged. Installer-backed desktop installs should take the automatic staged-updater handoff path.

[alpindiay]

Code Review: Fix desktop updater runtime cleanup

Summary

Adds update-processes.cjs to detect and kill Hermes runtime processes before an update, plus a Windows smoke test (update-handoff-smoke.cjs) that validates the full update handoff flow including process cleanup, updater launch, and relaunch.

Issues Found

1. Incomplete process detection coverage (medium)
commandLooksHermesOwned only matches three entrypoints:

hermes_cli.main
hermes_cli.gateway
scripts/whatsapp-bridge

Missing: hermes_cli.cron (scheduler), plugins that import hermes_cli, any user script invoking hermes_cli.*, and the desktop bootstrap itself. Consider broadening to match on hermes_cli. prefix or adding additional patterns. If a cron daemon or plugin worker is running in the venv, it will survive the update and potentially lock files.

2. killHermesRuntimeProcessesForUpdate silently no-ops without killTree (low)
If called without the killTree option, the function collects PIDs but never kills anything. The return value still reports the PIDs as if they were handled. In the current call site (main.cjs) this is fine because forceKillProcessTree is always passed, but the exported API is misleading. Consider throwing or logging a warning when killTree is missing.

3. listWindowsProcesses swallows all errors (low)
If PowerShell fails (e.g., not installed, permissions issue), the function silently returns []. This means the updater proceeds believing no runtime processes exist when in reality it just can not enumerate them. Consider logging the failure or returning a distinct sentinel (e.g., null) to signal "unknown" vs "empty."

4. Smoke test: verbose setup, fragile WebSocket dep (low)
The smoke test compiles a .NET fake updater and uses CDP via WebSocket. This requires dotnet SDK and Node 21+ (global WebSocket). The check typeof WebSocket !== "function" throws an error; consider skipping gracefully with a diagnostic message instead. Also DOTNET_CLI_TELEMETRY_OPTOUT is misspelled (OPTOUT vs OPTOUT) — should be DOTNET_CLI_TELEMETRY_OPTOUT (though the standard env var is actually DOTNET_CLI_TELEMETRY_OPTOUT).

What Looks Good

• Thorough process matching: venv path prefix checks + sibling checkout exclusion
• currentPid exclusion prevents self-kill
• Well-structured path normalization with normalizePathText handling both backslash and forward slash
• Comprehensive unit tests in update-processes.test.cjs: targets, false positives, sibling exclusion, parsing, collection
• Smoke test validates the full end-to-end flow: lock detection, updater args, relaunch, log assertions
• forceKillProcessTree is the production kill mechanism (tree kill, not single PID)
• Cleanup in smoke test catch block handles teardown properly

[tonydwb]

Code Review Summary

Verdict: Approved

What this PR does

Refactors the desktop updater's pre-update process-killing logic. Instead of scattered taskkill invocations, a new killHermesRuntimeProcessesForUpdate() routine in update-processes.cjs identifies Hermes-owned runtime processes (venv python, node, npm, uv, etc.) by path + command-line heuristics and kills them before the update proceeds. A new smoke-test (update-handoff-smoke.cjs) exercises the handoff logic on Windows; main.cjs wires the new routine into the existing releaseBackendLockForUpdate path.

Looks Good

• Well-scoped concern: process-detection is now unit-testable, file-match helper (isUnderPath) is deterministic.
• Terrific Windows context awareness: handles both forward and backslash directory separators, Win32_Process JSON parsing with ProcessId/ExecutablePath/CommandLine casing variants, maxBuffer: 16MB for dense process dumps, and windowsHide: true.
• Defensive try/catch + no hard failures if PowerShell listing returns nothing.
• New RUNTIME_PROCESS_NAMES Set is a good focal point for future additions.
• update-handoff-smoke.cjs is valuable — the postMessage handshake pattern cleanly verifies child-process lifecycle with the parent Electron process.

Minor notes (non-blocking)

• execFileSync('powershell.exe', [...], { timeout: 5000 }) will throw synchronously if PowerShell is slow (>5s) on very loaded machines. An async spawn + timeout path is more robust in edge cases, but the existing try/catch already degrades gracefully.
• commandLooksHermesOwned hardcodes whatsapp-bridge — that's fine but worth documenting why it's included (it runs alongside the runtime but outside the venv).

---

Reviewed by Hermes Agent

[grimmjoww]

Addressed the review follow-ups in the amended commit b543e40ee:

• Broadened Hermes-owned command matching to hermes_cli.*, still gated by process name and target checkout path.
• Added coverage for a non-venv python -m hermes_cli.cron --root <checkout> process.
• Made killHermesRuntimeProcessesForUpdate() throw if matching runtime PIDs are found without a killTree callback.
• Added non-silent process enumeration failure reporting through onListError, wired into the desktop update log.
• Made the update handoff smoke skip cleanly when optional prerequisites are missing (global WebSocket or dotnet).
• Left DOTNET_CLI_TELEMETRY_OPTOUT unchanged; the env var was already spelled correctly.

Verification run on Windows:

• node --test electron/update-processes.test.cjs
• npm run test:desktop:platforms from apps/desktop
• npx eslint electron/main.cjs electron/update-processes.cjs electron/update-processes.test.cjs electron/update-handoff-smoke.cjs --max-warnings=0
• git diff --check
• .\venv\Scripts\python.exe -m hermes_cli.main desktop --build-only
• npm run smoke:desktop:update-handoff
• Extra skip checks for missing global WebSocket and missing dotnet

Smoke confirmed runtime stopped, venv shim unlocked, updater launched, relaunch happened, and no smoke-root processes remained.

[grimmjoww]

Rebased this PR onto current origin/main after #40409 landed.

New head: b9960c2d2
Base: ebed881d (fix(cli): quarantine running hermes.exe during update dep-verification repair on Windows)

No rebase conflicts. Verification rerun on Windows from a clean PR worktree:

• node --test electron/update-processes.test.cjs
• npm run test:desktop:platforms from apps/desktop (90/90 pass)
• npx eslint electron/main.cjs electron/update-processes.cjs electron/update-processes.test.cjs electron/update-handoff-smoke.cjs --max-warnings=0
• git diff --check
• npm run pack from apps/desktop
• npm run smoke:desktop:update-handoff

Smoke again confirmed runtime stopped, venv shim unlocked, updater launched with --update --branch main, relaunch happened, and no smoke-root processes remained.

[grimmjoww]

Rebased this PR onto current origin/main again.

New head: b152dbe33
Base: 846821d8 (Merge pull request #40684 from NousResearch/bb/cron-sessions-sidebar)

Conflict resolved in apps/desktop/package.json by keeping both the upstream desktop-uninstall.test.cjs platform-suite entry and this PR's update-processes.test.cjs entry. Also updated the smoke assertion to match the current source log text (venv shim unlocked; safe to proceed).

Fresh Windows verification from the PR branch:

• git diff --check
• node --test electron/update-processes.test.cjs
• npm run test:desktop:platforms from apps/desktop (109/109 pass)
• npx eslint electron/main.cjs electron/update-processes.cjs electron/update-processes.test.cjs electron/update-handoff-smoke.cjs --max-warnings=0
• npm run pack from apps/desktop
• npm run smoke:desktop:update-handoff

Smoke confirmed runtime process cleanup, venv shim unlock, updater launch with --update --branch main, desktop relaunch, and no smoke-root processes left behind.

[grimmjoww]

Review follow-up pushed.

What changed since the last review pass:

• Broadened Hermes-owned runtime process detection to hermes_cli.* while still requiring the process to be a known runtime executable and tied to the target checkout.
• Made update process cleanup fail loudly if matching PIDs are found without a kill-tree callback.
• Routed process-list failures through logging instead of silently treating them as no processes.
• Forced desktop update commands through the backup path.
• Windows desktop update handoff now uses a visible command window wrapper for the staged updater.
• Hardened the update-handoff smoke so it models an already-installed Hermes root, verifies the runtime lock is released, tolerates the expected renderer shutdown race during handoff, and skips clearly when optional prerequisites are missing.

Fresh validation on Windows:

• node --test electron/update-handoff.test.cjs electron/update-processes.test.cjs: 16/16 passing.
• npm run test:desktop:platforms: 114/114 passing.
• npx eslint electron/main.cjs electron/update-processes.cjs electron/update-processes.test.cjs electron/update-handoff.cjs electron/update-handoff.test.cjs electron/update-handoff-smoke.cjs --max-warnings=0: passing.
• cargo test desktop_update_args_force_pre_update_backup: passing.
• git diff --check main...HEAD: passing.
• .\venv\Scripts\python.exe -m hermes_cli.main desktop --build-only: passing.
• npm run smoke:desktop:update-handoff: passing.

Smoke proof from the latest run:

• stopped 1 Hermes runtime process(es) before update
• venv shim unlocked; safe to proceed
• launched updater: cmd.exe /d /s /c ...hermes-updater-...cmd
• No uv installation failed and no fake backend exited before it became ready noise in the passing smoke log.

Live Windows proof from the local install:

• Rolled the local checkout back one commit, launched desktop, used the in-app version/update flow, and updated back to origin/main twice.
• The staged updater window ran the live update flow and relaunched Hermes.
• Pre-update backups were created under G:\hermes\backups\.
• Final live state reported Hermes Agent v0.16.0 at upstream 3289d4ad and up to date.

One note from live testing: the first live attempt exposed an unrelated external file lock from a PyCharm Tailwind helper holding @tailwindcss/oxide. Closing that external lock holder let the updater complete. This PR intentionally keeps Hermes process cleanup scoped to Hermes-owned runtime processes rather than killing unrelated editor/helper processes.

[austinpickett]

Hermes Agent Review — ✅ Approve

A large (+1281) but cohesive, well-tested PR. Verified novel vs origin/main (both new .cjs modules absent on main), checked out locally and ran the full new test surface.

What it does

Hardens the desktop→CLI update handoff with three additions, all cleanly extracted into testable modules:

1. update-processes.cjs — before an update overwrites files, enumerate and kill Hermes-owned runtime processes (node/python/uv/npm under the install or venv root) so Windows file locks don't fail the update. The match heuristic is appropriately conservative: a process-name allowlist AND (executable under updateRoot/venv OR commandLine mentions updateRoot AND looks Hermes-owned), and it excludes the current PID. killTree is injected (testable) and the function throws if matches exist but no killTree is supplied — fail-safe rather than silently skipping. Path normalization handles Windows backslash/case correctly.

2. update-handoff.cjs — on Windows, instead of a silent detached spawn, it writes a visible .cmd updater (progress echoes, pause-on-failure so users see errors) into HERMES_HOME/logs, with correct batch quoting (%%, ""). Centralizes --backup/--branch arg building shared by the GUI button and the manual-command card. POSIX path unchanged (plain detached spawn).

3. update.rs (Tauri installer) — refactors the inline arg vec into a testable build_update_args() and adds --backup so a desktop update (which replaces the app underneath the user) always creates a restore point — matching the JS side. --force/--branch rationale comments preserved; new Rust unit test added.

Verification

node --test apps/desktop/electron/update-processes.test.cjs  =>  10 pass, 1 skip, 0 fail
node --test apps/desktop/electron/update-handoff.test.cjs    =>   5 pass, 0 fail
node --check apps/desktop/electron/main.cjs                  =>  OK

The 1 skipped test is the PowerShell Get-CimInstance integration case (correctly platform-gated to Windows — not broken). Both unit suites are registered in test:desktop:platforms; the 626-line update-handoff-smoke.cjs is wired as a separate smoke:desktop:update-handoff script, so it doesn't bloat the normal test run — good separation. (Did not compile the Rust test here — no Rust toolchain in this env — but build_update_args + its assertion are straightforward and self-evidently correct.)

main.cjs wiring

Clean: killHermesRuntimeProcessesForUpdate is called in releaseBackendLock with injected forceKillProcessTree + rememberLog callbacks for list/kill errors; createUpdaterLaunchPlan replaces the inline spawn and the result drives spawn(launch.command, launch.args, {detached: launch.detached, windowsHide: launch.windowsHide}). Behavior preserved on POSIX.

Cluster note: part of the desktop module-extraction family — will need a trivial rebase against whichever sibling (#37471/#39554/#38292/#38589/#42901) lands first (textual conflicts on main.cjs imports + the test:desktop:platforms line), no semantic overlap.

Reviewed by Hermes Agent (local node --test on both suites + node --check; verified vs origin/main).

[grimmjoww]

@austinpickett I rebased this onto current main and resolved the conflicts in apps/bootstrap-installer/src-tauri/src/update.rs and apps/desktop/package.json (kept both test suites + the build_update_args --backup change; cargo check passes clean). Since you'd approved the earlier head, would you mind a quick re-review of the rebased version? And if a maintainer could authorize the workflow run so CI reports, that'd unblock the merge. Thanks!

[grimmjoww]

Repair update after rebasing against current main:

• Merged current main; GitHub now reports this PR as mergeable.
• Resolved the apps/desktop/package.json conflict by preserving current main's expanded desktop platform test list and adding this PR's update handoff/process tests plus the smoke command.

Local QA on Windows:

• node --test electron/update-processes.test.cjs electron/update-handoff.test.cjs — 16 passed.
• npm run typecheck from apps/desktop — passed.
• git diff --check origin/main...HEAD — clean.

I also re-read CONTRIBUTING.md: this stays scoped to desktop updater runtime cleanup/handoff behavior, includes targeted tests, and considers the Windows update-process path explicitly. GitHub still reports no checks on the branch, so CI may need maintainer authorization.

[teknium1]

Thanks for the focused Windows updater coverage. The current desktop still only kills backend PIDs it owns (apps/desktop/electron/main.ts:2410-2436), so the process-discovery idea addresses a real remaining gap.

Problems

• apps/desktop/electron/update-processes.cjs:79 treats every allowlisted runtime executable under the target checkout or its venv as Hermes-owned, even without a Hermes command line. That can kill a user terminal running venv\\Scripts\\python.exe script.py or another local task. Current main deliberately aborts rather than killing an outside holder (apps/desktop/electron/main.ts:2469-2480).
• The PR targets removed CJS Electron surfaces. Main migrated main.cjs and the platform tests to TypeScript in 39d09453f; this needs a TypeScript port, not a direct merge.

Suggested changes

• Require a Hermes-specific command signature for generic Python/Node/uv processes, with tests for non-Hermes commands under the target venv.
• Port the helpers, wiring, and tests to the current TypeScript Electron layout.

Automated hermes-sweeper review.

[teknium1]

This makes any allowlisted Python/Node executable under the install or venv eligible even if it is running a non-Hermes user command. Please require a Hermes-specific command signature for generic runtimes (and add a negative test); current main intentionally aborts rather than killing an outside venv holder.