morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #7128] [MERGED] fix(javascript): stop recording replaced slot values as store receivers #7661

New issue

Closed

opened 2026-11-03 13:39:27 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-03 13:39:27 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7128
Author: @kaoru0822-kitauji
Created: 10/25/2026
Status: ✅ Merged
Merged: 10/25/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/6311-ambient-store-identities

📝 Commits (1)

7fa53f8 fix(javascript): stop recording replaced slot values as store receivers

📊 Changes

2 files changed (+189 additions, -49 deletions)

View changed files

📝 src/domain/javascript/javascriptSemanticAmbientWrites.test.ts (+112 -0)
📝 src/domain/javascript/javascriptSemanticAmbientWrites.ts (+77 -49)

📄 Description

What broke: Stores into one local object at unknown keys (t[k] = {…}) made ambient-write indexing cubic. The query that finds which objects a store mutates also recorded the value already in the written slot as a mutated object. A 4 KB file took 12 s and an 8.8 KB file 97 s.
Fix: That query now drops items whose projection no longer ends at the written key. Selecting a stored value has one shared owner, and the store fixpoint re-queues a dependent only when a changed entry can still reach it.
Risk: Ambient write/escape facts shrink, but only the spurious ones derived from replaced slot values, which is checked fact by fact below. This does not make the reporter's castle bundle finish; a second, separate cost remains (see "Remaining").

Refs #6311. I used Refs rather than Fixes because the @castleio/castle-js bundle still times out after this change.

Summary

indexJavaScriptAmbientWritesSteps resolves each reference store (x.y = v) to the local objects it mutates and records those as stores entries. It did this by walking the written target t[k] as a value. Once a step consumed the written key, the walk continued into whatever was already in the slot (another stored value, an object-literal property, a destructured source) and recorded it as mutated at path [].

Two problems followed:

Wrong facts. Reads of those values then followed into every later store. For example, var g={}; t[k0]=g; t[k1]=window; g.fetch=x reported a write to globalThis.fetch.
Cubic cost. Every value stored at unknown keys received an entry for every such store, and each new store re-queued all earlier stores.

Problem and expected behavior

Smallest trigger: n statements of the form t[k[i]] = {v:i} on one local t.
Violated invariant, already documented on collectJavaScriptIdentityWritesSteps: resolve the complete object, then append the written key. Replacing a property is a distinct write. Overwriting a slot does not mutate the value it held.

Change and scope

createAmbientPathResolver: the optional callback is renamed receiver. When it is present, queue drops any item whose projection is exactly [], meaning a value read from the written slot. Items with a nonempty or widened projection still follow every existing step, including __proto__→prototype, constructor, prototype targets, rest copies, returned references and stored values.
storedValueProjection is now the single owner of "which stored value does a projection reach". The resolver and the fixpoint both use it.
The fixpoint records the projection each dependent reached and re-queues it only when a changed entry yields a selection its receiver query keeps. On the full corpus below, this step on its own changed no facts.
Not included: the remaining castle cost described under "Remaining". It needs a representation change for widened (cyclic) projections. Scope decision: not applicable, since this is a fix within an existing analysis boundary.

Contract and boundary impact

Semantic owner and earliest changed stage: the ambient-write index (javascriptSemanticAmbientWrites.ts). Its consumers are the effects, coverage and escaping-store facts on the semantic state, read by static analysis, semantic analysis and web-bundle inspection.
Contribution scope fit and linked maintainer decision: not applicable (bug fix).
CLI and MCP/tool-catalog contract: unchanged.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: no schema change. Spurious ambient writes and escapes derived from replaced slot values are no longer reported. The one consumer-visible case found is covered by a test: a new URL(..., import.meta.url) base stays proven when only a local copy of a replaced slot is written.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.

Evidence and regression coverage

Tests added in src/domain/javascript/javascriptSemanticAmbientWrites.test.ts. On the base revision, 4 of them fail and 22 pass after the change:

Replaced-slot cases that must report nothing: a stored object, and a literal property value.
Cases that must still report, and still do: a global read back through an unknown slot, an alias of that slot, nested stores through a slot, x.__proto__.slot, x.constructor.slot, and a rest copy.
A consumer-level new Worker(new URL("./worker.js", import.meta.url)) case.
A deterministic work guard. It counts reads of the stored values' own syntax during indexing, which gives n=32: 352, n=64: 704, n=128: 1408 (base: 659,824 / 4,998,880 / 38,870,464), and asserts that work(64) < 3 × work(32). It does not measure wall time.

Differential soundness check (executed; harness and corpus kept outside the repo):

