morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #4784] fix(javascript): preserve CommonJS exports through TypeScript wrappers #6407

New issue

Open

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4784
Author: @aoli0919
Created: 10/17/2026
Status: 🔄 Open

Base: main ← Head: fix/commonjs-typescript-receivers

📝 Commits (4)

a2ffc3f fix(javascript): preserve CommonJS exports through TypeScript wrappers
1071216 Merge remote-tracking branch 'origin/main' into HEAD
2865805 fix(javascript): reconcile TypeScript receivers with upstream lifetimes
72d97a0 fix(javascript): normalize wrapped CommonJS delete targets

📊 Changes

2 files changed (+136 additions, -20 deletions)

View changed files

📝 src/domain/javascript/javascriptCommonJsExportReceivers.ts (+34 -20)
📝 tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts (+102 -0)

📄 Description

Summary

Retain CommonJS exports when TypeScript wraps their receivers in runtime-erased expressions. For example, (exports as Record<string, unknown>).answer = 42 currently omits answer from the module's export_names and export relationships.

No separate issue is linked because the minimal reproduction and executed regression evidence are included below. Related #4725 addresses descriptor-defined exports; this change addresses receiver normalization.

Problem and expected behavior

These valid TypeScript writes target the same CommonJS objects as their unwrapped JavaScript equivalents:

(exports as Record<string, unknown>).asserted = 1;
(module!.exports satisfies Record<string, unknown>).checked = 2;
(<Record<string, unknown>>module.exports).cast = 3;
module.exports!.nonNull = 4;

The new production-graph regression returned export_names: [] before the fix. It should retain asserted, cast, checked, and nonNull, with their exposes relationships. Erasing a type wrapper must also preserve lexical shadowing and the distinction between module.exports and a detached exports receiver.

Change and scope

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Reuse unwrapJavaScriptExpression inside the existing CommonJS receiver owner before recognizing globals, module exports, and assignment receivers. Apply the same recognition to lifetime tracking so export names and receiver identities agree. Normalize delete/update arguments at this owner so name recognition, member checks, receiver selection, and write tracking use the same runtime expression. Retain production-graph coverage for all four wrapper forms, detached exports, and shadowed parameters; extend it to whole-property deletion wrappers.

This is a static-analysis correctness fix within existing boundaries. A separate maintainer scope decision is not applicable; no new provider, target execution, injection, mutation, or protection-recovery capability is introduced.

The branch now merges upstream main at aba487eec while retaining upstream value-only traversal, descriptor handling, and per-field publication/deletion tracking. The new deletion receiver check also uses the wrapper-aware global recognizer so detached exports cannot invalidate a live public field.

Contract and boundary impact

Semantic owner and earliest changed stage: javascriptCommonJsExportReceivers.ts, CommonJS syntax recognition and receiver identity indexing.
Contribution scope fit and linked maintainer decision: existing static analysis; separate scope decision not applicable.
CLI and MCP/tool-catalog contract: unchanged schemas and operations; corrected export facts flow through the shared analysis.
Provider, bridge, target-format, or platform compatibility: TypeScript CommonJS parsing; no provider or bridge changes.
Evidence, artifact, provenance, or reconstruction contract: retain previously omitted export names and relationships; existing identity and uncertainty handling remain in place.
Target selection, authority, upstream behavior, and cleanup details (if applicable): caller-selected static inputs and existing artifact/worker ownership are unchanged. No target process is launched or attached, and no target code execution is added.
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.

The effects above describe target-facing analysis. The existing internal analysis workers are unchanged; the process-launch checkbox is intentionally unchecked because this fix does not launch or attach to a target process.

Evidence and regression coverage

