fix: avoid compound rewrite on env-based spawn_via_env by charzhou · Pull Request #33756 · NousResearch/hermes-agent

charzhou · 2026-05-28T08:48:21Z

Summary

Prevent BaseEnvironment.execute() from reapplying compound background rewrite for spawn_via_env wrapper commands.
Pass rewrite_compound_background=False in ProcessRegistry.spawn_via_env.
Mark spawn as failed when PID is not produced instead of exposing a fake running background session.
Add registry-level regression tests for both launch-failure and rewrite-bypass behaviors.

Testing

./.venv/bin/python -m pytest tests/tools/test_process_registry.py tests/tools/test_terminal_compound_background.py -q

Result

100 passed

@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 #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 · 2026-05-31T19:14:04Z

Thanks @charzhou — the background-process fix here was salvaged and merged via #36023, extracted as a focused one-concern PR (the unrelated image_gen base_url/key_env and docker-media changes from this branch were left out, since they're separate concerns). The salvage also dropped a duplicate session.exited = True, added a regression assertion that a failed launch isn't added to _running, and added you to AUTHOR_MAP. You're credited as co-author. Closing in favor of #36023.

@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 #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>

@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>

@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>

@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>

@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>

@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>

@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>

@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>

@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>

charilezhou and others added 5 commits May 28, 2026 10:53

fix: support docker workspace media delivery paths

2388723

feat(image_gen): support openai base_url override

e313f2f

feat(image_gen): support openai key_env override

3351efa

Merge branch 'NousResearch:main' into main

29a22b4

fix: avoid compound rewrite on spawn_via_env background wrappers

780ec2c

alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels May 28, 2026

alt-glitch mentioned this pull request May 28, 2026

fix(process-registry): avoid background rewrite on env-based spawn #33754

Closed

kshitijk4poor mentioned this pull request May 31, 2026

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

Merged

kshitijk4poor closed this May 31, 2026

Navigation Menu

Uh oh!

fix: avoid compound rewrite on env-based spawn_via_env - #33756

fix: avoid compound rewrite on env-based spawn_via_env#33756
charzhou wants to merge 5 commits into
NousResearch:mainfrom
charzhou:codex/fix-docker-background-rewrite-pr-clean

charzhou commented May 28, 2026

Uh oh!

kshitijk4poor commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

charzhou commented May 28, 2026

Summary

Testing

Result

Uh oh!

kshitijk4poor commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants