[PR #6171] [CLOSED] fix(xrefs): reject ambiguous exact names in find_xrefs_to_name #7181

Closed
opened 2026-11-01 21:45:58 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6171
Author: @sx4im
Created: 10/21/2026
Status: ❌ Closed

Base: main ← Head: sx4im/fix/5908-find-xrefs-ambiguous-name


📝 Commits (3)

  • a0fe5f3 fix(xrefs): reject ambiguous exact names instead of picking the first address
  • 98b2c1d fix(xrefs): share exact-name selection and route ambiguity recovery to xrefs
  • 2d052ea Retrigger CI after a flaky hermes setup acceptance failure

📊 Changes

4 files changed (+120 additions, -15 deletions)

View changed files

📝 src/application/EnhancedTools.ts (+16 -8)
📝 src/application/XrefsAddressResolution.ts (+34 -6)
📝 src/contracts/enhancedToolContracts.ts (+1 -1)
📝 tests/boundary/mcp/enhancedTools.03-returns-typed-graph-failures-and.test.ts (+69 -0)

📄 Description

Summary

Fixes #5908.

Problem and expected behavior

find_xrefs_to_name resolved an exact name with Array.find, so a name present at two distinct addresses silently returned the first inventory row's address and its xrefs. Reversing the inventory changed which address and reference list came back, with no ambiguity signal. An exact name at more than one distinct address should not produce an unqualified resolved result.

Change and scope

EnhancedTools.#findXrefs now collects the distinct matching addresses instead of taking the first match. Zero matches keeps the stable unresolved / name_not_found result. One distinct address resolves as before (duplicate rows at one address still resolve). More than one distinct address returns a typed AnalysisInputError listing the candidates, e.g. Analyzed symbol name "shared_symbol" is ambiguous at 0x1000, 0x2000. Select an exact address., using the same wording as the existing shared resolveXrefsAddress resolver. The tool contract description documents the ambiguity rejection. Not in scope: no new result variant, no schema change; the unresolved contract is untouched.

Contribution scope fit: ordinary bug fix within existing boundaries. Not applicable (no new provider or capability, no new scope decision needed).

Contract and boundary impact

  • Semantic owner and earliest changed stage: application layer, EnhancedTools.#findXrefs.
  • Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable.
  • CLI and MCP/tool-catalog contract: find_xrefs_to_name description updated; output schema unchanged. Ambiguity surfaces as a typed tool error; callers can pick an exact address and call xrefs directly.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects:

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: two in tests/boundary/mcp/enhancedTools.03-returns-typed-graph-failures-and.test.ts. The ambiguity test runs both inventory orderings through the MCP harness, expects a typed ambiguity error naming both addresses, and asserts no xrefs call is issued. The duplicate-rows test asserts two rows at one address still resolve.
  • Base reproduction or other evidence: the ambiguity test fails on current main before the fix (the tool returned status: "resolved" with the first row's address) and passes after. Simulated provider only; no real engine was used, matching the issue's reproduction.
  • User-visible CLI/MCP output (if applicable): on ambiguity the tool now returns an MCP error whose typed payload contains Analyzed symbol name "shared_symbol" is ambiguous at 0x1000, 0x2000. Select an exact address.
  • Remaining proof gaps: no real Hopper/Ghidra/IDA run; simulated provider coverage only.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run tests/boundary/mcp/enhancedTools --maxWorkers=1 — 9 files, 72 passed
  • npx vitest run src/application/EnhancedTools.inputValidation.test.ts src/application/EnhancedTools.objcClasses.test.ts tests/boundary/mcp/ghidraRoutingMcp.test.ts --maxWorkers=1 — 3 files, 21 passed
  • npx vitest run src/contracts --maxWorkers=1 — 13 files, 137 passed, 3 skipped
  • npm run check:fast — typecheck and lint passed
  • npm run verify:test-discovery — discovered all 1946 test files exactly once
  • Real Hopper/Ghidra coverage: unavailable in this environment; simulated provider only
  • Follow-up commit 98b2c1df (automated review findings): the exact-name selection and ambiguity message now live in one shared matchExactName helper in XrefsAddressResolution.ts, used by both resolveXrefsAddress and #findXrefs; the ambiguity error names the xrefs operation and its address argument so the projected recovery is actionable, and the ambiguity test now asserts that projection. Re-ran: tests/boundary/mcp/enhancedTools — 72 passed; src/contracts — 137 passed, 3 skipped; npm run check:fast passed; npm run verify:test-discovery passed. (tests/boundary/cli/analysisSelectorCli.test.ts shows 3 target-selector failures that reproduce on the base tree without this change; unrelated.)

Compatibility, safety, and release

  • Breaking changes or migration steps: callers that relied on the silent first-address selection now receive a typed error for ambiguous names. That reliance was the bug; the workaround in the issue (inspect list_names, pick an address, call xrefs) keeps working.
  • Real Hopper/Ghidra, browser, or OS coverage: none; simulated provider only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; read-only analysis path, no new side effects.

Author readiness

Human owner: @sx4im

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6171 **Author:** [@sx4im](https://github.com/sx4im) **Created:** 10/21/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `sx4im/fix/5908-find-xrefs-ambiguous-name` --- ### 📝 Commits (3) - [`a0fe5f3`](https://github.com/morluto/rea/commit/a0fe5f32a9aba0958bb92734fbb777814c941896) fix(xrefs): reject ambiguous exact names instead of picking the first address - [`98b2c1d`](https://github.com/morluto/rea/commit/98b2c1dfad65780e776b3b022e8de7a3deeb0ab9) fix(xrefs): share exact-name selection and route ambiguity recovery to xrefs - [`2d052ea`](https://github.com/morluto/rea/commit/2d052ea57ca76d1a7ab90fda6d9e795b8b26c394) Retrigger CI after a flaky hermes setup acceptance failure ### 📊 Changes **4 files changed** (+120 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `src/application/EnhancedTools.ts` (+16 -8) 📝 `src/application/XrefsAddressResolution.ts` (+34 -6) 📝 `src/contracts/enhancedToolContracts.ts` (+1 -1) 📝 `tests/boundary/mcp/enhancedTools.03-returns-typed-graph-failures-and.test.ts` (+69 -0) </details> ### 📄 Description ## Summary Fixes #5908. ## Problem and expected behavior `find_xrefs_to_name` resolved an exact name with `Array.find`, so a name present at two distinct addresses silently returned the first inventory row's address and its xrefs. Reversing the inventory changed which address and reference list came back, with no ambiguity signal. An exact name at more than one distinct address should not produce an unqualified `resolved` result. ## Change and scope `EnhancedTools.#findXrefs` now collects the distinct matching addresses instead of taking the first match. Zero matches keeps the stable `unresolved` / `name_not_found` result. One distinct address resolves as before (duplicate rows at one address still resolve). More than one distinct address returns a typed `AnalysisInputError` listing the candidates, e.g. `Analyzed symbol name "shared_symbol" is ambiguous at 0x1000, 0x2000. Select an exact address.`, using the same wording as the existing shared `resolveXrefsAddress` resolver. The tool contract description documents the ambiguity rejection. Not in scope: no new result variant, no schema change; the `unresolved` contract is untouched. Contribution scope fit: ordinary bug fix within existing boundaries. Not applicable (no new provider or capability, no new scope decision needed). ## Contract and boundary impact - Semantic owner and earliest changed stage: application layer, `EnhancedTools.#findXrefs`. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: `find_xrefs_to_name` description updated; output schema unchanged. Ambiguity surfaces as a typed tool error; callers can pick an exact address and call `xrefs` directly. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: two in `tests/boundary/mcp/enhancedTools.03-returns-typed-graph-failures-and.test.ts`. The ambiguity test runs both inventory orderings through the MCP harness, expects a typed ambiguity error naming both addresses, and asserts no `xrefs` call is issued. The duplicate-rows test asserts two rows at one address still resolve. - Base reproduction or other evidence: the ambiguity test fails on current `main` before the fix (the tool returned `status: "resolved"` with the first row's address) and passes after. Simulated provider only; no real engine was used, matching the issue's reproduction. - User-visible CLI/MCP output (if applicable): on ambiguity the tool now returns an MCP error whose typed payload contains `Analyzed symbol name "shared_symbol" is ambiguous at 0x1000, 0x2000. Select an exact address.` - Remaining proof gaps: no real Hopper/Ghidra/IDA run; simulated provider coverage only. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run tests/boundary/mcp/enhancedTools --maxWorkers=1` — 9 files, 72 passed - `npx vitest run src/application/EnhancedTools.inputValidation.test.ts src/application/EnhancedTools.objcClasses.test.ts tests/boundary/mcp/ghidraRoutingMcp.test.ts --maxWorkers=1` — 3 files, 21 passed - `npx vitest run src/contracts --maxWorkers=1` — 13 files, 137 passed, 3 skipped - `npm run check:fast` — typecheck and lint passed - `npm run verify:test-discovery` — discovered all 1946 test files exactly once - Real Hopper/Ghidra coverage: unavailable in this environment; simulated provider only - Follow-up commit 98b2c1df (automated review findings): the exact-name selection and ambiguity message now live in one shared `matchExactName` helper in `XrefsAddressResolution.ts`, used by both `resolveXrefsAddress` and `#findXrefs`; the ambiguity error names the `xrefs` operation and its `address` argument so the projected recovery is actionable, and the ambiguity test now asserts that projection. Re-ran: `tests/boundary/mcp/enhancedTools` — 72 passed; `src/contracts` — 137 passed, 3 skipped; `npm run check:fast` passed; `npm run verify:test-discovery` passed. (`tests/boundary/cli/analysisSelectorCli.test.ts` shows 3 target-selector failures that reproduce on the base tree without this change; unrelated.) ## Compatibility, safety, and release - Breaking changes or migration steps: callers that relied on the silent first-address selection now receive a typed error for ambiguous names. That reliance was the bug; the workaround in the issue (inspect `list_names`, pick an address, call `xrefs`) keeps working. - Real Hopper/Ghidra, browser, or OS coverage: none; simulated provider only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; read-only analysis path, no new side effects. ## Author readiness Human owner: @sx4im - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] The PR has one focused outcome and the title follows `type(scope): outcome`. - [ ] Related issue is linked, or the reason for not linking one is stated above. - [ ] Tests cover changed observable behavior and meaningful failure paths. - [ ] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [ ] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:45:58 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#7181
No description provided.