fix(setup): retain skill publication ownership through rollback

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

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<string> =>
-  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<SkillFileState> =>
+): Promise<SkillFileSnapshot> =>
   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<string> =>
@@ -82,12 +82,7 @@ const canonicalSkillContent = (relativePath: string): Promise<string> =>
 export interface SkillInstallFileOperations {
   readonly mkdir: (path: string) => Promise<void>;
   readonly backup: (source: string, destination: string) => Promise<void>;
-  readonly write: (
-    path: string,
-    content: string | Buffer,
-    mode: number,
-  ) => Promise<void>;
-  readonly remove: (path: string) => Promise<void>;
+  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<readonly SkillInstallRollbackFailure[]> {
+    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<SkillFileState | undefined> => {
+): Promise<SkillFileSnapshot | undefined> => {
   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<readonly SkillInstallRollbackFailure[]> => {
   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<SkillInstallResult> => {
-  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<string, string>();
@@ -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,
       }),
     );

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<void>;
+}
+
+/** 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<SkillFilePublication> {
+    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<void> {
+    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<void> {
+    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<void> {
+    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<void> {
+    return this.lease.release();
+  }
+
+  async #assertNamed(expected: Stats): Promise<void> {
+    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<boolean> {
+    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<void> {
+    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<Stats | undefined> => {
+  try {
+    return await lstat(path);
+  } catch (cause: unknown) {
+    if (cause instanceof Error && "code" in cause && cause.code === "ENOENT")
+      return undefined;
+    throw cause;
+  }
+};

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<string, number>();
-    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`,
     );

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();
+    }
+  });
+});