[PR #3638] [MERGED] fix(javascript): exclude class member keys from lexical reads #5860

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3638
Author: @kshitizj03
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/17/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/3310-class-key-reads


📝 Commits (7)

  • 38c4fe9 fix(javascript): exclude class member keys from lexical reads
  • c8723f8 test(javascript): cover TypeScript class member keys
  • 2a10734 Merge branch 'main' into fix/3310-class-key-reads
  • ec25d78 Merge branch 'main' into fix/3310-class-key-reads
  • 95da7ad Merge branch 'main' into fix/3310-class-key-reads
  • f806944 Merge branch 'main' into fix/3310-class-key-reads
  • 9d2e1d5 Merge branch 'main' into fix/3310-class-key-reads

📊 Changes

2 files changed (+81 additions, -0 deletions)

View changed files

📝 src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts (+68 -0)
📝 src/domain/javascript/javascriptSemanticProjection.ts (+13 -0)

📄 Description

Summary

Fixes #3310.

A noncomputed class member name is a property key, but JavaScript semantic analysis treated it as a lexical read. const marker = "seed"; class Holder { marker() {} } recorded a resolved reads relation from the outer marker binding, so trace-javascript-semantics reported forward influence into a class whose key never evaluates that binding.

Problem and expected behavior

isNonReferenceKey (src/domain/javascript/javascriptSemanticProjection.ts) is the shared owner that decides whether an identifier occurrence is a lexical reference. It excluded noncomputed ObjectProperty/ObjectMethod keys but had no case for class members. Reproduced on main at module level (analyzeJavaScriptSemantics(...).references):

Input marker reads before After
class H { marker() {} } 1 resolved 0
static marker() {}, get/set marker, marker = 1, static marker = 2 1 resolved each 0
#marker = 3, this.#marker 1 resolved each 0
class H { [marker]() {} } (computed control) 1 resolved 1 resolved (unchanged)
field = marker (initializer control) 1 resolved 1 resolved (unchanged)
{ marker() {} } (object control) 0 0 (unchanged)

The issue reports methods. The same gap also covers static members, getters/setters, class fields, TypeScript declare fields, abstract methods and overload signatures, and private names. A PrivateName identifier can never denote a lexical binding.

There is a second symptom, a false closure capture. Inside a function, function outer() { const marker = 1; class Holder { marker() {} } } recorded closureCaptures: [{ owner: "method", binding: "marker" }]. The method never reads marker; only its name matches. After the fix, closureCaptures is empty.

Change and scope

The fix extends isNonReferenceKey with a isNoncomputedClassMemberKey predicate (ClassMethod, ClassProperty, TSDeclareMethod where key === node && !computed) and a PrivateName parent check. All consumers route through this predicate (collectSemanticReferencesSteps, semanticReferenceRole, closure-capture detection in javascriptSemanticCalls.ts, and reference-source require bindings via analyzeParsedJavaScriptReferences). JavaScriptSemanticGraphBuilder projects each read reference into a resolved reads relation, so references, semantic-graph relations, traces and captures stay consistent without downstream repairs. Computed keys are unaffected; their enclosing-callable ownership was handled in 76fa5304a (#2573).

ClassAccessorProperty is intentionally not listed: REA's parser rejects accessor fields (Babel requires the decoratorAutoAccessors plugin, which no REA parser configuration enables), so that node cannot reach semantic analysis.

Babel's t.isReferenced was considered and not used. It classifies assignment targets (x = 1) as non-references, but REA records them as write (and compound assignments as read+write), so it is not a drop-in replacement for this predicate.

Not included: other name-only positions outside classes that still produce resolved reads on main (TS interface/type-literal member keys, enum members, import.meta, new.target, import attribute keys). They are distinct from #3310 and left for a separate change.

Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: isNonReferenceKey in javascriptSemanticProjection.ts (reference classification).

  • Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing JavaScript analysis).

  • CLI and MCP/tool-catalog contract: none; schemas unchanged. Fewer false reads relations in semantic Evidence.

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: removes false resolved reads relations; no new fields.

  • Target selection, authority, upstream behavior, and cleanup details: not applicable.

  • Generated metadata, package, or installation impact: none.

  • 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

  • Tests added or updated: javascriptSemanticAnalysis.structure.test.ts, "class member keys": (1) every noncomputed class member and private-name form yields no marker references; (2) TypeScript declare fields, abstract methods and overload signatures yield no reads, while a computed abstract key keeps its one read; (3) a noncomputed method name creates no closure capture; (4) computed key and field initializer controls keep their resolved reads.

  • Base reproduction or other evidence: on the base revision, tests (1)-(3) fail (8 false reads; 4 false TypeScript-member reads; one false method capture) and test (4) passes before and after. Optional methods and #x in o were also probed: 0 false reads after the fix. Decorator arguments, static blocks and constructor parameter defaults keep their reads.

  • User-visible CLI/MCP output (if applicable): the public semantic graph, built with the graphForJavaScript test fixture (same projection the CLI returns), for the issue's two-line sample:

    Sample Base reads from marker Fix
    class Holder { marker() {} } 1 resolved at line 2, column 15 0
    class Holder { [marker]() {} } 1 resolved at line 2, column 16 1 resolved at line 2, column 16

    These match the locations reported in #3310.

  • Remaining proof gaps: I validated on Windows 11 from a source checkout, where every analyze-javascript-application CLI run is rejected by #3501 (missing native/windows/build/manifest.json). The issue's end-to-end CLI analyze-javascript-application / trace-javascript-semantics steps were not executed; coverage is the semantic IR plus the public semantic-graph projection that the trace consumes.

For evidence-bearing changes:

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

Validation performed

  • npx vitest run src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts -t "class member keys" (base code, new tests): 3 failed, 1 passed; (fixed code): 4 passed
  • npx vitest run src/domain/javascript src/application/javascript: 79 files, 1982 tests passed
  • npx vitest run src on base (efbc486b, separate worktree) vs this branch: 7173 vs 7177 tests; the same 191 tests fail on both (identical failure sets: IDA/Hopper/Ghidra/WABT/adb host-dependent tests on Windows); the 4 added tests pass
  • npm run check:fast (pre-push gate): passed
  • npm run lint: passed (no new warnings in changed files)
  • npm run verify:test-discovery: all 1290 test files discovered exactly once
  • npx tsc --noEmit: passed
  • npx oxfmt --check (changed files): passed
  • npx oxlint (changed files): no new warnings (pre-existing max-lines on javascriptSemanticProjection.ts only)
  • npm run verify:module-boundaries: passed

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis); validated on Windows 11, Node 24.14.0.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: none; parse-only change.

