morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #4196] [MERGED] fix(javascript): preserve values captured before later assignments #6119

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4196
Author: @N0zoM1z0
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/javascript-captured-parameter-3881

📝 Commits (6)

64ae8cf fix(javascript): preserve values captured before later assignments
3e753eb fix(javascript): keep instance field assignments delayed
27f03a4 fix(javascript): share conditional evaluation facts
a4b5a36 fix(javascript): preserve partial expression evaluation order
9363b15 fix(javascript): cache read snapshots within query budgets
3a61e09 fix(javascript): preserve skipped defaults and loop tests

📊 Changes

13 files changed (+1283 additions, -270 deletions)

View changed files

📝 docs/javascript-application-workflows.md (+7 -0)
📝 src/domain/javascript/javascriptAstValues.ts (+9 -0)
📝 src/domain/javascript/javascriptSemanticAnalysis.scope_depth.test.ts (+25 -7)
📝 src/domain/javascript/javascriptSemanticAnalysis.ts (+3 -47)
📝 src/domain/javascript/javascriptSemanticAnalysis.unions.test.ts (+2 -2)
➕ src/domain/javascript/javascriptSemanticEvaluationOrder.ts (+82 -0)
📝 src/domain/javascript/javascriptSemanticMutationInitializers.ts (+359 -151)
📝 src/domain/javascript/javascriptSemanticObjectReferences.ts (+3 -2)
📝 src/domain/javascript/javascriptSemanticReturns.ts (+2 -2)
📝 src/domain/javascript/javascriptSemanticValues.memo.test.ts (+194 -31)
📝 src/domain/javascript/javascriptSemanticValues.ts (+131 -27)
➕ tests/acceptance/applications/javascriptCapturedParameters.test.ts (+464 -0)
📝 tests/fixtures/javascriptReturnShapes.ts (+2 -1)

📄 Description

Summary

Fixes #3881. const saved=x; x=2; return {type:"ok",value:saved} previously reported literal 2, falsely equating its export shape with a function returning 2. The earlier capture now remains caller-dependent unknown; proven preceding assignments retain their constants.

Change and boundaries

One owner selects binding facts at each read for values, provenance, callable returns and object-reference tracking. Whole-binding and temporal-value caches stay separate. Binding production and mutation positioning share branch/instance-activation rules; transparent TypeScript wrappers use the existing parser with grouping preservation. Superseded helpers, duplicate branch rules and duplicate snapshot construction are removed.

Expression order explicitly distinguishes before / after / unresolved, including object/array evaluation order. Immutable ancestry and child indices are cached per parent map, without copying wide argument arrays; one expression-source context bounds retained ancestry to O(AST).

Review also exposed exponential shared-dependency evaluation after snapshots bypassed caches. Temporal values now have query-owned memos keyed by parsed initializer fact, source binding, capture point and exact depth. Dependency proofs reject unsafe caller-cycle reuse; budget dependence propagates on hits. Exhausted depth is not retried under speculative capture. Ending the query prevents reuse after effect changes, while existing whole-binding invalidation remains authoritative.

Ordinary static-analysis fixes; new maintainer scope decision not applicable. No new execution, injection, mutation or recovery capability; no public CLI/MCP schema, provider or installation change. Existing cross-block and same-statement multiple-write precision limits were independently reproduced with exactly equal base/head return-shape records and explained in review. Those explicit unknowns remain; general control-flow or complete constant folding is not claimed.

Executed validation

Public CLI on base 633e4ff43435480ededb5566754e16fb13b4582f reproduced false literal/equality; actual Node input 5 returns 5 versus 2. Original new regression subset against that production code: 4 failed / 13 passed.
Rebased onto fresh main 08a897b2cd7b359fcb0674c82baec3f1a74af632. Final npm run test:focused -- all 74 JavaScript domain files plus tests/acceptance/applications/javascriptCapturedParameters.test.ts: 1,830 passed / 75 files. Seven real Node/compiled CLI/production stdio-MCP acceptance tests cover read positions, alias/default/destructured/nested bindings, delayed fields versus immediate keys/static fields, optional/logical/typed/grouped calls, object/array ordering, a 30-layer reconvergent chain, distinct escape capture points, lazy destructuring defaults and do-while test skipping. Unknown ordering stays unknown. The final lazy-position regression failed on the previous head (1 failed / 6 passed); real Node object/array defaults, assignment defaults, nested patterns, unconditional keys/initializers and do-body controls all pass after the shared rule fix.
Review regressions were verified before fixes: instance fields (1 failed / 2 passed), optional chains (1 failed / 3 passed), same-expression false 0 (Node 1, base unknown), skipped defaults (Node 0, PR 1) and do-test skipping (Node 0, PR 1, pinned base unknown), and snapshot dependency work (1 failed / 7 passed): depths 8→12 produced 1,024→16,384 dependency records versus a <3× bound. Original growth bounds now pass. Tests also preserve remaining depth, stop exhausted-budget work, distinguish whole/read caches, respect effect invalidation and preserve cycles under different active ancestors, including rebound bindings.
Real SDK/stdio flows use summary analyses, seven retained-reference comparisons and full bundle retrieval. Parent Evidence, source links and inline/reference parity are checked. A 16-layer shared-dependency task runs three interleaved times/version with identical complete parent Evidence, catalog and all SDK messages before/after the cache fix. The original issue workflow also passes again with source records deeply equal to the retained full records.
Final npm run check, npm run docs:check (5 consistency tests), npm run verify:test-discovery (all 1,356 files exactly once), and git diff --check passed. Existing MCP export-shape suite also passed earlier. Linux static JavaScript/TypeScript and actual authored Node execution; no Hopper/Ghidra/browser-provider claim.

