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()