fix(cli): enforce the ! shell-mode timeout instead of draining to EOF

[briandevans]

What does this PR do?

hermes_cli/bang_shell.py::run_bang_command drains the merged output pipe to EOF on the calling thread, and only then applies the deadline:

if proc.stdout is not None:
    for line in proc.stdout:      # blocks until the PIPE reaches EOF
        emit(line.rstrip("\n"))
proc.wait(timeout=timeout)        # deadline applied only AFTER EOF

for line in proc.stdout returns only when the pipe reaches EOF, and EOF arrives only once the shell and every descendant that inherited the write end via fork() have closed stdout. So proc.wait(timeout=timeout) is reached only after the work is already over: the documented ceiling (DEFAULT_TIMEOUT = 120) is structurally unreachable, and the return 124 branch is dead code in exactly the case it was written for.

The repo states this invariant against itself, twice.

1. bang_shell.py's own comment, directly above DEFAULT_TIMEOUT:

   Bang commands are interactive convenience, not agent work. Keep the ceiling well under the terminal tool's foreground cap: a user watching output can Ctrl+C, and an accidental !sleep 999 should not wedge the composer.

   !sleep 999 wedges the composer for the full 999s today.

2. tools/environments/base.py carries a written post-mortem of this exact construct, for the terminal tool that ! is explicitly modelled on (same approval gate, per this module's docstring):

   The old pattern — for line in proc.stdout — blocks on readline() until the pipe reaches EOF. When the user's command backgrounds a process (cmd &, setsid cmd & disown, etc.), that backgrounded grandchild inherits the write-end of our stdout pipe via fork(). Even after bash itself exits, the pipe stays open because the grandchild still holds it — so the drain thread never returns and the tool hangs for the full lifetime of the grandchild (issue [Bug]: Terminal tool hangs indefinitely when using setsid + disown pattern to launch background services #8340 …).

   The terminal tool was hardened with a bounded drain. ! shell mode reuses none of it. ([Bug]: Terminal tool hangs indefinitely when using setsid + disown pattern to launch background services #8340 is closed and is cited here as precedent for the failure mode, not as the issue this fixes.)

User-visible symptom. A user types !npm run dev & at the composer. In a normal terminal the prompt returns instantly. In Hermes the composer never comes back — no output, no timeout message, indefinitely — because the backgrounded grandchild is holding the pipe the CLI is reading to EOF. The only escape is Ctrl+C, and since the command shared the CLI's process group, that also killed the server they had just launched. Measured before this change, with timeout=3:

command	result
sleep 8	rc=0 after 8.0s — the 3s deadline never fired
(sleep 8 &) ; echo started	rc=0 after 8.0s — the shell exited immediately; the call was held for the grandchild's whole lifetime

Reached by default, with no flag, on every platform, by the ordinary act of backgrounding a process from the composer.

Related Issue

No open issue — found by reading the module against its own stated ceiling.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

hermes_cli/bang_shell.py

• Drain on a daemon thread; wait on the shell. timeout becomes a real wall-clock ceiling instead of a post-EOF formality. This is the shape hermes_cli/main.py::_run_with_idle_timeout already uses in this package — reader on a daemon thread, main thread owning the deadline, rc = 124 on expiry.
• Stop draining once the shell itself is gone. _flush_tail() keeps reading while output is still arriving, so a large tail is never truncated, but gives up after an idle grace (_DRAIN_IDLE_GRACE = 0.3, bounded by _DRAIN_MAX_TAIL = 2.0) rather than waiting on an EOF that a backgrounded grandchild may never deliver. This mirrors the terminal tool's drain in tools/environments/base.py, which stops ~300ms after bash exits for the same reason. The idle window is measured from the last line seen or the shell's exit, whichever is later, so a command that is silent and then prints on its way out is not raced.
• Spawn into a dedicated process group — start_new_session=True on POSIX, CREATE_NEW_PROCESS_GROUP on Windows — and kill the whole group on expiry via _kill_bang_process_tree(). proc.kill() alone signals only the shell wrapper, leaving the grandchildren running and still holding the pipe. This matches how tools/environments/local.py already spawns (start_new_session=True) and kills (_kill_process → os.killpg) the terminal tool's own commands.
• Route KeyboardInterrupt through the same tree kill. tools/environments/base.py gives the reason directly: once a child is in its own process group, letting the interrupt propagate without killing the group reparents it to init and leaves it running as an orphan.
• The Windows creationflags are OR-ed into the existing windows_hide_flags() value, never replacing it — the child still needs CREATE_NO_WINDOW. windows_detach_flags_without_breakaway() is the existing public helper for exactly CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW; CREATE_BREAKAWAY_FROM_JOB is deliberately not used here.
• The drain thread now owns the stream and closes it on its way out. Closing it from the main thread while that thread is blocked inside read() would deadlock on the buffered reader's lock — the very hang class being removed.
• Once control has returned to the composer, later output is discarded but the pipe is not closed. Closing the read end would hand EPIPE to a descendant the user deliberately backgrounded, killing the !npm run dev & server this change exists to keep alive. The cost is one blocked daemon thread, released as soon as that descendant exits.

tests/cli/test_bang_shell_mode.py — new TestBangTimeout (the file previously contained zero occurrences of timeout, 124, or sleep; the 124 path was entirely uncovered).

How to Test

uv run --with pytest --with pytest-asyncio python3 -m pytest tests/cli/test_bang_shell_mode.py -v

Red before / green after, both directions run explicitly. Against unpatched bang_shell.py, all three regression tests fail — and fail for the stated reason, not a mismatched assertion:

E       assert 0 == 124                                            # test_timeout_is_enforced_on_a_silent_command
E       AssertionError: blocked on the grandchild's pipe for 5.0s   # test_returns_when_the_shell_exits_with_a_background_child
E       assert 5.01658833399415 < 3
E       assert 0 == 124                                            # test_timeout_kills_descendants_not_just_the_shell
=========================== 3 failed in 18.40s ============================

After the fix: 55 passed in 11.53s (was 50).

The five tests:

1. test_timeout_is_enforced_on_a_silent_command — run_bang_command("sleep 5", timeout=1) returns 124, emits timed out after 1s, and returns in under 3s.
2. test_returns_when_the_shell_exits_with_a_background_child — the [Bug]: Terminal tool hangs indefinitely when using setsid + disown pattern to launch background services #8340 shape, (sleep 5 &) ; echo started. The generous timeout=60 is deliberate: this pins the stop draining once the shell is gone contract, not the deadline, so it cannot pass merely because the deadline fired.
3. test_timeout_kills_descendants_not_just_the_shell — a grandchild that would touch a marker file 2s in never gets to; POSIX-gated on sys.platform.
4. test_a_backgrounded_descendant_survives_the_composer_returning — the other half of the contract: handing the composer back must not kill what !cmd & launched. Asserts both that no post-return line reaches the writer and that the descendant ran to completion. Fails against a variant that closes the read end, with descendant was killed by the read end closing.
5. test_a_tail_printed_after_a_silence_is_streamed_in_full — not a red-before case, and labelled as such in its docstring. It guards the risk this fix itself introduces: that bounding the drain could truncate output.

Full-suite check, run in a throwaway git worktree so nothing touched the live checkout:

• branch: 1 failed, 935 passed, 1 skipped (tests/cli/)
• clean origin/main @ 372b3b7: 1 failed, 931 passed, 1 skipped

Same single failure both sides — tests/cli/test_resume_quiet_stderr.py::test_session_not_found_goes_to_stdout_in_full_mode, a pre-existing ordering-dependent baseline (it passes in isolation on clean main). Delta is exactly the 4 added tests. Tested on macOS 15 / Python 3.11; the Windows legs are by inspection against the existing _subprocess_compat helpers, so the cross-platform box below is left unchecked.

Sibling-site sweep

Grepped Channel A (cli.py, hermes_cli/, gateway/, tui_gateway/, utils.py) for a live pipe drained by iteration — for line in proc.stdout, iter(proc.stdout, .stdout.readline(). Four sites, one defect:

site	verdict
hermes_cli/bang_shell.py	fixed here — the drain runs on the calling thread and gates a documented timeout
hermes_cli/main.py::_run_with_idle_timeout::_reader	not this defect — the read is already on a daemon=True thread while the main thread owns the deadline in its own proc.wait(timeout=5) loop, so EOF never gates the caller
tui_gateway/host_supervisor.py::_drain_stdout / _drain_stderr	not this defect — dedicated daemon drain threads, and the host is already spawned with start_new_session=True; no deadline depends on EOF
tui_gateway/server.py::_drain_stdout / _drain_stderr	same — started as threading.Thread(..., daemon=True), spawn already windows_hide_flags() + start_new_session=True

hermes_cli/main.py::_run_npm_watching_for_engine_failure iterates proc.stderr on the calling thread as well, but takes no timeout at all and calls a bare proc.wait() — nothing is being defeated there, so it is a different concern and deliberately left alone.

bang_shell.py is the only site in this class where the pipe drain runs on the calling thread and a documented deadline depends on it.

Duplicate check

Text search across the open PR corpus (gh search prs, which reaches all open PRs rather than a recent slice) for every symbol this diff centres on, re-run immediately before pushing: run_bang_command → 0, bang_shell → 0, shell mode timeout → 0, wedge the composer → 0, composer hang → 0. bang shell returns only #76749 (fix(cron): isolate approval context per job), whose only hunk in this file is in bang_shell_enabled() — a different function, no overlap.

Searching the helpers this PR calls into, _kill_git_process_tree and windows_detach_flags_without_breakaway, surfaces open work on hermes_cli/_subprocess_compat.py and tools/environments/local.py (#79235, #69083, #43253, #42868, #43252, #65990). None of them touch hermes_cli/bang_shell.py; this PR only calls those helpers and does not modify either file, so there is no textual overlap in either direction.

git log origin/main --since='30 days ago' -- hermes_cli/bang_shell.py tests/cli/test_bang_shell_mode.py shows a single commit, 9704ed86c13 (the feature itself), so this is not a contested file.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass — ran tests/cli/ in full against a clean-origin/main baseline (see above), not the entire suite
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15 (arm64), Python 3.11.15

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — run_bang_command's docstring now states what timeout guarantees and why it is enforced this way
• 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 — considered and coded for (flags OR-ed into windows_hide_flags(), tree kill via taskkill /T /F, process-group test POSIX-gated), but only executed on macOS, so leaving this unticked rather than claiming Windows verification
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Behaviour change at the composer, with the default 120s ceiling:

before:  !npm run dev &      → composer never returns; Ctrl+C is the only exit,
                               and it kills the dev server too
after:   !npm run dev &      → returns immediately; the shell exited, so we stop
                               draining rather than waiting on the grandchild

before:  !sleep 300          → composer frozen for 300s
after:   !sleep 300          → "!: command timed out after 120s", rc 124

[Copilot]

Pull request overview

Fixes ! shell-mode timeouts in the interactive CLI by ensuring the timeout is enforced as a wall-clock deadline (rather than being defeated by blocking stdout draining), and adds regression tests to cover the previously-dead timeout path.

Changes:

• Reworks run_bang_command() to drain output on a daemon thread while the main thread enforces the timeout, and adds bounded post-exit tail draining.
• Spawns ! commands in a dedicated process group and kills the full process tree on timeout / Ctrl+C.
• Adds a new TestBangTimeout suite covering timeout enforcement, background-child non-blocking behavior, and descendant termination.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
hermes_cli/bang_shell.py	Enforces timeout via main-thread wait + daemon drain; adds bounded tail flush and process-tree kill.
tests/cli/test_bang_shell_mode.py	Adds regression coverage for timeout + backgrounded-child pipe behavior and descendant kill semantics.

---

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

[briandevans]

@copilot All three findings addressed in c68cd1f1336 (current head).

• Closing the read end could kill a backgrounded descendant — correct, and it defeated the point of the PR. Verified before changing anything: with the old break, ( sleep 1; echo late-one; sleep 1; echo late-two; : > marker ) & echo started never reaches the marker, because the drain thread closes the pipe on late-one and late-two takes EPIPE. hermes_cli/bang_shell.py:254 now continues instead — post-deadline output is discarded so nothing lands in the composer after control returned to it, but the read end stays open and the descendant runs to completion. The pipe and its daemon reader are released when that descendant exits, so nothing is leaked past its lifetime. Pinned by test_a_backgrounded_descendant_survives_the_composer_returning (tests/cli/test_bang_shell_mode.py:195), which asserts both halves: no late-* line reaches the writer, and the marker exists. It fails on the pre-fix break with descendant was killed by the read end closing.

• _flush_tail() docstring vs. _DRAIN_MAX_TAIL — fixed at hermes_cli/bang_shell.py:282. It now says the idle grace is what normally ends the drain and that _DRAIN_MAX_TAIL is a hard cap on top of it, with the rationale that output still streaming that long after the shell exited is coming from a descendant rather than the command.

• POSIX shell syntax in the new tests — not changed, deliberately. The surrounding TestBangExecution in this same file already runs echo bang-one; echo bang-two, echo to-stderr >&2 and pwd ungated, so this class doesn't add a new assumption; and .github/workflows/tests.yml runs pytest on ubuntu-latest only, with no Windows matrix. Gating just the class I added would misrepresent the file's actual portability rather than improve it. The two tests that depend on POSIX semantics rather than just shell syntax — process groups and SIGPIPE — are skip-gated on sys.platform, since those would be wrong on Windows even with cross-platform command strings. Making the whole file runnable under cmd.exe looks worth doing, but as its own change.

tests/cli/test_bang_shell_mode.py is green at 55 passed; ruff check and scripts/check-windows-footguns.py --all both clean.

[kvnloo]

Exact-head comment on #81989 head 66407d0.

KEEP wall-clock bang-shell timeout enforced by waiting on the shell while a daemon drain thread reads the pipe — inline for line in proc.stdout cannot honor DEFAULT_TIMEOUT because EOF waits for every descendant that inherited the write end. KEEP _DRAIN_IDLE_GRACE/_DRAIN_MAX_TAIL so !npm run dev & does not wedge the composer after the shell exits. KEEP TestBangTimeout (sleep deadline → 124 + timed-out line; background grandchild returns promptly).

CHECK: closing the read end must not kill a still-running background grandchild the user intentionally detached (author already addressed prior Copilot note at c68cd1f — re-verify on this head). CHECK: writer callbacks stay line-oriented and never insert bang output into conversation history.

Author briandevans not kvnloo. No competing PR from me. Adjacent #98400 is parser/gate tests only — do not remint a second timeout PR.