From ef0d5931bc59c31bd7f861cc3f66233cdef4ae2e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 26 Aug 2026 14:37:15 -0700 Subject: [PATCH] fix(source-control): budget WSL bulk git command lines by bytes, not path count (#16634) Selecting ~100 changed files in a WSL worktree and hitting Stage All did nothing: the files stayed unstaged and the operation reported a failure. Bulk stage/unstage/discard chunked pathspecs 100 at a time, a count picked against a raw argv. A WSL-routed write is not a raw argv -- it is folded into one login-shell command line that shell-quotes every pathspec, quotes the result again, and embeds it three times (one branch per guest shell), so the finished line runs ~3.4x the raw pathspec bytes. Realistic project paths blew past the 32767-character CreateProcess cap at 100 paths and wsl.exe refused to spawn, with nothing staged. Chunking now measures the finished command line through the real resolver, so the wrapper's quoting rules live in one place and native, WSL and SSH hosts each get the budget of the host that actually spawns. A pathspec too long to fit alone still ships alone rather than being dropped, and no chunk is ever emitted empty -- a pathspec-free `clean -ffdx` would have swept the whole worktree. The tracked-path listing behind that discard also fences the WSL login shell now. Its stdout was parsed NUL-delimited without a fence, so Ubuntu's interactive rc banner glued itself onto the first record: that path failed to match anything git reported and was treated as untracked, sending a tracked file to `git clean` instead of `git restore`. Not observing a path in ls-files output is not evidence the path is untracked. The Windows command-line cap and its libuv-aware length estimate move out of the WSL runner into src/shared/windows-command-line-budget.ts, shared by both callers. --- .../bulk-pathspec-command-line-budget.test.ts | 193 ++++++++++++++++++ .../git/source-control/discard-changes.ts | 57 +++--- src/main/git/source-control/git-pathspec.ts | 84 +++++++- src/main/git/source-control/staging.ts | 31 +-- .../wsl-tracked-pathspec-banner.test.ts | 109 ++++++++++ .../status-discard-and-bulk-staging.test.ts | 4 +- src/main/wsl/wsl-runner.ts | 36 +--- src/shared/windows-command-line-budget.ts | 28 +++ 8 files changed, 454 insertions(+), 88 deletions(-) create mode 100644 src/main/git/source-control/bulk-pathspec-command-line-budget.test.ts create mode 100644 src/main/git/source-control/wsl-tracked-pathspec-banner.test.ts create mode 100644 src/shared/windows-command-line-budget.ts diff --git a/src/main/git/source-control/bulk-pathspec-command-line-budget.test.ts b/src/main/git/source-control/bulk-pathspec-command-line-budget.test.ts new file mode 100644 index 00000000000..39c4135aade --- /dev/null +++ b/src/main/git/source-control/bulk-pathspec-command-line-budget.test.ts @@ -0,0 +1,193 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + commandLineLength, + MAX_COMMAND_LINE_CHARS +} from '../../../shared/windows-command-line-budget' +import { resolveGitCommandWithoutProbe } from '../command-runner/git-command-resolution' + +const gitExecFileAsync = vi.fn(async () => ({ stdout: '', stderr: '' })) + +vi.mock('../runner', () => ({ + gitExecFileAsync: (...args: unknown[]) => + (gitExecFileAsync as unknown as (...a: unknown[]) => Promise<{ stdout: string }>)(...args) +})) +vi.mock('./git-read-cache-invalidation', () => ({ invalidateGitReadCaches: vi.fn() })) +vi.mock('../../../shared/git-discard-path-safety', () => ({ + removeSafeUntrackedDiscardTarget: vi.fn(), + removeSafeUntrackedDiscardTargets: async ( + _worktreePath: string, + untrackedPaths: string[], + cleanUntracked: (paths: string[]) => Promise, + restoreTracked: () => Promise + ) => { + await restoreTracked() + if (untrackedPaths.length > 0) { + await cleanUntracked(untrackedPaths) + } + } +})) + +const WSL_DISTRO = 'Ubuntu-24.04' +const WSL_WORKTREE = `\\\\wsl$\\${WSL_DISTRO}\\home\\emilio\\projects\\orca` + +/** Windows-side length of the line `wsl.exe` is spawned with, wrapper included. */ +function finishedCommandLineLength(args: readonly string[], wslDistro?: string): number { + const resolved = resolveGitCommandWithoutProbe([...args], { + cwd: wslDistro ? WSL_WORKTREE : '/home/emilio/projects/orca', + ...(wslDistro ? { wslDistro } : {}) + }) + return commandLineLength([resolved.binary, ...resolved.args]) +} + +/** Deep nesting, a space, and non-ASCII: all three inflate the quoted line. */ +function realisticChangedPaths(count: number): string[] { + return Array.from( + { length: count }, + (_, index) => + `apps/web/src/components/dashboard/widgets/analytics/rapport trimestriel ${String(index).padStart(3, '0')}/données-générales/AnalyticsSummaryWidget${index}.tsx` + ) +} + +function capturedInvocations(): string[][] { + return gitExecFileAsync.mock.calls.map((call) => (call as unknown as [string[]])[0]) +} + +describe('bulk pathspec command-line budget', () => { + const realPlatform = process.platform + + beforeEach(() => { + gitExecFileAsync.mockReset() + gitExecFileAsync.mockImplementation(async () => ({ stdout: '', stderr: '' })) + Object.defineProperty(process, 'platform', { value: 'win32' }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform }) + vi.resetModules() + }) + + it('keeps every WSL bulk-stage invocation inside the Windows command-line cap', async () => { + const { bulkStageFiles } = await import('./staging') + const filePaths = realisticChangedPaths(100) + + await bulkStageFiles(WSL_WORKTREE, filePaths, { wslDistro: WSL_DISTRO }) + + const invocations = capturedInvocations() + expect(invocations.length).toBeGreaterThan(0) + const lengths = invocations.map((args) => finishedCommandLineLength(args, WSL_DISTRO)) + expect(Math.max(...lengths)).toBeLessThanOrEqual(MAX_COMMAND_LINE_CHARS) + }) + + it('stages every path exactly once, in order, across the chunks', async () => { + const { bulkStageFiles } = await import('./staging') + const filePaths = realisticChangedPaths(250) + + await bulkStageFiles(WSL_WORKTREE, filePaths, { wslDistro: WSL_DISTRO }) + + const staged = capturedInvocations().flatMap((args) => args.slice(args.indexOf('--') + 1)) + expect(staged).toEqual(filePaths.map((filePath) => `:(literal)${filePath}`)) + }) + + it('keeps WSL bulk unstage inside the cap', async () => { + const { bulkUnstageFiles } = await import('./staging') + + await bulkUnstageFiles(WSL_WORKTREE, realisticChangedPaths(100), { wslDistro: WSL_DISTRO }) + + const lengths = capturedInvocations().map((args) => finishedCommandLineLength(args, WSL_DISTRO)) + expect(Math.max(...lengths)).toBeLessThanOrEqual(MAX_COMMAND_LINE_CHARS) + }) + + it('never emits a pathspec-free chunk, which would widen `clean -ffdx` to the worktree', async () => { + const { bulkStageFiles } = await import('./staging') + + await bulkStageFiles(WSL_WORKTREE, realisticChangedPaths(300), { wslDistro: WSL_DISTRO }) + + for (const args of capturedInvocations()) { + expect(args.slice(args.indexOf('--') + 1).length).toBeGreaterThan(0) + } + }) + + it('splits a WSL bulk discard of tracked paths into spawnable restores', async () => { + const filePaths = realisticChangedPaths(120) + gitExecFileAsync.mockImplementation(async () => ({ stdout: filePaths.join('\0'), stderr: '' })) + const { bulkDiscardChanges } = await import('./discard-changes') + + await bulkDiscardChanges(WSL_WORKTREE, filePaths, { wslDistro: WSL_DISTRO }) + + const restores = capturedInvocations().filter((args) => args[0] === 'restore') + expect(restores.length).toBeGreaterThan(1) + for (const args of restores) { + expect(finishedCommandLineLength(args, WSL_DISTRO)).toBeLessThanOrEqual( + MAX_COMMAND_LINE_CHARS + ) + } + }) + + it('never widens `clean -ffdx` past the paths it was given', async () => { + const filePaths = realisticChangedPaths(120) + const { bulkDiscardChanges } = await import('./discard-changes') + + // Empty ls-files output: every path is untracked, so all of them take the clean lane. + await bulkDiscardChanges(WSL_WORKTREE, filePaths, { wslDistro: WSL_DISTRO }) + + const cleans = capturedInvocations().filter((args) => args[0] === 'clean') + expect(cleans.length).toBeGreaterThan(1) + const cleaned = cleans.flatMap((args) => args.slice(args.indexOf('--') + 1)) + expect(cleaned).toEqual(filePaths.map((filePath) => `:(literal)${filePath}`)) + for (const args of cleans) { + expect(finishedCommandLineLength(args, WSL_DISTRO)).toBeLessThanOrEqual( + MAX_COMMAND_LINE_CHARS + ) + } + }) + + it('ships a single over-budget pathspec alone rather than dropping it', async () => { + const { bulkStageFiles } = await import('./staging') + const hugePath = `src/${'nested-directory/'.repeat(700)}Component.tsx` + + await bulkStageFiles(WSL_WORKTREE, [hugePath, 'src/app.tsx'], { wslDistro: WSL_DISTRO }) + + const invocations = capturedInvocations() + expect(invocations).toHaveLength(2) + expect(invocations[0]).toEqual(['add', '--', `:(literal)${hugePath}`]) + expect(invocations[1]).toEqual(['add', '--', ':(literal)src/app.tsx']) + }) + + it('packs chunks to the budget instead of splitting timidly', async () => { + const { bulkStageFiles } = await import('./staging') + + await bulkStageFiles(WSL_WORKTREE, realisticChangedPaths(100), { wslDistro: WSL_DISTRO }) + + const lengths = capturedInvocations().map((args) => finishedCommandLineLength(args, WSL_DISTRO)) + // Every chunk but the last is filled to within one pathspec of the cap. + expect(Math.min(...lengths.slice(0, -1))).toBeGreaterThan(MAX_COMMAND_LINE_CHARS * 0.9) + }) + + it('gives a native Windows git.exe the CreateProcess cap and a POSIX host a larger one', async () => { + const { bulkPathspecCommands } = await import('./git-pathspec') + // Long enough that the raw argv alone passes the Windows cap with no wrapper in sight. + const filePaths = Array.from( + { length: 100 }, + (_, index) => `packages/${'deeply-nested-module/'.repeat(18)}file-${index}.ts` + ) + + const windowsNative = bulkPathspecCommands(['add', '--'], filePaths, 'C:\\repo', {}) + expect(windowsNative.length).toBeGreaterThan(1) + for (const args of windowsNative) { + expect(finishedCommandLineLength(args)).toBeLessThanOrEqual(MAX_COMMAND_LINE_CHARS) + } + + Object.defineProperty(process, 'platform', { value: 'linux' }) + expect(bulkPathspecCommands(['add', '--'], filePaths, '/repo', {})).toHaveLength(1) + }) + + it('does not charge a native invocation for the WSL wrapper', async () => { + Object.defineProperty(process, 'platform', { value: 'darwin' }) + const { bulkStageFiles } = await import('./staging') + + await bulkStageFiles('/home/emilio/projects/orca', realisticChangedPaths(100)) + + // Same 100 paths that need several chunks under the WSL wrapper stay one native call. + expect(capturedInvocations()).toHaveLength(1) + }) +}) diff --git a/src/main/git/source-control/discard-changes.ts b/src/main/git/source-control/discard-changes.ts index 33fd2f3f84a..e06f76f0c44 100644 --- a/src/main/git/source-control/discard-changes.ts +++ b/src/main/git/source-control/discard-changes.ts @@ -7,7 +7,7 @@ import type { GitRuntimeOptions } from '../git-runtime-options' import { gitOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync } from '../runner' import { invalidateGitReadCaches } from './git-read-cache-invalidation' -import { BULK_CHUNK_SIZE, isTrackedPathSpec, literalPathspec } from './git-pathspec' +import { bulkPathspecCommands, isTrackedPathSpec, literalPathspec } from './git-pathspec' /** * Discard working tree changes for a file. @@ -62,14 +62,15 @@ async function listTrackedPathSpecs( options: GitRuntimeOptions = {} ): Promise { const trackedPaths: string[] = [] - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - const { stdout } = await gitExecFileAsync( - ['ls-files', '-z', '--', ...chunk.map((filePath) => literalPathspec(filePath, options))], - { - ...gitOptionsForWorktree(worktreePath, options) - } - ) + const commands = bulkPathspecCommands(['ls-files', '-z', '--'], filePaths, worktreePath, options) + for (const args of commands) { + const { stdout } = await gitExecFileAsync(args, { + ...gitOptionsForWorktree(worktreePath, options), + // Why: this buffers rather than streams, so it must fence -- an unfenced WSL + // login shell glues its rc banner onto the first NUL record, and a tracked + // path that fails to match is silently reclassified as untracked. + captureWslLoginShellOutput: true + }) // Why: a tracked directory can hold enough paths to exceed the JS argument limit. for (const trackedPath of stdout.split('\0')) { if (trackedPath) { @@ -85,17 +86,11 @@ async function cleanUntrackedPaths( filePaths: readonly string[], options: GitRuntimeOptions = {} ): Promise { - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - if (chunk.length > 0) { - // Why: Git pathspec cleanup avoids raw recursive deletion through symlinked parents. - await gitExecFileAsync( - ['clean', '-ffdx', '--', ...chunk.map((filePath) => literalPathspec(filePath, options))], - { - ...gitOptionsForWorktree(worktreePath, options) - } - ) - } + // Why: Git pathspec cleanup avoids raw recursive deletion through symlinked parents. + // A pathspec-free `clean -ffdx` would sweep the whole worktree; the chunker emits no empty chunk. + const commands = bulkPathspecCommands(['clean', '-ffdx', '--'], filePaths, worktreePath, options) + for (const args of commands) { + await gitExecFileAsync(args, { ...gitOptionsForWorktree(worktreePath, options) }) } } @@ -133,20 +128,14 @@ export async function bulkDiscardChanges( untrackedPaths, (targetPaths) => cleanUntrackedPaths(worktreePath, targetPaths, options), async () => { - for (let i = 0; i < trackedPaths.length; i += BULK_CHUNK_SIZE) { - const chunk = trackedPaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync( - [ - 'restore', - '--worktree', - '--source=HEAD', - '--', - ...chunk.map((filePath) => literalPathspec(filePath, options)) - ], - { - ...gitOptionsForWorktree(worktreePath, options) - } - ) + const commands = bulkPathspecCommands( + ['restore', '--worktree', '--source=HEAD', '--'], + trackedPaths, + worktreePath, + options + ) + for (const args of commands) { + await gitExecFileAsync(args, { ...gitOptionsForWorktree(worktreePath, options) }) } } ) diff --git a/src/main/git/source-control/git-pathspec.ts b/src/main/git/source-control/git-pathspec.ts index 29ce04ade99..c4fe23d697e 100644 --- a/src/main/git/source-control/git-pathspec.ts +++ b/src/main/git/source-control/git-pathspec.ts @@ -1,6 +1,20 @@ +import { + commandLineLength, + MAX_COMMAND_LINE_CHARS +} from '../../../shared/windows-command-line-budget' +import { resolveGitCommandWithoutProbe } from '../command-runner/git-command-resolution' import type { GitRuntimeOptions } from '../git-runtime-options' -export const BULK_CHUNK_SIZE = 100 +/** Ceiling on argv entries per invocation; under WSL the byte budget bites first. */ +const BULK_CHUNK_SIZE = 100 + +/** + * POSIX hosts have no CreateProcess cap: ARG_MAX is 256KB on macOS and 2MB on + * Linux, shared with the environment block. Half the macOS floor keeps a native + * or SSH-host invocation clear of E2BIG without charging it the WSL wrapper's + * quoting overhead, which is a different transport's problem. + */ +const POSIX_COMMAND_LINE_BUDGET = 128_000 function normalizeGitPathForCompare(filePath: string): string { return filePath.replace(/\\/g, '/').replace(/\/+$/, '') @@ -19,3 +33,71 @@ export function isTrackedPathSpec(filePath: string, trackedPaths: readonly strin return normalizedTracked === normalized || normalizedTracked.startsWith(`${normalized}/`) }) } + +/** + * Length of the line the OS will actually be handed, wrapper included. + * + * Why resolve rather than estimate: a WSL-routed write goes through the login + * shell, which shell-quotes every pathspec, quotes the resulting command line + * again, and embeds that three times (one branch per guest shell). The finished + * line runs ~3.4x the raw pathspec bytes, and nothing about the path list says + * so. Writes never take the direct-git lane, so this is the exact shape they + * get; a read that does take it resolves shorter, so the estimate stays safe. + */ +function finishedCommandLineLength( + args: readonly string[], + worktreePath: string, + options: GitRuntimeOptions +): number { + const resolved = resolveGitCommandWithoutProbe([...args], { + cwd: worktreePath, + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}) + }) + return commandLineLength([resolved.binary, ...resolved.args]) +} + +/** + * Split a bulk pathspec operation into invocations the host can actually spawn. + * + * Why a byte budget and not a path count: 100 was chosen against a raw argv, but + * a WSL-routed `git add` is folded into one login-shell command line, so 100 + * ordinary paths reached ~43,000 characters -- past the 32,767 CreateProcess cap + * -- and the bulk stage failed with nothing staged. Cost is measured per + * pathspec through the real resolver so the wrapper's quoting rules live in one + * place. + * + * Chunks split only between whole pathspecs, and a pathspec that alone exceeds + * the budget still ships alone rather than being dropped or truncated. If an + * invocation fails partway through, the earlier chunks stay applied: every + * operation here is idempotent and per-path, so `git status` shows the true + * state and re-running converges. + */ +export function bulkPathspecCommands( + leadingArgs: readonly string[], + filePaths: readonly string[], + worktreePath: string, + options: GitRuntimeOptions +): string[][] { + // Budget belongs to the host that spawns; the overhead measured above belongs to the transport. + const budget = process.platform === 'win32' ? MAX_COMMAND_LINE_CHARS : POSIX_COMMAND_LINE_BUDGET + const baseLength = finishedCommandLineLength(leadingArgs, worktreePath, options) + const commands: string[][] = [] + let pathspecs: string[] = [] + let length = baseLength + for (const filePath of filePaths) { + const pathspec = literalPathspec(filePath, options) + const cost = + finishedCommandLineLength([...leadingArgs, pathspec], worktreePath, options) - baseLength + if (pathspecs.length > 0 && (pathspecs.length >= BULK_CHUNK_SIZE || length + cost > budget)) { + commands.push([...leadingArgs, ...pathspecs]) + pathspecs = [] + length = baseLength + } + pathspecs.push(pathspec) + length += cost + } + if (pathspecs.length > 0) { + commands.push([...leadingArgs, ...pathspecs]) + } + return commands +} diff --git a/src/main/git/source-control/staging.ts b/src/main/git/source-control/staging.ts index 5aa8f55fa48..3aa6e16bbc9 100644 --- a/src/main/git/source-control/staging.ts +++ b/src/main/git/source-control/staging.ts @@ -2,7 +2,7 @@ import type { GitRuntimeOptions } from '../git-runtime-options' import { gitOptionsForWorktree } from '../git-runtime-options' import { gitExecFileAsync } from '../runner' import { invalidateGitReadCaches } from './git-read-cache-invalidation' -import { BULK_CHUNK_SIZE, literalPathspec } from './git-pathspec' +import { bulkPathspecCommands, literalPathspec } from './git-pathspec' /** * Stage a file. @@ -54,12 +54,8 @@ export async function bulkStageFiles( return } try { - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync( - ['add', '--', ...chunk.map((filePath) => literalPathspec(filePath, options))], - gitOptionsForWorktree(worktreePath, options) - ) + for (const args of bulkPathspecCommands(['add', '--'], filePaths, worktreePath, options)) { + await gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)) } } finally { invalidateGitReadCaches() @@ -79,19 +75,14 @@ export async function bulkUnstageFiles( return } try { - for (let i = 0; i < filePaths.length; i += BULK_CHUNK_SIZE) { - const chunk = filePaths.slice(i, i + BULK_CHUNK_SIZE) - await gitExecFileAsync( - [ - 'restore', - '--staged', - '--', - ...chunk.map((filePath) => literalPathspec(filePath, options)) - ], - { - ...gitOptionsForWorktree(worktreePath, options) - } - ) + const commands = bulkPathspecCommands( + ['restore', '--staged', '--'], + filePaths, + worktreePath, + options + ) + for (const args of commands) { + await gitExecFileAsync(args, { ...gitOptionsForWorktree(worktreePath, options) }) } } finally { invalidateGitReadCaches() diff --git a/src/main/git/source-control/wsl-tracked-pathspec-banner.test.ts b/src/main/git/source-control/wsl-tracked-pathspec-banner.test.ts new file mode 100644 index 00000000000..fbae736e77f --- /dev/null +++ b/src/main/git/source-control/wsl-tracked-pathspec-banner.test.ts @@ -0,0 +1,109 @@ +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() })) +vi.mock('./git-read-cache-invalidation', () => ({ invalidateGitReadCaches: vi.fn() })) +// Stand in for the on-disk safety filter: every discard target here exists and is symlink-free. +vi.mock('../../../shared/git-discard-path-safety', () => ({ + removeSafeUntrackedDiscardTarget: vi.fn(), + removeSafeUntrackedDiscardTargets: async ( + _worktreePath: string, + untrackedPaths: string[], + cleanUntracked: (paths: string[]) => Promise, + restoreTracked: () => Promise + ) => { + await restoreTracked() + if (untrackedPaths.length > 0) { + await cleanUntracked(untrackedPaths) + } + } +})) + +import { bulkDiscardChanges } from './discard-changes' +import { resetWslGitReadEnvironmentForTests } from '../wsl-git-read-environment' + +const DISTRO = 'Ubuntu-24.04' +const WSL_WORKTREE = `\\\\wsl$\\${DISTRO}\\home\\emilio\\projects\\orca` +const TRACKED_PATHS = ['docs/architecture.md', 'src/main/git/runner.ts'] +// Stock Ubuntu writes this to *stdout* from the interactive login shell's rc. +const BANNER = 'To run a command as administrator (user "root"), use "sudo ".\n\n' + +type MockChild = EventEmitter & { stdout: EventEmitter; stderr: EventEmitter; kill: () => void } + +function createMockChild(): MockChild { + const child = new EventEmitter() as MockChild + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + child.kill = vi.fn() + return child +} + +function guestScript(args: unknown): string { + return (args as string[] | undefined)?.join(' ') ?? '' +} + +/** Wrap the payload in the caller's own fence when it asked for one; otherwise hand it over raw. */ +function loginShellStdout(script: string, payload: string): string { + const nonce = /__ORCA_WSL_CAPTURE_BEGIN_([^_]+)__/.exec(script)?.[1] + return nonce + ? `${BANNER}__ORCA_WSL_CAPTURE_BEGIN_${nonce}__${payload}__ORCA_WSL_CAPTURE_END_${nonce}__` + : `${BANNER}${payload}` +} + +function gitCommandLines(): string[] { + return execFileMock.mock.calls + .map((call) => guestScript(call[1])) + .filter((script) => !script.includes('_orca_git_path=')) +} + +describe('WSL tracked-path listing behind a login-shell banner', () => { + const realPlatform = process.platform + + beforeEach(() => { + resetWslGitReadEnvironmentForTests() + execFileMock.mockReset() + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + execFileMock.mockImplementation((_command, args, _options, callback) => { + const script = guestScript(args) + if (script.includes('_orca_git_path=')) { + // Distro exports GIT_* / XDG_CONFIG_HOME, so the direct-git read probe is rejected for good. + queueMicrotask(() => callback?.(Object.assign(new Error('probe rejected'), { code: 78 }))) + return createMockChild() + } + const payload = script.includes('ls-files') ? `${TRACKED_PATHS.join('\0')}\0` : '' + queueMicrotask(() => callback?.(null, loginShellStdout(script, payload), '')) + return createMockChild() + }) + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: realPlatform }) + resetWslGitReadEnvironmentForTests() + }) + + it('restores every tracked path instead of routing the first one to git clean', async () => { + await bulkDiscardChanges(WSL_WORKTREE, [...TRACKED_PATHS], { wslDistro: DISTRO }) + + const commandLines = gitCommandLines() + expect(commandLines.filter((line) => line.includes('clean'))).toEqual([]) + const restored = commandLines.filter((line) => line.includes('restore')) + expect(restored.length).toBeGreaterThan(0) + for (const trackedPath of TRACKED_PATHS) { + expect(restored.some((line) => line.includes(`:(literal)${trackedPath}`))).toBe(true) + } + }) +}) diff --git a/src/main/git/status-discard-and-bulk-staging.test.ts b/src/main/git/status-discard-and-bulk-staging.test.ts index b1f39f9102b..8e0b12623c4 100644 --- a/src/main/git/status-discard-and-bulk-staging.test.ts +++ b/src/main/git/status-discard-and-bulk-staging.test.ts @@ -198,7 +198,9 @@ describe('bulk git helpers', () => { ':(literal)scratch' ], { - cwd: '/repo' + cwd: '/repo', + // Why: this read buffers, so it fences the WSL login shell's rc banner off its first NUL record. + captureWslLoginShellOutput: true } ) // Why: a pathspec is tracked if git reports either the exact path or a diff --git a/src/main/wsl/wsl-runner.ts b/src/main/wsl/wsl-runner.ts index 6a0fbc051ac..30097246a86 100644 --- a/src/main/wsl/wsl-runner.ts +++ b/src/main/wsl/wsl-runner.ts @@ -1,4 +1,5 @@ import { addWslEnvKeys } from '../../shared/wsl-env' +import { commandLineLength, MAX_COMMAND_LINE_CHARS } from '../../shared/windows-command-line-budget' import { runProcess } from '../../shared/child-process/run-process' import { buildWslExecArgs } from '../../shared/wsl-login-shell-command' import { getWslGuestEnvironment, type WslGuestEnvironment } from './wsl-guest-environment' @@ -159,25 +160,6 @@ function withGuestCwd(cwd: string | undefined, argv: readonly string[]): string[ return ['sh', '-c', 'cd "$1" || exit 1; shift; exec "$@"', 'orca-wsl', cwd, ...argv] } -/** - * Argv is the default, but it has a hard ceiling that stdin does not. - * - * Windows caps a command line at 32767 characters, and the distro, `--exec`, - * the env prefix and the args all share it. A user's `orca.yaml` hook is the - * one unbounded script Orca runs -- `run-both` concatenates two of them, and a - * vendored installer is ~15KB -- so past this size the choice is between - * failing to spawn at all and accepting the stdin caveat. Degrading beats - * failing: a large script that also reads stdin was already broken, while a - * large script that does not now works where it would have died. - * - * Measured on the WHOLE command line, not on the script alone. A login PATH is - * itself a few KB and is spliced in as `PATH=...`, so a script-only threshold - * produced a perverse band: with a long enough PATH, a 7,999-char hook went to - * argv and failed to spawn while the same hook at 8,001 chars flipped to stdin - * and ran. Size decided how a hook behaved, in the wrong direction. - */ -const MAX_COMMAND_LINE_CHARS = 30_000 - /** ` -c`/`-s` for a script, otherwise the program itself. */ function guestCommandArgv(spec: WslSpec, delivery: 'argv' | 'stdin'): string[] { if (spec.script === undefined) { @@ -190,19 +172,6 @@ function guestCommandArgv(spec: WslSpec, delivery: 'argv' | 'stdin'): string[] { : [shell, '-c', spec.script, '--', ...(spec.args ?? [])] } -/** - * What `CreateProcess` will count. - * - * libuv escapes every `"` and doubles a backslash run before a quote, so a - * quote-dense script costs more than its length. Charging one extra character - * per `"` or `\\` keeps the estimate on the safe side of the cap; an earlier - * version claimed to over-count and in fact under-counted, which put a - * quote-heavy ~26KB script on argv and over the real limit. - */ -function commandLineLength(args: readonly string[]): number { - return args.reduce((total, arg) => total + arg.length + 3 + (arg.match(/["\\]/g)?.length ?? 0), 0) -} - /** Shell-free argv, with the cached environment applied when one is available. */ function buildGuestArgv( environment: WslGuestEnvironment | null, @@ -255,6 +224,9 @@ export async function runWslProcess(spec: WslSpec): Promise { // Measure what is actually spawned: `wsl.exe` and `-d --exec` are // prepended after this point and are part of the same budget. const fullLine = [resolveWslExecutablePath(), ...buildWslExecArgs(spec.distro, argvForm)] + // Argv is the default, but it has a hard ceiling that stdin does not. A user's + // `orca.yaml` hook is the one unbounded script Orca runs, so past the cap the + // choice is between failing to spawn at all and accepting the stdin caveat. const delivery: 'argv' | 'stdin' = spec.script !== undefined && commandLineLength(fullLine) > MAX_COMMAND_LINE_CHARS ? 'stdin' diff --git a/src/shared/windows-command-line-budget.ts b/src/shared/windows-command-line-budget.ts new file mode 100644 index 00000000000..77074e3f531 --- /dev/null +++ b/src/shared/windows-command-line-budget.ts @@ -0,0 +1,28 @@ +/** + * How long a command line may get before `CreateProcess` refuses it. + * + * Windows caps a command line at 32767 characters, and *everything* shares that + * one budget: the binary, `wsl.exe -d --exec`, the login-shell wrapper + * and the payload. So the number only means anything when it is measured on the + * FINISHED line. Two shipped defects came from measuring a part instead: a + * script-only threshold in the WSL runner, where a multi-KB login PATH pushed a + * legal-looking script over the real limit, and count-only chunking of bulk git + * pathspecs, where the login-shell wrapper tripled the line behind our back. + * + * The 2767-character margin absorbs what we do not model exactly (the distro + * name, libuv's requoting of the outer argv). + */ +export const MAX_COMMAND_LINE_CHARS = 30_000 + +/** + * What `CreateProcess` will count. + * + * libuv escapes every `"` and doubles a backslash run before a quote, so a + * quote-dense script costs more than its length. Charging one extra character + * per `"` or `\\` keeps the estimate on the safe side of the cap; an earlier + * version claimed to over-count and in fact under-counted, which put a + * quote-heavy ~26KB script on argv and over the real limit. + */ +export function commandLineLength(args: readonly string[]): number { + return args.reduce((total, arg) => total + arg.length + 3 + (arg.match(/["\\]/g)?.length ?? 0), 0) +}