morluto/rea

Fork 0

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

[GH-ISSUE #3223] CommonJS var wrapper redeclarations lose semantic exports and require origins #1159

New issue

Closed

opened 2026-10-15 18:31:37 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-15 18:31:37 +00:00

Owner

Originally created by @xu91102 on GitHub (Oct 14, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/3223

On current main 58300885207133422af235e0f2b10658cf958b03, the semantic CommonJS graph loses normal exports when a .cjs source redeclares the implicit wrapper parameters with uninitialized top-level var declarations.

These declarations do not overwrite Node's wrapper parameters. Node 24.18.0 runs all three variants below successfully and returns the exported value 42. REA's public application CLI omits the commonjs-export / exposes facts for the exports and module variants. A control without the declarations correctly emits them. With var require;, the static require edge survives, but the semantic require origin/export dependency is lost, so two consumers disagree.

Reproduction after pinned npm ci and npm run build:cached:

mkdir -p /tmp/rea-wrapper-redeclaration
cat > /tmp/rea-wrapper-redeclaration/entry.cjs <<'JS'
var module, exports, require;
const value = require('./dep.cjs');
exports.value = value;
console.log(module.exports.value);
JS
printf 'module.exports = 42;\n' > /tmp/rea-wrapper-redeclaration/dep.cjs
node /tmp/rea-wrapper-redeclaration/entry.cjs
node scripts/rea.mjs analyze-javascript-application /tmp/rea-wrapper-redeclaration --format json

Node control: exit 0, stdout 42. In the REA graph, the entry should retain a semantic require relationship to dep.cjs and the value CommonJS export with its source identity/range. Instead its export facts are missing. Replacing exports.value with module.exports.value has the same omission. Removing the var declarations restores the facts. Individually tested var require;, var exports;, and var module; variants show the same source-boundary gap.

Expected scope: retain the original wrapper binding for uninitialized program-owned var redeclarations in explicit CommonJS sources. Nested locals, lexical declarations, initialized declarations, assignments, uncertain/dynamic environments and non-CommonJS sources must remain properly shadowed or unknown. No application code should be executed during analysis.

Root cause: semantic scope collection creates an ordinary local binding for these redeclarations, while isUnshadowedGlobal and semanticRequireOrigin then reject any lexical binding as a wrapper/global origin. Static require handling already retains uninitialized top-level wrapper redeclarations, which makes the disagreement visible in the same public graph. The correction belongs at the common binding/default-value owner, not in graph consumers.

Related search checked redeclarations, var-module, var-exports and var-require reports. #2989 addresses false static edges from truly shadowed require calls and explicitly keeps top-level uninitialized wrapper declarations; it does not restore these semantic export/default-origin facts.

Verified with actual Node 24.18.0 and REA's compiled public CLI on macOS arm64, locked dependencies and unchanged main. Fixtures are self-owned CommonJS code; this is static-analysis verification, not a native-provider claim.

Originally created by @xu91102 on GitHub (Oct 14, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/3223 On current main `58300885207133422af235e0f2b10658cf958b03`, the semantic CommonJS graph loses normal exports when a `.cjs` source redeclares the implicit wrapper parameters with uninitialized top-level `var` declarations. These declarations do not overwrite Node's wrapper parameters. Node 24.18.0 runs all three variants below successfully and returns the exported value 42. REA's public application CLI omits the `commonjs-export` / `exposes` facts for the `exports` and `module` variants. A control without the declarations correctly emits them. With `var require;`, the static require edge survives, but the semantic require origin/export dependency is lost, so two consumers disagree. Reproduction after pinned `npm ci` and `npm run build:cached`: ```sh mkdir -p /tmp/rea-wrapper-redeclaration cat > /tmp/rea-wrapper-redeclaration/entry.cjs <<'JS' var module, exports, require; const value = require('./dep.cjs'); exports.value = value; console.log(module.exports.value); JS printf 'module.exports = 42;\n' > /tmp/rea-wrapper-redeclaration/dep.cjs node /tmp/rea-wrapper-redeclaration/entry.cjs node scripts/rea.mjs analyze-javascript-application /tmp/rea-wrapper-redeclaration --format json ``` Node control: exit 0, stdout `42`. In the REA graph, the entry should retain a semantic require relationship to `dep.cjs` and the `value` CommonJS export with its source identity/range. Instead its export facts are missing. Replacing `exports.value` with `module.exports.value` has the same omission. Removing the `var` declarations restores the facts. Individually tested `var require;`, `var exports;`, and `var module;` variants show the same source-boundary gap. Expected scope: retain the original wrapper binding for uninitialized program-owned `var` redeclarations in explicit CommonJS sources. Nested locals, lexical declarations, initialized declarations, assignments, uncertain/dynamic environments and non-CommonJS sources must remain properly shadowed or unknown. No application code should be executed during analysis. Root cause: semantic scope collection creates an ordinary local binding for these redeclarations, while `isUnshadowedGlobal` and `semanticRequireOrigin` then reject any lexical binding as a wrapper/global origin. Static require handling already retains uninitialized top-level wrapper redeclarations, which makes the disagreement visible in the same public graph. The correction belongs at the common binding/default-value owner, not in graph consumers. Related search checked redeclarations, var-module, var-exports and var-require reports. #2989 addresses false static edges from truly shadowed require calls and explicitly keeps top-level uninitialized wrapper declarations; it does not restore these semantic export/default-origin facts. Verified with actual Node 24.18.0 and REA's compiled public CLI on macOS arm64, locked dependencies and unchanged main. Fixtures are self-owned CommonJS code; this is static-analysis verification, not a native-provider claim.

bsakaguchi

2026-10-15 18:31:37 +00:00

closed this issue
added the
bug
label

bsakaguchi commented

2026-10-15 18:31:38 +00:00

Author

Owner

@akushonkamen commented on GitHub (Oct 14, 2026):

Claiming this. Scope I am taking: exactly the correction you scoped — the common binding/default-value owner retains the original wrapper binding for uninitialized program-owned top-level var redeclarations of module/exports/require in explicit CommonJS sources, so the semantic export facts and require origins survive; nested locals, lexical declarations, initialized declarations, assignments, and non-CommonJS sources stay properly shadowed or unknown, and no graph consumer changes.

Reproduced locally on main at 500e32399 before claiming (macOS arm64, Node 24.14.0): your fixture's entry module loses the commonjs-export fact in the public graph while the control without the var declarations emits it, and Node 24.14.0 runs the fixture and prints 42. Out of scope: the static require edge (already retained per #2989's split), and any behavior change for genuinely shadowed wrappers. I will validate with unit-level red→green tests at the semantic layer plus the focused JavaScript semantic suites and the public CLI/MCP paths for both the fixture and the control.

@akushonkamen commented on GitHub (Oct 14, 2026): Claiming this. Scope I am taking: exactly the correction you scoped — the common binding/default-value owner retains the original wrapper binding for uninitialized program-owned top-level `var` redeclarations of `module`/`exports`/`require` in explicit CommonJS sources, so the semantic export facts and require origins survive; nested locals, lexical declarations, initialized declarations, assignments, and non-CommonJS sources stay properly shadowed or unknown, and no graph consumer changes. Reproduced locally on `main` at `500e32399` before claiming (macOS arm64, Node 24.14.0): your fixture's entry module loses the `commonjs-export` fact in the public graph while the control without the `var` declarations emits it, and Node 24.14.0 runs the fixture and prints 42. Out of scope: the static require edge (already retained per #2989's split), and any behavior change for genuinely shadowed wrappers. I will validate with unit-level red→green tests at the semantic layer plus the focused JavaScript semantic suites and the public CLI/MCP paths for both the fixture and the control.

bsakaguchi commented

2026-10-15 18:31:38 +00:00

Author

Owner

@akushonkamen commented on GitHub (Oct 14, 2026):

Delivery update. Fixed and validated on branch akushonkamen:fix/commonjs-wrapper-redeclarations (commit b4ed1c13608f21dea51303dd3b809c8ad73fd059, based on main at 500e32399, single commit). PR creation from this account remains repository-blocked (GraphQL CreatePullRequest permission error; REST 404, retested), so the branch and this summary stand in for the PR.

What the branch contains (both halves of the disagreement your analysis identified):

javascriptSemanticAnalysis.ts — the common binding owner: bindInnerDeclaration skips ordinary binding registration for an uninitialized var declarator that lands in the program scope (so block-level var redeclarations are covered and function-scoped ones are not) and names module/exports/require, in a source analyzed in require mode. The wrapper binding is thereby retained and every existing gate (isUnshadowedGlobal, semanticRequireOrigin) works unchanged — no graph consumer changes.
javascriptModuleResolutionMode.ts — .cjs now selects require mode like .cts. This was the second, silent half: the application layer derives the semantic resolution mode per source path, and .cjs mapped to "import", so the retained-wrapper rule could never apply to the very sources your report targets even after (1). The parser already treats .cjs as CommonJS (runtimeSourceTypeForPath); the mode now agrees.

Out of scope, unchanged: the static require edge (#2989's split), graph consumers, initialized/lexical/function-scoped shadowing (pinned by a new shadowing test), and non-CommonJS sources.

Executed validation (macOS arm64, Node 24.14.0, on the branch):

Red→green at the semantic layer: npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts — new retention test fails on the unmodified tree (1 failed | 25 passed), green after (26 passed); the new shadowing test (initialized var module = {}, let require, nested var exports) passes before and after and pins the boundary. npm run test:focused -- src/domain/javascript/javascriptModuleResolutionMode.test.ts — new .cjs mode test fails with the one-line mapping stashed, green after (2 passed).
Public CLI end-to-end (npm run compile, then node scripts/rea.mjs analyze-javascript-application): your fixture now emits the entry module's commonjs-export fact and the require edge with specifier ./dep.cjs; the control without the var declarations is unchanged. One verification trap worth noting: identical fixture content replays the retained evidence snapshot (identical evidence_id), so I re-verified with a fresh run and confirmed the pre-fix and post-fix graphs differ.
Focused suites: npx vitest run --project domain src/domain/javascript — 65 files, 1530 passed; --project services src/application/javascript — 11 files, 291 passed; --project adapters src/javascript — 4 files, 43 passed; --project boundary tests/boundary/filesystem/javascript-applications — 17 files, 184 passed; acceptance javascriptCommonJsDirectoryLookup + javascriptEndpointPayload + javascriptConfigurationReadsCli + applicationWorkflowCli — 41 passed; JS MCP boundary files — 5 passed/2 skipped; CLI boundary javascriptApplicationJsonOutput + javascriptApplicationProgress — 17 passed.
Gates: npm run format:check pass; npx tsc --noEmit clean; npm run lint 0 errors (module boundaries pass); npx knip clean; npm run jscpd 3.19% < 3.5%; npm run docs:check pass.

Not claimed: nothing runtime-related — this is static-analysis fidelity only, no code execution during analysis, matching your expected scope.

@akushonkamen commented on GitHub (Oct 14, 2026): **Delivery update.** Fixed and validated on branch `akushonkamen:fix/commonjs-wrapper-redeclarations` (commit `b4ed1c13608f21dea51303dd3b809c8ad73fd059`, based on `main` at `500e32399`, single commit). PR creation from this account remains repository-blocked (GraphQL `CreatePullRequest` permission error; REST 404, retested), so the branch and this summary stand in for the PR. **What the branch contains** (both halves of the disagreement your analysis identified): 1. `javascriptSemanticAnalysis.ts` — the common binding owner: `bindInnerDeclaration` skips ordinary binding registration for an uninitialized `var` declarator that lands in the program scope (so block-level `var` redeclarations are covered and function-scoped ones are not) and names `module`/`exports`/`require`, in a source analyzed in require mode. The wrapper binding is thereby retained and every existing gate (`isUnshadowedGlobal`, `semanticRequireOrigin`) works unchanged — no graph consumer changes. 2. `javascriptModuleResolutionMode.ts` — `.cjs` now selects require mode like `.cts`. This was the second, silent half: the application layer derives the semantic resolution mode per source path, and `.cjs` mapped to `"import"`, so the retained-wrapper rule could never apply to the very sources your report targets even after (1). The parser already treats `.cjs` as CommonJS (`runtimeSourceTypeForPath`); the mode now agrees. Out of scope, unchanged: the static require edge (#2989's split), graph consumers, initialized/lexical/function-scoped shadowing (pinned by a new shadowing test), and non-CommonJS sources. **Executed validation** (macOS arm64, Node 24.14.0, on the branch): - Red→green at the semantic layer: `npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts` — new retention test fails on the unmodified tree (1 failed | 25 passed), green after (26 passed); the new shadowing test (initialized `var module = {}`, `let require`, nested `var exports`) passes before and after and pins the boundary. `npm run test:focused -- src/domain/javascript/javascriptModuleResolutionMode.test.ts` — new `.cjs` mode test fails with the one-line mapping stashed, green after (2 passed). - Public CLI end-to-end (`npm run compile`, then `node scripts/rea.mjs analyze-javascript-application`): your fixture now emits the entry module's `commonjs-export` fact and the require edge with specifier `./dep.cjs`; the control without the `var` declarations is unchanged. One verification trap worth noting: identical fixture content replays the retained evidence snapshot (identical `evidence_id`), so I re-verified with a fresh run and confirmed the pre-fix and post-fix graphs differ. - Focused suites: `npx vitest run --project domain src/domain/javascript` — 65 files, **1530 passed**; `--project services src/application/javascript` — 11 files, **291 passed**; `--project adapters src/javascript` — 4 files, **43 passed**; `--project boundary tests/boundary/filesystem/javascript-applications` — 17 files, **184 passed**; acceptance `javascriptCommonJsDirectoryLookup` + `javascriptEndpointPayload` + `javascriptConfigurationReadsCli` + `applicationWorkflowCli` — **41 passed**; JS MCP boundary files — 5 passed/2 skipped; CLI boundary `javascriptApplicationJsonOutput` + `javascriptApplicationProgress` — **17 passed**. - Gates: `npm run format:check` pass; `npx tsc --noEmit` clean; `npm run lint` 0 errors (module boundaries pass); `npx knip` clean; `npm run jscpd` 3.19% < 3.5%; `npm run docs:check` pass. **Not claimed:** nothing runtime-related — this is static-analysis fidelity only, no code execution during analysis, matching your expected scope.

bsakaguchi commented

2026-10-15 18:31:38 +00:00

Author

Owner

@akushonkamen commented on GitHub (Oct 14, 2026):

Rebase + fresh validation (this session). PR creation retried just now and is still repository-blocked for this account (GraphQL CreatePullRequest permission error; REST 404) — the PR could not be opened from here. main advanced 25 commits since my delivery summary; the branch rebased cleanly (single commit 6afc377dd, patch-id-identical to the announced b4ed1c136, now based on main at 748440160) and was force-pushed to akushonkamen:fix/commonjs-wrapper-redeclarations. A maintainer can open the PR from that head (Closes #3223).

Every gate re-run on the rebased branch this session:

Red first, re-verified on the unmodified base 748440160 with this branch's test files: npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts = 1 failed | 25 passed (26); npm run test:focused -- src/domain/javascript/javascriptModuleResolutionMode.test.ts = 1 failed | 1 passed (2). Green on the branch: 26 passed and 2 passed.
npx vitest run --project domain src/domain/javascript — 67 files, 1563 passed (upstream additions grew this from the previously recorded 65/1530); --project services src/application/javascript — 11 files, 291 passed; --project adapters src/javascript — 4 files, 43 passed.
npx vitest run --project boundary tests/boundary/filesystem/javascript-applications — 17 files, 190 passed, 1 failed. That one failure (javascriptFileUrlPaths.test.ts, "resolves a decoded literal file URL at the filesystem boundary") is pre-existing on unmodified main: verified by detached checkout, it fails identically there (1 failed | 25 passed) and is unrelated to this branch.
Acceptance (javascriptCommonJsDirectoryLookup + javascriptEndpointPayload + javascriptConfigurationReadsCli + applicationWorkflowCli) — 41 passed; JS MCP boundary (all six tests/boundary/mcp/javascript*.test.ts) — 9 passed, 2 skipped (pre-existing javascriptRecoveryMcp skips); CLI boundary (javascriptApplicationJsonOutput + javascriptApplicationProgress) — 17 passed.
End-to-end re-run on the recompiled CLI (npm run compile + node scripts/rea.mjs analyze-javascript-application, fresh fixture content to avoid retained-evidence replay): your fixture now emits the entry commonjs-export fact and the require edge with specifier ./dep.cjs; the control without the var declarations is unchanged.
Gates: npm run format:check pass; npx tsc --noEmit clean; npm run lint 0 errors + module boundaries pass (1863 files); npx knip clean; npm run jscpd 3.20% < 3.5%; npm run docs:check 5 passed.

Unchanged and not claimed: static-analysis fidelity only — no code execution during analysis, no runtime claim.

@akushonkamen commented on GitHub (Oct 14, 2026): **Rebase + fresh validation (this session).** PR creation retried just now and is still repository-blocked for this account (GraphQL `CreatePullRequest` permission error; REST 404) — the PR could not be opened from here. `main` advanced 25 commits since my delivery summary; the branch rebased cleanly (single commit `6afc377dd`, patch-id-identical to the announced `b4ed1c136`, now based on `main` at `748440160`) and was force-pushed to `akushonkamen:fix/commonjs-wrapper-redeclarations`. A maintainer can open the PR from that head (Closes #3223). Every gate re-run on the rebased branch this session: - Red first, re-verified on the unmodified base `748440160` with this branch's test files: `npm run test:focused -- src/domain/javascript/javascriptSemanticAnalysis.require_links.test.ts` = **1 failed | 25 passed (26)**; `npm run test:focused -- src/domain/javascript/javascriptModuleResolutionMode.test.ts` = **1 failed | 1 passed (2)**. Green on the branch: 26 passed and 2 passed. - `npx vitest run --project domain src/domain/javascript` — 67 files, **1563 passed** (upstream additions grew this from the previously recorded 65/1530); `--project services src/application/javascript` — 11 files, **291 passed**; `--project adapters src/javascript` — 4 files, **43 passed**. - `npx vitest run --project boundary tests/boundary/filesystem/javascript-applications` — 17 files, **190 passed, 1 failed**. That one failure (`javascriptFileUrlPaths.test.ts`, "resolves a decoded literal file URL at the filesystem boundary") is **pre-existing on unmodified `main`**: verified by detached checkout, it fails identically there (1 failed | 25 passed) and is unrelated to this branch. - Acceptance (`javascriptCommonJsDirectoryLookup` + `javascriptEndpointPayload` + `javascriptConfigurationReadsCli` + `applicationWorkflowCli`) — **41 passed**; JS MCP boundary (all six `tests/boundary/mcp/javascript*.test.ts`) — **9 passed, 2 skipped** (pre-existing `javascriptRecoveryMcp` skips); CLI boundary (`javascriptApplicationJsonOutput` + `javascriptApplicationProgress`) — **17 passed**. - End-to-end re-run on the recompiled CLI (`npm run compile` + `node scripts/rea.mjs analyze-javascript-application`, fresh fixture content to avoid retained-evidence replay): your fixture now emits the entry `commonjs-export` fact and the require edge with specifier `./dep.cjs`; the control without the `var` declarations is unchanged. - Gates: `npm run format:check` pass; `npx tsc --noEmit` clean; `npm run lint` 0 errors + module boundaries pass (1863 files); `npx knip` clean; `npm run jscpd` **3.20%** < 3.5%; `npm run docs:check` 5 passed. Unchanged and not claimed: static-analysis fidelity only — no code execution during analysis, no runtime claim.

bsakaguchi referenced this issue

2026-10-17 21:09:08 +00:00

[PR #1159] [MERGED] fix(apple): report unusable dylib-resolution roots as invalid input #2693

bsakaguchi referenced this issue

2026-10-17 21:09:11 +00:00

[PR #1161] [MERGED] fix(artifacts): report wrong target kinds as unsupported targets #2696