diff --git a/src/renderer/src/components/terminal-pane/pty-connection-cold-restore-resume-command.test.ts b/src/renderer/src/components/terminal-pane/pty-connection-cold-restore-resume-command.test.ts index 1281f12b119..51851ead59d 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection-cold-restore-resume-command.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection-cold-restore-resume-command.test.ts @@ -462,10 +462,12 @@ describe('connectPanePty', () => { }) // Regression (#12320 residual): a cold restore after restart can run before the - // store hydrates `settings`, so terminalWindowsShell is momentarily undefined. The - // spawn side then launches %COMSPEC% (cmd.exe), so the resume must be cmd-quoted — - // PowerShell single quotes reach cmd.exe literally ("unrecognized subcommand - // ''resume''"), which is exactly the field report on a released build. + // store hydrates `settings`, so terminalWindowsShell is momentarily undefined and + // the pane may be the cmd.exe default. The codex resume argv is cmd-quote-safe, so + // the target resolver's race guess picks cmd — a bare "token" that also parses + // identically in PowerShell, so it fixes the cmd pane with no PowerShell risk. + // Before the guess, PowerShell single quotes reached cmd.exe literally + // ("unrecognized subcommand ''resume''"), exactly the field report. it('quotes for cmd.exe on cold restore when the shell setting has not hydrated', async () => { await expect(runWindowsColdRestoreResume({})).resolves.toBe( 'codex "--dangerously-bypass-approvals-and-sandbox" "resume" "codex-session-1"' diff --git a/src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts b/src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts index 3e4ab1e38d5..8087ecda8f3 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/cold-restore-resume-startup.ts @@ -8,6 +8,7 @@ import { } from '../../../../../shared/tui-agent-launch-defaults' import { agentProviderSessionsEqual, + getAgentResumeArgv, isResumableTuiAgent, normalizeAgentProviderSession } from '../../../../../shared/agent-session-resume' @@ -49,6 +50,10 @@ export function bindBuildColdRestoreAgentResumeStartup(session: ConnectPanePtySe const launchConfig = (useLiveEntry && entry ? state.getAgentLaunchConfigForStatusEntry(entry) : undefined) ?? matchingSleepingLaunchConfig + const effectiveAgentArgs = + launchConfig !== undefined + ? launchConfig.agentArgs + : resolveTuiAgentLaunchArgs(agent, state.settings?.agentDefaultArgs) // Why: the resume line is typed into this pane's live shell, so its quoting must // follow the tab's effective Windows shell, not the win32 PowerShell default. const resumeTarget = resolveAgentResumeLaunchTarget({ @@ -57,16 +62,19 @@ export function bindBuildColdRestoreAgentResumeStartup(session: ConnectPanePtySe executionHostId: session.executionHostId, worktreePath: session.worktree?.path, terminalWindowsShell: state.settings?.terminalWindowsShell, - tabShellOverride: session.shellOverride + tabShellOverride: session.shellOverride, + // The cold-restore path can run before the store hydrates the shell setting; + // pass the resume argv (and agentArgs, unless a custom command supersedes + // them) so the target resolver's race guess can prove cmd-quoting is safe. + resumeArgv: + getAgentResumeArgv(agent, providerSession, launchConfig?.ompResumeFilePath) ?? undefined, + resumeAgentArgs: launchConfig?.agentCommand?.trim() ? null : effectiveAgentArgs }) const startupPlan = buildAgentResumeStartupPlan({ agent, providerSession, cmdOverrides: state.settings?.agentCmdOverrides ?? {}, - agentArgs: - launchConfig !== undefined - ? launchConfig.agentArgs - : resolveTuiAgentLaunchArgs(agent, state.settings?.agentDefaultArgs), + agentArgs: effectiveAgentArgs, agentEnv: launchConfig !== undefined ? launchConfig.agentEnv diff --git a/src/renderer/src/lib/agent-resume-launch-target.test.ts b/src/renderer/src/lib/agent-resume-launch-target.test.ts index 064bcf8c699..a1bdebb4cfc 100644 --- a/src/renderer/src/lib/agent-resume-launch-target.test.ts +++ b/src/renderer/src/lib/agent-resume-launch-target.test.ts @@ -72,17 +72,78 @@ describe('resolveAgentResumeLaunchTarget on a Windows client', () => { }) }) - it('quotes for cmd.exe when no Windows shell is configured (mirrors the %COMSPEC% spawn fallback)', async () => { - // Why cmd, not PowerShell: with no configured shell the spawn side launches - // %COMSPEC% (cmd.exe), and this state is reachable when a cold restore runs - // before the store hydrates `settings`. PowerShell quoting here would put - // single quotes into a cmd.exe pane and break the resume (#12320 residual). + it('keeps the PowerShell default when no Windows shell is configured and no resume argv is given', async () => { + // Without the resume argv the race guess cannot prove cmd-quoting is safe, so + // it must not fire — an unset shell falls back to the win32 PowerShell default. await expect(resolveWith({})).resolves.toEqual({ platform: 'win32', - shell: 'cmd' + shell: 'powershell' }) }) + it('guesses cmd for an unset shell when every resume token is cmd-quote-safe', async () => { + // Cold-restore race: settings not yet hydrated, so the pane may be the + // %COMSPEC% (cmd.exe) default. A clean codex argv quotes identically in cmd + // and PowerShell, so guessing cmd fixes the cmd pane with no PowerShell risk. + await expect( + resolveWith({ resumeArgv: ['codex', 'resume', 'a1b2c3d4-0000-4000-8000-000000000000'] }) + ).resolves.toEqual({ platform: 'win32', shell: 'cmd' }) + }) + + it('keeps the PowerShell default for an unset shell when a resume token needs cmd escaping', async () => { + // An omp/pi/prime-agent transcript path can carry cmd-special chars (parens, + // e.g. `(x86)`). cmd `^`-escaping would corrupt that path in a PowerShell + // race pane, so the guess must stay off and leave the win32 default. + await expect( + resolveWith({ + resumeArgv: ['omp', '--resume', 'C:\\Users\\neil\\AppData (x86)\\omp\\session.jsonl'] + }) + ).resolves.toEqual({ platform: 'win32', shell: 'powershell' }) + }) + + it('keeps the PowerShell default for an unset shell when agentArgs need cmd escaping', async () => { + // The resume argv is clean, but agentArgs the command will ^-escape carry a + // path with parens; a cmd guess would corrupt them in a PowerShell race pane. + await expect( + resolveWith({ + resumeArgv: ['codex', 'resume', 'a1b2c3d4-0000-4000-8000-000000000000'], + resumeAgentArgs: '--add-dir C:\\Program Files (x86)\\proj' + }) + ).resolves.toEqual({ platform: 'win32', shell: 'powershell' }) + }) + + it('keeps the PowerShell default when an INTERIOR agentArgs token ends in a backslash', async () => { + // The raw string does not end in `\`, but the command tokenizes agentArgs + // and cmd-quotes each token, so the interior `C:\projects\` becomes + // `"C:\projects\"` — an arg-merge under CommandLineToArgvW. The gate must + // tokenize the same way and reject it, not scan the raw string. + await expect( + resolveWith({ + resumeArgv: ['codex', 'resume', 'a1b2c3d4-0000-4000-8000-000000000000'], + resumeAgentArgs: '--add-dir C:\\projects\\ --model gpt' + }) + ).resolves.toEqual({ platform: 'win32', shell: 'powershell' }) + }) + + it('still guesses cmd for an unset shell when clean agentArgs accompany a clean resume argv', async () => { + await expect( + resolveWith({ + resumeArgv: ['codex', 'resume', 'a1b2c3d4-0000-4000-8000-000000000000'], + resumeAgentArgs: '--dangerously-bypass-approvals-and-sandbox --model gpt' + }) + ).resolves.toEqual({ platform: 'win32', shell: 'cmd' }) + }) + + it('never overrides a configured shell with the race guess', async () => { + // A hydrated powershell.exe must win even when the argv is cmd-quote-safe. + await expect( + resolveWith({ + terminalWindowsShell: 'powershell.exe', + resumeArgv: ['codex', 'resume', 'a1b2c3d4-0000-4000-8000-000000000000'] + }) + ).resolves.toEqual({ platform: 'win32', shell: 'powershell' }) + }) + it('leaves an SSH workspace on its own default quoting', async () => { await expect( resolveWith({ diff --git a/src/renderer/src/lib/agent-resume-launch-target.ts b/src/renderer/src/lib/agent-resume-launch-target.ts index 90a6a243afb..af07619d484 100644 --- a/src/renderer/src/lib/agent-resume-launch-target.ts +++ b/src/renderer/src/lib/agent-resume-launch-target.ts @@ -4,7 +4,11 @@ import { parseExecutionHostId } from '../../../shared/execution-host' import { isWslUncPath } from '../../../shared/wsl-paths' import { resolveLocalWindowsAgentStartupShell } from '../../../shared/windows-terminal-shell' import type { ProjectExecutionRuntimeResolution } from '../../../shared/project-execution-runtime' -import type { AgentStartupShell } from '../../../shared/tui-agent-startup-shell' +import { + isCmdQuotingPowerShellSafe, + tokenizeStartupCommand, + type AgentStartupShell +} from '../../../shared/tui-agent-startup-shell' export type AgentResumeLaunchTarget = { platform: NodeJS.Platform @@ -22,6 +26,16 @@ export type AgentResumeLaunchTargetArgs = { terminalWindowsShell: string | null | undefined /** Per-tab Windows shell override, which beats the global setting at spawn time. */ tabShellOverride?: string | null + /** The resume argv this quoting is for. Only consulted for the cold-restore + * race guess below; omit it and the guess never fires. */ + resumeArgv?: readonly string[] | null + /** The raw agentArgs suffix the built command will tokenize and `^`-escape + * per token (unless a custom agentCommand supersedes them, in which case pass + * null). The race-guess gate tokenizes it the same way and vets each token, + * so a cmd guess can never `^`-corrupt an agentArg token in a PowerShell race + * pane — including an INTERIOR token ending in `\`, which a whole-string + * check would miss. */ + resumeAgentArgs?: string | null } function resolveResumeLaunchPlatform(args: AgentResumeLaunchTargetArgs): NodeJS.Platform { @@ -47,16 +61,44 @@ export function resolveAgentResumeLaunchTarget( args: AgentResumeLaunchTargetArgs ): AgentResumeLaunchTarget { const platform = resolveResumeLaunchPlatform(args) - return { + const isRemote = + Boolean(args.connectionId) || parseExecutionHostId(args.executionHostId)?.kind !== 'local' + const effectiveWindowsShell = resolveWindowsShellOverride( + args.tabShellOverride, + args.terminalWindowsShell + ) + const shell = resolveLocalWindowsAgentStartupShell({ platform, - shell: resolveLocalWindowsAgentStartupShell({ - platform, - isRemote: - Boolean(args.connectionId) || parseExecutionHostId(args.executionHostId)?.kind !== 'local', - terminalWindowsShell: resolveWindowsShellOverride( - args.tabShellOverride, - args.terminalWindowsShell - ) - }) + isRemote, + terminalWindowsShell: effectiveWindowsShell + }) + // Cold-restore race guess (#12320): a resume typed right after restart can run + // before the renderer store hydrates `settings`, so terminalWindowsShell is + // momentarily empty and we can't read which shell main actually spawned — the + // user's configured shell if set, otherwise the powershell.exe default. We + // still guess cmd here, but ONLY when every piece of free text the command + // `^`-escapes is cmd-quote-safe: `""` then parses identically in cmd + // AND PowerShell, so a configured cmd.exe pane is fixed (single quotes no + // longer reach it literally) with no risk to a PowerShell pane. Codex/Claude + // and every id-based agent qualify (clean flags + UUIDs). A path-carrying + // token (pi/prime-agent/omp transcript path, or a path in agentArgs, e.g. + // under `...\dir (x86)\...`) fails the gate and is left on the PowerShell + // default, because cmd `^`-escaping would corrupt it in a PowerShell race + // pane — never worse than the pre-guard behavior. + if (shell === 'powershell' && !effectiveWindowsShell?.trim() && args.resumeArgv) { + // Vet agentArgs as the command emits them — tokenized with cmd rules and + // `^`-escaped per token — not as one raw string: the safety of a token + // ending in `\` (arg-merge in cmd) is positional, so an interior token + // would slip a whole-string check. + const agentArgs = args.resumeAgentArgs?.trim() + ? tokenizeStartupCommand(args.resumeAgentArgs, 'cmd') + : null + if (!agentArgs || agentArgs.ok) { + const guardTokens = [...args.resumeArgv, ...(agentArgs?.tokens ?? [])] + if (guardTokens.every((token) => isCmdQuotingPowerShellSafe(token))) { + return { platform, shell: 'cmd' } + } + } } + return { platform, shell } } diff --git a/src/renderer/src/lib/sleeping-agent-session-launch.ts b/src/renderer/src/lib/sleeping-agent-session-launch.ts index 43d7bdb3b30..141cd1c2a67 100644 --- a/src/renderer/src/lib/sleeping-agent-session-launch.ts +++ b/src/renderer/src/lib/sleeping-agent-session-launch.ts @@ -13,7 +13,10 @@ import { resolveTuiAgentLaunchArgs, resolveTuiAgentLaunchEnv } from '../../../shared/tui-agent-launch-defaults' -import type { SleepingAgentSessionRecord } from '../../../shared/agent-session-resume' +import { + getAgentResumeArgv, + type SleepingAgentSessionRecord +} from '../../../shared/agent-session-resume' import { translate } from '@/i18n/i18n' export type ResumeSleepingAgentSessionsOptions = { @@ -28,7 +31,11 @@ export type ResumeSleepingAgentSessionsOptions = { onSessionLaunched?: (tabId: string) => void } -function getResumeLaunchTarget(worktreeId: string): AgentResumeLaunchTarget { +function getResumeLaunchTarget( + record: SleepingAgentSessionRecord, + effectiveAgentArgs: string | null | undefined +): AgentResumeLaunchTarget { + const worktreeId = record.worktreeId const state = useAppStore.getState() const worktree = state.getKnownWorktreeById(worktreeId) const repo = worktree ? state.repos.find((entry) => entry.id === worktree.repoId) : null @@ -38,7 +45,14 @@ function getResumeLaunchTarget(worktreeId: string): AgentResumeLaunchTarget { connectionId: repo?.connectionId, executionHostId: getExecutionHostIdForWorktree(state, worktreeId), worktreePath: worktree?.path, - terminalWindowsShell: state.settings?.terminalWindowsShell + terminalWindowsShell: state.settings?.terminalWindowsShell, + resumeArgv: + getAgentResumeArgv( + record.agent, + record.providerSession, + record.launchConfig?.ompResumeFilePath + ) ?? undefined, + resumeAgentArgs: record.launchConfig?.agentCommand?.trim() ? null : effectiveAgentArgs }) } @@ -68,15 +82,16 @@ export function launchSleepingAgentSession( ): boolean { const state = useAppStore.getState() const launchConfig = record.launchConfig - const resumeTarget = getResumeLaunchTarget(record.worktreeId) + const effectiveAgentArgs = + launchConfig !== undefined + ? launchConfig.agentArgs + : resolveTuiAgentLaunchArgs(record.agent, state.settings?.agentDefaultArgs) + const resumeTarget = getResumeLaunchTarget(record, effectiveAgentArgs) const startupPlan = buildAgentResumeStartupPlan({ agent: record.agent, providerSession: record.providerSession, cmdOverrides: state.settings?.agentCmdOverrides ?? {}, - agentArgs: - launchConfig !== undefined - ? launchConfig.agentArgs - : resolveTuiAgentLaunchArgs(record.agent, state.settings?.agentDefaultArgs), + agentArgs: effectiveAgentArgs, agentEnv: launchConfig !== undefined ? launchConfig.agentEnv diff --git a/src/shared/tui-agent-startup-shell.test.ts b/src/shared/tui-agent-startup-shell.test.ts index 1698e86b08b..6580bb3d7e6 100644 --- a/src/shared/tui-agent-startup-shell.test.ts +++ b/src/shared/tui-agent-startup-shell.test.ts @@ -3,6 +3,7 @@ import { buildShellCommandFromArgv, clearEnvCommand, commandSeparator, + isCmdQuotingPowerShellSafe, isPosixStartupShell, quoteStartupArg, tokenizeStartupCommand @@ -148,3 +149,48 @@ describe('one Unix startup dialect', () => { expect(plan?.env?.ORCA_PI_PREFILL).toBe('hello') }) }) + +describe('isCmdQuotingPowerShellSafe', () => { + it('accepts tokens cmd quotes as a bare "token" (safe in cmd AND PowerShell)', () => { + // Every id-based resume argv token: binary names, flags and UUIDs. + for (const token of [ + 'codex', + 'resume', + 'a1b2c3d4-0000-4000-8000-000000000000', + '--dangerously-bypass-approvals-and-sandbox', + '--resume=abc', + 'C:\\Users\\neil\\repo' + ]) { + expect(isCmdQuotingPowerShellSafe(token)).toBe(true) + // Contract: a safe token quotes to the same bare "token" in both shells. + expect(quoteStartupArg(token, 'cmd')).toBe(`"${token}"`) + } + }) + + it('rejects tokens that cmd would ^-escape, which corrupts them in a PowerShell pane', () => { + for (const token of [ + 'C:\\Users\\neil\\AppData (x86)\\omp\\session.jsonl', + 'a&b', + 'pct%VALUE%', + 'a|b', + 'ac', + 'has"quote' + ]) { + expect(isCmdQuotingPowerShellSafe(token)).toBe(false) + } + }) + + it('rejects tokens PowerShell expands inside double quotes ($ and backtick) even though cmd keeps them literal', () => { + // `"$env"` / "a`b" parse literally in cmd but expand/escape in PowerShell. + for (const token of ['$env', 'a$b', 'has`backtick', '`n']) { + expect(isCmdQuotingPowerShellSafe(token)).toBe(false) + } + }) + + it('rejects a trailing backslash, which would escape the appended closing quote', () => { + // `"C:\dir\"` → the child CommandLineToArgvW sees \" as a literal quote. + expect(isCmdQuotingPowerShellSafe('C:\\dir\\')).toBe(false) + // A backslash NOT at the end (a normal Windows path) stays safe. + expect(isCmdQuotingPowerShellSafe('C:\\dir\\file.jsonl')).toBe(true) + }) +}) diff --git a/src/shared/tui-agent-startup-shell.ts b/src/shared/tui-agent-startup-shell.ts index 94df0f80ac5..f7f74516f0d 100644 --- a/src/shared/tui-agent-startup-shell.ts +++ b/src/shared/tui-agent-startup-shell.ts @@ -215,6 +215,26 @@ function quotePortableUnixArg(value: string): string { return parts.join('') } +/** Characters that stop `quoteStartupArg(value, 'cmd')`'s `"value"` output from + * parsing identically in PowerShell. Two reasons a char lands here: + * - cmd quoting `^`-escapes it (`& | < > ( ) ^ % ! "`), and PowerShell does NOT + * strip the caret, so `"a^&b"` arrives as literal `a^&b` in a PowerShell pane; + * - PowerShell expands it inside a double-quoted string while cmd keeps it + * literal — `$` (variables / `$(...)`) and a backtick (escape char). */ +const CMD_QUOTING_POWERSHELL_UNSAFE = /[\^&|<>()%!"$`]/ + +/** True when `quoteStartupArg(value, 'cmd')` also parses identically in + * PowerShell — i.e. `"value"` is the literal `value` in BOTH shells. Used to + * decide the resume race guess when the pane's shell is not yet known: cmd + * quoting of such a token is safe whether the pane turns out cmd or PowerShell. + * A token with an unsafe char, or a trailing backslash (which would turn the + * appended closing quote into an escaped quote for the child's + * CommandLineToArgvW parser, e.g. `"C:\dir\"`), fails and keeps the caller on + * its shell default. */ +export function isCmdQuotingPowerShellSafe(value: string): boolean { + return !CMD_QUOTING_POWERSHELL_UNSAFE.test(value) && !value.endsWith('\\') +} + export function quoteStartupArg(value: string, shell: AgentStartupShell): string { if (shell === 'powershell') { return `'${value.replace(/'/g, "''")}'` diff --git a/src/shared/windows-terminal-shell.test.ts b/src/shared/windows-terminal-shell.test.ts index a746a1a7f3c..64a8b918348 100644 --- a/src/shared/windows-terminal-shell.test.ts +++ b/src/shared/windows-terminal-shell.test.ts @@ -68,9 +68,10 @@ describe('resolveLocalWindowsAgentStartupShell', () => { ).toBe('posix') }) - it('defaults an unset local Windows shell to cmd, mirroring the %COMSPEC% spawn fallback', () => { - // Why not PowerShell: the spawn side launches %COMSPEC% (cmd.exe) when no shell - // is configured, so quoting must match or single quotes reach cmd.exe literally. + it('defaults an unset local Windows shell to PowerShell, matching the win32 default', () => { + // The resume race guess that may prefer cmd for an unset shell lives in + // resolveAgentResumeLaunchTarget, not here; this resolver reflects only the + // configured shell and its win32 default. for (const shell of [undefined, null, '', ' ']) { expect( resolveLocalWindowsAgentStartupShell({ @@ -78,7 +79,7 @@ describe('resolveLocalWindowsAgentStartupShell', () => { isRemote: false, terminalWindowsShell: shell }) - ).toBe('cmd') + ).toBe('powershell') } }) }) diff --git a/src/shared/windows-terminal-shell.ts b/src/shared/windows-terminal-shell.ts index 57474b1d113..1f55bf89d35 100644 --- a/src/shared/windows-terminal-shell.ts +++ b/src/shared/windows-terminal-shell.ts @@ -52,16 +52,5 @@ export function resolveLocalWindowsAgentStartupShell(args: { if (args.platform !== 'win32' || args.isRemote) { return undefined } - // Why cmd, not the PowerShell default: with no configured shell the spawn side - // falls back to %COMSPEC% (cmd.exe) — local-pty-launch-plan.ts and - // local-pty-session-operations.ts both `... || process.env.COMSPEC || 'powershell.exe'`. - // A cold restore right after restart can run before the renderer store hydrates - // `settings`, so terminalWindowsShell is momentarily empty; predicting PowerShell - // then quotes the resume argv in single quotes that cmd.exe passes through - // literally, breaking the resume (#12320 residual: codex "unrecognized subcommand - // ''resume''", claude reads '--dangerously-skip-permissions' as a prompt). - if (!args.terminalWindowsShell?.trim()) { - return 'cmd' - } return resolveWindowsShellStartupFamily(args.terminalWindowsShell) }