mirror of
https://github.com/stablyai/orca.git
synced 2026-10-02 00:02:05 +00:00
fix(repos): tighten absence classification and the leftover hint
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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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 =
|
||||
|
||||
@@ -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' &&
|
||||
|
||||
@@ -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' &&
|
||||
|
||||
Reference in New Issue
Block a user