fix(setup): retain verified skill installation outcome (#9630)

* fix(setup): retain verified skill installation outcome

* fix(setup): settle skill cleanup after host close

src/application/Setup.planning.test.ts

@@ -717,3 +717,32 @@ describe("setup workflow lifecycle and provider planning", () => {
     expect(host.configurations).toBe(0);
   });
 });
+
+it("records verified skill installation while its staging cleanup remains incomplete", async () => {
+  const host = new FakeSetupHost();
+  host.hopper = "/Applications/Hopper";
+  host.installSkill = async () =>
+    err(
+      new SetupSkillInstallError({
+        path: "/fixture/home/.agents/skills/reverse-engineer-anything/SKILL.md",
+        stage: "readback",
+        reason: "Skill publication cleanup incomplete",
+        completed: true,
+        rollbackFailures: [
+          { path: "/fixture/staging", reason: "close failed" },
+        ],
+        recoveryBackupPaths: [],
+        cause: new Error("close failed"),
+      }),
+    );
+  const result = await runSetup(
+    { ...options(true), clientIds: [], installSkill: true },
+    host,
+  );
+  expect(result).toMatchObject({
+    status: "needs_human",
+    code: "skill_install_failed",
+    appliedActions: ["installed_skill"],
+  });
+  expect(result.remediation).toContain("close failed");
+});

src/application/Setup.ts

