fix(tools): background commands

[elisam0]

What does this PR do?

Makes terminal background-command handling safe and predictable. Shell-level & backgrounding can cause Hermes to track the shell rather than the actual child process, preventing reliable polling, waiting, and output streaming.

The terminal tool now recognizes real, unquoted shell background operators while ignoring quoted text, comments, redirects, and heredoc bodies. A single trailing & is normalized into Hermes-managed background=true execution with a warning. Internal backgrounding in compound commands, or an & used together with background=true, is rejected with instructions to split the work into managed terminal calls.

Related Issue

Related PRs:

• #63788 fixes false positives from & inside heredoc bodies. This PR includes the same class of heredoc-aware detection, but also normalizes one final top-level & into Hermes-managed background=true execution and rejects unsafe internal or double backgrounding.
• #68948 removes the existing compound-background command rewriter after finding it corrupts valid shell input. This PR does not rewrite compound commands; it rejects internal shell & backgrounding and asks the model to split the work into separately managed terminal calls.

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

• Added lexical parsing helpers in tools/terminal_tool.py for heredoc declarations and top-level shell & operators.
• Excluded quoted values, comments, file-descriptor redirects, &&, &>, and heredoc bodies from backgrounding detection.
• Converted one unambiguous final & into managed background=true execution and returned a warning explaining the normalization.
• Rejected internal shell backgrounding and background=true plus shell &, where Hermes cannot safely track the process.

How to Test

1. Run the focused terminal-tool regression tests once added (this branch currently does not add automated coverage for the new parser).
2. Call terminal(command="sleep 1 &") and verify it is started as a managed background command with a normalization warning.
3. Call terminal(command="sleep 1 & echo done") and verify it is rejected with guidance to split it into separate calls.
4. Verify commands containing & in quotes, comments, redirects, or heredoc content are not mistaken for backgrounding.

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
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Linux (benchmark sandbox/container)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

[spfcraze]

This was generated by AI during triage.

A defect in the change:

The new _shell_background_operator_positions classifies Bash's |& pipe operator as a job-control &, so cmd |& tee log is rejected with "Command contains internal shell '&' backgrounding" (exit -1) even with background=true, while current main runs it.

Problems:

• _shell_background_operator_positions excludes only &&, &>, and & preceded by </>; the & of |& (pipe both stdout and stderr, Bash 3+) is recorded as an operator. Verified by running the function from the diff: cmd |& cmd2 yields one position, it is not final, and the new reject block returns exit -1 before execution.
• This is a regression, not pre-existing behavior: the current detector on main matches \s&\s (whitespace before &), so |& never matched and such commands ran.
• A trailing cmd |& is "normalized" by stripping the &, producing cmd | — a bash syntax error — instead of being rejected.

Suggested changes:

• Skip & preceded by | alongside the existing <> check, so |& is treated as a pipe operator rather than backgrounding.
• Add a parametrized parser case for cmd |& cmd2 next to the existing && / &> / 2>&1 exclusions.

---

Checked against e4932cd — the tip of fix/background-commands when this was written — and 1be70d6, main at the same moment. Verified mechanically: the diff grep at the former; the code on main at the latter. The reading of intended scope is inference. If I have misread the intent here, please say so.

[kshitijk4poor]

Reviewed against current main (241605d), with the PR applied to a fresh worktree and exercised end-to-end (side-by-side against clean main). The heredoc-aware direction is genuinely good — heredoc bodies are data and the current regexes do false-positive on them — but as it stands this PR rejects or silently rewrites several valid commands that run fine on main today, so it can't merge as-is.

Confirmed regressions (each verified live, PR vs clean main):

1. Arithmetic &: x=$((5&3)); echo $x — runs on main, hard-rejected by the PR as "internal shell '&' backgrounding". Same for $((FLAGS & MASK)).
2. Bash |& pipe-stderr: make |& tee build.log — runs on main, rejected.
3. case fallthrough ;& / ;;&: rejected as internal backgrounding.
4. Parallel fan-out idiom with background=true: job1 & job2 & wait ran fine on main as a managed background call; the PR rejects it in both modes, killing single-call parallelism.
5. Backgrounded heredoc: python3 - <<'PY' & … — positions are computed on the heredoc-stripped string but finality is checked on the raw command, so the body text after & makes it "internal" → rejected. (And if it were normalized, _normalize_final_shell_background_operator returns only the prefix, dropping the heredoc body entirely.)

Design concerns:

