[PR #4259] [CLOSED] fix(javascript): preserve optional native-addon bindings #6144

New issue

Closed

opened 2026-10-31 01:12:12 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:12:12 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4259
Author: @ogutdgn
Created: 10/15/2026
Status: ❌ Closed

Base: main ← Head: fix/javascript-optional-native-addon-bindings

📝 Commits (1)

01599e7 fix(javascript): preserve optional native-addon bindings

📊 Changes

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

View changed files

📝 src/domain/javascript/electronStaticAnalysisNative.ts (+2 -1)
➕ tests/boundary/javascript/nativeOptionalBindings.test.ts (+96 -0)

📄 Description

Summary

Preserve native-addon binding records for optional CommonJS calls such as const addon = require?.("./addon.node").

Fixes #3885.

Problem and expected behavior

REA already resolves the dependency for this syntax, but the native collector rejects Babel's OptionalCallExpression. This drops the binding, native-export graph node, and summary counts. Optional and ordinary calls should preserve the same static request information without claiming that the addon executes or implements the requested members.

Change and scope

Accept both call node types in the existing nativeRequire helper. Existing loader-name, path, member, graph, summary, and handoff logic handles the result; no downstream workaround or additional representation is introduced. Add application-boundary regressions for namespace access, destructuring, optional member access, dynamic members, re-export, and negative controls.

Contract and boundary impact

Semantic owner and earliest changed stage: native request extraction in electronStaticAnalysisNative.ts.
Contribution scope / maintainer decision: ordinary correctness fix within existing static-analysis boundaries; advance scope decision not applicable.
CLI and MCP/tool-catalog contract: no input or schema changes; existing output now retains the omitted native finding.
Provider, bridge, target-format, or platform compatibility: no provider/bridge changes; existing JavaScript/CommonJS syntax.
Evidence, artifact, provenance, or reconstruction contract: retain source-derived requests and existing provenance; native-export verification remains not-performed.
Target selection, authority, and cleanup: unchanged file-reading analysis; no target execution or mutation added.
Generated metadata, 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

Executed on macOS arm64 with Node 24.18.0, based on upstream 2202e3f0ba556d40cc6e598df1ecbdf1eda56c13:

Exact new regression against unpatched production source: 5 failed / 3 passed. The five optional-call cases fail; negative controls pass.
Patched focused suite: 18 passed across 3 files.
Rebuilt public CLI, with inert .node fixture bytes:

Input	Before: bindings / resolved / nodes	After: bindings / resolved / nodes
`require("./addon.node")`	1 / 1 / 1	1 / 1 / 1
`require?.("./addon.node")`	0 / 0 / 0	1 / 1 / 1

The regression also checks the retained resolved dependency and requested-member handoff. Addon bytes are never executed.

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

npm run test:focused -- tests/boundary/javascript/nativeOptionalBindings.test.ts tests/boundary/javascript/nativeTypeBindings.test.ts src/domain/javascript/electronNativeLoaderPaths.test.ts — 18 passed.
node scripts/rea.mjs analyze-javascript-application <fixture> --format json — ordinary and optional fixtures verified before and after, as above.
npm run check — passed.
npm run verify:test-discovery — 1,369 test files discovered exactly once.
git diff --check — passed.
Pre-commit formatting/lint and pre-push npm run check:fast — passed; hooks preserved the tested file contents.

Compatibility, safety, and release

No breaking changes, migrations, package metadata changes, or new target interaction. Full suite/coverage, MCP-specific transport, package verification, and native-provider lanes were not run; this changes static request extraction and does not claim native execution or export verification. No performance improvement is claimed.

Author readiness

Human owner: @ogutdgn

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.

Maintainer blocker verification

The Codex failure summary has no actionable inline finding or active review thread; its execution comment reports an unknown review error. Current source and the independent human review agree on the optional-binding fix. On the exact current head, Node 24.18.0/npm 11.16.0: the focused nativeOptionalBindings, nativeTypeBindings and electronNativeLoaderPaths suites passed 18/18, and check:fast passed. No additional code change or runtime provider claim is made.

_{🔄 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/4259 **Author:** [@ogutdgn](https://github.com/ogutdgn) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/javascript-optional-native-addon-bindings` --- ### 📝 Commits (1) - [`01599e7`](https://github.com/morluto/rea/commit/01599e70681482f5c6a790a4275ecfbbe3201980) fix(javascript): preserve optional native-addon bindings ### 📊 Changes **2 files changed** (+98 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/electronStaticAnalysisNative.ts` (+2 -1) ➕ `tests/boundary/javascript/nativeOptionalBindings.test.ts` (+96 -0) </details> ### 📄 Description ## Summary Preserve native-addon binding records for optional CommonJS calls such as `const addon = require?.("./addon.node")`. Fixes #3885. ## Problem and expected behavior REA already resolves the dependency for this syntax, but the native collector rejects Babel's `OptionalCallExpression`. This drops the binding, native-export graph node, and summary counts. Optional and ordinary calls should preserve the same static request information without claiming that the addon executes or implements the requested members. ## Change and scope Accept both call node types in the existing `nativeRequire` helper. Existing loader-name, path, member, graph, summary, and handoff logic handles the result; no downstream workaround or additional representation is introduced. Add application-boundary regressions for namespace access, destructuring, optional member access, dynamic members, re-export, and negative controls. ## Contract and boundary impact - Semantic owner and earliest changed stage: native request extraction in `electronStaticAnalysisNative.ts`. - Contribution scope / maintainer decision: ordinary correctness fix within existing static-analysis boundaries; advance scope decision not applicable. - CLI and MCP/tool-catalog contract: no input or schema changes; existing output now retains the omitted native finding. - Provider, bridge, target-format, or platform compatibility: no provider/bridge changes; existing JavaScript/CommonJS syntax. - Evidence, artifact, provenance, or reconstruction contract: retain source-derived requests and existing provenance; native-export verification remains `not-performed`. - Target selection, authority, and cleanup: unchanged file-reading analysis; no target execution or mutation added. - Generated metadata, 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 Executed on macOS arm64 with Node 24.18.0, based on upstream `2202e3f0ba556d40cc6e598df1ecbdf1eda56c13`: - Exact new regression against unpatched production source: **5 failed / 3 passed**. The five optional-call cases fail; negative controls pass. - Patched focused suite: **18 passed across 3 files**. - Rebuilt public CLI, with inert `.node` fixture bytes: | Input | Before: bindings / resolved / nodes | After: bindings / resolved / nodes | | --- | --- | --- | | `require("./addon.node")` | 1 / 1 / 1 | 1 / 1 / 1 | | `require?.("./addon.node")` | 0 / 0 / 0 | 1 / 1 / 1 | The regression also checks the retained resolved dependency and requested-member handoff. Addon bytes are never executed. - [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 - `npm run test:focused -- tests/boundary/javascript/nativeOptionalBindings.test.ts tests/boundary/javascript/nativeTypeBindings.test.ts src/domain/javascript/electronNativeLoaderPaths.test.ts` — 18 passed. - `node scripts/rea.mjs analyze-javascript-application <fixture> --format json` — ordinary and optional fixtures verified before and after, as above. - `npm run check` — passed. - `npm run verify:test-discovery` — 1,369 test files discovered exactly once. - `git diff --check` — passed. - Pre-commit formatting/lint and pre-push `npm run check:fast` — passed; hooks preserved the tested file contents. ## Compatibility, safety, and release No breaking changes, migrations, package metadata changes, or new target interaction. Full suite/coverage, MCP-specific transport, package verification, and native-provider lanes were not run; this changes static request extraction and does not claim native execution or export verification. No performance improvement is claimed. ## Author readiness Human owner: @ogutdgn - [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. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims.  ### Maintainer blocker verification The Codex failure summary has no actionable inline finding or active review thread; its execution comment reports an unknown review error. Current source and the independent human review agree on the optional-binding fix. On the exact current head, Node 24.18.0/npm 11.16.0: the focused nativeOptionalBindings, nativeTypeBindings and electronNativeLoaderPaths suites passed 18/18, and check:fast passed. No additional code change or runtime provider claim is made. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:12:12 +00:00