Corpus: 4,140 JavaScript snippets extracted from the repo's JavaScript test files; 16 real bundles (pako, algoliasearch, diff, qs, js-yaml ×2, handlebars, vue runtime, jsonpath-plus, tinybench, listr2, and @castleio/castle-js 2.5.3 / 2.6.2 / 2.7.1 / 2.8.1 / esm-2.8.1); and 24 synthetic files (8 shapes at n = 10/40/100).
Dumped for each file: ambient write effects, escape effects, escaping reference stores, retained global call receivers, wildcard coverage and resource limits, plus hashes of the full static-analysis facts and the semantic module projection.
Static facts and semantic projection: unchanged for all 4,180 files.
Ambient facts: 635 removed, 0 added. One snippet, 8 real bundles and the 3 synthetic files of one shape are affected.
Every removal is attributed by a taint-labelled copy of the base resolver. It marks items read from a written slot, and everything derived from them, including widening those items cause.
- The taint run reproduces the base facts exactly for all 4,180 files.
- All 635 removed facts are labelled as slot-occupant-derived (snippets 1/1, real and synthetic 634/634).
- The 12,080 facts the change keeps are exactly the untainted base facts.
Examples of removed facts:
- qs: writes to its local INTRINSICS table reported as writes to globalThis.Array, AggregateError, and so on (the constructors stored in its slots).
- castle: writes reported on globalThis.Infinity and globalThis.undefined.
- js-yaml, vue, jsonpath-plus, handlebars: F.prototype = … stores reported as escaping because F.prototype.constructor = F read F back from the slot.

Remaining proof gaps:

The taint labels come from an instrumented copy of the base build, not from a formal proof.
The corpus covers browser and npm bundles, not Electron apps.

Validation performed

Base commit bc143125e, macOS arm64, Node 26.5.0 (below the engines range; installed with npm_config_engine_strict=false). Benchmarks went through the public CLI, analyze-javascript-application <dir> --format json --max-heap-mb 2048 --analysis-timeout-ms 900000 (castle before: 60000). Each figure is one cold process run.

"Static" is the time between the worker's static and semantic progress events.
CPU is user time from /usr/bin/time -l, which includes reaped workers.
RSS is the largest single process in the tree, not the family total.

input	static before → after	wall before → after	CPU (user) before → after	peak RSS before → after
synthetic n=125 (2.1 KB)	641 ms → 45 ms	2.97 s → 1.68 s	3.21 s → 1.49 s	351 → 347 MB
synthetic n=250 (4.3 KB)	5.20 s → 66 ms	12.05 s → 1.66 s	14.27 s → 1.64 s	367 → 355 MB
synthetic n=500 (8.8 KB)	48.4 s → 111 ms	96.66 s → 1.90 s	115.24 s → 2.09 s	535 → 369 MB
synthetic n=1000 (18 KB)	323.6 s → 166 ms	617.68 s → 2.30 s	731.58 s → 2.61 s	1078 → 388 MB
castle 2.8.2 ESM (664,054 B)	timeout → timeout	62.35 s (60 s deadline) → 901.38 s (900 s deadline)	64.62 s → 969.91 s	744 → 1652 MB

In-process indexing time on the real bundles was unchanged or lower. For example, vue runtime went from 378 ms to 193 ms for indexing and from 1,049 ms to 897 ms for the static phase.

Commands run:

npm run test:local -- <198 JavaScript domain/application/worker/web-bundle test files> — 198 files, 4,922 tests passed.
npm run test:local -- src/domain/javascript/javascriptSemanticAmbientWrites.test.ts — 22 passed. With the source change reverted, 4 failed.
npm run check:fast — passed.
npm run format:check — passed.

Remaining (not fixed here)

Castle 2.8.2 still reaches the 900 s deadline in the static phase. I profiled the fixed build:

Nearly all resolver work (148.25M of 148.5M worklist pops by fixpoint iteration 4,000) is on widened (undefined, any depth) projections.
The widening comes from cyclic nesting of local register arrays ([] reached at [null,null] and then [null,null,null]). This is legitimate widening, not slot-occupant aliasing.
Every store that reaches that region walks its whole closure again (about 190k pops per walk late in the fixpoint) and publishes an any-depth entry onto every fresh object in it.

Fixing this needs a shared representation for widened regions (for example, one summary per region instead of per-store entries). I'll report it on #6311 rather than widen this PR.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable. Static analysis only, run on macOS.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new process, file or network behavior.

Author readiness

