r2 gate: Path(stdout.strip()) dropped a trailing space in the checkout path, so include
targets resolved to a missing file and were skipped. With a caller-set GIT_DIR or
GIT_WORK_TREE git prints origins relative to another base; refuse instead of guessing.
Gate follow-ups for the includeIf walk: git prints repo-local origins relative to the
worktree top level, so discovery from a subdirectory looked for the target in the wrong
place and skipped it (fail open). Resolve relative origins against `rev-parse
--show-toplevel` (one extra spawn, only when an include is present). Read each target once,
refuse a non-regular target (FIFO/device/directory) and more than 16 targets, so a repo
cannot make every hardened call spawn without bound. One discovery-regexp constant and a
single include-key matcher.
Filter discovery refused every repository with a repo-local includeIf, so `hermes -w`,
kanban/subagent worktrees and the recovery hint stopped working inside actions/checkout
checkouts (which write an includeIf credential include) and similar setups. Read each
includeIf target directly, whatever its condition, and neutralize the filter commands it
defines alongside the ones found through --includes. One probe now lists both the filter
keys and the include paths (with --show-origin, so relative paths resolve against the file
that names them). A missing target is skipped as git skips it; an include nested inside an
include target, an unreadable target or a non-file origin still refuses.
bounded_probe_run returns a CompletedProcess from communicate() with
text=True, so stdout is always a str once proc is not None; the
`or ""` guard could never fire. GIT_CONFIG_COUNT is always written by
noninteractive_git_env (it pops any caller value and sets
str(len(overrides))), so the defensive `.get(..., "0") or 0` parse
only hid that invariant.
Three gaps in noninteractive_repo_git_env, two probe-verified:
- Dedup was on key.lower(), but git keeps the subsection's case:
[filter "evil"] + [filter "Evil"] with `filter=Evil` overrode only the
lowercase driver and the hostile smudge ran. Dedup on the exact name
`git config --name-only` prints.
- Only includeIf "onbranch:" failed closed. includeIf "gitdir:<repo>/.git/
worktrees/" matches the new worktree's git dir during `worktree add`, so it
loads a filter discovery never saw. Refuse on ANY repo-local includeIf
(global/system config is already /dev/null).
- The discovery probe used subprocess.run(timeout=2), which on Windows can
deadlock in the post-timeout communicate() and flashes a console. Use
bounded_probe_run (rc 1 = "no filters" stays a verdict, so not
bounded_git_probe).
Also share the refusal stderr as FILTER_DISCOVERY_FAILED across the kanban
and worktree-gc call sites and the test, and fold the fail-closed test into
the filter test (shared repo/env helpers) with case_collision and
gitdir_include rows.
Every discovered filter key adds GIT_CONFIG_KEY/VALUE env entries. A repository
with tens of thousands of filter sections makes the spawned git fail with
OSError(7, "Argument list too long") inside kanban/worktree-gc `_git`, which do
not expect it. Past 256 keys, fail closed like any other untrusted discovery.
One parametrized invariant covers both fail-closed paths (onbranch include and
filter flood) through the kanban `worktree add` call.
Fixes#126017
noninteractive_repo_git_env discovers repository-named filter keys with
`git config --includes`, which evaluates includeIf "onbranch:..." against the
branch checked out NOW. A `worktree add -b X` then checks out X, git loads the
X-only include, and its smudge filter runs with no override pinned. Any
onbranch include in the effective config means the inventory cannot be trusted,
so return None and let callers skip the automatic git call.
Refs #126017
The session-start workspace snapshot runs git status, which re-hashes a
re-timestamped file through the repo-named clean filter, and a delegated
subagent's worktree add runs the smudge filter. Both now use
noninteractive_repo_git_env and skip the call when filter discovery fails.
(cherry picked from commit 04170a83d19a026fe6c5f968ce6e61b61f53c418)
A repository names its clean/smudge/process filter drivers in .gitattributes, so the
fixed GIT_CONFIG_KEY pins in noninteractive_git_env cannot enumerate them. Discover the
checkout's filter command keys under the isolated env and append empty command overrides
plus required=false. Discovery failure returns None so callers skip the git call.
Ported from #126075.
(cherry picked from commit ebdb7feb263512a2e76f06d9ee20d483af6adc24)
A console-less parent (pythonw backend, detached daemons) allocates a visible
console per bare spawn, so Windows startup flashes ~7-8 console windows
(#117781) and the GPU statusbar flashes two more every 5s (#101895). Threads
creationflags=windows_hide_flags() through every hermes_cli spawn site the
desktop reaches: gitlock (tasklist probe + git plumbing), the scheduled-task
PowerShell probe, the updater's _git_run/_no_prompt_git_kwargs/_GIT_TEXT_KW
runners, the safe-directory config probe, and consolidates the statusbar's two
nvidia-smi reads per poll into one cached hidden query. The statusbar also
stops polling while the document is hidden and resumes on visibilitychange
(#120262).
Supersedes #117805, #117833, #102047, #120285 (thanks @JoaoMarcos44, @Finn763,
@monerostar, @szzhoujiarui).
After the package-manager handoff, hermes update finishes on the bare store
interpreter and spawns the detached restart watcher as sys.executable -c.
The watcher imported gateway.status (utils -> hermes_yaml -> ruamel) and
hermes_cli.config, died with ModuleNotFoundError before relaunching, and
left every manually started gateway down after the update (gated on #124649).
Move the stdlib liveness probe (zombie-aware POSIX kill(0), Windows
OpenProcess) into hermes_cli._subprocess_compat, have gateway.status's
fallback delegate to it, and import only stdlib-backed modules in the
watcher.
Fixes#124649.
A git-less Windows ZIP install has no .git and never runs git, yet
_prepare_git_command called expose_pm_git() before it checked
use_zip_update: offline or with no pin, pm.ensure raised and aborted an
update that never needed git. expose_pm_git now takes the project root
and does nothing unless it is a git checkout (both callers).
When install.ps1 runs as one process, the git it staged into PM's store
is on the inherited PATH, so expose_pm_git returned early and PM's facts
never recorded it; plain hermes processes (plugin git installs, doctor,
version info) had no git until the first `hermes update`. A git found
under PM's store is now treated as that unrecorded copy and ensured, so
the source completion writes the git fact at install time.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
expose_pm_git() probed with a bare shutil.which, which the managed-runtime
resolution guard rejects outside hermes_platform/. Route the lookup through
hermes_platform.resolver.locate_command instead.
On a Windows machine with no git, scripts/install.ps1 stages the pinned Git
for Windows into the pm store for its own process only, and pm's facts
never record it. Every later product process that ran a bare `git` failed:
- `hermes update` died before its first step:
"✗ Update failed: [WinError 2] The system cannot find the file specified"
- the source completion that finishes an installer re-run or an update
found no commit, deleted the install stamp, and the next boot wrote an
adoption stamp that names no commit. It also printed
"Could not refresh release history ([WinError 2] ...)".
expose_pm_git(): when Windows resolves no git, ensure PM's git explicitly
(both callers are user-initiated, like ensure_tools_for_sync) and put its
composed PATH (cmd and usr\bin) on the process, so children inherit it.
The update calls it before its first git, and the source completion calls it
before its builds and stamp. A machine with a working git is untouched.
main_desktop already ensures PM's git for its build for the same reason.
Conflict resolutions and semantic fixups:
- utils.py / hermes_yaml.py: main widened ruamel's round-trip emitter so a long
double-quoted scalar is never folded after an escaped backslash. pm-clean builds
every rt emitter through hermes_yaml.roundtrip_yaml(), so the width lives there
(ROUNDTRIP_YAML_WIDTH moves with it); xai_retirement imports it from hermes_yaml.
- hermes_cli/banner.py: keep pm-clean's removal of the banner update check. Main's
GIT_NO_LAZY_FETCH fix for it applies to its replacement, source_check: every
read-only probe (source_git_env) now refuses promisor lazy fetches, and the
partial-clone test targets that probe (red without the flag).
- .github/workflows/tests.yml: keep setup-pm; main's uv pin bump does not apply.
Main's WAL-capable SQLite gates are kept, run against $HERMES_PYTHON (the
PM-pinned interpreter, SQLite 3.53.1). The e2e step takes main's
--include-integration invocation.
- apps/desktop: package.json has no build block here, so main's macOS locale-marker
restore joins the darwin branch of the existing after-pack.mjs, and its test
loads the hook from electron-builder.config.cjs and imports PlatformPackager
from app-builder-lib's root (electron-builder 27 exports no ./out paths). The
win32 row is dropped: this hook sanitizes and signs PE trees on win32 by design.
- reconciliation.ts: main's rowId hydration (#119326) was merged into the first of
pm-clean's split helpers only; the resolver is now one helper both halves use.
- en.ts: both sides' keys kept. tests/tools/test_lazy_deps.py stays deleted.
- Tests main added with `import yaml` use hermes_yaml, like the rest of the tree.
In a partial (blobless/treeless) clone, the passive update check's
`git merge-base --is-ancestor <fresh upstream tip> HEAD` asked about an
object the clone had never fetched, so git spawned `git fetch` from the
promisor remote to download it (~233k objects for a real install, no
negotiation). The 5 s probe timeout killed only the merge-base process;
the fetch child kept running orphaned and left partial packs behind, on
every launch.
Every banner git probe and every `bounded_git_probe` (session-start
coding context, TUI gateway branch/root probes) now runs with
GIT_NO_LAZY_FETCH=1 (NO_LAZY_FETCH_ENV). A missing object makes the probe
fail fast; the ancestry check then falls through to the compare API, which
is already the path for tips not in local history. `hermes update` keeps
lazy fetch: its checkout/merge legitimately needs missing blobs.
Resolved toward the branch: PM provisions uv/python (main's install.ps1 uv-shim
salvage + its test and workflow steps dropped), the shim re-exec stays retired,
package.json carries no electron-builder block (afterExtract identity stamp wired
into electron-builder.config.cjs instead; after-pack.mjs keeps signing only),
Desktop workspace-deps helpers stay retired. Main's scratch-dir bootstrap
(export_scratch_tmp_env) is taken and re-run after profile resolution.
On windows-latest (wine2e run 35430217919) the MSYS `sleep` grandchild of the
bash probe survived kill_process_tree: taskkill /T walks live parent pids and a
Cygwin exec lets the forked stub exit once the new image runs, so the grandchild
had a dead parent and kept the captured pipes open — the #73403 pipe-holder.
bounded_probe_run now spawns through local_runtime.processes.spawn_server
(Job Object, KILL_ON_JOB_CLOSE, no breakaway) and closes the job on timeout and
after a normal exit. POSIX path unchanged (spawn_server returns a plain Popen).
Reconcile plugin declarations and validation through PM's atomic generation publication; preserve external runtimes, target markers, and conflict refusal. Keep one source-update completion owner and port upstream lifecycle changes to the PM desktop/runtime paths.
bounded_probe_run collapses spawn failure and timeout into one None, so a venv
interpreter that exists but cannot be executed (PermissionError, ENOEXEC, fork
failure) was reported as "timed out before reporting import health" -- a bogus
warning on the git path and sys.exit(1) on the ZIP path -- and the
`except OSError: return {}` branch beneath it was dead. Let the Popen error
propagate (opt-in flag, default unchanged for the other probes) so "we could not
run our own probe" says nothing about the checkout and no longer blocks the
update, while a spawned child that hangs is still a verdict.
Restores the non-fatal test the earlier rewrite dropped, now driving the real
bounded_probe_run/Popen: red on the previous head, green here.
Review finding: spawn failure of the import probe reported as a fatal timeout.
Delete consumer-side Git discovery caches and layout reconstruction. Share the selected environment while preserving PM acquisition policy, system Git fallback and downstream config isolation. Route Matrix binding through its existing dependency operation and explicit setup through sync_venv.
Real Git regression tests cover absent ambient PATH, changed selection, unsupported target and failed acquisition. Independent final review: 100 focused tests pass, including malicious Git config and Matrix setup contracts. Native Windows execution was not run.
Tests: fold the ambient GIT_CONFIG_KEY_n negative and the isolation-still-in-force
assertions into the ordering test, and drop the two positive-only cases it subsumes.
What remains pins the injected sequence to git's own `config -z --get-all` output and
asserts the real trust decision (named repo usable, unrelated cross-owner repo refused).
Cost: noninteractive_git_env() runs on every internal git call, including the banner
startup probe, and the two `git config` children added ~10 ms per call against ~0.2 ms
before. Memoise per process on the inputs that select the config files plus the
system/global candidates' mtimes, so an edit to ~/.gitconfig is picked up without a
restart (verified live: 128 -> 0 within one process after appending safe.directory).
Review of #107748 found the carry path serialized the user's trust policy
incorrectly, and the mistake could WIDEN trust rather than merely reformat it.
safe.directory is an ordered multi-valued protected setting: an empty value
resets every entry seen so far. That is the documented mechanism for revoking a
system-wide `safe.directory=*` and then naming only the repositories you
actually trust. The previous helper broke all three properties that make it work:
* read `--global` before `--system` (git's precedence is system, then global)
* dropped empty entries via `if entry:`, deleting the reset marker
* de-duplicated values, though it is a sequence and not a set
Reproduced with real git (2.55.0 here, 2.47.3 by the reviewer). Given
system `safe.directory=*`, global `safe.directory=` then `/trusted/only`:
real git, user's own policy -> rc=128 (dubious ownership)
previous head's sequence `/trusted/only`, `*` -> rc=0 (trust widened)
this head's sequence `*`, ``, `/trusted/only` -> rc=128 (matches real git)
So a user who had deliberately revoked a machine-wide wildcard silently got it
back inside Hermes's internal git calls. That contradicted the "read-only and
non-widening" claim the original change rested on.
Fix: read scopes lowest-precedence first (system, then global) and replay every
value verbatim -- no de-duplication, no dropping of empty reset markers. Read
with `git config -z` so a value containing whitespace or a newline stays the one
entry git reads it as, instead of being split into several bogus trust entries
by splitlines()/strip(). The trailing field of a -z stream is always empty and
is discarded; interior empty fields are real resets and survive.
Two invariant tests, both proven red on the previous implementation:
* the injected sequence equals `git config -z --get-all safe.directory` for
the same two config files, asserting git's documented reset shape
* an E2E trust decision under GIT_TEST_ASSUME_DIFFERENT_OWNER=1: the named
repo stays usable and an unrelated cross-owner repo is still refused,
proving the reset still revokes the wildcard
13 passed in tests/hermes_cli/test_noninteractive_git.py, 69 in
tests/security/test_gitspawn_config_injection.py and
tests/tools/test_checkpoint_manager.py.
`noninteractive_git_env()` blanks GIT_CONFIG_GLOBAL/SYSTEM to /dev/null so a
user's config cannot hang Hermes's internal git plumbing with pagers, hooks or
credential prompts. Sound intent, but it also discards `safe.directory` — and
git honours that key ONLY from global/system config (it is rejected from
repo-level config by design, so a hostile repo cannot self-authorise).
Result: every internal git call fails on a repo whose st_uid != geteuid():
$ hermes -w
✗ Failed to create worktree: fatal: detected dubious ownership in
repository at '/mnt/nas/py/repo'
That hits NFS/CIFS mounts without idmapping, shared checkouts, and containers
with a remapped uid. The user's own `git config --global --add safe.directory`
is correctly set and their interactive git works — Hermes throws the setting
away before git reads it, so the error's own suggested remedy can never fix it.
There is no config or env escape hatch: the blanking is unconditional.
Reproducer (any repo where the checkout uid differs from the caller's):
git rev-parse HEAD # works
GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \
GIT_CONFIG_NOSYSTEM=1 git rev-parse HEAD # dubious ownership
Fix: read the user's real `safe.directory` values before the isolation is
applied, then re-inject them over the GIT_CONFIG_KEY_n channel, which survives
GIT_CONFIG_GLOBAL=/dev/null. Isolation is unchanged — global/system config stay
pointed at /dev/null and every hardening override still applies, since a later
key of the same name wins in git's config order.
Read-only and non-widening: only values already present in the user's own
config are carried, so this grants no trust they had not granted. Ambient
GIT_CONFIG_KEY_n injection is still stripped first, so a caller cannot launder
an attacker-controlled path in this way — covered by a regression test.
Tests: three cases in TestNoninteractiveGitEnv — entries carried past
isolation, no entries injected when the user configured none, and ambient
injection not trusted. All pin GIT_CONFIG_SYSTEM at an empty file, since a real
/etc/gitconfig on the test host can otherwise leak entries and mask the
assertions.
Verified on Arch Linux, git 2.x, repo on an NFSv4 mount (uid 1024 vs caller
1000): `git worktree add` under the patched env returns rc=0 where it
previously failed. 80 passed in tests/hermes_cli/test_noninteractive_git.py,
tests/security/test_gitspawn_config_injection.py and
tests/tools/test_checkpoint_manager.py, including the pre-existing assertions
that the isolation stays in force.
The old contract described the replacement tree, not the code still running
in installations that update to it. Deleted managed_uv imports escaped it.
Inventory the full reachable update-channel history, discover old entrypoint
paths and helper extractions, then union those imports with the working tree.
Refuse shallow regeneration. Preserve old names when current callers vanish.
Keep same-named function alternatives and require module-scope bindings.
Restore the additional historical names as inert shims. Installer boundaries
stop for relaunch rather than enable fallback installs. Keep live DingTalk
dependency setup on PM rather than the retired lazy_deps import.
History enumeration covers 33,720 commits at 1021a03256 and 8,078 selected
file versions. The generated contract contains 496 required imports with no
missing names. Dynamic edges and their manual-review limits remain visible.
No claim is made that static analysis proves all runtime plugin imports or
every historical platform path. See tests/compat/README.md.
Verification: 622 targeted tests passed with file retries disabled, including
the frozen contract, historical shims, real PM launch tests, and DingTalk.
Ruff, focused type checks, import guard, and comment prose checks passed.
The full suite and native Windows execution were not run locally.
Merge upstream b1f003e186 while preserving PM runtime ownership and
Python 3.14 worker startup, Windows signing, and macOS wait recovery.
Keep retired runtime modules deleted. Port upstream updater preflight
checks into the checkout strategy and preserve live build logging.
Carry checkpoint filename handling and process recovery into the current
module layout. Regenerate locks and adapt incoming platform test markers.
Focused Python and JavaScript tests, desktop and root-test typechecks,
conflict-path lint checks, lock validation, and retired-import checks pass.
The full test suite and packaged release builds were not run.
ssh bypasses stdin=DEVNULL and GIT_TERMINAL_PROMPT: when a git child
dials an SSH remote whose host key is unknown, ssh opens /dev/tty
directly and its yes/no prompt steals the caller's terminal — exactly
what noninteractive_git_env exists to prevent. Pin core.sshCommand to
"ssh -o BatchMode=yes" at the config-injection layer so the ssh child
fails instead of prompting; an agent-authenticated ssh still succeeds,
and an explicit user GIT_SSH_COMMAND env var still takes precedence
(#104591).
For each issue anchor present in BASE 63279301bc non-test .py and absent on HEAD, the BASE comment/docstring block was re-attached at the HEAD location of the code it explained (matched by the distinctive code line / enclosing def). Sentences already covered by an existing HEAD comment were deduped; the issue number always survives. Insert-only: no code lines changed.
Hermes gathers workspace context by running git against the session
directory automatically — the coding-workspace snapshot, gateway
project-tree build, /diff, @diff|@staged context refs, goal-gate
fingerprint, and -w startup worktree add — before any prompt, tool call,
approval, or trust gate. Those probes ran the system git without
stripping the repository's own config, so a repo delivered as files with
its .git directory intact (a shared zip, sync folder, or USB stick;
git clone never transfers .git/config) could set an execution-sink git
setting and get arbitrary host code execution as the user with nothing
on screen.
- core.fsmonitor / core.hooksPath / pager / editor / credential helper:
neutralized by routing every automatic probe through
noninteractive_git_env(), which pins those keys to inert values via
GIT_CONFIG_* and ignores global/system config. bounded_git_probe (the
reported sink, coding_context._git + tui_gateway.git_probe) now defaults
to that env; worktree-add, working_diff, web_git, context_references,
goals, and subagent_worktree route through it too.
- Attribute-scoped [diff "x"] command=/textconv= drivers: the attacker
names the driver in .gitattributes, so GIT_CONFIG_KEY overrides can't
enumerate them. Added harden_git_argv(), which inserts
--no-ext-diff --no-textconv on diff-rendering subcommands (diff/show/
log/blame) only — status et al reject the flags. Both flags required
(verified empirically; each alone leaves the other live).
Builds on the noninteractive_git_env config-scrubbing from the
gemini-cli #28792 port. Real-git E2E regression suite arms a malicious
repo and asserts every automatic path neutralizes fsmonitor, hooks,
external-diff, and textconv; a baseline test proves the repo is armed.
Introduce the pm store: a unified, hash-verified package store that
replaces lazy_deps and the old installer's ad-hoc tool downloads.
Store tools are provisioned on PATH (ffmpeg, node/npm via pinned uv),
with a resumable 8-way downloader, verify() returning failure reasons,
and adopt() made EPERM-safe. chromium ships in the payload for every
target. The 3600-line install.sh is replaced by a staged bootstrapper
(heavy deps are pm's job after this); setup-hermes.sh, Dockerfile and
nix pin tables are rewired onto the store. Old install-script tests,
lazy_deps/managed_uv/build_info, and the ps1/bash installer test
batteries are removed with the machinery they tested.
Rebuilt from ethie/pm onto upstream/main (ac6c8028e0) after the
utf-8-sig sweep. 16 hot files (main also churned them) hand-merged:
platform adapters, main.py, electron/main.ts, tui_gateway/server.py,
cua_backend, installer-tests workflow, install.sh (full rewrite),
setup-hermes.sh, plugins doc.
Salvage hardening on top of the three cherry-picked contributor commits
(#91297 gebilaowang404 + AlexMnrs, #96741 burak33bb, #98826 ayushnangia),
closing the remaining unverified-PID kill sites as one class (#98814, #89614):
- pid_is_hermes: token-boundary 'hermes' match (no more loose substring
false-positives), and an explicit start-time expectation is now honored
on POSIX too (a mismatched fingerprint is a recycled PID on any platform).
- kill_process_tree: drop the guard on our OWN retained Popen child — a
retained handle pins the PID, so the check could only false-refuse.
- gateway.status.terminate_pid: POSIX force-kills also refuse when a
caller-provided expected_start_time no longer matches.
- kill_gateway_processes: re-verify the LIVE cmdline at kill time (the
scan-time match is a TOCTOU window).
- _reap_unsupervised_gateway_orphans: fingerprint orphans at scan time and
require a still-matching identity before the delayed SIGKILL escalation.
- whatsapp _kill_port_process: never kill a bare netstat/lsof-scanned PID
unless the live process is actually a node bridge (was a stranger-kill).
- browser daemon reap/close paths: pass the start-time fingerprint into
ProcessRegistry._terminate_host_pid (previously unverified), and the
session-close path now runs the same daemon identity verification as
the orphan reaper.
- tests/hermes_cli/test_taskkill_identity_windows_live.py: live Windows
probes (real spawned processes, real psutil ancestry) wired into the
on-demand windows-latest wine2e lane.
Fixes#98814Fixes#89614
Guard every Windows `taskkill /PID` against stale/recycled PIDs
(#89614: 8x 0xEF blue screens; a rebooted PID can be svchost.exe).
Adopted the community patch by AlexMnrs (commit 0162465): shared
psutil-based (pid, create_time) guard reusing the repo's existing
get_process_start_time machinery:
- fail closed on invalid/unknown/recycled identities (0/-1/None/bool/non-int)
- capture identity at discovery, re-validate at kill time
- all three sites through pid_is_hermes; taskkill stays hidden
Sites: _subprocess_compat.kill_process_tree,
dashboard_procs._kill_stale_dashboard_processes (win32),
update_cmd._stop_process_trees.
Refs #90471, #89614
Co-authored-by: Alex Monrás <AlexMnrs@users.noreply.github.com>
Per-site decisions:
1. hermes_cli/_subprocess_compat.py kill_process_tree(proc) -> None:
MIGRATED. Body now delegates to agent.deadline.kill_process_tree(proc.pid)
via a function-local import; keeps the swallow-everything fail-open
contract and the (proc) -> None signature (agent/shell_hooks.py imports
it by name; _kill_git_process_tree alias preserved). The old body is kept
verbatim as _legacy_kill_process_tree and used as fallback when the
delegation import/call fails. A final proc.kill() is retained on the
happy path so Popen bookkeeping sees the exit (matches old behavior).
2. tools/browser_tool.py _kill_process_tree(proc): MIGRATED, same pattern
(delegate + _legacy_kill_process_tree fallback). Behavior delta: the old
body sent SIGTERM then SIGKILL with zero grace between them; the shared
primitive sends SIGKILL only. With no grace period the observable effect
is identical, and the psutil descendant sweep now also reaches
agent-browser's setsid'd daemon grandchild, which killpg alone missed.
tests/tools/test_browser_npx_warmup.py's TestKillProcessTree repointed at
the legacy fallback (its assertions describe the fallback's internals).
3. tools/code_execution_tool.py _kill_process_group(proc, escalate):
MIGRATED. It was a plain parent+descendants terminate (then wait 5s +
kill when escalate=True) — expressed as two delegated calls:
kill_process_tree(pid, sig=SIGTERM), then on escalate-timeout
kill_process_tree(pid, sig=SIGKILL). Delegation failure degrades to
proc.kill(), mirroring the old psutil-failure fallback. Delta: the old
body terminated children before the parent; the shared primitive
signals the group atomically (child is a session leader via
start_new_session=True) plus an identity-aware descendant sweep —
strictly wider coverage, same signals.
4. gateway/status.py: KEPT BOTH SITES.
- terminate_pid (~l305) taskkill wrapper: NOT migrated. Its contract is
incompatible with the shared primitive — it must RAISE OSError with
taskkill's stderr on non-zero exit (callers branch on that), falls back
to os.kill on FileNotFoundError, and its POSIX branch is deliberately a
single-PID SIGTERM/SIGKILL, not a tree kill. Wrapping the bool-returning
fail-soft primitive would invert the error contract.
- reap_gateway_children (~l2029): NOT migrated. It operates on a
pre-snapshotted child list from a parent that is already dead
(psutil.Process(pid) on the parent would fail), and every signal is
wrapped in identity/ownership checks the primitive lacks: is_running()
identity, zombie skip, and the skip-if-ppid-still-equals-parent guard,
plus SIGTERM -> wait_procs -> SIGKILL staging and a reaped-count return.
The coupling is the feature; migrating would delete the safety logic.
5. scripts/run_tests_parallel.py _kill_process_tree (~l253): NOT migrated.
Dev tooling that intentionally kills by CAPTURED pgid because the direct
child is usually already reaped (psutil/pid-based primitive cannot find
it), and it avoids the psutil import on the test-runner hot path. Its
docstring already documents why psutil is the wrong tool there.
New tests: tests/agent/test_treekill_consolidation.py — delegation +
raise-swallowing tests per migrated wrapper, consumer-identity checks, and
a live end-to-end probe (setsid grandchild dies through the compat wrapper,
zero survivors).
subprocess.run(capture_output=True, timeout=N) is not hang-safe on
Windows: after the timeout fires, run()'s cleanup kills the direct child
and then joins the pipe reader threads with an UNBOUNDED communicate().
A descendant (conhost.exe under wmic/powershell) holding duplicated pipe
handles keeps the pipes from EOF and the join never returns.
_scan_gateway_pids() runs its wmic / Get-CimInstance Win32_Process scans
exactly that way, and on machines where the full process scan genuinely
exceeds its 10/15s budget (cold WMI on first boot, ARM VMs, heavy
Update/AV activity) hermes update wedged forever inside
_pause_windows_gateways_for_update() before printing a single line —
observed live on a fresh Windows 11 ARM64 VM with a faulthandler stack
pinning the main thread in subprocess._communicate and only a conhost.exe
child surviving. The single-flight update lock then blocks retries until
the wedged process is killed by hand.
This is the same deadlock class bounded_git_probe already fixed for git
probes (#68609 / #66037). Generalize that proven pattern into a shared
bounded_probe_run() — explicit communicate(timeout), kill_process_tree on
failure, bounded 1s drain, then abandon the daemonic readers — and
migrate the whole call-site class onto it:
- hermes_cli/gateway.py _scan_gateway_pids (the site that hung; reached
from hermes update, cron, gateway restart/status, dashboard)
- hermes_cli/dashboard_procs.py wmic scan (same shape, reached on update)
- hermes_cli/claw.py tasklist + PowerShell probes (same shape; its
try/except cannot catch a hang because a hang raises nothing)
- bounded_git_probe now delegates to bounded_probe_run (identical
contract, one copy of the cleanup logic)
Unlike bounded_git_probe, bounded_probe_run returns the CompletedProcess
(or None) rather than collapsing to stdout, because the gateway scan
branches on returncode to trip its wmic -> powershell fallback.
Tests: tests/hermes_cli/test_bounded_probe_run.py covers success,
nonzero-exit passthrough, spawn failure, bounded timeout (fails against
the old unbounded semantics — verified by sabotage), errors= decoding,
DEVNULL stdin, POSIX process-group placement, and the bounded_git_probe
delegation contract. Existing test_git_probe_tree_kill.py passes
unchanged against the delegated implementation.
Closes#87134
Port from openai/codex#37527: Terminate timed-out hook process trees.
A shell hook that forked helpers (scanners, watchers, "cmd &") and then hit
its timeout left those descendants running forever — subprocess.run() only
kills the direct child. Worse, descendants holding the inherited pipe write
ends could stall run()'s post-kill communicate() drain.
- agent/shell_hooks.py _spawn(): spawn hooks in their own process group on
POSIX (process_group=0, Python >=3.11); on timeout/error, reap the whole
tree via the shared kill_process_tree() helper, then drain bounded (1s).
Hooks that complete in time keep their descendants, so intentionally
detached helpers survive successful runs (mirrors codex semantics).
- hermes_cli/_subprocess_compat.py: rename _kill_git_process_tree ->
kill_process_tree (it was never git-specific; taskkill /T /F on Windows,
ownership-gated os.killpg on POSIX). Backward-compat alias retained.
- tests/agent/test_shell_hooks_tree_kill.py: real-subprocess regression
tests (descendant killed on timeout, preserved on success, own-group
spawn, fast-path contract, fail-open). Sabotage-verified: reverting the
process_group spawn fails exactly the two new behavior tests.
Gap proven live on main first: a forking hook timed out at 2s and its
descendant survived; same probe against this branch shows it reaped.
Sweep of open Windows issues affecting day-to-day agent operation
(explicitly excluding install/setup and locale classes):
- hermes_cli/_subprocess_compat.py: new split_command_line() — Windows-
safe command-line tokenizer (posix=False + quote stripping) so
backslash paths survive. POSIX behavior unchanged (plain shlex.split).
- hermes_cli/console_engine.py (#83934): console commands like
'sessions export C:\Users\me\out.jsonl' no longer silently mangle the
path into a relative filename in the cwd.
- agent/shell_hooks.py (#78293): hook commands with backslash paths now
spawn, resolve their script path, and pass hooks doctor instead of
reporting 'not executable'. All three shlex sites routed through the
shared splitter.
- agent/prompt_builder.py (#51755): system prompt now reports
Windows (11) on Windows 11 — platform.release() returns 10 for both;
distinguish via sys.getwindowsversion().build >= 22000.
- hermes_cli/commands.py (#42016): @ autocomplete no longer crashes the
prompt_toolkit event loop when rg emits a path on a different mount
(device paths \.\nul, other drive letters) — relpath ValueError is
skipped per-entry.
- tools/browser_use_cli.py (#83884): screenshot-path detection now
matches Windows drive-letter paths (C:\... and C:/...) in addition to
POSIX; Browser Use screenshots attach on Windows.
- tools/skills_hub.py + tools/skills_guard.py (#62310): the two 'MUST
stay symmetric' skill content hashes actually agree on Windows now.
Bundle keys are normalized to POSIX separators before hashing, and the
disk digest sorts by rel-posix STRING (case-sensitive) instead of Path
objects (case-insensitive on Windows). Fixes permanent false-positive
update_available for every installed skill.
Tests: tests/tools/test_windows_agent_loop_papercuts.py — 16 cases
covering each fix, including a disk-vs-bundle hash symmetry check built
with native Windows separators and a mixed-case filename.