Maintainer review update — Linux (17 October 2026)

Reviewed head: ec25d78d4d0ae168999265b85106affa88a5dd63, merged with main 150e996a86f2bec4982ba87f37bf502a8d5e8214. Its whole tree matches the independently computed integration tree; its stable patch ID matches the previously reviewed change, and it does not modify workflows.

184 semantic structure/call/dataflow tests pass. The previously timed-out CLI missing-argument formatter case passes locally at its unchanged 60-second limit (1 selected test, 19 unselected); this does not establish the old timeout's cause. The exact hosted case also passes in run 38995296245: 3,247.46 ms, zero retries, as verified in the structured vitest-report-built-4/blob-built-5-5.json report. All five built-dependent CI shards pass. Compiled CLI/MCP analysis and tracing pass: class member keys are excluded while initializer/computed-key reads, Evidence identity and partial coverage remain intact. The fixture is inert source, not an executed application.

Historical baseline: the former firmware and three async BinarySessionStatus failures were independently reproduced earlier; both are now repaired upstream. The current-head baseline is recorded below.

Local jobs ran serially on two CPU cores with a 3 GiB Node heap; maximum sampled job-family RSS across this retry was 3.28 GiB, with no resource stop. This is not a measurement of the assistant/shared host total. Model token use, controlled cold/warm comparison and remote resource costs are unmeasured.

Baseline follow-up (2026-10-17)