Measured costs and limitations

Serial jobs, two-core affinity and fresh servers/private homes; cold does not mean cold OS caches. Three interleaved runs/version on the identical 16-layer DAG task, reviewed head a4b5a363a → cache fix:

Median	Before	After
First two analyses + comparison	7.603 s	7.303 s
Complete MCP run	10.674 s	10.454 s
Reaped CPU	12.469 s	12.240 s
Peak family RSS	953.0 MiB	907.0 MiB
Warm comparisons (18/version)	7.143 ms	5.586 ms

All serialized SDK payloads/catalogs are identical: cl100k_base JSON proxies 799,018 catalog tokens, 11,855 first-goal tokens, 997,869 complete-workflow tokens (including seven comparisons and complete bundle verification), 140-token retained comparison request. Thus caching adds no transport context/calls. Actual model consumption/billing and initialization/wire framing are not measured.

Domain-only DAG profiling, three repetitions/size: 16 layers 345.5→6.3 ms; fixed 30/64 layers 8.1/17.7 ms; 256 layers returns depth-limited unknown in 66.6 ms. The billion-leaf old-head 30-layer case was not executed; the measured 8/12-layer work growth establishes the regression without doing that work.

Flat interleaved assignments/captures (200/400/800, three repetitions after warm-up), original main → final: 41/106/226→53/105/258 ms. Complete profiling process 1.394→1.570 s, CPU 1.870→2.055 s, peak RSS 172→172 MiB. The final result matches Node's captured literal; the base remains unknown. This modest extra cost is reported, rather than claiming universal speedup.

For the original issue task, matched earlier runs varied with host timing (wall 11.816–20.581→11.151–20.925 s, CPU 13.738–23.416→13.032–24.110 s, median peak 901.3→913.4 MiB); no causal rate is inferred. Correct uncertainty changes first-goal JSON proxies 12,110→12,684 and matched whole-workflow 905,129→911,298 with the then-identical 814,202-token catalog. Upstream schema presentation later reduces that catalog to 799,018; the final original-issue run is 896,114 tokens, 10.265 s, 12.040 CPU s, 938.9 MiB, with exactly unchanged request/reply payloads. Upstream savings are not attributed here. Retained comparison inputs use approximately 150 tokens versus 23,600 with full inline inputs, reusing an existing interface while keeping full Evidence accessible.

The DAG/cache measurements precede the final two-line lazy-position guard; the final original-issue SDK workflow was rerun and its complete catalog/messages are exactly equal.

Shared-host samples do not establish general speed/memory guarantees. Per-call CPU was not measured. Largest sampled local check family RSS including failed attempts 4.68 GiB, within the 8 GiB budget; no guard stop or session OOM. Sampling every 200 ms can miss brief peaks or double-count shared pages.

Author readiness

Human owner: @N0zoM1z0

