Merge pull request #235 from morluto/fix/open-bug-regressions

fix: address setup, upgrade, and process test regressions

scripts/verify-package.mjs

@@ -406,7 +406,7 @@ try {
   const aligned = json(alignedExecution.stdout);
   const secondExecution = await runWithStatus(
     cli,
-    ["setup", "--yes", "--install-hopper", "--json"],
+    ["setup", "--install-hopper", "--json"],
     environment,
   );
   const second = json(secondExecution.stdout);
@@ -414,17 +414,18 @@ try {
     const status = process.platform === "linux" ? "needs_human" : "ready";
     if (
       first.status !== status ||
-      second.status !== status ||
+      second.status !== "needs_confirmation" ||
       firstExecution.status !== (status === "ready" ? 0 : 1) ||
-      secondExecution.status !== (status === "ready" ? 0 : 1) ||
+      secondExecution.status !== 1 ||
       aligned.status !== status ||
       alignedExecution.status !== (status === "ready" ? 0 : 1) ||
       aligned.plannedActions.length !== 0 ||
       aligned.appliedActions.length !== 0 ||
+      !second.plannedActions.some(({ kind }) => kind === "install_hopper") ||
       second.appliedActions.length !== 0
     )
       throw new Error(
-        `packaged setup was not ready and idempotent without confirmation: ${JSON.stringify({ first, aligned, second })}`,
+        `packaged setup did not preserve idempotent defaults and explicit Hopper reinstall behavior: ${JSON.stringify({ first, aligned, second })}`,
       );
     for (const configPath of [claudeConfig, cursorConfig]) {
       const config = json(await readFile(configPath, "utf8"));

src/application/MacHopper.ts

@@ -55,6 +55,7 @@ export interface MacHopperInstallHost {
     source: string,
     destination: string,
     stage: string,
+    replaceExisting: boolean,
   ): Promise<boolean>;
   launcherReady(path: string): Promise<boolean>;
   openApplication(path: string): Promise<void>;
@@ -64,6 +65,7 @@ export interface MacHopperInstallHost {
 
 /** Download, verify, and install Hopper into the current user's Applications directory. */
 export const installMacHopper = async (
+  options: { readonly replaceExisting?: boolean } = {},
   host: MacHopperInstallHost = systemMacHopperInstallHost(),
 ): Promise<MacHopperInstallResult> => {
   let temporary: string | undefined;
@@ -83,7 +85,8 @@ export const installMacHopper = async (
       return { status: "failed", reason: "integrity" };
 
     const destination = host.destination();
-    if (await host.destinationExists(destination))
+    const destinationExists = await host.destinationExists(destination);
+    if (destinationExists && options.replaceExisting !== true)
       return { status: "failed", reason: "destination_exists" };
     temporary = await host.createTemporaryDirectory();
     const packagePath = join(temporary, "hopper.dmg");
@@ -97,7 +100,9 @@ export const installMacHopper = async (
     if (source === undefined || !(await host.validBundle(source)))
       return { status: "failed", reason: "bundle" };
     const stage = `${destination}.rea-stage`;
-    if (!(await host.installBundle(source, destination, stage)))
+    if (
+      !(await host.installBundle(source, destination, stage, destinationExists))
+    )
       return { status: "failed", reason: "copy" };
     const launcherPath = join(destination, "Contents/MacOS/hopper");
     if (!(await host.launcherReady(launcherPath)))
@@ -181,15 +186,27 @@ const systemMacHopperInstallHost = (): MacHopperInstallHost => ({
       return false;
     }
   },
-  async installBundle(source, destination, stage) {
+  async installBundle(source, destination, stage, replaceExisting) {
+    const previous = `${stage}-previous`;
     try {
       await mkdir(join(homedir(), "Applications"), { recursive: true });
       await rm(stage, { recursive: true, force: true });
+      await rm(previous, { recursive: true, force: true });
       await execFileAsync("ditto", [source, stage]);
+      await access(join(stage, "Contents/MacOS/hopper"));
+      if (replaceExisting) await rename(destination, previous);
       await rename(stage, destination);
+      if (replaceExisting) await rm(previous, { recursive: true, force: true });
       return true;
     } catch {
       await rm(stage, { recursive: true, force: true });
+      if (replaceExisting) {
+        try {
+          await rename(previous, destination);
+        } catch {
+          // The original destination was not moved, or rollback is impossible.
+        }
+      }
       return false;
     }
   },

src/application/Setup.ts

@@ -6,6 +6,7 @@ import {
   rm,
   writeFile,
 } from "node:fs/promises";
+import { constants as fsConstants } from "node:fs";
 import { homedir } from "node:os";
 import { dirname, resolve } from "node:path";
 import { z } from "zod";
@@ -68,7 +69,7 @@ export interface SetupHost {
   macosVersion(): Promise<string | undefined>;
   linuxDistribution(): Promise<LinuxDistribution | undefined>;
   hopperPath(): Promise<string | undefined>;
-  installHopper(): Promise<SetupHopperInstallResult>;
+  installHopper(replaceExisting: boolean): Promise<SetupHopperInstallResult>;
   detectedClients(): Promise<readonly SetupClient[]>;
   configureClient(
     client: SetupClient,
@@ -143,8 +144,13 @@ export const runSetup = async (
   const unsupported = await hostRemediation(host);
   if (unsupported !== undefined) return fail(unsupported);
   let hopperPath = await host.hopperPath();
-  const discovery = await discoverSetupActions(host, hopperPath);
-  const { detectedClients, installHopper, installSkill } = discovery;
+  const discovery = await discoverSetupActions(
+    host,
+    hopperPath,
+    options.installHopper,
+  );
+  const { installHopper, installSkill } = discovery;
+  let { detectedClients } = discovery;
   plannedActions = discovery.plannedActions;
   let approved = options.approved || plannedActions.length === 0;
   let interactiveApproval = false;
@@ -164,7 +170,7 @@ export const runSetup = async (
     };
 
   if (installHopper && (interactiveApproval || options.installHopper)) {
-    const installed = await host.installHopper();
+    const installed = await host.installHopper(options.installHopper);
     if (installed.status === "failed")
       return {
         status: "needs_human",
@@ -177,6 +183,12 @@ export const runSetup = async (
       };
     hopperPath = installed.launcherPath;
     appliedActions.push("installed_hopper");
+    detectedClients = await filterClientsNeedingConfigure(
+      host,
+      detectedClients,
+      hopperPath,
+      registrationCommand(),
+    );
   }
   const clientFailure = await configureDetectedClients({
     host,
@@ -215,6 +227,7 @@ export const runSetup = async (
 const discoverSetupActions = async (
   host: SetupHost,
   hopperPath: string | undefined,
+  forceHopperInstall: boolean,
 ) => {
   const initialDoctor = await host.doctor();
   const linuxHopperRepairNeeded = initialDoctor.checks.some(
@@ -223,7 +236,8 @@ const discoverSetupActions = async (
       (name === "hopper-demo-runtime" ||
         (name === "hopper-version" && detail === "/opt/hopper/bin/Hopper")),
   );
-  const installHopper = hopperPath === undefined || linuxHopperRepairNeeded;
+  const installHopper =
+    forceHopperInstall || hopperPath === undefined || linuxHopperRepairNeeded;
   const [detectedClients, installSkill] = await Promise.all([
     host.detectedClients(),
     host.skillNeedsInstall(),
@@ -306,11 +320,11 @@ const systemSetupHost = (): SetupHost => {
     macosVersion: () => doctorHost.macosVersion(),
     linuxDistribution: readLinuxDistribution,
     hopperPath: async () => (await runDoctor(undefined, doctorHost)).hopperPath,
-    installHopper: async () => {
+    installHopper: async (replaceExisting) => {
       const result =
         process.platform === "linux"
           ? await installLinuxHopper()
-          : await installMacHopper();
+          : await installMacHopper({ replaceExisting });
       if (result.status === "installed") return result;
       return setupInstallFailure(result.reason);
     },
@@ -387,11 +401,8 @@ export const configureJsonClient = async (
   let backupPath: string | undefined;
   if (original !== undefined) {
     backupPath = `${client.configPath}.rea.backup`;
-    try {
-      await copyFile(transactionPath, backupPath);
-    } catch {
+    if (!(await preserveConfigBackup(transactionPath, backupPath)))
       return { status: "failed", reason: "backup" };
-    }
   }
   document.mcpServers = {
     ...servers,
@@ -470,11 +481,11 @@ export const configureTomlClient = async (
     return { status: "unchanged" };
   const backupPath =
     original === undefined ? undefined : `${client.configPath}.rea.backup`;
-  try {
-    if (backupPath !== undefined) await copyFile(transactionPath, backupPath);
-  } catch {
+  if (
+    backupPath !== undefined &&
+    !(await preserveConfigBackup(transactionPath, backupPath))
+  )
     return { status: "failed", reason: "backup" };
-  }
   document.mcp_servers = {
     ...servers,
     [PRODUCT_IDENTITY.mcpServerKey]: desired,
@@ -516,6 +527,17 @@ const exists = async (path: string): Promise<boolean> => {
     return false;
   }
 };
+const preserveConfigBackup = async (
+  source: string,
+  destination: string,
+): Promise<boolean> => {
+  try {
+    await copyFile(source, destination, fsConstants.COPYFILE_EXCL);
+    return true;
+  } catch (cause: unknown) {
+    return cause instanceof Error && "code" in cause && cause.code === "EEXIST";
+  }
+};
 const objectSchema = z.record(z.string(), z.unknown());
 const parseOptionalObject = (value: unknown): Record<string, unknown> =>
   value === undefined ? {} : objectSchema.parse(value);

src/application/Upgrade.ts

@@ -208,8 +208,7 @@ const runCommand = (
 ): Promise<boolean> =>
   new Promise((resolveResult) => {
     const child = spawn(command, [...arguments_], {
-      stdio:
-        output === "human" ? "inherit" : ["inherit", process.stderr, "inherit"],
+      stdio: output === "human" ? "inherit" : ["inherit", "ignore", "inherit"],
     });
     child.once("error", () => resolveResult(false));
     child.once("exit", (code) => resolveResult(code === 0));

tests/macHopper.test.ts

@@ -19,6 +19,7 @@ class FakeMacHopperHost implements MacHopperInstallHost {
   unmounts = 0;
   cleanups = 0;
   opens = 0;
+  replaces = false;
 
   download = (url: string) =>
     Promise.resolve(
@@ -51,7 +52,15 @@ class FakeMacHopperHost implements MacHopperInstallHost {
   appBundle = () => Promise.resolve("/mounted/Hopper Disassembler.app");
   validBundle = () => Promise.resolve(this.valid);
   destinationExists = () => Promise.resolve(this.existing);
-  installBundle = () => Promise.resolve(this.copySucceeds);
+  installBundle = (
+    _source: string,
+    _destination: string,
+    _stage: string,
+    replaceExisting: boolean,
+  ) => {
+    this.replaces = replaceExisting;
+    return Promise.resolve(this.copySucceeds);
+  };
   launcherReady = () => Promise.resolve(this.launcherSucceeds);
   openApplication = () => {
     this.opens += 1;
@@ -67,7 +76,7 @@ class FakeMacHopperHost implements MacHopperInstallHost {
 describe("macOS Hopper installation", () => {
   it("installs, verifies, opens, and cleans the official package", async () => {
     const host = new FakeMacHopperHost();
-    await expect(installMacHopper(host)).resolves.toEqual({
+    await expect(installMacHopper({}, host)).resolves.toEqual({
       status: "installed",
       launcherPath:
         "/home/user/Applications/Hopper Disassembler.app/Contents/MacOS/hopper",
@@ -80,7 +89,7 @@ describe("macOS Hopper installation", () => {
   it("rejects an integrity mismatch before mounting", async () => {
     const host = new FakeMacHopperHost();
     host.badHash = true;
-    await expect(installMacHopper(host)).resolves.toEqual({
+    await expect(installMacHopper({}, host)).resolves.toEqual({
       status: "failed",
       reason: "integrity",
     });
@@ -91,16 +100,26 @@ describe("macOS Hopper installation", () => {
   it("refuses to overwrite an existing application", async () => {
     const host = new FakeMacHopperHost();
     host.existing = true;
-    await expect(installMacHopper(host)).resolves.toEqual({
+    await expect(installMacHopper({}, host)).resolves.toEqual({
       status: "failed",
       reason: "destination_exists",
     });
   });
 
+  it("replaces the managed application when explicitly requested", async () => {
+    const host = new FakeMacHopperHost();
+    host.existing = true;
+
+    await expect(
+      installMacHopper({ replaceExisting: true }, host),
+    ).resolves.toMatchObject({ status: "installed" });
+    expect(host.replaces).toBe(true);
+  });
+
   it("cleans and detaches after bundle validation fails", async () => {
     const host = new FakeMacHopperHost();
     host.valid = false;
-    await expect(installMacHopper(host)).resolves.toEqual({
+    await expect(installMacHopper({}, host)).resolves.toEqual({
       status: "failed",
       reason: "bundle",
     });

tests/processCapture.test.ts

@@ -967,7 +967,7 @@ describe("process capture adapter", () => {
     } finally {
       await rm(root, { recursive: true, force: true });
     }
-  });
+  }, 20_000);
 
   it("does not follow or disclose symlink targets outside declared roots", async () => {
     const root = await mkdtemp(join(tmpdir(), "rea-symlink-test-"));
@@ -1186,5 +1186,5 @@ describe("process capture adapter", () => {
     const { stdout } = await execFileAsync("ps", ["-axo", "command="]);
     expect(stdout).not.toContain(`${processFixture} tree-child`);
     expect(stdout).not.toContain(`${processFixture} tree-grandchild`);
-  });
+  }, 20_000);
 });

tests/setup.test.ts

@@ -22,8 +22,10 @@ class FakeSetupHost implements SetupHost {
   clients: readonly SetupClient[] = [];
   clientResults = new Map<string, ClientConfigurationResult>();
   hopperInstalls = 0;
+  hopperReplaceRequests: boolean[] = [];
   configurations = 0;
   skillInstalls = 0;
+  checkedHopperPaths: Array<string | undefined> = [];
   doctorHealthy: boolean | undefined;
   linuxDemoRuntimeMissing = false;
   unsupportedHopperVersion = false;
@@ -37,8 +39,11 @@ class FakeSetupHost implements SetupHost {
   linuxDistribution = (): Promise<LinuxDistribution | undefined> =>
     Promise.resolve(this.distribution);
   hopperPath = (): Promise<string | undefined> => Promise.resolve(this.hopper);
-  installHopper = (): Promise<SetupHopperInstallResult> => {
+  installHopper = (
+    replaceExisting: boolean,
+  ): Promise<SetupHopperInstallResult> => {
     this.hopperInstalls += 1;
+    this.hopperReplaceRequests.push(replaceExisting);
     this.linuxDemoRuntimeMissing = false;
     if (this.hopperInstallSucceeds) {
       this.hopper = "/manual/Hopper";
@@ -63,10 +68,15 @@ class FakeSetupHost implements SetupHost {
       this.clientResults.get(client.name) ?? { status: "configured" },
     );
   };
-  clientNeedsConfigure = (client: SetupClient): Promise<boolean> =>
-    Promise.resolve(
+  clientNeedsConfigure = (
+    client: SetupClient,
+    hopperPath: string | undefined,
+  ): Promise<boolean> => {
+    this.checkedHopperPaths.push(hopperPath);
+    return Promise.resolve(
       this.clientResults.get(client.name)?.status !== "unchanged",
     );
+  };
   skillNeedsInstall = (): Promise<boolean> =>
     Promise.resolve(this.skill !== "unchanged");
   installSkill = (): Promise<"installed" | "unchanged" | "failed"> => {
@@ -227,6 +237,26 @@ describe("setup workflow", () => {
     expect(host.hopperInstalls).toBe(0);
   });
 
+  it("reinstalls existing Hopper when explicitly requested", async () => {
+    const host = new FakeSetupHost();
+    host.hopper = "/Applications/Hopper";
+    host.skill = "unchanged";
+    host.clients = [{ name: "codex", configPath: "/codex.toml" }];
+    host.clientResults.set("codex", { status: "unchanged" });
+
+    const result = await runSetup(options(true, true), host);
+
+    expect(result.plannedActions.map(({ kind }) => kind)).toEqual([
+      "install_hopper",
+      "configure_client",
+    ]);
+    expect(result.appliedActions).toEqual(["installed_hopper"]);
+    expect(host.hopperInstalls).toBe(1);
+    expect(host.hopperReplaceRequests).toEqual([true]);
+    expect(host.configurations).toBe(0);
+    expect(host.checkedHopperPaths).toEqual(["/manual/Hopper"]);
+  });
+
   it("omits an aligned managed skill from an otherwise empty plan", async () => {
     const host = new FakeSetupHost();
     host.hopper = "/Applications/Hopper";

tests/setupConfig.test.ts

@@ -123,6 +123,8 @@ describe("JSON client configuration transaction", () => {
     const configPath = join(directory, "mcp.json");
     const hopperPath = "/Applications/Hopper v6.app/Contents/MacOS/hopper";
     const client = { name: "cursor", configPath };
+    const original = "{}\n";
+    await writeFile(configPath, original);
     expect(await configureJsonClient(client, hopperPath)).toMatchObject({
       status: "configured",
     });
@@ -138,6 +140,10 @@ describe("JSON client configuration transaction", () => {
     expect(await configureJsonClient(client, hopperPath)).toEqual({
       status: "unchanged",
     });
+    expect(
+      await configureJsonClient(client, "/opt/hopper/bin/Hopper"),
+    ).toMatchObject({ status: "configured" });
+    expect(await readFile(`${configPath}.rea.backup`, "utf8")).toBe(original);
   });
 
   it("refuses malformed existing JSON without overwriting it", async () => {