From 91e6650fed68a36428f1e1db0a6ccfc48d7da271 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 31 Aug 2026 14:44:24 -0700 Subject: [PATCH] fix(repos): tighten absence classification and the leftover hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 (0 high, 3 medium). Two fixed here; the third is documented rather than fixed, see below. Absence classification: `isENOENT`'s message fallback is unanchored, so an EACCES whose *path* contains "ENOENT: no such file or directory" read as absence. The obvious fix — reject whenever `code` is not ENOENT — would break the SSH lane, where the relay replaces string errnos with -32000 and the message is the only surviving signal. New `isProvenAbsent` distinguishes the two: a *string* code is a definitive errno and settles it alone; only when there is no string code does it fall back to the message. Leftover hint: it was gated on `!createdDir`, but removal of a directory we created is fire-and-forget. If cleanup fails (EBUSY, permissions, SSH disconnect) the directory and its `.git` survive and the retry hits "not empty" with no explanation. Now gated on whether the removal actually succeeded. Both guards mutation-tested: reverting either turns exactly its own new assertion red. NOT fixed — the `.git` refusal is a TOCTOU guard: another process can create `.git` between the probe and `git init`. Making that atomic means initializing in an exclusively-created private directory and moving it into place, which is a larger change than this ticket. The consequence here is bounded — a missed refusal means `git init` reinitializes, and reinitialization is not destructive now that the rollback no longer deletes `.git`. The genuinely destructive action remains gated on exclusive create. --- src/main/ipc/repos-create.test.ts | 35 +++++++++++++++++++ .../ipc/repos/existing-repository-probe.ts | 17 +++++++++ src/main/ipc/repos/remote-repo-creation.ts | 14 +++++--- src/main/ipc/repos/repo-creation-handlers.ts | 12 ++++--- src/main/runtime/orca-runtime.ts | 11 ++++-- 5 files changed, 77 insertions(+), 12 deletions(-) create mode 100644 src/main/ipc/repos/existing-repository-probe.ts diff --git a/src/main/ipc/repos-create.test.ts b/src/main/ipc/repos-create.test.ts index 8740e677cbd..48d06a994f5 100644 --- a/src/main/ipc/repos-create.test.ts +++ b/src/main/ipc/repos-create.test.ts @@ -460,6 +460,41 @@ describe('repos:create', () => { expect(result).not.toMatchObject({ error: expect.stringContaining('before retrying') }) }) + it('warns about the leftover when cleanup of a directory we created fails', async () => { + // `createdDir` proves ownership, not that the removal succeeded. If cleanup fails the + // directory and its .git remain, and the retry hits "not empty" with no explanation. + rmMock.mockRejectedValueOnce(new Error('EBUSY: resource busy or locked')) + gitExecFileAsyncMock + .mockReset() + .mockResolvedValueOnce({ stdout: '', stderr: '' }) + .mockRejectedValueOnce(new Error('commit broke')) + + const result = await callCreate({ parentPath: '/tmp', name: 'locked', kind: 'git' }) + + expect(result).toMatchObject({ error: expect.stringContaining('before retrying') }) + }) + + it('does not read a non-ENOENT errno as absence even if its message quotes ENOENT', async () => { + // The shared isENOENT message fallback is unanchored, so a path containing the canonical + // phrase would otherwise read as "absent". A string errno settles it on its own. + accessMock.mockResolvedValueOnce(undefined) // target exists + accessMock.mockRejectedValueOnce( + Object.assign( + new Error( + "EACCES: permission denied, access '/tmp/ENOENT: no such file or directory/.git'" + ), + { code: 'EACCES' } + ) + ) + gitExecFileAsyncMock.mockReset() + + const result = await callCreate({ parentPath: '/tmp', name: 'tricky', kind: 'git' }) + + expect(result).toMatchObject({ error: expect.stringContaining('Could not check') }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + expect(rmMock).not.toHaveBeenCalled() + }) + it('refuses a target that is already a git repository, before running git', async () => { // Positive evidence: we saw a .git. Refusing here is what stops `git init` from silently // reinitializing someone's repository. diff --git a/src/main/ipc/repos/existing-repository-probe.ts b/src/main/ipc/repos/existing-repository-probe.ts new file mode 100644 index 00000000000..01318c1848e --- /dev/null +++ b/src/main/ipc/repos/existing-repository-probe.ts @@ -0,0 +1,17 @@ +import { isENOENT } from '../filesystem-path-containment' + +/** + * Whether a failed existence probe proves the path is absent. + * + * Why not `isENOENT` alone: its message fallback is unanchored, so an `EACCES` whose *path* + * happens to contain "ENOENT: no such file or directory" would read as absent. A string `code` is + * a definitive errno and settles the question by itself. Only when there is no string code — the + * SSH relay replaces string errnos with a numeric one — do we fall back to the message. + */ +export function isProvenAbsent(error: unknown): boolean { + const code = (error as { code?: unknown } | null | undefined)?.code + if (typeof code === 'string') { + return code === 'ENOENT' + } + return isENOENT(error) +} diff --git a/src/main/ipc/repos/remote-repo-creation.ts b/src/main/ipc/repos/remote-repo-creation.ts index 2b43a37e25f..72126333f10 100644 --- a/src/main/ipc/repos/remote-repo-creation.ts +++ b/src/main/ipc/repos/remote-repo-creation.ts @@ -16,7 +16,7 @@ import { repositoryCheckUnavailableError } from './repository-creation-messages' import { resolveRemoteHomePath } from './remote-home-path' -import { isENOENT } from '../filesystem-path-containment' +import { isProvenAbsent } from './existing-repository-probe' export async function createRemoteRepo( store: Store, @@ -76,7 +76,7 @@ export async function createRemoteRepo( } catch (err) { // Why: only a proven ENOENT means absent. EACCES, ELOOP, a relay timeout or a disconnect say // nothing about the target, and must not become permission to create into it. - if (!isENOENT(err)) { + if (!isProvenAbsent(err)) { const message = err instanceof Error ? err.message : String(err) return { error: repositoryCheckUnavailableError(name, message) } } @@ -92,7 +92,7 @@ export async function createRemoteRepo( return { error: alreadyARepositoryError(name) } } catch (err) { // Why: same rule as above — an indeterminate probe is not evidence that `.git` is absent. - if (!isENOENT(err)) { + if (!isProvenAbsent(err)) { const message = err instanceof Error ? err.message : String(err) return { error: repositoryCheckUnavailableError(name, message) } } @@ -134,12 +134,16 @@ export async function createRemoteRepo( } catch (err) { // Why: only the exclusive createDirNoClobber proves we made this; `git init` is idempotent and // never reports whether it created or reinitialized, so a .git here may not be ours to delete. + let targetRemoved = false if (createdDir) { - await fsProvider.deletePath(targetPath, true).catch(() => undefined) + targetRemoved = await fsProvider + .deletePath(targetPath, true) + .then(() => true) + .catch(() => false) } // Why: the .git we leave makes the folder non-empty, so a silent retry would hit the // "not empty" guard with no clue why. - const leftover = !createdDir && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' + const leftover = !targetRemoved && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' const message = err instanceof Error ? err.message : String(err) if (step === 'commit' && /Please tell me who you are|user\.name|user\.email/i.test(message)) { const identityHint = diff --git a/src/main/ipc/repos/repo-creation-handlers.ts b/src/main/ipc/repos/repo-creation-handlers.ts index 28a90e2d12e..47252fa8661 100644 --- a/src/main/ipc/repos/repo-creation-handlers.ts +++ b/src/main/ipc/repos/repo-creation-handlers.ts @@ -15,7 +15,7 @@ import { LEFTOVER_GIT_DIR_RETRY_HINT, repositoryCheckUnavailableError } from './repository-creation-messages' -import { isENOENT } from '../filesystem-path-containment' +import { isProvenAbsent } from './existing-repository-probe' import { gitExecFileAsync } from '../../git/runner' import { detectRepoIconAndUpstream } from '../../repo-icon-autodetect' import { prepareLocalWorktreeRootForRepo } from '../../worktree-root-preparation' @@ -200,7 +200,7 @@ export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: S await access(join(targetPath, '.git')) return { error: alreadyARepositoryError(name) } } catch (err) { - if (!isENOENT(err)) { + if (!isProvenAbsent(err)) { const message = err instanceof Error ? err.message : String(err) return { error: repositoryCheckUnavailableError(name, message) } } @@ -251,12 +251,16 @@ export function registerRepoCreationHandlers(mainWindow: BrowserWindow, store: S } catch (err) { // Why: only the exclusive mkdir proves we made this; `git init` is idempotent and never // reports whether it created or reinitialized, so a .git here may not be ours to delete. + let targetRemoved = false if (createdDir) { - await rm(targetPath, { recursive: true, force: true }).catch(() => {}) + targetRemoved = await rm(targetPath, { recursive: true, force: true }) + .then(() => true) + .catch(() => false) } // Why: the .git we leave makes the folder non-empty, so a silent retry would hit the // "not empty" guard with no clue why. - const leftover = !createdDir && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' + const leftover = + !targetRemoved && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' const message = err instanceof Error ? err.message : String(err) if ( step === 'commit' && diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 189c43dc1e4..96163acf6be 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1277,6 +1277,7 @@ import { alreadyARepositoryError, LEFTOVER_GIT_DIR_RETRY_HINT } from '../ipc/repos/repository-creation-messages' +import { isProvenAbsent } from '../ipc/repos/existing-repository-probe' import { getWorktreeWatcherRemoval } from '../ipc/worktree-watcher-removal' import { acquireWatcherRemovalGate } from '../ipc/watcher-removal-gate' import { @@ -23915,7 +23916,7 @@ export class OrcaRuntimeService { // reinitialize it. A `.git` FILE counts — that is how linked worktrees and submodules point. const gitPath = join(targetPath, '.git') const gitStat = await stat(gitPath).catch((error: unknown) => { - if (isENOENT(error)) { + if (isProvenAbsent(error)) { return null } throw error @@ -23947,12 +23948,16 @@ export class OrcaRuntimeService { } catch (error) { // Why: only the exclusive mkdir proves we made this; `git init` is idempotent and never // reports whether it created or reinitialized, so a .git here may not be ours to delete. + let targetRemoved = false if (createdDir) { - await rm(targetPath, { recursive: true, force: true }).catch(() => {}) + targetRemoved = await rm(targetPath, { recursive: true, force: true }) + .then(() => true) + .catch(() => false) } // Why: the .git we leave makes the folder non-empty, so a silent retry would hit the // "not empty" guard with no clue why. - const leftover = !createdDir && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' + const leftover = + !targetRemoved && step === 'commit' ? ` ${LEFTOVER_GIT_DIR_RETRY_HINT}` : '' const message = error instanceof Error ? error.message : String(error) if ( step === 'commit' &&