fix(setup): keep client and skill setup when the Hopper install fails (#5120)

* fix(setup): keep client and skill setup when the Hopper install fails

A failed Hopper package install returned from runSetup before any client
registration or skill install ran, so one optional provider's package
conflict (zlib-ng-compat on Arch and CachyOS) cost every other approved
change. Record the failure instead, apply the remaining plan without a
Hopper launcher, and report needs_human with the install's code and
remediation.

Fixes #5113

* fix(setup): keep the Hopper failure when a client write or the skill install fails too

* fix(setup): keep client and skill setup when the Hopper install fails

A failed Hopper package install returned from runSetup before any client
registration or skill install ran, so one optional provider's package
conflict (zlib-ng-compat on Arch and CachyOS) cost every other approved
change. Record the failure instead, apply the remaining plan without a
Hopper launcher, and report needs_human with the install's code and
remediation.

Fixes #5113

* fix(setup): keep the Hopper failure when a client write or the skill install fails too

---------

Co-authored-by: morluto <76467478+morluto@users.noreply.github.com>

docs/installation.md

@@ -696,10 +696,12 @@ manual dependency repair.
 
 When the package-manager step fails, setup reports the failing command and its
 captured output, so a package conflict is distinguishable from an authorization
-failure. On CachyOS, an unversioned compatibility package such as
-`zlib-ng-compat` may conflict with Hopper's versioned `zlib` dependency. Resolve
-the package conflict yourself or install Hopper's official package manually,
-then rerun `rea setup`.
+failure. Hopper is optional, so setup still applies the approved client
+registrations and skill, then reports `needs_human` with the Hopper failure. On
+Arch Linux and CachyOS, the unversioned compatibility package `zlib-ng-compat`
+may conflict with Hopper's versioned `zlib` dependency. Resolve the package
+conflict yourself, install Hopper's official package manually, or use another
+provider such as Ghidra; rerun `rea setup` after installing Hopper.
 
 The supported Linux demo forwards additional launches into its existing
 application, even across private displays. REA therefore reserves one Linux

src/application/Setup.configuration.test.ts

@@ -105,20 +105,6 @@ describe("setup workflow", () => {
     );
   });
 
