From b45a595570fcc8df3d0f61be3f35c6887679c22a Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:51:57 -0700 Subject: [PATCH 1/3] fix(codex): harden WSL hook path trust lifecycle (#8191) * Fix WSL Codex hook path trust lifecycle * fix(codex): keep WSL hook trust across transient path rechecks Failed async wslpath/readlink revalidation no longer drops the last known-good cache or wipes managed trust. That path turned cold/wedged distro timeouts into silent untrusted hooks, and forced the next launch onto the logical /mnt guess under custom automount roots. Settlement now reinstalls only when the generation is current and the canonical trust path actually changed. * fix(codex): revoke trust for missing WSL hook paths --- .../codex/codex-wsl-hook-install-plan.test.ts | 125 +++++++++++++- src/main/codex/codex-wsl-hook-install-plan.ts | 120 +++++++++++--- src/main/codex/config-toml-trust.test.ts | 28 ++++ src/main/codex/config-toml-trust.ts | 6 +- .../codex/hook-service-wsl-runtime.test.ts | 88 ++++++++++ src/main/codex/hook-service.ts | 153 +++++++++++++++++- 6 files changed, 487 insertions(+), 33 deletions(-) diff --git a/src/main/codex/codex-wsl-hook-install-plan.test.ts b/src/main/codex/codex-wsl-hook-install-plan.test.ts index f16a6701126..1c485abf934 100644 --- a/src/main/codex/codex-wsl-hook-install-plan.test.ts +++ b/src/main/codex/codex-wsl-hook-install-plan.test.ts @@ -38,10 +38,19 @@ describe('canonicalizeWslLinuxPath', () => { expect(execFileMock).toHaveBeenCalledTimes(1) const [file, args] = execFileMock.mock.calls[0] expect(file).toBe('wsl.exe') - expect(args).toEqual(['-d', 'Ubuntu', '--', 'readlink', '-f', '--', '/home/alias']) + expect(args).toEqual([ + '-d', + 'Ubuntu', + '--', + 'sh', + '-c', + `if [ ! -d "$1" ]; then printf '%s\\n' '__ORCA_WSL_PATH_MISSING__'; exit 0; fi; readlink -f -- "$1"`, + 'sh', + '/home/alias' + ]) }) - it('caches the resolved canonical path and stops spawning wsl.exe', () => { + it('returns the cached path while revalidating it asynchronously', () => { setPlatform('win32') expect(_internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias')).toBeNull() @@ -49,7 +58,42 @@ describe('canonicalizeWslLinuxPath', () => { callback(null, '/home/alice\n') expect(_internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias')).toBe('/home/alice') - expect(execFileMock).toHaveBeenCalledTimes(1) + expect(execFileMock).toHaveBeenCalledTimes(2) + }) + + it('resolves a custom automount root and notifies the first launch', () => { + setPlatform('win32') + const settled = vi.fn() + const windowsPath = 'D:\\orca\\codex-runtime-home\\home' + + expect( + _internals.canonicalizeWslLinuxPath( + 'Ubuntu', + '/mnt/d/orca/codex-runtime-home/home', + windowsPath, + settled + ) + ).toBeNull() + + const [file, args, options, callback] = execFileMock.mock.calls[0] + expect(file).toBe('wsl.exe') + expect(args).toEqual([ + '-d', + 'Ubuntu', + '--', + 'sh', + '-c', + `resolved=$(wslpath -a -u "$1") || exit; if [ ! -d "$resolved" ]; then printf '%s\\n' '__ORCA_WSL_PATH_MISSING__'; exit 0; fi; readlink -f -- "$resolved"`, + 'sh', + windowsPath + ]) + expect(options).toMatchObject({ timeout: 5000, windowsHide: true }) + + callback(null, '/windows/d/orca/codex-runtime-home/home\n') + expect(settled).toHaveBeenCalledWith({ + status: 'resolved', + canonicalPath: '/windows/d/orca/codex-runtime-home/home' + }) }) it('does not spawn a second subprocess while one is in flight', () => { @@ -70,6 +114,81 @@ describe('canonicalizeWslLinuxPath', () => { expect(execFileMock).toHaveBeenCalledTimes(2) }) + it('keeps the last known-good cache when revalidation later fails', () => { + setPlatform('win32') + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias') + const firstCallback = execFileMock.mock.calls[0][3] as ( + error: Error | null, + stdout: string + ) => void + firstCallback(null, '/home/alice\n') + + const settled = vi.fn() + expect( + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias', '/home/alias', settled) + ).toBe('/home/alice') + const secondCallback = execFileMock.mock.calls[1][3] as ( + error: Error | null, + stdout: string + ) => void + secondCallback(new Error('path disappeared'), '') + + // Why: a cold or wedged distro times out without proving the path changed. + // Dropping the cache would force the next launch onto /mnt/... and rewrite + // trust under a path Codex will not use when automount is customized. + expect(settled).toHaveBeenCalledWith({ status: 'unavailable' }) + expect(_internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias')).toBe('/home/alice') + }) + + it('drops the cached identity when WSL confirms the path is missing', () => { + setPlatform('win32') + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias') + const firstCallback = execFileMock.mock.calls[0][3] as ( + error: Error | null, + stdout: string + ) => void + firstCallback(null, '/home/alice\n') + + const settled = vi.fn() + expect( + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias', '/home/alias', settled) + ).toBe('/home/alice') + const secondCallback = execFileMock.mock.calls[1][3] as ( + error: Error | null, + stdout: string + ) => void + secondCallback(null, '__ORCA_WSL_PATH_MISSING__\n') + + expect(settled).toHaveBeenCalledWith({ status: 'missing' }) + expect(_internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias')).toBeNull() + }) + + it('replaces a cached path when its canonical identity changes', () => { + setPlatform('win32') + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias') + const firstCallback = execFileMock.mock.calls[0][3] as ( + error: Error | null, + stdout: string + ) => void + firstCallback(null, '/home/alice-old\n') + + const settled = vi.fn() + expect( + _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias', '/home/alias', settled) + ).toBe('/home/alice-old') + const secondCallback = execFileMock.mock.calls[1][3] as ( + error: Error | null, + stdout: string + ) => void + secondCallback(null, '/home/alice-new\n') + + expect(settled).toHaveBeenCalledWith({ + status: 'resolved', + canonicalPath: '/home/alice-new' + }) + expect(_internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias')).toBe('/home/alice-new') + }) + it('ignores non-absolute readlink output', () => { setPlatform('win32') _internals.canonicalizeWslLinuxPath('Ubuntu', '/home/alias') diff --git a/src/main/codex/codex-wsl-hook-install-plan.ts b/src/main/codex/codex-wsl-hook-install-plan.ts index 00774a26ccb..6edc4aed660 100644 --- a/src/main/codex/codex-wsl-hook-install-plan.ts +++ b/src/main/codex/codex-wsl-hook-install-plan.ts @@ -15,7 +15,19 @@ export type CodexWslRuntimeHookInstallPlan = { trustConfigPath: string } -export type CanonicalizeWslLinuxPath = (distro: string, linuxPath: string) => string | null +export type WslCanonicalPathSettlement = + | { status: 'resolved'; canonicalPath: string } + | { status: 'missing' } + | { status: 'unavailable' } + +export type WslCanonicalPathSettled = (settlement: WslCanonicalPathSettlement) => void + +export type CanonicalizeWslLinuxPath = ( + distro: string, + linuxPath: string, + windowsPath?: string, + onSettled?: WslCanonicalPathSettled +) => string | null function trimTrailingSlash(value: string): string { return value.length > 1 ? value.replace(/\/+$/, '') : value @@ -30,61 +42,118 @@ function toDefaultWslLinuxPath(windowsPath: string): string { } const WSL_CANONICALIZE_TIMEOUT_MS = 5000 +const WSL_PATH_MISSING_OUTPUT = '__ORCA_WSL_PATH_MISSING__' // Why: `readlink -f` over wsl.exe stalls up to the timeout on a cold or wedged // distro. Running it synchronously on the Electron main process froze the UI on -// every Codex WSL launch, so resolve it off-thread and cache the stable result. +// every Codex WSL launch, so resolve it off-thread and cache the latest result. const canonicalWslPathCache = new Map() -const inFlightWslCanonicalizations = new Set() +const inFlightWslCanonicalizations = new Map>() function wslCanonicalizeCacheKey(distro: string, linuxPath: string): string { return `${distro}\x00${linuxPath}` } -function scheduleWslLinuxPathCanonicalization(distro: string, linuxPath: string): void { +function scheduleWslLinuxPathCanonicalization( + distro: string, + linuxPath: string, + windowsPath: string, + onSettled?: WslCanonicalPathSettled +): void { const key = wslCanonicalizeCacheKey(distro, linuxPath) - // Why: cap the wedged-WSL blast radius to a single background subprocess per - // (distro, path); the cache holds the answer once one resolution succeeds. - if (canonicalWslPathCache.has(key) || inFlightWslCanonicalizations.has(key)) { + const listeners = inFlightWslCanonicalizations.get(key) + if (listeners) { + if (onSettled) { + listeners.add(onSettled) + } return } - inFlightWslCanonicalizations.add(key) + const nextListeners = new Set() + if (onSettled) { + nextListeners.add(onSettled) + } + inFlightWslCanonicalizations.set(key, nextListeners) + const drivePath = /^[A-Za-z]:[/\\]/.test(windowsPath) + // Why: wslpath reads each distro's automount root, so a custom root such as + // /windows is discovered without synchronously starting WSL on Electron main. + const args = drivePath + ? [ + '-d', + distro, + '--', + 'sh', + '-c', + `resolved=$(wslpath -a -u "$1") || exit; if [ ! -d "$resolved" ]; then printf '%s\\n' '${WSL_PATH_MISSING_OUTPUT}'; exit 0; fi; readlink -f -- "$resolved"`, + 'sh', + windowsPath + ] + : [ + '-d', + distro, + '--', + 'sh', + '-c', + `if [ ! -d "$1" ]; then printf '%s\\n' '${WSL_PATH_MISSING_OUTPUT}'; exit 0; fi; readlink -f -- "$1"`, + 'sh', + linuxPath + ] execFile( 'wsl.exe', - ['-d', distro, '--', 'readlink', '-f', '--', linuxPath], + args, { encoding: 'utf-8', timeout: WSL_CANONICALIZE_TIMEOUT_MS, windowsHide: true }, (error, stdout) => { - inFlightWslCanonicalizations.delete(key) - if (error) { - return - } const canonicalPath = stdout.trim() - if (canonicalPath.startsWith('/')) { + const resolvedPath = !error && canonicalPath.startsWith('/') ? canonicalPath : null + const pathMissing = !error && canonicalPath === WSL_PATH_MISSING_OUTPUT + const settlement: WslCanonicalPathSettlement = resolvedPath + ? { status: 'resolved', canonicalPath: resolvedPath } + : pathMissing + ? { status: 'missing' } + : { status: 'unavailable' } + if (settlement.status === 'resolved') { canonicalWslPathCache.set(key, canonicalPath) + } else if (settlement.status === 'missing') { + // Why: a successful directory probe is stronger than a transport error; + // clear the identity so stale trust can be revoked and later rediscovered. + canonicalWslPathCache.delete(key) + } + // Why: keep the last known-good cache on timeout/transient WSL failures. + // Dropping it forces the next launch onto the logical `/mnt/...` guess, + // which is wrong under custom automount roots and rewrites trust keys. + const settledListeners = inFlightWslCanonicalizations.get(key) ?? new Set() + inFlightWslCanonicalizations.delete(key) + for (const listener of settledListeners) { + try { + listener(settlement) + } catch (listenerError) { + console.warn('[codex-wsl-hook-path] failed to reconcile canonical path', listenerError) + } } } ) } -function canonicalizeWslLinuxPath(distro: string, linuxPath: string): string | null { +function canonicalizeWslLinuxPath( + distro: string, + linuxPath: string, + windowsPath = linuxPath, + onSettled?: WslCanonicalPathSettled +): string | null { if (process.platform !== 'win32') { return linuxPath } const cached = canonicalWslPathCache.get(wslCanonicalizeCacheKey(distro, linuxPath)) - if (cached) { - return cached - } - // Why: no canonical path resolved yet — start the off-thread resolution and - // let the caller use the logical path for now (identical unless HOME crosses a - // symlink). The next launch reads the cached canonical path. - scheduleWslLinuxPathCanonicalization(distro, linuxPath) - return null + // Why: every launch revalidates asynchronously. Returning the cache keeps + // launch prep synchronous while settlement repairs or revokes trust in-place. + scheduleWslLinuxPathCanonicalization(distro, linuxPath, windowsPath, onSettled) + return cached ?? null } export function createCodexWslRuntimeHookInstallPlan( runtimeHomePath: string | null | undefined, target?: CodexWslRuntimeHookTarget, - canonicalize: CanonicalizeWslLinuxPath = canonicalizeWslLinuxPath + canonicalize: CanonicalizeWslLinuxPath = canonicalizeWslLinuxPath, + onCanonicalPathSettled?: WslCanonicalPathSettled ): CodexWslRuntimeHookInstallPlan | null { if (!runtimeHomePath) { return null @@ -106,7 +175,8 @@ export function createCodexWslRuntimeHookInstallPlan( // Why: Codex canonicalizes hook sources inside WSL; resolving there keeps // trust keys valid when HOME or the runtime directory crosses a symlink. const linuxRuntimeHome = trimTrailingSlash( - canonicalize(distro, logicalLinuxRuntimeHome) ?? logicalLinuxRuntimeHome + canonicalize(distro, logicalLinuxRuntimeHome, runtimeHomePath, onCanonicalPathSettled) ?? + logicalLinuxRuntimeHome ) return { diff --git a/src/main/codex/config-toml-trust.test.ts b/src/main/codex/config-toml-trust.test.ts index bda4ac3499f..4f4dcf4861f 100644 --- a/src/main/codex/config-toml-trust.test.ts +++ b/src/main/codex/config-toml-trust.test.ts @@ -1515,6 +1515,34 @@ describe('readHookTrustEntries', () => { } ) + it.skipIf(process.platform !== 'win32')( + 'keeps WSL hook trust paths case-sensitive on a Windows host', + () => { + const upperKey = '/windows/d/Repo/hooks.json:session_start:0:0' + const lowerKey = '/windows/d/repo/hooks.json:session_start:0:0' + writeFileSync( + configPath, + [ + `[hooks.state."${upperKey}"]`, + 'enabled = true', + 'trusted_hash = "sha256:UPPER"', + '', + `[hooks.state."${lowerKey}"]`, + 'enabled = true', + 'trusted_hash = "sha256:LOWER"', + '' + ].join('\n'), + 'utf-8' + ) + + const result = readHookTrustEntries(configPath) + + expect(result.get(upperKey)?.trustedHash).toBe('sha256:UPPER') + expect(result.get(lowerKey)?.trustedHash).toBe('sha256:LOWER') + expect(result.size).toBe(2) + } + ) + it('reads entries from a CRLF-terminated config', () => { const key = '/x/hooks.json:pre_tool_use:0:0' const original = [ diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index 54331900cd1..d3b827bd26b 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -529,7 +529,11 @@ export function normalizeHookTrustKeyForLookup(key: string): string { const separated = parsed ? `${normalizeWindowsPathSeparators(parsed.sourcePath)}:${parsed.eventLabel}:${parsed.groupIndex}:${parsed.handlerIndex}` : normalizeWindowsPathSeparators(key) - return process.platform === 'win32' ? separated.toLowerCase() : separated + // Why: Windows-native paths are case-insensitive, but WSL and SSH trust + // sources remain case-sensitive even when Orca's host process is Windows. + const caseInsensitiveWindowsPath = + process.platform === 'win32' && usesWindowsPathSeparators(parsed?.sourcePath ?? key) + return caseInsensitiveWindowsPath ? separated.toLowerCase() : separated } function findTrustBlockRanges(content: string, key: string): TrustBlockRange[] { diff --git a/src/main/codex/hook-service-wsl-runtime.test.ts b/src/main/codex/hook-service-wsl-runtime.test.ts index 20accbb5859..232725e5f66 100644 --- a/src/main/codex/hook-service-wsl-runtime.test.ts +++ b/src/main/codex/hook-service-wsl-runtime.test.ts @@ -122,6 +122,94 @@ describe('Codex WSL runtime hook install', () => { expect(plan?.configPath).toBe(pathWin32.join(runtimeHome, 'hooks.json')) }) + it('removes managed trust when the WSL canonical path changes', () => { + const plan = createTestPlan() + writeFileSync(plan.configPath, '{"hooks":{}}\n', 'utf-8') + writeFileSync(plan.tomlPath, '', 'utf-8') + + const oldPlan = { + ...plan, + commandScriptPath: '/old/home/.orca/agent-hooks/codex-hook.sh', + trustConfigPath: '/old/home/hooks.json' + } + expect(_internals.installManagedHooksIntoWslRuntime(oldPlan).state).toBe('installed') + const oldCommand = `if [ -r '${oldPlan.commandScriptPath}' ]; then /bin/sh '${oldPlan.commandScriptPath}'; fi` + const oldKey = computeTrustKey(getManagedTrustEntry(oldPlan, oldCommand)) + + const newPlan = { + ...plan, + commandScriptPath: '/new/home/.orca/agent-hooks/codex-hook.sh', + trustConfigPath: '/new/home/hooks.json' + } + expect(_internals.installManagedHooksIntoWslRuntime(newPlan).state).toBe('installed') + const newCommand = `if [ -r '${newPlan.commandScriptPath}' ]; then /bin/sh '${newPlan.commandScriptPath}'; fi` + const newKey = computeTrustKey(getManagedTrustEntry(newPlan, newCommand)) + const trustEntries = readHookTrustEntries(plan.tomlPath) + + expect(trustEntries.has(oldKey)).toBe(false) + expect(trustEntries.has(newKey)).toBe(true) + }) + + it('sweeps all managed WSL trust for disable or confirmed absence', () => { + // Why: disable and confirmed absence intentionally pass []. Transient + // unavailability must NOT use this path — last known-good trust remains. + const plan = createTestPlan() + writeFileSync(plan.configPath, '{"hooks":{}}\n', 'utf-8') + writeFileSync(plan.tomlPath, '', 'utf-8') + expect(_internals.installManagedHooksIntoWslRuntime(plan).state).toBe('installed') + + _internals.removeStaleWslRuntimeManagedHookTrustEntries(plan.tomlPath, []) + + expect(readHookTrustEntries(plan.tomlPath).size).toBe(0) + }) + + it('reconciles only current, conclusive WSL path settlements', () => { + expect( + _internals.getWslHookReconciliationAction({ + settlement: { status: 'unavailable' }, + isCurrentGeneration: true, + installedTrustConfigPath: '/mnt/d/home/hooks.json', + resolvedTrustConfigPath: null + }) + ).toBe('none') + + expect( + _internals.getWslHookReconciliationAction({ + settlement: { status: 'missing' }, + isCurrentGeneration: false, + installedTrustConfigPath: '/mnt/d/home/hooks.json', + resolvedTrustConfigPath: null + }) + ).toBe('none') + + expect( + _internals.getWslHookReconciliationAction({ + settlement: { status: 'missing' }, + isCurrentGeneration: true, + installedTrustConfigPath: '/mnt/d/home/hooks.json', + resolvedTrustConfigPath: null + }) + ).toBe('remove') + + expect( + _internals.getWslHookReconciliationAction({ + settlement: { status: 'resolved', canonicalPath: '/windows/d/home' }, + isCurrentGeneration: true, + installedTrustConfigPath: '/windows/d/home/hooks.json', + resolvedTrustConfigPath: '/windows/d/home/hooks.json' + }) + ).toBe('none') + + expect( + _internals.getWslHookReconciliationAction({ + settlement: { status: 'resolved', canonicalPath: '/windows/d/home' }, + isCurrentGeneration: true, + installedTrustConfigPath: '/mnt/d/home/hooks.json', + resolvedTrustConfigPath: '/windows/d/home/hooks.json' + }) + ).toBe('reinstall') + }) + it('generates a POSIX hook that bridges WSL loopback failures through Windows curl', () => { const script = _internals.getManagedScript('posix') expect(script).toContain('load_hook_endpoint()') diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index 66b2b65b077..97142a4a242 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -1,6 +1,6 @@ /* eslint-disable max-lines -- Why: getStatus + install + remove all share the managed-command and trust-key derivation. Splitting would hide that the three operations must agree on group index, event label, and command bytes. */ import { existsSync, readFileSync, unlinkSync } from 'node:fs' -import { join } from 'node:path' +import { join, win32 as pathWin32 } from 'node:path' import type { SFTPWrapper } from 'ssh2' import type { AgentHookInstallState, AgentHookInstallStatus } from '../../shared/agent-hook-types' import { @@ -46,7 +46,8 @@ import { syncSystemConfigIntoManagedCodexHome } from './codex-config-mirror' import { createCodexWslRuntimeHookInstallPlan, type CodexWslRuntimeHookInstallPlan, - type CodexWslRuntimeHookTarget + type CodexWslRuntimeHookTarget, + type WslCanonicalPathSettlement } from './codex-wsl-hook-install-plan' import { CODEX_HOOK_EVENT_LABEL, @@ -723,6 +724,52 @@ function removeWslRuntimeManagedHookTrustEntries(plan: CodexWslRuntimeHookInstal } } +function removeStaleWslRuntimeManagedHookTrustEntries( + tomlPath: string, + desiredEntries: readonly CodexTrustEntry[] +): void { + const desiredKeys = new Set( + desiredEntries.map((entry) => normalizeHookTrustKeyForLookup(computeTrustKey(entry))) + ) + const existingEntries = readHookTrustEntries(tomlPath) + const ourKeys: string[] = [] + for (const [key, state] of existingEntries) { + if (desiredKeys.has(normalizeHookTrustKeyForLookup(key))) { + continue + } + const parts = parseTrustKey(key) + if (!parts || !CODEX_MANAGED_EVENT_LABELS.has(parts.eventLabel)) { + continue + } + const sourcePath = parts.sourcePath + // Why: this cleanup owns only guest-side WSL trust. A runtime config can + // still contain user Windows/remote hooks, which must remain untouched. + if (!sourcePath.startsWith('/') || !sourcePath.endsWith('/hooks.json')) { + continue + } + const runtimeHome = sourcePath.slice(0, -'/hooks.json'.length) + const command = wrapReadablePosixHookCommand(`${runtimeHome}/.orca/agent-hooks/codex-hook.sh`) + const expectedEntry: CodexTrustEntry = { + sourcePath: parts.sourcePath, + eventLabel: parts.eventLabel, + groupIndex: parts.groupIndex, + handlerIndex: parts.handlerIndex, + command, + timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS + } + const recognizedHashes = new Set([ + computeTrustedHash(expectedEntry), + computeTrustedHash({ ...expectedEntry, timeoutSec: undefined }) + ]) + if (state.trustedHash && recognizedHashes.has(state.trustedHash)) { + ourKeys.push(key) + } + } + if (ourKeys.length > 0) { + removeHookTrustEntries(tomlPath, ourKeys) + } +} + function getManagedScript(target: 'local' | 'posix' = 'local'): string { if (target === 'local' && process.platform === 'win32') { return [ @@ -876,6 +923,7 @@ function installManagedHooksIntoWslRuntime( // Why: WSL runtime homes may carry user hook approvals we did not rebuild // here; only upsert Orca's entries instead of sweeping the whole source. upsertHookTrustEntries(plan.tomlPath, trustEntries) + removeStaleWslRuntimeManagedHookTrustEntries(plan.tomlPath, trustEntries) } catch (error) { return { agent: 'codex', @@ -922,6 +970,13 @@ function refreshWslRuntimeUserHooks(plan: CodexWslRuntimeHookInstallPlan): Agent } writeCodexHooksJson(plan.configPath, nextHooks) removeWslRuntimeManagedHookTrustEntries(plan) + try { + // Why: the disabled path may be reached after the WSL mount root changed, + // so cleanup cannot be scoped only to the plan's current source path. + removeStaleWslRuntimeManagedHookTrustEntries(plan.tomlPath, []) + } catch (error) { + console.warn('[codex-hook-service] failed to clean stale WSL trust entries', error) + } return { agent: 'codex', state: 'not_installed', @@ -931,12 +986,99 @@ function refreshWslRuntimeUserHooks(plan: CodexWslRuntimeHookInstallPlan): Agent } } +// Why: transport failures preserve the last known-good identity, while a +// successful absence probe is strong enough to revoke trust immediately. +function getWslHookReconciliationAction(args: { + settlement: WslCanonicalPathSettlement + isCurrentGeneration: boolean + installedTrustConfigPath: string | null + resolvedTrustConfigPath: string | null +}): 'none' | 'remove' | 'reinstall' { + if (!args.isCurrentGeneration) { + return 'none' + } + if (args.settlement.status === 'missing') { + return 'remove' + } + if ( + args.settlement.status !== 'resolved' || + !args.resolvedTrustConfigPath || + args.resolvedTrustConfigPath === args.installedTrustConfigPath + ) { + return 'none' + } + return 'reinstall' +} + export class CodexHookService { + private readonly wslReconciliationGeneration = new Map() + + private supersedeWslReconciliation(runtimeHomePath: string | null | undefined): number { + if (!runtimeHomePath) { + return 0 + } + const key = process.platform === 'win32' ? runtimeHomePath.toLowerCase() : runtimeHomePath + const generation = (this.wslReconciliationGeneration.get(key) ?? 0) + 1 + this.wslReconciliationGeneration.set(key, generation) + return generation + } + installForRuntimeHome( runtimeHomePath: string | null | undefined, target?: CodexWslRuntimeHookTarget ): AgentHookInstallStatus | null { - const wslPlan = createCodexWslRuntimeHookInstallPlan(runtimeHomePath, target) + const generation = this.supersedeWslReconciliation(runtimeHomePath) + let installedTrustConfigPath: string | null = null + const onCanonicalPathSettled = (settlement: WslCanonicalPathSettlement): void => { + if (!runtimeHomePath) { + return + } + const key = process.platform === 'win32' ? runtimeHomePath.toLowerCase() : runtimeHomePath + const resolvedPlan = + settlement.status === 'resolved' + ? createCodexWslRuntimeHookInstallPlan( + runtimeHomePath, + target, + () => settlement.canonicalPath + ) + : null + const action = getWslHookReconciliationAction({ + settlement, + isCurrentGeneration: this.wslReconciliationGeneration.get(key) === generation, + installedTrustConfigPath, + resolvedTrustConfigPath: resolvedPlan?.trustConfigPath ?? null + }) + if (action === 'none') { + return + } + if (action === 'remove') { + try { + removeStaleWslRuntimeManagedHookTrustEntries( + pathWin32.join(runtimeHomePath, 'config.toml'), + [] + ) + } catch (error) { + console.warn('[codex-hook-service] failed to revoke stale WSL hook trust', error) + } + return + } + if (!resolvedPlan) { + return + } + const status = installManagedHooksIntoWslRuntime(resolvedPlan) + if (status.state === 'error') { + console.warn('[codex-hook-service] failed to reconcile WSL hook path', status.detail) + return + } + installedTrustConfigPath = resolvedPlan.trustConfigPath + } + const wslPlan = createCodexWslRuntimeHookInstallPlan( + runtimeHomePath, + target, + undefined, + onCanonicalPathSettled + ) + installedTrustConfigPath = wslPlan?.trustConfigPath ?? null return wslPlan ? installManagedHooksIntoWslRuntime(wslPlan) : null } @@ -944,6 +1086,7 @@ export class CodexHookService { runtimeHomePath: string | null | undefined, target?: CodexWslRuntimeHookTarget ): AgentHookInstallStatus | null { + this.supersedeWslReconciliation(runtimeHomePath) const wslPlan = createCodexWslRuntimeHookInstallPlan(runtimeHomePath, target) return wslPlan ? refreshWslRuntimeUserHooks(wslPlan) : null } @@ -1413,5 +1556,7 @@ export const codexHookService = new CodexHookService() export const _internals = { getManagedScript, installManagedHooksIntoWslRuntime, - refreshWslRuntimeUserHooks + refreshWslRuntimeUserHooks, + removeStaleWslRuntimeManagedHookTrustEntries, + getWslHookReconciliationAction } From 977960ed4a7bf08dbca88f8e03ea2922195813b9 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Fri, 10 Jul 2026 17:52:50 -0700 Subject: [PATCH 2/3] Make Windows workspace deletion reliable and respect Git locks (#7963) * Handle locked worktree force deletion Co-authored-by: Orca * Harden locked worktree removal * Recover Windows worktree deletion after Git deregisters * Require force permission for lock overrides * Respect Git worktree locks during deletion * Require structural proof before Windows deletion recovery * Align deletion tests with fail-closed recovery * Restore deletion test filesystem spies * Scope remote deletion tests to runtime host --------- Co-authored-by: Orca --- src/main/git/remove-worktree.test.ts | 44 ++- src/main/git/worktree-list-paths.test.ts | 46 ++++ src/main/git/worktree.test.ts | 41 +++ src/main/git/worktree.ts | 16 +- src/main/ipc/worktrees.test.ts | 188 +++++++++++-- src/main/ipc/worktrees.ts | 63 ++++- ...cal-worktree-removal-recovery-live.test.ts | 128 +++++++++ .../local-worktree-removal-recovery.test.ts | 257 +++++++++++++++++- src/main/local-worktree-removal-recovery.ts | 147 ++++++---- src/main/runtime/orca-runtime.test.ts | 140 +++++++++- src/main/runtime/orca-runtime.ts | 59 +++- src/main/runtime/rpc/methods/worktree.test.ts | 19 ++ src/relay/git-handler-utils.test.ts | 45 ++- src/relay/git-handler-utils.ts | 9 + src/relay/git-handler-worktree-ops.test.ts | 57 ++++ src/relay/git-handler-worktree-remove.ts | 9 +- .../src/components/settings/DevToolsPane.tsx | 1 + .../sidebar/DeleteWorktreeDialog.test.tsx | 41 ++- .../sidebar/DeleteWorktreeDialog.tsx | 20 +- .../delete-worktree-dirty-change-counts.ts | 32 +++ .../delete-worktree-failure-toast.test.tsx | 43 +++ .../sidebar/delete-worktree-failure-toast.tsx | 28 +- .../sidebar/delete-worktree-flow.test.ts | 41 ++- .../sidebar/delete-worktree-flow.ts | 10 +- .../sidebar/delete-worktree-toast.test.ts | 33 ++- .../sidebar/delete-worktree-toast.ts | 38 ++- .../status-bar/WorkspaceSpaceManagerPanel.tsx | 2 + src/renderer/src/i18n/locales/en.json | 4 +- src/renderer/src/i18n/locales/es.json | 4 +- src/renderer/src/i18n/locales/ja.json | 4 +- src/renderer/src/i18n/locales/ko.json | 4 +- src/renderer/src/i18n/locales/zh.json | 4 +- .../src/store/slices/store-cascades.test.ts | 82 +++++- .../src/store/slices/worktree-helpers.ts | 8 +- src/renderer/src/store/slices/worktrees.ts | 50 ++-- src/renderer/src/web/web-preload-api.test.ts | 54 ++++ src/renderer/src/web/web-preload-api.ts | 5 +- src/shared/git-cquoted-path.test.ts | 12 + src/shared/git-cquoted-path.ts | 31 ++- src/shared/types.ts | 2 + src/shared/worktree-removal.ts | 75 +++++ 41 files changed, 1681 insertions(+), 215 deletions(-) create mode 100644 src/main/local-worktree-removal-recovery-live.test.ts create mode 100644 src/renderer/src/components/sidebar/delete-worktree-dirty-change-counts.ts create mode 100644 src/shared/git-cquoted-path.test.ts create mode 100644 src/shared/worktree-removal.ts diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index f2f356a8734..53c424e50f6 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -247,7 +247,7 @@ branch refs/heads/main ]) }) - it('passes --force before the worktree path when forced removal is requested', async () => { + it('passes one --force before the worktree path for dirty-file removal', async () => { mockGitCommands({ 'git worktree list --porcelain': { stdout: `worktree /repo @@ -272,6 +272,48 @@ branch refs/heads/main expect(getGitCalls()).toContain('git worktree remove --force /repo-feature') }) + it('rejects a locked worktree with stable app-owned copy before invoking remove', async () => { + mockGitCommands({ + 'git worktree list --porcelain': { + stdout: `worktree /repo +HEAD abc123 +branch refs/heads/main + +worktree /repo-feature +HEAD def456 +branch refs/heads/feature/test +locked active agent session +` + } + }) + + await expect(removeWorktree('/repo', '/repo-feature', true)).rejects.toThrow( + 'Worktree is locked by Git. Lock reason: active agent session.' + ) + expect(getGitCalls()).not.toContain('git worktree remove /repo-feature') + }) + + it('does not treat dirty-file force as permission to override a lock', async () => { + mockGitCommands({ + 'git worktree list --porcelain': { + stdout: `worktree /repo +HEAD abc123 +branch refs/heads/main + +worktree /repo-feature +HEAD def456 +branch refs/heads/feature/test +locked active agent session +` + } + }) + + await expect(removeWorktree('/repo', '/repo-feature', true)).rejects.toThrow( + 'Worktree is locked by Git. Lock reason: active agent session.' + ) + expect(getGitCalls()).not.toContain('git worktree remove --force /repo-feature') + }) + it('matches Windows worktree paths before deleting the branch', async () => { mockGitCommands({ 'git worktree list --porcelain': { diff --git a/src/main/git/worktree-list-paths.test.ts b/src/main/git/worktree-list-paths.test.ts index dc792712077..df83417e7cb 100644 --- a/src/main/git/worktree-list-paths.test.ts +++ b/src/main/git/worktree-list-paths.test.ts @@ -33,6 +33,39 @@ async function createRepoWithNewlineWorktree(): Promise<{ } } +async function createRepoWithLockedDeletedWorktree(): Promise<{ + repoPath: string + worktreePath: string +}> { + const root = await mkdtemp(path.join(tmpdir(), 'orca-worktree-locked-delete-')) + tempRoots.push(root) + const repoPath = path.join(root, 'repo') + const requestedWorktreePath = path.join(root, 'locked deleted worktree') + + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + git(repoPath, ['config', 'user.email', 'test@example.com']) + git(repoPath, ['config', 'user.name', 'Test User']) + git(repoPath, ['commit', '--allow-empty', '--quiet', '-m', 'initial']) + git(repoPath, [ + 'worktree', + 'add', + '--quiet', + '-b', + 'feature/locked-delete', + requestedWorktreePath + ]) + git(repoPath, ['worktree', 'lock', '--reason', 'orca locked stale repro', requestedWorktreePath]) + + const worktreePath = await realpath(requestedWorktreePath) + await rm(worktreePath, { recursive: true, force: true }) + + return { + repoPath: await realpath(repoPath), + worktreePath + } +} + function branchExists(repoPath: string, branchName: string): boolean { try { git(repoPath, ['show-ref', '--verify', '--quiet', `refs/heads/${branchName}`]) @@ -68,4 +101,17 @@ describe('git worktree paths', () => { expect(branchExists(repoPath, 'feature/newline')).toBe(false) } ) + + it('preserves a locked worktree whose directory was deleted manually', async () => { + const { repoPath, worktreePath } = await createRepoWithLockedDeletedWorktree() + + await expect(removeWorktree(repoPath, worktreePath, true)).rejects.toThrow( + 'Worktree is locked by Git' + ) + + expect(git(repoPath, ['worktree', 'list', '--porcelain'])).toContain( + worktreePath.replaceAll('\\', '/') + ) + expect(branchExists(repoPath, 'feature/locked-delete')).toBe(true) + }) }) diff --git a/src/main/git/worktree.test.ts b/src/main/git/worktree.test.ts index 43269a065a1..6388ccdbc37 100644 --- a/src/main/git/worktree.test.ts +++ b/src/main/git/worktree.test.ts @@ -98,6 +98,47 @@ describe('listWorktrees in-flight sharing', () => { }) describe('parseWorktreeList', () => { + it('preserves a locked marker and its reason', () => { + expect( + parseWorktreeList( + 'worktree /repo\nHEAD abc\nbranch refs/heads/main\n\nworktree /locked\nHEAD def\nbranch refs/heads/feature\nlocked active agent session\n' + )[1] + ).toMatchObject({ + path: '/locked', + locked: true, + lockReason: 'active agent session' + }) + }) + + it('decodes C-quoted lock reasons from legacy line porcelain output', () => { + const output = + 'worktree /repo\nHEAD abc\nbranch refs/heads/main\n\nworktree /locked\nHEAD def\nbranch refs/heads/feature\nlocked "first line\\nsecond line \\303\\251"\n' + + expect(parseWorktreeList(output)[1]).toMatchObject({ + locked: true, + lockReason: 'first line\nsecond line é' + }) + }) + + it('keeps NUL-delimited lock reasons raw', () => { + const output = [ + 'worktree /repo', + 'HEAD abc', + 'branch refs/heads/main', + '', + 'worktree /locked', + 'HEAD def', + 'branch refs/heads/feature', + 'locked "literal\\nquote"', + '' + ].join('\0') + + expect(parseWorktreeList(output, { nulDelimited: true })[1]).toMatchObject({ + locked: true, + lockReason: '"literal\\nquote"' + }) + }) + it('parses regular and bare worktree blocks from porcelain output', () => { const output = ` worktree /repo diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index eb30e13ad0a..11961404fb1 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -13,6 +13,8 @@ import type { LocalBaseRefUpdateSuggestion, RemoveWorktreeResult } from '../../shared/types' +import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree-removal' +import { decodeGitCQuotedPath } from '../../shared/git-cquoted-path' import { parseGitRevListAheadBehindCounts } from '../../shared/git-rev-list-output' import { parseWslUncPath } from '../../shared/wsl-paths' import { gitExecFileAsync, translateWslOutputPaths } from './runner' @@ -46,7 +48,7 @@ export type AddWorktreeOptions = GitWorktreeExecOptions & { export type RemoveWorktreeOptions = GitWorktreeExecOptions & { deleteBranch?: boolean forceBranchDelete?: boolean - knownRemovedWorktree?: Pick + knownRemovedWorktree?: Pick } type LocalBaseRefRefreshability = @@ -487,6 +489,8 @@ export function parseWorktreeList( let branch = '' let isBare = false let isSparse = false + let locked = false + let lockReason = '' for (const line of lines) { if (line.startsWith('worktree ')) { @@ -499,6 +503,10 @@ export function parseWorktreeList( isBare = true } else if (line === 'sparse') { isSparse = true + } else if (line === 'locked' || line.startsWith('locked ')) { + locked = true + const rawReason = line.slice('locked'.length).trim() + lockReason = options.nulDelimited ? rawReason : decodeGitCQuotedPath(rawReason) } } @@ -510,6 +518,8 @@ export function parseWorktreeList( branch, isBare, ...(isSparse ? { isSparse } : {}), + ...(locked ? { locked: true } : {}), + ...(lockReason ? { lockReason } : {}), isMainWorktree: worktrees.length === 0 }) } @@ -1074,6 +1084,10 @@ async function performRemoveWorktree( const branchName = normalizeLocalBranchRef(removedWorktree?.branch ?? '') const branchHead = removedWorktree?.head ?? '' + // Why: callers outside the IPC/runtime preflight must not bypass Git's lock + // contract or depend on localized stderr to discover it after side effects. + assertWorktreeUnlockedForRemoval(removedWorktree) + const args = ['worktree', 'remove'] if (force) { args.push('--force') diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 509c7243eca..8e128912517 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -5,8 +5,10 @@ import { lstat, mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join, resolve } from 'node:path' import type { CreateWorktreeResult, GitWorktreeInfo, Worktree } from '../../shared/types' +import * as localWorktreeFilesystem from '../local-worktree-filesystem' const ORIGINAL_PLATFORM = process.platform +const removeWorktreeLinkedPathsMock = vi.hoisted(() => vi.fn()) function setPlatform(platform: NodeJS.Platform): void { Object.defineProperty(process, 'platform', { @@ -168,6 +170,11 @@ vi.mock('../providers/ssh-filesystem-dispatch', () => ({ getSshFilesystemProvider: getSshFilesystemProviderMock })) +vi.mock('./worktree-symlinks', () => ({ + createWorktreeLinkedPaths: vi.fn(), + removeWorktreeLinkedPaths: removeWorktreeLinkedPathsMock +})) + vi.mock('./ssh', () => ({ getActiveMultiplexer: getActiveMultiplexerMock })) @@ -336,7 +343,8 @@ describe('registerWorktreeHandlers', () => { clearProviderPtyStateMock, getLocalPtyProviderMock, deleteWorktreeHistoryDirMock, - advertisedUrlWatcherForgetWorktreeMock + advertisedUrlWatcherForgetWorktreeMock, + removeWorktreeLinkedPathsMock ]) { m.mockReset() } @@ -661,8 +669,8 @@ describe('registerWorktreeHandlers', () => { function mockKnownFeatureWorktree( path = '/workspace/feature-wt', repoPath = '/workspace/repo' - ): void { - listWorktreesMock.mockResolvedValue([ + ): GitWorktreeInfo[] { + const worktrees: GitWorktreeInfo[] = [ { path: repoPath, head: 'main', @@ -677,7 +685,9 @@ describe('registerWorktreeHandlers', () => { isBare: false, isMainWorktree: false } - ]) + ] + listWorktreesMock.mockResolvedValue(worktrees) + return worktrees } function makeWorktreeMeta(overrides: Record = {}): Record { @@ -6261,7 +6271,11 @@ describe('registerWorktreeHandlers', () => { const worktreePath = join(parentDir, 'feature-wt') await mkdir(worktreePath, { recursive: true }) await writeFile(join(worktreePath, 'scratch.txt'), 'delete me') - mockKnownFeatureWorktree(worktreePath, repoPath) + const registeredWorktrees = mockKnownFeatureWorktree(worktreePath, repoPath) + listWorktreesMock + .mockResolvedValueOnce(registeredWorktrees) + .mockResolvedValueOnce(registeredWorktrees) + .mockResolvedValue([]) store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) const longPathError = Object.assign(new Error('git worktree remove failed'), { stderr: 'error: failed to delete deep/file.txt: Filename too long' @@ -6295,7 +6309,11 @@ describe('registerWorktreeHandlers', () => { it('does not create a preserved-branch target when long-path recovery preserves branch by policy', async () => { setPlatform('win32') - mockKnownFeatureWorktree() + const registeredWorktrees = mockKnownFeatureWorktree() + listWorktreesMock + .mockResolvedValueOnce(registeredWorktrees) + .mockResolvedValueOnce(registeredWorktrees) + .mockResolvedValue([]) store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta({ preserveBranchOnDelete: true })) removeWorktreeMock.mockRejectedValue( Object.assign(new Error('git worktree remove failed'), { @@ -6335,39 +6353,49 @@ describe('registerWorktreeHandlers', () => { expect(store.removeWorktreeMeta).not.toHaveBeenCalled() }) - it('keeps metadata when Windows long-path recovery deletes the directory but prune fails', async () => { + it('refuses Windows recovery while Git still reports the row and keeps metadata', async () => { setPlatform('win32') mockKnownFeatureWorktree() store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + const removePathSpy = vi + .spyOn(localWorktreeFilesystem, 'removeLocalWorktreePath') + .mockResolvedValue(undefined) removeWorktreeMock.mockRejectedValue( Object.assign(new Error('git worktree remove failed'), { stderr: 'error: failed to delete deep/file.txt: Filename too long' }) ) - gitExecFileAsyncMock.mockRejectedValue( - Object.assign(new Error('git prune failed'), { - stderr: 'fatal: unable to lock worktree admin dir' - }) - ) - await expect( - handlers['worktrees:remove'](null, { - worktreeId: 'repo-1::/workspace/feature-wt', - force: true - }) - ).rejects.toThrow('Git still has stale worktree registration') + try { + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt', + force: true + }) + ).rejects.toThrow( + 'Failed to force delete worktree at /workspace/feature-wt. error: failed to delete deep/file.txt: Filename too long' + ) - expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - expect(mainWindow.webContents.send).not.toHaveBeenCalledWith('worktrees:changed', { - repoId: 'repo-1' - }) + expect(removePathSpy).not.toHaveBeenCalled() + expect(gitExecFileAsyncMock).not.toHaveBeenCalledWith( + ['worktree', 'prune'], + expect.anything() + ) + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + expect(mainWindow.webContents.send).not.toHaveBeenCalledWith('worktrees:changed', { + repoId: 'repo-1' + }) + } finally { + removePathSpy.mockRestore() + } }) it('retries stale Git registration cleanup after prior local filesystem recovery', async () => { setPlatform('win32') const missingWorktreePath = 'C:\\workspace\\already-removed' const worktreeId = `repo-1::${missingWorktreePath}` - mockKnownFeatureWorktree(missingWorktreePath) + const registeredWorktrees = mockKnownFeatureWorktree(missingWorktreePath) + listWorktreesMock.mockResolvedValueOnce(registeredWorktrees).mockResolvedValue([]) store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) const result = await handlers['worktrees:remove'](null, { @@ -6387,6 +6415,40 @@ describe('registerWorktreeHandlers', () => { expect(store.removeWorktreeMeta).toHaveBeenCalledWith(worktreeId) }) + it('preserves a locked missing registration even with force', async () => { + setPlatform('win32') + const missingWorktreePath = 'C:\\workspace\\locked-already-removed' + const worktreeId = `repo-1::${missingWorktreePath}` + const registeredWorktrees: GitWorktreeInfo[] = [ + { + path: '/workspace/repo', + head: 'main', + branch: 'main', + isBare: false, + isMainWorktree: true + }, + { + path: missingWorktreePath, + head: 'feature', + branch: 'feature', + isBare: false, + isMainWorktree: false, + locked: true, + lockReason: 'active agent session' + } + ] + listWorktreesMock.mockResolvedValue(registeredWorktrees) + store.getWorktreeMeta.mockReturnValue(makeWorktreeMeta()) + removeWorktreeMock.mockResolvedValue({}) + + await expect(handlers['worktrees:remove'](null, { worktreeId, force: true })).rejects.toThrow( + 'Worktree is locked by Git. Lock reason: active agent session' + ) + + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + it('refuses to delete the root workspace for folder-mode repos', async () => { store.getRepo.mockReturnValue({ id: 'repo-folder', @@ -7905,6 +7967,86 @@ describe('registerWorktreeHandlers', () => { expect(removeWorktreeMock).not.toHaveBeenCalled() }) + it('fails locked dirty-force deletes before hooks, link cleanup, or PTY teardown', async () => { + listWorktreesMock.mockResolvedValue([ + { + path: '/workspace/repo', + head: 'main', + branch: 'main', + isBare: false, + isMainWorktree: true + }, + { + path: '/workspace/feature-wt', + head: 'feature', + branch: 'feature', + isBare: false, + isMainWorktree: false, + locked: true, + lockReason: 'active agent session' + } + ]) + store.getRepo.mockReturnValue({ + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + symlinkPaths: ['node_modules'] + }) + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'echo archived' } }) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt', + force: true + }) + ).rejects.toThrow( + 'Failed to force delete worktree at /workspace/feature-wt. Worktree is locked by Git.' + ) + + expect(assertWorktreeCleanForRemovalMock).not.toHaveBeenCalled() + expect(runHookMock).not.toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + + it('rechecks a local Git lock after the archive hook before teardown', async () => { + const unlockedWorktrees = mockKnownFeatureWorktree() + const lockedWorktrees = unlockedWorktrees.map((worktree) => + worktree.path === '/workspace/feature-wt' + ? { ...worktree, locked: true, lockReason: 'locked during archive' } + : worktree + ) + listWorktreesMock + .mockResolvedValueOnce(unlockedWorktrees) + .mockResolvedValueOnce(lockedWorktrees) + store.getRepo.mockReturnValue({ + id: 'repo-1', + path: '/workspace/repo', + displayName: 'repo', + badgeColor: '#000', + addedAt: 0, + symlinkPaths: ['node_modules'] + }) + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'echo archived' } }) + runHookMock.mockResolvedValue({ success: true, output: '' }) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt', + force: true + }) + ).rejects.toThrow('Worktree is locked by Git') + + expect(runHookMock).toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(assertWorktreeCleanForRemovalMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + it('formats preflight subprocess failures and does not tear down PTYs', async () => { mockKnownFeatureWorktree() getEffectiveHooksMock.mockReturnValue(null) diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 8f4e64e4207..67468a78b06 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -29,6 +29,7 @@ import type { Worktree, WorktreeMeta } from '../../shared/types' +import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree-removal' import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { buildKnownOrcaWorkspaceLayouts, @@ -160,8 +161,8 @@ import { toLocalWorktreeRuntimePath } from '../local-worktree-filesystem' import { - pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval, - recoverLocalWindowsLongPathWorktreeRemoval + removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval, + recoverLocalWindowsWorktreeRemoval } from '../local-worktree-removal-recovery' const WORKTREE_ARCHIVE_HOOK_TIMEOUT_MS = 120_000 @@ -1583,8 +1584,18 @@ export function registerWorktreeHandlers( const canonicalWorktreePath = registeredWorktree.path const deleteBranch = removedMeta?.preserveBranchOnDelete !== true + // Why: a Git lock must block before archive hooks or linked-path cleanup + // mutate the workspace; dirty-file force is a separate permission. + try { + assertWorktreeUnlockedForRemoval(registeredWorktree) + } catch (error) { + throw new Error( + formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) + ) + } + // Why: a prior forced Windows recovery can delete the directory but leave - // Git's stale registration; retry by pruning instead of removing a missing path. + // Git's stale registration; recover and verify it before clearing metadata. if ( !repo.connectionId && args.force === true && @@ -1594,7 +1605,7 @@ export function registerWorktreeHandlers( removedMeta && (await isAlreadyRemovedWorktreePath(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { - const removalResult = await pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, @@ -1655,9 +1666,10 @@ export function registerWorktreeHandlers( } } - const rawRemovalResult = await (deleteBranch - ? provider!.removeWorktree(canonicalWorktreePath, args.force) - : provider!.removeWorktree(canonicalWorktreePath, args.force, { deleteBranch })) + const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} + const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, args.force)) const removalResult = preserveBranchHeadFallback( rawRemovalResult, registeredWorktree.head @@ -1681,6 +1693,29 @@ export function registerWorktreeHandlers( return removalResult ?? {} } + const refreshedWorktrees = hasLocalWorktreeGitOptions + ? await listGitWorktreesStrict(repo.path, localWorktreeGitOptions) + : await listGitWorktreesStrict(repo.path) + const refreshedRegisteredWorktree = findRegisteredDeletableWorktree( + repo.path, + canonicalWorktreePath, + refreshedWorktrees + ) + if (!refreshedRegisteredWorktree) { + throw new Error( + `Worktree registration changed during deletion: ${canonicalWorktreePath}. Retry deletion.` + ) + } + try { + // Why: an archive hook can race another Git client that locks the row; + // recheck before linked-path, watcher, or terminal teardown side effects. + assertWorktreeUnlockedForRemoval(refreshedRegisteredWorktree) + } catch (error) { + throw new Error( + formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) + ) + } + // Why: `git worktree remove` (non-force) refuses to delete a worktree // that has untracked files. User-configured symlinks look untracked, // so unlink them before the clean check; regular APFS clone-copied @@ -1738,7 +1773,7 @@ export function registerWorktreeHandlers( // Why: this handler already paid for an authoritative worktree // list to validate the target; reuse it instead of rescanning // every sibling worktree during the hot delete path. - knownRemovedWorktree: registeredWorktree, + knownRemovedWorktree: refreshedRegisteredWorktree, ...(hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {}) } removalResult = preserveBranchHeadFallback( @@ -1748,18 +1783,18 @@ export function registerWorktreeHandlers( args.force ?? false, removeOptions ), - registeredWorktree.head + refreshedRegisteredWorktree.head ) } catch (error) { - // Why: Git for Windows can fail long-path directory deletion after - // Orca has already validated the target and explicit force delete. - const recoveredRemovalResult = await recoverLocalWindowsLongPathWorktreeRemoval({ + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ error, force: args.force ?? false, canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, - registeredWorktree, + registeredWorktree: refreshedRegisteredWorktree, deleteBranch, closeWatcher: closeLocalWatcherForRemoval }) @@ -1825,7 +1860,7 @@ export function registerWorktreeHandlers( rememberPreservedBranchCleanupTarget( args.worktreeId, removalResult, - registeredWorktree.head, + refreshedRegisteredWorktree.head, removedPushTarget ) runtime.clearOptimisticReconcileToken(args.worktreeId) diff --git a/src/main/local-worktree-removal-recovery-live.test.ts b/src/main/local-worktree-removal-recovery-live.test.ts new file mode 100644 index 00000000000..4d5a048b5f4 --- /dev/null +++ b/src/main/local-worktree-removal-recovery-live.test.ts @@ -0,0 +1,128 @@ +import { spawn, spawnSync, type ChildProcess } from 'node:child_process' +import { existsSync } from 'node:fs' +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { setTimeout as delay } from 'node:timers/promises' +import { afterEach, describe, expect, it } from 'vitest' +import { recoverLocalWindowsWorktreeRemoval } from './local-worktree-removal-recovery' + +const tempRoots: string[] = [] + +function git(cwd: string, args: string[]): string { + const result = spawnSync('git', args, { cwd, encoding: 'utf8' }) + if (result.status !== 0) { + throw new Error(result.stderr || result.stdout || `git ${args.join(' ')} failed`) + } + return result.stdout.trim() +} + +async function waitForPath(targetPath: string): Promise { + const deadline = Date.now() + 5_000 + while (!existsSync(targetPath)) { + if (Date.now() >= deadline) { + throw new Error(`Timed out waiting for ${targetPath}`) + } + await delay(20) + } +} + +async function stopProcess(child: ChildProcess): Promise { + if (child.exitCode !== null || child.signalCode !== null) { + return + } + const exited = new Promise((resolve) => child.once('exit', () => resolve())) + child.kill() + await Promise.race([exited, delay(500)]) + if (child.exitCode === null && child.pid) { + spawnSync('taskkill', ['/pid', String(child.pid), '/t', '/f'], { stdio: 'ignore' }) + await Promise.race([exited, delay(2_000)]) + } + if (child.exitCode === null && child.signalCode === null) { + throw new Error('Failed to stop the filesystem churn process') + } +} + +describe('local Windows worktree removal recovery (live Git)', () => { + afterEach(async () => { + await Promise.all( + tempRoots + .splice(0) + .map((root) => rm(root, { recursive: true, force: true, maxRetries: 8, retryDelay: 100 })) + ) + }) + + it.runIf(process.platform === 'win32')( + 'finishes deletion when Git deregisters before a directory-not-empty failure', + async () => { + const root = await mkdtemp(path.join(tmpdir(), 'orca-worktree-enotempty-')) + tempRoots.push(root) + const repoPath = path.join(root, 'repo') + const worktreePath = path.join(root, 'worktree') + + git(root, ['init', '--quiet', repoPath]) + git(repoPath, ['config', 'user.email', 'test@example.com']) + git(repoPath, ['config', 'user.name', 'Test User']) + await writeFile(path.join(repoPath, '.gitignore'), 'churn/\n') + git(repoPath, ['add', '.gitignore']) + git(repoPath, ['commit', '--quiet', '-m', 'initial']) + git(repoPath, ['worktree', 'add', '--quiet', '-b', 'feature/churn', worktreePath]) + const head = git(worktreePath, ['rev-parse', 'HEAD']) + + // Why: a producer racing Git's recursive delete deterministically exposes + // the Windows ENOTEMPTY state reported by real workspace deletions. + const churn = spawn( + process.execPath, + [ + '-e', + [ + "const fs = require('node:fs')", + "const path = require('node:path')", + 'const root = process.argv[1]', + 'let i = 0', + 'while (fs.existsSync(root)) {', + ' try {', + " const dir = path.join(root, 'churn')", + ' fs.mkdirSync(dir, { recursive: true })', + " fs.writeFileSync(path.join(dir, 'f' + (i++ % 64) + '.txt'), 'x')", + ' } catch {}', + '}' + ].join('\n'), + worktreePath + ], + { stdio: 'ignore' } + ) + + try { + await waitForPath(path.join(worktreePath, 'churn')) + const removal = spawnSync('git', ['worktree', 'remove', worktreePath], { + cwd: repoPath, + encoding: 'utf8' + }) + expect(removal.status).not.toBe(0) + expect(git(repoPath, ['worktree', 'list', '--porcelain'])).not.toContain(worktreePath) + expect(existsSync(worktreePath)).toBe(true) + + await stopProcess(churn) + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: removal.stderr + }), + force: false, + canonicalWorktreePath: worktreePath, + repoPath, + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature/churn', head }, + deleteBranch: false, + closeWatcher: async () => {} + }) + ).resolves.toEqual({}) + + expect(existsSync(worktreePath)).toBe(false) + } finally { + await stopProcess(churn) + } + } + ) +}) diff --git a/src/main/local-worktree-removal-recovery.test.ts b/src/main/local-worktree-removal-recovery.test.ts index 9f3d7550251..cb6dbcbb51e 100644 --- a/src/main/local-worktree-removal-recovery.test.ts +++ b/src/main/local-worktree-removal-recovery.test.ts @@ -1,9 +1,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { gitExecFileAsyncMock, removeLocalWorktreePathMock } = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - removeLocalWorktreePathMock: vi.fn() -})) +const { gitExecFileAsyncMock, listWorktreesStrictMock, removeLocalWorktreePathMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + listWorktreesStrictMock: vi.fn(), + removeLocalWorktreePathMock: vi.fn() + }) +) vi.mock('./git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock @@ -13,7 +16,14 @@ vi.mock('./local-worktree-filesystem', () => ({ removeLocalWorktreePath: removeLocalWorktreePathMock })) -import { recoverLocalWindowsLongPathWorktreeRemoval } from './local-worktree-removal-recovery' +vi.mock('./git/worktree', () => ({ + listWorktreesStrict: listWorktreesStrictMock +})) + +import { + recoverLocalWindowsWorktreeRemoval, + removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval +} from './local-worktree-removal-recovery' async function withPlatform(platform: NodeJS.Platform, fn: () => Promise): Promise { const original = process.platform @@ -25,11 +35,13 @@ async function withPlatform(platform: NodeJS.Platform, fn: () => Promise): } } -describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { +describe('recoverLocalWindowsWorktreeRemoval', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() + listWorktreesStrictMock.mockReset() removeLocalWorktreePathMock.mockReset() gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + listWorktreesStrictMock.mockResolvedValue([]) removeLocalWorktreePathMock.mockResolvedValue(undefined) }) @@ -43,7 +55,7 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { stderr: "error: failed to delete 'C:/repo/worktree/delete-e2e-held-cwd': Permission denied" }) - const result = await recoverLocalWindowsLongPathWorktreeRemoval({ + const result = await recoverLocalWindowsWorktreeRemoval({ error, force: true, canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', @@ -70,6 +82,90 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { }) }) + it('finishes a clean non-force deletion after Git leaves a Windows directory behind', async () => { + await withPlatform('win32', async () => { + const error = Object.assign(new Error('git worktree remove failed'), { + stderr: + "error: failed to delete 'C:/repo/worktree/delete-e2e-held-cwd': Directory not empty" + }) + + const result = await recoverLocalWindowsWorktreeRemoval({ + error, + force: false, + canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/delete-e2e-held-cwd', head: 'abc123' }, + deleteBranch: true, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith( + 'C:/repo/worktree/delete-e2e-held-cwd', + {} + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { + cwd: 'C:/repo' + }) + expect(result).toEqual({ + preservedBranch: { + branchName: 'delete-e2e-held-cwd', + head: 'abc123' + } + }) + }) + }) + + it('recovers localized Windows failures after Git already removed the registration', async () => { + await withPlatform('win32', async () => { + const result = await recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: "Fehler: 'C:/repo/worktree/feature' konnte nicht gelöscht werden" + }), + force: false, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: false, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + + expect(removeLocalWorktreePathMock).toHaveBeenCalledWith('C:/repo/worktree/feature', {}) + expect(result).toEqual({}) + }) + }) + + it('does not recover unrelated localized failures while Git still owns the row', async () => { + await withPlatform('win32', async () => { + listWorktreesStrictMock.mockResolvedValue([ + { + path: 'C:/repo/worktree/feature', + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } + ]) + + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: 'Fehler: unerwarteter Git-Zustand' + }), + force: false, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: false, + closeWatcher: vi.fn().mockResolvedValue(undefined) + }) + ).resolves.toBeUndefined() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + }) + it('does not recover partial filesystem deletion wording off Windows', async () => { await withPlatform('linux', async () => { const error = Object.assign(new Error('git worktree remove failed'), { @@ -77,7 +173,7 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { }) await expect( - recoverLocalWindowsLongPathWorktreeRemoval({ + recoverLocalWindowsWorktreeRemoval({ error, force: true, canonicalWorktreePath: 'C:/repo/worktree/delete-e2e-held-cwd', @@ -91,4 +187,149 @@ describe('recoverLocalWindowsLongPathWorktreeRemoval', () => { expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() }) }) + + it('does not recover a locked registration after a filesystem removal failure', async () => { + await withPlatform('win32', async () => { + const registeredWorktree = { + branch: 'refs/heads/feature', + head: 'abc123', + locked: true, + lockReason: 'active agent' + } + const closeWatcher = vi.fn().mockResolvedValue(undefined) + + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: 'error: failed to delete deep/file.txt: Filename too long' + }), + force: true, + canonicalWorktreePath: 'C:/workspaces/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree, + deleteBranch: true, + closeWatcher + }) + ).rejects.toThrow('Worktree is locked by Git. Lock reason: active agent') + + expect(closeWatcher).not.toHaveBeenCalled() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + expect(listWorktreesStrictMock).not.toHaveBeenCalled() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + }) + + it.each([ + "error: failed to delete 'C:/repo/worktree/feature': Directory not empty", + 'error: failed to delete deep/file.txt: Filename too long' + ])( + 'does not recover a recognized Windows failure while Git still owns the row: %s', + async (stderr) => { + await withPlatform('win32', async () => { + listWorktreesStrictMock.mockResolvedValue([ + { + path: 'C:/repo/worktree/feature', + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } + ]) + const closeWatcher = vi.fn().mockResolvedValue(undefined) + + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { stderr }), + force: true, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: { wslDistro: 'Ubuntu' }, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: true, + closeWatcher + }) + ).resolves.toBeUndefined() + + expect(listWorktreesStrictMock).toHaveBeenCalledWith('C:/repo', { wslDistro: 'Ubuntu' }) + expect(closeWatcher).not.toHaveBeenCalled() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + } + ) + + it('does not recover when structural registration verification fails', async () => { + await withPlatform('win32', async () => { + listWorktreesStrictMock.mockRejectedValue(new Error('git list failed')) + const closeWatcher = vi.fn().mockResolvedValue(undefined) + + await expect( + recoverLocalWindowsWorktreeRemoval({ + error: Object.assign(new Error('git worktree remove failed'), { + stderr: "error: failed to delete 'C:/repo/worktree/feature': Directory not empty" + }), + force: true, + canonicalWorktreePath: 'C:/repo/worktree/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: true, + closeWatcher + }) + ).resolves.toBeUndefined() + + expect(closeWatcher).not.toHaveBeenCalled() + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + }) +}) + +describe('removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + listWorktreesStrictMock.mockReset() + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + listWorktreesStrictMock.mockResolvedValue([]) + }) + + it('does not override a locked missing registration', async () => { + await expect( + removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + canonicalWorktreePath: 'C:/workspaces/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { + branch: 'refs/heads/feature', + head: 'abc123', + locked: true, + lockReason: 'active agent' + }, + deleteBranch: true + }) + ).rejects.toThrow('Worktree is locked by Git') + + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('does not report success when prune leaves the registration behind', async () => { + listWorktreesStrictMock.mockResolvedValue([ + { + path: 'C:/workspaces/feature', + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false + } + ]) + + await expect( + removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + canonicalWorktreePath: 'C:/workspaces/feature', + repoPath: 'C:/repo', + localWorktreeGitOptions: {}, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: true + }) + ).rejects.toThrow('Git still has stale worktree registration') + }) }) diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index 4e915bfa950..25f93e2b2de 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -1,25 +1,23 @@ import type { GitWorktreeInfo, RemoveWorktreeResult } from '../shared/types' -import { - formatWorktreeRemovalError, - isWindowsLongPathWorktreeRemovalError -} from './ipc/worktree-logic' +import { assertWorktreeUnlockedForRemoval } from '../shared/worktree-removal' +import { areWorktreePathsEqual, formatWorktreeRemovalError } from './ipc/worktree-logic' import { gitExecFileAsync } from './git/runner' -import type { GitWorktreeExecOptions } from './git/worktree' +import { listWorktreesStrict, type GitWorktreeExecOptions } from './git/worktree' import { removeLocalWorktreePath } from './local-worktree-filesystem' -type LocalWindowsLongPathRecoveryArgs = { +type LocalWindowsRemovalRecoveryArgs = { error: unknown force: boolean canonicalWorktreePath: string repoPath: string localWorktreeGitOptions: GitWorktreeExecOptions - registeredWorktree: Pick + registeredWorktree: Pick deleteBranch: boolean closeWatcher: (worktreePath: string) => Promise } type StaleLocalWorktreeRegistrationArgs = Omit< - LocalWindowsLongPathRecoveryArgs, + LocalWindowsRemovalRecoveryArgs, 'error' | 'force' | 'closeWatcher' > @@ -38,73 +36,120 @@ function preservedBranchResult( } } -async function pruneRequiredGitWorktreeRegistration( +function staleRegistrationRecoveryError( + error: unknown, + canonicalWorktreePath: string, + force: boolean +): Error { + return new Error( + `${formatWorktreeRemovalError( + error, + canonicalWorktreePath, + force + )} The worktree directory was removed, but Git still has stale worktree registration. Retry deletion after resolving the Git registration error.` + ) +} + +async function verifyGitWorktreeRegistrationRemoved( repoPath: string, localWorktreeGitOptions: GitWorktreeExecOptions, - canonicalWorktreePath: string + canonicalWorktreePath: string, + force: boolean ): Promise { try { - await gitExecFileAsync(['worktree', 'prune'], { - cwd: repoPath, - ...localWorktreeGitOptions - }) + const remainingWorktrees = await listWorktreesStrict(repoPath, localWorktreeGitOptions) + if ( + remainingWorktrees.some((worktree) => + areWorktreePathsEqual(worktree.path, canonicalWorktreePath) + ) + ) { + throw new Error('Git still reports the worktree registration after cleanup.') + } } catch (error) { - throw new Error( - `${formatWorktreeRemovalError( - error, - canonicalWorktreePath, - true - )} The worktree directory was removed, but Git still has stale worktree registration. Retry deletion after resolving the Git prune error.` - ) + throw staleRegistrationRecoveryError(error, canonicalWorktreePath, force) } } -export async function recoverLocalWindowsLongPathWorktreeRemoval( - args: LocalWindowsLongPathRecoveryArgs +async function removeRequiredGitWorktreeRegistration( + args: StaleLocalWorktreeRegistrationArgs, + forceForError = true +): Promise { + assertWorktreeUnlockedForRemoval(args.registeredWorktree) + + let result: RemoveWorktreeResult | undefined + let removalError: unknown + try { + await gitExecFileAsync(['worktree', 'prune'], { + cwd: args.repoPath, + ...args.localWorktreeGitOptions + }) + result = preservedBranchResult(args.registeredWorktree, args.deleteBranch) + } catch (error) { + removalError = error + } + + // Why: Git prune exits successfully while retaining locked registrations; + // a failed remove can also have detached the row before filesystem cleanup. + try { + await verifyGitWorktreeRegistrationRemoved( + args.repoPath, + args.localWorktreeGitOptions, + args.canonicalWorktreePath, + forceForError + ) + } catch (verificationError) { + throw removalError + ? staleRegistrationRecoveryError(removalError, args.canonicalWorktreePath, forceForError) + : verificationError + } + // Why: if Git detached the row before reporting its filesystem error, keep + // the branch rather than guessing whether the normal branch cleanup ran. + return result ?? preservedBranchResult(args.registeredWorktree, args.deleteBranch) +} + +export async function recoverLocalWindowsWorktreeRemoval( + args: LocalWindowsRemovalRecoveryArgs ): Promise { - if (!args.force || !isRecoverableWindowsFilesystemRemovalError(args.error)) { + // Why: recovery can recursively delete the remaining directory, so a Git + // lock must reject the attempt before classification or any side effects. + assertWorktreeUnlockedForRemoval(args.registeredWorktree) + + if (!(await isRecoverableWindowsFilesystemRemovalFailure(args))) { return undefined } - // Why: watcher shutdown is best-effort, but Git registration must be pruned - // before callers clear Orca metadata or the branch remains locked. + // Why: this error means Git accepted removal and started deleting the path; + // finish that partial success with Windows retries, then verify Git metadata. await args.closeWatcher(args.canonicalWorktreePath).catch(() => {}) try { await removeLocalWorktreePath(args.canonicalWorktreePath, args.localWorktreeGitOptions) } catch (error) { - throw new Error(formatWorktreeRemovalError(error, args.canonicalWorktreePath, true)) + throw new Error(formatWorktreeRemovalError(error, args.canonicalWorktreePath, args.force)) } - await pruneRequiredGitWorktreeRegistration( - args.repoPath, - args.localWorktreeGitOptions, - args.canonicalWorktreePath - ) - return preservedBranchResult(args.registeredWorktree, args.deleteBranch) + return removeRequiredGitWorktreeRegistration(args, args.force) } -function isRecoverableWindowsFilesystemRemovalError(error: unknown): boolean { - if (isWindowsLongPathWorktreeRemovalError(error)) { - return true +async function isRecoverableWindowsFilesystemRemovalFailure( + args: LocalWindowsRemovalRecoveryArgs +): Promise { + if (process.platform !== 'win32' || typeof args.error !== 'object' || args.error === null) { + return false } - if (process.platform !== 'win32' || typeof error !== 'object' || error === null) { + + try { + const worktrees = await listWorktreesStrict(args.repoPath, args.localWorktreeGitOptions) + // Why: error prose can be localized or ambiguous. Only a missing Git row + // proves removal started and makes recursive Windows cleanup safe. + return !worktrees.some((worktree) => + areWorktreePathsEqual(worktree.path, args.canonicalWorktreePath) + ) + } catch { return false } - const errorWithDetails = error as { message?: unknown; stderr?: unknown; stdout?: unknown } - const details = [errorWithDetails.stderr, errorWithDetails.stdout, errorWithDetails.message] - .filter((value): value is string => typeof value === 'string' && value.trim().length > 0) - .join('\n') - return /failed to delete .*(?:directory not empty|permission denied|access is denied|being used by another process)|(?:directory not empty|permission denied|access is denied|being used by another process).*failed to delete/i.test( - details - ) } -export async function pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval( +export async function removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval( args: StaleLocalWorktreeRegistrationArgs ): Promise { - await pruneRequiredGitWorktreeRegistration( - args.repoPath, - args.localWorktreeGitOptions, - args.canonicalWorktreePath - ) - return preservedBranchResult(args.registeredWorktree, args.deleteBranch) + return removeRequiredGitWorktreeRegistration(args) } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index a9f03794db0..c934a06a7a8 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -74,6 +74,7 @@ import { unregisterSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' import { registerSshGitProvider, unregisterSshGitProvider } from '../providers/ssh-git-dispatch' +import * as localWorktreeFilesystem from '../local-worktree-filesystem' import { DEFAULT_REPO_BADGE_COLOR, FLOATING_TERMINAL_WORKTREE_ID, @@ -89,6 +90,12 @@ import { TERMINAL_METHODS } from './rpc/methods/terminal' const ORIGINAL_PLATFORM = process.platform const ORIGINAL_PLATFORM_DESCRIPTOR = Object.getOwnPropertyDescriptor(process, 'platform') +const removeWorktreeLinkedPathsMock = vi.hoisted(() => vi.fn()) + +vi.mock('../ipc/worktree-symlinks', () => ({ + createWorktreeLinkedPaths: vi.fn(), + removeWorktreeLinkedPaths: removeWorktreeLinkedPathsMock +})) async function waitForMobileSessionTabsEvents( events: RuntimeMobileSessionTabsResult[], @@ -544,6 +551,7 @@ function resetRuntimeTestMocks(): void { vi.mocked(assertWorktreeCleanForRemoval).mockReset() vi.mocked(assertWorktreeCleanForRemoval).mockResolvedValue(undefined) vi.mocked(removeWorktree).mockReset() + removeWorktreeLinkedPathsMock.mockReset() vi.mocked(forceDeleteLocalBranchMock).mockReset() vi.mocked(forceDeleteLocalBranchMock).mockResolvedValue(undefined) sshGitProviders.clear() @@ -4443,7 +4451,7 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(remoteStore as never) try { - await runtime.removeManagedWorktree('path:/remote/feature', true) + await runtime.removeManagedWorktree('path:/remote/feature', true, false) } finally { unregisterSshGitProvider('ssh-1') } @@ -27098,6 +27106,10 @@ describe('OrcaRuntimeService', () => { stderr: 'error: failed to delete deep/file.txt: Filename too long' }) ) + vi.mocked(listWorktreesStrict) + .mockResolvedValueOnce(MOCK_GIT_WORKTREES) + .mockResolvedValueOnce(MOCK_GIT_WORKTREES) + .mockResolvedValue([]) try { const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, true) @@ -27119,7 +27131,7 @@ describe('OrcaRuntimeService', () => { } }) - it('keeps runtime metadata when long-path recovery deletes the directory but prune fails', async () => { + it('refuses runtime Windows recovery while Git still reports the row and keeps metadata', async () => { setPlatform('win32') const removeWorktreeMeta = vi.fn() const runtimeStore = { @@ -27127,11 +27139,13 @@ describe('OrcaRuntimeService', () => { removeWorktreeMeta } const runtime = new OrcaRuntimeService(runtimeStore as never) - const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockRejectedValue( - Object.assign(new Error('git prune failed'), { - stderr: 'fatal: unable to lock worktree admin dir' - }) - ) + const gitSpy = vi.spyOn(gitRunner, 'gitExecFileAsync').mockResolvedValue({ + stdout: '', + stderr: '' + }) + const removePathSpy = vi + .spyOn(localWorktreeFilesystem, 'removeLocalWorktreePath') + .mockResolvedValue(undefined) vi.mocked(getEffectiveHooks).mockReturnValue(null) vi.mocked(removeWorktree).mockRejectedValue( Object.assign(new Error('git worktree remove failed'), { @@ -27141,10 +27155,13 @@ describe('OrcaRuntimeService', () => { try { await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true)).rejects.toThrow( - 'Git still has stale worktree registration' + `Failed to force delete worktree at ${TEST_WORKTREE_PATH}. error: failed to delete deep/file.txt: Filename too long` ) + expect(removePathSpy).not.toHaveBeenCalled() + expect(gitSpy).not.toHaveBeenCalledWith(['worktree', 'prune'], expect.anything()) expect(removeWorktreeMeta).not.toHaveBeenCalled() } finally { + removePathSpy.mockRestore() gitSpy.mockRestore() } }) @@ -27176,7 +27193,7 @@ describe('OrcaRuntimeService', () => { stderr: '' }) vi.mocked(listWorktrees).mockResolvedValue(registeredWorktrees) - vi.mocked(listWorktreesStrict).mockResolvedValue(registeredWorktrees) + vi.mocked(listWorktreesStrict).mockResolvedValueOnce(registeredWorktrees).mockResolvedValue([]) vi.mocked(getEffectiveHooks).mockReturnValue({ scripts: { archive: 'pnpm worktree:archive' @@ -27200,6 +27217,34 @@ describe('OrcaRuntimeService', () => { } }) + it('preserves a locked missing runtime registration even with force', async () => { + setPlatform('win32') + const missingWorktreePath = 'C:\\workspace\\locked-already-removed' + const worktreeId = `${TEST_REPO_ID}::${missingWorktreePath}` + const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(worktreeId) + const runtime = new OrcaRuntimeService(runtimeStore as never) + const registeredWorktrees = [ + { + path: missingWorktreePath, + head: 'abc', + branch: 'refs/heads/feature/foo', + isBare: false, + isMainWorktree: false, + locked: true, + lockReason: 'active agent session' + } + ] + vi.mocked(listWorktreesStrict).mockResolvedValue(registeredWorktrees) + vi.mocked(removeWorktree).mockResolvedValue({}) + + await expect(runtime.removeManagedWorktree(worktreeId, true, false)).rejects.toThrow( + 'Worktree is locked by Git. Lock reason: active agent session' + ) + + expect(removeWorktree).not.toHaveBeenCalled() + expect(removeWorktreeMeta).not.toHaveBeenCalled() + }) + it('routes runtime worktree removal through the selected WSL project runtime', async () => { setPlatform('win32') const runtimeStore = { @@ -27928,6 +27973,83 @@ describe('OrcaRuntimeService', () => { expect(removeWorktree).not.toHaveBeenCalled() }) + it('fails locked dirty-force deletes before hooks, link cleanup, or PTY teardown', async () => { + const repo = { ...store.getRepos()[0], symlinkPaths: ['node_modules'] } + const runtimeStore = { + ...store, + getRepos: () => [repo], + getRepo: () => repo + } + const runtime = new OrcaRuntimeService(runtimeStore as never) + const killSpy = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write: () => true, + kill: (id) => killSpy(id), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + vi.mocked(getEffectiveHooks).mockReturnValue({ + scripts: { archive: 'pnpm worktree:archive' } + }) + vi.mocked(listWorktreesStrict).mockResolvedValue([ + { + ...MOCK_GIT_WORKTREES[0], + locked: true, + lockReason: 'active agent session' + } + ]) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, true)).rejects.toThrow( + `Failed to force delete worktree at ${TEST_WORKTREE_PATH}. Worktree is locked by Git.` + ) + + expect(assertWorktreeCleanForRemoval).not.toHaveBeenCalled() + expect(runHook).not.toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(killSpy).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + }) + + it('rechecks a runtime Git lock after the archive hook before teardown', async () => { + const repo = { ...store.getRepos()[0], symlinkPaths: ['node_modules'] } + const runtimeStore = { + ...store, + getRepos: () => [repo], + getRepo: () => repo + } + const runtime = new OrcaRuntimeService(runtimeStore as never) + const killSpy = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write: () => true, + kill: (id) => killSpy(id), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + vi.mocked(getEffectiveHooks).mockReturnValue({ + scripts: { archive: 'pnpm worktree:archive' } + }) + vi.mocked(runHook).mockResolvedValue({ success: true, output: '' }) + vi.mocked(listWorktreesStrict) + .mockResolvedValueOnce(MOCK_GIT_WORKTREES) + .mockResolvedValueOnce([ + { + ...MOCK_GIT_WORKTREES[0], + locked: true, + lockReason: 'locked during archive' + } + ]) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, true)).rejects.toThrow( + 'Worktree is locked by Git' + ) + + expect(runHook).toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(assertWorktreeCleanForRemoval).not.toHaveBeenCalled() + expect(killSpy).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + }) + it('formats preflight subprocess failures and skips PTY teardown', async () => { const runtime = new OrcaRuntimeService(store) const killSpy = vi.fn().mockReturnValue(true) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index be8dc19a9ee..789a50ce913 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -139,6 +139,7 @@ import type { WorkspaceSessionState, DirEntry } from '../../shared/types' +import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree-removal' import { getRepoExecutionHostId, parseExecutionHostId, @@ -469,8 +470,8 @@ import { toLocalWorktreeRuntimePath } from '../local-worktree-filesystem' import { - pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval, - recoverLocalWindowsLongPathWorktreeRemoval + removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval, + recoverLocalWindowsWorktreeRemoval } from '../local-worktree-removal-recovery' import { connect as connectLinear, @@ -16552,8 +16553,16 @@ export class OrcaRuntimeService { const canonicalWorktreePath = registeredWorktree.path const deleteBranch = removedMeta?.preserveBranchOnDelete !== true + // Why: a Git lock must block before archive hooks or linked-path cleanup + // mutate the workspace; dirty-file force is a separate permission. + try { + assertWorktreeUnlockedForRemoval(registeredWorktree) + } catch (error) { + throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) + } + // Why: a prior forced Windows recovery can delete the directory but leave - // Git's stale registration; retry by pruning instead of removing a missing path. + // Git's stale registration; recover and verify it before clearing metadata. if ( !repo.connectionId && force === true && @@ -16562,7 +16571,7 @@ export class OrcaRuntimeService { removedMeta && (await isRuntimeWorktreePathMissing(repo, canonicalWorktreePath, localWorktreeGitOptions)) ) { - const removalResult = await pruneStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, @@ -16590,9 +16599,10 @@ export class OrcaRuntimeService { return removalResult ?? {} } if (repo.connectionId) { - const rawRemovalResult = await (deleteBranch - ? provider!.removeWorktree(canonicalWorktreePath, force) - : provider!.removeWorktree(canonicalWorktreePath, force, { deleteBranch })) + const remoteRemoveOptions = !deleteBranch ? { deleteBranch } : {} + const rawRemovalResult = await (Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, force)) const removalResult = this.preserveBranchHeadFallback( rawRemovalResult, registeredWorktree.head @@ -16637,6 +16647,27 @@ export class OrcaRuntimeService { console.warn(`[hooks] ${warning}`) } + const refreshedWorktrees = hasLocalWorktreeGitOptions + ? await listWorktreesStrict(repo.path, localWorktreeGitOptions) + : await listWorktreesStrict(repo.path) + const refreshedRegisteredWorktree = findRegisteredDeletableWorktree( + repo.path, + canonicalWorktreePath, + refreshedWorktrees + ) + if (!refreshedRegisteredWorktree) { + throw new Error( + `Worktree registration changed during deletion: ${canonicalWorktreePath}. Retry deletion.` + ) + } + try { + // Why: an archive hook can race another Git client that locks the row; + // recheck before linked-path, watcher, or terminal teardown side effects. + assertWorktreeUnlockedForRemoval(refreshedRegisteredWorktree) + } catch (error) { + throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, force)) + } + let shouldTearDownPtys = true if (repo.symlinkPaths && repo.symlinkPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, repo.symlinkPaths) @@ -16691,23 +16722,23 @@ export class OrcaRuntimeService { ...(!deleteBranch ? { deleteBranch } : {}), // Why: removal already validated the Git row under the selected // project runtime; keep branch cleanup on that same canonical row. - knownRemovedWorktree: registeredWorktree, + knownRemovedWorktree: refreshedRegisteredWorktree, ...localWorktreeGitOptions } removalResult = this.preserveBranchHeadFallback( await removeWorktree(repo.path, canonicalWorktreePath, force, removeOptions), - registeredWorktree.head + refreshedRegisteredWorktree.head ) } catch (error) { - // Why: Git for Windows can fail long-path directory deletion after - // Orca has already validated the target and explicit force delete. - const recoveredRemovalResult = await recoverLocalWindowsLongPathWorktreeRemoval({ + // Why: Git for Windows can deregister a clean worktree before its + // recursive filesystem deletion fails transiently. + const recoveredRemovalResult = await recoverLocalWindowsWorktreeRemoval({ error, force, canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, - registeredWorktree, + registeredWorktree: refreshedRegisteredWorktree, deleteBranch, closeWatcher: (worktreePath) => closeLocalWatcherForWorktreePath(worktreePath).catch((err) => { @@ -16776,7 +16807,7 @@ export class OrcaRuntimeService { this.rememberPreservedBranchCleanupTarget( removalTarget.id, removalResult, - registeredWorktree.head, + refreshedRegisteredWorktree.head, removedPushTarget ) this.clearOptimisticReconcileToken(removalTarget.id) diff --git a/src/main/runtime/rpc/methods/worktree.test.ts b/src/main/runtime/rpc/methods/worktree.test.ts index ee4e011023a..ef7ef3910f0 100644 --- a/src/main/runtime/rpc/methods/worktree.test.ts +++ b/src/main/runtime/rpc/methods/worktree.test.ts @@ -67,6 +67,25 @@ describe('worktree RPC methods', () => { }) }) + it('routes dirty-file force to the runtime server', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + removeManagedWorktree: vi.fn().mockResolvedValue({}) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest('worktree.rm', { + worktree: 'id:wt-1', + force: true, + runHooks: false + }) + ) + + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', true, false) + expect(response).toMatchObject({ ok: true, result: { removed: true } }) + }) + it('routes create options to the runtime server', async () => { const runtime = { getRuntimeId: () => 'test-runtime', diff --git a/src/relay/git-handler-utils.test.ts b/src/relay/git-handler-utils.test.ts index 03d0fe14aed..211e4b205f4 100644 --- a/src/relay/git-handler-utils.test.ts +++ b/src/relay/git-handler-utils.test.ts @@ -1,6 +1,49 @@ import { describe, expect, it } from 'vitest' import { parseStatusOutput } from './git-status-output-parser' -import { isUnsupportedWorktreeListZError } from './git-handler-utils' +import { isUnsupportedWorktreeListZError, parseWorktreeList } from './git-handler-utils' + +describe('parseWorktreeList', () => { + it('preserves SSH worktree lock metadata from porcelain output', () => { + expect( + parseWorktreeList( + 'worktree /repo\nHEAD abc\nbranch refs/heads/main\n\nworktree /locked\nHEAD def\nbranch refs/heads/feature\nlocked remote session\n' + )[1] + ).toMatchObject({ + path: '/locked', + locked: true, + lockReason: 'remote session' + }) + }) + + it('decodes C-quoted lock reasons from legacy line porcelain output', () => { + const output = + 'worktree /repo\nHEAD abc\nbranch refs/heads/main\n\nworktree /locked\nHEAD def\nbranch refs/heads/feature\nlocked "first line\\nsecond line \\303\\251"\n' + + expect(parseWorktreeList(output)[1]).toMatchObject({ + locked: true, + lockReason: 'first line\nsecond line é' + }) + }) + + it('keeps NUL-delimited lock reasons raw', () => { + const output = [ + 'worktree /repo', + 'HEAD abc', + 'branch refs/heads/main', + '', + 'worktree /locked', + 'HEAD def', + 'branch refs/heads/feature', + 'locked "literal\\nquote"', + '' + ].join('\0') + + expect(parseWorktreeList(output, { nulDelimited: true })[1]).toMatchObject({ + locked: true, + lockReason: '"literal\\nquote"' + }) + }) +}) describe('isUnsupportedWorktreeListZError', () => { it('detects an unknown-switch usage error from stderr when the exit code is absent', () => { diff --git a/src/relay/git-handler-utils.ts b/src/relay/git-handler-utils.ts index f0a395da27b..744f4b65d83 100644 --- a/src/relay/git-handler-utils.ts +++ b/src/relay/git-handler-utils.ts @@ -7,6 +7,7 @@ */ import { existsSync } from 'node:fs' import * as path from 'node:path' +import { decodeGitCQuotedPath } from '../shared/git-cquoted-path' import { isBinaryBuffer } from '../shared/binary-buffer' import type { GitLineStats } from '../shared/git-uncommitted-line-stats' @@ -178,6 +179,8 @@ export function parseWorktreeList( let head = '' let branch = '' let isBare = false + let locked = false + let lockReason = '' for (const line of lines) { if (line.startsWith('worktree ')) { @@ -188,6 +191,10 @@ export function parseWorktreeList( branch = line.slice('branch '.length) } else if (line === 'bare') { isBare = true + } else if (line === 'locked' || line.startsWith('locked ')) { + locked = true + const rawReason = line.slice('locked'.length).trim() + lockReason = options.nulDelimited ? rawReason : decodeGitCQuotedPath(rawReason) } } @@ -197,6 +204,8 @@ export function parseWorktreeList( head, branch, isBare, + ...(locked ? { locked: true } : {}), + ...(lockReason ? { lockReason } : {}), isMainWorktree: worktrees.length === 0 }) } diff --git a/src/relay/git-handler-worktree-ops.test.ts b/src/relay/git-handler-worktree-ops.test.ts index 52cc1236054..e16d4d61fc9 100644 --- a/src/relay/git-handler-worktree-ops.test.ts +++ b/src/relay/git-handler-worktree-ops.test.ts @@ -128,6 +128,32 @@ describe('addWorktreeOp', () => { }) describe('removeWorktreeOp', () => { + it('rejects a locked SSH worktree before invoking remove', async () => { + const git = vi.fn(async (args) => { + if (args[0] === 'rev-parse') { + return { stdout: '/repo/.git\n', stderr: '' } + } + if (args[0] === 'worktree' && args[1] === 'list') { + return { + stdout: `${worktreeList( + { path: '/repo', branch: 'main' }, + { path: '/repo-feature', branch: 'feature/test' } + )}\nlocked remote agent`, + stderr: '' + } + } + return { stdout: '', stderr: '' } + }) + + await expect(removeWorktreeOp(git, { worktreePath: '/repo-feature' })).rejects.toThrow( + 'Worktree is locked by Git. Lock reason: remote agent.' + ) + expect(git).not.toHaveBeenCalledWith( + ['worktree', 'remove', '/repo-feature'], + expect.any(String) + ) + }) + it('deletes the now-unused branch after removing an SSH worktree', async () => { const calls: string[] = [] let listCount = 0 @@ -224,10 +250,41 @@ describe('removeWorktreeOp', () => { forceBranchDelete: true }) + expect(git).toHaveBeenCalledWith( + ['worktree', 'remove', '--force', '/repo-feature'], + expect.any(String) + ) expect(git).toHaveBeenCalledWith(['branch', '-D', '--', 'feature/test'], expect.any(String)) expect(git).not.toHaveBeenCalledWith(['branch', '-d', '--', 'feature/test'], expect.any(String)) }) + it('does not let force override a locked SSH worktree', async () => { + const git = vi.fn(async (args) => { + if (args[0] === 'rev-parse') { + return { stdout: '/repo/.git\n', stderr: '' } + } + if (args[0] === 'worktree' && args[1] === 'list') { + return { + stdout: `${worktreeList( + { path: '/repo', branch: 'main' }, + { path: '/repo-feature', branch: 'feature/test' } + )}\nlocked remote agent`, + stderr: '' + } + } + return { stdout: '', stderr: '' } + }) + + await expect( + removeWorktreeOp(git, { worktreePath: '/repo-feature', force: true }) + ).rejects.toThrow('Worktree is locked by Git. Lock reason: remote agent') + + expect(git).not.toHaveBeenCalledWith( + ['worktree', 'remove', '--force', '/repo-feature'], + expect.any(String) + ) + }) + it('skips branch deletion entirely when deleteBranch is false', async () => { const calls: string[] = [] const git = vi.fn(async (args, cwd) => { diff --git a/src/relay/git-handler-worktree-remove.ts b/src/relay/git-handler-worktree-remove.ts index edd3932ef3b..7c426facc1e 100644 --- a/src/relay/git-handler-worktree-remove.ts +++ b/src/relay/git-handler-worktree-remove.ts @@ -1,5 +1,6 @@ import * as path from 'node:path' import type { RemoveWorktreeResult } from '../shared/types' +import { assertWorktreeUnlockedForRemoval } from '../shared/worktree-removal' import { deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure } from './git-handler-branch-cleanup' import type { GitExec } from './git-handler-ops' import { isUnsupportedWorktreeListZError, parseWorktreeList } from './git-handler-utils' @@ -8,6 +9,8 @@ type RelayWorktreeInfo = { path: string branch?: string head?: string + locked?: boolean + lockReason?: string } function getErrorText(error: unknown): string { @@ -79,7 +82,9 @@ function normalizeRelayWorktrees(worktrees: Record[]): RelayWor .map((worktree) => ({ path: typeof worktree.path === 'string' ? worktree.path : '', head: typeof worktree.head === 'string' ? worktree.head : undefined, - branch: typeof worktree.branch === 'string' ? worktree.branch : undefined + branch: typeof worktree.branch === 'string' ? worktree.branch : undefined, + locked: worktree.locked === true ? true : undefined, + lockReason: typeof worktree.lockReason === 'string' ? worktree.lockReason : undefined })) .filter((worktree) => worktree.path.length > 0) } @@ -173,6 +178,8 @@ export async function removeWorktreeOp( const branchName = normalizeLocalBranchRef(removedWorktree?.branch ?? '') const branchHead = removedWorktree?.head ?? '' + assertWorktreeUnlockedForRemoval(removedWorktree) + const args = ['worktree', 'remove'] if (force) { args.push('--force') diff --git a/src/renderer/src/components/settings/DevToolsPane.tsx b/src/renderer/src/components/settings/DevToolsPane.tsx index e1779e9aead..11bfb36d933 100644 --- a/src/renderer/src/components/settings/DevToolsPane.tsx +++ b/src/renderer/src/components/settings/DevToolsPane.tsx @@ -100,6 +100,7 @@ function showDeleteFailureToast(): void { 'branch has changes' ), canForceDelete: true, + forceDeleteReason: 'dirty', onViewChanges: () => toast.message( translate( diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx index 2a7e5bfbc36..7f1576c720b 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.test.tsx @@ -19,7 +19,16 @@ const mocks = vi.hoisted(() => { settings: null, gitStatusByWorktree: {} as Record, setGitStatus: vi.fn(), - deleteStateByWorktreeId: {} + deleteStateByWorktreeId: {} as Record< + string, + { + isDeleting: boolean + error: string | null + canForceDelete: boolean + forceDeleteReason: 'dirty' | 'orphan-directory' | 'missing-registration' | null + lockReason?: string | null + } + > } return { state, buttonProps: [] as Record[] } }) @@ -74,6 +83,10 @@ vi.mock('./delete-worktree-flow', () => ({ runWorktreeDeletesInParallel: vi.fn() })) +vi.mock('./active-worktree-focus-after-delete', () => ({ + prepareActiveWorktreeFocusAfterDelete: () => vi.fn() +})) + import { runWorktreeDeletesInParallel } from './delete-worktree-flow' function makeWorktree(id: string, path: string): Worktree { @@ -249,6 +262,32 @@ describe('DeleteWorktreeDialog lineage copy', () => { expect(markup).not.toContain('Also delete local branch') }) + it('shows locked and known dirty details without offering a lock override', async () => { + const workspace = makeWorktree('Locked workspace', '/workspaces/locked') + mocks.state.modalData = { worktreeId: workspace.id } + mocks.state.allWorktrees.mockReturnValue([workspace]) + mocks.state.gitStatusByWorktree = { + [workspace.id]: [{ path: 'src/file.ts', status: 'modified' }] + } + mocks.state.deleteStateByWorktreeId = { + [workspace.id]: { + isDeleting: false, + error: 'Worktree is locked by Git. Lock reason: active agent session.', + canForceDelete: false, + forceDeleteReason: null, + lockReason: 'active agent session' + } + } + + const { default: DeleteWorktreeDialog } = await import('./DeleteWorktreeDialog') + const markup = renderToStaticMarkup() + + expect(markup).toContain('Worktree is locked by Git. Lock reason: active agent session.') + expect(markup).not.toContain('Force Delete') + expect(markup).toContain('1 uncommitted or untracked change') + expect(mocks.state.removeWorktree).not.toHaveBeenCalled() + }) + it('notifies the dialog caller after a toast force delete succeeds', async () => { const workspace = makeWorktree('Workspace', '/workspaces/workspace') const onDeleted = vi.fn() diff --git a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx index cb4c9f3da58..598f5bedd65 100644 --- a/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx +++ b/src/renderer/src/components/sidebar/DeleteWorktreeDialog.tsx @@ -21,6 +21,7 @@ import { DeleteWorktreeDialogDescription } from './DeleteWorktreeDialogDescripti import { DeleteWorktreeTargetPreview } from './DeleteWorktreeTargetPreview' import { DeleteWorktreeWarningPanels } from './DeleteWorktreeWarningPanels' import { persistDeleteWorktreeConfirmSkipPreference } from './delete-worktree-preference-toast' +import { getDeleteWorktreeDirtyChangeCounts } from './delete-worktree-dirty-change-counts' import { countFolderWorkspaceDeletes, getDeleteWorktreeDialogCopy, @@ -137,19 +138,12 @@ const DeleteWorktreeDialog = React.memo(function DeleteWorktreeDialog() { [canDeleteAllLineage, lineageDelete.deleteAllTargets, worktrees] ) const dirtyChangeCountsByWorktreeId = useMemo(() => { - const result = new Map() - for (const item of deleteTargets) { - if (item.isMainWorktree || getIsFolderWorkspaceDelete(repoMap, item)) { - continue - } - const statusEntries = gitStatusByWorktree[item.id] - if ((statusEntries?.length ?? 0) > 0) { - result.set(item.id, statusEntries?.length ?? 0) - } else if (deleteStateByWorktreeId[item.id]?.canForceDelete) { - result.set(item.id, 0) - } - } - return result + return getDeleteWorktreeDirtyChangeCounts({ + deleteTargets, + deleteStateByWorktreeId, + gitStatusByWorktree, + repoMap + }) }, [deleteStateByWorktreeId, deleteTargets, gitStatusByWorktree, repoMap]) if (!isOpen && dontAskAgain) { diff --git a/src/renderer/src/components/sidebar/delete-worktree-dirty-change-counts.ts b/src/renderer/src/components/sidebar/delete-worktree-dirty-change-counts.ts new file mode 100644 index 00000000000..3a0f709caae --- /dev/null +++ b/src/renderer/src/components/sidebar/delete-worktree-dirty-change-counts.ts @@ -0,0 +1,32 @@ +import type { Repo, Worktree } from '../../../../shared/types' +import type { WorktreeDeleteState } from '../../store/slices/worktree-helpers' +import { isFolderWorkspaceDelete } from './delete-worktree-dialog-copy' + +export function getDeleteWorktreeDirtyChangeCounts({ + deleteTargets, + deleteStateByWorktreeId, + gitStatusByWorktree, + repoMap +}: { + deleteTargets: readonly Worktree[] + deleteStateByWorktreeId: Record + gitStatusByWorktree: Record + repoMap: ReadonlyMap +}): Map { + const result = new Map() + for (const item of deleteTargets) { + if (item.isMainWorktree || isFolderWorkspaceDelete(repoMap, item)) { + continue + } + const forceDeleteReason = deleteStateByWorktreeId[item.id]?.forceDeleteReason + const changeCount = gitStatusByWorktree[item.id]?.length + if ((changeCount ?? 0) > 0) { + result.set(item.id, changeCount ?? 0) + } else if (forceDeleteReason === 'dirty') { + // Why: Git proved the worktree dirty even when renderer status has not + // loaded; keep the warning visible without inventing a file count. + result.set(item.id, 0) + } + } + return result +} diff --git a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx index 2d18316f9bd..fe2350f38f3 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx +++ b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx @@ -55,6 +55,7 @@ describe('showDeleteWorktreeFailureToast', () => { showDeleteWorktreeFailureToast({ error: 'branch has changes', canForceDelete: true, + forceDeleteReason: 'dirty', onViewChanges, onForceDelete, worktreeId: 'wt-1', @@ -93,6 +94,7 @@ describe('showDeleteWorktreeFailureToast', () => { showDeleteWorktreeFailureToast({ error: 'permission denied', canForceDelete: false, + forceDeleteReason: null, onViewChanges, onForceDelete: vi.fn(), worktreeId: 'wt-2', @@ -116,4 +118,45 @@ describe('showDeleteWorktreeFailureToast', () => { expect(toast.dismiss).toHaveBeenCalledWith('delete-worktree-failure:wt-2') expect(onViewChanges).toHaveBeenCalled() }) + + it('offers neither force delete nor View for a locked workspace', () => { + const onViewChanges = vi.fn() + + showDeleteWorktreeFailureToast({ + error: 'Worktree is locked by Git.', + canForceDelete: false, + forceDeleteReason: null, + onViewChanges, + onForceDelete: vi.fn(), + worktreeId: 'wt-locked', + worktreeName: 'feature/locked' + }) + + const body = renderToastBody('info') + expect(body.textContent).toContain('This workspace is locked by Git.') + expect(body.textContent).toContain( + 'Run git worktree unlock from its repository, then retry deletion' + ) + expect(body.textContent).not.toContain('Force Delete') + expect(body.textContent).not.toContain('View') + expect(onViewChanges).not.toHaveBeenCalled() + }) + + it('keeps View for a locked workspace when changed files are known', () => { + showDeleteWorktreeFailureToast({ + error: 'Worktree is locked by Git.', + canForceDelete: false, + forceDeleteReason: null, + lockReason: 'active agent session', + hasKnownChanges: true, + onViewChanges: vi.fn(), + onForceDelete: vi.fn(), + worktreeId: 'wt-locked-dirty', + worktreeName: 'feature/locked-dirty' + }) + + const body = renderToastBody('info') + expect(body.textContent).toContain('Git reported: active agent session') + expect(body.textContent).toContain('View') + }) }) diff --git a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx index ab073fe1372..bb9649f28f3 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx +++ b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx @@ -2,10 +2,17 @@ import { toast } from 'sonner' import { Button } from '../ui/button' import { getDeleteWorktreeToastCopy } from './delete-worktree-toast' import { translate } from '@/i18n/i18n' +import { + isLockedWorktreeRemovalError, + type WorktreeForceDeleteReason +} from '../../../../shared/worktree-removal' type DeleteWorktreeFailureToastOptions = { error: string canForceDelete: boolean + forceDeleteReason: WorktreeForceDeleteReason | null + lockReason?: string | null + hasKnownChanges?: boolean onViewChanges: () => void onForceDelete: () => void worktreeId: string @@ -19,12 +26,14 @@ function deleteWorktreeFailureToastId(worktreeId: string): string { function DeleteWorktreeFailureToastBody({ description, canForceDelete, + showViewChanges, onViewChanges, onForceDelete, toastId }: { description?: string canForceDelete: boolean + showViewChanges: boolean onViewChanges: () => void onForceDelete: () => void toastId: string @@ -44,9 +53,11 @@ function DeleteWorktreeFailureToastBody({

{description}

) : null}
- + {showViewChanges ? ( + + ) : null} {canForceDelete ? (