morluto/rea

Fork 0

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

[PR #2892] [CLOSED] fix(javascript): honor require bindings in static graphs #5475

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:06:56 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2892
Author: @JFSAS
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: codex/static-require-shadowing

📝 Commits (2)

c05f7eb fix(javascript): honor require bindings in static graphs
9f0cf34 fix(javascript): yield and order require classification

📊 Changes

9 files changed (+479 additions, -194 deletions)

View changed files

➕ src/domain/javascript/javascriptRequireBindings.ts (+294 -0)
➕ src/domain/javascript/javascriptStaticAnalysis.cooperative.test.ts (+43 -0)
📝 src/domain/javascript/javascriptStaticAnalysis.ts (+11 -1)
📝 src/domain/javascript/javascriptStaticAnalysisCalls.ts (+12 -3)
📝 src/domain/javascript/javascriptStaticAnalysisState.ts (+1 -0)
📝 src/domain/referenceSourceImportParsing.ts (+3 -3)
➖ src/domain/referenceSourceRequireBindings.ts (+0 -187)
➕ tests/acceptance/applications/shadowedRequireCli.test.ts (+108 -0)
📝 tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts (+7 -0)

📄 Description

Summary

Fixes #2890. Application analysis no longer invents a CommonJS file dependency when a bare require call resolves to a local binding. For example, function run(require) { return require("./ghost.cjs"); } previously created a high-confidence, resolved imports edge to ghost.cjs even when run received an ordinary local function.

Problem and expected behavior

The static-findings path recognized the callee name while the semantic and reference-source paths already had lexical binding information. Before this change, the public CLI and stdio MCP both return five false imports for parameter, optional-parameter, hoisted-function, catch and block cases in the regression fixture. Real Node runs of those self-authored cases print local without loading the throwing ghost.cjs control.

The corrected graph omits those false edges and retains ordinary and optional CommonJS loaders, wrapper var require, and direct createRequire(import.meta.url) calls. Calls before a later program-level reassignment remain visible; deferred calls and writes are not ordered by their textual offsets.

Change and scope

Move the existing reference-source loader classifier to the shared JavaScript domain and use it from both consumers. Static analysis reuses its binding index and yields through reference collection, reference indexing, loader inspection, and call selection before visiting static findings. isUnshadowedRequireCall only queries the completed set. The reference-source adapter synchronously drives the same stepped classifier; it retains its existing synchronous interface.

The shared scan records candidate calls and loader writes together. It preserves calls in earlier program statements instead of applying one file-wide “ever written” flag. Function bodies, instance initializers, unplaced writes, and same-statement ordering stay conservative; source position alone does not establish invocation order. Initialized wrapper var declarations and destructuring writes use the same ordering rule.

Bare require calls use this gate; recognized bundle-factory loaders keep their established path. Member-call heuristics are outside this correction. The old reference-specific module is removed, and the formerly separate top-level-var traversal is folded into the shared call/write scan.

This covers the cooperative classification, same responsiveness finding, and calls before reassignment review cases.

Ordinary static-analysis correctness fix. Maintainer scope decision: not applicable. This addresses a remaining application-graph consumer after the semantic-origin and reference-source fixes in #509 / #1657; it does not replace those fixes.

Contract and boundary impact

Semantic owner and earliest stage: shared loader-binding classification, before static module-reference findings become graph edges.
Contribution scope: existing inert JavaScript analysis.
CLI/MCP/tool catalog: no schema, tool-name or argument change.
Provider/platform compatibility: no new engine or host requirement.
Evidence: removes disproven dependency candidates; preserves source artifacts, ranges and real-loader relationships.
Target selection/authority/cleanup: no new target execution, resource acquisition or network behavior.
Generated metadata/package/installation: none.

Expected effects:

Read existing files or captured data.

Evidence and regression coverage

The acceptance regression exercises compiled CLI and stdio MCP, validates their result schema, and executes the self-authored cases with real Node as an independent binding control. It fails through both public interfaces on 3e7c6302 and passes after the fix. REA itself continues to inspect target code as data.

Positive controls retain all four genuine-loader forms. Existing reference-source, computed-require, dynamic-import, bundle module-index, Electron interop, semantic-link, source-location and large/deep-source cases also pass.

Observed, derived and inferred claims remain distinguishable.
Artifact identity and source provenance remain preserved.
Unsupported and uncertain outcomes retain their existing contracts.

Validation performed

macOS arm64; Node 24.18.0 / npm 11.16.0; updated 2026-10-14:

Original public shadowing regression on 3e7c6302: 2 failed (CLI and stdio MCP), exposing five false edges.
Review reproduction on c05f7eb: both public adapters omit the optional load before reassignment. The final cooperative regression also fails on that commit: one yielded step reads 5,000 distinct identifiers, exceeding the two-traversal-chunk bound.
Final 17-file regression selection: 192 passed. This includes public CLI/MCP shadowing and reassignment cases, real owned-analysis-worker tests, cancellation/cleanup checks, reference-source imports, bundle/Electron interop, semantic links and large/deep-source cases.
The public fixture runs real Node controls for loads before ordinary assignment, var initialization and destructuring, plus calls/writes in deferred functions and instance fields.
Final focused re-run after adding the reference-source before-write control: 2 passed (referenceSourceShadowedRequire.test.ts and javascriptStaticAnalysis.cooperative.test.ts).
npm run build:cached, npm run check:fast, npm run knip, npm run verify:architecture-guards, npm run verify:test-discovery: passed; 1,090 test files discovered exactly once. Changed-file formatting passes.
npm run check: still stops on the existing formatting failures in unchanged src/domain/functionComparison.test.ts and src/ida/IdaOperationRunner.ts. Separate typecheck/lint, knip and architecture checks complete successfully.

The deterministic responsiveness regression measures bounded identifier work at yielded traversal checkpoints rather than asserting a machine-dependent millisecond deadline. Existing scheduler cancellation and real worker lifecycle tests were also run. It does not establish a hard end-to-end cancellation bound.

Cost and scheduling check

Five serialized, alternating before/after runs compare c05f7eb with the review correction on the same 138,889-byte source containing 5,000 genuine loader calls. Each run uses a fresh Node process and the compiled production modules; both versions retain all 5,000 references.

Source-analysis measurement (median across five runs)	Before	After
Total stepped analysis wall time	109.40 ms	106.21 ms
Analysis CPU time	238.75 ms	238.51 ms
Longest nonterminal iterator step	20.24 ms	4.24 ms
Final iterator step	19.37 ms	17.11 ms
Process peak RSS	202,528 KiB	197,824 KiB

Wall/CPU intervals exclude import and parsing setup; RSS includes the entire process. These are small source-analysis samples, not agent-token measurements, an end-to-end speedup or a general performance guarantee. The final step includes pre-existing synchronous result/Electron projection outside this require-classification correction; it is shown separately and remains a responsiveness boundary. The deterministic test bounds intermediate traversal work, not that final projection.

Compatibility, safety, and release

No breaking schema or installation change. macOS public CLI/MCP and real Node controls were verified; Windows/Linux and native providers were not run. No Hopper/Ghidra/IDA behavior is claimed. Fixture execution is test-only and uses self-authored sources.

Before merge

Concrete reproduced problem and linked issue.
Scope and authority questions resolved for this ordinary fix.
One focused outcome and Conventional Commit title.
Regression plus positive and negative controls verified.
Shared interpretation owner reused; superseded module removed.
No public contract or generated metadata update required.
Public output changes and verification limits recorded.
Final diff checked for unrelated changes, secrets and unsupported claims.

Prepared with AI assistance; all reported executions were run locally.

_{🔄 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/2892 **Author:** [@JFSAS](https://github.com/JFSAS) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/static-require-shadowing` --- ### 📝 Commits (2) - [`c05f7eb`](https://github.com/morluto/rea/commit/c05f7eb723bceafd1bb3546f8b623195d6e872c3) fix(javascript): honor require bindings in static graphs - [`9f0cf34`](https://github.com/morluto/rea/commit/9f0cf34787a650bd54e6a74d0202d2b3418840c0) fix(javascript): yield and order require classification ### 📊 Changes **9 files changed** (+479 additions, -194 deletions) <details> <summary>View changed files</summary> ➕ `src/domain/javascript/javascriptRequireBindings.ts` (+294 -0) ➕ `src/domain/javascript/javascriptStaticAnalysis.cooperative.test.ts` (+43 -0) 📝 `src/domain/javascript/javascriptStaticAnalysis.ts` (+11 -1) 📝 `src/domain/javascript/javascriptStaticAnalysisCalls.ts` (+12 -3) 📝 `src/domain/javascript/javascriptStaticAnalysisState.ts` (+1 -0) 📝 `src/domain/referenceSourceImportParsing.ts` (+3 -3) ➖ `src/domain/referenceSourceRequireBindings.ts` (+0 -187) ➕ `tests/acceptance/applications/shadowedRequireCli.test.ts` (+108 -0) 📝 `tests/boundary/filesystem/referenceSourceShadowedRequire.test.ts` (+7 -0) </details> ### 📄 Description ## Summary Fixes #2890. Application analysis no longer invents a CommonJS file dependency when a bare `require` call resolves to a local binding. For example, `function run(require) { return require("./ghost.cjs"); }` previously created a high-confidence, resolved `imports` edge to `ghost.cjs` even when `run` received an ordinary local function. ## Problem and expected behavior The static-findings path recognized the callee name while the semantic and reference-source paths already had lexical binding information. Before this change, the public CLI and stdio MCP both return five false imports for parameter, optional-parameter, hoisted-function, catch and block cases in the regression fixture. Real Node runs of those self-authored cases print `local` without loading the throwing `ghost.cjs` control. The corrected graph omits those false edges and retains ordinary and optional CommonJS loaders, wrapper `var require`, and direct `createRequire(import.meta.url)` calls. Calls before a later program-level reassignment remain visible; deferred calls and writes are not ordered by their textual offsets. ## Change and scope Move the existing reference-source loader classifier to the shared JavaScript domain and use it from both consumers. Static analysis reuses its binding index and yields through reference collection, reference indexing, loader inspection, and call selection before visiting static findings. `isUnshadowedRequireCall` only queries the completed set. The reference-source adapter synchronously drives the same stepped classifier; it retains its existing synchronous interface. The shared scan records candidate calls and loader writes together. It preserves calls in earlier program statements instead of applying one file-wide “ever written” flag. Function bodies, instance initializers, unplaced writes, and same-statement ordering stay conservative; source position alone does not establish invocation order. Initialized wrapper `var` declarations and destructuring writes use the same ordering rule. Bare `require` calls use this gate; recognized bundle-factory loaders keep their established path. Member-call heuristics are outside this correction. The old reference-specific module is removed, and the formerly separate top-level-var traversal is folded into the shared call/write scan. This covers the [cooperative classification](https://github.com/morluto/rea/pull/2892#discussion_r4260936331), [same responsiveness finding](https://github.com/morluto/rea/pull/2892#discussion_r4260977959), and [calls before reassignment](https://github.com/morluto/rea/pull/2892#discussion_r4260977946) review cases. Ordinary static-analysis correctness fix. Maintainer scope decision: not applicable. This addresses a remaining application-graph consumer after the semantic-origin and reference-source fixes in #509 / #1657; it does not replace those fixes. ## Contract and boundary impact - Semantic owner and earliest stage: shared loader-binding classification, before static module-reference findings become graph edges. - Contribution scope: existing inert JavaScript analysis. - CLI/MCP/tool catalog: no schema, tool-name or argument change. - Provider/platform compatibility: no new engine or host requirement. - Evidence: removes disproven dependency candidates; preserves source artifacts, ranges and real-loader relationships. - Target selection/authority/cleanup: no new target execution, resource acquisition or network behavior. - Generated metadata/package/installation: none. Expected effects: - [x] Read existing files or captured data. ## Evidence and regression coverage The acceptance regression exercises **compiled CLI and stdio MCP**, validates their result schema, and executes the self-authored cases with real Node as an independent binding control. It fails through both public interfaces on `3e7c6302` and passes after the fix. REA itself continues to inspect target code as data. Positive controls retain all four genuine-loader forms. Existing reference-source, computed-require, dynamic-import, bundle module-index, Electron interop, semantic-link, source-location and large/deep-source cases also pass. - [x] Observed, derived and inferred claims remain distinguishable. - [x] Artifact identity and source provenance remain preserved. - [x] Unsupported and uncertain outcomes retain their existing contracts. ## Validation performed macOS arm64; Node 24.18.0 / npm 11.16.0; updated 2026-10-14: - Original public shadowing regression on `3e7c6302`: **2 failed** (CLI and stdio MCP), exposing five false edges. - Review reproduction on `c05f7eb`: both public adapters omit the optional load before reassignment. The final cooperative regression also fails on that commit: one yielded step reads 5,000 distinct identifiers, exceeding the two-traversal-chunk bound. - Final 17-file regression selection: **192 passed**. This includes public CLI/MCP shadowing and reassignment cases, real owned-analysis-worker tests, cancellation/cleanup checks, reference-source imports, bundle/Electron interop, semantic links and large/deep-source cases. - The public fixture runs real Node controls for loads before ordinary assignment, `var` initialization and destructuring, plus calls/writes in deferred functions and instance fields. - Final focused re-run after adding the reference-source before-write control: **2 passed** (`referenceSourceShadowedRequire.test.ts` and `javascriptStaticAnalysis.cooperative.test.ts`). - `npm run build:cached`, `npm run check:fast`, `npm run knip`, `npm run verify:architecture-guards`, `npm run verify:test-discovery`: passed; **1,090** test files discovered exactly once. Changed-file formatting passes. - `npm run check`: still stops on the existing formatting failures in unchanged `src/domain/functionComparison.test.ts` and `src/ida/IdaOperationRunner.ts`. Separate typecheck/lint, knip and architecture checks complete successfully. The deterministic responsiveness regression measures bounded identifier work at yielded traversal checkpoints rather than asserting a machine-dependent millisecond deadline. Existing scheduler cancellation and real worker lifecycle tests were also run. It does not establish a hard end-to-end cancellation bound. ### Cost and scheduling check Five serialized, alternating before/after runs compare `c05f7eb` with the review correction on the same 138,889-byte source containing 5,000 genuine loader calls. Each run uses a fresh Node process and the compiled production modules; both versions retain all 5,000 references. | Source-analysis measurement (median across five runs) | Before | After | | --- | ---: | ---: | | Total stepped analysis wall time | 109.40 ms | 106.21 ms | | Analysis CPU time | 238.75 ms | 238.51 ms | | Longest nonterminal iterator step | 20.24 ms | 4.24 ms | | Final iterator step | 19.37 ms | 17.11 ms | | Process peak RSS | 202,528 KiB | 197,824 KiB | Wall/CPU intervals exclude import and parsing setup; RSS includes the entire process. These are small source-analysis samples, not agent-token measurements, an end-to-end speedup or a general performance guarantee. The final step includes pre-existing synchronous result/Electron projection outside this require-classification correction; it is shown separately and remains a responsiveness boundary. The deterministic test bounds intermediate traversal work, not that final projection. ## Compatibility, safety, and release No breaking schema or installation change. macOS public CLI/MCP and real Node controls were verified; Windows/Linux and native providers were not run. No Hopper/Ghidra/IDA behavior is claimed. Fixture execution is test-only and uses self-authored sources. ## Before merge - [x] Concrete reproduced problem and linked issue. - [x] Scope and authority questions resolved for this ordinary fix. - [x] One focused outcome and Conventional Commit title. - [x] Regression plus positive and negative controls verified. - [x] Shared interpretation owner reused; superseded module removed. - [x] No public contract or generated metadata update required. - [x] Public output changes and verification limits recorded. - [x] Final diff checked for unrelated changes, secrets and unsupported claims. Prepared with AI assistance; all reported executions were run locally. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:06:56 +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#5475

No description provided.

Rows
Columns