@@ -43,10 +43,11 @@ export const runSetup = async (
   confirm?: SetupConfirmation,
 ): Promise<SetupResult> => {
   let result: SetupResult | undefined;
+  const pending: { skill?: SetupSkillInstallError } = {};
   let primaryFailure: unknown;
   let threw = false;
   try {
-    result = await runSetupWorkflow(options, host, confirm);
+    result = await runSetupWorkflow(options, host, confirm, pending);
   } catch (cause: unknown) {
     threw = true;
     primaryFailure = cause;
@@ -75,6 +76,26 @@ export const runSetup = async (
     );
   }
   if (result === undefined) throw new Error("Setup returned no result.");
+  if (pending.skill !== undefined) {
+    const settled =
+      pending.skill.cleanupOwners.length > 0 &&
+      pending.skill.cleanupOwners.every(({ lease }) => lease.released);
+    if (settled)
+      emitProgress(options, {
+        actionId: "install_skill",
+        label: "REA reverse-engineering skill",
+        state: "completed",
+      });
+    else
+      result = {
+        ...result,
+        status: "needs_human",
+        code: result.code ?? "skill_install_failed",
+        remediation:
+          combineRemediation(pending.skill.remediation, result.remediation) ??
+          pending.skill.remediation,
+      };
+  }
   if (cleanup === undefined) return result;
   const cleanupReason = setupCleanupReason(cleanup);
   const cleanupReport: AnalysisCleanupObservation = {
@@ -99,7 +120,8 @@ export const runSetup = async (
 const runSetupWorkflow = async (
   options: SetupOptions,
   host: SetupHost,
-  confirm?: SetupConfirmation,
+  confirm: SetupConfirmation | undefined,
+  pending: { skill?: SetupSkillInstallError },
 ): Promise<SetupResult> => {
   const appliedActions: string[] = [];
   let plannedActions: readonly SetupAction[] = [];
@@ -384,7 +406,9 @@ const runSetupWorkflow = async (
       selectedSkillClientIds,
     );
     if (skillFailure !== undefined) {
-      return withHopperFailure(await failSkill(skillFailure));
+      if (!skillFailure.completed)
+        return withHopperFailure(await failSkill(skillFailure));
+      pending.skill = skillFailure;
     }
   }
   const doctor = summarizeDoctor(await host.doctor(readinessScope));
@@ -487,10 +511,11 @@ const installSkillAction = async (
   });
   const skill = await host.installSkill(skillClientIds);
   if (!skill.ok) {
+    if (skill.error.completed) appliedActions.push("installed_skill");
     emitProgress(options, {
       actionId: "install_skill",
       label,
-      state: "failed",
+      state: skill.error.completed ? "warning" : "failed",
       detail: skill.error.message,
     });
     return skill.error;

src/application/SetupSkill.ts

@@ -109,6 +109,8 @@ export class SetupSkillInstallError extends Error {
   readonly reason: string;
   readonly rollbackFailures: readonly SkillInstallRollbackFailure[];
   readonly recoveryBackupPaths: readonly string[];
+  /** All selected files were published and verified before cleanup failed. */
+  readonly completed: boolean;
   readonly cleanupOwners: readonly SkillPublicationCleanupOwner[];
   readonly remediationAction: string;
 
@@ -123,17 +125,20 @@ export class SetupSkillInstallError extends Error {
     readonly reason: string;
     readonly rollbackFailures: readonly SkillInstallRollbackFailure[];
     readonly recoveryBackupPaths: readonly string[];
+    readonly completed?: boolean;
     readonly cleanupOwners?: readonly SkillPublicationCleanupOwner[];
     readonly remediationAction?: string;
     readonly cause: unknown;
   }) {
     const message = [
-      `REA skill ${input.stage} failed at ${input.path}.`,
+      input.completed === true
+        ? "REA skill files were installed and verified, but staging cleanup remains incomplete."
+        : `REA skill ${input.stage} failed at ${input.path}.`,
       `Reason: ${input.reason}`,
       ...(input.rollbackFailures.length === 0
         ? []
         : [
-            `Rollback also failed for ${input.rollbackFailures.map(({ path, reason }) => `${path} (${reason})`).join(", ")}.`,
+            `${input.completed === true ? "Cleanup" : "Rollback"} also failed for ${input.rollbackFailures.map(({ path, reason }) => `${path} (${reason})`).join(", ")}.`,
           ]),
       ...(input.recoveryBackupPaths.length === 0
         ? []
@@ -147,10 +152,13 @@ export class SetupSkillInstallError extends Error {
     this.reason = input.reason;
     this.rollbackFailures = input.rollbackFailures;
     this.recoveryBackupPaths = input.recoveryBackupPaths;
+    this.completed = input.completed ?? false;
     this.cleanupOwners = input.cleanupOwners ?? [];
     this.remediationAction =
       input.remediationAction ??
-      "Correct the reported skill installation failure and rerun setup.";
+      (input.completed === true
+        ? "Resolve the reported retained staging cleanup before continuing; the skill files were already installed and verified."
+        : "Correct the reported skill installation failure and rerun setup.");
   }
 
   get remediation(): string {
@@ -699,6 +707,7 @@ export const installCanonicalSkill = async (
     rollbackFailures.push(
       ...(await closeSkillPublications(cleanupOwners, resources)),
     );
+    if (verified && rollbackFailures.length === 0) return ok("installed");
     return err(
       new SetupSkillInstallError({
         path: failingPath,
@@ -706,6 +715,7 @@ export const installCanonicalSkill = async (
         reason: causeReason(cause),
         rollbackFailures,
         recoveryBackupPaths: [...recoveryPaths.values()],
+        completed: verified,
         cleanupOwners,
         cause,
       }),

tests/boundary/filesystem/setupSkillPublication.test.ts

@@ -15,6 +15,11 @@ import { describe, expect, it, vi } from "vitest";
 
 import { ArtifactResourceScope } from "../../../src/artifacts/ArtifactResourceScope.js";
 import { OwnedStagedOutput } from "../../../src/filesystem/OwnedStagedOutput.js";
+import { runSetup } from "../../../src/application/Setup.js";
+import {
+  FakeSetupHost,
+  options,
+} from "../../../src/application/Setup.fixture.js";
 import { SkillFilePublication } from "../../../src/application/SkillFilePublication.js";
 import {
   canonicalSkillNeedsInstall,
@@ -312,6 +317,10 @@ describe("skill publication recovery", () => {
       );
       if (result.ok) throw new Error("Expected cleanup failure");
       expect(result.error.reason).toContain("cleanup incomplete");
+      expect(result.error).toMatchObject({ completed: true });
+      expect(result.error.message).toContain("installed and verified");
+      expect(result.error.message).not.toContain("Rollback");
+      expect(result.error.remediation).toContain("staging cleanup");
       expect(result.error.rollbackFailures).toEqual([
         {
           path: expect.stringContaining(".rea-skill-"),
@@ -372,3 +381,168 @@ describe("skill staging preparation", () => {
     }
   });
 });
+
+it.each([false, true])(
+  "reports verified multi-file publication after transient cleanup succeeds (scope: %s)",
+  async (scoped) => {
+    const home = await createTestTempDirectory("rea-skill-transient-cleanup-");
+    const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md");
+    const resources = scoped ? new ArtifactResourceScope() : undefined;
+    let attempts = 0;
+    const result = await installCanonicalSkill(
+      home,
+      [],
+      {},
+      process.platform,
+      {
+        ...skillPublicationOperations,
+        prepare: async (...args) => {
+          const publication = await SkillFilePublication.prepare(...args);
+          if (publication.path === manifest) {
+            const release = publication.lease.release.bind(publication.lease);
+            vi.spyOn(publication.lease, "release").mockImplementation(
+              async () => {
+                if (++attempts === 1)
+                  throw new Error("transient staging close failure");
+                await release();
+              },
+            );
+          }
+          return publication;
+        },
+      },
+      resources,
+    );
+    expect(result).toEqual({ ok: true, value: "installed" });
+    expect(attempts).toBe(2);
+    expect(await canonicalSkillNeedsInstall(home)).toBe(false);
+    expect(resources?.hasPending).toBe(scoped ? false : undefined);
+  },
+);
+
+it("preserves a readback failure with mixed rollback and retryable cleanup failures", async () => {
+  const home = await createTestTempDirectory("rea-skill-mixed-finalization-");
+  const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md");
+  await mkdir(dirname(manifest), { recursive: true });
+  await writeFile(manifest, "original skill\n");
+  let attempts = 0;
+  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("selected readback failed"),
+        );
+        const release = publication.lease.release.bind(publication.lease);
+        vi.spyOn(publication.lease, "release").mockImplementation(async () => {
+          if (++attempts <= 1) throw new Error("staging cleanup failed");
+          await release();
+        });
+      }
+      return publication;
+    },
+  });
+  if (result.ok) throw new Error("Expected readback failure");
+  expect(result.error).toMatchObject({
+    completed: false,
+    stage: "readback",
+    reason: "selected readback failed",
+  });
+  expect(result.error.rollbackFailures).toEqual(
+    expect.arrayContaining([
+      expect.objectContaining({
+        reason: expect.stringContaining("manual removal"),
+      }),
+      expect.objectContaining({ reason: "staging cleanup failed" }),
+    ]),
+  );
+  expect(await readFile(manifest, "utf8")).toBe("original skill\n");
+  expect(await result.error.retryCleanup()).toEqual([]);
+});
+
+it.each(["settled", "unresolved", "hopper-failed"] as const)(
+  "finalizes verified skill installation with %s cleanup and independent outcomes",
+  async (scenario) => {
+    const home = await createTestTempDirectory("rea-skill-final-host-close-");
+    const manifest = join(skillDestinations(home, [])[0]!.path, "SKILL.md");
+    const resources = new ArtifactResourceScope();
+    const host = Object.assign(new FakeSetupHost(), {
+      close: async () => {
+        await resources.close();
+        return undefined;
+      },
+    });
+    if (scenario === "hopper-failed") host.hopperInstallSucceeds = false;
+    else host.hopper = "/Applications/Hopper";
+    let attempts = 0;
+    let blocked = scenario === "unresolved";
+    host.installSkill = () =>
+      installCanonicalSkill(
+        home,
+        [],
+        {},
+        process.platform,
+        {
+          ...skillPublicationOperations,
+          prepare: async (...args) => {
+            const publication = await SkillFilePublication.prepare(...args);
+            if (publication.path === manifest) {
+              const release = publication.lease.release.bind(publication.lease);
+              vi.spyOn(publication.lease, "release").mockImplementation(
+                async () => {
+                  if (++attempts <= 2 || blocked)
+                    throw new Error("temporary staging close failure");
+                  await release();
+                },
+              );
+            }
+            return publication;
+          },
+        },
+        resources,
+      );
+    const progress: string[] = [];
+    const result = await runSetup(
+      {
+        ...options(true, scenario === "hopper-failed"),
+        clientIds: [],
+        installSkill: true,
+        onProgress: (event) => {
+          if (event.actionId === "install_skill") progress.push(event.state);
+        },
+      },
+      host,
+    );
+    expect(attempts).toBe(3);
+    expect(await canonicalSkillNeedsInstall(home)).toBe(false);
+    expect(result.appliedActions).toContain("installed_skill");
+    if (scenario === "unresolved") {
+      expect(result).toMatchObject({
+        status: "needs_human",
+        code: "skill_install_failed",
+        cleanup: { resources: [expect.stringContaining(".rea-skill-")] },
+      });
+      expect(result.remediation).toContain("staging cleanup");
+      expect(resources.hasPending).toBe(true);
+      expect(progress).toEqual(["started", "warning"]);
+      blocked = false;
+      await resources.retryCleanup();
+      expect(resources.hasPending).toBe(false);
+    } else {
+      expect(resources.hasPending).toBe(false);
+      expect(progress).toEqual(["started", "warning", "completed"]);
+      if (scenario === "settled") {
+        expect(result.status).toBe("ready");
+        expect(result.code).toBeUndefined();
+        expect(result.remediation).toBeUndefined();
+      } else {
+        expect(result).toMatchObject({
+          status: "needs_human",
+          code: "download_failed",
+          remediation: "Download failed.",
+        });
+      }
+    }
+  },
+);