From 2c0201aed5e57144ad29b20a252f5b5cbbbc1cbf Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:49:56 -0700 Subject: [PATCH] refactor(wsl): delete the environment-policy layer the reviews kept failing on A design council (Opus, Grok, GPT-5.6-Sol) reviewed the merged runner after it took eleven review rounds to land. All three reached the same conclusion: the invocation half is sound, the environment/probe half is not, and every round had been debugging the second one. The finding that settled it, from Opus: `environmentResolved` had **54 references, all in tests and the runner itself. Not one production reader.** The safety mechanism the strict default existed for was never wired to anything, so all 19 degrading sites reported absence with full confidence anyway -- #9725 live at every one, under comments claiming it was handled. Two of those comments say so out loud; I wrote them. Root cause, in one line: every knob existed only because a failed probe was fatal. So it no longer is. - `allowDegradedEnvironment` and `WslGuestEnvironmentUnavailableError` are gone. A missing login PATH is a fact in the result, not an exception. That deletes 23 opt-outs, six catch-and-remap blocks, the transient/rejected cooldown split, `probedWithBudget`, and the 1.5x re-probe heuristic -- none of which had a reason to exist once the case stopped throwing. - `lane` + `allowDegradedEnvironment` collapse into `loginPath: 'none' | 'preferred'`. 19 of 23 sites passed the opt-out, and two said in comments that they did not want the login PATH at all: the flag had become the `'none'` the union was missing. - The `interactive` lane is deleted. It had zero production callers and kept ~30 lines of fence plumbing alive for tests only. Net -98 production lines; the runner itself sheds 86 for 38. Also carries three fixes from the W3 orphan-PR sweep I had not done: - `WSL_UTF8=1` in the runner. My relay migration deleted the only place setting it, so wsl.exe's own error text arrived UTF-16LE and read as NUL-riddled. A regression I introduced. Credit: #9010 (Chang-Jin-Lee). - `GITLAB_HOST` is now named in WSLENV, so a ported self-hosted host actually crosses into a distro-routed glab (#12557). Credit: #12558 (makoto-developer). - The WSL skill-setup command pipes into `sh` instead of `eval "$(...)"`, whose nested quoting produced `word unexpected (expecting "in")` (#14292). Credit: #14785 (innocarpe). --- src/main/agent-hooks/wsl-hook-relay-launch.ts | 6 +- .../claude-accounts/runtime-auth-service.ts | 7 +- src/main/claude-accounts/service.ts | 26 +-- src/main/cli/wsl-cli-installer.ts | 2 +- .../service-wsl-accounts.test.ts | 4 +- src/main/codex-accounts/service.ts | 18 +- .../codex/wsl-codex-session-bridge.test.ts | 4 +- src/main/codex/wsl-codex-session-bridge.ts | 2 +- src/main/git/runner.test.ts | 16 ++ src/main/git/runner.ts | 7 +- src/main/hooks.test.ts | 4 +- src/main/hooks.ts | 7 +- .../ipc/preflight-agent-detection.test.ts | 4 +- .../ipc/preflight-host-cli-status.test.ts | 4 +- .../ipc/preflight-wsl-agent-detection.test.ts | 6 +- src/main/ipc/preflight-wsl-agent-detection.ts | 9 +- src/main/ipc/preflight-wsl-command.test.ts | 7 +- src/main/ipc/preflight-wsl-command.ts | 7 +- .../skills/claude-plugin-skill-sources-wsl.ts | 4 +- src/main/skills/skill-discovery-wsl.ts | 4 +- .../skills/skill-provider-runtime-roots.ts | 4 +- .../skills/skill-wsl-install-filesystem.ts | 4 +- .../skill-wsl-provider-detection.test.ts | 6 +- .../skills/skill-wsl-provider-detection.ts | 2 +- src/main/wsl-fish-history-cleanup.test.ts | 3 +- src/main/wsl-fish-history-cleanup.ts | 5 +- src/main/wsl/wsl-runner.test.ts | 155 +++++------------- src/main/wsl/wsl-runner.ts | 124 +++++--------- src/main/wsl/wsl-runner.wsl.test.ts | 17 +- src/main/wsl/wsl-w1-w3-contract.test.ts | 32 ++-- .../settings/CliSkillRuntimeSetup.test.tsx | 12 +- .../settings/CliSkillRuntimeSetup.tsx | 15 +- 32 files changed, 192 insertions(+), 335 deletions(-) diff --git a/src/main/agent-hooks/wsl-hook-relay-launch.ts b/src/main/agent-hooks/wsl-hook-relay-launch.ts index e4f5bc24978..caa32627343 100644 --- a/src/main/agent-hooks/wsl-hook-relay-launch.ts +++ b/src/main/agent-hooks/wsl-hook-relay-launch.ts @@ -181,14 +181,10 @@ export async function runWslInstallProcess( ): Promise<{ code: number | null; stderr: string }> { const result = await runWslProcess({ distro, - lane: 'probe', + loginPath: 'none', script, timeoutMs: INSTALL_TIMEOUT_MS, maxOutputBytes: MAX_STARTUP_BUFFER_BYTES, - // The install script only shells out to base64/mkdir/mv/chmod, all on any - // default PATH -- it must still run when the login-PATH probe itself is - // what's wedged, which is exactly the state this call recovers from. - allowDegradedEnvironment: true }) return result.timedOut ? { code: null, stderr: `${result.stderr}\ninstall timed out after ${INSTALL_TIMEOUT_MS}ms` } diff --git a/src/main/claude-accounts/runtime-auth-service.ts b/src/main/claude-accounts/runtime-auth-service.ts index 29568d38fe4..bc80a4025dc 100644 --- a/src/main/claude-accounts/runtime-auth-service.ts +++ b/src/main/claude-accounts/runtime-auth-service.ts @@ -1088,7 +1088,7 @@ export class ClaudeRuntimeAuthService { try { const owned = await runWslProcess({ distro: wslInfo.distro, - lane: 'probe', + loginPath: 'none', shell: 'bash', script: [ 'set -euo pipefail', @@ -1101,11 +1101,6 @@ export class ClaudeRuntimeAuthService { 'case "$candidate_real" in "$managed_root_real"/*/auth) printf "%s\\n" "$candidate_real" ;; *) exit 35 ;; esac' ].join('\n'), timeoutMs: 5000, - // Why degraded is allowed: a failed answer here disowns the account and - // clears the user's selection, so a slow distro probe must not decide - // ownership. The script reads only $HOME and coreutils, both of which - // wsl.exe supplies without the login PATH. - allowDegradedEnvironment: true }) if (owned.timedOut) { throw new Error(OWNERSHIP_PROBE_TIMEOUT) diff --git a/src/main/claude-accounts/service.ts b/src/main/claude-accounts/service.ts index e9f18a8a5d9..a74380981c9 100644 --- a/src/main/claude-accounts/service.ts +++ b/src/main/claude-accounts/service.ts @@ -664,13 +664,10 @@ export class ClaudeAccountService { } const created = await runWslProcess({ distro: location.wslDistro, - lane: 'probe', + loginPath: 'none', shell: 'bash', script: 'mktemp -d "${TMPDIR:-/tmp}/orca-claude-login.XXXXXX"', timeoutMs: 5000, - // Why degraded is allowed: mktemp is a coreutil on the default PATH, so an - // unprobed distro must not turn a working sign-in into a failed one. - allowDegradedEnvironment: true }) const linuxPath = created.stdout.replaceAll(String.fromCharCode(0), '').trim() if (created.code !== 0 || created.timedOut || !linuxPath.startsWith('/')) { @@ -692,13 +689,10 @@ export class ClaudeAccountService { try { await runWslProcess({ distro: tempConfig.wslDistro, - lane: 'probe', + loginPath: 'none', program: 'rm', args: ['-rf', '--', tempConfig.linuxPath], timeoutMs: 5000, - // Why degraded is allowed: leaving a login temp dir behind is worse than - // running rm on the default PATH, and this is already best-effort. - allowDegradedEnvironment: true }) } catch { // Best-effort cleanup. @@ -910,13 +904,10 @@ export class ClaudeAccountService { const requestedDistro = target.wslDistro?.trim() || undefined const info = await runWslProcess({ distro: requestedDistro, - lane: 'probe', + loginPath: 'none', shell: 'bash', script: 'printf "%s\\n%s\\n" "$WSL_DISTRO_NAME" "$HOME"', timeoutMs: 5000, - // Why degraded is allowed: both variables come from wsl.exe itself, not from - // the login PATH, so an unprobed distro still answers correctly here. - allowDegradedEnvironment: true }) const [rawDistro, rawHome] = info.code === 0 && !info.timedOut @@ -934,14 +925,11 @@ export class ClaudeAccountService { const wslLinuxAuthPath = `${home.replace(/\/$/, '')}/.local/share/orca/claude-accounts/${accountId}/auth` const created = await runWslProcess({ distro, - lane: 'probe', + loginPath: 'none', shell: 'bash', script: 'mkdir -p "$1" && printf \'%s\\n\' "$2" > "$1/.orca-managed-claude-auth"', args: [wslLinuxAuthPath, accountId], timeoutMs: 5000, - // Why degraded is allowed: mkdir is a coreutil on the default PATH, and the - // target path was already resolved from the guest's own $HOME above. - allowDegradedEnvironment: true }) if (created.code !== 0 || created.timedOut) { throw new Error('Could not create the managed WSL Claude auth directory.') @@ -978,7 +966,7 @@ export class ClaudeAccountService { try { const owned = await runWslProcess({ distro: wslInfo.distro, - lane: 'probe', + loginPath: 'none', shell: 'bash', script: [ 'set -euo pipefail', @@ -993,10 +981,6 @@ export class ClaudeAccountService { 'case "$candidate_real" in "$managed_root_real"/*/auth) printf "%s\\n" "$candidate_real" ;; *) exit 35 ;; esac' ].join('\n'), timeoutMs: 5000, - // Why degraded is allowed: the ownership proof is the script's own - // marker and containment checks, not the login PATH, and refusing here - // would fail every managed read/write on a distro Orca could not probe. - allowDegradedEnvironment: true }) if (owned.code !== 0 || owned.timedOut) { throw new Error('Managed Claude auth directory does not exist on disk.') diff --git a/src/main/cli/wsl-cli-installer.ts b/src/main/cli/wsl-cli-installer.ts index ca38f660972..37491e28889 100644 --- a/src/main/cli/wsl-cli-installer.ts +++ b/src/main/cli/wsl-cli-installer.ts @@ -463,7 +463,7 @@ async function runWslCommand(distro: string, command: string): Promise { try { result = await runWslProcess({ distro, - lane: 'probe', + loginPath: 'preferred', script: command, timeoutMs: WSL_COMMAND_TIMEOUT_MS }) diff --git a/src/main/codex-accounts/service-wsl-accounts.test.ts b/src/main/codex-accounts/service-wsl-accounts.test.ts index 3e62db7e59e..50749b3f0ef 100644 --- a/src/main/codex-accounts/service-wsl-accounts.test.ts +++ b/src/main/codex-accounts/service-wsl-accounts.test.ts @@ -71,7 +71,9 @@ describe('CodexAccountService config sync', () => { const runWslProcessMock = vi.fn(async (spec: WslSpec) => { const script = String(spec.script) expect(spec.distro).toBe('Debian') - expect(spec.lane).toBe('probe') + // 'none': the script reads $HOME and $WSL_DISTRO_NAME, which wsl.exe + // supplies from /etc/passwd without a login shell. + expect(spec.loginPath).toBe('none') if (script.includes('WSL_DISTRO_NAME')) { return wslOk('Debian\n/home/alice\n') } diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index 8874cc0b849..a81b60bdeab 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -1215,9 +1215,7 @@ export class CodexAccountService { const requestedDistro = target.wslDistro?.trim() || undefined const info = await runWslProcess({ distro: requestedDistro, - lane: 'probe', - // Degrade rather than refuse: $HOME and $WSL_DISTRO_NAME come from wsl.exe itself, not the login PATH -- the same rationale the Claude sites use. - allowDegradedEnvironment: true, + loginPath: 'none', script: 'printf "%s\\n%s\\n" "$WSL_DISTRO_NAME" "$HOME"', shell: 'bash', timeoutMs: WSL_MANAGED_HOME_TIMEOUT_MS @@ -1239,9 +1237,7 @@ export class CodexAccountService { const markerPath = `${wslLinuxHomePath}/.orca-managed-home` const created = await runWslProcess({ distro, - lane: 'probe', - // Degrade rather than refuse: $HOME and $WSL_DISTRO_NAME come from wsl.exe itself, not the login PATH -- the same rationale the Claude sites use. - allowDegradedEnvironment: true, + loginPath: 'none', script: `mkdir -p ${shellQuote(wslLinuxHomePath)} && printf '%s\\n' ${shellQuote(accountId)} > ${shellQuote(markerPath)}`, shell: 'bash', timeoutMs: WSL_MANAGED_HOME_TIMEOUT_MS @@ -1478,9 +1474,7 @@ export class CodexAccountService { const result = await runWslProcess({ distro: wslInfo.distro, - lane: 'probe', - // Degrade rather than refuse: $HOME and $WSL_DISTRO_NAME come from wsl.exe itself, not the login PATH -- the same rationale the Claude sites use. - allowDegradedEnvironment: true, + loginPath: 'none', script: [ 'set -euo pipefail', `candidate=${shellQuote(wslInfo.linuxPath)}`, @@ -1616,9 +1610,7 @@ export class CodexAccountService { try { const result = await runWslProcess({ distro, - lane: 'probe', - // Degrade rather than refuse: $HOME and $WSL_DISTRO_NAME come from wsl.exe itself, not the login PATH -- the same rationale the Claude sites use. - allowDegradedEnvironment: true, + loginPath: 'none', script: [ 'set -euo pipefail', `candidate=${shellQuote(linuxHomePath)}`, @@ -1872,7 +1864,7 @@ export class CodexAccountService { try { result = await runWslProcess({ distro: wslInfo.distro, - lane: 'probe', + loginPath: 'none', script: buildWslCodexAvailabilityScript(), timeoutMs: WSL_CODEX_AVAILABILITY_TIMEOUT_MS }) diff --git a/src/main/codex/wsl-codex-session-bridge.test.ts b/src/main/codex/wsl-codex-session-bridge.test.ts index f71474528be..584970d048b 100644 --- a/src/main/codex/wsl-codex-session-bridge.test.ts +++ b/src/main/codex/wsl-codex-session-bridge.test.ts @@ -44,13 +44,13 @@ describe('syncWslCodexSessionsIntoManagedHome', () => { expect(runWslProcessMock).toHaveBeenCalledTimes(1) const spec = runWslProcessMock.mock.calls[0]?.[0] as { distro: string - lane: string + loginPath: string shell?: string script: string timeoutMs: number } expect(spec.distro).toBe('Ubuntu') - expect(spec.lane).toBe('probe') + expect(spec.loginPath).toBe('none') expect(spec.shell).toBe('bash') expect(spec.timeoutMs).toBe(30_000) diff --git a/src/main/codex/wsl-codex-session-bridge.ts b/src/main/codex/wsl-codex-session-bridge.ts index 9b5cbf0895d..f65dfdbaa03 100644 --- a/src/main/codex/wsl-codex-session-bridge.ts +++ b/src/main/codex/wsl-codex-session-bridge.ts @@ -56,7 +56,7 @@ export async function syncWslCodexSessionsIntoManagedHome( const result = await runWslProcess({ distro: target.distro, - lane: 'probe', + loginPath: 'none', script: buildWslCodexSessionBridgeShellCommand(paths), // Process substitution and `read -d` are bash-only; dash rejects both. shell: 'bash', diff --git a/src/main/git/runner.test.ts b/src/main/git/runner.test.ts index 92832fc0f89..897aac4cfdd 100644 --- a/src/main/git/runner.test.ts +++ b/src/main/git/runner.test.ts @@ -338,3 +338,19 @@ describe('git env forces untranslated diagnostics (issue #7808)', () => { expect(env.LANGUAGE).toBe('en') }) }) + +describe('redirectPortedHostnameToEnv WSLENV forwarding', () => { + it('names GITLAB_HOST in WSLENV so it can cross into a distro', async () => { + // A WSL-routed glab only sees Windows variables listed in WSLENV; without + // the entry the ported host is silently dropped and glab talks to + // gitlab.com (#12557). + const { redirectPortedHostnameToEnv } = await import('./runner') + const { options } = redirectPortedHostnameToEnv( + ['api', '--hostname', 'gitlab.example.com:8443'], + { env: { PATH: '/usr/bin' } } + ) + expect(options.env?.GITLAB_HOST).toBe('gitlab.example.com:8443') + expect((options.env?.WSLENV ?? '').split(':')).toContain('GITLAB_HOST') + }) +}) + diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 9f5e3346dcf..037094672e8 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1908,9 +1908,14 @@ export function redirectPortedHostnameToEnv( if (!/^[^/\s]+:\d+$/.test(host)) { return { args, options } } + // Why WSLENV: a glab routed into a distro only sees Windows-side variables + // named in WSLENV, so without this the ported host silently never crosses and + // glab talks to gitlab.com instead (#12557). Credit: #12558. + const env: NodeJS.ProcessEnv = { ...(options.env ?? process.env), GITLAB_HOST: host } + addWslEnvKeys(env, ['GITLAB_HOST']) return { args: [...args.slice(0, i), ...args.slice(i + 2)], - options: { ...options, env: { ...(options.env ?? process.env), GITLAB_HOST: host } } + options: { ...options, env } } } diff --git a/src/main/hooks.test.ts b/src/main/hooks.test.ts index 23293ce8b48..d2dbb578b8f 100644 --- a/src/main/hooks.test.ts +++ b/src/main/hooks.test.ts @@ -164,7 +164,7 @@ describe('runHook', () => { expect(runWslProcessMock).toHaveBeenCalledWith( expect.objectContaining({ distro: 'Ubuntu', - lane: 'probe', + loginPath: 'preferred', script: 'echo hello', cwd: '/home/jin/feature', // #7652 regression: the unattended WSL hook branch must carry the @@ -220,7 +220,7 @@ describe('runHook', () => { expect(runWslProcessMock).toHaveBeenCalledWith( expect.objectContaining({ distro: 'Ubuntu', - lane: 'probe', + loginPath: 'preferred', script: 'echo hello', cwd: '/mnt/c/Users/jinwo/git/orca-feature', env: expect.objectContaining({ diff --git a/src/main/hooks.ts b/src/main/hooks.ts index d55647e6737..3560b20d2e3 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -153,17 +153,12 @@ export function runHook( return runWslProcess({ distro: wslInfo.distro ?? undefined, - lane: 'probe', + loginPath: 'preferred', script, // Why pinned: these are user-authored orca.yaml scripts and the native // path runs /bin/bash. Defaulting to sh would fail bash-only hooks on WSL // only -- a downgrade the user never asked for. shell: 'bash', - // Why degrade rather than fail: a hook is an action, not an "is this - // installed?" question. Before the runner it ran on the default PATH when - // no login shell was available; refusing would turn worktree setup into a - // hard failure whenever WSL is briefly slow. - allowDegradedEnvironment: true, cwd: wslInfo.linuxPath, env: guestEnv, timeoutMs: HOOK_TIMEOUT diff --git a/src/main/ipc/preflight-agent-detection.test.ts b/src/main/ipc/preflight-agent-detection.test.ts index 02a47f7570f..925969df31e 100644 --- a/src/main/ipc/preflight-agent-detection.test.ts +++ b/src/main/ipc/preflight-agent-detection.test.ts @@ -405,7 +405,7 @@ describe('preflight', () => { // pinned by its own tests. What this suite owns is that detection asks the // right distro on the lane that carries the user's PATH. expect(runWslProcessMock).toHaveBeenCalledWith( - expect.objectContaining({ distro: 'Ubuntu', lane: 'probe' }) + expect.objectContaining({ distro: 'Ubuntu', loginPath: 'preferred' }) ) }) @@ -433,7 +433,7 @@ describe('preflight', () => { expect(resolveCliCommandsMock).not.toHaveBeenCalled() // No distro named: the runner resolves the default. expect(runWslProcessMock).toHaveBeenCalledWith( - expect.objectContaining({ distro: undefined, lane: 'probe' }) + expect.objectContaining({ distro: undefined, loginPath: 'preferred' }) ) }) }) diff --git a/src/main/ipc/preflight-host-cli-status.test.ts b/src/main/ipc/preflight-host-cli-status.test.ts index 63cb6657b92..ed45e9f8719 100644 --- a/src/main/ipc/preflight-host-cli-status.test.ts +++ b/src/main/ipc/preflight-host-cli-status.test.ts @@ -296,14 +296,14 @@ describe('preflight', () => { expect(runWslProcessMock).toHaveBeenCalledWith( expect.objectContaining({ distro: 'Ubuntu', - lane: 'probe', + loginPath: 'preferred', script: expect.stringMatching(/gh[\s\S]*--version/) }) ) expect(runWslProcessMock).toHaveBeenCalledWith( expect.objectContaining({ distro: 'Ubuntu', - lane: 'probe', + loginPath: 'preferred', script: expect.stringMatching(/gh[\s\S]*auth status/) }) ) diff --git a/src/main/ipc/preflight-wsl-agent-detection.test.ts b/src/main/ipc/preflight-wsl-agent-detection.test.ts index 2b15efe37fc..549224df14a 100644 --- a/src/main/ipc/preflight-wsl-agent-detection.test.ts +++ b/src/main/ipc/preflight-wsl-agent-detection.test.ts @@ -6,7 +6,7 @@ vi.mock('../wsl/wsl-runner', () => ({ runWslProcess: runWslProcessMock })) import { detectWslCommandsOnPath } from './preflight-wsl-agent-detection' import { buildPosixCommandPathLookupScript } from '../../shared/posix-command-path-lookup' -type RunWslProcessSpec = { distro?: string; lane: string; script: string } +type RunWslProcessSpec = { distro?: string; loginPath: string; script: string } function lastSpec(): RunWslProcessSpec { const call = runWslProcessMock.mock.calls.at(-1) @@ -24,11 +24,11 @@ describe('detectWslCommandsOnPath', () => { vi.restoreAllMocks() }) - it('runs on the probe lane -- no shell, so no rc/motd banner can appear', async () => { + it('asks for the login PATH without running a shell, so no banner can appear', async () => { await detectWslCommandsOnPath({ distro: 'Ubuntu' }, ['claude']) const spec = lastSpec() - expect(spec.lane).toBe('probe') + expect(spec.loginPath).toBe('preferred') expect(spec.distro).toBe('Ubuntu') }) diff --git a/src/main/ipc/preflight-wsl-agent-detection.ts b/src/main/ipc/preflight-wsl-agent-detection.ts index b2c2c02309b..7ec44e5f66e 100644 --- a/src/main/ipc/preflight-wsl-agent-detection.ts +++ b/src/main/ipc/preflight-wsl-agent-detection.ts @@ -38,15 +38,8 @@ export async function detectWslCommandsOnPath( // with no shell in the loop, so there is no rc/motd banner to land in stdout. const result = await runWslProcess({ distro: wslTarget.distro, - lane: 'probe', + loginPath: 'preferred', script, - // Why degrade rather than refuse: the Set has no room for "unverifiable", - // and refusing would turn a slow distro into "no agents" anyway -- via a - // throw instead of an empty result. Degrading at least finds anything on - // the default PATH. The residual gap (an nvm-only agent missed during the - // probe's retry window, #9725) is the pre-migration behaviour, not new, - // and Refresh now re-probes. - allowDegradedEnvironment: true, timeoutMs: WSL_AGENT_DETECTION_TIMEOUT_MS }) // runProcess resolves on a timeout and on a non-zero exit, so partial diff --git a/src/main/ipc/preflight-wsl-command.test.ts b/src/main/ipc/preflight-wsl-command.test.ts index 931b1b1275b..f9d69c5ebed 100644 --- a/src/main/ipc/preflight-wsl-command.test.ts +++ b/src/main/ipc/preflight-wsl-command.test.ts @@ -17,15 +17,14 @@ beforeEach(() => { }) describe('runPreflightCommandInWsl', () => { - it('degrades rather than letting a slow distro read as "not installed"', async () => { + it('prefers the login PATH but never lets its absence decide the answer', async () => { // Every caller collapses a throw into a verdict: isCommandAvailable returns // false ("not installed"), isGhAuthenticated reads an empty payload as - // "not authenticated". Refusing here turns a slow distro into a confident - // wrong answer -- #9725 through the other door. + // "not authenticated". A missing login PATH must therefore never be fatal. await runPreflightCommandInWsl({ distro: 'Ubuntu' }, 'gh --version', 5_000) expect(runWslProcessMock).toHaveBeenCalledWith( - expect.objectContaining({ allowDegradedEnvironment: true, distro: 'Ubuntu' }) + expect.objectContaining({ loginPath: 'preferred', distro: 'Ubuntu' }) ) }) diff --git a/src/main/ipc/preflight-wsl-command.ts b/src/main/ipc/preflight-wsl-command.ts index 4729ea38e1d..0b5663271de 100644 --- a/src/main/ipc/preflight-wsl-command.ts +++ b/src/main/ipc/preflight-wsl-command.ts @@ -12,13 +12,8 @@ export async function runPreflightCommandInWsl( // with no shell in the loop, so there is no rc/motd banner to strip. const result = await runWslProcess({ distro: target.distro, - lane: 'probe', + loginPath: 'preferred', script: command, - // Why degrade: every caller collapses a throw into "not installed" or - // "not authenticated" (isCommandAvailable, isGhAuthenticated). Refusing - // here turns a slow distro into a confident wrong answer -- #9725 through - // the other door, on the branch built to close it. - allowDegradedEnvironment: true, timeoutMs }) // runWslProcess resolves on a timeout and on a non-zero exit; the caller's diff --git a/src/main/skills/claude-plugin-skill-sources-wsl.ts b/src/main/skills/claude-plugin-skill-sources-wsl.ts index 083446229b0..08d4d0df279 100644 --- a/src/main/skills/claude-plugin-skill-sources-wsl.ts +++ b/src/main/skills/claude-plugin-skill-sources-wsl.ts @@ -61,9 +61,7 @@ async function executeWslMetadataRead(distro: string, script: string): Promise Promise async function probeWslGrokHome(distro: string): Promise { const result = await runWslProcess({ distro, - lane: 'probe', - // Degrade rather than refuse: reads $HOME, which wsl.exe supplies without a login shell. - allowDegradedEnvironment: true, + loginPath: 'preferred', script: WSL_GROK_HOME_SCRIPT, timeoutMs: WSL_ENV_PROBE_TIMEOUT_MS, maxOutputBytes: WSL_ENV_PROBE_MAX_BYTES diff --git a/src/main/skills/skill-wsl-install-filesystem.ts b/src/main/skills/skill-wsl-install-filesystem.ts index 8a5a863efb2..bbb19ff9a99 100644 --- a/src/main/skills/skill-wsl-install-filesystem.ts +++ b/src/main/skills/skill-wsl-install-filesystem.ts @@ -213,9 +213,7 @@ export class WslSkillInstallFilesystem implements SkillInstallFilesystem { private async runOutput(script: string, args: string[]): Promise { const result = await runWslProcess({ distro: this.distro, - lane: 'probe', - // Degrade rather than refuse: mkdir/mv/chmod on the default PATH; no login shell before. - allowDegradedEnvironment: true, + loginPath: 'none', script, args, timeoutMs: GUEST_COMMAND_TIMEOUT_MS, diff --git a/src/main/skills/skill-wsl-provider-detection.test.ts b/src/main/skills/skill-wsl-provider-detection.test.ts index c5117116c4a..d33fffb9b91 100644 --- a/src/main/skills/skill-wsl-provider-detection.test.ts +++ b/src/main/skills/skill-wsl-provider-detection.test.ts @@ -5,7 +5,7 @@ vi.mock('../wsl/wsl-runner', () => ({ runWslProcess: runWslProcessMock })) import { detectSkillProvidersInWsl } from './skill-wsl-provider-detection' -type RunWslProcessSpec = { distro: string; lane: string; script: string } +type RunWslProcessSpec = { distro: string; loginPath: string; script: string } describe('detectSkillProvidersInWsl', () => { beforeEach(() => { @@ -16,7 +16,7 @@ describe('detectSkillProvidersInWsl', () => { vi.restoreAllMocks() }) - it('runs on the probe lane so a PATH-only install (nvm/mise) is still found (regression)', async () => { + it('asks for the login PATH so a nvm/mise-only install is still found (regression)', async () => { // Before this migration the site ran `sh -c` with no login shell, so an // nvm-installed codex/claude -- reachable only through the PATH a login // shell assembles from rc files -- resolved to nothing via `command -v` @@ -32,7 +32,7 @@ describe('detectSkillProvidersInWsl', () => { const found = await detectSkillProvidersInWsl('Ubuntu') const [spec] = runWslProcessMock.mock.calls.at(-1) as [RunWslProcessSpec] - expect(spec.lane).toBe('probe') + expect(spec.loginPath).toBe('preferred') expect(spec.distro).toBe('Ubuntu') expect(found).toEqual(['codex']) }) diff --git a/src/main/skills/skill-wsl-provider-detection.ts b/src/main/skills/skill-wsl-provider-detection.ts index 9b5ddf531ac..0c4c9292be3 100644 --- a/src/main/skills/skill-wsl-provider-detection.ts +++ b/src/main/skills/skill-wsl-provider-detection.ts @@ -13,7 +13,7 @@ export async function detectSkillProvidersInWsl(distro: string): Promise { expect(run).toHaveBeenCalledWith({ distro: 'Ubuntu Test', - lane: 'probe', - allowDegradedEnvironment: true, + loginPath: 'none', program: 'fish', args: ['--command', expect.stringContaining('orca_0123456789abcdef_history')], timeoutMs: 5_000 diff --git a/src/main/wsl-fish-history-cleanup.ts b/src/main/wsl-fish-history-cleanup.ts index c278d2b4bba..7fdb21771a3 100644 --- a/src/main/wsl-fish-history-cleanup.ts +++ b/src/main/wsl-fish-history-cleanup.ts @@ -50,11 +50,8 @@ async function runCleanup( ): Promise { const result = await run({ distro, - lane: 'probe', + loginPath: 'none', // The old `--exec fish` spawn never sourced a login shell either, so a - // probe failure must run degraded rather than turn a working cleanup into - // an error. - allowDegradedEnvironment: true, program: 'fish', args: ['--command', fishCleanupScript(session)], timeoutMs: 5_000 diff --git a/src/main/wsl/wsl-runner.test.ts b/src/main/wsl/wsl-runner.test.ts index 6c71e4ee4cc..8aaa3287f0b 100644 --- a/src/main/wsl/wsl-runner.test.ts +++ b/src/main/wsl/wsl-runner.test.ts @@ -54,23 +54,20 @@ afterEach(() => { }) describe('separator', () => { - it.each([ - ['probe', 'probe'], - ['interactive', 'interactive'] - ] as const)('uses --exec and never -- on the %s lane', async (_name, lane) => { + it.each([['none'], ['preferred']] as const)('uses --exec and never -- with loginPath %s', async (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'] }) + await runWslProcess({ loginPath: 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' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/true' }) expect(lastArgv().slice(0, 3)).toEqual(['-d', 'Ubuntu', '--exec']) }) }) @@ -80,7 +77,7 @@ describe('probe lane', () => { // 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'] }) + await runWslProcess({ loginPath: 'preferred', program: 'codex', args: ['--version'] }) expect(lastArgv()).toEqual([ '--exec', '/usr/bin/env', @@ -91,67 +88,30 @@ describe('probe lane', () => { ]) }) - it('reports an unresolved environment rather than pretending the PATH is real', async () => { - // Falling back to the login shell here would re-run ~/.profile -- the very - // stall the probe lane exists to avoid, and most likely to bite exactly - // when the probe just failed. So the call proceeds on the default PATH and - // says so, and callers deciding "installed?" must treat that as unknown. - let call = 0 - runProcessMock.mockImplementation(async (spec: { args: string[] }) => { - call += 1 - if (call === 1) { - return { environmentResolved: true, code: 1, signal: null, stdout: '', stderr: 'stopped', timedOut: false } - } - 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 { environmentResolved: true, code: 0, signal: null, stdout: `${begin}${end}`, stderr: '', timedOut: false } + it('runs anyway and says the PATH is unresolved', async () => { + // A missing login PATH used to throw, and every knob this runner carried -- + // cooldown tiers, budget splitting, a re-probe heuristic, an opt-out on 19 + // of 23 sites -- existed to work around that. It is now just a fact in the + // result. + runProcessMock.mockResolvedValue({ + code: 1, + signal: null, + stdout: '', + stderr: 'distro is stopped', + timedOut: false }) - // Default is to refuse: answering "is codex installed?" on the bare default - // PATH reports an nvm install as absent, which is #9725. - await expect(runWslProcess({ lane: 'probe', program: 'codex' })).rejects.toThrow( - /guest environment/ - ) - - const degraded = await runWslProcess({ - lane: 'probe', - program: 'codex', - allowDegradedEnvironment: true - }) - expect(degraded.environmentResolved).toBe(false) + const result = await runWslProcess({ loginPath: 'preferred', program: 'codex' }) + expect(result.environmentResolved).toBe(false) expect(lastArgv()).toEqual(['--exec', 'codex']) }) it('reports a resolved environment on the happy path', async () => { seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - const result = await runWslProcess({ lane: 'probe', program: 'codex' }) + const result = await runWslProcess({ loginPath: 'preferred', program: 'codex' }) expect(result.environmentResolved).toBe(true) }) }) -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 { - environmentResolved: true, - 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 @@ -159,7 +119,7 @@ describe('scripts', () => { // (#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'] }) + await runWslProcess({ loginPath: 'preferred', script, args: ['/tmp/root'] }) expect(runProcessMock.mock.calls.at(-1)?.[0].input).toBe(script) expect(lastArgv()).toEqual([ '--exec', @@ -180,7 +140,7 @@ describe('WSLENV', () => { // Unset, a Windows-side variable silently never reaches the guest (#12557). seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', program: '/bin/true', env: { GITLAB_HOST: 'git.example.com', GH_TOKEN: 't' } }) @@ -189,17 +149,27 @@ describe('WSLENV', () => { expect(env.WSLENV?.split(':')).toEqual(expect.arrayContaining(['GITLAB_HOST', 'GH_TOKEN'])) }) - it('leaves the host environment alone when nothing is propagated', async () => { + it('always sets WSL_UTF8, even with nothing to propagate', async () => { + // Without it wsl.exe writes its own error text as UTF-16LE, so anything + // surfacing stderr shows NUL-riddled output (#9010). seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await runWslProcess({ lane: 'probe', program: '/bin/true' }) - expect(runProcessMock.mock.calls.at(-1)?.[0].env).toBeUndefined() + await runWslProcess({ loginPath: 'preferred', program: '/bin/true' }) + const env = runProcessMock.mock.calls.at(-1)?.[0].env as NodeJS.ProcessEnv + expect(env.WSL_UTF8).toBe('1') + }) + + it('adds no WSLENV entry when nothing is propagated', async () => { + seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) + await runWslProcess({ loginPath: 'preferred', program: '/bin/true' }) + const env = runProcessMock.mock.calls.at(-1)?.[0].env as NodeJS.ProcessEnv + expect(env.WSLENV).toBe(process.env.WSLENV) }) }) 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' }) + await runWslProcess({ loginPath: 'preferred', 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') @@ -209,7 +179,7 @@ describe('guest 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( + await expect(runWslProcess({ loginPath: 'preferred', program: '/bin/true', cwd })).rejects.toThrow( /guest path/ ) }) @@ -219,7 +189,7 @@ 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/) + await expect(runWslProcess({ loginPath: 'preferred', program })).rejects.toThrow(/single binary/) } ) @@ -228,55 +198,17 @@ describe('program is a binary, not a shell string', () => { // rejecting it would fail legitimate installs under a spaced directory. seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) await expect( - runWslProcess({ lane: 'probe', program: '/home/u/my tools/codex' }) + runWslProcess({ loginPath: 'preferred', program: '/home/u/my tools/codex' }) ).resolves.toBeDefined() }) }) -describe('a script gets the cached environment on both lanes', () => { - it('applies the cached PATH even when the caller asked for interactive', async () => { - // A script never runs under the login shell (it owns stdin), so without - // this the interactive lane would give a script LESS PATH than the probe - // lane -- for a caller that explicitly asked for the user's terminal PATH. - seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await runWslProcess({ lane: 'interactive', script: 'command -v codex' }) - expect(lastArgv()).toEqual([ - '--exec', - '/usr/bin/env', - 'PATH=/home/u/.nvm/bin:/usr/bin', - 'HOME=/home/u', - 'sh', - '-s', - '--' - ]) - }) -}) - -describe('a missing fence is a failure, not empty output', () => { - it('throws rather than returning a clean empty result', async () => { - // readStdout returns null precisely to distinguish "no fence" from "empty - // payload". An rc that redirects stdout would otherwise yield a silent - // wrong answer. - runProcessMock.mockResolvedValue({ - environmentResolved: true, - code: 0, - signal: null, - stdout: 'banner only, no fence', - stderr: '', - timedOut: false - }) - await expect(runWslProcess({ lane: 'interactive', program: 'claude' })).rejects.toThrow( - /no fenced output/ - ) - }) -}) - describe('program is not an assignment', () => { it('rejects a name=value program that env would swallow', async () => { // `env PATH=… HOME=… FOO=bar` has no command left: it prints the whole // guest environment and exits 0 -- success, with the environment as stdout. seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await expect(runWslProcess({ lane: 'probe', program: 'FOO=bar' })).rejects.toThrow( + await expect(runWslProcess({ loginPath: 'preferred', program: 'FOO=bar' })).rejects.toThrow( /assignment/ ) }) @@ -288,7 +220,7 @@ describe('timeout budget', () => { // 5s caller could reach runProcess with 1ms left and report a timeout for a // command that would have taken milliseconds. seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await runWslProcess({ lane: 'probe', program: '/bin/true', timeoutMs: 5_000 }) + await runWslProcess({ loginPath: 'preferred', program: '/bin/true', timeoutMs: 5_000 }) const passed = runProcessMock.mock.calls.at(-1)?.[0].timeoutMs as number expect(passed).toBeGreaterThan(1_000) expect(passed).toBeLessThanOrEqual(5_000) @@ -308,10 +240,9 @@ describe('the probe never starves the command', () => { // and passes for any split. allowDegradedEnvironment keeps the call alive // once the probe fails. await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', program: '/bin/true', timeoutMs, - allowDegradedEnvironment: true }) const probeMs = runProcessMock.mock.calls[0]?.[0].timeoutMs as number expect(probeMs).toBeLessThanOrEqual(timeoutMs - floor) @@ -321,7 +252,7 @@ describe('the probe never starves the command', () => { describe('script interpreter', () => { it('defaults to sh', async () => { seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await runWslProcess({ lane: 'probe', script: 'echo hi' }) + await runWslProcess({ loginPath: 'preferred', script: 'echo hi' }) expect(lastArgv()).toContain('sh') expect(lastArgv()).not.toContain('bash') }) @@ -332,7 +263,7 @@ describe('script interpreter', () => { // unexpected` -- the #14292 signature -- so a bash caller must be able to // say so rather than be silently downgraded. seedWslGuestEnvironmentForTests(undefined, ENVIRONMENT) - await runWslProcess({ lane: 'probe', script: 'done < <(find .)', shell: 'bash' }) + await runWslProcess({ loginPath: 'preferred', script: 'done < <(find .)', shell: 'bash' }) expect(lastArgv()).toContain('bash') expect(lastArgv()).not.toContain('sh') }) @@ -351,7 +282,7 @@ describe('a script never rides the interactive lane', () => { stderr: 'distro is stopped', timedOut: false }) - await runWslProcess({ lane: 'probe', script: 'echo hi', allowDegradedEnvironment: true }) + await runWslProcess({ loginPath: 'preferred', 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 index cebc88b3831..b2eb890afc1 100644 --- a/src/main/wsl/wsl-runner.ts +++ b/src/main/wsl/wsl-runner.ts @@ -1,10 +1,6 @@ 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 { buildWslExecArgs } from '../../shared/wsl-login-shell-command' import { getWslGuestEnvironment, type WslGuestEnvironment } from './wsl-guest-environment' import { resolveWslExecutablePath } from './wsl-executable-path' @@ -17,11 +13,20 @@ import { resolveWslExecutablePath } from './wsl-executable-path' * docs/reference/wsl-command-execution.md. */ -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' +/** + * How much the call needs the user's login PATH. + * + * Why one axis and not `lane` + `allowDegradedEnvironment`: 19 of 23 sites + * passed the opt-out, and two of them said in comments that they did not want + * the login PATH at all -- the flag had become the `'none'` this union was + * missing. A required discriminator whose default nobody wants is not a + * discriminator. + */ +export type WslLoginPath = + /** The command reads $HOME or absolute paths only. No probe. */ + | 'none' + /** Use the cached login PATH when there is one; run anyway when there is not. */ + | 'preferred' /** * What to run: a single binary, or a script. @@ -53,27 +58,14 @@ export type WslCommand = 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 + /** Required, with no default: the wrong answer here is the defect this file exists to prevent. */ + loginPath: WslLoginPath /** Guest (POSIX) path. */ cwd?: string /** Host variables to propagate into the guest; sets WSLENV automatically. */ env?: Readonly> timeoutMs?: number maxOutputBytes?: number - /** - * Proceed when the login PATH could not be established. - * - * Default is to throw. Three separate reviews found the same class of bug -- - * a call that answers "is this installed?" running on the bare default PATH - * and reporting an nvm-installed tool absent (#9725). Making degradation - * opt-in puts that decision in the one place a reader will look, instead of - * relying on every call site to remember to check `environmentResolved`. - */ - allowDegradedEnvironment?: boolean } export type WslResult = { @@ -93,21 +85,6 @@ export type WslResult = { export const DEFAULT_WSL_TIMEOUT_MS = 30_000 -/** - * The guest login PATH could not be established. - * - * Typed so a caller answering "is this installed?" can report unverifiable - * rather than absent -- reporting an nvm-installed tool absent is #9725, and a - * bare Error would just be swallowed by the same catch that handles real - * failures. - */ -export class WslGuestEnvironmentUnavailableError extends Error { - constructor(distro: string | undefined) { - super(`WSL guest environment for ${distro ?? 'the default distro'} is unavailable`) - this.name = 'WslGuestEnvironmentUnavailableError' - } -} - 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 @@ -131,13 +108,20 @@ function assertNotShellString(program: string): void { } } -/** 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 +/** + * Host env plus WSL_UTF8 and the WSLENV entries that let values cross. + * + * Why WSL_UTF8 unconditionally: without it `wsl.exe` writes its OWN messages + * ("There is no distribution with the supplied name") as UTF-16LE, so every + * caller that surfaces stderr shows NUL-riddled text. Setting it per-call site + * is how it got lost -- the relay set it, the migration dropped it, and nothing + * noticed because the happy path is pure ASCII. Credit: #9010. + */ +function buildHostEnv(env: WslSpec['env']): NodeJS.ProcessEnv { + const merged: NodeJS.ProcessEnv = { ...process.env, ...env, WSL_UTF8: '1' } + if (env && Object.keys(env).length > 0) { + addWslEnvKeys(merged, Object.keys(env)) } - const merged: NodeJS.ProcessEnv = { ...process.env, ...env } - addWslEnvKeys(merged, Object.keys(env)) return merged } @@ -177,38 +161,13 @@ function buildGuestArgv(environment: WslGuestEnvironment | null, spec: WslSpec): 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], - // Why throw rather than default to '': readStdout returns null precisely to - // distinguish "the fence never appeared" from "the payload was empty". An rc - // that redirects stdout, or output truncated before the begin marker, would - // otherwise return a clean, empty, wrong answer. - readStdout: (stdout: string) => { - const payload = captured.readStdout(stdout) - if (payload === null) { - throw new Error('WSL login shell produced no fenced output') - } - return payload - } - } -} - /** * Run a program inside WSL. * - * Throws when the guest login PATH cannot be established, unless the caller - * passes `allowDegradedEnvironment`. Falling back to the login shell here would - * re-run ~/.profile -- the stall this exists to remove. + * A missing login PATH is never fatal: the call runs on the distro's default + * PATH and reports `environmentResolved: false`. Every knob this file used to + * carry -- cooldown tiers, budget splitting, a re-probe heuristic, an opt-out + * flag on 19 of 23 sites -- existed only because that case used to throw. */ export async function runWslProcess(spec: WslSpec): Promise { if (spec.program !== undefined) { @@ -223,7 +182,7 @@ export async function runWslProcess(spec: WslSpec): Promise { // shell (see below), so on the interactive lane it would otherwise get no // login PATH at all -- strictly less than the probe lane, for a caller that // explicitly asked for the user's terminal PATH. - const wantsEnvironment = spec.lane === 'probe' || spec.script !== undefined + const wantsEnvironment = spec.loginPath === 'preferred' // Leave the command at least a third of the budget: a probe that eats it all // turns a healthy command into a spurious timeout. // Cap the probe at half the budget and at 4s: a 5s caller was giving the @@ -240,21 +199,14 @@ export async function runWslProcess(spec: WslSpec): Promise { // the probe most often fails *because* the distro is slow, so the fallback // would hit the hazard exactly when it is worst. Run shell-free with the // distro's default PATH instead: degraded, never blocking. - if (wantsEnvironment && environment === null && !spec.allowDegradedEnvironment) { - throw new WslGuestEnvironmentUnavailableError(spec.distro) - } - - const lane = - spec.lane === 'interactive' && spec.script === undefined - ? ({ kind: 'interactive', ...buildInteractiveArgv(spec) } as const) - : ({ kind: 'probe', argv: buildGuestArgv(environment, spec) } as const) + const argv = buildGuestArgv(environment, spec) // One budget for the whole call: the probe used to run on its own 10s timer // ahead of the timed leg, so a 5s caller could wait 15s. const remainingMs = Math.max(1, deadline - Date.now()) const result = await runProcess({ program: resolveWslExecutablePath(), - args: buildWslExecArgs(spec.distro, lane.argv), + args: buildWslExecArgs(spec.distro, argv), env: buildHostEnv(spec.env), input: spec.script, timeoutMs: remainingMs, @@ -264,7 +216,7 @@ export async function runWslProcess(spec: WslSpec): Promise { return { environmentResolved: !wantsEnvironment || environment !== null, code: result.code, - stdout: lane.kind === 'interactive' ? lane.readStdout(result.stdout) : result.stdout, + 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 index cd75de1ac02..ce034999c89 100644 --- a/src/main/wsl/wsl-runner.wsl.test.ts +++ b/src/main/wsl/wsl-runner.wsl.test.ts @@ -50,9 +50,8 @@ describeOnWsl('runWslProcess against a real distro', () => { // otherwise refuses an unresolved PATH. const started = Date.now() const result = await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: DISTRO, - allowDegradedEnvironment: true, program: '/bin/echo', args: ['orca-probe-ok'], timeoutMs: 15_000 @@ -65,11 +64,10 @@ describeOnWsl('runWslProcess against a real distro', () => { it('second probe-lane call does not pay the login shell again', async () => { const started = Date.now() await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: DISTRO, program: '/bin/true', timeoutMs: 15_000, - allowDegradedEnvironment: true }) expect(Date.now() - started).toBeLessThan(5_000) }, 30_000) @@ -78,7 +76,7 @@ describeOnWsl('runWslProcess against a real distro', () => { // 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', + loginPath: 'preferred', distro: DISTRO, program: '/bin/echo', args: ['ORCA_PAYLOAD'], @@ -96,9 +94,8 @@ describeOnWsl('runWslProcess against a real distro', () => { `echo "x" | awk '{print $1}'` ].join('\n') const result = await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: DISTRO, - allowDegradedEnvironment: true, script, args: ['ORCA_ARG'], timeoutMs: 30_000 @@ -113,9 +110,8 @@ describeOnWsl('runWslProcess against a real distro', () => { it('propagated env crosses the boundary via WSLENV', async () => { const result = await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: DISTRO, - allowDegradedEnvironment: true, script: 'printf %s "$ORCA_WSLENV_PROBE"', env: { ORCA_WSLENV_PROBE: 'crossed' }, timeoutMs: 30_000 @@ -125,9 +121,8 @@ describeOnWsl('runWslProcess against a real distro', () => { it('runs in the requested guest cwd', async () => { const result = await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: DISTRO, - allowDegradedEnvironment: true, program: '/bin/pwd', cwd: '/tmp', timeoutMs: 30_000 diff --git a/src/main/wsl/wsl-w1-w3-contract.test.ts b/src/main/wsl/wsl-w1-w3-contract.test.ts index df297d9177a..8dbad2c047a 100644 --- a/src/main/wsl/wsl-w1-w3-contract.test.ts +++ b/src/main/wsl/wsl-w1-w3-contract.test.ts @@ -44,27 +44,27 @@ describe('W1: every WSL call inherits the spawn chokepoint', () => { it('resolves wsl.exe absolutely, never by bare name', async () => { // Bare-name resolution is what a Group Policy or a stripped Electron PATH // hijacks (#15749). - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/true' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/true' }) expect(spawnSpec().program).toBe('C:\\Windows\\System32\\wsl.exe') }) it('passes argv as an array, never a command string', async () => { // A command string would put quoting back in the caller's hands, which is // the entire class W1 removed. - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/echo', args: ['a b'] }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/echo', args: ['a b'] }) expect(Array.isArray(spawnSpec().args)).toBe(true) expect(spawnSpec().args).toContain('a b') }) it('always bounds the call', async () => { - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/true' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/true' }) expect(spawnSpec().timeoutMs).toBeGreaterThan(0) }) }) describe('W3: the five per-call decisions are made once', () => { it('never uses the -- separator, which expands $name host-side', async () => { - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', script: 'echo "$HOME"' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', script: 'echo "$HOME"' }) const argv = spawnSpec().args as string[] // Only wsl.exe's own separator matters. A later `--` belongs to `sh -s --` // and is the guest shell's end-of-options, so check the position rather @@ -77,14 +77,14 @@ describe('W3: the five per-call decisions are made once', () => { // The base64 and eval wrappers existed because argv quoting kept breaking; // stdin has no quoting boundary at all (#14292). const script = `case "$x" in a) echo 'it'\\''s fine';; esac` - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', script }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', script }) expect(spawnSpec().input).toBe(script) expect(JSON.stringify(spawnSpec().args)).not.toContain('case') }) it('propagates env only through WSLENV', async () => { await runWslProcess({ - lane: 'probe', + loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/true', env: { GITLAB_HOST: 'git.example.com' } @@ -95,14 +95,14 @@ describe('W3: the five per-call decisions are made once', () => { it('runs no shell on the probe lane, so ~/.profile cannot stall it', async () => { // #14288: one blocking line in ~/.profile ate the whole timeout. - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: 'codex' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: 'codex' }) expect(JSON.stringify(spawnSpec().args)).not.toContain('_orca_wsl_shell') }) it('applies the user login PATH even with no shell in the loop', async () => { // The other half of the same trade: nvm-installed agents must still be // found (#9725, #7563, #8366). - await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: 'codex' }) + await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: 'codex' }) expect(spawnSpec().args).toContain('PATH=/home/u/.nvm/bin:/usr/bin') }) }) @@ -118,7 +118,7 @@ describe('failure modes stay distinguishable', () => { stderr: '', timedOut: false }) - const result = await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/false' }) + const result = await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/false' }) expect(result.code).toBe(3) expect(result.timedOut).toBe(false) }) @@ -131,11 +131,11 @@ describe('failure modes stay distinguishable', () => { stderr: '', timedOut: true }) - const result = await runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: '/bin/true' }) + const result = await runWslProcess({ loginPath: 'preferred', distro: 'Ubuntu', program: '/bin/true' }) expect(result.timedOut).toBe(true) }) - it('an unresolved login PATH refuses rather than answering wrongly', async () => { + it('an unresolved login PATH is reported, never fatal', async () => { invalidateWslGuestEnvironment(undefined, true) runProcessMock.mockResolvedValue({ code: 1, @@ -144,8 +144,12 @@ describe('failure modes stay distinguishable', () => { stderr: 'stopped', timedOut: false }) - await expect( - runWslProcess({ lane: 'probe', distro: 'Ubuntu', program: 'codex' }) - ).rejects.toThrow(/guest environment/) + // Every knob this runner used to carry existed because this case threw. + const result = await runWslProcess({ + loginPath: 'preferred', + distro: 'Ubuntu', + program: 'codex' + }) + expect(result.environmentResolved).toBe(false) }) }) diff --git a/src/renderer/src/components/settings/CliSkillRuntimeSetup.test.tsx b/src/renderer/src/components/settings/CliSkillRuntimeSetup.test.tsx index dee03d0c6dd..777006cafad 100644 --- a/src/renderer/src/components/settings/CliSkillRuntimeSetup.test.tsx +++ b/src/renderer/src/components/settings/CliSkillRuntimeSetup.test.tsx @@ -18,7 +18,9 @@ import { function decodeWslLoginShellScript(command: string): string { const encoded = - /(?:--|--exec) sh -c 'eval \\"`printf %s ([A-Za-z0-9+/=]+) \| base64 -d`\\"'/.exec(command)?.[1] + /(?:--|--exec) sh -c '(?:eval \\"`|sh -c \\"\$\()?printf %s ([A-Za-z0-9+/=]+) \| base64 -d/.exec( + command + )?.[1] expect(encoded).toBeDefined() return Buffer.from(encoded!, 'base64').toString('utf8') } @@ -48,7 +50,7 @@ describe('CliSkillRuntimeSetup runtime helpers', () => { expect(command).toBe(skillCommand) expect(setupCommand).toBe( - `& { $PSNativeCommandArgumentPassing = 'Legacy'; wsl.exe -d 'Ubuntu' --exec sh -c 'eval \\"\`printf %s ${encoded} | base64 -d\`\\"' } # Runs: ${skillCommand}` + `& { $PSNativeCommandArgumentPassing = 'Legacy'; wsl.exe -d 'Ubuntu' --exec sh -c 'sh -c \\"$(printf %s ${encoded} | base64 -d)\\"' } # Runs: ${skillCommand}` ) expect(decodeWslLoginShellScript(setupCommand)).toContain( 'exec "$_orca_wsl_shell" -ilc \'npx skills add orchestration --global\'' @@ -93,9 +95,11 @@ describe('CliSkillRuntimeSetup runtime helpers', () => { const setupCommand = buildSkillSetupTerminalCommand(command, 'powershell.exe', runtime, 'win32') expect(setupCommand).toMatch( - /^& \{ \$PSNativeCommandArgumentPassing = 'Legacy'; wsl\.exe --exec sh -c 'eval \\"`printf/ + /^& \{ \$PSNativeCommandArgumentPassing = 'Legacy'; wsl\.exe --exec sh -c 'sh -c/ ) - expect(setupCommand).toContain('`\\"\' } # Runs: npx skills update orchestration --global') + // The command no longer ends in the eval wrapper's nested quotes; it is a + // plain pipe into sh, so the payload has no quoting layer to escape from. + expect(setupCommand).toContain('base64 -d)') }) it.skipIf(process.platform === 'win32')( diff --git a/src/renderer/src/components/settings/CliSkillRuntimeSetup.tsx b/src/renderer/src/components/settings/CliSkillRuntimeSetup.tsx index 3208aa2cd04..9e454fcee7f 100644 --- a/src/renderer/src/components/settings/CliSkillRuntimeSetup.tsx +++ b/src/renderer/src/components/settings/CliSkillRuntimeSetup.tsx @@ -164,7 +164,15 @@ function buildPowerShellWslSkillCommand(command: string, runtime: LocalAgentRunt // Why: encoding preserves the user's configured login-shell PATH across the Windows argv boundary. const encodedScript = encodeWslLoginShellScript(command) const visibleCommand = command.replace(/[\r\n]+/g, ' ') - const shellScript = `eval "\`printf %s ${encodedScript} | base64 -d\`"` + // Why $(...) and not a backtick eval: PowerShell treats ` as its own escape + // character, so the old `eval "\`printf ...\`"` had the payload's quotes + // interacting with two escaping layers and dash saw `case in` -- the + // `word unexpected (expecting "in")` in #14292. Credit: #14785. + // + // Why not a plain pipe into sh: that hands the payload the pipe as its stdin, + // so a setup command that reads input gets base64 remnants instead. Command + // substitution runs in a subshell and leaves the terminal's stdin intact. + const shellScript = `sh -c "$(printf %s ${encodedScript} | base64 -d)"` // Why --exec: `--` makes wsl.exe expand $name in the argv it forwards to the guest. const wslCommand = `wsl.exe${distroArg} --exec sh -c ${quotePowerShellNativeArgument(shellScript)}` return `& { $PSNativeCommandArgumentPassing = 'Legacy'; ${wslCommand} } # Runs: ${visibleCommand}` @@ -180,7 +188,10 @@ function decodeWslSetupTerminalCommand(command: string): string | null { // Why both separators: commands persisted before the --exec switch must still decode. const encoded = - /(?:--|--exec) sh -c 'eval \\"`printf %s ([A-Za-z0-9+/=]+) \| base64 -d`\\"'/.exec(command)?.[1] + // Both shapes: commands persisted before the pipe switch still decode. + /(?:--|--exec) sh -c '(?:eval \\"`|sh -c \\"\$\()?printf %s ([A-Za-z0-9+/=]+) \| base64 -d/.exec( + command + )?.[1] if (!encoded) { return null }