mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 16:02:24 +00:00
fix(worktree): refuse a removal the execution host cannot vouch for
Review of the home guard found three ways it still let a delete proceed on evidence about the wrong machine, or on no evidence at all. `getPathOps` switches to win32 as soon as EITHER the worktree path or the repo path looks Windows-absolute, and `//nas/share/repo` does. A POSIX worktree path was then judged by Windows-only shape rules, which recognise `<root>\Users\<name>` and nothing else, so `/home/alice` — and any client home outside `\Users` — stopped matching and the last guard in front of a recursive delete went quiet. The home question involves the worktree path and a home, never the repo path, so the predicate now reads the path in its own syntax as well and refuses if either reading names a home. A union of refusals can only ever refuse more. An execution host that never reported its `$HOME` is `unverifiable`, and `unverifiable` does not authorise a delete. `isRemovalHomeAuthorityResolved` gates the two paths that recursively delete a directory — `canSafelyRemoveOrphanedWorktreeDirectory` and `canCleanupUnregisteredOrcaLeftoverDirectory` — because the orphan proof they accept, a `.git` file at the top of a directory, is also what a bare-repo dotfiles `$HOME` looks like, and there the guard is the only evidence there is. `git worktree remove` is deliberately not gated: the 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 a first one. An empty `$HOME` is normalised to unanswered rather than read as a resolved home. The IPC entry point spelled its host two ways. The metadata prune, the archive-hook route and now the home authority came from `getRepoExecutionHostId(repo)`, while the `git worktree list` and every delete came from raw `repo.connectionId`. A row carrying only `executionHostId: 'ssh:<target>'` therefore listed a remote checkout on this client and deleted a same-named local path while the guards vouched for the remote one; the mirror row did the reverse (#11163, previously fixed on the runtime path only). Neither spelling is evidence about the other, so a row that carries two host names is refused before anything is listed or deleted. Both sides are spelled by `getRepoExecutionHostId`, so they can differ on content but never on normalisation. A `runtime:<env>` row refuses here for the same reason. It is not reachable through this handler today — the renderer sends environment targets to `worktree.rm`, and the host-qualified catalog refuses to list a runtime host — so that arm closes a door rather than changing a flow. Fixtures that register an SSH provider now report a host home, because a connected relay session always has one: `remoteCliBridgeEnv` is assigned before `registerSshGitProvider`, is never cleared, and providers are unregistered before the session leaves `activeSessions`. The wiring lives in its own module called from the harness rather than in `worktrees-test-module-mocks`, which `vi.mock` factories import: reaching the production route module from there pulls in `providers/ssh-git-dispatch` while it is being mocked, and the module runner deadlocks.
This commit is contained in:
@@ -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',
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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:<target>'` 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(
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<RemoveWorktreeResult> {
|
||||
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({
|
||||
|
||||
+5
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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:<target>'` 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:<env>` 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
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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:<target>'` 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:<env>` 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. */
|
||||
|
||||
@@ -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<typeof isHomeDirectoryRemovalPath>[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<typeof isHomeDirectoryRemovalPath>[2]
|
||||
): boolean {
|
||||
return isHomeDirectoryRemovalPath(worktreePath, getPathOps(worktreePath, repoPath), home)
|
||||
}
|
||||
|
||||
function withProcessPlatform<T>(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
|
||||
// `<root>\\Users\\<name>` 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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<boolean> {
|
||||
// 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)
|
||||
|
||||
Reference in New Issue
Block a user