Skip to content

fix(tools): don't compound-rewrite spawn_via_env background wrappers - #36023

Merged
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:fix/spawn-via-env-bg-wrapper
May 31, 2026
Merged

fix(tools): don't compound-rewrite spawn_via_env background wrappers#36023
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:fix/spawn-via-env-bg-wrapper

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Salvage of #33756 by @charzhou — fixes background tasks failing to launch on non-local terminal backends (SSH/Docker/Modal/Daytona/Singularity). Extracted as a focused, one-concern PR from the original multi-feature branch (the unrelated image_gen / docker-media changes are intentionally not included).

The bug

Background tasks on non-local backends go through ProcessRegistry.spawn_via_env, which builds a hand-crafted, shell-safe wrapper:

mkdir -p T && ( nohup bash -lc CMD > LOG 2>&1; rc=$?; printf '%s\n' "$rc" > EXIT ) & echo $! > PID && cat PID

BaseEnvironment.execute() unconditionally ran _rewrite_compound_background on every command — including this wrapper. That rewrite (which defuses the A && B & subshell-wait trap for user commands) turns ( ... ) & echo $! into:

... && { ( ... ) & } echo $!
                   ^^^ `} echo` — no separator → bash syntax error

The wrapper then never produces a PID, the redirected output file is never created, and the agent sees an immediate exit code -1. This breaks every background launch on a non-local backend (e.g. a simple count-and-redirect script over SSH), which is why it reproduces on trivial setups.

Fix

  • Add rewrite_compound_background: bool = True to BaseEnvironment.execute() (and the BaseModalExecutionEnvironment override, which accepts and ignores it). Default preserves existing behavior; the user foreground terminal path still rewrites.
  • spawn_via_env passes rewrite_compound_background=False so its already shell-safe wrapper is left intact.
  • Treat a wrapper that produces no PID as a failed launch (mark the session exited with a real exit code instead of exposing a fake running session), and don't register/checkpoint a session that never started.

Changes vs #33756

  • Scoped to the background fix only (tools/process_registry.py, tools/environments/base.py, tools/environments/modal_utils.py + tests). The image_gen base_url/key_env overrides and docker-media-path changes from the original branch are dropped — those are separate concerns.
  • Removed a duplicate session.exited = True in the PID-failure guard.
  • Added a regression assertion that a failed launch is not added to _running.
  • Added @CharZhou to scripts/release.py::AUTHOR_MAP so release contributor attribution resolves the co-author trailer.

Testing

tests/tools/test_process_registry.py -k spawn_via_env   → 3 passed
tests/tools/test_terminal_compound_background.py        → 34 passed
ruff check (changed files)                              → clean
ty (changed regions)                                    → no new diagnostics

Verified empirically with the exact wrapper shape: skipping the rewrite produces valid bash that launches the process, captures the PID, and writes the log/pid/exit files; the old rewritten form fails bash -n with a syntax error.

Based on #33756 by @charzhou.

Background tasks on non-local backends (SSH/Docker/Modal/Daytona/Singularity)
go through `ProcessRegistry.spawn_via_env`, which builds a hand-crafted,
shell-safe wrapper:

    mkdir -p T && ( nohup bash -lc CMD > LOG 2>&1; rc=$?; ... ) & echo $! > PID && cat PID

`BaseEnvironment.execute()` unconditionally ran `_rewrite_compound_background`
on every command, including this wrapper. The rewrite (meant to defuse the
`A && B &` subshell-wait trap for user commands) turns `( ... ) & echo $!` into
`{ ( ... ) & } echo $!` — note `} echo` with no separator, which is a bash
syntax error. The wrapper then never produces a PID, the redirected output file
is never created, and the agent sees an immediate exit code -1. This breaks
*every* background launch on a non-local backend (e.g. a simple
count-and-redirect script over SSH), not just edge cases.

Fix:
- Add `rewrite_compound_background: bool = True` to `BaseEnvironment.execute()`
  (and the `BaseModalExecutionEnvironment` override, which accepts and ignores
  it). Default preserves existing behavior; the user foreground terminal path
  still rewrites.
- `spawn_via_env` passes `rewrite_compound_background=False` so its already
  shell-safe wrapper is left intact.
- Treat a wrapper that produces no PID as a failed launch (mark the session
  exited with a real exit code instead of exposing a fake running session), and
  don't register/checkpoint a session that never started.

Verified empirically: with the rewrite skipped, the wrapper is valid bash,
launches the process, captures the PID, and writes the log/pid/exit files; the
old rewritten form fails `bash -n` with a syntax error.

Based on NousResearch#33756 by @charzhou (extracted from a multi-feature branch; the
unrelated image_gen / docker-media changes are not included here).

Co-authored-by: CharZhou <17255546+CharZhou@users.noreply.github.com>
@kshitijk4poor
kshitijk4poor enabled auto-merge May 31, 2026 18:39
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management backend/ssh SSH remote execution backend/docker Docker container execution backend/modal Modal.com cloud execution labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Supersedes the background-fix portion of #33756 (scoped to spawn_via_env compound-rewrite only, excludes unrelated image_gen/docker-media changes). Also supersedes closed #33754.

@kshitijk4poor
kshitijk4poor merged commit 59cc7c3 into NousResearch:main May 31, 2026
30 of 31 checks passed
alt-glitch pushed a commit that referenced this pull request Jun 14, 2026
…pper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
T02200059 pushed a commit to T02200059/hermes-agent that referenced this pull request Jun 18, 2026
…a-env-bg-wrapper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…a-env-bg-wrapper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
…a-env-bg-wrapper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…a-env-bg-wrapper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
@kshitijk4poor
kshitijk4poor deleted the fix/spawn-via-env-bg-wrapper branch August 5, 2026 07:08
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
…a-env-bg-wrapper

fix(tools): don't compound-rewrite spawn_via_env background wrappers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/docker Docker container execution backend/modal Modal.com cloud execution backend/ssh SSH remote execution comp/tools Tool registry, model_tools, toolsets P1 High — major feature broken, no workaround tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants