From 0ab1ddec2c7f5494fab500797843d38d404eeece Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:45:52 -0700 Subject: [PATCH] fix fish startup and scoped history ownership --- src/main/daemon/daemon-pty-adapter.test.ts | 40 +++-- src/main/daemon/daemon-pty-adapter.ts | 31 +++- src/main/fish-history-session.ts | 57 +++++- src/main/host-agent-startup-shell.test.ts | 8 +- src/main/ipc/pty.ts | 12 +- src/main/ipc/worktrees.test.ts | 28 ++- src/main/ipc/worktrees.ts | 22 +-- src/main/providers/git-provider-contract.ts | 2 +- src/main/providers/local-pty-provider.test.ts | 4 + src/main/providers/local-pty-provider.ts | 5 + src/main/providers/pty-provider-contract.ts | 4 +- src/main/providers/ssh-git-provider.test.ts | 7 +- src/main/providers/ssh-git-provider.ts | 2 +- src/main/providers/ssh-pty-provider.test.ts | 10 +- src/main/providers/ssh-pty-provider.ts | 4 + src/main/providers/ssh-pty-spawn-request.ts | 4 + src/main/runtime/orca-runtime.test.ts | 8 +- src/main/runtime/orca-runtime.ts | 16 +- src/main/terminal-history-deletion.ts | 39 +++- src/main/terminal-history-gc.ts | 10 +- src/main/terminal-history-id.ts | 6 + src/main/terminal-history-paths.ts | 6 +- src/main/terminal-history.test.ts | 168 +++++++++++++----- src/main/terminal-history.ts | 103 ++++++++--- src/main/wsl-fish-history-cleanup.test.ts | 47 +++++ src/main/wsl-fish-history-cleanup.ts | 30 ++++ src/relay/fish-history-metadata.test.ts | 79 ++++++++ src/relay/fish-history-metadata.ts | 92 ++++++++++ src/relay/git-handler.test.ts | 31 ++++ src/relay/git-handler.ts | 10 +- src/relay/pty-handler.test.ts | 82 ++++++++- src/relay/pty-handler.ts | 47 ++++- .../FloatingTerminalWindowControls.tsx | 3 + .../lib/agent-resume-launch-target.test.ts | 6 +- .../src/lib/launch-work-item-direct.test.ts | 2 + .../lib/onboarding-folder-agent-startup.ts | 3 + src/shared/agent-resume-argv-drop.ts | 8 +- .../agent-resume-launch-command.test.ts | 1 + src/shared/fish-command-tokenizer.ts | 7 +- ...sh-draft-prefill-teardown.node-pty.test.ts | 34 +++- ...fish-startup-arg-quoting.live-fish.test.ts | 9 + src/shared/local-agent-startup-shell.test.ts | 14 +- src/shared/local-agent-startup-shell.ts | 8 +- src/shared/tui-agent-startup-shell.test.ts | 13 ++ src/shared/tui-agent-startup-shell.ts | 57 +++++- src/shared/tui-agent-startup.test.ts | 10 +- src/shared/windows-terminal-shell.test.ts | 8 +- src/shared/windows-terminal-shell.ts | 10 +- 48 files changed, 1009 insertions(+), 198 deletions(-) create mode 100644 src/main/terminal-history-id.ts create mode 100644 src/main/wsl-fish-history-cleanup.test.ts create mode 100644 src/main/wsl-fish-history-cleanup.ts create mode 100644 src/relay/fish-history-metadata.test.ts create mode 100644 src/relay/fish-history-metadata.ts diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 8aab1bd24d0..2cc9b66431e 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -24,18 +24,20 @@ import { getDaemonSocketPath, serializeDaemonPidFile } from './daemon-spawner' import { PtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' import { TERMINAL_HISTORY_INLINE_SEED_CODE_UNITS } from './terminal-history-seed-chunks' -const { getMacDaemonSystemResolverHealthMock, injectHistoryEnvMock } = vi.hoisted(() => ({ - getMacDaemonSystemResolverHealthMock: vi.fn(async () => 'unknown'), - injectHistoryEnvMock: vi.fn( - (env: Record, worktreeId: string, shellPath: string) => { - const fishSession = `orca_${worktreeId.replace(/\W/g, '')}` - if (shellPath.endsWith('fish')) { - env.fish_history = fishSession +const { getMacDaemonSystemResolverHealthMock, injectHistoryEnvMock, injectWslFishHistoryEnvMock } = + vi.hoisted(() => ({ + getMacDaemonSystemResolverHealthMock: vi.fn(async () => 'unknown'), + injectHistoryEnvMock: vi.fn( + (env: Record, worktreeId: string, shellPath: string) => { + const fishSession = `orca_${worktreeId.replace(/\W/g, '')}` + if (shellPath.endsWith('fish')) { + env.fish_history = fishSession + } + return { shell: 'fish', histFile: null, fishSession, historyDir: '/history' } } - return { shell: 'fish', histFile: null, fishSession, historyDir: '/history' } - } - ) -})) + ), + injectWslFishHistoryEnvMock: vi.fn() + })) const itOnPosix = process.platform === 'win32' ? it.skip : it @@ -49,6 +51,7 @@ vi.mock('./daemon-health', async (importOriginal) => { vi.mock('../terminal-history', () => ({ injectHistoryEnv: injectHistoryEnvMock, + injectWslFishHistoryEnv: injectWslFishHistoryEnvMock, logHistoryInjection: vi.fn() })) @@ -216,6 +219,21 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { expect(lastSpawnOpts?.env?.fish_history).toBeUndefined() }) + it('does not mutate scoped history on an existing-session attach', async () => { + await adapter.spawn({ cols: 80, rows: 24, sessionId: 'existing-session' }) + injectHistoryEnvMock.mockClear() + + await adapter.spawn({ + cols: 80, + rows: 24, + sessionId: 'existing-session', + worktreeId: 'repo-1::/repo', + historyIsolationEnabled: true + }) + + expect(injectHistoryEnvMock).not.toHaveBeenCalled() + }) + it('carries classified startup spans from the daemon source to the adapter', async () => { const onData = vi.fn() adapter.onData(onData) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index a3dbde1a7c4..2423e8e9632 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -68,7 +68,8 @@ import { shouldUseShellReadyStartupDelivery } from '../../shared/codex-startup-d import type { PtyIncarnationId } from '../../shared/pty-incarnation' import { resolveSafePtyDefaultCwd } from '../providers/pty-default-cwd' import { resolveUnixShellPath } from '../providers/local-pty-utils' -import { injectHistoryEnv, logHistoryInjection } from '../terminal-history' +import { injectHistoryEnv, injectWslFishHistoryEnv, logHistoryInjection } from '../terminal-history' +import { addWslEnvKeys } from '../wsl-env' import { PtyWriteUnavailableError } from '../providers/pty-write-unavailable-error' import { ColdRestorePayloadCache, type ColdRestorePayload } from './cold-restore-payload-cache' import { PtyProcessListAdmission } from '../providers/pty-process-list-admission' @@ -387,23 +388,39 @@ export class DaemonPtyAdapter implements IPtyProvider { } private withHistoryIsolation(opts: PtySpawnOptions): PtySpawnOptions { + const wslContext = resolveWslSessionContext({ + cwd: opts.cwd, + sessionId: opts.sessionId, + shellOverride: opts.shellOverride, + terminalWindowsWslDistro: opts.terminalWindowsWslDistro + }) if ( - process.platform === 'win32' || - opts.isNewSession !== true || + opts.attachOnly === true || + (opts.sessionId !== undefined && opts.isNewSession !== true) || !opts.worktreeId || - opts.historyIsolationEnabled !== true + opts.historyIsolationEnabled !== true || + (process.platform === 'win32' && !wslContext) ) { return opts } const env = { ...opts.env } - const preferredShell = opts.shellOverride || env.SHELL || process.env.SHELL || '/bin/zsh' + const preferredShell = wslContext + ? 'bash' + : opts.shellOverride || env.SHELL || process.env.SHELL || '/bin/zsh' const shellPath = resolveUnixShellPath(preferredShell) - const result = injectHistoryEnv( + const historyArgs = [ env, opts.worktreeId, shellPath, opts.cwd ?? resolveSafePtyDefaultCwd() - ) + ] as const + const result = wslContext + ? injectHistoryEnv(...historyArgs, { wslDistro: wslContext.distro }) + : injectHistoryEnv(...historyArgs) + if (wslContext) { + injectWslFishHistoryEnv(env, opts.worktreeId, wslContext.distro) + addWslEnvKeys(env, ['HISTFILE', 'fish_history']) + } logHistoryInjection(opts.worktreeId, result) return { ...opts, env } } diff --git a/src/main/fish-history-session.ts b/src/main/fish-history-session.ts index 3635f4b1f9f..4f9899a9e98 100644 --- a/src/main/fish-history-session.ts +++ b/src/main/fish-history-session.ts @@ -1,5 +1,5 @@ import { homedir } from 'node:os' -import { basename, isAbsolute, join } from 'node:path' +import { basename, dirname, isAbsolute, join } from 'node:path' import { existsSync, rmSync } from 'node:fs' /** @@ -24,6 +24,14 @@ const SESSION_PREFIX = 'orca_' // else means a caller drifted, and refusing beats deleting an unexpected path. const SAFE_SESSION_NAME = /^orca_[0-9a-f]{1,64}$/ +export const MAX_RETAINED_FISH_HISTORY_PATHS = 16 +export const MAX_FISH_HISTORY_META_BYTES = 32 * 1024 +const MAX_FISH_HISTORY_PATH_CANDIDATES = MAX_RETAINED_FISH_HISTORY_PATHS * 4 + +export function isSafeFishHistorySession(session: unknown): session is string { + return typeof session === 'string' && SAFE_SESSION_NAME.test(session) +} + export function fishHistorySessionName(worktreeHash: string): string { return `${SESSION_PREFIX}${worktreeHash}` } @@ -33,7 +41,7 @@ export function resolveFishHistoryFilePath( session: string, env: NodeJS.ProcessEnv = process.env ): string | null { - if (!SAFE_SESSION_NAME.test(session)) { + if (!isSafeFishHistorySession(session)) { return null } const xdgDataHome = env.XDG_DATA_HOME?.trim() @@ -49,16 +57,51 @@ export function resolveFishHistoryFilePath( * Accepts a path recorded at spawn time only if it still names this session's own * history file, so a tampered meta.json cannot steer `rmSync` somewhere else. */ -function isTrustedRecordedPath(session: string, path: unknown): path is string { +export function isTrustedFishHistoryPath(session: string, path: unknown): path is string { return Boolean( typeof path === 'string' && path && - SAFE_SESSION_NAME.test(session) && + isSafeFishHistorySession(session) && isAbsolute(path) && + basename(dirname(path)) === 'fish' && basename(path) === `${session}_history` ) } +export function normalizeFishHistoryPaths( + session: string, + singularPath: unknown, + paths: readonly unknown[] | undefined +): string[] { + const recentPaths = (paths ?? []).slice(-MAX_FISH_HISTORY_PATH_CANDIDATES) + const newestFirst: string[] = [] + const seen = new Set() + const retain = (path: unknown): void => { + if (!isTrustedFishHistoryPath(session, path) || seen.has(path)) { + return + } + seen.add(path) + newestFirst.push(path) + } + for (let index = recentPaths.length - 1; index >= 0; index -= 1) { + retain(recentPaths[index]) + if (newestFirst.length === MAX_RETAINED_FISH_HISTORY_PATHS) { + break + } + } + if (newestFirst.length < MAX_RETAINED_FISH_HISTORY_PATHS) { + retain(singularPath) + } + const retained: string[] = [] + for (let index = newestFirst.length - 1; index >= 0; index -= 1) { + if (retained.length === MAX_RETAINED_FISH_HISTORY_PATHS) { + break + } + retained.push(newestFirst[index]) + } + return retained +} + /** * Removes one worktree's fish history file. * @@ -80,11 +123,11 @@ export function deleteFishHistoryFile( } = {} ): void { const candidates = new Set() - if (isTrustedRecordedPath(session, options.recordedPath)) { + if (isTrustedFishHistoryPath(session, options.recordedPath)) { candidates.add(options.recordedPath) } - for (const path of options.recordedPaths ?? []) { - if (isTrustedRecordedPath(session, path)) { + for (const path of (options.recordedPaths ?? []).slice(0, MAX_RETAINED_FISH_HISTORY_PATHS)) { + if (isTrustedFishHistoryPath(session, path)) { candidates.add(path) } } diff --git a/src/main/host-agent-startup-shell.test.ts b/src/main/host-agent-startup-shell.test.ts index 69ad898e634..b358b3aff2c 100644 --- a/src/main/host-agent-startup-shell.test.ts +++ b/src/main/host-agent-startup-shell.test.ts @@ -24,11 +24,11 @@ describe('resolveHostAgentStartupShell', () => { ) }) - it('stays on the sh default for a remote workspace', () => { + it('uses portable Unix syntax for a remote workspace', () => { process.env.SHELL = '/opt/homebrew/bin/fish' - expect( - resolveHostAgentStartupShell({ platform: process.platform, isRemote: true }) - ).toBeUndefined() + expect(resolveHostAgentStartupShell({ platform: process.platform, isRemote: true })).toBe( + 'unix' + ) }) it('stays on the sh default when the workspace platform is not this host', () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 8c256692213..531f1856718 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -4694,11 +4694,7 @@ export function registerPtyHandlers( cwd, env, ...(isNewDaemonSession ? { isNewSession: true } : {}), - ...(isDaemonHostSpawn - ? { - historyIsolationEnabled: getSettings?.()?.terminalScopeHistoryByWorktree ?? true - } - : {}) + historyIsolationEnabled: getSettings?.()?.terminalScopeHistoryByWorktree ?? true } if (!args.connectionId && !isDaemonHostSpawn) { spawnOptions.codexHomePathOverride = { value: selectedCodexHomePath } @@ -6383,11 +6379,7 @@ export function registerPtyHandlers( ...(prevalidatedCwd && !isDaemonHostSpawn ? { prevalidatedCwd } : {}), env: spawnEnv, ...(isMintedSessionId ? { isNewSession: true } : {}), - ...(isDaemonHostSpawn - ? { - historyIsolationEnabled: getSettings?.()?.terminalScopeHistoryByWorktree ?? true - } - : {}) + historyIsolationEnabled: getSettings?.()?.terminalScopeHistoryByWorktree ?? true } if (!args.connectionId && !isDaemonHostSpawn) { spawnOptions.codexHomePathOverride = { value: selectedCodexHomePath } diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 02f23bc094a..b37009d49dd 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -8624,7 +8624,8 @@ describe('registerWorktreeHandlers', () => { // Folder projects can be SSH-backed, and folder workspace ids are `repoId::path::workspace:` // — reusable across hosts — so the sweep must name the owning connection. it('fences an SSH folder workspace PTY sweep to the owning connection', async () => { - const sshPtyProvider = { id: 'ssh-pty-provider' } as never + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + const sshPtyProvider = { id: 'ssh-pty-provider', deleteWorktreeHistory } as never const worktreeId = 'repo-folder::/remote/folder::workspace:child-1' store.getRepo.mockReturnValue({ id: 'repo-folder', @@ -8651,6 +8652,7 @@ describe('registerWorktreeHandlers', () => { includeProviderInventory: true, includeLocalRegistry: false }) + expect(deleteWorktreeHistory).toHaveBeenCalledWith(worktreeId) }) it('fences a mirrored runtime folder workspace sweep to its environment', async () => { @@ -8893,7 +8895,9 @@ describe('registerWorktreeHandlers', () => { ORCA_WORKTREE_PATH: '/remote/feature-wt' }) ) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { + worktreeId: 'repo-ssh::/remote/feature-wt' + }) expect(runtimeStub.closeFileWatchersForRemoval).toHaveBeenCalledWith( '/remote/feature-wt', 'conn-1' @@ -9019,7 +9023,9 @@ describe('registerWorktreeHandlers', () => { }) expect(provider.worktreeIsClean).not.toHaveBeenCalled() - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', true) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', true, { + worktreeId: 'repo-ssh::/remote/feature-wt' + }) }) it('continues SSH worktree removal when the archive hook fails', async () => { @@ -9075,7 +9081,9 @@ describe('registerWorktreeHandlers', () => { await handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { + worktreeId: 'repo-ssh::/remote/feature-wt' + }) expect(consoleErrorSpy).toHaveBeenCalledWith( '[hooks] archive hook failed for /remote/feature-wt:', expect.stringContaining('archive hook exited 7') @@ -9133,7 +9141,9 @@ describe('registerWorktreeHandlers', () => { await handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { + worktreeId: 'repo-ssh::/remote/feature-wt' + }) expect(consoleErrorSpy).toHaveBeenCalledWith( '[hooks] archive hook failed for /remote/feature-wt:', 'relay disconnected' @@ -9258,7 +9268,9 @@ describe('registerWorktreeHandlers', () => { }) expect(provider.execNonInteractive).not.toHaveBeenCalled() - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { + worktreeId: 'repo-ssh::/remote/feature-wt' + }) }) it('uses the workspace host when duplicate repo ids exist across local and SSH', async () => { @@ -9305,7 +9317,9 @@ describe('registerWorktreeHandlers', () => { hostId: 'ssh:conn-1' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { + worktreeId: 'repo-shared::/remote/feature-wt' + }) expect(removeWorktreeMock).not.toHaveBeenCalled() }) diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index ab299057169..cb0642b3daf 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -2386,15 +2386,15 @@ export function registerWorktreeHandlers( 'Cannot delete the project root workspace. Remove the folder project instead.' ) } + const ownerHost = parseExecutionHostId(removalHostId) + const sshPtyProvider = + ownerHost?.kind === 'ssh' ? getSshPtyProvider(ownerHost.targetId) : undefined // Why: folder workspaces share one root, so there's no Git remove step to close shells; sweep PTYs before dropping metadata. await withWorktreeRemoveStageSpan('pty_sweep', 'folder', async () => { // Folder projects can be SSH-backed, so fence the sweep to the owning host exactly // like the git paths — the local inventory must never reach a remote workspace's id. // The resolved repo is authoritative here: path-derived metadata is shared by // same-id host copies and can describe a different owner's workspace. - const ownerHost = parseExecutionHostId(removalHostId) - const sshPtyProvider = - ownerHost?.kind === 'ssh' ? getSshPtyProvider(ownerHost.targetId) : undefined const externalHost = ownerHost?.kind === 'ssh' || ownerHost?.kind === 'runtime' await killAllProcessesForWorktree(args.worktreeId, { runtime, @@ -2417,6 +2417,9 @@ export function registerWorktreeHandlers( }) }) await withWorktreeRemoveStageSpan('metadata_purge', 'folder', async () => { + await sshPtyProvider?.deleteWorktreeHistory?.(args.worktreeId).catch((err) => { + console.warn(`[pty:history] remote folder cleanup failed:`, err) + }) removeWorktreeMetadataAndTransientState(store, args.worktreeId, removalHostId) }) preservedBranchCleanupByScope.delete( @@ -2723,7 +2726,10 @@ export function registerWorktreeHandlers( } } - const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} + const remoteRemoveOptions = { + ...(!deleteBranch ? { deleteBranch } : {}), + worktreeId: args.worktreeId + } const removalGate = await withWorktreeRemoveStageSpan( 'watcher_gate', 'remote', @@ -2743,13 +2749,7 @@ export function registerWorktreeHandlers( 'git_remove', 'remote', async () => - Object.keys(remoteRemoveOptions).length > 0 - ? provider!.removeWorktree( - canonicalWorktreePath, - args.force, - remoteRemoveOptions - ) - : provider!.removeWorktree(canonicalWorktreePath, args.force) + provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) ) removalCompleted = true } finally { diff --git a/src/main/providers/git-provider-contract.ts b/src/main/providers/git-provider-contract.ts index d13bf5addf9..8b19a9c34f7 100644 --- a/src/main/providers/git-provider-contract.ts +++ b/src/main/providers/git-provider-contract.ts @@ -82,7 +82,7 @@ export type IGitProvider = { removeWorktree( worktreePath: string, force?: boolean, - options?: { deleteBranch?: boolean; forceBranchDelete?: boolean } + options?: { deleteBranch?: boolean; forceBranchDelete?: boolean; worktreeId?: string } ): Promise renameCurrentBranch?(worktreePath: string, newBranch: string): Promise forceDeletePreservedBranch?( diff --git a/src/main/providers/local-pty-provider.test.ts b/src/main/providers/local-pty-provider.test.ts index c9e20e717c1..08bdf3e2c27 100644 --- a/src/main/providers/local-pty-provider.test.ts +++ b/src/main/providers/local-pty-provider.test.ts @@ -1059,6 +1059,10 @@ describe('LocalPtyProvider', () => { ]) expect(spawnCall[1][5]).toContain('exec "\\$_orca_wsl_shell" -l') expect(spawnCall[2].env.HISTFILE).toContain('terminal-history-wsl/Debian') + expect(spawnCall[2].env.fish_history).toMatch(/^orca_[0-9a-f]{16}$/) + expect(spawnCall[2].env.WSLENV?.split(':')).toEqual( + expect.arrayContaining(['HISTFILE', 'fish_history']) + ) }) it.each(['/home/jin/repo', '/a', '/c'])( diff --git a/src/main/providers/local-pty-provider.ts b/src/main/providers/local-pty-provider.ts index fa9872d7456..436b225a76b 100644 --- a/src/main/providers/local-pty-provider.ts +++ b/src/main/providers/local-pty-provider.ts @@ -16,6 +16,7 @@ import { splitWorktreeIdForFilesystem } from '../../shared/worktree-id' import { isBracketedPasteSafeShell } from '../../shared/startup-command-submission' import { injectHistoryEnv, + injectWslFishHistoryEnv, updateHistoryEnvForFallback, logHistoryInjection, type HistoryInjectionResult @@ -847,6 +848,10 @@ export class LocalPtyProvider implements IPtyProvider { historyResult = injectHistoryEnv(finalEnv, worktreeId, effectiveShellPath, cwd, { wslDistro: launchWslDistro }) + if (isWslTerminal && launchWslDistro) { + injectWslFishHistoryEnv(finalEnv, worktreeId, launchWslDistro) + addWslEnvKeys(finalEnv, ['HISTFILE', 'fish_history']) + } logHistoryInjection(worktreeId, historyResult) } diff --git a/src/main/providers/pty-provider-contract.ts b/src/main/providers/pty-provider-contract.ts index 6fa346cc04f..aa1bf98c3b3 100644 --- a/src/main/providers/pty-provider-contract.ts +++ b/src/main/providers/pty-provider-contract.ts @@ -70,7 +70,7 @@ export type PtySpawnOptions = { * Existing-session attach paths must stay false so recovery checks do not * replace the daemon out from under a still-live PTY. */ isNewSession?: boolean - /** Host setting forwarded only to a local daemon adapter; never serialized on the daemon wire. */ + /** Host setting forwarded additively to the process owner; old owners ignore it. */ historyIsolationEnabled?: boolean /** Attach the named session atomically or fail without creating a process. */ attachOnly?: boolean @@ -116,6 +116,8 @@ export type IPtyProvider = { /** Re-probes a degraded durable host before main commits to fallback spawn semantics. */ recoverFreshSpawnRouting?: () => Promise spawn(opts: PtySpawnOptions): Promise + /** Process-owner cleanup for history stored outside the workspace tree. */ + deleteWorktreeHistory?: (worktreeId: string) => Promise /** Whether this spawn target can append the Git guard after its final env merge. */ supportsGitCredentialGuardHost?: (sessionId?: string) => boolean /** Explicit false selects pre-claim legacy spawn for a preserved old daemon. */ diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 20ebd9293a1..71c85d6bef7 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -1566,10 +1566,13 @@ describe('SshGitProvider', () => { }) it('removeWorktree sends git.removeWorktree request', async () => { - await provider.removeWorktree('/home/user/feat', true) + await provider.removeWorktree('/home/user/feat', true, { + worktreeId: 'repo-1::/home/user/feat' + }) expect(mux.request).toHaveBeenCalledWith('git.removeWorktree', { worktreePath: '/home/user/feat', - force: true + force: true, + worktreeId: 'repo-1::/home/user/feat' }) }) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index 51e2b91e1c2..ee791e2d1e3 100644 --- a/src/main/providers/ssh-git-provider.ts +++ b/src/main/providers/ssh-git-provider.ts @@ -740,7 +740,7 @@ export class SshGitProvider implements IGitProvider { async removeWorktree( worktreePath: string, force?: boolean, - options?: { deleteBranch?: boolean; forceBranchDelete?: boolean } + options?: { deleteBranch?: boolean; forceBranchDelete?: boolean; worktreeId?: string } ): Promise { return this.runWithDiffDedupeClear( async () => diff --git a/src/main/providers/ssh-pty-provider.test.ts b/src/main/providers/ssh-pty-provider.test.ts index 40301906f52..b3b64be7a71 100644 --- a/src/main/providers/ssh-pty-provider.test.ts +++ b/src/main/providers/ssh-pty-provider.test.ts @@ -369,14 +369,16 @@ describe('SshPtyProvider', () => { }) }) - it('forwards explicit shellOverride and terminalWindowsWslDistro to the relay mux', async () => { + it('forwards shell, WSL, and scoped-history options to the relay mux', async () => { mux.request.mockResolvedValue({ id: 'pty-2' }) await provider.spawn({ cols: 120, rows: 40, shellOverride: 'powershell.exe', - terminalWindowsWslDistro: 'Ubuntu' + terminalWindowsWslDistro: 'Ubuntu', + worktreeId: 'repo-1::/remote/wt', + historyIsolationEnabled: true }) expectRequest(mux.request, 'pty.spawn', { @@ -385,7 +387,9 @@ describe('SshPtyProvider', () => { cwd: undefined, env: { [POWERLEVEL10K_WIZARD_DISABLE_ENV]: 'true' }, shellOverride: 'powershell.exe', - terminalWindowsWslDistro: 'Ubuntu' + terminalWindowsWslDistro: 'Ubuntu', + worktreeId: 'repo-1::/remote/wt', + historyIsolationEnabled: true }) }) diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index 2bc52fe491d..9a8ab4cc2e1 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -147,6 +147,10 @@ export class SshPtyProvider implements IPtyProvider { }) } + async deleteWorktreeHistory(worktreeId: string): Promise { + await this.mux.request('pty.deleteWorktreeHistory', { worktreeId }) + } + async supportsAgentSessionClaims(options: { signal?: AbortSignal } = {}): Promise { return await this.agentSessionCapabilities.supportsClaims(options) } diff --git a/src/main/providers/ssh-pty-spawn-request.ts b/src/main/providers/ssh-pty-spawn-request.ts index 889c0813ee9..9f493d9e778 100644 --- a/src/main/providers/ssh-pty-spawn-request.ts +++ b/src/main/providers/ssh-pty-spawn-request.ts @@ -22,6 +22,10 @@ export function buildSshPtySpawnRequest(args: { // Why: the relay needs launch identity for plugin env overlays and provider-side delivery. ...(options.command ? { command: options.command } : {}), ...(options.launchAgent ? { launchAgent: options.launchAgent } : {}), + ...(options.worktreeId ? { worktreeId: options.worktreeId } : {}), + ...(options.historyIsolationEnabled !== undefined + ? { historyIsolationEnabled: options.historyIsolationEnabled } + : {}), ...(options.shellOverride !== undefined ? { shellOverride: options.shellOverride } : {}), ...(options.terminalWindowsWslDistro !== undefined ? { terminalWindowsWslDistro: options.terminalWindowsWslDistro } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index ec4d930ad79..517f22ade34 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -6064,7 +6064,9 @@ describe('OrcaRuntimeService', () => { unregisterSshGitProvider('ssh-1') } - expect(gitProvider.removeWorktree).toHaveBeenCalledWith('/remote/feature', true) + expect(gitProvider.removeWorktree).toHaveBeenCalledWith('/remote/feature', true, { + worktreeId: `${TEST_REPO_ID}::/remote/feature` + }) expect(ptyProvider.shutdown).toHaveBeenCalledWith( 'pty-remote', expect.objectContaining({ immediate: true }) @@ -32391,7 +32393,7 @@ describe('OrcaRuntimeService', () => { expect(spawn).not.toHaveBeenCalled() }) - it('uses POSIX quoting for mobile agent launch commands in WSL project runtimes', async () => { + it('uses portable Unix quoting for mobile agent launch commands in WSL project runtimes', async () => { await withPlatform('win32', async () => { const spawn = vi.fn().mockResolvedValue({ id: 'pty-agent' }) const runtime = new OrcaRuntimeService({ @@ -32429,7 +32431,7 @@ describe('OrcaRuntimeService', () => { expect(spawn).toHaveBeenCalledWith( expect.objectContaining({ - command: "command-code --profile mobile '--note' 'can'\\''t'", + command: `command-code --profile mobile '--note' 'can'"'"'t'`, cwd: TEST_WORKTREE_PATH, worktreeId: TEST_WORKTREE_ID }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3a713aa8295..a119855aee3 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -24495,6 +24495,9 @@ export class OrcaRuntimeService { console.warn(`[worktree-teardown] failed for ${removalTarget.id}:`, err) }) } + await folderSshPtyProvider?.deleteWorktreeHistory?.(removalTarget.id).catch((err) => { + console.warn(`[pty:history] remote folder cleanup failed:`, err) + }) this.removeWorktreeMetadataAndHistory(store, removalTarget.id) this.preservedBranchCleanupByScope.delete(cleanupScopeKey) this.invalidateResolvedWorktreeCache() @@ -24747,7 +24750,10 @@ export class OrcaRuntimeService { return removalResult ?? {} } if (repo.connectionId) { - const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} + const remoteRemoveOptions = { + ...(!deleteBranch ? { deleteBranch } : {}), + worktreeId: removalTarget.id + } const removalGate = await this.acquireFileWatcherRemoval( canonicalWorktreePath, repo.connectionId @@ -24759,9 +24765,11 @@ export class OrcaRuntimeService { connectionId: repo.connectionId, allowUnverifiedStop: allowUnverifiedPtyStop }) - rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 - ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) - : provider!.removeWorktree(canonicalWorktreePath, force)) + rawRemovalResult = await provider!.removeWorktree( + canonicalWorktreePath, + force, + remoteRemoveOptions + ) removalCompleted = true } finally { await removalGate.finish(removalCompleted) diff --git a/src/main/terminal-history-deletion.ts b/src/main/terminal-history-deletion.ts index 4cd421cf840..814015a0eca 100644 --- a/src/main/terminal-history-deletion.ts +++ b/src/main/terminal-history-deletion.ts @@ -1,14 +1,15 @@ -import { basename, join } from 'node:path' +import { basename, dirname, join } from 'node:path' import { existsSync, mkdirSync, readdirSync, renameSync } from 'node:fs' import { removeHostTree } from './host-tree-removal' import { deleteFishHistoryFile } from './fish-history-session' import { readHistoryMeta } from './terminal-history' import { getHistoryRoot, - hashWorktreeId, listWslHistoryRoots, PENDING_DELETE_DIR_NAME } from './terminal-history-paths' +import { hashWorktreeId } from './terminal-history-id' +import { deleteWslFishHistoryFile } from './wsl-fish-history-cleanup' const pendingHistoryTreeRemovals = new Map>() // Why: a tombstone that fails once (Windows EBUSY under AV) would otherwise sit on disk for the whole @@ -16,6 +17,13 @@ const pendingHistoryTreeRemovals = new Map>() export const HISTORY_TREE_REMOVAL_RETRY_DELAYS_MS = [30_000, 120_000] const historyTreeRemovalAttempts = new Map() const historyTreeRemovalRetryTimers = new Map>() +const wslDistroByTombstone = new Map() + +function wslDistroForHistoryRoot(historyRoot: string): string | undefined { + return basename(dirname(historyRoot)) === 'terminal-history-wsl' + ? basename(historyRoot) + : undefined +} function getPendingDeleteRoot(historyRoot: string): string { return join(historyRoot, PENDING_DELETE_DIR_NAME) @@ -53,6 +61,7 @@ function scheduleHistoryTreeRemovalRetry(dir: string): void { if (retryDelayMs === undefined) { // Out of in-process attempts: the tombstone stays on disk and the next startup drain re-queues it. historyTreeRemovalAttempts.delete(dir) + wslDistroByTombstone.delete(dir) return } historyTreeRemovalAttempts.set(dir, attempt + 1) @@ -64,7 +73,7 @@ function scheduleHistoryTreeRemovalRetry(dir: string): void { historyTreeRemovalRetryTimers.set(dir, timer) } -function scheduleHistoryTreeRemoval(dir: string): void { +function scheduleHistoryTreeRemoval(dir: string, wslDistro?: string): void { if (pendingHistoryTreeRemovals.has(dir)) { return } @@ -74,9 +83,23 @@ function scheduleHistoryTreeRemoval(dir: string): void { clearTimeout(pendingRetry) historyTreeRemovalRetryTimers.delete(dir) } - const removal = removeHostTree(dir) + if (wslDistro) { + wslDistroByTombstone.set(dir, wslDistro) + } + const cleanupDistro = wslDistroByTombstone.get(dir) + const meta = cleanupDistro ? readHistoryMeta(dir) : null + const cleanup = + cleanupDistro && meta?.fishSession + ? deleteWslFishHistoryFile(cleanupDistro, meta.fishSession).catch((err: unknown) => { + console.warn( + `[pty:history] Failed to delete WSL fish history: ${err instanceof Error ? err.message : String(err)}` + ) + }) + : null + const removal = (cleanup ? cleanup.then(() => removeHostTree(dir)) : removeHostTree(dir)) .then(() => { historyTreeRemovalAttempts.delete(dir) + wslDistroByTombstone.delete(dir) }) .catch((err: unknown) => { console.warn( @@ -98,7 +121,8 @@ export function scheduleWorktreeHistoryTreeDeletion(dir: string, historyRoot: st // Why first: fish keeps its history in the user's fish data dir, outside this tree, // so the meta.json naming the session must still be readable when we look it up. const meta = readHistoryMeta(dir) - if (meta?.fishSession) { + const wslDistro = wslDistroForHistoryRoot(historyRoot) + if (meta?.fishSession && !wslDistro) { deleteFishHistoryFile(meta.fishSession, { recordedPath: meta.fishHistoryPath, recordedPaths: meta.fishHistoryPaths @@ -108,7 +132,7 @@ export function scheduleWorktreeHistoryTreeDeletion(dir: string, historyRoot: st if (!tombstone) { return false } - scheduleHistoryTreeRemoval(tombstone) + scheduleHistoryTreeRemoval(tombstone, wslDistro) return true } @@ -120,7 +144,7 @@ export function schedulePendingHistoryTreeRemovals(historyRoot: string): void { } try { for (const entry of readdirSync(pendingRoot)) { - scheduleHistoryTreeRemoval(join(pendingRoot, entry)) + scheduleHistoryTreeRemoval(join(pendingRoot, entry), wslDistroForHistoryRoot(historyRoot)) } } catch { // Non-fatal. @@ -142,6 +166,7 @@ export function cancelPendingHistoryTreeRemovalRetries(): void { } historyTreeRemovalRetryTimers.clear() historyTreeRemovalAttempts.clear() + wslDistroByTombstone.clear() } /** Drain every history root's tombstones and await the in-flight removals. Tests only: production diff --git a/src/main/terminal-history-gc.ts b/src/main/terminal-history-gc.ts index 2a6d2280945..1d5481a7b6d 100644 --- a/src/main/terminal-history-gc.ts +++ b/src/main/terminal-history-gc.ts @@ -1,5 +1,5 @@ import { join } from 'node:path' -import { existsSync, readFileSync, readdirSync, statSync } from 'node:fs' +import { existsSync, readdirSync, statSync } from 'node:fs' import { getHistoryRoot, listWslHistoryRoots, @@ -9,6 +9,7 @@ import { schedulePendingHistoryTreeRemovals, scheduleWorktreeHistoryTreeDeletion } from './terminal-history-deletion' +import { readHistoryMeta } from './terminal-history' // Why 5 minutes: GC runs ~10s after startup, and the live-worktree snapshot is // taken just before. A worktree created between the snapshot and GC execution @@ -61,11 +62,8 @@ function gcScanRoot( continue } - const meta = JSON.parse(readFileSync(metaPath, 'utf-8')) as { - worktreeId?: string - createdAt?: string - } - if (!meta.worktreeId) { + const meta = readHistoryMeta(entryPath) + if (!meta?.worktreeId) { continue } diff --git a/src/main/terminal-history-id.ts b/src/main/terminal-history-id.ts new file mode 100644 index 00000000000..38454baa29e --- /dev/null +++ b/src/main/terminal-history-id.ts @@ -0,0 +1,6 @@ +import { createHash } from 'node:crypto' + +/** First 16 hex chars of SHA-256 of the worktreeId. */ +export function hashWorktreeId(worktreeId: string): string { + return createHash('sha256').update(worktreeId).digest('hex').slice(0, 16) +} diff --git a/src/main/terminal-history-paths.ts b/src/main/terminal-history-paths.ts index e942dd8bc3f..263f39c8c35 100644 --- a/src/main/terminal-history-paths.ts +++ b/src/main/terminal-history-paths.ts @@ -1,4 +1,3 @@ -import { createHash } from 'node:crypto' import { existsSync, readdirSync } from 'node:fs' import { join } from 'node:path' import { app } from 'electron' @@ -8,10 +7,7 @@ const HISTORY_DIR_NAME_WSL = 'terminal-history-wsl' // Why: rename live history out of the way first so a quit mid-rm still leaves a durable tombstone GC can finish. export const PENDING_DELETE_DIR_NAME = '.pending-delete' -/** First 16 hex chars of SHA-256 of the worktreeId. */ -export function hashWorktreeId(worktreeId: string): string { - return createHash('sha256').update(worktreeId).digest('hex').slice(0, 16) -} +export { hashWorktreeId } from './terminal-history-id' export function getHistoryRoot(): string { return join(app.getPath('userData'), HISTORY_DIR_NAME) diff --git a/src/main/terminal-history.test.ts b/src/main/terminal-history.test.ts index 81bfdf59110..f73774cdeba 100644 --- a/src/main/terminal-history.test.ts +++ b/src/main/terminal-history.test.ts @@ -12,7 +12,8 @@ const { readdirSyncMock, statSyncMock, getPathMock, - rmAsyncMock + rmAsyncMock, + deleteWslFishHistoryFileMock } = vi.hoisted(() => ({ existsSyncMock: vi.fn(), mkdirSyncMock: vi.fn(), @@ -23,7 +24,8 @@ const { readdirSyncMock: vi.fn(), statSyncMock: vi.fn(), getPathMock: vi.fn(), - rmAsyncMock: vi.fn(async () => undefined) + rmAsyncMock: vi.fn(async () => undefined), + deleteWslFishHistoryFileMock: vi.fn(async () => undefined) })) vi.mock('fs', () => ({ @@ -60,19 +62,30 @@ vi.mock('./wsl', () => ({ toLinuxPath: toLinuxPathMock })) +vi.mock('./wsl-fish-history-cleanup', () => ({ + deleteWslFishHistoryFile: deleteWslFishHistoryFileMock +})) + import { resolveShellKind, ensureHistoryDir, injectHistoryEnv, + MAX_HISTORY_META_BYTES, updateHistoryEnvForFallback, type HistoryInjectionResult } from './terminal-history' -import { fishHistorySessionName, resolveFishHistoryFilePath } from './fish-history-session' +import { + fishHistorySessionName, + MAX_RETAINED_FISH_HISTORY_PATHS, + resolveFishHistoryFilePath +} from './fish-history-session' import { hashWorktreeId } from './terminal-history-paths' import { deleteWorktreeHistoryDir, flushPendingWorktreeHistoryDeletions } from './terminal-history-deletion' + +type FishHistoryMeta = { fishHistoryPath: string; fishHistoryPaths: string[] } import { runHistoryGc, scheduleHistoryGc } from './terminal-history-gc' describe('terminal-history', () => { @@ -273,7 +286,7 @@ describe('terminal-history', () => { } }) - it('retains every fish path when XDG_DATA_HOME changes', () => { + it('retains fish paths when XDG_DATA_HOME changes and points rollback at the active path', () => { const firstEnv = { XDG_DATA_HOME: ['', 'data', 'one'].join(sep) } const secondEnv = { XDG_DATA_HOME: ['', 'data', 'two'].join(sep) } const worktreeId = 'repo-1::/path/wt' @@ -282,20 +295,17 @@ describe('terminal-history', () => { readFileSyncMock.mockReturnValue(firstMeta) injectHistoryEnv(secondEnv, worktreeId, '/usr/bin/fish', '/path/wt') - const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as { - fishHistoryPath: string - fishHistoryPaths: string[] - } + const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as FishHistoryMeta const filename = `${first.fishSession}_history` - expect(meta.fishHistoryPath).toBe(['', 'data', 'one', 'fish', filename].join(sep)) + expect(meta.fishHistoryPath).toBe(['', 'data', 'two', 'fish', filename].join(sep)) expect(meta.fishHistoryPaths).toEqual([ ['', 'data', 'one', 'fish', filename].join(sep), ['', 'data', 'two', 'fish', filename].join(sep) ]) }) - it('migrates legacy singular fish metadata without replacing its rollback path', () => { + it('migrates legacy singular fish metadata and points rollback at the active path', () => { const worktreeId = 'repo-1::/path/wt' const session = fishHistorySessionName(hashWorktreeId(worktreeId)) const legacyPath = ['', 'legacy', 'fish', `${session}_history`].join(sep) @@ -309,18 +319,57 @@ describe('terminal-history', () => { '/usr/bin/fish', '/path/wt' ) - const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as { - fishHistoryPath: string - fishHistoryPaths: string[] - } + const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as FishHistoryMeta - expect(meta.fishHistoryPath).toBe(legacyPath) + expect(meta.fishHistoryPath).toBe(['', 'current', 'fish', `${session}_history`].join(sep)) expect(meta.fishHistoryPaths).toEqual([ legacyPath, ['', 'current', 'fish', `${session}_history`].join(sep) ]) }) + it('bounds and normalizes retained fish paths while preserving the active path', () => { + const worktreeId = 'repo-1::/path/wt' + const session = fishHistorySessionName(hashWorktreeId(worktreeId)) + const filename = `${session}_history` + const paths = Array.from({ length: 40 }, (_, index) => + ['', 'data', String(index), 'fish', filename].join(sep) + ) + readFileSyncMock.mockReturnValue( + JSON.stringify({ + worktreeId, + fishSession: session, + fishHistoryPath: paths[0], + fishHistoryPaths: paths + }) + ) + + injectHistoryEnv( + { XDG_DATA_HOME: ['', 'active'].join(sep) }, + worktreeId, + '/usr/bin/fish', + '/path/wt' + ) + const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as FishHistoryMeta + const activePath = ['', 'active', 'fish', filename].join(sep) + + expect(meta.fishHistoryPaths).toHaveLength(MAX_RETAINED_FISH_HISTORY_PATHS) + expect(new Set(meta.fishHistoryPaths).size).toBe(meta.fishHistoryPaths.length) + expect(meta.fishHistoryPaths.at(-1)).toBe(activePath) + expect(meta.fishHistoryPath).toBe(activePath) + }) + + it('replaces oversized metadata without parsing it', () => { + statSyncMock.mockReturnValue({ isDirectory: () => true, size: MAX_HISTORY_META_BYTES + 1 }) + const env = { XDG_DATA_HOME: ['', 'active'].join(sep) } + + injectHistoryEnv(env, 'repo-1::/path/wt', '/usr/bin/fish', '/path/wt') + + expect(readFileSyncMock).not.toHaveBeenCalled() + const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as FishHistoryMeta + expect(meta.fishHistoryPaths).toEqual([meta.fishHistoryPath]) + }) + it('gives two fish worktrees different history sessions', () => { const envA: Record = {} injectHistoryEnv(envA, 'repo-1::/path/wt-a', '/usr/bin/fish', '/path/wt-a') @@ -425,6 +474,10 @@ describe('terminal-history', () => { }) describe('deleteWorktreeHistoryDir', () => { + const worktreeId = 'repo-1::/path/wt' + const session = fishHistorySessionName(hashWorktreeId(worktreeId)) + const historyFilename = `${session}_history` + it('tombstones then async-removes the history directory without recursive rmSync', async () => { existsSyncMock.mockReturnValue(true) deleteWorktreeHistoryDir('repo-1::/path/wt') @@ -443,12 +496,10 @@ describe('terminal-history', () => { process.env.XDG_DATA_HOME = ['', 'pinned', 'data'].join(sep) try { existsSyncMock.mockReturnValue(true) - readFileSyncMock.mockReturnValue( - JSON.stringify({ worktreeId: 'repo-1::/path/wt', fishSession: 'orca_abc123' }) - ) + readFileSyncMock.mockReturnValue(JSON.stringify({ worktreeId, fishSession: session })) deleteWorktreeHistoryDir('repo-1::/path/wt') expect(rmSyncMock).toHaveBeenCalledWith( - ['', 'pinned', 'data', 'fish', 'orca_abc123_history'].join(sep), + ['', 'pinned', 'data', 'fish', historyFilename].join(sep), expect.objectContaining({ force: true }) ) } finally { @@ -464,13 +515,13 @@ describe('terminal-history', () => { it('prefers the spawn-env path recorded in meta.json over this process env', async () => { const originalDataHome = process.env.XDG_DATA_HOME process.env.XDG_DATA_HOME = ['', 'main', 'data'].join(sep) - const recorded = ['', 'spawn', 'data', 'fish', 'orca_abc123_history'].join(sep) + const recorded = ['', 'spawn', 'data', 'fish', historyFilename].join(sep) try { existsSyncMock.mockReturnValue(true) readFileSyncMock.mockReturnValue( JSON.stringify({ - worktreeId: 'repo-1::/path/wt', - fishSession: 'orca_abc123', + worktreeId, + fishSession: session, fishHistoryPath: recorded }) ) @@ -478,7 +529,7 @@ describe('terminal-history', () => { expect(rmSyncMock).toHaveBeenCalledWith(recorded, expect.objectContaining({ force: true })) // The main-process guess stays as a fallback for pre-existing meta.json files. expect(rmSyncMock).toHaveBeenCalledWith( - ['', 'main', 'data', 'fish', 'orca_abc123_history'].join(sep), + ['', 'main', 'data', 'fish', historyFilename].join(sep), expect.objectContaining({ force: true }) ) } finally { @@ -492,12 +543,12 @@ describe('terminal-history', () => { }) it('deletes every recorded fish path while isolating individual failures', async () => { - const first = ['', 'data', 'one', 'fish', 'orca_abc123_history'].join(sep) - const second = ['', 'data', 'two', 'fish', 'orca_abc123_history'].join(sep) + const first = ['', 'data', 'one', 'fish', historyFilename].join(sep) + const second = ['', 'data', 'two', 'fish', historyFilename].join(sep) readFileSyncMock.mockReturnValue( JSON.stringify({ - worktreeId: 'repo-1::/path/wt', - fishSession: 'orca_abc123', + worktreeId, + fishSession: session, fishHistoryPath: first, fishHistoryPaths: [first, second] }) @@ -515,11 +566,35 @@ describe('terminal-history', () => { await flushPendingWorktreeHistoryDeletions() }) + it('bounds synchronous fish cleanup for oversized retained arrays', async () => { + const paths = Array.from({ length: 100 }, (_, index) => + ['', 'data', String(index), 'fish', `${session}_history`].join(sep) + ) + readFileSyncMock.mockReturnValue( + JSON.stringify({ + worktreeId, + fishSession: session, + fishHistoryPath: paths[0], + fishHistoryPaths: paths + }) + ) + + deleteWorktreeHistoryDir('repo-1::/path/wt') + + expect(rmSyncMock.mock.calls.length).toBeLessThanOrEqual(MAX_RETAINED_FISH_HISTORY_PATHS + 1) + expect(rmSyncMock).toHaveBeenCalledWith( + paths.at(-1), + expect.objectContaining({ force: true }) + ) + expect(rmSyncMock).not.toHaveBeenCalledWith(paths[0], expect.anything()) + await flushPendingWorktreeHistoryDeletions() + }) + it('runtime-validates malformed fish metadata before the normal tombstone', async () => { readFileSyncMock.mockReturnValue( JSON.stringify({ - worktreeId: 'repo-1::/path/wt', - fishSession: 'orca_abc123', + worktreeId, + fishSession: session, fishHistoryPath: 42, fishHistoryPaths: [null, 7, { path: '/tmp/nope' }] }) @@ -530,23 +605,21 @@ describe('terminal-history', () => { await flushPendingWorktreeHistoryDeletions() }) - it('refuses a recorded path that does not name its own session file', async () => { + it('refuses a matching session filename outside a fish data directory', async () => { const originalDataHome = process.env.XDG_DATA_HOME process.env.XDG_DATA_HOME = ['', 'main', 'data'].join(sep) + const unrelated = ['', 'tmp', historyFilename].join(sep) try { existsSyncMock.mockReturnValue(true) readFileSyncMock.mockReturnValue( JSON.stringify({ - worktreeId: 'repo-1::/path/wt', - fishSession: 'orca_abc123', - fishHistoryPath: ['', 'etc', 'passwd'].join(sep) + worktreeId, + fishSession: session, + fishHistoryPath: unrelated }) ) deleteWorktreeHistoryDir('repo-1::/path/wt') - expect(rmSyncMock).not.toHaveBeenCalledWith( - ['', 'etc', 'passwd'].join(sep), - expect.anything() - ) + expect(rmSyncMock).not.toHaveBeenCalledWith(unrelated, expect.anything()) } finally { if (originalDataHome === undefined) { delete process.env.XDG_DATA_HOME @@ -560,11 +633,9 @@ describe('terminal-history', () => { it('warns instead of going quiet when no fish history file is found', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) try { - readFileSyncMock.mockReturnValue( - JSON.stringify({ worktreeId: 'repo-1::/path/wt', fishSession: 'orca_abc123' }) - ) + readFileSyncMock.mockReturnValue(JSON.stringify({ worktreeId, fishSession: session })) // The history dir exists (so the tree is deleted) but the fish file does not. - existsSyncMock.mockImplementation((path: string) => !path.includes('orca_abc123_history')) + existsSyncMock.mockImplementation((path: string) => !path.includes(historyFilename)) deleteWorktreeHistoryDir('repo-1::/path/wt') expect(warn).toHaveBeenCalledWith(expect.stringContaining('No fish history file found')) } finally { @@ -573,6 +644,21 @@ describe('terminal-history', () => { await flushPendingWorktreeHistoryDeletions() }) + it('cleans WSL Fish history through the owning distro before removing metadata', async () => { + readFileSyncMock.mockReturnValue(JSON.stringify({ worktreeId, fishSession: session })) + readdirSyncMock.mockImplementation((path: string) => { + if (path.endsWith('terminal-history-wsl')) { + return ['Ubuntu'] + } + return [] + }) + + deleteWorktreeHistoryDir(worktreeId) + await flushPendingWorktreeHistoryDeletions() + + expect(deleteWslFishHistoryFileMock).toHaveBeenCalledWith('Ubuntu', session) + }) + it('leaves the live directory alone when the tombstone rename fails', async () => { existsSyncMock.mockReturnValue(true) renameSyncMock.mockImplementation(() => { diff --git a/src/main/terminal-history.ts b/src/main/terminal-history.ts index 7c4684ebcee..3379517824c 100644 --- a/src/main/terminal-history.ts +++ b/src/main/terminal-history.ts @@ -1,11 +1,20 @@ import { join, basename } from 'node:path' -import { mkdirSync, existsSync, readFileSync, writeFileSync } from 'node:fs' -import { fishHistorySessionName, resolveFishHistoryFilePath } from './fish-history-session' +import { mkdirSync, existsSync, readFileSync, statSync, writeFileSync } from 'node:fs' +import { + fishHistorySessionName, + isSafeFishHistorySession, + MAX_FISH_HISTORY_META_BYTES, + normalizeFishHistoryPaths, + resolveFishHistoryFilePath +} from './fish-history-session' import { parseWslPath, toLinuxPath } from './wsl' -import { getHistoryRoot, getHistoryRootWsl, hashWorktreeId } from './terminal-history-paths' +import { getHistoryRoot, getHistoryRootWsl } from './terminal-history-paths' +import { hashWorktreeId } from './terminal-history-id' type ShellKind = 'zsh' | 'bash' | 'fish' | 'pwsh' | 'powershell' | 'cmd' | 'unknown' +export const MAX_HISTORY_META_BYTES = MAX_FISH_HISTORY_META_BYTES + // ─── Shell Detection ─────────────────────────────────────────────── /** Resolve the shell kind from a shell binary path. @@ -83,20 +92,25 @@ function writeMetaFile( const metaPath = join(dir, 'meta.json') const existing = existsSync(metaPath) ? readHistoryMeta(dir) : null const sameFishSession = existing?.fishSession === fish?.session - const existingFishPaths = sameFishSession - ? [existing?.fishHistoryPath, ...(existing?.fishHistoryPaths ?? [])].filter( - (path): path is string => typeof path === 'string' - ) - : [] - const fishHistoryPaths = fish?.historyPath - ? [...new Set([...existingFishPaths, fish.historyPath])] + const existingFishPaths = + sameFishSession && fish + ? normalizeFishHistoryPaths( + fish.session, + existing?.fishHistoryPath, + existing?.fishHistoryPaths + ) + : [] + const fishHistoryPaths = fish + ? normalizeFishHistoryPaths(fish.session, undefined, [...existingFishPaths, fish.historyPath]) : existingFishPaths if ( existing && + !existing.fishHistoryMetadataNeedsRewrite && (!fish || (sameFishSession && (!fish.historyPath || - (existing.fishHistoryPath && existingFishPaths.includes(fish.historyPath))))) + (existing.fishHistoryPath === fish.historyPath && + existingFishPaths.at(-1) === fish.historyPath)))) ) { return } @@ -106,8 +120,8 @@ function writeMetaFile( worktreeId, createdAt: existing?.createdAt ?? new Date().toISOString(), ...(fish ? { fishSession: fish.session } : {}), - // Why: retain the oldest singular path so a rollback still deletes pre-upgrade history. - ...(fishHistoryPaths[0] ? { fishHistoryPath: fishHistoryPaths[0] } : {}), + // Why: old builds read only the singular field, so it must identify the active path. + ...(fishHistoryPaths.at(-1) ? { fishHistoryPath: fishHistoryPaths.at(-1) } : {}), ...(fishHistoryPaths.length > 0 ? { fishHistoryPaths } : {}) }), { mode: 0o600 } @@ -124,29 +138,52 @@ export type HistoryDirMeta = { fishSession?: string /** That file's path resolved from the PTY's spawn env, which the main process env can contradict. */ fishHistoryPath?: string - /** Every path this session used as XDG_DATA_HOME changed across launches. */ + /** Most-recent paths this session used as XDG_DATA_HOME changed across launches. */ fishHistoryPaths?: string[] + /** In-memory migration signal; never persisted. */ + fishHistoryMetadataNeedsRewrite?: true } /** Read one history directory's meta.json, or null when it is absent or unparseable. */ export function readHistoryMeta(dir: string): HistoryDirMeta | null { try { - const raw: unknown = JSON.parse(readFileSync(join(dir, 'meta.json'), 'utf-8')) + const metaPath = join(dir, 'meta.json') + if (statSync(metaPath).size > MAX_HISTORY_META_BYTES) { + return null + } + const raw: unknown = JSON.parse(readFileSync(metaPath, 'utf-8')) if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { return null } const record = raw as Record - const fishHistoryPaths = Array.isArray(record.fishHistoryPaths) - ? record.fishHistoryPaths.filter((path): path is string => typeof path === 'string') - : undefined + const expectedFishSession = fishHistorySessionName(basename(dir).split('.')[0]) + const fishSession = + isSafeFishHistorySession(record.fishSession) && record.fishSession === expectedFishSession + ? record.fishSession + : undefined + const fishHistoryPaths = fishSession + ? normalizeFishHistoryPaths( + fishSession, + record.fishHistoryPath, + Array.isArray(record.fishHistoryPaths) ? record.fishHistoryPaths : undefined + ) + : [] + const rawFishPaths = Array.isArray(record.fishHistoryPaths) ? record.fishHistoryPaths : [] + const canonicalFishPath = fishHistoryPaths.at(-1) + const fishHistoryMetadataNeedsRewrite = Boolean( + record.fishSession !== undefined && + (!fishSession || + record.fishHistoryPath !== canonicalFishPath || + rawFishPaths.length !== fishHistoryPaths.length || + rawFishPaths.some((path, index) => path !== fishHistoryPaths[index])) + ) return { ...(typeof record.worktreeId === 'string' ? { worktreeId: record.worktreeId } : {}), ...(typeof record.createdAt === 'string' ? { createdAt: record.createdAt } : {}), - ...(typeof record.fishSession === 'string' ? { fishSession: record.fishSession } : {}), - ...(typeof record.fishHistoryPath === 'string' - ? { fishHistoryPath: record.fishHistoryPath } - : {}), - ...(fishHistoryPaths && fishHistoryPaths.length > 0 ? { fishHistoryPaths } : {}) + ...(fishSession ? { fishSession } : {}), + ...(fishHistoryPaths.at(-1) ? { fishHistoryPath: fishHistoryPaths.at(-1) } : {}), + ...(fishHistoryPaths.length > 0 ? { fishHistoryPaths } : {}), + ...(fishHistoryMetadataNeedsRewrite ? { fishHistoryMetadataNeedsRewrite: true } : {}) } } catch { return null @@ -239,6 +276,26 @@ export function injectHistoryEnv( return result } +/** WSL's outer executable hides the login shell, so carry both history knobs. */ +export function injectWslFishHistoryEnv( + spawnEnv: Record, + worktreeId: string, + wslDistro: string +): string | null { + if (spawnEnv.fish_history) { + return null + } + const worktreeHash = hashWorktreeId(worktreeId) + const historyDir = ensureHistoryDir(worktreeHash, wslDistro) + if (!historyDir) { + return null + } + const session = fishHistorySessionName(worktreeHash) + writeMetaFile(historyDir, worktreeId, { session, historyPath: null }) + spawnEnv.fish_history = session + return session +} + /** Re-point the history env when shell fallback changes the shell kind — e.g. zsh * fails and bash takes over, so HISTFILE must name bash_history. A fish primary * injected `fish_history` instead, which the fallback shell cannot use. */ diff --git a/src/main/wsl-fish-history-cleanup.test.ts b/src/main/wsl-fish-history-cleanup.test.ts new file mode 100644 index 00000000000..cca59bf7586 --- /dev/null +++ b/src/main/wsl-fish-history-cleanup.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it, vi } from 'vitest' +import { deleteWslFishHistoryFile } from './wsl-fish-history-cleanup' + +describe('deleteWslFishHistoryFile', () => { + it('uses direct argv and bounds a distro cleanup subprocess', async () => { + const run = vi.fn().mockResolvedValue({ stdout: '', stderr: '' }) + + await deleteWslFishHistoryFile('Ubuntu Test', 'orca_0123456789abcdef', run) + + expect(run).toHaveBeenCalledWith( + 'wsl.exe', + [ + '--distribution', + 'Ubuntu Test', + '--exec', + 'fish', + '--command', + expect.stringContaining('orca_0123456789abcdef_history') + ], + { timeout: 5_000, windowsHide: true } + ) + }) + + it('rejects an unsafe session before spawning', async () => { + const run = vi.fn() + + await deleteWslFishHistoryFile('Ubuntu', '../../user-history', run) + + expect(run).not.toHaveBeenCalled() + }) + + it('serializes cleanup subprocesses across a startup tombstone batch', async () => { + let releaseFirst: (() => void) | undefined + const first = new Promise<{ stdout: string; stderr: string }>((resolve) => { + releaseFirst = () => resolve({ stdout: '', stderr: '' }) + }) + const run = vi.fn().mockReturnValueOnce(first).mockResolvedValueOnce({ stdout: '', stderr: '' }) + + const cleanupA = deleteWslFishHistoryFile('Ubuntu', 'orca_aaaaaaaaaaaaaaaa', run) + const cleanupB = deleteWslFishHistoryFile('Debian', 'orca_bbbbbbbbbbbbbbbb', run) + await vi.waitFor(() => expect(run).toHaveBeenCalledTimes(1)) + releaseFirst?.() + await Promise.all([cleanupA, cleanupB]) + + expect(run).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/wsl-fish-history-cleanup.ts b/src/main/wsl-fish-history-cleanup.ts new file mode 100644 index 00000000000..564f7adc43a --- /dev/null +++ b/src/main/wsl-fish-history-cleanup.ts @@ -0,0 +1,30 @@ +import { execFile } from 'node:child_process' +import { promisify } from 'node:util' +import { isSafeFishHistorySession } from './fish-history-session' + +const execFileAsync = promisify(execFile) +let cleanupTail = Promise.resolve() + +export function deleteWslFishHistoryFile( + distro: string, + session: string, + run: typeof execFileAsync = execFileAsync +): Promise { + if (!distro.trim() || !isSafeFishHistorySession(session)) { + return Promise.resolve() + } + const script = [ + 'set -l data_home $XDG_DATA_HOME', + 'string match -qr "^/" -- $data_home; or set data_home "$HOME/.local/share"', + `command rm -f -- "$data_home/fish/${session}_history"` + ].join('; ') + // Why: startup may discover many durable tombstones, but wsl.exe cleanup fanout must stay at one. + const cleanup = cleanupTail.then(async () => { + await run('wsl.exe', ['--distribution', distro, '--exec', 'fish', '--command', script], { + timeout: 5_000, + windowsHide: true + }) + }) + cleanupTail = cleanup.catch(() => undefined) + return cleanup +} diff --git a/src/relay/fish-history-metadata.test.ts b/src/relay/fish-history-metadata.test.ts new file mode 100644 index 00000000000..0781de8b7d7 --- /dev/null +++ b/src/relay/fish-history-metadata.test.ts @@ -0,0 +1,79 @@ +import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + fishHistorySessionName, + MAX_RETAINED_FISH_HISTORY_PATHS +} from '../main/fish-history-session' +import { hashWorktreeId } from '../main/terminal-history-id' +import { deleteRelayFishHistory, recordRelayFishHistoryPath } from './fish-history-metadata' + +describe('relay Fish history metadata', () => { + let root: string + let dataRoot: string + const worktreeId = 'repo-1::/remote/wt' + const hash = hashWorktreeId(worktreeId) + const session = fishHistorySessionName(hash) + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'orca-relay-fish-meta-')) + dataRoot = join(root, 'data') + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it('retains only the latest distinct paths and preserves the active path', () => { + const metadataRoot = join(root, 'metadata') + for (let index = 0; index < 40; index += 1) { + recordRelayFishHistoryPath( + worktreeId, + { XDG_DATA_HOME: join(dataRoot, String(index)) }, + metadataRoot + ) + } + recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: join(dataRoot, '39') }, metadataRoot) + + const meta = JSON.parse(readFileSync(join(metadataRoot, `${hash}.json`), 'utf8')) as { + fishHistoryPath: string + fishHistoryPaths: string[] + } + expect(meta.fishHistoryPaths).toHaveLength(MAX_RETAINED_FISH_HISTORY_PATHS) + expect(new Set(meta.fishHistoryPaths).size).toBe(MAX_RETAINED_FISH_HISTORY_PATHS) + expect(meta.fishHistoryPath).toBe(join(dataRoot, '39', 'fish', `${session}_history`)) + expect(meta.fishHistoryPaths.at(-1)).toBe(meta.fishHistoryPath) + }) + + it('replaces malformed metadata without retaining an unrelated path', () => { + const metadataRoot = join(root, 'metadata') + mkdirSync(metadataRoot, { recursive: true }) + const unrelated = join(dataRoot, 'fish', 'fish_history') + writeFileSync( + join(metadataRoot, `${hash}.json`), + JSON.stringify({ fishSession: session, fishHistoryPaths: [unrelated, null] }) + ) + + recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: dataRoot }, metadataRoot) + + const meta = JSON.parse(readFileSync(join(metadataRoot, `${hash}.json`), 'utf8')) as { + fishHistoryPaths: string[] + } + expect(meta.fishHistoryPaths).toEqual([join(dataRoot, 'fish', `${session}_history`)]) + }) + + it('deletes recorded XDG paths and its bounded metadata', () => { + const metadataRoot = join(root, 'metadata') + const historyFile = join(dataRoot, 'fish', `${session}_history`) + mkdirSync(join(dataRoot, 'fish'), { recursive: true }) + writeFileSync(historyFile, 'secret') + recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: dataRoot }, metadataRoot) + + deleteRelayFishHistory(worktreeId, metadataRoot, { XDG_DATA_HOME: dataRoot }) + + expect(existsSync(historyFile)).toBe(false) + expect(existsSync(join(metadataRoot, `${hash}.json`))).toBe(false) + }) +}) diff --git a/src/relay/fish-history-metadata.ts b/src/relay/fish-history-metadata.ts new file mode 100644 index 00000000000..80423b41eb4 --- /dev/null +++ b/src/relay/fish-history-metadata.ts @@ -0,0 +1,92 @@ +import { mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { homedir } from 'node:os' +import { join } from 'node:path' +import { + deleteFishHistoryFile, + fishHistorySessionName, + MAX_FISH_HISTORY_META_BYTES, + normalizeFishHistoryPaths, + resolveFishHistoryFilePath +} from '../main/fish-history-session' +import { hashWorktreeId } from '../main/terminal-history-id' + +function metadataRoot(): string { + return join(homedir(), '.orca-remote', 'fish-history') +} + +function metadataPath(worktreeId: string, root: string): string { + return join(root, `${hashWorktreeId(worktreeId)}.json`) +} + +function readPaths(worktreeId: string, root: string): string[] { + try { + const file = metadataPath(worktreeId, root) + if (statSync(file).size > MAX_FISH_HISTORY_META_BYTES) { + return [] + } + const raw: unknown = JSON.parse(readFileSync(file, 'utf8')) + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + return [] + } + const record = raw as Record + const session = fishHistorySessionName(hashWorktreeId(worktreeId)) + if (record.fishSession !== session) { + return [] + } + return normalizeFishHistoryPaths( + session, + record.fishHistoryPath, + Array.isArray(record.fishHistoryPaths) ? record.fishHistoryPaths : undefined + ) + } catch { + return [] + } +} + +export function recordRelayFishHistoryPath( + worktreeId: string, + env: NodeJS.ProcessEnv, + root = metadataRoot() +): void { + const session = fishHistorySessionName(hashWorktreeId(worktreeId)) + const activePath = resolveFishHistoryFilePath(session, env) + if (!activePath) { + return + } + try { + const paths = normalizeFishHistoryPaths(session, undefined, [ + ...readPaths(worktreeId, root), + activePath + ]) + mkdirSync(root, { recursive: true, mode: 0o700 }) + writeFileSync( + metadataPath(worktreeId, root), + JSON.stringify({ + fishSession: session, + fishHistoryPath: activePath, + fishHistoryPaths: paths + }), + { mode: 0o600 } + ) + } catch (error) { + console.warn( + `[pty:history] Failed to record relay fish history: ${error instanceof Error ? error.message : String(error)}` + ) + } +} + +export function deleteRelayFishHistory( + worktreeId: string, + root = metadataRoot(), + env: NodeJS.ProcessEnv = process.env +): void { + const session = fishHistorySessionName(hashWorktreeId(worktreeId)) + deleteFishHistoryFile(session, { recordedPaths: readPaths(worktreeId, root), env }) + try { + rmSync(metadataPath(worktreeId, root), { force: true }) + } catch (error) { + console.warn( + `[pty:history] Failed to delete relay fish metadata: ${error instanceof Error ? error.message : String(error)}` + ) + } +} diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 15c4884f1b1..258b699df2b 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -17,6 +17,14 @@ import { type RelayDispatcher } from './git-handler-test-setup' +const { deleteRelayFishHistoryMock } = vi.hoisted(() => ({ + deleteRelayFishHistoryMock: vi.fn() +})) + +vi.mock('./fish-history-metadata', () => ({ + deleteRelayFishHistory: deleteRelayFishHistoryMock +})) + type GitBufferSpyTarget = { gitBuffer(args: string[], cwd: string): Promise } @@ -62,6 +70,7 @@ describe('GitHandler', () => { dispatcher = createMockDispatcher() const ctx = new RelayContext() handler = new GitHandler(dispatcher as unknown as RelayDispatcher, ctx) + deleteRelayFishHistoryMock.mockReset() }) afterEach(async () => { @@ -155,6 +164,28 @@ describe('GitHandler', () => { expect(runWithRemovalFence).toHaveBeenCalledWith('/repo-feature', expect.any(Function)) }) + it('cleans process-owner Fish history after removing a remote worktree', async () => { + gitInit(tmpDir) + writeFileSync(path.join(tmpDir, 'tracked.txt'), 'initial') + gitCommit(tmpDir, 'initial') + const worktreePath = `${tmpDir}-feature` + try { + execFileSync('git', ['worktree', 'add', '-b', 'feature', worktreePath], { + cwd: tmpDir, + stdio: 'pipe' + }) + + await dispatcher.callRequest('git.removeWorktree', { + worktreePath, + worktreeId: 'repo-1::/remote/feature' + }) + + expect(deleteRelayFishHistoryMock).toHaveBeenCalledWith('repo-1::/remote/feature') + } finally { + await fs.rm(worktreePath, { recursive: true, force: true }) + } + }) + describe('abortMerge', () => { it('aborts an in-progress merge', async () => { gitInit(tmpDir) diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index 3e8a20ea992..b5f35a83513 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -96,6 +96,7 @@ import { endSubprocessStdin } from '../shared/subprocess-stdin-write' import { clearGitStatusLineStatsCache } from '../shared/git-status-line-stats-cache' import { invalidateGitBranchLineTotalInFlight } from '../shared/git-branch-line-total' import { streamRelayGitStdout } from './git-stdout-stream' +import { deleteRelayFishHistory } from './fish-history-metadata' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -1461,10 +1462,15 @@ export class GitHandler { } private async removeWorktree(params: Record) { - const remove = () => - this.runWithGitReadCacheClear(() => + const remove = async () => { + const result = await this.runWithGitReadCacheClear(() => removeWorktreeOp(this.git.bind(this), params, this.gitCapabilities) ) + if (typeof params.worktreeId === 'string') { + deleteRelayFishHistory(params.worktreeId) + } + return result + } const worktreePath = params.worktreePath return this.watcherRegistry && typeof worktreePath === 'string' ? this.watcherRegistry.runWithRemovalFence(expandTilde(worktreePath), remove) diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 06eaa15d5ff..e89483b0dd1 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -13,9 +13,17 @@ import { import { PTY_STARTUP_INGRESS_VERSION } from '../shared/pty-startup-ingress' import { stripLegacyTerminalShimEnv } from '../main/pty/legacy-terminal-shim-dir' -const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ +const { + deleteRelayFishHistoryMock, + mockPtySpawn, + mockPtyInstance, + mockCreateShellPromptReadinessProbe, + recordRelayFishHistoryPathMock +} = vi.hoisted(() => ({ + deleteRelayFishHistoryMock: vi.fn(), mockPtySpawn: vi.fn(), mockCreateShellPromptReadinessProbe: vi.fn(), + recordRelayFishHistoryPathMock: vi.fn(), mockPtyInstance: { // Why: attach now proves the backing pid is alive before replaying, so the // default managed PTY must report a live pid. Reuse the test runner's own @@ -44,6 +52,11 @@ vi.mock('../main/shell-prompt-readiness-probe', () => ({ createShellPromptReadinessProbe: mockCreateShellPromptReadinessProbe })) +vi.mock('./fish-history-metadata', () => ({ + deleteRelayFishHistory: deleteRelayFishHistoryMock, + recordRelayFishHistoryPath: recordRelayFishHistoryPathMock +})) + import { IMMEDIATE_PTY_EXIT_TIMEOUT_MS, MAX_RELAY_PTY_SESSIONS, @@ -146,6 +159,8 @@ describe('PtyHandler', () => { mockPtyInstance.pause.mockReset() mockPtyInstance.resume.mockReset() mockCreateShellPromptReadinessProbe.mockReset() + deleteRelayFishHistoryMock.mockReset() + recordRelayFishHistoryPathMock.mockReset() mockCreateShellPromptReadinessProbe.mockReturnValue({ notifyOutput: vi.fn(), dispose: vi.fn() @@ -183,6 +198,7 @@ describe('PtyHandler', () => { expect(methods).toContain('pty.inspectProcess') expect(methods).toContain('pty.listProcesses') expect(methods).toContain('pty.getDefaultShell') + expect(methods).toContain('pty.deleteWorktreeHistory') const notifMethods = Array.from(dispatcher._notificationHandlers.keys()) expect(notifMethods).toContain('pty.data') @@ -706,6 +722,44 @@ describe('PtyHandler', () => { } }) + it('uses the requested PTY Fish shell and scopes its history on the relay owner', async () => { + await dispatcher.callRequest('pty.spawn', { + env: { SHELL: '/usr/bin/fish' }, + worktreeId: 'repo-1::/remote/wt', + historyIsolationEnabled: true + }) + + expect(mockPtySpawn.mock.calls[0]?.[0]).toBe('/usr/bin/fish') + const spawnEnv = mockPtySpawn.mock.calls[0]?.[2]?.env as Record + expect(spawnEnv.fish_history).toMatch(/^orca_[0-9a-f]{16}$/) + expect(recordRelayFishHistoryPathMock).toHaveBeenCalledWith( + 'repo-1::/remote/wt', + expect.objectContaining({ fish_history: spawnEnv.fish_history }) + ) + }) + + it('deletes persisted Fish history on its process owner', async () => { + await dispatcher.callRequest('pty.deleteWorktreeHistory', { + worktreeId: 'repo-1::/remote/wt' + }) + + expect(deleteRelayFishHistoryMock).toHaveBeenCalledWith('repo-1::/remote/wt') + }) + + it.each([undefined, 'true'])( + 'leaves omitted or malformed scoped-history disabled', + async (flag) => { + await dispatcher.callRequest('pty.spawn', { + env: { SHELL: '/usr/bin/fish' }, + worktreeId: 'repo-1::/remote/wt', + historyIsolationEnabled: flag + }) + + const spawnEnv = mockPtySpawn.mock.calls[0]?.[2]?.env as Record + expect(spawnEnv.fish_history).toBeUndefined() + } + ) + it('ignores Windows shell overrides on non-Windows relay hosts', async () => { const originalPlatform = process.platform Object.defineProperty(process, 'platform', { @@ -3086,6 +3140,32 @@ describe('PtyHandler', () => { expect(callArgs.env.ORCA_SHELL_STARTUP_IDENTITY).toBe('0') }) + it('revive restores scoped Fish history without requiring it from legacy state', async () => { + await dispatcher.callRequest('pty.spawn', { + env: { SHELL: '/usr/bin/fish' }, + worktreeId: 'repo-1::/remote/wt', + historyIsolationEnabled: true + }) + const firstEnv = mockPtySpawn.mock.calls[0]?.[2]?.env as Record + const state = (await dispatcher.callRequest('pty.serialize', { ids: ['pty-1'] })) as string + + await handler.dispose({ waitForPhysicalExit: false }) + mockPtySpawn.mockClear() + dispatcher = createMockDispatcher() + handler = new PtyHandler(dispatcher as unknown as RelayDispatcher) + const shellSpy = vi.spyOn(ptyShellUtils, 'resolveDefaultShell').mockReturnValue('/usr/bin/fish') + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true) + try { + await dispatcher.callRequest('pty.revive', { state }) + } finally { + killSpy.mockRestore() + shellSpy.mockRestore() + } + + const revivedEnv = mockPtySpawn.mock.calls[0]?.[2]?.env as Record + expect(revivedEnv.fish_history).toBe(firstEnv.fish_history) + }) + it('fences both revived worktree identity and cwd with rollback', async () => { const finishSiblingAdmission = vi.fn() const beginWorktreePtySpawn = vi.fn((operationPath: string) => { diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 3f8bd593e08..50a6e4ba72e 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -2,7 +2,7 @@ import type { IPty } from 'node-pty' import type * as NodePty from 'node-pty' import { existsSync } from 'node:fs' -import { join } from 'node:path' +import { basename, join } from 'node:path' import { randomUUID } from 'node:crypto' import { resolveWindowsGitBashShellPath } from '../main/git-bash' import { WINDOWS_GIT_BASH_SHELL } from '../shared/windows-terminal-shell' @@ -79,6 +79,9 @@ import { type AgentSessionOwnerBinding } from '../shared/agent-session-host-authority' import { createPtySlaveEchoProbe, readPtySlavePath } from '../shared/pty-slave-line-discipline-echo' +import { fishHistorySessionName } from '../main/fish-history-session' +import { hashWorktreeId } from '../main/terminal-history-id' +import { deleteRelayFishHistory, recordRelayFishHistoryPath } from './fish-history-metadata' // Why: only Linux compiles node-pty (no prebuilt), so the build-tools remedy is a closable setup gap // there and wrong advice anywhere node-pty ships one. The relay only sees an unloadable binding, never @@ -158,6 +161,7 @@ type ManagedPty = { shellPathEnv?: string envToDelete: string[] gitCredentialPromptGuarded: boolean + historyIsolationEnabled?: boolean startupCommand?: ManagedStartupCommand physicalExit?: PhysicalExitTracker forceKillSent?: boolean @@ -327,6 +331,8 @@ type SerializedPtyEntry = { envToDelete?: string[] /** Optional for state serialized by relays predating the credential guard. */ gitCredentialPromptGuarded?: boolean + /** Optional for state serialized by relays predating scoped history. */ + historyIsolationEnabled?: boolean agentSessionOwners?: AgentSessionOwnerBinding[] } @@ -860,6 +866,12 @@ export class PtyHandler { this.dispatcher.onRequest('pty.serialize', (p) => this.serialize(p)) this.dispatcher.onRequest('pty.revive', (p) => this.revive(p)) this.dispatcher.onRequest('pty.getProfiles', async () => listShellProfiles()) + this.dispatcher.onRequest('pty.deleteWorktreeHistory', async (p) => { + if (typeof p.worktreeId === 'string') { + deleteRelayFishHistory(p.worktreeId) + } + return { ok: true } + }) this.dispatcher.onRequest('pty.closeStartupQueryAuthority', (p) => this.closeStartupQueryAuthority(p) ) @@ -1387,7 +1399,8 @@ export class PtyHandler { context?: RequestContext ): Promise { const env = params.env as Record | undefined - const worktreeId = env?.ORCA_WORKTREE_ID + const worktreeId = + typeof params.worktreeId === 'string' ? params.worktreeId : env?.ORCA_WORKTREE_ID const worktreePath = worktreeId ? splitWorktreeId(worktreeId)?.worktreePath : undefined const cwd = typeof params.cwd === 'string' ? params.cwd : resolveDefaultCwd() const finishCreation = this.beginPtyCreation([worktreePath, cwd]) @@ -1503,7 +1516,9 @@ export class PtyHandler { const shellOverride = typeof params.shellOverride === 'string' ? params.shellOverride.trim() : '' const resolvedShellOverride = resolvePtyShellOverride(shellOverride) - const shell = resolvedShellOverride || resolveDefaultShell() + const requestedEnvShell = + process.platform !== 'win32' && typeof env?.SHELL === 'string' ? env.SHELL.trim() : '' + const shell = resolvedShellOverride || requestedEnvShell || resolveDefaultShell() let id: string do { id = `pty-${this.nextId++}` @@ -1525,6 +1540,18 @@ export class PtyHandler { { id, paneKey, shell, command, launchAgent }, envToDelete ) + const worktreeId = + typeof params.worktreeId === 'string' ? params.worktreeId : env?.ORCA_WORKTREE_ID + const historyIsolationEnabled = params.historyIsolationEnabled === true + if ( + historyIsolationEnabled && + worktreeId && + basename(shell).toLowerCase().startsWith('fish') && + !spawnEnv.fish_history + ) { + spawnEnv.fish_history = fishHistorySessionName(hashWorktreeId(worktreeId)) + recordRelayFishHistoryPath(worktreeId, spawnEnv) + } const launchCommandHint = resolveSetupAgentSequenceLaunchCommand(spawnEnv, command) // Why: SSH PTYs bypass main's host-env builder, so apply the guard after the relay merges its authoritative env. const gitCredentialPromptGuarded = applyTerminalGitCredentialPromptGuard(spawnEnv, { @@ -1591,7 +1618,6 @@ export class PtyHandler { paneKey: typeof params.paneKey === 'string' ? params.paneKey : paneKey, tabId: typeof params.tabId === 'string' ? params.tabId : tabId } - const worktreeId = typeof env?.ORCA_WORKTREE_ID === 'string' ? env.ORCA_WORKTREE_ID : undefined const startupIngressIntent = params.startupIngressVersion === PTY_STARTUP_INGRESS_VERSION ? parsePtyStartupIngressIntent(params.startupIngress) @@ -1612,6 +1638,7 @@ export class PtyHandler { ...(explicitTerm !== undefined ? { explicitTerm } : {}), envToDelete, gitCredentialPromptGuarded, + ...(historyIsolationEnabled ? { historyIsolationEnabled: true } : {}), shellPath: shell, shellCwd: cwd, shellPathEnv: spawnEnv.PATH, @@ -2034,6 +2061,7 @@ export class PtyHandler { ...(managed.explicitTerm !== undefined ? { explicitTerm: managed.explicitTerm } : {}), envToDelete: managed.envToDelete, gitCredentialPromptGuarded: managed.gitCredentialPromptGuarded, + ...(managed.historyIsolationEnabled ? { historyIsolationEnabled: true } : {}), ...(managed.terminalHandle ? { terminalHandle: managed.terminalHandle } : {}) }) } @@ -2097,11 +2125,21 @@ export class PtyHandler { // Why: serialized state may come from an older/untrusted client; reapply fresh-spawn bounds. const envToDelete = sanitizeEnvToDelete(entry.envToDelete) const shell = resolveDefaultShell() + const historyIsolationEnabled = entry.historyIsolationEnabled === true const spawnEnv = this.buildSpawnEnv( revivedEnv, { id: entry.id, paneKey: entry.paneKey, shell }, envToDelete ) + if ( + historyIsolationEnabled && + entry.worktreeId && + basename(shell).toLowerCase().startsWith('fish') && + !spawnEnv.fish_history + ) { + spawnEnv.fish_history = fishHistorySessionName(hashWorktreeId(entry.worktreeId)) + recordRelayFishHistoryPath(entry.worktreeId, spawnEnv) + } // Why: revive lacks the original launch command, so reuse the fresh-spawn guard decision (legacy defaults to unguarded). const gitCredentialPromptGuarded = entry.gitCredentialPromptGuarded === true if (gitCredentialPromptGuarded) { @@ -2137,6 +2175,7 @@ export class PtyHandler { ...(explicitTerm !== undefined ? { explicitTerm } : {}), envToDelete, gitCredentialPromptGuarded, + ...(historyIsolationEnabled ? { historyIsolationEnabled: true } : {}), ownerBackend: resolvePtyOwnerBackend({ platform: process.platform, shellPath: shell diff --git a/src/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsx b/src/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsx index b7150aa7cef..ac00e845d97 100644 --- a/src/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsx +++ b/src/renderer/src/components/floating-terminal/FloatingTerminalWindowControls.tsx @@ -20,6 +20,8 @@ import { } from '../../../../shared/tui-agent-launch-defaults' import { translate } from '@/i18n/i18n' import { useOptionalShortcutLabel } from '@/hooks/useShortcutLabel' +import { getClientLoginShell } from '@/lib/client-login-shell' +import { resolveLoginShellStartupDialect } from '../../../../shared/tui-agent-startup-shell' type FloatingTerminalWindowControlsProps = { maximized: boolean @@ -79,6 +81,7 @@ export function FloatingTerminalWindowControls({ agentArgs: resolveTuiAgentLaunchArgs(defaultAgent, state.settings?.agentDefaultArgs), agentEnv: resolveTuiAgentLaunchEnv(defaultAgent, state.settings?.agentDefaultEnv), platform: CLIENT_PLATFORM, + shell: resolveLoginShellStartupDialect(getClientLoginShell()), allowEmptyPromptLaunch: true }) if (!startupPlan) { 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 4708981d6f1..9ce9693f3bd 100644 --- a/src/renderer/src/lib/agent-resume-launch-target.test.ts +++ b/src/renderer/src/lib/agent-resume-launch-target.test.ts @@ -86,7 +86,7 @@ describe('resolveAgentResumeLaunchTarget on a Windows client', () => { executionHostId: 'ssh:ssh-1', terminalWindowsShell: 'cmd.exe' }) - ).resolves.toEqual({ platform: 'linux', shell: undefined }) + ).resolves.toEqual({ platform: 'linux', shell: 'unix' }) }) it('does not describe a remote runtime host with the local Windows shell setting', async () => { @@ -104,7 +104,7 @@ describe('resolveAgentResumeLaunchTarget on a Windows client', () => { worktreePath: '\\\\wsl.localhost\\Ubuntu\\home\\neil\\repo', terminalWindowsShell: 'cmd.exe' }) - ).resolves.toEqual({ platform: 'linux', shell: undefined }) + ).resolves.toEqual({ platform: 'linux', shell: 'unix' }) }) it('keeps POSIX quoting for a project pinned to a WSL runtime', async () => { @@ -122,7 +122,7 @@ describe('resolveAgentResumeLaunchTarget on a Windows client', () => { } as AgentResumeLaunchTargetArgs['projectRuntime'], terminalWindowsShell: 'cmd.exe' }) - ).resolves.toEqual({ platform: 'linux', shell: undefined }) + ).resolves.toEqual({ platform: 'linux', shell: 'unix' }) }) }) diff --git a/src/renderer/src/lib/launch-work-item-direct.test.ts b/src/renderer/src/lib/launch-work-item-direct.test.ts index c2690c86ba6..798350437d5 100644 --- a/src/renderer/src/lib/launch-work-item-direct.test.ts +++ b/src/renderer/src/lib/launch-work-item-direct.test.ts @@ -599,6 +599,7 @@ describe('launchWorkItemDirect', () => { agentEnv: {}, sessionOptions: undefined, platform: 'linux', + shell: 'unix', isRemote: true }) expect(buildAgentStartupPlan).toHaveBeenCalledWith({ @@ -609,6 +610,7 @@ describe('launchWorkItemDirect', () => { agentEnv: {}, sessionOptions: undefined, platform: 'linux', + shell: 'unix', isRemote: true, allowEmptyPromptLaunch: true }) diff --git a/src/renderer/src/lib/onboarding-folder-agent-startup.ts b/src/renderer/src/lib/onboarding-folder-agent-startup.ts index 55668937430..17cfec41bdb 100644 --- a/src/renderer/src/lib/onboarding-folder-agent-startup.ts +++ b/src/renderer/src/lib/onboarding-folder-agent-startup.ts @@ -11,6 +11,8 @@ import type { SleepingAgentLaunchConfig } from '../../../shared/agent-session-re import type { GlobalSettings, OnboardingState, TuiAgent } from '../../../shared/types' import { resolveInitialNativeChatSessionOptions } from '@/components/native-chat/native-chat-launch-session-options' import type { SessionOptionValue } from '../../../shared/native-chat-session-options' +import { resolveLoginShellStartupDialect } from '../../../shared/tui-agent-startup-shell' +import { getClientLoginShell } from '@/lib/client-login-shell' export type OnboardingFolderAgentStartup = { command: string @@ -54,6 +56,7 @@ export function buildOnboardingFolderAgentStartup( nativeChatTranscriptIsLocalReadable }), platform: getClientPlatform(), + shell: resolveLoginShellStartupDialect(getClientLoginShell()), allowEmptyPromptLaunch: true }) if (!startupPlan) { diff --git a/src/shared/agent-resume-argv-drop.ts b/src/shared/agent-resume-argv-drop.ts index 55134fe45a4..f9b8de558fe 100644 --- a/src/shared/agent-resume-argv-drop.ts +++ b/src/shared/agent-resume-argv-drop.ts @@ -15,7 +15,13 @@ export type AgentResumeArgvDrop = // fish is listed because its quoting diverges from sh; without it a fish-quoted // suffix reads as `unrecognized` and the caller refuses the launch outright. -const RESUME_ARGV_SHELLS: readonly AgentStartupShell[] = ['posix', 'fish', 'powershell', 'cmd'] +const RESUME_ARGV_SHELLS: readonly AgentStartupShell[] = [ + 'posix', + 'fish', + 'unix', + 'powershell', + 'cmd' +] function resumeArgvSuffixCandidates(resumeArgs: readonly string[]): string[] { const candidates = new Set([resumeArgs.join(' ')]) diff --git a/src/shared/agent-resume-launch-command.test.ts b/src/shared/agent-resume-launch-command.test.ts index 5bf4797472b..7ff1bd42bec 100644 --- a/src/shared/agent-resume-launch-command.test.ts +++ b/src/shared/agent-resume-launch-command.test.ts @@ -9,6 +9,7 @@ const providerSession = { key: 'session_id', id: SESSION_ID } as const const SHELLS: { platform: NodeJS.Platform; shell: AgentStartupShell }[] = [ { platform: 'linux', shell: 'posix' }, + { platform: 'linux', shell: 'unix' }, { platform: 'darwin', shell: 'posix' }, { platform: 'win32', shell: 'powershell' }, { platform: 'win32', shell: 'cmd' } diff --git a/src/shared/fish-command-tokenizer.ts b/src/shared/fish-command-tokenizer.ts index 46973ed73de..99061bd5cfc 100644 --- a/src/shared/fish-command-tokenizer.ts +++ b/src/shared/fish-command-tokenizer.ts @@ -86,10 +86,13 @@ function decodeFishEscape(value: string, start: number): EscapeDecode { : fromCode(digits, 8, 0) } if (char === 'c' && value[start + 1]) { + const control = value[start + 1] return { - text: String.fromCharCode(value.charCodeAt(start + 1) & 0x1f), + text: /^[A-Za-z]$/.test(control) + ? String.fromCharCode(control.charCodeAt(0) & 0x1f) + : value.slice(start, start + 2), next: start + 2, - diverges: false + diverges: !/^[A-Za-z]$/.test(control) } } return { text: char, next: start + 1, diverges: false } diff --git a/src/shared/fish-draft-prefill-teardown.node-pty.test.ts b/src/shared/fish-draft-prefill-teardown.node-pty.test.ts index 16082f12029..18de2c71c74 100644 --- a/src/shared/fish-draft-prefill-teardown.node-pty.test.ts +++ b/src/shared/fish-draft-prefill-teardown.node-pty.test.ts @@ -139,10 +139,40 @@ describe('fish clears an agent draft prefill variable', () => { expect(await liveness('POSIXLEFT')).toBe(true) expect(statusOf('POSIXLEFT')).toBe('0') - expect(await run('FISHRC', clearEnvCommand(VAR, 'fish'))).toBe(true) - expect(statusOf('FISHRC')).toBe('0') + expect(await run('UNIXRC', clearEnvCommand(VAR, 'unix'))).toBe(true) + expect(statusOf('UNIXRC')).toBe('0') expect(await liveness('FISHLEFT')).toBe(true) expect(statusOf('FISHLEFT')).toBe('1') + + expect(await run('GLOBAL', `set -gx ${VAR} global; ${clearEnvCommand(VAR, 'unix')}`)).toBe( + true + ) + expect(await liveness('GLOBALLEFT')).toBe(true) + expect(statusOf('GLOBALLEFT')).toBe('1') + + expect( + await run( + 'LOCAL', + `function orca_clear_local; set -lx ${VAR} local; ${clearEnvCommand(VAR, 'unix')}; set -q ${VAR}; end; orca_clear_local` + ) + ).toBe(true) + expect(statusOf('LOCAL')).toBe('1') + + expect( + await run('UNIVERSAL', `set -Ux ${VAR} universal; ${clearEnvCommand(VAR, 'unix')}`) + ).toBe(true) + expect(await liveness('UNIVERSALLEFT')).toBe(true) + expect(statusOf('UNIVERSALLEFT')).toBe('1') + + expect( + await run( + 'ALIAS', + `set -gx ${VAR} alias-draft; alias orca_prefill_agent 'test "$${VAR}" = alias-draft'; orca_prefill_agent; ${clearEnvCommand(VAR, 'unix')}` + ) + ).toBe(true) + expect(statusOf('ALIAS')).toBe('0') + expect(await liveness('ALIASLEFT')).toBe(true) + expect(statusOf('ALIASLEFT')).toBe('1') } finally { term.write('exit\r') await waitUntil(() => exited, 5_000) diff --git a/src/shared/fish-startup-arg-quoting.live-fish.test.ts b/src/shared/fish-startup-arg-quoting.live-fish.test.ts index 6ed1e3ddf4f..97cb9cd11c6 100644 --- a/src/shared/fish-startup-arg-quoting.live-fish.test.ts +++ b/src/shared/fish-startup-arg-quoting.live-fish.test.ts @@ -97,6 +97,9 @@ describe('quoteStartupArg round-trips through real fish', () => { itWithFish(`survives ${name}`, () => { expect(fishArgv(quoteStartupArg(value, 'fish'))).toEqual([value]) }) + itWithFish(`survives ${name} through an unknown Unix host`, () => { + expect(fishArgv(quoteStartupArg(value, 'unix'))).toEqual([value]) + }) } itWithFish('keeps multiple arguments separate', () => { @@ -111,6 +114,12 @@ describe('quoteStartupArg round-trips through real fish', () => { expect(() => fishArgv(quoteStartupArg('ends with a backslash\\', 'posix'))).toThrow() }) + itWithFish('accepts the portable Unix trailing-backslash form', () => { + expect(fishArgv(quoteStartupArg('ends with a backslash\\', 'unix'))).toEqual([ + 'ends with a backslash\\' + ]) + }) + itWithFish('proves the sh tokenizer this replaced disagreed with fish', () => { const line = String.raw`claude --prompt 'match \d+ in \\server\share'` const posix = tokenizeStartupCommand(line, 'posix') diff --git a/src/shared/local-agent-startup-shell.test.ts b/src/shared/local-agent-startup-shell.test.ts index 48e5d85f6c2..d2869ccb5d2 100644 --- a/src/shared/local-agent-startup-shell.test.ts +++ b/src/shared/local-agent-startup-shell.test.ts @@ -24,19 +24,17 @@ describe('resolveLocalAgentStartupShell', () => { expect(resolveLocalAgentStartupShell({ ...posixHost, hostLoginShell: undefined })).toBe('posix') }) - // Why each of these matters: `set -e VAR` in bash enables errexit instead of - // clearing anything, so a wrong dialect is worse than the sh default. - it('refuses the local dialect for a remote target', () => { + it('uses the portable Unix dialect for a remote target', () => { expect( resolveLocalAgentStartupShell({ ...posixHost, isRemote: true, hostLoginShell: '/usr/bin/fish' }) - ).toBeUndefined() + ).toBe('unix') }) - it('refuses the local dialect for ssh and runtime execution hosts', () => { + it('uses the portable Unix dialect for ssh and runtime execution hosts', () => { for (const executionHostKind of ['ssh', 'runtime'] as const) { expect( resolveLocalAgentStartupShell({ @@ -44,11 +42,11 @@ describe('resolveLocalAgentStartupShell', () => { executionHostKind, hostLoginShell: '/usr/bin/fish' }) - ).toBeUndefined() + ).toBe('unix') } }) - it('refuses the local dialect when the target platform is not this one (WSL)', () => { + it('uses the portable Unix dialect when the target platform is not this one (WSL)', () => { expect( resolveLocalAgentStartupShell({ ...posixHost, @@ -56,7 +54,7 @@ describe('resolveLocalAgentStartupShell', () => { hostPlatform: 'win32', hostLoginShell: '/usr/bin/fish' }) - ).toBeUndefined() + ).toBe('unix') }) it('keeps the Windows shell families untouched', () => { diff --git a/src/shared/local-agent-startup-shell.ts b/src/shared/local-agent-startup-shell.ts index 31721c16f51..015f7172352 100644 --- a/src/shared/local-agent-startup-shell.ts +++ b/src/shared/local-agent-startup-shell.ts @@ -27,14 +27,16 @@ export function resolveLocalAgentStartupShell( args: LocalAgentStartupShellArgs ): AgentStartupShell | undefined { if (args.platform === 'win32' || args.isRemote) { - return resolveLocalWindowsAgentStartupShell(args) + return ( + resolveLocalWindowsAgentStartupShell(args) ?? (args.platform === 'win32' ? undefined : 'unix') + ) } if (args.executionHostKind === 'ssh' || args.executionHostKind === 'runtime') { - return undefined + return 'unix' } // Why: a WSL workspace resolves to 'linux' on a win32 host — a different shell entirely. if (args.hostPlatform !== args.platform) { - return undefined + return 'unix' } return resolveLoginShellStartupDialect(args.hostLoginShell) } diff --git a/src/shared/tui-agent-startup-shell.test.ts b/src/shared/tui-agent-startup-shell.test.ts index 1827698f29e..2c4317cf751 100644 --- a/src/shared/tui-agent-startup-shell.test.ts +++ b/src/shared/tui-agent-startup-shell.test.ts @@ -70,6 +70,9 @@ describe('fish startup shell dialect', () => { it('clears variables with fish syntax instead of the sh `unset` fish rejects', () => { expect(clearEnvCommand('CODEX_HOME', 'fish')).toBe('set -e CODEX_HOME') expect(clearEnvCommand('CODEX_HOME', 'posix')).toBe('unset CODEX_HOME') + expect(clearEnvCommand('CODEX_HOME', 'unix')).toBe( + `command test -n "$fish_pid" && builtin eval 'set -el CODEX_HOME; set -eg CODEX_HOME; set -eU CODEX_HOME; true' || command eval 'unset CODEX_HOME'` + ) expect(clearEnvCommand('CODEX_HOME', 'cmd')).toBe('set "CODEX_HOME="') expect(clearEnvCommand('CODEX_HOME', 'powershell')).toBe( 'Remove-Item Env:CODEX_HOME -ErrorAction SilentlyContinue' @@ -113,6 +116,16 @@ describe('fish startup shell dialect', () => { } }) + it('keeps unknown-Unix generated arguments valid for both parsers', () => { + for (const value of [String.raw`use \d+ and \\server\share`, "it's mine", 'ends\\']) { + const quoted = quoteStartupArg(value, 'unix') + const fish = tokenizeStartupCommand(quoted, 'fish') + const posix = tokenizeStartupCommand(quoted, 'posix') + expect(fish.ok && fish.tokens).toEqual([value]) + expect(posix.ok && posix.tokens).toEqual([value]) + } + }) + it('maps login shell paths to their dialect', () => { expect(resolveLoginShellStartupDialect('/opt/homebrew/bin/fish')).toBe('fish') expect(resolveLoginShellStartupDialect('/usr/local/bin/FISH')).toBe('fish') diff --git a/src/shared/tui-agent-startup-shell.ts b/src/shared/tui-agent-startup-shell.ts index 28e85533eed..de115d3c142 100644 --- a/src/shared/tui-agent-startup-shell.ts +++ b/src/shared/tui-agent-startup-shell.ts @@ -3,7 +3,7 @@ import { tokenizeFishStartupCommand } from './fish-command-tokenizer' // Why: fish shares POSIX word splitting and `;` chaining, but NOT quoting or // escaping — see quoteStartupArg and fish-command-tokenizer.ts. -export type AgentStartupShell = 'posix' | 'fish' | 'powershell' | 'cmd' +export type AgentStartupShell = 'posix' | 'fish' | 'unix' | 'powershell' | 'cmd' type WindowsStartupShell = Extract @@ -16,7 +16,7 @@ type WindowsStartupShell = Extract * never hardcode `'posix'` behind this check. */ export function isPosixStartupShell(shell: AgentStartupShell): boolean { - return shell === 'posix' || shell === 'fish' + return shell === 'posix' || shell === 'fish' || shell === 'unix' } function isWindowsStartupShell(shell: AgentStartupShell): shell is WindowsStartupShell { @@ -165,7 +165,22 @@ export function tokenizeStartupCommand( if (isWindowsStartupShell(shell)) { return tokenizeWindowsStartupCommand(value, shell) } - return shell === 'fish' ? tokenizeFishStartupCommand(value) : tokenizeCustomCommandTemplate(value) + if (shell === 'fish') { + return tokenizeFishStartupCommand(value) + } + const posix = tokenizeCustomCommandTemplate(value) + if (shell === 'posix' || !posix.ok) { + return posix + } + const fish = tokenizeFishStartupCommand(value) + if ( + !fish.ok || + fish.tokens.length !== posix.tokens.length || + fish.tokens.some((token, index) => token !== posix.tokens[index]) + ) { + return { ok: false, error: 'Command syntax depends on the remote Unix shell.' } + } + return posix } export function resolveStartupShell( @@ -176,7 +191,34 @@ export function resolveStartupShell( if (platform !== 'win32' && agentEnv?.SHELL) { return resolveLoginShellStartupDialect(agentEnv.SHELL) } - return shell ?? (platform === 'win32' ? 'powershell' : 'posix') + return shell ?? (platform === 'win32' ? 'powershell' : 'unix') +} + +function quotePortableUnixArg(value: string): string { + if (!value) { + return "''" + } + const parts: string[] = [] + let literal = '' + const flushLiteral = (): void => { + if (literal) { + parts.push(`'${literal}'`) + literal = '' + } + } + for (const char of value) { + if (char === "'") { + flushLiteral() + parts.push(`"'"`) + } else if (char === '\\') { + flushLiteral() + parts.push(`"\\\\"`) + } else { + literal += char + } + } + flushLiteral() + return parts.join('') } export function quoteStartupArg(value: string, shell: AgentStartupShell): string { @@ -186,6 +228,9 @@ export function quoteStartupArg(value: string, shell: AgentStartupShell): string if (shell === 'cmd') { return `"${value.replace(/([\^&|<>()%!"])/g, '^$1')}"` } + if (shell === 'unix') { + return quotePortableUnixArg(value) + } // Why: fish single quotes are NOT literal. `\\` and `\'` are escapes inside // them, so the sh `'\''` idiom collapses every backslash (a `\d+` regex, a // `\\server\share` UNC path) and a trailing backslash is a hard syntax error @@ -220,6 +265,10 @@ export function clearEnvCommand(name: string, shell: AgentStartupShell): string if (shell === 'fish') { return `set -e ${name}` } + if (shell === 'unix') { + // Why: inherited globals can hide universal values, so erase every Fish scope before the sh fallback. + return `command test -n "$fish_pid" && builtin eval 'set -el ${name}; set -eg ${name}; set -eU ${name}; true' || command eval 'unset ${name}'` + } return `unset ${name}` } diff --git a/src/shared/tui-agent-startup.test.ts b/src/shared/tui-agent-startup.test.ts index b3220956677..9d88a94db3b 100644 --- a/src/shared/tui-agent-startup.test.ts +++ b/src/shared/tui-agent-startup.test.ts @@ -39,7 +39,7 @@ describe('tui agent startup plans', () => { } ) - it('uses POSIX quoting when the target shell is Linux', () => { + it('uses portable Unix quoting when the target shell is unknown', () => { const plan = buildAgentStartupPlan({ agent: 'claude', prompt: "fix Bob's branch", @@ -47,7 +47,7 @@ describe('tui agent startup plans', () => { platform: 'linux' }) - expect(plan?.launchCommand).toBe("claude 'fix Bob'\\''s branch'") + expect(plan?.launchCommand).toBe(`claude 'fix Bob'"'"'s branch'`) }) it('uses PowerShell quoting by default when the target shell is Windows', () => { @@ -180,7 +180,8 @@ describe('tui agent startup plans', () => { agent: 'hermes', prompt: 'run it', cmdOverrides: {}, - platform: 'linux' + platform: 'linux', + shell: 'posix' }) expect(plan?.launchCommand).toMatch(/^sh -c /) @@ -850,7 +851,8 @@ describe('tui agent startup plans', () => { agent: 'omp', draft: 'fix the omp regression', cmdOverrides: {}, - platform: 'linux' + platform: 'linux', + shell: 'posix' }) expect(plan).not.toBeNull() diff --git a/src/shared/windows-terminal-shell.test.ts b/src/shared/windows-terminal-shell.test.ts index e7d86a07cc9..246db4bd14e 100644 --- a/src/shared/windows-terminal-shell.test.ts +++ b/src/shared/windows-terminal-shell.test.ts @@ -21,15 +21,15 @@ describe('resolveWindowsShellStartupFamily', () => { expect(resolveWindowsShellStartupFamily('C:\\Windows\\System32\\cmd.exe')).toBe('cmd') }) - it('maps Git Bash and WSL shells to POSIX quoting', () => { + it('maps Git Bash to POSIX and WSL to portable Unix quoting', () => { expect(resolveWindowsShellStartupFamily('git-bash')).toBe('posix') - expect(resolveWindowsShellStartupFamily('wsl.exe')).toBe('posix') + expect(resolveWindowsShellStartupFamily('wsl.exe')).toBe('unix') expect(resolveWindowsShellStartupFamily('C:\\Program Files\\Git\\bin\\bash.exe')).toBe('posix') }) - it('maps extension-less bash and wsl entries to POSIX quoting', () => { + it('maps extension-less bash and wsl entries to their Unix quoting', () => { expect(resolveWindowsShellStartupFamily('bash')).toBe('posix') - expect(resolveWindowsShellStartupFamily('wsl')).toBe('posix') + expect(resolveWindowsShellStartupFamily('wsl')).toBe('unix') expect(resolveWindowsShellStartupFamily('C:\\Program Files\\Git\\bin\\bash')).toBe('posix') }) }) diff --git a/src/shared/windows-terminal-shell.ts b/src/shared/windows-terminal-shell.ts index 1f55bf89d35..1700df3aa80 100644 --- a/src/shared/windows-terminal-shell.ts +++ b/src/shared/windows-terminal-shell.ts @@ -31,12 +31,10 @@ export function resolveWindowsShellStartupFamily( // Why: wsl.exe and bash.exe (Git for Windows) launch POSIX shells, so queued // commands must use POSIX quoting and `cd ''` rather than cmd/PowerShell. // Extension-less forms reach the same executables through PATHEXT. - if ( - basename === 'wsl.exe' || - basename === 'wsl' || - basename === 'bash.exe' || - basename === 'bash' - ) { + if (basename === 'wsl.exe' || basename === 'wsl') { + return 'unix' + } + if (basename === 'bash.exe' || basename === 'bash') { return 'posix' } return 'powershell'