The human owner explicitly authorized delegated review, proxy completion of these declarations, ready-for-review publication and normal merge for this batch. The agent reviewed the final diff and evidence; this records that delegation and does not assert a separate manual review occurred.

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/4196 **Author:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/javascript-captured-parameter-3881` --- ### 📝 Commits (6) - [`64ae8cf`](https://github.com/morluto/rea/commit/64ae8cf16e87d037091c97532bfa6583e350f0e9) fix(javascript): preserve values captured before later assignments - [`3e753eb`](https://github.com/morluto/rea/commit/3e753ebf590912c4320944bf85f54a21f79d3392) fix(javascript): keep instance field assignments delayed - [`27f03a4`](https://github.com/morluto/rea/commit/27f03a404c9cdca178db104eba8e760681abfeb2) fix(javascript): share conditional evaluation facts - [`a4b5a36`](https://github.com/morluto/rea/commit/a4b5a363a6c248050b1dd888b849bab736d1a455) fix(javascript): preserve partial expression evaluation order - [`9363b15`](https://github.com/morluto/rea/commit/9363b15df25205a2c4801a9ff21a7c5654bd3394) fix(javascript): cache read snapshots within query budgets - [`3a61e09`](https://github.com/morluto/rea/commit/3a61e0900b08123bfdffc8fb7f4ce397e9a142db) fix(javascript): preserve skipped defaults and loop tests ### 📊 Changes **13 files changed** (+1283 additions, -270 deletions) <details> <summary>View changed files</summary> 📝 `docs/javascript-application-workflows.md` (+7 -0) 📝 `src/domain/javascript/javascriptAstValues.ts` (+9 -0) 📝 `src/domain/javascript/javascriptSemanticAnalysis.scope_depth.test.ts` (+25 -7) 📝 `src/domain/javascript/javascriptSemanticAnalysis.ts` (+3 -47) 📝 `src/domain/javascript/javascriptSemanticAnalysis.unions.test.ts` (+2 -2) ➕ `src/domain/javascript/javascriptSemanticEvaluationOrder.ts` (+82 -0) 📝 `src/domain/javascript/javascriptSemanticMutationInitializers.ts` (+359 -151) 📝 `src/domain/javascript/javascriptSemanticObjectReferences.ts` (+3 -2) 📝 `src/domain/javascript/javascriptSemanticReturns.ts` (+2 -2) 📝 `src/domain/javascript/javascriptSemanticValues.memo.test.ts` (+194 -31) 📝 `src/domain/javascript/javascriptSemanticValues.ts` (+131 -27) ➕ `tests/acceptance/applications/javascriptCapturedParameters.test.ts` (+464 -0) 📝 `tests/fixtures/javascriptReturnShapes.ts` (+2 -1) </details> ### 📄 Description ## Summary Fixes #3881. `const saved=x; x=2; return {type:"ok",value:saved}` previously reported literal `2`, falsely equating its export shape with a function returning `2`. The earlier capture now remains caller-dependent `unknown`; proven preceding assignments retain their constants. ## Change and boundaries One owner selects binding facts at each read for values, provenance, callable returns and object-reference tracking. Whole-binding and temporal-value caches stay separate. Binding production and mutation positioning share branch/instance-activation rules; transparent TypeScript wrappers use the existing parser with grouping preservation. Superseded helpers, duplicate branch rules and duplicate snapshot construction are removed. Expression order explicitly distinguishes **before / after / unresolved**, including object/array evaluation order. Immutable ancestry and child indices are cached per parent map, without copying wide argument arrays; one expression-source context bounds retained ancestry to O(AST). Review also exposed exponential shared-dependency evaluation after snapshots bypassed caches. Temporal values now have query-owned memos keyed by parsed initializer fact, source binding, capture point and exact depth. Dependency proofs reject unsafe caller-cycle reuse; budget dependence propagates on hits. Exhausted depth is not retried under speculative capture. Ending the query prevents reuse after effect changes, while existing whole-binding invalidation remains authoritative. Ordinary static-analysis fixes; new maintainer scope decision not applicable. No new execution, injection, mutation or recovery capability; no public CLI/MCP schema, provider or installation change. Existing cross-block and same-statement multiple-write precision limits were independently reproduced with exactly equal base/head return-shape records and explained in review. Those explicit unknowns remain; general control-flow or complete constant folding is not claimed. ## Executed validation - Public CLI on base `633e4ff43435480ededb5566754e16fb13b4582f` reproduced false literal/equality; actual Node input `5` returns `5` versus `2`. Original new regression subset against that production code: **4 failed / 13 passed**. - Rebased onto fresh main `08a897b2cd7b359fcb0674c82baec3f1a74af632`. Final `npm run test:focused --` all 74 JavaScript domain files plus `tests/acceptance/applications/javascriptCapturedParameters.test.ts`: **1,830 passed / 75 files**. Seven real Node/compiled CLI/production stdio-MCP acceptance tests cover read positions, alias/default/destructured/nested bindings, delayed fields versus immediate keys/static fields, optional/logical/typed/grouped calls, object/array ordering, a 30-layer reconvergent chain, distinct escape capture points, lazy destructuring defaults and do-while test skipping. Unknown ordering stays unknown. The final lazy-position regression failed on the previous head (**1 failed / 6 passed**); real Node object/array defaults, assignment defaults, nested patterns, unconditional keys/initializers and do-body controls all pass after the shared rule fix. - Review regressions were verified before fixes: instance fields (**1 failed / 2 passed**), optional chains (**1 failed / 3 passed**), same-expression false `0` (Node `1`, base unknown), skipped defaults (Node `0`, PR `1`) and do-test skipping (Node `0`, PR `1`, pinned base unknown), and snapshot dependency work (**1 failed / 7 passed**): depths 8→12 produced **1,024→16,384** dependency records versus a <3× bound. Original growth bounds now pass. Tests also preserve remaining depth, stop exhausted-budget work, distinguish whole/read caches, respect effect invalidation and preserve cycles under different active ancestors, including rebound bindings. - Real SDK/stdio flows use summary analyses, seven retained-reference comparisons and full bundle retrieval. Parent Evidence, source links and inline/reference parity are checked. A 16-layer shared-dependency task runs three interleaved times/version with **identical complete parent Evidence, catalog and all SDK messages** before/after the cache fix. The original issue workflow also passes again with source records deeply equal to the retained full records. - Final `npm run check`, `npm run docs:check` (5 consistency tests), `npm run verify:test-discovery` (all **1,356** files exactly once), and `git diff --check` passed. Existing MCP export-shape suite also passed earlier. Linux static JavaScript/TypeScript and actual authored Node execution; no Hopper/Ghidra/browser-provider claim. <details> <summary>Measured costs and limitations</summary> Serial jobs, two-core affinity and fresh servers/private homes; cold does not mean cold OS caches. Three interleaved runs/version on the identical **16-layer DAG task**, reviewed head `a4b5a363a` → cache fix: | Median | Before | After | | --- | ---: | ---: | | First two analyses + comparison | 7.603 s | 7.303 s | | Complete MCP run | 10.674 s | 10.454 s | | Reaped CPU | 12.469 s | 12.240 s | | Peak family RSS | 953.0 MiB | 907.0 MiB | | Warm comparisons (18/version) | 7.143 ms | 5.586 ms | All serialized SDK payloads/catalogs are identical: `cl100k_base` JSON proxies **799,018 catalog tokens**, **11,855 first-goal tokens**, **997,869 complete-workflow tokens** (including seven comparisons and complete bundle verification), **140-token retained comparison request**. Thus caching adds no transport context/calls. Actual model consumption/billing and initialization/wire framing are not measured. Domain-only DAG profiling, three repetitions/size: 16 layers **345.5→6.3 ms**; fixed 30/64 layers **8.1/17.7 ms**; 256 layers returns depth-limited unknown in **66.6 ms**. The billion-leaf old-head 30-layer case was not executed; the measured 8/12-layer work growth establishes the regression without doing that work. Flat interleaved assignments/captures (200/400/800, three repetitions after warm-up), original main → final: **41/106/226→53/105/258 ms**. Complete profiling process **1.394→1.570 s**, CPU **1.870→2.055 s**, peak RSS **172→172 MiB**. The final result matches Node's captured literal; the base remains unknown. This modest extra cost is reported, rather than claiming universal speedup. For the original issue task, matched earlier runs varied with host timing (wall **11.816–20.581→11.151–20.925 s**, CPU **13.738–23.416→13.032–24.110 s**, median peak **901.3→913.4 MiB**); no causal rate is inferred. Correct uncertainty changes first-goal JSON proxies **12,110→12,684** and matched whole-workflow **905,129→911,298** with the then-identical 814,202-token catalog. Upstream schema presentation later reduces that catalog to 799,018; the final original-issue run is **896,114 tokens**, **10.265 s**, **12.040 CPU s**, **938.9 MiB**, with exactly unchanged request/reply payloads. Upstream savings are not attributed here. Retained comparison inputs use approximately 150 tokens versus 23,600 with full inline inputs, reusing an existing interface while keeping full Evidence accessible. The DAG/cache measurements precede the final two-line lazy-position guard; the final original-issue SDK workflow was rerun and its complete catalog/messages are exactly equal. Shared-host samples do not establish general speed/memory guarantees. Per-call CPU was not measured. Largest sampled local check family RSS including failed attempts **4.68 GiB**, within the 8 GiB budget; no guard stop or session OOM. Sampling every 200 ms can miss brief peaks or double-count shared pages. </details> ## Author readiness Human owner: @N0zoM1z0 The human owner explicitly authorized delegated review, proxy completion of these declarations, ready-for-review publication and normal merge for this batch. The agent reviewed the final diff and evidence; this records that delegation and does not assert a separate manual review occurred. - [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:00 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:16 +00:00

[PR #6247] [MERGED] fix(environment): report capture payload failures at their field paths #7224

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#6119

No description provided.

Rows
Columns