Repository navigation
fix(javascript): resolve module-level Electron options in functions - #8250
ShuaixinHuang wants to merge 5 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. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
 [Medium impact] The PR changes Electron option findings in JavaScript analysis. Fix the early global-call case before merging so uncertain Electron settings remain dynamic. Findings
Fix with agent prompt### Issue 1
src/domain/javascript/javascriptSemanticMutationInitializers.ts:874-881
In a classic script, `globalThis.createWindow()` can call a top-level function before `webPreferences` is initialized. This scan skips that member access because the property name is not a referenced identifier. If a bare `createWindow()` also appears after the constant, the scan uses that later call as proof and grants `hasDefiniteInitializer: true`.
For example: `globalThis.createWindow(); const webPreferences = { nodeIntegration: true }; function createWindow() { return new BrowserWindow({ webPreferences }); } createWindow();`. The early call throws before initialization, but Electron analysis can report literal preferences instead of `dynamic`. This violates the repository directive to preserve uncertainty.
Require parser-owned proof of module or CommonJS isolation before using this check, or account for global function access. Add an early-global-call regression.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThe PR retains initialized module-level Electron options inside functions and loops.
Validation limits: only the stubbed JavaScript timing example was executed during this review; the analyzer tests were not run. The supplied description says this PR replaces #8087. Current supersession status and prior discussion could not be checked because GitHub access was unavailable. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Read a program-scope constant inside a function] --> B[Find the enclosing function]
B --> C{Function declaration?}
C -->|Yes| D[Find first indexed module-level reference]
C -->|No| E[Compare initializer and function positions]
D --> F{Initializer comes first?}
E --> F
F -->|Yes| G[Grant definite initialization; keep capture order unknown]
F -->|No| H[Keep initialization uncertain]
G --> I[Electron checks object changes]
I --> J[Report literal preferences when unchanged]
Reviews (1) · Last reviewed commit: "fix(javascript): retain function ownersh..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| !t.isIdentifier(node) || | ||
| parent === null || | ||
| !t.isReferenced(node, parent) | ||
| ) | ||
| return; | ||
| const fn = functions.get(node.name); | ||
| if (fn == null || active.has(fn)) return; |
There was a problem hiding this comment.
In a classic script, globalThis.createWindow() can call a top-level function before webPreferences is initialized. This scan skips that member access because the property name is not a referenced identifier. If a bare createWindow() also appears after the constant, the scan uses that later call as proof and grants hasDefiniteInitializer: true.
For example: globalThis.createWindow(); const webPreferences = { nodeIntegration: true }; function createWindow() { return new BrowserWindow({ webPreferences }); } createWindow();. The early call throws before initialization, but Electron analysis can report literal preferences instead of dynamic. This violates the repository directive to preserve uncertainty.
Require parser-owned proof of module or CommonJS isolation before using this check, or account for global function access. Add an early-global-call regression.
Context Used: AGENTS.md (source)
Knowledge Base Used: Artifact and application analysis
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/domain/javascript/javascriptSemanticMutationInitializers.ts
Line: 874-881
Comment:
**Early global calls look initialized**
In a classic script, `globalThis.createWindow()` can call a top-level function before `webPreferences` is initialized. This scan skips that member access because the property name is not a referenced identifier. If a bare `createWindow()` also appears after the constant, the scan uses that later call as proof and grants `hasDefiniteInitializer: true`.
For example: `globalThis.createWindow(); const webPreferences = { nodeIntegration: true }; function createWindow() { return new BrowserWindow({ webPreferences }); } createWindow();`. The early call throws before initialization, but Electron analysis can report literal preferences instead of `dynamic`. This violates the repository directive to preserve uncertainty.
Require parser-owned proof of module or CommonJS isolation before using this check, or account for global function access. Add an early-global-call regression.
**Context Used:** AGENTS.md ([source](https://github.com/morluto/rea/blob/main/AGENTS.md))
**Knowledge Base Used:** [Artifact and application analysis](https://app.greptile.com/-/switch-tenant?tenant=pre&next=%2Fcustom-context%2Fknowledge-base%2Fmorluto%2Frea%2F-%2Fdocs%2Fartifact-and-application-analysis.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Superseded by #8331 at the author's explicit request: #8331. This PR is closed without merging to keep one replacement open. The replacement retains the identical published head and full patch, carries the unresolved P1 finding at #8250 (comment), and requires its own hosted CI and author confirmation. The local unpublished follow-up is not part of either published revision.
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.