[PR #4725] [MERGED] fix(javascript): retain CommonJS exports defined with Object.defineProperty #6373

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:13:58 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4725
Author: @rodricksz4h5
Created: 10/17/2026
Status: ✅ Merged
Merged: 10/17/2026
Merged by: @morluto

Base: main ← Head: fix/cjs-defineproperty-exports

📝 Commits (2)

c361719 fix(javascript): retain CommonJS exports defined with Object.defineProperty
6166cc6 fix(javascript): unwrap assignment chains in defined CommonJS export values

📊 Changes

3 files changed (+214 additions, -11 deletions)

View changed files

📝 src/domain/javascript/javascriptCommonJsExportReceivers.ts (+62 -6)
📝 src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts (+114 -0)
📝 src/domain/javascript/javascriptSemanticProjection.ts (+38 -5)

📄 Description

Summary

Fixes #4724

Babel and Rollup write CommonJS re-exports as Object.defineProperty(exports, "Client", { enumerable: true, get }). The semantic projection only recognised export assignments, so these names were missing from a module's export_names and exposes edges. The call is now read as a commonjs-export link, with the same receiver rules as an assignment.

Problem and expected behavior

Smallest trigger, from Babel 8 / Rollup 4 CJS output of export { Client } from "./client":

Object.defineProperty(exports, "Client", { enumerable: true, get: function () { return _client.Client; } });

collectSemanticModuleLink sent only AssignmentExpressions to collectCommonJsExport; a CallExpression went only to collectUnboundRequireLink, so no export link was produced. defineProperty on the exports object defines the same own property as exports.Client = ..., and REA's bundler-factory collector (collectCallExports in javascriptAstFingerprint.ts) already counts it as an export.

Change and scope

javascriptCommonJsExportReceivers.ts, the syntax owner for CJS export names and receivers: commonJsDefinedExport recognises Object.defineProperty(exports | module.exports, key, descriptor) using the existing checks (isUnmodifiedGlobalReference for an unshadowed, unpatched Object.defineProperty; isUnshadowedGlobal / isCommonJsModuleExports for the receiver; semanticStaticPropertyKey for the key, with a dynamic key reported as "*" like exports[key] = ...). The receiver index captures these calls with the same rule as assignments, so a replaced exports or module.exports is still detached or uncertain.
javascriptSemanticProjection.ts: the call branch emits the link through the same builder as collectCommonJsExport (shared as addCommonJsExport, which also takes the rightmost value of an assignment chain for both forms). A data descriptor's value (read with semanticFinalObjectProperty) is linked like an assigned value, including require origins; an accessor's result stays unknown (localName/callableId null), since the descriptor does not prove what the getter returns.
__esModule from Object.defineProperty(exports, "__esModule", { value: true }) now appears in export_names, as exports.__esModule = true already does on main and as collectCallExports does.
Not included: esbuild's __export(target, { ... }) helper form and resolving a getter's returned binding to its source module.

Follow the contribution scope.
Scope decision: not applicable (ordinary fix within existing boundaries).

Contract and boundary impact

Semantic owner and earliest changed stage: CommonJS export recognition in javascriptCommonJsExportReceivers.ts, consumed by the semantic projection.
Contribution scope fit and linked maintainer decision: not applicable.
CLI and MCP/tool-catalog contract: no schema change; export_names and exposes gain the defined names.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: more commonjs-export links for Babel/Rollup/tsc CJS output; __esModule listed for files that define it.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
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: javascriptSemanticAnalysis.structure.test.ts, CommonJS exports defined with Object.defineProperty (28 cases):
- compiler output with __esModule, an accessor re-export and assignments;
- a data descriptor links exactly like the equivalent assignment for a function, require(...), require(...).x, an identifier and assignment chains (holder.x = require(...).x, a = b = () => 1);
- connected, detached and uncertain receivers, taken from the existing receiver-lifetime tables;
- not exports: shadowed or patched Object, other targets, module, a local exports parameter, a missing descriptor, spread arguments;
- a dynamic key stays "*".
13 of them fail on main. With only the new receiver capture disabled, the 7 detached/uncertain cases fail, so they exercise receiver tracking.
Base reproduction (executed): REA built from source on Linux for main and this branch, node scripts/rea.mjs analyze-javascript-application <dir> --format json on one barrel (export *, export { Client }, export { Client as RenamedClient }, export const VERSION) compiled by the real compilers in REA's node_modules:

Compiler	main `c7e9e096`	this PR
Babel 8.0.7	`["VERSION"]`, no `Client` edge	`["*","Client","RenamedClient","VERSION","__esModule"]`, `Client` edge
Rollup 4.64.1	`["VERSION"]`, no `Client` edge	`["*","Client","RenamedClient","VERSION"]`, `Client` edge
tsc 7.0.2	`["Client","RenamedClient","VERSION"]`	same plus `__esModule`

"*" comes from the export * loop (Object.defineProperty(exports, key, ...) with a dynamic key inside a callback), reported like exports[key] = ....

User-visible CLI/MCP output (if applicable): above.
Remaining proof gaps: no real published package was analysed; MCP not run separately (same projection).

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 src/domain/javascript/ (Windows) — 85 files, 2223 tests passed
Linux (node:24.18.0-bookworm, npm ci + npm run build:cached), vitest run on every test path matching javascript/electron/commonjs/require/bundle/asar/semantic/application/unbound — 380 files, 5081 passed, 1 failed: hopperPublisherRejectionCli.test.ts, which fails the same way on main (Hopper publisher verification in a Linux container)
turbo run typecheck lint:built format:check knip metadata:check, npm run verify:architecture-guards, npm run verify:module-boundaries — pass

Compatibility, safety, and release

Breaking changes or migration steps: none; additive export links.
Real Hopper/Ghidra, browser, or OS coverage: not applicable.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none; static analysis of existing files.

Author readiness

Human owner: @rodricksz4h5

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/4725 **Author:** [@rodricksz4h5](https://github.com/rodricksz4h5) **Created:** 10/17/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/cjs-defineproperty-exports` --- ### 📝 Commits (2) - [`c361719`](https://github.com/morluto/rea/commit/c36171953e8b003189686ada534c641dcdc2ee2f) fix(javascript): retain CommonJS exports defined with Object.defineProperty - [`6166cc6`](https://github.com/morluto/rea/commit/6166cc6100d6c1e19b8ca3102af8edddbdd59367) fix(javascript): unwrap assignment chains in defined CommonJS export values ### 📊 Changes **3 files changed** (+214 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptCommonJsExportReceivers.ts` (+62 -6) 📝 `src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts` (+114 -0) 📝 `src/domain/javascript/javascriptSemanticProjection.ts` (+38 -5) </details> ### 📄 Description ## Summary Fixes #4724 Babel and Rollup write CommonJS re-exports as `Object.defineProperty(exports, "Client", { enumerable: true, get })`. The semantic projection only recognised export assignments, so these names were missing from a module's `export_names` and `exposes` edges. The call is now read as a `commonjs-export` link, with the same receiver rules as an assignment. ## Problem and expected behavior Smallest trigger, from Babel 8 / Rollup 4 CJS output of `export { Client } from "./client"`: ```js Object.defineProperty(exports, "Client", { enumerable: true, get: function () { return _client.Client; } }); ``` `collectSemanticModuleLink` sent only `AssignmentExpression`s to `collectCommonJsExport`; a `CallExpression` went only to `collectUnboundRequireLink`, so no export link was produced. `defineProperty` on the exports object defines the same own property as `exports.Client = ...`, and REA's bundler-factory collector (`collectCallExports` in `javascriptAstFingerprint.ts`) already counts it as an export. ## Change and scope - `javascriptCommonJsExportReceivers.ts`, the syntax owner for CJS export names and receivers: `commonJsDefinedExport` recognises `Object.defineProperty(exports | module.exports, key, descriptor)` using the existing checks (`isUnmodifiedGlobalReference` for an unshadowed, unpatched `Object.defineProperty`; `isUnshadowedGlobal` / `isCommonJsModuleExports` for the receiver; `semanticStaticPropertyKey` for the key, with a dynamic key reported as `"*"` like `exports[key] = ...`). The receiver index captures these calls with the same rule as assignments, so a replaced `exports` or `module.exports` is still `detached` or `uncertain`. - `javascriptSemanticProjection.ts`: the call branch emits the link through the same builder as `collectCommonJsExport` (shared as `addCommonJsExport`, which also takes the rightmost value of an assignment chain for both forms). A data descriptor's `value` (read with `semanticFinalObjectProperty`) is linked like an assigned value, including `require` origins; an accessor's result stays unknown (`localName`/`callableId` null), since the descriptor does not prove what the getter returns. - `__esModule` from `Object.defineProperty(exports, "__esModule", { value: true })` now appears in `export_names`, as `exports.__esModule = true` already does on main and as `collectCallExports` does. - Not included: esbuild's `__export(target, { ... })` helper form and resolving a getter's returned binding to its source module. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable (ordinary fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: CommonJS export recognition in `javascriptCommonJsExportReceivers.ts`, consumed by the semantic projection. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: no schema change; `export_names` and `exposes` gain the defined names. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: more `commonjs-export` links for Babel/Rollup/tsc CJS output; `__esModule` listed for files that define it. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - 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: `javascriptSemanticAnalysis.structure.test.ts`, `CommonJS exports defined with Object.defineProperty` (28 cases): - compiler output with `__esModule`, an accessor re-export and assignments; - a data descriptor links exactly like the equivalent assignment for a function, `require(...)`, `require(...).x`, an identifier and assignment chains (`holder.x = require(...).x`, `a = b = () => 1`); - connected, detached and uncertain receivers, taken from the existing receiver-lifetime tables; - not exports: shadowed or patched `Object`, other targets, `module`, a local `exports` parameter, a missing descriptor, spread arguments; - a dynamic key stays `"*"`. 13 of them fail on main. With only the new receiver capture disabled, the 7 detached/uncertain cases fail, so they exercise receiver tracking. - Base reproduction (executed): REA built from source on Linux for main and this branch, `node scripts/rea.mjs analyze-javascript-application <dir> --format json` on one barrel (`export *`, `export { Client }`, `export { Client as RenamedClient }`, `export const VERSION`) compiled by the real compilers in REA's `node_modules`: | Compiler | main c7e9e096 | this PR | | --- | --- | --- | | Babel 8.0.7 | `["VERSION"]`, no `Client` edge | `["*","Client","RenamedClient","VERSION","__esModule"]`, `Client` edge | | Rollup 4.64.1 | `["VERSION"]`, no `Client` edge | `["*","Client","RenamedClient","VERSION"]`, `Client` edge | | tsc 7.0.2 | `["Client","RenamedClient","VERSION"]` | same plus `__esModule` | `"*"` comes from the `export *` loop (`Object.defineProperty(exports, key, ...)` with a dynamic key inside a callback), reported like `exports[key] = ...`. - User-visible CLI/MCP output (if applicable): above. - Remaining proof gaps: no real published package was analysed; MCP not run separately (same projection). 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 src/domain/javascript/` (Windows) — 85 files, 2223 tests passed - Linux (`node:24.18.0-bookworm`, `npm ci` + `npm run build:cached`), `vitest run` on every test path matching javascript/electron/commonjs/require/bundle/asar/semantic/application/unbound — 380 files, 5081 passed, 1 failed: `hopperPublisherRejectionCli.test.ts`, which fails the same way on main (Hopper publisher verification in a Linux container) - `turbo run typecheck lint:built format:check knip metadata:check`, `npm run verify:architecture-guards`, `npm run verify:module-boundaries` — pass ## Compatibility, safety, and release - Breaking changes or migration steps: none; additive export links. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; static analysis of existing files. ## Author readiness Human owner: @rodricksz4h5 - [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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:13:58 +00:00