Improve Hopper session reliability

bridge/hopper_bridge.py

@@ -132,19 +132,13 @@ def _dispatch(method, params):
         return _selected_document
 
     document = _document(params.get("document"))
-    address = _address(document, params.get("address")) if method not in (
-        "list_segments", "list_procedures", "list_strings", "list_names",
-        "search_procedures", "search_strings", "procedure_address",
-        "procedure_info", "procedure_assembly", "procedure_pseudo_code",
-        "procedure_callers", "procedure_callees", "set_addresses_names",
-        "list_bookmarks", "current_address", "current_procedure"
-    ) else None
 
     if method == "current_address":
         return _hex(document.getCurrentAddress())
     if method == "current_procedure":
         return _procedure_name(_procedure(document))
     if method == "goto_address":
+        address = _address(document, params.get("address"))
         document.moveCursorAtAddress(address)
         return _hex(address)
     if method in ("address_name", "comment", "inline_comment", "xrefs"):
@@ -208,10 +202,12 @@ def _dispatch(method, params):
             length = sum(max(0, block.getEndingAddress() - block.getStartingAddress()) for block in blocks)
             return {"name": _procedure_name(procedure), "entrypoint": _hex(procedure.getEntryPoint()), "basicblock_count": procedure.getBasicBlockCount(), "length": length, "signature": procedure.signatureString(), "locals": procedure.getLocalVariableList()}
     if method == "set_address_name":
+        address = _address(document, params.get("address"))
         return document.setNameAtAddress(address, params["name"])
     if method == "set_addresses_names":
         return {key: document.setNameAtAddress(_address(document, key), value) for key, value in params["names"].items()}
     if method in ("set_comment", "set_inline_comment"):
+        address = _address(document, params.get("address"))
         segment = _segment(document, address)
         setter = segment.setCommentAtAddress if method == "set_comment" else segment.setInlineCommentAtAddress
         getter = segment.getCommentAtAddress if method == "set_comment" else segment.getInlineCommentAtAddress
@@ -220,9 +216,11 @@ def _dispatch(method, params):
     if method == "list_bookmarks":
         return [{"address": _hex(item), "name": document.getBookmarkName(item)} for item in document.getBookmarks()]
     if method == "set_bookmark":
+        address = _address(document, params.get("address"))
         document.setBookmarkAtAddress(address, params.get("name"))
         return document.hasBookmarkAtAddress(address)
     if method == "unset_bookmark":
+        address = _address(document, params.get("address"))
         document.removeBookmarkAtAddress(address)
         return not document.hasBookmarkAtAddress(address)
     raise ValueError("Unknown bridge method")

src/application/BinarySession.ts

@@ -39,21 +39,18 @@ export class BinarySession implements HopperToolPort {
         return ok(parsed.value);
       await this.#drainCalls();
       const previous = this.#active;
-      this.#active = undefined;
-      await previous?.client.close();
       const client = this.createClient(parsed.value);
       const started = await client.callTool("health", {}, options);
       if (!started.ok) {
         await client.close();
-        if (previous !== undefined) await this.#restore(previous.target);
         return started;
       }
       if (isAborted(options.signal)) {
         await client.close();
-        if (previous !== undefined) await this.#restore(previous.target);
         return err(new HopperCancelledError());
       }
       this.#active = { target: parsed.value, client };
+      await previous?.client.close();
       return ok(parsed.value);
     });
   }
