[PR #3566] [CLOSED] fix(javascript): ignore exports writes after the alias is detached #5829

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:09:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3566
Author: @maliv2
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/3541-detached-commonjs-exports-receiver

📝 Commits (2)

36dca1b fix(javascript): ignore exports writes after the alias is detached
e530154 fix(javascript): keep exports writes a later assignment can export

📊 Changes

3 files changed (+444 additions, -21 deletions)

View changed files

➕ src/domain/javascript/javascriptCommonJsExportsAlias.ts (+280 -0)
➕ src/domain/javascript/javascriptSemanticAnalysis.commonjs_exports_alias.test.ts (+158 -0)
📝 src/domain/javascript/javascriptSemanticProjection.ts (+6 -21)

📄 Description

Summary

Fixes #3541. After module.exports = {} (or exports = {}), a later top-level exports.name = value writes to an object the module does not export, but the JavaScript semantic projection still emitted a commonjs-export link for it. Export-shape comparison then reported a definite change for an export Node does not have.

Problem and expected behavior

module.exports = {};
exports.parse = () => ({ kind: "result", count: 1 });

require() of this file has no parse property, yet the IR contained a commonjs-export link named parse. The existing receiver guard (commonJsExportReceiverEffect) only inspected the replacement inside the same assignment chain, not the lifetime of the exports alias across statements.

Expected: a provably detached receiver creates no export link; unproven ordering stays an export candidate.

Change and scope

New module javascriptCommonJsExportsAlias.ts owns the exports / module.exports relation. It walks the top-level expression statements in order and tracks connected | detached | unknown:

module.exports = <fresh value> or exports = <fresh value> (object, array, function, class) detaches the alias.
exports = module.exports, module.exports = exports, and chains naming both reconnect it.
A write to exports.X is skipped only when it is itself a straight-line top-level statement and the relation before it is detached.
If any write to exports/module/module.exports or any other use of module (including calling a member such as module.valueOf()) lies outside those recognized statements, the whole timeline is treated as unordered and nothing is skipped.
A write is also kept when a later top-level rebinding assigns module.exports a value that is not freshly created (module.exports = exports), since that object may become the export.
A statement that itself rebinds the alias, or whose replacement chain contains a compound/logical assignment (cache ||= {}), gets no detached verdict.

Conditional, function-hosted, non-fresh (module.exports = require(...)) and compound cases keep today's behavior. isCommonJsModuleExports and isFreshAssignmentResult moved from javascriptSemanticProjection.ts into the new module so there is one owner. Not included: Object.defineProperty(exports, ...) / Object.assign(exports, ...) forms, and writes through a saved old alias (e.g. const e = exports), which the issue lists as still missing.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScript semantic projection (collectCommonJsExport), the producer of commonjs-export module links.
Contribution scope fit and linked maintainer decision: not applicable (bug fix inside existing static-analysis boundaries).
CLI and MCP/tool-catalog contract: none. Fewer false commonjs-export links in analyze_javascript_application evidence.
Provider, bridge, target-format, or platform compatibility: none; static source analysis only.
Evidence, artifact, provenance, or reconstruction contract: unchanged schema; unproven cases remain candidates rather than being dropped.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
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 (source tests):

Tests added: src/domain/javascript/javascriptSemanticAnalysis.commonjs_exports_alias.test.ts (20 cases): 5 detached cases, 5 connected/untouched controls, 10 unproven-order cases that must stay candidates (including the three review findings: logical assignment in the replacement chain, a write whose own statement reconnects the alias, a call through module, and a later module.exports = exports that exports the written object).
Base reproduction: with javascriptSemanticProjection.ts reverted to main, 4 of the first 13 cases fail (the detached cases: parse/status still appear in the exported names); the 9 controls pass on both revisions.
Executed CLI reproduction of #3541 (Linux x64, Node 24.18.0, built from this commit): two directories with module.exports = {}; exports.parse = () => ({ kind: "result", count: N }); (N = 1 / 2), analyze-javascript-application on each, then compare-javascript-export-shapes selecting app.cjs / parse:
- base projection (javascriptSemanticProjection.ts reverted to main): left/right status: "selected", summary: {changed: 1, unknown: 0}, coverage complete-within-inputs.
- this PR: left/right status: "missing", candidates only default, summary: {changed: 0, unknown: 1}, coverage partial.
Not run: real stdio MCP transport for this reproduction (CLI only); real-provider lanes.
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

Windows 11, Node 26.4.0 (npm ci --ignore-scripts), on the final commit:

node scripts/run-development-tests.mjs local on all src/domain/javascript/*.test.ts and src/application/javascript/*.test.ts (80 files) — 1949 passed
npx tsc --noEmit — exit 0
npm run lint (oxlint + module boundaries) — exit 0
npx knip — exit 0
npx oxfmt --check src/domain/javascript — clean

Linux x64, Node 24.18.0, on the final commit:

npm run test:focused -- tests/acceptance/applications/unboundRequires.test.ts plus the new and structure test files — 50 passed
CLI reproduction of #3541 (see above) was run on the earlier revision of this branch; the later review fixes only narrow when a write is skipped and were not re-run through the CLI.
npm run check:fast (typecheck + lint) passed on the earlier revision.

Not run: npm run check, docs:check, check:pr, real-provider lanes.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis only; verified on Linux x64 and Windows source tests).
Package or release metadata impact: none.
Security, privacy, process, or containment review: static parsing only; no execution of analyzed code.

Author readiness

Human owner: @maliv2

This change was prepared with AI assistance. The human owner (@maliv2) reviewed the diff and told the assistant in chat that everything was checked with no problems; the declarations below were completed on that instruction.

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/3566 **Author:** [@maliv2](https://github.com/maliv2) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3541-detached-commonjs-exports-receiver` --- ### 📝 Commits (2) - [`36dca1b`](https://github.com/morluto/rea/commit/36dca1b765aee4e9c6c89f5226dcc26df0a4fbe4) fix(javascript): ignore exports writes after the alias is detached - [`e530154`](https://github.com/morluto/rea/commit/e530154b232362f648b66274614da852acd8194d) fix(javascript): keep exports writes a later assignment can export ### 📊 Changes **3 files changed** (+444 additions, -21 deletions) <details> <summary>View changed files</summary> ➕ `src/domain/javascript/javascriptCommonJsExportsAlias.ts` (+280 -0) ➕ `src/domain/javascript/javascriptSemanticAnalysis.commonjs_exports_alias.test.ts` (+158 -0) 📝 `src/domain/javascript/javascriptSemanticProjection.ts` (+6 -21) </details> ### 📄 Description ## Summary Fixes #3541. After `module.exports = {}` (or `exports = {}`), a later top-level `exports.name = value` writes to an object the module does not export, but the JavaScript semantic projection still emitted a `commonjs-export` link for it. Export-shape comparison then reported a definite change for an export Node does not have. ## Problem and expected behavior ```js module.exports = {}; exports.parse = () => ({ kind: "result", count: 1 }); ``` `require()` of this file has no `parse` property, yet the IR contained a `commonjs-export` link named `parse`. The existing receiver guard (`commonJsExportReceiverEffect`) only inspected the replacement inside the same assignment chain, not the lifetime of the `exports` alias across statements. Expected: a provably detached receiver creates no export link; unproven ordering stays an export candidate. ## Change and scope New module `javascriptCommonJsExportsAlias.ts` owns the `exports` / `module.exports` relation. It walks the top-level expression statements in order and tracks `connected | detached | unknown`: - `module.exports = <fresh value>` or `exports = <fresh value>` (object, array, function, class) detaches the alias. - `exports = module.exports`, `module.exports = exports`, and chains naming both reconnect it. - A write to `exports.X` is skipped only when it is itself a straight-line top-level statement and the relation before it is `detached`. - If any write to `exports`/`module`/`module.exports` or any other use of `module` (including calling a member such as `module.valueOf()`) lies outside those recognized statements, the whole timeline is treated as unordered and nothing is skipped. - A write is also kept when a later top-level rebinding assigns `module.exports` a value that is not freshly created (`module.exports = exports`), since that object may become the export. - A statement that itself rebinds the alias, or whose replacement chain contains a compound/logical assignment (`cache ||= {}`), gets no detached verdict. Conditional, function-hosted, non-fresh (`module.exports = require(...)`) and compound cases keep today's behavior. `isCommonJsModuleExports` and `isFreshAssignmentResult` moved from `javascriptSemanticProjection.ts` into the new module so there is one owner. Not included: `Object.defineProperty(exports, ...)` / `Object.assign(exports, ...)` forms, and writes through a saved old alias (e.g. `const e = exports`), which the issue lists as still missing. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript semantic projection (`collectCommonJsExport`), the producer of `commonjs-export` module links. - Contribution scope fit and linked maintainer decision: not applicable (bug fix inside existing static-analysis boundaries). - CLI and MCP/tool-catalog contract: none. Fewer false `commonjs-export` links in `analyze_javascript_application` evidence. - Provider, bridge, target-format, or platform compatibility: none; static source analysis only. - Evidence, artifact, provenance, or reconstruction contract: unchanged schema; unproven cases remain candidates rather than being dropped. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - 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 (source tests): - Tests added: `src/domain/javascript/javascriptSemanticAnalysis.commonjs_exports_alias.test.ts` (20 cases): 5 detached cases, 5 connected/untouched controls, 10 unproven-order cases that must stay candidates (including the three review findings: logical assignment in the replacement chain, a write whose own statement reconnects the alias, a call through `module`, and a later `module.exports = exports` that exports the written object). - Base reproduction: with `javascriptSemanticProjection.ts` reverted to `main`, 4 of the first 13 cases fail (the detached cases: `parse`/`status` still appear in the exported names); the 9 controls pass on both revisions. - Executed CLI reproduction of #3541 (Linux x64, Node 24.18.0, built from this commit): two directories with `module.exports = {}; exports.parse = () => ({ kind: "result", count: N });` (N = 1 / 2), `analyze-javascript-application` on each, then `compare-javascript-export-shapes` selecting `app.cjs` / `parse`: - base projection (`javascriptSemanticProjection.ts` reverted to `main`): left/right `status: "selected"`, `summary: {changed: 1, unknown: 0}`, coverage `complete-within-inputs`. - this PR: left/right `status: "missing"`, candidates only `default`, `summary: {changed: 0, unknown: 1}`, coverage `partial`. - Not run: real stdio MCP transport for this reproduction (CLI only); real-provider lanes. - [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 Windows 11, Node 26.4.0 (`npm ci --ignore-scripts`), on the final commit: - `node scripts/run-development-tests.mjs local` on all `src/domain/javascript/*.test.ts` and `src/application/javascript/*.test.ts` (80 files) — 1949 passed - `npx tsc --noEmit` — exit 0 - `npm run lint` (oxlint + module boundaries) — exit 0 - `npx knip` — exit 0 - `npx oxfmt --check src/domain/javascript` — clean Linux x64, Node 24.18.0, on the final commit: - `npm run test:focused -- tests/acceptance/applications/unboundRequires.test.ts` plus the new and `structure` test files — 50 passed - CLI reproduction of #3541 (see above) was run on the earlier revision of this branch; the later review fixes only narrow when a write is skipped and were not re-run through the CLI. - `npm run check:fast` (typecheck + lint) passed on the earlier revision. Not run: `npm run check`, `docs:check`, `check:pr`, real-provider lanes. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis only; verified on Linux x64 and Windows source tests). - Package or release metadata impact: none. - Security, privacy, process, or containment review: static parsing only; no execution of analyzed code. ## Author readiness Human owner: @maliv2 This change was prepared with AI assistance. The human owner (@maliv2) reviewed the diff and told the assistant in chat that everything was checked with no problems; the declarations below were completed on that instruction. - [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:09:44 +00:00