Current head 9d2e1d5df1bbbc3d75a52f6e1f52772d56a44d5f incorporates main 5b243993872c2bcc70cb00060ffb79b347321370, including the normally merged #4691/#4703 fixture repairs. Exact merge-tree verification and stable patch IDs confirm that this PR's patch is unchanged by the update. Current-head CI: All five built-dependent shards, Source/static checks and required gates passed.

Fresh exact-head production build and public static JavaScript workflow pass through 2 CLI and 2 MCP calls, covering noncomputed/computed class keys, field initializers, method-body reads, matching Evidence and explicit coverage/partial facts. Input source remains unchanged. Completed task: 9.44 s wall, 11.87 s reaped CPU, 1.09 GiB sampled process-family RSS.

Local heavy jobs ran serially on two CPU cores with a 3 GiB Node heap. RSS is sampled process-family memory, may double-count shared pages and excludes assistant/shared services; CPU covers reaped descendants. OS/dependency caches were not controlled. Model token use, controlled cold/warm costs and remote resource costs remain unmeasured; no token or speedup claim is inferred. Prior observations above remain tied to their stated revisions. Initial human-owner readiness remains valid and was not supplied on the author's behalf. Available automated reviews have no unresolved findings; Codex explicitly reports repository review-credit exhaustion, so no fresh Codex review is claimed.

Author readiness

Human owner: @kshitizj03

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