-  it("reports a failed Hopper installation without configuring clients", async () => {
-    const host = new FakeSetupHost();
-    host.hopperInstallSucceeds = false;
-    host.clients = [{ name: "cursor", configPath: "/cursor.json" }];
-    const result = await runSetup(
-      { ...options(true, true), clientIds: [] },
-      host,
-    );
-    expect(result.status).toBe("needs_human");
-    expect(result.code).toBe("download_failed");
-    expect(result.remediation).toBe("Download failed.");
-    expect(host.configurations).toBe(0);
-  });
-
   it("blocks malformed client configuration before Hopper or file mutation", async () => {
     const host = new FakeSetupHost();
     host.clients = [
@@ -196,6 +182,63 @@ describe("setup workflow", () => {
   });
 });
 
+describe("optional Hopper installation failures", () => {
+  it("reports a failed Hopper installation and still configures the selected clients", async () => {
+    // Hopper is optional; a package conflict must not cost the approved client
+    // registrations and skill (#5113).
+    const host = new FakeSetupHost();
+    host.hopperInstallSucceeds = false;
+    host.clients = [{ name: "cursor", configPath: "/cursor.json" }];
+    const result = await runSetup(
+      {
+        ...options(true, true),
+        clientIds: ["cursor"],
+        skillClientIds: ["cursor"],
+      },
+      host,
+    );
+    expect(result.status).toBe("needs_human");
+    expect(result.code).toBe("download_failed");
+    expect(result.remediation).toBe("Download failed.");
+    expect(result.appliedActions).not.toContain("installed_hopper");
+    expect(host.configurations).toBe(1);
+    expect(host.configuredProviderEnvironments).toEqual([{}]);
+    expect(host.skillInstalls).toBe(1);
+  });
+
+  it("keeps the Hopper failure when a client write also fails", async () => {
+    const host = new FakeSetupHost();
+    host.hopperInstallSucceeds = false;
+    host.clients = [{ name: "cursor", configPath: "/cursor.json" }];
+    host.clientResults.set("cursor", { status: "failed", reason: "write" });
+    const result = await runSetup(
+      { ...options(true, true), clientIds: ["cursor"] },
+      host,
+    );
+    expect(result.code).toBe("download_failed");
+    expect(result.remediation).toMatch(/^Download failed\. ./);
+    expect(host.configurations).toBe(1);
+  });
+
+  it("keeps the Hopper repair steps when the skill install also fails", async () => {
+    const host = new FakeSetupHost();
+    host.hopperInstallSucceeds = false;
+    host.skill = "failed";
+    host.clients = [{ name: "cursor", configPath: "/cursor.json" }];
+    const result = await runSetup(
+      {
+        ...options(true, true),
+        clientIds: ["cursor"],
+        skillClientIds: ["cursor"],
+      },
+      host,
+    );
+    expect(result.code).toBe("skill_install_failed");
+    expect(result.remediation).toMatch(/^Download failed\. ./);
+    expect(host.skillInstalls).toBe(1);
+  });
+});
+
 describe("configured Linux Hopper selection", () => {
   it("keeps an explicitly configured launcher even when demo checks fail", async () => {
     const host = new FakeSetupHost("linux");

src/application/Setup.ts

@@ -31,6 +31,7 @@ import { AnalysisError } from "../domain/analysisErrorBase.js";
 import { analysisErrorWithCleanupFailure } from "../domain/analysisErrorCleanup.js";
 import { ProviderCleanupError } from "../domain/providerCleanupError.js";
 import type { SetupSkillInstallError } from "./SetupSkill.js";
+import type { SetupHopperInstallFailure } from "./SetupInstallFailure.js";
 
 /** Discover, approve, and apply setup actions idempotently. */
 export const runSetup = async (
@@ -312,6 +313,7 @@ const runSetupWorkflow = async (
   let selectedClients: readonly SetupClient[] =
     planSelection.clientsToConfigure;
   let hopperWarning: string | undefined;
+  let hopperFailure: SetupHopperInstallFailure | undefined;
   if (
     planSelection.installHopper &&
     (interactiveApproval || options.installHopper)
@@ -319,21 +321,20 @@ const runSetupWorkflow = async (
     const install = await installHopperAction({
       host,
       options,
-      plannedActions,
       appliedActions,
-      clients,
       selectedClients,
       providerEnvironment,
-      doctorScope: readinessScope,
-      clientStates,
     });
-    if ("failure" in install) return install.failure;
-    ({
-      hopperPath,
-      providerEnvironment,
-      selectedClients,
-      warning: hopperWarning,
-    } = install);
+    // Hopper is optional: a failed install still applies the client
+    // registrations and skill approved with it (#5113).
+    if ("failure" in install) hopperFailure = install.failure;
+    else
+      ({
+        hopperPath,
+        providerEnvironment,
+        selectedClients,
+        warning: hopperWarning,
+      } = install);
   }
   const clientFailure = await configureDetectedClients({
     host,
@@ -346,7 +347,20 @@ const runSetupWorkflow = async (
       ? {}
       : { onProgress: options.onProgress }),
   });
-  if (clientFailure !== undefined) return fail(clientFailure);
+  // A later failure must not hide a failed Hopper install: keep its repair
+  // steps, and its code when the later failure carries none.
+  const withHopperFailure = (result: SetupResult): SetupResult =>
+    hopperFailure === undefined
+      ? result
+      : {
+          ...result,
+          code: result.code ?? hopperFailure.code,
+          remediation:
+            combineRemediation(hopperFailure.remediation, result.remediation) ??
+            hopperFailure.remediation,
+        };
+  if (clientFailure !== undefined)
+    return withHopperFailure(await fail(clientFailure));
   if (planSelection.installSkill) {
     const skillFailure = await installSkillAction(
       host,
@@ -355,18 +369,19 @@ const runSetupWorkflow = async (
       selectedSkillClientIds,
     );
     if (skillFailure !== undefined) {
-      return failSkill(skillFailure);
+      return withHopperFailure(await failSkill(skillFailure));
     }
   }
   const doctor = summarizeDoctor(await host.doctor(readinessScope));
   const remediation = combineRemediation(
-    finalSetupRemediation(
-      host.platform,
-      appliedActions.includes("installed_hopper"),
-      doctor.healthy,
-      // Offer --install-hopper only where the explicit installation gate accepts it.
-      hopperPath === undefined && planDiscovery.hopperInstall.supported,
-    ),
+    hopperFailure?.remediation ??
+      finalSetupRemediation(
+        host.platform,
+        appliedActions.includes("installed_hopper"),
+        doctor.healthy,
+        // Offer installation only where the explicit gate accepts it.
+        hopperPath === undefined && planDiscovery.hopperInstall.supported,
+      ),
     hopperWarning,
     manualRemediation,
   );
@@ -378,19 +393,16 @@ const runSetupWorkflow = async (
     doctor,
     clientStates,
     ...(remediation === undefined ? {} : { remediation }),
+    ...(hopperFailure === undefined ? {} : { code: hopperFailure.code }),
   };
 };
 
 const installHopperAction = async (input: {
   readonly host: SetupHost;
   readonly options: SetupOptions;
-  readonly plannedActions: readonly SetupAction[];
   readonly appliedActions: string[];
-  readonly clients: Readonly<Record<string, ClientConfigurationResult>>;
   readonly selectedClients: readonly SetupClient[];
   readonly providerEnvironment: SetupProviderEnvironment;
-  readonly doctorScope: DoctorScope | undefined;
-  readonly clientStates: readonly SetupClientState[];
 }) => {
   const label = "Hopper deep-analysis provider";
   emitProgress(input.options, {
@@ -406,18 +418,7 @@ const installHopperAction = async (input: {
       state: "failed",
       detail: installed.remediation,
     });
-    return {
-      failure: {
-        status: "needs_human" as const,
-        plannedActions: input.plannedActions,
-        appliedActions: input.appliedActions,
-        clients: input.clients,
-        doctor: summarizeDoctor(await input.host.doctor(input.doctorScope)),
-        clientStates: input.clientStates,
-        code: installed.code,
-        remediation: installed.remediation,
-      },
-    };
+    return { failure: installed };
   }
   input.appliedActions.push("installed_hopper");
   const warning = [installed.warning, installed.cleanupFailure]