From 6b51ef4e2c4df22d9f2013d628105dbc031aacd0 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 22 Aug 2026 02:06:06 -0700 Subject: [PATCH] feat(wsl): one runner for every wsl.exe invocation (#15903) Five decisions have to be made on each `wsl.exe` call. Each has a right answer, each is invisible in a diff, and each has shipped wrong: - **Separator.** `--` makes wsl.exe expand `$name` in every forwarded argument before the guest runs -- even with no shell in the command -- so `awk '{print $2}'` loses its field reference (#12964). - **Shell.** A login shell on a probe path sources `~/.profile`, so one blocking line eats the whole timeout (#14288) and every call pays startup (#9768). No login shell on a user-facing path means PATH does not match the user's own terminal, so nvm-installed agents read as absent (#9725, #7563, #8366). - **Fencing.** An interactive login shell runs the distro rc, and stock Ubuntu writes its "run as administrator" hint to *stdout* -- so anything parsing that stream reads the banner as data (#11327, #11823). - **WSLENV.** Unset, a Windows-side variable silently never crosses (#12557). - **Payload.** Scripts go in on stdin. A script on stdin has no quoting boundary to escape from, which is what the base64 and `eval` wrappers work around (#14292). `filesystem-watcher-wsl.ts` already does this and is the only WSL caller with no quoting bug in its history. `runWslProcess` makes them once, on top of W1's `runProcess` so it inherits windowsHide, shell:false, timeouts and abort. `lane` is required with no default: picking the wrong lane by omission is the most common WSL defect here. The probe lane resolves the login PATH/HOME once per distro and then runs with no shell at all, so #14288 and #9768 are closed by construction rather than by a longer timeout. An unprobed distro degrades to the interactive lane -- "we could not ask" must not become "run with no PATH". Additive only: no call site is migrated yet. The new guard allowlists the 23 files that still spawn directly, and its length is the workstream's goalpost. Two guard bugs found by testing the guards against planted call sites: a bare `main/wsl` prefix also exempted `main/wsl.ts`, `wsl-availability.ts` and `wsl-unc-delete.ts` -- three real offenders. --- .../__fixtures__/wsl-invocation-allowlist.txt | 27 +++ src/main/wsl/wsl-executable-path.ts | 20 ++ src/main/wsl/wsl-guest-environment.test.ts | 138 +++++++++++ src/main/wsl/wsl-guest-environment.ts | 172 ++++++++++++++ src/main/wsl/wsl-invocation-boundary.test.ts | 101 ++++++++ src/main/wsl/wsl-runner.test.ts | 216 ++++++++++++++++++ src/main/wsl/wsl-runner.ts | 205 +++++++++++++++++ src/main/wsl/wsl-runner.wsl.test.ts | 125 ++++++++++ 8 files changed, 1004 insertions(+) create mode 100644 src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt create mode 100644 src/main/wsl/wsl-executable-path.ts create mode 100644 src/main/wsl/wsl-guest-environment.test.ts create mode 100644 src/main/wsl/wsl-guest-environment.ts create mode 100644 src/main/wsl/wsl-invocation-boundary.test.ts create mode 100644 src/main/wsl/wsl-runner.test.ts create mode 100644 src/main/wsl/wsl-runner.ts create mode 100644 src/main/wsl/wsl-runner.wsl.test.ts diff --git a/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt b/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt new file mode 100644 index 00000000000..6fa44e9de5f --- /dev/null +++ b/src/main/wsl/__fixtures__/wsl-invocation-allowlist.txt @@ -0,0 +1,27 @@ +# Files that spawn wsl.exe directly instead of through src/main/wsl/wsl-runner.ts. +# Enforced by ../wsl-invocation-boundary.test.ts. This list only shrinks -- its +# length is the W3 goalpost. Removing an entry means the file now goes through +# the runner, not that its call was tidied in place. +main/agent-hooks/wsl-hook-relay-launch.ts +main/claude-accounts/runtime-auth-service.ts +main/claude-accounts/service.ts +main/cli/wsl-cli-installer.ts +main/codex-accounts/runtime-home-service.ts +main/codex-accounts/service.ts +main/codex/codex-state-db-backfill-recovery.ts +main/codex/codex-trust-grant-host.ts +main/codex/codex-wsl-hook-install-plan.ts +main/git/wsl-git-read-environment.ts +main/hooks.ts +main/ipc/filesystem-watcher-wsl.ts +main/ipc/preflight-wsl-agent-detection.ts +main/ipc/preflight-wsl-command.ts +main/local-worktree-filesystem.ts +main/skills/claude-plugin-skill-sources-wsl.ts +main/skills/skill-discovery-wsl.ts +main/skills/skill-provider-runtime-roots.ts +main/skills/skill-wsl-install-filesystem.ts +main/skills/skill-wsl-provider-detection.ts +main/wsl-availability.ts +main/wsl-unc-delete.ts +main/wsl.ts diff --git a/src/main/wsl/wsl-executable-path.ts b/src/main/wsl/wsl-executable-path.ts new file mode 100644 index 00000000000..5767f7b0a79 --- /dev/null +++ b/src/main/wsl/wsl-executable-path.ts @@ -0,0 +1,20 @@ +import { existsSync } from 'node:fs' +import { win32 as pathWin32 } from 'node:path' + +/** + * Absolute path to `wsl.exe`. + * + * Why not the bare name: spawning by name resolves through the child's PATH, + * which a Group Policy, a stripped Electron environment or a shadowing entry + * can point somewhere else — the same class of failure W1 fixed for PowerShell + * (#15749). System32 is the launcher Microsoft ships; the Store package + * forwards through it. + */ +export function resolveWslExecutablePath(): string { + const systemRoot = process.env.SystemRoot || 'C:\\Windows' + const absolute = pathWin32.join(systemRoot, 'System32', 'wsl.exe') + // Why the fallback: a host that has WSL somewhere else still deserves a + // working call, and PATH resolution is strictly better than a path we know + // is wrong. + return existsSync(absolute) ? absolute : 'wsl.exe' +} diff --git a/src/main/wsl/wsl-guest-environment.test.ts b/src/main/wsl/wsl-guest-environment.test.ts new file mode 100644 index 00000000000..1e797d9d88e --- /dev/null +++ b/src/main/wsl/wsl-guest-environment.test.ts @@ -0,0 +1,138 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const runProcessMock = vi.hoisted(() => vi.fn()) +vi.mock('../../shared/child-process/run-process', () => ({ runProcess: runProcessMock })) +vi.mock('./wsl-executable-path', () => ({ resolveWslExecutablePath: () => 'wsl.exe' })) + +import { + getWslGuestEnvironment, + invalidateWslGuestEnvironment, + peekWslGuestEnvironment +} from './wsl-guest-environment' + +/** Echo a well-formed payload back inside whatever fence the probe generated. */ +function respondWithPayload(payload: string, code = 0): void { + runProcessMock.mockImplementation(async (spec: { args: string[] }) => { + const script = spec.args.at(-1) ?? '' + const begin = /__ORCA_WSL_CAPTURE_BEGIN_[a-z0-9]+__/.exec(script)?.[0] ?? '' + const end = /__ORCA_WSL_CAPTURE_END_[a-z0-9]+__/.exec(script)?.[0] ?? '' + return { + code, + signal: null, + stdout: `distro banner\n${begin}${payload}${end}`, + stderr: '', + timedOut: false + } + }) +} + +const GOOD = ['/home/u/.nvm/bin:/usr/bin', '/home/u', '/usr/bin/env'].join('\0') + +beforeEach(() => { + runProcessMock.mockReset() + invalidateWslGuestEnvironment() +}) +afterEach(() => invalidateWslGuestEnvironment()) + +describe('probing', () => { + it('reads PATH, HOME and env out of a banner-polluted stdout', async () => { + respondWithPayload(GOOD) + expect(await getWslGuestEnvironment('Ubuntu')).toEqual({ + path: '/home/u/.nvm/bin:/usr/bin', + home: '/home/u', + envBinary: '/usr/bin/env' + }) + }) + + it('collapses a concurrent burst into one probe', async () => { + // Teardown and detection both fan out; 32 login shells per burst is the + // cost this cache exists to avoid. + respondWithPayload(GOOD) + await Promise.all(Array.from({ length: 32 }, () => getWslGuestEnvironment('Ubuntu'))) + expect(runProcessMock).toHaveBeenCalledTimes(1) + }) + + it('keeps distros isolated', async () => { + respondWithPayload(GOOD) + await getWslGuestEnvironment('Ubuntu') + await getWslGuestEnvironment('Debian') + expect(runProcessMock).toHaveBeenCalledTimes(2) + }) +}) + +describe('bad answers are not cached as good ones', () => { + it.each([ + ['a relative HOME', ['/usr/bin', 'home/u', '/usr/bin/env'].join('\0')], + ['a PATH with a newline', ['/usr/bin\nx', '/home/u', '/usr/bin/env'].join('\0')], + ['a truncated payload', '/usr/bin'], + ['a relative env binary', ['/usr/bin', '/home/u', 'env'].join('\0')] + ])('rejects %s', async (_case, payload) => { + respondWithPayload(payload) + expect(await getWslGuestEnvironment('Ubuntu')).toBeNull() + expect(peekWslGuestEnvironment('Ubuntu')).toBeUndefined() + }) + + it('treats a missing fence as a failed probe, not an empty PATH', async () => { + runProcessMock.mockResolvedValue({ + code: 0, + signal: null, + stdout: 'only a banner, no fence', + stderr: '', + timedOut: false + }) + expect(await getWslGuestEnvironment('Ubuntu')).toBeNull() + }) +}) + +describe('transient versus permanent failure', () => { + it('does not retry a distro that cannot produce env', async () => { + // 127 is the probe's own "no usable env". Retrying that forever turns a + // probe into a poller. + runProcessMock.mockResolvedValue({ + code: 127, + signal: null, + stdout: '', + stderr: '', + timedOut: false + }) + await getWslGuestEnvironment('Ubuntu') + await getWslGuestEnvironment('Ubuntu') + expect(runProcessMock).toHaveBeenCalledTimes(1) + }) + + it('retries after the window when the probe timed out', async () => { + vi.useFakeTimers() + try { + runProcessMock.mockResolvedValue({ + code: null, + signal: null, + stdout: '', + stderr: '', + timedOut: true + }) + expect(await getWslGuestEnvironment('Ubuntu')).toBeNull() + expect(await getWslGuestEnvironment('Ubuntu')).toBeNull() + expect(runProcessMock).toHaveBeenCalledTimes(1) + + vi.setSystemTime(Date.now() + 31_000) + respondWithPayload(GOOD) + expect(await getWslGuestEnvironment('Ubuntu')).not.toBeNull() + expect(runProcessMock).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) +}) + +describe('invalidation', () => { + it('re-probes after the caller invalidates, so a newly installed tool appears', async () => { + // A user who installs nvm inside a running distro would otherwise keep the + // pre-install PATH until Orca restarts, and read that as the detection bug + // this cache exists to fix. + respondWithPayload(GOOD) + await getWslGuestEnvironment('Ubuntu') + invalidateWslGuestEnvironment('Ubuntu') + respondWithPayload(['/opt/new/bin', '/home/u', '/usr/bin/env'].join('\0')) + expect((await getWslGuestEnvironment('Ubuntu'))?.path).toBe('/opt/new/bin') + }) +}) diff --git a/src/main/wsl/wsl-guest-environment.ts b/src/main/wsl/wsl-guest-environment.ts new file mode 100644 index 00000000000..cf21b373c14 --- /dev/null +++ b/src/main/wsl/wsl-guest-environment.ts @@ -0,0 +1,172 @@ +import { runProcess } from '../../shared/child-process/run-process' +import { + buildWslCapturedLoginShellCommand, + buildWslExecArgs +} from '../../shared/wsl-login-shell-command' +import { resolveWslExecutablePath } from './wsl-executable-path' + +/** + * The login-shell environment of a distro, probed once and cached. + * + * Why this exists: a probe needs the user's real PATH (nvm, mise and asdf all + * install into rc files), but paying for a login shell on every probe is what + * made `cli:getWslInstallStatus` time out behind a blocking `~/.profile` + * (#14288) and what makes WSL git operations lag (#9768). Probe the login shell + * exactly once per distro, then run everything else with no shell at all. + * + * Generalised from `src/main/git/wsl-git-read-environment.ts`, which is the one + * WSL caller that already got this right. + */ + +export type WslGuestEnvironment = { + /** Login-shell PATH, as the user's own terminal would see it. */ + path: string + home: string + /** Absolute path to `env`, used to run programs without a shell. */ + envBinary: string +} + +const PROBE_TIMEOUT_MS = 10_000 +const PROBE_MAX_OUTPUT_BYTES = 64 * 1024 +/** + * Why retry a timeout but not a malformed answer: a stopped distro or a + * momentarily wedged `wsl.exe` recovers on its own, while a distro that cannot + * produce a POSIX PATH will not start doing so. Retrying the second forever is + * how a probe becomes a poller. + */ +const TRANSIENT_RETRY_MS = 30_000 + +type ProbeOutcome = + | { kind: 'resolved'; environment: WslGuestEnvironment } + | { kind: 'rejected' } + | { kind: 'transient' } + +const inFlight = new Map>() +const resolved = new Map() +const retryAfter = new Map() + +/** A payload that is not three absolute, single-line POSIX values is a failed probe. */ +function parseProbePayload(payload: string | null): WslGuestEnvironment | null { + if (payload === null) { + return null + } + const [path = '', home = '', envBinary = ''] = payload.split('\0') + const isCleanAbsolute = (value: string): boolean => + value.startsWith('/') && !value.includes('\n') && !value.includes('\r') + if (!path.includes('/') || path.length > 32_768 || path.includes('\n')) { + return null + } + if (!isCleanAbsolute(home) || !isCleanAbsolute(envBinary)) { + return null + } + return { path, home, envBinary } +} + +async function probeGuestEnvironment(distro: string | undefined): Promise { + // Why resolve `env` rather than assume /usr/bin/env: it is /usr/bin/env on + // Debian, Ubuntu, Fedora and Arch, but the probe costs nothing extra here and + // a distro that puts it elsewhere would otherwise fail every later call. + const script = [ + '_orca_env=$(command -v env 2>/dev/null || true)', + 'case "$_orca_env" in /*) [ -x "$_orca_env" ] || exit 127 ;; *) exit 127 ;; esac', + `printf '%s\\0%s\\0%s' "$PATH" "$HOME" "$_orca_env"` + ].join('\n') + const captured = buildWslCapturedLoginShellCommand(script) + const result = await runProcess({ + program: resolveWslExecutablePath(), + args: buildWslExecArgs(distro, ['sh', '-c', captured.command]), + timeoutMs: PROBE_TIMEOUT_MS, + maxOutputBytes: PROBE_MAX_OUTPUT_BYTES + }) + if (result.timedOut) { + return { kind: 'transient' } + } + if (result.code !== 0) { + // 127 is our own "no usable env"; anything else is the distro being + // unavailable, which is worth retrying. + return result.code === 127 ? { kind: 'rejected' } : { kind: 'transient' } + } + const environment = parseProbePayload(captured.readStdout(result.stdout)) + return environment ? { kind: 'resolved', environment } : { kind: 'rejected' } +} + +function cacheKey(distro: string | undefined): string { + return distro ?? '' +} + +/** + * The distro's login-shell environment, or null when it cannot be established. + * + * Null is "we could not ask", never "the distro has no PATH" — callers fall + * back to the interactive lane rather than running with an empty environment. + */ +export function getWslGuestEnvironment( + distro: string | undefined +): Promise { + const key = cacheKey(distro) + const retry = retryAfter.get(key) + if (retry !== undefined && Date.now() >= retry) { + inFlight.delete(key) + retryAfter.delete(key) + } + const existing = inFlight.get(key) + if (existing) { + return existing + } + // Why store the promise before awaiting: a 32-wide burst during teardown must + // collapse into one probe, not 32 login shells. + const probe = probeGuestEnvironment(distro).then((outcome) => { + if (inFlight.get(key) !== probe) { + return outcome.kind === 'resolved' ? outcome.environment : null + } + if (outcome.kind === 'resolved') { + resolved.set(key, outcome.environment) + retryAfter.delete(key) + return outcome.environment + } + if (outcome.kind === 'transient') { + retryAfter.set(key, Date.now() + TRANSIENT_RETRY_MS) + } + return null + }) + inFlight.set(key, probe) + return probe +} + +/** + * Drop a distro's cached environment. + * + * Why callers need this: a user who installs nvm inside a running distro would + * otherwise keep the pre-install PATH until Orca restarts, and read that as the + * same detection bug this cache exists to fix. + */ +export function invalidateWslGuestEnvironment(distro?: string): void { + if (distro === undefined) { + inFlight.clear() + resolved.clear() + retryAfter.clear() + return + } + const key = cacheKey(distro) + inFlight.delete(key) + resolved.delete(key) + retryAfter.delete(key) +} + +/** Test-only: the cached value without probing. */ +export function peekWslGuestEnvironment( + distro: string | undefined +): WslGuestEnvironment | undefined { + return resolved.get(cacheKey(distro)) +} + +/** Test-only: pretend a distro has already been probed. */ +export function seedWslGuestEnvironmentForTests( + distro: string | undefined, + environment: WslGuestEnvironment +): void { + const key = cacheKey(distro) + inFlight.set(key, Promise.resolve(environment)) + resolved.set(key, environment) + retryAfter.delete(key) +} diff --git a/src/main/wsl/wsl-invocation-boundary.test.ts b/src/main/wsl/wsl-invocation-boundary.test.ts new file mode 100644 index 00000000000..e2fc6440b3e --- /dev/null +++ b/src/main/wsl/wsl-invocation-boundary.test.ts @@ -0,0 +1,101 @@ +import { readdirSync, readFileSync, statSync } from 'node:fs' +import { join, relative, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +/** + * Every `wsl.exe` spawn must go through `runWslProcess`. + * + * Why a guard and not review: five decisions have to be made on each call + * (separator, shell, stdout fencing, WSLENV, payload transport), each is + * invisible in a diff, and each has shipped wrong. `wsl-exec-mode-separator` + * already guards one of the five — this guards the call itself, so the other + * four cannot be re-decided per site. + * + * The allowlist is the W3 migration worklist and only shrinks. Its length is + * the workstream's measured goalpost. + */ +const ALLOWLIST: readonly string[] = readFileSync( + join(__dirname, '__fixtures__', 'wsl-invocation-allowlist.txt'), + 'utf8' +) + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith('#')) + +const SOURCE_ROOT = resolve(__dirname, '../..') +// Why the trailing slash: a bare 'main/wsl' prefix also exempts main/wsl.ts, +// main/wsl-availability.ts and main/wsl-unc-delete.ts -- three files that spawn +// wsl.exe directly. Caught by testing the guard against a planted call site. +const OWNER_DIRECTORY = 'main/wsl/' +const IGNORED = new Set(['node_modules', 'dist', 'out', 'build', '.git', '__fixtures__']) +/** + * A spawn site: the `wsl.exe` literal sits directly in a spawn-family call, or + * in a `program:` field. A bare mention (a shell-name constant, a type, a + * comment) is not a call and is not the guard's business. + */ +const SPAWN_OPENER = + /(?:spawn|spawnSync|execFile|execFileSync|exec|fork|runProcess|spawnProcess|execFileAsync|execAsync)\s*\(\s*$|program:\s*$/ + +function isTestFile(path: string): boolean { + return ( + /\.(?:test|spec)\.tsx?$/.test(path) || + /(?:test-harness|test-utils|test-setup|test-fixture|repro)/.test(path) + ) +} + +function collectSourceFiles(root: string): string[] { + const found: string[] = [] + for (const entry of readdirSync(root)) { + if (IGNORED.has(entry) || entry.startsWith('.')) { + continue + } + const path = join(root, entry) + if (statSync(path).isDirectory()) { + found.push(...collectSourceFiles(path)) + continue + } + if (/\.tsx?$/.test(entry)) { + found.push(path) + } + } + return found +} + +function findSpawnSites(): string[] { + const offenders = new Set() + for (const path of collectSourceFiles(SOURCE_ROOT)) { + const relativePath = relative(SOURCE_ROOT, path).replace(/\\/g, '/') + if (isTestFile(relativePath) || relativePath.startsWith(OWNER_DIRECTORY)) { + continue + } + const source = readFileSync(path, 'utf8') + for (const match of source.matchAll(/['"]wsl\.exe['"]/g)) { + // Collapse the preceding whitespace so a call broken across lines by the + // formatter still reads as one opener. + const preceding = source.slice(Math.max(0, match.index - 60), match.index) + if (SPAWN_OPENER.test(preceding.replace(/\s+/g, ' ').replace(/ $/, ''))) { + offenders.add(relativePath) + } + } + } + return [...offenders].sort() +} + +describe('wsl.exe is spawned through one runner', () => { + const offenders = findSpawnSites() + + it('finds the call sites it is meant to guard', () => { + // Guards against a detection change quietly emptying the scan, which would + // make the assertions below pass without checking anything. + expect(offenders.length + ALLOWLIST.length).toBeGreaterThanOrEqual(20) + }) + + it('adds no new direct wsl.exe spawn', () => { + expect(offenders.filter((path) => !ALLOWLIST.includes(path))).toEqual([]) + }) + + it('carries no stale allowlist entry', () => { + // A migrated file must leave the list, or the goalpost stops moving. + expect(ALLOWLIST.filter((path) => !offenders.includes(path))).toEqual([]) + }) +}) diff --git a/src/main/wsl/wsl-runner.test.ts b/src/main/wsl/wsl-runner.test.ts new file mode 100644 index 00000000000..2c014fb9e86 --- /dev/null +++ b/src/main/wsl/wsl-runner.test.ts @@ -0,0 +1,216 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const runProcessMock = vi.hoisted(() => vi.fn()) +vi.mock('../../shared/child-process/run-process', () => ({ runProcess: runProcessMock })) +vi.mock('./wsl-executable-path', () => ({ + resolveWslExecutablePath: () => 'C:\\Windows\\System32\\wsl.exe' +})) + +import { runWslProcess } from './wsl-runner' +import { + invalidateWslGuestEnvironment, + seedWslGuestEnvironmentForTests +} from './wsl-guest-environment' + +const ENVIRONMENT = { + path: '/home/u/.nvm/bin:/usr/bin', + home: '/home/u', + envBinary: '/usr/bin/env' +} + +function lastArgv(): string[] { + return runProcessMock.mock.calls.at(-1)?.[0].args as string[] +} + +beforeEach(() => { + runProcessMock.mockReset() + runProcessMock.mockResolvedValue({ + code: 0, + signal: null, + stdout: '', + stderr: '', + timedOut: false + }) + invalidateWslGuestEnvironment() +}) + +afterEach(() => { + invalidateWslGuestEnvironment() +}) + +describe('separator', () => { + it.each([ + ['probe', 'probe'], + ['interactive', 'interactive'] + ] as const)('uses --exec and never -- on the %s lane', async (_name, lane) => { + // Why this is pinned on both lanes: under `--`, wsl.exe expands $name in + // every forwarded argument before the guest runs -- even with no shell in + // the command -- so a script means something other than what it says + // (#12964). No escaping on our side is a reliable substitute. + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ lane, program: '/usr/bin/git', args: ['status'] }) + expect(lastArgv()).toContain('--exec') + expect(lastArgv()).not.toContain('--') + }) + + it('passes the distro before --exec', async () => { + seedWslGuestEnvironmentForTests('Ubuntu', ENVIRONMENT) + await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/true' }) + expect(lastArgv().slice(0, 3)).toEqual(['-d', 'Ubuntu', '--exec']) + }) +}) + +describe('probe lane', () => { + it('runs with the cached login PATH and no shell', async () => { + // The whole point: the user's real PATH without paying for -- or being + // blocked by -- a login shell on every call (#14288, #9768). + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ lane: 'probe', program: 'codex', args: ['--version'] }) + expect(lastArgv()).toEqual([ + '--exec', + '/usr/bin/env', + 'PATH=/home/u/.nvm/bin:/usr/bin', + 'HOME=/home/u', + 'codex', + '--version' + ]) + }) + + it('falls back to the interactive lane when the distro cannot be probed', async () => { + // "We could not ask" must not become "run with no PATH" -- that would turn + // an unknown into a wrong answer. + runProcessMock.mockResolvedValue({ + code: 1, + signal: null, + stdout: '', + stderr: 'distro is stopped', + timedOut: false + }) + await runWslProcess({ lane: 'probe', program: 'codex' }) + const argv = lastArgv() + expect(argv).toContain('--exec') + expect(argv.slice(-2, -1)).toEqual(['-c']) + expect(argv.at(-1)).toContain('_orca_wsl_shell') + }) +}) + +describe('interactive lane', () => { + it('always fences stdout, even when the caller ignores it', async () => { + // Stock Ubuntu writes its rc hint to stdout, so an unfenced parse reads the + // banner as data (#11327, #11823). A caller that starts parsing later must + // not have to remember to opt in. + runProcessMock.mockImplementation(async (spec: { args: string[] }) => { + const script = spec.args.at(-1) ?? '' + const begin = /__ORCA_WSL_CAPTURE_BEGIN_[a-z0-9]+__/.exec(script)?.[0] ?? '' + const end = /__ORCA_WSL_CAPTURE_END_[a-z0-9]+__/.exec(script)?.[0] ?? '' + return { + code: 0, + signal: null, + stdout: `Ubuntu banner: run a command as administrator\n${begin}payload${end}`, + stderr: '', + timedOut: false + } + }) + const result = await runWslProcess({ lane: 'interactive', program: 'claude' }) + expect(result.stdout).toBe('payload') + }) +}) + +describe('scripts', () => { + it('delivers a script on stdin through sh -s, not in argv', async () => { + // A script on stdin has no quoting boundary for its own quotes to escape + // from. That is what the base64 and eval wrappers were working around + // (#14292), and why this is the only supported way to run one. + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + const script = `case "$x" in a) echo 'it'\\''s fine';; esac` + await runWslProcess({ lane: 'probe', script, args: ['/tmp/root'] }) + expect(runProcessMock.mock.calls.at(-1)?.[0].input).toBe(script) + expect(lastArgv()).toEqual([ + '--exec', + '/usr/bin/env', + 'PATH=/home/u/.nvm/bin:/usr/bin', + 'HOME=/home/u', + 'sh', + '-s', + '--', + '/tmp/root' + ]) + expect(lastArgv().join(' ')).not.toContain('case') + }) +}) + +describe('WSLENV', () => { + it('adds every propagated key so the value actually crosses the boundary', async () => { + // Unset, a Windows-side variable silently never reaches the guest (#12557). + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ + lane: 'probe', + program: '/bin/true', + env: { GITLAB_HOST: 'git.example.com', GH_TOKEN: 't' } + }) + const env = runProcessMock.mock.calls.at(-1)?.[0].env as NodeJS.ProcessEnv + expect(env.GITLAB_HOST).toBe('git.example.com') + expect(env.WSLENV?.split(':')).toEqual(expect.arrayContaining(['GITLAB_HOST', 'GH_TOKEN'])) + }) + + it('leaves the host environment alone when nothing is propagated', async () => { + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ lane: 'probe', program: '/bin/true' }) + expect(runProcessMock.mock.calls.at(-1)?.[0].env).toBeUndefined() + }) +}) + +describe('guest cwd', () => { + it('cds inside the guest rather than passing a Windows cwd to wsl.exe', async () => { + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ lane: 'probe', program: '/usr/bin/git', cwd: '/home/u/repo' }) + expect(runProcessMock.mock.calls.at(-1)?.[0].cwd).toBeUndefined() + expect(lastArgv()).toContain('/home/u/repo') + expect(lastArgv()).toContain('sh') + }) + + it.each([['C:\\repo'], ['relative/path']])('rejects %s as a guest cwd', async (cwd) => { + // A Windows path here means a mistake further up; converting it silently + // hides that. + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await expect(runWslProcess({ lane: 'probe', program: '/bin/true', cwd })).rejects.toThrow( + /guest path/ + ) + }) +}) + +describe('program is a binary, not a shell string', () => { + it.each([['sh -c echo hi'], ['a; b'], ['a | b'], ['a && b'], ['echo $HOME'], ['a > b']])( + 'rejects %s', + async (program) => { + await expect(runWslProcess({ lane: 'probe', program })).rejects.toThrow(/single binary/) + } + ) + + it('allows a guest path containing a space', async () => { + // --exec passes the program as one argv element, so a space is harmless; + // rejecting it would fail legitimate installs under a spaced directory. + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await expect( + runWslProcess({ lane: 'probe', program: '/home/u/my tools/codex' }) + ).resolves.toBeDefined() + }) +}) + +describe('a script never rides the interactive lane', () => { + it('runs sh -s even when the distro cannot be probed', async () => { + // The login shell owns stdin, and the script arrives on stdin. If the shell + // consumed it, `sh -s` would read EOF, run nothing and exit 0 -- a silent + // wrong answer, worse than the degraded PATH this avoids. + runProcessMock.mockResolvedValue({ + code: 1, + signal: null, + stdout: '', + stderr: 'distro is stopped', + timedOut: false + }) + await runWslProcess({ lane: 'probe', script: 'echo hi' }) + expect(lastArgv()).toEqual(['--exec', 'sh', '-s', '--']) + expect(runProcessMock.mock.calls.at(-1)?.[0].input).toBe('echo hi') + }) +}) diff --git a/src/main/wsl/wsl-runner.ts b/src/main/wsl/wsl-runner.ts new file mode 100644 index 00000000000..f1df1d76391 --- /dev/null +++ b/src/main/wsl/wsl-runner.ts @@ -0,0 +1,205 @@ +import { addWslEnvKeys } from '../../shared/wsl-env' +import { runProcess } from '../../shared/child-process/run-process' +import { + buildWslCapturedLoginShellCommand, + buildWslExecArgs, + quotePosixShell +} from '../../shared/wsl-login-shell-command' +import { getWslGuestEnvironment, type WslGuestEnvironment } from './wsl-guest-environment' +import { resolveWslExecutablePath } from './wsl-executable-path' + +/** + * The single place Orca runs a program inside WSL. + * + * Five decisions have to be made on every `wsl.exe` call, each has a right + * answer, and each has shipped wrong: + * + * - **Separator.** `--` makes wsl.exe expand `$name` in every forwarded + * argument before the guest runs, even with no shell in the command, so + * `awk '{print $2}'` loses its field reference (#12964). Always `--exec`. + * - **Shell.** A login shell on a probe path sources `~/.profile`, so one + * blocking line eats the whole timeout (#14288) and every call pays startup + * (#9768). No login shell on a user-facing path means PATH does not match the + * user's terminal, so nvm-installed agents read as absent (#9725, #7563, + * #8366). Hence two lanes, chosen explicitly. + * - **Fencing.** An interactive login shell runs the distro rc, and stock + * Ubuntu writes its "run as administrator" hint to *stdout*, so anything + * parsing that stream reads the banner as data (#11327, #11823). + * - **WSLENV.** Unset, a Windows-side variable silently never crosses into the + * guest (#12557). + * - **Payload.** Scripts go in on stdin. A script on stdin has no quoting + * boundary to escape from, which is what the base64 and `eval` wrappers were + * working around (#14292). + */ + +export type WslLane = + /** No shell. Cached login PATH/HOME, applied via `env`. */ + | 'probe' + /** Login shell, always fenced. For anything whose PATH must match the user's terminal. */ + | 'interactive' + +/** + * What to run: a single binary, or a script. + * + * Why a union rather than an optional `script`: a script has to arrive on + * stdin, which means the program must be a shell reading stdin. Left to the + * caller that is a footgun — and the wrappers this replaces exist because it + * was never made explicit. `script` makes the runner supply `sh -s` itself. + */ +export type WslCommand = + | { program: string; args?: readonly string[]; script?: never } + | { script: string; args?: readonly string[]; program?: never } + +export type WslSpec = WslCommand & { + /** Undefined selects the distro's default. */ + distro?: string + /** + * Required, with no default. The wrong lane is the most common WSL defect in + * this tree, and a default lets a call site pick it by omission. + */ + lane: WslLane + /** Guest (POSIX) path. */ + cwd?: string + /** Host variables to propagate into the guest; sets WSLENV automatically. */ + env?: Readonly> + timeoutMs?: number + maxOutputBytes?: number + signal?: AbortSignal +} + +export type WslResult = { + code: number | null + /** Payload only — on the interactive lane the rc banner is removed by the fence. */ + stdout: string + stderr: string + timedOut: boolean +} + +export const DEFAULT_WSL_TIMEOUT_MS = 30_000 + +function assertGuestPath(cwd: string): void { + // Why reject rather than convert: a caller passing a Windows path here has + // usually made a different mistake further up, and silently translating it + // hides that. + if (!cwd.startsWith('/')) { + throw new Error(`WSL cwd must be a guest path, received ${cwd}`) + } +} + +function assertNotShellString(program: string): void { + // Why: the base64 and eval wrappers exist because this boundary was never + // enforced. `script` is the supported way to run a script. + // + // Why metacharacters and not whitespace: a guest binary may legitimately live + // under a path containing a space, and --exec passes it as one argv element, + // so a space is harmless. A `;` or `|` means the caller is building a command + // line, which is the thing being prevented. + if (/[;&|<>$`\n\r]/.test(program) || /^\S+\s+-/.test(program)) { + throw new Error(`WSL program must be a single binary, received ${program}`) + } +} + +/** Host env plus the WSLENV entries that let it cross the boundary. */ +function buildHostEnv(env: WslSpec['env']): NodeJS.ProcessEnv | undefined { + if (!env || Object.keys(env).length === 0) { + return undefined + } + const merged: NodeJS.ProcessEnv = { ...process.env, ...env } + addWslEnvKeys(merged, Object.keys(env)) + return merged +} + +/** + * `cd` into the guest cwd before the program. + * + * Why not `runProcess`'s `cwd`: that is a *Windows* directory for `wsl.exe`, + * not a guest one. Passing a guest path there fails, and passing the UNC form + * makes wsl.exe start in a network location. + */ +function withGuestCwd(cwd: string | undefined, argv: readonly string[]): string[] { + if (!cwd) { + return [...argv] + } + assertGuestPath(cwd) + return ['sh', '-c', 'cd "$1" || exit 1; shift; exec "$@"', 'orca-wsl', cwd, ...argv] +} + +/** `sh -s --` for a script on stdin, otherwise the program itself. */ +function guestCommandArgv(spec: WslSpec): string[] { + return spec.script !== undefined + ? ['sh', '-s', '--', ...(spec.args ?? [])] + : [spec.program, ...(spec.args ?? [])] +} + +/** Shell-free argv, with the cached environment applied when one is available. */ +function buildGuestArgv(environment: WslGuestEnvironment | null, spec: WslSpec): string[] { + const command = guestCommandArgv(spec) + const argv = environment + ? [environment.envBinary, `PATH=${environment.path}`, `HOME=${environment.home}`, ...command] + : command + return withGuestCwd(spec.cwd, argv) +} + +function buildInteractiveArgv(spec: WslSpec): { + argv: string[] + readStdout: (stdout: string) => string +} { + // Why the whole invocation goes through the fence: a caller that does not + // parse stdout today may start tomorrow, and the banner is invisible until + // then. + const quoted = guestCommandArgv(spec).map(quotePosixShell).join(' ') + const body = spec.cwd ? `cd ${quotePosixShell(spec.cwd)} || exit 1\n${quoted}` : quoted + const captured = buildWslCapturedLoginShellCommand(body) + return { + argv: ['sh', '-c', captured.command], + readStdout: (stdout: string) => captured.readStdout(stdout) ?? '' + } +} + +/** + * Run a program inside WSL. + * + * Falls back from the probe lane to the interactive lane when the distro's + * environment cannot be probed — an unprobed distro is "we could not ask", and + * running with no PATH at all would turn that into a wrong answer. + */ +export async function runWslProcess(spec: WslSpec): Promise { + if (spec.program !== undefined) { + assertNotShellString(spec.program) + } + if (spec.cwd) { + assertGuestPath(spec.cwd) + } + + const environment = spec.lane === 'probe' ? await getWslGuestEnvironment(spec.distro) : null + // Why a script never takes the interactive lane: the login shell owns stdin, + // and the script is delivered on stdin. If the shell consumes it first, the + // inner `sh -s` reads EOF, runs nothing, and exits 0 -- a silent wrong answer, + // which is strictly worse than the degraded PATH avoided by taking this path. + // A script therefore always runs as `--exec sh -s --`, with the cached + // environment applied when there is one. + const lane = + environment === null && spec.script === undefined + ? // Why fall back rather than run with no PATH: an unprobed distro is + // "we could not ask", and answering with an empty environment turns + // that into a wrong answer. + ({ kind: 'interactive', ...buildInteractiveArgv(spec) } as const) + : ({ kind: 'probe', argv: buildGuestArgv(environment, spec) } as const) + + const result = await runProcess({ + program: resolveWslExecutablePath(), + args: buildWslExecArgs(spec.distro, lane.argv), + env: buildHostEnv(spec.env), + input: spec.script, + timeoutMs: spec.timeoutMs ?? DEFAULT_WSL_TIMEOUT_MS, + maxOutputBytes: spec.maxOutputBytes, + signal: spec.signal + }) + + return { + code: result.code, + stdout: lane.kind === 'interactive' ? lane.readStdout(result.stdout) : result.stdout, + stderr: result.stderr, + timedOut: result.timedOut + } +} diff --git a/src/main/wsl/wsl-runner.wsl.test.ts b/src/main/wsl/wsl-runner.wsl.test.ts new file mode 100644 index 00000000000..d1f89a23927 --- /dev/null +++ b/src/main/wsl/wsl-runner.wsl.test.ts @@ -0,0 +1,125 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { runWslProcess } from './wsl-runner' +import { invalidateWslGuestEnvironment } from './wsl-guest-environment' +import { runProcess } from '../../shared/child-process/run-process' +import { resolveWslExecutablePath } from './wsl-executable-path' + +/** + * The assertions no unit test can make: a real distro, a real login shell, a + * real rc banner. + * + * Gated behind an env var and win32 because it mutates the distro's `~/.profile` + * to reproduce #14288. Run with: + * ORCA_REAL_WSL_RUNNER_TEST=1 pnpm vitest run src/main/wsl/wsl-runner.wsl.test.ts + */ +const DISTRO = process.env.ORCA_WSL_TEST_DISTRO ?? 'Ubuntu-24.04' +const enabled = process.platform === 'win32' && process.env.ORCA_REAL_WSL_RUNNER_TEST === '1' +const describeOnWsl = enabled ? describe : describe.skip + +const PROFILE = '/tmp/orca-wsl-runner-profile.bak' + +async function guest(script: string): Promise { + const result = await runProcess({ + program: resolveWslExecutablePath(), + args: ['-d', DISTRO, '--exec', 'sh', '-c', script], + timeoutMs: 30_000 + }) + return result.stdout.trim() +} + +describeOnWsl('runWslProcess against a real distro', () => { + beforeAll(async () => { + // Save whatever profile exists, then make it block for far longer than the + // probe budget -- this is #14288 reproduced, not simulated. + await guest(`cp -f "$HOME/.profile" ${PROFILE} 2>/dev/null || true`) + await guest(`printf '\\nsleep 60\\n' >> "$HOME/.profile"`) + invalidateWslGuestEnvironment() + }, 120_000) + + afterAll(async () => { + await guest(`cp -f ${PROFILE} "$HOME/.profile" 2>/dev/null || rm -f "$HOME/.profile"`) + await guest(`rm -f ${PROFILE}`) + invalidateWslGuestEnvironment() + }, 120_000) + + it('probe lane survives a ~/.profile that blocks for a minute', async () => { + // The first call pays one login shell (which the blocking profile stalls, + // so it times out and the lane degrades); every later call must not. + // What must never happen is the probe lane inheriting the stall per call. + const started = Date.now() + const result = await runWslProcess({ + lane: 'probe', + distro: DISTRO, + program: '/bin/echo', + args: ['orca-probe-ok'], + timeoutMs: 15_000 + }) + const elapsed = Date.now() - started + expect(result.stdout.trim()).toContain('orca-probe-ok') + expect(elapsed).toBeLessThan(20_000) + }, 60_000) + + it('second probe-lane call does not pay the login shell again', async () => { + const started = Date.now() + await runWslProcess({ lane: 'probe', distro: DISTRO, program: '/bin/true', timeoutMs: 15_000 }) + expect(Date.now() - started).toBeLessThan(5_000) + }, 30_000) + + it('interactive lane strips the distro banner from parsed stdout', async () => { + // Stock Ubuntu writes its rc hint to stdout. Anything parsing that stream + // reads the banner as data unless the fence removes it (#11327, #11823). + const result = await runWslProcess({ + lane: 'interactive', + distro: DISTRO, + program: '/bin/echo', + args: ['ORCA_PAYLOAD'], + timeoutMs: 60_000 + }) + expect(result.stdout.trim()).toBe('ORCA_PAYLOAD') + }, 90_000) + + it('a script with quotes and $ arrives byte-identical', async () => { + // Both hazards in one payload: `--` would expand $2 host-side, and the + // base64/eval wrappers broke on the embedded quotes (#12964, #14292). + const script = [ + `printf '%s\\n' "$1"`, + `echo 'it'\\''s fine'`, + `echo "x" | awk '{print $1}'` + ].join('\n') + const result = await runWslProcess({ + lane: 'probe', + distro: DISTRO, + script, + args: ['ORCA_ARG'], + timeoutMs: 30_000 + }) + expect( + result.stdout + .split('\n') + .map((l) => l.trim()) + .filter(Boolean) + ).toEqual(['ORCA_ARG', "it's fine", 'x']) + }, 60_000) + + it('propagated env crosses the boundary via WSLENV', async () => { + const result = await runWslProcess({ + lane: 'probe', + distro: DISTRO, + script: 'printf %s "$ORCA_WSLENV_PROBE"', + env: { ORCA_WSLENV_PROBE: 'crossed' }, + timeoutMs: 30_000 + }) + expect(result.stdout.trim()).toBe('crossed') + }, 60_000) + + it('runs in the requested guest cwd', async () => { + const result = await runWslProcess({ + lane: 'probe', + distro: DISTRO, + program: '/bin/pwd', + cwd: '/tmp', + timeoutMs: 30_000 + }) + expect(result.stdout.trim()).toBe('/tmp') + }, 60_000) +})