From 6efd4061dc4cfee7d59db3ba85785818e7ccf733 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Tue, 18 Aug 2026 02:54:19 -0700 Subject: [PATCH] fix(git): run WSL git reads without a shell (#15257) * fix(git): run WSL git reads without a shell WSL-routed git ran through the distro user's interactive login shell for one reason: to inherit their PATH. That shell also runs the distro's rc/motd and writes it to the stdout callers parse, which is why #10917 reports a shell banner breaking GitHub source detection. A shell-free route already existed (`--exec /usr/bin/env PATH=... git`, added for status reads in #13207) but it was opt-in, and only gitStatusReadOptionsForWorktree opted in. Every other read -- remote get-url, config --get, log, show, rev-parse -- took the login shell on every call, so the reported parse never benefited. Classify reads at the resolver instead of at each caller. A read needs nothing the login shell provides, so it takes the direct route without the caller asking. Writes and network operations stay on the login shell: they can depend on credential helpers and ssh-agent that the user's profile sets up. Subcommands that both read and write (config, remote, branch, submodule) require an explicit read flag before they qualify, so `config --get` goes direct while `config user.email x` does not. `git show` blob reads stop forcing the login shell and go direct too. The fence stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. Behavior note: the first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra wsl.exe. Subsequent reads start no shell at all. * fix(git): keep queried WSL remote reads on login shell * style: format WSL runner test * fix(git): match WSL read markers positionally The conditional read markers were matched anywhere after the subcommand, so a positional argument that happened to share a marker's name routed a write shell-free: `worktree remove list` read as a listing, and `submodule foreach status` as a status query -- the latter can run arbitrary commands, including network ones. Split the two kinds of marker apart. `config`/`branch` are flag-marked and still match anywhere; `remote`/`worktree`/`submodule` are action-marked and must match the first non-flag argument. The queried `remote show` rule and the `symbolic-ref` arity rule are unchanged. Neither case is reachable today -- Orca issues no `submodule foreach`, and the worktree paths are its own CLI, not git argv -- but the loose match was the shape of the defect, not those two instances. * test(git): pin read routing behind global options Writes hidden behind -c/-C/--git-dir must not reach the shell-free route, and an unparsed global form must fall back to the login shell rather than guess at the subcommand. Both directions fail safe. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> --- src/main/git/runner-command-exec.test.ts | 10 +- .../runner-wsl-linked-gitdir-timeout.test.ts | 7 +- src/main/git/runner-wsl-read-routing.test.ts | 101 +++++++++++++++++ src/main/git/runner.ts | 19 ++-- .../git/wsl-direct-git-read-commands.test.ts | 91 +++++++++++++++ src/main/git/wsl-direct-git-read-commands.ts | 105 ++++++++++++++++++ 6 files changed, 323 insertions(+), 10 deletions(-) create mode 100644 src/main/git/runner-wsl-read-routing.test.ts create mode 100644 src/main/git/wsl-direct-git-read-commands.test.ts create mode 100644 src/main/git/wsl-direct-git-read-commands.ts diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index 29a3d903cc7..e29c9f650cc 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -296,7 +296,10 @@ describe('runner execFile timeout handling', () => { return child }) - await gitExecFileAsync(['worktree', 'list', '--porcelain', '-z'], { cwd: '/home5/Brian' }) + await gitExecFileAsync(['worktree', 'list', '--porcelain', '-z'], { + cwd: '/home5/Brian', + env: { ...process.env, GIT_ASKPASS: undefined, SSH_ASKPASS: undefined } + }) expect(capturedEnv?.GIT_TERMINAL_PROMPT).toBe('0') expect(capturedEnv?.GIT_ASKPASS).toBe('') @@ -507,7 +510,10 @@ describe('runner execFile timeout handling', () => { expect.objectContaining({ cwd: undefined }), expect.any(Function) ) - const shellCommand = execFileMock.mock.calls[0]?.[1]?.[5] as string + // A read also warms the direct-git environment probe in the background, so + // pick the git call rather than assuming it is the first spawn. + const gitCall = execFileMock.mock.calls.find((call) => String(call[1]?.[5]).includes("'git'")) + const shellCommand = gitCall?.[1]?.[5] as string expect(shellCommand).toContain('getent passwd') expect(shellCommand).toContain('exec "$_orca_wsl_shell" -ilc') expect(shellCommand).toContain('/mnt/c/repo') diff --git a/src/main/git/runner-wsl-linked-gitdir-timeout.test.ts b/src/main/git/runner-wsl-linked-gitdir-timeout.test.ts index efc37c192f7..553b07d6b38 100644 --- a/src/main/git/runner-wsl-linked-gitdir-timeout.test.ts +++ b/src/main/git/runner-wsl-linked-gitdir-timeout.test.ts @@ -92,7 +92,12 @@ describe('WSL linked-worktree routing probe timeout', () => { ).resolves.toEqual({ stdout: 'host git recovered\n', stderr: '' }) expect(statMock).toHaveBeenCalledTimes(2) - expect(execFileMock.mock.calls.map(([command]) => command)).toEqual(['wsl.exe', 'git']) + // A WSL read also warms the direct-git environment probe in the background; + // it is not part of the routing sequence under test. + const routedCommands = execFileMock.mock.calls + .filter(([, args]) => !String((args as string[])?.at(-1)).includes('^GIT_')) + .map(([command]) => command) + expect(routedCommands).toEqual(['wsl.exe', 'git']) }) }) }) diff --git a/src/main/git/runner-wsl-read-routing.test.ts b/src/main/git/runner-wsl-read-routing.test.ts new file mode 100644 index 00000000000..21c9bc45e38 --- /dev/null +++ b/src/main/git/runner-wsl-read-routing.test.ts @@ -0,0 +1,101 @@ +import { EventEmitter } from 'node:events' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { execFileMock, execFileSyncMock, spawnMock } = vi.hoisted(() => ({ + execFileMock: vi.fn(), + execFileSyncMock: vi.fn(), + spawnMock: vi.fn() +})) + +vi.mock('node:child_process', () => ({ + execFile: execFileMock, + execFileSync: execFileSyncMock, + spawn: spawnMock +})) +vi.mock('../observability/instrumentation', () => ({ + withGitSpan: (_attributes: unknown, run: () => unknown) => run() +})) +vi.mock('../diagnostics/main-thread-churn-probe', () => ({ recordSubprocessSpawn: vi.fn() })) + +import { gitExecFileAsync } from './runner' +import { + resetWslGitReadEnvironmentForTests, + seedWslGitReadEnvironmentForTests +} from './wsl-git-read-environment' + +const DISTRO = 'Ubuntu' +const WSL_CWD = String.raw`\wsl.localhost\Ubuntu\home\alice\repo` +const LOGIN_ENVIRONMENT = { + gitPath: '/usr/bin/git', + home: '/home/alice', + path: '/home/alice/bin:/usr/bin' +} + +function createMockChild(): EventEmitter & { stdout: EventEmitter; stderr: EventEmitter } { + const child = new EventEmitter() as EventEmitter & { + stdout: EventEmitter + stderr: EventEmitter + } + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + return child +} + +function lastArgs(): string[] { + return execFileMock.mock.calls.at(-1)?.[1] as string[] +} + +describe('WSL git read routing', () => { + beforeEach(() => { + execFileMock.mockReset() + execFileMock.mockImplementation((_command, _args, _options, callback) => { + queueMicrotask(() => callback?.(null, 'ok', '')) + return createMockChild() + }) + resetWslGitReadEnvironmentForTests() + seedWslGitReadEnvironmentForTests(DISTRO, LOGIN_ENVIRONMENT) + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: process.platform }) + resetWslGitReadEnvironmentForTests() + }) + + // Why these two: they are the parse #10917 reports, and they run through the + // general git options that never opted into the shell-free route. + it.each([ + ['remote get-url', ['remote', 'get-url', 'origin']], + ['remote list', ['remote']], + ['remote show without query', ['remote', 'show', '-n', 'origin']], + ['symbolic-ref', ['symbolic-ref', '--quiet', '--short', 'HEAD']], + ['worktree list', ['worktree', 'list', '--porcelain']], + ['config --get', ['config', '--get', 'remote.origin.url']], + ['log', ['log', '--oneline', '-n', '1']], + ['show blob', ['show', ':src/file.ts']] + ])('runs %s with no shell at all', async (_name, args) => { + await gitExecFileAsync(args, { cwd: WSL_CWD, wslDistro: DISTRO }) + + const resolved = lastArgs() + expect(resolved).toContain('--exec') + expect(resolved).toContain('/usr/bin/env') + expect(resolved).toContain(`PATH=${LOGIN_ENVIRONMENT.path}`) + // No shell means no rc file, so no banner can reach the parsed stdout. + expect(resolved).not.toContain('-lc') + expect(resolved).not.toContain('sh') + expect(resolved.join(' ')).not.toContain('getent passwd') + }) + + it.each([ + ['commit', ['commit', '-m', 'msg']], + ['config set', ['config', 'user.email', 'me@example.com']], + ['remote add', ['remote', 'add', 'upstream', 'https://example.com/r.git']], + ['remote show with query', ['remote', 'show', 'origin']] + ])('keeps %s on the login shell', async (_name, args) => { + await gitExecFileAsync(args, { cwd: WSL_CWD, wslDistro: DISTRO }) + + const resolved = lastArgs() + expect(resolved).toContain('-lc') + expect(resolved.join(' ')).toContain('getent passwd') + }) +}) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index a471d646e15..0f78961815c 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -34,6 +34,7 @@ import { gitCredentialPromptGuardEnv } from '../../shared/git-credential-prompt-env' import { getSpawnArgsForWindows, isWindowsBatchScript, resolveWindowsCommand } from '../win32-utils' +import { isWslDirectGitReadCommand } from './wsl-direct-git-read-commands' import { buildWslCapturedLoginShellCommand, buildWslExecArgs, @@ -446,7 +447,7 @@ function resolveGitCommand( // Why: WSL Git resolves a Windows-authored linked-worktree pointer relative to cwd. return { binary: 'git', args, cwd: options.cwd, wsl: null, wslMode: null } } - if (!forceLoginShell && shouldAttemptWslDirectGit(options)) { + if (!forceLoginShell && shouldAttemptWslDirectGit(args, options)) { const distro = wslDistroForCommand(options.cwd, options.wslDistro) const environment = distro ? peekWslGitReadEnvironment(distro) : undefined if (environment) { @@ -462,10 +463,13 @@ function resolveGitCommand( return resolveGitCommandWithoutProbe(args, options, captureLoginShellOutput) } -function shouldAttemptWslDirectGit(options: GitExecOptions): boolean { +function shouldAttemptWslDirectGit(args: string[], options: GitExecOptions): boolean { return Boolean( process.platform === 'win32' && - options.preferWslDirectGit && + // Why either: callers can still opt in explicitly, but a plain read no + // longer has to -- it needs nothing the login shell provides, and routing + // it through one is what exposes callers to the shell's rc output. + (options.preferWslDirectGit || isWslDirectGitReadCommand(args)) && !options.useConfiguredSshCommandForNetwork && !Object.entries(options.env ?? {}).some( ([key, value]) => @@ -1184,14 +1188,15 @@ export async function gitExecFileAsyncBuffer( if (isWslLinkedWorktreeGitRoutingCandidate(options.cwd, options.wslDistro)) { await prepareWslLinkedWorktreeGitRouting(options.cwd, options.wslDistro) } - // Why fenced: this returns raw blob bytes straight to the diff/blob viewer, so - // a login-shell banner would be prepended to displayed file content. - let resolved = resolveGitCommand(args, options, true, true) + // `git show` is a read, so this normally runs with no shell at all. The fence + // still matters for the login-shell fallback: these are raw blob bytes going + // straight to the diff/blob viewer, where a banner becomes file content. + let resolved = resolveGitCommand(args, options, false, true) const environmentReady = prepareWindowsHostGitEnvironment(resolved, undefined) if (environmentReady) { await environmentReady } - resolved = resolveGitCommand(args, options, true, true) + resolved = resolveGitCommand(args, options, false, true) const { stdout } = (await execFileCapture(resolved.binary, resolved.args, { cwd: resolved.cwd, encoding: 'buffer', diff --git a/src/main/git/wsl-direct-git-read-commands.test.ts b/src/main/git/wsl-direct-git-read-commands.test.ts new file mode 100644 index 00000000000..ed8c2458f69 --- /dev/null +++ b/src/main/git/wsl-direct-git-read-commands.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from 'vitest' +import { isWslDirectGitReadCommand } from './wsl-direct-git-read-commands' + +describe('isWslDirectGitReadCommand', () => { + it.each([ + [['status', '--porcelain=v2']], + [['log', '--oneline', '-n', '10']], + [['show', ':src/file.ts']], + [['show', '--end-of-options', 'HEAD:src/file.ts']], + [['rev-parse', '--show-toplevel']], + [['ls-files', '-z', '--', 'src']], + [['check-ignore', '-z', '--stdin']], + [['config', '--get', 'core.sshCommand']], + [['config', '--get-regexp', String.raw`^remote\.`]], + [['remote', 'get-url', 'origin']], + [['remote']], + [['remote', '-v']], + [['remote', 'show', '-n', 'origin']], + [['symbolic-ref', '--quiet', '--short', 'HEAD']], + [['worktree', 'list', '--porcelain']], + [['branch', '--list']], + [['branch', '--show-current']], + // Global options precede the subcommand. + [['-c', 'core.quotepath=off', 'log', '--oneline']], + [['-C', '/repo', 'rev-parse', 'HEAD']], + [['--git-dir=/repo/.git', 'ls-tree', 'HEAD']] + ])('routes %j without a shell', (args) => { + expect(isWslDirectGitReadCommand(args)).toBe(true) + }) + + // Why these matter most: a write misrouted to the shell-free path loses the + // credential helpers and ssh-agent the user's login shell sets up. + it.each([ + [['commit', '-m', 'msg']], + [['push', 'origin', 'main']], + [['fetch', '--all']], + [['pull', '--rebase']], + [['clone', 'https://example.com/r.git']], + [['checkout', '-b', 'feature']], + [['add', '--', 'file.ts']], + [['worktree', 'add', '/tmp/wt']], + [['submodule', 'update', '--init']], + // Same subcommands as above, in their writing form. + [['config', 'user.email', 'me@example.com']], + [['config', '--unset', 'core.sshCommand']], + [['remote', 'add', 'origin', 'https://example.com/r.git']], + [['remote', 'remove', 'origin']], + [['remote', 'show', 'origin']], + [['symbolic-ref', 'HEAD', 'refs/heads/main']], + [['worktree', 'add', '/tmp/wt']], + [['branch', '-D', 'feature']], + [['branch', 'newbranch']], + [[]] + ])('keeps %j on the login shell', (args) => { + expect(isWslDirectGitReadCommand(args)).toBe(false) + }) + + // Why positional: these read markers are matched as the action, not anywhere in + // argv. Matching loosely routed a write shell-free whenever a positional + // happened to share a read marker's name. + it.each([[['worktree', 'remove', 'list']], [['submodule', 'foreach', 'status']]])( + 'keeps %j on the login shell despite a read-shaped positional', + (args) => { + expect(isWslDirectGitReadCommand(args)).toBe(false) + } + ) + + it.each([[['submodule', 'status']], [['submodule']], [['remote']]])( + 'routes the listing form %j without a shell', + (args) => { + expect(isWslDirectGitReadCommand(args)).toBe(true) + } + ) + + // Why: a write hidden behind global options must never reach the shell-free + // route, and an unparsed global form must fall back to the login shell rather + // than guess. Both directions fail safe. + it.each([ + [['-c', 'k=v', 'push', 'origin', 'main']], + [['-C', '/repo', 'commit', '-m', 'x']], + [['-c', 'k=v', 'fetch', '--all']], + [['--git-dir=/x', 'push']], + // Space-separated global values are not parsed; the value reads as the + // subcommand, which is in no read set, so it stays on the login shell. + [['--git-dir', '/x', 'log']], + [['--work-tree', '/x', 'status']], + [['-c', 'k=v']] + ])('keeps %j on the login shell behind global options', (args) => { + expect(isWslDirectGitReadCommand(args)).toBe(false) + }) +}) diff --git a/src/main/git/wsl-direct-git-read-commands.ts b/src/main/git/wsl-direct-git-read-commands.ts new file mode 100644 index 00000000000..18bea5e46ab --- /dev/null +++ b/src/main/git/wsl-direct-git-read-commands.ts @@ -0,0 +1,105 @@ +/** + * Decide whether a git invocation is a plain read that can run without a shell. + * + * Why: WSL-routed git otherwise goes through the distro user's interactive login + * shell, purely to inherit their PATH. That shell also runs the distro's rc/motd + * and writes it to the stdout callers parse. Reads need none of it -- the direct + * route supplies PATH and HOME explicitly and starts no shell at all. + * + * Writes and network operations stay on the login shell: they can depend on + * credential helpers, ssh-agent and other environment only the user's profile + * sets up. + */ + +// Subcommands that only ever read. `status` is here for completeness; its +// callers already opted in explicitly. +const ALWAYS_READ_SUBCOMMANDS = new Set([ + 'blame', + 'cat-file', + 'check-ignore', + 'describe', + 'diff', + 'for-each-ref', + 'log', + 'ls-files', + 'ls-tree', + 'merge-base', + 'name-rev', + 'rev-list', + 'rev-parse', + 'show', + 'show-ref', + 'status', + 'var' +]) + +// Read markers that appear as a flag anywhere after the subcommand. +const READ_FLAG_SUBCOMMANDS: Record> = { + branch: new Set(['--list', '-l', '--show-current', '--contains', '--points-at']), + config: new Set(['--get', '--get-all', '--get-regexp', '--get-urlmatch', '--list', '-l']) +} + +// Read markers that must be the *first non-flag* argument, i.e. the action. +// Position matters here: matching them anywhere would read `worktree remove list` +// as a listing, because a worktree may legitimately be named "list". +const READ_ACTION_SUBCOMMANDS: Record> = { + remote: new Set(['get-url']), + submodule: new Set(['status']), + worktree: new Set(['list']) +} + +// Subcommands whose action-less form only lists (`git remote`, `git submodule`). +const BARE_FORM_IS_READ = new Set(['remote', 'submodule']) + +/** Leading `-c key=value` / `--git-dir=...` style options precede the subcommand. */ +function findSubcommandIndex(args: readonly string[]): number { + for (let index = 0; index < args.length; index += 1) { + const arg = args[index] + if (arg === '-c' || arg === '-C') { + index += 1 + continue + } + if (arg.startsWith('-')) { + continue + } + return index + } + return -1 +} + +export function isWslDirectGitReadCommand(args: readonly string[]): boolean { + const subcommandIndex = findSubcommandIndex(args) + if (subcommandIndex === -1) { + return false + } + const subcommand = args[subcommandIndex] + if (ALWAYS_READ_SUBCOMMANDS.has(subcommand)) { + return true + } + const rest = args.slice(subcommandIndex + 1) + + if (subcommand === 'symbolic-ref') { + if (rest.some((arg) => arg === '-d' || arg === '--delete' || arg === '-m')) { + return false + } + // Reading takes one ref; a second positional is the value being written. + return rest.filter((arg) => arg !== '--' && !arg.startsWith('-')).length <= 1 + } + + const readActions = READ_ACTION_SUBCOMMANDS[subcommand] + if (readActions) { + const action = rest.find((arg) => !arg.startsWith('-')) + if (!action) { + return BARE_FORM_IS_READ.has(subcommand) + } + // `remote show` queries the transport unless -n is given, so the queried + // form has to keep the profile's SSH and credential setup. + if (subcommand === 'remote' && action === 'show') { + return rest.includes('-n') + } + return readActions.has(action) + } + + const readFlags = READ_FLAG_SUBCOMMANDS[subcommand] + return Boolean(readFlags && rest.some((arg) => readFlags.has(arg.split('=')[0]))) +}