Tests added or updated: the two original graph regressions plus four parameterized wrapper/deletion cases in javascriptModuleRelationships.test.ts. Each deletion case now covers both a wrapped receiver and a wrapper around the whole property, asserting that deleted export bindings are absent. The detached-receiver control retains its original deletion and adds whole-argument-wrapped deletion and block-scoped shadowed exports/module controls.
Base reproduction or other evidence: on pre-follow-up head 286580522, all four extended wrapper/deletion cases failed with export_names: ["intact", "wrappedDeleted"] instead of ["intact"] (16 passed, 4 failed in the boundary file). Normalizing the delete argument before member/receiver checks made all four pass. The completed six-file focused run passed all 178 tests. The earlier detached-receiver red/green reproduction remains documented in the sync comment.
User-visible CLI/MCP output (if applicable): the compiled CLI was run on three self-owned .cts fixtures on the follow-up tree. The whole-argument-wrapped delete returned only ["intact"], with no deleted export-binding node; the four receiver wrapper forms returned ["asserted", "cast", "checked", "nonNull"]; the detached-receiver/deletion fixture returned ["connected", "default"].
Remaining proof gaps: the complete test suite and coverage were not run locally. No third-party application, MCP client, Windows/Linux host, or external native-analysis engine was exercised. Current-head CI and PR title workflows require maintainer approval; no successful full CI result is claimed.

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

Executed locally for the follow-up tree committed as 72d97a0d5f57b18cd2ef8434861f791999486d93 on macOS arm64, using the repository-pinned Node.js 24.22.1 and npm 11.16.0. Darwin helper compilation explicitly selected the installed Swift 6.3.3 compiler through REA_DARWIN_SWIFTC_PATH and the installed macOS 26.5 SDK through SDKROOT; no global toolchain configuration was changed.

npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts src/domain/javascript/javascriptSemanticAnalysis.export_lifetimes.test.ts src/domain/javascript/javascriptTypeErasure.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts tests/boundary/filesystem/javascript-applications/javascriptApplicationTypeScript.test.ts - passed: 178 tests across six files, including all four whole-argument wrapper forms and the retained/extended shadowing and detached-receiver controls.
npm run check - passed: typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards. Lint reported zero errors and 2,742 warnings.
npm run verify:test-discovery - passed: all 2,450 test files discovered exactly once.
node scripts/rea.mjs analyze-javascript-application <self-owned-whole-delete-fixture> --json - passed: export_names was ["intact"], and the graph had no deleted export-binding node.
node scripts/rea.mjs analyze-javascript-application <self-owned-wrapped-fixture> --json - passed: export_names was ["asserted", "cast", "checked", "nonNull"].
node scripts/rea.mjs analyze-javascript-application <self-owned-detached-fixture> --json - passed: export_names was ["connected", "default"] after deleting through the detached wrapped exports receiver.
git diff --check - passed.
Pre-push npm run check:fast - passed.

Red/green for this follow-up was executed before changing production code: the four extended deletion cases failed on 286580522, then passed after the shared-owner normalization. Earlier SDK-interrupted attempts and the detached-receiver red/green result remain recorded in the prior sync evidence; they are not counted as passes for this head.

GitHub workflow state for 72d97a0d5 was checked after pushing:

CI: action_required, awaiting maintainer approval.
PR title: action_required, awaiting maintainer approval.
PR readiness and Greptile review were triggered for this head; neither replaces full CI.

These local results do not replace the required current-head CI results. The full deterministic suite, coverage, and real external-provider lanes remain unverified.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 focused source/boundary tests and compiled CLI analysis of self-owned TypeScript fixtures. No external native-analysis engine or browser was exercised.
Package or release metadata impact: none.
Security, privacy, process, or containment review: source inspection and self-owned fixtures only; no added target execution, network access, mutation, or authority.

Author readiness

Human owner: @aoli0919

