From 0b2f698991643aa75ff0b8e802de6fbea863793a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 12 Aug 2026 22:17:10 -0700 Subject: [PATCH] fix(history): harden cleanup ownership --- src/main/fish-history-session.test.ts | 104 +++++++++ src/main/fish-history-session.ts | 216 ++++++++++++++---- src/main/ipc/worktrees.test.ts | 128 +++++++++-- src/main/ipc/worktrees.ts | 30 ++- src/main/providers/git-provider-contract.ts | 2 +- src/main/providers/ssh-git-provider.test.ts | 7 +- src/main/providers/ssh-git-provider.ts | 2 +- .../remote-worktree-history-cleanup.test.ts | 30 +++ src/main/remote-worktree-history-cleanup.ts | 15 ++ src/main/runtime/orca-runtime.test.ts | 149 +++++++++++- src/main/runtime/orca-runtime.ts | 33 ++- src/main/terminal-history-deletion.ts | 7 +- src/main/terminal-history.test.ts | 94 +++----- src/main/terminal-history.ts | 13 +- src/relay/fish-history-metadata.test.ts | 32 ++- src/relay/fish-history-metadata.ts | 59 +---- src/relay/git-handler.test.ts | 31 --- src/relay/git-handler.ts | 10 +- 18 files changed, 695 insertions(+), 267 deletions(-) create mode 100644 src/main/fish-history-session.test.ts create mode 100644 src/main/remote-worktree-history-cleanup.test.ts create mode 100644 src/main/remote-worktree-history-cleanup.ts diff --git a/src/main/fish-history-session.test.ts b/src/main/fish-history-session.test.ts new file mode 100644 index 00000000000..bc0842b5929 --- /dev/null +++ b/src/main/fish-history-session.test.ts @@ -0,0 +1,104 @@ +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + attestFishHistoryLocation, + deleteFishHistoryFile, + MAX_RETAINED_FISH_HISTORY_PATHS +} from './fish-history-session' + +describe('fish history location attestation', () => { + const session = 'orca_0123456789abcdef' + let root: string + let attestationPath: string + + beforeEach(() => { + root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-fish-attestation-'))) + attestationPath = join(root, 'orca-history', 'fish-history-locations.json') + }) + + afterEach(() => { + rmSync(root, { recursive: true, force: true }) + }) + + function historyPath(dataRoot: string): string { + return join(dataRoot, 'fish', `${session}_history`) + } + + it('deletes each attested XDG location and caps retained identities', () => { + const paths: string[] = [] + for (let index = 0; index < MAX_RETAINED_FISH_HISTORY_PATHS + 2; index += 1) { + const path = historyPath(join(root, `data-${index}`)) + attestFishHistoryLocation(attestationPath, session, path) + writeFileSync(path, `history-${index}`) + paths.push(path) + } + + deleteFishHistoryFile(session, attestationPath) + + expect(existsSync(paths[0])).toBe(true) + expect(existsSync(paths[1])).toBe(true) + expect(paths.slice(2).every((path) => !existsSync(path))).toBe(true) + }) + + it('ignores a tampered meta.json path to an unrelated same-named file', () => { + const owned = historyPath(join(root, 'owned-data')) + const unrelated = historyPath(join(root, 'unrelated-data')) + attestFishHistoryLocation(attestationPath, session, owned) + writeFileSync(owned, 'owned') + mkdirSync(join(root, 'unrelated-data', 'fish'), { recursive: true }) + writeFileSync(unrelated, 'unrelated') + writeFileSync( + join(root, 'orca-history', 'meta.json'), + JSON.stringify({ fishSession: session, fishHistoryPath: unrelated }) + ) + + deleteFishHistoryFile(session, attestationPath) + + expect(existsSync(owned)).toBe(false) + expect(readFileSync(unrelated, 'utf8')).toBe('unrelated') + }) + + it.skipIf(process.platform === 'win32')( + 'rejects a fish parent replaced by a symlink before cleanup', + () => { + const dataRoot = join(root, 'data') + const owned = historyPath(dataRoot) + attestFishHistoryLocation(attestationPath, session, owned) + rmSync(join(dataRoot, 'fish'), { recursive: true }) + + const targetDir = join(root, 'target-fish') + mkdirSync(targetDir) + const target = join(targetDir, `${session}_history`) + writeFileSync(target, 'unrelated') + symlinkSync(targetDir, join(dataRoot, 'fish'), 'dir') + + deleteFishHistoryFile(session, attestationPath) + + expect(readFileSync(target, 'utf8')).toBe('unrelated') + } + ) + + it('rejects a recreated fish directory with a different identity', () => { + const dataRoot = join(root, 'data') + const owned = historyPath(dataRoot) + attestFishHistoryLocation(attestationPath, session, owned) + rmSync(join(dataRoot, 'fish'), { recursive: true }) + mkdirSync(join(dataRoot, 'fish')) + writeFileSync(owned, 'replacement') + + deleteFishHistoryFile(session, attestationPath) + + expect(readFileSync(owned, 'utf8')).toBe('replacement') + }) +}) diff --git a/src/main/fish-history-session.ts b/src/main/fish-history-session.ts index 4f9899a9e98..ab2672afe33 100644 --- a/src/main/fish-history-session.ts +++ b/src/main/fish-history-session.ts @@ -1,6 +1,6 @@ import { homedir } from 'node:os' -import { basename, dirname, isAbsolute, join } from 'node:path' -import { existsSync, rmSync } from 'node:fs' +import { basename, dirname, isAbsolute, join, parse, relative, resolve, sep } from 'node:path' +import { existsSync, lstatSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' /** * Per-worktree fish history, which fish models as a session NAME rather than a path. @@ -27,6 +27,15 @@ 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 const FISH_HISTORY_LOCATION_ATTESTATION = 'fish-history-locations.json' + +type FishHistoryLocationAttestation = { + path: string + directoryDevice: string + directoryInode: string + directoryBirthtimeNs: string +} +type FishHistoryDirectoryIdentity = Omit export function isSafeFishHistorySession(session: unknown): session is string { return typeof session === 'string' && SAFE_SESSION_NAME.test(session) @@ -53,21 +62,147 @@ export function resolveFishHistoryFilePath( return join(dataHome, 'fish', `${session}_history`) } -/** - * 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. - */ -export function isTrustedFishHistoryPath(session: string, path: unknown): path is string { +/** Validate path shape only; deletion authority comes from a separate identity attestation. */ +function isCanonicalFishHistoryPath(session: string, path: unknown): path is string { return Boolean( typeof path === 'string' && path && isSafeFishHistorySession(session) && isAbsolute(path) && + resolve(path) === path && basename(dirname(path)) === 'fish' && basename(path) === `${session}_history` ) } +function fishHistoryDirectoryIdentity(path: string): FishHistoryDirectoryIdentity | null { + const directory = dirname(path) + const root = parse(directory).root + let current = root + try { + const segments = relative(root, directory).split(sep).filter(Boolean) + const roots = + segments.length === 0 ? [root] : segments.map((segment) => (current = join(current, segment))) + let identity: FishHistoryDirectoryIdentity | null = null + for (const candidate of roots) { + const stat = lstatSync(candidate, { bigint: true }) + if (stat.isSymbolicLink() || !stat.isDirectory()) { + return null + } + identity = { + directoryDevice: stat.dev.toString(), + directoryInode: stat.ino.toString(), + directoryBirthtimeNs: stat.birthtimeNs.toString() + } + } + return identity + } catch { + return null + } +} + +function readFishHistoryLocationAttestations( + attestationPath: string, + session: string +): FishHistoryLocationAttestation[] { + try { + const stat = lstatSync(attestationPath) + if (stat.isSymbolicLink() || !stat.isFile() || stat.size > MAX_FISH_HISTORY_META_BYTES) { + return [] + } + const raw: unknown = JSON.parse(readFileSync(attestationPath, 'utf8')) + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + return [] + } + const record = raw as Record + if ( + record.version !== 1 || + record.fishSession !== session || + !Array.isArray(record.locations) + ) { + return [] + } + const candidates = record.locations.slice(-MAX_FISH_HISTORY_PATH_CANDIDATES) + const retained: FishHistoryLocationAttestation[] = [] + const seen = new Set() + for (let index = candidates.length - 1; index >= 0; index -= 1) { + const candidate = candidates[index] + if (!candidate || typeof candidate !== 'object' || Array.isArray(candidate)) { + continue + } + const location = candidate as Record + if ( + !isCanonicalFishHistoryPath(session, location.path) || + typeof location.directoryDevice !== 'string' || + typeof location.directoryInode !== 'string' || + typeof location.directoryBirthtimeNs !== 'string' || + seen.has(location.path) + ) { + continue + } + seen.add(location.path) + retained.push({ + path: location.path, + directoryDevice: location.directoryDevice, + directoryInode: location.directoryInode, + directoryBirthtimeNs: location.directoryBirthtimeNs + }) + if (retained.length === MAX_RETAINED_FISH_HISTORY_PATHS) { + break + } + } + return retained.toReversed() + } catch { + return [] + } +} + +/** Record a spawn-derived fish location outside meta.json, bound to its directory identity. */ +export function attestFishHistoryLocation( + attestationPath: string, + session: string, + historyPath: string | null +): void { + if (!historyPath || !isCanonicalFishHistoryPath(session, historyPath)) { + return + } + try { + mkdirSync(dirname(historyPath), { recursive: true, mode: 0o700 }) + const identity = fishHistoryDirectoryIdentity(historyPath) + if (!identity) { + console.warn(`[pty:history] Refusing to attest symlinked fish history path ${historyPath}`) + return + } + const retained = readFishHistoryLocationAttestations(attestationPath, session) + const active = retained.at(-1) + if ( + active?.path === historyPath && + active.directoryDevice === identity.directoryDevice && + active.directoryInode === identity.directoryInode && + active.directoryBirthtimeNs === identity.directoryBirthtimeNs + ) { + return + } + const existing = retained.filter((entry) => entry.path !== historyPath) + const locations = [...existing, { path: historyPath, ...identity }].slice( + -MAX_RETAINED_FISH_HISTORY_PATHS + ) + mkdirSync(dirname(attestationPath), { recursive: true, mode: 0o700 }) + if (existsSync(attestationPath) && lstatSync(attestationPath).isSymbolicLink()) { + return + } + writeFileSync( + attestationPath, + JSON.stringify({ version: 1, fishSession: session, locations }), + { mode: 0o600 } + ) + } catch (error) { + console.warn( + `[pty:history] Failed to attest fish history location: ${error instanceof Error ? error.message : String(error)}` + ) + } +} + export function normalizeFishHistoryPaths( session: string, singularPath: unknown, @@ -77,7 +212,7 @@ export function normalizeFishHistoryPaths( const newestFirst: string[] = [] const seen = new Set() const retain = (path: unknown): void => { - if (!isTrustedFishHistoryPath(session, path) || seen.has(path)) { + if (!isCanonicalFishHistoryPath(session, path) || seen.has(path)) { return } seen.add(path) @@ -102,50 +237,37 @@ export function normalizeFishHistoryPaths( return retained } -/** - * Removes one worktree's fish history file. - * - * `recordedPath` is the path resolved from the PTY's own spawn env when the - * session was minted; the main process env is only a fallback, because the two - * disagree whenever Orca was launched with a different `XDG_DATA_HOME`/`HOME` - * than the shells it spawns. - * - * Neither can see a `set -gx XDG_DATA_HOME …` that lives inside the user's - * config.fish — fish resolves that after we have handed off. That file cannot be - * found from here, so it is reported instead of being dropped silently. - */ -export function deleteFishHistoryFile( - session: string, - options: { - recordedPath?: string | null - recordedPaths?: readonly unknown[] - env?: NodeJS.ProcessEnv - } = {} -): void { - const candidates = new Set() - if (isTrustedFishHistoryPath(session, options.recordedPath)) { - candidates.add(options.recordedPath) - } - for (const path of (options.recordedPaths ?? []).slice(0, MAX_RETAINED_FISH_HISTORY_PATHS)) { - if (isTrustedFishHistoryPath(session, path)) { - candidates.add(path) - } - } - const fromEnv = resolveFishHistoryFilePath(session, options.env ?? process.env) - if (fromEnv) { - candidates.add(fromEnv) - } - if (candidates.size === 0) { +/** Delete only spawn-attested locations whose non-symlink directory identity still matches. */ +export function deleteFishHistoryFile(session: string, attestationPath: string): void { + const locations = readFishHistoryLocationAttestations(attestationPath, session) + if (locations.length === 0) { + console.warn(`[pty:history] No attested fish history location for session ${session}`) return } let removed = false - for (const path of candidates) { + for (const location of locations) { try { - removed ||= existsSync(path) - rmSync(path, { force: true }) + const identity = fishHistoryDirectoryIdentity(location.path) + if ( + !identity || + identity.directoryDevice !== location.directoryDevice || + identity.directoryInode !== location.directoryInode || + identity.directoryBirthtimeNs !== location.directoryBirthtimeNs + ) { + console.warn(`[pty:history] Refusing fish history cleanup after directory identity changed`) + continue + } + if (existsSync(location.path)) { + const stat = lstatSync(location.path) + if (stat.isSymbolicLink() || !stat.isFile()) { + continue + } + removed = true + } + rmSync(location.path, { force: true }) } catch (err) { console.warn( - `[pty:history] Failed to delete fish history ${path}: ${err instanceof Error ? err.message : String(err)}` + `[pty:history] Failed to delete fish history ${location.path}: ${err instanceof Error ? err.message : String(err)}` ) } } @@ -153,7 +275,7 @@ export function deleteFishHistoryFile( // Explicit: a config.fish-set XDG_DATA_HOME leaves a file we cannot locate, // and silence would hide one leaked history file per deleted worktree. console.warn( - `[pty:history] No fish history file found for session ${session}; if fish keeps history outside ${[...candidates].join(' or ')} (e.g. XDG_DATA_HOME set in config.fish) that file is left behind.` + `[pty:history] No attested fish history file found for session ${session}; a config.fish-only XDG_DATA_HOME remains outside Orca's cleanup authority.` ) } } diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index b37009d49dd..24537a613df 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -8895,9 +8895,7 @@ describe('registerWorktreeHandlers', () => { ORCA_WORKTREE_PATH: '/remote/feature-wt' }) ) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) expect(runtimeStub.closeFileWatchersForRemoval).toHaveBeenCalledWith( '/remote/feature-wt', 'conn-1' @@ -9023,9 +9021,7 @@ describe('registerWorktreeHandlers', () => { }) expect(provider.worktreeIsClean).not.toHaveBeenCalled() - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', true, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', true) }) it('continues SSH worktree removal when the archive hook fails', async () => { @@ -9081,9 +9077,7 @@ describe('registerWorktreeHandlers', () => { await handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) expect(consoleErrorSpy).toHaveBeenCalledWith( '[hooks] archive hook failed for /remote/feature-wt:', expect.stringContaining('archive hook exited 7') @@ -9141,9 +9135,7 @@ describe('registerWorktreeHandlers', () => { await handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) expect(consoleErrorSpy).toHaveBeenCalledWith( '[hooks] archive hook failed for /remote/feature-wt:', 'relay disconnected' @@ -9268,9 +9260,7 @@ describe('registerWorktreeHandlers', () => { }) expect(provider.execNonInteractive).not.toHaveBeenCalled() - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) }) it('uses the workspace host when duplicate repo ids exist across local and SSH', async () => { @@ -9317,9 +9307,7 @@ describe('registerWorktreeHandlers', () => { hostId: 'ssh:conn-1' }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined, { - worktreeId: 'repo-shared::/remote/feature-wt' - }) + expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) expect(removeWorktreeMock).not.toHaveBeenCalled() }) @@ -9839,6 +9827,110 @@ describe('registerWorktreeHandlers', () => { }) }) + it('routes already-missing SSH history cleanup through the PTY owner', async () => { + const repo = { + id: 'repo-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1' + } + const worktreeId = 'repo-ssh::/remote/already-deleted' + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: repo.path, + head: 'main', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + } + const fsProvider = { + stat: vi.fn().mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + } + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + store.getRepo.mockReturnValue(repo) + store.getWorktreeMeta.mockReturnValue( + makeWorktreeMeta({ hostId: 'ssh:conn-1', orcaCreationSource: 'ssh' }) + ) + getSshGitProviderMock.mockReturnValue(gitProvider) + getSshFilesystemProviderMock.mockReturnValue(fsProvider) + getSshPtyProviderMock.mockReturnValue({ deleteWorktreeHistory } as never) + + await handlers['worktrees:remove'](null, { worktreeId }) + + expect(deleteWorktreeHistory).toHaveBeenCalledWith(worktreeId) + expect(deleteWorktreeHistory.mock.invocationCallOrder[0]).toBeLessThan( + store.removeWorktreeMeta.mock.invocationCallOrder[0] + ) + expect(store.removeWorktreeMeta).toHaveBeenCalledWith(worktreeId, 'ssh:conn-1') + }) + + it('routes SSH orphan-directory history cleanup through the PTY owner', async () => { + const repo = { + id: 'repo-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1' + } + const worktreePath = '/remote/orphan' + const worktreeId = `${repo.id}::${worktreePath}` + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: repo.path, + head: 'main', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + } + const fsProvider = { + lstat: vi.fn(async (path: string) => ({ + type: path === `${worktreePath}/.git` ? 'file' : 'directory' + })), + readFile: vi.fn(async (path: string) => ({ + isBinary: false, + content: + path === `${worktreePath}/.git` + ? `gitdir: ${repo.path}/.git/worktrees/orphan\n` + : `${worktreePath}/.git\n` + })), + deletePath: vi.fn().mockResolvedValue(undefined) + } + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + const ptyProvider = { + listProcesses: vi.fn().mockResolvedValue([]), + shutdown: vi.fn().mockResolvedValue(undefined), + deleteWorktreeHistory + } + store.getRepo.mockReturnValue(repo) + store.getWorktreeMeta.mockReturnValue( + makeWorktreeMeta({ + hostId: 'ssh:conn-1', + orcaCreatedAt: Date.now(), + orcaCreationSource: 'ssh' + }) + ) + getSshGitProviderMock.mockReturnValue(gitProvider) + getSshFilesystemProviderMock.mockReturnValue(fsProvider) + getSshPtyProviderMock.mockReturnValue(ptyProvider as never) + + await handlers['worktrees:remove'](null, { worktreeId, force: true }) + + expect(fsProvider.deletePath).toHaveBeenCalledWith(worktreePath, true) + expect(deleteWorktreeHistory).toHaveBeenCalledWith(worktreeId) + expect(deleteWorktreeHistory.mock.invocationCallOrder[0]).toBeLessThan( + store.removeWorktreeMeta.mock.invocationCallOrder[0] + ) + }) + it('force-removes a legacy Orca-created orphaned worktree directory after Git tracking is gone', async () => { const parentDir = await mkdtemp(join(tmpdir(), 'orca-ipc-orphan-')) const repoPath = join(parentDir, 'repo') diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index cb0642b3daf..da66e60dadc 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -249,6 +249,7 @@ import { removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval, recoverLocalWindowsWorktreeRemoval } from '../local-worktree-removal-recovery' +import { deleteRemoteWorktreeHistory } from '../remote-worktree-history-cleanup' const NullableWorkspaceLinkedItemSchema = WorkspaceLinkedItemSchema.nullable() const NullableTaskSourceContextSchema = TaskSourceContextSchema.nullable() @@ -2417,9 +2418,7 @@ 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) - }) + await deleteRemoteWorktreeHistory(sshPtyProvider, args.worktreeId) removeWorktreeMetadataAndTransientState(store, args.worktreeId, removalHostId) }) preservedBranchCleanupByScope.delete( @@ -2510,6 +2509,10 @@ export function registerWorktreeHandlers( removedPushTarget, store ) + await deleteRemoteWorktreeHistory( + getSshPtyProvider(repo.connectionId), + args.worktreeId + ) } else { const removalGate = await runtime.acquireFileWatcherRemoval(worktreePath) let removalCompleted = false @@ -2608,6 +2611,10 @@ export function registerWorktreeHandlers( removedPushTarget, store ) + await deleteRemoteWorktreeHistory( + getSshPtyProvider(repo.connectionId), + args.worktreeId + ) } else { await cleanupUnusedWorktreePushTargetRemote( repo.path, @@ -2726,10 +2733,7 @@ export function registerWorktreeHandlers( } } - const remoteRemoveOptions = { - ...(!deleteBranch ? { deleteBranch } : {}), - worktreeId: args.worktreeId - } + const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} const removalGate = await withWorktreeRemoveStageSpan( 'watcher_gate', 'remote', @@ -2749,7 +2753,13 @@ export function registerWorktreeHandlers( 'git_remove', 'remote', async () => - provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) + Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree( + canonicalWorktreePath, + args.force, + remoteRemoveOptions + ) + : provider!.removeWorktree(canonicalWorktreePath, args.force) ) removalCompleted = true } finally { @@ -2766,6 +2776,10 @@ export function registerWorktreeHandlers( removedPushTarget, store ) + await deleteRemoteWorktreeHistory( + getSshPtyProvider(remoteConnectionId), + args.worktreeId + ) rememberPreservedBranchCleanupTarget( args.worktreeId, removalHostId, diff --git a/src/main/providers/git-provider-contract.ts b/src/main/providers/git-provider-contract.ts index 8b19a9c34f7..d13bf5addf9 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; worktreeId?: string } + options?: { deleteBranch?: boolean; forceBranchDelete?: boolean } ): Promise renameCurrentBranch?(worktreePath: string, newBranch: string): Promise forceDeletePreservedBranch?( diff --git a/src/main/providers/ssh-git-provider.test.ts b/src/main/providers/ssh-git-provider.test.ts index 71c85d6bef7..20ebd9293a1 100644 --- a/src/main/providers/ssh-git-provider.test.ts +++ b/src/main/providers/ssh-git-provider.test.ts @@ -1566,13 +1566,10 @@ describe('SshGitProvider', () => { }) it('removeWorktree sends git.removeWorktree request', async () => { - await provider.removeWorktree('/home/user/feat', true, { - worktreeId: 'repo-1::/home/user/feat' - }) + await provider.removeWorktree('/home/user/feat', true) expect(mux.request).toHaveBeenCalledWith('git.removeWorktree', { worktreePath: '/home/user/feat', - force: true, - worktreeId: 'repo-1::/home/user/feat' + force: true }) }) diff --git a/src/main/providers/ssh-git-provider.ts b/src/main/providers/ssh-git-provider.ts index ee791e2d1e3..51e2b91e1c2 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; worktreeId?: string } + options?: { deleteBranch?: boolean; forceBranchDelete?: boolean } ): Promise { return this.runWithDiffDedupeClear( async () => diff --git a/src/main/remote-worktree-history-cleanup.test.ts b/src/main/remote-worktree-history-cleanup.test.ts new file mode 100644 index 00000000000..ce0f23a60f9 --- /dev/null +++ b/src/main/remote-worktree-history-cleanup.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from 'vitest' +import { deleteRemoteWorktreeHistory } from './remote-worktree-history-cleanup' + +describe('deleteRemoteWorktreeHistory', () => { + it('repeats idempotent cleanup through the PTY owner', async () => { + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + const provider = { deleteWorktreeHistory } as never + + await deleteRemoteWorktreeHistory(provider, 'repo-1::/remote/wt') + await deleteRemoteWorktreeHistory(provider, 'repo-1::/remote/wt') + + expect(deleteWorktreeHistory).toHaveBeenCalledTimes(2) + }) + + it('degrades safely when an old relay does not expose cleanup', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const provider = { + deleteWorktreeHistory: vi.fn().mockRejectedValue(new Error('unknown request method')) + } as never + + try { + await expect( + deleteRemoteWorktreeHistory(provider, 'repo-1::/remote/wt') + ).resolves.toBeUndefined() + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Remote cleanup unavailable')) + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/main/remote-worktree-history-cleanup.ts b/src/main/remote-worktree-history-cleanup.ts new file mode 100644 index 00000000000..8842758d15e --- /dev/null +++ b/src/main/remote-worktree-history-cleanup.ts @@ -0,0 +1,15 @@ +import type { IPtyProvider } from './providers/pty-provider-contract' + +/** Best-effort cleanup through the PTY owner; older relays may not expose the method. */ +export async function deleteRemoteWorktreeHistory( + provider: IPtyProvider | undefined, + worktreeId: string +): Promise { + try { + await provider?.deleteWorktreeHistory?.(worktreeId) + } catch (error) { + console.warn( + `[pty:history] Remote cleanup unavailable: ${error instanceof Error ? error.message : String(error)}` + ) + } +} diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 517f22ade34..ead3debf80e 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -6052,7 +6052,8 @@ describe('OrcaRuntimeService', () => { worktreeId: `${TEST_REPO_ID}::/remote/feature` } ]), - shutdown: vi.fn().mockResolvedValue(undefined) + shutdown: vi.fn().mockResolvedValue(undefined), + deleteWorktreeHistory: vi.fn().mockResolvedValue(undefined) } const runtime = new OrcaRuntimeService(remoteStore as never, undefined, { getSshProvider: () => ptyProvider as never @@ -6064,13 +6065,14 @@ describe('OrcaRuntimeService', () => { unregisterSshGitProvider('ssh-1') } - expect(gitProvider.removeWorktree).toHaveBeenCalledWith('/remote/feature', true, { - worktreeId: `${TEST_REPO_ID}::/remote/feature` - }) + expect(gitProvider.removeWorktree).toHaveBeenCalledWith('/remote/feature', true) expect(ptyProvider.shutdown).toHaveBeenCalledWith( 'pty-remote', expect.objectContaining({ immediate: true }) ) + expect(ptyProvider.deleteWorktreeHistory).toHaveBeenCalledWith( + `${TEST_REPO_ID}::/remote/feature` + ) expect(ptyProvider.shutdown.mock.invocationCallOrder[0]).toBeLessThan( gitProvider.removeWorktree.mock.invocationCallOrder[0] ) @@ -47103,6 +47105,145 @@ describe('OrcaRuntimeService', () => { } }) + it('routes already-missing SSH runtime history cleanup through the PTY owner', async () => { + const repo = { + id: 'repo-runtime-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: 'blue', + addedAt: 1, + connectionId: 'ssh-1' + } + const worktreeId = `${repo.id}::/remote/already-deleted` + const metaById: Record = { + [worktreeId]: makeWorktreeMeta({ hostId: 'ssh:ssh-1', orcaCreationSource: 'ssh' }) + } + const removeWorktreeMeta = vi.fn((id: string) => { + delete metaById[id] + }) + const runtimeStore = { + ...store, + getRepos: () => [repo], + getRepo: (id: string) => (id === repo.id ? repo : undefined), + getAllWorktreeMeta: () => metaById, + getWorktreeMeta: (id: string) => metaById[id], + removeWorktreeMeta + } + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: repo.path, + head: 'main', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + } + const fsProvider = { + stat: vi.fn().mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })) + } + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + const ptyProvider = { deleteWorktreeHistory } as never + registerSshGitProvider(repo.connectionId, gitProvider as never) + registerSshFilesystemProvider(repo.connectionId, fsProvider as never) + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getSshProvider: () => ptyProvider + }) + + try { + await expect(runtime.removeManagedWorktree(`id:${worktreeId}`)).resolves.toEqual({}) + } finally { + unregisterSshGitProvider(repo.connectionId) + unregisterSshFilesystemProvider(repo.connectionId) + } + + expect(deleteWorktreeHistory).toHaveBeenCalledWith(worktreeId) + expect(deleteWorktreeHistory.mock.invocationCallOrder[0]).toBeLessThan( + removeWorktreeMeta.mock.invocationCallOrder[0] + ) + expect(removeWorktreeMeta).toHaveBeenCalledWith(worktreeId, 'ssh:ssh-1') + }) + + it('routes SSH runtime orphan-directory history cleanup through the PTY owner', async () => { + const repo = { + id: 'repo-runtime-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: 'blue', + addedAt: 1, + connectionId: 'ssh-1' + } + const worktreePath = '/remote/orphan' + const worktreeId = `${repo.id}::${worktreePath}` + const metaById: Record = { + [worktreeId]: makeWorktreeMeta({ + hostId: 'ssh:ssh-1', + orcaCreatedAt: Date.now(), + orcaCreationSource: 'ssh' + }) + } + const removeWorktreeMeta = vi.fn((id: string) => { + delete metaById[id] + }) + const runtimeStore = { + ...store, + getRepos: () => [repo], + getRepo: (id: string) => (id === repo.id ? repo : undefined), + getAllWorktreeMeta: () => metaById, + getWorktreeMeta: (id: string) => metaById[id], + removeWorktreeMeta + } + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: repo.path, + head: 'main', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + } + ]) + } + const fsProvider = { + lstat: vi.fn(async (path: string) => ({ + type: path === `${worktreePath}/.git` ? 'file' : 'directory' + })), + readFile: vi.fn(async (path: string) => ({ + isBinary: false, + content: + path === `${worktreePath}/.git` + ? `gitdir: ${repo.path}/.git/worktrees/orphan\n` + : `${worktreePath}/.git\n` + })), + deletePath: vi.fn().mockResolvedValue(undefined) + } + const deleteWorktreeHistory = vi.fn().mockResolvedValue(undefined) + const ptyProvider = { + listProcesses: vi.fn().mockResolvedValue([]), + shutdown: vi.fn().mockResolvedValue(undefined), + deleteWorktreeHistory + } + registerSshGitProvider(repo.connectionId, gitProvider as never) + registerSshFilesystemProvider(repo.connectionId, fsProvider as never) + const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { + getSshProvider: () => ptyProvider as never + }) + + try { + await expect(runtime.removeManagedWorktree(`id:${worktreeId}`, true)).resolves.toEqual({}) + } finally { + unregisterSshGitProvider(repo.connectionId) + unregisterSshFilesystemProvider(repo.connectionId) + } + + expect(fsProvider.deletePath).toHaveBeenCalledWith(worktreePath, true) + expect(deleteWorktreeHistory).toHaveBeenCalledWith(worktreeId) + expect(deleteWorktreeHistory.mock.invocationCallOrder[0]).toBeLessThan( + removeWorktreeMeta.mock.invocationCallOrder[0] + ) + }) + it('force-removes a legacy Orca-created runtime orphaned worktree directory after Git tracking is gone', async () => { const parentDir = await mkdtemp(join(tmpdir(), 'orca-runtime-orphan-')) const repoPath = join(parentDir, 'repo') diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index a119855aee3..cd20b5266ed 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -918,6 +918,7 @@ import { resolveWorktreeSharedDirectories } from '../git/worktree-shared-directories' import { deleteWorktreeHistoryDir } from '../terminal-history-deletion' +import { deleteRemoteWorktreeHistory } from '../remote-worktree-history-cleanup' import { cleanupUnusedWorktreePushTargetRemote, cleanupUnusedWorktreePushTargetRemoteSsh, @@ -24451,6 +24452,7 @@ export class OrcaRuntimeService { .then((gate) => gate.finish(false)) .catch(() => {}) } + await deleteRemoteWorktreeHistory(sshPtyProvider, removalTarget.id) this.clearOptimisticReconcileToken(removalTarget.id) this.removeWorktreeMetadataAndHistory(store, removalTarget.id) this.preservedBranchCleanupByScope.delete(cleanupScopeKey) @@ -24495,9 +24497,7 @@ 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) - }) + await deleteRemoteWorktreeHistory(folderSshPtyProvider, removalTarget.id) this.removeWorktreeMetadataAndHistory(store, removalTarget.id) this.preservedBranchCleanupByScope.delete(cleanupScopeKey) this.invalidateResolvedWorktreeCache() @@ -24582,6 +24582,10 @@ export class OrcaRuntimeService { removedPushTarget, store ) + await deleteRemoteWorktreeHistory( + this.getSshProviderFn?.(repo.connectionId), + removalTarget.id + ) } else { const removalGate = await this.acquireFileWatcherRemoval(removalTarget.path) let removalCompleted = false @@ -24687,6 +24691,12 @@ export class OrcaRuntimeService { store, localWorktreeGitOptions )) + if (repo.connectionId) { + await deleteRemoteWorktreeHistory( + this.getSshProviderFn?.(repo.connectionId), + removalTarget.id + ) + } this.clearOptimisticReconcileToken(removalTarget.id) this.removeWorktreeMetadataAndHistory(store, removalTarget.id) this.preservedBranchCleanupByScope.delete(cleanupScopeKey) @@ -24750,10 +24760,7 @@ export class OrcaRuntimeService { return removalResult ?? {} } if (repo.connectionId) { - const remoteRemoveOptions = { - ...(!deleteBranch ? { deleteBranch } : {}), - worktreeId: removalTarget.id - } + const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} const removalGate = await this.acquireFileWatcherRemoval( canonicalWorktreePath, repo.connectionId @@ -24765,11 +24772,9 @@ export class OrcaRuntimeService { connectionId: repo.connectionId, allowUnverifiedStop: allowUnverifiedPtyStop }) - rawRemovalResult = await provider!.removeWorktree( - canonicalWorktreePath, - force, - remoteRemoveOptions - ) + rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, force)) removalCompleted = true } finally { await removalGate.finish(removalCompleted) @@ -24785,6 +24790,10 @@ export class OrcaRuntimeService { removedPushTarget, store ) + await deleteRemoteWorktreeHistory( + this.getSshProviderFn?.(repo.connectionId), + removalTarget.id + ) this.rememberPreservedBranchCleanupTarget( removalTarget.id, cleanupHostId, diff --git a/src/main/terminal-history-deletion.ts b/src/main/terminal-history-deletion.ts index 814015a0eca..4f3ce56833f 100644 --- a/src/main/terminal-history-deletion.ts +++ b/src/main/terminal-history-deletion.ts @@ -1,7 +1,7 @@ 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 { deleteFishHistoryFile, FISH_HISTORY_LOCATION_ATTESTATION } from './fish-history-session' import { readHistoryMeta } from './terminal-history' import { getHistoryRoot, @@ -123,10 +123,7 @@ export function scheduleWorktreeHistoryTreeDeletion(dir: string, historyRoot: st const meta = readHistoryMeta(dir) const wslDistro = wslDistroForHistoryRoot(historyRoot) if (meta?.fishSession && !wslDistro) { - deleteFishHistoryFile(meta.fishSession, { - recordedPath: meta.fishHistoryPath, - recordedPaths: meta.fishHistoryPaths - }) + deleteFishHistoryFile(meta.fishSession, join(dir, FISH_HISTORY_LOCATION_ATTESTATION)) } const tombstone = tombstoneHistoryTree(dir, historyRoot) if (!tombstone) { diff --git a/src/main/terminal-history.test.ts b/src/main/terminal-history.test.ts index f73774cdeba..83b87945dd3 100644 --- a/src/main/terminal-history.test.ts +++ b/src/main/terminal-history.test.ts @@ -7,6 +7,7 @@ const { mkdirSyncMock, writeFileSyncMock, readFileSyncMock, + lstatSyncMock, rmSyncMock, renameSyncMock, readdirSyncMock, @@ -19,6 +20,7 @@ const { mkdirSyncMock: vi.fn(), writeFileSyncMock: vi.fn(), readFileSyncMock: vi.fn(), + lstatSyncMock: vi.fn(), rmSyncMock: vi.fn(), renameSyncMock: vi.fn(), readdirSyncMock: vi.fn(), @@ -33,6 +35,7 @@ vi.mock('fs', () => ({ mkdirSync: mkdirSyncMock, writeFileSync: writeFileSyncMock, readFileSync: readFileSyncMock, + lstatSync: lstatSyncMock, rmSync: rmSyncMock, renameSync: renameSyncMock, readdirSync: readdirSyncMock, @@ -101,6 +104,15 @@ describe('terminal-history', () => { getPathMock.mockReturnValue('/fake/userData') existsSyncMock.mockReturnValue(true) statSyncMock.mockReturnValue({ isDirectory: () => true, size: 100 }) + lstatSyncMock.mockReturnValue({ + dev: 1n, + ino: 2n, + birthtimeNs: 3n, + size: 100, + isDirectory: () => true, + isFile: () => true, + isSymbolicLink: () => false + }) }) describe('resolveShellKind', () => { @@ -365,7 +377,7 @@ describe('terminal-history', () => { injectHistoryEnv(env, 'repo-1::/path/wt', '/usr/bin/fish', '/path/wt') - expect(readFileSyncMock).not.toHaveBeenCalled() + expect(readFileSyncMock).toHaveBeenCalledTimes(1) const meta = JSON.parse(writeFileSyncMock.mock.calls.at(-1)?.[1] as string) as FishHistoryMeta expect(meta.fishHistoryPaths).toEqual([meta.fishHistoryPath]) }) @@ -490,59 +502,30 @@ describe('terminal-history', () => { await flushPendingWorktreeHistoryDeletions() }) - it('removes the fish session file recorded in meta.json, which lives outside the tree', async () => { - // Pinned: fish resolves its data dir from the ambient XDG_DATA_HOME/HOME. - const originalDataHome = process.env.XDG_DATA_HOME - process.env.XDG_DATA_HOME = ['', 'pinned', 'data'].join(sep) - try { - existsSyncMock.mockReturnValue(true) - readFileSyncMock.mockReturnValue(JSON.stringify({ worktreeId, fishSession: session })) - deleteWorktreeHistoryDir('repo-1::/path/wt') - expect(rmSyncMock).toHaveBeenCalledWith( - ['', 'pinned', 'data', 'fish', historyFilename].join(sep), - expect.objectContaining({ force: true }) - ) - } finally { - if (originalDataHome === undefined) { - delete process.env.XDG_DATA_HOME - } else { - process.env.XDG_DATA_HOME = originalDataHome - } - } + it('does not infer deletion authority from the ambient fish data path', async () => { + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValue(JSON.stringify({ worktreeId, fishSession: session })) + + deleteWorktreeHistoryDir('repo-1::/path/wt') + + expect(rmSyncMock).not.toHaveBeenCalled() await flushPendingWorktreeHistoryDeletions() }) - 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) + it('does not trust a spawn-env path copied into meta.json', async () => { const recorded = ['', 'spawn', 'data', 'fish', historyFilename].join(sep) - try { - existsSyncMock.mockReturnValue(true) - readFileSyncMock.mockReturnValue( - JSON.stringify({ - worktreeId, - fishSession: session, - fishHistoryPath: recorded - }) - ) - deleteWorktreeHistoryDir('repo-1::/path/wt') - 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', historyFilename].join(sep), - expect.objectContaining({ force: true }) - ) - } finally { - if (originalDataHome === undefined) { - delete process.env.XDG_DATA_HOME - } else { - process.env.XDG_DATA_HOME = originalDataHome - } - } + existsSyncMock.mockReturnValue(true) + readFileSyncMock.mockReturnValue( + JSON.stringify({ worktreeId, fishSession: session, fishHistoryPath: recorded }) + ) + + deleteWorktreeHistoryDir('repo-1::/path/wt') + + expect(rmSyncMock).not.toHaveBeenCalled() await flushPendingWorktreeHistoryDeletions() }) - it('deletes every recorded fish path while isolating individual failures', async () => { + it('refuses every raw fish path in tampered metadata', async () => { const first = ['', 'data', 'one', 'fish', historyFilename].join(sep) const second = ['', 'data', 'two', 'fish', historyFilename].join(sep) readFileSyncMock.mockReturnValue( @@ -553,15 +536,9 @@ describe('terminal-history', () => { fishHistoryPaths: [first, second] }) ) - rmSyncMock.mockImplementation((path: string) => { - if (path === first) { - throw new Error('busy') - } - }) - expect(() => deleteWorktreeHistoryDir('repo-1::/path/wt')).not.toThrow() - expect(rmSyncMock).toHaveBeenCalledWith(first, expect.objectContaining({ force: true })) - expect(rmSyncMock).toHaveBeenCalledWith(second, expect.objectContaining({ force: true })) + expect(rmSyncMock).not.toHaveBeenCalledWith(first, expect.anything()) + expect(rmSyncMock).not.toHaveBeenCalledWith(second, expect.anything()) expect(renameSyncMock).toHaveBeenCalled() await flushPendingWorktreeHistoryDeletions() }) @@ -582,10 +559,7 @@ describe('terminal-history', () => { 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.at(-1), expect.anything()) expect(rmSyncMock).not.toHaveBeenCalledWith(paths[0], expect.anything()) await flushPendingWorktreeHistoryDeletions() }) @@ -637,7 +611,7 @@ describe('terminal-history', () => { // The history dir exists (so the tree is deleted) but the fish file does not. existsSyncMock.mockImplementation((path: string) => !path.includes(historyFilename)) deleteWorktreeHistoryDir('repo-1::/path/wt') - expect(warn).toHaveBeenCalledWith(expect.stringContaining('No fish history file found')) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('No attested fish history')) } finally { warn.mockRestore() } diff --git a/src/main/terminal-history.ts b/src/main/terminal-history.ts index 3379517824c..723aaa7ae80 100644 --- a/src/main/terminal-history.ts +++ b/src/main/terminal-history.ts @@ -1,6 +1,8 @@ import { join, basename } from 'node:path' import { mkdirSync, existsSync, readFileSync, statSync, writeFileSync } from 'node:fs' import { + attestFishHistoryLocation, + FISH_HISTORY_LOCATION_ATTESTATION, fishHistorySessionName, isSafeFishHistorySession, MAX_FISH_HISTORY_META_BYTES, @@ -103,6 +105,13 @@ function writeMetaFile( const fishHistoryPaths = fish ? normalizeFishHistoryPaths(fish.session, undefined, [...existingFishPaths, fish.historyPath]) : existingFishPaths + if (fish) { + attestFishHistoryLocation( + join(dir, FISH_HISTORY_LOCATION_ATTESTATION), + fish.session, + fish.historyPath + ) + } if ( existing && !existing.fishHistoryMetadataNeedsRewrite && @@ -136,9 +145,9 @@ export type HistoryDirMeta = { createdAt?: string /** fish session name whose history file lives in the user's fish data dir. */ fishSession?: string - /** That file's path resolved from the PTY's spawn env, which the main process env can contradict. */ + /** Legacy/rollback path hint; deletion requires the separate identity attestation. */ fishHistoryPath?: string - /** Most-recent paths this session used as XDG_DATA_HOME changed across launches. */ + /** Bounded legacy/rollback path hints across XDG_DATA_HOME changes. */ fishHistoryPaths?: string[] /** In-memory migration signal; never persisted. */ fishHistoryMetadataNeedsRewrite?: true diff --git a/src/relay/fish-history-metadata.test.ts b/src/relay/fish-history-metadata.test.ts index 0781de8b7d7..752a4e81d4c 100644 --- a/src/relay/fish-history-metadata.test.ts +++ b/src/relay/fish-history-metadata.test.ts @@ -1,4 +1,11 @@ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + realpathSync, + writeFileSync +} from 'node:fs' import { rm } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -18,7 +25,7 @@ describe('relay Fish history metadata', () => { const session = fishHistorySessionName(hash) beforeEach(() => { - root = mkdtempSync(join(tmpdir(), 'orca-relay-fish-meta-')) + root = realpathSync(mkdtempSync(join(tmpdir(), 'orca-relay-fish-meta-'))) dataRoot = join(root, 'data') }) @@ -38,13 +45,13 @@ describe('relay Fish history metadata', () => { recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: join(dataRoot, '39') }, metadataRoot) const meta = JSON.parse(readFileSync(join(metadataRoot, `${hash}.json`), 'utf8')) as { - fishHistoryPath: string - fishHistoryPaths: string[] + locations: { path: 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) + expect(meta.locations).toHaveLength(MAX_RETAINED_FISH_HISTORY_PATHS) + expect(new Set(meta.locations.map((location) => location.path)).size).toBe( + MAX_RETAINED_FISH_HISTORY_PATHS + ) + expect(meta.locations.at(-1)?.path).toBe(join(dataRoot, '39', 'fish', `${session}_history`)) }) it('replaces malformed metadata without retaining an unrelated path', () => { @@ -59,9 +66,11 @@ describe('relay Fish history metadata', () => { recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: dataRoot }, metadataRoot) const meta = JSON.parse(readFileSync(join(metadataRoot, `${hash}.json`), 'utf8')) as { - fishHistoryPaths: string[] + locations: { path: string }[] } - expect(meta.fishHistoryPaths).toEqual([join(dataRoot, 'fish', `${session}_history`)]) + expect(meta.locations.map((location) => location.path)).toEqual([ + join(dataRoot, 'fish', `${session}_history`) + ]) }) it('deletes recorded XDG paths and its bounded metadata', () => { @@ -71,7 +80,8 @@ describe('relay Fish history metadata', () => { writeFileSync(historyFile, 'secret') recordRelayFishHistoryPath(worktreeId, { XDG_DATA_HOME: dataRoot }, metadataRoot) - deleteRelayFishHistory(worktreeId, metadataRoot, { XDG_DATA_HOME: dataRoot }) + deleteRelayFishHistory(worktreeId, metadataRoot) + deleteRelayFishHistory(worktreeId, metadataRoot) 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 index 80423b41eb4..5fa09da0f8b 100644 --- a/src/relay/fish-history-metadata.ts +++ b/src/relay/fish-history-metadata.ts @@ -1,11 +1,10 @@ -import { mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { rmSync } from 'node:fs' import { homedir } from 'node:os' import { join } from 'node:path' import { + attestFishHistoryLocation, deleteFishHistoryFile, fishHistorySessionName, - MAX_FISH_HISTORY_META_BYTES, - normalizeFishHistoryPaths, resolveFishHistoryFilePath } from '../main/fish-history-session' import { hashWorktreeId } from '../main/terminal-history-id' @@ -18,31 +17,6 @@ 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, @@ -53,35 +27,12 @@ export function recordRelayFishHistoryPath( 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)}` - ) - } + attestFishHistoryLocation(metadataPath(worktreeId, root), session, activePath) } -export function deleteRelayFishHistory( - worktreeId: string, - root = metadataRoot(), - env: NodeJS.ProcessEnv = process.env -): void { +export function deleteRelayFishHistory(worktreeId: string, root = metadataRoot()): void { const session = fishHistorySessionName(hashWorktreeId(worktreeId)) - deleteFishHistoryFile(session, { recordedPaths: readPaths(worktreeId, root), env }) + deleteFishHistoryFile(session, metadataPath(worktreeId, root)) try { rmSync(metadataPath(worktreeId, root), { force: true }) } catch (error) { diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 258b699df2b..15c4884f1b1 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -17,14 +17,6 @@ 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 } @@ -70,7 +62,6 @@ describe('GitHandler', () => { dispatcher = createMockDispatcher() const ctx = new RelayContext() handler = new GitHandler(dispatcher as unknown as RelayDispatcher, ctx) - deleteRelayFishHistoryMock.mockReset() }) afterEach(async () => { @@ -164,28 +155,6 @@ 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 b5f35a83513..3e8a20ea992 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -96,7 +96,6 @@ 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 @@ -1462,15 +1461,10 @@ export class GitHandler { } private async removeWorktree(params: Record) { - const remove = async () => { - const result = await this.runWithGitReadCacheClear(() => + const remove = () => + 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)