fix(update): keep gateway launcher visible in _venv_launcher_ancestors

[mck156]

Bug Description

Follow-up to #87608. Fixes #87594. Even with the ancestor-gating fix in _scan_gateway_pids, a /update issued from a messaging platform can still abort with the venv-holder guard on Windows.

Scope note: Windows-only in effect — _venv_launcher_ancestors() returns [] before any of this logic on non-Windows, so there is no POSIX behaviour change to argue about.

The reason: _venv_launcher_ancestors() in hermes_cli/update_cmd.py has the same unconditional ancestor-skip that #87608 fixed on the scan side.

On Windows the gateway is a two-process chain:

• venv\Scripts\python.exe — the launcher, which holds the .pyd files mapped
• the uv-managed interpreter — the worker, which writes the PID file

The pause path calls _venv_launcher_ancestors([worker_pid]) to find the launcher. But its skip set contains the whole ancestor chain — and when the update is spawned by the gateway, the launcher sits in that chain. So the launcher is hidden, stays alive during the update, and keeps the venv .pyd files locked.

Root Cause

Location: hermes_cli/update_cmd.py, function _venv_launcher_ancestors() (around line 3418).

# before
skip: set[int] = {os.getpid()}
try:
    for anc in psutil.Process().parents():
        skip.add(int(anc.pid))   # ← skips the gateway launcher too

When the gateway spawns the updater, the updater's ancestor chain is:

wscript (dead) → venv launcher → uv worker → updater

The launcher discovery walks parents of the worker PID — but every ancestor (including the launcher itself) is already in skip, so the launcher is never returned.

Fix

Mirror the gating idea from #87608: an ancestor whose command line looks like a gateway runtime stays visible; everything else is skipped. A fallback keeps the old whole-chain skip on any error, so behaviour is unchanged in non-gateway-spawned scenarios.

The change is scoped in _venv_launcher_ancestors():

• Narrow the try to the fragile pieces only — the matcher import and the ancestry walk — instead of wrapping the whole loop (an inner try already keeps per-ancestor cmdline() errors as ""); this removes a duplicated fallback walk.
• For each ancestor, re-serialize the command line with subprocess.list2cmdline on Windows so paths with spaces (e.g. C:\Program Files\...) stay quoted for the shlex-based matcher.
• If the command line matches the broad {run, restart} matcher, skip it from the skip set so a gateway launcher stays visible.
• If the matcher cannot be imported, degrade to the previous behaviour (skip the whole ancestry) and log a warning — it silently reverts to the buggy form otherwise, and debug never shows in a normal run.

The same predicate keeps the two layers consistent: this PR also adds _suppressed_as_ancestor on the scan side (per-PID gating with the command line in hand — matcher detail below).

Note on the matcher (deliberate asymmetry): _suppressed_as_ancestor uses the broad {run, restart} matcher while the append path keeps the strict == "run" check — intentional, since a gateway restart ancestor is then neither suppressed nor appended when include_restart_managers is False (same as today). A comment in gateway/status.py also enumerates the verified launch shapes and explains why no venv-exe fallback signal was added.

Files changed

• gateway/status.py — document verified launch shapes on looks_like_gateway_runtime_command_line, note why no fallback signal.
• hermes_cli/gateway.py — add _suppressed_as_ancestor to gate per-PID on the scan side.
• hermes_cli/update_cmd.py — fix _venv_launcher_ancestors (main change); promote fallback log to warning.
• tests/hermes_cli/test_gateway_proc_fallback.py — cover _suppressed_as_ancestor keeping a gateway-runtime ancestor visible on the wmic path.
• tests/hermes_cli/test_update_venv_launcher_ancestor_gating.py — fake-proc ancestry walks the gating branch, hardcoded spaces path, found == [100], any() ordering, and gateway restart no-supervisor coverage.

How to Verify (reproducible)

On Windows with the gateway running as the two-process chain:

# minimal repro (run from the hermes-agent repo root)
import sys
sys.path.insert(0, '.')
from hermes_cli.gateway import find_gateway_pids
from hermes_cli.update_cmd import _venv_launcher_ancestors

