morluto/rea

Fork 0

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

[PR #3552] [CLOSED] fix(javascript): retain exports through renamed module parameters #5818

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/webpack-module-export-bindings

📝 Commits (3)

4aee6ef fix(javascript): retain exports through renamed module parameters
75de3b6 fix(javascript): retain exports through defaulted factory parameters
20643d4 test(javascript): pin positional factory export ownership

📊 Changes

3 files changed (+216 additions, -31 deletions)

View changed files

📝 src/domain/javascript/javascriptAstFingerprint.ts (+42 -31)
📝 src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts (+101 -0)
➕ tests/acceptance/applications/javascriptBundlerExportsCli.test.ts (+73 -0)

📄 Description

Summary

Fixes #3551.

Recover CommonJS exports through a Webpack/Rspack factory's first parameter even when its identifier has been minified. A factory that writes e.exports = { parse: value => value } now retains parse just like the equivalent module.exports spelling. The same change excludes nested or block-local variables that happen to be called module but do not own the factory's exports.

This corrects collectJavaScriptExports, with domain regressions and a real CLI/stdio MCP acceptance test.

Problem and expected behavior

The smallest trigger is a recognized module factory whose first parameter has a shortened name:

globalThis.webpackChunkApp.push([[1], {
  17(e, t) {
    e.exports = { parse: value => value };
  }
}]);

Released REA 6.5.0 recovers module 17 but gives it exports: []. Changing the parameter declarations to (module, exports) and the assignment to module.exports produces ["parse"]. Both public CLI commands exit 0 and attach observed/exact/complete factory evidence to the result. The expected invariant is that renaming a factory parameter preserves the export names recovered from the same static property operations.

The collector already used lexical binding identity for the second factory parameter. The first receiver was checked through the string module in two places, once for assignments and once for declaration calls. That inconsistency both missed valid renamed bindings and admitted unrelated same-spelled receivers. The correct question is which factory binding owns the receiver, rather than whether its name is familiar.

Change and scope

The production change stays in src/domain/javascript/javascriptAstFingerprint.ts:

Resolve the first two runtime factory parameters to semantic binding IDs once per collection, unwrapping an AssignmentPattern to its left-hand identifier when the parameter has a default value.
Match candidate receiver identifiers against those IDs at their lexical locations.
Use one receiver-to-property-offset interpretation for both assignment collection and declaration-call collection. The second parameter addresses exports directly; the first parameter addresses them through its exports property.
Keep the existing literal-name behavior for standalone helper calls without semantic binding state. For a factory with supplied binding state, an absent or unresolved parameter does not fall back to a guessed global name.

This removes the duplicated literal module checks at the owning boundary. javascriptStaticAnalysisBundler.ts already supplies the semantic state and consumes the returned export names, so no downstream adapter or graph-builder workaround is needed.

The correction covers the existing static declaration forms:

Factory body, with first parameter `e`	Correct export names	Regression coverage
`e.exports = { parse: value => value }`	`parse`	Domain and CLI/MCP
`e.exports.parse = value => value`	`parse`	Domain
`e["exports"][""] = 1`	Empty-string export name	Domain
`e.exports = value => value`	`default`	Domain
`Object.defineProperty(e.exports, "parse", { value: 1 })`	`parse`	Domain and CLI/MCP
`r.d(e.exports, { parse: () => 1 })`	`parse`	Domain and CLI/MCP
Nested `function local(e) { e.exports.fake = 1 }`	No export from the shadow	Domain and CLI/MCP
Block-local `e` or unrelated nested `module`	No export from the unrelated binding	Domain

Factories declaring only their first parameter also retain their export object. Defaulted parameters such as (module = {}, exports = {}) and their renamed equivalents retain the same export names. The collector resolves the declared identifier before comparing lexical binding IDs, so defaulted parameters in nested functions and block-local shadows still cannot contribute exports to the outer factory. Existing alias-rebinding, deeper-write, and property-key tests remain in the focused suite.

Contribution scope: an ordinary correctness fix within existing inert JavaScript reconstruction; no new capability or authority boundary requiring a maintainer decision.

Contract and boundary impact

Semantic owner and earliest changed stage: the bundler factory export collector, before recovered module facts are projected into application observations.
Contribution scope fit and linked maintainer decision: existing static-analysis behavior; no new authority or side-effect boundary requiring a scope decision.
CLI and MCP/tool-catalog contract: no argument, output-schema, tool-name, or catalog change. Existing exports arrays contain the corrected names. Both adapters use the same workflow.
Provider, bridge, target-format, or platform compatibility: existing JavaScript bundler recovery path. The Windows test environment is specified below; arbitrary third-party bundle equivalence is not established.
Evidence, artifact, provenance, or reconstruction contract: corrected source-derived export facts retain their existing source location, artifact identity, and observation envelope. No postprocessing invents export names or suppresses unrelated unknowns.
Target selection, authority, upstream behavior, and cleanup: the selected fixture directory is analyzed as inert source. The new acceptance test uses existing CLI/MCP fixtures and temporary-directory cleanup.
Generated metadata, package, or installation impact: none required; no package, lockfile, schema, tracked generated artifact, or installation change is included.

Expected effects of the analysis operation:

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 acceptance harness launches REA's CLI and MCP server processes to verify these interfaces; it does not launch or execute the analyzed target.

Evidence and regression coverage

Executed base evidence: the released 6.5.0 CLI returns ["parse"] for the original fixture and [] for the renamed fixture, with both commands exiting 0. A separate Node oracle on those source-owned fixtures returns ["parse"] for both. The original source analysis produced the same difference.

Regression sensitivity: the new ModuleIndex cases detect missing renamed exports and incorrect ownership of shadowed receivers on base ca1b2f8608c52ed721d3576ba5eeb1d66717b08c. With the producer fixed, these cases and the focused suite pass.

Public CLI/MCP behavior: tests/acceptance/applications/javascriptBundlerExportsCli.test.ts creates a source-owned bundle with original and renamed factories whose first two parameters have defaults, plus a first-parameter-only control without defaults. The renamed factory exercises multiple declaration forms and excludes a nested function with its own defaulted parameters. The result asserted through the actual CLI is:

{
  "17": ["parse"],
  "18": ["declared", "extra", "named", "parse"],
  "19": ["parse"]
}

The test asserts complete graph coverage and observed/exact/complete module evidence, repeats analysis through real stdio MCP, validates its output and identical Evidence ID, then pings the client. A top-level throw new Error("This analysis fixture must not execute") makes accidental bundle evaluation fail.

Domain tests cover lexical shadows and property keys; existing Webpack/Rspack and esbuild/Vite tests provide adjacent adapter coverage.

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

Final validation used pinned Node 24.18.0 / npm 11.16.0 on Windows x64:

npm run check — passed all six static tasks, including type checking, formatting, lint, metadata checking and unused-code checks; module boundaries and architecture guards passed.
The focused command below — 65 tests passed in 5 files, including eight default-parameter regressions and the real CLI/MCP acceptance test. The public acceptance test completed in 13.38 seconds.
npm run verify:test-discovery — 1285 test files discovered exactly once.
git diff --check — passed.

npm run test:focused -- \
  src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts \
  src/domain/javascript/javascriptStaticAnalysis.member_path_depth.test.ts \
  src/domain/javascript/javascriptAstFingerprint.test.ts \
  tests/acceptance/applications/javascriptBundlerExportsCli.test.ts \
  tests/boundary/filesystem/javascript-applications/javascriptBundlerAdapters.test.ts

The five paths above were executed together. The failing-base regression and released CLI comparison establish the before-fix behavior.

Compatibility, safety, and release

Breaking changes or migration steps: none. Existing callers receive corrected export names through the same fields.
Real Hopper/Ghidra, browser, or OS coverage: this is static JavaScript analysis on Windows x64. The JavaScript runtime was built from this branch. The public workflow used verified Windows native artifacts from the released 6.5.0 package; native code was not rebuilt. The full repository suite and unrelated real-provider lanes were not run.
Package or release metadata impact: no dependency, package metadata, generated catalog, or installation change.
Security, privacy, process, or containment review: source text remains inert. Receiver ownership uses lexical binding identity, and the new target fixture explicitly fails if evaluated. No new process-control or network behavior is added to analysis.

#2171 and #2234 addressed lexical identity for the second exports parameter. This change completes the corresponding first-parameter .exports path and removes its duplicated name checks; it does not replace the second-parameter work.

Review follow-up

The single-parameter contract is now explicit in two ModuleIndex cases. For 17(exports), exports.parse = 1 contributes no export, while exports.exports.parse = 1 contributes parse. The first runtime argument remains the module slot regardless of its identifier; recognizing the familiar spelling as a second-argument exports object would undermine that positional ownership rule.

A comment beside receiver matching records why the bundler's semantic-binding mode intentionally differs from syntax-only helper calls. The bundler supplies binding state, so an unresolved factory binding stays unmatched and cannot admit a same-spelled shadow. Helper calls without that state retain conventional literal-name matching. This follow-up documents the existing implementation without changing its runtime behavior.

Follow-up validation on Windows x64 with Node 24.18.0 / npm 11.16.0: the ModuleIndex, member-path-depth, and AST fingerprint source tests passed 64/64 across three files, including both new contract cases. npm run check:fast, formatting checks for both touched files, and git diff --check passed. The previously recorded CLI/MCP acceptance coverage remains the integration evidence; it was not rerun for this comment and test addition.

Author readiness

Human owner: @dakjdakd

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/3552 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/webpack-module-export-bindings` --- ### 📝 Commits (3) - [`4aee6ef`](https://github.com/morluto/rea/commit/4aee6ef1f5bb1932247b806548283843bf8eb98d) fix(javascript): retain exports through renamed module parameters - [`75de3b6`](https://github.com/morluto/rea/commit/75de3b66f7d9e5b10d8e56a963fb89032627ab13) fix(javascript): retain exports through defaulted factory parameters - [`20643d4`](https://github.com/morluto/rea/commit/20643d4a47b0562aa794f0a15903630c62912c80) test(javascript): pin positional factory export ownership ### 📊 Changes **3 files changed** (+216 additions, -31 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptAstFingerprint.ts` (+42 -31) 📝 `src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts` (+101 -0) ➕ `tests/acceptance/applications/javascriptBundlerExportsCli.test.ts` (+73 -0) </details> ### 📄 Description ## Summary Fixes #3551. Recover CommonJS exports through a Webpack/Rspack factory's first parameter even when its identifier has been minified. A factory that writes `e.exports = { parse: value => value }` now retains `parse` just like the equivalent `module.exports` spelling. The same change excludes nested or block-local variables that happen to be called `module` but do not own the factory's exports. This corrects `collectJavaScriptExports`, with domain regressions and a real CLI/stdio MCP acceptance test. ## Problem and expected behavior The smallest trigger is a recognized module factory whose first parameter has a shortened name: ```js globalThis.webpackChunkApp.push([[1], { 17(e, t) { e.exports = { parse: value => value }; } }]); ``` Released REA 6.5.0 recovers module `17` but gives it `exports: []`. Changing the parameter declarations to `(module, exports)` and the assignment to `module.exports` produces `["parse"]`. Both public CLI commands exit 0 and attach observed/exact/complete factory evidence to the result. The expected invariant is that renaming a factory parameter preserves the export names recovered from the same static property operations. The collector already used lexical binding identity for the second factory parameter. The first receiver was checked through the string `module` in two places, once for assignments and once for declaration calls. That inconsistency both missed valid renamed bindings and admitted unrelated same-spelled receivers. The correct question is which factory binding owns the receiver, rather than whether its name is familiar. ## Change and scope The production change stays in `src/domain/javascript/javascriptAstFingerprint.ts`: 1. Resolve the first two runtime factory parameters to semantic binding IDs once per collection, unwrapping an AssignmentPattern to its left-hand identifier when the parameter has a default value. 2. Match candidate receiver identifiers against those IDs at their lexical locations. 3. Use one receiver-to-property-offset interpretation for both assignment collection and declaration-call collection. The second parameter addresses exports directly; the first parameter addresses them through its `exports` property. 4. Keep the existing literal-name behavior for standalone helper calls without semantic binding state. For a factory with supplied binding state, an absent or unresolved parameter does not fall back to a guessed global name. This removes the duplicated literal `module` checks at the owning boundary. `javascriptStaticAnalysisBundler.ts` already supplies the semantic state and consumes the returned export names, so no downstream adapter or graph-builder workaround is needed. The correction covers the existing static declaration forms: | Factory body, with first parameter `e` | Correct export names | Regression coverage | | --- | --- | --- | | `e.exports = { parse: value => value }` | `parse` | Domain and CLI/MCP | | `e.exports.parse = value => value` | `parse` | Domain | | `e["exports"][""] = 1` | Empty-string export name | Domain | | `e.exports = value => value` | `default` | Domain | | `Object.defineProperty(e.exports, "parse", { value: 1 })` | `parse` | Domain and CLI/MCP | | `r.d(e.exports, { parse: () => 1 })` | `parse` | Domain and CLI/MCP | | Nested `function local(e) { e.exports.fake = 1 }` | No export from the shadow | Domain and CLI/MCP | | Block-local `e` or unrelated nested `module` | No export from the unrelated binding | Domain | Factories declaring only their first parameter also retain their export object. Defaulted parameters such as `(module = {}, exports = {})` and their renamed equivalents retain the same export names. The collector resolves the declared identifier before comparing lexical binding IDs, so defaulted parameters in nested functions and block-local shadows still cannot contribute exports to the outer factory. Existing alias-rebinding, deeper-write, and property-key tests remain in the focused suite. Contribution scope: an ordinary correctness fix within existing inert JavaScript reconstruction; no new capability or authority boundary requiring a maintainer decision. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** the bundler factory export collector, before recovered module facts are projected into application observations. - **Contribution scope fit and linked maintainer decision:** existing static-analysis behavior; no new authority or side-effect boundary requiring a scope decision. - **CLI and MCP/tool-catalog contract:** no argument, output-schema, tool-name, or catalog change. Existing `exports` arrays contain the corrected names. Both adapters use the same workflow. - **Provider, bridge, target-format, or platform compatibility:** existing JavaScript bundler recovery path. The Windows test environment is specified below; arbitrary third-party bundle equivalence is not established. - **Evidence, artifact, provenance, or reconstruction contract:** corrected source-derived export facts retain their existing source location, artifact identity, and observation envelope. No postprocessing invents export names or suppresses unrelated unknowns. - **Target selection, authority, upstream behavior, and cleanup:** the selected fixture directory is analyzed as inert source. The new acceptance test uses existing CLI/MCP fixtures and temporary-directory cleanup. - **Generated metadata, package, or installation impact:** none required; no package, lockfile, schema, tracked generated artifact, or installation change is included. Expected effects of the analysis operation: - [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 acceptance harness launches REA's CLI and MCP server processes to verify these interfaces; it does not launch or execute the analyzed target. ## Evidence and regression coverage **Executed base evidence:** the released 6.5.0 CLI returns `["parse"]` for the original fixture and `[]` for the renamed fixture, with both commands exiting 0. A separate Node oracle on those source-owned fixtures returns `["parse"]` for both. The original source analysis produced the same difference. **Regression sensitivity:** the new ModuleIndex cases detect missing renamed exports and incorrect ownership of shadowed receivers on base `ca1b2f8608c52ed721d3576ba5eeb1d66717b08c`. With the producer fixed, these cases and the focused suite pass. **Public CLI/MCP behavior:** `tests/acceptance/applications/javascriptBundlerExportsCli.test.ts` creates a source-owned bundle with original and renamed factories whose first two parameters have defaults, plus a first-parameter-only control without defaults. The renamed factory exercises multiple declaration forms and excludes a nested function with its own defaulted parameters. The result asserted through the actual CLI is: ```json { "17": ["parse"], "18": ["declared", "extra", "named", "parse"], "19": ["parse"] } ``` The test asserts complete graph coverage and observed/exact/complete module evidence, repeats analysis through real stdio MCP, validates its output and identical Evidence ID, then pings the client. A top-level `throw new Error("This analysis fixture must not execute")` makes accidental bundle evaluation fail. Domain tests cover lexical shadows and property keys; existing Webpack/Rspack and esbuild/Vite tests provide adjacent adapter coverage. 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 Final validation used pinned **Node 24.18.0 / npm 11.16.0** on Windows x64: - `npm run check` — passed all six static tasks, including type checking, formatting, lint, metadata checking and unused-code checks; module boundaries and architecture guards passed. - The focused command below — **65 tests passed in 5 files**, including eight default-parameter regressions and the real CLI/MCP acceptance test. The public acceptance test completed in 13.38 seconds. - `npm run verify:test-discovery` — **1285 test files discovered exactly once**. - `git diff --check` — passed. ```sh npm run test:focused -- \ src/domain/javascript/javascriptStaticAnalysisModuleIndex.test.ts \ src/domain/javascript/javascriptStaticAnalysis.member_path_depth.test.ts \ src/domain/javascript/javascriptAstFingerprint.test.ts \ tests/acceptance/applications/javascriptBundlerExportsCli.test.ts \ tests/boundary/filesystem/javascript-applications/javascriptBundlerAdapters.test.ts ``` The five paths above were executed together. The failing-base regression and released CLI comparison establish the before-fix behavior. ## Compatibility, safety, and release - **Breaking changes or migration steps:** none. Existing callers receive corrected export names through the same fields. - **Real Hopper/Ghidra, browser, or OS coverage:** this is static JavaScript analysis on Windows x64. The JavaScript runtime was built from this branch. The public workflow used verified Windows native artifacts from the released 6.5.0 package; native code was not rebuilt. The full repository suite and unrelated real-provider lanes were not run. - **Package or release metadata impact:** no dependency, package metadata, generated catalog, or installation change. - **Security, privacy, process, or containment review:** source text remains inert. Receiver ownership uses lexical binding identity, and the new target fixture explicitly fails if evaluated. No new process-control or network behavior is added to analysis. #2171 and #2234 addressed lexical identity for the second `exports` parameter. This change completes the corresponding first-parameter `.exports` path and removes its duplicated name checks; it does not replace the second-parameter work. ## Review follow-up The single-parameter contract is now explicit in two ModuleIndex cases. For `17(exports)`, `exports.parse = 1` contributes no export, while `exports.exports.parse = 1` contributes `parse`. The first runtime argument remains the module slot regardless of its identifier; recognizing the familiar spelling as a second-argument exports object would undermine that positional ownership rule. A comment beside receiver matching records why the bundler's semantic-binding mode intentionally differs from syntax-only helper calls. The bundler supplies binding state, so an unresolved factory binding stays unmatched and cannot admit a same-spelled shadow. Helper calls without that state retain conventional literal-name matching. This follow-up documents the existing implementation without changing its runtime behavior. Follow-up validation on Windows x64 with Node 24.18.0 / npm 11.16.0: the ModuleIndex, member-path-depth, and AST fingerprint source tests passed **64/64 across three files**, including both new contract cases. `npm run check:fast`, formatting checks for both touched files, and `git diff --check` passed. The previously recorded CLI/MCP acceptance coverage remains the integration evidence; it was not rerun for this comment and test addition. ## Author readiness Human owner: @dakjdakd - [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

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

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #5818] [MERGED] feat(sqlite): make worker deadlines adjustable with recovery guidance #7080

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5818

No description provided.

Rows
Columns