Prepared with AI assistance. Human owner @aoli0919 has confirmed review and requested initial maintainer review.

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/4784 **Author:** [@aoli0919](https://github.com/aoli0919) **Created:** 10/17/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/commonjs-typescript-receivers` --- ### 📝 Commits (4) - [`a2ffc3f`](https://github.com/morluto/rea/commit/a2ffc3fe1f86f35e09dbd0ba8d1713e68f934530) fix(javascript): preserve CommonJS exports through TypeScript wrappers - [`1071216`](https://github.com/morluto/rea/commit/1071216f53a19eea7ef910004604d3e7f02135cc) Merge remote-tracking branch 'origin/main' into HEAD - [`2865805`](https://github.com/morluto/rea/commit/28658052228d0ec975dd2219db0f5f28d0afca9e) fix(javascript): reconcile TypeScript receivers with upstream lifetimes - [`72d97a0`](https://github.com/morluto/rea/commit/72d97a0d5f57b18cd2ef8434861f791999486d93) fix(javascript): normalize wrapped CommonJS delete targets ### 📊 Changes **2 files changed** (+136 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptCommonJsExportReceivers.ts` (+34 -20) 📝 `tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts` (+102 -0) </details> ### 📄 Description  ## Summary  Retain CommonJS exports when TypeScript wraps their receivers in runtime-erased expressions. For example, `(exports as Record<string, unknown>).answer = 42` currently omits `answer` from the module's `export_names` and export relationships. No separate issue is linked because the minimal reproduction and executed regression evidence are included below. Related #4725 addresses descriptor-defined exports; this change addresses receiver normalization. ## Problem and expected behavior  These valid TypeScript writes target the same CommonJS objects as their unwrapped JavaScript equivalents: ```ts (exports as Record<string, unknown>).asserted = 1; (module!.exports satisfies Record<string, unknown>).checked = 2; (<Record<string, unknown>>module.exports).cast = 3; module.exports!.nonNull = 4; ``` The new production-graph regression returned `export_names: []` before the fix. It should retain `asserted`, `cast`, `checked`, and `nonNull`, with their `exposes` relationships. Erasing a type wrapper must also preserve lexical shadowing and the distinction between `module.exports` and a detached `exports` receiver. ## Change and scope  Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. Reuse `unwrapJavaScriptExpression` inside the existing CommonJS receiver owner before recognizing globals, module exports, and assignment receivers. Apply the same recognition to lifetime tracking so export names and receiver identities agree. Normalize delete/update arguments at this owner so name recognition, member checks, receiver selection, and write tracking use the same runtime expression. Retain production-graph coverage for all four wrapper forms, detached exports, and shadowed parameters; extend it to whole-property deletion wrappers. This is a static-analysis correctness fix within existing boundaries. A separate maintainer scope decision is not applicable; no new provider, target execution, injection, mutation, or protection-recovery capability is introduced. The branch now merges upstream `main` at `aba487eec` while retaining upstream value-only traversal, descriptor handling, and per-field publication/deletion tracking. The new deletion receiver check also uses the wrapper-aware global recognizer so detached `exports` cannot invalidate a live public field. ## Contract and boundary impact  - Semantic owner and earliest changed stage: `javascriptCommonJsExportReceivers.ts`, CommonJS syntax recognition and receiver identity indexing. - Contribution scope fit and linked maintainer decision: existing static analysis; separate scope decision not applicable. - CLI and MCP/tool-catalog contract: unchanged schemas and operations; corrected export facts flow through the shared analysis. - Provider, bridge, target-format, or platform compatibility: TypeScript CommonJS parsing; no provider or bridge changes. - Evidence, artifact, provenance, or reconstruction contract: retain previously omitted export names and relationships; existing identity and uncertainty handling remain in place. - Target selection, authority, upstream behavior, and cleanup details (if applicable): caller-selected static inputs and existing artifact/worker ownership are unchanged. No target process is launched or attached, and no target code execution is added. - 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. The effects above describe target-facing analysis. The existing internal analysis workers are unchanged; the process-launch checkbox is intentionally unchecked because this fix does not launch or attach to a target process. ## Evidence and regression coverage  - Tests added or updated: the two original graph regressions plus four parameterized wrapper/deletion cases in `javascriptModuleRelationships.test.ts`. Each deletion case now covers both a wrapped receiver and a wrapper around the whole property, asserting that deleted export bindings are absent. The detached-receiver control retains its original deletion and adds whole-argument-wrapped deletion and block-scoped shadowed `exports`/`module` controls. - Base reproduction or other evidence: on pre-follow-up head `286580522`, all four extended wrapper/deletion cases failed with `export_names: ["intact", "wrappedDeleted"]` instead of `["intact"]` (16 passed, 4 failed in the boundary file). Normalizing the delete argument before member/receiver checks made all four pass. The completed six-file focused run passed all 178 tests. The earlier detached-receiver red/green reproduction remains documented in the sync comment. - User-visible CLI/MCP output (if applicable): the compiled CLI was run on three self-owned `.cts` fixtures on the follow-up tree. The whole-argument-wrapped delete returned only `["intact"]`, with no `deleted` export-binding node; the four receiver wrapper forms returned `["asserted", "cast", "checked", "nonNull"]`; the detached-receiver/deletion fixture returned `["connected", "default"]`. - Remaining proof gaps: the complete test suite and coverage were not run locally. No third-party application, MCP client, Windows/Linux host, or external native-analysis engine was exercised. Current-head CI and PR title workflows require maintainer approval; no successful full CI result is claimed. 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  Executed locally for the follow-up tree committed as `72d97a0d5f57b18cd2ef8434861f791999486d93` on macOS arm64, using the repository-pinned Node.js 24.22.1 and npm 11.16.0. Darwin helper compilation explicitly selected the installed Swift 6.3.3 compiler through `REA_DARWIN_SWIFTC_PATH` and the installed macOS 26.5 SDK through `SDKROOT`; no global toolchain configuration was changed. - `npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts src/domain/javascript/javascriptSemanticAnalysis.export_lifetimes.test.ts src/domain/javascript/javascriptTypeErasure.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleRelationships.test.ts tests/boundary/filesystem/javascript-applications/javascriptApplicationTypeScript.test.ts` - passed: 178 tests across six files, including all four whole-argument wrapper forms and the retained/extended shadowing and detached-receiver controls. - `npm run check` - passed: typecheck, lint, formatting, knip, metadata, module boundaries, and architecture guards. Lint reported zero errors and 2,742 warnings. - `npm run verify:test-discovery` - passed: all 2,450 test files discovered exactly once. - `node scripts/rea.mjs analyze-javascript-application <self-owned-whole-delete-fixture> --json` - passed: `export_names` was `["intact"]`, and the graph had no `deleted` export-binding node. - `node scripts/rea.mjs analyze-javascript-application <self-owned-wrapped-fixture> --json` - passed: `export_names` was `["asserted", "cast", "checked", "nonNull"]`. - `node scripts/rea.mjs analyze-javascript-application <self-owned-detached-fixture> --json` - passed: `export_names` was `["connected", "default"]` after deleting through the detached wrapped `exports` receiver. - `git diff --check` - passed. - Pre-push `npm run check:fast` - passed. Red/green for this follow-up was executed before changing production code: the four extended deletion cases failed on `286580522`, then passed after the shared-owner normalization. Earlier SDK-interrupted attempts and the detached-receiver red/green result remain recorded in the prior sync evidence; they are not counted as passes for this head. GitHub workflow state for `72d97a0d5` was checked after pushing: - [CI](https://github.com/morluto/rea/actions/runs/40682302422): `action_required`, awaiting maintainer approval. - [PR title](https://github.com/morluto/rea/actions/runs/40682302389): `action_required`, awaiting maintainer approval. - PR readiness and Greptile review were triggered for this head; neither replaces full CI. These local results do not replace the required current-head CI results. The full deterministic suite, coverage, and real external-provider lanes remain unverified. ## Compatibility, safety, and release  - Breaking changes or migration steps: none.  - Real Hopper/Ghidra, browser, or OS coverage: macOS arm64 focused source/boundary tests and compiled CLI analysis of self-owned TypeScript fixtures. No external native-analysis engine or browser was exercised. - Package or release metadata impact: none. - Security, privacy, process, or containment review: source inspection and self-owned fixtures only; no added target execution, network access, mutation, or authority. ## Author readiness  Human owner: @aoli0919 Prepared with AI assistance. Human owner @aoli0919 has confirmed review and requested initial maintainer review. - [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 added the

pull-request

label

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

bsakaguchi referenced this issue

2026-11-01 21:11:00 +00:00

[GH-ISSUE #5653] [Tracker] New issue intake by area (October 2026) #3423