Skip to content

fix(tools): keep compound-background rewrite valid when a command follows - #41368

Closed
oryn-oryn wants to merge 1 commit into
NousResearch:mainfrom
oryn-oryn:fix/compound-background-trailing-command
Closed

oryn-oryn wants to merge 1 commit into
NousResearch:mainfrom
oryn-oryn:fix/compound-background-trailing-command

Conversation

@oryn-oryn

Copy link
Copy Markdown

_rewrite_compound_background turns A && B & into A && { B & } to
avoid the subshell-wait process leak. But when a command follows the
backgrounded job on the same line (A && B & C), the rewrite produced
A && { B & } C — a bash syntax error, since a brace group needs a
separator before the next command. Valid commands like
build && python app.py & echo started were rejected with
"syntax error near unexpected token". Splice in a ; when the tail is a
same-line command; tails that already start with a separator (newline,
&&/||/|) are untouched.

What does this PR do?

Fixes _rewrite_compound_background emitting invalid bash when a command
trails the backgrounded job on the same line.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/terminal_tool.py: in _rewrite_compound_background, insert ;
    after the brace group when a same-line command follows; leave tails
    beginning with a separator unchanged.
  • tests/tools/test_terminal_compound_background.py: add regression tests
    for trailing same-line commands, bash -n validity, and idempotence.

How to Test

  1. from tools.terminal_tool import _rewrite_compound_background as r
  2. r("A && B & C") → "A && { B & }; C" (was "A && { B & } C").
  3. bash -n -c "$(echo a && sleep 100 & echo done)" style output now
    passes; pre-fix it failed.
  4. pytest tests/tools/test_terminal_compound_background.py -q → 39 passed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the affected tests and they pass
  • I've added tests for my changes
  • I've tested on my platform: macOS 15 (Darwin 25.5)

Documentation & Housekeeping

  • I've updated relevant documentation — N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md — N/A
  • I've considered cross-platform impact — pure string logic, platform-agnostic
  • I've updated tool descriptions/schemas — N/A

…lows

`_rewrite_compound_background` turns `A && B &` into `A && { B & }` to
avoid the subshell-wait process leak. But when a command follows the
backgrounded job on the same line (`A && B & C`), the rewrite produced
`A && { B & } C` — a bash syntax error, since a brace group needs a
separator before the next command. Valid commands like
`build && python app.py & echo started` were rejected with
"syntax error near unexpected token". Splice in a `;` when the tail is a
same-line command; tails that already start with a separator (newline,
`&&`/`||`/`|`) are untouched.

## What does this PR do?

Fixes `_rewrite_compound_background` emitting invalid bash when a command
trails the backgrounded job on the same line.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)

## Changes Made

- `tools/terminal_tool.py`: in `_rewrite_compound_background`, insert `;`
  after the brace group when a same-line command follows; leave tails
  beginning with a separator unchanged.
- `tests/tools/test_terminal_compound_background.py`: add regression tests
  for trailing same-line commands, `bash -n` validity, and idempotence.

## How to Test

1. `from tools.terminal_tool import _rewrite_compound_background as r`
2. `r("A && B & C")` → `"A && { B & }; C"` (was `"A && { B & } C"`).
3. `bash -n -c "$(echo a && sleep 100 & echo done)"` style output now
   passes; pre-fix it failed.
4. `pytest tests/tools/test_terminal_compound_background.py -q` → 39 passed.

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains only changes related to this fix
- [x] I've run the affected tests and they pass
- [x] I've added tests for my changes
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)

### Documentation & Housekeeping

- [x] I've updated relevant documentation — N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` — N/A
- [x] I've considered cross-platform impact — pure string logic, platform-agnostic
- [x] I've updated tool descriptions/schemas — N/A
@alt-glitch alt-glitch added type/bug Something isn't working tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels Jun 7, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for isolating a real terminal rewrite bug. Current main still concatenates the suffix directly after the closing brace at tools/terminal_tool.py:866, and BaseEnvironment.execute() applies that rewriter by default at tools/environments/base.py:915-917.

Problems

  • tests/tools/test_terminal_compound_background.py:96 invokes bash unconditionally. Bash is not guaranteed on native Windows or minimal test environments; existing bash-dependent tests skip when shutil.which("bash") is unavailable (for example tests/tools/test_base_environment.py:222-224).

Suggested changes

  • Add that availability guard around the bash -n test. Keep the string-level regression assertions platform-independent.

The targeted production hunk remains unchanged on current main despite the PR's age, so the fix should otherwise salvage cleanly. This is an automated hermes-sweeper review.

"build && run & tail -f log",
):
out = rewrite(cmd)
proc = subprocess.run(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please skip this bash-dependent assertion when shutil.which("bash") is unavailable. Existing tests use that guard, and without it this regression test fails on native Windows or minimal environments before it can assert rewrite behavior.

@james47kjv

Copy link
Copy Markdown
Contributor

Supporting datapoint: this fix is still needed, still applies, and still works at current HEAD.

Verified against main @ 4c9f130bf9a6d0af196f3e94375d7834871f5ab8:

  • git apply --check of this PR's diff passes cleanly — no conflicts, even though _rewrite_compound_background has moved within tools/terminal_tool.py since the PR was opened.

  • Unpatched: pytest tests/tools/test_terminal_compound_background.py -q → 15 passed; adding only this PR's tests → 4 failed (the new TestTrailingCommandStaysValid syntax cases), confirming they still catch the live bug.

  • Patched: all 20 tests pass, and the remote session-kernel spawn template from tools/code_kernel_remote.py now rewrites to valid bash:

    cd /tmp/k && { nohup env X=1 python3 kernel_runner.py > /tmp/k/runner.log 2>&1 & }; echo PID:$!
    

    bash -n accepts it, and at runtime the trailing echo PID:$! prints the backgrounded runner's real PID — which is exactly what _spawn_remote_kernel needs to get a persistent kernel up.

Impact evidence (same failure as #98222): on an ssh terminal backend we observed 893 consecutive kernel spawn failures with zero successes — every spawn died with syntax error near unexpected token 'echo', no PID came back, and every execute_code call silently fell back to "remote session kernel unavailable on ssh; using per-call path". That downgrades every agent on an ssh backend to stateless per-call execution, defeating the documented purpose of the persistent kernel (variables surviving between execute_code calls).

The one change requested in review — skipping the bash-dependent assertion when shutil.which("bash") is unavailable, per the existing pattern in tests/tools/test_base_environment.py:195 — is a few lines. @oryn-oryn, if you're short on time, happy to supply that guard so this can land.

🤖 Generated with Claude Code

@kshitijk4poor

Copy link
Copy Markdown
Contributor

Fixed via #101662 (salvage of #42278 — the earliest PR on this rewrite and the one with the full separator set; the ;; case-arm suffix must not gain a ;, which the semicolon-restoration variants miss). Your PR and @james47kjv's re-verification against current HEAD confirmed the bug was still live — credited in the salvage body. Closing as superseded; thanks.

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

Labels

P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit 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 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.

5 participants