fix(hermes_cli): fail-closed PID-ownership guard before Windows taskkill

[gebilaowang404]

Summary

Windows PIDs are recycled aggressively after a reboot. Hermes persists process
PIDs across boots (state files, process lists) and kills them with bare
taskkill /PID <n> /F. When the recycled number lands on a protected system
process (svchost.exe), the kill triggers bugcheck 0xEF
(CRITICAL_PROCESS_DIED) — 8 occurrences in 48h on the reporting machine
(#89614).

This PR adds a fail-closed PID-ownership probe (pid_is_hermes()) and guards
the three Python kill boundaries:

• hermes_cli/_subprocess_compat.py → kill_process_tree (guarded via pid_is_hermes)
• hermes_cli/dashboard_procs.py → _kill_stale_dashboard_processes (win32 branch)
• hermes_cli/update_cmd.py → _stop_process_trees

Probe

pid_is_hermes(pid) queries Get-CimInstance Win32_Process and matches
CommandLine/ExecutablePath against 'hermes'. Every failure mode —
missing process, blank output, timeout, OSError — fails closed (no
taskkill). Non-Windows callers have no taskkill path and pass through.

Acceptance (#90471)

1. missing / unreadable / non-matching identity fails closed — no taskkill ✅
2. a recycled or foreign PID control process remains untouched ✅
3. probe failure or timeout is never converted into permission to kill ✅
4. call sites classified as owned-process teardown (children spawned by
   Hermes), not a foreign-kill policy ✅
5. Refs #90471 / Refs #89614, no class-closure claim ✅

Tests

tests/hermes_cli/test_stale_pid_guard.py — 16 tests covering the probe
(foreign reject, blank/timeout/OSError fail-closed, real missing-PID smoke on
Windows) and all three kill boundaries (foreign PID probed-but-not-killed,
Hermes PID still killed). All pass on Windows; the only platform-dependent
test is skipif-guarded.

Provenance & scope

The guard has run continuously on the reporter's machine since 2026-08-18 with
zero false kills. It is stage-1 containment only: an ABA (generation A
exits, generation B reuses the PID) remains theoretically possible until the
retained-authority / incarnation-proof architecture of #90250 / #90144 /
#90145 lands; gateway/status.py and tools/process_registry.py remain
explicit follow-ons.

Refs #90471, #89614

[jackulau]

Your diagnosis on #89614 is the best-evidenced Windows report I've read in this repo, and the Python-side call sites are genuinely uncovered by the two desktop PRs, so this PR is filling a real gap. I checked it out and measured it on Windows 11 26200 rather than eyeballing it. Everything below is reproducible.

The headline: the guard currently fails open on exactly the malformed PIDs it should refuse, and the repo already ships a mechanism that is both stronger and ~24,000x faster.

1. pid_is_hermes returns True for PID 0

if not IS_WINDOWS or not isinstance(pid, int) or pid <= 0:
    return True

One early return is doing two unrelated jobs. "Not Windows" correctly means "there is no taskkill path, proceed"; but the same branch also swallows every malformed input on Windows, and True here means authorize the kill.

Measured on your branch:

pid_is_hermes(0)     -> True     # System Idle Process
pid_is_hermes(-1)    -> True
pid_is_hermes('x')   -> True
pid_is_hermes(None)  -> True
pid_is_hermes(4)     -> False    # System - correctly refused

A PR titled fail-closed PID-ownership guard fails open on the four worst inputs. A stale .pid file truncated to 0, or a JSON field that decoded to None, walks straight through to taskkill. Splitting the condition is the whole fix:

if not isinstance(pid, int) or pid <= 0:
    return False          # nonsense PID: never authorize
if not IS_WINDOWS:
    return True           # no taskkill path on POSIX

2. The repo already solves this, with psutil, in one of the files you're patching

psutil==7.2.2 is a hard dependency, and pyproject.toml:103 calls it "the canonical" cross-platform process/PID management library. hermes_cli/dashboard_procs.py:152 — one of the three files this PR edits — already imports it to read psutil.Process(pid).environ().

More to the point, the codebase already implements the recycled-PID defense you're building, using the right primitive. hermes_cli/active_sessions.py:205:

def _process_start_time(pid: int) -> Optional[float]:
    # Pair pid with process create_time when psutil can read it, so a recycled
    # pid does not keep a stale lease alive indefinitely.

That comment is your bug, already reasoned about. And create_time is strictly stronger than a name match: recording (pid, create_time) when you spawn and re-validating both before the kill detects recycling even when the recycled process happens to also be a Hermes process. A CommandLine -match 'hermes' check cannot — it says yes to the wrong Hermes process just as readily as to the right one.

The speed difference is not marginal. Same machine, 5 runs each:

approach	per-PID	10-PID teardown
powershell -Command Get-CimInstance ... (this PR)	1.414 s	~14 s
psutil.Process(pid) name + create_time	0.000059 s	~0.6 ms

~24,000x. That's spawning a whole PowerShell runtime per PID to answer a question psutil answers from a struct.

3. The cost lands in a loop on the update path, which has wedged here before

_stop_process_trees(pids) iterates a list, and the probe is now inside that loop with timeout=5. So a teardown pays 1.4 s per PID best case and up to 5 s per PID when WMI is slow, serially, mid-update.

That is a known failure mode in this repo, not a hypothetical: #87144, fix(cli): bound the Windows process-scan probes so a slow WMI scan cannot wedge hermes update. This reintroduces per-PID WMI on the update path. Worth reading that PR before settling on the mechanism, because psutil sidesteps the whole question.

4. The substring match is looser than it looks, in both directions

-match 'hermes' is an unanchored, case-insensitive regex over CommandLine. Anything whose command line merely contains the string passes:

• a shell sitting in C:\Projects\hermes-agent
• an editor with a hermes file open on the command line
• any tool invoked with a hermes path as an argument

So the guard narrows the blast radius but does not close it, and it introduces a new way to kill something the user cares about (their terminal). Conversely, protected processes return a null CommandLine, where -match yields false and you skip — correct, and worth a comment so nobody "fixes" it later.

The (pid, create_time) pairing avoids this entirely by not asking "does this look like us" at all.

5. Three copies of the probe, and two of them drift from the helper

You wrote pid_is_hermes, then inlined two more copies in dashboard_procs.py and update_cmd.py instead of calling it. They have already diverged: the helper passes creationflags=windows_hide_flags() and neither inline copy does. Any GUI-parented invocation (the desktop update path reaches both of these) gets a visible console window per PID. Routing all three call sites through the one helper fixes the flash and the drift together.

6. The probe itself is the unbounded-drain shape, in the file that is being fixed for it

r = subprocess.run(..., capture_output=True, timeout=5, ...)

subprocess.run(capture_output=True, timeout=N) kills only the direct child on timeout and then calls communicate() with no timeout. A grandchild holding the duplicated pipe handles keeps the pipes from EOF and the call blocks past its own deadline. That is #91087, and powershell.exe spawning under a shim is exactly the shape that triggers it.

I have an open PR — #91219 — that adds bounded_probe_run to hermes_cli/_subprocess_compat.py precisely for this, so the seam you want is being added to the file you're editing. The two do not conflict: I applied both onto current main in order and they apply clean and compile, with pid_is_hermes and the env passthrough both present. If #91219 lands first, this probe becomes bounded_probe_run(argv, timeout=5, env=...) and inherits the tree-kill and bounded drain for free. Happy to sequence in whichever order maintainers prefer, or to hand you the call form.

(If you take the psutil route in #2, this point dissolves — there is no subprocess left to bound. That is another argument for it.)

7. Small: the USER PATCH 2026-08-18 (BSOD 0xEF fix) comments

These read as provenance from your local patch. Upstream they'd be better as a statement of the invariant, with the issue number carrying the history — something like "never taskkill a PID whose identity we have not re-validated; a recycled PID can be a protected process and killing one bugchecks 0xEF (#89614)". Same information, and it stays true after the patch stops being a patch.

What I'd suggest

Keep the shape, swap the mechanism: record (pid, create_time) at spawn, re-validate both through a single psutil-backed helper, and have all three call sites use it. That gets you a stronger guarantee than the name match, removes ~14 s from a 10-PID teardown, avoids re-opening #87144, and deletes the subprocess in #6 entirely.

Also worth knowing, since you listed it as call site 2: gateway-service/restart_gateway.bat is not in this repo. I checked current main and the full history — the path was never tracked, and the string taskkill /F /PID 33796 has never appeared in any commit. That file is local to your machine, which means no upstream fix can disarm it and it will keep firing after this PR merges. Worth deleting it yourself today. I've written that up in more detail on #89614.

Happy to send any of the above as a patch to your branch rather than leaving it as review notes — say the word.

[gebilaowang404]

Thanks for checking it out on Windows 11 and measuring rather than
eyeballing — this is exactly the review this PR needed. You're right on
every point, and the fail-open one is the most important.

1. Fail-open on malformed PIDs: pid_is_hermes(0/-1/'x'/None) -> True
   is a real bug in a PR titled fail-closed — a stale .pid file
   truncated to 0 or a None from JSON walks straight through to
   taskkill. I'll split the condition exactly as you suggested:
   not isinstance(pid, int) or pid <= 0 -> False, then
   not IS_WINDOWS -> True.
2. Mechanism — accepting your psutil (pid, create_time) direction.
   It is strictly stronger than a command-line match (it catches the
   recycled-Hermes-process case that -match 'hermes' cannot), ~24,000x
   faster (0.000059 s vs 1.414 s per PID), needs no subprocess to bound,
   and psutil is already a project dependency. It also aligns with the
   existing _get_process_start_time / _scoped_lock_owner_state
   machinery in the repo.
3. Single helper: one helper reworked to take (pid, create_time),
   all three sites routed through it — which also fixes the drift you
   found (both inline copies missing creationflags=windows_hide_flags()
   and the console flash that comes with it).
4. Comments: will reword the USER PATCH 2026-08-18 comments to state
   the invariant, with [Windows] Hermes kills svchost.exe via stale-PID taskkill /F /PID → repeated 0xEF (CRITICAL_PROCESS_DIED) blue screens #89614 carrying the history.

On your offer to send the patch: yes please — send the
(pid, create_time) helper plus call-site rework as a patch against my
branch and I'll apply it, run the test suite, verify on Windows, and
push. Your version will land faster and cleaner than my rewrite, and I'll
adapt the regression tests to the new helper (including the fail-open
cases).

[AlexMnrs]

I prepared a tested follow-up patch for this PR:

• Branch: https://github.com/AlexMnrs/hermes-agent/tree/contrib/91297-pid-ownership-guard
• Commit: 0162465e8ec1857ed88ce9101bc22500f073eee6
• Scope: replace the PowerShell PID probe with a shared psutil-based (pid, create_time) guard; fail closed on invalid, unknown, or recycled identities; pass the captured identity through the orphan-backend and stale-dashboard paths; keep taskkill hidden; and add regression coverage.
• Validation: 156 tests passed, 13 platform-specific tests skipped; ruff passed; Windows-footgun scan passed.

The patch is based on the current PR head and is intended to be cherry-picked or adapted here. No files outside this scope are included.

[gebilaowang404]

Applied — thank you! The patch is in as commit f97b0fd on the PR branch,
with your Co-authored-by credit preserved.

Verified locally on Windows before pushing:

• All 6 files applied cleanly against the PR head
• test_stale_pid_guard.py + test_update_orphan_backend_reap.py +
  test_update_stale_dashboard.py: 58 passed, 13 skipped (platform)
• The fail-open cases (0 / -1 / None / bool / non-int) now correctly
  fail closed, and the (pid, create_time) capture-at-discovery flow
  works as described

Also rebased onto current main (was 1161 commits behind — upstream moved
fast) and resolved the resulting _stop_process_trees conflict by keeping
the new upstream _ledger_reapable_backend_pids /
_handoff_reapable_backend_pids rungs alongside the psutil guard.
mergeable: true now.

Thanks for the sharper mechanism — psutil + the repo's existing
get_process_start_time is clearly the right shape vs. my PowerShell
probe.

[teknium1]

Thanks @gebilaowang404 — your fix landed on main via #99558, which composed the three taskkill-identity PRs (#96741 earliest predicate, #91297 fail-closed semantics, #98826 ancestor-tree refusal) into one fail-closed process-identity guard covering all 16 unverified kill sites, with authorship preserved via cherry-pick and proven on windows-latest CI. Closing as superseded by the merged class fix; your contribution is in the commit history.