mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
fix(worktrees): replay only real set flags and keep the gate in the pane's shell
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.
This commit is contained in:
@@ -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 <runner>`
|
||||
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.
|
||||
|
||||
@@ -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<typeof NodeFs>('node:fs')
|
||||
const { execFileSync } = await vi.importActual<typeof NodeChildProcess>('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
|
||||
|
||||
|
||||
@@ -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')(
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user