• Silent auto-flip changes deliberate behavior. python3 -m http.server & (foreground) is blocked with guidance on main by design — the model must consciously re-send with background=true. The PR silently converts it to a managed background process, bypassing FOREGROUND_MAX_TIMEOUT and the long-lived-process guidance; a caller-supplied timeout is quietly repurposed. Also A; B & backgrounds only B in a real shell, but the PR backgrounds the whole compound — a semantic change delivered as a warning.
• Conflicts with live machinery. Main's _rewrite_compound_background (the #68915 subshell-deadlock fix, called from environments/base.py and process_registry.py) becomes dead/contradictory on the terminal_tool path — the PR strips/rejects every & before the rewriter can see one, without removing or reconciling it. There's also open PR #68948 proposing to retire that rewriter, and #63788 which fixes the same heredoc false-positive class more narrowly.
• No tests for ~286 lines of shell lexing, and the quote/comment scanning loop is written three times with already-divergent behavior (_read_shell_token exists at terminal_tool.py:622 for this).
• The lexer runs 2× (plus 3–4 heredoc-strip string rebuilds) on every terminal call — the hottest tool path in the agent.

Suggested path forward: the heredoc-masking insight is worth landing — but scoped to fixing the existing detection false-positives (what #63788 does), not adding a normalize/reject layer. If auto-normalization of a lone trailing & is wanted, it needs a design pass coordinated with #68948 (rewriter retirement), an &-classifier shared with _rewrite_compound_background instead of a parallel one, tests covering the shell grammar corners above, and an explicit decision on the auto-flip vs. block-with-guidance behavior.

Happy to review a narrowed v2. Thanks for the thorough writeup in the PR body — the related-PR analysis made this review much easier.

[rroverin]

Thank you for the feedback @kshitijk4poor !
We can propose a narrowed PR to start with only the heredoc false positive fixes.
Is your plan to merge 63788 and should we add our potential changes and tests there, or shall we continue here?

[kshitijk4poor]

Good question — here's the plan on our side. The heredoc false-positive fix will land via #63788, since it's the earliest focused fix for that class — but not as-is: the review on that PR flagged bypass concerns (fake << markers in comments/quotes eating a real &; stripping shell-consumed bodies like bash <<'EOF' that are executable), so it'll be salvaged onto current main with a more conservative masking pass, and factored as a shared helper — the same root cause exists in the blocked-command checks (#83104) and the gateway-lifecycle guard (#81721/#79835), so one helper should serve all of them.

So please don't narrow this PR down to the heredoc fix — that would just duplicate #63788 and cost you both attribution. What would genuinely help:

1. Edge-case tests. You clearly explored the shell grammar corners here (|&, ;&, arithmetic, redirects). Bringing those cases as review comments on the fix(tools): strip heredoc bodies before background-'&' detection #63788 salvage once it's up would be very welcome and credited.
2. The trailing-& normalization — the part unique to this PR — needs a design conversation before code: whether auto-flipping to managed background should replace block-with-guidance at all, a single & classifier shared with (or replacing, per fix(terminal): retire the compound-background rewriter #68948) the compound rewriter, and what happens to caller-supplied timeouts. If you want to drive that, open it as a proposal issue and we can hash out the contract first.

I'd keep this PR open until the salvage lands (it's a useful reference), then close it with a pointer.

[kshitijk4poor]

Update: the salvage described above is now up as #84402 — #63788's fix cherry-picked onto current main (authorship preserved) plus a hardening commit that makes the heredoc masking conservative (quoted+terminated+single-command+non-shell-consumer only) and factors it into a shared tools/shell_heredoc.py module so the sibling guards (#83104, #81721/#79835) can adopt the same implementation.

@rroverin @elisam0 — edge-case review on #84402 is very welcome; you clearly explored the shell grammar corners here. The trailing-& normalization design conversation remains open as described above.

[kshitijk4poor]

Closing in favor of #84402, which lands the heredoc false-positive fix this PR targeted (via a salvage of the earlier #63788, hardened into a shared conservative masking module). The verified regressions listed in the review above (arithmetic &, |&, ;&, fan-out with background=true, backgrounded heredocs) plus the auto-flip design concerns mean the normalize/reject layer here can't merge as-is.

The trailing-& normalization idea — the part unique to this PR — remains open as a design conversation: if you want to drive it, please open a proposal issue covering the auto-flip vs block-with-guidance decision, a single & classifier shared with (or replacing, per #68948) the compound rewriter, and caller-supplied timeout semantics. Edge-case review on #84402 is also very welcome. Thanks @elisam0 @rroverin for the thorough related-PR analysis in the writeup — it made the review much easier.

[rroverin]

Thank you @kshitijk4poor . We have done the edge case review discussed above in a new PR ready for review (small fix and some missing tests added): #89994 .
We will come back regarding a potential plan for the trailing-& normalization idea.