[PR #4355] fix(javascript): preserve loader getters across inactive defaults #6197

New issue

Open

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

bsakaguchi commented

2026-10-31 01:12:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4355
Author: @To3akaRin
Created: 10/16/2026
Status: 🔄 Open

Base: main ← Head: codex/rea-oct16-javascript

📝 Commits (10+)

bdd57b4 fix(javascript): invalidate Webpack getters through loader aliases
641f662 fix(javascript): exclude only proven inactive loader defaults
3a0fd8a perf(javascript): reuse loader alias reachability across writes
00002e1 Merge commit '974487f00f' into codex/rea-oct16-javascript
04887b2 test(snapshot): preserve Evidence-scoped native workflow unknowns
43f5a11 Merge commit 'e8da7e1edd' into codex/rea-oct16-javascript
b88813c test(process): assert owned snapshot cancellation outcomes
4672f55 test(process): preserve snapshot race causes and owned cleanup
c3da39f test(cli): scope runtime cleanup provider injection
2e6978c test(artifacts): use valid ASAR bytes for signature control

📊 Changes

6 files changed (+387 additions, -3 deletions)

View changed files

📝 docs/javascript-artifact-reconstruction.md (+8 -2)
➕ src/domain/javascript/electronBundledGetter.fixture.ts (+21 -0)
➕ src/domain/javascript/electronBundledGetterMutations.test.ts (+106 -0)
➕ src/domain/javascript/javascriptBootstrapReferenceSources.ts (+133 -0)
📝 src/domain/javascript/javascriptBundledRequireOrigin.ts (+3 -1)
➕ tests/acceptance/applications/electronLoaderGetterAliasesCli.test.ts (+116 -0)

📄 Description

Summary

Preserve the canonical Webpack loader getter when a destructuring default cannot run. For example, const { api: alias = load } = { api: {} }; alias.n = replacement currently discards a valid Electron BrowserWindow observation even though alias cannot be the loader.

This focuses the remaining work requested in the maintainer review. The original #4229 alias-write handling is already on main.

Change and scope

Use main's existing read-position-selected loader traversal and resource limit. A bootstrap-specific reference-source filter calls the shared inactive-source predicate only when self-contained literal reads establish that own-slot selection is a valid property-read proof. Missing own slots do not exclude inherited values such as constructor, explicit prototype writes, or sparse-array inheritance. Own data overrides inherited possibilities; undefined, mutable/unknown sources and uncertain iteration retain possible defaults.

Removed the superseded alias graph/cache and its broad alias tests. The remaining controls cover default selection and inheritance, including explicit undefined slots and an escaped array prototype. Original require("electron") evidence remains available when getter identity is rejected.

Contract and boundary impact

Owner: bootstrap reference selection at the existing canonical loader-getter proof. Shared own-slot inventory, iteration analysis, binding selection and identity-write ownership remain unchanged.
Existing static-analysis scope; no new provider or target execution. CLI/MCP schemas, configuration, dependencies, package/install behavior and release version are unchanged.
Updated the reconstruction guide. No environment-variable, API or deployment documentation change is needed.

Validation

Against upstream bfdde7ee on macOS arm64, Node 24.22.1 / npm 11.16.0:

Before the filter: 5 failed / 16 passed in the 21-case source regression table. All five failures were inactive defaults losing valid window identity.
Final focused lane: 3 files / 94 tests passed, including 22 default/inheritance controls, 70 existing bundled-require controls, and actual compiled CLI plus stdio MCP. Each public surface parses 13 inert authored fixtures, retains exactly the expected 5 window sources, and preserves every fixture's Electron require observation.
npm run docs:check: passed, including 9 README consistency tests.
Architecture guards, production reachability and typed lint passed; test discovery found all 2,639 test files exactly once. git diff --check passed.
npm run check: typecheck, lint/module boundaries, formatting and metadata passed; Knip failed on two pre-existing unused exports (staticFileUrlPath and ObjcClassImportBindingCoverage). The same two findings reproduce on an independent upstream bfdde7ee source snapshot. No unrelated cleanup or check relaxation is included.

The fixtures are parsed, not executed. No full repository test suite, real Electron application, installed package, native provider, or Linux/Windows runtime was verified locally. Remote CI is reported below.

Current-head remote CI and review

CI run 40438429335 tested head ef0020ab in merge 622840d1 against upstream ded6542d.

Static checks, package build, documentation, Linux native package baseline, real JavaScript recovery, PR title and author readiness passed.
All 94 PR-focused tests passed remotely: 22 default/inheritance regressions, 70 bundled-require controls, and both compiled CLI/stdio MCP acceptance cases.
Source tests and the boundary shards are not green. The collected report contains 13 failures across appleApplication (8), providerBinding (1), javascriptApplicationEmptyLiterals (1), ciPrHealth (1), and safeOutputTreeCloseFailures (2). Several other shards were cancelled, so this run does not establish a full-suite pass.
Built an independent source snapshot of the exact upstream base ded6542d with the same locked dependencies and Node 24.22.1, then ran those five files on macOS arm64: 13 failed / 81 passed. The failed test names match all 13 remote failures exactly. These baseline failures are not changed here; no assertions, validation gates or required statuses were relaxed.
Greptile's review of ef0020ab is 5/5, with no new blocking issue. No unresolved inline review threads remain at this check.

Author readiness

Human owner: @To3akaRin

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.

_{🔄 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/4355 **Author:** [@To3akaRin](https://github.com/To3akaRin) **Created:** 10/16/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `codex/rea-oct16-javascript` --- ### 📝 Commits (10+) - [`bdd57b4`](https://github.com/morluto/rea/commit/bdd57b4f298936d2061dffb694a56a2b17d41607) fix(javascript): invalidate Webpack getters through loader aliases - [`641f662`](https://github.com/morluto/rea/commit/641f66255e143b20bf91923b4a1b94201e74325d) fix(javascript): exclude only proven inactive loader defaults - [`3a0fd8a`](https://github.com/morluto/rea/commit/3a0fd8ad6be4af1e5baa730d395fbc9f9c3abd81) perf(javascript): reuse loader alias reachability across writes - [`00002e1`](https://github.com/morluto/rea/commit/00002e188beb93e1d9d5c39e306c706d5c8fd1a6) Merge commit '974487f00f2d68912c7c2c9e033a2760a343a82e' into codex/rea-oct16-javascript - [`04887b2`](https://github.com/morluto/rea/commit/04887b28360e50c9e6e4bced25971a1a9c1c5d65) test(snapshot): preserve Evidence-scoped native workflow unknowns - [`43f5a11`](https://github.com/morluto/rea/commit/43f5a119d1ba5703acc69abd9a3f57d2e9d0f2ca) Merge commit 'e8da7e1edd075feec0e6beedec93d5645fed8125' into codex/rea-oct16-javascript - [`b88813c`](https://github.com/morluto/rea/commit/b88813ca3c84a38519f6a14cf11efd08e64009d9) test(process): assert owned snapshot cancellation outcomes - [`4672f55`](https://github.com/morluto/rea/commit/4672f55cd4e8ba50fdf9903bdb8513f9373644b8) test(process): preserve snapshot race causes and owned cleanup - [`c3da39f`](https://github.com/morluto/rea/commit/c3da39ffcb14a804c37ca48f66f6e1ec49b0f046) test(cli): scope runtime cleanup provider injection - [`2e6978c`](https://github.com/morluto/rea/commit/2e6978c10649cb92bad4ae93d7e20c50710db9c5) test(artifacts): use valid ASAR bytes for signature control ### 📊 Changes **6 files changed** (+387 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `docs/javascript-artifact-reconstruction.md` (+8 -2) ➕ `src/domain/javascript/electronBundledGetter.fixture.ts` (+21 -0) ➕ `src/domain/javascript/electronBundledGetterMutations.test.ts` (+106 -0) ➕ `src/domain/javascript/javascriptBootstrapReferenceSources.ts` (+133 -0) 📝 `src/domain/javascript/javascriptBundledRequireOrigin.ts` (+3 -1) ➕ `tests/acceptance/applications/electronLoaderGetterAliasesCli.test.ts` (+116 -0) </details> ### 📄 Description ## Summary Preserve the canonical Webpack loader getter when a destructuring default cannot run. For example, `const { api: alias = load } = { api: {} }; alias.n = replacement` currently discards a valid Electron `BrowserWindow` observation even though `alias` cannot be the loader. This focuses the remaining work requested in [the maintainer review](https://github.com/morluto/rea/pull/4355#issuecomment-6428043508). The original #4229 alias-write handling is already on main. ## Change and scope Use main's existing read-position-selected loader traversal and resource limit. A bootstrap-specific reference-source filter calls the shared inactive-source predicate only when self-contained literal reads establish that own-slot selection is a valid property-read proof. Missing own slots do not exclude inherited values such as `constructor`, explicit prototype writes, or sparse-array inheritance. Own data overrides inherited possibilities; `undefined`, mutable/unknown sources and uncertain iteration retain possible defaults. Removed the superseded alias graph/cache and its broad alias tests. The remaining controls cover default selection and inheritance, including explicit undefined slots and an escaped array prototype. Original `require("electron")` evidence remains available when getter identity is rejected. ## Contract and boundary impact - Owner: bootstrap reference selection at the existing canonical loader-getter proof. Shared own-slot inventory, iteration analysis, binding selection and identity-write ownership remain unchanged. - Existing static-analysis scope; no new provider or target execution. CLI/MCP schemas, configuration, dependencies, package/install behavior and release version are unchanged. - Updated the reconstruction guide. No environment-variable, API or deployment documentation change is needed. ## Validation Against upstream `bfdde7ee` on macOS arm64, Node 24.22.1 / npm 11.16.0: - Before the filter: **5 failed / 16 passed** in the 21-case source regression table. All five failures were inactive defaults losing valid window identity. - Final focused lane: **3 files / 94 tests passed**, including 22 default/inheritance controls, 70 existing bundled-require controls, and actual compiled CLI plus stdio MCP. Each public surface parses 13 inert authored fixtures, retains exactly the expected 5 window sources, and preserves every fixture's Electron require observation. - `npm run docs:check`: passed, including 9 README consistency tests. - Architecture guards, production reachability and typed lint passed; test discovery found all 2,639 test files exactly once. `git diff --check` passed. - `npm run check`: typecheck, lint/module boundaries, formatting and metadata passed; Knip failed on two pre-existing unused exports (`staticFileUrlPath` and `ObjcClassImportBindingCoverage`). The same two findings reproduce on an independent upstream `bfdde7ee` source snapshot. No unrelated cleanup or check relaxation is included. The fixtures are parsed, not executed. No full repository test suite, real Electron application, installed package, native provider, or Linux/Windows runtime was verified locally. Remote CI is reported below. ## Current-head remote CI and review [CI run 40438429335](https://github.com/morluto/rea/actions/runs/40438429335) tested head `ef0020ab` in merge `622840d1` against upstream `ded6542d`. - Static checks, package build, documentation, Linux native package baseline, real JavaScript recovery, PR title and author readiness passed. - All 94 PR-focused tests passed remotely: 22 default/inheritance regressions, 70 bundled-require controls, and both compiled CLI/stdio MCP acceptance cases. - Source tests and the boundary shards are not green. The collected report contains 13 failures across `appleApplication` (8), `providerBinding` (1), `javascriptApplicationEmptyLiterals` (1), `ciPrHealth` (1), and `safeOutputTreeCloseFailures` (2). Several other shards were cancelled, so this run does not establish a full-suite pass. - Built an independent source snapshot of the exact upstream base `ded6542d` with the same locked dependencies and Node 24.22.1, then ran those five files on macOS arm64: **13 failed / 81 passed**. The failed test names match all 13 remote failures exactly. These baseline failures are not changed here; no assertions, validation gates or required statuses were relaxed. - [Greptile's review of `ef0020ab`](https://github.com/morluto/rea/pull/4355#issuecomment-6258284147) is **5/5**, with no new blocking issue. No unresolved inline review threads remain at this check. ## Author readiness Human owner: @To3akaRin - [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. --- <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:12:37 +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