From 0ce2e6c6e3687b87ac7d3b06bab2b6d539f4db22 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 23:08:24 -0700 Subject: [PATCH] fix(terminal): stop the shim PATH scrub deleting a legitimate directory The previous round collapsed '..' lexically before classifying a PATH entry. That is not the same as resolving it: when /posix is a symlink, /posix/../posix lands elsewhere, so a legitimate directory was classified as the shim and removed, leaving git unresolvable. Reproduced with a symlinked shim/posix and a real git behind it. Resolving for real is not available here either -- this env is also built for remote and WSL panes whose paths name nothing on the local filesystem -- so the classifier is lexical again, deliberately. A '..' spelling that slips through costs nothing at runtime: that directory holds the pass-through tombstone, and the tombstone excludes its own directory by -ef, so the lookup still reaches the real git. Separately, pathEntrySpellings can only enumerate one added separator, so a captured directory spelled with two or more survived the literal removal. The split filter now also compares separator-stripped forms, which covers any number. Both changes are mutation-tested. Co-authored-by: Orca --- src/main/pty/legacy-terminal-shim-dir.test.ts | 35 +++++++++++---- src/main/pty/legacy-terminal-shim-dir.ts | 45 ++++++++++++++----- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/src/main/pty/legacy-terminal-shim-dir.test.ts b/src/main/pty/legacy-terminal-shim-dir.test.ts index c8e986e1e92..f6d2cefb874 100644 --- a/src/main/pty/legacy-terminal-shim-dir.test.ts +++ b/src/main/pty/legacy-terminal-shim-dir.test.ts @@ -332,21 +332,38 @@ describe('legacy terminal shim neutralization', () => { } }) - it('treats a shim directory reached through .. as a shim PATH entry', () => { - // Why: a plain suffix test missed `/orca-terminal-attribution/posix/../posix`, so the - // entry survived the scrub and the shim stayed reachable. + it('does not collapse .. when classifying a shim PATH entry', () => { + // Why deliberately not collapsed: collapsing `..` textually is not resolving it. If + // `/posix` is a symlink, `/posix/../posix` lands somewhere else, and classifying + // it as the shim directory deletes a legitimate PATH entry and leaves git unresolvable. + // Resolving for real is not available here either: this env is also built for remote and WSL + // panes whose paths do not exist on the local filesystem. expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/posix/../posix')).toBe( - true - ) - expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/win32/../win32')).toBe( - true - ) - expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/posix/../other')).toBe( false ) + expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/posix')).toBe(true) + expect(isLegacyTerminalShimPathEntry('/tmp/old/orca-terminal-attribution/win32//')).toBe(true) expect(isLegacyTerminalShimPathEntry('/usr/local/bin')).toBe(false) }) + it('strips a captured shim directory spelled with repeated separators', () => { + // Why: pathEntrySpellings can only enumerate one added separator, so an entry repeating it + // survived the literal removal and kept the captured directory on PATH. + const posixEnv = { + PATH: '/custom/elsewhere///:/usr/bin', + ORCA_ATTRIBUTION_SHIM_DIR: '/custom/elsewhere' + } + stripLegacyTerminalShimEnv(posixEnv, 'linux') + expect(posixEnv.PATH).toBe('/usr/bin') + + const windowsEnv = { + Path: 'C:\\Custom\\Else\\\\;C:\\Windows', + ORCA_ATTRIBUTION_SHIM_DIR: 'C:\\Custom\\Else' + } + stripLegacyTerminalShimEnv(windowsEnv, 'win32') + expect(windowsEnv.Path).toBe('C:\\Windows') + }) + itOnPosix('does not fall back to the cwd when every PATH entry is filtered out', () => { // Why: with the shim dir as the only entry the cleaned PATH is empty; without the // path_entry_kept guard the lookup runs against that empty PATH and finds a cwd-local git. diff --git a/src/main/pty/legacy-terminal-shim-dir.ts b/src/main/pty/legacy-terminal-shim-dir.ts index 02b2b8dc5d0..5bd15783ba5 100644 --- a/src/main/pty/legacy-terminal-shim-dir.ts +++ b/src/main/pty/legacy-terminal-shim-dir.ts @@ -1,5 +1,5 @@ import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, writeFileSync } from 'node:fs' -import { join, normalize } from 'node:path' +import { join } from 'node:path' import { readVerifiedShebangInterpreter, renderLegacyTerminalPosixTombstone, @@ -146,7 +146,7 @@ function writeFileAtomically(filePath: string, contents: string, mode: number): // literal compare itself has to stay: a shim path may contain the PATH delimiter, which splitting // would fragment. function pathEntrySpellings(dir: string, windows: boolean): string[] { - const base = dir.replace(/[\\/]+$/, '') + const base = stripTrailingSeparators(dir) if (!base) { return [dir] } @@ -157,20 +157,40 @@ function pathEntrySpellings(dir: string, windows: boolean): string[] { return [...spellings] } +// Why purely lexical, and why `..` is deliberately not collapsed: collapsing it textually is not +// the same as resolving it. If `/posix` is a symlink, `/posix/../posix` resolves +// somewhere else entirely, and treating it as the shim directory deletes a legitimate PATH entry +// and leaves git unresolvable. Resolving for real is not an option either: this env is also built +// for remote and WSL panes, where these paths do not name anything on the local filesystem. +// A `..` spelling that does slip through costs nothing at runtime -- the directory now holds the +// pass-through tombstone, and the tombstone excludes its own directory by -ef, so the lookup still +// reaches the real git. export function isLegacyTerminalShimPathEntry(entry: string): boolean { - // Why normalize first: a `.../orca-terminal-attribution/posix/../posix` spelling names the shim - // directory but fails a plain suffix test, so the entry survived the scrub and the shim stayed - // reachable. normalize collapses `..` textually, which is what a suffix test needs. - const normalized = normalize(entry.replaceAll('\\', '/')) - .replaceAll('\\', '/') - .replace(/\/+$/, '') - .toLowerCase() + const normalized = stripTrailingSeparators(entry.replaceAll('\\', '/')).toLowerCase() return ( normalized.endsWith(`/${LEGACY_SHIM_ROOT_DIR}/posix`) || normalized.endsWith(`/${LEGACY_SHIM_ROOT_DIR}/win32`) ) } +// Why: `pathEntrySpellings` can only enumerate one added separator, so an entry repeating it +// survived the literal removal. Comparing separator-stripped forms covers any number of them. +function stripTrailingSeparators(value: string): string { + return value.replace(/[\\/]+$/, '') +} + +function namesCapturedShimDir(entry: string, shimDirs: string[], windows: boolean): boolean { + const candidate = stripTrailingSeparators(entry) + return shimDirs.some((shimDir) => { + const target = stripTrailingSeparators(shimDir) + return target + ? windows + ? candidate.toLowerCase() === target.toLowerCase() + : candidate === target + : false + }) +} + export function stripLegacyTerminalShimEnv( env: Record, platform: NodeJS.Platform = process.platform @@ -210,7 +230,12 @@ export function stripLegacyTerminalShimEnv( ) const cleaned = withoutExplicitDirs .split(delimiter) - .filter((entry) => entry && !isLegacyTerminalShimPathEntry(entry)) + .filter( + (entry) => + entry && + !isLegacyTerminalShimPathEntry(entry) && + !namesCapturedShimDir(entry, explicitShimDirs, windows) + ) .join(delimiter) if (cleaned) { env[pathKey] = cleaned