From e4de5d93e200e0300dfe46c5c483667eff0db247 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 20:44:20 -0700 Subject: [PATCH] fix(terminal): strip the captured shim dir across trailing-separator spellings The scrub compared the captured ORCA_ATTRIBUTION_SHIM_DIR to PATH entries literally, so a trailing-separator difference left the legacy shim directory on the spawned PATH. Same class already fixed in the generated wrappers. Also drops a dead default in the POSIX filter, consolidates comments that had accumulated across fixes, and normalizes the legacy directory once rather than per PATH entry in the cmd wrapper. The boundary scanner stays: a shim path can contain the PATH delimiter, which splitting would fragment. --- .../pty/legacy-terminal-posix-tombstone.ts | 19 +++++++--------- src/main/pty/legacy-terminal-shim-dir.test.ts | 19 ++++++++++++++++ src/main/pty/legacy-terminal-shim-dir.ts | 22 ++++++++++++++++++- .../pty/legacy-terminal-windows-tombstone.ts | 6 +++-- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/main/pty/legacy-terminal-posix-tombstone.ts b/src/main/pty/legacy-terminal-posix-tombstone.ts index abe82ac485e..7c910b82217 100644 --- a/src/main/pty/legacy-terminal-posix-tombstone.ts +++ b/src/main/pty/legacy-terminal-posix-tombstone.ts @@ -73,16 +73,14 @@ const POSIX_TOMBSTONE = String.raw`#!__ORCA_INTERPRETER__ set -u command_name="__ORCA_COMMAND__" -# Why: parameter expansion, not the external dirname — if that were unresolvable the substitution -# would be empty and cd into it succeeds, silently making wrapper_dir the cwd so the wrapper -# fails to exclude itself from PATH. +# Why this shape, rather than dirname: an unresolvable external would leave the substitution empty +# and cd into it succeeds, silently making wrapper_dir the cwd. With no slash the %/* strip yields the +# file name rather than a directory, so that case takes $PWD. CDPATH is cleared because cd searches +# it for a relative operand and echoes where it landed, which the substitution would capture. +# Each of these made self-exclusion miss the wrapper's own directory, so the lookup resolved back +# to this script. wrapper_src="${SHELL_DOLLAR}{BASH_SOURCE[0]}" case "$wrapper_src" in - # Why: with no slash the %/* strip yields the file name, not a directory, so self-exclusion - # would miss the wrapper's own dir and the lookup would resolve back to this script. - # Why: with CDPATH set, cd searches it for a relative operand and echoes the directory it lands - # in, which the command substitution then captures — wrapper_dir ends up wrong and doubled. - # Clear it for this one command. */*) wrapper_dir="$(CDPATH= cd -P -- "${SHELL_DOLLAR}{wrapper_src%/*}" 2>/dev/null && pwd)" ;; *) wrapper_dir="$PWD" ;; esac @@ -99,7 +97,7 @@ filter_path() { local filtered_path="" local separator="" path_entry_kept=0 - local entry normalized candidate has_more + local entry normalized has_more while true; do if [[ "$remaining" == *:* ]]; then entry="${SHELL_DOLLAR}{remaining%%:*}" @@ -113,14 +111,13 @@ filter_path() { while [[ "$normalized" != "/" && "$normalized" == */ ]]; do normalized="${SHELL_DOLLAR}{normalized%/}" done - candidate="${SHELL_DOLLAR}{entry:-.}" if [[ "$entry" != /* ]]; then # Why: an empty or relative PATH element resolves against the current directory, so keeping # it would let a repository-local git/gh win the lookup below. : elif [[ -n "$legacy_target" && "$normalized" == "$legacy_target" ]]; then : - elif [[ "$candidate" -ef "$wrapper_dir" ]]; then + elif [[ "$entry" -ef "$wrapper_dir" ]]; then : else filtered_path+="$separator$entry" diff --git a/src/main/pty/legacy-terminal-shim-dir.test.ts b/src/main/pty/legacy-terminal-shim-dir.test.ts index 47a3b96825b..442dd77a6dc 100644 --- a/src/main/pty/legacy-terminal-shim-dir.test.ts +++ b/src/main/pty/legacy-terminal-shim-dir.test.ts @@ -707,6 +707,25 @@ describe('legacy terminal shim neutralization', () => { expect(env).toEqual({ PATH: '/usr/local/bin:/usr/bin', HOME: '/home/u' }) }) + it('strips the captured shim directory when PATH spells it with a trailing separator', () => { + // Why: the captured value and the PATH entry can differ by a trailing separator; comparing + // them literally left the shim directory on PATH and the wrapper reachable. + const posix: Record = { + PATH: '/custom/elsewhere/:/usr/bin', + ORCA_ATTRIBUTION_SHIM_DIR: '/custom/elsewhere' + } + stripLegacyTerminalShimEnv(posix, 'linux') + expect(posix.PATH).toBe('/usr/bin') + + // And the reverse spelling, plus Windows slash style. + const win: Record = { + Path: 'C:\\Custom\\Else;C:\\Windows', + ORCA_ATTRIBUTION_SHIM_DIR: 'C:\\Custom\\Else\\' + } + stripLegacyTerminalShimEnv(win, 'win32') + expect(win.Path).toBe('C:\\Windows') + }) + it('uses the captured POSIX shim directory literally when it contains a colon', () => { const shimDir = '/tmp/orca:user/orca-terminal-attribution/posix' const env: Record = { diff --git a/src/main/pty/legacy-terminal-shim-dir.ts b/src/main/pty/legacy-terminal-shim-dir.ts index 0d380bcfd65..128d88c1e9c 100644 --- a/src/main/pty/legacy-terminal-shim-dir.ts +++ b/src/main/pty/legacy-terminal-shim-dir.ts @@ -126,6 +126,22 @@ function writeFileAtomically(filePath: string, contents: string, mode: number): } } +// Why: the captured directory and the PATH entry naming it can differ by a trailing separator (or +// its slash style on Windows). A literal compare missed that and left the shim dir on PATH. The +// 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(/[\\/]+$/, '') + if (!base) { + return [dir] + } + const spellings = new Set([dir, base, `${base}/`]) + if (windows) { + spellings.add(`${base}\\`) + } + return [...spellings] +} + export function isLegacyTerminalShimPathEntry(entry: string): boolean { const normalized = entry.replaceAll('\\', '/').replace(/\/+$/, '').toLowerCase() return ( @@ -164,7 +180,11 @@ export function stripLegacyTerminalShimEnv( continue } const withoutExplicitDirs = explicitShimDirs.reduce( - (pathValue, shimDir) => removeLiteralPathEntry(pathValue, shimDir, delimiter, windows), + (pathValue, shimDir) => + pathEntrySpellings(shimDir, windows).reduce( + (value, spelling) => removeLiteralPathEntry(value, spelling, delimiter, windows), + pathValue + ), current ) const cleaned = withoutExplicitDirs diff --git a/src/main/pty/legacy-terminal-windows-tombstone.ts b/src/main/pty/legacy-terminal-windows-tombstone.ts index 8bff863ec18..d482d1479f2 100644 --- a/src/main/pty/legacy-terminal-windows-tombstone.ts +++ b/src/main/pty/legacy-terminal-windows-tombstone.ts @@ -6,6 +6,10 @@ setlocal set "orca_real=%ORCA_REAL___ORCA_UPPER_COMMAND__%" set "orca_wrapper_dir=%~dp0" set "orca_legacy_wrapper_dir=%ORCA_ATTRIBUTION_SHIM_DIR%" +rem Why: normalize once here rather than per PATH entry; the value cannot change mid-run. +set "orca_legacy_norm=" +if defined orca_legacy_wrapper_dir for %%G in ("%orca_legacy_wrapper_dir%") do set "orca_legacy_norm=%%~fG" +if defined orca_legacy_norm if "%orca_legacy_norm:~-1%"=="%orca_sep%" set "orca_legacy_norm=%orca_legacy_norm:~0,-1%" set "orca_clean_path=" rem Why: holds a single separator so the trailing-separator tests below need neither a literal rem backslash before a quote (which breaks cmd parsing) nor a sentinel character (which would @@ -80,8 +84,6 @@ if defined orca_clean_path (set "orca_clean_path=%orca_clean_path%;%~1") else se exit /b :orca_reject_legacy_dir -for %%G in ("%orca_legacy_wrapper_dir%") do set "orca_legacy_norm=%%~fG" -if "%orca_legacy_norm:~-1%"=="%orca_sep%" set "orca_legacy_norm=%orca_legacy_norm:~0,-1%" if /I "%orca_path_entry_dir%"=="%orca_legacy_norm%\" set "orca_skip_entry=1" exit /b `