# 1. worker PID comes from the PID-file based scan
worker = find_gateway_pids(all_profiles=True)[0]
print('worker:', worker)

# 2. launcher discovery — the buggy part
print('launchers:', _venv_launcher_ancestors([worker]))

Observed on a Windows 11 box (gateway launched via the official scheduled-task VBS):

# before this fix (update_cmd.py at main, with #87608 applied):
worker:   1580
launchers: []            # ← launcher hidden → venv stays locked → update aborts

# after this fix:
worker:   1580
launchers: [23668]       # ← launcher found → venv unlocked → update proceeds

# sanity check: launcher is indeed the worker's parent
worker 1580's parent PID = 23668   ✅

Also confirmed the first layer still works with both fixes applied: find_gateway_pids(all_profiles=True) → [1580] (no regression on #87608).

Test Plan

• Manual verification: launcher discovered after fix (before: [], after: [23668])
• Regression check: worker still discovered by find_gateway_pids (fix(gateway): keep an ancestor gateway visible so the update it spawned can pause it #87608 behaviour preserved)
• test_update_venv_launcher_ancestor_gating.py — gating-branch walk, spaces-path quoting, exact [100], any() ordering, gateway restart no-supervisor fallback
• test_gateway_proc_fallback.py — _suppressed_as_ancestor keeps a gateway-runtime ancestor visible on the wmic path

Risk Assessment

Low — mirrors the gating already merged in #87608, using the same predicate (looks_like_gateway_runtime_command_line). Non-gateway ancestors are still skipped exactly as before; the fallback preserves the old behaviour on any error path. The change affects only the venv-launcher discovery path and the per-PID gating on the scan side.

Relationship to #87608

Decision: keep this PR's per-PID form and close #87608 as subsumed (credit above). If you'd rather #87608 land first, I'll rebase — just say so.

---

Note for @jackulau: I initially closed this PR when I saw your comment on #87608 — I wanted to read your full reply before deciding, to make sure our patches wouldn't conflict. Reopening as requested — happy for you to review.

[mck156]

关闭此 PR,暂不开 follow-up。

[Copilot]

Pull request overview

This PR fixes a Windows-specific failure mode in hermes update --gateway when /update is initiated from a messaging gateway: _venv_launcher_ancestors() no longer unconditionally suppresses the updater’s own ancestor chain, so the gateway’s venv-side launcher can be discovered and stopped to release locked .pyd files.

Changes:

• Update _venv_launcher_ancestors() to keep gateway-runtime ancestors visible (using looks_like_gateway_runtime_command_line) while still skipping non-gateway ancestors.
• Add a defensive fallback path that preserves the previous “skip whole ancestry” behavior on errors.
• Expand inline commentary explaining why gateway ancestors must be treated differently.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jackulau]

Thanks for taking this, and for the independent repro on #87608 that turned it up. Reviewed as offered. The production change is right; two of the three tests do not test what their names claim, and one of them passes with the entire fix deleted.

The fix itself

Correct, and it lands in the right place. looks_like_gateway_runtime_command_line rather than the strict matcher is the right choice for the same reason it was on #87608: on a host without a service manager the restart fallback runs run_gateway() in-process while argv still reads gateway restart, so a strict == "run" would miss a launcher that genuinely holds the venv. And the call-time from gateway.status import ... matches the five existing ones in this module, so no import-cycle risk.

The subprocess.list2cmdline re-serialization is also right, and I want to say why in the thread so nobody later "simplifies" it to " ".join. _gateway_command_subcommand tokenizes with shlex.split(command, posix=False). In non-POSIX mode a quoted argument survives as one token and the quotes are stripped afterwards, so "C:\Program Files\Hermes\venv\Scripts\python.exe" -m hermes_cli.main gateway run matches. Joined with plain spaces it becomes four tokens, none of which is a gateway entrypoint, and the match fails. Your comment says this; I would only add the words posix=False so the reason is checkable without opening the other file.

test_non_gateway_ancestors_still_skipped never reaches the code it names

This is the one that matters. The tree is {300: 250, 250: 1, 1: None}, and the call is _venv_launcher_ancestors([200]). There is no 200 in that tree, so FakeProc(200).parent() hits proc_tree.get(200) which returns None, and the loop body continues on the very next line. The skip set is never consulted, found == [] is reached by a route that has nothing to do with gating, and the assertion holds identically if you delete the whole looks_like_gateway_runtime_command_line block. Your own inline comment says as much ("worker 200 doesn't exist in this tree") without drawing the conclusion.

The property you want is that a non-gateway ancestor that would otherwise qualify stays skipped, so the worker has to hang off it and the ancestor has to look like a venv launcher:

    # updater(300) -> shell(250, cmd.exe) -> wscript(1), and the worker also
    # hangs off the shell so ppid 250 actually reaches the skip check.
    tree = {300: 250, 250: 1, 200: 250, 1: None}
    cmdlines = {
        300: [r"C:\Windows\System32\cmd.exe"],
        250: [r"C:\Windows\System32\cmd.exe", "/c", "hermes update"],
        200: GATEWAY_WORKER_CMD,
        1: [],
    }
    ...
    found = cli_main._venv_launcher_ancestors([200])
    assert found == [], "a cmd.exe ancestor is not a gateway launcher"

250 is even, so FakeProc.exe() reports it under the venv prefix and it would be returned if the skip set had dropped it. That version fails if the gating is widened to skip nothing, which is what the test is for.

test_gateway_launcher_with_spaces_in_path_still_found has no spaces except on your machine

The comment says "PROJECT_ROOT itself contains a space (D:\Program file\...)". That is true of your checkout and of essentially no other. On CI the repo lives at a path with no space, list2cmdline adds no quotes, and the test becomes a byte-identical copy of the first one. The one property it exists to pin is the one it stops checking the moment it leaves your laptop.

Hardcode it instead of deriving it, since nothing here needs the real project root:

        100: [
            r"C:\Program Files\Hermes\venv\Scripts\python.exe",
            "-m", "hermes_cli.main", "gateway", "run",
        ],

and make FakeProc.exe() return a matching venv-prefixed path for that pid. Worth also asserting the quoting directly rather than only through the outcome, so a failure says which half broke:

    assert '"C:\\Program Files\\Hermes\\venv\\Scripts\\python.exe"' in serialized

Smaller things

The fallback silently restores the bug. If from gateway.status import ... ever fails, the except re-walks and skips the entire ancestry, which is precisely the behaviour this PR removes, with no trace. Since the whole failure mode is "the update aborts on the venv-holder guard and the user cannot tell why", a logger.debug naming the fallback would pay for itself the first time someone reads an update log. The try/except Exception: pass shape it replaces was at least not hiding a fix.

os.name == "nt" is unreachable here. _venv_launcher_ancestors returns [] at the top unless _m()._is_windows(), so the " ".join(raw) branch can never execute. Not harmful, but it implies a portability the function does not have, and a reader has to go check. Either drop it, or keep it with a one-line note that it is there so the helper stays correct if it is ever lifted out.

Unused in the test module: import pytest and WINDOWS_PATCH_TARGET.

Latent trap in the fake, worth a comment rather than a change: FakeProc.exe() keys on pid parity, so the worker (200) also reports a venv path. It does not affect the current tests because ppid in set(pids) excludes it, but the next person to add a case will trip over it.

Happy to re-review once these are in. Cross-linking #87608, which is the sibling fix in _gateway_ancestor_pids.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(update): keep gateway launcher visible in _venv_launcher_ancestors

The fix correctly distinguishes gateway-runtime ancestors from false positives, and the fake-psutil tests exercise the Windows quoting path. Observations:

1. hermes_cli/update_cmd.py _venv_launcher_ancestors: on POSIX the cmdline is re-serialized with " ".join(raw) without quoting. If a gateway runtime path contains a space (e.g. /Users/John Doe/hermes/venv/bin/python), the shlex-based matcher in looks_like_gateway_runtime_command_line will mis-tokenize and the gateway ancestor will be skipped again — the exact bug this PR fixes. shlex.join(raw) would quote correctly on both platforms.
2. The whole fix rests on the cmdline matcher recognizing every real gateway launch shape. A gateway started through an entry point the matcher does not cover (renamed script, uv run, extra interpreter flags) silently falls back to the old skip-everything behavior. Consider a fallback signal (e.g. venv exe path plus an ancestor argv containing gateway run) or a comment enumerating the verified launch shapes.
3. Minor: the outer except Exception fallback re-walks the entire ancestry, duplicating the loop. Harmless, but narrowing the try to the import + matcher (and letting per-ancestor cmdline errors fall through to anc_cmdline = "") would be simpler to reason about.

[mck156]

Hi @jackulau,

I saw your comments on PR #87666 — thanks for going through it so thoroughly. I also saw the cross-link to #87608 in your note; you're right that it's the sibling fix in _gateway_ancestor_pids, and I've kept that in mind while working through the points here. Where things stand:

• test_non_gateway_ancestors_still_skipped — you were right that the tree never mentioned pid 200, so the test just returned early and would have passed even without the fix. I've added 200: 250 so it actually walks into the gating branch now, with the ancestor looking like a venv launcher just as you suggested. I also mutation-checked it: if the gating is widened to skip nothing, the test fails as it should.
• The spacing test — it used to derive its path from PROJECT_ROOT, which only has spaces on my machine, so it wasn't really testing anything on CI. I've hardcoded a C:\Program Files\... path, asserted the quoting stays intact, and pinned os.name = "nt" so the list2cmdline branch actually runs on POSIX CI.
• FakeProc.exe() — I dropped the odd/even pid heuristic and now derive venv-ness from the cmdline instead, which also takes care of the even-pid trap you pointed out.
• gateway restart — the no-supervisor fallback case you mentioned now has its own test so the matcher's restart shape is covered too.
• Edge cases — added tests for the matcher-import failure path and the ancestry-walk failure path, including a check that the fallback logs instead of failing silently.
• The fallback itself — it now logs why it skipped the ancestry (no more silent dead-end), and I narrowed the try to the import + walk so we don't re-walk the whole ancestry when something goes wrong.
• os.name == "nt" — left it in place but documented why; same for the matcher, where I enumerated the verified launch shapes and noted why I didn't add a fallback signal.

The full gating test suite plus related tests are green (55 passed), and I verified the tests themselves with mutation testing in both directions so they're actually catching what they claim.

Would be great to hear if you think anything still needs another look. Thanks again!

[jackulau]

@mck156 - I owed you this review since you found the second site and took it on. Read the whole diff. The core reasoning is right, the _suppressed_as_ancestor shape is better than what I would have written, and the comment you left on the matcher about deliberately not adding a venv-exe fallback signal is the right call for a matcher that also serves as an ownership check.

Three things, in the order that will cost you time if they are left.

1. This PR has no CI at all

$ gh pr checks 87666
no checks reported on the 'fix/venv-launcher-ancestor-gating' branch

Not "failing" - zero workflows have ever run on it, and it has been open since 2026-08-16. Every other PR at this seam shows 45-50 checks. On this repo a fork PR's workflows need a maintainer to approve the run the first time, so this is very likely waiting on that rather than anything you did wrong. Worth asking for explicitly in a comment, because right now nobody can merge it: All required checks pass cannot go green if it never runs. Pushing a trivial commit sometimes re-triggers the approval prompt too.

2. It overlaps #87608, and stacking it will save you a rebase

Your diff changes hermes_cli/gateway.py::_scan_gateway_pids - the same function #87608 changes, in the same place (the exclude_pids | _get_ancestor_pids() line). #87608 replaces that line; you replace it too, with a per-PID _suppressed_as_ancestor gate. If #87608 merges first, this PR conflicts there and you will resolve it under time pressure; if this one merges first, mine becomes a no-op that I should close.

Cleanest resolution, and I am not asking for it because it is my PR:

• Either drop the gateway.py hunk here, rebase onto main once fix(gateway): keep an ancestor gateway visible so the update it spawned can pause it #87608 lands, and keep this PR purely about _venv_launcher_ancestors in update_cmd.py - which is your finding and the part nobody else has covered;
• or keep both hunks, say so in the body, and I will close fix(gateway): keep an ancestor gateway visible so the update it spawned can pause it #87608 as subsumed by this one. Your _suppressed_as_ancestor is honestly the better form of that fix, because it decides per PID with the command line in hand instead of pre-computing an exclusion set.

I am fine with either. Tell me which and I will do my half.

3. logger.debug on the fallback path defeats the reason you added it

logger.debug("ancestor gating unavailable (%s); skipping whole ancestry", exc)

Your own comment two lines up says the point of logging it is that "an update that trips the venv-holder guard (silently) never shows why". But debug is off in every normal run, so in exactly the scenario you are describing the line does not appear. The condition is also rare and consequential - it means the matcher could not be imported and the code has silently reverted to the pre-fix behaviour that produces the bug. That is a warning, and it costs nothing because it can only fire when something is genuinely broken.

Smaller notes, take or leave

• The os.name == "nt" branch with the "unreachable here" comment is fine by me and I would keep it. Re-serializing with subprocess.list2cmdline so quoting survives for shlex is a detail I would have missed, and a C:\Program Files\... path would have silently failed to match without it.
• looks_like_gateway_runtime_command_line (the broad {run, restart} matcher) in _suppressed_as_ancestor while the append path uses the strict == "run" one: I convinced myself this is safe, and worth a sentence in the PR body so a reviewer does not have to. A gateway restart ancestor ends up neither suppressed nor appended when include_restart_managers is False, which is the same outcome as today.
• Worth stating explicitly in the body that this is Windows-only in effect: _venv_launcher_ancestors returns [] before any of this on non-Windows, so there is no POSIX behaviour change to argue about.

Nothing here is a blocker on the logic. Item 1 is the one that is actually stopping this from merging.

[mck156]

Hi @teknium1 👋 — this is my first PR to this repo, and I noticed the workflows never ran on it (0 checks so far, open since Aug 16). Since fork PRs need a maintainer to approve the first run here, could you approve it when you get a chance? It's the fix for #87594 (gateway ancestor gating), already reviewed by @jackulau . Happy to push a trivial commit if that re-triggers the approval prompt. Thanks!

[mck156]

@jackulau — thanks for the thorough review, and for spelling out the three things in order of impact. That sequencing genuinely helped me prioritize.

#1 CI — you were right that the blocker is the missing workflow run rather than anything in the diff. I've left a comment asking a maintainer to approve the first run on my fork PR; if that doesn't get picked up after a bit I'll push a trivial commit to re-trigger the approval prompt.

#2 overlap with #87608 — I read your point about the two PRs touching the same line, and I've settled on keeping this PR's _suppressed_as_ancestor form and closing #87608 as subsumed, since you yourself called the per-PID shape the better one. To make sure your contribution isn't lost, I've added a "Relationship to #87608" section to the PR body crediting it, and there's a note in gateway/status.py enumerating the launch shapes. If you'd rather #87608 land first after all, I'm happy to rebase instead — your call.

#3 log level — you were right that debug defeats the point. I've promoted it to warning in _venv_launcher_ancestors; it only fires when the matcher genuinely fails to import, so it costs nothing and makes the silent fallback visible.

As for the smaller notes — I've added the Windows-only scope note and the matcher-asymmetry explanation to the body, and left the os.name == "nt" branch as-is per your take. Thanks again for the close read; happy to iterate further.

[mck156]

@kshitijk4poor @OutThisLife — sorry to ping you both, but hoping one of you can help unblock this fork PR's CI. It's been open for a bit with 0 workflow checks, because fork PRs on this repo need a maintainer to approve the first run. The branch is now fully merged with latest main (mergeable confirmed), reviewed by @jackulau, and tests pass locally (14 passed). If either of you has workflow-approve access, a quick ✓ would let the checks actually run. Thanks for your time!

[mck156]

Closing this PR — #91869 (merged) fixes #87594 with the same ancestor-carve-out at both _detect_venv_python_processes and _venv_launcher_ancestors, plus a live Windows E2E suite. No need for this one anymore. Thanks @teknium1 for taking it all the way to main (and @jackulau for the review that sharpened the diff along the way).

For the record: the debug → warning log point and the Windows-only scope note from the review are still worth keeping in mind for the merged implementation — happy to open a tiny follow-up if anyone wants that.