🔄 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/3638 **Author:** [@kshitizj03](https://github.com/kshitizj03) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/17/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/3310-class-key-reads` --- ### 📝 Commits (7) - [`38c4fe9`](https://github.com/morluto/rea/commit/38c4fe99b29c35d962ef9ae71edb3254eb6620f7) fix(javascript): exclude class member keys from lexical reads - [`c8723f8`](https://github.com/morluto/rea/commit/c8723f8a0000c7f330dfdddd9566d98ed737f0a5) test(javascript): cover TypeScript class member keys - [`2a10734`](https://github.com/morluto/rea/commit/2a1073485783898f224ab6c62a56d9e337bb73dd) Merge branch 'main' into fix/3310-class-key-reads - [`ec25d78`](https://github.com/morluto/rea/commit/ec25d78d4d0ae168999265b85106affa88a5dd63) Merge branch 'main' into fix/3310-class-key-reads - [`95da7ad`](https://github.com/morluto/rea/commit/95da7adbf3e40131ec5e349899e74f762181c935) Merge branch 'main' into fix/3310-class-key-reads - [`f806944`](https://github.com/morluto/rea/commit/f80694464c186a41a1a4c660c400977934bd4fad) Merge branch 'main' into fix/3310-class-key-reads - [`9d2e1d5`](https://github.com/morluto/rea/commit/9d2e1d5df1bbbc3d75a52f6e1f52772d56a44d5f) Merge branch 'main' into fix/3310-class-key-reads ### 📊 Changes **2 files changed** (+81 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts` (+68 -0) 📝 `src/domain/javascript/javascriptSemanticProjection.ts` (+13 -0) </details> ### 📄 Description ## Summary Fixes #3310. A noncomputed class member name is a property key, but JavaScript semantic analysis treated it as a lexical read. `const marker = "seed"; class Holder { marker() {} }` recorded a resolved `reads` relation from the outer `marker` binding, so `trace-javascript-semantics` reported forward influence into a class whose key never evaluates that binding. ## Problem and expected behavior `isNonReferenceKey` (`src/domain/javascript/javascriptSemanticProjection.ts`) is the shared owner that decides whether an identifier occurrence is a lexical reference. It excluded noncomputed `ObjectProperty`/`ObjectMethod` keys but had no case for class members. Reproduced on `main` at module level (`analyzeJavaScriptSemantics(...).references`): | Input | `marker` reads before | After | |---|---|---| | `class H { marker() {} }` | 1 resolved | 0 | | `static marker() {}`, `get`/`set marker`, `marker = 1`, `static marker = 2` | 1 resolved each | 0 | | `#marker = 3`, `this.#marker` | 1 resolved each | 0 | | `class H { [marker]() {} }` (computed control) | 1 resolved | 1 resolved (unchanged) | | `field = marker` (initializer control) | 1 resolved | 1 resolved (unchanged) | | `{ marker() {} }` (object control) | 0 | 0 (unchanged) | The issue reports methods. The same gap also covers static members, getters/setters, class fields, TypeScript `declare` fields, abstract methods and overload signatures, and private names. A `PrivateName` identifier can never denote a lexical binding. There is a second symptom, a false closure capture. Inside a function, `function outer() { const marker = 1; class Holder { marker() {} } }` recorded `closureCaptures: [{ owner: "method", binding: "marker" }]`. The method never reads `marker`; only its name matches. After the fix, `closureCaptures` is empty. ## Change and scope The fix extends `isNonReferenceKey` with a `isNoncomputedClassMemberKey` predicate (`ClassMethod`, `ClassProperty`, `TSDeclareMethod` where `key === node && !computed`) and a `PrivateName` parent check. All consumers route through this predicate (`collectSemanticReferencesSteps`, `semanticReferenceRole`, closure-capture detection in `javascriptSemanticCalls.ts`, and reference-source require bindings via `analyzeParsedJavaScriptReferences`). `JavaScriptSemanticGraphBuilder` projects each `read` reference into a resolved `reads` relation, so references, semantic-graph relations, traces and captures stay consistent without downstream repairs. Computed keys are unaffected; their enclosing-callable ownership was handled in 76fa5304a (#2573). `ClassAccessorProperty` is intentionally not listed: REA's parser rejects `accessor` fields (Babel requires the `decoratorAutoAccessors` plugin, which no REA parser configuration enables), so that node cannot reach semantic analysis. Babel's `t.isReferenced` was considered and not used. It classifies assignment targets (`x = 1`) as non-references, but REA records them as `write` (and compound assignments as read+write), so it is not a drop-in replacement for this predicate. Not included: other name-only positions outside classes that still produce resolved reads on `main` (TS interface/type-literal member keys, enum members, `import.meta`, `new.target`, import attribute keys). They are distinct from #3310 and left for a separate change. Contribution scope: ordinary fix within existing boundaries; maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `isNonReferenceKey` in `javascriptSemanticProjection.ts` (reference classification). - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing JavaScript analysis). - CLI and MCP/tool-catalog contract: none; schemas unchanged. Fewer false `reads` relations in semantic Evidence. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: removes false resolved `reads` relations; no new fields. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [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 - Tests added or updated: `javascriptSemanticAnalysis.structure.test.ts`, "class member keys": (1) every noncomputed class member and private-name form yields no `marker` references; (2) TypeScript `declare` fields, abstract methods and overload signatures yield no reads, while a computed abstract key keeps its one read; (3) a noncomputed method name creates no closure capture; (4) computed key and field initializer controls keep their resolved reads. - Base reproduction or other evidence: on the base revision, tests (1)-(3) fail (8 false reads; 4 false TypeScript-member reads; one false method capture) and test (4) passes before and after. Optional methods and `#x in o` were also probed: 0 false reads after the fix. Decorator arguments, static blocks and constructor parameter defaults keep their reads. - User-visible CLI/MCP output (if applicable): the public semantic graph, built with the `graphForJavaScript` test fixture (same projection the CLI returns), for the issue's two-line sample: | Sample | Base `reads` from `marker` | Fix | |---|---|---| | `class Holder { marker() {} }` | 1 resolved at line 2, column 15 | 0 | | `class Holder { [marker]() {} }` | 1 resolved at line 2, column 16 | 1 resolved at line 2, column 16 | These match the locations reported in #3310. - Remaining proof gaps: I validated on Windows 11 from a source checkout, where every `analyze-javascript-application` CLI run is rejected by #3501 (missing `native/windows/build/manifest.json`). The issue's end-to-end CLI `analyze-javascript-application` / `trace-javascript-semantics` steps were not executed; coverage is the semantic IR plus the public semantic-graph projection that the trace consumes. For evidence-bearing changes: - [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. ## Validation performed - `npx vitest run src/domain/javascript/javascriptSemanticAnalysis.structure.test.ts -t "class member keys"` (base code, new tests): 3 failed, 1 passed; (fixed code): 4 passed - `npx vitest run src/domain/javascript src/application/javascript`: 79 files, 1982 tests passed - `npx vitest run src` on base (efbc486b, separate worktree) vs this branch: 7173 vs 7177 tests; the same 191 tests fail on both (identical failure sets: IDA/Hopper/Ghidra/WABT/adb host-dependent tests on Windows); the 4 added tests pass - `npm run check:fast` (pre-push gate): passed - `npm run lint`: passed (no new warnings in changed files) - `npm run verify:test-discovery`: all 1290 test files discovered exactly once - `npx tsc --noEmit`: passed - `npx oxfmt --check` (changed files): passed - `npx oxlint` (changed files): no new warnings (pre-existing `max-lines` on `javascriptSemanticProjection.ts` only) - `npm run verify:module-boundaries`: passed ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis); validated on Windows 11, Node 24.14.0. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; parse-only change. ### Maintainer review update — Linux (17 October 2026) Reviewed head: `ec25d78d4d0ae168999265b85106affa88a5dd63`, merged with main `150e996a86f2bec4982ba87f37bf502a8d5e8214`. Its whole tree matches the independently computed integration tree; its stable patch ID matches the previously reviewed change, and it does not modify workflows. 184 semantic structure/call/dataflow tests pass. The previously timed-out CLI missing-argument formatter case passes locally at its unchanged 60-second limit (1 selected test, 19 unselected); this does not establish the old timeout's cause. The exact hosted case also passes in run 38995296245: 3,247.46 ms, zero retries, as verified in the structured vitest-report-built-4/blob-built-5-5.json report. All five built-dependent CI shards pass. Compiled CLI/MCP analysis and tracing pass: class member keys are excluded while initializer/computed-key reads, Evidence identity and partial coverage remain intact. The fixture is inert source, not an executed application. Historical baseline: the former firmware and three async BinarySessionStatus failures were independently reproduced earlier; both are now repaired upstream. The current-head baseline is recorded below. Local jobs ran serially on two CPU cores with a 3 GiB Node heap; maximum sampled job-family RSS across this retry was 3.28 GiB, with no resource stop. This is not a measurement of the assistant/shared host total. Model token use, controlled cold/warm comparison and remote resource costs are unmeasured. ## Baseline follow-up (2026-10-17) Current head `9d2e1d5df1bbbc3d75a52f6e1f52772d56a44d5f` incorporates main `5b243993872c2bcc70cb00060ffb79b347321370`, including the normally merged #4691/#4703 fixture repairs. Exact merge-tree verification and stable patch IDs confirm that this PR's patch is unchanged by the update. [Current-head CI](https://github.com/morluto/rea/actions/runs/39011246826): All five built-dependent shards, Source/static checks and required gates passed. Fresh exact-head production build and public static JavaScript workflow pass through 2 CLI and 2 MCP calls, covering noncomputed/computed class keys, field initializers, method-body reads, matching Evidence and explicit coverage/partial facts. Input source remains unchanged. Completed task: 9.44 s wall, 11.87 s reaped CPU, 1.09 GiB sampled process-family RSS. Local heavy jobs ran serially on two CPU cores with a 3 GiB Node heap. RSS is sampled process-family memory, may double-count shared pages and excludes assistant/shared services; CPU covers reaped descendants. OS/dependency caches were not controlled. Model token use, controlled cold/warm costs and remote resource costs remain unmeasured; no token or speedup claim is inferred. Prior observations above remain tied to their stated revisions. Initial human-owner readiness remains valid and was not supplied on the author's behalf. Available automated reviews have no unresolved findings; Codex explicitly reports repository review-credit exhaustion, so no fresh Codex review is claimed. ## Author readiness Human owner: @kshitizj03 - [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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:09:59 +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#5860
No description provided.