From d9c9025ffbb950b85202dab640677ba0233ec012 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sun, 12 Jul 2026 06:53:31 +0800 Subject: [PATCH] Improve Hopper session reliability --- bridge/hopper_bridge.py | 12 +++--- src/application/BinarySession.ts | 12 +----- src/application/Doctor.ts | 13 +++--- src/application/EnhancedTools.ts | 13 +++--- src/application/Setup.ts | 12 +++--- src/application/homebrew.ts | 17 ++++++++ src/hopper/HopperClient.ts | 74 +++++++++++++++++--------------- src/server/createServer.ts | 4 +- tests/binarySession.test.ts | 8 ++-- tests/createServer.test.ts | 13 ++++++ tests/hopperClient.test.ts | 26 +++++++++++ 11 files changed, 124 insertions(+), 80 deletions(-) create mode 100644 src/application/homebrew.ts diff --git a/bridge/hopper_bridge.py b/bridge/hopper_bridge.py index 2b1a3a456..d55b89ca9 100644 --- a/bridge/hopper_bridge.py +++ b/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") diff --git a/src/application/BinarySession.ts b/src/application/BinarySession.ts index b0e0dab52..9a1785be1 100644 --- a/src/application/BinarySession.ts +++ b/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 { - 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> { diff --git a/src/application/Doctor.ts b/src/application/Doctor.ts index d83411553..95615048d 100644 --- a/src/application/Doctor.ts +++ b/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")]; diff --git a/src/application/EnhancedTools.ts b/src/application/EnhancedTools.ts index 9868a0b5e..3e9b30b8c 100644 --- a/src/application/EnhancedTools.ts +++ b/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(); + 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 }); + } } } } diff --git a/src/application/Setup.ts b/src/application/Setup.ts index 282b25adc..2f35b41d3 100644 --- a/src/application/Setup.ts +++ b/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 => { - 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); diff --git a/src/application/homebrew.ts b/src/application/homebrew.ts new file mode 100644 index 000000000..34e529984 --- /dev/null +++ b/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 ( + probe: (command: string) => Promise, +): Promise => { + for (const command of HOMEBREW_COMMANDS) { + const result = await probe(command); + if (result !== undefined) return result; + } + return undefined; +}; diff --git a/src/hopper/HopperClient.ts b/src/hopper/HopperClient.ts index 08fd4d9f7..4b54681be 100644 --- a/src/hopper/HopperClient.ts +++ b/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(); - readonly #abandoned = new Set(); #socket: Socket | undefined; #launch: BridgeLaunch | undefined; #directory: string | undefined; @@ -63,6 +61,7 @@ export class HopperClient { #buffer = ""; #closing = false; #startPromise: Promise> | undefined; + #closePromise: Promise | 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 { - if (this.#closing) return; + close(): Promise { + this.#closePromise ??= Promise.resolve().then(() => this.#close()); + return this.#closePromise; + } + + async #close(): Promise { 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> { @@ -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): 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); } diff --git a/src/server/createServer.ts b/src/server/createServer.ts index 2c08facb3..93c87b5fb 100644 --- a/src/server/createServer.ts +++ b/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); diff --git a/tests/binarySession.test.ts b/tests/binarySession.test.ts index dc63477c4..fb6381565 100644 --- a/tests/binarySession.test.ts +++ b/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); }); }); diff --git a/tests/createServer.test.ts b/tests/createServer.test.ts index 2a9cc3483..47c3da51f 100644 --- a/tests/createServer.test.ts +++ b/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(); + }); }); diff --git a/tests/hopperClient.test.ts b/tests/hopperClient.test.ts index f4cdb1b7e..111598739 100644 --- a/tests/hopperClient.test.ts +++ b/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; + }); });