diff --git a/docs/installation.md b/docs/installation.md index 8cc71047dd..879b118cfb 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -170,14 +170,25 @@ paths as `unmanaged`, names the affected paths, and provides those alternatives; an unreadable skill identity remains `unknown`. Uninstall retains and reports files it cannot safely remove, including their ownership manifest. -The skill installer backs up changed existing files and verifies the installed -bytes. Its current rollback restores or removes files by pathname without -checking that they are still the files published by that installation attempt. -If another writer changes a skill file during a failed installation, rollback -can overwrite that edit or remove a replacement first-install file -([#5928](https://github.com/morluto/rea/issues/5928)). Avoid editing managed skill -files or running another skill installer while setup is applying them. This -limitation does not apply to the client-configuration rollback described above. +The skill installer preserves byte-exact backups of changed existing files, +including invalid UTF-8, BOMs, and permission modes, and verifies each published +file through its owned descriptor. Before publication it checks the selected +file identity; first installation uses an exclusive hard link and refuses a +concurrently created destination. Filesystems without hard-link support cannot +publish a first-install skill file. + +Rollback restores an existing file only while its published identity, state, +and bytes still match that installation attempt. A recovery backup preserves +the original bytes; it does not authorize overwriting a later user edit or +replacement. Conflicts report the affected paths and recovery backups. If a +newly created skill file cannot be verified, setup retains it and reports manual +recovery rather than deleting by pathname. Staging cleanup failures remain +owned and retryable, and setup does not report installation success while +cleanup is unresolved. + +These checks do not lock out another writer modifying the same inode or exclude +pathname changes between the last check and publication. Avoid editing managed +skill files or running another installer while setup applies them. `rea setup --skill-client claude_code --yes --json` refreshes and verifies the selected skill destination independently of MCP registration selection. The diff --git a/src/application/SetupSkill.ts b/src/application/SetupSkill.ts index 96423f64df..d1137214e9 100644 --- a/src/application/SetupSkill.ts +++ b/src/application/SetupSkill.ts @@ -1,7 +1,6 @@ -import { copyFile, lstat, mkdir, readFile, rm } from "node:fs/promises"; +import { copyFile, lstat, mkdir, readFile } from "node:fs/promises"; import { constants as fsConstants } from "node:fs"; import { dirname, join } from "node:path"; -import writeFileAtomic from "write-file-atomic"; import { err, ok, type Result } from "../domain/result.js"; import { readBoundedFileBytes } from "../process/BoundedFileBytes.js"; import type { ArtifactResourceScope } from "../artifacts/ArtifactResourceScope.js"; @@ -16,6 +15,12 @@ import { RegularFileCleanupFailure } from "../filesystem/OwnedRegularFile.js"; import { withRegularFile } from "./RegularFileRead.js"; import { clientSkillDirectories } from "./SupportedClients.js"; import { preserveCurrentRecoveryBackup } from "./SetupRecoveryBackup.js"; +import { + SkillFilePublication, + SkillFilePreparationFailure, + type SkillFileSnapshot, + type SkillPublicationCleanupOwner, +} from "./SkillFilePublication.js"; export const MANAGED_SKILL_FILES = [ "SKILL.md", @@ -35,19 +40,14 @@ export const readSkillFile = ( path: string, resources?: ArtifactResourceScope, ): Promise => - readSkillFileState(path, resources).then(({ bytes }) => + readSkillFileSnapshot(path, resources).then(({ bytes }) => bytes.toString("utf8"), ); -interface SkillFileState { - readonly bytes: Buffer; - readonly mode: number; -} - -const readSkillFileState = async ( +const readSkillFileSnapshot = async ( path: string, resources?: ArtifactResourceScope, -): Promise => +): Promise => withRegularFile( path, async (handle, stats) => { @@ -56,7 +56,7 @@ const readSkillFileState = async ( }); if (bytes === undefined || bytes.length !== stats.size) throw new RegularFileChangedError(path); - return { bytes, mode: stats.mode & 0o7777 }; + return { bytes, mode: stats.mode & 0o7777, state: stats }; }, { symlinks: "reject", @@ -67,7 +67,7 @@ const readSkillFileState = async ( interface CanonicalSkillFile { readonly content: string; readonly destination: string; - readonly original: SkillFileState | undefined; + readonly original: SkillFileSnapshot | undefined; } const canonicalSkillContent = (relativePath: string): Promise => @@ -82,12 +82,7 @@ const canonicalSkillContent = (relativePath: string): Promise => export interface SkillInstallFileOperations { readonly mkdir: (path: string) => Promise; readonly backup: (source: string, destination: string) => Promise; - readonly write: ( - path: string, - content: string | Buffer, - mode: number, - ) => Promise; - readonly remove: (path: string) => Promise; + readonly prepare: typeof SkillFilePublication.prepare; } export interface SkillInstallRollbackFailure { @@ -103,6 +98,12 @@ export class SetupSkillInstallError extends Error { readonly reason: string; readonly rollbackFailures: readonly SkillInstallRollbackFailure[]; readonly recoveryBackupPaths: readonly string[]; + readonly cleanupOwners: readonly SkillPublicationCleanupOwner[]; + + /** Retry only retained staging cleanup; never reapply a pathname rollback. */ + async retryCleanup(): Promise { + return closeSkillPublications(this.cleanupOwners); + } constructor(input: { readonly path: string; @@ -110,6 +111,7 @@ export class SetupSkillInstallError extends Error { readonly reason: string; readonly rollbackFailures: readonly SkillInstallRollbackFailure[]; readonly recoveryBackupPaths: readonly string[]; + readonly cleanupOwners?: readonly SkillPublicationCleanupOwner[]; readonly cause: unknown; }) { const message = [ @@ -118,11 +120,11 @@ export class SetupSkillInstallError extends Error { ? [] : [ `Rollback also failed for ${input.rollbackFailures.map(({ path, reason }) => `${path} (${reason})`).join(", ")}.`, - ...(input.recoveryBackupPaths.length === 0 - ? [] - : [ - `Existing-file backups remain at ${input.recoveryBackupPaths.join(", ")}.`, - ]), + ]), + ...(input.recoveryBackupPaths.length === 0 + ? [] + : [ + `Existing-file backups remain at ${input.recoveryBackupPaths.join(", ")}.`, ]), ].join(" "); super(message, { cause: input.cause }); @@ -131,6 +133,7 @@ export class SetupSkillInstallError extends Error { this.reason = input.reason; this.rollbackFailures = input.rollbackFailures; this.recoveryBackupPaths = input.recoveryBackupPaths; + this.cleanupOwners = input.cleanupOwners ?? []; } get remediation(): string { @@ -150,9 +153,7 @@ const systemSkillInstallFileOperations: SkillInstallFileOperations = { }, backup: (source, destination) => copyFile(source, destination, fsConstants.COPYFILE_EXCL), - write: (path, content, mode) => - writeFileAtomic(path, content, { encoding: "utf8", mode }), - remove: (path) => rm(path, { force: true }), + prepare: SkillFilePublication.prepare, }; /** Managed skill location selected for a client family. */ @@ -230,9 +231,9 @@ export const existingSkillDestinations = async ( const readOptionalSkillState = async ( path: string, resources?: ArtifactResourceScope, -): Promise => { +): Promise => { try { - return await readSkillFileState(path, resources); + return await readSkillFileSnapshot(path, resources); } catch (cause: unknown) { if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") return undefined; @@ -403,7 +404,7 @@ const inspectCanonicalSkill = async ( const rootIssues: SkillPathIssue[] = []; for (const relativePath of MANAGED_SKILL_FILES) { const destination = join(root, relativePath); - let original: SkillFileState | undefined; + let original: SkillFileSnapshot | undefined; try { original = await readOptionalSkillState(destination, resources); } catch (cause: unknown) { @@ -485,17 +486,24 @@ const causePath = (cause: unknown): string | undefined => ? cause.path : undefined; -const restoreSkillFiles = async ( - attempted: readonly CanonicalSkillFile[], - operations: SkillInstallFileOperations, +const closeSkillPublications = async ( + publications: readonly SkillPublicationCleanupOwner[], + resources?: ArtifactResourceScope, ): Promise => { const failures: SkillInstallRollbackFailure[] = []; - for (const { destination, original } of [...attempted].reverse()) { + for (const publication of publications) { + if (publication.lease.released) continue; try { - if (original === undefined) await operations.remove(destination); - else await operations.write(destination, original.bytes, original.mode); + if (resources === undefined) await publication.close(); + else { + const released = await resources.release(publication); + if (released.kind === "failed") throw released.cause; + } } catch (cause: unknown) { - failures.push({ path: destination, reason: causeReason(cause) }); + failures.push({ + path: publication.resource, + reason: causeReason(cause), + }); } } return failures; @@ -510,7 +518,9 @@ export const installCanonicalSkill = async ( operations: SkillInstallFileOperations = systemSkillInstallFileOperations, resources?: ArtifactResourceScope, ): Promise => { - let attempted: CanonicalSkillFile[] = []; + const publications: SkillFilePublication[] = []; + const preparedCleanupOwners: SkillPublicationCleanupOwner[] = []; + let verified = false; let stage: "prepare" | "backup" | "write" | "readback" = "prepare"; let failingPath = "skill bundle"; const recoveryPaths = new Map(); @@ -549,42 +559,61 @@ export const installCanonicalSkill = async ( stage = "write"; for (const file of changed) { failingPath = file.destination; - // An atomic writer can publish and then reject; include the target before - // entering it so rollback covers that ambiguous outcome. - attempted.push(file); - await operations.write( + const publication = await operations.prepare( file.destination, - file.content, - file.original?.mode ?? 0o600, + Buffer.from(file.content, "utf8"), + file.original, ); + // Ownership exists before publish can succeed and subsequently reject. + publications.push(publication); + await publication.publish(); } stage = "readback"; - for (const { destination, content } of changed) { - failingPath = destination; - if ( - !(await readSkillFileState(destination, resources)).bytes.equals( - Buffer.from(content, "utf8"), - ) - ) - throw new Error(`skill readback mismatch: ${destination}`); + for (const publication of publications) { + failingPath = publication.path; + await publication.assertCurrent(); } + verified = true; + const cleanupFailures = await closeSkillPublications( + publications, + resources, + ); + if (cleanupFailures.length > 0) + throw new Error( + `Skill publication cleanup incomplete: ${cleanupFailures.map(({ path, reason }) => `${path} (${reason})`).join(", ")}`, + ); return ok("installed"); } catch (cause: unknown) { if (stage === "prepare" && cause instanceof SetupSkillInstallError) return err(cause); failingPath = causePath(cause) ?? failingPath; - const rollbackFailures = await restoreSkillFiles(attempted, operations); + if (cause instanceof SkillFilePreparationFailure) + preparedCleanupOwners.push(cause.publication); + const rollbackFailures: SkillInstallRollbackFailure[] = []; + if (!verified) { + for (const publication of [...publications].reverse()) { + try { + await publication.restore(); + } catch (rollbackCause: unknown) { + rollbackFailures.push({ + path: publication.path, + reason: causeReason(rollbackCause), + }); + } + } + } + const cleanupOwners = [...publications, ...preparedCleanupOwners]; + rollbackFailures.push( + ...(await closeSkillPublications(cleanupOwners, resources)), + ); return err( new SetupSkillInstallError({ path: failingPath, stage, reason: causeReason(cause), rollbackFailures, - recoveryBackupPaths: attempted.flatMap(({ destination, original }) => - original === undefined || recoveryPaths.get(destination) === undefined - ? [] - : [recoveryPaths.get(destination)!], - ), + recoveryBackupPaths: [...recoveryPaths.values()], + cleanupOwners, cause, }), ); diff --git a/src/application/SkillFilePublication.ts b/src/application/SkillFilePublication.ts new file mode 100644 index 0000000000..0cc4977381 --- /dev/null +++ b/src/application/SkillFilePublication.ts @@ -0,0 +1,268 @@ +import { constants, type Stats } from "node:fs"; +import { link, lstat, mkdtemp, open, realpath, rename } from "node:fs/promises"; +import { basename, dirname, join } from "node:path"; + +import { AnalysisCleanupLease } from "../artifacts/AnalysisCleanupLease.js"; +import { OwnedFileHandle } from "../filesystem/OwnedFileHandle.js"; +import { OwnedStagedOutput } from "../filesystem/OwnedStagedOutput.js"; +import { + RegularFileChangedError, + sameRegularFileState, +} from "../filesystem/RegularFile.js"; + +/** The read owner's byte-exact original and selected identity. */ +export interface SkillFileSnapshot { + readonly bytes: Buffer; + readonly mode: number; + readonly state: Stats; +} + +export interface SkillPublicationCleanupOwner { + readonly path: string; + readonly kind: "lease"; + readonly resource: string; + readonly lease: AnalysisCleanupLease; + close(): Promise; +} + +/** Staging cleanup failed before a publication could be returned to its caller. */ +export class SkillFilePreparationFailure extends Error { + readonly _tag = "SkillFilePreparationFailure"; + constructor( + readonly publication: SkillPublicationCleanupOwner, + readonly primary: unknown, + readonly cleanupCause: unknown, + ) { + super( + `Skill staging cleanup failed for ${publication.path}: ${primary instanceof Error ? primary.message : String(primary)}; cleanup: ${cleanupCause instanceof Error ? cleanupCause.message : String(cleanupCause)}`, + { + cause: new AggregateError([primary, cleanupCause]), + }, + ); + } +} + +/** One byte-oriented publication, retained before entering a fallible publish. */ +export class SkillFilePublication { + #attempted = false; + #restored = false; + #publishedState: Stats | undefined; + readonly kind = "lease"; + readonly resource: string; + readonly lease: AnalysisCleanupLease; + + private constructor( + readonly path: string, + private readonly bytes: Buffer, + private readonly original: SkillFileSnapshot | undefined, + private readonly staging: OwnedStagedOutput, + private readonly owner: OwnedFileHandle, + private readonly stagedPath: string, + ) { + this.resource = staging.directory; + this.lease = new AnalysisCleanupLease( + () => staging.close(), + [staging.directory], + ); + } + + static async prepare( + path: string, + bytes: Buffer, + original: SkillFileSnapshot | undefined, + ): Promise { + const parent = await realpath(dirname(path)); + const destination = join(parent, basename(path)); + const directory = await mkdtemp(join(parent, ".rea-skill-")); + const staging = await OwnedStagedOutput.create(directory); + let publication: SkillFilePublication | undefined; + try { + const stagedPath = join(directory, "content"); + const owner = new OwnedFileHandle( + await open( + stagedPath, + constants.O_RDWR | + constants.O_CREAT | + constants.O_EXCL | + (constants.O_NOFOLLOW ?? 0), + 0o600, + ), + ); + staging.setFile(owner); + publication = new SkillFilePublication( + destination, + Buffer.from(bytes), + original, + staging, + owner, + stagedPath, + ); + await owner.handle.writeFile(bytes); + const created = await owner.handle.stat(); + if ( + original !== undefined && + (created.uid !== original.state.uid || + created.gid !== original.state.gid) + ) + await owner.handle.chown(original.state.uid, original.state.gid); + await owner.handle.chmod(original?.mode ?? 0o600); + await owner.handle.sync(); + await staging.assertReadyForPublication(stagedPath); + return publication; + } catch (primary: unknown) { + try { + await staging.close(); + } catch (cleanupCause: unknown) { + // Even an open failure leaves the staging directory owned and retryable. + if (publication === undefined) { + const lease = new AnalysisCleanupLease( + () => staging.close(), + [directory], + ); + throw new SkillFilePreparationFailure( + { + path: destination, + kind: "lease", + resource: directory, + lease, + close: () => lease.release(), + }, + primary, + cleanupCause, + ); + } + throw new SkillFilePreparationFailure( + publication, + primary, + cleanupCause, + ); + } + throw primary; + } + } + + async publish(): Promise { + await this.staging.assertReadyForPublication(this.stagedPath); + await this.#assertSelected(); + this.#attempted = true; + if (this.original === undefined) { + // Exclusive creation preserves a file created after the absence check. + await link(this.stagedPath, this.path); + } else { + await rename(this.stagedPath, this.path); + this.staging.recordPublishedRename(); + } + this.#publishedState = await this.owner.handle.stat(); + } + + async assertCurrent(): Promise { + const before = this.#publishedState; + if ( + before === undefined || + !sameRegularFileState(before, await this.owner.handle.stat()) + ) + throw new RegularFileChangedError(this.path); + await this.#assertNamed(before); + if (!(await this.#matches(this.bytes))) + throw new RegularFileChangedError(this.path); + if (!sameRegularFileState(before, await this.owner.handle.stat())) + throw new RegularFileChangedError(this.path); + await this.#assertNamed(before); + } + + /** Restore through the owned descriptor; a changed name never grants write authority. */ + async restore(): Promise { + if (!this.#attempted || this.#restored) return; + // A rejected exclusive link or rename may leave the original selection intact. + const named = await optionalState(this.path); + if ( + this.original !== undefined && + named !== undefined && + sameRegularFileState(this.original.state, named) + ) + return; + if (this.original === undefined && named === undefined) return; + await this.assertCurrent(); + if (this.original === undefined) + throw new Error( + "First-install skill recovery requires manual removal; pathname deletion cannot prove ownership", + ); + const original = this.original; + for (let position = 0; position < original.bytes.length;) { + const { bytesWritten } = await this.owner.handle.write( + original.bytes, + position, + original.bytes.length - position, + position, + ); + if (bytesWritten === 0) + throw new Error("Skill restoration made no write progress"); + position += bytesWritten; + } + await this.owner.handle.truncate(original.bytes.length); + await this.owner.handle.chmod(original.mode); + await this.owner.handle.sync(); + await this.#assertNamed(await this.owner.handle.stat()); + if (!(await this.#matches(original.bytes))) + throw new RegularFileChangedError(this.path); + this.#restored = true; + } + + close(): Promise { + return this.lease.release(); + } + + async #assertNamed(expected: Stats): Promise { + const named = await lstat(this.path); + if ( + expected.ino === 0 || + named.isSymbolicLink() || + !sameRegularFileState(expected, named) + ) + throw new RegularFileChangedError(this.path); + } + + async #matches(expected: Buffer): Promise { + if ((await this.owner.handle.stat()).size !== expected.length) return false; + const chunk = Buffer.allocUnsafe(Math.min(64 * 1024, expected.length)); + for (let position = 0; position < expected.length;) { + const { bytesRead } = await this.owner.handle.read( + chunk, + 0, + Math.min(chunk.length, expected.length - position), + position, + ); + if ( + bytesRead === 0 || + !chunk + .subarray(0, bytesRead) + .equals(expected.subarray(position, position + bytesRead)) + ) + return false; + position += bytesRead; + } + return true; + } + + async #assertSelected(): Promise { + const current = await optionalState(this.path); + if ( + this.original === undefined + ? current !== undefined + : current === undefined || + current.isSymbolicLink() || + !sameRegularFileState(this.original.state, current) + ) + throw new RegularFileChangedError(this.path); + } +} + +const optionalState = async (path: string): Promise => { + try { + return await lstat(path); + } catch (cause: unknown) { + if (cause instanceof Error && "code" in cause && cause.code === "ENOENT") + return undefined; + throw cause; + } +}; diff --git a/tests/boundary/filesystem/setupSkill.test.ts b/tests/boundary/filesystem/setupSkill.test.ts index 9cedb03e54..d3b16bb125 100644 --- a/tests/boundary/filesystem/setupSkill.test.ts +++ b/tests/boundary/filesystem/setupSkill.test.ts @@ -15,7 +15,7 @@ import { } from "node:fs/promises"; import { constants as fsConstants } from "node:fs"; import { dirname, join } from "node:path"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; import { @@ -33,7 +33,7 @@ import { import { createDoctorHostFixture } from "../../../src/application/Doctor.fixture.js"; import { TOOL_CONTRACTS } from "../../../src/contracts/toolContracts.js"; import { PRODUCT_IDENTITY } from "../../../src/identity.js"; -import writeFileAtomic from "write-file-atomic"; +import { SkillFilePublication } from "../../../src/application/SkillFilePublication.js"; import { z } from "zod"; import { skillReferenceIssues } from "../../../scripts/lib/docs-facts.mjs"; @@ -61,9 +61,7 @@ describe("skill recovery backup selection", () => { backup: async () => { throw new Error("A matching backup must not be copied again"); }, - write: (path, content, mode) => - writeFileAtomic(path, content, { encoding: "utf8", mode }), - remove: async (path) => rm(path, { force: true }), + prepare: SkillFilePublication.prepare, }); expect(result).toEqual({ ok: true, value: "installed" }); @@ -81,20 +79,25 @@ describe("skill recovery backup selection", () => { await mkdir(dirname(manifest), { recursive: true }); await writeFile(manifest, current); await writeFile(`${manifest}.rea.backup`, oldSnapshot); - let manifestWrites = 0; const operations: SkillInstallFileOperations = { mkdir: async (path) => mkdir(path, { recursive: true }).then(() => undefined), backup: async (source, target) => copyFile(source, target, fsConstants.COPYFILE_EXCL), - write: async (path, content, mode) => { - if (path === manifest && ++manifestWrites === 2) - throw new Error("injected rollback failure"); - await writeFileAtomic(path, content, { encoding: "utf8", mode }); - if (path === manifest && manifestWrites === 1) - throw new Error("injected failure after publication"); + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) { + const publish = publication.publish.bind(publication); + vi.spyOn(publication, "publish").mockImplementation(async () => { + await publish(); + throw new Error("injected failure after publication"); + }); + vi.spyOn(publication, "restore").mockRejectedValue( + new Error("injected rollback failure"), + ); + } + return publication; }, - remove: async (path) => rm(path, { force: true }), }; const result = await installCanonicalSkill( @@ -130,24 +133,28 @@ describe("skill recovery byte preservation", () => { await mkdir(dirname(manifest), { recursive: true }); await writeFile(manifest, original); await writeFile(firstBackup, "historical instructions V0\n"); - let manifestWrites = 0; const result = await installCanonicalSkill(home, [], {}, process.platform, { mkdir: async (path) => mkdir(path, { recursive: true }).then(() => undefined), backup: async (source, target) => copyFile(source, target, fsConstants.COPYFILE_EXCL), - write: async (path, content, mode) => { - if (path === manifest && ++manifestWrites === 2) - throw new Error("injected rollback failure"); - await writeFileAtomic(path, content, { encoding: "utf8", mode }); - if (path === manifest && manifestWrites === 1) - throw new Error("injected failure after publication"); + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) { + const publish = publication.publish.bind(publication); + vi.spyOn(publication, "publish").mockImplementation(async () => { + await publish(); + throw new Error("injected failure after publication"); + }); + } + return publication; }, - remove: async (path) => rm(path, { force: true }), }); if (result.ok) throw new Error("Expected the injected write failure"); expect(result.error.recoveryBackupPaths).toContain(recovery); expect(await readFile(recovery)).toEqual(original); + expect(await readFile(manifest)).toEqual(original); + expect(result.error.rollbackFailures).toEqual([]); expect(await readFile(firstBackup, "utf8")).toBe( "historical instructions V0\n", ); @@ -300,11 +307,10 @@ describe("canonical skill transaction", () => { if (backups === 2) throw new Error("injected backup failure"); await copyFile(source, target, fsConstants.COPYFILE_EXCL); }, - write: async (path, content, mode) => { - writes.push(path); - await writeFile(path, content, { mode }); + prepare: async (...args) => { + writes.push(args[0]); + return SkillFilePublication.prepare(...args); }, - remove: async (path) => rm(path, { force: true }), }; const result = await installCanonicalSkill( @@ -343,25 +349,26 @@ describe("canonical skill transaction", () => { await writeFile(manifest, "old manifest\n", { mode: 0o640 }); await writeFile(connection, "old connection\n", { mode: 0o644 }); await writeFile(native, "old native\n", { mode: 0o600 }); - const writes = new Map(); - const nativeWrites: (string | Buffer)[] = []; const operations: SkillInstallFileOperations = { mkdir: async (path) => { await mkdir(path, { recursive: true }); }, backup: async (source, target) => copyFile(source, target, fsConstants.COPYFILE_EXCL), - write: async (path, content, mode) => { - const attempt = (writes.get(path) ?? 0) + 1; - writes.set(path, attempt); - if (path === native) nativeWrites.push(content); - if (path === native && attempt === 2) - throw new Error("injected rollback failure"); - await writeFileAtomic(path, content, { encoding: "utf8", mode }); - if (path === native && attempt === 1) - throw new Error("injected ambiguous publication failure"); + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === native) { + const publish = publication.publish.bind(publication); + vi.spyOn(publication, "publish").mockImplementation(async () => { + await publish(); + throw new Error("injected ambiguous publication failure"); + }); + vi.spyOn(publication, "restore").mockRejectedValue( + new Error("injected rollback failure"), + ); + } + return publication; }, - remove: async (path) => rm(path, { force: true }), }; const result = await installCanonicalSkill( @@ -392,8 +399,6 @@ describe("canonical skill transaction", () => { await expectPermissionMode(connection, 0o644); expect(await readFile(native, "utf8")).not.toBe("old native\n"); expect(await readFile(`${native}.rea.backup`, "utf8")).toBe("old native\n"); - expect(nativeWrites).toHaveLength(2); - expect(nativeWrites[1]).toEqual(Buffer.from("old native\n", "utf8")); expect(result.error.message).toContain( `Existing-file backups remain at ${manifest}.rea.backup`, ); diff --git a/tests/boundary/filesystem/setupSkillPublication.test.ts b/tests/boundary/filesystem/setupSkillPublication.test.ts new file mode 100644 index 0000000000..336d88e1b2 --- /dev/null +++ b/tests/boundary/filesystem/setupSkillPublication.test.ts @@ -0,0 +1,293 @@ +import { constants as fsConstants } from "node:fs"; +import { + chmod, + copyFile, + mkdir, + readFile, + readdir, + rename, + stat, + writeFile, +} from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { describe, expect, it, vi } from "vitest"; + +import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js"; +import { OwnedStagedOutput } from "../../../src/filesystem/OwnedStagedOutput.js"; +import { SkillFilePublication } from "../../../src/application/SkillFilePublication.js"; +import { + canonicalSkillNeedsInstall, + installCanonicalSkill, + skillDestinations, + type SkillInstallFileOperations, +} from "../../../src/application/SetupSkill.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; + +const skillPublicationOperations: SkillInstallFileOperations = { + mkdir: async (path) => { + await mkdir(path, { recursive: true }); + }, + backup: (source, destination) => + copyFile(source, destination, fsConstants.COPYFILE_EXCL), + prepare: SkillFilePublication.prepare, +}; + +describe("skill publication ownership", () => { + it.each([ + { change: "edit", existing: true }, + { change: "replacement", existing: true }, + { change: "mode", existing: true }, + { change: "edit", existing: false }, + { change: "replacement", existing: false }, + ] as const)( + "preserves a user $change after publication (existing: $existing)", + async ({ change, existing }) => { + if (change === "mode" && process.platform === "win32") return; + const home = await createTestTempDirectory( + "rea-skill-published-conflict-", + ); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + if (existing) + await writeFile(manifest, "original instructions\n", { mode: 0o640 }); + let published: Buffer | undefined; + const userBytes = Buffer.from("new user instructions\n"); + const result = await installCanonicalSkill( + home, + [], + {}, + process.platform, + { + ...skillPublicationOperations, + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) { + const publish = publication.publish.bind(publication); + vi.spyOn(publication, "publish").mockImplementation(async () => { + await publish(); + published = await readFile(manifest); + if (change === "replacement") { + const replacement = `${manifest}.user`; + await writeFile(replacement, userBytes); + await rename(replacement, manifest); + } else if (change === "mode") await chmod(manifest, 0o600); + else await writeFile(manifest, userBytes); + throw new Error("post-publication failure"); + }); + } + return publication; + }, + }, + ); + if (result.ok) throw new Error("Expected controlled publication failure"); + expect(result.error.stage).toBe("write"); + expect(result.error.reason).toBe("post-publication failure"); + expect(result.error.rollbackFailures).toEqual([ + expect.objectContaining({ path: manifest }), + ]); + const expected = change === "mode" ? published : userBytes; + expect(await readFile(manifest)).toEqual(expected); + if (existing) + expect(await readFile(`${manifest}.rea.backup`, "utf8")).toBe( + "original instructions\n", + ); + else expect(result.error.recoveryBackupPaths).toEqual([]); + expect(await result.error.retryCleanup()).toEqual([]); + expect(await readFile(manifest)).toEqual(expected); + }, + ); + + it("preserves a concurrently created first-install destination", async () => { + const home = await createTestTempDirectory( + "rea-skill-first-install-conflict-", + ); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + const result = await installCanonicalSkill(home, [], {}, process.platform, { + ...skillPublicationOperations, + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) + await writeFile(manifest, "concurrent user file\n", { flag: "wx" }); + return publication; + }, + }); + if (result.ok) throw new Error("Expected destination conflict"); + expect(result.error.rollbackFailures).toEqual([]); + expect(await readFile(manifest, "utf8")).toBe("concurrent user file\n"); + expect(await readdir(dirname(manifest))).toEqual([ + "SKILL.md", + "references", + ]); + }); +}); + +describe("skill publication recovery", () => { + it.each([ + Buffer.from([0xff, 0xfe, 0x61, 0x00]), + Buffer.from("\ufefforiginal BOM instructions\r\n"), + ])( + "restores byte-exact originals and mode after ambiguous readback", + async (bytes) => { + const home = await createTestTempDirectory( + "rea-skill-readback-recovery-", + ); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile(manifest, bytes, { mode: 0o640 }); + const result = await installCanonicalSkill( + home, + [], + {}, + process.platform, + { + ...skillPublicationOperations, + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) + vi.spyOn(publication, "assertCurrent").mockRejectedValueOnce( + new Error("readback unavailable"), + ); + return publication; + }, + }, + ); + if (result.ok) throw new Error("Expected readback failure"); + expect(result.error.stage).toBe("readback"); + expect(result.error.reason).toBe("readback unavailable"); + expect(await readFile(manifest)).toEqual(bytes); + expect(await readFile(`${manifest}.rea.backup`)).toEqual(bytes); + if (process.platform !== "win32") + expect((await stat(manifest)).mode & 0o777).toBe(0o640); + expect( + result.error.rollbackFailures.every(({ reason }) => + reason.includes("First-install skill recovery"), + ), + ).toBe(true); + expect(result.error.rollbackFailures).toHaveLength(6); + }, + ); + + it("retains a first-install publication after failed verification", async () => { + const home = await createTestTempDirectory( + "rea-skill-first-install-readback-", + ); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + const result = await installCanonicalSkill(home, [], {}, process.platform, { + ...skillPublicationOperations, + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) + vi.spyOn(publication, "assertCurrent").mockRejectedValueOnce( + new Error("readback unavailable"), + ); + return publication; + }, + }); + if (result.ok) throw new Error("Expected readback failure"); + expect(result.error.rollbackFailures).toHaveLength(7); + expect( + result.error.rollbackFailures.every(({ reason }) => + reason.includes("manual removal"), + ), + ).toBe(true); + expect(await canonicalSkillNeedsInstall(home)).toBe(false); + expect( + (await readdir(dirname(manifest))).some((name) => + name.startsWith(".rea-skill-"), + ), + ).toBe(false); + }); + + it.each([false, true])( + "retains exact staging cleanup for retry (scope: %s)", + async (scoped) => { + const resources = scoped ? new ArtifactResourceScope() : undefined; + const home = await createTestTempDirectory("rea-skill-cleanup-retry-"); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + let closeAttempts = 0; + const result = await installCanonicalSkill( + home, + [], + {}, + process.platform, + { + ...skillPublicationOperations, + prepare: async (...args) => { + const publication = await SkillFilePublication.prepare(...args); + if (publication.path === manifest) { + const close = publication.lease.release.bind(publication.lease); + vi.spyOn(publication.lease, "release").mockImplementation( + async () => { + if (++closeAttempts <= 2) + throw new Error("staging close denied"); + await close(); + }, + ); + } + return publication; + }, + }, + resources, + ); + if (result.ok) throw new Error("Expected cleanup failure"); + expect(result.error.reason).toContain("cleanup incomplete"); + expect(result.error.rollbackFailures).toEqual([ + { + path: expect.stringContaining(".rea-skill-"), + reason: "staging close denied", + }, + ]); + expect(await canonicalSkillNeedsInstall(home)).toBe(false); + expect(resources?.hasPending).toBe(scoped ? true : undefined); + if (resources !== undefined) { + await resources.retryCleanup(); + expect(resources.hasPending).toBe(false); + } else expect(await result.error.retryCleanup()).toEqual([]); + expect(closeAttempts).toBe(3); + expect( + (await readdir(dirname(manifest))).some((name) => + name.startsWith(".rea-skill-"), + ), + ).toBe(false); + }, + ); +}); + +describe("skill staging preparation", () => { + it("retains the primary failure and staging owner when preparation cleanup fails", async () => { + const home = await createTestTempDirectory("rea-skill-prepare-cleanup-"); + const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md"); + await mkdir(dirname(manifest), { recursive: true }); + await writeFile(manifest, "original instructions\n"); + const ready = vi + .spyOn(OwnedStagedOutput.prototype, "assertReadyForPublication") + .mockRejectedValueOnce(new Error("staging verification denied")); + const close = vi + .spyOn(OwnedStagedOutput.prototype, "close") + .mockRejectedValueOnce(new Error("staging cleanup denied")) + .mockRejectedValueOnce(new Error("staging cleanup still denied")); + try { + const result = await installCanonicalSkill(home); + if (result.ok) throw new Error("Expected preparation failure"); + expect(result.error.reason).toContain("staging verification denied"); + expect(result.error.reason).toContain("staging cleanup denied"); + expect(result.error.rollbackFailures).toEqual([ + { + path: expect.stringContaining(".rea-skill-"), + reason: "staging cleanup still denied", + }, + ]); + expect(await readFile(manifest, "utf8")).toBe("original instructions\n"); + expect(await result.error.retryCleanup()).toEqual([]); + expect(close).toHaveBeenCalledTimes(3); + expect( + (await readdir(dirname(manifest))).some((name) => + name.startsWith(".rea-skill-"), + ), + ).toBe(false); + } finally { + ready.mockRestore(); + close.mockRestore(); + } + }); +});