[PR #6402] [MERGED] fix(javascript): associate requested bindings only with resolved native addons #7337

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6402
Author: @pablodiazjorge
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/23/2026
Merged by: @morluto

Base: main ← Head: fix/electron-native-addon-js-resolution


📝 Commits (3)

  • e04697c fix(javascript): associate requested bindings only with resolved native addons
  • cdeda06 test(javascript): cover the exact native addon association
  • 6fe1159 chore(javascript): resolve native-addon merge conflict

📊 Changes

2 files changed (+59 additions, -1 deletions)

View changed files

📝 src/application/javascript/ElectronBoundaryGraphNative.ts (+7 -1)
📝 tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts (+52 -0)

📄 Description

Summary

Fixes #5929. When require("./addon.node") resolves through extension probing to addon.node.js and no addon.node exists, the application graph added an associate-native-addon-requested-exports edge from that JavaScript file, with a limitation saying the .node artifact path is resolved. The same result reported resolved_native_addon_bindings: 0. The association now requires the resolved file to be classified as native-addon, the rule the summary already uses.

Thanks to @coygeek for the report and the three-case reproduction.

Problem and expected behavior

addNativeBinding in ElectronBoundaryGraphNative.ts took any file node at the resolved path and only checked that it existed before adding the association. resolveArtifactPath allows the .js fallback, so a JavaScript asset could receive a native-addon association. The summary's resolved_native_addon_bindings (ElectronBoundaryAnalysis.ts) already requires kind: "native-addon".

Expected, as the issue asks: the JavaScript fallback keeps its requested binding and ordinary dependency information, with no native-addon association; a missing target stays without one; an exact .node target keeps its association.

Change and scope

  • src/application/javascript/ElectronBoundaryGraphNative.ts: the addon node is taken only when context.filesByPath classifies the resolved file as native-addon; otherwise no association edge is added.
  • Unchanged: the requested-binding node (including its resolved_path), the map-native-addon-binding edge from the requesting file, and the exact-native case.
  • The merge retains main's resolveArtifactPathByContext resolver, including JSON-document and module-kind context, and applies the native-addon file-kind guard to its result. Summary and graph now both use the context-aware resolver and the same file-kind rule.

Contract and boundary impact

  • Semantic owner and earliest changed stage: native-addon graph projection (addNativeBinding).
  • Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable.
  • CLI and MCP/tool-catalog contract: analyze / analyze_javascript_application results no longer contain this association for non-native resolved files; no schema or catalog change.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: one inferred edge is no longer emitted when its source is not a native addon; the graph and the summary now agree on file kind.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • 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: tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts, new describe "native addon association", through reconstructJavaScriptArtifact, with main.js requiring ./addon.node and an addon.node.js beside it:
    • without an exact addon.node: one requested binding, zero resolved native bindings, one native-export node, and no associate-native-addon-requested-exports edge;
    • with an exact addon.node (inventory-only bytes, never loaded): one resolved native binding and exactly one association, from the native-addon node with resolved_path addon.node, so the test also fails if valid associations stop being emitted.
  • Base reproduction or other evidence: the issue's three cases, rebuilt as a temporary local test that calls buildJavaScriptArtifactGraph directly (not committed):
    • on main 7af1b2068: the JavaScript fallback gets one association (expected [ Array(1) ] to deeply equal []); the missing target has none; the exact native target has one from a native-addon node;
    • with the change: the fallback has none; the other two are unchanged.
  • User-visible CLI/MCP output (if applicable): not captured.
  • Remaining proof gaps: the original Windows run could not execute the committed public-workflow test because packaged native controls were unavailable. Both committed association cases passed in the macOS merge-head run below. MCP and ASAR inputs were not run.

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

Current conflict-resolution head 6fe11598bd42b874a9fe29148b2c94b34d4c0370, merging main 572d0c234089605f120dcfea340f11e38a3c3705 (macOS, Node 24.22.1/npm 11.16.0):

  • Retained main's context-aware path resolution and this PR's native-addon classification guard. The diff against the merged main remains limited to the original two files.
  • npm run test:focused -- tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts src/application/javascript/JavaScriptArtifactPathResolution.test.ts src/application/javascript/JavaScriptArtifactPathResolution.syntheticRootEscape.test.ts tests/boundary/filesystem/javascriptArtifactGraphObservationLookup.test.ts: 314 tests passed in 4 files (6.33s), including both committed native-addon association cases.
  • npm run check and formatting checks on both changed files passed. Normal pre-commit and pre-push (check:fast) checks passed.
  • No full suite, Windows rerun, MCP or ASAR validation on this merge head. Earlier validation below is historical.

Windows 11 x64 10.0.26300, Node 24.18.0, on main 7af1b2068:

  • The temporary graph-level test above: 3 of 3 pass with the change; on main the fallback case fails and the two controls pass.
  • npx vitest run src/application/javascript src/domain/javascript tests/boundary/filesystem/javascriptArtifactGraphObservationLookup.test.ts: 153 files, 4270 passed, 6 skipped.
  • npx vitest run tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts -t "resolved native addon": does not run on this host (REA Windows native controls are unavailable: Missing packaged Windows native file native/windows/build/manifest.json).
  • npm run check:fast: typecheck and lint pass.
  • npx oxfmt --check on the changed files: pass.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Consumers that relied on this association from a JavaScript asset no longer receive it; the issue describes it as incorrect.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; static JavaScript analysis only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; no target is executed or loaded.

Author readiness

Human owner: @pablodiazjorge

  • 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/6402 **Author:** [@pablodiazjorge](https://github.com/pablodiazjorge) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/23/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/electron-native-addon-js-resolution` --- ### 📝 Commits (3) - [`e04697c`](https://github.com/morluto/rea/commit/e04697c3dc3e8ea8482c769f4972361f99b87681) fix(javascript): associate requested bindings only with resolved native addons - [`cdeda06`](https://github.com/morluto/rea/commit/cdeda062f8198f2b16cdad8254e5158ede527b5b) test(javascript): cover the exact native addon association - [`6fe1159`](https://github.com/morluto/rea/commit/6fe11598bd42b874a9fe29148b2c94b34d4c0370) chore(javascript): resolve native-addon merge conflict ### 📊 Changes **2 files changed** (+59 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/ElectronBoundaryGraphNative.ts` (+7 -1) 📝 `tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts` (+52 -0) </details> ### 📄 Description ## Summary Fixes #5929. When `require("./addon.node")` resolves through extension probing to `addon.node.js` and no `addon.node` exists, the application graph added an `associate-native-addon-requested-exports` edge from that JavaScript file, with a limitation saying the `.node` artifact path is resolved. The same result reported `resolved_native_addon_bindings: 0`. The association now requires the resolved file to be classified as `native-addon`, the rule the summary already uses. Thanks to @coygeek for the report and the three-case reproduction. ## Problem and expected behavior `addNativeBinding` in `ElectronBoundaryGraphNative.ts` took any file node at the resolved path and only checked that it existed before adding the association. `resolveArtifactPath` allows the `.js` fallback, so a JavaScript asset could receive a native-addon association. The summary's `resolved_native_addon_bindings` (`ElectronBoundaryAnalysis.ts`) already requires `kind: "native-addon"`. Expected, as the issue asks: the JavaScript fallback keeps its requested binding and ordinary dependency information, with no native-addon association; a missing target stays without one; an exact `.node` target keeps its association. ## Change and scope - `src/application/javascript/ElectronBoundaryGraphNative.ts`: the addon node is taken only when `context.filesByPath` classifies the resolved file as `native-addon`; otherwise no association edge is added. - Unchanged: the requested-binding node (including its `resolved_path`), the `map-native-addon-binding` edge from the requesting file, and the exact-native case. - The merge retains main's `resolveArtifactPathByContext` resolver, including JSON-document and module-kind context, and applies the native-addon file-kind guard to its result. Summary and graph now both use the context-aware resolver and the same file-kind rule. ## Contract and boundary impact - Semantic owner and earliest changed stage: native-addon graph projection (`addNativeBinding`). - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: `analyze` / `analyze_javascript_application` results no longer contain this association for non-native resolved files; no schema or catalog change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: one inferred edge is no longer emitted when its source is not a native addon; the graph and the summary now agree on file kind. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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: `tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts`, new describe "native addon association", through `reconstructJavaScriptArtifact`, with `main.js` requiring `./addon.node` and an `addon.node.js` beside it: - without an exact `addon.node`: one requested binding, zero resolved native bindings, one `native-export` node, and no `associate-native-addon-requested-exports` edge; - with an exact `addon.node` (inventory-only bytes, never loaded): one resolved native binding and exactly one association, from the `native-addon` node with `resolved_path` `addon.node`, so the test also fails if valid associations stop being emitted. - Base reproduction or other evidence: the issue's three cases, rebuilt as a temporary local test that calls `buildJavaScriptArtifactGraph` directly (not committed): - on main `7af1b2068`: the JavaScript fallback gets one association (`expected [ Array(1) ] to deeply equal []`); the missing target has none; the exact native target has one from a `native-addon` node; - with the change: the fallback has none; the other two are unchanged. - User-visible CLI/MCP output (if applicable): not captured. - Remaining proof gaps: the original Windows run could not execute the committed public-workflow test because packaged native controls were unavailable. Both committed association cases passed in the macOS merge-head run below. MCP and ASAR inputs were not run. 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 Current conflict-resolution head `6fe11598bd42b874a9fe29148b2c94b34d4c0370`, merging main `572d0c234089605f120dcfea340f11e38a3c3705` (macOS, Node 24.22.1/npm 11.16.0): - Retained main's context-aware path resolution and this PR's native-addon classification guard. The diff against the merged main remains limited to the original two files. - `npm run test:focused -- tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts src/application/javascript/JavaScriptArtifactPathResolution.test.ts src/application/javascript/JavaScriptArtifactPathResolution.syntheticRootEscape.test.ts tests/boundary/filesystem/javascriptArtifactGraphObservationLookup.test.ts`: 314 tests passed in 4 files (6.33s), including both committed native-addon association cases. - `npm run check` and formatting checks on both changed files passed. Normal pre-commit and pre-push (`check:fast`) checks passed. - No full suite, Windows rerun, MCP or ASAR validation on this merge head. Earlier validation below is historical. Windows 11 x64 10.0.26300, Node 24.18.0, on main `7af1b2068`: - The temporary graph-level test above: 3 of 3 pass with the change; on main the fallback case fails and the two controls pass. - `npx vitest run src/application/javascript src/domain/javascript tests/boundary/filesystem/javascriptArtifactGraphObservationLookup.test.ts`: 153 files, 4270 passed, 6 skipped. - `npx vitest run tests/boundary/filesystem/javascript-applications/electronApplicationAnalysis.test.ts -t "resolved native addon"`: does not run on this host (`REA Windows native controls are unavailable: Missing packaged Windows native file native/windows/build/manifest.json`). - `npm run check:fast`: typecheck and lint pass. - `npx oxfmt --check` on the changed files: pass. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Consumers that relied on this association from a JavaScript asset no longer receive it; the issue describes it as incorrect. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; static JavaScript analysis only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; no target is executed or loaded. ## Author readiness Human owner: @pablodiazjorge - [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 - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [ ] User-visible CLI/MCP changes include representative output. - [x] 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:47:10 +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#7337
No description provided.