[PR #4342] [MERGED] fix(javascript): preserve uncertainty in compound assignment results #6187

Closed
opened 2026-10-31 01:12:32 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4342
Author: @maxazure
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @morluto

Base: main ← Head: fix/compound-assignment-paths-4206


📝 Commits (7)

  • f03c800 fix(javascript): preserve uncertainty in compound assignment results
  • 0e6b112 fix(javascript): retain callable logical assignment alternatives
  • 26b6e7f Merge current main and validate guarded factory compatibility
  • 56753ed fix(javascript): read prior module export assignment values
  • 95bed8f fix: keep mutable module export assignment candidates conservative
  • ac4a1df fix: preserve mixed receiver values and wrapped assignment targets
  • 221ed54 fix: carry mutable export uncertainty through copied values

📊 Changes

4 files changed (+705 additions, -36 deletions)

View changed files

📝 src/domain/javascript/electronWrappedCallPaths.test.ts (+618 -0)
📝 src/domain/javascript/javascriptSymbolicBindings.ts (+83 -36)
📝 src/domain/javascript/javascriptSymbolicControlFlow.ts (+1 -0)
📝 src/domain/javascript/javascriptSymbolicValues.ts (+3 -0)

📄 Description

Summary

Fixes #4206. Preserve guarded IPC path alternatives when a compound assignment's result is not modeled, instead of pruning a branch using its right-hand side.

Problem and expected behavior

With let flag = 1; if (flag -= 1), the symbolic evaluator used the literal RHS 1 and kept only the unguarded then-channel. The subtraction actually produces zero. A conservative static analysis must preserve the alternatives when it cannot evaluate the operation.

Change and scope

Plain = still evaluates its RHS. Arithmetic and bitwise compound assignment results become unknown. Logical assignments (&&=, ||=, ??=) use the existing logical-selection semantics and preserve selected or guarded callable alternatives. Stored namespace writes retain the full assignment node, so export-property writes and module.exports replacements use the same operator-aware evaluator. This removes the second place that stripped the operator.

No arithmetic interpreter or target execution is added. This does not change the separate native-addon binding classifier addressed by #4338.

Contract and boundary impact

  • Semantic owner and earliest changed stage: JavaScriptSymbolicBindings, before contextual path selection.
  • Contribution scope fit and linked maintainer decision: ordinary static-analysis correctness fix; no new authority, decision not applicable.
  • CLI and MCP/tool-catalog contract: unchanged shape; unknown results retain guarded static candidates.
  • Provider, bridge, target-format, or platform compatibility: no provider changes.
  • Evidence, artifact, provenance, or reconstruction contract: preserve raw IPC observations and predicates; do not imply runtime invocation.
  • Target selection, authority, upstream behavior, and cleanup details: unchanged.
  • Generated metadata, package, or installation impact: none.

Expected effects:

  • 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.

Evidence and regression coverage

12 added static-analysis boundary cases cover inline arithmetic, bitwise and logical assignments, unknown operands, plain-assignment controls, and stored namespace property/replacement writes.

On base 42fc6c5, the new cases produce 8 failures with 57 passing existing/control cases. With the fix, all 65 pass.

The exact three-case public CLI reproduction from #4206 was run before and after against inert extracted applications:

  • Before: compound keeps only unguarded then.
  • After: compound retains both then/else with their predicates.
  • Plain flag = 0 stays else-only; unknown operand stays guarded both.
  • Both raw IPC calls survive in every case.

Fixture JavaScript was never executed and Electron was not loaded.

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Initial revision validation (f03c800)

Pinned Node 24.18.0 / npm 11.16.0, Linux:

  • npm run test:focused -- src/domain/javascript/electronWrappedCallPaths.test.ts src/domain/javascript/javascriptSymbolicBindings.test.ts src/domain/javascript/electronModuleInterop.test.ts src/domain/javascript/electronBundledRequireIdentity.test.ts — 332 passed.
  • npm run build:cached and exact issue CLI reproduction — passed before/after with results above.
  • npm run check:fast — passed.
  • npm run check — passed.
  • npm run verify:test-discovery — 1,400 files discovered exactly once.
  • Normal commit hooks and diff whitespace checks — passed.

An independent macOS arm64 review of the initial f03c800 head repeated the four focused suites (332 passed), check:fast, check, discovery (1,400 files), and the exact CLI before/after reproduction with the pinned toolchain. Baseline production plus the regression tests produced the expected 8 failures / 57 passes; the PR head passed all 65. Additional inert static probes for nested assignments, later plain writes, and returned-function branches passed. No code changes were needed.

Review follow-up and CI preflight

The logical-assignment callable regression was reproduced with 11 failing cases before the follow-up. Fourteen added cases now cover RHS functions, resolvable LHS callables, skipped RHS and cyclic namespace reads. The focused suites passed 346 tests before syncing main.

After integrating main 2c75835 (including guarded factory completion), the tested tree is 4ff2da60:

  • Related regressions and upstream factory completion: 359 passed.
  • Cold check:ci, discovery (1,414 files), Python (106 tests), and test:prepare: passed.
  • Full source group: 8,574 passed, 12 failed, 8 skipped. All 12 failures reproduce on exact main in this environment.
  • Rechecked the 35 previously failing files: 153 failures across 29 files, all matching exact main by file and test name. Six formerly failing files passed.
  • Original issue CLI and logical-callable CLI reproductions passed.

Before syncing main, a clean-install CI-faithful source plus five built-shard run accounted for all 1,400 files: 16,032 passed, 159 failed, 141 skipped. All failures reproduced on the old base. Cold static/build, docs, real JavaScript recovery (12 observations), and Inspector (8 variants) passed. That complete shard run applies to the pre-sync tree, not the current tree. The full five built shards were not repeated after syncing.

Local host/provider restrictions prevent claiming a fully green local suite. Windows cross-build and the complete installed-package gate were not run because the cross-compiler/artifacts were unavailable. GitHub CI on the new head is pending.

Known pre-existing limitation: prior mutable namespace LHS callables can be lost through last-write lookup; this follow-up restores guarded RHS candidates without adding flow-sensitive write tracking.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: Linux and macOS arm64 static CLI; no external provider.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new target execution or authority.
  • Proof gaps: the current merged tree has not had all five built shards repeated locally; no complete installed-package or Windows cross-build validation. No performance claim; stored plain assignments add a bounded evaluation layer.

Author readiness

Human owner: @maxazure. Initial readiness was confirmed by the owner on 2026-10-16 for f03c800; subsequent review fixes and their verification are recorded above.

  • 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/4342 **Author:** [@maxazure](https://github.com/maxazure) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/compound-assignment-paths-4206` --- ### 📝 Commits (7) - [`f03c800`](https://github.com/morluto/rea/commit/f03c800bcfd8ba4cc2b1388c4308cb9da20177bc) fix(javascript): preserve uncertainty in compound assignment results - [`0e6b112`](https://github.com/morluto/rea/commit/0e6b1123d5baf86ed0ea67fffad83386e2c8faf0) fix(javascript): retain callable logical assignment alternatives - [`26b6e7f`](https://github.com/morluto/rea/commit/26b6e7f7bb7f4842fb54cdc43d51bb68e19fdff2) Merge current main and validate guarded factory compatibility - [`56753ed`](https://github.com/morluto/rea/commit/56753ed27ef8a986d531887abc6f11e36c8d0c7f) fix(javascript): read prior module export assignment values - [`95bed8f`](https://github.com/morluto/rea/commit/95bed8f1961255062832670b8badbce89467969f) fix: keep mutable module export assignment candidates conservative - [`ac4a1df`](https://github.com/morluto/rea/commit/ac4a1df04427a86cc565ab6cf2afb571dde0d4ed) fix: preserve mixed receiver values and wrapped assignment targets - [`221ed54`](https://github.com/morluto/rea/commit/221ed547c10f72a92df23903db2de87b448e0be5) fix: carry mutable export uncertainty through copied values ### 📊 Changes **4 files changed** (+705 additions, -36 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/electronWrappedCallPaths.test.ts` (+618 -0) 📝 `src/domain/javascript/javascriptSymbolicBindings.ts` (+83 -36) 📝 `src/domain/javascript/javascriptSymbolicControlFlow.ts` (+1 -0) 📝 `src/domain/javascript/javascriptSymbolicValues.ts` (+3 -0) </details> ### 📄 Description ## Summary Fixes #4206. Preserve guarded IPC path alternatives when a compound assignment's result is not modeled, instead of pruning a branch using its right-hand side. ## Problem and expected behavior With `let flag = 1; if (flag -= 1)`, the symbolic evaluator used the literal RHS `1` and kept only the unguarded then-channel. The subtraction actually produces zero. A conservative static analysis must preserve the alternatives when it cannot evaluate the operation. ## Change and scope Plain `=` still evaluates its RHS. Arithmetic and bitwise compound assignment results become unknown. Logical assignments (`&&=`, `||=`, `??=`) use the existing logical-selection semantics and preserve selected or guarded callable alternatives. Stored namespace writes retain the full assignment node, so export-property writes and `module.exports` replacements use the same operator-aware evaluator. This removes the second place that stripped the operator. No arithmetic interpreter or target execution is added. This does not change the separate native-addon binding classifier addressed by #4338. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScriptSymbolicBindings, before contextual path selection. - Contribution scope fit and linked maintainer decision: ordinary static-analysis correctness fix; no new authority, decision not applicable. - CLI and MCP/tool-catalog contract: unchanged shape; unknown results retain guarded static candidates. - Provider, bridge, target-format, or platform compatibility: no provider changes. - Evidence, artifact, provenance, or reconstruction contract: preserve raw IPC observations and predicates; do not imply runtime invocation. - Target selection, authority, upstream behavior, and cleanup details: unchanged. - Generated metadata, package, or installation impact: none. Expected effects: - [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. ## Evidence and regression coverage 12 added static-analysis boundary cases cover inline arithmetic, bitwise and logical assignments, unknown operands, plain-assignment controls, and stored namespace property/replacement writes. On base `42fc6c5`, the new cases produce 8 failures with 57 passing existing/control cases. With the fix, all 65 pass. The exact three-case public CLI reproduction from #4206 was run before and after against inert extracted applications: - Before: compound keeps only unguarded then. - After: compound retains both then/else with their predicates. - Plain `flag = 0` stays else-only; unknown operand stays guarded both. - Both raw IPC calls survive in every case. Fixture JavaScript was never executed and Electron was not loaded. - [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. ## Initial revision validation (`f03c800`) Pinned Node 24.18.0 / npm 11.16.0, Linux: - `npm run test:focused -- src/domain/javascript/electronWrappedCallPaths.test.ts src/domain/javascript/javascriptSymbolicBindings.test.ts src/domain/javascript/electronModuleInterop.test.ts src/domain/javascript/electronBundledRequireIdentity.test.ts` — 332 passed. - `npm run build:cached` and exact issue CLI reproduction — passed before/after with results above. - `npm run check:fast` — passed. - `npm run check` — passed. - `npm run verify:test-discovery` — 1,400 files discovered exactly once. - Normal commit hooks and diff whitespace checks — passed. An independent macOS arm64 review of the initial `f03c800` head repeated the four focused suites (332 passed), check:fast, check, discovery (1,400 files), and the exact CLI before/after reproduction with the pinned toolchain. Baseline production plus the regression tests produced the expected 8 failures / 57 passes; the PR head passed all 65. Additional inert static probes for nested assignments, later plain writes, and returned-function branches passed. No code changes were needed. ## Review follow-up and CI preflight The logical-assignment callable regression was reproduced with 11 failing cases before the follow-up. Fourteen added cases now cover RHS functions, resolvable LHS callables, skipped RHS and cyclic namespace reads. The focused suites passed 346 tests before syncing main. After integrating main `2c75835` (including guarded factory completion), the tested tree is `4ff2da60`: - Related regressions and upstream factory completion: 359 passed. - Cold `check:ci`, discovery (1,414 files), Python (106 tests), and `test:prepare`: passed. - Full source group: 8,574 passed, 12 failed, 8 skipped. All 12 failures reproduce on exact main in this environment. - Rechecked the 35 previously failing files: 153 failures across 29 files, all matching exact main by file and test name. Six formerly failing files passed. - Original issue CLI and logical-callable CLI reproductions passed. Before syncing main, a clean-install CI-faithful source plus five built-shard run accounted for all 1,400 files: 16,032 passed, 159 failed, 141 skipped. All failures reproduced on the old base. Cold static/build, docs, real JavaScript recovery (12 observations), and Inspector (8 variants) passed. That complete shard run applies to the pre-sync tree, not the current tree. The full five built shards were not repeated after syncing. Local host/provider restrictions prevent claiming a fully green local suite. Windows cross-build and the complete installed-package gate were not run because the cross-compiler/artifacts were unavailable. GitHub CI on the new head is pending. Known pre-existing limitation: prior mutable namespace LHS callables can be lost through last-write lookup; this follow-up restores guarded RHS candidates without adding flow-sensitive write tracking. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Linux and macOS arm64 static CLI; no external provider. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new target execution or authority. - Proof gaps: the current merged tree has not had all five built shards repeated locally; no complete installed-package or Windows cross-build validation. No performance claim; stored plain assignments add a bounded evaluation layer. ## Author readiness Human owner: @maxazure. Initial readiness was confirmed by the owner on 2026-10-16 for `f03c800`; subsequent review fixes and their verification are recorded above. - [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 2026-10-31 01:12:32 +00:00
Sign in to join this conversation.
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#6187
No description provided.