@@ -114,13 +111,6 @@ export class BinarySession implements HopperToolPort {
     await Promise.allSettled([...this.#calls]);
   }
 
-  async #restore(target: BinaryTarget): Promise<void> {
-    const client = this.createClient(target);
-    const started = await client.callTool("health", {});
-    if (started.ok) this.#active = { target, client };
-    else await client.close();
-  }
-
   async #waitForTransition(
     signal?: AbortSignal,
   ): Promise<Result<undefined, HopperCancelledError>> {

src/application/Doctor.ts

@@ -6,6 +6,8 @@ import { join } from "node:path";
 import { execFile } from "node:child_process";
 import { promisify } from "node:util";
 
+import { probeHomebrew } from "./homebrew.js";
+
 const execFileAsync = promisify(execFile);
 const DEFAULT_HOPPER =
   "/Applications/Hopper Disassembler.app/Contents/MacOS/hopper";
@@ -119,11 +121,7 @@ export const systemDoctorHost = (): DoctorHost => ({
     }
   },
   async brewHopperPath() {
-    for (const command of [
-      "brew",
-      "/opt/homebrew/bin/brew",
-      "/usr/local/bin/brew",
-    ]) {
+    return probeHomebrew(async (command) => {
       try {
         const prefix = (
           await execFileAsync(command, [
@@ -134,10 +132,9 @@ export const systemDoctorHost = (): DoctorHost => ({
         ).stdout.trim();
         return `${prefix}/Hopper Disassembler.app/Contents/MacOS/hopper`;
       } catch {
-        // Try the next standard Homebrew executable location.
+        return undefined;
       }
-    }
-    return undefined;
+    });
   },
   async manualHopperPaths() {
     const roots = ["/Applications", join(homedir(), "Applications")];

src/application/EnhancedTools.ts

@@ -131,7 +131,7 @@ export class EnhancedTools {
     const relation = input.direction === "forward" ? "callees" : "callers";
     const tool =
       input.direction === "forward" ? "procedure_callees" : "procedure_callers";
-    const visited = new Set<string>();
+    const discovered = new Set([input.address]);
     const queue: Array<{ address: string; depth: number }> = [
       { address: input.address, depth: 0 },
     ];
@@ -140,14 +140,9 @@ export class EnhancedTools {
 
     while (queueIndex < queue.length) {
       const current = queue[queueIndex++];
-      if (
-        current === undefined ||
-        visited.has(current.address) ||
-        current.depth >= input.depth
-      ) {
+      if (current === undefined || current.depth >= input.depth) {
         continue;
       }
-      visited.add(current.address);
       const level = String(current.depth);
       graph[level] ??= [];
 
@@ -177,8 +172,10 @@ export class EnhancedTools {
       });
       if (current.depth + 1 < input.depth) {
         for (const address of related.value) {
-          if (!visited.has(address))
+          if (!discovered.has(address)) {
+            discovered.add(address);
             queue.push({ address, depth: current.depth + 1 });
+          }
         }
       }
     }

src/application/Setup.ts

@@ -15,6 +15,7 @@ import { z } from "zod";
 
 import { PRODUCT_IDENTITY } from "../identity.js";
 import { runDoctor, systemDoctorHost } from "./Doctor.js";
+import { probeHomebrew } from "./homebrew.js";
 
 const execFileAsync = promisify(execFile);
 
@@ -271,13 +272,10 @@ const commandSucceeds = async (
   }
 };
 const brewSucceeds = async (args: readonly string[]): Promise<boolean> => {
-  for (const command of [
-    "brew",
-    "/opt/homebrew/bin/brew",
-    "/usr/local/bin/brew",
-  ])
-    if (await commandSucceeds(command, args)) return true;
-  return false;
+  const result = await probeHomebrew(async (command) =>
+    (await commandSucceeds(command, args)) ? true : undefined,
+  );
+  return result === true;
 };
 const major = (version: string): number =>
   Number.parseInt(version.split(".")[0] ?? "0", 10);

src/application/homebrew.ts

@@ -0,0 +1,17 @@
+/** Standard executable names and locations used to discover Homebrew. */
+export const HOMEBREW_COMMANDS = [
+  "brew",
+  "/opt/homebrew/bin/brew",
+  "/usr/local/bin/brew",
+] as const;
+
+/** Return the first successful result from the standard Homebrew executables. */
+export const probeHomebrew = async <T>(
+  probe: (command: string) => Promise<T | undefined>,
+): Promise<T | undefined> => {
+  for (const command of HOMEBREW_COMMANDS) {
+    const result = await probe(command);
+    if (result !== undefined) return result;
+  }
+  return undefined;
+};

src/hopper/HopperClient.ts

@@ -21,7 +21,6 @@ import {
 } from "./protocol.js";
 
 const MAX_LINE_BYTES = 10 * 1024 * 1024;
-const MAX_ABANDONED_REQUESTS = 1_024;
 const SESSION_ROOT = process.platform === "darwin" ? "/tmp" : tmpdir();
 
 export interface HopperClientOptions {
@@ -54,7 +53,6 @@ export class HopperClient {
   > &
     HopperClientOptions;
   readonly #pending = new Map<number, PendingRequest>();
-  readonly #abandoned = new Set<number>();
   #socket: Socket | undefined;
   #launch: BridgeLaunch | undefined;
   #directory: string | undefined;
@@ -63,6 +61,7 @@ export class HopperClient {
   #buffer = "";
   #closing = false;
   #startPromise: Promise<Result<HopperServerInfo, HopperError>> | undefined;
+  #closePromise: Promise<void> | undefined;
 
   constructor(options: HopperClientOptions) {
     this.#options = {
@@ -136,29 +135,37 @@ export class HopperClient {
   }
 
   /** Stop the bridge, settle outstanding requests, and remove session artifacts. */
-  async close(): Promise<void> {
-    if (this.#closing) return;
+  close(): Promise<void> {
+    this.#closePromise ??= Promise.resolve().then(() => this.#close());
+    return this.#closePromise;
+  }
+
+  async #close(): Promise<void> {
     this.#closing = true;
-    const socket = this.#socket;
-    if (socket !== undefined && !socket.destroyed) {
-      await this.#request("shutdown", {}, { timeoutMs: 500 }).catch(() =>
-        err(new HopperProcessError(null)),
-      );
+    try {
+      const socket = this.#socket;
+      if (socket !== undefined && !socket.destroyed) {
+        await this.#request("shutdown", {}, { timeoutMs: 500 }).catch(() =>
+          err(new HopperProcessError(null)),
+        );
+      }
+      this.#failAll(new HopperProcessError(null));
+      socket?.destroy();
+      this.#socket = undefined;
+      if (this.#launch?.ownsProcessLifetime === true) {
+        this.#launch.process.kill("SIGTERM");
+      }
+      this.#launch = undefined;
+      if (this.#directory !== undefined) {
+        await rm(this.#directory, { recursive: true, force: true });
+        this.#directory = undefined;
+      }
+      this.#token = undefined;
+      this.#startPromise = undefined;
+    } finally {
+      this.#closing = false;
+      this.#closePromise = undefined;
     }
-    this.#failAll(new HopperProcessError(null));
-    socket?.destroy();
-    this.#socket = undefined;
-    if (this.#launch?.ownsProcessLifetime === true) {
-      this.#launch.process.kill("SIGTERM");
-    }
-    this.#launch = undefined;
-    if (this.#directory !== undefined) {
-      await rm(this.#directory, { recursive: true, force: true });
-      this.#directory = undefined;
-    }
-    this.#token = undefined;
-    this.#startPromise = undefined;
-    this.#closing = false;
   }
 
   async #connect(socketPath: string): Promise<Result<undefined, HopperError>> {
@@ -256,17 +263,20 @@ export class HopperClient {
 
   #onData(chunk: string): void {
     this.#buffer += chunk;
-    if (Buffer.byteLength(this.#buffer) > MAX_LINE_BYTES) {
-      this.#abortProtocol("Hopper response exceeded the maximum line size");
-      return;
-    }
     let newline = this.#buffer.indexOf("\n");
     while (newline >= 0) {
       const line = this.#buffer.slice(0, newline).trim();
       this.#buffer = this.#buffer.slice(newline + 1);
+      if (Buffer.byteLength(line) > MAX_LINE_BYTES) {
+        this.#abortProtocol("Hopper response exceeded the maximum line size");
+        return;
+      }
       if (line.length > 0) this.#onLine(line);
       newline = this.#buffer.indexOf("\n");
     }
+    if (Buffer.byteLength(this.#buffer) > MAX_LINE_BYTES) {
+      this.#abortProtocol("Hopper response exceeded the maximum line size");
+    }
   }
 
   #onLine(line: string): void {
@@ -275,9 +285,10 @@ export class HopperClient {
       this.#abortProtocol(parsed.error.message, parsed.error);
       return;
     }
-    if (this.#abandoned.delete(parsed.value.id)) return;
     if (!this.#pending.has(parsed.value.id)) {
-      this.#abortProtocol("Hopper returned an unknown response id");
+      if (parsed.value.id >= this.#nextId) {
+        this.#abortProtocol("Hopper returned an unknown response id");
+      }
       return;
     }
     this.#settle(parsed.value.id, responseResult(parsed.value));
@@ -301,11 +312,6 @@ export class HopperClient {
 
   #abandon(id: number, result: Result<JsonValue, HopperError>): void {
     if (!this.#pending.has(id)) return;
-    this.#abandoned.add(id);
-    if (this.#abandoned.size > MAX_ABANDONED_REQUESTS) {
-      const oldest = this.#abandoned.values().next();
-      if (!oldest.done) this.#abandoned.delete(oldest.value);
-    }
     this.#settle(id, result);
   }
 

src/server/createServer.ts

@@ -17,7 +17,9 @@ export const createServer = (
     {
       capabilities: { tools: {} },
       instructions:
-        "Reverse-engineering tools for Hopper Disassembler. Open a target with open_binary, then start with binary_overview.",
+        session === undefined
+          ? "Reverse-engineering tools for an active Hopper Disassembler target. Start with binary_overview."
+          : "Reverse-engineering tools for Hopper Disassembler. Open a target with open_binary, then start with binary_overview.",
     },
   );
   registerOfficialTools(server, hopper);

tests/binarySession.test.ts

@@ -24,7 +24,7 @@ describe("binary session", () => {
     expect(await session.close()).toEqual({ ok: true, value: null });
   });
 
-  it("rolls back a failed switch", async () => {
+  it("keeps the active client when a switch fails", async () => {
     directory = await mkdtemp(join(tmpdir(), "bb-session-"));
     const first = join(directory, "first.hop");
     const second = join(directory, "second.hop");
@@ -36,7 +36,7 @@ describe("binary session", () => {
     expect((await session.open(second)).ok).toBe(false);
     expect(session.status()).toMatchObject({ open: true });
     expect(JSON.stringify(session.status())).toContain("first.hop");
-    expect(created).toBe(3);
+    expect(created).toBe(2);
   });
 
   it("serializes concurrent opens and leaves the last target active", async () => {
@@ -130,11 +130,11 @@ describe("binary session", () => {
     await session.open(first);
     await session.open(second);
     expect(clients[1]?.closed).toBe(1);
-    expect(clients[0]?.closed).toBe(1);
+    expect(clients[0]?.closed).toBe(0);
     expect(JSON.stringify(session.status())).toContain("first.hop");
     await session.close();
     await session.close();
-    expect(clients[2]?.closed).toBe(1);
+    expect(clients[0]?.closed).toBe(1);
   });
 });
 

tests/createServer.test.ts

@@ -1,3 +1,4 @@
+import { Client, InMemoryTransport } from "@modelcontextprotocol/client";
 import { describe, expect, it } from "vitest";
 
 import { ok } from "../src/domain/result.js";
@@ -11,4 +12,16 @@ describe("beta.3 server composition", () => {
   it("constructs independent MCP server instances", () => {
     expect(createServer(hopper)).not.toBe(createServer(hopper));
   });
+
+  it("does not advertise unavailable session tools", async () => {
+    const server = createServer(hopper);
+    const client = new Client({ name: "composition-test", version: "1.0.0" });
+    const [clientTransport, serverTransport] =
+      InMemoryTransport.createLinkedPair();
+    await server.connect(serverTransport);
+    await client.connect(clientTransport);
+    expect(client.getInstructions()).not.toContain("open_binary");
+    await client.close();
+    await server.close();
+  });
 });

tests/hopperClient.test.ts

@@ -135,4 +135,30 @@ describe("HopperClient", () => {
       value: { value: "alive" },
     });
   });
+
+  it("ignores more than 1,024 late responses without corrupting the session", async () => {
+    const client = await startClient();
+    const timedOut = await Promise.all(
+      Array.from({ length: 1_025 }, (_, index) =>
+        client.callTool("echo", { index, delay: 150 }, { timeoutMs: 5 }),
+      ),
+    );
+    expect(timedOut.every((result) => !result.ok)).toBe(true);
+    await new Promise((resolve) => setTimeout(resolve, 200));
+    await expect(client.callTool("echo", { value: "alive" })).resolves.toEqual({
+      ok: true,
+      value: { value: "alive" },
+    });
+  });
+
+  it("makes concurrent close callers await the same shutdown", async () => {
+    const client = await startClient();
+    let firstSettled = false;
+    const first = client.close().then(() => {
+      firstSettled = true;
+    });
+    await client.close();
+    expect(firstSettled).toBe(true);
+    await first;
+  });
 });