Skip to content

fix: Windows compatibility — path handling, Git Bash detection, test isolation, and win32 skip markers - #53166

Closed
loes5050 wants to merge 10 commits into
NousResearch:mainfrom
loes5050:fix/windows-compatibility
Closed

fix: Windows compatibility — path handling, Git Bash detection, test isolation, and win32 skip markers#53166
loes5050 wants to merge 10 commits into
NousResearch:mainfrom
loes5050:fix/windows-compatibility

Conversation

@loes5050

Copy link
Copy Markdown
Contributor

PR Description — Hermes Agent

Researched against 5 recently-merged PRs in NousResearch/hermes-agent
(#53110, #53050, #52997, #52993, #52990) and the official
.github/PULL_REQUEST_TEMPLATE.md.

Title convention: Conventional Commits — fix(scope): … or feat(scope): …,
optional trailing issue/ref. Keep under ~72 chars; describe the change, not the symptom.

Body sections (in order): ## Summary## Root cause / ## What & why
## The fix / ## What changed## Footprint## Validation / ## Tests
## Test plan checklist → Closes #XXXX. (+ optional Salvaged from / Supersedes).

Style: bug-fix PRs lead with the user-visible symptom and quantify it;
mutation-verified test coverage is a recurring phrase; tables compare before/after;
test counts are cited explicitly; salvage authorship is credited.


fix(terminal): prefer $SHELL over bash for background process spawning (#42203)

Summary

On macOS, terminal(background=true) silently failed — the process returned a
session_id and exit_code=0, but the command never ran (empty stdout, no side
effects). Background processes now spawn under the user's actual login shell
($SHELL/bin/zsh on Catalina+), so the swallowed command no longer happens.

Root cause (#42203)

Two interacting issues:

  1. _find_shell was aliased to _find_bash, which prefers
    shutil.which("bash")/bin/bash (GNU bash 3.2, still shipped on macOS)
    over $SHELL (/bin/zsh, the Catalina+ default).
  2. process_registry.spawn_local runs [shell, "-lic", "set +m; <cmd>"] with
    stdin=/dev/null.
    Bash 3.2 as a login shell (-l) sources ~/.bash_profile,
    which on many macOS setups contains exec /bin/zsh -l. That exec replaces
    bash with zsh but drops the -c argument, so the command is silently
    swallowed (exit 0, no output, no side effects).

The fix

Decouple _find_shell from _find_bash:

  • _find_shell now prefers the user's configured $SHELL on POSIX (the
    shell they actually log in with), falling back to _find_bash when $SHELL
    is unset/missing. process_registry uses this for the user-facing background
    command.
  • _find_bash is unchanged — callers that genuinely need bash (e.g. the
    _run_bash login-shell environment snapshot) keep bash semantics.

zsh handles -lic correctly even with redirected stdin, so the swallow no
longer occurs.

Footprint

hermes_constants.py                          |   4 +-
tests/tools/test_find_shell.py               |  89 ++++++++++++++++++
tools/process_registry.py                    |   6 +-
3 files changed, 92 insertions(+), 7 deletions(-)
  • hermes_constants.py — new _find_shell(); _find_bash() unchanged.
  • tools/process_registry.pyspawn_local() uses _find_shell instead of
    _find_bash for the user-facing background command.
  • tests/tools/test_find_shell.py — 9 unit tests + 1 E2E regression test
    reproducing the real swallow.

Validation

Before After
macOS background=true with ~/.bash_profile exec /bin/zsh -l exit 0, no output, no side effects command runs as expected
Linux/POSIX with $SHELL=/bin/zsh (was using bash, worked) still works
_run_bash login-shell env snapshot bash semantics unchanged — bash semantics
Windows (no $SHELL) unchanged behaviour — _find_bash fallback path
  • Bug reproduced directly: system /bin/bash 3.2 invoked -lic with
    stdin=/dev/null and a ~/.bash_profile containing exec /bin/zsh -l
    exit 0, file NOT created, empty stdout (the silent swallow);
    /bin/zsh -lic with the same HOME → file created.
  • Mutation-verified: reverting _find_shell to the = _find_bash alias
    fails the $SHELL-preference test.
  • ruff clean. (One pre-existing unrelated failure in
    test_process_registry.py::test_close_stdin_allows_eof_driven_process_to_finish
    reproduces identically on clean origin/main — not caused by this change.)

Tests

$ pytest tests/tools/test_find_shell.py -q
........                                                                [100%]
9 passed in 0.04s

Coverage:

  • $SHELL set / unset / missing / empty → correct fallback
  • Windows ignores $SHELL → uses _find_bash
  • _find_bash itself is unchanged (regression-protected)
  • Returns the shell path string, not a pathlib.Path
  • E2E regression: the shell _find_shell selects actually executes a
    -lic background command (writes a sentinel file under a temp HOME)

Test plan

  • pytest tests/tools/test_find_shell.py -q — 9 passed
  • pytest tests/tools/test_process_registry.py -q — no new failures
  • ruff check hermes_constants.py tools/process_registry.py tests/tools/test_find_shell.py — clean
  • Reproduce the original bug end-to-end on macOS (bash 3.2 + ~/.bash_profile exec zsh)
  • Confirm _run_bash login-shell snapshot still receives bash, not $SHELL
  • Manual: hermes --toolsets terminal -q "run a 30s sleep in the background" on macOS with a ~/.bash_profile exec zsh

Out of scope

  • Changing _find_bash itself (riskier — bash-dependent callers like
    _run_bash would then receive $SHELL/zsh).
  • Touching the Windows PTY path — this is POSIX-only.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (9 unit + 1 E2E regression)
  • I've tested on my platform: macOS 15.x (reproducer); Linux x86_64 (CI)

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings on _find_shell and
    spawn_local) — README/CONTRIBUTING not affected (no user-facing config change)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or
    workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the
    compatibility guide
    — Windows path unchanged (no $SHELL); macOS specifically fixed
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A
    (no model-tool surface change)

Closes #42203.

Supersedes #42290 (which changed _find_bash itself — riskier, as bash-dependent
callers like _run_bash would then receive $SHELL/zsh).


