From def7374658f5a2bc1a4b94e095bf65e78bee5275 Mon Sep 17 00:00:00 2001 From: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Date: Tue, 4 Aug 2026 01:18:24 -0400 Subject: [PATCH] fix(worktrees): replay only real `set` flags and keep the gate in the pane's shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two round-2 review findings: - `#!/bin/bash -l` replayed `set -l`, which exits 2 and aborted the runner under its own `set -e` before a single setup line ran (all platforms). Only the flags `set` documents are replayed now; a bare `-o` with no option name is dropped instead of dumping the shell-option table. - The wait-for-setup gate picked its language from the runner file, so a batch runner launched from a Git Bash pane got the PowerShell gate while the agent startup command was already POSIX-quoted — `Invoke-Expression` cannot parse `'\''`. The gate now follows the pane; the runner still launches through the ProcessStartInfo launcher, never through bash. --- docs/reference/windows-setup-shell.md | 9 ++++++- src/main/hooks-runner.test.ts | 30 +++++++++++++++++++++++ src/shared/setup-agent-sequencing.test.ts | 22 ++++++++++++++--- src/shared/setup-agent-sequencing.ts | 13 ++++++++-- src/shared/setup-script-shebang.test.ts | 16 +++++++++++- src/shared/setup-script-shebang.ts | 24 +++++++++++------- 6 files changed, 97 insertions(+), 17 deletions(-) diff --git a/docs/reference/windows-setup-shell.md b/docs/reference/windows-setup-shell.md index c8231d078d5..6c93185b602 100644 --- a/docs/reference/windows-setup-shell.md +++ b/docs/reference/windows-setup-shell.md @@ -47,7 +47,9 @@ every platform. The `#!` line therefore does two things: - It declares the script is written for a POSIX shell, which is what selects the bash runner. - Its option flags are replayed with `set`, so `#!/usr/bin/env -S bash -euo pipefail` really does get `pipefail`. Without that replay the flags would be silently dropped, because `bash ` - never parses the interpreter line. + never parses the interpreter line. Only the flags `set` itself accepts + (`[--abefhkmnptuvxBCHP] [-o option]`) are replayed; invocation-only ones such as `-l` are + dropped, because `set -l` exits 2 and would abort the runner before its first line. The interpreter name itself is not honored beyond "is this a POSIX shell": `#!/bin/sh` and `#!/bin/zsh` scripts run under bash, exactly as they already did on macOS and Linux. @@ -75,5 +77,10 @@ interactively and the runner never executes (issue #6896). Those launches reuse and the runner path outside the command line. `WorktreeSetupLaunch.shell` therefore describes the launching pane; the runner file's `.cmd`/`.sh` extension describes the format. +The `wait-for-setup` gate follows the same split. The pane types the gate and already quoted the +agent startup command for itself, so a `.cmd` runner launched from a Git Bash pane still gets the +bash gate — PowerShell's `Invoke-Expression` cannot parse POSIX `'\''` escaping. The gate wraps the +same `ProcessStartInfo` launcher, so the batch runner is never handed to bash. + WSL worktrees and non-Windows platforms are unaffected: they always use the bash runner. SSH hosts choose their runner from the remote path format, never from local Windows preferences. diff --git a/src/main/hooks-runner.test.ts b/src/main/hooks-runner.test.ts index 8815ef652d3..5ca5903d66f 100644 --- a/src/main/hooks-runner.test.ts +++ b/src/main/hooks-runner.test.ts @@ -1,3 +1,8 @@ +import type * as NodeChildProcess from 'node:child_process' +import type * as NodeFs from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + import type { Repo } from '../shared/types' import { describe, expect, it, vi } from 'vitest' @@ -210,6 +215,31 @@ describe('createSetupRunnerScript', () => { expect(runner).toContain('call rem still batch') }) + it.skipIf(process.platform === 'win32')( + 'runs a script whose shebang carries invocation-only bash flags', + async () => { + // Regression: `set` rejects `-l`/`-s`/`-i` with exit 2, and the runner's own `set -e` turns + // that into an aborted setup before the first user line. + const { buildPosixRunnerScript } = await import('./hooks') + const { mkdtempSync, rmSync, writeFileSync } = await vi.importActual('node:fs') + const { execFileSync } = await vi.importActual('node:child_process') + + const dir = mkdtempSync(join(tmpdir(), 'orca-posix-runner-')) + try { + const runnerPath = join(dir, 'setup-runner.sh') + writeFileSync( + runnerPath, + buildPosixRunnerScript('#!/bin/bash -l\necho SETUP_RAN\n'), + 'utf-8' + ) + + expect(execFileSync('bash', [runnerPath], { encoding: 'utf-8' })).toContain('SETUP_RAN') + } finally { + rmSync(dir, { recursive: true, force: true }) + } + } + ) + it('derives ORCA_WORKSPACE_NAME from a POSIX worktree path', async () => { const originalPlatform = process.platform diff --git a/src/shared/setup-agent-sequencing.test.ts b/src/shared/setup-agent-sequencing.test.ts index 828cd1eec65..61dc001e627 100644 --- a/src/shared/setup-agent-sequencing.test.ts +++ b/src/shared/setup-agent-sequencing.test.ts @@ -218,12 +218,13 @@ describe('createSequencedSetupAgentCommands', () => { }) }) - it('keeps the cmd gate for a batch runner launched from a Git Bash pane', () => { + it('launches a batch runner through the cmd launcher inside a Git Bash gate', () => { // Regression (#6896): a Git Bash terminal with a batch setup script still gets a .cmd - // runner, and the wait-for-setup gate must not try to run it under bash. + // runner, and the gate must not hand that runner to bash. The gate itself stays POSIX + // because the Git Bash pane types it and quoted the startup command for bash. const result = createSequencedSetupAgentCommands({ runnerScriptPath: 'C:\\repo\\.git\\orca\\setup-runner.cmd', - startupCommand: 'claude', + startupCommand: "claude 'fix the user'\\''s login'", platform: 'windows', shell: { family: 'posix' }, nonce: 'nonce-gitbash-cmd' @@ -232,10 +233,23 @@ describe('createSequencedSetupAgentCommands', () => { expect(result.setupCommand).toContain( 'powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -EncodedCommand' ) - expect(result.setupCommand).not.toContain('bash -lc') + expect(result.setupCommand).not.toMatch(/bash\s+\S*setup-runner/) expect(decodePowerShellScript(result.setupCommand)).toContain( "$runner = 'C:\\repo\\.git\\orca\\setup-runner.cmd'" ) + // Why: PowerShell's `Invoke-Expression` cannot parse the POSIX `'\''` escaping a Git Bash + // pane produces, so the gate that evaluates the startup command must be bash. + expect(result.setupCommand).toMatch(/^bash -lc /) + expect(result.startupCommand).toMatch(/^bash -lc /) + expect(result.startupCommand).not.toContain('Invoke-Expression') + expect(result.startupCommand).toContain('eval "$ORCA_SEQUENCED_STARTUP_COMMAND"') + // Why: bash writes and reads the marker here, so it needs the /c/... form of the path. + expect(result.setupCommand).toContain( + '/c/repo/.git/orca/setup-runner.cmd.nonce-gitbash-cmd.done' + ) + expect(result.startupCommand).toContain( + '/c/repo/.git/orca/setup-runner.cmd.nonce-gitbash-cmd.done' + ) }) it.skipIf(process.platform !== 'win32')( diff --git a/src/shared/setup-agent-sequencing.ts b/src/shared/setup-agent-sequencing.ts index 92376f10886..1ec888e3aa6 100644 --- a/src/shared/setup-agent-sequencing.ts +++ b/src/shared/setup-agent-sequencing.ts @@ -1,5 +1,6 @@ import { encodePowerShellCommand } from './powershell-command-encoding' import { + nativeWindowsPathToPosixShellPath, resolveSetupRunnerCommand, type SetupRunnerCommandPlatform, type SetupRunnerCommandShell, @@ -41,12 +42,20 @@ export function createSequencedSetupAgentCommands(args: { }): SequencedSetupAgentCommands { const nonce = args.nonce ?? createSetupAgentSequenceNonce() const resolution = resolveSetupRunnerCommand(args.runnerScriptPath, args.platform, args.shell) + // Why: the gate is typed into the terminal pane and `startupCommand` is already quoted for that + // pane, so a batch runner launched from a Git Bash pane still needs the bash gate — PowerShell's + // `Invoke-Expression` cannot parse the POSIX `'\''` escaping the pane's quoting produces. The + // runner itself still launches through `resolution.command`, never through bash. + const posixGateForWindowsRunner = resolution.shell === 'windows' && args.shell?.family === 'posix' + const markerBasePath = posixGateForWindowsRunner + ? nativeWindowsPathToPosixShellPath(resolution.runnerScriptPathForShell) + : resolution.runnerScriptPathForShell // Why: overlapping gated launches of the same setup runner must not race on // a shared completion marker. - const markerPath = `${resolution.runnerScriptPathForShell}.${nonce}.done` + const markerPath = `${markerBasePath}.${nonce}.done` const waitTimeoutSeconds = args.waitTimeoutSeconds ?? DEFAULT_WAIT_TIMEOUT_SECONDS - if (resolution.shell === 'windows') { + if (resolution.shell === 'windows' && !posixGateForWindowsRunner) { return { setupCommand: buildWindowsSetupCommand( resolution.runnerScriptPathForShell, diff --git a/src/shared/setup-script-shebang.test.ts b/src/shared/setup-script-shebang.test.ts index 5d85f5296a2..05dca1204da 100644 --- a/src/shared/setup-script-shebang.test.ts +++ b/src/shared/setup-script-shebang.test.ts @@ -54,8 +54,22 @@ describe('parseSetupScriptShebang', () => { }) it('ignores interpreter arguments that `set` cannot apply', () => { + // Regression: `set` rejects invocation-only flags with exit 2, and the runner's `set -e` + // turns that into an aborted setup before its first line runs. expect(parseSetupScriptShebang('#!/bin/bash --norc\nmake')?.shellOptions).toEqual([]) - expect(parseSetupScriptShebang('#!/bin/bash -o\nmake')?.shellOptions).toEqual(['-o']) + expect(parseSetupScriptShebang('#!/bin/bash -l\nmake')?.shellOptions).toEqual([]) + expect(parseSetupScriptShebang('#!/bin/bash -s\nmake')?.shellOptions).toEqual([]) + expect(parseSetupScriptShebang('#!/bin/bash -i\nmake')?.shellOptions).toEqual([]) + // Why: `-r` is accepted by `set` on some shells but silently restricts the rest of setup. + expect(parseSetupScriptShebang('#!/bin/bash -r\nmake')?.shellOptions).toEqual([]) + expect(parseSetupScriptShebang('#!/bin/bash -ex -l\nmake')?.shellOptions).toEqual(['-ex']) + // Why: a bare `-o` would print the whole shell-option table into the setup terminal. + expect(parseSetupScriptShebang('#!/bin/bash -o\nmake')?.shellOptions).toEqual([]) + expect(parseSetupScriptShebang('#!/bin/bash -euo\nmake')?.shellOptions).toEqual([]) + expect(parseSetupScriptShebang('#!/bin/bash +o posix\nmake')?.shellOptions).toEqual([ + '+o', + 'posix' + ]) }) it('returns null without an interpreter line', () => { diff --git a/src/shared/setup-script-shebang.ts b/src/shared/setup-script-shebang.ts index ceb5741d44b..f30bbab0959 100644 --- a/src/shared/setup-script-shebang.ts +++ b/src/shared/setup-script-shebang.ts @@ -2,8 +2,13 @@ // not of the user's terminal preference, so a `#!` line is how a project declares it. const POSIX_SHELL_BASENAMES = new Set(['sh', 'bash', 'zsh', 'dash', 'ksh', 'ash']) -// Why: `-e`, `+e`, `-euo`; `--norc` and interpreter arguments are not `set` options. -const SHELL_OPTION_FLAG_PATTERN = /^[-+][a-zA-Z]+$/ +// Why: only the letters `set` itself accepts (`set [--abefhkmnptuvxBCHP] [-o option]`). An +// invocation-only flag such as `-l` or `-r` makes `set` exit 2, which under the runner's `set -e` +// aborts setup before its first line runs. +const SET_OPTION_FLAG_PATTERN = /^[-+][abefhkmnptuvxBCHP]+$/ +// Why: `-o`/`-euo` name their option in the next token; a trailing `-o` with no name would make +// `set` dump the whole shell-option table into the setup terminal instead. +const SET_LONG_OPTION_FLAG_PATTERN = /^[-+][abefhkmnptuvxBCHP]*o$/ const SHELL_OPTION_NAME_PATTERN = /^[a-z_]+$/ export type SetupScriptShebang = { @@ -68,15 +73,16 @@ function parseShellOptions(tokens: string[]): string[] { const options: string[] = [] for (let index = 0; index < tokens.length; index++) { const token = tokens[index] - if (!SHELL_OPTION_FLAG_PATTERN.test(token)) { + if (SET_LONG_OPTION_FLAG_PATTERN.test(token)) { + const optionName = tokens[index + 1] + if (optionName && SHELL_OPTION_NAME_PATTERN.test(optionName)) { + options.push(token, optionName) + index++ + } continue } - options.push(token) - // Why: `-o`/`-euo` take the option name as a separate argument (`pipefail`). - const optionName = tokens[index + 1] - if (token.endsWith('o') && optionName && SHELL_OPTION_NAME_PATTERN.test(optionName)) { - options.push(optionName) - index++ + if (SET_OPTION_FLAG_PATTERN.test(token)) { + options.push(token) } } return options