From 25003b47152cde39773aa84fac150fba1970df11 Mon Sep 17 00:00:00 2001 From: oryn-oryn <290869356+oryn-oryn@users.noreply.github.com> Date: Sun, 7 Jun 2026 18:42:13 +0300 Subject: [PATCH] fix(tools): keep compound-background rewrite valid when a command follows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_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 --- .../test_terminal_compound_background.py | 37 +++++++++++++++++++ tools/terminal_tool.py | 15 +++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_terminal_compound_background.py b/tests/tools/test_terminal_compound_background.py index eeef435772e89..44b4d40a1f271 100644 --- a/tests/tools/test_terminal_compound_background.py +++ b/tests/tools/test_terminal_compound_background.py @@ -66,6 +66,43 @@ def test_multiple_rewrites_in_one_script(self): assert rewrite(cmd) == "A && { B & }\nfalse || { C & }" +class TestTrailingCommandStaysValid: + """A command following the backgrounded job on the SAME line must stay + syntactically valid: ``A && B & C`` rewritten to ``A && { B & } C`` is a + bash syntax error (a brace group needs a separator before the next + command). The rewrite must splice in a ``;``. + """ + + def test_trailing_command_same_line(self): + assert rewrite("A && B & C") == "A && { B & }; C" + + def test_trailing_command_realistic(self): + cmd = "build && python app.py & echo started" + assert rewrite(cmd) == "build && { python app.py & }; echo started" + + def test_trailing_command_no_space_after_amp(self): + assert rewrite("A && B &C") == "A && { B & }; C" + + def test_rewritten_output_is_valid_bash(self): + # The actual defect: the pre-fix output failed `bash -n`. + import subprocess + + for cmd in ( + "echo a && sleep 100 & echo done", + "A && B & C", + "build && run & tail -f log", + ): + out = rewrite(cmd) + proc = subprocess.run( + ["bash", "-n", "-c", out], capture_output=True, text=True + ) + assert proc.returncode == 0, f"{out!r} failed bash -n: {proc.stderr}" + + def test_trailing_command_idempotent(self): + once = rewrite("A && B & C") + assert rewrite(once) == once + + class TestPreserved: """Commands that DON'T have the bug MUST pass through unchanged.""" diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 3e81eff9f6763..504af68fdcd5e 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -744,7 +744,20 @@ def _rewrite_compound_background(command: str) -> str: suffix = result[amp_pos + 1 :] # `{` needs a trailing space in bash; the closing `}` needs to be # preceded by `;` or `&` — we're providing `&` from the backgrounding. - result = prefix + "{ " + middle + "& }" + suffix + # + # A brace group must also be *followed* by a separator before any + # further command on the same line: `A && B & C` would otherwise + # become `A && { B & } C`, which bash rejects with + # "syntax error near unexpected token". When a command directly + # follows the backgrounded job (`& C`), splice in a `;` so the + # rewrite stays valid. Tails that already begin with a separator + # — a newline-delimited next statement, or a `&&`/`||`/`|` chain — + # need nothing. + tail = suffix.lstrip(" \t") + if tail and tail[0] not in (";", "&", "|", "\n", ")"): + result = prefix + "{ " + middle + "& }; " + tail + else: + result = prefix + "{ " + middle + "& }" + suffix return result