fix: route the xref family to warm Ghidra in any mode, not just headless#286
Conversation
branover
left a comment
There was a problem hiding this comment.
Independent merge-gate review — PR #286 (fix/ghidra-xrefs-backend)
Reviewer: independent pr-reviewer (did not write this code). Ran both the /code-review (high effort) and /security-review methodologies directly against git diff main...HEAD (this reviewer context has no nested-Agent tool, so the angle/finder passes were executed inline). Scope: src/hexgraph/agent/agent_tools.py, tests/test_breadth_xrefs.py.
Verdict: APPROVE (no blocking issues)
The fix is correct and well-reasoned. _ghidra_backend_enabled() correctly gates all four xref entry points (_xrefs directly; _function_xrefs/_data_xrefs/_call_graph_tool via _run_xrefs_probe). The headless-degrade loop avoids double-running headless in the common no-bridge case (primary is headless → isinstance(..., GhidraBridgeDecompiler) is False → single backend). The isinstance check is sound and does not depend on the monkeypatched GhidraDecompiler. The _GHIDRA_XREF_LEAD rewrite is correctly narrowed to needs_analysis and error, so it never masks a genuinely-different r2 error. Adding project_mount to the _xrefs r2 path makes it consistent with its siblings and fixes a real pre-existing false-empty (it never reloaded the warm r2 project before).
Security review: no findings. The change is pure static-analysis routing + user-facing messaging. The diff touches no policy seam (assert_allows_execution/assert_allows_egress/current_policy), does not execute the target, does not open or change any egress/network path, and handles no secret (no subprocess/eval/pickle/key/password symbols anywhere in the diff — verified). Routing more xref queries to Ghidra keeps all target-byte handling inside the sandbox (Ghidra xrefs is static; the managed bridge is a local sandboxed pyghidra process). Loopback / sandbox / secret-never-logged / opt-in-execution-&-egress invariants all hold, unchanged.
None of the findings below are blocking; posting for discussion / optional follow-up.
Findings (all non-blocking)
1. [LOW · completeness/altitude] src/hexgraph/agent/agent_tools.py:2624 — the sibling search_code byte/immediate scan still has the exact headless-only bug this PR fixes.
_search_code_scan gates its warm-Ghidra path on the headless-only _ghidra_xrefs_active():
out = _ghidra_search(ctx, bytes_pat=bytes_pat, immediate=immediate) \
if _ghidra_xrefs_active() else NoneBut _ghidra_search already routes via the bridge-aware ghidra_op_backend(ctx.target).search_bytes(...) — identical to _ghidra_xrefs. So a bridge-mode Ghidra user's byte/immediate search falls through to the r2 raw scan and loses the precise warm-Ghidra function attribution (they have no warm r2 project, only Ghidra was analyzed) — the same mis-route this PR fixes for the xref family, left on a sibling. It's arguably outside the PR's stated "xref family" scope and the r2 raw scan still returns a result (search needs no analysis), so it's non-blocking — but switching this one gate to _ghidra_backend_enabled() in the same PR would close the bug class completely rather than per-tool. (This is the altitude signal: the gate broadening was applied per-call-site, so this call site was missed.)
2. [LOW · robustness] src/hexgraph/agent/agent_tools.py:1978-1988 — the headless degrade fires on a live-bridge error dict, not just an unreachable bridge.
The loop appends a headless GhidraDecompiler() whenever primary is a GhidraBridgeDecompiler, and falls through to it when the bridge call either raises (unreachable — the intended "stale bridge" case, safe because a dead bridge holds no project lock) or returns an {"error": ...} dict from a live managed bridge. In the latter sub-case, ghidra_op_backend's own docstring notes a headless op conflicts with a live bridge on the Ghidra project lock, so the appended headless attempt is futile (fails the lock, then falls to r2). End result is still a safe read-only r2 fallback — no corruption, no wrong answer — so this is wasted work / a possible spurious lock error in logs, not a regression. Consider only appending the headless degrade when the bridge call raised (except path sets a flag), not when it returned an error dict.
3. [LOW · test quality] tests/test_breadth_xrefs.py — two paths the fix adds are not actually exercised.
- (a) The headline bridge→headless degrade (
isinstance(primary, GhidraBridgeDecompiler)→backends.append(GhidraDecompiler())→ headless serves) is never hit.test_xrefs_bridge_mode_cold_returns_ghidra_lead_not_radare2is named "bridge mode" but monkeypatchesGhidraDecompiler → _FakeGhidrareturning None, soprimaryis the headless fake and theisinstancebranch is False — the append is dead in tests. Add a test that makesghidra_op_backendreturn a fakeGhidraBridgeDecompilerwhosexrefsraises, and assert the headless slot serves the result (and r2 is never reached). - (b)
_xrefs(re_xrefs) has its own inline cold-miss→_GHIDRA_XREF_LEADrewrite at lines 2141-2142, distinct from the_run_xrefs_proberewrite. Only the latter is covered (viadata_xrefs). Add a case drivingre_xrefs(with a symbol) into the r2 cold-miss and asserting the Ghidra-named lead (andradare2absent), mirroring the data_xrefs test.
Not issues (verified during review)
_ghidra_backend_enabled()gates all four xref tools;_ghidra_xrefs_active()(headless-only) is correctly retained only forre_script(line 2050).reis imported (line 24) for_no_data_xrefs_msg'sre.fullmatch. No double headless run in the common path. The new top-level imports in_ghidra_xrefs(GhidraBridgeDecompiler,GhidraDecompiler) are import-safe (stdlib/internal only; no third-partyghidra_bridgepackage at module load)._r2_project_mount(ctx)on the_xrefspath is the same best-effort helper the siblings already use.
Tests: not re-run in this worktree (no local venv; a reused venv would import the wrong src). Relying on CI (offline matrix runs on the PR) + the PR's reported just test result (1700 passed, 6 skipped, 14 deselected). Recommend the two test additions in finding #3 before/with merge.
branover
left a comment
There was a problem hiding this comment.
Follow-up re-review — delta 3debe4d ("fix: honest researcher-bridge xref message + review follow-ups")
Re-reviewed only the delta 4951e89..3debe4d (agent_tools.py +69/-16, test_breadth_xrefs.py, test_re_search_code.py) against my three prior findings plus the new refinement. Ran both /code-review (high) and /security-review methodologies over the delta directly.
Verdict: APPROVE — delta correctly resolves all three findings; merge-ready.
Finding #1 (search_code sibling) — RESOLVED. _search_code_scan now gates on _ghidra_backend_enabled() (line ~2670). Verified the "no refusal for search_code" reasoning holds end to end: GhidraDecompiler.search_bytes short-circuits on a missing warm slot (decompiler.py:330-332 — returns an error dict without spawning a container), so a bridge-mode target gets _ghidra_search → None → _r2_search (a RAW byte scan that needs no warm analysis) and still returns real hits. No wasteful cold spawn, no false refusal. Gate change is correct.
Finding #2 (degrade on live-error) — RESOLVED, no hole. _ghidra_xrefs (lines 2004-2026) now tracks a raised flag and degrades to headless ONLY when the primary RAISED (unreachable). Traced every branch: headless-primary-raises → return None (no double-headless); bridge-primary-returns-error-dict → not raised → return None (no headless op behind a live bridge's project lock — exactly the fix requested); bridge-primary-raises → headless degrade → good dict returned / else None. Matches the recommendation precisely.
Finding #3 (test gaps) — RESOLVED, coverage now adequate. test_xrefs_dead_managed_bridge_degrades_to_headless builds a real GhidraBridgeDecompiler subclass whose xrefs raises and asserts the headless slot serves (headless_calls == [("data","0x1")], r2 never hit) — the headline degrade path is now directly exercised. test_re_xrefs_cold_miss_returns_ghidra_lead_via_xrefs_path covers the _xrefs inline rewrite (distinct from _run_xrefs_probe's). The mis-named ..._bridge_mode_cold... test was renamed to ..._cold_miss.... Good.
New refinement (_ghidra_bridge_only / _GHIDRA_BRIDGE_ONLY_XREF_MSG) — correct and well-justified. Verified: (a) predicate is right — mode == "bridge" catches only the researcher jfx bridge; the managed resident bridge is detected at runtime via bridge_endpoint and runs under mode == "headless", so it is NOT caught (it still routes through _ghidra_xrefs to the live bridge). (b) The early-return placement in both _xrefs (after cache, before the gate) and _run_xrefs_probe (first statement) is sound; all four xref tools are covered, and search_code is deliberately excluded (its raw r2 fallback works). (c) Message is accurate — confirmed analysis._active_backend() maps ghidra_bridge → None (analysis.py:68), so re_analyze genuinely builds no slot in bridge mode and "switch to headless + re_analyze, or use radare2" is the correct guidance. This also removes a real latent problem in the prior delta: without it, bridge mode would cold-spawn a headless probe per xref query and then hand back a _GHIDRA_XREF_LEAD telling the user to "run re_analyze" — which does nothing in bridge mode.
Security: no findings. The delta adds a boolean-config predicate, two static message strings, and a control-flow restructure — no target execution, no egress, no secret handling, no policy-seam change, no subprocess/eval/pickle (verified). Static-analysis routing only; sandbox/loopback/secret invariants unchanged.
New nits (both LOW, non-blocking — no need to gate merge)
- Docstring drift —
agent_tools.py:1992-1995(_ghidra_xrefs). The top docstring still reads "If that bridge is registered but unreachable/errors, we RETRY the headless warm project explicitly." Per the finding-#2 fix, the code now retries headless ONLY on a raise (unreachable); a live-bridge error dict returns None. The inline comments (2010-2017) are correct — just update the docstring narrative to match ("unreachable" only, not "errors") so a future reader isn't misled. - Message wording —
_GHIDRA_BRIDGE_ONLY_XREF_MSG(agent_tools.py:~2033). It states the researcher bridge has "no reference index / P-Code surface for xrefs or search_code", but this message is only ever returned by the xref family;search_codein bridge mode actually returns real results via the r2 raw fallback and never shows this text. Consider dropping "or search_code" (or noting it still works via the raw scan) to avoid implying search_code is also blocked.
Both are pure wording; fix at leisure. From the review gate this delta is clean and merge-ready (I confirmed the fast suite is reported green at 1704 passed; I did not re-run in-worktree — no venv — and relied on CI + py_compile, which is clean).
The cross-reference tools (re_xrefs / re_function_xrefs / re_data_xrefs / re_call_graph) gated the warm-Ghidra path on `_ghidra_xrefs_active()`, which is true only for headless settings mode. A user running Ghidra in bridge mode (a managed resident bridge, or a researcher bridge) has a warm Ghidra project, but the gate returned false, so these tools fell through to the cold radare2 xrefs probe. That probe finds no warm *r2* project — only Ghidra was analyzed — and returns "No warm radare2 analysis for this target yet. Run re_analyze ...", naming the wrong engine and telling the user to build an analysis they don't use. Fix: - Add `_ghidra_backend_enabled()` (enabled in ANY mode) and gate the xref family on it. Keep the narrow `_ghidra_xrefs_active()` (headless-only) for re_script, which genuinely needs the headless probe's warm project / P-Code surface. - `_ghidra_xrefs` now degrades a registered-but-unreachable managed bridge to the headless warm slot before giving up, instead of dropping the query to r2. - When Ghidra is the active backend but its warm index can't answer, surface a Ghidra-named lead (`_GHIDRA_XREF_LEAD`) — never the r2 "no warm radare2" one. r2 still runs as a genuine last resort, but its cold-miss lead is rewritten so the message always matches the active backend. - `_xrefs` now passes `project_mount` on its r2 path (it previously never reloaded the warm r2 project) and surfaces the cold-miss lead instead of formatting a false-empty sink map. - data_xrefs: a subject that doesn't resolve to an address (most often a string VALUE mistakenly passed, e.g. a URL/path) now points at the resolve-the- string's-address workflow (re_list_strings / re_resolve) rather than a bare "no resolvable references". Tests: broaden the xref-routing wiring to the new gate; add coverage for the bridge-mode gate, the string-value data_xrefs hint, and the bridge-mode cold case returning the Ghidra lead (never radare2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refines the xref-backend routing from the first commit, addressing the merge review and extending the fix to a sibling that shared the bug: - Researcher jfx bridge (features.ghidra.mode=="bridge") is decompile-only and re_analyze builds no slot in that mode, so routing its xref into a headless cold-miss produced a "run re_analyze" lead that can't help. `_ghidra_bridge_only` now short-circuits re_xrefs/re_function_xrefs/re_data_xrefs/re_call_graph with an honest "switch to headless (the bridge is decompile-only)" message — no futile attempt, no radare2 cold-miss. Mirrors emulation's early mode-aware refusal. - _search_code_scan gated its warm path on the headless-only _ghidra_xrefs_active while _ghidra_search already routes bridge-aware — the exact mis-route this PR fixes for the xref family, on a sibling. Switched it to _ghidra_backend_enabled. It does NOT get the bridge-only refusal: its r2 fallback is a RAW byte scan that needs no warm analysis, so researcher-bridge targets still get real hits. - _ghidra_xrefs now degrades a dead managed bridge to headless ONLY when the bridge call RAISED (unreachable). A LIVE bridge returning an error dict no longer triggers a headless op that would conflict on the project lock the bridge holds — it returns None (caller surfaces the lead / r2) instead. Tests: the dead-managed-bridge→headless degrade (a real GhidraBridgeDecompiler that raises), the re_xrefs path's own cold-miss rewrite, the researcher-bridge refusal message, and search_code falling back to the r2 raw scan in bridge mode. Renamed the mis-named "bridge_mode_cold" test (its primary was headless). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two LOW wording nits from the #286 re-review: the _ghidra_xrefs docstring said the headless degrade fires when the bridge 'unreachable/errors', but it now degrades only on a raise (an error dict returns None); and the bridge-only message listed 'xrefs or search_code' though search_code never shows it (it falls back to the r2 raw scan in bridge mode). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3debe4d to
64bd8ac
Compare
What
The cross-reference tools (
re_xrefs,re_function_xrefs,re_data_xrefs,re_call_graph) gated the warm-Ghidra path on_ghidra_xrefs_active(), which is true only for headless settings mode. A user running Ghidra in bridge mode (a managed resident bridge, or a researcher bridge) has a warm Ghidra project, but the gate returned false, so these tools fell through to the cold radare2 xrefs probe. That probe finds no warm r2 project — only Ghidra was analyzed — and returns:naming the wrong engine and telling the user to build an analysis they don't use.
Why it happened
_run_xrefs_probe/_xrefschose the backend with_ghidra_xrefs_active()(enabled and mode == "headless"). Bridge-mode Ghidra is a warm backend too, but was excluded, so the query mis-routed to r2, whose cold-miss lead is radare2-specific.Fix
_ghidra_backend_enabled()(Ghidra enabled in any mode) and gate the xref family on it. Keep the narrow_ghidra_xrefs_active()(headless-only) forre_script, which genuinely needs the headless probe's warm project / P-Code surface._ghidra_xrefsdegrades a registered-but-unreachable managed bridge to the headless warm slot before giving up, instead of dropping the query to r2._GHIDRA_XREF_LEAD), never the r2 "no warm radare2" one. r2 still runs as a genuine last resort, but its cold-miss lead is rewritten so the message always matches the active backend._xrefsnow passesproject_mounton its r2 path (it previously never reloaded the warm r2 project) and surfaces the cold-miss lead instead of formatting a false-empty sink map.data_xrefs: a subject that doesn't resolve to an address (most often a string value mistakenly passed, e.g. a path like/api/v1/config) now points at the resolve-the-string's-address workflow (re_list_strings/re_resolve) rather than a bare "no resolvable references".Verification
just test(fast tier): 1700 passed, 6 skipped, 14 deselected.tests/test_breadth_xrefs.py: the bridge-mode backend gate, the string-valuedata_xrefshint, and the bridge-mode cold case returning the Ghidra lead (assertingradare2never appears).No tool rename/addition, so no catalog/
docs/mcp.md/VR-skill changes. No schema/migration/UI changes.