From d7e596afb3245555b87d20eb3f243da9cf20b0f8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 3 Aug 2026 00:35:52 -0700 Subject: [PATCH] fix(windows): resolve the live PATH spelling by block order, not casing Win32 resolves a duplicated variable by taking the first case-insensitive match in the block, so `resolvePathEnvKey`'s hardcoded `Path`-first preference targeted the shadowed spelling on the reporter's own `["PATH","Path"]` block. Drop the attribution-side collapse with it: it deleted the other spelling's value, and deleting the live key promotes the shadowed one, so an env that stripped down to empty lost both. --- ...rminal-attribution-path-env-casing.test.ts | 50 +++++++++++++++---- src/main/attribution/terminal-attribution.ts | 20 -------- src/main/pty/windows-environment-path.test.ts | 16 ++++-- src/main/pty/windows-environment-path.ts | 27 +++++----- 4 files changed, 68 insertions(+), 45 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 d301ad26cca..95ff6fdc2e3 100644 --- a/src/main/attribution/terminal-attribution-path-env-casing.test.ts +++ b/src/main/attribution/terminal-attribution-path-env-casing.test.ts @@ -48,22 +48,54 @@ describe('applyTerminalAttributionEnv PATH key casing', () => { expect(pathEntries).toContain('C:\\Program Files\\Git\\cmd') }) - it('collapses a dual-cased Windows env onto the OS-resolved `Path`', () => { - const root = makeTmpRoot() - const baseEnv: Record = { - Path: 'C:\\Windows\\system32', - PATH: 'C:\\Stale' + // Why: Win32 resolves a duplicated variable by taking the first match in the block, so the + // shim must land on the first-listed spelling and the shadowed one must keep its value — + // dropping it would promote it the moment anything downstream removes the live key. + it.each([ + { first: 'PATH', second: 'Path' }, + { first: 'Path', second: 'PATH' } + ])( + 'prepends the shim onto the first-listed `$first` and leaves `$second` untouched', + ({ first, second }) => { + const root = makeTmpRoot() + const userDataPath = join(root, 'user-data') + const baseEnv: Record = { + [first]: 'C:\\tools\\bin;C:\\Windows\\system32', + [second]: 'C:\\Windows\\system32' + } + + applyTerminalAttributionEnv(baseEnv, { + enabled: true, + platform: 'win32', + shellFamily: 'native-windows', + userDataPath + }) + + const win32Dir = join(userDataPath, 'orca-terminal-attribution', 'win32') + expect(baseEnv[first].split(';')).toEqual([ + win32Dir, + 'C:\\tools\\bin', + 'C:\\Windows\\system32' + ]) + expect(baseEnv[second]).toBe('C:\\Windows\\system32') } + ) + + it('keeps a shadowed duplicate when the live spelling strips down to nothing', () => { + const root = makeTmpRoot() + const userDataPath = join(root, 'user-data') + const shimDir = join(userDataPath, 'orca-terminal-attribution', 'win32') + const baseEnv: Record = { Path: shimDir, PATH: 'C:\\Windows\\system32' } applyTerminalAttributionEnv(baseEnv, { - enabled: true, + enabled: false, platform: 'win32', shellFamily: 'native-windows', - userDataPath: join(root, 'user-data') + userDataPath }) - expect(pathEnvKeys(baseEnv)).toEqual(['Path']) - expect(baseEnv.Path).toContain('C:\\Windows\\system32') + expect(pathEnvKeys(baseEnv)).toEqual(['PATH']) + expect(baseEnv.PATH).toBe('C:\\Windows\\system32') }) it('strips shim entries from the inherited `Path` when attribution is disabled', () => { diff --git a/src/main/attribution/terminal-attribution.ts b/src/main/attribution/terminal-attribution.ts index bbce8ff97a1..5d12c8289ec 100644 --- a/src/main/attribution/terminal-attribution.ts +++ b/src/main/attribution/terminal-attribution.ts @@ -105,7 +105,6 @@ export function applyTerminalAttributionEnv( // terminal environment instead of mutating global git/gh config or the // user's external shell PATH. baseEnv[pathKey] = [...prependDirs, cleanedBasePath].filter(Boolean).join(pathDelimiter) - collapseWindowsPathEnvKey(baseEnv, pathKey, platform) baseEnv.ORCA_ENABLE_GIT_ATTRIBUTION = '1' baseEnv.ORCA_GIT_COMMIT_TRAILER = ORCA_GIT_COMMIT_TRAILER baseEnv.ORCA_GH_PR_FOOTER = ORCA_GH_FOOTER @@ -141,25 +140,6 @@ function clearTerminalAttributionEnv( } else { delete baseEnv[pathKey] } - collapseWindowsPathEnvKey(baseEnv, pathKey, platform) -} - -/** - * Drops the non-resolved PATH spelling so a Windows child inherits exactly one. - * - * 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, - pathKey: 'PATH' | 'Path', - platform: NodeJS.Platform -): void { - if (platform !== 'win32') { - return - } - delete baseEnv[pathKey === 'Path' ? 'PATH' : 'Path'] } function stripAttributionPathEntries(pathValue: string, pathDelimiter: string): string { diff --git a/src/main/pty/windows-environment-path.test.ts b/src/main/pty/windows-environment-path.test.ts index 005accedf6e..dd7819fcf2d 100644 --- a/src/main/pty/windows-environment-path.test.ts +++ b/src/main/pty/windows-environment-path.test.ts @@ -301,19 +301,27 @@ describe('mergePersistedWindowsPath', () => { }) describe('resolvePathEnvKey', () => { - it('prefers the Windows `Path` spelling already present on the env', () => { + it('uses whichever Windows spelling is present on the env', () => { expect(resolvePathEnvKey({ Path: 'C:\\Windows' }, 'win32')).toBe('Path') - expect(resolvePathEnvKey({ Path: 'C:\\Windows', PATH: 'C:\\Stale' }, 'win32')).toBe('Path') + expect(resolvePathEnvKey({ PATH: 'C:\\Windows' }, 'win32')).toBe('PATH') }) - it('keeps `PATH` on Windows when that is the only spelling present', () => { - expect(resolvePathEnvKey({ PATH: 'C:\\Windows' }, 'win32')).toBe('PATH') + // Why: Win32 returns the first case-insensitive match in the block, so a dual-cased env is + // resolved by position; picking by casing would target the spelling the child never sees. + it('resolves a dual-cased Windows env by block order, not casing', () => { + expect(resolvePathEnvKey({ PATH: 'C:\\Live', Path: 'C:\\Shadowed' }, 'win32')).toBe('PATH') + expect(resolvePathEnvKey({ Path: 'C:\\Live', PATH: 'C:\\Shadowed' }, 'win32')).toBe('Path') + }) + + it('skips a path key explicitly set to undefined', () => { + expect(resolvePathEnvKey({ PATH: undefined, Path: 'C:\\Windows' }, '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', { Path: 'C:\\Live', PATH: 'C:\\Shadowed' })).toBe('Path') expect(resolvePathEnvKey({}, 'win32', {})).toBe('Path') }) diff --git a/src/main/pty/windows-environment-path.ts b/src/main/pty/windows-environment-path.ts index 7464051863d..a64eb7cceae 100644 --- a/src/main/pty/windows-environment-path.ts +++ b/src/main/pty/windows-environment-path.ts @@ -255,25 +255,28 @@ export function resolvePathEnvKey( if (platform !== 'win32') { return '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) + return firstWindowsPathEnvKey(env) ?? firstWindowsPathEnvKey(hostEnv) ?? 'Path' } /** - * Reads the path spelling the host's own environment block carries. + * Reads the path spelling a Windows child would actually resolve out of this block. * - * Why: Node's Windows `process.env` lookups are case-insensitive, so only the own keys - * reveal which spelling a child process will actually inherit. + * Why: `RtlQueryEnvironmentVariable` scans the block linearly and returns the first + * case-insensitive match, so position — not casing — decides the winner. Object key order is + * that block order: Node enumerates `process.env` in block order and node-pty emits + * `Object.keys(env)` verbatim without sorting. */ -function readHostPathEnvKey(hostEnv: NodeJS.ProcessEnv): 'PATH' | 'Path' { - return Object.keys(hostEnv).includes('PATH') ? 'PATH' : 'Path' +function firstWindowsPathEnvKey( + env: NodeJS.ProcessEnv | Record +): 'PATH' | 'Path' | undefined { + for (const key of Object.keys(env)) { + if ((key === 'PATH' || key === 'Path') && env[key] !== undefined) { + return key + } + } + return undefined } function mergeWindowsPathSegments(