Human owner: @kaoru0822-kitauji

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.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 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/7128 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/25/2026 **Status:** ✅ Merged **Merged:** 10/25/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6311-ambient-store-identities` --- ### 📝 Commits (1) - [`7fa53f8`](https://github.com/morluto/rea/commit/7fa53f881617508415a840dc7b7a4aaac6cebb11) fix(javascript): stop recording replaced slot values as store receivers ### 📊 Changes **2 files changed** (+189 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSemanticAmbientWrites.test.ts` (+112 -0) 📝 `src/domain/javascript/javascriptSemanticAmbientWrites.ts` (+77 -49) </details> ### 📄 Description **What broke:** Stores into one local object at unknown keys (`t[k] = {…}`) made ambient-write indexing cubic. The query that finds which objects a store mutates also recorded the value already in the written slot as a mutated object. A 4 KB file took 12 s and an 8.8 KB file 97 s. **Fix:** That query now drops items whose projection no longer ends at the written key. Selecting a stored value has one shared owner, and the store fixpoint re-queues a dependent only when a changed entry can still reach it. **Risk:** Ambient write/escape facts shrink, but only the spurious ones derived from replaced slot values, which is checked fact by fact below. This does not make the reporter's castle bundle finish; a second, separate cost remains (see "Remaining"). Refs #6311. I used Refs rather than Fixes because the `@castleio/castle-js` bundle still times out after this change. ## Summary `indexJavaScriptAmbientWritesSteps` resolves each reference store (`x.y = v`) to the local objects it mutates and records those as `stores` entries. It did this by walking the written target `t[k]` as a value. Once a step consumed the written key, the walk continued into whatever was already in the slot (another stored value, an object-literal property, a destructured source) and recorded it as mutated at path `[]`. Two problems followed: - **Wrong facts.** Reads of those values then followed into every later store. For example, `var g={}; t[k0]=g; t[k1]=window; g.fetch=x` reported a write to `globalThis.fetch`. - **Cubic cost.** Every value stored at unknown keys received an entry for every such store, and each new store re-queued all earlier stores. ## Problem and expected behavior - Smallest trigger: n statements of the form `t[k[i]] = {v:i}` on one local `t`. - Violated invariant, already documented on `collectJavaScriptIdentityWritesSteps`: resolve the complete object, then append the written key. Replacing a property is a distinct write. Overwriting a slot does not mutate the value it held. ## Change and scope - `createAmbientPathResolver`: the optional callback is renamed `receiver`. When it is present, `queue` drops any item whose projection is exactly `[]`, meaning a value read from the written slot. Items with a nonempty or widened projection still follow every existing step, including `__proto__`→`prototype`, `constructor`, prototype targets, rest copies, returned references and stored values. - `storedValueProjection` is now the single owner of "which stored value does a projection reach". The resolver and the fixpoint both use it. - The fixpoint records the projection each dependent reached and re-queues it only when a changed entry yields a selection its receiver query keeps. On the full corpus below, this step on its own changed no facts. - Not included: the remaining castle cost described under "Remaining". It needs a representation change for widened (cyclic) projections. Scope decision: not applicable, since this is a fix within an existing analysis boundary. ## Contract and boundary impact - Semantic owner and earliest changed stage: the ambient-write index (`javascriptSemanticAmbientWrites.ts`). Its consumers are the effects, coverage and escaping-store facts on the semantic state, read by static analysis, semantic analysis and web-bundle inspection. - Contribution scope fit and linked maintainer decision: not applicable (bug fix). - CLI and MCP/tool-catalog contract: unchanged. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: no schema change. Spurious ambient writes and escapes derived from replaced slot values are no longer reported. The one consumer-visible case found is covered by a test: a `new URL(..., import.meta.url)` base stays proven when only a local copy of a replaced slot is written. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage **Tests added** in `src/domain/javascript/javascriptSemanticAmbientWrites.test.ts`. On the base revision, 4 of them fail and 22 pass after the change: - Replaced-slot cases that must report nothing: a stored object, and a literal property value. - Cases that must still report, and still do: a global read back through an unknown slot, an alias of that slot, nested stores through a slot, `x.__proto__.slot`, `x.constructor.slot`, and a rest copy. - A consumer-level `new Worker(new URL("./worker.js", import.meta.url))` case. - A deterministic work guard. It counts reads of the stored values' own syntax during indexing, which gives n=32: 352, n=64: 704, n=128: 1408 (base: 659,824 / 4,998,880 / 38,870,464), and asserts that work(64) < 3 × work(32). It does not measure wall time. **Differential soundness check** (executed; harness and corpus kept outside the repo): - Corpus: 4,140 JavaScript snippets extracted from the repo's JavaScript test files; 16 real bundles (pako, algoliasearch, diff, qs, js-yaml ×2, handlebars, vue runtime, jsonpath-plus, tinybench, listr2, and `@castleio/castle-js` 2.5.3 / 2.6.2 / 2.7.1 / 2.8.1 / esm-2.8.1); and 24 synthetic files (8 shapes at n = 10/40/100). - Dumped for each file: ambient write effects, escape effects, escaping reference stores, retained global call receivers, wildcard coverage and resource limits, plus hashes of the full static-analysis facts and the semantic module projection. - Static facts and semantic projection: unchanged for all 4,180 files. - Ambient facts: 635 removed, 0 added. One snippet, 8 real bundles and the 3 synthetic files of one shape are affected. - Every removal is attributed by a taint-labelled copy of the base resolver. It marks items read from a written slot, and everything derived from them, including widening those items cause. - The taint run reproduces the base facts exactly for all 4,180 files. - All 635 removed facts are labelled as slot-occupant-derived (snippets 1/1, real and synthetic 634/634). - The 12,080 facts the change keeps are exactly the untainted base facts. - Examples of removed facts: - qs: writes to its local `INTRINSICS` table reported as writes to `globalThis.Array`, `AggregateError`, and so on (the constructors stored in its slots). - castle: writes reported on `globalThis.Infinity` and `globalThis.undefined`. - js-yaml, vue, jsonpath-plus, handlebars: `F.prototype = …` stores reported as escaping because `F.prototype.constructor = F` read `F` back from the slot. **Remaining proof gaps:** - The taint labels come from an instrumented copy of the base build, not from a formal proof. - The corpus covers browser and npm bundles, not Electron apps. ## Validation performed Base commit `bc143125e`, macOS arm64, Node 26.5.0 (below the engines range; installed with `npm_config_engine_strict=false`). Benchmarks went through the public CLI, `analyze-javascript-application <dir> --format json --max-heap-mb 2048 --analysis-timeout-ms 900000` (castle before: 60000). Each figure is one cold process run. - "Static" is the time between the worker's `static` and `semantic` progress events. - CPU is user time from `/usr/bin/time -l`, which includes reaped workers. - RSS is the largest single process in the tree, not the family total. | input | static before → after | wall before → after | CPU (user) before → after | peak RSS before → after | | --- | --- | --- | --- | --- | | synthetic n=125 (2.1 KB) | 641 ms → 45 ms | 2.97 s → 1.68 s | 3.21 s → 1.49 s | 351 → 347 MB | | synthetic n=250 (4.3 KB) | 5.20 s → 66 ms | 12.05 s → 1.66 s | 14.27 s → 1.64 s | 367 → 355 MB | | synthetic n=500 (8.8 KB) | 48.4 s → 111 ms | 96.66 s → 1.90 s | 115.24 s → 2.09 s | 535 → 369 MB | | synthetic n=1000 (18 KB) | 323.6 s → 166 ms | 617.68 s → 2.30 s | 731.58 s → 2.61 s | 1078 → 388 MB | | castle 2.8.2 ESM (664,054 B) | timeout → timeout | 62.35 s (60 s deadline) → 901.38 s (900 s deadline) | 64.62 s → 969.91 s | 744 → 1652 MB | In-process indexing time on the real bundles was unchanged or lower. For example, vue runtime went from 378 ms to 193 ms for indexing and from 1,049 ms to 897 ms for the static phase. Commands run: - `npm run test:local -- <198 JavaScript domain/application/worker/web-bundle test files>` — 198 files, 4,922 tests passed. - `npm run test:local -- src/domain/javascript/javascriptSemanticAmbientWrites.test.ts` — 22 passed. With the source change reverted, 4 failed. - `npm run check:fast` — passed. - `npm run format:check` — passed. ## Remaining (not fixed here) Castle 2.8.2 still reaches the 900 s deadline in the static phase. I profiled the fixed build: - Nearly all resolver work (148.25M of 148.5M worklist pops by fixpoint iteration 4,000) is on widened (`undefined`, any depth) projections. - The widening comes from cyclic nesting of local register arrays (`[]` reached at `[null,null]` and then `[null,null,null]`). This is legitimate widening, not slot-occupant aliasing. - Every store that reaches that region walks its whole closure again (about 190k pops per walk late in the fixpoint) and publishes an any-depth entry onto every fresh object in it. Fixing this needs a shared representation for widened regions (for example, one summary per region instead of per-store entries). I'll report it on #6311 rather than widen this PR. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. Static analysis only, run on macOS. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process, file or network behavior. ## Author readiness Human owner: @kaoru0822-kitauji - [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. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-03 13:39:27 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns