From 70e4653feb03dfa729cd1ec7346955db682dba97 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Mon, 9 Nov 2026 20:28:25 +0800 Subject: [PATCH] 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 | 29 +++ src/application/Setup.ts | 33 +++- src/application/SetupSkill.ts | 16 +- .../filesystem/setupSkillPublication.test.ts | 174 ++++++++++++++++++ 4 files changed, 245 insertions(+), 7 deletions(-) diff --git a/src/application/Setup.planning.test.ts b/src/application/Setup.planning.test.ts index 2b7dd3a07c..2e6d35338a 100644 --- a/src/application/Setup.planning.test.ts +++ b/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"); +}); diff --git a/src/application/Setup.ts b/src/application/Setup.ts index 754b902013..4cd51383be 100644 --- a/src/application/Setup.ts +++ b/src/application/Setup.ts @@ -43,10 +43,11 @@ export const runSetup = async ( confirm?: SetupConfirmation, ): Promise => { 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 => { 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; diff --git a/src/application/SetupSkill.ts b/src/application/SetupSkill.ts index 56947b8d7c..f66e7248e7 100644 --- a/src/application/SetupSkill.ts +++ b/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, }), diff --git a/tests/boundary/filesystem/setupSkillPublication.test.ts b/tests/boundary/filesystem/setupSkillPublication.test.ts index f487694aa0..e3c68ddd72 100644 --- a/tests/boundary/filesystem/setupSkillPublication.test.ts +++ b/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.", + }); + } + } + }, +);