From d61cf64e48c7cefdabd11d5cb2ace0752388d76b Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 31 Aug 2026 15:27:18 -0700 Subject: [PATCH] fix(repos): make errno classification self-contained and message-anchored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 5. Four findings, one root cause: the classifier delegated part of its job and trusted shapes it should not have. - It called `isENOENT`, which reads `.code` again without a guard — so a throwing `code` getter still escaped the supposedly fail-closed path. The helper no longer delegates; it reads code and message once, each guarded. - SSH `ENOTDIR` never matched. The relay replaces a string errno with -32000 and forwards only the message, so the remote shape is `{code: -32000, message: "ENOTDIR: ..."}` and the string-only check missed it — the lane reported a definite file-vs-folder collision as an unavailable probe. - Any string `code` was treated as authoritative, so a wrapper attaching `REMOTE_FS_ERROR` while keeping the canonical ENOENT message suppressed the message fallback. Only ENOENT/ENOTDIR are authoritative now; anything else falls through to the message. - The runtime `.git` probe stored its failure in a string and branched on truthy, so an error with an empty message read as "no failure" and failed open into `git init`. It uses an explicit flag. Messages are matched anchored at the start, so a path that merely quotes an errno cannot match. Regression tests for all four shapes, including an Error instance with a throwing getter (the previous test used a plain object, which is why this survived). --- src/main/ipc/repos/proven-absence.test.ts | 39 +++++++++++++++ src/main/ipc/repos/proven-absence.ts | 59 ++++++++++++++--------- src/main/runtime/orca-runtime.ts | 8 ++- 3 files changed, 81 insertions(+), 25 deletions(-) diff --git a/src/main/ipc/repos/proven-absence.test.ts b/src/main/ipc/repos/proven-absence.test.ts index 157e3cb6522..10bfbcd9abf 100644 --- a/src/main/ipc/repos/proven-absence.test.ts +++ b/src/main/ipc/repos/proven-absence.test.ts @@ -43,4 +43,43 @@ describe('proven-absence', () => { expect(isNotADirectory(Object.assign(new Error('x'), { code: 'ENOTDIR' }))).toBe(true) expect(isNotADirectory(Object.assign(new Error('x'), { code: 'ENOENT' }))).toBe(false) }) + + it('does not throw when an Error instance has a throwing code getter', () => { + // R5: the old version delegated to isENOENT, which read `.code` a SECOND time unguarded. + const e = new Error('probe') + Object.defineProperty(e, 'code', { + get() { + throw new Error('boom') + } + }) + expect(() => isProvenAbsent(e)).not.toThrow() + expect(isProvenAbsent(e)).toBe(false) + expect(() => isNotADirectory(e)).not.toThrow() + }) + + it('recognises the relay ENOTDIR shape, where the string errno did not survive', () => { + expect( + isNotADirectory( + Object.assign(new Error("ENOTDIR: not a directory, stat '/x/.git'"), { code: -32000 }) + ) + ).toBe(true) + }) + + it('still consults the message when a wrapper attached a non-errno string code', () => { + expect( + isProvenAbsent( + Object.assign(new Error("ENOENT: no such file or directory, stat '/x'"), { + code: 'REMOTE_FS_ERROR' + }) + ) + ).toBe(true) + }) + + it('does not match an errno quoted later in the message', () => { + expect( + isProvenAbsent( + new Error("EACCES: denied, access '/a/ENOENT: no such file or directory/.git'") + ) + ).toBe(false) + }) }) diff --git a/src/main/ipc/repos/proven-absence.ts b/src/main/ipc/repos/proven-absence.ts index 99e431809b4..30e464743fe 100644 --- a/src/main/ipc/repos/proven-absence.ts +++ b/src/main/ipc/repos/proven-absence.ts @@ -1,44 +1,57 @@ -import { isENOENT } from '../filesystem-path-containment' +// Why anchored messages rather than `isENOENT`: over SSH the relay replaces a string errno with +// -32000 and forwards only the message (`relay/dispatcher-rpc-routing.ts`, +// `ssh/ssh-channel-multiplexer.ts`), so the message is the ONLY classification path remotely. +// Anchoring at the start stops a path that merely quotes an errno from matching. +const ENOENT_MESSAGE = /^ENOENT: no such file or directory\b/ +const ENOTDIR_MESSAGE = /^ENOTDIR: not a directory\b/ -/** - * 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. - * - * The message fallback is not a convenience: over SSH it is the ONLY path. The relay forwards a - * handler's message verbatim but replaces a string errno with -32000 - * (`relay/dispatcher-rpc-routing.ts`), and the multiplexer rebuilds the error from that pair - * (`ssh/ssh-channel-multiplexer.ts`). So every remote ENOENT is classified by its text. Do not - * "simplify" this to a code-only check — it would refuse every legitimate remote creation. - */ +// Why a vocabulary: only real errno names carry errno meaning. A wrapper attaching a domain string +// (`REMOTE_FS_ERROR`) while keeping the canonical message must not suppress the message fallback. +const AUTHORITATIVE_ERRNO = new Set(['ENOENT', 'ENOTDIR']) + +/** Whether a failed probe proves the path is absent. Never throws. */ export function isProvenAbsent(error: unknown): boolean { const code = readErrnoCode(error) - if (typeof code === 'string') { + if (code !== undefined && AUTHORITATIVE_ERRNO.has(code)) { return code === 'ENOENT' } - return isENOENT(error) + return ENOENT_MESSAGE.test(readMessage(error)) } /** - * Whether the probe proved the *parent* is not a directory — a definite answer about the target, + * Whether the probe proved the parent is not a directory — a definite answer about the target, * not an indeterminate probe, so callers should say so rather than "could not check". */ export function isNotADirectory(error: unknown): boolean { - return readErrnoCode(error) === 'ENOTDIR' + const code = readErrnoCode(error) + if (code !== undefined && AUTHORITATIVE_ERRNO.has(code)) { + return code === 'ENOTDIR' + } + return ENOTDIR_MESSAGE.test(readMessage(error)) } -// Why: `?.` only guards a nullish base — it still invokes an accessor, so an error-like rejection -// with a throwing `code` getter would escape a fail-closed path as an unhandled rejection. -function readErrnoCode(error: unknown): unknown { +// Why guarded: `?.` only protects a nullish base — it still invokes an accessor, so a throwing +// `code` getter would escape a fail-closed path as an unhandled rejection. +function readErrnoCode(error: unknown): string | undefined { if (typeof error !== 'object' || error === null) { return undefined } try { - return (error as { code?: unknown }).code + const code = (error as { code?: unknown }).code + return typeof code === 'string' ? code : undefined } catch { return undefined } } + +function readMessage(error: unknown): string { + if (typeof error !== 'object' || error === null) { + return '' + } + try { + const message = (error as { message?: unknown }).message + return typeof message === 'string' ? message : '' + } catch { + return '' + } +} diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 449659d7446..db2651c8270 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -23916,16 +23916,20 @@ export class OrcaRuntimeService { // Why: refuse an existing repository on positive evidence, before `git init` can silently // reinitialize it. A `.git` FILE counts — that is how linked worktrees and submodules point. const gitPath = join(targetPath, '.git') - let gitProbeFailure: string | null = null + let gitProbeFailed = false + let gitProbeFailure = '' const gitStat = await stat(gitPath).catch((error: unknown) => { // Why: an indeterminate probe must surface as the shared "cannot check" message, not as // the generic prepare-directory error the outer catch would produce. + // Why a flag, not a truthy string: an error with an empty message would otherwise read + // as "no failure" and fail open into `git init`. if (!isProvenAbsent(error)) { + gitProbeFailed = true gitProbeFailure = error instanceof Error ? error.message : String(error) } return null }) - if (gitProbeFailure) { + if (gitProbeFailed) { return { error: repositoryCheckUnavailableError(trimmedName, gitProbeFailure) } } if (gitStat) {