From c2022351fbd69f5baa642c93cc2f3c4c04ae6d4e Mon Sep 17 00:00:00 2001 From: Neil Date: Mon, 14 Sep 2026 05:23:16 -0700 Subject: [PATCH] fix(worktrees): recover malformed prunable git-file registrations Use registration-only cleanup for proven named-branch .git regular-file rows before archive or checkout teardown. Keep all checkout files and the branch; document prune scope, dangling marker, older Git refusal and SSH boundaries. Addresses stablyai/orca#17316. --- ...malformed-worktree-registration-removal.md | 43 ++++++++++++ ...worktree-deferred-removal-real-git.test.ts | 35 +++++++++- .../ipc/worktrees-removal-recovery.test.ts | 25 +++++++ .../removal/execute-worktree-removal.ts | 16 +++-- .../local-worktree-removal-recovery.test.ts | 26 +++++-- src/main/local-worktree-removal-recovery.ts | 4 +- .../orca-runtime-remove-managed-worktree.ts | 26 +++---- src/main/worktree-prunable-git-file.test.ts | 70 +++++++++++++++++++ src/main/worktree-prunable-git-file.ts | 32 +++++++++ 9 files changed, 251 insertions(+), 26 deletions(-) create mode 100644 docs/reference/malformed-worktree-registration-removal.md create mode 100644 src/main/worktree-prunable-git-file.test.ts create mode 100644 src/main/worktree-prunable-git-file.ts diff --git a/docs/reference/malformed-worktree-registration-removal.md b/docs/reference/malformed-worktree-registration-removal.md new file mode 100644 index 00000000000..34a32e58caf --- /dev/null +++ b/docs/reference/malformed-worktree-registration-removal.md @@ -0,0 +1,43 @@ +# Malformed worktree registration removal + +Git can report a linked worktree at `/.git` when its administrative +`gitdir` backlink incorrectly ends in `.git/.git`. That reproduces #17316's +validation error. The reproduction establishes the malformed registration, not +which program created it; current OMP uses ordinary `git worktree add`. + +Orca's desktop and runtime removal entry points use registration-only recovery +when Git positively marks the row prunable, the row has a named local branch and +HEAD, it is neither main nor locked, and the execution filesystem confirms the +selected `.git` path is a regular file. Missing or unknown evidence does not +permit this recovery. A symlink or directory is not a regular-file proof. + +Recovery reuses `git worktree prune` followed by a strict worktree listing that +must confirm the selected registration is gone. It does not delete the selected +file, infer a parent path for deletion, or delete the branch. Archive hooks and +checkout teardown are skipped because the selected row is not a checkout. + +Two consequences are intentional: + +- Git's prune also clears other stale, unlocked registrations in the repository; + it is not a path-scoped command. Live and locked registrations remain Git's + responsibility, and Orca verifies that the requested registration disappeared. +- The surviving checkout's `.git` file points at removed administrative metadata. + Files and its named branch are preserved; recovery removes the broken navigation + entry and does not repair or claim to restore that checkout. + +Native and WSL checks use the existing execution-filesystem accessor. WSL prune +and verification use the same selected distro. Paired runtimes run the recovery +on their owning host. Direct SSH does not enter this local recovery: its current +provider has no registration-only removal operation, and a failed remote removal +never authorizes a local fallback. + +The Git commands already exist in the 2.25-compatible cleanup path. On an older +Git that cannot positively attest this file-shaped registration as prunable, Orca +refuses this recovery. Deferred deletion independently rejects non-directory and +symlink targets, so force cannot move a `.git` file into deletion trash. + +Regression coverage is in `worktree-prunable-git-file.test.ts`, +`worktrees-removal-recovery.test.ts`, and +`worktree-deferred-removal-real-git.test.ts`. The latter reproduces the exact +malformation against the installed Git binary in a disposable repository and +checks surviving file contents, branch HEAD, and removed registration. diff --git a/src/main/git/worktree-deferred-removal-real-git.test.ts b/src/main/git/worktree-deferred-removal-real-git.test.ts index 9366bebd88a..06b58b4319e 100644 --- a/src/main/git/worktree-deferred-removal-real-git.test.ts +++ b/src/main/git/worktree-deferred-removal-real-git.test.ts @@ -7,7 +7,9 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { removeWorktree } from './worktree' +import { listWorktreesStrict, removeWorktree } from './worktree' +import { isPrunableGitFileWorktree } from '../worktree-prunable-git-file' +import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery' import { getWorktreeTrashRoot, isWorktreeTrashEntryName, @@ -114,6 +116,37 @@ describe('deferred worktree removal against the real Git binary', () => { expect(existsSync(getWorktreeTrashRoot(markerPath))).toBe(false) }) + it('prunes a proven malformed registration while retaining checkout files and its branch', async () => { + const markerPath = join(worktreePath, '.git') + const marker = await readFile(markerPath, 'utf8') + const adminPath = marker.trim().replace(/^gitdir: /, '') + await writeFile(join(adminPath, 'gitdir'), `${join(markerPath, '.git')}\n`) + await writeFile(join(worktreePath, 'untracked.txt'), 'keep this work\n') + const row = (await listWorktreesStrict(repoPath)).find((entry) => entry.path === markerPath) + expect(row).toBeDefined() + if (!row) { + throw new Error('Missing malformed registration') + } + expect(await isPrunableGitFileWorktree(row)).toBe(true) + + const result = await removeStaleLocalWorktreeRegistration({ + canonicalWorktreePath: markerPath, + repoPath, + localWorktreeGitOptions: {}, + registeredWorktree: row, + deleteBranch: true + }) + + expect(result).toEqual({ preservedBranch: { branchName: 'feature', head: row.head } }) + expect(await readFile(markerPath, 'utf8')).toBe(marker) + expect(await readFile(join(worktreePath, 'untracked.txt'), 'utf8')).toBe('keep this work\n') + expect(await git(['rev-parse', 'refs/heads/feature'], repoPath)).toBe(`${row.head}\n`) + expect((await listWorktreesStrict(repoPath)).some((entry) => entry.path === markerPath)).toBe( + false + ) + expect(existsSync(adminPath)).toBe(false) + }) + it('sweeps trash a previous run left behind', async () => { const stalePath = join( workspaceRoot, diff --git a/src/main/ipc/worktrees-removal-recovery.test.ts b/src/main/ipc/worktrees-removal-recovery.test.ts index e8571449321..3c099b8ae6e 100644 --- a/src/main/ipc/worktrees-removal-recovery.test.ts +++ b/src/main/ipc/worktrees-removal-recovery.test.ts @@ -427,6 +427,31 @@ describe('registerWorktreeHandlers', () => { expect(store.removeWorktreeMeta).toHaveBeenCalledWith(worktreeId, 'local') }) + it('cleans a prunable Git-file row before archive or checkout teardown', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-prunable-ipc-')) + const markerPath = join(root, '.git') + await writeFile(markerPath, 'gitdir: /preserved/admin\n') + const worktreeId = `repo-1::${markerPath}` + const rows = mockKnownFeatureWorktree(markerPath).map((row) => + row.path === markerPath ? { ...row, branch: 'refs/heads/feature', prunable: true } : row + ) + listWorktreesMock.mockResolvedValueOnce(rows).mockResolvedValue([]) + try { + const result = await handlers['worktrees:remove'](null, { worktreeId }) + expect(result).toEqual({ preservedBranch: { branchName: 'feature', head: 'feature' } }) + expect(runHookMock).not.toHaveBeenCalled() + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { + cwd: '/workspace/repo' + }) + expect(store.removeWorktreeMeta).toHaveBeenCalledWith(worktreeId, 'local') + expect((await lstat(markerPath)).isFile()).toBe(true) + } finally { + await rm(root, { recursive: true, force: true }) + } + }) + it('preserves a locked missing registration even with force', async () => { setPlatform('win32') const missingWorktreePath = 'C:\\workspace\\locked-already-removed' diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index 6f0d91ffef5..443e05627ca 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -8,8 +8,9 @@ import { getLocalProjectWorktreeGitOptions } from '../../../project-runtime-git- import { listWorktreesStrict as listGitWorktreesStrict } from '../../../git/worktree' import { requireSshGitProvider } from '../../../providers/ssh-git-dispatch' import { resolveWorktreeRemovalMetadata } from '../../../worktree-removal-repo-owner' +import { isPrunableGitFileWorktree } from '../../../worktree-prunable-git-file' import { findRegisteredDeletableWorktree } from '../../../worktree-removal-safety' -import { removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval } from '../../../local-worktree-removal-recovery' +import { removeStaleLocalWorktreeRegistration } from '../../../local-worktree-removal-recovery' import { runHook } from '../../../hooks' import { withWorktreeRemoveStageSpan } from '../../../observability/instrumentation' import { @@ -85,13 +86,14 @@ export async function executeWorktreeRemoval( if ( !repo.connectionId && - args.force === true && - process.platform === 'win32' && - (isWindowsAbsolutePathLike(canonicalWorktreePath) || !!localWorktreeGitOptions.wslDistro) && - removedMeta && - (await isAlreadyRemovedWorktreePath(repo, canonicalWorktreePath, localWorktreeGitOptions)) + ((await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || + (args.force === true && + process.platform === 'win32' && + (isWindowsAbsolutePathLike(canonicalWorktreePath) || !!localWorktreeGitOptions.wslDistro) && + removedMeta && + (await isAlreadyRemovedWorktreePath(repo, canonicalWorktreePath, localWorktreeGitOptions)))) ) { - const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + const removalResult = await removeStaleLocalWorktreeRegistration({ canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, diff --git a/src/main/local-worktree-removal-recovery.test.ts b/src/main/local-worktree-removal-recovery.test.ts index 1be0fd02cce..7d5aa442a1e 100644 --- a/src/main/local-worktree-removal-recovery.test.ts +++ b/src/main/local-worktree-removal-recovery.test.ts @@ -22,7 +22,7 @@ vi.mock('./git/worktree', () => ({ import { recoverLocalWindowsWorktreeRemoval, - removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval + removeStaleLocalWorktreeRegistration } from './local-worktree-removal-recovery' async function withPlatform(platform: NodeJS.Platform, fn: () => Promise): Promise { @@ -306,7 +306,7 @@ describe('recoverLocalWindowsWorktreeRemoval', () => { }) }) -describe('removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval', () => { +describe('removeStaleLocalWorktreeRegistration', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() listWorktreesStrictMock.mockReset() @@ -314,9 +314,27 @@ describe('removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval', () => { listWorktreesStrictMock.mockResolvedValue([]) }) + it('prunes and strictly verifies on the selected WSL host without deleting files or branches', async () => { + const options = { wslDistro: 'Ubuntu' } + const result = await removeStaleLocalWorktreeRegistration({ + canonicalWorktreePath: '/home/dev/feature/.git', + repoPath: '/home/dev/repo', + localWorktreeGitOptions: options, + registeredWorktree: { branch: 'refs/heads/feature', head: 'abc123' }, + deleteBranch: true + }) + expect(result).toEqual({ preservedBranch: { branchName: 'feature', head: 'abc123' } }) + expect(gitExecFileAsyncMock).toHaveBeenCalledExactlyOnceWith(['worktree', 'prune'], { + cwd: '/home/dev/repo', + wslDistro: 'Ubuntu' + }) + expect(listWorktreesStrictMock).toHaveBeenCalledExactlyOnceWith('/home/dev/repo', options) + expect(removeLocalWorktreePathMock).not.toHaveBeenCalled() + }) + it('does not override a locked missing registration', async () => { await expect( - removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + removeStaleLocalWorktreeRegistration({ canonicalWorktreePath: 'C:/workspaces/feature', repoPath: 'C:/repo', localWorktreeGitOptions: {}, @@ -345,7 +363,7 @@ describe('removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval', () => { ]) await expect( - removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + removeStaleLocalWorktreeRegistration({ canonicalWorktreePath: 'C:/workspaces/feature', repoPath: 'C:/repo', localWorktreeGitOptions: {}, diff --git a/src/main/local-worktree-removal-recovery.ts b/src/main/local-worktree-removal-recovery.ts index 630dd338c20..46f1bb15c87 100644 --- a/src/main/local-worktree-removal-recovery.ts +++ b/src/main/local-worktree-removal-recovery.ts @@ -47,7 +47,7 @@ function staleRegistrationRecoveryError( error, canonicalWorktreePath, force - )} The worktree directory was removed, but Git still has stale worktree registration. Retry deletion after resolving the Git registration error.` + )} Git still has stale worktree registration. Retry deletion after resolving the Git registration error.` ) } @@ -151,7 +151,7 @@ async function isRecoverableWindowsFilesystemRemovalFailure( } } -export async function removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval( +export async function removeStaleLocalWorktreeRegistration( args: StaleLocalWorktreeRegistrationArgs ): Promise { return removeRequiredGitWorktreeRegistration(args) diff --git a/src/main/runtime/orca-runtime-remove-managed-worktree.ts b/src/main/runtime/orca-runtime-remove-managed-worktree.ts index 25a294c2a90..9cb95e5d354 100644 --- a/src/main/runtime/orca-runtime-remove-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-remove-managed-worktree.ts @@ -13,13 +13,14 @@ import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' import { resolveWorktreeRemovalRoute } from '../worktree-removal-execution-host-route' import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' import { listWorktreesStrict } from '../git/worktree' +import { isPrunableGitFileWorktree } from '../worktree-prunable-git-file' import { findRegisteredDeletableWorktree } from '../worktree-removal-safety' import { removeRuntimeUnregisteredWorktree } from './runtime-unregistered-worktree-removal' import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree/removal' import { formatWorktreeRemovalError } from '../ipc/worktree-logic' import { isWindowsAbsolutePathLike } from '../../shared/cross-platform-path' import { isRuntimeWorktreePathMissing } from './runtime-worktree-filesystem' -import { removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval } from '../local-worktree-removal-recovery' +import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery' import { cleanupUnusedWorktreePushTargetRemote } from '../ipc/worktree-remote' import { removeRuntimeRegisteredRemoteWorktree } from './runtime-registered-remote-worktree-removal' import { removeRuntimeRegisteredLocalWorktree } from './runtime-registered-local-worktree-removal' @@ -146,18 +147,19 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM } if ( route.kind === 'local' && - force === true && - process.platform === 'win32' && - (isWindowsAbsolutePathLike(canonicalWorktreePath) || - !!localWorktreeGitOptions.wslDistro) && - removedMeta && - (await isRuntimeWorktreePathMissing( - route.hostId, - canonicalWorktreePath, - localWorktreeGitOptions - )) + ((await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || + (force === true && + process.platform === 'win32' && + (isWindowsAbsolutePathLike(canonicalWorktreePath) || + !!localWorktreeGitOptions.wslDistro) && + removedMeta && + (await isRuntimeWorktreePathMissing( + route.hostId, + canonicalWorktreePath, + localWorktreeGitOptions + )))) ) { - const removalResult = await removeStaleLocalWorktreeRegistrationAfterFilesystemRemoval({ + const removalResult = await removeStaleLocalWorktreeRegistration({ canonicalWorktreePath, repoPath: repo.path, localWorktreeGitOptions, diff --git a/src/main/worktree-prunable-git-file.test.ts b/src/main/worktree-prunable-git-file.test.ts new file mode 100644 index 00000000000..de5921858b6 --- /dev/null +++ b/src/main/worktree-prunable-git-file.test.ts @@ -0,0 +1,70 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitWorktreeInfo } from '../shared/worktree/types' +import { isPrunableGitFileWorktree } from './worktree-prunable-git-file' + +const { statPath, pathAccess, runtimePath } = vi.hoisted(() => ({ + statPath: vi.fn(), + pathAccess: vi.fn(), + runtimePath: vi.fn() +})) +vi.mock('./local-worktree-filesystem', () => ({ + getLocalWorktreePathAccess: pathAccess, + toLocalWorktreeRuntimePath: runtimePath +})) +const worktree: GitWorktreeInfo = { + path: '/workspaces/feature/.git', + branch: 'refs/heads/feature', + head: 'a'.repeat(40), + isMainWorktree: false, + isBare: false, + prunable: true +} +beforeEach(() => { + vi.resetAllMocks() + statPath.mockResolvedValue({ isFile: () => true }) + pathAccess.mockReturnValue({ statPath }) + runtimePath.mockImplementation((path) => path) +}) +describe('prunable Git-file registration proof', () => { + it('accepts an attested named-branch file without reading or changing its parent', async () => { + await expect(isPrunableGitFileWorktree(worktree)).resolves.toBe(true) + expect(statPath).toHaveBeenCalledExactlyOnceWith(worktree.path) + }) + it.each([ + { prunable: false }, + { prunable: undefined }, + { isMainWorktree: true }, + { isBare: true }, + { locked: true }, + { branch: '' }, + { branch: 'refs/tags/feature' }, + { branch: 'refs/heads/' }, + { head: '' }, + { path: '/workspaces/feature' } + ])('refuses insufficient registration evidence %j', async (override) => { + await expect(isPrunableGitFileWorktree({ ...worktree, ...override })).resolves.toBe(false) + expect(statPath).not.toHaveBeenCalled() + }) + it.each([{ isFile: () => false }, { type: 'directory' }, { type: 'symlink' }, {}, null])( + 'refuses non-file or unknown filesystem evidence %j', + async (entry) => { + statPath.mockResolvedValue(entry) + await expect(isPrunableGitFileWorktree(worktree)).resolves.toBe(false) + } + ) + it('does not turn host failure into cleanup permission', async () => { + statPath.mockRejectedValue(new Error('host unavailable')) + await expect(isPrunableGitFileWorktree(worktree)).rejects.toThrow('host unavailable') + }) + it('uses the selected WSL distro and translated execution path', async () => { + const options = { wslDistro: 'Ubuntu' } + runtimePath.mockReturnValue('/home/dev/feature/.git') + statPath.mockResolvedValue({ type: 'file' }) + await expect( + isPrunableGitFileWorktree({ ...worktree, path: 'C:\\workspaces\\feature\\.git' }, options) + ).resolves.toBe(true) + expect(pathAccess).toHaveBeenCalledExactlyOnceWith(options) + expect(runtimePath).toHaveBeenCalledWith('C:\\workspaces\\feature\\.git', options) + expect(statPath).toHaveBeenCalledExactlyOnceWith('/home/dev/feature/.git') + }) +}) diff --git a/src/main/worktree-prunable-git-file.ts b/src/main/worktree-prunable-git-file.ts new file mode 100644 index 00000000000..d03c31e7295 --- /dev/null +++ b/src/main/worktree-prunable-git-file.ts @@ -0,0 +1,32 @@ +import type { GitWorktreeInfo } from '../shared/worktree/types' +import type { LocalWorktreeFilesystemOptions } from './local-worktree-filesystem' +import { getLocalWorktreePathAccess, toLocalWorktreeRuntimePath } from './local-worktree-filesystem' + +/** Registration cleanup must never reinterpret a malformed .git row as its parent checkout. */ +export async function isPrunableGitFileWorktree( + worktree: GitWorktreeInfo, + options: LocalWorktreeFilesystemOptions = {} +): Promise { + if ( + worktree.prunable !== true || + worktree.isMainWorktree || + worktree.isBare || + worktree.locked || + !worktree.branch.startsWith('refs/heads/') || + worktree.branch === 'refs/heads/' || + !worktree.head || + worktree.path.split(/[\\/]/).at(-1) !== '.git' + ) { + return false + } + const access = getLocalWorktreePathAccess(options) + const entry = await access.statPath(toLocalWorktreeRuntimePath(worktree.path, options)) + if (!entry || typeof entry !== 'object') { + return false + } + // WSL returns the owning guest's lstat-equivalent type; native lstat rejects symlinks too. + return ( + ('type' in entry && entry.type === 'file') || + ('isFile' in entry && typeof entry.isFile === 'function' && entry.isFile() === true) + ) +}