From fd7da77fc02dcfaf7d1bc9f2fb240d71cdd27c59 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Tue, 20 Oct 2026 22:39:23 +0800 Subject: [PATCH] fix(hopper): verify bookmark names and clear existing labels Use the public rename API for an explicit empty existing name, retain omitted names and raw listing values, and verify fresh unnamed creation and explicit mutations by readback. Preserve independent verifier restorations and their primary failures. Refs #5475. Focused bridge regressions and the complete real-Hopper suite passed on macOS arm64 with Hopper 6.6.0-demo 42.6316032. The verifier used a temporary unavailable Ghidra override to meet its sole-candidate prerequisite; reporter Hopper 6.1 remains unverified. --- bridge/hopper_bridge.py | 46 ++++- scripts/lib/real-hopper-boundaries.mjs | 102 +++++++++- src/contracts/officialToolContracts.ts | 2 +- .../providers/hopper/hopperBookmarks.test.ts | 114 +++++++++++ tests/fixtures/hopperBookmarkMutationProbe.py | 184 ++++++++++++++++++ 5 files changed, 437 insertions(+), 11 deletions(-) create mode 100644 tests/boundary/providers/hopper/hopperBookmarks.test.ts create mode 100644 tests/fixtures/hopperBookmarkMutationProbe.py diff --git a/bridge/hopper_bridge.py b/bridge/hopper_bridge.py index 02be09c844..73504b620d 100644 --- a/bridge/hopper_bridge.py +++ b/bridge/hopper_bridge.py @@ -236,6 +236,15 @@ def _name_matches(document, address, name): return observed == name or (not name and observed is None) +def _bookmark_name(document, address): + getter = getattr(document, "getBookmarkName", None) + if not callable(getter): + raise CapabilityUnavailableError( + "Hopper's public Python API does not expose getBookmarkName" + ) + return getter(address) + + def _address(document, value=None): """Keep explicit hexadecimal coordinates distinct from symbol names.""" if value is None: @@ -994,15 +1003,46 @@ def _dispatch(method, params): observed = getter(address) return observed == comment or (comment == "" and observed is None) if method == "list_bookmarks": - return [{"address": _hex(item), "name": document.getBookmarkName(item)} for item in document.getBookmarks()] + return [{"address": _hex(item), "name": _bookmark_name(document, item)} for item in document.getBookmarks()] if method == "set_bookmark": address = _address(document, params.get("address")) _segment(document, address) name = params.get("name") if name is not None: _api_text(name, "Bookmark name") - document.setBookmarkAtAddress(address, name) - return document.hasBookmarkAtAddress(address) + has_bookmark = getattr(document, "hasBookmarkAtAddress", None) + if not callable(has_bookmark): + raise CapabilityUnavailableError( + "Hopper's public Python API does not expose hasBookmarkAtAddress" + ) + exists = has_bookmark(address) + if name is None and exists: + # Omission means preserve the existing bookmark and its name. + return True + if not callable(getattr(document, "getBookmarkName", None)): + raise CapabilityUnavailableError( + "Hopper's public Python API does not expose getBookmarkName" + ) + if name == "" and exists: + rename = getattr(document, "renameBookmarkAtAddress", None) + if not callable(rename): + raise CapabilityUnavailableError( + "Hopper's public Python API does not expose renameBookmarkAtAddress" + ) + rename(address, name) + else: + setter = getattr(document, "setBookmarkAtAddress", None) + if not callable(setter): + raise CapabilityUnavailableError( + "Hopper's public Python API does not expose setBookmarkAtAddress" + ) + setter(address, name) + if not has_bookmark(address): + return False + observed = _bookmark_name(document, address) + if name is None: + return observed is None or observed == "" + return observed == name or (name == "" and observed is None) if method == "unset_bookmark": address = _address(document, params.get("address")) # Permit removing a legacy orphan bookmark, but do not report success diff --git a/scripts/lib/real-hopper-boundaries.mjs b/scripts/lib/real-hopper-boundaries.mjs index 822accf3c3..5f11bade79 100644 --- a/scripts/lib/real-hopper-boundaries.mjs +++ b/scripts/lib/real-hopper-boundaries.mjs @@ -96,7 +96,14 @@ export async function verifyHopperBoundaryContracts( ...(await verifyHopperSearch(call, invalid, procedure)), ...(await verifyHopperRegexIsolation(client, options, call, address)), }; + const originalBookmark = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + let bookmarkReplacement; + const boundaryFailures = []; try { + if (originalBookmark !== undefined) + assert.equal(await call("unset_bookmark", { address }), true); if (foreignDocument !== undefined) { await invalid( "read_bytes", @@ -128,6 +135,49 @@ export async function verifyHopperBoundaryContracts( ); } + assert.equal( + await call("set_bookmark", { + address, + name: "REA boundary bookmark before clear", + }), + true, + ); + const beforeClear = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + assert.deepEqual(beforeClear, { + address, + name: "REA boundary bookmark before clear", + }); + assert.equal(await call("set_bookmark", { address, name: "" }), true); + const afterClear = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + assert.deepEqual(afterClear, { address, name: null }); + + const replacementName = "REA boundary bookmark replacement"; + assert.equal( + await call("set_bookmark", { address, name: replacementName }), + true, + ); + const afterReplacement = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + assert.deepEqual(afterReplacement, { address, name: replacementName }); + assert.equal(await call("set_bookmark", { address }), true); + const afterOmittedName = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + assert.deepEqual(afterOmittedName, { + address, + name: replacementName, + }); + bookmarkReplacement = { + existingNameCleared: afterClear?.name === null, + nonemptyNameReplaced: afterReplacement?.name === replacementName, + omittedNamePreserved: afterOmittedName?.name === replacementName, + }; + for (const [setter, getter] of [ ["set_comment", "comment"], ["set_inline_comment", "inline_comment"], @@ -283,14 +333,51 @@ export async function verifyHopperBoundaryContracts( "idle", "a live GUI bridge must remain healthy after its launcher helper exits", ); + } catch (error) { + boundaryFailures.push(error); + throw error; } finally { - await call("set_address_name", { address, name: originalName ?? "" }); - await call("set_comment", { address, comment: originalComment ?? "" }); - await call("set_inline_comment", { - address, - comment: originalInlineComment ?? "", - }); - await call("unset_bookmark", { address }); + const restorations = [ + () => call("set_address_name", { address, name: originalName ?? "" }), + () => call("set_comment", { address, comment: originalComment ?? "" }), + () => + call("set_inline_comment", { + address, + comment: originalInlineComment ?? "", + }), + async () => { + await call("unset_bookmark", { address }); + if (originalBookmark === undefined) return; + assert.equal( + await call("set_bookmark", { + address, + ...(originalBookmark.name === null + ? {} + : { name: originalBookmark.name }), + }), + true, + "Could not restore the original disposable-target bookmark", + ); + const restored = (await call("list_bookmarks")).find( + (item) => item.address === address, + ); + assert.deepEqual(restored, originalBookmark); + }, + ]; + const restorationErrors = []; + for (const restore of restorations) { + try { + await restore(); + } catch (error) { + restorationErrors.push(error); + } + } + if (restorationErrors.length > 0) { + throw new AggregateError( + [...boundaryFailures, ...restorationErrors], + "Could not restore all disposable Hopper boundary state", + ); + } } return { schemaCount: catalog.tools.length, @@ -301,6 +388,7 @@ export async function verifyHopperBoundaryContracts( navigationAndText, workflows, search, + bookmarkReplacement, }; } diff --git a/src/contracts/officialToolContracts.ts b/src/contracts/officialToolContracts.ts index cde2b9adb5..c16f1f1357 100644 --- a/src/contracts/officialToolContracts.ts +++ b/src/contracts/officialToolContracts.ts @@ -220,7 +220,7 @@ export const OFFICIAL_TOOL_CONTRACTS = [ ), official( "set_bookmark", - "Create or replace a bookmark at a mapped hexadecimal address and report success. This mutates navigation metadata; bookmarks are analyst-authored navigation aids, not binary evidence.", + "Create or update a bookmark at a mapped hexadecimal address and report readback success. Omit name to preserve an existing name or create an unnamed bookmark; an empty string clears the bookmark name. This mutates navigation metadata; bookmarks are analyst-authored navigation aids, not binary evidence.", z.object({ address, name: z.string().optional(), document }), ), official( diff --git a/tests/boundary/providers/hopper/hopperBookmarks.test.ts b/tests/boundary/providers/hopper/hopperBookmarks.test.ts new file mode 100644 index 0000000000..774249b670 --- /dev/null +++ b/tests/boundary/providers/hopper/hopperBookmarks.test.ts @@ -0,0 +1,114 @@ +import { execFile } from "node:child_process"; +import { fileURLToPath } from "node:url"; +import { promisify } from "node:util"; + +import { describe, expect, it } from "vitest"; +import { z } from "zod"; + +const execute = promisify(execFile); +const bridgePath = fileURLToPath( + new URL("../../../../bridge/hopper_bridge.py", import.meta.url), +); +const probePath = fileURLToPath( + new URL("../../../fixtures/hopperBookmarkMutationProbe.py", import.meta.url), +); + +const bookmark = z.strictObject({ + address: z.literal("0x401000"), + name: z.string().nullable(), +}); +const bookmarks = z.array(bookmark); +const resultSchema = z.strictObject({ + fresh_empty: z.strictObject({ + result: z.boolean(), + listing: bookmarks, + raw_name: z.string().nullable(), + }), + default_named_omitted: z.strictObject({ + result: z.boolean(), + listing: bookmarks, + }), + fresh_omitted: z.strictObject({ result: z.boolean(), listing: bookmarks }), + named_creation_result: z.boolean(), + empty_clear: z.strictObject({ result: z.boolean(), listing: bookmarks }), + nonempty_replacement: z.strictObject({ + result: z.boolean(), + listing: bookmarks, + }), + omitted_existing: z.strictObject({ + result: z.boolean(), + setter_calls_before: z.number().int(), + setter_calls_after: z.number().int(), + listing: bookmarks, + }), + ignored_setter: z.strictObject({ result: z.boolean(), listing: bookmarks }), + failed_clear: z.strictObject({ result: z.boolean(), listing: bookmarks }), + missing_rename: z.strictObject({ + type: z.literal("CapabilityUnavailableError"), + diagnostic_type: z.literal("capability_unavailable"), + }), + preserved_after_missing_rename: bookmarks, + unset: z.strictObject({ result: z.boolean(), listing: bookmarks }), + rename_calls: z.array(z.tuple([z.string(), z.string()])), +}); + +describe("Hopper bookmark bridge mutations", () => { + it("uses exact provider readback and preserves omission semantics", async () => { + const { stdout } = await execute("python3", [probePath, bridgePath], { + encoding: "utf8", + timeout: 3_000, + maxBuffer: 1_024 * 1_024, + }); + const result = resultSchema.parse(JSON.parse(stdout)); + + expect(result.fresh_empty).toEqual({ + result: true, + listing: [{ address: "0x401000", name: "" }], + raw_name: "", + }); + expect(result.fresh_omitted).toEqual({ + result: true, + listing: [{ address: "0x401000", name: null }], + }); + expect(result.default_named_omitted).toEqual({ + result: false, + listing: [{ address: "0x401000", name: "provider-default" }], + }); + expect(result.named_creation_result).toBe(true); + expect(result.empty_clear).toEqual({ + result: true, + listing: [{ address: "0x401000", name: null }], + }); + expect(result.nonempty_replacement).toEqual({ + result: true, + listing: [{ address: "0x401000", name: "replacement-name" }], + }); + expect(result.omitted_existing).toMatchObject({ + result: true, + listing: [{ address: "0x401000", name: "replacement-name" }], + }); + expect(result.omitted_existing.setter_calls_after).toBe( + result.omitted_existing.setter_calls_before, + ); + expect(result.ignored_setter).toEqual({ + result: false, + listing: [{ address: "0x401000", name: "replacement-name" }], + }); + expect(result.failed_clear).toEqual({ + result: false, + listing: [{ address: "0x401000", name: "replacement-name" }], + }); + expect(result.missing_rename).toEqual({ + type: "CapabilityUnavailableError", + diagnostic_type: "capability_unavailable", + }); + expect(result.preserved_after_missing_rename).toEqual([ + { address: "0x401000", name: "replacement-name" }, + ]); + expect(result.unset).toEqual({ result: true, listing: [] }); + expect(result.rename_calls).toEqual([ + ["0x401000", ""], + ["0x401000", ""], + ]); + }); +}); diff --git a/tests/fixtures/hopperBookmarkMutationProbe.py b/tests/fixtures/hopperBookmarkMutationProbe.py new file mode 100644 index 0000000000..2552a67f60 --- /dev/null +++ b/tests/fixtures/hopperBookmarkMutationProbe.py @@ -0,0 +1,184 @@ +"""Exercise bookmark mutation and exact readback at the Hopper bridge boundary.""" + +import json +from pathlib import Path +import sys + + +class FakeSegment: + pass + + +class FakeDocument: + def __init__(self): + self.names = {} + self.setter_calls = [] + self.rename_calls = [] + self.ignore_next_set = False + self.ignore_next_rename = False + self.default_name_on_omitted = False + + def getDocumentName(self): + return "bookmark-fixture" + + def getExecutableFilePath(self): + return "/tmp/rea-hopper-bookmark-fixture" + + def getDatabaseFilePath(self): + return None + + def backgroundProcessActive(self): + return False + + def getSegmentAtAddress(self, address): + return FakeSegment() if address == 0x401000 else None + + def setBookmarkAtAddress(self, address, name=None): + self.setter_calls.append((address, name)) + if self.ignore_next_set: + self.ignore_next_set = False + return True + if address not in self.names: + if name is None and self.default_name_on_omitted: + self.names[address] = "provider-default" + else: + self.names[address] = "" if name == "" else name + elif name is not None and name != "": + self.names[address] = name + # The native API return is not authoritative; the bridge reads state back. + return False + + def renameBookmarkAtAddress(self, address, name): + self.rename_calls.append((address, name)) + if self.ignore_next_rename: + self.ignore_next_rename = False + return False + self.names[address] = None if name == "" else name + return False + + def hasBookmarkAtAddress(self, address): + return address in self.names + + def getBookmarkName(self, address): + return self.names[address] + + def getBookmarks(self): + return list(self.names) + + def removeBookmarkAtAddress(self, address): + self.names.pop(address, None) + return False + + +class FakeDocumentProvider: + document = FakeDocument() + + @classmethod + def getAllDocuments(cls): + return [cls.document] + + @classmethod + def getCurrentDocument(cls): + return cls.document + + +def main(): + bridge_path = sys.argv[1] + namespace = {"__file__": bridge_path, "__name__": "rea_hopper_bridge"} + source = Path(bridge_path).read_text(encoding="utf-8") + exec(compile(source, bridge_path, "exec"), namespace) + namespace["REA_TARGET_PATH"] = FakeDocumentProvider.document.getExecutableFilePath() + namespace["_configure_hopper_api"](FakeDocumentProvider) + namespace["_bind_session_document"]() + dispatch = namespace["_dispatch"] + address = "0x401000" + document = FakeDocumentProvider.document + + fresh_empty_result = dispatch("set_bookmark", {"address": address, "name": ""}) + fresh_empty_listing = dispatch("list_bookmarks", {}) + fresh_empty_raw_name = document.getBookmarkName(0x401000) + dispatch("unset_bookmark", {"address": address}) + + document.default_name_on_omitted = True + default_named_omitted_result = dispatch("set_bookmark", {"address": address}) + default_named_omitted_listing = dispatch("list_bookmarks", {}) + dispatch("unset_bookmark", {"address": address}) + document.default_name_on_omitted = False + + fresh_omitted_result = dispatch("set_bookmark", {"address": address}) + fresh_omitted_listing = dispatch("list_bookmarks", {}) + dispatch("unset_bookmark", {"address": address}) + + named_creation_result = dispatch( + "set_bookmark", {"address": address, "name": "initial-name"} + ) + clear_result = dispatch("set_bookmark", {"address": address, "name": ""}) + clear_listing = dispatch("list_bookmarks", {}) + renamed_result = dispatch( + "set_bookmark", {"address": address, "name": "replacement-name"} + ) + replacement_listing = dispatch("list_bookmarks", {}) + setter_calls_before_omitted = len(document.setter_calls) + omitted_result = dispatch("set_bookmark", {"address": address}) + setter_calls_after_omitted = len(document.setter_calls) + omitted_listing = dispatch("list_bookmarks", {}) + + document.ignore_next_set = True + ignored_set_result = dispatch( + "set_bookmark", {"address": address, "name": "ignored-name"} + ) + ignored_set_listing = dispatch("list_bookmarks", {}) + + document.ignore_next_rename = True + failed_clear_result = dispatch("set_bookmark", {"address": address, "name": ""}) + failed_clear_listing = dispatch("list_bookmarks", {}) + + rename_method = document.renameBookmarkAtAddress + document.renameBookmarkAtAddress = None + missing_rename = None + try: + dispatch("set_bookmark", {"address": address, "name": ""}) + except Exception as error: + missing_rename = { + "type": type(error).__name__, + "diagnostic_type": namespace["_diagnostic_type"](error), + } + document.renameBookmarkAtAddress = rename_method + preserved_after_missing_rename = dispatch("list_bookmarks", {}) + unset_result = dispatch("unset_bookmark", {"address": address}) + after_unset = dispatch("list_bookmarks", {}) + + print(json.dumps({ + "fresh_empty": { + "result": fresh_empty_result, + "listing": fresh_empty_listing, + "raw_name": fresh_empty_raw_name, + }, + "fresh_omitted": { + "result": fresh_omitted_result, + "listing": fresh_omitted_listing, + }, + "default_named_omitted": { + "result": default_named_omitted_result, + "listing": default_named_omitted_listing, + }, + "named_creation_result": named_creation_result, + "empty_clear": {"result": clear_result, "listing": clear_listing}, + "nonempty_replacement": {"result": renamed_result, "listing": replacement_listing}, + "omitted_existing": { + "result": omitted_result, + "setter_calls_before": setter_calls_before_omitted, + "setter_calls_after": setter_calls_after_omitted, + "listing": omitted_listing, + }, + "ignored_setter": {"result": ignored_set_result, "listing": ignored_set_listing}, + "failed_clear": {"result": failed_clear_result, "listing": failed_clear_listing}, + "missing_rename": missing_rename, + "preserved_after_missing_rename": preserved_after_missing_rename, + "unset": {"result": unset_result, "listing": after_unset}, + "rename_calls": [[hex(item[0]), item[1]] for item in document.rename_calls], + }, sort_keys=True)) + + +if __name__ == "__main__": + main()