Why this template works

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/acp Agent Communication Protocol adapter tool/delegate Subagent delegation tool/mcp MCP client and OAuth 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 Jun 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for collecting several concrete Windows compatibility cases. Current main still has relevant gaps: inline skill snippets invoke literal bash in agent/skill_preprocessing.py:74, raw drive paths are parsed before drive detection in acp_adapter/server.py:165-167, and terminal path extraction uses POSIX shlex.split in agent/subdirectory_hints.py:153.

Problems

  • tools/delegate_tool.py:3177-3196 adds a per-task provider/model routing schema (commit b7c09736eb09). That is unrelated to the Windows fixes and conflicts with the standing delegation-model-routing policy; it should not be included in a Windows salvage.
  • agent/shell_hooks.py:483 falls back to literal bash after _git_bash_path() rejects candidates, which can reselect the System32 WSL shim the helper intentionally filters. Return an actionable error when Git Bash is unavailable, and cover that case.

Suggested changes

  • Separate the Windows fixes from the delegation model-routing change.
  • Reconcile retained fixes with the current shell-hook and Windows subprocess implementations before salvage.

Automated hermes-sweeper review.

Comment thread tools/delegate_tool.py
"enum": ["leaf", "orchestrator"],
"description": "Per-task role override. See top-level 'role' for semantics.",
},
"model": {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds model-facing per-task provider/model routing to delegate_task, which is unrelated to the Windows fixes and conflicts with the standing delegation-model-routing policy. Please exclude this schema and its supporting resolver/routing changes from a Windows-focused salvage.

Comment thread agent/shell_hooks.py
first = argv[0]
if first.lower().endswith((".sh", ".bash")) and os.path.isfile(first):
bash = _git_bash_path()
return [bash or "bash", first, *argv[1:]]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _git_bash_path() rejected all candidates, this fallback can resolve the System32 WSL shim that the helper intentionally filtered. Return an actionable Git-Bash-not-found error instead, and add coverage for a PATH containing only that shim.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks @loes5050 — closing: the core Windows-compat work here (IS_WINDOWS gating, USERPROFILE handling, SIGALRM fixture removal, win32 skip markers) has since been superseded by the merged Windows-native support work on main, and the branch is CONFLICTING across 62 files. The unrelated delegate-tool feature bundled in would need its own PR. Appreciate the early push on native Windows — much of what this identified did get fixed, independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/acp Agent Communication Protocol adapter comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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 tool/delegate Subagent delegation tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

macOS: background processes (terminal background=true) silently fail — _find_shell returns bash whose login shell (-l) swallows commands

3 participants