From 749cb0fbfc5f991792cfd1a772009666e715df22 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:11:16 -0700 Subject: [PATCH] fix(windows): follow the host block's PATH spelling on sparse daemon env patches Resolving a path-less Windows env to `Path` handed the daemon's own `{...process.env, ...opts.env}` merge both spellings when the host block spelt `PATH`. Fall back to the host block's own key, and collapse again inside the daemon since that merge happens after attribution. Co-authored-by: Orca --- ...rminal-attribution-path-env-casing.test.ts | 35 +++++++++++++++++++ src/main/attribution/terminal-attribution.ts | 6 ++-- src/main/daemon/pty-subprocess.test.ts | 23 ++++++++++-- src/main/daemon/pty-subprocess.ts | 34 +++++++++++++++++- src/main/pty/windows-environment-path.test.ts | 7 ++-- src/main/pty/windows-environment-path.ts | 25 +++++++++++-- 6 files changed, 119 insertions(+), 11 deletions(-) diff --git a/src/main/attribution/terminal-attribution-path-env-casing.test.ts b/src/main/attribution/terminal-attribution-path-env-casing.test.ts index 8d4036ba4a2..d301ad26cca 100644 --- a/src/main/attribution/terminal-attribution-path-env-casing.test.ts +++ b/src/main/attribution/terminal-attribution-path-env-casing.test.ts @@ -83,6 +83,41 @@ describe('applyTerminalAttributionEnv PATH key casing', () => { expect(baseEnv.Path).toBe('C:\\Windows\\system32') }) + // Why: the daemon receives a sparse env patch with no path key and re-merges its own block + // underneath it, so guessing the spelling here hands the child both. + it.each([ + { hostKey: 'PATH', otherKey: 'Path' }, + { hostKey: 'Path', otherKey: 'PATH' } + ])('adopts a host block spelt `$hostKey` on a path-less env', ({ hostKey, otherKey }) => { + const root = makeTmpRoot() + const userDataPath = join(root, 'user-data') + const inherited = { PATH: process.env.PATH, Path: process.env.Path } + delete process.env.PATH + delete process.env.Path + process.env[hostKey] = 'C:\\Windows\\system32' + + try { + const baseEnv: Record = { SystemRoot: 'C:\\Windows' } + + applyTerminalAttributionEnv(baseEnv, { + enabled: true, + platform: 'win32', + shellFamily: 'native-windows', + userDataPath + }) + + expect(pathEnvKeys(baseEnv)).toEqual([hostKey]) + expect(baseEnv[otherKey]).toBeUndefined() + } finally { + delete process.env[hostKey] + for (const [key, value] of Object.entries(inherited)) { + if (value !== undefined) { + process.env[key] = value + } + } + } + }) + it('leaves a case-sensitive POSIX `Path` variable alone', () => { const root = makeTmpRoot() const baseEnv: Record = { diff --git a/src/main/attribution/terminal-attribution.ts b/src/main/attribution/terminal-attribution.ts index c2878252103..bbce8ff97a1 100644 --- a/src/main/attribution/terminal-attribution.ts +++ b/src/main/attribution/terminal-attribution.ts @@ -147,9 +147,9 @@ function clearTerminalAttributionEnv( /** * Drops the non-resolved PATH spelling so a Windows child inherits exactly one. * - * Why: attribution is the last env-key-shaping step before spawn, making it the single - * choke point where an inherited duplicate (WSL interop, third-party tools) can be - * collapsed deterministically instead of leaving the OS to pick a winner. + * Why: an inherited duplicate (WSL interop, third-party tools) is collapsed here rather than + * left for the OS to pick a winner. Providers that re-merge their own block after this point + * (the daemon) collapse again on their side, since this cannot see that merge. */ function collapseWindowsPathEnvKey( baseEnv: Record, diff --git a/src/main/daemon/pty-subprocess.test.ts b/src/main/daemon/pty-subprocess.test.ts index 7b40b2a1ae4..7f534d00ebf 100644 --- a/src/main/daemon/pty-subprocess.test.ts +++ b/src/main/daemon/pty-subprocess.test.ts @@ -2113,7 +2113,7 @@ describe('createPtySubprocess', () => { expect(lastCall[2].env.ORCA_ATTRIBUTION_SHIM_DIR).toBeUndefined() }) - it('promotes the agent-teams shim onto the Windows `Path` spelling', () => { + it('collapses its own env merge onto the requested Windows `Path` spelling', () => { const proc = mockPtyProcess() spawnMock.mockReturnValue(proc) const platform = Object.getOwnPropertyDescriptor(process, 'platform') @@ -2124,7 +2124,8 @@ describe('createPtySubprocess', () => { sessionId: 'test', cols: 80, rows: 24, - // Why: buildPtyHostEnv collapses Windows PATH onto one spelling before the daemon wire. + // Why: buildPtyHostEnv collapses Windows PATH onto one spelling before the daemon wire; + // the daemon then spreads its own block underneath and can re-mint the other one. env: { Path: '/tmp/orca-agent-teams-bin:/usr/bin', ORCA_AGENT_TEAMS_TEAM_ID: 'team-test' @@ -2137,7 +2138,25 @@ describe('createPtySubprocess', () => { } const env = spawnMock.mock.calls.at(-1)![2].env + expect(Object.keys(env).filter((key) => /^path$/i.test(key))).toEqual(['Path']) expect(env.Path.split(':')[0]).toBe('/tmp/orca-agent-teams-bin') + }) + + it('keeps the daemon `PATH` block when the requested env has no path key', () => { + const proc = mockPtyProcess() + spawnMock.mockReturnValue(proc) + const platform = Object.getOwnPropertyDescriptor(process, 'platform') + + Object.defineProperty(process, 'platform', { value: 'win32' }) + try { + createPtySubprocess({ sessionId: 'test', cols: 80, rows: 24, env: { FOO: 'bar' } }) + } finally { + if (platform) { + Object.defineProperty(process, 'platform', platform) + } + } + + const env = spawnMock.mock.calls.at(-1)![2].env expect(env.PATH).toBe(process.env.PATH) }) diff --git a/src/main/daemon/pty-subprocess.ts b/src/main/daemon/pty-subprocess.ts index 99ba90f13db..8cd3f1b35be 100644 --- a/src/main/daemon/pty-subprocess.ts +++ b/src/main/daemon/pty-subprocess.ts @@ -74,6 +74,7 @@ const PANE_IDENTITY_ENV_KEYS = [ 'ORCA_WORKTREE_ID', 'ORCA_AGENT_LAUNCH_TOKEN' ] as const +const WINDOWS_PATH_ENV_KEY_RE = /^path$/i const FOREGROUND_AGENT_CACHE_TTL_MS = 1000 const SHELL_FOREGROUND_REFRESH_RETRY_MS = 5_000 // Why: a Windows refresh forks a heavy powershell.exe CIM scan (~10-40x POSIX `ps`); idle shells retry slower, output re-arms the fast retry. @@ -162,6 +163,36 @@ function removeUnspecifiedPaneIdentityEnv( } } +/** + * Collapses the duplicate Windows path spelling this daemon's own env merge can mint. + * + * Why: main sends a sparse env patch, so `{...process.env, ...opts.env}` can pair the daemon + * block's spelling with the patch's; a Windows child inheriting both crashes the packaged CLI + * launcher with "Item has already been added. Key in dictionary: 'PATH'" (stablyai/orca#12046). + */ +function collapseWindowsPathEnvKeys( + env: Record, + requestedEnv: Record | undefined +): void { + if (process.platform !== 'win32') { + return + } + const pathKeys = Object.keys(env).filter((key) => WINDOWS_PATH_ENV_KEY_RE.test(key)) + if (pathKeys.length < 2) { + return + } + // Why: main already shaped the patch's spelling (attribution, CLI shims), so its value wins. + const requestedKey = requestedEnv + ? Object.keys(requestedEnv).find((key) => WINDOWS_PATH_ENV_KEY_RE.test(key)) + : undefined + const survivingKey = requestedKey && env[requestedKey] !== undefined ? requestedKey : pathKeys[0] + for (const key of pathKeys) { + if (key !== survivingKey) { + delete env[key] + } + } +} + /** * Promotes the agent-teams shim path ahead of inherited PATH entries. */ @@ -770,8 +801,9 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl ) { addWslEnvKeys(env, [POWERLEVEL10K_WIZARD_DISABLE_ENV]) } - // Why: opts.env arrives from buildPtyHostEnv, which collapses Windows PATH onto a single spelling. const requestedEnv = opts.env + // Why: collapse before promoting so the shim lands on the spelling the child actually inherits. + collapseWindowsPathEnvKeys(env, requestedEnv) const requestedPath = requestedEnv ? requestedEnv[resolvePathEnvKey(requestedEnv, process.platform)] : undefined diff --git a/src/main/pty/windows-environment-path.test.ts b/src/main/pty/windows-environment-path.test.ts index 150d4ebcb11..005accedf6e 100644 --- a/src/main/pty/windows-environment-path.test.ts +++ b/src/main/pty/windows-environment-path.test.ts @@ -310,8 +310,11 @@ describe('resolvePathEnvKey', () => { expect(resolvePathEnvKey({ PATH: 'C:\\Windows' }, 'win32')).toBe('PATH') }) - it('defaults to the OS spelling for a Windows env with no path key', () => { - expect(resolvePathEnvKey({}, 'win32')).toBe('Path') + it('falls back to the host block spelling for a Windows env with no path key', () => { + // Why: a sparse patch that guesses wrong leaves the daemon's own merge holding both keys. + expect(resolvePathEnvKey({}, 'win32', { PATH: 'C:\\Windows' })).toBe('PATH') + expect(resolvePathEnvKey({}, 'win32', { Path: 'C:\\Windows' })).toBe('Path') + expect(resolvePathEnvKey({}, 'win32', {})).toBe('Path') }) it('always resolves `PATH` off Windows so a POSIX `Path` variable is untouched', () => { diff --git a/src/main/pty/windows-environment-path.ts b/src/main/pty/windows-environment-path.ts index f34feac27cf..7464051863d 100644 --- a/src/main/pty/windows-environment-path.ts +++ b/src/main/pty/windows-environment-path.ts @@ -249,12 +249,31 @@ export function __resetPersistedWindowsPathCacheForTests(): void { */ export function resolvePathEnvKey( env: NodeJS.ProcessEnv | Record, - platform: NodeJS.Platform + platform: NodeJS.Platform, + hostEnv: NodeJS.ProcessEnv = process.env ): 'PATH' | 'Path' { if (platform !== 'win32') { return 'PATH' } - return env.Path !== undefined ? 'Path' : env.PATH !== undefined ? 'PATH' : 'Path' + if (env.Path !== undefined) { + return 'Path' + } + if (env.PATH !== undefined) { + return 'PATH' + } + // Why: the daemon receives a sparse env patch and re-merges its own block underneath it; + // matching the block's spelling stops that merge from resurrecting the other one. + return readHostPathEnvKey(hostEnv) +} + +/** + * Reads the path spelling the host's own environment block carries. + * + * Why: Node's Windows `process.env` lookups are case-insensitive, so only the own keys + * reveal which spelling a child process will actually inherit. + */ +function readHostPathEnvKey(hostEnv: NodeJS.ProcessEnv): 'PATH' | 'Path' { + return Object.keys(hostEnv).includes('PATH') ? 'PATH' : 'Path' } function mergeWindowsPathSegments( @@ -263,7 +282,7 @@ function mergeWindowsPathSegments( platform: NodeJS.Platform, sourceEnv: NodeJS.ProcessEnv ): void { - const pathKey = resolvePathEnvKey(env, platform) + const pathKey = resolvePathEnvKey(env, platform, sourceEnv) const pathDelimiter = getPathDelimiter(platform) const currentPath = env[pathKey] ?? sourceEnv.PATH ?? sourceEnv.Path ?? '' const currentSegments = splitPathSegments(currentPath, pathDelimiter)