fix(repos): make errno classification self-contained and message-anchored

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).
This commit is contained in:
Merge Sim
2026-08-31 15:27:18 -07:00
parent a07bf51266
commit d61cf64e48
3 changed files with 81 additions and 25 deletions
+39
View File
@@ -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)
})
})
+36 -23
View File
@@ -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 ''
}
}
+6 -2
View File
@@ -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) {