diff --git a/src/application/environment/environmentBundleWrite.ts b/src/application/environment/environmentBundleWrite.ts index 47f225e8a0..0f5c6cc2ed 100644 --- a/src/application/environment/environmentBundleWrite.ts +++ b/src/application/environment/environmentBundleWrite.ts @@ -15,6 +15,7 @@ import { streamChunkToBuffer } from "../../artifacts/StreamBytes.js"; import { ArtifactReaderFailure } from "../../artifacts/ArtifactReader.js"; import { ArtifactResourceScope } from "../../artifacts/ArtifactResourceScope.js"; import { SafeOutputTree } from "../../artifacts/SafeOutputTree.js"; +import { outputDirectoryAdmissionRefusal } from "../../artifacts/SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "../../artifacts/SafeOutputTreeCreationFailure.js"; import { RegularFileChangedError } from "../../filesystem/RegularFile.js"; import { withRegularFile } from "../RegularFileRead.js"; @@ -125,27 +126,6 @@ const writeTarball = async ( return { path: written.relativePath, bytes: written.bytesWritten }; }; -/** - * Only destination admission can blame the selected directory. Path failures - * after admission are host write failures on bundle files, not argument refusals. - */ -const directoryRefusal = (cause: unknown): unknown => - cause instanceof ArtifactReaderFailure && - cause.reason === "path" && - cause.cleanup === undefined - ? new AnalysisInputError( - ENVIRONMENT_EXPORT_OPERATION, - { cause, userMessage: cause.message }, - [ - { - path: ["directory"], - reason: "invalid_value", - message: cause.message, - }, - ], - ) - : cause; - /** The output owner anchors every create and verifies the complete tree before publication. */ const writeBundleFiles = async ( input: EnvironmentExportInput, @@ -155,7 +135,6 @@ const writeBundleFiles = async ( const resources = new ArtifactResourceScope(); let tree: SafeOutputTree | undefined; let written: WrittenBundleFile[] | undefined; - let admitted = false; let outcome: | { readonly kind: "completed"; @@ -172,7 +151,6 @@ const writeBundleFiles = async ( if (cause instanceof SafeOutputTreeCreationFailure) tree = cause.tree; throw cause; } - admitted = true; written = []; for (const file of plan.files) written.push( @@ -220,7 +198,12 @@ const writeBundleFiles = async ( } if (outcome.kind === "failed") { if (owner?.published === true && written !== undefined) return written; - throw admitted ? outcome.cause : directoryRefusal(outcome.cause); + // Only a destination refusal can blame the selected directory. + throw outputDirectoryAdmissionRefusal( + ENVIRONMENT_EXPORT_OPERATION, + "directory", + outcome.cause, + ); } return outcome.files; }; diff --git a/src/artifacts/SafeOutputTree.ts b/src/artifacts/SafeOutputTree.ts index 3141dc00fb..d113c42f2b 100644 --- a/src/artifacts/SafeOutputTree.ts +++ b/src/artifacts/SafeOutputTree.ts @@ -20,6 +20,7 @@ import { } from "./ArtifactPaths.js"; import { absoluteCallerPath } from "../filesystem/callerPath.js"; import { ArtifactReaderFailure } from "./ArtifactReader.js"; +import { SafeOutputTreeDestinationRefusal } from "./SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "./SafeOutputTreeCreationFailure.js"; import { hashOutputFile } from "./SafeOutputTreeReadback.js"; import { removeOwnedTree } from "./SafeOutputTreeCleanup.js"; @@ -95,15 +96,16 @@ export class SafeOutputTree { options: { readonly acceptExistingEmpty?: boolean } = {}, ): Promise { if (!isAbsolute(outputRoot)) - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Extraction output root must be absolute", ); // Path syntax follows the host; `platform` selects only the name policy. const requested = absoluteCallerPath(outputRoot); const name = basename(requested); if (name === "." || name === "..") - throw new ArtifactReaderFailure("path", "Invalid extraction output root"); + throw new SafeOutputTreeDestinationRefusal( + "Invalid extraction output root", + ); const parent = await realpath(dirname(requested)).catch( (cause: unknown) => { throw new ArtifactReaderFailure( @@ -121,8 +123,7 @@ export class SafeOutputTree { created = false; return; } - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Extraction output root already exists", { cause }, ); @@ -130,11 +131,24 @@ export class SafeOutputTree { const tree = new SafeOutputTree(canonicalOutput, platform, requested); tree.#createdRoot = created; try { - const rootIdentity = await readDirectoryIdentity(canonicalOutput); + const rootIdentity = await readDirectoryIdentity(canonicalOutput).catch( + (cause: unknown) => { + // An accepted existing root that is not a directory refuses the + // selection itself; a root this call created was replaced by a host race. + if ( + !created && + cause instanceof ArtifactReaderFailure && + cause.reason === "path" + ) + throw new SafeOutputTreeDestinationRefusal(cause.message, { + cause, + }); + throw cause; + }, + ); tree.#rootIdentity = rootIdentity; if (!created && (await readdir(canonicalOutput)).length > 0) - throw new ArtifactReaderFailure( - "path", + throw new SafeOutputTreeDestinationRefusal( "Output directory is not empty", ); if (created) diff --git a/src/artifacts/SafeOutputTreeAdmission.test.ts b/src/artifacts/SafeOutputTreeAdmission.test.ts new file mode 100644 index 0000000000..ac3d2a5a93 --- /dev/null +++ b/src/artifacts/SafeOutputTreeAdmission.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from "vitest"; + +import { AnalysisInputError } from "../domain/analysisErrorCore.js"; +import { ArtifactReaderFailure } from "./ArtifactReader.js"; +import type { SafeOutputTree } from "./SafeOutputTree.js"; +import { + outputDirectoryAdmissionRefusal, + SafeOutputTreeDestinationRefusal, +} from "./SafeOutputTreeAdmission.js"; +import { SafeOutputTreeCreationFailure } from "./SafeOutputTreeCreationFailure.js"; + +// Only the failure's type matters here; no tree method is called. +const acquiredTree = {} as SafeOutputTree; +const refuse = (cause: unknown) => + outputDirectoryAdmissionRefusal( + "export_web_scripts", + "output_directory", + cause, + ); + +describe("output directory admission refusal", () => { + it.each([ + [ + "a refusal before acquisition", + new SafeOutputTreeDestinationRefusal( + "Extraction output root already exists", + ), + ], + [ + "a refusal of an acquired non-empty root", + new SafeOutputTreeCreationFailure( + new SafeOutputTreeDestinationRefusal("Output directory is not empty"), + acquiredTree, + ), + ], + ])("blames the selected directory for %s", (_name, cause) => { + const refused = refuse(cause); + + expect(refused).toBeInstanceOf(AnalysisInputError); + expect(refused).toMatchObject({ + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: cause.message, + }, + ], + }); + }); + + it.each([ + [ + "an acquired-tree setup failure", + new SafeOutputTreeCreationFailure( + new ArtifactReaderFailure( + "path", + "Cannot restrict output root to 0700", + ), + acquiredTree, + ), + ], + [ + "a path failure that is not a destination refusal", + new ArtifactReaderFailure("path", "Output file path changed"), + ], + [ + "a refusal whose cleanup is unresolved", + new ArtifactReaderFailure( + "path", + "Extraction output root already exists", + { + cause: new SafeOutputTreeDestinationRefusal( + "Extraction output root already exists", + ), + cleanup: { reason: "rollback failed", resources: ["/out"] }, + }, + ), + ], + ])("keeps artifact semantics for %s", (_name, cause) => { + expect(refuse(cause)).toBe(cause); + }); +}); diff --git a/src/artifacts/SafeOutputTreeAdmission.ts b/src/artifacts/SafeOutputTreeAdmission.ts new file mode 100644 index 0000000000..5c3976f4d8 --- /dev/null +++ b/src/artifacts/SafeOutputTreeAdmission.ts @@ -0,0 +1,43 @@ +import { AnalysisInputError } from "../domain/analysisErrorCore.js"; +import { ArtifactReaderFailure } from "./ArtifactReader.js"; + +/** + * `SafeOutputTree.create` refused the caller's selected destination itself: + * a relative or reserved root, an existing root, or an accepted existing root + * that is not an empty directory. + * Setup failures after a tree is acquired are not refusals of the selection. + */ +export class SafeOutputTreeDestinationRefusal extends ArtifactReaderFailure { + constructor(message: string, options?: { readonly cause?: unknown }) { + super("path", message, options); + this.name = "SafeOutputTreeDestinationRefusal"; + } +} + +/** + * Attribute a destination refusal to the caller's selected directory argument. + * A refusal raised after tree acquisition, such as a non-empty accepted root, + * arrives wrapped by `SafeOutputTreeCreationFailure`; every other failure, + * including acquired-tree setup failures, keeps its artifact semantics. + */ +export const outputDirectoryAdmissionRefusal = ( + operation: string, + field: string, + cause: unknown, +): unknown => { + if (!(cause instanceof ArtifactReaderFailure) || cause.cleanup !== undefined) + return cause; + const refusal = + cause instanceof SafeOutputTreeDestinationRefusal + ? cause + : cause.cause instanceof SafeOutputTreeDestinationRefusal + ? cause.cause + : undefined; + return refusal === undefined + ? cause + : new AnalysisInputError( + operation, + { cause, userMessage: refusal.message }, + [{ path: [field], reason: "invalid_value", message: refusal.message }], + ); +}; diff --git a/src/browser/assets/PublishWebScripts.ts b/src/browser/assets/PublishWebScripts.ts index f264776812..2a56d64d18 100644 --- a/src/browser/assets/PublishWebScripts.ts +++ b/src/browser/assets/PublishWebScripts.ts @@ -6,6 +6,7 @@ import { ArtifactReaderFailure } from "../../artifacts/ArtifactReader.js"; import { ArtifactResourceScope } from "../../artifacts/ArtifactResourceScope.js"; import { DestinationCaseCollisionFailure } from "../../artifacts/ArtifactPaths.js"; import { SafeOutputTree } from "../../artifacts/SafeOutputTree.js"; +import { outputDirectoryAdmissionRefusal } from "../../artifacts/SafeOutputTreeAdmission.js"; import { SafeOutputTreeCreationFailure } from "../../artifacts/SafeOutputTreeCreationFailure.js"; import { PLAYWRIGHT_RESPONSE_BODY_PROVENANCE_LIMITATION } from "../../domain/browserNetworkEvidence.js"; import { AnalysisCancelledError } from "../../domain/analysisErrorCore.js"; @@ -175,7 +176,13 @@ export const publishWebScripts = async ( if (tree.published && result !== undefined) return result; } - throw cancellation ?? cause; + if (cancellation !== undefined) throw cancellation; + // Only a destination refusal can blame the selected output directory. + throw outputDirectoryAdmissionRefusal( + OPERATION, + "output_directory", + cause, + ); } }); diff --git a/tests/boundary/cli/webScriptSelectionCli.test.ts b/tests/boundary/cli/webScriptSelectionCli.test.ts index e2d60e093d..48abd4830d 100644 --- a/tests/boundary/cli/webScriptSelectionCli.test.ts +++ b/tests/boundary/cli/webScriptSelectionCli.test.ts @@ -1,4 +1,4 @@ -import { access } from "node:fs/promises"; +import { access, mkdir, readdir, symlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import pino from "pino"; @@ -10,6 +10,7 @@ import { CLI_COMMANDS } from "../../../src/cliCommandNames.js"; import { AnalysisCancelledError } from "../../../src/domain/analysisErrorCore.js"; import { err } from "../../../src/domain/result.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { scriptScenarioFixture } from "../../fixtures/webScriptCapture.js"; import { cliTest } from "../../support/cli/cliFixture.js"; it("forwards an interrupt into the export workflow and reports cancellation", async () => { @@ -89,3 +90,53 @@ cliTest.skipIf(process.platform === "win32")( }); }, ); + +cliTest( + "refuses an existing output directory or link as invalid output_directory input", + async ({ cli }) => { + const root = await createTestTempDirectory("rea-script-export-existing-"); + const capturePath = join(root, "capture.json"); + await writeFile(capturePath, JSON.stringify(scriptScenarioFixture())); + const retained = join(root, "retained"); + await mkdir(retained); + await writeFile(join(retained, "marker.txt"), "keep"); + const link = join(root, "output-link"); + await symlink( + retained, + link, + process.platform === "win32" ? "junction" : "dir", + ); + + for (const outputDirectory of [retained, link]) { + const response = await cli.run({ + arguments: [ + "export-web-scripts", + capturePath, + outputDirectory, + "--json", + ], + environment: { REA_LOG_LEVEL: "silent" }, + }); + expect(response.exitCode).toBe(1); + expect(response.json).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + recovery: { + kind: "inspect_input_contract", + operation: "export_web_scripts", + }, + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: "Extraction output root already exists", + }, + ], + }, + }); + } + expect(await readdir(retained)).toEqual(["marker.txt"]); + }, +); diff --git a/tests/boundary/filesystem/webScriptExport.test.ts b/tests/boundary/filesystem/webScriptExport.test.ts index ecb173287c..74e08c6fce 100644 --- a/tests/boundary/filesystem/webScriptExport.test.ts +++ b/tests/boundary/filesystem/webScriptExport.test.ts @@ -312,6 +312,11 @@ describe("captured script publication failures and cleanup", () => { ); const result = await exportWebScripts(input, resources); if (result.ok) throw new Error("Expected exclusive output failure"); + expect(result.error).toMatchObject({ + _tag: "AnalysisInputError", + operation: "export_web_scripts", + issues: [{ path: ["output_directory"], reason: "invalid_value" }], + }); expect(result.error.userMessage).toContain("already exists"); expect(await readFile(marker, "utf8")).toBe("keep"); }); diff --git a/tests/boundary/mcp/webScriptSelectionMcp.test.ts b/tests/boundary/mcp/webScriptSelectionMcp.test.ts index 381c1dcb25..7b75e9d49e 100644 --- a/tests/boundary/mcp/webScriptSelectionMcp.test.ts +++ b/tests/boundary/mcp/webScriptSelectionMcp.test.ts @@ -1,5 +1,5 @@ import { execFile } from "node:child_process"; -import { access } from "node:fs/promises"; +import { access, mkdir, readdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { promisify } from "node:util"; @@ -15,6 +15,7 @@ import { ToolResultDelivery } from "../../../src/server/toolResult.js"; import { readWithoutFifoWriter } from "../../fixtures/fifoInput.js"; import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { scriptScenarioFixture } from "../../fixtures/webScriptCapture.js"; it.skipIf(process.platform === "win32")( "returns a capture_path invalid_request for a named pipe without waiting or publishing output", @@ -80,3 +81,58 @@ it.skipIf(process.platform === "win32")( }); }, ); + +it("refuses an existing output directory as invalid output_directory input", async () => { + const root = await createTestTempDirectory("rea-script-export-existing-mcp-"); + const capturePath = join(root, "capture.json"); + await writeFile(capturePath, JSON.stringify(scriptScenarioFixture())); + const outputDirectory = join(root, "retained"); + await mkdir(outputDirectory); + await writeFile(join(outputDirectory, "marker.txt"), "keep"); + const server = new EvidenceMcpServer( + { name: "script-export-existing", version: "1" }, + {}, + undefined, + new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE), + ); + const artifactResources = new ArtifactResourceScope(); + registerWebScriptTool(server, { + logger: silentLogger, + recordEvidence: undefined, + artifactResources, + }); + const client = new Client({ name: "script-export-existing", version: "1" }); + onTestFinished(async () => { + await client.close(); + await server.close(); + await artifactResources.close(); + }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + await server.connect(serverTransport); + await client.connect(clientTransport); + + const result = await client.callTool({ + name: "export_web_scripts", + arguments: { capture_path: capturePath, output_directory: outputDirectory }, + }); + expect(parseMcpToolError(result).error).toMatchObject({ + code: "invalid_request", + category: "invalid_input", + recovery: { + kind: "inspect_input_contract", + operation: "export_web_scripts", + }, + details: { + operation: "export_web_scripts", + issues: [ + { + path: ["output_directory"], + reason: "invalid_value", + message: "Extraction output root already exists", + }, + ], + }, + }); + expect(await readdir(outputDirectory)).toEqual(["marker.txt"]); +});