diff --git a/src/main/ipc/worktrees-remove-host-disambiguation.test.ts b/src/main/ipc/worktrees-remove-host-disambiguation.test.ts index 289609a2b86..d456f1051e8 100644 --- a/src/main/ipc/worktrees-remove-host-disambiguation.test.ts +++ b/src/main/ipc/worktrees-remove-host-disambiguation.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { removeWorktreeMock, + listWorktreesMock, parseOrcaYamlMock, hasHooksFileMock, getSshGitProviderMock, @@ -144,6 +145,66 @@ describe('registerWorktreeHandlers', () => { expect(removeWorktreeMock).not.toHaveBeenCalled() }) + it('refuses a row whose execution host and connection id name different machines', async () => { + // #11163: everything below the handler picks the filesystem from `repo.connectionId` while the + // prune, the archive-hook route and the home guard come from the resolved execution host. This + // row would have listed a remote checkout on this client and deleted a same-named local path. + const brokenRepo = { + id: 'repo-host-only', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: null, + executionHostId: 'ssh:conn-1' as const + } + const provider = { + listWorktrees: vi.fn(), + removeWorktree: vi.fn(), + worktreeIsClean: vi.fn() + } + store.getRepo.mockReturnValue(brokenRepo) + store.getRepos.mockReturnValue([brokenRepo]) + getSshGitProviderMock.mockReturnValue(provider) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-host-only::/remote/feature-wt', + force: true + }) + ).rejects.toThrow( + 'Refusing to delete worktree: repo repo-host-only names execution host ssh:conn-1, but its checkout is only reachable as local.' + ) + + expect(listWorktreesMock).not.toHaveBeenCalled() + expect(provider.listWorktrees).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + + it('refuses the mirror row that names local while carrying a connection id', async () => { + const brokenRepo = { + id: 'repo-local-spelled', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1', + executionHostId: 'local' as const + } + store.getRepo.mockReturnValue(brokenRepo) + store.getRepos.mockReturnValue([brokenRepo]) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-local-spelled::/remote/feature-wt', + force: true + }) + ).rejects.toThrow( + 'Refusing to delete worktree: repo repo-local-spelled names execution host local, but its checkout is only reachable as ssh:conn-1.' + ) + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + it('tears down the remote session when an ownerless remote worktree is deleted', async () => { const sshRepo = { id: 'repo-1', diff --git a/src/main/ipc/worktrees-test-harness.ts b/src/main/ipc/worktrees-test-harness.ts index f438df48e3d..8297c695374 100644 --- a/src/main/ipc/worktrees-test-harness.ts +++ b/src/main/ipc/worktrees-test-harness.ts @@ -11,6 +11,7 @@ import { resetSshProviderAuthorities } from '../ssh/ssh-provider-authority' import { createWorktreeRuntimeStub, type WorktreeRuntimeStub } from './worktrees-test-runtime-stub' import { handlers, mainWindow, store } from './worktrees-test-ipc-surface' import { configureMetadataPruningStoreMocks } from './worktrees-test-metadata-pruning-store' +import { resetWorktreeTestSshHostHome } from './worktrees-test-ssh-host-home' import { ORIGINAL_PLATFORM, setPlatform, @@ -87,6 +88,7 @@ export const harnessRepo = { /** Registers worktree IPC handlers against freshly reset shared mocks and returns the runtime stub. */ export function setupWorktreeHandlers(): WorktreeRuntimeStub { + resetWorktreeTestSshHostHome() delete (store as typeof store & { getAllWorktreeMetaForHost?: (...args: unknown[]) => unknown }) .getAllWorktreeMetaForHost setPlatform(ORIGINAL_PLATFORM) diff --git a/src/main/ipc/worktrees-test-ssh-host-home.ts b/src/main/ipc/worktrees-test-ssh-host-home.ts new file mode 100644 index 00000000000..a373afe0e51 --- /dev/null +++ b/src/main/ipc/worktrees-test-ssh-host-home.ts @@ -0,0 +1,18 @@ +import { setWorktreeRemovalSshHostHomeResolver } from '../worktree-removal-execution-host-route' + +/** The `$HOME` the worktree IPC suites' SSH hosts report. */ +export const TEST_SSH_HOST_HOME = '/home/remote-user' + +/** + * Makes the harness's SSH hosts answer the removal guards' home question. + * + * Every suite that registers an SSH provider is modelling a connected relay session, and a + * connected session has always read the host's `$HOME`. Without it the guards refuse the recursive + * orphan delete — the right answer for a host that never answered, the wrong fixture for one that + * did. Deliberately not wired from `worktrees-test-module-mocks`: that module is imported from + * `vi.mock` factories, and reaching the production route module from there pulls in + * `providers/ssh-git-dispatch` while it is being mocked, which deadlocks the module runner. + */ +export function resetWorktreeTestSshHostHome(): void { + setWorktreeRemovalSshHostHomeResolver(() => TEST_SSH_HOST_HOME) +} diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index 4d91e0b57d1..6da1354f918 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -1,5 +1,5 @@ import type { Repo } from '../../../../shared/repo-types' -import type { ExecutionHostId } from '../../../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' import { isFolderRepo } from '../../../../shared/repo-kind' import { assertWorktreeUnlockedForRemoval } from '../../../../shared/worktree/removal' @@ -11,7 +11,7 @@ import { resolveWorktreeRemovalMetadata } from '../../../worktree-removal-repo-o import { isPrunableGitFileWorktree } from '../../../worktree-prunable-git-file' import { findRegisteredDeletableWorktree } from '../../../worktree-removal-safety' import { removeStaleLocalWorktreeRegistration } from '../../../local-worktree-removal-recovery' -import { resolveWorktreeRemovalHomeForConnection } from '../../../worktree-removal-execution-host-route' +import { resolveWorktreeRemovalHomeForHost } from '../../../worktree-removal-execution-host-route' import { runHook } from '../../../hooks' import type { ArchiveHookOverride } from '../../../../shared/worktree/archive-hook-removal-gate' import { gateWorktreeRemovalOnArchiveHook } from '../../../worktree-archive-hook-gate' @@ -33,6 +33,35 @@ import { removeUnregisteredWorktree } from './remove-unregistered-worktree' import { removeRegisteredRemoteWorktree } from './remove-registered-remote-worktree' import { removeRegisteredLocalWorktree } from './remove-registered-local-worktree' +/** + * Refuses a repo row whose two host spellings disagree. + * + * Everything below picks the filesystem it deletes on from `repo.connectionId`, while the metadata + * prune, the archive-hook route and the home authority all come from `removalHostId`. A row naming + * `executionHostId: 'ssh:'` with no `connectionId` therefore lists and deletes a same-named + * path on THIS machine while the guards vouch for the remote one, and the reverse row does the + * mirror image (#11163). Neither spelling is evidence about the other, so refuse instead of picking + * a winner: the worktree is left in place, which is the recoverable outcome + * (docs/reference/ssh-execution-boundary.md). + */ +function assertRemovalHostMatchesRepoRow( + repo: Repo, + repoId: string, + removalHostId: ExecutionHostId +): void { + // Same function `removalHostId` came from, with the row's own `executionHostId` withheld: the two + // spellings then differ only when the row really carries two host names, never on normalisation. + const repoRowHostId = getRepoExecutionHostId({ + connectionId: repo.connectionId, + executionHostId: null + }) + if (removalHostId !== repoRowHostId) { + throw new Error( + `Refusing to delete worktree: repo ${repoId} names execution host ${removalHostId}, but its checkout is only reachable as ${repoRowHostId}.` + ) + } +} + export async function executeWorktreeRemoval( context: WorktreeIpcContext, args: RemoveWorktreeArgs, @@ -45,6 +74,7 @@ export async function executeWorktreeRemoval( if (isFolderRepo(repo)) { return removeFolderWorkspace(context, args, repo, repoId, removalHostId) } + assertRemovalHostMatchesRepoRow(repo, repoId, removalHostId) const provider = repo.connectionId ? requireSshGitProvider(repo.connectionId) : null const localWorktreeGitOptions = repo.connectionId ? {} @@ -61,7 +91,7 @@ export async function executeWorktreeRemoval( repo.path, worktreePath, registeredWorktrees, - resolveWorktreeRemovalHomeForConnection(repo.connectionId) + resolveWorktreeRemovalHomeForHost(removalHostId) ) if (!registeredWorktree) { return removeUnregisteredWorktree( diff --git a/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts b/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts index 790623d539a..9d88143d3c4 100644 --- a/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts +++ b/src/main/ipc/worktrees/removal/remove-unregistered-worktree-host-home.test.ts @@ -115,4 +115,17 @@ describe('removeUnregisteredWorktree against an SSH host home', () => { expect(fsProvider.deletePath).toHaveBeenCalledWith(worktreePath, true) }) + + it('refuses a proven orphan when the host never reported a home', async () => { + // The orphan proof is complete and the path looks ordinary; the only thing missing is the + // host's answer. `unverifiable` leaves the directory in place rather than deleting it. + setWorktreeRemovalSshHostHomeResolver(() => null) + const worktreePath = `${HOST_HOME}/workspaces/leftover` + const fsProvider = provenOrphanFilesystem(worktreePath) + + await expect(removeOverSsh(worktreePath, fsProvider)).rejects.toThrow( + `Refusing to delete unregistered worktree path: ${worktreePath}` + ) + expect(fsProvider.deletePath).not.toHaveBeenCalled() + }) }) diff --git a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts index dc1b99a6d77..7eec3879080 100644 --- a/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts @@ -17,7 +17,7 @@ import { ORPHANED_WORKTREE_DIRECTORY_MESSAGE, UNREGISTERED_MISSING_WORKTREE_MESSAGE } from '../../../worktree-removal-safety' -import { resolveWorktreeRemovalHomeForConnection } from '../../../worktree-removal-execution-host-route' +import { resolveWorktreeRemovalHomeForHost } from '../../../worktree-removal-execution-host-route' import { getLocalWorktreePathAccess, removeLocalWorktreePath, @@ -54,7 +54,7 @@ export async function removeUnregisteredWorktree( ): Promise { const { mainWindow, store, runtime } = context const fsProvider = repo.connectionId ? getSshFilesystemProvider(repo.connectionId) : null - const removalHome = resolveWorktreeRemovalHomeForConnection(repo.connectionId) + const removalHome = resolveWorktreeRemovalHomeForHost(removalHostId) let canCleanOrphanedDirectory = false if ( canCleanupUnregisteredOrcaWorktreeDirectory({ diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts index 9006502ad73..941579fe645 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts @@ -24,6 +24,7 @@ import { writeFile } from '../orca-runtime-test-mocks.spec' import type { WorktreeMeta } from '../orca-runtime-test-mocks.spec' +import { setWorktreeRemovalSshHostHomeResolver } from '../../worktree-removal-execution-host-route' import { TEST_REPO_ID, TEST_REPO_PATH, @@ -462,6 +463,9 @@ describe('OrcaRuntimeService', () => { } registerSshGitProvider(repo.connectionId, gitProvider as never) registerSshFilesystemProvider(repo.connectionId, fsProvider as never) + // Why: the orphan-directory gate is a recursive delete, so it refuses until the host names its + // own home. A connected relay session always has, which is what this fixture stands for. + setWorktreeRemovalSshHostHomeResolver(() => '/home/remote-user') const runtime = new OrcaRuntimeService(runtimeStore as never, undefined, { getSshProvider: () => ptyProvider as never }) @@ -471,6 +475,7 @@ describe('OrcaRuntimeService', () => { runtime.removeManagedWorktree(`id:${worktreeId}`, { force: true }) ).resolves.toEqual({}) } finally { + setWorktreeRemovalSshHostHomeResolver(() => null) unregisterSshGitProvider(repo.connectionId) unregisterSshFilesystemProvider(repo.connectionId) } diff --git a/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts b/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts index b2e2f542369..0f57ed51cd2 100644 --- a/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts +++ b/src/main/runtime/runtime-unregistered-worktree-removal-host-home.test.ts @@ -92,4 +92,16 @@ describe('removeRuntimeUnregisteredWorktree against an SSH host home', () => { expect(fsProvider.deletePath).toHaveBeenCalledWith(worktreePath, true) }) + + it('refuses a proven orphan when the host never reported a home', async () => { + // Same orphan, same proof; the host just never answered. Loss of contact is not permission. + setWorktreeRemovalSshHostHomeResolver(() => null) + const worktreePath = `${HOST_HOME}/workspaces/leftover` + const fsProvider = provenOrphanFilesystem(worktreePath) + + await expect( + removeRuntimeUnregisteredWorktree(removalArgs(worktreePath, fsProvider)) + ).rejects.toThrow(`Refusing to delete unregistered worktree path: ${worktreePath}`) + expect(fsProvider.deletePath).not.toHaveBeenCalled() + }) }) diff --git a/src/main/worktree-removal-execution-host-route.test.ts b/src/main/worktree-removal-execution-host-route.test.ts index adb0caca15e..a9b7db8f060 100644 --- a/src/main/worktree-removal-execution-host-route.test.ts +++ b/src/main/worktree-removal-execution-host-route.test.ts @@ -12,6 +12,7 @@ import { ExecutionHostNotDispatchableError } from './providers/execution-host-pr import { getWorktreeRemovalConnectionId, resolveWorktreeRemovalHome, + resolveWorktreeRemovalHomeForHost, resolveWorktreeRemovalRoute, setWorktreeRemovalSshHostHomeResolver } from './worktree-removal-execution-host-route' @@ -129,3 +130,40 @@ describe('resolveWorktreeRemovalHome', () => { }) }) }) + +describe('resolveWorktreeRemovalHomeForHost', () => { + it('answers an ssh host id without needing a registered provider', () => { + // The IPC entry point resolves the home before it has a route, and a row naming its owner only + // as `executionHostId: 'ssh:'` has no `connectionId` to key on at all. + setWorktreeRemovalSshHostHomeResolver((id) => (id === HOST_A ? '/srv/homes/alice' : null)) + + expect(resolveWorktreeRemovalHomeForHost('ssh:target-a')).toEqual({ + kind: 'executionHost', + homePath: '/srv/homes/alice' + }) + expect(resolveWorktreeRemovalHomeForHost('ssh:target-b')).toEqual({ + kind: 'executionHost', + homePath: null + }) + }) + + it('keeps the client home for the local host', () => { + expect(resolveWorktreeRemovalHomeForHost('local')).toEqual({ kind: 'client' }) + }) + + it('refuses to answer a runtime host with this client s home', () => { + // `runtime:` deletes on that environment's own server; this client's home vouches for + // nothing there, so the authority stays unknown and the guard refuses. + expect(resolveWorktreeRemovalHomeForHost('runtime:env-1')).toEqual({ + kind: 'executionHost', + homePath: null + }) + }) + + it('refuses to answer an id that names no host', () => { + expect(resolveWorktreeRemovalHomeForHost('nonsense' as never)).toEqual({ + kind: 'executionHost', + homePath: null + }) + }) +}) diff --git a/src/main/worktree-removal-execution-host-route.ts b/src/main/worktree-removal-execution-host-route.ts index 2ad0a0aa211..b57d92327b9 100644 --- a/src/main/worktree-removal-execution-host-route.ts +++ b/src/main/worktree-removal-execution-host-route.ts @@ -27,7 +27,11 @@ * worktree in place, while the incumbent fallback deleted a client-side path. */ -import type { ExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' +import { + parseExecutionHostId, + type ExecutionHostId, + type LOCAL_EXECUTION_HOST_ID +} from '../shared/execution-host' import { CLIENT_REMOVAL_HOME, executionHostRemovalHome, @@ -98,23 +102,36 @@ export function setWorktreeRemovalSshHostHomeResolver( /** * Whose home directory the removal's safety guards may consult — one answer for - * the whole removal, taken from the same route that owns the filesystem. + * the whole removal, taken from the same host id that owns the filesystem. */ export function resolveWorktreeRemovalHome( route: WorktreeRemovalRoute ): WorktreeRemovalHomeAuthority { - return resolveWorktreeRemovalHomeForConnection( - route.kind === 'ssh' ? route.connectionId : undefined - ) + return resolveWorktreeRemovalHomeForHost(route.hostId) } -/** The same answer for the callers that still carry `repo.connectionId` instead of a route. */ -export function resolveWorktreeRemovalHomeForConnection( - connectionId: string | null | undefined +/** + * The same answer for the entry points that hold a host id rather than a route. + * + * Keyed on the resolved `ExecutionHostId`, not on `repo.connectionId`: a row naming its owner only + * as `executionHostId: 'ssh:'` has a null `connectionId`, and answering that with this + * client's home is how the guard would vouch for the wrong machine (#11163). + */ +export function resolveWorktreeRemovalHomeForHost( + hostId: ExecutionHostId ): WorktreeRemovalHomeAuthority { - return connectionId - ? executionHostRemovalHome(sshHostHomeResolver(connectionId)) - : CLIENT_REMOVAL_HOME + const parsed = parseExecutionHostId(hostId) + switch (parsed?.kind) { + case 'local': + return CLIENT_REMOVAL_HOME + case 'ssh': + return executionHostRemovalHome(sshHostHomeResolver(parsed.targetId)) + default: + // Why: `runtime:` deletes on that environment's own server, and an id that parses to + // nothing names no machine at all. Neither can be answered with this client's home, so both + // stay unknown and the guard refuses. + return executionHostRemovalHome(null) + } } /** The connection to teardown PTYs, watchers and history against — `undefined` on a local host. */ diff --git a/src/main/worktree-removal-home-guard.test.ts b/src/main/worktree-removal-home-guard.test.ts index a562bb3c448..d06940acdd3 100644 --- a/src/main/worktree-removal-home-guard.test.ts +++ b/src/main/worktree-removal-home-guard.test.ts @@ -8,15 +8,28 @@ vi.mock('node:os', async (importOriginal) => { return { ...actual, homedir: homedirMock } }) -const { CLIENT_REMOVAL_HOME, executionHostRemovalHome, getPathOps, isHomeDirectoryRemovalPath } = - await import('./worktree-removal-home-guard') +const { + CLIENT_REMOVAL_HOME, + executionHostRemovalHome, + getPathOps, + isHomeDirectoryRemovalPath, + isRemovalHomeAuthorityResolved +} = await import('./worktree-removal-home-guard') function isHome( worktreePath: string, home: Parameters[2] ): boolean { - const pathOps = getPathOps(worktreePath) - return isHomeDirectoryRemovalPath(pathOps.resolve(worktreePath), pathOps, home) + return isHomeDirectoryRemovalPath(worktreePath, getPathOps(worktreePath), home) +} + +/** The ops a removal actually gets: chosen from the worktree/repo pair, not the path alone. */ +function isHomeForPair( + worktreePath: string, + repoPath: string, + home: Parameters[2] +): boolean { + return isHomeDirectoryRemovalPath(worktreePath, getPathOps(worktreePath, repoPath), home) } function withProcessPlatform(platform: NodeJS.Platform, callback: () => T): T { @@ -102,16 +115,30 @@ describe('whose home the guard consults', () => { // Without the host's answer the same path has no recognisable home shape, // which is exactly why the client home must not stand in for it. expect(isHome('/srv/homes/alice', CLIENT_REMOVAL_HOME)).toBe(false) - expect(isHome('/srv/homes/alice', executionHostRemovalHome(null))).toBe(false) }) - it('never lets an unknown execution-host home fall back to the client homedir', () => { - // The client's home coincides with the remote path here; `null` still means unknown. + it('reports an unanswered execution host as unresolved, never as this client s home', () => { + // `null` is `unverifiable`. The client's home coincides with the remote path here, and must + // still not be the thing that answers — the shape rules are all that is left. homedirMock.mockReturnValue('/srv/homes/alice') + expect(isRemovalHomeAuthorityResolved(executionHostRemovalHome(null))).toBe(false) expect(isHome('/srv/homes/alice', executionHostRemovalHome(null))).toBe(false) + expect(isHome('/home/alice', executionHostRemovalHome(null))).toBe(true) expect(homedirMock).not.toHaveBeenCalled() }) + it('treats an empty execution-host home as unknown rather than as a resolved answer', () => { + // An empty `$HOME` is an absent answer; normalising it here keeps the authority type honest + // instead of leaving `''` to read as "resolved" at every consumer. + expect(executionHostRemovalHome('')).toEqual({ kind: 'executionHost', homePath: null }) + expect(isRemovalHomeAuthorityResolved(executionHostRemovalHome(''))).toBe(false) + }) + + it('treats the client and an answering host as resolved', () => { + expect(isRemovalHomeAuthorityResolved(CLIENT_REMOVAL_HOME)).toBe(true) + expect(isRemovalHomeAuthorityResolved(executionHostRemovalHome('/srv/homes/alice'))).toBe(true) + }) + it('honours a Windows execution-host home in the forward-slash form the relay reports', () => { // `normalizeRemoteHome` folds a Windows host's `$HOME` to `C:/Users/bob`, not `C:\Users\bob`. const hostHome = executionHostRemovalHome('C:/Users/bob/OneDrive') @@ -145,3 +172,50 @@ describe('whose home the guard consults', () => { ).toBe(true) }) }) + +describe('path ops chosen from the worktree/repo pair', () => { + // `getPathOps` switches to win32 as soon as EITHER path looks Windows-absolute, and `//nas/...` + // does. A POSIX worktree path then gets judged by Windows-only shape rules, which recognise + // `\\Users\\` and nothing else — so `/home/alice` and a non-standard client home + // both stopped being homes because of a path the home comparison never involved. + it('still recognises a POSIX home when the repo path drags the pair into win32 ops', () => { + homedirMock.mockReturnValue('/Users/ci') + expect(isHomeForPair('/home/alice', '//nas/share/repo', CLIENT_REMOVAL_HOME)).toBe(true) + expect(isHomeForPair('/home', '//nas/share/repo', CLIENT_REMOVAL_HOME)).toBe(true) + expect(isHomeForPair('/root', 'C:\\src\\repo', CLIENT_REMOVAL_HOME)).toBe(true) + }) + + it('still recognises the client home itself under the same contaminated ops', () => { + homedirMock.mockReturnValue('/srv/homes/ci') + expect( + withProcessPlatform('linux', () => + isHomeForPair('/srv/homes/ci', '//nas/share/repo', CLIENT_REMOVAL_HOME) + ) + ).toBe(true) + expect( + withProcessPlatform('linux', () => + isHomeForPair('/srv/homes/ci', 'C:\\src\\repo', CLIENT_REMOVAL_HOME) + ) + ).toBe(true) + }) + + it('still recognises an execution-host home under the same contaminated ops', () => { + expect( + isHomeForPair( + '/srv/homes/alice', + 'C:\\src\\repo', + executionHostRemovalHome('/srv/homes/alice') + ) + ).toBe(true) + }) + + it('keeps a linked worktree deletable when the pair is mixed-syntax', () => { + homedirMock.mockReturnValue('/srv/homes/ci') + expect( + withProcessPlatform('linux', () => + isHomeForPair('/srv/homes/ci/wt/feature', '//nas/share/repo', CLIENT_REMOVAL_HOME) + ) + ).toBe(false) + expect(isHomeForPair('/opt/src/checkout', '//nas/share/repo', CLIENT_REMOVAL_HOME)).toBe(false) + }) +}) diff --git a/src/main/worktree-removal-home-guard.ts b/src/main/worktree-removal-home-guard.ts index 85596093c13..d6c36fefb84 100644 --- a/src/main/worktree-removal-home-guard.ts +++ b/src/main/worktree-removal-home-guard.ts @@ -33,7 +33,23 @@ export const CLIENT_REMOVAL_HOME: WorktreeRemovalHomeAuthority = { kind: 'client export function executionHostRemovalHome( homePath: string | null | undefined ): WorktreeRemovalHomeAuthority { - return { kind: 'executionHost', homePath: homePath ?? null } + // Why `||`: an empty answer is an absent one, and `''` would otherwise read as a resolved home. + return { kind: 'executionHost', homePath: homePath || null } +} + +/** + * Whether the host that executes the removal actually named its home directory. + * + * `false` is `unverifiable`, not "no home here" (docs/reference/ssh-execution-boundary.md). The + * recursive-directory gates in `worktree-removal-safety.ts` require `true`, because there the home + * guard is the only evidence standing between an `rm -rf` and somebody's `$HOME` — a bare-repo + * dotfiles checkout puts a real `.git` file at the top of a home directory, which is exactly the + * orphan proof those gates accept. `git worktree remove` does not require it: the execution host's + * own Git registry already established that the path is a linked worktree of that repo, and a + * missing second opinion does not retract that. + */ +export function isRemovalHomeAuthorityResolved(home: WorktreeRemovalHomeAuthority): boolean { + return home.kind === 'client' || !!home.homePath } export function getPathOps(...paths: string[]): PathOps { @@ -54,16 +70,34 @@ export function containsPath(parentPath: string, childPath: string, pathOps: Pat } /** - * Whether removing `resolvedWorktreePath` would take a home directory with it. + * Whether removing `worktreePath` would take a home directory with it. * * True when the path is, or contains, the home of the machine that executes the - * removal, or when its shape is a home directory on the filesystem it names. + * removal, or when its shape is a home directory on the filesystem it names. An + * execution host that never reported a home answers neither — see + * `isRemovalHomeAuthorityResolved` for who has to insist on an answer. */ export function isHomeDirectoryRemovalPath( - resolvedWorktreePath: string, + worktreePath: string, pathOps: PathOps, home: WorktreeRemovalHomeAuthority ): boolean { + if (isHomeUnderPathOps(worktreePath, pathOps, home)) { + return true + } + // Why: `pathOps` is picked from the worktree/repo PAIR, so a Windows-shaped repo path drags a + // POSIX worktree path into win32 rules and `/home/alice` stops matching anything. Read the path + // in its own syntax as well, and refuse if either reading names a home. + const ownPathOps = getPathOps(worktreePath) + return ownPathOps !== pathOps && isHomeUnderPathOps(worktreePath, ownPathOps, home) +} + +function isHomeUnderPathOps( + worktreePath: string, + pathOps: PathOps, + home: WorktreeRemovalHomeAuthority +): boolean { + const resolvedWorktreePath = pathOps.resolve(worktreePath) const homePath = resolveGuardHomePath(home, pathOps) if (!!homePath && containsPath(resolvedWorktreePath, pathOps.resolve(homePath), pathOps)) { return true diff --git a/src/main/worktree-removal-safety.test.ts b/src/main/worktree-removal-safety.test.ts index 91859a3b423..5aae44274f9 100644 --- a/src/main/worktree-removal-safety.test.ts +++ b/src/main/worktree-removal-safety.test.ts @@ -553,8 +553,10 @@ describe('isDangerousWorktreeRemovalPath on an execution host', () => { ['C:\\Users', 'C:\\src\\repo', true], ['C:\\Users\\bob\\wt\\foo', 'C:\\src\\repo', false] ])('%s under %s -> dangerous=%s', (worktreePath, repoPath, expected) => { + // `/var/empty` is a resolved host home that matches no row, so each verdict comes from the + // path rules alone — the same verdicts the client authority reaches. expect( - isDangerousWorktreeRemovalPath(worktreePath, repoPath, executionHostRemovalHome(null)) + isDangerousWorktreeRemovalPath(worktreePath, repoPath, executionHostRemovalHome('/var/empty')) ).toBe(expected) expect(isDangerousWorktreeRemovalPath(worktreePath, repoPath, CLIENT_REMOVAL_HOME)).toBe( expected @@ -570,6 +572,32 @@ describe('isDangerousWorktreeRemovalPath on an execution host', () => { ) ).toBe(true) }) + + it('recognises a POSIX home when the repo path drags the pair into win32 path ops', () => { + // `getPathOps` reads both paths, so a `//`-rooted repo path put `/home/alice` under + // Windows-only shape rules and the last guard on a recursive delete stopped matching. + expect( + isDangerousWorktreeRemovalPath( + '/home/alice', + '//nas/share/repo', + executionHostRemovalHome('/var/empty') + ) + ).toBe(true) + expect( + isDangerousWorktreeRemovalPath( + '/srv/homes/alice', + '//nas/share/repo', + executionHostRemovalHome('/srv/homes/alice') + ) + ).toBe(true) + expect( + isDangerousWorktreeRemovalPath( + '/srv/homes/alice/wt/feature', + '//nas/share/repo', + executionHostRemovalHome('/srv/homes/alice') + ) + ).toBe(false) + }) }) describe('canSafelyRemoveOrphanedWorktreeDirectory on an execution host', () => { @@ -624,4 +652,63 @@ describe('canSafelyRemoveOrphanedWorktreeDirectory on an execution host', () => ) ).resolves.toBe(true) }) + + it('refuses a proven orphan of any shape while the host home is unanswered', async () => { + // A bare-repo dotfiles checkout puts exactly this `.git` file at the top of a home directory, + // and `/srv/homes/alice` has no home shape to fall back on. Unanswered is not permission. + const orphan = { + statPath: makeStatPath(['/srv/homes/alice/.git'], ['/opt/src/repo/.git']), + readPath: makeReadPath([ + ['/srv/homes/alice/.git', 'gitdir: /opt/src/repo/.git/worktrees/alice\n'], + ['/opt/src/repo/.git/worktrees/alice/gitdir', '/srv/homes/alice/.git\n'] + ]) + } + + await expect( + canSafelyRemoveOrphanedWorktreeDirectory( + '/srv/homes/alice', + '/opt/src/repo', + executionHostRemovalHome(null), + orphan.statPath, + orphan.readPath + ) + ).resolves.toBe(false) + // The same call with an answer that does not match still removes it, so the refusal above is + // the missing answer and not the path. + await expect( + canSafelyRemoveOrphanedWorktreeDirectory( + '/srv/homes/alice', + '/opt/src/repo', + executionHostRemovalHome('/srv/homes/bob'), + orphan.statPath, + orphan.readPath + ) + ).resolves.toBe(true) + }) + + it('refuses the leftover-directory cleanup while the host home is unanswered', async () => { + const leftoverArgs = { + meta: { orcaCreatedAt: 1, orcaCreationSource: 'ssh' } as never, + worktreePath: '/srv/homes/alice', + runtimeWorktreePath: '/srv/homes/alice', + repo: { path: '/opt/src/repo' }, + runtimeRepoPath: '/opt/src/repo', + registeredWorktrees: [], + statPath: makeStatPath([], ['/srv/homes/alice']), + isGitRepository: vi.fn().mockResolvedValue(false) + } + + await expect( + canCleanupUnregisteredOrcaLeftoverDirectory({ + ...leftoverArgs, + home: executionHostRemovalHome(null) + }) + ).resolves.toBe(false) + await expect( + canCleanupUnregisteredOrcaLeftoverDirectory({ + ...leftoverArgs, + home: executionHostRemovalHome('/srv/homes/bob') + }) + ).resolves.toBe(true) + }) }) diff --git a/src/main/worktree-removal-safety.ts b/src/main/worktree-removal-safety.ts index 129a44cfbef..c26d3fd0bf5 100644 --- a/src/main/worktree-removal-safety.ts +++ b/src/main/worktree-removal-safety.ts @@ -7,6 +7,7 @@ import { containsPath, getPathOps, isHomeDirectoryRemovalPath, + isRemovalHomeAuthorityResolved, type WorktreeRemovalHomeAuthority } from './worktree-removal-home-guard' import { @@ -75,7 +76,8 @@ export function isDangerousWorktreeRemovalPath( return true } - return isHomeDirectoryRemovalPath(resolvedWorktreePath, pathOps, home) + // Raw, not `resolvedWorktreePath`: the guard re-reads the path under its own syntax too. + return isHomeDirectoryRemovalPath(worktreePath, pathOps, home) } export function getRegisteredDeletableWorktree( @@ -135,6 +137,14 @@ export async function canSafelyRemoveOrphanedWorktreeDirectory( statPath: StatPath = lstat, readPath: ReadPath = (path) => readFile(path, 'utf8') ): Promise { + // Why: this answer authorises a recursive delete, and the proof it relies on — a `.git` file at + // the top of the directory — is also what a bare-repo dotfiles home looks like. An execution host + // that never named its home leaves that check with nothing to compare against, and + // `unverifiable` does not authorise a delete (docs/reference/ssh-execution-boundary.md). + if (!isRemovalHomeAuthorityResolved(home)) { + return false + } + if (isDangerousWorktreeRemovalPath(worktreePath, repoPath, home)) { return false } @@ -186,6 +196,11 @@ export async function canCleanupUnregisteredOrcaLeftoverDirectory(args: { return false } + // Why: same recursive delete, same rule — no home answer from the executing host, no delete. + if (!isRemovalHomeAuthorityResolved(args.home)) { + return false + } + if ( isDangerousWorktreeRemovalPath(args.worktreePath, args.repo.path, args.home) || isDangerousWorktreeRemovalPath(args.runtimeWorktreePath, args.runtimeRepoPath, args.home)