Repository navigation
fix(javascript): resolve module-level Electron options in functions - #8331
ShuaixinHuang wants to merge 6 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The change reaches shared JavaScript static reports through a narrowly gated binding rule. Fix the receiver check before merging so unrelated member calls do not erase known Electron settings. Findings
Fix with agent prompt### Issue 1
src/domain/javascript/javascriptSemanticMutationInitializers.ts:893-900
The new check treats locally bound `window`, `self`, and `globalThis` objects as the global object. For example, `const window = { createWindow() {} }; window.createWindow();` before the preferences initializer is unrelated to the module's `createWindow` function. If the real function is called after initialization, this unrelated member call still becomes its first recorded read. REA then drops the known preferences and reports `dynamic`.
Check the receiver's binding before counting its property access as a reference to the module function.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe PR resolves module-level Electron options inside functions while keeping uncertain initialization unknown.
Reviews (2) · Last reviewed commit: "fix(javascript): account for classic glo..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| t.isThisExpression(receiver) || | ||
| (t.isIdentifier(receiver) && | ||
| (receiver.name === "globalThis" || | ||
| receiver.name === "window" || | ||
| receiver.name === "self")) | ||
| ) | ||
| name = semanticStaticPropertyKey(node.property, node.computed); |
There was a problem hiding this comment.
The new check treats locally bound window, self, and globalThis objects as the global object. For example, const window = { createWindow() {} }; window.createWindow(); before the preferences initializer is unrelated to the module's createWindow function. If the real function is called after initialization, this unrelated member call still becomes its first recorded read. REA then drops the known preferences and reports dynamic.
Check the receiver's binding before counting its property access as a reference to the module function.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptSemanticMutationInitializers.ts
Line: 893-900
Comment:
**Known Electron settings disappear**
The new check treats locally bound `window`, `self`, and `globalThis` objects as the global object. For example, `const window = { createWindow() {} }; window.createWindow();` before the preferences initializer is unrelated to the module's `createWindow` function. If the real function is called after initialization, this unrelated member call still becomes its first recorded read. REA then drops the known preferences and reports `dynamic`.
Check the receiver's binding before counting its property access as a reference to the module function.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Supersedes #8250 at the author's explicit request to resubmit the existing PRs. This replacement reuses the identical published head
cc2bb1242a5611527bfb99935bc7dbcf85e902f7and full three-file patch. Prior discussion and historical verification remain at #8250 (earlier #8087). No code changes or new local test runs were made for this resubmission. This new PR requires its own hosted CI. The owner has specifically confirmed review of this replacement PR's final diff, evidence, limitations and unresolved P1, accepted full responsibility, and approved it for initial maintainer review. The P1 remains a before-merge blocker.Outstanding review follow-up — not fixed in this published revision
The known P1 review finding remains unresolved in this head. In a classic script, an early
globalThis.createWindow()followed by a later bare call can make module-constant initialization appear definite even though the early activation encounters the temporal dead zone. The previously reported 115-test verification below does not establish that this additional case is fixed.A separate local draft follow-up has 67 focused tests and three build tasks passing, plus
check:fast; expanded verification and commit/push are not complete. That draft is not part of this PR's published commit or diff, and those draft results are not claimed as a fix in this revision. This finding remains a before-merge blocker; the original review is preserved rather than marked resolved.Supersedes #8087 at the author's request. This replacement reuses verified head cc2bb12 and preserves the original discussion and validation record at #8087. Historical revision-specific validation below remains at its stated execution points. This new PR must pass its own hosted CI.
Problem and resulting behavior
Fixes #7227. Electron static analysis loses module-level immutable
webPreferencesread inside a later activation. Retain those settings when initialization precedes the function's module-level uses, while preserving uncertainty when a hoisted function can run earlier.For example, this source must remain dynamic rather than report a proven
nodeIntegration: true:Moving the call after the const initializer yields
object-literalwith the literal setting, including when the function declaration appears before the initializer.Owning change and scope
Initialization coverage stays in
javascriptSemanticMutationInitializers.ts; Electron consumers use that fact without reinterpreting declaration order. A hoisted declaration's position does not prove activation timing. The owner indexes module-level function references once per immutable module body and treats earlier aliases, indirect activations, live exports and unproven timing conservatively. This is a bounded reference-order check, not a general call graph or runtime execution proof.The existing ancestry cache also retains the enclosing function, avoiding repeated walks from deep reads. Initialization coverage remains separate from cross-function capture ordering: the latter stays unresolved so named child-process argv shapes retain their existing uncertainty. No public schema, provider, execution authority, deadline, resource ceiling or assertion was weakened.
Validation at the current follow-up
Windows, Node 24.22.1 / npm 11.16.0:
fb2532ad61ceb4cce7d233131fc1edd4305925ff, with the new source regressions: 4 failed / 47 passed. Three cases expose early direct, aliased and forwarding calls; the fourth exposes a late-call positive control.9bafc838629358fb8ebc0ce86abefa5c82c8a0ab: 112/112 passed across six complete files: Electron options 51, existing scope-depth/resource controls 14, binding initializers 13, argument shapes 30, existing child-process compiled CLI/stdio MCP 2, and new Electron initialization compiled CLI/stdio MCP 2. Three build prerequisite tasks passed.The earlier head's independent hosted CI passed, but it did not cover this hoisted-declaration review case. The new head requires its own hosted CI; an earlier green run does not validate it. Local checks use synthetic JavaScript through production static analysis and real local compiled CLI/stdio MCP. No real Electron application or Linux/macOS execution is claimed.
Loop ownership review follow-up
Commit
cc2bb1242a5611527bfb99935bc7dbcf85e902f7keeps enclosing-function identity in its own immutable ancestry cache. Loop ordering remains uncertain; entering a loop no longer erases which function owns the read. This retains initialized module-level Electron settings inside for-of, for and while loops while keeping early calls dynamic. No downstream reinterpretation or call graph expansion was added.On exact preceding head
9bafc838, the three new loop source cases fail while the existing 51 pass. With the correction, six complete files pass 115/115 (Electron 54, scope-depth 14, initializers 13, shapes 30, existing child-process CLI/MCP 2, Electron initialization CLI/MCP 2). The public adapter cases include both early and late calls inside loops and outside loops, with source provenance. All three committed files match the tested bytes exactly after normal precommit hooks. Three build tasks passed; normal prepush typecheck/lint passed 2/2, 46.166 seconds. No assertions or resource controls were relaxed.The earlier
9bafc838own hosted workflow41009833080completed successfully; the newcc2bb124head requires independent hosted validation. Validation uses synthetic inert JavaScript and real local compiled CLI/stdio MCP on Windows, Node 24.22.1 / npm 11.16.0; no real Electron application or Linux/macOS run is claimed.Author readiness
Human owner: @ShuaixinHuang
Before merge
type(scope): outcome.