Skip to content

wrap bash compound commands in bash -c before nohup#312202

Merged
meganrogge merged 3 commits intomainfrom
merogge/nohup
Apr 23, 2026
Merged

wrap bash compound commands in bash -c before nohup#312202
meganrogge merged 3 commits intomainfrom
merogge/nohup

Conversation

@meganrogge
Copy link
Copy Markdown
Collaborator

nohup can only exec external commands — it cannot run bash compound
statements (for, while, if, case, eval, export, etc.) or
shell builtins directly. Commands like:

nohup for i in $(seq 1 90); do echo $i; sleep 1; done &

fail silently because for is not an executable.

Adds _needsBashCWrapper() to detect compound constructs and builtins,
wrapping them as:

nohup bash -c 'for i in $(seq 1 90); do echo $i; sleep 1; done' &

Single quotes inside the command are escaped with the POSIX '\''
pattern.

Fixes eval regressions on terminal_long_running_prompt (confirmed
broken nohup rewrite) and improves reliability of all background
terminal commands involving shell syntax.

Only affects POSIX paths when chat.tools.terminal.detachBackgroundProcesses
is enabled (experimental, default off).

Copilot AI review requested due to automatic review settings April 23, 2026 19:09
@meganrogge meganrogge self-assigned this Apr 23, 2026
@meganrogge meganrogge added this to the 1.118.0 milestone Apr 23, 2026
@meganrogge meganrogge enabled auto-merge (squash) April 23, 2026 19:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the terminal background-detach command rewriter so that POSIX background commands using shell compound syntax or builtins (which nohup can’t exec directly) are wrapped in bash -c '...' before being passed to nohup, and adds coverage for these cases.

Changes:

  • Add _needsBashCWrapper() detection for compound commands/builtins and wrap with bash -c '...' (including single-quote escaping).
  • Adjust POSIX rewrite logic to run nohup against the wrapped command.
  • Add a dedicated test suite covering compound constructs, builtins, grouping, and quote escaping.
Show a summary per file
File Description
src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineBackgroundDetachRewriter.ts Adds detection + wrapping logic for compound commands/builtins before nohup.
src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/commandLineBackgroundDetachRewriter.test.ts Adds tests verifying bash -c wrapping and correct escaping/behavior.

Copilot's findings

Comments suppressed due to low confidence (2)

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineBackgroundDetachRewriter.ts:90

  • Comment says dot-source is detected by requiring “a space or end-of-string after the dot”, but the regex only matches ^\. (dot + space). Either update the comment, or adjust the regex to also match the . builtin with no arguments if that’s intended.
			// `. file` (dot-source builtin). Exclude `./script` (relative path) by requiring
			// a space or end-of-string after the dot.
			/^\. /.test(trimmed) ||
			// Compound groupings: subshell `( ... )` or brace group `{ ...; }`.

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/commandLineRewriter/commandLineBackgroundDetachRewriter.ts:66

  • endsWithBackgroundAmp is now tested against commandToWrap after wrapping. When the original command ends with a trailing & and _needsBashCWrapper is true, the & ends up inside the quoted bash -c '...&' string so the regex won’t match and an extra outer & gets appended (producing nohup bash -c '... &' &). Consider detecting/stripping a trailing background & from the original trimmed command before escaping/wrapping, and then only adding a single outer & to the final nohup invocation.
		let commandToWrap = trimmed;
		if (this._needsBashCWrapper(trimmed)) {
			// Escape single quotes for use inside a single-quoted bash -c '...' string.
			const escaped = trimmed.replace(/'/g, `'\\''`);
			commandToWrap = `bash -c '${escaped}'`;
		}

		// If the command already ends with a single trailing `&` (background operator,
		// as opposed to `&&` for command chaining), don't append another one.
		const endsWithBackgroundAmp = /(?:^|[^&])&$/.test(commandToWrap);
		const rewritten = endsWithBackgroundAmp
			? `nohup ${commandToWrap}`
			: `nohup ${commandToWrap} &`;
  • Files reviewed: 2/2 changed files
  • Comments generated: 1

@meganrogge meganrogge merged commit 6c8f319 into main Apr 23, 2026
26 checks passed
@meganrogge meganrogge deleted the merogge/nohup branch April 23, 2026 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants