From 232e398c35335d93cc3cb6130baad8de834b691d Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 13 Aug 2026 14:33:17 -0700 Subject: [PATCH 1/4] Fix paired remote terminal browser links STA-4181 --- .../lib/workspace-browser-tab-open.test.ts | 38 +++++++++++++++++++ .../src/lib/workspace-browser-tab-open.ts | 6 ++- .../src/runtime/web-runtime-session.ts | 3 +- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/renderer/src/lib/workspace-browser-tab-open.test.ts b/src/renderer/src/lib/workspace-browser-tab-open.test.ts index 936bed7bb2c..5d28024597d 100644 --- a/src/renderer/src/lib/workspace-browser-tab-open.test.ts +++ b/src/renderer/src/lib/workspace-browser-tab-open.test.ts @@ -6,6 +6,7 @@ import { } from '../../../shared/execution-host' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' import { BROWSER_SCREENCAST_RUNTIME_CAPABILITY } from '../../../shared/protocol-version' +import * as clientCreationActionPolicy from './client-creation-action-policy' import { canOpenWorkspaceBrowserTabOnRuntime, openWorkspaceBrowserTab @@ -131,6 +132,43 @@ describe('openWorkspaceBrowserTab', () => { expect(createBrowserTab).not.toHaveBeenCalled() }) + it('keeps an asserted paired-runtime link on the runtime when client policy selects local', async () => { + const createBrowserTab = vi.fn() + mocks.state = { + ...ownerState(toRuntimeExecutionHostId('hub-a')), + ...browserCapableRuntime('hub-a'), + createBrowserTab, + defaultBrowserSessionProfileId: 'client-profile', + defaultBrowserSessionProfileIdByHostId: {} + } + const policySpy = vi + .spyOn(clientCreationActionPolicy, 'getClientCreationActionPolicy') + .mockReturnValue({ + 'managed-browser': { state: 'enabled', provider: 'local-client' }, + 'mobile-emulator': { state: 'enabled', provider: 'local-client' } + }) + + try { + await openWorkspaceBrowserTab({ + workspaceId: WORKSPACE_ID, + url: 'https://example.com/pinned', + intent: { kind: 'url' }, + expectedRuntimeEnvironmentId: 'hub-a' + }) + + expect(mocks.createRemote).toHaveBeenCalledWith( + expect.objectContaining({ + environmentId: 'hub-a', + waitForRegistration: true, + worktreeId: WORKSPACE_ID + }) + ) + expect(createBrowserTab).not.toHaveBeenCalled() + } finally { + policySpy.mockRestore() + } + }) + it('fails closed when the workspace route swaps away from the pane runtime before opening', async () => { mocks.state = { ...ownerState(toRuntimeExecutionHostId('hub-a')), diff --git a/src/renderer/src/lib/workspace-browser-tab-open.ts b/src/renderer/src/lib/workspace-browser-tab-open.ts index cc6adbb5669..fe5f7b21217 100644 --- a/src/renderer/src/lib/workspace-browser-tab-open.ts +++ b/src/renderer/src/lib/workspace-browser-tab-open.ts @@ -212,7 +212,9 @@ export async function openWorkspaceBrowserTab( `host ${route.executionHostId} does not own runtime ${environmentId}` ) } - if (availability.provider === 'local-client') { + // A terminal link is pinned to its source runtime after the assertion above; + // provider policy may describe the client surface rather than that owner. + if (expectedEnvironmentId === null && availability.provider === 'local-client') { const localHostId = host && host.kind !== 'runtime' ? host.id : LOCAL_EXECUTION_HOST_ID createClientBrowserTab(state, request, localHostId, presentation) return @@ -224,6 +226,8 @@ export async function openWorkspaceBrowserTab( environmentId, url: request.url, targetGroupId: request.targetGroupId, + // Pinned terminal links must wait for the host tab to publish before reconciling the client. + ...(expectedEnvironmentId !== null ? { waitForRegistration: true } : {}), // Why: the tab is opened from this workspace's tab bar, so surface that // workspace — otherwise a background worktree looks like nothing happened. selectWorktree: true, diff --git a/src/renderer/src/runtime/web-runtime-session.ts b/src/renderer/src/runtime/web-runtime-session.ts index 34dcfae1d3a..b4818a36afe 100644 --- a/src/renderer/src/runtime/web-runtime-session.ts +++ b/src/renderer/src/runtime/web-runtime-session.ts @@ -463,6 +463,7 @@ export async function createWebRuntimeSessionBrowserTab(args: { clientTargetGroupId?: string clientTargetGroupCreated?: boolean focusOnCreate?: boolean + waitForRegistration?: boolean selectWorktree?: boolean stagedTitle?: string stagedFocusAddressBar?: boolean @@ -549,7 +550,7 @@ export async function createWebRuntimeSessionBrowserTab(args: { // Why: place the new browser in the clicked split group so the host snapshot is authoritative for it (no left-snap). ...(args.targetGroupId ? { targetGroupId: args.targetGroupId } : {}), // Why: web clients need the local tab now; waiting for host webview registration makes the workspace appear to close. - waitForRegistration: false + waitForRegistration: args.waitForRegistration ?? false }, timeoutMs: 15_000 })) as RuntimeRpcResponse From 2f85dda1830aae404beabe147649f43f8e992d80 Mon Sep 17 00:00:00 2001 From: m4air Date: Thu, 13 Aug 2026 14:56:50 -0700 Subject: [PATCH 2/4] Narrow paired browser link fix to registration race --- .../lib/workspace-browser-tab-open.test.ts | 40 +++++++------------ .../src/lib/workspace-browser-tab-open.ts | 6 +-- .../src/runtime/web-runtime-session.ts | 1 + 3 files changed, 17 insertions(+), 30 deletions(-) diff --git a/src/renderer/src/lib/workspace-browser-tab-open.test.ts b/src/renderer/src/lib/workspace-browser-tab-open.test.ts index 5d28024597d..3e8b93a6726 100644 --- a/src/renderer/src/lib/workspace-browser-tab-open.test.ts +++ b/src/renderer/src/lib/workspace-browser-tab-open.test.ts @@ -6,7 +6,6 @@ import { } from '../../../shared/execution-host' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' import { BROWSER_SCREENCAST_RUNTIME_CAPABILITY } from '../../../shared/protocol-version' -import * as clientCreationActionPolicy from './client-creation-action-policy' import { canOpenWorkspaceBrowserTabOnRuntime, openWorkspaceBrowserTab @@ -132,7 +131,7 @@ describe('openWorkspaceBrowserTab', () => { expect(createBrowserTab).not.toHaveBeenCalled() }) - it('keeps an asserted paired-runtime link on the runtime when client policy selects local', async () => { + it('waits for host registration before reconciling an asserted runtime link', async () => { const createBrowserTab = vi.fn() mocks.state = { ...ownerState(toRuntimeExecutionHostId('hub-a')), @@ -141,32 +140,21 @@ describe('openWorkspaceBrowserTab', () => { defaultBrowserSessionProfileId: 'client-profile', defaultBrowserSessionProfileIdByHostId: {} } - const policySpy = vi - .spyOn(clientCreationActionPolicy, 'getClientCreationActionPolicy') - .mockReturnValue({ - 'managed-browser': { state: 'enabled', provider: 'local-client' }, - 'mobile-emulator': { state: 'enabled', provider: 'local-client' } - }) + await openWorkspaceBrowserTab({ + workspaceId: WORKSPACE_ID, + url: 'https://example.com/pinned', + intent: { kind: 'url' }, + expectedRuntimeEnvironmentId: 'hub-a' + }) - try { - await openWorkspaceBrowserTab({ - workspaceId: WORKSPACE_ID, - url: 'https://example.com/pinned', - intent: { kind: 'url' }, - expectedRuntimeEnvironmentId: 'hub-a' + expect(mocks.createRemote).toHaveBeenCalledWith( + expect.objectContaining({ + environmentId: 'hub-a', + waitForRegistration: true, + worktreeId: WORKSPACE_ID }) - - expect(mocks.createRemote).toHaveBeenCalledWith( - expect.objectContaining({ - environmentId: 'hub-a', - waitForRegistration: true, - worktreeId: WORKSPACE_ID - }) - ) - expect(createBrowserTab).not.toHaveBeenCalled() - } finally { - policySpy.mockRestore() - } + ) + expect(createBrowserTab).not.toHaveBeenCalled() }) it('fails closed when the workspace route swaps away from the pane runtime before opening', async () => { diff --git a/src/renderer/src/lib/workspace-browser-tab-open.ts b/src/renderer/src/lib/workspace-browser-tab-open.ts index fe5f7b21217..52416b75739 100644 --- a/src/renderer/src/lib/workspace-browser-tab-open.ts +++ b/src/renderer/src/lib/workspace-browser-tab-open.ts @@ -212,9 +212,7 @@ export async function openWorkspaceBrowserTab( `host ${route.executionHostId} does not own runtime ${environmentId}` ) } - // A terminal link is pinned to its source runtime after the assertion above; - // provider policy may describe the client surface rather than that owner. - if (expectedEnvironmentId === null && availability.provider === 'local-client') { + if (availability.provider === 'local-client') { const localHostId = host && host.kind !== 'runtime' ? host.id : LOCAL_EXECUTION_HOST_ID createClientBrowserTab(state, request, localHostId, presentation) return @@ -226,7 +224,7 @@ export async function openWorkspaceBrowserTab( environmentId, url: request.url, targetGroupId: request.targetGroupId, - // Pinned terminal links must wait for the host tab to publish before reconciling the client. + // Owner-pinned links need the host tab published before client reconciliation. ...(expectedEnvironmentId !== null ? { waitForRegistration: true } : {}), // Why: the tab is opened from this workspace's tab bar, so surface that // workspace — otherwise a background worktree looks like nothing happened. diff --git a/src/renderer/src/runtime/web-runtime-session.ts b/src/renderer/src/runtime/web-runtime-session.ts index b4818a36afe..164edbed4b7 100644 --- a/src/renderer/src/runtime/web-runtime-session.ts +++ b/src/renderer/src/runtime/web-runtime-session.ts @@ -463,6 +463,7 @@ export async function createWebRuntimeSessionBrowserTab(args: { clientTargetGroupId?: string clientTargetGroupCreated?: boolean focusOnCreate?: boolean + /** Wait until a renderer-backed host can publish the new page in its session snapshot. */ waitForRegistration?: boolean selectWorktree?: boolean stagedTitle?: string From a27d046e5eedca1c49ad022a3abc9cb3b561b74e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:07:49 -0700 Subject: [PATCH 3/4] fix(terminal): reject cwd-resolving PATH entries and harden the legacy wrappers (#14370) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): guard cmd wrapper PATH walks against an empty variable An empty PATH or cleaned PATH left the substitution with an unbalanced quote, desynchronizing cmd parsing so the not-found branch emitted a parse error instead of its message. Reproduced and fixed on real Windows. * fix(terminal): reject relative PATH entries and drop the dirname dependency Adversarial review found the STA-4169 fix incomplete: it dropped empty PATH elements but kept relative ones, which resolve against the current directory identically. A repo-local git was still executed via PATH=. or node_modules/.bin, reproduced in all three wrappers. Also removes the external dirname call in the POSIX wrapper (an unresolvable dirname silently made wrapper_dir the cwd, so the wrapper failed to exclude itself and reported git missing while git was on PATH), compares against the cached wrapper dir in the cmd subroutine where %~dp0 is rebound by CALL, and probes .cmd after .exe so a non-.exe git is still found. * fix(terminal): test rooted paths in pure batch, not via an external tool The rooted-path guard shelled out to findstr, which cmd resolves from the current directory first — so a repository-local findstr could run, and a malicious one could report success for every entry and defeat the guard entirely. Same hijack the guard exists to prevent. Now a pure-batch substring test with no external process. Splits the Windows wrapper templates into their own module to stay under max-lines without a suppression. * fix(terminal): reject drive-relative PATH entries and fix no-slash wrapper dir Round-2 review: PowerShell IsPathRooted accepts drive-relative C:foo, which still resolves against the current directory on that drive. IsPathFullyQualified is absent on Windows PowerShell 5.1 (verified 5.1 on the test host), so match the same prefixes the cmd wrapper accepts. The POSIX %/* strip yields the file name when the path has no slash, so wrapper_dir became the name, self-exclusion missed the shim dir, and the lookup resolved back to the wrapper (spurious 127). Handled with a case split. Also probes .bat, since the replaced lookup honored PATHEXT. --- .../pty/legacy-terminal-posix-tombstone.ts | 18 ++- src/main/pty/legacy-terminal-shim-dir.test.ts | 80 ++++++++++- src/main/pty/legacy-terminal-shim-dir.ts | 102 +------------- .../pty/legacy-terminal-windows-tombstone.ts | 133 ++++++++++++++++++ 4 files changed, 231 insertions(+), 102 deletions(-) create mode 100644 src/main/pty/legacy-terminal-windows-tombstone.ts diff --git a/src/main/pty/legacy-terminal-posix-tombstone.ts b/src/main/pty/legacy-terminal-posix-tombstone.ts index bf575d3bc8b..7fd4a677b17 100644 --- a/src/main/pty/legacy-terminal-posix-tombstone.ts +++ b/src/main/pty/legacy-terminal-posix-tombstone.ts @@ -4,7 +4,17 @@ const POSIX_TOMBSTONE = String.raw`#!/usr/bin/env bash set -u command_name="__ORCA_COMMAND__" -wrapper_dir="$(cd -- "$(dirname -- "${SHELL_DOLLAR}{BASH_SOURCE[0]}")" && pwd)" +# 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. +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. + */*) wrapper_dir="$(cd -- "${SHELL_DOLLAR}{wrapper_src%/*}" 2>/dev/null && pwd)" ;; + *) wrapper_dir="$PWD" ;; +esac +[[ -n "$wrapper_dir" ]] || wrapper_dir="$PWD" legacy_wrapper_dir="${SHELL_DOLLAR}{ORCA_ATTRIBUTION_SHIM_DIR:-}" cleaned_path="${SHELL_DOLLAR}{PATH:-}" @@ -32,9 +42,9 @@ filter_path() { normalized="${SHELL_DOLLAR}{normalized%/}" done candidate="${SHELL_DOLLAR}{entry:-.}" - if [[ -z "$entry" ]]; then - # Why: an empty PATH element means the current directory, so keeping it would let a - # repository-local git/gh win the lookup below. + 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 : diff --git a/src/main/pty/legacy-terminal-shim-dir.test.ts b/src/main/pty/legacy-terminal-shim-dir.test.ts index 3bd489817a1..64c4436b2df 100644 --- a/src/main/pty/legacy-terminal-shim-dir.test.ts +++ b/src/main/pty/legacy-terminal-shim-dir.test.ts @@ -113,16 +113,58 @@ describe('legacy terminal shim neutralization', () => { expect(cmd).not.toContain('where.exe') expect(cmd).toContain('for %%P in ("%orca_clean_path:;=" "%") do call :orca_try_candidate') expect(cmd).toContain(`if exist "%%~fG\\${command}.exe"`) - // A candidate inside the wrapper directory must still be rejected. - expect(cmd).toContain('if /I not "%%~fG\\"=="%~dp0"') + // A candidate inside the wrapper directory must still be rejected, compared against the + // cached wrapper dir because %~dp0 is rebound inside a CALL. + expect(cmd).toContain('if /I not "%%~fG\\"=="%orca_wrapper_dir%"') + // Relative entries resolve against the cwd, so they must be rejected like empty ones. + // Why: the rooted-path test must not shell out — an external tool would itself be + // resolved from the cwd, reintroducing the hijack. + expect(cmd).not.toContain('findstr') + expect(cmd).toContain('if "%orca_candidate:~1,2%"==":\\" goto orca_candidate_rooted') + expect(cmd).toContain('if "%orca_candidate:~0,2%"=="\\\\" goto orca_candidate_rooted') + // Why: these two guards are the only thing stopping an empty element reaching the cwd on + // Windows; deleting either left every other assertion green. + expect(cmd).toContain('if "%~1"=="" exit /b') const powershell = readFileSync(join(win32Dir, `${command}-wrapper.ps1`), 'utf8') expect(powershell).not.toContain('Get-Command') expect(powershell).toContain("($env:PATH -split ';')") + // Why: the rooted check must reject drive-relative 'C:foo', which still resolves against + // the cwd — so the IsPathRooted call must be gone, replaced by an explicit prefix match. + expect(powershell).not.toContain('[IO.Path]::IsPathRooted(') + expect(powershell).toContain("-notmatch '^([A-Za-z]:") + expect(powershell).toContain('if (-not $dir) { continue }') expect(powershell).toContain('Test-Path -LiteralPath $candidate -PathType Leaf') } }) + it('guards both cmd PATH walks so an empty variable cannot break parsing', () => { + // Why: `%VAR:;=" "%` on an empty variable leaves an unbalanced quote that desynchronizes + // cmd parsing for the rest of the file, turning the not-found branch into + // `1>&2 was unexpected at this time.` Reproduced on Windows before this guard. + const userData = makeUserDataDir() + const win32Dir = join(userData, 'orca-terminal-attribution', 'win32') + mkdirSync(win32Dir, { recursive: true }) + + neutralizeLegacyTerminalShimDir(userData) + + for (const command of ['git', 'gh'] as const) { + const cmd = readFileSync(join(win32Dir, `${command}.cmd`), 'utf8') + expect(cmd).toContain('if not defined PATH goto :orca_path_walked') + expect(cmd).toContain('if not defined orca_clean_path goto :orca_candidates_walked') + for (const [guard, loop] of [ + ['if not defined PATH goto :orca_path_walked', 'for %%P in ("%PATH:;='], + [ + 'if not defined orca_clean_path goto :orca_candidates_walked', + 'for %%P in ("%orca_clean_path:;=' + ] + ] as const) { + expect(cmd.indexOf(guard)).toBeGreaterThan(-1) + expect(cmd.indexOf(guard)).toBeLessThan(cmd.indexOf(loop)) + } + } + }) + itOnPosix('does not let an empty PATH element resolve the command from the cwd', async () => { // Why (STA-4169): an empty PATH element means the current directory on POSIX. const userData = makeUserDataDir() @@ -141,6 +183,13 @@ describe('legacy terminal shim neutralization', () => { writeFileSync(join(hostile, 'git'), "#!/usr/bin/env bash\nprintf 'HOSTILE\\n'\n", { mode: 0o755 }) + // Reachable only through a relative PATH entry resolved against the hostile cwd. + mkdirSync(join(hostile, 'node_modules', '.bin'), { recursive: true }) + writeFileSync( + join(hostile, 'node_modules', '.bin', 'git'), + "#!/usr/bin/env bash\nprintf 'HOSTILE\\n'\n", + { mode: 0o755 } + ) const result = spawnSync(join(shimDir, 'git'), ['--version'], { cwd: hostile, @@ -152,6 +201,33 @@ describe('legacy terminal shim neutralization', () => { expect(result.stdout).toContain('REAL') expect(result.stdout).not.toContain('HOSTILE') + + // Why: every spelling that means "current directory" must lose — a leading/trailing empty + // element, and any relative entry (`.`, or a repo-local bin dir). + for (const cwdSpelling of [ + `:${realBin}:/usr/bin:/bin`, + `${realBin}:/usr/bin:/bin:`, + `.:${realBin}:/usr/bin:/bin`, + `..:${realBin}:/usr/bin:/bin`, + `node_modules/.bin:${realBin}:/usr/bin:/bin` + ]) { + const run = spawnSync(join(shimDir, 'git'), ['--version'], { + cwd: hostile, + env: { ...process.env, PATH: cwdSpelling }, + encoding: 'utf8' + }) + expect(run.stdout, `PATH=${cwdSpelling}`).toContain('REAL') + expect(run.stdout, `PATH=${cwdSpelling}`).not.toContain('HOSTILE') + } + + // Why: invoked with no slash in $0, `${BASH_SOURCE%/*}` yields the file name rather than a + // directory, so self-exclusion missed the shim dir and the lookup resolved back to itself. + const noSlash = spawnSync('bash', ['git', '--version'], { + cwd: shimDir, + env: { ...process.env, PATH: `${shimDir}:${realBin}:/usr/bin:/bin` }, + encoding: 'utf8' + }) + expect(noSlash.stdout).toContain('REAL') }) it('removes every Windows PATH occurrence of both captured wrapper directories', () => { diff --git a/src/main/pty/legacy-terminal-shim-dir.ts b/src/main/pty/legacy-terminal-shim-dir.ts index abeeb7a7263..0d380bcfd65 100644 --- a/src/main/pty/legacy-terminal-shim-dir.ts +++ b/src/main/pty/legacy-terminal-shim-dir.ts @@ -1,6 +1,10 @@ import { chmodSync, existsSync, mkdirSync, renameSync, rmSync, writeFileSync } from 'node:fs' import { join } from 'node:path' import { renderLegacyTerminalPosixTombstone } from './legacy-terminal-posix-tombstone' +import { + renderLegacyTerminalWindowsCmdTombstone, + renderLegacyTerminalWindowsPowerShellTombstone +} from './legacy-terminal-windows-tombstone' const LEGACY_TERMINAL_ATTRIBUTION_ENABLE_ENV_KEY = 'ORCA_ENABLE_GIT_ATTRIBUTION' const LEGACY_TERMINAL_ATTRIBUTION_BYPASS_ENV_KEY = 'ORCA_ATTRIBUTION_BYPASS' @@ -25,93 +29,6 @@ export const LEGACY_TERMINAL_SHIM_REMOTE_ENV_KEYS = [ LEGACY_TERMINAL_ATTRIBUTION_ENABLE_ENV_KEY ] as const -const WIN32_PASSTHROUGH_WRAPPER = String.raw`@echo off -setlocal -set "orca_real=%ORCA_REAL___ORCA_UPPER_COMMAND__%" -set "orca_wrapper_dir=%~dp0" -set "orca_legacy_wrapper_dir=%ORCA_ATTRIBUTION_SHIM_DIR%" -set "orca_clean_path=" -for %%P in ("%PATH:;=" "%") do call :orca_append_path "%%~P" -set "PATH=%orca_clean_path%" -set "ORCA_ENABLE_GIT_ATTRIBUTION=" -set "ORCA_GIT_COMMIT_TRAILER=" -set "ORCA_GH_PR_FOOTER=" -set "ORCA_GH_ISSUE_FOOTER=" -set "ORCA_ATTRIBUTION_SHIM_DIR=" -set "ORCA_ATTRIBUTION_BYPASS=" -set "ORCA_REAL_GIT=" -set "ORCA_REAL_GH=" -if defined orca_real for %%G in ("%orca_real%") do if /I "%%~dpG"=="%~dp0" set "orca_real=" -rem Why: clear a captured path that no longer exists, or the PATH walk below is skipped. -if defined orca_real if not exist "%orca_real%" set "orca_real=" -if defined orca_real goto run -rem Why: an unqualified Windows command lookup searches the current directory before PATH, so a -rem repository-local __ORCA_COMMAND__.exe would win. Walk the cleaned PATH ourselves instead. -for %%P in ("%orca_clean_path:;=" "%") do call :orca_try_candidate "%%~P" -if not defined orca_real ( - echo Orca compatibility wrapper could not locate __ORCA_COMMAND__ on PATH. 1>&2 - exit /b 127 -) -:run -"%orca_real%" %* -exit /b %ERRORLEVEL% - -:orca_try_candidate -if defined orca_real exit /b -if "%~1"=="" exit /b -for %%G in ("%~1") do if /I not "%%~fG\"=="%~dp0" if exist "%%~fG\__ORCA_COMMAND__.exe" set "orca_real=%%~fG\__ORCA_COMMAND__.exe" -exit /b - -:orca_append_path -for %%G in ("%~1") do set "orca_path_entry_dir=%%~fG\" -if /I "%orca_path_entry_dir%"=="%orca_wrapper_dir%" exit /b -if defined orca_legacy_wrapper_dir for %%G in ("%orca_legacy_wrapper_dir%") do if /I "%orca_path_entry_dir%"=="%%~fG\" exit /b -if defined orca_clean_path (set "orca_clean_path=%orca_clean_path%;%~1") else set "orca_clean_path=%~1" -exit /b -` - -const POWERSHELL_PASSTHROUGH_WRAPPER = String.raw`$ErrorActionPreference = 'Stop' -$commandName = '__ORCA_COMMAND__' -$realCommand = [Environment]::GetEnvironmentVariable('ORCA_REAL___ORCA_UPPER_COMMAND__') -$wrapperDir = Split-Path -Parent $MyInvocation.MyCommand.Path -$legacyWrapperDir = $env:ORCA_ATTRIBUTION_SHIM_DIR -$wrapperDirs = @($wrapperDir, $legacyWrapperDir) | Where-Object { $_ } | ForEach-Object { $_.TrimEnd('\') } -$env:PATH = (($env:PATH -split ';') | Where-Object { - $pathEntry = $_ - $pathEntry -and -not ($wrapperDirs | Where-Object { - [string]::Equals($_, $pathEntry.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase) - }) -}) -join ';' -'ORCA_ENABLE_GIT_ATTRIBUTION', 'ORCA_GIT_COMMIT_TRAILER', 'ORCA_GH_PR_FOOTER', 'ORCA_GH_ISSUE_FOOTER', 'ORCA_ATTRIBUTION_SHIM_DIR', 'ORCA_REAL_GIT', 'ORCA_REAL_GH', 'ORCA_ATTRIBUTION_BYPASS' | ForEach-Object { Remove-Item "Env:$_" -ErrorAction SilentlyContinue } -if ($realCommand) { - try { - $capturedDir = Split-Path -Parent ([IO.Path]::GetFullPath($realCommand)) - if ([string]::Equals($capturedDir.TrimEnd('\'), $wrapperDir.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase)) { - $realCommand = $null - } - } catch { - $realCommand = $null - } -} -if (-not $realCommand -or -not (Test-Path -LiteralPath $realCommand)) { - # Why: resolve only against the cleaned PATH directories. Ambient lookup could pick up a - # repository-local git.exe/gh.exe from the current directory. - $realCommand = $null - foreach ($dir in ($env:PATH -split ';')) { - if (-not $dir) { continue } - if ($wrapperDirs | Where-Object { [string]::Equals($_, $dir.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase) }) { continue } - $candidate = Join-Path $dir "$commandName.exe" - if (Test-Path -LiteralPath $candidate -PathType Leaf) { $realCommand = $candidate; break } - } -} -if (-not $realCommand) { - [Console]::Error.WriteLine("Orca compatibility wrapper could not locate $commandName on PATH.") - exit 127 -} -& $realCommand @args -exit $LASTEXITCODE -` - let neutralized = false let neutralizationRetryTimer: ReturnType | null = null let neutralizationRetryAttempt = 0 @@ -179,27 +96,20 @@ function writeNeutralWrappers(rootDir: string): void { mkdirSync(posixDir, { recursive: true }) mkdirSync(win32Dir, { recursive: true }) for (const command of ['git', 'gh'] as const) { - const upperCommand = command.toUpperCase() writeFileAtomically(join(posixDir, command), renderLegacyTerminalPosixTombstone(command), 0o755) writeFileAtomically( join(win32Dir, `${command}.cmd`), - renderWindowsWrapper(WIN32_PASSTHROUGH_WRAPPER, command, upperCommand), + renderLegacyTerminalWindowsCmdTombstone(command), 0o755 ) writeFileAtomically( join(win32Dir, `${command}-wrapper.ps1`), - renderWindowsWrapper(POWERSHELL_PASSTHROUGH_WRAPPER, command, upperCommand), + renderLegacyTerminalWindowsPowerShellTombstone(command), 0o755 ) } } -function renderWindowsWrapper(template: string, command: string, upperCommand: string): string { - return template - .replaceAll('__ORCA_UPPER_COMMAND__', upperCommand) - .replaceAll('__ORCA_COMMAND__', command) -} - function writeFileAtomically(filePath: string, contents: string, mode: number): void { const temporaryPath = `${filePath}.orca-neutralizing-${process.pid}` try { diff --git a/src/main/pty/legacy-terminal-windows-tombstone.ts b/src/main/pty/legacy-terminal-windows-tombstone.ts new file mode 100644 index 00000000000..4d5bce15044 --- /dev/null +++ b/src/main/pty/legacy-terminal-windows-tombstone.ts @@ -0,0 +1,133 @@ +// Why: kept beside the POSIX tombstone so the generated wrapper text for every platform lives +// in one place, and so the shim-dir module stays under the max-lines limit. + +const WIN32_PASSTHROUGH_WRAPPER = String.raw`@echo off +setlocal +set "orca_real=%ORCA_REAL___ORCA_UPPER_COMMAND__%" +set "orca_wrapper_dir=%~dp0" +set "orca_legacy_wrapper_dir=%ORCA_ATTRIBUTION_SHIM_DIR%" +set "orca_clean_path=" +rem Why: an empty PATH leaves the substitution below with an unbalanced quote, which +rem desynchronizes cmd parsing for the rest of the file. Skip the line entirely instead. +if not defined PATH goto :orca_path_walked +for %%P in ("%PATH:;=" "%") do call :orca_append_path "%%~P" +:orca_path_walked +set "PATH=%orca_clean_path%" +set "ORCA_ENABLE_GIT_ATTRIBUTION=" +set "ORCA_GIT_COMMIT_TRAILER=" +set "ORCA_GH_PR_FOOTER=" +set "ORCA_GH_ISSUE_FOOTER=" +set "ORCA_ATTRIBUTION_SHIM_DIR=" +set "ORCA_ATTRIBUTION_BYPASS=" +set "ORCA_REAL_GIT=" +set "ORCA_REAL_GH=" +if defined orca_real for %%G in ("%orca_real%") do if /I "%%~dpG"=="%~dp0" set "orca_real=" +rem Why: clear a captured path that no longer exists, or the PATH walk below is skipped. +if defined orca_real if not exist "%orca_real%" set "orca_real=" +if defined orca_real goto run +rem Why: an unqualified Windows command lookup searches the current directory before PATH, so a +rem repository-local __ORCA_COMMAND__.exe would win. Walk the cleaned PATH ourselves instead. +if not defined orca_clean_path goto :orca_candidates_walked +for %%P in ("%orca_clean_path:;=" "%") do call :orca_try_candidate "%%~P" +:orca_candidates_walked +if not defined orca_real ( + echo Orca compatibility wrapper could not locate __ORCA_COMMAND__ on PATH. 1>&2 + exit /b 127 +) +:run +"%orca_real%" %* +exit /b %ERRORLEVEL% + +:orca_try_candidate +if defined orca_real exit /b +if "%~1"=="" exit /b +rem Why: a relative entry resolves against the current directory, same exposure as an empty one. +rem Tested in pure batch on purpose: an external tool invoked here would itself be resolved from +rem the current directory, reintroducing the very hijack this guard exists to prevent. +set "orca_candidate=%~1" +if "%orca_candidate:~0,2%"=="\\" goto orca_candidate_rooted +if "%orca_candidate:~1,2%"==":\" goto orca_candidate_rooted +if "%orca_candidate:~1,2%"==":/" goto orca_candidate_rooted +exit /b +:orca_candidate_rooted +rem Why: %~dp0 is rebound to this label inside CALL, so compare against the cached wrapper dir. +for %%G in ("%~1") do ( + if /I not "%%~fG\"=="%orca_wrapper_dir%" ( + if exist "%%~fG\__ORCA_COMMAND__.exe" set "orca_real=%%~fG\__ORCA_COMMAND__.exe" + if not defined orca_real if exist "%%~fG\__ORCA_COMMAND__.cmd" set "orca_real=%%~fG\__ORCA_COMMAND__.cmd" + if not defined orca_real if exist "%%~fG\__ORCA_COMMAND__.bat" set "orca_real=%%~fG\__ORCA_COMMAND__.bat" + ) +) +exit /b + +:orca_append_path +for %%G in ("%~1") do set "orca_path_entry_dir=%%~fG\" +if /I "%orca_path_entry_dir%"=="%orca_wrapper_dir%" exit /b +if defined orca_legacy_wrapper_dir for %%G in ("%orca_legacy_wrapper_dir%") do if /I "%orca_path_entry_dir%"=="%%~fG\" exit /b +if defined orca_clean_path (set "orca_clean_path=%orca_clean_path%;%~1") else set "orca_clean_path=%~1" +exit /b +` + +const POWERSHELL_PASSTHROUGH_WRAPPER = String.raw`$ErrorActionPreference = 'Stop' +$commandName = '__ORCA_COMMAND__' +$realCommand = [Environment]::GetEnvironmentVariable('ORCA_REAL___ORCA_UPPER_COMMAND__') +$wrapperDir = Split-Path -Parent $MyInvocation.MyCommand.Path +$legacyWrapperDir = $env:ORCA_ATTRIBUTION_SHIM_DIR +$wrapperDirs = @($wrapperDir, $legacyWrapperDir) | Where-Object { $_ } | ForEach-Object { $_.TrimEnd('\') } +$env:PATH = (($env:PATH -split ';') | Where-Object { + $pathEntry = $_ + $pathEntry -and -not ($wrapperDirs | Where-Object { + [string]::Equals($_, $pathEntry.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase) + }) +}) -join ';' +'ORCA_ENABLE_GIT_ATTRIBUTION', 'ORCA_GIT_COMMIT_TRAILER', 'ORCA_GH_PR_FOOTER', 'ORCA_GH_ISSUE_FOOTER', 'ORCA_ATTRIBUTION_SHIM_DIR', 'ORCA_REAL_GIT', 'ORCA_REAL_GH', 'ORCA_ATTRIBUTION_BYPASS' | ForEach-Object { Remove-Item "Env:$_" -ErrorAction SilentlyContinue } +if ($realCommand) { + try { + $capturedDir = Split-Path -Parent ([IO.Path]::GetFullPath($realCommand)) + if ([string]::Equals($capturedDir.TrimEnd('\'), $wrapperDir.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase)) { + $realCommand = $null + } + } catch { + $realCommand = $null + } +} +if (-not $realCommand -or -not (Test-Path -LiteralPath $realCommand)) { + # Why: resolve only against the cleaned PATH directories. Ambient lookup could pick up a + # repository-local git.exe/gh.exe from the current directory. + $realCommand = $null + foreach ($dir in ($env:PATH -split ';')) { + if (-not $dir) { continue } + # Why: a relative entry resolves against the current directory, same exposure as an empty one. + # IsPathRooted is not enough: it accepts drive-relative 'C:foo', which resolves against the + # current directory on that drive. IsPathFullyQualified is absent on Windows PowerShell 5.1, + # so match the same prefixes the cmd wrapper accepts. + if ($dir -notmatch '^([A-Za-z]:[\\/]|\\\\)') { continue } + if ($wrapperDirs | Where-Object { [string]::Equals($_, $dir.TrimEnd('\'), [StringComparison]::OrdinalIgnoreCase) }) { continue } + foreach ($ext in @('.exe', '.cmd', '.bat')) { + $candidate = Join-Path $dir "$commandName$ext" + if (Test-Path -LiteralPath $candidate -PathType Leaf) { $realCommand = $candidate; break } + } + if ($realCommand) { break } + } +} +if (-not $realCommand) { + [Console]::Error.WriteLine("Orca compatibility wrapper could not locate $commandName on PATH.") + exit 127 +} +& $realCommand @args +exit $LASTEXITCODE +` + +function renderWindowsWrapper(template: string, command: string, upperCommand: string): string { + return template + .replaceAll('__ORCA_UPPER_COMMAND__', upperCommand) + .replaceAll('__ORCA_COMMAND__', command) +} + +export function renderLegacyTerminalWindowsCmdTombstone(command: 'git' | 'gh'): string { + return renderWindowsWrapper(WIN32_PASSTHROUGH_WRAPPER, command, command.toUpperCase()) +} + +export function renderLegacyTerminalWindowsPowerShellTombstone(command: 'git' | 'gh'): string { + return renderWindowsWrapper(POWERSHELL_PASSTHROUGH_WRAPPER, command, command.toUpperCase()) +} From c3b8c145e2e060da170a300151ebd1160c045243 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:19:26 -0700 Subject: [PATCH 4/4] fix(agent-status): preserve Codex escape interruption (#14372) --- src/main/agent-hooks/server.test.ts | 68 ++++++++++++++++ src/main/agent-hooks/server.ts | 5 +- .../agent-interrupt-inference.test.ts | 79 ++++++++++++------- .../agent-interrupt-inference.ts | 13 +-- .../terminal-pane/pty-connection.ts | 22 +++--- 5 files changed, 144 insertions(+), 43 deletions(-) diff --git a/src/main/agent-hooks/server.test.ts b/src/main/agent-hooks/server.test.ts index c1870d35a7c..34ef9b68db1 100644 --- a/src/main/agent-hooks/server.test.ts +++ b/src/main/agent-hooks/server.test.ts @@ -1070,6 +1070,74 @@ describe('AgentHookServer listener replay', () => { } }) + it('does not let late Codex tool hooks with explicit prompt resurrect an inferred interrupt', () => { + vi.useFakeTimers() + vi.setSystemTime(1_000) + try { + const server = new AgentHookServer() + server.ingestRemote( + { + paneKey: PANE, + tabId: 'tab-1', + worktreeId: 'wt-1', + hasExplicitPrompt: true, + hookEventName: 'UserPromptSubmit', + payload: { + state: 'working', + prompt: 'Run sleep 30, then reply done.', + agentType: 'codex' + } + }, + 'conn-1' + ) + const baseline = server.getStatusSnapshot()[0] + + vi.setSystemTime(1_500) + expect( + server.inferInterrupt({ + paneKey: PANE, + baselineUpdatedAt: baseline.receivedAt, + baselineStateStartedAt: baseline.stateStartedAt, + baselinePrompt: 'Run sleep 30, then reply done.', + baselineAgentType: 'codex', + intent: 'plain-escape' + }) + ).toBe(true) + + vi.setSystemTime(6_000) + server.ingestRemote( + { + paneKey: PANE, + tabId: 'tab-1', + worktreeId: 'wt-1', + hasExplicitPrompt: true, + hookEventName: 'PostToolUse', + payload: { + state: 'working', + prompt: 'Run sleep 30, then reply done.', + agentType: 'codex', + toolName: 'Bash', + toolInput: 'sleep 30' + } + }, + 'conn-1' + ) + + expect(server.getStatusSnapshot()).toEqual([ + expect.objectContaining({ + state: 'done', + prompt: 'Run sleep 30, then reply done.', + agentType: 'codex', + interrupted: true, + receivedAt: 1_500, + stateStartedAt: 1_500 + }) + ]) + } finally { + vi.useRealTimers() + } + }) + it('allows a new prompt after an inferred interrupt', () => { vi.useFakeTimers() vi.setSystemTime(1_000) diff --git a/src/main/agent-hooks/server.ts b/src/main/agent-hooks/server.ts index 4dc64d523de..8a10080168c 100644 --- a/src/main/agent-hooks/server.ts +++ b/src/main/agent-hooks/server.ts @@ -448,7 +448,7 @@ function isToolProgressWorkingAfterInterrupt(next: AgentHookEventPayload): boole if (next.payload.state !== 'working') { return false } - if (next.payload.agentType !== 'claude') { + if (next.payload.agentType !== 'claude' && next.payload.agentType !== 'codex') { return false } // Why: a same-prompt retry is another UserPromptSubmit, while late post-Ctrl+C progress arrives as tool lifecycle work. @@ -1260,6 +1260,9 @@ export class AgentHookServer { (effectivePayload.hasExplicitPrompt !== true && Date.now() - previous.receivedAt <= INTERRUPTED_DONE_LATE_WORKING_SUPPRESSION_MS)) ) { + if (effectivePayload.payload.agentType === 'codex') { + markCodexLeadTurnInterrupted(this.state, effectivePayload.paneKey) + } return previous } if ( diff --git a/src/renderer/src/components/terminal-pane/agent-interrupt-inference.test.ts b/src/renderer/src/components/terminal-pane/agent-interrupt-inference.test.ts index 98cc941aadd..9bab0bd5005 100644 --- a/src/renderer/src/components/terminal-pane/agent-interrupt-inference.test.ts +++ b/src/renderer/src/components/terminal-pane/agent-interrupt-inference.test.ts @@ -72,34 +72,59 @@ describe('agent interrupt inference', () => { it.each([ ['plain-escape', 'gemini'], - ['ctrl-c', 'gemini'] - ] as const)( - 'emits a strict baseline request for %s from Gemini immediately', - (intent, agentType) => { - vi.useFakeTimers() - let entry: AgentStatusEntry | undefined = makeEntry({ agentType }) - const inferInterrupt = vi.fn() - const tracker = createAgentInterruptInference({ - paneKey: PANE_KEY, - getStatusEntry: () => entry, - inferInterrupt, - now: () => 1_100 - }) + ['ctrl-c', 'gemini'], + ['plain-escape', 'codex'] + ] as const)('emits a strict baseline request for %s from %s immediately', (intent, agentType) => { + vi.useFakeTimers() + let entry: AgentStatusEntry | undefined = makeEntry({ agentType }) + const inferInterrupt = vi.fn() + const tracker = createAgentInterruptInference({ + paneKey: PANE_KEY, + getStatusEntry: () => entry, + inferInterrupt, + now: () => 1_100 + }) - tracker.observeInputIntent(intent) + tracker.observeInputIntent(intent) - expect(inferInterrupt).toHaveBeenCalledWith({ - paneKey: PANE_KEY, - baselineUpdatedAt: 1_000, - baselineStateStartedAt: 900, - baselinePrompt: 'write tests', - baselineAgentType: agentType, - intent - }) - tracker.dispose() - entry = undefined - } - ) + expect(inferInterrupt).toHaveBeenCalledWith({ + paneKey: PANE_KEY, + baselineUpdatedAt: 1_000, + baselineStateStartedAt: 900, + baselinePrompt: 'write tests', + baselineAgentType: agentType, + intent + }) + tracker.dispose() + entry = undefined + }) + + it('records a Codex Escape before its immediate done hook replaces the working row', () => { + vi.useFakeTimers() + let entry: AgentStatusEntry | undefined = makeEntry({ agentType: 'codex' }) + const inferInterrupt = vi.fn() + const tracker = createAgentInterruptInference({ + paneKey: PANE_KEY, + getStatusEntry: () => entry, + inferInterrupt, + now: () => 1_100 + }) + + tracker.observeInputIntent('plain-escape') + entry = makeEntry({ state: 'done', updatedAt: 1_101, stateStartedAt: 1_101 }) + vi.advanceTimersByTime(500) + + expect(inferInterrupt).toHaveBeenCalledTimes(1) + expect(inferInterrupt).toHaveBeenCalledWith({ + paneKey: PANE_KEY, + baselineUpdatedAt: 1_000, + baselineStateStartedAt: 900, + baselinePrompt: 'write tests', + baselineAgentType: 'codex', + intent: 'plain-escape' + }) + tracker.dispose() + }) it('reports Escape while Claude is waiting on AskUserQuestion', () => { vi.useFakeTimers() @@ -344,7 +369,7 @@ describe('agent interrupt inference', () => { it('cancels when a newer hook update arrives during the settle window', () => { vi.useFakeTimers() const inferInterrupt = vi.fn() - let entry: AgentStatusEntry | undefined = makeEntry() + let entry: AgentStatusEntry | undefined = makeEntry({ agentType: 'custom-agent' }) const tracker = createAgentInterruptInference({ paneKey: PANE_KEY, getStatusEntry: () => entry, diff --git a/src/renderer/src/components/terminal-pane/agent-interrupt-inference.ts b/src/renderer/src/components/terminal-pane/agent-interrupt-inference.ts index b847badcdfc..8e2f4cecde3 100644 --- a/src/renderer/src/components/terminal-pane/agent-interrupt-inference.ts +++ b/src/renderer/src/components/terminal-pane/agent-interrupt-inference.ts @@ -15,7 +15,7 @@ export type AgentInterruptInference = { intent: AgentInterruptInputIntent, entry?: AgentStatusEntry | null, baselineSequence?: number - ): void + ): boolean | Promise | undefined flushPending(): boolean | Promise dispose(): void } @@ -50,7 +50,8 @@ function shouldFlushInterruptImmediately( ): boolean { return ( requiresDoubleEscapeForAgent(baseline.agentType, baseline.intent) || - baseline.agentType === 'gemini' + baseline.agentType === 'gemini' || + (baseline.agentType === 'codex' && baseline.intent === 'plain-escape') ) } @@ -261,12 +262,12 @@ export function createAgentInterruptInference({ } pendingBaseline = baseline if (shouldFlushInterruptImmediately(baseline)) { - // Why: these agents can emit their idle/done hook immediately after an - // accepted interrupt. Flush before that hook overwrites the working baseline. - void flushPending() - return + // Why: these interrupts can emit an idle/done hook before the settle timer, + // overwriting the working baseline and losing the interrupted outcome. + return flushPending() } pendingTimer = setTimer(flushPendingFromTimer, AGENT_INTERRUPT_SETTLE_MS) + return undefined }, flushPending, dispose() { diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 572d3e68b19..b4a683874ee 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -2015,10 +2015,10 @@ export function connectPanePty( // "question answered" signal no hook will ever deliver. questionAnsweredInference.observeSentTerminalInput(data) } - let pendingTerminalInputWrite: Promise | null = null + let pendingTerminalInputWrite: Promise | null = null let sequencedInterruptStatusBaseline: AgentStatusEntry | null | undefined let interruptStatusBaselineSequence = 0 - const setPendingTerminalInputWrite = (promise: Promise): void => { + const setPendingTerminalInputWrite = (promise: Promise): void => { pendingTerminalInputWrite = promise void promise.finally(() => { if (pendingTerminalInputWrite === promise) { @@ -2031,7 +2031,9 @@ export function connectPanePty( if (!pendingWrite) { return interruptInference.flushPending() } - return pendingWrite.then(() => interruptInference.flushPending()) + return pendingWrite.then((immediateResult) => { + return immediateResult ?? interruptInference.flushPending() + }) } // Why: the 133;D confirmation guard and the visible-pane resampler both key off // "does this pane expect an agent"; derive each signal once so the two callers @@ -4166,26 +4168,28 @@ export function connectPanePty( clearPendingTerminalInputIntent() const writePromise = transport .sendInputAccepted(data) - .then((accepted) => { + .then((accepted): boolean | Promise | null => { if (accepted) { // Why: rejected writes use transport recovery and must not arm a parser probe. markAcceptedTerminalInputSent() observeAcceptedShellCommandInput(data) observeAcceptedTerminalInput(data, acknowledgedIntent) - interruptInference.observeInputIntent( + const immediateResult = interruptInference.observeInputIntent( acknowledgedIntent, interruptStatusBaseline, capturedBaselineSequence ) observeTitleOnlyInterrupt() - } else { - // Why: Esc/Ctrl+C are the first keys users press on a frozen pane; - // an unbound-transport reject here must arm recovery too. - requestRecoveryForUndeliverableInput() + return immediateResult ?? null } + // Why: Esc/Ctrl+C are the first keys users press on a frozen pane; + // an unbound-transport reject here must arm recovery too. + requestRecoveryForUndeliverableInput() + return null }) .catch((err) => { console.warn('[agent-interrupt] acknowledged terminal input failed:', err) + return null }) setPendingTerminalInputWrite(writePromise) return