From b81431d7ff55d8e89a523ef68fe48c0aca0f4393 Mon Sep 17 00:00:00 2001 From: Kaoru0822 Date: Tue, 20 Oct 2026 22:54:28 -0700 Subject: [PATCH] fix(artifacts): report a stable symlink loop as a path resolution failure (#5571) ELOOP raised while resolving the caller-selected path (access/realpath) is a symbolic link loop or over-deep chain in that path, not evidence of a change. Classify filesystem failures by resolution stage so only ELOOP from opening the canonical path with symbolic links rejected keeps the "target changed while being opened" reason. The structured code, category and retryability are unchanged. Fixes #5521 Co-authored-by: Claude Opus 5.5 --- src/application/BinaryTargetResolver.ts | 40 ++++++++++++++++--- .../acceptance/artifactSymlinkLoopCli.test.ts | 35 ++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) create mode 100644 tests/acceptance/artifactSymlinkLoopCli.test.ts diff --git a/src/application/BinaryTargetResolver.ts b/src/application/BinaryTargetResolver.ts index 0572d3ae12..7b7f58afb0 100644 --- a/src/application/BinaryTargetResolver.ts +++ b/src/application/BinaryTargetResolver.ts @@ -90,11 +90,15 @@ export const parseBinaryTarget = async ( const hostArchitecture = options.hostArchitecture ?? process.arch; const { targetKind, formatHint, signal, architecture, entryPoint } = options; const candidate = absoluteCallerPath(input, { cwd }); + // Failures before canonicalization concern the selected path itself; later + // failures concern the canonical path opened with symbolic links rejected. + let stage: TargetFilesystemStage = "resolve"; try { throwIfTargetResolutionCancelled(signal); await access(candidate, constants.R_OK); throwIfTargetResolutionCancelled(signal); const canonical = await realpath(candidate); + stage = "read"; // Windows native admission must inspect the caller-selected reparse components. const sourcePath = process.platform === "win32" ? candidate : canonical; throwIfTargetResolutionCancelled(signal); @@ -247,24 +251,39 @@ export const parseBinaryTarget = async ( ); if (cause instanceof BinaryTargetError) return err(cause); if (!(cause instanceof Error)) throw cause; - const failure = classifyBinaryTargetFilesystemFailure(candidate, cause); + const failure = classifyTargetFilesystemFailure(candidate, cause, stage); if (failure === undefined) throw cause; return err(failure); } }; +/** + * `resolve` covers the caller-selected path before canonicalization; `read` + * covers a canonical path opened with symbolic links rejected. + */ +type TargetFilesystemStage = "resolve" | "read"; + /** Classify a filesystem failure while reading a previously selected target. */ export const classifyBinaryTargetFilesystemFailure = ( path: string, cause: unknown, +): BinaryTargetError | undefined => + classifyTargetFilesystemFailure(path, cause, "read"); + +const classifyTargetFilesystemFailure = ( + path: string, + cause: unknown, + stage: TargetFilesystemStage, ): BinaryTargetError | undefined => { if (!(cause instanceof Error)) return undefined; const code = filesystemErrorCode(cause); return code === undefined ? undefined - : new BinaryTargetError(path, targetFailureReason(code, cause.message), { - cause, - }); + : new BinaryTargetError( + path, + targetFailureReason(code, cause.message, stage), + { cause }, + ); }; const filesystemErrorCode = (cause: Error): string | undefined => { @@ -272,7 +291,11 @@ const filesystemErrorCode = (cause: Error): string | undefined => { return typeof code === "string" ? code : undefined; }; -const targetFailureReason = (code: string, detail: string): string => { +const targetFailureReason = ( + code: string, + detail: string, + stage: TargetFilesystemStage, +): string => { switch (code) { case "ENOENT": case "ENOTDIR": @@ -285,7 +308,12 @@ const targetFailureReason = (code: string, detail: string): string => { case "ENOTFILE": return "target is not a regular file"; case "ELOOP": - return `target changed while being opened: ${detail}`; + // A canonical path has no symbolic links left, so a link rejected while + // reading it appeared after resolution. Before that, ELOOP is a loop or + // an over-deep link chain in the selected path itself. + return stage === "resolve" + ? `target path could not be resolved: too many symbolic links or a symbolic link loop: ${detail}` + : `target changed while being opened: ${detail}`; default: return `target could not be read (${code}): ${detail}`; } diff --git a/tests/acceptance/artifactSymlinkLoopCli.test.ts b/tests/acceptance/artifactSymlinkLoopCli.test.ts new file mode 100644 index 0000000000..d44e122f19 --- /dev/null +++ b/tests/acceptance/artifactSymlinkLoopCli.test.ts @@ -0,0 +1,35 @@ +import { readlink, symlink } from "node:fs/promises"; +import { join } from "node:path"; +import { expect } from "vitest"; + +import { createTestTempDirectory } from "../fixtures/temporaryDirectory.js"; +import { cliTest } from "../support/cli/cliFixture.js"; + +cliTest.skipIf(process.platform === "win32").for([ + { command: "inspect-artifact", link: "loop.db", selected: "loop.db" }, + { command: "extract-artifact", link: "tree", selected: join("tree", "file") }, +])( + "$command reports a stable symbolic link loop as a path resolution failure", + async ({ command, link, selected }, { cli }) => { + const root = await createTestTempDirectory("rea-artifact-symlink-loop-"); + await symlink(link, join(root, link)); + const path = join(root, selected); + const result = await cli.run({ + arguments: [command, path, "--json"], + environment: { REA_LOG_LEVEL: "silent" }, + }); + expect(result.exitCode).toBe(1); + const reason = expect.stringMatching( + /^target path could not be resolved: too many symbolic links or a symbolic link loop: ELOOP/u, + ); + expect(result.json).toMatchObject({ + code: "target_unavailable", + category: "unavailable", + retryable: false, + message: expect.stringContaining("symbolic link loop"), + details: { path, reason }, + }); + expect(result.stdout).not.toContain("changed while being opened"); + expect(await readlink(join(root, link))).toBe(link); + }, +);